fix: respect enableReasoningEffort setting when determining reasoning usage #7049
Conversation
![roomote[bot]](https://avatars.githubusercontent.com/in/1546624?s=60&v=4){kind=small}
… usage - Modified shouldUseReasoningEffort to return false when enableReasoningEffort is explicitly set to false - This ensures reasoning is disabled when the setting is false, regardless of reasoningEffort value - Added comprehensive tests to verify the fix Fixes #7048
dosubot
bot
added
size:M
hannesrudolph
added
the
Issue/PR - Triage
| }): boolean => { | ||
| // If enableReasoningEffort is explicitly set to false, reasoning should be disabled | ||
| if (settings?.enableReasoningEffort === false) { | ||
| return false |
There was a problem hiding this comment.
Is this intentional? When enableReasoningEffort is undefined (not explicitly set), reasoning can still be enabled if other conditions are met. Should we consider treating undefined the same as false for a more conservative approach, or is the current behavior where only an explicit false disables reasoning the desired one?
| @@ -63,7 +63,17 @@ export const shouldUseReasoningEffort = ({ | |||
| }: { | |||
| model: ModelInfo | |||
| settings?: ProviderSettings | |||
| }): boolean => (!!model.supportsReasoningEffort && !!settings?.reasoningEffort) || !!model.reasoningEffort | |||
| }): boolean => { | |||
| // If enableReasoningEffort is explicitly set to false, reasoning should be disabled | |||
There was a problem hiding this comment.
Consider adding a JSDoc comment above the function to document the precedence rules. This would help future developers understand that enableReasoningEffort: false takes absolute precedence over all other settings.
| reasoningEffort: "medium", | ||
| } | ||
|
|
||
| expect(shouldUseReasoningEffort({ model, settings })).toBe(false) |
There was a problem hiding this comment.
Good test coverage for the explicit false case! Consider also adding a test for when enableReasoningEffort is explicitly undefined (not just missing) to document that this allows reasoning to proceed - this would make the behavior more explicit.
hannesrudolph
added
PR - Needs Preliminary Review
and removed
Issue/PR - Triage
daniel-lxs
moved this from PR [Needs Prelim Review]
to PR [Needs Review]
in Roo Code Roadmap
| }): boolean => (!!model.supportsReasoningEffort && !!settings?.reasoningEffort) || !!model.reasoningEffort | ||
| }): boolean => { | ||
| // If enableReasoningEffort is explicitly set to false, reasoning should be disabled | ||
| if (settings?.enableReasoningEffort === false) { |
There was a problem hiding this comment.
Roomote makes a good point about the default. Should this be:
| if (settings?.enableReasoningEffort === false) { | |
| if (!settings?.enableReasoningEffort) { |
Summary
This PR fixes an issue where the
reasoningEffortparameter would override theenableReasoningEffortsetting, causing reasoning to be enabled even when explicitly disabled.Problem
When
enableReasoningEffortwas set tofalsein the API configuration, the model would still perform reasoning ifreasoningEffort(e.g., "medium") was present. This behavior was unexpected as theenableReasoningEffortsetting should take precedence.Solution
Modified the
shouldUseReasoningEffortfunction insrc/shared/api.tsto:enableReasoningEffortis explicitly set tofalsefalseimmediately, disabling reasoning regardless of other settingsTesting
enableReasoningEffortisfalseRelated Issue
Fixes #7048
Important
Fix
shouldUseReasoningEffortto respectenableReasoningEffortsetting, ensuring reasoning is disabled when explicitly set tofalse.shouldUseReasoningEffortinapi.tsto respectenableReasoningEffortsetting, returningfalseif set tofalse.enableReasoningEffortisfalse, regardless ofreasoningEffortvalue.api.spec.tsto verify reasoning is disabled whenenableReasoningEffortisfalse.This description was created by
for d4bc592. You can customize this summary. It will automatically update as commits are pushed.