From f13d415eaeb0f218dc0e41d0c43a5f7496f58817 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sat, 29 Aug 2026 03:57:25 -0400 Subject: [PATCH] chore: address Copilot review round 13 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - skill-manifest: an eval file may only expect its own skill. A foreign `expect: "other-skill"` passed whenever that other skill fired, so the file reported a measurement of something it does not describe — and it satisfied the positive-case requirement while doing so, masking the absence of a real positive. Both halves covered by a regression case. - local-dev: stop restating the dependency-placement rules. They are normative and belong only in AGENTS.md; the skill now points there and carries what a rule cannot — what each one is defending against, what it looked like when it was violated (#1970, #2082, #2067, #1952, #1896, #2058), and how to tell you have hit it. Including the two traps that make this class hard to see: the ordinary test tiers cannot catch a bundling miss, and probing one by hand with `node -e` falsely succeeds. Signed-off-by: cliffhall --- .claude/skills/local-dev/SKILL.md | 160 +++++++++++++++------------- scripts/lib/skill-manifest.mjs | 9 ++ scripts/lib/skill-manifest.test.mjs | 29 +++++ 3 files changed, 121 insertions(+), 77 deletions(-) diff --git a/.claude/skills/local-dev/SKILL.md b/.claude/skills/local-dev/SKILL.md index 56ecffcf..15854b99 100644 --- a/.claude/skills/local-dev/SKILL.md +++ b/.claude/skills/local-dev/SKILL.md @@ -81,102 +81,108 @@ build-time alias: ## Where a dependency goes -Three independent questions. Get them in this order. +**The rules are in [`AGENTS.md`](../../../AGENTS.md) → Dependency placement, and +they are not restated here.** Read them there and come back for the *why* — what +each rule is defending against, what it looked like when it was violated, and how +to tell you have hit one. -### 1. Which manifest declares it? +Work the rules in order: which manifest declares it, then `dependencies` vs +`devDependencies`, then whether the bundlers must also externalise it. -**The MCP SDK packages — `@modelcontextprotocol/client`, `core`, `server`, -`server-legacy`, `ext-apps` — are declared in the repo-root `package.json` and -nowhere else.** Node resolution walks up, so the root install already serves -every client; a per-client declaration installs a *second copy* that drifts. That -is not theoretical — it put two versions of `ext-apps` and of the transitive v1 -`@modelcontextprotocol/sdk` in the tree at once (#1970), and a second copy of -`client`/`core` is exactly the failure the `dedupe` + `server.deps.inline` -workaround in `vitest.shared.mts` exists for. +### Why the SDK packages are root-only -The same rule covers anything reached only through **root-owned code with no -manifest of its own** (`test-servers/src`, `core/`): declare it at the root, and -alias it to the **repo root** in `vitest.shared.mts`. `express` and `yaml` are -the two today. +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 +transitive v1 `@modelcontextprotocol/sdk` (1.29.0 / 1.30.0) in the tree at once +(#1970). A second copy of `client`/`core` is exactly the failure the `dedupe` + +`server.deps.inline` workaround in `vitest.shared.mts` exists for. -⚠️ The v1 SDK (`@modelcontextprotocol/sdk`) is **not** a dependency of this repo -and must not become one. It appears in the lock files only as a `"peer": true` -entry pulled in by `ext-apps`. +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. -### 2. `dependencies` or `devDependencies`? +### Why runtime consumption decides `dependencies` -Follows from who consumes it **at runtime**, not from where it is declared: +The client builds externalise npm packages, so a published install resolves them +from the **root** manifest — and devDependencies are not installed for +consumers. A runtime import parked there therefore **breaks the published +package while passing every local check**, which is the worst shape a mistake +can take here. -- A package `core/` imports at runtime → root **`dependencies`**. The client - builds externalize npm packages, so a published install resolves them from the - root manifest, and devDependencies are not installed for consumers — a runtime - import parked there breaks the published package while passing every local - check. -- A package only the tests, the test servers, or the build tooling need → - **`devDependencies`** (`express`). -- `vite` and `@vitejs/plugin-react` are root **`dependencies`** on purpose: - `clients/web/server/start-vite-dev-server.ts` imports them at runtime for - `--web --dev`, and `tsup.runner.config.ts` externalizes both. +Two live examples worth knowing: -### 3. Does it also need adding to the bundler `external` lists? +- `express` is test-only, so it is a devDependency. +- `vite` and `@vitejs/plugin-react` look like build tooling but are root + `dependencies`, because `clients/web/server/start-vite-dev-server.ts` imports + them at runtime for `--web --dev`. Moving them would break that for consumers + while every local check stayed green. -**Yes, if `core/` imports it at runtime.** tsup and Vite externalize what the -*client's* `package.json` declares, and a root-only dependency is in none of -them — so it gets **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. +### Why a root dependency must also be externalised -The three lists are `clients/cli/tsup.config.ts`, -`clients/tui/tsup.config.ts`, and `clients/web/tsup.runner.config.ts` — add it -to **all three**, since which client reaches it is a function of what `core/` -imports, not of what the client's own code names. +tsup 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, +so the binary dies before it parses a flag (`proper-lockfile`, #2082). -**`npm run verify:bundle-externals` enforces this.** It reads the **built -output**, not the config (the two disagreed for four releases), over the union of -each client's `external` array and the root manifest's `dependencies`. Note the -ordinary test tiers cannot catch this class: unit and integration tests run -against source, and the smokes never take the lazy-`import()` path. +`undici` (#2067) is the worse variant, because it is `import()`ed lazily: the +binary boots fine and only the proxied path dies — with esbuild's rewritten +specifier (`import("./undici-HXPKCIY3.js")`) meaning **no user-side install can +ever satisfy it**. It was declared in `clients/cli/package.json`, so the CLI +looked correct while web and TUI silently inlined 1.05 MB of it. -⚠️ Probing it by hand with `node -e` **falsely succeeds** — `node -e` exposes a -global `require` that satisfies esbuild's guard. Reproduce from a real `.mjs` +**How to tell.** The ordinary tiers cannot: unit and integration tests run +against source, where the real package is on the resolution path, and the smokes +never take the lazy path. `npm run verify:bundle-externals` reads the **built +output** for exactly this reason — the config and the output disagreed for four +releases. + +⚠️ **Probing by hand with `node -e` falsely succeeds.** `node -e` exposes a +global `require` that satisfies esbuild's `typeof require !== "undefined"` +guard, so the chunk looks loadable when it is not. Reproduce from a real `.mjs` file. -### The exception: a dependency that renders React must be bundled +### Why React-rendering packages are the exception -An externalized package resolves its own `react` from wherever npm placed *it*, -beside a React satisfying **that package's** peer range — looser than ours in -every case here, which is all it takes to split React. `ink-form` and -`ink-scroll-view` declare `">=18"`; the bundle renders through one React, those -packages call hooks on another, and the TUI crashes on the first hook (#1952). -Both are inlined by `clients/tui/tsup.config.ts` (`noExternal`) and declared -only in `clients/tui/package.json`. +An externalised package resolves its own `react` from wherever npm placed +**it** — beside a React satisfying *that package's* peer range, which is looser +than ours in every case here. `ink-form` and `ink-scroll-view` declare `">=18"`, +so a consumer's React 18 satisfies them and hoists them while our React 19 nests +underneath: the bundle renders through one React, those packages call hooks on +another, and the TUI crashes on the first hook (#1952). -**`ink` is the single exemption, justified by cost** (~1.4MB), not by safety. -Never justify an exemption by a peer range. What makes it safe is that the root -`react` range stays open to the whole major (`^19.0.0`), so npm can dedupe our -React with a consumer's. `clients/tui/__tests__/tsupConfig.test.ts` enforces all -of it. +`ink` is exempt on **cost** (~1.4 MB of `react-reconciler` + `yoga-layout`, plus +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` +enforces the whole split, the exemption included. -### One version per install-crossing dependency +### Why a version skew is worth aligning rather than working around Because v2 is not a workspace, the same package can appear **twice in one `tsc` -program** (a client's own install plus the root's, reached through `core/` or -`test-servers/src`). At the same version that is harmless; on a skew TypeScript -must relate two structurally-distinct declarations, which for a recursive-generic -surface is exponential — a zod `4.3.6`/`4.4.3` skew exhausted the 4GB tsc heap -(#1896). +program** — a client's own install plus the root's, reached through `core/` or +`test-servers/src`. At the same version that is harmless; on a skew TypeScript +must relate two structurally-distinct declarations, which for a +recursive-generic surface is exponential. A zod `4.3.6` / `4.4.3` skew exhausted +the 4 GB tsc heap outright with `TS2589` (#1896). -**Raising the heap hides this class; align the versions instead** — bump it in -every install that *declares* it (not all four unconditionally; a package absent -from an install can't skew). `npm run verify:dep-lockstep` is the guard. +⚠️ **Raising the heap hides the class rather than fixing it.** Align the +versions; `npm run verify:dep-lockstep` is the guard, and it derives its +candidate set from what actually enters each `tsc` program, so a package whose +declarations arrive only through another package's `.d.ts` is still seen. -### Pinning a transitive dependency +### Why `overrides` beats `npm audit fix` -Reach for an `overrides` entry before `npm audit fix`. -`clients/{web,cli,tui}/package.json` each override `esbuild` to `^0.28.2` -because `tsup@8.5.1` declares `^0.27.0` and the advisory has no upward escape -inside that range — `npm audit fix` "resolves" it by silently *downgrading*. -Two costs: it puts `tsup` on an esbuild major it does not declare, so `npm run -build` 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. +`tsup@8.5.1` declares `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 +`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. diff --git a/scripts/lib/skill-manifest.mjs b/scripts/lib/skill-manifest.mjs index b12669b6..f00e3946 100644 --- a/scripts/lib/skill-manifest.mjs +++ b/scripts/lib/skill-manifest.mjs @@ -221,6 +221,15 @@ export function validateEvalCases(skillName, cases) { (c.expect !== null && typeof c.expect !== "string") ) { errors.push(`case ${i}: \`expect\` must be a skill name or null`); + } else if (c.expect !== null && c.expect !== skillName) { + // A case living in this skill's evals may only expect THIS skill. A + // foreign name passes the eval whenever that other skill fires, so the + // file reports a measurement of something it does not describe — and it + // still satisfies the positive-case requirement, hiding the absence of a + // real one (Copilot). + errors.push( + `case ${i}: expects \`${c.expect}\`, but this file only measures \`${skillName}\``, + ); } }); const positives = cases.filter((c) => c && c.expect === skillName).length; diff --git a/scripts/lib/skill-manifest.test.mjs b/scripts/lib/skill-manifest.test.mjs index 1f387e70..ff6c8830 100644 --- a/scripts/lib/skill-manifest.test.mjs +++ b/scripts/lib/skill-manifest.test.mjs @@ -301,3 +301,32 @@ test("CRLF does not weaken the first-line rule", () => { /never closed/, ); }); + +test("an eval file may only expect its own skill", () => { + // A foreign name passes whenever that OTHER skill fires, so the file reports + // a measurement of something it does not describe — and it satisfies the + // positive-case requirement while doing it, hiding the absence of a real one. + assert.match( + validateEvalCases("testing", [ + { prompt: "a", expect: "local-dev" }, + { prompt: "b", expect: "testing" }, + { prompt: "c", expect: null }, + ]).join(), + /expects `local-dev`, but this file only measures `testing`/, + ); + // The masking case: a foreign positive is no longer enough on its own. + const masked = validateEvalCases("testing", [ + { prompt: "a", expect: "local-dev" }, + { prompt: "b", expect: null }, + ]); + assert.match(masked.join(), /only measures/); + assert.match(masked.join(), /no positive case/); + // Null and own-name cases are unaffected. + assert.deepEqual( + validateEvalCases("testing", [ + { prompt: "a", expect: "testing" }, + { prompt: "b", expect: null }, + ]), + [], + ); +});