diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index ccf73747..8f049824 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -76,7 +76,8 @@ Both exist and do different jobs. Theme files (`src/theme/.ts`) custo - **The MCP SDK packages (`@modelcontextprotocol/client`, `core`, `server`, `server-legacy`, `ext-apps`) belong in the root `package.json` only.** Adding one to a `clients/*/package.json` is a review finding: Node resolution walks up, so the root install already serves every client, and a per-client entry installs a second copy that drifts (it produced two versions of `ext-apps`, and of the transitive v1 SDK, at once — #1970) and reintroduces the duplicate-copy failure `vitest.shared.mts` has a `dedupe` workaround for. - **The v1 `@modelcontextprotocol/sdk` is not a dependency of this repo** and must not become one. It is a peer of `ext-apps`, present in lock files only. -- Dependencies used only by **root-owned code with no manifest** (`test-servers/src`, `core/`) go in the root `devDependencies` and are aliased to the **repo root** in `vitest.shared.mts` — as `express` and `yaml` are — not to `/node_modules` like the other pins there. +- Dependencies reached only through **root-owned code with no manifest** (`test-servers/src`, `core/`) are declared at the root and aliased to the **repo root** in `vitest.shared.mts` — as `express` and `yaml` are — not to `/node_modules` like the other pins there. +- **Which section is a separate question from which manifest.** A package `core/` imports at runtime must be in root **`dependencies`**: client builds externalize npm packages, so a published install resolves them from the root manifest and devDependencies are absent there. Only test/build-only packages (`express`) belong in `devDependencies`. Flag a runtime `core/` import added to `devDependencies` — it passes every local check and breaks the published package. ## Tests and the coverage gate diff --git a/AGENTS.md b/AGENTS.md index 164baff8..31d0fbed 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -117,7 +117,13 @@ After installing, `npm run build` builds all clients. The launcher scripts (`npm **The MCP SDK packages — `@modelcontextprotocol/client`, `core`, `server`, `server-legacy`, `ext-apps` — are declared in the repo-root `package.json` and nowhere else.** Do not add them to a `clients/*/package.json`: Node resolution walks up, so the root install already serves every client, and a per-client declaration installs a *second copy* that drifts from the root's. That drift is not theoretical — it put two versions of `ext-apps` (1.7.4 / 1.7.5), and of the transitive v1 `@modelcontextprotocol/sdk` (1.29.0 / 1.30.0), in the tree at once (#1970), and a second copy of `client`/`core` is exactly the failure the `dedupe` + `server.deps.inline` workaround in `vitest.shared.mts` exists for. -The same rule covers anything reached only through **root-owned code that has no manifest of its own**: `express` (imported by `test-servers/src`) and `yaml` are root devDependencies, and `vitest.shared.mts` aliases both to the **repo root** rather than to `/node_modules` like its other pins. If you add a dependency to `test-servers/src` or `core/`, declare it at the root and alias it there too. +The same **placement** rule covers anything reached only through **root-owned code that has no manifest of its own** (`test-servers/src`, `core/`): declare it at the root, and alias it to the **repo root** in `vitest.shared.mts` rather than to `/node_modules` like that file's other pins. `express` and `yaml` are the two today — both reached through `test-servers/src` (express by the http/oauth servers, yaml by `load-config.ts`). + +**Placement and classification are separate questions.** Which *section* of the root manifest it goes in follows from who consumes it at runtime: + +- A package `core/` imports at runtime belongs in root **`dependencies`**. The client builds bundle `core/` but externalize npm packages, so a published install resolves them from the root manifest — and devDependencies are not installed for consumers, so a runtime import parked there breaks the published package while passing every local check. +- A package only the tests, the test servers, or the build tooling need belongs in **`devDependencies`** — `express`, added there by #1970. +- `yaml` is in `dependencies` today even though its only importer is `test-servers/src/load-config.ts`. Left as-is deliberately (moving it changes what ships, which is not a docs change); if you touch it, confirm no published path reads YAML first. The v1 SDK (`@modelcontextprotocol/sdk`) is **not** a dependency of this repo and must not become one — v2 uses the packages above. It appears in the lock files only as a `"peer": true` entry pulled in by `ext-apps`. diff --git a/README.md b/README.md index 8ae07537..1237e541 100644 --- a/README.md +++ b/README.md @@ -66,7 +66,7 @@ npm install # root install; postinstall cascades into every client The cascade (`scripts/install-clients.mjs`) is dev-only — it exits early when the package is installed as a dependency, and the published tarball ships only each client's `build/`, so end users are unaffected. Set `INSPECTOR_SKIP_CLIENT_INSTALL=1` to skip it. -**Where a dependency is declared.** The MCP SDK packages (`@modelcontextprotocol/client`, `core`, `server`, `server-legacy`, `ext-apps`) live in the **root** `package.json` only — never in a client's. Node resolution walks up, so the root install is on every client's chain, and the root manifest is already what the published tarball resolves against. Declaring them per client installs a second copy that can drift from the root's, which is how two versions of `ext-apps` (and of the transitive v1 `@modelcontextprotocol/sdk`) ended up in the tree before [#1970](https://github.com/modelcontextprotocol/inspector/issues/1970) — and a second copy of `client`/`core` is the failure `vitest.shared.mts` carries a `dedupe` workaround for. The same holds for anything used only by root-owned code with no manifest of its own: `express` (imported by `test-servers/src`) and `yaml` are root devDependencies, resolved from the root by `vitest.shared.mts`. +**Where a dependency is declared.** The MCP SDK packages (`@modelcontextprotocol/client`, `core`, `server`, `server-legacy`, `ext-apps`) live in the **root** `package.json` only — never in a client's. Node resolution walks up, so the root install is on every client's chain, and the root manifest is already what the published tarball resolves against. Declaring them per client installs a second copy that can drift from the root's, which is how two versions of `ext-apps` (and of the transitive v1 `@modelcontextprotocol/sdk`) ended up in the tree before [#1970](https://github.com/modelcontextprotocol/inspector/issues/1970) — and a second copy of `client`/`core` is the failure `vitest.shared.mts` carries a `dedupe` workaround for. The same root-only placement holds for anything reached solely through root-owned code with no manifest of its own (`test-servers/src`, `core/`), and `vitest.shared.mts` aliases those to the repo root — `express` and `yaml`, both reached through `test-servers/src`, are the two today. **Whether such a package is a `dependency` or a `devDependency` follows from who consumes it at runtime, not from where it is declared:** anything `core/` imports at runtime must be a root **`dependency`**, because the client builds externalize npm packages and a published install resolves them from the root manifest, where devDependencies are absent. `express` is test-only and is a devDependency; `yaml` currently sits in `dependencies`. ## Running during development diff --git a/vitest.shared.mts b/vitest.shared.mts index f52919cc..1ebbc96a 100644 --- a/vitest.shared.mts +++ b/vitest.shared.mts @@ -83,16 +83,16 @@ export function vitestSharedPaths(clientDir: string) { replacement: path.resolve(dirname, "node_modules/@napi-rs/keyring"), }, // `express` and `yaml` resolve from the **repo root**, unlike every pin - // above. Both are reached only through root-owned code that has no manifest - // of its own — `test-servers/src` imports express, and `core`/the root - // tooling uses yaml — so the root is where they are declared and the only - // place a client's resolution chain is guaranteed to find them. Pointing - // these at `/node_modules` is what broke when the MCP packages moved - // to the root (#1970): express was never declared by a client at all, it - // arrived in `clients/cli` as a peer of `express-rate-limit` under - // `@modelcontextprotocol/server-legacy`, so removing that manifest entry - // took express with it and every cli test that spawns a test server failed - // to resolve it. + // above. Both are reached only through `test-servers/src` — express by the + // http/oauth servers, yaml by `load-config.ts` — which is root-owned code + // with no manifest of its own, so the root is where they are declared and + // the only place a client's resolution chain is guaranteed to find them. + // Pointing these at `/node_modules` is what broke when the MCP + // packages moved to the root (#1970): express was never declared by a client + // at all, it arrived in `clients/cli` as a peer of `express-rate-limit` + // under `@modelcontextprotocol/server-legacy`, so removing that manifest + // entry took express with it and every cli test that spawns a test server + // failed to resolve it. { find: /^express$/, replacement: path.resolve(repoRoot, "node_modules/express"),