review.yml's tally comment must carry the UNKNOWN reason, not only the log

[chiibitsu]

Founder-ruled 2026-09-10: backlog, with a fixture. Measured on a real run, not reasoned about.

What happened

chiibitsu/nextjs-supabase-template#4, head efd275a, base deda1fa. The base had no REVIEW.md — the PR was the one adding it — so the policy step did exactly what it should:

UNKNOWN [review] no REVIEW.md at the base revision deda1faeb04b9a3d5f81a02316003b010a8297f4
— the reviewer has no policy to apply and will not run unguided
::error title=UNKNOWN (review)::…
exit 1

The reviewer step was therefore skipped, so .review-findings-count never appeared, so the tally step posted this as a comment on the pull request:

reviewed efd275a: UNKNOWN — the reviewer did not report a count (step outcome: skipped)

Both lines are true. Only the first says why.

Why it matters more than it looks

The tally comment exists because it is the one thing a reader uses to tell "reviewed, nothing found" from "did not review" — that is the sentence in review.yml's own header. It is deliberately the artefact that does not depend on the reviewer finishing, and it is deliberately posted where a reader will see it without opening Actions.

So the tally is doing the job of being read, and it is the piece that drops the reason. The ::error annotation with the real cause is in the job log; the comment carries the generic one. A reader who does not open the log learns that something went wrong and not what — and "the reviewer did not report a count" reads like the reviewer misbehaved, when in fact the reviewer never ran and the repository was misconfigured in a way the author can fix in one commit.

This is the defect this toolkit exists to catch, in the toolkit: a message narrower than the fact it reports. It does not get an exemption for being in the reviewer, the same way review.yml's checkout step did not get one for its ref:.

The distinct causes that collapse into one comment

Cause	steps.review.outcome	Count file	Comment today
No REVIEW.md at the base revision	skipped	absent	"did not report a count (step outcome: skipped)"
Reviewer step errored (continue-on-error: true)	failure	absent	"did not report a count (step outcome: failure)"
Reviewer ran, never wrote the file	success	absent	"did not report a count (step outcome: success)"
Reviewer wrote something unparseable	success	present, junk	"did not report a count (step outcome: success)"

The outcome word separates rows 1–3 and not rows 3 and 4, and none of the four says the thing the reader needs. Row 1 is the only one whose fix is obvious from the cause, and it is the one whose cause is furthest from the comment.

Proposed shape

Have every step that can produce an UNKNOWN write its reason to a file, and have the tally read it:

• policy step, on the missing-REVIEW.md branch: write the message to .review-unknown-reason before exit 1 (the tally runs under success() || failure(), so the file survives the failed step)
• tally step: when there is no usable count, append the reason if the file exists; when it does not exist, say so — no reason recorded is itself information, and is what row 3 and row 4 look like
• rows 3 and 4 must stop sharing a line: "wrote no count file" and "wrote an unparseable count file" are different failures and the second one means the reviewer thought it had answered

Same rm -f discipline as .review-findings-count: the reason file is deleted at the start of the policy step, before anything can read a value a pull request committed. A reason string chosen by the author of the diff is the same defect as a count chosen by them.

The fixture, which is the part that needs designing

review.yml is a workflow, and selftest.sh runs gates, not workflows — so today the tally logic has no way to be shown failing, which is why this shipped. That is the real finding here and it is one level up from the comment text.

The tally is ~20 lines of inline run: bash. Extracting it to gates/review-tally.sh, taking its four inputs as arguments or environment (head sha, review outcome, count file path, reason file path) and printing the line on stdout, makes it testable by the rule the rest of the toolkit already lives under. Then fixtures/review-tally/bad/cases/ holds one tree per row of the table above, and the selftest asserts the emitted line, not the exit status — the assertion that separates "right verdict, wrong evidence" from a pass, which is the class-3 gap already open in #2.

Rows worth planting, each shown to produce the wrong line before the fix and the right one after:

• no reason file, outcome skipped → today's generic line; must name the missing policy once the reason file exists
• reason file present, outcome skipped → must carry the reason verbatim
• outcome success, count file absent → must say the reviewer wrote no count, distinctly from the next row
• outcome success, count file present and unparseable → must say unparseable, and must not salvage digits (the tr -dc '0-9' defect is already fixed; a fixture keeps it fixed)
• count file present, 0 → clean tally, not UNKNOWN. A valid form in the table as a first-class entry, per Validation beyond the selftest: parser-level tests for the gate internals #2 — the bad-trees-only fixture model cannot otherwise catch a regression that reports UNKNOWN on a good run, and a reviewer that says UNKNOWN when it found nothing is the false red that trains people to ignore the lane

Related to #2, and it is the same argument arriving from a live run rather than from review.

Not in scope

Changing when the tally posts, or its success() || failure() guard. That guard is correct and was fixed in v1.2.1 for a measured reason — always() made every cancelled run leave a spurious UNKNOWN comment.