From a4807f48ca54460e606055deb08fa62f83138925 Mon Sep 17 00:00:00 2001 From: ZacxDev Date: Fri, 7 Aug 2026 15:03:56 -0500 Subject: [PATCH 1/5] fix(exit-codes): a project path that does not exist is a usage error, not a manifest-less project MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `civitai app validate /nope` reported the missing path as "block.manifest.json not found at project root /nope" and exited 1; `civitai app validate README.md` printed `stat README.md/block.manifest.json: not a directory` — a path the CLI assembled and the user never typed — also on 1. The published contract says a path that does not exist is a mistake about the invocation and exits 2, and `generate --input` has honoured that since #251. `app submit ` had the identical hole. Root cause: 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". resolveProjectDir (internal/cmd/project_dir.go) branches three ways on the path the user NAMED — nonexistent -> ErrUsage (2), exists-but-not-a-directory -> ErrUsage (2), a real directory -> unchanged, so a manifest-less directory keeps its finding and exit 1 because the invocation was right and the project is wrong. It lives in internal/cmd because ErrUsage is this package's sentinel (AGENTS item 7) and validate.Dir returns a validation verdict; `app init`'s ManifestOnly self-check is untouched. One helper, both call sites, and the set is asserted by an AST guard that fails when it grows OR shrinks. The published code-2 note said "every local path a FLAG names", which excluded the two commands that broke it — both take the path positionally — so it is widened to "every local path the CLI is handed". `app validate`'s own exit code (a validation verdict is 1, and --json's `ok` is the structured form) is now documented under code 1. README + --help move together from exitCodeDocs. BREAKING (--json): `civitai app validate /nope --json` used to write {"ok":false,"errors":[...]} to stdout and exit 1. It now writes nothing and exits 2 — a path that does not exist produced no validation result — keeping the CLI-wide convention that a usage error emits no JSON object. Announced in the code-2 README cell. Closes #256. Co-Authored-By: Claude Opus 5 (1M context) --- AGENTS.md | 51 ++++ README.md | 4 +- internal/cmd/app_submit.go | 13 + internal/cmd/app_validate.go | 16 ++ internal/cmd/exitcodes_claims_test.go | 31 ++- internal/cmd/exitcodes_doc.go | 5 +- internal/cmd/project_dir.go | 67 +++++ internal/cmd/project_dir_test.go | 369 ++++++++++++++++++++++++++ 8 files changed, 549 insertions(+), 7 deletions(-) create mode 100644 internal/cmd/project_dir.go create mode 100644 internal/cmd/project_dir_test.go diff --git a/AGENTS.md b/AGENTS.md index 5d180ed..80bc4b0 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -1586,6 +1586,57 @@ neither one's. inconsistency from this bullet — and do not read the retraction as weakening the case against a code 7, which stands on the contract expansion alone. + 🔴 **AND THE SENTENCE THAT PUBLISHED THAT RULE EXCLUDED THE COMMANDS THAT + BROKE IT.** The code-2 note read "that split is the rule for **every local + path a FLAG names**" — so `civitai app validate ` and + `civitai app submit `, which take the path POSITIONALLY, were outside + the sentence and disagreed with it for a release. Issue #256: `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" — so `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 published note now says "every local path + the CLI is **handed** — a flag's value and a positional argument alike", + and `exitCodeContractClaims` carries a row for the widened wording so the + old, narrower sentence cannot come back quietly. + - **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 this whole item + 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: dropping the submit call alone reddens + 4 leaf subtests including this guard 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. Announced in the code-2 README cell. The gate therefore runs + BEFORE the `--json` block, and sliding it below is its own mutant: it + reddens exactly the JSON rows (3 leaf subtests) while the text-mode rows + stay green, so a table that only checked exit codes would miss it. + - **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; + a CONTROL row pins it so the fix cannot quietly widen into it. - 🔴 **THERE WERE TWO COPIES, AND FIXING ONE IS WHAT THIS ITEM NOW EXISTS TO PREVENT.** `pkg/civitai/retry.go`'s `isTransientNetErr` carried the IDENTICAL unfixed spelling through #242, and `syscall.Errno.Timeout()` is diff --git a/README.md b/README.md index 3108320..cff1246 100644 --- a/README.md +++ b/README.md @@ -1542,8 +1542,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`.) `civitai app validate --json` prints the full result and its `ok` field is the structured form of the same answer, so a script never has to read stderr. 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 the CLI is handed** — a flag's value and a **positional argument** alike, 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. 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.) 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_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..9254037 100644 --- a/internal/cmd/exitcodes_claims_test.go +++ b/internal/cmd/exitcodes_claims_test.go @@ -80,12 +80,35 @@ 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"}, + name: "the missing-vs-unreadable split covers every local path, flag OR positional", + phrases: []string{"every local path the CLI is handed", "positional argument", "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", + "anyone noticing it disagrees. 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", + pinnedBy: "TestGenerateInputExitCodes (cmd/civitai) + TestReadGraphInputClassification + TestProjectDirExitCodes", + }, + { + 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: 5, diff --git a/internal/cmd/exitcodes_doc.go b/internal/cmd/exitcodes_doc.go index befbd67..fa11138 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`.) `civitai app validate --json` prints the full result and its `ok` field is the structured form of the same answer, so a script never has to read stderr.", "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,12 @@ 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 the rule for **every local path the CLI is handed** — a flag's value and a **positional argument** alike, 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.", + "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.)", + "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..5069d4a --- /dev/null +++ b/internal/cmd/project_dir.go @@ -0,0 +1,67 @@ +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]`. +// +// 🔴 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. +func resolveProjectDir(dir string) error { + info, err := os.Stat(dir) + if err != nil { + if os.IsNotExist(err) { + return asUsageError(fmt.Errorf( + "%s: no such directory — pass the path to an App project root, or scaffold one with `civitai app init `", dir)) + } + return fmt.Errorf("stat %s: %w", dir, err) + } + if !info.IsDir() { + return asUsageError(fmt.Errorf( + "%s is not a directory — pass the App project ROOT (the directory holding %s), not a file", dir, manifest.Filename)) + } + return nil +} diff --git a/internal/cmd/project_dir_test.go b/internal/cmd/project_dir_test.go new file mode 100644 index 0000000..4aab9ef --- /dev/null +++ b/internal/cmd/project_dir_test.go @@ -0,0 +1,369 @@ +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. + err := resolveProjectDir(filepath.Join(root, "notadir.txt", "sub")) + if err == nil { + t.Skip("this filesystem resolves a path below a regular file") + } + 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 +} From 9b2ddab01a3ffb5052d50f512d5886ca3e4a67b1 Mon Sep 17 00:00:00 2001 From: ZacxDev Date: Fri, 7 Aug 2026 16:39:44 -0500 Subject: [PATCH 2/5] test(exit-codes): pin the --skip-validate ordering and the untagged stat arm; scope two published claims that overclaimed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Audit fixes on top of the #256 gate. Nothing here changes the exit code of any invocation; it closes the gap between what the contract SAYS and what anything observes, plus one message-stutter regression the gate introduced. F1 — `--skip-validate` ordering had ZERO test. `app_submit.go` states, in its own comment and in the PR body and in AGENTS.md, that `resolveProjectDir` runs ahead of `--skip-validate`. 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. TestSubmitGateRunsBeforeSkipValidate adds four rows (two exit-2, two controls); re-measured, that mutant now reddens 2 leaf subtests by name. F6 — the widening mutant rested on ONE skippable row. Re-tagging the untagged stat arm as asUsageError reddened exactly one leaf subtest, and that subtest carried a t.Skip when its fixture produced no error. That is AGENTS item 24's own recorded "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 another branch — plus a count floor. The pre-existing control row asserts its premise too. Re-measured: 1 -> 9 leaf subtests. F5 — message stutter. os.Stat returns an *fs.PathError whose Error() already begins `stat : `, so the wrapper printed the op and the path twice: `Error: stat …/file.txt/x.json: stat …/file.txt/x.json: not a directory`, where the base binary printed one `stat`. The error is now returned bare; the classification (untagged, exit 1) is unchanged. TestProjectDirStatErrorDoesNotStutter counts occurrences rather than matching a golden string, because the defect is a duplicate. F2 — the new code-1 note published a promise the code does not keep. It read "`app validate --json` prints the full result … so a script never has to read stderr", unqualified. Measured: `app validate --json` exits 1 with stdout EMPTY (0 bytes), because validate.Dir returns an error rather than a Result for a non-ENOENT stat failure and for a schema() failure. The note is SCOPED ("for a project directory it could read") rather than the code changed to match it, which would mean reworking every such arm. TestValidateJSONOnlyEmitsAResultItActuallyProduced pins it with a readable-dir positive control. F3 — the widened code-2 sentence still overclaimed. "Every local path the CLI is HANDED" has a live counterexample inside its own scope, measured identical on base and here: `civitai app listing status --dir /does/not/exist` exits 1, and so does `app submit --package-only --out /nodir/x.zip`. 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. `--dir` is deliberately NOT brought into the gate (that touches app_listing.go and is a behaviour change, not a docs fix). TestUngatedPathFlagsAreNotUsageErrors pins the residual and FAILS if it ever closes, which is the moment the published paragraph goes stale. F4 — the --json wire break was announced in exactly one place, and not the one a --json consumer reads. Added to the `app validate` row of the command table and to the canonical "The `--json` result shape" section, with an exit -> stdout table: 2 never carries an object, 1 carries one for a validation verdict but not for a filesystem failure, 0 always does. F7 — AGENTS.md placement. The new content was appended under item 24, whose index clause describes it as the ONE transport-vs-filesystem predicate. A project-path classification gate is not that predicate, so it moves to a new item 25 (items are append-only; nothing was renumbered) and the index clause is extended. Every `item N` reference was re-grepped; agents_index_test.go / agents_xrefs_test.go stay green. The README exit-code table is regenerated from exitCodeDocs, never hand-edited. Co-Authored-By: Claude Opus 5 (1M context) --- AGENTS.md | 172 ++++++++---- README.md | 36 ++- internal/cmd/exitcodes_claims_test.go | 37 ++- internal/cmd/exitcodes_doc.go | 5 +- internal/cmd/project_dir.go | 19 +- internal/cmd/project_dir_gate_test.go | 376 ++++++++++++++++++++++++++ internal/cmd/project_dir_test.go | 19 +- 7 files changed, 592 insertions(+), 72 deletions(-) create mode 100644 internal/cmd/project_dir_gate_test.go diff --git a/AGENTS.md b/AGENTS.md index 80bc4b0..e758a6e 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. @@ -1586,57 +1589,6 @@ neither one's. inconsistency from this bullet — and do not read the retraction as weakening the case against a code 7, which stands on the contract expansion alone. - 🔴 **AND THE SENTENCE THAT PUBLISHED THAT RULE EXCLUDED THE COMMANDS THAT - BROKE IT.** The code-2 note read "that split is the rule for **every local - path a FLAG names**" — so `civitai app validate ` and - `civitai app submit `, which take the path POSITIONALLY, were outside - the sentence and disagreed with it for a release. Issue #256: `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" — so `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 published note now says "every local path - the CLI is **handed** — a flag's value and a positional argument alike", - and `exitCodeContractClaims` carries a row for the widened wording so the - old, narrower sentence cannot come back quietly. - - **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 this whole item - 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: dropping the submit call alone reddens - 4 leaf subtests including this guard 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. Announced in the code-2 README cell. The gate therefore runs - BEFORE the `--json` block, and sliding it below is its own mutant: it - reddens exactly the JSON rows (3 leaf subtests) while the text-mode rows - stay green, so a table that only checked exit codes would miss it. - - **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; - a CONTROL row pins it so the fix cannot quietly widen into it. - 🔴 **THERE WERE TWO COPIES, AND FIXING ONE IS WHAT THIS ITEM NOW EXISTS TO PREVENT.** `pkg/civitai/retry.go`'s `isTransientNetErr` carried the IDENTICAL unfixed spelling through #242, and `syscall.Errno.Timeout()` is @@ -1915,6 +1867,120 @@ 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: dropping the submit call alone reddens 4 leaf subtests + including this guard by name. + - 🔴 **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: it reddens exactly the JSON rows (3 leaf subtests) while + the text-mode rows stay green, so a table that only checked exit codes + would miss it. 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 — "for a project directory it could **read**" — 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. + - **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 used to rest on ONE skippable row.** The + widening mutant — turning the untagged arm into `asUsageError`, i.e. "tag + every stat failure" — reddened exactly one leaf subtest, and that subtest + carried a `t.Skip` when the fixture produced no error, so a filesystem that + resolved the path would have taken the whole 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. + Re-measured: the widening mutant reddens **9 leaf subtests**, up from 1. + - **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 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. **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 cff1246..d6ced90 100644 --- a/README.md +++ b/README.md @@ -216,7 +216,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`** — calls the invite-gated mint route with your stored credential, reading scopes from your local `block.manifest.json` (so it works on an unsubmitted slug). `--spend` explicitly REQUESTS `ai:write:budgeted` (real Buzz); omit it and that scope is **filtered out** of the request — the CLI never asks for budgeted spend implicitly, even when your manifest declares it (the scaffolded money app does, so a live run that used to generate now needs `--spend`). Prints the token (`--env` prints `VITE_LIVE_BLOCK_TOKEN=`, paste-ready); warns at mint time if the token is read-only (can't spend). 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. Mints an **ephemeral in-memory ssh keypair**, opens a reverse tunnel from your dev port (start `npm run dev:tunnel` first) to the Civitai tunnel endpoint, prints the URL to open, and tears everything down on Ctrl-C or an idle timeout. Before minting it also **pre-flights whether the host can actually embed your dev server** — the host iframes it sandboxed (opaque `null` origin), so a dev server missing `Access-Control-Allow-Origin: *`, missing the `.civit.ai` entry in `allowedHosts`, or sending a framing header that excludes `civitai.com` loads as a blank iframe with no error anywhere. Those are printed as warnings (never fatal) **twice** — once the moment the checks run, so you still get them if you Ctrl-C the DNS wait, and again just above the URL, with the `vite.config.ts` fix. Apps scaffolded by `civitai app init --template page-money` already satisfy all of it. The tunnel endpoint (`sish.civitai.com:2224`) is **live**; access is gated behind an Apps-author invite **and** a server kill-switch flag, so if you are not enrolled the mint reports **"not available"** — ask to be added to the cohort. Publishing the tunnel host through external-dns + Cloudflare usually takes 1–3 min (occasionally longer); the command waits for it and prints the elapsed time. | -| `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 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). `civitai app submit` mints your listing as a **draft**, so you can set the media *while the app is in review* and it carries forward on approval. `listing status` prints what is attached vs. what the publish floor still requires. Source images are validated locally (png/jpeg/webp; icon ≤2 MiB, cover ≤4 MiB, screenshot ≤2 MiB) before upload, then wait for the content scan. On an **already-live** listing an attach opens a **revision** for moderator re-review (`--changelog`, `-y`). App resolved from `block.manifest.json` in the CWD, or `--slug`. See [After you submit](#after-you-submit-review--approve--deploy). | | `civitai app status [blockId] [--id ] [--json]` | Check the review/deploy status of **your own** submissions. No arg lists them all; a `blockId` (app slug) or `--id` shows one in detail (rejection reason if rejected, live URL once deployed). See [Submission status](#submission-status). | @@ -801,8 +801,34 @@ 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 project directory the CLI cannot **read** (an unreadable manifest, a +permissions error) likewise produces no object, and exits `1`. + +**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 the failure was a filesystem one | +| `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 @@ -1542,8 +1568,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 **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`.) `civitai app validate --json` prints the full result and its `ok` field is the structured form of the same answer, so a script never has to read stderr. 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 the CLI is handed** — a flag's value and a **positional argument** alike, 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. 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.) 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. | +| `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`.) For a project directory it could **read**, `civitai app validate --json` prints the full result and its `ok` field is the structured form of the same answer; a failure that produced 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/exitcodes_claims_test.go b/internal/cmd/exitcodes_claims_test.go index 9254037..5ca3f3d 100644 --- a/internal/cmd/exitcodes_claims_test.go +++ b/internal/cmd/exitcodes_claims_test.go @@ -80,15 +80,20 @@ func exitCodeContractClaims() []contractClaim { }, { code: 2, - name: "the missing-vs-unreadable split covers every local path, flag OR positional", - phrases: []string{"every local path the CLI is handed", "positional argument", "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. 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", - pinnedBy: "TestGenerateInputExitCodes (cmd/civitai) + TestReadGraphInputClassification + TestProjectDirExitCodes", + 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, @@ -110,6 +115,20 @@ func exitCodeContractClaims() []contractClaim { "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{"For a project directory it could", "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", + pinnedBy: "TestValidateJSONOnlyEmitsAResultItActuallyProduced (+ its readable-dir positive control)", + }, { code: 5, name: "5 is the retry code and a filesystem failure never lands there", diff --git a/internal/cmd/exitcodes_doc.go b/internal/cmd/exitcodes_doc.go index fa11138..788b960 100644 --- a/internal/cmd/exitcodes_doc.go +++ b/internal/cmd/exitcodes_doc.go @@ -75,7 +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`.) `civitai app validate --json` prints the full result and its `ok` field is the structured form of the same answer, so a script never has to read stderr.", + "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`.) For a project directory it could **read**, `civitai app validate --json` prints the full result and its `ok` field is the structured form of the same answer; a failure that produced 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.", }, }, @@ -85,11 +85,12 @@ 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 the CLI is handed** — a flag's value and a **positional argument** alike, 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 index 5069d4a..902f1c9 100644 --- a/internal/cmd/project_dir.go +++ b/internal/cmd/project_dir.go @@ -11,6 +11,11 @@ import ( // 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 @@ -57,7 +62,19 @@ func resolveProjectDir(dir string) error { return asUsageError(fmt.Errorf( "%s: no such directory — pass the path to an App project root, or scaffold one with `civitai app init `", dir)) } - return fmt.Errorf("stat %s: %w", dir, err) + // 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( diff --git a/internal/cmd/project_dir_gate_test.go b/internal/cmd/project_dir_gate_test.go new file mode 100644 index 0000000..23989f0 --- /dev/null +++ b/internal/cmd/project_dir_gate_test.go @@ -0,0 +1,376 @@ +package cmd + +import ( + "errors" + "os" + "path/filepath" + "strings" + "testing" +) + +// 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) + } +} + +// 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. +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) { + _, _, err := run(t, tc.args...) + if err == nil { + t.Fatalf("%s must fail", tc.name) + } + 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 index 4aab9ef..419b734 100644 --- a/internal/cmd/project_dir_test.go +++ b/internal/cmd/project_dir_test.go @@ -271,9 +271,24 @@ func TestResolveProjectDirClassification(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. - err := resolveProjectDir(filepath.Join(root, "notadir.txt", "sub")) - if err == nil { + // + // 🔴 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) From f4c1189dcf45c7c3697d8428c01ff161a1005556 Mon Sep 17 00:00:00 2001 From: ZacxDev Date: Fri, 7 Aug 2026 17:07:59 -0500 Subject: [PATCH 3/5] test(exit-codes): pin WHICH remedy each usage arm carries, and correct two mutation counts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Found by mutation-testing the audit fixes themselves — the fix round resets the gate, and this is what the re-measurement turned up. SWAPPED OPERANDS SURVIVED. Exchanging resolveProjectDir's two exit-2 remedies passed the ENTIRE suite: 0 failures. 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 either way), and the one message test asks only that the path the user typed appears — true of both spellings. The result is advice that is exactly backwards: a path that is simply missing told to "pass the App project ROOT ... not a file" (about a file that does not exist), and someone who pointed at their own manifest told to scaffold with `app init` a project they already have. That is AGENTS item 21(f)/(g)'s operand-order class, reached from a third direction. The two remedies are now named constants (remedyNoSuchDir / remedyNotADir) and TestProjectDirRemediesMatchTheirArm derives each arm's expected text FROM THE CONSTANT and requires 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. Re-measured with a swap that compiles and vets cleanly (swap which constant each arm passes, so the arg counts still match): 1 leaf subtest, by name. A swap of the constant BODIES is additionally caught by go vet's printf check, but that is the compiler's kill, not the guard's, so it is not what the count is measured on. TWO PUBLISHED MUTATION COUNTS CORRECTED, because a number nobody re-ran is a claim. AGENTS.md said the widening mutant "reddened exactly one leaf subtest" at the audited tip. Measured over the WHOLE module at a4807f4 it reddened TWO: 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. The finding itself stands — inside internal/cmd it was one row, and that row can t.Skip itself — but the item now states the measured figure, the re-measured 8 on the fixed tree, and the 7 that remain when the old single row is deleted too, which is what shows the new battery does not rest on a row anyone can remove. Co-Authored-By: Claude Opus 5 (1M context) --- AGENTS.md | 51 +++++++++++++++----- internal/cmd/project_dir.go | 30 ++++++++++-- internal/cmd/project_dir_gate_test.go | 69 +++++++++++++++++++++++++++ 3 files changed, 133 insertions(+), 17 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index e758a6e..6372353 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -1944,19 +1944,29 @@ neither one's. 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 used to rest on ONE skippable row.** The - widening mutant — turning the untagged arm into `asUsageError`, i.e. "tag - every stat failure" — reddened exactly one leaf subtest, and that subtest - carried a `t.Skip` when the fixture produced no error, so a filesystem that - resolved the path would have taken the whole 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. - Re-measured: the widening mutant reddens **9 leaf subtests**, up from 1. + 🔴 **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 @@ -1966,6 +1976,21 @@ neither one's. 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. - 🔴 **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). diff --git a/internal/cmd/project_dir.go b/internal/cmd/project_dir.go index 902f1c9..a951d53 100644 --- a/internal/cmd/project_dir.go +++ b/internal/cmd/project_dir.go @@ -55,12 +55,35 @@ import ( // 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( - "%s: no such directory — pass the path to an App project root, or scaffold one with `civitai app init `", dir)) + 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 @@ -77,8 +100,7 @@ func resolveProjectDir(dir string) error { return err } if !info.IsDir() { - return asUsageError(fmt.Errorf( - "%s is not a directory — pass the App project ROOT (the directory holding %s), not a file", dir, manifest.Filename)) + 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 index 23989f0..2bfd9d9 100644 --- a/internal/cmd/project_dir_gate_test.go +++ b/internal/cmd/project_dir_gate_test.go @@ -2,10 +2,13 @@ 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 @@ -331,6 +334,72 @@ func TestValidateJSONOnlyEmitsAResultItActuallyProduced(t *testing.T) { } } +// 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. +func TestProjectDirRemediesMatchTheirArm(t *testing.T) { + root := newProjectDirRoot(t) + missing := filepath.Join(root, "nope") + file := filepath.Join(root, "notadir.txt") + + // The remedies must be non-empty and distinct, or every Contains below is + // vacuous (`strings.Contains(x, "")` is always true) and the absence + // assertions can never fire. + noSuch := fmt.Sprintf(remedyNoSuchDir, missing) + notDir := fmt.Sprintf(remedyNotADir, file, manifest.Filename) + if strings.TrimSpace(noSuch) == "" || strings.TrimSpace(notDir) == "" { + t.Fatalf("a remedy rendered empty — every assertion in this test would be vacuous\nnoSuch=%q notDir=%q", noSuch, notDir) + } + if noSuch == notDir { + t.Fatal("the two remedies are identical — this guard cannot tell the arms apart, so a swap is undetectable") + } + + for _, tc := range []struct { + name string + dir string + want, deny string + why string + }{ + { + name: "nonexistent path gets the `app init` remedy", + dir: missing, want: noSuch, deny: notDir, + 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: notDir, deny: noSuch, + 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) + } + if got := err.Error(); !strings.Contains(got, tc.want) { + t.Errorf("wrong remedy for this arm.\nWhy it matters: %s\nwant it to contain: %s\ngot: %s", tc.why, tc.want, got) + } + if got := err.Error(); strings.Contains(got, tc.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 From 4abb2673a21ca77c6e88a2c8e9630e62227c9edb Mon Sep 17 00:00:00 2001 From: ZacxDev Date: Fri, 7 Aug 2026 18:13:39 -0500 Subject: [PATCH 4/5] test(exit-codes): un-inert two guards from the last round, and correct three published mutation numbers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round-2 audit fixes. All three findings are in guards I added last round — a fix round resets the gate, and this is what re-auditing the guards themselves turned up. 🔴 THE RESIDUAL GUARD WAS INERT ON CI. TestUngatedPathFlagsAreNotUsageErrors drives `app listing status --dir …`, but `newListingClient()` runs BEFORE `resolveListingSlug`, so with no credential the command dies at `no token configured` (ErrUnauthorized) and never reaches the --dir path. The bare `!errors.Is(err, ErrUsage)` assertion is satisfied by that auth failure. Reproduced hermetically (HOME + XDG_CONFIG_HOME at an empty dir, i.e. ubuntu-latest): both rows PASSED, and STILL PASSED with the residual-closing mutant applied — the change the guard exists to catch was invisible in the only environment that matters. It looked healthy locally only because a real config made it observe the manifest error instead. Fixed with t.Setenv of a dummy token (never sent — manifest.Load fails first) plus a premise assertion that t.Fatals on ErrUnauthorized. Re-measured hermetically: clean run passes, mutant reddens 2 leaf subtests. 🟡 THE REMEDY GUARD WAS HALF-INERT, AND THE DEAD HALF WAS THE ONE ITEM 25 ADVERTISES. Both remedies interpolate the path and the two rows rendered theirs with DIFFERENT paths, so `Contains(err, deny)` compared against a string carrying a path the error never mentions — false whatever the code does. Measured on the one-arm swap: 2 kills, 2 from `want`, 0 from `deny`. The same defect made the "distinct" precondition compare strings differing only by 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. Worth recording rather than claiming: what actually stops a duplicate reaching main today is `go vet`, not this guard — the two constants have different arities, so copy-pasting one over the other breaks a call site (measured: 2 `build failed`, 0 test failures). The precondition is the backstop for the day someone equalises those arities, and it now carries its own positive control so "it cannot reject anything" fails loudly instead of reading as a pass. 🟢 THREE PUBLISHED NUMBERS CORRECTED, in the item that says an unreproduced mutation number is a claim: * sliding the gate below the --json block reddens 4 leaves, not 3, and TWO of them are TEXT-mode rows — not "exactly the JSON rows while the text-mode rows stay green". Unavoidable: any placement below the --json block is also below validate.Dir. Same 4 at a4807f4 and at HEAD. * dropping the submit gate reddens 6 in the tree it ships in, not the 4 measured at a4807f4 (this round's two new rows account for it). * the code-1 scoping was over-broad by one arm: "for a project directory it could READ" still promises an object for the schema() arm, which is a directory the CLI reads fine that yields no Result. Both the note and the README now condition on whether validation PRODUCED a result — the thing the code branches on. Wording only; that arm is effectively unreachable in a released binary and the operative instruction was correct throughout. Gate: make ci green (18 pkgs ok, --- FAIL 0, build failed 0, timeout panics 0), gofmt clean, and golangci-lint v2.12.2 — the version CI pins, run separately because `make ci` is tidy+vet+test+build and does NOT lint — reports 0 issues, with a deliberate ST1005+ineffassign+unused probe confirming it reports 3 when there is something to find. Co-Authored-By: Claude Opus 5 (1M context) --- AGENTS.md | 79 ++++++++++++++++--- README.md | 10 ++- internal/cmd/exitcodes_claims_test.go | 8 +- internal/cmd/exitcodes_doc.go | 2 +- internal/cmd/project_dir_gate_test.go | 109 ++++++++++++++++++++++---- 5 files changed, 175 insertions(+), 33 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 6372353..e81b8dc 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -1899,8 +1899,13 @@ neither one's. 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: dropping the submit call alone reddens 4 leaf subtests - including this guard by name. + 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 @@ -1920,9 +1925,15 @@ neither one's. 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: it reddens exactly the JSON rows (3 leaf subtests) while - the text-mode rows stay green, so a table that only checked exit codes - would miss it. It is announced in THREE places, because the one that + 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 @@ -1936,10 +1947,20 @@ neither one's. 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 — "for a project directory it could **read**" — 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. + 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 @@ -1991,6 +2012,25 @@ neither one's. 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 on the one-arm swap: 2 kills, **2 from `want`, 0 from `deny`** — + the absence half, which is the half this item advertises, never fired + once. 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. + **What actually stops a duplicate reaching `main` today is `go vet`, not + this guard** — the two constants have different ARITIES (one `%s` vs two), + so copy-pasting one over the other breaks a call site (measured: 2 + `build failed`). The precondition is the backstop for the day someone + equalises those arities, at which point vet goes quiet; it carries its own + positive control so "it cannot reject anything" fails loudly rather than + reading as a pass. - 🔴 **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). @@ -2006,6 +2046,27 @@ neither one's. 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 that `t.Fatal`s on `ErrUnauthorized`. Re-measured hermetically: + the clean run passes and the mutant reddens **2 leaf subtests**. + **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.** + 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". 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 75bb588..850ccb9 100644 --- a/README.md +++ b/README.md @@ -912,15 +912,17 @@ exist**, or that is not a directory, is a mistake about the invocation: it write **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 project directory the CLI cannot **read** (an unreadable manifest, a -permissions error) likewise produces no object, and exits `1`. +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 the failure was a filesystem one | +| `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 @@ -1750,7 +1752,7 @@ 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 **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`.) For a project directory it could **read**, `civitai app validate --json` prints the full result and its `ok` field is the structured form of the same answer; a failure that produced 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. | +| `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. | diff --git a/internal/cmd/exitcodes_claims_test.go b/internal/cmd/exitcodes_claims_test.go index 5ca3f3d..215b306 100644 --- a/internal/cmd/exitcodes_claims_test.go +++ b/internal/cmd/exitcodes_claims_test.go @@ -118,7 +118,7 @@ func exitCodeContractClaims() []contractClaim { { code: 1, name: "`app validate --json` publishes a result only when it produced one", - phrases: []string{"For a project directory it could", "nothing on stdout", + 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 " + @@ -126,7 +126,11 @@ func exitCodeContractClaims() []contractClaim { "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", + "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)", }, { diff --git a/internal/cmd/exitcodes_doc.go b/internal/cmd/exitcodes_doc.go index 788b960..3c610d7 100644 --- a/internal/cmd/exitcodes_doc.go +++ b/internal/cmd/exitcodes_doc.go @@ -75,7 +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`.) For a project directory it could **read**, `civitai app validate --json` prints the full result and its `ok` field is the structured form of the same answer; a failure that produced no result at all — an unreadable manifest, say — still exits `1` with **nothing on stdout**, so branch on the exit code before parsing.", + "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.", }, }, diff --git a/internal/cmd/project_dir_gate_test.go b/internal/cmd/project_dir_gate_test.go index 2bfd9d9..b4cae64 100644 --- a/internal/cmd/project_dir_gate_test.go +++ b/internal/cmd/project_dir_gate_test.go @@ -9,6 +9,7 @@ import ( "testing" "github.com/civitai/cli/internal/manifest" + "github.com/civitai/cli/pkg/civitai" ) // This file is the second half of the #256 guards, added after an audit found @@ -346,38 +347,81 @@ func TestValidateJSONOnlyEmitsAResultItActuallyProduced(t *testing.T) { // 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 on the swap +// mutant: 2 kills, all 2 from `want`, **0 from `deny`**. +// - the "distinct" precondition compared two strings that differed only by +// their path, so it passed even for IDENTICAL remedy constants. Measured: +// giving `remedyNotADir` the text of `remedyNoSuchDir` survived the entire +// suite (rc 0, 18 pkgs ok, 0 FAIL) while the built binary answered a real +// file with `…/afile.txt: no such directory — … civitai app init …` at +// rc 2 — precisely the backwards advice this guard exists to prevent, live +// and fully green. +// +// 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`. +func noSuchAt(p string) string { return fmt.Sprintf(remedyNoSuchDir, p) } +func notDirAt(p string) string { return fmt.Sprintf(remedyNotADir, p, manifest.Filename) } + func TestProjectDirRemediesMatchTheirArm(t *testing.T) { root := newProjectDirRoot(t) missing := filepath.Join(root, "nope") file := filepath.Join(root, "notadir.txt") - // The remedies must be non-empty and distinct, or every Contains below is - // vacuous (`strings.Contains(x, "")` is always true) and the absence - // assertions can never fire. - noSuch := fmt.Sprintf(remedyNoSuchDir, missing) - notDir := fmt.Sprintf(remedyNotADir, file, manifest.Filename) - if strings.TrimSpace(noSuch) == "" || strings.TrimSpace(notDir) == "" { - t.Fatalf("a remedy rendered empty — every assertion in this test would be vacuous\nnoSuch=%q notDir=%q", noSuch, notDir) + // 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" + 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)) } - if noSuch == notDir { - t.Fatal("the two remedies are identical — this guard cannot tell the arms apart, so a swap is undetectable") + if noSuchAt(probe) == 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)) + } + // POSITIVE CONTROL for the line above — "can it go red?". A distinctness + // check that cannot reject anything is the reassuring-zero shape, and this + // one is doing real work only if it flags a KNOWN duplicate. Measured while + // writing it: the previous version compared the two remedies rendered with + // DIFFERENT paths, so it passed even for two identical constants. + // + // Note what actually stops a duplicate reaching main today: the two remedies + // have DIFFERENT ARITIES (one `%s` vs two), so copy-pasting one over the + // other breaks `go vet` at a call site — measured, 2 `build failed`. This + // precondition is the guard for the day someone equalises those arities, at + // which point vet goes quiet and nothing else would notice. + if dup := noSuchAt(probe); dup != noSuchAt(probe) || dup == notDirAt(probe) { + t.Fatal("the distinctness comparison cannot detect a duplicate — every `deny` assertion below is vacuous") } for _, tc := range []struct { - name string - dir string - want, deny string + 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: noSuch, deny: notDir, + 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: notDir, deny: noSuch, + 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", }, @@ -390,10 +434,11 @@ func TestProjectDirRemediesMatchTheirArm(t *testing.T) { if !errors.Is(err, ErrUsage) { t.Fatalf("premise: both arms must be usage errors, got %v", err) } - if got := err.Error(); !strings.Contains(got, tc.want) { - t.Errorf("wrong remedy for this arm.\nWhy it matters: %s\nwant it to contain: %s\ngot: %s", tc.why, tc.want, got) + 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, tc.deny) { + 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) } }) @@ -412,6 +457,23 @@ func TestProjectDirRemediesMatchTheirArm(t *testing.T) { // // 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) @@ -429,10 +491,23 @@ func TestUngatedPathFlagsAreNotUsageErrors(t *testing.T) { }, } { 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. An auth failure means the command never got as far as + // --dir, so whatever this row then asserts is about the wrong code + // path — exactly how this guard was inert on CI. + if errors.Is(err, civitai.ErrUnauthorized) { + t.Fatalf("PREMISE BROKEN: %s never reached the --dir path — it failed at the credential "+ + "gate in newListingClient(), which runs first. This row asserts nothing about --dir "+ + "while that is true.\nerr: %v", tc.name, 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 "+ From dbcd62b5e7982a6e84e59e49e7eee76ae7a7e11d Mon Sep 17 00:00:00 2001 From: ZacxDev Date: Fri, 7 Aug 2026 18:49:06 -0500 Subject: [PATCH 5/5] test(exit-codes): make the reach premise and the distinctness control POSITIVE, and retract a convenient conclusion MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round-3 audit fixes. No production defect; this is comments and test assertions, plus one zero-behaviour-change constant extraction. 🔴 RETRACTED: "go vet already catches a duplicate remedy". Two mutants were built and both observations were real — the disagreement was that we had built DIFFERENT mutants. Re-measured myself at ab1e685, three real edits to the two remedy constants: shape build vet full suite A copy the body over (arity 2->1) ok rc=1 1 `build failed` line (2 printf diags), 0 --- FAIL B same text + trailing `%.0s` ok rc=0 rc 0, 18 ok, 0 --- FAIL C same advice + ` (looking for %s)` ok rc=0 rc 0, 18 ok, 0 --- FAIL B and C are arity-preserving — no equalisation required — and C 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 ... civitai app init" at rc 2. And even A BUILDS: `go build ./cmd/civitai` succeeds, so what stops A is CI running `go test`, not a compile failure. At HEAD, B is killed by the precondition and C by the `deny` assertion. The guard is the live protection. AGENTS.md and the mirroring test comment now say so, and the test file no longer contradicts itself forty lines apart. The conclusion I published was the CONVENIENT one — if vet already catches it, the finding downgrades to a nit and no repair is needed. Worth recording as the direction to be most suspicious of, especially in AGENTS.md, which is the file the next person trusts instead of re-measuring. 🔴 THE BLOCK LABELLED "POSITIVE CONTROL" COULD NOT FAIL — 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; clause 2 was byte-identical to the precondition above it, which has already t.Fatalf'd. Measured: mutant B fires the precondition, not it; only after deleting the precondition does its one live clause fire — it backstopped a deleted test line. (staticcheck SA4000 misses the `var != f(x)` spelling.) Replaced with a real control: `remediesAreDistinct` is now a named comparator, fed a KNOWN DUPLICATE pair and required to report one, running FIRST so the precondition cannot shadow it. Proven: making the comparator return true unconditionally reddens the control by its own message. 🔴 THE REACH PREMISE WAS A DENYLIST OF ONE SENTINEL. Asserting the error is not ErrUnauthorized closes the gate we knew about and says nothing about reaching resolveListingSlug. Measured hermetically: inserting any new preflight ahead of it that fails with a plain untagged error left both rows PASSING — and with the residual-closing mutant ALSO applied they still both passed, i.e. the whole defect back, invisibly. Now positive: the error must carry resolveListingSlug's own wrapper, derived from the new `listingSlugResolveFailure` constant (extracted in app_listing.go; identical string, no behaviour change) so a reword moves both together. Re-running that same preflight mutant: 2 leaf subtests, 2 PREMISE BROKEN messages, where the denylist form survived it. 🟢 Two more numbers corrected. The "2 kills, 2 from want, 0 from deny" belongs to the BOTH-ARM swap; the one-arm swap gives 1 kill (also entirely from `want`). And "2 build failed" was 2 vet printf DIAGNOSTICS producing 1 `FAIL ... [build failed]` line. Gate: make ci green (18 pkgs ok, --- FAIL 0, build failed 0, timeout panics 0), gofmt clean, golangci-lint v2.12.2 reports 0 issues with a deliberate probe confirming it reports 3 when there is something to find. Co-Authored-By: Claude Opus 5 (1M context) --- AGENTS.md | 86 +++++++++++++++----- internal/cmd/app_listing.go | 9 ++- internal/cmd/project_dir_gate_test.go | 109 ++++++++++++++++++-------- 3 files changed, 152 insertions(+), 52 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index e81b8dc..34a0a11 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -2017,20 +2017,53 @@ neither one's. 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 on the one-arm swap: 2 kills, **2 from `want`, 0 from `deny`** — - the absence half, which is the half this item advertises, never fired - once. 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. - **What actually stops a duplicate reaching `main` today is `go vet`, not - this guard** — the two constants have different ARITIES (one `%s` vs two), - so copy-pasting one over the other breaks a call site (measured: 2 - `build failed`). The precondition is the backstop for the day someone - equalises those arities, at which point vet goes quiet; it carries its own - positive control so "it cannot reject anything" fails loudly rather than + 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). @@ -2059,13 +2092,28 @@ neither one's. 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 that `t.Fatal`s on `ErrUnauthorized`. Re-measured hermetically: - the clean run passes and the mutant reddens **2 leaf subtests**. + 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.** - 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". Item 23's `findingSiteHook` and item 24's `sentinelFree` + 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 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/project_dir_gate_test.go b/internal/cmd/project_dir_gate_test.go index b4cae64..3b8a38b 100644 --- a/internal/cmd/project_dir_gate_test.go +++ b/internal/cmd/project_dir_gate_test.go @@ -9,7 +9,6 @@ import ( "testing" "github.com/civitai/cli/internal/manifest" - "github.com/civitai/cli/pkg/civitai" ) // This file is the second half of the #256 guards, added after an audit found @@ -354,22 +353,52 @@ func TestValidateJSONOnlyEmitsAResultItActuallyProduced(t *testing.T) { // // - 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 on the swap -// mutant: 2 kills, all 2 from `want`, **0 from `deny`**. +// 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. Measured: -// giving `remedyNotADir` the text of `remedyNoSuchDir` survived the entire -// suite (rc 0, 18 pkgs ok, 0 FAIL) while the built binary answered a real -// file with `…/afile.txt: no such directory — … civitai app init …` at -// rc 2 — precisely the backwards advice this guard exists to prevent, live -// and fully green. +// 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`. +// `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") @@ -381,29 +410,36 @@ func TestProjectDirRemediesMatchTheirArm(t *testing.T) { // 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)) } - if 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)) } - // POSITIVE CONTROL for the line above — "can it go red?". A distinctness - // check that cannot reject anything is the reassuring-zero shape, and this - // one is doing real work only if it flags a KNOWN duplicate. Measured while - // writing it: the previous version compared the two remedies rendered with - // DIFFERENT paths, so it passed even for two identical constants. - // - // Note what actually stops a duplicate reaching main today: the two remedies - // have DIFFERENT ARITIES (one `%s` vs two), so copy-pasting one over the - // other breaks `go vet` at a call site — measured, 2 `build failed`. This - // precondition is the guard for the day someone equalises those arities, at - // which point vet goes quiet and nothing else would notice. - if dup := noSuchAt(probe); dup != noSuchAt(probe) || dup == notDirAt(probe) { - t.Fatal("the distinctness comparison cannot detect a duplicate — every `deny` assertion below is vacuous") - } for _, tc := range []struct { name string @@ -500,13 +536,22 @@ func TestUngatedPathFlagsAreNotUsageErrors(t *testing.T) { if err == nil { t.Fatalf("%s must fail", tc.name) } - // PREMISE. An auth failure means the command never got as far as - // --dir, so whatever this row then asserts is about the wrong code - // path — exactly how this guard was inert on CI. - if errors.Is(err, civitai.ErrUnauthorized) { - t.Fatalf("PREMISE BROKEN: %s never reached the --dir path — it failed at the credential "+ - "gate in newListingClient(), which runs first. This row asserts nothing about --dir "+ - "while that is true.\nerr: %v", tc.name, err) + // 🔴 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 "+