Skip to content

Conversation

@roomote
Copy link
Contributor

@roomote roomote bot commented Nov 12, 2025

This PR attempts to address Issue #9190. Feedback and guidance are welcome.

Problem

After the recent refactoring that moved auto-approval logic from ChatView to Task (#9157), the MCP auto-approval behavior changed. Previously, when alwaysAllowMcp was enabled, ALL MCP tools would be auto-approved. After the refactor, both alwaysAllowMcp AND the individual tool alwaysAllow flag had to be true, which broke the expected behavior.

Solution

Changed the logic from AND to OR condition:

  • When alwaysAllowMcp is enabled → auto-approve all MCP tools
  • When a specific tool has alwaysAllow flag → auto-approve that specific tool
  • Either condition can trigger auto-approval independently

Changes

  • Modified src/core/auto-approval/index.ts to use OR logic instead of AND
  • Added comprehensive test coverage in src/core/auto-approval/__tests__/index.spec.ts

Testing

  • ✅ All new tests pass
  • ✅ All existing tests pass
  • ✅ Linting passes
  • ✅ Type checking passes

Fixes #9190


Important

Fixes MCP auto-approval logic to approve tools if either global or individual flags are set, with added tests.

  • Behavior:
    • Changes MCP auto-approval logic in checkAutoApproval() in index.ts to use OR condition.
    • Auto-approves all MCP tools if alwaysAllowMcp is true or if the tool's alwaysAllow flag is true.
  • Testing:
    • Adds tests in index.spec.ts to cover scenarios for MCP tool and resource auto-approval.
    • Tests handle cases with missing or invalid JSON, and when neither flag is set.
  • Misc:

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

…bled

- Changed MCP auto-approval logic from AND to OR condition
- When alwaysAllowMcp is enabled, all MCP tools are auto-approved
- Individual tool alwaysAllow flag can still approve specific tools
- Added comprehensive tests for MCP auto-approval scenarios

Fixes #9190
@roomote roomote bot requested review from cte, jr and mrubens as code owners November 12, 2025 06:56
@dosubot dosubot bot added the size:L This PR changes 100-499 lines, ignoring generated files. label Nov 12, 2025
@dosubot dosubot bot added the bug Something isn't working label Nov 12, 2025
@roomote
Copy link
Contributor Author

roomote bot commented Nov 12, 2025

Rooviewer Clock   See task on Roo Cloud

Review completed. The changes look good - the fix correctly restores the MCP auto-approval behavior by changing from AND to OR logic. When alwaysAllowMcp is enabled, all MCP tools will be auto-approved as expected. Individual tools can still be approved via their alwaysAllow flag. The test coverage is comprehensive and validates all scenarios.

Mention @roomote in a comment to request specific changes to this pull request or fix all unresolved issues.

@hannesrudolph hannesrudolph added the Issue/PR - Triage New issue. Needs quick review to confirm validity and assign labels. label Nov 12, 2025
@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Nov 12, 2025
@daniel-lxs daniel-lxs moved this from Triage to PR [Needs Review] in Roo Code Roadmap Nov 12, 2025
@hannesrudolph hannesrudolph added PR - Needs Review and removed Issue/PR - Triage New issue. Needs quick review to confirm validity and assign labels. labels Nov 12, 2025
@nonsleepr
Copy link

Tested the fix, it's bad. Auto-approves even if the tool is configured NOT to auto-approve.

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 PR - Needs Review size:L This PR changes 100-499 lines, ignoring generated files.

Projects

Status: PR [Needs Review]

Development

Successfully merging this pull request may close these issues.

[BUG] auto approuve no more working

5 participants