Skip to content

Conversation

@cannuri
Copy link
Contributor

@cannuri cannuri commented Mar 11, 2025

Context

the browser_action instructions were included in system prompt even when Browser usage was turned off in the settings. This PR makes sure that they are only included in system prompt if the browser usage is enabled and the model is supporting computer use.

Implementation

Passing in browserUseEnabled and the actual supportsComputerUse values in ClineProvider.ts did the trick.

How to Test

Use a mode that supports computer use, turn Browser tool usage off in the settings, preview system prompt => it should not contain browser_tool instructions.

Use a mode that doesn't support computer use, turn Browser tool usage on in the settings, preview system prompt => it should not contain browser_tool instructions.

Use a mode that supports computer use, turn Browser tool usage on in the settings, preview system prompt => it should contain browser_tool instructions.

Notes

This is my second attempt. My first attempt #1514 was not that elegant.


Important

Fix browser instructions inclusion in system prompt to depend on both browser tool setting and model support in ClineProvider.ts.

  • Behavior:
    • Fix inclusion of browser instructions in system prompt in ClineProvider.ts.
    • Browser instructions now included only if browserToolEnabled is true and model supports computer use.
  • Implementation:
    • Add browserToolEnabled and supportsComputerUse checks in ClineProvider.ts.
    • Modify SYSTEM_PROMPT call to use canUseBrowserTool.
  • Testing:
    • Add tests in ClineProvider.test.ts to verify browser tool inclusion logic.
    • Test cases for different combinations of browserToolEnabled and model support.

This description was created by Ellipsis for a9773b7. It will automatically update as commits are pushed.

@changeset-bot
Copy link

changeset-bot bot commented Mar 11, 2025

⚠️ No Changeset found

Latest commit: a9773b7

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@dosubot dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. bug Something isn't working labels Mar 11, 2025
const systemPrompt = await SYSTEM_PROMPT(
this.context,
cwd,
apiConfiguration.openRouterModelInfo?.supportsComputerUse ?? false,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Wow, that is a good find / what was I thinking? 😂

@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Mar 11, 2025
@mrubens mrubens merged commit 9dfdc42 into RooCodeInc:main Mar 11, 2025
18 checks passed
@github-project-automation github-project-automation bot moved this from New to Done in Roo Code Roadmap Mar 11, 2025
@hannesrudolph
Copy link
Collaborator

hannesrudolph commented Mar 17, 2025

@mrubens @cannuri this is causing the browser browser system prompt rules to be excluded sometimes depending on the conditions of the saving of the settings I think.

@hannesrudolph
Copy link
Collaborator

Browser Tool Setting Bug Analysis

After examining the code, I've identified the root cause of the bug:

The Issue

  1. The browser tool is conditionally included in the system prompt based on the supportsComputerUse flag passed to SYSTEM_PROMPT.

  2. The original working implementation passes apiConfiguration.openRouterModelInfo?.supportsComputerUse ?? false directly to the system prompt generator.

  3. The buggy implementation attempted to combine "model capability" with "user preference" by doing:

    const modelSupportsComputerUse = this.getCurrentCline()?.api.getModel().info.supportsComputerUse ?? false
    const canUseBrowserTool = modelSupportsComputerUse && (browserToolEnabled ?? true)

Why It Fails

The bug occurs because:

  1. When generating the system prompt, getCurrentCline() is often undefined, especially:

    • During preview mode in settings
    • When no active task is running
    • When viewing the system prompt in various UI contexts
  2. When getCurrentCline() is undefined, the property chain can't be accessed:

    • modelSupportsComputerUse defaults to false due to the ?? false fallback
    • canUseBrowserTool always becomes false regardless of settings
    • The browser_action tool gets excluded from the prompt
  3. This means the browser_action tool is always excluded from the system prompt, regardless of the checkbox state in settings.

@hannesrudolph
Copy link
Collaborator

created #1754 to fix this, draft

@cannuri
Copy link
Contributor Author

cannuri commented Mar 18, 2025

created #1754 to fix this, draft

@hannesrudolph Does it really fix it though? It needs to follow this behavior:

Browser Enabled + Model supports computer use --> include browser prompt
Browser Enabled + Model doesn't support computer use --> don't include browser prompt
Browser DIsabled + Model supports computer use --> don't include browser prompt
Browser DIsabled + Model doesn't support computer use --> don't include browser prompt

did you test all cases?

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

Labels

bug Something isn't working lgtm This PR has been approved by a maintainer size:L This PR changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants