Repository navigation
fix(6954,6953): preserve system role + strip empty-signature thinking blocks #6982
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 1 commit
ecb3fb7
d043837
3548d3f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -554,7 +554,23 @@ function getContentBlocksFromMessage( | |||||||||||||||||||||||||||||||||
| if (part.type === "text" && part.text) { | ||||||||||||||||||||||||||||||||||
| blocks.push({ type: "text", text: part.text }); | ||||||||||||||||||||||||||||||||||
| } else if (part.type === "thinking" || part.type === "redacted_thinking") { | ||||||||||||||||||||||||||||||||||
| // Preserve thinking blocks with signature | ||||||||||||||||||||||||||||||||||
| // #6953 — thinking blocks with empty/missing signatures come from non-Anthropic | ||||||||||||||||||||||||||||||||||
| // providers (codex/gpt-5.x). Anthropic rejects replayed `thinking` blocks that | ||||||||||||||||||||||||||||||||||
| // carry a foreign or fabricated signature with HTTP 400. Fabricating a default | ||||||||||||||||||||||||||||||||||
| // signature (the old behaviour) made the poisoning permanent: once a codex-served | ||||||||||||||||||||||||||||||||||
| // turn introduced a `signature:""` thinking block, every subsequent Anthropic leg | ||||||||||||||||||||||||||||||||||
| // attempt 400'd and the router silently fell back to codex forever. | ||||||||||||||||||||||||||||||||||
| // | ||||||||||||||||||||||||||||||||||
| // Fix: strip thinking blocks whose signature is empty/absent entirely. The text | ||||||||||||||||||||||||||||||||||
| // content is preserved separately via reasoning_content handling below when needed; | ||||||||||||||||||||||||||||||||||
| // dropping the replay-only block is the same as what a native Claude client does | ||||||||||||||||||||||||||||||||||
| // when it has no valid signature to replay. | ||||||||||||||||||||||||||||||||||
| if (part.type === "thinking" && !part.signature) { | ||||||||||||||||||||||||||||||||||
| continue; // drop — no valid signature to replay | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
| if (part.type === "redacted_thinking" && !part.data) { | ||||||||||||||||||||||||||||||||||
| continue; // drop — redacted_thinking with no data is equally invalid | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For We should push
Suggested change
|
||||||||||||||||||||||||||||||||||
| blocks.push({ | ||||||||||||||||||||||||||||||||||
| ...part, | ||||||||||||||||||||||||||||||||||
| signature: part.signature || DEFAULT_THINKING_CLAUDE_SIGNATURE, | ||||||||||||||||||||||||||||||||||
|
|
@@ -622,7 +638,12 @@ function getContentBlocksFromMessage( | |||||||||||||||||||||||||||||||||
| (b) => b.type === "thinking" || b.type === "redacted_thinking" | ||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||
| const hasToolUseBlock = blocks.some((b) => b.type === "tool_use"); | ||||||||||||||||||||||||||||||||||
| if (msg.reasoning_content && thinkingEnabledForRequest && hasToolUseBlock && !hasThinkingBlock) { | ||||||||||||||||||||||||||||||||||
| if ( | ||||||||||||||||||||||||||||||||||
| msg.reasoning_content && | ||||||||||||||||||||||||||||||||||
| thinkingEnabledForRequest && | ||||||||||||||||||||||||||||||||||
| hasToolUseBlock && | ||||||||||||||||||||||||||||||||||
| !hasThinkingBlock | ||||||||||||||||||||||||||||||||||
| ) { | ||||||||||||||||||||||||||||||||||
| blocks.unshift({ | ||||||||||||||||||||||||||||||||||
| type: "redacted_thinking", | ||||||||||||||||||||||||||||||||||
| data: DEFAULT_THINKING_CLAUDE_SIGNATURE, | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,103 @@ | ||
| /** | ||
| * Tests for #6954 — mid-conversation system turns misattributed as assistant. | ||
| * | ||
| * `convertClaudeMessage` mapped any role that wasn't "user" or "tool" to | ||
| * "assistant", so a Claude message with `role: "system"` (e.g. an injected | ||
| * system reminder mid-conversation) was forwarded to OpenAI-format upstreams | ||
| * as an assistant turn — polluting the conversation history. | ||
| */ | ||
| import test from "node:test"; | ||
| import assert from "node:assert/strict"; | ||
|
|
||
| const { claudeToOpenAIRequest } = | ||
| await import("../../open-sse/translator/request/claude-to-openai.ts"); | ||
|
|
||
| // --------------------------------------------------------------------------- | ||
| // 1. system message mid-conversation keeps role: "system" | ||
| // --------------------------------------------------------------------------- | ||
| test("mid-conversation system message preserves role:system (not assistant)", () => { | ||
| const result = claudeToOpenAIRequest( | ||
| "gpt-4o", | ||
| { | ||
| messages: [ | ||
| { role: "user", content: "hello" }, | ||
| { role: "assistant", content: "hi" }, | ||
| { role: "system", content: "Reminder: be concise." }, | ||
| { role: "user", content: "ok" }, | ||
| ], | ||
| }, | ||
| false | ||
| ); | ||
|
|
||
| const roles = result.messages.map((m: { role: string }) => m.role); | ||
| assert.deepEqual(roles, ["user", "assistant", "system", "user"]); | ||
| }); | ||
|
|
||
| // --------------------------------------------------------------------------- | ||
| // 2. system message with array content keeps role: "system" | ||
| // --------------------------------------------------------------------------- | ||
| test("system message with array content preserves role:system", () => { | ||
| const result = claudeToOpenAIRequest( | ||
| "gpt-4o", | ||
| { | ||
| messages: [ | ||
| { role: "user", content: "hello" }, | ||
| { | ||
| role: "system", | ||
| content: [{ type: "text", text: "System reminder text" }], | ||
| }, | ||
| ], | ||
| }, | ||
| false | ||
| ); | ||
|
|
||
| const sysMsg = result.messages.find((m: { role: string }) => m.role === "system"); | ||
| assert.ok(sysMsg, "expected a system message in output"); | ||
| // Array content with text blocks is flattened to a string for system role | ||
| assert.equal( | ||
| typeof sysMsg.content === "string" ? sysMsg.content : JSON.stringify(sysMsg.content), | ||
| "System reminder text" | ||
| ); | ||
| }); | ||
|
|
||
| // --------------------------------------------------------------------------- | ||
| // 3. top-level body.system still produces role: "system" (regression check) | ||
| // --------------------------------------------------------------------------- | ||
| test("body.system still produces role:system at index 0", () => { | ||
| const result = claudeToOpenAIRequest( | ||
| "gpt-4o", | ||
| { | ||
| system: "You are helpful.", | ||
| messages: [{ role: "user", content: "hi" }], | ||
| }, | ||
| false | ||
| ); | ||
|
|
||
| assert.equal(result.messages[0].role, "system"); | ||
| assert.equal(result.messages[1].role, "user"); | ||
| }); | ||
|
|
||
| // --------------------------------------------------------------------------- | ||
| // 4. assistant with tool_use still maps to assistant (regression check) | ||
| // --------------------------------------------------------------------------- | ||
| test("assistant role still maps to assistant (no regression)", () => { | ||
| const result = claudeToOpenAIRequest( | ||
| "gpt-4o", | ||
| { | ||
| messages: [ | ||
| { role: "user", content: "use the tool" }, | ||
| { | ||
| role: "assistant", | ||
| content: [ | ||
| { type: "text", text: "calling tool" }, | ||
| { type: "tool_use", id: "t1", name: "foo", input: {} }, | ||
| ], | ||
| }, | ||
| ], | ||
| }, | ||
| false | ||
| ); | ||
|
|
||
| const roles = result.messages.map((m: { role: string }) => m.role); | ||
| assert.ok(roles.includes("assistant"), "assistant role must be preserved"); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,167 @@ | ||
| /** | ||
| * TDD regression for #6953 — thinking blocks with empty signatures poison the | ||
| * Anthropic leg of combo/blend routes. | ||
| * | ||
| * Non-Anthropic providers (codex/gpt-5.x) synthesize Anthropic-format `thinking` | ||
| * blocks with `signature: ""`. When the client replays these in the next | ||
| * request's history, the Anthropic leg rejects them with HTTP 400 "Invalid | ||
| * signature in thinking block", and the router silently falls back to codex | ||
| * permanently. | ||
| * | ||
| * The old code fabricated a DEFAULT_THINKING_CLAUDE_SIGNATURE to fill the empty | ||
| * signature — but that fabricated signature is equally foreign to Anthropic, so | ||
| * it also 400'd. | ||
| * | ||
| * Fix (#6953): strip thinking blocks with empty/missing signatures entirely. | ||
| * They carry no replayable cryptographic value. For `redacted_thinking`, strip | ||
| * if `data` is empty/missing for the same reason. | ||
| */ | ||
| import test from "node:test"; | ||
| import assert from "node:assert/strict"; | ||
|
|
||
| const { openaiToClaudeRequest } = | ||
| await import("../../open-sse/translator/request/openai-to-claude.ts"); | ||
|
|
||
| test('#6953: thinking block with signature:"" is stripped, not fabricated', () => { | ||
| const result = openaiToClaudeRequest( | ||
| "claude-opus-4-8", | ||
| { | ||
| messages: [ | ||
| { role: "user", content: "hello" }, | ||
| { | ||
| role: "assistant", | ||
| content: [ | ||
| { type: "text", text: "I will help you." }, | ||
| { type: "thinking", thinking: "reasoning here", signature: "" }, | ||
| { | ||
| type: "text", | ||
| text: "Let me use a tool.", | ||
| }, | ||
| ], | ||
| }, | ||
| { role: "user", content: "ok go ahead" }, | ||
| ], | ||
| }, | ||
| false | ||
| ); | ||
|
|
||
| const assistant = result.messages.find((m) => m.role === "assistant"); | ||
| assert.ok(assistant, "expected assistant message"); | ||
|
|
||
| // The thinking block with empty signature must be DROPPED, not preserved | ||
| // with a fabricated signature. | ||
| const thinkingBlocks = assistant.content.filter((b) => b && b.type === "thinking"); | ||
| assert.equal( | ||
| thinkingBlocks.length, | ||
| 0, | ||
| "thinking block with empty signature must be stripped, not fabricated" | ||
| ); | ||
|
|
||
| // Text blocks must survive | ||
| const textBlocks = assistant.content.filter((b) => b && b.type === "text"); | ||
| assert.ok(textBlocks.length >= 1, "text blocks must be preserved"); | ||
| }); | ||
|
|
||
| test("#6953: thinking block with valid signature is preserved verbatim", () => { | ||
| const realSig = "EuY2xhdWRlLXNpZ25hdHVyZS0xNzA5..."; | ||
| const result = openaiToClaudeRequest( | ||
| "claude-opus-4-8", | ||
| { | ||
| messages: [ | ||
| { role: "user", content: "hello" }, | ||
| { | ||
| role: "assistant", | ||
| content: [ | ||
| { type: "thinking", thinking: "real reasoning", signature: realSig }, | ||
| { type: "text", text: "response" }, | ||
| ], | ||
| }, | ||
| { role: "user", content: "ok" }, | ||
| ], | ||
| }, | ||
| false | ||
| ); | ||
|
|
||
| const assistant = result.messages.find((m) => m.role === "assistant"); | ||
| assert.ok(assistant); | ||
|
|
||
| const thinkingBlocks = assistant.content.filter((b) => b && b.type === "thinking"); | ||
| assert.equal(thinkingBlocks.length, 1, "valid thinking block must be preserved"); | ||
| assert.equal(thinkingBlocks[0].signature, realSig, "valid signature must be preserved verbatim"); | ||
| }); | ||
|
|
||
| test("#6953: redacted_thinking with empty data is stripped", () => { | ||
| const result = openaiToClaudeRequest( | ||
| "claude-opus-4-8", | ||
| { | ||
| messages: [ | ||
| { role: "user", content: "hello" }, | ||
| { | ||
| role: "assistant", | ||
| content: [ | ||
| { type: "redacted_thinking", data: "" }, | ||
| { type: "text", text: "response" }, | ||
| ], | ||
| }, | ||
| { role: "user", content: "ok" }, | ||
| ], | ||
| }, | ||
| false | ||
| ); | ||
|
|
||
| const assistant = result.messages.find((m) => m.role === "assistant"); | ||
| assert.ok(assistant); | ||
|
|
||
| const redactedBlocks = assistant.content.filter((b) => b && b.type === "redacted_thinking"); | ||
| assert.equal(redactedBlocks.length, 0, "redacted_thinking with empty data must be stripped"); | ||
| }); | ||
|
|
||
| test("#6953: combo scenario — codex-sourced thinking block does not block Anthropic leg", () => { | ||
| // Simulates a combo route: turn 1 served by codex produced a thinking block | ||
| // with signature:"". Turn 2 should be able to route to Anthropic without | ||
| // the poisoned block causing a 400. | ||
| const result = openaiToClaudeRequest( | ||
| "claude-opus-4-8", | ||
| { | ||
| messages: [ | ||
| { role: "user", content: "write a function" }, | ||
| { | ||
| role: "assistant", | ||
| content: [ | ||
| { | ||
| type: "thinking", | ||
| thinking: "**Reviewing the request**\n\nI need to write a function...", | ||
| signature: "", // codex-sourced, no real signature | ||
| }, | ||
| { type: "text", text: "Here's the function:" }, | ||
| { | ||
| type: "tool_use", | ||
| id: "toolu_01abc", | ||
| name: "write_file", | ||
| input: { path: "main.rs", content: "fn main() {}" }, | ||
| }, | ||
| ], | ||
| }, | ||
| { role: "user", content: "looks good, now add tests" }, | ||
| ], | ||
| }, | ||
| false | ||
| ); | ||
|
|
||
| const assistant = result.messages.find((m) => m.role === "assistant"); | ||
| assert.ok(assistant); | ||
|
|
||
| // No thinking block with empty signature should survive | ||
| const badThinking = assistant.content.find( | ||
| (b) => b && (b.type === "thinking" || b.type === "redacted_thinking") && !b.signature && !b.data | ||
| ); | ||
| assert.equal( | ||
| badThinking, | ||
| undefined, | ||
| "no thinking/redacted_thinking block with empty signature/data should survive" | ||
| ); | ||
|
|
||
| // Tool use must survive | ||
| const toolUse = assistant.content.find((b) => b && b.type === "tool_use"); | ||
| assert.ok(toolUse, "tool_use block must be preserved"); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a defensive guard at the beginning of
convertClaudeMessageto handle cases wheremsgis null, undefined, or not an object. This prevents potential runtime crashes (e.g.,TypeError: Cannot read properties of null (reading 'role')) if the incoming message array contains malformed or null elements.