diff --git a/README.md b/README.md index 2aa16952..e6ae6d95 100644 --- a/README.md +++ b/README.md @@ -1675,7 +1675,7 @@ was refused, while a **2.32 MB** one was accepted. That brackets the server's real ceiling to somewhere in **(2.32 MB, 8.20 MB]** β€” and no further, because each probe costs a real submission. -πŸ”΄ **The CLI now refuses above 10485760 bytes of body β€” and that number is not from +πŸ”΄ **The CLI now refuses at or above 10485760 bytes of body β€” and that number is not from inside the bracket.** It is Next.js's `proxyClientMaxBodySize` default, which applies because civitai's `src/proxy.ts` matches `/api/v1/:path*` and its `next.config.mjs` sets no override. It is external to this CLI and to civitai. @@ -1693,6 +1693,20 @@ sanity check, not evidence; the provenance is what the number rests on: The refusal costs no upload: it is checked against the marshalled document itself, before the request is built. +**Why "at or above" and not "above".** A body of *exactly* 10485760 is reachable in practice β€” +the JSON envelope for a `--allow-dirty` submit is 96 bytes, base64 output is always a multiple +of 4, so a 7,864,246-byte zip lands on the ceiling to the byte. Which side of it the platform +sits on is genuinely unresolved: Next.js's source reads `bytesRead > bodySizeLimit`, which would +accept that body, while an end-to-end measurement of the real endpoint answered `413` at exactly +10485760 (and `401` β€” i.e. the body got through to auth β€” one byte below). One of those is wrong +and this CLI cannot settle it without spending a real submission. + +It does not need to. The two mistakes are not equally priced: refusing one byte early costs an +author a flag they are already told about, and accepting one byte too many costs the whole +upload and returns an error naming nothing about size. So the guard takes the cheap side, and +this paragraph records that it is a choice under uncertainty rather than a fact about the +server. + ⚠ **It is still a vendored number, and `--allow-oversize` is the way out.** Nothing in this CLI notices the day civitai raises that default β€” and [civitai#4793](https://github.com/civitai/civitai/issues/4793) names raising it as the change @@ -3971,7 +3985,7 @@ credited it to the wrong command.) | `(token scope not reported by the server β€” Buzz capabilities unknown)` | `civitai whoami` got no `tokenScope`, so **Buzz** read/spend are unknowable and are omitted rather than printed as `no`. Scoped to Buzz deliberately: the **Submit Apps** row is still shown above it, because a personal key's submit answer does not depend on the scope mask. | [Submit & auth](#submit--auth) | | `not permitted to read this app's analytics (403)` | `app metrics` needs the **Apps submit scope**. An OAuth `civitai login` carries it β€” unless the token was minted before the scope existed, in which case re-run `civitai login`. A full-scope personal API key also works. | [App metrics](#app-metrics) | | `block lacks ai:write:budgeted scope` | Printed by your app at runtime under `dev:live`. The dev token was minted **without** `--spend`, so the CLI filtered the budgeted-spend scope out β€” it never requests that scope implicitly, even when your manifest declares it. | [Local dev loop](#local-dev-loop-harness-mock-vs-live) | -| `the server can receive` | The submit body exceeds **10485760 bytes** and `app submit` refused **before uploading**, so it cost you nothing. The number is the base64 JSON body, ~4/3 of the zip β€” shrink the bundle (the message lists the largest entries). ⚠ That ceiling is a *vendored* framework default, not something the server told us: if you believe it has been raised, `--allow-oversize` submits anyway. | [Submit & auth](#submit--auth) β€” *How big can a bundle be?* | +| `the server can receive` | The submit body reached or exceeded **10485760 bytes** and `app submit` refused **before uploading**, so it cost you nothing. The number is the base64 JSON body, ~4/3 of the zip β€” shrink the bundle (the message lists the largest entries). ⚠ That ceiling is a *vendored* framework default, not something the server told us: if you believe it has been raised, `--allow-oversize` submits anyway. | [Submit & auth](#submit--auth) β€” *How big can a bundle be?* | | `insufficient Buzz` / `generation disabled` | Not credential problems, which is why they exit `1` rather than `3` β€” a script must not loop on `civitai login` for either. | [Exit codes specific to `generate`](#exit-codes-specific-to-generate) | | `rate limited (429)` | πŸ”΄ **This one message has TWO exit codes β€” branch on the code, never on the text.** `2` when it is really the deep-paging cap (the message says "too many pages" / "use cursors instead"): the request is structurally doomed, so use `--cursor` rather than `--page`. `6` for a genuine throttle; retry with backoff. **Not `5`** β€” a 429 *can* exit `5`, but when it does it prints the next row's message instead of this one. | [Exit codes](#exit-codes) | | `Civitai returned HTTP` | **A retriable status that survived every read retry** β€” printed for the status that persisted, whichever it was: `502`/`503`/`504`, or a `429` that carried `Retry-After`. Exits **`5`** in every case, because once the retries are exhausted this is a service-availability failure and `5` is the code to retry on. Read the number in the message to know which you hit. ⚠ For the `429` case only: the header is checked *before* the message, so a deep-paging-cap `429` that carried `Retry-After` lands here too β€” exit `5` β€” rather than on `2`, and you will **not** see `rate limited (429)`. | [Exit codes](#exit-codes) | @@ -4007,7 +4021,7 @@ credited it to the wrong command.) | `nothing index.html loads reaches it` | The **strong** tier: the emitter is in your project but nothing the browser loads reaches it β€” an orphan file. Copying `civitai-host.js` in is only half the fix; it has to be referenced too. | [The host handshake](#the-host-handshake-block_ready) | | `no lockfile is committed` / `is not a lockfile` | The platform build installs **strictly** from the committed lockfile, so a missing one β€” or a zero-byte one created with `touch` β€” fails the build server-side. A lockfile is generated by the package manager, never hand-written. | [Validate fidelity](#validate-fidelity) | | `refusing to submit without --yes` | A submit that would really upload asked for confirmation and found no TTY. Pass `--yes` in CI, or `--package-only` to just write the .zip. | [Command reference](#command-reference) | -| `What this CLI sent` / `largest entries in the bundle` | Not an error of its own β€” it is printed **under** a failed submit, and it is the CLI's account of what left your machine: the exact bytes that went on the wire, and the biggest entries they were made of. It appears because the server's own message may name nothing you can act on; `400: Invalid JSON` is the measured case ([#423](https://github.com/civitai/cli/issues/423)), an error about the *parse* raised downstream of an oversized request body. **The CLI does not claim to know why the submit failed** β€” it cannot see the server's limits. ⚠ Since `#585` it *does* refuse a body over **10485760 bytes** up front, so reaching this block at all means the body was under that ceiling (or you passed `--allow-oversize`); `pkgzip`'s own caps remain much higher and still are not a server mirror. If the bundle is large, drop what the platform build does not need and retry. Not printed for a `401`/`403`/`429`. | [Submit & auth](#submit--auth) β€” *How big can a bundle be?* | +| `What this CLI sent` / `largest entries in the bundle` | Not an error of its own β€” it is printed **under** a failed submit, and it is the CLI's account of what left your machine: the exact bytes that went on the wire, and the biggest entries they were made of. It appears because the server's own message may name nothing you can act on; `400: Invalid JSON` is the measured case ([#423](https://github.com/civitai/cli/issues/423)), an error about the *parse* raised downstream of an oversized request body. **The CLI does not claim to know why the submit failed** β€” it cannot see the server's limits. ⚠ Since `#585` it *does* refuse a body of **10485760 bytes** or more up front, so reaching this block at all means the body was under that ceiling (or you passed `--allow-oversize`); `pkgzip`'s own caps remain much higher and still are not a server mirror. If the bundle is large, drop what the platform build does not need and retry. Not printed for a `401`/`403`/`429`. | [Submit & auth](#submit--auth) β€” *How big can a bundle be?* | | `Your repo may be behind what was last released` / `Resubmitting the version that is already live is almost always an accident` / `That version is approved but not live` | The **monotonic-version guard**: `civitai app submit` refused because the manifest version is not strictly above the highest **approved** version of that app, and approving an older (or identical) version supersedes the newer one β€” replacing the live deployment when that version is serving. Exit `1`; `--allow-downgrade` submits anyway. **The second line tells you which of four cases you are in, and each says only what is actually known.** A **lower** version against a version that is **live** replaces that deployment on approval, so your repo may be behind what was last released β€” bump the manifest, or `civitai app pull` first. A **lower** version against an approved row that is **not** live deploys code older than the highest approved version; no deployment of that version is being replaced, because none is running. The **same** version against a deployment that is really live is almost always an accident. The **same** version against an approved row that is **not** live is a resubmit of a deploy that has not landed β€” still building, still deploying, or failed β€” which is a plausible deliberate act, and `--allow-downgrade` submits that version again. **"Live" is the server's own answer** (the `liveUrl` it returns, the same field `civitai app status` prints `Live at:` from), not a guess from the deploy state β€” so a legacy approval that predates deploy-state tracking is correctly treated as serving. | [Exit code 1](#exit-code-1) | | `from a dirty git work tree` / `that go into the bundle are not committed` | The **dirty-work-tree guard**: `civitai app submit` refused because files that go into the bundle are uncommitted. The bundle is packaged from what is on disk, so approving one deploys code that exists in no commit and nothing afterwards can say which revision is live. The refusal names the paths (`git status` spelling, relative to the packaged directory) β€” commit them, or pass `--allow-dirty` to submit the tree exactly as it is. Exit `1`. **It only fires inside a git repository**: a scaffolded app that was never `git init`ed submits unchanged, and so does a machine with no `git` on `PATH`. Paths the packager never ships β€” `dist/`, `node_modules/`, a stray `.zip`, a `.env.local`, anything `.gitignore`d, and any symlink (the packager bundles regular files only) β€” are not counted, because they are not in the bundle. πŸ”΄ **A repository with no commits yet refuses *everything*, `block.manifest.json` included** β€” a `git init` you have not committed into means nothing in the bundle is in a commit, which is exactly what the guard checks. That is the row between "no repo" and "repo, dirty": make the first commit, or pass `--allow-dirty`. Scaffolding never puts you here β€” `civitai app create` / `app init` run no `git init`. **A `git mv` counts as two changes**, because the bundle gains the destination and loses the original; both are named, even when the destination is a path the packager drops (`git mv src/App.tsx dist/App.tsx` is refused, naming `src/App.tsx`). | [Exit code 1](#exit-code-1) | | `look like they hold credentials` | A **warning**, not a refusal β€” the submit continued and the exit code is unchanged. A file the packager KEPT holds a line shaped like a credential, and the bundle goes to the platform and to a human reviewer, where it cannot be recalled. The warning prints `path:line` and the key name and deliberately **not** the value β€” open the file to see what matched. It fires on every path including `--package-only`, is scoped to what was really packaged, and is a heuristic in both directions: a false positive is possible, and silence is not a guarantee. ⚠️ On a real `submit` it prints *after* the confirmation, so it reports the leak rather than preventing it β€” `--package-only` is the path where you can still act. If it says the scan **stopped** at its byte budget, the files after that point were not checked at all. | [Submit & auth](#submit--auth) β€” *What looks like a credential* | diff --git a/cmd/civitai/submit_oversize_exitcode_test.go b/cmd/civitai/submit_oversize_exitcode_test.go new file mode 100644 index 00000000..a59529e1 --- /dev/null +++ b/cmd/civitai/submit_oversize_exitcode_test.go @@ -0,0 +1,188 @@ +package main + +import ( + "bytes" + "crypto/rand" + "encoding/json" + "errors" + "fmt" + "net/http" + "net/http/httptest" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/civitai/cli/internal/appapi" + "github.com/civitai/cli/internal/cmd" + "github.com/civitai/cli/pkg/civitai" +) + +// THE EXIT CODE OF THE OVERSIZE-BODY REFUSAL (issue #585). +// +// πŸ”΄ THE CONTRACT WAS PUBLISHED AND NOT ASSERTED. internal/cmd/exitcodes_doc.go +// publishes two things about this refusal: that it exits `1`, and β€” under code 2 +// β€” that "a script branching on `2` for that case must branch on `1`", i.e. a +// documented BREAKING CHANGE for anyone who scripted against the old behaviour, +// where the server's `400: Invalid JSON` produced a 2. appapi's own comment on +// ErrBundleTooLarge says the code is "now assertable". +// +// It was assertable and not asserted. Measured at the commit that shipped #585: +// no row in exitCodeContractClaims(), no entry in exitCodeClaimsFloor, and no +// reference to ErrBundleTooLarge anywhere under cmd/civitai/. The behaviour was +// correct, but only by falling through exitCode's `default` β€” which is exactly +// the accident AGENTS.md item 7 says an untagged return produces, and which the +// dirty-tree refusal one file over has a test for precisely because being right +// by accident is not the same as being pinned. +// +// This is the missing half, shaped like its sibling +// (submit_dirty_guard_exitcode_test.go): the REAL command, the REAL error, a +// negative control on the instrument. + +// realOversizeRefusal runs the REAL `civitai app submit` against a project whose +// bundle cannot fit the submit body, and returns the error the command produced. +// +// πŸ”΄ IT TAKES THE REAL ERROR, NOT A HAND-TAGGED FIXTURE. A test that builds its +// own `fmt.Errorf("%w", appapi.ErrBundleTooLarge)` asserts the exit mapping of an +// error it invented; it cannot see the guard returning something else, or +// wrapping it in a way that breaks the errors.Is walk. That is the seam this +// package exists to join β€” classification in internal/cmd, process contract +// here β€” and the sibling test's header says so after the same defect. +// +// NO GIT REPO ON PURPOSE. The dirty-work-tree guard degrades silently outside a +// repository (the scaffolded-app path), so this fixture reaches the size guard +// without the dirty guard firing first β€” and the submit carries zero provenance, +// which is the shape --package-only and the no-token fallback also send. +func realOversizeRefusal(t *testing.T) (string, string, error) { + t.Helper() + + dir, err := filepath.EvalSymlinks(t.TempDir()) + if err != nil { + t.Fatal(err) + } + write := func(rel string, content []byte) { + t.Helper() + p := filepath.Join(dir, filepath.FromSlash(rel)) + if err := os.MkdirAll(filepath.Dir(p), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(p, content, 0o600); err != nil { + t.Fatal(err) + } + } + write("block.manifest.json", []byte(`{ + "$schema": "https://civitai.com/schemas/app-block/v1.json", + "blockId": "custom-generators", + "version": "0.6.1", + "name": "Custom Generators", + "type": "block", + "scopes": [], + "page": { "path": "/", "title": "Custom Generators", "icon": "bolt" }, + "iframe": { "minHeight": 400, "maxHeight": 4000, "resizable": true, "sandbox": "allow-scripts allow-forms" }, + "contentRating": "g", + "minApiVersion": "1.0" +}`)) + write("index.html", []byte("")) + + // INCOMPRESSIBLE, so the zip cannot shrink back under the ceiling. 3/4 of the + // ceiling in random bytes base64-encodes to about the ceiling; the extra MiB + // puts it unambiguously over. + big := make([]byte, (appapi.MaxSubmitBodyBytes/4)*3+(1<<20)) + if _, err := rand.Read(big); err != nil { + t.Fatalf("CONTROL failure, not a finding: could not build the fixture asset: %v", err) + } + write("big.bin", big) + + // πŸ”΄ THE SUBMIT ROUTE MUST NEVER BE REACHED β€” the refusal is a PREFLIGHT, and + // a handler that fails the test is the reachability control: if the guard + // stopped firing, this goes red here rather than passing quietly. + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if strings.Contains(r.URL.Path, "submit-version") { + t.Errorf("the oversize guard must refuse before any upload; got %s %s", r.Method, r.URL.Path) + w.WriteHeader(http.StatusInternalServerError) + return + } + // The version-regression guard's listing read, which happens first. + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(map[string]any{"submissions": []any{}}) + })) + t.Cleanup(srv.Close) + + t.Setenv("XDG_CONFIG_HOME", t.TempDir()) + t.Setenv("CIVITAI_TOKEN", "tok-test") // the guard only runs on the upload path + t.Setenv("CIVITAI_BASE_URL", srv.URL) + t.Setenv("CIVITAI_SUBMIT_PATH", "") + t.Setenv("CIVITAI_NO_UPDATE_CHECK", "1") + + root := cmd.NewRootCmd() + var out, errb bytes.Buffer + root.SetOut(&out) + root.SetErr(&errb) + root.SetArgs([]string{"app", "submit", dir, "--yes"}) + runErr := root.Execute() + if runErr == nil { + t.Fatalf("`app submit` must refuse a bundle over the submit-body ceiling; stdout:\n%s\nstderr:\n%s", + out.String(), errb.String()) + } + if !strings.Contains(runErr.Error(), "the server can receive") { + t.Fatalf("the error is not the size refusal (so this test would be about the wrong error "+ + "entirely): %v", runErr) + } + return out.String(), errb.String(), runErr +} + +func TestOversizeBundleExitsGeneric(t *testing.T) { + _, _, err := realOversizeRefusal(t) + + // The sentinel must be the one the contract names, on the REAL error. + if !errors.Is(err, appapi.ErrBundleTooLarge) { + t.Fatal("the REAL refusal does not carry appapi.ErrBundleTooLarge β€” the exit-code claim below " + + "would be about a sentinel nothing produces, and `app submit` would also print the " + + "post-UPLOAD diagnosis, which claims bytes went on the wire") + } + if got := exitCode(err); got != exitGeneric { + t.Errorf("exitCode(bundle over the submit-body ceiling) = %d, want %d (generic β€” a verdict about "+ + "the project).\n2 is a mistake about the INVOCATION; every flag, argument and path here is "+ + "well-formed. The contract publishes this reclassification explicitly: a script branching on "+ + "2 for this case must branch on 1.", got, exitGeneric) + } + + // Wrapped by an outer message (cobra/RunE chains do this) it must keep its + // code β€” an errors.Is walk, never a top-level type check. + wrapped := fmt.Errorf("app submit: %w", err) + if got := exitCode(wrapped); got != exitGeneric { + t.Errorf("wrapped: exitCode = %d, want %d", got, exitGeneric) + } + + // NEGATIVE CONTROL: exitCode CAN return something other than exitGeneric, so + // the assertion above is not a fact about a function that always says 1. + if got := exitCode(civitai.Tag(civitai.ErrBadRequest, errors.New("bad enum"))); got != exitUsage { + t.Fatalf("negative control: exitCode(ErrBadRequest) = %d, want %d β€” the instrument is not discriminating", + got, exitUsage) + } +} + +// TestOversizeSentinelIsNotAnAPIKind pins the other half: the sentinel must not +// accidentally satisfy an API classification kind, which would silently move the +// refusal onto 2/3/4/5/6 β€” and 2 is the code the contract specifically promises +// it moved AWAY from, so a drift there re-breaks the migration notice rather +// than merely being wrong. +func TestOversizeSentinelIsNotAnAPIKind(t *testing.T) { + err := fmt.Errorf("%w: submit body is too big", appapi.ErrBundleTooLarge) + kinds := map[string]error{ + "ErrBadRequest": civitai.ErrBadRequest, + "ErrUnauthorized": civitai.ErrUnauthorized, + "ErrNotFound": civitai.ErrNotFound, + "ErrRateLimited": civitai.ErrRateLimited, + "ErrNetwork": civitai.ErrNetwork, + } + for name, kind := range kinds { + if errors.Is(err, kind) { + t.Errorf("a bundle-too-large error must not match civitai.%s β€” that would move its exit code", name) + } + } + // POSITIVE CONTROL on the loop. + if !errors.Is(civitai.Tag(civitai.ErrNotFound, errors.New("x")), kinds["ErrNotFound"]) { + t.Fatal("the errors.Is walk cannot see a kind it should β€” the negatives above prove nothing") + } +} diff --git a/internal/appapi/appblocks.go b/internal/appapi/appblocks.go index 84413099..71e0ac72 100644 --- a/internal/appapi/appblocks.go +++ b/internal/appapi/appblocks.go @@ -656,47 +656,31 @@ func submitEnvelopeLen(prov Provenance) int { return len(env) } -// SubmitBodySize returns the exact size, in bytes, of the HTTP request body -// SubmitVersion sends for a zip of zipLen bytes carrying provenance prov. -// -// πŸ”΄ IT TAKES THE PROVENANCE BECAUSE THE BODY DOES. This number is PRINTED TO -// USERS β€” on the `Packaged …` line and again under a failed submit β€” as "what -// this CLI sent", and #411's stamp makes a submit that carries provenance ~70 -// bytes larger than one that does not. A signature that could not see the -// provenance would have kept reporting the smaller number, which is a small -// error in the quantity and a total one in the claim: the point of the line is -// that it is EXACT (see below), so it may not be an estimate the moment a -// feature lands. Pass a zero Provenance for a path that sends none -// (--package-only, the no-token fallback) and the number is unchanged. -// -// πŸ”΄ THE ZIP IS NOT WHAT GOES ON THE WIRE, AND THE DIFFERENCE IS THE WHOLE OF -// ISSUE #423. SubmitVersion base64-encodes the archive into a JSON document, so -// the bytes the server receives β€” and the bytes any request-body limit is -// applied to β€” are ~4/3 of the compressed size. An author reading -// `8201270 bytes compressed` off `app submit` had no way to see the ~10.9 MB -// that was actually sent, so nothing they could measure locally corresponded to -// the quantity that was refused. -// -// It is EXACT, not an estimate: base64's alphabet (A–Z a–z 0–9 + / =) contains -// no character encoding/json escapes, so the payload is copied through verbatim -// and the envelope is a constant. Do not substitute a 1.37 multiplier for it β€” -// the point of printing the number is that the author can compare it with a -// limit, and a rounded number cannot be compared with anything. -// -// πŸ”΄ THIS COMMENT DOCUMENTS SubmitBodySize, WHICH IS ~40 LINES BELOW. Everything -// between here and it β€” MaxSubmitBodyBytes, ErrBundleTooLarge β€” was inserted -// into the middle of this block, which handed godoc the whole thing as the -// CONSTANT's comment and left the exported function with none. `go doc -// appapi.SubmitBodySize` printed nothing. staticcheck cannot see it: .golangci.yml -// disables ST1020-ST1022. Keep the blank-line separations below. - // MaxSubmitBodyBytes is the largest request body the submit endpoint can actually // RECEIVE, in bytes. // -// πŸ”΄ This IS the server's number, unlike the caps in `internal/pkgzip` β€” which is -// exactly the claim `pkgzip/caps_claim_test.go` exists to keep OUT of that file, and -// the reason this constant lives here instead. It is stated with its evidence so the -// next reader can re-derive it rather than trust it: +// πŸ”΄ EVERY DECLARATION BELOW CARRIES ITS OWN DOC, AND THE BLANK LINES BETWEEN THEM +// ARE LOAD-BEARING. A doc block runs into the next one without a blank line and +// godoc silently hands the WHOLE thing to whichever declaration comes first in +// source order. That shipped: this block and ErrBundleTooLarge's were glued +// together, so `go doc appapi.MaxSubmitBodyBytes` printed the bare const line and +// `go doc appapi.ErrBundleTooLarge` printed this entire evidence chain as if it +// were the sentinel's. staticcheck cannot see it β€” .golangci.yml disables +// ST1020-ST1022 β€” so `go doc` is the only check. Run it, do not read for it. +// +// πŸ”΄ This IS the server's number, unlike the caps in `internal/pkgzip`, and that is +// why it lives here: `appapi` is what builds the body, so the body's limit belongs +// beside SubmitBodySize. ⚠ That placement is an architectural decision held by +// review, NOT by a test. `pkgzip/caps_claim_test.go` bans four literal phrases from +// pkgzip.go (`server max`, `server per-file max`, `caps mirror the server`, `will +// not be rejected on size grounds`); measured with both controls, a fully +// server-attributed constant using none of those four PASSES it, and appending one +// makes it fail with its own message. So the guard stops the four spellings #423 +// disproved β€” it does not force this constant to live here. An earlier version of +// this comment said it did. +// +// It is stated with its evidence so the next reader can re-derive it rather than +// trust it: // // - `/api/v1/blocks/submit-version` is matched by the platform's proxy matcher // (`/api/v1/:path*`), and a proxy-matched request body is capped at the framework's @@ -717,7 +701,9 @@ func submitEnvelopeLen(prov Provenance) int { // ⚠ It bounds the BODY, not the zip. A zip is base64-encoded (4/3) into a JSON // envelope, so the usable zip is roughly 7.5 MiB β€” but do not hardcode that number // anywhere: use SubmitBodySize, which is exact. -// ErrBundleTooLarge tags the preflight refusal above MaxSubmitBodyBytes. +const MaxSubmitBodyBytes = 10485760 + +// ErrBundleTooLarge tags the preflight refusal at or above MaxSubmitBodyBytes. // // πŸ”΄ IT EXISTS SO CALLERS CAN TELL "WE DECLINED" FROM "THE UPLOAD FAILED", and // the first version of this guard did not have it β€” so `app submit` ran its @@ -731,12 +717,48 @@ func submitEnvelopeLen(prov Provenance) int { // matches no arm of that switch, so the 1 is unchanged AND now assertable. var ErrBundleTooLarge = errors.New("bundle too large to upload") -const MaxSubmitBodyBytes = 10485760 - -// SubmitBodySize is the exact number of bytes the submit request body will carry -// for a zip of zipLen with provenance prov: base64 of the zip, plus the JSON -// envelope. See the derivation block above β€” it is exact, not an estimate, and it -// is the quantity a request-body limit applies to. +// SubmitBodySize returns the exact size, in bytes, of the HTTP request body +// SubmitVersion sends for a zip of zipLen bytes carrying provenance prov: +// base64 of the zip, plus the JSON envelope. +// +// πŸ”΄ IT TAKES THE PROVENANCE BECAUSE THE BODY DOES. This number is PRINTED TO +// USERS β€” on the `Packaged …` line and again under a failed submit β€” as "what +// this CLI sent", and #411's stamp makes a submit that carries provenance ~70 +// bytes larger than one that does not. A signature that could not see the +// provenance would have kept reporting the smaller number, which is a small +// error in the quantity and a total one in the claim: the point of the line is +// that it is EXACT (see below), so it may not be an estimate the moment a +// feature lands. Pass a zero Provenance for a path that sends none +// (--package-only, the no-token fallback) and the number is unchanged. +// +// πŸ”΄ THE ZIP IS NOT WHAT GOES ON THE WIRE, AND THE DIFFERENCE IS THE WHOLE OF +// ISSUE #423. SubmitVersion base64-encodes the archive into a JSON document, so +// the bytes the server receives β€” and the bytes any request-body limit is +// applied to β€” are ~4/3 of the compressed size. An author reading +// `8201270 bytes compressed` off `app submit` had no way to see the ~10.9 MB +// that was actually sent, so nothing they could measure locally corresponded to +// the quantity that was refused. +// +// It is EXACT, not an estimate: base64's alphabet (A–Z a–z 0–9 + / =) contains +// no character encoding/json escapes, so the payload is copied through verbatim +// and the envelope is a constant. Do not substitute a 1.37 multiplier for it β€” +// the point of printing the number is that the author can compare it with a +// limit, and a rounded number cannot be compared with anything. +// +// πŸ”΄ THE ENVELOPE LENGTH DEPENDS ON THE PROVENANCE, AND THAT DECIDES WHETHER A +// BODY CAN LAND EXACTLY ON MaxSubmitBodyBytes. base64 output length is always a +// multiple of 4, so only an envelope whose own length is ≑ 0 (mod 4) can reach +// the ceiling at all. Measured through this function: +// +// provenance envelope mod 4 zipLen hitting the ceiling exactly +// none 19 3 unreachable +// commit only 77 1 unreachable +// commit + Dirty=false 97 1 unreachable +// commit + Dirty=true 96 0 7864246 +// +// The last row is what `civitai app submit --allow-dirty` produces on a dirty +// tree, so the boundary is reachable in production. See the guard in +// SubmitVersion for which way it is resolved. func SubmitBodySize(zipLen int, prov Provenance) int { return base64.StdEncoding.EncodedLen(zipLen) + submitEnvelopeLen(prov) } @@ -801,9 +823,36 @@ func (c *Client) SubmitVersion(ctx context.Context, zipBytes []byte, slug, versi // The message reports the BODY size, because that is the quantity the limit applies // to and the one an author could otherwise not observe: `app submit` prints the // compressed zip size, which is ~3/4 of this and clears the local cap comfortably. - if !c.AllowOversizeBody && len(body) > MaxSubmitBodyBytes { + // + // πŸ”΄ `>=`, NOT `>`, AND THE REASON IS AN ASYMMETRY β€” NOT A SETTLED FACT ABOUT THE + // SERVER. There is an unresolved contradiction here and it is stated rather than + // papered over: + // + // - Next.js's own source reads `bytesRead > bodySizeLimit`, which would ACCEPT a + // body of exactly 10485760. + // - A prior end-to-end measurement against the real endpoint read + // 10485759 -> 401 (the body reached auth), 10485760 -> 413 (rejected as too + // large), 12000000 -> 400. + // + // One of those is wrong and neither can be re-measured here β€” it needs credentials + // and a real submit. Do NOT write down a mechanism for it; nobody has one. + // + // The choice does not depend on resolving it. Refusing one byte early costs an + // author a flag they are already told about (--allow-oversize); accepting one byte + // too many costs them the whole ~10 MB upload and returns an error naming nothing + // about size (#423). The consequences are wildly unequal, so the guard takes the + // cheap side. + // + // πŸ”΄ IT IS NOT A HYPOTHETICAL BOUNDARY. `>` vs `>=` was long believed unobservable, + // because with a zero Provenance the envelope is 19 bytes and base64 steps by 4, so + // no body lands on the ceiling. That is true only for zero provenance. The envelope + // for commit + Dirty=true is 96 bytes β€” ≑ 0 (mod 4) β€” so a 7,864,246-byte zip + // submitted with `--allow-dirty` produces a body of EXACTLY 10485760. Under `>` the + // guard did not fire and the full body was uploaded. See SubmitBodySize's table, and + // TestSubmitBodyExactlyAtCeilingIsRefused, which pins this operator. + if !c.AllowOversizeBody && len(body) >= MaxSubmitBodyBytes { return nil, fmt.Errorf( - "%w: submit body is %d bytes, over the %d the server can receive. "+ + "%w: submit body is %d bytes, at or over the %d the server can receive. "+ "Reduce it and try again β€” `civitai app submit` lists the largest entries. "+ "If you believe the server now accepts more than this, --allow-oversize submits anyway. "+ "(The compressed zip is smaller than this number; base64 encoding adds ~1/3.)", diff --git a/internal/appapi/submit_ceiling_godoc_test.go b/internal/appapi/submit_ceiling_godoc_test.go new file mode 100644 index 00000000..8771c0e3 --- /dev/null +++ b/internal/appapi/submit_ceiling_godoc_test.go @@ -0,0 +1,139 @@ +package appapi + +import ( + "go/ast" + "go/parser" + "go/token" + "os" + "path/filepath" + "strings" + "testing" +) + +// TestExportedSubmitCeilingDeclsCarryTheirOwnDoc pins the ONE thing that broke +// silently in #585 and that no other check in this repo can see. +// +// πŸ”΄ A MISSING BLANK LINE REASSIGNS A DOC BLOCK, AND EVERYTHING STILL COMPILES. +// MaxSubmitBodyBytes's doc ran into ErrBundleTooLarge's with no blank line +// between them, so godoc handed the whole thing β€” the proxy-matcher evidence, +// the truncation chain, the two upstream issue numbers β€” to whichever +// declaration came first in source order. Measured at the commit that shipped: +// +// go doc appapi.MaxSubmitBodyBytes -> the bare const line, nothing else +// go doc appapi.ErrBundleTooLarge -> "MaxSubmitBodyBytes is the largest…" +// +// The constant that three published surfaces quote literally shipped with no +// godoc at all, and the sentinel shipped wearing the constant's evidence. Gofmt +// does not mind. `go vet` does not mind. staticcheck's ST1020-ST1022 would have +// caught the empty half and .golangci.yml disables them. +// +// πŸ”΄ THE ASSERTION IS "THE DOC NAMES ITS OWN DECLARATION", NOT "THE DOC IS +// NON-EMPTY". Non-emptiness is the weaker half and cannot see the failure that +// actually happened: ErrBundleTooLarge's doc was long, detailed and about +// something else entirely. Go's own convention β€” a doc comment begins with the +// identifier it documents β€” is what makes the misattribution mechanically +// visible, so that is what is checked. +func TestExportedSubmitCeilingDeclsCarryTheirOwnDoc(t *testing.T) { + // Walked file by file rather than with parser.ParseDir (deprecated in Go + // 1.25) or go/packages (a dependency this repo does not carry for tests). + // Only the doc comment attached to each declaration is needed, and that is + // on the AST directly. + entries, err := os.ReadDir(".") + if err != nil { + t.Fatalf("CONTROL failure, not a finding: readdir: %v", err) + } + fset := token.NewFileSet() + docs := map[string]string{} + parsed := 0 + for _, e := range entries { + name := e.Name() + if e.IsDir() || !strings.HasSuffix(name, ".go") || strings.HasSuffix(name, "_test.go") { + continue + } + f, err := parser.ParseFile(fset, filepath.Clean(name), nil, parser.ParseComments) + if err != nil { + t.Fatalf("CONTROL failure, not a finding: parse %s: %v", name, err) + } + parsed++ + for _, decl := range f.Decls { + switch d := decl.(type) { + case *ast.FuncDecl: + if d.Recv == nil { + docs[d.Name.Name] = d.Doc.Text() + } + case *ast.GenDecl: + for _, spec := range d.Specs { + vs, ok := spec.(*ast.ValueSpec) + if !ok { + continue + } + // A spec's own doc wins; an ungrouped decl's doc belongs to + // its single spec. + text := vs.Doc.Text() + if text == "" && len(d.Specs) == 1 { + text = d.Doc.Text() + } + for _, n := range vs.Names { + docs[n.Name] = text + } + } + } + } + } + + want := []string{"MaxSubmitBodyBytes", "ErrBundleTooLarge", "SubmitBodySize"} + + // POSITIVE CONTROL: the extractor must actually be finding these + // declarations. A walk that silently matched nothing would satisfy every + // loop below by iterating an empty map β€” the reassuring zero. + if parsed == 0 { + t.Fatal("CONTROL failure, not a finding: no non-test .go files were parsed, so this test is " + + "measuring nothing") + } + for _, n := range want { + if _, ok := docs[n]; !ok { + t.Fatalf("CONTROL failure, not a finding: %s was not found among the declarations of the "+ + "%d parsed file(s), so this test is measuring nothing. Found %d named declarations.", + n, parsed, len(docs)) + } + } + + for _, n := range want { + text := strings.TrimSpace(docs[n]) + if text == "" { + t.Errorf("%s has no doc comment. `go doc appapi.%s` prints the declaration and nothing else.\n"+ + "The usual cause is a MISSING BLANK LINE: the block above it runs into the block above "+ + "that, and godoc gives the whole thing to the earlier declaration.", n, n) + continue + } + if !strings.HasPrefix(text, n) { + // Name the thief when we can, because the fix is a blank line and + // the reader needs to know where. + var thief string + for _, other := range want { + if other != n && strings.HasPrefix(text, other) { + thief = other + break + } + } + msg := "%s's doc comment does not begin with %q β€” it begins %q.\n" + + "Go's convention is that a doc comment opens with the identifier it documents, and " + + "breaking it here means godoc is serving one declaration's evidence as another's." + if thief != "" { + msg += "\nThis doc belongs to " + thief + ": the two blocks are glued together by a missing " + + "blank line, so both `go doc` outputs are wrong at once. Separate them." + } + t.Errorf(msg, n, n, firstLine(text)) + } + } +} + +func firstLine(s string) string { + if i := strings.IndexByte(s, '\n'); i >= 0 { + s = s[:i] + } + if len(s) > 72 { + s = s[:72] + "…" + } + return s +} diff --git a/internal/appapi/submit_ceiling_value_test.go b/internal/appapi/submit_ceiling_value_test.go index 2a245240..5c00b7c2 100644 --- a/internal/appapi/submit_ceiling_value_test.go +++ b/internal/appapi/submit_ceiling_value_test.go @@ -63,7 +63,23 @@ func TestSubmitCeilingValueAndBoundary(t *testing.T) { want := strconv.Itoa(MaxSubmitBodyBytes) // A number of this shape in these files is the ceiling; there is no // other 8-digit literal starting 104 or 209 in them today. - suspicious := regexp.MustCompile(`\b(?:10485760|20971520|10 ?MiB|10 ?MB)\b`) + // + // πŸ”΄ UNIT-SPELLED FORMS ARE DELIBERATELY NOT POLICED, AND THE LIST NO + // LONGER PRETENDS THEY ARE. It used to read + // `10485760|20971520|10 ?MiB|10 ?MB` and then exempt matches with + // `!strings.HasPrefix(m, "10 ")` β€” which disagreed with itself about one + // number written two ways: the SPACED forms could never be reported while + // the UNSPACED `10MiB` could. Neither half was reachable coverage. + // + // They are dropped rather than repaired because a MiB spelling here is + // AMBIGUOUS, not merely unchecked. Enumerated over both files at this + // revision: `10 MiB` appears in each, and in both it is pkgzip's per-FILE + // cap, not this ceiling β€” alongside `2 MiB`, `50 MiB`, `200 MiB`, `32 MB` + // and others, none of which are the submit body limit. A rule that + // reported them would be wrong, and one that exempted them by prefix only + // looks like a rule. The byte literal is the unambiguous spelling and is + // what these surfaces quote; that is what this checks. + suspicious := regexp.MustCompile(`\b(?:10485760|20971520)\b`) for _, rel := range []string{ "README.md", "claudedocs/decisions/31-pkgzip-caps-are-not-a-server-mirror.md", @@ -79,7 +95,7 @@ func TestSubmitCeilingValueAndBoundary(t *testing.T) { continue } for _, m := range suspicious.FindAllString(body, -1) { - if m != want && !strings.HasPrefix(m, "10 ") { + if m != want { t.Errorf("%s still quotes %q alongside the current ceiling %s β€” one of them is stale.", rel, m, want) } @@ -89,35 +105,33 @@ func TestSubmitCeilingValueAndBoundary(t *testing.T) { // ── The BOUNDARY, driven through the REAL guard ──────────────────────── // - // Next.js refuses on `bytesRead > bodySizeLimit`, so a body of EXACTLY the - // ceiling is delivered intact and must NOT be refused here. `>=` would - // silently reject a submit the server accepts. - // // πŸ”΄ THIS CALLS SubmitVersion. The first version of this subtest asserted // `tc.size > MaxSubmitBodyBytes` β€” a RE-IMPLEMENTATION of the predicate in // the test, which mutating production could not affect. Measured: the `>=` // mutant SURVIVED it. An expectation derived from the implementation is not // a test of the implementation; the only thing that works here is running it. // - // πŸ”΄ AND EVEN SO, `>` vs `>=` IS UNOBSERVABLE THROUGH THIS API β€” SAID PLAINLY - // RATHER THAN LEFT AS FALSE COVERAGE. Measured: the envelope is 19 bytes and - // base64 steps by 4, so body sizes go … 10485759, 10485763 β€” nothing lands on - // 10485760 at all. There is no input that distinguishes the two operators, so - // the `>=` mutant survives this subtest too, and that is a fact about the - // arithmetic rather than a hole in the test. `>` is still the correct operator - // (Next.js refuses on `bytesRead > bodySizeLimit`), it is simply not reachable - // from here. What IS pinned below is the reachable boundary β€” the largest body - // accepted and the next one refused β€” which catches a removed guard and a - // changed constant. The envelope assertion guards the claim itself: if a - // future envelope makes the ceiling reachable, this stops being true and you - // should come back and pin the operator directly. - t.Run("a body of exactly the ceiling is accepted; one byte more is refused", func(t *testing.T) { - // Find the largest zip length whose body lands exactly on the ceiling or - // just under it. base64 is 3-byte granular, so not every body size is - // reachable β€” take the boundary pair that IS. + // πŸ”΄ THIS SUBTEST IS ZERO-PROVENANCE, AND THAT MAKES IT BLIND TO THE OPERATOR. + // With Provenance{} the envelope is 19 bytes and base64 steps by 4, so body + // sizes go … 10485759, 10485763 β€” nothing lands on 10485760, and `>` vs `>=` + // is genuinely unobservable FROM HERE. What it pins is the reachable pair + // either side of the ceiling, which catches a removed guard and a changed + // constant. + // + // ⚠ AN EARLIER VERSION OF THIS COMMENT GENERALISED THAT INTO "there is no + // input that distinguishes the two operators", AND THAT WAS FALSE. It + // measured one provenance and spoke for all of them. The envelope depends on + // the provenance, and commit + Dirty=true gives 96 bytes β€” ≑ 0 (mod 4) β€” so + // the ceiling IS reachable, at a zip of 7,864,246 bytes, on exactly the + // invocation `--allow-dirty` produces. The operator is pinned by + // TestSubmitBodyExactlyAtCeilingIsRefused below, not here. + t.Run("a body just under the ceiling is accepted; one step more is refused", func(t *testing.T) { + // Find the largest zip length whose body stays strictly under the ceiling. + // base64 is 3-byte granular, so not every body size is reachable β€” take + // the boundary pair that IS. zipAt := 0 for n := 0; ; n++ { - if SubmitBodySize(n, Provenance{}) > MaxSubmitBodyBytes { + if SubmitBodySize(n, Provenance{}) >= MaxSubmitBodyBytes { zipAt = n - 1 break } @@ -127,17 +141,19 @@ func TestSubmitCeilingValueAndBoundary(t *testing.T) { } underBody := SubmitBodySize(zipAt, Provenance{}) overBody := SubmitBodySize(zipAt+1, Provenance{}) - if !(underBody <= MaxSubmitBodyBytes && overBody > MaxSubmitBodyBytes) { + if !(underBody < MaxSubmitBodyBytes && overBody > MaxSubmitBodyBytes) { t.Fatalf("CONTROL failure: the bracket is wrong β€” %d bytes of zip gives a %d-byte body and "+ "%d gives %d, against a %d ceiling", zipAt, underBody, zipAt+1, overBody, MaxSubmitBodyBytes) } - // The claim above, pinned. If this ever fails, a body CAN land exactly on - // the ceiling and `>` vs `>=` became observable β€” pin the operator here. - if underBody == MaxSubmitBodyBytes { - t.Errorf("a body of exactly %d is now reachable (zip %d). The comment above says it is not, "+ - "and on that basis this test does NOT pin `>` vs `>=`. It can now: assert that this exact "+ - "size is ACCEPTED, which `>=` would refuse.", MaxSubmitBodyBytes, zipAt) + // CONTROL on this subtest's own stated scope: with zero provenance the + // ceiling must be UNREACHABLE, which is why this subtest cannot see the + // operator. If it ever becomes reachable here, this subtest's comment is + // wrong and it should pin the operator directly like the test below does. + if overBody == MaxSubmitBodyBytes || underBody == MaxSubmitBodyBytes { + t.Errorf("a zero-provenance body of exactly %d is now reachable (zip %d/%d). This subtest's "+ + "comment says it is not, and on that basis it does not pin `>` vs `>=`.", + MaxSubmitBodyBytes, zipAt, zipAt+1) } srv, hits := acceptingSubmitServer(t) @@ -145,13 +161,12 @@ func TestSubmitCeilingValueAndBoundary(t *testing.T) { c := NewWithSource(srv.URL, civitai.StaticToken("t"), "/api/blocks/submit-version") if _, err := c.SubmitVersion(context.Background(), make([]byte, zipAt), "slug", "1.0.0", Provenance{}); err != nil { - t.Errorf("a %d-byte body β€” at or under the %d ceiling β€” was REFUSED: %v\n"+ - "The comparison must be `>`, not `>=`: Next.js refuses on `bytesRead > bodySizeLimit`, so a "+ - "body of exactly the ceiling is delivered intact, and refusing it rejects a submit the "+ - "server accepts.", underBody, MaxSubmitBodyBytes, err) + t.Errorf("a %d-byte body β€” strictly UNDER the %d ceiling β€” was REFUSED: %v\n"+ + "Only a body AT or over the ceiling may be refused; refusing one below it rejects a submit "+ + "nothing suggests the server would.", underBody, MaxSubmitBodyBytes, err) } if *hits != 1 { - t.Errorf("the at-ceiling case contacted the server %d time(s), want 1 β€” it must actually upload", *hits) + t.Errorf("the under-ceiling case contacted the server %d time(s), want 1 β€” it must actually upload", *hits) } before := *hits @@ -168,6 +183,107 @@ func TestSubmitCeilingValueAndBoundary(t *testing.T) { }) } +// dirtyProvenance is the provenance `civitai app submit --allow-dirty` produces on +// a dirty tree: a real 40-hex commit plus Dirty=true. It is the ONLY one of the +// four shapes the CLI can send whose JSON envelope length is ≑ 0 (mod 4), which is +// what makes a body of exactly MaxSubmitBodyBytes reachable. +func dirtyProvenance() Provenance { + dirty := true + return Provenance{Commit: "0123456789abcdef0123456789abcdef01234567", Dirty: &dirty} +} + +// TestSubmitBodyExactlyAtCeilingIsRefused pins `>=` against `>` at the ONE input +// that can distinguish them, driven through the real SubmitVersion. +// +// πŸ”΄ WHY THIS EXISTS. #585 shipped with `len(body) > MaxSubmitBodyBytes` and a +// sibling subtest asserting in prose that the operator was unobservable β€” "there +// is no input that distinguishes the two operators". That generalised a +// measurement taken at Provenance{} to every provenance, and it is false. base64 +// output is always a multiple of 4, so the body can land on the ceiling only when +// the envelope length is too. Measured through SubmitBodySize: +// +// provenance envelope mod 4 zipLen hitting the ceiling exactly +// none 19 3 unreachable +// commit only 77 1 unreachable +// commit + Dirty=false 97 1 unreachable +// commit + Dirty=true 96 0 7864246 +// +// So `civitai app submit --allow-dirty` on a 7,864,246-byte zip produced a body of +// exactly 10485760 and `>` let the whole thing upload. The `>` mutant SURVIVED the +// entire suite before this test; it must not now. +// +// ⚠ WHAT THIS TEST DOES NOT CLAIM. It does not assert the server rejects exactly +// the cap. There is an unresolved contradiction β€” Next.js's source reads +// `bytesRead > bodySizeLimit` (which would accept it), while an end-to-end +// measurement read 413 at exactly 10485760 β€” and neither can be re-measured from a +// unit test. This pins the CLI's chosen side of an ASYMMETRY: refusing one byte +// early costs an author a documented flag, accepting one byte too many costs the +// full upload and an error naming nothing about size. +func TestSubmitBodyExactlyAtCeilingIsRefused(t *testing.T) { + prov := dirtyProvenance() + + // CONTROL: the fixture must actually sit ON the boundary, or every assertion + // below is about an ordinary over-ceiling body and the operator is untested. + const zipAtCeiling = 7864246 + if got := SubmitBodySize(zipAtCeiling, prov); got != MaxSubmitBodyBytes { + t.Fatalf("CONTROL failure, not a finding: a %d-byte zip with --allow-dirty provenance gives a "+ + "%d-byte body, not exactly %d. The envelope arithmetic moved, so this test no longer sits on "+ + "the boundary and cannot distinguish `>` from `>=`. Re-derive the zip length: it is the n for "+ + "which SubmitBodySize(n, dirtyProvenance()) == MaxSubmitBodyBytes.", + zipAtCeiling, got, MaxSubmitBodyBytes) + } + // CONTROL: and the provenance must be the one that survives sanitisation, or + // the envelope silently collapses to the zero-provenance 19 bytes. + if commit, dirty := prov.sanitised(); commit == "" || dirty == nil || !*dirty { + t.Fatalf("CONTROL failure, not a finding: dirtyProvenance() does not survive sanitised() "+ + "(commit=%q dirty=%v), so the body carries the zero-provenance envelope and the boundary "+ + "is unreachable", commit, dirty) + } + + srv, hits := acceptingSubmitServer(t) + defer srv.Close() + c := NewWithSource(srv.URL, civitai.StaticToken("t"), "/api/blocks/submit-version") + + _, err := c.SubmitVersion(context.Background(), make([]byte, zipAtCeiling), "slug", "1.0.0", prov) + if err == nil { + t.Fatalf("a submit body of EXACTLY %d bytes was uploaded. The guard must refuse at the ceiling, "+ + "not one byte above it: the comparison is `>=`, and `>` lets this exact body through. This is "+ + "reachable in production β€” it is what `civitai app submit --allow-dirty` sends for a "+ + "%d-byte zip.", MaxSubmitBodyBytes, zipAtCeiling) + } + if !errors.Is(err, ErrBundleTooLarge) { + t.Fatalf("the at-ceiling body was refused, but not by the size guard β€” so this test would stay "+ + "green with the guard deleted: %v", err) + } + // The refusal must be a PREFLIGHT, which is the entire point of the guard. + if *hits != 0 { + t.Errorf("the at-ceiling case contacted the server %d time(s); it must refuse before uploading", *hits) + } + // The message must report the real body size, not the zip. + if !strings.Contains(err.Error(), strconv.Itoa(MaxSubmitBodyBytes)) { + t.Errorf("the refusal names neither the body size nor the ceiling (both are %d here):\n %v", + MaxSubmitBodyBytes, err) + } + + // πŸ”΄ NEGATIVE CONTROL, and it is the half that makes the assertion above mean + // `>=` rather than "refuses everything". One base64 quantum BELOW the ceiling + // must still upload β€” `>=` accepts it and a guard that had drifted to `>=-4` + // or to an unconditional refusal would not. + belowZip := zipAtCeiling - 3 + belowBody := SubmitBodySize(belowZip, prov) + if belowBody >= MaxSubmitBodyBytes { + t.Fatalf("CONTROL failure, not a finding: the below-ceiling fixture is %d bytes, not under %d", + belowBody, MaxSubmitBodyBytes) + } + if _, err := c.SubmitVersion(context.Background(), make([]byte, belowZip), "slug", "1.0.0", prov); err != nil { + t.Errorf("a %d-byte body β€” under the %d ceiling β€” was refused: %v\n"+ + "The guard must be `>=` on the ceiling, not a blanket refusal.", belowBody, MaxSubmitBodyBytes, err) + } + if *hits != 1 { + t.Errorf("the under-ceiling case reached the server %d time(s), want 1 β€” it must actually upload", *hits) + } +} + // acceptingSubmitServer returns a server that accepts any submit, plus a counter // of how many requests reached it β€” the positive control for "did this actually // upload", which a bare error check cannot answer. diff --git a/internal/cmd/app_submit.go b/internal/cmd/app_submit.go index c7a9ac89..c18810d5 100644 --- a/internal/cmd/app_submit.go +++ b/internal/cmd/app_submit.go @@ -39,7 +39,11 @@ const submitDiagnosisEntries = 5 // this same block prints under a 500 that has nothing to do with the bundle. // What it can say is true of every one of those cases: here is what left this // machine. See issue #423 for the failure that made the distinction matter, and -// pkgzip's cap comment for why the honest move is to report rather than refuse. +// pkgzip's cap comment for why no ceiling on the compressed archive is guessed +// there. (That comment used to be summarised here as "the honest move is to +// report rather than refuse", which stopped describing the CLI at #585: the +// request BODY does have a vendored ceiling and is refused on it. Reaching this +// block means that ceiling did not fire.) // // It is on the FAILURE path only. On a success there is nothing to diagnose, // and the size already appears on the `Packaged …` line for anyone who wants @@ -58,10 +62,26 @@ func printSubmitSizeDiagnosis(w io.Writer, zipBytes []byte, prov appapi.Provenan } } - for _, line := range wrapRunes("The size the server applies any request-body limit to is the first number, "+ - "not the zip. This CLI's own size caps are not the server's and are much higher, so clearing them is "+ - "not a prediction that a submit will be accepted (issue #423). If the bundle carries files the platform "+ - "build does not need, drop them and retry:", 78) { + // πŸ”΄ THIS BLOCK RUNS ONLY WHEN THE VENDORED CEILING DID NOT FIRE, WHICH IS + // PRECISELY WHEN THE CEILING MAY BE WRONG. The paragraph below used to say + // only that the CLI's packaging caps are not the platform's and are much + // higher β€” true, and since #585 badly incomplete, because there is now a + // vendored request-body ceiling too and a body that got here cleared it. + // + // If the platform ever LOWERS proxyClientMaxBodySize below the number this + // CLI vendors, that is exactly the shape #423 had: the body clears the local + // guard, is truncated in transit, and comes back `400: Invalid JSON` β€” an + // error about the parse, naming nothing about size. An author told at that + // moment that the CLI has no idea what the platform accepts has been pointed + // away from the one number that could explain it. So the ceiling is named, + // with the direction it can be wrong in. + for _, line := range wrapRunes(fmt.Sprintf("The size a request-body limit applies to is the first number, "+ + "not the zip. This CLI's packaging caps are its own and are much higher, so clearing them is not a "+ + "prediction that a submit will be accepted (issue #423). It does refuse a body of %d bytes or more "+ + "before uploading, so this body was under that β€” but that ceiling is a vendored default, not "+ + "something the platform told us, and if the real limit has been lowered since it is now too high. "+ + "If the bundle carries files the platform build does not need, drop them and retry:", + appapi.MaxSubmitBodyBytes), 78) { fmt.Fprintf(w, " %s\n", line) } fmt.Fprintf(w, " %s # writes the exact .zip, so you can list it before retrying\n", @@ -95,13 +115,22 @@ func printSubmitSizeRefusal(w io.Writer, zipBytes []byte, prov appapi.Provenance } } + // πŸ”΄ EACH SENTENCE'S COLON INTRODUCES THE COMMAND THAT DOES WHAT IT SAID. The + // wording this replaced ended "--allow-oversize submits anyway:" and then + // printed `civitai app submit --package-only`, which never submits β€” the one + // command an author reaching for the escape hatch must not copy. Two claims + // were being made and only one command was offered, so the trailing colon + // attached the wrong one. for _, line := range wrapRunes("The first number is what the limit applies to, not the zip. Drop what the "+ - "platform build does not need and retry. If you believe the server now accepts more than this CLI "+ - "expects, --allow-oversize submits anyway:", 78) { + "platform build does not need and retry β€” this writes the exact .zip, so you can list it first:", 78) { fmt.Fprintf(w, " %s\n", line) } - fmt.Fprintf(w, " %s # writes the exact .zip, so you can list it before retrying\n", - ui.Code("civitai app submit --package-only")) + fmt.Fprintf(w, " %s\n", ui.Code("civitai app submit --package-only")) + for _, line := range wrapRunes("If you believe the platform now accepts more than this CLI expects, "+ + "submit anyway:", 78) { + fmt.Fprintf(w, " %s\n", line) + } + fmt.Fprintf(w, " %s\n", ui.Code("civitai app submit --allow-oversize")) } // skippedListCap is how many skipped paths the `Skipped …` line names before it @@ -558,7 +587,7 @@ Defaults to the current directory.`, // 3a. Programmatic submit if we have a token (OAuth or personal key). // The gate above already confirmed (or --yes bypassed) it. if canUpload { - return doUpload(cmd, client, pkg.Zip, m, cfg.BaseURL(), prov) + return doUpload(cmd, client, pkg.Zip, m, cfg.BaseURL(), prov, allowOversize) } // 3b. Fallback: write the canonical .zip + print next steps. @@ -624,23 +653,46 @@ func confirmSubmit(cmd *cobra.Command, m *manifest.Manifest, baseURL string, ass } } -func doUpload(cmd *cobra.Command, client appapi.Submitter, zipBytes []byte, m *manifest.Manifest, baseURL string, prov appapi.Provenance) error { +func doUpload(cmd *cobra.Command, client appapi.Submitter, zipBytes []byte, m *manifest.Manifest, baseURL string, prov appapi.Provenance, allowOversize bool) error { out := cmd.OutOrStdout() ctx := cmd.Context() if ctx == nil { ctx = context.Background() } + var r *appapi.SubmitResult + submit := func(ctx context.Context) error { + var e error + r, e = client.SubmitVersion(ctx, zipBytes, m.BlockID, m.Version, prov) + return e + } + + // πŸ”΄ DO NOT ANNOUNCE AN UPLOAD THAT WILL NOT HAPPEN. `Submitting …` goes to + // STDOUT and the refusal goes to STDERR, so an author over the ceiling read + // "Submitting demo@1.0.0" and then, from the other stream, that nothing was + // uploaded. The two are interleaved on a terminal and separated in a pipe; + // either way the first line is a claim the second contradicts. + // + // πŸ”΄ THIS PREDICTS, IT DOES NOT DECIDE. appapi.SubmitVersion remains the only + // place the refusal is made β€” one rule, one place. SubmitBodySize is exact + // (pinned by TestSubmitBodySizeMatchesRealMarshal against json.Marshal + // itself), so the two agree; but if they ever did not, the whole consequence + // is a spinner shown or withheld. Nothing here can refuse a submit the + // library would have sent, or send one it would have refused. + // + // allowOversize short-circuits because that flag makes the guard inert, and + // an upload really is about to start. + spin := allowOversize || appapi.SubmitBodySize(len(zipBytes), prov) < appapi.MaxSubmitBodyBytes + // Spin (on a TTY) while the bundle uploads β€” a real network wait. On a non-TTY // (pipe/CI/tests) WithSpinner prints one plain "Submitting …" line and runs the // upload inline, so scripted/captured output stays deterministic. - var r *appapi.SubmitResult - err := ui.WithSpinner(ctx, out, fmt.Sprintf("Submitting %s@%s", m.BlockID, m.Version), - func(ctx context.Context) error { - var e error - r, e = client.SubmitVersion(ctx, zipBytes, m.BlockID, m.Version, prov) - return e - }) + var err error + if spin { + err = ui.WithSpinner(ctx, out, fmt.Sprintf("Submitting %s@%s", m.BlockID, m.Version), submit) + } else { + err = submit(ctx) + } if err != nil { // πŸ”΄ THE SERVER'S MESSAGE IS ALREADY VERBATIM, AND FOR #423 IT NAMES // NOTHING. appapi.serverError prints the response body as it arrived, so diff --git a/internal/cmd/app_submit_oversize_test.go b/internal/cmd/app_submit_oversize_test.go index 776a0a95..ed87aeee 100644 --- a/internal/cmd/app_submit_oversize_test.go +++ b/internal/cmd/app_submit_oversize_test.go @@ -83,6 +83,23 @@ func TestAppSubmitOversizeEndToEnd(t *testing.T) { "`civitai app submit` lists the largest entries, so dropping the list makes that "+ "sentence false.\nstderr:\n%s", stderr) } + // πŸ”΄ STDOUT IS A SEPARATE SURFACE AND THIS TEST WAS BLIND TO IT. Every + // assertion above reads stderr. `Submitting demo@1.0.0` is written to + // STDOUT by the spinner, before SubmitVersion is called β€” so the CLI + // announced an upload on one stream and reported on the other that + // nothing had been uploaded, and the whole suite was green. A reader of + // a captured stdout log saw only the announcement. + if strings.Contains(stdout, "Submitting") { + t.Errorf("stdout announced the upload for a bundle that was never uploaded. `Submitting …` "+ + "goes to stdout and the refusal goes to stderr, so this line is contradicted by a stream "+ + "the reader may not have.\nstdout:\n%s", stdout) + } + // POSITIVE CONTROL on that assertion: it must be able to see the word at + // all, or it is a check against a stream nothing is written to. + if !strings.Contains(stdout, "Packaged") { + t.Fatalf("CONTROL failure, not a finding: stdout carries no `Packaged …` line, so the "+ + "`Submitting` check above is reading an empty stream and proves nothing.\nstdout:\n%s", stdout) + } }) t.Run("--allow-oversize reaches the client and the upload happens", func(t *testing.T) { diff --git a/internal/cmd/app_submit_test.go b/internal/cmd/app_submit_test.go index e8ae7fed..17abf893 100644 --- a/internal/cmd/app_submit_test.go +++ b/internal/cmd/app_submit_test.go @@ -41,7 +41,7 @@ func TestDoUploadHandsBytesToSubmitter(t *testing.T) { c.SetOut(&out) m := &manifest.Manifest{BlockID: "demo", Version: "0.1.0", Name: "Demo"} - if err := doUpload(c, fs, []byte("ZIPBYTES"), m, "https://civitai.com/", appapi.Provenance{}); err != nil { + if err := doUpload(c, fs, []byte("ZIPBYTES"), m, "https://civitai.com/", appapi.Provenance{}, false); err != nil { t.Fatalf("doUpload: %v", err) } if string(fs.got) != "ZIPBYTES" { @@ -69,7 +69,7 @@ func TestDoUploadSuccessOutput(t *testing.T) { m := &manifest.Manifest{BlockID: "my-block", Version: "0.2.0", Name: "My Block"} // trailing slash on baseURL must be trimmed when composing the link. - if err := doUpload(c, fs, []byte("ZIP"), m, "https://civitai.com/", appapi.Provenance{}); err != nil { + if err := doUpload(c, fs, []byte("ZIP"), m, "https://civitai.com/", appapi.Provenance{}, false); err != nil { t.Fatalf("doUpload: %v", err) } s := out.String() diff --git a/internal/cmd/exitcodes_claims_test.go b/internal/cmd/exitcodes_claims_test.go index ee8fdb8e..c11d0c9c 100644 --- a/internal/cmd/exitcodes_claims_test.go +++ b/internal/cmd/exitcodes_claims_test.go @@ -85,6 +85,8 @@ var exitCodeClaimsFloor = []string{ "a validation VERDICT is 1, and a manifest-less directory is a verdict", "`app validate --json` publishes a result only when it produced one", "5 is the retry code and a filesystem failure never lands there", + "a bundle the platform cannot receive is 1, not 2", + "the oversize refusal MOVED from 2 to 1, and scripts must follow", "a 429 is not always 6 β€” the deep-paging cap is a usage error and exits 2", "a retried 429 that never cleared exits 5, and prints a different message", "the Retry-After header is consulted before the message, so a cap 429 carrying it exits 5", @@ -92,6 +94,52 @@ var exitCodeClaimsFloor = []string{ func exitCodeContractClaims() []contractClaim { return []contractClaim{ + { + code: 1, + name: "a bundle the platform cannot receive is 1, not 2", + phrases: []string{ + // Chosen so a reversal cannot keep them. "refuses BEFORE + // uploading" is the preflight promise β€” the whole point of the + // code being a verdict rather than a transport outcome β€” and + // "the project is too big" is the sentence that places it with + // the other `app submit` verdicts rather than with usage errors. + "refuses BEFORE uploading", "the project is too big", + }, + why: "this refusal reaches exit 1 through exitCode's `default` arm β€” it matches no case in that " + + "switch β€” so nothing about the code is chosen by the mapper, and AGENTS.md item 7 is explicit " + + "that an untagged return unpins a published code. Right by accident is not the same as pinned: " + + "tagging the refusal with civitai.ErrBadRequest (the only route from a command error to 2) " + + "moves it silently. Measured at the commit that shipped #585, that mutation survived the " + + "entire suite", + pinnedBy: "TestOversizeBundleExitsGeneric in cmd/civitai, which takes the REAL error from the real " + + "`app submit` rather than a hand-tagged fixture, plus TestOversizeSentinelIsNotAnAPIKind", + }, + { + code: 2, + name: "the oversize refusal MOVED from 2 to 1, and scripts must follow", + phrases: []string{ + // The migration notice, not the classification. A rewrite that + // dropped the history would keep code 1's bullet entirely + // intact while deleting the only warning an existing script has. + // + // πŸ”΄ SPELLED SO ANOTHER BULLET CANNOT SATISFY THEM. A first draft + // used the bare "used to reach" and "must branch on", and the + // second was WALKABLE: code 2's `--json` bullet already contains + // "must branch on the exit code first", so rewording this bullet + // to "Scripts may need updating." left the row GREEN. Measured. + // Both phrases now carry the `1`/`2` the claim is actually about. + "is now refused LOCALLY and exits `1`", "must branch on `1`", + }, + why: "this is a BREAKING CHANGE to a published contract, and the only surface that says so. Before " + + "#585 an over-ceiling bundle reached the server, came back `400: Invalid JSON`, and exited 2 " + + "as a usage error; it is now refused locally and exits 1. A script that branches on 2 to " + + "report a too-large bundle silently stops matching β€” it does not error, it just never fires " + + "again. Code 1's own bullet states the new behaviour and cannot carry this: the reader who " + + "needs it is looking at 2, which is where their script is looking", + pinnedBy: "TestOversizeBundleExitsGeneric pins the destination (1). Nothing pins the ORIGIN, and " + + "nothing can β€” 2 was the server's answer, not a local decision, and it is no longer reachable " + + "from here. That is why the history is published rather than tested", + }, { code: 6, name: "the Retry-After header is consulted before the message, so a cap 429 carrying it exits 5", diff --git a/internal/pkgzip/pkgzip.go b/internal/pkgzip/pkgzip.go index a576fb61..1cdf3c0d 100644 --- a/internal/pkgzip/pkgzip.go +++ b/internal/pkgzip/pkgzip.go @@ -67,20 +67,36 @@ import ( // because caps_claim_test.go bans those phrasings from this file outright, and // a quotation the guard has to make an exception for is a guard with a hole. // -// πŸ”΄ THE SERVER'S REAL BUNDLE CEILING IS NOT KNOWN HERE, AND MUST NOT BE -// GUESSED. #423's measurement bounds it to the interval (2.32 MB, 8.20 MB] and +// πŸ”΄ NO CEILING FOR THE COMPRESSED BUNDLE IS KNOWN HERE, AND NONE MAY BE +// GUESSED. Read "bundle" strictly: the archive these caps measure, not the +// base64 JSON request body built from it β€” the CLI does vendor a limit for that +// one, and the last paragraph below says why the two do not collide. +// #423's measurement bounds a bundle ceiling to the interval (2.32 MB, 8.20 MB] and // no further β€” each additional probe costs a real submission. Vendoring a // number picked from inside that bracket would refuse bundles the server // accepts, with no appeal and nothing to tell the author it was the CLI's guess // rather than a real limit: strictly worse than the failure it would be fixing. // (Same reasoning as AGENTS.md item 25, one layer up.) // -// So these caps stay generous, and the CLI REPORTS instead of refusing: -// appapi.SubmitBodySize is the size a body limit would actually apply to (the -// zip is base64-encoded into a JSON document before it is sent, so the server -// sees ~4/3 of the number below), and LargestEntries names what to delete -// first. Both are printed by `civitai app submit` β€” the second only when a -// submit has already failed. +// So these caps stay generous, and nothing in THIS package refuses on a platform +// limit; it reports. appapi.SubmitBodySize is the size a body limit actually +// applies to (the zip is base64-encoded into a JSON document before it is sent, +// so what crosses the wire is ~4/3 of the number below), and LargestEntries names +// what to delete first. Both are printed by `civitai app submit` β€” the second +// only when a submit has already failed. +// +// ⚠ SINCE #585 THE CLI DOES REFUSE β€” ONE LAYER UP, ON A DIFFERENT QUANTITY. An +// earlier version of this paragraph said flatly that the CLI reports instead of +// refusing, and that is no longer true of the CLI as a whole: `civitai app +// submit` declines before uploading when the JSON BODY would reach +// appapi.MaxSubmitBodyBytes, with `--allow-oversize` as the way past it. +// +// That does not license a number here, and it is not a contradiction of the +// paragraph above. It bounds the request BODY, not the archive; it is a framework +// default the CLI vendors rather than anything measured about a bundle; and the +// interval above is still the whole of what is known about the compressed +// archive. The two limits are independent β€” a bundle can clear every cap in this +// file and still be refused up there, which is exactly what #423 was. const ( MaxFiles = 2000 MaxFileSizeBytes = 10 * 1024 * 1024 // 10 MiB per file @@ -777,12 +793,22 @@ func Build(dir string) (*Result, error) { } if int64(len(buf.b)) > MaxBundleSizeBytes { - // MaxBundleSizeBytes is this CLI's own cap, and the server's real - // ceiling is LOWER and unknown here (#423). The wording this replaced + // MaxBundleSizeBytes is this CLI's own cap. The wording this replaced // attributed the number to the platform, which told an author that - // clearing this bar meant the server would take the bundle β€” the claim - // #423 disproved. caps_claim_test.go keeps that attribution out. - return nil, fmt.Errorf("package is %d bytes compressed (the CLI's own cap is %d; the server's own limit is lower and is not known to this CLI β€” see issue #423)", len(buf.b), MaxBundleSizeBytes) + // clearing this bar meant the bundle would be taken β€” the claim #423 + // disproved. caps_claim_test.go keeps that attribution out. + // + // ⚠ IT ALSO SAID "the server's own limit is lower and is not known to + // this CLI", AND THE SECOND HALF STOPPED BEING TRUE AT #585. The CLI now + // vendors a request-BODY limit and refuses on it before uploading, so an + // author reading "not known to this CLI" would conclude no local ceiling + // exists when one had just refused them. What genuinely remains unknown + // is a ceiling on the COMPRESSED ARCHIVE, which is what this message is + // about β€” so it says that, and points at the limit that does exist. + return nil, fmt.Errorf("package is %d bytes compressed (the CLI's own cap is %d). "+ + "No platform limit on the compressed archive is known to this CLI β€” see issue #423. "+ + "A separate and much lower ceiling applies to the base64 JSON request body `civitai app submit` "+ + "uploads, and that one is enforced before the upload", len(buf.b), MaxBundleSizeBytes) } return &Result{