refactor: move the schema source modules into @modelcontextprotocol/core#2477
refactor: move the schema source modules into @modelcontextprotocol/core#2477felixweinberger wants to merge 7 commits into
Conversation
The MCP spec schemas (types/schemas.ts), the OAuth/OpenID schemas (shared/auth.ts), the protocol constants (types/constants.ts), and the three JSON value types move verbatim from core-internal into packages/core/src — core now owns the schema sources instead of bundling them out of core-internal through build-only subpath aliases. Mechanism: - core's curated root entry re-exports the same public surface as before (unchanged 172 exports), now from its own local modules. - A new ./internal subpath re-exports the moved modules wholesale for the sibling SDK packages (internal helper schemas, auth types, constants, JSON types — names that are deliberately not public on the root entry). - core-internal keeps the old module paths as one-to-one named re-export shims, so no importer or test changes anywhere. - client/server/server-legacy declare @modelcontextprotocol/core as a real dependency; their bundles keep @modelcontextprotocol/core/internal as an external runtime import (explicit tsdown external entry) instead of carrying their own bundled schema copies. - The build-only core-internal/schemas + core-internal/auth tsconfig/tsdown aliases are deleted everywhere; the per-package tsconfigs gain a single source-first alias for @modelcontextprotocol/core/internal so typecheck and vitest stay build-order independent. Layering stays acyclic: core depends only on zod; core-internal depends on core; the wire era modules are untouched (their frozen copies and their runtime constants imports resolve through the shims unchanged).
…home Test-only adaptations to the schema-source move; each guard keeps pinning the same invariant, only pointed at the new canonical location: - packageTopologyPins: pin @modelcontextprotocol/core's export map as ['.', './internal'] — the internal seam is deliberate, not public API. - coreSchemas: read the spec-schema source from core's own src/schemas.ts (the auth group still reads core-internal's authSchemas registry). - wireOnlyHiding: read the @deprecated task-schema and constants sources from packages/core/src (the old paths are now re-export shims with no doc comments to scan). - codemod authSchemaNames: core's barrel now ends its auth block with "} from './auth'" instead of the deleted build-only alias specifier. - cloudflareWorkers: generalize packServerPackage to packWorkspacePackage and install the workspace core tarball alongside the server tarball — the packed server resolves @modelcontextprotocol/core/internal at runtime, which the registry copy of core does not carry yet.
The schema sources moved into @modelcontextprotocol/core with the sibling packages resolving @modelcontextprotocol/core/internal at runtime. That seam can rot in ways no existing test catches; this locks it down: - Exact sibling pins: client/server/server-legacy (and core-internal, for consistency) depend on core via workspace:* so pnpm publishes an exact version pin instead of a caret range. The ./internal surface is only guaranteed for the core version each sibling was built against, so a caret range would let installs mix skewed versions. - Changesets fixed group for core + client + server + server-legacy, keeping the pinned versions releasable in lockstep. - ./internal is labeled as an SDK-internal contract in its header, with @internal JSDoc on the re-exports (source-level only: the dts bundler flattens the re-exports and drops statement-level comments, so the built declarations do not carry the tag). - A client boundary test parses the built dists in both directions: every name the client bundle imports or re-exports from core must resolve against core's built entry export lists (skew), and sentinel schemas that exist only in core's modules must never appear as definitions in the client bundle (re-inlining via lost external config or eager aliases). - A core-internal shim-purity test pins the old schema/auth/constants module paths as pure re-export forwards: no zod import, no local definitions, imports only from @modelcontextprotocol/core/internal — so new schemas can't accrete at the old paths and silently miss core's published entries. The shim headers now state that rule, and the type-only JSON value re-export in types.ts is pinned as erasable. No-Verification-Needed: tests, manifest pins, release config, and comments only — no runtime surface change
No-Verification-Needed: release-metadata-only change
🦋 Changeset detectedLatest commit: 68688c9 The changes in this PR will be included in the next version bump. This PR includes changesets to release 9 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: |
barrelClean and coreBoundary each rebuilt a missing dist from their own beforeAll; vitest runs test files in separate workers, so a cold checkout raced two pnpm builds in the same package and the clean step deleted files under the other worker's reader. Route both through a shared single-flight helper: an atomic mkdir lock with one canonical sentinel set per package, a stale-lock steal for holders that died without cleanup, an async build bounded by a timeout so the worker's event loop and vitest's hook timer stay live, and only EEXIST treated as contention. No-Verification-Needed: test-only change, no runtime surface
|
@claude review |
The builds take seconds; the previous bounds were sized by stacking worst cases on worst cases. Keep the ordering invariant (build kill < waiter deadline < stale-lock steal < hook timeout) at realistic magnitudes: 60s/90s/120s with 180s and 240s hooks. No-Verification-Needed: test-only timeout constants
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
Reopening as a fresh PR to retrigger the automated review. |
… blind spots - Add typesVersions for the ./internal subpath so moduleResolution:node10 consumers resolve its declarations (exports maps are invisible there) - Update wire-schemas and packages docs: core now arrives transitively as the shared runtime schema graph of client/server/server-legacy - Boundary test: scan dist recursively (validators/ chunks were excluded) and reject bare side-effect imports of core, which have no from-clause
|
@claude review |
| // Moved: the spec Zod schemas now live in @modelcontextprotocol/core (packages/core/src/schemas.ts). | ||
| // This module re-exports them one-to-one so every existing import path keeps working; a name | ||
| // added to core/src/schemas.ts must be added here too (specTypeSchema.ts imports through this | ||
| // module, so a missing name fails typecheck loudly). | ||
| // | ||
| // Add new schemas in packages/core/src/schemas.ts, never here — this file only forwards, and |
There was a problem hiding this comment.
🟡 The frozen-wire-schema guards are still keyed only to the old module path: the eslint no-restricted-imports group in packages/core-internal/eslint.config.mjs (['**/types/schemas', '**/types/schemas.js']) and the RUNTIME_SCHEMAS_IMPORT regex in packages/core-internal/test/wire/layeringInvariants.test.ts:50 both require the literal substring types/schemas, so a src/wire/rev*/ file importing the same mutable schemas via the PR's new @modelcontextprotocol/core/internal seam would lint clean, pass the invariant test, typecheck, and resolve at runtime. No wire/rev file does this today, so this is a guard coverage gap rather than a live bug — extending both patterns to also match @modelcontextprotocol/core and @modelcontextprotocol/core/internal closes it.
Extended reasoning...
What the gap is
The repo enforces a frozen-wire-schema invariant — "A change to types/schemas.ts must never alter what a codec emits or accepts on the wire" (the comment above the rule in packages/core-internal/eslint.config.mjs:34-37) — in two places, and both are keyed to the schemas' old home only:
- eslint (
packages/core-internal/eslint.config.mjs:38-55): forsrc/wire/rev*/**/*.ts,@typescript-eslint/no-restricted-importsrestricts only the group['**/types/schemas', '**/types/schemas.js']. - layeringInvariants (
packages/core-internal/test/wire/layeringInvariants.test.ts:50):RUNTIME_SCHEMAS_IMPORT = /^(import|export)\s+(?!type\b)[^;]*?\bfrom\s+['\"][^'\"]*types\/schemas(?:\.js)?['\"]/m, applied to every file undersrc/wire/rev*/by invariant (b) — a test that exists explicitly so "the lint rules cannot be silently weakened".
Neither pattern matches @modelcontextprotocol/core/internal or @modelcontextprotocol/core — both require the literal substring types/schemas in the module specifier.
Why this PR opens the bypass
Before the move, the dependency direction was core→core-internal, so no @modelcontextprotocol/core specifier could even resolve from inside core-internal. This PR reverses that: @modelcontextprotocol/core becomes a runtime dependency of core-internal (packages/core-internal/package.json), and packages/core-internal/tsconfig.json gains a package-wide paths alias for @modelcontextprotocol/core/internal → core/src/internal.ts. core/src/internal.ts does export * from './schemas', so the new specifier is a second canonical path to the exact same mutable schema objects the wire-isolation guards exist to freeze against.
Step-by-step proof
- A contributor adding 2026-era wire support writes, inside
src/wire/rev2026-07-28/:instead of freezing a copy into the rev directory.import { ContentBlockSchema } from '@modelcontextprotocol/core/internal';
- typecheck: passes — the tsconfig paths alias resolves the specifier to
core/src/internal.ts. - lint: passes —
'@modelcontextprotocol/core/internal'matches neither'**/types/schemas'nor'**/types/schemas.js'. - layeringInvariants (b): passes — the specifier contains no
types/schemassubstring, soRUNTIME_SCHEMAS_IMPORTdoesn't match. - runtime: works — core is a real dependency, and the bundles keep
core/internalexternal. - Later, a spec-tracking widening of
ContentBlockSchemaincore/src/schemas.tsretroactively changes what the supposedly-frozen 2026 era accepts on the wire — with every gate green. That is precisely the drift the invariant was written to prevent.
Why nothing else catches it
The PR's own new guards cover other directions but not this one: schemaShims.test.ts only pins the three re-export shims to be forwarding-only, and coreBoundary.test.ts only checks that names the client dist imports exist in core's built exports. A wire/rev file importing a perfectly valid mutable schema trips neither. Notably, the PR did update its other guard keyed to the schemas' location — wireOnlyHiding.test.ts now reads core/src/schemas.ts directly — so leaving the two wire-isolation guards on the retired path looks like an oversight rather than a design choice.
Impact and fix
I verified (grep) that no file under src/wire/ imports any @modelcontextprotocol/core specifier today, so nothing breaks on merge — this is future-drift insurance with a hole in it, not a live defect; hence nit rather than blocking. The fix is two one-line pattern extensions: add '@modelcontextprotocol/core' and '@modelcontextprotocol/core/internal' (plus subpath-glob variants as appropriate) to the eslint group in eslint.config.mjs, and extend RUNTIME_SCHEMAS_IMPORT in layeringInvariants.test.ts to also match the core specifiers, keeping the existing allowTypeImports/type-only exemptions.
| /** | ||
| * Canonical dist sentinels per package. Every caller waits on the same | ||
| * complete set: if callers checked their own subsets, a partial dist (from an | ||
| * interrupted build) could satisfy one caller's fast path while another still | ||
| * rebuilds — and the rebuild's clean step would wipe files under the first | ||
| * caller's running tests, which is exactly the race this helper exists to | ||
| * prevent. | ||
| */ | ||
| const DIST_SENTINELS: Record<string, string[]> = { | ||
| client: ['index.mjs', 'stdio.mjs'], | ||
| core: ['index.mjs', 'internal.mjs'] | ||
| }; |
There was a problem hiding this comment.
🟡 Two robustness gaps in the new ensureBuilt helper: (1) DIST_SENTINELS.client = ['index.mjs', 'stdio.mjs'] is a strict subset of what the callers actually read (barrelClean.test.ts needs dist/index.d.mts/.d.cts, the shims*.mjs entries, and dist/validators/ajv.cjs), so a build interrupted after tsdown's JS bundling but before the slower tsc dts pass leaves a dist that satisfies haveAll() forever — the helper never rebuilds, and every later run fails with a confusing ENOENT until someone manually rebuilds. (2) The stale-lock steal is a non-atomic stat→rmSync sequence: two waiters can both observe the stale mtime, and the second's rmSync(force: true) deletes the first's fresh lock, letting two pnpm build runs (each with clean: true) execute concurrently — exactly the race this helper exists to prevent. Fix (1) by adding one sentinel per build phase (e.g. index.d.mts, shimsBrowser.mjs) or a completion-marker file written last, and (2) by making the steal atomic (e.g. renameSync the stale lock to a unique name; only the winner succeeds) or re-verifying staleness after acquiring.
Extended reasoning...
Issue 1: incomplete sentinel set can permanently wedge the dist
The helper's own docstring states the invariant: "Every caller waits on the same complete set: if callers checked their own subsets, a partial dist (from an interrupted build) could satisfy one caller's fast path…" — but the registered set itself is a subset of what the callers read. DIST_SENTINELS.client lists only index.mjs and stdio.mjs, while barrelClean.test.ts reads dist/index.d.mts and dist/index.d.cts (via rootExportBlockOf), dist/shimsNode.mjs / shimsWorkerd.mjs / shimsBrowser.mjs, and requires dist/validators/ajv.cjs; coreBoundary.test.ts reads every top-level *.mjs chunk.
Step-by-step failure: (1) On a cold checkout, worker A takes the lock and runs pnpm build. tsdown emits the rolldown JS bundles (including both sentinels) quickly; the resolver: 'tsc' dts pass is a distinctly slower, later phase. (2) The build is killed mid-flight — by the helper's own BUILD_TIMEOUT_MS = 60_000 execFile timeout, or a Ctrl-C propagating SIGINT to the child. The finally block still removes the lock. (3) dist now contains index.mjs + stdio.mjs but no declaration files. (4) On every subsequent run, haveAll() is true and no lock exists, so the fast path returns immediately — and even a worker that somehow takes the lock skips the build because !haveAll() is false. Per the design comment, "once the dist exists nobody ever rebuilds." (5) Every later test run fails with an ENOENT on index.d.mts (or a regex mismatch) — a confusing failure that points nowhere near the actual cause — and the state never self-heals until someone manually deletes dist/ or runs pnpm build.
Fix: include one artifact from each build phase in the sentinel list (e.g. add index.d.mts for the dts phase and shimsBrowser.mjs for the entry set), or have the build path write a completion-marker file as its last step and use that as the sole sentinel.
Issue 2: the stale-lock steal is not atomic
The steal path is stat → decide → rmSync(recursive, force) → continue → mkdirSync. The decision is made from a stat of the old stale lock, but rmSync unconditionally deletes whatever directory exists at lockDir at rm-time, with no re-check.
Step-by-step interleaving: a leftover .dist-build-lock from a previously killed run (SIGKILL/OOM — the documented scenario the steal exists for) means every vitest worker of the next run hits the steal path simultaneously at startup, since barrelClean.test.ts and coreBoundary.test.ts both call ensureBuilt(clientPkgDir) from separate parallel workers and all observe age > STALE_LOCK_MS at once. Then: (1) C stats the stale lock and decides to steal; (2) B stats it too, rmSyncs the old lock, continues, mkdirSyncs a fresh lock, and spawns pnpm build; (3) C — delayed between its stat and its rm — now executes rmSync(force: true), silently deleting B's fresh lock; (4) C's mkdirSync succeeds and C also spawns pnpm build. Two concurrent builds now run in the same package, and tsdown's clean: true wipes dist out from under the sibling build; worse, with B's lock gone, any third worker's fast path (haveAll() && !existsSync(lockDir)) can return and start reading a dist that C's clean step is about to delete. This is precisely the failure mode the file header says the helper prevents, reintroduced in its own recovery path.
Fix: make the steal atomic — e.g. renameSync the stale lock dir to a unique name (rename is atomic; only one stealer succeeds, losers get ENOENT and re-loop as ordinary waiters), or re-stat/verify the lock's mtime immediately after acquiring.
Why nit, not blocking
Both issues live in test-only infrastructure and fail loudly (ENOENT / a flaky red run), never as a silent false green, and nothing shipped to consumers is affected. Issue 1 requires an interrupted build on a local machine (CI runs from clean checkouts or pre-built dists) and recovers with one pnpm build; issue 2 additionally requires a dead-worker stale lock plus a microseconds-wide interleaving, and usually self-heals on the next run. But since this file's entire reason to exist is making on-demand builds race-free, both fixes are cheap and worth taking: they're each roughly a one-line change to freshly-written locking code.
| ## Reach for `core` only to validate raw wire JSON | ||
|
|
||
| `@modelcontextprotocol/core` exports the Zod schema constants the SDK validates protocol payloads against, for code that handles raw JSON-RPC payloads itself — gateways, proxies, log pipelines. Neither `server` nor `client` exports a Zod schema, and the matching TypeScript types ship with both, so if you only call `registerTool` and `callTool` you never install it. [Wire schemas](../advanced/wire-schemas.md) is the how-to. | ||
| `@modelcontextprotocol/core` exports the Zod schema constants the SDK validates protocol payloads against, for code that handles raw JSON-RPC payloads itself — gateways, proxies, log pipelines. Neither `server` nor `client` exports a Zod schema, and the matching TypeScript types ship with both, so if you only call `registerTool` and `callTool` you never import it directly — it arrives transitively, since `server` and `client` resolve their shared schema graph from it at runtime. [Wire schemas](../advanced/wire-schemas.md) is the how-to. |
There was a problem hiding this comment.
🟡 A third instance of the stale core-topology claim survives in docs/migration/upgrade-to-v2.md:1328: it still says @modelcontextprotocol/core "is not required by client / server — install it only if you import the raw schemas directly", which is false after this PR makes core an exact-pinned runtime dependency. Commit 68688c9 fixed the same sentence in packages.md and wire-schemas.md but missed this one — reword it to match the corrected "arrives transitively as the shared runtime schema graph" language.
Extended reasoning...
What's stale. This PR makes @modelcontextprotocol/core an exact-pinned runtime dependency of client, server, and server-legacy (each package.json now declares "@modelcontextprotocol/core": "workspace:*"), and the built bundles resolve @modelcontextprotocol/core/internal as a live external import — pinned by the new coreBoundary.test.ts. The earlier review round flagged two docs asserting the old topology, and commit 68688c9 fixed both (docs/get-started/packages.md:69 and docs/advanced/wire-schemas.md:60 now say core "arrives transitively" as the shared runtime schema graph). But a third instance survived: docs/migration/upgrade-to-v2.md:1326-1329 still reads "It is runtime-neutral (its only dependency is zod) and is not required by client / server — install it only if you import the raw schemas directly."\n\nStep-by-step proof that the sentence is false post-merge. (1) A v1 user upgrading per this guide runs npm install @modelcontextprotocol/client (or server). (2) npm reads the client's dependencies, resolves @modelcontextprotocol/core transitively, and it lands in node_modules on every install — directly contradicting "not required by client / server". (3) At runtime the installed client's dist chunks contain live import { … } from '@modelcontextprotocol/core/internal' statements, so the dependency is real at module-resolution level, not just in the manifest. A grep over docs/ and package READMEs confirms this is the only surviving occurrence of the old "not required / never depend / never install" framing after 68688c9.\n\nWhy it matters beyond mere staleness. The "install it only if…" framing in the v1→v2 migration guide — the doc most upgrading users actually read — invites users to pin their own core version alongside client/server. Because core/client/server/server-legacy now release as a changesets fixed group with exact pins, a version-skewed self-pinned core resolved in place of the paired one fails at import time with ERR_PACKAGE_PATH_NOT_EXPORTED (the PR description names this failure mode). So the sentence steers readers toward exactly the configuration the fixed group and exact pins were added to prevent.\n\nWhy nothing in the PR catches it. The drift guards added here (coreBoundary.test.ts, schemaShims.test.ts, packageTopologyPins.test.ts) pin build artifacts and manifests, not prose. The doc-topology fix in 68688c9 was scoped to the two files the earlier comment named, and the migration guide was not in the diff.\n\nFix. A one-sentence reword at upgrade-to-v2.md:1328 matching the corrected language in packages.md/wire-schemas.md — e.g.: "It is runtime-neutral (its only dependency is zod). client / server resolve their shared schema graph from it at runtime, so it already arrives transitively in your tree; add it to your own dependencies only when you import the raw schemas directly." The runtime-neutral / zod-only half of the claim remains true and should stay.\n\nSeverity. Nit: doc-only — nothing breaks at runtime if merged as-is, and the "only import it directly if you need raw schemas" advice half remains sound. But it is the same class of stale prose the author already accepted and fixed twice in this PR, so finishing the sweep is cheap and worthwhile.
1 participant
Moves the neutral schema modules — the spec schemas, the OAuth/OpenID schemas, and the protocol constants — out of core-internal and into
@modelcontextprotocol/coreas real source.client,server, andserver-legacynow resolve them from core as a regular (exact-pinned) runtime dependency instead of each bundling a private copy.Motivation and Context
#2459 deduplicated the schema graph by rewriting module ids to the core specifier at build time. That produced the right artifacts, but the dependency direction lived only inside three build configs — source still said the schemas belonged to core-internal, and a rename or split could silently re-inline ~2,400 lines of Zod graph into every consumer with all gates green. Moving the source makes the published edge real: core owns the schemas, core-internal forwards from the old paths (one-line-per-name shims; a test keeps them forwarding-only), and the bundlers externalize a declared dependency with no plugin at all. Supersedes #2459.
An application importing more than one of the packages evaluates one schema graph instead of two or three, with shared object identity across packages.
How Has This Been Tested?
Breaking Changes
None. Public APIs, types, and behavior are unchanged. Additive: core gains a
./internalsubpath (documented as an SDK-internal contract that may change in any release); the three packages gain core as an exact-pinned dependency, resolved automatically by npm. core/client/server/server-legacy now version together via a changesets fixed group, so a mismatched pair can never be published or installed.Types of changes
Checklist
Additional context
ERR_PACKAGE_PATH_NOT_EXPORTEDrather than drifting silently../internalonce the barrel export-star cleanup lands.