-
Notifications
You must be signed in to change notification settings - Fork 2k
perf: resolve schema modules from @modelcontextprotocol/core instead of inlining them #2459
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
f59ff4d
6ac1d06
9443e62
afcf255
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,12 @@ | ||
| --- | ||
| '@modelcontextprotocol/client': minor | ||
| '@modelcontextprotocol/server': minor | ||
| '@modelcontextprotocol/server-legacy': minor | ||
| --- | ||
|
|
||
| Resolve the spec and OAuth schema modules from `@modelcontextprotocol/core` at | ||
| runtime instead of inlining a copy into each package's bundle. `client`, | ||
| `server`, and `server-legacy` gain a runtime dependency on | ||
| `@modelcontextprotocol/core`, and their builds mark it external so the schema | ||
| definitions ship once instead of once per package. Public import paths and the | ||
| exported API are unchanged. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,53 @@ | ||
| import { execFileSync, spawnSync } from 'node:child_process'; | ||
| import { existsSync, readdirSync, readFileSync } from 'node:fs'; | ||
| import { dirname, join } from 'node:path'; | ||
| import { fileURLToPath, pathToFileURL } from 'node:url'; | ||
|
|
||
| import { beforeAll, describe, expect, test } from 'vitest'; | ||
|
|
||
| const pkgDir = join(dirname(fileURLToPath(import.meta.url)), '../..'); | ||
| const distDir = join(pkgDir, 'dist'); | ||
| const coreDir = join(pkgDir, '..', 'core'); | ||
| const CORE_IMPORT = /\bfrom\s+["']@modelcontextprotocol\/core["']/; | ||
|
|
||
| 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'))); | ||
| } | ||
|
Check warning on line 18 in packages/client/test/client/externalizedCoreBoundary.test.ts
|
||
|
Comment on lines
+13
to
+18
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 The new consumer-side boundary guard is applied to only one of the three packages that received the externalize plugin: Extended reasoning...The gap. This PR's safety mechanism for the new runtime boundary — plain-node importing every built chunk that names |
||
|
|
||
| // Consumer side of the externalize-core-schemas boundary (see | ||
| // packages/core-internal/externalizeCoreSchemas.tsdown.mjs): the built package must resolve the | ||
| // schema modules from @modelcontextprotocol/core at runtime, and everything it imports from there | ||
| // must actually exist in core. The build-time assertions in the plugin guard the producer side; | ||
| // this test pins the consumer side of the published contract. | ||
| describe('@modelcontextprotocol/client dist resolves schemas from @modelcontextprotocol/core', () => { | ||
| 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); | ||
|
Check warning on line 33 in packages/client/test/client/externalizedCoreBoundary.test.ts
|
||
|
Comment on lines
+26
to
+33
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 The build-on-demand Extended reasoning...What the bug is. This PR gives Why the builds run concurrently. The shared Step-by-step failure scenario.
Why nothing prevents it. There is no lock, marker file, or Scope caveat (what is not broken). The cross-package variant of this concern — the boundary test's second hook building Impact. A new probabilistic flake vector in the per-PR CI gate. It requires the two files' How to fix. Any of: (a) move the dist build into a vitest |
||
|
|
||
| test('built ESM chunks import the schemas from @modelcontextprotocol/core instead of inlining them', () => { | ||
| expect(chunksImportingCore().length).toBeGreaterThan(0); | ||
| }); | ||
|
|
||
| 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); | ||
|
Check warning on line 52 in packages/client/test/client/externalizedCoreBoundary.test.ts
|
||
|
Comment on lines
+38
to
+52
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 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 Extended reasoning...What the gap is. Why the test's comment overstates its coverage. The comment at The concrete code path that fails.
Two realistic triggers. (a) Version skew: a future PR adds a schema to 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 How to fix (cheap). Either or both: (1) make the registration loop eager-fail — in 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. |
||
| }); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,16 @@ | ||
| import type { Rolldown } from 'tsdown'; | ||
|
|
||
| /** | ||
| * Absolute paths of the core-internal schema source modules that are published verbatim as | ||
| * @modelcontextprotocol/core (types/schemas.ts + shared/auth.ts). Single source of truth for | ||
| * {@link externalizeCoreSchemas}; keep in sync with the aliases in packages/core/tsdown.config.ts. | ||
| */ | ||
| export declare const CORE_SCHEMA_MODULES: readonly string[]; | ||
|
|
||
| /** | ||
| * tsdown/rolldown plugin that rewrites every import resolving to one of {@link CORE_SCHEMA_MODULES} into an external | ||
| * import of '@modelcontextprotocol/core', so built packages share ONE evaluated copy of the spec + OAuth Zod schema | ||
| * graph per application. Asserts at build time, in both directions, that the rewrite actually happened — see the | ||
| * implementation in externalizeCoreSchemas.tsdown.mjs for the full rationale. | ||
| */ | ||
| export declare function externalizeCoreSchemas(): Rolldown.Plugin; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,108 @@ | ||
| // Shared tsdown build plugin. Plain .mjs (not .ts) because tsdown's config loader transpiles only | ||
| // the entry tsdown.config.ts itself and native-imports everything else, and Node 20 cannot import | ||
| // TypeScript sources. Types for consumers live in externalizeCoreSchemas.tsdown.d.mts. | ||
| import { existsSync } from 'node:fs'; | ||
| import path from 'node:path'; | ||
| import { fileURLToPath } from 'node:url'; | ||
|
|
||
| const HERE = path.dirname(fileURLToPath(import.meta.url)); | ||
|
|
||
| /** | ||
| * core-internal's two schema source modules are published verbatim as @modelcontextprotocol/core (see | ||
| * packages/core/tsdown.config.ts). The externalizeCoreSchemas() tsdown plugin rewrites every import that RESOLVES to | ||
| * one of them — whatever the import specifier looked like — into an external import of '@modelcontextprotocol/core', | ||
| * so built packages resolve the schemas from that package at runtime instead of inlining yet another copy. That keeps | ||
| * ONE evaluated copy of the spec + OAuth Zod schema graph per application, however many SDK packages it uses. | ||
| * | ||
| * Safe because core exports every name that crosses this boundary at runtime: SDK-internal helper schemas live in | ||
| * separate core-internal modules (e.g. types/listChangedOptions.ts) and remain bundled, and the remaining | ||
| * internal-only exports of the two modules are imported type-only (client's externalizedCoreBoundary.test.ts checks | ||
| * the built output against core's real export surface). Type declarations still inline the types via each consumer's | ||
| * dts `paths` mapping — only runtime duplication costs. | ||
| * | ||
| * @type {readonly string[]} | ||
| */ | ||
| export const CORE_SCHEMA_MODULES = [ | ||
| path.join(HERE, 'src', 'types', 'schemas.ts'), // MCP spec schemas | ||
| path.join(HERE, 'src', 'shared', 'auth.ts') // OAuth/OpenID schemas | ||
| ]; | ||
|
|
||
| const CORE_INTERNAL_SRC = path.join(HERE, 'src') + path.sep; | ||
|
|
||
| /** | ||
| * Resolvers may decorate an id beyond the plain file path (`\0` virtual-module prefix, `?query` suffix). | ||
| * | ||
| * @param {string} id | ||
| * @returns {string} | ||
| */ | ||
| function undecorate(id) { | ||
| return id.replace(/^\0/, '').replace(/\?.*$/, ''); | ||
| } | ||
|
|
||
| /** | ||
| * Rewrites imports of {@link CORE_SCHEMA_MODULES} to external '@modelcontextprotocol/core' imports, and fails the | ||
| * build when the rewrite cannot have worked, in both directions: | ||
| * | ||
| * - a listed module that does not exist on disk, or is never rewritten by a build that emits runtime chunks bundling | ||
| * core-internal sources, fails the build — otherwise renaming or moving a schema module would silently re-inline | ||
| * the whole schema graph into every consumer; | ||
| * - a listed module that still ends up in an emitted runtime chunk fails the build — the rewrite was bypassed, e.g. | ||
| * by a resolver id decoration the comparison does not recognize. | ||
| * | ||
| * The checks look at emitted runtime chunks (.mjs/.cjs), not the raw module graph, because the d.ts pass loads the | ||
| * schema .ts sources on purpose: type declarations inline the types via each consumer's dts `paths` mapping. | ||
| * | ||
| * @returns {import('tsdown').Rolldown.Plugin} | ||
| */ | ||
| export function externalizeCoreSchemas() { | ||
| const rewrites = new Map(CORE_SCHEMA_MODULES.map(module => [module, 0])); | ||
| return { | ||
| name: 'externalize-core-schemas', | ||
| buildStart() { | ||
| for (const module of CORE_SCHEMA_MODULES) { | ||
| if (!existsSync(module)) { | ||
| this.error( | ||
| `externalize-core-schemas: listed schema module ${module} does not exist. ` + | ||
| `If it moved, update CORE_SCHEMA_MODULES in packages/core-internal/externalizeCoreSchemas.tsdown.mjs ` + | ||
| `and the aliases in packages/core/tsdown.config.ts, which publish the same modules.` | ||
| ); | ||
| } | ||
| } | ||
| }, | ||
| async resolveId(source, importer, options) { | ||
| if (importer === undefined) return null; | ||
| const resolved = await this.resolve(source, importer, options); | ||
| if (resolved === null) return null; | ||
| const id = path.normalize(undecorate(resolved.id)); | ||
| const count = rewrites.get(id); | ||
| if (count === undefined) return null; | ||
| rewrites.set(id, count + 1); | ||
| return { id: '@modelcontextprotocol/core', external: true }; | ||
| }, | ||
| generateBundle(_outputOptions, bundle) { | ||
| const bundledIds = new Set( | ||
| Object.values(bundle) | ||
| .filter(output => output.type === 'chunk' && !/\.d\.[cm]?ts$/.test(output.fileName)) | ||
| .flatMap(chunk => chunk.moduleIds.map(id => path.normalize(undecorate(id)))) | ||
| ); | ||
| // Outputs with no core-internal runtime code (e.g. the d.ts pass) prove nothing about the rewrite. | ||
| if (![...bundledIds].some(id => id.startsWith(CORE_INTERNAL_SRC))) return; | ||
| const missed = CORE_SCHEMA_MODULES.filter(module => rewrites.get(module) === 0); | ||
| if (missed.length > 0) { | ||
| this.error( | ||
| `externalize-core-schemas: no import resolved to ${missed.join(' or ')}, ` + | ||
| `so nothing was externalized from there and the schema graph would be inlined. ` + | ||
| `If the module was renamed or split, update CORE_SCHEMA_MODULES in ` + | ||
| `packages/core-internal/externalizeCoreSchemas.tsdown.mjs and packages/core/tsdown.config.ts.` | ||
| ); | ||
| } | ||
| const inlined = CORE_SCHEMA_MODULES.filter(module => bundledIds.has(module)); | ||
| if (inlined.length > 0) { | ||
| this.error( | ||
| `externalize-core-schemas: ${inlined.join(' and ')} reached an emitted runtime chunk ` + | ||
| `despite being listed for externalization — the resolveId rewrite was bypassed.` | ||
| ); | ||
| } | ||
| } | ||
| }; | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,31 @@ | ||
| import * as z from 'zod/v4'; | ||
|
|
||
| /** | ||
| * Base schema for list changed subscription options (without callback). | ||
| * Used internally for Zod validation of `autoRefresh` and `debounceMs`. | ||
| * | ||
| * Lives outside `schemas.ts` deliberately: it is an SDK option schema, not a spec | ||
| * schema, so it is not part of `@modelcontextprotocol/core`'s public surface — | ||
| * and `schemas.ts` is resolved from that package at runtime by the client/server | ||
| * bundles, which can only see the names core actually exports. | ||
| */ | ||
| export const ListChangedOptionsBaseSchema = z.object({ | ||
| /** | ||
| * If `true`, the list will be refreshed automatically when a list changed notification is received. | ||
| * The callback will be called with the updated list. | ||
| * | ||
| * If `false`, the callback will be called with `null` items, allowing manual refresh. | ||
| * | ||
| * @default true | ||
| */ | ||
| autoRefresh: z.boolean().default(true), | ||
| /** | ||
| * Debounce time in milliseconds for list changed notification processing. | ||
| * | ||
| * Multiple notifications received within this timeframe will only trigger one refresh. | ||
| * Set to `0` to disable debouncing. | ||
| * | ||
| * @default 300 | ||
| */ | ||
| debounceMs: z.number().int().nonnegative().default(300) | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -21,7 +21,7 @@ import * as schemas from './schemas'; | |
| * Explicit allowlist of protocol Zod schemas that correspond to a public spec type in `types.ts`. | ||
| * | ||
| * This intentionally excludes internal helper schemas exported from `schemas.ts` that have no | ||
| * matching public type (e.g. `ListChangedOptionsBaseSchema`, `BaseRequestParamsSchema`, | ||
| * matching public type (e.g. `BaseRequestParamsSchema`, | ||
| * `NotificationsParamsSchema`, `ClientTasksCapabilitySchema`, `ServerTasksCapabilitySchema`). | ||
|
Comment on lines
+24
to
25
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 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 |
||
| * Keeping the list explicit means new public spec types must be added here deliberately, and | ||
| * internals never leak into `SpecTypeName`. | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,38 @@ | ||
| import { execFileSync } from 'node:child_process'; | ||
| import { existsSync } from 'node:fs'; | ||
| import { dirname, join } from 'node:path'; | ||
| import { fileURLToPath } from 'node:url'; | ||
| import { beforeAll, describe, expect, test } from 'vitest'; | ||
|
|
||
| const pkgDir = join(dirname(fileURLToPath(import.meta.url)), '..'); | ||
| const distEntry = join(pkgDir, 'dist', 'index.mjs'); | ||
|
|
||
| // client, server, and server-legacy resolve their schema modules from this | ||
| // package's dist entry at runtime, so a missing or incomplete build breaks | ||
| // every consumer of a built package (the repo's CI test job never runs a | ||
| // standalone build — dist only exists if a test builds it, the same way | ||
| // client's barrelClean.test.ts builds client). Build on demand and pin the | ||
| // runtime surface the boundary depends on. | ||
| describe('@modelcontextprotocol/core dist entry', () => { | ||
| beforeAll(() => { | ||
| if (!existsSync(distEntry)) { | ||
| execFileSync('pnpm', ['build'], { cwd: pkgDir, stdio: 'inherit' }); | ||
| } | ||
| }, 120_000); | ||
|
|
||
| test('root entry resolves and exposes the runtime schema surface', async () => { | ||
| const mod = await import(distEntry); | ||
| for (const name of [ | ||
| 'CallToolResultSchema', | ||
| 'InitializeRequestSchema', | ||
| 'JSONRPCMessageSchema', | ||
| 'OAuthMetadataSchema', | ||
| 'OAuthProtectedResourceMetadataSchema' | ||
| ]) { | ||
| expect(mod[name], `core dist entry must export ${name}`).toBeDefined(); | ||
| } | ||
| // The externalized boundary re-exports the whole spec schema surface; | ||
| // a partial build would show up as a collapsed export count. | ||
| expect(Object.keys(mod).length).toBeGreaterThan(150); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔴 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-schemasplugin makes the publishedclient/server/server-legacybundles emit real named ESM imports from@modelcontextprotocol/core(~32 named imports plus namespace access to all ~160 spec schema keys via the bundledspecTypeSchema.ts). Under Node's ESM link-time validation, a single name missing from core's dist is not a lazy failure — it isSyntaxError: 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.jsonhas"fixed": []and"linked": []— no lockstep group covers core + its three new consumers.updateInternalDependencies: patchonly propagates in the dependency→dependent direction (when core itself bumps); it never forces a core bump when a consumer bumps.core-internalisprivate: trueand invisible to changesets, so edits to the externalized source modules (core-internal/src/types/schemas.ts,core-internal/src/shared/auth.ts) prompt nothing. Andworkspace:^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.tssyncscore/src/index.tswith core-internal source; the newdistEntry.test.tsbuilds core from current source; CI test jobs and this PR's packed-tarball smoke resolvelink:../corefrom 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) addsNewThingSchematocore-internal/src/types/schemas.ts, adds it toSPEC_SCHEMA_KEYSandcore/src/index.ts(the drift test incoreSchemas.test.tsforces the source sync), and uses it at runtime inpackages/client.\n2. The author adds a changeset forclientonly — 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 publishesclient@2.0.0-beta.Nwith dependency^2.0.0-beta.3while core is not republished.\n4. Every freshnpm install @modelcontextprotocol/clientresolves the newest published core (2.0.0-beta.3, missingNewThingSchema), 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: [], andserver-legacyis at2.0.0-beta.2while core/client are at2.0.0-beta.3today); 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) addcore,client,server,server-legacyto a changesetsfixed(or at leastlinked) 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/coreimport names against the exports of the latest published core tarball. Option (a) is a one-line config change and closes the gap entirely.