fix(cli): submit fast tool results after stream end#5071
Conversation
| }); | ||
|
|
||
| await act(async () => { | ||
| await staleOnComplete?.([fastFailedTool]); |
There was a problem hiding this comment.
[Critical] TS2349: staleOnComplete is typed as ((completedTools: TrackedToolCall[]) => Promise<void>) | null, but fastFailedTool is TrackedCompletedToolCall. These types are incompatible — tsc --noEmit fails with "This expression is not callable." Tests pass at runtime because vitest doesn't typecheck.
| await staleOnComplete?.([fastFailedTool]); | |
| await (staleOnComplete as ((tools: TrackedCompletedToolCall[]) => Promise<void>) | null)?.([fastFailedTool]); |
Or update capturedOnComplete's type annotation to accept TrackedCompletedToolCall[].
— qwen3.7-max via Qwen Code /review
| @@ -2409,7 +2418,6 @@ export const useGeminiStream = ( | |||
| submitQuery(responsesToSend, SendMessageType.ToolResult, prompt_ids[0]); | |||
| }, | |||
| [ | |||
There was a problem hiding this comment.
[Critical] Missing cancellation guard: removing isResponding from the dependency array eliminates the stale-closure that accidentally protected against post-cancellation tool submissions. Now activeModelStreamsRef.current always reads 0 after stream abort, so handleCompletedTools proceeds to submitQuery at line 2418 — sending tool results (potentially containing file contents or shell output) to a new API call the user explicitly tried to cancel via Ctrl+C.
Scenario: Ctrl+C → stream aborts → counter→0 in finally → tool completes → activeModelStreamsRef.current > 0 guard passes → submitQuery fires unwanted API call. The submitQuery function resets turnCancelledRef.current = false, so the cancel state is lost.
Before this PR, the stale isResponding === true closure returned early. Now the ref is always current, making this path deterministic.
Fix: Add a cancellation check before submitQuery at line 2418:
if (turnCancelledRef.current || abortControllerRef.current?.signal.aborted) {
markToolsAsSubmitted(callIdsToMarkAsSubmitted);
return;
}— qwen3.7-max via Qwen Code /review
✅ Local verification report — PR #5071Verified locally by a maintainer on macOS — the OS this PR's matrix marked Environment
Results at a glance
1. Code reviewThe fix swaps the early-return guard in
2. Full suite on macOSMatches the author's Windows run and fills the macOS gap. 3. Test-effectiveness counter-proof — the decisive evidenceA regression test for a race is only worth as much as its ability to fail on the bug. I reverted only The single failure is the new test 4. Static checks
5. Real CLI end-to-end (tmux + mock OpenAI server, YOLO)This race lives in the React TUI path ( Observed in the TUI: Mock server log shows the two model requests, the second carrying the tool result back 145 ms after the first: The existence of turn 2 (
VerdictLGTM — recommend merge. Every claim in the PR test plan reproduces on macOS, the new regression test is proven to actually catch the race (red on the parent commit, green on the PR), and the real CLI handles the YOLO tool-error → continue path end-to-end with no regression. 🇨🇳 中文版（点击展开）✅ 本地验证报告 — PR #5071由维护者在 macOS 上本地验证 —— 正是本 PR 矩阵里标记 环境
结果速览
1. 代码审查修复把
2. macOS 全套测试与作者的 Windows 结果一致,补上了 macOS 的空缺。 3. 测试有效性反证 —— 决定性证据竞态的回归测试,其价值取决于它"在 bug 上能否变红"。我只把 唯一失败的就是新测试 4. 静态检查
5. 真实 CLI 端到端(tmux + mock OpenAI server,YOLO)这个竞态存在于 React TUI 路径( TUI 中观察到: mock server 日志显示两次模型请求,第二次在第一次之后 145 ms 把工具结果带了回来: turn 2(
结论LGTM —— 建议合并。 PR 测试计划中的每一项都在 macOS 复现;新回归测试被证明确实能抓住该竞态(在父提交上红、在 PR 上绿);真实 CLI 在 YOLO 工具出错 → 继续的路径上端到端工作,无回归。 |
What this PR does
Fixes the tool-result handoff race where a very fast tool completion could be dropped after the model stream had already ended but before React replaced
handleCompletedToolswith a fresh callback. The hook now tracks the actualsendMessageStreamlifetime in a synchronous ref and uses that for the active-stream guard, while keeping the existingisRespondingstate for UI rendering.Why it's needed
#4672 reports AUTO_EDIT / YOLO turns stalling after a read/edit error until the next user instruction. The maintainer root-cause analysis points to
handleCompletedToolsreading staleisRespondingfrom a React callback closure: the stream finishes, a tool fails immediately, and the old callback still seesisResponding === true, returns early, and never sends the tool error back to the model. AUTO_EDIT / YOLO removes the approval delay, so the race is easier to hit there.Reviewer Test Plan
How to verify
Run
npm run test --workspace=packages/cli -- src/ui/hooks/useGeminiStream.test.tsx. The new regression test issubmits a fast tool result after the stream ended but before React replaces the callback; it saves the callback captured while React state still says responding, ends the held stream, then fires a fast failedread_fileresult through that stale callback and verifies a ToolResult request is sent.Evidence (Before & After)
Before: the stale callback path returned at the
isRespondingguard and dropped the tool result, so the secondsendMessageStreamcall never happened. After: the guard reads the actual stream-lifetime ref, sees that the stream has ended, marks the tool submitted, and sends the failed tool result back to the model.Tested on
Environment (optional)
Windows 11, Node v24.15.0, npm 11.12.1.
Validation run locally:
Risk & Scope
/btw, do not clear the guard early.Linked Issues
Fixes #4672
中文说明
What this PR does
修复一个工具结果回传的竞态：模型流已经结束，但 React 还没来得及把
handleCompletedTools换成新闭包时，快速完成或快速失败的工具结果可能被旧闭包丢掉。现在 hook 用同步 ref 记录真实的sendMessageStream生命周期，并用它做 active-stream guard；原来的isRespondingstate 仍然只负责 UI 渲染语义。Why it's needed
#4672 反馈 AUTO_EDIT / YOLO 模式下读文件或编辑出错后，本轮不会继续更新文件，需要下一次用户指令才恢复。maintainer 已定位根因：
handleCompletedTools从 React callback 闭包里读到旧的isResponding。时序是模型流先结束，工具马上失败，旧 callback 仍认为isResponding === true，于是提前 return，模型收不到工具错误，agent loop 卡住。AUTO_EDIT / YOLO 没有 approval 延迟，所以更容易撞到这个窗口。Reviewer Test Plan
How to verify
运行
npm run test --workspace=packages/cli -- src/ui/hooks/useGeminiStream.test.tsx。新增回归测试submits a fast tool result after the stream ended but before React replaces the callback：测试先保存 React state 仍是 responding 时捕获的旧 callback，再结束 held stream，然后让一个快速失败的read_file结果走这个旧 callback，验证它仍会发送 ToolResult 请求。Evidence (Before & After)
修复前：旧 callback 会在
isRespondingguard 直接 return，工具结果被丢弃，第二次sendMessageStream不会发生。修复后：guard 读取真实 stream-lifetime ref，发现模型流已经结束，于是标记工具已提交，并把失败的工具结果回传给模型。Tested on
Environment (optional)
Windows 11，Node v24.15.0，npm 11.12.1。
Risk & Scope
/btw这类重叠 stream 提前清掉 guard。Linked Issues
Fixes #4672