refactor: move the schema source modules into @modelcontextprotocol/core (#2477)

packages/client/package.json

@@ -133,6 +133,7 @@
         "test:watch": "vitest"
     },
     "dependencies": {
+        "@modelcontextprotocol/core": "workspace:*",
         "cross-spawn": "catalog:runtimeClientOnly",
         "eventsource": "catalog:runtimeClientOnly",
         "eventsource-parser": "catalog:runtimeClientOnly",

packages/client/test/client/barrelClean.test.ts

@@ -1,11 +1,12 @@
-import { execFileSync } from 'node:child_process';
-import { existsSync, readFileSync } from 'node:fs';
+import { readFileSync } from 'node:fs';
 import { createRequire } from 'node:module';
 import { dirname, join } from 'node:path';
 import { fileURLToPath } from 'node:url';
 
 import { beforeAll, describe, expect, test } from 'vitest';
 
+import { ensureBuilt } from '../helpers/ensureBuilt';
+
 const pkgDir = join(dirname(fileURLToPath(import.meta.url)), '../..');
 const distDir = join(pkgDir, 'dist');
 const requireDist = createRequire(join(pkgDir, 'package.json'));
@@ -37,11 +38,9 @@ function rootExportBlockOf(content: string): string {
 }
 
 describe('@modelcontextprotocol/client root entry is browser-safe', () => {
-    beforeAll(() => {
-        if (!existsSync(join(distDir, 'index.mjs')) || !existsSync(join(distDir, 'stdio.mjs'))) {
-            execFileSync('pnpm', ['build'], { cwd: pkgDir, stdio: 'inherit' });
-        }
-    }, 60_000);
+    beforeAll(async () => {
+        await ensureBuilt(pkgDir);
+    }, 180_000);
 
     test('dist/index.mjs contains no process-spawning runtime imports', () => {
         const entry = join(distDir, 'index.mjs');

packages/client/test/client/coreBoundary.test.ts

@@ -0,0 +1,168 @@
+/**
+ * Schema-module boundary pins: the built client resolves its schemas from
+ * @modelcontextprotocol/core instead of carrying its own copies.
+ *
+ * The client bundle keeps `@modelcontextprotocol/core/internal` as an external
+ * runtime import (see tsdown.config.ts), so two things can silently go wrong:
+ *
+ * 1. Skew — the client dist imports a name that core's built entries no longer
+ *    export (e.g. a schema added to core-internal's shims without adding it to
+ *    core, or a rename that only landed on one side). That fails at consumer
+ *    runtime, not at build time.
+ * 2. Re-inlining — a config change (dropping the `external` entry, a paths
+ *    alias resolving too early) makes the bundler inline the schema sources
+ *    again, duplicating hundreds of Zod schemas into every sibling package.
+ *
+ * Both directions are asserted against the real build outputs. Only the ESM
+ * chunks are parsed; the CJS build is produced from the same module graph, so
+ * skew shows up identically in both.
+ */
+import { readdirSync, readFileSync } from 'node:fs';
+import { dirname, join } from 'node:path';
+import { fileURLToPath } from 'node:url';
+
+import { beforeAll, describe, expect, test } from 'vitest';
+
+import { ensureBuilt } from '../helpers/ensureBuilt';
+
+const clientPkgDir = join(dirname(fileURLToPath(import.meta.url)), '../..');
+const clientDistDir = join(clientPkgDir, 'dist');
+const corePkgDir = join(clientPkgDir, '..', 'core');
+const coreDistDir = join(corePkgDir, 'dist');
+
+/** Built core entry file per import specifier the client may use. */
+const CORE_ENTRIES: Record<string, string> = {
+    '@modelcontextprotocol/core': 'index.mjs',
+    '@modelcontextprotocol/core/internal': 'internal.mjs'
+};
+
+/**
+ * Sentinel schemas that exist ONLY in core's source modules. The frozen
+ * wire-era modules (bundled into the client on purpose) define their own
+ * copies of many spec schema names, but not these — so a `const` definition
+ * of any of them inside the client dist can only mean the neutral schema
+ * modules got re-inlined. The first two are names the client actually uses,
+ * so they must also show up as imports; SafeUrlSchema is unused by the client
+ * (legitimately tree-shaken away) and is pinned as never-defined only.
+ */
+const IMPORTED_SENTINELS = ['OAuthMetadataSchema', 'JSONRPCMessageSchema'];
+const NEVER_DEFINED_SENTINELS = [...IMPORTED_SENTINELS, 'SafeUrlSchema'];
+
+function clientChunks(): string[] {
+    return readdirSync(clientDistDir, { recursive: true })
+        .map(String)
+        .filter(f => f.endsWith('.mjs'));
+}
+
+/** Exported names of a built core entry (its trailing `export { a, b as c };` blocks). */
+function coreExportedNames(entryFile: string): Set<string> {
+    const src = readFileSync(join(coreDistDir, entryFile), 'utf8');
+    const blocks = [...src.matchAll(/export \{([\s\S]*?)\}/g)];
+    expect(blocks.length, `no export block found in core dist/${entryFile}`).toBeGreaterThan(0);
+    const names = new Set<string>();
+    for (const block of blocks) {
+        for (const entry of block[1]!.split(',')) {
+            const name = entry
+                .trim()
+                .split(/\s+as\s+/)
+                .pop()
+                ?.trim();
+            if (name) {
+                names.add(name);
+            }
+        }
+    }
+    return names;
+}
+
+/** All names a client chunk pulls from a core specifier, keyed by specifier. */
+function coreImportsOf(chunkSource: string): Map<string, Set<string>> {
+    const imports = new Map<string, Set<string>>();
+    // import { A, B as C } from "@modelcontextprotocol/core[/internal]"
+    // export { A, B as C } from "@modelcontextprotocol/core[/internal]"
+    for (const m of chunkSource.matchAll(
+        /(?:import|export)\s*\{([^}]*)\}\s*from\s*["'](@modelcontextprotocol\/core(?:\/internal)?)["']/g
+    )) {
+        const names = imports.get(m[2]!) ?? new Set<string>();
+        for (const entry of m[1]!.split(',')) {
+            // In both clause forms the name resolved against core is the one BEFORE `as`.
+            const name = entry
+                .trim()
+                .split(/\s+as\s+/)[0]
+                ?.trim();
+            if (name) {
+                names.add(name);
+            }
+        }
+        imports.set(m[2]!, names);
+    }
+    return imports;
+}
+
+describe('@modelcontextprotocol/client ↔ core schema boundary', () => {
+    beforeAll(async () => {
+        await ensureBuilt(corePkgDir);
+        await ensureBuilt(clientPkgDir);
+    }, 240_000);
+
+    test('every name the client dist imports from core resolves against core’s built exports', () => {
+        let sawCoreImport = false;
+        for (const chunk of clientChunks()) {
+            const source = readFileSync(join(clientDistDir, chunk), 'utf8');
+            for (const [specifier, names] of coreImportsOf(source)) {
+                sawCoreImport = true;
+                const entryFile = CORE_ENTRIES[specifier];
+                expect(entryFile, `client dist/${chunk} imports unknown core subpath ${specifier}`).toBeDefined();
+                const exported = coreExportedNames(entryFile!);
+                const missing = [...names].filter(name => !exported.has(name));
+                expect(
+                    missing,
+                    `client dist/${chunk} imports names from ${specifier} that core's built ${entryFile} does not export`
+                ).toEqual([]);
+            }
+            // Non-named forms (namespace/default/bare imports, `export *`) would dodge the check above.
+            expect(
+                source,
+                `client dist/${chunk} uses a non-named import/re-export of @modelcontextprotocol/core — update this test to cover it`
+            ).not.toMatch(/(?:import|export)\s+(?!\{)[^;]*?from\s*["']@modelcontextprotocol\/core(?:\/internal)?["']/);
+            // Bare side-effect imports have no `from` clause and would dodge the pattern above.
+            expect(
+                source,
+                `client dist/${chunk} uses a bare side-effect import of @modelcontextprotocol/core — update this test to cover it`
+            ).not.toMatch(/import\s*["']@modelcontextprotocol\/core(?:\/internal)?["']/);
+        }
+        expect(sawCoreImport, 'no client chunk imports @modelcontextprotocol/core at all — external wiring changed?').toBe(true);
+    });
+
+    test('the neutral schema bodies stay OUT of the client dist (imported, never re-inlined)', () => {
+        const sources = clientChunks().map(chunk => ({
+            chunk,
+            source: readFileSync(join(clientDistDir, chunk), 'utf8')
+        }));
+
+        const importedNames = new Set<string>();
+        for (const { source } of sources) {
+            for (const names of coreImportsOf(source).values()) {
+                for (const name of names) {
+                    importedNames.add(name);
+                }
+            }
+        }
+
+        for (const sentinel of IMPORTED_SENTINELS) {
+            expect(importedNames.has(sentinel), `${sentinel} is not imported from core by any client chunk`).toBe(true);
+        }
+
+        for (const sentinel of NEVER_DEFINED_SENTINELS) {
+            // `$N` suffix included: a re-inlined copy colliding with the import gets renamed by the
+            // bundler. The `[=(]` tail covers both plain bindings and function-form definitions.
+            const definition = new RegExp(`\\b(?:const|let|var|function)\\s+${sentinel}(?:\\$\\d+)?\\s*[=(]`);
+            for (const { chunk, source } of sources) {
+                expect(
+                    source,
+                    `client dist/${chunk} DEFINES ${sentinel} instead of importing it from @modelcontextprotocol/core — the neutral schema modules were re-inlined`
+                ).not.toMatch(definition);
+            }
+        }
+    });
+});

packages/client/test/helpers/ensureBuilt.ts

@@ -0,0 +1,98 @@
+import { execFile } from 'node:child_process';
+import { existsSync, mkdirSync, renameSync, rmSync, statSync } from 'node:fs';
+import { basename, join } from 'node:path';
+import { setTimeout as sleep } from 'node:timers/promises';
+import { promisify } from 'node:util';
+
+const execFileAsync = promisify(execFile);
+
+/**
+ * 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[]> = {
+    // Include late-written artifacts (dts pass, CJS validators) so an
+    // interrupted build never satisfies the fast path.
+    client: ['index.mjs', 'stdio.mjs', 'validators/ajv.cjs', 'index.d.mts', 'index.d.cts'],
+    core: ['index.mjs', 'internal.mjs', 'index.d.mts', 'internal.d.mts']
+};
+
+/** The build is killed after this long; execFile's kill still runs our finally. */
+const BUILD_TIMEOUT_MS = 60_000;
+/** How long a waiter polls for another worker's build — outlasts BUILD_TIMEOUT_MS. */
+const WAIT_DEADLINE_MS = 90_000;
+/** A lock older than this belongs to a dead worker (finally never ran) — steal it. */
+const STALE_LOCK_MS = 120_000;
+
+/**
+ * Build a package's dist on demand, safely across parallel vitest workers.
+ *
+ * Vitest runs test files in separate worker processes, so two files that each
+ * "build if dist is missing" can race on a cold checkout: both see no dist,
+ * both spawn `pnpm build`, and tsdown's clean step deletes files out from
+ * under whichever worker is already reading them. This helper makes the build
+ * single-flight with an atomic mkdir lock: the first worker builds, everyone
+ * else waits for the sentinel files to appear and the lock to clear, and once
+ * the dist exists nobody ever rebuilds (so no later clean can wipe it
+ * mid-read).
+ */
+export async function ensureBuilt(pkgDir: string): Promise<void> {
+    const sentinels = DIST_SENTINELS[basename(pkgDir)];
+    if (!sentinels) {
+        throw new Error(`No dist sentinels registered for ${pkgDir} — add the package to DIST_SENTINELS`);
+    }
+    const lockDir = join(pkgDir, '.dist-build-lock');
+    const haveAll = () => sentinels.every(s => existsSync(join(pkgDir, 'dist', s)));
+    const deadline = Date.now() + WAIT_DEADLINE_MS;
+    for (;;) {
+        if (haveAll() && !existsSync(lockDir)) return;
+        try {
+            mkdirSync(lockDir);
+        } catch (err) {
+            if ((err as NodeJS.ErrnoException).code !== 'EEXIST') throw err;
+            // Another worker holds the lock. If the holder died without its
+            // finally running (SIGKILL/OOM), the lock never clears — steal it
+            // once it is older than any live build could be.
+            try {
+                if (Date.now() - statSync(lockDir).mtimeMs > STALE_LOCK_MS) {
+                    // renameSync is atomic: exactly one waiter steals; losers throw and re-check.
+                    const graveyard = `${lockDir}.stale-${process.pid}-${Date.now()}`;
+                    renameSync(lockDir, graveyard);
+                    rmSync(graveyard, { recursive: true, force: true });
+                    continue;
+                }
+            } catch {
+                continue; // lock vanished or another waiter stole it — re-check now
+            }
+            if (Date.now() > deadline) {
+                throw new Error(
+                    `Timed out waiting for ${pkgDir}/dist (${sentinels.join(', ')}) ` +
+                        `while another worker held the build lock at ${lockDir}`
+                );
+            }
+            await sleep(250);
+            continue;
+        }
+        try {
+            if (!haveAll()) {
+                try {
+                    await execFileAsync('pnpm', ['build'], {
+                        cwd: pkgDir,
+                        timeout: BUILD_TIMEOUT_MS,
+                        maxBuffer: 16 * 1024 * 1024
+                    });
+                } catch (err) {
+                    const stderr = (err as { stderr?: string }).stderr ?? '';
+                    throw new Error(`pnpm build failed in ${pkgDir}: ${(err as Error).message}\n${stderr.slice(-2000)}`);
+                }
+            }
+            return;
+        } finally {
+            rmSync(lockDir, { recursive: true, force: true });
+        }
+    }
+}

packages/client/tsconfig.json

@@ -6,6 +6,7 @@
         "paths": {
             "*": ["./*"],
             "@modelcontextprotocol/core-internal": ["./node_modules/@modelcontextprotocol/core-internal/src/index.ts"],
+            "@modelcontextprotocol/core/internal": ["./node_modules/@modelcontextprotocol/core/src/internal.ts"],
             "@modelcontextprotocol/core-internal/public": [
                 "./node_modules/@modelcontextprotocol/core-internal/src/exports/public/index.ts"
             ],

packages/client/tsdown.config.ts

@@ -34,5 +34,8 @@ export default defineConfig({
         }
     },
     noExternal: ['@modelcontextprotocol/core-internal', 'ajv', 'ajv-formats', '@cfworker/json-schema'],
-    external: ['@modelcontextprotocol/client/_shims']
+    // The schema modules live in @modelcontextprotocol/core (a real runtime dependency); the
+    // bundled core-internal shims import them via the './internal' subpath, which must stay an
+    // external import (explicit entry — the tsconfig paths alias would otherwise inline it).
+    external: ['@modelcontextprotocol/client/_shims', '@modelcontextprotocol/core/internal']
 });