fix(sdk/go): rewrite max_tokens to max_completion_tokens for newer OpenAI models#724
Open
7vignesh wants to merge 2 commits into
Open
fix(sdk/go): rewrite max_tokens to max_completion_tokens for newer OpenAI models#7247vignesh wants to merge 2 commits into
7vignesh wants to merge 2 commits into
Conversation
…enAI models (Agent-Field#441) Newer OpenAI models (o1, o3, gpt-4o, gpt-4.x) dropped support for the legacy max_tokens parameter in favor of max_completion_tokens. The Go SDK was always emitting max_tokens, causing the output-length cap to be silently ignored. Changes: - Add model-aware marshalRequest() that detects newer OpenAI models and rewrites max_tokens → max_completion_tokens at serialization time - Detection uses model prefix matching (o1, o3, o4, gpt-4o, gpt-4.) - Preserves backward compatibility: legacy models still use max_tokens - Request-level model override takes precedence over client default - Add 6 focused tests covering new models, legacy models, nil handling, and request-level model override Closes Agent-Field#441
Contributor
Performance
✓ No regressions detected |
There was a problem hiding this comment.
Pull request overview
This PR updates the Go SDK’s request serialization to avoid OpenAI silently ignoring max_tokens for newer model families by rewriting it to max_completion_tokens when applicable.
Changes:
- Added model detection logic to decide when
max_tokensmust be rewritten tomax_completion_tokens. - Routed request serialization in
Clientthrough a new rewrite-awaremarshalRequestmethod. - Added unit tests covering model detection and JSON marshaling behavior for rewritten vs legacy models.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| sdk/go/ai/model_params.go | Adds model/base-URL detection and rewrite-aware request marshaling to emit max_completion_tokens for newer OpenAI model families. |
| sdk/go/ai/model_params_test.go | Adds tests validating model detection and ensuring marshaled JSON uses the correct token field. |
| sdk/go/ai/client.go | Switches request serialization to c.marshalRequest(req) for both non-streaming and streaming calls. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| cfg.APIKey = "test-key" | ||
| cfg.Model = "gpt-4o" | ||
|
|
||
| client, _ := NewClient(cfg) |
| cfg.APIKey = "test-key" | ||
| cfg.Model = "gpt-3.5-turbo" | ||
|
|
||
| client, _ := NewClient(cfg) |
| cfg.APIKey = "test-key" | ||
| cfg.Model = "o1-mini" | ||
|
|
||
| client, _ := NewClient(cfg) |
| cfg.APIKey = "test-key" | ||
| cfg.Model = "gpt-4o" | ||
|
|
||
| client, _ := NewClient(cfg) |
| cfg.APIKey = "test-key" | ||
| cfg.Model = "gpt-3.5-turbo" // client default is legacy | ||
|
|
||
| client, _ := NewClient(cfg) |
Contributor
📊 Coverage gateThresholds from
✅ Gate passedNo surface regressed past the allowed threshold and the aggregate stayed above the floor. |
Contributor
📐 Patch coverage gateThreshold: 80% on lines this PR touches vs
✅ Patch gate passedEvery surface whose lines were touched by this PR has patch coverage at or above the threshold. |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The Go SDK always emits
max_tokensin API requests, but newer OpenAI models (o1,o3,gpt-4o,gpt-4.x) only acceptmax_completion_tokensand silently ignoremax_tokens. This causes Go agents to lose their output-lengthcap with no error. This PR adds a model-aware serialization pass that rewrites the parameter for affected models.
Closes #441
Type of Change
Test Plan
cd sdk/go && go test ./ai/ -v -run "TestNeedsMaxCompletion|TestMarshalRequest"cd sdk/go && go build ./...cd sdk/go && CGO_ENABLED=0 GOOS=linux go build ./...golangci-lint run ./ai/...no new issuesTest Coverage
coverage-baseline.jsonin this PR only ifthe removal caused a legitimate regression and I called it out in the
summary above.
Checklist
docs/DEVELOPMENT.md.
Related Issues / PRs
Fixes #441