Fix reasoning adaptor for openrouter - #1577
Conversation
WalkthroughAdds clearing of internal reasoning fields and extends thinking support: ConvertOpenAIRequest now clears Changes
Sequence Diagram(s)sequenceDiagram
participant Relay as Relay
participant OpenRouter as OpenRouterRequest
Relay->>Relay: Receive ConvertOpenAIRequest(request, info)
alt thinking_suffix path
Relay->>Relay: Handle thinking suffix
Relay->>Relay: Convert ReasoningEffort -> Reasoning
Relay->>Relay: request.ReasoningEffort = ""
else non-thinking path
Relay->>Relay: Possibly marshal ReasoningEffort -> Reasoning
Relay->>Relay: request.ReasoningEffort = ""
end
alt Anthropic upstream & THINKING present
Relay->>Relay: Unmarshal request.THINKING -> dto.Thinking
alt thinking.Type == "enabled"
Relay->>Relay: require BudgetTokens present
Relay->>Relay: Create RequestReasoning{MaxTokens: BudgetTokens}
Relay->>Relay: Marshal -> request.Reasoning
end
Relay->>Relay: request.THINKING = nil
end
Relay->>OpenRouter: Send converted request (with Reasoning cleared/updated)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15 minutes Possibly related PRs
Poem
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
relay/channel/openai/adaptor.go (1)
259-259: Same concern as above: this clear can be re-introduced later. Consider relying on a single final guard.This clear is good, but it can still be undone by the later o*/gpt-5 model-suffix parsing block. If you adopt the final guard suggested above, these earlier per-branch clears become optional noise. Either keep both (belt-and-suspenders) or remove the earlier ones for simplicity.
| // 清空多余的ReasoningEffort | ||
| request.ReasoningEffort = "" | ||
| } else { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Clearing ReasoningEffort here is correct but can be undone later in this function. Add a final guard.
Nice catch clearing ReasoningEffort for the -thinking OpenRouter path. However, a few lines below (Lines 262-287), the code may re-populate request.ReasoningEffort when parsing model-suffix-based effort for o*/gpt-5 models. This can leak ReasoningEffort back into the final request for OpenRouter and contradict the PR’s goal.
Add a finalization step after the suffix-parsing block to:
- If ChannelType is OpenRouter, move any residual ReasoningEffort into request.Reasoning (if still empty) and then clear ReasoningEffort again to guarantee no residue.
Apply this diff near the end of the o*/gpt-5 block (just before returning), while keeping info.ReasoningEffort intact:
@@
- info.ReasoningEffort = request.ReasoningEffort
+ info.ReasoningEffort = request.ReasoningEffort
@@
- }
+ }
+
+ // Ensure OpenRouter never carries ReasoningEffort; fold it into Reasoning if needed.
+ if info.ChannelType == constant.ChannelTypeOpenRouter {
+ if len(request.Reasoning) == 0 && request.ReasoningEffort != "" && request.ReasoningEffort != "none" {
+ reasoning := map[string]any{
+ "enabled": true,
+ }
+ if request.ReasoningEffort != "none" {
+ reasoning["effort"] = request.ReasoningEffort
+ }
+ marshal, err := common.Marshal(reasoning)
+ if err != nil {
+ return nil, fmt.Errorf("error marshalling reasoning: %w", err)
+ }
+ request.Reasoning = marshal
+ }
+ request.ReasoningEffort = ""
+ }Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In relay/channel/openai/adaptor.go around lines 240-242 (and the o*/gpt-5
suffix-parsing block at ~262-287), there's a risk that ReasoningEffort cleared
earlier gets re-populated during suffix parsing; add a final guard just before
returning from the o*/gpt-5 handling: if ChannelType == OpenRouter then if
request.Reasoning is empty and request.ReasoningEffort is non-empty move
request.ReasoningEffort into request.Reasoning, then set request.ReasoningEffort
= "" to ensure no residual ReasoningEffort is sent to OpenRouter; do not modify
info.ReasoningEffort.
nekohy
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
relay/channel/openai/adaptor.go (1)
311-318: Verify ReasoningEffort doesn't leak to OpenRouter after suffix parsingAfter parsing the model suffix and setting
request.ReasoningEffort = effort(line 313), this value is stored ininfo.ReasoningEffort(line 318) butrequest.ReasoningEffortremains populated. For OpenRouter requests, this could potentially send ReasoningEffort in the final request, which contradicts the earlier clearing logic.Consider adding a final guard to ensure ReasoningEffort is cleared for OpenRouter:
info.ReasoningEffort = request.ReasoningEffort + + // Ensure OpenRouter never carries ReasoningEffort + if info.ChannelType == constant.ChannelTypeOpenRouter { + request.ReasoningEffort = "" + }
♻️ Duplicate comments (1)
relay/channel/openai/adaptor.go (1)
240-242: Potential issue: ReasoningEffort may be re-populated laterWhile clearing
ReasoningEfforthere is correct for OpenRouter's -thinking suffix path, this can be undone later in the function. Lines 318 populateinfo.ReasoningEffort = request.ReasoningEffortwhen processing o*/gpt-5 models with suffix-based effort parsing (lines 311-316), which can re-populate the field.To verify if this is still an issue, let me check if there's any code that might send
ReasoningEffortto OpenRouter after this point:#!/bin/bash # Check if ReasoningEffort is used anywhere else in OpenRouter-related code rg -A 5 "ReasoningEffort" --glob "**/*openrouter*"
🧹 Nitpick comments (1)
relay/channel/openai/adaptor.go (1)
273-275: Consider more specific error message for missing BudgetTokensThe error message could be more informative by including the model name for easier debugging.
- return nil, fmt.Errorf("BudgetTokens is nil when thinking is enabled") + return nil, fmt.Errorf("BudgetTokens is required when thinking is enabled for model %s", info.UpstreamModelName)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
relay/channel/openai/adaptor.go(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-06-21T03:37:41.726Z
Learnt from: 9Ninety
PR: QuantumNous/new-api#1273
File: relay/channel/gemini/relay-gemini.go:97-116
Timestamp: 2025-06-21T03:37:41.726Z
Learning: In relay/channel/gemini/relay-gemini.go, the thinking budget calculation logic (including the MaxOutputTokens multiplication) was introduced in PR #1247. PR #1273 focused specifically on decoupling the thoughts summary feature from thinking budget settings and did not modify the existing thinking budget behavior.
Applied to files:
relay/channel/openai/adaptor.go
🧬 Code Graph Analysis (1)
relay/channel/openai/adaptor.go (4)
dto/claude.go (1)
Thinking(267-270)common/json.go (2)
Unmarshal(8-10)Marshal(20-22)relay/channel/openrouter/dto.go (1)
RequestReasoning(3-9)dto/openai_request.go (1)
Reasoning(648-651)
🔇 Additional comments (2)
relay/channel/openai/adaptor.go (2)
259-259: Good: Properly clearing ReasoningEffort for non-thinking OpenRouter requestsThis ensures that after converting ReasoningEffort to the OpenRouter-specific Reasoning format, the original field is cleared to prevent sending duplicate information.
262-291: Well-implemented Anthropic thinking support for OpenRouterThe implementation correctly:
- Checks for THINKING field and Anthropic model prefix
- Unmarshals to the proper dto.Thinking structure
- Validates that BudgetTokens is not nil when thinking is enabled
- Converts to OpenRouter's RequestReasoning format with MaxTokens
- Clears the THINKING field after conversion
The error handling is comprehensive with clear error messages for debugging.
…for-openrouter Fix reasoning adaptor for openrouter
2 participants
Summary by CodeRabbit
New Features
Bug Fixes
Notes