Skip to content

Breaking: reject a callable tool_filter on the deprecated OpenAI Agents MCP providers' servers #291

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
campoy wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Breaking: reject a callable tool_filter on the deprecated OpenAI Agents MCP providers' servers #291

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
768292a
Address review findings
campoy Oct 29, 2026
c396427
Address review findings
campoy Oct 30, 2026
37c49d6
openai_agents: reject a callable tool_filter on the deprecated MCP pr…
campoy Oct 30, 2026
bf1e422
Merge temporalio/ai-integrations#275's review round
campoy Oct 30, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion python/openai_agents/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

@DABH DABH Nov 1, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P3 (non-blocking): this sentence is the only place the new rule is documented, and it leaves out the part that breaks code that works today. The API docs for these routes do not mention it. stateless_mcp_server() and stateful_mcp_server() document only the require_approval rule (workflow.py:387-391, :450-454). The providers' server_factory arguments (_mcp.py:167-169, :471-473) and mcp_server_providers (_temporal_openai_agents.py:372-374) document neither rule, although the mcp_servers entry just below states both rejections for the v2 route. Release notes carry only the commit subject, so this paragraph is also where an upgrader learns that the check runs in every operation. Probes: a Workflow that only calls call_tool() and list_prompts() on such a server completes on main but fails at its first MCP Activity at the head, on both routes. A run left retrying list-tools on main fails at its next attempt once a head Worker picks it up. Suggest one sentence in both server_factory docstrings and in mcp_server_providers (a callable tool_filter on the returned server is rejected in every MCP operation; a static one is applied). Here, something like: '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.'

static filter still works.

### Hosted MCP Tool

Expand Down
16 changes: 11 additions & 5 deletions python/openai_agents/src/temporalio/openai_agents/_mcp.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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)

@DABH DABH Nov 1, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P3 (non-blocking): on the deprecated routes, the error's only remedy, 'pass the filter to temporal_mcp_server() instead', is incomplete, and following it as written trades one endless retry for another. temporal_mcp_server() reaches only factories registered with OpenAIAgentsPlugin(mcp_servers=...). Probe at the head: a Worker that keeps StatelessMCPServerProvider("tools-filtered", factory), with the Workflow changed to temporal_mcp_server("tools-filtered", tool_filter=drop_danger), is still running after 20 s. temporalio.mcp.tools-filtered.list-tools is at attempt 6, and its last failure is the Worker's retryable NotFoundError ('is not registered on this worker'). A stateful user also loses the persistent session on that route (your 'Not covered' says so). The message never mentions the in-place fix that works on both deprecated routes: a static filter on the factory's server. The body's Upgrade paragraph has the complete answer. The #275 review made the same point about the approval message, and #275 fixed it by passing the route (line 197). Do the same here. Give _reject_dynamic_tool_filter an optional route or remedy argument that _mcp_backend.py:163 leaves at today's text, and have lines 195 and 500 say something like '...; 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=...)'. The assertion at tests/test_openai.py:3147 would need the new text.

_reject_worker_side_require_approval(
self._server_name, server, "stateless_mcp_server"
)
Expand Down Expand Up @@ -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"
)
Expand Down Expand Up @@ -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:
Expand Down
7 changes: 6 additions & 1 deletion python/openai_agents/src/temporalio/openai_agents/_mcp_server_checks.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

Expand All @@ -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 "
Expand Down
26 changes: 18 additions & 8 deletions python/openai_agents/tests/test_openai.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -3132,9 +3137,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:
Expand Down
Loading