Repository navigation
Python: compose GitHub Copilot session hooks correctly - #9649
Conversation
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟢 Approval recommended
The narrow change preserves hook precedence and approval behavior, with regression coverage for both run modes and no identified blocking issues.
No open issues 🎉
What changed in this PR
This PR fixes GitHub Copilot session-hook composition so unrelated caller hooks no longer suppress Agent Framework’s default tool-approval hook.
Changes:
- Preserve caller-provided
on_pre_tool_useprecedence while merging unrelated hooks with default approval behavior. - Type the merged hook dictionary as
SessionHooksand document the composition rules. - Extend the forwarding regression test to verify approval decisions in streaming and non-streaming runs.
| File | Description |
|---|---|
| python/packages/github_copilot/tests/test_github_copilot_agent.py | Tests unrelated-hook preservation and default approval in both run modes. |
| python/packages/github_copilot/README.md | Documents preservation of unrelated session hooks. |
| python/packages/github_copilot/agent_framework_github_copilot/_agent.py | Corrects hook composition while preserving custom pre-tool-use precedence. |
Review effort: Balanced
There was a problem hiding this comment.
MAF Automated Review — Iteration 1
Result: No findings
Scope: full PR (1 commit(s)): 034106253689
Model: gpt-5.6-sol
Overview
The change composes unrelated caller session hooks with the framework-managed approval hook while preserving caller on_pre_tool_use precedence. A fresh typed hook dictionary, deny-by-default permission handling, deprecated-path guards, and parameterized streaming/non-streaming coverage constrain the changed behavior. No publishable Critical, High, or Medium defect was established.
Reviewed the supplied pull-request change set across correctness, security/reliability, architecture, and failure behavior.
No publishable findings remained after source verification for this scope.
Jose Alvarez (jpalvarezl)
left a comment
There was a problem hiding this comment.
Reviewed the hook composition, session creation and resumption paths, sync/async callback contract, approval precedence, deprecated callback compatibility, and streaming/non-streaming regression coverage. No blocking issues found.
Motivation & Context
GitHub Copilot sessions can combine caller-configured lifecycle hooks with Agent Framework's default tool-approval hook. Hook composition should preserve unrelated caller hooks without changing the established precedence of a caller-provided
on_pre_tool_usehandler.Description & Review Guide
SessionHooks, and extend the existing forwarding regression test across streaming and non-streaming runs.on_pre_tool_usehandlers retain precedence, while other lifecycle hooks no longer replace provider-managed approval behavior.Related Issue
N/A - privately tracked security issue. Public issue details are intentionally omitted.
AI Assistance
AI assistance details: Implementation refinement, regression tests, documentation, and PR description.
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.