internal/report claims it never exits, and it does

[haribo]

Why

internal/report's package comment, written when the package was split out in #491, says:

Nothing here writes to stdout or decides an exit code; it takes results and returns text,
which is what makes it testable.

report.go:192 calls os.Exit(1), inside JSON(), on a marshal failure.

The comment is mine, and so is the os.Exit. It was named in #585's description as "the
one piece of entanglement left in the package" and deliberately left — then the package
comment was written claiming the opposite. A reader has no way to tell which is true
without grepping.

What to do

Two coherent options, and the choice matters more than the line:

1. Make the comment true. JSON returns the marshal error, the caller decides. Its
   signature becomes (string, bool, error) — or the error folds into the bool with the
   message in the string, which is uglier but keeps the shape. Render and both call sites
   follow.
2. Make the comment honest. Keep the exit, and say why: the failure is documented as
   impossible ("every field is a plain Go value"), and an empty report would be worse than a
   loud death.

Option 1 is the one that matches what the package is for. Option 2 is defensible only if
the exit is genuinely unreachable — in which case the honest form is a panic with a message,
not an exit, since an unreachable branch that exits looks like a decision.

Validation

• Whichever way: grep -n "os.Exit" internal/report/ and the package comment agree.
• The JSON report still ends with a newline and still redacts, which its tests cover.

[haribo]

Audit against the current code

Confirmed, and there are two contradicting comments, not one.

• internal/report/report.go:6-8 (package comment): "Nothing here writes to stdout or
  decides an exit code; it takes results and returns text, which is what makes it testable."
• internal/report/report.go:50-51 (on Render): "Printing it and exiting stay with the
  caller — a package that calls os.Exit cannot be tested, which is the entanglement this
  split undoes."
• internal/report/report.go:188-192: on a marshal failure, fmt.Fprintf(os.Stderr, …) then
  os.Exit(1).

The second comment is the sharper contradiction: it states the rule as the reason for the
package's existence, twelve lines above the line that breaks it.

Feasibility of option 1, checked: JSON is called from exactly two places —
report.go:57 (inside Render) and cmd/shellf/main.go:554 (the status --json path). Both
already handle a (string, bool) pair and both sit in a function that can exit. So the
signature change reaches three functions and no further; it is not the invasive option it
might look like.

One point against option 2 as written. The issue says a panic would be the honest form of
an unreachable branch, and the code agrees with itself here: JSON marshals a struct of
strings, bools and slices, so json.Marshal cannot fail on it. But panic in a library is
not obviously better than os.Exit for an operator — it prints a stack trace over the report.
Option 1 removes the question instead of answering it.

Recommendation: option 1. It is what the package was split out to be, it costs three
functions, and it leaves no comment needing to explain an exception.

[haribo]

Delivered in #650, squashed on develop.

Option 1: JSON returns (string, bool, error), Render passes it up, printReports and the
status --json path decide. grep -n "os.Exit" internal/report/ now returns only the two
comments that state the rule — the package comment and the one on Render, which stated it as
the reason the package exists twelve lines above the line that broke it.

The unreachability argument is kept on the branch, where it explains why the error is
returned rather than handled. No test for it: JSON marshals a struct of strings, bools and
slices, so a test would have to fake the failure it claims to prove.

Existing tests were touched in arity only — eight call sites take the third return value, no
assertion changed.

[haribo]

Delivered in #650, squashed on develop.

Option 1: JSON returns (string, bool, error), Render passes it up, printReports and the
status --json path decide. grep -n "os.Exit" internal/report/ now returns only the two
comments that state the rule — the package comment and the one on Render, which stated it as
the reason the package exists twelve lines above the line that broke it.

The unreachability argument is kept on the branch, where it explains why the error is
returned rather than handled. No test for it: JSON marshals a struct of strings, bools and
slices, so a test would have to fake the failure it claims to prove.

Existing tests were touched in arity only — eight call sites take the third return value, no
assertion changed.