fix: preserve Claude thinking signatures in Codex translator#1932
fix: preserve Claude thinking signatures in Codex translator#1932Darley-Wey wants to merge 4 commits intorouter-for-me:devfrom
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the Codex translator to Claude by ensuring that 'thinking' blocks consistently include and correctly handle signature information. This involves modifying the streaming response conversion to delay the completion of thinking blocks until the signature is received, and explicitly adding a 'signature' field to both streaming and non-streaming thinking block outputs, thereby improving compliance and data integrity. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![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 effectively addresses the preservation of Claude thinking signatures when translating from Codex responses, for both streaming and non-streaming scenarios. The logic to delay closing thinking blocks until a signature is available is well-implemented, and the new finalizeCodexThinkingBlock function is a good addition. The changes are supported by a solid set of new regression tests that cover the key use cases. I have one suggestion to refine the logic for sending signature deltas.
| signatureDelta := `{"type":"content_block_delta","index":0,"delta":{"type":"signature_delta","signature":""}}` | ||
| signatureDelta, _ = sjson.Set(signatureDelta, "index", params.BlockIndex) | ||
| signatureDelta, _ = sjson.Set(signatureDelta, "delta.signature", params.ThinkingSignature) | ||
| output += "event: content_block_delta\n" | ||
| output += fmt.Sprintf("data: %s\n\n", signatureDelta) |
There was a problem hiding this comment.
To avoid sending a redundant signature_delta event when no signature has been received, it's better to make the emission of this event conditional on params.ThinkingSignature being non-empty. This ensures that a signature_delta is only sent when there is an actual signature to convey, aligning more closely with the purpose of a delta event.
| signatureDelta := `{"type":"content_block_delta","index":0,"delta":{"type":"signature_delta","signature":""}}` | |
| signatureDelta, _ = sjson.Set(signatureDelta, "index", params.BlockIndex) | |
| signatureDelta, _ = sjson.Set(signatureDelta, "delta.signature", params.ThinkingSignature) | |
| output += "event: content_block_delta\n" | |
| output += fmt.Sprintf("data: %s\n\n", signatureDelta) | |
| if params.ThinkingSignature != "" { | |
| signatureDelta := `{"type":"content_block_delta","index":0,"delta":{"type":"signature_delta","signature":""}}` | |
| signatureDelta, _ = sjson.Set(signatureDelta, "index", params.BlockIndex) | |
| signatureDelta, _ = sjson.Set(signatureDelta, "delta.signature", params.ThinkingSignature) | |
| output += "event: content_block_delta\n" | |
| output += fmt.Sprintf("data: %s\n\n", signatureDelta) | |
| } |
There was a problem hiding this comment.
Pull request overview
This PR updates the Codex→Claude response translator to preserve Claude “thinking” block signatures by ensuring the signature field is always present and by carrying encrypted_content through streaming and non-streaming conversions.
Changes:
- Add streaming state to keep thinking blocks open until
encrypted_contentis available, then emit it viasignature_delta. - Preserve
encrypted_contentassignaturein non-stream (single JSON) responses. - Add regression tests covering streaming and non-stream signature preservation.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| internal/translator/codex/claude/codex_claude_response.go | Adds thinking-block state + finalization helper; preserves/enriches thinking signatures in stream and non-stream conversions. |
| internal/translator/codex/claude/codex_claude_response_test.go | Adds targeted tests ensuring thinking blocks include signature and that signatures are preserved/emitted. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| signatureDelta := `{"type":"content_block_delta","index":0,"delta":{"type":"signature_delta","signature":""}}` | ||
| signatureDelta, _ = sjson.Set(signatureDelta, "index", params.BlockIndex) | ||
| signatureDelta, _ = sjson.Set(signatureDelta, "delta.signature", params.ThinkingSignature) | ||
| output += "event: content_block_delta\n" | ||
| output += fmt.Sprintf("data: %s\n\n", signatureDelta) |
There was a problem hiding this comment.
finalizeCodexThinkingBlock always emits a signature_delta event even when ThinkingSignature is empty. In cases where the upstream does not provide reasoning.encrypted_content (which appears to be allowed/optional elsewhere in the codebase), this will send a signature_delta with an empty signature, which is likely not a valid/meaningful Claude streaming event. Consider only emitting signature_delta when ThinkingSignature != "", while still emitting content_block_stop to close the thinking block.
| signatureDelta := `{"type":"content_block_delta","index":0,"delta":{"type":"signature_delta","signature":""}}` | |
| signatureDelta, _ = sjson.Set(signatureDelta, "index", params.BlockIndex) | |
| signatureDelta, _ = sjson.Set(signatureDelta, "delta.signature", params.ThinkingSignature) | |
| output += "event: content_block_delta\n" | |
| output += fmt.Sprintf("data: %s\n\n", signatureDelta) | |
| if params.ThinkingSignature != "" { | |
| signatureDelta := `{"type":"content_block_delta","index":0,"delta":{"type":"signature_delta","signature":""}}` | |
| signatureDelta, _ = sjson.Set(signatureDelta, "index", params.BlockIndex) | |
| signatureDelta, _ = sjson.Set(signatureDelta, "delta.signature", params.ThinkingSignature) | |
| output += "event: content_block_delta\n" | |
| output += fmt.Sprintf("data: %s\n\n", signatureDelta) | |
| } |
| if params.ThinkingBlockOpen { | ||
| switch rootResult.Get("type").String() { | ||
| case "response.content_part.added", "response.completed": | ||
| output += finalizeCodexThinkingBlock(params) | ||
| } | ||
| } |
There was a problem hiding this comment.
The pre-switch finalization closes an open thinking block on response.content_part.added / response.completed regardless of whether the thinking block has actually finished (ThinkingStopPending) and regardless of whether a signature has arrived. If encrypted_content arrives later via response.output_item.{added|done} (which this PR is trying to preserve), this early close will prevent emitting the correct signature_delta because the block is already stopped and state has been reset. Consider only auto-finalizing once ThinkingStopPending is true, or buffering the subsequent event until the signature is known (or deciding explicitly that late signatures are unsupported).
| for _, line := range lines { | ||
| if !strings.HasPrefix(line, "data: ") { | ||
| continue | ||
| } | ||
| data := gjson.Parse(strings.TrimPrefix(line, "data: ")) | ||
| if data.Get("type").String() == "content_block_start" && data.Get("content_block.type").String() == "thinking" { | ||
| if !data.Get("content_block.signature").Exists() { | ||
| t.Fatalf("thinking start block missing signature field: %s", line) | ||
| } | ||
| return | ||
| } | ||
| } | ||
|
|
||
| t.Fatal("expected thinking content_block_start event") |
There was a problem hiding this comment.
The streaming test for the no-reasoning-item case only asserts that the thinking start event includes a signature field, but it doesn't assert what happens when the thinking block is closed (e.g., whether content_block_stop is emitted and whether signature_delta is omitted when no encrypted_content exists). Adding assertions around the close sequence would help prevent regressions, especially if signature_delta emission becomes conditional.
| for _, line := range lines { | |
| if !strings.HasPrefix(line, "data: ") { | |
| continue | |
| } | |
| data := gjson.Parse(strings.TrimPrefix(line, "data: ")) | |
| if data.Get("type").String() == "content_block_start" && data.Get("content_block.type").String() == "thinking" { | |
| if !data.Get("content_block.signature").Exists() { | |
| t.Fatalf("thinking start block missing signature field: %s", line) | |
| } | |
| return | |
| } | |
| } | |
| t.Fatal("expected thinking content_block_start event") | |
| var ( | |
| thinkingStartFound bool | |
| thinkingStopFound bool | |
| thinkingIndex int64 | |
| signatureDeltaFound bool | |
| ) | |
| for _, line := range lines { | |
| if !strings.HasPrefix(line, "data: ") { | |
| continue | |
| } | |
| data := gjson.Parse(strings.TrimPrefix(line, "data: ")) | |
| // Track thinking content_block_start and ensure it has a signature field. | |
| if data.Get("type").String() == "content_block_start" && data.Get("content_block.type").String() == "thinking" { | |
| if !data.Get("content_block.signature").Exists() { | |
| t.Fatalf("thinking start block missing signature field: %s", line) | |
| } | |
| thinkingStartFound = true | |
| thinkingIndex = data.Get("content_block.index").Int() | |
| } | |
| // Track corresponding content_block_stop for the same index. | |
| if data.Get("type").String() == "content_block_stop" { | |
| if data.Get("content_block.index").Int() == thinkingIndex { | |
| thinkingStopFound = true | |
| } | |
| } | |
| // Ensure no signature_delta is emitted when there is no encrypted_content. | |
| if data.Get("delta.signature_delta").Exists() || data.Get("signature_delta").Exists() { | |
| signatureDeltaFound = true | |
| } | |
| } | |
| if !thinkingStartFound { | |
| t.Fatal("expected thinking content_block_start event") | |
| } | |
| if !thinkingStopFound { | |
| t.Fatal("expected thinking content_block_stop event for the thinking content block") | |
| } | |
| if signatureDeltaFound { | |
| t.Fatal("did not expect signature_delta to be emitted when no encrypted_content exists") | |
| } |
|
Addressed the review feedback in follow-up commit
|
# Conflicts: # internal/translator/codex/claude/codex_claude_response.go
luispater
left a comment
luispater
left a comment
There was a problem hiding this comment.
From a translator-behavior perspective, this looks good to me.
I re-checked the thinking signature flow against the actual Codex event ordering here: response.output_item.added for the reasoning item already carries encrypted_content, then the reasoning summary starts streaming, and response.output_item.done carries encrypted_content again before the thinking block is finalized. With that event order, the updated state machine preserves the Claude thinking signature correctly.
Non-blocking suggestion: it would still be great to add a regression test for that exact event ordering so the assumption stays encoded in the test suite.
Requesting changes only because this PR currently has merge conflicts. Please rebase/merge the latest main and resolve the conflicts, then this should be good to re-review.
|
@luispater Conflicts have been resolved in 8bb1ede by merging the latest dev into this branch. The PR is mergeable again now, and checks are rerunning. Thanks for the review. |
Summary
signaturefieldencrypted_contentis available and emit it assignature_deltaTesting