Skip to content

Conversation

@mrubens
Copy link
Collaborator

@mrubens mrubens commented Oct 28, 2025

No setting for this one


Important

Simplifies getRooReasoning() logic by removing unnecessary enableReasoningEffort check in reasoning.ts.

  • Behavior:
    • Removes check for enableReasoningEffort === false in getRooReasoning() in reasoning.ts.
    • Now, if reasoningEffort is provided and not "minimal", returns { enabled: true, effort: reasoningEffort }.
    • If reasoningEffort is undefined, returns { enabled: false }.
  • Misc:
    • Simplifies logic by removing unnecessary condition in getRooReasoning().

This description was created by Ellipsis for 699ffbd. You can customize this summary. It will automatically update as commits are pushed.

@mrubens mrubens requested review from cte and jr as code owners October 28, 2025 18:47
@dosubot dosubot bot added the size:XS This PR changes 0-9 lines, ignoring generated files. label Oct 28, 2025
@roomote
Copy link

roomote bot commented Oct 28, 2025

Issues Found

  • Removed enableReasoningEffort check breaks user settings - The deleted code block (lines 57-62) was necessary to respect when users explicitly disable reasoning via enableReasoningEffort: false. Without this check, the function ignores this setting and may enable reasoning anyway, creating inconsistency with other providers and breaking the existing test at line 774-792.

Follow Along on Roo Code Cloud

@dosubot dosubot bot added the bug Something isn't working label Oct 28, 2025
Comment on lines -57 to 62

// If enableReasoningEffort is explicitly false, return enabled: false
if (settings.enableReasoningEffort === false) {
return { enabled: false }
}

// If reasoning effort is provided, return it with enabled: true
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing this check breaks the enableReasoningEffort setting for the Roo provider. When a user explicitly sets enableReasoningEffort: false in their settings, the function should return { enabled: false } to respect their preference, but now it will ignore this setting and potentially enable reasoning anyway if reasoningEffort has a value. This creates inconsistency with other providers (getOpenRouterReasoning, getAnthropicReasoning, getOpenAiReasoning) which all respect enableReasoningEffort: false via shouldUseReasoningEffort(). The existing test at line 774-792 expects this behavior and will fail without this check.

@mrubens mrubens closed this Oct 28, 2025
@github-project-automation github-project-automation bot moved this from Triage to Done in Roo Code Roadmap Oct 28, 2025
@github-project-automation github-project-automation bot moved this from New to Done in Roo Code Roadmap Oct 28, 2025
Copy link

@roomote roomote bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review complete. Found 1 issue that needs to be addressed before approval.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working size:XS This PR changes 0-9 lines, ignoring generated files.

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

2 participants