perf: resolve schema modules from @modelcontextprotocol/core instead of inlining them#2459
perf: resolve schema modules from @modelcontextprotocol/core instead of inlining them#2459felixweinberger wants to merge 4 commits into
Conversation
🦋 Changeset detectedLatest commit: afcf255 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 |
@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}
| * matching public type (e.g. `BaseRequestParamsSchema`, | ||
| * `NotificationsParamsSchema`, `ClientTasksCapabilitySchema`, `ServerTasksCapabilitySchema`). |
There was a problem hiding this comment.
🟡 The stale 'ListChangedOptionsBaseSchema' entry remains in the INTERNAL_HELPER_SCHEMAS array in packages/core-internal/test/types/specTypeSchema.test.ts (line 169), even though this PR moved that schema out of types/schemas.ts and removed it from the exclusion comment in specTypeSchema.ts and from the parallel list in packages/core/test/coreSchemas.test.ts. The entry is now dead (it can never match Object.keys(schemas)) and the array's "mirrors the exclusion comment" claim is no longer accurate — removing that one line keeps the three lists consistent.
Extended reasoning...
What the issue is: The PR relocates ListChangedOptionsBaseSchema from packages/core-internal/src/types/schemas.ts into the new packages/core-internal/src/types/listChangedOptions.ts (so it stays bundled while schemas.ts gets externalized to @modelcontextprotocol/core). The two lists that track schemas.ts internal helpers were updated accordingly: the exclusion comment in specTypeSchema.ts (this PR drops the name from the "e.g." list) and SPEC_INTERNAL_HELPERS in packages/core/test/coreSchemas.test.ts (entry removed in the diff). But the third parallel list — INTERNAL_HELPER_SCHEMAS in packages/core-internal/test/types/specTypeSchema.test.ts line 169 — still contains 'ListChangedOptionsBaseSchema'.\n\nCode path: That test does import * as schemas from '../../src/types/schemas' and its drift-guard filters Object.keys(schemas) against INTERNAL_HELPER_SCHEMAS to compute the set of exported schemas that must appear in SPEC_SCHEMA_KEYS. After this PR, schemas.ts no longer exports ListChangedOptionsBaseSchema, so the entry never matches anything — it is dead data.\n\nStep-by-step proof:\n1. Post-PR, grep 'export const ListChangedOptionsBaseSchema' packages/core-internal/src/types/schemas.ts returns nothing (the definition now lives in types/listChangedOptions.ts).\n2. The test's namespace import targets ../../src/types/schemas directly (not the types/index.ts barrel), so Object.keys(schemas) no longer contains 'ListChangedOptionsBaseSchema'.\n3. The filter !INTERNAL_HELPER_SCHEMAS.includes(name) therefore never sees that name; the entry has no effect on the assertion.\n4. The array's own comment (line 166) says it "Mirrors the exclusion comment in specTypeSchema.ts" — but this PR edited that exclusion comment to remove ListChangedOptionsBaseSchema, so the mirror claim is now stale.\n\nWhy nothing prevents it: The test still passes — an extra never-matching name in an exclusion list cannot fail the drift guard — so CI does not flag the leftover.\n\nImpact: Purely cosmetic/maintenance drift: a future reader comparing the three lists (specTypeSchema.ts comment, coreSchemas.test.ts, this test) will find them inconsistent, and the dead entry could mask intent if the schema name were ever reintroduced in schemas.ts.\n\nFix: Delete the 'ListChangedOptionsBaseSchema', line from INTERNAL_HELPER_SCHEMAS in packages/core-internal/test/types/specTypeSchema.test.ts (line 169), matching the update already made to the sibling list in packages/core/test/coreSchemas.test.ts.
| - **`@modelcontextprotocol/core-internal/public`** (`packages/core-internal/src/exports/public/index.ts`) — Curated public API. Exports only TypeScript types, error classes, constants, and guards. Re-exported by client and server packages. | ||
| - **`@modelcontextprotocol/client`** and **`@modelcontextprotocol/server`** (`packages/*/src/index.ts`) — Final public surface. Package-specific exports (named explicitly) plus re-exports from `core-internal/public`. | ||
| - **`@modelcontextprotocol/core`** (`packages/core/src/index.ts`) — Public Zod-schema package. Re-exports **only** the `*Schema` Zod constants (MCP spec + OAuth/OpenID), bundled from `core-internal` at build time. The published home for raw runtime validation (`CallToolResultSchema.parse(...)`); runtime-neutral (`zod` is its only dependency). Not consumed by the sibling packages — `client`/`server` keep their own bundled schema copies and stay Zod-free in their public surface. | ||
| - **`@modelcontextprotocol/core`** (`packages/core/src/index.ts`) — Public Zod-schema package. Re-exports **only** the `*Schema` Zod constants (MCP spec + OAuth/OpenID), bundled from `core-internal` at build time. The published home for raw runtime validation (`CallToolResultSchema.parse(...)`); runtime-neutral (`zod` is its only dependency). Consumed by the sibling packages at runtime: `client`/`server`/`server-legacy` bundles resolve the schema modules from this package (see the `externalize-core-schemas` plugin in their tsdown configs) so one evaluated schema-graph copy is shared per application; their public surfaces stay Zod-free. |
There was a problem hiding this comment.
🟡 The user-facing docs still describe the pre-PR relationship between @modelcontextprotocol/core and the client/server packages: docs/advanced/wire-schemas.md:59 says server and client "never depend on it" and docs/get-started/packages.md:69 says "you never install it", but after this PR core is a runtime dependency of client/server/server-legacy and their bundles import the schema modules from it. These two doc sentences should be updated in this PR alongside the CLAUDE.md change that already reflects the new relationship.
Extended reasoning...
What's stale. This PR turns @modelcontextprotocol/core into a real runtime dependency of client, server, and server-legacy: each package.json adds "@modelcontextprotocol/core": "workspace:^" to dependencies, and the new externalize-core-schemas plugin in the three tsdown configs rewrites imports of core-internal/src/types/schemas.ts and core-internal/src/shared/auth.ts to @modelcontextprotocol/core as an external, so the published bundles import the schema modules from that package at runtime. CLAUDE.md line 73 was updated to describe exactly this ("Consumed by the sibling packages at runtime…"), but the user-facing docs were not.\n\nWhere the docs now contradict the implementation.\n\n- docs/advanced/wire-schemas.md:59: "Install it separately (npm install @modelcontextprotocol/core) — @modelcontextprotocol/server and @modelcontextprotocol/client keep a Zod-free public surface and never depend on it." — the "never depend on it" clause is now flatly false, and "install it separately" is misleading since core now arrives transitively with client/server.\n- docs/get-started/packages.md:69: "…so if you only call registerTool and callTool you never install it." — after this PR core is always installed transitively whenever client or server is installed.\n\nConcrete walk-through. A user runs npm install @modelcontextprotocol/server. npm resolves dependencies and pulls @modelcontextprotocol/core into node_modules (the pnpm-lock.yaml diff in this PR shows the same for the workspace). The server bundle's dist/index.mjs now contains import … from '@modelcontextprotocol/core' where the schema graph used to be inlined, so the package is loaded at runtime, not just present on disk. Meanwhile the docs the user reads say the package is something they'd "install separately" and that server/client "never depend on it" — the opposite of what npm ls @modelcontextprotocol/core will show them.\n\nWhy nothing else prevents this. The parts of those sentences about the public surface remain true — client/server still export no Zod schemas and their public API stays Zod-free — so the pages don't fail any snippet-sync or example CI check; only the prose about the dependency graph is wrong, and nothing automated validates prose against package.json.\n\nImpact and fix. No runtime behavior is affected; this is documentation drift only, but it's drift introduced by this PR (which already fixed the equivalent sentence in CLAUDE.md), so it's cheapest to fix here. Suggested edits: in wire-schemas.md, replace "Install it separately … never depend on it" with wording like "It is installed automatically as a dependency of @modelcontextprotocol/server and @modelcontextprotocol/client (their bundles resolve the schema modules from it), but their public surfaces stay Zod-free — import from @modelcontextprotocol/core directly when you need the raw schemas." In packages.md, change "you never install it" to something like "you never need to import it directly (it's installed transitively with client/server)."
…ining them client, server, and server-legacy bundle @modelcontextprotocol/core-internal wholesale, which inlines the spec + OAuth Zod schema modules that are also published verbatim as @modelcontextprotocol/core. An application using client + server therefore evaluates the same large schema graph three times (and nests three copies of the code). Add an externalize-core-schemas rolldown plugin to the three configs that resolves core-internal/src/types/schemas.ts and core-internal/src/shared/auth.ts to @modelcontextprotocol/core at runtime, and declare core as a real dependency of the three packages. One evaluated schema-graph copy is now shared per application. Every name crossing the boundary at runtime is exported by core: - guards.ts / specTypeSchema.ts / client.ts runtime imports are all in core's export list (the specTypeSchema namespace access is keyed by the SPEC_SCHEMA_KEYS allowlist, which excludes internal helpers); - the remaining internal-only exports (BaseRequestParamsSchema, NotificationsParamsSchema, ClientTasksCapabilitySchema, ServerTasksCapabilitySchema, SafeUrlSchema, OptionalSafeUrlSchema) are only imported type-only or within the externalized modules themselves; - ListChangedOptionsBaseSchema was the one runtime-used SDK-internal helper living in schemas.ts, so it moves to types/listChangedOptions.ts and stays bundled (it is an SDK option schema, not a spec schema, and is deliberately not part of core's public surface). Type declarations still inline the schema types via the dts paths mappings — only runtime duplication costs. The wire-revision codec schemas and other core-internal modules remain bundled (they are private and not published anywhere), so client/server keep their direct zod dependency.
The repo's CI test job never runs a standalone build — a package's dist exists only if one of its tests builds it (client's barrelClean.test.ts does this for client). Now that client, server, and server-legacy resolve their schema modules from @modelcontextprotocol/core's dist entry at runtime, a missing core build breaks every consumer of a built package, which is exactly how the examples' suites failed in CI. Mirror the barrelClean pattern: build core on demand in beforeAll, then pin the runtime surface the boundary depends on (representative spec + OAuth schema exports and the overall export count).
No-Verification-Needed: changeset-only release metadata
felixweinberger
force-pushed
the
fweinberger/externalize-core-schemas
branch
from
e4f826a to
9443e62
Compare
| "test:watch": "vitest" | ||
| }, | ||
| "dependencies": { | ||
| "@modelcontextprotocol/core": "workspace:^", |
There was a problem hiding this comment.
🔴 This PR creates a hard runtime ABI between the published client/server/server-legacy bundles and @modelcontextprotocol/core (the bundles emit named ESM imports from core, so a single missing export makes the whole package fail at import time), but nothing ties changes to the two externalized modules (core-internal/src/types/schemas.ts, shared/auth.ts) to a core release: .changeset/config.json has fixed: []/linked: [], and workspace:^ resolves at publish to a caret floor at core's current version — which, if core wasn't bumped, is exactly the published artifact missing the new export. Consider adding core to a changesets fixed/linked group with client/server/server-legacy (or a CI check diffing the bundles' core import names against the latest published core) so a routine future schema addition can't publish packages that hard-fail to import against the newest published core.
Extended reasoning...
The bug. The externalize-core-schemas plugin makes the published client/server/server-legacy bundles emit real named ESM imports from @modelcontextprotocol/core (~32 named imports plus namespace access to all ~160 spec schema keys via the bundled specTypeSchema.ts). Under Node's ESM link-time validation, a single name missing from core's dist is not a lazy failure — it is SyntaxError: The requested module '@modelcontextprotocol/core' does not provide an export named '...', and the entire consuming package fails to import (verified experimentally by the finders). That means the published bundles now have a runtime ABI against whatever core version npm resolves — and nothing in the repo forces the two sides of that ABI to release together.\n\nWhy nothing enforces the coupling. .changeset/config.json has "fixed": [] and "linked": [] — no lockstep group covers core + its three new consumers. updateInternalDependencies: patch only propagates in the dependency→dependent direction (when core itself bumps); it never forces a core bump when a consumer bumps. core-internal is private: true and invisible to changesets, so edits to the externalized source modules (core-internal/src/types/schemas.ts, core-internal/src/shared/auth.ts) prompt nothing. And workspace:^ is rewritten by pnpm at publish time to a caret floor at core's current workspace version — if core wasn't bumped in the same release, that floor is exactly the already-published core artifact that lacks the new export.\n\nWhy no existing guard catches it. Every guard in the tree is source-level or same-tree: coreSchemas.test.ts syncs core/src/index.ts with core-internal source; the new distEntry.test.ts builds core from current source; CI test jobs and this PR's packed-tarball smoke resolve link:../core from the same tree. The skew this bug describes exists only between two independently published npm artifacts — a surface no check in the repo ever compares. Every CI check stays green while the broken pair publishes.\n\nConcrete failure walk-through.\n1. A future PR (spec revisions add schemas routinely) adds NewThingSchema to core-internal/src/types/schemas.ts, adds it to SPEC_SCHEMA_KEYS and core/src/index.ts (the drift test in coreSchemas.test.ts forces the source sync), and uses it at runtime in packages/client.\n2. The author adds a changeset for client only — nothing prompts a core changeset, and the changeset-bot only comments; there is no blocking check.\n3. CI is fully green (workspace core always contains the new export), and the release publishes client@2.0.0-beta.N with dependency ^2.0.0-beta.3 while core is not republished.\n4. Every fresh npm install @modelcontextprotocol/client resolves the newest published core (2.0.0-beta.3, missing NewThingSchema), and the client package cannot even be imported.\n\nA milder variant hits stale lockfiles even when core is correctly republished, because the caret floor isn't raised unless core bumps in the same release.\n\nWhy this PR is the right place to fix it. Before this PR the failure mode did not exist: each package inlined its own schema copy, so a schema addition shipped atomically inside the consuming package. This PR is what establishes the cross-package runtime ABI. Note also that the PR description justifies the new dependency edge with core being 'already published and versioned in lockstep' — but no lockstep exists (fixed: [], and server-legacy is at 2.0.0-beta.2 while core/client are at 2.0.0-beta.3 today); the stated safety premise is unenforced prose. Repo history contains a 'chore: add missing changesets' fix-up commit, showing forgotten changesets are a real occurrence here.\n\nHow to fix (cheap options). Any of: (a) add core, client, server, server-legacy to a changesets fixed (or at least linked) group in .changeset/config.json; (b) pin the dependency exactly (workspace:*) so consumers always require the core built alongside them; or (c) add a CI/publish-time check that diffs the dist bundles' @modelcontextprotocol/core import names against the exports of the latest published core tarball. Option (a) is a one-line config change and closes the gap entirely.
Extract the plugin duplicated across the client, server, and server-legacy tsdown configs into packages/core-internal/externalizeCoreSchemas.tsdown.mjs, and replace the path regex with an explicit list of the two schema modules. The build now fails loudly when the rewrite cannot have worked, in both directions: a listed module that is missing or never rewritten fails the build (renaming or moving a schema module previously re-inlined the whole schema graph into every consumer with all gates green), and a listed module that still reaches an emitted runtime chunk fails the build (a resolver id decoration bypassing the comparison). The checks look at emitted runtime chunks rather than the module graph because the d.ts pass loads the schema sources on purpose to inline type declarations. Also add a consumer-side dist test asserting the built client chunks import '@modelcontextprotocol/core' and link against its real export surface under plain-node ESM semantics, closing the hole where a value import of an SDK-internal helper schema would build fine but throw a link error in published consumers.
|
|
||
| test('every name the built chunks import from @modelcontextprotocol/core exists in core, under real ESM linking', () => { | ||
| for (const chunk of chunksImportingCore()) { | ||
| // Plain-node import gives real ESM link semantics: it fails at instantiation if the chunk | ||
| // names an export core does not provide (e.g. an SDK-internal helper schema accidentally | ||
| // imported as a value) — exactly the error a published package would throw in consumers. | ||
| // Vitest's own module transform would report such a name as `undefined` instead of failing. | ||
| const result = spawnSync( | ||
| process.execPath, | ||
| ['--input-type=module', '-e', `await import(${JSON.stringify(pathToFileURL(chunk).href)});`], | ||
| { encoding: 'utf8', timeout: 30_000 } | ||
| ); | ||
| expect(result.status, `importing ${chunk} failed:\n${result.error ?? result.stderr}`).toBe(0); | ||
| } | ||
| }, 60_000); |
There was a problem hiding this comment.
🟡 The new boundary test's comment says a plain-node import "fails at instantiation if the chunk names an export core does not provide" — but that is only true for named import specifiers. The bundled specTypeSchema.ts reaches the externalized module via import * as schemas from '@modelcontextprotocol/core' plus dynamic schemas[key] access over all ~160 SPEC_SCHEMA_KEYS, and ESM linking never validates namespace member access, so a key missing from the resolved core imports cleanly as undefined and only fails at the consumer's first specTypeSchemas.X/isSpecType.X call. Cheap hardening: throw in the register() loop when schemas[key] === undefined (making future drift loud at import time, where this test would catch it), and/or assert every SPEC_SCHEMA_KEYS name is in Object.keys of core's built dist entry.
Extended reasoning...
What the gap is. packages/core-internal/src/types/specTypeSchema.ts is bundled into client/server/server-legacy and reaches the externalized schema module through a namespace import: import * as schemas from './schemas' is rewritten by the externalize-core-schemas plugin into import * as schemas from \"@modelcontextprotocol/core\" (verified in the built output — e.g. packages/server/dist/mcp-*.mjs contains exactly that statement, followed by for (const key of SPEC_SCHEMA_KEYS) register(key, schemas[key]);). ES module linking validates only named import specifiers; namespace member access is never checked at link time. So if a key in SPEC_SCHEMA_KEYS has no corresponding export in the resolved @modelcontextprotocol/core, schemas[key] silently evaluates to undefined, the package imports without error, and register() happily stores the undefined value (it only builds a lazy safeParse closure — nothing dereferences the schema at import time).
Why the test's comment overstates its coverage. The comment at externalizedCoreBoundary.test.ts:41-44 claims the plain-node import "fails at instantiation if the chunk names an export core does not provide." That holds for the ~32 named imports the chunks emit, but not for the single largest surface crossing the boundary: all ~160 spec schema keys accessed dynamically through the namespace. Verified experimentally: import * as ns from ...; ns.doesNotExist is undefined under plain-node ESM — no SyntaxError, no link failure. The test passes even when this surface is broken.
The concrete code path that fails. isSpecType/specTypeSchemas are public API (core-internal/public exports them, and client/server re-export public wholesale), so the registration loop runs at package import time in every built artifact. A missing key produces:
schemas['NewThingSchema']→undefined(no error).register('NewThingSchema', undefined)storesundefinedand a closure(v) => schema.safeParse(v).success.- Package import succeeds; every test and smoke that merely imports the package stays green.
- The consumer's first
isSpecType.NewThing(value)call throwsTypeError: Cannot read properties of undefined (reading 'safeParse')— a baffling error far from the actual cause.
Two realistic triggers. (a) Version skew: a future PR adds a schema to schemas.ts + SPEC_SCHEMA_KEYS with no other direct SDK runtime use (common for spec vocabulary that exists only on the validation surface, like several current Task* entries). The bundle then carries no named import of it — only namespace access — so against an older published core the package does not hard-fail at import (as the existing ABI review comment assumes for named imports); it ships broken validators. (b) Helper-list drift with same-version core: the subset relation SPEC_SCHEMA_KEYS ⊆ core-exports is maintained only by hand-synced lists (SPEC_INTERNAL_HELPERS in packages/core/test/coreSchemas.test.ts, INTERNAL_HELPER_SCHEMAS in core-internal's specTypeSchema.test.ts, and the allowlist itself); a mis-sync where a name lands in SPEC_SCHEMA_KEYS but is excluded from core reproduces the same silent state, and every vitest run hides it because tests resolve schemas.ts from source via tsconfig paths, where the name always exists.
Why nothing else prevents it. The plugin's build-time assertions only prove the rewrite happened (external imports are never validated against the target's export surface at build time); this test's plain-node import validates named specifiers only and evaluation defers all schemas[key] reads; coreSchemas.test.ts and specTypeSchema.test.ts are source-text checks on separate lists with no assertion tying SPEC_SCHEMA_KEYS to the built core's Object.keys.
How to fix (cheap). Either or both: (1) make the registration loop eager-fail — in specTypeSchema.ts, if (schemas[key] === undefined) throw new Error(\spec schema ${key} missing from resolved @modelcontextprotocol/core`)— which converts both scenarios into a loud import-time error that this exact test (and every consumer) would surface immediately; (2) add an assertion (here or indistEntry.test.ts) that every SPEC_SCHEMA_KEYSname appears inObject.keys(await import(core/dist/index.mjs))`.
Why this is a nit, not blocking. All 160 keys are present in core's dist today (independently verified), so nothing is broken as merged; both triggers require future drift or version skew. This is hardening of the guard design this PR itself introduces — worth doing here because the test comment currently overstates what the test proves, but merging without it breaks nothing. It also complements (does not duplicate) the earlier ABI review comment, which assumes missing exports fail loudly at import — this is the complementary silent-failure path that that comment's proposed checks would also miss.
| function chunksImportingCore(): string[] { | ||
| return (readdirSync(distDir, { recursive: true }) as string[]) | ||
| .filter(file => file.endsWith('.mjs')) | ||
| .map(file => join(distDir, file)) | ||
| .filter(chunk => CORE_IMPORT.test(readFileSync(chunk, 'utf8'))); | ||
| } |
There was a problem hiding this comment.
🟡 The new consumer-side boundary guard is applied to only one of the three packages that received the externalize plugin: packages/server and packages/server-legacy get the same rewrite but have no equivalent of externalizedCoreBoundary.test.ts, so a future server-only value import of a name core does not export would build green and fail at consumers' import time; additionally, chunksImportingCore() filters .mjs only, leaving the CJS half of the dual build unscanned. Consider replicating this test in server/server-legacy (the main gap) and optionally extending the chunk scan to .cjs artifacts plus a require() smoke in packages/core/test/distEntry.test.ts.
Extended reasoning...
The gap. This PR's safety mechanism for the new runtime boundary — plain-node importing every built chunk that names @modelcontextprotocol/core, so a value import of a name core does not export fails in CI with a real ESM link error — exists only for packages/client. The same externalize-core-schemas plugin is applied to packages/server and packages/server-legacy (their tsdown.config.ts), whose bundles emit their own sets of named imports from core (server-side code touches schemas client never imports, e.g. server auth paths through shared/auth.ts). A grep of both packages' test/ trees finds zero references to @modelcontextprotocol/core or the externalized boundary; server's barrelClean.test.ts only regex-greps emitted chunk text and never plain-node-imports the dist, so it cannot catch a missing export under real ESM link semantics. The plugin's own doc comment acknowledges the guard is client-only ("client's externalizedCoreBoundary.test.ts checks the built output against core's real export surface").\n\nWhy the plugin itself doesn't cover this. The plugin's generateBundle assertions verify only that the rewrite happened (a listed module was resolved and rewritten, and none reached an emitted chunk) — they never check that the rewritten import names actually exist in core's export surface. And typecheck can't catch it either: the dts paths mapping resolves core-internal to source, where internal-only helpers (e.g. SafeUrlSchema, OptionalSafeUrlSchema in shared/auth.ts, which core deliberately does not re-export) are visible.\n\nStep-by-step failure scenario. (1) A future change makes server code value-import SafeUrlSchema from shared/auth.ts. (2) Typecheck passes (source-level resolution sees the export). (3) The plugin rewrites the import to @modelcontextprotocol/core and its assertions pass — the rewrite happened. (4) The built dist/index.mjs now contains import { SafeUrlSchema } from '@modelcontextprotocol/core', a name core does not export. (5) For @modelcontextprotocol/server this would incidentally fail the examples CI job (stories load the built server dist under native ESM linking), but that coverage is accidental, not a pinned contract, and only for the entry points examples happen to exercise. For server-legacy there is no incidental coverage at all: no example depends on it, and its own tests run from src via tsconfig paths — its dist→core boundary has zero runtime exercise anywhere in CI. The published package would then throw SyntaxError: The requested module '@modelcontextprotocol/core' does not provide an export named 'SafeUrlSchema' in every consumer, at import time.\n\nThe CJS dimension, and the refutation. chunksImportingCore() filters .endsWith('.mjs') only, and distEntry.test.ts imports only dist/index.mjs, so the CJS half of the dual builds (main: ./dist/index.cjs, which emits require('@modelcontextprotocol/core')) is never scanned — and CJS fails silently (destructuring a missing export yields undefined, not a link error). One verifier refuted this half on the grounds that the failure is format-independent: the .mjs and .cjs chunks are emitted from the same module graph by the same build, so any bad name necessarily appears in the ESM chunks too, where the committed test fails loudly; a CJS-only divergence requires a hypothetical tsdown/rolldown CJS-emission regression, and core's two dist artifacts currently expose identical export surfaces (verified: 172 names each). That objection is fair, and it is why the .cjs extension is the secondary, belt-and-braces part of this finding rather than the core of it — the primary, source-level-reachable gap is the two packages with no test at all, which the refutation does not dispute.\n\nImpact and why nit. Nothing is broken as merged — the author verified every name currently crossing the boundary exists in core, so this is a completeness gap in a guard this PR introduces, not a live defect. It only bites on a future change, and for server the examples job provides partial incidental coverage. But it is exactly the hazard the client test was written to catch, left open on two of the three packages that got the plugin.\n\nFix. Copy externalizedCoreBoundary.test.ts into packages/server/test and packages/server-legacy/test (it is already parameterized only by pkgDir; a shared helper would also work). Optionally, extend the chunk filter to .cjs (matching require(\"@modelcontextprotocol/core\") syntax as well, since the from '...' regex won't match CJS) and add a require() smoke of dist/index.cjs to packages/core/test/distEntry.test.ts.
| beforeAll(() => { | ||
| if (!existsSync(join(distDir, 'index.mjs'))) { | ||
| execFileSync('pnpm', ['build'], { cwd: pkgDir, stdio: 'inherit' }); | ||
| } | ||
| if (!existsSync(join(coreDir, 'dist', 'index.mjs'))) { | ||
| execFileSync('pnpm', ['build'], { cwd: coreDir, stdio: 'inherit' }); | ||
| } | ||
| }, 240_000); |
There was a problem hiding this comment.
🟡 The build-on-demand beforeAll here can race with the identical hook in barrelClean.test.ts: vitest runs test files in parallel workers (the shared vitest config doesn't disable fileParallelism), and on a fresh CI checkout dist never exists, so both files can see a missing dist/index.mjs and spawn concurrent pnpm build runs of the same package — and client's tsdown config has clean: true, so the second build rimrafs dist while the first build's output is being written or read, producing intermittent ENOENT/import flakes. Consider building via a shared globalSetup, a lock/marker file, or colocating the two dist-building suites in one file.
Extended reasoning...
What the bug is. This PR gives packages/client a second test file that builds dist on demand. barrelClean.test.ts:40-44 and the new externalizedCoreBoundary.test.ts:26-33 both gate on existsSync(join(distDir, 'index.mjs')) and both run execFileSync('pnpm', ['build'], { cwd: pkgDir }) when it's missing. Client's tsdown.config.ts sets clean: true, so every build starts by deleting dist. Two concurrent builds of the same package are therefore not idempotent: the second build's clean step can delete chunks the first build just wrote — or chunks the first file's tests are actively reading.
Why the builds run concurrently. The shared common/vitest-config/vitest.config.js does not set fileParallelism: false, maxWorkers, or any sequence option, and client's own config doesn't override it, so vitest runs test files across parallel workers by default. CI's test job runs pnpm install followed by pnpm -r test with no prior build step — distEntry.test.ts's own comment notes "the repo's CI test job never runs a standalone build — dist only exists if a test builds it." So on every fresh CI run, client's dist is absent, and both files' beforeAll hooks fire.
Step-by-step failure scenario.
- Fresh CI runner;
packages/client/distdoes not exist. - Vitest schedules
barrelClean.test.tsandexternalizedCoreBoundary.test.tsin two parallel workers. - Worker A's
beforeAllsees nodist/index.mjsand spawnspnpm build(a tsdown build that takes tens of seconds — the entire duration is the race window). - Worker B's
beforeAllruns while A's build is still in flight, also sees nodist/index.mjs(or a partial dist), and spawns a secondpnpm build. - Build B's
clean: truestep rimrafsdist, deleting chunks build A already emitted. - Worker A's tests then hit
readFileSyncENOENT inchunkImportsOf(), an emptychunksImportingCore(), or a spawned plain-nodeimport()of a half-written chunk — a red CI run with a failure signature unrelated to any real regression.
Why nothing prevents it. There is no lock, marker file, or globalSetup serializing the builds; the existsSync check is a classic TOCTOU gate. Before this PR, barrelClean.test.ts was the only builder of client dist, so exactly one build ever ran and no race existed — the double-builder is introduced by this PR.
Scope caveat (what is not broken). The cross-package variant of this concern — the boundary test's second hook building packages/core while core's own distEntry.test.ts builds it — does not manifest under the standard invocations: pnpm -r test executes suites in topological order, and this PR makes client depend on core (workspace:^), so core's suite (which builds core's dist) completes before client's suite starts, and the boundary test's core build is a no-op skip. Only the intra-client race is real.
Impact. A new probabilistic flake vector in the per-PR CI gate. It requires the two files' beforeAll windows to overlap and an unlucky interleaving (two concurrent builds emit identical output, so many interleavings are benign — consistent with this PR's green node 20/22/24 matrix), so it costs intermittent triage time rather than a deterministic failure. Notably, the PR description already complains about triaging an unrelated wrangler flake; this adds another of the same species.
How to fix. Any of: (a) move the dist build into a vitest globalSetup for the client package so it runs exactly once before workers start; (b) colocate the boundary tests in barrelClean.test.ts (or a shared describe sequence) so one file owns the build; (c) serialize with a lockfile/marker around the existsSync + build; or (d) have the second hook wait-and-recheck instead of building. Option (a) is the cleanest and also covers any future dist-reading test file.
|
Closing in favor of #2477 — same externalization outcome with the schema modules moved into @modelcontextprotocol/core as real source, so the resolveId rewrite isn't needed at all. The boundary assertions from this PR's last commit carry over there. |
1 participant
Resolves the spec + OAuth schema modules from
@modelcontextprotocol/coreat runtime instead of inlining them into the client, server, and server-legacy bundles.Motivation and Context
clientandservereach bundle their own copy of core's schema modules, so an application that imports both evaluates the full spec-schema graph (hundreds of zod schema constructions) two to three times at startup. Measured on a large embedding application, deduplicating this to one evaluated copy recovers ~50ms of cold start. It also means one set of schema instances per process rather than several.Type declarations are unchanged — dts still inlines types via the existing paths mapping; only runtime resolution moves to the real
@modelcontextprotocol/coredependency. The one SDK-internal helper used at runtime by client (ListChangedOptionsBaseSchema) moved out of the schema module so it stays bundled, since core deliberately doesn't export it.How Has This Been Tested?
Full workspace suite green (
test:all,typecheck:all,lint:all,build:all); every name crossing the module boundary at runtime verified exported by core (spec schema keys 160/160, guards, 12 auth schemas); tamper test confirms core is a real runtime dependency; verified installable via packed tarballs with a runtime smoke over InMemoryTransport.Breaking Changes
None for consumers — public surfaces unchanged;
corebecomes a regular dependency of client/server/server-legacy (it was already published and versioned in lockstep).Types of changes
Checklist
Additional context
The externalize rewrite is now a single shared plugin (
packages/core-internal/externalizeCoreSchemas.tsdown.mjs) consumed by all three tsdown configs, with the module list as explicit resolved paths instead of a regex, plus build-time assertions in both directions: a listed schema module that is missing or never rewritten fails the build (a rename/move can no longer silently re-inline the schema graph into every consumer), and a listed module reaching an emitted runtime chunk fails the build (a bypassed rewrite can no longer ship). A consumer-side dist test (packages/client/test/client/externalizedCoreBoundary.test.ts) additionally checks that the built chunks import@modelcontextprotocol/coreand link against its real export surface under plain-node ESM semantics, so a value import of an internal helper schema fails in CI instead of at consumers' import time.Possible follow-up (post-beta, deliberately not this PR): move the two schema source modules into
packages/coreso the source dependency graph matches the published one and the build-time rewrite disappears entirely, with re-export shims left in core-internal. That is a large mostly-verbatim relocation across central files, so it should be its own change.Note on CI: the
test (20)/test (24)failures on the previous push were the known wrangler/miniflare "500 Network connection lost" flake intest/integration/test/server/cloudflareWorkers.test.ts(byte-identical signature failedmainon 2026-07-06 and 2026-07-09), unrelated to this branch.