Repository navigation
Conversation
- 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.
…oviders' 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.
Run the tool_filter check from #275's new _mcp_server_checks.py, with the registered server name, in the same places as the require_approval check.
a8c8da0 to
bf1e422
Compare
DABH
left a comment
DABH
left a comment
There was a problem hiding this comment.
Review at head bf1e422. Main has nothing newer under python/openai_agents, and the two 'Address review findings' commits from #275's round net to zero against it, so the diff is the fix alone. It closes the gap the #275 review listed as out of scope: the legacy routes lacked the v2 route's callable tool_filter guard. In a worktree at the head: make lint clean, make test 447 passed, and the six test_mcp_server_factory_settings_are_rejected cases passed three more times in a row; with the head's test file on main, the two tool_filter cases fail with DID NOT RAISE WorkflowFailureError and the other four pass. End-to-end probes on a local dev server used a real MCPServerStdio, so the Agents SDK applies the filter itself. The issue's reproduction exits 1 on main (both deprecated routes at list-tools attempt 6 with the retryable UserError after 30 s, controls as in the issue) and 0 at the head (both fail at once with the non-retryable message, and the Workflow fails instead of looping on Workflow Tasks). A static filter still completes on both routes at the head. A Workflow that only makes direct call_tool() and list_prompts() calls completes on main and fails at its first call at the head. A run left retrying list-tools on main (attempt 4) fails at its next attempt once a head Worker picks it up. The body discloses both. A rejected stateful server is never connected. Replay: five histories recorded on main (a static filter on each route, direct calls on each route, and a bounded-retry UserError failure) replay on the head, and the five recorded at the head replay on main. The check runs only inside Activities, and no command, Activity type or input changes, so no version gate is needed. Dropped after checking: the cached-tool-list path from the #275 review (a callable filter on these routes could never list on main, stateless caching is off by default and the stateful proxy has no cache); a non-callable, non-dict tool_filter passing the predicate (invalid natively too, where the SDK raises ValueError); and the session Activity outliving a failed Workflow (the same 'Activity not found on completion' warning appears for healthy stateful runs on main). Nothing material was left unverified.
Severity: P1 = wrong result or stuck run in realistic use; P2 = correctness or robustness in an edge case; P3 = docs, tests, cleanup.
Where the problems cluster: both comments are about what a user of the deprecated routes is told, in the error's remedy and in the API docs. Approving: the fix is correct at all three call sites, its tests fail on main and pass here, replay is unaffected in both directions and the break is disclosed; both comments are non-blocking.
PR body: it still describes the stacked state. 'Stacked on #275 ... merge it after #275': #275 merged as aad7a9d and GitHub retargeted this PR to main. 'The check, _reject_dynamic_tool_filter, moves from _mcp_backend.py to _mcp.py': #275's round already moved it to _mcp_server_checks.py; this PR adds _has_dynamic_tool_filter there and calls the check from _mcp.py. 'all 4 cases pass': there are six now, counting #275's instance_evaluator case. 'Draft while #275 and the breaking change are open': it is ready for review. 'StatefulMCPServerProvider before its session connects' reads as if the session Activity fails. It does not: it skips connect() and keeps the dedicated Worker running so that each operation fails, as the comment at _mcp.py:545-549 says. GitHub also lists no closing issue for this PR (closingIssuesReferences is empty, probably because the PR was opened against #275's branch), so as things stand merging will not close #290. Editing the body or linking the issue under Development should restore the link.
Parity: the TypeScript providers on main already reject a callable toolFilter on the factory's server in every operation (mcp-provider.ts:14-25, stateful-mcp-provider.ts:76), so this brings Python in line. Two differences remain. Python's deprecated routes lack the workflow-side option (your 'Not covered'). TypeScript also tags the failure MCPToolFilterUnsupported, while Python's error has no type; that predates this PR on the v2 route.
| ) | ||
| else: | ||
| server = cast(Callable[[], MCPServer], self._server_factory)() | ||
| _reject_dynamic_tool_filter(self._server_name, server) |
There was a problem hiding this comment.
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.
| `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 |
There was a problem hiding this comment.
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.'
Stacked on #275. This PR's base is #275's branch; merge it after #275.
What was changed
StatelessMCPServerProviderandStatefulMCPServerProvidernow reject a callabletool_filteronthe server that the Worker's factory returns, with the same non-retryable
ApplicationErrorthatOpenAIAgentsPlugin(mcp_servers=...)already raises for it:require_approvalpolicy, so every MCP Activity of theseroutes fails for such a server:
StatelessMCPServerProviderwhen it creates the server, andStatefulMCPServerProviderbefore its session connects and in every operation on the dedicatedWorker. A rejected server is never connected.
_reject_dynamic_tool_filter, moves from_mcp_backend.pyto_mcp.py, next toBreaking: reject a require_approval policy on the OpenAI Agents MCP factory's server #275's check, since
_mcp.pymust stay importable with MCP v1._mcp_backend.pyimports it fromthere and still runs it for
temporal_mcp_server().Breaking change: a Workflow that uses such a server only for direct
call_tool()orlist_prompts()calls never lists tools, so it completes today; natively, too, the filter appliesonly to tool listing. With this change its first MCP Activity fails with the error above. Runs where
an agent lists the tools already fail today, with a retryable
UserError; they now fail at once.There is no version gate. The check runs only in Activities, so recorded Histories replay as before.
A run still open when its Worker is upgraded fails at its next MCP Activity for that server. Before,
removing the filter from the factory let a run that was retrying the list Activity continue; now it
has to be started again.
Upgrade: pass the filter to
temporal_mcp_server(tool_filter=...), withOpenAIAgentsPlugin(mcp_servers=...), or use a static filter (create_static_tool_filter(...)) onthe factory's server.
Why?
The Agents SDK applies a callable
tool_filterwhen an agent lists tools, and raisesUserError("run_context and agent are required for dynamic tool filtering")when it gets no runcontext or agent. The deprecated providers' list Activity calls
list_tools()with neither, soevery attempt raised that retryable error. Under the default config the Activity retried forever
and the run never reached the model.
temporal_mcp_server()already rejects such a filter, on everyMCP Activity, with a clear error. This change gives the deprecated routes the same rule, and the same
place as #275's
require_approvalcheck.Checklist
Closes [Bug] openai_agents: a callable tool_filter on a deprecated MCP provider's server makes tool listing retry forever #290
How was this tested:
test_mcp_server_factory_require_approval_is_rejectedfrom Breaking: reject a require_approval policy on the OpenAI Agents MCP factory's server #275 becomestest_mcp_server_factory_settings_are_rejected, with a callabletool_filtercase next to therequire_approvalone, stateless and stateful. It also checks that the error names therejected setting. On Breaking: reject a require_approval policy on the OpenAI Agents MCP factory's server #275's branch the 2
tool_filtercases fail withDID NOT RAISE WorkflowFailureError; with the change all 4 cases pass, and theypass again in each of 5 runs in a row.
mainand on Breaking: reject a require_approval policy on the OpenAI Agents MCP factory's server #275's branch (both deprecatedroutes still retrying
UserErrorafter 30 seconds) and 0 with this change (both fail at oncewith the error above). Its controls are the same on all of them.
call_tool()andlist_prompts()directly through both deprecated routesshows the breaking change: on
mainand on Breaking: reject a require_approval policy on the OpenAI Agents MCP factory's server #275 all four calls complete, as they do natively; withthis change all four fail with the error above.
mainwith a callable and a static filter (stateless, maximum attemptsmake syncandmake lint(ruff, pyright, mypy, basedpyright,pydocstyle) pass.
make testran on a loaded machine: 430 passed and 4 failed. One was the newstateless
tool_filtercase, which hit the test's 10-second execution timeout withError reporting WFT to serverin the log; the other three are tracing and tool-error tests thischange does not touch. Run alone, all 4 pass. CI is the full run.
Any docs updates needed?
The README's paragraph on the deprecated APIs is updated here.
Not covered:
stateful_mcp_server()has no replacement with a persistent session, so a stateful caller that needs one has no route.
A
tool_filteroption onstateless_mcp_server()andstateful_mcp_server(), like openai_agents: add require_approval to stateless_mcp_server() and stateful_mcp_server() #220'srequire_approval, would add one.temporalio1.34.x still bundles the old providers astemporalio.contrib.openai_agents. Nobackport here.
Draft while #275 and the breaking change are open.