Skip to content

Remove redundant manual EnsureApproval calls from MCP tools#78

Merged
johnkord merged 3 commits intomainfrom
copilot/fix-77
Jun 26, 2025
Merged

Remove redundant manual EnsureApproval calls from MCP tools#78
johnkord merged 3 commits intomainfrom
copilot/fix-77

Conversation

Copy link
Copy Markdown

Copilot AI commented Jun 26, 2025

This PR removes all manual ToolApprovalManager.Instance.EnsureApproved() calls from MCP tool methods, ensuring that tool approval is controlled exclusively by the [RequiresApproval] attribute through the ToolApprovalWrapper system.

Problem

Many MCP tools had both:

  1. [RequiresApproval] attributes on methods
  2. Manual EnsureApproved() calls within the method implementations

This created redundant approval logic and violated the design principle that the [RequiresApproval] attribute should be the single source of truth for approval requirements.

Solution

Removed manual approval calls from methods WITH [RequiresApproval]:

  • ShellTools.RunCommand()
  • FileTools.WriteFile(), DeleteFile(), CreateDirectory()
  • HttpTools.HttpRequest()
  • CodeReviewTools.GenerateReviewComment()
  • GitHubTools.PostPullRequestComment()

Removed manual approval calls from methods WITHOUT [RequiresApproval]:

  • All read-only GitHub operations (GetPullRequest, GetPullRequestFiles, etc.)
  • Code review analysis operations (AnalyzePullRequestForReview, ExtractCodePatterns)
  • All Azure DevOps operations (6 methods)
  • All OpenAI Vector Store operations (5 methods)

Impact

  • ~200+ lines of redundant code removed
  • Approval logic now centralized in ToolApprovalWrapper
  • Cleaner, more maintainable codebase
  • All 61 existing tests continue to pass

The approval system continues to work correctly - tools with [RequiresApproval] are automatically wrapped with approval logic, while tools without the attribute execute immediately.

Fixes #77.


💬 Share your feedback on Copilot coding agent for the chance to win a $200 gift card! Click here to start the survey.

Copilot AI and others added 2 commits June 26, 2025 05:04
… attributes

Co-authored-by: johnkord <16021727+johnkord@users.noreply.github.com>
Co-authored-by: johnkord <16021727+johnkord@users.noreply.github.com>
Copilot AI changed the title [WIP] Many of the tools have EnsureApproval checks in the code for the MCP tools even if they don't have RequiresApproval annotation Remove redundant manual EnsureApproval calls from MCP tools Jun 26, 2025
Copilot AI requested a review from johnkord June 26, 2025 05:11
@johnkord johnkord marked this pull request as ready for review June 26, 2025 05:12
@johnkord johnkord merged commit 057f768 into main Jun 26, 2025
@johnkord johnkord deleted the copilot/fix-77 branch June 26, 2025 05:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Many of the tools have EnsureApproval checks in the code for the MCP tools even if they don't have RequiresApproval annotation

2 participants