Repository navigation
fix(openai-agents): send MCP tool call _meta and refuse a factory-side tool meta resolver (TypeScript and Python, breaking) - #210
Conversation
DABH
left a comment
DABH
left a comment
There was a problem hiding this comment.
Review at head 8dad56a (three commits over c664a34, seven files under typescript/openai-agents). Recipe: ten finder angles, 35 raw items deduplicated to 15 candidates, one adversarial verifier per candidate, then a gap sweep that found nothing new. Verdicts: 9 confirmed, 1 plausible, 5 refuted and dropped (the rollback paragraph is accurate, verified by replaying recorded histories on main's handle; the arity-preserving ternary mirrors upstream mcp.js:620-622 and is observable by arity-sensitive spies; a two-argument user wrapper behaves the same under the upstream Runner; the Activity logger only throws on the undocumented _getActivities() path; the non-generic MCPToolMetaResolver type matches upstream's own option types). In an isolated clone at the head: make sync, tsc and eslint clean (8 pre-existing warnings), 208 tests pass; make lint fails locally only because its nested pnpm resolves the global pnpm 9, not a PR issue. The fix itself is right and matches the Python plugin's design; I verified it against issue #208's reproduction and left my recommendation on the warn-versus-fail question there (keep the warning, fail fast for the whole family in a follow-up marked breaking).
Severity: P1 = wrong result or stuck run in realistic use; P2 = correctness or robustness in an edge case; P3 = docs, tests, cleanup.
Where the problems cluster: both real defects are about what the resolver is handed, not about forwarding: a nested agent's resolver sees an empty run context, and a stateful handle's resolver sees the internal suffixed server name. The rest is a mixed-fleet caveat the README omits, warning noise, and the call-tool input contract now spelled in four places.
Findings on lines outside the diff
typescript/openai-agents/src/workflow/agent-tools.ts:48: P2 (root cause predates the PR; this README section is the first documented feature that depends on it):agentAsToolstarts the nested run without the parent's run context, so a nested agent'stoolMetaResolverbuilds_metafrom an empty{}context and the README's own example silently sends a tenant-less request. Lines 48-52 callrunner.run(agent, parsed.input, sandbox === undefined ? undefined : { runConfig: { sandbox } })and ignore_runContext;runner.ts:297setscontext: options?.context(undefined) and upstreamrunContext.js:97-98defaults it to{}; upstreamAgent.asTool(agent.js:228-229, :292) passescontext: runContext, so native nested servers get the tenant. Reproduced at the head: an orchestrator run with{ context: { tenant: 'tenant-7' } }delegating viaagentAsTool(child), where the child's server uses the README resolver, made the Worker-sidecallToolreceive_meta{"contextType":"object","contextJson":"{}"}; with the one-line fix ({ context: _runContext, ... }) it receivedtenant-7. No error, warning or failed task either way. Fix it here or add the caveat to the new README section; #270 touches the same code.typescript/openai-agents/src/workflow/stateful-mcp-client.ts:123: P2: the new option hands user codeserverName<name>-statefulforstatefulMcpServer(name)but<name>forstatelessMcpServer(name), so the same resolver behaves differently per handle kind and can send an internal name to the MCP server. UpstreamresolveMcpToolMeta(mcp.js:562) buildsserverName: server.name; this getter returnsthis._name, which line 105 set to${name}-stateful, while the stateless handle (mcp-client.ts:41-43) returns the bare name.statefulMcpServer('tickets', { toolMetaResolver: ({ serverName }) => metaByServer[serverName] })withmetaByServer = { tickets: {...} }misses the lookup and sends no_meta; the identical resolver on the stateless handle works; a resolver echoingserverNamesendstickets-stateful. Reproduced at the head by addingserver: serverNameto the test resolver: recordedmetaStatelessversusmetaStateful-stateful. The suffix predates the PR (it already reaches thetoolFiltercontext and span data), but this option is what makes it reach the server; the new test cannot catch it because its resolver omitsserverNameand both expected entries are identical (test-openai-agents.ts:227-230). Fix without changingget name()(upstream's tool cache is keyed by it): wrap the option in the constructor,this.toolMetaResolver = r && ((ctx) => r({ ...ctx, serverName: name })), and assertserverNameper server in the test.typescript/openai-agents/src/__tests__/test-openai-agents.ts:134: P3 (tests): no test pins the compatibility claim that no-resolver calls still reach the server ascallTool(toolName, args)with nometakey in the Activity input. Changing the handles tometa: meta ?? {}and the Worker toserver.callTool(toolName, args, meta ?? {})at the head leaves all 208 tests green: this test asserts onlyfactoryArgument, the History-inspecting tests filterACTIVITY_TASK_SCHEDULEDby Activity type name only (95-100, 267-277, 561-567), and onlymetaTestServerrecordsmeta, on the with-resolver path. A 16-line addition here (rest-paramcallToolasserting two arguments, plus decoding thetestMcp-stateless-call-toolscheduled input and deep-equalling it to{ toolName: 'get_time', args: {}, factoryArgument: { tenantId: 'tenant-42' } }) fails under that edit and passes at head.
PR body: it ends 'Draft while this question is open', but the PR is not marked draft. Not verifiable here: the mixed-fleet scenario (code reading of main's handlers, no two-version run); the result-metadata item depends on users setting those options.
|
Thanks for the review. Pushed 55a1bb6 and 0309d28 (shared handle options, agreed with #209). On the findings outside the diff:
|
campoy
changed the title
|
One change since my last comment: I had already picked refusal for factory-side options the Worker can't run, and #209 now does that for a callable Every MCP Activity for a server whose factory returns a |
…rvers The agent builds each MCP tool call's `_meta` with the server's `toolMetaResolver` and passes it to `callTool`. The plugin's references dropped that argument, and the Worker-side server's resolver never ran, so MCP servers got no `_meta`. - `statelessMcpServer()` and `statefulMcpServer()` take a new `toolMetaResolver` option that the agent runs in the Workflow with the run context, as for a native server. - The references pass the `_meta` to the call-tool Activity, which passes it to the server's `callTool`. A `toolMetaResolver` on the factory's server needs the run context, so the Worker skips it and logs a warning.
- The stateful handle gives the toolMetaResolver option the name passed to statefulMcpServer(), not the internal suffixed one, as the stateless handle does. - The Worker warns about a toolMetaResolver on the factory's server only when the Workflow sent no _meta, and once per server. - The factoryArgument test checks that callTool gets no meta argument when there is no resolver. - README: older Workers drop _meta during an upgrade.
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.
c4c135b to
bfc90d6
Compare
DABH
left a comment
DABH
left a comment
There was a problem hiding this comment.
Re-review at head bfc90d6 (eight commits over f395ae7; the branch was rebased since my review at 8dad56a, and the new work is the stateful serverName fix, the shared McpServerHandleOptions, and the switch from a Worker warning to a non-retryable refusal of a factory-side toolMetaResolver). In a worktree at the head: make sync, make lint (0 errors, the 8 existing warnings) and make test (248 pass, including the four new MCP tests). Probes, each a temporary test removed afterwards: a stateless factory server with a resolver, run through an agent, fails the Workflow in about 100 ms with ActivityFailure (testMcp-stateless-list-tools) caused by a non-retryable MCPToolMetaResolverUnsupported, with no connect() and no failed Workflow Task, so a stuck run is ruled out; the same refusal from a call-tool Activity does not fail the run (first comment); adding one await Promise.resolve() to the stateful handle's callTool path makes the new replay test fail with [TMPRL1100] (orderingAfterMcp versus orderingAfterGate), so the recorded History guards what it claims; making the Worker call callTool(name, args, meta ?? {}) fails the factoryArgument arity assertion. Not verified: a real two-version fleet; the upgrade-window statements come from the probe that simulates it and from reading the code. The branch now conflicts with main in the import block of test-openai-agents.ts after #215 (db14ec3); CI is green on the current base and needs a rerun after the rebase.
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 stateful handle now gives the option serverName <name> (constructor wrap, asserted per server in the new test); the README states the mixed-fleet upgrade order (README:526); the factoryArgument test pins the two-argument call with no resolver; the option and its doc comment live once in McpServerHandleOptions; the stale draft line is gone from the body. Superseded: the per-call warning, replaced by the refusal. Accepted declines: agentAsTool dropping the run context is left to #270, which deletes it (until #270 lands, a nested agent's resolver still builds _meta from {}, so the two are best shipped in the same release); { signal } forwarding, a shared call-tool input type, the full-result Activity and a shared fake MCP server are follow-ups. On #208 I suggested keeping the warning and failing fast in a later breaking change; refusing here to match #209 is a reasonable call. The first two comments below are about where the refusal doesn't yet do what that decision promises. Nothing from earlier rounds is still open.
Where the problems cluster: the P2 and the first P3 are about how the refusal lands: from a tool call it turns into text for the model instead of failing the run, and on a stateful server it waits the schedule-to-start timeout, unlike #209's. The other two P3s are a missing export with stale doc comments, and the name fix not covering a resolver assigned on the handle.
Commenting rather than approving: new runs fail fast as intended, but the upgrade-window behavior of the refusal (P2) and its placement relative to #209 should be settled before this merges.
| factoryArgument?: unknown; | ||
| }) => { | ||
| const s = this.serverFactory(input.factoryArgument); | ||
| rejectToolMetaResolver(s); |
There was a problem hiding this comment.
P2: the refusal fails the run only when it comes from listing tools; from a tool call it becomes a tool output the model reads, so runs that cross the upgrade lose the tool without failing. Upstream runs every MCP tool call inside the function tool's error handling: the rejected callTool reaches invokeWithoutTimeout (tool.js:823-833 in agents-core 0.14.3), and because by default neither the handle nor the agent's mcpConfig sets an errorFunction, defaultToolErrorFunction (tool.js:559-561) turns the failure into text. Probe at the head: a stateless provider whose factory returns a server without a resolver for the list-tools Activity and with one for the call-tool Activity, which is what a run sees when it listed tools on an older Worker (a rolling upgrade without Worker Versioning, or a run open at the upgrade) and calls the tool on this version. The Workflow completes with 'final answer without the tool', History shows the call-tool Activity failed, the tool never ran, and the model's next input holds An error occurred while running the tool. Please try again. Error: ActivityFailure: Activity task failed. This needs the stateless handle's default cacheToolsList, so the run doesn't list again on the new Worker; a stateful run keeps the per-run Worker its session started on. So the body's "A run still open ... fails at its next MCP Activity for that server" holds only when that Activity is a listing; otherwise the failure shows up only as a failed Activity in History and the Worker log, and the model may answer without the data, where before this change the call succeeded without _meta. #209's call-tool refusal has the same path. The smallest fix is a sentence in the README section: remove a toolMetaResolver from the factory's server before upgrading Workers, because a run that listed tools on an older Worker gets a tool error instead of a failure from then on. Making the refusal fail the run would need an errorFunction on the handles that rethrows it (a throwing errorFunction propagates, tool.js:868-873), but a server's errorFunction takes precedence over the agent's mcpConfig.errorFunction (mcp.js:578-581), so that has its own cost.
There was a problem hiding this comment.
Thanks for the probe. I added the README sentence you suggested in 62599e6. A new run fails at its first list-tools Activity, but a stateless run that listed tools on an older Worker gets a tool error for each call to that server and keeps going, so remove the factory-side resolver before upgrading Workers. The body's upgrade line says the same now. I didn't add a rethrowing errorFunction on the handles, since it would take precedence over the agent's mcpConfig.errorFunction. #209 answers its call-tool refusal the same way.
|
|
||
| const server = this._serverFactory(input?.factoryArgument); | ||
| // Refuse before connecting, so the session fails at once instead of retrying the connection. | ||
| rejectToolMetaResolver(server); |
There was a problem hiding this comment.
P3: the stateful refusal doesn't behave like #209's, which is the reason given for refusing here instead of warning. This line refuses in the session Activity before connect(), so no dedicated Worker starts and an operation already scheduled waits its full schedule-to-start timeout (config.scheduleToStartTimeout, 30 s by default) before the handle rethrows the session failure. Through an agent that is the usual case, since the runner's first step after connect() is listTools(), scheduled in the same Workflow Task as the session; statefulMcpFactoryResolverWorkflow exercises the same timing directly with a 3 s timeout. That wait is also why stateful-mcp-client.ts needs _sessionRefusal, the extra handleWorkerFailure argument and the replay fixture. #209 at 055c3ad refuses a callable toolFilter in getServerOrThrow(), so its session connects and every operation fails at once with no Workflow-side change. Once both merge, a stateful server with a factory resolver stalls for the schedule-to-start timeout and fails from the session Activity, while one with a callable filter fails immediately from the operation Activity. Pick one placement before the second PR merges: if not connecting matters, move #209's check into the session as well; if fast failure matters, call rejectToolMetaResolver in getServerOrThrow() and drop the handle changes. Non-blocking.
There was a problem hiding this comment.
Moved it back to getServerOrThrow() in 602f1d1, where #209 refuses a callable toolFilter. The session connects and every operation fails at once, so I dropped _sessionRefusal, the extra handleWorkerFailure argument, the replay fixture and its Workflow. The stateful integration test now expects listTools() and callTool() to get the refusal and cleanup() to succeed.
| export const DEDICATED_WORKER_FAILURE_TYPE = 'DedicatedWorkerFailure'; | ||
|
|
||
| /** Error type for a `toolMetaResolver` on the server the factory returns, which the Worker refuses. */ | ||
| export const TOOL_META_RESOLVER_UNSUPPORTED_TYPE = 'MCPToolMetaResolverUnsupported'; |
There was a problem hiding this comment.
P3: TOOL_META_RESOLVER_UNSUPPORTED_TYPE isn't exported, unlike DEDICATED_WORKER_FAILURE_TYPE above it, so Workflow code that handles the refusal must hard-code the string. src/index.ts:15 and src/workflow.ts:15 export only DEDICATED_WORKER_FAILURE_TYPE, and the README describes that one as "exported as". The doc comments on statefulMcpServer() (stateful-mcp-client.ts:250-251) and StatefulMCPServerProvider (stateful-mcp-provider.ts:31-32) still say a dedicated Worker that never starts makes operations throw DedicatedWorkerFailure, which is no longer the whole story. Export the constant from both entry points and add a sentence to those two comments. Non-blocking.
There was a problem hiding this comment.
Exported from both entry points next to DEDICATED_WORKER_FAILURE_TYPE in aa51e5f. The README says "exported as" and lists it with the other exports, and the unit test now imports it from the package. With the refusal back in the operation Activities, the dedicated Worker starts as before, so the two doc comments are still right and I left them. #209 exports TOOL_FILTER_UNSUPPORTED_TYPE the same way.
| this._cacheToolsList = options?.cacheToolsList ?? true; | ||
| // Give the resolver the name the caller passed, as statelessMcpServer() does, not the suffixed one. | ||
| const toolMetaResolver = options?.toolMetaResolver; | ||
| this.toolMetaResolver = toolMetaResolver && ((context) => toolMetaResolver({ ...context, serverName: name })); |
There was a problem hiding this comment.
P3: the name fix covers only the option; a resolver assigned on the stateful handle still gets serverName <name>-stateful, and that is the path the README presents as rollback-safe. toolMetaResolver is a public field (line 115) and this wrap runs once in the constructor, so const s = statefulMcpServer('tickets'); s.toolMetaResolver = r; replaces it. Upstream builds the context from server.name (mcp.js:562), which here is ${name}-stateful (lines 118 and 137-139), while the same assignment on statelessMcpServer('tickets') gives tickets. README:528 names server.toolMetaResolver = ... next to the option without the caveat. Either wrap on assignment with an accessor pair that applies the same { ...context, serverName: name } override, or add one sentence to the README saying that a resolver set directly on a stateful handle sees the suffixed name. Non-blocking.
…escript-mcp-tool-meta-dropped # Conflicts: # typescript/openai-agents/src/__tests__/test-openai-agents.ts
… 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.
Export the refusal's failure type from both entry points, next to DEDICATED_WORKER_FAILURE_TYPE, so Workflow code that handles it doesn't hard-code the string.
…'s name A run that listed tools on an older Worker gets the call-tool refusal as a tool error and keeps going, so say to remove a factory-side toolMetaResolver before upgrading Workers. Also say that a resolver set on a stateful handle gets the internal `<name>-stateful` name.
…escript-mcp-tool-meta-dropped # Conflicts: # typescript/openai-agents/README.md # typescript/openai-agents/src/__tests__/mcp-provider.test.ts # typescript/openai-agents/src/__tests__/test-openai-agents.ts # typescript/openai-agents/src/__tests__/workflows/openai-agents.ts # typescript/openai-agents/src/common/mcp-types.ts # typescript/openai-agents/src/index.ts # typescript/openai-agents/src/worker/mcp-provider.ts # typescript/openai-agents/src/worker/stateful-mcp-provider.ts # typescript/openai-agents/src/workflow.ts # typescript/openai-agents/src/workflow/mcp-client.ts # typescript/openai-agents/src/workflow/stateful-mcp-client.ts
DABH
left a comment
DABH
left a comment
There was a problem hiding this comment.
Third round, at head 0af5470 (my last review was at bfc90d6; since then 602f1d1, aa51e5f and 62599e6, plus two merges of main, the second of which brings in #209). In a worktree at the head with pnpm 10.27.0: make lint (0 errors, the 8 existing warnings) and make test (254 pass). The body's replay check and #208 reproduction were run at 964bc6b, the original warn-only commit, so I re-ran replay at this head with a probe Workflow: a stateless or stateful handle with no resolver, a resolver assigned on the handle, the new option, or a resolver only on the factory's server, and one tool call, two parallel calls to one server, or parallel calls to two servers, recorded on origin/main 88745dc and on the head (18 histories each; the option only on the head, the factory's resolver only on main). All 18 main histories replay on the head, so upgrading the plugin is replay-safe. The head's histories replay on main except the option with calls to two servers, which fails with [TMPRL1100], as the README's rollback paragraph says. A resolver on the factory's server now fails a new run at its first list-tools Activity for both handle kinds (MCPToolMetaResolverUnsupported, no failed Workflow Task), and deleting the check from getServerOrThrow() makes the new stateful test fail with 'unexpected-success' twice. Refuted: a replay break on upgrade (above), and a lost option or README section in the #209 merge (both options share McpServerHandleOptions, and both sections are intact). Not verified: a live two-version fleet; the README's mixed-fleet sentence rests on main's handlers, which ignore the meta key.
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 README and the body now say that a stateless run that listed tools on an older Worker gets a tool error from the refusal instead of failing, and that the factory's resolver should be removed before upgrading (the smallest fix I suggested; declining a rethrowing errorFunction because it would override mcpConfig.errorFunction is reasonable). The stateful refusal moved to getServerOrThrow(), where #209 refuses a callable filter, so the session connects, both operations fail at once, and _sessionRefusal, the extra handleWorkerFailure argument and the replay fixture are gone. TOOL_META_RESOLVER_UNSUPPORTED_TYPE is exported from both entry points, and the two doc comments are right again now that the dedicated Worker starts. The README says a resolver assigned on a stateful handle sees <name>-stateful (the probe records rv5sf-stateful for an assigned resolver and rv5sf for the option). The round-one fixes still hold. Accepted declines stand: { signal }, the shared call-tool input type, the full-result Activity and a shared fake server are follow-ups, and agentAsTool dropping the run context is left to #270, which is still open, so the two should ship in the same release. Every comment has a reply, and nothing from earlier rounds is still open.
Where the problems cluster: the code is right and the upgrade replays cleanly; all three items are documentation. The one that matters: this PR now requires everyone with a resolver on the factory's server to move it into Workflow code, and that move is itself a Workflow change that open runs can fail to replay.
Commenting rather than approving: nothing in the code blocks, but the README should say that adding a resolver to a Workflow with open runs needs a patched() guard or those runs to finish first (the P2 below), and that one sentence is all I'd want before approving.
Parity: the Python plugin forwards _meta through its call-tool Activity the same way, but on origin/main it accepts a tool_meta_resolver on the factory's server and silently ignores it: the v2 backend's factory check rejects only a callable tool_filter and an approval policy (_mcp_backend.py:163-164), the deprecated providers check only the approval policy, and the Python SDK reads the resolver only on the agent's side. After this PR, TypeScript fails a run that Python completes without _meta. A Python follow-up in the #275 pattern would line the two up; not a blocker here.
PR body and title: the title becomes the release-note line and names only the fix; naming the break would help readers of the notes, for example '... and refuse a toolMetaResolver on the factory's server (breaking)'. The #208 reproduction also gives a different result at this head than the body states: its factory server always sets toolMetaResolver, so both of its cases now hit the refusal and the script exits 2 at its first execute, not 1. To show the fix, it needs the resolver removed from the factory's server.
| }); | ||
| ``` | ||
|
|
||
| A new run with a `toolMetaResolver` on the factory's server fails at its first list-tools Activity. 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 the run keeps going. So remove the `toolMetaResolver` from the factory's server before you upgrade Workers. |
There was a problem hiding this comment.
P2: the migration the refusal points to is itself a Workflow code change that can fail replay of open runs, and the README describes the command change only as a rollback concern. The refusal's message, line 534 and the body's API paragraph all tell users with a resolver on the factory's server to pass it as the toolMetaResolver option. A run that is open across that deploy then replays a History recorded without a resolver. Because the agent awaits the resolver, a turn that called a tool on this server and a tool on another server in parallel now schedules the other server's Activity first. Reproduced at this head: a History recorded on origin/main 88745dc with statelessMcpServer('rv5sl') (resolver only on the factory's server, or none) plus statelessMcpServer('rv5plain'), where one model turn calls echo_a and plain_c in parallel. Replayed on the head with the Workflow changed only to pass a pure synchronous { toolMetaResolver }, it fails with [TMPRL1100] Nondeterminism error: Activity type of scheduled event 'rv5sl-stateless-call-tool' does not match activity type of activity command 'rv5plain-stateless-call-tool'. The stateful handle fails the same way with rv5sf-stateful-call-tool, and a resolver assigned on the handle reorders identically (main itself records rv5plain first once one is assigned). One call per turn and parallel calls to one server replay fine, and the same main histories replay on the head when the Workflow adds no resolver. So the plugin upgrade is safe; the break is the code change. That change is easy to judge safe, because it looks like it only adds _meta to Activity inputs, which replay does not compare. On a live Worker, each affected run then fails its Workflow Tasks until the change is reverted or patched. A resolver that changes the run context can break open runs the same way. Fix: one sentence after this paragraph and in the body's upgrade line, for example: 'Adding a resolver to a Workflow that has open runs, including moving one off the factory's server, can fail their replay for the reasons given below for a rollback, so guard it with patched() or let those runs finish first.'
There was a problem hiding this comment.
Agreed, and thanks for the reproduction. In e100cfa I added a paragraph after the rollback one: adding a resolver to a Workflow that has open runs, including moving one off the factory's server, can fail their replay for the same reasons, so guard the change with patched() or let those runs finish first. The body's API line says the same now.
| | `@temporalio/openai-agents/workflow-interceptor` | Worker bundling | Manual `workflowInterceptorModules` wiring without a plugin | | ||
|
|
||
| `@temporalio/openai-agents` exports `OpenAIAgentsPlugin`, MCP provider classes, `SandboxClientProvider`, model option types, hosted tool credential types, and `DEDICATED_WORKER_FAILURE_TYPE`. `OpenAIAgentsTraceClientInterceptor` is also public for Clients that need manual interceptor wiring instead of plugin registration. | ||
| `@temporalio/openai-agents` exports `OpenAIAgentsPlugin`, MCP provider classes, `SandboxClientProvider`, model option types, hosted tool credential types, `DEDICATED_WORKER_FAILURE_TYPE` and `TOOL_META_RESOLVER_UNSUPPORTED_TYPE`. `OpenAIAgentsTraceClientInterceptor` is also public for Clients that need manual interceptor wiring instead of plugin registration. |
There was a problem hiding this comment.
P3: the export lists name TOOL_META_RESOLVER_UNSUPPORTED_TYPE but not TOOL_FILTER_UNSUPPORTED_TYPE, which src/index.ts:15 and src/workflow.ts:15 export on the same line. The gap dates from #209, but this PR rewrites both sentences (here and line 747), and the tool-filter section already says 'exported as TOOL_FILTER_UNSUPPORTED_TYPE'. Add it to both lists. Non-blocking.
There was a problem hiding this comment.
Added TOOL_FILTER_UNSUPPORTED_TYPE to both lists in e100cfa.
| * Builds each tool call's MCP `_meta` in the Workflow, as for a native MCP server. It gets the | ||
| * run context and must be deterministic. Older plugin versions ignore this option and may not | ||
| * replay a History in which it ran, because running it can change the Workflow's commands or | ||
| * their order. |
There was a problem hiding this comment.
P3: this doc comment, which is what callers see on hover, doesn't say that the resolver's result is recorded in Workflow History. The README says so (line 544), and it is the caveat with a security cost: the values sit in the call-tool Activity input, visible in the Web UI without a payload codec. Add a sentence such as 'Its result is recorded in Workflow History as part of the call-tool Activity input, so it must not hold secrets.' Non-blocking.
There was a problem hiding this comment.
Added in e100cfa: "Its result is recorded in Workflow History as part of the call-tool Activity input, so it must not hold secrets."
- README: adding a resolver to a Workflow with open runs, including moving one off the factory's server, can fail their replay, so guard it with patched() or let those runs finish. - README: both export lists name TOOL_FILTER_UNSUPPORTED_TYPE too. - The option's doc comment says its result is recorded in History, so it must not hold secrets.
campoy
changed the title
|
Thanks for the third round. e100cfa covers the three threads; replies are in each.
|
…escript-mcp-tool-meta-dropped
The same decision covers both languages: the Python plugin silently ignored a tool_meta_resolver on the server a factory returns, so the server got no _meta. temporal_mcp_server, StatelessMCPServerProvider and StatefulMCPServerProvider now refuse one with a non-retryable error, as they do a callable tool_filter and a require_approval policy, and the stateful session never connects such a server. The README and docstrings say so.
campoy
changed the title
|
Following up on the Python parity note: I've added it here rather than in a separate PR. After merging |
What was changed
statelessMcpServer()andstatefulMcpServer()take themetaargument ofcallTooland put it in the call-tool Activity input, only when there is one, so the input of other calls doesn't change. The call-tool Activities pass it to the server'scallTool(toolName, args, meta), as the agent does for a native server.toolMetaResolveroption onstatelessMcpServer()andstatefulMcpServer(). The handle exposes it as itstoolMetaResolver, so the agent runs it in the Workflow with the run context, as for a native server. It must be deterministic. On the stateful handle it gets the name passed tostatefulMcpServer()asserverName, as on the stateless one, not the internal-statefulname.toolMetaResolveron the factory's server needs the run context, which only the Workflow has, so the Worker now refuses it with a non-retryableApplicationFailureof typeMCPToolMetaResolverUnsupported, exported asTOOL_META_RESOLVER_UNSUPPORTED_TYPE. Every list-tools and call-tool Activity for that server fails that way: the stateless ones before connecting, and the stateful ones ingetServerOrThrow(), where fix(openai-agents): apply MCP tool filters on the plugin's MCP servers (breaking) #209 refuses a callabletoolFilter. The handles add no promise steps.patched()or those runs to finish, that a resolver set on a stateful handle gets<name>-statefulasserverName, that the_metavalues are recorded in History, so they must not hold secrets, that a Worker with an older plugin version drops_meta(upgrade every Worker on the Task Queue first, or use Worker Versioning), and that Workflows using the new option should finish before Workers roll back to an older plugin version.temporal_mcp_server,StatelessMCPServerProviderandStatefulMCPServerProvidernow refuse atool_meta_resolveron the factory's server with a non-retryableApplicationError, as they refuse a callabletool_filterand arequire_approvalpolicy since Breaking: reject a require_approval policy on the OpenAI Agents MCP factory's server #275. Before, it was silently ignored and the server got no_meta. The stateful session never connects such a server. The Python README and docstrings say so. A resolver passed totemporal_mcp_server(tool_meta_resolver=...)already worked.API: Caller-breaking. Before, a
toolMetaResolveron the factory's server was skipped and the run completed with no_meta. Now a new run fails at its first list-tools Activity for that server, even when the resolver returns nothing. To upgrade, move the resolver to the newtoolMetaResolveroption. That move is a Workflow change that can reorder parallel tool calls, so guard it withpatched()or let open runs finish first. The new option is additive, and a resolver set on the handle in Workflow code (server.toolMetaResolver = ...) now works too. Python is Caller-breaking the same way: a run whose factory's server setstool_meta_resolverused to complete without_metaand now fails at its first MCP Activity for that server; move the resolver totemporal_mcp_server(tool_meta_resolver=...).Replay and rollback: Activity inputs are not compared on replay. The Python refusal runs only in Activities, so recorded results replay as before. The handles add no promise steps to
listTools()orcallTool(), so concurrent work keeps its command order. Histories recorded onmainreplay on this change. A run still open whose factory server has atoolMetaResolverfails at its next list-tools Activity for that server. A stateless run that already listed tools on an older Worker instead gets a tool error, which the model reads, for each call to that server, and keeps going; the README says to remove the resolver from the factory's server before upgrading Workers. Rollback tomain(there is no version gate) is safe only for some Workflows, becausemainignores the new option and doesn't run its resolver. An older version replays a History only if it produces the same commands in the same order:main, and the handles take the same promise steps, so the commands don't change. After a rollback, calls have no_metaagain.mainfails these histories with[TMPRL1100]. Let Workflows that use the option finish before rolling Workers back. The README and the option's doc comment state this.Why?
See #208. The agent builds each call's
_metawith the server'stoolMetaResolverand passes it tocallTool. The plugin's handles dropped that argument, and a resolver on the factory's server never ran, so MCP servers got no_meta. The Python plugin silently ignored atool_meta_resolveron the factory's server the same way.Checklist
Closes [Bug] openai-agents (TypeScript): MCP tool calls get no _meta from a toolMetaResolver #208
How was this tested:
MCP toolMetaResolver sends _meta on stateless and stateful serversinsrc/__tests__/test-openai-agents.ts. The Workflow passes a resolver built from the run context and the tool name; the server records the_metait gets. Withmain's provider and handle code (keeping the new option types so the test compiles), it fails at itst.deepEqual: the server gotundefinedtwice. With this change it gets{ tenant: 'tenant-7', tool: 'echo_meta', server: <name> }from both the stateless and the stateful handle, with the name each was created with. Without the stateful handle's name fix, the stateful call getsserver: 'metaStateful-stateful'.factoryArgument is passed through to MCP activitiesnow checks thatcallToolgets two arguments when there is no resolver. It fails if the handle sendsmeta: {}or the Worker passesmeta ?? {}.StatelessMCPServerProvider refuses a toolMetaResolver without connectinginsrc/__tests__/mcp-provider.test.ts(fix(openai-agents): apply MCP tool filters on the plugin's MCP servers (breaking) #209 adds tests to the same file): the server'sconnect()rejects; both Activities fail with a non-retryableTOOL_META_RESOLVER_UNSUPPORTED_TYPE, andconnect()is never called.Stateful MCP: a factory toolMetaResolver fails the operationsinsrc/__tests__/test-openai-agents.ts: the Workflow'slistTools()andcallTool()each get the non-retryable refusal, andcleanup()succeeds. Without the check ingetServerOrThrow(), both operations succeed and the test fails.make lintpasses (8 existing warnings, none in changed files),make testpasses (248 tests),make buildpacks, andcheck_npm_package.py,smoke.mjsandcheck_conventions.pypass.test_worker_side_tool_meta_resolver_is_rejectedintests/test_mcp_v2.py, and atool_meta_resolvercase in the existingtest_mcp_server_factory_settings_are_rejected(stateless and stateful) intests/test_openai.py. Each fails with its refusal call removed. In Docker,make lintpasses andmake testpasses (450 tests, run with-n 4; with-n autoon a loaded machine, timing-sensitive tracing tests failed on both this branch andmain).62599e6, exits 0 (bug) on 1.24.0 and onmainatdb14ec3. With this change it exits 2: its factory server always setstoolMetaResolver, so its first run fails withMCPToolMetaResolverUnsupported. Showing the fix with it needs the resolver removed from the factory's server; the tests above cover the Workflow-side option.964bc6b, rerun at62599e6againstmainatdb14ec3with the same results) with a local dev server (Temporal CLI 1.9.1), stateless and stateful handles, with no resolver, a resolver set on the handle, and the new option with a synchronous resolver on a single MCP call, on a Worker with no Workflow cache:main's 6 histories (2 still open) replay onmainand on this change; this change's 8 replay on itself and onmain. Open runs carried across a Worker change complete with no failed Workflow Task, frommainto this change and back, and the carried histories replay on both. A control with the MCP server renamed fails all 6 with[TMPRL1100]. Review found option-resolver histories that replay on this change but fail onmainwith[TMPRL1100]: parallel MCP calls with a pure synchronous resolver, resolvers that change the run context, and resolvers that await an Activity or reject. That is why the rollback guidance above doesn't cover the option.CI covers the other Node versions and platforms.
Any docs updates needed?
The README section, included here.
Related: #209 (merged) fixed
toolFilterthe same way; this PR mergedmainafter it, keeping both changes in the sharedMcpServerHandleOptionsinterface, the refusals and the README.A resolver on the factory's server is refused, the same policy #209 uses for a callable
toolFilter.