chore(deps): reject an unreadable lockfile instead of failing open
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
This commit is contained in:
parent
32c827f089
commit
af63a07817
3 changed files with 137 additions and 14 deletions
|
|
@ -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(
|
||||
{
|
||||
|
|
|
|||
|
|
@ -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));
|
||||
|
|
|
|||
|
|
@ -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"]]) },
|
||||
|
|
|
|||
Loading…
Reference in a new issue