- Include `optionalDependencies` in the declared tier's candidate union. A
peer range constrains the consumer's host; an optional range is a direct
declaration npm attempts to install, so omitting it left an optional-only
package free to skew unseen. `peerDependencies` stays excluded, with the
reason stated.
- Make the end-to-end fixture's client copy UNDECLARED — only the root
manifest names `solo`, while `clients/web` merely holds a top-level copy.
That is #2226's defining shape, and it is what makes the candidate set a
union across installs rather than a per-install intersection; the old
fixture would have passed a per-install implementation. Adds the mirror
case (declared only by a client, held by the root) so the union is pinned
in both directions.
- Update the two docs that still described the single-tier guard:
`.claude/skills/local-dev/SKILL.md` said "nothing gates either of those,
and `verify:dep-lockstep` is not it" and cited the live `@types/node`
drift in the present tense; `docs/quality-gate.md` described only the
program tier. Both now state what each tier covers and the two limits
that remain.
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>
`clients/cli` resolved `@types/node` 24.13.1 against the root's 24.13.3.
Neither client declares it — #2196 consolidated it to the root — so the cli
copy arrived transitively via `@types/express` and was constrained by no
range of ours. `verify:dep-lockstep` reported OK and was right to: it
compares only packages that reach one `tsc` **program** from two installs,
and no one program sees both copies.
Fixes the instance and the class, which the issue notes are not exclusive.
**The class.** The guard grows a second tier alongside the program-derived
one, asking the weaker but broader question `AGENTS.md`'s "one version per
install-crossing dependency" rule actually states: does a package this repo
declares anywhere resolve to two versions across our installs at all? The
candidate set is every name in any install's `dependencies`/`devDependencies`
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, not ours.
Neither tier subsumes the other. The program tier sees a copy no manifest
names (the `@modelcontextprotocol/sdk` case, arriving through another
package's `.d.ts`). The declared tier sees a transitive copy no program
loads (this issue), two *clients* disagreeing with no root copy involved
(`@types/react`, web against tui), and the peer shadows `AGENTS.md` calls out
as ungated — `eslint`, `typescript`, `vitest`.
Deny-by-default, with its own empty `TOLERATED_DECLARED_SKEW` and the same
within-a-major rule, now stated once in a shared `toleratesSkew` so the two
tiers cannot drift.
**The instance, and three others the new tier surfaced.** All four were
patch-level:
- `@types/node` — cli 24.13.1 vs root 24.13.3. Transitive, so it takes an
`overrides` entry in `clients/cli` (the repo's sanctioned lever for a
transitive pin), at the root's own `^24.12.4` rather than a frozen number.
- `@types/react` — web 19.2.18 vs tui 19.2.17.
- `react` — web 19.2.8 vs root/tui 19.2.7.
- `@vitejs/plugin-react` — web 6.0.5 vs root 6.0.2.
The last three are `npm update` in the stale install: it moves the lockfile
within the declared range without widening a range, which matters for the
root `react ^19.0.0` that `ink`'s externalized-bundle exemption depends on.
No declaration or range changed.
Tests: 4 end-to-end cases (the fixture gains manifest deps and a client-side
`solo` install, so the skew is real and no program holds both copies) and 13
helper cases.
Closes#2226
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>
Two findings from the Copilot review of #1965, both real.
1. The candidates were flattened to package names before the version
comparison, so a third install holding a different copy failed the guard
even when no program loads that copy beside another — and the diagnostic
named an install that never took part. `crossInstallPackages` now returns
the co-occurrences themselves (name → program → install → entry paths) and
`findSkew` compares per program, across only the installs that met there.
2. Folding a nested copy onto its outermost install lost the path that
identifies which copy was loaded, and the version was then read from the
install's top-level entry — absent or differently versioned, either way
letting a real pair pass. `classifyModulePath` now also returns the
`entryPath` npm keys the lockfile by, `topLevelLockVersions` becomes
`lockVersionsByPath`, and each holder is priced from the entry the program
resolved. A resolved copy with no lockfile entry is now a hard failure
rather than a silently dropped holder.
The failure output prints `in <program>` with each holder's install path
beside its version, so a nested copy is distinguishable from a top-level one.
Signed-off-by: cliffhall <cliff@futurescale.com>
`verify:dep-lockstep` derived its candidate set from the packages the shared
first-party sources named *directly*, so a package whose declarations reach a
program only through another package's `.d.ts` was invisible to it — it could
skew across installs and recreate the two-copy blow-up #1896 was about while
the guard stayed green. `@modelcontextprotocol/sdk` was the live example: never
written in first-party code, yet 16 of its `.d.ts` files land in clients/web's
test program.
Replace the derivation with the measured one: list every client tsconfig
project with `tsc --listFilesOnly`, map each resolved `node_modules` file to
its owning install and package, and keep the packages that reach ONE program
from TWO installs. That is exactly the set that can put two structurally
distinct copies of a type in front of one checker. The rest of the guard is
unchanged — top-level lockfile comparison, deny-by-default, `TOLERATED_SKEW` as
a name allowlist scoped to within-a-major.
The listing machinery moves to `scripts/lib/tsc-program.mjs`, shared with
`verify:typecheck-coverage`, so the two guards can't disagree about what a
program contains. It is deliberately not cached to disk between the two
processes: a stale fingerprint would make a guard measure a program that no
longer exists and pass on a real miss. The second pass costs ~14s.
`TOLERATED_SKEW` is now empty. `jose` and `@modelcontextprotocol/ext-apps` are
root-only since #1970 and cannot skew; `react` (whose types are `@types/react`,
resolved from one install) and `hono` never reach one program from two
installs. Each would be a rationale for a skew that cannot occur.
Closes#1965
Signed-off-by: cliffhall <cliff@futurescale.com>
From Copilot's fourteenth pass of #1962. `hasReadableLockShape` and the
diagnostic it drives both promise "lockfileVersion 2+", but the check only
looked for a `packages` table with a root entry. A file declaring v1 while
carrying such a table was accepted, produced an empty version map, and could
make a real skew read as aligned once only one other holder remained — the
same fail-open the shape check was added to close, through the half of the
contract that wasn't enforced.
The declared version is now verified: a finite number, 2 or greater. Absent,
non-numeric, and declared-v1-with-packages cases are all in the table.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3
From Copilot's twelfth pass of #1962. A `/// <reference types="x" />` pulls in
a package's declarations exactly like an import does, but TypeScript reports
it in `preProcessFile`'s `typeReferenceDirectives`, not `importedFiles` — and
only the latter was read. A shared source using one could therefore load the
root's copy of a package alongside a client's without it ever entering the
candidate set, so its skew would pass. Same silent-miss direction as the
earlier syntax gaps, through a channel the parser switch didn't cover because
I only read one of its two outputs.
`typeReferencePackageNames` maps a directive to both candidates, since the
name is the *type* name and not the package: `node` resolves to `@types/node`,
while a package shipping its own declarations resolves to itself. Returning
both over-approximates, the safe direction — whichever isn't installed drops
out downstream. Scoped names mangle `@scope/pkg` → `@types/scope__pkg`,
TypeScript's convention. `path` references name a file, not a package, and are
ignored.
The real candidate set is unchanged at 17: no shared source uses a triple-slash
reference today. As with the other syntax fixes, this closes the channel
against the one that arrives later.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3
From Copilot's eleventh pass of #1962, and the most consequential kind of bug
for a gate: it failed *open*.
`topLevelLockVersions` returns an empty map for anything without a v2+
`packages` table — a lockfileVersion 1 file, or a malformed one. `main()` fed
that straight into the comparison, so such an install contributed no holders
at all, and a genuine skew among the remaining installs was reported as
aligned. The guard would have said OK to exactly what it exists to catch.
`hasReadableLockShape` now gates every discovered lockfile before any
comparison, requiring a `packages` object carrying the `""` root entry npm
always writes — "has a packages key" is not enough. An unreadable one is named
and exits 1, telling you to regenerate it. A JSON parse failure likewise
reports the file rather than dying on a bare SyntaxError with no path in it.
The empty-map behavior of the pure helper is kept and its test now says why it
is safe: the shape check rejects those inputs before they can reach
`findSkew`.
Three regression cases: a v1 lockfile in a fixture that IS skewed (so failing
open would show as a pass), a `packages` table with no root entry, and the
unit table for the predicate. Fixtures now write the `""` root entry, matching
real npm output.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3
From Copilot's eighth pass of #1962. The no-op guard tested only
`files.length === 0`, which stopped meaning anything once
`SHARED_SOURCE_FILES` was added: `vitest.shared.mts` alone keeps the total
nonzero, so a moved or renamed `core/` would leave the guard deriving
candidates from a near-empty set and passing on skew it exists to catch.
`sourcesWithNoFiles` now requires every configured dir and every named file to
have contributed, and names the ones that didn't. Directory matching is on a
path separator, so a prefix sibling (`core-internal/`) can't vouch for a
renamed `core/`.
Also drops two test comments left stale by the move to `ts.preProcessFile` —
they still described the deleted TRIVIA regex.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3
From Copilot's sixth pass of #1962.
Take the TypeScript-parser suggestion, which was raised across three rounds
and was right. The regex scan got both directions wrong and every widening
traded one failure for the other: it *missed* valid syntax (`import x =
require(…)`, an import-attributes argument, a comment between tokens — each a
silent miss, so the package never entered the candidate set and its skew
passed), and it *invented* names from prose, since `// adapted from "react"`
is indistinguishable from an import to a pattern that cannot tell code from a
comment. The earlier prose test only passed because `cwd omitted` isn't a
valid package name.
`ts.preProcessFile` is the cheap answer I'd missed while arguing against the
full AST: TypeScript's lightweight pre-parse scanner, no parse tree and no
type checking, purpose-built to return module specifiers. It handles every
import form, trivia, strings, and regex literals, and never reads a comment as
code. typescript resolves from `clients/web`, which already carries it, via
the `createRequire` base `smoke-web-browser.mjs` established — a bare import
would resolve relative to `scripts/`, not the cwd.
The derived set drops 18 → 17 real names: every genuine package is retained
and the one prose artifact is gone. Regression tests now use *real* package
names in comments and strings, the case the old test couldn't cover.
Also records the KNOWN BOUNDARY the review's other finding identified.
Candidates cover what the shared sources name directly, so a package reaching
the program only through another package's `.d.ts` is invisible —
`@modelcontextprotocol/sdk` is the live example, skewed 1.29.0 vs 1.30.0 and
present in web's program from both installs, never written in first-party
code. Closing it properly changes what the guard measures, so it is tracked
as #1965, with both candidate derivations measured there: a lockfile closure
is unusable (155 packages, 25 skewed, nearly all irrelevant tooling, and it
misses the SDK anyway), while `tsc --listFilesOnly` is correct and small
(15 for clients/web).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3
From Copilot's fifth pass of #1962. `TRIVIA` covered block comments but not
`//`, despite the contract right above it saying a comment is legal wherever
whitespace is — so `import(// lazy\n"pkg")`, `require(// lazy\n"pkg")`, and
`from // reason\n"pkg"` were stepped over, and such a package would never
enter the candidate set. Same silent-miss direction as the block-comment gap.
Both comment syntaxes are now covered. The line-comment branch runs to
end-of-line only; the newline itself is matched by TRIVIA's `\s` branch.
Regression test added alongside the block-comment one.
The derived set is unchanged at 18 real names — widening trivia does not
admit prose, since a specifier must still follow immediately and
`packageNameOf` rejects anything not shaped like one.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3
Both suppressed comments from Copilot's fourth pass of #1962.
TypeScript allows a comment anywhere whitespace is legal, so
`import(/* webpackIgnore: true */ "pkg")` and `from /* why */ "pkg"` are
valid imports the patterns stepped over. That is the dangerous direction: a
missed specifier never enters the candidate set, so the package's skew passes
the guard silently, against the over-approximation the module claims. A
`TRIVIA` fragment now stands in for `\s*` at every inter-token position, with
regression cases for the commented static, dynamic, side-effect, and
`require` forms.
I did not move to the TypeScript parser. This is a root `scripts/` `.mjs`
tool with no bundler and no TS dependency; pulling in a compiler API to read
import specifiers is a lot of machinery for a scan that only needs to
over-approximate, and each missed form is a one-line pattern with a test
beside it. The derived set is unchanged at 18 real names, so nothing here
widens what is checked today.
The README's command row also still described the candidate scan as `core/`
and `test-servers/src` only, omitting the `vitest.shared.mts` the guard now
includes; it also never mentioned that an allowlisted package is tolerated
only within a major. Both stated.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3
All four suppressed comments from Copilot's third pass of #1962. Each was
correct.
The candidate boundary omitted `vitest.shared.mts` — root-owned, imported by
every client's vitest config, and already treated as shared non-client source
by `verify:typecheck-coverage`. It imports only Node built-ins today, which
is exactly why the omission would have gone unnoticed until a third-party
import appeared there, resolved from the root, and skewed. Shared *files* are
now a first-class list beside the shared dirs.
The allowlist was a blanket exemption by name. Each entry's rationale
establishes that a patch/minor difference is benign, which is no evidence
that a React 18-vs-19 or Hono 4-vs-5 split would be — that is a different
type surface. Being listed now tolerates skew only *within a major version*;
a cross-major skew fails like anything else. Deriving the major from the
lockfile version keeps this automatic, with no per-package range to maintain
and bump. A version that can't be parsed is treated as "cannot prove same
major" and fails rather than passes.
`verify-format-coverage.mjs`'s comment overstated the vouching graph: the
three guards do not each check the others. This one checks both siblings;
each sibling checks only this one. Described as it actually works.
The AGENTS.md rule said to bump a shared dependency in "root + all four
clients", but launcher declares no zod and `findSkew` ignores installs where
a package is absent — so the rule as written asked for spurious dependencies.
Reworded to every install that *declares* it, and mirrored in
`.github/copilot-instructions.md`.
The failure output follows: it no longer advises adding a package that is
already allowlisted, saying "MAJOR skew" instead, and it names all three
shared sources rather than the two dirs.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3
From the suppressed comment on Copilot's second pass of #1962. Under-
approximating is the dangerous direction here: a package the scan misses
never enters the candidate set, so its skew passes the guard silently.
Three forms went unmatched. `import x = require("pkg")` and a bare
`require("pkg")` are the ordinary import syntax in `.cts` — which the
previous commit just admitted to the scan, so leaving them out would have
been an inconsistency introduced by that widening. A dynamic import carrying
import attributes (`import("pkg", { with: … })`) was missed because the
pattern required the closing paren. A static template literal
(`import(\`pkg\`)`) was missed for want of a backtick in the quote class.
Backticks are accepted only in the *call* forms. A `from` clause requires a
string literal, so allowing them there would buy nothing while reopening the
prose hazard the anchored specifier pattern closes: inline code in comments
is written with backticks throughout this codebase, and admitting it inflated
the derived set from 18 names to 41 with prose fragments (`tools`,
`connect()`, `messages`). Inert today, since a name absent from every
lockfile contributes nothing — but a prose word colliding with a real package
name would silently widen the set. Pinned with a regression test.
The real set is unchanged at 18, so these forms add no package today; they
close the syntax against the one that arrives later.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3
Both from Copilot's review of #1962.
The candidate-set enumeration matched only `.ts`/`.tsx`, while
`verify:format-coverage` and `verify:typecheck-coverage` both gate all four
TypeScript extensions. No `.mts`/`.cts` exists under `core/` or
`test-servers/src` today, which is exactly why the omission would have gone
unnoticed until a shared dependency arrived through one and skewed — the
failure this guard exists to prevent. Extract the rule as the exported
`isSharedSourceFile` predicate, cover all four extensions, and pin it with
regression tests (including path-boundary anchoring, so a `core-internal/`
sibling isn't swept in).
The README's `validate` row also omitted `test:scripts` and claimed typecheck
was cli/tui only; launcher runs one too, and web typechecks via `tsc -b`
inside its `build`. Restate the row to match the actual script chain.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3
Closes#1896
`clients/web`'s `tsc -b` exhausted the 4GB default heap at zod 4.4.x. The
cause is not a zod regression nor a pathological inference site of ours: it
is two copies of zod at *mismatched versions* inside one tsc program.
`clients/web/tsconfig.test.json` compiles `test-servers/src`, whose files
resolve zod (and the SDK's zod) from the **root** install, alongside web's
own sources, which resolve from `clients/web/node_modules`. At 4.3.6 vs
4.4.3 TypeScript must relate two structurally-distinct declarations of every
`@modelcontextprotocol/*` schema type, which is exponential over a recursive
generic surface — `TS2589 Type instantiation is excessively deep`, then OOM.
Bumping root zod to 4.4.3 and changing nothing else returns the build to
baseline: 11.4s and 2,567,882 instantiations, against 11.2s / 2,567,503 at
4.3.6. Two copies at the same version are harmless; skew is what explodes.
So drop the `~4.3.6` hold on `clients/web` and move all four manifests and
lockfiles to zod 4.4.3 together, and add `verify:dep-lockstep` to `validate`
to keep them there. The guard derives its candidate set from what `core/`
and `test-servers/src` import, compares the committed lockfiles' top-level
entries, and fails deny-by-default on any skew not in the annotated
`TOLERATED_SKEW` allowlist. It joins the existing mutual-vouch ring so
dropping it from `validate` is caught by `verify:format-coverage`.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HDdo1rNRVQnVRRdsqSrvG3
