Bundle automatic validator defaults in client/server shims#2088
Bundle automatic validator defaults in client/server shims#2088mattzcarey wants to merge 2 commits into
Conversation
🦋 Changeset detectedLatest commit: e8006c0 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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
@modelcontextprotocol/client
@modelcontextprotocol/server
@modelcontextprotocol/express
@modelcontextprotocol/fastify
@modelcontextprotocol/hono
@modelcontextprotocol/node
commit: |
mattzcarey
force-pushed
the
fix/server-cfworker-json-schema-shim
branch
from
54c18d7 to
ba08235
Compare
mattzcarey
changed the title
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
mattzcarey
force-pushed
the
fix/server-cfworker-json-schema-shim
branch
2 times, most recently
from
c7601a2 to
5db154c
Compare
mattzcarey
changed the title
mattzcarey
force-pushed
the
fix/server-cfworker-json-schema-shim
branch
from
5db154c to
78bd6c2
Compare
mattzcarey
force-pushed
the
fix/server-cfworker-json-schema-shim
branch
5 times, most recently
from
9c5c541 to
57c3a0b
Compare
| "types": "./dist/stdio.d.mts", | ||
| "import": "./dist/stdio.mjs" | ||
| }, | ||
| "./validators/cf-worker": { | ||
| "types": "./dist/validators/cfWorker.d.mts", | ||
| "import": "./dist/validators/cfWorker.mjs" | ||
| }, | ||
| "./_shims": { |
There was a problem hiding this comment.
🟡 nit: a few prose references to the removed validator paths survive — .changeset/cfworker-out-of-barrel.md:6 still says the validator is "reachable only via … the explicit @modelcontextprotocol/{server,client}/validators/cf-worker subpath … No public API change", packages/core/src/validators/ajvProvider.ts:36's @see still points at those same removed subpaths (this JSDoc is bundled into the published .d.mts), and .changeset/support-standard-json-schema.md:24-27 still shows import { fromJsonSchema, AjvJsonSchemaValidator } from '@modelcontextprotocol/core' + new AjvJsonSchemaValidator(), which is now a type-only export. Worth grepping for validators/cf-worker and updating these alongside the migration-doc rewrites so the next CHANGELOG and IDE hovers don't advertise paths this PR deletes.
Extended reasoning...
What the issue is
This PR removes the ./validators/cf-worker subpath from packages/{client,server}/package.json (and deletes both src/validators/cfWorker.ts files), and demotes AjvJsonSchemaValidator from a runtime re-export to export type in both core barrels. The PR updates the migration docs, the new changeset, and the JSDoc in packages/core/src/index.ts to match — but three pre-existing prose references to the old form survive untouched:
.changeset/cfworker-out-of-barrel.md:6— still reads: "The validator is now reachable only via the_shimsconditional (workerd/browser) and the explicit@modelcontextprotocol/{server,client}/validators/cf-workersubpath, so consumers that don't opt into it no longer ship that code. No public API change." This changeset is not in.changeset/pre.json's consumedchangesetsarray, so it is unreleased and will land in the same CHANGELOG version as this PR'sworkerd-shim-vendors-cfworker.md.packages/core/src/validators/ajvProvider.ts:36—@seeCfWorkerJsonSchemaValidatorfor an edge-runtime-compatible alternative (import from@modelcontextprotocol/server/validators/cf-workeror@modelcontextprotocol/client/validators/cf-worker). This file is not in the PR's changed-files list..changeset/support-standard-json-schema.md:24-27—import { fromJsonSchema, AjvJsonSchemaValidator } from '@modelcontextprotocol/core';followed bynew AjvJsonSchemaValidator(). Afterpackages/core/src/index.ts:22becomesexport type { AjvJsonSchemaValidator }, that example is a TS error ("cannot be used as a value because it was exported using 'export type'"). This changeset is inpre.json(already applied to an alpha CHANGELOG), but in changesets pre-mode the.mdpersists and is re-aggregated into the final 2.0.0 CHANGELOG onchangeset pre exit.
A repo-wide grep for validators/cf-worker confirms (1) and (2) are the only two surviving references to the removed subpath.
Why existing review feedback doesn't cover it
The earlier inline comment on packages/core/src/exports/public/index.ts:145 enumerated four doc sites that pointed at @modelcontextprotocol/core/... (docs/migration.md, docs/migration-SKILL.md, .changeset/workerd-shim-vendors-cfworker.md, and the JSDoc in packages/core/src/index.ts). The current revision reworked all four. None of the three locations above were on that list, so they were missed in the same cleanup pass — this is the REVIEW.md "Completeness" recurring catch ("when a PR replaces a pattern, grep the package for surviving instances of the old form").
Step-by-step proof
- This PR deletes
"./validators/cf-worker"frompackages/server/package.jsonexports + typesVersions and frompackages/client/package.json, removes bothsrc/validators/cfWorker.tsfiles, and drops them from bothtsdown.config.tsentry arrays. changesets versionruns on the next release.cfworker-out-of-barrel.mdis not inpre.json'schangesets[], so it is consumed alongsideworkerd-shim-vendors-cfworker.md. The generatedpackages/server/CHANGELOG.mdfor that version contains both entries: one says "reachable only via …/validators/cf-workersubpath … No public API change", the other says explicit validator subpaths are removed. A user reading the release notes gets directly contradictory guidance.tsdownbundlesajvProvider.ts's class JSDoc intodist/shimsNode.d.mts(it's re-exported asDefaultJsonSchemaValidator) and into the root.d.mts(type-only). A consumer hoversDefaultJsonSchemaValidatororAjvJsonSchemaValidatorin their IDE and sees@see … import from @modelcontextprotocol/server/validators/cf-worker. They tryimport { CfWorkerJsonSchemaValidator } from '@modelcontextprotocol/server/validators/cf-worker'→ERR_PACKAGE_PATH_NOT_EXPORTED.- On
changeset pre exit,support-standard-json-schema.mdis re-emitted into the stable 2.0.0 CHANGELOG. A user copies thefromJsonSchemaexample, gets'AjvJsonSchemaValidator' cannot be used as a value because it was exported using 'export type'(and@modelcontextprotocol/coreisprivate: true, so the import path was already wrong for external consumers even before this PR).
Impact
Doc-only; no runtime effect. The cfworker-out-of-barrel.md case is the most concrete because it produces self-contradicting published release notes in the very next version bump. The ajvProvider.ts @see surfaces in end-user IDE tooltips. The support-standard-json-schema.md example was already broken for external users (private package), so this PR just makes it slightly more broken; still worth fixing while validator docs are being touched.
How to fix
.changeset/cfworker-out-of-barrel.md: either delete it (its purpose is subsumed byworkerd-shim-vendors-cfworker.md) or rewrite line 6 to drop the/validators/cf-workersubpath reference and the "No public API change" claim.packages/core/src/validators/ajvProvider.ts:36: change the@seeto reference automatic runtime selection (matching the updatedpackages/core/src/index.tsJSDoc), or simply@see {@link CfWorkerJsonSchemaValidator}without an import path..changeset/support-standard-json-schema.md:23-28: rewrite the example toimport { fromJsonSchema } from '@modelcontextprotocol/server'and drop the explicitnew AjvJsonSchemaValidator()argument, matching the updateddocs/migration.mdguidance.
There was a problem hiding this comment.
Thanks — ajvProvider.ts:36's @see is now fixed. The two changeset files are still untouched, though:
.changeset/cfworker-out-of-barrel.md:6still says the validator is "reachable only via … the explicit@modelcontextprotocol/{server,client}/validators/cf-workersubpath … No public API change". This changeset is unreleased and will land in the same CHANGELOG version asworkerd-shim-vendors-cfworker.md, producing directly contradictory release notes. Easiest fix is to delete it (its purpose is now subsumed by the new changeset)..changeset/support-standard-json-schema.md:24-27still showsimport { fromJsonSchema, AjvJsonSchemaValidator } from '@modelcontextprotocol/core'+new AjvJsonSchemaValidator(), which is now a type-only export. Suggest rewriting toimport { fromJsonSchema } from '@modelcontextprotocol/server'and dropping the explicit validator arg, matching the updateddocs/migration.mdexample.
(Repo-wide grep for validators/cf-worker now matches only cfworker-out-of-barrel.md, confirming the ajvProvider.ts reference is gone.)
mattzcarey
force-pushed
the
fix/server-cfworker-json-schema-shim
branch
from
57c3a0b to
a2b954b
Compare
| test('should register tool with raw JSON Schema input', async () => { | ||
| const inputSchema = fromJsonSchema<{ name: string }>( | ||
| { type: 'object', properties: { name: { type: 'string' } }, required: ['name'] }, | ||
| validator | ||
| ); | ||
| const inputSchema = serverFromJsonSchema<{ name: string }>({ | ||
| type: 'object', | ||
| properties: { name: { type: 'string' } }, | ||
| required: ['name'] | ||
| }); | ||
|
|
||
| mcpServer.registerTool('greet', { inputSchema }, async ({ name }) => ({ | ||
| content: [{ type: 'text', text: `Hello, ${name}!` }] |
There was a problem hiding this comment.
🟡 nit: this rewrite makes describe('Raw JSON Schema via fromJsonSchema') and describe('fromJsonSchema with default validator (server wrapper)') exercise the same code path — both now call serverFromJsonSchema(...) with no explicit validator on the same {name:string} / {count:number} schemas, so the second block is a strict subset of the first (the first additionally asserts the tools/list inputSchema shape). The explicit-validator coverage that block 1 used to provide moved to the new packages/{client,server}/test/**/jsonSchemaValidatorOverride.test.ts files, so the simplest cleanup is to delete the second describe block.
Extended reasoning...
What changed
Before this PR, the two adjacent describe blocks in standardSchema.test.ts covered distinct axes:
describe('Raw JSON Schema via fromJsonSchema')built schemas via core'sfromJsonSchema(schema, new AjvJsonSchemaValidator())— i.e. an explicit validator instance passed by the caller.describe('fromJsonSchema with default validator (server wrapper)')built schemas viaserverFromJsonSchema(schema)with no validator argument — i.e. the server wrapper's runtime-selected default.
This PR removes AjvJsonSchemaValidator and core fromJsonSchema from the file's imports and rewrites block 1 to use serverFromJsonSchema with no validator argument (and renames its second test from 'should reject invalid input via AJV validation' → '…via default validation'). After the rewrite, both blocks call exactly the same function with the same default-validator semantics.
Side-by-side after the diff
| Block 1: "Raw JSON Schema via fromJsonSchema" | Block 2: "fromJsonSchema with default validator (server wrapper)" | |
|---|---|---|
| Test 1 schema | serverFromJsonSchema<{name:string}>({…name…}) |
serverFromJsonSchema<{name:string}>({…name…}) |
| Test 1 assertions | tools/list inputSchema shape + tools/call returns Hello, World! |
tools/call returns Hello, World! |
| Test 2 schema | serverFromJsonSchema({…count:number…}) |
serverFromJsonSchema({…count:number…}) |
| Test 2 assertions | count:'not a number' → isError + 'Input validation error' |
identical |
Block 2 is now a strict subset of block 1 — the only delta is block 1's extra tools/list toMatchObject assertion. The two "reject invalid input" tests are functionally identical.
Why nothing was actually lost (addressing the refutation)
One reviewer argued this isn't worth flagging because (a) the blocks aren't strictly identical and (b) the explicit-validator coverage didn't disappear. Both points are correct and are why this is a nit, not a defect:
- The rewrite was forced by this PR's own change —
AjvJsonSchemaValidatoris no longer a runtime export from@modelcontextprotocol/core's root barrel, sonew AjvJsonSchemaValidator()here would have required rewiring to the/validators/ajvsubpath (aselicitation.test.tsdoes in this same diff). - The explicit-override path that block 1 used to cover did move, not vanish: this PR adds
packages/server/test/server/jsonSchemaValidatorOverride.test.tsandpackages/client/test/client/jsonSchemaValidatorOverride.test.ts, each containing a'fromJsonSchema uses an explicitly supplied custom validator'test with aRecordingValidatorthat asserts the exact schema and value passed through. That's stronger coverage than the old AJV smoke test.
So no coverage regression — the only residue is that this file now contains two describe blocks doing the same thing, which is the kind of test-hygiene cleanup that's cheap to do while the file is already being touched.
Step-by-step proof
- Pre-PR line 8 imports
AjvJsonSchemaValidator, fromJsonSchemafrom core; block 1 opens withconst validator = new AjvJsonSchemaValidator()and callsfromJsonSchema(schema, validator). - Diff replaces line 8 with
import { InMemoryTransport } from '@modelcontextprotocol/core'(dropping both symbols), deletes thevalidatorconst, and rewrites bothfromJsonSchema(schema, validator)calls toserverFromJsonSchema({...}). serverFromJsonSchemawas already imported on line 9 and is what block 2 (lines ~431-467, untouched by this diff) already uses.- Result: block 1 test 1 ≡ block 2 test 1 minus the
tools/listassertion; block 1 test 2 ≡ block 2 test 2 (tool name'double'vs'double-default'is the only difference).
Suggested fix
Delete describe('fromJsonSchema with default validator (server wrapper)') (lines ~431-467) — its tests are now fully covered by the rewritten block above it. Alternatively, fold block 1's tools/list assertion into block 2 and delete block 1; either way one block goes.
| test('runtime shims vendor default validator backends instead of requiring consumers to install them', () => { | ||
| for (const shim of ['shimsNode.mjs', 'shimsWorkerd.mjs']) { | ||
| const entry = join(distDir, shim); | ||
| expect(readFileSync(entry, 'utf8')).not.toMatch(VALIDATOR_BACKEND_IMPORT); | ||
|
|
||
| for (const chunk of chunkImportsOf(entry)) { | ||
| expect({ chunk, content: readFileSync(chunk, 'utf8') }).not.toEqual( | ||
| expect.objectContaining({ content: expect.stringMatching(VALIDATOR_BACKEND_IMPORT) }) | ||
| ); | ||
| } | ||
| } | ||
| }); |
There was a problem hiding this comment.
🟡 nit (pre-existing): this regression test only covers .mjs, but the published .d.mts still bare-imports ajv — building packages/server emits dist/ajvProvider-*.d.mts with import { Ajv } from "ajv"; (for constructor(ajv?: Ajv)), transitively pulled in by dist/index.d.mts and dist/shimsNode.d.mts, and dist/types-*.d.mts likewise has import { JSONSchema } from "json-schema-typed";. Neither ajv nor json-schema-typed is in server/client dependencies/peerDependencies, so a skipLibCheck: false consumer who follows the new docs/migration-SKILL.md:524 instruction "Do not install ajv, ajv-formats, or @cfworker/json-schema" gets TS2307: Cannot find module 'ajv'. Either extend the scan to dist/*.d.mts and replace the Ajv ctor-arg type with a local structural type (matching what cfWorkerProvider already does — its .d.mts leaks no @cfworker/json-schema import), or add a skipLibCheck caveat to the migration-SKILL line. Same applies to packages/client/test/client/barrelClean.test.ts.
Extended reasoning...
What the issue is
This PR adds a regression test asserting that the runtime shim .mjs files (and their .mjs chunk imports) contain no bare from 'ajv'|'ajv-formats'|'@cfworker/json-schema' import. That correctly enforces the runtime half of the "users do not need to install validator packages" goal. But the published type declarations still leak a bare ajv import that this test does not cover, and this PR's new migration-doc line tells users not to install ajv.
Building packages/server at this PR's HEAD produces:
dist/ajvProvider-BD030R9d.d.mts:2→import { Ajv } from "ajv";(needed forconstructor(ajv?: Ajv)on line 39).dist/index.d.mts:3anddist/shimsNode.d.mts:1both import that chunk, so any consumer importing anything from@modelcontextprotocol/serverhas TypeScript load a.d.mtsthat doesimport { Ajv } from "ajv".dist/types-YaX0ZWcZ.d.mts:1→import { JSONSchema } from "json-schema-typed";.
Neither ajv nor json-schema-typed appears in packages/server/package.json dependencies or peerDependencies — ajv is added only to devDependencies in this PR, and json-schema-typed is only in private core's deps. By contrast, dist/cfWorkerProvider-*.d.mts leaks no @cfworker/json-schema import because its constructor uses a locally-declared CfWorkerSchemaDraft type — so the AJV side is asymmetrically worse and the fix pattern already exists in-tree.
Why existing code doesn't catch it
The new VALIDATOR_BACKEND_IMPORT check iterates only ['shimsNode.mjs', 'shimsWorkerd.mjs'], and chunkImportsOf() follows only ./*.mjs relative imports — .d.mts files are never visited. tsdown's noExternal bundles the runtime code but does not inline external type references in the DTS output, so the Ajv parameter type survives as a bare import.
Why this is worth a nit on this PR despite being pre-existing
The .d.mts leak itself is pre-existing — the parent commit 2c0c481 also emits from "ajv" in dist/index-*.d.mts — so it is not a regression. What this PR newly adds is docs/migration-SKILL.md:524: "Do not install ajv, ajv-formats, or @cfworker/json-schema; client/server bundle the runtime-selected defaults." That instruction, taken at face value by a consumer with "skipLibCheck": false, produces a compile error on the published artifacts. This PR also touches ajvProvider.ts directly (the @see line), so the file in question is in-diff. The new test gives a (mild) false sense that the "no validator install needed" goal is fully enforced — it covers runtime but not types.
This is not a duplicate of the open inline comments: #3241089399 covers stale prose referencing the removed /validators/cf-worker subpath, and #3241222796 covers stale @example blocks in core/src/index.ts. Neither mentions the .d.mts bare-import leak or the skipLibCheck interaction.
Step-by-step proof
- A consumer creates a fresh project with
"skipLibCheck": falseintsconfig.jsonand runsnpm install @modelcontextprotocol/server. - Following the new
docs/migration-SKILL.md:524guidance, they do not installajv,ajv-formats, or@cfworker/json-schema. - They write
import { McpServer } from '@modelcontextprotocol/server';and runtsc. - TypeScript resolves
node_modules/@modelcontextprotocol/server/dist/index.d.mts, which on line 3 doesimport … from './ajvProvider-BD030R9d.mjs';→ resolvesajvProvider-BD030R9d.d.mts. - Line 2 of that file is
import { Ajv } from "ajv";.ajvis not innode_modules(onlyzodis a runtime dependency of@modelcontextprotocol/server). tscfails witherror TS2307: Cannot find module 'ajv' or its corresponding type declarations.(and likewiseTS2307forjson-schema-typedviatypes-*.d.mts).- The new
barrelClean.test.tsregression test passes throughout because it never reads*.d.mts.
Impact
Low — most TypeScript projects set skipLibCheck: true (it's the tsc --init default), so the failure mode is narrow. Runtime behavior is correct: the bundled .mjs chunks contain the AJV/cfworker code and no consumer install is needed to run. The concrete harm is doc-vs-artifact contradiction for the skipLibCheck:false minority, plus a test that enforces only half of the stated goal. The json-schema-typed half is clearly out-of-scope for this PR; mentioned for completeness.
How to fix
Either of:
- Close the leak — in
packages/core/src/validators/ajvProvider.ts, replace theconstructor(ajv?: Ajv)parameter type with a locally-declared structural type (orunknown/an internal interface), mirroringcfWorkerProvider.ts'sCfWorkerSchemaDraftapproach so the emitted.d.mtsno longer needsimport { Ajv } from "ajv". Optionally extend this test (and the client twin) to also scandist/*.d.mtsforVALIDATOR_BACKEND_IMPORTso the invariant is enforced for types as well as runtime. - Soften the doc — add a brief caveat to
docs/migration-SKILL.md:524(and the correspondingdocs/migration.mdparagraph) noting that projects withskipLibCheck: falsemay still needajvinstalled for type resolution until the declaration leak is closed.
| Make validator backends symmetrical in core and bundle automatic defaults in client/server runtime shims. | ||
|
|
||
| Core no longer re-exports concrete validator providers as runtime values from the root/public barrels. AJV/AJV formats and `@cfworker/json-schema` are optional peer backends behind explicit core validator provider subpaths, used internally by client/server shims. | ||
|
|
||
| Client/server continue to select defaults automatically: Node shims use AJV, while browser/workerd shims use `@cfworker/json-schema`. Those backends are bundled into the shim chunks that select them, so users do not need to install validator packages or import explicit validators for default behavior. Advanced users can still pass their own `jsonSchemaValidator` implementation. |
There was a problem hiding this comment.
🟡 nit: this changeset describes the core barrel change and the shim bundling, but never says that the ./validators/cf-worker subpath export was removed from @modelcontextprotocol/{client,server} — line 11's "do not need to … import explicit validators" reads as a convenience, not a removal. Worth one explicit sentence here, especially since the open comment on packages/server/package.json:31 suggests deleting .changeset/cfworker-out-of-barrel.md, which would leave this as the only changeset covering the subpath and the removal entirely undocumented in the generated CHANGELOG.
Extended reasoning...
What the issue is
This PR removes a public subpath export from two published packages: ./validators/cf-worker is deleted from the exports and typesVersions maps in packages/{client,server}/package.json, both src/validators/cfWorker.ts re-export files are deleted, and the entry is dropped from both tsdown.config.ts arrays. The previously-documented pattern import { CfWorkerJsonSchemaValidator } from '@modelcontextprotocol/server/validators/cf-worker' now resolves to ERR_PACKAGE_PATH_NOT_EXPORTED.
The PR's own changeset, .changeset/workerd-shim-vendors-cfworker.md, describes two things: (a) core no longer re-exporting concrete validator providers as runtime values from its barrels, and (b) client/server bundling default backends into their shim chunks. Line 11 says "users do not need to install validator packages or import explicit validators for default behavior" — but that frames explicit imports as no longer necessary, not as no longer possible. Nowhere does the changeset state that the ./validators/cf-worker subpath was removed from client/server.
Why this isn't covered by existing comments
The still-open inline comment on packages/server/package.json:31 is about other prose that still references the removed subpath (.changeset/cfworker-out-of-barrel.md, ajvProvider.ts:36's @see, .changeset/support-standard-json-schema.md). This finding is the inverse: the PR's own new changeset fails to announce the removal it introduces.
The two interact: that open comment's first suggested fix is "delete cfworker-out-of-barrel.md (its purpose is subsumed by workerd-shim-vendors-cfworker.md)". If the author follows that suggestion as-is, workerd-shim-vendors-cfworker.md becomes the only changeset touching this area — and it doesn't mention the subpath removal, so the generated CHANGELOG would contain no record that @modelcontextprotocol/{client,server}/validators/cf-worker ever went away.
The resolved comment on exports/public/index.ts:145 did raise the patch-vs-breaking concern, but for the earlier export type change. The author's resolution expanded the breaking surface (it removed the subpath entirely instead of adding a parallel one) without revisiting the changeset prose. The bump-level concern itself is muted by 2.0.0-alpha.2 pre-release status, so the actionable residue is just the missing prose.
Step-by-step proof
- A consumer on
@modelcontextprotocol/server@2.0.0-alpha.2hasimport { CfWorkerJsonSchemaValidator } from '@modelcontextprotocol/server/validators/cf-worker'— the exact pattern the pre-PRdocs/migration.mdanddocs/migration-SKILL.mddocumented. - They upgrade to the next alpha containing this PR.
packages/server/package.jsonno longer has"./validators/cf-worker"inexports; Node throwsERR_PACKAGE_PATH_NOT_EXPORTED. - They open the generated
packages/server/CHANGELOG.mdfor that version. The entry fromworkerd-shim-vendors-cfworker.mdreads: "Client/server continue to select defaults automatically … users do not need to install validator packages or import explicit validators for default behavior. Advanced users can still pass their ownjsonSchemaValidatorimplementation." - Nothing in that text says the subpath was removed. The consumer has to infer the API removal from a sentence about defaults, or grep the diff.
- If
.changeset/cfworker-out-of-barrel.mdis also deleted per the open comment's suggestion, there is no CHANGELOG entry mentioning/validators/cf-workerat all.
Impact
Doc-only; no runtime effect. The PR description and updated migration guides do call this out ("Public surface: explicit concrete validator subpaths are removed from client/server"), so the gap is specifically in the changeset → CHANGELOG path. Filed as a nit given the alpha pre-release context and that three related doc nits are already open — but it's a one-line addition while the changeset is being touched anyway.
How to fix
Add one sentence to .changeset/workerd-shim-vendors-cfworker.md, e.g. after line 11:
The explicit
./validators/cf-workersubpath export has been removed from@modelcontextprotocol/clientand@modelcontextprotocol/server; rely on automatic runtime selection or pass a customjsonSchemaValidator.
(If .changeset/cfworker-out-of-barrel.md is kept and rewritten per the other open comment instead of deleted, that would also cover it — either location works as long as one changeset states the removal.)
After PR #2088, end users can no longer import or construct AjvJsonSchemaValidator / CfWorkerJsonSchemaValidator — client/server bundle them via the runtime shim and expose only the jsonSchemaValidator interface as the public extension point. - Strip impossible `new AjvJsonSchemaValidator()` / `new CfWorkerJsonSchemaValidator()` snippets from JSDoc on the validation module, ajvProvider, cfWorkerProvider, and fromJsonSchema; mark the provider classes @internal. - fromJsonSchema example now declares `validator: jsonSchemaValidator` and notes that consumers importing from server/client omit the second arg. - Drop the type-only re-exports of AjvJsonSchemaValidator, CfWorkerJsonSchemaValidator, and CfWorkerSchemaDraft from core/public — with the runtime classes unreachable, a type-only handle is dead surface. - Reword the migration docs to describe backends by name (AJV / @cfworker/json-schema) instead of the now-internal class names.
After PR #2088, end users can no longer import or construct AjvJsonSchemaValidator / CfWorkerJsonSchemaValidator — client/server bundle them via the runtime shim and expose only the jsonSchemaValidator interface as the public extension point. - Strip impossible `new AjvJsonSchemaValidator()` / `new CfWorkerJsonSchemaValidator()` snippets from JSDoc on the validation module, ajvProvider, cfWorkerProvider, and fromJsonSchema; mark the provider classes @internal. - fromJsonSchema example now declares `validator: jsonSchemaValidator` and notes that consumers importing from server/client omit the second arg. - Drop the type-only re-exports of AjvJsonSchemaValidator, CfWorkerJsonSchemaValidator, and CfWorkerSchemaDraft from core/public — with the runtime classes unreachable, a type-only handle is dead surface. - Reword the migration docs to describe backends by name (AJV / @cfworker/json-schema) instead of the now-internal class names.
| * requests against the requested schema. | ||
| * | ||
| * @default {@linkcode DefaultJsonSchemaValidator} ({@linkcode index.AjvJsonSchemaValidator | AjvJsonSchemaValidator} on Node.js, `CfWorkerJsonSchemaValidator` on Cloudflare Workers) | ||
| * @default Runtime-selected validator (`AjvJsonSchemaValidator` on Node.js, `CfWorkerJsonSchemaValidator` on browser/workerd runtimes) |
There was a problem hiding this comment.
🟡 nit: commit 51f42cd reworded user-facing prose to "describe backends by name (AJV / @cfworker/json-schema) instead of the now-internal class names", but skipped the two @default lines this PR's earlier commit added — this one and the identical packages/client/src/client/client.ts:164 still say "AjvJsonSchemaValidator on Node.js, CfWorkerJsonSchemaValidator on browser/workerd". Both surface verbatim in the published .d.mts IDE hover for {Server,Client}Options.jsonSchemaValidator, naming classes the same commit just marked @internal and removed from core/public. Suggest matching the migration-doc phrasing, e.g. @default Runtime-selected validator (AJV-backed on Node.js, @cfworker/json-schema-backed on browser/workerd runtimes).
Extended reasoning...
What the issue is
Commit 51f42cd (the second commit in this PR) is an explicit doc-cleanup pass whose message says: "Reword the migration docs to describe backends by name (AJV / @cfworker/json-schema) instead of the now-internal class names." It applies that rewording to docs/migration.md:903-904 ("Node.js → AjvJsonSchemaValidator" → "Node.js → AJV"), docs/migration-SKILL.md:503-504, the @module validation JSDoc in packages/core/src/index.ts, the changeset, and the class JSDoc in both ajvProvider.ts and cfWorkerProvider.ts (where it also adds @internal and removes the classes from core/public entirely — "not even as types" per the changeset).
But git show --stat 51f42cd confirms it does not touch packages/server/src/server/server.ts or packages/client/src/client/client.ts. Those two files were last modified by this PR's earlier commit a2b954b, which rewrote both @default lines to:
@default Runtime-selected validator (AjvJsonSchemaValidatoron Node.js,CfWorkerJsonSchemaValidatoron browser/workerd runtimes)
So the cleanup commit established a convention (refer to backends by library name, not by now-@internal class name) and applied it everywhere except two lines this same PR had just added. This is the REVIEW.md "Completeness" recurring catch — when a PR replaces a pattern, two surviving instances of the old form remain in lines the PR itself introduced.
Why existing comments don't cover it
None of the open inline comments mention the ServerOptions/ClientOptions @default JSDoc:
- #3241089399 / #3241246611 on
packages/server/package.json:31cover.changeset/cfworker-out-of-barrel.md,.changeset/support-standard-json-schema.md, and the (now-fixed)ajvProvider.ts:36@see. - #3241222796 on
core/src/index.tscovered the (now-fixed)@exampleblocks below the@module validationprose. - #3241478258 on the changeset covers the missing subpath-removal announcement.
- #3241478255 on
barrelClean.test.tscovers the.d.mtsajvimport leak.
The two @default lines contain neither validators/cf-worker nor an import path, so the greps suggested in those comments would not surface them.
Why it matters (and why it's only a nit)
These @default lines surface verbatim in the published dist/index.d.mts IDE hover for ServerOptions.jsonSchemaValidator / ClientOptions.jsonSchemaValidator — the primary user-facing override option this PR is documenting. After this PR, AjvJsonSchemaValidator and CfWorkerJsonSchemaValidator are @internal, removed from core/public (exports/public/index.ts:144-146 now has only a comment where the exports used to be), and per the new ajvProvider.ts JSDoc "cannot [be] import[ed] from @modelcontextprotocol/client or @modelcontextprotocol/server". A user hovering the option therefore sees class names they cannot import, reference, or find documented anywhere.
The counter-argument is that these are plain backticks (descriptive prose, not {@link} references), so nothing is broken — and naming the concrete implementation in a @default tag is arguably useful for stack-trace recognition. That's fair, and is why this is at the low end of nit-worthiness. But the analogous purely-descriptive prose in docs/migration-SKILL.md:503-504 was changed by 51f42cd for exactly this reason, so these two lines are a straightforward miss in the same pass rather than a deliberate exception.
Step-by-step proof
- Commit
a2b954b(this PR) rewritespackages/server/src/server/server.ts:86andpackages/client/src/client/client.ts:164to@default Runtime-selected validator (AjvJsonSchemaValidatoron Node.js,CfWorkerJsonSchemaValidatoron browser/workerd runtimes). - Commit
51f42cd(this PR) marks both classes@internal, drops them fromcore/public, and rewrites every other user-facing reference from class names to backend names (AJV/@cfworker/json-schema). Its file list does not includeserver.tsorclient.ts. tsdownbundles theServerOptions/ClientOptionsJSDoc intopackages/{server,client}/dist/index.d.mts. A consumer hoversjsonSchemaValidatorin their IDE and sees@default … AjvJsonSchemaValidator … CfWorkerJsonSchemaValidator ….- They search the package surface for either name:
@modelcontextprotocol/serverexports neither (root barrel re-exportscore/public, which dropped them; the/validators/cf-workersubpath is removed). The migration guide they consult says "AJV" / "@cfworker/json-schema", not the class names. Mild dead-end.
Impact
Doc-only; no runtime effect. Two-line fix in two files already in this PR's diff, cheap to fold into the same cleanup pass that addresses the other open doc nits.
How to fix
Reword both @default lines to match the convention 51f42cd applied elsewhere, e.g.:
* @default Runtime-selected validator (AJV-backed on Node.js, `@cfworker/json-schema`-backed on browser/workerd runtimes)at packages/server/src/server/server.ts:86 and packages/client/src/client/client.ts:164.
|
|
||
| Client/server continue to select defaults automatically: Node shims use AJV, while browser/workerd shims use `@cfworker/json-schema`. Those backends are bundled into the shim chunks that select them, so users do not need to install validator packages or import explicit validators for default behavior. Advanced users can still pass their own `jsonSchemaValidator` implementation. | ||
|
|
||
| `AjvJsonSchemaValidator` and `CfWorkerJsonSchemaValidator` are now `@internal` and no longer surface from `@modelcontextprotocol/core/public` (not even as types). The `jsonSchemaValidator` interface remains the public extension point for custom validators. Example JSDoc snippets no longer demonstrate direct validator instantiation. |
There was a problem hiding this comment.
🟡 nit: this new sentence (and line 9's "root/public barrels") will be emitted into the published @modelcontextprotocol/{client,server} CHANGELOGs, but readers of those CHANGELOGs have never seen @modelcontextprotocol/core/public — core is private: true and the migration docs explicitly tell users not to import from it. Since this paragraph is already being reworked per the open comment on line 11, suggest phrasing it from the consumer's vantage point, e.g. "AjvJsonSchemaValidator and CfWorkerJsonSchemaValidator are now @internal and no longer exported from @modelcontextprotocol/client or @modelcontextprotocol/server (not even as types)".
Extended reasoning...
What the issue is
Line 13 of .changeset/workerd-shim-vendors-cfworker.md was added in head commit 51f42cd (the docs-alignment pass) and reads:
AjvJsonSchemaValidatorandCfWorkerJsonSchemaValidatorare now@internaland no longer surface from@modelcontextprotocol/core/public(not even as types).
The changeset front-matter lists '@modelcontextprotocol/client': patch and '@modelcontextprotocol/server': patch, so changesets will copy this prose verbatim into packages/client/CHANGELOG.md and packages/server/CHANGELOG.md on the next version bump. But @modelcontextprotocol/core/public is an internal curated-barrel subpath: packages/core/package.json:3 has "private": true, CLAUDE.md describes core as "only consumed by sibling packages within the monorepo", and docs/migration.md (updated in this same PR) tells users "Do not import from @modelcontextprotocol/core directly — it is an internal package." An external consumer reading the client or server CHANGELOG has no frame of reference for "core/public" — what they can observe is that these symbols are no longer exported from the package they actually installed. Line 9 has the same issue with "root/public barrels".
Why this isn't covered by existing open comments
The open inline comment #3241478258 on line 11 is about missing content — the changeset never states that the ./validators/cf-worker subpath was removed from client/server. This finding is about present content using internal terminology. Crucially, line 13 was added in commit 51f42cd after #3241478258 was posted (2026-05-14T12:57:53Z), so the author has not yet received feedback on it. The two issues live in adjacent sentences and combine naturally into one rewrite, which is why the suggested fix is framed as a fold-in rather than a standalone demand.
Addressing the counter-arguments
One reviewer argued this is subjective style, that line 9 already has the same jargon and survived review, and that with 5+ open comments a separate wording nit is noise. Taking each:
- "Line 9 already says 'root/public barrels' and survived review" — line 9 has the same problem and is named in the fix. It "surviving" earlier rounds isn't an endorsement: the changeset has been substantially rewritten across this PR's revisions (it started as a server-only shim note), and the author is currently editing this exact paragraph in response to #3241478258. Now is the cheapest moment to flag both lines.
- "Should be a sub-point of the line-11 thread, not a separate comment" — fair, and the comment is written to read that way ("since this paragraph is already being reworked per the open comment on line 11…"). But #3241478258 cannot literally cover line 13 because line 13 didn't exist when it was posted; anchoring on line 13 makes the new prose the subject.
- "The statement is accurate, just unfamiliar terminology" — agreed it's accurate from the monorepo's perspective, which is exactly the mismatch: the CHANGELOG audience is outside the monorepo. The user-observable fact ("no longer exported from
@modelcontextprotocol/{client,server}") is equally accurate and is what a reader can verify against their ownnode_modules. - "Not egregious" — agreed; hence nit, not a blocker.
Step-by-step proof
- Front-matter at
.changeset/workerd-shim-vendors-cfworker.md:1-5lists@modelcontextprotocol/clientand@modelcontextprotocol/server. Onchangeset version, the body (lines 7-13) is appended to bothpackages/client/CHANGELOG.mdandpackages/server/CHANGELOG.md. - A consumer on
@modelcontextprotocol/server@2.0.0-alpha.2who previously wroteimport { AjvJsonSchemaValidator } from '@modelcontextprotocol/server'upgrades and gets a TS error. - They open
node_modules/@modelcontextprotocol/server/CHANGELOG.mdand read: "…no longer surface from@modelcontextprotocol/core/public…". - They have never imported from
@modelcontextprotocol/core/public.npm ls @modelcontextprotocol/coreshows nothing (it'sprivate: true, bundled into server, not a dependency). The migration guide they were pointed at says not to import from core. The CHANGELOG line doesn't tell them what changed in the package they installed. - The consumer-observable equivalent — "no longer exported from
@modelcontextprotocol/clientor@modelcontextprotocol/server" — is whatpackages/core/src/validators/ajvProvider.ts:28-29andcfWorkerProvider.ts:26-27(both rewritten in this PR) already say in their@internalJSDoc.
Impact
Doc-only; no runtime effect. Filed as a nit because (a) alpha pre-release, (b) the statement is technically true, just framed from the wrong vantage point, and (c) the fix is a one-clause reword in a file that's already being edited per #3241478258.
How to fix
Reword line 13 (and optionally tighten line 9) to speak in terms of the published packages, e.g.:
AjvJsonSchemaValidatorandCfWorkerJsonSchemaValidatorare now@internaland no longer exported from@modelcontextprotocol/clientor@modelcontextprotocol/server(not even as types). ThejsonSchemaValidatorinterface remains the public extension point for custom validators.
This also dovetails with #3241478258's request to state the ./validators/cf-worker subpath removal — both edits land in the same paragraph.
mattzcarey
force-pushed
the
fix/server-cfworker-json-schema-shim
branch
from
51f42cd to
e8006c0
Compare
| ); | ||
| } | ||
| } | ||
| }); |
There was a problem hiding this comment.
🟡 Follow-up: after e8006c0 dropped AjvJsonSchemaValidator/CfWorkerJsonSchemaValidator from core/public entirely, rebuilt dist/index.d.mts no longer imports the ajvProvider-*.d.mts chunk — the from "ajv" leak is now reachable only via dist/shimsNode.d.mts (the underscore-prefixed internal _shims subpath, which the rewritten migration docs no longer tell users to import). So the headline failure mode above ("a skipLibCheck:false consumer who imports McpServer from the root entry gets TS2307: Cannot find module 'ajv'") no longer reproduces, and the suggested Ajv ctor-arg rewrite would now benefit essentially nobody. The only surviving root-entry .d.mts leak is json-schema-typed, which this comment already calls out-of-scope — so this thread can probably be resolved (or narrowed to json-schema-typed). Same applies to packages/client.
Extended reasoning...
What changed since this comment was posted
Inline comment #3241478255 (posted 2026-05-14T12:57) states that "dist/index.d.mts:3 and dist/shimsNode.d.mts:1 both import that chunk" (the ajvProvider-*.d.mts chunk containing import { Ajv } from "ajv"), and concludes that a skipLibCheck:false consumer who follows docs/migration-SKILL.md:524 and imports McpServer from @modelcontextprotocol/server will hit TS2307: Cannot find module 'ajv'. That was accurate at the revision the comment was filed against.
HEAD commit e8006c0 (the rebased 51f42cd referenced in the 15:09 comments) subsequently dropped AjvJsonSchemaValidator, CfWorkerJsonSchemaValidator, and CfWorkerSchemaDraft from core/public entirely — not even as types (packages/core/src/exports/public/index.ts:144-146 now contains only an explanatory comment where the exports were). packages/server/src/index.ts:52 re-exports core/public, and DefaultJsonSchemaValidator is consumed in server.ts only as a runtime value (never in an exported type signature), so tsdown's DTS bundler no longer has a reason to pull the ajvProvider chunk into the root entry's .d.mts graph.
Verified at HEAD
Rebuilding packages/server at e8006c0 and grepping the emitted declarations:
$ grep -l 'ajvProvider\|from "ajv"' packages/server/dist/*.d.mts
packages/server/dist/ajvProvider-ClUAHE10.d.mts
packages/server/dist/shimsNode.d.mts
dist/index.d.mts now imports only ./index-dAsK-dK1.mjs (whose .d.mts imports only zod/v4), ./types-YaX0ZWcZ.mjs (whose .d.mts imports json-schema-typed), and zod/v4. The ajvProvider-*.d.mts chunk is reachable only via dist/shimsNode.d.mts, which backs the ./_shims subpath. packages/client/dist shows the same pattern.
Why the open comment is now mostly stale
- Headline failure no longer reproduces. TypeScript only loads
.d.tsfiles reachable from the resolved entry. SinceajvProvider-*.d.mtsis unreachable fromindex.d.mts, askipLibCheck:falseconsumer who writesimport { McpServer } from '@modelcontextprotocol/server'and follows the "do not installajv" instruction atdocs/migration-SKILL.md:524no longer hitsTS2307forajv. - The suggested fix is now near-zero-value. The comment recommends replacing the
constructor(ajv?: Ajv)parameter type with a local structural type so the emitted.d.mtsdoesn't needimport { Ajv } from "ajv". Aftere8006c0that rewrite would benefit only consumers who import from@modelcontextprotocol/server/_shims— an underscore-prefixed internal export that this PR's migration-doc rewrite explicitly removed from the "Access validators explicitly" section (the oldimport { DefaultJsonSchemaValidator } from '@modelcontextprotocol/server/_shims'line is gone). - The surviving root-entry leak is
json-schema-typed(viatypes-*.d.mts→JsonSchemaType = JSONSchema.Interface), which #3241478255 already mentions but explicitly labels "clearly out-of-scope for this PR".
Step-by-step proof
- At the revision #3241478255 was posted against,
core/publicexportedAjvJsonSchemaValidator(as a type) andCfWorkerSchemaDraft. Because the server root barrel re-exportscore/public, tsdown emitted those types intodist/index.d.mts's graph, which transitively pulled inajvProvider-*.d.mtsand its bareimport { Ajv } from "ajv". - Commit
e8006c0replaces those exports with a comment (exports/public/index.ts:144-146). Nothing in the server root barrel now referencesajvProvider.tsat the type level. - Rebuild:
dist/index.d.mtslines 1-3 import./index-dAsK-dK1.mjs,./types-YaX0ZWcZ.mjs,zod/v4. None of those referenceajvProvider. - A consumer with
"skipLibCheck": falserunsnpm i @modelcontextprotocol/server(noajv), writesimport { McpServer } from '@modelcontextprotocol/server', and runstsc. TypeScript resolvesdist/index.d.mts→index-dAsK-dK1.d.mts→types-YaX0ZWcZ.d.mts.ajvProvider-ClUAHE10.d.mtsis never visited; noTS2307forajv. (They would still hitTS2307forjson-schema-typed— the out-of-scope half.) - Only a consumer who explicitly writes
import … from '@modelcontextprotocol/server/_shims'would resolveshimsNode.d.mts→ajvProvider-*.d.mts→from "ajv". The migration docs no longer document that import.
Impact and suggested resolution
This is review-thread bookkeeping in the same vein as the existing #3241246611 reply ("Thanks — ajvProvider.ts:36's @see is now fixed. The two changeset files are still untouched…"). It prevents the author from doing the now-unnecessary Ajv ctor-arg-type rewrite in response to a partially-stale finding. The thread can be resolved (its primary user-facing concern was addressed by e8006c0), or narrowed to json-schema-typed if the author wants to track that residual leak — but per the original comment that's out-of-scope here. Filed as a nit since it's meta-feedback about review-comment staleness rather than a code defect.
2 participants
Summary
@cfworker/json-schemafor browser/workerd defaults.validators/cf-workersubpath exports. Validator selection is automatic for normal users; advanced users can provide their ownjsonSchemaValidatorimplementation.AjvJsonSchemaValidatorandCfWorkerJsonSchemaValidator@internal; drop their type-only re-exports fromcore/publicso the runtime classes and their TypeScript type handles are both unreachable from@modelcontextprotocol/client/@modelcontextprotocol/server..examples.tsfiles and JSDoc snippets inpackages/core/so they no longer demonstrate end-user code (new AjvJsonSchemaValidator(),new CfWorkerJsonSchemaValidator()) that the new surface makes impossible.fromJsonSchema's example now uses a declaredjsonSchemaValidatorand notes that consumers importing from server/client omit the second arg.ajv,ajv-formats, or@cfworker/json-schemafor consumers to resolve.@cfworker/json-schema) rather than the now-internal class names.Why
Client and server choose runtime defaults via conditional
_shims:@cfworker/json-schema-backed validatorBecause client/server make those default choices, consumers should not need to know about or install validator implementation dependencies. Those backends are now bundled into the shim chunks that select them. The implementation classes are also no longer part of the public surface — neither as runtime constructors nor as TypeScript types — because exposing a handle to a constructor consumers can't reach is purely confusing.
Impact
jsonSchemaValidator: myCustomValidatorwith their own implementation of thejsonSchemaValidatorinterface.@modelcontextprotocol/core,ajv,ajv-formats, or@cfworker/json-schema; those implementations are bundled where client/server need them.@internal, andcore/publicno longer re-exports them as types. The supported customisation path is thejsonSchemaValidatorinterface (still exported fromcore/publicand re-exported by client/server).Follow-up for codemod reviewers
If this validator packaging change ships, the v1-to-v2 codemod should remove redundant manual SDK validator imports/configuration for the built-in AJV and Cloudflare Workers validators. Those validators are no longer exported from client/server (not even as types), and normal migrations should rely on automatic runtime defaults instead.
Examples the codemod should simplify/remove when they refer to the SDK built-ins:
For custom user validators, the codemod should preserve the override:
Reviewer note: this PR intentionally makes built-in validator selection automatic for client/server users; codemod output should not teach users to import SDK concrete validators for the default case.