Repository navigation
Breaking: reject a require_approval policy on the OpenAI Agents MCP factory's server - #275
Conversation
…server The agent reads the approval policy from the MCP server it holds in the Workflow. A require_approval set on the server that the Worker's factory returns is never read, so the tools ran without approval and nothing failed or warned. Reject such a policy where the plugin creates the factory's server, on all three routes: OpenAIAgentsPlugin(mcp_servers=...) with temporal_mcp_server(), StatelessMCPServerProvider and StatefulMCPServerProvider. The error is a non-retryable ApplicationError, like the one for a callable tool_filter there, and says to pass require_approval to temporal_mcp_server() or to the deprecated helpers. A policy that never asks for approval, such as "never", has the same effect as no policy and is allowed. Breaking: runs whose factory sets such a policy completed before and now fail.
DABH
left a comment
DABH
left a comment
There was a problem hiding this comment.
Review at head 165f7c7 (merge base 69db660). Recipe: ten finder angles, 52 raw items deduplicated to 23 candidates, one adversarial verifier per candidate (most with end-to-end runs on the dev server across the v2, stateless and stateful routes), then a gap sweep that found nothing new. Verdicts: 16 confirmed, 2 plausible, 5 refuted and dropped (the stateful 'rejected mode' keeping the dedicated Worker alive is the right design, histories are event-for-event identical to healthy sessions; a cleanup() exception during cancellation keeps base precedence; rejected servers hold nothing releasable before connect(); the heartbeat task ordering changes nothing measurable; a startup probe of factories contradicts their documented contract). In a worktree at the head: make lint clean, make test 432 passed, conventions OK, Activity names byte-identical to base. Not verified: the body's reproduction exit codes and 4-of-4 replay run; the 'about 7 seconds' figure was not reproduced (the rejection path takes 50 to 300 ms).
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: the check itself is correct for every SDK-normalized form and the rejection is the right call. One behavioural gap: a run that already holds its cached tool list meets the check at call-tool, where the Agents SDK turns the non-retryable failure into tool-error text and the run completes. Everything else is wording that misstates consequences, tests whose guards are vacuous or whose deadlines were already purged from this file once, and layering of the helper in the deprecated module.
PR body: 'Draft while the breaking change and this question are open.' is stale (not a draft); the open question about the private field is answered by the evidence above; 'a mapping or tool list with an "always" entry' over-generalizes (an 'always' entry with an empty tool_names list normalizes to {} and is allowed); the first commit's subject lacks the Breaking: prefix the title carries, and release notes derive from commit subjects, so the squash subject should keep the title; the upgrade guidance should add the cached-tool-list behaviour from the P2. Pre-existing, out of scope: the legacy routes lack the callable tool_filter guard the v2 route has; the except asyncio.CancelledError: pass around await heartbeat_task can swallow the Activity's own cancellation in a one-await window; the general session-start failure gap (factory or connect() failures surface only via schedule-to-start as a generic DedicatedWorkerFailure, or never); no v2 test covers workflow-side temporal_mcp_server(..., require_approval=...) although it works.
| def create(argument: Any = _NOT_SUPPLIED) -> _OpenAIMCPServerBackend: | ||
| server = cast(MCPServer, invoke(argument)) | ||
| _reject_dynamic_tool_filter(name, server) | ||
| _reject_worker_side_require_approval(name, server) |
There was a problem hiding this comment.
P2: when a run already holds its default-cached tool list, the first MCP Activity to meet the check is call-tool, and the Agents SDK converts the non-retryable rejection into model-visible tool-error text, so the run completes instead of failing. The body's 'the first MCP Activity fails ... so the run fails unless the Workflow catches it' and 'a run still open when its Worker is upgraded fails at its next MCP Activity' do not hold on that path. Reproduced end to end: a v2 run (cache_tools_list=True by default; TemporalMCPClient returns the cache without an Activity, python/mcp/src/temporalio/mcp/_workflow.py:97-98, rebuilt by replay after a restart) waiting on a signal when the Workers are upgraded, or when the factory's policy is added, without draining: activities [list-tools, model, call-tool, model, model, call-tool, model]; call-tool fails with the require_approval message (non_retryable=True); the SDK wraps it (agents/mcp/util.py:733-762, tool.py:653-667, default_tool_error_function); the model receives 'An error occurred while running the tool. Please try again. Error: ... Activity task failed' with no reason; the Workflow COMPLETES with an answer produced without the tool; the require_approval text exists only in the ActivityTaskFailed event. With failure_error_function=None the run fails, but as a retryable AgentsWorkflowError, because _reraise_workflow_failure (_openai_runner.py:469-477) inspects only the direct cause, the ActivityError, not its non-retryable ApplicationError. The stateful proxy has no cache; new runs fail loudly at list-tools; tools never run without approval. Fix the upgrade guidance in the body and README (drain open runs, or pass failure_error_function=None on the workflow-side server), and optionally make _reraise_workflow_failure classify through an ActivityError's cause.
There was a problem hiding this comment.
Thanks, confirmed: the tool still never runs, but a run that already holds its cached tool list can end without the error. The README and the PR body now say so: let open runs finish before upgrading, or pass failure_error_function=None to temporal_mcp_server() to fail such a run. _reraise_workflow_failure looking only at the direct cause is older and affects every non-retryable Activity error, so I filed it separately: #302.
| if callable(policy) or policy: | ||
| raise ApplicationError( | ||
| f"MCP server {name!r} sets require_approval on its worker-side " | ||
| "MCPServer, where the agent never reads it, so its tools would run " |
There was a problem hiding this comment.
P3: the message (and the helper docstring at 36-38 and README 449-451) asserts 'so its tools would run without approval', which is false whenever the workflow-side policy is also set. That belt-and-braces configuration (MCPServerStdio(..., require_approval="always") in the factory and temporal_mcp_server(name, require_approval="always") in the Workflow) enforced approval before this PR, because the agent reads the policy from the proxy it holds (agents/mcp/util.py:565, server.py:810-841) and all three proxies set it from the workflow-side argument (98, 355; _temporal_mcp_server.py:37); verified on all three routes with the check neutralized (zero tool executions, 'Approval needed'). At the head it is rejected with a message that misstates the consequence and recommends the remedy the user already applied. Realistic population: adopters of #220 who left the then-harmless copy on the factory, and factories shared with non-Temporal code (the body's own example). State the mechanism without asserting the run's approval behaviour: 'the agent never reads that copy, so the policy takes effect only when passed to temporal_mcp_server() ...; remove it from the factory's server'; shorten the docstring to point at the message; qualify the README sentence ('without a workflow-side policy its tools would run without approval').
There was a problem hiding this comment.
Agreed. The message now states the mechanism only: "MCP server 'X' sets require_approval on its worker-side MCPServer; the agent never reads that copy, so the policy takes effect only when passed to (require_approval=...); remove it from the factory's server". The docstring points to the message, and the README sentence now says "without a workflow-side policy".
| """ | ||
| # The Agents SDK keeps the normalized policy only in this private field: | ||
| # a bool, a dict of tool names to bools, or a callable. | ||
| policy = getattr(server, "_needs_approval_policy", False) |
There was a problem hiding this comment.
P3: the check keys only on the private _needs_approval_policy field, but the agent consumes approval through _get_needs_approval_for_tool, so a subclass overriding that method pauses under the plain SDK yet passes this check and runs its tools without approval; conversely any object whose attribute is auto-created and callable (a spec-less MagicMock or AsyncMock) is now rejected on the two legacy routes for a policy it never set. (a) class S(InProcessMCPServer) with _get_needs_approval_for_tool returning True in a plugin factory: the backend is built, the v2 Workflow returned the answer with the tool executed and no pause, while the plain SDK run reported interruptions; same on both legacy routes. The body scopes the check to the field, and overriding a private method is outside the documented API, so an accepted edge. (b) StatelessMCPServerProvider('x', lambda _: AsyncMock()) worked before the PR and a spec-less mock now fails every MCP Activity non-retryably; the v2 route already rejected spec-less mocks through the sibling tool_filter guard (_mcp_backend.py:161). Treat an overridden evaluator as unevaluable like the callable case (if type(server)._get_needs_approval_for_tool is not MCPServer._get_needs_approval_for_tool: raise; no SDK class overrides it in 0.22.0 to 0.22.3) and mention spec=MCPServer for mocks in the README.
P3 (optional robustness): getattr(server, "_needs_approval_policy", False) fails open: were the field absent, every factory policy would pass on all three routes with no warning until the next pin bump failed the tests. No plausible trigger: all 0.22.x releases carry the field, 0.23.1 and 0.24.0 keep it, the name has been stable since 0.14.0, and upstream has never patched a superseded minor across 124 releases; that evidence also answers the body's open question about reading the private field. If wanted, a cached one-time probe constructing MCPServerStdio(params={"command": "unused"}, require_approval="always") (microseconds, nothing spawned) that fails the Worker unless the field is True, keeping the lenient default for user subclasses.
There was a problem hiding this comment.
(a) Done: a subclass that overrides _get_needs_approval_for_tool now counts as a policy that can ask, like a callable, with a new test. (b) I kept mocks out of the README: a spec-less mock now fails the same way it already did on the v2 route, and a mock with spec=MCPServer passes both checks (checked). The startup probe: not added, given the field's history you found; I moved that evidence into the PR body as the answer to the open question.
| 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: |
There was a problem hiding this comment.
P3 (tests): the new cleanup() guard in the tracking server raises before recording the call, so the stateful rejection test's 'never cleaned up' assertion (calls == [] at 3023) is vacuous; its comment also attributes connect-time exit stacks to servers that do not exist. Dropping if not rejected: around await server.cleanup() at _mcp.py:594 (the base shape) makes cleanup() run on the never-connected server and raise AttributeError into an already-closed Workflow, yet all seven assertions of the stateful case still pass (reproduced with a spy: cleanup called, calls == []); recording the call before raising makes 3023 fail on that regression. The stateless variant still catches it (3020-3021). Every openai-agents release from 0.3.2 to 0.22.3 creates the exit stack in __init__, and MCPServerStdio, Sse and StreamableHttp each returned normally from cleanup() twice before any connect(). Append 'cleanup' before raising (or assert on a flag) and reword the comment to its actual purpose: SDK servers tolerate cleanup() before connect(); raise here so a cleanup() call on a rejected server is detectable.
There was a problem hiding this comment.
Good catch. The tracking server now records cleanup before it raises, and the comment says what the guard is for. With the base shape put back (no if not rejected: around cleanup()), the stateful rejection test now fails on calls == [].
| finally: | ||
| await server.cleanup() | ||
| heartbeat_task.cancel() | ||
| try: |
There was a problem hiding this comment.
P3 (tests): the nested try/finally that guarantees the heartbeat task is cancelled even when server.cleanup() raises, a guarantee the body states, is exercised by no test. A private copy with 593-601 reverted to the base's flat form (guard kept) passes all 432 tests, while a stateful tracking server whose cleanup() raises after a normal connect() then leaks one forever-sleeping heartbeat_every task per session, exactly the base behaviour the PR fixes; at the head the same scratch shows zero leaked tasks. Give get_tracking_server a cleanup_error parameter (raise after appending 'cleanup') and add a stateful test running McpServerStatefulWorkflow.run(timedelta(seconds=30), None, None) with it, asserting the result, calls[-1] == 'cleanup', provider._servers == {} and the existing no-heartbeat-task scan after the Worker exits (passes at head in 0.44 s, fails on the flat-finally mutant).
There was a problem hiding this comment.
Covered, together with the heartbeat change below: the session now uses auto_heartbeater(interval=30), so the nested finally is gone. test_mcp_server has a new stateful-cleanup-error mode where cleanup() raises after a normal session; it checks the result, the call order, _servers == {} and that no _heartbeat_every task is left.
| from temporalio.workflow import ActivityConfig, ActivityHandle | ||
|
|
||
|
|
||
| def _reject_worker_side_require_approval(name: str, server: MCPServer) -> None: |
There was a problem hiding this comment.
P3 (plausible, scope): the fix rejects only require_approval while every other agent-facing setting on the factory's server (use_structured_content, an explicit failure_error_function, tool_meta_resolver, custom_data_extractor, and with openai-agents 0.22.3 inside the pin, server-level tool_input_guardrails and tool_output_guardrails) is still silently ignored on all three routes. The body's reason for excluding tool_meta_resolver ('setting it on the Workflow side works') applies verbatim to require_approval, so the real scoping rule (only safety-weakening settings are rejected) is unstated, and under that rule the 0.22.3 guardrails are a same-class safety setting the check does not cover: MCPServerStreamableHttp(..., tool_input_guardrails=[block_pii]) is accepted by create() and the agent builds tools from the proxy whose guardrails are None, so the tool receives the input with no guardrail and no warning. use_structured_content=True on the factory with a default workflow-side server delivers the text block instead of the structured JSON (verified). Pre-existing and not made worse here; README 440-443 already routes workflow-facing settings but omits three of these. Warn once per server name at the check sites for any non-default agent-facing setting (fields versus defaults; guardrails via getattr), or at minimum state the scoping rule and list use_structured_content; a single _validate_factory_server(name, server) covering tool_filter, approval and guardrails is the deeper form.
There was a problem hiding this comment.
Agreed that the rule was unstated. The README now lists the agent-facing settings that belong on temporal_mcp_server(), and says that on the factory's server a static filter still applies, a callable filter and an approval policy are rejected, and the rest are ignored. The guardrails are the same safety class, so I filed them separately: #303. A callable tool_filter on the legacy routes is #291, stacked on this PR.
| # 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. | ||
| try: |
There was a problem hiding this comment.
P3 (cleanup, layering): on the stateful route the rejection is evaluated twice per session, first through try/except ApplicationError only to derive a boolean (the error is thrown away) and again on every op in _server() (511-514), although the SDK assigns the policy only in __init__; and the helper is homed in this deprecated module while its sibling _reject_dynamic_tool_filter lives in the extra-gated backend, which now imports it (_mcp_backend.py:29, the live route's first module-level dependency on the deprecated module). A future statement inside this try that raises ApplicationError is swallowed into rejected = True and surfaces at the first op as the require_approval message; when the deprecated providers go, _mcp_backend.py:29 breaks the v2 route's import until the helper is relocated. A single-evaluation provider reproduced every PR guarantee. Split into _has_worker_side_require_approval(server) -> bool plus the raising wrapper (keep the cheap per-op raise in _server(); never cache an exception instance), and put both checks in a small extra-free module (precedent: _heartbeat_decorator.py, _errors.py) without unifying them into one validator on the legacy routes without an explicit decision, since a callable tool_filter there fails retryably today.
There was a problem hiding this comment.
Done: both checks now live in _mcp_server_checks.py, which needs no mcp extra. The approval check is split into _has_worker_side_require_approval() and the raising wrapper; the session evaluates it once, and _server() keeps the per-operation raise. This PR doesn't run the tool_filter check on the legacy routes; #291 does that.
| except ApplicationError: | ||
| rejected = True | ||
|
|
||
| heartbeat_task = asyncio.create_task(heartbeat_every(30)) |
There was a problem hiding this comment.
P3 (cleanup): the session Activity re-implements the plugin's auto_heartbeater lifecycle (heartbeat_every at 544-548 duplicates _heartbeat_decorator._heartbeat_every, and 596-601 duplicates its cancel-and-await line for line), and the PR had to hand-fix two leak classes in that copy that the decorator's try/finally prevents structurally. It is not a drop-in only because the decorator heartbeats solely when a heartbeat timeout is set while the session needs an unconditional 30 s cadence (the Workflow's cancel reaches the Activity only through heartbeat responses). A fixed-interval decorator applied to connect reproduced identical outcomes, call order and leak-freedom on the normal and rejected paths and the same cancel-delivery latency (29.42 s versus 29.78 s). auto_heartbeater(interval=30) creating _heartbeat_every(interval) unconditionally, applied under @activity.defn(name=self.name + "-server-session"); delete heartbeat_every, the manual task and the nested finally (keeping if not rejected: await server.cleanup()); point the scan at tests/test_openai.py:3029 to _heartbeat_every. Pre-existing: the hard-coded 30 s ignores a configured session heartbeat_timeout.
There was a problem hiding this comment.
Done: auto_heartbeater takes an optional interval, the session uses auto_heartbeater(interval=30), and the hand-written heartbeat task and nested finally are gone. The model Activities keep the default. The hard-coded 30 s is unchanged.
| ) | ||
| def test_worker_side_require_approval_is_rejected(require_approval: Any) -> None: | ||
| def factory() -> AgentsMCPServer: | ||
| return MCPServerStdio( |
There was a problem hiding this comment.
P3 (tests): the two new v2 tests bypass the module's InProcessMCPServer fixture (used by the sibling tool_filter tests) with MCPServerStdio(params={"command": "unused"}, require_approval=...) and copy the backend-invocation cast twice more (five copies in the module), while the PR extended the other file's fixture exactly this way. Verified: an InProcessMCPServer subclass forwarding require_approval and **server_options yields identical normalized policies and byte-identical error messages for all six parametrized values; the pre-existing MCPServerStdio use at 332 legitimately needs the SDK class. Forward the options in InProcessMCPServer.__init__, build the new factories with it, and fold the cast into a create_backend(factory) helper.
P3 (tests, plausible): the never-allowed parametrization at 266 lists only "never" and {"say_hello": "never"} while the body documents False and never-only tool lists as allowed; the SDK normalizes those to the same values in 0.22.0 and 0.22.3, so no helper edit can distinguish them today, but extending the list to ["never", False, {"say_hello": "never"}, {"never": {"tool_names": ["say_hello"]}}] pins the documented promise.
There was a problem hiding this comment.
Done: InProcessMCPServer forwards server options, the new factories use it, and a create_backend() helper replaces the casts. The allowed list now also has False and a never-only tool list.
| factory_argument: Any | None, | ||
| require_approval: Literal["always"] | None, | ||
| ) -> str: | ||
| config = ActivityConfig(start_to_close_timeout=timedelta(seconds=30)) |
There was a problem hiding this comment.
P3 (tests): the conditional ActivityConfig mutation at 2688-2690 is unnecessary. A None schedule_to_start_timeout is indistinguishable from an absent key (typed timedelta | None; the SDK sets the proto field only when truthy; nothing in the plugin inspects config keys). The base's single expression with the widened annotation passes pyright, basedpyright and mypy, reproduces every assertion of the stateful test for None and the normal path for 30 s, and an outbound-interceptor capture shows identical StartActivityInput.schedule_to_start_timeout values. Keep timeout: timedelta | None at 2684 and pass the config inline as the base did.
There was a problem hiding this comment.
Done: back to the inline config, with timeout: timedelta | None.
- 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.
Like _mcp_backend.py, the module's private functions are used only from other modules.
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.
|
Thanks for the review. Replies are in the threads; this round is e8f6e88.
|
DABH
left a comment
DABH
left a comment
There was a problem hiding this comment.
Second round, at head e8f6e88 (two commits over 165f7c7, merge base 69db660). In a worktree at the head: make lint is clean (ruff, pyright, mypy, basedpyright, pydocstyle); make test gave 432 passed and 4 failures in tracing, sandbox and tool-argument tests this PR does not touch, all of which pass when re-run in one process (load from parallel runs on this machine); the MCP tests pass serially, and CI is green on all four Python cells. Activity names are byte-identical to the base and the workflow.py changes are docstring-only. Verified by mutation: restoring the unguarded cleanup() on a rejected stateful server fails test_mcp_server_factory_require_approval_is_rejected (['cleanup'] == []), and moving auto_heartbeater's cancel out of its finally fails the stateful-cleanup-error mode and the stateful rejection case with a leaked _heartbeat_every task. In openai-agents 0.22.3 and 0.24.0 no SDK class overrides _get_needs_approval_for_tool, so the new override check rejects only user subclasses, and no 0.22.x release has followed 0.22.3. One new finding, verified by replay on the dev server. Dropped after checking: the error text for an overridden evaluator still says 'sets require_approval' (no known subclass overrides it), and the README's new list of workflow-side settings (the proxy forwards them to MCPServer.__init__, and openai-agents 0.22.3 reads tool guardrails from the server the agent holds). Not verified at runtime: the guardrail path, since the lock pins 0.22.0.
Severity: P1 = wrong result or stuck run in realistic use; P2 = correctness or robustness in an edge case; P3 = docs, tests, cleanup.
Previous rounds. Addressed: the cached-tool-list behaviour (P2) is now in the README and the body, with the _reraise_workflow_failure classification filed as #302; the message states the mechanism only and the README says 'without a workflow-side policy'; an overridden evaluator is rejected and tested; the tracking server records cleanup before raising, so the rejection test now catches the unguarded cleanup() (confirmed above); the nested finally is gone with auto_heartbeater(interval=30), and the new stateful-cleanup-error mode covers the leak (confirmed above); the deadlines are 30 s; every doc site gives one rule and the two v2 docstrings point to it; the message uses the registered name and leads with the route's own option; both checks live in _mcp_server_checks.py, and the session evaluates the policy once; the test fixture forwards server options, create_backend() replaces the casts and the allowed list includes False and a never-only tool list; ActivityConfig is inline again; the PR body items are fixed and the squash will keep the Breaking: title. Accepted declines: no README note on spec-less mocks (they already failed on the v2 route, and spec=MCPServer passes both checks), no startup probe for the private field (its history is now in the body), tool guardrails (#303) and the legacy callable tool_filter (#291) as follow-ups, and the pre-existing items left out of scope. Still open: nothing from round one. The one new item comes from the remedy I suggested last round.
Decision: comment rather than approve only because the README's new upgrade remedy can leave open runs stuck in a Workflow Task retry loop; with that sentence qualified, this is ready to approve.
| These checks run in every MCP Activity. A run that already holds its cached tool | ||
| list, the default, first meets them when it calls a tool: the agent gets the | ||
| error as the tool's output, and the run can complete without the tool. To fail | ||
| such a run instead, pass `failure_error_function=None` to |
There was a problem hiding this comment.
P2: the remedy 'pass failure_error_function=None to temporal_mcp_server(...)' is itself a Workflow change that open runs replay, so a run whose History already holds a failed call-tool Activity for that server, including one that already met this rejection, fails replay with a nondeterminism error and gets stuck. I suggested this remedy last round without that caveat. Reproduced on the dev server with the plugin: a v2 Workflow using temporal_mcp_server("hello"), whose factory server's call_tool raises a non-retryable ApplicationError for one of two calls, completes with Hi Tom and Tim! and History [list-tools, model, call-tool (failed), model, call-tool, model]. Replaying that History with the unchanged Workflow passes. Replaying it with temporal_mcp_server("hello", failure_error_function=None) raises NondeterminismError: [TMPRL1100] Nondeterminism error: Fail workflow machine does not handle this event: HistoryEvent(id: 23, ActivityTaskScheduled); on a live Worker that is a Workflow Task retry loop. This paragraph targets runs like that: if the remedy ships after the upgrade, every open run that already met the rejection at call-tool and carried on has exactly this History. Earlier, unrelated failures count too: temporalio.mcp makes JSON-RPC errors such as INVALID_PARAMS and METHOD_NOT_FOUND non-retryable (_activities.py:48-57), and the default error function turned each into tool output. The setting also covers every tool failure on that server, so afterwards such errors fail the run instead of reaching the model as tool output it can recover from. Suggested: lead with 'let open runs finish before you upgrade or add such a setting to the factory's server', then qualify the alternative: 'Passing failure_error_function=None to temporal_mcp_server(...) makes such a run fail instead. It is a Workflow change: every tool failure on that server then fails the run, and an open run whose History already contains a failed call-tool Activity for that server fails replay with a nondeterminism error. Deploy it with the upgrade, and only where open runs have no earlier tool failure on that server.' The PR body's upgrade paragraph ('To make such a run fail instead ... pass failure_error_function=None') needs the same qualification.
There was a problem hiding this comment.
Thanks, agreed. Fixed in 6391dbf: the README now leads with letting open runs finish, and says that failure_error_function=None is a Workflow change. Every tool failure on that server then fails the run, and an open run whose History already has a failed call-tool Activity for that server fails replay, so deploy it with the upgrade, and only where open runs have no earlier tool failure there. The PR body's upgrade paragraph says the same.
The setting is a workflow change: an open run whose history already holds a failed call-tool Activity for that server fails replay, and every later tool failure fails the run. Lead with draining open runs.
DABH
left a comment
DABH
left a comment
There was a problem hiding this comment.
Third round, at head 6391dbf (one README commit over e8f6e88; merge base 69db660). In a worktree at the head, make lint is clean (ruff, pyright, mypy, basedpyright, pydocstyle) and make test gave 436 passed; a trial merge with current main (4bcde9a) is clean, lints clean and gives 437 passed; CI is green on all four Python cells. Replay, on the dev server: seven Histories recorded on main (v2, stateless and stateful routes, each with and without a factory-side policy, plus a workflow-side policy) replay on the head and on the merged tree, so 'recorded Histories replay as before' holds. Run live on the merged build, the three factory-policy cases fail at their first MCP Activity with the non-retryable message naming the registered server and the route's option, and the other four behave as on main. The findings below were verified by mutation (weakening the head's code under its tests, and running the head's tests against main's source) and by probes on the dev server. Checked and dropped: the check against 17 approval forms on the three SDK server classes (it matches the SDK's evaluator for every tool in 0.22.0, and 0.22.3 keeps the normalizer and evaluator unchanged); argument decoding through functools.wraps under @activity.defn (test_mcp_server_factory_argument[True] passes); a rejected v2 server entering the connection cache (it never does); an evaluator patched onto an instance rather than a subclass (outside any documented use); upgrade notes for the legacy routes (only the non-default stateless_mcp_server(cache_tools_list=True) keeps a cached list, and the legacy paragraph points back to the v2 rules); and a check that skips call-tool on the legacy routes (each legacy route checks in a helper that every operation goes through). TypeScript has nothing to align with: agents-core 0.14.3 takes an approval policy only on hosted MCP tools, not on local MCP servers. Not verified locally: Python 3.10 (CI covers it), and a stateful session carried across a real Worker restart.
Severity: P1 = wrong result or stuck run in realistic use; P2 = correctness or robustness in an edge case; P3 = docs, tests, cleanup.
Previous rounds. Addressed: last round's P2. The README now leads with letting open runs finish and calls failure_error_function=None a workflow change, with the replay condition and when to deploy it, and the PR body's upgrade paragraph says the same; the first comment below corrects one clause of the wording I suggested. Round-one items stand as recorded last round, since the code and tests are unchanged since e8f6e88. Accepted declines unchanged: spec-less mocks, the startup probe for the private field, #291, #302 and #303, and the pre-existing items left out of scope. Still open: nothing.
Where the problems cluster: the rejection is right on all three routes and replay is unaffected. What is left is the README's upgrade paragraph, which understates two workflow changes, and test coverage: the session's heartbeat and a mixed approval policy are untested, and the new cleanup-error mode also passes on main's source.
Decision: approve, because the check is correct on all three routes, recorded Histories replay, last round's blocking item is fixed and CI is green; the three items below are non-blocking documentation and test gaps.
| error as the tool's output, and the run can complete without the tool. Let open | ||
| runs finish before you upgrade or add such a setting to the factory's server. | ||
| Passing `failure_error_function=None` to `temporal_mcp_server(...)` makes such a | ||
| run fail instead. It is a workflow change: every tool failure on that server |
There was a problem hiding this comment.
P3 (docs, non-blocking): the upgrade guidance still understates two workflow changes: moving the policy into temporal_mcp_server(require_approval=...), which the error message prescribes, also breaks replay for open runs, and failure_error_function=None does not fail the run on every tool failure. The second half is wording I suggested last round. (1) Recorded on main (4bcde9a) on the dev server: a Workflow on temporal_mcp_server("bank-policy") whose factory server sets require_approval="always" completes with [list-tools, model, call-tool, model]. Replaying it on the merged tree with require_approval="always" passed to temporal_mcp_server("bank-policy", ...) raises [TMPRL1100] Nondeterminism error: Complete workflow machine does not handle this event: HistoryEvent(id: 17, ActivityTaskScheduled). The PR body says this ('let runs that use a factory policy finish on the earlier version first, or keep them there with Worker Versioning'); the README, which outlives the body, only implies it. (2) With temporal_mcp_server("hello", failure_error_function=None), a scripted model that sends say_hello the truncated arguments {"name": gets two Workflow Task failures, Invalid JSON input for tool say_hello, until the probe's 12 s execution timeout, instead of a failed run: the SDK raises ModelBehaviorError, which has no Temporal cause, so _reraise_workflow_failure (_openai_runner.py:461-477) leaves it to fail the Workflow Task. The same model with the default handler completes. Replaying that default-handler History, which has no failed Activity, with failure_error_function=None also fails (Workflow activation completion failed: Invalid JSON input for tool say_hello), so 'a failed call-tool Activity' is narrower than the real condition. Suggested for 465-472: 'Let open runs finish before you upgrade or add such a setting to the factory's server. Moving the policy into temporal_mcp_server(require_approval=...) is also a workflow change: an open run that already called one of that server's tools fails replay with a nondeterminism error. Passing failure_error_function=None to temporal_mcp_server(...) makes a run that holds its cached tool list fail instead, but it is a workflow change too: a failed tool Activity on that server then fails the run, a malformed tool call from the model fails the workflow task until the setting is removed, and an open run with any earlier tool error on that server, including a malformed call that never reached an Activity, fails replay. Deploy it with the upgrade, and only where open runs have no earlier tool error on that server.' Dropping the failure_error_function alternative altogether would also be fine.
There was a problem hiding this comment.
Thanks. I went with dropping the alternative, in 0632489: the paragraph leads with letting open runs finish, and now adds that moving the policy into temporal_mcp_server(require_approval=...) is also a workflow change that breaks replay for open runs that already called that server's tools. The PR body drops it too.
| return await _server().get_prompt(args.name, args.arguments) | ||
|
|
||
| @activity.defn(name=self.name + "-server-session") | ||
| @auto_heartbeater(interval=30) |
There was a problem hiding this comment.
P3 (tests, non-blocking): nothing checks that the session heartbeats, which is how a Workflow's cancel, or its completion, reaches this Activity, and the PR moved that cadence onto a new keyword whose omission silently turns it off. With this line changed to the bare @auto_heartbeater that the model Activities use, the session never heartbeats (its default config sets no heartbeat timeout, 333-335), yet all 11 MCP and stateful tests in test_openai.py pass. In a probe on the dev server, the session's cleanup() runs 30.0 s after the run completes at the head; with the bare decorator the server is still connected 50 s after completion, and nothing ends the connection or the dedicated Worker before the session's start-to-close timeout (one hour by default) or Worker shutdown. Two related gaps in the new checks: the leak scans (tests/test_openai.py:2905-2909 and 3042-3046) match only the head's helper name, so the new stateful-cleanup-error mode passes against main's source, which leaks its heartbeat_every task when cleanup() raises (widening the scan to "heartbeat_every" in task.get_coro().__name__ fails on main with that pending task); and stateful-approval returns at 2891, before the _servers and heartbeat checks, so the body's 'Every stateful mode now checks the heartbeat task' does not hold for that mode. The heartbeat gap predates the PR (removing main's heartbeat task also passes main's tests). Suggested: a unit test of auto_heartbeater(interval=0.01) under temporalio.testing.ActivityEnvironment with an on_heartbeat recorder and no heartbeat timeout; a scan that matches "heartbeat" in the coroutine name, or compares asyncio.all_tasks() before and after; and the stateful checks moved above the approval return.
There was a problem hiding this comment.
Added in 0632489. test_auto_heartbeater_interval_heartbeats_without_timeout runs auto_heartbeater(interval=0.01) under ActivityEnvironment with no heartbeat timeout and checks that it heartbeats, and that the bare decorator doesn't; a mutant that ignores interval fails it. The leak scans now match any task whose coroutine name contains heartbeat, and test_mcp_server runs the stateful checks before the approval mode returns. The test pins the decorator rather than the session's use of it; checking the session's 30 s cadence directly would need a 30 s test, so I left that out.
| [ | ||
| "always", | ||
| {"say_hello": "always"}, | ||
| {"always": {"tool_names": ["say_hello"]}}, |
There was a problem hiding this comment.
P3 (tests, non-blocking): no case mixes "always" and "never", so the dict branch is pinned only by single-entry policies, where any and all agree. Changing any(policy.values()) to all(...) at _mcp_server_checks.py:53 passes all 13 approval tests in both files. Under that change a mixed policy such as {"transfer": "always", "lookup": "never"}, the usual way to gate one dangerous tool, passes the check and is silently ignored again, and an always list with no names is rejected. Add {"say_hello": "always", "other": "never"} and {"always": {"tool_names": ["say_hello"]}, "never": {"tool_names": ["other"]}} to this list.
There was a problem hiding this comment.
Added both in 0632489. With any changed to all, those two cases now fail.
- README: lead with letting open runs finish, drop the failure_error_function=None alternative, and say that moving the policy into temporal_mcp_server() also breaks replay for open runs. - Test that auto_heartbeater(interval=...) heartbeats without a heartbeat timeout, scan for any heartbeat task, run the stateful checks before the approval mode returns, and add mixed always/never policies to the rejected list.
The SDK calls server._get_needs_approval_for_tool(), so an evaluator a constructor assigns on the instance replaces the SDK's, but the check looked only at the class. Resolve it with inspect.getattr_static, which finds an instance attribute before the class's method without running it, and count a custom __getattribute__ as an override too.
|
One more change after your approval, in f163287: the check now also rejects an approval evaluator assigned on the server instance ( |
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.
What was changed
The OpenAI Agents plugin now rejects a
require_approvalpolicy set on the MCP server that the Worker's factory returns, instead of silently ignoring it. The error is a non-retryableApplicationError, like the existing one for a callabletool_filterthere, and names the registered server and the option to use on that route:OpenAIAgentsPlugin(mcp_servers=...)andtemporal_mcp_server()(next to thetool_filtercheck),StatelessMCPServerProvider(in every MCP Activity) andStatefulMCPServerProvider. On the stateful route, the session Activity checks the policy once, before it connects, and every operation on the dedicated Worker raises the error. A rejected server is never connected or cleaned up."never",False, or a mapping or tool list with only"never"entries. Every other policy is rejected, including a mapping or tool list with an"always"entry that names a tool, and any callable, which cannot be evaluated on the worker. A server class or instance that replaces how the SDK evaluates the policy (_get_needs_approval_for_tool, including an evaluator assigned on the instance or a custom__getattribute__) is rejected the same way._mcp_server_checks.py, which needs nomcpextra. The approval check is a yes/no test plus a raising wrapper.auto_heartbeater(interval=30)instead of its own task.auto_heartbeatertakes an optional fixedinterval; the model Activities keep the default.stateless_mcp_server(),stateful_mcp_server(),temporal_mcp_server()andOpenAIAgentsPlugin(mcp_servers=...)give the same rule. The README also lists the agent-facing settings that belong ontemporal_mcp_server(), and says which are applied, rejected or ignored on the factory's server.On the stateful route, the Workflow gets the error at its first MCP call, whatever its
schedule_to_start_timeout. Withasync with stateful_mcp_server(...), the run then fails with anActivityErrorcaused by this error.Breaking change: an app whose MCP factory returns a server with a
require_approvalpolicy that can ask for approval completes its runs today, and the tools run without approval unless the Workflow also sets a policy. With this change, the first MCP Activity for that server fails with the error above. This includes apps that also pass the policy in Workflow code but left a copy on the factory's server, for example in a factory shared with code outside Temporal.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 meets the check at its next MCP Activity for that server. A new run, or one that lists tools again, fails there. A run that already holds its cached tool list (the default on
temporal_mcp_server()) first meets it when it calls a tool: the Agents SDK gives the agent the error as the tool's output, so the tool does not run but the run can complete without it.Upgrade: remove the policy from the factory's server and pass it to
temporal_mcp_server(require_approval=...), or tostateless_mcp_server()orstateful_mcp_server()(#220). A"never"policy can stay. Moving the policy into Workflow code makes the tools pause for approval, which changes what open runs do, so let runs that use a factory policy finish on the earlier version first, or keep them there with Worker Versioning.Squash with this PR's title so the release notes keep
Breaking:.Why?
#220, for #219, added
require_approvaltostateless_mcp_server()andstateful_mcp_server(), so every route now has a Workflow-side option. A policy left on the factory's server was still silently ignored on all three routes, includingtemporal_mcp_server(): a tool meant to wait for approval ran at once, with no error or warning. #220 left this open ("Should a policy or resolver on the factory's server be rejected, like a callabletool_filter?"). This change rejects the policy, so the mistake is visible.Checklist
Closes [Bug] openai_agents: a require_approval policy on the MCP factory's server is silently ignored #274
How was this tested:
test_mcp_v2.py: the backend factory must reject"always", a tool mapping, a tool list, a mapping and a tool list that mix"always"and"never", and a callable policy, and a server whose class, instance or__getattribute__replaces the evaluator, with a non-retryable error that namestemporal_mcp_server(require_approval=...);"never",False, a"never"mapping and a never-only tool list must still build the backend. The new factories use the module'sInProcessMCPServer.test_mcp_server_factory_require_approval_is_rejectedintest_openai.py, stateless and stateful: a Workflow whose provider's factory setsrequire_approval="always", or installs an evaluator on the server instance, must fail with the error, naming'HelloServer'and the route's option, within a 30-second execution timeout; the server must never be connected, cleaned up or called. The tracking server records acleanup()call before it raises, so putting back the base's unguardedcleanup()fails this test (checked).test_mcp_serverhas a newstateful-cleanup-errormode:cleanup()raises after a normal session, and the test checks the result, the call order, that the session is removed and that no heartbeat task is left. Every stateful mode, the approval one included, now checks that no heartbeat task is left.test_auto_heartbeater_interval_heartbeats_without_timeoutchecks thatauto_heartbeater(interval=...)heartbeats with no heartbeat timeout, as the session needs.make lintpasses (ruff, pyright, mypy, basedpyright, pydocstyle), and the whole suite passes in one process (436 passed). Parallelmake testruns on the loaded machine timed out in unrelated tests; CI is the full run.Any docs updates needed?
The README's MCP section and the docstrings listed above are updated here.
Not covered:
mainwas carried onto this change.use_structured_contentortool_meta_resolver, are still ignored; the README now says so. Tool guardrails there are skipped too ([Bug] openai_agents: tool guardrails on the MCP factory's server are silently skipped #303).temporalio1.34.x still bundles the old helpers astemporalio.contrib.openai_agents. No backport here.connect()raising, still reach the Workflow only through the dedicated Worker'sschedule_to_start_timeout, as before this change.tool_filteron the legacy routes is Breaking: reject a callable tool_filter on the deprecated OpenAI Agents MCP providers' servers #291, stacked on this PR.The check reads the SDK's private
_needs_approval_policyfield, since the SDK has no public way to read the policy. Review found the field in every release from 0.14.0 through 0.24.0, and the tests fail if it is renamed within the pinned range.