Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 18 additions & 16 deletions .claude/skills/local-dev/SKILL.md
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ npm run web:dev # web launcher in --dev mode (Vite)
Individual builds: `build:web`, `build:cli`, `build:mcpdo`, `build:tui`,
`build:launcher`. The
web build produces both the browser SPA (`clients/web/dist`, Vite) and the Node
prod-server runner (`clients/web/build`, tsup).
prod-server runner (`clients/web/build`, tsdown).

To run the CLI or TUI: `node clients/launcher/build/index.js --cli …` /
`--tui …`. The connection CLI (`mcpdo`) has its own bin:
Expand All @@ -73,18 +73,17 @@ To run the CLI or TUI: `node clients/launcher/build/index.js --cli …` /
`package.json`** — it is not published on its own. Each client bundles it via a
build-time alias:

- **TUI / mcpdo** — `esbuildOptions.alias` in `tsup.config.ts` maps
- **CLI / TUI / mcpdo** — a top-level `alias` in `tsdown.config.ts` maps
`@inspector/core` → the repo `core/` directory, with
`noExternal: [/^@inspector\/core/]` inlining it.
- **CLI** — the same two settings under their tsdown names in
`tsdown.config.ts`: a top-level `alias`, and `deps.alwaysBundle` (#1492).
Its externals are `deps.neverBundle`, where a bare string matches the exact
specifier only — the config expands each name to cover its subpaths.
`deps.alwaysBundle: [/^@inspector\/core/]` inlining it (#1492, #2797).
Externals are `deps.neverBundle`, where a bare string matches the exact
specifier only — each config expands every name in its `EXTERNALS` array to
cover its subpaths.
- **Web** — **two** configs, because the client is two builds:
`clients/web/vite.config.ts` for the browser app, and
`clients/web/tsup.runner.config.ts` for the Node backend runner, which
`clients/web/tsdown.runner.config.ts` for the Node backend runner, which
defines the alias itself. Dependency or debugging work on the runner belongs
in the tsup config, not the Vite one (Copilot).
in the tsdown config, not the Vite one (Copilot).

## Where a dependency goes

Expand Down Expand Up @@ -182,7 +181,7 @@ toolchain". `tsx`, `playwright`, `storybook`, `happy-dom`, `ink-testing-library`
`vite-node` and each client's own `@types/*` are toolchain too and stay where
they are — hoisting them would make every client install the union of all five.
So do the ones **more than one** client declares without all of them doing so:
`tsup` sits in web, mcpdo and tui, and `vite` in web and tui on top of the root
`tsdown` sits in web, cli, mcpdo and tui, and `vite` in web and tui on top of the root
_runtime_ `dependency` that `--web --dev` needs. Neither is in scope here;
whether to consolidate them is a separate call with a separate rationale (`vite`
especially, since its root declaration is a `dependency`, not a
Expand Down Expand Up @@ -274,7 +273,7 @@ Two live examples worth knowing:

### Why a root dependency must also be externalised

tsup, tsdown and Vite externalise what the **client's** `package.json` declares, and a
tsdown and Vite externalise what the **client's** `package.json` declares, and a
root-only dependency is in none of them — so it is **bundled**, silently. For a
CJS package inlined into an ESM bundle that is fatal: esbuild leaves a
`Dynamic require of "path" is not supported` shim that throws at _import_ time,
Expand Down Expand Up @@ -311,7 +310,7 @@ a `createRequire` banner). ⚠️ **Never justify that exemption by a peer range
it briefly read "its `">=19"` peer keeps npm honest", which is false: a consumer
pinning React 19.0 satisfies `">=19"` while a narrower range of ours nests
underneath. What makes it safe is the _root `react` range staying open to the
whole major_, so npm can dedupe. `clients/tui/__tests__/tsupConfig.test.ts`
whole major_, so npm can dedupe. `clients/tui/__tests__/tsdownConfig.test.ts`
enforces the whole split, the exemption included.

### Why a version skew is worth aligning rather than working around
Expand All @@ -338,14 +337,17 @@ an `overrides` entry in that install (see the next section).

### Why `overrides` beats `npm audit fix`

`tsup@8.5.1` declares `esbuild: ^0.27.0`, and the advisory covers
`tsup@8.5.1` declared `esbuild: ^0.27.0`, and the advisory covers
`0.27.3 - 0.28.0` with `0.27.7` the last 0.27.x — so there is no _upward_ escape
inside that range, and `npm audit fix` "resolves" it by silently **downgrading**
to `0.27.2` across three installs (~700 lines of lockfile churn for a low-severity
dev-only advisory; tried and reverted in #2058). The override forces one deduped
copy above the range instead.

Two costs: it puts `tsup` on an esbuild **major it does not declare**, so
Two costs: it puts a consumer on an esbuild **major it does not declare**, so
`npm run build` — not `npm audit` — is the real gate on such a pin; and it is
invisible to the audit once clean, so **drop it when `tsup` widens its range**
rather than carrying it forever.
invisible to the audit once clean, so **drop it when no consumer's range
admits the vulnerable versions** rather than carrying it forever. tsup is gone
(#1492, #2797), and mcpdo and the CLI dropped the override with it, but web
and tui still carry it: Vite (both) and Storybook (web) accept `^0.27.0`,
which still admits the advisory range.
2 changes: 1 addition & 1 deletion .claude/skills/project-structure/SKILL.md
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ inspector/
│ │ ├── server/ Node-only dev/prod backend wiring (see below)
│ │ └── static/ sandbox_proxy.html — served for the MCP Apps tab
│ ├── cli/ Scriptable CLI (tsdown bundle, @inspector/core alias)
│ ├── tui/ Ink + React terminal UI (tsup bundle)
│ ├── tui/ Ink + React terminal UI (tsdown bundle)
│ └── launcher/ The `mcp-inspector` bin; dispatches to web/cli/tui in-process
├── core/ Shared code, consumed via the `@inspector/core` alias (no package.json)
├── test-servers/ Composable MCP test servers + JSON configs used by tests and by hand
Expand Down
2 changes: 1 addition & 1 deletion .claude/skills/release/SKILL.md
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ Anything it reports is fixed **deliberately** — a direct bump, or an
[Dependency placement](../../../AGENTS.md#dependency-placement) rules it out,
and the reason is not `--force`: plain `audit fix` resolves an advisory that has
no *upward* escape inside a declared range by silently **downgrading**. That is
not hypothetical here — `tsup@8.5.1` declares `esbuild: ^0.27.0` against an
not hypothetical here — `tsup@8.5.1` declared `esbuild: ^0.27.0` against an
advisory covering `0.27.3 - 0.28.0`, and `audit fix` walked three installs back
to `0.27.2` (~700 lines of lockfile churn for a low-severity dev-only advisory;
tried and reverted in #2058, written up in the `local-dev` skill). `local:gate`
Expand Down
10 changes: 5 additions & 5 deletions AGENTS.md
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ inspector/
│ ├── mcpdo/ Experimental connection CLI (`mcpdo` bin — connect once, many
│ │ commands; implicit Unix-socket connection daemon). Bundled
│ │ into the published package — see clients/mcpdo/README.md
│ ├── tui/ Ink + React terminal UI (tsup bundle)
│ ├── tui/ Ink + React terminal UI (tsdown bundle)
│ └── launcher/ The `mcp-inspector` bin; dispatches to web/cli/tui in-process
├── core/ Shared code, consumed via the `@inspector/core` alias (no package.json)
│ ├── auth/ OAuth end to end + the per-server SecretStore backends
Expand Down Expand Up @@ -107,11 +107,11 @@ The reasoning behind each of these, and what breaks when it is ignored, is the
- **A client declares only what that client alone consumes** — its own UI stack, its bundler-inlined packages, its dev tooling. `clients/cli`, `clients/mcpdo` and `clients/launcher` therefore declare **no** runtime dependencies at all, and that is the expected steady state, not an omission: everything they run on is root-declared and resolves by walk-up from the client directory. Re-adding a root-declared package to a client manifest re-creates the second copy this rule exists to make impossible (#1896), so a missing module at runtime is a signal to check the **root** manifest and the client's `external` list, never to add it back.
- **A package that moves to the root moves its `vitest.shared.mts` pin with it.** Left pointing at `<client>/node_modules` a pin resolves to a directory that no longer exists — or, where a transitive copy happens to sit there (`chokidar` under `vite`, `react` as a peer of `react-dom` and `ink`), to the very duplicate the pin list exists to prevent. **`react` and `react-dom` are the deliberate exception** and stay pinned per client, so a client's renderer and the React it calls into come from one install; every other root-owned pin resolves from the repo root.
- **`dependencies` vs `devDependencies` follows from who consumes it at runtime**, not from where it is declared. Anything `core/` imports at runtime must be a root **`dependency`** — the client builds externalize npm packages and a published install resolves them from the root manifest, where devDependencies are absent.
- **The shared toolchain is declared once, at the repo root, and in no client manifest.** `eslint`, `@eslint/js`, `typescript-eslint`, `globals`, `prettier`, `typescript`, `vitest`, `@vitest/coverage-v8` and `@types/node` are used by every client's own scripts, and a client that declares none of them still resolves the root copy by walk-up — `npm run` puts each ancestor `node_modules/.bin` on `PATH`, and Node and TypeScript walk parent `node_modules` / `node_modules/@types` the same way. `clients/launcher` declares no `devDependencies` at all and its `validate` is unchanged. A client-side declaration buys nothing and installs a second copy free to drift, as `globals` (`^17.7.0` root / `^17.4.0` clients) and `typescript-eslint` (`^8.65.0` / `^8.56.1`) had before #2196. These stay **`devDependencies`** — none is consumed at runtime and the tarball ships only each client's `build/`. The boundary is **used by every client**, not "used by one": anything narrower stays where it is, whether one client declares it (`tsx`, `playwright`, `storybook`, `happy-dom`, `ink-testing-library`, `vite-node`, each client's own `@types/*`) or several do — `tsup` is declared in web, mcpdo and tui (`tsdown` in cli, #1492), and `vite` in web and tui on top of the root **runtime** `dependency` that `--web --dev` needs. Those are out of scope here; consolidating them is a different call with a different rationale.
- **The shared toolchain is declared once, at the repo root, and in no client manifest.** `eslint`, `@eslint/js`, `typescript-eslint`, `globals`, `prettier`, `typescript`, `vitest`, `@vitest/coverage-v8` and `@types/node` are used by every client's own scripts, and a client that declares none of them still resolves the root copy by walk-up — `npm run` puts each ancestor `node_modules/.bin` on `PATH`, and Node and TypeScript walk parent `node_modules` / `node_modules/@types` the same way. `clients/launcher` declares no `devDependencies` at all and its `validate` is unchanged. A client-side declaration buys nothing and installs a second copy free to drift, as `globals` (`^17.7.0` root / `^17.4.0` clients) and `typescript-eslint` (`^8.65.0` / `^8.56.1`) had before #2196. These stay **`devDependencies`** — none is consumed at runtime and the tarball ships only each client's `build/`. The boundary is **used by every client**, not "used by one": anything narrower stays where it is, whether one client declares it (`tsx`, `playwright`, `storybook`, `happy-dom`, `ink-testing-library`, `vite-node`, each client's own `@types/*`) or several do — `tsdown` is declared in web, cli, mcpdo and tui (#1492, #2797), and `vite` in web and tui on top of the root **runtime** `dependency` that `--web --dev` needs. Those are out of scope here; consolidating them is a different call with a different rationale.
- ⚠️ **Deleting the declaration does not always delete the copy, and the local copy still wins.** npm auto-installs an unmet **peer** into the install that needs it, and it has no visibility into the root's tree — so a client-only ESLint plugin drags a client-local `eslint` in (`eslint-plugin-react-refresh`/`-storybook` in web, `eslint-plugin-react-hooks` in tui), and web's Storybook/Vitest stack drags in a local `typescript` and `vitest`. A hoisted transitive does the same: `@types/express` puts an `@types/node` in web and cli. Those copies sit _nearer_ than the root's and take precedence. The consolidation is therefore about **one declaration and one place to bump**, not about a single copy on disk. ⚠️ **Nothing keeps the surviving copies aligned automatically — but since #2226 the guard rejects the drift.** A **peer** copy is at least constrained by its holder's peer range — tightly for `vitest` (an exact peer, hence the pin below), loosely for `eslint` (`^9 || ^10`), where the copies agree only because npm resolves the same latest in both installs. A **transitive** copy is constrained by nothing of ours at all, and cli's `@types/node` (`24.13.1` against the root's `24.13.3`) diverged on exactly that. **That is detection, not alignment: `verify:dep-lockstep` fails on this class since #2226, and you still do the bump by hand.** Its second tier compares every package any install _declares_ (`dependencies`, `devDependencies`, `optionalDependencies`; not peers) against every top-level copy across all five installs, independent of what a `tsc` program loads, so a transitive drift and a peer shadow (`eslint`, `typescript`, `vitest`) are both in scope now. Two limits remain: the tier reads lockfiles, so a tool binary you installed by hand and never committed is still invisible; and it only compares names some manifest declares, so a purely transitive package no manifest names is out of scope in both tiers unless a `tsc` program loads both copies. Aligning a stale install is `npm update <pkg>` there; a transitive copy that will not move takes an `overrides` entry in that install (`clients/cli` pins `@types/node` this way).
- ⚠️ **`vitest`, `@vitest/coverage-v8` and web's `@vitest/browser-playwright` are pinned exactly, and move together.** `@vitest/browser-playwright` declares an **exact** peer on `vitest`, so it — not the root range — decides which `vitest` web installs. Left to float, the root resolves a newer patch and web's tests then run on one `vitest` while loading a coverage provider built against another. Bumping means editing all three in one change, the same discipline the exact `prettier` pin (#1790) exists for. ⚠️ **Editing the three is necessary but not sufficient — `clients/web` also carries a `vitest` `overrides` entry that has to move with them.** Web does not declare `vitest`, so its copy is the peer shadow above; its lockfile pins that copy at the old patch, and the exact peer plus the lockfile form a knot `npm install` resolves by refusing outright (`Conflicting peer dependency: vitest@<new>`), while `npm update` will not move it either. Deleting web's lockfile clears the error and re-resolves every caret range in the tree at once — an uncontrolled dependency update wearing a security patch's clothes. The `overrides` entry is the controlled alternative, the same mechanism `clients/cli` uses for `@types/node`: it moves the shadowed copy and nothing else, keeping the churn inside the vitest constellation. So a vitest bump is **four** edits, and the override's version is an exact pin like the other three (#2301).
- **A root-declared package that `core/` imports at runtime must also be named in all four bundler `external` lists** (`clients/cli/tsdown.config.ts`, `clients/{mcpdo,tui}/tsup.config.ts`, `clients/web/tsup.runner.config.ts`), since which client reaches it is a function of what `core/` imports rather than of what the client's own code names. `npm run verify:bundle-externals` enforces this against the **built output**.
- **A dependency that renders React components must be bundled** into the client that uses it (`noExternal`) and declared only there — an externalized one resolves its own `react` and splits the tree. `ink` is the single exemption, on cost, and it is only safe while the root `react` range stays open to the whole major (`^19.0.0`).
- **A root-declared package that `core/` imports at runtime must also be named in all four bundler `external` lists** (`clients/{cli,mcpdo,tui}/tsdown.config.ts`, `clients/web/tsdown.runner.config.ts`), since which client reaches it is a function of what `core/` imports rather than of what the client's own code names. `npm run verify:bundle-externals` enforces this against the **built output**.
- **A dependency that renders React components must be bundled** into the client that uses it (`deps.alwaysBundle`) and declared only there — an externalized one resolves its own `react` and splits the tree. `ink` is the single exemption, on cost, and it is only safe while the root `react` range stays open to the whole major (`^19.0.0`).
- **One version per install-crossing dependency.** When bumping a dependency the shared sources pull in, bump it in every install that declares it. Consolidating to the root is what makes most of these unbumpable in two places at once, but it does not retire the rule — a client's `devDependencies`, and any package that arrives transitively into a client install, can still skew against the root. Never raise the tsc heap to work around one. `npm run verify:dep-lockstep` enforces this in two tiers: packages that reach one `tsc` **program** from two installs (the #1896 heap-exhaustion class), and — since #2226 — every package any install **declares** that more than one install holds a top-level copy of, whether or not a program ever sees both.
- **Pin a transitive dependency with an `overrides` entry**, not with `npm audit fix` — which "resolves" an advisory with no upward escape by silently downgrading.

Expand Down Expand Up @@ -551,7 +551,7 @@ Pick by how many notifications the wait needs:

## Build output is never a gate target

**No gate — lint, format, or typecheck — may read generated output.** The gated surface is first-party source only: `clients/*/src`, `clients/*/__tests__`, `clients/web/{server,.storybook}`, each client's top-level configs, `core/`, `test-servers/src`, `scripts/`, and the root shared files. Everything a build writes is out of scope: `clients/*/build` (the tsup/tsdown/tsc bundles), `clients/web/dist` (the Vite SPA), `clients/web/storybook-static`, `clients/*/coverage`, `test-servers/build`, `core/**/{build,dist}`, and any `*.tsbuildinfo`. Each scope states this itself — `globalIgnores([...])` in every `eslint.config.js`, the `format`/`format:check` globs in each `package.json`, and a tsconfig `include` that names source directories rather than the package root.
**No gate — lint, format, or typecheck — may read generated output.** The gated surface is first-party source only: `clients/*/src`, `clients/*/__tests__`, `clients/web/{server,.storybook}`, each client's top-level configs, `core/`, `test-servers/src`, `scripts/`, and the root shared files. Everything a build writes is out of scope: `clients/*/build` (the tsdown/tsc bundles), `clients/web/dist` (the Vite SPA), `clients/web/storybook-static`, `clients/*/coverage`, `test-servers/build`, `core/**/{build,dist}`, and any `*.tsbuildinfo`. Each scope states this itself — `globalIgnores([...])` in every `eslint.config.js`, the `format`/`format:check` globs in each `package.json`, and a tsconfig `include` that names source directories rather than the package root.

Why it matters, given that these paths are all gitignored and the findings are usually warnings:

Expand Down
Loading
Loading