fix(tui): bundle React-rendering deps so a consumer install can't split React

.github/copilot-instructions.md

@@ -77,6 +77,7 @@ Both exist and do different jobs. Theme files (`src/theme/<Component>.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 `<client>/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

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

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 `<runner dir>/../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

clients/tui/README.md

@@ -102,7 +102,61 @@ The repo-root `validate:tui` just delegates here. `eslint.config.js` registers
 stricter react-hooks@7 rules are not enforced on the interim component surface
 (#1501).
 
-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.
+Tests live in `__tests__/`. The coverage gate covers **all of `src/**`**, React
+surface included — the Ink components mount through `ink-testing-library` (with
+the `ink-scroll-view` / `ink-form` passthrough doubles in `__tests__/helpers/`),
+`App.tsx` mounts against a mock of the `@inspector/core` surface, and keypresses
+are driven through stdin. The former interim exclusion of the components and
+`App.tsx` was lifted in #1501; the only exclusion left in `vitest.config.ts` is
+`src/tui-servers.ts`, a pure re-export of core's server resolver with no runtime
+statements of its own (its logic is measured in `core/` via the web suite, and
+`tui-servers.test.ts` still exercises it behaviorally — it is excluded only so it
+doesn't surface as a misleading 0/0 row).
+
+### 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.
+
+### The `ink-form` label patch
+
+Bundling `ink-form` also makes it patchable, which one label needs: the hint
+under an incomplete form reads "you have not **competed** yet". It is upstream's
+string, hardcoded in `ink-form/lib/SubmitButton.js` with no prop to override,
+and `ink-form` was last published in 2024 — so `tsup.config.ts` corrects it with
+an esbuild `onLoad` hook as the file enters the bundle. It is reported upstream
+as [lukasbach/ink-form#14](https://github.com/lukasbach/ink-form/issues/14); if
+a release ever carries the fix, drop the patch.
+
+The hook **throws** when the string isn't found rather than passing the source
+through. A silent no-op would let an `ink-form` upgrade retire the patch without
+anyone noticing it had stopped applying — or leave a patch aimed at a string
+that no longer exists. If the build fails there, check whether upstream fixed
+the typo and delete the patch instead of re-targeting it.