diff --git a/python/packages/github_copilot/README.md b/python/packages/github_copilot/README.md index 1dbdbe05a46..25dec937a64 100644 --- a/python/packages/github_copilot/README.md +++ b/python/packages/github_copilot/README.md @@ -20,7 +20,8 @@ When you register a `FunctionTool` declared with `approval_mode="always_require" do **not** supply your own `on_pre_tool_use` hook, `GitHubCopilotAgent` installs a default `on_pre_tool_use` hook that returns `"ask"` for that tool and defers (`None`) for all other tools. The `"ask"` decision routes to your `on_permission_request` handler, where you -approve or deny the call: +approve or deny the call. Other session hooks, such as `on_post_tool_use` or +`on_session_start`, are preserved alongside this default approval hook: ```python from agent_framework import tool diff --git a/python/packages/github_copilot/agent_framework_github_copilot/_agent.py b/python/packages/github_copilot/agent_framework_github_copilot/_agent.py index 64a9f125e42..4dfdd70894a 100644 --- a/python/packages/github_copilot/agent_framework_github_copilot/_agent.py +++ b/python/packages/github_copilot/agent_framework_github_copilot/_agent.py @@ -1505,17 +1505,16 @@ def _build_session_hooks( ``approval_mode="always_require"`` is delegated to the Copilot SDK's native ``on_pre_tool_use`` hook: - - If the caller supplies their own session hooks -- either the SDK-native - ``hooks`` dict or the convenience ``on_pre_tool_use`` handler (via per-run - ``options`` or ``default_options``) -- those take precedence and are used - as-is. When both are given, the explicit ``hooks`` dict wins for any key it - defines and the ``on_pre_tool_use`` shortcut fills in that key otherwise. A - warning is logged naming any approval-required tool that will therefore not - be automatically gated, since the caller's hooks are responsible for - enforcing approval. + - If the caller supplies their own ``on_pre_tool_use`` handler -- either in + the SDK-native ``hooks`` dict or through the convenience option (via per-run + ``options`` or ``default_options``) -- it takes precedence. When both are + given, the explicit ``hooks`` dict wins for any key it defines and the + shortcut fills in that key otherwise. A warning names any approval-required + tool that the caller's pre-tool-use handler is responsible for gating. - Otherwise, when any approval-required tool is present, a default hook is installed that returns ``"ask"`` for those tools (routing the decision to ``on_permission_request``) and defers (``None``) for all other tools. + Unrelated caller hooks are preserved and do not replace this default. - The default hook is **not** installed when the deprecated ``on_function_approval`` callback is configured: in that case approval is enforced inside the tool handler (see :meth:`_tool_to_copilot_tool`) to @@ -1532,12 +1531,12 @@ def _build_session_hooks( The hooks to register for the session, or ``None`` if none are needed. """ user_hook: PreToolUseHandler | None = options.get("on_pre_tool_use") or self._on_pre_tool_use - caller_hooks: Mapping[str, Any] | None = options.get("hooks") + caller_hooks: SessionHooks | None = options.get("hooks") # Combine caller-provided hooks: the SDK-native ``hooks`` dict plus the # convenience ``on_pre_tool_use`` shortcut. The explicit dict wins for the # keys it defines; the shortcut only fills in ``on_pre_tool_use`` otherwise. - combined: dict[str, Any] = {} + combined: SessionHooks = {} if user_hook is not None: combined["on_pre_tool_use"] = user_hook if caller_hooks: @@ -1547,7 +1546,7 @@ def _build_session_hooks( tool.name for tool in all_tools if isinstance(tool, FunctionTool) and tool.approval_mode == "always_require" } - if combined: + if combined.get("on_pre_tool_use") is not None: if approval_required_names: logger.warning( "Custom session hooks are configured, so %d approval-required tool(s) (%s) " @@ -1556,15 +1555,15 @@ def _build_session_hooks( len(approval_required_names), ", ".join(sorted(approval_required_names)), ) - return cast("SessionHooks", combined) + return combined if not approval_required_names: - return None + return combined or None # The deprecated on_function_approval callback enforces approval in the tool # handler; don't also install the default ask-hook (which would double-gate). if self._function_approval_handler is not None: - return None + return combined or None def default_pre_tool_use( hook_input: Mapping[str, Any], @@ -1580,7 +1579,8 @@ def default_pre_tool_use( } return None - return {"on_pre_tool_use": default_pre_tool_use} + combined["on_pre_tool_use"] = default_pre_tool_use + return combined def _session_run_lock(self, service_session_id: str) -> asyncio.Lock: """Return the lock that serializes runs on one Copilot service session.""" diff --git a/python/packages/github_copilot/tests/test_github_copilot_agent.py b/python/packages/github_copilot/tests/test_github_copilot_agent.py index 60debf9c74e..d6da1598b6c 100644 --- a/python/packages/github_copilot/tests/test_github_copilot_agent.py +++ b/python/packages/github_copilot/tests/test_github_copilot_agent.py @@ -2959,26 +2959,47 @@ def dangerous(path: str) -> str: hooks = agent._build_session_hooks([dangerous], {"on_pre_tool_use": runtime_hook}) # type: ignore[reportPrivateUsage] assert hooks == {"on_pre_tool_use": runtime_hook} - async def test_default_hook_forwarded_to_create_session( + @pytest.mark.parametrize("streaming", [False, True]) + async def test_default_hook_forwarded_with_unrelated_hooks( self, mock_client: MagicMock, mock_session: MagicMock, assistant_message_event: SessionEvent, + assistant_delta_event: SessionEvent, + session_idle_event: SessionEvent, + streaming: bool, ) -> None: - """An always_require tool causes the default hook to be forwarded to the SDK session.""" + """Unrelated hooks are forwarded without suppressing approval in either run path.""" mock_session.send_and_wait.return_value = assistant_message_event + post_tool_use = MagicMock() @tool(approval_mode="always_require") def dangerous(path: str) -> str: """A tool that requires human approval.""" return f"deleted {path}" - agent = GitHubCopilotAgent(client=mock_client, tools=[dangerous]) - await agent.run("hello") + agent = GitHubCopilotAgent( + client=mock_client, + tools=[dangerous], + default_options=cast(Any, {"hooks": {"on_post_tool_use": post_tool_use}}), + ) + if streaming: + events = [assistant_delta_event, session_idle_event] + + def mock_on(handler: Any) -> Any: + for event in events: + handler(event) + return lambda: None - hooks = mock_client.create_session.call_args.kwargs["hooks"] - assert hooks is not None - assert "on_pre_tool_use" in hooks + mock_session.on = mock_on + await agent.run("hello", stream=True).get_final_response() + else: + await agent.run("hello") + + session_hooks = mock_client.create_session.call_args.kwargs["hooks"] + assert session_hooks["on_post_tool_use"] is post_tool_use + decision = session_hooks["on_pre_tool_use"](pre_tool_use_input("dangerous"), {"session_id": "s"}) + assert decision["permissionDecision"] == "ask" class TestGitHubCopilotAgentDeprecatedFunctionApproval: