fix(sdk/go): rewrite max_tokens to max_completion_tokens for newer OpenAI models#724
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
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.
📊 Coverage gateThresholds from
✅ Gate passedNo surface regressed past the allowed threshold and the aggregate stayed above the floor. |
📐 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>
AbirAbbas
left a comment
AbirAbbas
left a comment
There was a problem hiding this comment.
Ran a multi-agent review over this (PR-AF on AgentField, findings hand-verified before posting). The fix itself is sound — model-gated rewrite, both request paths covered, shadow-struct trick is correct (outer fields at depth 0 win over the embedded alias). Two things worth addressing before merge, inline below.
Minor notes, no action needed: isOpenAICompatible and the tests never assert the wire format on the tool-calling/streaming paths (only the non-streaming one), and the anthropic/cohere checks are substring matches on the whole URL — fine today, but a dedicated test for isOpenAICompatible would pin all of this down.
Address reviewer feedback: - TestIsOpenAICompatible: pin down behavior for OpenAI, Azure, OpenRouter, Anthropic, Cohere, and custom/self-hosted endpoints - TestMarshalRequest_NonOpenAIEndpointKeepsMaxTokens: verify non-OpenAI endpoints keep max_tokens even for new model names - TestMarshalRequest_StreamingPathRewritesMaxTokens: verify the rewrite works correctly when stream=true (same marshalRequest path)
santoshkumarradha
left a comment
santoshkumarradha
left a comment
There was a problem hiding this comment.
Checked this in a clean clone and reran the Go AI tests locally. The model-aware rewrite path and tests look good to me.
…oof model matching Address review feedback on Agent-Field#724: - Invert the model check: instead of allowlisting o1/o3/o4/gpt-4o/gpt-4.x (which missed gpt-5 and any future family), deny the closed legacy set (gpt-4, gpt-4-*, gpt-3*) and rewrite for every other gpt-* model plus any o<digit> reasoning model. - Invert the endpoint default: rewrite max_tokens -> max_completion_tokens only for endpoints known to accept it (api.openai.com, *.openai.azure.com, openrouter.ai), matched by parsed hostname rather than substring. Unknown or self-hosted BaseURLs (Ollama, LM Studio, llama.cpp, vLLM) keep max_tokens so servers that silently drop unknown fields never lose the output cap (Agent-Field#441). - Tests: check the NewClient error instead of discarding it, pin BaseURL explicitly so env vars cannot flip the endpoint gate, and add coverage for gpt-5/o5 rewrites, unknown-host passthrough, host lookalikes, and two end-to-end wire-body assertions (recording RoundTripper against api.openai.com and a real httptest server for the self-hosted case). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Maintainer takeover to address the outstanding review feedback (pushed 5c1c10f to this branch;
Verification: |
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