Repository navigation
fix(models): reasoning_effort for azure grok-4.3 - #3438
andyst-dev wants to merge 5 commits into
Conversation
WalkthroughThe change adds provider-specific ChangesxAI reasoning effort support
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/actions/src/prepare-request-body.spec.ts`:
- Around line 1120-1141: Update the reasoning_effort test around
prepareRequestBody to verify the supportedParameters gate rather than only
serialization. Assert that all six relevant provider mappings include
"reasoning_effort", or invoke the caller path that filters unsupported
parameters before prepareRequestBody receives them, ensuring the test fails when
the mapping removes the parameter.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 86c2ed39-2532-490f-bd3e-bb5f5633a07d
📒 Files selected for processing (2)
packages/actions/src/prepare-request-body.spec.tspackages/models/src/models/xai.ts
| test.each(xaiEffortCases)( | ||
| "forwards reasoning_effort for %s/%s once declared in supportedParameters", | ||
| async (provider, model, effort) => { | ||
| const requestBody = (await prepareRequestBody( | ||
| provider, | ||
| model, | ||
| null, | ||
| model, | ||
| [{ role: "user", content: "Hello!" }], | ||
| false, // stream | ||
| undefined, // temperature | ||
| undefined, // max_tokens | ||
| undefined, // top_p | ||
| undefined, // frequency_penalty | ||
| undefined, // presence_penalty | ||
| undefined, // response_format | ||
| undefined, // tools | ||
| undefined, // tool_choice | ||
| effort, | ||
| true, // supportsReasoning | ||
| )) as unknown as Record<string, unknown>; | ||
| expect(requestBody.reasoning_effort).toBe(effort); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Test the supportedParameters gate.
prepareRequestBody serializes any defined reasoning_effort. This test passes if a mapping removes "reasoning_effort" because it passes effort directly to the serializer.
Assert that the six provider mappings contain "reasoning_effort", or exercise the caller that removes unsupported parameters before it calls prepareRequestBody.
Based on learnings: supportedParameters is the source of truth, and the gateway maps unsupported parameters to undefined before request construction.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/actions/src/prepare-request-body.spec.ts` around lines 1120 - 1141,
Update the reasoning_effort test around prepareRequestBody to verify the
supportedParameters gate rather than only serialization. Assert that all six
relevant provider mappings include "reasoning_effort", or invoke the caller path
that filters unsupported parameters before prepareRequestBody receives them,
ensuring the test fails when the mapping removes the parameter.
Source: Learnings
|
The Minor comment about test fidelity is worth addressing before merge — right now the tests confirm the request body ends up correct, but don't actually assert that supportedParameters contains "reasoning_effort" for the 6 fixed mappings. That distinction matters: if a future refactor changes how the gate reads supportedParameters, a serialization-only test could keep passing while the actual capability check silently regresses. A direct assertion on each mapping's supportedParameters array would catch that class of regression, not just today's behavior. |
…ings grok-4-3 and grok-4-1-fast-reasoning (azure-ai-foundry), grok-4-20-reasoning (vertex-openai), grok-4, grok-4-20-beta-0309-reasoning and grok-build-0-1 (xai) all declare reasoning: true but omitted reasoning_effort from their supportedParameters, so any effort value was silently dropped before the request was built. Inline the shared xaiSupportedParamsNoFreqPresence list (following theopenco#3159) and add reasoning_effort to each active reasoning mapping; deactivated and non-reasoning mappings keep the shared constant untouched. Fixes theopenco#3436, Fixes theopenco#3403
…or xAI mappings Direct capability assertion on each fixed mapping's supportedParameters array, not just the serialized request body — catches a future refactor that stops reading supportedParameters while serialization tests keep passing.
ed47305 to
7cff5cc
Compare
|
Good point — the serialization assertion alone wouldn't catch a refactor that stops reading const modelDef = models.find((m) => m.id === model);
const mapping = modelDef?.providers.find((p) => p.providerId === provider);
expect(mapping?.supportedParameters).toContain("reasoning_effort");This walks the same resolution path the request builder uses ( Validation: Branch rebased on current |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/actions/src/prepare-request-body.spec.ts (1)
2683-2703: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winReplace broad type escapes with narrow payload types.
The new tests use
anyandas any. Define local provider-specific request-body types, then cast throughunknownonly where theprepareRequestBodyunion requires narrowing.
packages/actions/src/prepare-request-body.spec.ts#L2683-L2703: Type the Google tool request body and its schema parameters.packages/actions/src/prepare-request-body.spec.ts#L2760-L2780: Reuse the same Google tool request-body type.packages/actions/src/prepare-request-body.spec.ts#L5995-L6021: Typeprovider,webSearchTool, and the DashScope request payload withoutany.As per coding guidelines,
**/*.{ts,tsx}must “avoidanyandas anyunless absolutely necessary.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/actions/src/prepare-request-body.spec.ts` around lines 2683 - 2703, Replace broad any/as any escapes in prepare-request-body.spec.ts with local provider-specific types: at lines 2683-2703 define and use a Google tool request-body type including schema parameters, and at lines 2760-2780 reuse it; at lines 5995-6021 type provider, webSearchTool, and the DashScope request payload explicitly. Cast through unknown only where prepareRequestBody’s union requires narrowing, and make no other changes.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@packages/actions/src/prepare-request-body.spec.ts`:
- Around line 2683-2703: Replace broad any/as any escapes in
prepare-request-body.spec.ts with local provider-specific types: at lines
2683-2703 define and use a Google tool request-body type including schema
parameters, and at lines 2760-2780 reuse it; at lines 5995-6021 type provider,
webSearchTool, and the DashScope request payload explicitly. Cast through
unknown only where prepareRequestBody’s union requires narrowing, and make no
other changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ac3f09c1-3dad-4987-874d-f578198997cf
📒 Files selected for processing (1)
packages/actions/src/prepare-request-body.spec.ts
Only azure-ai-foundry/grok-4.3 keeps the parameter: its sibling xai and aws-bedrock mappings of the same model already declare reasoningEfforts, so the deployment is known to accept it. The other five mappings go back to the shared const — grok-4 is documented as rejecting reasoning_effort (its own comment says it always reasons), grok-build-0.1 and grok-4.20-beta-0309-reasoning were deliberately skipped in theopenco#3046 for lack of documented values, and the vertex-openai and Grok 4.1 Fast mappings are the live checks theopenco#3403 and theopenco#3436 explicitly asked for before enabling. Each of those models has a single routable mapping, so a rejected parameter is a hard error with no fallback, and auto routing injects reasoning_effort: "low" on its own. Azure Foundry grok-4.3 now also declares reasoningEfforts so clients can discover the accepted tiers, and a metadata invariant keeps every future xAI mapping that advertises the parameter from omitting them. Claude-Session: https://claude.ai/code/session_01BrVPkQMps6NrMacF8Pkx1r
The table read supportedParameters off the const-asserted catalogue union, which is a TS2339 error and broke tsc for the whole package even though vitest transpiled it fine. It also passed internal model ids as usedExternalId, so the vertex-openai case built a partner-prefixed model string production never emits, and it sat inside the Moonshot describe. Replace it with a dedicated describe that uses real external ids, keeps the positive case for the one mapping that forwards the parameter, and restores negative coverage for the mappings that deliberately drop it. Claude-Session: https://claude.ai/code/session_01BrVPkQMps6NrMacF8Pkx1r
e2e against api.x.ai shows three of the six mappings widened in #3438 answer reasoning_effort with 400 'does not support parameter reasoningEffort': xai/grok-4-20-beta-0309-reasoning, xai/grok-build-0-1, and vertex-openai/grok-4-20-reasoning (same upstream model as the former). Declaring the parameter made the gateway forward it, turning every effort-bearing request into an upstream 400 — 4 gateway e2e tests that pass on main fail with it. Restore the shared parameter list for those three, with a comment on each recording why the omission is deliberate, and split the spec's table so the rejecting mappings assert the drop rather than the forward. xai/grok-4 keeps the parameter (verified 200; it aliases to grok-4.3 upstream). The two azure-ai-foundry mappings keep it as well and remain unverified here — no Azure credentials. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RbUnEaa7vrZZkD82wSStTr
|
Closing in favor of #3483, which carries this work forward with a correction. E2E-verified this branch against a live xAI key. Three of the six mappings widened here reject Concretely, Separately, #3483 keeps your addition for Generated by Claude Code |
Supersedes #3438 (fixes #3436, #3403). ## Problem Six xAI mappings declare `reasoning: true` but omit `reasoning_effort` from `supportedParameters`, so a caller-supplied effort is dropped at the gate in `packages/actions/src/prepare-request-body.ts:4077-4086`. #3438 treated all six as the same oversight and declared the parameter on every one. E2E against `api.x.ai` shows that premise does not hold. Three of those deployments reason unconditionally but reject the effort knob outright: | externalId | `reasoning_effort` | Result | | --- | --- | --- | | `grok-4` | low | 200 (aliases to `grok-4.3` upstream) | | `grok-4.3` | low | 200 | | `grok-4.20-beta-0309-reasoning` | low / medium / high | **400** `does not support parameter reasoningEffort` | | `grok-build-0.1` | low / medium / high | **400** same | | `grok-4.20-reasoning` | low | **400** same | Control requests with the parameter omitted return 200 for all three, so the parameter is the sole cause; streaming behaves identically. For those mappings the omission was the catalogue correctly reflecting upstream, not the bug the issues describe — declaring it turns every effort-bearing request into an upstream 400. ## Change `packages/models/src/models/xai.ts` — keep #3438's addition for `xai/grok-4`, `azure-ai-foundry/grok-4-3` and `azure-ai-foundry/grok-4-1-fast-reasoning`; restore the shared `xaiSupportedParamsNoFreqPresence` list for the three rejecting mappings, each with a comment recording why the omission is load-bearing so it does not get "fixed" again: - `xai` / `grok-4-20-beta-0309-reasoning` - `xai` / `grok-build-0-1` - `vertex-openai` / `grok-4-20-reasoning` — same upstream model as the first `packages/actions/src/prepare-request-body.spec.ts` — split #3438's six-row table into a positive table (three mappings forward the effort) and a negative one asserting the rejecting mappings are `reasoning: true`, do **not** list `reasoning_effort`, and drop the value from the request body. Also narrows the mapping lookup to `ProviderModelMapping`; reading `.supportedParameters` off the raw catalogue union fails `tsc` with TS2339, which broke `pnpm build` on #3438. ## Verification Ran with a live xAI key. - `TEST_MODELS="xai/grok-4,xai/grok-4-20-beta-0309-reasoning,xai/grok-build-0-1" pnpm test:e2e` — **29 passed, 2 skipped, 0 failed**. On #3438 the same command produced 4 failures (`chat-reasoning.e2e.ts > basic reasoning` and `chat-rs.e2e.ts > reasoning + streaming`, both models, 400 vs expected 200). Those four pass on `main`, confirming the regression rather than a pre-existing failure. - `prepare-request-body.spec.ts` + models specs: 380 passed. - `pnpm build`, `pnpm format`, `pnpm lint`: clean. ## Not verified `azure-ai-foundry/grok-4-1-fast-reasoning` keeps the parameter but was not exercised — no Azure credentials here, and the id does not resolve on native xAI (`grok-4.1-fast-reasoning` → "Model not found"; our `xai` mapping for it is deactivated). Since every xAI reasoning model probed except `grok-4.3` rejects `reasoning_effort`, this one is worth a `/e2e` run with the Azure key before merging. `azure-ai-foundry/grok-4-3` is safer — native `grok-4.3` accepts the parameter. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01RbUnEaa7vrZZkD82wSStTr --- _Generated by [Claude Code](https://claude.ai/code/session_01RbUnEaa7vrZZkD82wSStTr)_ <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved handling of the `reasoning_effort` setting across supported xAI and Azure AI Foundry models. * Ensured the setting is forwarded only when supported and omitted for deployments that reject it. * Updated Azure AI Foundry Grok 4.3 to use a default reasoning effort of `medium`. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: andyst-dev <150129844+andyst-dev@users.noreply.github.com> Co-authored-by: Claude <noreply@anthropic.com>
Refs #3436, refs #3403 — see "Deliberately not changed" for why neither is fully closed here.
Problem
azure-ai-foundry/grok-4.3declaresreasoning: truebut omitsreasoning_effortfrom itssupportedParameters, so any effort value a client sends is silently dropped at the default-case gate inprepare-request-body.tsbefore the request is built. The same underlying model already forwards the parameter on itsxaiandaws-bedrockmappings, both of which declarereasoningEfforts: ["none", "low", "medium", "high"]— Azure Foundry was simply never given the equivalent fix (#3159 scoped its change to the native mappings).Change
packages/models/src/models/xai.ts:reasoning_efforttoazure-ai-foundry/grok-4.3's inlinesupportedParameters, and declarereasoningEffortsmatching its sibling mappings so/v1/modelspublishes the accepted tiers.xaiSupportedParamsNoFreqPresencewhyreasoning_effortis deliberately absent from the shared const, so the next reader doesn't add it back wholesale.packages/models/src/model-metadata.spec.ts:reasoning_effortmust also declarereasoningEfforts. The tiers are the record that the deployment was actually verified; without them neither the gateway nor a client knows which values are safe.packages/actions/src/prepare-request-body.spec.ts:prepareRequestBody - xAI reasoning_effortdescribe: positive case for the Azure Foundry mapping, plus negative cases pinning the mappings that deliberately still drop the parameter.keeps Grok 4.3 as Azure Foundry chat completionstest now assertsreasoning_effort: "medium"is forwarded, where it previously assertedundefinedand encoded the bug.Deliberately not changed
An earlier revision of this PR also enabled
reasoning_effortonxai/grok-4,azure-ai-foundry/grok-4-1-fast-reasoning,vertex-openai/grok-4.20-reasoning,xai/grok-4.20-beta-0309-reasoningandxai/grok-build-0.1. Those are reverted:grok-4is documented as rejecting the parameter, and its own mapping comment says the model always reasons.grok-build-0.1andgrok-4.20-beta-0309-reasoningwere explicitly skipped in feat(models): declare reasoning efforts for xai #3046 for lack of documented values.vertex-openaiand Grok 4.1 Fast mappings are exactly the live checks [Bug]: reasoning_effort untested for grok-4.20-reasoning on vertex-openai — deferred check from #3046 #3403 and [Bug]: reasoning_effort dropped entirely for grok-4.3 and grok-4-1-fast-reasoning on azure-ai-foundry #3436 asked for before enabling — that verification still hasn't happened.The gateway forwards effort verbatim, each of those models has a single routable mapping, and auto routing injects
reasoning_effort: "low"on its own — so an unaccepted parameter is a hard user-visible error with no fallback to absorb it. Enabling them needs one live call each first. #3403 and thegrok-4-1-fast-reasoninghalf of #3436 therefore stay open.Two adjacent gaps are also out of scope here:
xainorazure-ai-foundryis inhandlesNoneNatively, sononeis normalized away on both hosts even thoughgrok-4.3declares it andaws-bedrockforwards it. Pre-existing on the native mapping; a gateway fix, not a catalogue one.supportedParametersgate.Validation
tsc -p packages/actionsclean (the previous revision's test helper failed with TS2339 and brokepnpm build).packages/models,packages/actions, and the gateway/playground specs that touch grok models.reasoningEffortsdeclaration makes it fail and name the offending mapping.pnpm formatand a fullpnpm build.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes