diff --git a/scripts/verify-dep-lockstep.main.test.mjs b/scripts/verify-dep-lockstep.main.test.mjs index 6321d24e..50db6b99 100644 --- a/scripts/verify-dep-lockstep.main.test.mjs +++ b/scripts/verify-dep-lockstep.main.test.mjs @@ -37,14 +37,18 @@ const typescriptDir = path.dirname( ), ); +/** A lockfileVersion 3 lockfile, including the `""` root entry npm always writes. */ const lock = (deps) => ({ lockfileVersion: 3, - packages: Object.fromEntries( - Object.entries(deps).map(([name, version]) => [ - `node_modules/${name}`, - { version }, - ]), - ), + packages: { + "": { name: "fixture" }, + ...Object.fromEntries( + Object.entries(deps).map(([name, version]) => [ + `node_modules/${name}`, + { version }, + ]), + ), + }, }); /** @@ -52,7 +56,7 @@ const lock = (deps) => ({ * install and one client install, and the guard itself. `rootDeps`/`webDeps` * decide whether the two installs agree. */ -function makeFixture({ rootDeps, webDeps, scripts }) { +function makeFixture({ rootDeps, webDeps, scripts, rawWebLock }) { // realpath matters: on macOS `tmpdir()` is `/var/...`, a symlink to // `/private/var/...`. The guard only runs `main()` when `import.meta.url` // (always the resolved path) matches `process.argv[1]`, so launching it via @@ -88,7 +92,7 @@ function makeFixture({ rootDeps, webDeps, scripts }) { ); write("package-lock.json", lock(rootDeps)); write("clients/web/package.json", { name: "web" }); - write("clients/web/package-lock.json", lock(webDeps)); + write("clients/web/package-lock.json", rawWebLock ?? lock(webDeps)); // The guard resolves its repo root from its own location, so it has to live // inside the fixture; `lib/npm-scripts.mjs` comes along as its import. @@ -182,6 +186,48 @@ test("main: exits 1 when a configured shared source matches no file", () => { }); }); +test("main: exits 1 on a lockfile it cannot read, rather than failing open (Copilot, #1962)", () => { + // A v1 lockfile has `dependencies` and no `packages` table. Treating it as an + // install that simply holds nothing would leave the *root's* zod unopposed + // and the skew below reported as aligned — the gate failing open. + withFixture( + { + rootDeps: { ...ALIGNED, zod: "4.3.6" }, + webDeps: ALIGNED, + rawWebLock: { + lockfileVersion: 1, + dependencies: { zod: { version: "4.4.3" } }, + }, + }, + (dir) => { + const { status, out } = runGuard(dir); + assert.equal(status, 1, out); + assert.match(out, /not in a readable format/); + assert.match(out, /clients\/web\/package-lock\.json/); + }, + ); +}); + +test("main: exits 1 on a lockfile with a `packages` table but no root entry", () => { + // The shape check is not just "has a packages key" — a table without the + // `""` root npm always writes is not a lockfile this guard can trust. + withFixture( + { + rootDeps: ALIGNED, + webDeps: ALIGNED, + rawWebLock: { + lockfileVersion: 3, + packages: { "node_modules/zod": { version: "4.4.3" } }, + }, + }, + (dir) => { + const { status, out } = runGuard(dir); + assert.equal(status, 1, out); + assert.match(out, /not in a readable format/); + }, + ); +}); + test("main: exits 1 when the root validate no longer runs the sibling guard", () => { withFixture( { diff --git a/scripts/verify-dep-lockstep.mjs b/scripts/verify-dep-lockstep.mjs index de4cccf0..ab928835 100644 --- a/scripts/verify-dep-lockstep.mjs +++ b/scripts/verify-dep-lockstep.mjs @@ -209,6 +209,25 @@ export function topLevelLockVersions(lock) { return versions; } +/** + * Whether a parsed lockfile has the shape this guard can read: a + * `lockfileVersion` 2+ `packages` table, keyed by install path with `""` for + * the root project. + * + * This is checked rather than tolerated because the gate is deny-by-default and + * `topLevelLockVersions` returns an empty map for anything else. An unreadable + * lockfile would otherwise contribute no holders, and a real skew among the + * remaining installs would be reported as aligned — the gate failing *open*, + * which is the one way it must never fail (Copilot, #1962). A v1 lockfile + * (`dependencies` only, no `packages`) lands here too, correctly: this guard + * cannot read it, so it must say so rather than skip the install. + */ +export function hasReadableLockShape(lock) { + const packages = lock?.packages; + if (typeof packages !== "object" || packages === null) return false; + return Object.prototype.hasOwnProperty.call(packages, ""); +} + /** * Find candidate packages that resolve to more than one version across the * installs. `installs` is an array of `{ dir, versions }`. Returns one entry per @@ -375,13 +394,42 @@ export function main() { } const dirs = installDirs(); - const installs = dirs.map((dir) => ({ + const locks = dirs.map((dir) => { + const file = path.join(repoRoot, dir, "package-lock.json"); + let lock; + try { + lock = JSON.parse(readFileSync(file, "utf8")); + } catch (cause) { + // Unparseable is the same failure as unreadable — say which file, rather + // than dying on a raw SyntaxError with no path in it. + throw new Error( + `verify:dep-lockstep — could not parse ${dir}/package-lock.json.`, + { cause }, + ); + } + return { dir, lock }; + }); + + // Refuse to compare against a lockfile whose shape we can't read, instead of + // treating it as an install that holds nothing — see `hasReadableLockShape`. + const unreadable = locks.filter(({ lock }) => !hasReadableLockShape(lock)); + if (unreadable.length > 0) { + console.error( + `verify:dep-lockstep — ${unreadable.length} lockfile(s) are not in a readable format:\n`, + ); + for (const { dir } of unreadable) + console.error(` ${dir}/package-lock.json`); + console.error( + "\nThis guard reads the `packages` table of a lockfileVersion 2+ lockfile. Without it the install" + + "\ncontributes no versions, so a real skew among the others would be reported as aligned — the gate" + + "\nfailing open. Regenerate the lockfile with a current npm (`npm install`).", + ); + process.exit(1); + } + + const installs = locks.map(({ dir, lock }) => ({ dir, - versions: topLevelLockVersions( - JSON.parse( - readFileSync(path.join(repoRoot, dir, "package-lock.json"), "utf8"), - ), - ), + versions: topLevelLockVersions(lock), })); const { failures, ignored } = partitionSkew(findSkew(candidates, installs)); diff --git a/scripts/verify-dep-lockstep.test.mjs b/scripts/verify-dep-lockstep.test.mjs index 89268ee4..79974404 100644 --- a/scripts/verify-dep-lockstep.test.mjs +++ b/scripts/verify-dep-lockstep.test.mjs @@ -8,6 +8,7 @@ import { test } from "node:test"; import assert from "node:assert/strict"; import { findSkew, + hasReadableLockShape, importedPackageNames, isSharedSourceFile, majorOf, @@ -243,10 +244,38 @@ test("topLevelLockVersions: nested duplicates are ignored", () => { }); test("topLevelLockVersions: a malformed or empty lockfile yields nothing", () => { + // Safe as a pure helper *because* `hasReadableLockShape` rejects these before + // any comparison — an empty map reaching `findSkew` is the fail-open path. for (const lock of [undefined, null, {}, { packages: {} }]) assert.equal(topLevelLockVersions(lock).size, 0); }); +test("hasReadableLockShape: only a v2+ packages table with a root entry (Copilot, #1962)", () => { + assert.equal( + hasReadableLockShape({ lockfileVersion: 3, packages: { "": {} } }), + true, + ); + assert.equal( + hasReadableLockShape({ + lockfileVersion: 2, + packages: { "": {}, "node_modules/zod": { version: "4.4.3" } }, + }), + true, + ); + const rejected = [ + undefined, + null, + {}, + { packages: null }, + { packages: [] }, // an array has no `""` key + { packages: {} }, // no root entry + { packages: { "node_modules/zod": { version: "4.4.3" } } }, + { lockfileVersion: 1, dependencies: { zod: { version: "4.4.3" } } }, + ]; + for (const lock of rejected) + assert.equal(hasReadableLockShape(lock), false, JSON.stringify(lock)); +}); + test("findSkew: reports a package held at two versions", () => { const installs = [ { dir: ".", versions: new Map([["zod", "4.3.6"]]) },