Skip to content

fix(validate): an empty package-lock.json is not a lockfile (#255) - #263

Open
ZacxDev wants to merge 1 commit into
mainfrom
fix/255-empty-lockfile
Open

fix(validate): an empty package-lock.json is not a lockfile (#255)#263
ZacxDev wants to merge 1 commit into
mainfrom
fix/255-empty-lockfile

Conversation

@ZacxDev

@ZacxDev ZacxDev commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

Closes #255.

The defect

civitai app validate printed ✓ … is valid and exited 0 for a project whose committed package-lock.json was 0 bytes. The platform build failed anyway.

internal/validate/lockfile.go's regularFileExists was os.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 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.

🔴 And the old message invited the input that defeated it. The missing-lockfile error names the filename the build wants, so touch package-lock.json reads as the fix, produces a green validate, and lands the author in the identical opaque server-side "build failed".

Reproduced with real binaries, base (origin/main 8ed4d69) vs HEAD, on the issue's exact shape:

lockfile body base HEAD
0 bytes ✓ … is valid rc=0 ✗ 1 validation error(s) rc=1
{} (npm refuses this identically) ✓ … is valid rc=0 ✗ 1 validation error(s) rc=1
real lockfile (positive control) rc=0 rc=0
missing (control) the existing message byte-identical

Pre-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, and npm ci / --frozen-lockfile still 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:

  • npm (package-lock.json): parse as JSON and require a numeric lockfileVersion >= 1. That is npm's own precondition, mirrored the way the rest of this file mirrors the build recipe (AGENTS item 3).
  • pnpm (pnpm-lock.yaml) / yarn (yarn.lock): non-empty after a whitespace trim, and nothing more. pnpm-lock.yaml needs 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:

  1. The Lstat/IsRegular gate stays IN FRONT of the read. os.ReadFile follows symlinks, and pkgzip.Build drops non-regular entries from the bundle — reading through a link would vouch for bytes the submitted zip does not carry. TestLockfileSymlinkToAValidLockfileIsStillAbsent pins the order by pointing the link at a valid lockfile, so only a check that reads through it can pass.
  2. This is a FATAL check, so an UNOBSERVABLE state degrades to today's presence-only PASS, never to an error. A read failure or a file over the size cap means we did not look; manufacturing a hard error that blocks a submit out of a gap is the expensive direction (item 18's "reading nothing is not finding nothing", applied to a check that can block).
  3. Only the REQUIRED lockfile's content is judged. A foreign lockfile is evidence of which package manager the project really uses, and that reading does not depend on its bytes — judging it too would stack a second confusing finding on the real one.

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 validate pulls 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 .js took peak RSS to 316 MB (item 18).

The {} decision, stated deliberately

{"…"} with no lockfileVersionfails. npm ci rejects {} with the same EUSAGE as an empty file, so accepting it would leave the headline defect half-open: echo '{}' > package-lock.json substituting for touch. Pinned with a row and a comment in TestLockfileNpmContentRule.

One bug the table caught during development

json.Number is type Number string, so unmarshalling the value straight into one accepts the JSON string "3". {"lockfileVersion": "3"} sailed through the first draft; the fix decodes into any with UseNumber and 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:

package-lock.json is committed but it is EMPTY (0 bytes), so it is not a lockfile the
platform build can install from — it will run `npm ci`, which hard-fails on it exactly as
if nothing were committed. A lockfile is GENERATED by the package manager, never
hand-written and never created with `touch`: delete package-lock.json, run `npm install`,
and commit the package-lock.json it writes. `npm ci` requires a package-lock.json that
parses as JSON and declares a numeric "lockfileVersion" of 1 or more (npm's own words: …).

Field stays FieldProject per item 23 (repository state, not a manifest key), built through newFinding, and pinned by a new bidirectional ledger row in findingFieldLedger() — the sentinel collapse (project)(root) is a mutant that item 23 records as having survived a green suite once.

Mutation matrix

# mutation result
1 Revert the fix — never report a content defect (the pre-#255 predicate) 🔴 RED: 3 top-level + 12 leaf subtests. TestLockfileEmptyNpmLockfileIsFatal fails with "a committed package-lock.json that is not a lockfile must be a hard error, got a clean pass"; 8 rows of TestLockfileNpmContentRule, 4 of TestLockfileNonJSONManagersRejectOnlyEmptiness, plus the size-cap test's own control and TestEveryFindingCarriesItsDocumentedField (the ledger row goes stale)
2 Reject every lockfile (positive control) 🔴 RED: 8 top-level + 25 leaf subtests, including every accepts … row, TestLockfileRealNpmLockfileStillPasses, TestLockfileMatchingLockfilePasses (all 7), TestLockfileMultipleWithRequiredPresentIsWarning, TestLockfileContentOfAForeignLockfileIsNotJudged
3 Unobservable → hard error (drop the degrade) 🔴 RED, and only the two rows that own it: …OverTheSizeCapDegradesToPresenceOnly, …UnreadableDegradesToPresenceOnly
4 os.Lstatos.Stat (read through a symlink) 🔴 RED: TestLockfileSymlinkToAValidLockfileIsStillAbsent + the two pre-existing symlink guards

Mutants 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 ci

rc=0--- FAIL count 0, grep -c 'build failed' 0, test timed out panics 0, 18 packages ok. (Counted from the output, not read off the exit code.)

⚠️ Two files outside my assigned ownership

I own internal/validate/lockfile.go + tests under internal/validate/. Two test-fixture helpers in internal/cmd had to move or CI is red — flagging for conflict resolution:

  • internal/cmd/app_create_cmd_test.gosimulateInstall wrote {} as the lockfile
  • internal/cmd/app_validate_lockfile_test.goscaffoldWithLockfiles wrote {} for every lockfile name

Both are one-helper changes replacing {} with a body an install actually writes. They are not cosmetic: a {} fixture standing in for "the author ran npm install" was asserting that a build-breaking project validates clean, which is precisely how this bug stayed invisible. internal/cmd/cmd_test.go and internal/cmd/app_submit_lockfile_test.go go green through those two helpers with no edit of their own. I did not touch internal/cmd/app_init.go or README.md.

Proposed follow-ups (not done here)

  • A new AGENTS.md item was deliberately NOT added (six parallel agents would collide on the number). I edited item 3 instead, which is what changed. If a maintainer wants this as its own item, the content is the 🔴 block now inside item 3.
  • README.md (~line 691) and internal/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.
  • The two scaffold README templates (page-vite, page-money) say "without a committed package-lock.json the build hard-fails" — still true, now also true of an empty one.

`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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

app validate passes an empty package-lock.json — a green check for a build that cannot succeed

1 participant