diff --git a/AGENTS.md b/AGENTS.md index 5d180ed..f26b1fc 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -220,6 +220,44 @@ neither one's. schema's `allOf` requires it whenever `buildCommand` is set, so a remedy that omits it walks the author into a second failure. It fires only when `package.json` exists; static blocks never install and must never be flagged. + 🔴 **IT IS A PRESENCE CHECK ABOUT THE FILE, NOT ABOUT ITS BYTES — AND THAT IS + WHY IT NOW READS THE REQUIRED LOCKFILE (issue #255).** The old check was + `os.Lstat` + `IsRegular` and nothing else, so a **0-byte** + `package-lock.json` printed `✓ … is valid` and exited 0 while the platform + build failed anyway: 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, so **the check invited the input that defeated it**: + `touch package-lock.json` reads as the fix. The SCOPE note above still + stands — this is not a *freshness* check and never runs a package manager; + an empty file is not a freshness question, it is "not a lockfile at all". + Three things about `lockfileContentDefect` are decisions, not details. + (a) **The rule is PER-MANAGER and deliberately asymmetric.** npm: parse as + JSON and require a **numeric** `lockfileVersion >= 1` — npm's own + precondition, mirrored the way the rest of this file mirrors the recipe; + note that unmarshalling the value straight into a `json.Number` does NOT + discriminate (it is `type Number string`, so the JSON string `"3"` decodes + into one), hence the `any` + `UseNumber` type assertion. pnpm and yarn: + **non-empty after a whitespace trim and nothing more** — `pnpm-lock.yaml` + needs a YAML parser (a new dependency, "ask first" below) and a yarn v1 + `yarn.lock` carries no version key at all, so "not empty" is the whole of + what can be said without inventing authority. + (b) **The `Lstat`/`IsRegular` gate stays IN FRONT of the read**, because + `os.ReadFile` FOLLOWS symlinks and a symlinked lockfile is dropped from the + bundle by `pkgzip.Build` — reading through one would vouch for bytes the + submitted zip does not carry. `TestLockfileSymlinkToAValidLockfileIsStillAbsent` + pins the ORDER by pointing the link at a *valid* lockfile. + (c) 🔴 **This is a FATAL check, so an UNOBSERVABLE state degrades to the old + presence-only PASS, never to an error** — a read failure or a file over the + 64 MiB cap means we did not look, and manufacturing a hard error that blocks + a submit out of a gap is the expensive direction (item 18's doctrine applied + to a check that can block). 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. And the empty/invalid case + has its own message that says the file EXISTS and that a lockfile is + GENERATED rather than hand-written — reusing the missing-lockfile wording + would re-invite `touch`. 4. **The CLI does NOT vendor the server's token-scope bitmask — and shouldn't.** The `whoami` / `dev-token` "can spend Buzz" capability check decodes the JWT `scopes` (a **string array**) and looks for the `ai:write:budgeted` scope diff --git a/internal/cmd/app_create_cmd_test.go b/internal/cmd/app_create_cmd_test.go index 43083d5..d20ac64 100644 --- a/internal/cmd/app_create_cmd_test.go +++ b/internal/cmd/app_create_cmd_test.go @@ -19,7 +19,13 @@ func simulateInstall(t *testing.T, dir string) { if _, err := os.Stat(filepath.Join(dir, "package.json")); err != nil { return // static template: no install step, no lockfile } - if err := os.WriteFile(filepath.Join(dir, "package-lock.json"), []byte("{}\n"), 0o600); err != nil { + // The body must be what `npm install` WRITES. It used to be `{}`, and that was + // wrong about the platform in the direction that hid issue #255: `npm ci` + // refuses `{}` with the same EUSAGE ("lockfileVersion >= 1") as an empty file, + // so validate now rejects it too and a `{}` fixture would be standing in for + // an install that did not happen. + if err := os.WriteFile(filepath.Join(dir, "package-lock.json"), + []byte(`{"name":"scaffold","version":"0.1.0","lockfileVersion":3,"requires":true,"packages":{}}`+"\n"), 0o600); err != nil { t.Fatalf("write package-lock.json: %v", err) } } diff --git a/internal/cmd/app_validate_lockfile_test.go b/internal/cmd/app_validate_lockfile_test.go index d04b35b..10c011e 100644 --- a/internal/cmd/app_validate_lockfile_test.go +++ b/internal/cmd/app_validate_lockfile_test.go @@ -19,13 +19,31 @@ func scaffoldWithLockfiles(t *testing.T, lockfiles ...string) string { } dir := filepath.Join(tmp, "lock-block") for _, name := range lockfiles { - if err := os.WriteFile(filepath.Join(dir, name), []byte("{}\n"), 0o600); err != nil { + if err := os.WriteFile(filepath.Join(dir, name), []byte(lockfileBodyFor(name)), 0o600); err != nil { t.Fatal(err) } } return dir } +// lockfileBodyFor returns a body the named package manager would actually WRITE. +// +// These fixtures were all the literal `{}`, which is not a lockfile: measured on +// npm 11.17.0, `npm ci` refuses `{}` with the same EUSAGE ("lockfileVersion >= +// 1") as an empty file, and validate now says so too (issue #255). A `{}` body +// under `package-lock.json` would make the PASS-expecting tests below assert that +// a build-breaking project validates clean. +func lockfileBodyFor(name string) string { + switch name { + case "package-lock.json": + return `{"name":"lock-block","version":"0.1.0","lockfileVersion":3,"requires":true,"packages":{}}` + "\n" + case "pnpm-lock.yaml": + return "lockfileVersion: '9.0'\n" + default: + return "# yarn lockfile v1\n" + } +} + // The headline case, end to end through the CLI: a pnpm lockfile under the // scaffold's `npm run build`. It must fail (non-zero exit), and the message must // name the committed lockfile, the install the platform will actually run, and diff --git a/internal/validate/finding_contract_test.go b/internal/validate/finding_contract_test.go index 354ad6d..e20a94b 100644 --- a/internal/validate/finding_contract_test.go +++ b/internal/validate/finding_contract_test.go @@ -1029,14 +1029,27 @@ func fieldCoverageCorpus() []fieldFixture { }, }, { + // The required lockfile's body must be a REAL one here, or this + // fixture trips the exists-but-invalid error instead and stops + // reaching extraLockfileWarning at all — a fixture that silently + // changes which site it exercises is the shape the reachability + // ledger exists to catch. name: "two lockfiles committed (lockfileChecks, advisory)", files: map[string]string{ "block.manifest.json": goodBase, "package.json": `{"name":"cov-block","private":true}`, - "package-lock.json": "{}\n", + "package-lock.json": npmLockBody, "pnpm-lock.yaml": "lockfileVersion: '9.0'\n", }, }, + { + name: "an EMPTY package-lock.json (lockfileChecks, hard error — issue #255)", + files: map[string]string{ + "block.manifest.json": goodBase, + "package.json": `{"name":"cov-block","private":true}`, + "package-lock.json": "", + }, + }, { name: "a page app whose source never posts the ready ack (readyAckChecks)", files: map[string]string{ diff --git a/internal/validate/finding_fields_test.go b/internal/validate/finding_fields_test.go index 04f37ca..e11fe5b 100644 --- a/internal/validate/finding_fields_test.go +++ b/internal/validate/finding_fields_test.go @@ -148,6 +148,10 @@ func findingFieldLedger() []fieldExpectation { "block.manifest.json for a file that is missing from the repo"}, {"more than one lockfile is committed", FieldProject, "same: the finding is about which files are committed"}, + {"not a lockfile the platform build can install from", FieldProject, + "same again, and the one most likely to be mis-filed: the remedy is `rm` plus " + + "`npm install`, so it touches no manifest key at all. `(root)` would send a " + + "CI job into block.manifest.json looking for a file on disk"}, // --- readyack.go ------------------------------------------------ {"nothing in this project's source posts BLOCK_READY", FieldProject, diff --git a/internal/validate/lockfile.go b/internal/validate/lockfile.go index e0e9b3b..b2afcc8 100644 --- a/internal/validate/lockfile.go +++ b/internal/validate/lockfile.go @@ -36,8 +36,22 @@ package validate // `validate` deliberately does not do. `npm ci` / `--frozen-lockfile` still catch // a stale lockfile server-side (loudly, which is the point of removing the // fallback). +// +// 🔴 "PRESENCE" IS ABOUT THE FILE, NOT ABOUT ITS BYTES — AND AN EMPTY FILE IS NOT +// A FRESHNESS QUESTION, IT IS "NOT A LOCKFILE AT ALL" (issue #255). The check used +// to ask os.Lstat and nothing else, so a 0-byte `package-lock.json` validated +// clean, exited 0, and the platform build failed anyway: 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 +// reading and a silently wrong one: the check invited the exact input that +// defeated it. So the required lockfile is now read, and the content rule is +// PER-MANAGER — see lockfileContentDefect. import ( + "bytes" + "encoding/json" "fmt" "os" "path/filepath" @@ -67,16 +81,41 @@ type packageManager struct { // buildCommand is the manifest buildCommand that selects this manager, used // when suggesting "switch the manifest to match the lockfile you have". buildCommand string + // lockfileIsJSON selects the CONTENT rule. Only npm's lockfile is JSON with a + // version key the CLI can check without a parser it does not have; see + // lockfileContentDefect for why pnpm and yarn deliberately stop at "not + // empty". + lockfileIsJSON bool + // contentRule states, in the author's terms, what the strict install requires + // of the lockfile's CONTENT. It is appended to the "this is not a lockfile" + // message so the remedy is checkable rather than a bare assertion. + contentRule string } var ( - pmNpm = packageManager{"npm", "package-lock.json", "npm ci", "npm install", "npm run build"} - pmPnpm = packageManager{"pnpm", "pnpm-lock.yaml", "pnpm install --frozen-lockfile", "pnpm install", "pnpm run build"} + pmNpm = packageManager{ + name: "npm", lockfile: "package-lock.json", + installCmd: "npm ci", refreshCmd: "npm install", buildCommand: "npm run build", + lockfileIsJSON: true, + contentRule: "`npm ci` requires a package-lock.json that parses as JSON and declares a " + + `numeric "lockfileVersion" of 1 or more (npm's own words: "can only install with an ` + + `existing package-lock.json or npm-shrinkwrap.json with lockfileVersion >= 1")`, + } + pmPnpm = packageManager{ + name: "pnpm", lockfile: "pnpm-lock.yaml", + installCmd: "pnpm install --frozen-lockfile", refreshCmd: "pnpm install", buildCommand: "pnpm run build", + contentRule: "`pnpm install --frozen-lockfile` needs the file `pnpm install` wrote, and that file is never empty", + } // The yarn branch of the recipe dispatches on `yarn --version`: // 1.*) yarn install --frozen-lockfile --ignore-scripts // *) YARN_ENABLE_SCRIPTS=false yarn install --immutable // so the message names both instead of claiming --frozen-lockfile always. - pmYarn = packageManager{"yarn", "yarn.lock", "yarn install --frozen-lockfile (yarn 1) / --immutable (yarn 2+)", "yarn install", "yarn run build"} + pmYarn = packageManager{ + name: "yarn", lockfile: "yarn.lock", + installCmd: "yarn install --frozen-lockfile (yarn 1) / --immutable (yarn 2+)", + refreshCmd: "yarn install", buildCommand: "yarn run build", + contentRule: "a strict `yarn install` needs the file `yarn install` wrote, and that file is never empty", + } ) // packageManagers is the scan order for reporting which lockfiles are committed. @@ -134,13 +173,20 @@ func lockfileChecks(dir string, m *manifest.Manifest) (errs []Finding, warns []F var committed, foreign []packageManager haveWanted := false + wantedDefect := "" for _, pm := range packageManagers { - if !regularFileExists(filepath.Join(dir, pm.lockfile)) { + path := filepath.Join(dir, pm.lockfile) + if !regularFileExists(path) { continue } committed = append(committed, pm) if pm.name == want.name { haveWanted = true + // Only the REQUIRED lockfile's content is judged. A foreign one is + // reported for what it is — a file that is committed and tells us + // which package manager this project really uses — and that reading + // does not depend on its bytes. + wantedDefect = lockfileContentDefect(path, pm) } else { foreign = append(foreign, pm) } @@ -149,6 +195,13 @@ func lockfileChecks(dir string, m *manifest.Manifest) (errs []Finding, warns []F if !haveWanted { return []Finding{newFinding(FieldProject, missingLockfileError(want, foreign, build))}, nil } + if wantedDefect != "" { + // The file is there and is provably not a lockfile, so the build fails + // exactly as it would with nothing committed. Same tier, different + // message: telling this author "no lockfile is committed" when one is + // sitting in their tree is what makes `touch` look like the fix. + return []Finding{newFinding(FieldProject, unusableLockfileError(want, wantedDefect))}, nil + } // The required lockfile IS there, so the build installs strictly and // reproducibly — any extra lockfile is unused by the platform. That is not // build-breaking, so it belongs in the advisory tier, but it is worth saying: @@ -218,6 +271,111 @@ func missingLockfileError(want packageManager, foreign []packageManager, build s } } +// maxLockfileBytes bounds the read. Real lockfiles are kilobytes to a few +// megabytes (a big npm monorepo lock is single-digit MB), so 64 MiB is roughly +// two orders of magnitude above anything a package manager writes and cannot be +// reached by a genuine lockfile — while still bounding what `validate` will pull +// into memory for a file whose only job is to be checked for one key. +// `internal/validate` has been here before: before the ready-ack scan grew caps, +// one 88 MB `.js` took peak RSS to 316 MB (AGENTS.md item 18). +const maxLockfileBytes = 64 << 20 + +// lockfileContentDefect reports WHY the file at path is not a usable lockfile, +// or "" if it is one — or if we could not tell. +// +// 🔴 THIS CHECK IS FATAL, SO AN UNOBSERVABLE STATE MUST FALL BACK TO THE OLD +// PRESENCE-ONLY PASS AND NEVER TO AN ERROR. A read failure, or a file over +// maxLockfileBytes, means we did not look — and manufacturing a hard error that +// blocks a submit out of a gap is the expensive direction (AGENTS.md item 18: +// "reading nothing is not finding nothing"). The rule the check adds is "a file +// we READ and that provably is not a lockfile", never "a file we could not +// vouch for". +// +// 🔴 THE Lstat/IsRegular GATE STAYS IN FRONT OF THE READ, and the order is +// load-bearing rather than incidental. regularFileExists mirrors pkgzip.Build, +// which skips every non-regular entry, so a SYMLINKED lockfile is dropped from +// the submitted bundle. os.ReadFile follows symlinks; reading through one would +// vouch for content the bundle does not carry. Callers therefore call +// regularFileExists FIRST and only then reach this. +// +// The rule is per-manager, and the asymmetry is deliberate: +// +// - npm: parse as JSON and require a NUMERIC `lockfileVersion` >= 1. That is +// not a guess at npm's intent — it is npm's own precondition, quoted in +// pmNpm.contentRule, so this mirrors the platform build recipe exactly the +// way the rest of this file does. +// - pnpm / yarn: non-empty after a whitespace trim, and nothing more. +// `pnpm-lock.yaml` would need a YAML parser (a new third-party dependency, +// which is an "ask first" in AGENTS.md) and a yarn v1 `yarn.lock` carries no +// version key at all — it is a comment header and a flat list. "Not empty" +// is the whole of what can be said here without inventing authority, and it +// is exactly the reported defect from issue #255. +func lockfileContentDefect(path string, pm packageManager) string { + info, err := os.Lstat(path) + if err != nil || info.Size() > maxLockfileBytes { + return "" // unobservable — degrade to presence-only + } + body, err := os.ReadFile(path) + if err != nil { + return "" // unobservable — degrade to presence-only + } + if len(bytes.TrimSpace(body)) == 0 { + if len(body) == 0 { + return "it is EMPTY (0 bytes)" + } + return "it is EMPTY (whitespace only)" + } + if !pm.lockfileIsJSON { + return "" + } + // A lockfile is a JSON OBJECT; an array, a string or a bare number decodes + // without error and is still not one, so decode into a map rather than into + // `any`. + var doc map[string]json.RawMessage + if err := json.Unmarshal(body, &doc); err != nil { + return "it does not parse as a JSON object" + } + raw, ok := doc["lockfileVersion"] + if !ok { + return `it declares no "lockfileVersion"` + } + // 🔴 UNMARSHALLING THE VALUE STRAIGHT INTO A json.Number DOES NOT DISCRIMINATE: + // json.Number is `type Number string`, so the JSON STRING "3" decodes into one + // happily and `{"lockfileVersion": "3"}` sailed through. Decoding into `any` + // with UseNumber is what keeps the two apart — a JSON number arrives as + // json.Number, a JSON string as a string. + var val any + dec := json.NewDecoder(bytes.NewReader(raw)) + dec.UseNumber() + if err := dec.Decode(&val); err != nil { + return `its "lockfileVersion" is not a number` + } + num, ok := val.(json.Number) + if !ok { + return `its "lockfileVersion" is not a number` + } + v, err := num.Float64() + if err != nil || v < 1 { + return `its "lockfileVersion" is below 1` + } + return "" +} + +// unusableLockfileError is the message for "the file is there and is not a +// lockfile". It is deliberately NOT the missing-lockfile message. +// +// 🔴 THE MESSAGE MUST NOT RE-INVITE THE WRONG TURN. missingLockfileError names +// the filename the build wants, and issue #255 is what an author does with that: +// `touch package-lock.json`, a green `validate`, and the identical opaque +// server-side build failure. So this one says the file EXISTS, says what is +// wrong with it, and says in as many words that a lockfile is generated by the +// package manager rather than created by hand. +func unusableLockfileError(want packageManager, defect string) string { + return fmt.Sprintf( + "%s is committed but %s, so it is not a lockfile the platform build can install from — it will run `%s`, 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 %s, run `%s`, and commit the %s it writes. %s.", + want.lockfile, defect, want.installCmd, want.lockfile, want.refreshCmd, want.lockfile, want.contentRule) +} + func extraLockfileWarning(want packageManager, committed []packageManager, build string) string { return fmt.Sprintf( "more than one lockfile is committed (%s) — %s, so the platform build installs only from %s and ignores the rest. The unused lockfiles will silently drift out of sync; delete the ones for package managers this app does not use.", diff --git a/internal/validate/lockfile_content_test.go b/internal/validate/lockfile_content_test.go new file mode 100644 index 0000000..783c8b8 --- /dev/null +++ b/internal/validate/lockfile_content_test.go @@ -0,0 +1,454 @@ +package validate + +import ( + "os" + "path/filepath" + "strings" + "testing" +) + +// lockfile_content_test.go is the guard for issue #255: `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. +// +// 🔴 THE DEFECT WAS INVITED BY THE MESSAGE THAT REPLACED IT. The missing-lockfile +// error names the filename the build wants, so `touch package-lock.json` is a +// natural reading of it — and it produced a green validate in front of the +// identical opaque server-side failure. That is why the tests below assert the +// two messages are DIFFERENT, not merely that something failed. +// +// Both directions are pinned on purpose, because either half alone is satisfiable +// by a broken fix: a predicate that rejects EVERY lockfile passes every negative +// case here, and a predicate that rejects none passes every positive control. The +// positive controls are the rows that make the negatives mean anything. + +// pkgJSON is the file whose presence is what makes the lockfile check fire at +// all (the recipe's `if [ -f package.json ]`). +const pkgJSON = `{"name":"lock-block","private":true}` + +// npmProject validates a project with package.json + a package-lock.json holding +// exactly the given bytes, under the default (npm) build branch. +func npmProject(t *testing.T, lock string) Result { + t.Helper() + return project(t, lockManifest("npm run build"), map[string]string{ + "package.json": pkgJSON, + "package-lock.json": lock, + }) +} + +// wantUnusableLockfileError asserts the finding is the EXISTS-BUT-IS-NOT-ONE +// message rather than the missing-lockfile one, and that it carries the remedy +// that does not re-invite `touch`. +func wantUnusableLockfileError(t *testing.T, res Result, lockfile, refreshCmd string, defect string) { + t.Helper() + if res.OK() { + t.Fatalf("a committed %s that is not a lockfile must be a hard error, got a clean pass", lockfile) + } + wantLockError(t, res, + lockfile+" is committed but "+defect, + "not a lockfile the platform build can install from", + // 🔴 The remedy has to say the file is GENERATED. Without this clause the + // message is compatible with "create the file", which is the input that + // defeated the old check. + "GENERATED by the package manager, never hand-written and never created with `touch`", + "delete "+lockfile, + refreshCmd, + ) + // It must NOT be the missing-lockfile message: an author whose file is + // sitting right there and is told "no lockfile is committed" learns to + // distrust the check, and `touch` looks like the fix all over again. + msg := lockErrors(res)[0] + for _, forbidden := range []string{"no lockfile is committed", "package.json is present but"} { + if strings.Contains(msg, forbidden) { + t.Errorf("the exists-but-invalid message must not reuse the MISSING message's wording %q:\n %s", + forbidden, msg) + } + } +} + +// --------------------------------------------------------------------------- +// (1) The regression: a 0-byte package-lock.json. RED at base. +// --------------------------------------------------------------------------- + +func TestLockfileEmptyNpmLockfileIsFatal(t *testing.T) { + res := npmProject(t, "") + wantUnusableLockfileError(t, res, "package-lock.json", "npm install", "it is EMPTY (0 bytes)") + // The message must also carry npm's OWN precondition, so the author can check + // the file against a rule rather than against our assertion. Measured on npm + // 11.17.0: an empty package-lock.json fails `npm ci` with EUSAGE naming + // exactly this. + wantLockError(t, res, "lockfileVersion >= 1", "npm ci") +} + +// --------------------------------------------------------------------------- +// (2) POSITIVE CONTROL — a real lockfile still passes. +// +// Without this, "reject everything" satisfies every negative case above. +// --------------------------------------------------------------------------- + +func TestLockfileRealNpmLockfileStillPasses(t *testing.T) { + res := npmProject(t, npmLockBody) + if !res.OK() { + t.Fatalf("a real package-lock.json must validate clean, got: %v", Messages(res.Errors)) + } + if res.HasWarnings() { + t.Errorf("a real package-lock.json must be warning-free, got: %v", Messages(res.Warnings)) + } +} + +// --------------------------------------------------------------------------- +// (3)(4)(5) The npm content rule, in one table with both directions. +// --------------------------------------------------------------------------- + +func TestLockfileNpmContentRule(t *testing.T) { + cases := []struct { + name string + body string + // defect is the clause the message must carry; "" means the body is + // ACCEPTED (a positive control). + defect string + why string + }{ + { + name: "a real lockfile", body: npmLockBody, + why: "POSITIVE CONTROL: the shape `npm install` writes", + }, + { + name: "lockfileVersion 3 and nothing else", body: `{"lockfileVersion": 3}`, + why: "POSITIVE CONTROL: the version key is the whole of npm's precondition; " + + "the CLI must not invent further structure it has no authority over", + }, + { + name: "lockfileVersion 1, the floor", body: `{"lockfileVersion": 1}`, + why: "POSITIVE CONTROL: npm's own bound is `>= 1`, inclusive", + }, + { + name: "0 bytes", body: "", defect: "it is EMPTY (0 bytes)", + why: "issue #255 itself", + }, + { + name: "whitespace only", body: " \n\t\r\n ", defect: "it is EMPTY (whitespace only)", + why: "a file `touch`ed and then saved by an editor is whitespace, not 0 bytes — " + + "and it is exactly as much of a lockfile", + }, + { + name: "valid JSON, no lockfileVersion", body: "{}\n", + defect: `it declares no "lockfileVersion"`, + why: "🔴 DELIBERATE: `npm ci` rejects `{}` with the SAME EUSAGE as an empty file " + + "(\"with lockfileVersion >= 1\"), so accepting it would leave the headline " + + "defect half-open — `echo '{}' > package-lock.json` for `touch`", + }, + { + name: "valid JSON object with other keys but no version", body: `{"name":"x","packages":{}}`, + defect: `it declares no "lockfileVersion"`, + why: "same rule; a plausible-looking hand-written file is still not a lockfile", + }, + { + name: "not JSON at all", body: "lockfileVersion: 3\n", + defect: "it does not parse as a JSON object", + why: "a YAML body under the npm filename — the mixed-package-manager mistake, in one file", + }, + { + name: "a JSON ARRAY", body: `[{"lockfileVersion": 3}]`, + defect: "it does not parse as a JSON object", + why: "decoding into `any` would accept this; a lockfile is an OBJECT, and the " + + "version key of an element is not the document's", + }, + { + name: "lockfileVersion as a string", body: `{"lockfileVersion": "3"}`, + defect: `its "lockfileVersion" is not a number`, + why: "npm writes a number. Accepting a string would be the CLI inventing a rule " + + "the platform does not have — see the residual in the doc comment", + }, + { + name: "lockfileVersion 0", body: `{"lockfileVersion": 0}`, + defect: `its "lockfileVersion" is below 1`, + why: "npm's bound is `>= 1`; 0 is the pre-v5 sentinel and `npm ci` refuses it", + }, + } + + var accepted, rejected int + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + res := npmProject(t, tc.body) + if tc.defect == "" { + if !res.OK() { + t.Fatalf("this body must be ACCEPTED (%s), got: %v", tc.why, Messages(res.Errors)) + } + return + } + wantUnusableLockfileError(t, res, "package-lock.json", "npm install", tc.defect) + }) + if tc.defect == "" { + accepted++ + } else { + rejected++ + } + } + + // Count floors on the table itself. A table that drifted to all-negative + // would be satisfied by "reject everything", and one that drifted to + // all-positive by "accept everything"; either reads as a serene pass. + if accepted < 3 { + t.Errorf("only %d ACCEPTED rows — with fewer, a predicate that rejects every "+ + "lockfile passes this whole table", accepted) + } + if rejected < 5 { + t.Errorf("only %d REJECTED rows — with fewer, a predicate that accepts every "+ + "lockfile passes this whole table", rejected) + } +} + +// --------------------------------------------------------------------------- +// (6) CONTROL — the MISSING case is unchanged. +// --------------------------------------------------------------------------- + +// The whole point of the new message is that it is a different one, so the old +// message must still fire, unchanged, for the case it was written for. +func TestLockfileMissingMessageIsUnchangedByTheContentCheck(t *testing.T) { + res := project(t, lockManifest("npm run build"), map[string]string{"package.json": pkgJSON}) + if res.OK() { + t.Fatal("package.json with no lockfile must still be a hard error") + } + wantLockError(t, res, + "package.json is present but no lockfile is committed", + "npm ci", + "package-lock.json", + "npm install", + ) + // And it must NOT have acquired the new message's wording — one finding, one + // diagnosis. + if msg := lockErrors(res)[0]; strings.Contains(msg, "is committed but") { + t.Errorf("the missing-lockfile message must not claim a file is committed:\n %s", msg) + } +} + +// --------------------------------------------------------------------------- +// (7) pnpm and yarn: NON-EMPTY, and deliberately nothing more. +// --------------------------------------------------------------------------- + +// The asymmetry with npm is a decision, not an omission: `pnpm-lock.yaml` would +// need a YAML parser (a new third-party dependency — "ask first" in AGENTS.md) +// and a yarn v1 `yarn.lock` carries no version key at all. "Not empty" is the +// whole of what can be said without inventing authority, and it is exactly the +// defect issue #255 reported. +func TestLockfileNonJSONManagersRejectOnlyEmptiness(t *testing.T) { + cases := []struct { + build string + lockfile string + refreshCmd string + }{ + {"pnpm run build", "pnpm-lock.yaml", "pnpm install"}, + {"yarn run build", "yarn.lock", "yarn install"}, + } + for _, tc := range cases { + t.Run(tc.lockfile, func(t *testing.T) { + for _, empty := range []struct{ name, body, defect string }{ + {"0 bytes", "", "it is EMPTY (0 bytes)"}, + {"whitespace only", "\n\n \t\n", "it is EMPTY (whitespace only)"}, + } { + t.Run(empty.name, func(t *testing.T) { + res := project(t, lockManifest(tc.build), map[string]string{ + "package.json": pkgJSON, + tc.lockfile: empty.body, + }) + wantUnusableLockfileError(t, res, tc.lockfile, tc.refreshCmd, empty.defect) + }) + } + + // POSITIVE CONTROLS. The second is the one that pins the asymmetry: + // a body that npm's rule would REJECT must be accepted here, or the + // npm rule has silently leaked onto every package manager. + for _, ok := range []struct{ name, body string }{ + {"a real lockfile", lockBody(tc.lockfile)}, + {"a body npm's rule would reject", "{}\n"}, + {"anything non-blank at all", "x"}, + } { + t.Run("accepts "+ok.name, func(t *testing.T) { + res := project(t, lockManifest(tc.build), map[string]string{ + "package.json": pkgJSON, + tc.lockfile: ok.body, + }) + if !res.OK() { + t.Fatalf("%s must accept any non-empty body, got: %v", tc.lockfile, Messages(res.Errors)) + } + }) + } + }) + } +} + +// --------------------------------------------------------------------------- +// (8) CONTROL — a static app is never flagged, whatever the lockfile state. +// --------------------------------------------------------------------------- + +// The recipe's `if [ -f package.json ]` guard is what makes a static block skip +// the install step entirely. It must keep deciding this BEFORE any content is +// read: an empty lockfile in a tree that never installs is inert, and flagging +// it would be a hard error on a project the platform builds fine. +func TestLockfileContentCheckNeverFiresWithoutPackageJSON(t *testing.T) { + for _, body := range []string{"", " \n", "{}\n", "not json", npmLockBody} { + res := project(t, lockManifest(""), map[string]string{"package-lock.json": body}) + if !res.OK() { + t.Errorf("a static app (no package.json) must never be flagged for its lockfile "+ + "content (%q), got: %v", body, Messages(res.Errors)) + } + } +} + +// --------------------------------------------------------------------------- +// (9) UNOBSERVABLE -> the old presence-only PASS, never an error. +// --------------------------------------------------------------------------- + +// 🔴 This check is FATAL, so a state we could not read must degrade to what the +// check did before rather than manufacture a hard error out of a gap +// (AGENTS.md item 18). The size cap is the observable half of that: over it, the +// file is not read at all. +// +// The sub-cap row is the CONTROL that makes the over-cap row mean something. The +// two files hold the SAME bytes and differ only in length, so a pass at 64 MiB+1 +// is attributable to the cap and to nothing else — without it, "the big file +// passed" is equally consistent with a predicate that accepts everything. +func TestLockfileOverTheSizeCapDegradesToPresenceOnly(t *testing.T) { + // A sparse file: Truncate reports the size without writing 64 MiB. + sparse := func(t *testing.T, dir string, size int64) { + t.Helper() + f, err := os.Create(filepath.Join(dir, "package-lock.json")) + if err != nil { + t.Fatal(err) + } + defer f.Close() + if err := f.Truncate(size); err != nil { + t.Skipf("cannot create a sparse file here: %v", err) + } + } + run := func(t *testing.T, size int64) Result { + t.Helper() + dir := t.TempDir() + for name, body := range map[string]string{ + "block.manifest.json": lockManifest("npm run build"), + "package.json": pkgJSON, + } { + if err := os.WriteFile(filepath.Join(dir, name), []byte(body), 0o600); err != nil { + t.Fatal(err) + } + } + sparse(t, dir, size) + res, err := Dir(dir) + if err != nil { + t.Fatalf("Dir: %v", err) + } + return res + } + + // CONTROL: the same NUL bytes, comfortably under the cap, ARE read and ARE + // rejected. If this passes, the row below proves nothing. + if res := run(t, 4096); res.OK() { + t.Fatal("control: a sub-cap file of NUL bytes is not JSON and must be rejected — " + + "the over-cap assertion below is vacuous without this") + } + + if res := run(t, maxLockfileBytes+1); !res.OK() { + t.Errorf("a lockfile over the %d-byte cap is UNOBSERVABLE, so the check must fall back "+ + "to the presence-only pass rather than manufacture a hard error from a gap; got: %v", + int64(maxLockfileBytes), Messages(res.Errors)) + } +} + +// An unreadable lockfile is the other unobservable state. Same doctrine: we did +// not look, so we say nothing. +func TestLockfileUnreadableDegradesToPresenceOnly(t *testing.T) { + dir := t.TempDir() + for name, body := range map[string]string{ + "block.manifest.json": lockManifest("npm run build"), + "package.json": pkgJSON, + } { + if err := os.WriteFile(filepath.Join(dir, name), []byte(body), 0o600); err != nil { + t.Fatal(err) + } + } + lock := filepath.Join(dir, "package-lock.json") + // Content that WOULD be rejected, so a pass can only come from the read + // failing — not from the bytes being acceptable. + if err := os.WriteFile(lock, []byte(""), 0o000); err != nil { + t.Fatal(err) + } + if _, err := os.ReadFile(lock); err == nil { + t.Skip("this process can read a mode-000 file (running as root?), so there is no " + + "unreadable state to observe here") + } + + res, err := Dir(dir) + if err != nil { + t.Fatalf("Dir: %v", err) + } + if !res.OK() { + t.Errorf("an unreadable lockfile is UNOBSERVABLE — the check must degrade to the "+ + "presence-only pass, not block a submit on a gap; got: %v", Messages(res.Errors)) + } +} + +// --------------------------------------------------------------------------- +// The Lstat/IsRegular gate must stay IN FRONT of the read. +// --------------------------------------------------------------------------- + +// 🔴 regularFileExists mirrors pkgzip.Build, which skips every non-regular entry, +// so a SYMLINKED lockfile is dropped from the submitted bundle. os.ReadFile +// FOLLOWS symlinks — so a content check that read first (or read independently) +// would vouch for bytes the bundle does not carry, and the platform build would +// hard-fail behind a green validate. TestLockfileSymlinkedLockfileIsNotAccepted +// pins the verdict; this pins the ORDER, by pointing the symlink at a VALID +// lockfile. Only a check that reads through the link can pass it. +func TestLockfileSymlinkToAValidLockfileIsStillAbsent(t *testing.T) { + dir := t.TempDir() + for name, body := range map[string]string{ + "block.manifest.json": lockManifest("npm run build"), + "package.json": pkgJSON, + } { + if err := os.WriteFile(filepath.Join(dir, name), []byte(body), 0o600); err != nil { + t.Fatal(err) + } + } + outside := filepath.Join(t.TempDir(), "package-lock.json") + if err := os.WriteFile(outside, []byte(npmLockBody), 0o600); err != nil { + t.Fatal(err) + } + if err := os.Symlink(outside, filepath.Join(dir, "package-lock.json")); err != nil { + t.Skipf("cannot create a symlink here: %v", err) + } + + res, err := Dir(dir) + if err != nil { + t.Fatalf("Dir: %v", err) + } + if res.OK() { + t.Fatal("a symlinked lockfile is dropped from the bundle, so even a perfectly valid " + + "one must read as ABSENT — the Lstat/IsRegular gate has to run before the read") + } + wantLockError(t, res, "no lockfile is committed") +} + +// --------------------------------------------------------------------------- +// Scope: only the REQUIRED lockfile's content is judged. +// --------------------------------------------------------------------------- + +// A foreign lockfile is reported for what it 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 top of the +// real one, exactly as deriving a package manager from a disallowed buildCommand +// would. +func TestLockfileContentOfAForeignLockfileIsNotJudged(t *testing.T) { + res := project(t, lockManifest("pnpm run build"), map[string]string{ + "package.json": pkgJSON, + "pnpm-lock.yaml": lockBody("pnpm-lock.yaml"), + "package-lock.json": "", // empty, and irrelevant: pnpm is what installs + }) + if !res.OK() { + t.Fatalf("an empty FOREIGN lockfile must not be a hard error: %v", Messages(res.Errors)) + } + // It is still reported as an extra lockfile, unchanged. + joined := strings.Join(Messages(res.Warnings), "\n") + if !strings.Contains(joined, "more than one lockfile is committed") { + t.Errorf("the extra-lockfile advisory must be unchanged:\n%s", joined) + } +} diff --git a/internal/validate/lockfile_test.go b/internal/validate/lockfile_test.go index 90ae88f..ee71377 100644 --- a/internal/validate/lockfile_test.go +++ b/internal/validate/lockfile_test.go @@ -25,6 +25,38 @@ func lockManifest(buildCommand string) string { }` } +// npmLockBody is a REAL package-lock.json: the shape `npm install` writes, +// trimmed to what matters here. +// +// 🔴 These fixtures used to be the literal `{}`, and that was WRONG about the +// platform in the direction that hid issue #255: measured on npm 11.17.0, `npm +// ci` over `{}` dies with the same EUSAGE ("lockfileVersion >= 1") as over an +// empty file. A fixture that stands in for "the author ran the install" has to +// carry what the install writes, or every "this passes" assertion below is +// asserting that a build-breaking project validates clean. +const npmLockBody = `{ + "name": "lock-block", + "version": "0.1.0", + "lockfileVersion": 3, + "requires": true, + "packages": {"": {"name": "lock-block", "version": "0.1.0"}} +} +` + +// lockBody returns a real, package-manager-written body for the named lockfile. +func lockBody(name string) string { + switch name { + case "package-lock.json": + return npmLockBody + case "pnpm-lock.yaml": + return "lockfileVersion: '9.0'\n" + case "yarn.lock": + return "# yarn lockfile v1\n" + default: + panic("lockBody: unknown lockfile " + name) + } +} + // project writes a manifest plus the given extra files (name -> contents) into a // temp dir and validates it. func project(t *testing.T, manifestJSON string, files map[string]string) Result { @@ -134,7 +166,7 @@ func TestLockfileMatchingLockfilePasses(t *testing.T) { t.Run(tc.name, func(t *testing.T) { res := project(t, lockManifest(tc.build), map[string]string{ "package.json": `{"name":"lock-block","private":true}`, - tc.lockfile: "{}\n", + tc.lockfile: lockBody(tc.lockfile), }) if !res.OK() { t.Fatalf("should validate clean, got: %v", res.Errors) @@ -192,7 +224,7 @@ func TestLockfileMismatchIsFatalForEveryPairing(t *testing.T) { t.Run(tc.name, func(t *testing.T) { res := project(t, lockManifest(tc.build), map[string]string{ "package.json": `{"name":"lock-block","private":true}`, - tc.lockfile: "{}\n", + tc.lockfile: lockBody(tc.lockfile), }) if res.OK() { t.Fatal("mismatched lockfile must be a hard error") @@ -340,7 +372,7 @@ func TestLockfileSymlinkedLockfileIsNotAccepted(t *testing.T) { // A REAL lockfile outside the project, symlinked in — the shape that used to // pass because os.Stat follows symlinks. outside := filepath.Join(t.TempDir(), "package-lock.json") - if err := os.WriteFile(outside, []byte("{}\n"), 0o600); err != nil { + if err := os.WriteFile(outside, []byte(npmLockBody), 0o600); err != nil { t.Fatal(err) } link := filepath.Join(dir, "package-lock.json") @@ -398,7 +430,7 @@ func TestLockfileSymlinkedPackageJSONIsTreatedAsStatic(t *testing.T) { func TestLockfileMultipleWithRequiredPresentIsWarning(t *testing.T) { res := project(t, lockManifest("npm run build"), map[string]string{ "package.json": `{"name":"lock-block","private":true}`, - "package-lock.json": "{}\n", + "package-lock.json": npmLockBody, "pnpm-lock.yaml": "lockfileVersion: '9.0'\n", }) if !res.OK() { diff --git a/internal/validate/validate_test.go b/internal/validate/validate_test.go index 17db490..e216dae 100644 --- a/internal/validate/validate_test.go +++ b/internal/validate/validate_test.go @@ -29,7 +29,10 @@ func scaffoldGood(t *testing.T, tmpl scaffold.Template) string { dir := scaffoldRaw(t, tmpl) if _, err := os.Stat(filepath.Join(dir, "package.json")); err == nil { // The npm templates declare `npm run build`; stand in for `npm install`. - if err := os.WriteFile(filepath.Join(dir, "package-lock.json"), []byte("{}\n"), 0o600); err != nil { + // The body has to be what an install WRITES, not `{}` — `npm ci` refuses + // `{}` with the same EUSAGE as an empty file (issue #255), so a `{}` + // fixture would assert that a build-breaking project validates clean. + if err := os.WriteFile(filepath.Join(dir, "package-lock.json"), []byte(npmLockBody), 0o600); err != nil { t.Fatalf("write package-lock.json: %v", err) } }