From 2a64c6716b4b8e3553a96256da3dc4792ad11628 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Mon, 31 Aug 2026 20:11:21 -0400 Subject: [PATCH] chore: address Copilot review round 2 on #2200 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - clients/web/tsconfig.{app,node,test}.json: repoint the pino, hono, hono/streaming, @hono/node-server, atomically, chokidar and @napi-rs/keyring path mappings from ./node_modules to ../../node_modules. The stale mappings were not failing the build — an unresolvable paths entry falls back to ordinary walk-up resolution, which finds the root copy — but chokidar is the case where the fallback never runs: Vite's transitive copy still sits under clients/web, so tsc was type-checking against the client's copy while Vitest and the shipped bundle both resolved the root's. Same version today, silent skew tomorrow. Both path prefixes now carry a comment saying which ownership each one asserts. - local-dev skill: the consolidation rationale no longer groups commander, open and @hono/node-server (client-imported) with the core/ set, no longer claims #2195 moved undici (it was already root-only), and no longer implies every root-declared package has an alias. npm run local:gate passes with clients/web/node_modules/.tmp cleared, so the tsc results are from a cold build rather than an incremental cache. Signed-off-by: cliffhall --- .claude/skills/local-dev/SKILL.md | 22 +++++++++++++++++----- clients/web/tsconfig.app.json | 24 +++++++++++++++++------- clients/web/tsconfig.node.json | 24 +++++++++++++++++------- clients/web/tsconfig.test.json | 14 +++++++------- 4 files changed, 58 insertions(+), 26 deletions(-) diff --git a/.claude/skills/local-dev/SKILL.md b/.claude/skills/local-dev/SKILL.md index 19d24cb1..c22bf6a9 100644 --- a/.claude/skills/local-dev/SKILL.md +++ b/.claude/skills/local-dev/SKILL.md @@ -98,11 +98,23 @@ 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`. That used to be `express` and `yaml` -alone; #2195 made it the general case, moving `ajv`, `atomically`, `chokidar`, -`commander`, `hono`, `@hono/node-server`, `@napi-rs/keyring`, `open`, `pino`, -`react`, `undici` and `zod` to the root as well. +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 — a transitive copy sits in the client install, or the + importer is `core/`, which has no manifest to resolve from. A package that + resolves unambiguously by walk-up (`ajv`, `commander`, `open`, `undici`, + `zod`) is deliberately absent from both. 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**. diff --git a/clients/web/tsconfig.app.json b/clients/web/tsconfig.app.json index df982811..57d211f0 100644 --- a/clients/web/tsconfig.app.json +++ b/clients/web/tsconfig.app.json @@ -30,6 +30,16 @@ * imports type-check; Vite's `dedupe` handles the runtime resolution — * see vite.config.ts. * + * Two prefixes, and the difference is ownership rather than style. A + * `./node_modules/...` mapping names a package this client declares — the + * `@types/*` packages, `vitest`, `yaml`. A `../../node_modules/...` one + * names a package declared only in the root manifest (#2195), which is + * every runtime dependency `core/` imports. Those must be spelled out: + * `chokidar` in particular is still present under `clients/web` as a + * transitive copy of Vite's, so a `./node_modules` mapping resolves the + * client's copy at compile time while Vitest and the shipped bundle both + * resolve the root's. Same version today, and a silent skew tomorrow. + * * Paths are relative to this config file (no `baseUrl` — deprecated in * TypeScript 6 / removed in 7). */ @@ -38,15 +48,15 @@ "react": ["./node_modules/@types/react"], "react-dom": ["./node_modules/@types/react-dom"], "react/jsx-runtime": ["./node_modules/@types/react/jsx-runtime"], - "pino": ["./node_modules/pino"], - "hono": ["./node_modules/hono"], + "pino": ["../../node_modules/pino"], + "hono": ["../../node_modules/hono"], "hono/streaming": [ - "./node_modules/hono/dist/types/helper/streaming/index" + "../../node_modules/hono/dist/types/helper/streaming/index" ], - "@hono/node-server": ["./node_modules/@hono/node-server"], - "atomically": ["./node_modules/atomically"], - "chokidar": ["./node_modules/chokidar"], - "@napi-rs/keyring": ["./node_modules/@napi-rs/keyring"], + "@hono/node-server": ["../../node_modules/@hono/node-server"], + "atomically": ["../../node_modules/atomically"], + "chokidar": ["../../node_modules/chokidar"], + "@napi-rs/keyring": ["../../node_modules/@napi-rs/keyring"], "react-syntax-highlighter/dist/esm/prism-light": [ "./src/types/react-syntax-highlighter-prism-light" ], diff --git a/clients/web/tsconfig.node.json b/clients/web/tsconfig.node.json index 81389121..30702aee 100644 --- a/clients/web/tsconfig.node.json +++ b/clients/web/tsconfig.node.json @@ -32,20 +32,30 @@ * tsc follows those transitive imports while type-checking vite.config.ts, * so this tsconfig needs the same alias tsconfig.app.json has. * + * Two prefixes, and the difference is ownership rather than style. A + * `./node_modules/...` mapping names a package this client declares — the + * `@types/*` packages, `vitest`, `yaml`. A `../../node_modules/...` one + * names a package declared only in the root manifest (#2195), which is + * every runtime dependency `core/` imports. Those must be spelled out: + * `chokidar` in particular is still present under `clients/web` as a + * transitive copy of Vite's, so a `./node_modules` mapping resolves the + * client's copy at compile time while Vitest and the shipped bundle both + * resolve the root's. Same version today, and a silent skew tomorrow. + * * Paths are relative to this config file (no `baseUrl` — deprecated in * TypeScript 6 / removed in 7). */ "paths": { "@inspector/core/*": ["../../core/*"], - "pino": ["./node_modules/pino"], - "hono": ["./node_modules/hono"], + "pino": ["../../node_modules/pino"], + "hono": ["../../node_modules/hono"], "hono/streaming": [ - "./node_modules/hono/dist/types/helper/streaming/index" + "../../node_modules/hono/dist/types/helper/streaming/index" ], - "@hono/node-server": ["./node_modules/@hono/node-server"], - "atomically": ["./node_modules/atomically"], - "chokidar": ["./node_modules/chokidar"], - "@napi-rs/keyring": ["./node_modules/@napi-rs/keyring"] + "@hono/node-server": ["../../node_modules/@hono/node-server"], + "atomically": ["../../node_modules/atomically"], + "chokidar": ["../../node_modules/chokidar"], + "@napi-rs/keyring": ["../../node_modules/@napi-rs/keyring"] } }, "include": ["vite.config.ts", "tsup.runner.config.ts", "server/**/*.ts"] diff --git a/clients/web/tsconfig.test.json b/clients/web/tsconfig.test.json index 23dd9625..6d8fd810 100644 --- a/clients/web/tsconfig.test.json +++ b/clients/web/tsconfig.test.json @@ -10,17 +10,17 @@ "react-dom": ["./node_modules/@types/react-dom"], "react/jsx-runtime": ["./node_modules/@types/react/jsx-runtime"], "vitest": ["./node_modules/vitest"], - "pino": ["./node_modules/pino"], - "hono": ["./node_modules/hono"], + "pino": ["../../node_modules/pino"], + "hono": ["../../node_modules/hono"], "hono/streaming": [ - "./node_modules/hono/dist/types/helper/streaming/index" + "../../node_modules/hono/dist/types/helper/streaming/index" ], - "@hono/node-server": ["./node_modules/@hono/node-server"], - "atomically": ["./node_modules/atomically"], - "chokidar": ["./node_modules/chokidar"], + "@hono/node-server": ["../../node_modules/@hono/node-server"], + "atomically": ["../../node_modules/atomically"], + "chokidar": ["../../node_modules/chokidar"], "express": ["./node_modules/@types/express"], "yaml": ["./node_modules/yaml"], - "@napi-rs/keyring": ["./node_modules/@napi-rs/keyring"], + "@napi-rs/keyring": ["../../node_modules/@napi-rs/keyring"], "react-syntax-highlighter/dist/esm/prism-light": [ "./src/types/react-syntax-highlighter-prism-light" ],