diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index 0628741c..24367041 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -77,6 +77,7 @@ 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 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. +- **A dependency that renders React components must be bundled into the client that uses it, and is then not a root dependency.** An externalized package resolves its own `react` from wherever npm placed it in the *consumer's* tree; `ink-form` and `ink-scroll-view` declare `react: ">=18"`, so a consumer's React 18 satisfied them and the TUI ended up with two React instances, crashing on the first hook (#1952). Inlining them (`noExternal` in `clients/tui/tsup.config.ts`) makes their `import "react"` resolve from the build directory like the bundle's own. `clients/tui/__tests__/tsupConfig.test.ts` enforces it; `ink` is the documented exception (unbundlable CJS `signal-exit@3`, and a `">=19"` peer). - **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 5f9e44bc..8cbfdcf5 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -126,6 +126,8 @@ The same **placement** rule covers anything reached only through **root-owned co - 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. +**A package a client build _inlines_ is not a runtime dependency, and is not declared at the root.** The published tarball carries its code inside that client's bundle, so declaring it at the root only makes consumers install a second, unused copy. `ink-form` and `ink-scroll-view` are the two today: both render React and both declare a loose `react` peer (`">=18"`), which let npm hoist them next to a consumer's React 18 while the Inspector's React 19 nested underneath — two React instances, and the TUI crashed on the first hook they called (#1952). They are inlined by `clients/tui/tsup.config.ts` (`noExternal`) and declared only in `clients/tui/package.json`, where the build resolves them. The general rule: **a dependency that renders React components must be bundled into the client that uses it**, so its `import "react"` resolves from the build directory alongside the bundle's own; `clients/tui/__tests__/tsupConfig.test.ts` enforces it, and `ink` is the documented exception (unbundlable — its CJS `signal-exit@3` fails ESM interop — and safe, since its `">=19"` peer keeps npm from misplacing it). + 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`. ## Contributing diff --git a/README.md b/README.md index 21b91d04..7f952494 100644 --- a/README.md +++ b/README.md @@ -310,6 +310,7 @@ The root `package.json` `"files"` allowlist is the source of truth for the tarba - **No source maps.** The client bundlers set `sourcemap: false` (`clients/{cli,tui}/tsup.config.ts`, `clients/web/tsup.runner.config.ts`); Vite and the launcher's `tsc` already emit none. Maps are ~half the unpacked size and aren't needed at runtime — debug via `npm run dev` on the source. - **`clients/web/build` ships via `clients/web/.npmignore`.** `clients/web/.gitignore` lists `build/`, and npm's packlist honors that nested `.gitignore` over the root `"files"` allowlist — so the prod web-server runner was silently missing from the tarball while `clients/web/dist` slipped through (its `.gitignore` only lists `dist-ssr`). `clients/web/.npmignore` overrides the `.gitignore` for publishing so both `build/` (runner) and `dist/` (SPA) ship. The other clients don't need this — none ship a nested `.gitignore`. - **`clients/web/static` ships the MCP Apps sandbox proxy.** `clients/web/static/sandbox_proxy.html` is a committed source file (not a build artifact), read from disk at runtime by `clients/web/server/sandbox-controller.ts` as `/../static/sandbox_proxy.html`. It was missing from the root `"files"` allowlist entirely, so every published build failed the Apps tab with **"Sandbox not loaded"** ([#1859](https://github.com/modelcontextprotocol/inspector/issues/1859)) while working fine in the repo. Because the path is resolved _relative to_ `clients/web/build`, the directory must ship at that exact location — `pack:verify` asserts both the tarball entry and the installed-on-disk path. +- **A dependency that renders React is bundled, not externalized.** An externalized package resolves its own `react` from wherever npm placed **it** in the consumer's tree, which is not necessarily where the bundle resolves ours. `ink-form` and `ink-scroll-view` declare a loose `react` peer (`">=18"`), so a project holding React 18 satisfies them and hoists them to its own root while the Inspector's React 19 nests underneath — two React copies, and the TUI dies with `TypeError: Cannot read properties of null (reading 'useState')` the moment a tool test form or a scroll view mounts ([#1952](https://github.com/modelcontextprotocol/inspector/issues/1952)). Both are therefore inlined by `clients/tui/tsup.config.ts` and are **not** root dependencies: the tarball ships their code inside `clients/tui/build/index.js` rather than having consumers install them. Bundling also pins their transitive deps to what this repo's install resolved (notably `ink-select-input@6` via `overrides`, which npm ignores for a package installed as a dependency). `ink` is the one React renderer left external — it can't be bundled (CJS `signal-exit@3` fails ESM interop) and its `">=19"` peer keeps npm from misplacing it. `clients/tui/__tests__/tsupConfig.test.ts` guards the split; see the [TUI README](./clients/tui/README.md#bundling-react-rendering-dependencies-must-be-inlined-1952). - **A single version number, read from the root `package.json`.** The Inspector ships as one package with one version, so only the **root** `package.json` carries a `version` — the four `clients/*/package.json`s deliberately have none. Every Node client (CLI, TUI, and the web backend) resolves the version through the shared `readInspectorVersion()` reader in `core/node/version.ts`, which walks up to the root manifest (always present in the tarball). No client `package.json` is read at runtime, so none needs to ship. The web **browser** can't read the filesystem; it gets its version from the backend via `GET /api/config` (see [#1639](https://github.com/modelcontextprotocol/inspector/issues/1639)). ### `npm run pack:verify` — publish smoke against the real tarball diff --git a/clients/tui/README.md b/clients/tui/README.md index 7a784a97..bcf102c3 100644 --- a/clients/tui/README.md +++ b/clients/tui/README.md @@ -106,3 +106,35 @@ Tests live in `__tests__/`. The coverage gate currently covers the TUI's non-React logic (server resolution, logger, tab metadata, and the `utils/` form/URL helpers); the Ink components and `App.tsx` are an interim exclusion in `vitest.config.ts` pending a renderer-based follow-up. + +### Bundling: React-rendering dependencies must be inlined (#1952) + +`tsup.config.ts` splits the TUI's dependencies into bundled (`noExternal`) and +externalized (`external`). For anything that **renders React components**, that +choice decides which React instance it gets at runtime, and getting it wrong +crashes the TUI in a way nothing in this repo can see. + +An external package's `import "react"` resolves from wherever npm placed **that +package** — and npm places it next to a React satisfying its own peer range. +`ink-form` and `ink-scroll-view` both accept `react: ">=18"`, so a project that +installs the Inspector alongside its own React 18 satisfies them: they hoist to +that project's root while the Inspector's React 19 nests underneath. The bundle +then renders through React 19 while those two call hooks on React 18, whose +dispatcher is null — so opening a tool test form (or any scroll view) dies with +`TypeError: Cannot read properties of null (reading 'useState')`. + +Bundling them removes npm from the decision: their `import "react"` is emitted +into `build/index.js`, so it resolves from the build directory exactly like the +bundle's own. It also pins their transitive deps to what *this* install +resolved — notably `ink-select-input@6` via the `overrides` entry, since npm +ignores a dependency's `overrides` and a consumer install would otherwise pull +the React-18-era v5 that `ink-form` asks for. + +`ink` itself stays external: it can't be bundled (its CJS `signal-exit@3` +dependency fails ESM interop with `Dynamic require of "assert" is not +supported`), and it doesn't need to be — its `react` peer is `">=19"`, which +keeps npm from hoisting it next to a React the Inspector couldn't also use. + +`__tests__/tsupConfig.test.ts` enforces this: every dependency declaring a +`react` peer must be in `noExternal` unless it is listed as external by design. +Add a React-rendering dependency, and that test tells you to bundle it. diff --git a/clients/tui/__tests__/tsupConfig.test.ts b/clients/tui/__tests__/tsupConfig.test.ts new file mode 100644 index 00000000..b66cf13b --- /dev/null +++ b/clients/tui/__tests__/tsupConfig.test.ts @@ -0,0 +1,130 @@ +/** + * Bundling invariant for the TUI's React-rendering dependencies (#1952). + * + * The published TUI is a single ESM bundle whose bare `import "react"` resolves + * from `clients/tui/build/`. Any package left *external* resolves its own + * `react` from wherever npm placed **it** instead — and npm places a package + * next to a version satisfying its declared peer range. `ink-form` and + * `ink-scroll-view` accept `react: ">=18"`, so a consumer project holding React + * 18 satisfies them, they hoist to that project's root, and the Inspector's + * React 19 nests beneath it. Two React copies later, the first hook either of + * them calls reads a null dispatcher and the TUI dies with + * "Cannot read properties of null (reading 'useState')" — the moment a tool + * test form or a scroll view mounts. + * + * Inlining them removes npm from the decision entirely: their `import "react"` + * is emitted into the bundle, so it resolves exactly where the bundle's does. + * + * This test is the durable guard, because the failure is invisible in the repo + * (a dev install has one React) and in every smoke (same) — it only appears + * once the package is installed *under* another project that renders React. + */ + +import { describe, it, expect } from "vitest"; +import { readFileSync } from "node:fs"; +import path from "node:path"; +import { fileURLToPath } from "node:url"; +import type { Options } from "tsup"; +import tsupConfig from "../tsup.config.js"; + +const clientDir = path.resolve( + path.dirname(fileURLToPath(import.meta.url)), + "..", +); + +/** + * `defineConfig()` is typed as a union — a single options object, an array of + * them, or a function returning either. The TUI config is the first form, so + * narrow to it at runtime rather than assuming: a future config that grows a + * second build should fail loudly here instead of silently checking nothing. + */ +function singleConfig(config: typeof tsupConfig): Options { + if (typeof config !== "object" || Array.isArray(config)) { + throw new Error( + "clients/tui/tsup.config.ts is expected to export one options object", + ); + } + return config; +} + +/** The string entries of an `external` / `noExternal` list (ours are all strings but the type allows RegExp). */ +function stringEntries(list: (string | RegExp)[] | undefined): string[] { + return (list ?? []).filter((entry) => typeof entry === "string"); +} + +/** + * The one React-rendering dependency deliberately left external. + * + * `ink` cannot be bundled — its CJS `signal-exit@3` dependency fails ESM + * interop ("Dynamic require of \"assert\" is not supported") — and does not + * need to be: its `react` peer is ">=19", so npm cannot hoist it next to a + * React the Inspector itself could not use. + */ +const EXTERNAL_BY_DESIGN = new Set(["ink"]); + +function readPackageJson(name: string): { + peerDependencies?: Record; +} { + // Resolve through the client's own node_modules rather than `require.resolve` + // on the package name: several of these packages export only their entry + // point, so `/package.json` is not a resolvable subpath. + const manifest = path.join(clientDir, "node_modules", name, "package.json"); + return JSON.parse(readFileSync(manifest, "utf8")); +} + +/** Dependencies that declare a `react` peer — i.e. that render React themselves. */ +function reactRenderingDependencies(): string[] { + const { dependencies } = JSON.parse( + readFileSync(path.join(clientDir, "package.json"), "utf8"), + ) as { dependencies: Record }; + + return Object.keys(dependencies).filter( + (name) => readPackageJson(name).peerDependencies?.react !== undefined, + ); +} + +const config = singleConfig(tsupConfig); +const noExternal = stringEntries(config.noExternal); +const external = stringEntries(config.external); + +describe("tui tsup config", () => { + it("finds the React-rendering dependencies to check", () => { + // A guard on the guard: if this resolves to nothing (a rename, a moved + // node_modules), every assertion below would pass vacuously. + expect(reactRenderingDependencies().length).toBeGreaterThan(0); + }); + + it("bundles every React-rendering dependency that can be bundled", () => { + const shouldInline = reactRenderingDependencies().filter( + (name) => !EXTERNAL_BY_DESIGN.has(name), + ); + + expect(shouldInline.length).toBeGreaterThan(0); + for (const name of shouldInline) { + expect( + noExternal, + `${name} renders React, so it must be bundled`, + ).toContain(name); + expect(external, `${name} must not be external`).not.toContain(name); + } + }); + + it("keeps react itself external, as the single shared instance", () => { + expect(external).toContain("react"); + expect(noExternal).not.toContain("react"); + }); + + it("documents each React-rendering package left external", () => { + for (const name of EXTERNAL_BY_DESIGN) { + expect(external, `${name} is external by design`).toContain(name); + } + }); + + it("verifies the deps it exempts are still declared", () => { + // If `ink` ever leaves the dependency list, the exemption above is stale + // and would silently excuse a future package that took its name. + for (const name of EXTERNAL_BY_DESIGN) { + expect(reactRenderingDependencies()).toContain(name); + } + }); +}); diff --git a/clients/tui/tsup.config.ts b/clients/tui/tsup.config.ts index 92a637b3..1a90e87f 100644 --- a/clients/tui/tsup.config.ts +++ b/clients/tui/tsup.config.ts @@ -15,12 +15,28 @@ export default defineConfig({ sourcemap: false, target: "node22", platform: "node", - noExternal: [/^@inspector\/core/], + // Every package here renders React components, so it MUST share the one + // React instance the bundle imports. Bundling them is what guarantees that: + // an inlined package's `import "react"` is emitted into build/index.js and so + // resolves from *this* directory, exactly like the bundle's own — no consumer + // install layout can point it somewhere else (#1952). Left external, npm is + // free to hoist them next to a *different* React: `ink-form` and + // `ink-scroll-view` declare a loose `react` peer (">=18"), so a consumer + // project holding React 18 satisfies it and gets them hoisted to its own root + // while the Inspector's React 19 nests under it — two React copies, and the + // first hook they call reads a null dispatcher ("Cannot read properties of + // null (reading 'useState')") the moment a tool test form or a scroll view + // mounts. `__tests__/tsupConfig.test.ts` guards this list. + noExternal: [/^@inspector\/core/, "ink-form", "ink-scroll-view"], external: [ + // `react` is deliberately external — it is the single instance everything + // above resolves to, from this build directory. "react", + // `ink` stays external too: it cannot be bundled (its CJS `signal-exit@3` + // dependency fails ESM interop with "Dynamic require of \"assert\""), and + // it does not need to be — its `react` peer is ">=19", which keeps npm + // from hoisting it next to a React the Inspector could not also use. "ink", - "ink-form", - "ink-scroll-view", "open", "commander", "pino", diff --git a/package-lock.json b/package-lock.json index 61e95289..972b65f2 100644 --- a/package-lock.json +++ b/package-lock.json @@ -24,8 +24,6 @@ "commander": "^13.1.0", "hono": "^4.12.18", "ink": "^6.0.0", - "ink-form": "^2.0.1", - "ink-scroll-view": "^0.3.6", "open": "^10.2.0", "pino": "^9.14.0", "react": "^19.2.4", @@ -2181,21 +2179,6 @@ } } }, - "node_modules/figures": { - "version": "6.1.0", - "resolved": "https://registry.npmjs.org/figures/-/figures-6.1.0.tgz", - "integrity": "sha512-d+l3qxjSesT4V7v2fh+QnmFnUWv9lSpjarhShNTgBOfA0ttejbQUAlHLitbjkoRiDulW0OPoQPYIGhIC8ohejg==", - "license": "MIT", - "dependencies": { - "is-unicode-supported": "^2.0.0" - }, - "engines": { - "node": ">=18" - }, - "funding": { - "url": "https://github.com/sponsors/sindresorhus" - } - }, "node_modules/file-entry-cache": { "version": "8.0.0", "resolved": "https://registry.npmjs.org/file-entry-cache/-/file-entry-cache-8.0.0.tgz", @@ -2552,76 +2535,6 @@ } } }, - "node_modules/ink-form": { - "version": "2.0.1", - "resolved": "https://registry.npmjs.org/ink-form/-/ink-form-2.0.1.tgz", - "integrity": "sha512-vo0VMwHf+HOOJo7026K4vJEN8xm4sP9iWlQLx4bngNEEY5K8t30CUvVjQCCNAV6Mt2ODt2Aq+2crCuBONReJUg==", - "license": "MIT", - "dependencies": { - "ink-select-input": "^5.0.0", - "ink-text-input": "^6.0.0" - }, - "peerDependencies": { - "ink": ">=4", - "react": ">=18" - } - }, - "node_modules/ink-scroll-view": { - "version": "0.3.7", - "resolved": "https://registry.npmjs.org/ink-scroll-view/-/ink-scroll-view-0.3.7.tgz", - "integrity": "sha512-lUBLxSbVry/+UtJhuvRu6wumP43+ScVp69J7e+hmYwz1kTkahWfkVwWOu7Mn1DMPb8AU8bnsmEsr6kvSJvnaRw==", - "license": "MIT", - "peerDependencies": { - "ink": "^5 || ^6 || ^7", - "react": "^18 || ^19" - } - }, - "node_modules/ink-select-input": { - "version": "6.2.0", - "resolved": "https://registry.npmjs.org/ink-select-input/-/ink-select-input-6.2.0.tgz", - "integrity": "sha512-304fZXxkpYxJ9si5lxRCaX01GNlmPBgOZumXXRnPYbHW/iI31cgQynqk2tRypGLOF1cMIwPUzL2LSm6q4I5rQQ==", - "license": "MIT", - "dependencies": { - "figures": "^6.1.0", - "to-rotated": "^1.0.0" - }, - "engines": { - "node": ">=18" - }, - "peerDependencies": { - "ink": ">=5.0.0", - "react": ">=18.0.0" - } - }, - "node_modules/ink-text-input": { - "version": "6.0.0", - "resolved": "https://registry.npmjs.org/ink-text-input/-/ink-text-input-6.0.0.tgz", - "integrity": "sha512-Fw64n7Yha5deb1rHY137zHTAbSTNelUKuB5Kkk2HACXEtwIHBCf9OH2tP/LQ9fRYTl1F0dZgbW0zPnZk6FA9Lw==", - "license": "MIT", - "dependencies": { - "chalk": "^5.3.0", - "type-fest": "^4.18.2" - }, - "engines": { - "node": ">=18" - }, - "peerDependencies": { - "ink": ">=5", - "react": ">=18" - } - }, - "node_modules/ink-text-input/node_modules/type-fest": { - "version": "4.41.0", - "resolved": "https://registry.npmjs.org/type-fest/-/type-fest-4.41.0.tgz", - "integrity": "sha512-TeTSQ6H5YHvpqVwBRcnLDCBnDOHWYu7IvGbHT6N8AOymcr9PJGjc1GTtiWZTYg0NCgYwvnYWEkVChQAr9bjfwA==", - "license": "(MIT OR CC0-1.0)", - "engines": { - "node": ">=16" - }, - "funding": { - "url": "https://github.com/sponsors/sindresorhus" - } - }, "node_modules/ip-address": { "version": "10.1.0", "resolved": "https://registry.npmjs.org/ip-address/-/ip-address-10.1.0.tgz", @@ -2732,18 +2645,6 @@ "integrity": "sha512-hvpoI6korhJMnej285dSg6nu1+e6uxs7zG3BYAm5byqDsgJNWwxzM6z6iZiAgQR4TJ30JmBTOwqZUw3WlyH3AQ==", "license": "MIT" }, - "node_modules/is-unicode-supported": { - "version": "2.1.0", - "resolved": "https://registry.npmjs.org/is-unicode-supported/-/is-unicode-supported-2.1.0.tgz", - "integrity": "sha512-mE00Gnza5EEB3Ds0HfMyllZzbBrmLOX3vfWoj9A9PEnTfratQ/BcaJOuMhnkhjXvb2+FkY3VuHqtAGpTPmglFQ==", - "license": "MIT", - "engines": { - "node": ">=18" - }, - "funding": { - "url": "https://github.com/sponsors/sindresorhus" - } - }, "node_modules/is-wsl": { "version": "3.1.1", "resolved": "https://registry.npmjs.org/is-wsl/-/is-wsl-3.1.1.tgz", @@ -4080,18 +3981,6 @@ "url": "https://github.com/sponsors/SuperchupuDev" } }, - "node_modules/to-rotated": { - "version": "1.0.0", - "resolved": "https://registry.npmjs.org/to-rotated/-/to-rotated-1.0.0.tgz", - "integrity": "sha512-KsEID8AfgUy+pxVRLsWp0VzCa69wxzUDZnzGbyIST/bcgcrMvTYoFBX/QORH4YApoD89EDuUovx4BTdpOn319Q==", - "license": "MIT", - "engines": { - "node": ">=18" - }, - "funding": { - "url": "https://github.com/sponsors/sindresorhus" - } - }, "node_modules/toidentifier": { "version": "1.0.1", "resolved": "https://registry.npmjs.org/toidentifier/-/toidentifier-1.0.1.tgz", diff --git a/package.json b/package.json index 05155e99..c09afb9d 100644 --- a/package.json +++ b/package.json @@ -91,8 +91,6 @@ "commander": "^13.1.0", "hono": "^4.12.18", "ink": "^6.0.0", - "ink-form": "^2.0.1", - "ink-scroll-view": "^0.3.6", "open": "^10.2.0", "pino": "^9.14.0", "react": "^19.2.4",