fix(cli): surface the daemon duplicate tool-call breaker as a visible loop-detected stop - #9435
Merged
Merged
Conversation
… stop The ACP daemon Session was the only duplicate-provider-id circuit breaker path (PR #5657) that terminated silently: the turn ended as a normal end_turn with nothing in the transcript and no telemetry, so the session looked hung. Route the breaker through recordDaemonLoopDetected with LoopType.GLOBAL_TOOL_CALL_DUPLICATE — the same loop type the non-interactive CLI reports — so foreground turns fail with the visible LOOP_DETECTED turn error, the context message is preserved for the next turn, and the LoopDetectedEvent telemetry is emitted. The bespoke repeatedDuplicateProviderToolCall result flag and its dead consumer branch are removed in favor of the existing loopDetected plumbing.
Collaborator
|
✅ Qwen Triage finished — CI landed green on ✅ Qwen Triage 已完成 —— |
Collaborator
|
Thanks for the PR! Template looks good ✓ Problem: real and verified. On Direction: aligned. It completes parity with the three sibling paths (AgentCore, TUI, non-interactive CLI) instead of inventing a new mechanism — the error shape ( Size: no core-module paths touched ( Approach: minimal and subtractive — routes the breaker through the existing Risk: Stage 1e matches — Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ **问题：**真实且已验证。在 **方向：**对齐。它补齐了与另外三条路径（AgentCore、TUI、非交互 CLI）的一致性，没有引入新机制——错误形状（ **规模：**未触及核心模块路径（ **方案：**最小化且以删除为主——把熔断接入既有的 **风险：**Stage 1e 命中—— 进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
Collaborator
Code reviewEvidence in this comment: static review of the diff against My independent proposal for this problem was: call
No critical findings, no convention violations. This is a subtractive change: +15/−16 production lines, no new abstractions. The flow the change establishes, across the four entry points: sequenceDiagram
participant P1 as Provider replay
participant P2 as Session runToolCalls
participant P3 as recordDaemonLoopDetected
participant P4 as Turn loop
participant P5 as ACP client
P1->>P2: same tool-call id, third round
P2->>P3: set loopDetected and loopType, emit telemetry
P2-->>P4: result with loopDetected true
P4->>P4: suspend todo stop guard, preserve context message
alt foreground prompt
P4-->>P5: reject with LOOP_DETECTED turn error carrying loopType
else cron or background notification
P4-->>P5: graceful end_turn, context kept for next turn
end
Testing (the PR's own CI, fetched via API)At review time the primary unit suite The skips are by design, not gaps: One completed check is directly relevant: Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行（取最新一次运行），省略 skipped，失败项排在最前。 Sandboxed verification would settle this: 中文说明本评论的证据：针对 我对该问题的独立方案是：在熔断触发处用既有
无阻塞问题，无规范违规。这是一个删减型变更：生产代码 +15/−16，无新抽象。 测试部分：审查时主单元套件 沙箱验证可坐实核心论断： — Qwen Code · qwen3.8-max Reviewed at |
Collaborator
|
Confidence: 4/5 — a clean, subtractive parity fix verified across all four daemon entry points; the only open items are the still-running unit suite and the absence of an automated wire-level check for the breaker scenario. Stepping back: this PR's approach is exactly the one I would have proposed independently — route the breaker through the existing loop-detected machinery instead of keeping a second, silent stop path. That's why it ends up net-negative on production lines: no new abstraction, just the deletion of a bespoke flag whose special-casing was the bug. The problem was real (verified in code on What I verified rather than took on faith: all four What keeps this from 5/5: the primary unit suite was still running at review time, and no automated check drives the actual breaker scenario (a provider replaying an answered tool-call id through a live daemon) — the Approval deferred until CI lands green on 中文说明置信度：4/5 —— 一个干净的删减型对齐修复，四个 daemon 入口均已验证；仅剩事项是单元套件仍在运行，以及熔断场景尚无自动化 wire 级检查。 回头看：这个 PR 的方案与我独立提出的方案完全一致——把熔断接入既有的 loop-detected 机制，而不是保留第二条静默停止路径。正因如此，生产代码净减少：没有新抽象，只是删除了专用标志，而它的特殊化处理正是 bug 本身。问题真实（已在 我没有轻信而是逐项验证：四个 未到 5/5 的原因：审查时主单元套件仍在运行；且没有任何自动化检查驱动真实熔断场景（provider 通过运行中的 daemon 重放已应答的 tool-call id）——Stage 2 已点名 批准将推迟到 CI 在 — Qwen Code · qwen3.8-max Reviewed at |
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. |
qwen-code-ci-bot
approved these changes
Aug 19, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
LGTM, looks ready to ship — CI landed green after the review. ✅
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
中文说明
已审查——无阻断问题。 建议见行内评论。
— qwen3.8-max via Qwen Code /review (v0.21.13)
Comment on lines
+7599
to
+7603
| expect(debugLoggerWarnSpy).toHaveBeenCalledWith( | ||
| expect.stringContaining( | ||
| 'Stopping ACP turn after repeated duplicate provider tool-call id: shell_1', | ||
| ), | ||
| ); |
Collaborator
There was a problem hiding this comment.
[Suggestion] Neither of the two tests this PR updates pins the deliverable the design doc it adds promises for this path: the LoopDetectedEvent telemetry. logLoopDetectedSpy is never asserted for this trigger — every one of its ~15 existing assertions in this file is for other loop types (e.g. TURN_TOOL_CALL_CAP at 16610/28216/33814/33903) — so the breaker branch's telemetry wiring (the recordDaemonLoopDetected call in Session.ts) is pinned by zero tests. — Failure scenario: a future refactor of the breaker branch that inlines the state mutation and warn without recordDaemonLoopDetected keeps both updated tests green while silently dropping the telemetry event again, restoring the exact invisibility this PR eliminates. Verified by mutation probe at f350e83: removing the telemetry call leaves both tests green (Tests 2 passed | 637 skipped); adding the assertion below catches it (AssertionError: expected "spy" to be called with arguments).
Suggested change
| expect(debugLoggerWarnSpy).toHaveBeenCalledWith( | |
| expect.stringContaining( | |
| 'Stopping ACP turn after repeated duplicate provider tool-call id: shell_1', | |
| ), | |
| ); | |
| expect(debugLoggerWarnSpy).toHaveBeenCalledWith( | |
| expect.stringContaining( | |
| 'Stopping ACP turn after repeated duplicate provider tool-call id: shell_1', | |
| ), | |
| ); | |
| expect(logLoopDetectedSpy).toHaveBeenCalledWith( | |
| mockConfig, | |
| expect.objectContaining({ | |
| loop_type: core.LoopType.GLOBAL_TOOL_CALL_DUPLICATE, | |
| }), | |
| {}, | |
| ); |
中文说明
[建议] 本 PR 更新的两个测试都没有为其设计文档承诺的该路径交付物——LoopDetectedEvent 遥测——添加断言。logLoopDetectedSpy 在该触发器上从未被断言——本文件中约 15 处现有断言全部针对其他 loop 类型（如 16610/28216/33814/33903 处的 TURN_TOOL_CALL_CAP）——因此熔断路径的遥测接线（Session.ts 中的 recordDaemonLoopDetected 调用）没有任何测试保护。— 失败场景：未来对熔断路径的重构如果绕过 recordDaemonLoopDetected、直接内联状态赋值与 warn 日志，两个更新后的测试仍会全部通过，而遥测事件会再次被静默丢弃，重新引入本 PR 要消除的不可见停止。已在 f350e83 上通过变异探针验证：移除遥测调用后两个测试依旧通过（Tests 2 passed | 637 skipped）；加入下方断言后可将其捕获（AssertionError: expected "spy" to be called with arguments）。
— qwen3.8-max via Qwen Code /review (v0.21.13)
yiliang114
approved these changes
Aug 19, 2026
yiliang114
left a comment
yiliang114
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed the diff (3 files: Session.ts + Session.test.ts + a new design doc).
Verdict: approve. A correct, well-scoped fix — it makes a previously silent hang diagnosable by routing the daemon duplicate tool-call breaker through the loop-detected machinery that already exists, instead of a bespoke flag that only produced a debug log.
What I verified against the source:
-
The routing is correct.
recordDaemonLoopDetected(config, promptId, loopType, message, toolLoopState)setsloopState.loopDetected = true+loopType, logs atwarn(an upgrade from the olddebugline), and emitsLoopDetectedEventtelemetry. ThetoolRun.loopDetectedreturn then flows into the caller's existing handling (line ~5083):todoStopGuard.suspend()+#preserveStoppedToolRun(preservesLOOP_DETECTED_CONTEXT_MESSAGE) +rejectOnLoopDetected ? cancelledOrThrowLoopDetected(...) : getAbortAwareEndTurnStopReason(...)— i.e. foreground rejects with theLOOP_DETECTEDturn error carryingloopType, cron/background ends gracefully. So the three guarantees the PR body claims are genuinely provided by the reused plumbing, not re-implemented. -
The removed
repeatedDuplicateProviderToolCallbranch was genuinely dead weight. Its oldtodoStopGuard.suspend()+return { message: null }is now subsumed by the loop-detected caller path, so no suspend is lost. -
The
toolLoopStateoptionality is handled.runToolCalls(toolLoopState?: ...)— theif (toolLoopState) … else debugLogger.warnfallback degrades gracefully (warn, no telemetry) when called without state; all production call sites pass it, so this is defensive only. -
Test coverage is precise. The updated test pins the foreground rejection shape (
data.code === 'LOOP_DETECTED',loopType === GLOBAL_TOOL_CALL_DUPLICATE), the warn message, and the context-message preservation intoaddHistory. The second test pinstoolLoopState.loopDetected/loopTypeare set.
One non-blocking nit: the design doc header says Status: approved while the PR is still REVIEW_REQUIRED — consider it "design approved, PR pending" or drop the status until merged. Not a correctness issue.
Did not re-run the Session test suite — reviewed the diff + traced the loopDetected plumbing directly in source.
Collaborator
Author
|
[qwen] @yiliang114 Thanks for the thorough verification — all four traced points match the intent. On the status nit: agreed, the header conflated design approval with PR state. Rather than amending it mid-flight on #9436 (which just picked up a fresh approval that a new push would invalidate), the header will be corrected in a small follow-up once the stack lands — switching |
Collaborator
|
Released in v0.21.15. |
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
3 participants
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.
What this PR does
When the repeated-duplicate provider tool-call circuit breaker (added in #5657) fires inside an ACP daemon session, the turn now terminates the same way as every other daemon loop-protection stop: a foreground prompt fails with the standard loop-detected error carrying the
global_tool_call_duplicateloop type, the loop-context note is preserved into unsent history for the next turn, and the loop-detected telemetry event is emitted. Cron and background-notification turns keep their existing graceful end-turn semantics, matching how daemon loop detection already behaves. The bespoke result flag this path used internally is removed in favor of the existing loop-detected plumbing.Why it's needed
PR #5657's review required user-visible termination for this circuit breaker and delivered it on three of the four entry points: the subagent runtime terminates with a loop-detected mode, the TUI posts the loop message, and the non-interactive CLI emits a
global_tool_call_duplicateloop result. The ACP daemon session was the outlier: the tool batch was dropped with only a debug log — no transcript output, no telemetry, and the prompt resolved as a normal end of turn. To the user the session simply looked hung mid-turn. This was observed live in local daemon sessions where a model kept reusing a provider tool-call id: each prompt died silently by its third round, with the transcript ending on a thought fragment. This PR makes the stop diagnosable on its own; the root cause of the false-positive id collisions is fixed separately in the stacked follow-up PR.Reviewer Test Plan
How to verify
Drive a daemon session (
qwen serve+ any ACP client) with an OpenAI-compatible provider that replays an already-answered tool-call id with identical arguments on consecutive rounds — a deterministic mock provider works. Expected: round 1 executes; round 2 receives the synthetic duplicate-error tool result; round 3 fails the prompt with the visible loop-protection error ("Tool-call loop protection stopped this turn. The session is still available; send a more specific instruction to continue.", error data carryingloopType: global_tool_call_duplicate) instead of silently ending, and the session stays usable for the next prompt.Focused regression suite (639→640 tests, includes the updated prompt-level case asserting the rejection, the preserved context message, and the warn log):
Evidence (Before & After)
Before: the breaker path returned an empty tool run and the prompt resolved
end_turnwith nothing user-visible — daemon transcripts end mid-turn on a thought fragment with no tool result and no assistant text (observed on v0.21.13 daemon sessions).After: the prompt rejects with the
LOOP_DETECTEDturn error (loop type included),System: this turn was terminated because the model exceeded tool-call safety limits...is preserved for the next turn, andLoopDetectedEventtelemetry is recorded. N/A for screenshots (headless/daemon protocol behavior).Tested on
Environment (optional)
Unit suites via vitest; behavior also exercised end-to-end on the bundled CLI as part of the stacked follow-up PR's mock-provider verification.
Risk & Scope
Linked Issues
Design doc committed under
docs/design/2026-08-19-duplicate-provider-toolcall-id-guard.md(this PR implements decision D1).中文说明
本 PR 做了什么
当 #5657 引入的重复 provider tool-call 熔断在 ACP daemon 会话中触发时，turn 现在与 daemon 其他 loop 保护停止走完全相同的通路终止：前台 prompt 以标准 loop-detected 错误失败（携带
global_tool_call_duplicateloop 类型）、loop 上下文提示保留进未发送历史供下一轮使用、并发出 loop-detected 遥测事件。cron 与后台通知 turn 保持既有的优雅 end-turn 语义，与 daemon 现有 loop 检测行为一致。该路径内部使用的专用结果标志被移除，改用既有 loop-detected 管线。为什么需要
PR #5657 的 review 要求该熔断必须用户可见地终止，四个入口中有三个做到了：subagent 运行时以 loop-detected 模式终止、TUI 落 loop 消息、非交互 CLI 输出
global_tool_call_duplicateloop 结果。ACP daemon 会话是遗漏项：整批工具调用被丢弃时只有一行 debug 日志——transcript 无输出、无遥测，prompt 以正常 end-turn 结束。对用户来说会话就像卡死在 turn 中间。本地 daemon 会话已实际观察到该现象：模型持续复用 provider tool-call id 时，每个 prompt 都在第三轮静默死亡，transcript 停在一段思考文本上。本 PR 使该停止自身可诊断；误判 id 碰撞的根因由堆叠的后续 PR 修复。Reviewer 测试计划
如何验证
用会在连续轮次重放同 id 同参数工具调用的 OpenAI 兼容 provider（确定性 mock 即可）驱动 daemon 会话（
qwen serve+ 任意 ACP 客户端）。预期：第 1 轮执行；第 2 轮收到合成 duplicate 错误工具结果；第 3 轮 prompt 以可见的 loop 保护错误失败（错误数据携带loopType: global_tool_call_duplicate）而非静默结束，会话对下一个 prompt 仍可用。回归套件（639→640 测试，含更新后的 prompt 级用例，断言拒绝、保留的上下文消息与 warn 日志）：
cd packages/cli && npx vitest run src/acp-integration/session/Session.test.ts证据（Before & After）
Before：熔断路径返回空工具结果、prompt 以
end_turn正常结束，用户不可见——daemon transcript 停在思考片段上，无工具结果、无助手正文（v0.21.13 daemon 会话实测）。After：prompt 以
LOOP_DETECTEDturn 错误拒绝（含 loop 类型），"System: this turn was terminated because the model exceeded tool-call safety limits..." 保留给下一轮，并记录LoopDetectedEvent遥测。无 UI 截图（headless/daemon 协议行为）。测试平台
macOS ✅；Windows / Linux⚠️ 未本地验证（依赖 CI）。
风险与范围
关联 Issue
设计文档见
docs/design/2026-08-19-duplicate-provider-toolcall-id-guard.md（本 PR 实现其中决策 D1）。