feat(core,server,client): implement SEP-2106 (tool schemas conform to JSON Schema 2020-12)#2249
feat(core,server,client): implement SEP-2106 (tool schemas conform to JSON Schema 2020-12)#2249mattzcarey wants to merge 17 commits into
Conversation
🦋 Changeset detectedLatest commit: 64848bd The changes in this PR will be included in the next version bump. This PR includes changesets to release 7 packages
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 |
felixweinberger
left a comment
There was a problem hiding this comment.
Awesome thanks for working on this!
Quick note before a deeper read - there should be a currently failing conformance scenario in expected-failures.yaml here:
Can we remove that expected failure and confirm conformance passes with this change?
@modelcontextprotocol/client
@modelcontextprotocol/codemod
@modelcontextprotocol/server
@modelcontextprotocol/server-legacy
@modelcontextprotocol/express
@modelcontextprotocol/fastify
@modelcontextprotocol/hono
@modelcontextprotocol/node
commit: |
6b451c5 to
a277433
Compare
…review) Addresses claude-bot review on PR #2249: assertSchemaSafeToCompile (SEP-2106 SSRF / composition-DoS guard) throws inside getValidator(), and cacheToolMetadata() eagerly compiled every advertised tool's outputSchema with no per-tool error handling. A single tool advertising a non-local $ref or an over-budget schema therefore rejected the entire listTools() call, leaving the client unable to list or call ANY tool from that server. - catch compilation per-tool in cacheToolMetadata(); store the error in a new _toolOutputValidatorErrors map instead of letting it propagate. - surface the scoped error from callTool() only when the offending tool is actually called (clear, descriptive ProtocolError). - integration test: one tool with a non-local $ref outputSchema does not break listTools() or the use of a sibling good tool; the bad tool errors only on call. Note: the second review comment (experimental/tasks callToolStream truthiness) is stale \u2014 the tasks feature was removed in c8d7401 before this branch, no callToolStream exists, and no truthiness structuredContent checks remain.
…review) Addresses claude-bot review on PR #2249: assertSchemaSafeToCompile (SEP-2106 SSRF / composition-DoS guard) throws inside getValidator(), and cacheToolMetadata() eagerly compiled every advertised tool's outputSchema with no per-tool error handling. A single tool advertising a non-local $ref or an over-budget schema therefore rejected the entire listTools() call, leaving the client unable to list or call ANY tool from that server. - catch compilation per-tool in cacheToolMetadata(); store the error in a new _toolOutputValidatorErrors map instead of letting it propagate. - surface the scoped error from callTool() only when the offending tool is actually called (clear, descriptive ProtocolError). - integration test: one tool with a non-local $ref outputSchema does not break listTools() or the use of a sibling good tool; the bad tool errors only on call. Note: the second review comment (experimental/tasks callToolStream truthiness) is stale \u2014 the tasks feature was removed in c8d7401 before this branch, no callToolStream exists, and no truthiness structuredContent checks remain.
336f4fb to
c2533e1
Compare
|
Removed the Also rebased onto main (reconciled with the per-revision spec types from #2252) and scoped output-schema compile failures per-tool, so one tool advertising an uncompilable schema no longer rejects the whole |
There was a problem hiding this comment.
Implementation and the callTool JSDoc example look good. Two consumer-facing docs still show the pre-SEP pattern, and one of them re-teaches the bug this PR fixes, so could we update those before it lands?
-
docs/client.md(thestructuredContentsection) still calls it a JSON "object" and its snippet usesclient.callTool(...)withif (result.structuredContent)andconsole.log(result.structuredContent). That truthy check is the falsy bug this PR fixes I think. It sources fromexamples/client/src/clientGuide.examples.ts#callTool_structuredOutput(a different file from the JSDoc example you updated), so that one needs the same treatment:callTool<{ bmi: number }>()and\!== undefined. -
docs/server.mdhas a note recommending atypealias over aninterfacebecause interfaces "aren't assignable to{ [key: string]: unknown }." SincestructuredContentis nowunknown, that caveat no longer applies. Could we drop or revise it?
Another question:
I like the idea of introducing the callTool<T> generic for structuredContent, but it does mean people currently using structuredContent will have to change their code, doesn't it? I don't think we actually validate that the content that comes back is actually the shape <T> that's passed in, so we're just casting by another name potentially?
Thinking more about this - how would a client implementation ever know what to pass into That seems like a problem 🤔 The I think we need a stronger solution - previously it was still "unknown" what the return shape was but it was at least What we need here then seems like showing the patterns for how to deal with the Something like: const sc = result.structuredContent;
if (typeof sc === 'object' && sc !== null && !Array.isArray(sc)) {
const temp = (sc as Record<string, unknown>).temperature; // NOW the cast is safe; temp is unknown
} else if (Array.isArray(sc)) {
// handle array case
} else {
// string | number | boolean | null
}Should we just remove the |
…schema-validators array example migration.md §JSON Schema covers the three breaks (ref-denied, Ajv default 2020-12 with per-dialect routing, structuredContent unknown) with opt-backs. schema-validators example gains list-forecasts (array outputSchema) demonstrating the known-server cast idiom and the auto-TextContent fallback / legacy strip on all four legs. Thanks @mattzcarey (#2249).
…schema-validators array example migration.md §JSON Schema covers the three breaks (ref-denied, Ajv default 2020-12 with per-dialect routing, structuredContent unknown) with opt-backs. schema-validators example gains list-forecasts (array outputSchema) demonstrating the known-server cast idiom and the auto-TextContent fallback / legacy strip on all four legs. Thanks @mattzcarey (#2249).
…review) Addresses claude-bot review on PR #2249: assertSchemaSafeToCompile (SEP-2106 SSRF / composition-DoS guard) throws inside getValidator(), and cacheToolMetadata() eagerly compiled every advertised tool's outputSchema with no per-tool error handling. A single tool advertising a non-local $ref or an over-budget schema therefore rejected the entire listTools() call, leaving the client unable to list or call ANY tool from that server. - catch compilation per-tool in cacheToolMetadata(); store the error in a new _toolOutputValidatorErrors map instead of letting it propagate. - surface the scoped error from callTool() only when the offending tool is actually called (clear, descriptive ProtocolError). - integration test: one tool with a non-local $ref outputSchema does not break listTools() or the use of a sibling good tool; the bad tool errors only on call. Note: the second review comment (experimental/tasks callToolStream truthiness) is stale \u2014 the tasks feature was removed in c8d7401 before this branch, no callToolStream exists, and no truthiness structuredContent checks remain.
102f94b to
a9ee392
Compare
|
Rebased onto current |
|
Addressed the new SEP-2106 review threads in Changes:
Verified:
|
… schemas Tools' inputSchema/outputSchema now conform to full JSON Schema 2020-12 and structuredContent may be any JSON value (#2192). - outputSchema accepts any 2020-12 schema (arrays/primitives/objects/compositions); inputSchema keeps `type: "object"` but allows all 2020-12 keywords. Updated the generated spec types, the zod schemas, and standardSchemaToJsonSchema (output path no longer forces `type: "object"`). - structuredContent widens from `{ [key: string]: unknown }` to `unknown` (source-breaking for typed consumers). - Stronger typing: new CallToolResultWithStructuredContent<T>; client.callTool<T>() generic (cast-free via overload); registerTool type-checks a handler's structuredContent against its outputSchema's inferred output. - Fix falsy-structuredContent bug (=== undefined) in server + client. - Server auto-emits a serialized TextContent fallback for non-object structuredContent (pre-SEP client interop). - Security: validators reject non-same-document $ref/$dynamicRef (SSRF) and schemas exceeding depth/subschema bounds (composition DoS) via schemaBounds. Adds unit + integration tests, migration docs (migration.md + migration-SKILL.md), and a changeset.
Register the SEP-2106 conformance scenario in the everythingClient and remove it from expected-failures.yaml. The client already blocks non-local $ref dereferencing via assertSchemaSafeToCompile.
- ajvProvider: use Ajv2020 so the default Node validator honors the 2020-12 dialect (prefixItems etc.); previously new Ajv() ran draft-07 and silently ignored 2020-12 keywords (R-2106-1/2). - add MCP_DEFAULT_SCHEMA_DIALECT='2020-12' as the single source of truth; cfWorker provider defaults through it. - refactor the server structuredContent text-fallback from in-place mutation to a pure withStructuredContentTextFallback() so the tools/call path is side-effect-free. - tests: Ajv2020 prefixItems regression (both validators); standardSchema io:'output' branch; spec.types<->schemas field-level mirror; registerTool compile-time Output typing; falsy structuredContent round-trip (false/""/null); schema-safety guards surfacing cleanly via fromJsonSchema. - changeset: note the Ajv2020 default-dialect fix.
…review) Addresses claude-bot review on PR #2249: assertSchemaSafeToCompile (SEP-2106 SSRF / composition-DoS guard) throws inside getValidator(), and cacheToolMetadata() eagerly compiled every advertised tool's outputSchema with no per-tool error handling. A single tool advertising a non-local $ref or an over-budget schema therefore rejected the entire listTools() call, leaving the client unable to list or call ANY tool from that server. - catch compilation per-tool in cacheToolMetadata(); store the error in a new _toolOutputValidatorErrors map instead of letting it propagate. - surface the scoped error from callTool() only when the offending tool is actually called (clear, descriptive ProtocolError). - integration test: one tool with a non-local $ref outputSchema does not break listTools() or the use of a sibling good tool; the bad tool errors only on call. Note: the second review comment (experimental/tasks callToolStream truthiness) is stale \u2014 the tasks feature was removed in c8d7401 before this branch, no callToolStream exists, and no truthiness structuredContent checks remain.
Adds resolveExternalSchemaRefs(schema, options) — the SEP's optional, opt-in external-$ref mode. Validators stay synchronous and never fetch; this helper runs ahead of time and returns a self-contained schema (external docs fetched once, flattened into $defs, every $ref rewritten to a root-relative JSON Pointer), so the result passes the default safety guard and compiles with AJV/cfworker without any network access at validation time. Per the SEP it is: - disabled by default (a separate function requiring explicit operator action); - host-restricted: enforces an allowlist when given, else rejects loopback/link-local/private literal targets; https-only by default; - bounded: per-request timeout, response byte cap (streaming-enforced), max-documents cap; - observable: onDereference callback logs each fetched URI; - fail-closed: unresolved/oversized/non-JSON/disallowed refs throw, never a silent pass. Transitive refs are resolved; external $anchor fragments and nested $id/$anchor in fetched docs are rejected (cannot be flattened safely). 17 unit tests (happy path, end-to-end compile+validate via both AJV and cfworker, transitive resolution, allowlist/protocol/SSRF rejections, byte/document bounds, fail-closed cases). Exported from core; examples synced; changeset updated.
… fixture tool The json-schema-2020-12 server scenario asserts that $schema, $defs/$anchor, additionalProperties, composition (allOf/anyOf), and conditional (if/then/else) keywords are preserved verbatim in tools/list. The fixture registered the tool with a zod schema that never contained those keywords, so the scenario failed with 'field was likely stripped'. Register the exact scenario schema as raw JSON Schema via fromJsonSchema() instead; all 7 checks now pass. Also note the per-tool outputSchema compile-failure scoping in the SEP-2106 changeset.
a944f69 to
4229df5
Compare
| * @module | ||
| */ | ||
|
|
||
| import type { JsonSchemaType } from './types.js'; |
There was a problem hiding this comment.
🟡 The two new files externalRefResolver.ts (line 36) and externalRefResolver.examples.ts (lines 9-10) use .js-suffixed relative imports (e.g. from './types.js'), which violates the repo convention documented in CLAUDE.md ("no .js extension on relative imports") and is inconsistent with the PR's other new files (schemaBounds.ts, schemaCompatibility.ts) and the rest of the package. Dropping the .js suffixes makes them consistent.
Extended reasoning...
What the issue is. CLAUDE.md (Code Style Guidelines > Imports, line 39) documents the repo convention: "ES module style, no .js extension on relative imports (project uses moduleResolution: bundler)". The two new source files added by this PR violate it:
packages/core/src/validators/externalRefResolver.ts:36—import type { JsonSchemaType } from './types.js';packages/core/src/validators/externalRefResolver.examples.ts:9-10—import { resolveExternalSchemaRefs } from './externalRefResolver.js';andimport type { JsonSchemaType } from './types.js';
Why it stands out. A grep over packages/core/src (and the client/server packages) shows these are the only source files using .js-suffixed relative imports — every other source file in the package follows the extensionless convention. The inconsistency is even internal to this PR: its other new files (schemaBounds.ts, schemaCompatibility.ts) and its edits to ajvProvider.ts/cfWorkerProvider.ts correctly use extensionless relative imports (e.g. ajvProvider.ts has import { assertSchemaSafeToCompile } from './schemaBounds';).
Why nothing catches it. With moduleResolution: bundler both spellings resolve, so typecheck:all/build:all/lint:all all pass either way — there is no functional impact. The convention is only enforced by documentation and reviewer eyes, which is why these slipped through.
Concrete walk-through. 1) Open externalRefResolver.ts:36 — the import reads from './types.js'. 2) Open the sibling schemaCompatibility.ts:1 (added by the same PR) — the equivalent import reads from './types'. 3) CLAUDE.md line 39 says the latter is the documented form. So two files in the same directory, added by the same PR, importing the same module, use two different styles, with the documented one being the extensionless form.
Impact. Purely stylistic/consistency — no runtime or compile-time effect. The cost is future drift: copy-pasting from these files propagates the off-convention style.
How to fix. Drop the .js suffixes in the three import statements:
externalRefResolver.ts:36→from './types'externalRefResolver.examples.ts:9→from './externalRefResolver'externalRefResolver.examples.ts:10→from './types'
(Also worth checking externalRefResolver.test.ts in packages/core/test/validators, which imports source modules with .js suffixes — though test files elsewhere may already vary, so the source files are the main concern.)
Implements SEP-2106 — tool
inputSchema/outputSchemaconform to JSON Schema 2020-12, andstructuredContentmay be any JSON value.Closes #2192.
What changed
Schema surface
inputSchemakeepstype: "object"at the root but now accepts any JSON Schema 2020-12 keyword (composition, conditional,$ref/$defs/$anchor, …).outputSchemamay now be any valid JSON Schema 2020-12 — objects, arrays, primitives, or compositions (was restricted totype: "object").structuredContentwidens from{ [key: string]: unknown }tounknown(source-breaking for typed consumers).spec.types.ts(surgically, matching the upstream regen for these fields), the hand-maintained zod inschemas.ts, andstandardSchemaToJsonSchema(theoutputpath no longer forcestype: "object";inputand prompt args stay object-only).Stronger typing (cast-free)
CallToolResultWithStructuredContent<T>type for code paths whose output shape is known ahead of time.client.callTool()returnsCallToolResultwithstructuredContent: unknown; callers should narrow at runtime before reading object properties.McpServer.registerToolnow type-checks a handler's returnedstructuredContentagainst the tool'soutputSchemainferred output (ToolResultFor<Output>threaded throughToolCallback).Behavior
0/false/""as "no structured content"; now check=== undefined.structuredContentautomatically also emit a serializedTextContentblock so pre-SEP clients can fall back to the text content (object output, or results that already carry text, are untouched).Security
$ref/$dynamicRef(SSRF guard) and reject schemas exceeding depth / subschema-count bounds (composition-DoS guard), via the newschemaBounds.ts. CustomjsonSchemaValidatorimplementations are unaffected.resolveExternalSchemaRefs(schema, options)helper fetches and inlines external refs ahead of validation; it is exported through the curated public API and re-exported by client/server.Tests & docs
test/integration/test/sep2106.test.ts(array/primitive round-trips, falsy values, TextContent auto-inject, runtime narrowing guidance) andpackages/core/test/validators/schemaBounds.test.ts; provider-level SSRF tests added tovalidators.test.ts.docs/migration.mdanddocs/migration-SKILL.md; minor changeset.Verification
typecheck:all✅ ·lint:all✅ ·build:all✅ ·sync:snippetsno-diff ✅ · core/client/server/integration slices ✅.Note on generated types
spec.types.tscarries only the SEP-2106 field changes (so the spec↔SDK assignability test passes) rather than a fullfetch:spec-typesregen, which would pull unrelated drift owned by the dedicatedupdate-spec-typesbranch. The edits match that branch verbatim for these fields, so a later full regen merges cleanly.