fix(review): fail-closed verdicts - an unsubstantiated finding can no longer yield ALLOW

computeVerdict counted only the findings handed to it (reasoned.kept), so a
finding removed by Pass 2 or Pass 3, and a reviewer whose payload was thrown
away or never arrived, were arithmetically identical to a finding that never
existed. All three pushed the verdict toward ALLOW.

Measured before the fix (probes, 2026-09-01):
  - a BLOCKER with a 101-character title -> ALLOW (Pass 2 succinctness)
  - a payload with one ad-hoc rule_key is skipped WHOLE at ingest, taking a
    valid BLOCKER sibling with it -> ALLOW
  - a reviewer that never reported -> ALLOW
Pass 3's own no-citation / unknown-rule_key branches turned out unreachable
through runContract (validateFindings rejects those payloads first), so the
reachable exposure was Pass 2 plus the skipped/absent reviewer.

THE OPEN DESIGN DECISION, and why it went against the order's default.
The order proposed: indeterminate file-existence YES, plain succinctness NO
("a too-long finding is not an uncertain finding"). I kept the first and
overrode the second, on one principle:

  A removal is `dropped` only when the test REFUTED the finding as a claim
  about this codebase. Every other removal is `unverified`.

Succinctness and actionability read a `.length`. They never examine the claim,
so they cannot establish the finding is unreal - and dropping a BLOCKER for a
101-character title is precisely the fail-open shape being fixed. Three things
settled it:

1. Under the order's default the fix would have been almost inert. Pass 3's
   drop branches are unreachable via runContract, so leaving Pass 2 out would
   have left the only reachable finding-level exposure open.
2. Cost asymmetry, priced rather than asserted: the verdict is not a gate.
   Handover 6 feeds `findings` filtered to BLOCKER+MAJOR into /trekplan
   (commands/trekplan.md:218); `verdict` is optional metadata
   (docs/HANDOVER-CONTRACTS.md:353). Nothing loops or re-plans on WARN. So a
   false `unverified` costs WARN plus a printed reason; a false drop costs a
   silent ALLOW over a live BLOCKER.
3. unknown-rule_key joins them for the same reason: an ad-hoc key is a real
   defect wearing the wrong label, and v5.1.1 high-effort mode already KEEPS
   those, normalised to PLAN_EXECUTE_DRIFT. Refuting them at normal effort
   while keeping them at high effort would be incoherent.

no-citation stays a drop: a finding whose file is empty or whose line is
negative names no location, so it makes no checkable claim at all - the one
deterministic refutation, and what the Pass 3 prose already said it was.

Iron Law: tests/lib/coordinator-contract.test.mjs first, red (missing export +
the three measured ALLOWs), then production code. Two existing assertions were
updated AFTER implementation as contract changes, not to make the red pass.
A known-positive control pins that ALLOW is still reachable - without it,
"no ALLOW" is not a fail-closed contract, only a broken one.

lib/review/coordinator-contract.mjs
  + classifySuppression / REFUTING_REASONS / UNVERIFIED_REASONS - one
    vocabulary owned by the lib, including the tokens only the LLM
    coordinator emits (accuracy:refuted, file-existence:refuted/indeterminate),
    so prose and lib cannot drift. Unclassified reasons default to unverified:
    the default fails closed.
  ~ judgeFilter / reasonablenessFilter return {kept, dropped, unverified}
  ~ computeVerdict(findings, {unverified, missingReviewers}) -> + allow_blocked_by.
    Never raises a verdict, only withholds ALLOW. Unverified findings are NOT
    counted into a severity tier: their severity was never substantiated, and
    counting it would be invention.
  ~ runContract(payloads, {expectedReviewers}) -> + unverified,
    missing_reviewers, allow_blocked_by. `suppressed` stays the union of
    dropped + unverified, so existing consumers (gold-eval) keep their meaning.

agents/review-coordinator.md - Pass 2/3 tables gain a fate column, new
  "Suppression is two-valued" section, Pass 4 threshold table gains the two
  fail-closed rows, Executive Summary must state a withheld ALLOW, Suppressed
  Findings tags each line [dropped]/[unverified]. Pass 3's unknown-rule_key
  bullet explicitly says high-effort does not reach that branch, so the same
  input never has two documented fates.

commands/trekreview.md - Phase 5 "Reviewer accounting": the expected set is
  written down before the spawn, a silent reviewer gets one re-ask and then
  STOP. That extends the pattern already in the file (schema failure -> 2
  bounded re-asks -> "do not feed unvalidated findings to the coordinator") to
  the other two ways a reviewer goes missing, rather than softening it to WARN.
  The lib's missing_reviewers stays as belt-and-braces for direct callers.

docs/agent-return-channel-defect.md - the "inferred, not observed" caveat on
  the unnamed arm above 66 lines is struck: akashic-intelligence S27
  (f168630) measured 2/2 unnamed agents returning against a 4370-line plan,
  30449 B and 10989 B, both valid JSON. Recorded with akashic's own two
  caveats intact - the measurer owns the finding, and byte-identity between
  the returned string and the file on disk was not proven. The separate S25
  named-arm figures are left standing; these are two measurements, not a
  correction of one by the other.

No release, no version bump, no tag, no catalogue ref, no Workflow port.
Suite 1025 (1023/0/2) -> 1034 (1032/0/2), 0 failures.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
Kjell Tore Guttormsen 2026-09-01 22:46:39 +02:00
commit e2aec019ac
6 changed files with 469 additions and 50 deletions

View file

@ -249,6 +249,37 @@ do not feed unvalidated findings to the coordinator.
In `quick` mode, launch only `code-correctness-reviewer`. The Executive
Summary will note the brief-conformance pass was skipped.
### Reviewer accounting — every expected reviewer MUST report
Write down the expected reviewer set BEFORE the spawn: both reviewers in
default mode, `code-correctness-reviewer` alone in `quick` mode. After the
spawn, account for each one by name.
**Zero findings from a silent reviewer is indistinguishable from zero findings
from a clean diff** — unless you check. A reviewer is *accounted for* only when
it returned a payload that validated. Three ways it fails to:
| Failure | Handling |
|---------|----------|
| Output fails the schema after the 2 bounded re-asks | STOP (already specified above) |
| Returned no final message at all | Re-ask that reviewer **once**. Still nothing → STOP. |
| Was never launched (spawn error, wrong mode) | STOP. |
**On STOP: name the reviewer and the failure, and do not proceed to Phase 6.**
Do not let the coordinator compute a verdict over a review one of whose
reviewers never spoke — the count would be complete-looking and wrong. This is
the same shape as the schema branch above ("do not feed unvalidated findings to
the coordinator"), applied to the other two ways a reviewer can go missing.
A reviewer that ran but never delivered is most often the return-channel
defect: check `~/.claude/projects/<proj>/<session>/subagents/agent-*.jsonl` for
its final assistant block before re-asking, and confirm no `name` parameter was
passed at the spawn (see the warning at the top of this phase).
If you proceed anyway under an explicit operator instruction, pass the expected
set to the coordinator as `expectedReviewers` so the missing reviewer at least
forbids `ALLOW` (`lib/review/coordinator-contract.mjs`, `missing_reviewers`).
## Phase 6 — Coordinator dedup + verdict
Launch `review-coordinator` (Agent tool) with the merged findings array
@ -259,10 +290,20 @@ The coordinator runs the 4-pass process documented in
1. **Dedup** by `(file, line, rule_key)` triplet.
2. **HubSpot Judge filters** — Succinctness, Accuracy, Actionability.
3. **Cloudflare reasonableness**drop speculative or catalogue-violating
3. **Cloudflare reasonableness**remove speculative or catalogue-violating
findings (skipped in `quick` mode).
4. **Verdict** — BLOCK / WARN / ALLOW per the threshold table.
**Fail-closed.** Every removal in Pass 2 and Pass 3 is either
**dropped** (the test refuted the finding as a claim about this codebase) or
**unverified** (the finding was removed without its claim ever being settled).
A non-empty `unverified` bucket forbids `ALLOW`; the verdict becomes `WARN` and
the Executive Summary's first sentence must say why. The fail-closed rule never
raises a verdict — it only withholds the clean one. Fate table, reason
vocabulary, and the `allow_blocked_by` field: `agents/review-coordinator.md`
§*Suppression is two-valued*, mirrored deterministically in
`lib/review/coordinator-contract.mjs`.
The coordinator's output is the full review.md content — frontmatter +
body sections + trailing JSON block. Do NOT re-run the reviewers based
on the coordinator's output.