fix(anthropic): dedup duplicate tool_result blocks sharing a tool_use_id - #8163
Conversation
…ssage Fixes QwenLM#8159 cleanOrphanedToolCalls treated any assistant tool_use with no matching tool_result as an orphan and stripped it -- including a tool_use in the very last message, where there is no subsequent message to have found a result in yet. "No result yet" is not the same as "no result ever": the tool may simply not have finished executing, or the conversion may be happening for a reason other than sending the completed turn to Anthropic (token counting, a resumed/replayed session snapshot, a retry issued before tool execution completes, ...). Silently deleting a currently-active tool_use corrupts the assistant's most recent turn, and the damage compounds when that turn also carries a signed extended-thinking block: the block's signature is computed over the full sibling content of the turn, so removing the tool_use next to it invalidates the signature and replaying the mutated turn produces Anthropic 400 "thinking blocks in the latest assistant message cannot be modified" -- a genuinely confusing error for something the client did to its own outgoing request. Fix: when an assistant tool_use is in the last message of the array (no message follows it at all), treat its tool_use blocks as valid/unresolved rather than scanning for a match that can't exist yet. A tool_use is only condemned as orphaned when a subsequent message was actually scanned and found to lack a matching tool_result. Also fixed one existing test that unintentionally pinned the buggy behavior as expected output ("cleans orphaned tool_use blocks without matching tool_result" used a fixture with no subsequent message at all, which is the trailing case, not a genuine orphan) -- added a real next message with unrelated content so it now exercises an actual orphan. Added a new regression test for the trailing case. Verification: - New/updated unit tests in converter.test.ts (73 tests, was 72). - Full anthropicContentGenerator/ suite: 193 tests pass. - tsc --noEmit -p packages/core/tsconfig.json and eslint clean for touched files.
github-actions
Bot
added
the
review/self-reported
|
Thanks for the PR! Template: headings don't match the repo template exactly ( Problem: observed bug with solid evidence. Linked issue #8160, live API reproduction showing HTTP 400 Direction: clearly aligned. Anthropic's API contract requires exactly one Size: core paths touched ( Approach: minimal and focused — two call sites, one shared factory, four new tests. The PR description notes earlier revisions included a trailing- Risk: no elevated risk signals — no high-risk paths matched. Moving on to code review. 🔍 中文说明感谢贡献！ 模板：标题与仓库模板不完全一致（ 问题：已观测到的 bug，证据充分。关联 issue #8160，有真实 API 复现（HTTP 400 方向：明确对齐。Anthropic API 契约要求每个 规模：触及核心路径（ 方案：最小且聚焦——两个调用点，一个共享工厂函数，四个新测试。PR 描述说明早期版本包含的尾部 风险：无升级风险信号——未匹配高风险路径。 进入代码审查 🔍 — Qwen Code · qwen3.8-max-preview Reviewed at |
Code reviewIndependent proposal: given "Anthropic rejects duplicate Comparison with the diff: the PR does exactly this. The refactoring of the No correctness issues, no security concerns, no convention violations. The doc comment on Tests: four new cases covering single-message dup, dup alongside a distinct id, cross-message dup via merge (asserting full merged content including the surviving text block), and three-message accumulation. Good coverage of the edge cases. No findings. ✅ Testing
Ubuntu unit tests green. macOS/Windows tests and integration tests were skipped (fork PR CI limitation). The PR's central claim — dedup turns a live Anthropic 400 into a 200 — is behavioural and not fully pinned by unit tests alone, but the maintainer (@wenshao) independently confirmed with A/B wire-level testing against two live Anthropic-protocol endpoints, and a sandboxed 中文说明代码审查独立方案： 给定"Anthropic 拒绝同一 与 diff 的对比： PR 完全这样做了。 无正确性问题、无安全隐患、无规范违反。 测试：四个新用例覆盖单消息重复、重复与不同 id 并存、跨消息合并重复（断言包含存活文本块的完整合并内容）、三消息累积。边缘情况覆盖良好。 无发现。✅ 测试Ubuntu 单元测试通过。macOS/Windows 测试和集成测试被跳过（fork PR CI 限制）。PR 的核心主张——去重将 Anthropic 的 400 变为 200——是行为性的，单靠单元测试不能完全验证，但维护者（@wenshao）已独立通过两个真实 Anthropic 协议端点的 A/B wire 级测试确认，且本 PR 上正在运行沙箱化的 — Qwen Code · qwen3.8-max-preview Reviewed at |
|
Confidence: 5/5 — clean across every stage; would merge without hesitation. This is a textbook small fix: real bug, clear reproduction, minimal diff, two-site dedup with a shared factory, four focused tests. The independent proposal I wrote before reading the diff matches the PR's approach almost exactly — I didn't find a simpler path it missed. What makes this PR stand out is the verification depth. The author live-tested against the real Anthropic API (400 → 200), and the maintainer independently confirmed with A/B wire-level testing against two separate Anthropic-protocol endpoints, verified both hunks are independently load-bearing (reverting either alone regresses distinct test cases), and drove an end-to-end session through a recording proxy to confirm real parallel tool calls pass through the dedup untouched. That's not a box-ticking exercise — it's the kind of evidence that makes approval straightforward. The code is easy to maintain: No reservations. Approving. ✅ 中文说明置信度：5/5 —— 每个阶段都干净；毫不犹豫地合并。 这是一个教科书式的小型修复：真实 bug、清晰复现、最小 diff、两处去重共用一个工厂函数、四个聚焦测试。我在阅读 diff 之前写的独立方案与 PR 的方法几乎完全一致——没有找到它遗漏的更简路径。 这个 PR 的突出之处是验证深度。作者对真实 Anthropic API 做了实测（400 → 200），维护者独立确认——对两个独立的 Anthropic 协议端点做了 A/B wire 级测试，验证了两处改动各自都不可省（单独回退任意一处都会导致不同的测试用例回归），并通过记录型代理驱动了完整的端到端会话，确认真实的并行工具调用原封不动地通过了去重逻辑。这不是走过场——这种证据让批准变得顺理成章。 代码易于维护： 无保留意见。批准。✅ — Qwen Code · qwen3.8-max-preview Reviewed at |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
— qwen3.8-max-preview via Qwen Code /review
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 — CI landed green after the review. ✅
Anthropic rejects a message that contains more than one tool_result block for the same tool_use_id with HTTP 400 "each tool_use must have a single result" -- nothing in cleanOrphanedToolCalls (or the merge pass that follows it) deduplicated tool_result blocks, so a tool call result recorded twice in history (a retried conversion pass, or a history source that double-appends a function response) would produce exactly that malformed shape. Two sites needed the same guard: - cleanOrphanedToolCalls: keeps only the first tool_result seen for a given tool_use_id within one message, dropping later duplicates. This is the primary, most common case. - mergeConsecutiveUserMessages: runs after cleanOrphanedToolCalls and can combine two originally-separate user messages that each independently carried a (individually valid) tool_result for the same tool_use_id -- cleanOrphanedToolCalls's per-message dedup can't see across that boundary, so without a second pass here the merge step resurfaces the exact duplicate shape the first fix removes. Live-verified against the real Anthropic Messages API (via our corporate LiteLLM proxy in front of Vertex, model claude-sonnet-4-6): a message with two tool_result blocks sharing one tool_use_id 400s with "each tool_use must have a single result. Found multiple `tool_result` blocks with id: dup_call"; the deduplicated single-tool_result shape returns HTTP 200. Verification: - Three new tests in converter.test.ts: single-message duplicate, duplicate alongside a distinct id (regression guard against over-filtering), and the cross-message duplicate surfaced by mergeConsecutiveUserMessages. - Full anthropicContentGenerator/converter.test.ts suite: 76 tests pass (was 73). - Live proxy verification as described above. - tsc --noEmit -p packages/core/tsconfig.json and eslint clean for touched files.
netbrah
changed the title
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
中文说明
— qwen3.8-max-preview via Qwen Code /review
Maintainer verification report — built and run locally at
|
| Commit | What it does | Live-server result | Recommendation |
|---|---|---|---|
c1acc426bc — dedup duplicate tool_result |
Removes a shape a real server rejects | main → 400, PR → 200 | ✅ Merge |
2ba990bde7 — preserve trailing tool_use |
Emits a shape a real server rejects | main → 200, PR → 400 | ⛔ Hold / rework |
1 · The PR's tests are load-bearing (A/B against merge-base)
Same test file, only converter.ts swapped for the merge-base copy. All 4 new/changed assertions fail without the fix, so the tests genuinely pin the new behavior.
2 · Independent probe of the real converter (not the PR's test file)
A standalone tsx script driving AnthropicContentConverter.convertGeminiRequestToAnthropic directly, 13 cases, run in both worktrees and diffed. Both halves of the fix do exactly what the description claims, and the genuine-orphan / normal-round-trip / id-reuse cases are byte-identical to main (no collateral damage).
3 · The decisive test: the two shapes on the wire
I POSTed the exact bytes each branch emits to two live Anthropic-protocol endpoints.
api.deepseek.com/anthropic (a provider this repo explicitly supports — see the DeepSeek branches in anthropicContentGenerator.ts):
- main's duplicate-
tool_resultshape → HTTP 400messages.2.content.1: each tool_use must have a single result. Found multipletool_resultblocks with id: dup_call— exactly the failure the dedup commit fixes. Independently reproduced; the PR's shape returns 200. ✅ - The PR's preserved trailing
tool_use→ HTTP 400:main's stripped shape returns 200.messages.1:`tool_use` ids were found without `tool_result` blocks immediately after: toolu_live123. Each `tool_use` block must have a corresponding `tool_result` block in the next message.
That second error string is not incidental — it is verbatim the error cleanOrphanedToolCalls was introduced to prevent. It is quoted in this file's own comments (converter.ts:174 and :1016, from #6651 "fix tool_use/tool_result pairing for Anthropic-compatible providers"). The trailing commit re-emits the shape that function exists to eliminate.
open.bigmodel.cn/api/anthropic (GLM) accepted all four shapes — its validator is lenient. So after this commit qwen's output becomes provider-dependent where it was previously universally valid.
4 · Reachability — real binary, recording mock
node scripts/dev.js --yolo -p '…' with ANTHROPIC_BASE_URL pointed at a mock that records every request body and enforces both Anthropic validation rules. Three scenarios: normal tool round-trip, stop_reason=max_tokens on a tool_use turn, and a call to a non-existent tool.
No real request ever ends on an assistant tool_use — the agent loop always appends the tool_result before the next send. Reading the code agrees:
- the max-tokens recovery loop explicitly skips a truncated turn containing a
functionCall(geminiChat.ts~L3046: "Skip recovery when the truncated turn already contains a functionCall"); - the invalid-stream retry calls
popPendingPartialAssistantTurn()before re-sending; fork_turnsseeds history that ends on a model turn and then appends the task prompt as a user message (agent.ts~L1693), so the fork's first request is a genuine-orphan shape, not a trailing one.
So today the trailing change is latent, not an active regression — but it is a landmine: any future path that produces the shape gets a hard 400 where main silently repairs it into a request the model answers by simply re-issuing the tool call.
5 · Two premises in the description that don't hold in this codebase
- "token counting" —
AnthropicContentGenerator.countTokens()usesRequestTokenEstimator, not the converter (anthropicContentGenerator.ts:430). The converter's only consumer isbuildRequest()→ the outgoing HTTP body. There is no non-wire consumer for "unresolved but preserved" to serve: at conversion time the request is either sendable (needs atool_result) or it is not. - "corrupts the assistant's most recent turn" —
cleanOrphanedToolCallsbuilds a fresh array ({...message, content: filtered}); session history is never mutated. The effect is scoped to one outgoing payload.
6 · The signature-invalidation case the issue describes is not the case this PR changes
The blast-radius argument (signed thinking next to a stripped tool_use → thinking blocks in the latest assistant message cannot be modified) needs the turn to actually reach the wire. The shape that does that is a partially-resolved parallel tool call — signed thinking + tool_use A + tool_use B, with only A resolved. That is a genuine orphan (a later message exists), so this PR leaves it untouched:
contents: user → model[thinking(SIGNED), tool_use A, tool_use B] → user[tool_result A]
main: assistant[thinking(sig=SIGNED_ABC), tool_use(A)] ← tool_use B stripped next to a signed block
PR: assistant[thinking(sig=SIGNED_ABC), tool_use(A)] ← IDENTICAL
Verified byte-identical between the two worktrees. This is the real, reachable bug in the issue's narrative and it is still open after this PR. Fixing that — e.g. dropping the whole turn (or the thinking block) when an orphan is stripped from a signed-thinking turn, rather than mutating its siblings — would deliver the issue's stated goal without touching the trailing case.
7 · Regression checks
| Check | Result |
|---|---|
converter.test.ts on the PR |
76/76 pass |
anthropicContentGenerator + openaiContentGenerator suites |
888/888 pass |
Full @qwen-code/qwen-code-core suite |
18,532 pass; 2 fail in session-service-writer-lease.test.ts — reproduce identically on the merge-base, environmental, unrelated |
tsc --noEmit -p packages/core/tsconfig.json |
clean |
eslint + prettier --check on both touched files |
clean |
| CI on the PR | green |
8 · Recommendation
Land c1acc426bc (dedup) as-is. It is a well-found bug — including the sharp second-order catch that mergeConsecutiveUserMessages can re-create the duplicate after cleanOrphanedToolCalls removed it — it is confirmed against a live server, its tests are load-bearing, and dropping a duplicate can never turn a valid request into an invalid one.
Hold 2ba990bde7 (trailing tool_use) until one of these is true:
- a concrete caller is identified that converts a trailing-
tool_usehistory and needs the block preserved (I could not find one, and the converter has no non-wire consumer); or - the change is paired with a story for the resulting 400 — because "no result yet" and "sendable to Anthropic" are different predicates, and the converter's output is judged only by the second.
If the goal is to make the pending-tool_use case visible rather than silently repaired, a debugLogger.warn at that branch achieves it at zero wire risk.
Follow-up (not blocking): cleanOrphanedToolCalls in openaiContentGenerator/converter.ts has the identical trailing semantics (validToolCalls requires an adjacent tool response). If the Anthropic side ever changes here, the two converters silently diverge.
中文版报告
维护者本地验证报告 —— 基于 c1acc426bc 实际构建运行
我在 merge-base（584f6a4bec）之上建立 worktree 构建了两个 commit，并依次做了：单测 A/B、独立的转换器探针、用真实 qwen 二进制跑录制型 Anthropic mock，以及最关键的——向真实的 Anthropic 协议服务端发送实际字节。
结论：这个 PR 的两个 commit 方向相反。
| Commit | 作用 | 真实服务端结果 | 建议 |
|---|---|---|---|
c1acc426bc —— 去重重复 tool_result |
移除了真实服务端会拒绝的形状 | main → 400，PR → 200 | ✅ 合并 |
2ba990bde7 —— 保留尾部 tool_use |
产生了真实服务端会拒绝的形状 | main → 200，PR → 400 | ⛔ 暂缓 / 重做 |
1 · PR 的测试确实有效（对 merge-base 做 A/B）
只把 converter.ts 换回 merge-base 版本、测试文件不变：4 个新增/修改的断言全部失败。说明测试真正锁住了新行为，不是自我印证。
2 · 独立探针（不使用 PR 自带测试文件）
用一个独立 tsx 脚本直接驱动 convertGeminiRequestToAnthropic，覆盖 13 个用例，在两个 worktree 分别运行并 diff。两处修复的行为与描述完全一致；真正的孤立调用、正常一轮工具往返、跨轮次 id 复用等用例与 main 逐字节相同，没有波及其他路径。
3 · 决定性证据：把两种形状发到真实服务端
我把两个分支各自产生的实际字节分别 POST 到两个真实的 Anthropic 协议端点。
api.deepseek.com/anthropic（本仓库明确支持的 provider，anthropicContentGenerator.ts 里有专门的 DeepSeek 分支）：
- main 的“重复
tool_result”形状 → HTTP 400each tool_use must have a single result. Found multipletool_resultblocks with id: dup_call—— 正是去重那个 commit 修掉的问题，我独立复现了；PR 的形状返回 200。✅ - PR 保留下来的尾部
tool_use→ HTTP 400：而 main 剥离后的形状返回 200。messages.1:`tool_use` ids were found without `tool_result` blocks immediately after: toolu_live123. Each `tool_use` block must have a corresponding `tool_result` block in the next message.
第二条错误信息不是巧合：它正是 cleanOrphanedToolCalls 当初被引入所要避免的那个错误，本文件自己的注释里就逐字引用了它（converter.ts:174 与 :1016，来自 #6651 “fix tool_use/tool_result pairing for Anthropic-compatible providers”）。尾部那个 commit 等于把这个函数存在的意义重新放了回去。
open.bigmodel.cn/api/anthropic（GLM）对四种形状全部返回 200，其校验较宽松。也就是说这个 commit 之后，qwen 的输出从“对所有 provider 都合法”变成了“依 provider 而定”。
4 · 可达性 —— 真实二进制 + 录制 mock
node scripts/dev.js --yolo -p '…'，ANTHROPIC_BASE_URL 指向一个会记录每个请求体、并强制执行上述两条 Anthropic 校验规则的 mock。三个场景：正常工具往返、tool_use 回合上 stop_reason=max_tokens、模型调用不存在的工具。
没有任何一次真实请求以助手 tool_use 结尾 —— agent 循环总是先补上 tool_result 再发下一次请求。代码阅读结论一致：
- max_tokens 恢复循环显式跳过含
functionCall的截断回合（geminiChat.ts约 L3046）； - 非法流重试会先调用
popPendingPartialAssistantTurn()再重发； fork_turns播种的历史以 model 回合结尾，随后把 task prompt 作为 user 消息追加（agent.ts约 L1693），所以 fork 的首个请求是“真正的孤立调用”形状，而不是尾部形状。
因此目前尾部这处改动是潜伏的，而非已经生效的回归——但它是一颗地雷：将来任何一条产生该形状的路径都会直接拿到 400，而 main 会静默修复成一个模型只需重新发起工具调用即可继续的合法请求。
5 · 描述中有两个前提在本仓库并不成立
- “token counting” ——
AnthropicContentGenerator.countTokens()使用的是RequestTokenEstimator，并不经过转换器（anthropicContentGenerator.ts:430）。转换器唯一的消费者是buildRequest()→ 出站 HTTP body。不存在“保留未解决 tool_use”能服务的非上线消费者：转换发生时，请求要么可发送（就必须有tool_result），要么不可发送。 - “破坏助手最近一次回合” ——
cleanOrphanedToolCalls构造的是新数组（{...message, content: filtered}），从不修改会话历史。影响仅限于单次出站 payload。
6 · issue 里描述的签名失效场景，恰恰不是本 PR 改动的场景
“签名被破坏”这条论证要成立，该回合必须真的发到线上。真正会发生这种情况的形状是部分完成的并行工具调用——带签名的 thinking + tool_use A + tool_use B，只有 A 有结果。它属于真正的孤立调用（后面确实有消息），因此本 PR 完全没有改到它：
contents: user → model[thinking(SIGNED), tool_use A, tool_use B] → user[tool_result A]
main: assistant[thinking(sig=SIGNED_ABC), tool_use(A)] ← tool_use B 在带签名的块旁边被剥离
PR: assistant[thinking(sig=SIGNED_ABC), tool_use(A)] ← 完全相同
两个 worktree 输出逐字节一致。这才是 issue 叙述中真实且可达的 bug，而它在本 PR 之后依然存在。 若改为修这个（例如：当从带签名 thinking 的回合中剥离孤立调用时，整回合丢弃或连同 thinking 块一起丢弃，而不是只动它的兄弟块），就能达成 issue 的目标，且完全不必碰尾部这个场景。
7 · 回归检查
| 检查项 | 结果 |
|---|---|
PR 上的 converter.test.ts |
76/76 通过 |
anthropicContentGenerator + openaiContentGenerator 两个目录 |
888/888 通过 |
@qwen-code/qwen-code-core 全量套件 |
18,532 通过；2 个失败在 session-service-writer-lease.test.ts，在 merge-base 上同样失败，属环境问题，与本 PR 无关 |
tsc --noEmit -p packages/core/tsconfig.json |
无错误 |
两个改动文件的 eslint + prettier --check |
无问题 |
| PR 的 CI | 全绿 |
8 · 建议
c1acc426bc（去重）建议直接合并。 这是一处发现得很到位的 bug——尤其是那个二阶洞察：mergeConsecutiveUserMessages 会把 cleanOrphanedToolCalls 刚去掉的重复再造出来；它已被真实服务端确认，测试有效，而且“丢弃重复项”永远不可能把一个合法请求变成非法请求。
2ba990bde7（尾部 tool_use）建议暂缓，直到满足以下之一：
- 找到一个确实会转换“尾部
tool_use历史”、并且需要保留该块的具体调用方（我没有找到，且转换器不存在非上线消费者）；或者 - 这处改动配套给出 400 之后的处理方案——因为“还没有结果”和“可以发给 Anthropic”是两个不同的判定，而转换器的输出只受后者检验。
如果目的只是让“存在未解决 tool_use”这件事可见而非被静默修复，在该分支加一条 debugLogger.warn 即可实现，且对线上零风险。
后续（不阻塞）： openaiContentGenerator/converter.ts 里的 cleanOrphanedToolCalls 有完全相同的尾部语义（validToolCalls 要求存在相邻的 tool response）。如果 Anthropic 这一侧改了语义，两个转换器就会悄悄产生分歧。
Verification environment: Linux, Node 22, worktrees pr8163 @ c1acc426bc and base @ 584f6a4bec (converter files are identical between the merge-base and current origin/main, so the A/B is clean). Live-server checks used third-party Anthropic-protocol endpoints (DeepSeek, GLM) with synthetic 30-token payloads; I have no api.anthropic.com key on this machine, so the upstream verdict is inferred from the verbatim error text this repo already cites for that provider class.
|
@qwen-code /resolve |
…d id-sanitization suite
|
Qwen Code resolved the merge conflicts and pushed the branch update. Root causemain's #8164 ( Textual or semantic
What is load-bearing
What I could not verifyNo build/typecheck/tests run (per this command's contract); the sanitization↔dedup interaction was verified by reading, not execution — the PR's CI covers it. Reviewers note: since #8164 already shipped this PR's first commit, the merged tree carries the trailing-tool_use regression test and the "genuine orphan" fixture rewrite exactly once, not twice. 中文说明根因：main 上的 #8164（tool_use.id 清洗）已原样吸收本 PR 第一个提交（#8159 尾部 tool_use 修复），其提交说明明确写有 "merges in the trailing-tool_use fix from #8163"。两侧向 文本还是语义：测试文件为纯相邻冲突，取并集解决——先放 PR 去重测试，再放 main 的 sanitization describe，相对任一父提交零删除。 关键约束：去重以清洗后的线上 未能验证：按约定未运行构建/测试，清洗与去重的交互仅经阅读核验，由 PR 自身 CI 覆盖。另注意：#8164 既已包含本 PR 第一提交内容，合并后尾部 tool_use 回归测试与 fixture 改写各仅一份，不会重复。 |
…M#8164) * fix(anthropic): don't strip a trailing tool_use with no subsequent message Fixes QwenLM#8159 cleanOrphanedToolCalls treated any assistant tool_use with no matching tool_result as an orphan and stripped it -- including a tool_use in the very last message, where there is no subsequent message to have found a result in yet. "No result yet" is not the same as "no result ever": the tool may simply not have finished executing, or the conversion may be happening for a reason other than sending the completed turn to Anthropic (token counting, a resumed/replayed session snapshot, a retry issued before tool execution completes, ...). Silently deleting a currently-active tool_use corrupts the assistant's most recent turn, and the damage compounds when that turn also carries a signed extended-thinking block: the block's signature is computed over the full sibling content of the turn, so removing the tool_use next to it invalidates the signature and replaying the mutated turn produces Anthropic 400 "thinking blocks in the latest assistant message cannot be modified" -- a genuinely confusing error for something the client did to its own outgoing request. Fix: when an assistant tool_use is in the last message of the array (no message follows it at all), treat its tool_use blocks as valid/unresolved rather than scanning for a match that can't exist yet. A tool_use is only condemned as orphaned when a subsequent message was actually scanned and found to lack a matching tool_result. Also fixed one existing test that unintentionally pinned the buggy behavior as expected output ("cleans orphaned tool_use blocks without matching tool_result" used a fixture with no subsequent message at all, which is the trailing case, not a genuine orphan) -- added a real next message with unrelated content so it now exercises an actual orphan. Added a new regression test for the trailing case. Verification: - New/updated unit tests in converter.test.ts (73 tests, was 72). - Full anthropicContentGenerator/ suite: 193 tests pass. - tsc --noEmit -p packages/core/tsconfig.json and eslint clean for touched files. * fix(anthropic): sanitize tool_use.id / tool_result.tool_use_id Fixes QwenLM#8160 The Anthropic converter passed Part.functionCall.id / FunctionResponse.id straight through as tool_use.id / tool_result.tool_use_id with only an empty-string-style fallback, never validating or sanitizing against Anthropic's accepted character set. Anthropic validates both fields against ^[a-zA-Z0-9_-]+$ server-side and rejects anything else -- including the empty string, since `+` requires at least one character. functionCall.id is the Gemini lingua-franca's own id field with no such constraint, so it can legitimately carry another provider's id scheme, a composite/namespaced id, or be entirely absent. Live-verified against the real Anthropic Messages API (via our corporate LiteLLM proxy in front of Vertex, model claude-sonnet-4-6): - A tool_use.id containing characters outside [a-zA-Z0-9_-] (e.g. `call:abc.def/ghi?jkl`) 400s: "String should match pattern '^[a-zA-Z0-9_-]+$'". - An empty-string tool_result.tool_use_id (the exact fallback the old code emitted) 400s the same way. - The sanitized replacement (`call_abc_def_ghi_jkl`) round-trips successfully (HTTP 200) with the pair still linked. Fix: resolveToolUseId() sanitizes non-conforming characters to `_`, generates a real fallback id (never an empty string) when the source is missing, and memoizes the source id -> sanitized id mapping per request (toolIdMap) so a tool_use/tool_result pair sharing a source id still resolves to the same wire id after sanitization. Uniqueness is enforced across the whole request (usedToolIds) rather than the previous per-Content-object counter, which fixes a latent collision: two different Content objects each missing an id previously both started their local counter at 0, producing duplicate fallback ids like `tool_0` in two different assistant turns. State is scoped to a single convertGeminiRequestToAnthropic call (reset via resetToolIdState) since the converter instance is long-lived across requests (constructed once per generator, not once per call). Also merges in the trailing-tool_use fix from QwenLM#8163 (fixes QwenLM#8159): one of the new tests here needs the model's missing-id tool_use to survive as the final message in order to observe the generated fallback id, which depends on that fix. This PR's net diff will shrink to just the sanitization change once QwenLM#8163 merges. Verification: - New tests in converter.test.ts covering: bad-char sanitization with linkage preserved, non-empty fallback generation (not empty string), no id collisions across two missing-id calls in one request, and same-source-id consistency across tool_use/tool_result in different messages. - Full anthropicContentGenerator/ suite: 197 tests pass. - Live proxy verification as described above. - tsc --noEmit -p packages/core/tsconfig.json and eslint clean for touched files. --------- Co-authored-by: Palanisamy, Dinesh <Dinesh.Palanisamy@netapp.com>
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline. Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
中文说明
已审查。 建议见行内评论。 未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
— qwen3.8-max-preview via Qwen Code /review
| if (!validToolResultBlocks.has(b as object)) return false; | ||
| if (seenToolResultIds.has(id)) return false; | ||
| seenToolResultIds.add(id); | ||
| return true; |
There was a problem hiding this comment.
[Suggestion] The new per-block dedup drops a duplicate tool_result silently, whereas the existing whole-message drop in this same function logs via debugLogger.debug (line ~1531). — Concrete cost: an oncall engineer investigating why a specific tool result is absent from the Anthropic request finds no trace of the per-block drop in debug logs and must read the converter source and mentally trace the pipeline to discover the dedup. Adding a debug line at each drop site (here and in the parallel mergeConsecutiveUserMessages dedup) matches the file's existing logging pattern. Note the pre-existing per-block orphan drops are also silent, so this is a broader observability improvement rather than a regression introduced here.
| if (!validToolResultBlocks.has(b as object)) return false; | |
| if (seenToolResultIds.has(id)) return false; | |
| seenToolResultIds.add(id); | |
| return true; | |
| if (!validToolResultBlocks.has(b as object)) return false; | |
| if (seenToolResultIds.has(id)) { | |
| debugLogger.debug( | |
| `cleanOrphanedToolCalls: dropping duplicate tool_result for tool_use_id=${id}`, | |
| ); | |
| return false; | |
| } | |
| seenToolResultIds.add(id); | |
| return true; |
中文说明
[Suggestion] 新增的按块去重逻辑会静默丢弃重复的 tool_result，而同一函数中已有的整条消息丢弃逻辑（约第 1531 行）会通过 debugLogger.debug 打日志。— 具体代价：当值班工程师排查“某个工具结果为何没有出现在发往 Anthropic 的请求中”时，在调试日志里找不到这次按块丢弃的任何痕迹，只能去读转换器源码并在脑中追踪整条 pipeline 才能发现这个去重逻辑。在两处丢弃点（此处以及 mergeConsecutiveUserMessages 中对应的去重）各加一行调试日志，与本文件已有的日志模式保持一致。注意：既有的按块孤立调用丢弃同样是静默的，因此这更像是一项整体可观测性改进，而非本次改动引入的回归。
— qwen3.8-max-preview via Qwen Code /review
ReviewVerdict: the code change is correct and minimal, and I verified both hunks are independently necessary. One thing should be fixed before merge — the title/description no longer match the diff. 1. Title and description describe work that is already on
|
| Claim | Status |
|---|---|
| Title: "don't strip a trailing tool_use and …" | first half is a no-op vs. base |
| Whole Trailing tool_use paragraph under Fix | already on main via #8164 |
| "One existing test unintentionally pinned the buggy trailing-orphan behavior … I gave it a real next message … added a new regression test for the trailing case" | already on main |
"New/updated unit tests in converter.test.ts (72 → 76 tests)" |
head is at 93 tests |
Suggested title: fix(anthropic): dedup duplicate tool_result blocks sharing a tool_use_id.
Root cause is the chore(anthropic): merge main … commit (c03d5ac06b) — a rebase onto main instead of a merge would keep the reviewable diff aligned with the description. The branch is also 16 commits behind main.
2. Verification I ran
A/B harness driving the real AnthropicContentConverter.convertGeminiRequestToAnthropic under tsx, PR head vs. merge-base adfdd33aeb, asserting on the shape Anthropic rejects (>1 tool_result per tool_use_id in one message):
| scenario | base | PR head |
|---|---|---|
| A — dup in one message | [tool_result:dup="first", tool_result:dup="second"] ❌ 400 shape |
[tool_result:dup="first"] ✅ |
| B — dup + distinct id | 2× dup + other ❌ |
dup, other ✅ |
| C — dup across two consecutive user messages (merge site) | 2× dup ❌ |
1× dup ✅ |
| D — 3-way cross-message dup (untested in PR) | 3× dup ❌ |
1× dup ✅ |
| H — dup where 2nd block is the newer result | 2× dup ❌ |
1× dup ✅ (keeps "stale-placeholder", see §4) |
| J — cross-message dup, 2nd message also carries text | 2× dup ❌ |
1× dup + text preserved ✅ |
Both hunks are load-bearing — I re-ran with each reverted individually:
- clean-site hunk only (merge-site dedup reverted) → C and D regress to the 400 shape.
- merge-site hunk only (clean-site dedup reverted) → A, B, H, I regress to the 400 shape.
So all three new tests are genuinely efficacious; none is redundant with the other hunk.
Also:
converter.test.tsat head: 93 passed.tsc --noEmit -p packages/core/tsconfig.json: clean (exit 0).- I checked the mirrored
openaiContentGenerator/converter.tsfor the same bug class (the Anthropic docstring says it mirrors that function). Both the same-message and cross-message duplicate cases already collapse to a singletoolmessage pertool_call_idthere — no follow-up needed on the OpenAI path.
Things the implementation gets right that are easy to get wrong:
- At the clean site,
validToolResultBlocks.has(...)is checked beforeseenToolResultIds.add(id), so an orphaned duplicate doesn't consume the slot and starve a later validtool_resultfor the same id. - The merge-site dedup recomputes over the accumulated
combinedon every iteration, so 3+ consecutive user messages collapse correctly (verified in case D) rather than only pairwise. if (!id) return trueat both sites preserves prior behavior for id-less blocks.
3. The two dedup sites are copy-paste, not shared (minor)
The description says "Both sites now share the same first-wins dedup" — they don't literally share code; they're two independent inline filters that happen to agree today. Since this is exactly the kind of invariant that drifts when only one site gets a later fix, consider a tiny shared factory:
/** First-wins dedup of tool_result blocks by tool_use_id; id-less blocks always pass. */
function makeToolResultDeduper(): (id: string | undefined) => boolean {
const seen = new Set<string>();
return (id) => {
if (!id) return true;
if (seen.has(id)) return false;
seen.add(id);
return true;
};
}Both call sites then read keepToolResult(id), and the clean site keeps its extra validToolResultBlocks check in front.
4. first-wins keeps the older result — worth a one-line rationale (minor)
Verified: given [tool_result dup="stale-placeholder", tool_result dup="FINAL-real-output"], the head keeps stale-placeholder.
For the two causes the description names (a retried conversion pass; a history source that double-appends), the two blocks are byte-identical, so first-wins vs. last-wins is indistinguishable — this is not a bug today. But packages/core has several places that rebuild functionResponse payloads (compactionInputSlimming.ts:360, services/microcompaction/microcompact.ts:669, services/image-payload-references.ts:234); they all replace rather than append, and if one ever appends instead, first-wins would silently ship the pre-rewrite copy. A one-line comment stating why first-wins was chosen (observed duplicates are identical) would make the choice deliberate rather than incidental.
5. Test nits
(messages[2]?.content as Array<…>).filter(...)— the?.guardsmessages[2], but.filterthen runs on the cast result, so an index shift yieldsTypeError: Cannot read properties of undefined (reading 'filter')instead of a readable assertion failure.expect(messages).toHaveLength(3)first, or assert onmessages.at(-1), would fail more legibly.- The cross-message test only asserts the
tool_resultblocks. Since the merge-site fix rebuildslastMessage.contentwholesale, it would be worth asserting the full merged content in at least one test — that non-tool_resultsiblings survive and still sort after the results. - Optional: the 3-way cross-message case (case D above) works but isn't covered; one more fixture would pin that the merge-site dedup is cumulative, not pairwise.
Summary: correctness, style, and test efficacy all look good — the dedup is minimal, correctly ordered, and each hunk is provably necessary. Please retitle and trim the description to the dedup-only scope (ideally rebasing onto main rather than merging), and the rest are optional polish.
netbrah
changed the title
… tests Per wenshao's review on QwenLM#8163: factor the identical first-wins Set-based tool_result dedup logic out of cleanOrphanedToolCalls and mergeConsecutiveUserMessages into a single makeToolResultDeduper() factory with the dedup rationale centralized in one doc comment. Also strengthens the merge-site dedup tests: assert the full merged content array (not just the filtered tool_result blocks) for the two-message case, and add a three-message case pinning that dedup accumulates across the whole combined array on every merge iteration, not just pairwise between the two most recently merged messages.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. 1 Suggestion-level finding(s) could not be anchored to a changed line and were dropped; nothing further to act on here.
— qwen3.8-max-preview via Qwen Code /review
Maintainer verification report — re-verified end-to-end at
|
| # | Check | Result |
|---|---|---|
| 1 | New tests fail without the fix (A/B vs. merge-base) | 4/4 fail on main, 94/94 pass on the PR |
| 2 | Each of the two hunks independently necessary | Yes — disjoint 2 + 2 failure sets |
| 3 | Independent converter probe, 14 shapes | 7 fixed, 7 byte-identical to main |
| 4 | Live Anthropic-protocol servers | DeepSeek 400 → 200; Z.AI GLM 200 either way |
| 5 | Real qwen binary, real model, wire recorded |
Clean round trip, no duplicate ever emitted |
| 6 | tsc --noEmit / eslint / whole suite in the touched dir |
clean / clean / 219 passed |
1 · The new tests are load-bearing
Same test file in both trees; only converter.ts differs. All four new assertions fail against the merge-base, so they genuinely pin the new behavior rather than restating it.
2 · Both hunks are independently load-bearing
Reverting exactly one hunk at a time on the PR head, with everything else untouched. The two failure sets are disjoint — neither call site is redundant with the other, which is the claim the description makes.
3 · Independent probe of the real converter (not the PR's test file)
A standalone tsx script driving AnthropicContentConverter.convertGeminiRequestToAnthropic directly across 14 shapes, run in both worktrees and compared per case. 7 cases fixed, 7 byte-identical to main — including the ones that would have been easy to break: a normal round trip, a genuine orphan tool_use, partially-resolved parallel calls, and the same id legitimately reused in two separate turns.
Two things worth recording from this run:
- Case H (
reusedid in two separate turns) is byte-identical. The deduper is correctly scoped per message / per merge, not global — a globalSethere would have silently eaten the second turn's result. - Case N shows the converter always emits
tool_resultblocks ahead of text blocks inside a user message. That means theseenNonToolResultineligibility path can never strand a duplicate mid-message, so the ordering at the clean site (validity check before the deduper) is defensive rather than reachable — correct either way, just not currently exercisable.
4 · The decisive test: the two shapes on live servers
I POSTed the exact bytes each branch emits to two live Anthropic-protocol endpoints. No mocks, no local validator.
api.deepseek.com/anthropic—main's output for both duplicate shapes is rejected:The PR's output for the same history returns HTTP 200. This is the reported bug, reproduced against a real server and fixed. ✅messages.2.content.1: each tool_use must have a single result. Found multiple `tool_result` blocks with id: dup_callopen.bigmodel.cn/api/anthropic(glm-5.2) — returns 200 for all shapes; its validator is lenient about this rule. Useful as a no-regression control: the PR's deduplicated shape is accepted there too, and the normal round trip is 200 on both branches everywhere.
So the user-visible impact is provider-dependent — which fits #8160 being reported at all rather than being caught by everyone.
5 · Reachability — real binary, real model, recorded wire
The qwen binary built from the PR head, run in an isolated HOME against a recording proxy that forwards verbatim to DeepSeek's Anthropic endpoint and audits every request body for duplicate tool_use_ids.
The session completed normally (correct answer from the file, correct directory listing), and the captured request shows a real parallel tool call — two distinct ids in one user message — passing through the dedup untouched. That is the regression I most wanted to see ruled out on real traffic, not just in fixtures.
6 · Every point from my previous review was addressed
| Previous finding | Status in 2b51b893c |
|---|---|
| Two copy-paste dedup sites that could drift | Extracted into makeToolResultDeduper(), used by both |
first-wins chosen without a stated rationale |
Documented in the new doc comment (observed duplicates are byte-identical) |
(messages[2]?.content as …).filter(...) fails illegibly on an index shift |
expect(messages).toHaveLength(3) added first |
Cross-message test only asserted the tool_result blocks |
Now asserts the full merged content |
| 3-way cross-message case uncovered | New test added; my probe case D confirms it independently |
7 · Regression checks
| Check | Result |
|---|---|
converter.test.ts on the PR head |
94 passed |
Whole anthropicContentGenerator/ suite |
219 passed (3 files) |
tsc --noEmit -p packages/core/tsconfig.json |
clean (exit 0) |
eslint on both changed files |
clean |
| Normal round trip / genuine orphan / parallel partial / id reuse across turns | byte-identical to main |
No blockers. LGTM.
中文版本
维护者验证报告 —— 在 2b51b893c 上重新做了端到端验证
在 refactor 提交之后，针对当前 head 重新跑了一遍完整验证。结论：✅ 可以合并。
环境。 两个全新的 worktree，各自 npm ci，Node v24.18.1：PR head 2b51b893c 对比 merge-base adfdd33ae。main 自 merge-base 以来有 32 个提交，但没有任何一个改动 packages/core/src/core/anthropicContentGenerator/，所以基于 merge-base 的 A/B 对比可以代表合并后的结果，不存在藏在 rebase 里的意外。
| # | 检查项 | 结果 |
|---|---|---|
| 1 | 没有这个修复时新测试是否会失败（对 merge-base 做 A/B） | main 上 4/4 失败，PR 上 94/94 通过 |
| 2 | 两处改动是否各自都必要 | 是 —— 失败用例集合互不相交（2 + 2） |
| 3 | 独立 converter 探针，14 种形状 | 7 种被修复，7 种与 main 逐字节一致 |
| 4 | 真实 Anthropic 协议服务器 | DeepSeek 400 → 200；Z.AI GLM 两边都是 200 |
| 5 | 真实 qwen 二进制 + 真实模型，记录 wire 报文 |
一轮完整工具调用正常，从未发出重复块 |
| 6 | tsc --noEmit / eslint / 改动目录的完整测试 |
干净 / 干净 / 219 通过 |
1 · 新测试确实是"有承重作用"的
两棵树里用的是同一份测试文件，只有 converter.ts 不同。四条新断言在 merge-base 上全部失败，说明它们真的锁定了新行为，而不是在复述既有行为。
2 · 两处改动各自都不可省
在 PR head 上每次只回退其中一处，其余保持不变。两次实验失败的用例集合完全不相交——两个调用点谁也替代不了谁，这正是 PR 描述所主张的。
3 · 对真实 converter 的独立探针（不是 PR 自带的测试文件）
一个独立的 tsx 脚本直接驱动 AnthropicContentConverter.convertGeminiRequestToAnthropic，覆盖 14 种形状，在两个 worktree 中分别运行并逐用例比对。7 种被修复，7 种与 main 逐字节一致——其中包括最容易被误伤的那些：正常的一轮工具往返、真正的孤立 tool_use、只解决了一半的并行工具调用，以及同一个 id 在两个独立轮次中被合法复用。
这次运行里有两点值得记录：
- 用例 H（
reusedid 在两个独立轮次中复用）逐字节一致。去重器的作用域被正确地限制在"单条消息 / 单次合并"内，而不是全局——这里如果用一个全局Set，第二轮的结果会被静默吃掉。 - 用例 N 显示：在一条 user 消息内部，converter 总是把
tool_result块排在 text 块之前。这意味着seenNonToolResult那条"不合格"路径永远不会把一个重复块卡在消息中间，所以 clean 处的顺序（先做合法性检查、再做去重）属于防御性写法而非当前可达路径——两种写法都对，只是目前触发不到。
4 · 决定性的一步：把两种形状打到真实服务器
我把两个分支各自产生的实际字节分别 POST 到了两个真实的 Anthropic 协议端点。没有 mock，也没有本地校验器。
api.deepseek.com/anthropic——main针对两种重复形状产生的输出都被拒绝：而 PR 针对同样历史产生的输出返回 HTTP 200。这就是被报告的那个 bug，在真实服务器上复现并被修复。✅messages.2.content.1: each tool_use must have a single result. Found multiple `tool_result` blocks with id: dup_callopen.bigmodel.cn/api/anthropic（glm-5.2） —— 对所有形状都返回 200，它的校验器对这条规则比较宽松。这正好可以作为无回归的对照组：PR 去重之后的形状在这里同样被接受，而正常的一轮往返在两个分支、两个端点上都是 200。
所以这个问题对用户的可见影响是依赖 provider 的——这也解释了为什么 #8160 会被报上来，而不是所有人都撞到。
5 · 可达性 —— 真实二进制、真实模型、记录下来的 wire 报文
用 PR head 构建出的 qwen 二进制，在一个隔离的 HOME 下运行，指向一个记录型代理；该代理把请求原样转发给 DeepSeek 的 Anthropic 端点，并对每个请求体审计是否存在重复的 tool_use_id。
会话正常完成（从文件中读出了正确的值，也正确列出了目录），并且抓到的请求显示：一次真实的并行工具调用——同一条 user 消息里两个不同的 id——原封不动地通过了去重逻辑。这正是我最想在真实流量（而不仅仅是测试夹具）上排除掉的回归。
6 · 我上一轮 review 提的每一点都已处理
| 上一轮的意见 | 在 2b51b893c 中的状态 |
|---|---|
| 两处去重是复制粘贴，可能各自漂移 | 已抽成 makeToolResultDeduper()，两处共用 |
选择 first-wins 但没有说明理由 |
已在新的文档注释里说明（观察到的重复块是逐字节相同的） |
(messages[2]?.content as …).filter(...) 在下标偏移时报错难读 |
已先加 expect(messages).toHaveLength(3) |
跨消息用例只断言了 tool_result 块 |
现在断言了合并后的完整 content |
| 三条消息的跨消息用例没有覆盖 | 已新增测试；我的探针用例 D 也独立确认了这一点 |
7 · 回归检查
| 检查项 | 结果 |
|---|---|
PR head 上的 converter.test.ts |
94 通过 |
整个 anthropicContentGenerator/ 测试套件 |
219 通过（3 个文件） |
tsc --noEmit -p packages/core/tsconfig.json |
干净（exit 0） |
对两个改动文件跑 eslint |
干净 |
| 正常往返 / 真正的孤立调用 / 并行只解决一半 / 跨轮次 id 复用 | 与 main 逐字节一致 |
没有阻塞项。LGTM。
|
@qwen-code /triage |
|
Sandboxed verification: ✅ passed — merge-ready (agent verdict) - workflow run Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 132 passed · 0 failed · 132 total 中文 — 判定：✅ 通过 · 可合入（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：132 通过 · 0 失败 · 132 总计 Verification report (report.md)Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention). — Qwen Code · sandboxed verification |
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. ✅
…_use is orphaned (QwenLM#8166) * fix(anthropic): don't strip a trailing tool_use with no subsequent message Fixes QwenLM#8159 cleanOrphanedToolCalls treated any assistant tool_use with no matching tool_result as an orphan and stripped it -- including a tool_use in the very last message, where there is no subsequent message to have found a result in yet. "No result yet" is not the same as "no result ever": the tool may simply not have finished executing, or the conversion may be happening for a reason other than sending the completed turn to Anthropic (token counting, a resumed/replayed session snapshot, a retry issued before tool execution completes, ...). Silently deleting a currently-active tool_use corrupts the assistant's most recent turn, and the damage compounds when that turn also carries a signed extended-thinking block: the block's signature is computed over the full sibling content of the turn, so removing the tool_use next to it invalidates the signature and replaying the mutated turn produces Anthropic 400 "thinking blocks in the latest assistant message cannot be modified" -- a genuinely confusing error for something the client did to its own outgoing request. Fix: when an assistant tool_use is in the last message of the array (no message follows it at all), treat its tool_use blocks as valid/unresolved rather than scanning for a match that can't exist yet. A tool_use is only condemned as orphaned when a subsequent message was actually scanned and found to lack a matching tool_result. Also fixed one existing test that unintentionally pinned the buggy behavior as expected output ("cleans orphaned tool_use blocks without matching tool_result" used a fixture with no subsequent message at all, which is the trailing case, not a genuine orphan) -- added a real next message with unrelated content so it now exercises an actual orphan. Added a new regression test for the trailing case. Verification: - New/updated unit tests in converter.test.ts (73 tests, was 72). - Full anthropicContentGenerator/ suite: 193 tests pass. - tsc --noEmit -p packages/core/tsconfig.json and eslint clean for touched files. * fix(anthropic): prune stale thinking signatures after a sibling tool_use is removed Fixes QwenLM#8162 Anthropic validates a thinking/redacted_thinking block's opaque signature against the content it was originally computed over. Removing a sibling tool_use from that same turn -- whether in this request's own cleanup pass or in an earlier compaction cycle now baked into stored history -- can leave a thinking block whose signature no longer matches, producing: "thinking blocks in the latest assistant message cannot be modified" We independently hit this exact error text this session from a related cause (a different orphan-cleanup implementation stripping the *current* turn's tool_use and leaving its thinking sibling stale -- see QwenLM#8159/ QwenLM#8163), which is what prompted auditing this converter for the same class of gap. Two patches, ported from a downstream fork's previously-tested fix (closed a "residual class of 400 ... errors on Vertex-routed claude-opus-4.x sessions with adaptive thinking" per that fix's own commit message): PATCH-A (same-turn cascade, in cleanOrphanedToolCalls): when a tool_use is stripped from an assistant turn by this same cleanup pass, its thinking/redacted_thinking siblings in that turn are now cascade-removed too, since their signature was computed over content that included the now-gone tool_use. PATCH-B (cross-turn, new pruneUntrustworthyThinking pass): catches the case PATCH-A can't -- a non-latest assistant turn whose thinking survived earlier trims but whose tool_use was already gone by the time it entered this request's history. Only ever touches turns that are NOT the most recent assistant turn (Anthropic's contract requires the latest turn's signatures to replay byte-exact regardless). A strictly thinking-only older turn (no surviving tool_use, no other text) has its thinking downgraded to plain text so the model still sees the historical reasoning without an unreplayable signature; redacted_thinking has no plaintext fallback and is dropped instead. A turn that already carries real text alongside the untrustworthy thinking is left as-is (narrower than a blanket rewrite, matching the tested downstream fix). pruneUntrustworthyThinking is skipped when injectThinkingOnToolUseTurns is set (DeepSeek compatibility path): DeepSeek requires a synthetic empty thinking placeholder structurally on every tool-use turn and doesn't validate a signature the way Anthropic does, so this pass' "empty/untrustworthy thinking" concept doesn't apply there and would strip the very placeholder DeepSeek needs. IMPORTANT calibration note on live verification: I fully live-verified PATCH-A's mechanism this session via a related bug (QwenLM#8159/QwenLM#8163) and via code-reading of this exact cascade. For PATCH-B (the cross-turn case), I attempted a live reproduction against the real Anthropic Messages API (via our corporate proxy, Vertex-routed claude-sonnet-4-6, extended thinking enabled): obtained a genuine signed thinking+tool_use turn, then replayed it as a non-latest turn with the tool_use stripped but thinking intact, followed by a new user turn. This did NOT reproduce a 400 -- the API returned 200, with the model noticing the missing tool call itself and self-correcting in its response text rather than the server rejecting the malformed signature context. So the cross-turn mechanism, while structurally sound and matching an already-tested downstream fix's own historical diagnosis, is not independently live-confirmed via this proxy path in my environment. PATCH-B is still included as a defensive, non-regressive improvement (it can only ever remove content that's already been flagged as untrustworthy, never add risk), but I want reviewers to weigh this transparently rather than overclaim a live 400 I couldn't reproduce. Verification: - New tests: same-turn cascade (thinking dropped alongside its orphaned tool_use sibling), cross-turn thinking-only downgrade, cross-turn redacted_thinking-derived drop, latest-turn exemption, and narrower-scope-preserved (real text alongside stale thinking left untouched). - Full anthropicContentGenerator/ suite: 198 tests pass (was 193; net +5 tests). One pre-existing DeepSeek test required the injectThinkingOnToolUseTurns gate described above to keep passing. - tsc --noEmit -p packages/core/tsconfig.json and eslint clean for touched files. - Live proxy verification for PATCH-A's mechanism per above; PATCH-B's cross-turn case did not reproduce a 400 in my environment (see note above and the corresponding comment on QwenLM#8162). * docs(anthropic): document pruneUntrustworthyThinking's false-positive heuristic Per review feedback on PR QwenLM#8166: the function cannot distinguish 'this turn's tool_use was removed by an earlier trim' from 'this turn was always thinking-only' -- both are structurally identical by the time this pass runs (no surviving tool_use, no other text). Make that imprecision explicit in the doc comment rather than implying this only touches genuinely-stale turns, and note the live-verification gap (the specific 400 this guards against did not reproduce against a Vertex-routed proxy). * refactor(anthropic): narrow to the same-turn cascade, drop the cross-turn heuristic Per wenshao's live-verified review on QwenLM#8166: split PATCH-A and PATCH-B, keep PATCH-A plus the one genuine fix inside PATCH-B, drop the rest. Kept and refined (PATCH-A, cleanOrphanedToolCalls's same-turn cascade): scoped the thinking/redacted_thinking cascade to only fire when NO tool_use survives the same turn, not merely "any tool_use removed" (S2). A turn shaped [thinking, tool_use A, tool_use B] where only B is a genuine orphan now keeps both the surviving tool_use A and its thinking sibling -- previously the thinking was stripped even though A still needed it to satisfy Anthropic's manual-mode "final turn must begin with thinking when a tool_use is present" rule, trading one 400 for another. Kept (S6, extracted into its own function, dropEmptyTextThinkingBlocks): an unconditional guard dropping any thinking block with empty text on a non-latest assistant turn. This is unconditionally correct regardless of tool_use presence and isn't redundant with the existing dropUnsignedThinkingFromAssistantMessages pass, which is itself gated to non-native-baseURL + adaptive-thinking + non-DeepSeek configs -- this guard also covers native Anthropic API sessions that pass never touches. Must run AFTER dropUnsignedThinkingFromAssistantMessages, not before: that pass has a deliberate fail-loud design -- a thinking block with a missing/empty signature on a turn inside the still-active tool-use chain throws rather than silently drops, since Claude requires all of an active loop's thinking blocks to be passed back complete and unmodified. An empty-text, unsigned redacted_thinking-derived block is unsigned by that same definition; if dropEmptyTextThinkingBlocks ran first it deleted the block before the fail-loud check ever saw it, silently swallowing exactly the proxy bug that throw exists to surface. This is the identical pass-ordering hazard the removed PATCH-B heuristic was rejected for, reintroduced by this refactor's own extraction -- caught in review and fixed here by reordering, with a regression test. Dropped entirely (the broader pruneUntrustworthyThinking heuristic): "a non-latest, thinking-only turn with no surviving tool_use is structurally untrustworthy, downgrade its thinking to text." Removed because: - Pass ordering: it ran before dropUnsignedThinkingFromAssistantMessages for the same reason described above -- re-typing untrustworthy thinking to plain text first made the drop pass no longer recognize it as thinking at all, so genuinely unsigned reasoning that should have been removed was instead sent to the model as assistant text. - Its DeepSeek exclusion only covered thinking-on mode (!injectThinkingOnToolUseTurns); DeepSeek-with-thinking-off went through a different, deliberately-permissive strip pass that this heuristic wasn't gated against, rewriting a shape that pass intentionally leaves alone. - Live A/B verification against a real session showed a thinking-only turn (that never carried a tool_use, so its signature was never actually invalidated) getting re-typed to text one request later, purely because a newer assistant turn had displaced it as "latest" -- changing the serialized prefix between two consecutive requests (invalidating a cache breakpoint) and costing extra tokens on last-turn-only models that would otherwise strip prior-turn thinking for free. - The state this heuristic exists to clean up -- a non-latest turn whose tool_use went stale in an earlier trim -- did not reproduce against this codebase's actual machinery: chatCompressionService compresses the full history rather than partially trimming it (confirmed the post-compaction attachment path preserves a trailing functionCall turn's thinking sibling intact rather than splitting it), and repairOrphanedToolUseTurns already synthesizes an error tool_result for a genuine cross-turn orphan on every send, upstream of the converter, before this pass would ever see it. truncateHistory and stripThoughtsFromHistory were also checked and don't produce the target state either. Also fixes two bot review Suggestions on the current diff: a duplicated JSDoc paragraph in cleanOrphanedToolCalls's doc comment, and missing test coverage for the branch where a cascade empties a turn out entirely (the message must be dropped and its surrounding user turns merged). --------- Co-authored-by: Palanisamy, Dinesh <Dinesh.Palanisamy@netapp.com> Co-authored-by: qwen-code-dev-bot <qwen-code-dev-bot@users.noreply.github.com>
|
Released in v0.21.3. |
…nt parses Requested after I argued against it, and the argument was thin. I read the corpus — one Anthropic mock in 94 — and treated that as demand. The product says otherwise: `anthropicContentGenerator/` is 11,618 lines and three PRs touched it in the last three months, #8163 and #8166 among them. One of those was settled by posting the two branches' actual bytes to a real Anthropic endpoint and reading back 400 versus 200. "Only one mock exists" does not mean nobody needed one; it means faking it by hand was harder than finding a real endpoint, which is the case FOR shipping it rather than against. The shapes are taken from this repo's own generator, not from memory: the six SSE events it parses, `input_json_delta` for streamed tool arguments, the four usage fields including the cache counters, and the block-array content. A mock whose protocol came from recollection would be testing the recollection. Three differences that a from-memory version gets wrong: `/v1/messages` ends on `message_stop` and sends no `[DONE]` — that is OpenAI's terminator, and a client waiting for it hangs, which is the same failure this command just fixed on the responder side. Tool arguments stream as `partial_json`, not as a finished `input` object; a client that accumulates them and one that expects them whole behave differently. The system prompt is a top-level `system`, not a message — and "the system prompt says verifier" is how 20% of the corpus classified its requests, so a responder branching on prompt text would never see it. The wire is recorded per request as well as passed to the responder: an A/B whose two sides dialled different endpoints is not comparing the same thing, and the log is where that shows. Two of the four guarantees were pinned by assertions that did not pin them — `toContain('input_json_delta')` survives handing the arguments over whole, and testing `anthropicText` as a pure function survives the request path never calling it. Both are parsed and driven now, and all four fail a mutation.
…s yours (QwenLM#8355) * feat(review): mock-provider — the protocol is a fixture, the answer is yours The single most-rewritten artifact in this repo's verification corpus: 94 hand-written mock servers across the maintainer sessions, median 3.3 KB each. What they share is the protocol; what they differ on is the reply. 93% SSE framing 92% a `[DONE]` terminator 73% /v1/chat/completions 69% a usage block 62% a request log Below that the agreement stops, and the split is not "how much mock to ship" but which half. 43% emit `tool_calls` — and across those the SSE SKELETON is unanimous (41 of 41 carry the chunk `index`) while the CONTENT is not: thirteen distinct tool names, four appearing exactly once, the most common at 29%. Request classification is 20%, scenario switching 14%. So this command owns the framing and the caller's responder owns the answer, which is the same line `build-test` and `test-delta` already draw. Two things it takes over that the corpus did by hand: The port. 67 of 94 read one from an env var — a number someone chose and hoped was free, so the second review on a machine collides and its failure looks like the product's. This listens on 0 and reports what the OS gave it. The record. A request log is where an A/B gets its evidence: the same drive against two trees, then a diff of the two request sequences. That only works if both sides write the same shape, so it is JSONL here rather than whatever a given session's mock happened to print. One correction worth recording: an earlier pass over this corpus claimed 68% did error injection and that a `context_window` rule was a common hardening. Both came from loose patterns. Measured properly, injection is about 17% and the context rule appears in **0 of 94** — it was one session's good idea, not a practice. Neither shaped this command. * fix(review): bound the request, and number only what the responder saw Round-1 findings on this PR, both measured before fixing. `/v1/models` is answered without consulting the responder, but it still took a request number: two calls with a models fetch between them handed the responder `[1, 3]`. The field's own doc says "a responder that answers the Nth call needs this", and a responder keyed on its third call would have fired on its second. Nothing bounded a request body. Measured: a 40 MiB POST was read whole into memory, handed to the responder, and written to the JSONL twice — once as the request and once inside the reply record — leaving an 80 MiB log. `drive` had just grown an 8 MiB log cap for this exact hazard, and this was a larger door beside it; `build-test`'s disk floors exist because a review filled its own machine once already. Over the ceiling is 413, not a trim. A truncated body parses to different JSON, so the mock would answer a request the client never sent — a behaviour difference introduced by the harness, which is the one thing it must never introduce. What the RECORD keeps is trimmed instead, and says where: an A/B needs the shape of the request sequence, not every byte of every prompt. Same measurement after: 28 KiB. * docs(review): say which protocol this fakes, and which tool covers the rest The command read as more general than it is. It fakes ONE protocol — the OpenAI-compatible chat API — and nothing in it is qwen-specific, so any project whose product calls such an endpoint can drive against it. But the protocol is also the boundary, and neither the module header nor `--help` said so. Of the 94 mocks measured: 73% spoke this protocol, 23% stood up the project's own HTTP service, and single cases faked MCP/JSON-RPC, OAuth and the Anthropic API. For those, the tool is `drive` — readiness, completion and cleanup for any process a reviewer starts. That is the same division the rest of this line draws, and worth stating where a reader meets it: a protocol 73 files agree on is a fixture, a service that differs across all 22 is a judgement. Said in the header, in `--help`, and in the PR body in both languages. * fix(review): a malformed reply from the responder HUNG the request Round-3 finding, and the worst shape the failure could take. The responder is the caller's module, and a caller's module is exactly the kind of thing that returns `undefined` from a branch nobody took. Measured, one probe per shape: `undefined`, `null`, a bare string, a `status` that is not a number, and tool args holding a circular reference each left the request with **no response at all**. Not an error — a hang. That matters more than a wrong answer would. The product under test sits waiting, `drive` eventually reports `timed-out`, and a bug in the harness has been presented as the behaviour of the diff. This whole line of work exists to stop the harness manufacturing findings, and here it was manufacturing the loudest kind. Two quieter shapes went with it: `{}` and `{foo: 1}` answered 200 with an empty completion, indistinguishable from a model that legitimately said nothing, and `status: 999` was sent on the wire as an HTTP status. Every reply is now checked before it is used, and a bad one is a 500 naming what the responder returned and what it should have. Nine malformed shapes, nine immediate 500s; the two valid shapes are untouched. Pinned by comparing the whole result table, so a regression names which shape broke rather than just failing a count. * feat(review): speak Anthropic too, in the shapes this repo's own client parses Requested after I argued against it, and the argument was thin. I read the corpus — one Anthropic mock in 94 — and treated that as demand. The product says otherwise: `anthropicContentGenerator/` is 11,618 lines and three PRs touched it in the last three months, QwenLM#8163 and QwenLM#8166 among them. One of those was settled by posting the two branches' actual bytes to a real Anthropic endpoint and reading back 400 versus 200. "Only one mock exists" does not mean nobody needed one; it means faking it by hand was harder than finding a real endpoint, which is the case FOR shipping it rather than against. The shapes are taken from this repo's own generator, not from memory: the six SSE events it parses, `input_json_delta` for streamed tool arguments, the four usage fields including the cache counters, and the block-array content. A mock whose protocol came from recollection would be testing the recollection. Three differences that a from-memory version gets wrong: `/v1/messages` ends on `message_stop` and sends no `[DONE]` — that is OpenAI's terminator, and a client waiting for it hangs, which is the same failure this command just fixed on the responder side. Tool arguments stream as `partial_json`, not as a finished `input` object; a client that accumulates them and one that expects them whole behave differently. The system prompt is a top-level `system`, not a message — and "the system prompt says verifier" is how 20% of the corpus classified its requests, so a responder branching on prompt text would never see it. The wire is recorded per request as well as passed to the responder: an A/B whose two sides dialled different endpoints is not comparing the same thing, and the log is where that shows. Two of the four guarantees were pinned by assertions that did not pin them — `toContain('input_json_delta')` survives handing the arguments over whole, and testing `anthropicText` as a pure function survives the request path never calling it. Both are parsed and driven now, and all four fail a mutation. * fix(review): stop answering requests this mock has no business answering Round-4 findings, both measured. A call to `/v1/embeddings`, a GET to the chat endpoint, a non-JSON body and an entirely empty body each came back as a plausible 200 completion. So a product that dialled the wrong endpoint, used the wrong method, or sent a broken payload looked, from the review's side, like it was working. That is the mock concealing the exact defect the review exists to find — worse than any wrong answer it could give. Each is a 400 now, saying which of the three it was, and the refusal goes in the record so a run that dialled wrong can be seen afterwards. The log bound from round 1 only held half. `text` was trimmed while `...mreq` spread the parsed `body` into the same entry carrying the identical payload: a 200 KB system prompt gave an 8 KB `text` and a 205 KB log. The record now summarises the body by its keys rather than copying it — the evidence an A/B needs is the request's shape, not every byte of every prompt. Same measurement after: 9.2 KiB. Four probes in this round, four combinations that were never exercised: Anthropic non-stream tool use, empty text on both wires, and the four malformed requests above. The first three were already correct. * fix(review): a number is spent only when the responder is actually asked Round-5 finding, and the second pass over something round 1 thought it had closed. Round 1 stopped `/v1/models` from taking a request number. Running all four rounds' fixes together on one mixed sequence showed the other two ways it still could — the record read `[1, 2, 2, 3, 4, 5]`: the models call REUSED the previous request's number, having none of its own to report; a refused `/v1/embeddings` incremented past it, because the refusal added in round 4 lands after the counter. A responder keyed on its Nth call reads a sequence like that and fires on the wrong request — which is exactly the failure round 1 set out to fix, surviving in two shapes it had not looked at. The counter now moves only for a request that will reach the responder, and the record carries `null` for everything else rather than a number that would read as "the responder handled this". `Responder` takes a `RespondedRequest` where `n` is non-null, so the invariant is in the type rather than in a comment. Measured after, same sequence: the responder sees `[1, 2, 3]` and the record reads `[1, null, 2, null, null, 3]`. * test(review): pin the invariants across a mixed sequence, not one path at a time Round-6, and a response to what round 5 taught rather than a new defect: every fix so far was right on its own, and the counter was wrong across them. A single-path assertion cannot see that — it exercises one route and stops. So this drives 120 requests over every known path, in a deterministic pseudo-random order, and checks what must hold whatever the order is: one record per request; the responder asked exactly for the requests that reach it, numbered 1..N with no gaps; the record's numbering agreeing with the responder's; no entry carrying the parsed body however large the request; every entry naming a valid wire. It earns its place by catching what the single-path tests did not. Restoring each of three historical bugs — round 5's `!isModels` increment, round 4's missing refusal, round 1's copied body — turns this one test red on its own. Nothing was broken this round: all seven invariants held before it was written. It is here so the next stack of fixes cannot quietly break each other the way round 5's did. * fix(review): four findings from a /review run on this PR All four hold, and each names something six rounds of my own review had not looked at. R1-4 is the one that matters. `replyProblem` guarded the tool branch's `args` against a circular reference and left the status branch's `body` unguarded — so `{status: 500, body: circularObj}` passed validation, and `record()`'s `JSON.stringify` then threw "Converting circular structure to JSON" as an unhandled rejection: the request hung and the drive around it timed out. Confirmed by probe before fixing. Whether a reply can be serialised is a property of the reply, not of the branch it arrived on — the same one-directional reasoning round 5 caught in the counter. R1-1: the `describe` still said "OPENAI-COMPATIBLE endpoint (that protocol only)" after `/v1/messages` was added. A user reading `--help` concludes the command cannot serve an Anthropic-wire product and hand-writes a second mock. Pinned against the ROUTES the implementation serves rather than against a sentence, so a third one added without saying so fails the test instead of misleading someone quietly. R1-2 and R1-3: the `--responder` module loader and the CLI handler had zero coverage — every test passed `respondOverride` and called `startMockProvider` directly, so neither branch had ever executed. The review found the `?? mod.default` fallback survives deletion with every test green; a caller writing `export default function respond` would be told their module exports no `respond` function. Both are driven now, through fixtures in the repo rather than a temp file, because vitest cannot import a module from outside the project root — which is why these paths went untested in the first place. The handler test asserts the TTL in seconds by elapsed time: dropping the `* 1000` turns a 600-second TTL into 0.6, and the mock would exit before the product connects, which the drive would report as a product that never answered.
4 participants
Fixes #8160
What
cleanOrphanedToolCalls(and themergeConsecutiveUserMessagespass that runs after it) had no logic to deduplicatetool_resultblocks sharing the sametool_use_id. Anthropic rejects a message containing more than onetool_resultfor the sametool_use_idwith HTTP 400each tool_use must have a single result, and a duplicate can happen when a tool call's result gets recorded twice in history (a retried conversion pass, or a history source that double-appends a function response).Fix
cleanOrphanedToolCallsnow keeps only the firsttool_resultseen for a giventool_use_idwithin a message and drops later duplicates. A second, less obvious spot needed the same guard:mergeConsecutiveUserMessagesruns aftercleanOrphanedToolCallsand can combine two originally-separate user messages that each independently carried a validtool_resultfor the sametool_use_id— without a second dedup pass at the merge site, the merge would resurface the exact duplicate shape the first fix removes. Both sites now share the same first-wins dedup.Verification
converter.test.ts: single-message duplicate, duplicate alongside a distinct id, and the cross-message duplicate surfaced by the merge step.claude-sonnet-4-6): a message with twotool_resultblocks sharing onetool_use_id400s witheach tool_use must have a single result. Found multiple \tool_result` blocks with id: dup_call; the deduplicated single-tool_result` shape returns HTTP 200.tsc --noEmit -p packages/core/tsconfig.jsonand eslint clean for touched files.Independently confirmed by maintainer review (@wenshao): built both worktrees, ran an A/B against the merge-base, drove an independent converter probe, and POSTed the exact bytes each branch emits to two live Anthropic-protocol endpoints (
api.deepseek.com/anthropic,open.bigmodel.cn/api/anthropic). Confirmed the dedup fix turns a live 400 into a 200 with no collateral changes to the genuine-orphan/normal-round-trip/id-reuse cases, and confirmed both hunks (same-message dedup, cross-message dedup at the merge site) are independently load-bearing — reverting either alone regresses distinct test cases.(Earlier revisions of this PR also included a trailing-
tool_usepreservation fix; that content has since landed onmainvia #8164, so it's no longer part of this PR's diff. This PR is now dedup-only.)Linked Issues
Fixes #8160
中文说明
Fixes #8160
问题
cleanOrphanedToolCalls（以及在其之后运行的mergeConsecutiveUserMessages）没有任何逻辑会对共享同一个tool_use_id的tool_result块去重。当一条消息中出现多个针对同一个tool_use_id的tool_result时，Anthropic 会返回 HTTP 400 错误each tool_use must have a single result；而当某次工具调用的结果在历史记录中被重复记录时（例如一次被重试的转换过程，或者某个历史来源重复追加了同一个函数响应），就可能出现这种重复。修复
cleanOrphanedToolCalls现在只保留同一条消息内针对同一个tool_use_id出现的第一个tool_result，并丢弃之后的重复项。还有一个不那么明显的地方也需要同样的保护：mergeConsecutiveUserMessages会在cleanOrphanedToolCalls之后运行，它可能会把两条原本独立、各自都携带了一个（各自都合法的）同一tool_use_id的tool_result的用户消息合并在一起——如果合并处没有再做一次去重，合并后的消息就会重新出现第一处修复本应消除的重复结构。现在这两处都共用同一套"先到先得"的去重逻辑。验证
converter.test.ts中新增了三个测试：单条消息内的重复、重复项与另一个不同 id 并存、以及由合并步骤暴露出来的跨消息重复。tool_use_id的tool_result块会返回 400 错误each tool_use must have a single result. Found multiple \tool_result` blocks with id: dup_call；去重后只保留一个tool_result` 的请求则返回 HTTP 200。tsc --noEmit -p packages/core/tsconfig.json与 eslint，均无报错。已由维护者审查独立确认（@wenshao）：构建了两个 worktree，对 merge-base 做了 A/B 测试，运行了独立的转换器探针，并把两个分支各自产生的实际字节 POST 到两个真实的 Anthropic 协议端点（
api.deepseek.com/anthropic、open.bigmodel.cn/api/anthropic）。确认去重修复把一个真实的 400 变成了 200，且对真正的孤立调用/正常一轮工具往返/跨轮次 id 复用等场景没有任何副作用；并确认两处改动（同消息内去重、合并处的跨消息去重）都是独立必要的——单独回退任意一处都会导致不同的测试用例回归。（本 PR 早期版本还包含一个尾部
tool_use保留修复；该内容已通过 #8164 合并进main，因此不再是本 PR diff 的一部分。本 PR 现在仅包含去重修复。）关联 Issue
Fixes #8160