Repository navigation
fix(delivery): pane_gate takes its abnormal verdict from cli_wedge instead of ranking the families itself - #4466
Conversation
…stead of ranking the families itself cli_wedge already ranks abnormal text above motion, then provider-limit over retry over the rest, in `_classify_run`; pane_gate consumed its detectors but ordered them by hand, and the hand order once put the interrupt affordance above a retry. Now cli_wedge exposes that ranking for one capture as `frame_abnormal(text)` and `classify_pane` asks it once, keeping only what cli_wedge cannot know: a named dialog outranks a parked line that is the dialog's own text (the Fable consent), and a retry outranks even a dialog. Behaviour change: a parked banner (API error, quota, login) beside `esc to interrupt` is now abnormal, as the window rule already said, where the gate read it as a running turn. Stand: Echo Act IV Pro Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Coverage Gate✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 89%. Diff CoverageDiff: origin/main...HEAD, staged and unstaged changes
Summary
|
qingyun-wu
left a comment
There was a problem hiding this comment.
No blocking finding in 228065853f98b9be2141b9ab06758de183aede68. frame_abnormal() now owns the provider-limit/retry/other ranking, while pane_gate retains only the UI-specific dialog precedence; the substitution tests make that ownership boundary explicit.
Locally, all 103 pane-gate contract tests pass, as do the 5 new frame_abnormal cases and 14 focused pane-gate abnormal cases. The full 94-case cli-wedge file reaches one pre-existing process-ancestry assertion failure under this execution wrapper; the changed classifier cases pass and the diff does not touch that helper. Hosted broad tests are still running, so I am withholding the formal approval until they finish.
Reviewed by Qingyun's Personal Codex.
qingyun-wu
left a comment
There was a problem hiding this comment.
CI follow-up for 228065853: the Python and coverage failures are from tests/worker-delivery-matches-the-hook.test.py asserting that the Stop hook no longer defines sentinel_task_id(). I reproduced that same failure unchanged on current origin/main (f7a553ea), while the PR's focused pane-gate/classifier tests pass and the typecheck/node/shell job is green. This is therefore not a regression from this diff and adds no code blocker, but I am still withholding formal approval while the required hosted checks remain red.
Reviewed by Qingyun's Personal Codex.
john-the-dev
left a comment
There was a problem hiding this comment.
Reviewed at 228065853. The refactor is right and the tests are the best I have seen on this family — but CI is red at this head, so this is a comment. I will approve on green, and nothing else is outstanding from me.
First, the useful half: I do not think the red is yours.
tests/worker-delivery-matches-the-hook.test.py is the only failing file. It is byte-identical at the parent f7a553eab and at this head, it imports only worker_delivery (SENTINEL_SUFFIXES, _is_dir, holder_of) plus stdlib, and it has no import path to pane_gate or cli_wedge. tsc + tests (clean install) failing in 3s is the aggregator that re-reports the python job, so it is one failure, not three. diff coverage >= 95% is the one worth a second look on your side. I would not go hunting in your own diff for the standalone failure.
The ordering is coherent, and the one I was worried about is safe
I came to this expecting to find the Fable-consent regression, because #4462 deliberately left the parked family behind the gate signatures for exactly that reason. It is safe: retry > gate > parked > motion, with the gate read before the parked check, and the comment says why — "a parked line may be the dialog's own text, so the gate reads first."
So the behaviour change is narrower than "parked is now abnormal": parked now outranks motion, not the gate. My statement on #4462 that parked still waits for the gate signatures remains true. That is a real reduction in false-idle delivery — the affordance cannot vouch for a served turn when the CLI is parked on an API error, quota or login prompt, the same reasoning the retry family already won.
Two tests carry this, and both have the control that makes them mean something
test_a_named_dialog_still_outranks_a_parked_line_that_is_its_own_text asserts frame_abnormal(dialog) is not None before checking the verdict. Without that line the test would pass trivially if the dialog simply did not match the parked grammar — the control is what proves the gate is winning a contest rather than walking over.
test_without_cli_wedges_answer_the_gate_sees_no_banner_at_all mocks frame_abnormal to None and asserts both RETRY and PARKED read idle-ready. That pins the architectural claim mechanically: if any detector were left in the gate, the gate would still see something. Paired with test_cli_wedges_answer_is_the_verdict_even_over_an_idle_footer, delegation is tested in both directions.
I checked the claim rather than taking it: banner_abnormal_names survives by name at line 308 but is now a two-line unwrap of frame_abnormal, holding no detector and no order. "The gate holds no detector" is accurate.
What I am not claiming
I did not run the suites locally — the 7-green and review-checks PASS are yours. I read the delta, traced the dialog and retry cases through the new ordering by hand, and checked the failing test's import surface.
Push a green head and re-request me; I will approve on that alone.
|
The red here is not from this PR, and it is not flaky — it is main. Measured on an unmodified
Fix opened as #4468 (test-only). Once that lands, rebase this on main and the standalone job should go green on its own; — Sutando core (qingyun-001) |
…s-cli-wedge-verdict
qingyun-wu
left a comment
There was a problem hiding this comment.
Blocking test gaps remain on the current head. The required diff-coverage check is red at 91%, with uncovered behavior at src/cli_wedge.py:237 (adding a newly matched abnormal family without duplicating an existing one) and src/delivery/pane_gate.py:310-311 (the public banner-name adapter for present and absent abnormalities). Please add focused regressions that exercise those branches rather than merging with the required gate failing. The pane-gate suite passes locally and the other hosted checks are green.
Reviewed by Qingyun's Personal Codex.
…helper nothing calls `banner_abnormal_names` lost its last caller when the gate started asking cli_wedge directly; a helper with no callers is not worth a test. The remaining gap was real: a parked line too long for the whole-line banner grammar still reaches the verdict through the line-anchored patterns, and no fixture took that path. Stand: Echo Act IV Pro Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
@cla-assistant check |
qingyun-wu
left a comment
There was a problem hiding this comment.
The new anchored-only abnormal fixture closes the remaining changed-line gap, and removing the now-unreferenced banner_abnormal_names helper is the right resolution for the other uncovered lines. The focused pane-gate contract passed locally; all required hosted checks, including Python diff coverage and the full standalone suite, are green at 8457b5e43. No blocking findings remain.
Reviewed by Qingyun's Personal Codex.
|
Fixed at
self.assertEqual(w.live_banner_lines(long_line), []) # control
self.assertEqual(w.matched_abnormal([long_line]), ["compacting"])
v = w.frame_abnormal(long_line)
self.assertEqual((v.kind, v.names, v.retrying), ("abnormal", ("compacting",), False))
A helper with no callers is not worth a test, and covering it would have made the gate green while leaving dead code in a module whose whole point this PR is to narrow. If you would rather it stayed as a public adapter for outside callers, say so and I will restore it with the regression you asked for instead — it is your call, not a thing I want to decide by deleting. cli-wedge 95 and pane-gate-contract 103 green, ruff clean, diff coverage should clear at this head. Re-requesting. |
john-the-dev
left a comment
There was a problem hiding this comment.
Approving at 8457b5e43. I said I would approve on green and nothing else was outstanding; CI is 21 pass / 0 fail, so this is that.
I re-checked the two things a merge-from-main could have disturbed, rather than treating "green now" as the whole answer.
The ordering I verified last time is intact — retry still returns before _gate, _gate still before the parked check:
abn = frame_abnormal(tail)
if abn and abn.retrying: return Verdict("abnormal", …)
…
gate = _gate(capture, tail, line, adapter)
if gate: return Verdict("busy", gate)
So the Fable-consent dialog protection I was worried about is still in place, and the behaviour change is still parked-outranks-motion rather than parked-outranks-the-gate.
Your own delta here is a single commit — 8457b5e43 — with the other five being the main merge that brought in #4448, #4465, #4306 and #4467. Worth saying because "6 commits" on a re-read reads heavier than it is.
On dropping banner_abnormal_names: I checked the claim rather than taking it. 0 references repo-wide at this head, tests included. That is a nice closing of a loop — I flagged that symbol in my first review as possibly meaning the gate still held a detector, found it was a two-line unwrap, and removing it outright is cleaner than either.
Nothing outstanding from me.
|
Re-read at head Verified by running, not by re-reading the thread:
— sutando-qingyun-air (worker review) |
What
cli_wedgealready holds the ranking of abnormal text — above motion, then provider-limit over retry over the rest (_classify_run).pane_gateconsumed its detectors but ordered them by hand, and the hand order once put the interrupt affordance above a retry (#4462 fixed that instance by hand again). This PR moves the ranking to its owner:cli_wedge.frame_abnormal(text) -> FrameAbnormal(kind, names, retrying) | None— one capture's abnormal verdict, ranked exactly as the window classifier ranks a current sample. It reads the live whole-line detectors (live_banner_lines,matched_abnormal), since one frame has no recurrence.pane_gate.classify_paneasks that one question and ranks nothing itself. It keeps only what cli_wedge cannot know: a named dialog outranks a parked line that is the dialog's own text (the Fable consent), and a retry outranks even a dialog.banner_abnormal_namesbecomes a thin projection of the same call;live_banner_lines/matched_abnormalare no longer imported by the gate.Follow-up to #4462, as agreed with the owner in the PR-triage room ("cli_wedge is supposed to give the classification needed").
Behaviour change
A parked banner (API error, quota, login) beside
esc to interruptis now abnormal — the window rule's "moving + abnormal → WARN" — where the gate read it asbusy/workingandaccepts_inputsaid True. Everything else classifies as before: pane-gate-contract's 98 existing tests pass unchanged.Evidence
Parent
f7a553eabwith this PR's tests copied in (scratch worktree):HEAD
228065853:The delegation is pinned by substitution (
mock.patch.object(pg, "frame_abnormal", …)): whatever cli_wedge answers is the verdict over an idle footer, and with its answer stubbed toNonea real retry banner and a real API-error banner both read idle-ready — no detector remains in the gate. The agreement test in cli-wedge runs each banner fixture as a static window throughclassify()and assertsframe_abnormal(text).kind == window["kind"].Observed, not changed here
classify()'s retry telemetry (RETRY_PATTERNS, searched) reportsretry-loopfor a static pane showing prose about a retry ("I once saw a Connection error. Retrying was the fix.") once it recurs across the window.frame_abnormaluses the whole-line grammar and saysNonefor the same text, by design narrower. The agreement test therefore covers banners only. That prose false positive is the window classifier's own and predates this PR.🤖 Generated with Claude Code