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
57 changes: 54 additions & 3 deletions .claude/skills/local-dev/SKILL.md
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ to tell you have hit one.
Work the rules in order: which manifest declares it, then `dependencies` vs
`devDependencies`, then whether the bundlers must also externalise it.

### Why the SDK packages are root-only
### Why `core/`'s runtime dependencies are root-only

A per-client declaration installs a **second copy** that drifts from the root's.
Not theoretical: it put two versions of `ext-apps` (1.7.4 / 1.7.5) and of the
Expand All @@ -98,8 +98,59 @@ transitive v1 `@modelcontextprotocol/sdk` (1.29.0 / 1.30.0) in the tree at once
`server.deps.inline` workaround in `vitest.shared.mts` exists for.

The same reasoning extends to anything reached only through root-owned code that
has no manifest of its own (`test-servers/src`, `core/`) — hence the repo-root
alias for those in `vitest.shared.mts`. `express` and `yaml` are the two today.
has no manifest of its own (`test-servers/src`, `core/`). #2195 made that the
general case: `ajv`, `atomically`, `chokidar`, `hono`, `@napi-rs/keyring`,
`pino`, `react` and `zod` moved to the root, joining `proper-lockfile` and
`undici`, which were already there.

Keep two distinctions straight, because AGENTS.md's rules split on them:

- **Root-declared is not the same as `core/`-imported.** `commander`, `open` and
`@hono/node-server` are root `dependencies` too, but they are reached only
from client code. Only the `core/` set has to appear in *all three* bundler
`external` lists.
- **Root-declared is not the same as aliased.** The `vitest.shared.mts` pins and
the `clients/web/tsconfig.*.json` `paths` cover the packages whose resolution
is genuinely ambiguous, which is two different situations: the importer is
`core/`, which has no manifest to resolve from, **or** a copy still sits in
the client install and would win. The second case is the one that is easy to
get wrong — `chokidar` (under `vite`), `zod` (under
`eslint-plugin-react-hooks`), `open` (under Storybook) and `react` (a peer of
`react-dom` and `ink`) are all present in a client's `node_modules` without
being declared there, so "we deleted the declaration" is not the same as "it
resolves from the root now". **Check the install, don't reason from the
manifest.** `ajv`, `commander` and `undici` carry no pin because no client
install has a copy of them at all.

The point of deleting the client-side copies rather than merely keeping them in
step is that **a package installs only into an install root that declares it**.
Aligned duplicate declarations still drift the next time someone bumps one of
them; no declaration at all cannot. `npm run verify:dep-lockstep` is the detector
for the skew, and consolidation is what removes the opportunity.

Two consequences that read as bugs and are not:

- **`clients/cli` and `clients/launcher` declare no runtime dependencies.** Their
manifests carry `devDependencies` only. Everything they import at runtime is
root-declared and resolves by walking up from the client directory.
- **A client's `node_modules` still contains some of these names.** They arrive
transitively (`chokidar` under `vite`, `react` as a peer of `react-dom` and
`ink`). That is why the `vitest.shared.mts` pins matter: an unpinned specifier
would resolve the nearest copy, which is the transitive one, not the root's.

When a package moves to the root, its `vitest.shared.mts` pin has to move with
it — a `path.resolve(dirname, …)` pin left behind points at a directory that no
longer exists, or (for the transitive cases above) at the duplicate the pin was
supposed to rule out.

**`react` and `react-dom` are the exception, and stay pinned per client.** They
are a matched pair — `react-dom` reaches into React internals — and `react-dom`
is still web-declared, so npm resolves it and its React peer together inside
`clients/web/node_modules` (19.2.8 today, against the root's 19.2.7). Pointing
`react` at the root while `react-dom` resolves from the client would pair a
renderer with a React it was not installed against, which is the same split the
pin exists to prevent, arrived at from the other side. `dedupe` still collapses
each install to one copy, which is what actually has to hold.

### Why runtime consumption decides `dependencies`

Expand Down
7 changes: 5 additions & 2 deletions AGENTS.md
Original file line number Diff line number Diff line change
Expand Up @@ -85,11 +85,14 @@ worktree trap — is the `local-dev` skill.
The reasoning behind each of these, and what breaks when it is ignored, is the
`local-dev` skill. The rules themselves:

- **The MCP SDK packages** — `@modelcontextprotocol/client`, `core`, `server`, `server-legacy`, `ext-apps` — **are declared in the repo-root `package.json` and nowhere else.** So is anything reached only through root-owned code with no manifest of its own (`test-servers/src`, `core/`), aliased to the repo root in `vitest.shared.mts`. The v1 SDK (`@modelcontextprotocol/sdk`) is **not** a dependency of this repo and must not become one.
- **Every runtime dependency `core/` imports is declared in the repo-root `package.json` and nowhere else.** That is the MCP SDK packages (`@modelcontextprotocol/client`, `core`, `server`, `server-legacy`, `ext-apps`) and, since #2195, the rest of what `core/` reaches: `ajv`, `atomically`, `chokidar`, `hono`, `@napi-rs/keyring`, `pino`, `proper-lockfile`, `react`, `undici`, `zod`. So is anything reached only through root-owned code with no manifest of its own (`test-servers/src`, `core/`). The v1 SDK (`@modelcontextprotocol/sdk`) is **not** a dependency of this repo and must not become one.
- **A root declaration is not by itself a claim that `core/` imports it.** `commander`, `open`, `@hono/node-server`, `vite` and `@vitejs/plugin-react` are root `dependencies` reached only from *client* code, for the runtime-consumption reason below: a published install resolves every externalized import from the root manifest, so a client's runtime import has to be declared there whether or not `core/` also reaches it. Those need naming only in the `external` list of the client that actually imports them, not in all three.
- **A client declares only what that client alone consumes** — its own UI stack, its bundler-inlined packages, its dev tooling. `clients/cli` 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.
- **A root-declared package that `core/` imports at runtime must also be named in all three bundler `external` lists** (`clients/{cli,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`).
- **One version per install-crossing dependency.** When bumping a dependency the shared sources pull in, bump it in every install that declares it. Never raise the tsc heap to work around a skew. `npm run verify:dep-lockstep` enforces this.
- **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.
- **Pin a transitive dependency with an `overrides` entry**, not with `npm audit fix` — which "resolves" an advisory with no upward escape by silently downgrading.

## Contributing
Expand Down
2 changes: 1 addition & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ npm run web # prod web launcher against clients/web/dist
npm run web:dev # web launcher in --dev mode (Vite)
```

v2 is **not** an npm workspace — each client under `clients/*` keeps its own `package.json` and `node_modules`, and shared code lives in `core/`, consumed via a `@inspector/core` build-time alias. What that means for adding a dependency (root vs. client, `dependencies` vs. `devDependencies`, and the bundler `external` lists) is in the [`local-dev` skill](./.claude/skills/local-dev/SKILL.md).
v2 is **not** an npm workspace — each client under `clients/*` keeps its own `package.json` and `node_modules`, and shared code lives in `core/`, consumed via a `@inspector/core` build-time alias. **Every runtime dependency `core/` imports is declared once, in the repo-root `package.json`**, and each client declares only what that client alone consumes — its UI stack, its bundler-inlined packages, its dev tooling — which leaves `clients/cli` and `clients/launcher` with no runtime dependencies of their own. What that means for adding a dependency (root vs. client, `dependencies` vs. `devDependencies`, and the bundler `external` lists) is in the [`local-dev` skill](./.claude/skills/local-dev/SKILL.md).

## Project layout

Expand Down
Loading