From edef659fd9a925f3412d4c00775b4b30b6ebf4c2 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Mon, 31 Aug 2026 19:25:27 -0400 Subject: [PATCH] fix: pin the vitest trio exactly, and document the peer/transitive exceptions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Copilot review round 1. @vitest/browser-playwright declares an EXACT peer on vitest ("vitest": "4.1.10"), so that package — not the root's range — decides which vitest lands in clients/web. Dropping web's own vitest declaration left the root free to resolve 4.1.11 while web's peer stayed at 4.1.10, so web's tests ran on one vitest while loading a @vitest/coverage-v8 provider built against another. Both still passed, which is the bad part. vitest and @vitest/coverage-v8 at the root and @vitest/browser-playwright in clients/web are now pinned exactly to 4.1.10 and move together, the same discipline as the exact prettier pin (#1790). The docs also claimed more than the change delivers. npm auto-installs an unmet peer into the install that needs it, and a client install cannot see the root's tree — so web's eslint-plugin-react-refresh / -storybook and the TUI's eslint-plugin-react-hooks each pull a client-local eslint, and web's Storybook/Vitest stack pulls a local typescript and vitest; @types/express hoists an @types/node into web and cli. Those copies sit nearer than the root's and win. AGENTS.md and the local-dev skill now say so, and state what the consolidation actually buys: one declaration and one place to bump, with verify:dep-lockstep as the guard that the copies agree. Signed-off-by: cliffhall --- .claude/skills/local-dev/SKILL.md | 45 +++++++++++-- AGENTS.md | 4 +- clients/web/package-lock.json | 2 +- clients/web/package.json | 2 +- package-lock.json | 104 +++++++++++++++--------------- package.json | 4 +- 6 files changed, 100 insertions(+), 61 deletions(-) diff --git a/.claude/skills/local-dev/SKILL.md b/.claude/skills/local-dev/SKILL.md index 813b36a3..36f6d43f 100644 --- a/.claude/skills/local-dev/SKILL.md +++ b/.claude/skills/local-dev/SKILL.md @@ -106,10 +106,10 @@ alias for those in `vitest.shared.mts`. `express` and `yaml` are the two today. Every client's `validate` shells out to `prettier`, `eslint`, `tsc` and `vitest`, which makes them look like they belong beside the scripts that call them. They do not need to be: `npm run` prepends **every ancestor** `node_modules/.bin` to -`PATH`, and Node and TypeScript walk parent `node_modules` / `node_modules/@types` -the same way — so a client with no toolchain declaration at all still resolves -the root's copy. `clients/launcher` declares no `devDependencies` whatsoever and -its `validate` is unchanged. +`PATH`, and Node and TypeScript walk parent `node_modules` / +`node_modules/@types` the same way — so a client that declares no toolchain at +all still resolves the root's copy. `clients/launcher` declares no +`devDependencies` whatsoever and its `validate` is unchanged. What a per-client declaration *does* buy is a second copy free to drift, and it had (#2196): `globals` sat at `^17.7.0` at the root against `^17.4.0` in all four @@ -124,6 +124,43 @@ only means something when there is one of it. where they are, because only one client runs them. Hoisting those would make every client install the union of all four. +#### What the walk-up does *not* buy you + +⚠️ **Deleting a client's declaration does not always delete the copy** — and +where a copy survives, it is the one that wins. Two mechanisms put one back, +neither of which the manifest mentions: + +- **An unmet peer.** npm auto-installs a peer into the install that needs it, and + a client install has no visibility into the root's tree, so the root's copy + cannot satisfy it. Web's `eslint-plugin-react-refresh` / `eslint-plugin-storybook` + and the TUI's `eslint-plugin-react-hooks` each pull a client-local `eslint`; + web's Storybook/Vitest stack pulls a local `typescript` and `vitest`. +- **A hoisted transitive.** `@types/express` brings `@types/node` into web and + cli's trees on its own. + +Those copies sit *nearer* than the root's, so `clients/web/node_modules/.bin` +precedes the root bin directory on `PATH` and TypeScript resolves the nearest +`node_modules/@types`. Verify with `npm exec -- which eslint` from the client +rather than assuming — the assumption is what made the first cut of #2196 claim +more than it delivered (Copilot). + +So the consolidation buys **one declaration and one place to bump**, not one copy +on disk. What has to hold is that the copies *agree*, and +`npm run verify:dep-lockstep` is the guard for exactly that (see below). + +#### Why the vitest trio is pinned exactly + +`@vitest/browser-playwright` declares an **exact** peer on `vitest` (`"vitest": +"4.1.10"`, not a range), so that package — not the root's range — decides which +`vitest` lands in `clients/web`. Left floating, the root resolves the newest +patch while web's peer stays pinned to the older one, and web's tests then run on +one `vitest` while loading a `@vitest/coverage-v8` provider built against +another. Both would still pass, which is the bad part. + +So `vitest` and `@vitest/coverage-v8` at the root and `@vitest/browser-playwright` +in `clients/web` are all pinned **exactly**, and a bump edits all three in one +change — the same discipline as the exact `prettier` pin (#1790). + ### Why runtime consumption decides `dependencies` The client builds externalise npm packages, so a published install resolves them diff --git a/AGENTS.md b/AGENTS.md index 0f29f31a..7152cc1b 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -87,7 +87,9 @@ The reasoning behind each of these, and what breaks when it is ignored, is the - **The MCP SDK packages** — `@modelcontextprotocol/client`, `core`, `server`, `server-legacy`, `ext-apps` — **are declared in the repo-root `package.json` and nowhere else.** So is anything reached only through root-owned code with no manifest of its own (`test-servers/src`, `core/`), aliased to the repo root in `vitest.shared.mts`. The v1 SDK (`@modelcontextprotocol/sdk`) is **not** a dependency of this repo and must not become one. - **`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 every client reaches the root copy by walk-up — `npm run` puts each ancestor `node_modules/.bin` on `PATH`, and Node and TypeScript resolve modules and `@types` the same way — so a client-side declaration buys nothing and installs a second copy that can drift. It had already drifted before #2196 (`globals` `^17.7.0` at the root against `^17.4.0` in all four clients, `typescript-eslint` `^8.65.0` against `^8.56.1`), and one pin instead of five is what makes the exact `prettier` pin (#1790) mean the format gate cannot return a different verdict per client. These stay **`devDependencies`** — none is consumed at runtime and the tarball ships only each client's `build/`. A tool **one** client uses stays in that client (`tsup`, `tsx`, `vite`, `vite-node`, `playwright`, `storybook`, `happy-dom`, `ink-testing-library`, and the `@types/*` for its own libraries). +- **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/`. A tool **one** client runs stays in that client (`tsup`, `tsx`, `vite`, `vite-node`, `playwright`, `storybook`, `happy-dom`, `ink-testing-library`, and the `@types/*` for its own libraries). + - ⚠️ **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; what must hold is that the copies **agree**, and `npm run verify:dep-lockstep` is the guard that says so. + - ⚠️ **`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. Never raise the tsc heap to work around a skew. `npm run verify:dep-lockstep` enforces this. diff --git a/clients/web/package-lock.json b/clients/web/package-lock.json index 7ef62e6a..5261828d 100644 --- a/clients/web/package-lock.json +++ b/clients/web/package-lock.json @@ -53,7 +53,7 @@ "@types/react-dom": "^19.2.3", "@types/react-syntax-highlighter": "^15.5.13", "@vitejs/plugin-react": "^6.0.0", - "@vitest/browser-playwright": "^4.1.10", + "@vitest/browser-playwright": "4.1.10", "eslint-plugin-react-hooks": "^7.1.1", "eslint-plugin-react-refresh": "^0.5.2", "eslint-plugin-storybook": "^10.5.5", diff --git a/clients/web/package.json b/clients/web/package.json index 454759a1..8cccc77e 100644 --- a/clients/web/package.json +++ b/clients/web/package.json @@ -76,7 +76,7 @@ "@types/react-dom": "^19.2.3", "@types/react-syntax-highlighter": "^15.5.13", "@vitejs/plugin-react": "^6.0.0", - "@vitest/browser-playwright": "^4.1.10", + "@vitest/browser-playwright": "4.1.10", "eslint-plugin-react-hooks": "^7.1.1", "eslint-plugin-react-refresh": "^0.5.2", "eslint-plugin-storybook": "^10.5.5", diff --git a/package-lock.json b/package-lock.json index b3ea9026..3bf9bb22 100644 --- a/package-lock.json +++ b/package-lock.json @@ -40,7 +40,7 @@ "@eslint/js": "^10.0.1", "@types/node": "^24.12.4", "@types/proper-lockfile": "^4.1.4", - "@vitest/coverage-v8": "^4.1.10", + "@vitest/coverage-v8": "4.1.10", "eslint": "^10.8.0", "eslint-plugin-react-hooks": "^7.1.1", "express": "^5.2.1", @@ -48,7 +48,7 @@ "prettier": "3.8.4", "typescript": "~5.9.3", "typescript-eslint": "^8.65.0", - "vitest": "^4.1.10" + "vitest": "4.1.10" }, "engines": { "node": ">=22.19.0" @@ -1620,14 +1620,14 @@ } }, "node_modules/@vitest/coverage-v8": { - "version": "4.1.11", - "resolved": "https://registry.npmjs.org/@vitest/coverage-v8/-/coverage-v8-4.1.11.tgz", - "integrity": "sha512-8MVGEFnJIcdGjcbfKmeq8z0pZHH0JlVtoVZH9Q/qwUp6wyFnEJUBMrw9DCaj+ra3vShGmhavjalMIhPNxZAUcw==", + "version": "4.1.10", + "resolved": "https://registry.npmjs.org/@vitest/coverage-v8/-/coverage-v8-4.1.10.tgz", + "integrity": "sha512-IM49HmthevbgAO4anp1hwtoT9wYe59w0LR00gr+eagHE+ZJ5lK4sLPeO0ubgoJcwLk6dehU3R24N+FbEEKDc8g==", "dev": true, "license": "MIT", "dependencies": { "@bcoe/v8-coverage": "^1.0.2", - "@vitest/utils": "4.1.11", + "@vitest/utils": "4.1.10", "ast-v8-to-istanbul": "^1.0.0", "istanbul-lib-coverage": "^3.2.2", "istanbul-lib-report": "^3.0.1", @@ -1641,8 +1641,8 @@ "url": "https://opencollective.com/vitest" }, "peerDependencies": { - "@vitest/browser": "4.1.11", - "vitest": "4.1.11" + "@vitest/browser": "4.1.10", + "vitest": "4.1.10" }, "peerDependenciesMeta": { "@vitest/browser": { @@ -1651,16 +1651,16 @@ } }, "node_modules/@vitest/expect": { - "version": "4.1.11", - "resolved": "https://registry.npmjs.org/@vitest/expect/-/expect-4.1.11.tgz", - "integrity": "sha512-VX2x5vNJXET47KAFzwERI+KRMtTTCSWTfSMKsW7JsUsXV4psq++e3DvZpuTDOpHcxytiDs6p2nhVb2tVDiiUYw==", + "version": "4.1.10", + "resolved": "https://registry.npmjs.org/@vitest/expect/-/expect-4.1.10.tgz", + "integrity": "sha512-YsCn+qAk1GWjQOWFEsEcL2gNQ0zmVmQu3T03qP6UyjhtmdtwtbuI+DASn/7iQB3HGTXkdBwGddzxPlmiql5vlA==", "dev": true, "license": "MIT", "dependencies": { "@standard-schema/spec": "^1.1.0", "@types/chai": "^5.2.2", - "@vitest/spy": "4.1.11", - "@vitest/utils": "4.1.11", + "@vitest/spy": "4.1.10", + "@vitest/utils": "4.1.10", "chai": "^6.2.2", "tinyrainbow": "^3.1.0" }, @@ -1669,13 +1669,13 @@ } }, "node_modules/@vitest/mocker": { - "version": "4.1.11", - "resolved": "https://registry.npmjs.org/@vitest/mocker/-/mocker-4.1.11.tgz", - "integrity": "sha512-2XJVD55d1o5AZous5CCGKS74g/riOj9odEt2bQpCVZeblHyHdnMeFl4jl0XjU21stf4mbjUkew2eXQZt65g5CQ==", + "version": "4.1.10", + "resolved": "https://registry.npmjs.org/@vitest/mocker/-/mocker-4.1.10.tgz", + "integrity": "sha512-v0xaezt+DKEmKfaxg133ldzADrwLGd7Ze1MfQQTYfvs8OqZIwbxyxaYURivwV7sWy5fqn3rH5uOrSp07bp44Ow==", "dev": true, "license": "MIT", "dependencies": { - "@vitest/spy": "4.1.11", + "@vitest/spy": "4.1.10", "estree-walker": "^3.0.3", "magic-string": "^0.30.21" }, @@ -1696,9 +1696,9 @@ } }, "node_modules/@vitest/pretty-format": { - "version": "4.1.11", - "resolved": "https://registry.npmjs.org/@vitest/pretty-format/-/pretty-format-4.1.11.tgz", - "integrity": "sha512-yiZzPbGTS9Sr/JpFl8zHrcIkAofNbFV6k21vIgQN/cY/oxZeXhJv5sc/MBJ5jFKWmWs+oJHw0UXLZjmf931+Vw==", + "version": "4.1.10", + "resolved": "https://registry.npmjs.org/@vitest/pretty-format/-/pretty-format-4.1.10.tgz", + "integrity": "sha512-W1HsjSH4MXQ9YfmmhLAoIYf1HRfekQCGngeIgcei6MP5QQGWUe0gkopdZQaVCFO+JDJMrAJGwa5pRpNpvy4P8Q==", "dev": true, "license": "MIT", "dependencies": { @@ -1709,13 +1709,13 @@ } }, "node_modules/@vitest/runner": { - "version": "4.1.11", - "resolved": "https://registry.npmjs.org/@vitest/runner/-/runner-4.1.11.tgz", - "integrity": "sha512-LztvUgdwMNJMIkj3hQnnxiC2Xy1zNxq928W/xhjCLaNCzqTZOudjwbQf6v9IntZGPw132i2Lq2rgTRZHD3JHNw==", + "version": "4.1.10", + "resolved": "https://registry.npmjs.org/@vitest/runner/-/runner-4.1.10.tgz", + "integrity": "sha512-IKI6kpIH+LmpROplyLwBBaCfMgOZOMsygVa6BARD6ahA04VRuJSa6OaVG7kRvSEMD870Vd91rSSw0eegtWyLGg==", "dev": true, "license": "MIT", "dependencies": { - "@vitest/utils": "4.1.11", + "@vitest/utils": "4.1.10", "pathe": "^2.0.3" }, "funding": { @@ -1723,14 +1723,14 @@ } }, "node_modules/@vitest/snapshot": { - "version": "4.1.11", - "resolved": "https://registry.npmjs.org/@vitest/snapshot/-/snapshot-4.1.11.tgz", - "integrity": "sha512-pN7ikn1ON7h8ee4gIAp4AzyK+zBtJPzVbqOgu5LCEh4VaJVbPQcgYQYJIMGQPXVeJJq1fnfazis7a5pFNPahog==", + "version": "4.1.10", + "resolved": "https://registry.npmjs.org/@vitest/snapshot/-/snapshot-4.1.10.tgz", + "integrity": "sha512-xRkfOT1qpTAi/Ti4Y1LtfRc3kEuqxGw59eN2jN9pRWMtS/XDevekhcFSqvQqjUNGksfjMJu3Y+oJ+4Ypn2OaJw==", "dev": true, "license": "MIT", "dependencies": { - "@vitest/pretty-format": "4.1.11", - "@vitest/utils": "4.1.11", + "@vitest/pretty-format": "4.1.10", + "@vitest/utils": "4.1.10", "magic-string": "^0.30.21", "pathe": "^2.0.3" }, @@ -1739,9 +1739,9 @@ } }, "node_modules/@vitest/spy": { - "version": "4.1.11", - "resolved": "https://registry.npmjs.org/@vitest/spy/-/spy-4.1.11.tgz", - "integrity": "sha512-apNa/prQy2qCeywhnixOHPRCgGNhvg7T4Dapfl1GahLp/R+uhBm5cPyFoNVyqsNd2h1nJxL6BqqdIjiABL60YA==", + "version": "4.1.10", + "resolved": "https://registry.npmjs.org/@vitest/spy/-/spy-4.1.10.tgz", + "integrity": "sha512-PLf/Ugvoq5wO/b4rwYCR1h2PSIdXz7wnkQFMiUpLdtM7l6pqVFcQIBEHyT1+l+cj7mNwAfZHzqXqDyjvOuwbDw==", "dev": true, "license": "MIT", "funding": { @@ -1749,13 +1749,13 @@ } }, "node_modules/@vitest/utils": { - "version": "4.1.11", - "resolved": "https://registry.npmjs.org/@vitest/utils/-/utils-4.1.11.tgz", - "integrity": "sha512-zTCVGpyFsGWBhllOyKlTw/vnr6D9qxsfSDyfbyZmTyjHw5N/VuvzHpHoQjm2ZJzn4RJgx5w4r7V0er69CmLgPQ==", + "version": "4.1.10", + "resolved": "https://registry.npmjs.org/@vitest/utils/-/utils-4.1.10.tgz", + "integrity": "sha512-fy9am/HWxbaGt/Sawrp90vt6Y6jQwf1RX77cz3uwoJwJVMli/e1IEwRPnMNJ7vKfPTwo0diXifkpPvwH9v7nGA==", "dev": true, "license": "MIT", "dependencies": { - "@vitest/pretty-format": "4.1.11", + "@vitest/pretty-format": "4.1.10", "convert-source-map": "^2.0.0", "tinyrainbow": "^3.1.0" }, @@ -5251,19 +5251,19 @@ } }, "node_modules/vitest": { - "version": "4.1.11", - "resolved": "https://registry.npmjs.org/vitest/-/vitest-4.1.11.tgz", - "integrity": "sha512-fhACrNXUidIbGSBr5FlbuBkO7VWC1ZyLl0DO4CU2DrQoAPxX84Ysxs+HeGQpii5lZWV1Q4gBZTTu49mF+A6Edw==", + "version": "4.1.10", + "resolved": "https://registry.npmjs.org/vitest/-/vitest-4.1.10.tgz", + "integrity": "sha512-R9jUTe5S4Qb0HCd4TNqpC7oGcrMssMRGXLW80ubjWsW9VH5GF8y1Y0SFLY9AbqSk6nt0PnOx4H4WNJYZ13GUPw==", "dev": true, "license": "MIT", "dependencies": { - "@vitest/expect": "4.1.11", - "@vitest/mocker": "4.1.11", - "@vitest/pretty-format": "4.1.11", - "@vitest/runner": "4.1.11", - "@vitest/snapshot": "4.1.11", - "@vitest/spy": "4.1.11", - "@vitest/utils": "4.1.11", + "@vitest/expect": "4.1.10", + "@vitest/mocker": "4.1.10", + "@vitest/pretty-format": "4.1.10", + "@vitest/runner": "4.1.10", + "@vitest/snapshot": "4.1.10", + "@vitest/spy": "4.1.10", + "@vitest/utils": "4.1.10", "es-module-lexer": "^2.0.0", "expect-type": "^1.3.0", "magic-string": "^0.30.21", @@ -5291,12 +5291,12 @@ "@edge-runtime/vm": "*", "@opentelemetry/api": "^1.9.0", "@types/node": "^20.0.0 || ^22.0.0 || >=24.0.0", - "@vitest/browser-playwright": "4.1.11", - "@vitest/browser-preview": "4.1.11", - "@vitest/browser-webdriverio": "4.1.11", - "@vitest/coverage-istanbul": "4.1.11", - "@vitest/coverage-v8": "4.1.11", - "@vitest/ui": "4.1.11", + "@vitest/browser-playwright": "4.1.10", + "@vitest/browser-preview": "4.1.10", + "@vitest/browser-webdriverio": "4.1.10", + "@vitest/coverage-istanbul": "4.1.10", + "@vitest/coverage-v8": "4.1.10", + "@vitest/ui": "4.1.10", "happy-dom": "*", "jsdom": "*", "vite": "^6.0.0 || ^7.0.0 || ^8.0.0" diff --git a/package.json b/package.json index a3f31000..d59d0c6c 100644 --- a/package.json +++ b/package.json @@ -117,7 +117,7 @@ "@eslint/js": "^10.0.1", "@types/node": "^24.12.4", "@types/proper-lockfile": "^4.1.4", - "@vitest/coverage-v8": "^4.1.10", + "@vitest/coverage-v8": "4.1.10", "eslint": "^10.8.0", "eslint-plugin-react-hooks": "^7.1.1", "express": "^5.2.1", @@ -125,6 +125,6 @@ "prettier": "3.8.4", "typescript": "~5.9.3", "typescript-eslint": "^8.65.0", - "vitest": "^4.1.10" + "vitest": "4.1.10" } }