fix(cli): stop repeated duplicate provider responses - #5657
Conversation
|
@qwen-code /triage |
|
Thanks for the PR! (Re-run after latest revision.) Template looks good ✓ — all required sections present, bilingual body, linked issue, test plan with commands. On direction: this fixes a real, deterministic bug (#5641) where an OpenAI-compatible provider replays a completed tool-call id in a loop. Users hit this in production. Clearly aligned with qwen-code's core mission of reliable tool execution. On approach: the scope feels right. The fix adds a shared Moving on to code review. 🔍 中文说明感谢贡献！（最新修订后重新运行。） 模板完整 ✓ — 所有必需章节齐全，双语正文，关联 issue，带命令的测试计划。 方向：修复了一个真实的、确定性的 bug（#5641）—— OpenAI 兼容 provider 会循环重放已完成的 tool-call id。用户已在生产中遇到。与 qwen-code 可靠工具执行的核心使命完全一致。 方案：范围合理。在 进入代码审查 🔍 — Qwen Code · qwen3.7-max |
Code reviewThe implementation is clean and correct. The shared primitive
ACP's per-prompt Set clear (in One nuance: the fresh sibling tool call paired with the first replay still executes (by design — the first replay gets a synthetic response, the breaker fires on the second replay). This is the documented conservative trade-off and matches the PR's intent to preserve first-replay pairing. No correctness bugs, security issues, or regressions found. No AGENTS.md violations — no over-abstraction, the helper lives in the right package ( Focused unit test resultsAll 15 focused tests pass across all entry points: CI: all checks green (3 successful, 14 skipped). Note: E2E reproduction with a mock provider requires a custom harness (the bug is provider-side behavior that can't be triggered via a normal 中文说明代码审查实现简洁且正确。 agent-core 的 ACP 的 per-prompt Set 清理（在 一个细节：与第一次重放配对的 fresh sibling tool call 仍然会执行（这是有意为之 —— 第一次重放收到合成响应，断路器在第二次重放时才触发）。这是文档化的保守取舍，与 PR 保留首次重放配对的意图一致。 未发现正确性 bug、安全问题或回归。未违反 AGENTS.md —— 没有过度抽象，helper 放在正确的包中（ 聚焦单测结果全部 15 个聚焦测试通过： CI：全部检查绿色（3 个成功，14 个跳过）。 注意：用 mock provider 进行 E2E 复现需要自定义测试框架（该 bug 是 provider 端行为，无法通过普通 — Qwen Code · qwen3.7-max |
ReflectionThis is a solid bug fix for a real production issue. The provider tool-call replay loop (#5641) could trap users in a seemingly infinite run with repeated tool-result submissions. The fix is minimal in concept — track which provider ids have already received a synthetic duplicate response, and drop the batch when the same id appears again — but it has to be applied in four separate places because each tool-call processing loop is independent. The implementation matches what I'd propose independently: a shared detection helper in What makes this PR stand out is the verification depth. Beyond the 15 focused unit tests (all passing here), three independent maintainer verifications — including mutation testing that proved 5/14 tests catch the actual bug, real-binary E2E with mock providers showing 41→4 request reduction, and Scenario B demonstrating that the fresh-sibling-in-batch case is uniquely caught by this fix — provide strong confidence. The one honest caveat (noted by both the author and reviewers): the fresh sibling paired with the first replay still executes once before the breaker trips. This is a conscious trade-off to preserve first-replay pairing, and it's documented in the PR body. Verdict: ships a focused, well-tested fix for a real bug. Clean to review and maintain. Approving. ✅ 中文说明反思这是一个针对真实生产问题的扎实 bug 修复。provider tool-call 重放循环（#5641）会让用户陷入看似无限的运行，不断重复提交 tool-result。修复在概念上是最小的 —— 跟踪哪些 provider id 已收到合成 duplicate 响应，在同一 id 再次出现时丢弃整批 —— 但必须在四个独立位置应用，因为每个 tool-call 处理循环都是独立的。 实现与我独立提出的方案一致： 这个 PR 突出之处在于验证深度。除了 15 个聚焦单测（此处全部通过），还有三轮独立的维护者验证 —— 包括证明 14 个测试中有 5 个能捕获实际 bug 的变异测试、显示请求从 41→4 减少的真实二进制 E2E mock provider 测试、以及证明 batch 中 fresh-sibling 情况仅被此修复捕获的场景 B —— 提供了充分的信心。 一个诚实的说明（作者和 reviewer 都已提到）：与第一次重放配对的 fresh sibling 在断路器触发前仍会执行一次。这是为保留首次重放配对而有意识的取舍，已在 PR 正文中记录。 结论：为一个真实 bug 交付了聚焦、经过充分测试的修复。易于审查和维护。批准。✅ — 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. ✅
Maintainer verification — local real-binary + A/B testingI built and ran this PR locally and went beyond re-running the unit tests: I (a) proved the new tests actually catch the bug via a merge-base overlay, and (b) reproduced #5641 end-to-end against the real Verdict: ✅ Works as described. Recommend merge. Two honest nuances for reviewers at the bottom. Setup
1. Tests on the fix — all green
2. A/B: do the new tests actually exercise the fix?Overlaid the baseline versions of the 5 source files (
3. Real-binary E2E — reproducing #5641Mock provider modeled on the issue's reproducer: request 1 → tool-call id
Scenario B is the important one. When the replayed completed id is mixed with a fresh sibling tool call, the always-on 4. Typecheck / lint
Two nuances for reviewers (not blockers)
中文版（点击展开）维护者验证 — 本地真实二进制 + A/B 测试我在本地构建并运行了这个 PR，并且没有止步于重跑单元测试：(a) 通过 merge-base 覆盖证明了新增测试确实能捕获该 bug；(b) 用一个 mock OpenAI-compatible provider 对真实 结论：✅ 行为与描述一致，建议合并。 文末有两点供 reviewer 留意的诚实说明。 环境
1. 修复版上的测试 —— 全绿
2. A/B：新增测试是否真的覆盖了修复？把 5 个源文件（
3. 真实二进制 E2E —— 复现 #5641Mock provider 按 issue 的 reproducer 建模：第 1 个请求 → tool-call id
场景 B 是关键。当被重放的已完成 id 与一个 fresh sibling tool call 混在一起时，常驻的 4. Typecheck / lint
两点供 reviewer 留意（非阻塞）
|
| ) { | ||
| loopDetectedMessage = emitLoopDetectedMessage( | ||
| config, | ||
| LoopType.GLOBAL_TOOL_CALL_DUPLICATE, |
There was a problem hiding this comment.
[Critical] The loop detection message tells users they can disable this guard via skipLoopDetection, but GLOBAL_TOOL_CALL_DUPLICATE is not in the isAlwaysOn list (line 128-130). The circuit breaker in processToolCallBatch is unconditional — toggling skipLoopDetection has no effect.
| LoopType.GLOBAL_TOOL_CALL_DUPLICATE, | |
| const isAlwaysOn = | |
| loopType === LoopType.TURN_TOOL_CALL_CAP || | |
| loopType === LoopType.CONSECUTIVE_IDENTICAL_TOOL_CALLS || | |
| loopType === LoopType.GLOBAL_TOOL_CALL_DUPLICATE; |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
There was a problem hiding this comment.
fixed. GLOBAL_TOOL_CALL_DUPLICATE is now treated as always-on in the headless message, and the repeated-duplicate test pins that it does not suggest skipLoopDetection.
| const pushDuplicateBatch = (request: ToolCallRequestInfo): void => { | ||
| const providerCallId = request.providerCallId ?? request.callId; | ||
| if ( | ||
| !markDuplicateProviderToolCallResponseSent( |
There was a problem hiding this comment.
[Suggestion] The per-call markDuplicateProviderToolCallResponseSent false-branch inside pushDuplicateBatch is dead code. The pre-scan above (line ~3235) already catches all repeated duplicate cases — duplicateProviderToolCallResponseIds.has(providerCallId) and count > 1 — and returns early. The return false path in markDuplicateProviderToolCallResponseSent is never reached at any of the 5 call sites across the 4 implementations (Session.ts, nonInteractiveCli.ts, useGeminiStream.ts, agent-core.ts).
Suggested fix: remove the false-branch and repeatedDuplicateProviderToolCall mutation. Keep only this.duplicateProviderToolCallResponseIds.add(providerCallId) to populate the tracking set.
— DeepSeek/deepseek-v4-pro via Qwen Code /review
There was a problem hiding this comment.
cleaned this up. The per-call false branch is gone; repeated duplicates are handled by the batch pre-scan, and the duplicate path now only marks the provider id before sending the first synthetic response.
| debugLogger.debug( | ||
| `[processGeminiStreamEvents] Dropping batch after repeated duplicate provider tool-call id: ${repeatedDuplicateRequest.providerCallId} (tool: ${repeatedDuplicateRequest.name})`, | ||
| ); | ||
| return StreamProcessingStatus.Completed; |
There was a problem hiding this comment.
[Suggestion] When the circuit breaker fires in the TUI path, processGeminiStreamEvents returns StreamProcessingStatus.Completed with only a debugLogger.debug call — no user-visible feedback. The turn ends silently. Compare with nonInteractiveCli.ts which calls emitLoopDetectedMessage and exits with code 1.
Suggested fix: set loopDetectedRef.current = true before returning Completed so the TUI displays a visible loop-detected message.
— DeepSeek/deepseek-v4-pro via Qwen Code /review
There was a problem hiding this comment.
done. The TUI repeated-duplicate batch path now sets loopDetectedRef.current = true before completing, so the visible loop-detected message is shown.
| } | ||
|
|
||
| if ( | ||
| !markDuplicateProviderToolCallResponseSent( |
There was a problem hiding this comment.
[Suggestion] In the unique batch loop, markDuplicateProviderToolCallResponseSent receives requestInfo.providerCallId (type string | undefined), while the adjacent duplicate batch loop correctly resolves it via requestInfo.providerCallId ?? requestInfo.callId. The pre-scan and all other 3 implementations consistently use the resolved value.
| !markDuplicateProviderToolCallResponseSent( | |
| !markDuplicateProviderToolCallResponseSent( | |
| requestInfo.providerCallId ?? requestInfo.callId, | |
| duplicateProviderToolCallResponseIds, | |
| ) |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
There was a problem hiding this comment.
updated. The non-interactive path now resolves the id consistently before marking the duplicate response. I kept the structured-output special case from the existing tests so sibling structured_output calls with no provider id do not get misclassified as provider duplicates.
✅ Local verification report (maintainer)Built and tested this PR locally to confirm the fix before merge. Environment
Results
Mutation test (the key evidence). I reverted the four layer source files ( The other 387 tests stay green. The headless test is the behavioral reproduction of #5641: a deterministic provider re-submits a completed tool call across 3 streamed batches, and it asserts the real tool Correctness review
Minor / non-blocking observation. The ~20-line "repeated duplicate" detection block (build the count map, then Verdict: LGTM. A serious re-submission-loop bug; the fix is correct, applied uniformly across all four paths, and every layer is mutation-verified. Merges cleanly, types/lint/format clean. Safe to merge from my side. 🇨🇳 中文版✅ 本地验证报告（维护者）合并前在本地构建并测试了本 PR 以确认修复。 环境
结果
变异测试（关键证据）。 我把四个层的源文件（ 其余 387 个测试保持绿色。headless 那个测试就是 #5641 的行为复现：一个确定性 provider 在 3 个流式 batch 里重复提交一个已完成的 tool call，断言真实工具 正确性审查
次要 / 非阻塞观察。 那段 ~20 行的「重复检测」逻辑（建计数 map 再 结论：LGTM。 一个严重的重复提交循环 bug；修复正确、在全部四条路径上一致应用、每层都经变异测试验证。可干净合并，类型/lint/格式干净。从我这边看可以安全合并。 |
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
No additional findings from my pass. I did not duplicate the existing open review threads; those remain the actionable items to resolve before treating the PR as fully clear.
Local verification: focused duplicate-provider-id tests passed for non-interactive CLI, TUI stream handling, ACP session, and AgentCore; npm run build, npm run typecheck, and git diff --check passed. CI is green.
— GPT-5 Codex via Qwen Code /review
48d19cb to
e056681
Compare
| // ACP can continue one logical conversation through prompt, cron, and | ||
| // background loops, so keep this with the session instead of a single | ||
| // runToolCalls invocation. | ||
| private readonly duplicateProviderToolCallResponseIds = new Set<string>(); |
There was a problem hiding this comment.
[Critical] duplicateProviderToolCallResponseIds is a session-scoped Set<string> that is never cleared between ACP prompts — unlike every other code path:
- TUI:
duplicateProviderToolCallResponseIdsRef.current.clear()at each new user submit (useGeminiStream.ts:2047) - nonInteractiveCli: local variable, fresh per invocation
- agent-core: local variable, fresh per reasoning loop
In a long-lived ACP session, once a provider tool-call ID is flagged in prompt N, any subsequent prompt where the model legitimately emits the same provider ID will immediately trip the circuit breaker — the entire batch is dropped and the turn silently terminates. The bug worsens over time as the Set grows monotonically.
| private readonly duplicateProviderToolCallResponseIds = new Set<string>(); | |
| private readonly duplicateProviderToolCallResponseIds = new Set<string>(); |
Then add this.duplicateProviderToolCallResponseIds.clear() at the top of prompt() or #executePrompt, alongside other per-prompt resets. If the class-level lifetime is truly intended (per the comment above), document the trade-off and verify that provider IDs never collide across prompts.
— qwen3.7-max via Qwen Code /review
| duplicateProviderToolCallResponseIds, | ||
| ); | ||
| if (toolCallResult.repeatedDuplicateProviderToolCall) { | ||
| terminateMode = AgentTerminateMode.ERROR; |
There was a problem hiding this comment.
[Suggestion] When the circuit breaker fires here, terminateMode = AgentTerminateMode.ERROR gives no user-facing indication that the failure was a duplicate-tool-call loop. Compare with nonInteractiveCli.ts which calls emitLoopDetectedMessage(config, LoopType.GLOBAL_TOOL_CALL_DUPLICATE) and the TUI which sets loopDetectedRef.current = true. At 3 AM, "Agent stopped due to an error" gives zero signal about the root cause.
Consider emitting a debug log or user-facing loop-detected message before the break, or adding a dedicated AgentTerminateMode.LOOP_DETECTED.
— qwen3.7-max via Qwen Code /review
| @@ -661,6 +661,73 @@ describe('runNonInteractive', () => { | |||
| expect(processStdoutSpy).toHaveBeenCalledWith('Final answer\n'); | |||
| }); | |||
|
|
|||
There was a problem hiding this comment.
[Suggestion] Test coverage gaps:
-
Count-in-batch (>1) trigger path untested — All 4 new tests exercise only the cross-batch Set-based trigger. None test the within-batch
(count ?? 0) > 1branch where two copies of the same already-handled provider ID appear in a single response batch. This is a distinct code branch in all 4 implementations. -
Drain-item loop handling untested — The drain path's circuit breaker (
nonInteractiveCli.ts:~1501) setsloopDetected = trueand returns from the inner function, but no test covers a drain item triggering the circuit breaker.
— qwen3.7-max via Qwen Code /review
| // Build allowed tool names set for filtering | ||
| const allowedToolNames = new Set(toolsList.map((t) => t.name)); | ||
| const repeatedDuplicateProviderIds = new Map<string, number>(); | ||
| for (const fc of uniqueFunctionCalls) { |
There was a problem hiding this comment.
[Suggestion] The 3-step repeated-duplicate detection (count Map → find first repeated → short-circuit) is structurally copy-pasted in Session.ts:3234, useGeminiStream.ts:1870, and nonInteractiveCli.ts:845. Any semantic fix must be applied in 4 places. Consider extracting a shared helper in turn.ts alongside the existing createDuplicateProviderToolCallResponse:
export function findRepeatedDuplicateProviderId<T>(
items: T[],
getId: (item: T) => string | undefined,
knownIds: Set<string>,
previouslySentIds: Set<string>,
): T | undefined { ... }Each callsite passes its own ID-extraction function.
— qwen3.7-max via Qwen Code /review
✅ Real-environment verification report — PR #5657 (fixes #5641)I built the real CLI from this PR and ran an A/B against a mock OpenAI‑compatible provider that replays an already‑completed tool‑call id (the exact shape of #5641). Verified across all four tool‑execution loops. Setup
Headline A/B —
|
| Tool loop (entry point) | BASE d350dd8df |
PR e05668109 |
|---|---|---|
Non‑interactive qwen -p (nonInteractiveCli.ts) |
41 provider requests — loops until the mock's safety valve | 3 requests, halted by always‑on global_tool_call_duplicate |
Daemon / ACP qwen serve (Session.runToolCalls) |
42 provider requests — loops until safety valve | 4 requests, [Session.runToolCalls] Dropping batch… |
Interactive TUI (useGeminiStream.ts, driven via tmux) |
42 requests, 39 synthetic duplicate submissions, only stopped by the mock | 3 requests, shows "A potential loop was detected" dialog |
Subagent / headless (agent-core.ts) |
— | PR's focused agent-headless regression tests pass (3/3) |
In every arm the shell side‑effect ran exactly once (HIT=1) → the bug is repeated result submission, not re‑execution; the PR preserves single execution.
The guard fired in each path (PR debug logs, QWEN_DEBUG_LOG_FILE=1)
-p:[NON_INTERACTIVE_CLI] [runNonInteractive] Dropping batch after repeated duplicate provider tool-call id: call_dup_0001- daemon:
[SESSION] [Session.runToolCalls] Dropping batch after repeated duplicate provider tool-call id …+Stopping ACP turn after dropping repeated duplicate provider tool-call response. - TUI:
[GEMINI_STREAM] [processGeminiStreamEvents] Dropping batch after repeated duplicate provider tool-call id …
PR -p terminal result: Loop detection halted the run (global_tool_call_duplicate: the model repeated the same tool call across the turn, even when not back-to-back). This is an always-on guard…
Additional scenarios
- Identical‑args replay (
-p): BASE submitted 4 accumulating synthetic duplicate results (__qwen_dup_2/3/4) before the pre‑existingconsecutive_identical_tool_callsguard caught it at turn 5; PR halts at turn 3 with 1 synthetic result. The PR catches it earlier and with a guard specific to the actual failure mode. - Duplicate id + a fresh sibling tool call in the same batch (
-p): BASE executed the fresh sibling (SIBLING=1); PR dropped the whole batch so the sibling did not run (SIBLING=0) — matches the PR's intended "no partial tool responses" contract.
Notes
- First replay still pairs correctly: exactly one synthetic
duplicate provider tool call … was already handledresponse is sent before the batch‑drop kicks in on the next replay. ✔ - Unit suites from the PR test plan on this build:
agent-headless3 pass,Session5 pass.nonInteractiveCli.test.ts/useGeminiStream.test.tsxfailed to load here due to an environment‑only dependency‑resolution issue (@qwen-code/web-templatesentry / testing‑library), unrelated to the change — both of those paths are independently proven by the live E2E above.
Verdict: the fix resolves #5641 and the loop is broken on all four tool‑execution paths, with single‑execution and first‑replay pairing preserved. LGTM from a behavioral standpoint. 👍
中文版验证报告（点击展开）
✅ 真实环境验证报告 — PR #5657（修复 #5641）
我用本 PR 实际构建了 CLI，并针对一个 会重放“已完成 tool-call id”的 mock OpenAI 兼容 provider（正是 #5641 的复现形态）做了 A/B 对比，覆盖了全部四条 tool 执行回路。
环境
- A/B 使用相同的工具链 /
node_modules，仅 5 个改动源文件不同：- BASE = merge-base
d350dd8df（无修复） · PR = heade05668109（有修复）
- BASE = merge-base
- 打包后的 CLI
v0.19.0（node esbuild.config.js）；provider 为零依赖 mock，每一轮都返回同一个 provider tool-call id（run_shell_command追加标记，用于统计真实执行次数）；--approval-mode yolo。 - mock 设了 40 轮安全阀，避免 buggy 构建无限循环；BASE 能跑到 ~40 即说明它本身永不自停（原 issue 观测到 101 次）。
核心 A/B — loopvary（重放同一 id，但参数每轮变化，从而绕过既有的 consecutive_identical_tool_calls 守卫，单独检验本 PR 新增的守卫）
| Tool 回路（入口） | BASE d350dd8df |
PR e05668109 |
|---|---|---|
非交互 qwen -p（nonInteractiveCli.ts） |
41 次 provider 请求 — 一直循环直到 mock 安全阀 | 3 次，被常开守卫 global_tool_call_duplicate 中止 |
Daemon / ACP qwen serve（Session.runToolCalls） |
42 次请求 — 循环至安全阀 | 4 次，[Session.runToolCalls] Dropping batch… |
交互 TUI（useGeminiStream.ts，经 tmux 驱动） |
42 次请求，39 次合成重复提交，仅靠 mock 才停下 | 3 次，弹出 “A potential loop was detected” 对话框 |
子代理 / headless（agent-core.ts） |
— | PR 自带 agent-headless 回归用例通过（3/3） |
每一组 shell 副作用都只执行了 一次（HIT=1）→ bug 是重复提交结果，而非重复执行；本 PR 保持了“只执行一次”。
各路径守卫确实触发（PR 的 debug 日志，QWEN_DEBUG_LOG_FILE=1）
-p：[NON_INTERACTIVE_CLI] [runNonInteractive] Dropping batch after repeated duplicate provider tool-call id: call_dup_0001- daemon：
[SESSION] [Session.runToolCalls] Dropping batch …+Stopping ACP turn after dropping repeated duplicate provider tool-call response. - TUI：
[GEMINI_STREAM] [processGeminiStreamEvents] Dropping batch …
PR -p 终态：Loop detection halted the run (global_tool_call_duplicate: …). This is an always-on guard…
附加场景
- 相同参数重放（
-p）：BASE 在被既有consecutive_identical_tool_calls守卫于第 5 轮拦下之前，已累计提交 4 次合成重复结果（__qwen_dup_2/3/4）；PR 在第 3 轮即停，仅 1 次合成结果。本 PR 更早拦截，且用的是针对该失败模式的专用守卫。 - 重复 id 与一个全新 sibling tool call 同批（
-p）：BASE 执行了该全新 sibling（SIBLING=1）；PR 整批丢弃，sibling 未执行（SIBLING=0）—— 符合本 PR “不发送部分 tool 响应”的设计约定。
说明
- 首次重放仍正确配对：在下一次重放触发整批丢弃之前，恰好发送 一条
duplicate provider tool call … was already handled合成响应。✔ - PR 测试计划中的单测在本构建：
agent-headless3 通过、Session5 通过；nonInteractiveCli.test.ts/useGeminiStream.test.tsx因纯环境依赖解析问题（@qwen-code/web-templates入口 / testing-library）加载失败，与本改动无关 —— 这两条路径已由上面的实跑 E2E 独立证明。
结论：该修复解决了 #5641，四条 tool 执行路径上的循环均被打断，且保持“只执行一次”与“首次重放配对”。从行为层面 LGTM。 👍
Verification method: built BASE vs PR bundles from an isolated git worktree; deterministic mock OpenAI provider; A/B over qwen -p, qwen serve (HTTP/ACP), and the tmux‑driven TUI; metrics = provider request count + real shell executions + debug‑log guard traces.
|
@qwen-code /triage |
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. ✅
35a04d5 to
60cc667
Compare
|
Thanks, I checked the latest revision. My previous blocking concerns look addressed: ACP duplicate-response tracking is now cleared per prompt, repeated-duplicate detection is shared, the TUI path now surfaces loop detection, and the non-interactive drain path has coverage. CI is green from the current check rollup. I am satisfied with the follow-up revision. 中文说明谢谢，我检查了最新版本。之前阻塞性的反馈看起来已经处理：ACP duplicate-response tracking 现在会按 prompt 清理，repeated-duplicate 检测已抽成共享逻辑，TUI 路径现在会展示 loop detection，non-interactive drain 路径也补了覆盖。当前 CI 结果为绿色。我对这轮修改满意。 |
|
@qwen-code /triage |
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. ✅
104e983 to
ef4bbe8
Compare
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Re-reviewed the latest revision. All previous blocking concerns are addressed: ACP duplicate-response tracking is now cleared per prompt, repeated-duplicate detection is shared via findRepeatedDuplicateProviderToolCall, the TUI path surfaces loop detection, AgentCore uses LOOP_DETECTED termination mode, and the non-interactive path resolves IDs consistently. Build passes, all 25 focused tests pass. LGTM. ✅
— qwen3.7-max via Qwen Code /review
|
@qwen-code /triage |
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. ✅
| return { text: 'Agent stopped: time limit reached.', level: 'warning' }; | ||
| case AgentTerminateMode.ERROR: | ||
| return { text: 'Agent stopped due to an error.', level: 'error' }; | ||
| case AgentTerminateMode.LOOP_DETECTED: |
There was a problem hiding this comment.
[Suggestion] forkedAgent.ts:559 misclassifies LOOP_DETECTED as 'completed' — the failure branch only checks CANCELLED, ERROR, and TIMEOUT. Forked agents (dream, extraction, skill-review) that hit this new circuit breaker would be treated as successful completions, producing incomplete output.
Fix in forkedAgent.ts:
if (
terminateReason === AgentTerminateMode.ERROR ||
terminateReason === AgentTerminateMode.TIMEOUT ||
terminateReason === AgentTerminateMode.LOOP_DETECTED
) {— qwen3.7-max via Qwen Code /review
| // is a model-facing detail, not part of the original message. | ||
| this.emitExternalInputEvents(externalInputs); | ||
| } | ||
| if ((currentMessages[0]?.parts?.length ?? 0) === 0) { |
There was a problem hiding this comment.
[Suggestion] This empty-parts guard is unreachable with the current implementation — processFunctionCalls always returns non-empty toolResponseParts when repeatedDuplicateProviderToolCall is false (every call hits unauthorized-error, duplicate-error, or executed-tool path). The LOOP_DETECTED break above already covers the only known empty-parts scenario. If a future refactor makes this reachable, it terminates as generic ERROR with no debug log. Consider adding a comment explaining this is purely defensive, or adding a debugLogger.warn before the break.
— qwen3.7-max via Qwen Code /review
| return emitStructuredSuccess(); | ||
| } | ||
| if ( | ||
| repeatedDuplicateProviderToolCall && |
There was a problem hiding this comment.
[Suggestion] The toolResponseParts.length === 0 condition is always true when repeatedDuplicateProviderToolCall is true — processToolCallBatch always returns empty responseParts alongside the flag. If a future change makes it return non-empty parts with the flag set, this guard silently skips the loop-detection exit. Consider dropping the redundant condition or replacing it with an assertion.
— qwen3.7-max via Qwen Code /review
| duplicateProviderToolCallResponseIds.add(providerCallId); | ||
| } | ||
|
|
||
| export function findRepeatedDuplicateProviderToolCall<T>( |
There was a problem hiding this comment.
[Suggestion] The two-stage pipeline (handledProviderToolCallIds = all processed IDs, duplicateProviderToolCallResponseIds = subset that received synthetic error) is implicit across 4 call sites. A brief doc comment here explaining the semantics — "first replay → synthetic error response, second replay → terminal batch drop" — would save future maintainers significant tracing time.
— qwen3.7-max via Qwen Code /review
| await this.messageRewriter?.waitForPendingRewrites(); | ||
| } | ||
|
|
||
| async #buildNextMessageAfterToolRun( |
There was a problem hiding this comment.
[Suggestion] This method returns null to signal "stop the turn," consumed by 4 while (nextMessage !== null) call sites. A brief JSDoc noting the null contract ("returns null to signal the turn should stop; callers must not dereference the return value without a null check") would prevent future callers from crashing.
— qwen3.7-max via Qwen Code /review
| ...(await this.#drainMidTurnUserMessages(ac.signal)), | ||
| ], | ||
| }; | ||
| nextMessage = await this.#buildNextMessageAfterToolRun( |
There was a problem hiding this comment.
[Suggestion] The cron loop (and background notification loop at ~line 2901) exit silently when #buildNextMessageAfterToolRun returns null on repeated duplicate. Unlike the main prompt path, these paths emit no loop-detected event or user-visible signal — a cron job that hits the circuit breaker appears to complete normally in metrics. Consider emitting a conversation_finished event with a loop-detected reason, or logging at info level.
— qwen3.7-max via Qwen Code /review
… stop (QwenLM#9435) The ACP daemon Session was the only duplicate-provider-id circuit breaker path (PR QwenLM#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.
…matching args The duplicate provider tool-call guard (#5038/#5657) keyed on the id alone, so models whose ids are only unique within a single response — e.g. Kimi emits {name}_{index} and the index can restart at 0 on any round — had fresh calls misclassified as replays: the second collision got a synthetic duplicate error and the third tripped the circuit breaker, killing every turn by round three. A handled id now maps to a (name, canonical args) fingerprint — the same sha256 repeat key the loop guards use, moved to a leaf module so toolCallIdUtils can share it without an import cycle. An incoming call is a replay only when its fingerprint matches the call that first executed under that provider id; id collisions with different args execute normally under the unique suffixed id that normalization already assigns. Exact same-args replays keep the unchanged #5014 suppression and #5657 breaker behavior at all four entry points (AgentCore, TUI stream, non-interactive CLI, ACP daemon session). The synthetic duplicate message now tells the model to re-issue with a fresh tool-call id when a new invocation was intended, giving id-emitting models a recovery path.
…n arguments match (QwenLM#9436) * fix(core): treat duplicate provider tool-call ids as replays only on matching args The duplicate provider tool-call guard (QwenLM#5038/QwenLM#5657) keyed on the id alone, so models whose ids are only unique within a single response — e.g. Kimi emits {name}_{index} and the index can restart at 0 on any round — had fresh calls misclassified as replays: the second collision got a synthetic duplicate error and the third tripped the circuit breaker, killing every turn by round three. A handled id now maps to a (name, canonical args) fingerprint — the same sha256 repeat key the loop guards use, moved to a leaf module so toolCallIdUtils can share it without an import cycle. An incoming call is a replay only when its fingerprint matches the call that first executed under that provider id; id collisions with different args execute normally under the unique suffixed id that normalization already assigns. Exact same-args replays keep the unchanged QwenLM#5014 suppression and QwenLM#5657 breaker behavior at all four entry points (AgentCore, TUI stream, non-interactive CLI, ACP daemon session). The synthetic duplicate message now tells the model to re-issue with a fresh tool-call id when a new invocation was intended, giving id-emitting models a recovery path. * qwen: address PR review feedback (QwenLM#9436) - Fingerprint each incoming call once per carrier object: the WeakMap cache now keys on any stable carrier (FunctionCall part or ToolCallRequestInfo), and the replay predicate / recording helpers take the precomputed fingerprint instead of rehashing (name, args) on every breaker scan, admission pass, and record. - Move the getToolCallRepeatKey tests next to the extracted leaf module instead of exercising it through the loop detection service's compatibility re-export. - Restore the assertion pinning that runToolCalls never mutates the history accessor's returned fingerprint map, now that the defensive copy is load-bearing. * qwen: address PR review feedback (QwenLM#9436) Criticals: - Canonicalize onto a null-prototype object so a literal __proto__ own key (preserved by JSON.parse) stays a data property instead of vanishing through the inherited setter — two calls differing only in __proto__ no longer collide on one repeat key, which the replay oracle would have turned into a wrongly suppressed execution. - Clone request args at scheduler intake: callers pass args that can alias the model-emitted functionCall part stored in chat history, and the executor rewrites PATH_ARG_KEYS on request.args in place (a persistence the post-'ask' bounce re-execution relies on). Without the clone those rewrites leak into history and skew the replay fingerprints derived from it, letting genuine replays of path-carrying calls re-execute in multi-round agent runtimes. Suggestions: - Complete the duplicate message with the different-arguments recovery path required by design decision D3, for models whose provider assigns ids. - Fix the copy-convention comments (the accessor returns a fresh map per call; copies are future-proofing) and align all four entry points on copying the accessor result. - Pin the untested branches: history first-occurrence-wins for reused ids and orphan response-id exclusion, the fingerprint cache-hit path, the __proto__ distinction, the caller-args no-mutation invariant, and a cross-round runtime test that a replay of the original call stays suppressed after an id-colliding execution.
5 participants
What this PR does
Stops repeated duplicate provider tool-call responses from keeping Qwen Code in a tool-result loop. The first duplicate provider tool-call id still gets a synthetic duplicate-error function response so the provider's replayed tool call is paired. If the same provider id repeats again within the same prompt, the current batch is treated as terminal/drop-only before any fresh sibling tool call is executed or scheduled.
Applies the same guard to the non-interactive CLI, TUI stream handling, AgentCore, and ACP session tool execution paths. The repeated-duplicate detection is now shared through
findRepeatedDuplicateProviderToolCall, so the four entry points use one consistent branch for both "already sent a synthetic duplicate response" and "same handled provider id appears more than once in one batch".AgentCore now terminates this case with
AgentTerminateMode.LOOP_DETECTEDinstead of a generic error. Interactive agent runs surface that asAgent stopped: duplicate tool-call loop detected., making this loop class visible to users and callers instead of looking like an arbitrary failure.This revision also clears ACP duplicate-response tracking at the start of each new user prompt. ACP sessions are long-lived, but the repeated-duplicate circuit breaker is a per-prompt guard; a provider id seen in prompt N should not poison prompt N+1.
Why it's needed
Fixes a deterministic loop where an OpenAI-compatible provider can replay an already-completed tool-call id, receive another synthetic duplicate tool result, and then replay the same id again. On the current npm release this can lead to repeated tool-result submissions until a session/turn limit stops the run.
The fix keeps the existing duplicate suppression behavior for the first replay, but adds a circuit breaker for repeated duplicate responses so Qwen Code does not send partial or repeated tool responses back to the provider.
The explicit AgentCore termination mode is needed because otherwise the headless/interactive agent path stops correctly but reports the stop as a generic error. Review feedback called out that the user-facing path should identify the duplicate tool-call loop.
The ACP reset is needed because
Sessioninstances survive across prompts. Without clearing the duplicate-response Set per prompt, a legitimate same provider id in a later ACP prompt could be dropped immediately because an earlier prompt had already received a synthetic duplicate response for that id.Reviewer Test Plan
How to verify
Run the focused duplicate-provider-id tests for the affected entry points. The regression cases cover repeated duplicate ids mixed with a fresh sibling tool call, the shared repeated-duplicate helper, AgentCore's visible loop termination mode, the TUI/non-interactive paths, and ACP prompt-to-prompt Set clearing.
Commands run locally after rebasing onto
upstream/main:Evidence (Before & After)
Before: a provider that replayed the same completed provider tool-call id could receive a synthetic duplicate tool result on every round, keeping Qwen Code in a duplicate tool-result loop.
Before, in ACP specifically: once a long-lived
Sessionrecorded that a provider id had already received a synthetic duplicate response, that Set was never cleared between user prompts.Before, in AgentCore: the repeated duplicate guard stopped the run as a generic error, so users and callers could not distinguish this loop class from unrelated failures.
After: the first replay still gets a synthetic duplicate-error function response; the next replay of the same provider id in that prompt terminates/drops the batch before any fresh sibling tool response is sent. ACP clears this per-prompt guard before starting the next user prompt. AgentCore reports
LOOP_DETECTED, and the interactive runner shows a duplicate-loop-specific stop message.Local results:
--max-warnings 0git diff --checkpassednpm run typecheckpassednpm run buildpassedcurlylint warnings plus existing Browserslist and chunk-size warnings; exit code was 0 and none are from touched files.Tested on
Environment (optional)
Local macOS workspace, Node.js
v26.3.0, focused Vitest suites plus repo-level typecheck/build.Risk & Scope
LOOP_DETECTED; existing cancellation, shutdown, max-turn, and generic-error modes are unchanged.Linked Issues
Fixes #5641
AI Assistance Disclosure
I used Codex to review the changes, sanity-check the implementation against existing patterns, and help spot potential edge cases.
中文说明
What this PR does
阻止重复的 provider tool-call 响应让 Qwen Code 陷入 tool-result 循环。第一次重复的 provider tool-call id 仍会收到一个 synthetic duplicate-error function response，用来配对 provider 重放的 tool call。如果同一个 provider id 在同一个 prompt 内再次重复，当前 batch 会在执行或调度任何 fresh sibling tool call 之前被视为终止/仅丢弃。
这个保护覆盖 non-interactive CLI、TUI stream handling、AgentCore 和 ACP session tool execution 路径。repeated-duplicate 检测现在通过
findRepeatedDuplicateProviderToolCall共享，因此四个入口对“已经发过 synthetic duplicate response”和“同批内同一个已处理 provider id 出现多次”使用同一套判断。AgentCore 现在会用
AgentTerminateMode.LOOP_DETECTED终止这种情况，而不是泛化成普通 error。interactive agent run 会展示Agent stopped: duplicate tool-call loop detected.，让用户和调用方能明确看到这是 duplicate tool-call loop，而不是任意失败。这一版也会在每个新的用户 prompt 开始时清理 ACP duplicate-response tracking。ACP session 是长生命周期对象，但 repeated-duplicate circuit breaker 是 per-prompt guard；prompt N 里见过的 provider id 不应该污染 prompt N+1。
Why it's needed
修复一个确定性的循环：OpenAI-compatible provider 可以重放已经完成的 tool-call id，收到另一个 synthetic duplicate tool result，然后再次重放同一个 id。在当前 npm release 上，这可能导致重复提交 tool-result，直到 session/turn 限制中止运行。
这个修复保留第一次 replay 时已有的 duplicate suppression 行为，同时为 repeated duplicate response 增加断路器，避免 Qwen Code 再向 provider 发送 partial 或重复的 tool response。
显式的 AgentCore termination mode 是必要的，因为否则 headless/interactive agent 路径虽然会正确停止，但会把这个停止报告成普通 error。review feedback 已指出 user-facing 路径应该能识别 duplicate tool-call loop。
ACP reset 是必要的，因为
Session实例会跨 prompt 存活。如果不按 prompt 清理 duplicate-response Set，后续 ACP prompt 里合法出现的相同 provider id 可能会因为早先 prompt 已经给该 id 发过 synthetic duplicate response 而被直接丢弃。Reviewer Test Plan
How to verify
运行受影响入口的 focused duplicate-provider-id 测试。回归用例覆盖 repeated duplicate id 和 fresh sibling tool call 混在同一批、共享 repeated-duplicate helper、AgentCore 的 visible loop termination mode、TUI/non-interactive 路径，以及 ACP prompt 间 Set 清理。
在 rebase 到
upstream/main后本地运行的命令：Evidence (Before & After)
修复前：如果 provider 重放同一个已经完成的 provider tool-call id，每一轮都可能收到 synthetic duplicate tool result，让 Qwen Code 留在 duplicate tool-result 循环里。
修复前，在 ACP 路径里还有一个额外问题：长生命周期
Session一旦记录某个 provider id 已经收到 synthetic duplicate response，这个 Set 在用户 prompt 之间不会清理。修复前，在 AgentCore 路径里 repeated duplicate guard 会把运行作为普通 error 停止，因此用户和调用方无法把这个循环和其他失败区分开。
修复后：第一次 replay 仍会收到 synthetic duplicate-error function response；同一个 provider id 在该 prompt 内下一次 replay 会在发送任何 fresh sibling tool response 之前终止/丢弃当前 batch。ACP 会在下一个用户 prompt 开始前清理这个 per-prompt guard。AgentCore 会报告
LOOP_DETECTED，interactive runner 会显示 duplicate-loop-specific stop message。本地结果：
--max-warnings 0通过git diff --check通过npm run typecheck通过npm run build通过curlylint warnings，以及既有 Browserslist 和 chunk-size warnings；退出码为 0，且都不来自本 PR 修改文件Tested on
Environment (optional)
本地 macOS workspace，Node.js
v26.3.0，运行 focused Vitest suites 以及 repo-level typecheck/build。Risk & Scope
LOOP_DETECTED被观察到；现有 cancellation、shutdown、max-turn 和 generic-error modes 不变。Linked Issues
Fixes #5641
AI Assistance Disclosure
I used Codex to review the changes, sanity-check the implementation against existing patterns, and help spot potential edge cases.