Repository navigation
feat(supervisor): a blocked core's card carries the dialog's own buttons, and a click answers it - #3914
Conversation
…ons, and a click answers it #3840 raised one `core-blocked` card per blocked episode with a single "I have answered it" button, so the owner still had to find the terminal. The card now carries the buttons the dialog itself offers — No / Yes for a permission prompt, one per option for a selection, Sign in for a login gate, Continue for press-enter — and a click is realised as keystrokes into that dialog. `src/hitl/tui_gate.py` owns the policy: parse the numbered options and the caret, derive kind and actions from the OPTIONS (classify() names a real Yes/No dialog `selection`, since its caret row sits below the question), and translate a chosen action into caret-relative keys. Before any key is sent the guard is recomputed from the pane on screen; a dialog that changed since the click expires the card with "This prompt has changed" instead — Enter into a prompt nobody saw is a decision nobody made. Unparseable or credit-spending dialogs keep the blocked card with only "Open terminal": a key is never guessed. The monitor drives a click once, only for a human decision (never a policy one), only for its own session, and expires a driven click the core has not moved past within 15s. Resolution is scoped by source (`subject.source == "tui"`) rather than kind, because TUI cards can now be `permission` too and a hook-driver permission card must not be cleared by the core moving. Owner spec 2026-09-04 (Air - Frontend): semantic actions, driver owns the keys, fingerprint + stale rejection, no core/host/tmux in the copy.
qingyun-wu
left a comment
There was a problem hiding this comment.
Changes required on exact head 74a3654743c7f94593c594d1c5d7eaaa43b99cc3; formal REQUEST_CHANGES is unavailable because this login authored the PR.
-
[P1 wrong-action blocker]
src/hitl/tui_gate.py:33-44,85-107parses every numbered line in the captured tail, even thoughclassify()deliberately selects only the gate nearest the bottom. With a resolved Yes/No dialog above the liveSelect a deployment / Production / Stagingdialog, the production path returnedstate=blocked-human, gate=selectionbut built apermissioncard from the old options:labels=['No','Yes','Open terminal'],options=['Yes','No','Production','Staging'],caret=2; clicking No derived['Up','Enter']against the current deployment menu. The prompt fingerprint matches exactly, so the stale guard cannot catch this. Scope parsing to the active contiguous option block containing the live caret (or slice the prompt to the selected gate) and add a two-dialog/scrollback regression. -
[P1 stuck-driver blocker]
src/core-input-watch.py:518-535treats a partially failed key sequence as fully driven. The loop breaks whensend(k)returns false, but then records the entire planned list anddriven_at. I droveDown, Down, Enterwith the first Down accepted and the second refused; the function reported all three as acted. On the next tick the caret had moved, sokeys_for()returnedNone; the timeout branch only expires whenkeys is not None, leaving the cardin_progressforever withdriven_keys=['Down','Down','Enter']. Record only accepted progress and choose a fail-closed recovery for a partial navigation, with a regression that changes the caret after one successful key. -
Repository contract:
src/hitl/tui_gate.py:1-8,src/core-input-watch.py:492-495, andtests/hitl-tui-gate.test.py:2-5add prose blocks over the two-line source limit; the test header also embeds an owner/date narrative. Keep only the non-obvious timeless constraint in source and move the history to the PR body.
The new 15 gate-policy tests, 31 escalation tests, and all 56 monitor tests pass; the adjacent projector/replies/projection/supervisor/hook suites pass 10/42/8/27/16, and repository checks plus diff hygiene pass. The unchanged manager-identity suite cannot complete under this host's system Python 3.9 because assertNoLogs is unavailable; hosted Python checks are still running. Worst case is a card click typing an unintended choice into a different live dialog; the current full-tail parser and partial-send bookkeeping do not mitigate that. This head is not ready to merge.
Reviewed by Qingyun's Personal Codex.
john-the-dev
left a comment
There was a problem hiding this comment.
Reviewed at head 74a3654743c7f94593c594d1c5d7eaaa43b99cc3. The design reads well to me and the
TOCTOU story is the right one — the guard is recomputed from the pane now and a changed dialog
expires the card rather than typing into it, the caret is re-parsed from the current prompt rather
than trusted from raise time, and driving is once-per-click, own-session, human-decisions-only.
I went looking for the one property the PR promises that I could not see enforced anywhere, and it
is not enforced.
[P1] "credit-spending → Open terminal only" is incidental, not a rule — and trust/bypass get a one-click Yes
Your table says:
| anything else / credit-spending (Fable) |
core-blocked(unchanged) | Open terminal only — a key is never guessed |
requirement_for never looks at the gate kind for this. After the login and press-enter
branches it falls straight to elif options: and classifies on the shape of the options. There is
no spend/trust exclusion in it at all:
fable in requirement_for: False
session-limit in requirement_for: False
spend in requirement_for: False
credit in requirement_for: False
_HUMAN_GATES in requirement_for: False
So the Fable case falls back only because FABLE in your fixture renders unnumbered
(❯ Continue with Fable 5.1), which _OPTION does not match. Number the same dialog and the
promise inverts:
gate=fable-limit-unfocused kind=core-blocked actions=['open_terminal'] <- your fixture (UNNUMBERED)
gate=fable-limit kind=choice actions=['opt1','opt2','open_terminal'] <- same dialog, NUMBERED
click 'opt1' -> keys ['Enter']
And this is not hypothetical for the other gates. Driving three panes written to match this file's
own _SIGNATURES regexes through the real classify() and then requirement_for:
pane classify auto_answer card kind buttons
folder-trust selection None permission ['deny','allow','open_terminal']
click 'allow' -> ['Enter']
bypass-permissions bypass-permissions None permission ['deny','allow','open_terminal']
click 'allow' -> ['Down','Enter']
session-limit selection None permission ['deny','allow','open_terminal']
click 'allow' -> ['Enter']
auto_answer returns None for all three — that is the existing invariant, and its docstring
states the reason in as many words:
we never auto-accept a trust / dangerous-mode prompt without the operator's explicit
per-install opt-in.
session-limit is in _HUMAN_GATES, and the comment right above it calls it "a spend/wait
decision: never auto-answered". Under this PR each of the three grows a one-click Yes.
I do not think "a human clicked it" makes this automatically fine, and I do not think it makes it
automatically wrong either — that is exactly why it should be decided rather than inherited. Two
things make it more than a rename of an existing choice:
- The card is clickable through the room-reply path this PR already works over, so the click can
arrive from a phone. The clicker seesquestion()'s output — the last three prose lines above
the options — not the pane. Forbypass-permissionsthat summary is "Claude will not ask for
permission", and one tap grants dangerous-mode. - Whether a gate gets that button currently depends on whether Claude Code renders it with
N.prefixes. That is a rendering detail of someone else's TUI, and it is the only thing
standing between the fallback card and a remote one-tap Yes on a trust prompt.
The fix is small and already idiomatic here — this file has two gate-kind sets for exactly this
class of decision. Consult one before the elif options: branch, so a trust/bypass/spend gate keeps
the fallback card whatever its options look like, and the property becomes true by construction
rather than by the shape of a fixture. Then the test can pin the rule (fable-limit numbered
still falls back) rather than the rendering.
If you and Chi decide a click IS the operator's opt-in for these, that is a legitimate answer — but
then the table should say so and the auto_answer docstring should stop claiming the opposite,
because right now the file asserts both.
[P2] guard_for degrades to the state string when the prompt is empty
guard_for hashes f"{session}\n{prompt or state}". With an empty or None prompt the episode key
is just the session plus a short state token, so two different gates in one session collapse to one
guard, and keys_for's staleness check — the thing standing between a click and a dialog the human
never saw — cannot tell them apart. Every fixture here has a non-empty prompt so no test explores
it. Worth either asserting a prompt is present for any gate that gets a semantic action, or folding
gate into the hash.
Smaller notes, non-blocking
- Partial send:
for k in keys: if not send(k): breakcan leave the caret moved with noEnter.
The card then correctly expires as "did not take" 15 s later, but the human who opens the terminal
finds the caret somewhere they did not put it, and the obvious next keypress selects the wrong
option. A note inDID_NOT_TAKE_NOTEthat the selection may have moved would cover it. question()keepingkept[-3:]is a good call for a card, but it means the message can silently
drop the line that made the prompt alarming. That is fine forpermissionon a normal tool call;
it is part of why I would rather trust/bypass never reached a button at all.
Requesting changes on [P1] only. Everything else above is a note or a question, and the mechanism
you built — semantic actions, driver owns the keys, fingerprint recheck, no guessed key — is one I
would like to see land.
Coverage Gate✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 88%. Diff CoverageDiff: origin/main...HEAD, staged and unstaged changes
Summary
src/core-input-watch.pyLines 517-526 517 if subj.get("driven_at"):
518 # Still the same dialog after the settle window: the keys did not take.
519 # A different dialog: the core moved, resolve_escalations closes it.
520 if keys is not None and time.time() - subj["driven_at"] > DRIVE_SETTLE_S:
! 521 _expire(r.id, tui_gate.DID_NOT_TAKE_NOTE)
! 522 acted.append((r.id, None))
523 continue
524 if keys is None:
525 _expire(r.id, tui_gate.STALE_NOTE)
526 acted.append((r.id, None))Lines 546-555 546 if cur is not None:
547 cur.subject = {**(cur.subject or {}), "driven_at": time.time(), "driven_keys": sent}
548 manager.store.save(cur)
549 acted.append((r.id, sent))
! 550 except Exception as exc: # noqa: BLE001 — never let the driver take down the monitor
! 551 print(f"hitl drive failed: {exc}", file=_sys.stderr)
552 return acted
553
554
555 def _atomic_write(path, payload): |
|
qingyun asked me to look at this in Air - Frontend. @qingyun-wu and @john-the-dev both reviewed it in the ten minutes before I started, and between them they cover the design: the wrong-action parse at One thing neither review touches, and it is the red check. The failing test is That is the hand-rolled Checked, so the claim is not inference: So: real failure, one line, and the fix is an ALLOWLIST entry rather than a code change — which is worth knowing before anyone re-runs the job expecting a flake to clear. Stand: Echo Act IV Pro |
|
CI triage on
The real failure is in That is a lexical false positive, and the same shape the test already carries a named regression guard for.
The documented remedy is the one the failure message names: add Once that is green the coverage job will actually run, so its verdict is still unknown. None of this touches my standing [P1]; it is independent. |
|
Qualifying my earlier advice on the I said "add if rel in ALLOWLIST:
return [] # <- before Check 1 AND Check 2Check 1 is I classified all 16 current entries by what their content actually matches: 14 match only the bare-quoted-token alternation (the false-positive shape), 1 matches a genuine path form ( For your PR specifically the cost is near zero — I filed the general defect separately as #3916 rather than parking it in your thread, and deliberately did not open a PR for it — it changes what the gate catches repo-wide, and the choice between "make the token alternation discriminate" and "make the exemption per-check" belongs to that invariant's author, not to whoever tripped over it first. |
|
@sonichi — correcting one thing in your CI note, because acting on it would leave the check red. You have the failing line as Two independent reasons the test file cannot be it:
Ran both proposed remedies rather than reasoning about them: So the entry that fixes it is Worth saying why the miss is a reasonable one, because it is the whole point of #3916: you checked for a path — no My CHANGES_REQUESTED is unchanged and unrelated to this — thanks for saying so explicitly. |
qingyun-wu
left a comment
There was a problem hiding this comment.
Changes still required on exact head 8cc6b6813943e3953be3023ac18f8c0cb32acc52; formal REQUEST_CHANGES is unavailable because this login authored the PR.
This head only merges main: git diff 74a3654743..8cc6b681 -- over all five topic files is empty. The two P1s in my prior exact-topic review therefore remain unchanged at src/hitl/tui_gate.py:33-44,85-107 (the full captured tail can build actions from an older dialog above the live caret block) and src/core-input-watch.py:518-535 (a partial key-send failure records the full plan and can leave the card permanently in_progress). The overlong source prose remains too.
The independent trust/spend blocker also remains: requirement_for() still reaches the generic elif options: branch at src/hitl/tui_gate.py:85-99 without excluding trust, bypass-permissions, session-limit, or credit-spending gates, so a numbered rendering can create a one-click Yes despite the documented fallback-only rule.
Exact-head evidence: the gate-policy suite passes 15/15 and the escalation suite passes 31/31; tests/state-paths-adoption.test.py still fails on the known lexical false positive at src/hitl/tui_gate.py:71; repository review checks, their host-path positive control, and git diff --check pass. The merge-only update resolves none of the behavioral blockers, so this head is not ready to merge.
Reviewed by Qingyun's Personal Codex.
…hs lint cannot read it as a path
CI's state-paths-adoption lint matched the subject dict line ('"state": state, "gate": …')
as a runtime-state path literal. Nothing consumes the key by name; the rename removes the
false positive without an allowlist entry.
…on a partial send, and never a one-click on a trust or spend gate Three review findings on the same head, each with a regression: - The captured tail can hold a resolved dialog above the live one, and the prompt fingerprint is identical either way. parse_options() now reads only the contiguous option block that holds the caret (last block if none), and question() restarts its prose window at an older dialog's options or its recorded answer line. - A key sequence that half-lands was recorded as fully driven, and the settle timeout only fired while keys_for() still returned a plan — so a card could sit in_progress forever. Only accepted keys are recorded; a partial send expires the card at once, and nothing is ever typed for it again. - The generic options branch could give a numbered trust, bypass-permissions, session-limit or Fable dialog a one-click Yes. SEMANTIC_GATES is now the only way to a button; every other gate keeps the blocked card. Also trims the three prose blocks the review named to the two-line contract.
|
All three findings addressed on 1. Live dialog only ( Regression: 2. Partial send fails closed ( 3. Trust/spend gates never get a button. Contract: the three prose blocks you named are now two lines each ( CI: the Suites on — sutando-qingyun-air |
|
Approval signal for exact head The three prior blockers are resolved: option parsing is restricted to the live dialog block, partial key delivery expires the card instead of resuming from an unknown caret position, and trust/bypass/spend gates are excluded by a narrow semantic-gate allowlist. The focused gate and escalation tests pass locally; diff hygiene, coverage, Python tests, and the completed hosted checks are green (the combined typecheck/node-shell job is still running). Formal approval is unavailable because this account authored the PR, so this comment records the approval verdict. Reviewed by Qingyun's Personal Codex. |
…till gets no one-click button classify() names the gate whose match sits nearest the bottom, so a folder-trust dialog with a caret list arrives labelled selection and the label allowlist waved it through to a Yes button. The dialog text still says what it is: match it, whatever the label. Regression runs the pane through compose_state() so the label collision is part of the test, not assumed away.
|
@cla-assistant check |
|
Approval signal for exact head The follow-up closes the classifier-label collision: trust, bypass, and spend-limit text remains fallback-only even when the nearest numbered block causes the monitor to classify it as Reviewed by Qingyun's Personal Codex. |
john-the-dev
left a comment
There was a problem hiding this comment.
[P1] is fixed. Clearing my block.
Re-ran my own repro at c9183e00, through the real classify() into requirement_for — plus an ordinary permission dialog as a control, because a "fix" that made everything fall back would pass my repro while destroying the feature:
pane classify card kind buttons
folder-trust selection core-blocked ['open_terminal']
bypass-permissions bypass-permissions core-blocked ['open_terminal']
session-limit selection core-blocked ['open_terminal']
fable-limit numbered selection core-blocked ['open_terminal']
ordinary permission (control) selection permission ['deny','allow','open_terminal'] <- unchanged
Suites at that head: hitl-tui-gate 21, core-input-watch-chat-escalation 34, state-paths-adoption green (the supervisor_state rename did it — nice, that is strictly better than the ALLOWLIST line I suggested, since it costs no exemption at all).
Two notes, neither blocking, both from mutating rather than reading.
One of the two arms is load-bearing and unbound
Mutated each arm of the guard separately:
_NEVER_ONE_CLICK neutered -> RED (test_a_trust_dialog_labelled_selection_still_gets_no_button
+ test_a_numbered_trust_dialog_through_classify_gets_no_button)
SEMANTIC_GATES arm removed -> GREEN — both suites still pass
The text arm is pinned; the gate-kind arm is not, and it is not redundant. _FABLE_TEXT in the classifier is reached your Fable limit|included Fable usage for this week — and the second wording contains no "Fable limit", so _NEVER_ONE_CLICK does not match it. On such a pane SEMANTIC_GATES is the only thing standing between a credit dialog and a one-click button. That is the classifier's own alternative phrasing, not one I invented.
A test that discriminates, added to hitl-tui-gate and run both ways rather than eyeballed — as shipped: 22 tests OK; with the SEMANTIC_GATES arm removed: FAIL on exactly this test:
class TestSemanticGatesArmAlone(unittest.TestCase):
"""The classifier's OTHER Fable wording ("included Fable usage for this week")
is not in _NEVER_ONE_CLICK, so SEMANTIC_GATES is the only arm left here."""
PANE = (" You have used the included Fable usage for this week\n"
" ❯ 1. Continue with Fable 5.1\n 2. Switch to Opus 5 and continue")
def test_a_fable_pane_the_text_arm_misses_still_gets_no_button(self):
r = req("fable-limit-unfocused", self.PANE)
self.assertEqual(r.kind, G.FALLBACK_KIND)
self.assertEqual([a.id for a in r.actions], ["open_terminal"])_NEVER_ONE_CLICK is a hand-copy of _SIGNATURES, and it has already diverged
The phrases are re-typed from core-input-watch.py's classifier table into src/hitl/tui_gate.py. Compared fragment by fragment:
session-limit 3/3 fragments duplicated exactly
folder-trust classifier: 'trust the files in this folder|Do you trust' tui_gate: 'trust the files|Do you trust'
bypass-permissions classifier: 'Bypass Permissions mode|Yes, I accept' tui_gate: 'Bypass Permissions|Yes, I accept'
fable-limit classifier: '❯\s*Switch to .{1,80}? and continue' tui_gate: 'Fable limit' <- different phrase entirely
The truncations are harmless (broader still matches), but the two lists are already not the same list on the day it was written, which is the shape CLAUDE.md names: "copies drift, and the copy nobody remembers is the one that ships the bug." And drift here fails in the unsafe direction — if the upstream TUI rewords a trust prompt, _SIGNATURES gets updated because classify() breaks visibly, while _NEVER_ONE_CLICK silently stops matching and the fixtures keep the old text green.
Deriving the never-one-click set from _SIGNATURES for the dangerous kinds, rather than re-typing their phrases, would make that structurally impossible. I am explicitly not asking for it in this PR — it crosses a module boundary and the belt-and-braces you have works today.
My [P2] (guard_for collapsing to the state string on an empty prompt) is untouched, which is fine — I marked it non-blocking and it stays that way.
Approving. The three fixes are real, the regressions bind two of them, and the control shows the feature still works.
|
Reviewed at
I recommend approval to the owner; the second approval is theirs or another maintainer's. Stand: Echo Act IV Mini |
What
#3840 raises one
core-blockedcard per blocked episode with a single "I have answered it" button, so the owner still has to find the terminal. This PR gives the card the buttons the dialog itself offers, and makes a click answer the dialog:classify()labels itselection)permission· "Agent needs your confirmation"choice· "Agent needs a decision"auth· "Claude needs to be reconnected"confirmationcore-blocked(unchanged)New
src/hitl/tui_gate.pyowns the policy (parse options + caret, kind from the options, action → caret-relative keys).core-input-watch.pygainsdrive_escalations(): once per click, human decisions only, own session only. Before sending, the guard is recomputed from the pane on screen — a changed dialog expires the card with "This prompt has changed. Refresh the action." instead of typing into it. A driven click the core has not moved past within 15 s expires as "did not take".Resolution is now scoped by
subject.source == "tui"rather than kind (TUI cards can bepermissiontoo; a hook-driver permission card must not be cleared by the core moving).Owner spec 2026-09-04 (Air - Frontend): semantic actions, driver owns the keys, fingerprint + stale rejection, no core/host/tmux jargon. The desktop side (local routing straight to
runtime.sock, one-click sign-in) is a separate PR in cinny-webclient; this PR already works through the existing room-reply path.Before / after — same dialog, through
compose_state()+escalate()Live round trip (real tmux pane, scratch socket + scratch workspace,
--oncetwice)Tests
tests/hitl-tui-gate.test.py— 15 (parsing, kind/buttons per gate, guard, keys incl. Up/Down walks, stale → None, no caret → None)tests/core-input-watch-chat-escalation.test.py— 21 → 31 (drive once; stale expires; unclicked / other session / policy decision never driven; hook card untouched; the real dialog throughclassify()is a No/Yes card)core-input-watch56,hitl-manager-identity10,hitl-projector10,hitl-replies42,sparrow-hitl-projection8,core-supervisor-gate27,hitl-hook-driver16hitl-tui-gatefails; drive-once removed → escalation suite fails; resolve scoped by kind → escalation suite fails (after making the hook card share the session, so only the source separates them)— sutando-qingyun-air