From 8e88a60d3aae89f2b0747dfba479341e2f597cc1 Mon Sep 17 00:00:00 2001 From: Olaf Alders Date: Thu, 8 Oct 2026 00:57:42 +0000 Subject: [PATCH 1/2] Fail closed when an unsupported lockfile changes (#133) Warn on stderr for changed pnpm/yarn/bun/shrinkwrap lockfiles (matched by basename) and exit 1 under --fail-on-downgrade, since they can't be verified. Co-Authored-By: Claude Opus 5.5 --- CLAUDE.md | 2 +- README.md | 9 +++++++-- __tests__/cli.test.js | 45 ++++++++++++++++++++++++++++++++++++++++++- bin/diff-lockfiles.js | 29 ++++++++++++++++++++-------- 4 files changed, 73 insertions(+), 12 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 0635e2c..0b8515d 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -28,7 +28,7 @@ Runs on Node 20+ (CI matrix: 20, 22, 24, 26). These span `bin/` + `lib/` and are easy to break inadvertently: - **CLI flow:** the CLI diffs *every* changed `package-lock.json` in the ref range. For each file it `git show`s both sides, `JSON.parse`s them, calls `diff()`, and `print()`s the result with `title: filename` — this is why the formatters take a `title`. The exit code is set once, after all files. -- **Exit-code contract:** `0` = success; `1` = error (bad ref, unparseable lockfile); `2` = a downgrade was found *and* `--fail-on-downgrade` was passed. Output is identical with or without the flag — it only changes the exit code. A "downgrade" requires both sides to be valid semver with the new one lower; adds, removes, and non-semver values never count (see `hasDowngrade`). This drives Dependabot auto-merge gating (see README). +- **Exit-code contract:** `0` = success; `1` = error (bad ref, unparseable lockfile), or an unsupported lockfile type (e.g. `pnpm-lock.yaml`, matched by basename) changed under `--fail-on-downgrade` — always warned on stderr, fail closed; `2` = a downgrade was found *and* `--fail-on-downgrade` was passed. Output is identical with or without the flag — it only changes the exit code. A "downgrade" requires both sides to be valid semver with the new one lower; adds, removes, and non-semver values never count (see `hasDowngrade`). This drives Dependabot auto-merge gating (see README). - **Missing lockfile at a ref** (added/removed between refs) is modeled as `{ packages: {} }`, i.e. all-added or all-removed — not an error. `bin` detects this by string-matching git's "does not exist" stderr, which is why git is spawned with `LC_ALL: 'C'` (a translated locale would break the match). - **`diff()` fails closed:** it throws if either lockfile has no top-level `packages` map (npm lockfileVersion < 2). Returning an empty diff would be fail-open — a downgrade could slip past `--fail-on-downgrade`. An *empty-but-present* `packages` object is the allowed added/removed case above. - **Untrusted input hardening:** package names come from lockfile contents. `diff()` accumulates into `Object.create(null)` so a package literally named `__proto__` stays a data key (there are tests for this). Git is always spawned via `execFile` with an argument array (never a shell) and `--end-of-options`, so a ref beginning with `-` can't be treated as an option or injected. Preserve these when touching `bin/diff-lockfiles.js`. diff --git a/README.md b/README.md index c1782bf..28139d8 100644 --- a/README.md +++ b/README.md @@ -35,7 +35,7 @@ Options: -m, --max-buffer maximum read buffer size -c, --color colorizes certain output formats (default: false) -s, --shallow only include direct dependencies of the project (default: false) - -d, --fail-on-downgrade exit 2 if any package version is decremented (default: false) + -d, --fail-on-downgrade exit 2 if any package version is decremented (1 if an unsupported lockfile changed) (default: false) -h, --help display help for command ``` @@ -49,13 +49,18 @@ so a script or CI job can gate on it: | --- | --- | | `0` | Success, and no package version was decremented | | `2` | At least one package version was decremented | -| `1` | Error (e.g. an invalid git ref or unreadable lockfile) | +| `1` | Error (e.g. an invalid git ref or unreadable lockfile), or an unsupported lockfile changed | Output is unchanged — the downgrade is already visible in the diff — so this flag only adds the exit code. A downgrade means both the old and new values are valid semver and the new one is lower; added, removed, and non-semver entries never count. +Only `package-lock.json` files are diffed. If another lockfile changed +(`pnpm-lock.yaml`, `yarn.lock`, `bun.lock`, `bun.lockb`, `npm-shrinkwrap.json`, +in any directory), a warning naming it goes to stderr, and with +`--fail-on-downgrade` the exit code is `1`, since it can't be verified. + This is useful for auto-merging Dependabot PRs only when the lockfile moves strictly forward. When PR A is merged before PR B, B's lockfile can show a mix of upgrades and downgrades; catch that and ask Dependabot to rebase instead: diff --git a/__tests__/cli.test.js b/__tests__/cli.test.js index a9360c3..d8cb53e 100644 --- a/__tests__/cli.test.js +++ b/__tests__/cli.test.js @@ -1,5 +1,5 @@ import { spawnSync } from 'node:child_process'; -import { existsSync, mkdtempSync, rmSync, unlinkSync, writeFileSync } from 'node:fs'; +import { existsSync, mkdirSync, mkdtempSync, rmSync, unlinkSync, writeFileSync } from 'node:fs'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; import { fileURLToPath } from 'node:url'; @@ -123,3 +123,46 @@ describe('diff-lockfiles CLI added/removed lockfiles', () => { expect(result.stdout).toMatch(/removed/); }); }); + +// A lockfile type the tool can't diff must not pass a --fail-on-downgrade gate +// silently: it can't be verified, so the run fails closed. +describe('diff-lockfiles CLI unsupported lockfiles', () => { + let repo; + const unsupported = join('frontend', 'pnpm-lock.yaml'); + + const git = (...args) => + spawnSync('git', args, { cwd: repo, encoding: 'utf8' }); + + const run = (...args) => + spawnSync('node', [bin, ...args], { cwd: repo, encoding: 'utf8' }); + + beforeAll(() => { + repo = mkdtempSync(join(tmpdir(), 'diff-lockfiles-unsupported-')); + git('init', '-q'); + git('config', 'user.email', 'test@example.com'); + git('config', 'user.name', 'test'); + mkdirSync(join(repo, 'frontend')); + writeFileSync(join(repo, unsupported), 'lockfileVersion: 1\n'); + git('add', '-A'); + git('commit', '-qm', 'base'); + writeFileSync(join(repo, unsupported), 'lockfileVersion: 2\n'); + git('add', '-A'); + git('commit', '-qm', 'change pnpm lockfile'); + }); + + afterAll(() => { + rmSync(repo, { recursive: true, force: true }); + }); + + it('exits 1 with --fail-on-downgrade and names the file on stderr', () => { + const result = run('HEAD~1', 'HEAD', '--fail-on-downgrade'); + expect(result.status).toBe(1); + expect(result.stderr).toContain(unsupported); + }); + + it('exits 0 without the flag but still warns on stderr', () => { + const result = run('HEAD~1', 'HEAD'); + expect(result.status).toBe(0); + expect(result.stderr).toContain(unsupported); + }); +}); diff --git a/bin/diff-lockfiles.js b/bin/diff-lockfiles.js index bb77cf8..bdb6ac0 100755 --- a/bin/diff-lockfiles.js +++ b/bin/diff-lockfiles.js @@ -4,6 +4,7 @@ import { Command, InvalidArgumentError } from 'commander'; import { execFile } from 'node:child_process'; import { promisify } from 'node:util'; import { createRequire } from 'node:module'; +import { basename } from 'node:path'; import { diff, hasDowngrade, print } from '../lib/index.js'; const execFilePromise = promisify(execFile); @@ -16,6 +17,10 @@ const { version } = require('../package.json'); // and a benign added/removed lockfile would surface as a hard error. const gitEnv = { ...process.env, LC_ALL: 'C' }; +const unsupportedLockFiles = new Set([ + 'pnpm-lock.yaml', 'yarn.lock', 'bun.lock', 'bun.lockb', 'npm-shrinkwrap.json', +]); + async function lockFiles(a, b) { // Filter in JS rather than piping through grep: a pipeline masks a failing // `git diff` (e.g. a bad ref) behind grep's own exit code. Letting git's @@ -31,10 +36,11 @@ async function lockFiles(a, b) { if (output.stderr.trim() !== '') { console.error(output.stderr.trim()); } - return output.stdout - .trim() - .split(/\r\n|\r|\n/) - .filter((line) => /package-lock\.json$/.test(line)); + const names = output.stdout.trim().split(/\r\n|\r|\n/); + return { + supported: names.filter((line) => /package-lock\.json$/.test(line)), + unsupported: names.filter((line) => unsupportedLockFiles.has(basename(line))), + }; }; // Return the file's contents at `ref`, or null when the file does not exist @@ -94,12 +100,16 @@ cli .option('-m, --max-buffer ', 'maximum read buffer size in bytes', parseMaxBuffer, 1024 * 10000) .option('-c, --color', 'colorizes certain output formats', false) .option('-s, --shallow', 'only include direct dependencies of the project', false) - .option('-d, --fail-on-downgrade', 'exit 2 if any package version is decremented', false) + .option('-d, --fail-on-downgrade', 'exit 2 if any package version is decremented (1 if an unsupported lockfile changed)', false) .action(async (from, to, options) => { - const filenames = await lockFiles(from, to); + const { supported, unsupported } = await lockFiles(from, to); let downgradeFound = false; - for (const filename of filenames) { + for (const filename of unsupported) { + console.error(`Unsupported lockfile changed, not diffed: ${filename}`); + } + + for (const filename of supported) { const oldLock = parseLock(await lockFileString(options.maxBuffer, from, filename), filename); const newLock = parseLock(await lockFileString(options.maxBuffer, to, filename), filename); const changes = diff(oldLock, newLock, options.shallow); @@ -115,7 +125,10 @@ cli }); } - if (options.failOnDowngrade && downgradeFound) { + // An undiffable lockfile can't be verified, so fail closed rather than pass the gate. + if (options.failOnDowngrade && unsupported.length > 0) { + process.exitCode = 1; + } else if (options.failOnDowngrade && downgradeFound) { process.exitCode = 2; } }) From 93356981c0ec51e8c2795d4f021defdbc33f0dca Mon Sep 17 00:00:00 2001 From: Olaf Alders Date: Thu, 8 Oct 2026 00:59:55 +0000 Subject: [PATCH 2/2] Address review: NUL-delimited git diff, precedence test, docs (#133) Co-Authored-By: Claude Opus 5.5 --- CLAUDE.md | 4 ++-- README.md | 2 +- __tests__/cli.test.js | 48 +++++++++++++++++++++++++++++++++++-------- bin/diff-lockfiles.js | 7 ++++--- 4 files changed, 47 insertions(+), 14 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 0b8515d..b6dfee7 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -27,8 +27,8 @@ Runs on Node 20+ (CI matrix: 20, 22, 24, 26). These span `bin/` + `lib/` and are easy to break inadvertently: -- **CLI flow:** the CLI diffs *every* changed `package-lock.json` in the ref range. For each file it `git show`s both sides, `JSON.parse`s them, calls `diff()`, and `print()`s the result with `title: filename` — this is why the formatters take a `title`. The exit code is set once, after all files. -- **Exit-code contract:** `0` = success; `1` = error (bad ref, unparseable lockfile), or an unsupported lockfile type (e.g. `pnpm-lock.yaml`, matched by basename) changed under `--fail-on-downgrade` — always warned on stderr, fail closed; `2` = a downgrade was found *and* `--fail-on-downgrade` was passed. Output is identical with or without the flag — it only changes the exit code. A "downgrade" requires both sides to be valid semver with the new one lower; adds, removes, and non-semver values never count (see `hasDowngrade`). This drives Dependabot auto-merge gating (see README). +- **CLI flow:** the CLI diffs *every* changed `package-lock.json` in the ref range. For each file it `git show`s both sides, `JSON.parse`s them, calls `diff()`, and `print()`s the result with `title: filename` — this is why the formatters take a `title`. Other lockfile types are warned about and skipped (see Exit-code contract). `git diff` uses `-z` so C-quoted (non-ASCII) paths aren't missed. The exit code is set once, after all files. +- **Exit-code contract:** `0` = success; `1` = error (bad ref, unparseable lockfile), or — with `--fail-on-downgrade` only — an unsupported lockfile type changed (matched by basename, e.g. `pnpm-lock.yaml`; list in `bin/diff-lockfiles.js`). Unsupported lockfiles are warned on stderr regardless of the flag and never diffed. `2` = a downgrade was found *and* `--fail-on-downgrade` was passed (`1` wins if both apply). Output is identical with or without the flag — it only changes the exit code. A "downgrade" requires both sides to be valid semver with the new one lower; adds, removes, and non-semver values never count (see `hasDowngrade`). This drives Dependabot auto-merge gating (see README). - **Missing lockfile at a ref** (added/removed between refs) is modeled as `{ packages: {} }`, i.e. all-added or all-removed — not an error. `bin` detects this by string-matching git's "does not exist" stderr, which is why git is spawned with `LC_ALL: 'C'` (a translated locale would break the match). - **`diff()` fails closed:** it throws if either lockfile has no top-level `packages` map (npm lockfileVersion < 2). Returning an empty diff would be fail-open — a downgrade could slip past `--fail-on-downgrade`. An *empty-but-present* `packages` object is the allowed added/removed case above. - **Untrusted input hardening:** package names come from lockfile contents. `diff()` accumulates into `Object.create(null)` so a package literally named `__proto__` stays a data key (there are tests for this). Git is always spawned via `execFile` with an argument array (never a shell) and `--end-of-options`, so a ref beginning with `-` can't be treated as an option or injected. Preserve these when touching `bin/diff-lockfiles.js`. diff --git a/README.md b/README.md index 28139d8..ced1975 100644 --- a/README.md +++ b/README.md @@ -35,7 +35,7 @@ Options: -m, --max-buffer maximum read buffer size -c, --color colorizes certain output formats (default: false) -s, --shallow only include direct dependencies of the project (default: false) - -d, --fail-on-downgrade exit 2 if any package version is decremented (1 if an unsupported lockfile changed) (default: false) + -d, --fail-on-downgrade exit 2 if any package version is decremented (default: false) -h, --help display help for command ``` diff --git a/__tests__/cli.test.js b/__tests__/cli.test.js index d8cb53e..ef5a98d 100644 --- a/__tests__/cli.test.js +++ b/__tests__/cli.test.js @@ -129,10 +129,22 @@ describe('diff-lockfiles CLI added/removed lockfiles', () => { describe('diff-lockfiles CLI unsupported lockfiles', () => { let repo; const unsupported = join('frontend', 'pnpm-lock.yaml'); + // git C-quotes non-ASCII paths unless -z is used; these must still be matched. + const cafeUnsupported = join('café', 'yarn.lock'); + const cafeLock = join('café', 'package-lock.json'); const git = (...args) => spawnSync('git', args, { cwd: repo, encoding: 'utf8' }); + const commit = (files, tag) => { + for (const [name, contents] of Object.entries(files)) { + writeFileSync(join(repo, name), contents); + } + git('add', '-A'); + git('commit', '-qm', tag); + git('tag', tag); + }; + const run = (...args) => spawnSync('node', [bin, ...args], { cwd: repo, encoding: 'utf8' }); @@ -142,12 +154,16 @@ describe('diff-lockfiles CLI unsupported lockfiles', () => { git('config', 'user.email', 'test@example.com'); git('config', 'user.name', 'test'); mkdirSync(join(repo, 'frontend')); - writeFileSync(join(repo, unsupported), 'lockfileVersion: 1\n'); - git('add', '-A'); - git('commit', '-qm', 'base'); - writeFileSync(join(repo, unsupported), 'lockfileVersion: 2\n'); - git('add', '-A'); - git('commit', '-qm', 'change pnpm lockfile'); + mkdirSync(join(repo, 'café')); + commit({ + [unsupported]: 'lockfileVersion: 1\n', + [cafeUnsupported]: 'v1\n', + [cafeLock]: lockfile('4.17.21'), + 'package-lock.json': lockfile('4.17.21'), + }, 'base'); + commit({ [unsupported]: 'lockfileVersion: 2\n' }, 'pnpm'); + commit({ [cafeUnsupported]: 'v2\n', [cafeLock]: lockfile('4.17.15') }, 'cafe'); + commit({ [unsupported]: 'lockfileVersion: 3\n', 'package-lock.json': lockfile('4.17.11') }, 'mixed'); }); afterAll(() => { @@ -155,14 +171,30 @@ describe('diff-lockfiles CLI unsupported lockfiles', () => { }); it('exits 1 with --fail-on-downgrade and names the file on stderr', () => { - const result = run('HEAD~1', 'HEAD', '--fail-on-downgrade'); + const result = run('base', 'pnpm', '--fail-on-downgrade'); expect(result.status).toBe(1); expect(result.stderr).toContain(unsupported); }); it('exits 0 without the flag but still warns on stderr', () => { - const result = run('HEAD~1', 'HEAD'); + const result = run('base', 'pnpm'); expect(result.status).toBe(0); expect(result.stderr).toContain(unsupported); }); + + it('matches lockfiles under a non-ASCII directory', () => { + const result = run('pnpm', 'cafe', '--fail-on-downgrade', '--format', 'text'); + expect(result.status).toBe(1); + expect(result.stderr).toContain(cafeUnsupported); + expect(result.stdout).toContain(cafeLock); + }); + + it('exits 1, not 2, when a downgrade and an unsupported lockfile both change', () => { + const plain = run('cafe', 'mixed', '--format', 'text'); + const gated = run('cafe', 'mixed', '--format', 'text', '--fail-on-downgrade'); + expect(gated.status).toBe(1); + expect(gated.stdout).toBe(plain.stdout); + expect(plain.stdout).toContain('package-lock.json'); + expect(plain.stdout).toContain('4.17.11'); + }); }); diff --git a/bin/diff-lockfiles.js b/bin/diff-lockfiles.js index bdb6ac0..a3e83d4 100755 --- a/bin/diff-lockfiles.js +++ b/bin/diff-lockfiles.js @@ -31,12 +31,13 @@ async function lockFiles(a, b) { // can never be interpreted as shell syntax (command injection). `--end-of-options` // then stops git from treating a ref that starts with `-` as an option // (e.g. `--output=…`); `--name-only` must precede the marker to stay an option. + // `-z` emits raw NUL-delimited paths; otherwise git C-quotes non-ASCII names and they'd go unmatched. const output = await execFilePromise( - 'git', ['diff', '--name-only', '--end-of-options', a, b], { env: gitEnv }); + 'git', ['diff', '--name-only', '-z', '--end-of-options', a, b], { env: gitEnv }); if (output.stderr.trim() !== '') { console.error(output.stderr.trim()); } - const names = output.stdout.trim().split(/\r\n|\r|\n/); + const names = output.stdout.split('\0').filter((name) => name !== ''); return { supported: names.filter((line) => /package-lock\.json$/.test(line)), unsupported: names.filter((line) => unsupportedLockFiles.has(basename(line))), @@ -100,7 +101,7 @@ cli .option('-m, --max-buffer ', 'maximum read buffer size in bytes', parseMaxBuffer, 1024 * 10000) .option('-c, --color', 'colorizes certain output formats', false) .option('-s, --shallow', 'only include direct dependencies of the project', false) - .option('-d, --fail-on-downgrade', 'exit 2 if any package version is decremented (1 if an unsupported lockfile changed)', false) + .option('-d, --fail-on-downgrade', 'exit 2 if any package version is decremented', false) .action(async (from, to, options) => { const { supported, unsupported } = await lockFiles(from, to); let downgradeFound = false;