fix: ensure MaxTokens >= budget when extended thinking is enabled#48
Conversation
When extended thinking is enabled via the reasoning parameter, Anthropic requires max_tokens to be at least the thinking budget_tokens. This fix ensures MaxTokens is bumped to the budget value when it's lower, in both ChatRequest and ResponsesRequest conversion paths.
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughOptimization and validation improvements made to the Anthropic provider's request conversion functions. A computed budget value is reused instead of being recalculated, and guards are added to ensure MaxTokens meets the computed budget requirement. No changes to public interfaces. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@internal/providers/anthropic/anthropic.go`:
- Around line 193-196: Add a debug/info log when you bump anthropicReq.MaxTokens
to the budget so users can see the change: capture the original
anthropicReq.MaxTokens, then when you set anthropicReq.MaxTokens = budget emit a
log using the same logger and message style as the temperature-override warning
(the same logger used near the temperature override) showing the original value
and the new budget value; apply the identical change in
convertResponsesRequestToAnthropic around the MaxTokens adjustment (lines
~522-525) so both conversion functions log adjustments consistently.
| @@ -185,9 +185,14 @@ func convertToAnthropicRequest(req *core.ChatRequest) *anthropicRequest { | |||
|
|
|||
| // Map reasoning effort to Anthropic extended thinking | |||
| if req.Reasoning != nil && req.Reasoning.Effort != "" { | |||
There was a problem hiding this comment.
@copilot, @coderabbitai Do we need a checking against - req.Reasoning.Effort not set / nil ?
There was a problem hiding this comment.
@SantiagoDePolonia The current checking is already sufficient. Here's why:
The condition if req.Reasoning != nil && req.Reasoning.Effort != "" properly handles all cases:
req.Reasoning != nil- This prevents a nil pointer dereference by checking if the Reasoning object exists firstreq.Reasoning.Effort != ""- This checks if Effort is set (non-empty)
Since Go uses short-circuit evaluation with &&, if req.Reasoning is nil, the second condition won't be evaluated, preventing any panic.
Regarding Effort specifically: Based on the usage pattern (comparing with ""), Effort appears to be a string type, not a *string. In Go:
- String fields cannot be nil—they default to
""(empty string) when not set - The check
!= ""already handles the "not set" case
Conclusion: No additional nil checking is needed. The current implementation correctly handles:
- ✅
req.Reasoningbeing nil - ✅
req.Reasoning.Effortbeing empty/not set - ✅ Prevents nil pointer dereference with short-circuit evaluation
|
@SantiagoDePolonia I've opened a new pull request, #49, to work on those changes. Once the pull request is ready, I'll request review from you. |
| } | ||
| // Ensure MaxTokens is at least the budget tokens | ||
| if anthropicReq.MaxTokens < budget { | ||
| anthropicReq.MaxTokens = budget |
There was a problem hiding this comment.
@copilot Add some logs here - that MaxTokens has been increased because ... Anthropic require etc...
|
@SantiagoDePolonia I've opened a new pull request, #50, to work on those changes. Once the pull request is ready, I'll request review from you. |
) * Initial plan * feat: add logging for MaxTokens adjustment in Anthropic provider Add informative log statements when MaxTokens is adjusted to meet Anthropic extended thinking requirements. Logs include original value, adjusted value, and the reason for adjustment. Co-authored-by: SantiagoDePolonia <16936376+SantiagoDePolonia@users.noreply.github.com> * refactor: extract MaxTokens logging into helper function Extract duplicate logging statements into a dedicated helper function to improve code maintainability and ensure consistency across both ChatCompletion and StreamChatCompletion flows. Co-authored-by: SantiagoDePolonia <16936376+SantiagoDePolonia@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: SantiagoDePolonia <16936376+SantiagoDePolonia@users.noreply.github.com>
* Initial plan * test: add comprehensive tests for reasoning effort edge cases Co-authored-by: SantiagoDePolonia <16936376+SantiagoDePolonia@users.noreply.github.com> * fix: improve test code quality based on review feedback Co-authored-by: SantiagoDePolonia <16936376+SantiagoDePolonia@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: SantiagoDePolonia <16936376+SantiagoDePolonia@users.noreply.github.com>
When extended thinking is enabled via the reasoning parameter, Anthropic requires max_tokens to be at least the thinking budget_tokens. This fix ensures MaxTokens is bumped to the budget value when it's lower, in both ChatRequest and ResponsesRequest conversion paths.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.