chore: consolidate shared toolchain devDependencies into the root package.json - #2199
Conversation
…kage.json Declare the toolchain every client uses once, at the repo root, and delete the per-client copies: eslint, @eslint/js, typescript-eslint, globals, prettier and typescript (all five manifests), plus vitest, @vitest/coverage-v8 and @types/node (four). Adds the latter three to the root manifest; the first six were already there. 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 nothing about the client scripts changes. clients/launcher now declares no devDependencies at all and its validate is unchanged. The copies had already drifted: globals sat at ^17.7.0 at the root against ^17.4.0 in all four clients, and typescript-eslint at ^8.65.0 against ^8.56.1. Consolidating takes the root's version in both cases, and makes the exact prettier pin (#1790) a single pin rather than five that can diverge. Tools only one client runs stay where they are — tsup, tsx, vite, vite-node, playwright, storybook, happy-dom, ink-testing-library, and each client's own @types/*. Documents the rule in AGENTS.md's Dependency placement section and the reasoning in the local-dev skill. Signed-off-by: cliffhall <cliff@futurescale.com>
There was a problem hiding this comment.
🟡 Changes recommended
Client-local peer and transitive installations still shadow root tooling, including mismatched web Vitest versions.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Consolidates shared development tooling at the repository root to reduce version drift across clients.
Changes:
- Moves shared toolchain declarations to the root manifest.
- Regenerates root and client lockfiles.
- Documents the root-only dependency policy.
File summaries
| File | Description |
|---|---|
package.json |
Adds shared root tooling. |
package-lock.json |
Locks root tooling dependencies. |
clients/web/package.json |
Removes shared declarations. |
clients/web/package-lock.json |
Regenerates web dependency tree. |
clients/cli/package.json |
Removes shared declarations. |
clients/cli/package-lock.json |
Regenerates CLI dependency tree. |
clients/tui/package.json |
Removes shared declarations. |
clients/tui/package-lock.json |
Regenerates TUI dependency tree. |
clients/launcher/package.json |
Removes all launcher devDependencies. |
clients/launcher/package-lock.json |
Regenerates launcher dependency tree. |
AGENTS.md |
Adds the root-only toolchain rule. |
.claude/skills/local-dev/SKILL.md |
Explains toolchain placement. |
Review details
Files not reviewed (3)
- clients/launcher/package-lock.json: Generated file
- clients/tui/package-lock.json: Generated file
- clients/web/package-lock.json: Generated file
Suppressed comments (1)
clients/web/package-lock.json:9653
- The regenerated web lock still installs Vitest 4.1.10 locally as an auto-installed peer of
@vitest/browser-playwright, while the root lock resolves Vitest and@vitest/coverage-v8to 4.1.11. Becauseclients/web/node_modules/.binprecedes the root bin directory, web tests use 4.1.10 and coverage loads the root 4.1.11 provider, so the toolchain is both unconsolidated and version-mismatched. The peer provider and adapter need to share an install root (or client peer installation must be deliberately omitted) before regenerating the locks.
- Files reviewed: 7/12 changed files
- Comments generated: 6
- Review effort level: Balanced
…ceptions
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 <cliff@futurescale.com>
Copilot review round 1 — responsesMirroring the inline replies here, since they go outdated once the fix is pushed. One of these was a real defect and is fixed (edef659); the other five were accurate observations about what the change claims, and the docs now say the true thing. Fixed — the vitest version mismatch
Documented, not fixed — the surviving local copiesCopilot is right that deleting a declaration does not delete the copy, and that where a copy survives it is the one that wins. Two mechanisms, neither visible in a manifest:
Measured, after the fix:
No skew except the So the docs now state what the consolidation actually buys — one declaration and one place to bump, not one copy on disk — with Re-verified after the fix
|
There was a problem hiding this comment.
🟡 Changes recommended
The new guidance and PR description inaccurately characterize multi-client tooling and the scope of the dependency lockstep guard.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Files not reviewed (3)
- clients/launcher/package-lock.json: Generated file
- clients/tui/package-lock.json: Generated file
- clients/web/package-lock.json: Generated file
Suppressed comments (4)
Previously missed (1) — in code that hasn't changed since the last review.
AGENTS.md:90
- The “one client runs” rule is contradicted by the manifests:
tsupremains in web, CLI, and TUI, whileviteis declared at the root and in web and TUI. The actual boundary is whether tooling is shared by all clients; documenting it as single-client ownership can lead future dependency moves to the wrong manifest.
This issue also appears on line 91 of the same file.
- **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).
AGENTS.md:91
verify:dep-lockstepdoes not guarantee that these surviving tool copies agree. It only compares packages loaded from two installs into onetscprogram (scripts/verify-dep-lockstep.mjs:23-29), so local ESLint/Vitest binaries are outside its candidate set; even the documented CLI@types/nodecopy is 24.13.1 while the root resolves 24.13.3. This rule needs to distinguish that narrow type-level guard from whatever keeps tool binaries aligned.
- ⚠️ **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.
.claude/skills/local-dev/SKILL.md:149
- This overstates
verify:dep-lockstep: it only compares dependencies that onetscprogram loads from multiple installs, not surviving ESLint, Vitest, or other tool binaries. Those copies can drift without this guard reporting them, so the text should state the narrower guarantee.
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).
clients/web/package-lock.json:5064
- The PR description still says web, CLI, and TUI produce the same root-bin result as launcher, but this regenerated entry auto-installs a web-local ESLint peer, and the accepted verification shows that local binary wins. Update the PR description’s “Why it works” section to reflect the documented peer/transitive exceptions rather than claiming identical root resolution.
- Files reviewed: 7/12 changed files
- Comments generated: 1
- Review effort level: Balanced
Copilot review round 2. Docs only; no manifest or lockfile change. Two claims were wrong: - "A tool one client runs stays in that client" is contradicted by the manifests — 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. The boundary is "used by every client", not "used by one"; anything narrower stays put whether one client declares it or several do. Both are out of scope here and say so. - verify:dep-lockstep was named as the guard that the surviving copies agree. It is not: it derives its candidate set from what each tsc program resolves, so a tool binary never enters a program and is outside the set no matter how far it drifts — and cli's @types/node 24.13.1 against the root's 24.13.3 is a live skew that passes for the same reason. Both files now state the narrower guarantee, say that nothing gates the binaries automatically, note that each surviving copy is pinned by its holder's peer range, and give `npm exec -- which` as the manual check. Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot review round 2 — responsesAll four comments landed in the Suppressed block, so there is nothing inline to reply to. All four are correct; all four are addressed in 613c494 (docs only — no manifest or lockfile change) plus a PR-description rewrite. 1.
|
There was a problem hiding this comment.
🔵 Needs a closer look
The new guidance incorrectly claims all surviving copies remain peer-range aligned despite documenting a transitive version skew.
Review details
Files not reviewed (3)
- clients/launcher/package-lock.json: Generated file
- clients/tui/package-lock.json: Generated file
- clients/web/package-lock.json: Generated file
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
.claude/skills/local-dev/SKILL.md:154
- “Each surviving copy” includes hoisted transitives, yet the next paragraph records CLI
@types/node@24.13.1versus root24.13.3. Only peer-installed copies are constrained by holder peer ranges; transitive copies can already differ, so this rationale currently contradicts its own example.
So the consolidation buys **one declaration and one place to bump**, not one copy
on disk. Each surviving copy is pinned by its holder's peer range rather than by
a second declaration of ours, which is why they agree today.
AGENTS.md:91
- This says every surviving copy is kept aligned by a peer range, but the same paragraph documents a surviving transitive CLI
@types/node@24.13.1against root24.13.3. Transitive copies are not constrained by a holder's peer range, so distinguish the currently aligned peer-installed copies from independently resolved transitives.
- ⚠️ **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 keeps the surviving copies aligned is that each is pinned by its holder's peer range rather than by a second declaration of ours — and **nothing gates it automatically**. ⚠️ `verify:dep-lockstep` is *not* that gate: 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 `@types/node` patch difference between cli (`24.13.1`) and the root (`24.13.3`) is real and unreported for exactly that reason. Check a tool copy by hand — `npm exec -- which eslint` from the client — when you change what a client declares.
- Files reviewed: 7/12 changed files
- Comments generated: 0 new
- Review effort level: Balanced
Copilot review round 3. Docs only. The round-2 wording said every surviving copy is pinned by its holder's peer range, in the same paragraph that documents cli's @types/node 24.13.1 against the root's 24.13.3 — a transitive, which no peer range constrains. The rationale contradicted its own example. Both files now split the two mechanisms and grade them: - A peer copy is constrained by its holder's peer range, which is a real pin only when that range is exact. @vitest/browser-playwright pins vitest to a single version (hence the exact trio). eslint-plugin-react-refresh accepts ^9 || ^10, so the eslint copies agree only because npm resolves the same latest in both installs — a coincidence, not a guarantee. - A transitive copy is constrained by nothing of ours, and cli's @types/node has already diverged. It was 24.13.1 on v2/main too: a declared ^24.12.4 still loses to a nearer transitive. Nothing gates either, and verify:dep-lockstep is not that gate. Signed-off-by: cliffhall <cliff@futurescale.com>
Copilot review round 3 — responseOne comment, appearing twice ( Fixed in e76e32c (docs only). Both files now split the two mechanisms and grade them separately:
Followed by the unchanged point that nothing gates either, and that Re-verified
|
There was a problem hiding this comment.
🔵 Needs a closer look
Issue #2196 requires #2195 to land first, so this PR must be rebased and its lockfiles regenerated afterward.
Review details
Files not reviewed (3)
- clients/launcher/package-lock.json: Generated file
- clients/tui/package-lock.json: Generated file
- clients/web/package-lock.json: Generated file
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
package.json:118
- The linked #2196 explicitly requires #2195 to land first, but this PR states that #2195 has not started and is based directly on
v2/main. That prerequisite is therefore unmet; hold and rebase this consolidation after #2195, then regenerate the affected lockfiles before approval.
"@types/node": "^24.12.4",
- Files reviewed: 7/12 changed files
- Comments generated: 0 new
- Review effort level: Balanced
Copilot review round 4 — responseOne comment, in the Suppressed block, marked "Previously missed" and reporting 0 new comments generated. It restates the ordering caveat this PR already discloses in its own Ordering note, rather than finding anything in the diff. No code or docs change follows from it. On the substance: #2196 does name #2195 as step 1, and #2195 has not been started. I built this on Holding this until #2195 exists would leave a live version skew in place for no benefit — the
Review loop closedFour rounds. Summary of what changed as a result:
|
…olidate-toolchain-devdeps Signed-off-by: cliffhall <cliff@futurescale.com> # Conflicts: # clients/cli/package-lock.json # clients/launcher/package-lock.json # clients/launcher/package.json # clients/tui/package-lock.json # clients/web/package-lock.json
Closes #2196
Declares the toolchain every client uses once, at the repo root, and deletes the per-client copies.
What moved
eslint,@eslint/js,typescript-eslint,globals,prettier,typescriptvitest,@vitest/coverage-v8,@types/nodeclients/launchernow declares nodevDependenciesat all; the key is removed rather than left as{}.Left where they are, because only one client runs them:
tsup,tsx,vite,vite-node,playwright,storybook,happy-dom,ink-testing-library,@vitest/browser-playwright, the@storybook/*and@testing-library/*addons, and each client's own@types/*(@types/react,@types/express,@types/papaparse, …). Hoisting those would make every client install the union of all four.These all stay
devDependencies— nothing here is consumed at runtime, and the published tarball ships only each client'sbuild/.Why it works — and what it does not buy
npm runprepends every ancestornode_modules/.bintoPATH, and Node and TypeScript walk parentnode_modules/node_modules/@typesthe same way, so a client that declares none of the toolchain resolves the root's copy.clients/launcheris the clean case:prettiereslinttypescriptvitest@vitest/coverage-v8@types/nodeThe holders are web's
eslint-plugin-react-refresh/eslint-plugin-storybookand the TUI'seslint-plugin-react-hooks(ESLint),tsup/@storybook/react-vite(TypeScript),@vitest/browser-playwright/@storybook/addon-vitest(Vitest), and@types/express(@types/node). Eliminating them is not available — hoisting the plugins cascades (eslint-plugin-storybookalso peers onstorybook) and would make every client install the union of all four.The one real skew is cli's
@types/node24.13.1against the root's24.13.3, and it predates this PR: cli was already on24.13.1onv2/main, because a declared^24.12.4still loses to a nearer transitive.So what the change buys is one declaration and one place to bump, not one copy on disk — and the two mechanisms are not equally safe (Copilot, round 3). A peer copy is constrained by its holder's peer range, which is a real pin only when that range is exact:
@vitest/browser-playwrightpinsvitestto a single version, whileeslint-plugin-react-refreshaccepts^9 || ^10, so the ESLint copies agree only because npm resolves the same latest in both installs. A transitive copy is constrained by nothing of ours at all — which is why cli's@types/nodeis the one that has already diverged. Nothing gates either (see the guard note below).Everything else is unchanged: each client's
npm run validatepasses from its own directory,npm run test:storybookstill resolves its Playwright browser (local:gate'slocal:storybookstage is green), and the postinstall cascade (scripts/install-clients.mjs) runsnpm installin each client dir whenever apackage.jsonis present, regardless of what that manifest declares.The vitest trio is pinned exactly
@vitest/browser-playwright@4.1.10declares an exact peer onvitest("vitest": "4.1.10", not a range), so that package — not the root's range — decides whichvitestlands inclients/web. The first cut of this PR left the root floating^4.1.10, it resolved4.1.11, and web then ran its tests on the local4.1.10while loading a@vitest/coverage-v8@4.1.11provider. It passed either way, which is the bad part, and it was a regression this PR introduced (onv2/mainweb declared both at^4.1.10and got a matched pair). Caught by Copilot.vitestand@vitest/coverage-v8at the root and@vitest/browser-playwrightinclients/webare now pinned exactly to4.1.10and move together, the same discipline as the exactprettierpin (#1790).It had already drifted
This is not a cosmetic dedupe — two of these were already skewed before this change:
globals^17.7.0^17.4.0typescript-eslint^8.65.0^8.56.1Consolidating takes the root's version in both cases. It also makes the exact
prettierpin from #1790 a single pin instead of five that can diverge independently, so the format gate cannot return a different verdict depending on which client you ran it in.Guards
npm run verify:dep-lockstep— OK: 4 install-crossing dependencies agree across 5 installs (derived from 10 tsc programs), unchanged from thev2/mainbaseline. The candidate set is:@standard-schema/specclients/web/tsconfig.test.jsonhonoclients/web/tsconfig.{app,node,test}.jsonpinoclients/tui/tsconfig.json,clients/tui/tsconfig.test.jsonzodclients/web/tsconfig.{node,test}.jsonNone of the consolidated packages is a candidate.⚠️ That is a narrower guarantee than it looks (Copilot, round 2): the candidate set is derived from what each
tscprogram resolves, so a tool binary never enters a program and is outside the set no matter how far it drifts — and cli's@types/nodeskew above passes for the same reason (no single program sees both copies).verify:dep-lockstepis not what keeps the tool copies aligned, and bothAGENTS.mdand the skill now say so.TOLERATED_SKEWremains empty.Docs
AGENTS.md→ Dependency placement gains the rule.local-devskill gains the reasoning (Why the shared toolchain is root-only, What the walk-up does not buy you, Why the vitest trio is pinned exactly), per the rules-here / recipes-there split.tsup(web, cli, tui) andvite(web, tui, plus the root runtimedependency--web --devneeds) are shared by several clients without being shared by all, and consolidating them is a separate call with a separate rationale.README.mdsection on this points at thelocal-devskill rather than restating placement, so it needed no edit. There is no.github/copilot-instructions.mdin this repo, so the mirror named in the issue does not exist.Ordering note
#2196 is step 2 of #2030 and names #2195 (runtime
dependencies) as a prerequisite. #2195 has not been started and has no PR, so this was built directly onv2/main. The two steps touch disjoint sets — runtimedependenciesthere, toolchaindevDependencieshere — and no manifest line is claimed by both, so #2195 should rebase cleanly on top of this (or vice versa); only the lockfiles will need regenerating.Verification
Re-run after each review round:
npm run format— cleannpm run local:gate— passnpm run pack:verify— pass (tarball installs clean into a fresh consumer; web/cli/tui all drive end to end)