fix(cli): Fail dangling replayed tool calls - #5624
Conversation
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Downgraded from Approve to Comment: CI still running. R2 addresses both R1 suggestions (replay-loop try/catch and per-dangling-call error continuation). Four new suggestions below.
— qwen3.7-max via Qwen Code /review
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
✅ Local verification report (real build + tests on Linux)I verified this PR locally with real builds and test runs to support the merge decision. Net result: the fix works end-to-end and is safe to merge. This also fills the 🐧 Linux row the PR table left as "not tested". Setup
1. Author's test plan reproduced ✅
2. The new tests genuinely guard the fix (revert-proof) ✅Ran the new test file against the base The other 2 new tests (matched-result completes / TodoWrite-skip) pass on base too — correct, they guard that the fix does not break existing good behavior. Note: the production 3. End-to-end runtime A/B — the actual "stuck UI" symptom 🎯I drove the real
The actual terminal frame the fix emits: {
"sessionUpdate": "tool_call_update",
"toolCallId": "call-shell-1",
"status": "failed",
"content": [{ "type": "content", "content": { "type": "text",
"text": "Tool result missing from saved history; the previous run likely ended before this tool completed." }}],
"_meta": { "toolName": "run_shell_command", "provenance": "builtin", "timestamp": 1782122401000 }
}This is the whole point of the PR confirmed across both changed packages working together — not just the unit tests in isolation. 4. Lint / format / typecheck ✅
Observations for merge
Verdict: LGTM from a verification standpoint — approve to merge after the test-count text fix. 🇨🇳 中文版（点击展开）✅ 本地验证报告（Linux 真实构建 + 测试）为辅助合并决策，我在本地用真实构建与测试对本 PR 做了验证。结论：修复端到端有效，可以安全合并。 同时补齐了 PR 表格里标为 "not tested" 的 🐧 Linux 一行。 环境
1. 复现作者测试计划 ✅
2. 新测试确实在守护本次修复（反向验证）✅把新测试文件跑在 base 版 另外 2 个新测试（匹配成功→completed / TodoWrite 跳过）在 base 上也通过——这是对的，它们守护的是"修复不破坏既有正确行为"。 注意：生产代码 3. 端到端运行时 A/B —— 真实复现"UI 卡住"症状 🎯我用真实的
修复实际发出的终态帧： {
"sessionUpdate": "tool_call_update",
"toolCallId": "call-shell-1",
"status": "failed",
"content": [{ "type": "content", "content": { "type": "text",
"text": "Tool result missing from saved history; the previous run likely ended before this tool completed." }}],
"_meta": { "toolName": "run_shell_command", "provenance": "builtin", "timestamp": 1782122401000 }
}这正是本 PR 的核心目标，且是在两个改动包协同工作下验证的——不仅仅是各自孤立的单测。 4. Lint / 格式 / 类型检查 ✅
合并相关观察
结论：从验证角度 LGTM —— 改完测试数量文案后可合并。 Verified locally on Linux via tmux with real build/test runs (focused vitest, revert-proof A/B, end-to-end HistoryReplayer→selector A/B, eslint/prettier/tsc). |
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Two Critical findings below need addressing before merge.
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
Reviewed the replay change independently. The approach is right: track emitted assistant tool starts by source call id, drop them on a matching result, fail whatever's left after the loop — it's contained to transcript reconstruction and reuses the existing emitResult path. The per-call isolation in failDanglingToolCalls, the replay/cleanup AggregateError split, and gating pending on sourceCallId all hold up, and the new tests genuinely guard them (verified non-vacuous).
One residual, non-blocking: if a result record has a real upstream functionCall.id but carries neither toolCallResult.callId nor a functionResponse.id, getToolResultCallId falls back to record.uuid, so the pending entry isn't deleted and the call ends up with both a completed update (under the uuid) and a synthetic failed one. That's strictly better than before (the block was already orphaned and stuck), just not perfectly clean for that rare old-history shape — fine to leave.
LGTM.
|
Thanks for the PR! Template looks good ✓ — all required sections present, bilingual translation included. On direction: this fixes a real, user-facing bug — replaying incomplete sessions leaves the UI stuck in "responding" forever. Clearly within scope as a session-management bug fix. CHANGELOG has prior replay-related fixes (#5108, #3747) in the same area, confirming this is a recognized concern. On approach: scope feels right. The fix is confined to the replay path — no changes to live tool execution, protocol shapes, or REST/SDK/ACP contracts. The call-ID fallback chain ( One minor note: the PR description says "26 tests" but I verified 26 tests passing locally (matching the actual count), so the description is accurate on the latest head. Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ — 所有必填章节齐全，包含双语翻译。 方向：修复了一个真实的、用户可感知的 bug — 回放不完整会话时 UI 永远卡在 "responding" 状态。明确属于 session 管理 bug 修复范围。CHANGELOG 中已有先前的回放相关修复（#5108、#3747），说明这是已知的关注领域。 方案：范围恰当。修复仅限于回放路径 — 不改动 live 工具执行、协议形态或 REST/SDK/ACP 合约。call-ID 回退链（ 一个小注：PR 描述说 "26 个测试"，我在本地验证了实际 26 个测试通过（与描述匹配）。 进入代码审查 🔍 — Qwen Code · qwen3.7-max |
Code ReviewIndependent proposal (before reading the diff): I'd track emitted Comparison with the diff: the PR's approach matches this exactly. The No critical blockers found. No AGENTS.md violations — the change is minimal, well-placed, and follows project conventions. Reuse check: the pending-call tracking and synthetic failure emission are specific to the replay path. No existing utility or shared module covers this. The Test ResultsKey test scenarios verified:
The prior verification comment on this PR already includes an end-to-end A/B test driving the real No concerns from code review or testing. Moving to final assessment. 中文说明代码审查独立方案（读 diff 前）：在回放过程中用 pending map 按 call ID 跟踪已发出的 与 diff 对比：PR 的方案与此完全一致。 未发现关键阻塞问题。未违反 AGENTS.md — 改动最小化、放置位置正确、遵循项目约定。 复用检查：pending-call 跟踪和合成失败发出是回放路径特有的逻辑，没有现有工具或共享模块覆盖。 测试结果已验证的关键测试场景：
PR 上先前的验证评论已包含端到端 A/B 测试（驱动真实 代码审查和测试均无顾虑。进入最终评估。 — Qwen Code · qwen3.7-max |
|
This is a clean, focused bug fix that does exactly what it says: dangling tool calls from incomplete sessions now get a terminal The implementation matches my independent proposal — pending Map, match-and-remove, fail-the-rest in 26 tests in the CLI package and 7 in the webui package all pass, covering the important edge cases: mixed matched/dangling, error-during-replay, TodoWrite skip, and the No over-engineering, no drive-by refactors, no scope creep. Every line in the diff serves the stated goal. If I had to maintain this in six months, I'd thank the author. Approving. ✅ 中文说明这是一个干净、专注的 bug 修复，完全如其所述：不完整会话中的悬空工具调用现在在回放时会获得终态 实现方案与我的独立方案一致 — pending Map、匹配移除、在 CLI 包 26 个测试和 webui 包 7 个测试全部通过，覆盖了重要边界场景：混合匹配/悬空、回放中出错、TodoWrite 跳过、 没有过度工程、没有顺手重构、没有范围蔓延。diff 中每一行都服务于既定目标。如果六个月后需要维护这段代码，我会感谢作者。 批准 ✅ — Qwen Code · qwen3.7-max |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| const callId = result?.callId ?? record.uuid; | ||
| const callId = this.getToolResultCallId(record); | ||
| this.pendingReplayToolCalls.delete(callId); | ||
|
|
There was a problem hiding this comment.
[Critical] pendingReplayToolCalls.delete(callId) runs before emitResult(). If emitResult throws, the entry is already removed from the Map and failDanglingToolCalls() can't synthesize a terminal update — the tool stays in_progress forever, the exact bug this PR aims to fix.
| await this.toolCallEmitter.emitResult({ | |
| toolName, | |
| callId, | |
| success: !result?.error, | |
| message: record.message.parts, | |
| resultDisplay: result?.resultDisplay, | |
| args: undefined, | |
| timestamp: record.timestamp, | |
| }); | |
| this.pendingReplayToolCalls.delete(callId); |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| danglingError = error; | ||
| } | ||
|
|
||
| if (replayError && danglingError) { |
There was a problem hiding this comment.
[Suggestion] The AggregateError message 'Replay and dangling-cleanup both failed' discards the underlying error details. Include the actual error messages for incident response debuggability.
| if (replayError && danglingError) { | |
| `Replay and dangling-cleanup both failed: ${(replayError as Error).message} | ${(danglingError as Error).message}` |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
What this PR does
This PR makes history replay close tool calls that were saved with a start event but no matching result. During replay, emitted assistant tool starts are tracked by call ID, real tool results remove their matching pending entry, and any remaining pending calls are replayed as failed tool updates with a clear missing-result message. Tool result matching now also falls back from the saved result call ID to the first function response ID before using the record UUID, preserving older history behavior while covering the saved-history shape that triggered the stuck UI.
The replay change is intentionally limited to historical transcript reconstruction. It does not change REST, SDK, ACP protocol shapes, or live tool execution semantics. TodoWrite starts that are skipped by the existing emitter are not tracked as dangling calls.
Why it's needed
Some saved sessions can contain assistant function calls without corresponding tool result records when the previous run ended before the tool completed or the history was otherwise incomplete. Replaying those sessions currently recreates an in-progress tool block but never emits a terminal update, so clients can continue showing the session as processing even though there is no backend work left.
Failing these dangling replay-only calls makes restored/exported transcripts terminal and prevents old incomplete history from keeping the UI in a responding state.
Reviewer Test Plan
How to verify
Run
cd packages/cli && npx vitest run src/acp-integration/session/HistoryReplayer.test.ts; expect all 25 tests to pass, including dangling, matched, mixed, call ID fallback, and TodoWrite skip cases.Run
cd packages/webui && npx vitest run src/daemon/session/selectors.test.ts; expect all 7 tests to pass, including failed/completed tool blocks remaining idle.Run
cd packages/cli && npx eslint src/acp-integration/session/HistoryReplayer.ts src/acp-integration/session/HistoryReplayer.test.tsandnpx prettier --check packages/cli/src/acp-integration/session/HistoryReplayer.ts packages/cli/src/acp-integration/session/HistoryReplayer.test.ts packages/webui/src/daemon/session/selectors.test.ts; both should pass.I also ran
npm run buildandnpm run typecheck. Both are currently blocked by existingpackages/cli/src/ui/components/BaseTextInput.tsxerrors resolvingink/domandink/components/CursorContext, plus relatedcursorCtxunknown-type errors; those files are outside this PR.Evidence (Before & After)
Before: replaying saved history with a function call but no saved tool result recreated an in-progress tool block with no terminal update, leaving the session visually stuck as processing.
After: the same replay emits the original
tool_callfollowed by atool_call_updatewithstatus: failedand the messageTool result missing from saved history; the previous run likely ended before this tool completed.Matched tool calls still complete normally and skipped TodoWrite starts do not produce synthetic failures.Tested on
Environment (optional)
Local macOS checkout, Node.js v26.0.0, gh 2.92.0. Focused Vitest, ESLint, Prettier, build, and typecheck commands were run locally; build/typecheck are blocked by the unrelated BaseTextInput Ink type errors noted above.
Risk & Scope
Linked Issues
N/A
中文说明
What this PR does
这个 PR 让历史回放能够终结那些已保存了工具开始事件、但没有匹配工具结果的调用。回放过程中，已发出的 assistant 工具开始事件会按 call ID 进入 pending；真实工具结果会移除匹配项；回放结束后剩余的 pending 调用会被重放为 failed 工具更新，并带上明确的缺失结果说明。工具结果匹配现在也会在保存的结果 call ID 缺失时，先回退到第一个 function response ID，再回退到 record UUID；这样既保留老历史行为，也覆盖导致卡住 UI 的保存历史形态。
这个回放改动刻意限制在历史 transcript 重建层，不修改 REST、SDK、ACP 协议形态，也不改变 live 工具执行语义。现有 emitter 会跳过的 TodoWrite start 不会被记录为 dangling call。
Why it's needed
部分已保存会话会在上一次运行于工具完成前结束、或历史不完整时，只包含 assistant function call，而没有对应的 tool result record。当前回放这些会话时会重新创建一个 in-progress 工具块，但永远不会发出终态更新，因此客户端可能持续显示会话仍在处理中，即使后端已经没有工作在运行。
把这些仅存在于 replay 阶段的悬空调用标记为 failed，可以让恢复和导出的 transcript 进入终态，并避免旧的不完整历史让 UI 一直保持 responding 状态。
Reviewer Test Plan
How to verify
运行
cd packages/cli && npx vitest run src/acp-integration/session/HistoryReplayer.test.ts；预期全部 25 个测试通过，包括 dangling、matched、mixed、call ID fallback 和 TodoWrite skip 场景。运行
cd packages/webui && npx vitest run src/daemon/session/selectors.test.ts；预期全部 7 个测试通过，包括 failed/completed 工具块保持 idle 的回归断言。运行
cd packages/cli && npx eslint src/acp-integration/session/HistoryReplayer.ts src/acp-integration/session/HistoryReplayer.test.ts和npx prettier --check packages/cli/src/acp-integration/session/HistoryReplayer.ts packages/cli/src/acp-integration/session/HistoryReplayer.test.ts packages/webui/src/daemon/session/selectors.test.ts；两者都应通过。我也运行了
npm run build和npm run typecheck。两者目前都被既有的packages/cli/src/ui/components/BaseTextInput.tsx错误阻塞：无法解析ink/dom和ink/components/CursorContext，并出现相关的cursorCtxunknown 类型错误；这些文件不在本 PR 范围内。Evidence (Before & After)
Before：当已保存历史里有 function call 但没有保存的 tool result 时，回放会重新创建一个 in-progress 工具块，但没有终态更新，导致会话视觉上一直停留在处理中。
After：同样的回放会先发出原始
tool_call，随后发出tool_call_update，其中status: failed，并带有Tool result missing from saved history; the previous run likely ended before this tool completed.。已匹配的工具调用仍会正常 completed，被跳过的 TodoWrite start 不会产生合成失败。Tested on
Environment (optional)
本地 macOS checkout，Node.js v26.0.0，gh 2.92.0。已在本地运行 focused Vitest、ESLint、Prettier、build 和 typecheck 命令；build/typecheck 被上面提到的无关 BaseTextInput Ink 类型错误阻塞。
Risk & Scope
Linked Issues
N/A