Merge branch 'v2/main' into v2/feat/2263-skills-detail-pane
This commit is contained in:
Cliff Hall
GitHub
commit
98b39f95c5
18 changed files with 937 additions and 96 deletions
|
|
@ -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:
|
||||
|
||||
|
|
@ -146,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
|
||||
|
|
@ -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 <pkg>` there — it moves the lockfile
|
||||
within the declared range without widening the range, which
|
||||
`npm install <pkg>@<version>` 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`
|
||||
|
||||
|
|
|
|||
|
|
@ -11,11 +11,56 @@ 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.**
|
||||
|
||||
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 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
|
||||
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/`
|
||||
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.
|
||||
|
||||
**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
|
||||
|
||||
|
|
@ -38,7 +83,12 @@ 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. ⚠️ 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 **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`.
|
||||
|
||||
|
|
@ -82,6 +132,20 @@ 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, and the boundary cuts
|
||||
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, 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.
|
||||
|
||||
`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 +223,19 @@ 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.
|
||||
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
|
||||
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.
|
||||
|
|
|
|||
|
|
@ -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 `<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`) 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 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`).
|
||||
- **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
|
||||
|
|
|
|||
6
clients/cli/package-lock.json
generated
6
clients/cli/package-lock.json
generated
|
|
@ -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": {
|
||||
|
|
|
|||
|
|
@ -36,6 +36,7 @@
|
|||
"tsup": "^8.5.0"
|
||||
},
|
||||
"overrides": {
|
||||
"@types/node": "^24.12.4",
|
||||
"esbuild": "^0.28.2"
|
||||
}
|
||||
}
|
||||
|
|
|
|||
12
clients/tui/package-lock.json
generated
12
clients/tui/package-lock.json
generated
|
|
@ -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": {
|
||||
|
|
|
|||
10
clients/web/package-lock.json
generated
10
clients/web/package-lock.json
generated
|
|
@ -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
|
||||
}
|
||||
}
|
||||
},
|
||||
|
|
|
|||
|
|
@ -26,6 +26,34 @@ import {
|
|||
EMPTY_NETWORK_UI,
|
||||
} from "../components/screens/screenUiState.js";
|
||||
|
||||
/**
|
||||
* Run `body` with one `crypto` member hidden, then restore it.
|
||||
*
|
||||
* `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", 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 Partial<Pick<Crypto, "randomUUID">>)[name];
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
describe("oauthResume", () => {
|
||||
const storage = new Map<string, string>();
|
||||
|
||||
|
|
@ -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,39 +487,64 @@ 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(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", () => {
|
||||
// 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");
|
||||
// `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: {},
|
||||
});
|
||||
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;
|
||||
Object.defineProperty(globalThis, "crypto", original);
|
||||
}
|
||||
}
|
||||
expect(globalThis.crypto.randomUUID).toEqual(expect.any(Function));
|
||||
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", () => {
|
||||
|
|
|
|||
|
|
@ -227,12 +227,28 @@ 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();
|
||||
}
|
||||
// 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)}`;
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -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);
|
||||
});
|
||||
});
|
||||
|
||||
|
|
|
|||
|
|
@ -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: "<message> — <progress> / <total> (NN%)". The fraction
|
||||
|
|
|
|||
|
|
@ -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,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*. 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). |
|
||||
|
||||
|
|
|
|||
|
|
@ -284,22 +284,51 @@ 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 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.
|
||||
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.
|
||||
|
||||
**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.
|
||||
|
||||
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 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
|
||||
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
|
||||
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
|
||||
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
|
||||
|
|
@ -372,7 +401,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
|
||||
|
|
|
|||
18
package-lock.json
generated
18
package-lock.json
generated
|
|
@ -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"
|
||||
|
|
|
|||
|
|
@ -56,11 +56,20 @@ 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 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.
|
||||
// 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 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 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
|
||||
// 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
|
||||
|
|
|
|||
|
|
@ -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,100 @@ 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, 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" },
|
||||
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", () => {
|
||||
// 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" },
|
||||
webSolo: "7.8.9",
|
||||
},
|
||||
(dir) => {
|
||||
const { status, out } = runGuard(dir);
|
||||
assert.equal(status, 0, out);
|
||||
assert.match(out, /1 declared dependencies agree/);
|
||||
},
|
||||
);
|
||||
});
|
||||
|
||||
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
|
||||
// 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/);
|
||||
},
|
||||
);
|
||||
});
|
||||
|
|
|
|||
|
|
@ -44,6 +44,41 @@
|
|||
// 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` 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:
|
||||
//
|
||||
// • 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 <pkg>` 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 +132,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 +256,116 @@ 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`,
|
||||
* `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
|
||||
* for us", not "does it ship":
|
||||
*
|
||||
* • `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
|
||||
* 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",
|
||||
"optionalDependencies",
|
||||
])
|
||||
for (const name of Object.keys(manifest?.[field] ?? {})) names.add(name);
|
||||
return names;
|
||||
}
|
||||
|
||||
/**
|
||||
* An install's TOP-LEVEL installed versions — `node_modules/<name>`, with
|
||||
* nested copies (`node_modules/a/node_modules/<name>`) excluded.
|
||||
*
|
||||
* Top-level is the right depth here: it is what the install's own code resolves,
|
||||
* and it is the single copy a `<name>` 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 +646,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 <pkg>` in whichever install is behind — that moves the lockfile" +
|
||||
"\nwithin the declared range, without widening a range as `npm install <pkg>@<version>` 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)
|
||||
|
|
|
|||
|
|
@ -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,245 @@ 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, 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([
|
||||
{ dir: ".", manifest: { dependencies: { react: "^19.0.0" } } },
|
||||
{
|
||||
dir: "clients/web",
|
||||
manifest: {
|
||||
dependencies: { "react-dom": "^19.2.4" },
|
||||
devDependencies: { "@types/react": "^19.2.14" },
|
||||
optionalDependencies: { fsevents: "^2.3.3" },
|
||||
},
|
||||
},
|
||||
]),
|
||||
].sort(),
|
||||
["@types/react", "fsevents", "react", "react-dom"],
|
||||
);
|
||||
});
|
||||
|
||||
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" } } },
|
||||
]),
|
||||
],
|
||||
[],
|
||||
);
|
||||
});
|
||||
|
||||
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,
|
||||
);
|
||||
});
|
||||
|
|
|
|||
Loading…
Reference in a new issue