diff --git a/.changeset/tool-call-definition-snapshot.md b/.changeset/tool-call-definition-snapshot.md new file mode 100644 index 0000000000..fe683fc399 --- /dev/null +++ b/.changeset/tool-call-definition-snapshot.md @@ -0,0 +1,5 @@ +--- +'@modelcontextprotocol/client': patch +--- + +Keep tool-call header mirroring and output validation on the same cached tool definition when a catalog refresh overlaps a call. diff --git a/packages/client/src/client/client.ts b/packages/client/src/client/client.ts index 8118b60cea..75b0cb1c73 100644 --- a/packages/client/src/client/client.ts +++ b/packages/client/src/client/client.ts @@ -536,14 +536,13 @@ export class Client extends Protocol { /** * The response-cache substrate. Owns the backing store, the per-method * eviction-generation counter, the user-supplied/default flag, and the - * stamp-memoized derived `name → Tool` / `name → output-validator` + * stamp-memoized derived `name → Tool` / `name → output-metadata` * indices — the cache-coordination state that used to live as separate * private fields here. The internal aggregating walk writes one entry per * list verb; `list_changed` evicts the matching method; * `_resetConnectionState` resets the lot. {@linkcode callTool}'s - * output-schema validation reads the derived `outputValidator` index (the - * substrate's first production caller); the stacked SEP-2243 PR wires - * `Mcp-Param-*` mirroring through `toolDefinition` on top. + * header mirroring and output-schema validation share the source Tool and + * compiled validator held together in the `outputValidator` index. */ private readonly _cache: ClientResponseCache; private readonly _defaultCacheTtlMs: number; @@ -1849,24 +1848,6 @@ export class Client extends Protocol { } } - /** - * Resolve the SEP-2243 `x-mcp-header` declaration scan for a tool name. - * - * The caller-supplied `toolDefinition` escape hatch wins; otherwise the - * cached `tools/list` entry (via the cache's `toolDefinition`) is the - * source. Freshness is the response cache's lifecycle: `list_changed` - * evicts, otherwise the held schema is the best information available - * regardless of age, and a stale schema is recovered through the - * `HEADER_MISMATCH` → evict-refetch-retry path in {@linkcode callTool}. - * On a miss the call proceeds without `Mcp-Param-*` headers (the spec's - * "client SHOULD send without custom headers" guidance) and relies on the - * same recovery. - */ - private async _resolveXMcpHeaderScan(name: string, override: Tool | undefined): Promise { - const tool = override ?? (await this._cache.toolDefinition(name)); - return tool === undefined ? undefined : scanXMcpHeaderDeclarations(tool.inputSchema); - } - /** * Reads the contents of a resource by URI. * @@ -2343,9 +2324,9 @@ export class Client extends Protocol { // of running SEP-2243 from a browser; pass `null` for the designated // argument or supply `options.toolDefinition` against a relaxed server. const mirroringActive = this.getProtocolEra() === 'modern' && detectProbeEnvironment() !== 'browser'; - // Mirroring (and output-schema validation below) read the cached - // `tools/list` entry directly via `_cache.toolDefinition` / - // `_cache.outputValidator`. `callTool` never issues a `tools/list` + // Mirroring and output-schema validation share one cached tool + // definition per attempt, including across an asynchronous store read. + // `callTool` never issues a `tools/list` // itself — the cache is populated by the caller's own // {@linkcode listTools | listTools()} (which now auto-aggregates) and // by the `HEADER_MISMATCH` recovery path below. A cold cache means @@ -2353,15 +2334,11 @@ export class Client extends Protocol { // "client SHOULD send without custom headers" guidance) and without // output-schema validation (the v1.x opportunistic behaviour, kept so // a legacy/stdio `callTool` still issues zero extra requests). - const buildSendOptions = async (): Promise => { - if (!mirroringActive) return options; - // A custom store's `get()` may reject (the documented store - // contract); route that to `onerror` and degrade to sending - // without `Mcp-Param-*` headers — same posture as a cold cache — - // rather than aborting the call before it reaches the wire. + const buildSendOptions = (): CallToolRequestOptions | undefined => { + if (!mirroringActive || metadata === undefined) return options; let scan: XMcpHeaderScanResult | undefined; try { - scan = await this._resolveXMcpHeaderScan(params.name, options?.toolDefinition); + scan = scanXMcpHeaderDeclarations(metadata.tool.inputSchema); } catch (error) { this._reportStoreError(error); } @@ -2378,13 +2355,18 @@ export class Client extends Protocol { // derived views must agree — and is compiled in isolation (never written to the cache). The // cache read is guarded: a custom store whose `get()` rejects routes to `onerror` and // degrades to skipping validation (same outcome as a cold cache). - let compiled = + // Memoize the source definition together with its compiled validator: + // a concurrent catalog refresh must not mix one generation's output + // schema with another generation's header declarations. + const resolveMetadata = () => options?.toolDefinition === undefined - ? await this._cache - .outputValidator(params.name, tool => this._compileOutputValidator(tool)) + ? this._cache + .outputValidator(params.name, tool => ({ tool, compiled: this._compileOutputValidator(tool) })) .catch(error => void this._reportStoreError(error)) - : this._compileOutputValidator(options.toolDefinition); + : { tool: options.toolDefinition, compiled: this._compileOutputValidator(options.toolDefinition) }; + let metadata = await resolveMetadata(); const assertCompiled = (): void => { + const compiled = metadata?.compiled; if (compiled === undefined || compiled.ok) return; const err = compiled.compileError; const message = (err instanceof Error ? err.message : String(err)).slice(0, 200); @@ -2397,7 +2379,7 @@ export class Client extends Protocol { // map there is no wider union to narrow away. let result: CallToolResult; try { - result = await this.request({ method: 'tools/call', params }, await buildSendOptions()); + result = await this.request({ method: 'tools/call', params }, buildSendOptions()); } catch (error) { // SEP-2243 one-refresh-on-miss: a `HEADER_MISMATCH` rejection on a // modern connection means the server enforced an `Mcp-Param-*` @@ -2429,18 +2411,17 @@ export class Client extends Protocol { // `HEADER_MISMATCH` — the refetch failure is observable only // through `onerror`. await this.listTools(undefined, refreshOptions).catch(error_ => this._reportStoreError(error_)); - // Re-resolve the output validator against the freshly-fetched entry — the pre-flight - // `compiled` was resolved from the now-evicted cache and may be stale (different - // outputSchema) or absent (cold cache on the first attempt). The recovery path is only + // Re-resolve both metadata views against the freshly-fetched entry — the pre-flight + // snapshot came from the now-evicted cache and may be stale (different input/output + // schemas) or absent (cold cache on the first attempt). The recovery path is only // entered when `options.toolDefinition` is undefined, so the cache is the sole source. // Re-run the same fail-fast compile-error check before issuing the retry. - compiled = await this._cache - .outputValidator(params.name, tool => this._compileOutputValidator(tool)) - .catch(error_ => void this._reportStoreError(error_)); + metadata = await resolveMetadata(); assertCompiled(); - result = await this.request({ method: 'tools/call', params }, await buildSendOptions()); + result = await this.request({ method: 'tools/call', params }, buildSendOptions()); } + const compiled = metadata?.compiled; const validator = compiled !== undefined && compiled.ok ? compiled.validator : undefined; if (validator) { // If tool has outputSchema, it MUST return structuredContent (unless it's an error). diff --git a/packages/client/src/client/responseCache.ts b/packages/client/src/client/responseCache.ts index e6103d34ce..140e940a29 100644 --- a/packages/client/src/client/responseCache.ts +++ b/packages/client/src/client/responseCache.ts @@ -291,7 +291,7 @@ export class ClientResponseCache { */ private _toolIndex?: { stamp: number; byName: Map }; /** - * `name → compiled output-schema validator` derived from the cached + * `name → caller-defined output metadata` derived from the cached * `tools/list` entry; same stamp-keyed memoization as `_toolIndex`. Typed * `unknown` so this class stays free of any validator-provider dependency * — the compile callback supplied to {@linkcode outputValidator} owns the @@ -605,8 +605,9 @@ export class ClientResponseCache { * Returns `undefined` only when no `tools/list` response is held at all, * or the held list does not contain `name`. * - * Consumed by `callTool()`'s SEP-2243 `_resolveXMcpHeaderScan` (mirroring) - * and, via {@linkcode outputValidator}, its output-schema validation. + * For consumers needing only the descriptor. `Client.callTool()` uses + * {@linkcode outputValidator} to retain the source Tool alongside its + * compiled output validator in one metadata snapshot. */ async toolDefinition(name: string): Promise { const entry = await this._probe('tools/list'); @@ -627,16 +628,17 @@ export class ClientResponseCache { } /** - * The compiled output-schema validator for tool `name`, derived from the + * Caller-defined output metadata for tool `name`, derived from the * cached `tools/list` entry — same source and same stamp-keyed - * memoization as {@linkcode toolDefinition}. The `name → validator` index + * memoization as {@linkcode toolDefinition}. The `name → metadata` index * re-derives only when the backing entry's stamp changes (a refetched * `tools/list` recompiles; a `list_changed` eviction drops it). Returns - * `undefined` when no `tools/list` is held, the tool is absent, or it has - * no `outputSchema`. + * `undefined` when no `tools/list` is held, the tool is absent, or the + * callback returns `undefined` for it. * - * `compile` is the caller-supplied validator-compile callback (the - * `Client` passes its `_jsonSchemaValidator` wrapper) so this + * `compile` is the caller-supplied callback. `Client` retains the source + * Tool alongside its compiled validator, including Tools without an + * `outputSchema`, so mirroring and validation share one snapshot. This * class carries no validator-provider dependency. One tool's uncompilable * `outputSchema` (e.g. an invalid `pattern` regex or unresolvable `$ref`) * must not poison every other tool's `callTool` — the callback isolates diff --git a/packages/client/test/client/mcpParamMirroring.test.ts b/packages/client/test/client/mcpParamMirroring.test.ts index b0959663ed..39fdbc3a43 100644 --- a/packages/client/test/client/mcpParamMirroring.test.ts +++ b/packages/client/test/client/mcpParamMirroring.test.ts @@ -12,7 +12,8 @@ import { encodeMcpParamValue, HEADER_MISMATCH_ERROR_CODE, InMemoryTransport, - PROTOCOL_VERSION_META_KEY + PROTOCOL_VERSION_META_KEY, + ProtocolErrorCode } from '@modelcontextprotocol/core-internal'; import { describe, expect, it, vi } from 'vitest'; @@ -125,6 +126,107 @@ function modernClient(store?: InMemoryResponseCacheStore): Client { } describe('SEP-2243 Mcp-Param-* mirroring (modern era)', () => { + it.each(['New', 'Old'])( + 'keeps mirroring and output validation on the same catalog generation during a refresh (%s result)', + async resultGeneration => { + const versionedTool = (generation: string): Tool => ({ + name: 'route', + inputSchema: { + type: 'object', + properties: { region: { type: 'string', 'x-mcp-header': `${generation}-Region` } } + }, + outputSchema: { + type: 'object', + properties: { generation: { const: generation } }, + required: ['generation'] + } + }); + const oldTool = versionedTool('Old'); + const newTool = versionedTool('New'); + const store = new InMemoryResponseCacheStore(); + const { clientTx, serverTx, callHeaders, listCount } = await scriptedModernServer([[newTool]]); + const originalHandler = serverTx.onmessage!; + let executions = 0; + serverTx.onmessage = (message, extra) => { + const request = message as JSONRPCRequest; + if (request.method !== 'tools/call') { + originalHandler(message, extra); + return; + } + // A rejected stale header does not execute the handler. Choose the + // response generation to exercise validation after recovery. + if (callHeaders.at(-1)?.['Mcp-Param-New-Region'] !== 'eu') { + void serverTx.send({ + jsonrpc: '2.0', + id: request.id, + error: { code: HEADER_MISMATCH_ERROR_CODE, message: 'stale header definition' } + }); + return; + } + executions++; + void serverTx.send({ + jsonrpc: '2.0', + id: request.id, + result: { resultType: 'complete', content: [], structuredContent: { generation: resultGeneration } } + }); + }; + const client = modernClient(store); + await client.connect(clientTx); + store.set({ method: 'tools/list', partition: PART }, { value: JSON.stringify({ tools: [oldTool] }) }); + + // Pause one async store read after it captured the old entry, then + // complete a real listTools refresh before allowing that read to finish. + // No timers or private client/cache methods are involved. + const get = store.get.bind(store); + let captured!: () => void; + const readCaptured = new Promise(resolve => { + captured = resolve; + }); + let release!: () => void; + const readReleased = new Promise(resolve => { + release = resolve; + }); + let pause = true; + (store as ResponseCacheStore).get = key => { + const entry = get(key); + if (pause && key.method === 'tools/list' && key.partition === PART) { + pause = false; + captured(); + return readReleased.then(() => entry); + } + return entry; + }; + + try { + const outcome = client.callTool({ name: 'route', arguments: { region: 'eu' } }).then( + result => ({ result }), + error => ({ error }) + ); + await readCaptured; + await client.listTools(undefined, { cacheMode: 'refresh' }); + release(); + const observed = await outcome; + if (resultGeneration === 'New') { + expect(observed).toEqual({ result: { content: [], structuredContent: { generation: 'New' } } }); + } else { + expect(observed).toMatchObject({ + error: { + code: ProtocolErrorCode.InvalidParams, + message: expect.stringContaining("does not match the tool's output schema") + } + }); + } + expect(executions).toBe(1); + expect(callHeaders).toEqual([{ 'Mcp-Param-Old-Region': 'eu' }, { 'Mcp-Param-New-Region': 'eu' }]); + expect(listCount()).toBe(2); + } finally { + release(); + await client.close(); + await serverTx.close(); + } + } + ); + it('listTools() and the cached tools/list entry exclude constraint-violating x-mcp-header tools and warn', async () => { const warn = vi.spyOn(console, 'warn').mockImplementation(() => undefined); const store = new InMemoryResponseCacheStore(); @@ -285,18 +387,14 @@ describe('SEP-2243 Mcp-Param-* mirroring (modern era)', () => { client.onerror = e => errors.push(e); await client.connect(clientTx); - // The pre-send mirroring read AND the post-success validator read both - // hit a rejecting `get()`. Neither aborts the call: the request goes - // out without `Mcp-Param-*` headers (cold-cache posture), the - // server-side result is returned, and both store failures surface via - // `onerror`. The post-success guard is the critical one — a store - // failure after the server has executed the call must never surface - // as a `callTool()` rejection (duplicate-execution hazard on retry). + // One metadata read supplies both mirroring and validation. Its store + // failure is reported once, and the successful call remains available + // to the caller without a second read after server execution. const result = await client.callTool({ name: 'route', arguments: { region: 'ap' } }); expect(result.content?.[0]).toEqual({ type: 'text', text: 'ok' }); expect(callHeaders).toEqual([undefined]); expect(listCount()).toBe(0); - expect(errors.map(e => e.message)).toEqual(['redis down', 'redis down']); + expect(errors.map(e => e.message)).toEqual(['redis down']); }); it('a paginating server: the cached aggregate holds every page and a page-2 x-mcp-header tool mirrors on the first call', async () => {