-
-
Notifications
You must be signed in to change notification settings - Fork 630
fix(claude): avoid reusing content_block indexes in Codex SSE #538
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
base: dev
Are you sure you want to change the base?
fix(claude): avoid reusing content_block indexes in Codex SSE #538
Conversation
Summary of ChangesHello @fuguiKz, 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 resolves a critical bug in the translation of OpenAI/Codex streaming responses to Claude-compatible Server-Sent Events (SSE). Previously, the system could incorrectly reuse content block indexes across different types of content (like thinking, text, or tool use), leading to client-side errors. The fix introduces a sophisticated state management system to ensure each unique content block type receives its own sequential index, guaranteeing proper event sequencing and preventing type mismatches in the Claude streaming format. This significantly enhances the robustness and reliability of the streaming conversion process. Highlights
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
|
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.
Code Review
This pull request effectively resolves a critical issue with content block index reuse in the Codex-to-Claude SSE translator, which was causing client-side errors. The introduction of a dedicated state management struct (ConvertCodexResponseToClaudeParams) to generate unique, sequential indices for each logical block is a robust solution. I appreciate the added resilience, such as ensuring content_block_start is emitted before deltas and closing all open blocks on completion. The new regression test is well-written and crucial for preventing similar issues in the future. The overall changes significantly improve the translator's correctness and reliability. I have one suggestion to refactor a small piece of duplicated logic to enhance maintainability.
| if blockKey == "" { | ||
| blockKey = fmt.Sprintf("tool_use:%d", outputIndex) | ||
| } |
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.
The logic to get a blockKey for a tool call, with a fallback for when an added event hasn't been seen, is duplicated in the handler for response.function_call_arguments.delta at lines 300-305.
This duplication can make maintenance harder. Additionally, the current implementation is inconsistent: the delta handler saves the fallback key to p.ToolKeyByOutputIndex, but this done handler does not. This could lead to subtle bugs if event ordering is unexpected.
For consistency, this handler should also save the fallback key. For example:
if blockKey == "" {
blockKey = fmt.Sprintf("tool_use:%d", outputIndex)
p.ToolKeyByOutputIndex[outputIndex] = blockKey // Add this line
}Furthermore, to improve maintainability and avoid code duplication, I recommend extracting this logic into a helper method on ConvertCodexResponseToClaudeParams and using it in both places.
|
FYI: this PR touches Per the workflow guidance, I opened issue #539 with repro + proposed fix and this patch for maintainers to apply: #539 If you want to cherry-pick from my fork: |
|
Please attach the failed request payload for our verification. |
|
Repro from Claude Code (VSCode) against CLIProxyAPI Codex→Claude Client error: Captured the exact failing Observation: upstream This appears to violate Claude SSE expectations (unique content_block.index per block) and triggers the client-side mismatch error. (Full payload + upstream reasoning_summary indices + outgoing snippet are included in the gist files.) |
What
When translating OpenAI/Codex streaming (
/v1/responses) into Claude streaming (/v1/messages), we previously reused Codexoutput_indexas Claudecontent_block.index.If a single
output_indexproduces multiple block types (e.g.thinking+text+tool_use), this causes index/type collisions and some Claude clients fail with errors like:Mismatched content block type content_block_delta thinkingFix
content_block.indexvalues per logical block key (per stream).content_block_startis emitted before anycontent_block_deltafor that block.message_delta/message_stopon completion.Tests
(Notes:
go test ./internal/translator/codex/claudepasses;go test ./...currently has unrelated failures upstream.)