[codex] fix: sanitize unsigned Claude thinking blocks before upstream requests#2534
[codex] fix: sanitize unsigned Claude thinking blocks before upstream requests#2534leehogwang wants to merge 3 commits intorouter-for-me:mainfrom
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a request sanitization layer for Claude executors to remove assistant thinking blocks that lack a valid signature, preventing API rejections during mixed-provider sessions. The changes include the implementation of the sanitizer using gjson/sjson and comprehensive unit tests. Review feedback points out a redundant conditional check when updating the JSON body and highlights a potential edge case where removing all thinking blocks could result in an empty content array, which may still be rejected by the Anthropic API.
| if len(keepBlocks) == 0 { | ||
| body, err = sjson.SetBytes(body, contentPath, []any{}) | ||
| } else { | ||
| body, err = sjson.SetBytes(body, contentPath, keepBlocks) | ||
| } |
There was a problem hiding this comment.
The conditional check for len(keepBlocks) == 0 is redundant. sjson.SetBytes correctly handles an empty slice by encoding it as an empty JSON array []. You can simplify this by calling sjson.SetBytes directly with keepBlocks.
body, err = sjson.SetBytes(body, contentPath, keepBlocks)There was a problem hiding this comment.
Updated in 7678161a. The sanitizer now rewrites the full messages array in one pass, so the redundant empty-slice branch is gone.
| keepBlocks := make([]any, 0, len(blocks)) | ||
| removedCount := 0 | ||
|
|
||
| for _, block := range blocks { | ||
| if block.Get("type").String() == "thinking" { | ||
| sig := block.Get("signature") | ||
| if !sig.Exists() || sig.Type != gjson.String || strings.TrimSpace(sig.String()) == "" { | ||
| removedCount++ | ||
| continue | ||
| } | ||
| } | ||
| keepBlocks = append(keepBlocks, block.Value()) | ||
| } | ||
|
|
||
| if removedCount == 0 { | ||
| continue | ||
| } | ||
|
|
||
| contentPath := fmt.Sprintf("messages.%d.content", msgIdx) | ||
| var err error | ||
| if len(keepBlocks) == 0 { | ||
| body, err = sjson.SetBytes(body, contentPath, []any{}) | ||
| } else { | ||
| body, err = sjson.SetBytes(body, contentPath, keepBlocks) | ||
| } |
There was a problem hiding this comment.
If an assistant message contains only invalid thinking blocks, removing them will leave the message with an empty content array (e.g., "content": []). Anthropic's API will reject such requests with a 400 Bad Request (error message: at least one content block is required). While this is a different error than the signature mismatch, it still results in a failed request. Consider if you should log a specific warning when a message becomes empty or handle it by removing the message entirely (though the latter requires careful handling of role alternation).
There was a problem hiding this comment.
Updated in 7678161a. Assistant messages that become empty after stripping invalid thinking blocks are now dropped from the forwarded history instead of leaving content: [] behind. I also added executor-path coverage plus a live proxy smoke test for that case.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7678161af8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if !sig.Exists() || sig.Type != gjson.String || strings.TrimSpace(sig.String()) == "" { | ||
| removedCount++ |
There was a problem hiding this comment.
The sanitizer currently accepts any non-empty string as a "valid" thinking signature (signature check here), so it will keep synthetic signatures from non-Claude translators instead of stripping them. For example, internal/translator/antigravity/claude/antigravity_claude_response.go sets signature to "<modelGroup>#<thoughtSignature>", which is not a Claude-issued signature; when such history is sent back to Anthropic, the upstream can still return Invalid signature in thinking block. This means the mixed-provider failure this change is addressing is still reproducible for providers that emit non-empty synthetic signatures.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Updated in 7256e72c. The sanitizer now treats translator-prefixed signatures (gpt#..., gemini#..., claude#...) as synthetic and strips those thinking blocks before forwarding back to Anthropic. I also tightened the regression coverage so the executor-path tests now verify that raw long Claude-style signatures survive while synthetic prefixed signatures are removed.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7256e72ca2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| switch prefix { | ||
| case "gpt", "claude", "gemini": | ||
| return prefix, rawSignature, true |
There was a problem hiding this comment.
splitSyntheticThinkingSignature only marks gpt, claude, and gemini prefixes as synthetic, but translator output is created as <cache.GetModelGroup(modelName)>#<sig> and GetModelGroup falls back to the raw model name for other families (for example o3/qwen). In those sessions, prefixed signatures are not stripped here, then cache.HasValidSignature accepts them by length, and Claude upstream can still reject the request with Invalid signature in thinking block when users switch back to Anthropic.
Useful? React with 👍 / 👎.
luispater
left a comment
luispater
left a comment
There was a problem hiding this comment.
Summary:
This change addresses the main Invalid signature in thinking block failure mode by sanitizing Claude-bound histories across Execute, ExecuteStream, and CountTokens, and the targeted regression coverage is good. However, one sanitizer rule is still too aggressive and can drop valid Claude reasoning blocks.
Blocking:
internal/translator/antigravity/claude/antigravity_claude_response.gointentionally serializes Claude thinking signatures asclaude#<raw-signature>for downstream transport, andinternal/translator/antigravity/claude/antigravity_claude_request.goalready knows how to unwrap that format back to the raw signature.- The new sanitizer in
internal/runtime/executor/claude_request_sanitizer.gotreats every known prefixed signature as invalid and drops the whole thinking block instead of stripping the prefix and preserving the valid Claude-issued signature. - That means valid Claude thinking blocks coming back from the Antigravity Claude path are discarded on the next direct Claude request, which is a behavior regression and also contradicts the inline comment saying these prefixes "must be stripped before forwarding".
Requested fix:
- Preserve
claude#...thinking blocks by unwrapping them to the raw signature before forwarding upstream. - Add a regression test that proves a prefixed Claude signature survives sanitization as a raw Claude signature, instead of being removed.
Non-blocking:
- The new warning log for every removed assistant turn may be noisy for expected mixed-provider traffic;
debugorinfowould likely be a better fit once the behavior is intentional.
Test plan:
go test ./internal/runtime/executor -run 'TestSanitizeClaudeRequestBody|TestClaudeExecutor_'go test ./internal/translator/antigravity/claude -run 'TestConvertAntigravityResponseToClaude|TestConvertClaudeRequestToAntigravity'
2 participants
Summary
thinkingblocks that do not carry a valid signatureExecute,ExecuteStream, andCountTokensbefore tool-prefix rewriting or optional body signingWhy
Mixed-provider sessions can return to Claude with unsigned
thinkingblocks synthesized by non-Claude translators. Anthropic rejects those histories withInvalid signature in thinking block, which breaks same-sessioncodex -> sonnet/opusreturn.Impact
Visible assistant text, tool calls, and tool results are preserved. Only unsigned hidden
thinkingblocks are stripped before the request is forwarded back to Claude upstream.Validation
go test ./internal/runtime/executor/v1/messageswith an unsigned prior assistantthinkingblock; request returned200 OKinstead of400 Invalid signature in thinking block