feats: repair the thinking of claude to openrouter convert - #3120
Conversation
WalkthroughAdds effort extraction from Claude request OutputConfig and extends OpenRouter reasoning handling: new RequestReasoning.Enabled field, setting Enabled during conversion, support for "enabled" and "adaptive" thinking types, and mapping extracted effort to OpenRouter verbosity. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ConvertService as Convert Service
participant ClaudeDTO as Claude DTO
participant AdapterOpenAI as OpenAI Adapter
participant OpenRouterDTO as OpenRouter DTO
participant OpenRouterAPI as OpenRouter API
Client->>ConvertService: ClaudeToOpenAIRequest(claudeReq)
ConvertService->>ClaudeDTO: GetEfforts()
ClaudeDTO-->>ConvertService: effort string
alt thinking.Type == "enabled" or "adaptive"
ConvertService->>ConvertService: Build RequestReasoning (Enabled: true)
alt thinking.Type == "enabled" with budgets
ConvertService->>ConvertService: Set MaxTokens from budget
end
ConvertService->>ConvertService: Marshal reasoning JSON
end
alt effort non-empty
ConvertService->>ConvertService: Set Verbosity = effort
end
ConvertService->>AdapterOpenAI: Apply Reasoning/Verbosity
AdapterOpenAI-->>OpenRouterDTO: Request with Reasoning & Verbosity
OpenRouterDTO-->>OpenRouterAPI: Send
OpenRouterAPI-->>Client: Response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
relay/channel/openrouter/dto.go (1)
6-6: Makeenabledomittable to avoid forcing"enabled": falseby default.Line [6] currently serializes
enabled=falsefor zero-value structs. If this flag is optional, addomitempty(or use*bool) so it is only sent when explicitly set.Proposed fix
- Enabled bool `json:"enabled"` + Enabled bool `json:"enabled,omitempty"`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@relay/channel/openrouter/dto.go` at line 6, The Enabled field on the DTO currently uses a non-omitting bool which forces `"enabled": false` for zero-value structs; update the struct tag for Enabled (Enabled bool) to make it omittable by either adding `omitempty` to the json tag or changing the type to `*bool` so the field is only serialized when explicitly set—modify the Enabled field's definition in relay/channel/openrouter/dto.go accordingly and ensure callers handle the pointer or the omitted case if you choose `*bool`.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@dto/claude.go`:
- Around line 417-423: Replace the direct call to json.Unmarshal in GetEfforts
with the project's wrapper common.Unmarshal: call
common.Unmarshal(c.OutputConfig, &OutputConfig) inside ClaudeRequest.GetEfforts
(which uses the OutputConfigForEffort struct) and preserve the current behavior
of returning OutputConfig.Effort on success and "" on error; ensure the function
handles the returned error from common.Unmarshal the same way the current code
did (i.e., only return effort when err == nil).
In `@service/convert.go`:
- Around line 39-56: Replace direct calls to json.Marshal in the convert logic
with the common.Marshal wrapper and handle errors instead of ignoring them: when
setting openAIRequest.Verbosity from claudeRequest.GetEfforts() use
common.Marshal(effort) and check/return/log the error rather than using "_" to
discard it; likewise replace json.Marshal(reasoning) (the reasoningJSON and err
usage in the Thinking handling block) with common.Marshal(reasoning) and handle
the error branch consistently (propagate or log and return). Update references
around GetEfforts(), openAIRequest.Verbosity assignment, and the reasoningJSON
variable in the Thinking handling code to use common.Marshal and proper error
handling.
- Around line 45-49: When building openrouter.RequestReasoning in the block
where claudeRequest.Thinking.Type == "enabled", add an explicit nil-check for
claudeRequest.Thinking.BudgetTokens (don’t rely on GetBudgetTokens() returning
0); if BudgetTokens is nil return an error or reject the request consistent with
relay/channel/openai/adaptor.go behavior, otherwise use the dereferenced value
for MaxTokens and set Enabled=true on openrouter.RequestReasoning; update the
clause that currently uses claudeRequest.Thinking.GetBudgetTokens() to first
validate BudgetTokens != nil and only then construct the reasoning object.
---
Nitpick comments:
In `@relay/channel/openrouter/dto.go`:
- Line 6: The Enabled field on the DTO currently uses a non-omitting bool which
forces `"enabled": false` for zero-value structs; update the struct tag for
Enabled (Enabled bool) to make it omittable by either adding `omitempty` to the
json tag or changing the type to `*bool` so the field is only serialized when
explicitly set—modify the Enabled field's definition in
relay/channel/openrouter/dto.go accordingly and ensure callers handle the
pointer or the omitted case if you choose `*bool`.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8b0fa8b3-0414-4df5-a0dc-d78b5644c554
📒 Files selected for processing (4)
dto/claude.gorelay/channel/openai/adaptor.gorelay/channel/openrouter/dto.goservice/convert.go
| func (c *ClaudeRequest) GetEfforts() string { | ||
| var OutputConfig OutputConfigForEffort | ||
| if err := json.Unmarshal(c.OutputConfig, &OutputConfig); err == nil { | ||
| effort := OutputConfig.Effort | ||
| return effort | ||
| } | ||
| return "" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Read the target file to see the actual code at lines 417-423
cd /dev/shm && find . -type f -name "claude.go" -path "*/dto/*" 2>/dev/null | head -5Repository: QuantumNous/new-api
Length of output: 121
🏁 Script executed:
# List files in dto directory
ls -la dto/ 2>/dev/null | head -20Repository: QuantumNous/new-api
Length of output: 1351
🏁 Script executed:
# Check what files exist in dto
git ls-files dto/ | head -20Repository: QuantumNous/new-api
Length of output: 484
🏁 Script executed:
# Search for GetEfforts in the codebase
rg -n "GetEfforts" --type goRepository: QuantumNous/new-api
Length of output: 207
🏁 Script executed:
# Read the GetEfforts method from dto/claude.go with context
sed -n '410,430p' dto/claude.goRepository: QuantumNous/new-api
Length of output: 507
🏁 Script executed:
# Check imports at the top of dto/claude.go
head -50 dto/claude.goRepository: QuantumNous/new-api
Length of output: 1534
🏁 Script executed:
# Check what JSON wrapper functions are available in common/json.go
cat common/json.goRepository: QuantumNous/new-api
Length of output: 817
Use common.Unmarshal in GetEfforts instead of json.Unmarshal.
Line 419 directly calls json.Unmarshal in business code. This violates the project guideline requiring all JSON marshal/unmarshal operations to use wrapper functions from common/json.go.
Proposed fix
func (c *ClaudeRequest) GetEfforts() string {
- var OutputConfig OutputConfigForEffort
- if err := json.Unmarshal(c.OutputConfig, &OutputConfig); err == nil {
- effort := OutputConfig.Effort
- return effort
- }
- return ""
+ if len(c.OutputConfig) == 0 {
+ return ""
+ }
+ var outputConfig OutputConfigForEffort
+ if err := common.Unmarshal(c.OutputConfig, &outputConfig); err != nil {
+ return ""
+ }
+ return outputConfig.Effort
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (c *ClaudeRequest) GetEfforts() string { | |
| var OutputConfig OutputConfigForEffort | |
| if err := json.Unmarshal(c.OutputConfig, &OutputConfig); err == nil { | |
| effort := OutputConfig.Effort | |
| return effort | |
| } | |
| return "" | |
| func (c *ClaudeRequest) GetEfforts() string { | |
| if len(c.OutputConfig) == 0 { | |
| return "" | |
| } | |
| var outputConfig OutputConfigForEffort | |
| if err := common.Unmarshal(c.OutputConfig, &outputConfig); err != nil { | |
| return "" | |
| } | |
| return outputConfig.Effort | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@dto/claude.go` around lines 417 - 423, Replace the direct call to
json.Unmarshal in GetEfforts with the project's wrapper common.Unmarshal: call
common.Unmarshal(c.OutputConfig, &OutputConfig) inside ClaudeRequest.GetEfforts
(which uses the OutputConfigForEffort struct) and preserve the current behavior
of returning OutputConfig.Effort on success and "" on error; ensure the function
handles the returned error from common.Unmarshal the same way the current code
did (i.e., only return effort when err == nil).
| if effort := claudeRequest.GetEfforts(); effort != "" { | ||
| effortBytes, _ := json.Marshal(effort) | ||
| openAIRequest.Verbosity = effortBytes | ||
| } | ||
| reasoningJSON, err := json.Marshal(reasoning) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to marshal reasoning: %w", err) | ||
| if claudeRequest.Thinking != nil { | ||
| var reasoning openrouter.RequestReasoning | ||
| if claudeRequest.Thinking.Type == "enabled" { | ||
| reasoning = openrouter.RequestReasoning{ | ||
| Enabled: true, | ||
| MaxTokens: claudeRequest.Thinking.GetBudgetTokens(), | ||
| } | ||
| } else if claudeRequest.Thinking.Type == "adaptive" { | ||
| reasoning = openrouter.RequestReasoning{ | ||
| Enabled: true, | ||
| } | ||
| } | ||
| reasoningJSON, err := json.Marshal(reasoning) | ||
| if err != nil { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check if the file exists and read the relevant section
if [ -f "service/convert.go" ]; then
echo "=== Lines 35-60 of service/convert.go ==="
sed -n '35,60p' service/convert.go
else
echo "File not found: service/convert.go"
fiRepository: QuantumNous/new-api
Length of output: 1069
🏁 Script executed:
# Verify common.Marshal wrapper function exists
echo "=== Checking for common.Marshal in common/json.go ==="
if [ -f "common/json.go" ]; then
rg -A 5 "func (.*)?Marshal\(" common/json.go | head -40
else
echo "File common/json.go not found"
fiRepository: QuantumNous/new-api
Length of output: 263
🏁 Script executed:
# Check all json.Marshal and json.Unmarshal usage in service/convert.go
echo "=== All json.Marshal and json.Unmarshal calls in service/convert.go ==="
rg "(json\.Marshal|json\.Unmarshal)" service/convert.go -nRepository: QuantumNous/new-api
Length of output: 448
🏁 Script executed:
# Check for json imports in service/convert.go
echo "=== Imports in service/convert.go ==="
head -30 service/convert.go | grep -E "^import|^\\s+\".*\""Repository: QuantumNous/new-api
Length of output: 399
Replace direct json.Marshal calls with common.Marshal wrapper function.
Lines 40 and 55 use json.Marshal directly in business code instead of the common.Marshal wrapper function from common/json.go. Additionally, line 40 ignores the error return value with _, which should be properly handled.
Proposed fix
if effort := claudeRequest.GetEfforts(); effort != "" {
- effortBytes, _ := json.Marshal(effort)
+ effortBytes, err := common.Marshal(effort)
+ if err != nil {
+ return nil, fmt.Errorf("failed to marshal effort: %w", err)
+ }
openAIRequest.Verbosity = effortBytes
}- reasoningJSON, err := json.Marshal(reasoning)
+ reasoningJSON, err := common.Marshal(reasoning)
if err != nil {
return nil, fmt.Errorf("failed to marshal reasoning: %w", err)
}Per the coding guideline for **/*.go: "All JSON marshal/unmarshal operations MUST use wrapper functions in common/json.go (common.Marshal, common.Unmarshal, common.UnmarshalJsonStr, common.DecodeJson, common.GetJsonType). Do NOT directly call encoding/json in business code."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@service/convert.go` around lines 39 - 56, Replace direct calls to
json.Marshal in the convert logic with the common.Marshal wrapper and handle
errors instead of ignoring them: when setting openAIRequest.Verbosity from
claudeRequest.GetEfforts() use common.Marshal(effort) and check/return/log the
error rather than using "_" to discard it; likewise replace
json.Marshal(reasoning) (the reasoningJSON and err usage in the Thinking
handling block) with common.Marshal(reasoning) and handle the error branch
consistently (propagate or log and return). Update references around
GetEfforts(), openAIRequest.Verbosity assignment, and the reasoningJSON variable
in the Thinking handling code to use common.Marshal and proper error handling.
| if claudeRequest.Thinking.Type == "enabled" { | ||
| reasoning = openrouter.RequestReasoning{ | ||
| Enabled: true, | ||
| MaxTokens: claudeRequest.Thinking.GetBudgetTokens(), | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find and examine the service/convert.go file around lines 45-49
fd convert.go | grep -E "service|convert"Repository: QuantumNous/new-api
Length of output: 81
🏁 Script executed:
# Get the actual code at lines 45-49 in service/convert.go
find . -path "*service/convert.go" -type f | head -1 | xargs head -60 | tail -20Repository: QuantumNous/new-api
Length of output: 709
🏁 Script executed:
# Search for GetBudgetTokens method implementation
rg "GetBudgetTokens" -A 5 -B 2Repository: QuantumNous/new-api
Length of output: 710
🏁 Script executed:
# Look at relay/channel/openai/adaptor.go around lines 296-298
find . -path "*relay/channel/openai/adaptor.go" -type f | xargs sed -n '290,305p'Repository: QuantumNous/new-api
Length of output: 521
🏁 Script executed:
# Search for the Thinking struct definition to understand BudgetTokens type
rg "type.*Thinking\s*struct" -A 20 | head -50Repository: QuantumNous/new-api
Length of output: 2180
Add nil validation for BudgetTokens when thinking.type == "enabled".
At Lines [45-49], GetBudgetTokens() silently returns 0 when BudgetTokens is nil, allowing enabled reasoning without an explicit token budget. This diverges from the stricter validation in relay/channel/openai/adaptor.go (Lines [296-298]), which explicitly rejects nil BudgetTokens when thinking is enabled. For consistency and correctness, validate BudgetTokens != nil before building reasoning.
Proposed fix
if claudeRequest.Thinking.Type == "enabled" {
+ if claudeRequest.Thinking.BudgetTokens == nil {
+ return nil, fmt.Errorf("BudgetTokens is nil when thinking is enabled")
+ }
reasoning = openrouter.RequestReasoning{
Enabled: true,
MaxTokens: claudeRequest.Thinking.GetBudgetTokens(),
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if claudeRequest.Thinking.Type == "enabled" { | |
| reasoning = openrouter.RequestReasoning{ | |
| Enabled: true, | |
| MaxTokens: claudeRequest.Thinking.GetBudgetTokens(), | |
| } | |
| if claudeRequest.Thinking.Type == "enabled" { | |
| if claudeRequest.Thinking.BudgetTokens == nil { | |
| return nil, fmt.Errorf("BudgetTokens is nil when thinking is enabled") | |
| } | |
| reasoning = openrouter.RequestReasoning{ | |
| Enabled: true, | |
| MaxTokens: claudeRequest.Thinking.GetBudgetTokens(), | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@service/convert.go` around lines 45 - 49, When building
openrouter.RequestReasoning in the block where claudeRequest.Thinking.Type ==
"enabled", add an explicit nil-check for claudeRequest.Thinking.BudgetTokens
(don’t rely on GetBudgetTokens() returning 0); if BudgetTokens is nil return an
error or reject the request consistent with relay/channel/openai/adaptor.go
behavior, otherwise use the dereferenced value for MaxTokens and set
Enabled=true on openrouter.RequestReasoning; update the clause that currently
uses claudeRequest.Thinking.GetBudgetTokens() to first validate BudgetTokens !=
nil and only then construct the reasoning object.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
service/convert.go (2)
44-48:⚠️ Potential issue | 🟠 MajorValidate budget tokens explicitly for
thinking.type == "enabled".Line [47] uses
GetBudgetTokens()without checking whetherBudgetTokensis present. If omitted, this can silently produceMaxTokens: 0instead of rejecting an invalid enabled-thinking request.Proposed fix
if claudeRequest.Thinking.Type == "enabled" { + if claudeRequest.Thinking.BudgetTokens == nil { + return nil, fmt.Errorf("budget_tokens is required when thinking.type is enabled") + } reasoning = openrouter.RequestReasoning{ Enabled: true, MaxTokens: claudeRequest.Thinking.GetBudgetTokens(), }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@service/convert.go` around lines 44 - 48, When claudeRequest.Thinking.Type == "enabled" you must validate that BudgetTokens is provided and positive before using GetBudgetTokens(); update the logic around openrouter.RequestReasoning creation (the reasoning variable) to check claudeRequest.Thinking.BudgetTokens (or an equivalent presence flag) and that GetBudgetTokens() > 0, and if not return/propagate a validation error for the enabled-thinking request instead of silently setting MaxTokens to 0.
38-40:⚠️ Potential issue | 🟠 MajorUse
common.Marshal(and handle errors) in this conversion path.Line [39] and Line [54] call
json.Marshaldirectly; Line [39] also discards the error. This violates repo JSON handling rules and can mask malformed payload serialization.Proposed fix
if effort := claudeRequest.GetEfforts(); effort != "" { - effortBytes, _ := json.Marshal(effort) + effortBytes, err := common.Marshal(effort) + if err != nil { + return nil, fmt.Errorf("failed to marshal effort: %w", err) + } openAIRequest.Verbosity = effortBytes } @@ - reasoningJSON, err := json.Marshal(reasoning) + reasoningJSON, err := common.Marshal(reasoning) if err != nil { return nil, fmt.Errorf("failed to marshal reasoning: %w", err) }#!/bin/bash # Verify direct json.Marshal usage inside this file. rg -n '\bjson\.Marshal\(' service/convert.go sed -n '34,60p' service/convert.goAs per coding guidelines: "All JSON marshal/unmarshal operations MUST use wrapper functions in
common/json.go... Do NOT directly callencoding/jsonin business code."Also applies to: 54-56
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@service/convert.go` around lines 38 - 40, Replace direct calls to json.Marshal in the conversion path with the repo wrapper common.Marshal and properly handle errors: when converting claudeRequest.GetEfforts() for assignment to openAIRequest.Verbosity (and the other json.Marshal usage around lines where openAIRequest.SystemMessages/related fields are set), call common.Marshal(effort) and check the returned error; on error return or propagate a descriptive error from the conversion function (or log and return) instead of discarding it. Update the code paths that set openAIRequest.Verbosity and the other affected field(s) to only assign the marshaled bytes on success and ensure you reference the functions/fields claudeRequest.GetEfforts(), openAIRequest.Verbosity (and the other place using json.Marshal) when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@service/convert.go`:
- Around line 42-54: claudeRequest.Thinking may be non-nil with an unsupported
Type, leaving reasoning as a zero-value struct that still gets marshaled; update
the block around claudeRequest.Thinking to explicitly validate Type (e.g.,
switch on claudeRequest.Thinking.Type) and only construct and marshal
openrouter.RequestReasoning when Type is "enabled" or "adaptive"; for other
types either skip setting reasoning (do not call json.Marshal) or
return/propagate an error, ensuring you reference claudeRequest.Thinking,
openrouter.RequestReasoning, and the reasoningJSON/json.Marshal lines so the
marshal is only called when a valid reasoning value was created.
---
Duplicate comments:
In `@service/convert.go`:
- Around line 44-48: When claudeRequest.Thinking.Type == "enabled" you must
validate that BudgetTokens is provided and positive before using
GetBudgetTokens(); update the logic around openrouter.RequestReasoning creation
(the reasoning variable) to check claudeRequest.Thinking.BudgetTokens (or an
equivalent presence flag) and that GetBudgetTokens() > 0, and if not
return/propagate a validation error for the enabled-thinking request instead of
silently setting MaxTokens to 0.
- Around line 38-40: Replace direct calls to json.Marshal in the conversion path
with the repo wrapper common.Marshal and properly handle errors: when converting
claudeRequest.GetEfforts() for assignment to openAIRequest.Verbosity (and the
other json.Marshal usage around lines where openAIRequest.SystemMessages/related
fields are set), call common.Marshal(effort) and check the returned error; on
error return or propagate a descriptive error from the conversion function (or
log and return) instead of discarding it. Update the code paths that set
openAIRequest.Verbosity and the other affected field(s) to only assign the
marshaled bytes on success and ensure you reference the functions/fields
claudeRequest.GetEfforts(), openAIRequest.Verbosity (and the other place using
json.Marshal) when making the change.
| if claudeRequest.Thinking != nil { | ||
| var reasoning openrouter.RequestReasoning | ||
| if claudeRequest.Thinking.Type == "enabled" { | ||
| reasoning = openrouter.RequestReasoning{ | ||
| Enabled: true, | ||
| MaxTokens: claudeRequest.Thinking.GetBudgetTokens(), | ||
| } | ||
| } else if claudeRequest.Thinking.Type == "adaptive" { | ||
| reasoning = openrouter.RequestReasoning{ | ||
| Enabled: true, | ||
| } | ||
| } | ||
| reasoningJSON, err := json.Marshal(reasoning) |
There was a problem hiding this comment.
Guard unsupported thinking types before marshaling reasoning.
If Thinking is non-nil but Type is neither "enabled" nor "adaptive", reasoning remains zero-value and still gets marshaled/assigned. Prefer explicit validation (or skip assignment) to avoid emitting ambiguous {} reasoning payloads.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@service/convert.go` around lines 42 - 54, claudeRequest.Thinking may be
non-nil with an unsupported Type, leaving reasoning as a zero-value struct that
still gets marshaled; update the block around claudeRequest.Thinking to
explicitly validate Type (e.g., switch on claudeRequest.Thinking.Type) and only
construct and marshal openrouter.RequestReasoning when Type is "enabled" or
"adaptive"; for other types either skip setting reasoning (do not call
json.Marshal) or return/propagate an error, ensuring you reference
claudeRequest.Thinking, openrouter.RequestReasoning, and the
reasoningJSON/json.Marshal lines so the marshal is only called when a valid
reasoning value was created.
feats: repair the thinking of claude to openrouter convert
2 participants
feats: repair the thinking of claude to openrouter convert
Summary by CodeRabbit