Merged
Conversation
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review infoConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
WalkthroughAdds per-tool streaming state tracking to Claude→OpenAI conversion: StreamResponseClaude2OpenAI now accepts a ClaudeResponseInfo pointer, records and tracks ToolCallStreamStates across content_block_start/delta/stop, emits function tool_call events only for non-empty PartialJson deltas, and emits fallback empty-args tool calls at stop when needed. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
relay/channel/claude/relay-claude.go (1)
785-792:⚠️ Potential issue | 🟠 MajorGuard nil chunks before writing SSE data
At Line 785,
StreamResponseClaude2OpenAIcan legitimately returnnil(for example, emptyinput_json_deltaon Line 463-469), but Line 791 still writesresponse. Please short-circuit whenresponse == nilto avoid emitting invalid/no-op payloads.Suggested patch
response := StreamResponseClaude2OpenAI(&claudeResponse, claudeInfo) if !FormatClaudeResponseInfo(&claudeResponse, response, claudeInfo) { return nil } + if response == nil { + return nil + } err = helper.ObjectData(c, response)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@relay/channel/claude/relay-claude.go` around lines 785 - 792, StreamResponseClaude2OpenAI may return nil (e.g., empty input_json_delta), so after calling response := StreamResponseClaude2OpenAI(&claudeResponse, claudeInfo) short-circuit if response == nil to avoid passing a nil payload into FormatClaudeResponseInfo and helper.ObjectData; add a nil check (if response == nil { return nil }) immediately after that call so the function returns early and does not emit invalid SSE data.
🧹 Nitpick comments (1)
relay/channel/claude/relay_claude_test.go (1)
219-232: Add handler-level regression for ignored empty JSON deltasThis test verifies converter output (
nil), but not stream write behavior. Please add aHandleStreamResponseData-level test to assert no chunk is written when converter returnsnil.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@relay/channel/claude/relay_claude_test.go` around lines 219 - 232, Add a handler-level test that ensures HandleStreamResponseData does not write any chunks when the converter (StreamResponseClaude2OpenAI) returns nil for an input_json_delta with empty PartialJson: create a ClaudeResponse like in TestStreamResponseClaude2OpenAI_EmptyInputJSONDeltaIgnored, pass it into HandleStreamResponseData using a mock/io.Writer or an in-memory buffer that records writes, and assert the writer received no data; reference the existing helper StreamResponseClaude2OpenAI and call the top-level handler HandleStreamResponseData (or the exported handler function used in production) to validate end-to-end write behavior rather than just converter return value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@relay/channel/claude/relay-claude.go`:
- Around line 439-444: The current keying into claudeInfo.ToolCallStreamStates
uses the normalized fcIdx (derived via max(0, Index-1)), which collapses
zero/one indexes and causes state collisions; update the code to use the raw
Anthropic index (e.g., claudeResponse.ContentBlock.Index) as the map key instead
of fcIdx, or alternatively add a field (e.g., RawIndex) to ToolCallStreamState
and store the raw index there while mapping by the raw index, ensuring
ToolCallStreamStates, ToolCallStreamState, claudeInfo, and
claudeResponse.ContentBlock.Id/Name are updated accordingly so each tool_use
block keeps a unique state.
---
Outside diff comments:
In `@relay/channel/claude/relay-claude.go`:
- Around line 785-792: StreamResponseClaude2OpenAI may return nil (e.g., empty
input_json_delta), so after calling response :=
StreamResponseClaude2OpenAI(&claudeResponse, claudeInfo) short-circuit if
response == nil to avoid passing a nil payload into FormatClaudeResponseInfo and
helper.ObjectData; add a nil check (if response == nil { return nil })
immediately after that call so the function returns early and does not emit
invalid SSE data.
---
Nitpick comments:
In `@relay/channel/claude/relay_claude_test.go`:
- Around line 219-232: Add a handler-level test that ensures
HandleStreamResponseData does not write any chunks when the converter
(StreamResponseClaude2OpenAI) returns nil for an input_json_delta with empty
PartialJson: create a ClaudeResponse like in
TestStreamResponseClaude2OpenAI_EmptyInputJSONDeltaIgnored, pass it into
HandleStreamResponseData using a mock/io.Writer or an in-memory buffer that
records writes, and assert the writer received no data; reference the existing
helper StreamResponseClaude2OpenAI and call the top-level handler
HandleStreamResponseData (or the exported handler function used in production)
to validate end-to-end write behavior rather than just converter return value.
Comment on lines
+439
to
+444
| if claudeInfo != nil { | ||
| claudeInfo.ToolCallStreamStates[fcIdx] = &ToolCallStreamState{ | ||
| ID: claudeResponse.ContentBlock.Id, | ||
| Name: claudeResponse.ContentBlock.Name, | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Key tool-call index and state keying collision on normalized index
ToolCallStreamStates is keyed by fcIdx, which is derived from the upstream Index field using floor-to-zero normalization (line 418-421: approximately fcIdx = max(0, Index - 1)). Since Anthropic's content_block.index is zero-based and can emit sequential indexes (0, 1, 2...) for multiple tool_use blocks in a single response, the normalization causes a collision: both Index 0 and Index 1 map to fcIdx 0, causing the state for the first block to be overwritten. This breaks stop emission, which may use incorrect tool metadata.
Use raw Index directly as the key for ToolCallStreamStates, or if normalization is required for another reason, store the raw index separately in ToolCallStreamState for use during finalization.
Affects: lines 439-444 and 494-499
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@relay/channel/claude/relay-claude.go` around lines 439 - 444, The current
keying into claudeInfo.ToolCallStreamStates uses the normalized fcIdx (derived
via max(0, Index-1)), which collapses zero/one indexes and causes state
collisions; update the code to use the raw Anthropic index (e.g.,
claudeResponse.ContentBlock.Index) as the map key instead of fcIdx, or
alternatively add a field (e.g., RawIndex) to ToolCallStreamState and store the
raw index there while mapping by the raw index, ensuring ToolCallStreamStates,
ToolCallStreamState, claudeInfo, and claudeResponse.ContentBlock.Id/Name are
updated accordingly so each tool_use block keeps a unique state.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
relay/channel/claude/relay-claude.go (1)
439-443:⚠️ Potential issue | 🟠 MajorUse a non-colliding key for
ToolCallStreamStates.
ToolCallStreamStatesis keyed byfcIdx, butfcIdxis normalized (see Line 418+), so different upstream indexes can collapse to the same key and overwrite tool state. That can attach wrong ID/Name or break stop emission/cleanup.🛠️ Minimal fix (separate state key from output index)
func StreamResponseClaude2OpenAI(claudeResponse *dto.ClaudeResponse, claudeInfo *ClaudeResponseInfo) *dto.ChatCompletionsStreamResponse { @@ fcIdx := 0 + stateKey := 0 if claudeResponse.Index != nil { + stateKey = *claudeResponse.Index fcIdx = *claudeResponse.Index - 1 if fcIdx < 0 { fcIdx = 0 } } @@ if claudeInfo != nil { - claudeInfo.ToolCallStreamStates[fcIdx] = &ToolCallStreamState{ + claudeInfo.ToolCallStreamStates[stateKey] = &ToolCallStreamState{ ID: claudeResponse.ContentBlock.Id, Name: claudeResponse.ContentBlock.Name, } return nil } @@ if claudeInfo != nil { - if state, ok := claudeInfo.ToolCallStreamStates[fcIdx]; ok { + if state, ok := claudeInfo.ToolCallStreamStates[stateKey]; ok { state.Emitted = true toolCall.ID = state.ID toolCall.Function.Name = state.Name } } @@ - state, ok := claudeInfo.ToolCallStreamStates[fcIdx] + state, ok := claudeInfo.ToolCallStreamStates[stateKey] if !ok { return nil } - delete(claudeInfo.ToolCallStreamStates, fcIdx) + delete(claudeInfo.ToolCallStreamStates, stateKey)Also applies to: 479-483, 498-503
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@relay/channel/claude/relay-claude.go` around lines 439 - 443, ToolCallStreamStates is currently keyed by the normalized output index variable fcIdx which can collide; change to use a non-colliding key (for example a composite key that includes the original/upstream tool call index plus the normalized fcIdx or an explicit unique ID) when setting and reading entries in ToolCallStreamStates so state entries cannot be overwritten by different upstream indexes. Update the assignment sites that write ToolCallStreamStates (the block that assigns &ToolCallStreamState using claudeResponse.ContentBlock.Id/Name near the current fcIdx usage) and the corresponding read/cleanup sites (also the similar blocks around the other occurrences you noted) to use the new unique key variable instead of fcIdx. Ensure all places that delete or check ToolCallStreamStates use the same composite key logic so lookups and removals stay consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@relay/channel/claude/relay-claude.go`:
- Around line 439-443: ToolCallStreamStates is currently keyed by the normalized
output index variable fcIdx which can collide; change to use a non-colliding key
(for example a composite key that includes the original/upstream tool call index
plus the normalized fcIdx or an explicit unique ID) when setting and reading
entries in ToolCallStreamStates so state entries cannot be overwritten by
different upstream indexes. Update the assignment sites that write
ToolCallStreamStates (the block that assigns &ToolCallStreamState using
claudeResponse.ContentBlock.Id/Name near the current fcIdx usage) and the
corresponding read/cleanup sites (also the similar blocks around the other
occurrences you noted) to use the new unique key variable instead of fcIdx.
Ensure all places that delete or check ToolCallStreamStates use the same
composite key logic so lookups and removals stay consistent.
seefs001
force-pushed
the
fix/aws-non-empty-text
branch
from
1b33df3 to
032a3ec
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Bug Fixes
Tests