Skip to content

fix(openai-agents): apply MCP tool filters on the plugin's MCP servers (breaking) - #209

Merged
campoy merged 6 commits into
mainfrom
fix/openai-agents-typescript-mcp-tool-filter-ignored
Oct 31, 2026
Merged

campoy merged 6 commits into
mainfrom
fix/openai-agents-typescript-mcp-tool-filter-ignored

Conversation

@campoy

@campoy campoy commented Oct 27, 2026 •

Copy link
Copy Markdown
Contributor

What was changed

  • StatelessMCPServerProvider and StatefulMCPServerProvider: the list-tools Activities apply the static toolFilter (allowedToolNames, blockedToolNames) of the server the factory returns, with the same rules the agent uses for a native server.
  • A callable toolFilter on that server is refused. Every list-tools and call-tool Activity for that server fails with a non-retryable ApplicationFailure of type MCPToolFilterUnsupported, exported as TOOL_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).
  • New toolFilter option on statelessMcpServer() and statefulMcpServer(), declared once in McpServerHandleOptions. The handle exposes it as its toolFilter, so the agent applies it in the Workflow, as for a native server. A callable filter gets the real Agent, the run context and the name the server was registered with (not the stateful handle's internal <name>-stateful), and must be deterministic.
  • With a filter set, a handle reports cacheToolsList: false to the agent SDK and caches the server's tool list itself. The stateless handle already had that cache; the stateful handle gets one that invalidateToolsCache() 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. Assigning cacheToolsList on the stateless handle still works as on main; with a filter set, the handle still reports false.
  • RunState.fromString, which the runner calls when a run resumes, lists tools and so runs the filters. Its errors now go through normalizeAgentsRunError, so a filter that throws there fails the run with AgentsWorkflowError instead of retrying the Workflow Task. Before, the agent SDK's cache usually answered there; with the change above the filter runs on every resume.
  • README: a short "MCP tool filters" section.
  • Tests: FakeModel now 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 toolFilter option, 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's listTools() 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 main ignores it.

  • Upgrade from main: histories recorded on main replay on this change. A statelessMcpServer() 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 on main that assigned a toolFilter property to a handle. A run of such code still open at the upgrade can fail replay:

    • A stateful handle refreshed with the agent SDK's invalidateServerToolsCache(name) alone listed tools again on main. This change keeps the handle's own cache, so it lists once, and main's History fails replay with [TMPRL1100]. Migration: also call the handle's invalidateToolsCache(), which works on main too. Run in the analysis.
    • A filter that depends on the run context. On main a 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.
    • From the code, not run: a stateful handle shared by two or more agents with a callable filter (one list-tools Activity per agent on main, one per handle now); a run that called the handle's invalidateToolsCache() (nothing scheduled on main, because the agent SDK's cache still answered; a new listing now); a stateful run that called listTools() directly more than once (one Activity per call on main, one now).
  • Rollback to main, runs that don't use the new option: their histories replay on main, including ones where the filter on the factory's server removed tools.

  • Rollback to main, runs that use the new option: main ignores the option, so such a history replays on main only if ignoring the option leaves the Workflow's commands unchanged. Anything the option does that adds, removes or reorders a command breaks replay on main. Examples:

    • The model asked for a tool the option rejects. This change fails that call with Tool <name> not found and schedules no call-tool Activity; main schedules one.
    • A callable filter threw. This change fails before the model Activity; main schedules it.
    • A callable filter scheduled commands itself, even when it returned normally: one that awaits sleep(), or an Activity for a permission check, adds a timer or Activity that main never schedules. It runs on every listing, so it adds them every turn. Review reproduced this with a filter that awaits sleep(25) and a model that called no tool: both handles' runs completed, replay on this change, and fail on main with [TMPRL1100] at TimerStarted.
    • Workflow code called a stateful handle's listTools() directly more than once. With the option, this change caches the list; main schedules 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 a main Worker replays it, even with no further model call. Code that uses the option doesn't compile against main'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 like main. 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 main replays on this change only if applying the option leaves the commands main recorded 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 toolFilter from 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

  1. Closes [Bug] openai-agents (TypeScript): an MCP server's toolFilter is ignored, so blocked tools run #207

  2. How was this tested:

    • MCP tool filters apply on stateless and stateful servers in src/__tests__/test-openai-agents.ts. The factory's server blocks danger_tool with a static filter. The Workflow passes a callable toolFilter that checks serverName and allows extra_tool only 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 offered extra_tool from the admin run's cached list, the stateful filter saw filterStateful-stateful and blocked every tool, and the stateful handle scheduled two list-tools Activities. With main's provider and handle code it fails too: both handles offered danger_tool.
    • a callable toolFilter on the factory server fails the run (same file): both handles' runs fail with MCPToolFilterUnsupported.
    • 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 in RunState.fromString drops. The run fails with AgentsWorkflowError. Without the runner change, its Workflow Task fails with TypeError: runContext.context.session.isAdmin is not a function and is retried, and the run ends only at the test's 15-second execution timeout.
    • src/__tests__/mcp-provider.test.ts: filterListedTools with 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's cacheToolsList stays assignable.
    • Locally, in Docker (Node v24.22.0, Linux arm64): make lint passes (8 existing warnings, none in changed files), make test passes (251 tests), make build packs, and check_npm_package.py, smoke.mjs and check_conventions.py pass.
    • [Bug] openai-agents (TypeScript): an MCP server's toolFilter is ignored, so blocked tools run #207's reproduction exits 0 (bug) on 1.24.0 and on main at c664a34, and 1 with this change: plugin: offered ["safe_tool"], ran []. With a real stdio MCP server (MCPServerStdio, @modelcontextprotocol/sdk 1.32.1), the History holds only ops-stateless-list-tools and invokeModelActivity, and the blocked tool never runs.
    • Replay check with a local dev server (Temporal CLI 1.9.1), stateless and stateful handles, with and without the new option, on a Worker with no Workflow cache: main's 6 histories (2 still open) replay on main and on this change; this change's 6 replay on itself and on main; 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, from main to 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 to main, and called a tool the option blocks fails on this change, as described above.
    • Review found the rollback limits described above. A Workflow catches the error from a tool the handle's option rejects, then sleeps. Its History replays on this change but not on main: the stateless run fails with [TMPRL1100] at TimerStarted, and the stateful run with an invalid ActivityTaskCancelRequested transition. Open runs carried to main fail the same way. Review also ran a callable filter that awaits sleep(25) and returns normally, with no tool call: both handles' histories replay on this change and fail on main with [TMPRL1100] at TimerStarted, while the same filter without sleep replays on both. Successful runs whose filter schedules nothing replay on both versions.

    CI covers the other Node versions and platforms.

  3. Any docs updates needed?
    The README section, included here.

Related:

Not covered:

  • useStructuredContent, errorFunction and customDataExtractor on the factory's server are still ignored.
  • A static filter applies only to tool listing, as for native servers. A call-tool Activity runs any tool name.

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 DABH left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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's toolFilter is applied only to the first tool listing per upstream cache key. The handles default cacheToolsList to true (native servers default to false), and the agent SDK's per-Workflow cache is keyed by server.name for 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 real Runner. (a) Two handles for one provider with different static filters share one list: an Admin agent with statelessMcpServer('files', { toolFilter: createMCPToolStaticFilter({ allowed: ['read', 'delete'] }) }) and a Triage agent (handoff target, agentAsTool, or a later runner.run) with allowed: ['read']: whichever lists first wins, so admin-first offers Triage [read, delete] and a model call to delete executes through the call-tool Activity with no call-time check; Triage's listTools() is never called. (b) A context-dependent callable filter is evaluated once per agent per Workflow execution: a Workflow looping runner.run(agent, prompt, { context: { role } }) with toolFilter: async ({ runContext }, tool) => tool.name !== 'delete_file' || runContext.context.role === 'admin' offers delete_file to the second, non-admin run and executes it (probe: run two offered [read_file, delete_file], executed delete_file; with the native default cacheToolsList: false the call fails with ModelBehaviorError). The same happens inside one run when a tool or signal flips a context flag. Upstream's own guide uses a runContext-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 on agent.name, which is part of the cache key, so it cannot see either case. Fix: keep the handle's raw Activity-result cache but report cacheToolsList: false upstream whenever toolFilter is 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 by invalidateToolsCache() (next item), which interacts with draft #242: if the handle owns the cache, invalidateToolsCache() must clear it behind the same patched() gate #242 introduces, and #242's invalidateServerToolsCache(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 require cacheToolsList: 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 callable toolFilter on 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-tools Activity (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 and Worker.runReplayHistory: one statefulMcpServer('handoffFilter', { toolFilter: async () => true }) shared by AgentA (handoffs to AgentB) and AgentB; the PR build with the option schedules server-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: defaultMCPToolCacheKey returns ${server.name}:${agent.name} only for callable filters, each agent's getAllTools misses its own key and calls listTools(), and lines 184-187 schedule an Activity on every call with invalidateToolsCache() a no-op. Fix: make the stateful Activity count depend on the handle's own cacheToolsList, either a handle-side cache like mcp-client.ts:47-55 cleared in invalidateToolsCache() (needs a patched() gate because direct server.listTools() calls on main schedule per call, as the plugin's own statefulMcpIsolationWorkflow does, and should land with #242's MCP_INVALIDATE_TOOLS_CACHE_PATCH), or minimally cache only when typeof 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 and stateful-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 using statelessMcpServer('files') whose factory server has createMCPToolStaticFilter({ 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, and invalidateToolsCache() does not reopen it. The model is offered delete_file, and the new Worker's Activity runs s.callTool(input.toolName, input.args) without reading s.toolFilter. Probe against upstream: a cache-enabled server listed once across two Runner.run calls offered delete_file every turn and executed it twice; adding a static filter after the cache was warm changed nothing. Direct server.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 to input.toolName in both call-tool Activities (a shared helper next to filterListedTools; skip callable filters) and throw a non-retryable ApplicationFailure naming 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' (under cacheToolsList: true the 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 inside RunState.fromString, which run() awaits outside the try whose catch normalizes upstream errors into a non-retryable AgentsWorkflowError, so a filter that throws, or rejects a tool with a serialized call during a same-run resume, escapes as a plain UserError or TypeError and fails the Workflow Task in a retry loop instead of failing the Workflow. This await precedes the try at 306-318; fromString applies the handle's toolFilter per 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 throws UserError('Tool <x> not found'). Probe: a run paused on an approval with a serialized delete_file call, a filter rejecting delete_file throws UserError from fromString; a filter reading a prototype method on the JSON-rehydrated runContext.context throws TypeError; with no workflowFailureErrorTypes configured, such errors are recorded as Workflow Task failures. Scope is narrow: in the documented continue-as-new flow the user calls RunState.fromString first, so the error surfaces in user code; the surviving trigger is a same-run resume with cacheToolsList: false and 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 the if (input instanceof RunState) block (279-291) inside the try or wrap the await with normalizeAgentsRunError, and add a README sentence that a callable filter also runs when a RunState is 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') {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

#270 stops cloning agents (convertAgent becomes checkAgent), so once it lands the filter gets the caller's own instance and identity, instanceof and agent.model checks work as they do natively. Neither is released yet, so I left the docs as they are rather than add a note that #270 would remove.

* 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[] {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

P3 (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).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

P3 (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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.
campoy added a commit that referenced this pull request Oct 30, 2026
Both handle option types extend McpServerHandleOptions, so the
toolMetaResolver option and its doc comment are written once. #209 adds
toolFilter to the same interface.
@campoy campoy changed the title fix(openai-agents): apply MCP tool filters on the plugin's MCP servers fix(openai-agents): apply MCP tool filters on the plugin's MCP servers (breaking) Oct 30, 2026
@campoy

campoy commented Oct 30, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the review. Out-of-diff findings, all in 74e03c6 unless noted:

  • P1, filter applied only to the first listing (mcp-client.ts:33): fixed as you suggested. With a toolFilter set, both handles report cacheToolsList: false to the agent SDK and keep their own cache of the raw Activity result: the stateless handle already had one, and the stateful handle gets one that invalidateToolsCache() clears. The agent now filters every listing with no extra Activity. The test Workflow runs one agent with an admin context, then a non-admin context, then a second agent on the same handle. It checks the offered tools ([safe_tool, extra_tool], [safe_tool], [safe_tool]) and that each handle schedules one list-tools Activity. The stateful cacheToolsList JSDoc now says "for the rest of the Workflow run". Draft fix(openai-agents): clear the agent SDK's MCP tool cache in invalidateToolsCache #242 also changes invalidateToolsCache(), so whichever lands second has to keep this cache clear.
  • P2, stateful handle shared by two agents (stateful-mcp-client.ts:186): fixed by the same change. The stateful handle caches only when a filter is set, so histories without the option schedule the same Activities as before. The PR body's replay bullets are updated, and the README now says a callable filter runs on every listing and that a timer or Activity it awaits adds commands older versions never schedule.
  • P2, static filter only at list time (mcp-provider.ts:56): not changed. Native servers and the Python plugin filter only the list, and I'd rather match them. The gap is runs that listed tools before the upgrade. The README no longer says "the agent never sees a blocked tool", and the PR body says such a run can still call a blocked tool while its list is cached.
  • P3, RunState.fromString outside the try (runner.ts:285): not changed here. That placement already existed, any error there escapes today, and fix(openai-agents): run agents as written, support agent.asTool() and deprecate agentAsTool (breaking) #270 keeps it. It fits a small follow-up better than this PR.
  • PR body: the draft line and the open question are gone. With callable filters refused, "Closes [Bug] openai-agents (TypeScript): an MCP server's toolFilter is ignored, so blocked tools run #207" now covers both cases.

…escript-mcp-tool-filter-ignored

# Conflicts:
#	typescript/openai-agents/src/__tests__/test-openai-agents.ts
campoy added a commit that referenced this pull request Oct 30, 2026
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.
campoy added a commit that referenced this pull request Oct 30, 2026
Both handle option types extend McpServerHandleOptions, so the
toolMetaResolver option and its doc comment are written once. #209 adds
toolFilter to the same interface.
campoy added a commit that referenced this pull request Oct 30, 2026
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 DABH left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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.fromString still 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 reports cacheToolsList: false, so deserializeProcessedResponse (runState.js:2994-2999) always reaches the filter through getAllTools (the cache check at mcp.js:222 no 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 the if (input instanceof RunState) block inside the try, or wrap that await with normalizeAgentsRunError, and add one README sentence that a callable filter also runs when a RunState is 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.

Comment thread typescript/openai-agents/README.md Outdated

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:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

P3: 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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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',

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

P3: 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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

campoy added a commit that referenced this pull request Oct 30, 2026
… 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.
@campoy

campoy commented Oct 31, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the second pass. Round two is in 4c1b325, on top of a merge of main (63815ec) for #215's import conflict.

  • RunState.fromString (runner.ts:285): fixed. You're right that this round's cache change makes it reachable on every resume. The fromString call's errors now go through normalizeAgentsRunError, so a filter that throws there fails the run with AgentsWorkflowError instead of retrying the Workflow Task. In the new test a tool filter that throws while resuming from a RunState fails the run, the filter calls a method on a context object, and the JSON round trip drops that method. Without the runner change, its Workflow Task fails with TypeError: runContext.context.session.isAdmin is not a function and is retried, and the run ends only at the test's 15-second execution timeout. The README now says a callable filter runs on resume and sees the context rebuilt from JSON. My round-one answer was in a PR comment rather than a thread, so it was easy to miss.
  • PR body: the Workaround-breaking bullet now also covers a run that calls invalidateToolsCache() and a stateful run that calls listTools() directly more than once, and the call-tool sentence is corrected.
  • fix(openai-agents): send MCP tool call _meta from the plugin's MCP servers (breaking) #210 alignment: both PRs now refuse in the stateful getServerOrThrow() (fix(openai-agents): send MCP tool call _meta from the plugin's MCP servers (breaking) #210 moved back there), use the same upgrade wording, and export their constants the same way.

- 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.
@campoy

campoy commented Oct 31, 2026

Copy link
Copy Markdown
Contributor Author

One more commit, 0e0392a, from our own review of this round. 74e03c6 made the stateless handle's cacheToolsList getter-only, so server.cacheToolsList = false threw in strict Workflow code. It has a setter again: as on main, an assignment changes only what the agent SDK sees, and with a filter set the handle still reports false. A unit test covers it.

The README also covers two more cases for code on main that assigned a toolFilter property to a handle. First, the agent SDK's invalidateServerToolsCache(name) alone no longer refreshes a filtered stateful handle; also call the handle's invalidateToolsCache(), which works on main too. Second, a run whose filter depends on the run context, started on main, can fail replay after the upgrade if it called a tool the filter now removes. The PR body lists both next to the cases you raised.

@campoy
campoy merged commit 4bcde9a into main Oct 31, 2026
27 checks passed
@campoy
campoy deleted the fix/openai-agents-typescript-mcp-tool-filter-ignored branch October 31, 2026 01:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] openai-agents (TypeScript): an MCP server's toolFilter is ignored, so blocked tools run

2 participants