diff --git a/AGENTS.md b/AGENTS.md index 5d180ed..34a0a11 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -166,10 +166,13 @@ is model substitution, and 22 is the one gate on that path that guards CONTENT rather than money); items 18 and 20 cover the checks that tell an author their EXISTING app is missing the item-11 handshake (20 is the reachability repair to 18's presence-only scan); item 23 covers the SHAPE of a validation finding — the -`field` every `--json` consumer groups on; and item 24 covers the ONE +`field` every `--json` consumer groups on; item 24 covers the ONE transport-vs-filesystem predicate now shared by the CLI-wide exit-code classifier (which every command's published exit code funnels through) and -`pkg/civitai`'s read-GET retry loop. +`pkg/civitai`'s read-GET retry loop; and item 25 covers the OTHER gate on that +same published contract — the classification of the project path +`civitai app validate` / `app submit` are handed, which is a different rule from +item 24's predicate and is filed separately for exactly that reason. The durable fix for the mirroring is a server-side `civitai app validate` endpoint that calls the real `BlockManifestValidator` — until that exists, vendoring is on purpose. @@ -1864,6 +1867,254 @@ neither one's. dot-imported `net`) are listed in the file's own header; there is no full type resolution because `golang.org/x/tools/go/packages` would be a new dependency, which is an "ask first" below. +25. **`civitai app validate ` / `app submit ` CLASSIFY THE PATH THE + USER NAMED BEFORE VALIDATING ANYTHING, and that gate is deliberately NOT + item 24's transport predicate — it is a separate rule that happens to live + next door on the exit-code contract.** Issue #256. The code-2 note read + "that split is the rule for **every local path a FLAG names**", so the two + commands that take the path POSITIONALLY were outside the sentence and + disagreed with it for a release: `validate.Dir` stats the JOINED + `/block.manifest.json` and branches on `os.IsNotExist`, which COLLAPSES + "the directory does not exist" into "the directory exists but has no + manifest". `app validate /nope` reported a missing manifest at a project root + nobody has and exited **1**, and `app validate README.md` fell through to the + raw syscall and printed `stat README.md/block.manifest.json: not a + directory` — a path the CLI assembled and the user never typed — also on 1. + Closed by `resolveProjectDir` (`internal/cmd/project_dir.go`), a three-way + branch on the path the user NAMED: nonexistent → 2, + exists-but-not-a-directory → 2, a real directory → unchanged (`validate.Dir` + decides, and a manifest-less directory keeps its finding and exit 1, because + the invocation was right and the project is wrong). + - **The gate is in `internal/cmd`, not `internal/validate`, and that is + item 7's boundary.** `ErrUsage` is this package's sentinel and + `validate.Dir` returns a validation VERDICT; pushing the tag down would + make `internal/validate` import the usage sentinel and hold a slice of the + exit-code contract. It is also deliberately NOT on + `validate.ManifestOnly`'s path — `app init` self-checks a directory it just + created and has no user-named path to classify. + - 🔴 **ONE HELPER, TWO CALL SITES, AND THE SET IS ASSERTED.** `app submit` + had the identical hole; a per-command copy is the shape item 24 is about. + `TestEveryValidateDirCallerGatesOnResolveProjectDir` AST-walks the package + and requires the set of files calling `validate.Dir` to EQUAL the set + calling `resolveProjectDir` — failing when it grows (a third command + validating a user-named directory without the gate) and when it shrinks (a + deleted gate, which would otherwise leave the ledger a false map). + Mutation-measured **in the tree it ships in**: dropping the submit call + alone reddens **6** leaf subtests including this guard by name. (It was 4 + at `a4807f4`; the two new `TestSubmitGateRunsBeforeSkipValidate` rows below + account for the difference. An inherited mutation number is a claim about + a tree that no longer exists — re-run it or drop it.) Commenting the call + OUT rather than deleting it reddens the same 6: the ledger is an AST walk, + so the surviving text in a comment does not satisfy it. + - 🔴 **THE GATE RUNS AHEAD OF `--skip-validate`, AND THAT CLAUSE SHIPPED + WITH NO TEST AT ALL.** `--skip-validate` waives our opinion of the + MANIFEST; it cannot waive the question of whether the directory the user + typed exists, because there is no manifest to have an opinion about. The + ordering was stated in `app_submit.go`'s own comment, in the PR body and + here — and moving the call inside the `if !skipValidate` block, a one-line + move that reads like a tidy-up, left the ENTIRE suite green while reverting + `app submit --package-only --skip-validate` from rc **2** to + rc **1**. The only test that mentioned the flag used a VALID directory, so + nothing in the repo observed the interaction. `TestSubmitGateRunsBefore + SkipValidate` is the repair; re-measured, that mutant now reddens **2 leaf + subtests** by name. + - 🔴 **`app validate --json` IS A DELIBERATE WIRE BREAK, of item 23's + class.** `civitai app validate /nope --json` used to write + `{"ok":false,"dir":"/nope","errors":[…]}` to stdout and exit 1 — a + fabricated validation result, complete with a finding about a manifest + nobody could have written. It now writes **nothing** to stdout and exits 2, + keeping the CLI-wide convention that a usage error emits no JSON object. + The gate therefore runs BEFORE the `--json` block, and sliding it below is + its own mutant. 🔴 **Re-measured, and the number this item first published + was wrong in a way that misdescribes the guard**: it reddens **4** leaf + subtests, not 3, and **two of them are TEXT-mode rows** + (`TestProjectDirExitCodes/validate/path_is_a_regular_file` and + `TestProjectDirRefusalNamesThePathTheUserTyped/validate`) — not, as first + written, "exactly the JSON rows while the text-mode rows stay green". That + is unavoidable rather than sloppy: any placement below the `--json` block + is also below `validate.Dir`, so the text path loses the gate too. Same 4 + at `a4807f4` and at HEAD. It is announced in THREE places, because the one that + matters is the one a `--json` consumer reads: the code-2 README cell, the + `app validate` row of the command table, and the canonical + "The `--json` result shape" section — which is where a script author is + when they decide whether `| jq` is safe. + - 🔴 **`--json` PUBLISHES A RESULT ONLY WHEN VALIDATION PRODUCED ONE, AND + THE CODE-1 NOTE PROMISED OTHERWISE.** It read "`app validate --json` prints + the full result … so a script never has to read stderr", unqualified. + Measured on this branch: `app validate --json` exits 1 + with stdout **empty (0 bytes)** and the error on stderr only, because + `validate.Dir` returns an `error` rather than a `Result` for a non-ENOENT + stat failure and for a `schema()` failure (the `return res, err` arms in + `internal/validate/validate.go`). An unqualified promise is worse than + silence on the one command whose job is to be machine-read, so the note is + SCOPED rather than the code being changed to match it, which is a much + larger change (every such arm would have to become a Result). Exit 1 with + no object is therefore a real state a consumer must handle; the README + carries the exit→stdout table. + 🔴 **THE FIRST SCOPING WAS ALSO WRONG, BY ONE ARM.** It read "for a project + directory it could **read**", which names only the stat arm — but the + `schema()` arm is a directory the CLI reads perfectly well that still + yields no Result, so the sentence still promised an object for it (and the + README still said "nothing when the failure was a filesystem one", which + that arm is not). Both now condition on **whether validation produced a + result**, which is what the code actually branches on. Effectively + unreachable in a released binary — the schema is vendored and compiled at + init — so this is wording, not behaviour, and the operative instruction + ("branch on the exit code before parsing") was correct throughout. + - **A stat failure that is neither ENOENT nor a non-directory stays UNTAGGED + and exits 1** — EACCES on a parent, or ENOTDIR partway down a longer path. + `app validate /x.json` is one of the six invocations measured + in #241, and 1 is the answer that issue settled on. + 🔴 **The battery that pins it rested on ONE SKIPPABLE row inside + `internal/cmd`.** The widening mutant — turning the untagged arm into + `asUsageError`, i.e. "tag every stat failure" — reddened a single leaf + subtest there, and that subtest carried a `t.Skip` when the fixture + produced no error, so a filesystem that resolved the path would have taken + the guard with it silently. This is item 24's own recorded "a battery + rested on a single row" shape, regenerated. `project_dir_gate_test.go` now + runs TWO independent stat shapes (ENOTDIR below a regular file, EACCES on + an unsearchable parent) across THREE surfaces (the helper, `app validate`, + `app submit`), each row ASSERTING ITS OWN PREMISE — an independent + `os.Stat` must fail with something that is neither ENOENT nor a live + non-directory, or the row FAILS rather than quietly testing a different + branch — plus a count floor. + 🔴 **The audit's "exactly one" was itself scoped, and the corrected count + is stated because a mutation number nobody re-ran is a claim.** Measured + over the WHOLE module at the audited tip `a4807f4`, the widening reddened + **2** leaf subtests, not 1: the `internal/cmd` control row plus a + pre-existing `cmd/civitai` end-to-end row + (`TestFilesystemErrorsExitGenericEndToEnd/app_validate_(ENOTDIR)`), which a + package-scoped run does not see. Re-measured on the fixed tree: **8** leaf + subtests. And with that single old row DELETED as well, the new battery + alone still kills it — 7 leaves — so the guard no longer rests on a row + anyone can remove. + - **The message is NOT wrapped, and that is the fix rather than an + omission.** `os.Stat`'s error is an `*fs.PathError` whose `Error()` already + begins `stat : `, so a `fmt.Errorf("stat %s: %w", dir, err)` printed + the op and the path twice: measured on the first cut of this PR, + `Error: stat …/file.txt/x.json: stat …/file.txt/x.json: not a directory`, + where the base binary printed one `stat` (naming the JOINED path, which is + the defect above). There is no context left to add — we stat the path the + user typed. `TestProjectDirStatErrorDoesNotStutter` COUNTS occurrences + rather than matching a golden string, because the defect is a duplicate. + - 🔴 **THE TWO EXIT-2 REMEDIES ARE NAMED CONSTANTS, BECAUSE SWAPPING THEM + PASSED THE ENTIRE SUITE.** Both arms tag the same `ErrUsage` sentinel, so + no `errors.Is` assertion can tell them apart (item 7 is about the exit + code, and the exit code is identical), and the one message test asks only + that the path the user typed appears — true of either spelling. Measured: + exchanging the two format strings produced **0 failures**, leaving the CLI + telling a missing path to "pass the ROOT, not a file" and telling someone + who pointed at their manifest to `app init` a project they already have. + That is item 21(f)/(g)'s operand-order class, arrived at from a third + direction. `remedyNoSuchDir` / `remedyNotADir` and + `TestProjectDirRemediesMatchTheirArm` close it by deriving each arm's + expected text FROM THE CONSTANT and requiring the other arm's to be + ABSENT, with a non-empty + distinct precondition — because + `strings.Contains(x, "")` is always true and an empty or duplicated remedy + would silently disarm every assertion in the guard. + 🔴 **BOTH REMEDIES MUST BE RENDERED WITH THE SAME PATH, AND THE FIRST CUT + WAS NOT — WHICH LEFT HALF THE GUARD DEAD.** Each remedy interpolates the + path, and the two rows rendered theirs with DIFFERENT paths (the missing + one vs the file). So `Contains(err, deny)` compared against a string + carrying a path the error never mentions: false whatever the code does. + Measured at `ab1e685`: the BOTH-ARM swap gave 2 kills, **2 from `want`, + 0 from `deny`** — the absence half, which is the half this item + advertises, never fired once. (The ONE-ARM swap gave 1 kill, also entirely + from `want`; an earlier revision of this bullet attributed the 2 to the + one-arm swap, which is wrong about which mutant and right about the + `0 from deny` that is its point.) The same defect made the "distinct" + precondition compare two strings that differed only by their path, so + **two IDENTICAL remedy constants passed it**. Fixed by rendering both arms + from `tc.dir` via `noSuchAt` / `notDirAt`, with the precondition on one + shared probe path. Re-measured: one-arm swap `want` 1 / `deny` 1, + both-arms swap `want` 2 / `deny` 2. + 🔴 **THE CLAIM THAT `go vet` ALREADY CATCHES A DUPLICATE IS RETRACTED — IT + WAS TRUE OF ONE SPELLING AND FALSE OF THE ONES A REFACTOR PRODUCES.** This + bullet said "what actually stops a duplicate reaching `main` today is + `go vet`, not this guard … the precondition is the backstop for the day + someone equalises those arities". No equalisation is required. Measured at + `ab1e685`, three real edits to these two constants: + + | shape | `go build` | `go vet` | full suite | + |---|---|---|---| + | A — copy the body over verbatim (arity 2→1) | **ok** | rc=1 | 1 `build failed` line (2 printf diagnostics), 0 `--- FAIL`, 17 ok pkgs | + | B — same text + a trailing `%.0s` consuming arg 2 | ok | **rc=0** | **rc 0, 18 pkgs ok, 0 `--- FAIL`** | + | C — same advice + ` (looking for %s)` | ok | **rc=0** | **rc 0, 18 pkgs ok, 0 `--- FAIL`** | + + B and C are arity-preserving, and C especially is a completely natural way + to write the message — both shipped the backwards advice **fully green** + while the binary answered a real file with `…: no such directory — + … scaffold one with `civitai app init`` at rc 2. Note also that even A + **builds**: `go build ./cmd/civitai` succeeds and produces a binary with + the bad advice, so what stops A is CI running `go test` (which runs vet), + not a compile failure. At HEAD, B is killed by the precondition and C by + the `deny` assertion. **This guard is the live protection.** It carries its + own positive control — the comparator is handed a KNOWN duplicate pair and + must report it — so "it cannot reject anything" fails loudly rather than + reading as a pass. + 🔴 **And the block that positive control REPLACED could not fail**, which + is the third instance of that shape in this PR: it was + `dup != noSuchAt(probe) || dup == notDirAt(probe)` with + `dup := noSuchAt(probe)` — clause 1 is `s == s` on a deterministic pure + call, false in every state, and clause 2 was byte-identical to the + precondition immediately above it, which has already `t.Fatalf`'d. No + production change could reach its `t.Fatal`; measured, mutant B fires the + precondition and not it, and only after DELETING the precondition does its + one live clause fire — i.e. it backstopped a deleted test line. + staticcheck's SA4000 misses it because the spelling is `var != f(x)` + rather than the direct `f(x) != f(x)` it does fire on. **A control must be + fed a known-bad INPUT, not spelled as a condition about the code.** + - 🔴 **THE PUBLISHED SPLIT IS A LEDGER OF ENUMERATED PATHS, NOT A + QUANTIFIER — AND BOTH OVER-NARROW AND OVER-BROAD WORDINGS HAVE SHIPPED.** + "Every local path a FLAG names" excluded the positional commands (above). + The replacement, "every local path the CLI is **handed**", is ALSO false, + and has a live counterexample inside its own scope: measured identical on + base and on this branch, `civitai app listing status --dir /does/not/exist` + exits **1** (`app_listing.go`, `manifest.Load(lc.dir)` wrapped untagged), + and so does `app submit --package-only --out /nodir/x.zip`. So the + sentence now publishes the SHAPE — "a flag's value and a positional + argument alike" — over the paths it enumerates, and the README states the + residual instead of hiding it. `TestUngatedPathFlagsAreNotUsageErrors` + pins the residual and FAILS if `--dir` is ever brought into the gate, + which is the moment the published paragraph becomes wrong. Bringing it in + is a fine change; doing it without moving the docs is the failure this + guards. + 🔴 **THAT GUARD NEEDS A CREDENTIAL, AND WITHOUT ONE IT WAS INERT IN THE + ONLY ENVIRONMENT THAT MATTERS.** `app listing status` calls + `newListingClient()` BEFORE `resolveListingSlug`, so with no token + configured it fails at `no token configured` — an `ErrUnauthorized` — and + never reaches the `--dir` path at all. A bare `!errors.Is(err, ErrUsage)` + assertion is satisfied by that auth failure, so the guard passed for the + wrong reason. Measured with `HOME`/`XDG_CONFIG_HOME` pointed at an empty + directory (what `ubuntu-latest` is): both rows PASSED, **and still passed + with the residual-closing mutant applied** — the change it exists to catch + was invisible. On a developer box with a real config it happened to observe + the manifest error, so it looked healthy locally and was dead in CI, which + is the worst arrangement of those two facts. Fixed with `t.Setenv` of a + dummy token (nothing is sent — `manifest.Load` fails first) PLUS a premise + assertion. Re-measured hermetically: the clean run passes and the + residual-closing mutant reddens **2 leaf subtests**. + 🔴 **AND THE FIRST PREMISE WAS A DENYLIST OF ONE SENTINEL, WHICH IS NOT A + REACH ASSERTION — THE DEFECT REGENERATED IN FULL.** It asserted the error + was NOT `civitai.ErrUnauthorized`: that closes the one gate already known + about and says nothing about whether the row reached `resolveListingSlug`. + Measured hermetically, inserting ANY new preflight ahead of it that fails + with a plain untagged error: both rows **PASSED**, and with the + residual-closing mutant ALSO applied they **still both passed** — the + whole defect back, invisibly, for the third time in this PR. The premise + is now POSITIVE: the error must carry `resolveListingSlug`'s own wrapper, + derived from the `listingSlugResolveFailure` constant in `app_listing.go` + rather than spelled in the test, so a reword moves both together. Proven + by re-running that same preflight mutant: **2 leaf subtests, 2 + `PREMISE BROKEN` messages**, where the denylist form survived it. + **The general rule, and this is its third instance in this repo: a guard + that drives a whole COMMAND must assert it REACHED the code under test, + POSITIVELY.** An earlier gate in the same `RunE` — credentials, a TTY + check, flag parsing — fails first and satisfies any assertion phrased as + "it did not do the wrong thing", and enumerating the gates you know about + is not a substitute, because the next one is by definition the one you did + not enumerate. Item 23's `findingSiteHook` and item 24's `sentinelFree` + rows are the same device: prove the path ran, do not infer it from a pass. **When you change a validation rule, keep all four vendored mirrors in sync with the server — `schema/`, the ported Go checks in `internal/validate/` (including diff --git a/README.md b/README.md index dccfd6f..850ccb9 100644 --- a/README.md +++ b/README.md @@ -277,7 +277,7 @@ README. For the end-to-end walkthrough, see | `civitai app init [name] [dir] [...]` | Same scaffolder as `create` with a no-build `static` default (back-compat alias). | | `civitai app dev-token [--env] [--spend] [--budget ]` | **Mint a short-lived (~4h) dev block token for `npm run dev:live`.** `--spend` must be asked for explicitly to request real-Buzz spend — without it the CLI filters `ai:write:budgeted` out of the mint request. `--env` prints a paste-ready `VITE_LIVE_BLOCK_TOKEN=`. See [Local dev loop](#local-dev-loop-harness-mock-vs-live). | | `civitai app dev-tunnel [blockId] [--port] [--tunnel-endpoint] [--idle-timeout]` | **(Pre-GA / invite-gated)** Preview your **local** dev server inside the **real** Civitai host at `civitai.com/apps/dev/` — a prod-fidelity inner-dev-loop. Pre-flights whether the host can actually **embed** your dev server and warns (never fatally) when it cannot. See [Preview in the real host](#preview-in-the-real-host-app-dev-tunnel). | -| `civitai app validate [dir] [--strict] [--json]` | Best-effort local pre-check of `block.manifest.json`; emits non-fatal warnings (`--strict` fails on them). `--json` emits the structured result (`ok`, plus `errors`/`warnings` each with `field`/`message` — **`field` is always present and never `null`**) for scriptable parsing — still exits non-zero on failure. See [Validate fidelity](#validate-fidelity) and [The `--json` result shape](#the---json-result-shape). | +| `civitai app validate [dir] [--strict] [--json]` | Best-effort local pre-check of `block.manifest.json`; emits non-fatal warnings (`--strict` fails on them). `--json` emits the structured result (`ok`, plus `errors`/`warnings` each with `field`/`message` — **`field` is always present and never `null`**) for scriptable parsing — still exits non-zero on failure. 🔴 **BREAKING:** a `[dir]` that does not exist, or is not a directory, is now a **usage error** — exit `2` with **no JSON object** on stdout, where it used to print `{"ok": false, …}` and exit `1`. See [Validate fidelity](#validate-fidelity) and [The `--json` result shape](#the---json-result-shape). | | `civitai app submit [dir] [--package-only] [--out f.zip] [--skip-validate]` | Validate + package the source tree + upload it with your stored token (or, with no token, write the bundle + print next steps). | | `civitai app pull [dir] --app ` | **Clone (or sync) the canonical git repository behind one of your approved Apps** — the read side of git authoring. ⚠ The clone URL embeds your access token, and a fresh clone persists it into `.git/config`. See [Pull your app's repository](#pull-your-apps-repository-app-pull). | | `civitai app listing status\|set-icon \|set-cover \|add-screenshot \|rm-screenshot \|reorder ` | **Attach the store-listing media your App needs before it can be published** — an **icon and a cover are mandatory** (screenshots are optional, up to 8). `listing status` prints what is attached vs. what the publish floor still requires. See [After you submit](#after-you-submit-review--approve--deploy). | @@ -902,8 +902,36 @@ omitting the key: `ok` already accounts for `--strict`: it is `false` when there are hard errors, and also when `--strict` is passed and there are warnings. The process exit code -matches, and the JSON still goes to **stdout** while the failure is reported on -**stderr** — so `civitai app validate --json | jq` works on a failing project. +matches, and the JSON goes to **stdout** while the failure is reported on +**stderr** — so `civitai app validate --json | jq` works on a project that fails +*validation*. + +🔴 **BREAKING — a refused path now emits no object at all.** This object is +written only when validation actually produced a result. A path that does **not +exist**, or that is not a directory, is a mistake about the invocation: it writes +**nothing** to stdout and exits `2`. It used to print +`{"ok": false, "dir": "/nope", "errors": [ … ]}` and exit `1` — a fabricated +validation result, complete with a finding about a manifest nobody could have +written. A failure that produces **no validation result at all** likewise emits +no object and exits `1` — an unreadable manifest or a permissions error, and in +principle an internal schema failure, which is a directory the CLI *can* read +that still yields nothing to print. + +**So branch on the exit code before parsing:** + +| exit | stdout | +| --- | --- | +| `0` | the object, `"ok": true` | +| `1` | the object with `"ok": false` for a validation **verdict** — but **nothing** when validation produced no result at all (an unreadable manifest; also an internal schema failure, which a released binary should never hit) | +| `2` | **nothing** — the path does not exist, or is not a directory | + +```bash +out=$(civitai app validate ./my-block --json); rc=$? +case $rc in + 2) echo "bad path — check the argument"; exit 2 ;; + 0|1) [ -n "$out" ] && jq -e .ok <<<"$out" || echo "no result to parse (rc=$rc)" ;; +esac +``` ## Submit & auth @@ -1724,8 +1752,8 @@ by this — only `echo $?` differs. | Code | Meaning | | --- | --- | | `0` | Success. | -| `1` | Generic / unclassified error. A **filesystem failure** lands here — a file that exists but cannot be read, an unwritable config directory, an I/O error. It is neither a mistake about the invocation (`2`) nor a transport failure (`5`), and there is no filesystem-specific code. A resource that **exists but is not ready** lands here too, and deliberately not on `4`: `civitai app metrics ` for an app whose submitted version is still in review exits `1`, because the slug is right and the app does exist — only its analytics do not exist yet, and the error names `civitai app status ` as the next command. `4` stays reserved for a slug with no submissions at all, so the two remain separately actionable: fix the slug, versus wait for approval. | -| `2` | Usage error — a bad flag, a **missing required flag or argument** (e.g. `civitai app withdraw` with no publish-request id), a bad flag **value** (`--limit` out of range, a non-integer id, `--template nope`), or a request the API rejected as malformed (HTTP 400, e.g. a bad `--period`/`--sort` enum). This does not depend on where the refusal happens: a mistake the CLI catches locally and one the server rejects both exit `2`. A local image the CLI refuses before uploading anything (`civitai app listing set-icon `, `civitai generate --image`) exits `2` when the file is missing, empty, a directory, over the size cap, or not a PNG/JPEG/WebP — but a file that exists and cannot be **read** (permissions, an I/O error) is a filesystem failure rather than a mistake about the invocation, and exits `1`, not `2`. That split is the rule for **every local path a flag names**, not just images: `civitai generate --input ` likewise exits `2` for a path that is not there or is a directory, and `1` when the file is there and the read fails. `app listing set-cover` and `app listing add-screenshot` take the same positional `` and refuse it the same way. (The CLI has no `--file` image flag at all: the only `--file` is `civitai download --file`, which picks a file *inside* a model version.) | +| `1` | Generic / unclassified error. A **filesystem failure** lands here — a file that exists but cannot be read, an unwritable config directory, an I/O error. It is neither a mistake about the invocation (`2`) nor a transport failure (`5`), and there is no filesystem-specific code. A **validation verdict** lands here, and deliberately not on `2`: `civitai app validate` exits `1` when the manifest is invalid, and likewise when the directory you named is a real directory with no `block.manifest.json` at its root — you pointed at a real place, so the invocation was right and the project is wrong. (A path that does **not exist**, or that is not a directory, is the invocation being wrong, and exits `2`.) **When validation produces a result**, `civitai app validate --json` prints it in full and its `ok` field is the structured form of the same answer; a failure that produces no result at all — an unreadable manifest, say — still exits `1` with **nothing on stdout**, so branch on the exit code before parsing. A resource that **exists but is not ready** lands here too, and deliberately not on `4`: `civitai app metrics ` for an app whose submitted version is still in review exits `1`, because the slug is right and the app does exist — only its analytics do not exist yet, and the error names `civitai app status ` as the next command. `4` stays reserved for a slug with no submissions at all, so the two remain separately actionable: fix the slug, versus wait for approval. | +| `2` | Usage error — a bad flag, a **missing required flag or argument** (e.g. `civitai app withdraw` with no publish-request id), a bad flag **value** (`--limit` out of range, a non-integer id, `--template nope`), or a request the API rejected as malformed (HTTP 400, e.g. a bad `--period`/`--sort` enum). This does not depend on where the refusal happens: a mistake the CLI catches locally and one the server rejects both exit `2`. A local image the CLI refuses before uploading anything (`civitai app listing set-icon `, `civitai generate --image`) exits `2` when the file is missing, empty, a directory, over the size cap, or not a PNG/JPEG/WebP — but a file that exists and cannot be **read** (permissions, an I/O error) is a filesystem failure rather than a mistake about the invocation, and exits `1`, not `2`. That split is not images-only and it is not flags-only — it holds for **a flag's value and a positional argument alike**, over the paths listed here: `civitai generate --input ` likewise exits `2` for a path that is not there or is a directory, and `1` when the file is there and the read fails. The project commands take a positional path and refuse it the same way: `civitai app validate ` and `civitai app submit ` exit `2` when the path does not exist **or is not a directory**, because both are mistakes about the invocation. A directory that **does** exist but holds no `block.manifest.json` is a validation verdict instead, and exits `1`. `app listing set-cover` and `app listing add-screenshot` take the same positional `` and refuse it the same way. (The CLI has no `--file` image flag at all: the only `--file` is `civitai download --file`, which picks a file *inside* a model version.) **Paths outside that list are not covered, and mostly exit `1`.** `civitai app listing … --dir ` exits `1` (it reports "no `block.manifest.json` found in …", the same way it does for a directory that is really there but holds no manifest), and so does `civitai app submit … --out `. Both are stated rather than promised: this is a ledger of the paths the split is published for, not a claim about every path in the CLI. A usage error emits **no JSON object**, in every mode. `civitai app validate /nope --json` therefore writes nothing to stdout and exits `2`; it used to print `{"ok": false, …}` and exit `1`, which reported a nonexistent path as a validation result. Scripts that parsed that object must branch on the exit code first. | | `3` | Authentication/authorization — login required, token invalid/expired, or the credential lacks the needed scope (HTTP 401/403, or no token configured). **`civitai generate` refines this**: several of its failures are *not* credential problems but would otherwise land here or on `2`, so they exit `1` instead and a script never loops on `civitai login`. A **muted account or incomplete onboarding** arrives as a bare `403` that is byte-identical to a missing scope; **out of Buzz** and **generation disabled** arrive as `400` (the upstream 403 is re-thrown server-side as a tRPC `BAD_REQUEST`), which would otherwise read as "bad flags". See [Generate](#exit-codes-specific-to-generate). | | `4` | Not found — the requested resource does not exist. Usually an HTTP 404, but not always: some lookups answer `200` with an empty result set instead (`civitai app status ` for an unregistered slug, `civitai users get` for an unknown username), and those exit `4` too. The same question therefore exits the same way however the API happens to phrase the miss. | | `5` | Network/transport failure or service unavailable — dial/timeout, or HTTP 502/503/504 after retries. This is the code to **retry** on, so a **filesystem** failure never lands here however retryable its errno looks: a permissions or I/O problem does not fix itself, and a loop that sleeps and re-runs would never terminate. Those exit `1`. | diff --git a/internal/cmd/app_listing.go b/internal/cmd/app_listing.go index ea5910a..4afe048 100644 --- a/internal/cmd/app_listing.go +++ b/internal/cmd/app_listing.go @@ -98,6 +98,13 @@ func newListingClient() (*appapi.Client, error) { return appapi.NewWithSource(cfg.BaseURL(), auth.New(cfg), ""), nil } +// listingSlugResolveFailure is the prefix resolveListingSlug puts on a manifest +// failure. It is a named constant so a test can assert POSITIVELY that a row +// actually REACHED this function, rather than merely that it did not fail at one +// specific earlier gate — see TestUngatedPathFlagsAreNotUsageErrors, and AGENTS +// item 25 for why a denylist-of-one premise let the defect regenerate twice. +const listingSlugResolveFailure = "could not resolve the app — run this from your app directory (with block.manifest.json) or pass --slug" + // resolveListingSlug resolves the app slug from --slug or the manifest. func resolveListingSlug(lc listingCommon) (string, error) { if lc.slug != "" { @@ -105,7 +112,7 @@ func resolveListingSlug(lc listingCommon) (string, error) { } m, err := manifest.Load(lc.dir) if err != nil { - return "", fmt.Errorf("could not resolve the app — run this from your app directory (with block.manifest.json) or pass --slug: %w", err) + return "", fmt.Errorf(listingSlugResolveFailure+": %w", err) } if m.BlockID == "" { // The remedy is a flag, so this is a usage error (exit 2). The sibling diff --git a/internal/cmd/app_submit.go b/internal/cmd/app_submit.go index 17bea78..401ad2f 100644 --- a/internal/cmd/app_submit.go +++ b/internal/cmd/app_submit.go @@ -65,6 +65,19 @@ Defaults to the current directory.`, } out := cmd.OutOrStdout() + // 0. Classify the path the USER named. Same gate `app validate` + // uses — one rule, one place (resolveProjectDir, project_dir.go): + // a nonexistent path or a file exits 2, a real directory with no + // manifest keeps its validation verdict and exit 1. + // + // It runs UNCONDITIONALLY, ahead of --skip-validate, because it is + // not a validation check: `--skip-validate` waives our opinion of + // the manifest, not the question of whether the directory the user + // typed exists at all. + if err := resolveProjectDir(dir); err != nil { + return err + } + // 1. Validate first — never submit a known-bad manifest. if !skipValidate { res, err := validate.Dir(dir) diff --git a/internal/cmd/app_validate.go b/internal/cmd/app_validate.go index 7335b42..f0c1e1f 100644 --- a/internal/cmd/app_validate.go +++ b/internal/cmd/app_validate.go @@ -66,6 +66,22 @@ Defaults to the current directory.`, out := cmd.OutOrStdout() errw := cmd.ErrOrStderr() + // Classify the path the USER named before validating anything. + // A path that is not there, or that is not a directory, is a + // mistake about the invocation (exit 2) — not a validation + // verdict. See resolveProjectDir (project_dir.go) for why the + // branch is here and not in internal/validate. + // + // 🔴 It runs BEFORE the --json block on purpose, and that is a + // deliberate wire break: `app validate /nope --json` used to print + // {"ok":false,"errors":[…]} and exit 1. It now prints NOTHING on + // stdout and exits 2, because a path that does not exist produced + // no validation result to report. That matches the CLI-wide + // convention that a usage error emits no JSON object. + if err := resolveProjectDir(dir); err != nil { + return err + } + res, err := validate.Dir(dir) if err != nil { return err diff --git a/internal/cmd/exitcodes_claims_test.go b/internal/cmd/exitcodes_claims_test.go index 528ddb5..215b306 100644 --- a/internal/cmd/exitcodes_claims_test.go +++ b/internal/cmd/exitcodes_claims_test.go @@ -80,12 +80,58 @@ func exitCodeContractClaims() []contractClaim { }, { code: 2, - name: "the missing-vs-unreadable split covers every local path flag, not just images", - phrases: []string{"every local path a flag names", "generate --input"}, - why: "stated generally because it was NOT general: --input was the counterexample, and a " + - "rule written only about images is one a future path flag can be added beside without " + - "anyone noticing it disagrees", - pinnedBy: "TestGenerateInputExitCodes (cmd/civitai) + TestReadGraphInputClassification", + name: "the missing-vs-unreadable split holds for a flag's value and a positional alike", + phrases: []string{"a flag's value and a positional argument alike", "generate --input"}, + why: "the shape of the rule, not its extent: a rule written only about images is one a future " + + "path flag can be added beside without anyone noticing it disagrees, and --input was that " + + "counterexample. It said \"every local path a FLAG names\" for a release, and the two " + + "commands that broke it — `app validate ` / `app submit `, issue #256 — take the " + + "path POSITIONALLY, so the sentence excluded exactly the cases that disagreed with it. " + + "🔴 The replacement then over-corrected to \"every local path the CLI is HANDED\", which is " + + "ALSO false and has a live counterexample INSIDE its own scope: `app listing --dir ` " + + "exits 1, measured identical on base and on this branch. So the sentence now publishes the " + + "shape over an enumerated ledger and states the residual, rather than quantifying over " + + "paths nobody has audited", + pinnedBy: "TestGenerateInputExitCodes (cmd/civitai) + TestReadGraphInputClassification + " + + "TestProjectDirExitCodes + TestUngatedPathFlagsAreNotUsageErrors (the residual)", + }, + { + code: 2, + name: "a project path that does not exist, or is not a directory, is 2", + phrases: []string{"app validate ", "app submit ", "or is not a directory"}, + why: "issue #256: `app validate /nope` reported the missing path as \"a project root without a " + + "manifest\" and exited 1, so a script could not tell a typo'd path from an app that " + + "genuinely fails validation — the one distinction the exit-code contract exists to draw", + pinnedBy: "TestProjectDirExitCodes + TestResolveProjectDirClassification", + }, + { + code: 1, + name: "a validation VERDICT is 1, and a manifest-less directory is a verdict", + phrases: []string{"validation verdict", "app validate", "no `block.manifest.json` at its root", + "app validate --json"}, + why: "the counterweight to the #256 fix: it would be easy to \"tidy\" the manifest-less " + + "directory onto 2 alongside the nonexistent path. It must stay 1 — the user pointed at a " + + "real place, so the invocation was right and the project is wrong, and that is the " + + "answer `--json`'s `ok` field reports", + pinnedBy: "TestProjectDirExitCodes (the control rows)", + }, + { + code: 1, + name: "`app validate --json` publishes a result only when it produced one", + phrases: []string{"When validation produces a result", "nothing on stdout", + "branch on the exit code before parsing"}, + why: "the note said `--json` \"prints the full result … so a script never has to read stderr\", " + + "full stop — and that is false for the failures that produce no Result at all. Measured on " + + "this branch: `app validate --json` exits 1 with stdout EMPTY, because " + + "validate.Dir returns an error rather than a Result for a non-ENOENT stat failure and for a " + + "schema() failure. An unqualified promise here is worse than silence: it tells a script " + + "author they may parse stdout unconditionally, on the one command whose whole job is to be " + + "machine-read. 🔴 The first scoping was ALSO wrong, in the other direction: it read \"for a " + + "project directory it could READ\", which still promises an object for the schema() arm — a " + + "directory the CLI can read perfectly well that yields no Result. The condition is whether " + + "validation PRODUCED a result, which is the thing the code actually branches on, so that is " + + "what the sentence now says", + pinnedBy: "TestValidateJSONOnlyEmitsAResultItActuallyProduced (+ its readable-dir positive control)", }, { code: 5, diff --git a/internal/cmd/exitcodes_doc.go b/internal/cmd/exitcodes_doc.go index befbd67..3c610d7 100644 --- a/internal/cmd/exitcodes_doc.go +++ b/internal/cmd/exitcodes_doc.go @@ -75,6 +75,7 @@ var exitCodeDocs = []ExitCodeDoc{ Summary: "Generic / unclassified error.", Notes: []string{ "A **filesystem failure** lands here — a file that exists but cannot be read, an unwritable config directory, an I/O error. It is neither a mistake about the invocation (`2`) nor a transport failure (`5`), and there is no filesystem-specific code.", + "A **validation verdict** lands here, and deliberately not on `2`: `civitai app validate` exits `1` when the manifest is invalid, and likewise when the directory you named is a real directory with no `block.manifest.json` at its root — you pointed at a real place, so the invocation was right and the project is wrong. (A path that does **not exist**, or that is not a directory, is the invocation being wrong, and exits `2`.) **When validation produces a result**, `civitai app validate --json` prints it in full and its `ok` field is the structured form of the same answer; a failure that produces no result at all — an unreadable manifest, say — still exits `1` with **nothing on stdout**, so branch on the exit code before parsing.", "A resource that **exists but is not ready** lands here too, and deliberately not on `4`: `civitai app metrics ` for an app whose submitted version is still in review exits `1`, because the slug is right and the app does exist — only its analytics do not exist yet, and the error names `civitai app status ` as the next command. `4` stays reserved for a slug with no submissions at all, so the two remain separately actionable: fix the slug, versus wait for approval.", }, }, @@ -84,10 +85,13 @@ var exitCodeDocs = []ExitCodeDoc{ Notes: []string{ "This does not depend on where the refusal happens: a mistake the CLI catches locally and one the server rejects both exit `2`.", "A local image the CLI refuses before uploading anything (`civitai app listing set-icon `, `civitai generate --image`) exits `2` when the file is " + joinPhrases(imageUsageRefusals) + " — but a file that exists and cannot be **read** (permissions, an I/O error) is a filesystem failure rather than a mistake about the invocation, and exits `1`, not `2`.", - "That split is the rule for **every local path a flag names**, not just images: `civitai generate --input ` likewise exits `2` for a path that is not there or is a directory, and `1` when the file is there and the read fails.", + "That split is not images-only and it is not flags-only — it holds for **a flag's value and a positional argument alike**, over the paths listed here: `civitai generate --input ` likewise exits `2` for a path that is not there or is a directory, and `1` when the file is there and the read fails.", + "The project commands take a positional path and refuse it the same way: `civitai app validate ` and `civitai app submit ` exit `2` when the path does not exist **or is not a directory**, because both are mistakes about the invocation. A directory that **does** exist but holds no `block.manifest.json` is a validation verdict instead, and exits `1`.", }, Extra: []string{ "`app listing set-cover` and `app listing add-screenshot` take the same positional `` and refuse it the same way. (The CLI has no `--file` image flag at all: the only `--file` is `civitai download --file`, which picks a file *inside* a model version.)", + "**Paths outside that list are not covered, and mostly exit `1`.** `civitai app listing … --dir ` exits `1` (it reports \"no `block.manifest.json` found in …\", the same way it does for a directory that is really there but holds no manifest), and so does `civitai app submit … --out `. Both are stated rather than promised: this is a ledger of the paths the split is published for, not a claim about every path in the CLI.", + "A usage error emits **no JSON object**, in every mode. `civitai app validate /nope --json` therefore writes nothing to stdout and exits `2`; it used to print `{\"ok\": false, …}` and exit `1`, which reported a nonexistent path as a validation result. Scripts that parsed that object must branch on the exit code first.", }, }, { diff --git a/internal/cmd/project_dir.go b/internal/cmd/project_dir.go new file mode 100644 index 0000000..a951d53 --- /dev/null +++ b/internal/cmd/project_dir.go @@ -0,0 +1,106 @@ +package cmd + +import ( + "fmt" + "os" + + "github.com/civitai/cli/internal/manifest" +) + +// resolveProjectDir classifies the path the user NAMED as an App project root, +// before any validation runs. It is the one gate `app validate` and `app submit` +// share; both take the same optional positional `[dir]`. +// +// The full rationale, the measured mutants, and the residual the published +// contract knowingly ships with are AGENTS.md item 25 — read it before changing +// any branch here. (It is deliberately NOT item 24: that is the +// transport-vs-filesystem predicate, a different rule on the same contract.) +// +// 🔴 IT STATS THE PATH THE USER TYPED — NEVER the joined manifest path, and that +// is the whole fix (issue #256). `validate.Dir` stats `/block.manifest.json` +// and branches on os.IsNotExist, which collapses two different mistakes into one +// verdict: +// +// - `civitai app validate /nope` reported "block.manifest.json not found at +// project root /nope" and exited 1 — a validation FINDING about a directory +// that is not there. The published contract says a path that does not exist +// is a mistake about the invocation and exits 2 (see exitCodeDocs code 2, +// and readGraphInput in generate_input.go, which already honours it for +// `generate --input`). +// - `civitai app validate README.md` fell through to the raw syscall and +// printed `stat README.md/block.manifest.json: not a directory` — a path the +// user never named, assembled by us, on exit 1. +// +// So the branch is three-way on the NAMED path: +// +// does not exist -> ErrUsage (exit 2) +// exists, not a dir -> ErrUsage (exit 2) +// a directory -> nil; validate.Dir decides, and a directory with no +// manifest keeps its existing finding and exit 1. +// +// That last row is deliberate and must stay: "this directory is not an App +// project" is a validation verdict about a real place the user pointed at, not a +// malformed invocation. Do not tag it. +// +// 🔴 IT LIVES IN internal/cmd, NOT internal/validate. ErrUsage is this package's +// sentinel (item 7), and `validate.Dir` returns a validation VERDICT — a Result +// — not an opinion about the command line. Pushing the tag down there would make +// internal/validate import the usage sentinel and would leak the exit-code +// contract into a package that has no business holding it. It also must not move +// into validate.ManifestOnly's path: `app init` self-checks a directory it just +// created, and has no user-named path to classify. +// +// Any OTHER stat failure (EACCES on a parent, ENOTDIR partway down a longer +// path) is returned UNTAGGED, so it exits 1 — the generic/filesystem code item +// 24 already assigns to exactly those shapes. `app validate /x.json` +// is one of the six invocations measured in #241, and 1 is the answer that issue +// settled on. +// The two exit-2 arms carry DIFFERENT remedies, and which arm gets which is +// part of the contract rather than cosmetic: "the path is not there" sends you +// to `app init`, while "you pointed at a file" sends you to the parent +// directory. Swapped, the CLI tells someone whose path is simply missing to +// pass "the ROOT, not a file" — advice about a file that does not exist — and +// tells someone who pointed at their manifest to scaffold a project they +// already have. +// +// 🔴 They are named constants because a swap is otherwise INVISIBLE: measured +// on this branch, exchanging the two format strings passed the ENTIRE suite +// (0 failures). Every classification assertion uses errors.Is and both arms +// carry the same ErrUsage sentinel, so nothing downstream can tell them apart, +// and the one message test only asks that the path the user typed appears — +// which both spellings do. That is AGENTS item 21(f)'s recorded shape: the +// operand ORDER of a message is a contract no exit-code assertion can see. +// TestProjectDirRemediesMatchTheirArm requires each arm to carry its own AND +// NOT the other's, and pins that the two are non-empty and distinct — an empty +// or duplicated remedy would make `strings.Contains` vacuously true and disarm +// the whole guard. +const ( + remedyNoSuchDir = "%s: no such directory — pass the path to an App project root, or scaffold one with `civitai app init `" + remedyNotADir = "%s is not a directory — pass the App project ROOT (the directory holding %s), not a file" +) + +func resolveProjectDir(dir string) error { + info, err := os.Stat(dir) + if err != nil { + if os.IsNotExist(err) { + return asUsageError(fmt.Errorf(remedyNoSuchDir, dir)) + } + // Returned BARE, and that is not laziness. os.Stat's error is an + // *fs.PathError whose Error() already reads `stat : `, so + // a `fmt.Errorf("stat %s: %w", dir, err)` wrapper printed the op and the + // path TWICE — measured on this branch before the fix: + // `Error: stat …/file.txt/x.json: stat …/file.txt/x.json: not a + // directory`, where the base binary printed one `stat`. There is no + // context left to add: we stat the path the USER typed, never the joined + // manifest path, so the PathError already names the right thing. + // + // Do not re-add a prefix; if you ever need one, it must not repeat `stat` + // or the path. The classification is unchanged either way — untagged is + // exit 1, per the comment above. + return err + } + if !info.IsDir() { + return asUsageError(fmt.Errorf(remedyNotADir, dir, manifest.Filename)) + } + return nil +} diff --git a/internal/cmd/project_dir_gate_test.go b/internal/cmd/project_dir_gate_test.go new file mode 100644 index 0000000..3b8a38b --- /dev/null +++ b/internal/cmd/project_dir_gate_test.go @@ -0,0 +1,565 @@ +package cmd + +import ( + "errors" + "fmt" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/civitai/cli/internal/manifest" +) + +// This file is the second half of the #256 guards, added after an audit found +// three ways the first half could go green while the contract was broken. +// project_dir_test.go pins the two exit-2 rows and the two verdict controls; +// everything here pins a clause that was STATED and unheld: +// +// 1. the gate runs AHEAD of --skip-validate (TestSubmitGateRunsBeforeSkipValidate), +// 2. the untagged stat arm stays untagged, on more than one row and more than +// one surface (TestStatFailuresBelowTheGateStayUntagged), +// 3. `--json` publishes a result only when validation produced one +// (TestValidateJSONOnlyEmitsAResultItActuallyProduced), +// 4. the paths the exit-code contract does NOT cover really are not covered +// (TestUngatedPathFlagsAreNotUsageErrors). +// +// 🔴 EVERY CLASSIFICATION ASSERTION IS errors.Is, NEVER MESSAGE TEXT (AGENTS +// item 7): ErrUsage carries no visible wording of its own, so an assertion on +// the string says nothing about `echo $?`. + +// euid0 reports whether this process can defeat mode bits, which makes every +// permission-denied fixture in this file unbuildable. +func euid0() bool { return os.Geteuid() == 0 } + +// TestSubmitGateRunsBeforeSkipValidate pins the ORDERING clause in +// app_submit.go's own comment, which had no test at all. +// +// 🔴 Measured: moving `resolveProjectDir` inside the `if !skipValidate` block — +// a one-line move that reads like a tidy-up — left the ENTIRE suite green while +// reverting `app submit --skip-validate` from exit 2 to exit 1. +// Only one test mentioned --skip-validate at all, and it used a VALID directory, +// so nothing observed the flag's interaction with the gate. +// +// The clause is not a nicety: `--skip-validate` waives our opinion of the +// MANIFEST. It cannot waive the question of whether the directory the user typed +// exists, because there is no manifest to have an opinion about. +func TestSubmitGateRunsBeforeSkipValidate(t *testing.T) { + root := newProjectDirRoot(t) + + for _, tc := range []struct { + name string + dir string + wantUsage bool + wantErr bool + why string + }{ + { + name: "nonexistent path", + dir: filepath.Join(root, "does", "not", "exist"), + wantUsage: true, + wantErr: true, + why: "the regression the audit built: with the gate moved inside the !skipValidate block " + + "this falls through to manifest.Load and comes back untagged, i.e. exit 1 — the exact " + + "collapse issue #256 fixed, re-opened by a flag", + }, + { + name: "regular file", + dir: filepath.Join(root, "notadir.txt"), + wantUsage: true, + wantErr: true, + why: "same shape, second arm of the gate: a file is a mistake about the invocation, not a project", + }, + { + name: "CONTROL: a real directory with no manifest", + dir: filepath.Join(root, "empty"), + wantUsage: false, + wantErr: true, + why: "the gate must let a real directory through even under --skip-validate. Without this row " + + "a build that tagged EVERY --skip-validate failure would pass the two rows above", + }, + { + name: "CONTROL: a valid project still packages", + dir: filepath.Join(root, "ok"), + wantUsage: false, + wantErr: false, + why: "--skip-validate must still do the thing it exists for", + }, + } { + t.Run(tc.name, func(t *testing.T) { + _, stderr, err := run(t, "app", "submit", tc.dir, + "--skip-validate", "--package-only", "--out", filepath.Join(t.TempDir(), "b.zip")) + + if !tc.wantErr { + if err != nil { + t.Fatalf("app submit --skip-validate %s must succeed: %v\n%s", tc.name, err, stderr) + } + return + } + if err == nil { + t.Fatalf("app submit --skip-validate must fail for %s (%s)", tc.name, tc.why) + } + if got := errors.Is(err, ErrUsage); got != tc.wantUsage { + t.Errorf("app submit --skip-validate, %s: errors.Is(err, ErrUsage) = %v, want %v\nWhy it matters: %s\nerr: %v", + tc.name, got, tc.wantUsage, tc.why, err) + } + }) + } +} + +// statFailureCase is a path whose os.Stat fails with something that is NEITHER +// ENOENT nor "it is there and is not a directory" — the third arm of +// resolveProjectDir, which returns the error UNTAGGED so it exits 1. +type statFailureCase struct { + name string + // build returns the path to hand the gate, or "" to skip (with reason). + build func(t *testing.T, root string) (path, skip string) + why string +} + +func statFailureCases() []statFailureCase { + return []statFailureCase{ + { + name: "ENOTDIR below a regular file", + build: func(t *testing.T, root string) (string, string) { + return filepath.Join(root, "notadir.txt", "sub"), "" + }, + why: "`app validate /x.json` is one of the six invocations measured in issue " + + "#241, and exit 1 (generic/filesystem) is the answer that issue settled on", + }, + { + name: "EACCES on an unsearchable parent", + build: func(t *testing.T, root string) (string, string) { + if euid0() { + return "", "running as root: mode bits do not deny this process" + } + parent := filepath.Join(root, "locked") + if err := os.Mkdir(parent, 0o755); err != nil { + t.Fatal(err) + } + child := filepath.Join(parent, "proj") + if err := os.Mkdir(child, 0o755); err != nil { + t.Fatal(err) + } + if err := os.Chmod(parent, 0o000); err != nil { + t.Fatal(err) + } + // t.TempDir's own cleanup cannot walk a 000 directory. + t.Cleanup(func() { _ = os.Chmod(parent, 0o755) }) + return child, "" + }, + why: "a permissions failure on the way to the project root is a filesystem problem, not a " + + "malformed invocation — and it is a SECOND shape, so a mutant that only survives on " + + "ENOTDIR cannot hide behind one row", + }, + } +} + +// TestStatFailuresBelowTheGateStayUntagged is the F6 repair: the widening +// mutant — turning resolveProjectDir's untagged arm into `asUsageError` — used +// to redden EXACTLY ONE leaf subtest, and that subtest could `t.Skip` itself on +// a filesystem that resolves a path below a regular file. A battery resting on +// one skippable row is AGENTS item 24's recorded failure shape. +// +// So: two independent stat shapes × three surfaces (the helper, `app validate`, +// `app submit`), each row asserting its OWN PREMISE — an independent os.Stat +// must fail with something that is neither ENOENT nor a live non-directory — +// so a row whose fixture quietly stops reaching the untagged arm FAILS instead +// of continuing to look like coverage. Plus a count floor. +func TestStatFailuresBelowTheGateStayUntagged(t *testing.T) { + var reached int + + for _, c := range statFailureCases() { + t.Run(c.name, func(t *testing.T) { + root := newProjectDirRoot(t) + path, skip := c.build(t, root) + if skip != "" { + t.Skip(skip) + } + + // PREMISE, asserted independently of the implementation: this + // fixture must actually reach the third arm. If os.Stat succeeds, + // or fails with ENOENT, the row is exercising a different branch + // and its green says nothing about the untagged one. + info, serr := os.Stat(path) + if serr == nil { + t.Skipf("this filesystem resolves %s (mode %v) — the untagged arm is unreachable here", path, info.Mode()) + } + if os.IsNotExist(serr) { + t.Fatalf("PREMISE BROKEN: %s stats as ENOENT (%v), so this row exercises the exit-2 arm, "+ + "not the untagged one. It would stay green under the widening mutant and is not evidence.", path, serr) + } + reached++ + + t.Run("resolveProjectDir", func(t *testing.T) { + err := resolveProjectDir(path) + if err == nil { + t.Fatal("the gate must surface the stat failure") + } + if errors.Is(err, ErrUsage) { + t.Errorf("a stat failure that is neither ENOENT nor a non-directory must stay UNTAGGED (exit 1).\n"+ + "Why: %s\nerr: %v", c.why, err) + } + }) + + for _, cmdName := range []string{"validate", "submit"} { + t.Run("app "+cmdName, func(t *testing.T) { + args := []string{"app", cmdName, path} + if cmdName == "submit" { + args = append(args, "--package-only", "--out", filepath.Join(t.TempDir(), "b.zip")) + } + _, _, err := run(t, args...) + if err == nil { + t.Fatalf("app %s must fail for %s", cmdName, c.name) + } + if errors.Is(err, ErrUsage) { + t.Errorf("app %s classified a filesystem stat failure as a USAGE error (exit 2).\n"+ + "Why that is wrong: %s\nerr: %v", cmdName, c.why, err) + } + }) + } + }) + } + + // COUNT FLOOR. A silent skip on every row is indistinguishable from a + // battery wired to nothing, which is precisely how the single-row version + // of this guard could have gone quiet. + if reached < 1 { + t.Fatalf("no stat-failure fixture reached the untagged arm (%d of %d rows) — this battery is proving nothing", + reached, len(statFailureCases())) + } + if testing.Short() { + return + } + if reached < 2 { + t.Errorf("only %d of %d stat-failure shapes ran; the whole point of this file is that the widening "+ + "mutant must not rest on a single row", reached, len(statFailureCases())) + } +} + +// TestProjectDirStatErrorDoesNotStutter is F5. +// +// os.Stat returns an *fs.PathError whose Error() ALREADY begins `stat : `, +// so the `fmt.Errorf("stat %s: %w", dir, err)` wrapper printed the op and the +// path twice: measured on the PR tip, +// `Error: stat …/file.txt/x.json: stat …/file.txt/x.json: not a directory`, +// where the base binary printed one `stat`. +// +// The assertion COUNTS occurrences rather than matching a golden string — the +// defect is a duplicate, and a duplicate is a count. +func TestProjectDirStatErrorDoesNotStutter(t *testing.T) { + root := newProjectDirRoot(t) + path := filepath.Join(root, "notadir.txt", "sub") + if _, err := os.Stat(path); err == nil || os.IsNotExist(err) { + t.Skip("this filesystem does not produce ENOTDIR below a regular file") + } + + err := resolveProjectDir(path) + if err == nil { + t.Fatal("the gate must surface the stat failure") + } + msg := err.Error() + if n := strings.Count(msg, "stat "); n != 1 { + t.Errorf("the message says %q %d time(s), want 1 — os.Stat's own *fs.PathError already carries it:\n%s", + "stat ", n, msg) + } + if n := strings.Count(msg, path); n != 1 { + t.Errorf("the message names %s %d time(s), want 1:\n%s", path, n, msg) + } + // It must still name the path the USER typed — the whole point of the gate + // is that the base binary named the JOINED manifest path instead. + if !strings.Contains(msg, path) { + t.Errorf("the message must name the path the user typed (%s):\n%s", path, msg) + } +} + +// TestValidateJSONOnlyEmitsAResultItActuallyProduced is F2: the published code-1 +// note used to promise `app validate --json` "prints the full result … so a +// script never has to read stderr", full stop. +// +// 🔴 Measured on the PR tip: `civitai app validate --json` +// exits 1 with stdout EMPTY (0 bytes) and the error on stderr only, because +// validate.Dir returns an `error` rather than a Result for a non-ENOENT stat +// failure. The note is now scoped to "a project directory it could READ", and +// this test is the behaviour that scoping describes. +// +// The positive control is what makes the empty-stdout row mean anything: a +// --json mode that printed nothing ever would satisfy the first half alone. +func TestValidateJSONOnlyEmitsAResultItActuallyProduced(t *testing.T) { + if euid0() { + t.Skip("running as root: mode bits do not deny this process") + } + root := newProjectDirRoot(t) + + // A real, well-formed project whose manifest cannot be READ. The directory + // itself stats fine, so it passes the gate; validate.Dir then fails on the + // manifest and returns an error instead of a Result. + unreadable := filepath.Join(root, "unreadable") + if err := os.Mkdir(unreadable, 0o755); err != nil { + t.Fatal(err) + } + writeStaticManifest(t, unreadable) + if err := os.Chmod(unreadable, 0o000); err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _ = os.Chmod(unreadable, 0o755) }) + + stdout, stderr, err := run(t, "app", "validate", unreadable, "--json") + if err == nil { + t.Fatal("an unreadable project must fail") + } + if errors.Is(err, ErrUsage) { + t.Errorf("an unreadable project directory is a FILESYSTEM failure (exit 1), not a usage error (exit 2):\n%v", err) + } + if strings.TrimSpace(stdout) != "" { + t.Errorf("validation produced no result, so --json must emit no object.\n"+ + "If this now prints one, the code-1 note in exitcodes_doc.go is under-scoped again — "+ + "re-widen it rather than deleting this test.\nstdout:\n%s", stdout) + } + if strings.TrimSpace(stderr) == "" && err.Error() == "" { + t.Error("the failure has to be reported somewhere") + } + + // POSITIVE CONTROL: for a directory it CAN read, the object is still there + // and `ok` still carries the verdict — the half of the claim that survives. + stdout, _, err = run(t, "app", "validate", filepath.Join(root, "empty"), "--json") + if err == nil { + t.Fatal("a directory with no manifest must still fail validation") + } + if errors.Is(err, ErrUsage) { + t.Error("a directory with no manifest is a validation verdict, not a usage error") + } + if !strings.Contains(stdout, `"ok"`) { + t.Errorf("--json must still emit the result object for a directory it could read:\n%s", stdout) + } +} + +// TestProjectDirRemediesMatchTheirArm pins WHICH remedy each exit-2 arm +// carries, which no errors.Is assertion can see: both arms tag ErrUsage, so +// they are indistinguishable downstream, and the pre-existing message test only +// requires the path the user typed to appear — true of both spellings. +// +// 🔴 Measured before this guard existed: exchanging the two format strings +// passed the ENTIRE suite, 0 failures. The result is advice that is exactly +// backwards — a missing path told to "pass the ROOT, not a file", a manifest +// path told to scaffold a project the author already has. AGENTS item 21(f) +// records the same class for the substitution reporter, and prescribes this +// shape: derive the expected text from the CONSTANT and require the other arm's +// to be ABSENT. +// 🔴 BOTH REMEDIES ARE RENDERED WITH THE **SAME** PATH, and that is the whole +// mechanic. Each remedy interpolates the path, so rendering the two with +// DIFFERENT paths (the missing one vs the file) silently disarmed half the +// guard: +// +// - the "other arm's text is ABSENT" assertion could never fire, because +// `Contains(err, notDir)` where `notDir` carries a path the error never +// mentions is false no matter what the code does. Measured at `ab1e685`: +// the BOTH-ARM swap gave 2 kills, all 2 from `want`, **0 from `deny`** +// (the one-arm swap gave 1, also entirely from `want`). +// - the "distinct" precondition compared two strings that differed only by +// their path, so it passed even for IDENTICAL remedy constants. +// +// 🔴 THE DUPLICATE THAT MATTERS IS ARITY-PRESERVING, and an earlier revision of +// this comment got that backwards — it said a duplicate "breaks `go vet`", which +// is true of exactly one spelling and false of the ones a refactor produces. +// Measured at `ab1e685`, all three built as real edits to these two constants: +// +// shape build vet suite +// A copy the body over verbatim (arity 2→1) ok rc=1 1 `build failed`, 0 `--- FAIL` +// B same text + trailing `%.0s` (arity kept) ok rc=0 rc 0, 18 pkgs ok, 0 `--- FAIL` +// C same advice + ` (looking for %s)` ok rc=0 rc 0, 18 pkgs ok, 0 `--- FAIL` +// +// So B and C shipped the backwards advice **fully green** — `go build` clean, +// `go vet` clean, whole suite clean — while the binary answered a real file with +// `…/afile.txt: no such directory — … scaffold one with `civitai app init“ +// at rc 2. Only A is caught by vet, and even A **builds**: what stops it is CI +// running `go test`, not a compile failure. At HEAD, B is caught by the +// precondition and C by the `deny` assertion. +// +// So the remedies are rendered by helpers taking the path, the precondition +// runs on one shared probe path, and each row renders both arms with its own +// `tc.dir`. This guard is the live protection, not a backstop for a future +// refactor. +func noSuchAt(p string) string { return fmt.Sprintf(remedyNoSuchDir, p) } +func notDirAt(p string) string { return fmt.Sprintf(remedyNotADir, p, manifest.Filename) } + +// remediesAreDistinct reports whether two remedy renderers produce different +// text for the SAME path. +// +// It is a named helper purely so the positive control below can hand it a KNOWN +// DUPLICATE pair and require it to say so. The previous attempt at that control +// was `dup != noSuchAt(probe) || dup == notDirAt(probe)` with +// `dup := noSuchAt(probe)`: clause 1 is `s == s` on a deterministic pure call +// and is false in every possible state, and clause 2 was byte-identical to the +// precondition immediately above it, which has already called t.Fatalf. No +// change to production code could reach its failure branch — it was labelled +// POSITIVE CONTROL and could not fail. (staticcheck's SA4000 misses that +// spelling because it is `var != f(x)` rather than `f(x) != f(x)`.) +func remediesAreDistinct(a, b func(string) string, probe string) bool { + return a(probe) != b(probe) +} + +func TestProjectDirRemediesMatchTheirArm(t *testing.T) { + root := newProjectDirRoot(t) + missing := filepath.Join(root, "nope") + file := filepath.Join(root, "notadir.txt") + + // PRECONDITION, on ONE path so it compares the remedies and nothing else. + // Non-empty, because `strings.Contains(x, "")` is always true and would make + // every `want` vacuous; and DISTINCT, because two identical remedies make + // every `deny` unsatisfiable — the arms would be indistinguishable and a + // swap undetectable. + const probe = "/probe/path" + + // POSITIVE CONTROL, and it runs FIRST so it cannot be shadowed by the + // precondition it validates: hand the comparator two IDENTICAL renderers and + // require it to report them as not distinct. A comparator nobody has watched + // reject anything is indistinguishable from one wired to nothing, and that + // is what makes every `deny` assertion below meaningful rather than assumed. + if remediesAreDistinct(noSuchAt, noSuchAt, probe) { + t.Fatal("remediesAreDistinct called two IDENTICAL renderers distinct — it cannot reject anything, " + + "so the precondition below is vacuous and nothing guards the `deny` assertions") + } + + if strings.TrimSpace(noSuchAt(probe)) == "" || strings.TrimSpace(notDirAt(probe)) == "" { + t.Fatalf("a remedy rendered empty — every assertion in this test would be vacuous\nnoSuch=%q notDir=%q", + noSuchAt(probe), notDirAt(probe)) + } + // 🔴 THIS PRECONDITION IS THE LIVE PROTECTION AGAINST A DUPLICATE, not a + // backstop for some future refactor. Measured at `ab1e685` (before the + // same-path repair), two ARITY-PRESERVING duplicates — same text plus a + // trailing `%.0s` to consume arg 2, and the same advice plus + // ` (looking for %s)`, which is a completely natural way to write it — both + // `go build` clean AND `go vet` clean, and both survived the entire suite at + // rc 0 / 18 pkgs ok / 0 `--- FAIL` while the binary answered a real file + // with "no such directory … scaffold one with `civitai app init`" at rc 2. + // At HEAD the `%.0s` shape is caught HERE and the `(looking for %s)` shape by + // the `deny` assertion below. + if !remediesAreDistinct(noSuchAt, notDirAt, probe) { + t.Fatalf("the two remedies are IDENTICAL when rendered with the same path (%q), so this guard cannot "+ + "tell the arms apart and a swap is undetectable. One constant has been given the other's text.", + noSuchAt(probe)) + } + + for _, tc := range []struct { + name string + dir string + // want/deny are rendered from tc.dir, so both strings describe the SAME + // path and the deny assertion is about the remedy rather than the path. + want, deny func(string) string + why string + }{ + { + name: "nonexistent path gets the `app init` remedy", + dir: missing, want: noSuchAt, deny: notDirAt, + why: "the path is not there, so there is no file to have pointed at — telling the user to " + + "pass the ROOT rather than a file is advice about something that does not exist", + }, + { + name: "a regular file gets the project-ROOT remedy", + dir: file, want: notDirAt, deny: noSuchAt, + why: "the user has a real project and pointed one level too deep (typically at the manifest) — " + + "telling them to scaffold a new one with `app init` sends them to create what they already have", + }, + } { + t.Run(tc.name, func(t *testing.T) { + err := resolveProjectDir(tc.dir) + if err == nil { + t.Fatal("must be refused") + } + if !errors.Is(err, ErrUsage) { + t.Fatalf("premise: both arms must be usage errors, got %v", err) + } + want, deny := tc.want(tc.dir), tc.deny(tc.dir) + if got := err.Error(); !strings.Contains(got, want) { + t.Errorf("wrong remedy for this arm.\nWhy it matters: %s\nwant it to contain: %s\ngot: %s", tc.why, want, got) + } + if got := err.Error(); strings.Contains(got, deny) { + t.Errorf("this arm carries the OTHER arm's remedy — the two are swapped.\nWhy it matters: %s\ngot: %s", tc.why, got) + } + }) + } +} + +// TestUngatedPathFlagsAreNotUsageErrors is F3: the code-2 note claimed the +// missing-vs-unreadable split was the rule for "every local path the CLI is +// handed — a flag's value and a positional argument alike". That quantifier had +// a live counterexample INSIDE its own scope, measured identical on base and on +// this branch: `civitai app listing status --dir /does/not/exist` exits 1. +// +// The sentence is now scoped to the enumerated ledger and the residual is stated +// in the README. This test is what keeps the two in step: it FAILS if the +// residual closes, which is the moment the README paragraph becomes wrong. +// +// It is deliberately NOT an argument that exiting 1 here is right. It is a +// record that the published contract does not claim otherwise. +// 🔴 IT NEEDS A CREDENTIAL, AND WITHOUT ONE IT IS INERT ON CI — which is the +// only environment that matters for a guard. `app listing status` calls +// `newListingClient()` BEFORE `resolveListingSlug`, so with no token configured +// it fails at `no token configured` (an `ErrUnauthorized`) and never reaches the +// `--dir` path at all. The bare `!errors.Is(err, ErrUsage)` assertion is +// satisfied by that auth failure, so the residual-closing change this test +// exists to catch left it PASSING. +// +// Measured before the fix, with HOME and XDG_CONFIG_HOME pointed at an empty +// directory (what `ubuntu-latest` looks like): both rows PASS, and they still +// pass with `resolveProjectDir` wired into `resolveListingSlug` — the mutant is +// invisible. On a developer box with a real config it happens to observe the +// manifest error instead, so the guard looked fine locally and was dead in CI. +// +// So: a dummy token gets past the client constructor (no network is reached — +// `resolveListingSlug` fails on `manifest.Load` first), and the PREMISE is +// asserted. A row that stops reaching the thing it tests must FAIL, not pass. +func TestUngatedPathFlagsAreNotUsageErrors(t *testing.T) { + root := newProjectDirRoot(t) + + for _, tc := range []struct { + name string + args []string + }{ + { + name: "app listing status --dir ", + args: []string{"app", "listing", "status", "--dir", filepath.Join(root, "does", "not", "exist")}, + }, + { + name: "app listing status --dir ", + args: []string{"app", "listing", "status", "--dir", filepath.Join(root, "notadir.txt")}, + }, + } { + t.Run(tc.name, func(t *testing.T) { + // Past newListingClient()'s credential gate, which sits AHEAD of the + // --dir resolution. The value is never sent anywhere: the command + // fails at manifest.Load before any request is built. + t.Setenv("CIVITAI_TOKEN", "dummy-not-sent-anywhere") + + _, _, err := run(t, tc.args...) + if err == nil { + t.Fatalf("%s must fail", tc.name) + } + // 🔴 PREMISE — POSITIVE, not a denylist. The first version asserted + // only that the error was NOT civitai.ErrUnauthorized, which closes + // the one gate we already knew about and says nothing about whether + // the row reached `--dir`. Measured: inserting ANY new preflight + // ahead of resolveListingSlug that fails with a plain untagged error + // left both rows PASSING — and still passing with the + // residual-closing mutant also applied, so the defect regenerated + // completely and invisibly. + // + // So the premise is evidence the path EXECUTED: resolveListingSlug's + // own wrapper, derived from the production constant rather than + // spelled here, so a reword moves both together. + if !strings.Contains(err.Error(), listingSlugResolveFailure) { + t.Fatalf("PREMISE BROKEN: %s never reached resolveListingSlug — the error does not carry its "+ + "wrapper, so something earlier in the command failed first and this row asserts nothing "+ + "about --dir.\nwant it to contain: %s\nerr: %v", tc.name, listingSlugResolveFailure, err) + } + if errors.Is(err, ErrUsage) { + t.Errorf("%s now exits 2. That may well be an improvement — but the code-2 Extra note in "+ + "exitcodes_doc.go currently tells scripts this path exits 1, and the README's ledger "+ + "paragraph says the split is published only for the paths it enumerates. Bring --dir "+ + "into the ledger (and into resolveProjectDir's set) in the same change, or this test is "+ + "the only thing that noticed the docs went stale.\nerr: %v", tc.name, err) + } + }) + } +} diff --git a/internal/cmd/project_dir_test.go b/internal/cmd/project_dir_test.go new file mode 100644 index 0000000..419b734 --- /dev/null +++ b/internal/cmd/project_dir_test.go @@ -0,0 +1,384 @@ +package cmd + +import ( + "encoding/json" + "errors" + "go/ast" + "go/parser" + "go/token" + "os" + "path/filepath" + "strings" + "testing" +) + +// Issue #256. `civitai app validate /nope` reported a path that is not there as +// "block.manifest.json not found at project root /nope" and exited 1 — a +// validation FINDING about a directory nobody has. The published contract says a +// path that does not exist is a mistake about the invocation and exits 2, and +// `civitai generate --input /nope/x.json` has honoured that since #251. +// +// 🔴 EVERY ROW PINS THE CLASSIFICATION WITH errors.Is, NEVER MESSAGE TEXT +// (AGENTS item 7). The exit code is carried by the ErrUsage sentinel, which has +// no visible text of its own: `asUsageError` preserves the message byte for +// byte. So a test that asserts on wording says NOTHING about `echo $?` — that is +// the measured failure #251 recorded, where a one-token classification change +// moved a command's exit code through a fully green suite. +// +// The table is deliberately BOTH directions. The two exit-2 rows are the +// regression; the two control rows are what stop the obvious over-fix, which is +// to tag every path failure and make `app validate` report a genuinely broken +// project as a usage error. A build that tagged everything passes the first half +// and fails the second. + +// projectDirCase is one path shape and the classification it must get. +type projectDirCase struct { + name string + // dir is built from the temp root the harness prepares. + dir func(root string) string + // wantUsage is the CLASSIFICATION under test: true means ErrUsage (exit 2). + wantUsage bool + // wantErr is whether the command must fail at all. A valid project fails + // with neither. + wantErr bool + // why records what breaks if this row's answer changes. + why string +} + +// projectDirCases is shared by the `app validate` and `app submit` tables — the +// gate is ONE helper (resolveProjectDir), so the two commands must answer +// identically, and running the same rows through both is what makes that +// observable rather than assumed. +func projectDirCases() []projectDirCase { + return []projectDirCase{ + { + name: "path does not exist", + dir: func(root string) string { return filepath.Join(root, "does", "not", "exist") }, + wantUsage: true, + wantErr: true, + why: "the headline defect: it exited 1 and blamed a missing manifest, so a script could not " + + "tell a typo'd path from an app that genuinely fails validation", + }, + { + name: "path is a regular file", + dir: func(root string) string { return filepath.Join(root, "notadir.txt") }, + wantUsage: true, + wantErr: true, + why: "it fell through to the raw syscall and printed `stat notadir.txt/block.manifest.json: " + + "not a directory` — a path the user never named, on exit 1", + }, + { + name: "CONTROL: a real directory with no manifest", + dir: func(root string) string { return filepath.Join(root, "empty") }, + wantUsage: false, + wantErr: true, + why: "a validation VERDICT, not a usage error: the user pointed at a real place, so the " + + "invocation was right and the project is wrong. Tagging this would make the fix " + + "indistinguishable from `tag every path failure`", + }, + { + name: "CONTROL: a valid project", + dir: func(root string) string { return filepath.Join(root, "ok") }, + wantUsage: false, + wantErr: false, + why: "the gate must not refuse the case both commands exist to serve", + }, + } +} + +// newProjectDirRoot builds the fixture tree every row above indexes into. +func newProjectDirRoot(t *testing.T) string { + t.Helper() + root := t.TempDir() + + if err := os.WriteFile(filepath.Join(root, "notadir.txt"), []byte("not a project"), 0o600); err != nil { + t.Fatal(err) + } + if err := os.Mkdir(filepath.Join(root, "empty"), 0o755); err != nil { + t.Fatal(err) + } + ok := filepath.Join(root, "ok") + if err := os.Mkdir(ok, 0o755); err != nil { + t.Fatal(err) + } + writeStaticManifest(t, ok) + if err := os.WriteFile(filepath.Join(ok, "index.html"), []byte(""), 0o600); err != nil { + t.Fatal(err) + } + return root +} + +// TestProjectDirExitCodes drives the REAL commands through NewRootCmd, which is +// the only way the assertion covers the wiring as well as the helper: a +// resolveProjectDir that is correct and never called would pass a unit test and +// fail every row here. +func TestProjectDirExitCodes(t *testing.T) { + for _, cmdName := range []string{"validate", "submit"} { + t.Run(cmdName, func(t *testing.T) { + for _, c := range projectDirCases() { + t.Run(c.name, func(t *testing.T) { + root := newProjectDirRoot(t) + dir := c.dir(root) + + args := []string{"app", cmdName, dir} + if cmdName == "submit" { + // --package-only never submits and needs no token, so + // the valid-project row exercises the gate rather than + // the network. + args = append(args, "--package-only", "--out", filepath.Join(t.TempDir(), "b.zip")) + } + _, stderr, err := run(t, args...) + + if !c.wantErr { + if err != nil { + t.Fatalf("app %s %s must succeed: %v\n%s", cmdName, c.name, err, stderr) + } + return + } + if err == nil { + t.Fatalf("app %s must fail for %s (%s)", cmdName, c.name, c.why) + } + if got := errors.Is(err, ErrUsage); got != c.wantUsage { + t.Errorf("app %s, %s: errors.Is(err, ErrUsage) = %v, want %v\nWhy it matters: %s\nerr: %v", + cmdName, c.name, got, c.wantUsage, c.why, err) + } + }) + } + }) + } +} + +// TestProjectDirRefusalNamesThePathTheUserTyped is the message half, and it is +// deliberately a NEGATIVE assertion about one specific string rather than a +// positive one about wording. +// +// The old failure was not "an unhelpful message" — it was a message about a path +// the CLI ASSEMBLED. `validate.Dir` stats `/block.manifest.json`, so a +// regular file produced `stat README.md/block.manifest.json: not a directory`, +// naming a path that cannot exist and that the user never wrote. Asserting the +// joined form is ABSENT survives any rewording of the replacement; asserting the +// replacement's own text would not. +func TestProjectDirRefusalNamesThePathTheUserTyped(t *testing.T) { + root := newProjectDirRoot(t) + file := filepath.Join(root, "notadir.txt") + joined := filepath.Join(file, "block.manifest.json") + + for _, cmdName := range []string{"validate", "submit"} { + t.Run(cmdName, func(t *testing.T) { + args := []string{"app", cmdName, file} + if cmdName == "submit" { + args = append(args, "--package-only", "--out", filepath.Join(t.TempDir(), "b.zip")) + } + _, _, err := run(t, args...) + if err == nil { + t.Fatalf("app %s must fail", cmdName) + } + if strings.Contains(err.Error(), joined) { + t.Errorf("the error names a path the CLI assembled and the user never typed (%s):\n%v", joined, err) + } + if !strings.Contains(err.Error(), file) { + t.Errorf("the error must name the path the user DID type (%s):\n%v", file, err) + } + }) + } +} + +// TestValidateJSONEmitsNothingForARefusedPath pins the deliberate WIRE BREAK. +// +// `civitai app validate /nope --json` used to write +// {"ok":false,"dir":"/nope","errors":[…]} to stdout and exit 1 — i.e. it +// reported a path that does not exist as a validation RESULT, with a fabricated +// finding about a manifest nobody could have written. It now writes nothing and +// exits 2, matching the CLI-wide convention that a usage error emits no JSON +// object. +// +// The stdout assertion is the load-bearing half: the exit code alone is +// satisfied by a build that tags the error AND still prints the object, which +// would leave a parsing script reading a result for a path that is not there. +func TestValidateJSONEmitsNothingForARefusedPath(t *testing.T) { + root := newProjectDirRoot(t) + + for _, tc := range []struct { + name string + dir string + }{ + {"missing path", filepath.Join(root, "nope")}, + {"regular file", filepath.Join(root, "notadir.txt")}, + } { + t.Run(tc.name, func(t *testing.T) { + stdout, _, err := run(t, "app", "validate", tc.dir, "--json") + if err == nil { + t.Fatal("a refused path must still fail") + } + if !errors.Is(err, ErrUsage) { + t.Errorf("--json must not change the classification: errors.Is(err, ErrUsage) = false\nerr: %v", err) + } + if strings.TrimSpace(stdout) != "" { + t.Errorf("a usage error must emit no JSON object; stdout was:\n%s", stdout) + } + }) + } + + // POSITIVE CONTROL: --json still emits a real object for a path the gate + // ACCEPTS. Without it, "stdout is empty" is equally satisfied by a --json + // mode that never prints anything at all. + stdout, _, err := run(t, "app", "validate", filepath.Join(root, "empty"), "--json") + if err == nil { + t.Fatal("a directory with no manifest must still fail validation") + } + if errors.Is(err, ErrUsage) { + t.Error("a directory with no manifest is a validation verdict, not a usage error") + } + var payload map[string]any + if uerr := json.Unmarshal([]byte(stdout), &payload); uerr != nil { + t.Fatalf("--json must still emit the result object for a real directory: %v\nstdout: %s", uerr, stdout) + } + if ok, _ := payload["ok"].(bool); ok { + t.Errorf("the result object should report ok:false: %s", stdout) + } +} + +// TestResolveProjectDirClassification exercises the helper directly, including +// the shapes the command-level table cannot reach portably. +func TestResolveProjectDirClassification(t *testing.T) { + root := newProjectDirRoot(t) + + t.Run("nonexistent is a usage error", func(t *testing.T) { + err := resolveProjectDir(filepath.Join(root, "nope")) + if !errors.Is(err, ErrUsage) { + t.Errorf("want ErrUsage, got %v", err) + } + }) + t.Run("regular file is a usage error", func(t *testing.T) { + err := resolveProjectDir(filepath.Join(root, "notadir.txt")) + if !errors.Is(err, ErrUsage) { + t.Errorf("want ErrUsage, got %v", err) + } + }) + t.Run("a directory passes", func(t *testing.T) { + if err := resolveProjectDir(filepath.Join(root, "empty")); err != nil { + t.Errorf("a real directory must pass the gate, got %v", err) + } + }) + t.Run("CONTROL: the default `.` passes", func(t *testing.T) { + // Both commands default dir to "." when given no argument. A gate that + // refused it would break every bare `civitai app validate`. + if err := resolveProjectDir("."); err != nil { + t.Errorf("the default project dir must pass the gate, got %v", err) + } + }) + t.Run("CONTROL: ENOTDIR below a file is NOT a usage error", func(t *testing.T) { + // `app validate /x.json` is one of the six invocations + // measured in issue #241, and exit 1 (generic/filesystem) is the answer + // that issue settled on. It must not silently become a 2 here. + // + // 🔴 This row ASSERTS ITS OWN PREMISE. It used to be the ONLY leaf + // subtest the "tag every stat failure" widening reddened, and it carries + // a t.Skip — so a fixture that quietly stopped reaching the untagged arm + // would have taken the whole battery with it, silently. The wider, + // multi-shape battery now lives in project_dir_gate_test.go; this row + // stays because the helper-level assertion is cheap, but it no longer + // stands alone and it no longer skips for the wrong reason. + path := filepath.Join(root, "notadir.txt", "sub") + if _, serr := os.Stat(path); serr == nil { + t.Skip("this filesystem resolves a path below a regular file") + } else if os.IsNotExist(serr) { + t.Fatalf("PREMISE BROKEN: %s stats as ENOENT (%v) — this row would then be exercising the "+ + "exit-2 arm and would stay green under the widening mutant", path, serr) + } + err := resolveProjectDir(path) + if err == nil { + t.Fatal("the gate must surface the stat failure") + } + if errors.Is(err, ErrUsage) { + t.Errorf("a stat failure that is neither ENOENT nor a non-directory must stay untagged (exit 1): %v", err) + } + }) +} + +// TestEveryValidateDirCallerGatesOnResolveProjectDir is the SEAM guard, and it +// is the reason the branch is one shared helper rather than two copies. +// +// AGENTS.md's "one rule, one place": a predicate open-coded at N sites is +// typically wrong at N−1 of them. This defect WAS that shape — `app submit` had +// the identical hole and nothing connected the two, exactly as the four copies +// of item 24's transport predicate were found one at a time. So the ledger is +// asserted structurally: the set of files calling `validate.Dir` must equal the +// set of files calling `resolveProjectDir`. +// +// 🔴 It fails when the set GROWS (a third command validates a user-named +// directory without the gate) AND when it SHRINKS (someone deletes a gate, which +// would otherwise leave this file green and the ledger a false map). +// +// It deliberately does NOT cover `validate.ManifestOnly`: `app init` self-checks +// a directory it just created, so there is no user-named path to classify. +func TestEveryValidateDirCallerGatesOnResolveProjectDir(t *testing.T) { + fset := token.NewFileSet() + entries, err := os.ReadDir(".") + if err != nil { + t.Fatal(err) + } + + validateDirFiles := map[string]bool{} + gateFiles := map[string]bool{} + + for _, e := range entries { + name := e.Name() + if e.IsDir() || !strings.HasSuffix(name, ".go") || strings.HasSuffix(name, "_test.go") { + continue + } + f, perr := parser.ParseFile(fset, name, nil, 0) + if perr != nil { + t.Fatalf("parse %s: %v", name, perr) + } + ast.Inspect(f, func(n ast.Node) bool { + call, ok := n.(*ast.CallExpr) + if !ok { + return true + } + switch fn := call.Fun.(type) { + case *ast.SelectorExpr: + // validate.Dir(...) — a package-qualified call. + pkg, ok := fn.X.(*ast.Ident) + if ok && pkg.Name == "validate" && fn.Sel.Name == "Dir" { + validateDirFiles[name] = true + } + case *ast.Ident: + if fn.Name == "resolveProjectDir" { + gateFiles[name] = true + } + } + return true + }) + } + + // Positive control: the scanner must have found the sites at all. A pass + // built on two empty sets is a guard wired to nothing. + if len(validateDirFiles) < 2 { + t.Fatalf("found validate.Dir in %d file(s) (%v) — expected at least app_validate.go and app_submit.go; the scanner is looking at the wrong tree", + len(validateDirFiles), keysOf(validateDirFiles)) + } + + for f := range validateDirFiles { + if !gateFiles[f] { + t.Errorf("%s calls validate.Dir on a user-named path but never calls resolveProjectDir.\n"+ + "That is issue #256 regenerated at a new call site: a path that does not exist would be "+ + "reported as a project without a manifest and exit 1 instead of 2.", f) + } + } + for f := range gateFiles { + if !validateDirFiles[f] { + t.Errorf("%s calls resolveProjectDir but no longer calls validate.Dir — the ledger in this test "+ + "has gone stale. Either the gate moved (update this guard) or a validate.Dir call was "+ + "dropped (which is a behaviour change worth stating).", f) + } + } +} + +func keysOf(m map[string]bool) []string { + out := make([]string, 0, len(m)) + for k := range m { + out = append(out, k) + } + return out +}