fix(core): Repair duplicate tool call IDs#5107
Conversation
Co-authored-by: Qwen-Coder <[email protected]>
|
Thanks for the PR! Template looks good ✓ — all required sections present with bilingual coverage. On direction: this is a solid bug fix. Issue #5099 documents 587 duplicate On approach: the layered defense is clean — normalize at ingestion ( Moving on to code review. 🔍 中文说明感谢贡献! 模板完整 ✓ — 所有章节齐全,中英文对照覆盖。 方向:扎实的 bug 修复。Issue #5099 记录了一个被复用的 id 产生 587 条重复 方案:多层防御结构清晰——入口规范化、各执行点去重、流式解析器防护、出站转换清理。新的 进入代码审查 🔍 — Qwen Code · qwen3.7-max |
|
Qwen Code review did not complete successfully: Qwen review aborted with an API error before posting comments. See workflow logs. |
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. |
Code ReviewThe implementation is clean and well-structured. Defense-in-depth at every layer where duplicate tool call IDs could enter the system: ingestion normalization ( Security claim: false positiveThe "Critical" finding about an empty-string delta bypass in the replay guard is incorrect. The guard in Within-message dedup in converter.tsThe PR now adds symmetric dedup in Streaming parser regression: resolvedThe original What looks good
Test Results
Total: 928 passed, 3 skipped — build and typecheck both clean. CI on GitHub Actions: ✅ all green (Test on macOS, Linux, Windows; Lint; CodeQL; Classify PR). Real-Scenario TestingThis PR fixes a provider protocol issue (duplicate tool call IDs in OpenAI-compatible conversations). Reproducing it requires a mock provider that replays tool call IDs — not a scenario that can be driven through 中文说明代码审查实现结构清晰,在重复 tool call ID 可能进入系统的每个环节都做了防御:入口规范化、执行层去重、流式解析器 replay 防护、出站 payload 清理。新的 安全性声明:误报关于 replay guard 中空字符串 delta 绕过的"Critical"发现不正确。 converter.ts 内消息内去重PR 现在在 流式解析器回归:已修复旧测试已重命名并更新期望值,匹配新的 replay-guard 行为。新增了 metadata-only replay 测试。全部 60 个流式解析器测试通过。 测试结果928 通过,3 跳过。Build 和 typecheck 均干净通过。CI(GitHub Actions)全绿。 真实场景测试本 PR 修复的是 provider 协议层面的问题。@wenshao 已用严格 mock provider 完成了本地运行时验证,确认跨 turn 和同 turn 场景在 PR 下均正确工作,无 PR 时均失败。 — Qwen Code · qwen3.7-max |
ReflectionThis PR solves a real, well-documented bug — #5099 shows 587 duplicate tool results poisoning provider conversations. The layered defense approach is correct and thorough: normalize IDs at ingestion so duplicates never enter history, dedupe defensively at every execution and conversion layer, guard the streaming parser against replay, and clean up the outbound OpenAI payload as a final safety net. 928 tests pass across all affected code paths. Build and typecheck are clean. CI is green on all three platforms. @wenshao's local runtime verification with a strict mock provider confirms the end-to-end claim — the exact The "Critical" security finding about an empty-string delta bypass turned out to be a false positive — the replay guard checks The code is straightforward, the shared LGTM — ready to ship. ✅ 中文说明总结本 PR 解决了一个真实的、有详细记录的 bug(#5099 — 587 条重复 tool result 污染 provider 对话历史)。多层防御方案正确且全面:入口处规范化 ID、各执行和转换层防御性去重、流式解析器 replay 防护、出站 OpenAI payload 最终安全网。 928 个测试全部通过。Build 和 typecheck 干净。CI 三个平台全绿。@wenshao 的本地运行时验证用严格 mock provider 确认了端到端结论——无修复时精确复现 "Critical"安全性发现(空字符串 delta 绕过)经验证为误报——replay guard 检查 代码直观,共享模块 — Qwen Code · qwen3.7-max |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
The streaming parser replay guard in addChunk breaks an existing test (streamingToolCallParser.test.ts:545). The new behavior is correct, but the old test expectations need to be updated — otherwise CI stays red. See my review comments above for details. 🙏
Avoid mutating completed streaming tool calls when a provider replays the same tool call ID with another arguments chunk. This keeps the first surviving call metadata and buffer intact while preserving fragmented JSON accumulation before completion. Co-authored-by: Qwen-Coder <[email protected]>
Use the shared tool-call dedupe helper from ACP Session, preserve distinct empty call IDs in scheduler and non-interactive batches, and log dropped duplicate scheduler requests for diagnosis. Co-authored-by: Qwen-Coder <[email protected]>
|
Qwen Code review did not complete successfully: Qwen review aborted with an API error before posting comments. See workflow logs. |
|
[Suggestion] Test coverage gap: the within-message dedup logic in If this dedup is incorrect (off-by-one, wrong entry preserved), the OpenAI API could receive duplicate tool_call IDs in one assistant message and reject the request. Suggested test: it('should drop duplicate tool_call IDs within a single assistant message', () => {
const request: GenerateContentParameters = {
model: 'models/test',
contents: [
{
role: 'model',
parts: [
{ functionCall: { id: 'dup', name: 'read_file', args: { file_path: 'a.ts' } } },
{ functionCall: { id: 'dup', name: 'read_file', args: { file_path: 'b.ts' } } },
],
},
{
role: 'user',
parts: [
{ functionResponse: { id: 'dup', name: 'read_file', response: { output: 'A' } } },
],
},
],
};
const messages = converter.convertGeminiRequestToOpenAI(request, requestContext);
const assistantMsg = messages.find(m => hasOpenAIToolCalls(m));
expect(assistantMsg.tool_calls).toHaveLength(1);
expect(assistantMsg.tool_calls[0].id).toBe('dup');
});— qwen3.7-max via Qwen Code /review |
|
[Critical] Security vulnerability: the replay guard uses Attack sequence:
Impact: Tool-call redirect. A compromised proxy or malicious provider can redirect permitted tool calls (e.g., Suggested fix: Remove the if (isKnownId && currentBuffer.trim() && currentDepth === 0) {
try {
JSON.parse(currentBuffer);
return { complete: false };
} catch {
// Not complete yet; append the incoming chunk below.
}
}— qwen3.7-max via Qwen Code /review |
|
[Suggestion] Asymmetric dedup in If a single assistant Suggested fix: Mirror the const emittedFunctionCallIds = new Set<string>();
// ...
if (part.functionCall && role === 'assistant') {
const callId = part.functionCall.id;
if (callId) {
if (emittedFunctionCallIds.has(callId)) continue;
emittedFunctionCallIds.add(callId);
}
toolCalls.push({ ... });
toolCallIndex += 1;
}This makes the dedup symmetric at the — qwen3.7-max via Qwen Code /review |
Co-authored-by: Qwen-Coder <[email protected]>
qqqys
left a comment
There was a problem hiding this comment.
Critical issue resolved: the completed-ID replay guard now also drops metadata-only / empty-argument replay chunks before they can overwrite tool metadata, and the added regression test covers that path. I did not find any new critical blocker in this pass.
|
Qwen Code review did not complete successfully: Qwen review aborted with an API error before posting comments. See workflow logs. |
1 similar comment
|
Qwen Code review did not complete successfully: Qwen review aborted with an API error before posting comments. See workflow logs. |
wenshao
left a comment
There was a problem hiding this comment.
No review findings. Downgraded from Approve to Comment: CI failing (review-pr). R3 review at c5f8447 — tsc 0 errors, eslint 0 errors, 925 tests pass (644 core + 281 CLI). The new commit resolves the R2 Critical (empty-string streaming parser replay guard) with both a code fix and regression test. All R2 Suggestions remain valid and non-blocking. — qwen3.7-max via Qwen Code /review
|
Qwen Code review did not complete successfully: Qwen review aborted with an API error before posting comments. See workflow logs. |
✅ Local runtime verification — duplicate tool-call id repair (incl. #5099 repro + A/B)I reproduced the #5099 symptom locally and confirmed this PR fixes it, using a real How I tested
Result — CROSSTURN (the #5099 reproduction): model reuses one
|
| outbound payload (assistant tool-call ids) | provider | CLI | |
|---|---|---|---|
| WITH PR | dup_id_0001, dup_id_0001__qwen_dup_2 (suffixed → unique pairs) |
accepts | DONE_OK, exit 0 |
| without PR | dup_id_0001, dup_id_0001 (duplicate) |
400 duplicate_tool_result_in_request dup_id_0001 |
[API Error: 400 …], exit 1 |
The cross-turn reused raw id is normalized to …__qwen_dup_2 before it enters history, so the next request carries unique, well-paired ids and the strict provider no longer rejects it. Reverting the fix brings back the exact #5099 error.
Result — SAMETURN: model replays the same id twice in one stream
| behavior | |
|---|---|
| WITH PR | duplicate dropped → tool executes once (1 side-effect), one tool result, exit 0 |
| without PR | id collision corrupts the args to {} → tool fails params must have required property 'command' (0 successful executions) |
The PR makes same-turn replays harmless (clean single execution); pre-fix they corrupt the shared argument buffer.
Result — PR's automated coverage (all green)
| Suite | Result |
|---|---|
core: toolCallIdUtils / streamingToolCallParser / converter / geminiChat / coreToolScheduler / agent-headless / speculation |
607 passed, 2 skipped |
core: microcompaction (conservative disarm for old corrupted history) |
37 passed |
cli: nonInteractiveCli / useGeminiStream / ACP Session |
281 passed, 1 skipped |
tsc --noEmit (core + cli) |
clean |
Notes
- I drove the non-interactive path (exercises
geminiChatid-normalization →nonInteractiveCliexecution guard →converterpayload cleanup). The ACP Session and AgentCore dedupe paths are covered by their unit suites above (Session 127, agent-headless 33). - The external Qwen Code sends duplicate tool-result history for a reused tool-call id #5099 / Qwen Code executes duplicate tool calls #5014 repro gists were not rerun (as the PR states); instead I reproduced the symptom synthetically with the strict mock — the produced payload (
duplicate_tool_result_in_request) matches the reported provider error verbatim. npm installregeneratedpackages/vscode-ide-companion/NOTICES.txt(a generated artifact); not a code change.
Verdict
The core claim holds end-to-end: same-turn duplicate ids execute once, cross-turn reused ids are suffixed before history, and the outbound OpenAI payload stays de-duplicated — which is exactly what stops the duplicate_tool_result_in_request failure. No regression in the focused core/cli suites. LGTM from a verification standpoint. 👍
中文说明
✅ 本地运行时验证 —— 重复 tool-call id 修复(含 #5099 复现 + A/B 对照)
我在本地复现了 #5099 的症状,并确认本 PR 修复了它。方法:用真实 cli.js 构建,对接一个严格 mock provider——它会拒绝重复 tool result(返回 400 duplicate_tool_result_in_request <id>,即 #5099 报的那个错)。基于 PR HEAD c5f84474a,在 🐧 Linux、Node v22.22.2 上测试。
测试方法
- 在隔离 worktree 构建 PR:
npm install→ 全工作区tsc构建 + bundle(exit 0);core与cli(本 PR 唯二改动的包)tsc --noEmit均 exit 0。 - 在 tmux 中运行真实非交互 CLI(
node dist/cli.js -p … --approval-mode yolo),对接的 mock provider:(a) 任何含重复 tool result 的 payload 直接 400;(b) dump 每个出站 payload,便于检查线上的 tool-call id。 - 两个场景,各跑一次 PR 版 和一次 反事实版(把 9 个生产文件回退到 merge-base
8472c6fce+ 重新 bundle;toolCallIdUtils.ts作为死代码保留,已确认无残留引用)。
结果 —— CROSSTURN(#5099 复现):模型跨 turn 复用同一个 tool_call.id
| 出站 payload(assistant tool-call id) | provider | CLI | |
|---|---|---|---|
| 有本 PR | dup_id_0001、dup_id_0001__qwen_dup_2(加后缀 → 唯一且配对) |
接受 | DONE_OK,exit 0 |
| 无本 PR | dup_id_0001、dup_id_0001(重复) |
400 duplicate_tool_result_in_request dup_id_0001 |
[API Error: 400 …],exit 1 |
跨 turn 复用的 raw id 在进入 history 之前被规范化为 …__qwen_dup_2,因此下一次请求带的是唯一且正确配对的 id,严格 provider 不再拒绝。回退修复后,#5099 的错误原样重现。
结果 —— SAMETURN:模型在同一个 stream 内重复同一个 id
| 行为 | |
|---|---|
| 有本 PR | 重复被丢弃 → 工具只执行一次(1 次副作用),一个 tool result,exit 0 |
| 无本 PR | id 冲突把参数损坏成 {} → 工具报错 params must have required property 'command'(0 次成功执行) |
本 PR 让同 turn replay 变得无害(干净地只执行一次);修复前它会损坏共享的参数缓冲。
结果 —— PR 自带自动化覆盖(全绿)
| 套件 | 结果 |
|---|---|
core:toolCallIdUtils / streamingToolCallParser / converter / geminiChat / coreToolScheduler / agent-headless / speculation |
607 通过,2 跳过 |
core:microcompaction(旧腐化历史的保守 disarm) |
37 通过 |
cli:nonInteractiveCli / useGeminiStream / ACP Session |
281 通过,1 跳过 |
tsc --noEmit(core + cli) |
干净通过 |
说明
- 我驱动的是非交互路径(覆盖
geminiChatid 规范化 →nonInteractiveCli执行兜底 →converterpayload 清理)。ACP Session 与 AgentCore 的去重路径由上面的单测覆盖(Session 127、agent-headless 33)。 - 未重跑外部 Qwen Code sends duplicate tool-result history for a reused tool-call id #5099 / Qwen Code executes duplicate tool calls #5014 复现 gist(PR 已说明);我用严格 mock 合成复现了症状——产出的 payload(
duplicate_tool_result_in_request)与上报的 provider 错误逐字一致。 npm install重新生成了packages/vscode-ide-companion/NOTICES.txt(生成物),非代码改动。
结论
核心主张端到端成立:同 turn 重复 id 只执行一次,跨 turn 复用 id 在进入 history 前加后缀,出站 OpenAI payload 保持去重——这正是阻止 duplicate_tool_result_in_request 失败的关键。focused core/cli 套件无回归。从验证角度 LGTM。👍
|
Qwen Code review did not complete successfully: Qwen review aborted with an API error before posting comments. See workflow logs. |
|
@qwen-code /triage |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
* fix(core): Repair duplicate tool call IDs Co-authored-by: Qwen-Coder <[email protected]> * fix(core): Preserve streaming tool replay metadata Avoid mutating completed streaming tool calls when a provider replays the same tool call ID with another arguments chunk. This keeps the first surviving call metadata and buffer intact while preserving fragmented JSON accumulation before completion. Co-authored-by: Qwen-Coder <[email protected]> * fix(core): Address duplicate call id review feedback Use the shared tool-call dedupe helper from ACP Session, preserve distinct empty call IDs in scheduler and non-interactive batches, and log dropped duplicate scheduler requests for diagnosis. Co-authored-by: Qwen-Coder <[email protected]> * fix(core): Address duplicate tool call review feedback Co-authored-by: Qwen-Coder <[email protected]> --------- Co-authored-by: Qwen-Coder <[email protected]>
…t tool calls Review follow-up: after empty buffers became a legal completed state, a replayed opener (duplicate ID, QwenLM#5107 lineage) could overwrite a completed no-argument call's name metadata, since the replay guard only engaged on non-empty buffers. Swallowing every known-ID chunk at that state would drop ID-bearing argument fragments for providers whose opener streams empty arguments, so the guard uses the protocol shape as discriminator: a chunk carrying a name but no argument content is an opener replay and is ignored; a chunk with argument content is a continuation and appends. Regression tests cover both directions. Co-Authored-By: Claude Fable 5 <[email protected]>
…ents string (QwenLM#6250) * fix(core): preserve no-argument tool calls that stream an empty arguments string For tools that take no parameters, some OpenAI-compatible providers stream `arguments: ""` (or omit the field entirely) and never send an argument fragment. The streaming parser dropped such calls wholesale (`meta?.name && buffer.trim()`), while the non-streaming path keeps them with `args: {}` — so a turn containing only that call looked empty and geminiChat raised "Model stream ended with empty response text", triggering pointless retries. Align the streaming parser with the non-streaming path: emit the call with empty args when the buffer is empty at stream end. Rewrite the unit test that encoded the drop, and add regression coverage at parser and converter chunk level. * fix(core): use name metadata as slot-occupancy signal for no-argument tool calls Follow-up to review feedback: after empty buffers became a legal end state for no-argument tool calls, three parser methods still used buffer.trim() to decide whether an index slot was occupied. A provider reusing indices could then silently overwrite a completed no-argument call (addChunk collision guard, findNextAvailableIndex) or append stray continuation chunks to it (findMostRecentIncompleteIndex). Switch the occupancy signal in all three places to the name metadata, keeping the JSON-completeness check for non-empty buffers. Add regression tests for both corruption paths and update the stale getCompletedToolCalls JSDoc. Co-Authored-By: Claude Fable 5 <[email protected]> * fix(core): collapse non-object argument parses at emit and lock canonical empty-opener shape Review follow-up on the no-ID continuation routing at addChunk. Mid-stream, an empty buffer with name metadata is formally undecidable between "completed no-argument call" and "canonical opener awaiting its first argument fragment" (every OpenAI-compatible provider opens with arguments:"" and streams fragments ID-less at the same index). Routing must favor the canonical shape, so the guard stays; a new test pins that shape, which the suite previously did not cover. The corruption concern from review is instead bounded at emit time: a buffer polluted by a stray fragment can parse or repair to a non-object value, which now collapses to {} so a polluted no-argument call still emits empty args. Co-Authored-By: Claude Fable 5 <[email protected]> * fix(core): add debug logging for empty-buffer emission and non-object argument collapse Review follow-up: a stray fragment that happens to parse as a valid JSON object is indistinguishable from real arguments at emit time, so log both the non-object collapse and empty-buffer emissions to aid diagnosis. Co-Authored-By: Claude Fable 5 <[email protected]> * fix(core): extend replay guard to opener-shaped replays of no-argument tool calls Review follow-up: after empty buffers became a legal completed state, a replayed opener (duplicate ID, QwenLM#5107 lineage) could overwrite a completed no-argument call's name metadata, since the replay guard only engaged on non-empty buffers. Swallowing every known-ID chunk at that state would drop ID-bearing argument fragments for providers whose opener streams empty arguments, so the guard uses the protocol shape as discriminator: a chunk carrying a name but no argument content is an opener replay and is ignored; a chunk with argument content is a continuation and appends. Regression tests cover both directions. Co-Authored-By: Claude Fable 5 <[email protected]> * test(core): cover null/array argument collapse and multi-slot relocation scan Review follow-up: pin the null and array branches of the emit-time non-object collapse, and exercise findNextAvailableIndex scanning past multiple occupied no-argument slots during collision relocation. Co-Authored-By: Claude Fable 5 <[email protected]> --------- Co-authored-by: FiaShi <[email protected]> Co-authored-by: tomsen-ai <[email protected]> Co-authored-by: Claude Fable 5 <[email protected]>
What this PR does
This PR makes tool-call ids unique and well-paired across OpenAI-compatible conversations. It normalizes model-emitted function call ids before they enter history, drops same-turn replayed ids, suffixes cross-turn reused ids, and adds final OpenAI payload cleanup so only one surviving assistant tool call and one adjacent tool result remain for each id.
It also adds duplicate-id execution guards for the core scheduler, non-interactive CLI, AgentCore, and ACP Session paths, and keeps speculation-generated function calls and responses paired with the same id.
Why it's needed
Some OpenAI-compatible providers can reuse the same
tool_call.idacross turns or replay a completed call in the same stream. Qwen previously stored those raw ids directly in history, which could create duplicate tool results in later requests, inflate payloads across turns, and trigger provider validation errors such asduplicate_tool_result_in_request dup_id_0001.The fix keeps same-turn duplicate ids from causing duplicate side effects, while treating cross-turn reused raw ids as new local calls with unique suffixed ids.
Reviewer Test Plan
How to verify
Run the focused core tests for id normalization, parser/converter cleanup, GeminiChat history ingestion, scheduler execution, AgentCore execution, and speculation pairing. Run the microcompaction tests to confirm old corrupted reused-id history still uses the conservative disarm behavior. Run the focused CLI tests for non-interactive, TUI stream handling, and ACP Session. Finally run the repository build and typecheck.
Commands used locally:
cd packages/core && npx vitest run src/core/toolCallIdUtils.test.ts src/core/openaiContentGenerator/converter.test.ts src/core/geminiChat.test.ts src/core/coreToolScheduler.test.ts src/agents/runtime/agent-headless.test.ts src/followup/speculation.test.ts;cd packages/core && npx vitest run src/services/microcompaction/microcompact.test.ts;cd packages/cli && npx vitest run src/nonInteractiveCli.test.ts src/ui/hooks/useGeminiStream.test.tsx src/acp-integration/session/Session.test.ts;npm run build && npm run typecheck.Expected result: same-turn duplicate tool call ids execute only once, cross-turn reused raw ids are suffixed before entering history, OpenAI payload conversion removes duplicate surviving pairs, and build/typecheck exit successfully.
Evidence (Before & After)
N/A. This is protocol/history repair with focused automated coverage and no TUI rendering change.
Tested on
Environment (optional)
macOS local workspace, Node.js v22.22.3, npm 10.9.8.
Risk & Scope
Linked Issues
Fixes #5099
中文说明
What this PR does
这个 PR 修复 OpenAI-compatible 会话里的 tool-call id 唯一性和配对问题。它会在模型返回的 function call 写入历史前规范化 id,丢弃同一 turn 内 replay 的重复 id,对跨 turn 复用的 raw id 添加 suffix,并在 OpenAI 出站 payload 阶段做最终清理,保证每个存活 id 只保留一个 assistant tool call 和一个相邻 tool result。
它同时为 core scheduler、non-interactive CLI、AgentCore、ACP Session 增加 duplicate-id 执行兜底,并保证 speculation 生成的 function call 和 function response 使用同一个 id 配对。
Why it's needed
部分 OpenAI-compatible provider 会跨 turn 复用同一个
tool_call.id,或者在同一个 stream 内 replay 一个已完成调用。Qwen 之前会直接把这些 raw id 写入 history,导致后续请求中出现重复 tool result、payload 随轮次膨胀,并触发duplicate_tool_result_in_request dup_id_0001这类 provider 校验错误。这次修复会避免同 turn 重复 id 造成重复副作用,同时把跨 turn 复用的 raw id 视为新的本地调用并改写成唯一 suffixed id。
Reviewer Test Plan
How to verify
运行 focused core 测试,覆盖 id 规范化、parser/converter 清理、GeminiChat 历史摄入、scheduler 执行、AgentCore 执行以及 speculation 配对。运行 microcompaction 测试,确认旧腐化 reused-id history 仍保留保守 disarm 行为。运行 focused CLI 测试,覆盖 non-interactive、TUI stream handling 和 ACP Session。最后运行仓库 build 和 typecheck。
本地执行命令:
cd packages/core && npx vitest run src/core/toolCallIdUtils.test.ts src/core/openaiContentGenerator/converter.test.ts src/core/geminiChat.test.ts src/core/coreToolScheduler.test.ts src/agents/runtime/agent-headless.test.ts src/followup/speculation.test.ts;cd packages/core && npx vitest run src/services/microcompaction/microcompact.test.ts;cd packages/cli && npx vitest run src/nonInteractiveCli.test.ts src/ui/hooks/useGeminiStream.test.tsx src/acp-integration/session/Session.test.ts;npm run build && npm run typecheck。预期结果:同 turn 重复 tool call id 只执行一次,跨 turn 复用 raw id 会在进入 history 前添加 suffix,OpenAI payload conversion 会移除重复存活 pair,并且 build/typecheck 成功退出。
Evidence (Before & After)
N/A。这是协议和历史修复,有 focused 自动化覆盖,没有 TUI 渲染变化。
Tested on
Environment (optional)
macOS 本地工作区,Node.js v22.22.3,npm 10.9.8。
Risk & Scope
Linked Issues
Fixes #5099