fix(cli): drop tool calls after cancellation#5020
Conversation
| expect(result.current.streamingState).toBe(StreamingState.Idle); | ||
| }); | ||
|
|
||
| it('should drop queued tool calls when user cancels the turn', async () => { |
There was a problem hiding this comment.
[Suggestion] Test only validates guard 1 (toolCallRequests.length = 0 on UserCancelled), not guard 2 (!signal.aborted).
When the stream yields ToolCallRequest → UserCancelled, the first guard empties the array. By the time execution reaches if (toolCallRequests.length > 0 && !signal.aborted), the length check is already false — !signal.aborted is never evaluated. This test would pass even if guard 2 were removed.
Guard 2 covers the scenario where the client-side abort fires but the stream drains without yielding UserCancelled (e.g., async iterator buffer race). Consider adding a complementary test:
it('should not schedule tool calls when signal is aborted without UserCancelled', async () => {
mockSendMessageStream.mockReturnValue(
(async function* () {
yield {
type: ServerGeminiEventType.ToolCallRequest,
value: { callId: 'call_1', name: 'write_file', args: { path: 'a.txt' } },
};
// Stream ends normally — no UserCancelled event
})(),
);
const { result } = renderTestHook();
// Simulate client-side abort (e.g., user presses Escape in UI)
// The abort signal should prevent scheduleToolCalls even though
// no UserCancelled event was yielded by the stream.
await act(async () => {
await result.current.submitQuery('abort without cancel event');
});
expect(mockScheduleToolCalls).not.toHaveBeenCalled();
});— qwen3.7-max via Qwen Code /review
f953729 to
8fcfb00
Compare
|
Thanks, added a second regression for the Validated locally: |
Runtime verification — real TUI, tmux-driven (maintainer merge reference)Verdict: PASS — built PR head Claim (my read of the 2-line src diff)A tool call is only materialized by the OpenAI converter when the Method
Steps
Findings
Build: 中文版（验证报告）运行时验证 — tmux 驱动真实 TUI（合并参考）结论：PASS — 本地构建 PR head 对这 2 行源码改动的理解tool call 只有在 方法
步骤
观察
构建:PR worktree 上 |
|
While reviewing this fix I checked whether the same race exists on the other entry points. Finding: the ACP session path (and therefore the daemon, which multiplexes sessions onto a What ACP already does right: inside the stream loop, every event checks the prompt signal and bails before anything is dispatched (Session.ts#L1156) — so a cancel that lands mid-stream correctly drops the collected What's missing is the second seatbelt: after the stream ends naturally, there are await points before dispatch — e.g.
So a cancel that lands in the post-stream window still dispatches every collected tool call. The signal is passed into tool execution, so well-behaved tools self-abort, but the dispatch itself is not gated — a tool that doesn't poll the signal runs to completion with side effects. Within a sequential batch, cancelling mid-batch also doesn't stop the remaining not-yet-started calls from being built and dispatched. The daemon inherits all of this unchanged (the bridge multiplexes sessions onto one Suggested follow-up fix (symmetric to this PR): an 中文说明复查这个修复时顺带验证了其他入口是否有同款竞态。结论：ACP session 路径存在同类竞态（窗口更窄但真实），daemon 完全继承——bridge 将多个 session 复用到一个
建议 follow-up（与本 PR 对称）： |
8fcfb00 to
8f27f5e
Compare
|
Addressed the review suggestion and rebased the branch onto the latest The PR already had the Validated on Windows:
Typecheck note: after rebasing, local workspace declaration files were stale. I rebuilt |
🔁 Re-verification — head
|
| Guard (src) | Test |
|---|---|
case UserCancelled: … toolCallRequests.length = 0; |
should drop queued tool calls when user cancels the turn |
if (toolCallRequests.length > 0 && !signal.aborted) |
should not dispatch queued tool calls after the request is aborted (new — my suggestion) |
Full suite: 107 passed (107) — 105 inherited from main-with-#5071 + the 2 new #5020 tests.
Counter-proofs — the two tests independently pin the two guards (decisive)
| Revert (src only) | Result |
|---|---|
remove && !signal.aborted (guard 2) |
should not dispatch queued tool calls after the request is aborted → RED; guard-1 test stays green |
remove toolCallRequests.length = 0 (guard 1) |
should drop queued tool calls when user cancels the turn → RED; guard-2 test stays green |
Each guard reddens exactly one test, with zero collateral on the other 105 — so the guards are not redundant: one covers UserCancelled-without-abort, the other aborted-signal-without-UserCancelled. This is precisely the gap my [Suggestion] flagged, now closed. Restoring → 107/107.
Real-TUI run (tmux + mock OpenAI, --approval-mode yolo)
Mock stream: content → tool_call(run_shell_command) → finish_reason+usage → [6 s stall] → [DONE], so the ToolCallRequest is collected at finish_reason while processing still awaits [DONE] (the #5016 "collected-not-dispatched" window).
- AFTER, normal path (no cancel):
✦ Writing the demo file now…→✓ Shell printf … > cancelled-proof.txt→ proof file written →✦ Tool result received. Done.→ no regression from the guards. - AFTER, cancel mid-stall (ESC):
✦ Writing…→● Request cancelled.→ no Shell card (the collected tool is dropped).
Honest scope note: on current main (post-#5071 + intervening commits) my ESC-during-SSE-stall abort drops the tool on the baseline build too — i.e. I could not reproduce the BEFORE/AFTER runtime divergence here that I got 6/6 on the older base (
5854e28) in my first review. #5016 is an extremely narrow timing race (originally surfaced by Docker fuzzing); on today's main the abort interleaving routes around the schedule via the exception path rather than theUserCancelled-after-collection path. The deterministic before/after divergence is the unit counter-proofs above — they exercise the exact event sequences and red/green precisely on each guard. The runtime run here confirms the fix behaves correctly and, importantly, does not regress normal tool execution.
Static & CI
prettier --check✅,eslint … --max-warnings 0exit0,tsc --noEmit(cli) ✅ no errors. Working tree byte-identical to PR head after all A/B swaps.- CI
8f27f5e0c: Lint ✅, CodeQL ✅, Classify ✅, Test macOS ✅, Test ubuntu ✅; Windows still running at post time.
🇨🇳 中文版（点击展开）
🔁 复核 — head 8f27f5e0c：我的评审建议已落实，两道守卫均被锁定 ✅
承接我的首次运行时验证（PASS）与行内 [Suggestion]：原测试只覆盖了守卫 1（UserCancelled 清空 toolCallRequests），等执行到 if (toolCallRequests.length > 0 && !signal.aborted) 时长度已为 0，守卫 2（!signal.aborted）从未被求值。这次强推新增了专门针对守卫 2 的回归，并变基到当前 main。在 Linux 6.12 / Node v22.22.2 复核：完整 hook 测试、两次“测试有效性反证”、tmux 真实 TUI 运行。
结论：建议已正确落实——守卫 2 现在有了自己的测试并被决定性锁定——两行修复完好，干净地变基在已合并的 #5071 之上，且不回归正常工具路径。建议合并。
变基 / 与 #5071 的关系（合并时值得注意）
origin/main 现在是 0db327317 = #5071 的 squash 合并。#5020 变基其上，真实改动只有两行守卫 + 2 个测试（GitHub 显示 useGeminiStream.ts 为 2+/1-）。两个 PR 修的是不同的取消竞态，干净共存：
- fix(cli): submit fast tool results after stream end #5071 →
handleCompletedTools完成回调（提交工具结果）。 - fix(cli): drop tool calls after cancellation #5020 →
submitQuery流循环的scheduleToolCalls派发（issue Qwen Code executes a tool after cancellation #5016）。
无冲突；变基后的分支以 fix(cli): submit fast tool results after stream end #5071 的activeModelStreamsRef为基。
两道守卫与对应测试
| 守卫（源码） | 测试 |
|---|---|
case UserCancelled: … toolCallRequests.length = 0; |
should drop queued tool calls when user cancels the turn |
if (toolCallRequests.length > 0 && !signal.aborted) |
should not dispatch queued tool calls after the request is aborted（新增——我的建议） |
完整测试：107 通过 / 107（含 main+#5071 的 105 + 新增 2）。
反证——两个测试各自独立锁定两道守卫（决定性）
| 仅回退（源码） | 结果 |
|---|---|
去掉 && !signal.aborted（守卫 2） |
should not dispatch queued tool calls after the request is aborted → 变红；守卫 1 测试仍绿 |
去掉 toolCallRequests.length = 0（守卫 1） |
should drop queued tool calls when user cancels the turn → 变红；守卫 2 测试仍绿 |
每道守卫只让对应的一个测试变红，对其余 105 个零误伤——说明两道守卫并不冗余：一个覆盖“UserCancelled 但 signal 未 abort”，另一个覆盖“signal 已 abort 但无 UserCancelled”。这正是我 [Suggestion] 指出的缺口，现已补上。还原 → 107/107。
真实 TUI 运行（tmux + mock OpenAI，--approval-mode yolo）
mock 流：content → tool_call(run_shell_command) → finish_reason+usage → [6 秒 stall] → [DONE]，因此 ToolCallRequest 在 finish_reason 时被收集，而处理仍在等 [DONE]（即 #5016 的“已收集未派发”窗口）。
- AFTER 正常路径（不取消）：
✦ Writing the demo file now…→✓ Shell printf … > cancelled-proof.txt→ 证据文件写出 →✦ Tool result received. Done.→ 无回归。 - AFTER stall 中取消（ESC）：
✦ Writing…→● Request cancelled.→ 无 Shell 卡片（已收集的工具被丢弃）。
诚实的范围说明：在当前 main（#5071 合并 + 期间其他提交）上，我的“stall 中 ESC”取消在基线构建上也会丢弃该工具——也就是说，我这次没能复现首评里在更旧基（
5854e28）上 6/6 的运行时 BEFORE/AFTER 分化。#5016 是一个极窄的时序竞态（最初由 Docker 模糊测试发现）；在今天的 main 上，abort 走的是异常路径而非“收集后再UserCancelled”路径，从而绕开了派发。确定性的 before/after 分化来自上面的单测反证——它们精确复现事件序列，并对每道守卫精确红/绿。本次运行时运行确认修复行为正确，且关键在于不回归正常工具执行。
静态 & CI
prettier --check✅、eslint … --max-warnings 0exit0、tsc --noEmit(cli) ✅ 无错误。所有 A/B 替换后工作区与 PR head 字节一致。- CI
8f27f5e0c：Lint ✅、CodeQL ✅、Classify ✅、Test macOS ✅、Test ubuntu ✅；撰写时 Windows 仍在运行。
qqqys
left a comment
There was a problem hiding this comment.
Critical-only re-review: the cancellation scheduling fix remains scoped and the new head 8f27f5e adds independent coverage for the aborted-signal guard as requested. CI is green, and I did not find a new critical blocker in this pass.
What this PR does
When an interactive turn is cancelled after the stream has already yielded a tool-call request, the pending tool calls are now discarded before the turn returns to idle. The final scheduling step also skips dispatch if the stream signal has been aborted.
Why it's needed
This fixes #5016. Previously, a cancellation could stop the visible response while still leaving a collected tool call to be scheduled after the stream finished. That made a cancelled turn capable of running tool work from the interrupted response.
Reviewer Test Plan
How to verify
Start an interactive turn that receives a tool call and cancel the turn before the tool dispatches. The cancellation message should be shown, the UI should return to idle, and the tool call from that cancelled response should not run. The added regression test exercises the same ordering by yielding a tool-call request followed by a cancellation event and asserting that no tool scheduling occurs.
Evidence (Before & After)
Before: a tool-call request collected before cancellation could still be passed to the tool scheduler when stream processing finished. After: cancellation clears queued tool calls, and aborted stream processing does not schedule remaining tool calls.
Tested on
Environment (optional)
Windows PowerShell, Node/npm from the repository development environment.
Risk & Scope
Linked Issues
Fixes #5016
中文说明
What this PR does
交互式回合在 stream 已经产出 tool call request 之后被用户取消时，现在会先丢弃尚未调度的 tool call，再回到 idle。最终调度阶段也会在 stream signal 已经 abort 时跳过 tool dispatch。
Why it's needed
修复 #5016。之前取消操作可能停止可见回复，但已经收集到的 tool call 仍然会在 stream 结束后被调度，导致一个已经取消的回合继续执行被中断响应里的工具操作。
Reviewer Test Plan
How to verify
启动一个会收到 tool call 的交互式回合，并在工具调度前取消。界面应该显示取消信息、回到 idle，并且不执行这个已取消响应里的 tool call。新增回归测试用同样的事件顺序模拟：先 yield tool-call request，再 yield cancellation event，并断言没有发生 tool scheduling。
Evidence (Before & After)
Before：取消前已经收集到的 tool-call request 仍可能在 stream 处理结束时传给 tool scheduler。After：取消会清空待调度 tool call，并且 abort 后的 stream 处理不会继续调度剩余 tool call。
Tested on
Windows 本地已验证；macOS / Linux 留给 CI 矩阵覆盖。
Environment (optional)
Windows PowerShell，使用仓库开发环境中的 Node/npm。
Risk & Scope
Linked Issues
Fixes #5016