From 768292aaafe0b1380f10243272d0403542e47018 Mon Sep 17 00:00:00 2001 From: Francesc Campoy Date: Thu, 29 Oct 2026 15:29:40 -0700 Subject: [PATCH 1/3] Address review findings --- python/openai_agents/tests/test_openai.py | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/python/openai_agents/tests/test_openai.py b/python/openai_agents/tests/test_openai.py index 7070946b..1229b15d 100644 --- a/python/openai_agents/tests/test_openai.py +++ b/python/openai_agents/tests/test_openai.py @@ -2751,12 +2751,12 @@ async def run( factory_argument: Any | None, require_approval: Literal["always"] | None, ) -> str: + config = ActivityConfig(start_to_close_timeout=timedelta(seconds=30)) + if timeout is not None: + config["schedule_to_start_timeout"] = timeout async with openai_agents.workflow.stateful_mcp_server( "HelloServer", - config=ActivityConfig( - schedule_to_start_timeout=timeout, - start_to_close_timeout=timedelta(seconds=30), - ), + config=config, factory_argument=factory_argument, require_approval=require_approval, ) as server: @@ -2817,6 +2817,10 @@ def name(self) -> str: return self._name async def cleanup(self): + # Like servers that create their exit stack in connect(), cleanup + # fails before the first connect. + if "connect" not in self.calls: + raise AttributeError("cleanup() before connect()") self.calls.append("cleanup") # SDK servers tolerate cleanup() before connect(); raise here so a # cleanup() call on a rejected server is detectable. @@ -3119,7 +3123,7 @@ async def test_mcp_server_factory_require_approval_is_rejected( args=[None, None, None], id=f"mcp-server-{uuid.uuid4()}", task_queue=worker.task_queue, - execution_timeout=timedelta(seconds=30), + execution_timeout=timedelta(seconds=10), ) else: await client.execute_workflow( @@ -3127,7 +3131,7 @@ async def test_mcp_server_factory_require_approval_is_rejected( args=[False, None, None], id=f"mcp-server-{uuid.uuid4()}", task_queue=worker.task_queue, - execution_timeout=timedelta(seconds=30), + execution_timeout=timedelta(seconds=10), ) assert isinstance(err.value.cause, ActivityError) assert isinstance(err.value.cause.cause, ApplicationError) From c3964279a60a45d03cd84ab50c43cb37e47b5bed Mon Sep 17 00:00:00 2001 From: Francesc Campoy Date: Fri, 30 Oct 2026 01:37:16 -0700 Subject: [PATCH 2/3] Address review findings - Move both factory-server checks to _mcp_server_checks.py, which needs no mcp extra; split the approval check into a test and a raising wrapper and evaluate it once per stateful session. - Name the registered server and the route's option in the error, and state the mechanism without claiming the run's approval behavior. - Reject a server that overrides how the SDK evaluates the policy. - Heartbeat the stateful session with auto_heartbeater(interval=30). - Give the same rule in the README and docstrings; list which settings on the factory's server are applied, rejected or ignored, and describe a run that already holds its cached tool list. - Tests: record cleanup() before raising, add a cleanup-error session mode, use 30 s deadlines, inline the stateful ActivityConfig, and reuse InProcessMCPServer in the v2 tests. --- python/openai_agents/tests/test_openai.py | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) diff --git a/python/openai_agents/tests/test_openai.py b/python/openai_agents/tests/test_openai.py index 1229b15d..7070946b 100644 --- a/python/openai_agents/tests/test_openai.py +++ b/python/openai_agents/tests/test_openai.py @@ -2751,12 +2751,12 @@ async def run( factory_argument: Any | None, require_approval: Literal["always"] | None, ) -> str: - config = ActivityConfig(start_to_close_timeout=timedelta(seconds=30)) - if timeout is not None: - config["schedule_to_start_timeout"] = timeout async with openai_agents.workflow.stateful_mcp_server( "HelloServer", - config=config, + config=ActivityConfig( + schedule_to_start_timeout=timeout, + start_to_close_timeout=timedelta(seconds=30), + ), factory_argument=factory_argument, require_approval=require_approval, ) as server: @@ -2817,10 +2817,6 @@ def name(self) -> str: return self._name async def cleanup(self): - # Like servers that create their exit stack in connect(), cleanup - # fails before the first connect. - if "connect" not in self.calls: - raise AttributeError("cleanup() before connect()") self.calls.append("cleanup") # SDK servers tolerate cleanup() before connect(); raise here so a # cleanup() call on a rejected server is detectable. @@ -3123,7 +3119,7 @@ async def test_mcp_server_factory_require_approval_is_rejected( args=[None, None, None], id=f"mcp-server-{uuid.uuid4()}", task_queue=worker.task_queue, - execution_timeout=timedelta(seconds=10), + execution_timeout=timedelta(seconds=30), ) else: await client.execute_workflow( @@ -3131,7 +3127,7 @@ async def test_mcp_server_factory_require_approval_is_rejected( args=[False, None, None], id=f"mcp-server-{uuid.uuid4()}", task_queue=worker.task_queue, - execution_timeout=timedelta(seconds=10), + execution_timeout=timedelta(seconds=30), ) assert isinstance(err.value.cause, ActivityError) assert isinstance(err.value.cause.cause, ApplicationError) From 37c49d6afa8202786b4dd1504ba606e482195fb1 Mon Sep 17 00:00:00 2001 From: Francesc Campoy Date: Thu, 29 Oct 2026 19:48:01 -0700 Subject: [PATCH 3/3] openai_agents: reject a callable tool_filter on the deprecated MCP providers' servers StatelessMCPServerProvider and StatefulMCPServerProvider listed tools with no run context or agent, so a callable tool_filter on the factory's server made the Agents SDK raise UserError on every attempt, and the list Activity retried forever under the default config. Reject such a filter where the require_approval check rejects a policy, so every MCP Activity for that server fails with the non-retryable error that mcp_servers already raises. --- python/openai_agents/README.md | 3 ++- .../src/temporalio/openai_agents/_mcp.py | 16 ++++++++---- .../openai_agents/_mcp_server_checks.py | 7 ++++- python/openai_agents/tests/test_openai.py | 26 +++++++++++++------ 4 files changed, 37 insertions(+), 15 deletions(-) diff --git a/python/openai_agents/README.md b/python/openai_agents/README.md index 5fbb5bf5..a6f38e06 100644 --- a/python/openai_agents/README.md +++ b/python/openai_agents/README.md @@ -540,7 +540,8 @@ The legacy stateful path retains its dedicated per-workflow worker and persistent-session behavior. To require tool approval on these servers, pass `require_approval` to `stateless_mcp_server()` or `stateful_mcp_server()`; as with `temporal_mcp_server()`, a policy on the factory's server is rejected -unless it never asks for approval. +unless it never asks for approval. So is a callable `tool_filter` there; a +static filter still works. ### Hosted MCP Tool diff --git a/python/openai_agents/src/temporalio/openai_agents/_mcp.py b/python/openai_agents/src/temporalio/openai_agents/_mcp.py index c0c4120d..3c9e260c 100644 --- a/python/openai_agents/src/temporalio/openai_agents/_mcp.py +++ b/python/openai_agents/src/temporalio/openai_agents/_mcp.py @@ -27,7 +27,9 @@ ) from temporalio.openai_agents._heartbeat_decorator import auto_heartbeater from temporalio.openai_agents._mcp_server_checks import ( + _has_dynamic_tool_filter, _has_worker_side_require_approval, + _reject_dynamic_tool_filter, _reject_worker_side_require_approval, ) from temporalio.worker import PollerBehaviorSimpleMaximum, Worker @@ -190,6 +192,7 @@ def _create_server(self, factory_argument: Any | None) -> MCPServer: ) else: server = cast(Callable[[], MCPServer], self._server_factory)() + _reject_dynamic_tool_filter(self._server_name, server) _reject_worker_side_require_approval( self._server_name, server, "stateless_mcp_server" ) @@ -494,6 +497,7 @@ def _server_id(): def _server() -> MCPServer: server = self._servers[_server_id()] + _reject_dynamic_tool_filter(self._server_name, server) _reject_worker_side_require_approval( self._server_name, server, "stateful_mcp_server" ) @@ -538,11 +542,13 @@ async def connect( "Cannot connect to an already running server. Use a distinct name if running multiple servers in one workflow." ) server = self._server_factory(args.factory_argument if args else None) - # A server with a worker-side approval policy is never connected. - # The dedicated worker still runs, and each of its operations fails - # with the policy error, so the workflow sees that error at its - # first MCP call whatever its schedule-to-start timeout. - rejected = _has_worker_side_require_approval(server) + # A server with a callable tool filter or a worker-side approval + # policy is never connected. The dedicated worker still runs, and + # each of its operations fails with the same error, so the workflow + # sees that error at its first MCP call whatever its + # schedule-to-start timeout. + has_filter = _has_dynamic_tool_filter(server) + rejected = has_filter or _has_worker_side_require_approval(server) self._servers[server_id] = server try: try: diff --git a/python/openai_agents/src/temporalio/openai_agents/_mcp_server_checks.py b/python/openai_agents/src/temporalio/openai_agents/_mcp_server_checks.py index 41eb2729..221340fc 100644 --- a/python/openai_agents/src/temporalio/openai_agents/_mcp_server_checks.py +++ b/python/openai_agents/src/temporalio/openai_agents/_mcp_server_checks.py @@ -21,6 +21,11 @@ _SDK_APPROVAL_EVALUATOR = getattr(MCPServer, "_get_needs_approval_for_tool", None) +def _has_dynamic_tool_filter(server: MCPServer) -> bool: + """Return whether the server has a callable ``tool_filter``.""" + return callable(getattr(server, "tool_filter", None)) + + def _reject_dynamic_tool_filter(name: str, server: MCPServer) -> None: """Fail fast on a worker-side filter that can never be applied. @@ -29,7 +34,7 @@ def _reject_dynamic_tool_filter(name: str, server: MCPServer) -> None: list-tools Activity would otherwise retry forever. A static filter needs neither and is applied here as usual. """ - if callable(getattr(server, "tool_filter", None)): + if _has_dynamic_tool_filter(server): raise ApplicationError( f"MCP server {name!r} sets a callable tool_filter on its worker-side " "MCPServer, which cannot be applied there because the run context " diff --git a/python/openai_agents/tests/test_openai.py b/python/openai_agents/tests/test_openai.py index 1229b15d..73b36fbd 100644 --- a/python/openai_agents/tests/test_openai.py +++ b/python/openai_agents/tests/test_openai.py @@ -3083,19 +3083,24 @@ def factory(args: Any | None) -> MCPServer: ) -@pytest.mark.parametrize("instance_evaluator", [False, True]) +@pytest.mark.parametrize( + "setting", ["require_approval", "instance_evaluator", "tool_filter"] +) @pytest.mark.parametrize("stateful", [True, False]) -async def test_mcp_server_factory_require_approval_is_rejected( - client: Client, stateful: bool, instance_evaluator: bool +async def test_mcp_server_factory_settings_are_rejected( + client: Client, stateful: bool, setting: str ): - if instance_evaluator: + if setting == "require_approval": + tracking_server = get_tracking_server("HelloServer", require_approval="always") + elif setting == "instance_evaluator": # The default policy, but an evaluator that always asks. tracking_server = get_tracking_server("HelloServer") tracking_server._get_needs_approval_for_tool = ( # type: ignore[method-assign] lambda tool, agent: True # type: ignore[reportUnknownLambdaType] ) else: - tracking_server = get_tracking_server("HelloServer", require_approval="always") + tracking_server = get_tracking_server("HelloServer") + tracking_server.tool_filter = lambda context, tool: True # type: ignore[attr-defined] server: StatefulMCPServerProvider | StatelessMCPServerProvider = ( StatefulMCPServerProvider("HelloServer", lambda _: tracking_server) if stateful @@ -3136,9 +3141,14 @@ async def test_mcp_server_factory_require_approval_is_rejected( assert isinstance(err.value.cause, ActivityError) assert isinstance(err.value.cause.cause, ApplicationError) assert err.value.cause.cause.non_retryable - route = "stateful_mcp_server" if stateful else "stateless_mcp_server" - assert "'HelloServer' sets require_approval" in err.value.cause.cause.message - assert f"{route}(require_approval=...)" in err.value.cause.cause.message + message = err.value.cause.cause.message + if setting != "tool_filter": + route = "stateful_mcp_server" if stateful else "stateless_mcp_server" + assert "'HelloServer' sets require_approval" in message + assert f"{route}(require_approval=...)" in message + else: + assert "'HelloServer' sets a callable tool_filter" in message + assert "temporal_mcp_server()" in message # The rejected server is never connected, cleaned up or called. assert tracking_server.calls == [] if stateful: