diff --git a/python/openai_agents/README.md b/python/openai_agents/README.md index 5fbb5bf5..e62322f5 100644 --- a/python/openai_agents/README.md +++ b/python/openai_agents/README.md @@ -540,7 +540,10 @@ 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, in +every MCP operation, direct `call_tool()` and prompt calls included; a static +filter still works. Remove such a filter from the factory in the same deploy, +or let open runs finish first. ### 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..fac7c391 100644 --- a/python/openai_agents/src/temporalio/openai_agents/_mcp.py +++ b/python/openai_agents/src/temporalio/openai_agents/_mcp.py @@ -27,12 +27,21 @@ ) 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 from temporalio.workflow import ActivityConfig, ActivityHandle +# What the deprecated routes' tool_filter error tells the user to do instead. +_LEGACY_TOOL_FILTER_REMEDY = ( + "use a static filter (create_static_tool_filter) on the factory's server, " + "or register the factory with OpenAIAgentsPlugin(mcp_servers=...) and pass " + "the filter to temporal_mcp_server(tool_filter=...)" +) + @dataclasses.dataclass class _StatelessListToolsArguments: @@ -164,7 +173,9 @@ def __init__( name: The name of the MCP server. server_factory: A function which will produce MCPServer instances. It should return a new server each time so that state is not shared between workflow runs. It may accept a single positional parameter, which - receives a ``factory_argument`` from the workflow. + receives a ``factory_argument`` from the workflow. A callable ``tool_filter`` or a ``require_approval`` + policy that can ask for approval on the returned server is rejected in every MCP operation; a static + filter is applied. """ warnings.warn( "StatelessMCPServerProvider is deprecated; use " @@ -190,6 +201,9 @@ 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, _LEGACY_TOOL_FILTER_REMEDY + ) _reject_worker_side_require_approval( self._server_name, server, "stateless_mcp_server" ) @@ -467,7 +481,8 @@ def __init__( name: The name of the MCP server. server_factory: A function which will produce MCPServer instances. It should return a new server each time so that state is not shared between workflow runs. It receives an optional ``factory_argument`` from the - workflow. + workflow. A callable ``tool_filter`` or a ``require_approval`` policy that can ask for approval on the + returned server is rejected in every MCP operation; a static filter is applied. """ warnings.warn( "StatefulMCPServerProvider is deprecated; use " @@ -494,6 +509,9 @@ def _server_id(): def _server() -> MCPServer: server = self._servers[_server_id()] + _reject_dynamic_tool_filter( + self._server_name, server, _LEGACY_TOOL_FILTER_REMEDY + ) _reject_worker_side_require_approval( self._server_name, server, "stateful_mcp_server" ) @@ -538,11 +556,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..a15815df 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,20 +21,28 @@ _SDK_APPROVAL_EVALUATOR = getattr(MCPServer, "_get_needs_approval_for_tool", None) -def _reject_dynamic_tool_filter(name: str, server: MCPServer) -> 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, + remedy: str = "pass the filter to temporal_mcp_server() instead", +) -> None: """Fail fast on a worker-side filter that can never be applied. A callable ``tool_filter`` needs the run context and agent, which exist only in the workflow, so the Agents SDK rejects one on the worker side and the list-tools Activity would otherwise retry forever. A static filter needs - neither and is applied here as usual. + neither and is applied here as usual. ``remedy`` ends the error message. """ - 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 " - "and agent it receives exist only in the workflow; pass the filter " - "to temporal_mcp_server() instead", + f"and agent it receives exist only in the workflow; {remedy}", non_retryable=True, ) diff --git a/python/openai_agents/src/temporalio/openai_agents/_temporal_openai_agents.py b/python/openai_agents/src/temporalio/openai_agents/_temporal_openai_agents.py index 98fa6fc0..b8803a58 100644 --- a/python/openai_agents/src/temporalio/openai_agents/_temporal_openai_agents.py +++ b/python/openai_agents/src/temporalio/openai_agents/_temporal_openai_agents.py @@ -371,7 +371,10 @@ def __init__( Useful for testing or custom model integrations. mcp_server_providers: Legacy MCP server providers to register with the worker. Deprecated; use ``mcp_servers`` for new MCP v2 - integrations. + integrations. A provider's server with a callable + ``tool_filter`` or a ``require_approval`` policy that can ask for + approval is rejected in every MCP operation; a static filter is + applied. mcp_servers: Named OpenAI MCP server factories to register on the worker. Each factory may declare at most one positional parameter, which receives the workflow's ``factory_argument`` (``None`` when the diff --git a/python/openai_agents/tests/test_openai.py b/python/openai_agents/tests/test_openai.py index 7070946b..bc25aa93 100644 --- a/python/openai_agents/tests/test_openai.py +++ b/python/openai_agents/tests/test_openai.py @@ -3079,19 +3079,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 @@ -3132,9 +3137,15 @@ 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 "create_static_tool_filter" in message + assert "temporal_mcp_server(tool_filter=...)" in message # The rejected server is never connected, cleaned up or called. assert tracking_server.calls == [] if stateful: