chore(deps): scope the allowlist to a major, cover vitest.shared.mts, fix the vouch comment
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
This commit is contained in:
parent
47e0ee059a
commit
35317fd9ed
5 changed files with 162 additions and 20 deletions
2
.github/copilot-instructions.md
vendored
2
.github/copilot-instructions.md
vendored
|
|
@ -92,7 +92,7 @@ Both exist and do different jobs. Theme files (`src/theme/<Component>.ts`) custo
|
||||||
## Gates and PR hygiene
|
## Gates and PR hygiene
|
||||||
|
|
||||||
- `npm run format` before committing; **`npm run ci` before pushing** (`validate` → `coverage` → `verify:build-gate` → `smoke` → Storybook). `npm run validate` is the fast inner-loop check and is **not** a substitute — it runs `test`, not `test:coverage`, so it does zero coverage gating.
|
- `npm run format` before committing; **`npm run ci` before pushing** (`validate` → `coverage` → `verify:build-gate` → `smoke` → Storybook). `npm run validate` is the fast inner-loop check and is **not** a substitute — it runs `test`, not `test:coverage`, so it does zero coverage gating.
|
||||||
- **A dependency bump must land in every install.** v2 is not a workspace — the root and each `clients/*` have their own `node_modules`, and a client's test project compiles `core/` and `test-servers/src` (which resolve from the **root**) alongside the client's own sources. Bumping a shared dependency in one manifest only puts two versions of it in one `tsc` program; for a recursive-generic surface like zod that exhausts the tsc heap (#1896). `verify:dep-lockstep` fails the build on this, so a PR touching one `package.json`/`package-lock.json` for a package `core/` or `test-servers/src` imports should touch all of them.
|
- **A dependency bump must land in every install that declares it.** v2 is not a workspace — the root and each `clients/*` have their own `node_modules`, and a client's test project compiles `core/` and `test-servers/src` (which resolve from the **root**) alongside the client's own sources. Bumping a shared dependency in one manifest only puts two versions of it in one `tsc` program; for a recursive-generic surface like zod that exhausts the tsc heap (#1896). `verify:dep-lockstep` fails the build on this, so a PR bumping a package that the shared sources import should update the root **and every client that already lists it** — not every client unconditionally, since a package absent from an install can't skew and adding it there would be a spurious dependency.
|
||||||
- **Every PR references an issue**, first body line `Closes #<ISSUE_NUMBER>`.
|
- **Every PR references an issue**, first body line `Closes #<ISSUE_NUMBER>`.
|
||||||
- **Every PR carries exactly one version label**, `v1` or `v2`, matching its base branch.
|
- **Every PR carries exactly one version label**, `v1` or `v2`, matching its base branch.
|
||||||
- Update the relevant `README.md` / `AGENTS.md` when a change adds, removes, renames, or repurposes a file or folder, changes the structure or tech stack, or introduces a command, dependency, or architectural pattern.
|
- Update the relevant `README.md` / `AGENTS.md` when a change adds, removes, renames, or repurposes a file or folder, changes the structure or tech stack, or introduces a command, dependency, or architectural pattern.
|
||||||
|
|
|
||||||
File diff suppressed because one or more lines are too long
|
|
@ -21,8 +21,9 @@
|
||||||
// two copies — changing nothing else — returned the build to its baseline cost.
|
// two copies — changing nothing else — returned the build to its baseline cost.
|
||||||
//
|
//
|
||||||
// The candidate set is DERIVED, not hand-listed: it is the packages imported by
|
// The candidate set is DERIVED, not hand-listed: it is the packages imported by
|
||||||
// `core/` and `test-servers/src`, the two first-party surfaces compiled into
|
// the shared first-party TypeScript — `core/`, `test-servers/src`, and the
|
||||||
// more than one client's program. Skew is then denied by default, with a small
|
// root-owned `vitest.shared.mts` — the surfaces compiled into more than one
|
||||||
|
// client's program. Skew is then denied by default, with a small
|
||||||
// allowlist of packages verified to tolerate it (below). A dependency that
|
// allowlist of packages verified to tolerate it (below). A dependency that
|
||||||
// starts skewing therefore fails `validate` and forces a decision, rather than
|
// starts skewing therefore fails `validate` and forces a decision, rather than
|
||||||
// surfacing months later as an unexplained OOM.
|
// surfacing months later as an unexplained OOM.
|
||||||
|
|
@ -45,11 +46,25 @@ const repoRoot = path.resolve(
|
||||||
// `test-servers/src` is pulled into the web and cli test projects.
|
// `test-servers/src` is pulled into the web and cli test projects.
|
||||||
const SHARED_SOURCE_DIRS = ["core", "test-servers/src"];
|
const SHARED_SOURCE_DIRS = ["core", "test-servers/src"];
|
||||||
|
|
||||||
|
// Individual root-owned TypeScript files that are shared the same way but sit
|
||||||
|
// outside those trees. `vitest.shared.mts` is imported by every client's vitest
|
||||||
|
// config, and `verify:typecheck-coverage` already treats it as shared
|
||||||
|
// non-client source. It imports only Node built-ins today — which is precisely
|
||||||
|
// why omitting it would go unnoticed until a third-party import appeared there,
|
||||||
|
// resolved from the root, and skewed (Copilot, #1962).
|
||||||
|
const SHARED_SOURCE_FILES = ["vitest.shared.mts"];
|
||||||
|
|
||||||
// Packages whose cross-install skew is verified benign, each with the reason.
|
// Packages whose cross-install skew is verified benign, each with the reason.
|
||||||
// This is an allowlist of *names*, not of version pairs, so an ordinary patch
|
// This is an allowlist of *names*, not of version pairs, so an ordinary patch
|
||||||
// float within one of these does not churn the file — while any package NOT
|
// float within one of these does not churn the file — while any package NOT
|
||||||
// listed here failing the check is a genuine, unreviewed new skew.
|
// listed here failing the check is a genuine, unreviewed new skew.
|
||||||
//
|
//
|
||||||
|
// Being listed is NOT a blanket exemption: it tolerates skew only *within a
|
||||||
|
// major version*. Each rationale below establishes that a patch/minor
|
||||||
|
// difference is harmless, which is not evidence that a React 18-vs-19 or Hono
|
||||||
|
// 4-vs-5 split across installs would be — that is a different type surface, and
|
||||||
|
// it fails like anything else (Copilot, #1962).
|
||||||
|
//
|
||||||
// The admission test is the one the zod incident established: does the
|
// The admission test is the one the zod incident established: does the
|
||||||
// package's public type surface consist of deeply recursive generics that
|
// package's public type surface consist of deeply recursive generics that
|
||||||
// first-party code relates across the boundary? If yes it must stay in
|
// first-party code relates across the boundary? If yes it must stay in
|
||||||
|
|
@ -175,11 +190,34 @@ export function findSkew(candidates, installs) {
|
||||||
return skewed;
|
return skewed;
|
||||||
}
|
}
|
||||||
|
|
||||||
/** Split skewed packages into the tolerated ones and the failures. */
|
/**
|
||||||
|
* The major-version component of a lockfile version string. Prerelease and
|
||||||
|
* build metadata are irrelevant here (`2.0.0-beta.5` → `2`). Returns null for
|
||||||
|
* anything not starting with an integer, which is treated as "cannot prove same
|
||||||
|
* major" and therefore fails rather than passes.
|
||||||
|
*/
|
||||||
|
export function majorOf(version) {
|
||||||
|
const m = /^(\d+)\./.exec(String(version ?? ""));
|
||||||
|
return m ? m[1] : null;
|
||||||
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Split skewed packages into the tolerated ones and the failures.
|
||||||
|
*
|
||||||
|
* Being on the allowlist tolerates skew only *within a major version*: each
|
||||||
|
* entry's rationale establishes that a patch/minor difference is benign, which
|
||||||
|
* says nothing about a major split, where the type surface itself changes. So a
|
||||||
|
* listed package whose holders disagree on major is still a failure.
|
||||||
|
*/
|
||||||
export function partitionSkew(skewed, tolerated = TOLERATED_SKEW) {
|
export function partitionSkew(skewed, tolerated = TOLERATED_SKEW) {
|
||||||
|
const isTolerated = (s) => {
|
||||||
|
if (!tolerated.has(s.name)) return false;
|
||||||
|
const majors = new Set(s.holders.map((h) => majorOf(h.version)));
|
||||||
|
return majors.size === 1 && !majors.has(null);
|
||||||
|
};
|
||||||
return {
|
return {
|
||||||
failures: skewed.filter((s) => !tolerated.has(s.name)),
|
failures: skewed.filter((s) => !isTolerated(s)),
|
||||||
ignored: skewed.filter((s) => tolerated.has(s.name)),
|
ignored: skewed.filter(isTolerated),
|
||||||
};
|
};
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
@ -191,8 +229,12 @@ export function partitionSkew(skewed, tolerated = TOLERATED_SKEW) {
|
||||||
// unnoticed until a new shared dependency arrived through one and skewed.
|
// unnoticed until a new shared dependency arrived through one and skewed.
|
||||||
const SOURCE_EXTENSIONS = [".ts", ".tsx", ".mts", ".cts"];
|
const SOURCE_EXTENSIONS = [".ts", ".tsx", ".mts", ".cts"];
|
||||||
|
|
||||||
/** Whether a repo-relative path is a TypeScript source of a shared tree. */
|
/**
|
||||||
|
* Whether a repo-relative path is shared first-party TypeScript — under one of
|
||||||
|
* the shared trees, or one of the individually-named shared files.
|
||||||
|
*/
|
||||||
export function isSharedSourceFile(file) {
|
export function isSharedSourceFile(file) {
|
||||||
|
if (SHARED_SOURCE_FILES.includes(file)) return true;
|
||||||
if (!SOURCE_EXTENSIONS.some((ext) => file.endsWith(ext))) return false;
|
if (!SOURCE_EXTENSIONS.some((ext) => file.endsWith(ext))) return false;
|
||||||
// Anchored on a path boundary so a sibling whose name merely starts with a
|
// Anchored on a path boundary so a sibling whose name merely starts with a
|
||||||
// shared dir (`core-internal/`, `test-servers/src-legacy/`) isn't swept in.
|
// shared dir (`core-internal/`, `test-servers/src-legacy/`) isn't swept in.
|
||||||
|
|
@ -203,7 +245,12 @@ export function isSharedSourceFile(file) {
|
||||||
function sharedSourceFiles() {
|
function sharedSourceFiles() {
|
||||||
const out = execFileSync(
|
const out = execFileSync(
|
||||||
"git",
|
"git",
|
||||||
["ls-files", "--", ...SHARED_SOURCE_DIRS.map((d) => `${d}/**`)],
|
[
|
||||||
|
"ls-files",
|
||||||
|
"--",
|
||||||
|
...SHARED_SOURCE_DIRS.map((d) => `${d}/**`),
|
||||||
|
...SHARED_SOURCE_FILES,
|
||||||
|
],
|
||||||
{ cwd: repoRoot, encoding: "utf8" },
|
{ cwd: repoRoot, encoding: "utf8" },
|
||||||
);
|
);
|
||||||
return out.split("\n").filter(isSharedSourceFile);
|
return out.split("\n").filter(isSharedSourceFile);
|
||||||
|
|
@ -278,22 +325,36 @@ export function main() {
|
||||||
console.error(
|
console.error(
|
||||||
`verify:dep-lockstep — ${failures.length} ${failures.length === 1 ? "dependency resolves" : "dependencies resolve"} to different versions across installs:\n`,
|
`verify:dep-lockstep — ${failures.length} ${failures.length === 1 ? "dependency resolves" : "dependencies resolve"} to different versions across installs:\n`,
|
||||||
);
|
);
|
||||||
|
let anyListed = false;
|
||||||
for (const { name, holders } of failures) {
|
for (const { name, holders } of failures) {
|
||||||
console.error(` ${name}`);
|
// A package already on the allowlist reached here only by skewing across
|
||||||
|
// a MAJOR boundary, so say that rather than advising an entry that exists.
|
||||||
|
const listed = TOLERATED_SKEW.has(name);
|
||||||
|
anyListed ||= listed;
|
||||||
|
console.error(
|
||||||
|
` ${name}${listed ? " (allowlisted — but this is a MAJOR skew)" : ""}`,
|
||||||
|
);
|
||||||
for (const { dir, version } of holders)
|
for (const { dir, version } of holders)
|
||||||
console.error(` ${version} (${dir})`);
|
console.error(` ${version} (${dir})`);
|
||||||
}
|
}
|
||||||
|
const shared = [...SHARED_SOURCE_DIRS, ...SHARED_SOURCE_FILES].join(", ");
|
||||||
console.error(
|
console.error(
|
||||||
"\nThese packages' types are compiled into a single `tsc` program from two installs" +
|
"\nThese packages' types are compiled into a single `tsc` program from two installs" +
|
||||||
`\n(${SHARED_SOURCE_DIRS.join(" and ")} resolve from the root, a client's own sources from the client),` +
|
`\n(${shared} resolve from the root, a client's own sources from the client),` +
|
||||||
"\nso a version skew makes TypeScript relate two structurally-distinct copies of the same" +
|
"\nso a version skew makes TypeScript relate two structurally-distinct copies of the same" +
|
||||||
"\ntype. For a recursive-generic surface like zod that is what exhausted the tsc heap in #1896.",
|
"\ntype. For a recursive-generic surface like zod that is what exhausted the tsc heap in #1896.",
|
||||||
);
|
);
|
||||||
console.error(
|
console.error(
|
||||||
"\nAlign them — `npm install <pkg>@<version>` in each install so all lockfiles agree — or, if this" +
|
"\nAlign them — `npm install <pkg>@<version>` in each install that declares the package, so all" +
|
||||||
"\npackage's types genuinely cannot blow up, add it to TOLERATED_SKEW in scripts/verify-dep-lockstep.mjs" +
|
"\nlockfiles agree. (Don't add it to an install that doesn't declare it: a package absent from an" +
|
||||||
"\nwith the reason. See AGENTS.md.",
|
"\ninstall can't skew.) If instead its types genuinely cannot blow up, add it to TOLERATED_SKEW in" +
|
||||||
|
"\nscripts/verify-dep-lockstep.mjs with the reason. See AGENTS.md.",
|
||||||
);
|
);
|
||||||
|
if (anyListed)
|
||||||
|
console.error(
|
||||||
|
"\nNote: an allowlisted package is tolerated only WITHIN a major version — the rationale for one" +
|
||||||
|
"\nestablishes that a patch/minor difference is benign, not that a major split is. Align the major.",
|
||||||
|
);
|
||||||
process.exit(1);
|
process.exit(1);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
|
||||||
|
|
@ -10,6 +10,7 @@ import {
|
||||||
findSkew,
|
findSkew,
|
||||||
importedPackageNames,
|
importedPackageNames,
|
||||||
isSharedSourceFile,
|
isSharedSourceFile,
|
||||||
|
majorOf,
|
||||||
packageNameOf,
|
packageNameOf,
|
||||||
partitionSkew,
|
partitionSkew,
|
||||||
topLevelLockVersions,
|
topLevelLockVersions,
|
||||||
|
|
@ -240,8 +241,20 @@ test("partitionSkew: the allowlist is by name, not by version pair", () => {
|
||||||
// So an ordinary patch float within a tolerated package does not churn the
|
// So an ordinary patch float within a tolerated package does not churn the
|
||||||
// allowlist, while any *unlisted* package that starts skewing still fails.
|
// allowlist, while any *unlisted* package that starts skewing still fails.
|
||||||
const skewed = [
|
const skewed = [
|
||||||
{ name: "react", holders: [] },
|
{
|
||||||
{ name: "zod", holders: [] },
|
name: "react",
|
||||||
|
holders: [
|
||||||
|
{ dir: ".", version: "19.2.7" },
|
||||||
|
{ dir: "clients/web", version: "19.2.8" },
|
||||||
|
],
|
||||||
|
},
|
||||||
|
{
|
||||||
|
name: "zod",
|
||||||
|
holders: [
|
||||||
|
{ dir: ".", version: "4.3.6" },
|
||||||
|
{ dir: "clients/web", version: "4.4.3" },
|
||||||
|
],
|
||||||
|
},
|
||||||
];
|
];
|
||||||
const tolerated = new Map([["react", "shallow interfaces"]]);
|
const tolerated = new Map([["react", "shallow interfaces"]]);
|
||||||
const { failures, ignored } = partitionSkew(skewed, tolerated);
|
const { failures, ignored } = partitionSkew(skewed, tolerated);
|
||||||
|
|
@ -256,6 +269,72 @@ test("partitionSkew: the allowlist is by name, not by version pair", () => {
|
||||||
});
|
});
|
||||||
|
|
||||||
test("partitionSkew: deny by default — nothing tolerated fails everything", () => {
|
test("partitionSkew: deny by default — nothing tolerated fails everything", () => {
|
||||||
const skewed = [{ name: "zod", holders: [] }];
|
const skewed = [{ name: "zod", holders: [{ dir: ".", version: "1.0.0" }] }];
|
||||||
assert.equal(partitionSkew(skewed, new Map()).failures.length, 1);
|
assert.equal(partitionSkew(skewed, new Map()).failures.length, 1);
|
||||||
});
|
});
|
||||||
|
|
||||||
|
test("partitionSkew: the allowlist tolerates skew only within a major (Copilot, #1962)", () => {
|
||||||
|
// Each rationale establishes that a patch/minor difference is benign; that is
|
||||||
|
// not evidence a React 18-vs-19 split is, so a listed package still fails
|
||||||
|
// across a major boundary.
|
||||||
|
const tolerated = new Map([["react", "shallow interfaces"]]);
|
||||||
|
const withinMajor = [
|
||||||
|
{
|
||||||
|
name: "react",
|
||||||
|
holders: [
|
||||||
|
{ dir: ".", version: "19.2.7" },
|
||||||
|
{ dir: "clients/web", version: "19.2.8" },
|
||||||
|
],
|
||||||
|
},
|
||||||
|
];
|
||||||
|
const acrossMajor = [
|
||||||
|
{
|
||||||
|
name: "react",
|
||||||
|
holders: [
|
||||||
|
{ dir: ".", version: "18.3.1" },
|
||||||
|
{ dir: "clients/web", version: "19.2.8" },
|
||||||
|
],
|
||||||
|
},
|
||||||
|
];
|
||||||
|
assert.equal(partitionSkew(withinMajor, tolerated).failures.length, 0);
|
||||||
|
assert.equal(partitionSkew(withinMajor, tolerated).ignored.length, 1);
|
||||||
|
assert.equal(partitionSkew(acrossMajor, tolerated).failures.length, 1);
|
||||||
|
assert.equal(partitionSkew(acrossMajor, tolerated).ignored.length, 0);
|
||||||
|
});
|
||||||
|
|
||||||
|
test("partitionSkew: an unparseable version can't be proven same-major, so it fails", () => {
|
||||||
|
const tolerated = new Map([["react", "shallow interfaces"]]);
|
||||||
|
const skewed = [
|
||||||
|
{
|
||||||
|
name: "react",
|
||||||
|
holders: [
|
||||||
|
{ dir: ".", version: "19.2.7" },
|
||||||
|
{ dir: "clients/web", version: "next" },
|
||||||
|
],
|
||||||
|
},
|
||||||
|
];
|
||||||
|
assert.equal(partitionSkew(skewed, tolerated).failures.length, 1);
|
||||||
|
});
|
||||||
|
|
||||||
|
test("majorOf: prerelease and build metadata are irrelevant", () => {
|
||||||
|
const cases = [
|
||||||
|
["4.4.3", "4"],
|
||||||
|
["2.0.0-beta.5", "2"],
|
||||||
|
["19.2.8", "19"],
|
||||||
|
["1.10.3+build.7", "1"],
|
||||||
|
];
|
||||||
|
for (const [input, expected] of cases)
|
||||||
|
assert.equal(majorOf(input), expected, input);
|
||||||
|
for (const bad of ["next", "", undefined, null, "v4.4.3"])
|
||||||
|
assert.equal(majorOf(bad), null, JSON.stringify(bad));
|
||||||
|
});
|
||||||
|
|
||||||
|
test("isSharedSourceFile: individually-named shared files are included (Copilot, #1962)", () => {
|
||||||
|
// `vitest.shared.mts` is root-owned, imported by every client's vitest
|
||||||
|
// config, and already treated as shared by `verify:typecheck-coverage`. It
|
||||||
|
// imports only Node built-ins today, which is why omitting it would go
|
||||||
|
// unnoticed until a third-party import appeared there and skewed.
|
||||||
|
assert.equal(isSharedSourceFile("vitest.shared.mts"), true);
|
||||||
|
// Still anchored: a same-named file nested elsewhere is not the shared one.
|
||||||
|
assert.equal(isSharedSourceFile("clients/web/vitest.shared.mts"), false);
|
||||||
|
});
|
||||||
|
|
|
||||||
|
|
@ -166,9 +166,11 @@ function trackedSourceFiles() {
|
||||||
return out.split("\n").filter(Boolean);
|
return out.split("\n").filter(Boolean);
|
||||||
}
|
}
|
||||||
|
|
||||||
// Vouch for the sibling guards: a guard can't detect being unrun itself, but the
|
// Vouch for the sibling guards: a guard can't detect being unrun itself, so the
|
||||||
// three can each assert the others are still wired into `validate`, so dropping
|
// three form a cycle instead. This one checks BOTH others; each of them checks
|
||||||
// any one is caught here. Only deleting all of them slips through.
|
// only this one. So dropping `verify:typecheck-coverage` or
|
||||||
|
// `verify:dep-lockstep` is caught here, and dropping *this* guard is caught by
|
||||||
|
// either of them. Only removing all three at once slips through.
|
||||||
const rootScripts = JSON.parse(
|
const rootScripts = JSON.parse(
|
||||||
readFileSync(path.join(repoRoot, "package.json"), "utf8"),
|
readFileSync(path.join(repoRoot, "package.json"), "utf8"),
|
||||||
).scripts;
|
).scripts;
|
||||||
|
|
|
||||||
Loading…
Reference in a new issue