chore: address Copilot review round 2 (#2226)
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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01J86a6vMcA9csPtdsg7ZcCZ Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
cliffhall
parent
8e2c4d49fa
commit
5daa3f5060
3 changed files with 8 additions and 11 deletions
|
|
@ -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 `<client>/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 <pkg>` 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 <pkg>` 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`).
|
||||
|
|
|
|||
|
|
@ -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). |
|
||||
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
Loading…
Reference in a new issue