[kanchingyg]

Fixes #2614

Backports d9ff1b3c1d7e25198b062ab33769262bf533eb1b to v1.x.

cacheToolMetadata previously cleared _cachedToolOutputValidators, _cachedKnownTaskTools, and _cachedRequiredTaskTools before iterating over tools. If getValidator(tool.outputSchema) threw on a malformed schema midway through the loop, the client's cached tool metadata was left partially populated.

This change builds the map and sets in local variables first and swaps them onto the instance only after all tool schemas compile.

[changeset-bot]

🦋 Changeset detected

Latest commit: 1c161e4

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[pkg-pr-new]

Open in StackBlitz

npm i https://pkg.pr.new/@modelcontextprotocol/sdk@3093

commit: 1c161e4

[arhancanli]

Read the diff against the test it adds. The swap-after-build is the right shape for a throw in getValidator: the old code cleared the three caches first, so a bad outputSchema halfway through the list left a half-filled cache, and now it leaves the previous one intact.

Two consequences of keeping the previous cache that the new test pins without saying so:

1. The failing response is all-or-nothing. In the test the second listTools() contains only the invalid tool, but if it also listed a valid tool with an outputSchema, that valid tool would get no validator, while a tool the server has since removed (versioned) keeps validating against its old schema. So the client's cache can match neither the old nor the new server state. Wrapping the per-tool getValidator in a try/catch (skip that one tool, keep building, rethrow or report once at the end) would make the cache match the new list minus the broken tool. Is "previous cache survives intact" the intended contract, or just the simplest fix?
2. cacheToolMetadata replaces the maps on every call. If a server pages tools/list with cursor, does each page call it? If so, page 2 would drop the validators from page 1 (that predates this PR, but now the replace-on-success makes it the only path). A test with two pages would settle it.

Also the changeset bot reports no changeset; a patch entry like the one in #3091 would be enough.

[kanchingyg]

Thanks for the review @arhancanli!

1. Failure-atomic replacement vs. per-tool try/catch: Preserving the previous generation intact on throw is the explicit contract requested in Client.listTools() corrupts its cached tool metadata when an output schema fails to compile #2614 ("A failed catalog refresh should leave the previous complete metadata generation unchanged"). If listTools() rejects because a tool's outputSchema fails compilation, keeping the prior complete validator/task-metadata snapshot avoids silently downgrading validation for tools that were already valid.

2. Paginated tools/list (cursor): On v1.x, listTools(params) performs a single tools/list request and passes result.tools directly to cacheToolMetadata(result.tools). That full-catalog replacement behavior per listTools() call pre-exists this change; accumulating or merging across manual cursor pages would be a separate behavioral change from Client.listTools() corrupts its cached tool metadata when an output schema fails to compile #2614.

3. Changeset: Added the patch changeset in the latest commit.

[arhancanli]

Thanks, that settles both for me. If #2614 asks for "previous complete generation unchanged", then all-or-nothing is the contract, and the cache is always one complete catalog the server really returned, never a mix of old and new. And a manual cursor page replacing the map is the existing behaviour, so it belongs in its own change. The changeset is there now too.

One thing the rejected refresh leaves open: the caller gets the compile error from listTools(), but the client keeps validating against the old catalog from then on. Does the error name the tool whose outputSchema failed to compile? If it does not, someone whose server ships one bad schema has to bisect the list to find it, and a single assertion on the message in this test would pin that.

[kanchingyg]

getValidator() lets the underlying validator (Ajv.compile) throw directly, so the error message reports the schema path and reason (e.g. schema is invalid: data/properties/value/type must be equal to one of the allowed values...) rather than the tool name.

Keeping the original throw untouched matches main and #2614 and avoids changing the error message/shape callers may already match on v1.x.

[arhancanli]

Fair, leaving the throw untouched keeps this PR to the one contract in #2614, and I would not change the message shape here either.

The cost is that data/properties/value/type ... is a path inside one schema, so a server with ten tools that all have a value property gives the user nothing to search for. That is a separate follow-up, and it can stay compatible: keep the same Error and its message, and attach the tool name as an extra field or as cause, so anything matching on the message today still matches. Do you know of callers that match on that text, or is it only a worry about the shape? If nobody does, a one-line assertion in this test that listTools() rejects with the invalid tool's schema error would at least pin what users see today.

[kanchingyg]

It was only caution around changing the thrown error on \�1.x, not a known caller matching on that text; in fact, because \createDefaultAjvInstance()\ sets \�alidateSchema: false, \Ajv.compile()\ throws \ ype must be JSONType or JSONType[]: not-a-json-schema-type\ directly from codegen without a schema path. I tightened the \expect(client.listTools()).rejects.toThrow(/^type must be JSONType or JSONType[]: not-a-json-schema-type$/)\ assertion in the test to pin that exact message as suggested.

[arhancanli]

That pins it well: the anchored ^...$ regex means any later change to the thrown message fails this test on purpose, so the tool-name follow-up has to go through cause or an extra field to stay compatible, as you describe. Good that validateSchema: false is documented by the assertion too, since it explains why no schema path shows up.

For the follow-up: would you attach the tool name when cacheToolMetadata compiles each schema, or only wrap the error at the listTools() level? The first sees the tool directly, the second would need the tool list passed down.

[kanchingyg]

Attaching it inside cacheToolMetadata as it iterates each tool is much cleaner since tool.name is already in scope there without threading the tool list back up to listTools(). In a follow-up, catching the validateOrCreateValidator error inside that loop and attaching tool.name (e.g., via a non-breaking property or cause while preserving err.message) would give callers the exact tool name cleanly.

[arhancanli]

Agreed, the loop is the natural place, and it keeps the atomic part intact as long as the wrapper rethrows before the swap of the staged map, so a failed tool still leaves the previous cache untouched. A test for that ordering (call listTools() once with good schemas, then once with one bad schema, and assert the first result is still served) would pin the atomicity and the tool-name follow-up together.

One thing to check before choosing cause: does the v1.x build target include the ES2022 Error options argument (new Error(msg, { cause }))? If the tsconfig lib is older, a plain toolName property on the error would type-check where cause would not. Which one does the v1.x tsconfig allow?

[kanchingyg]

Follow-up for @arhancanli: cacheToolMetadata now wraps getValidator inside the per-tool loop, attaches toolName as a non-enumerable property on the original thrown Error (preserving message/shape for v1.x callers), and still rethrows before swapping the three caches so the previous complete generation stays intact. The two-call listTools() test asserts both the exact Ajv message and error.toolName === "invalid", then confirms versioned / task-required still use the first-call cache.

[arhancanli]

Read 1c161e4. The shape now matches what I asked for: the original Error is rethrown untouched for message and class, toolName is non-enumerable so it will not show up in JSON or toEqual comparisons, and the three assignments after the loop mean a throw can never leave a half-built generation. Asserting versioned still rejects against the old generation: 'old' schema is the right proof that the old cache survived.

One test detail: expect.unreachable(...) sits inside the try, so if a regression made the second listTools() succeed, the catch would swallow that error and the failure would show up as a message-regex mismatch instead of "should have thrown". Something like const err = await client.listTools().then(() => undefined, e => e) followed by expect(err).toBeInstanceOf(Error) keeps the failure readable.

A design question for you or the maintainers: with this change, one tool with a bad outputSchema makes the whole listTools() reject, while the client keeps serving the previous catalog. Do you want a caller that connects to a server with one bad schema on the very first listTools() to get nothing at all, or to get the other tools with the bad one skipped? Which of those the SDK promises is not covered by the tests yet.