fix(validators): honor declared draft-07/06 JSON Schema dialects instead of rejecting them - #2534
Conversation
🦋 Changeset detectedLatest commit: 4bbede3 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.
bd3a071 to
e7392f5
Compare
| return this.ajv; | ||
| } | ||
| if (dialect === '2019-09') { | ||
| return (this._ajv2019 ??= createDefaultAjvInstance(Ajv2019)); |
There was a problem hiding this comment.
is this still a default?
There was a problem hiding this comment.
Not sure what you mean here - default without a $schema specified remains 2020-12, the default in createDefault... means engine configuration in this case, but now supporting the different dialects instead of being coupled to 2020-12 specifically.
e7392f5 to
754b435
Compare
754b435 to
6a661ac
Compare
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 a
2019-09 $recursiveRef converts to the statically equivalent rewritten
$ref whenever the document root carries no $recursiveAnchor (joining
an allOf when $ref co-occurs conjunctively; root-anchored recursion
is a documented limitation). A coverage block in legacyWrap.ts
enumerates every reference/base-affecting keyword across the four
dialects. Two known draft-07 engine differences are documented and
pinned by tests: classic Ajv evaluates $ref sibling keywords
(stricter than draft-07's ignore rule, matching v1's default engine),
and the cfworker engine throws on $ref inside keyword-named
'dependencies' entries.
6a661ac to
7387593
Compare
| it('AJV: custom Ajv instance bypasses the $schema check (caller owns dialect)', () => { | ||
| // A draft-07 Ajv passed explicitly: even with `$schema: draft-07`, the provider does not | ||
| // throw — and `prefixItems` is unknown to draft-07 Ajv and silently ignored. |
There was a problem hiding this comment.
🟡 The two escape-hatch tests here ('AJV: custom Ajv instance bypasses the $schema check' and 'CfWorker: explicit {draft} bypasses the $schema check') probe with a draft-07-stamped schema, but this PR makes draft-07 a supported dialect — so both tests now pass even if the bypass (_userAjv / this.draft short-circuit) is removed, since the default dispatch handles draft-07 identically without throwing. Probing with a still-rejected dialect (e.g. draft-04, as this PR already did for the analogous rejection test in ajvLazyInit.test.ts) restores their discriminating power.
Extended reasoning...
What the issue is. validators.test.ts keeps two pre-existing tests whose stated contract is that the escape hatch bypasses the $schema dialect check: 'AJV: custom Ajv instance bypasses the $schema check (caller owns dialect)' and 'CfWorker: explicit {draft} bypasses the $schema check (caller owns dialect)'. Both probe with prefixItemsSchema(DRAFT_07_URI). Before this PR, a draft-07 $schema was rejected by the default path, so these tests genuinely discriminated: if the bypass regressed, getValidator would throw and the test would fail. After this PR, draft-07 is a supported dialect, so the default path no longer throws on the probe — and both tests pass whether or not the bypass code path exists.\n\nAJV leg. The test's only assertions are that getValidator does not throw and that prefixItems is ignored (v(PREFIX_ITEMS_BAD).valid === true). Suppose the _userAjv short-circuit in _engineFor (ajvProvider.ts) were removed — i.e. the bypass regressed and the provider dispatched by $schema even for a caller-supplied engine. Then declaredDialect returns 'draft-7' (no throw post-PR) and the provider builds its own default classic Draft7Ajv (strict: false), which also silently ignores prefixItems — so v(PREFIX_ITEMS_BAD).valid is still true and the test stays green. Nothing in the test distinguishes "the caller's engine ran" from "the provider's own draft-07 engine ran."\n\nCfWorker leg. The test asserts only .not.toThrow() on a draft-07-stamped schema. If the this.draft short-circuit in getValidator regressed, _draftFor classifies draft-07 to '7' without throwing — so the assertion still holds with a broken bypass.\n\nWhy existing coverage doesn't catch this. The one sibling test that touches the AJV bypass path, 'uses a caller-supplied engine immediately and never replaces it' in ajvLazyInit.test.ts, uses a $schema-less schema — with the _userAjv check removed, declaredDialect returns '2020-12', which dispatches to this.ajv, i.e. the same fake stored in _ajv — so that test passes too. After this PR the bypass short-circuits are therefore genuinely unpinned by any test.\n\nStep-by-step proof (AJV leg). (1) Delete the if (this._userAjv) return this.ajv; lines from _engineFor. (2) The test constructs new AjvJsonSchemaValidator(draft07) and calls getValidator(prefixItemsSchema(DRAFT_07_URI)). (3) _engineFor now calls declaredDialect, which classifies the draft-07 URI as 'draft-7' and returns this._ajvDraft7 ??= createDefaultAjvInstance(Draft7Ajv) — no throw. (4) That engine, like the caller's, ignores prefixItems under strict: false, so v(['x', 1]).valid === true. (5) The test passes with the bypass fully removed. Pre-PR, step (3) would have thrown unsupported dialect, failing the test — the discriminating power was lost by this PR's semantic change.\n\nImpact and fix. No runtime behavior is wrong — this is a mutation-coverage regression in tests whose names promise a contract they no longer verify. The PR itself shows awareness of the hazard: it moved the analogous rejection test in ajvLazyInit.test.ts (and the rejection it.each rows) from draft-07 to draft-04 for exactly this reason, but missed these two bypass tests. The fix is one line per test: probe with 'http://json-schema.org/draft-04/schema#' (still rejected by the default dispatch). A custom classic Ajv with validateSchema: false compiles a draft-04-stamped schema fine, and cfworker with an explicit {draft: '7'} skips dispatch entirely — so both constructions succeed only if the bypass really skips the dialect check, restoring the tests' discriminating power.
2 participants
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