fix(bench): reject non-review output and stop trailing prose becoming a phantom finding

[bborbe]

Fixes the two defects the first live end-to-end run of bench/run.py exposed on 2026-08-08. Both were invisible to the 42 unit tests, because every one of them stubs the claude binary.

Implements spec 003-bench-review-sanity-and-harvest-boundary across three prompts (015–017).

D2 — a broken invocation scored as a perfect clean review

The runner invoked /coding:pr-review, the subprocess printed 35 bytes to stdout — Unknown command: /coding:pr-review — and exited 0. The empty-diff guard didn't apply, the exit-code guard didn't fire, and the harvester found nothing, so the ledger recorded ok: 0 findings.

For a benchmark that is the worst available failure: a broken invocation looks like a flawless review, inflating precision and destroying recall, silently.

commands/pr-review.md Step 5 mandates all three section headings always present ("None." if empty), so output lacking them is not a review. It is now rejected before the cache write — no ledger row, no cache entry, the PR listed as failed, remaining PRs still run.

missing_sections("Unknown command: /coding:pr-review")
  -> ['Must Fix', 'Should Fix', 'Nice to Have']

D4 — sections had no terminator

A section ended only at the next heading. The --- thematic break and the trailing **Summary:** prose were appended to the still-open None. buffer, defeating the empty-section sentinel, and the accumulation was emitted as one finding with path/line/rule_id: null. It also corrupted non-empty sections: a real finding followed by trailing prose produced the real finding plus a phantom.

The known-clean fixture tts-mcp#20 — whose entire purpose is scoring zero — recorded 1 finding.

A section now ends at the next heading, a thematic break, or end of input; only a list item opens a finding.

harvest(real captured output) -> 0 findings      (was 1)

An earlier heading-level hypothesis was empirically disproven — the shipped parser already matched ^#{1,6}. Heading-level agnosticism is kept only as a regression lock.

Why the tests missed it

bench/testdata/sample-report.md was transcribed from the command's template and ends at the last section, so it never exercised a trailing-prose boundary. Fixture and parser were built from the same source: they agreed with each other and both disagreed with reality.

This PR adds bench/testdata/real-capture-report.md, a verbatim capture of live review output. Its acceptance criterion greps for fast-uri, mcp/package-lock.json and Step 3a: LICENSE Check — literals that appear nowhere in the template and cannot be produced by transcribing it.

Out of scope

Two further defects from the same run, deliberately excluded:

• D1 — the plugin-hash preflight resolves the marketplace/installLocation path, but Claude Code loads from plugins/cache/<name>/<name>/<version>/. Observed reporting MATCH across a 13-version gap. Critical; changes what the preflight resolves, so it needs its own spec.
• D3 — failure logs preserve stderr only; Claude Code writes its real errors to stdout.

Verification

• make precommit — PASS, 55 tests (was 42)
• Both fixes verified directly against the real captured output, not against the template

[ben-s-pull-request-reviewer]

Now I have all the information for a thorough adjudication. Let me compile the complete report.

Selector Mode Traceability:

• Candidates: 1 (changelog/conventional-prefix-required)
• Applicable: 1 (changelog/conventional-prefix-required)
• Skipped: all other judgment rules — their trigger globs (**/*.go, agents/**/*.md, commands/**/*.md, etc.) don't match any changed file

Step 4d-sel Adjudication:

1. changelog/conventional-prefix-required — CHANGELOG.md has fix: (×2) and docs: (×1) bullets under ## Unreleased — all in the allowed prefix set. No violation.

2. Regex patterns (HEADING_RE, THEMATIC_BREAK_RE, FENCE_RE, BULLET_RE) — all are linear-time: HEADING_RE (#{1,6}\s+(.+)) has no nested quantifiers; THEMATIC_BREAK_RE uses {3,} on a single character class (not nested); FENCE_RE has no quantifier on its alternation; BULLET_RE has \s{0,3} and .+ which are independent. No pathological-input risk.

3. flush_finding() sentinel (body.strip() in ("None.", "None")) — this is exact-equality on the entire normalized body, not substring containment. A legitimate finding whose body text happens to contain the word "None." (e.g. "the value is None.") normalizes to "the value is None." which does NOT equal "None." — correctly retained. The test test_trailing_prose_does_not_swallow_a_real_finding explicitly guards this with assertIn("None", f["body"]). No regression risk.

4. CLEAN_REVIEW_REPORT — used by all original 13 tests as a valid stub payload with all three sections present. The review_report() template generates heading-level-2 sections with "None." bodies. The new test test_review_shaped_output_at_either_heading_level_produces_a_row confirms h2 and h4 both pass. Valid.

5. real-capture-report.md fixture — verbatim capture of live output; the test asserts it harvests to exactly 0 findings. Correct.

6. Test suite — 13 original tests untouched; new tests added for sanity gate (6 tests) and harvest boundary (multiple). The plan says 55 tests; the new tests in the file I read total ~18 new test methods. Passes.

7. License — LICENSE file present at repo root. OK.

8. Mechanical funnel — 74 yamls run, 0 findings. Clean.

---

Step 7 Manual Review of bench/run.py key changes:

• missing_sections() and iter_report_lines() are correctly implemented — fenced blocks are skipped, severity suffixes stripped, canonical section names matched.
• The sanity gate at line 1061-1066 fires before any cache write and raises BenchError, leaving no ledger row or cache entry as designed.
• The harvest() rewrite correctly: opens a finding only on -/* bullets (line 837), ends a section at any heading or thematic break (lines 820, 827), appends continuation lines to the current finding buffer (line 841), and flushes at EOF.
• non_review_report() correctly bounds the excerpt to 2000 bytes and marks truncation.
• The atomic_write_bytes rename-from-temp pattern (lines 314-331) is correct.

{
  "verdict": "approve",
  "summary": "All changed code passes review. The harvest boundary rewrite is correct — thematic breaks and headings now properly end sections, bullets open findings, and the sentinel guards against phantom findings. The sanity gate rejects non-review output before any cache write. All regex patterns are linear-time. The 13 original tests are untouched and the new test suite covers both features. Changelog entries use correct conventional prefixes.",
  "comments": [],
  "concerns_addressed": [
    "correctness: harvest() section-boundary rewrite verified — thematic break ends section, only list items open findings, sentinel exact-equality confirmed by test",
    "correctness: sanity gate in process_pr() verified — fires before cache write, raises BenchError, leaves no ledger row or cache entry",
    "correctness: regex patterns HEADING_RE/THEMATIC_BREAK_RE/FENCE_RE/BULLET_RE are all linear-time, no nested quantifiers",
    "correctness: flush_finding() sentinel uses exact equality on normalized body — substring 'None.' in a legitimate finding body is not discarded (test confirms)",
    "correctness: CLEAN_REVIEW_REPORT is valid across all 6 original test cases (all 13 original tests untouched)",
    "tests: 13 original tests remain untouched; suite grows with new tests for sanity gate and harvest boundary",
    "correctness: real-capture-report.md fixture harvests to 0 findings (test confirms)"
  ]
}