fix(submit): refuse AT the body ceiling, and pin the contract #585 published

[ZacxDev]

Follow-up to #585, carrying findings from a round-0 and a round-1 audit of it. Base is main @ 7467c62. No file overlap with #596 or #600.

The headline is an off-by-one that a shipped test asserted was unobservable.

---

1. The ceiling guard was off by one, and its "unreachable" claim was false

internal/appapi/appblocks.go refused on len(body) > MaxSubmitBodyBytes, so a body of exactly 10485760 uploaded in full.

submit_ceiling_value_test.go said in prose that this could not matter — "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." That measured one provenance and spoke for all four. Measured through the real SubmitBodySize:

provenance	envelope	mod 4	zipLen giving body == 10485760
none	19	3	unreachable
commit only	77	1	unreachable
commit + Dirty:false	97	1	unreachable
commit + Dirty:true	96	0	7,864,246

base64 output is always a multiple of 4, so only an envelope that is itself ≡ 0 (mod 4) can land on the ceiling — and that is exactly what civitai app submit --allow-dirty sends. A 7,864,246-byte zip therefore produced a 10,485,760-byte body the guard waved through: the precise body #585 exists to stop.

Changed to >=.

The reasoning is an asymmetry, not a claim about the server

There is a real unresolved contradiction here and the code, the README and this PR all say so rather than inventing a mechanism:

• Next.js's source reads bytesRead > bodySizeLimit, which would accept a body of exactly 10485760.
• An end-to-end measurement of the real endpoint read 10485759 -> 401 (the body reached auth), 10485760 -> 413, 12000000 -> 400.

One of those is wrong. Neither can be re-measured from a unit test — it needs credentials and a real submit — so nobody should write down which. The decision does not depend on resolving it: refusing one byte early costs an author a flag they are already told about (--allow-oversize), while accepting one byte too many costs the whole ~10 MB upload and returns an error naming nothing about size. The consequences are wildly unequal, so the guard takes the cheap side.

Pinned by TestSubmitBodyExactlyAtCeilingIsRefused, which drives the real SubmitVersion at the one input that can tell the operators apart, with a negative control one base64 quantum below (so the assertion means >= and not "refuses everything") and two CONTROL checks that the fixture really sits on the boundary and that the provenance survives sanitised().

2. MaxSubmitBodyBytes shipped with no godoc

A missing blank line glued its doc block to ErrBundleTooLarge's. Measured before:

go doc appapi.MaxSubmitBodyBytes  ->  the bare const line, nothing else
go doc appapi.ErrBundleTooLarge   ->  "MaxSubmitBodyBytes is the largest…" + the whole evidence chain

The constant three published surfaces quote literally had no godoc; the sentinel wore the constant's evidence. Each declaration now carries its own doc (verified with go doc, not by reading), and the derivation block that documented nothing moved onto SubmitBodySize, which it was always about.

Added TestExportedSubmitCeilingDeclsCarryTheirOwnDoc, which asserts each doc begins with its own identifier rather than merely being non-empty — non-emptiness cannot see the failure that happened, since ErrBundleTooLarge's doc was long, detailed, and about something else.

3. The published exit-code contract had no guard

exitcodes_doc.go publishes that the refusal exits 1, and — under code 2 — that "a script branching on 2 for that case must branch on 1", a documented breaking change. appblocks.go called the code "now assertable". It was assertable and not asserted: no ledger row, no floor entry, no reference to ErrBundleTooLarge under cmd/civitai/. Behaviour was correct only by falling through exitCode's default.

Added TestOversizeBundleExitsGeneric (the real error from the real command, not a hand-tagged fixture) + TestOversizeSentinelIsNotAnAPIKind, mirroring TestDirtyWorkTreeExitsGeneric, plus two contractClaim rows and their exitCodeClaimsFloor entries.

4. A comment named a guard that does not enforce what it claimed

appblocks.go said pkgzip/caps_claim_test.go is "exactly the claim … that test exists to keep OUT of that file, and the reason this constant lives here instead." Measured with both controls:

• Real probe — a fully server-attributed constant (🔴 This IS the server's number + the whole evidence chain) using none of the four banned phrases: PASSES.
• Negative control — appending caps mirror the server: FAILS with the test's own message.

So the guard bans four literal spellings; it does not force the placement. The placement is still correct on architectural grounds (appapi builds the body, so the body's limit belongs beside SubmitBodySize) — the constant did not move, only the stated reason. claudedocs/decisions/31 was already accurate on this and is untouched.

5. Three user-facing strings contradicted shipped behaviour

All three said the CLI does not know any server ceiling, while it vendors 10485760 and refuses on it.

The one with teeth is printSubmitSizeDiagnosis: it runs only when the vendored ceiling did not fire, which is precisely the situation where that number may be too high. If the platform ever lowers proxyClientMaxBodySize, a body under the vendored value clears the guard, is truncated, returns 400: Invalid JSON — #423 exactly — and the diagnosis told the author no local cap existed, pointing them away from the only number that could explain it. It now names the ceiling and the direction it can be wrong in.

pkgzip's wording is narrowed to the compressed archive, which genuinely has no known platform ceiling — as distinct from the request body, which does. caps_claim_test.go re-run and green; no banned phrase introduced.

6. Three cheap ones

• Spinner — Submitting … goes to stdout and the refusal to stderr, so the CLI announced an upload it then declined. The spinner is now skipped for a body it can predict will be refused. It predicts, it does not decide: appapi.SubmitVersion remains the only place the refusal is made, so a disagreement could only show or withhold a spinner. app_submit_oversize_test.go read only stderr and was blind to this; it now asserts stdout, with a positive control that it is reading a non-empty stream.
• Mismatched command — the refusal's trailing colon promised "--allow-oversize submits anyway" and then printed civitai app submit --package-only, which never submits. Both commands are now offered under their own sentence.
• Self-inconsistent regex — 10 ?MiB|10 ?MB were listed as suspicious then exempted via !strings.HasPrefix(m, "10 "), so spaced forms could never be reported while unspaced 10MiB could. Dropped rather than repaired: enumerated over both files, 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). A rule reporting them would be wrong; one exempting them by prefix only looks like a rule.

---

Mutation results

Each mutant applied in a cp -a copy with rm -f <copy>/.git first, and watched to fail for its own reason:

mutant	result
>= → >	RED — TestSubmitBodyExactlyAtCeilingIsRefused: "a submit body of EXACTLY 10485760 bytes was uploaded"
ceiling shifted down one base64 quantum	RED — same test's negative control: "a 10485756-byte body — under the 10485760 ceiling — was refused"
refusal tagged civitai.ErrBadRequest	RED — TestOversizeBundleExitsGeneric: "exitCode(...) = 2, want 1". SURVIVES the full suite at main (rc=0, 21/21 packages)
code-2 ledger row deleted	RED — the floor: "the ledger row … is gone"
each of the 4 ledger phrases reworded out of the doc	RED independently, one assertion each

Red at origin/main, green here: TestSubmitBodyExactlyAtCeilingIsRefused; the new stdout assertion in TestAppSubmitOversizeEndToEnd; both halves of TestExportedSubmitCeilingDeclsCarryTheirOwnDoc (which names MaxSubmitBodyBytes as the thief correctly).

TestOversizeBundleExitsGeneric passes at main unmutated — it is an invariant guard on the published contract, not a regression test, and is labelled as such. The mutation above is what shows it has teeth.

One finding this PR made against itself

A first draft of the code-2 ledger phrases used the bare "must branch on". That is walkable: code 2's --json bullet already contains "must branch on the exit code first", so rewording the migration notice to "Scripts may need updating." left the row GREEN. Caught by mutating, then respelled with the 1/2 the claim is actually about, and re-mutated to confirm both phrases now kill independently.

Gate

Makefile target ci = tidy vet test build. All four run:

• go vet ./... — clean
• go test ./... -count=1 — rc=0, 21/21 packages ok, 0 failures
• go build ./... — ok
• go mod tidy — no change

Both tiers, stated separately. The above is the full-clone tier. The depth-1 tier (scripts/ci-shallow.sh, which is also what CI's build-test job runs — actions/checkout@v4 defaults to fetch-depth: 1) was run too: ok=21/21 failing-tests=0 failing-packages=0 timeouts=0 filtered-empty=0.

golangci-lint 2.13.2 locally: 0 issues. CI's lint job (2.12.2) initially caught two staticcheck findings in this PR's own new test files — ST1008 (error not last) and SA1019 (parser.ParseDir deprecated in Go 1.25). Both fixed in the second commit; the godoc guard was re-verified RED at main after the rewrite rather than assumed. All 13 checks now green.

Not verified

• Which side of 10485760 the platform actually sits on. Deliberately unresolved above, and no code comment or doc claims a mechanism for it.
• This does not address app submit: MaxSubmitBodyBytes is vendored and unguarded — add a drift guard like bump-scaffold-pins #599 (a drift guard that would notice the day proxyClientMaxBodySize moves). It makes the staleness legible — printSubmitSizeDiagnosis now names the vendored number and the direction it can be wrong in — but nothing here detects the change.

[ZacxDev]

Heads-up from the session that just merged #601 (b727a83): this PR and that one both edit internal/cmd/exitcodes_claims_test.go, and #601 landed first, so this branch is now 1 commit behind main and its shared-file measurement is stale.

gh pr view reports MERGEABLE/CLEAN, but that is only the textual claim. The two changes are in different parts of that file — #602 appends exitCodeClaimsFloor entries and prepends claim rows; #601 rewrote the code-6 row's pinnedBy (it now names two guards instead of saying "nothing local") and added a comment above it. An audit test-merged an earlier pair of heads (cc24058 + f02c2003, branching on git merge-tree --write-tree's exit code, not a marker grep) and got a clean tree at 21 ok / 0 FAIL — but #601 then gained a second commit (af35fbf) that touches that file again, so that reading no longer covers this pair.

Cheap re-check before merging, since a clean git merge is not a clean merge:

git fetch origin
git merge-tree --write-tree origin/main <602-head>; echo "rc=$?"   # non-zero = conflict
# then materialise it and run the suite, and diff for silently-dropped rows:
diff <(git show origin/main:internal/cmd/exitcodes_claims_test.go) internal/cmd/exitcodes_claims_test.go

The last one matters most here: #585's merge silently dropped #591's two Troubleshooting rows on a resolution that conflicted nowhere (fixed by #598). The "lines main has that we lack" set should be empty except for what this PR deliberately changes.

No action needed from #601's side — flagging it so the second merge doesn't repeat that.