fix(validate): an empty package-lock.json is not a lockfile (#255) - #263
Open
ZacxDev wants to merge 1 commit into
Open
fix(validate): an empty package-lock.json is not a lockfile (#255)#263ZacxDev wants to merge 1 commit into
ZacxDev wants to merge 1 commit into
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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.