fix: add auto-approval support for MCP resources (#5300) #5709
Conversation
dosubot
bot
added
the
size:XL
PR Review: Add Auto-Approval Support for MCP Resources (#5709)Executive SummaryThis PR successfully implements auto-approval functionality for MCP resources, mirroring the existing pattern used for MCP tools. The implementation is well-structured and follows established patterns, but there are several critical issues and pattern inconsistencies that need to be addressed before approval. Overall Assessment: NEEDS CHANGES Critical Issues (Must Fix)1. UI Pattern Inconsistency in McpResourceRowSeverity: HIGH 🔴 The new Issues:
Evidence from McpToolRow pattern: // McpToolRow - Proper pattern
<div className="flex items-center gap-4 flex-shrink-0">
{alwaysAllowMcp && isToolEnabled && (
<VSCodeCheckbox
checked={tool.alwaysAllow}
onChange={handleAlwaysAllowChange}
className="text-xs">
<span className="text-vscode-descriptionForeground whitespace-nowrap">
{t("mcp:tool.alwaysAllow")}
</span>
</VSCodeCheckbox>
)}
</div>McpResourceRow should follow the same pattern for consistency. 2. Regex Pattern Security ConcernSeverity: HIGH 🔴 The URI template pattern matching in const pattern = template.uriTemplate
.replace(/[.*+?^${}()|[\]\\]/g, "\\$&") // Escape special regex chars
.replace(/\\\{[^}]+\\\}/g, "[^/]+") // Match path segments, not everythingIssues:
Recommendation: Add validation and consider using a more restrictive pattern or a proper URI template library. Pattern Inconsistencies3. Missing Error Handling PatternSeverity: MEDIUM 🟡 The Current implementation: case "toggleResourceAlwaysAllow": {
try {
await provider.getMcpHub()?.toggleResourceAlwaysAllow(/*...*/)
} catch (error) {
provider.log(`Failed to toggle auto-approve for resource ${message.resourceUri}: ${JSON.stringify(error, Object.getOwnPropertyNames(error), 2)}`)
}
break
}Missing compared to
4. Test Organization IssueSeverity: MEDIUM 🟡 The resource auto-approval tests in // Lines 769-824: Manual logic implementation in tests
const mcpServerUse = JSON.parse(lastMessage.text)
if (mcpServerUse.type === "access_mcp_resource") {
const server = mcpServers?.find((s: any) => s.name === mcpServerUse.serverName)
// ... manual logic
}Issues:
Architecture Concerns5. Configuration Schema InconsistencySeverity: MEDIUM 🟡 The Current BaseConfigSchema (line 46): const BaseConfigSchema = z.object({
// ... other fields
alwaysAllow: z.array(z.string()).default([]), // Only for tools
// Missing: alwaysAllowResources schema definition
})Missing:
Redundancy Findings6. Duplicate Pattern ImplementationSeverity: LOW 🟢 The resource auto-approval implementation correctly follows the existing tool pattern, but there are opportunities for code reuse: Similar patterns found:
Recommendation: Consider extracting common patterns into shared utilities to reduce maintenance burden. Test Coverage Assessment7. Comprehensive Test Coverage ✅Severity: GOOD 🟢 The PR includes good test coverage for resource auto-approval scenarios: Covered scenarios:
However: Tests need to be refactored to test actual component behavior rather than manual logic implementation. Translation Quality8. Consistent Translation Implementation ✅Severity: GOOD 🟢 The translations for Verified languages:
Pattern consistency: All translations follow the same key structure as the existing tool translations. Security Assessment9. URI Template Validation NeededSeverity: MEDIUM 🟡 The resource template matching logic needs additional validation: Current implementation allows:
Recommendations:
RecommendationsImmediate Actions Required:
Future Improvements:
ConclusionThis PR implements a much-needed feature that follows the established architectural patterns. The core functionality is sound and the test coverage is comprehensive. However, critical UI inconsistencies and security concerns must be addressed before this can be approved. The implementation demonstrates good understanding of the existing codebase patterns, but needs refinement to meet the project's quality standards. Recommendation: REQUEST CHANGES - Address critical issues before re-review. |
daniel-lxs
moved this from PR [Needs Prelim Review]
to PR [Draft / In Progress]
in Roo Code Roadmap
- Fix UI pattern inconsistency in McpResourceRow to match McpToolRow - Secure URI pattern matching with validation and timeout protection - Add comprehensive error handling to toggleResourceAlwaysAllow - Update tests to match secure implementation patterns - Add alwaysAllowResources field to BaseConfigSchema validation Addresses review feedback from PR RooCodeInc#5709
Related GitHub IssueCloses: #5300 Roo Code Task Context (Optional)No Roo Code task context for this PR. DescriptionThis PR addresses the comprehensive review feedback and fixes all 5 critical issues identified for #5709. Key Changes Made:
Implementation Details:
Areas for review focus:
Test ProcedureTesting performed:
To verify these changes:
Test Environment:
Pre-Submission Checklist
Screenshots / VideosUI changes involve styling consistency improvements in McpResourceRow component - no visual functionality changes. Documentation Updates
Additional NotesAll critical review feedback has been addressed with comprehensive fixes that enhance security, maintainability, and user experience. The implementation follows established patterns and maintains backward compatibility. Files Modified: Security Improvements:
Pattern Consistency:
Get in TouchDiscord: @MuriloFP |
hannesrudolph
added
PR - Draft / In Progress
and removed
PR - Needs Preliminary Review
labels
- Remove inverted isInChatContext condition from McpResourceRow - Remove unused isInChatContext parameter completely - Add missing props (serverName, alwaysAllowMcp) to McpResourceRow in ChatRow - Make resource auto-approve behavior consistent with tool implementation This fixes the issue where the 'Always Allow' checkbox was not appearing for MCP resources in the chat approval dialog.
- Fix state persistence for MCP resource auto-approve checkbox - Refactor ChatRow to use live state from mcpServers instead of temporary objects - Fix auto-approval logic in ChatView to check alwaysAllow before pattern matching - Add proper type imports for McpResource and McpResourceTemplate - Ensure resource auto-approval works exactly like tool auto-approval Addresses review feedback from PR RooCodeInc#5709
github-project-automation
bot
moved this from PR [Draft / In Progress]
to Done
in Roo Code Roadmap
2 participants
PR Title: fix: add auto-approval support for MCP resources (#5300)
Related GitHub Issue
Closes: #5300
Roo Code Task Context (Optional)
N/A
Description
This PR fixes the auto-approval mechanism for
access_mcp_resourcerequests by implementing the missing "Always Allow" functionality for MCP resources, matching the existing behavior for MCP tools.Key implementation details:
alwaysAllow?: booleanproperty toMcpResourceandMcpResourceTemplatetypes in the shared types packageMcpResourceRowcomponent to display an "Always Allow" checkbox when global MCP auto-approval is enabledChatViewcomponent to check resource auto-approval status foraccess_mcp_resourcerequestsMcpHubto persist resource auto-approval preferences in MCP settings filestoggleResourceAlwaysAllowto handle UI interactionsDesign choices:
alwaysAllowResourcesarray default to empty arrayAreas for review focus:
ChatView.tsxMcpHub.tsTest Procedure
Unit tests added:
ChatView.auto-approve.spec.tsxManual testing steps:
Testing environment:
Pre-Submission Checklist
Screenshots / Videos
[Screenshots to be added showing the "Always Allow" checkbox for MCP resources in the settings view]
Documentation Updates
Additional Notes
This implementation follows the same pattern as the existing MCP tool auto-approval feature to ensure consistency in the user experience. All translations have been validated using the project's translation validation script.
Get in Touch
MuriloFP