fix: optimize Claude tool-use relay logic to prevent state reset and data loss#4204
Conversation
WalkthroughThis PR adds state tracking to prevent duplicate Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
There was a problem hiding this comment.
Pull request overview
This PR improves the OpenAI→Claude streaming conversion logic (Anthropic Messages SSE) to make tool-use relaying more robust, addressing repeated content_block_start, pointer aliasing during streaming deltas, and end-of-stream chunk handling.
Changes:
- Track per-index
content_block_start/content_block_stopemission to avoid duplicate starts and repeated stops. - Fix pointer-aliasing risks by copying streamed delta strings before taking addresses.
- Reorder finish/usage handling to avoid dropping the final content chunk when
finish_reasonarrives.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| service/convert.go | Adds start/stop tracking for content blocks, fixes partial-json pointer aliasing, adjusts finish/usage closing logic. |
| relay/common/relay_info.go | Extends ClaudeConvertInfo with maps used to track emitted block start/stop events. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| blockIndex := base + offset | ||
|
|
||
| idx := blockIndex | ||
| if toolCall.Function.Name != "" { | ||
| if toolCall.Function.Name != "" && !info.ClaudeConvertInfo.ContentBlockStartSent[idx] { | ||
| claudeResponses = append(claudeResponses, &dto.ClaudeResponse{ | ||
| Index: &idx, | ||
| Type: "content_block_start", |
There was a problem hiding this comment.
In the for i, toolCall := range toolCalls loop, idx := blockIndex is a single variable reused across iterations (scope is the loop body). Taking &idx means all appended responses can end up sharing the same pointer and ultimately the last index value. Use common.GetPointer(idx) / create a per-iteration copy (e.g., idxCopy := idx) before taking the address.
| claudeResponses = append(claudeResponses, &dto.ClaudeResponse{ | ||
| Type: "message_stop", | ||
| }) | ||
| info.ClaudeConvertInfo.Done = true | ||
| } |
There was a problem hiding this comment.
message_stop (and setting Done=true) is currently emitted only when oaiUsage != nil. If the upstream sends finish_reason but never sends usage (or usage was never captured), the Claude stream may never be closed. Emit message_stop unconditionally on doneChunk/Done, and keep the message_delta usage payload conditional.
| claudeResponses = append(claudeResponses, &dto.ClaudeResponse{ | |
| Type: "message_stop", | |
| }) | |
| info.ClaudeConvertInfo.Done = true | |
| } | |
| } | |
| claudeResponses = append(claudeResponses, &dto.ClaudeResponse{ | |
| Type: "message_stop", | |
| }) | |
| info.ClaudeConvertInfo.Done = true |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
service/convert.go (1)
612-633:⚠️ Potential issue | 🟠 MajorEmit
message_stopindependently of usage.This branch still closes the Claude message only when
oaiUsage != nil. ButGenRelayInfoClaudesetsShouldIncludeUsage = false, so normal multi-chunk Claude relays can hitdoneChunkwith no usage payload at all. In that case we sendcontent_block_stopand then return withoutmessage_stop, which leaves downstream Anthropic clients hanging on an unterminated stream.
If you still want to forward a later usage-only chunk when one exists, that needs separate pending-finalization state; it shouldn't be the condition that decides whether the message is closed.Suggested direction
if doneChunk || info.ClaudeConvertInfo.Done { oaiUsage := openAIResponse.Usage if oaiUsage == nil { oaiUsage = info.ClaudeConvertInfo.Usage } if oaiUsage != nil { claudeResponses = append(claudeResponses, &dto.ClaudeResponse{ Type: "message_delta", Usage: buildClaudeUsageFromOpenAIUsage(oaiUsage), Delta: &dto.ClaudeMediaMessage{ StopReason: common.GetPointer[string](stopReasonOpenAI2Claude(info.FinishReason)), }, }) - claudeResponses = append(claudeResponses, &dto.ClaudeResponse{ - Type: "message_stop", - }) - info.ClaudeConvertInfo.Done = true } + if doneChunk { + claudeResponses = append(claudeResponses, &dto.ClaudeResponse{ + Type: "message_stop", + }) + info.ClaudeConvertInfo.Done = true + } return claudeResponses }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@service/convert.go` around lines 612 - 633, Change the branch that handles doneChunk / info.ClaudeConvertInfo.Done so that message termination ("message_stop") is emitted unconditionally when doneChunk is true (do not gate it on oaiUsage != nil); still append a "message_delta" with Usage when either openAIResponse.Usage or info.ClaudeConvertInfo.Usage is present (use buildClaudeUsageFromOpenAIUsage and prefer openAIResponse.Usage if non-nil), and append the StopReason delta (dto.ClaudeMediaMessage with stopReasonOpenAI2Claude) as before; ensure you set info.ClaudeConvertInfo.Done once the stop has been emitted to avoid duplicates so downstream Anthropic clients always receive a terminating "message_stop" even if ShouldIncludeUsage is false.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@service/convert.go`:
- Around line 612-633: Change the branch that handles doneChunk /
info.ClaudeConvertInfo.Done so that message termination ("message_stop") is
emitted unconditionally when doneChunk is true (do not gate it on oaiUsage !=
nil); still append a "message_delta" with Usage when either openAIResponse.Usage
or info.ClaudeConvertInfo.Usage is present (use buildClaudeUsageFromOpenAIUsage
and prefer openAIResponse.Usage if non-nil), and append the StopReason delta
(dto.ClaudeMediaMessage with stopReasonOpenAI2Claude) as before; ensure you set
info.ClaudeConvertInfo.Done once the stop has been emitted to avoid duplicates
so downstream Anthropic clients always receive a terminating "message_stop" even
if ShouldIncludeUsage is false.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c3a2fa9c-07db-473c-82b1-45ba3a9ee292
📒 Files selected for processing (2)
relay/common/relay_info.goservice/convert.go
📝 变更描述
優化 Claude 協議 (Anthropic API) 下的工具調用 (tool-use) 轉發邏輯，解決以下問題：
ContentBlockStartSent與ContentBlockStopSent追蹤狀態，確保content_block_start事件在整個流中只發送一次。這解決了因上游重複發送工具名導致客戶端input被重置為{}的問題。tool_calls循環時使用局部變量副本，確保PartialJson指向正確的地址。StreamResponseOpenAI2Claude邏輯，確保在finish_reason到達時先處理當前 Chunk 的內容數據，再處理usage和關閉流。🚀 变更类型
🔗 关联任务
✅ 提交前检查项
📸 运行证明
使用
moonshotai/kimi-k2.5測試輸出 SSE 片段：Summary by CodeRabbit
Release Notes