-
Notifications
You must be signed in to change notification settings - Fork 2.3k
fix: support simple on/off reasoning toggle for grok-4-fast on OpenRouter #8711
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
…uter - Added special handling for x-ai/grok-4-fast model in OpenRouter parser - Disabled reasoning effort for this model while keeping reasoning parameter - Updated reasoning logic to handle models with simple on/off toggle - Added comprehensive tests for the new functionality Fixes #8709
Code Review SummaryI've reviewed the changes and identified 2 issues that need to be addressed:
These issues affect the correctness of the reasoning toggle implementation. Please address them before merging. |
// Check if model supports reasoning but not effort or budget (e.g., grok-4-fast on OpenRouter) | ||
// These models use a simple on/off toggle via the exclude parameter | ||
if ( | ||
model.supportedParameters?.includes("reasoning") && | ||
!model.supportsReasoningEffort && | ||
!model.supportsReasoningBudget | ||
) { | ||
// If reasoning is not explicitly disabled, enable it | ||
// We use exclude: false to enable reasoning, and exclude: true to disable | ||
// Check both settings.reasoningEffort and the passed reasoningEffort parameter | ||
const effectiveEffort = reasoningEffort || settings.reasoningEffort | ||
const reasoningEnabled = effectiveEffort !== "minimal" | ||
return reasoningEnabled ? { exclude: false } : { exclude: true } | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The simple toggle logic doesn't respect the settings.enableReasoningEffort === false
setting. When users explicitly disable reasoning effort in settings, the model should not use reasoning, but this code will still enable it based on the effort values. This should check settings?.enableReasoningEffort === false
and return undefined
if true, similar to how shouldUseReasoningEffort
handles this in src/shared/api.ts lines 66-69.
// Check both settings.reasoningEffort and the passed reasoningEffort parameter | ||
const effectiveEffort = reasoningEffort || settings.reasoningEffort | ||
const reasoningEnabled = effectiveEffort !== "minimal" | ||
return reasoningEnabled ? { exclude: false } : { exclude: true } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When both reasoningEffort
and settings.reasoningEffort
are undefined, this code enables reasoning by default ({ exclude: false }
). This is inconsistent with how other reasoning models work, where undefined effort typically results in no reasoning parameters being sent. For grok-4-fast, when no effort preference is set, it should return undefined
instead of defaulting to enabled. This ensures the API's default behavior is respected.
Summary
This PR attempts to address Issue #8709 where the grok-4-fast model on OpenRouter was incorrectly showing reasoning effort options (low/medium/high) instead of a simple on/off toggle.
Problem
The grok-4-fast model on OpenRouter currently displays "reasoning effort" options, but according to the OpenRouter documentation, this model should only have reasoning either enabled or disabled (on/off toggle).
Solution
Changes
Testing
All tests pass successfully:
Fixes #8709
Feedback and guidance are welcome!
Important
Add simple on/off reasoning toggle for
grok-4-fast
model on OpenRouter, updatingparseOpenRouterModel
andgetOpenRouterReasoning
with tests.parseOpenRouterModel
inopenrouter.ts
now handlesgrok-4-fast
with a simple on/off reasoning toggle, disabling reasoning effort.getOpenRouterReasoning
inreasoning.ts
supports models with simple on/off toggle, usingexclude
parameter.openrouter.spec.ts
forgrok-4-fast
reasoning toggle behavior.reasoning.spec.ts
for models with simple on/off toggle and ensuring no impact on models with effort or budget capabilities.This description was created by
for 22e9b98. You can customize this summary. It will automatically update as commits are pushed.