fix(validators): honor declared draft-07/06 JSON Schema dialects instead of rejecting them#2534
fix(validators): honor declared draft-07/06 JSON Schema dialects instead of rejecting them#2534felixweinberger wants to merge 1 commit into
Conversation
🦋 Changeset detectedLatest commit: e7392f5 The changes in this PR will be included in the next version bump. This PR includes changesets to release 6 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 |
@modelcontextprotocol/client
@modelcontextprotocol/codemod
@modelcontextprotocol/core
@modelcontextprotocol/server
@modelcontextprotocol/server-legacy
@modelcontextprotocol/express
@modelcontextprotocol/fastify
@modelcontextprotocol/hono
@modelcontextprotocol/node
commit: |
@modelcontextprotocol/client
@modelcontextprotocol/codemod
@modelcontextprotocol/core
@modelcontextprotocol/server
@modelcontextprotocol/server-legacy
@modelcontextprotocol/express
@modelcontextprotocol/fastify
@modelcontextprotocol/hono
@modelcontextprotocol/node
commit: |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the inline findings, a few adjacent concerns were checked and ruled out: (1) per-dialect engine cache isolation — the $id-based getSchema lookup runs against the engine already selected by _engineFor, so a 2020-12 and a draft-07 schema sharing an $id cannot hit each other's compiled cache; (2) both escape hatches are unchanged — _userAjv short-circuits before declaredDialect, and a caller-forced {draft} bypasses _draftFor via this.draft ?? …; (3) no stale "2020-12 only" prose survives elsewhere — the only remaining mentions in packages/ and docs/ are the updated migration-guide sections and the new shared error message.
Extended reasoning...
The three inline findings are all nit-severity (an engine-semantics divergence worth documenting, a test-only type-parameter mismatch, and pre-existing legacy-wrap rewriter gaps newly activated by the dialect dispatch). Since findings exist, the inline comments already signal that a human should look; this note only records the additional checks performed this run — engine-cache isolation across the two lazily built Ajv instances, preservation of the bring-your-own-engine and forced-draft bypass contracts, and a completeness sweep for surviving old-posture prose — so a later pass need not re-derive them.
d2ce73c to
3b062e9
Compare
There was a problem hiding this comment.
Beyond the inline changeset-wording nit, this run also examined the PR description's claim of a /_shims-subpath integration test: no such test is in the diff (only outputSchemaDialect.test.ts ships, and the integration suite has no shims legs) — judged a description-level discrepancy with no code impact, since the shipped integration/e2e tests exercise the dispatch path directly. The prior round's substantive findings ($ref-sibling deviation documentation, legacyWrap dependencies/fragment-$id handling, the test generic) all appear addressed in this revision.
Extended reasoning...
This revision (3b062e9) resolves every substantive item from the previous review pass: the Ajv-vs-cfworker $ref-sibling divergence is now documented in the provider JSDoc and migration guide and pinned by a recorded-contract test, the 2025-era $ref rewriter gained dependencies as a name-map key and fragment-only-$id descent with dedicated tests, and the integration test's fromJsonSchema generic now matches its schema. The only residual finding is the changeset sentence still claiming unqualified cross-provider draft-07 parity, posted inline as a nit. I additionally verified the ruled-out candidate myself: grepping the diff and test/integration confirms no /_shims-subpath test exists and the integration vitest config has no shims environments, so the PR description's testing claim is inaccurate prose only — the actual dispatch behavior is covered by the shipped unit, integration, and e2e updates. Not approving because, beyond the nit, the PR changes the default validator's acceptance behavior in core-internal (a design call about which dialects the SDK honors by default) that a maintainer should weigh.
0a8ab1c to
bd3a071
Compare
There was a problem hiding this comment.
Beyond the new inline note on $recursiveRef, I verified all prior review-round findings are addressed at bd3a071: dependencies is now in REF_REWRITE_NAME_MAP_KEYS and fragment-only $id no longer suppresses the ref rewrite (both with tests), the fromJsonSchema generic now matches its schema, and the changeset/JSDoc/migration guide all carry the documented classic-Ajv $ref-sibling deviation.
Extended reasoning...
One nit was found this run (2019-09 $recursiveRef left unrewritten by the 2025-era legacy wrap), posted inline. Separately, I re-checked each of the four issues raised in the previous review rounds against the current revision (bd3a071) and confirmed all are resolved in code and prose, so the earlier bot comments on this thread are stale. The remaining nit is narrow (hand-authored recursive 2019-09 schema with a non-object root on the 2025-11-25 projection) and does not affect the PR's motivating producers.
The default validator rejected any schema declaring a non-2020-12
$schema, so tools from servers emitting zod-to-json-schema output
failed pre-wire with InvalidParams before a request was ever sent —
its default target stamps draft-07 (e.g. the Filesystem reference
server), and its 2019-09/openAi targets stamp 2019-09.
Dispatch on the declared dialect instead, via a classifier shared by
both providers: no $schema or 2020-12 keeps the 2020-12 engine,
2019-09 validates with Ajv2019 (native 2019-09 on the cfworker
provider), draft-07/06 validate with a draft-07 engine (Ajv classic
on Node, draft '7' on cfworker), and unknown dialects still throw the
typed error, now listing the supported dialects. Caller-supplied
engines and explicit { draft } continue to bypass the dispatch.
The 2025-era outputSchema wrapper's $ref rewriter learns the same
vocabulary: draft-07 'dependencies' is a name-to-subschema map, a
fragment-only $id does not establish a new resolution base, and an
anchor-less 2019-09 $recursiveRef converts to the statically
equivalent rewritten $ref (anchored recursion is a documented
limitation). A coverage block in legacyWrap.ts enumerates every
reference/base-affecting keyword across the four dialects. The known
classic-Ajv deviation on $ref sibling keywords (evaluated, stricter
than draft-07's ignore rule, matching v1's default engine) is
documented and pinned by a test.
bd3a071 to
e7392f5
Compare
| // Anchor-less documents only: `$recursiveRef: '#'` is statically `$ref: '#'`-equivalent | ||
| // and is converted to the rewritten pointer. See the coverage block above. | ||
| const convertRecursiveRefs = !hasRecursiveAnchor(natural, false); |
There was a problem hiding this comment.
🟡 The new $recursiveRef:'#' → $ref:'#/properties/result' conversion is under-applied in two spec-valid 2019-09 shapes, both leaving '#' verbatim so it statically targets the wrapper envelope and spec-valid recursive structuredContent is rejected on the 2025-11-25 projection while the identical tool works on 2026-07-28: (1) the hasRecursiveAnchor gate scans the whole document, but per 2019-09 core §8.2.4.2.1 only a $recursiveAnchor at the document root makes a root-base $recursiveRef dynamic — a non-root anchor (e.g. under $defs) is inert, so the gate should be natural['$recursiveAnchor'] !== true (and the comment above hasRecursiveAnchor claiming presence 'anywhere in the document' switches it to dynamic is factually wrong); (2) the !('$ref' in node) guard skips conversion when $ref and $recursiveRef legally co-occur (conjunctive in 2019-09), which needs an allOf split or at least a KNOWN LIMITATION entry. In both cases the coverage block's 'Anchor-less documents: converted (statically equivalent)' overclaims, and neither shape has a pinning test.
Extended reasoning...
What the bug is. This PR's wrapOutputSchemaForLegacy adds a $recursiveRef:'#' → $ref:'#/properties/result' conversion for the 2019-09 recursion idiom (legacyWrap.ts:163), gated by (a) convertRecursiveRefs = !hasRecursiveAnchor(natural, false) at line 147 and (b) !('$ref' in node) on the conversion branch itself. Both gates are over-broad, and in both suppressed cases the $recursiveRef falls to the final else and is copied verbatim into the wrapped schema — where '#' now statically resolves to the wrapper envelope ({type:'object', required:['result']}) instead of the natural schema root. The result: spec-valid recursive structuredContent fails callTool with "does not match the tool's output schema" on the 2025-11-25 projection while the identical tool works on 2026-07-28 — the exact cross-era inconsistency the surrounding comments say the rewrite exists to prevent. Both shapes were empirically reproduced on the pinned engines (ajv 8.17.1 Ajv2019 with the provider's exact options, @cfworker/json-schema 4.1.1 in '2019-09' mode).\n\nCase 1 — document-wide anchor scan (line 147). Per 2019-09 core §8.2.4.2.1, $recursiveRef's value is restricted to '#' and dynamic re-resolution triggers only when the initially-resolved target carries $recursiveAnchor: true — for a root-base ref that target is the document root (refs inside base-establishing $id subtrees are skipped by rewriteRefs anyway). A $recursiveAnchor at any non-root position (e.g. at the top of an inlined library fragment under $defs) can never be the initial target, so it is inert: the ref resolves statically and conversion is both safe and required after relocation. But hasRecursiveAnchor scans the entire document (including the name-map path into $defs), so a non-root anchor flips convertRecursiveRefs to false and the ref is copied verbatim. The comment above hasRecursiveAnchor ("Its presence anywhere in the document switches $recursiveRef from static to dynamic") is factually wrong, and this case is not covered by the documented KNOWN LIMITATION — that limitation's rationale ("relocation cannot preserve dynamic re-resolution") doesn't apply when there is no dynamic re-resolution to preserve.\n\nProof (case 1), verified on both pinned engines. Natural schema:\njson\n{ \"$schema\": \"https://json-schema.org/draft/2019-09/schema\",\n \"type\": \"array\",\n \"items\": { \"anyOf\": [{ \"type\": \"number\" }, { \"$recursiveRef\": \"#\" }] },\n \"$defs\": { \"unused\": { \"$recursiveAnchor\": true, \"type\": \"string\" } } }\n\n1. Both Ajv2019 and cfworker accept [1,[2,3]] against the natural schema — the non-root anchor is inert, confirming static resolution — so the tool works on the 2026-07-28 era.\n2. hasRecursiveAnchor finds the $defs anchor → convertRecursiveRefs = false → {\"$recursiveRef\":\"#\"} is copied verbatim into the wrapped schema.\n3. The wrapped schema rejects {result:[1,[2,3]]} on both engines (the inner [2,3] is validated against the envelope root, which requires an object with result) while accepting {result:[1]} — every recursion-depth ≥ 1 result fails on the 2025 projection only.\n4. Simulating the conversion ($ref:'#/properties/result') restores acceptance of {result:[1,[2,3]]} and still rejects {result:[1,'x']} on both engines — the static rewrite is exactly equivalent; the gate is the only blocker.\n\nCase 2 — the !('$ref' in node) guard. In 2019-09 (the only dialect with $recursiveRef), $ref stopped replacing its siblings: $ref and $recursiveRef are independent in-place applicators that may legally co-occur in one schema object and both apply (core §8.2.4). For an anchor-less document like { $schema: 2019-09, type:'array', items: { $ref:'#/$defs/leaf', $recursiveRef:'#' }, $defs:{ leaf:… } }, after wrapping the $ref is correctly rewritten to #/properties/result/$defs/leaf but the $recursiveRef stays '#', mis-targeting the envelope. Verified: the natural schema accepts recursive data on both engines; the wrapped output rejects it; an allOf split ({ allOf: [{$ref: <relocated>}, {$ref: '#/properties/result'}] }) accepts it — so a semantics-preserving fix exists and the current output is silently wrong, not merely lenient. The guard itself is understandable (a naive conversion would collide with the existing $ref key), but as shipped this is a second unconverted, mis-resolving case that is neither converted nor documented.\n\nWhy existing code/tests don't catch it. Both shapes are newly reachable via this PR (declared 2019-09 schemas now dispatch to a real engine end-to-end). legacyWrap.test.ts pins anchor-less conversion, root-level-$recursiveAnchor verbatim, and name-position anchors — but the anchored tests only place the anchor at the root (where verbatim is correct), and no test covers $ref+$recursiveRef co-occurrence. Meanwhile the coverage block's unconditional claim — "Anchor-less documents: converted to $ref: '#/properties/result' (statically equivalent)" — overclaims for case 2, and only the anchored-root case is listed as a KNOWN LIMITATION.\n\nHow to fix. (1) Replace the deep scan with a root check: const convertRecursiveRefs = natural['$recursiveAnchor'] !== true — root-base refs are static iff the document root lacks the anchor — and correct the hasRecursiveAnchor comment (the helper can then be deleted). This preserves the documented limitation for genuinely anchored roots while converting all statically-resolvable cases. (2) For the $ref-sibling case, either implement the allOf split (out.allOf = [...(existing allOf ?? []), { $ref: '#/properties/result' }] semantics) or add it to the coverage block as a second KNOWN LIMITATION. Add pinning tests for both shapes.\n\nWhy nit, not blocking. The failures are concrete and silent, but the triggers are corner-of-a-corner: a 2019-09-declared, non-object-root outputSchema using $recursiveRef whose only $recursiveAnchor sits at a non-root position (a placement 2019-09 gives no defined behavior to, so real corpora essentially never author it), or $recursiveRef sharing a schema object with $ref — each additionally requiring a 2025-era peer. Realistic $recursiveRef usage pairs it with a root $recursiveAnchor, which is already the documented limitation. Nothing mainstream breaks on merge; the fixes are a one-line gate change plus a doc/limitation entry and tests.
| * Pick the engine draft for a schema's declared dialect (a caller-forced `{draft}` bypasses | ||
| * this — do not second-guess by `$schema`). No `$schema` or 2020-12 → `'2020-12'`; 2019-09 → | ||
| * `'2019-09'`; draft-07 or draft-06 → `'7'`; anything else → `Error`. | ||
| */ | ||
| private _draftFor(schema: JsonSchemaType): CfWorkerSchemaDraft { | ||
| const dialect = declaredDialect(schema, 'pass an explicit { draft } to CfWorkerJsonSchemaValidator to validate other dialects.'); | ||
| return dialect === 'draft-7' ? '7' : dialect; | ||
| } | ||
|
|
There was a problem hiding this comment.
🟡 A second, undocumented draft-07 engine divergence: @cfworker/json-schema's dereference() doesn't treat draft-07 dependencies as a name→subschema map, so an entry whose key collides with the engine's ignored-keyword set (type, default, format, required, pattern, id, …) and contains a $ref is skipped during ref registration — the returned validator then throws Unresolved $ref data-dependently when the dependency triggers, surfacing as InvalidParams in callTool on browser/Workers while the byte-identical schema validates correctly on the Node (classic Ajv) path. Consider documenting it alongside the $ref-siblings deviation (JSDoc + migration guide) and adding a cfworker engine-leg pinning test; longer-term, pre-normalizing draft-07 dependencies into dependentSchemas/dependentRequired before handing the schema to cfworker would close the gap.
Extended reasoning...
What the bug is. Routing declared draft-07/06 schemas to @cfworker/json-schema draft '7' (_draftFor, cfWorkerProvider.ts:63-71) makes draft-07 dependencies reachable through the default browser/Workers validator for the first time — but the pinned engine (4.1.1) has a gap the SDK now surfaces. Its dereference() does not list dependencies in its schemaMapKeyword set (only $defs/definitions/properties/patternProperties/dependentSchemas), so the dependencies map is walked in keyword position: any entry whose author-chosen property-name key collides with the engine's ignoredKeyword set (type, default, format, required, pattern, enum, const, id, $id, minimum, maxLength, …) is skipped entirely, and a $ref inside it is never registered. At validate time, when the dependency triggers, the engine throws Error('Unresolved $ref "#/definitions/…"') instead of returning a result.
Step-by-step proof (verified against the pinned @cfworker/json-schema 4.1.1 with the provider's exact construction, new Validator(schema, '7', true)):
const schema = {
$schema: 'http://json-schema.org/draft-07/schema#',
type: 'object',
definitions: { addr: { type: 'string' } },
properties: { type: { type: 'string' }, billing: {} },
// canonical draft-07 idiom: 'when `type` is present, constrain `billing`'
dependencies: { type: { properties: { billing: { $ref: '#/definitions/addr' } } } }
};
// cfworker (browser/Workers default path):
validate({ billing: 'x' }) // → valid: true (dependency not triggered)
validate({ type: 'card', billing: 'x' }) // → THROWS Error('Unresolved $ref "#/definitions/addr"')
// classic Ajv (Node default path, provider's exact options):
// → true for valid data, false for invalid data — correct draft-07 behaviorThe same throw reproduces for entry keys default, format, required, pattern, id, minimum; a non-colliding key (e.g. creditCard) validates correctly — confirming the dereference-skip mechanism. The engine's known-schemas map registers #/dependencies but not #/dependencies/type, so validate.js falls back to the raw pointer, which is not a lookup key, and throws.
Why existing safeguards don't catch it. The Validator constructor succeeds, so this is not caught by the per-tool compile-error isolation (client:jsonschema:bad-schema-isolates-tool) — the throw happens inside the returned validator closure, data-dependently. In Client.callTool (client.ts ~2449-2467) it is caught and converted to ProtocolError(InvalidParams, 'Failed to validate structured content: Unresolved $ref …'). The shared declaredDialect classifier guarantees both platforms pick the same dialect, but cannot make the cfworker engine's draft-7 keyword coverage complete. And the new engine-leg tests are Ajv-only — the validators.test.ts comment assumes cfworker dispatch differences are benign leniency ("applies both keyword sets in either draft mode"), which this shape disproves: it is a hard throw, not leniency. Notably, dependencies is exactly the keyword this PR deliberately supported in the legacyWrap rewriter — including a test for an entry keyed default with a $ref inside — yet that shape never ran through the cfworker engine.
Impact. A spec-valid draft-07 tool with spec-valid structuredContent works on a default Node client but fails intermittently (only on results where the dependency triggers) with InvalidParams on the byte-identical browser/Cloudflare Workers client. The same asymmetry applies server-side via fromJsonSchema input validation on Workers runtimes. This is a second engine divergence beyond the $ref-siblings one the PR documents — in the opposite direction (hard failure rather than Ajv-side strictness) — so the changeset's "one documented engine difference" framing and its "validates with draft-07 semantics on both … providers" claim are incomplete.
Why nit rather than blocking. Nothing that worked pre-PR regresses: before this change, any draft-07-declared schema was rejected pre-wire with the typed error on both platforms (100% failure), so the divergence is newly created surface, not a regression. The trigger intersection is narrow — schema-form dependencies + an entry key colliding with cfworker's ignored-keyword list + a $ref inside + a Workers/browser runtime + data that actually fires the dependency (zod-to-json-schema does not emit dependencies). And the root cause is an upstream engine limitation the SDK surfaces, not wrong SDK dispatch logic.
How to fix. Match the treatment the PR gave the $ref-siblings divergence: document it in the CfWorkerJsonSchemaValidator JSDoc and the migration guide's "known draft-07 engine difference" paragraph, and add a cfworker engine-leg pinning test (so a dependency bump that fixes or shifts the behavior is caught). Longer-term options: pre-normalize draft-07 dependencies into dependentSchemas/dependentRequired before constructing the cfworker Validator, or fix upstream by adding dependencies to schemaMapKeyword in @cfworker/json-schema's dereference.
1 participant
The default JSON Schema validator rejects any schema that declares a non-2020-12 dialect:
getValidatorthrows on"$schema": "http://json-schema.org/draft-07/schema#", soclient.callToolfails with-32602before the request reaches the wire. Much of the ecosystem emits draft-07 or 2019-09 —zod-to-json-schema's default output stamps draft-07 (including the schemas shipped by the official Filesystem server) and its2019-09/openAitargets stamp 2019-09 — so a client with default options cannot call tools on those servers, even though the spec explicitly permits them: schemas "MAY include a$schemafield to specify a different dialect" and implementations "MUST validate schemas according to their declared or default dialect" (basic/index.mdx, Schema Dialect; tools.mdx ships a draft-07 example).Motivation and Context
A conforming server that declares draft-07 output schemas is unusable from a default-configured v2 client. The v1 default engine (classic Ajv) accepted these schemas, so this is also a v1→v2 behavior gap.
The fix dispatches on the declared dialect instead of rejecting it:
2020-12(or no$schema) keeps the current Ajv 2020 engine; declared 2019-09 compiles with Ajv 2019 (native2019-09draft on the @cfworker provider); declared draft-07/draft-06 (httporhttpsURIs) compiles with classic Ajv, giving real declared-dialect semantics rather than a downleveled approximation; a genuinely unknown dialect still produces the typed error, which now lists the supported dialects — per the spec's "handle unsupported dialects gracefully" requirement. The dialect classifier is shared between the Ajv and @cfworker providers so the two platforms cannot drift. ThejsonSchemaValidatorescape hatch is untouched. The draft-07 engine was already bundled (~4 KB dispatch-code delta); the 2019-09 engine adds ~14 KB (unminified) to the Node provider chunk and ~0.1 KB to the browser/Workers chunk.How Has This Been Tested?
main, green here: a draft-07outputSchemathrough a realclient.callToolround-trip (Filesystem-server-shaped), a 2019-09 one (openAi-target-shaped), and the same via the/_shimssubpath.items-array (tuple) form validates under draft-07 rules — proving the dispatch selects the right engine, not merely that compilation succeeds (for 2019-09,unevaluatedPropertiesenforcement pins Ajv 2019 against both wrong-engine alternatives). Violating draft-07 and 2019-09 results still fail validation.test:all, examples, e2e) passing.Breaking Changes
None. Schemas that previously compiled behave identically; schemas that previously threw now validate under their declared dialect (2019-09, draft-07/06) or produce the same typed error (unknown dialects).
Types of changes
Checklist