mirror of https://github.com/garrytan/gstack.git
fix(ship): preflight the lockfile parse before any version write
Addresses the partial-write case raised in review on #1862. cmdWrite wrote VERSION, then package.json, and only then called writeLockVersion(), which did its read + JSON.parse internally. A malformed lockfile therefore exited 2 with VERSION and package.json already bumped — three files disagreeing, and no state classify() can name. cmdRepair had the same ordering. Split writeLockVersion() into planLockVersion() (read + parse + prepare the updated content in memory, exit 2 on unreadable/malformed/non-object) and commitLockVersion() (write the prepared plan). Both write and repair now preflight before their first write, so the deterministic failure — a lockfile we can't parse — is caught with nothing written. Exit codes are unchanged: malformed still exits 2, a genuine I/O failure at write time still exits 3. Only the filesystem state on the exit-2 path changes. Full atomicity across three files would need temp-file + rename to close the preflight/write TOCTOU window on permissions; that's out of scope here and the exit-3 message still reports the half-write honestly. The packages[""] sync is now shape-checked rather than truthiness-checked. A corrupt lockfile can put a primitive there, and assigning .version to it throws under ESM strict mode — previously absorbed by the caller's catch as exit 3, but after the split it would escape planLockVersion as an uncaught TypeError. It now degrades to a top-level-only sync, matching how an absent packages[""] (every lockfileVersion 1 file) is already skipped. Also adds the no-lockfile coverage suggested in review (absence is a silent skip on both write and repair, and never creates a lockfile), and corrects the file header, which still claimed write/repair "mutate VERSION + package.json only". Tests: 38 pass across gstack-version-bump + ship-version-sync (was 32). The atomicity and packages-shape tests each fail against the implementation they guard. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
Oliver Harriehausen
parent
ace89ac3e7
commit
0aa7e33ab4
|
|
@ -21,18 +21,22 @@
|
|||
// the caller must handle), exit 2 on bad args / unresolvable base.
|
||||
//
|
||||
// write --version <X.Y.Z.W> [--version-path <p>]
|
||||
// Validates the 4-digit pattern, writes VERSION + package.json.version.
|
||||
// Use for the FRESH bump (or an approved queue rebump). Exit 3 on a
|
||||
// half-write (VERSION written, package.json failed) so the caller knows
|
||||
// drift exists; the next classify() will report DRIFT_STALE_PKG.
|
||||
// Validates the 4-digit pattern, writes VERSION + package.json.version +
|
||||
// package-lock.json.version (when a lockfile is present). Use for the FRESH
|
||||
// bump (or an approved queue rebump). Exit 2 on a lockfile we can't parse —
|
||||
// preflighted, so nothing is written. Exit 3 on a half-write (an earlier
|
||||
// file written, a later one failed) so the caller knows drift exists; the
|
||||
// next classify() will report DRIFT_STALE_PKG.
|
||||
//
|
||||
// repair [--version-path <p>]
|
||||
// DRIFT_STALE_PKG path: sync package.json.version to the current VERSION
|
||||
// file. No bump. Validates the VERSION pattern first.
|
||||
// DRIFT_STALE_PKG path: sync package.json.version (+ the lockfile, when
|
||||
// present) to the current VERSION file. No bump. Validates the VERSION
|
||||
// pattern first.
|
||||
//
|
||||
// Contract: classify NEVER writes. write/repair mutate VERSION + package.json
|
||||
// only. No git mutation, no network. Mirrors gstack-next-version's reader/writer
|
||||
// split so /ship composes them.
|
||||
// Contract: classify NEVER writes. write/repair mutate VERSION + package.json +
|
||||
// package-lock.json only. Parse failures are preflighted before the first write,
|
||||
// so a file we can't read never leaves a partial bump behind. No git mutation, no
|
||||
// network. Mirrors gstack-next-version's reader/writer split so /ship composes them.
|
||||
|
||||
import { existsSync, readFileSync, writeFileSync } from "node:fs";
|
||||
import { execFileSync } from "node:child_process";
|
||||
|
|
@ -101,32 +105,74 @@ function writePkgVersion(cwd: string, version: string): void {
|
|||
writeFileSync(pkgPath, JSON.stringify(parsed, null, 2) + "\n");
|
||||
}
|
||||
|
||||
/** A prepared package-lock.json write, or null when there's no lockfile to sync. */
|
||||
type LockPlan = { path: string; content: string } | null;
|
||||
|
||||
/**
|
||||
* Opportunistically sync package-lock.json (lockfileVersion 3 stores the version
|
||||
* in two places: top-level `version` and `packages[""].version`). Returns true if
|
||||
* the lockfile was present and written, false if absent (yarn/pnpm/bun lockfile,
|
||||
* or no JS at all). Throws on malformed JSON or write failure.
|
||||
* Preflight the package-lock.json sync: read, parse, and prepare the updated
|
||||
* content in memory WITHOUT writing anything. Returns null when the lockfile is
|
||||
* absent (yarn/pnpm/bun lockfile, or no JS at all) — a silent skip, not an error.
|
||||
* Exits 2 on an unreadable/malformed/non-object lockfile.
|
||||
*
|
||||
* Caller decides whether absence is an error — write() and repair() both treat
|
||||
* absence as a silent skip. The state classifier intentionally does NOT inspect
|
||||
* the lockfile: a stale lockfile root version doesn't break `npm ci` or anything
|
||||
* runtime, so lockfile-only drift is opportunistically corrected on FRESH/repair
|
||||
* but is not a halt condition.
|
||||
* Split from the write so the deterministic failure — a lockfile we can't parse —
|
||||
* is caught BEFORE VERSION or package.json are touched. Previously this ran after
|
||||
* both were written, so a malformed lockfile exited 2 having already bumped them,
|
||||
* leaving the three files inconsistent.
|
||||
*
|
||||
* lockfileVersion 3 stores the version in two places: top-level `version` and
|
||||
* `packages[""].version`. Both must move together or `npm ci` warns.
|
||||
*
|
||||
* The state classifier intentionally does NOT inspect the lockfile: a stale
|
||||
* lockfile root version doesn't break `npm ci` or anything runtime, so lockfile-only
|
||||
* drift is opportunistically corrected on FRESH/repair but is not a halt condition.
|
||||
* That non-blocking stance applies to a STALE lockfile, not to a malformed one we
|
||||
* were asked to write.
|
||||
*/
|
||||
function writeLockVersion(cwd: string, version: string): boolean {
|
||||
function planLockVersion(cwd: string, version: string): LockPlan {
|
||||
const lockPath = join(cwd, "package-lock.json");
|
||||
if (!existsSync(lockPath)) return false;
|
||||
const raw = readFileSync(lockPath, "utf-8");
|
||||
if (!existsSync(lockPath)) return null;
|
||||
let raw: string;
|
||||
try {
|
||||
raw = readFileSync(lockPath, "utf-8");
|
||||
} catch {
|
||||
fail("package-lock.json exists but could not be read. Fix the file before re-running /ship.", 2);
|
||||
}
|
||||
let parsed: Record<string, unknown>;
|
||||
try {
|
||||
parsed = JSON.parse(raw) as Record<string, unknown>;
|
||||
} catch {
|
||||
fail("package-lock.json is not valid JSON. Fix the file before re-running /ship.", 2);
|
||||
}
|
||||
// JSON.parse happily returns a number/string/array/null. Assigning .version to a
|
||||
// primitive throws under ESM strict mode, so reject a non-object shape up front.
|
||||
if (typeof parsed !== "object" || parsed === null || Array.isArray(parsed)) {
|
||||
fail("package-lock.json is not a JSON object. Fix the file before re-running /ship.", 2);
|
||||
}
|
||||
parsed.version = version;
|
||||
const packages = parsed.packages as Record<string, { version?: string }> | undefined;
|
||||
if (packages && packages[""]) packages[""].version = version;
|
||||
writeFileSync(lockPath, JSON.stringify(parsed, null, 2) + "\n");
|
||||
// packages[""] is best-effort: absent on lockfileVersion 1, and a corrupt lockfile
|
||||
// can put anything there. A truthy primitive would throw on property assignment
|
||||
// under ESM strict mode, so check the shape rather than just truthiness — skipping
|
||||
// an unusable nested root matches how we already skip an absent one.
|
||||
const packages = parsed.packages;
|
||||
if (packages && typeof packages === "object" && !Array.isArray(packages)) {
|
||||
const root = (packages as Record<string, unknown>)[""];
|
||||
if (root && typeof root === "object" && !Array.isArray(root)) {
|
||||
(root as Record<string, unknown>).version = version;
|
||||
}
|
||||
}
|
||||
return { path: lockPath, content: JSON.stringify(parsed, null, 2) + "\n" };
|
||||
}
|
||||
|
||||
/**
|
||||
* Commit a plan prepared by planLockVersion. Returns true if a lockfile was
|
||||
* written, false when there was none. Throws on I/O failure; the caller maps that
|
||||
* to exit 3 (a genuine half-write we report rather than hide — permissions can
|
||||
* change between preflight and write, and closing that TOCTOU window fully would
|
||||
* need temp-file + rename, which is out of scope for this fix).
|
||||
*/
|
||||
function commitLockVersion(plan: LockPlan): boolean {
|
||||
if (!plan) return false;
|
||||
writeFileSync(plan.path, plan.content);
|
||||
return true;
|
||||
}
|
||||
|
||||
|
|
@ -189,6 +235,9 @@ function cmdWrite(args: string[], cwd: string): void {
|
|||
fail(`NEW_VERSION (${version}) does not match MAJOR.MINOR.PATCH.MICRO. Aborting.`, 2);
|
||||
}
|
||||
const versionPath = resolveVersionPath(cwd, argVal(args, "--version-path"));
|
||||
// Preflight the lockfile before touching anything: a malformed lockfile must
|
||||
// fail with VERSION and package.json untouched, not half-bumped.
|
||||
const lockPlan = planLockVersion(cwd, version!);
|
||||
writeFileSync(versionPath, version + "\n");
|
||||
if (existsSync(join(cwd, "package.json"))) {
|
||||
try {
|
||||
|
|
@ -203,7 +252,7 @@ function cmdWrite(args: string[], cwd: string): void {
|
|||
}
|
||||
let lockfile = false;
|
||||
try {
|
||||
lockfile = writeLockVersion(cwd, version!);
|
||||
lockfile = commitLockVersion(lockPlan);
|
||||
} catch {
|
||||
fail(
|
||||
"failed to update package-lock.json. VERSION and package.json were written but the lockfile is now stale.",
|
||||
|
|
@ -228,6 +277,8 @@ function cmdRepair(args: string[], cwd: string): void {
|
|||
if (!existsSync(join(cwd, "package.json"))) {
|
||||
fail("repair: no package.json to sync.", 2);
|
||||
}
|
||||
// Same preflight as write: parse the lockfile before mutating package.json.
|
||||
const lockPlan = planLockVersion(cwd, current);
|
||||
try {
|
||||
writePkgVersion(cwd, current);
|
||||
} catch {
|
||||
|
|
@ -235,7 +286,7 @@ function cmdRepair(args: string[], cwd: string): void {
|
|||
}
|
||||
let lockfile = false;
|
||||
try {
|
||||
lockfile = writeLockVersion(cwd, current);
|
||||
lockfile = commitLockVersion(lockPlan);
|
||||
} catch {
|
||||
fail("drift repair failed — could not update package-lock.json.", 3);
|
||||
}
|
||||
|
|
|
|||
|
|
@ -146,6 +146,95 @@ describe('package-lock.json sync', () => {
|
|||
expect(code).toBe(2);
|
||||
fs.rmSync(d, { recursive: true, force: true });
|
||||
});
|
||||
|
||||
// The lockfile is parsed in a preflight, before VERSION or package.json are
|
||||
// touched. Without it, a malformed lockfile exits 2 having already bumped both —
|
||||
// the caller is left with three files disagreeing and no state that classify()
|
||||
// can name. These two lock the "nothing written" half of that guarantee.
|
||||
test('malformed package-lock.json leaves VERSION + package.json untouched on write', () => {
|
||||
const d = fs.mkdtempSync(path.join(os.tmpdir(), 'vbump-lock-atomic-write-'));
|
||||
fs.writeFileSync(path.join(d, 'VERSION'), '1.0.0.0\n');
|
||||
fs.writeFileSync(path.join(d, 'package.json'), JSON.stringify({ name: 'x', version: '1.0.0.0' }, null, 2) + '\n');
|
||||
fs.writeFileSync(path.join(d, 'package-lock.json'), '{ not valid json');
|
||||
let code = 0;
|
||||
try { execFileSync('bun', [BIN, 'write', '--version', '1.1.0.0'], { cwd: d, stdio: 'pipe' }); }
|
||||
catch (e: any) { code = e.status; }
|
||||
expect(code).toBe(2);
|
||||
expect(fs.readFileSync(path.join(d, 'VERSION'), 'utf-8').trim()).toBe('1.0.0.0');
|
||||
expect(JSON.parse(fs.readFileSync(path.join(d, 'package.json'), 'utf-8')).version).toBe('1.0.0.0');
|
||||
expect(fs.readFileSync(path.join(d, 'package-lock.json'), 'utf-8')).toBe('{ not valid json');
|
||||
fs.rmSync(d, { recursive: true, force: true });
|
||||
});
|
||||
|
||||
test('malformed package-lock.json leaves package.json untouched on repair', () => {
|
||||
const d = fs.mkdtempSync(path.join(os.tmpdir(), 'vbump-lock-atomic-repair-'));
|
||||
fs.writeFileSync(path.join(d, 'VERSION'), '2.0.0.0\n');
|
||||
fs.writeFileSync(path.join(d, 'package.json'), JSON.stringify({ name: 'x', version: '1.9.0.0' }, null, 2) + '\n');
|
||||
fs.writeFileSync(path.join(d, 'package-lock.json'), '{ not valid json');
|
||||
let code = 0;
|
||||
try { execFileSync('bun', [BIN, 'repair'], { cwd: d, stdio: 'pipe' }); }
|
||||
catch (e: any) { code = e.status; }
|
||||
expect(code).toBe(2);
|
||||
expect(JSON.parse(fs.readFileSync(path.join(d, 'package.json'), 'utf-8')).version).toBe('1.9.0.0');
|
||||
fs.rmSync(d, { recursive: true, force: true });
|
||||
});
|
||||
|
||||
test('non-object package-lock.json fails write with exit 2, nothing written', () => {
|
||||
const d = fs.mkdtempSync(path.join(os.tmpdir(), 'vbump-lock-nonobj-'));
|
||||
fs.writeFileSync(path.join(d, 'VERSION'), '1.0.0.0\n');
|
||||
fs.writeFileSync(path.join(d, 'package.json'), JSON.stringify({ name: 'x', version: '1.0.0.0' }, null, 2) + '\n');
|
||||
// Valid JSON, but not an object — assigning .version to it would throw.
|
||||
fs.writeFileSync(path.join(d, 'package-lock.json'), '[]');
|
||||
let code = 0;
|
||||
try { execFileSync('bun', [BIN, 'write', '--version', '1.1.0.0'], { cwd: d, stdio: 'pipe' }); }
|
||||
catch (e: any) { code = e.status; }
|
||||
expect(code).toBe(2);
|
||||
expect(fs.readFileSync(path.join(d, 'VERSION'), 'utf-8').trim()).toBe('1.0.0.0');
|
||||
fs.rmSync(d, { recursive: true, force: true });
|
||||
});
|
||||
|
||||
// A corrupt lockfile can put anything at packages[""]. A truthy primitive there
|
||||
// throws on property assignment under strict mode; the sync must degrade to
|
||||
// "update what I can" rather than crash out with a stack trace.
|
||||
test('unusable packages[""] degrades to a top-level-only sync, no crash', () => {
|
||||
const d = fs.mkdtempSync(path.join(os.tmpdir(), 'vbump-lock-badpkgs-'));
|
||||
fs.writeFileSync(path.join(d, 'VERSION'), '1.0.0.0\n');
|
||||
fs.writeFileSync(path.join(d, 'package.json'), JSON.stringify({ name: 'x', version: '1.0.0.0' }, null, 2) + '\n');
|
||||
fs.writeFileSync(
|
||||
path.join(d, 'package-lock.json'),
|
||||
JSON.stringify({ name: 'x', version: '1.0.0.0', lockfileVersion: 3, packages: { '': 5 } }, null, 2) + '\n',
|
||||
);
|
||||
const out = execFileSync('bun', [BIN, 'write', '--version', '1.1.0.0'], { cwd: d }).toString();
|
||||
expect(JSON.parse(out)).toEqual({ wrote: '1.1.0.0', packageJson: true, packageLock: true });
|
||||
const lock = JSON.parse(fs.readFileSync(path.join(d, 'package-lock.json'), 'utf-8'));
|
||||
expect(lock.version).toBe('1.1.0.0');
|
||||
expect(lock.packages['']).toBe(5); // left alone, not crashed on
|
||||
fs.rmSync(d, { recursive: true, force: true });
|
||||
});
|
||||
|
||||
// The yarn/pnpm/bun (and no-JS) path: absence is a silent skip, never an error
|
||||
// and never a created file. We only sync a lockfile that already exists; we never
|
||||
// bring one into being.
|
||||
test('write silently skips when no package-lock.json exists', () => {
|
||||
const d = fs.mkdtempSync(path.join(os.tmpdir(), 'vbump-lock-absent-write-'));
|
||||
fs.writeFileSync(path.join(d, 'VERSION'), '1.0.0.0\n');
|
||||
fs.writeFileSync(path.join(d, 'package.json'), JSON.stringify({ name: 'x', version: '1.0.0.0' }, null, 2) + '\n');
|
||||
const out = execFileSync('bun', [BIN, 'write', '--version', '1.1.0.0'], { cwd: d }).toString();
|
||||
expect(JSON.parse(out)).toEqual({ wrote: '1.1.0.0', packageJson: true, packageLock: false });
|
||||
expect(fs.existsSync(path.join(d, 'package-lock.json'))).toBe(false);
|
||||
expect(fs.readFileSync(path.join(d, 'VERSION'), 'utf-8').trim()).toBe('1.1.0.0');
|
||||
fs.rmSync(d, { recursive: true, force: true });
|
||||
});
|
||||
|
||||
test('repair silently skips when no package-lock.json exists', () => {
|
||||
const d = fs.mkdtempSync(path.join(os.tmpdir(), 'vbump-lock-absent-repair-'));
|
||||
fs.writeFileSync(path.join(d, 'VERSION'), '2.0.0.0\n');
|
||||
fs.writeFileSync(path.join(d, 'package.json'), JSON.stringify({ name: 'x', version: '1.9.0.0' }, null, 2) + '\n');
|
||||
const out = execFileSync('bun', [BIN, 'repair'], { cwd: d }).toString();
|
||||
expect(JSON.parse(out)).toEqual({ repaired: '2.0.0.0', packageLock: false });
|
||||
expect(fs.existsSync(path.join(d, 'package-lock.json'))).toBe(false);
|
||||
fs.rmSync(d, { recursive: true, force: true });
|
||||
});
|
||||
});
|
||||
|
||||
describe('classify (idempotency over a real git base)', () => {
|
||||
|
|
|
|||
Loading…
Reference in New Issue