Repository navigation
Python: Align GitHub Copilot provider function approval to use SDK on_pre_tool_use hook - #6750
Giles Odigwe (giles17) merged 5 commits into
Conversation
Sorry, something went wrong.
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>
Python Unit Test Overview
|
||||||||||||||||||||||||||||||
Sorry, something went wrong.
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_approval enforcement and shifted approval-required tool gating to an SDK on_pre_tool_use hook that returns "ask" for approval_mode="always_require" tools.
- Added on_pre_tool_use to GitHubCopilotOptions, wired hooks= 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. |
Sorry, something went wrong.
There was a problem hiding this comment.
Automated Code Review
Reviewers: 5 | Confidence: 90% | Result: All clear
Reviewed: Correctness, Security Reliability, Test Coverage, Failure Modes, Design Approach
Automated review by giles17's agents
Sorry, something went wrong.
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>
There was a problem hiding this comment.
Automated Code Review
Reviewers: 5 | Confidence: 86% | Result: All clear
Reviewed: Correctness, Security Reliability, Test Coverage, Failure Modes, Design Approach
Automated review by giles17's agents
Sorry, something went wrong.
Per PR review feedback, keep the on_function_approval callback working (still enforced in the tool handler for approval_mode='always_require' tools) but emit a DeprecationWarning at construction, so existing users get a signal rather than a silent behavior change. The default on_pre_tool_use ask-hook is not installed when on_function_approval is set, avoiding double-gating. Precedence: user on_pre_tool_use > on_function_approval > default ask-hook. Adds tests for the deprecated path and documents it in the package README. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Automated Code Review
Reviewers: 5 | Confidence: 85%
✓ Correctness
The PR's production code correctly implements the on_pre_tool_use hook precedence logic and deprecation path. However, the test file has a structural defect: the class TestGitHubCopilotAgentErrorHandling: line was accidentally deleted during the refactoring of the adjacent test class. This causes all error-handling tests (e.g., test_start_raises_on_client_error) to be incorrectly absorbed into TestGitHubCopilotAgentDeprecatedFunctionApproval.
✓ Security Reliability
The PR correctly implements secure-by-default tool approval through the SDK's native on_pre_tool_use hook. The deny-all default permission handler ensures approval-required tools remain gated unless explicitly approved. The precedence logic is sound and the deprecated path is preserved for backward compatibility. One minor reliability concern: the falsy-coalescing or on line 927 could theoretically suppress a runtime hook that is provided but falsy, though this is unlikely with callable hooks in practice.
✓ Test Coverage
The test refactoring has a structural defect: the class TestGitHubCopilotAgentErrorHandling: declaration was accidentally dropped, causing its tests to be absorbed into TestGitHubCopilotAgentDeprecatedFunctionApproval. Additionally, two previously-existing tests for the deprecated callback's exception-handling and async-callback paths were removed without replacement, leaving those still-active code paths untested.
✓ Failure Modes
No actionable issues found in this dimension.
✓ Design Approach
The new hook-based design is close, but one precedence path is still wired incorrectly: on_function_approval is enforced in the tool handler whenever it is configured, even when a caller-suplied on_pre_tool_use hook is supposed to take precedence. That means the documented and tested precedence order is not actually honored for approval-required tools.
Automated review by giles17's agents
Sorry, something went wrong.
Per automated review feedback, instead of a precedence ordering between the deprecated on_function_approval callback and the new on_pre_tool_use hook (which silently double-gated when both were set), raise ValueError if both are supplied - at construction (both in default_options) or per run (per-run on_pre_tool_use with a construction-time on_function_approval). This matches the repo convention for deprecated-vs-new params (see _workflows/_workflow.py) and removes the flag-threading. Updates tests and the package README. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Automated Code Review
Reviewers: 5 | Confidence: 88% | Result: All clear
Reviewed: Correctness, Security Reliability, Test Coverage, Failure Modes, Design Approach
Automated review by giles17's agents
Sorry, something went wrong.
Motivation & Context
The Python agent-framework-github-copilot provider enforced function approval itself: every FunctionTool was wrapped in an SDK tool-handler that, for tools declared with approval_mode="always_require", awaited an on_function_approval callback 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 to on_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?
What is the impact of these changes?
What do you want reviewers to focus on?
Related Issue
Fixes #6746
Contribution Checklist