fix(cli): auto-restore prompt and preserve queue on cancel#4023
Merged
Conversation
…h Claude Code
When a user pressed ESC immediately after submitting a prompt (before the
model produced any meaningful output), qwen-code left the cancelled prompt
stranded in the transcript and in cross-session ↑-history. Cancelling
during tool execution also silently dropped any queued follow-up input.
Mirror Claude Code's auto-restore-on-interrupt:
- Drain the queue back into the input buffer on EVERY cancel path,
including tool-execution cancels (replaces the unconditional
clearQueue() that motivated #3204 with a non-destructive pop).
- When the user cancels with no draft text, no queued input, and no
meaningful pending/committed assistant content, truncate the user
item and trailing INFO from history and pull the prompt text back
into the input box for editing.
- Add Logger.removeLastUserMessage so the disk-backed cross-session
↑-history (getPreviousUserMessages) is also cleaned on cancel.
The "meaningful content" check matches Claude Code's
messagesAfterAreOnlySynthetic: gemini text and tool runs are meaningful;
info/error/warning/retry/notification/tool_use_summary/thoughts are
synthetic. truncateToItem uses functional setState so it batches with
the INFO addItem from cancelOngoingRequest in the same render pass —
no flicker.
Tests cover all five guard branches and the logger undo across normal,
no-op, one-shot, MODEL_SWITCH-interleaved, disk-rotation, and
uninitialized cases.
Contributor
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
wenshao
commented
May 10, 2026
Without this, a transient writeFile error during a USER logMessage left the undo tracker pointing at the previous successful entry. A subsequent removeLastUserMessage (e.g., from auto-restore on cancel) would then silently delete an unrelated earlier row from disk-backed history. Add a regression test that mocks a writeFile rejection and asserts the tracker is null and the prior entry survives. Reported in PR review.
wenshao
commented
May 10, 2026
wenshao
left a comment
Collaborator
Author
There was a problem hiding this comment.
wenshao
commented
May 10, 2026
wenshao
left a comment
Collaborator
Author
There was a problem hiding this comment.
Overall: solid implementation of auto-restore-on-cancel, well-tested (117/117 pass), good code quality. One Suggestion inline.
— DeepSeek/deepseek-v4-pro via Qwen Code /review
…serialize writes
PR-review follow-up addressing two issues in the cancel-undo path.
1. Logger instance mismatch (Critical):
`useGeminiStream` and `AppContainer` each called `useLogger()`, which
instantiates a fresh `Logger` per call. `lastLoggedUserEntry` lives on
the instance, so the undo invoked from `AppContainer` was always a
no-op — the cancelled prompt still surfaced via cross-session
`getPreviousUserMessages`. Move the `useLogger` ownership to
`AppContainer` and pass the same instance into `useGeminiStream` via a
new optional `logger` parameter.
2. Logger write ordering:
Both `logMessage` and `removeLastUserMessage` do read → splice/append
→ writeFile without a lock. A fast cancel-then-resubmit could let
`removeLast` clobber a just-appended new entry. Add a per-instance
`serialize()` helper (a Promise-chained write queue) and route both
mutating ops through it. Reset the queue on `close()`. New regression
test fires removeLast and a fresh logMessage in parallel and asserts
the resubmitted entry survives.
3. Stale React-state race in cancel guard (Suggestion):
The auto-restore guard read `pendingGeminiHistoryItems` from React
state, which can lag a stream chunk that just set
`pendingHistoryItemRef.current`. Snapshot the pending item at the
start of `cancelOngoingRequest` and pass it through the new
`onCancelSubmit({ pendingItem })` info parameter. The guard combines
it with the React-state items so any meaningful in-flight content
blocks auto-restore even before re-render. New test covers the case
where pendingHistoryItems is empty but info.pendingItem carries
`gemini_content`.
All touched-area suites pass: 64 cli AppContainer, 9 historyUtils,
85 useGeminiStream, 46 core logger.
…cel-handler test types
The pre-commit eslint --fix on the previous commit collapsed the two
consecutive `import { ... } from '@qwen-code/qwen-code-core'` blocks in
useGeminiStream.ts into a single statement, but kept the `import type`
modifier from the first block — silently turning every runtime symbol
(SendMessageType, MessageSenderType, GitService, ApprovalMode, …) into
type-only imports. tsc rejected with TS2206 + a wave of TS1361 errors
that only surfaced on CI.
Restore the two separate imports: pure-type symbols (Logger included)
in `import type { ... }`, runtime symbols in plain `import { ... }`.
Also: the AppContainer cancel-handler tests captured `onCancelSubmit`
as `() => void`, but the hook signature now takes an optional info
arg. Widen the captured-callback type so passing `{ pendingItem }`
typechecks (TS2554 on line 1053).
wenshao
commented
May 10, 2026
wenshao
left a comment
Collaborator
Author
There was a problem hiding this comment.
— deepseek-v4-pro via Qwen Code /review
Four follow-ups from a /review pass on the auto-restore-on-cancel path. * logger.ts — only invalidate `lastLoggedUserEntry` when the failed write was itself a USER attempt. A failed non-USER write (MODEL_SWITCH on a transient disk error, etc.) doesn't change which row was the most recent user prompt, so the prior undo target is still valid. Without this, MODEL_SWITCH disk hiccups silently disabled cancel-undo. * useGeminiStream.ts — wrap `onCancelSubmit` in try/finally so a throw in AppContainer's cancel handler can't strand the stream in Responding (the UI would lock — Esc would no-op until process restart). `setIsResponding(false)` and `setShellInputFocused(false)` always run. * useGeminiStream.ts — also document the three-way coupling between the INFO `addItem` here and AppContainer's auto-restore guard: the guard reads `historyRef.current` which doesn't yet contain this INFO (React batches), and the guard's correctness depends on the items added here staying synthetic. * historyUtils.ts — make `isSyntheticHistoryItem` exhaustive over the 35-member `HistoryItemWithoutId` union. Every case is explicit; the default branch carries a `_exhaustive: never` so adding a new HistoryItem variant without classifying it triggers a compile-time error rather than silently disabling auto-restore. Runtime fallback is "meaningful" (safe — bail rather than wipe content). Tests: +1 logger case (non-USER failure preserves the USER tracker), +1 useGeminiStream case (throwing handler still flushes Responding). All touched suites pass: 47 logger, 9 historyUtils, 86 useGeminiStream, 64 AppContainer.
…eckpoints) Reword the comment above `writeQueue` and the `serialize()` JSDoc to state explicitly that the queue only serializes log-history mutations (`logMessage` / `removeLastUserMessage`). Checkpoint ops (saveCheckpoint / deleteCheckpoint / loadCheckpoint) touch separate files and intentionally don't share this queue, so the previous "every disk-mutating op chains here" wording overstated the guarantee.
…m on cancel Stream content/thought events are throttled into a per-turn `bufferedEvents` array; only when `flushBufferedStreamEvents` runs do they reach `pendingHistoryItemRef.current`. Snapshotting BEFORE the flush meant cancels that fired inside the throttle window (60ms) saw a null `pendingItem` even when meaningful text was sitting in the buffer. AppContainer's auto-restore guard then read null, decided "model produced nothing", and called `truncateToItem` — which silently wiped the very content that the subsequent `addItem(pendingHistoryItemRef.current)` had just committed. Move the snapshot to AFTER the flush so it sees the same value as the addItem call directly below it. Regression test: yields a content event and cancels without advancing fake timers, asserts `info.pendingItem` carries the buffered "partial response" text rather than null.
wenshao
commented
May 12, 2026
wenshao
left a comment
Collaborator
Author
There was a problem hiding this comment.
No blocking issues found. One Suggestion inline for minor code clarity improvement.
— DeepSeek/deepseek-v4-pro via Qwen Code /review
…tore The auto-restore branch was cleaning up two of the three places a cancelled prompt lives — the UI transcript via `truncateToItem` and the disk-backed ↑-history via `Logger.removeLastUserMessage`. The third — the in-memory chat history on `GeminiChat` — was left untouched. `sendMessageStream` appends the user content to `chat.history` BEFORE the stream generator runs and the abort path doesn't pop it. After a successful auto-restore the next request's wire payload still carried the cancelled prompt as a leading user turn alongside the new prompt, so the model saw context the user believed had been undone (and in some shapes the API would reject two consecutive user turns). Mirror the existing strip the Retry submit path uses (`GeminiClient.sendMessageStream` at the `Retry` branch): make `GeminiClient.stripOrphanedUserEntriesFromHistory` public and call it from the auto-restore success path, sitting next to the UI truncate and the disk-log undo. The method already pops trailing user entries and clears the `FileReadCache` (which can otherwise hold dangling `read_file` results from the stripped turn). End-to-end reproduction from the PR review: 1. Submit `what time is it?` → ESC during pre-token delay → auto-restore (UI rewound, buffer pre-filled). 2. Edit buffer to `what year is it?` → submit. 3. Pre-fix: outbound `messages` carried both prompts as consecutive user turns. Post-fix: only the new prompt. Test: extend the auto-restore-success AppContainer test with a mock `stripOrphanedUserEntriesFromHistory` spy and assert it fires. The non-restore branches don't install the spy (it's optionally chained at the call site).
Collaborator
Author
|
@tanzhenxin 已采纳第三处清理，commit 1238206： 改动：
为什么沿用现成函数：它本就在 Retry submit 路径用过同样的语义 —— pop trailing user entries 并 clear FileReadCache（避免 stripped turn 留下的 read_file functionResponse 让后续 Read 命中 file_unchanged 占位符）。 回归测试：扩展 auto-restore-success 用例，在 按你给的端到端场景再走一遍：第二次请求的 wire payload 现在只剩新 prompt，被取消的那一条不再出现。 Re: your review |
wenshao
commented
May 12, 2026
wenshao
left a comment
Collaborator
Author
There was a problem hiding this comment.
[Critical] SendMessageType.Retry bypasses prepareQueryForGemini, so lastTurnUserItemRef.current is not reset for retry turns. If a prior normal prompt populated the ref, then a Ctrl+Y retry is cancelled before meaningful output, cancelOngoingRequest can pass stale ownership info and AppContainer may rewind the older user prompt even though retry did not add a new user history item. Reset the ownership ref before the retry fast path, or make retry cancellation explicitly report lastTurnUserItem: null, so retry cancels cannot target a user item from an earlier turn.
[Critical] lastTurnUserItemRef.current is set after addItem({ type: USER, text }), but useHistory.addItem silently skips consecutive duplicate user messages with the same text. Manually submitting the same prompt again and cancelling immediately can make auto-restore treat the older identical user item as belonging to the cancelled turn and truncate it. The ownership token should come from the actual inserted history item, not just the text: have addItem report whether it appended, set lastTurnUserItemRef only when insertion happened, and have CancelSubmitInfo/AppContainer verify that id instead of relying on text equality.
— gpt-5.5 via Qwen Code /review
…Item and dup-skip identity contracts
Three follow-ups from PR review batch:
* core/logger.ts — `serialize()` was `this.writeQueue.then(op, op)`.
The second callback was dead code: `writeQueue` is seeded with
`Promise.resolve()` and reassigned through `.catch(() => undefined)`,
so the queue tail can never reject. Worse, `then(op, op)` reads as
"retry op on rejection" — wrong intent. Switch to `.then(() => op())`
with a comment spelling out the no-reject invariant.
* cli/useGeminiStream.test.tsx — add ownership-contract tests at the
PRODUCER side of `info.lastTurnUserItem`. Until now only the
AppContainer tests pinned the contract, and they fabricate the
value, so a regression that drops `lastTurnUserItemRef.current = {
text: trimmedQuery }` in `prepareQueryForGemini` would slip
through. New tests:
- normal `UserQuery` submit → cancel → assert
`info.lastTurnUserItem === { text: 'what time is it?' }`.
- `SendMessageType.Notification` submit → cancel → assert
`info.lastTurnUserItem === null` (path doesn't push a user
history item, the ref reset at the top of
prepareQueryForGemini must keep it null).
* core/logger.test.ts — strengthen the duplicate-skip regression.
The previous test only checked the tracker advanced text; the
important identity contract is that the recalculated 5-tuple
matches the disk row, so a subsequent `removeLastUserMessage()`
removes the duplicate-skipped row rather than the older USER.
New test seeds disk with [first, second], stubs `_updateLogFile`
for the second call to mimic the duplicate-skip branch (mutate
newEntryObject's messageId+timestamp to align with the disk row,
return null), then asserts removeLastUserMessage() leaves
['first'] on disk and removes 'second'.
wenshao
commented
May 12, 2026
wenshao
left a comment
Collaborator
Author
There was a problem hiding this comment.
No blocking issues found. Second opinion with glm-5.1 after 5+ prior review rounds (DeepSeek-v4-pro × 4, Copilot, human @tanzhenxin).
Changes since last review (2 commits):
- Fix
serializeto usethen(() => op())instead ofthen(op, op)— eliminates the misleading retry semantics on error recovery. - Make
stripOrphanedUserEntriesFromHistorypublic and add it as the third cleanup leg in auto-restore — prevents cancelled prompt from riding along on the next API request as an orphan user turn. - Pin
lastTurnUserItemproducer-side contract with tests (UserQuery emits text, Notification emits null). - Pin duplicate-skip identity contract with regression test.
Assessment: Deterministic analysis clean (tsc 0, eslint 0). Build PASS, Tests PASS (216/216). 9 review agents + 1 reverse-audit round found no high-confidence Critical or Suggestion issues. Three low-confidence observations in terminal output only (not posted inline to avoid noise).
— glm-5.1 via Qwen Code /review
tanzhenxin
requested changes
May 12, 2026
tanzhenxin
left a comment
Collaborator
There was a problem hiding this comment.
Review
The latest wave — the in-memory chat-history strip (the third cleanup leg), the duplicate-skip shift in the user-log writer, and the write-queue signature cleanup — all look correct and well-tested. The third cleanup leg is the right call, and the comment explaining the orphan-user-turn risk on the next request is excellent. One correctness gap remains.
1. The cancelled prompt stays printed in the terminal after auto-restore (severity: medium · confidence: very high)
Reproduced on the PR's stated golden path. Type what time is it?, press Enter, press ESC before any output. The transcript still shows > what time is it? in the static-rendered region, AND the input buffer is pre-filled with the same text — the user sees the prompt twice. Reproduced at both ~0.5s and ~2s Enter-to-ESC delays, so it's not a timing edge case.
The auto-restore branch truncates React history state but never refreshes Ink's static transcript region. Static is append-only — once an item has been printed, changing React state alone won't unprint it. The existing Ctrl+O rewind path does the same kind of truncate but follows it with an explicit static-refresh (which writes the terminal-clear escape and remounts) for exactly this reason. Auto-restore omits that refresh, so the React tree, the disk log, and the in-memory chat history all rewind correctly while the visible transcript does not. The new tests don't catch it because they mock the history manager and never exercise a real Ink render.
Verdict
REQUEST_CHANGES — visible regression on the PR's stated golden path, reproduced on a real terminal. The fix is small.
wenshao
commented
May 12, 2026
wenshao
left a comment
Collaborator
Author
There was a problem hiding this comment.
…PR review Four critical findings from gpt-5.5 /review pass: 1. **Retry skipped the lastTurnUserItem reset** (useGeminiStream.ts) `Retry` bypasses `prepareQueryForGemini`, which is where the `lastTurnUserItemRef.current = null` reset lived. A retry that followed a normal `UserQuery` carried the stale ownership snapshot into `onCancelSubmit`, and cancelling the retry before any meaningful output let `AppContainer` auto-restore truncate the original failed prompt. Move the reset (and the new content-seen reset, see #4) to the top of `submitQuery`, gated only on "this is a top-level submit" — covers Retry, Cron, Notification, and ordinary UserQuery alike. 2. **Text-only ownership matched dedup'd duplicates** (AppContainer.tsx, useGeminiStream.ts) `useHistoryManager.addItem` skips inserting a consecutive-duplicate user message while still returning a freshly generated id. The text-only ownership check would match the OLDER identical-text USER row, so a re-submitted same prompt + cancel would wrongly truncate the prior turn. Carry id+text in `CancelSubmitInfo.lastTurnUserItem` (using `addItem`'s return value) and require both id AND text to match before truncating. 3. **stripOrphan left IDE context state advanced** (client.ts) Other history-mutating paths (`setHistory`, `truncateHistory`) set `forceFullIdeContext = true` after mutating; the orphan-strip didn't, so a subsequent request could send a diff against a removed baseline. Gate cache-clear + IDE-context invalidation on an actual before/after length drop, so no-op strips don't churn state. 4. **Flush-then-thought race let auto-restore wipe committed content** (useGeminiStream.ts, AppContainer.tsx) `cancelOngoingRequest`'s pre-cancel flush can `addItem` a meaningful `gemini_content` (via handleContentEvent's split path) and then a later thought event overwrites `pendingHistoryItem` with a synthetic value. The AppContainer guard's React history snapshot is stale, so the trailing-only-synthetic check passes and the just-committed text gets truncated. Track a synchronous `turnSawContentEventRef` set in handleContentEvent, ship it through `CancelSubmitInfo`, and make the guard bail when set. Tests: - core/client.test.ts: stripOrphan only forces full IDE context on actual removal; existing retry tests updated to mock `getHistoryLength`. - cli/useGeminiStream.test.tsx: ownership uses { id, text }, Retry reset works after a prior UserQuery cancel, turnProducedMeaningfulContent flips true when content lands. - cli/AppContainer.test.tsx: guard bails on `turnProducedMeaningfulContent: true`, guard bails on id mismatch (catches addItem dedup case). cli 162/162 + core client 99/99 + core logger 51/51.
Reported by @tanzhenxin: auto-restore truncated React `history` state but the cancelled `> prompt` and `Request cancelled.` lines stayed printed in the terminal — Ink's `<Static>` region is append-only, so shrinking the underlying array doesn't unprint already-flushed lines. On the PR's golden path (type prompt → Enter → ESC) the user sees the prompt twice: once in scrollback, once pre-filled in the input buffer. Confirmed at multiple Enter-to-ESC delays, so it's not a timing fluke. Call `refreshStatic()` immediately after `truncateToItem(...)` in the auto-restore success path. `refreshStatic` writes the ANSI clear-terminal escape AND bumps the static remount key — the exact recipe `/clear` (`handleClearScreen`) already uses for the same reason. The targeted-repaint helper used for terminal resizes is intentionally NOT used here: it preserves scrollback, which would leave the cancelled prompt visible above the new viewport. Test: extend the existing auto-restore happy-path AppContainer test to assert `mockStdout.write` was called with `ansiEscapes.clearTerminal`. The other auto-restore-bail tests don't install the assertion so they naturally verify the negative case (no clear when guard rejects).
Collaborator
Author
|
@tanzhenxin 已采纳，commit fdd6b06 修复。 Root cause 你描述得很准：Ink Fix：auto-restore 成功分支里 不用更轻量的 Test：在 Re: your review |
Comment on lines
866
to
874
| // Track output chars for real-time token estimation & mark as receiving. | ||
| streamingResponseLengthRef.current += eventValue.length; | ||
| setIsReceivingContent(true); | ||
| // Pin "this turn produced meaningful content" so the cancel | ||
| // handler's snapshot reflects content events even when they land | ||
| // during the pre-cancel flush (their addItem hasn't re-rendered | ||
| // React history by the time AppContainer's guard runs). | ||
| turnSawContentEventRef.current = true; | ||
| let newGeminiMessageBuffer = currentGeminiMessageBuffer + eventValue; |
Collaborator
Author
|
null |
tanzhenxin
approved these changes
May 13, 2026
tanzhenxin
left a comment
Collaborator
There was a problem hiding this comment.
Review
Both critical findings from the prior round land cleanly. The retry-cancel disk erasure is gone — the ownership-ref reset now sits at the top of every top-level submit, so Retry no longer carries a stale snapshot into its cancel info. The Ink <Static> repaint also matches the rewind handler's recipe, so the cancelled prompt actually disappears from the terminal on the golden path. The three additional mechanisms in the same wave (the content-event flag for the flush race, id-based ownership for the consecutive-duplicate dedup, and length-gated cache invalidation in the orphan strip) each address a distinct gap and come with focused tests.
Verdict
APPROVE — both critical findings closed.
wenshao
commented
May 13, 2026
wenshao
left a comment
Collaborator
Author
There was a problem hiding this comment.
The following two findings could not be anchored to a diff hunk (the affected lines are pre-existing context that this PR's new auto-restore path now critically depends on); recording in the body instead.
F4 (Suggestion) — packages/cli/src/ui/AppContainer.tsx:271-272
historyRef.current = historyManager.history is assigned during the render phase. The sibling pattern at packages/cli/src/ui/hooks/useGeminiStream.ts:343-346 uses useLayoutEffect and explicitly warns that "writing refs in the render phase is unsafe under React's concurrent rendering". The PR's cancel handler now reads historyRef.current to compute findLastUserItemIndex and itemsAfterAreOnlySynthetic — these decisions are safety-critical (they decide whether to truncate the user's transcript), so the asymmetry is a latent landmine.
Fix: move to useLayoutEffect(() => { historyRef.current = historyManager.history; }, [historyManager.history]);, mirroring the useGeminiStream pattern.
F9 (Suggestion) — packages/cli/src/ui/hooks/useGeminiStream.ts:745
Duplicate consecutive submits leak a disk USER entry. await logger?.logMessage(MessageSenderType.USER, trimmedQuery) runs unconditionally for every USER submit, but addItem(USER, ...) (~L798) skips consecutive duplicates and returns a fresh phantom id (useHistoryManager.ts:57-63). The disk gets a new entry every time; the UI history does not. lastTurnUserItemRef.current.id is then the phantom id, so on cancel the AppContainer guard's identity check lastUserItem.id !== cancelledTurnUserItem.id fails → bail "lastUserItem identity does not match" → removeLastUserMessage is never called.
Fix: either call removeLastUserMessage() before the identity bail (the disk persisted regardless), or short-circuit logMessage when addItem reports a dedup. Update the bail comment to acknowledge the dedup-leaked row.
— claude-opus-4-7 via Claude Code /qreview
| // this — paths that don't add a USER history item (Cron / | ||
| // Notification / slash submit_prompt) leave it null so cancel | ||
| // never wrongly targets an older user item. | ||
| lastTurnUserItemRef.current = null; |
Collaborator
Author
There was a problem hiding this comment.
[Critical] Concurrent /btw mid-stream clobbers the in-flight main turn's ownership snapshot.
submitQuery's reset at line 1629 is correctly gated on !allowConcurrentBtwDuringResponse so the btw path skips it. But prepareQueryForGemini is invoked unconditionally at line 1676 (only Retry skips), and this un-gated reset (followed by addItem(USER, '/btw …') at line 807) overwrites the main turn's snapshot to point at the btw row. Same applies to await logger?.logMessage(USER, '/btw …') at line 745, which advances lastLoggedUserEntry to the btw entry.
User submits "long task" → submits /btw … mid-throttle → ESC-cancels long task. info.lastTurnUserItem now holds the btw row's id and text. Either (a) the auto-restore guard's identity check matches the btw row → truncateToItem truncates the btw user item from the UI and removeLastUserMessage deletes the wrong entry from disk; or (b) it doesn't match → bail, but lastLoggedUserEntry still points at btw, so any later removeLastUserMessage would target the wrong row. Untested.
Fix: drop this reset and rely solely on submitQuery's gated L1629 reset. Add a regression test that submits a UserQuery, then a /btw … mid-stream, then cancels — assert info.lastTurnUserItem still holds the original UserQuery's id/text.
— claude-opus-4-7 via Claude Code /qreview
| // contradict what the UI told the user was rewound. Mirrors the | ||
| // existing strip in the Retry submit path | ||
| // (GeminiClient.sendMessageStream). | ||
| geminiClient?.stripOrphanedUserEntriesFromHistory?.(); |
Collaborator
Author
There was a problem hiding this comment.
[Critical] geminiClient?.stripOrphanedUserEntriesFromHistory?.() runs without try/catch. The method delegates to getChat() (packages/core/src/core/client.ts:294-298) which throws 'Chat not initialized' when this.chat is undefined. Reachable when the user submits before chat init completes (addItem(USER) is synchronous and sets lastTurnUserItem; chat = new GeminiChat(...) runs later in the same flow), then immediately ESC-cancels.
By the time line 1516 runs, the cancel handler has already executed historyManager.truncateToItem, refreshStatic, and buffer.setText — so a throw skips the next-line logger?.removeLastUserMessage() (cancelled prompt resurrects in next session's ↑-history) and the error bubbles into the Ink keypress handler. The success-path debug message ("auto-restore: rewinding…") still prints, which is misleading — the operation did NOT fully succeed.
Suggested change
| geminiClient?.stripOrphanedUserEntriesFromHistory?.(); | |
| try { | |
| geminiClient?.stripOrphanedUserEntriesFromHistory?.(); | |
| } catch (err) { | |
| debugLogger.debug('auto-restore: stripOrphanedUserEntriesFromHistory failed', err); | |
| } |
— claude-opus-4-7 via Claude Code /qreview
| // Entry already gone from disk (concurrent rotation/clear). | ||
| // Adopt disk state as truth so the in-memory cache doesn't | ||
| // diverge from a freshly-rotated file. | ||
| this.logs = currentLogsOnDisk; |
Collaborator
Author
There was a problem hiding this comment.
[Critical] Corrupt logs.json silently wipes the entire in-memory ↑-history beyond the optimistically-removed target.
_readLogFile silently rewrites a malformed-array (line ~130) or invalid-JSON (line ~151) file as [] and returns [] (no throw). The idx === -1 branch then sets this.logs = currentLogsOnDisk (= []), wiping more than just the target. The caller already optimistically removed the target before this; this branch then clobbers everything else too. The .invalid_json.<ts>.bak file on disk is the only forensic trail; no signal reaches AppContainer (the false return is swallowed at the call site — see the AppContainer:1525 finding).
Fix: distinguish the two [] cases inside _readLogFile (return a tagged result) so removeLastUserMessage can choose between "external rotation, adopt disk truth" and "corruption, preserve in-memory unchanged". Failing that, at minimum wrap the idx === -1 corruption case with restoreOptimistic() instead of overwriting this.logs wholesale, and emit a distinguishable debug breadcrumb so oncall can correlate.
— claude-opus-4-7 via Claude Code /qreview
| // handler's snapshot reflects content events even when they land | ||
| // during the pre-cancel flush (their addItem hasn't re-rendered | ||
| // React history by the time AppContainer's guard runs). | ||
| turnSawContentEventRef.current = true; |
Collaborator
Author
There was a problem hiding this comment.
[Suggestion] turnSawContentEventRef.current = true is set BEFORE the whitespace-only short-circuit at line 879-881 (newGeminiMessageBuffer.trim().length === 0 → return).
A model that streams only whitespace tokens (\n\n, ) before the user cancels still flips the ref to true, which causes info.turnProducedMeaningfulContent: true and the AppContainer auto-restore guard at line ~1437 to bail — even though no addItem or setPendingHistoryItem actually ran. This duplicates the open Copilot inline comment on this line that has no reply.
False-positive on the auto-restore guard. The "model emitted only whitespace before cancel" scenario silently regresses to pre-PR behavior (cancelled prompt left in transcript, not restored to buffer).
Fix: move the turnSawContentEventRef.current = true assignment AFTER the early-return so it only fires when content actually advances to a gemini/gemini_content pending item — e.g., place it just before the splitPoint/addItem work that follows the trim guard. Add a useGeminiStream regression test feeding [{ type: Content, value: '\n\n' }] and asserting info.turnProducedMeaningfulContent === false.
— claude-opus-4-7 via Claude Code /qreview
| // errors and returns false, but attach a .catch as defence so a | ||
| // future code path that throws doesn't surface as an | ||
| // UnhandledPromiseRejection. | ||
| void logger?.removeLastUserMessage().catch((err: unknown) => { |
Collaborator
Author
There was a problem hiding this comment.
[Suggestion] void logger?.removeLastUserMessage().catch(...) — the .catch only fires on thrown errors. removeLastUserMessage returns false on five separate failure modes (uninitialized, no tracker, external rotation, read-fail with rollback, write-fail with rollback) and none of those false returns are observable from this call site.
"Cancelled prompt resurrects next session" repro path is silently undebuggable. Oncall cannot tell from a single debug log whether (a) Logger was uninitialized, (b) tracker was null because the prior write failed, (c) the file got rotated externally, (d) read threw, or (e) write threw — each has different remediation.
Fix: capture the boolean return:
void logger?.removeLastUserMessage()
.then((ok) => {
if (!ok) debugLogger.debug('auto-restore: removeLastUserMessage returned false');
})
.catch((err: unknown) => {
debugLogger.debug('Failed to undo cancelled prompt from log:', err);
});Optionally, change removeLastUserMessage's return type to a discriminated union ('ok' | 'no-tracker' | 'rotated' | 'read-fail' | 'write-fail') so reasons are machine-queryable.
— claude-opus-4-7 via Claude Code /qreview
| // Always drain the queue back into the buffer (claude-code parity: | ||
| // popAllEditable preserves queued text on every cancel path, including | ||
| // tool-execution cancels — never silently drop the user's queued work). | ||
| const popped = popAllMessages(); |
Collaborator
Author
There was a problem hiding this comment.
[Suggestion] popAllMessages() joins all queued messages with \n\n including any queued slash commands. drainQueue filters them (useMessageQueue.ts:57) but the cancel path uses popAllMessages. Slash commands ARE allowed into the queue (the cancel-time fall-through adds them when streamingState !== Idle).
Queued /clear (or /quit, /auth, etc.) lands verbatim in the input buffer on cancel. If the user hits Enter without scrolling, the slash command runs — /clear wipes the session, /quit exits. Pre-PR clearQueue() dropped the queue so this couldn't happen; the new "preserve queue text" path is more user-friendly for plain text but unsafe for slash commands.
Fix: reuse the slash/non-slash split that drainQueue already implements — drop slash commands from the popped text (or surface them as a discarded-notification), and only push plain-text fragments into the buffer.
— claude-opus-4-7 via Claude Code /qreview
TaimoorSiddiquiOfficial
added a commit
to TaimoorSiddiquiOfficial/HopCode
that referenced
this pull request
May 13, 2026
…) [upstream cherry-pick] Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
wenshao
added a commit
that referenced
this pull request
May 17, 2026
* fix(cli): auto-restore prompt and preserve queue on cancel; align with Claude Code
When a user pressed ESC immediately after submitting a prompt (before the
model produced any meaningful output), qwen-code left the cancelled prompt
stranded in the transcript and in cross-session ↑-history. Cancelling
during tool execution also silently dropped any queued follow-up input.
Mirror Claude Code's auto-restore-on-interrupt:
- Drain the queue back into the input buffer on EVERY cancel path,
including tool-execution cancels (replaces the unconditional
clearQueue() that motivated #3204 with a non-destructive pop).
- When the user cancels with no draft text, no queued input, and no
meaningful pending/committed assistant content, truncate the user
item and trailing INFO from history and pull the prompt text back
into the input box for editing.
- Add Logger.removeLastUserMessage so the disk-backed cross-session
↑-history (getPreviousUserMessages) is also cleaned on cancel.
The "meaningful content" check matches Claude Code's
messagesAfterAreOnlySynthetic: gemini text and tool runs are meaningful;
info/error/warning/retry/notification/tool_use_summary/thoughts are
synthetic. truncateToItem uses functional setState so it batches with
the INFO addItem from cancelOngoingRequest in the same render pass —
no flicker.
Tests cover all five guard branches and the logger undo across normal,
no-op, one-shot, MODEL_SWITCH-interleaved, disk-rotation, and
uninitialized cases.
* fix(core): clear lastLoggedUserEntry on logMessage write failure
Without this, a transient writeFile error during a USER logMessage left
the undo tracker pointing at the previous successful entry. A subsequent
removeLastUserMessage (e.g., from auto-restore on cancel) would then
silently delete an unrelated earlier row from disk-backed history.
Add a regression test that mocks a writeFile rejection and asserts the
tracker is null and the prior entry survives.
Reported in PR review.
* fix(cli, core): share Logger across AppContainer/useGeminiStream and serialize writes
PR-review follow-up addressing two issues in the cancel-undo path.
1. Logger instance mismatch (Critical):
`useGeminiStream` and `AppContainer` each called `useLogger()`, which
instantiates a fresh `Logger` per call. `lastLoggedUserEntry` lives on
the instance, so the undo invoked from `AppContainer` was always a
no-op — the cancelled prompt still surfaced via cross-session
`getPreviousUserMessages`. Move the `useLogger` ownership to
`AppContainer` and pass the same instance into `useGeminiStream` via a
new optional `logger` parameter.
2. Logger write ordering:
Both `logMessage` and `removeLastUserMessage` do read → splice/append
→ writeFile without a lock. A fast cancel-then-resubmit could let
`removeLast` clobber a just-appended new entry. Add a per-instance
`serialize()` helper (a Promise-chained write queue) and route both
mutating ops through it. Reset the queue on `close()`. New regression
test fires removeLast and a fresh logMessage in parallel and asserts
the resubmitted entry survives.
3. Stale React-state race in cancel guard (Suggestion):
The auto-restore guard read `pendingGeminiHistoryItems` from React
state, which can lag a stream chunk that just set
`pendingHistoryItemRef.current`. Snapshot the pending item at the
start of `cancelOngoingRequest` and pass it through the new
`onCancelSubmit({ pendingItem })` info parameter. The guard combines
it with the React-state items so any meaningful in-flight content
blocks auto-restore even before re-render. New test covers the case
where pendingHistoryItems is empty but info.pendingItem carries
`gemini_content`.
All touched-area suites pass: 64 cli AppContainer, 9 historyUtils,
85 useGeminiStream, 46 core logger.
* fix(cli): unbreak build after import-merge regression and tighten cancel-handler test types
The pre-commit eslint --fix on the previous commit collapsed the two
consecutive `import { ... } from '@qwen-code/qwen-code-core'` blocks in
useGeminiStream.ts into a single statement, but kept the `import type`
modifier from the first block — silently turning every runtime symbol
(SendMessageType, MessageSenderType, GitService, ApprovalMode, …) into
type-only imports. tsc rejected with TS2206 + a wave of TS1361 errors
that only surfaced on CI.
Restore the two separate imports: pure-type symbols (Logger included)
in `import type { ... }`, runtime symbols in plain `import { ... }`.
Also: the AppContainer cancel-handler tests captured `onCancelSubmit`
as `() => void`, but the hook signature now takes an optional info
arg. Widen the captured-callback type so passing `{ pendingItem }`
typechecks (TS2554 on line 1053).
* fix(cli, core): tighten cancel-undo robustness from PR review batch 3
Four follow-ups from a /review pass on the auto-restore-on-cancel path.
* logger.ts — only invalidate `lastLoggedUserEntry` when the failed
write was itself a USER attempt. A failed non-USER write (MODEL_SWITCH
on a transient disk error, etc.) doesn't change which row was the
most recent user prompt, so the prior undo target is still valid.
Without this, MODEL_SWITCH disk hiccups silently disabled cancel-undo.
* useGeminiStream.ts — wrap `onCancelSubmit` in try/finally so a throw
in AppContainer's cancel handler can't strand the stream in
Responding (the UI would lock — Esc would no-op until process
restart). `setIsResponding(false)` and `setShellInputFocused(false)`
always run.
* useGeminiStream.ts — also document the three-way coupling between
the INFO `addItem` here and AppContainer's auto-restore guard:
the guard reads `historyRef.current` which doesn't yet contain
this INFO (React batches), and the guard's correctness depends on
the items added here staying synthetic.
* historyUtils.ts — make `isSyntheticHistoryItem` exhaustive over the
35-member `HistoryItemWithoutId` union. Every case is explicit; the
default branch carries a `_exhaustive: never` so adding a new
HistoryItem variant without classifying it triggers a compile-time
error rather than silently disabling auto-restore. Runtime fallback
is "meaningful" (safe — bail rather than wipe content).
Tests: +1 logger case (non-USER failure preserves the USER tracker),
+1 useGeminiStream case (throwing handler still flushes Responding).
All touched suites pass: 47 logger, 9 historyUtils, 86 useGeminiStream,
64 AppContainer.
* docs(core): clarify Logger writeQueue scope (log-history only, not checkpoints)
Reword the comment above `writeQueue` and the `serialize()` JSDoc to
state explicitly that the queue only serializes log-history mutations
(`logMessage` / `removeLastUserMessage`). Checkpoint ops
(saveCheckpoint / deleteCheckpoint / loadCheckpoint) touch separate
files and intentionally don't share this queue, so the previous
"every disk-mutating op chains here" wording overstated the
guarantee.
* fix(cli): flush buffered stream events before snapshotting pendingItem on cancel
Stream content/thought events are throttled into a per-turn `bufferedEvents`
array; only when `flushBufferedStreamEvents` runs do they reach
`pendingHistoryItemRef.current`. Snapshotting BEFORE the flush meant cancels
that fired inside the throttle window (60ms) saw a null `pendingItem` even
when meaningful text was sitting in the buffer. AppContainer's auto-restore
guard then read null, decided "model produced nothing", and called
`truncateToItem` — which silently wiped the very content that the
subsequent `addItem(pendingHistoryItemRef.current)` had just committed.
Move the snapshot to AFTER the flush so it sees the same value as the
addItem call directly below it.
Regression test: yields a content event and cancels without advancing
fake timers, asserts `info.pendingItem` carries the buffered "partial
response" text rather than null.
* fix(core): apply Logger.removeLastUserMessage in-memory removal synchronously
AppContainer's `userMessages` effect calls `getPreviousUserMessages()`
on the same render that history truncation fires (it depends on
`historyManager.history`). The previous implementation only updated
`this.logs` after `await fs.writeFile(...)` settled, so the effect
read stale logs and ↑-history surfaced the cancelled prompt until
some unrelated future history change re-ran the effect.
Move the cache filter ahead of the serialize queue so consumers see
the removal immediately. The async serialize op continues to read,
splice, and write disk, then re-syncs `this.logs` from disk on
success or rotation.
Regression test fires removeLast without awaiting, then asserts the
very next `getPreviousUserMessages()` returns [] (no cancelled
prompt), and that the background promise still resolves to true.
* docs(core, cli): clarify removeLastUserMessage contract; observability for cancel-undo
* logger.ts — extend the JSDoc on `removeLastUserMessage` to spell
out the two-phase semantics (sync optimistic in-memory removal +
async serialized disk reconciliation), and explicitly document that
the boolean return value reflects the *disk* outcome while the
in-memory cache is updated unconditionally. Also explain why disk
failures are NOT rolled back: rolling back would resurrect the
cancelled prompt in ↑-history, which is worse UX than a temporary
cache/disk divergence (which converges on next op or on
`initialize()` of the next session).
* AppContainer.tsx — wrap the fire-and-forget
`logger.removeLastUserMessage()` in `.catch(debugLogger.debug)`.
The Logger's internal try/catches mean the Promise should never
reject today, but a future code-path change shouldn't surface as
an UnhandledPromiseRejection — and a debug-level log is the right
observability hook for "cancel succeeded in UI but disk-undo
failed silently".
* fix(core,cli): #4023 review wave — logger atomicity + observable undo failure
3 #4023 review threads addressed:
- core/logger.ts: `removeLastUserMessage` now ROLLS BACK the
optimistic in-memory removal when the disk read or write fails.
Previously the JSDoc/return contract was violated: the method
returned `false` on failure but `this.logs` already showed the
entry removed — callers (AppContainer's `userMessages` effect)
saw the inconsistency and the cancelled prompt vanished from
↑-history despite the disk still carrying it. The rollback
re-inserts the target at its original index when no concurrent
mutation took its place, and restores `lastLoggedUserEntry` so
a follow-up retry has a target. Regression test pinned: spy on
fs.writeFile to throw, assert `removed === false` AND
getPreviousUserMessages() still surfaces the entry.
- cli/AppContainer.tsx: `void logger?.removeLastUserMessage()` no
longer silently swallows failures. Added `.catch` that routes
through `debugLogger.debug` so a disk-write failure leaves a
diagnostic trail; without it the cancelled prompt would
resurrect next session via ↑-history with no observability into
why.
- cli/historyUtils.ts: `gemini_thought` / `gemini_thought_content`
classification reaffirmed as SYNTHETIC with explicit JSDoc on
WHY (Claude Code parity + auto-restore is most valuable in the
cancel-during-thinking case which is exactly the case where
thoughts have appeared but no committed `gemini_content`).
Future readers won't re-litigate the classification by accident.
Tests: 49/49 logger.test.ts pass; tsc + ESLint clean.
* docs(core): align removeLastUserMessage JSDoc with rollback-on-failure behaviour
The previous commit added a rollback path to `removeLastUserMessage`
(re-insert the optimistically-removed entry and restore
`lastLoggedUserEntry` when the disk read or write throws), but the
JSDoc still said the in-memory removal is "intentionally NOT rolled
back" — a copy-paste leftover from the earlier design that picked
optimistic-and-diverge. Rewrite the failure-handling paragraph and
`@returns` line to describe the rollback contract instead.
No code change.
* fix(cli, core): scope auto-restore to the cancelled turn + tighten typings/tests
Three follow-ups from PR #4023 review batch 5.
* cli — `CancelSubmitInfo` gains `lastTurnUserItem` carrying the user
prompt text that THIS turn's `prepareQueryForGemini` added (or
`null` for paths that don't push a user history item: Cron /
Notification / slash `submit_prompt`). `cancelOngoingRequest`
snapshots `lastTurnUserItemRef.current` and ships it through. The
AppContainer auto-restore guard now requires
`info.lastTurnUserItem` to be present AND match the candidate
user item's text before truncating/rewinding — closing the case
where an older user item happens to be followed by only-synthetic
trailing content and the current cancelled turn never owned a
user item to begin with.
Two new regression tests pin both halves: cancel of a non-USER
turn bails despite trailing-synthetic, and a deliberate text
mismatch also bails.
* cli — `.catch((err)` widened to `(err: unknown)` on the
fire-and-forget `logger.removeLastUserMessage()` call. Belt-and-
braces: `Promise.catch`'s lib typing is `(reason: any) =>` so
this is not currently TS7006, but tightening keeps the codebase
ready for `@typescript-eslint/no-implicit-any-catch`-style rules
and matches the rest of the codebase's strict-error patterns.
* core — Added a `removeLastUserMessage` regression test pinning
the `_readLogFile` failure branch (mocks `fs.readFile` to throw
Permission denied). The symmetric `writeFile` failure case was
already covered; this closes the gap on the read leg.
Tests: AppContainer 67/67 (+2), useGeminiStream 87/87, historyUtils 11/11,
logger 50/50 (+1). Type-check and lint clean.
* chore(cli): add debug observability for each auto-restore-on-cancel bail-out
The cancel handler in AppContainer has seven independent guards that
silently `return` when auto-restore is unsafe (buffer non-empty, queue
non-empty, pending meaningful content, no last-turn user item, no user
in history, trailing items not all synthetic, candidate-text mismatch).
Until now, users reporting "I pressed ESC but my prompt didn't come
back" had no way to know which guard tripped without a debugger.
Log a specific `debugLogger.debug(...)` line at each bail-out and one
on the success path. Debug level keeps production output silent;
re-enableable by running with `DEBUG=1` (per existing convention in
this file). No control-flow change.
* docs(core): scope removeLastUserMessage's "false ⇒ observable in-memory" guarantee
The previous JSDoc implied the guarantee held for every `false` return,
but it only really holds on the disk read/write THROW path (where we
roll back the optimistic in-memory removal). Two other `false`-paths
behave differently:
- Initial guards (logger uninitialized / no tracked entry): nothing
was ever removed, nothing to restore — entry stays in whatever
state it was already in.
- Disk read succeeds but the tracked row is missing on disk (e.g. a
concurrent rotation/clear): we adopt disk state into `this.logs`,
so both sides agree the entry is gone — `false` is returned but
the entry is NOT observable in-memory either.
Rewrite the failure-handling paragraph and `@returns` line to spell
out both branches explicitly. No code change.
* fix(core): shift lastLoggedUserEntry on USER logMessage duplicate-skip
When `_updateLogFile` detects another instance already wrote an
identical (sessionId, messageId, timestamp, message) row and returns
null, the previous logMessage code path left `lastLoggedUserEntry`
pointing at the prior USER entry. A subsequent cancel/auto-restore
would then call `removeLastUserMessage()` and silently delete the
wrong row — typically an older prompt that the user did not intend
to undo.
The fix: when the duplicate skip happens on a USER attempt, advance
`lastLoggedUserEntry` to the entry object we just tried to write.
`_updateLogFile` mutates that object's `messageId` in-place to align
with the disk row before the duplicate check, so the 5-tuple matches
the row that's actually on disk and an undo correctly targets it.
The natural race (`max+1` colliding with an existing `messageId`)
is not reachable by sequential awaits — the snapshot used for the
duplicate check is always max+1-strict. The regression test drives
the contract directly by mocking `_updateLogFile` to resolve to
null and asserting `lastLoggedUserEntry` shifts to the new entry.
* fix(cli, core): strip orphan user entry from chat history on auto-restore
The auto-restore branch was cleaning up two of the three places a
cancelled prompt lives — the UI transcript via `truncateToItem` and
the disk-backed ↑-history via `Logger.removeLastUserMessage`. The
third — the in-memory chat history on `GeminiChat` — was left
untouched. `sendMessageStream` appends the user content to
`chat.history` BEFORE the stream generator runs and the abort path
doesn't pop it. After a successful auto-restore the next request's
wire payload still carried the cancelled prompt as a leading user
turn alongside the new prompt, so the model saw context the user
believed had been undone (and in some shapes the API would reject
two consecutive user turns).
Mirror the existing strip the Retry submit path uses
(`GeminiClient.sendMessageStream` at the `Retry` branch): make
`GeminiClient.stripOrphanedUserEntriesFromHistory` public and call
it from the auto-restore success path, sitting next to the UI
truncate and the disk-log undo. The method already pops trailing
user entries and clears the `FileReadCache` (which can otherwise
hold dangling `read_file` results from the stripped turn).
End-to-end reproduction from the PR review:
1. Submit `what time is it?` → ESC during pre-token delay →
auto-restore (UI rewound, buffer pre-filled).
2. Edit buffer to `what year is it?` → submit.
3. Pre-fix: outbound `messages` carried both prompts as consecutive
user turns. Post-fix: only the new prompt.
Test: extend the auto-restore-success AppContainer test with a
mock `stripOrphanedUserEntriesFromHistory` spy and assert it fires.
The non-restore branches don't install the spy (it's optionally
chained at the call site).
* fix(core, cli): tighten Logger.serialize signature + pin lastTurnUserItem and dup-skip identity contracts
Three follow-ups from PR review batch:
* core/logger.ts — `serialize()` was `this.writeQueue.then(op, op)`.
The second callback was dead code: `writeQueue` is seeded with
`Promise.resolve()` and reassigned through `.catch(() => undefined)`,
so the queue tail can never reject. Worse, `then(op, op)` reads as
"retry op on rejection" — wrong intent. Switch to `.then(() => op())`
with a comment spelling out the no-reject invariant.
* cli/useGeminiStream.test.tsx — add ownership-contract tests at the
PRODUCER side of `info.lastTurnUserItem`. Until now only the
AppContainer tests pinned the contract, and they fabricate the
value, so a regression that drops `lastTurnUserItemRef.current = {
text: trimmedQuery }` in `prepareQueryForGemini` would slip
through. New tests:
- normal `UserQuery` submit → cancel → assert
`info.lastTurnUserItem === { text: 'what time is it?' }`.
- `SendMessageType.Notification` submit → cancel → assert
`info.lastTurnUserItem === null` (path doesn't push a user
history item, the ref reset at the top of
prepareQueryForGemini must keep it null).
* core/logger.test.ts — strengthen the duplicate-skip regression.
The previous test only checked the tracker advanced text; the
important identity contract is that the recalculated 5-tuple
matches the disk row, so a subsequent `removeLastUserMessage()`
removes the duplicate-skipped row rather than the older USER.
New test seeds disk with [first, second], stubs `_updateLogFile`
for the second call to mimic the duplicate-skip branch (mutate
newEntryObject's messageId+timestamp to align with the disk row,
return null), then asserts removeLastUserMessage() leaves
['first'] on disk and removes 'second'.
* fix(cli, core): close four cancel-auto-restore correctness gaps from PR review
Four critical findings from gpt-5.5 /review pass:
1. **Retry skipped the lastTurnUserItem reset** (useGeminiStream.ts)
`Retry` bypasses `prepareQueryForGemini`, which is where the
`lastTurnUserItemRef.current = null` reset lived. A retry that
followed a normal `UserQuery` carried the stale ownership snapshot
into `onCancelSubmit`, and cancelling the retry before any
meaningful output let `AppContainer` auto-restore truncate the
original failed prompt. Move the reset (and the new content-seen
reset, see #4) to the top of `submitQuery`, gated only on
"this is a top-level submit" — covers Retry, Cron, Notification,
and ordinary UserQuery alike.
2. **Text-only ownership matched dedup'd duplicates** (AppContainer.tsx,
useGeminiStream.ts) `useHistoryManager.addItem` skips inserting a
consecutive-duplicate user message while still returning a freshly
generated id. The text-only ownership check would match the OLDER
identical-text USER row, so a re-submitted same prompt + cancel
would wrongly truncate the prior turn. Carry id+text in
`CancelSubmitInfo.lastTurnUserItem` (using `addItem`'s return
value) and require both id AND text to match before truncating.
3. **stripOrphan left IDE context state advanced** (client.ts) Other
history-mutating paths (`setHistory`, `truncateHistory`) set
`forceFullIdeContext = true` after mutating; the orphan-strip
didn't, so a subsequent request could send a diff against a
removed baseline. Gate cache-clear + IDE-context invalidation on
an actual before/after length drop, so no-op strips don't churn
state.
4. **Flush-then-thought race let auto-restore wipe committed content**
(useGeminiStream.ts, AppContainer.tsx) `cancelOngoingRequest`'s
pre-cancel flush can `addItem` a meaningful `gemini_content` (via
handleContentEvent's split path) and then a later thought event
overwrites `pendingHistoryItem` with a synthetic value. The
AppContainer guard's React history snapshot is stale, so the
trailing-only-synthetic check passes and the just-committed text
gets truncated. Track a synchronous `turnSawContentEventRef` set
in handleContentEvent, ship it through `CancelSubmitInfo`, and
make the guard bail when set.
Tests:
- core/client.test.ts: stripOrphan only forces full IDE context on
actual removal; existing retry tests updated to mock
`getHistoryLength`.
- cli/useGeminiStream.test.tsx: ownership uses { id, text }, Retry
reset works after a prior UserQuery cancel,
turnProducedMeaningfulContent flips true when content lands.
- cli/AppContainer.test.tsx: guard bails on `turnProducedMeaningfulContent: true`,
guard bails on id mismatch (catches addItem dedup case).
cli 162/162 + core client 99/99 + core logger 51/51.
* fix(cli): repaint static transcript after auto-restore truncate
Reported by @tanzhenxin: auto-restore truncated React `history` state
but the cancelled `> prompt` and `Request cancelled.` lines stayed
printed in the terminal — Ink's `<Static>` region is append-only, so
shrinking the underlying array doesn't unprint already-flushed lines.
On the PR's golden path (type prompt → Enter → ESC) the user sees the
prompt twice: once in scrollback, once pre-filled in the input buffer.
Confirmed at multiple Enter-to-ESC delays, so it's not a timing fluke.
Call `refreshStatic()` immediately after `truncateToItem(...)` in the
auto-restore success path. `refreshStatic` writes the ANSI
clear-terminal escape AND bumps the static remount key — the exact
recipe `/clear` (`handleClearScreen`) already uses for the same
reason. The targeted-repaint helper used for terminal resizes is
intentionally NOT used here: it preserves scrollback, which would
leave the cancelled prompt visible above the new viewport.
Test: extend the existing auto-restore happy-path AppContainer test
to assert `mockStdout.write` was called with `ansiEscapes.clearTerminal`.
The other auto-restore-bail tests don't install the assertion so they
naturally verify the negative case (no clear when guard rejects).
xaelistic
pushed a commit
to xaelistic/qwen-code
that referenced
this pull request
Jun 7, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
When a user pressed ESC immediately after submitting a prompt (before the model produced any meaningful output), qwen-code left the cancelled prompt stranded in the transcript and in cross-session ↑-history. Cancelling during tool execution also silently dropped any queued follow-up input.
This PR mirrors Claude Code's auto-restore-on-interrupt behaviour:
clearQueue()(added for ESC后经常默认输入了上一次的输入 #3204) with a non-destructive pop, so queued follow-ups are preserved (editable in the buffer) rather than silently lost.Logger.removeLastUserMessageundoes the just-persisted disk entry fromgetPreviousUserMessagesso the cancelled prompt doesn't resurrect next session.Reproduction (before this PR)
what time is it?and press Enter.> what time is it?followed byUser cancelled the request.; ↑ recall lists the cancelled prompt; input box is empty (the user must retype to retry).After this PR: the transcript shows nothing for that turn (rewound), the input box is pre-filled with
what time is it?ready to edit or resubmit, and ↑ recall doesn't show the cancelled prompt in the current or next session.A second variant: submit
slow command, queuenext thingwhile it streams, hit ESC during a long-running tool. Before: queuednext thingis silently dropped. After:next thinglands in the input buffer for review.Behaviour change (qwen-code)
Out of scope / unchanged
messagesAfterAreOnlySynthetic-style trailing check + pending non-synthetic check).cancelOngoingRequest → onCancelSubmit.StreamingState.WaitingForConfirmation): different code path, not affected./some-shortcut → submit_prompt) and Cron / Notification submits: these never add auserhistory item to begin with, so the auto-restore guard naturally bails (nolastUserItembelongs to the cancelled turn).Context on #3204
#3204 was a regression where items the user had queued during a tool run would auto-fire after the tool settled, even though the user had pressed ESC to cancel. The fix at the time was the unconditional
clearQueue()in the tool-cancel branch — correct for the auto-fire problem but blunt: any queued text the user might have wanted to edit was lost.This PR replaces
clearQueue()withpopAllMessages()for the same path. The auto-fire bug stays fixed (the queue still ends up empty after cancel —drainQueuemid-turn returns nothing), but the text lands in the input buffer for the user to edit or discard, matching the response-cancel branch's behaviour and Claude Code'spopAllEditable.Implementation notes
The "meaningful content" check matches Claude Code's
messagesAfterAreOnlySynthetic(REPL.tsx / MessageSelector.tsx): assistant text and tool runs are meaningful; info/error/warning/retry/notification/tool_use_summary/thoughts are synthetic. The new helper lives atpackages/cli/src/ui/utils/historyUtils.ts.Auto-restore uses
historyManager.truncateToItem(functional setState) so the slice batches with the INFOaddItemqueued bycancelOngoingRequestin the same render pass — no flicker, no double-render.Logger.removeLastUserMessagetracks the most recently persisted USER entry (mirroring claude-code'slastAddedEntryinhistory.ts) and identifies it by(sessionId, messageId, timestamp, message, type)for safe undo. It is one-shot, no-ops cleanly on uninitialized loggers and on external rotation, and never touches MODEL_SWITCH entries.The auto-restore guards are intentionally conservative — any of
buffer non-empty,queue had items,pending stream has meaningful content,committed trailing items aren't all synthetic, orno last-user itemcauses a bail. Better to leave the cancelled prompt in the transcript than to wrongly clobber a draft or a real response.Test plan
packages/cli/src/ui/utils/historyUtils.test.ts— 9 cases covering synthetic / meaningful classification, trailing-only-synthetic detection, last-user-index lookup, including thought-as-synthetic parity with Claude Code.packages/cli/src/ui/AppContainer.test.tsx— 6 cancel-handler cases covering: auto-restore happy path, bail when assistant produced content, bail when buffer has draft text, bail when queue had items (queue still drained to buffer), bail when tool_group is pending, queue drained on tool-execution cancel.packages/core/src/core/logger.test.ts— 7removeLastUserMessagecases: basic undo, no-op when nothing tracked, one-shot semantic, MODEL_SWITCH not undone, external rotation handled, uninitialized-logger no-op, tracker cleared on transientwriteFileerror.tsc --noEmitclean across cli + core (apart from pre-existingdiffCommanderrors).eslintclean on all touched files.🇨🇳 中文版本（点击展开）
概述
当用户提交 prompt 后立即按 ESC（在模型产出任何有意义输出之前），qwen-code 会把已取消的 prompt 残留在 transcript 和跨会话 ↑ 历史中。在 tool 执行期间取消还会静默丢弃已排队的后续输入。
本 PR 复刻 Claude Code 的 auto-restore-on-interrupt 行为：
clearQueue()替换为非破坏性的 pop：队列中的后续指令保留下来（落到输入框可编辑），而不是被无声丢弃。Logger.removeLastUserMessage，撤销刚刚写入磁盘的条目，避免下一次会话从getPreviousUserMessages复活已取消的 prompt。复现步骤（PR 之前）
what time is it?并回车。> what time is it?+User cancelled the request.；↑ 回溯能看到这条已取消的 prompt；输入框为空（用户必须重新打字才能重试）。应用本 PR 之后：transcript 对这一轮干干净净（已回滚），输入框预填了
what time is it?可直接编辑或重发，本会话和下一次会话的 ↑ 回溯都不再出现这条已取消的 prompt。第二种场景：提交
slow command，在它流式响应期间排入next thing，然后在 long-running tool 中按 ESC。改动前：next thing被静默丢弃；改动后：next thing落到输入框供查看。行为变化（qwen-code）
不在范围 / 不变更
messagesAfterAreOnlySynthetic风格的尾部检查 + pending 非 synthetic 检查共同守卫）。cancelOngoingRequest → onCancelSubmit的路径才会触发。StreamingState.WaitingForConfirmation）：走另一条代码路径，不受影响。/some-shortcut → submit_prompt）以及 Cron / Notification 类提交：这些路径本就不会写入user类型的 history 项，所以 auto-restore 守卫自然 bail（找不到属于这一轮的lastUserItem）。#3204 背景
#3204 报的是一个回归：用户在 tool 运行期间排队的指令，在 tool 结束、状态恢复 idle 后会自动触发，即便用户按过 ESC 想取消。当时的修复是在 tool-cancel 分支无条件
clearQueue()—— 解决了 auto-fire，但代价较大：用户可能想编辑/复用的排队文本被一并丢弃。本 PR 把这一处的
clearQueue()换成popAllMessages()：auto-fire 仍然不会发生（取消后队列依旧为空，drainQueue在轮中读到的就是空），但文本会落到输入框供用户编辑或丢弃，与 response-cancel 分支的行为以及 Claude Code 的popAllEditable保持一致。实现要点
"有意义内容" 的判定对齐 Claude Code 的
messagesAfterAreOnlySynthetic（REPL.tsx / MessageSelector.tsx）：assistant 文本和 tool 运行算 meaningful；info/error/warning/retry/notification/tool_use_summary/thoughts 算 synthetic。新工具函数放在packages/cli/src/ui/utils/historyUtils.ts。Auto-restore 走
historyManager.truncateToItem（函数式 setState），所以这次切片会跟cancelOngoingRequest刚 enqueue 的 INFOaddItem在同一次 React render 中合批 —— 无闪烁、无双 render。Logger.removeLastUserMessage追踪最近一次成功持久化的 USER 条目（对应 claude-codehistory.ts的lastAddedEntry），并以(sessionId, messageId, timestamp, message, type)五元组精确匹配，避免误删。一次性语义；logger 未初始化或外部已轮转时安全 no-op；不会动 MODEL_SWITCH 条目。Auto-restore 的守卫有意保守 ——
buffer 非空、队列原本有内容、pending 流有 meaningful 内容、committed 尾部不全是 synthetic、找不到 last-user 项任一为真就 bail。宁可让已取消的 prompt 留在 transcript 里，也不要错误地覆盖用户草稿或真实回答。测试计划
packages/cli/src/ui/utils/historyUtils.test.ts—— 9 个用例，覆盖 synthetic / meaningful 分类、尾部全 synthetic 判定、last-user 索引查找，包含 thought-as-synthetic 与 Claude Code 对齐的判定。packages/cli/src/ui/AppContainer.test.tsx—— 6 个 cancel-handler 用例：auto-restore 正常路径；assistant 已产出内容时 bail；buffer 有草稿时 bail；队列有内容时 bail（队列仍回填 buffer）；pending tool_group 时 bail；tool 执行中取消时队列回填 buffer。packages/core/src/core/logger.test.ts—— 7 个removeLastUserMessage用例：基础撤销、空时 no-op、一次性语义、不误删 MODEL_SWITCH、外部轮转处理、未初始化 no-op、瞬态writeFile异常时清空 tracker。tsc --noEmit干净（cli + core，pre-existingdiffCommand噪声除外）。eslint在所有触动文件上干净。