Repository navigation
fix(actions): forward declared reasoning none - #3427
Conversation
WalkthroughThe request preparation logic preserves ChangesReasoning effort forwarding
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/actions/src/prepare-request-body.spec.ts (1)
981-990: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExpand coverage for the catalog contract.
This test covers only
xai/grok-4-3, while the PR targets eight mappings. Add table-driven cases for the other affected mappings.Also add a mapping that uses
openai-chat-completionsbut does not declare"none". Assert that normalization still removes"none"for that mapping.🤖 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 981 - 990, Expand the test around prepare in prepare-request-body.spec.ts into table-driven cases covering all eight affected mappings, including the existing xai/grok-4-3 case and the seven other mappings targeted by the PR. Add a separate openai-chat-completions mapping that does not declare "none", and assert normalization removes "none" for it while preserving "none" for mappings that declare it.
🤖 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.ts`:
- Around line 1173-1176: Update the “none” reasoning-effort handling near the
apiFormat check to remove the broad OpenAI-compatible mapping fallback. Preserve
the existing provider allowlist, and allow mapping-specific forwarding only when
the mapping’s reasoningEfforts includes "none".
---
Nitpick comments:
In `@packages/actions/src/prepare-request-body.spec.ts`:
- Around line 981-990: Expand the test around prepare in
prepare-request-body.spec.ts into table-driven cases covering all eight affected
mappings, including the existing xai/grok-4-3 case and the seven other mappings
targeted by the PR. Add a separate openai-chat-completions mapping that does not
declare "none", and assert normalization removes "none" for it while preserving
"none" for mappings that declare it.
🪄 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: afd284d5-7289-4848-b00c-e857199764eb
📒 Files selected for processing (2)
packages/actions/src/prepare-request-body.spec.tspackages/actions/src/prepare-request-body.ts
cda1fd6 to
4f60e34
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
…none CodeRabbit review (theopenco#3427): the apiFormat === openai-chat-completions fallback preserved 'none' for any OpenAI-compatible mapping even when its catalog entry omits 'none' from reasoningEfforts, risking a provider 4xx from an unsupported enum. The mapping's reasoningEfforts is authoritative (matching the PR objective theopenco#3423); the provider allowlist still handles native cases. Add table-driven coverage for non-allowlisted mappings that declare 'none' (deepinfra, novita, runware) plus a regression test proving a non-allowlisted OpenAI-compatible mapping without 'none' gets the value stripped.
|
Good point — the Fix (2f9ca3c): removed the broad Tests added (all 238 pass in
|
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/actions/src/prepare-request-body.spec.ts (2)
992-1006: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the remaining affected mappings.
The table does not exercise
canopywave/kimi-k3orrunware/gemma-4-31b-it, although both are listed among the eight affected mappings. Add rows for both mappings. Otherwise, a future catalog regression for either mapping can pass without a failing test.Suggested additions
test.each([ + ["canopywave", "kimi-k3"], ["deepinfra", "deepseek-v4-pro"], ["deepinfra", "hy3"], ["novita", "hy3"], ["runware", "deepseek-v4-flash"], + ["runware", "gemma-4-31b-it"], ])(🤖 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 992 - 1006, Add test cases for the missing affected mappings, canopywave/kimi-k3 and runware/gemma-4-31b-it, to the parameter table in the “forwards none to %s when the mapping declares it” test. Use each provider/model pair and preserve the existing expectation that reasoning_effort equals “none”.
1000-1002: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCorrect the allowlist description.
The table values are
deepinfra,novita, andrunware, but thehandlesNoneNativelyallowlist shown inpackages/actions/src/prepare-request-body.ts:1155-1205lists none of those provider IDs. These cases exercise the mapping catalog path. Update the comment so it does not claim that the provider allowlist is also covered.🤖 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 1000 - 1002, Update the comment above these provider cases in prepare-request-body.spec.ts to state that their catalog entries publish `none` and exercise the mapping catalog path, without claiming they are included in the handlesNoneNatively allowlist. Keep the note that both relevant paths must agree where applicable.
🤖 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.
Nitpick comments:
In `@packages/actions/src/prepare-request-body.spec.ts`:
- Around line 992-1006: Add test cases for the missing affected mappings,
canopywave/kimi-k3 and runware/gemma-4-31b-it, to the parameter table in the
“forwards none to %s when the mapping declares it” test. Use each provider/model
pair and preserve the existing expectation that reasoning_effort equals “none”.
- Around line 1000-1002: Update the comment above these provider cases in
prepare-request-body.spec.ts to state that their catalog entries publish `none`
and exercise the mapping catalog path, without claiming they are included in the
handlesNoneNatively allowlist. Keep the note that both relevant paths must agree
where applicable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 8b69da22-7850-4e4f-b00f-8e3aac97338c
📒 Files selected for processing (2)
packages/actions/src/prepare-request-body.spec.tspackages/actions/src/prepare-request-body.ts
💤 Files with no reviewable changes (1)
- packages/actions/src/prepare-request-body.ts
|
While auditing aws-bedrock today, found something that directly supports this fix approach: xai/grok-4.3 on aws-bedrock already has an apiFormat: "openai-chat-completions" exemption that bypasses both the handlesNoneNatively gate and the supportedParameters check — so it already forwards reasoning_effort: "none" correctly today, unlike the 8 mappings this PR fixes. Same model, same value, working on one host and broken on another purely due to this per-mapping wiring gap. Confirms the pattern this PR addresses is real and the fix (catalog-driven, per-mapping) is the right shape — aws-bedrock is basically already doing what this PR makes possible everywhere else. |
|
Confirmed this resolves the root cause described in #3423. One thing worth adding to the test table, tying back to the original scope: #3423 named 8 mappings total, and this diff's tests cover 5 of them (grok-4.3/xai, deepinfra/deepseek-v4-pro, deepinfra/hy3, novita/hy3, runware/deepseek-v4-flash). The 3 not yet covered — canopywave/kimi-k3, runware/deepseek-v4-pro, runware/gemma-4-31b-it — were explicitly part of the original finding, so adding them would close the loop on the full reported scope, not just a subset. |
…none CodeRabbit review (theopenco#3427): the apiFormat === openai-chat-completions fallback preserved 'none' for any OpenAI-compatible mapping even when its catalog entry omits 'none' from reasoningEfforts, risking a provider 4xx from an unsupported enum. The mapping's reasoningEfforts is authoritative (matching the PR objective theopenco#3423); the provider allowlist still handles native cases. Add table-driven coverage for non-allowlisted mappings that declare 'none' (deepinfra, novita, runware) plus a regression test proving a non-allowlisted OpenAI-compatible mapping without 'none' gets the value stripped.
…ng_effort none table Adds canopywave/kimi-k3, runware/deepseek-v4-pro and runware/gemma-4-31b-it to the table-driven cases — the three mappings named in the original issue that were still untested. All declare "none" in reasoningEfforts.
2f9ca3c to
224c62e
Compare
|
Thanks for the precise call-out — all 8 mappings from #3423 are now covered. Added the 3 missing cases to the table-driven test, rebased the branch onto current
Each entry was verified against the model catalog before adding ( Validation: Branch rebased on |
steebchen
changed the title
3 participants
Fixes #3423
What & why
8 mappings across 5 providers (canopywave, runware, deepinfra, novita, xai) publish
"none"in their catalogreasoningEfforts, but none of those providers are in thehandlesNoneNativelyallowlist inprepare-request-body.ts— so a user requestingreasoning_effort: "none"gets the value silently stripped before the request is built, regardless of what the mapping declares. Same bug shape as #3365 (bytedance/glm-5-2), but spanning 8 mappings.For a mapping that publishes
noneinreasoningEfforts, that catalog entry is authoritative: it documents that the provider accepts the value. The fix lets the catalog speak for the 8 mappings instead of hardcoding each provider into the allowlist.Changes
packages/actions/src/prepare-request-body.ts: addproviderMappingForOptions?.reasoningEfforts?.includes("none")tohandlesNoneNatively. The existing provider allowlist is untouched (still authoritative for providers that acceptnonewithout declaring it); the catalog entry now also forwards the value when declared. Applied with parentheses to keep||/??mixing valid.packages/actions/src/prepare-request-body.spec.ts: regression testforwards none to xAI Grok when the mapping declares it(xai/grok-4-3 publishesnoneinreasoningEfforts).How I tested it
main(requestBody.reasoning_effortisundefined— the value was stripped); passes after the fix.vitest run packages/actions/src/prepare-request-body.spec.ts— 233 passed.packages/actionssuite: 531 passed; the 4 failures inenv-inventory.spec.tsare Redis-dependent (verified failing identically on a cleanmaincheckout without this change — no Redis/Docker available here).pnpm format+ lint-staged (eslint/prettier) pass on the changed files.Summary by CodeRabbit
reasoning_effort: "none"setting for supported provider configurations.