Skip to content

Python: Add name collision warnings for auto-approvals#7090

Merged
moonbox3 merged 2 commits into
microsoft:mainfrom
westey-m:python-autoapproval-name-collision-warnings
Jul 14, 2026
Merged

Python: Add name collision warnings for auto-approvals#7090
moonbox3 merged 2 commits into
microsoft:mainfrom
westey-m:python-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 12:55
@giles17 giles17 added documentation Usage: [Issues, PRs], Target: documentation in the code base and learn docs python Usage: [Issues, PRs], Target: Python labels Jul 13, 2026
@github-actions github-actions Bot changed the title Add name collision warnings for auto-approvals Python: 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 adds explicit security documentation warning that built-in tool auto-approval rules can match by tool name and may unintentionally auto-approve unrelated tools if names collide, encouraging users to avoid reserved tool-name collisions as a defense-in-depth measure.

Changes:

  • Added name-collision security warnings to the skills auto-approval and harness sample READMEs.
  • Added .. warning:: documentation to built-in auto-approval rule docstrings (SkillsProvider/FileAccessProvider) and to ToolApprovalMiddleware guidance.
  • Documented the same collision risk on the configurable shell tool as_function(name=...) API in both local and Docker implementations.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.

Show a summary per file
File Description
python/samples/02-agents/skills/skills_auto_approval/README.md Adds a security callout warning about reserved tool-name collisions for skills auto-approval rules.
python/samples/02-agents/harness/README.md Documents collision risk for built-in auto-approval rules in the harness security considerations section.
python/samples/02-agents/harness/build_your_own_claw/README.md Adds an in-sample warning about auto-approval matching by tool name for file-access tools.
python/packages/tools/agent_framework_tools/shell/_tool.py Adds docstring warning that caller-chosen shell tool names can collide with auto-approval rules.
python/packages/tools/agent_framework_tools/shell/_docker.py Adds the same docstring warning for DockerShellTool’s as_function naming.
python/packages/core/agent_framework/_skills.py Adds .. warning:: blocks documenting collision risk for SkillsProvider auto-approval rules.
python/packages/core/agent_framework/_harness/_tool_approval.py Adds warning guidance in ToolApprovalMiddleware docs about name-based auto-approval collisions.
python/packages/core/agent_framework/_harness/_file_access.py Adds .. warning:: blocks documenting collision risk for FileAccessProvider auto-approval rules.

@github-actions

Copy link
Copy Markdown
Contributor

Flagged issue

Docs-only here leaves the approval bypass fully intact: ToolApprovalMiddleware auto-approves any request for which a callback returns true (_tool_approval.py:620-629), the built-in file-access and skills rules still match only on function_call.name (_file_access.py:1379-1382, _skills.py:198-1991), and LocalShellTool.as_function still lets calers choose any exposed name (shell/_tool.py:278-319). That means LocalShellTool().as_function(name="file_access_read") will still be auto-approved without a prompt when FileAccessProvider.read_only_tools_auto_approval_rule is installed. The repo already uses a stronger pattern for reserved names in progressive MCP mode by actively rejecting/omitting colliding tools instead of warning about them (_mcp.py:885-895, _mcp.py:1030-1035).


Source: automated DevFlow PR review

@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 is purely documentation: it adds security warnings about tool-name collision risks in auto-approval rules across docstrings, README files, and the shell tool's as_function method. All documented tool names were verified against the actual code constants and match exactly. The warnings accurately describe the name-based matching behavior of the approval rules. No correctness issues found.

✓ Security Reliability

This PR adds documentation-only security warnings about tool-name collision risks in auto-approval rules. The warnings are accurate: the existing rules approve local tool calls by name (plus a server_label check to exclude hosted tools), so a local tool registered under a colliding name could be auto-approved. No code logic is changed, and no security or reliability issues are introduced by these documentation additions.

✓ Test Coverage

This PR adds documentation-only changes (docstring warnings and README updates) about tool-name collision risks in auto-approval rules. No behavioral code is changed. Existing tests cover the auto-approval rules' intended behavior well, including rejection of hosted tools with matching names. However, the specific collision scenario being warned about — a local tool (e.g., shell tool) given a custom name matching an auto-approved tool name — is not tested anywhere. Since the PR is purely documentation, no blocking issues are raised, but a demonstrative test would strengthen the defense-in-depth intent.

✓ Failure Modes

This PR is entirely documentation: it adds security warnings to docstrings and README files about tool-name collision risks with auto-approval rules. No functional code is changed — only .. warning:: blocks in Python docstrings and markdown callouts in sample READMEs. There are no failure modes introduced by these changes.

✗ Design Approach

The PR improves documentation, but it leaves the underlying approval-boundary collision unmitigated. The current design still silently auto-approves any local tool whose name matches a built-in auto-approval rule, including caller-renamed shell tools, so documenting the hazard alone does not address the security failure mode this PR describes.

Flagged Issues

  • Docs-only here leaves the approval bypass fully intact: ToolApprovalMiddleware auto-approves any request for which a callback returns true (_tool_approval.py:620-629), the built-in file-access and skills rules still match only on function_call.name (_file_access.py:1379-1382, _skills.py:198-1991), and LocalShellTool.as_function still lets calers choose any exposed name (shell/_tool.py:278-319). That means LocalShellTool().as_function(name="file_access_read") will still be auto-approved without a prompt when FileAccessProvider.read_only_tools_auto_approval_rule is installed. The repo already uses a stronger pattern for reserved names in progressive MCP mode by actively rejecting/omitting colliding tools instead of warning about them (_mcp.py:885-895, _mcp.py:1030-1035).

Suggestions

  • Enforce reserved auto-approved names at runtime rather than only documenting them — for example, reject tool registration or middleware setup when a local tool requiring approval is exposed under a name claimed by one of the built-in auto-approval rules, mirroring the collision handling used for progressive MCP loader names (_mcp.py:885-895, _mcp.py:1030-1035).

Automated review by westey-m's agents

@github-actions

github-actions Bot commented Jul 13, 2026

Copy link
Copy Markdown
Contributor

Python Test Coverage

Python Test Coverage Report •
FileStmtsMissCoverMissing
packages/core/agent_framework
   _skills.py10833996%314, 584, 1102, 1117, 1119–1120, 1486–1487, 1729, 1758, 2270, 2459, 2967–2968, 3065, 3073, 3078, 3081, 3086, 3106, 3118, 3123, 3218, 3226, 3231, 3234, 3239, 3259, 3268, 3273, 3539–3540, 4092, 4340–4341, 4368–4369, 4376–4377
packages/core/agent_framework/_harness
   _file_access.py6144592%187, 218, 281, 391, 393, 405, 409, 437, 450–451, 458–465, 469, 473, 839–840, 866, 870, 924–926, 948–951, 1445–1446, 1457–1458, 1470–1471, 1485–1486, 1509–1510, 1525, 1530–1531, 1560
   _tool_approval.py3584487%69, 72, 77, 115, 132, 135, 138, 196–197, 216, 245, 253, 264, 282–287, 299, 313, 316, 336, 387, 410, 412–413, 445, 455, 470, 472–473, 475–476, 501–502, 523–525, 563–564, 614, 618, 623
packages/tools/agent_framework_tools/shell
   _docker.py23212446%135, 143–144, 381–383, 391–400, 406, 412, 416–421, 423–426, 429–430, 433, 448–456, 458, 460–465, 467, 473–474, 498–506, 508–509, 514–518, 524, 533–542, 547, 555–558, 564–568, 572–574, 579, 584, 586–589, 601, 616, 621–623, 626, 635, 637, 645–652, 654–657, 663–664, 667, 702–706
   _tool.py1041585%156, 158, 179–180, 182, 201, 254–255, 263, 306–310, 331
TOTAL44212524488% 

Python Unit Test Overview

Tests Skipped Failures Errors Time
8890 33 💤 0 ❌ 0 🔥 1m 57s ⏱️

@moonbox3 moonbox3 added this pull request to the merge queue Jul 14, 2026
@github-merge-queue github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Jul 14, 2026
@moonbox3 moonbox3 enabled auto-merge July 14, 2026 06:28
@moonbox3 moonbox3 added this pull request to the merge queue Jul 14, 2026
Merged via the queue into microsoft:main with commit 0ceca9a Jul 14, 2026
38 checks passed
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 python Usage: [Issues, PRs], Target: Python

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants