Repository navigation
fix(actions): send nested reasoning object to embercloud instead of flat reasoning_effort - #3477
Closed
andyst-dev wants to merge 1 commit into
Closed
andyst-dev wants to merge 1 commit into
andyst-dev wants to merge 1 commit into
Conversation
…lat reasoning_effort embercloud's documented request body uses reasoning.enabled + reasoning.effort (https://embercloud.ai/docs/request-body); the flat OpenAI-style reasoning_effort field is not documented there and is silently ignored, so the declared effort never reached the GLM/Minimax reasoning-capable mappings. Build the nested shape for embercloud (enabled: false for none) while every other provider keeps the flat field. Regression tests assert the nested shape and the absence of the flat parameter.
Contributor
WalkthroughThe request builder now serializes Embercloud reasoning settings through a nested ChangesEmbercloud reasoning mapping
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/actions/src/prepare-request-body.spec.ts (2)
1129-1129: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace the broad
anyassertion.Line 1129 disables type checking for all request-body assertions. Use a narrow test-only assertion type, or narrow
FormDatabefore returning the request body.🤖 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` at line 1129, Replace the broad any assertion in the request-body test around the FormData return with a narrow test-only assertion or explicitly narrow the FormData type before returning it, while preserving the existing request-body assertions.Source: Coding guidelines
1132-1148: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd coverage for the nested disable shape.
The builder has a separate
reasoning_effort === "none"branch. Add a test that expectsreasoning: { enabled: false }and noreasoning_effortfield.🤖 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 1132 - 1148, Add a test alongside the existing nested reasoning tests in prepare-request-body.spec.ts that invokes prepare with "none" and asserts reasoning equals { enabled: false } while reasoning_effort is undefined, covering the dedicated disabled branch.
🤖 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 1111-1129: Update the prepare helper’s effort parameter to use the
reasoning-effort literal union accepted by prepareRequestBody, restricted to the
values exercised by these embercloud tests, instead of accepting any string.
Keep the existing call and reasoning_effort behavior unchanged.
---
Nitpick comments:
In `@packages/actions/src/prepare-request-body.spec.ts`:
- Line 1129: Replace the broad any assertion in the request-body test around the
FormData return with a narrow test-only assertion or explicitly narrow the
FormData type before returning it, while preserving the existing request-body
assertions.
- Around line 1132-1148: Add a test alongside the existing nested reasoning
tests in prepare-request-body.spec.ts that invokes prepare with "none" and
asserts reasoning equals { enabled: false } while reasoning_effort is undefined,
covering the dedicated disabled branch.
🪄 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: 7ab44e6d-cbdd-4089-b698-006b883fdb3a
📒 Files selected for processing (2)
packages/actions/src/prepare-request-body.spec.tspackages/actions/src/prepare-request-body.ts
Comment on lines
+1111
to
+1129
| async function prepare(effort: string) { | ||
| return (await prepareRequestBody( | ||
| "embercloud", | ||
| "glm-5.2", | ||
| null, | ||
| "glm-5.2", | ||
| [{ 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, // reasoning_effort | ||
| true, // supportsReasoning | ||
| )) as any; |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files | rg 'packages/actions/src/prepare-request-body\.spec\.ts|prepare-request-body' || true
echo "== outline/spec helper region =="
if [ -f packages/actions/src/prepare-request-body.spec.ts ]; then
wc -l packages/actions/src/prepare-request-body.spec.ts
sed -n '1080,1155p' packages/actions/src/prepare-request-body.spec.ts | cat -n
fi
echo "== related function signatures / reasoning effort usages =="
rg -n "reasoning_effort|function prepareRequestBody|const prepareRequestBody|\\bhigh\\b|\\bnone\\b|\\bmax\\b" packages/actions/src packages -g '*.ts' -g '!**/*.spec.ts' | head -n 200Repository: theopenco/llmgateway
Length of output: 24277
Narrow the reasoning-effort parameter for embercloud tests.
prepareRequestBody accepts only reasoningEffort literals, but the helper allows any string. Use a literal union that matches the tests.
Proposed fix
- async function prepare(effort: string) {
+ async function prepare(effort: "none" | "high" | "max") {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| async function prepare(effort: string) { | |
| return (await prepareRequestBody( | |
| "embercloud", | |
| "glm-5.2", | |
| null, | |
| "glm-5.2", | |
| [{ 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, // reasoning_effort | |
| true, // supportsReasoning | |
| )) as any; | |
| async function prepare(effort: "none" | "high" | "max") { | |
| return (await prepareRequestBody( | |
| "embercloud", | |
| "glm-5.2", | |
| null, | |
| "glm-5.2", | |
| [{ 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, // reasoning_effort | |
| true, // supportsReasoning | |
| )) as any; |
🤖 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 1111 - 1129,
Update the prepare helper’s effort parameter to use the reasoning-effort literal
union accepted by prepareRequestBody, restricted to the values exercised by
these embercloud tests, instead of accepting any string. Keep the existing call
and reasoning_effort behavior unchanged.
Source: Coding guidelines
Member
|
Superseded by #3482, closing this one. The change itself is correct and is carried over as-is — verified live against
Thanks @andyst-dev — the original commit is preserved in #3482. Generated by Claude Code |
steebchen
added a commit
that referenced
this pull request
Aug 8, 2026
Fixes #3444. Supersedes #3477 (rebased onto current `main`, plus a typecheck fix and live/e2e verification against the provider). Original change by @andyst-dev. ## Problem EmberCloud does not accept the flat OpenAI-style `reasoning_effort` field — it silently ignores it. The gateway was forwarding that flat field for the reasoning-capable GLM mappings, so a declared effort never reached the provider and no reasoning was ever produced. Confirmed live against `api.embercloud.ai` on `glm-4.7`: | request | result | | --- | --- | | `reasoning_effort: "high"` (before this PR) | 200, 2 completion tokens, **no reasoning** | | no reasoning field at all | 200, 2 completion tokens, no reasoning | | `reasoning: {enabled: true, effort: "high"}` | 200, 191 completion tokens, full `message.reasoning` | | `reasoning: {enabled: false}` | 200, no reasoning | `GET /v1/models` agrees: the reasoning-capable models advertise `reasoning` in `supported_parameters` and never `reasoning_effort`. ## Approach `prepare-request-body.ts` builds the nested shape for `embercloud` only: - an effort tier → `reasoning: { enabled: true, effort: "<tier>" }` - `none` → `reasoning: { enabled: false }` - every other provider keeps the flat `reasoning_effort` field unchanged The `none` branch is currently unreachable — `none` is normalized to `undefined` for providers not in `handlesNoneNatively` before this point — and is kept for when a mapping-declared `none` opt-in lands. ## Things a reviewer would otherwise have to check - **No new 4xx risk from effort tiers.** Every value the `reasoning_effort` union can emit (`minimal`/`low`/`medium`/`high`/`xhigh`/`max`) returns 200 upstream; the provider does not validate the enum at all (even a bogus string is accepted), so nothing regresses for tiers EmberCloud does not recognize. - **No risk for non-reasoning mappings.** The nested object is tolerated by EmberCloud models that do not advertise reasoning (`kimi-k2.5`, `qwen3-coder-next` both 200), which covers mappings with an empty `supportedParameters`. - **Streaming is fine.** With reasoning enabled the provider emits `delta.reasoning` chunks plus `completion_tokens_details.reasoning_tokens`; the existing streaming transform already normalizes `reasoning`/`reasoning_content`. - **Provider coverage limits.** `glm-5.2`, `glm-4.6` and `glm-4.5-air` return 429 on every request and `glm-5.1` returns `400 "Temporary routing error"`, matching the `stability: "unstable"` comments already in the catalogue. Live coverage therefore rests on `glm-4.7` and `glm-5`. - **Not changed here:** the "reasoning requests time out repeatedly in e2e" comments on the `glm-5` / `glm-4.7` mappings no longer reproduce (both suites ran green end to end), but revisiting those stability flags is a separate change. ## Verification Merged onto `main` at f2056f9. ``` TEST_MODELS="embercloud/glm-4.7" FULL_MODE=true pnpm test:e2e Test Files 29 passed | 2 skipped (31) Tests 100 passed | 90 skipped (190) TEST_MODELS="embercloud/glm-5" FULL_MODE=true pnpm test:e2e Test Files 29 passed | 2 skipped (31) Tests 98 passed | 90 skipped (188) pnpm test:unit 4270 passed | 3 skipped pnpm build 17/17 pnpm format clean ``` Counter-check: reverting only the `prepare-request-body.ts` hunk fails exactly three e2e tests for `embercloud/glm-4.7` — `basic reasoning`, `reasoning + streaming`, `reasoning + tool calls`. The nested shape is what makes them pass. The spec helper's `effort` parameter was typed `string`, which does not narrow to `prepareRequestBody`'s `reasoning_effort` union and failed `tsc` with TS2345, breaking `pnpm build`. It is now typed as the union, matching the other helpers in that file. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01YLDMLFqT6A7Lnk1i61eFCm --- _Generated by [Claude Code](https://claude.ai/code/session_01YLDMLFqT6A7Lnk1i61eFCm)_ <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved reasoning controls for EmberCloud’s GLM-5.2 models. * “None” now correctly disables reasoning, while other effort levels are sent in the format required by the provider. * Reasoning settings for other providers remain unchanged. <!-- 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>
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
2 participants
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.
Fixes #3444
What
embercloud's documented request body uses a nested
reasoningobject (reasoning.enabled+reasoning.effort) — the flat OpenAI-stylereasoning_effortfield is not documented and is silently ignored. The gateway was forwarding the flat field for the 4 reasoning-capable GLM mappings (glm-5.2 / 5.1 / 5 / 4.7), so the declared effort never reached the provider.This PR builds the nested shape for embercloud in
prepare-request-body.ts:reasoning: { enabled: true, effort: "<tier>" }none→reasoning: { enabled: false }(defensive — currently normalized away before this point, kept for after the mapping-declarednoneopt-in lands)reasoning_effortfield unchangedThe mapping's
supportedParametersalready declares"reasoning", so this aligns the sent body with the declared capability surface.Validation
Docs checked live at https://embercloud.ai/docs/request-body and https://embercloud.ai/docs/reasoning (nested shape confirmed; reasoning-capable models listed: glm-5.2, glm-5.1, glm-5, glm-4.7, minimax-m2.5).
New regression tests assert the nested shape for
high/maxand the absence of the flat parameter. Prettier clean.Note: live probing is not possible right now — embercloud rate-limits these models (429 on every request, per the catalog comments) — but the shape now matches their published request-body docs.
Summary by CodeRabbit
New Features
Bug Fixes