Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a mechanism to ensure that the max_tokens field is present in requests for Claude models, preventing potential crashes or rejections from upstream APIs. A helper function, ensureModelMaxTokens, was added and integrated into both the synchronous and streaming execution paths. Feedback was provided to enhance the robustness of this check by verifying that max_tokens is a positive integer rather than merely checking for its existence.
| if maxTokens := gjson.GetBytes(body, "max_tokens"); maxTokens.Exists() { | ||
| return body | ||
| } |
There was a problem hiding this comment.
The current check for max_tokens only verifies its existence. If max_tokens is present but set to 0, null, or another non-positive value, the function will not set the default value. This could lead to the upstream API rejecting the request or crashing, which this function aims to prevent.
To make this more robust, the check should also ensure that max_tokens is a positive integer before returning.
| if maxTokens := gjson.GetBytes(body, "max_tokens"); maxTokens.Exists() { | |
| return body | |
| } | |
| if maxTokens := gjson.GetBytes(body, "max_tokens"); maxTokens.Exists() && maxTokens.Int() > 0 { | |
| return body | |
| } |
![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: 145e0e0b5d
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
|
||
| for _, provider := range registry.GetGlobalRegistry().GetModelProviders(strings.TrimSpace(modelID)) { | ||
| if strings.EqualFold(provider, "claude") { | ||
| body, _ = sjson.SetBytes(body, "max_tokens", defaultModelMaxTokens) |
There was a problem hiding this comment.
ensureModelMaxTokens unconditionally injects max_tokens=1024 when the field is absent, but both Execute and ExecuteStream call it after thinking.ApplyThinking, which may already set thinking.budget_tokens to values much larger than 1024 (for example from a model suffix like (8192)). Anthropic requests require max_tokens to be greater than the thinking budget, so this change can turn previously valid thinking-enabled requests into deterministic 400 errors for dynamically registered Claude models.
Useful? React with 👍 / 👎.
…etion_tokens and fallback to default
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f033d3a6df
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| for _, provider := range registry.GetGlobalRegistry().GetModelProviders(strings.TrimSpace(modelID)) { | ||
| if strings.EqualFold(provider, "claude") { | ||
| maxTokens := defaultModelMaxTokens | ||
| if info := registry.GetGlobalRegistry().GetModelInfo(strings.TrimSpace(modelID), "claude"); info != nil && info.MaxCompletionTokens > 0 { |
There was a problem hiding this comment.
ensureModelMaxTokens looks up MaxCompletionTokens using the modelID argument instead of the request’s final body.model, but this function is called after applyPayloadConfigWithRoot, which can override model. In configurations that remap models via payload rules, this can inject a max_tokens value from the old model (too high or too low for the rewritten one), causing avoidable Anthropic validation errors or unintended truncation.
Useful? React with 👍 / 👎.
2 participants
No description provided.