feat(friendli): add reasoning params support#886
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughFriendli model metadata now defines reasoning settings for GLM models. The provider applies those settings to streaming and non-streaming requests, and tests cover enabled, disabled, model-specific, and streamed reasoning behavior. ChangesFriendli reasoning support
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant FriendliHandler
participant FriendliAPI
participant StreamConsumer
FriendliHandler->>FriendliAPI: send reasoning parameters and messages
FriendliAPI-->>FriendliHandler: return response or stream chunks
FriendliHandler-->>StreamConsumer: emit reasoning and text chunks
Possibly related PRs
Suggested labels: 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 |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@src/api/providers/__tests__/friendli.spec.ts`:
- Around line 397-583: The Friendli reasoning tests lack coverage for default
model settings and explicit reasoning disablement. Extend the “Friendli-specific
reasoning params” suite with a GLM-5.2 case omitting both enableReasoningEffort
and reasoningEffort, asserting the model default enables reasoning with the
expected reasoning_effort and chat_template_kwargs, and a disabled-reasoning
case asserting chat_template_kwargs.enable_thinking is explicitly false while
reasoning parameters remain disabled.
In `@src/api/providers/friendli.ts`:
- Around line 87-121: Update buildFriendliReasoningParams so
controllable-reasoning models receive chat_template_kwargs with enable_thinking
false when reasoningEffort is disabled or set to none, including the
early-return path. Preserve the existing empty-params behavior for models
without controllable reasoning support, and keep enable_thinking true for active
reasoning requests.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b8feab0f-3a58-41ff-9a11-bee8c5dbdc20
📒 Files selected for processing (3)
packages/types/src/providers/friendli.tssrc/api/providers/__tests__/friendli.spec.tssrc/api/providers/friendli.ts
github-actions
Bot
added
the
awaiting-review
taltas
left a comment
taltas
left a comment
There was a problem hiding this comment.
Thanks for the PR. A couple of questons on some of the models I found.
| outputPrice: 4.4, | ||
| cacheWritesPrice: 0, | ||
| cacheReadsPrice: 0.26, | ||
| supportsReasoningEffort: ["minimal", "low", "medium", "high", "xhigh", "max"], |
There was a problem hiding this comment.
Does GLM offer a way to disable reasoning effort completely using 'disable' as a key?
There was a problem hiding this comment.
Nope, verified that it throws 422 when disable or none is given
Endpoint: https://api.friendli.ai/serverless/v1/chat/completions
Model: zai-org/GLM-5.2
{"message":"invalid JSON payload: unknown enum value: 'disable'"}
{"message":"invalid JSON payload: unknown enum value: 'none'"}
To disable thinking it has to be done via chat_template_kwargs: { enable_thinking: true } option
| outputPrice: 1.2, | ||
| cacheWritesPrice: 0, | ||
| cacheReadsPrice: 0.06, | ||
| supportsReasoningBinary: true, |
There was a problem hiding this comment.
On MiniMax-M2.5 This will render the checkbox Use reasoning in the settings.... but the thing is, this model always has reasoning on though right? We should introduce or reuse metadata that distinguishes required/always reasoning from optional binary reasoning, hide or disable the on/off control for this model, and always request parsed reasoning for MiniMax-M2.5. In addition to this, we should add tests for MiniMax with the reasoning flag false and unset, not only true.
I think these changes should be in a separate PR in order not to bloat this one
There was a problem hiding this comment.
Thanks for the review! I reverted changes related to Minimax-m2.5 issues and planning to treat this in separate PR.
github-actions
Bot
added
awaiting-author
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/api/providers/friendli.ts (1)
172-175: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove unused variable.
The variable
modelInfois destructured but never used in this method.♻️ Proposed refactor
override async completePrompt(prompt: string): Promise<string> { - const { id: modelId, info: modelInfo } = this.getModel() + const { id: modelId } = this.getModel() const friendliExtra = this.buildFriendliReasoningParams()🤖 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 `@src/api/providers/friendli.ts` around lines 172 - 175, Remove the unused modelInfo binding from the getModel() destructuring in completePrompt, while preserving the modelId assignment and the existing buildFriendliReasoningParams flow.
🤖 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 `@src/api/providers/friendli.ts`:
- Around line 172-175: Remove the unused modelInfo binding from the getModel()
destructuring in completePrompt, while preserving the modelId assignment and the
existing buildFriendliReasoningParams flow.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1db29ef9-2606-4e79-ab82-ec57f833dbb7
📒 Files selected for processing (3)
packages/types/src/providers/friendli.tssrc/api/providers/__tests__/friendli.spec.tssrc/api/providers/friendli.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/api/providers/tests/friendli.spec.ts
github-actions
Bot
added
awaiting-review
|
@taltas hey, I know you probably have a lot going on right now, but would you mind taking another look when you get a chance? Both of your comments are addressed since the last review:
Thanks again — the review pointed out exactly where the boundary should be. |
edelauna
left a comment
edelauna
left a comment
There was a problem hiding this comment.
Thanks for this contribution, can with reviewing - had some comments regarding implementation details.
github-actions
Bot
added
awaiting-author
…ions - completePrompt: accept CompletePromptOptions, pass abortSignal to SDK - split FriendliChatCompletionParams into streaming/non-streaming variants so completePrompt no longer needs 'as any' on a streaming-required type - remove unused modelInfo destructuring in completePrompt - add include_reasoning assertions to none/disable/completePrompt tests Address @edelauna review on Zoo-Code-Org#886.
|
Thanks for the review @edelauna — all five points are addressed in a66aab1:
|
github-actions
Bot
removed
the
awaiting-author
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/api/providers/friendli.ts (2)
75-81: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse an own-property check for configured model IDs.
inalso matches inherited properties, so strings like"toString"can be accepted as a supported Friendli model instead of falling back to the default. UseObject.prototype.hasOwnProperty.call(this.providerModels, this.options.apiModelId)before readingproviderModels[id].🤖 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 `@src/api/providers/friendli.ts` around lines 75 - 81, Update getModel to validate options.apiModelId with Object.prototype.hasOwnProperty.call(this.providerModels, this.options.apiModelId) instead of the in operator, ensuring inherited names such as toString fall back to defaultProviderModelId before providerModels is accessed.
178-185: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winAwait the streamed completion before wrapping in
try.With
stream: true,create()may reject aftercreateStream()returns, so the current synchronoustry/catchwon’t catch initial request failures. Await the SDK call inside thetryblock or attach.catch(...)to the returned SDK promise sohandleOpenAIErrorcan normalize initial streaming request errors.🤖 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 `@src/api/providers/friendli.ts` around lines 178 - 185, The streaming completion in the provider method must be awaited within the try block so asynchronous SDK rejections are normalized by handleOpenAIError. Update the client.chat.completions.create call in the shown method to await its returned promise while preserving the existing parameters and error handler.
🤖 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 `@src/api/providers/friendli.ts`:
- Around line 188-204: Update completePrompt() to forward options.timeoutMs in
the requestOptions passed to this.client.chat.completions.create, alongside the
existing abortSignal forwarding. Preserve undefined behavior when no timeout is
configured and continue honoring both per-call options.
---
Outside diff comments:
In `@src/api/providers/friendli.ts`:
- Around line 75-81: Update getModel to validate options.apiModelId with
Object.prototype.hasOwnProperty.call(this.providerModels,
this.options.apiModelId) instead of the in operator, ensuring inherited names
such as toString fall back to defaultProviderModelId before providerModels is
accessed.
- Around line 178-185: The streaming completion in the provider method must be
awaited within the try block so asynchronous SDK rejections are normalized by
handleOpenAIError. Update the client.chat.completions.create call in the shown
method to await its returned promise while preserving the existing parameters
and error handler.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 31dccd4a-08d7-4ab2-b5ea-5b9cc7bc2ec6
📒 Files selected for processing (2)
src/api/providers/__tests__/friendli.spec.tssrc/api/providers/friendli.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/api/providers/tests/friendli.spec.ts
github-actions
Bot
added
the
awaiting-review
9740644 to
3b36302
Compare
3 participants
Summary
Add Friendli-specific reasoning parameter support for GLM-5.x and MiniMax-M2.5 models served via Friendli Model APIs.
Friendli extends the OpenAI Chat Completions API with non-standard reasoning fields (
reasoning_effort,chat_template_kwargs,parse_reasoning,include_reasoning) that the baseBaseOpenAiCompatibleProviderdoes not handle. This PR overridescreateStreamandcompletePromptinFriendliHandlerto inject these parameters.Ref: https://friendli.ai/docs/guides/reasoning
Changes
Model catalog (
packages/types/src/providers/friendli.ts)supportsReasoningEffort: ["minimal","low","medium","high","xhigh","max"]+reasoningEffort: "high"(controllable reasoning models)supportsReasoningBinary: true(always-reasoning model)Handler (
src/api/providers/friendli.ts)createStreamandcompletePromptto inject Friendli reasoning params viabuildFriendliReasoningParams()reasoning_effort+chat_template_kwargs: { enable_thinking: true }+parse_reasoning/include_reasoningwhen user selects a valid effortparse_reasoning/include_reasoningonly"none"or"disable", orenableReasoningEffort=false, no Friendli reasoning params are sent — the model runs at its default behaviordelta.reasoning_contentstreaming already handled byextractReasoningFromDeltain base classTests (
src/api/providers/__tests__/friendli.spec.ts)Design decisions
enable_thinking: false: When reasoning is disabled (none/disable), we send nothing rather than explicitly disabling thinking. This lets the model use its default behavior.ultracode: Friendli API supportsultracodein the enum but we do not surface it — only the standard efforts up tomax.preserveReasoning: Friendli only supports stateless responses API — multi-turn reasoning re-submission has no effect.Test plan
pnpm vitest run api/providers/__tests__/friendli.spec.ts— 29/29 passpnpm run build— successpnpm run lint(src + packages/types) — cleanSummary by CodeRabbit