From 36f90ea4e5b254f854710145a4fa82918a7c7972 Mon Sep 17 00:00:00 2001 From: Kazuhiro Sera Date: Wed, 5 Aug 2026 09:22:29 +0900 Subject: [PATCH] fix(approvals): honor resolved status before policy checks Treat stored approval decisions as authoritative across execution, resume planning, Realtime, and multi-operation apply-patch paths. Co-authored-by: Henry Su --- src/agents/realtime/session.py | 30 +- src/agents/run_internal/tool_actions.py | 156 +++--- src/agents/run_internal/tool_execution.py | 33 +- src/agents/run_internal/tool_planning.py | 39 +- src/agents/run_internal/turn_resolution.py | 23 +- .../capabilities/tools/apply_patch_tool.py | 8 +- tests/mcp/test_mcp_approval.py | 3 +- tests/realtime/test_session.py | 180 +++++++ .../capabilities/test_apply_patch_tool.py | 68 +++ tests/test_hitl_error_scenarios.py | 488 +++++++++++++++++- 10 files changed, 919 insertions(+), 109 deletions(-) diff --git a/src/agents/realtime/session.py b/src/agents/realtime/session.py index 43009511f9..f224bbf068 100644 --- a/src/agents/realtime/session.py +++ b/src/agents/realtime/session.py @@ -668,19 +668,25 @@ async def _maybe_request_tool_approval( agent, tool_lookup_key=tool_lookup_key, ) - - needs_approval = await self._function_needs_approval(function_tool, tool_call) - if self._closing or self._closed: - return None - if not needs_approval: - return True - approval_status = self._context_wrapper.get_approval_status( function_tool.name, tool_call.call_id, existing_pending=approval_item, tool_lookup_key=tool_lookup_key, ) + if approval_status is None: + needs_approval = await self._function_needs_approval(function_tool, tool_call) + if self._closing or self._closed: + return None + approval_status = self._context_wrapper.get_approval_status( + function_tool.name, + tool_call.call_id, + existing_pending=approval_item, + tool_lookup_key=tool_lookup_key, + ) + if approval_status is None and not needs_approval: + return True + if approval_status is True: return True if approval_status is False: @@ -694,6 +700,16 @@ async def _maybe_request_tool_approval( ) if self._closing or self._closed: return None + approval_status = self._context_wrapper.get_approval_status( + function_tool.name, + tool_call.call_id, + existing_pending=approval_item, + tool_lookup_key=tool_lookup_key, + ) + if approval_status is True: + return True + if approval_status is False: + return False if rejected_message is not None: return self._build_realtime_tool_output( tool=function_tool, diff --git a/src/agents/run_internal/tool_actions.py b/src/agents/run_internal/tool_actions.py index 0421c15c43..f2872770d8 100644 --- a/src/agents/run_internal/tool_actions.py +++ b/src/agents/run_internal/tool_actions.py @@ -454,11 +454,23 @@ async def _run_call(span: Any | None) -> RunItem: dataclasses.asdict(shell_call.action) ) - needs_approval_result = await evaluate_needs_approval_setting( - shell_tool.needs_approval, context_wrapper, shell_call.action, shell_call.call_id + approval_status = context_wrapper.get_approval_status( + shell_tool.name, shell_call.call_id ) + if approval_status is None: + needs_approval_result = await evaluate_needs_approval_setting( + shell_tool.needs_approval, + context_wrapper, + shell_call.action, + shell_call.call_id, + ) + approval_status = context_wrapper.get_approval_status( + shell_tool.name, shell_call.call_id + ) + else: + needs_approval_result = False - if needs_approval_result: + if approval_status is None and needs_approval_result: approval_status, approval_item = await resolve_approval_status( tool_name=shell_tool.name, call_id=shell_call.call_id, @@ -468,24 +480,24 @@ async def _run_call(span: Any | None) -> RunItem: on_approval=shell_tool.on_approval, ) - if approval_status is False: - rejection_message = await resolve_approval_rejection_message( - context_wrapper=context_wrapper, - run_config=config, - tool_type="shell", - tool_name=shell_tool.name, - call_id=shell_call.call_id, - ) - return shell_rejection_item( - agent, - shell_call.call_id, - tool_call=call.tool_call, - rejection_message=rejection_message, - ) - - if approval_status is not True: + if approval_status is None: return approval_item + if approval_status is False: + rejection_message = await resolve_approval_rejection_message( + context_wrapper=context_wrapper, + run_config=config, + tool_type="shell", + tool_name=shell_tool.name, + call_id=shell_call.call_id, + ) + return shell_rejection_item( + agent, + shell_call.call_id, + tool_call=call.tool_call, + rejection_message=rejection_message, + ) + await asyncio.gather( hooks.on_tool_start(context_wrapper, agent, shell_tool), ( @@ -649,11 +661,16 @@ async def _run_call(span: Any | None) -> RunItem: if span and config.trace_include_sensitive_data: span.span_data.input = tool_input - needs_approval_result = await evaluate_needs_approval_setting( - custom_tool.runtime_needs_approval(), context_wrapper, tool_input, call_id - ) + approval_status = context_wrapper.get_approval_status(custom_tool.name, call_id) + if approval_status is None: + needs_approval_result = await evaluate_needs_approval_setting( + custom_tool.runtime_needs_approval(), context_wrapper, tool_input, call_id + ) + approval_status = context_wrapper.get_approval_status(custom_tool.name, call_id) + else: + needs_approval_result = False - if needs_approval_result: + if approval_status is None and needs_approval_result: approval_status, approval_item = await resolve_approval_status( tool_name=custom_tool.name, call_id=call_id, @@ -663,27 +680,27 @@ async def _run_call(span: Any | None) -> RunItem: on_approval=custom_tool.runtime_on_approval(), ) - if approval_status is False: - rejection_message = await resolve_approval_rejection_message( - context_wrapper=context_wrapper, - run_config=config, - tool_type="custom", - tool_name=custom_tool.name, - call_id=call_id, - ) - return cls._tool_output_item( - agent, + if approval_status is None: + return approval_item + + if approval_status is False: + rejection_message = await resolve_approval_rejection_message( + context_wrapper=context_wrapper, + run_config=config, + tool_type="custom", + tool_name=custom_tool.name, + call_id=call_id, + ) + return cls._tool_output_item( + agent, + call_id, + rejection_message, + raw_item=cls._raw_tool_output_item( call_id, rejection_message, - raw_item=cls._raw_tool_output_item( - call_id, - rejection_message, - tool_call=call.tool_call, - ), - ) - - if approval_status is not True: - return approval_item + tool_call=call.tool_call, + ), + ) await asyncio.gather( hooks.on_tool_start(tool_context, agent, custom_tool), @@ -830,15 +847,20 @@ async def _run_call(span: Any | None) -> RunItem: ] ) + approval_status = context_wrapper.get_approval_status(apply_patch_tool.name, call_id) needs_approval_result = False - for operation in operations: - if await evaluate_needs_approval_setting( - apply_patch_tool.needs_approval, context_wrapper, operation, call_id - ): - needs_approval_result = True - break - - if needs_approval_result: + if approval_status is None: + for operation in operations: + needs_approval_result = await evaluate_needs_approval_setting( + apply_patch_tool.needs_approval, context_wrapper, operation, call_id + ) + approval_status = context_wrapper.get_approval_status( + apply_patch_tool.name, call_id + ) + if approval_status is not None or needs_approval_result: + break + + if approval_status is None and needs_approval_result: approval_status, approval_item = await resolve_approval_status( tool_name=apply_patch_tool.name, call_id=call_id, @@ -848,25 +870,25 @@ async def _run_call(span: Any | None) -> RunItem: on_approval=apply_patch_tool.on_approval, ) - if approval_status is False: - rejection_message = await resolve_approval_rejection_message( - context_wrapper=context_wrapper, - run_config=config, - tool_type="apply_patch", - tool_name=apply_patch_tool.name, - call_id=call_id, - ) - return apply_patch_rejection_item( - agent, - call_id, - tool_call=call.tool_call, - output_type="apply_patch_call_output", - rejection_message=rejection_message, - ) - - if approval_status is not True: + if approval_status is None: return approval_item + if approval_status is False: + rejection_message = await resolve_approval_rejection_message( + context_wrapper=context_wrapper, + run_config=config, + tool_type="apply_patch", + tool_name=apply_patch_tool.name, + call_id=call_id, + ) + return apply_patch_rejection_item( + agent, + call_id, + tool_call=call.tool_call, + output_type="apply_patch_call_output", + rejection_message=rejection_message, + ) + await asyncio.gather( hooks.on_tool_start(context_wrapper, agent, apply_patch_tool), ( diff --git a/src/agents/run_internal/tool_execution.py b/src/agents/run_internal/tool_execution.py index 4663ca5897..07aa611c68 100644 --- a/src/agents/run_internal/tool_execution.py +++ b/src/agents/run_internal/tool_execution.py @@ -1724,14 +1724,6 @@ async def _maybe_execute_tool_approval( raw_tool_call: ResponseFunctionToolCall, span_fn: Span[Any], ) -> Any | None: - needs_approval_result = await function_needs_approval( - func_tool, - self.context_wrapper, - tool_call, - ) - if not needs_approval_result: - return None - tool_namespace = get_tool_call_namespace(raw_tool_call) if tool_namespace is None and is_deferred_top_level_function_tool(func_tool): tool_namespace = func_tool.name @@ -1744,6 +1736,21 @@ async def _maybe_execute_tool_approval( tool_namespace=tool_namespace, tool_lookup_key=tool_lookup_key, ) + if approval_status is None: + needs_approval_result = await function_needs_approval( + func_tool, + self.context_wrapper, + tool_call, + ) + approval_status = self.context_wrapper.get_approval_status( + func_tool.name, + tool_call.call_id, + tool_namespace=tool_namespace, + tool_lookup_key=tool_lookup_key, + ) + if approval_status is None and not needs_approval_result: + return None + if approval_status is None: if self._should_run_pre_approval_tool_input_guardrails(): tool_context_namespace = get_tool_call_namespace(raw_tool_call) @@ -1763,7 +1770,13 @@ async def _maybe_execute_tool_approval( agent=self.public_agent, tool_input_guardrail_results=self.tool_input_guardrail_results, ) - if rejected_message is not None: + approval_status = self.context_wrapper.get_approval_status( + func_tool.name, + tool_call.call_id, + tool_namespace=tool_namespace, + tool_lookup_key=tool_lookup_key, + ) + if approval_status is None and rejected_message is not None: return FunctionToolResult( tool=func_tool, output=rejected_message, @@ -1776,6 +1789,8 @@ async def _maybe_execute_tool_approval( tool_origin=get_function_tool_origin(func_tool), ), ) + + if approval_status is None: approval_item = ToolApprovalItem( agent=self.public_agent, raw_item=raw_tool_call, diff --git a/src/agents/run_internal/tool_planning.py b/src/agents/run_internal/tool_planning.py index e960b1edf0..84cd323fbf 100644 --- a/src/agents/run_internal/tool_planning.py +++ b/src/agents/run_internal/tool_planning.py @@ -404,6 +404,20 @@ async def _collect_runs_by_approval( if output_exists_checker and output_exists_checker(call_id): continue + needs_approval = True + if approval_status is None and needs_approval_checker: + try: + needs_approval = await needs_approval_checker(run) + except UserError: + raise + except Exception: + needs_approval = True + approval_status = context_wrapper.get_approval_status( + tool_name, + call_id, + existing_pending=existing_pending, + ) + if approval_status is False: rejection = rejection_builder(run, call_id) if inspect.isawaitable(rejection): @@ -417,15 +431,6 @@ async def _collect_runs_by_approval( approved_runs.append(run) continue - needs_approval = True - if needs_approval_checker: - try: - needs_approval = await needs_approval_checker(run) - except UserError: - raise - except Exception: - needs_approval = True - if not needs_approval: approved_runs.append(run) continue @@ -517,6 +522,16 @@ async def _select_function_tool_runs_for_resume( existing_pending=approval_items_by_call_id.get(call_id), ) + requires_approval = True + if approval_status is None: + requires_approval = await needs_approval_checker(run) + approval_status = context_wrapper.get_approval_status( + run.function_tool.name, + call_id, + tool_namespace=get_tool_call_namespace(run.tool_call), + existing_pending=approval_items_by_call_id.get(call_id), + ) + if approval_status is False: await record_rejection(call_id, run.tool_call, run.function_tool) continue @@ -525,12 +540,6 @@ async def _select_function_tool_runs_for_resume( selected.append(run) continue - # Only invoke needs_approval_checker when the approval state is unresolved; - # for explicit approve/reject decisions the checker's result is unused, and - # invoking it eagerly risks user-side effects (or exceptions that swallow - # rejections) on calls whose outcome is already determined. - requires_approval = await needs_approval_checker(run) - if not requires_approval: selected.append(run) continue diff --git a/src/agents/run_internal/turn_resolution.py b/src/agents/run_internal/turn_resolution.py index a103513eed..49dd5c439a 100644 --- a/src/agents/run_internal/turn_resolution.py +++ b/src/agents/run_internal/turn_resolution.py @@ -1220,10 +1220,16 @@ async def _apply_patch_needs_approval(run: ToolRunApplyPatchCall) -> bool: ) call_id = extract_apply_patch_call_id(run.tool_call) for operation in operations: - if await evaluate_needs_approval_setting( + needs_approval = await evaluate_needs_approval_setting( run.apply_patch_tool.needs_approval, context_wrapper, operation, call_id - ): - return True + ) + approval_status = context_wrapper.get_approval_status( + run.apply_patch_tool.name, + call_id, + existing_pending=approval_items_by_call_id.get(call_id), + ) + if approval_status is not None or needs_approval: + return needs_approval return False async def _custom_tool_needs_approval(run: ToolRunCustom) -> bool: @@ -1841,11 +1847,18 @@ def _rebind_function_run( ) rejected_function_call_ids.add(call_id) + collector_owned_call_ids = { + *(_shell_call_id_from_run(run) for run in processed_response.shell_calls), + *(_apply_patch_call_id_from_run(run) for run in processed_response.apply_patch_calls), + *(_custom_call_id_from_run(run) for run in processed_response.custom_tool_calls), + } for original_approval in pending_approval_items: approval_snapshot = validated_function_approval_items.get(original_approval) if approval_snapshot is None: approval = original_approval approval_call_id = extract_tool_call_id(approval.raw_item) + if approval_call_id in collector_owned_call_ids: + continue if ( approval_call_id is None or context_wrapper.get_approval_status( @@ -2004,8 +2017,8 @@ def _rebind_function_run( for interruption in _collect_tool_interruptions( function_results=function_results, custom_tool_results=custom_tool_results, - shell_results=[], - apply_patch_results=[], + shell_results=shell_results, + apply_patch_results=apply_patch_results, ): _add_pending_interruption(interruption) diff --git a/src/agents/sandbox/capabilities/tools/apply_patch_tool.py b/src/agents/sandbox/capabilities/tools/apply_patch_tool.py index 20ffb10b3b..5aa653a0ef 100644 --- a/src/agents/sandbox/capabilities/tools/apply_patch_tool.py +++ b/src/agents/sandbox/capabilities/tools/apply_patch_tool.py @@ -202,13 +202,15 @@ async def _needs_custom_approval( return False for operation in operations: - if await evaluate_needs_approval_setting( + needs_approval = await evaluate_needs_approval_setting( self.needs_approval, ctx_wrapper, operation, call_id, - ): - return True + ) + approval_status = ctx_wrapper.get_approval_status(self.name, call_id) + if approval_status is not None or needs_approval: + return needs_approval return False async def _on_invoke_tool(self, ctx: ToolContext[Any], raw_input: str) -> str: diff --git a/tests/mcp/test_mcp_approval.py b/tests/mcp/test_mcp_approval.py index 791fa71c24..873746f0fd 100644 --- a/tests/mcp/test_mcp_approval.py +++ b/tests/mcp/test_mcp_approval.py @@ -191,7 +191,7 @@ def require_approval( assert not second.interruptions, "safe should bypass approval via callable policy" assert second.final_output == "safe done" - assert seen == ["guarded", "guarded", "safe"] + assert seen == ["guarded", "safe"] @pytest.mark.asyncio @@ -236,7 +236,6 @@ async def require_approval( assert second.final_output == "no approval path" assert seen_contexts == [ - {"needs_approval": True}, {"needs_approval": True}, {"needs_approval": False}, ] diff --git a/tests/realtime/test_session.py b/tests/realtime/test_session.py index a549c13fe9..9a6309fcc5 100644 --- a/tests/realtime/test_session.py +++ b/tests/realtime/test_session.py @@ -3343,6 +3343,186 @@ async def invoke_tool(_ctx: ToolContext[Any], _arguments: str) -> str: ] assert tool_calls == [] + @pytest.mark.asyncio + async def test_sticky_rejection_skips_dynamic_approval_checker(self, mock_model): + checker_calls: list[str] = [] + tool_calls: list[str] = [] + + async def needs_approval(_ctx: Any, _params: dict[str, Any], call_id: str) -> bool: + checker_calls.append(call_id) + if call_id != "call-reject-first": + raise AssertionError("sticky rejection must bypass needs_approval") + return True + + async def invoke_tool(_ctx: ToolContext[Any], _arguments: str) -> str: + tool_calls.append("called") + return "should-not-run" + + tool = FunctionTool( + name="send_email", + description="Send an email.", + params_json_schema={"type": "object", "properties": {}}, + on_invoke_tool=invoke_tool, + needs_approval=needs_approval, + ) + agent = RealtimeAgent(name="agent", tools=[tool]) + session = RealtimeSession(mock_model, agent, None, run_config={"async_tool_calls": False}) + first_call = RealtimeModelToolCallEvent( + name=tool.name, call_id="call-reject-first", arguments="{}" + ) + second_call = RealtimeModelToolCallEvent( + name=tool.name, call_id="call-reject-second", arguments="{}" + ) + + await session._handle_tool_call(first_call) + await session.reject_tool_call(first_call.call_id, always=True) + await session._handle_tool_call(second_call) + + assert checker_calls == ["call-reject-first"] + assert tool_calls == [] + assert session._pending_tool_calls == {} + assert len(mock_model.sent_tool_outputs) == 2 + + @pytest.mark.asyncio + async def test_sticky_rejection_wins_while_dynamic_approval_checker_is_pending( + self, mock_model + ): + checker_started = asyncio.Event() + checker_release = asyncio.Event() + checker_calls: list[str] = [] + tool_calls: list[str] = [] + + async def needs_approval(_ctx: Any, _params: dict[str, Any], call_id: str) -> bool: + checker_calls.append(call_id) + if call_id == "call-pending-checker": + checker_started.set() + await checker_release.wait() + return False + return True + + async def invoke_tool(_ctx: ToolContext[Any], _arguments: str) -> str: + tool_calls.append("called") + return "should-not-run" + + tool = FunctionTool( + name="send_email", + description="Send an email.", + params_json_schema={"type": "object", "properties": {}}, + on_invoke_tool=invoke_tool, + needs_approval=needs_approval, + ) + agent = RealtimeAgent(name="agent", tools=[tool]) + session = RealtimeSession(mock_model, agent, None) + first_call = RealtimeModelToolCallEvent( + name=tool.name, call_id="call-reject-first", arguments="{}" + ) + pending_checker_call = RealtimeModelToolCallEvent( + name=tool.name, call_id="call-pending-checker", arguments="{}" + ) + + await session._handle_tool_call(first_call) + pending_checker_task = asyncio.create_task(session._handle_tool_call(pending_checker_call)) + try: + await asyncio.wait_for(checker_started.wait(), timeout=1) + await session.reject_tool_call( + first_call.call_id, + always=True, + rejection_message="sticky rejection", + ) + finally: + checker_release.set() + await pending_checker_task + + assert checker_calls == ["call-reject-first", "call-pending-checker"] + assert tool_calls == [] + assert session._pending_tool_calls == {} + assert [output for _call, output, _start in mock_model.sent_tool_outputs] == [ + "sticky rejection", + "sticky rejection", + ] + + @pytest.mark.parametrize("approved", [True, False], ids=["approved", "rejected"]) + @pytest.mark.asyncio + async def test_sticky_decision_wins_while_rejecting_pre_approval_guardrail_is_pending( + self, mock_model, approved: bool + ): + guardrail_started = asyncio.Event() + guardrail_release = asyncio.Event() + guardrail_calls: list[str | None] = [] + tool_calls: list[str] = [] + + @tool_input_guardrail + async def blocking_guardrail( + data: ToolInputGuardrailData, + ) -> ToolGuardrailFunctionOutput: + call_id = data.context.tool_call_id + guardrail_calls.append(call_id) + if call_id == "call-pending-guardrail": + guardrail_started.set() + await guardrail_release.wait() + return ToolGuardrailFunctionOutput.reject_content("guardrail rejection") + return ToolGuardrailFunctionOutput.allow() + + async def invoke_tool(_ctx: ToolContext[Any], _arguments: str) -> str: + tool_calls.append("called") + return "tool output" + + tool = FunctionTool( + name="send_email", + description="Send an email.", + params_json_schema={"type": "object", "properties": {}}, + on_invoke_tool=invoke_tool, + needs_approval=True, + tool_input_guardrails=[blocking_guardrail], + ) + agent = RealtimeAgent(name="agent", tools=[tool]) + session = RealtimeSession( + mock_model, + agent, + None, + run_config={"tool_execution": {"pre_approval_tool_input_guardrails": True}}, + ) + first_call = RealtimeModelToolCallEvent( + name=tool.name, call_id="call-reject-first", arguments="{}" + ) + pending_guardrail_call = RealtimeModelToolCallEvent( + name=tool.name, call_id="call-pending-guardrail", arguments="{}" + ) + + await session._handle_tool_call(first_call) + pending_guardrail_task = asyncio.create_task( + session._handle_tool_call(pending_guardrail_call) + ) + try: + await asyncio.wait_for(guardrail_started.wait(), timeout=1) + approval_item = session._pending_tool_calls[first_call.call_id].approval_item + if approved: + session._context_wrapper.approve_tool(approval_item, always_approve=True) + else: + session._context_wrapper.reject_tool( + approval_item, + always_reject=True, + rejection_message="sticky rejection", + ) + finally: + guardrail_release.set() + await pending_guardrail_task + + assert pending_guardrail_call.call_id not in session._pending_tool_calls + outputs = [output for _call, output, _start in mock_model.sent_tool_outputs] + if approved: + assert guardrail_calls == [ + "call-reject-first", + "call-pending-guardrail", + "call-pending-guardrail", + ] + assert tool_calls == [] + assert outputs == ["guardrail rejection"] + else: + assert guardrail_calls == ["call-reject-first", "call-pending-guardrail"] + assert tool_calls == [] + assert outputs == ["sticky rejection"] + @pytest.mark.asyncio async def test_function_tool_exception_handling( self, mock_model, mock_agent, mock_function_tool diff --git a/tests/sandbox/capabilities/test_apply_patch_tool.py b/tests/sandbox/capabilities/test_apply_patch_tool.py index 450d2f8763..a69d34ddf3 100644 --- a/tests/sandbox/capabilities/test_apply_patch_tool.py +++ b/tests/sandbox/capabilities/test_apply_patch_tool.py @@ -1,5 +1,6 @@ from __future__ import annotations +import asyncio from collections.abc import Awaitable from pathlib import Path from typing import Any, cast @@ -77,6 +78,73 @@ async def needs_approval( assert isinstance(result, ToolApprovalItem) + @pytest.mark.parametrize("approved", [True, False], ids=["approved", "rejected"]) + @pytest.mark.asyncio + async def test_multi_operation_checker_stops_when_approval_resolves( + self, + approved: bool, + ) -> None: + checker_started = asyncio.Event() + release_checker = asyncio.Event() + checked_paths: list[str] = [] + + async def needs_approval( + _ctx: RunContextWrapper[Any], operation: ApplyPatchOperation, _call_id: str + ) -> bool: + checked_paths.append(operation.path) + if len(checked_paths) > 1: + raise AssertionError("resolved approval must stop later callbacks") + checker_started.set() + await release_checker.wait() + return False + + session = ApplyPatchSession() + tool = SandboxApplyPatchTool(session=session, needs_approval=needs_approval) + context_wrapper = make_context_wrapper() + raw_input = ( + "*** Begin Patch\n" + "*** Add File: first.txt\n" + "+first\n" + "*** Add File: second.txt\n" + "+second\n" + "*** End Patch\n" + ) + approval_item = ToolApprovalItem( + agent=Agent(name="patcher"), + raw_item={ + "type": "custom_tool_call", + "name": tool.name, + "call_id": "call_apply", + "input": raw_input, + }, + tool_name=tool.name, + ) + execution_task = asyncio.create_task( + _execute_custom_tool_call( + tool, + context_wrapper=context_wrapper, + raw_input=raw_input, + ) + ) + try: + await asyncio.wait_for(checker_started.wait(), timeout=1) + if approved: + context_wrapper.approve_tool(approval_item) + else: + context_wrapper.reject_tool(approval_item) + release_checker.set() + result = await execution_task + finally: + release_checker.set() + + assert checked_paths == ["first.txt"] + assert isinstance(result, ToolCallOutputItem) + if approved: + assert session.files[Path("/workspace/first.txt")] == b"first" + assert session.files[Path("/workspace/second.txt")] == b"second" + else: + assert session.files == {} + @pytest.mark.asyncio async def test_invalid_patch_input_surfaces_tool_error_after_approval_precheck(self) -> None: tool = SandboxApplyPatchTool(session=ApplyPatchSession(), needs_approval=True) diff --git a/tests/test_hitl_error_scenarios.py b/tests/test_hitl_error_scenarios.py index 23d4002c1d..7f936d4ca0 100644 --- a/tests/test_hitl_error_scenarios.py +++ b/tests/test_hitl_error_scenarios.py @@ -2,11 +2,16 @@ from __future__ import annotations +import asyncio from collections.abc import Callable from typing import Any, Optional, cast import pytest -from openai.types.responses import ResponseComputerToolCall, ResponseFunctionToolCall +from openai.types.responses import ( + ResponseComputerToolCall, + ResponseCustomToolCall, + ResponseFunctionToolCall, +) from openai.types.responses.response_computer_tool_call import ActionScreenshot from openai.types.responses.response_input_param import ( ComputerCallOutput, @@ -18,12 +23,14 @@ Agent, ApplyPatchTool, ComputerTool, + CustomTool, LocalShellTool, Runner, RunResult, RunState, ShellTool, ToolApprovalItem, + ToolExecutionConfig, function_tool, tool_namespace, ) @@ -53,12 +60,20 @@ ToolRunShellCall, extract_tool_call_id, ) +from agents.run_internal.run_steps import ToolRunCustom +from agents.run_internal.tool_actions import ApplyPatchAction, CustomToolAction, ShellAction +from agents.run_internal.tool_execution import execute_function_tool_calls from agents.run_internal.tool_planning import ( _collect_runs_by_approval, _select_function_tool_runs_for_resume, ) from agents.run_state import RunState as RunStateClass from agents.tool import FunctionTool, HostedMCPTool +from agents.tool_guardrails import ( + ToolGuardrailFunctionOutput, + ToolInputGuardrailData, + tool_input_guardrail, +) from agents.usage import Usage from .fake_model import FakeModel @@ -1393,6 +1408,309 @@ async def _record_rejection( assert rejections == ["rejected-call"] +@pytest.mark.asyncio +async def test_resume_rechecks_rejection_after_function_approval_checker() -> None: + """A rejection recorded while the checker waits must prevent another interruption.""" + + @function_tool(needs_approval=True) + async def sensitive() -> str: + return "should-not-run" + + tool_call = make_function_tool_call(sensitive.name, call_id="call-concurrent-function") + run = ToolRunFunction(tool_call=tool_call, function_tool=sensitive) + agent = Agent(name="agent", tools=[sensitive]) + approval_item = ToolApprovalItem(agent=agent, raw_item=tool_call) + context_wrapper = make_context_wrapper() + checker_started = asyncio.Event() + release_checker = asyncio.Event() + + async def _needs_approval_checker(_run: ToolRunFunction) -> bool: + checker_started.set() + await release_checker.wait() + return True + + pending: list[ToolApprovalItem] = [] + rejections: list[str | None] = [] + + async def _record_rejection( + call_id: str | None, + _tool_call: ResponseFunctionToolCall, + _tool: FunctionTool, + ) -> None: + rejections.append(call_id) + + selection_task = asyncio.create_task( + _select_function_tool_runs_for_resume( + [run], + approval_items_by_call_id={tool_call.call_id: approval_item}, + context_wrapper=context_wrapper, + needs_approval_checker=_needs_approval_checker, + output_exists_checker=lambda _run: False, + record_rejection=_record_rejection, + pending_interruption_adder=pending.append, + pending_item_builder=lambda _run: approval_item, + ) + ) + try: + await asyncio.wait_for(checker_started.wait(), timeout=1) + context_wrapper.reject_tool(approval_item) + release_checker.set() + selected = await selection_task + finally: + release_checker.set() + + assert selected == [] + assert pending == [] + assert rejections == [tool_call.call_id] + + +@pytest.mark.parametrize("approved", [True, False], ids=["approved", "rejected"]) +@pytest.mark.asyncio +async def test_execute_path_prefers_decision_resolved_during_rejecting_guardrail( + approved: bool, +) -> None: + """Stored approval status must win when a rejecting guardrail was already waiting.""" + guardrail_started = asyncio.Event() + release_guardrail = asyncio.Event() + guardrail_calls = 0 + executed: list[str] = [] + + @tool_input_guardrail + async def rejecting_guardrail( + _data: ToolInputGuardrailData, + ) -> ToolGuardrailFunctionOutput: + nonlocal guardrail_calls + guardrail_calls += 1 + guardrail_started.set() + await release_guardrail.wait() + return ToolGuardrailFunctionOutput.reject_content("guardrail rejection") + + @function_tool(needs_approval=True, tool_input_guardrails=[rejecting_guardrail]) + async def sensitive() -> str: + executed.append("ran") + return "tool output" + + tool_call = make_function_tool_call(sensitive.name, call_id="call-pending-guardrail") + tool_run = ToolRunFunction(tool_call=tool_call, function_tool=sensitive) + agent = Agent(name="agent", tools=[sensitive]) + approval_item = ToolApprovalItem(agent=agent, raw_item=tool_call) + context_wrapper = make_context_wrapper() + execution_task = asyncio.create_task( + execute_function_tool_calls( + bindings=bind_public_agent(agent), + tool_runs=[tool_run], + hooks=RunHooks(), + context_wrapper=context_wrapper, + config=RunConfig( + tool_execution=ToolExecutionConfig(pre_approval_tool_input_guardrails=True) + ), + ) + ) + try: + await asyncio.wait_for(guardrail_started.wait(), timeout=1) + if approved: + context_wrapper.approve_tool(approval_item) + else: + context_wrapper.reject_tool(approval_item, rejection_message="stored rejection") + release_guardrail.set() + results, _, _ = await execution_task + finally: + release_guardrail.set() + + assert len(results) == 1 + if approved: + assert results[0].output == "guardrail rejection" + assert guardrail_calls == 2 + assert executed == [] + else: + assert results[0].output == "stored rejection" + assert guardrail_calls == 1 + assert executed == [] + + +@pytest.mark.asyncio +async def test_execute_path_skips_needs_approval_checker_when_status_resolved() -> None: + """Resuming an approved call must not re-evaluate its dynamic approval policy.""" + checker_calls: list[str] = [] + + async def needs_approval(_ctx: Any, _args: dict[str, Any], call_id: str) -> bool: + checker_calls.append(call_id) + if len(checker_calls) > 1: + raise AssertionError("resolved approval must bypass needs_approval") + return True + + @function_tool(needs_approval=needs_approval) + async def sensitive(value: str) -> str: + return f"ran:{value}" + + model = FakeModel() + agent = Agent(name="agent", model=model, tools=[sensitive]) + model.add_multiple_turn_outputs( + [ + [make_function_tool_call(sensitive.name, call_id="call-1", arguments='{"value":"x"}')], + [get_text_message("done")], + ] + ) + + first = await Runner.run(agent, "hello") + assert len(first.interruptions) == 1 + assert checker_calls == ["call-1"] + + state = first.to_state() + state.approve(first.interruptions[0]) + resumed = await Runner.run(agent, state) + + assert resumed.final_output == "done" + assert checker_calls == ["call-1"] + assert any( + isinstance(item, ToolCallOutputItem) and item.output == "ran:x" + for item in resumed.new_items + ) + + +@pytest.mark.parametrize("tool_kind", ["function", "shell", "custom", "apply_patch"]) +@pytest.mark.asyncio +async def test_execute_path_honors_sticky_rejection_before_checker(tool_kind: str) -> None: + """A sticky rejection must bypass dynamic policies and prevent side effects.""" + executed: list[str] = [] + context_wrapper = make_context_wrapper() + + async def unexpected_checker(_ctx: Any, _payload: Any, _call_id: str) -> bool: + raise AssertionError("sticky rejection must bypass needs_approval") + + if tool_kind == "function": + + @function_tool(needs_approval=unexpected_checker) + async def sensitive() -> str: + executed.append("function") + return "should-not-run" + + agent = Agent(name="agent", tools=[sensitive]) + context_wrapper.reject_tool( + ToolApprovalItem( + agent=agent, + raw_item=make_function_tool_call(sensitive.name, call_id="call-prior"), + ), + always_reject=True, + ) + function_results, _, _ = await execute_function_tool_calls( + bindings=bind_public_agent(agent), + tool_runs=[ + ToolRunFunction( + tool_call=make_function_tool_call(sensitive.name, call_id="call-next"), + function_tool=sensitive, + ) + ], + hooks=RunHooks(), + context_wrapper=context_wrapper, + config=RunConfig(), + ) + assert [result.output for result in function_results] == [HITL_REJECTION_MSG] + elif tool_kind == "shell": + + def shell_executor(_req: Any) -> str: + executed.append("shell") + return "should-not-run" + + shell_tool = ShellTool(executor=shell_executor, needs_approval=unexpected_checker) + agent = Agent(name="agent", tools=[shell_tool]) + context_wrapper.reject_tool( + ToolApprovalItem( + agent=agent, + raw_item=cast(dict[str, Any], make_shell_call("call-prior")), + tool_name=shell_tool.name, + ), + always_reject=True, + ) + result = await ShellAction.execute( + agent=agent, + call=ToolRunShellCall( + tool_call=cast(dict[str, Any], make_shell_call("call-next")), + shell_tool=shell_tool, + ), + hooks=RunHooks(), + context_wrapper=context_wrapper, + config=RunConfig(), + ) + assert isinstance(result, ToolCallOutputItem) + assert HITL_REJECTION_MSG in str(result.output) + elif tool_kind == "custom": + + async def invoke_custom(_ctx: Any, _raw: str) -> str: + executed.append("custom") + return "should-not-run" + + custom_tool = CustomTool( + name="raw_editor", + description="Edit raw text.", + on_invoke_tool=invoke_custom, + format={"type": "text"}, + needs_approval=unexpected_checker, + ) + agent = Agent(name="agent", tools=[custom_tool]) + context_wrapper.reject_tool( + ToolApprovalItem( + agent=agent, + raw_item=cast( + Any, + ResponseCustomToolCall( + type="custom_tool_call", + name=custom_tool.name, + call_id="call-prior", + input="prior", + ), + ), + tool_name=custom_tool.name, + ), + always_reject=True, + ) + next_call = ResponseCustomToolCall( + type="custom_tool_call", + name=custom_tool.name, + call_id="call-next", + input="next", + ) + result = await CustomToolAction.execute( + agent=agent, + call=ToolRunCustom(tool_call=next_call, custom_tool=custom_tool), + hooks=RunHooks(), + context_wrapper=context_wrapper, + config=RunConfig(), + ) + assert isinstance(result, ToolCallOutputItem) + assert result.output == HITL_REJECTION_MSG + else: + editor = RecordingEditor() + apply_patch_tool = ApplyPatchTool( + editor=editor, + needs_approval=unexpected_checker, + ) + agent = Agent(name="agent", tools=[apply_patch_tool]) + context_wrapper.reject_tool( + ToolApprovalItem( + agent=agent, + raw_item=cast(dict[str, Any], make_apply_patch_dict("call-prior")), + tool_name=apply_patch_tool.name, + ), + always_reject=True, + ) + result = await ApplyPatchAction.execute( + agent=agent, + call=ToolRunApplyPatchCall( + tool_call=cast(dict[str, Any], make_apply_patch_dict("call-next")), + apply_patch_tool=apply_patch_tool, + ), + hooks=RunHooks(), + context_wrapper=context_wrapper, + config=RunConfig(), + ) + assert isinstance(result, ToolCallOutputItem) + assert HITL_REJECTION_MSG in str(result.output) + assert editor.operations == [] + + assert executed == [] + + @pytest.mark.asyncio async def test_collect_runs_by_approval_skips_checker_when_status_resolved() -> None: """Approved/rejected shell calls must not invoke needs_approval_checker. @@ -1456,6 +1774,174 @@ async def _build_rejection(run: ToolRunShellCall, call_id: str) -> RunItem: assert len(rejections) == 1 +@pytest.mark.parametrize("approved", [True, False], ids=["approved", "rejected"]) +@pytest.mark.asyncio +async def test_resume_apply_patch_uses_concurrent_decision_without_reinterrupting( + approved: bool, +) -> None: + """A resolved apply-patch decision must stop callbacks and avoid stale interruptions.""" + checker_started = asyncio.Event() + release_checker = asyncio.Event() + checked_paths: list[str] = [] + + async def _needs_approval(_ctx: Any, operation: Any, _call_id: str) -> bool: + checked_paths.append(operation.path) + if len(checked_paths) > 1: + raise AssertionError("resolved rejection must stop later approval callbacks") + checker_started.set() + await release_checker.wait() + return False + + editor = RecordingEditor() + apply_patch_tool = ApplyPatchTool(editor=editor, needs_approval=_needs_approval) + _model, public_agent = make_model_and_agent(tools=[apply_patch_tool]) + execution_agent = public_agent.clone() + set_public_agent(execution_agent, public_agent) + raw_item = cast( + Any, + { + "type": "apply_patch_call", + "call_id": "call-concurrent-apply-patch", + "operations": [ + {"type": "update_file", "path": "first.txt", "diff": "-old\n+new\n"}, + {"type": "delete_file", "path": "second.txt"}, + ], + }, + ) + approval_item = ToolApprovalItem( + agent=public_agent, + raw_item=raw_item, + tool_name=apply_patch_tool.name, + ) + context_wrapper = make_context_wrapper() + processed_response = ProcessedResponse( + new_items=[], + handoffs=[], + functions=[], + computer_actions=[], + local_shell_calls=[], + shell_calls=[], + apply_patch_calls=[ + ToolRunApplyPatchCall(tool_call=raw_item, apply_patch_tool=apply_patch_tool) + ], + tools_used=[], + mcp_approval_requests=[], + interruptions=[], + ) + + resolution_task = asyncio.create_task( + _resolve_interrupted_turn( + agent=execution_agent, + original_input="resume apply patch", + original_pre_step_items=[], + new_response=ModelResponse(output=[], usage=Usage(), response_id="resp"), + processed_response=processed_response, + hooks=RunHooks(), + context_wrapper=context_wrapper, + run_config=RunConfig(), + run_state=make_state_with_interruptions(public_agent, [approval_item]), + ) + ) + try: + await asyncio.wait_for(checker_started.wait(), timeout=1) + if approved: + context_wrapper.approve_tool(approval_item) + else: + context_wrapper.reject_tool(approval_item) + release_checker.set() + result = await resolution_task + finally: + release_checker.set() + + assert checked_paths == ["first.txt"] + assert not isinstance(result.next_step, NextStepInterruption) + rejection_outputs = [ + item + for item in result.new_step_items + if isinstance(item, ToolCallOutputItem) and item.output == HITL_REJECTION_MSG + ] + if approved: + assert rejection_outputs == [] + assert len(editor.operations) == 2 + else: + assert len(rejection_outputs) == 1 + assert editor.operations == [] + + +@pytest.mark.parametrize("tool_kind", ["shell", "apply_patch"]) +@pytest.mark.asyncio +async def test_resume_preserves_approval_created_during_tool_execution(tool_kind: str) -> None: + """A second policy evaluation may create a new approval interruption during execution.""" + checker_calls = 0 + executed: list[str] = [] + context_wrapper = make_context_wrapper() + processed_response = ProcessedResponse( + new_items=[], + handoffs=[], + functions=[], + computer_actions=[], + local_shell_calls=[], + shell_calls=[], + apply_patch_calls=[], + tools_used=[], + mcp_approval_requests=[], + interruptions=[], + ) + + async def needs_approval(_ctx: Any, _payload: Any, _call_id: str) -> bool: + nonlocal checker_calls + checker_calls += 1 + return checker_calls == 2 + + tool: Any + if tool_kind == "shell": + + def execute_shell(_request: Any) -> str: + executed.append("shell") + return "should-not-run" + + tool = ShellTool(executor=execute_shell, needs_approval=needs_approval) + raw_item = cast(dict[str, Any], make_shell_call("call-execution-approval")) + processed_response.shell_calls = [ToolRunShellCall(tool_call=raw_item, shell_tool=tool)] + else: + editor = RecordingEditor() + tool = ApplyPatchTool(editor=editor, needs_approval=needs_approval) + raw_item = cast(Any, make_apply_patch_dict("call-execution-approval")) + processed_response.apply_patch_calls = [ + ToolRunApplyPatchCall(tool_call=raw_item, apply_patch_tool=tool) + ] + + _model, public_agent = make_model_and_agent(tools=[tool]) + execution_agent = public_agent.clone() + set_public_agent(execution_agent, public_agent) + original_approval = ToolApprovalItem( + agent=public_agent, + raw_item=raw_item, + tool_name=tool.name, + ) + + result = await _resolve_interrupted_turn( + agent=execution_agent, + original_input="resume approval", + original_pre_step_items=[], + new_response=ModelResponse(output=[], usage=Usage(), response_id="resp"), + processed_response=processed_response, + hooks=RunHooks(), + context_wrapper=context_wrapper, + run_config=RunConfig(), + run_state=make_state_with_interruptions(public_agent, [original_approval]), + ) + + assert checker_calls == 2 + assert isinstance(result.next_step, NextStepInterruption) + assert [extract_tool_call_id(item.raw_item) for item in result.next_step.interruptions] == [ + "call-execution-approval" + ] + assert executed == [] + if tool_kind == "apply_patch": + assert editor.operations == [] + + @pytest.mark.asyncio async def test_resume_rebuilds_function_runs_from_object_approvals() -> None: """Rebuild should handle ResponseFunctionToolCall approval items."""