fix(azure): send max_completion_tokens for gpt-5/o-series reasoning deployments#2691
Open
chy1211 wants to merge 2 commits into
Open
fix(azure): send max_completion_tokens for gpt-5/o-series reasoning deployments#2691chy1211 wants to merge 2 commits into
chy1211 wants to merge 2 commits into
Conversation
…eployments Azure OpenAI reasoning-model deployments (gpt-5*, o1-/o3-/o4- series) reject the legacy max_tokens param with a 400 Unsupported parameter error and require max_completion_tokens instead. AzureExecutor.transformRequest was a pure passthrough with no param translation, so every request carrying max_tokens 400d against these deployments. Mirrors the requiresMaxCompletionTokens regex + rename-and-delete pattern already used by the github executor for Copilot-hosted reasoning models. Reproduced against a live Azure AI Foundry gpt-5.6-luna deployment: - POST .../chat/completions with max_tokens -> 400 unsupported_parameter - same request with max_completion_tokens -> 200 OK
There was a problem hiding this comment.
Pull request overview
This PR fixes Azure AI Foundry reasoning-model compatibility by translating the OpenAI-style max_tokens request field into Azure-required max_completion_tokens for reasoning deployments, aligning Azure behavior with the existing Copilot/GitHub executor handling.
Changes:
- Add
AzureExecutor.requiresMaxCompletionTokens(model)to detect reasoning deployments (gpt-5*,o1-,o3-,o4-). - Update
AzureExecutor.transformRequestto renamemax_tokens→max_completion_tokensonly when needed (and otherwise preserve passthrough behavior). - Add a new Vitest unit test file covering Azure executor registry wiring and request transformation behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
open-sse/executors/azure.js |
Adds reasoning-model detection and renames max_tokens to max_completion_tokens for Azure reasoning deployments. |
tests/unit/azure-executor.test.js |
New unit tests validating Azure executor registration and transformRequest rename/no-op behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+67
to
+70
| const transformed = { ...body }; | ||
| transformed.max_completion_tokens = transformed.max_tokens; | ||
| delete transformed.max_tokens; | ||
| return transformed; |
Comment on lines
+37
to
+63
| it("renames max_tokens to max_completion_tokens for reasoning deployments", () => { | ||
| const body = { messages: [{ role: "user", content: "hi" }], max_tokens: 50 }; | ||
| const out = executor.transformRequest("gpt-5.6-luna", body, true, {}); | ||
|
|
||
| expect(out.max_tokens).toBeUndefined(); | ||
| expect(out.max_completion_tokens).toBe(50); | ||
| // original body must not be mutated | ||
| expect(body.max_tokens).toBe(50); | ||
| }); | ||
|
|
||
| it("leaves max_tokens untouched for non-reasoning deployments", () => { | ||
| const body = { messages: [{ role: "user", content: "hi" }], max_tokens: 50 }; | ||
| const out = executor.transformRequest("gpt-4o", body, true, {}); | ||
|
|
||
| expect(out).toBe(body); | ||
| expect(out.max_tokens).toBe(50); | ||
| expect(out.max_completion_tokens).toBeUndefined(); | ||
| }); | ||
|
|
||
| it("is a no-op when max_tokens is absent", () => { | ||
| const body = { messages: [{ role: "user", content: "hi" }] }; | ||
| const out = executor.transformRequest("gpt-5.6-luna", body, true, {}); | ||
|
|
||
| expect(out).toBe(body); | ||
| expect(out.max_completion_tokens).toBeUndefined(); | ||
| }); | ||
| }); |
Per Copilot review on PR decolua#2691: transformRequest unconditionally overwrote an explicit max_completion_tokens with max_tokens when both were present in the request body. Only fall back to max_tokens when max_completion_tokens is absent; max_tokens is always deleted either way since Azure rejects it outright on these deployments. Adds a regression test locking in the precedence.
Author
|
Good catch — fixed in 6764532 : |
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
AzureExecutor.transformRequestwas a pure passthrough (return body), so it never translatedmax_tokensfor reasoning-model deployments.requiresMaxCompletionTokens(model)+ renamemax_tokens->max_completion_tokensforgpt-5*/o1-/o3-/o4-deployments, mirroring the pattern already used by thegithubexecutor for Copilot-hosted reasoning models.gpt-4o) are untouched —transformRequeststill returns the original body reference when no rename is needed.Why
Azure AI Foundry reasoning-model deployments reject the legacy
max_tokensparameter:{"error":{"message":"Unsupported parameter: 'max_tokens' is not supported with this model. Use 'max_completion_tokens' instead.","type":"invalid_request_error","param":"max_tokens","code":"unsupported_parameter"}}Any client that sends
max_tokens(the common/default param name in most OpenAI-compatible SDKs) gets a 400 on every request to agpt-5*/o-series Azure deployment, with no way to work around it from the client side through 9Router. Thegithubexecutor already special-cases this for Copilot;azurewas missing the same handling.Reproduced end-to-end against a live Azure AI Foundry
gpt-5.6-luna(GlobalStandard) deployment:POST .../openai/deployments/gpt-5.6-luna/chat/completions?api-version=2025-04-01-previewwithmax_tokensin the body ->400 unsupported_parametermax_tokensrenamed tomax_completion_tokens->200 OKTest Plan
Result: 1 file passed, 7 tests passed (new file, covers registry wiring, the
gpt-5/o[134]-regex, andtransformRequestrename/no-op/passthrough behavior including non-mutation of the original body object).Also ran the full
tests/unitsuite before and after this change to confirm no regressions:The 4-passed / 4-failed delta is exactly this PR's 7 new tests (net vs. pre-existing flake in that run); all pre-existing failures are unrelated
open-sseworkspace-alias resolution errors intests/unit/xai-oauth-service.test.jsand friends, present identically on unmodifiedupstream/masterin this environment (Node 20 portable build without the maintainer's workspace linking) — not caused by this change.