fix(anthropic): cascade-strip stale thinking siblings when their tool_use is orphaned - #8166
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.
…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).
github-actions
Bot
added
the
review/self-reported
|
✅ Qwen Triage finished — CI landed green on ✅ Qwen Triage 已完成 —— |
|
Thanks for the PR — and thanks to @wenshao for the deep live-verification that shaped this narrowed version. Template looks good ✓ Problem: observed bug with evidence. The same-turn variant was hit live this session (the Direction: aligned — this is a correctness fix in the Anthropic content converter's orphan-cleanup pipeline, squarely within core mission. The PR was significantly narrowed after maintainer review: the broader cross-turn heuristic ( Size: 151 production lines (converter.ts: +146 −5), 267 test lines (converter.test.ts: +267 −0). Core paths touched ( Approach: the scope feels right. Two changes, both minimal: (1) cascade-strip thinking siblings in Risk: no elevated risk signals — no high-risk paths matched. Moving on to code review. 🔍 中文说明感谢贡献！也感谢 @wenshao 的深度 live 验证塑造了这个精简版本。 模板完整 ✓ 问题：已观测到的 bug，有证据。同 turn 变体在本次 session 中实际触发（ 方向：对齐——这是 Anthropic 内容转换器孤儿清理管道中的正确性修复，完全在核心使命范围内。PR 在维护者审查后大幅收窄：更宽泛的跨 turn 启发式（ 规模：151 行生产代码（converter.ts: +146 −5），267 行测试代码（converter.test.ts: +267 −0）。触及核心路径（ 方案：范围合理。两个改动，都是最小化的：(1) 在 风险：无升级风险信号——未匹配高风险路径。 进入代码审查 🔍 — Qwen Code · qwen3.8-max-preview Reviewed at |
Code reviewIndependent proposal: given the problem (Anthropic 400s when a thinking block's signature no longer matches after its sibling Comparison with the diff: the PR's approach matches this proposal closely. No simpler path missed. The cascade logic in Pipeline ordering is the subtle part, and it's handled well: the empty-text guard runs after Tests: 7 new cases covering the cascade (fires / doesn't fire on partial orphan / empties the turn entirely), the pipeline ordering regression, and the empty-text guard (signed non-empty untouched / empty dropped / latest exempt). The test comments are unusually good — they explain the invariant being pinned, not just what the test does. No critical blockers. No AGENTS.md violations. The comments are extensive but justified — this is a correctness-sensitive area where the ordering constraints and rejected alternatives are genuinely non-obvious. TestingFinal CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行（取最新一次运行），省略 skipped，失败项排在最前。 The main unit suite ( Sandboxed verification would settle the remaining gap: That said, the maintainer (@wenshao) has already done two rounds of live A/B verification against a real Anthropic-protocol endpoint (documented in the PR comments), which substantially de-risks the behavioural claim. 中文说明代码审查独立方案： 给定问题（thinking 块的签名在其兄弟 与 diff 的比较： PR 的方案与此提案高度一致。没有遗漏更简路径。
管道顺序处理得当：空文本保护在 测试：7 个新用例覆盖级联（触发/部分孤儿不触发/完全清空 turn）、管道顺序回归、和空文本保护。测试注释质量很高。 无关键阻塞。无 AGENTS.md 违规。 测试主单元测试（ubuntu）仍在运行中。预检和标签任务通过。无失败——但主要测试证据尚不可用。 沙箱验证可以填补剩余空白： 不过，维护者（@wenshao）已经做了两轮针对真实 Anthropic 协议端点的 live A/B 验证（记录在 PR 评论中），这大幅降低了行为性声明的风险。 — Qwen Code · qwen3.8-max-preview Reviewed at |
|
Confidence: 4/5 — solid, narrow fix with comprehensive tests and two rounds of maintainer live-verification; only non-blocking nit is CI still running on the reviewed commit. This PR went through exactly the right evolution: a broader heuristic was proposed, stress-tested by a maintainer against real infrastructure, found wanting, and stripped down to the two parts that are unconditionally correct. The result is better for it. The cascade-strip in Every change in the diff is necessary for the stated goal. No drive-by refactors, no scope creep. The test-to-production ratio (~1.8:1) is appropriate for a correctness fix in a protocol converter. The comments explain rejected alternatives and ordering constraints that would otherwise be invisible — six months from now, a maintainer touching this pipeline will thank the author rather than curse them. The one reservation keeping this at 4 rather than 5: the primary CI suite hasn't landed green on this commit yet, so the test evidence is the author's local run plus the maintainer's A/B verification rather than the repo's own CI. That's a timing issue, not a code issue. Approval deferred until CI lands green on 中文说明置信度：4/5 ——扎实的窄修复，测试全面，维护者做了两轮 live 验证；唯一的非阻塞小问题是 CI 仍在运行中。 这个 PR 经历了完全正确的演进：提出了一个更宽泛的启发式，由维护者在真实基础设施上压力测试，发现不足，然后精简到两个无条件正确的部分。结果因此更好。
diff 中的每个改动都是实现目标所必需的。无顺手重构，无范围蔓延。测试与生产代码比率（约 1.8:1）对于协议转换器的正确性修复是合适的。 唯一的保留意见是 4 而非 5：主 CI 套件尚未在此 commit 上绿色通过，所以测试证据是作者的本地运行加维护者的 A/B 验证，而非仓库自身的 CI。这是时序问题，不是代码问题。 审批推迟到 CI 在 — Qwen Code · qwen3.8-max-preview Reviewed at |
… 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).
| if (!options.injectThinkingOnToolUseTurns) { | ||
| messages = pruneUntrustworthyThinking(messages); | ||
| } |
There was a problem hiding this comment.
[Suggestion] pruneUntrustworthyThinking also runs on the DeepSeek thinking-OFF path (stripAssistantThinking), where it defeats stripThinkingFromAssistantMessages's deliberate thinking-only-turn passthrough. — Concrete cost: DeepSeek with thinking disabled sets stripAssistantThinking=true and injectThinkingOnToolUseTurns=false (anthropicContentGenerator.ts:669-670,702-705), so this guard passes and prune runs. For a non-latest thinking-only turn, prune re-types the thinking block to text; the second stripThinkingFromAssistantMessages pass then cannot remove it (it only filters thinking/redacted_thinking), and its deliberate filtered.length === 0 passthrough — which preserves the thinking type the strip author documented DeepSeek tolerates — never fires. Net: old reasoning is shipped as a visible assistant text utterance instead of a tolerated thinking block, a wire-shape change this diff introduces and the guard comment does not address. Verified with a probe: current code emits text, the fix below emits thinking.
| if (!options.injectThinkingOnToolUseTurns) { | |
| messages = pruneUntrustworthyThinking(messages); | |
| } | |
| if (!options.injectThinkingOnToolUseTurns && !options.stripAssistantThinking) { | |
| messages = pruneUntrustworthyThinking(messages); | |
| } |
中文说明
pruneUntrustworthyThinking 在 DeepSeek 关闭 thinking 的路径（stripAssistantThinking）上同样会运行，从而破坏了 stripThinkingFromAssistantMessages 刻意保留的"纯 thinking turn 透传"行为。具体代价：DeepSeek 关闭 thinking 时会设置 stripAssistantThinking=true 且 injectThinkingOnToolUseTurns=false（anthropicContentGenerator.ts:669-670,702-705），因此该守卫通过、prune 运行。对于非最新的纯 thinking turn，prune 会把 thinking 块重新标记为 text；随后第二次 stripThinkingFromAssistantMessages 无法移除它（它只过滤 thinking/redacted_thinking），而其刻意的 filtered.length === 0 透传——即保留 strip 作者记录的、DeepSeek 可容忍的 thinking 类型——永远不会触发。结果：旧的推理内容会作为一个可见的 assistant text 语句被发出，而不是一个被容忍的 thinking 块；这是本 diff 引入的 wire 形态变化，且守卫注释并未涉及。已用 probe 验证：当前代码输出 text，上方修复输出 thinking。
— qwen3.8-max-preview via Qwen Code /review
| if (i === messages.length - 1) { | ||
| for (const block of toolUseBlocks.values()) { | ||
| validToolUseBlocks.add(block as object); |
There was a problem hiding this comment.
[Suggestion] This new trailing-tool_use guard makes the Anthropic cleanOrphanedToolCalls intentionally diverge from the OpenAI one, but the function's doc still says "Mirrors the same-name function in the OpenAI converter." — Concrete cost: the OpenAI converter (openaiContentGenerator/converter.ts:1888) still strips a trailing tool call with no adjacent tool response. A future change that "re-syncs" the two implementations — or a reviewer reasoning from the "Mirrors" comment instead of this #8159 guard — could port the OpenAI trailing-strip back in and silently reintroduce the exact latest-turn-corruption / stale-thinking-signature 400 this PR fixes.
Suggested fix (the "Mirrors" sentence is pre-existing, so not a one-click suggestion on this hunk): amend that doc sentence to note the deliberate divergence, e.g. "Mirrors the same-name function in the OpenAI converter, except that a tool_use in the very last message is preserved as unresolved rather than stripped (see #8159); the OpenAI version still strips it."
中文说明
这个新增的尾部 tool_use 保护使 Anthropic 的 cleanOrphanedToolCalls 有意地与 OpenAI 版本产生分歧，但该函数的文档仍写着 "Mirrors the same-name function in the OpenAI converter"。具体代价：OpenAI 转换器（openaiContentGenerator/converter.ts:1888）仍会剥离没有相邻 tool response 的尾部 tool call。未来某个"重新同步"两份实现的改动——或者一个依据 "Mirrors" 注释而非此 #8159 守卫进行推理的 reviewer——可能会把 OpenAI 的尾部剥离逻辑移植回来，从而静默地重新引入本 PR 所修复的那个"最新 turn 被破坏 / thinking 签名过期"的 400。
建议修复（"Mirrors" 那句是既有代码，故不在此 hunk 上提供一键 suggestion）：修改该文档句以注明这一刻意分歧，例如："Mirrors the same-name function in the OpenAI converter, except that a tool_use in the very last message is preserved as unresolved rather than stripped (see #8159); the OpenAI version still strips it."
— qwen3.8-max-preview via Qwen Code /review
| const finalBlocks = | ||
| toolUseRemoved && message.role === 'assistant' | ||
| ? filtered.filter((b) => { | ||
| const t = (b as { type?: string }).type; | ||
| return t !== 'thinking' && t !== 'redacted_thinking'; | ||
| }) | ||
| : filtered; |
There was a problem hiding this comment.
[Critical] The same-turn cascade strips DeepSeek's synthetic thinking placeholder on the injectThinkingOnToolUseTurns path, producing a DeepSeek 400. — Failure scenario: with DeepSeek thinking-ON (injectThinkingOnToolUseTurns: true), injectEmptyThinkingOnToolUseTurns (line ~164) prepends a synthetic {type:'thinking', thinking:'', signature:''} placeholder before cleanOrphanedToolCalls (line ~177). When a non-latest assistant turn carries multiple tool_use blocks and only some are orphaned (partial tool completion, compaction, or timeout), toolUseRemoved is true and this cascade strips all thinking from the turn — including the injected placeholder — leaving a surviving tool_use turn with no thinking block. DeepSeek then rejects the request: "The content[].thinking in the thinking mode must be passed back to the API" (see issue 3786). Before this PR there was no cascade, so the placeholder survived; no later pass re-injects it. The pruneUntrustworthyThinking guard at line ~186 skips exactly this path for exactly this reason, but the cascade does not respect the same constraint. Verified with a runnable probe: on current code the surviving tool_use turn has 0 thinking blocks; protecting the placeholder restores it.
Suggested fix (spans two locations, so not a one-click suggestion): either thread injectThinkingOnToolUseTurns into cleanOrphanedToolCalls and skip the thinking-strip when it is true (the synthetic placeholder's empty signature was never computed over the tool_use content, so the removal does not invalidate it), or re-run the injection after the cleanup:
messages = cleanOrphanedToolCalls(messages);
messages = injectEmptyThinkingOnToolUseTurns(messages); // restore placeholders the cascade stripped中文说明
同一 turn 内的级联清理会在 injectThinkingOnToolUseTurns 路径上剥掉 DeepSeek 的合成 thinking 占位块，从而导致 DeepSeek 返回 400。失败场景：DeepSeek 开启 thinking（injectThinkingOnToolUseTurns: true）时，injectEmptyThinkingOnToolUseTurns（约第 164 行）会在 cleanOrphanedToolCalls（约第 177 行）之前预先插入一个合成的 {type:'thinking', thinking:'', signature:''} 占位块。当某个非最新 assistant turn 含有多个 tool_use 块、其中只有部分被判定为孤儿（部分工具完成、压缩或超时）时，toolUseRemoved 为 true，此级联会剥掉该 turn 中所有 thinking——包括刚插入的占位块——使得幸存的 tool_use turn 没有 thinking 块。DeepSeek 随即拒绝请求："The content[].thinking in the thinking mode must be passed back to the API"（见 issue 3786）。本 PR 之前没有级联，占位块得以存活；后续也没有任何 pass 重新插入它。约第 186 行的 pruneUntrustworthyThinking 守卫正是出于这个原因显式跳过该路径，但级联没有遵守同样的约束。已用可运行 probe 验证：当前代码下幸存的 tool_use turn 上 thinking 块数为 0；保护占位块后得以恢复。
建议修复（涉及两处，故不提供一键 suggestion）：要么把 injectThinkingOnToolUseTurns 传入 cleanOrphanedToolCalls，在其为 true 时跳过 thinking 剥离（合成占位块的空签名从未基于 tool_use 内容计算，因此移除 tool_use 不会使其失效）；要么在清理之后重新执行注入。
— qwen3.8-max-preview via Qwen Code /review
| if (bType === 'thinking' && !hasSurvivingToolUse && !hasText) { | ||
| filtered.push({ | ||
| type: 'text', |
There was a problem hiding this comment.
[Suggestion] The !hasSurvivingToolUse guard here is never the deciding factor in any test — a mutation that removes it survives the whole suite. — Concrete cost: no test creates a non-latest assistant turn with both a surviving tool_use and non-empty thinking (with pruning active). Delete !hasSurvivingToolUse && and every test still passes: the "real accompanying text" test is saved by !hasText, and the DeepSeek multi-turn tests skip pruning via the injectThinkingOnToolUseTurns guard. The surviving mutant would incorrectly downgrade thinking on a [thinking, tool_use] turn whose tool_use is still present (signature still valid) — a false-positive rewrite of valid historical content that CI would not catch.
Suggested fix (a new test, so not a one-click suggestion here): add a case in the pruneUntrustworthyThinking describe block — a non-latest assistant turn with non-empty thinking and a surviving tool_use (followed by a matching tool_result user message and a final assistant turn) — asserting the thinking block is preserved with its original type and signature.
中文说明
此处的 !hasSurvivingToolUse 守卫在任何测试中都不是决定性因素——一个移除该守卫的变异能在整套测试中存活。具体代价：没有任何测试构造一个同时含有幸存 tool_use 和非空 thinking 的非最新 assistant turn（且 prune 生效）。删掉 !hasSurvivingToolUse && 后所有测试仍通过："含真实文本"测试由 !hasText 救下，DeepSeek 多 turn 测试则通过 injectThinkingOnToolUseTurns 守卫跳过 prune。存活的变异会错误地降级一个 [thinking, tool_use] turn 上的 thinking（该 turn 的 tool_use 仍在、签名仍有效）——这是对有效历史内容的假阳性重写，而 CI 无法捕获。
建议修复（新增测试，故此处不提供一键 suggestion）：在 pruneUntrustworthyThinking 的 describe 块中新增一个用例——一个含有非空 thinking 且 含有幸存 tool_use 的非最新 assistant turn（其后跟一个匹配的 tool_result user 消息和一个最终 assistant turn）——断言 thinking 块以原始类型和签名被保留。
— qwen3.8-max-preview via Qwen Code /review
| if (bType === 'redacted_thinking' && !hasSurvivingToolUse && !hasText) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
[Suggestion] This redacted_thinking drop branch has no real test coverage — deleting it leaves all 78 tests green (verified with a probe). — Concrete cost: the test named "drops an empty redacted_thinking-derived turn entirely" actually feeds {text:'', thought:true}, which processContent turns into an empty-text thinking block caught by the empty-text guard above (lines ~1311-1317) — it never reaches this branch (the test's own comment concedes the branch is "not independently reachable through this converter's own request-building path"). So the test title overstates coverage, and a future regression that stops dropping a literal redacted_thinking block (replaying a stale signature → 400) would pass CI. Lower priority since the branch is documented as defensive/unreachable today, but the gap and the misleading title are real.
Suggested fix (a new test): place a literal {type:'redacted_thinking', data:'opaque'} block on a non-latest assistant turn with no surviving tool_use and no text, and assert the block (and the turn, if it becomes empty) is dropped — exporting pruneUntrustworthyThinking for a direct unit test if needed.
中文说明
这个 redacted_thinking 丢弃分支没有真正的测试覆盖——删除它后全部 78 个测试仍为绿色（已用 probe 验证）。具体代价：名为 "drops an empty redacted_thinking-derived turn entirely" 的测试实际传入 {text:'', thought:true}，processContent 会把它变成一个空文本的 thinking 块，被上方的空文本守卫（约 1311-1317 行）捕获——它永远不会到达此分支（该测试自己的注释也承认此分支 "not independently reachable through this converter's own request-building path"）。因此测试标题夸大了覆盖范围，而未来某个停止丢弃字面 redacted_thinking 块的回归（重放过期签名 → 400）将能通过 CI。由于该分支今天被记录为防御性/不可达，优先级较低，但覆盖缺口和误导性标题是真实存在的。
建议修复（新增测试）：在一个没有幸存 tool_use、也没有文本的非最新 assistant turn 上放置一个字面的 {type:'redacted_thinking', data:'opaque'} 块，断言该块（以及若变空则该 turn）被丢弃——必要时导出 pruneUntrustworthyThinking 以做直接单元测试。
— qwen3.8-max-preview via Qwen Code /review
| if (filtered.length === 0) continue; | ||
| out.push({ role: msg.role, content: filtered }); |
There was a problem hiding this comment.
[Suggestion] pruneUntrustworthyThinking silently rewrites and drops historical content with zero telemetry, unlike its sibling cleanOrphanedToolCalls (which logs at line ~1226) and the OpenAI converter's three debug lines. — Concrete cost: by this function's own doc it is a "conservative, false-positive-prone heuristic" that "will also downgrade a turn whose thinking signature was never actually invalidated … a real change to valid historical content." When a user reports "the model lost the thread of its earlier reasoning," nothing distinguishes "this pass rewrote/dropped the turn" from "the model answered differently" — the oncall must instrument the code and reproduce the exact history shape to confirm this pass fired.
Add a debug log mirroring the sibling, at both the downgrade branch above and the whole-message drop below:
| if (filtered.length === 0) continue; | |
| out.push({ role: msg.role, content: filtered }); | |
| if (filtered.length === 0) { | |
| debugLogger.debug( | |
| 'pruneUntrustworthyThinking: dropping message with only untrustworthy thinking blocks', | |
| ); | |
| continue; | |
| } | |
| out.push({ role: msg.role, content: filtered }); |
中文说明
pruneUntrustworthyThinking 在零遥测的情况下静默重写并丢弃历史内容，而其兄弟函数 cleanOrphanedToolCalls（约第 1226 行有日志）以及 OpenAI 转换器的三处 debug 日志都有遥测。具体代价：按本函数自身文档所述，它是一个"保守、易假阳性的启发式"，"也会降级一个 thinking 签名从未真正失效的 turn……这是对有效历史内容的真实修改"。当用户反馈"模型丢失了早先推理的脉络"时，没有任何东西能区分"是这个 pass 重写/丢弃了该 turn"还是"模型只是回答得不一样"——oncall 必须给代码加埋点并精确复现该历史形态，才能确认这个 pass 是否触发。
参照兄弟函数添加 debug 日志，分别在上方降级分支和下方整条消息丢弃处。
— qwen3.8-max-preview via Qwen Code /review
Local verification report (maintainer)I built this branch locally and ran it against a real client stack rather than reading the diff. Short version: PATCH-A is sound and I'd take it with one added guard; PATCH-B I'd split out and hold — your own calibration note turned out to be right, and the local runs also surfaced one concrete regression in it. Thanks for flagging the reproduction gap up front; that is what made this worth measuring instead of guessing. What I ran
Findings✅ PATCH-A does exactly what it claims (S1). Orphaned ✅ One genuine fix is hiding inside PATCH-B (S6). On the base build a redacted-derived turn ships 🔴 Pass ordering: 🟠 The DeepSeek exclusion is only half-applied (S11). The gate is 🟠 PATCH-A has an unguarded partial-orphan case (S2). Turn = 🔴 PATCH-B's false positive is real, and I caught it on the wire in a live session, not in a fixture.
Turn 1 was a thinking-only model reply that never carried a 🟠 The stated trigger for PATCH-B doesn't seem to exist in this codebase. Three things I checked by running them, not by reading:
Docs corroboration for the calibration note: validation is scoped to "Within the latest assistant message, the sequence of consecutive What I'd like to see before merge
None of this is a knock on the work — the code is careful, the tests are real regression tests, and the honesty about what you couldn't reproduce is exactly why this review could be short. It just lands on: half of it is ready, half of it is guarding a state this codebase doesn't produce. Test rig (screenshot)The live runs drive the real TUI against a local Anthropic-protocol endpoint, so history recording, the converter and the SDK are all exercised; the server logs the request body verbatim for the A/B.
中文说明本地验证报告（maintainer）我在本地把这个分支构建出来，用真实的客户端链路跑了一遍，而不是只读 diff。结论先说：PATCH-A 是站得住的，补一个 guard 之后我可以接受；PATCH-B 我建议拆出去先搁置 —— 你自己写的那段校准说明结果是对的，而且本地跑下来还额外发现了它引入的一处真实回归。感谢你把复现的缺口提前标出来，正因为如此这次才值得实测而不是靠推断。 我跑了什么
结论✅ PATCH-A 完全符合它的描述（S1）。 孤儿 ✅ PATCH-B 里藏着一个真实的修复（S6）。 base 构建上，redacted 衍生的那个 turn 会把 🔴 Pass 顺序问题： 🟠 DeepSeek 的豁免只做了一半（S11）。 现在的 gate 是 🟠 PATCH-A 有一个没有防护的"部分孤儿"场景（S2）。 turn = 🔴 PATCH-B 的假阳性是真实存在的，而且我是在一次 live 会话的 wire 上抓到的，不是在 fixture 里。
turn 1 是一次纯 thinking 的模型回复，它从来没有携带过 🟠 PATCH-B 所描述的触发条件，在这个代码库里似乎并不存在。 有三件事我是跑出来的，不是读出来的：
文档层面对那段校准说明的印证：校验的范围是 "Within the latest assistant message, the sequence of consecutive 合并前我希望看到的
以上都不是对这份工作的否定 —— 代码写得很细致，测试是真正的回归测试，而你对"哪些没能复现"的坦诚正是这次 review 能这么快收敛的原因。只是结论落在：一半已经可以合了，另一半守的是这个代码库并不会产生的状态。 测试装置（截图）live 验证是用真实 TUI 打到本地的 Anthropic 协议端点上，因此历史记录、转换器和 SDK 都被真实执行；服务端逐字记录请求体用于 A/B 对比。
Verified locally with Claude Code (model: Claude Opus 5, 1M context). |
|
@qwen-code /resolve |
Resolve the converter.test.ts conflict by keeping this PR's cascade-strip test alongside the trailing-tool_use test that main landed independently.
|
Qwen Code resolved the merge conflicts and pushed the branch update. Merge resolution — PR #8166Root cause. This PR's first commit ( Textual, not semantic. Only expect(assistantMsg!.content).toEqual([
{ type: 'text', text: 'Let me help' },
]);
});
Load-bearing. The Could not verify. No build/typecheck/tests run. Two notes:
中文说明根因。 本 PR 首个提交（ 仅文本冲突。 只有 关键点。 未能验证。 未运行构建/类型检查/测试。两点：
|
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
[Critical] Re-checked against the current HEAD (abe0c01) — STILL STANDS. The same-turn cascade added to cleanOrphanedToolCalls strips ALL thinking/redacted_thinking from an assistant turn whenever ANY tool_use is removed, with no injectThinkingOnToolUseTurns guard. Because injectEmptyThinkingOnToolUseTurns (converter.ts:264) runs BEFORE cleanOrphanedToolCalls, on the DeepSeek thinking-ON path the injected synthetic {type:'thinking',thinking:'',signature:''} placeholder is stripped when a sibling tool_use is partially orphaned, leaving a surviving tool_use turn with no thinking block → DeepSeek 400 ("The content[].thinking in the thinking mode must be passed back to the API", issue 3786). The 5 commits since the original comment (3686768345) gated only the new pruneUntrustworthyThinking call, not the cascade. The maintainer's wire-verified "S2" is the same unguarded cascade viewed from the Anthropic manual-mode angle (a final assistant turn of a thinking-enabled request must begin with a thinking block). Suggested fix: cascade only when NO tool_use survives in that turn, or thread injectThinkingOnToolUseTurns into cleanOrphanedToolCalls and skip the thinking-strip when it is true (the synthetic placeholder's empty signature was never computed over the tool_use content, so removing the tool_use does not invalidate it).
— qwen3.8-max-preview via Qwen Code /review
| * modified". The model regenerates thinking on its next turn regardless. | ||
| * | ||
| * A `tool_use` in the very last message (no message follows it at all) is |
There was a problem hiding this comment.
[Suggestion] The JSDoc paragraph that starts here ("A tool_use in the very last message…") is a verbatim duplicate of the paragraph immediately below it. This diff inserted a new copy directly above the pre-existing one, so cleanOrphanedToolCalls's doc now contains the same eight-line paragraph twice in a row (lines ~1420–1427 and ~1429–1436). — Concrete cost: a future edit to the last-message exemption (e.g. refining the "no result yet" semantics) can update one copy and leave the other stale, making the function's contract ambiguous, and every reader has to parse the same paragraph twice.
Fix: delete one of the two identical copies so a single "A tool_use in the very last message…" paragraph remains.
中文说明
从这里开始的 JSDoc 段落（"A tool_use in the very last message…"）与紧接在它下方的段落逐字重复。本次 diff 在已有段落上方又插入了一份完全相同的副本，因此 cleanOrphanedToolCalls 的文档现在连续出现了两遍同一段八行文字（约第 1420–1427 行与第 1429–1436 行）。具体代价：将来修改"最后一条消息豁免"策略（例如细化 "no result yet" 的语义）时，可能只更新其中一份而让另一份过期，从而使该函数的契约变得含糊；每个读者也都不得不把同一段话读两遍。
修复：删除两份完全相同的副本中的一份，只保留一个 "A tool_use in the very last message…" 段落。
— qwen3.8-max-preview via Qwen Code /review
| const finalBlocks = | ||
| toolUseRemoved && message.role === 'assistant' | ||
| ? filtered.filter((b) => { |
There was a problem hiding this comment.
[Suggestion] The cascade's "turn emptied → whole message dropped" branch has no test. The only cascade test ("cascade-strips a signed thinking block when its sibling tool_use is orphaned in the same pass", converter.test.ts:1151) gives the turn a surviving text block, so finalBlocks is never empty and the else drop branch here is never exercised by the cascade. — Concrete cost: the untested variant is a same-turn assistant turn whose only blocks are a signed thinking plus an orphaned tool_use (no surviving text). The cascade strips the thinking, finalBlocks becomes empty, and the if (finalBlocks.length > 0) guard drops the message — a path this diff newly creates (pre-diff the thinking survived). A regression here (pushing a content: [] message, or failing to drop the emptied turn so an invalid-signature thinking block is replayed and Anthropic 400s) would ship undetected.
Fix: add a test — a model turn with only { text: 'reasoning', thought: true, thoughtSignature: 'sig' } plus an orphaned functionCall, followed by a user message lacking the matching tool_result and surrounded by two user messages; assert the assistant turn is gone and the two surrounding user messages are merged.
中文说明
级联清理中"turn 被清空 → 整条消息被丢弃"的分支没有测试覆盖。唯一的级联测试（"cascade-strips a signed thinking block when its sibling tool_use is orphaned in the same pass"，converter.test.ts:1151）给该 turn 保留了一个 text 块，因此 finalBlocks 永远不会为空，这里的 else 丢弃分支也就永远不会被级联触发。具体代价：未被测试的形态是这样一个 assistant turn——它仅有的块是一个带签名的 thinking 加一个孤儿 tool_use（没有幸存的文本）。级联会剥掉 thinking，finalBlocks 变为空，if (finalBlocks.length > 0) 守卫随即丢弃该消息——这是本 diff 新引入的路径（改动前 thinking 会存活）。此处的回归（例如推出一条 content: [] 消息，或未能丢弃被清空的 turn 导致一个签名失效的 thinking 块被重放、从而 Anthropic 返回 400）将不会被检测到。
修复：补一个测试——一个 model turn 只含 { text: 'reasoning', thought: true, thoughtSignature: 'sig' } 加一个孤儿 functionCall，其后跟一条缺少对应 tool_result 的 user 消息，并在两侧各有一条 user 消息；断言该 assistant turn 被移除、且两侧的两条 user 消息被合并。
— qwen3.8-max-preview via Qwen Code /review
…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).
netbrah
changed the title
|
@wenshao Pushed the narrowed version per your review — thank you for the depth here, especially tracing the actual reachability of the cross-turn state through
97/97 tests pass, tsc and eslint clean. Ready for another look whenever you have time. |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
中文说明
已审查——无阻断问题。 建议见行内评论。
— qwen3.8-max-preview via Qwen Code /review
| ]); | ||
| }); | ||
|
|
||
| it('drops an empty redacted_thinking-derived turn entirely (defensive, no plaintext fallback)', () => { |
There was a problem hiding this comment.
[Suggestion] No test covers the partial-filter branch of dropEmptyTextThinkingBlocks — a non-latest assistant turn where the empty-text thinking block is stripped but a sibling (e.g. text) survives and the message is kept via out.push({ role: msg.role, content: filtered }) (converter.ts:1662). — Concrete cost: a mutation out.push({ role: msg.role, content: filtered }) → out.push(msg) survives the entire suite (probe-verified): the "leaves a signed, non-empty thinking block" test's filter is a no-op (filtered === msg.content), the "drops an empty redacted_thinking-derived turn entirely" test below takes the filtered.length === 0 branch and never reaches the push, and the latest-turn test takes the early return. A non-latest turn shaped [{ text: '', thought: true }, { text: 'hello' }] distinguishes them — correct code emits [{ type: 'text', text: 'hello' }], the mutant emits [{ type: 'thinking', thinking: '' }, { type: 'text', text: 'hello' }], sending an invalid empty-text thinking block on the wire. Add a test next to this one:
it('removes an empty-text thinking block but keeps sibling text on a non-latest turn', () => {
const { messages } = converter.convertGeminiRequestToAnthropic({
model: 'models/test',
contents: [
{ role: 'user', parts: [{ text: 'Hi' }] },
{ role: 'model', parts: [{ text: '', thought: true }, { text: 'Hello there' }] },
{ role: 'user', parts: [{ text: 'anything else?' }] },
{ role: 'model', parts: [{ text: 'Sure.' }] },
],
});
const olderAssistant = messages[1];
expect(olderAssistant.role).toBe('assistant');
expect(olderAssistant.content).toEqual([{ type: 'text', text: 'Hello there' }]);
});中文说明
没有测试覆盖 dropEmptyTextThinkingBlocks 的部分过滤分支——即某个非最新 assistant turn 中文本为空的 thinking 块被剥离、但兄弟块（如 text）幸存、消息通过 out.push({ role: msg.role, content: filtered })（converter.ts:1662）被保留的情形。具体代价：将 out.push({ role: msg.role, content: filtered }) 突变为 out.push(msg) 后，整个测试套件仍能通过（已用 probe 验证）："leaves a signed, non-empty thinking block" 测试的过滤是空操作（filtered === msg.content）；下方 "drops an empty redacted_thinking-derived turn entirely" 测试走 filtered.length === 0 分支、永远到不了 push；最新 turn 测试走提前返回。形如 [{ text: '', thought: true }, { text: 'hello' }] 的非最新 turn 可以区分二者——正确代码输出 [{ type: 'text', text: 'hello' }]，突变体输出 [{ type: 'thinking', thinking: '' }, { type: 'text', text: 'hello' }]，从而把一个无效的文本为空 thinking 块发到线上。建议在旁边补充一个测试（见上方代码块）。
— qwen3.8-max-preview via Qwen Code /review
Local verification report — round 2 (maintainer)I rebuilt this branch from scratch and re-ran the whole verification against the narrowed revision ( What I ran
The strict server is the part that makes the live layer meaningful: a 1. My previous round's findings are fixed — verified, not assumedSame probe, run against
That third one is worth calling out: the fix wasn't just deleting the broken heuristic. The same hazard applied to the extracted empty-text guard, and you caught it in your own follow-up round and moved the guard after the fail-loud check. That's the right call and it's the change I'd have asked for. 2. Behaviour matrix — merge-base vs head
3 of 9 scenarios change; the other 6 are pinned as guards.
S7 matters more than it looks. S4's gating is also worth stating explicitly, because it's what makes this guard non-redundant: 3. Live A/B over a real socket
Turn 1 is a real streaming request; the server answers with a signed thinking block plus a sibling
4. End-to-end CLI session
The bundled 5. Build, tests, test efficacy
97/97 in On test efficacy: I replayed the head's test file against the merge-base implementation. 3 of the 7 new tests fail there — Two notes, neither blockingReachability of the same-turn cascade. I went looking for a path where the converter's I'm raising this because it's the same argument that (correctly) killed PATCH-B, and it deserves an answer rather than being quietly asymmetric. The answer is that these are different kinds of code. PATCH-B was a speculative cross-turn heuristic inferring an unobservable past state from an ambiguous present one. This is a local self-consistency invariant of a single function: whenever Residual boundary: the latest-turn exemption. An empty-text Minor, non-blocking: VerdictApprove. This is now the narrow, provable version of the change: it only removes content that would otherwise cause a confirmed 400, or that is unconditionally invalid regardless of context. Every concern from my previous round reproduces on the old revision and is resolved on this one, and the DeepSeek and active-tool-loop boundaries hold on the wire. Thanks for taking the reproduction question seriously instead of arguing the contract — cutting your own change down to its provable core on review evidence is the harder and better call. 中文版本本地验证报告 —— 第 2 轮（维护者）我从零重新构建了这个分支，针对收窄后的版本（ 我跑了什么
严格服务端是让 live 这层有意义的关键： 1. 上一轮的发现确已修复 —— 是验证过的，不是假定的同一个探针，分别跑在
第三点值得单独说：修复方式不只是删掉那个有问题的启发式。同一个隐患也适用于被提取出来的空文本保护逻辑，而你在自己后续的一轮 review 中发现了这点，并把该保护逻辑移到了快速失败检查之后。这是正确的处理，也正是我本来会要求的改法。 2. 行为矩阵 —— merge-base vs head9 个场景中 3 个发生变化，其余 6 个作为护栏被钉住（见上方第一张图）。
S7 比看上去更重要。 S4 的门控同样值得明说，因为这正是这个保护逻辑不冗余的原因：对原生 3. 真实 socket 上的 live A/B第一轮是真实流式请求；服务端返回一个已签名的 thinking 块加一个兄弟
4. 端到端 CLI 会话PR head 打包出的 5. 构建、测试、测试有效性
关于测试有效性：我把 head 的测试文件放到 merge-base 实现上重放。7 个新测试中有 3 个在那里失败 —— 两点说明，均不阻塞同 turn 级联的可达性。 我去找了今天转换器的 我提这一点，是因为这正是（正确地）否决 PATCH-B 的同一个论据，它值得一个回答，而不是被悄悄地区别对待。回答是：这是两类不同的代码。PATCH-B 是一个推测性的跨 turn 启发式，试图从一个含混的当前状态推断一个不可观测的过去状态。而这个改动是单个函数的局部自洽不变式：只要 残留边界：最新 turn 的豁免。 出现在最新 assistant turn 上、由 小问题，不阻塞： 结论同意合并。 这已经是该改动收窄后可证明的版本：它只移除会导致已确认 400 的内容，或在任何上下文下都无条件无效的内容。我上一轮的每一条顾虑都在旧版上复现、在这一版上解决，DeepSeek 与活跃工具循环两个边界也在 wire 上成立。感谢你认真对待可复现性这个问题，而不是拿契约来争辩 —— 依据 review 证据把自己的改动收窄到可证明的核心，是更难也更正确的选择。 |
|
@qwen-code /triage |
|
Sandboxed verification: Skipped because the pre-execution risk screen refused this sponsored run: the model risk screen failed or returned an unparseable answer. A maintainer who has reviewed the diff can run the verification manually in a disposable environment. 中文 — 判定：
|
|
@qwen-code /resolve |
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. ✅
|
Qwen Code resolved the merge conflicts and pushed the branch update. Root cause
Textual, not semanticAdjacent insertions of independent features; the filter body auto-merged and already referenced both variables, so the fix was keeping both declarations: let toolUseRemoved = false; // PR #8166: cascade-strip tracking
const keepToolResult = makeToolResultDeduper(); // main #8163: tool_result dedup
const filtered = blocks.filter((b) => {
// tool_use: if (!keep) toolUseRemoved = true; (PR)
// tool_result: return keepToolResult(id); (main)
});
What is load-bearing
What I could not verifyNo build/typecheck/tests run here. Both sides also added tests to 中文说明根因： 文本冲突，非语义冲突：两个功能相互独立，git 已自动合并过滤回调主体，其中同时引用了两个变量，因此解决方式是保留两行声明： 关键约束：有效块集合（ 未能验证：此处未运行构建/类型检查/测试。双方都向 |
44fe5d7
|
@qwen-code /triage |
|
Triage re-run completed without a new review.
The stage comments above were updated with the latest result. View workflow run. |
|
Sandboxed verification: Skipped because the pre-execution risk screen refused this sponsored run: the model risk screen failed or returned an unparseable answer. A maintainer who has reviewed the diff can run the verification manually in a disposable environment. 中文 — 判定：
|
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. ✅
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
|
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
What this PR does
Cascade-strips a
thinking/redacted_thinkingsibling from an assistant turn when the last survivingtool_useis stripped from that same turn as an orphan (incleanOrphanedToolCalls), plus a small unconditional guard dropping anythinkingblock with literally empty text on a non-latest assistant turn.This PR previously also included a broader cross-turn heuristic (
pruneUntrustworthyThinking) attempting to catch a non-latest, thinking-only turn whosetool_usehad gone stale via an earlier history trim. That heuristic has been removed after @wenshao's live-verified review — see "What changed since the original submission" below.Why it's needed
Anthropic validates a
thinking/redacted_thinkingblock's opaque signature against the content it was originally computed over. Removing a siblingtool_usefrom that turn — via this converter's own orphan-cleanup pass — leaves athinkingblock 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_useand leaving itsthinkingsibling stale — see #8159/#8163), which is what prompted auditing this converter for the same class of gap.What changed since the original submission
@wenshao built both worktrees locally, ran an A/B against the merge-base, drove a live session against a real Anthropic-protocol endpoint, and tested this codebase's actual compaction and orphan-repair machinery directly rather than reasoning about it abstractly. Findings:
[thinking, tool_use A, tool_use B]where only B is a genuine orphan still cascaded away the thinking even though A survives and still needs it — per Anthropic's manual-mode contract, the final turn must begin with a thinking block when anytool_useis present, so stripping it here traded one 400 for another. Fixed: the cascade now only fires when notool_usesurvives the turn.thinkingblock with literally empty text (arising when aredacted_thinkingblock round-trips through Gemini-Partconversion and loses its opaquedata). This is correct regardless of the broader heuristic's premise. Kept, extracted into its own function (dropEmptyTextThinkingBlocks).dropUnsignedThinkingFromAssistantMessages, which exists specifically to fail loudly when a proxy omits a signature mid-active-tool-loop — re-typing the block to text first made that check no longer recognize it as thinking, silently swallowing exactly the proxy bug the check exists to surface); an incomplete DeepSeek exclusion (only gated one of DeepSeek's two thinking modes); and a live-verified false positive (a thinking-only turn that never carried atool_use— so its signature was never actually invalidated — got re-typed to text one request later purely because a newer assistant turn displaced it as "latest," breaking a cache breakpoint and costing tokens on last-turn-only models). wenshao also tried to reproduce the state the heuristic exists to clean up (a non-latest turn whosetool_usewent stale via an earlier trim) against this codebase's real compaction and orphan-repair paths and could not. Removed entirely — this repo's own convention is not to carry defensive code for a state that can't currently occur.During implementation, the same pass-ordering hazard identified in point 3 above turned out to still apply to the extracted guard from point 2 (an empty-text, unsigned
redacted_thinking-derived block on an active tool-loop turn could be silently deleted before the fail-loud check saw it) — caught in a follow-up review round and fixed by running the guard after that check, with a regression test.Reviewer Test Plan
How to verify
Expected: all 97 tests pass. Key scenarios: the same-turn cascade fires only when no
tool_usesurvives the turn; an empty-text thinking block is dropped on non-latest turns but the latest turn is always exempt; the active-tool-loop fail-fast still throws when an empty-text unsigned thinking block sits on a non-latest step of an in-progress tool loop; a cascade that empties a turn out entirely drops the message and merges the surrounding user turns.Evidence (Before & After)
N/A — no UI surface; behavior change is in which
thinking/redacted_thinkingblocks survive conversion to the outbound Anthropic request body. See the test names for exact before/after shapes per scenario.Tested on
Risk & Scope
Linked Issues
Fixes #8162
中文说明
本 PR 做了什么
当一个 assistant turn 中最后一个幸存的
tool_use作为孤儿被剥离时（在cleanOrphanedToolCalls中），级联移除该 turn 中的thinking/redacted_thinking兄弟块；此外还有一个很小的、无条件的保护逻辑，用于丢弃非最新 assistant turn 上任何文本为空的thinking块。本 PR 此前还包含一个更宽泛的跨 turn 启发式（
pruneUntrustworthyThinking），试图捕获一个非最新、纯 thinking 的 turn，其tool_use可能在更早的历史裁剪中已经过期。该启发式已被移除——详见下方"自最初提交以来的变化"。为什么需要这个改动
Anthropic 会将
thinking/redacted_thinking块的不透明签名与其最初计算时所依据的内容进行校验。通过本转换器自身的孤儿清理过程移除该 turn 的一个兄弟tool_use，会留下一个签名不再匹配的thinking块，导致：thinking blocks in the latest assistant message cannot be modified。本次 session 中我们从一个相关但不同的成因独立撞上了完全相同的报错文本（另一种孤儿清理实现剥离了当前 turn 的
tool_use，却留下了过期的thinking兄弟块——见 #8159/#8163），这正是促使我们审查此转换器是否存在同类缺口的原因。自最初提交以来的变化
@wenshao 在本地构建了两个 worktree，对 merge-base 做了 A/B 测试，针对真实的 Anthropic 协议端点跑了一次 live 会话，并直接对这个代码库真实的压缩与孤儿修复机制做了测试，而不是停留在抽象推理层面。发现如下：
[thinking, tool_use A, tool_use B]的 turn，如果只有 B 是真正的孤儿，此前仍会把 thinking 一并级联移除，即便 A 幸存下来仍然需要它——按照 Anthropic 手动模式的契约，只要 turn 中还存在tool_use，最终 turn 就必须以 thinking 块开头，因此在这里剥离 thinking 相当于用一个 400 换了另一个 400。已修复：现在只有当该 turn 中没有任何tool_use幸存时，级联才会触发。thinking块（这种情况出现在redacted_thinking块经过 GeminiPart转换往返、丢失其不透明data之后）。无论更宽泛启发式的前提是否成立，这一点本身都是正确的。已保留，并被提取为独立函数（dropEmptyTextThinkingBlocks）。dropUnsignedThinkingFromAssistantMessages之前运行，而后者正是专门用来在代理漏传签名、且该 turn 仍处于一个正在进行中的工具调用循环时主动报错的——先把该块重新标记为文本，会让后面这个检查再也无法识别出它是 thinking，从而悄悄掩盖了这个检查本应暴露出的代理问题）；一个不完整的 DeepSeek 排除逻辑（只覆盖了 DeepSeek 两种 thinking 模式中的一种）；以及一个经 live 验证的假阳性（一个从未携带过tool_use、因而签名从未真正失效的纯 thinking turn，仅仅因为被一个更新的 assistant turn 取代了"最新"的位置，就在下一次请求中被重新标记为文本——这会破坏缓存断点，并且在只信任最后一轮的模型上白白消耗 token）。wenshao 还尝试针对这个代码库真实的压缩与孤儿修复路径去复现该启发式本应清理的状态（一个tool_use在更早的裁剪中已经过期的非最新 turn），但未能复现。已完全移除——为一个当前架构下根本不会出现的状态编写防御性代码，不符合本仓库自身的约定。在实现过程中，第 3 点中提到的同一种执行顺序隐患，被发现同样适用于第 2 点中提取出来的保护逻辑（一个处于活跃工具调用循环中的非最新步骤上、文本为空且未签名的
redacted_thinking衍生块，可能在快速失败检查看到它之前就被悄悄删除）——这一点在后续的一轮审查中被发现，并通过把该保护逻辑调整到那个检查之后运行来修复，同时补充了对应的回归测试。审阅者测试计划
如何验证
预期：全部 97 个测试通过。关键场景包括：只有当该 turn 中没有任何
tool_use幸存时，同 turn 级联才会触发；文本为空的 thinking 块会在非最新 turn 上被丢弃，但最新 turn 始终被豁免；当一个文本为空、未签名的 thinking 块出现在一个正在进行中的工具调用循环的非最新步骤上时，主动失败检查仍然会正确报错；当级联把某个 turn 完全清空时，该消息会被丢弃，并且前后相邻的 user turn 会被合并。证据（前后对比）
不适用——没有 UI 界面；行为变化体现在转换到出站 Anthropic 请求体时哪些
thinking/redacted_thinking块得以存活。各场景具体的前后形态见测试名称。测试环境
风险与范围
关联 Issue
Fixes #8162