Repository navigation
fix(openai-agents): apply MCP tool filters on the plugin's MCP servers (breaking) - #209
Conversation
The agent reads `toolFilter` from the MCP server it holds. In a Workflow that is the plugin's reference, which had no filter, so a filter on the factory's server was ignored and blocked tools were offered and run. - The list-tools Activities of `StatelessMCPServerProvider` and `StatefulMCPServerProvider` apply the factory server's static filter. A callable filter needs the agent and the run context, so the Worker skips it and logs a warning. - `statelessMcpServer()` and `statefulMcpServer()` take a new `toolFilter` option that the agent applies in the Workflow, as for a native server.
DABH
left a comment
There was a problem hiding this comment.
Review at head 919bcfb (one commit over c664a34). Recipe: ten finder angles plus four independent finders (line scan, cross-file trace, Temporal semantics, tests and docs), 16 deduplicated candidates, one adversarial verifier per candidate, then a gap sweep. Verdicts: 12 confirmed, 1 plausible, 3 refuted and dropped (deduplicating the warning, subsumed below and not recommended; the Activity logger outside an Activity context, refuted on mechanism; a stub duplication with no consumer). Evidence: probes against the installed @openai/agents-core 0.14.3, several through the real Runner and two against the PR's compiled handles; one finding reproduced end to end with real Workers, a dev server and Worker.runReplayHistory in both directions; a mutation analysis of the filter helper; a trial three-way apply of draft #242 onto this head (five of six files conflict). On a worktree at the head with pnpm 10.27.0 on PATH: make lint clean (8 pre-existing warnings), make test 208 of 208, build clean. 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: the Worker-side static filter is right and matches upstream's rule on 400 randomized inputs. The problems are in how the Workflow-side option meets the agent SDK's caching and this plugin's own handle shapes: the handles default cacheToolsList to true while upstream's per-Workflow cache is keyed only by server name (or name plus agent name for a callable), so a second handle's filter or a context-dependent filter is evaluated once and then ignored; a callable filter left on the factory's server still fails open; a stateful handle shared by two agents now schedules an extra Activity per agent; and the callable filter is handed the suffixed stateful name and the plugin's cloned agent. The PR says 'Closes #207', but for callable filters the issue's expected behaviour is still unmet.
Findings on lines outside the diff
typescript/openai-agents/src/workflow/mcp-client.ts:33: P1: the handle'stoolFilteris applied only to the first tool listing per upstream cache key. The handles defaultcacheToolsListto true (native servers default to false), and the agent SDK's per-Workflow cache is keyed byserver.namefor a static or absent filter and by${server.name}:${agent.name}for a callable (mcp.js:200-205, 271-277), with the filter applied only on a miss (mcp.js:222-232). Two consequences, both probed through the realRunner. (a) Two handles for one provider with different static filters share one list: an Admin agent withstatelessMcpServer('files', { toolFilter: createMCPToolStaticFilter({ allowed: ['read', 'delete'] }) })and a Triage agent (handoff target,agentAsTool, or a laterrunner.run) withallowed: ['read']: whichever lists first wins, so admin-first offers Triage[read, delete]and a model call todeleteexecutes through the call-tool Activity with no call-time check; Triage'slistTools()is never called. (b) A context-dependent callable filter is evaluated once per agent per Workflow execution: a Workflow loopingrunner.run(agent, prompt, { context: { role } })withtoolFilter: async ({ runContext }, tool) => tool.name !== 'delete_file' || runContext.context.role === 'admin'offersdelete_fileto the second, non-admin run and executes it (probe: run two offered[read_file, delete_file], executeddelete_file; with the native defaultcacheToolsList: falsethe call fails withModelBehaviorError). The same happens inside one run when a tool or signal flips a context flag. Upstream's own guide uses arunContext-dependent filter as its example, the Python plugin re-applies callable filters per list, and the README's per-agent example only makes sense when agents share a server; the PR's test keys only onagent.name, which is part of the cache key, so it cannot see either case. Fix: keep the handle's raw Activity-result cache but reportcacheToolsList: falseupstream whenevertoolFilteris set (static too), so upstream re-filters the handle-cached list per turn at no Activity cost; the stateful handle then needs its own raw cache cleared byinvalidateToolsCache()(next item), which interacts with draft #242: if the handle owns the cache,invalidateToolsCache()must clear it behind the samepatched()gate #242 introduces, and #242'sinvalidateServerToolsCache(name)calls become dead for filtered handles, so whichever lands second must rework that method rather than just resolve conflicts. At minimum, document that same-name handles share one cached list per run and that context-dependent filters requirecacheToolsList: false, correct the stateful JSDoc ('for the agent's lifetime' is really per Workflow execution, per name), and add a test running the same agent twice with context admin then user. P2 if per-execution-varying context and same-name multi-handle setups are judged uncommon.typescript/openai-agents/src/workflow/stateful-mcp-client.ts:186: P2: with a callabletoolFilteron a stateful handle shared by more than one agent, upstream's per-agent cache key makes every additional agent schedule its own<name>-stateful-list-toolsActivity (the stateful handle has no handle-side cache, unlike the stateless closure cache), so the command stream differs from main and the body's claim that a filter which 'schedules nothing' keeps the same commands is false in both directions. Reproduced with real Workers, a dev server andWorker.runReplayHistory: onestatefulMcpServer('handoffFilter', { toolFilter: async () => true })shared by AgentA (handoffs to AgentB) and AgentB; the PR build with the option schedulesserver-session | list-tools | invokeModelActivity | list-tools | invokeModelActivity, while the PR build without the option and main schedule list-tools once. Replaying the PR history on main fails with[TMPRL1100] Activity type of scheduled event 'handoffFilter-stateful-list-tools' does not match activity type of activity command 'invokeModelActivity'at AgentB's first command; replaying main's history on the PR build fails the other way; controls pass; the stateless variant schedules once. Mechanism:defaultMCPToolCacheKeyreturns${server.name}:${agent.name}only for callable filters, each agent'sgetAllToolsmisses its own key and callslistTools(), and lines 184-187 schedule an Activity on every call withinvalidateToolsCache()a no-op. Fix: make the stateful Activity count depend on the handle's owncacheToolsList, either a handle-side cache likemcp-client.ts:47-55cleared ininvalidateToolsCache()(needs apatched()gate because directserver.listTools()calls on main schedule per call, as the plugin's ownstatefulMcpIsolationWorkflowdoes, and should land with #242'sMCP_INVALIDATE_TOOLS_CACHE_PATCH), or minimally cache only whentypeof toolFilter === 'function'(gate-free: no main history has a callable handle filter). Either way correct the body's 'Replay and rollback' bullets and add to the README, mirroring #210's adjacent section, that a callable filter runs once per tool each time the agent lists tools, that any Activity or timer it awaits is a command older versions never schedule, and that Workflows using the option should finish before a rollback.typescript/openai-agents/src/worker/mcp-provider.ts:56: P2: the static filter is enforced only at list time; the call-tool Activities (here andstateful-mcp-provider.ts:85) execute any tool name, so a run that listed tools before the Worker upgrade keeps the unfiltered list for the rest of the run under default caching and the new Worker still runs the blocked tool. A Workflow on main usingstatelessMcpServer('files')whose factory server hascreateMCPToolStaticFilter({ blocked: ['delete_file'] })completed its list-tools Activity before the upgrade, so History holds the unfiltered list; after the upgrade the open run replays,handle.listTools()resolves from the recorded result, both the closure (mcp-client.ts:53-55) and upstream cache it, no later turn re-lists, andinvalidateToolsCache()does not reopen it. The model is offereddelete_file, and the new Worker's Activity runss.callTool(input.toolName, input.args)without readings.toolFilter. Probe against upstream: a cache-enabled server listed once across twoRunner.runcalls offereddelete_fileevery turn and executed it twice; adding a static filter after the cache was warm changed nothing. Directserver.callTool()from Workflow code bypassing the filter is parity with native TypeScript, native Python and the Python plugin, so not a defect by itself. Fix: apply the factory server's static filter toinput.toolNamein both call-tool Activities (a shared helper next tofilterListedTools; skip callable filters) and throw a non-retryableApplicationFailurenaming the tool, which changes only Activity outcomes, never commands, and which upstream's default error function turns into a tool error for the model; and correct the body's 'applies from the next list-tools call' and README line 516's 'The agent never sees a blocked tool' (undercacheToolsList: truethe window lasts the rest of the run or until Continue-As-New). If call-time enforcement lands, file the sibling note for the Python plugin.typescript/openai-agents/src/workflow/runner.ts:285: P3: the Workflow-side filter also runs insideRunState.fromString, whichrun()awaits outside the try whose catch normalizes upstream errors into a non-retryableAgentsWorkflowError, so a filter that throws, or rejects a tool with a serialized call during a same-run resume, escapes as a plainUserErrororTypeErrorand fails the Workflow Task in a retry loop instead of failing the Workflow. This await precedes the try at 306-318;fromStringapplies the handle'stoolFilterper tool with no try/catch (runState.js:1043-1075, 2139-2148;mcp.js:228-237), and a serialized function entry whose tool is no longer offered throwsUserError('Tool <x> not found'). Probe: a run paused on an approval with a serializeddelete_filecall, a filter rejectingdelete_filethrowsUserErrorfromfromString; a filter reading a prototype method on the JSON-rehydratedrunContext.contextthrowsTypeError; with noworkflowFailureErrorTypesconfigured, such errors are recorded as Workflow Task failures. Scope is narrow: in the documented continue-as-new flow the user callsRunState.fromStringfirst, so the error surfaces in user code; the surviving trigger is a same-run resume withcacheToolsList: falseand a filter whose result differs on the rehydrated context. The placement is pre-existing; the PR adds user-authored filter code to that path. Move theif (input instanceof RunState)block (279-291) inside the try or wrap the await withnormalizeAgentsRunError, and add a README sentence that a callable filter also runs when aRunStateis rehydrated, must tolerate the rehydrated context, and must not reject a tool the serialized state already called.
PR body: it still says 'Draft while this question is open', but the PR is not marked draft; and 'Closes #207' holds only for static filters while the callable case stays open. Related: #210 (same duplicated-option pattern) and draft #242 (invalidateToolsCache), which conflicts with this PR in five of six files. One pre-existing observation outside this PR's scope, not independently verified: convertAgent's clone gets a fresh event emitter, so agent.on('agent_start', ...) listeners registered on the user's instance never fire.
| if (!filter) { | ||
| return tools; | ||
| } | ||
| if (typeof filter === 'function') { |
There was a problem hiding this comment.
P2: a callable toolFilter on the server the factory returns is skipped with only a Worker-side log line and the unfiltered list is returned, so the exact #207 symptom persists for every callable filter, while the Python plugin fails the same configuration fast. A factory returning new MCPServerStdio({ ..., toolFilter: async (_ctx, tool) => tool.name !== 'delete_file' }) with statelessMcpServer('files') and no handle option (the natural port of native code) hits typeof filter === 'function' here, logs, and returns every tool (line 20, used at 43; stateful at stateful-mcp-provider.ts:80-83); the handle's toolFilter is undefined, upstream applies nothing, delete_file is offered, and a model call runs through the call-tool Activity with no filter consulted (probe: no handle filter offered [read_file, delete_file] and ran callTool('delete_file'); the same callable as the handle option offered only read_file). The warning is Worker stderr only, nothing in History, and #207's own reproduction installs DefaultLogger('ERROR'), which hides it; no test exercises this path. Python's _reject_dynamic_tool_filter (_mcp_backend.py:152-167) raises a non-retryable ApplicationError inside the first MCP Activity. Recommended: fail closed with throw ApplicationFailure.create({ nonRetryable: true, type: 'MCPToolFilterUnsupported', message }) (ApplicationFailure.create from @temporalio/common needs no Activity context); an Activity failure is a History event, not a command change, so it is replay-safe and raises none of the rollback concerns in the body; add a test for the callable-on-factory path and state the failure in the README. If warn-only stays: say plainly in the README and in the log line that the tools it would have blocked are offered and will run, add that the Workflow-side option must be deterministic with I/O through Activities, and add a test asserting the warning; do not deduplicate it (it fires once per list-tools execution, proportional to the Activity rate the user configured, which is the SDK norm).
There was a problem hiding this comment.
Agreed, changed in 74e03c6. A callable toolFilter on the server the factory returns now fails every MCP Activity for that server with a non-retryable ApplicationFailure of type MCPToolFilterUnsupported: the stateless list and call Activities before they connect, and the stateful list and call Activities on the dedicated Worker. That's the same rule as the Python fix in #291. The README says so, mcp-provider.test.ts checks the stateless Activities, and the new integration test a callable toolFilter on the factory server fails the run checks both handles. The PR is now marked breaking, since a run with such a filter, even one that blocks nothing, now fails instead of completing.
| }; | ||
| this._factoryArgument = options?.factoryArgument; | ||
| this._cacheToolsList = options?.cacheToolsList ?? true; | ||
| this.toolFilter = options?.toolFilter; |
There was a problem hiding this comment.
P2: a callable toolFilter on a stateful handle receives serverName <name>-stateful while the stateless handle and the Python plugin pass the bare registered name; neither JSDoc block nor the README says so, so a filter keyed on serverName silently takes the wrong branch. statefulMcpServer('db', { toolFilter: async ({ serverName }, tool) => serverName === 'db' ? tool.name !== 'drop_table' : true }): upstream builds the context as { runContext, agent, serverName: server.name } (mcp.js:229) and the getter at 120-122 returns ${name}${MCP_STATEFUL_SUFFIX}, so the branch never matches and drop_table is offered and executed; the identical filter on statelessMcpServer('db') blocks it (probe through upstream getAllMcpTools with the compiled handles: db-stateful offered [query, drop_table], db offered [query]). Upstream documents serverName as the server providing the tools and ships it for shared multi-server filters; Python's ToolFilterContext.server_name is the bare registered name. The suffix already leaks into span data and includeServerInToolNames, but this is the first place the plugin hands it to user code as a discriminator, and the README has no occurrence of -stateful. Do not change the name getter (it keys upstream's cache, tool-name prefixes and trace data, and a bare name would collide the two handle kinds); wrap a callable filter so it receives the registered name, (ctx, tool) => userFilter({ ...ctx, serverName: registeredName }, tool), via a getter and setter so later assignment is covered; apply the same mapping to the toolMetaResolver context when #210 lands (mcp.js:562 has the identical shape); document it in both JSDoc blocks and the README; extend the test to assert the serverName seen by the filter for both handle kinds.
There was a problem hiding this comment.
Fixed in 74e03c6. The stateful handle now wraps a callable toolFilter so it gets the registered name (db), as the stateless handle does; name itself is unchanged. The test filter now checks serverName on both handles. I wrapped it in the constructor only, without a getter and setter: the option is the documented route, and #210 does the same for toolMetaResolver, so the two options behave alike.
| retryPolicy?: RetryPolicy; | ||
| factoryArgument?: unknown; | ||
| /** | ||
| * Tool filter the agent applies in the Workflow, as for a native MCP server. A callable filter |
There was a problem hiding this comment.
P2: the agent a Workflow callable filter receives is convertAgent's clone (agent.clone({ model: activityBackedModel })), not the caller's instance: identity and instanceof checks are always false and agent.model is an ActivityBackedModel object rather than the declared string, so such filters silently block, or in the inverse form allow, everything. runner.ts:274 converts the agent (convert-agent.ts:60; handoff targets at 63-69) and upstream passes the running agent to the filter; Agent.clone is new Agent({ ...this, ...config }). Probe with AdminAgent extends Agent through getAllMcpTools with the exact clone shape: clone === admin false, instanceof AdminAgent false, subclass fields undefined, typeof clone.model 'object'; filter outcomes original versus clone: identity 1 tool versus 0, instanceof 1 versus 0, agent.model === 'gpt-4o-mini' || ... 1 versus 0, agent.name === 'FileAdmin' 1 versus 1. The inverse form agent !== guestAgent || tool.name !== 'delete_file' is always true under the clone, so the restricted agent is offered delete_file. Natively upstream passes the user's own instance for non-sandbox agents, so identity works there and breaks only under the plugin; the README has no occurrence of 'clone' or ActivityBackedModel. Minimal fix: one sentence in both JSDoc blocks and the README that the agent is the plugin's converted copy (same name, instructions, tools, handoffs, mcpServers, outputType; not the caller's instance or subclass; model replaced), so key on agent.name or runContext.context. Sharper: record clone-to-original pairs in convertAgent (a WeakMap, or a getter on ActivityBackedModel, which already holds the original via setAgent at convert-agent.ts:58) and have the handles wrap a callable filter to present the original; the cache key reads agent.name, so caching is unaffected.
There was a problem hiding this comment.
| * A callable filter needs the agent and the run context, which only the Workflow has, so it is | ||
| * skipped with a warning. | ||
| */ | ||
| export function filterListedTools<T extends { name: string }>(server: MCPServer, tools: T[]): T[] { |
There was a problem hiding this comment.
P3 (tests): filterListedTools has no unit test and the integration test's model never calls a tool, so the allowed-list path, the allowed-plus-blocked precedence and the callable-filter warning branch are untested, and nothing pins that a blocked tool cannot be called, which is the command difference the rollback analysis rests on. Nothing under src/__tests__ references filterListedTools, allowedToolNames or blockedToolNames; the only exercise is test-openai-agents.ts:163-222 with a blocked-only filter and two text replies. Mutants against those inputs (precedence inverted, allowed-list prefix matching, blocked-list prefix matching, allowed list ignored) all still return ['safe_tool'] and pass. The callable branch at 14-20 has zero coverage: removing or rewording log.warn, switching to console.warn, or returning [] all stay green while README 514-518 promises the warning. Verified that the suggested tests work: MockActivityEnvironment (already used in three sibling test files) accepts a logger, and a probe running the compiled helper inside it with a capturing DefaultLogger captured the exact message and the pass-through; outside an Activity log.warn throws, which is why the branch needs that environment. Suggested: src/__tests__/mcp-provider.test.ts with a table over undefined, blocked-only, allowed-only, allowed-plus-blocked and prefix-not-name rows pinned to upstream's rule at mcp.js:242-256, one MockActivityEnvironment case for the callable branch, and optionally an integration run whose fake model emits toolCallResponse('danger_tool', {}) asserting the Tool danger_tool not found failure and, via fetchHistory, no call-tool ActivityTaskScheduled event. The Python sibling unit-tests both branches (tests/test_mcp_v2.py:217-271).
There was a problem hiding this comment.
Added in 74e03c6. src/__tests__/mcp-provider.test.ts checks filterListedTools with no filter, empty lists, an allowed list, a blocked list, both (blocked wins), and whole-name matching (read vs read_all). It also checks that the stateless list and call Activities refuse a callable filter without connecting, with a non-retryable MCPToolFilterUnsupported failure. The warning branch is gone, replaced by that refusal. I left out the optional Tool danger_tool not found integration case: that check is the agent SDK's own.
| * Tool filter the agent applies in the Workflow, as for a native MCP server. A callable filter | ||
| * gets the agent and the run context, and must be deterministic. | ||
| */ | ||
| toolFilter?: MCPToolFilterCallable | MCPToolFilterStatic; |
There was a problem hiding this comment.
P3 (design): the handle option surface grows one MCPServer option at a time, with the option and its JSDoc duplicated verbatim in both option interfaces (here and stateful-mcp-client.ts:49-53; #210 appends an identical toolMetaResolver block to both), while errorFunction, useStructuredContent and customDataExtractor on the factory's server are still silently dropped and only a callable toolFilter gets a Worker warning. Upstream reads all of them from the server object the agent holds, i.e. the handle, which forwards only cacheToolsList and now toolFilter; src/ has zero occurrences of the other names. Concretely, errorFunction: null on the factory's server (user intent: tool failures fail the run) is ignored with no warning and the run continues on a model-visible error string; useStructuredContent and customDataExtractor cannot be fixed by forwarding alone because upstream gates full results on server.callToolResult, which neither handle implements, and the call-tool Activities drop the non-enumerable result metadata at the payload boundary. Drift has already happened for cacheToolsList (documented in the stateful interface only). The Python plugin forwards **server_options into one handle class, so each option is declared once; the native classes copy all five fields with no defaults, so a Worker warning on them would never fire spuriously. Non-blocking suggestion, possibly in #210 or a follow-up to avoid a cross-PR rebase: declare the forwarded set once in src/common/mcp-types.ts and have both option interfaces extend it, generalize the Worker warning into a helper beside filterListedTools that names every factory-server option the Worker cannot honor, add a README sentence that these options are read from the handle, and open a tracking issue for the two that need callToolResult plumbing. Minimum if the PR stays strictly on #207: the generalized warning plus the issue, since the warning added here otherwise implies the other options are honored.
There was a problem hiding this comment.
Agreed on the duplication: both option interfaces now extend one McpServerHandleOptions in src/common/mcp-types.ts (74e03c6). I agreed it with #210, which adds toolMetaResolver to the same interface. errorFunction, useStructuredContent and customDataExtractor stay out of this PR: they're a separate gap, two of them need callToolResult plumbing, and the PR body lists them under "Not covered". The Worker now refuses the callable filter instead of warning, so nothing here suggests the other options are honored.
- A callable toolFilter on the server the factory returns now fails every MCP Activity for that server with a non-retryable MCPToolFilterUnsupported ApplicationFailure, instead of being skipped with a warning. - With a toolFilter set, both handles report cacheToolsList false to the agent SDK and cache the server's tool list themselves, so the agent filters every listing without scheduling more Activities. - A callable filter on a stateful handle gets the registered server name, not the internal `-stateful` one. - Both handle option interfaces extend one McpServerHandleOptions. - Unit tests for filterListedTools and the refusal; the integration test runs an agent with two contexts and a second agent on one handle.
Both handle option types extend McpServerHandleOptions, so the toolMetaResolver option and its doc comment are written once. #209 adds toolFilter to the same interface.
|
Thanks for the review. Out-of-diff findings, all in 74e03c6 unless noted:
|
…escript-mcp-tool-filter-ignored # Conflicts: # typescript/openai-agents/src/__tests__/test-openai-agents.ts
The Worker can't run a resolver on the server the factory returns, since it needs the run context. Instead of skipping it with a warning, every MCP Activity for that server now fails with a non-retryable ApplicationFailure of type MCPToolMetaResolverUnsupported, as #209 does for a callable toolFilter.
Both handle option types extend McpServerHandleOptions, so the toolMetaResolver option and its doc comment are written once. #209 adds toolFilter to the same interface.
The Worker can't run a resolver on the server the factory returns, since it needs the run context. Instead of skipping it with a warning, every MCP Activity for that server now fails with a non-retryable ApplicationFailure of type MCPToolMetaResolverUnsupported, as #209 does for a callable toolFilter.
DABH
left a comment
There was a problem hiding this comment.
Second round, at head 055c3ad (74e03c6 plus a merge of main at 1e8ae93). On a worktree at the head with pnpm 10.27.0 on PATH: make lint clean (8 pre-existing warnings), make test 247 of 247, including the four new tests; CI is green. Each round-one item was re-traced against the head and the installed @openai/agents-core 0.14.3 (mcp.js:222-277, tool.js:823-854, runState.js:2994-2999); the one new behavioural item was reproduced with a temporary integration test, since removed; trial merges were run against current main, #210 and #270. Refuted and dropped: the stateful handle's _cachedTools surviving cleanup() and a reconnect (on main the agent SDK's name-keyed cache does the same), and the extra mcp_tools span per turn for a filtered handle (what a native server with cacheToolsList: false does, and no commands). 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.
Previous rounds. Addressed: the P1 (with a filter set, both handles report cacheToolsList: false to the agent SDK and cache the raw list themselves, so the agent re-filters every listing and two same-name handles no longer share one filtered list; the new test runs an admin run, a non-admin run and a second agent on one handle and sees one list-tools Activity per handle; the stateful JSDoc no longer says 'for the agent's lifetime'); a callable filter on the factory's server now fails closed with a non-retryable MCPToolFilterUnsupported; the extra stateful list-tools Activity per agent is gone; the stateful filter gets the registered name; filterListedTools and the refusal have unit tests; the option is declared once in McpServerHandleOptions; the README states the rollback rule; the body no longer says draft, and 'Closes #207' now holds for callable filters too. Declines I accept: no call-time enforcement of the static filter (README and body say so), the plugin's agent copy reaching a callable filter until #270 lands (noted in the body), no getter and setter for a filter assigned to a stateful handle later (consistent with #210), the other three factory-server options left out of scope, and no 'Tool not found' integration case. Still open, with no reply: the RunState.fromString placement (below), which this round's design makes more reachable.
Where the problems cluster: the fix is right; what is left is how the refusal behaves for a run that is already open, one naming detail, and alignment with #210.
Approving: the round-one P1 and every P2 the author accepted are fixed and tested, CI is green, and only P3 items remain; the branch needs a rebase on main for an import-only conflict (below).
Findings on lines outside the diff
typescript/openai-agents/src/workflow/runner.ts:285: P3 (round one, still open):RunState.fromStringstill runs before the try at line 306, so a callable filter that throws, or that rejects a tool the serialized state already called, during the plugin's round trip fails the Workflow Task in a retry loop instead of failing the run. This round widens the window: a filtered handle now reportscacheToolsList: false, sodeserializeProcessedResponse(runState.js:2994-2999) always reaches the filter throughgetAllTools(the cache check atmcp.js:222no longer short-circuits), on the JSON-rehydrated context, on every same-run resume such as an approval. Under round one's design the cached entry usually answered. Same fix as before: move theif (input instanceof RunState)block inside the try, or wrap that await withnormalizeAgentsRunError, and add one README sentence that a callable filter also runs when aRunStateis resumed and sees the rehydrated context.
PR body: besides the call-tool point above, the 'Workaround-breaking' bullet reads as the only upgrade case for code on main that assigned toolFilter to a handle. Two more change the Activity count the same way: such a run that calls invalidateToolsCache() (on main the agent SDK's name-keyed cache still answered, so nothing was scheduled; now the handle re-lists, mcp-client.ts:39-41, stateful-mcp-client.ts:188-212), and a stateful run that calls listTools() directly more than once (main scheduled one Activity per call; now the handle caches). One clause covers both. Mergeability: GitHub reports a conflict with main. #215 rewrote the @openai/agents-core import block at the top of test-openai-agents.ts; nothing else conflicts. Related: #270 conflicts with this head only in the two test files. #210 conflicts in all nine shared files, as expected, and one design point needs a decision rather than a mechanical merge: #210 refuses toolMetaResolver in the stateful session Activity before the server connects (operations then raise that refusal after their schedule-to-start timeout), while this PR refuses a callable filter in each operation Activity after the session has connected the server. A factory server with both takes #210's path. Either choice is defensible, but the two refusals for one handle kind should behave the same way.
|
|
||
| A static `toolFilter` on the server your factory returns, such as `createMCPToolStaticFilter({ blocked: ['delete_file'] })`, applies in the list-tools Activity, so the model is not offered a blocked tool. The filter applies only to the tool list: a run that listed tools before the Worker was upgraded keeps that list while it is cached. | ||
|
|
||
| A callable filter gets the agent and the run context, which exist only in the Workflow, so the Worker refuses one on the factory's server: every MCP Activity for that server fails with a non-retryable `ApplicationFailure` of type `MCPToolFilterUnsupported`. Pass it as the `toolFilter` option of `statelessMcpServer()` or `statefulMcpServer()` instead: |
There was a problem hiding this comment.
P3: the refusal fails the run only when it hits the list-tools Activity; when it hits a call-tool Activity, which is the next MCP Activity for a run that listed tools before the upgrade, the agent SDK turns the failure into a tool result the model sees, and the run continues. Reproduced with a temporary integration test (removed): a StatelessMCPServerProvider whose factory returns a server with no filter for the first (list-tools) Activity and one with async () => true afterwards, mcpToolFilterWorkflow(false), and a model that calls safe_tool once. The Workflow completed with ['done', 'done', 'done']; History has one failed Activity (MCPToolFilterUnsupported, non-retryable); the server's callTool never ran; the model's next request carried the function_call_result 'An error occurred while running the tool. Please try again. Error: ActivityFailure: Activity task failed'. Mechanism: mcpToFunctionTool (mcp.js:575-625) uses the default errorFunction, and tool.js:823-854 resolves any thrown error to that string. The tool never runs, so the change fails closed as intended, but the body's 'A run still open whose factory server has a callable filter fails at its next MCP Activity for that server' is wrong for exactly that population: the run keeps going, the model is told to try again (each retry is another failed Activity, up to maxTurns), and the message it sees does not mention the filter. Suggested wording for the body and this paragraph: a new run fails at its first list-tools Activity; a run that listed tools before the upgrade gets a tool error for each call to that server and continues.
There was a problem hiding this comment.
Agreed. Wording changed in 4c1b325, no code change. The README now says that only list-tools and call-tool Activities refuse and that a new run fails at its first list-tools Activity. It also says that a statelessMcpServer() run that listed tools on an older Worker instead gets a tool error, which the model reads, for each call to that server, and keeps going, so the callable filter should come off the factory's server before Workers are upgraded. The PR body says the same. Failing such a run instead would need a rethrowing errorFunction on the handle, which would override the agent's mcpConfig.errorFunction (mcp.js:578-581), and that's more than this fix should change. #210 uses the same wording.
| `MCP server '${server.name}' has a callable toolFilter, which the Worker cannot run because the agent ` + | ||
| 'and run context it gets exist only in the Workflow. Pass it as the toolFilter option of ' + | ||
| 'statelessMcpServer() or statefulMcpServer() instead.', | ||
| type: 'MCPToolFilterUnsupported', |
There was a problem hiding this comment.
P3: the error type is a string literal, while DEDICATED_WORKER_FAILURE_TYPE is a named, exported constant (README line 512) and #210 names its sibling refusal TOOL_META_RESOLVER_UNSUPPORTED_TYPE in src/common/mcp-types.ts. A caller who wants to tell this refusal apart has to copy the string, as both new tests do (mcp-provider.test.ts:55, test-openai-agents.ts:302). Suggested: export const MCP_TOOL_FILTER_UNSUPPORTED_TYPE = 'MCPToolFilterUnsupported' in mcp-types.ts, re-exported from both entry points next to DEDICATED_WORKER_FAILURE_TYPE and used here, in the tests and in the README. Neither PR re-exports its constant from src/index.ts or src/workflow.ts today, so whichever of this PR and #210 lands second can export both.
There was a problem hiding this comment.
Done in 4c1b325. TOOL_FILTER_UNSUPPORTED_TYPE = 'MCPToolFilterUnsupported' is now in src/common/mcp-types.ts, next to DEDICATED_WORKER_FAILURE_TYPE, and exported with it from src/index.ts and src/workflow.ts. The provider, both tests and the README use it; the integration test imports it from the package. #210 does the same with TOOL_META_RESOLVER_UNSUPPORTED_TYPE, so whichever lands second keeps both names on those lines.
… does Move the stateful refusal back to getServerOrThrow(), where #209 refuses a callable toolFilter. The session connects and every operation fails at once, with no Workflow-side change, so drop the handle's session-refusal handling, the ordering replay fixture and its Workflow.
…escript-mcp-tool-filter-ignored # Conflicts: # typescript/openai-agents/src/__tests__/test-openai-agents.ts
…type - RunState.fromString lists tools, which runs MCP tool filters. Its errors now go through normalizeAgentsRunError, so a filter that throws while a run resumes fails the run with AgentsWorkflowError instead of retrying the Workflow Task. - TOOL_FILTER_UNSUPPORTED_TYPE names the refusal's error type, exported next to DEDICATED_WORKER_FAILURE_TYPE. - README: only list-tools and call-tool Activities refuse; a run that listed tools on an older Worker gets a tool error instead of failing; a filter assigned to a stateful handle later sees `<name>-stateful`; filters run on resume with the context rebuilt from JSON.
|
Thanks for the second pass. Round two is in 4c1b325, on top of a merge of
|
- The stateless handle's cacheToolsList has a setter again. 74e03c6 made it getter-only, so `server.cacheToolsList = false` threw in strict Workflow code. As on main, an assignment changes only what the agent SDK sees; with a filter set, the handle still reports false. - README: the agent SDK's invalidateServerToolsCache(name) does not refresh a handle with a filter, so also call the handle's invalidateToolsCache(); and runs that started on an older version with a context-dependent toolFilter property can fail replay after the upgrade.
|
One more commit, 0e0392a, from our own review of this round. The README also covers two more cases for code on |
What was changed
StatelessMCPServerProviderandStatefulMCPServerProvider: the list-tools Activities apply the statictoolFilter(allowedToolNames,blockedToolNames) of the server the factory returns, with the same rules the agent uses for a native server.toolFilteron that server is refused. Every list-tools and call-tool Activity for that server fails with a non-retryableApplicationFailureof typeMCPToolFilterUnsupported, exported asTOOL_FILTER_UNSUPPORTED_TYPE: the stateless list and call Activities before they connect, and the stateful list and call Activities on the dedicated Worker. The Python plugin uses the same rule (Breaking: reject a callable tool_filter on the deprecated OpenAI Agents MCP providers' servers #291).toolFilteroption onstatelessMcpServer()andstatefulMcpServer(), declared once inMcpServerHandleOptions. The handle exposes it as itstoolFilter, so the agent applies it in the Workflow, as for a native server. A callable filter gets the realAgent, the run context and the name the server was registered with (not the stateful handle's internal<name>-stateful), and must be deterministic.cacheToolsList: falseto the agent SDK and caches the server's tool list itself. The stateless handle already had that cache; the stateful handle gets one thatinvalidateToolsCache()clears. The agent SDK caches the filtered list by server name (and agent name, for a callable filter). Without this, a second handle with the same name, or a later run with another context, got the first filtered list. Now the agent filters every listing, with no extra Activity. AssigningcacheToolsListon the stateless handle still works as onmain; with a filter set, the handle still reportsfalse.RunState.fromString, which the runner calls when a run resumes, lists tools and so runs the filters. Its errors now go throughnormalizeAgentsRunError, so a filter that throws there fails the run withAgentsWorkflowErrorinstead of retrying the Workflow Task. Before, the agent SDK's cache usually answered there; with the change above the filter runs on every resume.FakeModelnow records the requests it gets, so a test can check which tools the model was offered.API: Caller-breaking. Before, a callable filter on the factory's server was skipped and the run completed with every tool offered. Now a new run fails at its first list-tools Activity, even when the filter blocks nothing. To upgrade, move the filter to the new
toolFilteroption, or use a static filter on the factory's server. Code with a static filter there is fixed without changes. The new option is additive.One difference from a native server: a handle's own
listTools()returns the statically filtered list, while a native server'slistTools()returns every tool and the agent filters later. The agent sees the same tools either way.Replay and rollback: there is no version gate. The static filter and the refusal run in Activities, so recorded results replay as before. The new handle option runs in Workflow code, and
mainignores it.Upgrade from
main: histories recorded onmainreplay on this change. AstatelessMcpServer()run still open whose factory server has a callable filter, and that listed tools on an older Worker, gets a tool error, which the model reads, for each call to that server, and the run keeps going. So remove the callable filter from the factory's server before you upgrade Workers. A run that listed tools before the upgrade keeps that list while the handle caches it (cacheToolsList, on by default), usually for the rest of the run, so the model can still be offered and call a tool the static filter now blocks. Runs started after the upgrade are filtered. Workaround-breaking: code onmainthat assigned atoolFilterproperty to a handle. A run of such code still open at the upgrade can fail replay:invalidateServerToolsCache(name)alone listed tools again onmain. This change keeps the handle's own cache, so it lists once, andmain's History fails replay with[TMPRL1100]. Migration: also call the handle'sinvalidateToolsCache(), which works onmaintoo. Run in the analysis.maina later run of the same agent got the first run's filtered list; this change filters each run. If that run called a tool the filter now removes,main's History fails replay (stateless with[TMPRL1100], stateful with an invalid Activity state transition). There is no migration; let such runs finish before upgrading. Run in the analysis.main, one per handle now); a run that called the handle'sinvalidateToolsCache()(nothing scheduled onmain, because the agent SDK's cache still answered; a new listing now); a stateful run that calledlistTools()directly more than once (one Activity per call onmain, one now).Rollback to
main, runs that don't use the new option: their histories replay onmain, including ones where the filter on the factory's server removed tools.Rollback to
main, runs that use the new option:mainignores the option, so such a history replays onmainonly if ignoring the option leaves the Workflow's commands unchanged. Anything the option does that adds, removes or reorders a command breaks replay onmain. Examples:Tool <name> not foundand schedules no call-tool Activity;mainschedules one.mainschedules it.sleep(), or an Activity for a permission check, adds a timer or Activity thatmainnever schedules. It runs on every listing, so it adds them every turn. Review reproduced this with a filter that awaitssleep(25)and a model that called no tool: both handles' runs completed, replay on this change, and fail onmainwith[TMPRL1100]at TimerStarted.listTools()directly more than once. With the option, this change caches the list;mainschedules a list-tools Activity per call.Runs whose callable filter only reads its arguments and schedules nothing, and where the option only hid tools the model never asked for, keep the same commands and replay on
main. Any other run that is still open fails its next Workflow Task with a non-determinism error as soon as amainWorker replays it, even with no further model call. Code that uses the option doesn't compile againstmain's types, so this comes up when the plugin is rolled back without that code. Taking the option out of the Workflow code doesn't help, because replay then behaves likemain. Before such a rollback, let runs that use the option finish, or plan to reset or terminate the ones that fail.Back to this change after a rollback: the same rule applies in reverse. A run that uses the option and is carried to
mainreplays on this change only if applying the option leaves the commandsmainrecorded unchanged. For example, if the model then calls a tool the option blocks, or a filter that schedules commands would have run, that run can't move back to this version: its replay fails with a non-determinism error.Why?
See #207. The agent reads
toolFilterfrom the server object it holds. In a Workflow that is the plugin's handle, which had no filter and took none, so a filter on the factory's server was ignored: the model was offered the blocked tools, and they ran.Checklist
Closes [Bug] openai-agents (TypeScript): an MCP server's toolFilter is ignored, so blocked tools run #207
How was this tested:
MCP tool filters apply on stateless and stateful serversinsrc/__tests__/test-openai-agents.ts. The factory's server blocksdanger_toolwith a static filter. The Workflow passes a callabletoolFilterthat checksserverNameand allowsextra_toolonly when the run's context is admin. It runs one agent with an admin context, then a non-admin context, then a second agent on the same handle, for each handle. Both handles offer[safe_tool, extra_tool],[safe_tool],[safe_tool], and each schedules one list-tools Activity. At the previous head (919bcfb) it fails: the non-admin stateless run was offeredextra_toolfrom the admin run's cached list, the stateful filter sawfilterStateful-statefuland blocked every tool, and the stateful handle scheduled two list-tools Activities. Withmain's provider and handle code it fails too: both handles offereddanger_tool.a callable toolFilter on the factory server fails the run(same file): both handles' runs fail withMCPToolFilterUnsupported.a tool filter that throws while resuming from a RunState fails the run(same file): the filter calls a method on a context object, which the JSON round trip inRunState.fromStringdrops. The run fails withAgentsWorkflowError. Without the runner change, its Workflow Task fails withTypeError: runContext.context.session.isAdmin is not a functionand is retried, and the run ends only at the test's 15-second execution timeout.src/__tests__/mcp-provider.test.ts:filterListedToolswith no filter, empty lists, allowed only, blocked only, both (blocked wins) and whole-name matching; the stateless list and call Activities refuse a callable filter without connecting; the stateless handle'scacheToolsListstays assignable.make lintpasses (8 existing warnings, none in changed files),make testpasses (251 tests),make buildpacks, andcheck_npm_package.py,smoke.mjsandcheck_conventions.pypass.mainatc664a34, and 1 with this change:plugin: offered ["safe_tool"], ran []. With a real stdio MCP server (MCPServerStdio,@modelcontextprotocol/sdk1.32.1), the History holds onlyops-stateless-list-toolsandinvokeModelActivity, and the blocked tool never runs.main's 6 histories (2 still open) replay onmainand on this change; this change's 6 replay on itself and onmain; the previous revision's (919bcfb) histories replay on this change, apart from the one carried run below. In these runs the option never changed a command: no rejected tool request, no throwing filter, and only filters that schedule nothing. Open runs carried across a Worker change complete with no failed Workflow Task, frommainto this change and back. A control with the MCP server renamed fails all 6 with[TMPRL1100]. Replaying a run that used the new option, was carried tomain, and called a tool the option blocks fails on this change, as described above.main: the stateless run fails with[TMPRL1100]at TimerStarted, and the stateful run with an invalid ActivityTaskCancelRequested transition. Open runs carried tomainfail the same way. Review also ran a callable filter that awaitssleep(25)and returns normally, with no tool call: both handles' histories replay on this change and fail onmainwith[TMPRL1100]at TimerStarted, while the same filter withoutsleepreplays on both. Successful runs whose filter schedules nothing replay on both versions.CI covers the other Node versions and platforms.
Any docs updates needed?
The README section, included here.
Related:
toolMetaResolverto the sameMcpServerHandleOptionsand a README section next to this one, so whichever merges second resolves a small conflict.invalidateToolsCache()on both handles; whichever lands second must keep clearing this PR's handle cache.agenta callable filter gets is the plugin's copy, so identity andinstanceofchecks fail. fix(openai-agents): run agents as written, support agent.asTool() and deprecate agentAsTool (breaking) #270 also changes theRunState.fromStringline inrunner.ts, so whichever merges second resolves a one-line conflict there.Not covered:
useStructuredContent,errorFunctionandcustomDataExtractoron the factory's server are still ignored.