Skip to content

.NET: Add name collision warnings for auto-approvals#7089

Open
westey-m wants to merge 2 commits into
microsoft:mainfrom
westey-m:dotnet-autoapproval-name-collision-warnings
Open

.NET: Add name collision warnings for auto-approvals#7089
westey-m wants to merge 2 commits into
microsoft:mainfrom
westey-m:dotnet-autoapproval-name-collision-warnings

Conversation

@westey-m

Copy link
Copy Markdown
Contributor

Motivation & Context

Defense in depth

Description & Review Guide

Fully document how auto-approval rules match by name only, and warn users to ensure that there are no name collisions.

Related Issue

#7088

Contribution Checklist

  • The code builds clean without any errors or warnings
  • All unit tests pass, and I have added new tests where possible
  • The PR follows the Contribution Guidelines
  • This PR is linked to an issue and there is no other open PR for this issue (see Related Issue above).
  • This is not a breaking change. If it is a breaking change, add the breaking change label (or add "[BREAKING]" to the title prefix, before or after any language prefix) — a workflow keeps the label and title prefix in sync automatically.

Copilot AI review requested due to automatic review settings July 13, 2026 11:34
@giles17 giles17 added documentation Usage: [Issues, PRs], Target: documentation in the code base and learn docs .NET Usage: [Issues, PRs], Target: .Net labels Jul 13, 2026
@github-actions github-actions Bot changed the title Add name collision warnings for auto-approvals .NET: Add name collision warnings for auto-approvals Jul 13, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR strengthens defense-in-depth guidance around tool auto-approval by documenting that several built-in auto-approval rules rely on tool names, and that name collisions can silently bypass the intended human-approval boundary (notably for configurable-name tools like the Harness shell tool).

Changes:

  • Expanded XML docs for built-in provider auto-approval rules (skills + file access) to enumerate the approved tool names and highlight collision risk.
  • Added security warnings to tool-approval configuration and to shell-tool naming surfaces (AsAIFunction(...), ShellToolName) to discourage name collisions.
  • Updated relevant .NET samples to call out the same collision hazard when using read-only auto-approval rules.

Reviewed changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
dotnet/src/Microsoft.Agents.AI/Skills/AgentSkillsProvider.cs Documents approved tool names and name-collision risk for skill auto-approval rules.
dotnet/src/Microsoft.Agents.AI/Harness/ToolApproval/ToolApprovalAgentOptions.cs Adds security warning about name-based auto-approvals in configuration docs.
dotnet/src/Microsoft.Agents.AI/Harness/ToolApproval/ToolApprovalAgent.cs Adds documentation clarifying the “approve everything” rule.
dotnet/src/Microsoft.Agents.AI/Harness/FileAccess/FileAccessProvider.cs Documents approved tool names and collision risk for file-access auto-approval rules.
dotnet/src/Microsoft.Agents.AI.Tools.Shell/ShellExecutor.cs Adds warning about choosing shell tool names that won’t collide with auto-approved names.
dotnet/src/Microsoft.Agents.AI.Tools.Shell/LocalShellExecutor.cs Adds warning about shell tool name collisions bypassing approval prompts.
dotnet/src/Microsoft.Agents.AI.Tools.Shell/DockerShellExecutor.cs Adds warning about shell tool name collisions bypassing approval prompts.
dotnet/src/Microsoft.Agents.AI.Harness/HarnessAgentOptions.cs Adds warning about ShellToolName collisions with auto-approval rules.
dotnet/samples/02-agents/Harness/Harness_Step03_DataProcessing/README.md Adds sample-level guidance to avoid tool-name collisions when using read-only auto-approval.
dotnet/samples/02-agents/Harness/BuildYourOwnClaw/Claw_Step02_WorkingWithData/README.md Adds sample-level warning about name collisions with auto-approved file-access tools.
dotnet/samples/02-agents/AgentSkills/Agent_Step07_SkillsAutoApproval/README.md Adds sample-level warning about collisions across skills/file-access/shell tools.

Comment thread dotnet/src/Microsoft.Agents.AI/Harness/ToolApproval/ToolApprovalAgentOptions.cs Outdated
Comment thread dotnet/src/Microsoft.Agents.AI.Tools.Shell/ShellExecutor.cs Outdated
Comment thread dotnet/src/Microsoft.Agents.AI.Tools.Shell/LocalShellExecutor.cs Outdated
Comment thread dotnet/src/Microsoft.Agents.AI.Tools.Shell/DockerShellExecutor.cs Outdated
Comment thread dotnet/src/Microsoft.Agents.AI.Harness/HarnessAgentOptions.cs Outdated

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Automated Code Review

Reviewers: 5 | Confidence: 85%

✓ Correctness

This PR adds documentation warnings about tool-name collision risks in auto-approval rules. All changes are XML doc comments, README additions, and no code logic is modified. The documented tool names and set memberships are verified to be accurate against the actual source constants and HashSet definitions.

✓ Security Reliability

This PR is purely a documentation change adding security warnings about tool-name collisions in auto-approval rules. All documented claims (tool names, matching behavior, and rule semantics) have been verified against the actual source code and are accurate. The warnings correctly describe a real defense-in-depth concern: auto-approval rules match solely by tool name, so a configurable-name tool (like the shell executor) could inadvertently be auto-approved if assigned a colliding name. No code logic is changed, no new vulnerabilities are introduced, and the documentation accurately reflects the existing behavior.

✓ Test Coverage

This PR is entirely a documentation change — adding XML doc comments and README security warnings about tool-name collisions in auto-approval rules. No runtime code was modified. The existing test suite already verifies the name-based matching behavior (approving known names, rejecting unknown ones) that these warnings document. No new test coverage is needed for documentation-only changes.

✓ Failure Modes

This PR adds only documentation: XML doc comments and README warnings about tool-name collision risks in auto-approval rules. There are no functional code changes — no new logic, no modified behavior, no altered control flow. No failure modes are introduced or preserved by this diff.

✓ Design Approach

The PR’s overall direction is sound, but one of the new central API warnings overstates how auto-approval works. ToolApprovalAgentOptions.AutoApprovalRules accepts arbitrary predicates over FunctionCallContent, so documenting the feature as inherently name-only misdescribes the design and may steer users away from safer argument-aware rules. I did not find a code-path or test-backed reason to request changes beyond tightening that wording.

Suggestions

  • In dotnet/src/Microsoft.Agents.AI/Harness/ToolApproval/ToolApprovalAgentOptions.cs:44-48, scope the warning to the built-in name-based rules rather than all AutoApprovalRules, because the contract at ToolApprovalAgentOptions.cs:34-36 allows rules to inspect the full FunctionCallContent, and ToolApprovalAgent.AllToolsAutoApprovalRule is explicitly name-agnostic at ToolApprovalAgent.cs:95-96.

Automated review by westey-m's agents

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

Labels

documentation Usage: [Issues, PRs], Target: documentation in the code base and learn docs .NET Usage: [Issues, PRs], Target: .Net

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants