fix(coding-agent): harden queued prompts and macOS Option+Q - #4182
Conversation
Foreground bash and eval correctly blocked auto-continuation, but a follow-up queued during that fence had no completion trigger once the process settled. Resume the canonical queue only after the final execution lease clears, with regression coverage for eval completion. Tested: bun test test/issue-825-repro.test.ts test/agent-session-queued-prompts.test.ts Constraint: retain ordered follow-up delivery without concurrent model turns
The macOS verification gate only understood iTerm2 profiles, so normal Terminal.app sessions were rejected or left without an actionable check. Validate Terminal.app default and startup profiles for Use Option as Meta key, retain the iTerm2 path, and document the required profile setting. Constraint: Option shortcuts require the terminal to preserve the modifier as Meta/Esc. Confidence: high Scope-risk: low Reversibility: straightforward Tested: Terminal.app profile verification, failure diagnostic, 102 focused keybinding tests, gjc smoke test Not-tested: manual hardware keypress without synthesized CGEvent
Resolve the current dev conflicts while retaining the Terminal.app Option+Q verifier and the upstream queued-continuation behavior. Confidence: high Scope-risk: medium Tested: conflict-free tree and focused queue/keybinding suite
|
All required PR checks are green. The Terminal.app verifier passes with |
Yeachan-Heo
left a comment
There was a problem hiding this comment.
Adversarial review — exact head d033040db0abb21179033e93be761ad1e78da797 (base 7858b0ff63db)
Verdict: MERGE_READY (review-only; no source mutation, push, merge, or release performed).
What this PR actually is
The full PR diff against base touches exactly 3 files (verified locally via git diff on the exact head and via the GitHub files API): scripts/verify-option-key.sh, docs/keybindings.md, packages/coding-agent/CHANGELOG.md. Zero source files, zero test files, zero schema/gate files. The queued-prompt hardening described in the title/body (input-controller.ts, agent-session.ts, ui-helpers.ts, packages/agent/agent.ts, and the five listed test files) all landed in base 7858b0ff — those commits (c63e8975, 7088467a, 1dd7675d) are in the merge base, not in this PR's patch. I verified the 195-test inventory exists in the base tree and passes there; the PR merge commit only resolves the Terminal.app verifier on top.
Exact-head verification (run locally at head d033040d, Linux x64)
- Focused suite (the 5 files the PR cites):
104 pass, 0 fail(agent-session-queued-prompts,input-controller-keybindings,input-controller-skill-queue,issue-825-repro,keybindings-display). First run needed the natives addon built (bun run build:native) because this checkout had nonode_modules/.node; afterbun install+ native build, all 104 pass with 0 failures. No test in the set asserts anything the PR changes. bun --cwd=packages/coding-agent run check: exit 0 (biome checkclean on 2660 files +tsc --noEmit).gjc --smoke-test:smoke-test: ok.- Hosted CI at exact head: terminal — 27 check runs, 21 success, 6 skipped, 0 failed, 0 pending (includes
gjc-state-gatesintegrity/static/read/runtime,Affected path validation,Telegram daemon generation guard,Local public surfaces). Skipped runs are path-gated/Windows/macOS-only jobs; no head-dependent job is pending or failed.
Adversarial attacks against the user-facing contract
- Queue loss/duplication during compaction/Bash/Python/agent tail — not applicable to this PR: all of that logic is in base and unchanged by the diff. Reviewed it anyway:
#queueFollowUp→agent.followUppush +#scheduleQueuedFollowUpContinuationwith#canAutoContinueForFollowUpgating onisCompacting/isBashRunning/isEvalRunning/isRetrying; delivery dequeuesforceOneAtATime-ordered messages;flushCompactionQueuerestores the queue tocompactionQueuedMessageson failure and re-drains. No path duplicates or drops a queued message. - Stale continuation — the new
agent.continueQueuedMessagespath only runs when the tail is NOT assistant (startsQueuedSuccessorfalse) and is preceded by#stripOverflowFailedTurnForRetry-style checks; a queued drain from a user/toolResult tail uses the exact-tail branch to avoid a stale-turn replay. Tested byissue-825-repro+agent-session-queued-prompts(7+7 pass). - Busy-state races —
#scheduleAgentContinuere-checksshouldContinueunder the scheduled task and skips (queue_drained/aborted_signal) rather than double-running;flushCompactionQueue's 50 ms retry loop is guarded by#compactionFlushRetryScheduledso only one drainer exists. - Terminal/layout false claims — the verifier is honest and fails closed:
check_terminalrequiresWindow Settings[<default/startup profile>].useOptionAsMetaKey ∈ {true,1}(Terminal.app),check_itermrequires both Option keys+Escand no Option+Q/I Keyboard Map overrides, and the separate HIToolbox block hard-requiresAppleCurrentKeyboardLayoutInputSourceID == com.apple.keylayout.ABCAND an ABCAppleSelectedInputSourcesentry. No claim that Option+Q works for non-ABC layouts or for terminals that don't forward Option — the script exits 1 otherwise, and the docs now state the Option-as-Meta requirement instead of asserting unconditional Option+Q. - Breaking existing keybindings — none: this PR does not touch
config/keybindings.tsor the default key map;alt+q(darwin/win32) /alt+enter(linux) defaults are unchanged in base and at head. The docs edit only qualifies the macOS claim. - Docs-contract mismatch — the two docs hunks are consistent with the verifier's contract: "when the active terminal profile forwards Option as Meta/Esc; in Apple Terminal, controlled by Use Option as Meta key" matches
check_terminalexactly; the "ABC" requirement is documented in the PR body/verifier and surfaced in the fail output ("active keyboard layout is not ABC"). No overclaim found. - Dev-blocker overlap — diff is disjoint from current dev (
session/hardening, CI topology, sdk fixes); the merge commit is conflict-free against7858b0ffand CI is green on it.
Only note (not blocking)
docs/keybindings.md and the script refer to "Settings > Profiles > Keyboard > Use Option as Meta key" while the plist key is useOptionAsMetaKey — the label matches Apple's UI, so the diagnostic is actionable as written. Cosmetic only.
Evidence bounds
- Terminal.app/iTerm2 profile checks are macOS-only; on this Linux runner I validated the plist logic by reading the script at head (no macOS host available). Hosted CI shows the macOS gated jobs skipped (path-gated), so no hosted macOS run of the script exists for this head.
- "195 tests" as cited = the 5-file inventory at base; at head those files contain 104 test cases across 5 files (the number counts differ by how parameterized cases are counted). Either way: all pass at both base and head on my run.
Verdict: MERGE_READY
—
[repo owner's gaebal-gajae (clawdbot) 🦞]
Terminal.app sends Option+Arrow as an ESC-wrapped CSI sequence when Option is configured as Meta. The input buffer previously split the prefix, so queued-message selection could not see alt+up/down. Preserve the wrapped sequence and decode it in the native matcher so selector restore, delete, and reorder flows receive the intended shortcut. Constraint: preserve existing CSI modifier keybindings Confidence: high Scope-risk: medium Reversibility: easy Tested: cargo fmt, pi-natives key tests, focused TUI and coding-agent tests, package checks, CLI smoke test
|
Follow-up fix in commit Verified with actual Terminal.app capture ( |
probepark
left a comment
There was a problem hiding this comment.
Stale-review refresh — head 8a0e04ca9560b8ba5eeb68e9aba4a8b363af2309
I read the prior review submitted against d033040db0abb21179033e93be761ad1e78da797 and concentrated on the sole later commit, 8a0e04ca (fix(tui): decode Terminal.app Option arrow sequences).
Findings
No critical, major, or minor findings.
The new input path is coherent end to end:
packages/tui/src/stdin-buffer.ts:116-123keeps the outer Meta escape and inner CSI/SS3 sequence atomic, including chunked delivery.crates/pi-natives/src/keys.rs:489-499,572-581,1030-1037parses and matches the wrapped sequence while preserving the inner key's modifiers and adding canonicalalt.packages/coding-agent/src/modes/components/queued-message-selector.ts:95-129already consumes canonicalalt+up/alt+down, so the change reaches the observable queued-message selection behavior without a parallel path or compatibility shim.
The added tests exercise observable contracts rather than placeholders: framing across stdin chunks, parse/match symmetry, editor dispatch, and selector restoration. I found no added mock.module(), bare not.toThrow(), or tautological assertions.
Verification
cargo test -p pi-natives keys::tests: 15 passed, 0 failed.git diff --check d033040d..HEAD: clean.- Exact-head CI has passed the four changed focused test files, native build, coding-agent TS build, and root check. Some broader Rust/affected jobs were still pending when reviewed.
- The local Bun run could not validate the new native parsing because this checkout's installed
.nodepredates8a0e04ca; exact-head CI rebuilt the native addon and passed the affected tests.
gajae.pr-review-verdict.v1: needs-human
Yeachan-Heo
left a comment
There was a problem hiding this comment.
Adversarial delta re-review — exact head 8a0e04ca9560b8ba5eeb68e9aba4a8b363af2309 (base 7858b0ff63db, prior approved head d033040db0)
Verdict: MERGE_READY (review-only; no source mutation, push, merge, or release performed).
Delta under review
The sole new commit over the previously approved d033040db0 is 8a0e04ca (fix(tui): decode Terminal.app Option arrow sequences): 9 files, +74/−2. Core change is two-sided:
crates/pi-natives/src/keys.rs(+38):parse_meta_wrapped_keydecodesESC ESC <inner>by stripping the Meta marker and re-parsing the inner sequence withMOD_ALTadded;matches_key_innergains a permissive alt path;parse_key_innerinvokes the wrapper before the legacy/kitty branches.packages/tui/src/stdin-buffer.ts(+7):isCompleteSequencedelegatesESC ESC …completeness to the inner sequence so the pair is emitted atomically, including chunked delivery.- Tests + changelogs:
stdin-buffer.test.ts,keys.test.ts,custom-editor-keybindings.test.ts,input-controller-keybindings.test.ts, and the three package changelogs.
Cumulative integration: the queued-message selector (modes/components/queued-message-selector.ts:95-129) already consumed canonical alt+up/alt+down for restore/delete/reorder; this commit makes Terminal.app's actual Option+Arrow byte sequence reach that path. The Option+Q path (\x1bq) is untouched.
Findings
No critical, major, or minor findings.
Adversarial verification (exact head, Linux x64, native addon rebuilt from head source)
Focused suites — all green:
cargo test -p pi-natives keys(keys::tests): 17 passed, 0 failed.bun test packages/tui/test/stdin-buffer.test.ts packages/tui/test/keys.test.ts: 130 pass, 0 fail.bun test packages/coding-agent/test/custom-editor-keybindings.test.ts packages/coding-agent/test/input-controller-keybindings.test.ts: 88 pass, 0 fail.
Adversarial probes against the rebuilt native parser + StdinBuffer (both kitty states, both split and coalesced chunking):
- Bare Escape —
\x1bstill parsesescapeand matchesesc; flush after the 10 ms window unchanged. PASS. - Double/triple Escape —
\x1b\x1bis now held as incomplete until flush (previously emitted immediately as an unparseable sequence) and parsesalt+escape;\x1b\x1b\x1bstill cuts at 2 (ESC-cancel) then flushes\x1b— same observable sequence split as before the commit. No regression. PASS. - Partial/chunked input —
\x1b+\x1b+[A,\x1b\x1b[+A, and\x1b/\x1b/[Aat 3 ms gaps all assemble into one atomic\x1b\x1b[A→alt+up. PASS. - Alt+arrow vs literal ESC timing — with keystrokes ≥ 80 ms apart,
\x1bflushes asescbefore\x1b[Aarrives (esc, thenup) — identical to pre-commit behavior. Only when two keystrokes land within the 10 ms flush window do they coalesce intoalt+up; that is the same inherent ESC-prefix ambiguity the codebase already resolves for fastESC i→alt+i, so the policy is consistent and the practical window is sub-10 ms. Not a regression class; documented here for completeness. PASS. - CSI modifier compatibility —
\x1b[1;3Astill parsesalt+up(kitty on/off), matching the existing iTerm/Ghostty encoding; the wrapped path does not intercept it. PASS. - Kitty protocol — wrapped sequences parse identically with kitty active/inactive; kitty CSI-u (
\x1b[113;3u→alt+q) and release-event suppression are unchanged. PASS. - Unrelated alt keys —
\x1bq→alt+q,\x1bi→alt+i,\x1b\x1bq→alt+q,\x1b\x1bjstays one sequence; no collateral remap of plain letters. PASS. - Malformed sequences —
\x1b\x1b[and\x1b\x1b[2parsenulland remain buffered/flushed rather than crashing or misparsing. PASS. - Wrapped Option+Delete —
\x1b\x1b[3~→alt+deletein both kitty states, with the negative assertion that it no longer matches plaindelete. PASS. - Back-to-back wrapped arrows —
\x1b\x1b[A\x1b\x1b[Bemits exactly two sequences, parsedalt+up,alt+down, with no leftover remainder. PASS.
git diff --check d033040d..HEAD is clean; tracked worktree is clean at the end of the review (generated packages/natives/native/index.d.ts drift produced by the local native rebuild was restored to HEAD).
CI at exact head
Run 31408437506: status: completed, conclusion: success, head_sha: 8a0e04ca. Check-runs at the exact head: 25 success, 5 skipped (path-gated/macOS/Windows), 0 failed, 0 pending.
Evidence bounds
- Terminal.app byte behavior (
ESC ESC [ Awith "Use Option as Meta key") is validated through the wire-format analysis and the native parser/matcher tests; no macOS host is available on this runner, and the macOS path-gated hosted jobs are skipped. - The inherent
ESC ESCambiguity (Meta-wrapped Option+Arrow vs. two rapid keypresses) is byte-level unavoidable in legacy encoding; the fix follows the codebase's existing meta-prefix policy and adds no new failure mode beyond the sub-10 ms coalescing window.
Verdict: MERGE_READY
—
[repo owner's gaebal-gajae (clawdbot) 🦞]
Terminal.app exposes one Use Option as Meta key profile setting for both the left and right Option keys. Make that contract explicit in the parser, verifier, capture helper, documentation, and regression coverage so either key follows the same Meta/Esc path for queued-message controls. Constraint: preserve the existing terminal-independent alt key IDs Confidence: high Scope-risk: low Reversibility: easy Tested: Rust keys, focused TUI/coding-agent tests, package checks, shell/Python syntax
|
Follow-up commit Local verification: Rust keys 17 pass; focused TUI/coding-agent tests 220 pass; both package checks pass; shell/Python syntax and smoke checks pass. The live verifier confirmed |
Yeachan-Heo
left a comment
There was a problem hiding this comment.
Adversarial delta re-review — exact head bc6f58abb17b131dd91c813d1525d8c140082861 (base = prior approved head 8a0e04ca9560b8ba5eeb68e9aba4a8b363af2309)
Verdict: MERGE_READY (review-only; no source mutation, push, merge, or release performed). This approval explicitly supersedes the stale 8a0e04ca approval and applies to the exact current head bc6f58ab.
Delta under review
The sole new commit over the previously approved 8a0e04ca is bc6f58ab (fix(macOS): cover both physical Option keys): 7 files, +21/−9. No functional code changes — every edit is a comment, doc, changelog, print-string, or additive test:
crates/pi-natives/src/keys.rs— comment onparse_meta_wrapped_keyonly; parser logic untouched.packages/tui/src/stdin-buffer.ts— comment onisCompleteSequenceonly; completeness logic untouched.docs/keybindings.md,packages/coding-agent/CHANGELOG.md— prose.scripts/capture-option-key.py,scripts/verify-option-key.sh— print strings only.packages/tui/test/keys.test.ts— additivedescribe.each(["left","right"])regression test.
Findings
No critical, major, or minor findings.
Criterion-by-criterion verification
-
Byte streams cannot distinguish physical left/right Option — PASS. Terminal.app exposes a single
Use Option as Meta keyprofile setting that drives both physical Option keys; the wire bytes are identical (ESC ESC [ Afor either key). The parser decodes the outer ESC as a Meta marker with no left/right dimension, and no code change introduces one. The new test feeds identical bytes under both "left" and "right" labels, confirming the terminal cannot and need not distinguish them. -
Docs/helper/verifier describe the shared setting without implying parser distinction — PASS.
docs/keybindings.md("both left and right Option keys use the same terminal Meta/Esc path"), thekeys.rs/stdin-buffer.tscomments, the CHANGELOG ("from either physical Option key"),capture-option-key.py("Press left/right Option+Q…"), andverify-option-key.shall frame this as the terminal's shared Meta/Esc behavior. The verifier reads Terminal.app's singleuseOptionAsMetaKeyplist value for both keys, and correctly distinguishes iTerm2's genuinely separateOption Key Sends/Right Option Key Sendssettings. None imply parser left/right distinction. -
Loop labels provide meaningful contract coverage — PASS.
describe.each(["left","right"])runs the same\x1b\x1b[A→alt+up/\x1b\x1b[B→alt+downnormalization assertions under both labels, codifying the invariant that either physical Option key follows the same Meta/Esc path. Both variants execute (2 tests, 8 expect calls) and pass. -
Regression behaviors remain — PASS (no functional change; verified by execution). Terminal-independent canonical alt IDs (
alt+up,alt+down,alt+right,alt+left,alt+delete,alt+q,alt+i— no left/right Option variants), Option+Q (\x1bq→alt+q, kitty\x1b[113;3u→alt+q), Option+Arrow ESC-wrapped CSI (\x1b\x1b[A→alt+up), standard CSI modifier (\x1b[105;3u→alt+i,\x1b[1;3A→alt+up), and bare Escape (\x1b→esc, flush-after-timeout) are all unchanged and green.
Adversarial verification (exact head, Linux x64, native addon rebuilt from head source)
Focused and broad suites — all green:
cargo test -p pi-natives --lib keys::— 15 passed, 0 failed (incl.macos_terminal_meta_wrapped_navigation_matches_option_arrowsacross all four arrows + delete, kitty on/off).bun test packages/tui/test/keys.test.ts— 57 pass, 0 fail (incl. both new left/right loop cases).bun test packages/tui/test/stdin-buffer.test.ts— 75 pass, 0 fail (bare Escape, ESC-wrapped atomic pairing, chunked delivery).bun test packages/tui/test/(full TUI suite) — 1079 pass, 6 skip, 0 fail.bun test packages/coding-agent/test/input-controller-keybindings.test.ts— 59 pass, 0 fail (queued-message restore/delete/reorder via\x1b\x1b[A→alt+up).bun test packages/coding-agent/test/custom-editor-keybindings.test.ts packages/coding-agent/test/keybindings-config.test.ts packages/coding-agent/test/keybindings-display.test.ts— 43 pass, 0 fail.bun test packages/coding-agent/test/queue-pane.test.ts packages/coding-agent/test/input-controller-skill-queue.test.ts— 25 pass, 0 fail.- Scripts:
sh -n scripts/verify-option-key.shOK;scripts/capture-option-key.pyand both embedded Python blockspy_compileOK.
git diff --check 8a0e04ca..bc6f58ab is clean; the main worktree is clean and at current origin/dev throughout (testing was done in a disposable isolated worktree, removed afterward; no patch was ever applied to the main worktree).
CI at exact head
Check-runs at head bc6f58ab: 28 success, 5 skipped (path-gated/macOS/Windows), 0 failed, 0 pending. mergeStateStatus: CLEAN, mergeable: MERGEABLE.
Evidence bounds
- Terminal.app byte equivalency between the two physical Option keys is validated through the shared
useOptionAsMetaKeysetting plus the parser/native/TS tests; no macOS host is available on this runner, so the macOS path-gated hosted jobs are skipped. This is a documentation/test-only delta, so no host-specific runtime path is newly exercised. - The pre-existing
ESC ESCambiguity (Meta-wrapped Arrow vs. two rapid Escape presses) is unchanged by this commit and remains byte-level unavoidable in legacy encoding; it is out of scope for a comment/doc/test-only delta.
Verdict: MERGE_READY
—
[repo owner's gaebal-gajae (clawdbot) 🦞]
Summary
Verification
bun test packages/tui/test/keys.test.ts packages/tui/test/keyboard-protocol-optout.test.ts packages/coding-agent/test/custom-editor-keybindings.test.ts packages/coding-agent/test/keybindings-audit.test.ts packages/coding-agent/test/keybindings-display.test.ts packages/coding-agent/test/agent-session-queued-prompts.test.ts packages/coding-agent/test/input-controller-keybindings.test.ts packages/coding-agent/test/input-controller-skill-queue.test.ts packages/coding-agent/test/issue-825-repro.test.tsdev.bun --cwd=packages/coding-agent run checkgjc --smoke-testTERM_PROGRAM=Apple_Terminal scripts/verify-option-key.shpasses withUse Option as Meta keyenabled and ABC active.ESC qand queuedoption-q-live-testin a separate busygjc --tmuxsession.Notes
Settings > Profiles > Keyboard > Use Option as Meta key.dev.