fix(cli): let ESC cancel ongoing work before popping queued messages - #8353
Conversation
When the agent is actively responding (streamingState === Responding), InputPrompt's ESC handler consumed the key before AppContainer's global cancel-work handler could fire. Users had to press ESC 3 times (pop queue, clear input, cancel work) to stop the agent. Skip the pop-queue-into-input and double-ESC-clear logic when the agent is responding, returning false so the key propagates to the global handler which cancels the ongoing request. The up-arrow key still pops queued messages into the input at any time. Fixes QwenLM#8201
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Thanks for this — the fix itself looks sound. I traced the keypress dispatch: broadcast() fans every key out to all subscribers, so AppContainer's global ESC handler always runs; the real reason cancel-work never fired is that the queue-pop fills the shared input buffer first, which steers the global handler into its "input has content → double-ESC to clear" branch instead of the "empty buffer + responding → cancel request" branch. Guarding both the pop and the double-ESC consume on streamingState !== Responding correctly keeps the buffer empty so the cancel branch fires. Nice, minimal change.
Before this can move forward, the PR body needs the required template sections that are currently missing (see pull_request_template.md):
### Evidence (Before & After)— this is the important one for a user-visible keybinding change. Please paste a tmux before/after capture (or screenshots / a short recording) showing ESC cancelling the ongoing request on the first press while the agent is responding with queued messages. The "Tested on: Windows ✅" row is a claim, not evidence — the capture is what lets a reviewer confirm it without rebuilding. If you genuinely can't capture it, say why rather than leaving the section out.- Chinese translation (
<details><summary>中文说明</summary>) — the template asks for a full paragraph-by-paragraph translation of the body.
One non-blocking note to fold in while you're at it: there's no regression test for the new Responding guards — the 7 existing ESC tests pass with or without this diff, so nothing pins the behavior. A case in InputPrompt.test.tsx that asserts ESC returns false (and does not pop the queue) when streamingState === Responding would lock it in.
Once the template sections are in, re-run with @qwen-code /triage.
中文说明
感谢贡献——修复本身看起来是正确的。我跟进了按键分发逻辑：broadcast() 会把每个按键广播给所有订阅者，所以 AppContainer 的全局 ESC handler 始终会运行；cancel-work 之前无法触发的真正原因是 queue-pop 会先把共享输入缓冲区填满，导致全局 handler 走进"输入有内容 → 双击 ESC 清空"分支，而不是"缓冲区为空 + 正在响应 → 取消请求"分支。对 pop 和双击 ESC 消费都加上 streamingState !== Responding 守卫，能正确保持缓冲区为空，从而触发取消分支。改动很小、很到位。
在继续之前，PR 正文需要补上模板中缺失的必填部分（见 pull_request_template.md）：
### Evidence (Before & After)——对于一个用户可见的按键行为改动，这是最重要的部分。请粘贴 tmux 的 before/after 捕获（或截图/短录屏），展示在 agent 正在响应且有排队消息时，按一次 ESC 就能取消正在进行的请求。"Tested on: Windows ✅" 只是一个声明，不是证据——捕获内容才能让 reviewer 无需重新构建即可确认行为。如果确实无法捕获，请说明原因，而不是直接省略这一节。- 中文翻译（
<details><summary>中文说明</summary>）——模板要求逐段完整翻译正文。
一个非阻塞的建议，可以顺便一起处理：目前没有针对新增 Responding 守卫的回归测试——现有的 7 个 ESC 测试在有/没有这个 diff 时都会通过，所以没有任何东西固定住这个行为。可以在 InputPrompt.test.tsx 里加一个用例，断言当 streamingState === Responding 时 ESC 返回 false（且不弹出队列）。
补上模板部分后，用 @qwen-code /triage 重新运行即可。
— Qwen Code · qwen3.8-max-preview
Review:
|
buffer.setText calls |
onEscapePromptChange calls |
|
|---|---|---|
| before this PR | [] (text kept) |
[[false], [true]] → "Press ESC again to clear" shown |
| with this PR | [[""]] (text wiped) |
[[false]] → no warning |
So a user who types their next message while the agent is streaming — a very common flow, and the same flow that produces the queued messages this PR is about — loses that text on the first ESC, with no confirmation step and no undo. That first ESC also probably doesn't cancel: AppContainer's handler runs on the same broadcast and checks buffer.text.length > 0 before the cancel branch (AppContainer.tsx:3905), reading the pre-clear value from its render closure, so it takes the double-press path. Net effect for that case: text destroyed, still 2 presses to cancel.
🟠 The stated root cause isn't what's happening (affects how to fix it)
The global cancel-work handler in AppContainer was never reached because InputPrompt always returned
truefor ESC when the input was focused.
InputPrompt's return value has never gated AppContainer. KeypressContext broadcasts each key to every subscriber and ignores return values (KeypressContext.tsx:774-776), and AppContainer subscribes with isActive: true unconditionally (AppContainer.tsx:4114) — the vim-INSERT guard at AppContainer.tsx:3886 exists precisely because both handlers always fire.
The actual cause is ordering plus buffer state: InputPrompt (child) subscribes first, pops the queue into the buffer, and AppContainer then sees a non-empty buffer and takes "double-press to clear" instead of cancel. That means the queue-pop guard alone is what fixes #8201; the return false is not required for the fall-through and is what introduces the regression above.
🟡 Suggested minimal fix
Keep the pop-queue guard, and gate the fall-through on an empty buffer — that's exactly the #8201 scenario (queued messages, nothing typed) and it leaves nothing to lose:
// Only defer to AppContainer's global cancel handler when there is no
// typed input to protect — otherwise BaseTextInput's default ESC branch
// would wipe the buffer on a single press. #8201
if (uiState.streamingState === StreamingState.Responding && buffer.text === '') {
resetEscapeState();
return false;
}With typed input present the user keeps today's guarded behavior (ESC → "press again to clear" → ESC clears), and because the buffer is then empty the next ESC cancels — no silent data loss.
🟡 Test coverage
No tests are added, and the existing suite passes vacuously: the useUIState mocks in InputPrompt.test.tsx (e.g. :71, :5036) never set streamingState, so undefined !== StreamingState.Responding and neither new branch is exercised. "7/7 ESC tests pass" therefore isn't evidence for this change. Please add cases with streamingState: StreamingState.Responding:
- queue non-empty + ESC →
popAllQueuedMessagesnot called (this one does fail onmain, so it's a real regression test) - buffer has text + one ESC →
buffer.setText('')not called - buffer empty + ESC → not consumed (global handler can cancel)
🔵 Minor
resetEscapeState()isn't called beforereturn false, so a pendingescPressCount/"press ESC again" prompt from just before streaming started can linger for the remainder of the 500 ms window while ESC no longer does what the prompt says.StreamingState.WaitingForConfirmationisn't covered — ESC during a tool-confirmation prompt still pops the queue. Pre-existing and arguably out of scope, but worth a follow-up since it's the other "agent is busy" state.- Style/conventions otherwise look fine: value import of the enum is correct, comments reference the issue, no perf or security implications.
Summary: the queue-pop guard is a correct, well-targeted fix. The return false fall-through should be narrowed to the empty-buffer case before merge — as written it trades a 3-press annoyance for silent loss of typed input — and both branches need direct tests.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
— qwen3.8-max-preview via Qwen Code /review
| if (uiState.streamingState === StreamingState.Responding) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
[Critical] Returning false here does more than let AppContainer's global handler fire — BaseTextInput.handleKey treats a false return as "not consumed" (if (onKeypress?.(key)) return;) and falls into its own ESC handler, which runs buffer.setText('') on a single keypress. — Failure scenario: agent is Responding, user has typed a follow-up draft (not submitted) and presses ESC once to cancel → BaseTextInput wipes the draft immediately (no double-ESC confirmation), then AppContainer's broadcast handler sees an empty buffer + Responding and cancels the request. One ESC destroys both the unsent draft and the in-flight response, bypassing the deliberate double-ESC "press Esc again to clear" safeguard that the code below this guard (and AppContainer's "input has content" branch) otherwise enforces. Pre-diff, the same press reached the double-ESC arm and returned true, preserving the draft. Only fall through when there is no local input to protect (returning true is a viable alternative, since AppContainer cancels via the unconditional broadcast regardless of this return value):
| if (uiState.streamingState === StreamingState.Responding) { | |
| return false; | |
| } | |
| if ( | |
| uiState.streamingState === StreamingState.Responding && | |
| buffer.text === '' | |
| ) { | |
| return false; | |
| } |
— qwen3.8-max-preview via Qwen Code /review
| // #8201: without this guard, ESC pops the queue and returns true, | ||
| // consuming the key before the global cancel-work handler can fire. |
There was a problem hiding this comment.
[Suggestion] The comment's causal model is inaccurate. KeypressContext.broadcast() iterates every subscriber unconditionally and KeypressHandler returns void, so returning true never prevented AppContainer's global handler from firing — the real pre-fix bug was that popQueueIntoInput() filled the shared buffer, steering AppContainer's handler into its "input has content → double-press to clear" branch instead of its cancel branch. — Concrete cost: a future maintainer debugging ESC dispatch may trust this comment, conclude that returning true suppresses the global handler (it does not), and gate a refactor on that wrong model. The same inaccurate phrasing recurs in the comment block above the guard below (let ESC fall through to AppContainer's global handler).
| // #8201: without this guard, ESC pops the queue and returns true, | |
| // consuming the key before the global cancel-work handler can fire. | |
| // #8201: without this guard, ESC pops the queue into the input buffer; | |
| // the now-non-empty buffer makes AppContainer's global ESC handler take | |
| // its "double-press to clear" branch instead of cancelling the request. |
— qwen3.8-max-preview via Qwen Code /review
Address wenshao's review on QwenLM#8353: - Gate the return false on buffer.text === '' to prevent BaseTextInput's default ESC from silently wiping typed input without double-press confirmation - Add resetEscapeState() before return false to clear any pending escPressCount/escape-prompt timer - Add two regression tests with streamingState: StreamingState.Responding: 1. queue non-empty + ESC -> popAllQueuedMessages NOT called 2. buffer has text + single ESC -> buffer NOT cleared
|
Addressed all review feedback in
@qwen-code /triage |
Address bot suggestion: the comment claimed returning true 'consumed the key before the global cancel-work handler could fire', but KeypressContext broadcasts to all handlers regardless of return value. The real mechanism is that popQueueIntoInput() fills the shared buffer, steering AppContainer's handler into its 'input has content -> double-press to clear' branch instead of the cancel-work branch.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline. Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
— qwen3.8-max-preview via Qwen Code /review
| // its cancel-work branch (KeypressContext broadcasts to all handlers | ||
| // regardless of this return value; returning false here simply avoids | ||
| // BaseTextInput's default ESC wiping the buffer, which would have | ||
| // already been empty anyway). Gate on empty buffer to prevent |
There was a problem hiding this comment.
[Suggestion] The parenthetical inverts the control flow. BaseTextInput.handleKey short-circuits only on a truthy onKeypress return (if (onKeypress?.(key)) return;), so a false return falls through to the default Escape → clear input branch (if (buffer.text.length > 0) buffer.setText('')). return false therefore does not avoid BaseTextInput's wipe — it enables it; the buffer is safe only because the buffer.text === '' gate makes that wipe a no-op (as the comment's own final sentence correctly states). — Concrete cost: this subtlety is exactly what caused the earlier single-ESC-wipe regression on this PR; a maintainer trusting "returning false here simply avoids BaseTextInput's default ESC wiping the buffer" could relax the buffer.text === '' gate (e.g. to cancel while text is typed) and reintroduce it.
| // its cancel-work branch (KeypressContext broadcasts to all handlers | |
| // regardless of this return value; returning false here simply avoids | |
| // BaseTextInput's default ESC wiping the buffer, which would have | |
| // already been empty anyway). Gate on empty buffer to prevent | |
| // its cancel-work branch (KeypressContext broadcasts to all handlers | |
| // regardless of this return value; returning false lets BaseTextInput's | |
| // default ESC branch run, which is a no-op here because the buffer is | |
| // empty). Gate on empty buffer to prevent |
— qwen3.8-max-preview via Qwen Code /review
…nput fall-through Bot suggestion: the comment said returning false 'avoids' BaseTextInput's wipe, but return false actually *enables* it (BaseTextInput only short-circuits on truthy returns). The buffer is safe because the buffer.text === '' gate makes the wipe a no-op, not because return false prevents it. Reworded to make this explicit and warn against relaxing the gate.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
— qwen3.8-max-preview via Qwen Code /review (v0.21.3)
| describe('ESC during active agent response (#8201)', () => { | ||
| it('does not pop queued messages into input when responding', async () => { |
There was a problem hiding this comment.
[Suggestion] These two tests pin only the InputPrompt-side side effects (queue not popped; non-empty buffer preserved). The headline behaviour this PR enables — a single ESC during Responding with an empty buffer cancels the ongoing request — has no positive test: the cancel itself fires in AppContainer's broadcast ESC handler (empty buffer + Responding → cancelOngoingRequest()), which today has only a negative test (the vim-INSERT guard) plus a Ctrl+C/D-path positive. — Concrete cost: if a future change gates that cancel branch behind escapePressedOnce (re-introducing a multi-press cancel) or treats a non-empty queue as "input has content", every test added here and every existing test stays green, and the #8201 three-ESC-to-cancel regression ships silently. Consider adding an AppContainer-level test alongside the existing "Cancel Handler" block: render with streamingState: Responding and a non-empty messageQueue, dispatch one ESC with an empty buffer, and assert cancelOngoingRequest was called once and the queue was not consumed (this is also the third test case requested in the earlier maintainer review).
— qwen3.8-max-preview via Qwen Code /review (v0.21.3)
The PR's Responding guards were pinned only by InputPrompt-side tests (queue not popped; non-empty buffer preserved). Add the positive case the review asked for: while Responding with an empty buffer and queued follow-ups, a single Esc reaches the global handler's cancel-work branch (cancelOngoingRequest called once) and the queue is not consumed. QwenLM#8201
|
@qwen-code /triage |
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
— qwen3.7-max via Qwen Code /review (v0.21.3)
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline. Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
— qwen3.8-max via Qwen Code /review (v0.21.4)
| // ...and must not consume the queued follow-ups (InputPrompt owns | ||
| // that decision and skips the pop while Responding; #8201). | ||
| expect(mockPopAllMessages).not.toHaveBeenCalled(); |
There was a problem hiding this comment.
[Suggestion] This assertion and its comment claim the #8201 scenario (single Esc, empty buffer, queued follow-ups) leaves the queue untouched end-to-end, but in production that exact path consumes the queue: handleGlobalKeypress → cancelOngoingRequest (AppContainer.tsx:3751) → useGeminiStream's synchronous onCancelSubmit (useGeminiStream.ts:887) → cancelHandlerRef.current → popAllMessages() (AppContainer.tsx:2479), draining the queue into the buffer under the documented "never silently drop the user's queued work" invariant (AppContainer.tsx:2476-2478). The assertion passes only because installCancelCapture supplies cancelOngoingRequest: cancelSpy (a bare vi.fn()), severing that chain — verified by probe: wiring the captured onCancelSubmit into the spy makes this assertion fail (pass → fail → pass after restore). — Concrete cost: a maintainer debugging "my queued message appeared in the input after I cancelled" is pointed at a false contract by this comment, and "fixing" production to match it would break the documented cancel-path drain invariant.
| // ...and must not consume the queued follow-ups (InputPrompt owns | |
| // that decision and skips the pop while Responding; #8201). | |
| expect(mockPopAllMessages).not.toHaveBeenCalled(); | |
| // ...and the global keypress handler itself must not pop the queue | |
| // (InputPrompt owns the ESC pop decision and skips it while Responding; | |
| // #8201). End-to-end the cancel path then drains the queue back into | |
| // the buffer via the cancel handler — that hop is severed here because | |
| // cancelOngoingRequest is replaced by a spy. | |
| expect(mockPopAllMessages).not.toHaveBeenCalled(); |
— qwen3.8-max via Qwen Code /review (v0.21.4)
The positive ESC cancel test asserts popAllMessages is not called, but the comment framed it as 'must not consume the queue' end-to-end. In production that exact cancel path DOES drain the queue back into the buffer via the cancel handler (cancelOngoingRequest -> onCancelSubmit -> popAllMessages), under the 'never silently drop queued work' invariant. The assertion only holds because cancelOngoingRequest is replaced by a spy here, severing that hop. Reword the comment to describe the real contract: the global keypress handler itself doesn't pop the queue (InputPrompt owns that and skips it while Responding; QwenLM#8201), while the end-to-end drain is a separate hop severed by the spy. Addresses the review finding on AppContainer.test.tsx:2405.
|
@qwen-code /review |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline. Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
— qwen3.8-max via Qwen Code /review (v0.21.3)
| const handleKeypress = mockedUseKeypress.mock.calls | ||
| .map((call) => call[0]) | ||
| .reverse() | ||
| .find( | ||
| (handler): handler is (key: Key) => void => | ||
| typeof handler === 'function' && | ||
| handler.toString().includes('TOGGLE_THINKING_EXPANDED'), | ||
| ) as ((key: Key) => void) | undefined; |
There was a problem hiding this comment.
[Suggestion] This inlines a byte-identical copy of the existing describe-local helper getGlobalKeypress() (defined in describe('Thinking expansion (Ctrl+O) integration') further down) instead of reusing it — Concrete cost: the fragile handler.toString().includes('TOGGLE_THINKING_EXPANDED') discovery idiom now exists in two identical copies ~2,600 lines apart in this file; renaming Command.TOGGLE_THINKING_EXPANDED, or any transform change to handler stringification, forces lockstep edits in both places, and the resulting failure (expect(handleKeypress).toBeDefined()) names neither cause. Verified by probe: hoisting the helper (and the adjacent makeKey factory) to the enclosing describe and calling them here keeps both consumer tests green. The fix spans two spots, so no one-click suggestion:
// in describe('AppContainer State Management'), hoisted out of the
// 'Thinking expansion (Ctrl+O) integration' describe:
const getGlobalKeypress = () =>
mockedUseKeypress.mock.calls
.map((call) => call[0])
.reverse()
.find(
(handler): handler is (key: Key) => void =>
typeof handler === 'function' &&
handler.toString().includes('TOGGLE_THINKING_EXPANDED'),
) as ((key: Key) => void) | undefined;
// here:
const handleKeypress = getGlobalKeypress();— qwen3.8-max via Qwen Code /review (v0.21.3)
| if ( | ||
| uiState.streamingState === StreamingState.Responding && | ||
| buffer.text === '' | ||
| ) { | ||
| resetEscapeState(); | ||
| return false; | ||
| } |
There was a problem hiding this comment.
[Suggestion] This branch is ungated by any test — Concrete cost: reverting this hunk alone (queue-pop guard kept) leaves all 334 tests in the two changed files green (measured), so a future refactor of the ESC handling can delete the branch or flip return false to return true with nothing firing in CI. The new AppContainer test bypasses InputPrompt (it extracts the global handler from mockedUseKeypress.mock.calls and invokes it directly), and the two InputPrompt tests assert only pop non-invocation and the non-empty-buffer path. The branch is behaviorally inert at this commit (the broadcast ignores return values; BaseTextInput's ESC clear is gated on a non-empty buffer), but nothing pins the #8201 deferral contract the comment above warns about. Consider adding a case to describe('ESC during active agent response (#8201)') with messageQueue: [], empty buffer, streamingState: Responding, that observes this branch (e.g. the handler returns false / BaseTextInput's fall-through engages), so removing the branch fails the suite.
— qwen3.8-max via Qwen Code /review (v0.21.3)
Address two review suggestions on the ESC cancel tests: - The `return false` branch in InputPrompt.tsx (Responding + empty buffer + empty queue -> defer to AppContainer's cancel-work branch) had no test coverage: reverting it left all 334 tests green. Add an InputPrompt test that pins it (no queue pop, no buffer mutation). - The AppContainer cancel test inlined a byte-identical copy of the getGlobalKeypress() helper that already existed ~2600 lines down in the Ctrl+O describe block. Hoist the helper to the outer describe so both blocks share one definition of the fragile toString() discovery idiom.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline. Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
— qwen3.8-max via Qwen Code /review (v0.21.5)
| const escKey: Key = { | ||
| name: 'escape', |
There was a problem hiding this comment.
[Suggestion] R4-1: This fixture embeds a raw ESC control byte (0x1B) in the sequence literal — it is invisible in diffs and tooling: the PR diff renders the line as sequence: '', and grep -n "sequence:" truncates the line at the control byte (verified), so nobody reading this PR or searching the file can tell what key data the test sends. Failure scenario: a cleanup that "fixes" the seemingly-empty string silently changes the fixture. Every sibling fixture escapes its control bytes ('\u001b' in the vim-INSERT test directly above, '\x02'/'\x0f' in the Ctrl+O block), and ESCAPE matching is name-only ([Command.ESCAPE]: [{ key: 'escape' }]), so the escaped form is behavior-identical.
| const escKey: Key = { | |
| name: 'escape', | |
| const escKey: Key = { | |
| name: 'escape', | |
| sequence: '\u001b', |
— qwen3.8-max via Qwen Code /review (v0.21.5)
Per review (R4-1): the sequence literal embedded a raw 0x1B control byte that renders as an empty string in diffs and truncates grep output, so the fixture was unreadable. Use the escaped form matching the sibling vim-INSERT fixture on the line above.
|
@qwen-code /review |
…-while-responding
|
Qwen Code review timed out. Qwen review timed out after 10800 seconds (of the 180-minute budget). For large PRs, retry with a longer timeout by commenting: |
|
@qwen-code /review --timeout=360 |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline. Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
— qwen3.8-max via Qwen Code /review (v0.21.5)
| // Pins the `return false` branch at InputPrompt.tsx when the agent is | ||
| // Responding AND both the buffer and the queue are empty - the path that | ||
| // lets AppContainer's global ESC handler take its cancel-work branch. | ||
| // Without this, deleting that branch leaves all tests green. #8201. |
There was a problem hiding this comment.
[Suggestion] R5-1: This test's comment claims it pins the return false branch in InputPrompt.tsx ("Without this, deleting that branch leaves all tests green"), but a mutation run shows the opposite: deleting that branch leaves this test — and the whole file's suite — green (measured: 200/200 with the branch deleted). With the branch gone, the same input (Responding, empty buffer, empty queue) falls through to escPressCount === 0 && buffer.text === '' → return true, producing identical observables (no popAllQueuedMessages call, no setText call); the handler's boolean return is unobservable here — KeypressContext.broadcast discards return values, and BaseTextInput's ESC clear is a no-op on an empty buffer. The test was added to answer the round-3 "branch is ungated by any test" thread, but as written it pins the no-pop/no-buffer-mutation contract, not the branch. — Concrete cost: a maintainer later deleting or rewriting the return false branch relies on this comment's explicit promise and ships with a green suite, leaving the #8201 deferral contract (the production-side "Do NOT relax this gate" warning) silently unpinned in CI.
| // Pins the `return false` branch at InputPrompt.tsx when the agent is | |
| // Responding AND both the buffer and the queue are empty - the path that | |
| // lets AppContainer's global ESC handler take its cancel-work branch. | |
| // Without this, deleting that branch leaves all tests green. #8201. | |
| // Pins the no-side-effect contract for ESC while the agent is Responding | |
| // and both the buffer and the queue are empty: no queue pop and no | |
| // buffer mutation. Note: the `return false` branch itself is defensive — | |
| // KeypressContext.broadcast ignores handler return values and | |
| // BaseTextInput's ESC clear is a no-op on an empty buffer — so deleting | |
| // that branch would still leave this test green. #8201. |
— qwen3.8-max via Qwen Code /review (v0.21.5)
| // popAllQueuedMessages must NOT be called when agent is responding | ||
| expect(mockPopAllQueued).not.toHaveBeenCalled(); |
There was a problem hiding this comment.
[Suggestion] R5-2: This flagship #8201 test asserts only the mechanism (popAllQueuedMessages not called), not the effect the diff's own production comment names — that the shared buffer stays empty so AppContainer's global ESC handler takes its cancel branch instead of "input has content → double-press to clear". The fixture's setText updates .text synchronously, so the effect-shaped assertion is a one-liner. — Failure scenario: a future change reintroduces filling the buffer with queued messages on ESC during Responding through any path other than uiActions.popAllQueuedMessages (e.g. draining the queue directly into the buffer) → this test stays green, the global handler sees non-empty text and takes the double-press branch instead of cancelling — the exact #8201 regression ships. Measured: a synthetic drain mutation bypassing popAllQueuedMessages passes the test as-is and fails once the suggested assertion is added.
| // popAllQueuedMessages must NOT be called when agent is responding | |
| expect(mockPopAllQueued).not.toHaveBeenCalled(); | |
| // popAllQueuedMessages must NOT be called when agent is responding | |
| expect(mockPopAllQueued).not.toHaveBeenCalled(); | |
| // ...and the shared buffer must stay empty so AppContainer's global ESC | |
| // handler takes its cancel branch instead of "input has content". | |
| expect(props.buffer.text).toBe(''); |
— qwen3.8-max via Qwen Code /review (v0.21.5)
Per review (R5-1/R5-2): the flagship QwenLM#8201 test asserted only the mechanism (popAllQueuedMessages not called), not the effect (buffer stays empty so AppContainer takes its cancel branch). Add the buffer assertion. Also correct the return-false test comment: deleting that branch leaves the test green (KeypressContext.broadcast ignores return values, BaseTextInput's clear is a no-op on empty buffer), so the test pins the no-side-effect contract, not the branch itself.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline. Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
— qwen3.8-max via Qwen Code /review (v0.21.5)
| describe('ESC during active agent response (#8201)', () => { | ||
| it('does not pop queued messages into input when responding', async () => { |
There was a problem hiding this comment.
[Suggestion] R6-1: The new Responding-state matrix covers queue + empty buffer and empty queue + typed text, but not the combination (non-empty queue AND typed text), so the pop-skip guard in InputPrompt.tsx is pinned only for the empty-buffer case.
Failure scenario: mutating the guard to uiState.streamingState !== StreamingState.Responding || buffer.text !== '' survives all 200 tests in this file (verified by a live mutation run), yet ESC during Responding would again prepend the queued messages onto half-typed text — the same surprise-buffer-mutation UX #8201 fixes. A probe test for the combination fails under that mutant and passes on this PR's code.
Suggested fourth test for this describe block:
it('does not pop queued messages over typed input when responding', async () => {
mockedUseUIState.mockReturnValue({
isFeedbackDialogOpen: false,
messageQueue: ['queued message'],
pendingGeminiHistoryItems: [],
streamingState: StreamingState.Responding,
} as unknown as ReturnType<typeof useUIState>);
const mockPopAllQueued = vi.fn(() => null);
mockedUseUIActions.mockReturnValue({
handleRetryLastPrompt: vi.fn(),
temporaryCloseFeedbackDialog: vi.fn(),
popAllQueuedMessages: mockPopAllQueued,
invalidateSubmittedPromptProvenance: vi.fn(),
} as unknown as ReturnType<typeof useUIActions>);
props.buffer.setText('half typed');
const { stdin, unmount } = renderWithProviders(<InputPrompt {...props} />);
await wait();
stdin.write('\x1B');
await wait();
expect(mockPopAllQueued).not.toHaveBeenCalled();
expect(props.buffer.text).toBe('half typed');
unmount();
});— qwen3.8-max via Qwen Code /review (v0.21.5)
| // Buffer must NOT be cleared - double-ESC confirmation still applies | ||
| expect(props.buffer.text).toBe('half typed message'); |
There was a problem hiding this comment.
[Suggestion] R6-2: No test pins the second half of the double-press contract this diff explicitly preserves — that double-ESC still clears typed input while Responding ("Do NOT relax this gate … double-press confirmation"). All three new tests press ESC only once, and the pre-existing double-ESC describe runs with streamingState undefined.
Failure scenario: adding if (uiState.streamingState === StreamingState.Responding) return true; at the top of the double-ESC clear branch survives all 200 tests in this file and the repo's remaining InputPrompt test surface (150 tests) — verified by a live mutation. That mutant ships a regression where mid-response the "press ESC again to clear" prompt appears but the second press does nothing, so typed text can never be cleared from the keyboard while the agent responds.
Companion test (passes on this PR's code, fails under the mutant — verified by probe), following the act() + wait(50) idiom of the pre-existing double-ESC tests:
it('still clears typed input on double ESC when responding', async () => {
mockedUseUIState.mockReturnValue({
isFeedbackDialogOpen: false,
messageQueue: [],
pendingGeminiHistoryItems: [],
streamingState: StreamingState.Responding,
} as unknown as ReturnType<typeof useUIState>);
props.buffer.setText('half typed message');
const { stdin, unmount } = renderWithProviders(<InputPrompt {...props} />);
await wait();
act(() => {
stdin.write('\x1B');
});
await wait(50);
act(() => {
stdin.write('\x1B');
});
await wait();
expect(props.buffer.setText).toHaveBeenCalledWith('');
unmount();
});— qwen3.8-max via Qwen Code /review (v0.21.5)
Per review (R6-1/R6-2): the pop-skip guard was pinned only for the empty-buffer case, and the double-press clear contract had no test under Responding. Add: - non-empty queue AND typed text: ESC does not pop the queue and preserves the buffer (pins the guard regardless of buffer content). - double-ESC while Responding: first ESC preserves typed text, second clears it (pins the double-press contract this diff preserves).
…ing Esc Per review (R10-1/R10-2): the shared escKey fixture embedded a raw 0x1B control byte (invisible in diffs, truncates grep). Use the escaped form. Also update keyboard-shortcuts.md: Esc now cancels the ongoing request while the agent is responding instead of moving queued messages back into the input.
|
@qwen-code /review |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline. Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
— qwen3.8-max via Qwen Code /review (v0.21.5)
| | `Down Arrow` | Row down, then snap to end, then history next. | | ||
| | `Enter` | Submit the current prompt. While a response is running, steer the current turn. | | ||
| | `Ctrl+Q` | Queue the current prompt or command for the next turn instead of steering; it runs after Qwen Code returns to idle. | | ||
| | `Up Arrow` (at the top) / `Esc` | When queued messages are present and the agent is idle, move them back into the input for editing. While the agent is responding, `Esc` cancels the ongoing request instead (queued messages are preserved). | |
There was a problem hiding this comment.
[Suggestion] R11-1: The rewritten row's "and the agent is idle" qualifier does not match the code for either key it documents. The Up-arrow-at-top queue pop in InputPrompt.tsx (~1597-1606) has no streamingState guard, so it pops queued messages into the input even while the agent is Responding (probe-verified: Up Arrow + Responding + non-empty queue calls popAllQueuedMessages). The Esc pop guard is streamingState !== StreamingState.Responding, so Esc also pops during WaitingForConfirmation (not idle). The second sentence omits that the responding-cancel only fires when the input buffer is empty — with typed text present, the first Esc takes the double-press-to-clear branch. — Failure scenario: while the agent is responding with queued follow-ups, a user following these docs presses Up Arrow expecting nothing until idle; the queue is popped into the input. Pressing Esc expecting the documented cancel then shows "press Esc again to clear" while the in-flight work keeps running, and a second Esc wipes the popped queued text — re-entering the exact trap issue 8201 fixes for the ESC path, via the sibling Up-Arrow path. Suggested fix below rewords the row to match the code; the alternative is to add the same streamingState !== StreamingState.Responding guard to the Up-Arrow pop site (and widen the Esc guard to === StreamingState.Idle), with tests, if idle-only recall is the intended contract.
| | `Up Arrow` (at the top) / `Esc` | When queued messages are present and the agent is idle, move them back into the input for editing. While the agent is responding, `Esc` cancels the ongoing request instead (queued messages are preserved). | | |
| | `Up Arrow` (at the top) / `Esc` | When queued messages are present, move them back into the input for editing (`Up Arrow` at the top in any state; `Esc` whenever the agent is not actively responding). While the agent is responding and the input is empty, `Esc` cancels the ongoing request instead (queued messages are preserved). | |
— qwen3.8-max via Qwen Code /review (v0.21.5)
Per review (R11-1): the previous wording said queue pop happens only when idle, but Up Arrow pops in any state (no streamingState guard) and Esc pops whenever not actively responding (including WaitingForConfirmation). Reword to match the code, and note that the responding-cancel only fires when the input is empty.
|
@qwen-code /review |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
— qwen3.8-max via Qwen Code /review (v0.21.5)
|
@qwen-code /review |
Review:
|
| Check | Result |
|---|---|
InputPrompt.test.tsx + AppContainer.test.tsx full suites |
344/344 pass (the "pre-existing flaky clipboard timeout" you mention did not reproduce here) |
Mutation: drop streamingState !== Responding from guard #1 |
2 tests fail → guard #1 is pinned ✅ |
| Mutation: delete guard #2 entirely (all 7 lines) | all 12 ESC tests still pass ❌ |
prettier --check / eslint on all 4 changed files |
clean |
1. Guard #2 is dead code — recommend removing it
if (uiState.streamingState === StreamingState.Responding && buffer.text === '') {
resetEscapeState();
return false;
}Deleting this block changes no observable behavior. With it gone, control falls to if (escPressCount === 0) { if (buffer.text === '') return true; } — no buffer mutation, no queue pop, and AppContainer cancels either way since the return value is ignored by the broadcast. escPressCount === 1 is unreachable here: it is only ever set when buffer.text !== '', and any non-ESC key resets it, so the else branch can't be entered with an empty buffer.
That's 7 lines of code plus a 10-line comment that carries no weight, and the comment's warning — "Do NOT relax this gate: returning false with non-empty text would let BaseTextInput wipe it" — guards a branch that can't currently be reached. Your own test comment already concedes the branch is unpinned. I'd either drop it and fold the useful part of the explanation into guard #1's comment, or keep it only with an explicit // intentionally inert today; documents the invariant note. Right now the PR description presents it as half the fix, which overstates it.
2. The real coverage gap: nothing tests the integration
Both new test groups mock the other side of the interaction:
InputPrompt.test.tsxassertspopAllQueuedMessagesisn't called — but never runsAppContainer's handler.AppContainer.test.tsxpullshandleGlobalKeypressout of theuseKeypressmock and invokes it directly — soInputPrompt's guard is never in the picture. Itsexpect(mockPopAllMessages).not.toHaveBeenCalled()is close to tautological, sincecancelOngoingRequestis a spy andonCancelSubmitnever fires (your comment says as much). ThecancelSpyassertion is a useful regression guard on pre-existing code, though.
So the PR's central claim — "one ESC instead of three" — has no test. Worse, the fix silently depends on two implicit invariants:
InputPrompt/BaseTextInputsubscribing toKeypressContextbeforeAppContainer, andbuffer.textreading through tostateRef.currentsynchronously.
Break either (e.g. memoize text off React state, or hoist the input's keypress subscription) and the 3-press bug returns with a fully green suite. One test that renders the real provider tree and asserts cancelOngoingRequest is called once after a single \x1B with a non-empty queue would lock this down. If that's too heavy, at minimum reference both invariants in the guard #1 comment — it currently explains what happens but not why the ordering holds.
3. Behavior notes (not blockers)
- The guard fires regardless of buffer content, so while Responding the ESC→pop-queue route is gone entirely, not just in the empty-buffer case. Up arrow still covers it, and the cancel path drains the queue back into the input anyway (
AppContainer.tsx:2736), so nothing is lost. Good outcome — just broader than "when the input is empty". - Docs wording: "queued messages are preserved" is slightly imprecise — after the cancel they are moved into the input box, not left queued. Something like "queued messages are moved back into the input" matches what actually happens.
WaitingForConfirmationis not covered by either guard. That's consistent —AppContainer's cancel branch also only checksResponding— so ESC during a tool confirmation behaves as before. Worth a sentence in the guard comment so nobody "fixes" it later by broadening the check to!== Idle, which would make ESC a no-op there.- Typed text + Responding: guard Where is the config saved? #2 doesn't apply, so the first ESC shows the double-press prompt and the second both clears the buffer and cancels (AppContainer sees the freshly-emptied live buffer in the same broadcast). Pre-existing, unchanged by this PR, but it means the double-press contract is a bit fuzzier than the tests suggest.
4. Test/style nits
- The
getGlobalKeypressdedupe is a genuine improvement. It does change the vim test's discovery token fromhandleExittoTOGGLE_THINKING_EXPANDED— both live inhandleGlobalKeypress, so it's fine, thoughhandler.toString().includes(...)remains brittle (pre-existing idiom). - The docs diff reflows all 42 table rows because the column width changed. Prettier-clean, just diff noise — nothing to do.
Summary
| Correctness | ✅ root cause identified correctly, guard #1 fixes it |
| Conventions | ✅ prettier + eslint clean, matches surrounding test idioms |
| Performance | ✅ two enum comparisons per keypress |
| Security | ✅ n/a |
| Tests |
Suggested before merge: (a) drop guard #2 or mark it explicitly inert, (b) add one integration test for the single-ESC cancel, (c) tighten the docs sentence about queued messages.
wenshao's mutation test showed guard QwenLM#2 (Responding + empty buffer -> return false) is dead code: with it gone, control falls to the escPressCount===0 branch which returns true on an empty buffer, and KeypressContext.broadcast ignores handler return values anyway - AppContainer's own cancel branch acts on the empty buffer either way. Remove it, fold the subscription-ordering invariants it relied on into guard QwenLM#1's comment, and document that only Responding is gated (do not broaden to !== Idle or ESC becomes a no-op during a tool confirmation). Also tighten the docs wording: cancelled queued messages are moved back into the input, not preserved. QwenLM#8201
|
@qwen-code /review |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
— qwen3.8-max via Qwen Code /review (v0.21.6)
| // stateRef.current synchronously. Break either and the single-ESC | ||
| // cancel regresses with a fully green suite - see the integration test. |
There was a problem hiding this comment.
[Suggestion] R13-1: This comment points at a safety net that does not exist — there is no integration test covering single-ESC-cancel-with-queued-messages anywhere (integration-tests/, .qwen/e2e-tests/, and package-local tests were all searched; the pointer was introduced by this PR's final commit). Invariant (1) is also overstated: useKeypress re-subscribes on isActive flips, so after any embedded-shell focus toggle BaseTextInput's handler re-joins the subscriber Set behind AppContainer's — though the Responding-cancel path is order-independent at HEAD, so this is comment inaccuracy, not a live regression. — Failure scenario: a future refactor of KeypressContext subscription order, useKeypress memoization, or buffer.text liveness silently restores the 3-ESC cancel from #8201 with a fully green suite, and the maintainer following this pointer finds no test → the regression ships and the debugging is actively misdirected. Preferred fix: add the referenced interactive test (queue a follow-up, start a response, press one ESC, assert the request is cancelled and the queued text lands back in the input) — @wenshao's review suggested exactly this before merge. If the test is deferred to a follow-up, make the comment honest in the meantime:
| // stateRef.current synchronously. Break either and the single-ESC | |
| // cancel regresses with a fully green suite - see the integration test. | |
| // stateRef.current synchronously. Break either and the single-ESC | |
| // cancel regresses with a fully green suite (no integration test covers this hop yet). |
— qwen3.8-max via Qwen Code /review (v0.21.6)
| // No queue pop, no buffer mutation - the branch returns false so | ||
| // AppContainer's broadcast ESC handler is the one that acts. |
There was a problem hiding this comment.
[Suggestion] R13-2: This comment misstates the mechanism on both counts. At HEAD the exercised branch returns true — after the final commit removed the dead guard there is no return false anywhere in the ESC path (handleInput returns true on every ESC branch). And AppContainer's handler does not act because of any return value: KeypressContext.broadcast calls every subscriber unconditionally and KeypressHandler is (key: Key) => void — the return value only gates BaseTextInput's own fallback. The final commit fixed the test title and leading comment but missed this trailing one; it is the last surviving instance of the inverted-control-flow model an earlier round corrected in the production comment. — Failure scenario: a maintainer debugging a future single-ESC-cancel regression reads this regression test's comment and 'fixes' an adjacent branch by flipping a return value expecting different dispatch (no effect), or reintroduces return-value-based consumption believing the convention exists → wrong causal model cemented into the suite that is supposed to pin the correct one.
| // No queue pop, no buffer mutation - the branch returns false so | |
| // AppContainer's broadcast ESC handler is the one that acts. | |
| // No queue pop, no buffer mutation - this branch consumes the key (returns true) | |
| // but mutates nothing; AppContainer's broadcast ESC handler runs after this one | |
| // regardless of return values and cancels because the buffer stayed empty. |
— qwen3.8-max via Qwen Code /review (v0.21.6)
| | `Down Arrow` | Row down, then snap to end, then history next. | | ||
| | `Enter` | Submit the current prompt. While a response is running, steer the current turn. | | ||
| | `Ctrl+Q` | Queue the current prompt or command for the next turn instead of steering; it runs after Qwen Code returns to idle. | | ||
| | `Up Arrow` (at the top) / `Esc` | When queued messages are present, move them back into the input for editing (`Up Arrow` at the top in any state; `Esc` whenever the agent is not actively responding). While the agent is responding and the input is empty, `Esc` cancels the ongoing request instead (queued messages are then moved back into the input). | |
There was a problem hiding this comment.
[Suggestion] R13-4: Both state qualifiers in this rewritten row are false for WaitingForConfirmation. Composer renders InputPrompt only when uiState.isInputActive, and isInputActiveForState admits only Idle/Responding — so during a tool confirmation InputPrompt is unmounted, neither key's queue-pop branch can run, and AppContainer's global ESC handler has no queue-pop branch either (empty buffer + WFC falls to the reset-flag path). Verified: Composer.tsx is the sole production mount site; QueuedMessageDisplay stays visible during WFC, so the recall attempt is realistic. (Up-at-top is additionally blocked by attachment mode and, when mounted, the hasActiveToolConfirmation navigation swallow.) — Failure scenario: user queues a follow-up, the agent pauses on a tool confirmation; following this row they press Up Arrow or Esc to pull the queued text back — nothing happens, and the queue appears stuck until the confirmation resolves. Suggested reword (or drop the state claims and restore the prior unconditional phrasing):
| | `Up Arrow` (at the top) / `Esc` | When queued messages are present, move them back into the input for editing (`Up Arrow` at the top in any state; `Esc` whenever the agent is not actively responding). While the agent is responding and the input is empty, `Esc` cancels the ongoing request instead (queued messages are then moved back into the input). | | |
| | `Up Arrow` (at the top) / `Esc` | When queued messages are present, move them back into the input for editing (`Up Arrow` at the top whenever the input is shown; `Esc` only when the agent is idle). While the agent is responding and the input is empty, `Esc` cancels the ongoing request instead (queued messages are then moved back into the input). | |
— qwen3.8-max via Qwen Code /review (v0.21.6)
| // Only Responding is gated (matching AppContainer's cancel branch); | ||
| // WaitingForConfirmation is deliberately not covered - do NOT broaden | ||
| // this to !== Idle or ESC becomes a no-op during a tool confirmation. |
There was a problem hiding this comment.
[Suggestion] R13-5: The warning's consequence is impossible: during a tool confirmation Composer unmounts InputPrompt entirely (isInputActiveForState admits only Idle/Responding), so this handler never runs in that state and the guard's shape cannot make ESC a no-op there — !== Responding and the forbidden !== Idle are behaviorally identical at runtime for this branch. The comment presents the !== Responding permissiveness as protecting live WaitingForConfirmation behavior that does not execute; same wrong-mechanism class as the two comment findings above. — Failure scenario: a maintainer heeding this warning (or writing a WaitingForConfirmation pop test) cements the false model that InputPrompt handles Esc during tool confirmation, then rejects a correct simplification because the comment forbids it, or 'fixes' an imagined regression → future edits reasoned from an unreachable-state model.
| // Only Responding is gated (matching AppContainer's cancel branch); | |
| // WaitingForConfirmation is deliberately not covered - do NOT broaden | |
| // this to !== Idle or ESC becomes a no-op during a tool confirmation. | |
| // Only Responding is gated (matching AppContainer's cancel branch). | |
| // WaitingForConfirmation needs no handling here: Composer unmounts InputPrompt | |
| // whenever isInputActive is false, so this branch never runs during a tool confirmation. |
— qwen3.8-max via Qwen Code /review (v0.21.6)
Round-13 review (no blockers) flagged comment inaccuracies that could misdirect future debugging: - R13-1: the invariant comment pointed at an integration test that does not exist - note the harnesses mock each other's side instead. - R13-2: the regression-test comment still said the branch returns false and that AppContainer acts on return values; it returns true and broadcast ignores return values. - R13-4: the docs row claimed Up Arrow/Esc pop in any state, but during WaitingForConfirmation Composer unmounts InputPrompt (isInputActive admits only Idle/Responding), so neither key pops. - R13-5: the guard comment warned against broadening to !== Idle as if WFC ran this branch; it never does because the component is unmounted. No behavior change. QwenLM#8201
|
@qwen-code /review |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline. Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
— qwen3.8-max via Qwen Code /review (v0.21.6)
| // Relies on two invariants: (1) InputPrompt/BaseTextInput subscribe | ||
| // before AppContainer, and (2) buffer.text reads through to | ||
| // stateRef.current synchronously. Break either and the single-ESC | ||
| // cancel regresses with a fully green suite (no integration test | ||
| // covers this hop yet - the two harnesses mock each other's side). |
There was a problem hiding this comment.
[Suggestion] This comment asserts invariant (1) — "InputPrompt/BaseTextInput subscribe before AppContainer" — unconditionally and warns "Break either and the single-ESC cancel regresses", but the ordering does not survive an InputPrompt remount and the cancel does not depend on it. Composer renders InputPrompt only when isInputActive, and isInputActiveForState admits only Idle/Responding, so every tool-confirmation round trip unmounts InputPrompt (embedded-shell focus toggling does the same via BaseTextInput's isActive), and on remount useKeypress re-subscribes at the end of the insertion-ordered subscriber set — after AppContainer's always-subscribed handler. The comment acknowledges that unmount two lines later. Single-ESC cancel is order-independent at HEAD anyway: the Responding pop guard skips the pop regardless of which handler runs first, so breaking invariant (1) regresses nothing. — Concrete cost: a maintainer who later observes the flipped post-remount order chases a phantom regression or adds ordering machinery the behavior does not need, misdirected by the comment whose purpose is to say what is safe to break. (Residue of R13-1 — the final commit fixed the phantom-test pointer but left this part unaddressed.)
| // Relies on two invariants: (1) InputPrompt/BaseTextInput subscribe | |
| // before AppContainer, and (2) buffer.text reads through to | |
| // stateRef.current synchronously. Break either and the single-ESC | |
| // cancel regresses with a fully green suite (no integration test | |
| // covers this hop yet - the two harnesses mock each other's side). | |
| // Relies on one invariant: buffer.text reads through to | |
| // stateRef.current synchronously. Subscription order does not gate the | |
| // cancel: the Responding pop guard skips the pop in either order (and | |
| // BaseTextInput re-subscribes after AppContainer after any remount of | |
| // InputPrompt, e.g. a tool-confirmation round trip). Break the buffer | |
| // invariant and the single-ESC cancel regresses with a fully green | |
| // suite (no integration test covers this hop yet - the two harnesses | |
| // mock each other's side). |
— qwen3.8-max via Qwen Code /review (v0.21.6)
| // Handle double ESC for clearing input. (When the agent is | ||
| // Responding and the buffer is empty, AppContainer's broadcast ESC | ||
| // handler - running after this one - takes its cancel-work branch | ||
| // directly, so no separate guard is needed here.) #8201. |
There was a problem hiding this comment.
[Suggestion] The double-ESC clear composes with AppContainer's broadcast ESC handler into "clear draft AND cancel the request" on the same keypress in the initial subscription order — this branch empties the shared buffer synchronously, and AppContainer's handler in the same broadcast reads empty buffer + Responding and calls cancelOngoingRequest() — while after any remount of InputPrompt (every tool-confirmation round trip) AppContainer runs first, clears on the second press itself, and the cancel needs a third ESC: same gesture, two outcomes, selected by session history. The queue case is newly reachable because of this diff — pre-diff, the first ESC popped the queue and resetEscapeState() kept the second press out of the clearing branch (the no-queue variant pre-exists). This comment, the docs row, and the new "still clears typed input on double-ESC while responding" test (whose harness renders no AppContainer, so the cancel hop is invisible to it) all describe the gesture as clear-without-cancel. No data is lost — the cancel handler drains the queue back into the input — but the cancel rides on a gesture documented only as clearing. — Concrete cost: a user with a typed draft double-presses ESC while the agent is Responding with queued follow-ups, intending to clear the input; in a fresh session the second ESC silently also cancels the in-flight response, while after any tool confirmation the same two presses only clear and a third press is needed.
| // Handle double ESC for clearing input. (When the agent is | |
| // Responding and the buffer is empty, AppContainer's broadcast ESC | |
| // handler - running after this one - takes its cancel-work branch | |
| // directly, so no separate guard is needed here.) #8201. | |
| // Handle double ESC for clearing input. Note: while Responding this | |
| // clear composes with AppContainer's broadcast ESC handler - in the | |
| // initial subscription order this handler empties the buffer first and | |
| // AppContainer's cancel branch fires on the SAME keypress; after any | |
| // remount of InputPrompt (e.g. a tool-confirmation round trip) | |
| // AppContainer runs first, so the cancel lands on the next press | |
| // instead. #8201. |
— qwen3.8-max via Qwen Code /review (v0.21.6)
Round-14 review: the invariant comment overstated subscription order as load-bearing - the Responding pop guard skips the pop in either order, and InputPrompt re-subscribes after AppContainer on any remount (e.g. a tool-confirmation round trip), so only the buffer.text-liveness invariant matters. And the double-ESC clear comment now notes it composes with AppContainer's cancel on the same keypress in the initial order but lands on the next press after a remount. No behavior change. QwenLM#8201
|
@qwen-code /review |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
No issues found. LGTM! ✅
— qwen3.8-max via Qwen Code /review (v0.21.6)
Real-environment verification — ✅ behaves as advertised, safe to mergeI built two real CLI bundles from source and drove both through an actual TUI (tmux, 120×40) against a live streaming provider, then compared them key-press for key-press. The fix does exactly what the description claims, and the three paths it must not touch are byte-identical to Setup
Bundle provenance checked before running, so the two binaries really are the two revisions:
Results — 5 scenarios, both builds
A — the reported bug (#8201), before: ESC #1 pops the queue into the input while the model keeps streaming, ESC #2 clears it (the queued text is now gone for good), ESC #3 finally cancels.
A — after: one ESC cancels, and the queued message survives — the cancel path drains it back into the input box.
B — typed text and a queue (the messier case).
C — the untouched baseline. Streaming with an empty input and an empty queue: one ESC cancels on both builds. The ledgers are indistinguishable — both aborted at
D + E — the regression checks. Up-Arrow still reaches the queue while streaming, and the double-ESC-clear contract for typed text is intact (ESC #1 preserves the draft and shows
Wire-level evidenceScenario A ledgers, same script, same timings: Scenario C ledgers (the path the guard must not affect): Test suite + mutation check
Notes, not blockers
Verdict: LGTM — the root-cause analysis is right, the fix is minimal, and every neighbouring ESC path is provably unchanged. 🇨🇳 中文版报告真实环境验证 —— ✅ 行为与描述一致，可以合入我从源码构建了两份真实 CLI bundle，在真实 TUI（tmux，120×40）里对接真实流式 provider，逐次按键对比。修复确实做到了描述里说的事情，而它不该动的三条路径与 环境
跑之前先核对了 bundle 出处，确认两个二进制确实是两个 revision：
结果 —— 5 个场景，两份构建
A —— 问题复现（#8201），修复前： ESC #1 把队列弹进输入框、模型还在输出；ESC #2 清空输入框（排队文本就此丢失）；ESC #3 才真正取消。（见上文第一张图） A —— 修复后： 一次 ESC 就取消，并且排队消息不丢 —— 取消路径把它回灌进了输入框。（第二张图） B —— 同时有草稿和队列（更麻烦的情况）： C —— 不该被影响的基线。 流式中输入框为空、队列为空：两份构建都是一次 ESC 取消，账本完全一致 —— 都在 D + E —— 回归检查。 流式中 ↑ 依然能取到队列；已输入文本的双击 ESC 清空契约完好（ESC #1 保留草稿并提示 网络层证据场景 A 的账本（同一脚本、同一时序）： 场景 C 的账本（守卫不该影响的路径）： 单测与变异测试
说明（非阻塞项）
结论：LGTM —— 根因分析正确，修复足够小，相邻的每条 ESC 路径都被证明未受影响。 |
…t review Ten findings from a 9-agent review round plus one reverse-audit round over the PR's own diff; all verified against the cited lines before fixing. - Step 4 k=0 batch: pre-confirmed [build]/[test] entries enter the cumulative list untagged (the every-entry-tagged wording would have sent deterministic findings into the tag backstop and capped the verdict); the no-shard clause covers the all-pre-confirmed case, not just zero findings. - Convergence pair x retroactive-dry: the two rules now compose instead of conflicting - a pair retired in full counts as the dry predecessor at round 3's return; round k-1 counts launches, not labels; verdicts are applied at a round's return before the dry consultation; the pair's verification is one build over the deduped union passed as --round 2. - Tail batching: a group (3) that cannot be resolved no longer gates cleanup forever - disclose, skip record_artifact, preserve the inputs beside the report, and still run cleanup so the bypass audit and the completion line always happen; the old before-cleanup ordering sentence now names the group (3)/(4) split instead of contradicting it. - Accuracy: "byte-identical" weakened to what the pipelined merge rule actually guarantees, and the pair's round-2 suppression window named as the already-accepted trade; the #8353 incident recast to the forward pairing the rule licenses (round 5 was the waste, not rounds 4-5) with the ledger-vs-audit round numbering disambiguated. - save-artifact: comment block and test comments no longer describe the flag as embedder-only - it is the containment anchor the skill passes on every run; new CLI option contract test drives --workspace-root through real yargs into saveReviewArtifact (mirrors the test-plan/test-delta precedent for camel-case flag regressions).
…t review Ten findings from a 9-agent review round plus one reverse-audit round over the PR's own diff; all verified against the cited lines before fixing. - Step 4 k=0 batch: pre-confirmed [build]/[test] entries enter the cumulative list untagged (the every-entry-tagged wording would have sent deterministic findings into the tag backstop and capped the verdict); the no-shard clause covers the all-pre-confirmed case, not just zero findings. - Convergence pair x retroactive-dry: the two rules now compose instead of conflicting - a pair retired in full counts as the dry predecessor at round 3's return; round k-1 counts launches, not labels; verdicts are applied at a round's return before the dry consultation; the pair's verification is one build over the deduped union passed as --round 2. - Tail batching: a group (3) that cannot be resolved no longer gates cleanup forever - disclose, skip record_artifact, preserve the inputs beside the report, and still run cleanup so the bypass audit and the completion line always happen; the old before-cleanup ordering sentence now names the group (3)/(4) split instead of contradicting it. - Accuracy: "byte-identical" weakened to what the pipelined merge rule actually guarantees, and the pair's round-2 suppression window named as the already-accepted trade; the #8353 incident recast to the forward pairing the rule licenses (round 5 was the waste, not rounds 4-5) with the ledger-vs-audit round numbering disambiguated. - save-artifact: comment block and test comments no longer describe the flag as embedder-only - it is the containment anchor the skill passes on every run; new CLI option contract test drives --workspace-root through real yargs into saveReviewArtifact (mirrors the test-plan/test-delta precedent for camel-case flag regressions).
…needed (QwenLM#8642) * perf(review): remove serial wall-clock the audit loop and tail never needed Four latency cuts, each backed by stage-level timings extracted from six real CI review runs (2026-08-05/06); none changes what a review covers or what evidence a verdict requires. - Step 4's initial verification rides with the first reverse-audit launch (the k=0 case of the existing pipelined-loop rule). One measured run held its round-1 auditor 22 minutes behind verdicts the launch never consumed, while a sibling run the same day launched the two together. - Reverse-audit rounds 1 and 2 become the convergence pair on 3A plans: two independent auditors launched together, both-dry = the two dry audits the criterion already demands. A dry round leaves the findings list unchanged, so serial round 2's input was byte-identical to round 1's; the pair removes the wall clock between them, not either audit. Also spares the budget-stop cap a run took for want of a second dry round it had time to run in parallel but not in series. - A reporting round whose every finding the verifier rejected is retroactively dry: rejections already remove the entries, but not the reset they applied to the dry counter, and one measured run spent two extra rounds re-proving what round 2 plus a rejection had already answered. - save-artifact resolves its workspace root from cwd (or an explicit --workspace-root), not QWEN_CODE_PROJECT_DIR - the harness exports that variable as the session-storage dir under the runtime base, never the main checkout, and all six runs burned minutes rediscovering it before improvising workarounds. Steps 8/9 also batch the tail bookkeeping into four responses instead of one command per model turn. * perf(review): address review feedback on the pipeline-latency changes - Pin the cwd default with the trap armed: the regression test now chdirs into the root, passes no workspace root, and points QWEN_CODE_PROJECT_DIR at a decoy - reintroducing the removed env preference turns it red (verified by negative control). - Have the skill pass --workspace-root explicitly in the Step 8 block, so the containment anchor never depends on where the command happened to run. - Pin the retroactive-dry pairing direction: forward only, consulted on a round's return; the upgrade never ends the loop by itself and never discards an in-flight round. - Make the Step 4 batch self-contained (findings files written before either prompt set is built), and gate tail group 4 on group 3's success so cleanup cannot destroy save-artifact's only inputs after a failure. - Add the defensive rule for a deadline-refused pair member: launch round 1 alone and treat the refusal as the budget stop. * perf(review): fix rule conflicts and stale claims found by multi-agent review Ten findings from a 9-agent review round plus one reverse-audit round over the PR's own diff; all verified against the cited lines before fixing. - Step 4 k=0 batch: pre-confirmed [build]/[test] entries enter the cumulative list untagged (the every-entry-tagged wording would have sent deterministic findings into the tag backstop and capped the verdict); the no-shard clause covers the all-pre-confirmed case, not just zero findings. - Convergence pair x retroactive-dry: the two rules now compose instead of conflicting - a pair retired in full counts as the dry predecessor at round 3's return; round k-1 counts launches, not labels; verdicts are applied at a round's return before the dry consultation; the pair's verification is one build over the deduped union passed as --round 2. - Tail batching: a group (3) that cannot be resolved no longer gates cleanup forever - disclose, skip record_artifact, preserve the inputs beside the report, and still run cleanup so the bypass audit and the completion line always happen; the old before-cleanup ordering sentence now names the group (3)/(4) split instead of contradicting it. - Accuracy: "byte-identical" weakened to what the pipelined merge rule actually guarantees, and the pair's round-2 suppression window named as the already-accepted trade; the QwenLM#8353 incident recast to the forward pairing the rule licenses (round 5 was the waste, not rounds 4-5) with the ledger-vs-audit round numbering disambiguated. - save-artifact: comment block and test comments no longer describe the flag as embedder-only - it is the containment anchor the skill passes on every run; new CLI option contract test drives --workspace-root through real yargs into saveReviewArtifact (mirrors the test-plan/test-delta precedent for camel-case flag regressions). * perf(review): shard the convergence pair's verification per verifyShard Round-2 reverse audit: the pair bullet's "one verify build over the deduped union" contradicted Step 4's verifyShard sharding (quality collapses past 8 findings per verifier) and its own plural "the pair's verifiers". The rule now shards the deduped union exactly as any reporting round's findings are, every shard passed as --round 2; the per-member ban is unchanged. --------- Co-authored-by: verify <verify@local>
|
Released in v0.21.8. |
4 participants
What this PR does
When the agent is actively responding (
streamingState === Responding), ESC now falls through to AppContainer's global handler which cancels the ongoing request, instead of being consumed by InputPrompt's pop-queue / double-ESC-clear logic.Why it's needed
Users had to press ESC 3 times to stop the agent when they had queued messages: first ESC popped the queue into the input box, second ESC cleared the input, third ESC finally canceled the work. The global cancel-work handler in AppContainer was never reached because InputPrompt always returned
truefor ESC when the input was focused.What changed
Two guards added to InputPrompt's ESC handler (
packages/cli/src/ui/components/InputPrompt.tsx):popQueueIntoInput()whenstreamingState === Responding-- the queue stays untouched, ESC propagates.false(don't consume ESC) whenstreamingState === Responding-- lets AppContainer's global handler fire its cancel-ongoing-request step.When the agent is idle, ESC behavior is unchanged: pop queue, double-ESC clear, etc. all work as before. The up-arrow key still pops queued messages into the input at any time.
Reviewer Test Plan
How to verify
InputPrompt.test.tsxpass (7/7).keeps generated attachment references out of submitted provenance) is a pre-existing flaky clipboard timeout, unrelated to this change.Tested on
Risk & Scope
false.Linked Issues
Fixes #8201