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 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 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>
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>
- The CI wiring guard matched a substring, and last round's justification for
that was wrong: it accepts a longer script name
(`npm run verify:skills:cli:disabled`) and a mere mention (`echo npm run
verify:skills:cli`), either of which lets the real validator be removed while
the guard stays green. `runsCommand` now splits the script on the separators
that begin a new command and requires a segment to equal the invocation
exactly, with both counterexamples as regression cases.
- `local-dev`'s own description and its AGENTS.md index row both still promised
"where a new dependency must be declared" — the rules round 13 moved out of it
entirely. Both now describe what the skill actually carries: the reasoning
behind those rules. Recorded listing share updated to 1,199/4,000.
Signed-off-by: cliffhall <cliff@futurescale.com>
- skill-manifest: an eval file may only expect its own skill. A foreign
`expect: "other-skill"` passed whenever that other skill fired, so the file
reported a measurement of something it does not describe — and it satisfied
the positive-case requirement while doing so, masking the absence of a real
positive. Both halves covered by a regression case.
- local-dev: stop restating the dependency-placement rules. They are normative
and belong only in AGENTS.md; the skill now points there and carries what a
rule cannot — what each one is defending against, what it looked like when it
was violated (#1970, #2082, #2067, #1952, #1896, #2058), and how to tell you
have hit it. Including the two traps that make this class hard to see: the
ordinary test tiers cannot catch a bundling miss, and probing one by hand with
`node -e` falsely succeeds.
Signed-off-by: cliffhall <cliff@futurescale.com>
- verify:skills:cli — the authoritative validator is now a GUARANTEED step in
`local:gate` and CI, not a best-effort hand-off. It resolves the CLI itself
(an installed one at 2.1.233+, else a pinned `npx -y`), so both tiers run the
same script and cannot drift. `verify:skills` keeps skipping when the CLI is
absent, which is right for `validate`: fast, offline, no Claude Code required.
- skill-manifest: the 1,536-char listing-entry cap covers `description` +
`when_to_use` together; checking the description alone passed an over-cap
entry. `when_to_use` is now parsed, validated and counted, with boundary tests
at 1,536/1,537.
- verify-skills: report the budget as *this repo's contribution*. The real
listing budget is shared with bundled skills and the contributor's own
~/.claude/skills, none of which is visible from here, so the number was
claiming more than it measured.
- skill-eval: a negative case now means "no skill OF THIS REPO'S fired". It
failed on any Skill invocation, so a contributor with an unrelated global
skill got a false failure about their environment rather than these skills.
- docs/architecture + local-dev: the Node backend runner gets its
`@inspector/core` alias from `clients/web/tsup.runner.config.ts`, not from
Vite. The old text sent runner work to the wrong config.
- local-dev: the tarball does not ship "only each client's build/" — the
allowlist also carries `clients/web/dist`, `clients/web/static` and the
install helper. States the invariant that actually matters instead.
- issue-triage: the test for needing triage is "no board card". Adding "and no
milestone" would skip the approved-but-unboarded case the same section then
handles. Also corrected the `$M` note: the lookup is `--state all`, so closed
issues DO have entries — `isopen` is about the invariants, not the lookup.
- issue-create: replaced "all five" with a per-version table. Steps 3 and 6
(milestone, Priority) do not exist for v1, so the old list opened with a rule
the same skill later tells you to violate.
Signed-off-by: cliffhall <cliff@futurescale.com>
Closes#2163
`AGENTS.md` was one 1,223-line contract loaded in full on every turn, whether
the task was a one-line CSS fix or a release cut. This splits it: rules stay
resident, procedures move to `.claude/skills/`.
- Ten skills under `.claude/skills/`, seven `disable-model-invocation: true`
(invoked by name only, so they cost no listing context and have no trigger
question) and three model-invoked with committed eval cases.
- `AGENTS.md` 1,223 → 380 lines: rules, a skills index, and a project tree
reduced to one line per entry. The removed recipes are not duplicated.
- `README.md` 811 → 105 lines, split into `docs/{architecture,quality-gate,
test-servers,publishing,docker}.md`, and dropped from `CLAUDE.md`'s auto-load.
- `.github/copilot-instructions.md` and its mirroring rule removed — Copilot
reads `AGENTS.md`.
- `npm run verify:skills` (in `validate`, so in `local:gate` and CI) parses each
SKILL.md the way Claude Code does, requires an explicit invocation mode and
eval cases for model-invoked skills, and budgets the listing.
- `npm run skills:eval` runs the committed trigger cases headless.
Signed-off-by: cliffhall <cliff@futurescale.com>
