Python: [BREAKING] Make all SkillsProvider tools require approval by default#6754
Python: [BREAKING] Make all SkillsProvider tools require approval by default#6754giles17 wants to merge 5 commits into
Conversation
…default All tools exposed by SkillsProvider (load_skill, read_skill_resource, run_skill_script) now require approval by default. Previously only run_skill_script could be gated, and only when require_script_approval=True. - Register all three tools with approval_mode="always_require" - Add read_only_tools_auto_approval_rule and all_tools_auto_approval_rule static rules plus tool-name constants (mirrors FileAccessProvider) - Remove the require_script_approval option from __init__ and from_paths - Add skills_auto_approval sample; update script_approval sample/docs Closes microsoft#6728 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
moonbox3
added
documentation
Python Test Coverage Report •
Python Unit Test Overview
|
||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Pull request overview
This PR makes Python SkillsProvider secure-by-default by requiring host approval for all skill tools (load_skill, read_skill_resource, run_skill_script), and adds opt-in auto-approval rules to support unattended scenarios (mirroring the existing FileAccessProvider approval pattern and the .NET change in #6729).
Changes:
- Register all
SkillsProvidertools withapproval_mode="always_require"and remove therequire_script_approvalconfiguration option (breaking change). - Add
SkillsProvider.read_only_tools_auto_approval_ruleandSkillsProvider.all_tools_auto_approval_ruleplus tool-name constants for rule authoring. - Add/update samples + docs to demonstrate the new approval-by-default model and auto-approval configuration.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| python/packages/core/agent_framework/_skills.py | Makes all skill tools approval-required by default; adds auto-approval rules and tool-name constants. |
| python/packages/core/tests/core/test_skills.py | Updates/extends tests to validate approval-by-default and the new rule/constant APIs. |
| python/packages/core/AGENTS.md | Documents the new approval-by-default behavior and how to run unattended via ToolApprovalMiddleware. |
| python/samples/02-agents/skills/skills_auto_approval/skills_auto_approval.py | New sample showing read-only auto-approval + manual approval for script execution. |
| python/samples/02-agents/skills/skills_auto_approval/README.md | New sample documentation for configuring auto-approval rules for skill tools. |
| python/samples/02-agents/skills/script_approval/script_approval.py | Updates the manual approval sample to reflect approval-by-default and removal of require_script_approval. |
| python/samples/02-agents/skills/script_approval/README.md | Updates documentation to the new “skill tool approval” model and correct env var names. |
| python/samples/02-agents/skills/README.md | Adds the new skills_auto_approval sample and updates descriptions accordingly. |
- Collect a response for every approval request and send them in a single agent.run so the approval loop always makes progress (no infinite loop when a request lacks a function_call); reject non-function requests instead of skipping them. Applied to both the skills_auto_approval and script_approval samples. - Extract ToolApprovalMiddleware into a local variable in skills_auto_approval for readability. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Automated Code Review
Reviewers: 5 | Confidence: 92%
✓ Correctness
This PR cleanly implements the breaking change to make all SkillsProvider tools require approval by default. The implementation follows the established FileAccessProvider pattern faithfully: ClassVar frozensets are properly initialized, the
_is_local_tool_callhelper correctly guards against hosted-tool name collisions viaadditional_properties.get("server_label"), and therequire_script_approvalparameter is fully removed from all call sites. The auto-approval rules use short-circuitandcorrectly, the TYPE_CHECKING import withfrom __future__ import annotationsis sound, and tests comprehensively validate the new behavior. No correctness issues found.
✓ Security Reliability
The PR correctly implements secure-by-default approval for all SkillsProvider tools. The implementation mirrors the existing FileAccessProvider pattern, tools are unconditionally registered with approval_mode='always_require', and the auto-approval rules properly guard against hosted-tool name collisions via server_label checks. The additional_properties attribute is always initialized as a dict so .get() calls are safe. The sample code handles edge cases (no function_call) properly, preventing infinite loops. No security or reliability issues found.
✓ Test Coverage
Test coverage for this PR is thorough and follows the same pattern as the existing FileAccessProvider tests. The auto-approval rules are tested as pure functions with positive cases, negative cases, and the server_label guard. The breaking removal of require_script_approval is properly reflected by removing all references and updating affected tests. The one minor gap is the absence of an integration test showing the rules work end-to-end when wired to ToolApprovalMiddleware with SkillsProvider, but this matches the project's existing pattern where FileAccessProvider similarly relies on generic middleware tests plus isolated rule tests.
✓ Failure Modes
The production code changes are clean and correct. The
require_script_approvalparameter is fully removed with no orphaned references. The auto-approval rules safely handle edge cases:additional_propertiesis always a dict (confirmed at_types.py:534-536), so_is_local_tool_callwon't crash;None in frozenset(...)correctly returns False; and ClassVar frozenset initialization references already-defined class-level strings. The_create_toolsmethod unconditionally setsapproval_mode="always_require"on all three tools. No silent failure modes or lost errors were identified in the changed production code.
✗ Design Approach
I found one design-level issue: the secure-by-default change is implemented consistently inside
SkillsProvider, but the PR does not carry that new approval contract through the other published skills samples that still useSkillsProviderwithout any approval handling. That leaves concrete sample code paths returning approval requests instead of the documented final answers.
Flagged Issues
- The new
approval_mode="always_require"default strands existing skills samples that construct a plainAgent(..., context_providers=[SkillsProvider(...)])and immediately printawait agent.run(...). Specifically,python/samples/02-agents/skills/code_defined_skill/code_defined_skill.py:147-158andpython/samples/02-agents/skills/file_based_skill/file_based_skill.py:68-79do not installToolApprovalMiddlewareor run a manual approval loop. With the new defaults, those flows now surfacefunction_approval_requestcontent instead of the documented final answers. Please update the remaining skills samples to add approval handling.
Automated review by giles17's agents
|
Flagged issue The new Source: automated DevFlow PR review |
The secure-by-default change makes all SkillsProvider tools require approval, which left the other skills samples emitting approval requests instead of the documented answers. Add ToolApprovalMiddleware with the all-tools auto-approval rule (and a session, which the middleware requires) so these samples run unattended as before: - code_defined_skill, file_based_skill, class_based_skill, mixed_skills, skill_filtering, mcp_based_skill - providers/foundry/foundry_chat_client_with_toolbox_skills The dedicated script_approval (manual) and skills_auto_approval (selective) samples continue to demonstrate interactive approval handling. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Apply maintainer suggestions dropping "host" from the skill-approval docstrings, and align the matching SkillsProvider docstring/AGENTS.md note for consistency. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Motivation & Context
Skill tools previously ran without any approval gate by default: only
run_skill_scriptcould be gated, and only when the caller opted in viarequire_script_approval=True. The content-reading tools (load_skill,read_skill_resource) were never gated. This is a port of the .NET change (#6729) that makesSkillsProvidersecure-by-default, so an agent cannot load skill content or execute skill scripts from a (potentially untrusted) source without host approval, while still allowing unattended runs through explicit auto-approval rules.This follows the existing Python
FileAccessProviderapproval pattern rather than copying the .NET API surface.Description & Review Guide
What are the major changes?
SkillsProvider(load_skill,read_skill_resource,run_skill_script) are now registered withapproval_mode="always_require"unconditionally.SkillsProvider.read_only_tools_auto_approval_rule(approvesload_skill+read_skill_resource) andSkillsProvider.all_tools_auto_approval_rule(approves all three) — for use withToolApprovalMiddleware(auto_approval_rules=[...]). Both reject calls carrying aserver_labelso they stay scoped to this provider's local tools.LOAD_SKILL_TOOL_NAME,READ_SKILL_RESOURCE_TOOL_NAME,RUN_SKILL_SCRIPT_TOOL_NAME.require_script_approvaloption fromSkillsProvider.__init__andSkillsProvider.from_paths.skills_auto_approvalsample; updated thescript_approvalsample to the approval-by-default model; updated skills sample READMEs andpackages/core/AGENTS.md.What is the impact of these changes?
require_script_approvalis gone. Existing code passing it must remove it; skill tools now require approval by default, and unattended runs must opt in via an auto-approval rule onToolApprovalMiddleware.What do you want reviewers to focus on?
server_labelguard, mirroringFileAccessProvider) is the right Python shape versus the .NETFunc<>properties.Related Issue
Fixes #6728
Contribution Checklist
breaking changelabel (or add "[BREAKING]" to the title prefix, before or after any language prefix) — a workflow keeps the label and title prefix in sync automatically.