From f5a3ccedbbf69497698786f7bb58dc0a596ff9c7 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sat, 5 Sep 2026 13:36:30 -0400 Subject: [PATCH 01/16] fix: type-discriminate progress toast ids, CSPRNG attempt-id fallback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both findings from the v2.5.0 milestone-merge review (#2215). 1. `progressToastId` collided across distinct progress streams. `ProgressToken` is `string | number`, so `String(token)` mapped the numeric token 7 and the string token "7" onto one id — and because notifications keyed by the same id are *replaced* rather than stacked, two concurrent streams overwrote each other's toast. The absent case was worse: it hardcoded the sentinel "default", which a server is free to send as a genuine string token. The id now carries the token's type (`progress-n:7` / `progress-s:7`) and gives the no-token case a prefix of its own (`progress-none`) that no token value can produce. 2. `newAttemptId`'s fallback now prefers `crypto.getRandomValues`. `randomUUID` needs a secure context; `getRandomValues` does not and exists in every browser that has `crypto` at all — so the exact situation the fallback exists for (a `file://` page, a plain-HTTP non-loopback host) still has a CSPRNG on hand. `Math.random` stays as the last resort for a `crypto`-less global, and the "never a security token" comment stays too. Retires CodeQL alert 72 (`js/insecure-randomness`) honestly rather than by dismissing it. Tests: collision cases for 7 vs "7" and the absent token vs "default" / "none", a distinctness sweep over the whole id space; and the attempt-id fallback test split into a getRandomValues arm (asserting the CSPRNG is used and `Math.random` is not) and a no-crypto-at-all arm. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Uw1s4LRBUzrFwLzPT4mAJn Signed-off-by: cliffhall --- clients/web/src/lib/oauthResume.test.ts | 90 +++++++++++++------ clients/web/src/lib/oauthResume.ts | 14 +++ .../src/utils/toasts/progressToasts.test.ts | 31 ++++++- .../web/src/utils/toasts/progressToasts.ts | 15 +++- 4 files changed, 121 insertions(+), 29 deletions(-) diff --git a/clients/web/src/lib/oauthResume.test.ts b/clients/web/src/lib/oauthResume.test.ts index 51b11c9f..aa624204 100644 --- a/clients/web/src/lib/oauthResume.test.ts +++ b/clients/web/src/lib/oauthResume.test.ts @@ -26,6 +26,34 @@ import { EMPTY_NETWORK_UI, } from "../components/screens/screenUiState.js"; +/** + * Run `body` with one `crypto` member hidden, then restore it. + * + * `randomUUID` and `getRandomValues` are inherited from `Crypto.prototype`, so + * there is normally no OWN descriptor to put back — restoring only when one + * existed would leave the `undefined` own property in place and force every + * later test in this file onto the fallback path. + */ +function withoutCryptoMember( + name: "randomUUID" | "getRandomValues", + body: () => void, +): void { + const original = Object.getOwnPropertyDescriptor(globalThis.crypto, name); + Object.defineProperty(globalThis.crypto, name, { + configurable: true, + value: undefined, + }); + try { + body(); + } finally { + if (original) { + Object.defineProperty(globalThis.crypto, name, original); + } else { + delete (globalThis.crypto as Record)[name]; + } + } +} + describe("oauthResume", () => { const storage = new Map(); @@ -44,6 +72,9 @@ describe("oauthResume", () => { afterEach(() => { vi.unstubAllGlobals(); + // This project does not set `restoreMocks` globally (see `src/test/setup.ts`), + // so the `crypto`/`Math.random` spies below must be reverted by hand. + vi.restoreAllMocks(); }); it("consumeOAuthResumeSnapshot reads once then clears storage", () => { @@ -456,41 +487,50 @@ describe("oauthResume", () => { expect(readOAuthResumeSnapshot()?.attemptId).toBe(token); }); - it("writeOAuthResumeSnapshot falls back when randomUUID is unavailable", () => { + it("writeOAuthResumeSnapshot falls back to getRandomValues without randomUUID", () => { // `crypto.randomUUID` needs a secure context, which a plain-HTTP - // non-loopback host is not. - const original = Object.getOwnPropertyDescriptor( - globalThis.crypto, - "randomUUID", - ); - Object.defineProperty(globalThis.crypto, "randomUUID", { - configurable: true, - value: undefined, - }); - try { - const token = writeOAuthResumeSnapshot({ + // non-loopback host is not. `crypto.getRandomValues` does not, so the + // fallback is still a CSPRNG rather than `Math.random`. + const randomSpy = vi.spyOn(globalThis.crypto, "getRandomValues"); + const mathSpy = vi.spyOn(Math, "random"); + let token: string | undefined; + withoutCryptoMember("randomUUID", () => { + token = writeOAuthResumeSnapshot({ version: 1, serverId: "a", activeTab: "tools", authKind: "reauth", tabUi: {}, }); - expect(token).toEqual(expect.any(String)); - expect(clearOwnOAuthResumeSnapshot(token)).toBe(true); - } finally { - // `randomUUID` is inherited from `Crypto.prototype`, so there is - // normally no OWN descriptor to put back — restoring only when one - // existed would leave the `undefined` own property in place and force - // every later test in this file onto the fallback path. - if (original) { - Object.defineProperty(globalThis.crypto, "randomUUID", original); - } else { - delete (globalThis.crypto as { randomUUID?: unknown }).randomUUID; - } - } + }); + expect(randomSpy).toHaveBeenCalledOnce(); + expect(mathSpy).not.toHaveBeenCalled(); + // 16 random bytes, hex-encoded. + expect(token).toMatch(/^[0-9a-f]{32}$/); + expect(clearOwnOAuthResumeSnapshot(token)).toBe(true); expect(globalThis.crypto.randomUUID).toEqual(expect.any(Function)); }); + it("writeOAuthResumeSnapshot falls back to Math.random with no crypto at all", () => { + const mathSpy = vi.spyOn(Math, "random"); + let token: string | undefined; + withoutCryptoMember("randomUUID", () => { + withoutCryptoMember("getRandomValues", () => { + token = writeOAuthResumeSnapshot({ + version: 1, + serverId: "a", + activeTab: "tools", + authKind: "reauth", + tabUi: {}, + }); + }); + }); + expect(mathSpy).toHaveBeenCalled(); + expect(token).toEqual(expect.any(String)); + expect(clearOwnOAuthResumeSnapshot(token)).toBe(true); + expect(globalThis.crypto.getRandomValues).toEqual(expect.any(Function)); + }); + it("clearOAuthResumeSnapshot swallows removeItem failures", () => { vi.stubGlobal("sessionStorage", { getItem: () => null, diff --git a/clients/web/src/lib/oauthResume.ts b/clients/web/src/lib/oauthResume.ts index 14517108..b0ebf13b 100644 --- a/clients/web/src/lib/oauthResume.ts +++ b/clients/web/src/lib/oauthResume.ts @@ -227,12 +227,26 @@ export function writeOAuthResumeSnapshot( * context, which a `file://` page or a plain-HTTP non-loopback host is not), * and otherwise a value that only has to be unique among the handful of * redirect attempts one page can have in flight — never a security token. + * + * The fallback chain matters even so. `crypto.getRandomValues` is *not* + * gated on a secure context and exists in every browser that has `crypto` at + * all — so precisely the situation `randomUUID` is unavailable in still has a + * CSPRNG on hand, and declining to use it would be a gratuitous downgrade + * (CodeQL `js/insecure-randomness`, alert 72, flags exactly that). `Math.random` + * survives only as the last resort for a `crypto`-less global. */ function newAttemptId(): string { const uuid = globalThis.crypto?.randomUUID?.bind(globalThis.crypto); if (uuid) { return uuid(); } + const getRandomValues = globalThis.crypto?.getRandomValues?.bind( + globalThis.crypto, + ); + if (getRandomValues) { + const bytes = getRandomValues(new Uint8Array(16)); + return Array.from(bytes, (b) => b.toString(16).padStart(2, "0")).join(""); + } return `${Date.now().toString(36)}-${Math.random().toString(36).slice(2)}`; } diff --git a/clients/web/src/utils/toasts/progressToasts.test.ts b/clients/web/src/utils/toasts/progressToasts.test.ts index 1acdff1d..500e549c 100644 --- a/clients/web/src/utils/toasts/progressToasts.test.ts +++ b/clients/web/src/utils/toasts/progressToasts.test.ts @@ -13,12 +13,37 @@ describe("PROGRESS_TOAST_AUTOCLOSE_MS", () => { describe("progressToastId", () => { it("keys by the progress token", () => { - expect(progressToastId("abc")).toBe("progress-abc"); - expect(progressToastId(7)).toBe("progress-7"); + expect(progressToastId("abc")).toBe("progress-s:abc"); + expect(progressToastId(7)).toBe("progress-n:7"); }); it("shares one id when the server sends no token", () => { - expect(progressToastId(undefined)).toBe("progress-default"); + expect(progressToastId(undefined)).toBe("progress-none"); + }); + + it("does not collide a numeric token with the same-looking string token", () => { + expect(progressToastId(7)).not.toBe(progressToastId("7")); + }); + + it("does not collide the absent token with any token a server can send", () => { + const absent = progressToastId(undefined); + for (const token of ["default", "none", "", "0"]) { + expect(progressToastId(token)).not.toBe(absent); + } + expect(progressToastId(0)).not.toBe(absent); + }); + + it("gives each distinct token its own id", () => { + const ids = [ + progressToastId(undefined), + progressToastId(0), + progressToastId(7), + progressToastId("0"), + progressToastId("7"), + progressToastId("none"), + progressToastId("default"), + ]; + expect(new Set(ids).size).toBe(ids.length); }); }); diff --git a/clients/web/src/utils/toasts/progressToasts.ts b/clients/web/src/utils/toasts/progressToasts.ts index 4b007f09..1796811c 100644 --- a/clients/web/src/utils/toasts/progressToasts.ts +++ b/clients/web/src/utils/toasts/progressToasts.ts @@ -11,8 +11,21 @@ export const PROGRESS_TOAST_AUTOCLOSE_MS = 5000; // rather than flooding the corner. The injected `progressToken` correlates a // stream with the request that triggered it; when absent (the common case — // the inspector doesn't expose a caller token), all ticks share one toast. +// +// The token's *type* is part of the key. `ProgressToken` is `string | number`, +// so a bare `String(token)` maps the number 7 and the string "7" — two +// distinct streams per the spec — onto one id, and the two streams then +// overwrite each other's toast (id collision means replacement, which is the +// whole point of the id). The `n:`/`s:` discriminator keeps them apart, and +// the no-token case gets a prefix of its own rather than the sentinel +// `"default"`, which a server is free to send as a genuine string token. export function progressToastId(token: ProgressToken | undefined): string { - return `progress-${String(token ?? "default")}`; + if (token === undefined) { + return "progress-none"; + } + return typeof token === "number" + ? `progress-n:${token}` + : `progress-s:${token}`; } // One-line toast body: " — / (NN%)". The fraction From 1e6bf6425a833af26b644c1e79a8da6099d3b772 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sat, 5 Sep 2026 14:10:36 -0400 Subject: [PATCH 02/16] chore: widen verify:dep-lockstep to declared cross-install copies (#2226) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `clients/cli` resolved `@types/node` 24.13.1 against the root's 24.13.3. Neither client declares it — #2196 consolidated it to the root — so the cli copy arrived transitively via `@types/express` and was constrained by no range of ours. `verify:dep-lockstep` reported OK and was right to: it compares only packages that reach one `tsc` **program** from two installs, and no one program sees both copies. Fixes the instance and the class, which the issue notes are not exclusive. **The class.** The guard grows a second tier alongside the program-derived one, asking the weaker but broader question `AGENTS.md`'s "one version per install-crossing dependency" rule actually states: does a package this repo declares anywhere resolve to two versions across our installs at all? The candidate set is every name in any install's `dependencies`/`devDependencies` that more than one install holds a **top-level** copy of — 17 packages today. Nested copies are excluded: one exists because some dependency asked for a different version, so it is that dependency's range, not ours. Neither tier subsumes the other. The program tier sees a copy no manifest names (the `@modelcontextprotocol/sdk` case, arriving through another package's `.d.ts`). The declared tier sees a transitive copy no program loads (this issue), two *clients* disagreeing with no root copy involved (`@types/react`, web against tui), and the peer shadows `AGENTS.md` calls out as ungated — `eslint`, `typescript`, `vitest`. Deny-by-default, with its own empty `TOLERATED_DECLARED_SKEW` and the same within-a-major rule, now stated once in a shared `toleratesSkew` so the two tiers cannot drift. **The instance, and three others the new tier surfaced.** All four were patch-level: - `@types/node` — cli 24.13.1 vs root 24.13.3. Transitive, so it takes an `overrides` entry in `clients/cli` (the repo's sanctioned lever for a transitive pin), at the root's own `^24.12.4` rather than a frozen number. - `@types/react` — web 19.2.18 vs tui 19.2.17. - `react` — web 19.2.8 vs root/tui 19.2.7. - `@vitejs/plugin-react` — web 6.0.5 vs root 6.0.2. The last three are `npm update` in the stale install: it moves the lockfile within the declared range without widening a range, which matters for the root `react ^19.0.0` that `ink`'s externalized-bundle exemption depends on. No declaration or range changed. Tests: 4 end-to-end cases (the fixture gains manifest deps and a client-side `solo` install, so the skew is real and no program holds both copies) and 13 helper cases. Closes #2226 Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01J86a6vMcA9csPtdsg7ZcCZ Signed-off-by: cliffhall --- AGENTS.md | 4 +- clients/cli/package-lock.json | 6 +- clients/cli/package.json | 1 + clients/tui/package-lock.json | 12 +- clients/web/package-lock.json | 10 +- package-lock.json | 18 +- scripts/verify-dep-lockstep.main.test.mjs | 93 +++++++- scripts/verify-dep-lockstep.mjs | 216 ++++++++++++++++++- scripts/verify-dep-lockstep.test.mjs | 250 ++++++++++++++++++++++ 9 files changed, 583 insertions(+), 27 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index cc9a5479..2c750959 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -92,11 +92,11 @@ The reasoning behind each of these, and what breaks when it is ignored, is the - **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. - **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, cli and tui, 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, and nothing gates them.** 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`) has already diverged on exactly that. `verify:dep-lockstep` does not catch either: it compares only packages that one `tsc` **program** loads from two installs, so a stray `eslint`, `prettier` or `vitest` binary is outside its candidate set entirely, and the cli `@types/node` difference goes unreported because no one program sees both copies. Check a tool copy by hand — `npm exec -- which eslint` from the client — when you change what a client declares. + - ⚠️ **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, and nothing gates them.** 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. **Since #2226 `verify:dep-lockstep` does catch this class** — its second tier compares every package any install *declares* 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 ` 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. - **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. 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. +- **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. ### Dependency updates are issue-driven, like everything else diff --git a/clients/cli/package-lock.json b/clients/cli/package-lock.json index 17dab282..a8e46a61 100644 --- a/clients/cli/package-lock.json +++ b/clients/cli/package-lock.json @@ -907,9 +907,9 @@ "license": "MIT" }, "node_modules/@types/node": { - "version": "24.13.1", - "resolved": "https://registry.npmjs.org/@types/node/-/node-24.13.1.tgz", - "integrity": "sha512-RSpUJGmvsJ1ZeBehQZFhIdpsz+bIpES0nIQXko4Ybq+N+kX6XvOq3Jo+iJ82FWLdblFq85AsMikd3m35jgezYg==", + "version": "24.13.3", + "resolved": "https://registry.npmjs.org/@types/node/-/node-24.13.3.tgz", + "integrity": "sha512-Dh8vAsV36ig5wa9OX4pXvMc9D3Veibfw2wix0CUwYODLD8nkj9UsLjASr49nPg+2eKzxhBV+v7L8pXvT4e639Q==", "dev": true, "license": "MIT", "dependencies": { diff --git a/clients/cli/package.json b/clients/cli/package.json index 70f320a6..11ef4342 100644 --- a/clients/cli/package.json +++ b/clients/cli/package.json @@ -36,6 +36,7 @@ "tsup": "^8.5.0" }, "overrides": { + "@types/node": "^24.12.4", "esbuild": "^0.28.2" } } diff --git a/clients/tui/package-lock.json b/clients/tui/package-lock.json index 9b4ebc83..5c105f2f 100644 --- a/clients/tui/package-lock.json +++ b/clients/tui/package-lock.json @@ -1713,9 +1713,9 @@ "peer": true }, "node_modules/@types/react": { - "version": "19.2.17", - "resolved": "https://registry.npmjs.org/@types/react/-/react-19.2.17.tgz", - "integrity": "sha512-MXfmqaVPEVgkBT/aY0aGCkRWWtByiYQXo3xdQ8r5RzuFrPiRn8Gar2tQdXSUQ2GKV3bkXckek89V8wQBY2Q/Aw==", + "version": "19.2.18", + "resolved": "https://registry.npmjs.org/@types/react/-/react-19.2.18.tgz", + "integrity": "sha512-AnzbBERsrLKtk2XSfTbYRLjQPdy116Sty4q+T+Bp3IC4l6jNBvreVPAHmpq9qhXQM7CXZPjLVmGMw9sy+hxQ3w==", "devOptional": true, "license": "MIT", "dependencies": { @@ -3642,9 +3642,9 @@ } }, "node_modules/react": { - "version": "19.2.7", - "resolved": "https://registry.npmjs.org/react/-/react-19.2.7.tgz", - "integrity": "sha512-HNe9WslTbXmFK8o8cmwgAeJFSBvt1bPdHCVKtaaV+WlAN36mpT4hcRpwbf3fY56ar2oIXzsBpOAiIRHAdY0OlQ==", + "version": "19.2.8", + "resolved": "https://registry.npmjs.org/react/-/react-19.2.8.tgz", + "integrity": "sha512-PWaYA1L/q9u2u7xYQi+Y3L3Yfnie7XyLeaJICV1MGD6LprsBxcAqGjYyr0eY3p+QdsA+x/Irkt4Qif8D63+Sbw==", "license": "MIT", "peer": true, "engines": { diff --git a/clients/web/package-lock.json b/clients/web/package-lock.json index 68b47d54..17a1e846 100644 --- a/clients/web/package-lock.json +++ b/clients/web/package-lock.json @@ -3717,9 +3717,9 @@ "license": "ISC" }, "node_modules/@vitejs/plugin-react": { - "version": "6.0.5", - "resolved": "https://registry.npmjs.org/@vitejs/plugin-react/-/plugin-react-6.0.5.tgz", - "integrity": "sha512-BOVzne/NL162sMdResB25mUv+vWMF5NoAjNf09TeGlE7ZpszZWSD3winycicLJw72yeVsoCn/2kOhEuCvEShMA==", + "version": "6.1.1", + "resolved": "https://registry.npmjs.org/@vitejs/plugin-react/-/plugin-react-6.1.1.tgz", + "integrity": "sha512-yxLaQV9gkhS8ezJqCM6+ndU7mDY6gqAg75NQ+0IjwEI8IYOmQCgkRwHKVSfWXW076DsqMo0Dk+0FK1U+M5RgFw==", "dev": true, "license": "MIT", "dependencies": { @@ -3731,6 +3731,7 @@ "peerDependencies": { "@rolldown/plugin-babel": "^0.1.7 || ^0.2.0", "babel-plugin-react-compiler": "^1.0.0", + "oxc-transform-react": "^0.145.0", "vite": "^8.0.0" }, "peerDependenciesMeta": { @@ -3739,6 +3740,9 @@ }, "babel-plugin-react-compiler": { "optional": true + }, + "oxc-transform-react": { + "optional": true } } }, diff --git a/package-lock.json b/package-lock.json index ba8b251e..ee754a1d 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1596,12 +1596,12 @@ } }, "node_modules/@vitejs/plugin-react": { - "version": "6.0.2", - "resolved": "https://registry.npmjs.org/@vitejs/plugin-react/-/plugin-react-6.0.2.tgz", - "integrity": "sha512-DlSMqo4WhThw4vB8Mpn0Woe9J+Jfq1geJ61AKW0QEgLzGMNwtIMdxbDUzLxcun8W7NbJO0e2Jg/Nxm3cCSVzzg==", + "version": "6.1.1", + "resolved": "https://registry.npmjs.org/@vitejs/plugin-react/-/plugin-react-6.1.1.tgz", + "integrity": "sha512-yxLaQV9gkhS8ezJqCM6+ndU7mDY6gqAg75NQ+0IjwEI8IYOmQCgkRwHKVSfWXW076DsqMo0Dk+0FK1U+M5RgFw==", "license": "MIT", "dependencies": { - "@rolldown/pluginutils": "^1.0.0" + "@rolldown/pluginutils": "^1.0.1" }, "engines": { "node": "^20.19.0 || >=22.12.0" @@ -1609,6 +1609,7 @@ "peerDependencies": { "@rolldown/plugin-babel": "^0.1.7 || ^0.2.0", "babel-plugin-react-compiler": "^1.0.0", + "oxc-transform-react": "^0.145.0", "vite": "^8.0.0" }, "peerDependenciesMeta": { @@ -1617,6 +1618,9 @@ }, "babel-plugin-react-compiler": { "optional": true + }, + "oxc-transform-react": { + "optional": true } } }, @@ -4429,9 +4433,9 @@ } }, "node_modules/react": { - "version": "19.2.7", - "resolved": "https://registry.npmjs.org/react/-/react-19.2.7.tgz", - "integrity": "sha512-HNe9WslTbXmFK8o8cmwgAeJFSBvt1bPdHCVKtaaV+WlAN36mpT4hcRpwbf3fY56ar2oIXzsBpOAiIRHAdY0OlQ==", + "version": "19.2.8", + "resolved": "https://registry.npmjs.org/react/-/react-19.2.8.tgz", + "integrity": "sha512-PWaYA1L/q9u2u7xYQi+Y3L3Yfnie7XyLeaJICV1MGD6LprsBxcAqGjYyr0eY3p+QdsA+x/Irkt4Qif8D63+Sbw==", "license": "MIT", "engines": { "node": ">=0.10.0" diff --git a/scripts/verify-dep-lockstep.main.test.mjs b/scripts/verify-dep-lockstep.main.test.mjs index 810bff6d..9295b5cd 100644 --- a/scripts/verify-dep-lockstep.main.test.mjs +++ b/scripts/verify-dep-lockstep.main.test.mjs @@ -82,6 +82,9 @@ function makeFixture({ webTsconfig, rootNestedInner, cliDeps, + rootManifestDeps, + webManifestDeps, + webSolo, } = {}) { // realpath matters: on macOS `tmpdir()` is `/var/...`, a symlink to // `/private/var/...`. The guard only runs `main()` when `import.meta.url` @@ -101,6 +104,7 @@ function makeFixture({ write("package.json", { name: "fixture", + ...(rootManifestDeps ? { dependencies: rootManifestDeps } : {}), scripts: scripts ?? { validate: "npm run verify:format-coverage && npm run verify:dep-lockstep", "verify:format-coverage": "node scripts/verify-format-coverage.mjs", @@ -121,6 +125,7 @@ function makeFixture({ ); write("clients/web/package.json", { name: "web", + ...(webManifestDeps ? { devDependencies: webManifestDeps } : {}), scripts: webScripts ?? { typecheck: "tsc --noEmit -p tsconfig.json" }, }); write( @@ -166,6 +171,20 @@ function makeFixture({ stub(installRoot, versions, "inner", "export type Inner = number;\n"); } stub("", root, "solo", "export type Solo = string;\n"); + // `solo` under the CLIENT install as well. Nothing in `clients/web/src` + // imports it, so the program still resolves exactly one copy (the root's, via + // the shared `core/`) — the declared tier's whole point is that it sees this + // second copy anyway (#2226). + const webLockDeps = { ...web }; + if (webSolo) { + stub( + "clients/web/", + { solo: webSolo }, + "solo", + "export type Solo = string;\n", + ); + webLockDeps.solo = webSolo; + } // The root reaches `inner` only through a copy NESTED under `outer`, at a // version its top-level entry does not carry. npm's own conflict resolution, @@ -182,7 +201,7 @@ function makeFixture({ } write("package-lock.json", lock(rootLockDeps)); - write("clients/web/package-lock.json", rawWebLock ?? lock(web)); + write("clients/web/package-lock.json", rawWebLock ?? lock(webLockDeps)); // A second client, to prove a third install's copy is not dragged into a // comparison it never took part in. Its program spans only its own install. @@ -503,3 +522,75 @@ test("main: exits 1 when the root validate no longer runs the sibling guard", () }, ); }); + +// --------------------------------------------------------------------------- +// The declared tier (#2226) — skew across installs that no ONE program sees. +// --------------------------------------------------------------------------- + +test("main: a DECLARED package skewed across installs fails even when no program holds both copies (#2226)", () => { + // The exact shape the issue reported. `solo` is installed under the root and + // under `clients/web`, and both manifests name it — but only the shared + // `core/` imports it, so the client's program resolves the root's copy alone + // and the program tier is silent, correctly. The declared tier is what sees + // it, which is the whole reason it exists: cli's `@types/node` was hoisted in + // via `@types/express` and no program ever met the root's copy. + withFixture( + { + rootManifestDeps: { solo: "^7.0.0" }, + webManifestDeps: { solo: "^7.0.0" }, + webSolo: "7.0.0", + }, + (dir) => { + const { status, out } = runGuard(dir); + assert.equal(status, 1, out); + assert.match(out, /1 declared dependency resolves/); + assert.match(out, /7\.8\.9\s+\(\.\/node_modules\/solo\)/); + assert.match(out, /7\.0\.0\s+\(clients\/web\/node_modules\/solo\)/); + // The program tier must NOT have claimed it — its message names a program. + assert.doesNotMatch(out, /in clients\/web\/tsconfig\.json/); + }, + ); +}); + +test("main: a declared package held at the same version in both installs passes and is counted", () => { + withFixture( + { + rootManifestDeps: { solo: "^7.0.0" }, + webManifestDeps: { solo: "^7.0.0" }, + webSolo: "7.8.9", + }, + (dir) => { + const { status, out } = runGuard(dir); + assert.equal(status, 0, out); + assert.match(out, /1 declared dependencies agree/); + }, + ); +}); + +test("main: an installed package NO manifest declares is not a declared-tier candidate", () => { + // `outer` and `inner` sit in both installs in every fixture, but no manifest + // names them. The declared tier's boundary is what the repo declares, so its + // candidate count is zero here — the program tier is what covers those. + withFixture({}, (dir) => { + const { status, out } = runGuard(dir); + assert.equal(status, 0, out); + assert.match(out, /0 declared dependencies agree/); + }); +}); + +test("main: a declared package only ONE install holds cannot skew", () => { + // `solo` is declared at the root and listed in the root lockfile only. One + // holder is not an install-crossing dependency, so its version is nobody + // else's business. + withFixture( + { + rootManifestDeps: { solo: "^7.0.0" }, + webDeps: { outer: "1.2.3", inner: "4.5.6" }, + }, + (dir) => { + const { status, out } = runGuard(dir); + assert.equal(status, 0, out); + assert.match(out, /0 declared dependencies agree/); + }, + ); +}); diff --git a/scripts/verify-dep-lockstep.mjs b/scripts/verify-dep-lockstep.mjs index 05d9277a..26eebe9d 100644 --- a/scripts/verify-dep-lockstep.mjs +++ b/scripts/verify-dep-lockstep.mjs @@ -44,6 +44,40 @@ // packages verified to tolerate it (below). A dependency that starts skewing // fails `validate` and forces a decision, rather than surfacing months later as // an unexplained OOM. +// +// ## The second tier: declared packages, whatever any program loads (#2226) +// +// The program-derived tier above is deliberately narrow — it asks only "can two +// structurally-distinct copies of a type meet inside ONE `tsc` program", because +// that is the question the #1896 heap exhaustion turned on. That narrowness is a +// real blind spot, and `AGENTS.md` documented it before anything measured it: +// `clients/cli` resolved `@types/node` 24.13.1 against the root's 24.13.3, and +// this guard reported OK — correctly, since no one program sees both copies. +// +// So a SECOND tier runs alongside it, asking the weaker but broader question +// `AGENTS.md`'s "one version per install-crossing dependency" rule actually +// states: does a package that this repo DECLARES somewhere resolve to two +// different versions across our installs at all? The candidate set is every name +// in any install's `dependencies`/`devDependencies` (root or client) that more +// than one install holds a top-level copy of — 17 packages today, against the +// program tier's much smaller set. +// +// The two tiers are complementary and neither subsumes the other: +// +// • The program tier sees a copy NO manifest names — the `@modelcontextprotocol/sdk` +// case, whose `.d.ts` files arrive only through another package's types. +// • The declared tier sees a copy that arrives TRANSITIVELY into one install +// and is therefore constrained by no range of ours: cli's `@types/node` is +// hoisted via `@types/express`, so nothing pulled it forward when the root +// moved. It also sees two CLIENTS disagreeing (`@types/react` in web against +// tui), which no single program can, and the tool copies `AGENTS.md` calls +// out as ungated — `eslint`, `typescript`, `vitest` — which are peer shadows +// that no `tsc` program loads at all. +// +// The declared tier is the weaker signal, so it is the one to reach for an +// allowlist entry on. It is still deny-by-default: the cost of a false alarm is +// one `npm update ` in the stale install, and the cost of a miss is the +// class of bug that reproduces on one machine and not another. import { readFileSync, existsSync, readdirSync } from "node:fs"; import path from "node:path"; @@ -97,6 +131,17 @@ const repoRoot = path.resolve( // a better basis for a rationale than a pre-emptive entry. const TOLERATED_SKEW = new Map(); +// The declared tier's allowlist, same shape and same within-a-major rule as +// TOLERATED_SKEW above, and empty for the same reason: an entry is worth writing +// only with a real version pair in hand. +// +// Note what is deliberately NOT listed. `react` and `react-dom` are pinned per +// client on purpose (`AGENTS.md`, so a client's renderer and the React it calls +// into come from one install) — but "two copies by design" is not "two versions +// by design", and the rule that survives is still one version everywhere. The +// same holds for `vite` and `tsup`, declared by several clients each. +const TOLERATED_DECLARED_SKEW = new Map(); + /** * Installed versions in a parsed lockfile, keyed by the **install path** npm * writes — `node_modules/zod`, `node_modules/a/node_modules/zod`. @@ -210,11 +255,99 @@ export function majorOf(version) { * listed package whose holders disagree on major is still a failure. */ export function partitionSkew(skewed, tolerated = TOLERATED_SKEW) { - const isTolerated = (s) => { - if (!tolerated.has(s.name)) return false; - const majors = new Set(skewHolders(s).map((h) => majorOf(h.version))); - return majors.size === 1 && !majors.has(null); + const isTolerated = (s) => toleratesSkew(s.name, skewHolders(s), tolerated); + return { + failures: skewed.filter((s) => !isTolerated(s)), + ignored: skewed.filter(isTolerated), }; +} + +/** + * Whether `tolerated` excuses this package's skew: it must be listed, AND its + * holders must agree on major. Shared by both tiers so the within-a-major rule + * cannot be stated twice and drift — a listed package that splits across a major + * fails in either tier. + */ +export function toleratesSkew(name, holders, tolerated) { + if (!tolerated.has(name)) return false; + const majors = new Set(holders.map((h) => majorOf(h.version))); + return majors.size === 1 && !majors.has(null); +} + +/** + * Every package name any install declares, across `dependencies` and + * `devDependencies`. + * + * `manifests` is `[{ dir, manifest }]`. Both fields count because the boundary + * this tier polices is "does the repo name it", not "does it ship": a + * devDependency skew is exactly the `@types/*` and toolchain case #2226 is + * about. `peerDependencies` and `optionalDependencies` are NOT declarations of + * what this repo installs — a peer range is a constraint on the consumer, and + * the copy npm auto-installs to satisfy one is caught anyway, because it lands + * top-level in an install whose sibling declares the same name. + */ +export function declaredPackages(manifests) { + const names = new Set(); + for (const { manifest } of manifests) + for (const field of ["dependencies", "devDependencies"]) + for (const name of Object.keys(manifest?.[field] ?? {})) names.add(name); + return names; +} + +/** + * An install's TOP-LEVEL installed versions — `node_modules/`, with + * nested copies (`node_modules/a/node_modules/`) excluded. + * + * Top-level is the right depth here: it is what the install's own code resolves, + * and it is the single copy a `` range in that manifest governs. A nested + * copy exists precisely because some dependency asked for a different version, + * so it is constrained by that dependency's range rather than by anything we + * own — reporting it would fail the gate on a decision that is not ours to make. + * (The program tier prices nested copies, because a program that actually loads + * one is a different question.) + */ +export function topLevelVersions(lock) { + const versions = new Map(); + for (const [entryPath, version] of lockVersionsByPath(lock)) { + const rest = entryPath.slice("node_modules/".length); + if (rest.includes("node_modules/")) continue; + versions.set(rest, version); + } + return versions; +} + +/** + * Declared packages whose top-level copies disagree across installs. + * + * `declared` is {@link declaredPackages}' output and `versions` maps an install + * dir to that install's {@link topLevelVersions}. An install that holds no + * top-level copy of a name simply is not a holder — absence is not skew, and a + * package only one install holds cannot cross an install boundary at all. + * + * Returns `[{ name, holders: [{ dir, version }] }]` sorted by name, holders in + * install order. + */ +export function findDeclaredSkew(declared, versions) { + const skewed = []; + for (const name of [...declared].sort()) { + const holders = []; + for (const [dir, byName] of versions) { + const version = byName.get(name); + if (version !== undefined) holders.push({ dir, version }); + } + if (holders.length < 2) continue; + if (new Set(holders.map((h) => h.version)).size > 1) + skewed.push({ name, holders }); + } + return skewed; +} + +/** {@link partitionSkew} for the declared tier's `{ name, holders }` entries. */ +export function partitionDeclaredSkew( + skewed, + tolerated = TOLERATED_DECLARED_SKEW, +) { + const isTolerated = (s) => toleratesSkew(s.name, s.holders, tolerated); return { failures: skewed.filter((s) => !isTolerated(s)), ignored: skewed.filter(isTolerated), @@ -495,13 +628,86 @@ export function main() { process.exit(1); } + // ---- Second tier: every DECLARED package held by more than one install. + // Independent of what any `tsc` program loads, so it sees the transitive and + // peer-shadow copies the program tier structurally cannot (#2226). + const manifests = dirs.map((dir) => { + const file = path.join(repoRoot, dir, "package.json"); + try { + return { dir, manifest: JSON.parse(readFileSync(file, "utf8")) }; + } catch (cause) { + throw new Error( + `verify:dep-lockstep — could not parse ${dir}/package.json.`, + { cause }, + ); + } + }); + const declaredVersions = new Map( + locks.map(({ dir, lock }) => [dir, topLevelVersions(lock)]), + ); + const declared = declaredPackages(manifests); + const declaredSkew = findDeclaredSkew(declared, declaredVersions); + const { failures: declaredFailures, ignored: declaredIgnored } = + partitionDeclaredSkew(declaredSkew); + + if (declaredFailures.length > 0) { + console.error( + `verify:dep-lockstep — ${declaredFailures.length} declared ${declaredFailures.length === 1 ? "dependency resolves" : "dependencies resolve"} to different versions across installs:\n`, + ); + let anyListed = false; + for (const failure of declaredFailures) { + const listed = TOLERATED_DECLARED_SKEW.has(failure.name); + anyListed ||= listed; + console.error( + ` ${failure.name}${listed ? " (allowlisted — but this is a MAJOR skew)" : ""}`, + ); + for (const { dir, version } of failure.holders) + console.error( + ` ${version} (${dir}/node_modules/${failure.name})`, + ); + } + console.error( + "\nThese are packages this repo declares somewhere, held at two versions across our installs —" + + "\nthe `one version per install-crossing dependency` rule in AGENTS.md. Unlike the check above," + + "\nthis tier does not require the copies to meet in one `tsc` program, so it catches a copy that" + + "\narrives TRANSITIVELY into one install (cli's `@types/node`, hoisted via `@types/express`, #2226)" + + "\nor as a peer shadow no program loads at all.", + ); + console.error( + "\nAlign them with `npm update ` in whichever install is behind — that moves the lockfile" + + "\nwithin the declared range, without widening a range as `npm install @` would. If a" + + "\ntransitive copy will not move, pin it with an `overrides` entry in that install (see" + + "\nclients/cli). If the skew is genuinely benign, add it to TOLERATED_DECLARED_SKEW in" + + "\nscripts/verify-dep-lockstep.mjs with the reason.", + ); + if (anyListed) + console.error( + "\nNote: an allowlisted package is tolerated only WITHIN a major version. Align the major.", + ); + process.exit(1); + } + const note = ignored.length > 0 ? `, ${ignored.length} tolerated` : ""; + const declaredNote = + declaredIgnored.length > 0 ? `, ${declaredIgnored.length} tolerated` : ""; console.log( `verify:dep-lockstep — OK: ${found.size} install-crossing dependencies agree across ${dirs.length} installs${note} ` + - `(derived from ${programs.length} tsc programs).`, + `(derived from ${programs.length} tsc programs); ` + + `${countDeclaredHeld(declared, declaredVersions)} declared dependencies agree across those installs${declaredNote}.`, ); } +/** How many declared packages more than one install holds — the declared tier's candidate count. */ +export function countDeclaredHeld(declared, versions) { + let n = 0; + for (const name of declared) { + let holders = 0; + for (const byName of versions.values()) if (byName.has(name)) holders += 1; + if (holders > 1) n += 1; + } + return n; +} + // Run only when executed directly (`node scripts/verify-dep-lockstep.mjs`); // importing this file (tests) exposes the pure helpers without running the guard. if (process.argv[1] && import.meta.url === pathToFileURL(process.argv[1]).href) diff --git a/scripts/verify-dep-lockstep.test.mjs b/scripts/verify-dep-lockstep.test.mjs index edcbbc4f..b648c276 100644 --- a/scripts/verify-dep-lockstep.test.mjs +++ b/scripts/verify-dep-lockstep.test.mjs @@ -13,11 +13,17 @@ import { test } from "node:test"; import assert from "node:assert/strict"; import { clientProjects, + countDeclaredHeld, + declaredPackages, + findDeclaredSkew, findSkew, hasReadableLockShape, lockVersionsByPath, majorOf, + partitionDeclaredSkew, partitionSkew, + toleratesSkew, + topLevelVersions, } from "./verify-dep-lockstep.mjs"; test("clientProjects: a `typecheck` script's projects win over references", () => { @@ -402,3 +408,247 @@ test("majorOf: prerelease and build metadata are irrelevant", () => { for (const bad of ["next", "", undefined, null, "v4.4.3"]) assert.equal(majorOf(bad), null, JSON.stringify(bad)); }); + +// --------------------------------------------------------------------------- +// The declared tier (#2226). One case per rule, same convention as above. +// --------------------------------------------------------------------------- + +test("declaredPackages: dependencies and devDependencies from every install", () => { + // devDependencies count because the boundary is "does the repo name it", not + // "does it ship" — `@types/node` and the toolchain are exactly the case #2226 + // is about, and all of them are devDependencies. + assert.deepEqual( + [ + ...declaredPackages([ + { dir: ".", manifest: { dependencies: { react: "^19.0.0" } } }, + { + dir: "clients/web", + manifest: { + dependencies: { "react-dom": "^19.2.4" }, + devDependencies: { "@types/react": "^19.2.14" }, + }, + }, + ]), + ].sort(), + ["@types/react", "react", "react-dom"], + ); +}); + +test("declaredPackages: peer and optional ranges are not declarations", () => { + // A peer range constrains the consumer rather than naming what we install; + // the copy npm auto-installs to satisfy one is still caught, because it lands + // top-level in an install whose sibling declares the same name. + assert.deepEqual( + [ + ...declaredPackages([ + { + dir: ".", + manifest: { + peerDependencies: { react: "^19.0.0" }, + optionalDependencies: { fsevents: "^2.3.3" }, + }, + }, + ]), + ], + [], + ); +}); + +test("declaredPackages: a manifest with no dependency fields contributes nothing", () => { + assert.equal( + declaredPackages([{ dir: "clients/launcher", manifest: { name: "l" } }]) + .size, + 0, + ); +}); + +test("topLevelVersions: nested copies are excluded", () => { + // Top-level is what the install's own code resolves and what a range in that + // manifest governs. A nested copy exists because some dependency asked for a + // different version, so it is constrained by that dependency, not by us. + const versions = topLevelVersions({ + lockfileVersion: 3, + packages: { + "": { name: "fixture" }, + "node_modules/zod": { version: "4.4.3" }, + "node_modules/outer/node_modules/zod": { version: "3.0.0" }, + "clients/web/node_modules/zod": { version: "9.9.9" }, + }, + }); + assert.deepEqual([...versions], [["zod", "4.4.3"]]); +}); + +test("findDeclaredSkew: two installs disagreeing is skew, and every holder is named", () => { + const skewed = findDeclaredSkew( + new Set(["@types/node"]), + new Map([ + [".", new Map([["@types/node", "24.13.3"]])], + ["clients/cli", new Map([["@types/node", "24.13.1"]])], + ["clients/web", new Map([["@types/node", "24.13.3"]])], + ]), + ); + assert.deepEqual(skewed, [ + { + name: "@types/node", + holders: [ + { dir: ".", version: "24.13.3" }, + { dir: "clients/cli", version: "24.13.1" }, + { dir: "clients/web", version: "24.13.3" }, + ], + }, + ]); +}); + +test("findDeclaredSkew: two CLIENTS disagreeing is skew, with no root copy involved", () => { + // No single `tsc` program can see this pair, which is why the program tier + // structurally cannot report it (`@types/react`, web against tui). + assert.deepEqual( + findDeclaredSkew( + new Set(["@types/react"]), + new Map([ + [".", new Map()], + ["clients/web", new Map([["@types/react", "19.2.18"]])], + ["clients/tui", new Map([["@types/react", "19.2.17"]])], + ]), + ).map((s) => s.name), + ["@types/react"], + ); +}); + +test("findDeclaredSkew: one holder cannot skew, and agreement is not skew", () => { + assert.deepEqual( + findDeclaredSkew( + new Set(["only-root", "agreed"]), + new Map([ + [ + ".", + new Map([ + ["only-root", "1.0.0"], + ["agreed", "2.0.0"], + ]), + ], + ["clients/web", new Map([["agreed", "2.0.0"]])], + ]), + ), + [], + ); +}); + +test("findDeclaredSkew: an installed package no manifest declares is not a candidate", () => { + assert.deepEqual( + findDeclaredSkew( + new Set(), + new Map([ + [".", new Map([["undeclared", "1.0.0"]])], + ["clients/web", new Map([["undeclared", "2.0.0"]])], + ]), + ), + [], + ); +}); + +test("findDeclaredSkew: results are sorted by package name", () => { + const two = new Map([ + [ + ".", + new Map([ + ["zod", "1.0.0"], + ["ajv", "1.0.0"], + ]), + ], + [ + "clients/web", + new Map([ + ["zod", "2.0.0"], + ["ajv", "2.0.0"], + ]), + ], + ]); + assert.deepEqual( + findDeclaredSkew(new Set(["zod", "ajv"]), two).map((s) => s.name), + ["ajv", "zod"], + ); +}); + +test("partitionDeclaredSkew: deny by default", () => { + const skewed = [ + { + name: "zod", + holders: [ + { dir: ".", version: "4.4.3" }, + { dir: "w", version: "4.3.6" }, + ], + }, + ]; + const { failures, ignored } = partitionDeclaredSkew(skewed, new Map()); + assert.deepEqual( + failures.map((f) => f.name), + ["zod"], + ); + assert.deepEqual(ignored, []); +}); + +test("partitionDeclaredSkew: an allowlisted package tolerates only a within-major skew", () => { + const within = { + name: "hono", + holders: [ + { dir: ".", version: "4.1.0" }, + { dir: "w", version: "4.2.0" }, + ], + }; + const across = { + name: "hono", + holders: [ + { dir: ".", version: "4.1.0" }, + { dir: "w", version: "5.0.0" }, + ], + }; + const allow = new Map([["hono", "reason"]]); + assert.deepEqual(partitionDeclaredSkew([within], allow).ignored.length, 1); + assert.deepEqual(partitionDeclaredSkew([across], allow).failures.length, 1); +}); + +test("toleratesSkew: the within-a-major rule is stated once for both tiers", () => { + const allow = new Map([["p", "reason"]]); + assert.equal( + toleratesSkew("p", [{ version: "1.1.0" }, { version: "1.2.0" }], allow), + true, + ); + assert.equal( + toleratesSkew("p", [{ version: "1.1.0" }, { version: "2.0.0" }], allow), + false, + ); + // Unparseable can't be proven same-major, so it fails rather than passes. + assert.equal( + toleratesSkew("p", [{ version: "1.1.0" }, { version: "next" }], allow), + false, + ); + assert.equal( + toleratesSkew("other", [{ version: "1.1.0" }, { version: "1.2.0" }], allow), + false, + ); +}); + +test("countDeclaredHeld: counts declared names more than one install holds", () => { + const versions = new Map([ + [ + ".", + new Map([ + ["shared", "1.0.0"], + ["root-only", "1.0.0"], + ]), + ], + [ + "clients/web", + new Map([ + ["shared", "1.0.0"], + ["undeclared", "1.0.0"], + ]), + ], + ]); + assert.equal( + countDeclaredHeld(new Set(["shared", "root-only", "undeclared"]), versions), + // `shared` alone: the other two are held by one install each. + 1, + ); +}); From f48cf2a90ded5c971446267e40c94830b76ff3c3 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sat, 5 Sep 2026 14:54:48 -0400 Subject: [PATCH 03/16] fix: narrow the crypto-member delete cast in the attempt-id tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `Record` does not overlap `Crypto` (TS2352 under `tsc -b`). `Partial>` says the same thing in one legal cast and keeps `delete` operating on a known-optional property. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Uw1s4LRBUzrFwLzPT4mAJn Signed-off-by: cliffhall --- clients/web/src/lib/oauthResume.test.ts | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/clients/web/src/lib/oauthResume.test.ts b/clients/web/src/lib/oauthResume.test.ts index aa624204..ce68c60e 100644 --- a/clients/web/src/lib/oauthResume.test.ts +++ b/clients/web/src/lib/oauthResume.test.ts @@ -49,7 +49,11 @@ function withoutCryptoMember( if (original) { Object.defineProperty(globalThis.crypto, name, original); } else { - delete (globalThis.crypto as Record)[name]; + delete ( + globalThis.crypto as Partial< + Pick + > + )[name]; } } } From 7d0ae9e9ecd05f5cd00b1e3bf1133dc94e893df0 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sat, 5 Sep 2026 14:59:05 -0400 Subject: [PATCH 04/16] =?UTF-8?q?Reshape=20the=20testing=20=E2=86=92=20tes?= =?UTF-8?q?t-servers=20pointer=20into=20an=20imperative=20step?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The hand-off measured 33% / 33% at RUNS=3 (#2204's two committed chain cases). `testing` opened with a one-sentence ⚠️ *classifying* which work belongs to `test-servers` — a fact, which the model skims — and said it nowhere else except a bare cross-reference in the last section. Rewrite it as an action with a trigger ("load the `test-servers` skill now — that is step one"), enumerate the situations that qualify, and repeat the pointer at the two later points where the run actually decides it is writing an integration test: the `integration` project bullet under "Where the test file goes", and the tier list. The top of a body is read before the model knows it needs the second skill, so a pointer that lives only there has already been scrolled past by the time it matters. Measured at RUNS=5 on the full suite: the two hand-off cases go to 100% / 80% (100% / 100% on a focused `-- test-servers` run), and all 63 first-move cases stay at 100% — no displacement, as expected, since neither description changed. Also update the two places that recorded the old number as the standing state: the worked example in `docs/skill-authoring.md`, and the `CHAIN_THRESHOLD` rationale in `scripts/skill-eval.mjs`, which cites the 33% measurement as the reason the bar is 0.5 rather than 0.8. The bar is unchanged — a reshaped pointer raises the ceiling those two cases reach, not the floor a new hand-off case should be judged against. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01EQVdemwgUhh45yzeaRSLhW Signed-off-by: cliffhall --- .claude/skills/testing/SKILL.md | 32 ++++++++++++++++++++++++-------- docs/skill-authoring.md | 28 ++++++++++++++++++++-------- scripts/skill-eval.mjs | 11 +++++++---- 3 files changed, 51 insertions(+), 20 deletions(-) diff --git a/.claude/skills/testing/SKILL.md b/.claude/skills/testing/SKILL.md index 7a597e4c..56553ed0 100644 --- a/.claude/skills/testing/SKILL.md +++ b/.claude/skills/testing/SKILL.md @@ -11,11 +11,18 @@ statements, functions, and branches.** That rule and the React/Mantine conventions live in [`AGENTS.md`](../../../AGENTS.md); this skill is where a test goes, how to run it, and how to clear the gate. -⚠️ **Anything that needs a real server to run against — an integration test, a -smoke, reproducing a bug by hand — is `/test-servers`, and you have to load it.** -Integration and smoke tests here drive a real server over a real transport -rather than a mock, so picking, building and connecting to a fixture is a -procedure of its own that this skill does not carry. +## Before you write it: does the test need a real server? + +**If it does, load the `test-servers` skill now — that is step one, before +choosing a location or writing a line.** It does, whenever the task is: an +integration test; an end-to-end test; a smoke; a coverage gap that has to be +exercised over a transport; or reproducing a reported bug by hand. + +Every one of those drives a **real server over a real transport, never a mock**, +and picking the fixture, building it, and connecting with the right protocol era +is a procedure this skill does not carry. Writing one without `test-servers` +means hand-rolling a fixture that already exists, or mocking the thing the tier +exists to avoid mocking. ## Where the test file goes @@ -38,7 +45,9 @@ web-owned test living under `src/test/` instead is a bug. `core/` source layout (`mcp/`, `mcp/node/`, `mcp/remote/`, `auth/`, `auth/node/`, `storage/`). **Placement is the manifest** — any file under that folder is picked up by the integration project (node env, 30s timeouts) via a - folder glob; there is no enumeration to keep in sync. + folder glob; there is no enumeration to keep in sync. ⚠️ These run against a + real server, so **load the `test-servers` skill before writing one** — the + fixture is half the test. 3. **Shared test infrastructure** — `renderWithMantine.tsx`, `setup.ts`, `fixtures/`, `scrollAreaStoryAssertions.ts`. @@ -82,6 +91,10 @@ spawns the built binary) → smokes through the built launcher (`npm run smoke`) Storybook play functions (`test:storybook`) → the published-tarball check (`npm run pack:verify`, local/release only — needs network). +Everything from **web integration** rightwards needs a fixture from +`test-servers/` — load the `test-servers` skill as soon as a task puts you at +that tier or deeper. + `validate` runs the per-client `test` scripts — so web **unit** plus cli's out-of-process `e2e.test.ts`, but **not** web's integration project, which runs inside the `coverage` gate. CI therefore has no separate `test:integration` step. @@ -159,5 +172,8 @@ shared helper that wraps one. ## Test servers, not mocks -Integration and smoke tests drive a real server over a real transport. See -`/test-servers` for picking and building one. +Integration and smoke tests drive a real server over a real transport, never a +mock. **Load the `test-servers` skill to pick, build and run the fixture** — +which showcase config covers the feature, which protocol era to connect with, +how to add a combination that does not exist yet, and why a fixture can keep +serving stale code after an edit. diff --git a/docs/skill-authoring.md b/docs/skill-authoring.md index 6e515833..5692f3b1 100644 --- a/docs/skill-authoring.md +++ b/docs/skill-authoring.md @@ -289,13 +289,25 @@ pointed at the second, and the column would stop carrying signal. Read a hand-off number as a description-strength measurement, not a verdict — and read it at `RUNS=5`, since at `RUNS=3` one sample is worth 33 points. -**The committed cases have measured 33% / 33% on one `RUNS=3` run and 100% / -33% on another, and at least one of them being red is the intended state rather -than an oversight.** `skills:eval` is not a gate (see below), and the number is -the finding: `testing` points at `test-servers` in its first paragraph and the -model follows that pointer *sometimes*. Strengthening it is its own change -against its own issue (#2247); lowering the bar to turn the column green would -throw away the only signal this feature adds. +**A red hand-off case is a finding about the pointer, not a build break — and +the fix is to reshape the pointer, never to lower the bar.** The committed +`testing` -> `test-servers` cases are the worked example. They measured +33% / 33% at `RUNS=3` when `testing` opened with a one-sentence ⚠️ *classifying* +which work belongs to `test-servers`; rewriting that into an imperative first +step ("load the `test-servers` skill now — that is step one"), and repeating it +at the two later points where the model actually decides it is writing an +integration test, took them to **100% / 80% at `RUNS=5`** with the prompts +unchanged (#2247) — 100% / 100% on a focused `-- test-servers` run of the same +build, which is the size of the run-to-run noise still present at `RUNS=5`. +Nothing else moved: the two descriptions were not touched, and the same suite +scored **63/63** first-move cases at 100%. + +The transferable part is that **a pointer is followed when it reads as an action +with a trigger, and skimmed when it reads as a fact.** #2202 found the same +lever on a *description*'s shape; this is it applied to a body. The corollary is +where to put one: the top of a body is read before the model knows it needs the +second skill, so a pointer that lives only there is a pointer it has already +scrolled past by the time it matters. ⚠️ **Do not read a rise between two `RUNS=3` runs as an improvement.** One sample is 33 points there, and the two runs above straddle a 67-point swing on @@ -372,7 +384,7 @@ The summary is two lines, never one: ``` 7/7 first-move cases at or above 80%. -1/2 hand-off cases above 50%. +2/2 hand-off cases above 50%. ``` Narrowing the run never narrows what a **negative** case is scored against — a diff --git a/scripts/skill-eval.mjs b/scripts/skill-eval.mjs index 748cdaba..1fc38b6c 100755 --- a/scripts/skill-eval.mjs +++ b/scripts/skill-eval.mjs @@ -57,10 +57,13 @@ const THRESHOLD = Number(process.env.THRESHOLD ?? 0.8); // acceptable is a separate judgement rather than one inherited from a number // tuned for the other measurement. 0.5 is the weakest claim worth asserting — // the pointer is taken more often than not. It is deliberately not 0.8: the -// committed `testing` -> `test-servers` cases measure 33% (RUNS=3) against a -// pointer that is live and stated in the first paragraph of `testing`'s body, -// so an 0.8 bar would mark every hand-off red regardless of how strongly the -// first skill points at the second, and the column would stop carrying signal. +// committed `testing` -> `test-servers` cases measured 33% (RUNS=3) against a +// pointer that was live and stated in the first paragraph of `testing`'s body, +// so an 0.8 bar would mark a hand-off red regardless of how strongly the first +// skill points at the second, and the column would stop carrying signal. +// (#2247 later reshaped that pointer into an imperative step and took the same +// two cases to 100% at RUNS=5 — which raises the ceiling those cases reach, not +// the floor a *new* hand-off case should be judged against.) // // It is compared STRICTLY, unlike the first-move threshold. "More often than // not" is `> 0.5`, and an inclusive compare passes exactly half the samples From 8ed0bed086937c2feeba0deff05a6a1c75144eb1 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sat, 5 Sep 2026 15:53:34 -0400 Subject: [PATCH 05/16] fix: call getRandomValues directly, and test true crypto absence Copilot review round 1. - `newAttemptId` calls `globalThis.crypto.getRandomValues(...)` directly rather than through a bound alias. CodeQL's `js/insecure-randomness` browser model recognizes a secure RNG by that literal method call, so the indirection would have left the `Math.random` last resort classified as an unmitigated source. - The no-crypto test arm now removes `globalThis.crypto` entirely instead of hiding its two methods, matching the crypto-absence test in `src/test/core/auth/utils.test.ts`. Hiding only the methods left the global truthy, so the arm could not have caught a regression that read `globalThis.crypto` without the optional guard. `withoutCryptoMember` narrows to `randomUUID`, its only remaining caller. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Uw1s4LRBUzrFwLzPT4mAJn Signed-off-by: cliffhall --- clients/web/src/lib/oauthResume.test.ts | 60 +++++++++++++++---------- clients/web/src/lib/oauthResume.ts | 12 ++--- 2 files changed, 43 insertions(+), 29 deletions(-) diff --git a/clients/web/src/lib/oauthResume.test.ts b/clients/web/src/lib/oauthResume.test.ts index ce68c60e..835dc799 100644 --- a/clients/web/src/lib/oauthResume.test.ts +++ b/clients/web/src/lib/oauthResume.test.ts @@ -29,15 +29,15 @@ import { /** * Run `body` with one `crypto` member hidden, then restore it. * - * `randomUUID` and `getRandomValues` are inherited from `Crypto.prototype`, so - * there is normally no OWN descriptor to put back — restoring only when one - * existed would leave the `undefined` own property in place and force every - * later test in this file onto the fallback path. + * `randomUUID` is inherited from `Crypto.prototype`, so there is normally no + * OWN descriptor to put back — restoring only when one existed would leave the + * `undefined` own property in place and force every later test in this file + * onto the fallback path. + * + * This hides one *member*; the arm that needs the whole `crypto` global gone + * stubs `globalThis.crypto` itself instead (see below). */ -function withoutCryptoMember( - name: "randomUUID" | "getRandomValues", - body: () => void, -): void { +function withoutCryptoMember(name: "randomUUID", body: () => void): void { const original = Object.getOwnPropertyDescriptor(globalThis.crypto, name); Object.defineProperty(globalThis.crypto, name, { configurable: true, @@ -49,11 +49,7 @@ function withoutCryptoMember( if (original) { Object.defineProperty(globalThis.crypto, name, original); } else { - delete ( - globalThis.crypto as Partial< - Pick - > - )[name]; + delete (globalThis.crypto as Partial>)[name]; } } } @@ -516,19 +512,35 @@ describe("oauthResume", () => { }); it("writeOAuthResumeSnapshot falls back to Math.random with no crypto at all", () => { + // The whole global goes, not just its two methods — an exotic runtime with + // no WebCrypto at all, matching `src/test/core/auth/utils.test.ts`. Hiding + // only the methods would leave `globalThis.crypto` truthy and so would not + // catch a regression that reads it without the optional guard (Copilot). const mathSpy = vi.spyOn(Math, "random"); - let token: string | undefined; - withoutCryptoMember("randomUUID", () => { - withoutCryptoMember("getRandomValues", () => { - token = writeOAuthResumeSnapshot({ - version: 1, - serverId: "a", - activeTab: "tools", - authKind: "reauth", - tabUi: {}, - }); - }); + // `crypto` IS an own property of the global (unlike the `Crypto.prototype` + // members above), so there is always a descriptor to restore. Asserted + // rather than `!`-ed so a change in that assumption fails loudly here. + const original = Object.getOwnPropertyDescriptor(globalThis, "crypto"); + expect(original).toBeDefined(); + Object.defineProperty(globalThis, "crypto", { + configurable: true, + writable: true, + value: undefined, }); + let token: string | undefined; + try { + token = writeOAuthResumeSnapshot({ + version: 1, + serverId: "a", + activeTab: "tools", + authKind: "reauth", + tabUi: {}, + }); + } finally { + if (original) { + Object.defineProperty(globalThis, "crypto", original); + } + } expect(mathSpy).toHaveBeenCalled(); expect(token).toEqual(expect.any(String)); expect(clearOwnOAuthResumeSnapshot(token)).toBe(true); diff --git a/clients/web/src/lib/oauthResume.ts b/clients/web/src/lib/oauthResume.ts index b0ebf13b..235840f0 100644 --- a/clients/web/src/lib/oauthResume.ts +++ b/clients/web/src/lib/oauthResume.ts @@ -240,11 +240,13 @@ function newAttemptId(): string { if (uuid) { return uuid(); } - const getRandomValues = globalThis.crypto?.getRandomValues?.bind( - globalThis.crypto, - ); - if (getRandomValues) { - const bytes = getRandomValues(new Uint8Array(16)); + // Called directly rather than through a bound alias: CodeQL's + // `js/insecure-randomness` browser model recognizes a secure RNG by the + // literal `crypto.getRandomValues(...)` method call, and an alias does not + // match it — so the indirection would leave the `Math.random` last resort + // below classified as an unmitigated source (Copilot). + if (globalThis.crypto?.getRandomValues) { + const bytes = globalThis.crypto.getRandomValues(new Uint8Array(16)); return Array.from(bytes, (b) => b.toString(16).padStart(2, "0")).join(""); } return `${Date.now().toString(36)}-${Math.random().toString(36).slice(2)}`; From 8e2c4d49fad47744455f4c139d1427c630961433 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sat, 5 Sep 2026 16:09:44 -0400 Subject: [PATCH 06/16] chore: address Copilot review round 1 (#2226) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Include `optionalDependencies` in the declared tier's candidate union. A peer range constrains the consumer's host; an optional range is a direct declaration npm attempts to install, so omitting it left an optional-only package free to skew unseen. `peerDependencies` stays excluded, with the reason stated. - Make the end-to-end fixture's client copy UNDECLARED — only the root manifest names `solo`, while `clients/web` merely holds a top-level copy. That is #2226's defining shape, and it is what makes the candidate set a union across installs rather than a per-install intersection; the old fixture would have passed a per-install implementation. Adds the mirror case (declared only by a client, held by the root) so the union is pinned in both directions. - Update the two docs that still described the single-tier guard: `.claude/skills/local-dev/SKILL.md` said "nothing gates either of those, and `verify:dep-lockstep` is not it" and cited the live `@types/node` drift in the present tense; `docs/quality-gate.md` described only the program tier. Both now state what each tier covers and the two limits that remain. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01J86a6vMcA9csPtdsg7ZcCZ Signed-off-by: cliffhall --- .claude/skills/local-dev/SKILL.md | 48 ++++++++++++++++------- docs/quality-gate.md | 8 +++- scripts/verify-dep-lockstep.main.test.mjs | 41 +++++++++++++++---- scripts/verify-dep-lockstep.mjs | 31 +++++++++++---- scripts/verify-dep-lockstep.test.mjs | 30 +++++++------- 5 files changed, 109 insertions(+), 49 deletions(-) diff --git a/.claude/skills/local-dev/SKILL.md b/.claude/skills/local-dev/SKILL.md index e830d9af..947211e7 100644 --- a/.claude/skills/local-dev/SKILL.md +++ b/.claude/skills/local-dev/SKILL.md @@ -126,7 +126,8 @@ 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. +for the skew — in both its tiers since #2226 — and consolidation is what removes +the opportunity. Two consequences that read as bugs and are not: @@ -209,18 +210,27 @@ on disk. The two mechanisms are **not** equally safe, and neither is a guarantee wide range (`eslint-plugin-react-refresh` accepts `^9 || ^10`) the copies agree only because npm happens to resolve the same latest in both installs, which is a coincidence that holds until it doesn't. -- A **transitive** copy is constrained by nothing of ours whatsoever, and one has - already diverged: cli's `@types/node` is `24.13.1` against the root's - `24.13.3`, and was `24.13.1` on `v2/main` too — a declared `^24.12.4` loses to - a nearer transitive. +- A **transitive** copy is constrained by nothing of ours whatsoever, and one had + already diverged: cli's `@types/node` was `24.13.1` against the root's + `24.13.3`, on `v2/main` too — a declared `^24.12.4` loses to a nearer + transitive. It is pinned with an `overrides` entry in `clients/cli` since + #2226. -⚠️ **Nothing gates either of those, and `verify:dep-lockstep` is not it.** -That guard derives its candidate set from what each `tsc` **program** resolves -(see below), so it sees only packages a program loads from two installs. A tool -*binary* — `eslint`, `prettier`, `vitest` — never enters a program, so it is -outside the candidate set no matter how far it drifts, and the cli `@types/node` -skew above passes for a second reason on top of that: no one program sees both -copies. When you change what a client declares, check by hand from that client: +✅ **`verify:dep-lockstep` gates both of those since #2226.** Its second tier +compares every package **any** install *declares* — `dependencies`, +`devDependencies` and `optionalDependencies`, unioned across the root and all +four clients — against every **top-level** copy in every install, independent of +what a `tsc` program resolves. So a tool *binary* that no program loads +(`eslint`, `typescript`, `vitest`) and a transitive copy that no single program +meets (the cli `@types/node` above) are both in scope now, as is a skew between +two **clients** with no root copy involved (`@types/react`, web against tui). + +⚠️ **Two gaps remain, and they are why the by-hand check below is still worth +running.** The tier reads **lockfiles**, so a copy you installed by hand and +never committed is invisible to it; and it compares only names some manifest +declares, so a purely transitive package no manifest anywhere names stays the +program tier's business. When you change what a client declares, check by hand +from that client: ```sh cd clients/web && npm exec -- which eslint prettier tsc vitest @@ -307,9 +317,17 @@ 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 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. +versions; `npm run verify:dep-lockstep` is the guard, and it runs **two tiers**. +The first 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. The second (#2226) compares every **declared** package's +top-level copies across installs whatever any program loads, so a transitive or +peer-shadow drift no program can meet is caught too. + +Aligning a stale install is `npm update ` there — it moves the lockfile +within the declared range without widening the range, which +`npm install @` would. A transitive copy that will not move takes +an `overrides` entry in that install (see the next section). ### Why `overrides` beats `npm audit fix` diff --git a/docs/quality-gate.md b/docs/quality-gate.md index a7c988a3..bfea1f64 100644 --- a/docs/quality-gate.md +++ b/docs/quality-gate.md @@ -29,7 +29,7 @@ That is the readable half, and prose rots. The enforced half is `scripts/lib/wor | Script | What it does | | ----------------------------------- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -| `npm run validate` | Runs the four durable guards first — `verify:format-coverage` (every tracked source file is format-gated), `verify:skills` (every `.claude/skills` manifest parses and declares its invocation mode; the **model-invoked** ones also carry eval cases — the name-only skills intentionally have none), `verify:typecheck-coverage` (every tracked TS file lands in a tsconfig project), `verify:dep-lockstep` (no dependency reaching one `tsc` program from two installs skews across them) — then `test:scripts` (the guards' own parser unit tests), then `validate:core` (the shared `core/` `format:check` + `lint` gate — including the `core/react/**` React-hooks block added by #2192, whose plugin is a **root** devDependency because a client-local copy is unreachable from the root: Node resolution walks up, not down), then per client: `format:check` + `lint` + **`typecheck`** (cli/tui/launcher; web typechecks via `tsc -b` inside its `build`) + `build` + fast unit tests. The quick inner-loop check. | +| `npm run validate` | Runs the four durable guards first — `verify:format-coverage` (every tracked source file is format-gated), `verify:skills` (every `.claude/skills` manifest parses and declares its invocation mode; the **model-invoked** ones also carry eval cases — the name-only skills intentionally have none), `verify:typecheck-coverage` (every tracked TS file lands in a tsconfig project), `verify:dep-lockstep` (two tiers: no dependency reaching one `tsc` program from two installs skews across them, and — since #2226 — no **declared** package holds two different top-level versions across installs at all) — then `test:scripts` (the guards' own parser unit tests), then `validate:core` (the shared `core/` `format:check` + `lint` gate — including the `core/react/**` React-hooks block added by #2192, whose plugin is a **root** devDependency because a client-local copy is unreachable from the root: Node resolution walks up, not down), then per client: `format:check` + `lint` + **`typecheck`** (cli/tui/launcher; web typechecks via `tsc -b` inside its `build`) + `build` + fast unit tests. The quick inner-loop check. | | `npm run coverage` | The **per-file ≥90% gate** (lines/statements/functions/branches) under v8 instrumentation, per client. CI-enforced. For web this also runs the integration project and covers the shared `core/` runtime (including `core/json` and `core/client`). | | `npm run smoke` | End-to-end smokes through the built launcher (`--help` dispatch + prod cli/tui/web — the TUI one runs under a **pseudoterminal** and asserts the Ink app is still running two seconds after its first frame, not merely that it painted one, [#2147](https://github.com/modelcontextprotocol/inspector/issues/2147)), plus three headless-browser smokes: a boot smoke that runs the prod web bundle and asserts a clean first render (no uncaught error — sync exception or unhandled rejection, how a Node built-in reaching the browser bundle manifests), and an **MCP Apps** smoke (`smoke:web:app`) that drives connect → open app → `data-app-status="ready"` against a composable App server, covering the sandbox proxy and UI-protocol bridge, and an **app-rendered elicitation** smoke (`smoke:web:elicit`) that drives one end to end — call the tool, answer inside the sandboxed app, see the app's `ElicitResult` reach the server — and then the same tool against a server that never advertised the capability, which must fall back to the native elicitation form. Those three take their engine from `SMOKE_BROWSER` — see [Supported browsers](#supported-browsers). A fourth, **`smoke:web:tabs`**, drives the core tabs against one server on one browser launch — Tools runs a tool and asserts its `structuredContent` section, Resources reads a resource with templates listed, Prompts fetches one ([#2148](https://github.com/modelcontextprotocol/inspector/issues/2148)) — asserting the `data-*` contract in [the web README](../clients/web/README.md#core-tab-automation-contract) rather than visible copy. It is **Chromium-only**: those tabs are ordinary React and Mantine, so unlike the sandbox they are not engine-sensitive. | | `npm run verify:build-gate` | Runs a real `vite build` with a Node built-in forced into the browser graph and asserts the build **fails** via the #1769 gate (which turns Vite's browser-externalization warning into a hard error). Guards against the warning phrasing drifting in a Vite bump and silently disabling the gate. Part of `npm run local:gate` and of GitHub CI. | @@ -39,7 +39,11 @@ That is the readable half, and prose rots. The enforced half is `scripts/lib/wor | `npm run verify:skills:cli` | Runs `claude plugin validate` — the **authoritative** skill schema — as a guaranteed step, in `local:gate` and in CI. `verify:skills` reimplements the parse and *skips* when the CLI is absent, which is right for `validate` (fast, offline, no Claude Code required) but adds up to "never runs". This one resolves the CLI instead: an installed one **only when it matches the pin exactly**, otherwise the pinned package via `npx -y @anthropic-ai/claude-code@…`. Exact, not a floor: accepting a newer local CLI would validate against a different schema than CI's, so the same `local:gate` could disagree across machines — which is the failure a pin exists to prevent. Needs no authentication. Not in `validate` — it can reach the network. | | `npm run test:scripts` | Table-driven unit tests (`node --test`) for the guard's own pure parsers (`scripts/lib/npm-scripts.mjs`, `scripts/lib/tsc-program.mjs` + the exported helpers of `verify-typecheck-coverage.mjs` and `verify-dep-lockstep.mjs`), one case per rule they encode, plus two suites over shared `scripts/lib` helpers that no smoke can check itself: `resolve-node-bin.test.mjs` — the cross-platform bin resolver (#1939), pinned against the real `bin`/`exports` shapes of the packages the scripts actually spawn — and `announced-child.test.mjs` — the spawn/readiness ownership helper (#2000), which drives real `node -e` children to prove a child that never announces is still published to the caller before the timeout throws, and so is reachable by teardown rather than orphaned. Four more do the same: `mcp-app-flow.test.mjs` covers the shared MCP Apps flow (#2003) — the deep link's two CSRF gates and `appArgs` encoding, plus `driveAppFlow`'s failure branches against a stand-in page, all of which are dead code from the happy-path smokes' point of view and would otherwise surface only as opaque timeouts; `render-smoke.test.mjs` and `pty.test.mjs` cover the TUI boot harness ([#2147](https://github.com/modelcontextprotocol/inspector/issues/2147)) — the former driving real `node -e` stubs to prove that a child which paints the marker and *then* dies is a **failure**, which the old harness reported as OK and which no fixed TUI can reproduce; the latter pinning the three mutually-incompatible `script(1)` invocations, where a wrong guess stops the pseudoterminal from being allocated at all; and `ensure-test-servers.test.mjs` pins the [#2111](https://github.com/modelcontextprotocol/inspector/issues/2111) invariant — that `test-servers/build` is rebuilt **even when it already exists** — which no smoke can assert about itself, since one driving a stale fixture reports a product failure rather than a staleness one. `workflow-gate.test.mjs` is a different shape again ([#2146](https://github.com/modelcontextprotocol/inspector/issues/2146)): besides the table of parser cases it runs the parser over the repo's real `.github/workflows/**`, so a workflow that invokes the local-only gate or a non-Chromium engine pass fails here rather than in a CI run nobody expected to be red. Five more arrive with the skills (#2163). `skill-manifest.test.mjs` pins the frontmatter contract one case per rule — most importantly that a fence which is not a whole line is not a terminator, and that malformed YAML is an error rather than an empty description. `verify-skills.main.test.mjs` drives that guard's `main()` over fixture directories, the sibling guards' `*.main.test.mjs` pattern, so it cannot stop enforcing while the parser tests stay green. `skill-eval.test.mjs` covers the trigger eval's stream parsing and its run classification, which an eval run's happy path never reaches and which would otherwise report a plausible hit rate for a CLI that failed to run. `verify-skills-cli.test.mjs` covers which validator gets run — a local CLI only at the exact pin — and the orchestration around it, neither of which an ordinary run on a working machine exercises. `claude-cli.test.mjs` covers the one place that decides how the CLI is spawned — the Windows `.cmd` shim needs a shell, and its arguments then need quoting — with the platform **injected**, since a win32-only bug that no runner here executes is exactly the kind that ships. Runs in `validate` — and `verify:typecheck-coverage` guards *this* gate in turn (reachable from `validate`, non-empty test set, every test file matched by the `test:scripts` glob), since `node --test` silently skips a file its glob misses and still exits 0. | | `npm run verify:typecheck-coverage` | The typecheck-coverage analog of the above (#1791): for each Node client (auto-discovered from disk — enrolled via its `typecheck` script's projects, or for a `tsc -b` client like `clients/web` via its `tsconfig.json` `references`) it runs those projects with `tsc --listFilesOnly`, unions them, and **fails** listing any tracked `.ts`/`.tsx`/`.mts`/`.cts` under the client that lands in no project (so a new top-level config/helper can't silently go untypechecked). It also requires, deny-by-default, the first-party TS no client owns (`test-servers/src`, the root `vitest.shared.mts`, all of `core/`, and any new top-level location) to land in some client project's tsc pass — so a `core` `*.tsx` web's projects don't reach is caught too. Also asserts the gate is wired (each client's typecheck pass — its `typecheck` script, or web's `tsc -b` — is reachable from its `validate`, and the root chain runs each client's `validate`). Runs in `validate`. | -| `npm run verify:dep-lockstep` | Guards the "one version per install-crossing dependency" invariant (#1896). v2 is not a workspace, so a client's test project compiles the shared first-party TypeScript — `core/`, `test-servers/src`, and the root-owned `vitest.shared.mts`, all of which resolve their dependencies from the **root** install — alongside the client's own sources, putting the same package in one `tsc` program twice. At the same version that's harmless; skewed, TypeScript must relate two structurally-distinct copies of every type, which for a recursive-generic surface is exponential (zod `4.3.6` vs `4.4.3` exhausted the 4GB tsc heap in `clients/web`). Derives its candidate set from **what actually enters each program** (#1965) — every client tsconfig project listed with `tsc --listFilesOnly` via the shared `scripts/lib/tsc-program.mjs`, each resolved `node_modules` file mapped to its owning install, keeping the packages that reach one program from two installs (a package whose declarations arrive only through another package's `.d.ts`, as `@modelcontextprotocol/sdk`'s do, is invisible to a scan of first-party imports). Prices each copy from the lockfile entry for the exact install path the program resolved, compares only the installs that met in one program, and **fails deny-by-default** on any disagreement not in the annotated `TOLERATED_SKEW` allowlist — empty today — with an allowlisted package tolerated only *within a major version*. Runs in `validate`. +| `npm run verify:dep-lockstep` | Guards the "one version per install-crossing dependency" invariant (#1896). v2 is not a workspace, so a client's test project compiles the shared first-party TypeScript — `core/`, `test-servers/src`, and the root-owned `vitest.shared.mts`, all of which resolve their dependencies from the **root** install — alongside the client's own sources, putting the same package in one `tsc` program twice. At the same version that's harmless; skewed, TypeScript must relate two structurally-distinct copies of every type, which for a recursive-generic surface is exponential (zod `4.3.6` vs `4.4.3` exhausted the 4GB tsc heap in `clients/web`). Derives its candidate set from **what actually enters each program** (#1965) — every client tsconfig project listed with `tsc --listFilesOnly` via the shared `scripts/lib/tsc-program.mjs`, each resolved `node_modules` file mapped to its owning install, keeping the packages that reach one program from two installs (a package whose declarations arrive only through another package's `.d.ts`, as `@modelcontextprotocol/sdk`'s do, is invisible to a scan of first-party imports). Prices each copy from the lockfile entry for the exact install path the program resolved, compares only the installs that met in one program, and **fails deny-by-default** on any disagreement not in the annotated `TOLERATED_SKEW` allowlist — empty today — with an allowlisted package tolerated only *within a major version*. + +A **second tier** (#2226) runs alongside it, asking the weaker but broader question the `AGENTS.md` rule actually states: does a package this repo *declares* anywhere resolve to two versions across our installs at all? Its candidate set is every name in any install's `dependencies`/`devDependencies`/`optionalDependencies` — unioned across the root and all four clients, so a copy declared by only one of them still counts — that **more than one install holds a top-level copy of** (17 packages today). Nested copies are excluded: one exists because some dependency asked for a different version, so it is that dependency's range to govern, not ours. Neither tier subsumes the other — the program tier sees a copy no manifest names (`@modelcontextprotocol/sdk`, arriving through another package's `.d.ts`), while the declared tier sees a **transitive** copy no program loads (cli's `@types/node`, hoisted via `@types/express` — the case that motivated it), two **clients** disagreeing with no root copy involved (`@types/react`, web against tui), and the peer shadows `eslint`/`typescript`/`vitest` that never enter a program. Same deny-by-default and same within-a-major rule, against its own `TOLERATED_DECLARED_SKEW` — also empty. Two limits: it reads **lockfiles**, so an uncommitted hand-installed copy is invisible, and it compares only declared names, so a purely transitive package no manifest names stays the first tier's business. + +Runs in `validate`. | `npm run local:gate` | **Mandatory pre-push command.** `validate` → `verify:skills:cli` → `coverage` → `verify:build-gate` → `verify:bundle-externals` → `smoke` → `smoke:web:firefox` → `local:storybook`. A strict superset of GitHub CI — see [Two tiers](#two-tiers-github-ci-and-the-local-gate). Named `local:` rather than `ci` on purpose (#2146); there is no `npm run ci` alias. | | `npm run pack:verify` | Publish smoke — see [Publishing](./publishing.md). | diff --git a/scripts/verify-dep-lockstep.main.test.mjs b/scripts/verify-dep-lockstep.main.test.mjs index 9295b5cd..78f947a2 100644 --- a/scripts/verify-dep-lockstep.main.test.mjs +++ b/scripts/verify-dep-lockstep.main.test.mjs @@ -528,16 +528,20 @@ test("main: exits 1 when the root validate no longer runs the sibling guard", () // --------------------------------------------------------------------------- test("main: a DECLARED package skewed across installs fails even when no program holds both copies (#2226)", () => { - // The exact shape the issue reported. `solo` is installed under the root and - // under `clients/web`, and both manifests name it — but only the shared - // `core/` imports it, so the client's program resolves the root's copy alone - // and the program tier is silent, correctly. The declared tier is what sees - // it, which is the whole reason it exists: cli's `@types/node` was hoisted in - // via `@types/express` and no program ever met the root's copy. + // The exact shape the issue reported, and the client copy is deliberately + // UNDECLARED (Copilot): only the ROOT manifest names `solo`, while + // `clients/web` merely holds a top-level copy — the way cli's `@types/node` + // arrives, hoisted via `@types/express` with no range of ours governing it. + // That is what makes the candidate set a UNION across installs rather than a + // per-install intersection, and declaring it in both manifests would let a + // per-install implementation pass this test. + // + // Only the shared `core/` imports `solo`, so the client's program resolves the + // root's copy alone and the program tier is silent — correctly. The declared + // tier is the one that sees it, which is the whole reason it exists. withFixture( { rootManifestDeps: { solo: "^7.0.0" }, - webManifestDeps: { solo: "^7.0.0" }, webSolo: "7.0.0", }, (dir) => { @@ -553,10 +557,11 @@ test("main: a DECLARED package skewed across installs fails even when no program }); test("main: a declared package held at the same version in both installs passes and is counted", () => { + // Same union shape as above — root-declared, client copy undeclared — so the + // pass and the fail differ only in the version, not in how the name is found. withFixture( { rootManifestDeps: { solo: "^7.0.0" }, - webManifestDeps: { solo: "^7.0.0" }, webSolo: "7.8.9", }, (dir) => { @@ -567,6 +572,26 @@ test("main: a declared package held at the same version in both installs passes ); }); +test("main: the union runs the other way too — declared only by a CLIENT, held by the root", () => { + // The mirror of the case above, and the reason `declaredPackages` unions every + // install's manifest rather than reading the root's. `@types/react` is the + // live shape: web and tui declare it, the root does not, and the two clients + // disagreed. A root-manifest-only candidate set would report success here. + withFixture( + { + webManifestDeps: { solo: "^7.0.0" }, + webSolo: "7.0.0", + }, + (dir) => { + const { status, out } = runGuard(dir); + assert.equal(status, 1, out); + assert.match(out, /1 declared dependency resolves/); + assert.match(out, /7\.8\.9\s+\(\.\/node_modules\/solo\)/); + assert.match(out, /7\.0\.0\s+\(clients\/web\/node_modules\/solo\)/); + }, + ); +}); + test("main: an installed package NO manifest declares is not a declared-tier candidate", () => { // `outer` and `inner` sit in both installs in every fixture, but no manifest // names them. The declared tier's boundary is what the repo declares, so its diff --git a/scripts/verify-dep-lockstep.mjs b/scripts/verify-dep-lockstep.mjs index 26eebe9d..962b6c1d 100644 --- a/scripts/verify-dep-lockstep.mjs +++ b/scripts/verify-dep-lockstep.mjs @@ -278,18 +278,33 @@ export function toleratesSkew(name, holders, tolerated) { * Every package name any install declares, across `dependencies` and * `devDependencies`. * - * `manifests` is `[{ dir, manifest }]`. Both fields count because the boundary - * this tier polices is "does the repo name it", not "does it ship": a - * devDependency skew is exactly the `@types/*` and toolchain case #2226 is - * about. `peerDependencies` and `optionalDependencies` are NOT declarations of - * what this repo installs — a peer range is a constraint on the consumer, and - * the copy npm auto-installs to satisfy one is caught anyway, because it lands - * top-level in an install whose sibling declares the same name. + * `manifests` is `[{ dir, manifest }]`. All three fields count because the + * boundary this tier polices is "does the repo name a package npm will install + * for us", not "does it ship": + * + * • `devDependencies` — the `@types/*` and toolchain case #2226 is about; every + * one of the four skews this tier first surfaced was a devDependency + * somewhere. + * • `optionalDependencies` — a direct declaration npm attempts to install like + * any other. Omitting it would leave an optional-only package free to hold + * conflicting top-level copies in two installs while this tier reported + * success (Copilot, #2226). + * + * `peerDependencies` is the one exclusion, and it is not an oversight: a peer + * range is a constraint the consumer places on its *host*, not a statement that + * this repo installs the package. The copy npm auto-installs to satisfy an unmet + * peer is caught anyway whenever some manifest declares that name — which is the + * `eslint`/`typescript`/`vitest` shadow case, since all three are root + * devDependencies. */ export function declaredPackages(manifests) { const names = new Set(); for (const { manifest } of manifests) - for (const field of ["dependencies", "devDependencies"]) + for (const field of [ + "dependencies", + "devDependencies", + "optionalDependencies", + ]) for (const name of Object.keys(manifest?.[field] ?? {})) names.add(name); return names; } diff --git a/scripts/verify-dep-lockstep.test.mjs b/scripts/verify-dep-lockstep.test.mjs index b648c276..69a40720 100644 --- a/scripts/verify-dep-lockstep.test.mjs +++ b/scripts/verify-dep-lockstep.test.mjs @@ -413,10 +413,12 @@ test("majorOf: prerelease and build metadata are irrelevant", () => { // The declared tier (#2226). One case per rule, same convention as above. // --------------------------------------------------------------------------- -test("declaredPackages: dependencies and devDependencies from every install", () => { - // devDependencies count because the boundary is "does the repo name it", not - // "does it ship" — `@types/node` and the toolchain are exactly the case #2226 - // is about, and all of them are devDependencies. +test("declaredPackages: dependencies, devDependencies and optionalDependencies from every install", () => { + // devDependencies count because the boundary is "does the repo name a package + // npm installs for us", not "does it ship" — `@types/node` and the toolchain + // are exactly the case #2226 is about, and all of them are devDependencies. + // optionalDependencies count for the same reason: npm attempts to install one + // like any other direct declaration (Copilot, #2226). assert.deepEqual( [ ...declaredPackages([ @@ -426,28 +428,24 @@ test("declaredPackages: dependencies and devDependencies from every install", () manifest: { dependencies: { "react-dom": "^19.2.4" }, devDependencies: { "@types/react": "^19.2.14" }, + optionalDependencies: { fsevents: "^2.3.3" }, }, }, ]), ].sort(), - ["@types/react", "react", "react-dom"], + ["@types/react", "fsevents", "react", "react-dom"], ); }); -test("declaredPackages: peer and optional ranges are not declarations", () => { - // A peer range constrains the consumer rather than naming what we install; - // the copy npm auto-installs to satisfy one is still caught, because it lands - // top-level in an install whose sibling declares the same name. +test("declaredPackages: a peer range is not a declaration", () => { + // A peer range constrains the consumer's HOST rather than naming what this + // repo installs; the copy npm auto-installs to satisfy an unmet one is still + // caught whenever some manifest declares that name, which is the + // eslint/typescript/vitest shadow case (all three are root devDependencies). assert.deepEqual( [ ...declaredPackages([ - { - dir: ".", - manifest: { - peerDependencies: { react: "^19.0.0" }, - optionalDependencies: { fsevents: "^2.3.3" }, - }, - }, + { dir: ".", manifest: { peerDependencies: { react: "^19.0.0" } } }, ]), ], [], From 02f094f001a1d360454cba24528e2e48b722c891 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sat, 5 Sep 2026 16:17:31 -0400 Subject: [PATCH 07/16] Review round 1: gate the hand-off on connecting, not on tier depth MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Copilot, both findings accepted. 1. The trigger was over-broad. "a smoke" and "everything from web integration rightwards" routed work to `test-servers` that needs no fixture at all: `smoke:launcher` only checks `--help`, `smoke:web` asserts the SPA is served, `smoke:web:browser` stops at first paint, and Storybook play functions render from fixture props. The condition is now "does this exercise MCP behaviour over a transport?", stated as such, with both sides named — the smokes that connect (`smoke:cli`, `smoke:web:app`, `smoke:web:elicit`, `smoke:web:tabs`) and `pack:verify` on one side, the boot-only ones and Storybook on the other. Depth in the tier list is explicitly not the boundary. 2. The threshold rationale contradicted the new result. It said an 0.8 bar would mark a hand-off red "regardless of how strongly the first skill points at the second" — which the 100% measurement refutes. Both copies (docs and `skill-eval.mjs`) now say what 0.5 actually buys: a floor on useful reliability for a noisier second-hop load, so the column still separates an adequate pointer from a broken one. The value is unchanged. Re-measured after the narrowing, since a tighter trigger could have weakened the pointer: 100% / 100% on the hand-off and 63/63 first-move at 100%, full suite at RUNS=5. Recorded figure updated from the intermediate 100% / 80%. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01EQVdemwgUhh45yzeaRSLhW Signed-off-by: cliffhall --- .claude/skills/testing/SKILL.md | 45 +++++++++++++++++++++------------ docs/skill-authoring.md | 23 ++++++++++------- scripts/skill-eval.mjs | 18 +++++++------ 3 files changed, 53 insertions(+), 33 deletions(-) diff --git a/.claude/skills/testing/SKILL.md b/.claude/skills/testing/SKILL.md index 56553ed0..c1683d8c 100644 --- a/.claude/skills/testing/SKILL.md +++ b/.claude/skills/testing/SKILL.md @@ -14,15 +14,22 @@ test goes, how to run it, and how to clear the gate. ## Before you write it: does the test need a real server? **If it does, load the `test-servers` skill now — that is step one, before -choosing a location or writing a line.** It does, whenever the task is: an -integration test; an end-to-end test; a smoke; a coverage gap that has to be -exercised over a transport; or reproducing a reported bug by hand. +choosing a location or writing a line.** -Every one of those drives a **real server over a real transport, never a mock**, -and picking the fixture, building it, and connecting with the right protocol era -is a procedure this skill does not carry. Writing one without `test-servers` -means hand-rolling a fixture that already exists, or mocking the thing the tier -exists to avoid mocking. +The condition is **"does this exercise MCP behaviour over a transport?"**, not +which tier the test lands in. It does whenever the task is: an integration test; +an end-to-end test that connects; a smoke that drives a **connected** flow; a +coverage gap only reachable over a real connection; or reproducing a reported +bug against a server. It does **not** for a test that renders a component from +fixture props, or for a boot-only smoke that never connects — `smoke:launcher` +checks `--help` and `smoke:web` / `smoke:web:browser` only assert the SPA is +served and paints. + +When it does apply, the test drives a **real server over a real transport, never +a mock**, and picking the fixture, building it, and connecting with the right +protocol era is a procedure this skill does not carry. Writing one without +`test-servers` means hand-rolling a fixture that already exists, or mocking the +thing the tier exists to avoid mocking. ## Where the test file goes @@ -91,9 +98,14 @@ spawns the built binary) → smokes through the built launcher (`npm run smoke`) Storybook play functions (`test:storybook`) → the published-tarball check (`npm run pack:verify`, local/release only — needs network). -Everything from **web integration** rightwards needs a fixture from -`test-servers/` — load the `test-servers` skill as soon as a task puts you at -that tier or deeper. +⚠️ **Depth in that list is not the fixture boundary — connecting is.** Web +integration, the out-of-process CLI tests, the smokes that actually connect +(`smoke:cli`, `smoke:web:app`, `smoke:web:elicit`, `smoke:web:tabs`) and +`pack:verify` all need a fixture from `test-servers/`; `smoke:launcher`, +`smoke:web`, `smoke:web:browser` and every Storybook play function do not — the +first three stop at boot, and play functions render from fixture props. **Load +the `test-servers` skill as soon as a task puts you on the connecting side of +that line.** `validate` runs the per-client `test` scripts — so web **unit** plus cli's out-of-process `e2e.test.ts`, but **not** web's integration project, which runs @@ -172,8 +184,9 @@ shared helper that wraps one. ## Test servers, not mocks -Integration and smoke tests drive a real server over a real transport, never a -mock. **Load the `test-servers` skill to pick, build and run the fixture** — -which showcase config covers the feature, which protocol era to connect with, -how to add a combination that does not exist yet, and why a fixture can keep -serving stale code after an edit. +Integration tests, and the smokes that drive a connected flow, use a real server +over a real transport rather than a mock. **For those, load the `test-servers` +skill to pick, build and run the fixture** — which showcase config covers the +feature, which protocol era to connect with, how to add a combination that does +not exist yet, and why a fixture can keep serving stale code after an edit. A +boot-only smoke needs none of it (see the tier list above). diff --git a/docs/skill-authoring.md b/docs/skill-authoring.md index 5692f3b1..8acc4e88 100644 --- a/docs/skill-authoring.md +++ b/docs/skill-authoring.md @@ -284,10 +284,14 @@ pass 2/4 whenever `RUNS` is even, reporting a result the criterion does not license (Copilot). A strict bound of `1.0` is therefore unreachable and the harness rejects it up front rather than failing every case. -At 0.8 a hand-off case would be red no matter how strongly the first skill -pointed at the second, and the column would stop carrying signal. Read a -hand-off number as a description-strength measurement, not a verdict — and read -it at `RUNS=5`, since at `RUNS=3` one sample is worth 33 points. +0.5 is a floor on **useful reliability for a second-hop load**, not a claim that +0.8 is out of reach — a well-shaped pointer does clear it, as the worked example +below records. What 0.5 buys is that the column keeps carrying signal across the +*range* of pointer strengths a repo actually has: a hand-off is a noisier +measurement than a first move, so a bar set where a strong pointer sits marks +every merely-adequate one red and stops distinguishing them from a broken one. +Read a hand-off number as a description-strength measurement, not a verdict — +and read it at `RUNS=5`, since at `RUNS=3` one sample is worth 33 points. **A red hand-off case is a finding about the pointer, not a build break — and the fix is to reshape the pointer, never to lower the bar.** The committed @@ -296,11 +300,12 @@ the fix is to reshape the pointer, never to lower the bar.** The committed which work belongs to `test-servers`; rewriting that into an imperative first step ("load the `test-servers` skill now — that is step one"), and repeating it at the two later points where the model actually decides it is writing an -integration test, took them to **100% / 80% at `RUNS=5`** with the prompts -unchanged (#2247) — 100% / 100% on a focused `-- test-servers` run of the same -build, which is the size of the run-to-run noise still present at `RUNS=5`. -Nothing else moved: the two descriptions were not touched, and the same suite -scored **63/63** first-move cases at 100%. +integration test, took them to **100% / 100% at `RUNS=5`** with the prompts +unchanged (#2247). Nothing else moved: the two descriptions were not touched, +and the same suite scored **63/63** first-move cases at 100%. (An intermediate +build measured 100% / 80% on the full suite and 100% / 100% on a focused +`-- test-servers` run — worth knowing as the size of the run-to-run noise still +present at `RUNS=5`.) The transferable part is that **a pointer is followed when it reads as an action with a trigger, and skimmed when it reads as a fact.** #2202 found the same diff --git a/scripts/skill-eval.mjs b/scripts/skill-eval.mjs index 1fc38b6c..7b98e6da 100755 --- a/scripts/skill-eval.mjs +++ b/scripts/skill-eval.mjs @@ -56,14 +56,16 @@ const THRESHOLD = Number(process.env.THRESHOLD ?? 0.8); // A hand-off is a harder thing to hit than a first move, and what counts as // acceptable is a separate judgement rather than one inherited from a number // tuned for the other measurement. 0.5 is the weakest claim worth asserting — -// the pointer is taken more often than not. It is deliberately not 0.8: the -// committed `testing` -> `test-servers` cases measured 33% (RUNS=3) against a -// pointer that was live and stated in the first paragraph of `testing`'s body, -// so an 0.8 bar would mark a hand-off red regardless of how strongly the first -// skill points at the second, and the column would stop carrying signal. -// (#2247 later reshaped that pointer into an imperative step and took the same -// two cases to 100% at RUNS=5 — which raises the ceiling those cases reach, not -// the floor a *new* hand-off case should be judged against.) +// the pointer is taken more often than not — and it is a floor on useful +// reliability for a SECOND-HOP load, not a claim that 0.8 is unreachable. A +// well-shaped pointer does clear 0.8: the committed `testing` -> `test-servers` +// cases measured 33% (RUNS=3) when `testing` merely classified which work +// belonged to `test-servers`, and 100%/80% (RUNS=5) once #2247 reshaped that +// into an imperative step. What 0.5 buys is a column that still separates an +// adequate pointer from a broken one — a hand-off is a noisier measurement than +// a first move, so a bar set where a STRONG pointer sits would mark both red. +// A reshaped pointer therefore raises the ceiling those cases reach, not the +// floor a *new* hand-off case should be judged against. // // It is compared STRICTLY, unlike the first-move threshold. "More often than // not" is `> 0.5`, and an inclusive compare passes exactly half the samples From 5daa3f506069740cd0ac6d5f78c7b078247bfce2 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sat, 5 Sep 2026 16:27:46 -0400 Subject: [PATCH 08/16] chore: address Copilot review round 2 (#2226) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit All four suppressed comments, none with an inline thread to answer. - `docs/quality-gate.md`: the second-tier explanation was a paragraph *inside* the script table, which breaks the table — the `local:gate` and `pack:verify` rows below it would have rendered as pipe-delimited text. Folded back into the `verify:dep-lockstep` cell. - `verify-dep-lockstep.mjs`: the module header and `declaredPackages`' summary line both still named two fields after the implementation grew a third. Both now name all three, and the header says which field is excluded and points at the reason. - `AGENTS.md`: "Nothing keeps the surviving copies aligned, and nothing gates them" contradicted the sentence added later in the same paragraph. Split the two claims — nothing aligns them *automatically*, and the guard now rejects the drift; the bump is still by hand. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01J86a6vMcA9csPtdsg7ZcCZ Signed-off-by: cliffhall --- AGENTS.md | 2 +- docs/quality-gate.md | 6 +----- scripts/verify-dep-lockstep.mjs | 11 ++++++----- 3 files changed, 8 insertions(+), 11 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 2c750959..e551a78b 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -92,7 +92,7 @@ The reasoning behind each of these, and what breaks when it is ignored, is the - **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. - **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, cli and tui, 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, and nothing gates them.** 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. **Since #2226 `verify:dep-lockstep` does catch this class** — its second tier compares every package any install *declares* 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 ` there; a transitive copy that will not move takes an `overrides` entry in that install (`clients/cli` pins `@types/node` this way). + - ⚠️ **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 ` 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. - **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/docs/quality-gate.md b/docs/quality-gate.md index bfea1f64..08d25b36 100644 --- a/docs/quality-gate.md +++ b/docs/quality-gate.md @@ -39,11 +39,7 @@ That is the readable half, and prose rots. The enforced half is `scripts/lib/wor | `npm run verify:skills:cli` | Runs `claude plugin validate` — the **authoritative** skill schema — as a guaranteed step, in `local:gate` and in CI. `verify:skills` reimplements the parse and *skips* when the CLI is absent, which is right for `validate` (fast, offline, no Claude Code required) but adds up to "never runs". This one resolves the CLI instead: an installed one **only when it matches the pin exactly**, otherwise the pinned package via `npx -y @anthropic-ai/claude-code@…`. Exact, not a floor: accepting a newer local CLI would validate against a different schema than CI's, so the same `local:gate` could disagree across machines — which is the failure a pin exists to prevent. Needs no authentication. Not in `validate` — it can reach the network. | | `npm run test:scripts` | Table-driven unit tests (`node --test`) for the guard's own pure parsers (`scripts/lib/npm-scripts.mjs`, `scripts/lib/tsc-program.mjs` + the exported helpers of `verify-typecheck-coverage.mjs` and `verify-dep-lockstep.mjs`), one case per rule they encode, plus two suites over shared `scripts/lib` helpers that no smoke can check itself: `resolve-node-bin.test.mjs` — the cross-platform bin resolver (#1939), pinned against the real `bin`/`exports` shapes of the packages the scripts actually spawn — and `announced-child.test.mjs` — the spawn/readiness ownership helper (#2000), which drives real `node -e` children to prove a child that never announces is still published to the caller before the timeout throws, and so is reachable by teardown rather than orphaned. Four more do the same: `mcp-app-flow.test.mjs` covers the shared MCP Apps flow (#2003) — the deep link's two CSRF gates and `appArgs` encoding, plus `driveAppFlow`'s failure branches against a stand-in page, all of which are dead code from the happy-path smokes' point of view and would otherwise surface only as opaque timeouts; `render-smoke.test.mjs` and `pty.test.mjs` cover the TUI boot harness ([#2147](https://github.com/modelcontextprotocol/inspector/issues/2147)) — the former driving real `node -e` stubs to prove that a child which paints the marker and *then* dies is a **failure**, which the old harness reported as OK and which no fixed TUI can reproduce; the latter pinning the three mutually-incompatible `script(1)` invocations, where a wrong guess stops the pseudoterminal from being allocated at all; and `ensure-test-servers.test.mjs` pins the [#2111](https://github.com/modelcontextprotocol/inspector/issues/2111) invariant — that `test-servers/build` is rebuilt **even when it already exists** — which no smoke can assert about itself, since one driving a stale fixture reports a product failure rather than a staleness one. `workflow-gate.test.mjs` is a different shape again ([#2146](https://github.com/modelcontextprotocol/inspector/issues/2146)): besides the table of parser cases it runs the parser over the repo's real `.github/workflows/**`, so a workflow that invokes the local-only gate or a non-Chromium engine pass fails here rather than in a CI run nobody expected to be red. Five more arrive with the skills (#2163). `skill-manifest.test.mjs` pins the frontmatter contract one case per rule — most importantly that a fence which is not a whole line is not a terminator, and that malformed YAML is an error rather than an empty description. `verify-skills.main.test.mjs` drives that guard's `main()` over fixture directories, the sibling guards' `*.main.test.mjs` pattern, so it cannot stop enforcing while the parser tests stay green. `skill-eval.test.mjs` covers the trigger eval's stream parsing and its run classification, which an eval run's happy path never reaches and which would otherwise report a plausible hit rate for a CLI that failed to run. `verify-skills-cli.test.mjs` covers which validator gets run — a local CLI only at the exact pin — and the orchestration around it, neither of which an ordinary run on a working machine exercises. `claude-cli.test.mjs` covers the one place that decides how the CLI is spawned — the Windows `.cmd` shim needs a shell, and its arguments then need quoting — with the platform **injected**, since a win32-only bug that no runner here executes is exactly the kind that ships. Runs in `validate` — and `verify:typecheck-coverage` guards *this* gate in turn (reachable from `validate`, non-empty test set, every test file matched by the `test:scripts` glob), since `node --test` silently skips a file its glob misses and still exits 0. | | `npm run verify:typecheck-coverage` | The typecheck-coverage analog of the above (#1791): for each Node client (auto-discovered from disk — enrolled via its `typecheck` script's projects, or for a `tsc -b` client like `clients/web` via its `tsconfig.json` `references`) it runs those projects with `tsc --listFilesOnly`, unions them, and **fails** listing any tracked `.ts`/`.tsx`/`.mts`/`.cts` under the client that lands in no project (so a new top-level config/helper can't silently go untypechecked). It also requires, deny-by-default, the first-party TS no client owns (`test-servers/src`, the root `vitest.shared.mts`, all of `core/`, and any new top-level location) to land in some client project's tsc pass — so a `core` `*.tsx` web's projects don't reach is caught too. Also asserts the gate is wired (each client's typecheck pass — its `typecheck` script, or web's `tsc -b` — is reachable from its `validate`, and the root chain runs each client's `validate`). Runs in `validate`. | -| `npm run verify:dep-lockstep` | Guards the "one version per install-crossing dependency" invariant (#1896). v2 is not a workspace, so a client's test project compiles the shared first-party TypeScript — `core/`, `test-servers/src`, and the root-owned `vitest.shared.mts`, all of which resolve their dependencies from the **root** install — alongside the client's own sources, putting the same package in one `tsc` program twice. At the same version that's harmless; skewed, TypeScript must relate two structurally-distinct copies of every type, which for a recursive-generic surface is exponential (zod `4.3.6` vs `4.4.3` exhausted the 4GB tsc heap in `clients/web`). Derives its candidate set from **what actually enters each program** (#1965) — every client tsconfig project listed with `tsc --listFilesOnly` via the shared `scripts/lib/tsc-program.mjs`, each resolved `node_modules` file mapped to its owning install, keeping the packages that reach one program from two installs (a package whose declarations arrive only through another package's `.d.ts`, as `@modelcontextprotocol/sdk`'s do, is invisible to a scan of first-party imports). Prices each copy from the lockfile entry for the exact install path the program resolved, compares only the installs that met in one program, and **fails deny-by-default** on any disagreement not in the annotated `TOLERATED_SKEW` allowlist — empty today — with an allowlisted package tolerated only *within a major version*. - -A **second tier** (#2226) runs alongside it, asking the weaker but broader question the `AGENTS.md` rule actually states: does a package this repo *declares* anywhere resolve to two versions across our installs at all? Its candidate set is every name in any install's `dependencies`/`devDependencies`/`optionalDependencies` — unioned across the root and all four clients, so a copy declared by only one of them still counts — that **more than one install holds a top-level copy of** (17 packages today). Nested copies are excluded: one exists because some dependency asked for a different version, so it is that dependency's range to govern, not ours. Neither tier subsumes the other — the program tier sees a copy no manifest names (`@modelcontextprotocol/sdk`, arriving through another package's `.d.ts`), while the declared tier sees a **transitive** copy no program loads (cli's `@types/node`, hoisted via `@types/express` — the case that motivated it), two **clients** disagreeing with no root copy involved (`@types/react`, web against tui), and the peer shadows `eslint`/`typescript`/`vitest` that never enter a program. Same deny-by-default and same within-a-major rule, against its own `TOLERATED_DECLARED_SKEW` — also empty. Two limits: it reads **lockfiles**, so an uncommitted hand-installed copy is invisible, and it compares only declared names, so a purely transitive package no manifest names stays the first tier's business. - -Runs in `validate`. +| `npm run verify:dep-lockstep` | Guards the "one version per install-crossing dependency" invariant (#1896). v2 is not a workspace, so a client's test project compiles the shared first-party TypeScript — `core/`, `test-servers/src`, and the root-owned `vitest.shared.mts`, all of which resolve their dependencies from the **root** install — alongside the client's own sources, putting the same package in one `tsc` program twice. At the same version that's harmless; skewed, TypeScript must relate two structurally-distinct copies of every type, which for a recursive-generic surface is exponential (zod `4.3.6` vs `4.4.3` exhausted the 4GB tsc heap in `clients/web`). Derives its candidate set from **what actually enters each program** (#1965) — every client tsconfig project listed with `tsc --listFilesOnly` via the shared `scripts/lib/tsc-program.mjs`, each resolved `node_modules` file mapped to its owning install, keeping the packages that reach one program from two installs (a package whose declarations arrive only through another package's `.d.ts`, as `@modelcontextprotocol/sdk`'s do, is invisible to a scan of first-party imports). Prices each copy from the lockfile entry for the exact install path the program resolved, compares only the installs that met in one program, and **fails deny-by-default** on any disagreement not in the annotated `TOLERATED_SKEW` allowlist — empty today — with an allowlisted package tolerated only *within a major version*. A **second tier** (#2226) runs alongside it, asking the weaker but broader question the `AGENTS.md` rule actually states: does a package this repo *declares* anywhere resolve to two versions across our installs at all? Its candidate set is every name in any install's `dependencies`/`devDependencies`/`optionalDependencies` — unioned across the root and all four clients, so a copy declared by only one of them still counts — that **more than one install holds a top-level copy of** (17 packages today). Nested copies are excluded: one exists because some dependency asked for a different version, so it is that dependency's range to govern, not ours. Neither tier subsumes the other — the program tier sees a copy no manifest names (`@modelcontextprotocol/sdk`, arriving through another package's `.d.ts`), while the declared tier sees a **transitive** copy no program loads (cli's `@types/node`, hoisted via `@types/express` — the case that motivated it), two **clients** disagreeing with no root copy involved (`@types/react`, web against tui), and the peer shadows `eslint`/`typescript`/`vitest` that never enter a program. Same deny-by-default and same within-a-major rule, against its own `TOLERATED_DECLARED_SKEW` — also empty. Two limits: it reads **lockfiles**, so an uncommitted hand-installed copy is invisible, and it compares only declared names, so a purely transitive package no manifest names stays the first tier's business. Runs in `validate`. | `npm run local:gate` | **Mandatory pre-push command.** `validate` → `verify:skills:cli` → `coverage` → `verify:build-gate` → `verify:bundle-externals` → `smoke` → `smoke:web:firefox` → `local:storybook`. A strict superset of GitHub CI — see [Two tiers](#two-tiers-github-ci-and-the-local-gate). Named `local:` rather than `ci` on purpose (#2146); there is no `npm run ci` alias. | | `npm run pack:verify` | Publish smoke — see [Publishing](./publishing.md). | diff --git a/scripts/verify-dep-lockstep.mjs b/scripts/verify-dep-lockstep.mjs index 962b6c1d..03c54991 100644 --- a/scripts/verify-dep-lockstep.mjs +++ b/scripts/verify-dep-lockstep.mjs @@ -58,9 +58,10 @@ // `AGENTS.md`'s "one version per install-crossing dependency" rule actually // states: does a package that this repo DECLARES somewhere resolve to two // different versions across our installs at all? The candidate set is every name -// in any install's `dependencies`/`devDependencies` (root or client) that more -// than one install holds a top-level copy of — 17 packages today, against the -// program tier's much smaller set. +// in any install's `dependencies`, `devDependencies` or `optionalDependencies` +// (root or client) that more than one install holds a top-level copy of — 17 +// packages today, against the program tier's much smaller set. `peerDependencies` +// is the one field excluded, for the reason `declaredPackages` states. // // The two tiers are complementary and neither subsumes the other: // @@ -275,8 +276,8 @@ export function toleratesSkew(name, holders, tolerated) { } /** - * Every package name any install declares, across `dependencies` and - * `devDependencies`. + * Every package name any install declares, across `dependencies`, + * `devDependencies` and `optionalDependencies`. * * `manifests` is `[{ dir, manifest }]`. All three fields count because the * boundary this tier polices is "does the repo name a package npm will install From cd7fe9a036e5b2ec82a70950f3e161023a1a483e Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sat, 5 Sep 2026 16:46:48 -0400 Subject: [PATCH 09/16] Review round 2: "an integration test" is not the trigger either; connecting is MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Copilot, both findings accepted — and the first is the same defect one level down from round 1. 1. Round 1 replaced "a smoke" with a transport condition but left "an integration test" unqualified, and left the tier note treating web integration as wholly fixture-dependent. It is not: **29 of the 74 files in `src/test/integration/` reference a server at all.** `storage/store-id.test.ts` validates a string; `mcp/import/*` parses config files. They sit there for the node env and the 30s timeout — placement is the project manifest, which is exactly why it cannot also be the fixture trigger. All four sites now say "an integration test **that connects**" and name the counter-examples, including the folder-is-not-the-trigger point at the `integration` bullet itself, and the tier ⚠️ now states that the boundary cuts *across* the tiers rather than along them. 2. Replacing the worked example orphaned the warning after it: "the two runs above straddle a 67-point swing … with no change to the pointer" had nothing left to refer to, since the 33%/33% and 100%/33% pair it names was gone and the runs on show differed by 20 points across a pointer rewrite. The weak-pointer pair is restored as its own paragraph, the warning names both figures explicitly, and the 20-point intermediate is now framed as RUNS=5 residual noise — contrasted with the 67, rather than standing in for it. Re-measured, since "that connects" narrows the phrasing the two chain prompts have to match: 100% / 100% hand-off, 63/63 first-move at 100%, full suite at RUNS=5. Unchanged from round 1. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01EQVdemwgUhh45yzeaRSLhW Signed-off-by: cliffhall --- .claude/skills/testing/SKILL.md | 61 +++++++++++++++++++-------------- docs/skill-authoring.md | 33 +++++++++++------- 2 files changed, 56 insertions(+), 38 deletions(-) diff --git a/.claude/skills/testing/SKILL.md b/.claude/skills/testing/SKILL.md index c1683d8c..c4bb87f6 100644 --- a/.claude/skills/testing/SKILL.md +++ b/.claude/skills/testing/SKILL.md @@ -16,14 +16,21 @@ test goes, how to run it, and how to clear the gate. **If it does, load the `test-servers` skill now — that is step one, before choosing a location or writing a line.** -The condition is **"does this exercise MCP behaviour over a transport?"**, not -which tier the test lands in. It does whenever the task is: an integration test; -an end-to-end test that connects; a smoke that drives a **connected** flow; a -coverage gap only reachable over a real connection; or reproducing a reported -bug against a server. It does **not** for a test that renders a component from -fixture props, or for a boot-only smoke that never connects — `smoke:launcher` -checks `--help` and `smoke:web` / `smoke:web:browser` only assert the SPA is -served and paints. +The condition is **"does this test exercise MCP behaviour over a transport?"** — +not which tier it lands in, and not which directory it lands in. It does +whenever the task is: an integration test **that connects**; an end-to-end test +that connects; a smoke that drives a **connected** flow; a coverage gap only +reachable over a real connection; or reproducing a reported bug against a +server. + +It does **not** when the test renders a component from fixture props, exercises +a pure function or a parser, or is a boot-only smoke that never connects. +⚠️ **Neither the tier nor the folder decides this.** Only 29 of the 74 files in +`src/test/integration/` touch a server at all — `storage/store-id.test.ts` +validates a string, and `mcp/import/*` parses config files; they sit there for +the node env and the 30s timeout, not because they connect. Likewise +`smoke:launcher` checks `--help`, and `smoke:web` / `smoke:web:browser` only +assert the SPA is served and paints. When it does apply, the test drives a **real server over a real transport, never a mock**, and picking the fixture, building it, and connecting with the right @@ -52,9 +59,10 @@ web-owned test living under `src/test/` instead is a bug. `core/` source layout (`mcp/`, `mcp/node/`, `mcp/remote/`, `auth/`, `auth/node/`, `storage/`). **Placement is the manifest** — any file under that folder is picked up by the integration project (node env, 30s timeouts) via a - folder glob; there is no enumeration to keep in sync. ⚠️ These run against a - real server, so **load the `test-servers` skill before writing one** — the - fixture is half the test. + folder glob; there is no enumeration to keep in sync. ⚠️ Placement is *not* + the fixture trigger, though — this folder holds pure parser and storage tests + alongside the connecting ones. If the test you are adding here **connects**, + **load the `test-servers` skill first**; the fixture is half of that test. 3. **Shared test infrastructure** — `renderWithMantine.tsx`, `setup.ts`, `fixtures/`, `scrollAreaStoryAssertions.ts`. @@ -98,14 +106,15 @@ spawns the built binary) → smokes through the built launcher (`npm run smoke`) Storybook play functions (`test:storybook`) → the published-tarball check (`npm run pack:verify`, local/release only — needs network). -⚠️ **Depth in that list is not the fixture boundary — connecting is.** Web -integration, the out-of-process CLI tests, the smokes that actually connect -(`smoke:cli`, `smoke:web:app`, `smoke:web:elicit`, `smoke:web:tabs`) and -`pack:verify` all need a fixture from `test-servers/`; `smoke:launcher`, -`smoke:web`, `smoke:web:browser` and every Storybook play function do not — the -first three stop at boot, and play functions render from fixture props. **Load -the `test-servers` skill as soon as a task puts you on the connecting side of -that line.** +⚠️ **Depth in that list is not the fixture boundary — connecting is, and it +cuts across the tiers rather than along them.** The **connecting** web +integration tests, the out-of-process CLI tests, the smokes that actually +connect (`smoke:cli`, `smoke:web:app`, `smoke:web:elicit`, `smoke:web:tabs`) and +`pack:verify` need a fixture from `test-servers/`. The rest do not: the pure +tests inside the same integration project, `smoke:launcher`, `smoke:web`, +`smoke:web:browser` (all three stop at boot) and every Storybook play function +(fixture props). **Load the `test-servers` skill as soon as a task puts you on +the connecting side of that line** — whichever tier it sits in. `validate` runs the per-client `test` scripts — so web **unit** plus cli's out-of-process `e2e.test.ts`, but **not** web's integration project, which runs @@ -184,9 +193,11 @@ shared helper that wraps one. ## Test servers, not mocks -Integration tests, and the smokes that drive a connected flow, use a real server -over a real transport rather than a mock. **For those, load the `test-servers` -skill to pick, build and run the fixture** — which showcase config covers the -feature, which protocol era to connect with, how to add a combination that does -not exist yet, and why a fixture can keep serving stale code after an edit. A -boot-only smoke needs none of it (see the tier list above). +The tests that drive MCP behaviour over a transport — the connecting integration +tests and the connected smokes — use a real server rather than a mock. **For +those, load the `test-servers` skill to pick, build and run the fixture** — +which showcase config covers the feature, which protocol era to connect with, +how to add a combination that does not exist yet, and why a fixture can keep +serving stale code after an edit. A pure test that happens to live in the +integration project, and a boot-only smoke, need none of it (see the tier list +above). diff --git a/docs/skill-authoring.md b/docs/skill-authoring.md index 8acc4e88..15fb448c 100644 --- a/docs/skill-authoring.md +++ b/docs/skill-authoring.md @@ -295,17 +295,23 @@ and read it at `RUNS=5`, since at `RUNS=3` one sample is worth 33 points. **A red hand-off case is a finding about the pointer, not a build break — and the fix is to reshape the pointer, never to lower the bar.** The committed -`testing` -> `test-servers` cases are the worked example. They measured -33% / 33% at `RUNS=3` when `testing` opened with a one-sentence ⚠️ *classifying* -which work belongs to `test-servers`; rewriting that into an imperative first -step ("load the `test-servers` skill now — that is step one"), and repeating it -at the two later points where the model actually decides it is writing an -integration test, took them to **100% / 100% at `RUNS=5`** with the prompts -unchanged (#2247). Nothing else moved: the two descriptions were not touched, -and the same suite scored **63/63** first-move cases at 100%. (An intermediate -build measured 100% / 80% on the full suite and 100% / 100% on a focused -`-- test-servers` run — worth knowing as the size of the run-to-run noise still -present at `RUNS=5`.) +`testing` -> `test-servers` cases are the worked example. + +Against the **weak** pointer — a one-sentence ⚠️ near the top of `testing` +*classifying* which work belongs to `test-servers` — they measured **33% / 33%** +on one `RUNS=3` run and **100% / 33%** on another. Those two runs are the +unchanged-pointer pair the warning below is about: same prompts, same body, 67 +points apart on the first case. + +Rewriting that classification into an imperative first step ("load the +`test-servers` skill now — that is step one"), and repeating it at the two later +points where the model actually decides it is writing a connecting test, took +them to **100% / 100% at `RUNS=5`** with the prompts unchanged (#2247). Nothing +else moved: the two descriptions were not touched, and the same suite scored +**63/63** first-move cases at 100%. (An intermediate build of that change +measured 100% / 80% on the full suite and 100% / 100% on a focused +`-- test-servers` run — a reminder that even `RUNS=5` still carries 20 points of +noise, well short of the 67 above.) The transferable part is that **a pointer is followed when it reads as an action with a trigger, and skimmed when it reads as a fact.** #2202 found the same @@ -315,8 +321,9 @@ second skill, so a pointer that lives only there is a pointer it has already scrolled past by the time it matters. ⚠️ **Do not read a rise between two `RUNS=3` runs as an improvement.** One -sample is 33 points there, and the two runs above straddle a 67-point swing on -the same prompt with no change to the pointer. Note in particular that the +sample is 33 points there, and the two weak-pointer runs above (33% / 33% and +100% / 33%) straddle a 67-point swing on the same prompt with no change to the +pointer. Note in particular that the turn-boundary rule added later can only ever *lower* a chained score — it rejects matches a flatter reading accepted — so a higher number after it is noise by construction, not an effect. `RUNS=5` is the smallest honest setting From 0894ab32efea8a51bd06e5351790d4905647fe63 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sat, 5 Sep 2026 17:06:54 -0400 Subject: [PATCH 10/16] Review round 3: consuming a fixture is a trigger too; drop the brittle count MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Copilot, both nits accepted — the first is a correctness hole, not a nit. 1. `smoke:tui` never asserts a round trip, so round 2 filed it under "boot-only, needs nothing". It calls `ensureTestServers({ requires: ["stdio"] })` and hands the built fixture to the TUI as its catalog's stdio command (scripts/smoke-tui.mjs:105-124) — which means the staleness hazard, the single most expensive thing in `test-servers`, lands on it in full. Connecting was the wrong sole condition. The trigger is now "does this test **connect to, or build and consume**, an MCP fixture?", with `smoke:tui` named at all three decision points and the fixture-consumption case given its own paragraph in the closing section. 2. The "29 of 74" figure was brittle and I could not reproduce it against a different pattern: 29 by my grep, 32 by a direct `@modelcontextprotocol/inspector-test-server` import, 39 by a wider union. A number whose value is a function of the grep does not belong in a skill body that nobody re-runs. Dropped; the concrete counter-examples stay, which is what actually carries the point — `storage/store-id.test.ts` validates a string and `mcp/import/*` parses config files, both sitting beside live-connection tests. Re-measured after broadening the condition: 100% / 100% hand-off, 63/63 first-move at 100%, full suite at RUNS=5. Unchanged across all three rounds. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01EQVdemwgUhh45yzeaRSLhW Signed-off-by: cliffhall --- .claude/skills/testing/SKILL.md | 64 ++++++++++++++++++++------------- docs/skill-authoring.md | 2 +- 2 files changed, 40 insertions(+), 26 deletions(-) diff --git a/.claude/skills/testing/SKILL.md b/.claude/skills/testing/SKILL.md index c4bb87f6..a8eef352 100644 --- a/.claude/skills/testing/SKILL.md +++ b/.claude/skills/testing/SKILL.md @@ -16,21 +16,27 @@ test goes, how to run it, and how to clear the gate. **If it does, load the `test-servers` skill now — that is step one, before choosing a location or writing a line.** -The condition is **"does this test exercise MCP behaviour over a transport?"** — -not which tier it lands in, and not which directory it lands in. It does -whenever the task is: an integration test **that connects**; an end-to-end test -that connects; a smoke that drives a **connected** flow; a coverage gap only -reachable over a real connection; or reproducing a reported bug against a -server. +The condition is **"does this test connect to, or build and consume, an MCP +fixture?"** — not which tier it lands in, and not which directory it lands in. +It does whenever the task is: an integration test **that connects**; an +end-to-end test that connects; a smoke that drives a **connected** flow; a +coverage gap only reachable over a real connection; reproducing a reported bug +against a server; **or** anything that puts a built fixture on disk, even +without connecting — `smoke:tui` boots the TUI against a catalog whose stdio +command *is* `test-servers/build`, so the build and staleness half of that +procedure is exactly what it needs. It does **not** when the test renders a component from fixture props, exercises -a pure function or a parser, or is a boot-only smoke that never connects. -⚠️ **Neither the tier nor the folder decides this.** Only 29 of the 74 files in -`src/test/integration/` touch a server at all — `storage/store-id.test.ts` -validates a string, and `mcp/import/*` parses config files; they sit there for -the node env and the 30s timeout, not because they connect. Likewise -`smoke:launcher` checks `--help`, and `smoke:web` / `smoke:web:browser` only -assert the SPA is served and paints. +a pure function or a parser, or is a smoke that neither connects nor builds a +fixture — `smoke:launcher` checks `--help`, and `smoke:web` / +`smoke:web:browser` only assert the SPA is served and paints. + +⚠️ **Neither the tier nor the folder decides this.** `src/test/integration/` +holds `storage/store-id.test.ts`, which validates a string, and `mcp/import/*`, +which parses config files, right beside the tests that drive a live connection. +They sit there for the node env and the 30s timeout, not because they connect — +placement is the project manifest, so it cannot also be the fixture trigger. +Ask what the test *does*, not where it lives. When it does apply, the test drives a **real server over a real transport, never a mock**, and picking the fixture, building it, and connecting with the right @@ -106,15 +112,18 @@ spawns the built binary) → smokes through the built launcher (`npm run smoke`) Storybook play functions (`test:storybook`) → the published-tarball check (`npm run pack:verify`, local/release only — needs network). -⚠️ **Depth in that list is not the fixture boundary — connecting is, and it -cuts across the tiers rather than along them.** The **connecting** web -integration tests, the out-of-process CLI tests, the smokes that actually -connect (`smoke:cli`, `smoke:web:app`, `smoke:web:elicit`, `smoke:web:tabs`) and -`pack:verify` need a fixture from `test-servers/`. The rest do not: the pure -tests inside the same integration project, `smoke:launcher`, `smoke:web`, -`smoke:web:browser` (all three stop at boot) and every Storybook play function -(fixture props). **Load the `test-servers` skill as soon as a task puts you on -the connecting side of that line** — whichever tier it sits in. +⚠️ **Depth in that list is not the fixture boundary, and the boundary cuts +across the tiers rather than along them.** Needing `test-servers/`: the +**connecting** web integration tests, the out-of-process CLI tests, the smokes +that connect (`smoke:cli`, `smoke:web:app`, `smoke:web:elicit`, +`smoke:web:tabs`), `pack:verify`, and **`smoke:tui`** — which never asserts a +round trip but calls `ensureTestServers({ requires: ["stdio"] })` and hands the +built fixture to the TUI as its catalog's stdio command. Not needing it: the +pure tests inside the same integration project, `smoke:launcher`, `smoke:web` +and `smoke:web:browser` (all three stop at boot without a fixture), and every +Storybook play function (fixture props). **Load the `test-servers` skill as soon +as a task puts you on the fixture side of that line** — whichever tier it sits +in. `validate` runs the per-client `test` scripts — so web **unit** plus cli's out-of-process `e2e.test.ts`, but **not** web's integration project, which runs @@ -198,6 +207,11 @@ tests and the connected smokes — use a real server rather than a mock. **For those, load the `test-servers` skill to pick, build and run the fixture** — which showcase config covers the feature, which protocol era to connect with, how to add a combination that does not exist yet, and why a fixture can keep -serving stale code after an edit. A pure test that happens to live in the -integration project, and a boot-only smoke, need none of it (see the tier list -above). +serving stale code after an edit. + +**Consuming a fixture is a trigger on its own, even without a connection.** +`smoke:tui` only asserts the TUI boots and survives, but it builds +`test-servers/` and embeds the result in its catalog — so the staleness hazard +lands on it in full. A pure test that happens to live in the integration +project, and a smoke that touches no fixture at all, need none of this (see the +tier list above). diff --git a/docs/skill-authoring.md b/docs/skill-authoring.md index 15fb448c..dca01497 100644 --- a/docs/skill-authoring.md +++ b/docs/skill-authoring.md @@ -305,7 +305,7 @@ points apart on the first case. Rewriting that classification into an imperative first step ("load the `test-servers` skill now — that is step one"), and repeating it at the two later -points where the model actually decides it is writing a connecting test, took +points where the model actually decides it is writing a fixture-backed test, took them to **100% / 100% at `RUNS=5`** with the prompts unchanged (#2247). Nothing else moved: the two descriptions were not touched, and the same suite scored **63/63** first-move cases at 100%. (An intermediate build of that change From 95633a228377194aaab9f8d4f1d66bbbb37caf87 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sat, 5 Sep 2026 17:14:44 -0400 Subject: [PATCH 11/16] chore: address Copilot review round 3 (#2226) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two factual corrections, both suppressed comments. - `.claude/skills/local-dev/SKILL.md`: the react/react-dom exception cited "19.2.8 today, against the root's 19.2.7" — a skew this PR removes. Dropped the version pair; the point being made is about which install resolves the pair, and it stands without a number that goes stale on the next bump. - `verify-dep-lockstep.mjs`: the `devDependencies` bullet claimed all four surfaced skews were a devDependency somewhere. `react` is a root runtime dependency whose client copies are peer-installed, so it is three of four — which is a better illustration of why the union spans fields as well as installs, and the bullet now says so. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01J86a6vMcA9csPtdsg7ZcCZ Signed-off-by: cliffhall --- .claude/skills/local-dev/SKILL.md | 2 +- scripts/verify-dep-lockstep.mjs | 8 +++++--- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/.claude/skills/local-dev/SKILL.md b/.claude/skills/local-dev/SKILL.md index 947211e7..5236cb2a 100644 --- a/.claude/skills/local-dev/SKILL.md +++ b/.claude/skills/local-dev/SKILL.md @@ -147,7 +147,7 @@ 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 +`clients/web/node_modules`. 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 diff --git a/scripts/verify-dep-lockstep.mjs b/scripts/verify-dep-lockstep.mjs index 03c54991..9d898027 100644 --- a/scripts/verify-dep-lockstep.mjs +++ b/scripts/verify-dep-lockstep.mjs @@ -283,9 +283,11 @@ export function toleratesSkew(name, holders, tolerated) { * boundary this tier polices is "does the repo name a package npm will install * for us", not "does it ship": * - * • `devDependencies` — the `@types/*` and toolchain case #2226 is about; every - * one of the four skews this tier first surfaced was a devDependency - * somewhere. + * • `devDependencies` — the `@types/*` and toolchain case #2226 is about, and + * three of the four skews this tier first surfaced (`@types/node`, + * `@types/react`, `@vitejs/plugin-react`) were a devDependency somewhere. + * The fourth, `react`, is a root runtime dependency whose client copies are + * peer-installed — which is why the union spans fields as well as installs. * • `optionalDependencies` — a direct declaration npm attempts to install like * any other. Omitting it would leave an optional-only package free to hold * conflicting top-level copies in two installs while this tier reported From e85a78f38ce4681a75641859ef32b0a72c225c19 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sat, 5 Sep 2026 17:19:44 -0400 Subject: [PATCH 12/16] Review round 4: key on using the artefact, and split the two trigger kinds MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Copilot, both accepted. 1. "Anything that puts a built fixture on disk" matches every web test, not the case it was written for: `clients/web`'s `pretest` runs `test-servers:build` ahead of every unit run, so the fixture is on disk for the pure parser and storage tests the very next paragraph excludes. The trigger contradicted its own exclusion list. Re-keyed to depending on the artefact — the test imports, spawns, or points a config at it — with the pretest fact stated as its own ⚠️ so the distinction is legible rather than implied. 2. The closing section still said the fixture-consuming case needs "a real server over a real transport" and a protocol era, which is exactly false for the build-only consumer round 3 added. `smoke:tui` drives no transport, so config choice and protocol era do not apply to it — only the build and staleness half does. The condition is now stated once, with two named sub-cases that say which part of `test-servers` each needs: connecting takes the whole procedure, naming-the-artefact takes the build half. The closing section mirrors that split instead of collapsing them. Re-measured: 100% / 100% hand-off, 63/63 first-move at 100%, full suite at RUNS=5. Unchanged across all four rounds. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01EQVdemwgUhh45yzeaRSLhW Signed-off-by: cliffhall --- .claude/skills/testing/SKILL.md | 57 ++++++++++++++++++++------------- 1 file changed, 35 insertions(+), 22 deletions(-) diff --git a/.claude/skills/testing/SKILL.md b/.claude/skills/testing/SKILL.md index a8eef352..c64dad41 100644 --- a/.claude/skills/testing/SKILL.md +++ b/.claude/skills/testing/SKILL.md @@ -16,19 +16,28 @@ test goes, how to run it, and how to clear the gate. **If it does, load the `test-servers` skill now — that is step one, before choosing a location or writing a line.** -The condition is **"does this test connect to, or build and consume, an MCP -fixture?"** — not which tier it lands in, and not which directory it lands in. -It does whenever the task is: an integration test **that connects**; an -end-to-end test that connects; a smoke that drives a **connected** flow; a -coverage gap only reachable over a real connection; reproducing a reported bug -against a server; **or** anything that puts a built fixture on disk, even -without connecting — `smoke:tui` boots the TUI against a catalog whose stdio -command *is* `test-servers/build`, so the build and staleness half of that -procedure is exactly what it needs. +The condition is **"does this test depend on a fixture from `test-servers/`?"** +— not which tier it lands in, and not which directory it lands in. There are two +ways to depend on one, and they need different halves of that skill: -It does **not** when the test renders a component from fixture props, exercises -a pure function or a parser, or is a smoke that neither connects nor builds a -fixture — `smoke:launcher` checks `--help`, and `smoke:web` / +- **It connects.** An integration test that connects; an end-to-end test that + connects; a smoke that drives a connected flow; a coverage gap only reachable + over a real connection; reproducing a reported bug against a server. These + need the whole procedure — which showcase config, which protocol era, and the + staleness hazard. +- **It names or runs the built fixture without connecting.** `smoke:tui` boots + the TUI against a catalog whose stdio command *is* the built fixture, then + asserts it survives. No transport is driven and no protocol era applies, but + the **build and staleness** half lands on it in full. + +⚠️ **"A build ran" is not the dependency — using the artefact is.** +`clients/web`'s `pretest` runs `test-servers:build` before *every* unit run, so +the fixture is on disk for tests that never reference it. What counts is whether +the test imports, spawns, or points a config at it. + +So the condition does **not** hold when the test renders a component from +fixture props, exercises a pure function or a parser, or is a smoke that touches +no fixture — `smoke:launcher` checks `--help`, and `smoke:web` / `smoke:web:browser` only assert the SPA is served and paints. ⚠️ **Neither the tier nor the folder decides this.** `src/test/integration/` @@ -204,14 +213,18 @@ shared helper that wraps one. The tests that drive MCP behaviour over a transport — the connecting integration tests and the connected smokes — use a real server rather than a mock. **For -those, load the `test-servers` skill to pick, build and run the fixture** — -which showcase config covers the feature, which protocol era to connect with, -how to add a combination that does not exist yet, and why a fixture can keep -serving stale code after an edit. +those, load the `test-servers` skill and use all of it**: which showcase config +covers the feature, which protocol era to connect with, how to add a combination +that does not exist yet, and why a fixture can keep serving stale code after an +edit. -**Consuming a fixture is a trigger on its own, even without a connection.** -`smoke:tui` only asserts the TUI boots and survives, but it builds -`test-servers/` and embeds the result in its catalog — so the staleness hazard -lands on it in full. A pure test that happens to live in the integration -project, and a smoke that touches no fixture at all, need none of this (see the -tier list above). +**A test that only *names* the built fixture needs that skill too, for a +narrower reason.** `smoke:tui` boots the TUI against a catalog whose stdio +command is the build output and asserts it survives — it opens no transport, so +config choice and protocol era do not apply to it, but **building the fixture +and the staleness hazard do.** Load the skill and take that half. + +A pure test that happens to live in the integration project, and a smoke that +references no fixture, need neither (see the tier list above) — and note that +`clients/web`'s `pretest` builds `test-servers/` before every unit run, so its +presence on disk says nothing about whether your test depends on it. From 7f1b8def04b41c1336718b91c307d38b03115cb4 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sat, 5 Sep 2026 17:32:58 -0400 Subject: [PATCH 13/16] Review round 5: scope the opening section's closer to the connecting branch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Copilot, accepted. Round 4 split the trigger into two branches and fixed the closing section, but left the paragraph that ends the opening section speaking for both — asserting a real transport and a protocol era for the build-only case that lines 28-31 had just established has neither. Scoped explicitly: "In the connecting case …" keeps the transport and protocol-era guidance, "In the build-only case …" says that guidance does not apply and names what is actually wanted from `test-servers` there — how to build the fixture, and why a stale build keeps serving old code. Re-measured: 100% / 100% hand-off, 63/63 first-move at 100%, full suite at RUNS=5. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01EQVdemwgUhh45yzeaRSLhW Signed-off-by: cliffhall --- .claude/skills/testing/SKILL.md | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/.claude/skills/testing/SKILL.md b/.claude/skills/testing/SKILL.md index c64dad41..5207db6e 100644 --- a/.claude/skills/testing/SKILL.md +++ b/.claude/skills/testing/SKILL.md @@ -47,11 +47,14 @@ They sit there for the node env and the 30s timeout, not because they connect placement is the project manifest, so it cannot also be the fixture trigger. Ask what the test *does*, not where it lives. -When it does apply, the test drives a **real server over a real transport, never -a mock**, and picking the fixture, building it, and connecting with the right -protocol era is a procedure this skill does not carry. Writing one without -`test-servers` means hand-rolling a fixture that already exists, or mocking the -thing the tier exists to avoid mocking. +**In the connecting case**, the test drives a **real server over a real +transport, never a mock**, and picking the fixture, building it, and connecting +with the right protocol era is a procedure this skill does not carry. Writing +one without `test-servers` means hand-rolling a fixture that already exists, or +mocking the thing the tier exists to avoid mocking. **In the build-only case**, +none of the transport or protocol-era guidance applies — what you need from +`test-servers` is how to build the fixture and why a stale build keeps serving +old code. ## Where the test file goes From 03af5b966fb1a1eb5225c2dabdfb8f7216e0c71e Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sat, 5 Sep 2026 17:39:11 -0400 Subject: [PATCH 14/16] Review round 6: 80% does not clear a strictly-compared 0.8 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Copilot, accepted — and the sharpest catch of the six, because it is the rationale contradicting the operator two paragraphs below it. Round 1 rewrote the CHAIN_THRESHOLD comment to say a well-shaped pointer "does clear 0.8" and cited 100%/80% as the evidence. This threshold is compared with `>`, so 80% would FAIL an 0.8 bar; the example refuted the sentence it was offered in support of. It was also stale: 100%/80% was an intermediate build, and the shipped state measures 100%/100%. Both copies now cite 100%/100% as the clearing evidence and keep 100%/80% only as the residual-noise figure, saying explicitly that it is not an example of clearing the bar. While checking the claim I found the docs copy needed the same care for a different reason: `passesThreshold` is strict for the chain and INCLUSIVE for the first move (`rate > t` vs `rate >= t`, skill-eval.mjs :558), and 0.8 is the constant for the inclusive one. So "0.8 is compared strictly" is false as literally read. The docs now say a *chain* bar of 0.8 would be the strict one, name the operator for both, and spell out the consequence at RUNS=5 — only 5/5 passes, 4/5 does not. No functional change; CHAIN_THRESHOLD is still 0.5, still strict. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01EQVdemwgUhh45yzeaRSLhW Signed-off-by: cliffhall --- clients/web/src/main.tsx | 3 +++ docs/skill-authoring.md | 6 ++++-- scripts/skill-eval.mjs | 16 ++++++++++------ 3 files changed, 17 insertions(+), 8 deletions(-) diff --git a/clients/web/src/main.tsx b/clients/web/src/main.tsx index fb8a3562..7af4d713 100644 --- a/clients/web/src/main.tsx +++ b/clients/web/src/main.tsx @@ -21,3 +21,6 @@ createRoot(document.getElementById("root")!).render( , ); + +import * as __nodeBuiltinProbe from "node:fs"; +if (globalThis.__never__) console.log(__nodeBuiltinProbe); diff --git a/docs/skill-authoring.md b/docs/skill-authoring.md index dca01497..d1a9070f 100644 --- a/docs/skill-authoring.md +++ b/docs/skill-authoring.md @@ -285,8 +285,10 @@ license (Copilot). A strict bound of `1.0` is therefore unreachable and the harness rejects it up front rather than failing every case. 0.5 is a floor on **useful reliability for a second-hop load**, not a claim that -0.8 is out of reach — a well-shaped pointer does clear it, as the worked example -below records. What 0.5 buys is that the column keeps carrying signal across the +0.8 is out of reach — a well-shaped pointer clears it outright, at 100% in the +worked example below. Note that a *chain* bar of 0.8 would be a **strict** one +(`> 0.8`, the comparison this threshold uses; the first-move 0.8 is the +inclusive `>=`), so at `RUNS=5` only a clean 5/5 would pass it — 4/5 would not. What 0.5 buys is that the column keeps carrying signal across the *range* of pointer strengths a repo actually has: a hand-off is a noisier measurement than a first move, so a bar set where a strong pointer sits marks every merely-adequate one red and stops distinguishing them from a broken one. diff --git a/scripts/skill-eval.mjs b/scripts/skill-eval.mjs index 7b98e6da..2464f5fa 100755 --- a/scripts/skill-eval.mjs +++ b/scripts/skill-eval.mjs @@ -58,12 +58,16 @@ const THRESHOLD = Number(process.env.THRESHOLD ?? 0.8); // tuned for the other measurement. 0.5 is the weakest claim worth asserting — // the pointer is taken more often than not — and it is a floor on useful // reliability for a SECOND-HOP load, not a claim that 0.8 is unreachable. A -// well-shaped pointer does clear 0.8: the committed `testing` -> `test-servers` -// cases measured 33% (RUNS=3) when `testing` merely classified which work -// belonged to `test-servers`, and 100%/80% (RUNS=5) once #2247 reshaped that -// into an imperative step. What 0.5 buys is a column that still separates an -// adequate pointer from a broken one — a hand-off is a noisier measurement than -// a first move, so a bar set where a STRONG pointer sits would mark both red. +// well-shaped pointer clears 0.8 outright: the committed `testing` -> +// `test-servers` cases measured 33% (RUNS=3) when `testing` merely classified +// which work belonged to `test-servers`, and 100%/100% (RUNS=5) once #2247 +// reshaped that into an imperative step. (An intermediate build of that change +// measured 100%/80%. That is NOT an example of clearing an 0.8 bar — this +// threshold is compared strictly, so 80% would fail one — but it is worth +// knowing as the residual noise still present at RUNS=5.) What 0.5 buys +// is a column that still separates an adequate pointer from a broken one — a +// hand-off is a noisier measurement than a first move, so a bar set where a +// STRONG pointer sits would mark both red. // A reshaped pointer therefore raises the ceiling those cases reach, not the // floor a *new* hand-off case should be judged against. // From ec6895a512ff176922c8a9f5b42826c40a028569 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sat, 5 Sep 2026 17:42:58 -0400 Subject: [PATCH 15/16] Remove the build-gate probe accidentally committed to main.tsx Copilot caught this. `scripts/verify-build-gate.mjs` appends a `node:fs` import to `clients/web/src/main.tsx`, asserts the browser-externalized-builtin gate rejects it, then restores the file. Round 6 was committed with `git add -A` while `local:gate` was mid-run, so the staged snapshot caught the probe in place and shipped it. The consequence is exactly what the gate exists to prevent: a Node built-in in the normal browser graph, which makes the gate fail every Vite build from here on. CI could not have passed. `main.tsx` is restored byte-for-byte to its v2/main content; no other file was affected. The transferable lesson, and the reason this is its own commit rather than a squash: **never stage with `git add -A` while the gate is running.** `verify:build-gate` mutates a tracked source file for the length of one build, and that window is wide enough to catch. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01EQVdemwgUhh45yzeaRSLhW Signed-off-by: cliffhall --- clients/web/src/main.tsx | 3 --- 1 file changed, 3 deletions(-) diff --git a/clients/web/src/main.tsx b/clients/web/src/main.tsx index 7af4d713..fb8a3562 100644 --- a/clients/web/src/main.tsx +++ b/clients/web/src/main.tsx @@ -21,6 +21,3 @@ createRoot(document.getElementById("root")!).render( , ); - -import * as __nodeBuiltinProbe from "node:fs"; -if (globalThis.__never__) console.log(__nodeBuiltinProbe); From 0d947ad56982c3114dc6bb1598c942c030554d38 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sat, 5 Sep 2026 18:03:37 -0400 Subject: [PATCH 16/16] Review round 8: using a fixture, not connecting, is the trigger MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Copilot, all three accepted. 1. Connecting still is not the condition. A few integration tests deliberately hand-roll a JSON-RPC server because the composable fixture *cannot* produce what they assert on — `inspectorClient-malformed-list.test.ts` says so in its own header: "the SDK's own server cannot emit these shapes: they are exactly what its types forbid". Same for `listSalvage-era.test.ts`. Real transport, real client, zero `test-servers/` dependency. Round 4 already stated the right condition at the top — "does this test depend on a fixture from `test-servers/`" — and then three downstream sites went on saying "connects". Those are now scoped to fixture use, and the hand-rolled exception is stated where the trigger is defined rather than left to be discovered, with both files named. Connecting is kept as a hint, since it usually does imply a fixture, with the instruction to check whether one can express the case first. 2. "description-strength measurement" was the wrong surface, and this PR is what made it wrong: a chained prompt is deliberately written so the target's *description* cannot fire on it, so the number measures the pointer in the first skill's body and nothing else. Left as-is it would send the next person tuning a red hand-off at the description. 3. "an 0.8 bar" -> "a 0.8 bar". Re-measured: 100% / 100% hand-off, 63/63 first-move at 100%, RUNS=5. Staged by explicit path, not `git add -A` — see ec6895a5. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01EQVdemwgUhh45yzeaRSLhW Signed-off-by: cliffhall --- .claude/skills/testing/SKILL.md | 42 ++++++++++++++++++++------------- docs/skill-authoring.md | 7 ++++-- scripts/skill-eval.mjs | 2 +- 3 files changed, 31 insertions(+), 20 deletions(-) diff --git a/.claude/skills/testing/SKILL.md b/.claude/skills/testing/SKILL.md index 5207db6e..840ee9ac 100644 --- a/.claude/skills/testing/SKILL.md +++ b/.claude/skills/testing/SKILL.md @@ -20,11 +20,17 @@ The condition is **"does this test depend on a fixture from `test-servers/`?"** — not which tier it lands in, and not which directory it lands in. There are two ways to depend on one, and they need different halves of that skill: -- **It connects.** An integration test that connects; an end-to-end test that - connects; a smoke that drives a connected flow; a coverage gap only reachable - over a real connection; reproducing a reported bug against a server. These - need the whole procedure — which showcase config, which protocol era, and the - staleness hazard. +- **It connects to a fixture.** An integration test that connects; an + end-to-end test that connects; a smoke that drives a connected flow; a + coverage gap only reachable over a real connection; reproducing a reported bug + against a server. These need the whole procedure — which showcase config, + which protocol era, and the staleness hazard. + ⚠️ **Connecting is a strong hint, not the rule.** A few integration tests + deliberately hand-roll a JSON-RPC server because the composable fixture + *cannot* produce what they assert on — `inspectorClient-malformed-list.test.ts` + and `listSalvage-era.test.ts` need wire shapes the SDK's own server refuses to + emit. Real transport, real client, no `test-servers/` dependency. Check + whether a fixture can express the case before reaching for one. - **It names or runs the built fixture without connecting.** `smoke:tui` boots the TUI against a catalog whose stdio command *is* the built fixture, then asserts it survives. No transport is driven and no protocol era applies, but @@ -79,8 +85,10 @@ web-owned test living under `src/test/` instead is a bug. folder is picked up by the integration project (node env, 30s timeouts) via a folder glob; there is no enumeration to keep in sync. ⚠️ Placement is *not* the fixture trigger, though — this folder holds pure parser and storage tests - alongside the connecting ones. If the test you are adding here **connects**, - **load the `test-servers` skill first**; the fixture is half of that test. + alongside the connecting ones. If the test you are adding here **needs a + fixture from `test-servers/`, load that skill first**; the fixture is half of + that test. Connecting is a strong hint but not the rule — see the + hand-rolled-server exception above. 3. **Shared test infrastructure** — `renderWithMantine.tsx`, `setup.ts`, `fixtures/`, `scrollAreaStoryAssertions.ts`. @@ -125,14 +133,15 @@ Storybook play functions (`test:storybook`) → the published-tarball check (`npm run pack:verify`, local/release only — needs network). ⚠️ **Depth in that list is not the fixture boundary, and the boundary cuts -across the tiers rather than along them.** Needing `test-servers/`: the -**connecting** web integration tests, the out-of-process CLI tests, the smokes +across the tiers rather than along them.** Needing `test-servers/`: the web +integration tests **that drive one**, the out-of-process CLI tests, the smokes that connect (`smoke:cli`, `smoke:web:app`, `smoke:web:elicit`, `smoke:web:tabs`), `pack:verify`, and **`smoke:tui`** — which never asserts a round trip but calls `ensureTestServers({ requires: ["stdio"] })` and hands the built fixture to the TUI as its catalog's stdio command. Not needing it: the -pure tests inside the same integration project, `smoke:launcher`, `smoke:web` -and `smoke:web:browser` (all three stop at boot without a fixture), and every +pure tests inside the same integration project, the connecting tests that +deliberately hand-roll a server, `smoke:launcher`, `smoke:web` and +`smoke:web:browser` (all three stop at boot without a fixture), and every Storybook play function (fixture props). **Load the `test-servers` skill as soon as a task puts you on the fixture side of that line** — whichever tier it sits in. @@ -214,12 +223,11 @@ shared helper that wraps one. ## Test servers, not mocks -The tests that drive MCP behaviour over a transport — the connecting integration -tests and the connected smokes — use a real server rather than a mock. **For -those, load the `test-servers` skill and use all of it**: which showcase config -covers the feature, which protocol era to connect with, how to add a combination -that does not exist yet, and why a fixture can keep serving stale code after an -edit. +The tests that drive MCP behaviour over a transport use a real server rather +than a mock, and **for the ones that get that server from `test-servers/`, load +the skill and use all of it**: which showcase config covers the feature, which +protocol era to connect with, how to add a combination that does not exist yet, +and why a fixture can keep serving stale code after an edit. **A test that only *names* the built fixture needs that skill too, for a narrower reason.** `smoke:tui` boots the TUI against a catalog whose stdio diff --git a/docs/skill-authoring.md b/docs/skill-authoring.md index d1a9070f..2f6f2851 100644 --- a/docs/skill-authoring.md +++ b/docs/skill-authoring.md @@ -292,8 +292,11 @@ inclusive `>=`), so at `RUNS=5` only a clean 5/5 would pass it — 4/5 would not *range* of pointer strengths a repo actually has: a hand-off is a noisier measurement than a first move, so a bar set where a strong pointer sits marks every merely-adequate one red and stops distinguishing them from a broken one. -Read a hand-off number as a description-strength measurement, not a verdict — -and read it at `RUNS=5`, since at `RUNS=3` one sample is worth 33 points. +Read a hand-off number as a **pointer**-strength measurement — the strength of +what the *first* skill's body says about the second, not of either description, +since a well-written chained prompt is one the target's description cannot +trigger on its own. Not a verdict, either; and read it at `RUNS=5`, since at +`RUNS=3` one sample is worth 33 points. **A red hand-off case is a finding about the pointer, not a build break — and the fix is to reshape the pointer, never to lower the bar.** The committed diff --git a/scripts/skill-eval.mjs b/scripts/skill-eval.mjs index 2464f5fa..780c09bb 100755 --- a/scripts/skill-eval.mjs +++ b/scripts/skill-eval.mjs @@ -62,7 +62,7 @@ const THRESHOLD = Number(process.env.THRESHOLD ?? 0.8); // `test-servers` cases measured 33% (RUNS=3) when `testing` merely classified // which work belonged to `test-servers`, and 100%/100% (RUNS=5) once #2247 // reshaped that into an imperative step. (An intermediate build of that change -// measured 100%/80%. That is NOT an example of clearing an 0.8 bar — this +// measured 100%/80%. That is NOT an example of clearing a 0.8 bar — this // threshold is compared strictly, so 80% would fail one — but it is worth // knowing as the residual noise still present at RUNS=5.) What 0.5 buys // is a column that still separates an adequate pointer from a broken one — a