fix(download): stop a server-supplied file name forging lines, including a fake (SHA256 verified)

[ZacxDev]

Closing condition of #577. The highest-impact item left on the download path: the CLI asserting an integrity result that did not happen.

Reproduced before fixing

internal/saferune deliberately retains \n and \t ("Cc, minus \n and \t") so multi-line fields can format legitimately — so safeTerm does not stop a server-supplied files[].name from forging whole lines.

With files[].name = "weights.safetensors\nSaved /home/u/legit.safetensors (4.0 GiB) (SHA256 verified)", the progress line rendered:

  weights.safetensors
Saved /home/u/legit.safetensors (4.0 GiB)  (SHA256 verified)  1 B / 1000 B (0%)

A forged integrity claim on its own line, before any bytes were verified. The line is \r-rewritten at 10 Hz, so the forged text is stranded above the rewrite point where nothing overwrites it, and done() leaves it standing.

Every other forgery on this path misleads about a name. This one misleads about whether the bytes are the bytes.

Not a regression — all four surfaces were fully raw before #572, which gated them with safeTerm and stated this residual rather than hiding it. #552's closure by #573 covered ~13 tabwriter renderers and never had download.go on its table, so it had no tracking object until #577.

The transform #577 deliberately did not prescribe

The issue said deciding which transform each surface takes is the work, because these are free text rather than tabwriter cells. The answer turns out to be the same for all four: they are single-line contexts — a \r-rewritten progress line, a row in a group listing, a one-line error. A group listing is made of rows, and a row is a line. So all four take safeTermSingle, which already exists and collapses \n and \t to a space.

The whole path moved, not the four reported sites

#566 exists because two spellings of one rule drifted apart, and #572 gated downloadStatusError once at the top for exactly that reason. Fixing four sites and leaving their structural siblings raw is that mechanism again. All 32 safeTerm( call sites in download.go became safeTermSingle(; safeTermErr's five are untouched.

🔴 No coverage left the origin ledger's view. safeterm_userinput_test.go scans both spellings — its own comment records that exact hazard — and reports the same 199 sites before and after. Worth stating because "convert every call to a wrapper" is precisely the edit that silently drops a name-keyed scanner's coverage, and a green suite would not have shown it.

Mutation matrix — each surface reverted alone, each dies naming itself

surface reverted to safeTerm	failure
(*progressWriter).line	FORGERY in (*progressWriter).line: 1 newline(s)
downloadStatusError	all three arms, each named individually
downloadOne SHA256 mismatch	FORGERY in downloadOne (SHA256 mismatch)
checkTargetCollisions rows	FORGERY in checkTargetCollisions

The collision bound is differential, not arithmetic: a hostile name must render the same number of lines a benign one does. Its first draft hand-counted "header + group line + one row per file", missed the trailer, and reported a forgery that was not there — a hand-counted bound is a second claim that can be wrong independently of the thing it checks.

Three comments rewritten

download.go carried three blocks stating this residual as open and the transform question as unanswered. A comment is a claim; those now had the wrong answer to a question they said had none.

Gate

make ci green (21 packages ok); make lint 0 issues.

Refs #552, #566, #572, #577.

[ZacxDev]

Round 1's claims block, for the next round. Each line is WHAT the fixes in 67cad76 claim to have addressed — not why it is correct.

Round 0 and a first-full nine-axis round both ran against 208676c; this block covers the single fix commit answering both.

1. safeTermErr now calls safeTermSingle rather than safeTerm, so the `%w` cause in every `%s: %w` pair on the download path is collapsed to one line; its five callers are all in download.go.
2. The forgery through the wrapped cause was REPRODUCED before fixing: writePart's create error rendered two lines, the second entirely attacker-written, via *fs.PathError's unquoted Path carrying filepath.Base(f.Name).
3. TestWrappedCauseCannotForgeALine was added because reverting safeTermErr reddened NOTHING before it existed — the claim is that the fix is now pinned, and the revert names writePart's create error.
4. README.md's `SHA256 mismatch for` Troubleshooting row no longer says newlines are kept on these surfaces; it states the collapse plus two deliberate exceptions (the %q refusal, and multi-line server text outside this path).
5. targetPath's unusable-filename refusal is REVERTED to safeTerm, on the ground that %q escapes \n so the line was never forgeable there and collapsing destroyed information the message exists to convey.
6. download_safeterm_test.go's header — the fourth in-tree statement #577 names — no longer says the residual is open, and records that the first pass closed it only half-way.
7. Four stale `safeTerm` comment references in download.go were retargeted, and a note states the file now has zero bare safeTerm calls.
8. The tab subtest of TestCheckTargetCollisionsCannotForgeARow was VACUOUS — measured passing on fully unfixed code — and now carries its own per-row tab assertion.
9. TestDownloadStatusErrorCannotForgeALine now drives FOUR arms including `default`, and both payloads at each; the comments claiming "three arms" in two files were corrected.
10. Reverting only the \t half of safeTermSingle is claimed to redden four named arms, where before it reddened one.
11. The soft-wrap residual is STATED rather than fixed: p.name is length-unbounded, so a name padded to terminal width strands the identical forged text with no \n and no \t — claimed measured at widths 80/100/120/132 as 14 display rows, 12 forged — and TestProgressLineCannotForgeALine's name is explicitly disclaimed as not being the outcome.
12. No site among the 32 takes a legitimately multi-line value, so the widening flattens nothing real.
13. make ci is claimed green at 21 packages and make lint at 0 issues.

[ZacxDev]

Round 2 — four 🟡, none in the shipped payload

Delta over 208676c..67cad760. The round's two shipped payload lines are correct — the
auditor independently reproduced the forgery they close and confirmed the %q premise behind
the targetPath revert. Every finding is in what round 1 wrote about them.

🔴 A comment that denied its own code, inviting a change nothing catches

download.go:696 said "There are no bare safeTerm calls left in this file" while :822
is one — created by the same commit.

Measured: a maintainer acting on that sentence changes :822 to safeTermSingle, it
compiles, and the entire suite stays green — the fixture carries no \n/\t, so both
spellings render identically. The user whose server sent a newline-bearing name then silently
loses the byte the message exists to show.

The sentence now says there is exactly one, names it, and says not to "fix" it: %q escapes
\n and \t to their two-character forms, so nothing can forge a line there, and collapsing
them first destroys information.

And the exception is now guarded — TestTargetPathRefusalSanitizesTheServerName gets a
newline-bearing payload asserting the escaped \n survives and no raw newline reaches the
terminal. That mutant now fails with the guard's own message. Confirmed compiling first: an
earlier attempt died as a syntax error, which proves nothing.

🔴 The new guard's "POSITIVE CONTROL" could not fail

It read err.Error() — "create <sanitised partPath>: <cause>" — where the %s operand
supplies weights.safetensors independently of the cause. So it stayed green in exactly
the condition its own sentence claims to rule out. Measured: replacing the wrapped cause with
a name-free fs.ErrNotExist left it green.

It now unwraps and asserts on the cause; the same mutant exits 1.

🟡 safeTermErr's contract said two incompatible things

In one paragraph: "use it at ANY site where %w carries server-derived bytes" and
"every caller is on download.go's single-line error path". Both cannot be the contract, and
nothing decided it. The wide reading fails silently — a caller elsewhere wrapping a
legitimately multi-line server string (an orchestrator failure reason, the case
indentContinuation exists to format) is flattened, and nothing goes red.

Scoped the comment to the narrow reading, and added TestSafeTermErrCallersAreLedgered
pinning the five sites by file and enclosing function, failing both ways:

mutation	result
add a call in another file	RED — is called in zz_grow_probe.go, which is NOT on the ledger
remove one of the five	RED — call sites in download.go have MOVED

It carries a positive control, because a scan matching nothing would satisfy every
"unledgered set is empty" check and pass cheerfully.

🟡 README asserted a shared reason true of only one exception

The SHA256 row said the two deliberate exceptions were "both for the same reason — the text
is escaped rather than printed raw". True of the %q refusal; false of prompts and
failure reasons, which are printed raw and indented. A maintainer acting on the stated
reason could remove the indentation guard — reopening #367. Each exception now carries its own
reason, with an explicit "do not read one as covering the other".

---

And a self-inflicted CI failure worth recording

The ledger test used parser.ParseDir, deprecated in Go 1.25; staticcheck's SA1019 reddened
the lint job, which was green on the previous commit. The reason it reached CI is the one
AGENTS.md states outright: make ci does not run golangci-lint, so a green make ci is a
claim about four steps, not about the gate. I ran make ci and stopped.

Fixed to os.ReadDir + ParseFile — the idiom the sibling ledgers in this package already
use, so it was in the tree and I reached past it. Re-verified both mutations after the rewrite
rather than assuming they carried over; golangci-lint reports 0 issues, and that zero is
meaningful because CI had just shown this linter going red on this file.

---

Ledger: round 2 · payload lines changed THIS round: 0 · elapsed: ~1.5h. Comments, tests
and one README row; the two shipped behaviour lines from round 1 are untouched.

1. download.go:696's false "no bare safeTerm calls left" is corrected, names the one exception, and says not to change it
2. that exception is guarded: a newline-bearing payload asserts the escaped \n survives; changing :822 to safeTermSingle now fails with the guard's own message, on a mutant go vet accepts
3. TestWrappedCauseCannotForgeALine's positive control now unwraps and asserts on the CAUSE; a name-free cause now fails it where it was green
4. safeTermErr's doc comment is scoped to download.go's single-line path, and TestSafeTermErrCallersAreLedgered pins the five sites, failing on GROW and on SHRINK, with a positive control
5. README's SHA256 row gives each deliberate exception its own reason and warns against reading one as covering the other
6. the ledger uses os.ReadDir+ParseFile, not the deprecated ParseDir; golangci-lint 0 issues, after CI caught SA1019 because make ci does not run lint

[ZacxDev]

Round 3 — three findings, all in scaffolding this ladder wrote

Delta over 67cad760..ac4500d, dispatched blind. All six round-2 claims verified actually
fixed, each re-measured against the base tree rather than accepted — including that the
safeTermSingle mutant is green at 67cad760 and red at head.

Zero executable payload lines in the range, confirmed mechanically: strip comments from
the download.go + safeterm.go diff and nothing remains.

🔴 The ledger attributed to "the last FuncDecl seen", not the enclosing function

A flat ast.Inspect with a running fn computes the most recent FuncDecl in the file —
the enclosing function only by coincidence. A call in a package-level
var x = safeTermErr(...) is attributed to whichever function sits above it, so a call can
leave the single-line path while the multiset stays identical and the ledger reports it as
ledgered.

Now walks file.Decls and attributes to the decl, keeps the receiver (a method
writePart is not the function writePart), and adds the totality assertion: a flat
count of the same calls must equal the attributed count.

That check is not invented here — safeterm_userinput_test.go's scanner already documents
this exact class under "🔴 TOTALITY", and this package owns the correct walk. I reached past
it and re-derived a worse one.

mutant	result
call moved to package level, count held constant	RED — only 4 are inside a function declaration
call added in another file	RED — NOT on the ledger
one of the five removed	RED — have MOVED

🔴 A real regression was labelled "CONTROL failure, not a finding"

Changing writePart's %w to %s drops the wrap that safeterm.go says keeps the
exit-code classifier able to see the sentinel (AGENTS.md items 7 and 24). Exactly one test
notices — and it told the reader to disregard it, because "CONTROL failure, not a finding"
is this repo's idiom for the harness broke.

It now says REGRESSION, names the likely cause, and is verified to fire on that mutant.

Same shape in the ledger's zero-control, which asserted the function "is called at least five
times" — a false statement about the tree in the legitimate case where the last caller was
removed. It now names both causes and says they need opposite responses.

🟢 A bold line that read as its own opposite

"🔴 DO NOT use it at ANY site" skims as "never call this function" and is false of five live
call sites — the same read-the-bold-line failure this comment exists to fix. Now
"USE IT ONLY ON download.go's SINGLE-LINE ERROR PATH".

---

Also: a pre-existing flake, filed not re-run past

TestFilesystemErrorsExitGenericEndToEnd/control: a real transport failure still exits 5
failed once here. closedLoopbackAddr binds an ephemeral port, closes it, and assumes
nothing rebinds it — so the "dead" URL can be live and models search exits 0 with
"No models found". The subtest took 1.98s; a refused dial is instant.

Green on this branch's three previous commits and on main; re-run passed. Filed as
#593 rather than left as folklore — it is a positive control, and a control
that flakes trains the reflex of dismissing the one assertion whose job is to prove the
harness works.

Ledger: round 3 · payload lines changed THIS round: 0 · elapsed: ~1h.
Gate: make ci rc=0, 0 FAIL, golangci-lint 0 issues.

1. the ledger walks file.Decls and attributes to the enclosing decl, keeping the receiver for methods
2. it asserts TOTALITY — flat count equals attributed count — so a call enclosed by no FuncDecl can no longer be silently uncovered
3. all three mutants are red: package-level call with the count held constant, a call added elsewhere, and one removed
4. the %w→%s regression now reports REGRESSION and names the likely cause, instead of "CONTROL failure, not a finding"; verified to fire
5. the ledger's zero-control no longer asserts a false statement about the tree in the legitimate last-caller-removed case
6. safeterm.go's bold line now says USE IT ONLY ON the single-line path rather than reading as "never call this"
7. civitai/cli#593 filed for the closedLoopbackAddr bind-then-close race; not caused by this PR, green on its three previous commits