Skip to content

Conversation

@cannuri
Copy link
Contributor

@cannuri cannuri commented Mar 9, 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

I added browserToolEnabled to the system prompt everywhere where supportsComputerUse was checked.

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.


Important

Add browserToolEnabled flag to control browser action visibility, updating logic and tests to ensure browser actions are included only when both supportsComputerUse and browserToolEnabled are true.

  • Behavior:
    • Adds browserToolEnabled flag to control browser action visibility in system.ts, capabilities.ts, and rules.ts.
    • Browser actions are included only if both supportsComputerUse and browserToolEnabled are true.
  • Tests:
    • Updates system.test.ts and ClineProvider.test.ts to test scenarios with different combinations of supportsComputerUse and browserToolEnabled.
  • Functions:
    • Modifies getCapabilitiesSection(), getRulesSection(), and getToolDescriptionsForMode() to incorporate browserToolEnabled logic.
    • Updates ClineProvider to handle browserToolEnabled state.

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

@changeset-bot
Copy link

changeset-bot bot commented Mar 9, 2025

⚠️ No Changeset found

Latest commit: 15cb4ff

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:M This PR changes 30-99 lines, ignoring generated files. enhancement New feature or request labels Mar 9, 2025
@hannesrudolph hannesrudolph added the Issue - In Progress Someone is actively working on this. Should link to a PR soon. label Mar 9, 2025
@hannesrudolph hannesrudolph moved this from New to Issue [In Progress] in Roo Code Roadmap Mar 9, 2025
Copy link
Collaborator

@mrubens mrubens left a comment

Choose a reason for hiding this comment

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

I thought I had logic somewhere that only allowed supportsComputerUse to be true if enableBrowserTool was also true. It seemed a little cleaner than passing another variable through everywhere. Seems like it didn’t work as intended though? I wonder if worth a short investigation into why.

@cannuri
Copy link
Contributor Author

cannuri commented Mar 10, 2025

I thought I had logic somewhere that only allowed supportsComputerUse to be true if enableBrowserTool was also true. It seemed a little cleaner than passing another variable through everywhere. Seems like it didn’t work as intended though? I wonder if worth a short investigation into why.

Have I missed that? I cannot find that logic.
I would not suggest to allow supportsComputerUse to be true if browserToolEnabled is true also, because it might be, that it confuses later for another functionality (for example accessing and controlling GUI applications).
Instead we could combine both into a single one called "browserToolAvailable").

@mrubens
Copy link
Collaborator

mrubens commented Mar 10, 2025

I thought I had logic somewhere that only allowed supportsComputerUse to be true if enableBrowserTool was also true. It seemed a little cleaner than passing another variable through everywhere. Seems like it didn’t work as intended though? I wonder if worth a short investigation into why.

Have I missed that? I cannot find that logic. I would not suggest to allow supportsComputerUse to be true if browserToolEnabled is true also, because it might be, that it confuses later for another functionality (for example accessing and controlling GUI applications). Instead we could combine both into a single one called "browserToolAvailable").

This is the line I was thinking of https://github.com/RooVetGit/Roo-Code/blob/224303660bc66d1a89e6918431828671df1b7d6c/src/core/Cline.ts#L1097

@cannuri
Copy link
Contributor Author

cannuri commented Mar 10, 2025

I thought I had logic somewhere that only allowed supportsComputerUse to be true if enableBrowserTool was also true. It seemed a little cleaner than passing another variable through everywhere. Seems like it didn’t work as intended though? I wonder if worth a short investigation into why.

Have I missed that? I cannot find that logic. I would not suggest to allow supportsComputerUse to be true if browserToolEnabled is true also, because it might be, that it confuses later for another functionality (for example accessing and controlling GUI applications). Instead we could combine both into a single one called "browserToolAvailable").

This is the line I was thinking of

https://github.com/RooVetGit/Roo-Code/blob/224303660bc66d1a89e6918431828671df1b7d6c/src/core/Cline.ts#L1097

Damn, you are right. Let me investigate why it didn't work

@cannuri
Copy link
Contributor Author

cannuri commented Mar 10, 2025

I thought I had logic somewhere that only allowed supportsComputerUse to be true if enableBrowserTool was also true. It seemed a little cleaner than passing another variable through everywhere. Seems like it didn’t work as intended though? I wonder if worth a short investigation into why.

Have I missed that? I cannot find that logic. I would not suggest to allow supportsComputerUse to be true if browserToolEnabled is true also, because it might be, that it confuses later for another functionality (for example accessing and controlling GUI applications). Instead we could combine both into a single one called "browserToolAvailable").

This is the line I was thinking of

https://github.com/RooVetGit/Roo-Code/blob/224303660bc66d1a89e6918431828671df1b7d6c/src/core/Cline.ts#L1097

I think this is valid, but the line in https://github.com/RooVetGit/Roo-Code/blob/main/src/core/webview/ClineProvider.ts#L1847 causes the Preview Prompt to include browser_action. @mrubens I don't fully get why this exists actually. Why is it asking for tool use capabilities of openRouter there?

@hannesrudolph hannesrudolph moved this from Issue [In Progress] to PR [Pre Approval Review] in Roo Code Roadmap Mar 10, 2025
@mrubens
Copy link
Collaborator

mrubens commented Mar 11, 2025

Thanks for the fix in #1556! I can close this now right?

@mrubens mrubens closed this Mar 11, 2025
@github-project-automation github-project-automation bot moved this from PR [Pre Approval Review] to Done in Roo Code Roadmap Mar 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request Issue - In Progress Someone is actively working on this. Should link to a PR soon. size:M This PR changes 30-99 lines, ignoring generated files.

Projects

None yet

3 participants