fix: cap OpenAI Chat/Responses max_tokens fields without forcing a default - #1294
fix: cap OpenAI Chat/Responses max_tokens fields without forcing a default#12940x0079 wants to merge 1 commit into
Conversation
The Anthropic-only max_tokens pre-chain (buildAnthropicPreChain) was the only caller of MaxTokensTransform; OpenAI Chat and Responses never used it, so requests with max_completion_tokens/max_output_tokens above the model's cap went upstream uncapped, and the existing ad-hoc cap in openai_chat.go only ever looked at max_tokens. Add executeOpenAIChatPreChain/executeOpenAIResponsesPreChain and call them from runOpenAIChatAttempt/runOpenAIResponsesAttempt. defaultMaxTokens is passed as 0 at both call sites (not GetDefaultMaxTokens(), which is documented as Anthropic-only and always non-zero) so omitted fields stay omitted — only present values above maxAllowed get capped. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MqN2PrHaQfhFyLNR2tf5Z2
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 46d6197ddf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // setting (max_tokens is mandatory on that API); OpenAI callers that omit | ||
| // max_tokens get no field injected, same as before this cap existed, and the | ||
| // upstream provider applies its own default. | ||
| if err := executeOpenAIChatPreChain(req.ChatCompletionNewParams, 0, maxAllowed); err != nil { |
There was a problem hiding this comment.
When Chat Completions clients send both max_tokens and max_completion_tokens, this replacement no longer caps max_tokens: MaxTokensTransform.applyOpenAIChat returns as soon as MaxCompletionTokens.Valid() is true, whereas the removed guard capped req.MaxTokens independently. In that compatibility scenario an oversized max_tokens is still forwarded to OpenAI-style providers and can be rejected even if max_completion_tokens was capped; please cap any present max_tokens as well or retain the old guard after this call.
Useful? React with 👍 / 👎.
| // applies its own default (the WithMaxTokens(maxAllowed) option further | ||
| // below is unrelated — it only seeds a fallback for cross-protocol | ||
| // conversions that require a value, e.g. when the target is Anthropic). | ||
| if err := executeOpenAIResponsesPreChain(req.ResponseNewParams, 0, maxAllowed); err != nil { |
There was a problem hiding this comment.
This cap mutates req.ResponseNewParams in place, but in the multi-service path above areq := req means assigning the per-attempt clone back onto areq.ResponseNewParams also replaces the shared template. If the first Responses attempt caps max_output_tokens for a lower-limit model and then fails over to a higher-limit model, the retry clones the already-capped params and silently keeps the lower cap, producing shorter outputs than requested. Keep an immutable template or allocate a fresh ResponseCreateRequest wrapper per attempt before running this mutating pre-chain.
Useful? React with 👍 / 👎.
2 participants
Summary
Follow-up to #1287.
MaxTokensTransformgained OpenAI Chat Completions / Responses support in that PR, but nothing actually wired it into the OpenAI request paths —NewMaxTokensTransformwas only ever appended bybuildAnthropicPreChain. As a result, OpenAI requests usingmax_completion_tokensormax_output_tokensabove the model's cap still went upstream uncapped, and the pre-existing ad-hoc cap inopenai_chat.goonly ever looked atmax_tokens.Changes
executeOpenAIChatPreChain/executeOpenAIResponsesPreChainininternal/server/protocol_transform.goand call them fromrunOpenAIChatAttempt/runOpenAIResponsesAttempt.defaultMaxTokensis passed as0at both call sites (nots.config.GetDefaultMaxTokens(), which is documented as Anthropic-only and always non-zero) — this means omitted fields stay omitted; only present values abovemaxAllowedget capped. This avoids silently forcing amax_tokens/max_output_tokensvalue onto OpenAI-style requests that never set one, which would have been a real behavior change for clients relying on the upstream provider's own default.Test plan
go build ./...go vet ./internal/server/...go test ./internal/server/...https://claude.ai/code/session_01MqN2PrHaQfhFyLNR2tf5Z2
Generated by Claude Code