Python: Align GitHub Copilot provider function approval to use SDK on_pre_tool_use hook#6750
Open
giles17 wants to merge 2 commits into
Open
Python: Align GitHub Copilot provider function approval to use SDK on_pre_tool_use hook#6750giles17 wants to merge 2 commits into
giles17 wants to merge 2 commits into
Conversation
Replace the bespoke on_function_approval enforcement in the GitHub Copilot provider with the Copilot SDK's native on_pre_tool_use hook. When no caller hook is supplied, a default hook returns 'ask' for approval_mode='always_require' tools (routed to on_permission_request) and defers others; a caller-supplied on_pre_tool_use takes precedence and logs a warning for any unenforced approval tool. Fixes microsoft#6746 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
moonbox3
added
documentation
Contributor
Contributor
There was a problem hiding this comment.
Pull request overview
Aligns the Python agent-framework-github-copilot provider’s function-tool approval gating with the GitHub Copilot SDK’s native on_pre_tool_use hook, removing the bespoke on_function_approval path so behavior matches the .NET provider and the SDK’s intended flow.
Changes:
- Removed agent-level
on_function_approvalenforcement and shifted approval-required tool gating to an SDKon_pre_tool_usehook that returns"ask"forapproval_mode="always_require"tools. - Added
on_pre_tool_usetoGitHubCopilotOptions, wiredhooks=into session create/resume, and implemented precedence + warning behavior when callers supply their own hook. - Updated unit tests, README guidance, and the function-approval sample to reflect the new approval mechanism via
on_permission_request.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| python/samples/02-agents/providers/github_copilot/github_copilot_with_function_approval.py | Updates the sample to demonstrate SDK-native tool approval via on_pre_tool_use → on_permission_request. |
| python/packages/github_copilot/tests/test_github_copilot_agent.py | Reworks tests to validate default hook behavior, precedence/warnings, and forwarding of hooks to SDK sessions. |
| python/packages/github_copilot/README.md | Documents the new approval flow and precedence rules for custom on_pre_tool_use. |
| python/packages/github_copilot/agent_framework_github_copilot/_agent.py | Implements _build_session_hooks and forwards hooks to create_session/resume_session; removes bespoke approval callback machinery. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Use a complete PreToolUseHookInput in on_pre_tool_use hook tests so pyright/pyrefly/ty/zuban no longer report missing required TypedDict keys. Restore load_dotenv() in the function-approval sample for consistency with the other GitHub Copilot samples (PR review feedback). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation & Context
The Python
agent-framework-github-copilotprovider enforced function approval itself: everyFunctionToolwas wrapped in an SDK tool-handler that, for tools declared withapproval_mode="always_require", awaited anon_function_approvalcallback before executing (denying by default when none was configured).The GitHub Copilot SDK already exposes a native pre-execution hook (
on_pre_tool_use) that can return"ask"and route the decision toon_permission_request. The AF-level callback therefore duplicated SDK functionality and diverged from the .NET provider. This change adopts the SDK-native model so the two providers behave consistently (follow-up to #6671 / #6674).Description & Review Guide
What are the major changes?
FunctionTooldeclared withapproval_mode="always_require"is registered and the caller has not supplied their ownon_pre_tool_usehook,GitHubCopilotAgentinstalls a defaulton_pre_tool_usehook that returns"ask"for those tools (routing the decision toon_permission_request) and defers (None) for all other tools.on_pre_tool_use(viadefault_optionsor per-runoptions), it takes precedence and the caller owns approval handling; the agent logs a warning naming any approval-required tool that will therefore not be auto-gated._build_session_hooksand passed via the SDK's existinghooks=parameter tocreate_session/resume_session(computed where the full tool set is known).on_function_approvaloption, theFunctionApprovalCallbacktype, the_resolve_function_approvalhelper, the per-run rejection guards, and the approval branch in the tool handler.on_pre_tool_useoption toGitHubCopilotOptions; updated tests, the package README, and the function-approval sample.What is the impact of these changes?
on_permission_request. With the default deny-all permission handler they remain secure-by-default. Non-approval tools are unaffected.on_function_approval: they now configure approval viaon_permission_request(and optionallyon_pre_tool_use). The provider is pre-release, so this is not a breaking change to a stable API.What do you want reviewers to focus on?
on_pre_tool_use(override + warning), and that the default hook only asks foralways_requiretools while deferring everything else.Related Issue
Fixes #6746
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.