fix(validate): an empty package-lock.json is not a lockfile (#255) - #263
Conversation
`civitai app validate` printed `✓ … is valid` and exited 0 for a project
whose committed `package-lock.json` was 0 bytes, and the platform build
failed anyway. `regularFileExists` was `os.Lstat` + `IsRegular` and read
nothing, so the check asked whether the file EXISTS, not whether it is a
lockfile. Measured on npm 11.17.0: `npm ci` over an empty package-lock.json
dies with EUSAGE — "can only install with an existing package-lock.json or
npm-shrinkwrap.json with lockfileVersion >= 1" — the same class of failure
as a missing one.
Worse, the missing-lockfile message names the filename, which makes
`touch package-lock.json` a natural and silently-wrong response: the check
invited the input that defeated it. So the exists-but-invalid case gets its
own message, which says the file is there, says what is wrong with it, and
says a lockfile is GENERATED rather than created by hand.
The content rule is PER-MANAGER and deliberately asymmetric:
- npm (package-lock.json): parse as JSON and require a NUMERIC
`lockfileVersion` >= 1 — npm's own precondition, mirrored the way the
rest of this file mirrors the build recipe.
- pnpm / yarn: non-empty after a whitespace trim, and nothing more.
`pnpm-lock.yaml` needs a YAML parser (a new dependency, "ask first")
and a yarn v1 `yarn.lock` carries no version key at all.
Three properties are load-bearing rather than incidental:
- The `Lstat`/`IsRegular` gate stays IN FRONT of the read. `os.ReadFile`
follows symlinks and `pkgzip.Build` drops non-regular entries from the
bundle, so reading through a link would vouch for bytes the submitted
zip does not carry.
- This is a FATAL check, so an UNOBSERVABLE state (read error, or a file
over the 64 MiB cap) degrades to the old presence-only PASS. Blocking a
submit on a gap is the expensive direction.
- Only the REQUIRED lockfile's content is judged; a foreign one is
evidence of which package manager the project uses, and that reading
does not depend on its bytes.
This does not change the SCOPE note: still not a freshness check, still
never runs a package manager. An empty file is not a freshness question —
it is "not a lockfile at all".
Fixtures that wrote the literal `{}` as a stand-in for "the author ran the
install" were wrong about the platform in the direction that hid this bug
(`npm ci` refuses `{}` identically), and now carry a body an install writes.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…m claims Audit follow-ups on #263. All npm behaviour below re-measured independently on npm 11.17.0 (node v26.5.0), offline, against BOTH a zero-dependency project and one with a real local-tarball dependency. 1. 🔴 A UTF-8 BOM was a hard-blocking FALSE POSITIVE. npm's parser tolerates one; Go's encoding/json does not. Measured: a real package-lock.json prefixed with EF BB BF installs cleanly (`npm ci` rc 0, node_modules populated) while validate reported "does not parse as a JSON object" and exited 1 — and because the finding is fatal it blocked `app submit` too (app_submit.go gates on res.OK()). Stripped before parsing. A file holding only a BOM still fails, which is what npm does. 2. The "npm ci refuses {} with the SAME EUSAGE as an empty file" claim was wrong, and it had been replicated into AGENTS.md plus four test comments. Measured, with a real dependency, npm splits into TWO failures: - empty / whitespace / bare BOM / YAML / garbage -> "can only install with an existing package-lock.json or npm-shrinkwrap.json with lockfileVersion >= 1" - {} / no version / array / string version / version 0 PARSE, clear that gate, and fail the sync check: "…are in sync… Missing: <pkg> from lock file" Both rc 1, so every verdict stands, but the message no longer says "exactly as if nothing were committed" and all six sites now state the measured story. Residual, now documented rather than unsaid: on a ZERO-dependency project `npm ci` SUCCEEDS (rc 0) over {}, a version-less object, an array, a string version and version 0. The CLI still refuses them — npm writes none of them, and accepting {} reopens the headline defect with `echo '{}' >` in place of `touch`. 3. The size cap was unpinned and its guard SKIPPED itself: raising maxLockfileBytes to 1<<62 left the suite green, because f.Truncate failed and t.Skipf read as a pass. The constant is now asserted directly and the sparse-file helper FAILS instead of skipping. 4. The cap is not the memory ceiling — it is ~2.2x the cap. Measured peak RSS, 3 runs each, on REALISTIC lockfiles (real packages entries with resolved URLs and sha512 integrity hashes; a whitespace-padded fixture measures nothing): 66 MB / 232,595 entries costs 146.9-147.3 MB vs 17.9-18.0 MB at base; 10 MB costs 37.4-37.7 MB; a 73 MB file (over the cap) costs the same as base, which is what proves the cap is applied before the read. 5. README and `civitai app validate --help` described the check as presence-only; both now state the content rule and the unobservable fallback. 6. `num.Float64()` overflow reported "below 1", which is wrong if it ever fires; it gets its own clause. And the message no longer quotes npm's EUSAGE sentence naming npm-shrinkwrap.json, because this CLI does not recognise that filename — `npm ci` installs from a shrinkwrap (measured rc 0) while validate calls it missing. That gap is pre-existing and now recorded. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Audit follow-ups — all six addressedAll npm behaviour re-measured independently on npm 11.17.0 / node v26.5.0, offline, against both a zero-dependency project and one with a real (local-tarball) dependency. Delta for re-audit:
🔴 1 — BOMConfirmed both halves myself. 🟡 2 — the false claimReproduced your split precisely. With a dependency: empty / whitespace / bare-BOM / YAML / garbage → 🟡 4 — memory, re-measured on my own fixtures3 runs each, realistic lockfiles (real
2.23× the cap. Table + fixture shape now in the code comment and AGENTS.md. 🟢 7 — one correction to your findingMy first shrinkwrap measurement contradicted you (rc 1, nothing installed). That was my script's bug — I copied the project directory while Mutation matrix, fully re-measured (checksum-gated)
M11 is a genuine equivalent mutant, not a coverage gap: these bytes feed only a decoder asked for one key, so a U+FEFF inside a string value cannot change whether Gate
End-to-end, fresh binary
|
…ce claim
Delta-audit follow-ups. Claim 6 of the previous round was REFUTED; every npm
behaviour below was re-measured independently on npm 11.17.0 / node v26.5.0,
offline, against a project with a real local-tarball dependency.
F1 — "strip-anywhere ReplaceAll is an EQUIVALENT mutant" was WRONG, and
recording it was the more dangerous half of the mistake: a recorded measurement
in this repo gets trusted instead of re-derived, so the note would have waved
through exactly the simplification that breaks it. The retracted reasoning was
scoped to a BOM inside a string VALUE — the one position where the two agree.
Measured, strip-anywhere ACCEPTS two shapes npm refuses and the run-strip
rejects: a BOM in a structural slot ("lockfileVersion"<BOM>:) and one straight
after the opening brace (both npm rc 1). Fixtures added; the mutant now dies.
F2 — the one-BOM TrimPrefix introduced a NEW false positive. Measured, npm
tolerates exactly ONE OR TWO leading BOMs (rc 0, node_modules populated) and
rejects 3+. So a DOUBLE-BOM lockfile was reported "does not parse as a JSON
object" — a fatal finding that also blocks `app submit`, on a project that
builds: the same class as the bug the strip was added to fix, one BOM further
out. Now strips a leading RUN, which closes F2 and is immune to F1's interior
hazard by construction. Its one cost is a knowing false negative at 3+ BOMs,
pinned as a fixture so it stays a choice.
F3 — "a numeric lockfileVersion >= 1 (what `npm ci` itself requires)" is npm's
STATED precondition, not its measured behaviour. Measured, editing only that key
on a real in-sync lockfile: 0, "3", null, -5, 1e999 AND THE KEY REMOVED
ENTIRELY all install fine (rc 0; several print `npm warn old lockfile`). npm's
lockfileVersion >= 1 EUSAGE fires only when the file fails to LOAD. So the CLI
is stricter than npm on any project whose lockfile is otherwise intact — a third
population, not just dependency-free ones. The rule is kept (npm writes none of
those shapes) but is now described as OURS rather than as a mirror, in
lockfile.go, AGENTS.md item 3 and the README.
F4 — the "not a number this tool can read" clause had zero coverage and was
wrong for -1e999, which is provably below 1. strconv returns ±Inf on ERANGE, so
the error is now ignored and the value ordered directly: -1e999 is "below 1",
1e999 is accepted (matching npm, rc 0). Both directions covered.
F5 — the cap comparison was unpinned: `>` -> `>=` survived, because the tests
used 4096 and cap+1 but never exactly the cap. Added a boundary fixture.
F6 — restoring "exactly as if nothing were committed" reddened nothing, while
its sibling correction was pinned. Added to the forbidden-wording assertion.
Mutation matrix re-measured, 15/15 killed (was 14/15, M02 surviving).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Delta re-audit round 2 — F1–F6 addressed, and you were right on all threeNew HEAD 🟡 F1 — the equivalence claim is retracted. Fixed.Reproduced your table. The retracted note reasoned over a BOM inside a string value — the one position the two strategies agree on, which is exactly how it reached the wrong conclusion. Measured, strip-anywhere accepts shapes npm refuses:
The two REJECT rows are now fixtures and M02 dies on them. Your point that recording the false equivalence was the damage mechanism is the part I've written into the code and AGENTS.md, not just the correction. 🟡 F2 — my fix introduced a double-BOM false positive. Fixed.Confirmed: npm tolerates exactly one or two leading BOMs (rc 0, * The 3+ row is a knowing false negative, pinned as a fixture with its rationale so it stays a choice: mirroring npm's limit of two means vendoring a magic number for a shape nobody has measured in the wild. 🟡 F3 — the overclaim is corrected, and it is wider than reported. Fixed.Editing only the version key of a real in-sync lockfile, project with a dependency: 🟢 F4–F6 — all taken
Mutation matrix — 15/15 killed (was 14/15)
GatesMerged tree ( Branch: End-to-end, fresh binary, CLI verdict vs measured |
Closes #255.
The defect
civitai app validateprinted✓ … is validand exited 0 for a project whose committedpackage-lock.jsonwas 0 bytes. The platform build failed anyway.internal/validate/lockfile.go'sregularFileExistswasos.Lstat+Mode().IsRegular()and read nothing, so the check asked whether the file exists, not whether it is a lockfile. Measured on npm 11.17.0,npm ciover an emptypackage-lock.jsondies withEUSAGE— "can only install with an existing package-lock.json or npm-shrinkwrap.json with lockfileVersion >= 1" — the same class of failure as a missing one.🔴 And the old message invited the input that defeated it. The missing-lockfile error names the filename the build wants, so
touch package-lock.jsonreads as the fix, produces a greenvalidate, and lands the author in the identical opaque server-side "build failed".Reproduced with real binaries, base (
origin/main8ed4d69) vs HEAD, on the issue's exact shape:✓ … is validrc=0✗ 1 validation error(s)rc=1{}(npm refuses this identically)✓ … is validrc=0✗ 1 validation error(s)rc=1✓rc=0✓rc=0Pre-empting the obvious objection
lockfile.go's SCOPE comment says this is deliberately a presence check and not a freshness check. That stands and is not changed here — the check still never runs a package manager, andnpm ci/--frozen-lockfilestill catch a stale lockfile server-side. An empty file is not a freshness question: it is "not a lockfile at all". The scope that widened is "presence of the FILE" → "presence of a LOCKFILE", which is the thing the platform recipe actually requires.The rule
Per-manager, and deliberately asymmetric — no new dependency:
package-lock.json): parse as JSON and require a numericlockfileVersion >= 1. That is npm's own precondition, mirrored the way the rest of this file mirrors the build recipe (AGENTS item 3).pnpm-lock.yaml) / yarn (yarn.lock): non-empty after a whitespace trim, and nothing more.pnpm-lock.yamlneeds a YAML parser (a new third-party dependency — "ask first" under Permission boundaries) and a yarn v1 lockfile has no version key at all. "Not empty" is the whole of what can be said without inventing authority, and it is exactly the reported defect.Three properties are load-bearing rather than incidental:
Lstat/IsRegulargate stays IN FRONT of the read.os.ReadFilefollows symlinks, andpkgzip.Builddrops non-regular entries from the bundle — reading through a link would vouch for bytes the submitted zip does not carry.TestLockfileSymlinkToAValidLockfileIsStillAbsentpins the order by pointing the link at a valid lockfile, so only a check that reads through it can pass.Size cap: 64 MiB. Real lockfiles are kilobytes to a few megabytes (a large npm monorepo lock is single-digit MB), so 64 MiB is ~2 orders of magnitude above anything a package manager writes and cannot be reached by a genuine lockfile — while still bounding what
validatepulls into memory for a file whose only job is to be checked for one key. This package has been here before: before the ready-ack scan grew caps, one 88 MB.jstook peak RSS to 316 MB (item 18).The
{}decision, stated deliberately{"…"}with nolockfileVersion→ fails.npm cirejects{}with the same EUSAGE as an empty file, so accepting it would leave the headline defect half-open:echo '{}' > package-lock.jsonsubstituting fortouch. Pinned with a row and a comment inTestLockfileNpmContentRule.One bug the table caught during development
json.Numberistype Number string, so unmarshalling the value straight into one accepts the JSON string"3".{"lockfileVersion": "3"}sailed through the first draft; the fix decodes intoanywithUseNumberand type-asserts. Documented at the site.Message
The exists-but-invalid case gets its own message, and the tests assert it does not reuse the missing-lockfile wording:
Field stays
FieldProjectper item 23 (repository state, not a manifest key), built throughnewFinding, and pinned by a new bidirectional ledger row infindingFieldLedger()— the sentinel collapse(project)→(root)is a mutant that item 23 records as having survived a green suite once.Mutation matrix
TestLockfileEmptyNpmLockfileIsFatalfails with "a committed package-lock.json that is not a lockfile must be a hard error, got a clean pass"; 8 rows ofTestLockfileNpmContentRule, 4 ofTestLockfileNonJSONManagersRejectOnlyEmptiness, plus the size-cap test's own control andTestEveryFindingCarriesItsDocumentedField(the ledger row goes stale)accepts …row,TestLockfileRealNpmLockfileStillPasses,TestLockfileMatchingLockfilePasses(all 7),TestLockfileMultipleWithRequiredPresentIsWarning,TestLockfileContentOfAForeignLockfileIsNotJudged…OverTheSizeCapDegradesToPresenceOnly,…UnreadableDegradesToPresenceOnlyos.Lstat→os.Stat(read through a symlink)TestLockfileSymlinkToAValidLockfileIsStillAbsent+ the two pre-existing symlink guardsMutants 1 and 2 are the required both-directions pair: neither battery is satisfiable by the other's fix. Mutant 3 is targeted enough to prove the degrade is its own contract rather than a side effect.
make circ=0—--- FAILcount 0,grep -c 'build failed'0,test timed outpanics 0, 18 packagesok. (Counted from the output, not read off the exit code.)I own
internal/validate/lockfile.go+ tests underinternal/validate/. Two test-fixture helpers ininternal/cmdhad to move or CI is red — flagging for conflict resolution:internal/cmd/app_create_cmd_test.go—simulateInstallwrote{}as the lockfileinternal/cmd/app_validate_lockfile_test.go—scaffoldWithLockfileswrote{}for every lockfile nameBoth are one-helper changes replacing
{}with a body an install actually writes. They are not cosmetic: a{}fixture standing in for "the author rannpm install" was asserting that a build-breaking project validates clean, which is precisely how this bug stayed invisible.internal/cmd/cmd_test.goandinternal/cmd/app_submit_lockfile_test.gogo green through those two helpers with no edit of their own. I did not touchinternal/cmd/app_init.goorREADME.md.Proposed follow-ups (not done here)
README.md(~line 691) andinternal/cmd/app_validate.go's long help both describe the lockfile check as presence-only. Both are owned by other agents in this batch and are left alone; they want a sentence about the content rule.page-vite,page-money) say "without a committedpackage-lock.jsonthe build hard-fails" — still true, now also true of an empty one.