fix: try to fix tool call issues - #2397
Conversation
WalkthroughStreamResponseOpenAI2Claude in Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (2)📓 Common learnings📚 Learning: 2025-08-21T06:31:11.073ZApplied to files:
🔇 Additional comments (5)
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}
There was a problem hiding this comment.
Actionable comments posted: 2
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)
225-225: Remove no-op append statement.This line appends nothing to
claudeResponsesand serves no purpose. It appears to be leftover code from development.Apply this diff to remove it:
claudeResponses = append(claudeResponses, &dto.ClaudeResponse{ Type: "message_start", Message: msg, }) - claudeResponses = append(claudeResponses) //claudeResponses = append(claudeResponses, &dto.ClaudeResponse{ // Type: "ping", //})
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
service/convert.go(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
service/convert.go (6)
relay/common/relay_info.go (4)
ClaudeConvertInfo(33-39)LastMessageTypeThinking(30-30)LastMessageTypeText(28-28)LastMessageTypeTools(29-29)relay/helper/common.go (1)
Done(93-95)dto/openai_response.go (2)
ToolCallResponse(121-127)Usage(222-240)dto/claude.go (4)
ClaudeResponse(450-464)ClaudeMediaMessage(18-37)Thinking(411-414)ClaudeUsage(515-525)dto/openai_request.go (1)
Input(900-904)common/utils.go (1)
GetPointer(292-294)
🔇 Additional comments (5)
service/convert.go (5)
204-206: LGTM: Early exit prevents duplicate processing.The early return when conversion is already done prevents duplicate message processing and ensures clean state management.
268-309: LGTM: Proper handling of reasoning and text content in first chunk.The logic correctly differentiates between reasoning (thinking) and text content, creates appropriate content blocks, and updates the state tracker. This addresses the issue where tool calls required "thinking" to be enabled.
311-337: LGTM: Handles single-chunk completion correctly.Properly handles the case where a response completes in the first chunk by emitting stop blocks, usage information, and setting the
Doneflag to prevent further processing.
416-458: LGTM: State transitions handled correctly.The logic properly handles state transitions between thinking, text, and tool content types, including generating appropriate stop blocks and incrementing indices when switching content types.
466-490: LGTM: Finish handling is correct.Properly handles completion by generating stop blocks, reporting usage statistics with appropriate fallback, and setting the
Doneflag to prevent further processing.
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 (2)
service/convert.go (2)
225-225: Remove no-op statement.
append(claudeResponses)with no additional arguments is a no-op that does nothing.- claudeResponses = append(claudeResponses)
341-365: RedundantDonecheck - this branch is unreachable.The condition
info.ClaudeConvertInfo.Doneat line 344 will never be true because the function already returns at lines 204-206 whenDoneis true. This entire branch is dead code.Consider whether this was intended as a fallback. If not, simplify by removing the dead branch:
if len(openAIResponse.Choices) == 0 { // no choices - // 可能为非标准的 OpenAI 响应，判断是否已经完成 - if info.ClaudeConvertInfo.Done { - claudeResponses = append(claudeResponses, generateStopBlock(info.ClaudeConvertInfo.Index)) - oaiUsage := info.ClaudeConvertInfo.Usage - if oaiUsage != nil { - claudeResponses = append(claudeResponses, &dto.ClaudeResponse{ - Type: "message_delta", - Usage: &dto.ClaudeUsage{ - InputTokens: oaiUsage.PromptTokens, - OutputTokens: oaiUsage.CompletionTokens, - CacheCreationInputTokens: oaiUsage.PromptTokensDetails.CachedCreationTokens, - CacheReadInputTokens: oaiUsage.PromptTokensDetails.CachedTokens, - }, - Delta: &dto.ClaudeMediaMessage{ - StopReason: common.GetPointer[string](stopReasonOpenAI2Claude(info.FinishReason)), - }, - }) - } - claudeResponses = append(claudeResponses, &dto.ClaudeResponse{ - Type: "message_stop", - }) - } + // Empty choices with no content - skip return claudeResponses
♻️ Duplicate comments (2)
service/convert.go (2)
231-263: Validate tool call data before creating content blocks.This issue was flagged in a previous review but remains unaddressed. When
GetFirstToolCall()returnsnil, the code creates an emptyToolCallResponse{}(line 239), resulting in atool_usecontent block with emptyIdandNamefields. This may cause downstream issues when Claude clients process these invalid blocks.Apply this diff to add validation:
if openAIResponse.IsToolCall() { info.ClaudeConvertInfo.LastMessagesType = relaycommon.LastMessageTypeTools var toolCall dto.ToolCallResponse + var hasValidToolCall bool if len(openAIResponse.Choices) > 0 && len(openAIResponse.Choices[0].Delta.ToolCalls) > 0 { toolCall = openAIResponse.Choices[0].Delta.ToolCalls[0] + hasValidToolCall = true } else { first := openAIResponse.GetFirstToolCall() if first != nil { toolCall = *first + hasValidToolCall = true - } else { - toolCall = dto.ToolCallResponse{} } } + if hasValidToolCall { resp := &dto.ClaudeResponse{ Type: "content_block_start", ContentBlock: &dto.ClaudeMediaMessage{ Id: toolCall.ID, Type: "tool_use", Name: toolCall.Function.Name, Input: map[string]interface{}{}, }, } resp.SetIndex(0) claudeResponses = append(claudeResponses, resp) // 首块包含工具 delta，则追加 input_json_delta if toolCall.Function.Arguments != "" { claudeResponses = append(claudeResponses, &dto.ClaudeResponse{ Index: &info.ClaudeConvertInfo.Index, Type: "content_block_delta", Delta: &dto.ClaudeMediaMessage{ Type: "input_json_delta", PartialJson: &toolCall.Function.Arguments, }, }) } + } } else {
384-414: Index not incremented after tool call loop.After processing multiple tool calls,
info.ClaudeConvertInfo.Indexis set toblockIndexinside the loop (line 413), leaving it at the last tool call's index. The next content block should start at the next available index.For example: if three tool calls are processed at indices 0, 1, 2, the next non-tool block should use index 3, not 2.
Move the index update outside the loop and increment it:
for i, toolCall := range toolCalls { blockIndex := info.ClaudeConvertInfo.Index if toolCall.Index != nil { blockIndex = *toolCall.Index } else if len(toolCalls) > 1 { blockIndex = info.ClaudeConvertInfo.Index + i } idx := blockIndex claudeResponses = append(claudeResponses, &dto.ClaudeResponse{ Index: &idx, Type: "content_block_start", ContentBlock: &dto.ClaudeMediaMessage{ Id: toolCall.ID, Type: "tool_use", Name: toolCall.Function.Name, Input: map[string]interface{}{}, }, }) claudeResponses = append(claudeResponses, &dto.ClaudeResponse{ Index: &idx, Type: "content_block_delta", Delta: &dto.ClaudeMediaMessage{ Type: "input_json_delta", PartialJson: &toolCall.Function.Arguments, }, }) - - info.ClaudeConvertInfo.Index = blockIndex } + // Update index to point to the next available slot after all tool calls + if len(toolCalls) > 0 { + lastToolCall := toolCalls[len(toolCalls)-1] + if lastToolCall.Index != nil { + info.ClaudeConvertInfo.Index = *lastToolCall.Index + 1 + } else { + info.ClaudeConvertInfo.Index = info.ClaudeConvertInfo.Index + len(toolCalls) + } + }
🧹 Nitpick comments (1)
service/convert.go (1)
264-266: Remove empty else block.This empty else block serves no purpose and should be removed.
- } else { - }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
service/convert.go(4 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: jiajunly
Repo: QuantumNous/new-api PR: 1629
File: relay/channel/openai/relay-openai.go:170-174
Timestamp: 2025-08-21T06:31:11.073Z
Learning: In relay/channel/openai/relay-openai.go, the streaming logic for the AddThinkFirst feature is designed so that only the first chunk of a stream gets the "<think>\n" prefix. The final flush in the streaming handler intentionally uses addThink=false because the last chunk should never receive the prefix, even in single-chunk streams where the prefix would have been applied during normal processing.
🔇 Additional comments (2)
service/convert.go (2)
204-206: LGTM! Early exit guard prevents duplicate processing.This correctly short-circuits processing when the stream conversion has already completed.
466-491: Final stop block emission logic is correct.The handling of finish reason and stop block emission is well-structured. Setting
info.ClaudeConvertInfo.Done = trueafter emitting the stop blocks ensures the early exit guard will prevent duplicate processing.Note: The
info.ClaudeConvertInfo.Donecondition at line 466 is redundant due to the early exit at lines 204-206, but it's harmless.
a1007a7 to
1cb2b6f
Compare
fix: try to fix tool call issues
2 participants
fix #2292
同时处理工具调用要开thinking才能使用的问题
Summary by CodeRabbit
Bug Fixes
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.