diff --git a/CHANGELOG.md b/CHANGELOG.md index bfa3e52..f1f1c4b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,12 @@ Please choose versions by [Semantic Versioning](http://semver.org/). * MINOR version when you add functionality in a backwards-compatible manner, and * PATCH version when you make backwards-compatible bug fixes. +## Unreleased + +- fix: bench runner — add a sanity gate that rejects review output not structurally a review (missing any of Must Fix, Should Fix, Nice to Have headings) before the raw output cache write; rejected output produces no ledger row, no cache entry, the PR is listed as failed, and remaining PRs still run +- fix: bench runner — fix findings section boundary: a section now ends at the next heading, a thematic break, or end of input, and only a list item (`-` or `*`) opens a finding, so trailing summary prose in a real review can no longer be appended to the `None.` sentinel and emitted as a phantom finding +- docs: bench — document the harvest contract in `bench/README.md` (what ends a section, what opens a finding, the three mandatory sections, the fixture table); add verbatim-capture fixture `bench/testdata/real-capture-report.md` that locks the boundary fix + ## v0.35.1 - bench: add `make bench` and `make bench-test` Makefile targets; wire `bench-test` into `make precommit` so bench unit tests gate every later change diff --git a/bench/README.md b/bench/README.md index 89aaf52..c316415 100644 --- a/bench/README.md +++ b/bench/README.md @@ -49,6 +49,39 @@ The manifest's recorded `base_sha..head_sha` is the single authoritative source The runner aborts loudly on an empty diff (`EMPTY DIFF`) — a resolved range with zero changed files is never recorded as a zero-finding review and produces no row and no cache entry. +## Reading review output + +### The three required sections + +`commands/pr-review.md` Step 5 marks **Must Fix**, **Should Fix** and **Nice to Have** as mandatory sections and mandates the literal `None.` when a section has no findings. A report is a review only when all three appear as markdown headings; output missing any of them is **rejected before the raw output is cached** and before it is harvested. A rejected PR leaves no ledger row and no cache entry, the remaining PRs still run, and the process exits non-zero — the same treatment an `EMPTY DIFF` gets, for the same reason. + +The rejection names the PR, names each missing section on its own `missing sections: ` line, and carries a bounded verbatim excerpt on stderr. + +> **Why the gate is necessary.** A subprocess that exits 0 after printing `Unknown command: /coding:pr-review` is otherwise indistinguishable from a genuinely clean review. A fabricated clean row is byte-for-byte identical to a real one. + +### What ends a findings section + +A findings section's content ends at the **next markdown heading of any level**, at a **thematic break** (`---`, `***` or `___` on its own line), or at **end of input** — whichever comes first. Section names are matched as headings at any level; a mention in prose, in a bold run, or inside a fenced code block is not a heading. + +> **Why heading level carries no information.** The command's template renders sections at one level and captured live output rendered them at another, so level is not evidence of anything. + +### What opens a finding + +Inside a findings section, a finding starts when a **list item** begins (`-` or `*`). Subsequent non-list lines extend the finding already open. Prose appearing in a section **before** any list item — most importantly the mandated `None.` sentinel — contributes no finding and cannot be extended by anything that follows. + +> **Why the sentinel cannot be extended.** Real review output carries a diff summary and a closing status panel after the last section. Before the boundary rules existed, those lines were appended as continuation lines to the still-open `None.` buffer, defeating the sentinel check and emitting the accumulated text as one finding with no path, line or rule id. + +All three section names — **Must Fix**, **Should Fix**, **Nice to Have** — are mandatory; the gate accepts a report as a review only when all three appear as headings. + +### Fixtures + +| Fixture | Origin | Harvests to | +|---|---|---| +| `bench/testdata/sample-report.md` | derived from the review command's Step 5 template, `####` headings | 3 findings | +| `bench/testdata/real-capture-report.md` | verbatim capture of live review output, `##` headings, all three sections `None.`, trailing prose | 0 findings | + +Both defects this section documents survived 42 green unit tests because the tests were built from the same template the parser was built from. A fixture for a new defect must be a **capture of real output**, not a transcription of the template. + ## Verifying an entry without cloning ```bash @@ -65,6 +98,9 @@ These are deliberately not configurable: - **Cache:** lives under `bench/.cache/` (gitignored — no benchmark output is ever committed) - **Results:** live under `bench/results/` (gitignored) - **Isolated config:** `$HOME/.claude-verify` with `DISABLE_AUTOUPDATER=1`; the runner aborts the whole run before the first review if that directory would resolve the `coding` plugin to content whose hash differs from `--coding-repo`'s +- **Required section names:** `Must Fix`, `Should Fix`, `Nice to Have` — all three mandatory in every review report; output missing any one is rejected before cache write +- **List-item markers:** only `-` and `*` open a finding; prose before the first list item cannot form a finding +- **Stderr excerpt bound:** at most 2,000 bytes of rejected output are printed to stderr (truncation is marked) ## Safety invariant diff --git a/bench/run.py b/bench/run.py index beb68a3..79f9615 100755 --- a/bench/run.py +++ b/bench/run.py @@ -43,11 +43,22 @@ VALID_MODES = ("short", "full", "selector") NAME_RE = re.compile(r"^[A-Za-z0-9][A-Za-z0-9._-]*$") PR_ID_RE = re.compile(r"^[A-Za-z0-9][A-Za-z0-9._-]*#[0-9]+$") +REQUIRED_SECTION_NAMES = ("Must Fix", "Should Fix", "Nice to Have") +HEADING_RE = re.compile(r"^#{1,6}\s+(.+)$") +SEVERITY_SUFFIX_RE = re.compile(r"\s*\([^)]+\)\s*$") +THEMATIC_BREAK_RE = re.compile(r"^ {0,3}(?:-{3,}|\*{3,}|_{3,}) *$") +FENCE_RE = re.compile(r"^ {0,3}(?:```|~~~)") +BULLET_RE = re.compile(r"^\s{0,3}([-*])\s+(.+)$") +_SECTION_BY_LOWER = {name.lower(): name for name in REQUIRED_SECTION_NAMES} REQUIRED_ENTRY_FIELDS = ( "id", "owner", "repo", "number", "merge_strategy", "merge_sha", "base_sha", "head_sha", "changed_files", ) +# Gate constants (frozen invariants — not configurable) +NON_REVIEW_MARKER = "NOT A REVIEW" +REJECTION_EXCERPT_BYTES = 2000 + # ---------------------------------------------------------------------- # Exceptions # ---------------------------------------------------------------------- @@ -690,6 +701,80 @@ def _extract_path_line(text: str, known_rule_ids: set) -> tuple[str | None, int return None, None +def heading_section_name(line: str) -> str | None: + """Return the canonical findings-section name a markdown heading line names, or None. + + Matches a heading at any level 1-6, strips a trailing parenthesised severity + annotation such as "(Critical)", and compares case-insensitively against + REQUIRED_SECTION_NAMES. Returns the canonical spelling ("Must Fix", + "Should Fix", "Nice to Have") or None when the line is not a heading or names + something else. + """ + m = HEADING_RE.match(line) + if not m: + return None + stripped = m.group(1).strip() + stripped = SEVERITY_SUFFIX_RE.sub("", stripped).strip() + return _SECTION_BY_LOWER.get(stripped.lower()) + + +def iter_report_lines(report_text: str): + """Yield (line, in_fence) for every line of report_text. + + in_fence is True for the fence delimiter lines themselves and for every line + between an opening and a closing fence. A line inside a fence is never a + heading, never a thematic break and never a bullet; it is ordinary text. + """ + in_fence = False + for line in report_text.splitlines(): + if FENCE_RE.match(line): + in_fence = not in_fence + yield (line, True) + else: + yield (line, in_fence) + + +def missing_sections(report_text: str) -> list[str]: + """Return the required findings-section names absent from report_text, in canonical order. + + A section counts as present only when it appears as a markdown heading at any + level 1-6 outside a fenced code block. The words appearing in prose, in a + bold run, or inside a fence do not count. Returns [] when all three are present. + """ + present: set[str] = set() + for line, in_fence in iter_report_lines(report_text): + if in_fence: + continue + name = heading_section_name(line) + if name is not None: + present.add(name) + return [name for name in REQUIRED_SECTION_NAMES if name not in present] + + +def rejection_excerpt(text: str, limit: int = REJECTION_EXCERPT_BYTES) -> str: + """Return at most limit bytes of text's UTF-8 prefix, marked when truncated.""" + encoded = text.encode("utf-8") + total = len(encoded) + if total <= limit: + return text + prefix = encoded[:limit].decode("utf-8", errors="ignore") + return f"{prefix}\n[... truncated, {total} bytes total]" + + +def non_review_report(pr_id: str, missing: list[str], stdout_text: str) -> str: + """Build the multi-line stderr diagnosis for output rejected as a non-review.""" + total = len(stdout_text.encode("utf-8")) + excerpt = rejection_excerpt(stdout_text) + return ( + f"{NON_REVIEW_MARKER}: {pr_id}\n" + f"missing sections: {', '.join(missing)}\n" + f"no ledger row and no cache entry were written; this PR is retried on the next run\n" + f"--- rejected output excerpt ({total} bytes total) ---\n" + f"{excerpt}\n" + f"--- end excerpt ---" + ) + + def _normalize_body(lines: list[str]) -> str: """Strip bullet marker and join continuation lines into one whitespace-collapsed string.""" body = lines[0] @@ -708,7 +793,6 @@ def harvest(report_text: str, known_rule_ids: set) -> list: findings: list = [] current_section: str | None = None current_finding_lines: list[str] = [] - section_names = {"must fix", "should fix", "nice to have"} def flush_finding(): nonlocal current_finding_lines, current_section, findings @@ -716,7 +800,7 @@ def flush_finding(): return text = " ".join(current_finding_lines) body = _normalize_body(current_finding_lines) - # Skip the "None." empty-section sentinel + # Skip the "None." empty-section sentinel (exact equality only) if body.strip() in ("None.", "None"): current_finding_lines = [] return @@ -730,37 +814,31 @@ def flush_finding(): }) current_finding_lines = [] - lines = report_text.splitlines() - i = 0 - while i < len(lines): - line = lines[i] - heading_match = re.match(r"^#{1,6}\s+(.+)$", line) - if heading_match: - heading_text = heading_match.group(1).strip() - # Strip trailing severity annotation like (Critical) - heading_text = re.sub(r"\s*\([^)]+\)\s*$", "", heading_text).strip() - heading_lower = heading_text.lower() - if heading_lower in section_names: + for line, in_fence in iter_report_lines(report_text): + if not in_fence: + name = heading_section_name(line) + if name is not None or HEADING_RE.match(line): + # Any heading — findings or not — ends whatever section was open flush_finding() - current_section = heading_lower + current_section = name # None for non-findings headings current_finding_lines = [] - else: - # Any other heading (including traceability) ends the current section + continue + + if THEMATIC_BREAK_RE.match(line): flush_finding() current_section = None current_finding_lines = [] - i += 1 + continue + + if current_section is None: continue - if current_section is not None: - stripped = line.strip() - bullet_match = re.match(r"^\s{0,3}([-*])\s+(.+)$", stripped) - if bullet_match: - flush_finding() - current_finding_lines = [bullet_match.group(2)] - elif stripped: - current_finding_lines.append(stripped) - i += 1 + stripped = line.strip() + if stripped and BULLET_RE.match(stripped): + flush_finding() + current_finding_lines = [BULLET_RE.match(stripped).group(2)] + elif stripped and current_finding_lines: + current_finding_lines.append(stripped) flush_finding() return findings @@ -979,6 +1057,14 @@ def process_pr(*, entry: dict, coding_repo: pathlib.Path, failure_log.write_bytes(proc.stderr.encode("utf-8") if proc.stderr else b"") raise BenchError(f"{pr_id}: review invocation failed: exit {proc.returncode}") + # 4. Sanity gate — reject non-review output before anything is written + missing = missing_sections(proc.stdout) + if missing: + print(non_review_report(pr_id, missing, proc.stdout), file=sys.stderr) + raise BenchError( + f"{NON_REVIEW_MARKER}: {pr_id}: missing sections: {', '.join(missing)}" + ) + duration_seconds = time.monotonic() - t0 # 5. Write raw stdout verbatim before any parsing diff --git a/bench/test_review.py b/bench/test_review.py index 2de4466..58d0b8b 100644 --- a/bench/test_review.py +++ b/bench/test_review.py @@ -1,6 +1,8 @@ #!/usr/bin/env python3 """Unit tests for bench/run.py review invocation, caching, harvesting, and ledger.""" +import contextlib +import io import json import os import pathlib @@ -15,6 +17,68 @@ import testsupport +# ---------------------------------------------------------------------- +# Shared test harness helpers +# ---------------------------------------------------------------------- +def run_one_pr_with_payload(td: pathlib.Path, payload: str) -> tuple[int, str, pathlib.Path, pathlib.Path]: + """Run bench over a one-PR temp manifest with the given stub payload. + + Returns (returncode, captured_stderr, results_dir, cache_root). + The stub claude is installed on PATH before the call. + """ + td = pathlib.Path(td) + cache_root = td / "cache" + results_dir = td / "results" + results_dir.mkdir(parents=True) + bin_dir = td / "bin" + counter = td / "counter" + stub = testsupport.stub_claude(bin_dir, counter, payload) + env = testsupport.with_path(bin_dir) + env["HOME"] = str(td) + + # Seed one merge repo + repos_root = cache_root / "repos" + repos_root.mkdir(parents=True) + repo_a = repos_root / "testowner" / "repo_a" + repo_a.mkdir(parents=True) + info_a = testsupport.make_merge_repo(repo_a) + + manifest_entries = [ + { + "id": "test#1", + "owner": "testowner", + "repo": "repo_a", + "number": 1, + "merge_strategy": "merge-commit", + "merge_sha": info_a["merge_sha"], + "base_sha": info_a["base_sha"], + "head_sha": info_a["head_sha"], + "changed_files": 1, + }, + ] + manifest_path = td / "manifest.json" + testsupport.make_manifest(manifest_path, manifest_entries) + + plugin_src = testsupport.build_coding_repo(td / "repo") + cfg = testsupport.build_verify_config_dir(td / "cfg", plugin_src, + use_known_marketplaces=True) + + captured_stderr = io.StringIO() + with contextlib.redirect_stderr(captured_stderr): + with mock.patch.dict(os.environ, env): + rc = run.run_bench( + coding_repo=plugin_src, + manifest_path=manifest_path, + results_dir=results_dir, + cache_root=cache_root, + model="test-model", + effort="high", + mode="short", + config_dir=cfg, + ) + return rc, captured_stderr.getvalue(), results_dir, cache_root + + class TestSecondRunIsCacheHit(unittest.TestCase): """AC4: a second invocation of the same configuration invokes zero reviews.""" @@ -26,7 +90,7 @@ def test_second_run_is_cache_hit_and_invokes_zero_reviews(self): results_dir.mkdir(parents=True) bin_dir = td / "bin" counter = td / "counter" - stub = testsupport.stub_claude(bin_dir, counter, "findings: []") + stub = testsupport.stub_claude(bin_dir, counter, testsupport.CLEAN_REVIEW_REPORT) env = testsupport.with_path(bin_dir) env["HOME"] = str(td) @@ -129,7 +193,7 @@ def test_mode_change_is_cache_miss(self): results_dir.mkdir(parents=True) bin_dir = td / "bin" counter = td / "counter" - stub = testsupport.stub_claude(bin_dir, counter, "findings: []") + stub = testsupport.stub_claude(bin_dir, counter, testsupport.CLEAN_REVIEW_REPORT) env = testsupport.with_path(bin_dir) env["HOME"] = str(td) @@ -397,7 +461,7 @@ def test_row_carries_every_required_field(self): results_dir.mkdir(parents=True) bin_dir = td / "bin" counter = td / "counter" - stub = testsupport.stub_claude(bin_dir, counter, "findings: []") + stub = testsupport.stub_claude(bin_dir, counter, testsupport.CLEAN_REVIEW_REPORT) env = testsupport.with_path(bin_dir) env["HOME"] = str(td) @@ -472,7 +536,9 @@ def test_raw_output_is_cached_verbatim(self): results_dir.mkdir(parents=True) bin_dir = td / "bin" counter = td / "counter" - report_text = "findings: [{\"rule_id\":\"foo/bar\",\"path\":\"x.go\",\"line\":1}]" + report_text = testsupport.review_report( + must_fix="- `agent-cmd/command-thin`: sample finding at `agents/x.md:12`." + ) stub = testsupport.stub_claude(bin_dir, counter, report_text) env = testsupport.with_path(bin_dir) env["HOME"] = str(td) @@ -625,7 +691,7 @@ def test_failed_pr_does_not_prevent_later_prs(self): results_dir.mkdir(parents=True) bin_dir = td / "bin" counter = td / "counter" - stub = testsupport.stub_claude(bin_dir, counter, "findings: []") + stub = testsupport.stub_claude(bin_dir, counter, testsupport.CLEAN_REVIEW_REPORT) env = testsupport.with_path(bin_dir) env["HOME"] = str(td) @@ -698,7 +764,7 @@ def test_corrupt_cache_row_is_treated_as_miss(self): results_dir.mkdir(parents=True) bin_dir = td / "bin" counter = td / "counter" - stub = testsupport.stub_claude(bin_dir, counter, "findings: []") + stub = testsupport.stub_claude(bin_dir, counter, testsupport.CLEAN_REVIEW_REPORT) env = testsupport.with_path(bin_dir) env["HOME"] = str(td) @@ -762,5 +828,380 @@ def test_corrupt_cache_row_is_treated_as_miss(self): self.assertIn("pr_id", row_data) +class TestRealCaptureHarvestsToZeroFindings(unittest.TestCase): + """AC8: the verbatim-capture fixture harvests to zero findings.""" + + def test_real_capture_harvests_to_zero_findings(self): + text = (run.BENCH_DIR / "testdata" / "real-capture-report.md").read_text() + ids = run.load_rule_ids(run.REPO_ROOT) + findings = run.harvest(text, ids) + self.assertEqual(findings, [], f"real capture must harvest to zero findings, got: {findings}") + + +class TestTrailingProseDoesNotSwallowARealFinding(unittest.TestCase): + """AC9: a real finding's body is not corrupted by trailing prose.""" + + def test_trailing_prose_does_not_swallow_a_real_finding(self): + known_ids = run.load_rule_ids(run.REPO_ROOT) + # Pick a rule that actually exists in the index + real_rule_id = next((rid for rid in known_ids if "/" in rid), None) + self.assertIsNotNone(real_rule_id, "rules/index.json must contain at least one rule with a slash") + + report = ( + f"## Must Fix (Critical)\n" + f"- `{real_rule_id}`: passing `None` as the default here hides the missing-value case in src/foo.go:12\n" + f"## Should Fix (Important)\n" + f"None.\n" + f"## Nice to Have (Optional)\n" + f"None.\n" + f"---\n" + f"**Summary:** This is the closing panel prose.\n" + f"Some additional context about what was reviewed.\n" + ) + findings = run.harvest(report, known_ids) + self.assertEqual(len(findings), 1, f"expected exactly 1 finding, got: {findings}") + f = findings[0] + self.assertEqual(f["path"], "src/foo.go") + self.assertEqual(f["line"], 12) + self.assertIn("None", f["body"], "body must contain the word None (exact equality sentinel regression guard)") + self.assertNotIn("Summary", f["body"], "body must not contain trailing prose") + self.assertNotIn("closing panel", f["body"], "body must not contain trailing prose") + + +class TestHeadingLevelDoesNotChangeHarvest(unittest.TestCase): + """AC10: heading level is irrelevant to harvesting.""" + + def test_heading_level_does_not_change_harvest(self): + known_ids = run.load_rule_ids(run.REPO_ROOT) + real_rule_id = next((rid for rid in known_ids if "/" in rid), None) + self.assertIsNotNone(real_rule_id) + + report_template = ( + lambda prefix: ( + f"{prefix} Must Fix (Critical)\n" + f"- `{real_rule_id}`: a finding in file.go:5\n" + f"{prefix} Should Fix (Important)\n" + f"None.\n" + f"{prefix} Nice to Have (Optional)\n" + f"None.\n" + ) + ) + + reports = {level: report_template(level) for level in ("##", "###", "####")} + harvests = {level: run.harvest(text, known_ids) for level, text in reports.items()} + + self.assertEqual( + harvests["##"], + harvests["###"], + f"## vs ###: {harvests['##']} vs {harvests['###']}", + ) + self.assertEqual( + harvests["##"], + harvests["####"], + f"## vs ####: {harvests['##']} vs {harvests['####']}", + ) + + +class TestSectionNameInProseOrFenceIsNotAHeading(unittest.TestCase): + """Section names in prose or inside a fenced block do not open a section.""" + + def test_heading_section_name_rejects_prose_and_fence(self): + # Prose mentions are not headings + self.assertIsNone(run.heading_section_name("**Must Fix**")) + self.assertIsNone(run.heading_section_name("We looked at Must Fix items.")) + self.assertIsNone(run.heading_section_name("must fix:")) + # Real headings at various levels + self.assertEqual(run.heading_section_name("## Must Fix (Critical)"), "Must Fix") + self.assertEqual(run.heading_section_name("###### nice to have"), "Nice to Have") + self.assertEqual(run.heading_section_name("### Should Fix (Important)"), "Should Fix") + + def test_fence_contains_heading_not_a_section(self): + known_ids = run.load_rule_ids(run.REPO_ROOT) + report = "```\n## Must Fix (Critical)\n- a finding\n```\n" + findings = run.harvest(report, known_ids) + self.assertEqual(findings, [], f"fenced heading must not open a section, got: {findings}") + + +class TestThematicBreakEndsASection(unittest.TestCase): + """A thematic break immediately after a finding ends the section.""" + + def test_thematic_break_ends_a_section(self): + known_ids = run.load_rule_ids(run.REPO_ROOT) + real_rule_id = next((rid for rid in known_ids if "/" in rid), None) + self.assertIsNotNone(real_rule_id) + + report = ( + f"## Nice to Have (Optional)\n" + f"- `{real_rule_id}`: a real finding in file.go:99\n" + f"---\n" + f"**Summary:** This is trailing prose that must not be appended to the finding.\n" + f"Another paragraph of closing remarks.\n" + ) + findings = run.harvest(report, known_ids) + self.assertEqual(len(findings), 1, f"expected 1 finding, got: {findings}") + self.assertNotIn("Summary", findings[0]["body"]) + self.assertNotIn("trailing prose", findings[0]["body"]) + self.assertNotIn("closing remarks", findings[0]["body"]) + + +class TestProseBeforeAListItemOpensNothing(unittest.TestCase): + """Prose in a section before any list item buffers nothing.""" + + def test_prose_before_a_list_item_opens_nothing(self): + known_ids = run.load_rule_ids(run.REPO_ROOT) + real_rule_id = next((rid for rid in known_ids if "/" in rid), None) + self.assertIsNotNone(real_rule_id) + + report = ( + f"## Must Fix (Critical)\n" + f"None.\n" + f"\n" + f"- `{real_rule_id}`: the actual finding in bar.go:7\n" + f"## Should Fix (Important)\n" + f"None.\n" + f"## Nice to Have (Optional)\n" + f"None.\n" + ) + findings = run.harvest(report, known_ids) + self.assertEqual(len(findings), 1, f"expected 1 finding, got: {findings}") + self.assertNotIn("None.", findings[0]["body"]) + self.assertEqual(findings[0]["path"], "bar.go") + self.assertEqual(findings[0]["line"], 7) + + +# ---------------------------------------------------------------------- +# New tests for the non-review sanity gate +# ---------------------------------------------------------------------- +class TestNonReviewOutputIsRejected(unittest.TestCase): + """AC2: output that is not review-shaped is rejected.""" + + def test_non_review_output_is_rejected(self): + with tempfile.TemporaryDirectory() as td: + td = pathlib.Path(td) + rc, stderr, results_dir, cache_root = run_one_pr_with_payload( + td, "Unknown command: /coding:pr-review" + ) + + self.assertEqual(rc, 1, "run must exit 1 for non-review") + self.assertIn(run.NON_REVIEW_MARKER, stderr) + self.assertIn("test#1", stderr) + self.assertIn("Unknown command:", stderr) + + # No ledger row + ledger = run.ledger_path(results_dir) + if ledger.exists(): + rows = [json.loads(ln) for ln in ledger.read_text().splitlines()] + else: + rows = [] + self.assertEqual(len(rows), 0, "no ledger row for rejected review") + + # No cache entry + reviews = run.reviews_root(cache_root) + if reviews.exists(): + files = list(reviews.glob("*.json")) + list(reviews.glob("*.stdout.txt")) + else: + files = [] + self.assertEqual(len(files), 0, "no cache files for rejected review") + + +class TestSectionNamesOutsideHeadingsDoNotSatisfyTheGate(unittest.TestCase): + """AC3: bare section literals in prose/fence/bold do not satisfy the gate.""" + + def test_section_names_outside_headings_do_not_satisfy_the_gate(self): + payload = ( + "We looked at Must Fix items.\n\n" + "```\n" + "## Should Fix (Important)\n" + "```\n\n" + "**Nice to Have**\n" + ) + with tempfile.TemporaryDirectory() as td: + td = pathlib.Path(td) + rc, stderr, results_dir, cache_root = run_one_pr_with_payload(td, payload) + + self.assertEqual(rc, 1) + self.assertIn(run.NON_REVIEW_MARKER, stderr) + + ledger = run.ledger_path(results_dir) + if ledger.exists(): + rows = [json.loads(ln) for ln in ledger.read_text().splitlines()] + else: + rows = [] + self.assertEqual(len(rows), 0) + + reviews = run.reviews_root(cache_root) + if reviews.exists(): + files = list(reviews.glob("*.json")) + list(reviews.glob("*.stdout.txt")) + else: + files = [] + self.assertEqual(len(files), 0) + + +class TestMissingSectionNamesAreReportedExactly(unittest.TestCase): + """AC4: the missing-sections diagnosis names only the absent sections.""" + + def test_missing_section_names_are_reported_exactly(self): + # Case A: Nice to Have absent + with tempfile.TemporaryDirectory() as td: + td = pathlib.Path(td) + rc, stderr, results_dir, cache_root = run_one_pr_with_payload( + td, testsupport.review_report(nice_to_have=None) + ) + self.assertEqual(rc, 1) + + missing_line = next( + (l for l in stderr.splitlines() if l.startswith("missing sections: ")), + "", + ) + remainder = missing_line[len("missing sections: "):] + self.assertEqual(remainder, "Nice to Have", + "Case A: only Nice to Have missing") + self.assertNotIn("Must Fix", remainder) + self.assertNotIn("Should Fix", remainder) + + ledger = run.ledger_path(results_dir) + rows = [] if not ledger.exists() else [ + json.loads(ln) for ln in ledger.read_text().splitlines() + ] + self.assertEqual(len(rows), 0) + + # Case B: Should Fix and Nice to Have absent + with tempfile.TemporaryDirectory() as td: + td = pathlib.Path(td) + rc, stderr, results_dir, cache_root = run_one_pr_with_payload( + td, testsupport.review_report(should_fix=None, nice_to_have=None) + ) + self.assertEqual(rc, 1) + + missing_line = next( + (l for l in stderr.splitlines() if l.startswith("missing sections: ")), + "", + ) + remainder = missing_line[len("missing sections: "):] + self.assertEqual(remainder, "Should Fix, Nice to Have", + "Case B: Should Fix and Nice to Have missing in that order") + self.assertNotIn("Must Fix", remainder) + + ledger = run.ledger_path(results_dir) + rows = [] if not ledger.exists() else [ + json.loads(ln) for ln in ledger.read_text().splitlines() + ] + self.assertEqual(len(rows), 0) + + +class TestRejectionExcerptIsBounded(unittest.TestCase): + """AC5: rejection excerpt is bounded and carries a truncation marker.""" + + def test_rejection_excerpt_is_bounded(self): + result = run.non_review_report("test#1", ["Must Fix"], "x" * 100_000) + self.assertLess(len(result.encode("utf-8")), 8192, + "rejection diagnosis must be under 8 kB") + self.assertIn("[... truncated,", result) + self.assertIn("100000", result) + + +class TestReviewShapedOutputAtEitherHeadingLevelProducesARow(unittest.TestCase): + """AC6: review-shaped output at h2 and h4 both produce a ledger row.""" + + def test_review_shaped_output_at_either_heading_level_produces_a_row(self): + for level in (2, 4): + with tempfile.TemporaryDirectory() as td: + td = pathlib.Path(td) + rc, stderr, results_dir, cache_root = run_one_pr_with_payload( + td, testsupport.review_report(heading_level=level) + ) + + self.assertEqual(rc, 0, + f"heading_level={level}: run must succeed") + + ledger = run.ledger_path(results_dir) + self.assertTrue(ledger.exists()) + rows = [json.loads(ln) for ln in ledger.read_text().splitlines()] + self.assertEqual(len(rows), 1, + f"heading_level={level}: exactly 1 row expected") + self.assertEqual(rows[0]["pr_id"], "test#1", + f"heading_level={level}: row must carry correct pr_id") + + +class TestGateDoesNotApplyToACacheHit(unittest.TestCase): + """The sanity gate is not applied to previously cached output.""" + + def test_gate_does_not_apply_to_a_cache_hit(self): + with tempfile.TemporaryDirectory() as td: + td = pathlib.Path(td) + cache_root = td / "cache" + results_dir1 = td / "results1" + results_dir1.mkdir(parents=True) + results_dir2 = td / "results2" + results_dir2.mkdir(parents=True) + bin_dir = td / "bin" + counter = td / "counter" + + # First run: review-shaped payload, produces a row and cache entry + stub = testsupport.stub_claude(bin_dir, counter, testsupport.CLEAN_REVIEW_REPORT) + env = testsupport.with_path(bin_dir) + env["HOME"] = str(td) + + repos_root = cache_root / "repos" + repos_root.mkdir(parents=True) + repo_a = repos_root / "testowner" / "repo_a" + repo_a.mkdir(parents=True) + info_a = testsupport.make_merge_repo(repo_a) + + manifest_entries = [ + { + "id": "test#1", + "owner": "testowner", + "repo": "repo_a", + "number": 1, + "merge_strategy": "merge-commit", + "merge_sha": info_a["merge_sha"], + "base_sha": info_a["base_sha"], + "head_sha": info_a["head_sha"], + "changed_files": 1, + }, + ] + manifest_path = td / "manifest.json" + testsupport.make_manifest(manifest_path, manifest_entries) + + plugin_src = testsupport.build_coding_repo(td / "repo") + cfg = testsupport.build_verify_config_dir(td / "cfg", plugin_src, + use_known_marketplaces=True) + + with mock.patch.dict(os.environ, env): + rc1 = run.run_bench( + coding_repo=plugin_src, + manifest_path=manifest_path, + results_dir=results_dir1, + cache_root=cache_root, + model="test-model", + effort="high", + mode="short", + config_dir=cfg, + ) + + self.assertEqual(rc1, 0, "first run must succeed") + + # Second run: same cache, but stub replaced with non-review payload. + # Must be a cache hit — gate must NOT be applied. + bad_counter = td / "bad_counter" + stub2 = testsupport.stub_claude(bin_dir, bad_counter, "Unknown command: /coding:pr-review") + captured_stderr = io.StringIO() + with contextlib.redirect_stderr(captured_stderr): + with mock.patch.dict(os.environ, env): + rc2 = run.run_bench( + coding_repo=plugin_src, + manifest_path=manifest_path, + results_dir=results_dir2, + cache_root=cache_root, + model="test-model", + effort="high", + mode="short", + config_dir=cfg, + ) + + self.assertEqual(rc2, 0, "second run must be a cache hit (gate not applied)") + + if __name__ == "__main__": unittest.main() diff --git a/bench/testdata/real-capture-report.md b/bench/testdata/real-capture-report.md new file mode 100644 index 0000000..0b66e35 --- /dev/null +++ b/bench/testdata/real-capture-report.md @@ -0,0 +1,37 @@ +The diff is only `mcp/package-lock.json` updating `fast-uri` from `3.1.4` -> `3.1.5`. This is a vendored lockfile change - the `node_modules/fast-uri/` path segment means it doesn't count toward rule-relevant files. + +--- + +## PR Review - bench-pr-20 -> bench-base-20 + +**Diff size:** 1 file, 3 insertions, 3 deletions + +**Changed file:** +- `mcp/package-lock.json` - `fast-uri` 3.1.4 -> 3.1.5 (node_modules vendored dependency) + +--- + +### Step 3a: LICENSE Check +LICENSE file present in repo root. + +### Step 4: Automated Checks +**Skipped.** The diff is `mcp/package-lock.json`, which contains only vendored node_modules paths. + +--- + +## Must Fix (Critical) +None. + +## Should Fix (Important) +None. + +## Nice to Have (Optional) +None. + +--- + +**Summary:** This PR bumps `fast-uri` in the MCP's vendored lockfile. No source code changed. No review findings. + +--- + +The review completed successfully. All automated checks passed. diff --git a/bench/testsupport.py b/bench/testsupport.py index 2571d90..67e4367 100755 --- a/bench/testsupport.py +++ b/bench/testsupport.py @@ -97,6 +97,36 @@ def stub_claude(bin_dir: pathlib.Path, counter_file: pathlib.Path, return make_stub_bin(bin_dir, "claude", body) +def review_report(*, must_fix: str | None = "None.", should_fix: str | None = "None.", + nice_to_have: str | None = "None.", heading_level: int = 2, + preamble: str = "", trailing: str = "") -> str: + """Build a review-shaped stub payload. + + Renders the three mandatory sections at heading_level, in the order Must Fix, + Should Fix, Nice to Have, each carrying the given body. Passing None for a + section omits that section entirely, which is how a non-review payload is + built. preamble is emitted before the first section and trailing after the + last one, both verbatim and both empty by default. + """ + hashes = "#" * heading_level + parts = [preamble] + for name, body in [ + ("Must Fix", must_fix), + ("Should Fix", should_fix), + ("Nice to Have", nice_to_have), + ]: + if body is not None: + annotation = "(Critical)" if name == "Must Fix" else "(Important)" if name == "Should Fix" else "(Optional)" + parts.append(f"{hashes} {name} {annotation}") + parts.append(body) + parts.append(trailing) + return "\n".join(parts) + + +# Module-level clean review for common use +CLEAN_REVIEW_REPORT = review_report() + + def stub_claude_failing(bin_dir: pathlib.Path, counter_file: pathlib.Path, exit_code: int = 3) -> pathlib.Path: """Install a stub `claude` that appends args to counter_file and exits with exit_code.""" diff --git a/prompts/completed/015-spec-003-harvest-section-boundaries.md b/prompts/completed/015-spec-003-harvest-section-boundaries.md new file mode 100644 index 0000000..26df8c4 --- /dev/null +++ b/prompts/completed/015-spec-003-harvest-section-boundaries.md @@ -0,0 +1,275 @@ +--- +status: completed +spec: [003-bench-review-sanity-and-harvest-boundary] +summary: Give harvest() explicit section end boundaries and add verbatim-capture fixture with 6 new tests +execution_id: coding-bench-harvest-exec-015-spec-003-harvest-section-boundaries +dark-factory-version: v0.192.9 +created: "2026-08-08T02:35:00Z" +queued: "2026-08-08T00:49:09Z" +started: "2026-08-08T00:49:11Z" +completed: "2026-08-08T00:52:55Z" +--- + + +- The benchmark's findings harvester stops inventing findings out of a review's closing prose +- A findings section now ends where a reader would say it ends: at the next heading, at a horizontal rule, or at the end of the report +- Only a bullet point can start a finding; plain sentences sitting in a section start nothing and can no longer absorb everything written after them +- The mandated "None." that a review writes for an empty section therefore yields nothing at all, which is what it always meant +- A real finding keeps exactly its own text — trailing summary paragraphs no longer get glued onto the end of it +- Section names written inside a fenced code block are treated as code, not as section headings +- The heading level a review happens to use stops mattering anywhere in the harvest +- Test data gains a verbatim capture of output a live review really emitted — the exact input that produced the phantom finding — instead of another transcription of the command's own template +- The existing template-derived fixture and every assertion about it stay untouched, so both renderings are locked against regression + + + +Give `harvest()` in `bench/run.py` explicit section end boundaries and an explicit rule for what opens a finding, so the trailing prose that every real review wraps around its findings can never become a finding. Add a checked-in fixture that is a verbatim capture of live review output — the exact input that produced the phantom finding on `tts-mcp#20` — plus the tests that lock the new contract. This prompt changes a pure function and its test data only; it does not touch the runner's control flow. + + + +Read `CLAUDE.md` for project conventions (Python 3 stdlib only, no personal paths, generic examples only, never commit). + +Read `specs/in-progress/003-bench-review-sanity-and-harvest-boundary.md` — this prompt implements **Desired Behaviors 3, 4 and 5** and **Acceptance Criteria AC7, AC8, AC9, AC10, AC11**. Two sections are load-bearing: +- **`## Reference: captured real output`** — the 1,380-byte stdout captured from the 2026-08-08 run against `tts-mcp#20`. The fixture you add is derived from that block, verbatim. Read it before writing the fixture. +- **The paragraph after that block ("Verified root cause, which differs from the first diagnosis")** — heading level is *not* the defect. The harvester already matches all six heading levels. The defect is that nothing ends a section except another heading, so a thematic break and the trailing summary get appended as continuation lines to the still-open `None.` buffer, which defeats the sentinel check. + +Read `bench/run.py` — the file you are changing. The pieces that matter: + +```python +def harvest(report_text: str, known_rule_ids: set) -> list: + findings: list = [] + current_section: str | None = None + current_finding_lines: list[str] = [] + section_names = {"must fix", "should fix", "nice to have"} + + def flush_finding(): + ... + text = " ".join(current_finding_lines) + body = _normalize_body(current_finding_lines) + # Skip the "None." empty-section sentinel + if body.strip() in ("None.", "None"): + current_finding_lines = [] + return + rule_id = _extract_rule_id(text, known_rule_ids) + path, line_num = _extract_path_line(text, known_rule_ids) + findings.append({"path": path, "line": line_num, "rule_id": rule_id, "body": body}) + current_finding_lines = [] +``` + +and its main loop, which today does three things and nothing else: a heading of any level (`^#{1,6}\s+(.+)$`) flushes and either opens a section or closes one; inside a section a bullet (`^\s{0,3}([-*])\s+(.+)$`, matched against the *stripped* line) flushes and starts a finding; any other non-empty line inside a section is appended as a continuation line **whether or not a finding is open** — that last clause is the bug. + +Also read, and reuse unchanged: `_extract_rule_id(text, known_rule_ids)`, `_extract_path_line(text, known_rule_ids)`, `_normalize_body(lines)`, `load_rule_ids(coding_repo)`, `BENCH_DIR`, `REPO_ROOT`. + +Read `bench/testdata/sample-report.md` — the pre-existing template-derived fixture. It harvests to exactly 3 findings today and must still harvest to exactly the same 3 after your change. Its SHA-256 is `de40c00e7d3c452fa7475be9fa6541426058a96dba91487460aa53be0bd186ae`; that value must be unchanged when you are done. + +Read `bench/test_review.py` — the test file you extend. 13 tests today, plain `unittest.TestCase` classes with a docstring naming the AC, `import run` / `import testsupport` (discovery puts `bench/` on `sys.path`). Match that style; do not introduce a test framework. The three tests that already cover harvesting are `test_harvest_normalizes_sample_report`, `test_harvest_keeps_finding_without_any_rule_id`, `test_harvest_ignores_empty_section` — none of them may be deleted, renamed, or have an assertion removed or loosened. + +Read `commands/pr-review.md` Step 5 (search for `**MANDATORY**: Always include all three headers`) for the three section names and the mandated `None.` sentinel. **Do not edit that file.** + +Read `docs/dod.md` — no personal paths anywhere in a shipped file, `## Unreleased` CHANGELOG entry required. + + + + +## 1. Module-level pattern constants in `bench/run.py` + +Add these next to the existing module constants (`NAME_RE`, `PR_ID_RE`). They are compiled once and used by both `harvest()` here and, in a later prompt, the sanity gate — a single definition per pattern is what keeps the two from drifting. + +```python +REQUIRED_SECTION_NAMES = ("Must Fix", "Should Fix", "Nice to Have") +HEADING_RE = re.compile(r"^#{1,6}\s+(.+)$") +SEVERITY_SUFFIX_RE = re.compile(r"\s*\([^)]+\)\s*$") +THEMATIC_BREAK_RE = re.compile(r"^ {0,3}(?:-{3,}|\*{3,}|_{3,}) *$") +FENCE_RE = re.compile(r"^ {0,3}(?:```|~~~)") +BULLET_RE = re.compile(r"^\s{0,3}([-*])\s+(.+)$") +_SECTION_BY_LOWER = {name.lower(): name for name in REQUIRED_SECTION_NAMES} +``` + +Every one of these is line-anchored with a bounded quantifier and no nested repetition, so matching is linear in input size — the spec's "denial by pathological input" case. Do not introduce a pattern with a nested quantifier. + +`REQUIRED_SECTION_NAMES` is a frozen invariant. Do not add a flag, an environment variable, a parameter or a config field for it. + +## 2. `heading_section_name(line)` — the single heading rule + +```python +def heading_section_name(line: str) -> str | None: + """Return the canonical findings-section name a markdown heading line names, or None. + + Matches a heading at any level 1-6, strips a trailing parenthesised severity + annotation such as "(Critical)", and compares case-insensitively against + REQUIRED_SECTION_NAMES. Returns the canonical spelling ("Must Fix", + "Should Fix", "Nice to Have") or None when the line is not a heading or names + something else. + """ +``` + +Implementation: `HEADING_RE.match(line)`; on no match return `None`. Otherwise take group 1, `.strip()`, apply `SEVERITY_SUFFIX_RE.sub("", ...)`, `.strip()` again, and look the lowercased result up in `_SECTION_BY_LOWER`. + +Heading level carries no information — `## Must Fix (Critical)` and `#### Must Fix (Critical)` are the same section. Do not branch on the number of `#` characters anywhere. + +A line that merely mentions the words is not a heading: prose sentences and bold runs such as `**Must Fix**` have no leading `#` and therefore return `None`. + +## 3. `iter_report_lines(report_text)` — fence-aware line iteration + +```python +def iter_report_lines(report_text: str): + """Yield (line, in_fence) for every line of report_text. + + in_fence is True for the fence delimiter lines themselves and for every line + between an opening and a closing fence. A line inside a fence is never a + heading, never a thematic break and never a bullet; it is ordinary text. + """ +``` + +Implementation: iterate `report_text.splitlines()` with a boolean. When `FENCE_RE.match(line)`, toggle the boolean and yield `(line, True)`. Otherwise yield `(line, in_fence)` with the current value. + +Fence delimiters themselves yield `True` so an opening ```` ``` ```` can never be read as a heading. An unterminated fence leaves the remainder of the report inside the fence — that is the fail-closed reading and is correct: nothing after an unterminated fence can open a section. + +## 4. Rewrite the body of `harvest()` + +Keep the signature exactly as it is: `def harvest(report_text: str, known_rule_ids: set) -> list:`. Keep the returned dict shape exactly as it is: `{"path": ..., "line": ..., "rule_id": ..., "body": ...}` in document order. Keep `flush_finding`'s internals exactly as they are, including the `None.` / `None` exact-equality sentinel skip — **exact equality only, never a substring or `in` test**; a genuine finding whose body contains the word "None" must survive. + +Replace the loop with this contract, driven by `iter_report_lines`: + +1. **When the line is not inside a fence:** + - `name = heading_section_name(line)`. If the line matches `HEADING_RE` at all (heading of any level), `flush_finding()` first, then set `current_section = name` (which is `None` for any heading that is not one of the three), clear `current_finding_lines`, and continue to the next line. A heading always ends whatever section was open — a non-findings heading such as `### Step 4: Automated Checks` closes the section rather than extending it. + - Else if `THEMATIC_BREAK_RE.match(line)`: `flush_finding()`, set `current_section = None`, clear `current_finding_lines`, continue. **This is the fix.** A `---` line is how real review output separates its report body from the diff summary and the closing panel; without it nothing ends the last section and every trailing paragraph is swallowed. +2. **When `current_section is None`, ignore the line entirely** — no finding is opened, nothing is buffered. Preamble prose, the diff summary, the `Step 3a` / `Step 4` trace and the closing panel all land here. +3. **Inside a section**, with `stripped = line.strip()`: + - If the line is *not* in a fence and `BULLET_RE.match(stripped)` matches: `flush_finding()`, then `current_finding_lines = [match.group(2)]`. **A list item is the only thing that opens a finding.** + - Else if `stripped` is non-empty **and `current_finding_lines` is non-empty**: append `stripped` as a continuation line of the finding already open. + - Else: ignore. Non-empty prose in a section where no list item has opened a finding contributes nothing and buffers nothing — this is the mandated `None.` sentinel of an empty section, and it is why a `None.` section followed by a thematic break and a paragraph of summary now yields zero findings instead of one phantom. + - A blank line inside an open finding is ignored and does **not** close the finding — that is existing behaviour and must not change. +4. After the loop, `flush_finding()` once — end of input is the third and last thing that ends a section. + +Do not broaden what opens a finding. Numbered lists, bold-lead paragraphs and table rows are explicitly out of scope (spec Non-goals); adding speculative parsing is how the current fixture drifted from reality. + +Do not add any parameter, flag or keyword argument to `harvest()`. + +## 5. Add `bench/testdata/real-capture-report.md` + +A verbatim capture, not a transcription. Copy the fenced `text` block under `## Reference: captured real output` in `specs/in-progress/003-bench-review-sanity-and-harvest-boundary.md` character for character from its first line (`The diff is only ...`) through its `**Summary:** ...` line. Then append a thematic break line (`---`) and at least three non-empty prose lines standing in for the closing status panel the operator's global memory appends — write generic wording, no personal paths, no `/Users/`, no `~/Documents/`, no machine names. + +The file must satisfy all of these, and you must run them: + +``` +grep -cE '^## (Must Fix|Should Fix|Nice to Have)' bench/testdata/real-capture-report.md # 3 +grep -cE '^#### (Must Fix|Should Fix|Nice to Have)' bench/testdata/real-capture-report.md # 0 +grep -c '^\*\*Summary:\*\*' bench/testdata/real-capture-report.md # >= 1 +grep -c 'fast-uri' bench/testdata/real-capture-report.md # >= 1 +grep -c 'mcp/package-lock.json' bench/testdata/real-capture-report.md # >= 1 +grep -c 'Step 3a: LICENSE Check' bench/testdata/real-capture-report.md # >= 1 +grep -rn '/Users/\|~/Documents/' bench/testdata/ # no output, exit 1 +``` + +The last three content greps are the point of the whole fixture: those literals appear nowhere in `commands/pr-review.md`'s template and cannot be produced by transcribing it. A file hand-written to satisfy only the structural greps reintroduces, at the fixture layer, exactly the template-agrees-with-itself failure this spec exists to close. + +Use the filename `bench/testdata/real-capture-report.md` exactly — a later prompt in this spec references it by name. + +## 6. Add tests to `bench/test_review.py` + +Append new `unittest.TestCase` classes in the existing style. Every test runs offline: no network, no `claude` binary, no GitHub access. Use `run.load_rule_ids(run.REPO_ROOT)` for the id set, as the existing harvest tests do. + +1. **`test_real_capture_harvests_to_zero_findings`** (AC8) — read `run.BENCH_DIR / "testdata" / "real-capture-report.md"`, call `run.harvest(text, ids)`, and assert the **full returned list** equals `[]` with `assertEqual(findings, [], f"real capture must harvest to zero findings, got: {findings}")`. Assert on the whole list, not on `len()` — the failure message must print the unexpected findings. This is the exact input that produced the phantom finding. + +2. **`test_trailing_prose_does_not_swallow_a_real_finding`** (AC9) — build an inline report with exactly one list-item finding under `## Must Fix (Critical)` citing a rule id that really exists in `rules/index.json` and a `path.ext:NN` reference, `None.` under `## Should Fix (Important)` and `## Nice to Have (Optional)`, then a `---` thematic break and two paragraphs of summary prose (at least one of them beginning `**Summary:**`). Assert the full returned list equals a one-element list whose `body` is exactly the list item's own text with no trailing prose appended, and whose `path` and `line` are the values cited in that item. Compare the whole list with one `assertEqual`, not field by field. + + **The finding's own body MUST contain the literal word `None` as a substring** — write it about a `None` default argument, e.g. a body along the lines of "passing `None` as the default here hides the missing-value case". This is the single case that distinguishes the required implementation from the lazy one: requirement 4 pins the empty-section sentinel to exact equality (`body.strip() in ("None.", "None")`), and a regression to substring membership (`"None" in body`) passes every other test in this prompt while silently discarding any legitimate finding that mentions `None` — a common word in Python review comments. Without this, the D4 fix can regress into a body-substring filter undetected. + +3. **`test_heading_level_does_not_change_harvest`** (AC10) — the test method name must contain `heading_level`. Render one identical body of section content at `##`, `###` and `####` (use a loop or an f-string over `("##", "###", "####")`), harvest all three, and assert with a single `assertEqual` that all three lists are equal to each other, with a failure message printing all three lists. + +4. **`test_section_name_in_prose_or_fence_is_not_a_heading`** — assert `run.heading_section_name("**Must Fix**")` is `None`, `run.heading_section_name("We looked at Must Fix items.")` is `None`, `run.heading_section_name("## Must Fix (Critical)")` is `"Must Fix"`, `run.heading_section_name("###### nice to have")` is `"Nice to Have"`, and that harvesting a report whose only `## Must Fix (Critical)` line sits inside a ```` ``` ```` fenced block yields `[]`. + +5. **`test_thematic_break_ends_a_section`** — a report with `## Nice to Have (Optional)` holding one real list-item finding, followed by `---` and two paragraphs of prose. Assert exactly one finding is returned and its `body` does not contain any word from the trailing prose. Without the thematic-break rule the trailing prose is appended to that finding, so this test fails on the old code for a different reason than test 1 does. + +6. **`test_prose_before_a_list_item_opens_nothing`** — a section whose content is `None.` followed by a blank line and then a real list-item finding. Assert exactly one finding is returned, and its body is the list item's text alone with no `None.` prefix. This pins requirement 4.3: prose that opened nothing cannot be extended, and cannot merge into the finding that follows it. + +Do not delete, rename, or weaken any of the 13 existing tests in `bench/test_review.py`, and do not touch `bench/test_config.py` or `bench/test_resolve.py`. + +## 7. CHANGELOG + +`CHANGELOG.md` currently has no `## Unreleased` section — its first version heading is `## v0.35.1`. Insert a `## Unreleased` section immediately above `## v0.35.1` and add a bullet using a conventional prefix (`fix:` — every other CHANGELOG entry uses `feat:`/`fix:`/`docs:` per `docs/changelog-guide.md`; the two non-conforming `bench:` bullets are on v0.35.1, the misclassified release this spec exists because of) describing the harvest section-boundary fix and the new captured fixture. Do not create a version section, do not touch any released section, and do not touch the four version strings in `.claude-plugin/`. + +## 8. Do not modify + +`bench/testdata/sample-report.md` (its SHA-256 must stay `de40c00e7d3c452fa7475be9fa6541426058a96dba91487460aa53be0bd186ae`), `bench/prs.json`, `bench/README.md`, `Makefile`, `commands/`, `rules/`, `agents/`, `docs/`, `scripts/`, `specs/`, `.claude-plugin/`. + +`bench/testsupport.py`, `process_pr`, `run_bench` and the raw-output cache write are untouched by this prompt — the non-review sanity gate is prompt 2 of this spec. Do not add a gate, a section-presence check, or any new behaviour to `process_pr` here. + + + +- Python 3 standard library only — no `pip`, no third-party imports, no `requirements.txt`, no `pyproject.toml`, no new top-level files outside `bench/` +- Changes land only in `bench/run.py`, `bench/test_review.py`, `bench/testdata/real-capture-report.md` and `CHANGELOG.md` +- The 42 existing tests keep passing and their assertions are not weakened. Deleting a test, removing an assertion, or relaxing an assertion is not acceptable. The suite's test count after this prompt is strictly greater than 42 +- `bench/testdata/sample-report.md` still harvests to its previously asserted 3 findings, unchanged, and the file itself is byte-identical +- Frozen invariants — not configurable, not flagged: the three required section names, the list-item markers that open a finding, the 45-minute review timeout, the cache and results locations, the `--golden` exit-2 rejection +- `bench/prs.json` remains a frozen input — schema, entries and `dev-1` version unchanged +- No rule, agent, command or doc that participates in a review may be edited, including `commands/pr-review.md`. This spec adapts the instrument to the review's real output, never the reverse +- Do NOT broaden what opens a finding: no numbered lists, no bold-lead paragraphs, no tables +- Review output is third-party-influenced text. The harvester only matches and slices text — no value from it is evaluated, passed to a shell, used to build a filesystem path, or used to construct a subprocess argument +- Section, heading, fence and thematic-break matching is line-oriented with bounded patterns; no construct in a report may cause unbounded backtracking or a scan that is not linear in input size +- No personal paths (`/Users/`, `~/Documents/`) in any shipped file, including the new fixture (`docs/dod.md`) +- `CHANGELOG.md` gains an entry under `## Unreleased` (`docs/dod.md`) +- All new tests run offline: no network, no real `claude` binary, no GitHub access +- Do NOT re-harvest or migrate anything already sitting in `bench/.cache/reviews/` +- Do NOT commit — dark-factory handles git + + + +``` +# Full suite: must be OK with strictly more than 42 tests +python3 -m unittest discover -s bench -p 'test_*.py' -v 2>&1 | tail -20 + +# The new tests exist by name +grep -n 'def test_real_capture_harvests_to_zero_findings\|def test_trailing_prose_does_not_swallow_a_real_finding\|heading_level\|def test_thematic_break_ends_a_section\|def test_prose_before_a_list_item_opens_nothing' bench/test_review.py + +# No pre-existing test was deleted or renamed (all 13 originals still present) +for t in test_second_run_is_cache_hit_and_invokes_zero_reviews test_mode_change_is_cache_miss \ + test_cache_path_differs_when_only_mode_differs test_harvest_normalizes_sample_report \ + test_harvest_keeps_finding_without_any_rule_id test_harvest_ignores_empty_section \ + test_ledger_is_append_only_and_atomic test_second_runner_exits_without_touching_ledger \ + test_row_carries_every_required_field test_raw_output_is_cached_verbatim \ + test_failed_review_leaves_no_row_and_no_cache_entry test_failed_pr_does_not_prevent_later_prs \ + test_corrupt_cache_row_is_treated_as_miss; do + grep -q "def $t" bench/test_review.py || echo "MISSING TEST: $t" +done + +# The template-derived fixture is byte-identical (expect the hash below) +sha256sum bench/testdata/sample-report.md +echo "expected: de40c00e7d3c452fa7475be9fa6541426058a96dba91487460aa53be0bd186ae" + +# The new fixture is a capture, not a transcription +grep -cE '^## (Must Fix|Should Fix|Nice to Have)' bench/testdata/real-capture-report.md # expect 3 +grep -cE '^#### (Must Fix|Should Fix|Nice to Have)' bench/testdata/real-capture-report.md # expect 0 +grep -c '^\*\*Summary:\*\*' bench/testdata/real-capture-report.md # expect >= 1 +grep -c 'fast-uri' bench/testdata/real-capture-report.md # expect >= 1 +grep -c 'mcp/package-lock.json' bench/testdata/real-capture-report.md # expect >= 1 +grep -c 'Step 3a: LICENSE Check' bench/testdata/real-capture-report.md # expect >= 1 + +# End-to-end proof the phantom finding is gone and the old fixture is unchanged +python3 -c " +import sys; sys.path.insert(0, 'bench') +import run +ids = run.load_rule_ids(run.REPO_ROOT) +cap = (run.BENCH_DIR / 'testdata' / 'real-capture-report.md').read_text() +old = (run.BENCH_DIR / 'testdata' / 'sample-report.md').read_text() +print('real capture ->', run.harvest(cap, ids)) +assert run.harvest(cap, ids) == [], 'real capture must harvest to zero findings' +print('sample-report ->', len(run.harvest(old, ids)), 'findings') +assert len(run.harvest(old, ids)) == 3, 'sample-report must still harvest to 3 findings' +print('OK') +" + +# No personal paths, stdlib-only imports +grep -rn '/Users/\|~/Documents/' bench/ ; echo "personal-path grep exit=$? (expect 1)" +grep -nE '^(import |from )' bench/run.py + +# Unreleased section exists +sed -n '/^## Unreleased/,/^## v/p' CHANGELOG.md + +# Repo gate +make precommit +``` + +Expected: `make precommit` exits 0; the verbose unittest run prints `OK` and `Ran N tests` with `N > 42`; the `MISSING TEST:` loop prints nothing; the `sha256sum` matches the expected value; the fixture greps print `3`, `0`, and `>= 1` for the four content checks; the inline Python prints `real capture -> []`, `sample-report -> 3 findings` and `OK`; the personal-path grep exits 1 with no output. + diff --git a/prompts/completed/016-spec-003-non-review-sanity-gate.md b/prompts/completed/016-spec-003-non-review-sanity-gate.md new file mode 100644 index 0000000..0d334c3 --- /dev/null +++ b/prompts/completed/016-spec-003-non-review-sanity-gate.md @@ -0,0 +1,310 @@ +--- +status: completed +spec: [003-bench-review-sanity-and-harvest-boundary] +summary: 'Added non-review sanity gate to bench/run.py: rejects output missing Must Fix/Should Fix/Nice to Have headings before caching or harvesting, wired after the non-zero-exit check and before atomic_write_bytes, with bounded stderr diagnosis and 6 new tests' +execution_id: coding-bench-harvest-exec-016-spec-003-non-review-sanity-gate +dark-factory-version: v0.192.9 +created: "2026-08-08T02:35:00Z" +queued: "2026-08-08T00:49:09Z" +started: "2026-08-08T00:52:57Z" +completed: "2026-08-08T00:57:27Z" +--- + + +- Output that is not structurally a review can no longer be written down as a perfect clean review +- A report counts as a review only when all three mandatory finding sections are actually present as headings; anything else fails that pull request loudly +- A subprocess that exits successfully after printing an error message is now caught — that is the exact failure that scored a broken invocation as flawless +- A rejected pull request leaves nothing behind: no result row, no cached output, so the next run simply retries it +- The remaining pull requests still run, and the whole run finishes with a failing exit code and lists the rejection in its summary +- The rejection message names the pull request, names each section that was missing, and quotes what the runner actually got, so the operator diagnoses it without opening a cache file +- The quoted excerpt is size-bounded, so a runaway subprocess printing megabytes cannot flood the terminal +- Section names mentioned in prose, in bold, or inside a code block do not count as sections — only real headings do +- The heading level a review uses still does not matter: reports at either observed level are accepted and recorded +- The existing tests that drove the runner with output that was never review-shaped are updated to real review shape, with every assertion kept + + + +Add a sanity gate to `bench/run.py` that rejects review output which is not structurally a review — output missing any of the three mandatory finding sections — before the raw output is cached and before it is harvested, so no ledger row and no cache entry can ever be written for a non-review. The rejection names the PR, names each missing section, and carries a bounded verbatim excerpt on stderr. This closes the defect where `Unknown command: /coding:pr-review` on stdout with exit code 0 was recorded as `ok: 0 findings`. + + + +Read `CLAUDE.md` for project conventions (Python 3 stdlib only, no personal paths, never commit). + +Read `specs/in-progress/003-bench-review-sanity-and-harvest-boundary.md` — this prompt implements **Desired Behaviors 1 and 2** and **Acceptance Criteria AC2, AC3, AC4, AC5, AC6**. Load-bearing sections: the **Failure Modes** table (rows for "subprocess exits 0 but prints an error", "truncated mid-report", "contract drift", "rejected output is large", "crash between the gate and the ledger"), and **Security / Abuse Cases** ("fail-closed, not fail-open"; "denial by volume"). + +**This prompt depends on prompt 1 of this spec having landed.** Prompt 1 added `REQUIRED_SECTION_NAMES`, `HEADING_RE`, `SEVERITY_SUFFIX_RE`, `FENCE_RE`, `heading_section_name(line)` and `iter_report_lines(report_text)` to `bench/run.py`. Verify they exist before you start: + +```bash +grep -n 'REQUIRED_SECTION_NAMES\|def heading_section_name\|def iter_report_lines' bench/run.py +``` + +If any is absent, stop and report `status: failed` with the message `"prompt 1 of spec 003 (harvest section boundaries) not yet landed"`. Do not re-implement them here — a second copy of the heading rule is exactly the drift this spec exists to prevent. + +Read `bench/run.py`. The parts you touch: + +- `BenchError` — the single exception type; raised inside `process_pr` it becomes a per-PR `failed:` outcome, raised outside the loop it becomes exit code 2. +- `process_pr(*, entry, coding_repo, results_dir, cache_root, model, effort, mode, config_dir, cfg_hash, rc_hash, prs_version, known_rule_ids) -> tuple[str, str]` — its current sequence is: (1) cache check and early return, (2) `resolve_pr`, (3) `build_review_argv` + `invoke_review`, (4) `TimeoutExpired` → failure log + re-raise, (5) non-zero `returncode` → failure log + `BenchError`, (6) `atomic_write_bytes(raw_path, proc.stdout.encode("utf-8"))`, (7) `harvest`, (8) `build_row` + `append_row`, (9) cache marker. **The gate goes between step 5 and step 6.** +- `run_bench` — it catches `BenchError` per PR, records `outcome, detail = "failed", str(err)`, prints `f"{pr_id}: {outcome}: {detail}"` **to stdout**, counts outcomes by the prefixes `"ok:"` / `"cache hit:"` / `"failed:"`, prints `summary: N ok, N cache hit, N failed`, and returns `1` when any PR failed. Do not change any of that. Because it prints to stdout, the gate must write its own diagnosis to stderr itself. +- `sys` is already imported. `failures_root` / `failure_log_path` exist and are used by the timeout and non-zero-exit paths only. + +Read `bench/testsupport.py`. `stub_claude(bin_dir, counter_file, report_text="")` writes a `/bin/sh` stub that appends `"$*"` to `counter_file` and then emits `report_text` via `cat <<'REPORT_EOF' ... REPORT_EOF`, so a multi-line payload works as long as no line of it equals `REPORT_EOF`, and the stub's stdout is always `report_text + "\n"`. + +Read `bench/test_review.py`. Six tests currently pass a payload to `stub_claude` that is **not review-shaped** and will be rejected by the gate — all six must be updated: + +| Test | Current payload | +|---|---| +| `test_second_run_is_cache_hit_and_invokes_zero_reviews` | `"findings: []"` | +| `test_mode_change_is_cache_miss` | `"findings: []"` | +| `test_row_carries_every_required_field` | `"findings: []"` | +| `test_failed_pr_does_not_prevent_later_prs` | `"findings: []"` | +| `test_corrupt_cache_row_is_treated_as_miss` | `"findings: []"` | +| `test_raw_output_is_cached_verbatim` | `"findings: [{\"rule_id\":\"foo/bar\",\"path\":\"x.go\",\"line\":1}]"` | + +`test_failed_review_leaves_no_row_and_no_cache_entry` uses `stub_claude_failing` (non-zero exit) and needs no payload change. + +Read `commands/pr-review.md` Step 5 (search for `**MANDATORY**: Always include all three headers`) — the contract the gate enforces. **Do not edit that file.** + +Read `docs/dod.md` — no personal paths, `## Unreleased` CHANGELOG entry. + + + + +## 1. Gate constants in `bench/run.py` + +Add next to the existing module constants: + +```python +NON_REVIEW_MARKER = "NOT A REVIEW" +REJECTION_EXCERPT_BYTES = 2000 +``` + +Both are frozen invariants. Do not add a flag, an environment variable, a parameter or a config field for either. `REJECTION_EXCERPT_BYTES` is what keeps a runaway subprocess printing megabytes from flooding the operator's terminal, and it must be small enough that a 100 kB payload still produces a total stderr diagnosis under 8 kB. + +## 2. `missing_sections(report_text)` + +```python +def missing_sections(report_text: str) -> list[str]: + """Return the required findings-section names absent from report_text, in canonical order. + + A section counts as present only when it appears as a markdown heading at any + level 1-6 outside a fenced code block. The words appearing in prose, in a + bold run, or inside a fence do not count. Returns [] when all three are + present. + """ +``` + +Implementation: walk `iter_report_lines(report_text)`; skip every line whose `in_fence` flag is `True`; for the rest, collect `heading_section_name(line)` when it is not `None`; return `[name for name in REQUIRED_SECTION_NAMES if name not in present]`. + +**Return canonical order** — `REQUIRED_SECTION_NAMES` order, i.e. `Must Fix`, `Should Fix`, `Nice to Have`. AC4 Case B asserts `Should Fix` and `Nice to Have` appear in that order; iterating the tuple gives it for free. + +**Compute per-heading presence.** Do not special-case "report the last section as missing" or any other shortcut: a payload carrying only `Must Fix` must return both `Should Fix` and `Nice to Have`, and a payload carrying `Must Fix` and `Should Fix` must return only `Nice to Have`. + +## 3. `rejection_excerpt(text)` + +```python +def rejection_excerpt(text: str, limit: int = REJECTION_EXCERPT_BYTES) -> str: + """Return at most limit bytes of text's UTF-8 prefix, marked when truncated.""" +``` + +Encode `text` as UTF-8. If the encoding is at most `limit` bytes, decode and return it unchanged. Otherwise slice the first `limit` bytes, decode with `errors="ignore"` so a multi-byte character split at the boundary is dropped rather than raising, and append a visible truncation marker naming the total byte count, e.g. `f"\n[... truncated, {total} bytes total]"`. The excerpt reproduces the subprocess's own output and nothing else — never append an environment variable, a token, or any credential material. + +## 4. `non_review_report(pr_id, missing, stdout_text)` + +```python +def non_review_report(pr_id: str, missing: list[str], stdout_text: str) -> str: + """Build the multi-line stderr diagnosis for output rejected as a non-review.""" +``` + +Return exactly this shape: + +``` +NOT A REVIEW: +missing sections: +no ledger row and no cache entry were written; this PR is retried on the next run +--- rejected output excerpt ( bytes total) --- + +--- end excerpt --- +``` + +where `NOT A REVIEW` is `NON_REVIEW_MARKER` and `` is `len(stdout_text.encode("utf-8"))`. + +**The line beginning `missing sections: ` must be the only place any required section name appears in the diagnosis, apart from the verbatim excerpt.** Do not enumerate the full set of required sections, do not print "expected Must Fix, Should Fix, Nice to Have", and do not add a legend. AC4 asserts on the content of that one line; a message that also lists what *was* found makes the criterion unverifiable. + +## 5. Wire the gate into `process_pr` + +Insert immediately after the existing non-zero-`returncode` block and **before** `atomic_write_bytes(raw_path, ...)`: + +```python +missing = missing_sections(proc.stdout) +if missing: + print(non_review_report(pr_id, missing, proc.stdout), file=sys.stderr) + raise BenchError( + f"{NON_REVIEW_MARKER}: {pr_id}: missing sections: {', '.join(missing)}" + ) +``` + +Consequences that must hold and that you must not work around: + +- The gate runs on **fresh subprocess output only**. It sits after the cache-hit early return, so a cache hit is never re-validated, and it is not applied to previously cached output. +- Nothing is written for a rejected review: the raw-output cache write, the harvest, the ledger append and the cache marker all come after it. Do **not** write a failure log for a gate rejection — failure logs stay exclusively on the timeout and non-zero-exit paths (the spec's Non-goals put the failure-log mechanism out of scope). +- The `BenchError` propagates to `run_bench`'s per-PR handler, which records `failed: NOT A REVIEW: ...`, prints it, continues with the remaining PRs, and returns `1`. Do not add a retry, do not add a fallback, do not downgrade the rejection to a warning. +- The `BenchError` message carries the marker, the PR id and the missing list, but **not** the excerpt — the excerpt is stderr-only, so the stdout summary line stays one readable line. + +Fail-closed: on any ambiguity about whether output is a review, the outcome is rejection. A false rejection costs one re-run; a false acceptance writes a fabricated measurement into an append-only ledger. + +## 6. Add `review_report(...)` to `bench/testsupport.py` + +```python +def review_report(*, must_fix: str | None = "None.", should_fix: str | None = "None.", + nice_to_have: str | None = "None.", heading_level: int = 2, + preamble: str = "", trailing: str = "") -> str: + """Build a review-shaped stub payload. + + Renders the three mandatory sections at heading_level, in the order Must Fix, + Should Fix, Nice to Have, each carrying the given body. Passing None for a + section omits that section entirely, which is how a non-review payload is + built. preamble is emitted before the first section and trailing after the + last one, both verbatim and both empty by default. + """ +``` + +Render each present section as `"#" * heading_level + " " + name + " " + annotation`, using the annotations the review command's template writes: `(Critical)`, `(Important)`, `(Optional)`. No line of the result may equal `REPORT_EOF` (the stub's heredoc delimiter). + +Also add a module-level `CLEAN_REVIEW_REPORT = review_report()` for the common case, so the six updated tests read as one word rather than six copies of the same literal. + +## 7. Update the six existing stub payloads — without weakening any assertion + +Replace each `"findings: []"` payload listed in `` with `testsupport.CLEAN_REVIEW_REPORT`. For `test_raw_output_is_cached_verbatim`, set `report_text = testsupport.review_report(must_fix="- `agent-cmd/command-thin`: sample finding at `agents/x.md:12`.")` and leave the assertion `self.assertEqual(raw_path.read_text(), report_text + "\n", ...)` exactly as it is — the stub's heredoc still yields `report_text + "\n"` for a multi-line payload. + +Rule for this whole requirement: **update payloads only.** Do not delete a test, do not remove an assertion, do not relax an assertion, do not change an expected count, and do not add a skip. Every rule id you write into a payload must be an `id` that really exists in `rules/index.json`; do not invent ids and do not edit `rules/index.json`. + +## 8. Add tests to `bench/test_review.py` + +Existing style: plain `unittest.TestCase`, `tempfile.TemporaryDirectory()`, seed a repo with `testsupport.make_merge_repo` under `/repos//`, build a one-PR manifest with `testsupport.make_manifest`, build a plugin with `testsupport.build_coding_repo` and a matching config dir with `testsupport.build_verify_config_dir(..., use_known_marketplaces=True)`, install the stub with `testsupport.stub_claude`, then call `run.run_bench(...)` inside `with mock.patch.dict(os.environ, env):`. Copy that harness; do not invent a new one. + +The gate writes to `sys.stderr` via `print(..., file=sys.stderr)`, so capture it with `contextlib.redirect_stderr(io.StringIO())` around the `run_bench` call. Add `import contextlib` and `import io` to the test file's imports. + +Because this harness repeats six times, factor it into one module-level helper in `bench/test_review.py` — e.g. `run_one_pr_with_payload(td, payload) -> tuple[int, str, pathlib.Path, pathlib.Path]` returning `(returncode, captured_stderr, results_dir, cache_root)` — and have the new tests call it. Do not move the helper into `testsupport.py`; it is specific to these tests. + +1. **`test_non_review_output_is_rejected`** (AC2, AC5) — payload is exactly `Unknown command: /coding:pr-review`. Assert: `run_bench` returns `1`; captured stderr contains `run.NON_REVIEW_MARKER`; captured stderr contains the PR id; captured stderr contains the literal `Unknown command:` (the operator must diagnose it without opening a cache file); the ledger file either does not exist or has 0 lines; `run.reviews_root(cache_root)` contains no `.json` and no `.stdout.txt` file. + +2. **`test_section_names_outside_headings_do_not_satisfy_the_gate`** (AC3) — payload in which all three literals appear but none as a heading: `Must Fix` in a prose sentence, `## Should Fix (Important)` inside a ```` ``` ```` fenced block, and `**Nice to Have**` as a bold run. Assert the same outcomes as test 1 (return `1`, marker on stderr, zero ledger rows, no review cache files). Without this test a substring check would satisfy AC2. + +3. **`test_missing_section_names_are_reported_exactly`** (AC4) — two cases in one test, each asserting on the single stderr line that starts with `missing sections: `. Extract it with something like `next(l for l in stderr.splitlines() if l.startswith("missing sections: "))` and assert on the remainder of that line, **not** on the whole stderr (the verbatim excerpt reproduces the payload, which legitimately contains the section names that were present). + - Case A: `testsupport.review_report(nice_to_have=None)` → the list is exactly `Nice to Have`; assert it does not contain `Must Fix` and does not contain `Should Fix`; ledger gains 0 rows; exit code `1`. + - Case B: `testsupport.review_report(should_fix=None, nice_to_have=None)` → the list is exactly `Should Fix, Nice to Have`, in that order; assert it does not contain `Must Fix`; ledger gains 0 rows; exit code `1`. + + Case B is what makes a hardcoded "report the last section as missing" implementation fail. + +4. **`test_rejection_excerpt_is_bounded`** (AC5) — a pure unit test on `run.non_review_report("test#1", ["Must Fix"], "x" * 100_000)`: assert `len(result.encode("utf-8")) < 8192`, and assert the result contains the truncation marker. No subprocess, no temp dirs. + +5. **`test_review_shaped_output_at_either_heading_level_produces_a_row`** (AC6) — two independent runs over one-PR manifests, one with `testsupport.review_report(heading_level=2)` and one with `testsupport.review_report(heading_level=4)`. Assert for both: `run_bench` returns `0`; the ledger contains exactly 1 row; the row's `pr_id` equals the manifest's single id. Use separate results directories and separate cache roots so the second run is not a cache hit. + +6. **`test_gate_does_not_apply_to_a_cache_hit`** — run once with a review-shaped payload so a row and a cache entry are written; then run again against the same cache root with the stub payload replaced by `Unknown command: /coding:pr-review`. Assert the second run returns `0` and reports a cache hit, proving the gate is not applied to previously cached output. This pins the spec constraint "the gate runs on fresh subprocess output only ... it is not applied to cache hits". + +Every test runs offline: no network, no real `claude` binary, no GitHub access. + +## 9. CHANGELOG + +Add a bullet under the existing `## Unreleased` heading in `CHANGELOG.md` (prompt 1 of this spec created that section) using a conventional prefix (`fix:`, per `docs/changelog-guide.md` — not the non-conforming `bench:` style on v0.35.1), describing the rejection of output that is not structurally a review. Do not create a version section, do not touch any released section, and do not touch the four version strings in `.claude-plugin/`. + +## 10. Do not modify + +`bench/testdata/sample-report.md`, `bench/testdata/real-capture-report.md`, `bench/prs.json`, `bench/README.md`, `Makefile`, `commands/`, `rules/`, `agents/`, `docs/`, `scripts/`, `specs/`, `.claude-plugin/`. + +Do not change `harvest()`, `heading_section_name()`, `iter_report_lines()` or any pattern constant that prompt 1 added — reuse them. Do not change `run_bench`'s banner, per-PR exception handling, outcome counting, summary printing or return-code logic. Do not touch the timeout or non-zero-exit failure-log paths. `bench/README.md` and the whole-change CHANGELOG bullet are prompt 3 of this spec. + + + +- Python 3 standard library only — no `pip`, no third-party imports, no new top-level files outside `bench/` +- Changes land only in `bench/run.py`, `bench/testsupport.py`, `bench/test_review.py` and `CHANGELOG.md` +- The existing tests keep passing and their assertions are not weakened. Updating the six stub payloads to review-shaped output is expected and correct; deleting a test, removing an assertion, or relaxing an assertion to accommodate the gate is not. The suite's test count after this prompt is strictly greater than after prompt 1 +- The gate runs on fresh subprocess output only, ahead of the raw-output cache write, so a rejected review leaves nothing under `bench/.cache/reviews/` and is retried naturally on the next invocation. It is not applied to cache hits and does not re-validate previously cached output +- A rejected PR produces no ledger row and no cache entry; the remaining PRs still run; the process exits non-zero — the same treatment an empty diff gets today +- Do NOT add a retry loop, a fallback, or a warning-only mode around a rejection. Fail-closed, not fail-open +- Do NOT write a failure log for a gate rejection — the failure-log mechanism is out of scope for this spec (Non-goals D3) +- Frozen invariants — not configurable, not flagged: the three required section names, the fact that all three are required, the list-item markers, the stderr excerpt bound, the 45-minute review timeout, the cache and results locations, the `--golden` exit-2 rejection +- `bench/prs.json` remains a frozen input — schema, entries and `dev-1` version unchanged +- No rule, agent, command or doc that participates in a review may be edited, including `commands/pr-review.md` +- Review output is third-party-influenced text: the gate only matches text and slices it. No value from it is evaluated, passed to a shell, used to build a filesystem path, or used to construct a subprocess argument +- The rejection excerpt is bounded and written to stderr only; it reproduces the subprocess's own output and never copies an environment variable, token or credential into any artifact +- Heading, fence and section matching is line-oriented with bounded patterns; no construct in a report may cause unbounded backtracking or a scan that is not linear in input size +- No personal paths (`/Users/`, `~/Documents/`) in any shipped file (`docs/dod.md`) +- `CHANGELOG.md` gains an entry under `## Unreleased` (`docs/dod.md`) +- All new tests run offline: no network, no real `claude` binary, no GitHub access +- Do NOT re-harvest or migrate anything already sitting in `bench/.cache/reviews/` +- Do NOT commit — dark-factory handles git + + + +``` +# Prompt 1 primitives are present and reused, not duplicated +grep -n 'REQUIRED_SECTION_NAMES\|def heading_section_name\|def iter_report_lines\|def missing_sections\|def rejection_excerpt\|def non_review_report\|NON_REVIEW_MARKER\|REJECTION_EXCERPT_BYTES' bench/run.py + +# The gate sits before the raw-output cache write in process_pr +grep -n 'missing_sections\|atomic_write_bytes(raw_path\|findings = harvest\|append_row' bench/run.py + +# Full suite +python3 -m unittest discover -s bench -p 'test_*.py' -v 2>&1 | tail -25 + +# New tests exist by name +grep -n 'def test_non_review_output_is_rejected\|def test_section_names_outside_headings_do_not_satisfy_the_gate\|def test_missing_section_names_are_reported_exactly\|def test_rejection_excerpt_is_bounded\|def test_review_shaped_output_at_either_heading_level_produces_a_row\|def test_gate_does_not_apply_to_a_cache_hit' bench/test_review.py + +# No pre-existing test was deleted or renamed +for t in test_second_run_is_cache_hit_and_invokes_zero_reviews test_mode_change_is_cache_miss \ + test_cache_path_differs_when_only_mode_differs test_harvest_normalizes_sample_report \ + test_harvest_keeps_finding_without_any_rule_id test_harvest_ignores_empty_section \ + test_ledger_is_append_only_and_atomic test_second_runner_exits_without_touching_ledger \ + test_row_carries_every_required_field test_raw_output_is_cached_verbatim \ + test_failed_review_leaves_no_row_and_no_cache_entry test_failed_pr_does_not_prevent_later_prs \ + test_corrupt_cache_row_is_treated_as_miss; do + grep -q "def $t" bench/test_review.py || echo "MISSING TEST: $t" +done + +# The gate's own contract, exercised directly +python3 -c " +import sys; sys.path.insert(0, 'bench') +import run, testsupport +print('non-review ->', run.missing_sections('Unknown command: /coding:pr-review')) +print('all three present ->', run.missing_sections(testsupport.review_report())) +print('level 4 present ->', run.missing_sections(testsupport.review_report(heading_level=4))) +print('case A ->', run.missing_sections(testsupport.review_report(nice_to_have=None))) +print('case B ->', run.missing_sections(testsupport.review_report(should_fix=None, nice_to_have=None))) +print('prose/fence/bold ->', run.missing_sections('We looked at Must Fix items.\n\n\`\`\`\n## Should Fix (Important)\n\`\`\`\n\n**Nice to Have**\n')) +assert run.missing_sections(testsupport.review_report()) == [] +assert run.missing_sections(testsupport.review_report(heading_level=4)) == [] +assert run.missing_sections(testsupport.review_report(nice_to_have=None)) == ['Nice to Have'] +assert run.missing_sections(testsupport.review_report(should_fix=None, nice_to_have=None)) == ['Should Fix', 'Nice to Have'] +big = run.non_review_report('test#1', ['Must Fix'], 'x' * 100000) +print('100kB payload -> stderr bytes:', len(big.encode('utf-8'))) +assert len(big.encode('utf-8')) < 8192 +print('OK') +" + +# The real capture from prompt 1 still passes the gate (it IS a review) +python3 -c " +import sys; sys.path.insert(0, 'bench') +import run +t = (run.BENCH_DIR / 'testdata' / 'real-capture-report.md').read_text() +print('real capture missing sections ->', run.missing_sections(t)) +assert run.missing_sections(t) == [] +print('OK') +" + +# No personal paths, stdlib-only imports +grep -rn '/Users/\|~/Documents/' bench/ ; echo "personal-path grep exit=$? (expect 1)" +grep -nE '^(import |from )' bench/run.py bench/testsupport.py bench/test_review.py + +# Reserved and mandatory flags unchanged +python3 bench/run.py --golden bench/golden.json ; echo "golden exit=$? (expect 2)" +python3 bench/run.py ; echo "no-flags exit=$? (expect 2)" + +# Unreleased section +sed -n '/^## Unreleased/,/^## v/p' CHANGELOG.md + +# Repo gate +make precommit +``` + +Expected: `make precommit` exits 0; the verbose unittest run prints `OK` with a test count strictly greater than after prompt 1 and shows the non-review-gate, missing-section, excerpt-bound, heading-level and cache-hit tests by name; the `MISSING TEST:` loop prints nothing; the first inline Python prints `['Must Fix', 'Should Fix', 'Nice to Have']` for the non-review and the prose/fence/bold payload, `[]` for both review-shaped payloads, `['Nice to Have']` for case A, `['Should Fix', 'Nice to Have']` for case B, a stderr byte count under 8192, and `OK`; the second inline Python prints `[]` and `OK`; the personal-path grep exits 1 with no output; both `run.py` invocations exit 2. + diff --git a/prompts/completed/017-spec-003-harvest-contract-docs-and-changelog.md b/prompts/completed/017-spec-003-harvest-contract-docs-and-changelog.md new file mode 100644 index 0000000..e22a1ae --- /dev/null +++ b/prompts/completed/017-spec-003-harvest-contract-docs-and-changelog.md @@ -0,0 +1,188 @@ +--- +status: completed +spec: [003-bench-review-sanity-and-harvest-boundary] +summary: Documented harvest contract in bench/README.md and consolidated Unreleased changelog entry covering all of spec 003 +execution_id: coding-bench-harvest-exec-017-spec-003-harvest-contract-docs-and-changelog +dark-factory-version: v0.192.9 +created: "2026-08-08T02:35:00Z" +queued: "2026-08-08T00:49:10Z" +started: "2026-08-08T00:57:29Z" +completed: "2026-08-08T00:59:20Z" +--- + + +- The rules the benchmark uses to read a review are written down where the next fixture author will actually look +- Anyone adding test data now learns, without reading the parser, what ends a findings section and what starts a finding +- The three sections a review must contain, and what happens when one is missing, are documented alongside those rules +- The documentation names the two checked-in fixtures and says which one is a real capture and which one is derived from the command's template +- The changelog entry describes the whole change — both the rejection of non-review output and the harvest boundary fix — so the release classifier weighs it correctly instead of cutting a patch for a feature +- A final sweep confirms no personal filesystem paths and no third-party dependencies were introduced anywhere in the benchmark +- The full repository gate runs green with a larger test suite than before + + + +Write the harvest contract down in `bench/README.md` — what ends a findings section, what opens a finding, and the three-section requirement the sanity gate enforces — and consolidate `CHANGELOG.md`'s `## Unreleased` section so one reader can see the whole change. The root cause of the phantom finding was a contract that existed only as a parser and a fixture that agreed with each other and with nothing else; this prompt closes that. Last prompt of spec 003, deliberately last so the changelog can describe what actually shipped. + + + +Read `CLAUDE.md` for project conventions (Python 3 stdlib only, no personal paths, generic examples only, never commit). + +Read `specs/in-progress/003-bench-review-sanity-and-harvest-boundary.md` — this prompt satisfies **Acceptance Criteria AC1, AC12, AC13, AC14** and **Desired Behavior 5**'s second half ("The harvest contract ... is written down alongside the fixtures"). Two constraints are load-bearing: +- *"The CHANGELOG bullet describes the whole change, not the last prompt's slice."* Spec 002 shipped a 1,075-line runner whose Unreleased section never described it, and the release classifier consequently cut a patch instead of a minor. +- *"Do NOT make the sanity gate's stderr excerpt length, the set of required section names, or the list-item markers configurable — all three are invariants."* Document them as fixed; do not document a knob that does not exist. + +**This prompt depends on prompts 1 and 2 of this spec having landed.** Verify before you start: + +```bash +grep -n 'def heading_section_name\|def iter_report_lines\|def missing_sections\|def non_review_report' bench/run.py +ls bench/testdata/ +``` + +If `heading_section_name`, `iter_report_lines`, `missing_sections` or `non_review_report` is absent, or `bench/testdata/real-capture-report.md` does not exist, stop and report `status: failed` with the message `"prompts 1-2 of spec 003 not yet landed"`. Do not implement them here. + +Read `bench/run.py` — the shipped behaviour you are documenting. Take the contract from the code, not from memory: `REQUIRED_SECTION_NAMES`, `THEMATIC_BREAK_RE`, `BULLET_RE`, `FENCE_RE`, `heading_section_name`, `iter_report_lines`, `harvest`, `missing_sections`, `rejection_excerpt`, `non_review_report`, and the gate call inside `process_pr`. Do not describe anything the code does not do. + +Read `bench/README.md` — the file you extend. It already documents the configuration tuple, `prs.json`, how to run it, the diff-range rule, the `EMPTY DIFF` abort, the fixed invariants, the safety invariant and the result-row schema. It says nothing about how review output is read. Match its voice: short declarative sections, a `##` heading per topic, tables where the shape fits, a "why" paragraph wherever a reader would otherwise restore the wrong behaviour. + +Read `bench/testdata/sample-report.md` (template-derived, `####` headings, three findings) and `bench/testdata/real-capture-report.md` (verbatim capture of live output, `##` headings, all three sections `None.`, trailing prose, zero findings) — the two fixtures you name in the new section. + +Read `CHANGELOG.md` — its `## Unreleased` section, created by prompt 1 and appended to by prompt 2, sits above `## v0.35.1`. Read `docs/changelog-guide.md` for entry style. + +Read `docs/dod.md` — no personal paths anywhere, `## Unreleased` entry required, 4-version alignment not touched. + + + + +## 1. Add a `## Reading review output` section to `bench/README.md` + +Place it after the `## Diff-range rule` section and before `## Verifying an entry without cloning`. It documents three things and their rationale. + +**a. The three required sections and the sanity gate.** `commands/pr-review.md` Step 5 marks `Must Fix`, `Should Fix` and `Nice to Have` **MANDATORY** and mandates the literal `None.` for an empty section. A report is a review only when all three appear as markdown headings; output missing any of them is rejected before the raw output is cached and before it is harvested, so a rejected PR leaves no ledger row and no cache entry, the remaining PRs still run, and the process exits non-zero — the same treatment an `EMPTY DIFF` gets, for the same reason. The rejection names the PR, names each missing section on its own `missing sections: ` line, and carries a bounded verbatim excerpt on stderr. Record why: a subprocess that exits 0 after printing `Unknown command: /coding:pr-review` is otherwise indistinguishable from a genuinely clean review, and a fabricated clean row is byte-shaped exactly like a real one. + +**b. What ends a findings section.** A section's content ends at the next markdown heading of any level, at a thematic break (`---`, `***` or `___` on its own line), or at end of input — whichever comes first. The section names are matched as headings at any level with the severity annotation optional; a mention in prose, in a bold run, or inside a fenced code block is not a heading. Record why heading level carries no information: the command's template renders the sections at one level and captured live output rendered them at another, so level is not evidence of anything. + +**c. What opens a finding.** Inside a findings section a finding starts when a list item starts (`-` or `*`); subsequent non-list lines extend the finding already open. Prose appearing in a section before any list item — most importantly the mandated `None.` sentinel — contributes no finding and cannot be extended by anything that follows it. Record why: real review output carries a diff summary and a closing status panel after the last section, and before the boundary rules existed those lines were appended as continuation lines to the still-open `None.` buffer, which defeated the sentinel check and emitted the accumulated text as one finding with no path, line or rule id. That is the phantom finding the known-clean fixture PR recorded. + +Name both fixtures and say what each one locks: + +| Fixture | Origin | Harvests to | +|---|---|---| +| `bench/testdata/sample-report.md` | derived from the review command's Step 5 template, `####` headings | 3 findings | +| `bench/testdata/real-capture-report.md` | verbatim capture of live review output, `##` headings, all three sections `None.`, trailing prose | 0 findings | + +State the rule for anyone adding a third: a fixture is a capture of real output, not a transcription of the template. Both defects this section documents survived 42 green unit tests because the tests were built from the same template the parser was built from. + +Do not restate the harvest normalization rules the code does not have. Do not document a configuration knob: the three required section names, the fact that all three are required, the list-item markers and the stderr excerpt bound are fixed invariants. Add them to the existing `## Fixed invariants` list rather than inventing a parallel list. + +The section must satisfy, and you must run: + +``` +grep -nE 'thematic break|ends at the next' bench/README.md # >= 1 line +grep -cE 'Must Fix|Should Fix|Nice to Have' bench/README.md # >= 3 +``` + +## 2. Consolidate the `## Unreleased` CHANGELOG entry + +Rewrite the bullets under `## Unreleased` in `CHANGELOG.md` so they describe the whole of spec 003 — not the last prompt's slice. Use conventional prefixes (`fix:` for both defect fixes, `docs:` for the contract documentation) per `docs/changelog-guide.md`, one bullet per logical change. Do NOT reuse the `bench: ...` style: those two bullets on v0.35.1 are the only non-conforming entries in the file, and v0.35.1 is precisely the release whose bullets under-described a 1,075-line change and drew a patch bump instead of a minor. The section must cover, at minimum: + +- the bench runner rejecting review output that is not structurally a review (all three mandatory finding sections present as headings), before the raw-output cache write, leaving no ledger row and no cache entry, with the PR listed as failed and a bounded stderr excerpt naming each missing section +- the harvest section-boundary fix: a findings section ends at the next heading, a thematic break, or end of input, and only a list item opens a finding, so trailing prose can no longer become a phantom finding +- the new `bench/testdata/real-capture-report.md` fixture — a verbatim capture of live review output rather than a transcription of the command's template +- the documented harvest contract in `bench/README.md` + +The release classifier reads these bullets to choose the version bump. The literal checks it must pass: + +``` +sed -n '/^## Unreleased/,/^## v/p' CHANGELOG.md > /tmp/unreleased.txt +grep -ciE 'bench' /tmp/unreleased.txt # >= 1 +grep -ciE 'harvest|finding' /tmp/unreleased.txt # >= 1 +grep -ciE 'not a review|non-review|sanity' /tmp/unreleased.txt # >= 1 +``` + +Do not create a version section, do not rename `## Unreleased` to a version, do not touch any released section, and do not touch the four version strings in `.claude-plugin/plugin.json` and `.claude-plugin/marketplace.json` — releases are manual and handled by maintainer-agent-releaser. + +## 3. Final sweep + +Run and fix anything these surface. They are checks, not licence to refactor: + +- `grep -rn '/Users/\|~/Documents/' bench/` must return no lines (exit 1). This includes both fixtures and the README. +- Every `import` / `from` line in `bench/run.py`, `bench/testsupport.py` and every `bench/test_*.py` names a Python 3 standard-library module only. No third-party imports, no `requirements.txt`, no `pyproject.toml`, no `setup.py`. +- `python3 -m unittest discover -s bench -p 'test_*.py'` reports `OK` with `Ran N tests`, `N > 42`. +- `make precommit` exits 0. + +If any of these fails for a reason introduced by prompt 1 or prompt 2, fix it here rather than leaving the spec unshippable, and say so in the completion report. + +## 4. Do not modify + +`bench/run.py` behaviour (documentation-only prompt — change it only if the sweep in requirement 3 surfaces a genuine defect, and say so in the report), `bench/testdata/sample-report.md`, `bench/testdata/real-capture-report.md`, `bench/prs.json`, `Makefile`, `commands/`, `rules/`, `agents/`, `docs/`, `scripts/`, `specs/`, `.claude-plugin/`. + +Do not add a scenario. The spec's **Scenario coverage** section is explicit: both defects are reachable in unit tests, and the remaining evidence (AC15–AC18) needs real tokens against a live review, which no scenario harness can supply. + + + +- Python 3 standard library only — no third-party dependencies anywhere in `bench/` +- Changes land only in `bench/README.md` and `CHANGELOG.md` (plus any fix requirement 3 genuinely forces) +- The existing tests keep passing and their assertions are not weakened. The suite's test count is strictly greater than 42 +- `make precommit` (which runs `bench-test`) stays green. Bench tests must not require network access, a real `claude` binary, or GitHub access +- Frozen invariants — document them as fixed, never as configurable: the three required section names, the fact that all three are required, the list-item markers that open a finding, the stderr excerpt bound, the 45-minute review timeout, the cache and results locations, the `--golden` exit-2 rejection +- `bench/prs.json` remains a frozen input — schema, entries and `dev-1` version unchanged +- No rule, agent, command or doc that participates in a review may be edited, including `commands/pr-review.md`. This spec adapts the instrument to the review's real output, never the reverse +- Generic examples only — no trading-domain content (`CLAUDE.md`) +- No personal paths (`/Users/`, `~/Documents/`) in any shipped file (`docs/dod.md`) +- The `## Unreleased` CHANGELOG section describes the whole change, in terms a release classifier can weigh +- The 4-version alignment is NOT touched — releases are manual (`docs/dod.md`) +- Do NOT re-harvest or migrate anything already sitting in `bench/.cache/reviews/` +- Do NOT add a scenario file +- Do NOT commit — dark-factory handles git + + + +``` +# AC12 — the harvest contract is written down where the next fixture author reads it +grep -nE 'thematic break|ends at the next' bench/README.md +grep -cE 'Must Fix|Should Fix|Nice to Have' bench/README.md +grep -n 'real-capture-report.md\|sample-report.md' bench/README.md + +# AC14 — the Unreleased section describes the whole change +sed -n '/^## Unreleased/,/^## v/p' CHANGELOG.md | tee /tmp/unreleased.txt +test -s /tmp/unreleased.txt && echo "unreleased section non-empty" +grep -ciE 'bench' /tmp/unreleased.txt # expect >= 1 +grep -ciE 'harvest|finding' /tmp/unreleased.txt # expect >= 1 +grep -ciE 'not a review|non-review|sanity' /tmp/unreleased.txt # expect >= 1 + +# AC13 — no personal paths, stdlib-only imports +grep -rn '/Users/\|~/Documents/' bench/ ; echo "personal-path grep exit=$? (expect 1)" +grep -nE '^(import |from )' bench/run.py bench/testsupport.py bench/test_*.py + +# No packaging crept in +ls bench/ ; test ! -e requirements.txt && test ! -e pyproject.toml && test ! -e setup.py && echo "no packaging files" + +# AC1 — the suite grew and is green +python3 -m unittest discover -s bench -p 'test_*.py' -v 2>&1 | tail -25 + +# Both fixtures still harvest to their asserted answers +python3 -c " +import sys; sys.path.insert(0, 'bench') +import run +ids = run.load_rule_ids(run.REPO_ROOT) +cap = (run.BENCH_DIR / 'testdata' / 'real-capture-report.md').read_text() +tpl = (run.BENCH_DIR / 'testdata' / 'sample-report.md').read_text() +assert run.harvest(cap, ids) == [], run.harvest(cap, ids) +assert len(run.harvest(tpl, ids)) == 3, run.harvest(tpl, ids) +assert run.missing_sections(cap) == [] +assert run.missing_sections('Unknown command: /coding:pr-review') == list(run.REQUIRED_SECTION_NAMES) +print('OK') +" + +# Reserved and mandatory flags unchanged +python3 bench/run.py --golden bench/golden.json ; echo "golden exit=$? (expect 2)" +python3 bench/run.py ; echo "no-flags exit=$? (expect 2)" + +# Repo gate +make precommit +``` + +Expected: `make precommit` exits 0; `grep -nE 'thematic break|ends at the next' bench/README.md` returns at least one line; the section-name count is at least 3; the extracted Unreleased section is non-empty and all three `grep -ci` checks return at least 1; the personal-path grep exits 1 with no output; every import line names a stdlib module; the verbose unittest run prints `OK` with `Ran N tests`, `N > 42`; the inline Python prints `OK`; both `run.py` invocations exit 2. + +Operator-executed after merge, in the spec-verification phase (real tokens, live review command, not runnable here): AC15–AC18 in the spec's **Operator-executable** block — a fresh five-PR run recording five rows, `tts-mcp#20` scoring `0` findings, zero phantom findings across the run, and every recorded row's raw output carrying all three section headings. + diff --git a/specs/in-progress/003-bench-review-sanity-and-harvest-boundary.md b/specs/in-progress/003-bench-review-sanity-and-harvest-boundary.md new file mode 100644 index 0000000..dc66c2e --- /dev/null +++ b/specs/in-progress/003-bench-review-sanity-and-harvest-boundary.md @@ -0,0 +1,211 @@ +--- +status: verifying +tags: + - dark-factory + - spec +approved: "2026-08-08T00:23:04Z" +generating: "2026-08-08T00:23:31Z" +prompted: "2026-08-08T00:39:42Z" +verifying: "2026-08-08T00:59:20Z" +branch: dark-factory/bench-review-sanity-and-harvest-boundary +--- + +## Summary + +- The benchmark runner shipped in v0.35.1 and its first real end-to-end run on 2026-08-08 produced two wrong numbers. This spec fixes exactly those two, and nothing else. +- **A non-review was scored as a perfect clean review.** The review subprocess printed `Unknown command: /coding:pr-review` (35 bytes) and exited 0. No guard fired, so the ledger recorded `ok: 0 findings` — the single worst outcome a benchmark can have, because it inflates precision, destroys recall, and does it silently. +- **The findings harvester invented a finding that was not there.** The known-clean fixture PR, whose entire purpose is to score zero, recorded one finding with no path, no line, and no rule id. +- Both defects survived 42 green unit tests because the tests were built from the same template the parser was built from, never from captured output. The fix therefore includes a fixture that is a real capture, not a transcription. +- Two other known defects from the same run (plugin path resolution, stderr-only failure logs) are deliberately excluded and named in Non-goals. + +## Problem + +The bench runner is the instrument that decides whether a rule change made reviews better or worse. Its first real run against the five pinned fixture PRs produced a result file that looks clean and is wrong in both directions at once. A broken command invocation — the subprocess exiting 0 after printing an error to stdout — passed every guard the runner has and was written down as a review that found nothing, which a scorer would read as flawless precision. At the same time the harvester, on genuinely good output, swept the report's closing prose into a phantom finding, so the one fixture PR curated specifically because the correct answer is zero recorded a one. An instrument that reports a fabricated zero for a broken run and a fabricated one for a clean run is not measuring anything; every downstream number — precision, recall, noise floor, model comparison — is built on it, so the error does not stay contained. Neither defect is detectable from the outside: the ledger row for a non-review is byte-shaped exactly like the row for a real clean review. + +## Goal + +The runner refuses to write down a number it cannot justify. Review output that is not structurally a review — missing any of the three mandatory finding sections that the review command declares mandatory — fails that PR loudly and leaves no ledger row and no cache entry behind, exactly as an empty diff already does. Review output that *is* a review is harvested with explicit section boundaries, so the narrative prose a real review wraps around its findings never becomes a finding, at any markdown heading level. The test fixtures that guard both behaviours are captures of real output rather than transcriptions of the command's template, so a future divergence between template and reality fails a test instead of shipping. + +## Non-goals + +- Do NOT fix the plugin path resolution defect (D1) — the preflight resolves the marketplace / `installLocation` path while Claude Code actually loads from `plugins/cache////`. Real and critical, but it changes *what the preflight resolves*, which is a different contract with its own failure modes. Separate spec. +- Do NOT change the failure-log mechanism (D3) — failure logs preserve stderr only, while Claude Code writes real errors to stdout. This spec's sanity gate writes its own bounded stderr excerpt; it does not touch how timeout and non-zero-exit failure logs are written. Separate spec. +- Do NOT make `review_env()` supply an authentication token — the runner cannot authenticate on its own; that is an operator-environment concern. +- Do NOT build scoring, a golden set, or any precision/recall semantics. `--golden` stays reserved-and-rejected with exit 2, exactly as it is today. +- Do NOT change any rule, agent, command, or doc that participates in a review, including `commands/pr-review.md`. The measured configuration must stay fixed while the instrument is repaired — changing both at once destroys the baseline the first run established. +- Do NOT re-harvest or migrate raw outputs already sitting in `bench/.cache/reviews/`. The new boundary rules apply to output harvested from this change forward; an operator who wants the old rows re-normalized deletes the cache and re-runs. +- Do NOT make the sanity gate's stderr excerpt length, the set of required section names, or the list-item markers configurable — all three are invariants. If a future consumer demands variation, that is a separate spec. +- Do NOT broaden what opens a finding (numbered lists, bold-lead paragraphs, tables). No observed review output uses them; adding speculative parsing is how the current fixture drifted from reality in the first place. + +## Desired Behavior + +1. **Output that is not structurally a review fails the PR, loudly.** `commands/pr-review.md` Step 5 states, in bold, that all three finding sections are mandatory and that an empty section is written as `None.` — so a report is a review only if all three of Must Fix, Should Fix, and Nice to Have are present as section headings. Output that lacks any of them is not a review, whatever the subprocess exit code was. The runner rejects it before the raw output is cached and before harvesting: no ledger row, no cache entry, the PR listed as failed in the run summary, remaining PRs still processed, and the process exits non-zero — the same treatment an empty diff gets today, for the same reason (it is otherwise indistinguishable from a genuinely clean review). + +2. **The gate recognises headings, names what is missing, and shows what it got.** The three section names are matched as markdown headings at any level, with the severity annotation (`(Critical)`, `(Important)`, `(Optional)`) optional — the review command's template writes them at one level and real output was observed at another, so level is not evidence of anything. A bare mention of the words in prose or inside a fenced block is not a heading and does not satisfy the gate. The rejection message names the PR id, names each missing section, and carries a bounded verbatim excerpt of the rejected output, so the operator can tell `Unknown command: /coding:pr-review` apart from a truncated report without opening a cache file. + +3. **Findings sections have explicit end boundaries.** A section's content ends at the next markdown heading, at a thematic break, or at end of input — whichever comes first. Real review output carries arbitrary trailing prose after the last section: a diff summary, and the operator's global memory appends a closing status panel. None of that is inside a findings section, so none of it can become a finding, and none of it can attach itself to the finding above it. + +4. **Only a list item opens a finding, and `None.` opens nothing.** Inside a findings section, a finding starts when a list item starts; subsequent non-list lines extend the finding already open. Prose appearing in a section before any list item — most importantly the mandated `None.` sentinel of an empty section — contributes no finding and cannot be extended by anything that follows it. The observable consequence is that a report whose three sections all read `None.`, followed by a thematic break and a paragraph of summary prose, harvests to zero findings; and a report with one real finding plus the same trailing prose harvests to exactly that one finding, unaltered. + +5. **Fixtures are captured, not transcribed.** The checked-in test data includes at least one file that is a verbatim capture of output a live review actually emitted, carrying its real heading levels, its `None.` sentinels, and its trailing prose. The existing template-derived fixture stays, so both renderings are locked against regression. The harvest contract — what ends a section, what opens a finding, what the sanity gate requires — is written down alongside the fixtures, because the root cause of the phantom finding was a contract that existed only as a parser and a fixture that agreed with each other and with nothing else. + +## Constraints + +- **Language and dependencies:** Python 3 standard library only. Changes land in `bench/run.py`, `bench/test_*.py`, `bench/testsupport.py`, `bench/testdata/`, `bench/README.md`, and `CHANGELOG.md`. No packaging, no third-party imports, no new top-level files outside `bench/`. +- **The 42 existing tests keep passing, and their assertions are not weakened.** Six of them drive the runner with a stub `claude` whose stdout is `findings: []` — which is not review-shaped and will be rejected by Desired Behavior 1. Updating those stub payloads to review-shaped output is expected and correct; deleting a test, removing an assertion, or relaxing an assertion to accommodate the gate is not. The suite's test count after this work is strictly greater than 42. +- `make precommit` (which runs `bench-test`) stays green. Bench tests must not require network access, a real `claude` binary, or GitHub access. +- **The gate runs on fresh subprocess output only**, ahead of the raw-output cache write, so a rejected review leaves nothing under `bench/.cache/reviews/` and is retried naturally on the next invocation. It is not applied to cache hits and does not re-validate previously cached output. +- **Frozen invariants** (not configurable, not flagged): the three required section names; the fact that all three are required; the list-item markers that open a finding; the stderr excerpt bound; the 45-minute review timeout; the cache and results locations; the `--golden` exit-2 rejection. +- `bench/prs.json` remains a frozen input — schema, entries, and `dev-1` version unchanged. +- No rule, agent, command, or doc that participates in a review is edited, including `commands/pr-review.md`. This spec adapts the instrument to the review's real output, never the reverse. +- **Repo conventions that must not regress** (`docs/dod.md`): no personal paths (`/Users/`, `~/Documents/`) in any shipped file including the new fixture, and a `## Unreleased` CHANGELOG entry. +- **The CHANGELOG bullet describes the whole change, not the last prompt's slice.** Spec 002 shipped a 1,075-line runner whose Unreleased section never described it, and the release classifier consequently cut a patch instead of a minor. The entry for this work names both the sanity gate and the harvest boundary fix in terms a release classifier can weigh. + +## Assumptions + +- The review command's three mandatory section headings are a stable contract. `commands/pr-review.md` Step 5 marks them **MANDATORY** with a mandated `None.` for empty sections, which is what makes their absence a reliable non-review signal rather than a stylistic difference. +- Heading *level* is not part of that contract and is not stable: the command's template renders the sections at one level and captured live output rendered them at another. The gate and the harvester therefore treat every heading level as equivalent. +- Real review output carries arbitrary prose before the first section and after the last one — a preamble narrating the diff, a `Step 3a` / `Step 4` trace, a diff-size summary, and a trailing closing panel contributed by the operator's global memory. None of it is under the runner's control and none of it may be interpreted. +- A stub executable on `PATH` that prints a chosen payload to stdout and exits 0 is sufficient to reproduce the non-review defect in a unit test; `bench/testsupport.py` already provides that harness. No live `claude` binary is needed for any container-verifiable criterion. +- The fixture PR set is unchanged and `tts-mcp#20` remains the known-clean entry whose correct answer is zero findings. + +## Failure Modes + +| Trigger | Expected behavior | Recovery | Detection | Reversibility | Concurrency | +|---|---|---|---|---|---| +| Review subprocess exits 0 but prints an error instead of a report (`Unknown command: …`, usage text, an empty string) | That PR is rejected as a non-review: no ledger row, no cache entry; remaining PRs still run; process exits non-zero | Operator fixes the invocation or the plugin installation and re-runs — the uncached PR is retried, cached PRs are skipped | Non-zero exit; stderr carries the non-review marker, the PR id, the missing section names, and a bounded excerpt of the rejected output; summary lists the PR as failed | Fully reversible — nothing was written | Rows and cache entries for other PRs are untouched | +| Review output is truncated mid-report (subprocess killed, pipe closed) so only the first one or two sections are present | Rejected as a non-review; stderr names the sections that were missing | Re-run the same invocation | Same marker, with the missing section names distinguishing truncation from a wholesale non-review | Fully reversible | Append-only ledger unaffected | +| The review command's template changes the heading level of the three sections | No effect — the gate and the harvester match any heading level | None needed | Fixtures at both observed levels stay green | n/a | n/a | +| The review command renames or drops one of the three sections (contract drift) | Every PR is rejected as a non-review; the whole run fails loudly with zero rows | Operator reconciles the runner's required section names with the command in a follow-up change | All five PRs listed as failed with the same missing-section name — an unmistakable signature of contract drift rather than a per-PR fault | Fully reversible — no rows written | Whole run fails uniformly; no partial ledger to reconcile | +| Real output carries trailing prose (summary, closing panel) after the last section, or preamble prose before the first | Prose outside a findings section is never a finding; prose inside a section that never opened a list item is never a finding | None needed | The clean fixture PR reports zero findings | n/a | n/a | +| A genuine finding's continuation lines wrap across several lines, or a finding body contains the word `None` | The finding is preserved intact with its path, line, and rule id; only a section whose content never opened a list item yields nothing | None needed | Fixture with one real finding plus trailing prose harvests to exactly one finding with the body unchanged | n/a | n/a | +| Rejected output is large (a runaway subprocess printing megabytes) | Only a bounded prefix reaches stderr; the full output is not written to the cache | None needed | Excerpt is visibly truncated | Fully reversible | No disk growth in the cache from a rejected review | +| Crash or interrupt between the gate and the ledger append | Nothing is written: the raw-output cache write and the row append both happen after the gate passes, and the row append is atomic | Re-run; the PR is uncached and retried | Ledger has fewer rows than the manifest | Fully reversible | Atomic write-then-rename means no truncated row is ever observed | + +## Security / Abuse Cases + +- **Attacker-controlled surface:** the stdout of the review subprocess. It is third-party-influenced text (the reviewed repository's content flows into the model's output) that this change parses more carefully than before. +- **No evaluation, no execution:** the gate and the harvester only match text and slice it. No value from review output is passed to a shell, used to build a filesystem path, or used to construct a subprocess argument. +- **Denial by volume:** review output can be arbitrarily large. The rejection excerpt is bounded so a runaway subprocess cannot flood the operator's terminal or a log file, and rejected output is never persisted to the cache. +- **Denial by pathological input:** section and heading matching is line-oriented with bounded patterns; no construct in the report can cause unbounded backtracking or a scan that is not linear in the input size. +- **Secret leakage:** the rejection excerpt is written to stderr only and reproduces the subprocess's own output; the runner still copies no environment variables, tokens, or credential material into any artifact. +- **Fail-closed, not fail-open:** on any ambiguity about whether output is a review, the outcome is rejection. A false rejection costs one re-run; a false acceptance writes a fabricated measurement into an append-only ledger, which is the failure this spec exists to prevent. + +## Acceptance Criteria + +Each AC is tagged **[container]** (verifiable at prompt time with no network, no tokens, and no real `claude` binary) or **[operator]** (only observable on the host, because it spends real tokens against the live review command over the fixture PRs) — the same convention as spec 002, whose operator criteria are what caught both of these defects. + +- [ ] **AC1 [container]** `make precommit` exits 0 and the bench suite grew — evidence: exit code 0; `python3 -m unittest discover -s bench -p 'test_*.py'` stderr contains `OK` and a `Ran N tests` line with `N > 42`. +- [ ] **AC2 [container]** The exact observed non-review is rejected: with a stub `claude` on `PATH` that prints exactly `Unknown command: /coding:pr-review` to stdout and exits 0, run the runner over a one-PR temp manifest — evidence: process exit code non-zero; stderr contains the non-review marker literal and the PR id; the results file line count is unchanged (`wc -l` before == after); `ls bench/.cache/reviews/` shows no new file for that (PR, configuration) pair; stdout summary line reports `1 failed`. +- [ ] **AC3 [container]** The gate matches headings, not substrings: a stub payload in which the literals `Must Fix`, `Should Fix`, and `Nice to Have` all appear — one in a prose sentence, one inside a fenced code block, one in a bold run — but none as a markdown heading, is rejected exactly as in AC2 — evidence: exit code non-zero; stderr contains the non-review marker; results file gains 0 lines. (Without this criterion a substring check satisfies AC2.) +- [ ] **AC4 [container]** A partially-present report is rejected and the diagnosis names exactly what was missing, across **two** different combinations — evidence, both cases: exit code non-zero, results file gains 0 lines, and the missing-sections list in stderr matches exactly. + - Case A: payload carries Must Fix and Should Fix, omits Nice to Have → list contains `Nice to Have` and does **not** contain `Must Fix` or `Should Fix`. + - Case B: payload carries Must Fix only → list contains **both** `Should Fix` and `Nice to Have`, in that order, and does not contain `Must Fix`. + + Case B exists because a single-combination criterion is satisfiable by hardcoding "report the last section as missing" — that implementation passes Case A, AC2 and AC3 without ever computing per-heading presence, and fails Case B. +- [ ] **AC5 [container]** The rejection is diagnosable without opening a cache file: the stderr produced in AC2 contains the literal string `Unknown command:` from the rejected output — evidence: `grep -c 'Unknown command:' ` returns ≥1, and the captured excerpt is bounded (a stub payload of 100 kB produces stderr smaller than 8 kB). The exact excerpt size within that envelope, and the fixture's filename in AC7, are agent decides at impl time. +- [ ] **AC6 [container]** Review-shaped output at either observed heading level still produces a row: two runs over one-PR temp manifests, one stub payload rendering the three sections as `##` headings and one rendering them as `####` headings — evidence: both runs exit 0; each results file contains exactly 1 row; `jq -r .pr_id` on each prints the manifest's single id. +- [ ] **AC7 [container]** `bench/testdata/` contains a capture, not a transcription: a fixture file exists whose three section headings are `##`-level, whose three sections all read `None.`, and which carries a thematic break plus at least three non-empty prose lines after the last section — evidence: `grep -cE '^## (Must Fix|Should Fix|Nice to Have)' ` prints 3; `grep -cE '^#### (Must Fix|Should Fix|Nice to Have)' ` prints 0; `grep -c '^\*\*Summary:\*\*' ` prints ≥1; `grep -rn '/Users/\|~/Documents/' bench/testdata/` returns 0 lines (exit 1). + + **Content fidelity — the fixture must derive from the capture in `## Reference: captured real output`, not be written to satisfy the greps above.** Evidence: `grep -c 'fast-uri' ` returns ≥1 **and** `grep -c 'mcp/package-lock.json' ` returns ≥1 **and** `grep -c 'Step 3a: LICENSE Check' ` returns ≥1. These three literals appear nowhere in `commands/pr-review.md`'s template and cannot be produced by transcribing it. + + Without this, a hand-crafted file satisfying only the four structural greps passes AC7 and AC8 in full — which is precisely the template-agrees-with-itself failure this spec exists to close, reintroduced at the fixture layer. +- [ ] **AC8 [container]** That capture harvests to zero findings: a unit test feeds the AC7 fixture to the harvester — evidence: test exits 0; the assertion compares the full returned list against the empty list and its failure message prints the unexpected findings. (This is the exact input that produced the phantom finding on `tts-mcp#20`.) +- [ ] **AC9 [container]** Trailing prose does not swallow or corrupt a real finding: a unit test feeds a report with exactly one list-item finding under Must Fix, `None.` under the other two sections, then a thematic break and two paragraphs of summary prose — evidence: test exits 0; the assertion compares the full returned list against exactly one finding whose `body` equals the list item's text with no trailing prose appended, and whose `path` and `line` are the values cited in that item. (Without this criterion, dropping every finding whose body contains `None` satisfies AC8.) +- [ ] **AC10 [container]** Heading level is irrelevant to harvesting: a unit test renders identical section content at `##`, `###`, and `####` and asserts all three harvest to the same list — evidence: test exits 0; test name contains `heading_level`; the assertion compares all three lists for equality and prints them on failure. +- [ ] **AC11 [container]** The pre-existing template-derived fixture still harvests to its previously asserted findings, unchanged — evidence: `git diff origin/master -- bench/testdata/sample-report.md` is empty, and the existing sample-report harvest test's expected list is unmodified (`git diff origin/master -- bench/test_review.py | grep -c '^-.*def test_'` prints 0, proving no test was deleted). +- [ ] **AC12 [container]** The harvest contract is written down where the next fixture author will read it: `bench/README.md` documents what ends a findings section, what opens a finding, and the three-section requirement the sanity gate enforces — evidence: `grep -nE 'thematic break|ends at the next' bench/README.md` returns ≥1 line; `grep -cE 'Must Fix|Should Fix|Nice to Have' bench/README.md` returns ≥3. +- [ ] **AC13 [container]** The runner still carries no personal paths and no third-party dependencies — evidence: `grep -rn '/Users/\|~/Documents/' bench/` returns 0 lines (exit 1); every `import` / `from` line in `bench/run.py` names a Python 3 standard-library module only. +- [ ] **AC14 [container]** The CHANGELOG entry describes the whole change so a release classifier can weigh it — evidence: the `## Unreleased` section (extracted from `## Unreleased` up to the next `## ` line) contains a bullet mentioning the bench runner, the rejection of non-review output, and the harvest section-boundary fix; `grep -ciE 'bench' ` ≥1, `grep -ciE 'harvest|finding' ` ≥1, `grep -ciE 'not a review|non-review|sanity' ` ≥1; the extracted section is non-empty. +- [ ] **AC15 [operator]** A fresh full run over the five-PR fixture completes and records five rows — evidence: `make bench BENCH_ARGS="--model --effort --mode "` exits 0 after deleting `bench/.cache/reviews/` and `bench/results/`; `jq -s 'length' bench/results/results.jsonl` prints 5. +- [ ] **AC16 [operator]** The known-clean fixture PR scores zero, which is the number this spec exists to restore — evidence: `jq -r 'select(.pr_id=="tts-mcp#20") | .findings | length' bench/results/results.jsonl` prints `0`. +- [ ] **AC17 [operator]** No phantom findings anywhere in the run — evidence: `jq -r '.findings[].body' bench/results/results.jsonl | grep -cE '^None\.?( |$)|^\*\*Summary'` prints 0. +- [ ] **AC18 [operator]** Every recorded row came from output that really was a review — evidence: for each row's `raw_output_ref`, `grep -ciE '^#{1,6} +(must fix|should fix|nice to have)' ` prints 3; the count of raw-output files equals the row count. + +**Scenario coverage — NO new scenario.** Both defects are reachable in unit tests: the non-review is a stub executable printing 35 bytes and exiting 0, and the phantom finding is a fixture file fed to a pure function. The remaining evidence needs real tokens against a live review, which the scenario harness cannot supply either — AC15-AC18 are operator-executed after merge, exactly as spec 002's operator criteria were, and they are what surfaced these two defects in the first place. + +## Verification + +### Container-executable (runs inside the YOLO container at prompt time) + +``` +make precommit +python3 -m unittest discover -s bench -p 'test_*.py' -v +grep -rn '/Users/\|~/Documents/' bench/ +grep -cE '^## (Must Fix|Should Fix|Nice to Have)' bench/testdata/ +grep -cE '^#### (Must Fix|Should Fix|Nice to Have)' bench/testdata/ +git diff origin/master -- bench/testdata/sample-report.md +git diff origin/master -- bench/test_review.py | grep -c '^-.*def test_' +sed -n '/^## Unreleased/,/^## v/p' CHANGELOG.md +``` + +Expected: `make precommit` exits 0; the verbose unittest run reports `OK` with `Ran N tests`, `N > 42`, and shows the non-review-gate, missing-section, heading-level, real-capture, and trailing-prose tests by name; the personal-path grep returns nothing (exit 1); the real-capture fixture greps print `3` and `0` respectively; the `sample-report.md` diff is empty; the deleted-test grep prints `0`; the extracted Unreleased section names the runner, the rejection of non-review output, and the harvest fix. + +### Operator-executable (runs on the host, spends real tokens) + +``` +rm -rf bench/.cache/reviews bench/results +make bench BENCH_ARGS="--model --effort --mode " +jq -s 'length' bench/results/results.jsonl +jq -r 'select(.pr_id=="tts-mcp#20") | .findings | length' bench/results/results.jsonl +jq -r '.findings[].body' bench/results/results.jsonl | grep -cE '^None\.?( |$)|^\*\*Summary' +jq -r .raw_output_ref bench/results/results.jsonl | while read f; do grep -ciE '^#{1,6} +(must fix|should fix|nice to have)' "$f"; done +``` + +Expected: the run exits 0 and writes five rows; `tts-mcp#20` reports `0` findings; the phantom-finding grep prints `0`; every raw-output file reports `3`. + +## Reference: captured real output + +The 1,380-byte stdout captured from the 2026-08-08 run against `tts-mcp#20` — the input that produced the phantom finding. The fixture required by AC7/AC8 reproduces this through the `**Summary:**` line and then carries a trailing thematic break plus the closing-panel lines the real session emitted (their exact wording is operator-specific; what the fixture must preserve is that non-empty prose follows the last section). + +```text +The diff is only `mcp/package-lock.json` updating `fast-uri` from `3.1.4` -> `3.1.5`. This is a vendored lockfile change - the `node_modules/fast-uri/` path segment means it doesn't count toward rule-relevant files. + +--- + +## PR Review - bench-pr-20 -> bench-base-20 + +**Diff size:** 1 file, 3 insertions, 3 deletions + +**Changed file:** +- `mcp/package-lock.json` - `fast-uri` 3.1.4 -> 3.1.5 (node_modules vendored dependency) + +--- + +### Step 3a: LICENSE Check +LICENSE file present in repo root. + +### Step 4: Automated Checks +**Skipped.** The diff is `mcp/package-lock.json`, which contains only vendored node_modules paths. + +--- + +## Must Fix (Critical) +None. + +## Should Fix (Important) +None. + +## Nice to Have (Optional) +None. + +--- + +**Summary:** This PR bumps `fast-uri` in the MCP's vendored lockfile. No source code changed. No review findings. +``` + +**Verified root cause, which differs from the first diagnosis.** The harvester already matches the three section names at heading levels 1 through 6, so heading level alone is not the defect — feeding the block above to the shipped harvester and feeding the same block with `####` headings both produce the same single phantom finding, body `None. --- **Summary:** This PR bumps ...`. The mechanism is that nothing ends a section except another heading: the thematic break and the trailing summary are appended as continuation lines to the still-open `None.` buffer, which defeats the sentinel check and emits the accumulated text as one finding with no path, line, or rule id. Desired Behaviors 3 and 4 target that mechanism. The heading-level requirement is retained as a regression lock (AC6, AC10) because the current behaviour is correct and untested, not because it is broken. + +## Suggested Decomposition + +| # | Prompt focus | Covers DBs | Covers ACs | Depends on | +|---|---|---|---|---| +| 1 | Harvest section boundaries: sections end at the next heading, a thematic break, or end of input; only a list item opens a finding; the `None.` sentinel yields nothing. Add the real-capture fixture and the heading-level, trailing-prose, and clean-capture tests. | 3, 4, 5 | AC7, AC8, AC9, AC10, AC11 | — | +| 2 | The non-review sanity gate ahead of the raw-output cache write; missing-section diagnosis with a bounded stderr excerpt; update the six existing stub payloads to review-shaped output without weakening their assertions. | 1, 2 | AC2, AC3, AC4, AC5, AC6 | prompt 1 | +| 3 | `bench/README.md` harvest-contract section, CHANGELOG `## Unreleased` entry covering the whole change, personal-path and stdlib-only sweep, full precommit. | 5 | AC1, AC12, AC13, AC14 | prompts 1-2 | + +Rationale: prompt 1 is a pure-function change with fixture-only evidence and no runner wiring, so it lands and proves itself independently. Prompt 2 changes the runner's control flow and is the prompt that must update the six existing stub payloads — sequencing it after prompt 1 means the review-shaped payloads it writes are already validated by the corrected harvester, rather than both changes moving at once and neither being provable. Prompt 3 is docs and packaging, deliberately last so the CHANGELOG bullet can describe what actually shipped in both prompts — the specific failure mode from spec 002, where the final prompt described only its own slice and the release classifier cut a patch. AC15-AC18 are operator-executed after merge in the spec-verification phase. + +## Do-Nothing Option + +Doing nothing leaves the instrument reporting confident wrong numbers in both directions. The first real run has already produced them: a broken invocation scored as flawless, and the one PR curated because its correct answer is zero scored a one. Neither is visible in the result file — a fabricated clean row is byte-shaped exactly like a real one — so the failure mode is not "the benchmark is down", it is "the benchmark quietly agrees with whatever you hoped". Every downstream deliverable (golden set, scoring, noise floor, model comparison) reads these rows, so shipping scoring on top of them would launder the errors into numbers nobody can trace back. The alternatives considered: (a) hand-inspect every raw output before trusting a run — restores correctness but discards the entire point of a mechanical instrument, and does not scale past five PRs; (b) fix only the phantom finding and leave the sanity gate for later — cheaper, but the non-review defect is the more dangerous of the two precisely because it looks like success, and it is the one that would silently survive a rules refactor that broke the command; (c) fix only the gate and leave the harvester — leaves the known-clean fixture permanently unable to score zero, which makes the clean PR useless as a control. Both defects were found by the same single run, both live in the same file, and neither is measurable until the other is fixed.