fix(claudeHelper): preserve latest assistant thinking blocks verbatim - #2224
Conversation
Derived from squash commit 161cfcf (PR #9). The original squash was fat (316 files) because the source branch was rebased on an old base; this commit applies only the claudeHelper-relevant files surgically onto deploy. Computes latestAssistantIndex once before the message loop and skips the rewrite-to-redacted-thinking transform on the latest assistant message. Symmetric guard for non-Anthropic Claude-shape providers preserves plain thinking.thinking text on the latest message. Co-authored-by: OmniRoute Ops <ops@nomenak.dev>
There was a problem hiding this comment.
Code Review
This pull request introduces provider-aware handling for Claude's thinking blocks, specifically addressing compatibility issues with non-Anthropic providers (like kimi-coding or glmt) that do not support redacted thinking blobs. It implements a reasoning cache lookup to restore original thinking text for these providers and adds a guard to preserve thinking blocks verbatim in the latest assistant message to comply with Anthropic's API requirements. Feedback focuses on improving maintainability by exporting the new placeholder constant from the helper module instead of duplicating it in test files.
| // - reasoningCache has no entry for the corresponding tool_use.id | ||
| // Must be non-empty: kimi-coding treats empty `thinking.thinking` as | ||
| // `reasoning_content missing` and 400s. | ||
| const NON_ANTHROPIC_THINKING_PLACEHOLDER = "(prior reasoning summary unavailable)"; |
There was a problem hiding this comment.
To improve maintainability and avoid duplication across the codebase, consider exporting this constant so it can be reused directly in the unit tests.
| const NON_ANTHROPIC_THINKING_PLACEHOLDER = "(prior reasoning summary unavailable)"; | |
| export const NON_ANTHROPIC_THINKING_PLACEHOLDER = "(prior reasoning summary unavailable)"; |
There was a problem hiding this comment.
Done in ddf1ba2 — exported the constant from claudeHelper.ts (now export const NON_ANTHROPIC_THINKING_PLACEHOLDER). Thanks.
| const { prepareClaudeRequest } = await import("../../open-sse/translator/helpers/claudeHelper.ts"); | ||
| const { DEFAULT_THINKING_CLAUDE_SIGNATURE } = | ||
| await import("../../open-sse/config/defaultThinkingSignature.ts"); | ||
| const reasoningCache = await import("../../open-sse/services/reasoningCache.ts"); | ||
|
|
||
| // Placeholder string from claudeHelper.ts — kept in sync via direct constant. | ||
| const PLACEHOLDER = "(prior reasoning summary unavailable)"; |
There was a problem hiding this comment.
Instead of hardcoding the placeholder string again, you can import it from the claudeHelper module (assuming it is exported as suggested). This ensures that the tests stay in sync with the implementation and reduces the risk of regression if the placeholder text changes.
const { prepareClaudeRequest, NON_ANTHROPIC_THINKING_PLACEHOLDER: PLACEHOLDER } = await import("../../open-sse/translator/helpers/claudeHelper.ts");
const { DEFAULT_THINKING_CLAUDE_SIGNATURE } =
await import("../../open-sse/config/defaultThinkingSignature.ts");
const reasoningCache = await import("../../open-sse/services/reasoningCache.ts");There was a problem hiding this comment.
Done in ddf1ba2 — test now imports it via { NON_ANTHROPIC_THINKING_PLACEHOLDER: PLACEHOLDER } from the destructure, dropped the duplicated literal. Thanks.
… reuse in tests Per gemini-code-assist review on diegosouzapw#2224: export the placeholder constant from claudeHelper.ts and import it in the unit test rather than duplicating the literal. Keeps test in sync with implementation.
|
Hey @NomenAK! Thanks for the rigorous claudeHelper rework. Heads-up: this PR significantly overlaps with two PRs already merged into
Your approach (per-provider switch between redacted_thinking vs plain thinking) is structurally different from the upstream approach (provider-agnostic placeholder + data field). Mergeable shows UNKNOWN — likely heavy conflicts. Could you rebase against
A smaller, focused follow-up PR would be much easier to merge than the current full-rewrite. Leaving this open. |
|
Thanks for the catch — already addressed in
Happy to consolidate further if it's worth a follow-up commit. |
Deep audit of all 320 commits since v3.7.9 found: - 18 merged PRs not documented in CHANGELOG (4 features, 10 bug fixes, 1 security, 2 chores, 1 debug improvement) - 3 contributors entirely missing from credits table (@NomenAK with 12 PRs, @kang-heewon, @one-vs) - 4 existing contributors with inaccurate PR counts (@oyi77 8→12, @ddarkr 2→3, @andrewmunsell 2→3, @nickwizard 2→3) New entries added: - feat: #2135 (1proxy settings), #2227 (antigravity project ID), #2238 (Z.AI Search), #2240 (CLI Suite) - fix: #2217, #2218, #2219, #2221, #2222, #2223, #2224, #2231, #2233, #2236, #2242, #2243 - security: #2209 (stack trace exposure) - chore: #2228, #2234 Total contributors updated from 50+ to 55+.
… reuse in tests Per gemini-code-assist review on diegosouzapw#2224: export the placeholder constant from claudeHelper.ts and import it in the unit test rather than duplicating the literal. Keeps test in sync with implementation.
…ant thinking blocks verbatim Fixes Anthropic HTTP 400 errors (~49/h on claude-opus-4-7) by preserving the latest assistant message's thinking blocks verbatim instead of rewriting them to redacted_thinking. Co-authored-by: NomenAK <anton@nomenak.dev>
Deep audit of all 320 commits since v3.7.9 found: - 18 merged PRs not documented in CHANGELOG (4 features, 10 bug fixes, 1 security, 2 chores, 1 debug improvement) - 3 contributors entirely missing from credits table (@NomenAK with 12 PRs, @kang-heewon, @one-vs) - 4 existing contributors with inaccurate PR counts (@oyi77 8→12, @ddarkr 2→3, @andrewmunsell 2→3, @nickwizard 2→3) New entries added: - feat: diegosouzapw#2135 (1proxy settings), diegosouzapw#2227 (antigravity project ID), diegosouzapw#2238 (Z.AI Search), diegosouzapw#2240 (CLI Suite) - fix: diegosouzapw#2217, diegosouzapw#2218, diegosouzapw#2219, diegosouzapw#2221, diegosouzapw#2222, diegosouzapw#2223, diegosouzapw#2224, diegosouzapw#2231, diegosouzapw#2233, diegosouzapw#2236, diegosouzapw#2242, diegosouzapw#2243 - security: diegosouzapw#2209 (stack trace exposure) - chore: diegosouzapw#2228, diegosouzapw#2234 Total contributors updated from 50+ to 55+.
… reuse in tests Per gemini-code-assist review on diegosouzapw#2224: export the placeholder constant from claudeHelper.ts and import it in the unit test rather than duplicating the literal. Keeps test in sync with implementation.
…ant thinking blocks verbatim Fixes Anthropic HTTP 400 errors (~49/h on claude-opus-4-7) by preserving the latest assistant message's thinking blocks verbatim instead of rewriting them to redacted_thinking. Co-authored-by: NomenAK <anton@nomenak.dev>
Deep audit of all 320 commits since v3.7.9 found: - 18 merged PRs not documented in CHANGELOG (4 features, 10 bug fixes, 1 security, 2 chores, 1 debug improvement) - 3 contributors entirely missing from credits table (@NomenAK with 12 PRs, @kang-heewon, @one-vs) - 4 existing contributors with inaccurate PR counts (@oyi77 8→12, @ddarkr 2→3, @andrewmunsell 2→3, @nickwizard 2→3) New entries added: - feat: diegosouzapw#2135 (1proxy settings), diegosouzapw#2227 (antigravity project ID), diegosouzapw#2238 (Z.AI Search), diegosouzapw#2240 (CLI Suite) - fix: diegosouzapw#2217, diegosouzapw#2218, diegosouzapw#2219, diegosouzapw#2221, diegosouzapw#2222, diegosouzapw#2223, diegosouzapw#2224, diegosouzapw#2231, diegosouzapw#2233, diegosouzapw#2236, diegosouzapw#2242, diegosouzapw#2243 - security: diegosouzapw#2209 (stack trace exposure) - chore: diegosouzapw#2228, diegosouzapw#2234 Total contributors updated from 50+ to 55+.
What
Preserve the LATEST assistant message's
thinkingblocks verbatim instead of rewriting them toredacted_thinking. Apply the same invariant to non-Anthropic Claude-shape upstreams (kimi-coding, glmt, zai…) where the latest message's plainthinking.thinkingtext must be sent as-is.Why
Anthropic's 2026 thinking-mode contract distinguishes:
redacted_thinking { data }— Anthropic accepts the synthetic blob."thinking or redacted_thinking blocks in the latest assistant message cannot be modified. These blocks must remain as they were in the original response.".On our fork, we observed 49 occurrences/hour of this error on
claude-opus-4-7with conversational replay before this fix.Distinction from already-merged
5c11b575: that commit added thedatafield on syntheticredacted_thinking(preventingmessages.N.content.M.redacted_thinking.data: Field required400s). It is a different invariant — about the field shape of rewritten blocks. This PR is about which assistant message is rewriteable at all: the index-based "skip rewrite on the latest assistant" guard. The two fixes are complementary and both required.How
latestAssistantIndexonce before the message loop inprepareClaudeRequest:thinking.thinkingtext when it's non-empty (only fall through toreasoningCache/placeholder for older messages or empty-text latest)tests/unit/translator-claude-helper-thinking.test.tscovering Anthropic preservation and non-Anthropic symmetry; updates to 2 existing tests that previously encoded the all-rewrites behaviorNotes
signature/datafield cleanup and normalizetypeto"thinking"— only the text content is preserved verbatim on the latest message