From f62cfbaeeb277534880b8e64f93808b3ecbee337 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Mon, 31 Aug 2026 19:58:22 -0400 Subject: [PATCH] chore: address Copilot review on #2200 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Complete the bundler external lists against the AGENTS.md invariant rather than against today's reachability: chokidar, hono and react to clients/cli, chokidar and hono to clients/tui, react to the web runner. Every root-declared package core/ imports at runtime is now named in all three, so a new import in core/ cannot silently inline one. - AGENTS.md: split the single over-broad bullet in two. commander, open, @hono/node-server, vite and @vitejs/plugin-react are root dependencies reached only from client code, so they are not part of the core/ set and need naming only in the external list of the client that imports them. Give the vitest.shared.mts pins their own rule, stating the react/react-dom exception instead of implying every root-owned package is aliased to the root. - vitest.shared.mts: the umbrella comment now says root-OWNED rather than root runtime dependency — express is test-only and lives in the root devDependencies — and notes that the list is deliberately not the complete root set, since a package that resolves unambiguously needs no pin. - local-dev skill: document why react/react-dom stay client-pinned, which the previous text contradicted. npm run local:gate and npm run verify:bundle-externals pass. Signed-off-by: cliffhall --- .claude/skills/local-dev/SKILL.md | 12 +++++++++++- AGENTS.md | 4 +++- clients/cli/tsup.config.ts | 8 ++++++++ clients/tui/tsup.config.ts | 7 +++++++ clients/web/tsup.runner.config.ts | 6 ++++++ vitest.shared.mts | 12 ++++++++---- 6 files changed, 43 insertions(+), 6 deletions(-) diff --git a/.claude/skills/local-dev/SKILL.md b/.claude/skills/local-dev/SKILL.md index 99327a5a..19d24cb1 100644 --- a/.claude/skills/local-dev/SKILL.md +++ b/.claude/skills/local-dev/SKILL.md @@ -122,7 +122,17 @@ Two consequences that read as bugs and are not: 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. +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` diff --git a/AGENTS.md b/AGENTS.md index 3f96444b..1c6bf976 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -85,8 +85,10 @@ 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: -- **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 them: `ajv`, `atomically`, `chokidar`, `commander`, `hono`, `@hono/node-server`, `@napi-rs/keyring`, `open`, `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/`). All of it is aliased to the repo root in `vitest.shared.mts` — a pin left pointing at `/node_modules` after a package moves resolves to a directory that no longer exists, or to whatever transitive copy an unrelated dependency dragged in. 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 `/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`). diff --git a/clients/cli/tsup.config.ts b/clients/cli/tsup.config.ts index 366864c0..99ffd898 100644 --- a/clients/cli/tsup.config.ts +++ b/clients/cli/tsup.config.ts @@ -55,6 +55,14 @@ export default defineConfig({ "atomically", "open", "zod", + // Reached through `core/` but not through this client's own code today. + // AGENTS.md requires every root-declared package `core/` imports at runtime + // in ALL three lists regardless, because which client reaches one is a + // function of what `core/` imports rather than of what the client names — + // so the list must not depend on today's reachability (Copilot). + "chokidar", + "hono", + "react", ], esbuildOptions(options) { options.alias = { diff --git a/clients/tui/tsup.config.ts b/clients/tui/tsup.config.ts index 3bc8a965..c13711ff 100644 --- a/clients/tui/tsup.config.ts +++ b/clients/tui/tsup.config.ts @@ -149,6 +149,13 @@ export default defineConfig({ "ajv", "atomically", "zod", + // Reached through `core/` but not through this client's own code today. + // AGENTS.md requires every root-declared package `core/` imports at runtime + // in ALL three lists regardless, because which client reaches one is a + // function of what `core/` imports rather than of what the client names — + // so the list must not depend on today's reachability (Copilot). + "chokidar", + "hono", ], esbuildPlugins: [inkFormLabelPatch], esbuildOptions(options) { diff --git a/clients/web/tsup.runner.config.ts b/clients/web/tsup.runner.config.ts index e66478e2..0b354d42 100644 --- a/clients/web/tsup.runner.config.ts +++ b/clients/web/tsup.runner.config.ts @@ -57,6 +57,12 @@ export default defineConfig({ // reached by a manifest edit rather than an omission. "ajv", "zod", + // Reached through `core/` but not through this client's own code today. + // AGENTS.md requires every root-declared package `core/` imports at runtime + // in ALL three lists regardless, because which client reaches one is a + // function of what `core/` imports rather than of what the client names — + // so the list must not depend on today's reachability (Copilot). + "react", ], esbuildOptions(options) { options.alias = { diff --git a/vitest.shared.mts b/vitest.shared.mts index 0526ade1..6f854312 100644 --- a/vitest.shared.mts +++ b/vitest.shared.mts @@ -50,10 +50,14 @@ export function vitestSharedPaths(clientDir: string) { find: /^react-dom\/client$/, replacement: path.resolve(dirname, "node_modules/react-dom/client.js"), }, - // Everything below resolves from the **repo root**, unlike the `react` / - // `react-dom` pins above. These are the runtime dependencies declared in - // the root manifest and nowhere else, so the root install is the only place - // a client's resolution chain is guaranteed to find them. + // Everything below is **root-owned** and resolves from the repo root, + // unlike the `react` / `react-dom` pins above. What they have in common is + // that no client declares them, so the root install is the only place a + // client's resolution chain is guaranteed to find one — not that they are + // all root `dependencies`: `express` is test-only and sits in the root + // `devDependencies`. Nor is this the complete root runtime set; a package + // that resolves unambiguously without help (`ajv`, `commander`, `open`, + // `undici`, `zod`) needs no pin and has none. // // `express` and `yaml` are reached only through `test-servers/src` — // express by the http/oauth servers, yaml by `load-config.ts` — which is