From 94c42f824e6241444599e44f9a508cca6a390c65 Mon Sep 17 00:00:00 2001 From: Benjamin Borbe Date: Sat, 8 Aug 2026 13:22:24 +0200 Subject: [PATCH 1/6] add four real pr-review captures as harvest fixtures --- bench/testdata/capture-notes-block-h2.md | 17 ++++++ .../testdata/capture-numbered-findings-h3.md | 28 +++++++++ bench/testdata/capture-summary-trailer-h4.md | 21 +++++++ bench/testdata/capture-traceability-h4.md | 57 +++++++++++++++++++ 4 files changed, 123 insertions(+) create mode 100644 bench/testdata/capture-notes-block-h2.md create mode 100644 bench/testdata/capture-numbered-findings-h3.md create mode 100644 bench/testdata/capture-summary-trailer-h4.md create mode 100644 bench/testdata/capture-traceability-h4.md diff --git a/bench/testdata/capture-notes-block-h2.md b/bench/testdata/capture-notes-block-h2.md new file mode 100644 index 0000000..5d42e9f --- /dev/null +++ b/bench/testdata/capture-notes-block-h2.md @@ -0,0 +1,17 @@ +This is a clean, well-thought-out conversion. Everything checks out — CLAUDE.md itself was correctly updated to match (source of truth stays consistent), the `any`/`as`/`!` ban is honored, invariant comments were preserved and relocated to the new file extensions, and the `noUncheckedIndexedAccess` workaround in `log.ts` is a good example of solving the type problem without an escape hatch. No `make precommit` run (short mode skips it per instructions). + +## Must Fix (Critical) +None. + +## Should Fix (Important) +None. + +## Nice to Have (Optional) +None. + +**Notes:** +- precommit skipped (selector mode/short mode) — CI covers lint+test+typecheck +- `npm ci` was not run in this environment, so `tsc --noEmit` / `node --test` were not executed live; static review of all touched `.ts` files found no type errors, no `any`/`as`/`!`/`@ts-ignore`, and correct `.ts`-extension `require()` usage matching the documented `moduleResolution: "bundler"` invariant. Recommend confirming CI's `make check` is green before merge. +- LICENSE file present; README gained a License section pointing to it — harmless, unrelated to the TS conversion but fine. + +Worktree cleanup: not applicable — review ran directly in the current checkout (already at PR head), no worktree was created. diff --git a/bench/testdata/capture-numbered-findings-h3.md b/bench/testdata/capture-numbered-findings-h3.md new file mode 100644 index 0000000..80228fb --- /dev/null +++ b/bench/testdata/capture-numbered-findings-h3.md @@ -0,0 +1,28 @@ +## PR Review: `bench-pr-3` vs `bench-base-3` (python-skeleton, full mode) + +**Step 0**: Already at PR head (worktree short-circuit) — no worktree created/removed. +**Step 3a**: LICENSE present. +**Step 3b**: `make precommit` — **PASS** (ruff format/check, pytest 30 passed, mypy 12 files, trivy scan clean). +**Step 4**: Mechanical funnel (`ast-grep-runner`) — 0 findings across 74 YAML checks. Judgment-tier candidates dispatched to 5 owners (agent-auditor, go-quality-assistant, node-quality-assistant, python-architecture-assistant, python-quality-assistant). Go/Node rules and Python-architecture rules were confirmed false-positive triggers (glob matches on `Dockerfile`/`Makefile.*`/test file, not actual Go/Node/composition-root code) — no violations. + +### Must Fix (Critical) +None. + +### Should Fix (Important) +1. **`CHANGELOG.md:18`** — `- ci: install trivy in CI` uses prefix `ci:`, not in the recognized set (`feat/fix/refactor/test/docs/chore/perf`). Breaks automated version-bump detection. Fix: use `chore:`. *(rule: `changelog/conventional-prefix-required`)* +2. **`README.md` "Security gates" section (~lines 76-94)** — rationale/ADR-style content (why no severity threshold, why `osv-scanner` excluded, cross-skeleton comparison table) belongs in `CLAUDE.md`/ADR, not user-facing README. Fix: trim to a short factual statement; move rationale elsewhere. *(rule: `readme/user-facing-not-agent-context`)* +3. **`.github/workflows/ci.yml:32`** — `sudo apt-key add -` is deprecated; can silently break on a future `ubuntu-latest` bump. Fix: use a keyring-based install or switch to `aquasecurity/setup-trivy` action. +4. **CI + `Makefile.precommit`** — Trivy has no version pin (unlike `PIP_AUDIT_VERSION ?= 2.9.0` set for pip-audit in the same PR), so CI and local runs can diverge over time. Fix: pin a Trivy version. +5. **`Makefile.precommit` `trivy` target** — no `--severity` filter and no documented rationale for failing on any severity, unlike the `audit` target which explicitly explains its "any severity" choice. Fix: either add a severity threshold or document the deliberate all-severity choice. + +### Nice to Have (Optional) +- Manual Trivy apt-install (update/install/repo-key) duplicates the maintained `aquasecurity/setup-trivy` action — adds ~30-60s/run and maintenance surface with no caching/pinning. +- Commit subject `switch build backend to hatchling and add conventional changelog prefixes` is 73 chars (soft cap 50) — FYI only, not in the active rule set. + +### Positive notes +- `hatchling` build-backend switch is clean and correctly scoped; Dockerfile `README.md` copy fix is correct and well-commented. +- `pip-audit`/`trivy` wiring into `check`, `uv export` + `uvx pip-audit@pinned`, `mktemp`+`trap` cleanup — solid. +- ruff `S` (bandit) ruleset addition with reasoned `S104`/`S101` suppressions — good pattern. +- `tests/test_factory.py` swap to `TestClient` HTTP calls instead of `app.routes` introspection is a more resilient test, with a clear "why" comment. + +No functional or architectural code changed (`src/skeleton/` untouched) — this PR is build-backend + CI security-gate plumbing only. diff --git a/bench/testdata/capture-summary-trailer-h4.md b/bench/testdata/capture-summary-trailer-h4.md new file mode 100644 index 0000000..6a8197f --- /dev/null +++ b/bench/testdata/capture-summary-trailer-h4.md @@ -0,0 +1,21 @@ +## Report — Short Mode Review (bench-pr-11 vs bench-base-11) + +Step 4 skipped (short mode). LICENSE present. `go build ./...` clean; `go test ./pkg/... ./cmd/...` passes (11 packages). + +#### Must Fix (Critical) +None. + +#### Should Fix (Important) +None. + +#### Nice to Have (Optional) +None. + +**Summary**: Adds `--skip-post` to `cmd/run-task`, threading `SkipPost` through `RunConfig` → `ResolvePosters` (returns interface-typed nils, not concrete-pointer nils — correctly avoids the typed-nil-interface trap). Adds a matching nil guard in `reviewStep.tryDismissHallucinated` (the previously-unreachable dead path that would have panicked under `--skip-post --phase ai_review`). Test coverage is thorough — direction tests for `ResolvePosters`, a dedicated boundary-contract test file (`pkg/skip_post_boundary_test.go`) exercising all three poster/verifier consumers through the real return value rather than a literal `nil`. Docs (`CLAUDE.md`, `README.md`, `docs/pr-post-back.md`) correctly de-conflate `cmd/run-task` vs `cmd/cli`. Unrelated `klauspost/compress` bump is a legitimate vuln fix, changelog'd appropriately. `prompts/` additions are dark-factory inbox artifacts, not reviewable code. + +precommit skipped (short mode) — CI covers lint+test. + +### Step 6 +No test coverage gaps — none. + +Worktree not used (already at PR head); nothing to clean up. diff --git a/bench/testdata/capture-traceability-h4.md b/bench/testdata/capture-traceability-h4.md new file mode 100644 index 0000000..ca4f1e2 --- /dev/null +++ b/bench/testdata/capture-traceability-h4.md @@ -0,0 +1,57 @@ +## PR Review: bench-pr-2 → bench-base-2 (full mode) + +**Scope**: CommonJS JS → CommonJS TypeScript conversion of the node-skeleton reference service (18 files, 783/-111). + +### Automated Checks +- `make precommit`: **PASS** (install, format, test 7/7, lint, typecheck, audit, trivy all clean) +- ast-grep mechanical funnel (74 YAML rules, diff-scoped): **0 findings** +- LICENSE file: present +- Judgment-rule candidates triggered by diff: all reviewed manually below (node-quality-assistant owns 22 of them) + +### Manual Review + +Read every changed `.ts` source in full (not diff fragments) plus `tsconfig.json`, `eslint.config.js`, `Makefile.precommit`. Cross-checked against `node-service-guide.md`'s TypeScript section and the repo's own `CLAUDE.md` invariants — the PR matches both exactly: +- No `any`, `as`, `!`, or `@ts-ignore` anywhere in source (verified by grep) +- `tsc --noEmit` wired into `make check` as `typecheck` +- `erasableSyntaxOnly` + no build step preserved; `require('./x.ts')` explicit-extension pattern used consistently +- All prior non-obvious invariants (readiness-before-close, 503 not 500, unref'd timer, stderr/stdout split, unmatched-label bucketing, error-handler arity) preserved verbatim in the ported code +- New `config.check()` validator is explicitly exempted by `node/config/data-not-behaviour` (a `check()` validator on config data is allowed) — not a violation despite adding a method to the config object +- CHANGELOG entry uses conventional `feat:` prefix correctly + +#### Must Fix (Critical) +None. + +#### Should Fix (Important) +- **No test coverage for `src/config.ts`'s new validation logic.** `config.check()` is new: it collects multiple problems instead of throwing on the first (previously only `PORT` was validated via immediate `throw`), and it adds a brand-new `SHUTDOWN_TIMEOUT_MS` check that didn't exist before. Zero tests exercise either the multi-problem collection or the new field validation — `test/` only covers `health.ts` via `server.ts`. Worth a small `test/config.test.ts` covering: valid config → `[]`, invalid `PORT` → problem, invalid `SHUTDOWN_TIMEOUT_MS` → problem, both invalid → both problems. + +#### Nice to Have (Optional) +None. + +### Traceability +| Rule ID | Owner | Verdict | +|---|---|---| +| node/config/data-not-behaviour | node-quality-assistant | compliant (check() exempted) | +| node/config/validate-before-serving | node-quality-assistant | compliant, improved (explicit check() before listen) | +| node/architecture/inject-dependencies | node-quality-assistant | compliant, unchanged | +| node/health/liveness-has-no-dependencies | node-quality-assistant | compliant, unchanged | +| node/health/readiness-returns-503 | node-quality-assistant | compliant, unchanged | +| node/http/error-handler-arity | node-quality-assistant | compliant, unchanged | +| node/k8s/scrape-annotation-matches-metrics | node-quality-assistant | not touched (k8s/ unchanged) | +| node/lifecycle/handles-sigterm | node-quality-assistant | compliant, unchanged | +| node/lifecycle/readiness-fails-before-close | node-quality-assistant | compliant, unchanged | +| node/lifecycle/shutdown-timer-unref | node-quality-assistant | compliant, unchanged | +| node/logging/errors-to-stderr | node-quality-assistant | compliant, unchanged | +| node/make/install-from-lockfile | node-quality-assistant | compliant, unchanged | +| node/make/required-targets | node-quality-assistant | compliant | +| node/make/security-gates-in-check | node-quality-assistant | compliant | +| node/make/typecheck-for-typescript | node-quality-assistant | compliant (this rule's target, added by the PR) | +| node/metrics/bounded-label-cardinality | node-quality-assistant | compliant, unchanged | +| node/metrics/own-registry | node-quality-assistant | compliant, unchanged | +| node/metrics/service-exposes-metrics | node-quality-assistant | compliant, unchanged | +| node/test/ephemeral-port | node-quality-assistant | compliant, unchanged | +| claude-md/agent-context-not-user-docs | agent-auditor | compliant | +| readme/user-facing-not-agent-context | agent-auditor | compliant | +| changelog/conventional-prefix-required | agent-auditor | compliant | + +### Next Steps +Add `test/config.test.ts` for the new `check()` validation paths, otherwise ready to merge. From f2d6655ec4cef44bd3099f5b14cc611d7a1ab22c Mon Sep 17 00:00:00 2001 From: Benjamin Borbe Date: Sat, 8 Aug 2026 14:29:41 +0200 Subject: [PATCH 2/6] 023-spec-005-ordered-items-and-body-fidelity --- CHANGELOG.md | 6 + bench/run.py | 73 +++- bench/test_review.py | 370 +++++++++++++++++- ...22-spec-005-harvest-section-terminators.md | 248 ++++++++++++ ...pec-005-ordered-items-and-body-fidelity.md | 263 +++++++++++++ ...c-005-attribution-from-reviewer-markers.md | 290 ++++++++++++++ ...-spec-005-unattributable-item-rejection.md | 298 ++++++++++++++ ...005-harvest-contract-docs-and-changelog.md | 240 ++++++++++++ ...05-bug-bench-harvest-finding-extraction.md | 351 +++++++++++++++++ 9 files changed, 2116 insertions(+), 23 deletions(-) create mode 100644 prompts/cancelled/022-spec-005-harvest-section-terminators.md create mode 100644 prompts/completed/023-spec-005-ordered-items-and-body-fidelity.md create mode 100644 prompts/in-progress/024-spec-005-attribution-from-reviewer-markers.md create mode 100644 prompts/in-progress/025-spec-005-unattributable-item-rejection.md create mode 100644 prompts/in-progress/026-spec-005-harvest-contract-docs-and-changelog.md create mode 100644 specs/in-progress/005-bug-bench-harvest-finding-extraction.md diff --git a/CHANGELOG.md b/CHANGELOG.md index d829d6a..c3ca46d 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 — ordered list items (`1.`, `2.`, `10.`) now open findings exactly as unordered items do; items inside fenced code blocks are ignored +- fix: bench runner — item-body normaliser no longer strips one asterisk from a leading bold run, preserving `**path**` references in finding text +- test: bench — add 5 new tests covering ordered/unordered list recognition, fenced-item suppression, and leading-bold-run preservation; add sha256 digest verification for 4 verbatim capture fixtures + ## v0.35.3 - fix: bench runner — the start-up check now hashes the directory Claude Code actually loads the coding plugin from, taken from the isolated config directory's install record, and aborts the whole run by name when no record exists, when it cannot be parsed, when it names a directory missing on disk, when it points outside that config directory's own cache tree, or when it applies only to a different working directory; a passing check prints the resolved directory, the recorded version and the content hash diff --git a/bench/run.py b/bench/run.py index 7522626..22fdbee 100755 --- a/bench/run.py +++ b/bench/run.py @@ -49,6 +49,8 @@ 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+(.+)$") +ORDERED_ITEM_RE = re.compile(r"^\s{0,3}\d+\.\s+(.+)$") +BOLD_RUN_START_RE = re.compile(r"^\s*\*\*") _SECTION_BY_LOWER = {name.lower(): name for name in REQUIRED_SECTION_NAMES} REQUIRED_ENTRY_FIELDS = ( "id", "owner", "repo", "number", @@ -1106,20 +1108,55 @@ def non_review_report(pr_id: str, missing: list[str], stdout_text: str) -> str: ) +def list_item_body(stripped_line: str) -> str | None: + """Return the item text when stripped_line opens a list item, else None. + + Both list styles the reviewer uses open a finding: an unordered item + (`-` or `*` followed by whitespace) and an ordered item (a run of digits + followed by `.` and whitespace). The marker is removed; nothing else about + the text is changed, so a leading bold run survives intact. + """ + m = BULLET_RE.match(stripped_line) + if m: + return m.group(2) + m = ORDERED_ITEM_RE.match(stripped_line) + if m: + return m.group(1) + return None + + def _normalize_body(lines: list[str]) -> str: - """Strip bullet marker and join continuation lines into one whitespace-collapsed string.""" - body = lines[0] - if body.startswith(("*", "-")): - body = body[1:].lstrip() - body = " ".join([body] + lines[1:]) - body = re.sub(r"\s+", " ", body).strip() - return body + """Join an item's lines into one whitespace-collapsed string. + + The list marker was already removed by list_item_body; nothing else is + stripped, so the item's leading bold run is preserved verbatim. + """ + body = " ".join(lines) + return re.sub(r"\s+", " ", body).strip() + +@dataclasses.dataclass +class HarvestResult: + """The two-part outcome of harvesting one review report. + + findings — items inside a severity section that carry an attribution. + unattributable — items inside a severity section that carry none. Reserved + here and always empty; populated by the unattributable-item + gate, which classifies items once attribution extraction + exists. It is a separate component precisely so a caller + can distinguish "nothing was found" from "something was + found and could not be keyed" (spec 005 AC3). + """ + findings: list + unattributable: list -def harvest(report_text: str, known_rule_ids: set) -> list: - """Normalize a /coding:pr-review Step 5 report into a list of findings. - Returns a list of dicts, each with keys: path, line, rule_id, body. +def harvest(report_text: str, known_rule_ids: set) -> HarvestResult: + """Normalize a /coding:pr-review Step 5 report into a HarvestResult. + + Returns a HarvestResult with two lists: findings (attributed items) and + unattributable (items with no path and no rule_id). Each finding dict + has keys: path, line, rule_id, body. """ findings: list = [] current_section: str | None = None @@ -1161,18 +1198,25 @@ def flush_finding(): current_finding_lines = [] continue + if BOLD_RUN_START_RE.match(line): + flush_finding() + current_section = None + current_finding_lines = [] + continue + if current_section is None: continue stripped = line.strip() - if stripped and BULLET_RE.match(stripped): + item = list_item_body(stripped) if (stripped and not in_fence) else None + if item is not None: flush_finding() - current_finding_lines = [BULLET_RE.match(stripped).group(2)] + current_finding_lines = [item] elif stripped and current_finding_lines: current_finding_lines.append(stripped) flush_finding() - return findings + return HarvestResult(findings=findings, unattributable=[]) # ---------------------------------------------------------------------- @@ -1407,7 +1451,8 @@ def process_pr(*, entry: dict, coding_repo: pathlib.Path, atomic_write_bytes(raw_path, proc.stdout.encode("utf-8")) # 6. Harvest findings - findings = harvest(proc.stdout, known_rule_ids) + harvested = harvest(proc.stdout, known_rule_ids) + findings = harvested.findings # 7. Build row and append to ledger review_command = shlex.join(argv) diff --git a/bench/test_review.py b/bench/test_review.py index 02ddbc8..9901056 100644 --- a/bench/test_review.py +++ b/bench/test_review.py @@ -3,10 +3,12 @@ import contextlib from contextlib import nullcontext +import hashlib import io import json import os import pathlib +import re import shutil import subprocess import sys @@ -379,7 +381,8 @@ class TestHarvestNormalizesSampleReport(unittest.TestCase): def test_harvest_normalizes_sample_report(self): text = (run.BENCH_DIR / "testdata" / "sample-report.md").read_text() known_ids = run.load_rule_ids(run.REPO_ROOT) - findings = run.harvest(text, known_ids) + result = run.harvest(text, known_ids) + findings = result.findings # Expected: 3 findings (Must Fix x2, Should Fix x1) # The Nice to Have section ("None.") and traceability section produce 0 findings @@ -418,7 +421,8 @@ def test_harvest_keeps_finding_without_any_rule_id(self): - This finding has no rule ID at all but should still be kept. """ ids = run.load_rule_ids(run.REPO_ROOT) - findings = run.harvest(report, ids) + result = run.harvest(report, ids) + findings = result.findings self.assertEqual(len(findings), 1) self.assertIsNone(findings[0]["rule_id"]) self.assertIn("no rule ID", findings[0]["body"]) @@ -432,7 +436,8 @@ def test_harvest_ignores_empty_section(self): None. """ ids = run.load_rule_ids(run.REPO_ROOT) - findings = run.harvest(report, ids) + result = run.harvest(report, ids) + findings = result.findings self.assertEqual(len(findings), 0) @@ -915,8 +920,11 @@ class TestRealCaptureHarvestsToZeroFindings(unittest.TestCase): 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) + result = run.harvest(text, ids) + findings = result.findings + unattributable = result.unattributable self.assertEqual(findings, [], f"real capture must harvest to zero findings, got: {findings}") + self.assertEqual(unattributable, [], f"real capture must harvest to zero unattributable, got: {unattributable}") class TestTrailingProseDoesNotSwallowARealFinding(unittest.TestCase): @@ -939,7 +947,8 @@ def test_trailing_prose_does_not_swallow_a_real_finding(self): f"**Summary:** This is the closing panel prose.\n" f"Some additional context about what was reviewed.\n" ) - findings = run.harvest(report, known_ids) + result = run.harvest(report, known_ids) + findings = result.findings self.assertEqual(len(findings), 1, f"expected exactly 1 finding, got: {findings}") f = findings[0] self.assertEqual(f["path"], "src/foo.go") @@ -969,7 +978,8 @@ def test_heading_level_does_not_change_harvest(self): ) reports = {level: report_template(level) for level in ("##", "###", "####")} - harvests = {level: run.harvest(text, known_ids) for level, text in reports.items()} + results = {level: run.harvest(text, known_ids) for level, text in reports.items()} + harvests = {level: r.findings for level, r in results.items()} self.assertEqual( harvests["##"], @@ -999,7 +1009,8 @@ def test_heading_section_name_rejects_prose_and_fence(self): 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) + result = run.harvest(report, known_ids) + findings = result.findings self.assertEqual(findings, [], f"fenced heading must not open a section, got: {findings}") @@ -1018,7 +1029,8 @@ def test_thematic_break_ends_a_section(self): 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) + result = run.harvest(report, known_ids) + findings = result.findings self.assertEqual(len(findings), 1, f"expected 1 finding, got: {findings}") self.assertNotIn("Summary", findings[0]["body"]) self.assertNotIn("trailing prose", findings[0]["body"]) @@ -1043,7 +1055,8 @@ def test_prose_before_a_list_item_opens_nothing(self): f"## Nice to Have (Optional)\n" f"None.\n" ) - findings = run.harvest(report, known_ids) + result = run.harvest(report, known_ids) + findings = result.findings self.assertEqual(len(findings), 1, f"expected 1 finding, got: {findings}") self.assertNotIn("None.", findings[0]["body"]) self.assertEqual(findings[0]["path"], "bar.go") @@ -1598,5 +1611,344 @@ def test_failure_artifact_is_not_a_cache_entry(self): f"no ledger rows for failed runs in {rd}") +class TestCaptureFixturesMatchPublishedDigests(unittest.TestCase): + """AC2: four operator-installed verbatim capture fixtures match their published digests.""" + + FIXTURES = [ + ("bench/testdata/capture-notes-block-h2.md", + "6427028bef301ff822cca6dbf9308896f1899ac5a972ed3fddc276f2216552b9", + 17), + ("bench/testdata/capture-numbered-findings-h3.md", + "5530049fa4d116dc5762b69c9c9498ff0865c0ae0c6b1de7b3ae4cc846643e93", + 28), + ("bench/testdata/capture-traceability-h4.md", + "2922746bb95bdb3a67a683942531362271d8f3ccd558067d910146e054bcfe7c", + 57), + ("bench/testdata/capture-summary-trailer-h4.md", + "36e15eca61133033d81687f87a82b044333c6a7465508d1757f8493361137e79", + 21), + ] + + def test_each_fixture_matches_its_published_sha256_and_line_count(self): + for rel_path, expected_sha256, expected_lines in self.FIXTURES: + with self.subTest(fixture=rel_path): + path = run.BENCH_DIR / rel_path.replace("bench/", "") + observed_sha256 = hashlib.sha256(path.read_bytes()).hexdigest() + observed_lines = len(path.read_text().splitlines()) + self.assertEqual( + observed_sha256, expected_sha256, + f"fixture {rel_path}: expected sha256 {expected_sha256}, got {observed_sha256}" + ) + self.assertEqual( + observed_lines, expected_lines, + f"fixture {rel_path}: expected {expected_lines} lines, got {observed_lines}" + ) + + +class TestNotesBlockCaptureHarvestsToNothing(unittest.TestCase): + """AC3: trailing notes block harvests to nothing at all.""" + + def test_notes_block_capture_harvests_to_empty_findings_and_empty_unattributable(self): + text = (run.BENCH_DIR / "testdata" / "capture-notes-block-h2.md").read_text() + ids = run.load_rule_ids(run.REPO_ROOT) + result = run.harvest(text, ids) + self.assertEqual( + result.findings, [], + f"notes-block capture must yield empty findings, got: {result.findings}" + ) + self.assertEqual( + result.unattributable, [], + f"notes-block capture must yield empty unattributable, got: {result.unattributable}" + ) + + +class TestBoldLabelTerminatorIsGeneral(unittest.TestCase): + """AC4: bold-run terminator is general, not a hardcoded label.""" + + def test_case_a_notes_block_with_three_bullets_yields_nothing(self): + report = ( + "## Must Fix (Critical)\n" + "None.\n" + "\n" + "**Notes:**\n" + "- precommit skipped\n" + "- npm ci was not run\n" + "- LICENSE file present\n" + ) + ids = run.load_rule_ids(run.REPO_ROOT) + result = run.harvest(report, ids) + self.assertEqual(result.findings, [], + f"Case A: expected empty findings, got: {result.findings}") + self.assertEqual(result.unattributable, [], + f"Case A: expected empty unattributable, got: {result.unattributable}") + + def test_case_b_summary_label_with_prose_and_bullet_yields_nothing(self): + report = ( + "## Must Fix (Critical)\n" + "None.\n" + "\n" + "**Summary:** This is a closing panel.\n" + "- precommit skipped\n" + ) + ids = run.load_rule_ids(run.REPO_ROOT) + result = run.harvest(report, ids) + self.assertEqual(result.findings, [], + f"Case B: expected empty findings, got: {result.findings}") + self.assertEqual(result.unattributable, [], + f"Case B: expected empty unattributable, got: {result.unattributable}") + + def test_case_c_one_real_finding_before_notes_block_yields_only_that_finding(self): + report = ( + "## Must Fix (Critical)\n" + "- **`src/foo.py:7`** the one real finding, which must survive.\n" + "\n" + "**Notes:**\n" + "- this bullet must not appear\n" + "- nor this one\n" + ) + ids = run.load_rule_ids(run.REPO_ROOT) + result = run.harvest(report, ids) + self.assertEqual(len(result.findings), 1, + f"Case C: expected exactly 1 finding, got: {result.findings}") + self.assertEqual(result.unattributable, [], + f"Case C: expected empty unattributable, got: {result.unattributable}") + self.assertEqual(result.findings[0]["path"], "src/foo.py") + self.assertEqual(result.findings[0]["line"], 7) + self.assertIn("the one real finding", result.findings[0]["body"]) + self.assertNotIn("this bullet must not appear", result.findings[0]["body"]) + self.assertNotIn("nor this one", result.findings[0]["body"]) + + +class TestContentOutsideASeveritySectionIsNeverAFinding(unittest.TestCase): + """AC10: content outside a severity section is never a finding and never unattributable.""" + + def test_case_a_numbered_findings_positive_notes_bullets_not_in_harvest(self): + # Substrings that occur exactly once in the ### Positive notes bullets: + # fixture line 23: "build-backend switch is clean" + # fixture line 24: "mktemp" + # fixture line 25: "S104" + # fixture line 26: "TestClient" + # Each was chosen because it occurs exactly once in the fixture and that + # one occurrence is inside a ### Positive notes bullet. "hatchling" and + # "pip-audit" are disqualified: "hatchling" appears twice (line 20 is a + # Nice to Have finding, line 23 is positive notes); "pip-audit" appears + # three times (lines 15, 24, 24). "ruff " appears twice (lines 5, 25). + text = (run.BENCH_DIR / "testdata" / "capture-numbered-findings-h3.md").read_text() + ids = run.load_rule_ids(run.REPO_ROOT) + result = run.harvest(text, ids) + + self.assertTrue( + result.findings or result.unattributable, + "corpus is non-degenerate: harvest returned something" + ) + + # All four positive-notes substrings must be absent from both components + positive_substrings = ( + "build-backend switch is clean", # line 23 + "mktemp", # line 24 + "S104", # line 25 + "TestClient", # line 26 + ) + for substr in positive_substrings: + for finding in result.findings: + self.assertNotIn( + substr, finding["body"], + f"substring {substr!r} must not appear in findings body" + ) + for item in result.unattributable: + self.assertNotIn( + substr, item.get("body", ""), + f"substring {substr!r} must not appear in unattributable body" + ) + + def test_case_b_traceability_table_rule_ids_not_in_harvest(self): + text = (run.BENCH_DIR / "testdata" / "capture-traceability-h4.md").read_text() + ids = run.load_rule_ids(run.REPO_ROOT) + result = run.harvest(text, ids) + + # Collect rule IDs from the traceability table (fixture lines 33-54, 22 rows) + table_ids = re.findall(r"^\| ([a-z][a-z0-9/-]+) \|", text, re.MULTILINE) + self.assertEqual( + len(table_ids), 22, + f"traceability table must have 22 rows, got: {len(table_ids)} — " + "check regex anchoring (re.MULTILINE required)" + ) + + for finding in result.findings: + self.assertNotIn( + finding.get("rule_id"), table_ids, + f"rule_id {finding.get('rule_id')!r} must not come from traceability table" + ) + + def test_case_c_both_zero_finding_captures_harvest_to_empty(self): + for rel_path in ( + "bench/testdata/real-capture-report.md", + "bench/testdata/capture-summary-trailer-h4.md", + ): + with self.subTest(fixture=rel_path): + text = (run.BENCH_DIR / rel_path.replace("bench/", "")).read_text() + ids = run.load_rule_ids(run.REPO_ROOT) + result = run.harvest(text, ids) + self.assertEqual( + result.findings, [], + f"{rel_path}: expected empty findings, got: {result.findings}" + ) + self.assertEqual( + result.unattributable, [], + f"{rel_path}: expected empty unattributable, got: {result.unattributable}" + ) + + +class TestHeadingLevelIsIrrelevantToTermination(unittest.TestCase): + """AC14: heading level is irrelevant to termination and harvesting.""" + + def test_heading_level_does_not_change_harvest_at_different_levels(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) + + # Render identical content at ##, ###, #### — each carrying one finding followed + # by a **Notes:** block and two more bullets. The finding must appear at every + # level, and all three harvests must be equal. + def make_report(level): + return ( + f"{level} Must Fix (Critical)\n" + f"- **`src/foo.py:7`** a finding that must appear at every level.\n" + f"\n" + f"**Notes:**\n" + f"- this bullet must not appear\n" + f"- nor this one\n" + ) + + results = {lvl: run.harvest(make_report(lvl), known_ids) for lvl in ("##", "###", "####")} + findings = {lvl: r.findings for lvl, r in results.items()} + + # Per-level assertions — each level must yield exactly one finding + for lvl in ("##", "###", "####"): + with self.subTest(level=lvl): + self.assertEqual(len(findings[lvl]), 1, + f"{lvl}: expected exactly 1 finding, got: {findings[lvl]}") + self.assertEqual(findings[lvl][0]["path"], "src/foo.py") + self.assertEqual(findings[lvl][0]["line"], 7) + self.assertEqual(results[lvl].unattributable, [], + f"{lvl}: expected empty unattributable, got: {results[lvl].unattributable}") + + # Three-way equality — empty results compare equal, so per-level assertions + # are required alongside this equality check + self.assertEqual( + findings["##"], findings["###"], + f"## vs ###: {findings['##']} vs {findings['###']}" + ) + self.assertEqual( + findings["##"], findings["####"], + f"## vs ####: {findings['##']} vs {findings['####']}" + ) + + def test_h3_terminates_open_h2_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 = ( + "## Must Fix (Critical)\n" + f"- `{real_rule_id}`: a real finding in file.go:99\n" + "### Some other heading\n" + "- this bullet must not appear\n" + ) + result = run.harvest(report, known_ids) + self.assertEqual(len(result.findings), 1, + f"expected exactly 1 finding, got: {result.findings}") + + def test_h2_terminates_open_h4_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 = ( + "#### Must Fix (Critical)\n" + f"- `{real_rule_id}`: a real finding in file.go:99\n" + "## Some other heading\n" + "- this bullet must not appear\n" + ) + result = run.harvest(report, known_ids) + self.assertEqual(len(result.findings), 1, + f"expected exactly 1 finding, got: {result.findings}") + + +class TestOrderedAndUnorderedItemsBothOpenFindings(unittest.TestCase): + """AC6: ordered and unordered list items both open findings, in document order.""" + + def test_ordered_and_unordered_items_yield_findings_in_document_order(self): + # Mixing unordered (-, *) and ordered (3., 4., 10.) styles. + # Ordered numbering starts at 3 and includes a two-digit marker. + # Every item carries a path in backticks so paths are extractable (prompt 3). + report = ( + "## Must Fix (Critical)\n" + "- **`a/one.py:1`** first item, unordered dash.\n" + "* **`a/two.py:2`** second item, unordered star.\n" + "3. **`a/three.py:3`** third item, ordered starting at three.\n" + "4. **`a/four.py:4`** fourth item.\n" + "10. **`a/ten.py:10`** fifth item, two-digit marker.\n" + ) + ids = run.load_rule_ids(run.REPO_ROOT) + result = run.harvest(report, ids) + findings = result.findings + + self.assertEqual( + len(findings), 5, + f"expected 5 findings, got {len(findings)}: {[f['body'] for f in findings]}" + ) + paths = [f["path"] for f in findings] + self.assertEqual( + paths, ["a/one.py", "a/two.py", "a/three.py", "a/four.py", "a/ten.py"], + f"paths must be in document order, got: {paths}" + ) + + def test_ordered_item_inside_fence_yields_nothing(self): + # An ordered item inside a fenced block is example text, not a finding. + # Both findings and unattributable must be empty. + report = ( + "## Should Fix (Important)\n" + "None.\n" + "\n" + "```\n" + "1. this ordered item is inside a fence and is not a finding\n" + "```\n" + ) + ids = run.load_rule_ids(run.REPO_ROOT) + result = run.harvest(report, ids) + self.assertEqual( + result.findings, [], + f"expected empty findings, got: {result.findings}" + ) + self.assertEqual( + result.unattributable, [], + f"expected empty unattributable (not an unattributable item), got: {result.unattributable}" + ) + + +class TestBodyPreservesLeadingBoldRun(unittest.TestCase): + """AC9: the list-item body preserves a leading bold run verbatim.""" + + def test_traceability_capture_bold_run_survives_normalization(self): + text = (run.BENCH_DIR / "testdata" / "capture-traceability-h4.md").read_text() + ids = run.load_rule_ids(run.REPO_ROOT) + result = run.harvest(text, ids) + + self.assertEqual( + len(result.findings), 1, + f"expected 1 finding, got {len(result.findings)}: {[f['body'] for f in result.findings]}" + ) + observed_body = result.findings[0]["body"] + self.assertTrue( + observed_body.startswith("**No test coverage for"), + f"leading bold run was mangled, got: {observed_body[:60]!r}" + ) + self.assertEqual( + result.unattributable, [], + f"expected empty unattributable, got: {result.unattributable}" + ) + + if __name__ == "__main__": unittest.main() diff --git a/prompts/cancelled/022-spec-005-harvest-section-terminators.md b/prompts/cancelled/022-spec-005-harvest-section-terminators.md new file mode 100644 index 0000000..3a3b9b9 --- /dev/null +++ b/prompts/cancelled/022-spec-005-harvest-section-terminators.md @@ -0,0 +1,248 @@ +--- +status: cancelled +spec: [005-bug-bench-harvest-finding-extraction] +execution_id: coding-exec-022-spec-005-harvest-section-terminators +dark-factory-version: v0.192.9 +created: "2026-08-08T11:40:00Z" +queued: "2026-08-08T12:14:19Z" +started: "2026-08-08T12:24:41Z" +completed: "2026-08-08T12:23:22Z" +lastFailReason: 'validate completion report: completion report status: partial' +cancelled: "2026-08-08T12:24:55Z" +--- + + +- The benchmark stops counting a reviewer's trailing housekeeping notes as review findings +- A review that correctly found nothing now records nothing, instead of recording the three bullets of its closing notes block +- The rule that ends a findings section is generalised: any bold-label block starts something new, whatever that label says +- Two more shapes the reviewer really emits — a `**Notes:**` block and a `**Summary**:` block — stop leaking into the counts +- Content the reviewer placed outside a findings section is neither counted as a finding nor treated as an error +- Harvesting now returns two things instead of one: the findings, and a report of items that could not be attributed (empty for now, filled in by a later prompt) +- Four verbatim captures of real review output are checked against their published fingerprints before any test is written against them +- If a capture does not match its fingerprint the work stops and says so, rather than rewriting the capture to fit +- Heading depth is confirmed irrelevant: the same review content at three different heading depths produces the same result +- First of five prompts; it fixes only where a section ends, and deliberately touches nothing else + + + +Give a findings section a terminator that matches what the reviewer actually writes — a line whose first non-whitespace content opens a bold run ends the section, exactly as a heading or a thematic break already does — so a trailing `**Notes:**` or `**Summary**:` block can no longer be harvested as findings. Change `harvest` to return a two-part result (findings plus a reserved unattributable-item report) so the later prompts of this spec have somewhere to put a parse failure, and lock all of it against four operator-installed verbatim captures of live review output. + + + +Read `CLAUDE.md` for project conventions (Python 3 standard library only, no personal paths, generic examples only, never commit — dark-factory handles git). + +Read `specs/in-progress/005-bug-bench-harvest-finding-extraction.md`. This prompt satisfies **Desired Behavior 1** and the fixtures half of **Desired Behavior 6**, and **Acceptance Criteria AC2, AC3, AC4, AC10 and AC14**. Load-bearing sections to read in full: `## Reproduction` → `### RC1`, `### RC4`, `## Acceptance Criteria` AC2/AC3/AC4/AC10/AC14, `## Constraints`, `## Assumptions`. + +Read `bench/run.py`. The code you change lives between the `# Harvesting` banner and the `# Result row assembly` banner. Specifically read, and take every signature from the file rather than from this prompt: the module constants block (`HEADING_RE`, `THEMATIC_BREAK_RE`, `FENCE_RE`, `BULLET_RE`, `REQUIRED_SECTION_NAMES`), `heading_section_name`, `iter_report_lines`, `missing_sections`, `_normalize_body`, `harvest`, and the single call site `findings = harvest(proc.stdout, known_rule_ids)` in `process_pr` (step 6, immediately after the step-5 raw-stdout write). + +Read `bench/test_review.py`. Every existing call site of `run.harvest(...)` must keep working after the return type changes. They are in these classes: `TestHarvestNormalizesSampleReport`, `TestHarvestKeepsFindingWithoutAnyRuleId`, `TestHarvestIgnoresEmptySection`, `TestRealCaptureHarvestsToZeroFindings`, `TestTrailingProseDoesNotSwallowARealFinding`, `TestHeadingLevelDoesNotChangeHarvest`, `TestSectionNameInProseOrFenceIsNotAHeading`, `TestThematicBreakEndsASection`, `TestProseBeforeAListItemOpensNothing`. + +Read the four fixtures before writing any assertion against them — they are verbatim captures, not templates, and the exact text matters: +`bench/testdata/capture-notes-block-h2.md`, `bench/testdata/capture-numbered-findings-h3.md`, `bench/testdata/capture-traceability-h4.md`, `bench/testdata/capture-summary-trailer-h4.md`. + +Read `bench/testdata/sample-report.md` and `bench/testdata/real-capture-report.md` — both are byte-frozen and must still harvest to their previously asserted results. + +Read `docs/dod.md` for the repository's Definition of Done. + + + + +## 1. Verify the four fixtures before touching any code + +The four capture fixtures were installed by the operator before this spec was approved. **You never create, regenerate, overwrite, reconstruct or edit a file under `bench/testdata/`.** Verify them first: + +```bash +for f in bench/testdata/capture-notes-block-h2.md \ + bench/testdata/capture-numbered-findings-h3.md \ + bench/testdata/capture-traceability-h4.md \ + bench/testdata/capture-summary-trailer-h4.md; do + printf '%s %s %s\n' "$f" "$(shasum -a 256 "$f" | cut -d' ' -f1)" "$(grep -c '' "$f")" +done +``` + +Expected exactly: + +| File | `sha256` | Lines | Heading level | +|---|---|---|---| +| `bench/testdata/capture-notes-block-h2.md` | `6427028bef301ff822cca6dbf9308896f1899ac5a972ed3fddc276f2216552b9` | 17 | `##` | +| `bench/testdata/capture-numbered-findings-h3.md` | `5530049fa4d116dc5762b69c9c9498ff0865c0ae0c6b1de7b3ae4cc846643e93` | 28 | `###` | +| `bench/testdata/capture-traceability-h4.md` | `2922746bb95bdb3a67a683942531362271d8f3ccd558067d910146e054bcfe7c` | 57 | `####` | +| `bench/testdata/capture-summary-trailer-h4.md` | `36e15eca61133033d81687f87a82b044333c6a7465508d1757f8493361137e79` | 21 | `####` | + +Also verify, per file, that `grep -cE '^ (Must Fix|Should Fix|Nice to Have)' ` returns `3` at the stated level and `0` at each of the other two levels, and that `grep -rn '/Users/\|~/Documents/' bench/testdata/` returns no lines. + +If any file is missing, or any digest, line count or heading count differs, **stop immediately and report `status: failed`** with the message `"fixture : expected sha256 , got "`. Do not write the file, do not adjust the expectation, do not continue with the rest of this prompt. + +## 2. Add a machine-checkable fixture-digest test + +In `bench/test_review.py`, add `class TestCaptureFixturesMatchPublishedDigests(unittest.TestCase)` with one test method that, for each of the four files, computes `hashlib.sha256(path.read_bytes()).hexdigest()` and asserts it equals the published digest above, and asserts the line count. Import `hashlib` at the top of the file if it is not already imported. The failure message must print the fixture name, the expected digest and the observed digest. + +This test is the AC2 gate: it makes a silently-rewritten fixture a red test rather than a green one. + +## 3. Introduce the two-part harvest result + +In `bench/run.py`, immediately above `def harvest(...)`, add: + +```python +@dataclasses.dataclass +class HarvestResult: + """The two-part outcome of harvesting one review report. + + findings — items inside a severity section that carry an attribution. + unattributable — items inside a severity section that carry none. Reserved + here and always empty; populated by the unattributable-item + gate, which classifies items once attribution extraction + exists. It is a separate component precisely so a caller + can distinguish "nothing was found" from "something was + found and could not be keyed" (spec 005 AC3). + """ + findings: list + unattributable: list +``` + +Use `@dataclasses.dataclass`, matching `PrCheckout` and `PluginResolution` in the same file. `dataclasses` is already imported; add no new import. + +Change `harvest`'s signature to `def harvest(report_text: str, known_rule_ids: set) -> HarvestResult:` and its final statement to `return HarvestResult(findings=findings, unattributable=[])`. Keep the parameter list unchanged. Update the docstring to describe both components. + +Update the call site in `process_pr` (step 6) to: + +```python + # 6. Harvest findings + harvested = harvest(proc.stdout, known_rule_ids) + findings = harvested.findings +``` + +Nothing else in `process_pr` changes in this prompt. + +Update every existing `run.harvest(...)` call site in `bench/test_review.py` to read `.findings` — for example `findings = run.harvest(text, ids).findings`. Do not delete or weaken any existing assertion; this is a call-shape change only. + +## 4. Add the bold-run section terminator + +Add a module constant next to `BULLET_RE` in the constants block: + +```python +BOLD_RUN_START_RE = re.compile(r"^\s*\*\*") +``` + +In `harvest`, inside the existing `if not in_fence:` block, add a branch **after** the `THEMATIC_BREAK_RE` branch and before the `if current_section is None:` guard: + +```python + if BOLD_RUN_START_RE.match(line): + flush_finding() + current_section = None + current_finding_lines = [] + continue +``` + +Ordering is load-bearing and must not be changed: `HEADING_RE` first, `THEMATIC_BREAK_RE` second, `BOLD_RUN_START_RE` third. A thematic break written as `***` also matches `BOLD_RUN_START_RE`, and it must be consumed as a thematic break. The branch sits inside the `if not in_fence:` block so a `**bold**` line inside a fenced code block terminates nothing. + +`BOLD_RUN_START_RE` must not match a list item. `BULLET_RE` requires `[-*]` followed by whitespace, so `* **emphasised**` is a bullet (single `*` then a space) while `**Notes:**` is a bold run (two adjacent asterisks). Do not widen the pattern. Do not narrow it either. A finding's continuation paragraph that begins with a bold run will truncate that finding's body and close the section early — the spec's Failure Modes table names this shape, accepts it (the failure direction is loss, never fabrication), and records that no captured output exhibits it. Do not add a lookahead, a continuation exemption, or any heuristic to avoid it. + +## 5. Tests for the bold-run terminator and the captures + +Add these to `bench/test_review.py`. Every assertion below compares **both** components of the `HarvestResult`, and every failure message prints both lists. + +**AC3** — `class TestNotesBlockCaptureHarvestsToNothing`: read `bench/testdata/capture-notes-block-h2.md`, harvest it, assert `result.findings == []` **and** `result.unattributable == []`. The failure message must print both. Asserting only the findings list would let an implementation reclassify the three `**Notes:**` bullets as unattributable items, which fails the PR loudly on a review that correctly found nothing — RC1 with the sign flipped. + +**AC4** — `class TestBoldLabelTerminatorIsGeneral`, three separate test methods over synthetic reports: + +- Case A — a section reading `None.`, then `**Notes:**`, then three bullets → `findings == []` and `unattributable == []`. +- Case B — a section reading `None.`, then `**Summary**:` followed by prose on the same line, then a bullet on a later line → `findings == []` and `unattributable == []`. This is the shape `bench/testdata/capture-summary-trailer-h4.md` actually uses; matching the literal string `**Notes:**` passes Case A alone. +- Case C — a section carrying one real attributed bullet, then `**Notes:**`, then two more bullets → exactly one finding. Use this bullet verbatim so it stays attributable under the later prompts of this spec: + + ``` + - **`src/foo.py:7`** the one real finding, which must survive. + ``` + + Assert `len(result.findings) == 1`, `result.unattributable == []`, `result.findings[0]["path"] == "src/foo.py"`, `result.findings[0]["line"] == 7`, `"the one real finding" in result.findings[0]["body"]`, and that the body contains none of the text of the two trailing bullets. Do **not** assert on the leading `**` of the body in this prompt — body fidelity is prompt 2's requirement. + +**AC10** — `class TestContentOutsideASeveritySectionIsNeverAFinding`, three test methods: + +- Case A — harvest `bench/testdata/capture-numbered-findings-h3.md` and assert that none of the four `### Positive notes` bullets appears in `result.findings` or in `result.unattributable`. Assert it by text: for each of the four distinctive substrings `"build-backend switch is clean"`, `"mktemp"`, `"S104"` and `"TestClient"`, assert it appears in no `body` of either list. **First assert the corpus is non-degenerate** — `self.assertTrue(result.findings or result.unattributable)` — because an absence-only probe over two empty lists passes even if harvesting broke completely. Do not assert an exact count: this fixture's harvested count changes at prompts 2 and 4. Content outside a severity section is not a finding and is not a parse failure either. + + **Do not substitute these four substrings.** Each was chosen because it occurs **exactly once** in the fixture and that one occurrence is inside a `### Positive notes` bullet (fixture lines 23, 24, 25, 26 respectively) — so the probe can only ever fail for the reason it is testing. Record that constraint as a comment directly above the substring tuple in the test source, naming the fixture line numbers, so a later prompt does not swap in a colliding token. Two obvious-looking alternatives are already disqualified and must not be reintroduced: + - `"hatchling"` occurs twice — fixture line 20 is a **Nice to Have bullet that is a legitimate finding today**, so the probe is red on arrival. + - `"pip-audit"` occurs three times, on two lines (line 15 once, line 24 twice) — fixture line 15 is inside **Should Fix item 4**, invisible to the parser now but harvested as a finding once prompt 2 lands ordered items, so the probe would go red one prompt later with nobody remembering why. + - `"ruff "` occurs twice (lines 5 and 25); the line 5 hit is pre-section prose that is never harvested, so it is *safe* but not *exactly once* — `"S104"` is used instead to keep one uniform rule for all four. +- Case B — harvest `bench/testdata/capture-traceability-h4.md` and assert no finding carries a `rule_id` drawn from the file's `### Traceability` table. Assert it by reading the fixture and collecting every value in the table's first column — match line by line, or use `re.findall(r"^\| ([a-z][a-z0-9/-]+) \|", text, re.MULTILINE)`; **without `re.MULTILINE` the pattern anchors to the start of the whole string and returns nothing** (verified: 0 matches without the flag, 22 with it). **First assert `len(table_ids) == 22`** (the table has 22 rows, fixture lines 33-54), then assert no harvested `rule_id` is a member of that set. Without the cardinality assertion a mis-anchored regex yields an empty set and the membership check becomes a test that can never fail — a new variant of this project's recurring family: not an assertion whose *expected value* the implementation chooses, but one whose *input corpus* silently empties. +- Case C — both zero-finding captures harvest to an empty findings list and an empty unattributable list: `bench/testdata/real-capture-report.md` (pre-existing, frozen) and `bench/testdata/capture-summary-trailer-h4.md`. + +**AC14** — `class TestHeadingLevelIsIrrelevantToTermination` with a method whose name contains `heading_level`: + +- Render identical section content at `##`, `###` and `####` and assert all three harvest to equal `HarvestResult`s. **Equality alone is vacuous — three empty results compare equal**, so an implementation that harvests nothing at any level would pass a bare equality assertion. The rendered content must therefore carry a real finding, and each level must be asserted on its own as well as against the others. + + Use this exact Must Fix body at every level, followed by a `**Notes:**` block and two further bullets: + + ``` + - **`src/foo.py:7`** a finding that must appear at every level. + ``` + + Assert the three-way equality **and**, separately for each of the three levels: `len(result.findings) == 1`, `result.findings[0]["path"] == "src/foo.py"`, `result.findings[0]["line"] == 7`, and `result.unattributable == []`. The per-level assertions are what make the equality meaningful; do not drop them in favour of the equality check alone. + + The existing `test_heading_level_does_not_change_harvest` stays; this is an additional test that also covers termination. +- Assert a `###` heading terminates an open `##` section: a `## Must Fix (Critical)` section carrying one attributed bullet, followed by `### Some other heading` and two more bullets, yields exactly one finding. +- Assert a `##` heading terminates an open `####` section: the same shape with the levels swapped, yields exactly one finding. +- The failure messages print all lists. + +## 6. Keep the frozen fixtures green + +After the change, `bench/testdata/sample-report.md` must still harvest to its three previously asserted findings and `bench/testdata/real-capture-report.md` to zero. Neither file may be edited. If either test goes red, the terminator is wrong — fix the terminator, never the fixture and never the assertion. + +## 7. Failure handling + +`harvest` remains a pure function over text: it never raises for malformed input, never opens a file, never invokes a subprocess, and stays linear in input size with line-oriented matching only. A pathological report yields a wrong section boundary at worst, never a hang. `process_pr`'s existing error paths are unchanged in this prompt. + + + + +- **Python 3 standard library only.** No third-party imports, no new top-level files outside `bench/`. Changes land only in `bench/run.py` and `bench/test_review.py` in this prompt. +- **Never create, regenerate, overwrite or edit any file under `bench/testdata/`.** A transcribed or regenerated fixture is the exact defect this spec exists to close. On a digest mismatch, stop and report failed. +- **`bench/testdata/sample-report.md` and `bench/testdata/real-capture-report.md` are byte-frozen** and must still harvest to their previously asserted results. +- **No test function may be deleted and no assertion relaxed.** Per-file assertion floors that must hold after this prompt: `grep -cE '^\s*(self\.assert|assert )' bench/test_config.py` ≥ 63, `bench/test_resolve.py` ≥ 46, `bench/test_review.py` ≥ 165. The suite's test count must stay strictly greater than 72. +- **Do NOT change the `NOT A REVIEW` gate** — same required sections, same heading matching, same bounded excerpt, same position ahead of the raw-output cache write. +- **Do NOT change `bench/prs.json`, `commands/pr-review.md`, or any rule, agent, command or doc that participates in a review.** +- **Do NOT make the terminators configurable** and do NOT add any flag, env var or opt-out that relaxes them. They are frozen invariants. +- **Do NOT touch the raw-stdout-before-parsing ordering** in `process_pr` (step 5 stays ahead of step 6). +- **Harvested values are data, never paths.** No value read out of review output is opened, stat-ed, joined onto a filesystem root, or passed to a subprocess. +- No personal paths (`/Users/`, `~/Documents/`) in any shipped file. +- Bench tests must not require network access, a real `claude` binary, or GitHub access. +- Do not read from or write to `bench/.cache/` or `bench/results/`. +- Do NOT edit `bench/README.md` or `CHANGELOG.md` in this prompt — prompt 5 of this spec owns both (spec 005 AC16/AC17). `docs/dod.md`'s "CHANGELOG.md has an entry under `## Unreleased`" criterion is deliberately deferred to prompt 5 and its absence here is **expected — do NOT report it as a blocker** and do NOT add an entry to satisfy it. +- Do NOT commit — dark-factory handles git. + + + +``` +# Fixture provenance — the four digests and line counts (AC2) +for f in bench/testdata/capture-notes-block-h2.md \ + bench/testdata/capture-numbered-findings-h3.md \ + bench/testdata/capture-traceability-h4.md \ + bench/testdata/capture-summary-trailer-h4.md; do + printf '%s %s %s\n' "$f" "$(shasum -a 256 "$f" | cut -d' ' -f1)" "$(grep -c '' "$f")" +done +grep -rn '/Users/\|~/Documents/' bench/testdata/ ; echo "exit=$? (expect 1)" + +# Section-heading anchors, per fixture +grep -cE '^## (Must Fix|Should Fix|Nice to Have)' bench/testdata/capture-notes-block-h2.md # expect 3 +grep -cE '^### (Must Fix|Should Fix|Nice to Have)' bench/testdata/capture-numbered-findings-h3.md # expect 3 +grep -cE '^#### (Must Fix|Should Fix|Nice to Have)' bench/testdata/capture-traceability-h4.md # expect 3 +grep -cE '^#### (Must Fix|Should Fix|Nice to Have)' bench/testdata/capture-summary-trailer-h4.md # expect 3 + +# The new constant and the two-part result exist +grep -n 'BOLD_RUN_START_RE\|class HarvestResult\|def harvest' bench/run.py + +# Assertion floors (AC15) and suite size (AC1) +grep -cE '^\s*(self\.assert|assert )' bench/test_config.py # expect >= 63 +grep -cE '^\s*(self\.assert|assert )' bench/test_resolve.py # expect >= 46 +grep -cE '^\s*(self\.assert|assert )' bench/test_review.py # expect >= 165 + +# Full suite, named tests visible +python3 -m unittest discover -s bench -p 'test_*.py' -v 2>&1 | tail -40 + +# Repository gate +make precommit +``` + +Expected: the four digests equal the published values and the line counts print `17`, `28`, `57`, `21`; the personal-path grep exits 1 with no output; each heading grep prints `3`; the unittest run reports `OK` with `Ran N tests` where `N > 72` and lists the new notes-block, bold-label, outside-section and `heading_level` tests by name; `make precommit` exits 0. + + diff --git a/prompts/completed/023-spec-005-ordered-items-and-body-fidelity.md b/prompts/completed/023-spec-005-ordered-items-and-body-fidelity.md new file mode 100644 index 0000000..d9a6deb --- /dev/null +++ b/prompts/completed/023-spec-005-ordered-items-and-body-fidelity.md @@ -0,0 +1,263 @@ +--- +status: completed +spec: [005-bug-bench-harvest-finding-extraction] +summary: Ordered list items now open findings, fenced code block items are suppressed, and leading bold run in body is preserved +execution_id: coding-exec-023-spec-005-ordered-items-and-body-fidelity +dark-factory-version: v0.192.9 +created: "2026-08-08T11:41:00Z" +queued: "2026-08-08T12:14:19Z" +started: "2026-08-08T12:24:56Z" +completed: "2026-08-08T12:29:41Z" +--- + + +- The benchmark now sees the findings the reviewer writes as a numbered list, which it previously discarded entirely +- These are the reviewer's most severe tier, so the instrument was losing precisely the findings that matter most +- Numbering does not have to start at one and is not limited to single digits — a list starting at three and running to ten counts every item +- Bulleted and numbered items are treated identically; both open one finding per item, in the order the reviewer wrote them +- A list written inside a fenced code block is example text, not a finding, and is now ignored as such +- A finding's text keeps the reviewer's emphasis intact — the leading bold run is no longer half-eaten as though it were a second list marker +- That corruption mattered because it chewed exactly the position where the file reference lives +- One real capture carrying a bold-headed finding is locked down as a regression fixture +- Second of five prompts; it changes only what opens a finding and what its text looks like +- Attribution — where the path, line and rule id come from — is deliberately left to the next prompt + + + +Make an ordered list item open a finding exactly as an unordered one does, suppress list items inside fenced code blocks, and stop the body normaliser from stripping an asterisk off the item's leading bold run. After this prompt the five numbered Should Fix items in the real capture are visible to the harvester as five findings, in document order, and the one bold-headed finding in the traceability capture keeps its `**` intact. + + + +Read `CLAUDE.md` for project conventions (Python 3 standard library only, no personal paths, generic examples only, never commit — dark-factory handles git). + +Read `specs/in-progress/005-bug-bench-harvest-finding-extraction.md`. This prompt satisfies **Desired Behavior 2** and **Acceptance Criteria AC6 and AC9**. Load-bearing sections: `### RC2 — numbered items are invisible`, `### Bonus defect, same layer`, `## Acceptance Criteria` AC6/AC9, `## Constraints`. + +**This prompt depends on prompt 1 of this spec having landed.** Verify before you start: + +```bash +grep -n 'BOLD_RUN_START_RE\|class HarvestResult' bench/run.py +``` + +If either is absent, stop and report `status: failed` with the message `"prompt 1 of spec 005 not yet landed"`. Do not implement prompt 1's changes here. + +Read `bench/run.py` — specifically the constants block (`BULLET_RE`, `FENCE_RE`, `BOLD_RUN_START_RE`), `iter_report_lines`, `_normalize_body`, `HarvestResult` and `harvest`. Take every signature from the file. + +Read `bench/testdata/capture-numbered-findings-h3.md` in full. Its `### Should Fix (Important)` section carries five items numbered `1.` through `5.`; its `### Nice to Have (Optional)` section carries two `- ` bullets; its `### Positive notes` heading is not a severity section. + +Read `bench/testdata/capture-traceability-h4.md` in full. Its `#### Should Fix (Important)` section carries exactly one item that begins `- **No test coverage for \`src/config.ts\`'s new validation logic.**`. Under the shipped parser the harvested body begins `*No test coverage for` — one asterisk short. + +Read `bench/test_review.py` for the existing test style, in particular `TestHarvestNormalizesSampleReport` and `TestSectionNameInProseOrFenceIsNotAHeading`. + +Read `docs/dod.md` for the repository's Definition of Done. + + + + +## 1. Re-verify the fixtures you assert against + +Before writing any test, confirm the two fixtures this prompt reads are still the operator-installed captures: + +```bash +shasum -a 256 bench/testdata/capture-numbered-findings-h3.md | cut -d' ' -f1 # expect 5530049fa4d116dc5762b69c9c9498ff0865c0ae0c6b1de7b3ae4cc846643e93 +shasum -a 256 bench/testdata/capture-traceability-h4.md | cut -d' ' -f1 # expect 2922746bb95bdb3a67a683942531362271d8f3ccd558067d910146e054bcfe7c +``` + +On any mismatch, stop and report `status: failed` with the observed digest. **Never write, regenerate or edit a file under `bench/testdata/`.** + +## 2. Recognise ordered list items + +Add a module constant next to `BULLET_RE`: + +```python +ORDERED_ITEM_RE = re.compile(r"^\s{0,3}\d+\.\s+(.+)$") +``` + +A run of one or more digits followed by `.` and at least one space opens a finding. The numbering need not start at 1 and is not limited to one digit — `3.` and `10.` both open findings. + +Add a helper immediately above `_normalize_body`: + +```python +def list_item_body(stripped_line: str) -> str | None: + """Return the item text when stripped_line opens a list item, else None. + + Both list styles the reviewer uses open a finding: an unordered item + (`-` or `*` followed by whitespace) and an ordered item (a run of digits + followed by `.` and whitespace). The marker is removed; nothing else about + the text is changed, so a leading bold run survives intact. + """ + m = BULLET_RE.match(stripped_line) + if m: + return m.group(2) + m = ORDERED_ITEM_RE.match(stripped_line) + if m: + return m.group(1) + return None +``` + +In `harvest`, replace the two-step `if stripped and BULLET_RE.match(stripped): ... current_finding_lines = [BULLET_RE.match(stripped).group(2)]` with a single call to `list_item_body(stripped)`, opening a finding when it returns a non-`None` value. Do not call the regex twice. + +`ORDERED_ITEM_RE` must not be reachable for a line inside a fenced block — see requirement 3. + +## 3. Suppress list items inside fenced code blocks + +`iter_report_lines` already yields `in_fence` for every line, but the item-matching tail of `harvest` runs outside the `if not in_fence:` guard, so today a list item inside a fenced code block opens a finding. Fix it: when `in_fence` is true, a line is ordinary text — it opens nothing. It may still extend a finding that is already open (a fenced snippet inside a finding is part of that finding's text), so the continuation branch stays reachable. + +Concretely, the item branch becomes conditional on `not in_fence`: + +```python + item = list_item_body(stripped) if (stripped and not in_fence) else None + if item is not None: + flush_finding() + current_finding_lines = [item] + elif stripped and current_finding_lines: + current_finding_lines.append(stripped) +``` + +The existing `test_fence_contains_heading_not_a_section` must stay green. + +## 4. Preserve the leading bold run in the body + +`_normalize_body` receives item text from which the list marker has **already** been removed, then strips one more leading `*` or `-`: + +```python + body = lines[0] + if body.startswith(("*", "-")): + body = body[1:].lstrip() +``` + +That second strip is the bonus defect: on `**No test coverage for …` it eats one asterisk of the emphasis, corrupting the body at exactly the position where the path reference lives. Delete those three lines. `_normalize_body` now only joins the item's lines and collapses runs of whitespace: + +```python +def _normalize_body(lines: list[str]) -> str: + """Join an item's lines into one whitespace-collapsed string. + + The list marker was already removed by list_item_body; nothing else is + stripped, so the item's leading bold run is preserved verbatim. + """ + body = " ".join(lines) + return re.sub(r"\s+", " ", body).strip() +``` + +The `None.` sentinel check in `flush_finding` compares `body.strip()` against `("None.", "None")` and is unaffected — leave it exactly as it is. + +## 5. Tests + +Add to `bench/test_review.py`. Every assertion compares against a `HarvestResult` and prints the observed lists on failure. + +**AC6 — ordered-item recognition is general, not fitted to the capture.** `class TestOrderedAndUnorderedItemsBothOpenFindings`, two test methods: + +- A synthetic severity section mixing both list styles, with ordered numbering that starts at `3.` and includes a two-digit marker. Use exactly this section body so every item stays attributable under the later prompts of this spec: + + ``` + ## Must Fix (Critical) + - **`a/one.py:1`** first item, unordered dash. + * **`a/two.py:2`** second item, unordered star. + 3. **`a/three.py:3`** third item, ordered starting at three. + 4. **`a/four.py:4`** fourth item. + 10. **`a/ten.py:10`** fifth item, two-digit marker. + ``` + + Assert exactly five findings, and assert the list of `path` values equals `["a/one.py", "a/two.py", "a/three.py", "a/four.py", "a/ten.py"]` — that is document order, and it is what proves a parser keyed to `1.`–`5.` does not pass. + +- A negative case: an ordered item inside a fenced code block within a severity section yields no finding. Build a report whose `## Should Fix (Important)` section reads `None.`, then a blank line, then a fenced block (three backticks) containing the line `1. this ordered item is inside a fence and is not a finding`, then the closing fence. Assert **both** `result.findings == []` **and** `result.unattributable == []`, with a failure message printing both lists. Asserting only `findings` lets an implementation classify the fenced line as an unattributable item instead of ignoring it — findings is still empty, and the PR would then fail loudly on example text inside a code block, which is the same sign-flipped failure prompt 1's AC3/AC4 guard against. + +**AC9 — the body preserves the leading bold run verbatim.** `class TestBodyPreservesLeadingBoldRun`, one test method that harvests `bench/testdata/capture-traceability-h4.md` and asserts: + +- exactly one finding, +- `result.findings[0]["body"].startswith("**No test coverage for")` — two asterisks, with a failure message printing the first 60 characters of the observed body, +- `result.unattributable == []`. + +Prompt 3 of this spec adds one further assertion to this same test method (`result.findings[0]["path"] == "src/config.ts"`); do not add it here, because the leading-bold-reference extraction it depends on does not exist yet. + +**Sibling-test re-anchor (required).** Prompt 1 added `TestContentOutsideASeveritySectionIsNeverAFinding`, whose Case A probes this same fixture by asserting four distinctive substrings appear in no harvested body. Once ordered items open findings, the fixture's harvest grows from 2 findings to 7, and a substring that sat inside a previously-invisible Should Fix item becomes reachable. If prompt 1's four substrings are `"build-backend switch is clean"`, `"mktemp"`, `"S104"` and `"TestClient"`, none of them moves and the probe stays green — verify that and move on. If you instead find a probe substring that has gone red (`"pip-audit"` at fixture line 15 is the known case, inside Should Fix item 4), **re-anchor that probe** to a substring that occurs exactly once in the fixture and only inside a `### Positive notes` bullet. + +Re-anchoring a probe substring is **not** "relaxing an assertion" under requirement 5's constraint: the assertion count is unchanged and the property under test — no positive-note content is ever harvested — is unchanged; only the token used to observe it moves off a collision. Do not delete the test, do not drop one of its cases, do not weaken it to a membership check, and do not edit the fixture. If you cannot find a non-colliding substring, stop and report rather than removing the case. + +**Regression guard.** `bench/testdata/sample-report.md` must still harvest to its three previously asserted findings and `bench/testdata/real-capture-report.md` to zero. Neither file may be edited. + +## 6. Failure handling + +Matching stays line-oriented and bounded: `ORDERED_ITEM_RE` is anchored, has no nested quantifier and cannot backtrack unboundedly on a line of thousands of list markers. `harvest` still never raises for malformed input, never opens a file and never invokes a subprocess, and remains linear in input size. An unclosed fence yields a wrong section boundary at worst, never a hang. + + + + +- **Python 3 standard library only.** No third-party imports, no new top-level files outside `bench/`. Changes land only in `bench/run.py` and `bench/test_review.py` in this prompt. +- **Never create, regenerate, overwrite or edit any file under `bench/testdata/`.** On a digest mismatch, stop and report failed. +- **`bench/testdata/sample-report.md` and `bench/testdata/real-capture-report.md` are byte-frozen** and must still harvest to their previously asserted results. +- **No test function may be deleted and no assertion relaxed.** Per-file assertion floors that must hold after this prompt: `grep -cE '^\s*(self\.assert|assert )' bench/test_config.py` ≥ 63, `bench/test_resolve.py` ≥ 46, `bench/test_review.py` ≥ 165. The suite's test count must stay strictly greater than 72. +- **Do NOT change the `NOT A REVIEW` gate.** +- **Do NOT change `bench/prs.json`, `commands/pr-review.md`, or any rule, agent, command or doc that participates in a review.** +- **Do NOT make the list-item markers configurable** and do NOT add any flag, env var or opt-out. They are frozen invariants. +- **Do NOT change attribution extraction in this prompt.** `_extract_rule_id` and `_extract_path_line` are prompt 3's subject; leave them exactly as they are. +- **Do NOT touch the raw-stdout-before-parsing ordering** in `process_pr`. +- **Harvested values are data, never paths.** No value read out of review output is opened, stat-ed, joined onto a filesystem root, or passed to a subprocess. +- No personal paths (`/Users/`, `~/Documents/`) in any shipped file. +- Bench tests must not require network access, a real `claude` binary, or GitHub access. +- Do not read from or write to `bench/.cache/` or `bench/results/`. +- Do NOT edit `bench/README.md` or `CHANGELOG.md` in this prompt — prompt 5 of this spec owns both (spec 005 AC16/AC17). `docs/dod.md`'s "CHANGELOG.md has an entry under `## Unreleased`" criterion is deliberately deferred to prompt 5 and its absence here is **expected — do NOT report it as a blocker** and do NOT add an entry to satisfy it. +- Do NOT commit — dark-factory handles git. + + + +``` +# Prompt 1 landed (precondition, not a new check) +grep -n 'BOLD_RUN_START_RE\|class HarvestResult' bench/run.py + +# Fixtures untouched +shasum -a 256 bench/testdata/capture-numbered-findings-h3.md | cut -d' ' -f1 +shasum -a 256 bench/testdata/capture-traceability-h4.md | cut -d' ' -f1 +shasum -a 256 bench/testdata/sample-report.md | cut -d' ' -f1 # expect de40c00e7d3c452fa7475be9fa6541426058a96dba91487460aa53be0bd186ae +shasum -a 256 bench/testdata/real-capture-report.md | cut -d' ' -f1 # expect be1400f065d6b856910e7ac91c7f4801598b57afb444f55cf2e257a43619f4db + +# The new recognition surface exists and the double-strip is gone +grep -n 'ORDERED_ITEM_RE\|def list_item_body\|def _normalize_body' bench/run.py +grep -n 'startswith((\"\*\", \"-\"))' bench/run.py ; echo "exit=$? (expect 1 — the extra strip is deleted)" + +# The five numbered items are now visible — ASSERTED, not printed. +# `print` always exits 0 and the daemon does not gate on verification exit codes, +# so a harvest yielding 2, or 5, or 7-with-corrupted-bodies would otherwise "pass" +# the one check that proves this prompt's headline objective. +python3 - <<'EOF' +import sys, pathlib +sys.path.insert(0, 'bench') +import run as R +ids = R.load_rule_ids(pathlib.Path('.')) +r = R.harvest(pathlib.Path('bench/testdata/capture-numbered-findings-h3.md').read_text(), ids) +bodies = [f['body'] for f in r.findings] +print('findings', len(bodies)) +for b in bodies: + print(' ', b[:70]) + +assert len(bodies) == 7, f"expected 7 findings, got {len(bodies)}: {bodies}" +expected_prefixes = [ + '**`CHANGELOG.md:18`**', + '**`README.md`', + '**`.github/workflows/ci.yml:32`**', + '**CI + `Makefile.precommit`**', + '**`Makefile.precommit` `trivy` target**', +] +for i, pfx in enumerate(expected_prefixes): + assert bodies[i].startswith(pfx), f"body {i} must start with {pfx!r}, got {bodies[i][:80]!r}" + +# The traceability capture proves the _normalize_body bold-run fix: both asterisks survive. +t = R.harvest(pathlib.Path('bench/testdata/capture-traceability-h4.md').read_text(), ids) +assert len(t.findings) == 1, f"expected 1 finding, got {len(t.findings)}" +assert t.findings[0]['body'].startswith('**No test coverage for'), \ + f"leading bold run was mangled: {t.findings[0]['body'][:80]!r}" + +print('replay assertions OK') +EOF + +# Assertion floors (AC15) +grep -cE '^\s*(self\.assert|assert )' bench/test_config.py # expect >= 63 +grep -cE '^\s*(self\.assert|assert )' bench/test_resolve.py # expect >= 46 +grep -cE '^\s*(self\.assert|assert )' bench/test_review.py # expect >= 165 + +# Full suite and repository gate +python3 -m unittest discover -s bench -p 'test_*.py' -v 2>&1 | tail -40 +make precommit +``` + +Expected: the four digests are unchanged; the extra-strip grep exits 1; the replay ends with `replay assertions OK` (any `AssertionError` is a failure, not a diagnostic) and reports `findings 7` for the numbered capture (five Should Fix items plus the two Nice to Have bullets — the two become unattributable in prompt 4, not here) and the first five bodies begin `**\`CHANGELOG.md:18\`**`, `**\`README.md\` …`, `**\`.github/workflows/ci.yml:32\`**`, `**CI + \`Makefile.precommit\`**`, `**\`Makefile.precommit\` \`trivy\` target**`; the unittest run reports `OK` with `Ran N tests`, `N > 72`, and lists the ordered-item and body-fidelity tests by name; `make precommit` exits 0. + diff --git a/prompts/in-progress/024-spec-005-attribution-from-reviewer-markers.md b/prompts/in-progress/024-spec-005-attribution-from-reviewer-markers.md new file mode 100644 index 0000000..a20d7b9 --- /dev/null +++ b/prompts/in-progress/024-spec-005-attribution-from-reviewer-markers.md @@ -0,0 +1,290 @@ +--- +status: approved +spec: [005-bug-bench-harvest-finding-extraction] +created: "2026-08-08T11:42:00Z" +queued: "2026-08-08T12:14:19Z" +--- + + +- Every finding the benchmark records now carries the file, line and rule the reviewer itself attached to it +- Until now the ledger held thirty findings with zero rule ids, while the raw output carried the tags all along +- The rule id is read from the reviewer's own inline tag and recorded exactly as written, even when that rule is not in the runner's own index +- That matters because a renamed or newly added rule previously vanished into a null, turning attribution into a fact about the runner's bookkeeping rather than the review +- A rule name mentioned in passing — in prose, or in a closing traceability table — is never attributed to a finding it does not belong to +- One legacy shape survives on purpose: when an item carries no inline tag and opens with a rule name the runner already knows, that name is still used, because three findings in a frozen fixture depend on it +- The file reference is read from the front of the item, where the reviewer puts it, in the four shapes it actually writes +- A file path mentioned later in a long finding no longer overrides the one at the front +- No path is ever guessed by searching the repository, and no line number is inferred from surrounding text +- The five previously invisible numbered findings are now locked down with their exact expected attribution +- Third of five prompts; it changes only where attribution comes from, and still writes every item to the ledger + + + +Read `path`, `line` and `rule_id` from the reviewer's own markers rather than by scanning the whole item against the runner's copy of the rule index: `rule_id` from the item's inline `*(rule: \`\`)*` tag, recorded as the literal string whether or not that id is in `rules/index.json`; `path` and `line` from the bold run at the head of the item, in the four shapes the reviewer actually writes. After this prompt the five numbered Should Fix items in the real capture carry exactly the attribution the capture supplies. + + + +Read `CLAUDE.md` for project conventions (Python 3 standard library only, no personal paths, generic examples only, never commit — dark-factory handles git). + +Read `specs/in-progress/005-bug-bench-harvest-finding-extraction.md`. This prompt satisfies **Desired Behaviors 3 and 4** and **Acceptance Criteria AC5, AC7 and AC8**. Load-bearing sections: `### RC3 — attribution is coupled to the runner's copy of the rule index`, `## Desired Behavior` items 3 and 4, `## Acceptance Criteria` AC5/AC7/AC8, the `## Non-goals` clause forbidding path inference, `## Constraints`, and the `## Failure Modes` rows for a drifted rule index and for a traceability table. + +**This prompt depends on prompts 1 and 2 of this spec having landed.** Verify before you start: + +```bash +grep -n 'BOLD_RUN_START_RE\|class HarvestResult\|ORDERED_ITEM_RE\|def list_item_body' bench/run.py +``` + +If any of the four is absent, stop and report `status: failed` with the message `"prompts 1-2 of spec 005 not yet landed"`. Do not implement them here. + +Read `bench/run.py` — specifically `load_rule_ids`, `_extract_rule_id`, `_extract_path_line`, `_normalize_body`, `list_item_body`, `HarvestResult` and `harvest`'s `flush_finding`. Take every signature from the file. + +Read `bench/testdata/capture-numbered-findings-h3.md` in full. Its five numbered Should Fix items are the input for AC5 and the source of the four bold-reference shapes in AC8. Read them character by character — the backticks, the quotation marks and the `(~lines 76-94)` parenthetical all matter. + +Read `bench/testdata/sample-report.md`. It is **byte-frozen** and its three findings must keep the `rule_id`, `path` and `line` values `TestHarvestNormalizesSampleReport` already asserts. Its items carry no leading bold run and no inline `*(rule: …)*` tag; each begins with a backticked rule id at the head of the item. Requirement 3 below defines the fallback that keeps them green. + +Read `bench/test_review.py` — in particular `TestHarvestNormalizesSampleReport`, `TestTrailingProseDoesNotSwallowARealFinding`, `TestHeadingLevelDoesNotChangeHarvest` and `TestProseBeforeAListItemOpensNothing`, all of which assert `path`, `line` or `rule_id` on items whose attribution sits at the head of the item rather than in a bold run. + +Read `docs/dod.md` for the repository's Definition of Done. + +**Two rule-id sources, in priority order — both are part of the shipped contract.** + +Spec Desired Behavior 4 (as amended) names two sources and one asymmetry between them: + +1. The item's own inline `*(rule: ``)*` marker always wins, and its value is recorded **verbatim, without checking `rules/index.json`** — that index coupling is the defect RC3 names, and this path is free of it. +2. **Only when no marker is present**, a backticked token at the very head of the item that **is a member of `rules/index.json`** is used. This legacy shape is what the three findings in the byte-frozen `bench/testdata/sample-report.md` depend on; deleting it would null all three and break a frozen fixture's asserted result. It stays index-gated on purpose: at the head of an item, an unknown backticked token is far more likely to be a file path or a symbol than a rule name. + +Nothing is ever taken from the middle or the tail of an item, and nothing from outside it, under either source. Spec AC7 Cases C and D lock both halves of the second source — C that a known head token is used, D that an unknown one yields `None`. + +This asymmetry is deliberate and **must be documented as such**. Prompt 5 rewrites `bench/README.md`; its text must describe **both** sources and must not claim rule ids are "never validated against `rules/index.json`" without qualifying that to the inline-marker path only. A doc that disagrees with the parser is the exact mechanism behind D1, D2, D4 and D7. + + + + +## 1. Re-verify the fixtures you assert against + +```bash +python3 -c 'import hashlib,sys;print(hashlib.sha256(open(sys.argv[1],"rb").read()).hexdigest())' bench/testdata/capture-numbered-findings-h3.md # expect 5530049fa4d116dc5762b69c9c9498ff0865c0ae0c6b1de7b3ae4cc846643e93 +python3 -c 'import hashlib,sys;print(hashlib.sha256(open(sys.argv[1],"rb").read()).hexdigest())' bench/testdata/capture-traceability-h4.md # expect 2922746bb95bdb3a67a683942531362271d8f3ccd558067d910146e054bcfe7c +python3 -c 'import hashlib,sys;print(hashlib.sha256(open(sys.argv[1],"rb").read()).hexdigest())' bench/testdata/sample-report.md # expect de40c00e7d3c452fa7475be9fa6541426058a96dba91487460aa53be0bd186ae +``` + +On any mismatch, stop and report `status: failed` with the observed digest. **Never write, regenerate or edit a file under `bench/testdata/`.** + +## 2. Add the marker constants + +Add to the module constants block in `bench/run.py`, next to `BULLET_RE` / `ORDERED_ITEM_RE`: + +```python +RULE_TAG_RE = re.compile(r"\*\(rule:\s*`([^`]+)`\)\*") +HEAD_RULE_TAG_RE = re.compile(r"^`([^`]+)`") +LEADING_BOLD_RE = re.compile(r"^\*\*(.+?)\*\*") +PATH_LINE_RE = re.compile(r"([A-Za-z0-9_./-]+\.[A-Za-z0-9_./-]+):(\d+)") +BACKTICK_TOKEN_RE = re.compile(r"`([^`\s]+)`") +LINE_MENTION_RE = re.compile(r"(?i)\blines?\s*~?\s*(\d+)") +``` + +`LEADING_BOLD_RE` is non-greedy so it stops at the first closing `**`, which is where the reviewer's reference ends. `PATH_LINE_RE` is the pattern already inlined in `_extract_path_line`, hoisted to a constant so both call sites share it. + +## 3. Replace attribution extraction with positional, priority-ordered rules + +Add one function immediately above `_normalize_body`, and route `flush_finding` through it. It operates on the **normalised body** (list marker already removed by `list_item_body`, leading bold run intact), never on raw lines. + +```python +def extract_attribution(body: str, known_rule_ids: set) -> tuple[str | None, int | None, str | None]: + """Return (path, line, rule_id) for one finding item, read from the item's own markers. + + rule_id, in priority order: + 1. the item's own inline `*(rule: ``)*` marker — recorded as the literal + string the reviewer wrote, whether or not it appears in rules/index.json; + 2. otherwise a backticked token at the very head of the item that is a member + of known_rule_ids (the shape the review template emits when it tags a + finding by leading its body with the rule id); + 3. otherwise None. + An id named anywhere else in the item, or anywhere outside it, is never used. + + path/line, in priority order: + 1. the bold run at the head of the item, when it names a path; + 2. otherwise the first path:line reference anywhere in the item that is not + a known rule id; + 3. otherwise (None, None). + When the leading bold run supplies a path, line is taken from that bold run and + from nowhere else — a line number appearing only in the item's trailing prose is + not used. No path is ever inferred by searching the repository and no line is + ever guessed from surrounding text. + """ +``` + +Implementation rules, exactly: + +**rule_id** +1. `RULE_TAG_RE.search(body)` → return `group(1)` verbatim. Do **not** check membership in `known_rule_ids`. A tag naming a renamed rule, a rule added since the index was written, or a rule from a different rules revision is recorded as written; reconciling it against the shipped rule set is the scorer's job. +2. Else `HEAD_RULE_TAG_RE.match(body)` → if `group(1) in known_rule_ids`, return it. +3. Else `None`. + +**path / line** +1. `LEADING_BOLD_RE.match(body)`. If it matches, let `ref = group(1)` and read only from `ref`: + - `PATH_LINE_RE.search(ref)` → `(group(1), int(group(2)))`. Return. + - else the first `BACKTICK_TOKEN_RE` match in `ref` whose token contains a `.` → that token is the path; the line is `int(LINE_MENTION_RE.search(ref).group(1))` when that pattern matches `ref`, else `None`. Return. + - else fall through to step 2 (a bold run that names no path supplies no attribution). +2. `_extract_path_line(body, known_rule_ids)` — the existing whole-item scan, unchanged, which skips tokens that are known rule ids. +3. Else `(None, None)`. + +Delete `_extract_rule_id` entirely; it implements exactly the membership scan RC3 identifies as the defect and has no remaining caller. Keep `_extract_path_line` as the step-2 fallback and change nothing inside it except using the hoisted `PATH_LINE_RE`. + +In `flush_finding`, drop the `text = " ".join(current_finding_lines)` variable and its two uses, and call `extract_attribution(body, known_rule_ids)` once after the `None.` sentinel check. Both extractors must read the same normalised `body`, so that `^` anchoring means "the head of the item". + +`known_rule_ids` stays in `harvest`'s signature and stays wired from `load_rule_ids` in `run_bench` — it is still consulted, for the head-anchored fallback and for the `_extract_path_line` skip list only. + +## 4. The four bold-reference shapes, verified against the capture + +These are the four shapes the capture actually contains. Your implementation must produce exactly these results: + +| Leading bold run | `path` | `line` | +|---|---|---| +| ``**`CHANGELOG.md:18`**`` | `CHANGELOG.md` | 18 | +| ``**`README.md` "Security gates" section (~lines 76-94)**`` | `README.md` | 76 | +| ``**`.github/workflows/ci.yml:32`**`` | `.github/workflows/ci.yml` | 32 | +| ``**CI + `Makefile.precommit`**`` | `Makefile.precommit` | `None` | + +Note the third: `PATH_LINE_RE` must match a path that begins with a dot. Note the fourth: a bold run may carry ordinary words around the backticked path, and a backticked token with no dot (for example `` `trivy` `` in the fifth capture item) is not a path. + +## 5. Tests + +Add to `bench/test_review.py`. Every failure message prints the full observed list. + +**AC5 — the five previously-dropped numbered findings carry the capture's attribution.** `class TestNumberedCaptureFindingsCarryAttribution`, one test method that harvests `bench/testdata/capture-numbered-findings-h3.md` and compares paths, lines, rule ids and the count together: + +```python +expected = [ + ("CHANGELOG.md", 18, "changelog/conventional-prefix-required"), + ("README.md", 76, "readme/user-facing-not-agent-context"), + (".github/workflows/ci.yml", 32, None), + ("Makefile.precommit", None, None), + ("Makefile.precommit", None, None), +] +``` + +Assert `[(f["path"], f["line"], f["rule_id"]) for f in result.findings[:5]] == expected` and assert `len(result.findings) == 7`. The trailing two are the `### Nice to Have (Optional)` bullets; prompt 4 of this spec reclassifies them as unattributable and updates this count to `5` — do not anticipate that here, and do not weaken the assertion to an inequality. + +**AC7 — `rule_id` comes from the reviewer's marker first, and from an index-gated head-anchored token second.** `class TestRuleIdComesFromTheItemsOwnMarkers`, four test methods, one per spec AC7 case: + +- Case A — an item tagged with an id absent from `rules/index.json` yields that literal string, not `None`: + + ``` + ## Must Fix (Critical) + - **`src/x.py:1`** something is wrong here. *(rule: `made-up/not-in-the-index`)* + ``` + + Assert `rule_id == "made-up/not-in-the-index"`, and assert in the same test that `"made-up/not-in-the-index" not in run.load_rule_ids(run.REPO_ROOT)` so the case cannot silently become vacuous if someone adds that id. + +- Case B — an item whose prose names a different, real rule id **before** its own marker yields the marker's id. Pick two distinct real ids from `run.load_rule_ids(run.REPO_ROOT)` at test time rather than hardcoding them, assert they differ, put one in the prose and the other in the marker, and assert the marker's id is returned. A first-membership-match scan passes Case A alone; this is what closes it. + +- **Case C** — an item carrying **no** marker whose body opens with a backticked token that **is** a member of `rules/index.json` yields that id: + + ``` + ## Must Fix (Critical) + - ``: a finding in src/x.py:1 + ``` + + Pick `` from `run.load_rule_ids(run.REPO_ROOT)` at test time rather than hardcoding it, and shape the item so the token sits at position 0 of the normalised body — the head anchor cannot see past a leading bold run. Assert `rule_id == `. This is the head-anchored legacy shape the byte-frozen `sample-report.md` depends on for three findings; `TestHarvestNormalizesSampleReport` is only an indirect guard and, as spec AC7 states, would not survive a refactor of that fixture's test. + +- **Case D** — the same shape whose head token is **absent** from `rules/index.json` yields `None`: + + ``` + ## Must Fix (Critical) + - `not-a-real-rule-id`: a finding in src/x.py:7 + ``` + + Assert `rule_id is None`, assert `path == "src/x.py"` and `line == 7`, and assert in the same test that `"not-a-real-rule-id" not in run.load_rule_ids(run.REPO_ROOT)` so the case cannot become vacuous. **The item must carry a path** — without one it yields neither `path` nor `rule_id`, and prompt 4 would reclassify it as unattributable, moving it out of `result.findings` and breaking this assertion. + + **Case D is the only test in the whole suite that fails if the index gate on the head-anchored source is dropped.** Against an implementation that records any head-anchored backticked token as a rule id, every other test stays green: `sample-report.md`'s head tokens are all real ids; the five capture items and every AC8 synthetic open with `**`; Cases A and B carry markers; the traceability item opens with `**`; and `TestHarvestKeepsFindingWithoutAnyRuleId` has no head backtick. Do not omit or merge this case. + +**AC8 — `path` and `line` come from the leading bold reference.** `class TestPathAndLineComeFromTheLeadingBoldReference`, with: + +- one assertion per row of the table in requirement 4, over synthetic single-item sections carrying that exact bold run; +- a negative case: an item whose leading bold reference names `a/b.py:10` and whose trailing prose mentions `c/d.py:99` resolves to `("a/b.py", 10)`. **This is a shape check, not a regression guard — it also passes against the unchanged parser**, because `_extract_path_line` (`bench/run.py:1028`) scans the whole item and returns the *first* `path:line` match, which for this input is already the leading one. Keep it, but do not treat its green as evidence the change works; +- a second negative case: an item whose leading bold run names a path with no line (``**`a/b.py`**``) and whose trailing prose mentions `line 42` resolves to `("a/b.py", None)` — a line appearing only in the prose is not used when the bold run supplies the path; +- **a third negative case, the discriminating one — do not omit it and do not merge it into the case above**: an item whose leading bold run names a path with **no** line (``**`a/b.py`**``) and whose trailing prose carries a **full** `c/d.py:99` reference resolves to `("a/b.py", None)`. The shipped parser returns `("c/d.py", 99)` for this input (verified), so this is the one AC8 case that is **red before the change and green after**. Without it, every AC8 negative case is satisfiable with zero code change. + +**AC9 completion.** Add one assertion to the existing `TestBodyPreservesLeadingBoldRun` test created by prompt 2: `result.findings[0]["path"] == "src/config.ts"`. Do not remove any assertion already in that method. + +**AC10 Case B regression.** The existing outside-section test that asserts no harvested `rule_id` from `bench/testdata/capture-traceability-h4.md` is drawn from its `### Traceability` table must stay green. It is now non-vacuous: the table sits outside the item and outside the section, and `RULE_TAG_RE` never reaches it. + +**Frozen-fixture regression.** `TestHarvestNormalizesSampleReport` must still pass unchanged, including its final loop asserting every non-null `rule_id` is a member of the real index. If it goes red, the head-anchored fallback in requirement 3 is wrong — fix the fallback, never the fixture and never the assertion. + +## 6. Failure handling and safety + +- Every new pattern is anchored or bounded, contains no nested quantifier, and cannot backtrack unboundedly. `LEADING_BOLD_RE` is non-greedy and anchored at `^`; an item with no closing `**` simply does not match and falls through to the whole-item scan. +- `extract_attribution` never raises: every `search`/`match` result is checked for `None` before `group` is called, and `int(...)` is only reached on a `(\d+)` capture. +- **Extracted values are data, never paths.** A `path` or `rule_id` read out of review output is written to the ledger row and used for nothing else — never opened, never stat-ed, never joined onto a filesystem root, never passed to a subprocess, never compared against the filesystem. A review emitting ``**`../../etc/passwd:1`**`` produces a ledger row containing that string and no filesystem access. Add a test asserting exactly that: harvest a section carrying that item and assert `path == "../../etc/passwd"` and `line == 1`, with no file access anywhere in the code path. +- `harvest` remains a pure function over text, linear in input size, and still never raises for malformed input. + + + + +- **Python 3 standard library only.** No third-party imports, no new top-level files outside `bench/`. Changes land only in `bench/run.py` and `bench/test_review.py` in this prompt. +- **Never create, regenerate, overwrite or edit any file under `bench/testdata/`.** On a digest mismatch, stop and report failed. +- **`bench/testdata/sample-report.md` and `bench/testdata/real-capture-report.md` are byte-frozen** and must still harvest to their previously asserted results, including every `rule_id`, `path` and `line` value already asserted. +- **Do NOT infer a path by searching the repository** for a filename mentioned in a finding's prose, and do NOT guess a line number from surrounding text. Attribution comes from the reviewer's markers or it does not come at all. +- **Do NOT add the unattributable-item classification or any runner-side rejection in this prompt.** `HarvestResult.unattributable` stays empty; every item still becomes a finding. Prompt 4 of this spec owns that. +- **No test function may be deleted and no assertion relaxed.** Per-file assertion floors that must hold after this prompt: `grep -cE '^\s*(self\.assert|assert )' bench/test_config.py` ≥ 63, `bench/test_resolve.py` ≥ 46, `bench/test_review.py` ≥ 165. The suite's test count must stay strictly greater than 72. +- **Do NOT change the `NOT A REVIEW` gate.** +- **Do NOT change `bench/prs.json`, `commands/pr-review.md`, or any rule, agent, command or doc that participates in a review.** +- **Do NOT make the inline rule-tag marker shape configurable** and do NOT add any flag, env var or opt-out. It is a frozen invariant. +- **Do NOT touch the raw-stdout-before-parsing ordering** in `process_pr`. +- No personal paths (`/Users/`, `~/Documents/`) in any shipped file. +- Bench tests must not require network access, a real `claude` binary, or GitHub access. +- Do not read from or write to `bench/.cache/` or `bench/results/`. +- Do NOT edit `bench/README.md` or `CHANGELOG.md` in this prompt — prompt 5 of this spec owns both (spec 005 AC16/AC17). `docs/dod.md`'s "CHANGELOG.md has an entry under `## Unreleased`" criterion is deliberately deferred to prompt 5 and its absence here is **expected — do NOT report it as a blocker** and do NOT add an entry to satisfy it. +- Do NOT commit — dark-factory handles git. + + + +``` +# Prompts 1-2 landed (precondition, not a new check) +grep -n 'BOLD_RUN_START_RE\|class HarvestResult\|ORDERED_ITEM_RE\|def list_item_body' bench/run.py + +# Fixtures untouched +python3 -c 'import hashlib,sys;print(hashlib.sha256(open(sys.argv[1],"rb").read()).hexdigest())' bench/testdata/capture-numbered-findings-h3.md +python3 -c 'import hashlib,sys;print(hashlib.sha256(open(sys.argv[1],"rb").read()).hexdigest())' bench/testdata/capture-traceability-h4.md +python3 -c 'import hashlib,sys;print(hashlib.sha256(open(sys.argv[1],"rb").read()).hexdigest())' bench/testdata/sample-report.md +python3 -c 'import hashlib,sys;print(hashlib.sha256(open(sys.argv[1],"rb").read()).hexdigest())' bench/testdata/real-capture-report.md + +# The membership scan is gone and the positional extractors exist +grep -n 'def _extract_rule_id' bench/run.py ; echo "exit=$? (expect 1 — deleted)" +grep -n 'RULE_TAG_RE\|HEAD_RULE_TAG_RE\|LEADING_BOLD_RE\|BACKTICK_TOKEN_RE\|LINE_MENTION_RE\|def extract_attribution' bench/run.py + +# AC5 — the five numbered items now carry their attribution +python3 - <<'EOF' +import sys, json, pathlib +sys.path.insert(0, 'bench') +import run as R +ids = R.load_rule_ids(pathlib.Path('.')) +r = R.harvest(pathlib.Path('bench/testdata/capture-numbered-findings-h3.md').read_text(), ids) +for f in r.findings: + print(json.dumps({k: f[k] for k in ('path', 'line', 'rule_id')})) +EOF + +# Frozen fixture still attributes as before +python3 - <<'EOF' +import sys, json, pathlib +sys.path.insert(0, 'bench') +import run as R +ids = R.load_rule_ids(pathlib.Path('.')) +r = R.harvest(pathlib.Path('bench/testdata/sample-report.md').read_text(), ids) +for f in r.findings: + print(json.dumps({k: f[k] for k in ('path', 'line', 'rule_id')})) +EOF + +# Assertion floors (AC15) +grep -cE '^\s*(self\.assert|assert )' bench/test_config.py # expect >= 63 +grep -cE '^\s*(self\.assert|assert )' bench/test_resolve.py # expect >= 46 +grep -cE '^\s*(self\.assert|assert )' bench/test_review.py # expect >= 165 + +# Full suite and repository gate +python3 -m unittest discover -s bench -p 'test_*.py' -v 2>&1 | tail -40 +make precommit +``` + +Expected: the four digests are unchanged; the `_extract_rule_id` grep exits 1; the numbered-capture replay prints the five AC5 rows in order followed by two rows of all-nulls; the sample-report replay prints `agents/my-agent.md` / `3` / `agent-cmd/agent-frontmatter`, then `null` / `null` / `agent-cmd/command-thin`, then `null` / `null` / `changelog/unreleased-entry-required`; the unittest run reports `OK` with `Ran N tests`, `N > 72`, and lists the rule-tag, leading-bold-reference and numbered-capture tests by name; `make precommit` exits 0. + diff --git a/prompts/in-progress/025-spec-005-unattributable-item-rejection.md b/prompts/in-progress/025-spec-005-unattributable-item-rejection.md new file mode 100644 index 0000000..3fc6a8d --- /dev/null +++ b/prompts/in-progress/025-spec-005-unattributable-item-rejection.md @@ -0,0 +1,298 @@ +--- +status: approved +spec: [005-bug-bench-harvest-finding-extraction] +created: "2026-08-08T11:43:00Z" +queued: "2026-08-08T12:14:19Z" +--- + + +- A review item the benchmark cannot attribute now fails that pull request loudly instead of being written down as a finding with nothing in it +- An unkeyable finding is an unmatchable measurement dressed as a data point; recording it is worse than refusing it +- The refusal is visible: the run exits non-zero, names the pull request, names the section, and quotes the offending item word for word +- It behaves exactly like the existing "not a review" refusal — no ledger row, no cache entry, and the pull request is retried on the next run +- The reviewer's raw output is deliberately kept on disk, so the operator can re-examine it later without spending tokens again +- Every other pull request in the same run still gets processed and recorded +- The refusal is not a blanket one: the same review with a file reference on the item is accepted and recorded normally +- The honest consequence on one real capture is that a genuine review fails rather than contributing two findings that could never be scored +- There is deliberately no opt-out — an escape hatch on this behavior is the regression the whole change exists to close +- Fourth of five prompts, and the only one that touches the runner's control flow + + + +Make harvesting report every item inside a severity section that yields neither a `path` nor a `rule_id`, and make a non-empty report fail the PR in the same class as the existing `NOT A REVIEW` gate — no ledger row, no `.json` row marker, a both-stream failure artifact, the raw `.stdout.txt` left in place, the PR listed as failed, the remaining PRs still processed, the process exiting non-zero, with a diagnosis headed by the frozen literal `UNATTRIBUTABLE FINDING`. + + + +Read `CLAUDE.md` for project conventions (Python 3 standard library only, no personal paths, generic examples only, never commit — dark-factory handles git). + +Read `specs/in-progress/005-bug-bench-harvest-finding-extraction.md`. This prompt satisfies **Desired Behavior 5** and **Acceptance Criteria AC11, AC12 and AC13**. Load-bearing sections: `## Desired Behavior` item 5, `## Acceptance Criteria` AC11/AC12/AC13 **including both of their explanatory paragraphs about the `.json` discriminator**, the `## Constraints` bullet "The raw-stdout-before-parsing invariant is preserved, and the two rejection classes differ in what they leave behind", the `## Non-goals` bullet forbidding an opt-out, and the first three `## Failure Modes` rows. + +**This prompt depends on prompts 1, 2 and 3 of this spec having landed.** Verify before you start: + +```bash +grep -n 'class HarvestResult\|def list_item_body\|def extract_attribution' bench/run.py +``` + +If any of the three is absent, stop and report `status: failed` with the message `"prompts 1-3 of spec 005 not yet landed"`. Do not implement them here. + +Read `bench/run.py`. Take every signature from the file: + +- `NON_REVIEW_MARKER = "NOT A REVIEW"` and `REJECTION_EXCERPT_BYTES = 2000` in the constants block — the new marker is their sibling. +- `rejection_excerpt` and `non_review_report` — the diagnosis you mirror. +- `failures_root`, `failure_artifact_path`, `failure_artifact_text`, `write_failure_artifact` — the failure artifact shipped in v0.35.3, reused unchanged. +- `reviews_root`, `cache_row_path`, `cache_raw_path`, `atomic_write_bytes`, `append_row`. +- `harvest`, `HarvestResult`, `flush_finding`, `extract_attribution`. +- `process_pr` end to end. Note the numbered steps: **1** cache check keyed on the row marker alone, **2** resolve, **3** invoke, **4** the `NOT A REVIEW` gate (which fires *before* anything is written), **5** the raw-stdout write with its comment "Write raw stdout verbatim before any parsing", **6** harvest, **7** ledger append, **8** row-marker write. +- `run_bench`'s per-PR `try` block: a `BenchError` becomes `("failed", str(err))`, the loop continues to the next PR, and the summary line counts `failed:` prefixes. + +Read `bench/test_review.py`, in particular the module-level helper `run_one_pr_with_payload(td, payload) -> (returncode, captured_stderr, results_dir, cache_root)` and its **five call sites across four classes** (`bench/test_review.py` ~1062, ~1101, ~1128, ~1152, ~1191 — `TestNonReviewOutputIsRejected`, `TestSectionNamesOutsideHeadingsDoNotSatisfyTheGate`, `TestMissingSectionNamesAreReportedExactly` twice, and `TestReviewShapedOutputAtEitherHeadingLevelProducesARow`). They are the shape your AC11 and AC12 tests follow. Also read `TestHarvestKeepsFindingWithoutAnyRuleId`, whose stub payload requirement 5 below changes. + +Read `bench/testsupport.py` — `review_report(...)`, `CLEAN_REVIEW_REPORT`, `stub_claude`, `with_path`. + +Read `bench/testdata/capture-numbered-findings-h3.md` in full; its two `### Nice to Have (Optional)` bullets are the AC13 subject. + +Read `docs/dod.md` for the repository's Definition of Done. + + + + +## 1. Re-verify the fixture you assert against + +```bash +python3 -c 'import hashlib,sys;print(hashlib.sha256(open(sys.argv[1],"rb").read()).hexdigest())' bench/testdata/capture-numbered-findings-h3.md # expect 5530049fa4d116dc5762b69c9c9498ff0865c0ae0c6b1de7b3ae4cc846643e93 +``` + +On mismatch, stop and report `status: failed` with the observed digest. **Never write, regenerate or edit a file under `bench/testdata/`.** + +## 2. Add the frozen marker literal + +In the constants block of `bench/run.py`, directly beneath `NON_REVIEW_MARKER = "NOT A REVIEW"`: + +```python +UNATTRIBUTABLE_MARKER = "UNATTRIBUTABLE FINDING" +``` + +That exact string, with a single space, uppercase, no punctuation. It is a frozen invariant: do not derive it, do not make it configurable, do not choose a different one. + +## 3. Classify unattributable items during harvesting + +In `harvest`'s `flush_finding`, after the `None.` sentinel check and after the call to `extract_attribution`, split the item: + +```python + if path is None and rule_id is None: + unattributable.append({ + "section": current_section, + "body": body, + }) + else: + findings.append({ + "path": path, + "line": line_num, + "rule_id": rule_id, + "body": body, + }) +``` + +`line` alone never makes an item attributable — only a `path` or a `rule_id` can key a finding against a golden set. The finding dict keeps exactly its four existing keys `path`, `line`, `rule_id`, `body`; the ledger row schema does not change. + +Declare `unattributable: list = []` alongside `findings` at the top of `harvest`, add it to the `nonlocal` declaration in `flush_finding`, and return `HarvestResult(findings=findings, unattributable=unattributable)`. Update `HarvestResult`'s docstring: the field is no longer reserved. + +Only items **inside** a severity section are ever classified. Content outside one — a positive-notes list, a traceability table, a notes block — opens no item at all and therefore appears in neither component. This is what keeps a correctly-clean review from failing loudly. + +## 4. Build the diagnosis and wire the gate + +Add, immediately after `non_review_report`: + +```python +def unattributable_report(pr_id: str, items: list, stdout_text: str) -> str: + """Build the multi-line stderr diagnosis for a review carrying unkeyable items. + + Names the PR, then each offending item's severity section and its text + verbatim. The item block is passed through the same bounded excerpt the + NOT A REVIEW gate uses, so a runaway subprocess cannot flood the terminal. + """ +``` + +Its output must contain, in order: a first line `f"{UNATTRIBUTABLE_MARKER}: {pr_id}"`; a line stating how many items could not be attributed; then one block per item of the form `f"[{item['section']}] {item['body']}"`; then the line `no ledger row and no row marker were written; this PR is retried on the next run`. Pass the joined item block through `rejection_excerpt(...)` so stderr stays bounded exactly as the existing gate's is. Do not re-print the whole subprocess stdout — the raw capture is already on disk and the failure artifact already carries both streams. + +In `process_pr`, insert a new step between the harvest and the ledger append — after step 6, before step 7: + +```python + # 6a. Unattributable-item gate — a finding that cannot be keyed is a parse + # failure, not a body-only finding. Fires *after* the step-5 raw write, + # so the raw capture stays on disk and is re-harvestable after a fix, + # and *before* the ledger append and the row-marker write, so the PR is + # retried next run (the cache check keys on the row marker alone). + if harvested.unattributable: + n = len(harvested.unattributable) + write_failure_artifact( + cache_root, cfg_hash, pr_id, + reason=f"{UNATTRIBUTABLE_MARKER}: {n} unattributable item(s)", + stdout=proc.stdout, stderr=proc.stderr, + ) + print(unattributable_report(pr_id, harvested.unattributable, proc.stdout), + file=sys.stderr) + raise BenchError( + f"{UNATTRIBUTABLE_MARKER}: {pr_id}: {n} unattributable item(s)" + ) +``` + +Nothing else in `process_pr` moves. Step 5 stays ahead of step 6; the `NOT A REVIEW` gate stays at step 4, ahead of step 5, and is not touched. `run_bench` needs no change — it already turns a `BenchError` into a `failed` outcome, continues with the remaining PRs, and returns 1. + +## 5. Repair the stub payloads that now carry unattributable items + +Running the suite after requirement 4 will turn some existing tests red because their stub payloads contain a finding item with no attribution. Fix the **payload**, never the assertion — the spec calls this out explicitly as a payload change, not an assertion change. + +The known one is `TestHarvestKeepsFindingWithoutAnyRuleId`. Change its report from + +``` +#### Must Fix (Critical) +- This finding has no rule ID at all but should still be kept. +``` + +to + +``` +#### Must Fix (Critical) +- **`src/x.py:4`** This finding has no rule ID at all but should still be kept. +``` + +Its three assertions stay exactly as they are: one finding, `rule_id is None`, `"no rule ID"` in the body. The test still asserts what it always asserted — an item citing no rule id is kept with `rule_id` null — and it now does so on an item that is attributable by path, which is the contract. + +**The second known-red test is `TestRawOutputIsCachedVerbatim` (`bench/test_review.py:613`).** Its payload's finding item has no leading bold run and no rule marker, so after prompt 3 both `path` and `rule_id` are `None`, the new gate fires, `rc` becomes 1 and its `assertEqual(rc, 0)` fails. Repair the **payload** — change its Must Fix item to: + +``` +- **`agents/x.md:12`** sample finding. +``` + +All of its assertions stay exactly as they are. **Under no circumstances relax `assertEqual(rc, 0)`**: that assertion is the whole point of the test — the raw output is cached verbatim on a *successful* run — and turning it into `assertIn(rc, (0, 1))` or deleting it would hide the very regression this prompt introduces. + +Run the full suite and repair any other **in-test literal payload** the same way: add a leading bold path reference of the form ``**`some/file.ext:NN`**`` to the item. Do not delete a test, do not relax an assertion, and do not add an opt-out to make a payload pass. + +**This repair instruction is fenced to in-test literal payloads only.** If a test that reads a file from `bench/testdata/` goes red — in particular prompt 1's `result.unattributable == []` assertions over `capture-notes-block-h2.md`, `real-capture-report.md` or `capture-summary-trailer-h4.md` — that is **not** a payload to repair. It means the "only items inside a severity section are classified" rule is implemented wrong: content outside a severity section is being swept into `unattributable`, which would fail a PR on a review that correctly found nothing. Fix `harvest`. **Editing a fixture under `bench/testdata/` is forbidden** — all six are digest-pinned by spec AC2 and prompt 5 verifies them with `shasum -c`. + +## 6. Update the AC5 count + +Prompt 3 of this spec left `TestNumberedCaptureFindingsCarryAttribution` asserting `len(result.findings) == 7`. With classification in place the two `### Nice to Have (Optional)` bullets move out of `findings`, so change that assertion to `len(result.findings) == 5`. The five-tuple comparison for `result.findings[:5]` is unchanged. This tightens the assertion; do not replace it with an inequality. + +## 7. Tests + +**AC13 — the unattributable items in the real capture are reported as such.** `class TestNiceToHaveBulletsAreReportedUnattributable`, one test method that harvests `bench/testdata/capture-numbered-findings-h3.md` and asserts: + +- `len(result.unattributable) == 2`; +- both entries have `section == "Nice to Have"`; +- the two bodies equal, verbatim, the fixture's two `### Nice to Have (Optional)` bullets with their `- ` marker removed and internal whitespace collapsed. Take the two strings from the fixture and confirm them against it before writing the test; they are: + + ``` + Manual Trivy apt-install (update/install/repo-key) duplicates the maintained `aquasecurity/setup-trivy` action — adds ~30-60s/run and maintenance surface with no caching/pinning. + ``` + ``` + Commit subject `switch build backend to hatchling and add conventional changelog prefixes` is 73 chars (soft cap 50) — FYI only, not in the active rule set. + ``` + +- **and** that the five Should Fix findings from AC5 are still present in `result.findings`, asserted by the same five `(path, line, rule_id)` tuples. This is the honest consequence of the governing rule on a genuine review: that PR fails loudly rather than contributing two body-only findings. + +**AC11 — an unattributable item fails the PR loudly.** `class TestUnattributableItemFailsThePrLoudly`, one test method using `run_one_pr_with_payload` with the payload + +```python +testsupport.review_report( + should_fix="- an item with neither a file reference nor a rule tag, so it cannot be keyed." +) +``` + +Assert all of: + +- the return code is non-zero; +- stderr contains the **spelled-out literal** `"UNATTRIBUTABLE FINDING"` — write that string in the test source; do **not** assert `run.UNATTRIBUTABLE_MARKER`, whose value the implementer chooses, which would make the assertion compare the implementation against itself and pass for any spelling; +- as its own assertion with its own message, `self.assertEqual(run.UNATTRIBUTABLE_MARKER, "UNATTRIBUTABLE FINDING")` — the marker is a frozen spec invariant (spec 005 AC11, AC16 and Constraints), not an implementation choice. An underscore variant such as `"UNATTRIBUTABLE_FINDING"` would otherwise satisfy every check in this prompt while silently violating spec AC11, prompt 5's `grep -cF` and operator AC20; +- the PR id `test#1`, the section name `Should Fix`, and the item's text verbatim; +- the results ledger gains **0** lines (handle the file not existing at all, as the existing gate tests do); +- under `run.reviews_root(cache_root)` — the temp cache root the test passed to the runner, never `bench/.cache/` — exactly **one** file exists, and it is the `.stdout.txt` raw capture; `list(reviews.glob("*.json"))` has length **0**. Assert the `.json` count as its own assertion with its own message: it is the discriminator, and it is what proves the PR is retried rather than served from cache next run; +- a failure artifact exists under `run.failures_root(cache_root)` and its text contains the item's text; +- the captured stdout summary line reports `1 failed`. Capture stdout with `contextlib.redirect_stdout` alongside the existing stderr capture, or extend `run_one_pr_with_payload` to return it — if you extend the helper, update **all five existing call sites** (~1062, ~1101, ~1128, ~1152, ~1191) rather than duplicating it. Verify the count with `grep -n 'run_one_pr_with_payload' bench/test_review.py` before you start; missing one leaves the suite red for a reason unrelated to this prompt. + +**AC12 — the loud failure is not a blanket rejection, and AC11's probe is live.** `class TestAttributedItemStillProducesARow`, one test method, the same runner invocation and the same review shape with the single item now headed by a bold reference: + +```python +testsupport.review_report( + should_fix="- **`src/x.py:4`** an item with a file reference and no rule tag." +) +``` + +Assert all of: + +- the return code is `0`; +- the results ledger gains exactly **1** line, and that row's `findings[0]["path"] == "src/x.py"` and `findings[0]["line"] == 4`; +- the **same probe AC11 uses**, run here, finds **two** files under `run.reviews_root(cache_root)` — one `.stdout.txt` and exactly **one** `.json` row marker. State the expected counts as `2` and `1` in the assertion messages. The success path writes two files (the raw capture at step 5 and the row marker at step 8); an implementer who observes 2 where the test says 1 would "fix" the assertion and silently dissolve the AC11/AC12 pairing. + +## 8. Failure handling + +- The gate is fail-closed: every ambiguity about whether an item is a keyed finding resolves to rejection. A false rejection costs one operator decision and a re-run; a false acceptance writes an unmatchable measurement into an append-only ledger. +- The diagnosis reproduces the subprocess's own output and nothing else — no environment variables, no tokens, no credential material. +- Review output can be arbitrarily large: the item block passes through `rejection_excerpt`, so stderr stays bounded at `REJECTION_EXCERPT_BYTES` with the truncation marked. +- When **every** PR in a run trips the gate, the run produces zero rows and fails uniformly. That is the correct, visible outcome; do not add a threshold, a tolerance or a downgrade-to-warning path. +- `write_failure_artifact` already tolerates `None` and non-string streams via `stream_text`; do not add error handling around it. + + + + +- **Python 3 standard library only.** No third-party imports, no new top-level files outside `bench/`. Changes land only in `bench/run.py`, `bench/test_review.py` and — only if you extend a helper — `bench/testsupport.py`. +- **Do NOT add an opt-out.** No flag, no environment variable, no manifest field, no config knob that lets a run accept body-only findings or downgrade the rejection to a warning. An escape hatch on this Goal is the regression this spec exists to close. +- **Do NOT make the rejection, the marker literal or the excerpt bound configurable.** All are frozen invariants. +- **Do NOT change the `NOT A REVIEW` gate** — same required sections, same heading matching, same bounded excerpt, same position at step 4 ahead of the step-5 raw write. The new gate is a second, later gate that reuses the same no-row / no-row-marker / retry-next-run semantics. +- **Do NOT move the step-5 raw-stdout write.** It stays ahead of harvesting. The two rejection classes differ deliberately in what they leave behind: `NOT A REVIEW` leaves nothing under `reviews_root`, the new gate leaves the `.stdout.txt` and writes no `.json`. +- **Do NOT delete the raw capture on rejection.** The Failure Modes table promises it is re-harvestable after a fix without spending tokens again. +- **Never create, regenerate, overwrite or edit any file under `bench/testdata/`.** On a digest mismatch, stop and report failed. +- **`bench/testdata/sample-report.md` and `bench/testdata/real-capture-report.md` are byte-frozen** and must still harvest to their previously asserted results. +- **No test function may be deleted and no assertion relaxed.** Per-file assertion floors that must hold after this prompt: `grep -cE '^\s*(self\.assert|assert )' bench/test_config.py` ≥ 63, `bench/test_resolve.py` ≥ 46, `bench/test_review.py` ≥ 165. The suite's test count must stay strictly greater than 72. +- **Do NOT change `bench/prs.json`, `commands/pr-review.md`, or any rule, agent, command or doc that participates in a review.** If the loud-failure rule proves the review command must mandate an attribution on every finding, that is a separate spec. +- **Do NOT re-harvest or migrate rows and raw outputs already written.** +- **Harvested values are data, never paths.** No `path` or `rule_id` read out of review output is opened, stat-ed, joined onto a filesystem root, or passed to a subprocess. +- No personal paths (`/Users/`, `~/Documents/`) in any shipped file. Every test must use its own `tempfile.TemporaryDirectory()` cache root — never `bench/.cache/`, which is gitignored and absent in a fresh clone, making a probe against it vacuous. +- Bench tests must not require network access, a real `claude` binary, or GitHub access. +- Do NOT edit `bench/README.md` or `CHANGELOG.md` in this prompt — prompt 5 of this spec owns both (spec 005 AC16/AC17). `docs/dod.md`'s "CHANGELOG.md has an entry under `## Unreleased`" criterion is deliberately deferred to prompt 5 and its absence here is **expected — do NOT report it as a blocker** and do NOT add an entry to satisfy it. +- Do NOT commit — dark-factory handles git. + + + +``` +# Prompts 1-3 landed (precondition, not a new check) +grep -n 'class HarvestResult\|def list_item_body\|def extract_attribution' bench/run.py + +# The frozen marker and the gate exist, in the right places +grep -nF 'UNATTRIBUTABLE_MARKER = "UNATTRIBUTABLE FINDING"' bench/run.py +grep -n 'def unattributable_report' bench/run.py +grep -n '# 4. Sanity gate\|# 5. Write raw stdout\|# 6. Harvest\|# 6a. Unattributable\|# 7. Build row\|# 8. Write cache marker' bench/run.py + +# No opt-out was introduced +grep -niE 'allow.unattributable|skip.unattributable|--allow|accept_body_only|ignore_unattributable' bench/run.py ; echo "exit=$? (expect 1)" + +# Fixtures untouched +python3 -c 'import hashlib,sys;print(hashlib.sha256(open(sys.argv[1],"rb").read()).hexdigest())' bench/testdata/capture-numbered-findings-h3.md +python3 -c 'import hashlib,sys;print(hashlib.sha256(open(sys.argv[1],"rb").read()).hexdigest())' bench/testdata/sample-report.md # expect de40c00e7d3c452fa7475be9fa6541426058a96dba91487460aa53be0bd186ae +python3 -c 'import hashlib,sys;print(hashlib.sha256(open(sys.argv[1],"rb").read()).hexdigest())' bench/testdata/real-capture-report.md # expect be1400f065d6b856910e7ac91c7f4801598b57afb444f55cf2e257a43619f4db + +# AC13 — the real capture now splits five findings from two unattributable items +python3 - <<'EOF' +import sys, json, pathlib +sys.path.insert(0, 'bench') +import run as R +ids = R.load_rule_ids(pathlib.Path('.')) +r = R.harvest(pathlib.Path('bench/testdata/capture-numbered-findings-h3.md').read_text(), ids) +print('findings', len(r.findings), 'unattributable', len(r.unattributable)) +for u in r.unattributable: + print(' ', json.dumps(u)) +EOF + +# Assertion floors (AC15) +grep -cE '^\s*(self\.assert|assert )' bench/test_config.py # expect >= 63 +grep -cE '^\s*(self\.assert|assert )' bench/test_resolve.py # expect >= 46 +grep -cE '^\s*(self\.assert|assert )' bench/test_review.py # expect >= 165 + +# Full suite and repository gate +python3 -m unittest discover -s bench -p 'test_*.py' -v 2>&1 | tail -50 +make precommit +``` + +Expected: the marker is declared with the exact literal `UNATTRIBUTABLE_MARKER = "UNATTRIBUTABLE FINDING"` (a name-only match is not sufficient — the spelling is the invariant), `unattributable_report` is present, and the step comments appear in the order 4, 5, 6, 6a, 7, 8; the opt-out grep exits 1; the three digests are unchanged; the replay prints `findings 5 unattributable 2` with both entries carrying `"section": "Nice to Have"`; the unittest run reports `OK` with `Ran N tests`, `N > 72`, and lists the unattributable-capture, loud-failure and attributed-row tests by name; `make precommit` exits 0. + diff --git a/prompts/in-progress/026-spec-005-harvest-contract-docs-and-changelog.md b/prompts/in-progress/026-spec-005-harvest-contract-docs-and-changelog.md new file mode 100644 index 0000000..e020fa0 --- /dev/null +++ b/prompts/in-progress/026-spec-005-harvest-contract-docs-and-changelog.md @@ -0,0 +1,240 @@ +--- +status: approved +spec: [005-bug-bench-harvest-finding-extraction] +created: "2026-08-08T11:44:00Z" +queued: "2026-08-08T12:14:20Z" +--- + + +- The benchmark's own documentation now states the rules the code actually follows for turning review text into findings +- It says what ends a findings section, including the bold-label block that previously defeated the boundary and produced invented findings +- It says both list styles open a finding, so the next reader does not re-derive the rule that lost the reviewer's most severe tier +- It says where a finding's file, line and rule attribution comes from, and that it comes from the reviewer's own markers rather than the runner's bookkeeping +- It names the loud refusal by its literal marker, so an operator hitting it can act without reading the runner's source +- The fixtures table lists all four new captures with their origin, fingerprint and expected result, so the next fixture author copies real output instead of transcribing a template +- The changelog entry describes the whole change — invented findings, dropped findings, missing attribution, loud refusal — rather than the last slice of it +- That matters because the release classifier reads the entry to decide how big the change was, and an under-described entry once drew the wrong version bump +- Final sweeps confirm no personal filesystem paths and no third-party dependencies anywhere in the benchmark +- Last of five prompts, deliberately last so the documentation describes what actually shipped + + + +Write the corrected harvest contract into `bench/README.md` § "Reading review output" — what ends a findings section, both list styles that open one, where attribution is read from, and what happens to an item that cannot be attributed — extend its fixtures table with the four new captures, and consolidate `CHANGELOG.md`'s `## Unreleased` section so one reader sees the whole of spec 005 rather than this prompt's slice. Then run the repository-wide sweeps that close the spec's container-verifiable criteria. + + + +Read `CLAUDE.md` for project conventions (Python 3 standard library only, no personal paths, generic examples only, never commit — dark-factory handles git). + +Read `specs/in-progress/005-bug-bench-harvest-finding-extraction.md`. This prompt satisfies the contract half of **Desired Behavior 6** and **Acceptance Criteria AC1, AC15, AC16, AC17 and AC18**. Load-bearing sections: `## Acceptance Criteria` AC15/AC16/AC17/AC18 **including all three anti-keyword-stuffing clauses**, `## Why this is a bug`, `## Constraints`, and `## Suggested Decomposition`'s rationale for putting this prompt last. + +**This prompt depends on prompts 1-4 of this spec having landed.** Verify before you start: + +```bash +grep -n 'BOLD_RUN_START_RE\|ORDERED_ITEM_RE\|def extract_attribution\|UNATTRIBUTABLE_MARKER\|def unattributable_report' bench/run.py +``` + +If any of the five is absent, stop and report `status: failed` with the message `"prompts 1-4 of spec 005 not yet landed"`. Do not implement them here. + +Read `bench/run.py` — **take every documented fact from the code, not from this prompt and not from memory.** In particular read: `BOLD_RUN_START_RE`, `BULLET_RE`, `ORDERED_ITEM_RE`, `THEMATIC_BREAK_RE`, `FENCE_RE`, `RULE_TAG_RE`, `HEAD_RULE_TAG_RE`, `LEADING_BOLD_RE`, `PATH_LINE_RE`, `LINE_MENTION_RE`, `UNATTRIBUTABLE_MARKER`, `list_item_body`, `_normalize_body`, `extract_attribution`, `harvest`, `HarvestResult`, `unattributable_report`, and step 6a of `process_pr`. Quote the real marker literals and the real behaviour; do not describe anything the code does not do. + +Read `bench/README.md`. You extend `## Reading review output`, which currently has the sub-sections "The three required sections", "What ends a findings section", "What opens a finding" and "Fixtures", and you update `## Fixed invariants`, whose "List-item markers" bullet still says only `-` and `*` open a finding. Match the file's voice: short declarative sections, a `###` heading per topic, tables where the shape fits, and a `> **Why …**` paragraph wherever a reader would otherwise restore the wrong behaviour. + +Read `CHANGELOG.md`. The frozen preamble runs from `# Changelog` down to the last `* PATCH …` bullet; the newest section is `## v0.35.3`. `## Unreleased` goes immediately after that preamble and directly above `## v0.35.3`. Note that v0.35.1's three `- bench: …` bullets are the file's only non-conforming entries and are exactly the release that under-described a large change. + +Read `docs/changelog-guide.md` — `RULE changelog/preamble-frozen` and `RULE changelog/conventional-prefix-required`, plus the prefix→bump table. + +Read `docs/dod.md` — no personal paths anywhere, a `## Unreleased` CHANGELOG entry required. + + + + +## 1. Rewrite `bench/README.md` § "Reading review output" + +Four topics, **each its own `###` sub-section with its own prose block of at least two non-empty lines**. One keyword-dense line satisfies every grep while documenting nothing; the four topics must also match on at least four distinct line numbers. + +**Topic 1 — what ends a findings section.** Update the existing "What ends a findings section". A section's content ends at the next markdown heading of any level, at a thematic break, at a line outside a fenced block whose first non-whitespace content opens a **bold run**, or at end of input — whichever comes first. Use the words "bold run" and "bold label". Name the two shapes that were observed defeating the old boundary: `**Notes:**` and `**Summary**:`. State that termination never depends on the terminating heading's level relative to the section's own, because level varies run to run inside a single mode. Add: + +> **Why a bold label ends a section.** A review whose three severity sections all read `None.` — the correct answer, zero — was harvested as three findings, verbatim the three bullets of a trailing `**Notes:**` block. A bold label introduces a new block, and everything under it belongs to that block. + +**Topic 2 — what opens a finding.** Update the existing "What opens a finding". Both list styles open one: an unordered item (`-` or `*` followed by whitespace) and an **ordered/numbered** item (a run of digits followed by `.` and whitespace). Use both the words "numbered" and "ordered list". State that ordered numbering need not start at 1 and is not limited to one digit; that non-list lines following an open item extend it; that a line inside a fenced code block is ordinary text and opens nothing; and that prose appearing before the first item — most importantly the mandated `None.` sentinel — contributes no finding. State that only the list marker is stripped from the body and the item's leading bold run is preserved verbatim. Add: + +> **Why numbered items matter most.** The reviewer writes its more severe tiers as numbered lists. A capture carrying five numbered Should Fix items harvested to two — the two Nice to Have bullets — so the parser lost precisely the findings that matter most, and left no trace of the loss. + +**Topic 3 — where attribution comes from.** A new `###` sub-section. `rule_id` has **two sources, in strict priority order**, and the README must document both — a doc that names only the first would be false about the shipped parser, which is the docs-disagree-with-code mechanism behind D1, D2, D4 and D7: + +1. The item's own inline `*(rule: \`\`)*` marker. When present it always wins and is recorded as the literal string the reviewer wrote, **whether or not that id appears in `rules/index.json`**. +2. **Only when no marker is present**, a backticked token at the very head of the item that **is a member of `rules/index.json`**. This legacy shape is what three findings in the frozen `bench/testdata/sample-report.md` depend on, and it stays index-gated on purpose: at the head of an item an unknown backticked token is far more likely to be a file path or a symbol than a rule name. State this asymmetry explicitly rather than glossing it. + +Under either source, an id named in the item's prose, in the item's tail, in a traceability table, or anywhere outside the item is never attributed to it. `path` and `line` are read from the bold run at the head of the item, in the shapes the reviewer writes; a path or line appearing only in the trailing prose is not used when the leading bold run supplies one; no path is ever inferred by searching the repository and no line is ever guessed from surrounding text. Use the literal text `rule: ` and the phrase "inline rule tag" or "inline rule marker". Include the four observed bold-reference shapes as a small table with their resulting `path` and `line`. Add: + +> **Why the inline marker is not validated against the rule index.** A tag naming a renamed rule, a rule added since the index was written, or a review run against a different rules revision would otherwise yield `null` — attribution would become a property of the runner's bookkeeping rather than of the reviewer's output. The instrument records what the reviewer claimed; reconciling that against the shipped rule set is the scorer's job, and coupling the two hides rule drift instead of surfacing it. The head-anchored legacy source keeps its index gate because it has no explicit marker to trust: there, membership is the only signal that a backticked token is a rule name at all. + +**Topic 4 — when a finding cannot be attributed.** A new `###` sub-section containing the literal string `UNATTRIBUTABLE FINDING`. An item inside a severity section that yields neither a `path` nor a `rule_id` cannot be keyed, cannot be matched against a golden set, and is never written down as a body-only finding. Such a PR fails in the same class as the existing `NOT A REVIEW` gate: no ledger row, no `.json` row marker, the PR listed as failed, the remaining PRs still processed, the process exits non-zero. State the deliberate difference between the two gates — `NOT A REVIEW` fires before the raw-output write and leaves nothing behind, while this gate fires after it, so the `.stdout.txt` stays on disk and the review is re-harvestable after a parser fix without spending tokens again — and that a both-stream failure artifact is written under `bench/.cache/failures/`. State that there is no opt-out. Add: + +> **Why a body-only finding is refused.** A finding with no path and no rule id is an unmatchable measurement dressed as a data point. A false rejection costs one operator decision and a re-run; a false acceptance writes an unscoreable row into an append-only ledger. + +## 2. Extend the fixtures table + +Replace the existing two-row table under `### Fixtures` with a six-row table carrying **Fixture / Origin / `sha256` / Harvests to** for all six files. The two pre-existing rows keep their content. The four new rows: + +| Fixture | Origin | `sha256` | Harvests to | +|---|---|---|---| +| `bench/testdata/capture-notes-block-h2.md` | verbatim capture, `node-skeleton#2` short mode, `##` headings, all three sections `None.` with a trailing `**Notes:**` block | `6427028bef301ff822cca6dbf9308896f1899ac5a972ed3fddc276f2216552b9` | 0 findings, 0 unattributable | +| `bench/testdata/capture-numbered-findings-h3.md` | verbatim capture, `python-skeleton#3` full mode, `###` headings, five numbered Should Fix items with inline rule tags plus a positive-notes list | `5530049fa4d116dc5762b69c9c9498ff0865c0ae0c6b1de7b3ae4cc846643e93` | 5 findings, 2 unattributable | +| `bench/testdata/capture-traceability-h4.md` | verbatim capture, `node-skeleton#2` full mode, `####` headings, one bold-headed finding plus a 22-row traceability table | `2922746bb95bdb3a67a683942531362271d8f3ccd558067d910146e054bcfe7c` | 1 finding, 0 unattributable | +| `bench/testdata/capture-summary-trailer-h4.md` | verbatim capture, `github-pr-review-agent#11` short mode, `####` headings, all three sections `None.` with a `**Summary**:` trailer | `36e15eca61133033d81687f87a82b044333c6a7465508d1757f8493361137e79` | 0 findings, 0 unattributable | + +Confirm every digest against the file before writing it (`shasum -a 256 | cut -d' ' -f1`) and confirm every "Harvests to" figure by replaying the harvester over the fixture. Do not copy a number from this prompt without checking it. + +Keep and strengthen the existing closing sentence: a fixture for a new defect must be a **capture of real output**, not a transcription of the template. Add that every capture checked in before this change carried zero findings, so no fixture in the repository had ever exercised the parser against a real finding — which is how this defect family reached five occurrences. + +## 3. Update `## Fixed invariants` + +- Replace the "List-item markers" bullet: both `-`/`*` unordered items and digit-run ordered items open a finding; prose before the first item cannot form one; a list item inside a fenced code block opens nothing. +- Add a "Section terminators" bullet: next heading of any level, thematic break, bold-run line outside a fence, end of input. +- Add an "Inline rule tag" bullet: `*(rule: \`\`)*`, read positionally from the item and recorded literally, **not validated against `rules/index.json`** — and, in the same bullet or an adjacent one, the head-anchored fallback used only when no marker is present, which **is** index-gated. Do not write "never validated against `rules/index.json`" unqualified; that claim is false for the fallback path. +- Add an "Unattributable-item rejection" bullet naming `UNATTRIBUTABLE FINDING` and stating that it is not configurable and has no opt-out. +- Leave every other bullet, and the `## Safety invariant` and `## Result row` sections, untouched. The `findings` row-schema entry is still `Normalised list of {path, line, rule_id, body}` — the schema did not change. + +## 4. Write the `CHANGELOG.md` `## Unreleased` entry + +Insert `## Unreleased` immediately after the frozen preamble and directly above `## v0.35.3`. Do not touch the preamble and do not touch any released section. + +Every bullet must match `^- (fix|feat|docs): `. The non-conforming `- bench: ` style must not appear. The section needs **at least three bullets**, and the three topic patterns must land on **three distinct bullet lines** — one bullet naming all three defects passes the loose greps while under-describing the change, which is the failure that drew a patch bump on v0.35.1. + +Write one bullet per shipped behaviour, describing the whole of spec 005 and not this prompt's slice: + +1. `fix:` — a trailing bold-label block (`**Notes:**`, `**Summary**:`) now ends a findings section, so housekeeping bullets after a clean review are no longer invented as findings. Must match `notes|trailing|invent|phantom`. +2. `fix:` — numbered/ordered items open a finding exactly as bullets do, so the reviewer's most severe tier is no longer dropped; a list item inside a fenced block opens nothing; the item's leading bold run survives marker stripping. Must match `numbered|dropped`. +3. `fix:` — `path`, `line` and `rule_id` are read from the reviewer's own markers — the item's leading bold reference and its inline `*(rule: …)*` tag — the inline tag's id recorded literally and independent of the runner's copy of `rules/index.json`, with the head-anchored legacy shape retained as an index-gated fallback. Must match `rule id|path|attribut`. +4. `fix:` — an item inside a severity section that yields no attribution now fails the PR loudly with `UNATTRIBUTABLE FINDING`, leaving no ledger row and no row marker while preserving the raw capture, in the same class as the existing `NOT A REVIEW` gate. +5. `docs:` — the harvest contract in `bench/README.md` and four verbatim capture fixtures that lock it down. + +## 5. Run the closing sweeps + +- **AC18 — no personal paths, no third-party dependencies.** `grep -rn '/Users/\|~/Documents/' bench/` must return nothing (exit 1). Every `import` / `from` line in `bench/run.py` must name a Python 3 standard-library module only — list them and check each. +- **AC15 — no test deleted, no test file gutted.** Use the **immutable tag `v0.35.3`** as the diff baseline, not `origin/master`. `origin/master` is a moving ref: if the daemon pushed between prompts 1 and 5 it has advanced past prompts 1-4, so a diff against it covers only prompt 5 — which touches no test file — and AC15 goes vacuously green on exactly the work it polices. It also fails green when it does not resolve at all, because git writes to stderr and `grep -c` then prints `0`. + + Resolve the baseline in this order: `v0.35.3^{commit}` if it verifies; else `origin/master^{commit}` with an explicit warning in your report that the result may be vacuous; else neither, in which case print `BASELINE UNAVAILABLE`, say so in your report, and **do not report AC15's diff checks as passing**. + + With a baseline, `git diff "$BASE" -- bench/test_config.py bench/test_resolve.py bench/test_review.py | grep -c '^-.*def test_'` must return `0` — list the three files explicitly rather than using the `bench/test_*.py` glob, which expands against the working tree and would silently miss a file that was deleted. `git diff "$BASE" -- bench/testdata/sample-report.md bench/testdata/real-capture-report.md` must be empty. The per-file assertion floors must hold regardless of baseline availability: `bench/test_config.py` ≥ 63, `bench/test_resolve.py` ≥ 46, `bench/test_review.py` ≥ 165. +- **AC1 — the suite grew and the gate is green.** `python3 -m unittest discover -s bench -p 'test_*.py'` reports `OK` with `Ran N tests`, `N > 72`; `make precommit` exits 0. +- **Fixture provenance.** All four capture digests still match the published values, and the two pre-existing fixtures still match `de40c00e7d3c452fa7475be9fa6541426058a96dba91487460aa53be0bd186ae` (`sample-report.md`) and `be1400f065d6b856910e7ac91c7f4801598b57afb444f55cf2e257a43619f4db` (`real-capture-report.md`). + +If any sweep fails, fix the cause in the file that caused it. Never edit a fixture, never delete a test, never relax an assertion to make a sweep pass. + +## 6. Failure handling + +This prompt writes documentation and a changelog entry; it changes no runtime behaviour. If a documented fact and the code disagree, the **code** is authoritative for what the documentation says — re-read `bench/run.py` and document what it does. Do not change `bench/run.py` here to match a sentence you wrote; if the code is genuinely wrong, stop and report the discrepancy. + + + + +- **Changes land only in `bench/README.md` and `CHANGELOG.md`.** Do NOT change `bench/run.py`, `bench/test_*.py` or `bench/testsupport.py` in this prompt. +- **Never create, regenerate, overwrite or edit any file under `bench/testdata/`.** On a digest mismatch, stop and report failed. +- **The CHANGELOG preamble is frozen** (`RULE changelog/preamble-frozen`): `# Changelog` down to the last `* PATCH …` bullet is untouched, and no released section is edited. +- **CHANGELOG entries use conventional prefixes** `fix:` / `feat:` / `docs:` per `docs/changelog-guide.md`. The `- bench: ` style used in v0.35.1 is not repeated — dark-factory's version-bump detector reads the prefix. +- **The entry describes the whole change, not this prompt's slice.** +- **No test function may be deleted and no assertion relaxed.** Per-file assertion floors: `bench/test_config.py` ≥ 63, `bench/test_resolve.py` ≥ 46, `bench/test_review.py` ≥ 165. Suite count strictly greater than 72. +- **Do NOT document a behaviour the code does not have**, and do NOT document a knob, flag or opt-out — there are none, and describing one would invite an implementer to add it. +- No personal paths (`/Users/`, `~/Documents/`) in any shipped file, including the README. +- Python 3 standard library only; no third-party dependencies anywhere under `bench/`. +- Do NOT change `bench/prs.json`, `commands/pr-review.md`, or any rule, agent, command or doc that participates in a review. +- Do NOT commit — dark-factory handles git. + + + +``` +# Prompts 1-4 landed (precondition, not a new check) +grep -n 'BOLD_RUN_START_RE\|ORDERED_ITEM_RE\|def extract_attribution\|UNATTRIBUTABLE_MARKER\|def unattributable_report' bench/run.py + +# AC16 — the four README patterns, each present +grep -ciE 'bold label|bold run' bench/README.md # expect >= 1 +grep -ciE 'numbered|ordered list' bench/README.md # expect >= 1 +grep -ciE 'rule: |inline (rule )?(tag|marker)' bench/README.md # expect >= 1 +grep -cF 'UNATTRIBUTABLE FINDING' bench/README.md # expect >= 1 +grep -ciE 'rules/index\.json' bench/README.md # expect >= 2 +grep -niE 'rules/index\.json' bench/README.md | cut -d: -f1 | sort -u | wc -l # expect >= 2 distinct lines +grep -ciE 'head-anchored|head of the item' bench/README.md # expect >= 1 + +# AC16 anti-keyword-stuffing — the four patterns match on >= 4 distinct lines +{ grep -niE 'bold label|bold run' bench/README.md; \ + grep -niE 'numbered|ordered list' bench/README.md; \ + grep -niE 'rule: |inline (rule )?(tag|marker)' bench/README.md; \ + grep -nF 'UNATTRIBUTABLE FINDING' bench/README.md; } | cut -d: -f1 | sort -u | wc -l # expect >= 4 + +# AC16 — each topic is its own sub-section +grep -n '^### ' bench/README.md + +# AC16 — the fixtures table lists all four new captures with their digests +grep -c 'capture-notes-block-h2.md\|capture-numbered-findings-h3.md\|capture-traceability-h4.md\|capture-summary-trailer-h4.md' bench/README.md # expect >= 4 +grep -c '6427028bef301ff8\|5530049fa4d116dc\|2922746bb95bdb3a\|36e15eca61133033' bench/README.md # expect >= 4 + +# AC17 — the Unreleased section, its prefixes and its bullet count +sed -n '/^## Unreleased/,/^## v/p' CHANGELOG.md | tee /tmp/unreleased.txt +test -s /tmp/unreleased.txt && echo "unreleased section non-empty" +grep -c '^- ' /tmp/unreleased.txt # expect >= 3 +grep -c '^- bench: ' /tmp/unreleased.txt # expect 0 +grep -E '^- ' /tmp/unreleased.txt | grep -cvE '^- (fix|feat|docs): ' ; echo "non-conforming bullets above (expect 0)" +grep -ciE 'notes|trailing|invent|phantom' /tmp/unreleased.txt # expect >= 1 +grep -ciE 'numbered|dropped' /tmp/unreleased.txt # expect >= 1 +grep -ciE 'rule id|path|attribut' /tmp/unreleased.txt # expect >= 1 + +# AC17 anti-keyword-stuffing — three distinct bullet lines carry the three topics +{ grep -niE 'notes|trailing|invent|phantom' /tmp/unreleased.txt; \ + grep -niE 'numbered|dropped' /tmp/unreleased.txt; \ + grep -niE 'rule id|path|attribut' /tmp/unreleased.txt; } | cut -d: -f1 | sort -u | wc -l # expect >= 3 + +# The frozen CHANGELOG preamble was not disturbed +head -12 CHANGELOG.md +grep -n '^## ' CHANGELOG.md | head -3 # expect '## Unreleased' first, then '## v0.35.3' + +# AC18 — no personal paths, stdlib only +grep -rn '/Users/\|~/Documents/' bench/ ; echo "exit=$? (expect 1)" +grep -nE '^(import|from) ' bench/run.py + +# AC15 — no test deleted, fixtures frozen, assertion floors held +# AC15 baseline: use the IMMUTABLE tag, not the moving branch ref. +# `origin/master` fails green two ways: if it does not resolve, git writes to stderr, +# `grep -c` prints 0 and the anti-gutting gate passes; and if the daemon pushed between +# prompts 1 and 5 it has advanced past prompts 1-4, so the diff covers only prompt 5 — +# which touches no test file — making AC15 vacuously green on exactly the work it polices. +if git rev-parse --verify -q 'v0.35.3^{commit}' >/dev/null; then + BASE=v0.35.3 +elif git rev-parse --verify -q 'origin/master^{commit}' >/dev/null; then + BASE=origin/master + echo "WARNING: tag v0.35.3 unavailable; falling back to the moving ref origin/master." + echo " If it has advanced past prompts 1-4 these diffs are vacuous — say so in your report." +else + BASE= + echo "BASELINE UNAVAILABLE: neither v0.35.3 nor origin/master resolves in this environment." + echo "Report this explicitly; do NOT report AC15's diff checks as passing." +fi +if [ -n "$BASE" ]; then + git diff "$BASE" -- bench/test_config.py bench/test_resolve.py bench/test_review.py | grep -c '^-.*def test_' # expect 0 + git diff "$BASE" -- bench/testdata/sample-report.md bench/testdata/real-capture-report.md # expect empty +fi +grep -cE '^\s*(self\.assert|assert )' bench/test_config.py # expect >= 63 +grep -cE '^\s*(self\.assert|assert )' bench/test_resolve.py # expect >= 46 +grep -cE '^\s*(self\.assert|assert )' bench/test_review.py # expect >= 165 + +# Fixture provenance, all six — CHECKED, not printed. A print loop compares against +# nothing: a mismatched fixture would emit output an agent reports as a pass, reducing +# the provenance gate to decoration. +shasum -a 256 -c - <<'SHA' +6427028bef301ff822cca6dbf9308896f1899ac5a972ed3fddc276f2216552b9 bench/testdata/capture-notes-block-h2.md +5530049fa4d116dc5762b69c9c9498ff0865c0ae0c6b1de7b3ae4cc846643e93 bench/testdata/capture-numbered-findings-h3.md +2922746bb95bdb3a67a683942531362271d8f3ccd558067d910146e054bcfe7c bench/testdata/capture-traceability-h4.md +36e15eca61133033d81687f87a82b044333c6a7465508d1757f8493361137e79 bench/testdata/capture-summary-trailer-h4.md +de40c00e7d3c452fa7475be9fa6541426058a96dba91487460aa53be0bd186ae bench/testdata/sample-report.md +be1400f065d6b856910e7ac91c7f4801598b57afb444f55cf2e257a43619f4db bench/testdata/real-capture-report.md +SHA +echo "shasum -c exit=$? (expect 0; any FAILED line means stop and report failed)" + +# AC1 — suite grew, gate green +python3 -m unittest discover -s bench -p 'test_*.py' 2>&1 | tail -5 +make precommit +``` + +Expected: the four README greps each return ≥1 on ≥4 distinct lines; the four new fixture names and their digest prefixes appear in the README; the extracted Unreleased section carries ≥3 bullets, all `fix:`/`feat:`/`docs:`, zero `- bench: `, with the three topics on ≥3 distinct bullet lines; `## Unreleased` is the first `## ` section and `## v0.35.3` the second; the personal-path grep exits 1; every `bench/run.py` import names a standard-library module; the deleted-test grep prints `0` and the frozen-fixture diff is empty; all six digests match their published values; the unittest run reports `OK` with `Ran N tests`, `N > 72`; `make precommit` exits 0. + diff --git a/specs/in-progress/005-bug-bench-harvest-finding-extraction.md b/specs/in-progress/005-bug-bench-harvest-finding-extraction.md new file mode 100644 index 0000000..417ceae --- /dev/null +++ b/specs/in-progress/005-bug-bench-harvest-finding-extraction.md @@ -0,0 +1,351 @@ +--- +status: prompted +tags: + - dark-factory + - spec +approved: "2026-08-08T11:24:09Z" +generating: "2026-08-08T12:14:23Z" +prompted: "2026-08-08T12:14:23Z" +branch: dark-factory/bug-bench-harvest-finding-extraction +--- + +## Summary + +- The bench runner's HARVEST layer turns review output into a list of findings. It does not parse the shapes the reviewer actually emits, and it fails in both directions at once: it invents findings that were never written, and it drops findings that were. +- **Trailing housekeeping bullets become findings.** A review whose three severity sections all read `None.` — the correct answer, zero — was harvested as three findings, verbatim the three bullets of a trailing `**Notes:**` block. v0.35.2 gave sections an end boundary; a non-heading bold label is not one, so the section stays open and swallows whatever bullets follow. +- **Numbered findings are invisible.** The reviewer writes its more severe tiers as `1.` / `2.` / `3.` lists. Only `-` and `*` open a finding, so a capture carrying five numbered Should Fix items harvested to two — the two Nice to Have bullets. The parser loses precisely the findings that matter most. +- **Attribution is absent from every finding.** Across all runs to date the harvester has produced 30 findings with 9 paths, 9 lines, and zero rule ids — while the raw output carries inline `*(rule: …)*` tags and a bold `path:line` reference at the head of nearly every item. The data is in the output and nothing reads it. +- The governing repair: the harvester extracts what the review contract defines or refuses loudly. A finding it cannot attribute is a parse failure, not a body-only finding. This is the fifth defect in one family — a layer emitting a plausible wrong number instead of refusing — and the first four (D1, D2, D4, D7) all recurred because the fixtures were written from the same template the parser was written from. The fixtures this spec mandates are four verbatim captures of real review output, two of which carry real findings; every capture checked in before now carried zero, which is why no fixture in the repository has ever exercised the parser against a real finding. + +## Problem + +The benchmark exists to produce a number that can be compared across configurations. The HARVEST layer decides what that number is, and it is currently wrong in both directions on real input — simultaneously, on the same run. On a review that correctly found nothing, it recorded three findings scraped from a trailing notes block. On a review that found seven things, it recorded two, discarding all five of the numbered items and every path, line and rule id the reviewer had attached to them. Neither failure is visible in the result file: a fabricated finding is byte-shaped exactly like a real one, and a dropped finding leaves no trace at all. For a measurement instrument this is worse than a crash — a crash gets investigated, `3 findings` gets recorded, averaged into a score, and cited. It already has been: three AC18 runs from spec 004 were written up as evidence of model non-determinism, with spreads of 0–5 and 1–6 findings, before the raw captures showed the counts were partly tracking how many housekeeping bullets the reviewer happened to append. The noise floor is therefore unmeasured, and the downstream task "Bootstrap and Score the Golden Finding Set" is blocked outright, because precision and recall need a stable per-finding key — a path, a line, a rule id — and the harvester currently produces findings with none of the three. + +## Goal + +Every finding the runner writes down corresponds to an item the reviewer actually wrote inside a severity section, carries the attribution the reviewer attached to it, and is keyed stably enough to be matched against a golden set. Content the reviewer placed outside a severity section — a trailing notes block, a positive-notes list, a traceability table, a closing summary — is never a finding, at any heading level and whatever label introduces it. Items the reviewer wrote as numbered lists are findings exactly as bullets are. `path`, `line` and `rule_id` come from the reviewer's own markers in its own output, not from the runner's copy of the rule index. And an item inside a severity section that the runner cannot attribute fails that PR loudly, in the same class as the existing `NOT A REVIEW` gate — the runner never silently falls back to a body-only finding, because a body-only finding is an unmatchable measurement dressed as a data point. The fixtures that lock all of this down are verbatim captures of live review output carrying real findings, and the harvest contract is written down beside them. + +## Non-goals + +- Do NOT change any rule, agent, command, or doc that participates in a review, including `commands/pr-review.md`. The measured configuration stays fixed while the instrument is repaired — this is inherited unchanged from specs 003 and 004. If the loud-failure rule proves that the review command must mandate an attribution on every finding, that is a separate spec against the reviewed configuration, filed with its own baseline impact. +- 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. This spec makes the key that scoring needs exist; it does not consume it. +- Do NOT change the `NOT A REVIEW` gate's own semantics — which sections are required, what counts as a heading, the bounded excerpt, or what it accepts. This spec adds a second rejection class downstream of it and leaves the first one untouched. +- Do NOT change the plugin preflight, the ref pruning, or the failure-artifact writer shipped in v0.35.3. This spec touches the harvest layer and the per-PR control flow immediately around it. +- Do NOT re-harvest, migrate, or re-validate rows and raw outputs already written. Rows recorded before this change carry fabricated and missing findings; the recovery is to delete `bench/.cache/reviews/` and `bench/results/` and re-run, which the README already states. +- Do NOT infer a path by searching the repository for a filename mentioned in a finding's prose, and do NOT guess a line number from surrounding text. Attribution comes from the reviewer's markers or it does not come at all — inference is how an instrument starts agreeing with what you hoped. +- Do NOT make the section terminators, the list-item markers, the rule-tag marker, or the unattributable-item rejection configurable. All four are invariants; if a future consumer demands variation, that is a separate spec. +- Do NOT add an opt-out that lets a run accept body-only findings. An escape hatch on this Goal is the regression this spec exists to close. +- Do NOT change `bench/prs.json` — schema, entries, and `dev-1` version are frozen inputs. + +## Reproduction + +Runner version: `CHANGELOG.md` head is `v0.35.3`; bench suite is 72 tests, all green (`python3 -m unittest discover -s bench -p 'test_*.py'` → `Ran 72 tests` / `OK`). + +Raw captures live under `bench/.cache/reviews/*.stdout.txt`, which is gitignored (`.gitignore:9` — `/bench/.cache/`) and therefore local to the operator's host. Five captures are present from two runs, both `sonnet` / `medium`, one `short` and one `full`. Replaying the shipped harvester over them: + +```bash +python3 - <<'EOF' +import sys, json, pathlib +sys.path.insert(0, 'bench') +import run as R +ids = R.load_rule_ids(pathlib.Path('.')) +for f in sorted(pathlib.Path('bench/.cache/reviews').glob('*.stdout.txt')): + out = R.harvest(f.read_text(), ids) + print(f.name.split('__')[1], len(out)) + for o in out: + print(' ', json.dumps(o)[:160]) +EOF +``` + +### Observed, capture by capture + +| Capture | Mode | Heading level | Findings the reviewer wrote | Findings harvested | Correct | +|---|---|---|---|---|---| +| `dc7efd9d…__node-skeleton_2` | short | `##` | 0 | **3** | no — all three invented | +| `dc7efd9d…__github-pr-review-agent_11` | short | `####` | 0 | 0 | yes | +| `dc7efd9d…__tts-mcp_20` | short | `####` | 0 | 0 | yes | +| `ce1703bc…__node-skeleton_2` | full | `####` | 1 | 1 | body corrupted, unattributed | +| `ce1703bc…__python-skeleton_3` | full | `###` | 7 | **2** | no — five dropped | + +Aggregate over these five captures: 8 findings written, 6 harvested, of which 3 are genuine. Precision 3/6, recall 3/8. Zero of the 6 carry a non-null `path`, `line` or `rule_id`, while the raw output supplies a derivable path for 6 of the 8 true findings and an explicit rule tag for 2 of them. The operator's aggregate across all runs to date is the same picture at larger n: short mode 27 findings with 9 paths, 9 lines and 0 rule ids; full mode 3 findings with 0 of each. + +### RC1 — a trailing `**Notes:**` block is parsed as findings (D4, reopened) + +`dc7efd9d…__node-skeleton_2.stdout.txt`, verbatim: + +```text +## Must Fix (Critical) +None. + +## Should Fix (Important) +None. + +## Nice to Have (Optional) +None. + +**Notes:** +- precommit skipped (selector mode/short mode) — CI covers lint+test+typecheck +- `npm ci` was not run in this environment, so `tsc --noEmit` / `node --test` were not executed live; static review of all touched `.ts` files found no type errors, no `any`/`as`/`!`/`@ts-ignore`, and correct `.ts`-extension `require()` usage matching the documented `moduleResolution: "bundler"` invariant. Recommend confirming CI's `make check` is green before merge. +- LICENSE file present; README gained a License section pointing to it — harmless, unrelated to the TS conversion but fine. +``` + +Every severity section reads `None.`. The harvester returned three findings whose bodies are those three bullets. v0.35.2 gave a section an end boundary — the next heading, a thematic break, or end of input. `**Notes:**` is none of the three, so the Nice to Have section is still open when the bullets arrive and each one opens a finding inside it. + +### RC2 — numbered items are invisible + +`ce1703bc…__python-skeleton_3.stdout.txt`, under `### Should Fix (Important)`, verbatim: + +```text +1. **`CHANGELOG.md:18`** — `- ci: install trivy in CI` uses prefix `ci:`, not in the recognized set (`feat/fix/refactor/test/docs/chore/perf`). Breaks automated version-bump detection. Fix: use `chore:`. *(rule: `changelog/conventional-prefix-required`)* +2. **`README.md` "Security gates" section (~lines 76-94)** — rationale/ADR-style content (why no severity threshold, why `osv-scanner` excluded, cross-skeleton comparison table) belongs in `CLAUDE.md`/ADR, not user-facing README. Fix: trim to a short factual statement; move rationale elsewhere. *(rule: `readme/user-facing-not-agent-context`)* +3. **`.github/workflows/ci.yml:32`** — `sudo apt-key add -` is deprecated; can silently break on a future `ubuntu-latest` bump. Fix: use a keyring-based install or switch to `aquasecurity/setup-trivy` action. +4. **CI + `Makefile.precommit`** — Trivy has no version pin (unlike `PIP_AUDIT_VERSION ?= 2.9.0` set for pip-audit in the same PR), so CI and local runs can diverge over time. Fix: pin a Trivy version. +5. **`Makefile.precommit` `trivy` target** — no `--severity` filter and no documented rationale for failing on any severity, unlike the `audit` target which explicitly explains its "any severity" choice. Fix: either add a severity threshold or document the deliberate all-severity choice. +``` + +The harvester returned two findings for this PR — the two `- ` bullets under `### Nice to Have (Optional)`. All five numbered items were dropped. Spec 003 named this outcome in advance and chose it: its Non-goals say *"Do NOT broaden what opens a finding (numbered lists, bold-lead paragraphs, tables). No observed review output uses them."* Observed review output now does, in the reviewer's most severe tier. + +### RC3 — attribution is coupled to the runner's copy of the rule index + +The rule tag `*(rule: `changelog/conventional-prefix-required`)*` is present inline in the capture above. Feeding that item's text directly to the shipped extractor does return the id, so RC3 is not "nothing parses it" — the item is never parsed at all (RC2), and RC3 is what remains after RC2 is fixed: + +- `rule_id` is recovered only when the token happens to be a member of the id set loaded from `rules/index.json`. A tag naming a renamed rule, a rule added since the index was written, or a review run against a different rules revision yields `null` rather than the tag's literal value — attribution becomes a property of the runner's bookkeeping rather than of the reviewer's output. +- The extractor returns the **first** token anywhere in the item's text that is a member of that set. `ce1703bc…__node-skeleton_2` ends with a 22-row traceability table naming rule ids, and review prose routinely discusses rules by name, so a membership scan can attribute a finding to a rule the reviewer never tagged it with. +- `path` and `line` are likewise scanned from the whole item text rather than read from the leading bold reference, so the first `something.ext:NN` anywhere in a long body wins. + +### RC4 — heading level varies run to run, not by mode + +The captures show `##`, `###` and `####` all in use, and the level is **not** determined by mode: short mode emitted `##` on one PR and `####` on two others in the same run; full mode emitted `###` on one PR and `####` on another in the same run. The harvester already matches heading levels 1–6, so this is not an active defect — it is an untested invariant that any fix to item recognition can silently break, and the reason section termination must never key off a heading's level. It matters concretely: in `ce1703bc…__python-skeleton_3` the `### Positive notes` heading is what currently keeps four positive-note bullets out of the findings list, and that heading sits at the same level as the severity headings, while in `ce1703bc…__node-skeleton_2` a `### Traceability` heading terminates a `####` section from a shallower level. + +### Bonus defect, same layer + +`ce1703bc…__node-skeleton_2`'s single genuine finding is `- **No test coverage for `src/config.ts`'s new validation logic.** …`. The harvested body begins `*No test coverage for …` — one asterisk of the leading bold run is stripped as if it were a second list marker. The body is corrupted at exactly the position where the path reference lives, which is why it must be fixed as part of reading the leading bold reference rather than separately. + +## Expected vs Actual + +| | Expected | Actual | +|---|---|---| +| Review with all three sections `None.` and a trailing `**Notes:**` block | 0 findings — `bench/README.md` § "What opens a finding": prose before any list item "contributes no finding", and the notes block is not inside a findings section | 3 findings, bodies verbatim the notes bullets | +| Review with five numbered Should Fix items and two Nice to Have bullets | 7 findings | 2 findings | +| Finding tagged `*(rule: `changelog/conventional-prefix-required`)*` | `rule_id` = that string | `null` on every finding in every run to date | +| Finding headed `**`CHANGELOG.md:18`**` | `path` = `CHANGELOG.md`, `line` = 18 | `null`, `null` | +| Finding body `- **No test coverage …` | body begins `**No test coverage` | body begins `*No test coverage` | +| An item the runner cannot attribute | the PR fails loudly, no row written | a body-only finding is written and scored | + +## Why this is a bug + +`bench/README.md` § "Reading review output" states the harvest contract the runner is supposed to implement, and the runner does not implement it against real input: the section-boundary rule is documented as complete when a bold label defeats it, and the finding-opening rule is documented as `-` or `*` when the reviewer's severe tiers use `1.`. The same section states the fixture rule — *"A fixture for a new defect must be a capture of real output, not a transcription of the template"* — and that rule was followed only for captures that contained zero findings, so no fixture in the repository has ever exercised the parser against a real finding. That is precisely how this defect family reaches five occurrences: the parser, the fixtures and the documentation all agree with one another and all disagree with the model's actual output. The result is an instrument that reports a confident wrong number in both directions on the same run, which has already produced one published wrong conclusion about model determinism. + +## 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). The convention is inherited from specs 002, 003 and 004 — whose operator criteria found every defect in this family while the container suite found none. + +**Why the fixtures must be captures.** Four earlier defects in this family (D1, D2, D4, D7) recurred because each fix shipped with fixtures written alongside the parser from the same mental template, so the tests and the code agreed with each other and both disagreed with the model's real output. Every unit AC below that names a fixture requires a **verbatim capture of live review output**, and each is gated by a content-fidelity check on literals the review command's template cannot produce. + +- [ ] **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 > 72`. +- [ ] **AC2 [operator pre-step, then container]** Four captures are installed verbatim as fixtures under `bench/testdata/` and each matches its **published `sha256`** — evidence, for every row: `shasum -a 256 | cut -d' ' -f1` equals the stated digest exactly; `grep -c '' ` returns the stated line count; `grep -cE '^ (Must Fix|Should Fix|Nice to Have)' ` returns 3 and the same grep at each other level returns 0. + + | Fixture file | Capture | Level | Lines | `sha256` | + |---|---|---|---|---| + | `bench/testdata/capture-notes-block-h2.md` | `node-skeleton#2`, short mode — trailing `**Notes:**` block | `##` | 17 | `6427028bef301ff822cca6dbf9308896f1899ac5a972ed3fddc276f2216552b9` | + | `bench/testdata/capture-numbered-findings-h3.md` | `python-skeleton#3`, full mode — five numbered findings | `###` | 28 | `5530049fa4d116dc5762b69c9c9498ff0865c0ae0c6b1de7b3ae4cc846643e93` | + | `bench/testdata/capture-traceability-h4.md` | `node-skeleton#2`, full mode — traceability table | `####` | 57 | `2922746bb95bdb3a67a683942531362271d8f3ccd558067d910146e054bcfe7c` | + | `bench/testdata/capture-summary-trailer-h4.md` | `github-pr-review-agent#11`, short mode — `**Summary**:` trailer | `####` | 21 | `36e15eca61133033d81687f87a82b044333c6a7465508d1757f8493361137e79` | + + The four filenames are the contract: two fixtures sit at `####`, so a heading level does not identify a fixture and every reference below names a file. + + Plus `grep -rn '/Users/\|~/Documents/' bench/testdata/` returns 0 lines (exit 1). + + **The digest is the provenance gate; the line counts and heading greps are readability anchors only.** Published literals cannot serve as the gate: this spec quotes 13 of the `##` capture's 17 lines verbatim in RC1, and for the `###` capture RC2 quotes all five numbered items while AC5, AC10 Case A and AC13 publish their exact attribution, the positive-note count and the Nice-to-Have count — the entire asserted shape is already on the page, leaving only unasserted prose padding free. A `sha256` is the one anchor that can be published without enabling reconstruction. + + **Installation is an operator pre-step, not container work.** `.dark-factory.yaml` sets `worktree: false` and `.gitignore:9` excludes `/bench/.cache/`, so a container clone never contains the captures and has no way to obtain them. The operator copies the four files into `bench/testdata/` and commits them **before** `dark-factory spec approve`, so every prompt finds them already present and writes only tests against them. No prompt may create, regenerate, or reconstruct a fixture; a prompt that finds a digest mismatch stops and reports it rather than rewriting the file. + +- [ ] **AC3 [container]** The trailing-notes capture yields **nothing at all** — evidence: a unit test feeds `bench/testdata/capture-notes-block-h2.md` to the harvester and compares **both** components of the AC13 two-part result against empty: the findings list equals `[]` **and** the unattributable-item report equals `[]`; the assertion's failure message prints both. Asserting only the findings list lets an implementation reclassify the three `**Notes:**` bullets as unattributable items — findings is still empty, and the PR then fails loudly on a review that correctly found nothing, which is RC1 with the sign flipped. (This capture is the exact input that produced the three phantom findings.) +- [ ] **AC4 [container]** The bold-label terminator is general, not a hardcoded label, across **three** cases — evidence, all three: the assertion compares **both** components of the two-part result — findings list and unattributable report — against their expected values. Terminated content must land in neither component. + - Case A: a section reading `None.` followed by `**Notes:**` and three bullets → findings `[]` **and** unattributable report `[]`. + - Case B: the same shape with the label `**Summary**:` followed by prose and then a bullet → findings `[]` **and** unattributable report `[]`. (`**Summary**:` is the shape the `github-pr-review-agent#11` capture actually uses.) + - Case C: a section carrying **one real attributed bullet**, then `**Notes:**`, then two more bullets → exactly the one real finding, body unchanged. + + Cases B and C exist because matching the literal string `**Notes:**`, or dropping every finding that follows a `None.` sentinel, passes Case A alone. +- [ ] **AC5 [container]** The five previously-dropped numbered findings are extracted with the attribution the capture supplies — evidence: a unit test feeds `bench/testdata/capture-numbered-findings-h3.md` to the harvester and compares the Should Fix findings against this exact list, in order: + + | # | path | line | rule_id | + |---|---|---|---| + | 1 | `CHANGELOG.md` | 18 | `changelog/conventional-prefix-required` | + | 2 | `README.md` | 76 | `readme/user-facing-not-agent-context` | + | 3 | `.github/workflows/ci.yml` | 32 | null | + | 4 | `Makefile.precommit` | null | null | + | 5 | `Makefile.precommit` | null | null | + + The assertion compares paths, lines, rule ids and the count together and prints the full observed list on failure. +- [ ] **AC6 [container]** Ordered-item recognition is general, not fitted to the capture — evidence: a unit test over a synthetic section mixing `- `, `* ` and ordered items whose numbering starts at `3.` and includes `10.`, asserting one finding per item in document order; plus a negative case where an ordered item inside a fenced code block within a severity section yields no finding. (Without the non-1 start and the two-digit marker, a parser keyed to `1.`–`5.` passes AC5.) +- [ ] **AC7 [container]** `rule_id` comes from the reviewer's inline marker, with the head-anchored legacy shape as an index-gated fallback, across **four** cases — evidence, all four: the assertion compares the returned `rule_id` against the expected value. + - Case A: an item tagged `*(rule: `made-up/not-in-the-index`)*`, an id absent from `rules/index.json`, yields that literal string, not null. + - Case B: an item whose prose names a **different**, real rule id before its own `*(rule: …)*` marker yields the marker's id, not the prose one. + - Case C: an item carrying **no** marker whose body opens with a backticked token that **is** present in `rules/index.json` yields that id — the head-anchored legacy shape, which the byte-frozen `sample-report.md` depends on for three findings. + - Case D: the same item shape whose head token is **absent** from `rules/index.json` yields `None` — the legacy source stays index-gated, so an unknown backticked token at the head of an item is not recorded as a rule. + + Case B exists because a first-membership-match scan passes Case A. Cases C and D exist because the legacy source is otherwise held in place only by `TestHarvestNormalizesSampleReport`, an indirect guard that would not survive a refactor of that fixture's test, and because C without D would let the fallback drift into recording arbitrary head tokens. Together with AC10's traceability-table check, these close attribution-by-proximity in both directions. +- [ ] **AC8 [container]** `path` and `line` come from the leading bold reference, across **four** shapes — evidence: unit assertions on `(path, line)` pairs for each: `**`CHANGELOG.md:18`**` → (`CHANGELOG.md`, 18); `**`README.md` "Security gates" section (~lines 76-94)**` → (`README.md`, 76); `**`.github/workflows/ci.yml:32`**` → (`.github/workflows/ci.yml`, 32); `**CI + `Makefile.precommit`**` → (`Makefile.precommit`, null). Plus a negative case: an item whose leading bold reference names `a/b.py:10` and whose trailing prose mentions `c/d.py:99` resolves to (`a/b.py`, 10). +- [ ] **AC9 [container]** The body preserves the leading bold run verbatim — evidence: a unit test feeds `bench/testdata/capture-traceability-h4.md` to the harvester and asserts exactly one finding whose `body` starts with the literal `**No test coverage for` (two asterisks) and whose `path` is `src/config.ts`. (The shipped parser produces `*No test coverage for` and `path` null.) +- [ ] **AC10 [container]** Content outside a severity section is never a finding and never attributes one, across **three** cases — evidence, all three: assertions on the full returned list. + - Case A: `bench/testdata/capture-numbered-findings-h3.md`'s four `### Positive notes` bullets contribute zero findings **and** appear in no unattributable report — content outside a severity section is not a finding and is not a parse failure either. (None of those four bullets carries a path or a rule tag, so classifying them as unattributable would fail the PR on a correctly-reviewed capture.) + - Case B: no finding harvested from `bench/testdata/capture-traceability-h4.md` carries a `rule_id` drawn from its 22-row `### Traceability` table. + - Case C: the two zero-finding captures each harvest to the empty list — the pre-existing `bench/testdata/real-capture-report.md`, and `bench/testdata/capture-summary-trailer-h4.md`. +- [ ] **AC11 [container]** An item the runner cannot attribute fails the PR loudly — evidence: with a stub `claude` on `PATH` printing a review whose Should Fix section carries one item with neither a path reference nor a rule tag, run the runner over a one-PR temp manifest → process exit code non-zero; stderr contains the literal `UNATTRIBUTABLE FINDING`, the PR id, the severity section name, and the item's text verbatim; the results file gains 0 lines; and under the **temp cache root the test passed to the runner** (`reviews_root(cache_root)`, `bench/run.py:305` — never `bench/.cache/`, which is gitignored at `.gitignore:9`, absent in a fresh clone, and therefore a vacuous probe) exactly **one** file exists, the `.stdout.txt` raw capture, and **zero** `*.json` row markers; a both-stream failure artifact exists under `failures_root(cache_root)` carrying the rejected output; the stdout summary reports `1 failed`. + + The `.json` count is the discriminator, not the total entry count: the runner writes the raw stdout at step 5 *before* harvesting at step 6 (`bench/run.py:1404-1410`, comment "Write raw stdout verbatim before any parsing"), so a harvest-derived gate necessarily fires with the raw file already on disk. Requiring zero total entries would force either reordering harvest ahead of that write or deleting the raw output — both regressions, the second one destroying the text the Failure Modes table promises is re-harvestable after a fix. +- [ ] **AC12 [container]** The loud failure is not a blanket rejection, and AC11's probe is live — evidence: the same runner invocation with the same review shape, the single item now headed `**`src/x.py:4`**`, exits 0; the results file gains exactly 1 line; `jq -r '.findings[0].path'` on that row prints `src/x.py`; and the **same probe AC11 uses**, run here, finds **two** files under `reviews_root(cache_root)` — one `.stdout.txt` and exactly **one** `.json` row marker — against AC11's one-and-zero. + + The success path writes two files, the raw capture at step 5 and the row marker at step 8 (`bench/run.py:1437`); stating "1" would be off by one and an implementer observing 2 would "fix" the assertion, silently dissolving the AC11/AC12 pairing. The `*.json` count (1 here, 0 in AC11) is what proves the PR is cache-served next run in one case and retried in the other — the cache check keys on the row marker alone (`bench/run.py:1349-1350`). +- [ ] **AC13 [container]** The unattributable items in the real capture are reported as such, not silently dropped and not silently kept — evidence: a unit test over `bench/testdata/capture-numbered-findings-h3.md` asserts that harvesting reports both `### Nice to Have (Optional)` bullets as unattributable, quoting each verbatim, **and** that the five Should Fix findings from AC5 are present in the parse result. This is the honest consequence of the governing rule on a genuine review: that PR fails loudly rather than contributing two body-only findings. +- [ ] **AC14 [container]** Heading level is irrelevant to both termination and harvesting — evidence: a unit test renders identical section content at `##`, `###` and `####` and asserts all three harvest to the same list; a second case asserts a `###` heading terminates an open `##` section and a `##` heading terminates an open `####` section; the test name contains `heading_level`; the assertion prints all lists on failure. +- [ ] **AC15 [container]** No test was deleted **and no test file was gutted** — evidence, all four, each a binary mechanical check: + - `git diff origin/master -- bench/test_*.py | grep -c '^-.*def test_'` returns 0 (no test function removed). + - **Per-file assertion floors**, each its own check: `grep -cE '^\s*(self\.assert|assert )' bench/test_config.py` ≥ 63; the same on `bench/test_resolve.py` ≥ 46; the same on `bench/test_review.py` ≥ 165. These are the `origin/master` counts. + - `git diff origin/master -- bench/testdata/sample-report.md bench/testdata/real-capture-report.md` is empty. + - Combined with AC1's `N > 72`. + + Per-file floors, not a repo-wide total: the glob `bench/test_*.py` admits new files, so a repo-wide floor of 274 is satisfied by adding `bench/test_harvest.py` with 200 assertions while cutting `test_review.py` from 165 to 20 — and `test_review.py` is exactly the file this work touches. A floor per existing file cannot be paid for with a new one. + +- [ ] **AC16 [container]** `bench/README.md` § "Reading review output" states the corrected contract — evidence: `grep -ciE 'bold label|bold run' bench/README.md` ≥1; `grep -ciE 'numbered|ordered list' bench/README.md` ≥1; `grep -ciE 'rule: |inline (rule )?(tag|marker)' bench/README.md` ≥1; `grep -ciE 'rules/index\.json' bench/README.md` ≥2 **on at least two distinct line numbers** — one for the inline marker's exemption from the index, one for the head-anchored fallback's index gate; `grep -ciE 'head-anchored|head of the item' bench/README.md` ≥1; `grep -cF 'UNATTRIBUTABLE FINDING' bench/README.md` ≥1; the fixtures table lists all **four** new capture fixtures with their origin, `sha256` and expected harvest — `bench/testdata/` previously held only `sample-report.md` and `real-capture-report.md`, so every AC2 fixture is new. + + **Anti-keyword-stuffing:** the four patterns must match on **at least four distinct line numbers** (union of `grep -n` line numbers has ≥4 unique values), and each of the four topics must be its own prose block of ≥2 non-empty lines. One keyword-dense line satisfies every grep while documenting nothing. +- [ ] **AC17 [container]** The CHANGELOG entry describes the whole change so a release classifier can weigh it — evidence: the section from `## Unreleased` to the next `## ` line is non-empty; every bullet in it matches `^- (fix|feat|docs): `; `grep -c '^- bench: '` over it returns 0; within it `grep -ciE 'notes|trailing|invent|phantom'` ≥1, `grep -ciE 'numbered|dropped'` ≥1, `grep -ciE 'rule id|path|attribut'` ≥1. + + **Anti-keyword-stuffing:** the three topic patterns must match on **three distinct bullet lines** and the section must contain ≥3 bullets. One bullet naming all three defects passes the loose form while under-describing the change — the failure that drew a patch bump on v0.35.1. +- [ ] **AC18 [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. +- [ ] **AC19 [operator]** A clean review scores zero on the live five-PR `dev-1` fixture — evidence: after `rm -rf bench/.cache/reviews bench/results`, `make bench BENCH_ARGS="--model --effort --mode short"` completes; for every results row whose `raw_output_ref` file satisfies `grep -cE '^#{1,6} +(Must Fix|Should Fix|Nice to Have)' == 3` **and** `grep -cE '^None\.$' == 3`, `jq '.findings | length'` prints `0`. At least one row must satisfy the two greps, otherwise the criterion is untested and the run is repeated. +- [ ] **AC20 [operator]** Every finding recorded in a live run carries an attribution — evidence: `jq -r 'select(.findings[]? | (.path == null and .rule_id == null)) | .pr_id' bench/results/results.jsonl` prints nothing (0 lines). Any PR whose review contained an unattributable item appears instead as a loud failure in the run summary, never as a row. +- [ ] **AC21 [operator]** Numbered findings and inline rule tags survive to the ledger — evidence: for every results row, the count from `jq '.findings | length'` is ≥ the count from `grep -c '\*(rule: ' `; and across the run, `jq -r '.findings[].rule_id' bench/results/results.jsonl | grep -vc '^null$'` is ≥1 whenever any raw output contains `*(rule: `. Under the shipped runner this second count is 0 in every run to date. + +**Scenario coverage — NO new scenario.** The harvester is a pure function over text and every capture is a file on disk, so all four root causes are reachable by unit tests at the exact boundary that produced them. The loud-failure control flow is reachable with the existing stub-executable harness in `bench/testsupport.py`. The remaining evidence needs real tokens against a live review, which the scenario harness cannot supply either — AC19–AC21 are operator-executed after merge, exactly as specs 002, 003 and 004 did. + +## Verification + +### Container-executable (runs inside the YOLO container at prompt time) + +``` +make precommit +python3 -m unittest discover -s bench -p 'test_*.py' -v +git diff origin/master -- bench/test_config.py bench/test_resolve.py bench/test_review.py | grep -c '^-.*def test_' +git diff origin/master -- bench/testdata/sample-report.md bench/testdata/real-capture-report.md +grep -rn '/Users/\|~/Documents/' bench/ +for f in bench/testdata/capture-notes-block-h2.md \ + bench/testdata/capture-numbered-findings-h3.md \ + bench/testdata/capture-traceability-h4.md \ + bench/testdata/capture-summary-trailer-h4.md; do + shasum -a 256 "$f" | cut -d' ' -f1; grep -c '' "$f" +done +grep -cE '^\s*(self\.assert|assert )' bench/test_config.py +grep -cE '^\s*(self\.assert|assert )' bench/test_resolve.py +grep -cE '^\s*(self\.assert|assert )' bench/test_review.py +grep -niE 'bold label|numbered|unattributable|inline rule' bench/README.md +sed -n '/^## Unreleased/,/^## v/p' CHANGELOG.md +``` + +Expected: `make precommit` exits 0; the verbose unittest run reports `OK` with `Ran N tests`, `N > 72`, and shows the trailing-notes, bold-label, numbered-item, rule-tag, leading-bold-reference, body-fidelity, outside-section, unattributable and `heading_level` tests by name; the deleted-test grep prints `0`; the four digests equal AC2's published `sha256` values and the four line counts print `17`, `28`, `57` and `21`; the three per-file assertion counts are at least `63`, `46` and `165`; the two pre-existing fixture diffs are empty; the personal-path grep returns nothing (exit 1); the README greps return ≥4 distinct lines; the extracted Unreleased section carries ≥3 `fix:`/`feat:`/`docs:` bullets naming the phantom findings, the dropped numbered findings, and attribution. + +### Operator-executable (runs on the host, spends real tokens) + +``` +rm -rf bench/.cache/reviews bench/results +make bench BENCH_ARGS="--model --effort --mode short" +jq -s 'length' bench/results/results.jsonl +jq -r 'select(.findings[]? | (.path == null and .rule_id == null)) | .pr_id' bench/results/results.jsonl +jq -r '.findings[].rule_id' bench/results/results.jsonl | grep -vc '^null$' +jq -r '.raw_output_ref' bench/results/results.jsonl | while read f; do + printf '%s none=%s tags=%s\n' "$f" "$(grep -cE '^None\.$' "$f")" "$(grep -c '\*(rule: ' "$f")" +done +``` + +Expected: the run completes; the both-null selector prints nothing; every raw output whose three sections read `None.` corresponds to a row with zero findings; every row's findings count is at least its raw output's inline-rule-tag count. Any PR whose review carried an unattributable item is reported in the summary as failed with `UNATTRIBUTABLE FINDING` and has no row — that outcome satisfies the criteria and is recorded, not worked around. + +## Desired Behavior + +1. **A severity section ends at the first block that is not part of it.** Its content ends at the next markdown heading of any level, at a thematic break, at end of input, or at a line outside a fenced block whose first non-whitespace content begins a bold run — whichever comes first. The bold-run terminator is what closes the observed defect: `**Notes:**` and `**Summary**:` introduce a new block, and everything under them belongs to that block, not to the severity section above. Termination never depends on the terminating heading's level relative to the section's own, because level varies run to run within a single mode. + +2. **A finding opens on any list item the reviewer writes, in either list style.** Unordered items (`-`, `*`) and ordered items (a run of digits followed by `.` and a space) both open a finding; ordered numbering need not start at 1 and is not restricted to one digit. Non-list lines following an open item extend it. A line inside a fenced code block is ordinary text and opens nothing. Prose appearing in a section before any item — most importantly the mandated `None.` sentinel — contributes no finding, unchanged from v0.35.2. + +3. **The item's leading bold reference supplies `path` and `line`, and the body keeps it intact.** Attribution is read from the bold run at the head of the item, in the shapes the reviewer actually writes: a backticked `path:line`, a backticked path followed by a line-range mention (the range's first number is the line), and a bold run containing a backticked path-shaped token among other words. A path or line appearing only in the item's trailing prose is not used when the leading bold run supplies one, and no path is ever inferred by searching the repository. The bold run itself is preserved verbatim in the body — the list marker is stripped, the emphasis is not. + +4. **`rule_id` is read positionally, from the reviewer's inline tag first and a head-anchored legacy shape second.** Two sources are consulted, in strict priority order, and never any other position: + + 1. **The item's own `*(rule: ``)*` marker.** When present it always wins, and the id is recorded as the literal string the reviewer wrote, **whether or not it appears in `rules/index.json`** — this is the coupling RC3 names as the defect, and this path is free of it. + 2. **Only when no marker is present**, a backticked token at the very head of the item that is a member of `rules/index.json`. This is the shape the review template emits when it tags a finding by leading the body with the rule id, and three findings in the byte-frozen `bench/testdata/sample-report.md` depend on it. This second source **remains index-gated**: a head token absent from the index yields `None` rather than being recorded blind, because at the head of an item an unknown backticked token is far more likely to be a file path or a symbol than a rule name. + + An id named **outside a marker** — in the item's prose, in a traceability table, or anywhere outside the item — is never attributed to it, under either source. The marker itself is read wherever it appears within the item, **including at its tail**, which is where the reviewer actually writes it (every `*(rule: …)*` tag in `capture-numbered-findings-h3.md` sits at the end of its item, and AC5 requires exactly those ids). The instrument records what the reviewer claimed; reconciling that against the shipped rule set is the scorer's job, not the harvester's. + + **Why the legacy source survives on purpose.** Deleting it would null the three rule ids in `sample-report.md`, which the Constraints freeze byte-for-byte *and* freeze the asserted harvest result of. Retiring it is a change to a frozen fixture's asserted result and belongs in its own spec, not here. Its index gate is a deliberate asymmetry with source 1, not an oversight, and `bench/README.md` must document both sources and the asymmetry (AC16) — a doc claiming rule ids are "never validated against `rules/index.json`" would be false about the shipped parser, which is the exact docs-disagree-with-code mechanism behind D1, D2, D4 and D7. + +5. **An unattributable item fails the PR loudly.** An item inside a severity section that yields neither a `path` nor a `rule_id` cannot be keyed, therefore cannot be matched against a golden set, and is never written down as a body-only finding. Harvesting reports every such item; when the report is non-empty the PR fails in the same class as the existing `NOT A REVIEW` gate — no ledger row, no review cache entry, the PR listed as failed, remaining PRs still processed, the process exits non-zero — with a diagnosis headed by the frozen literal `UNATTRIBUTABLE FINDING` (the sibling of `NON_REVIEW_MARKER = "NOT A REVIEW"` at `bench/run.py:64`) and naming the PR, the severity section, and each offending item verbatim within the same bounded excerpt envelope the existing gate uses. Nothing else about that gate changes. + +6. **The fixtures are captures with findings in them, and the contract is written beside them.** `bench/testdata/` carries four verbatim captures of live review output spanning all three observed heading levels: one whose sections all read `None.` with a trailing notes block, one carrying five numbered findings with inline rule tags plus a positive-notes list, one carrying a single bold-headed finding plus a traceability table, and one zero-finding short-mode report whose trailer is a `**Summary**:` bold label. The two pre-existing fixtures stay byte-identical. `bench/README.md` states the corrected boundary rules, the two list styles, the positional rule-tag source, and the unattributable-item rejection, so the next fixture author reads the contract rather than re-deriving it from the parser. + +## 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 72 existing tests keep passing and their assertions are not weakened.** No test function may be deleted and no assertion relaxed to accommodate the new parser; the suite's test count after this work is strictly greater than 72. Existing stub payloads may gain an attribution on their finding items — that is a payload change, not an assertion change — but a test that asserted a specific harvest result keeps asserting a result at least as specific. The evidence for this Constraint is AC15's per-file assertion floors (`test_config.py` ≥ 63, `test_resolve.py` ≥ 46, `test_review.py` ≥ 165), because a surviving `def test_` signature with an emptied body satisfies a deleted-test check and a test-count check simultaneously, and a repo-wide total is payable with a new file. +- **The two pre-existing fixtures are frozen.** `bench/testdata/sample-report.md` and `bench/testdata/real-capture-report.md` are byte-identical after this work; both still harvest to their previously asserted results. +- **The raw-stdout-before-parsing invariant is preserved, and the two rejection classes differ in what they leave behind.** The raw capture is written verbatim before any parsing (`bench/run.py` step 5, ahead of harvest at step 6) and that ordering does not change. Consequently the `NOT A REVIEW` gate, which fires before step 5, still leaves nothing under `reviews_root`; the new unattributable-item gate fires after it and leaves the `.stdout.txt` in place while writing **no** `.json` row marker and **no** ledger row. Since the cache check keys on the row marker alone (`bench/run.py:1349-1350`), the PR is still retried on the next run, and the raw text stays on disk so the review is re-harvestable once the parser is fixed. A rejected PR also writes a both-stream failure artifact under `failures_root`, the same diagnostic shipped in v0.35.3. +- **The `NOT A REVIEW` gate is unchanged** — same required sections, same heading matching, same bounded excerpt, same position ahead of the raw-output cache write. The unattributable-item rejection is a second, later gate that reuses the same no-row / no-cache-entry / retry-next-run semantics. +- `make precommit` (which runs `bench-test`) stays green. Bench tests must not require network access, a real `claude` binary, or GitHub access. +- **Frozen invariants** (not configurable, not flagged): the three required section names; the section terminators; the list-item markers; the inline rule-tag marker shape; the `UNATTRIBUTABLE FINDING` marker literal; the rejection excerpt bound; the 45-minute review timeout; the cache, results and failure-artifact locations; the isolated config directory; the `--golden` exit-2 rejection. +- **Harvested values are data, never paths.** A `path` or `rule_id` read out of review output is written to the ledger and to nothing else. No harvested value is opened, stat-ed, joined onto a filesystem root, or passed to a subprocess. +- No rule, agent, command, or doc that participates in a review is edited, including `commands/pr-review.md`. `bench/prs.json` stays frozen. +- **Repo conventions that must not regress** (`docs/dod.md`): no personal paths (`/Users/`, `~/Documents/`) in any shipped file including the new capture fixtures, and a `## Unreleased` CHANGELOG entry. +- **CHANGELOG entries use conventional prefixes** (`fix:` / `feat:` / `docs:`) per `docs/changelog-guide.md`, and the entry describes the whole change rather than the last prompt's slice. + +## Assumptions + +- **The four fixtures are installed by the operator before approval, not by any prompt.** The captures live only under `bench/.cache/reviews/`, which `.gitignore:9` excludes, and `.dark-factory.yaml` sets `worktree: false`, so a container clone can never reach them. The operator copies the four files into `bench/testdata/` and commits them before `dark-factory spec approve`; every prompt therefore finds them present and only writes tests. They were checked for personal paths (`grep -rl '/Users/' bench/.cache/reviews/` → no matches) and contain none. +- **No prompt creates, regenerates or reconstructs a fixture.** A transcribed fixture is the defect this spec exists to close, and a regenerated one is a different document that would not match AC2's published digest. A prompt that finds a fixture missing or digest-mismatched stops and reports it; it does not write the file. Handing an agent a fully-published acceptance shape and an unreachable source is what makes synthesis the default path, so the source is made reachable instead. +- `*(rule: ``)*` is the reviewer's inline tagging convention. It was observed in full mode only; short mode produced no attributed findings in any capture. The harvester reads it wherever it appears within an item and does not require it. +- The reviewer places its file reference in a bold run at the head of a finding item. Every attributed item in every capture follows this; the four shapes in AC8 are the observed variants. +- Reviews will sometimes contain items with no file reference and no rule tag — two of the eight true findings in the captures are of this kind. Under Desired Behavior 5 those reviews fail loudly. That is the intended, visible cost of refusing to record unmatchable findings; the resolution path, if the rate proves high, is a separate spec against `commands/pr-review.md` requiring an attribution on every finding, not a relaxation here. +- A stub executable on `PATH` printing a chosen payload to stdout and exiting with a chosen code is sufficient to reach the runner's control flow for the loud-failure criteria; `bench/testsupport.py` already provides it. +- The five fixture PRs stay merged, their recorded SHAs stay reachable, and `dev-1` stays the manifest version. + +## Failure Modes + +| Trigger | Expected behavior | Recovery | Detection | Reversibility | Concurrency | +|---|---|---|---|---|---| +| Severity section reads `None.` and is followed by a bold-label block with bullets (observed) | The bold-label line ends the section; the bullets are not findings; the PR records 0 findings | None needed | `bench/testdata/capture-notes-block-h2.md` harvests to the empty list | n/a | n/a | +| Severity section carries numbered items (observed) | One finding per item, in document order, with the attribution each carries | None needed | `bench/testdata/capture-numbered-findings-h3.md` harvests its five Should Fix items | n/a | n/a | +| An item inside a severity section carries neither a path reference nor a rule tag (observed twice in one capture) | The PR fails loudly: no ledger row, no `.json` row marker, item quoted verbatim on stderr; the `.stdout.txt` raw capture stays on disk and a both-stream failure artifact is written; other PRs still processed; process exits non-zero | Operator reads the quoted item and decides — either the review command must mandate attribution (separate spec) or the item was genuinely not a finding; the preserved raw capture is re-harvestable without spending tokens again | Non-zero exit; the literal `UNATTRIBUTABLE FINDING` plus PR id, section name and item text on stderr; the failure artifact under `failures_root`; summary lists the PR as failed | Fully reversible — no row and no row marker written; the PR is retried because the cache check keys on the row marker alone | Rows and cache entries for other PRs untouched | +| **Every** PR in a run fails with `UNATTRIBUTABLE FINDING` | Whole run produces zero rows and fails loudly with a uniform signature — the review command routinely emits unattributed findings and the two contracts have diverged | Operator files a follow-up spec against `commands/pr-review.md`; this spec's Non-goals forbid changing it here | All five PRs failed with `UNATTRIBUTABLE FINDING` — unmistakably contract drift rather than a per-PR fault | Fully reversible — no rows written | Whole run fails uniformly; no partial ledger to reconcile | +| Rule tag names an id absent from `rules/index.json` (renamed rule, drifted index, review run against a different rules revision) | The literal string from the tag is recorded; no null, no abort | None needed | The row carries a `rule_id` the index does not contain — visible to the scorer, which is where reconciliation belongs | n/a | n/a | +| Review prose or a traceability table names rule ids other than the item's own tag (observed: a 22-row table) | Neither is attributed to any finding | None needed | `bench/testdata/capture-traceability-h4.md` yields one finding with `rule_id` null | n/a | n/a | +| A finding's continuation paragraph begins with a bold run | The finding body is truncated at that line and the section closes early | Operator adds the capture as a fixture and files a follow-up | Observable as a short body, never as an invented finding — the failure direction is loss, not fabrication. No captured output exhibits this shape | Fully reversible — the raw output is cached and re-harvestable after a fix | n/a | +| Heading level changes between runs or within a run (observed: `##`, `###`, `####` across five captures in two runs) | No effect — termination and section matching are level-agnostic | None needed | Fixtures at all three levels stay green | n/a | n/a | +| Review output is megabytes | Harvesting is linear in input size; the unattributable diagnosis reuses the existing bounded excerpt so stderr stays bounded | Operator deletes `bench/.cache/` | Excerpt visibly truncated; artifact file size | Fully reversible | Disk growth confined to the cache the README names as deletable | +| Review output contains a pathological construct (deeply nested emphasis, an unclosed fence, thousands of list markers) | Matching stays line-oriented and bounded; no unbounded backtracking; worst case is a wrong section boundary, never a hang | Operator adds the capture as a fixture | Run completes; findings visibly wrong for that PR | Fully reversible | n/a | +| Crash between harvesting and the ledger append | Nothing written — the row append is atomic and happens after both gates | Re-run; the PR is uncached and retried | Ledger has fewer rows than the manifest | Fully reversible | Atomic write-then-rename; no truncated row is ever observed | +| Cached rows and raw outputs written by the shipped parser are present | They are not re-harvested and not migrated; cache hits are served as-is | Operator deletes `bench/.cache/reviews/` and `bench/results/` and re-runs, as the README states | Rows predating the fix carry body-only findings | Fully reversible by deletion | Append-only ledger unaffected | +| Two runners started against the same output directory | Unchanged: the second exits immediately without touching the ledger or cache | Operator waits and re-runs | Non-zero exit; stderr states a run is in progress | Fully reversible | Single-instance lock, unchanged from spec 002 | + +## Security / Abuse Cases + +- **Attacker-controlled surface:** the review subprocess's stdout. It is third-party-influenced text — the reviewed repository's content flows into the model's output — and this change parses more of it, and extracts more values out of it, than before. +- **Extracted values are data, not paths.** `path` and `rule_id` are recorded into the ledger and used for nothing else: never opened, never stat-ed, never joined onto a filesystem root, never passed to a subprocess. A review that emits `**`../../etc/passwd:1`**` produces a ledger row with that string in it and no filesystem access. +- **No evaluation, no execution:** the harvester only matches text and slices it. No value from review output reaches a shell, and subprocesses continue to be invoked with argument lists rather than shell strings. +- **Denial by volume:** review output can be arbitrarily large. Harvesting is linear in input size, and the unattributable-item diagnosis reuses the existing bounded excerpt, so a runaway subprocess cannot flood the operator's terminal. +- **Denial by pathological input:** section, heading, emphasis and list matching stay line-oriented with bounded patterns; no construct in a report can cause unbounded backtracking or a scan that is not linear. +- **Secret leakage:** the new diagnosis reproduces the subprocess's own output and nothing else — no environment variables, no tokens, no credential material. The new fixtures are captures that were checked for personal paths before being checked in. +- **Fail-closed, not fail-open:** every ambiguity about whether an item is a keyed finding resolves to rejection. A false rejection costs one operator decision and a re-run; a false acceptance writes an unmatchable measurement into an append-only ledger, which is the failure this spec exists to prevent. + +## Suggested Decomposition + +| # | Prompt focus | Covers DBs | Covers ACs | Depends on | +|---|---|---|---|---| +| 1 | Verify the four operator-installed fixtures against AC2's published digests (stop and report on any mismatch — never write or regenerate one); add the bold-run section terminator; lock outside-section content and heading-level independence. Pure parser + tests, no runner wiring. | 1, 6 (fixtures half) | AC2, AC3, AC4, AC10, AC14 | operator pre-step: fixtures committed before approval | +| 2 | Ordered-list item recognition alongside unordered, in-fence suppression, and body fidelity (the leading bold run survives marker stripping). | 2 | AC6, AC9 | prompt 1 (fixtures) | +| 3 | Attribution extraction: `path` / `line` from the leading bold reference in its four observed shapes; `rule_id` from the item's own inline tag, positionally and independent of `rules/index.json`. | 3, 4 | AC5, AC7, AC8 | prompt 2 | +| 4 | The unattributable-item rejection and its runner wiring: harvesting reports unattributable items, the PR fails in the `NOT A REVIEW` class, no row and no cache entry, other PRs still processed. | 5 | AC11, AC12, AC13 | prompt 3 | +| 5 | `bench/README.md` harvest-contract rewrite (terminators, both list styles, positional rule tag, unattributable rejection, fixtures table); CHANGELOG `## Unreleased` entry; personal-path and stdlib-only sweep; deleted-test and frozen-fixture checks; full precommit. | 6 (contract half) | AC1, AC15, AC16, AC17, AC18 | prompts 1-4 | + +Rationale: the chain 1 → 2 → 3 → 4 is genuinely sequential because each step's tests are written against behaviour the previous step made reachable — attribution (prompt 3) cannot be asserted against the five numbered items until ordered items are recognised (prompt 2), which cannot be asserted at all until the section boundaries stop swallowing them (prompt 1). All four fixtures are present from the operator pre-step before prompt 1 starts, so no prompt is ever blocked on an artifact it cannot obtain. Prompt 4 is the only one that touches the runner's control flow, isolated deliberately so the parser changes land and prove themselves as pure functions first, and so the one prompt that can fail a whole run is reviewable on its own. Prompt 5 is docs and packaging, last so the CHANGELOG bullets describe what all four actually shipped — the specific failure from spec 002, where the final prompt described only its own slice and the release classifier cut a patch instead of a minor. AC19–AC21 are operator-executed after merge in the spec-verification phase. + +## Do-Nothing Option + +Doing nothing leaves the instrument reporting a confident wrong number in both directions on every run, with no external signal that anything is wrong. The measured state is already known: on five real captures the harvester achieves precision 3/6 and recall 3/8, with zero attribution on every finding it emits. The consequences are not hypothetical — three runs were already published as evidence of model non-determinism when the spread was partly an artifact of how many housekeeping bullets the reviewer appended, and the golden-set task downstream is blocked because precision and recall need a per-finding key the harvester does not produce. Every further run spends real tokens generating rows that will have to be discarded. The alternatives considered: (a) hand-inspect every raw capture before trusting a run — restores correctness for one careful operator on five PRs and discards the entire point of a mechanical instrument; (b) fix only the phantom findings (RC1), the visible half — the recall hole is the more dangerous half precisely because a missing finding leaves no trace at all, and it is the one that would silently absorb a rules regression that stopped the reviewer finding things; (c) fix extraction but keep the body-only fallback instead of failing loudly — the ledger keeps filling with rows that look like data and cannot be scored, which is the exact defect family this is the fifth instance of; (d) rewrite the review command to emit machine-readable findings instead of parsing markdown — the right long-term answer and explicitly out of scope, because changing the measured configuration and the instrument in the same change destroys the baseline that specs 002–004 established. All four root causes live in one layer, are reachable by unit tests against captures that already exist on disk, and none of the downstream measurement work can start until they are closed. From 99d4f794ce8029da75260b5d42d0c2aa944b7b03 Mon Sep 17 00:00:00 2001 From: Benjamin Borbe Date: Sat, 8 Aug 2026 14:33:54 +0200 Subject: [PATCH 3/6] 024-spec-005-attribution-from-reviewer-markers --- bench/run.py | 92 ++++++- bench/test_review.py | 255 ++++++++++++++++++ ...c-005-attribution-from-reviewer-markers.md | 7 +- 3 files changed, 341 insertions(+), 13 deletions(-) rename prompts/{in-progress => completed}/024-spec-005-attribution-from-reviewer-markers.md (98%) diff --git a/bench/run.py b/bench/run.py index 22fdbee..6a2a616 100755 --- a/bench/run.py +++ b/bench/run.py @@ -51,6 +51,13 @@ BULLET_RE = re.compile(r"^\s{0,3}([-*])\s+(.+)$") ORDERED_ITEM_RE = re.compile(r"^\s{0,3}\d+\.\s+(.+)$") BOLD_RUN_START_RE = re.compile(r"^\s*\*\*") +# Attribution extraction patterns +RULE_TAG_RE = re.compile(r"\*\(rule:\s*`([^`]+)`\)") +HEAD_RULE_TAG_RE = re.compile(r"^`([^`]+)`") +LEADING_BOLD_RE = re.compile(r"^\*\*(.+?)\*\*") +PATH_LINE_RE = re.compile(r"([A-Za-z0-9_./-]+\.[A-Za-z0-9_./-]+):(\d+)") +BACKTICK_TOKEN_RE = re.compile(r"`([^`\s]+)`") +LINE_MENTION_RE = re.compile(r"(?i)\blines?\s*~?\s*(\d+)") _SECTION_BY_LOWER = {name.lower(): name for name in REQUIRED_SECTION_NAMES} REQUIRED_ENTRY_FIELDS = ( "id", "owner", "repo", "number", @@ -1012,14 +1019,6 @@ def load_rule_ids(coding_repo: pathlib.Path) -> set: return {entry["id"] for entry in data if "id" in entry} -def _extract_rule_id(text: str, known_rule_ids: set) -> str | None: - """Extract the first rule ID token from text, or None.""" - for token in re.split(r"[\s`\(\)\[\],:]+", text): - if token in known_rule_ids: - return token - return None - - def _extract_path_line(text: str, known_rule_ids: set) -> tuple[str | None, int | None]: """Extract the first path:line reference from text, skipping known rule IDs. @@ -1027,13 +1026,84 @@ def _extract_path_line(text: str, known_rule_ids: set) -> tuple[str | None, int A token that is a known rule ID is never treated as a path. """ # Find all potential path:line matches - for m in re.finditer(r"([A-Za-z0-9_./-]+\.[A-Za-z0-9_./-]+):(\d+)", text): + for m in PATH_LINE_RE.finditer(text): candidate = m.group(1) if candidate not in known_rule_ids: return candidate, int(m.group(2)) return None, None +def extract_attribution(body: str, known_rule_ids: set) -> tuple[str | None, int | None, str | None]: + """Return (path, line, rule_id) for one finding item, read from the item's own markers. + + rule_id, in priority order: + 1. the item's own inline `*(rule: ``)*` marker — recorded as the literal + string the reviewer wrote, whether or not it appears in rules/index.json; + 2. otherwise a backticked token at the very head of the item that is a member + of known_rule_ids (the shape the review template emits when it tags a + finding by leading its body with the rule id); + 3. otherwise None. + An id named anywhere else in the item, or anywhere outside it, is never used. + + path/line, in priority order: + 1. the bold run at the head of the item, when it names a path; + 2. otherwise the first path:line reference anywhere in the item that is not + a known rule id; + 3. otherwise (None, None). + When the leading bold run supplies a path, line is taken from that bold run and + from nowhere else — a line number appearing only in the item's trailing prose is + not used. No path is ever inferred by searching the repository and no line is + ever guessed from surrounding text. + """ + # rule_id — source 1: inline *(rule: `id`)* tag, verbatim, no index check + m = RULE_TAG_RE.search(body) + if m: + rule_id = m.group(1) + # path/line from bold run at head of item + bold_m = LEADING_BOLD_RE.match(body) + if bold_m: + ref = bold_m.group(1) + path_m = PATH_LINE_RE.search(ref) + if path_m: + return path_m.group(1), int(path_m.group(2)), rule_id + # try backtick token with dot as path + tok_m = BACKTICK_TOKEN_RE.search(ref) + if tok_m: + token = tok_m.group(1) + if "." in token: + line_m = LINE_MENTION_RE.search(ref) + line = int(line_m.group(1)) if line_m else None + return token, line, rule_id + # fall through to step 2 for path/line + else: + rule_id = None + # rule_id — source 2: head-anchored backtick token, index-gated + head_m = HEAD_RULE_TAG_RE.match(body) + if head_m: + token = head_m.group(1) + if token in known_rule_ids: + rule_id = token + + # path/line — source 1: bold run at head of item + bold_m = LEADING_BOLD_RE.match(body) + if bold_m: + ref = bold_m.group(1) + path_m = PATH_LINE_RE.search(ref) + if path_m: + return path_m.group(1), int(path_m.group(2)), rule_id + tok_m = BACKTICK_TOKEN_RE.search(ref) + if tok_m: + token = tok_m.group(1) + if "." in token: + line_m = LINE_MENTION_RE.search(ref) + line = int(line_m.group(1)) if line_m else None + return token, line, rule_id + + # path/line — source 2: whole-item scan (existing _extract_path_line logic) + path, line = _extract_path_line(body, known_rule_ids) + return path, line, rule_id + + def heading_section_name(line: str) -> str | None: """Return the canonical findings-section name a markdown heading line names, or None. @@ -1166,14 +1236,12 @@ def flush_finding(): nonlocal current_finding_lines, current_section, findings if not current_finding_lines or current_section is None: return - text = " ".join(current_finding_lines) body = _normalize_body(current_finding_lines) # Skip the "None." empty-section sentinel (exact equality only) 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) + path, line_num, rule_id = extract_attribution(body, known_rule_ids) findings.append({ "path": path, "line": line_num, diff --git a/bench/test_review.py b/bench/test_review.py index 9901056..99d3845 100644 --- a/bench/test_review.py +++ b/bench/test_review.py @@ -1948,6 +1948,261 @@ def test_traceability_capture_bold_run_survives_normalization(self): result.unattributable, [], f"expected empty unattributable, got: {result.unattributable}" ) + # AC9 completion: path is read from the leading bold reference + self.assertEqual( + result.findings[0]["path"], "src/config.ts", + f"expected path src/config.ts from leading bold reference, got: {result.findings[0]['path']}" + ) + + +class TestNumberedCaptureFindingsCarryAttribution(unittest.TestCase): + """AC5: the five previously-dropped numbered findings carry the capture's attribution.""" + + def test_numbered_capture_findings_carry_attribution(self): + text = (run.BENCH_DIR / "testdata" / "capture-numbered-findings-h3.md").read_text() + ids = run.load_rule_ids(run.REPO_ROOT) + result = run.harvest(text, ids) + + expected = [ + ("CHANGELOG.md", 18, "changelog/conventional-prefix-required"), + ("README.md", 76, "readme/user-facing-not-agent-context"), + (".github/workflows/ci.yml", 32, None), + ("Makefile.precommit", None, None), + ("Makefile.precommit", None, None), + ] + observed = [(f["path"], f["line"], f["rule_id"]) for f in result.findings[:5]] + self.assertEqual( + observed, expected, + f"first five findings: expected {expected}, got: {observed}" + ) + self.assertEqual( + len(result.findings), 7, + f"expected 7 total findings, got: {len(result.findings)}" + ) + + +class TestRuleIdComesFromTheItemsOwnMarkers(unittest.TestCase): + """AC7: rule_id comes from the item's own markers in priority order.""" + + def test_case_a_tag_with_unknown_id_yields_literal(self): + # Case A: an item tagged with an id absent from rules/index.json yields that literal + known_ids = run.load_rule_ids(run.REPO_ROOT) + self.assertNotIn( + "made-up/not-in-the-index", known_ids, + "test precondition: made-up/not-in-the-index must not be in the index" + ) + + report = ( + "## Must Fix (Critical)\n" + "- **`src/x.py:1`** something is wrong here. *(rule: `made-up/not-in-the-index`)*\n" + ) + result = run.harvest(report, known_ids) + self.assertEqual(len(result.findings), 1) + self.assertEqual( + result.findings[0]["rule_id"], "made-up/not-in-the-index", + f"Case A: expected literal rule_id, got: {result.findings[0]['rule_id']}" + ) + + def test_case_b_prose_names_different_rule_before_marker_yields_marker(self): + # Case B: prose names a different real rule before the item's own marker + known_ids = run.load_rule_ids(run.REPO_ROOT) + real_ids = sorted(known_ids) + id_a, id_b = real_ids[0], real_ids[1] if len(real_ids) > 1 else real_ids[0] + if id_a == id_b: + id_b = next((r for r in known_ids if r != id_a), id_a) + self.assertNotEqual(id_a, id_b, "test needs two distinct rule IDs") + + report = ( + f"## Must Fix (Critical)\n" + f"- **{id_a}** is wrong here. *(rule: `{id_b}`)*\n" + ) + result = run.harvest(report, known_ids) + self.assertEqual(len(result.findings), 1) + self.assertEqual( + result.findings[0]["rule_id"], id_b, + f"Case B: expected marker id {id_b}, got: {result.findings[0]['rule_id']}" + ) + + def test_case_c_no_marker_head_token_in_index_yields_that_id(self): + # Case C: no marker, head-anchored backtick token IS in index + known_ids = run.load_rule_ids(run.REPO_ROOT) + real_id = next((rid for rid in known_ids if "/" in rid), None) + self.assertIsNotNone(real_id, "test needs a real rule ID with a slash") + + report = ( + f"## Must Fix (Critical)\n" + f"- `{real_id}`: a finding in src/x.py:1\n" + ) + result = run.harvest(report, known_ids) + self.assertEqual(len(result.findings), 1) + self.assertEqual( + result.findings[0]["rule_id"], real_id, + f"Case C: expected head token {real_id}, got: {result.findings[0]['rule_id']}" + ) + + def test_case_d_no_marker_head_token_not_in_index_yields_none(self): + # Case D: no marker, head-anchored backtick token NOT in index + known_ids = run.load_rule_ids(run.REPO_ROOT) + self.assertNotIn( + "not-a-real-rule-id", known_ids, + "test precondition: not-a-real-rule-id must not be in the index" + ) + + report = ( + "## Must Fix (Critical)\n" + "- `not-a-real-rule-id`: a finding in src/x.py:7\n" + ) + result = run.harvest(report, known_ids) + self.assertEqual(len(result.findings), 1) + self.assertIsNone( + result.findings[0]["rule_id"], + f"Case D: expected rule_id None, got: {result.findings[0]['rule_id']}" + ) + self.assertEqual( + result.findings[0]["path"], "src/x.py", + f"Case D: expected path src/x.py, got: {result.findings[0]['path']}" + ) + self.assertEqual( + result.findings[0]["line"], 7, + f"Case D: expected line 7, got: {result.findings[0]['line']}" + ) + + +class TestPathAndLineComeFromTheLeadingBoldReference(unittest.TestCase): + """AC8: path and line come from the leading bold reference.""" + + def test_changelog_dot_md_colon_18(self): + report = "## Must Fix (Critical)\n- **`CHANGELOG.md:18`** something.\n" + ids = run.load_rule_ids(run.REPO_ROOT) + result = run.harvest(report, ids) + self.assertEqual(len(result.findings), 1) + self.assertEqual( + (result.findings[0]["path"], result.findings[0]["line"]), + ("CHANGELOG.md", 18), + f"expected (CHANGELOG.md, 18), got: ({result.findings[0]['path']}, {result.findings[0]['line']})" + ) + + def test_readme_md_with_lines_76_to_94(self): + report = "## Must Fix (Critical)\n- **`README.md` \"Security gates\" section (~lines 76-94)** something.\n" + ids = run.load_rule_ids(run.REPO_ROOT) + result = run.harvest(report, ids) + self.assertEqual(len(result.findings), 1) + self.assertEqual( + (result.findings[0]["path"], result.findings[0]["line"]), + ("README.md", 76), + f"expected (README.md, 76), got: ({result.findings[0]['path']}, {result.findings[0]['line']})" + ) + + def test_github_workflows_ci_yml_colon_32(self): + report = "## Must Fix (Critical)\n- **`.github/workflows/ci.yml:32`** something.\n" + ids = run.load_rule_ids(run.REPO_ROOT) + result = run.harvest(report, ids) + self.assertEqual(len(result.findings), 1) + self.assertEqual( + (result.findings[0]["path"], result.findings[0]["line"]), + (".github/workflows/ci.yml", 32), + f"expected (.github/workflows/ci.yml, 32), got: ({result.findings[0]['path']}, {result.findings[0]['line']})" + ) + + def test_ci_plus_makefile_precommit(self): + report = "## Must Fix (Critical)\n- **CI + `Makefile.precommit`** something.\n" + ids = run.load_rule_ids(run.REPO_ROOT) + result = run.harvest(report, ids) + self.assertEqual(len(result.findings), 1) + self.assertEqual( + (result.findings[0]["path"], result.findings[0]["line"]), + ("Makefile.precommit", None), + f"expected (Makefile.precommit, None), got: ({result.findings[0]['path']}, {result.findings[0]['line']})" + ) + + def test_leading_bold_takes_precedence_over_trailing_prose(self): + # Negative case: leading bold names a/b.py:10, trailing prose mentions c/d.py:99 + report = ( + "## Must Fix (Critical)\n" + "- **`a/b.py:10`** something is wrong.\n" + "More prose here, mentioning c/d.py:99 in passing.\n" + ) + ids = run.load_rule_ids(run.REPO_ROOT) + result = run.harvest(report, ids) + self.assertEqual(len(result.findings), 1) + self.assertEqual( + (result.findings[0]["path"], result.findings[0]["line"]), + ("a/b.py", 10), + f"expected (a/b.py, 10) from leading bold, got: ({result.findings[0]['path']}, {result.findings[0]['line']})" + ) + + def test_bold_run_with_path_only_ignores_trailing_line_prose(self): + # Negative case: leading bold names a/b.py only, prose mentions line 42 + report = ( + "## Must Fix (Critical)\n" + "- **`a/b.py`** something is wrong.\n" + "More prose here, referencing line 42.\n" + ) + ids = run.load_rule_ids(run.REPO_ROOT) + result = run.harvest(report, ids) + self.assertEqual(len(result.findings), 1) + self.assertEqual( + (result.findings[0]["path"], result.findings[0]["line"]), + ("a/b.py", None), + f"expected (a/b.py, None) — line in prose is not used when bold supplies path, got: ({result.findings[0]['path']}, {result.findings[0]['line']})" + ) + + def test_bold_run_without_line_does_not_use_trailing_full_path_line(self): + # Discriminating negative case: leading bold names a/b.py only, trailing prose + # carries a full c/d.py:99 reference — the trailing reference must NOT be used + report = ( + "## Must Fix (Critical)\n" + "- **`a/b.py`** something is wrong.\n" + "More prose here, mentioning c/d.py:99 in passing.\n" + ) + ids = run.load_rule_ids(run.REPO_ROOT) + result = run.harvest(report, ids) + self.assertEqual(len(result.findings), 1) + self.assertEqual( + (result.findings[0]["path"], result.findings[0]["line"]), + ("a/b.py", None), + f"expected (a/b.py, None) — trailing c/d.py:99 must not be used when bold already supplied path, got: ({result.findings[0]['path']}, {result.findings[0]['line']})" + ) + + +class TestExtractedValuesAreDataNotPaths(unittest.TestCase): + """Safety: extracted path values are never opened or accessed as filesystem paths.""" + + def test_etc_passwd_path_is_recorded_without_filesystem_access(self): + # A finding emitting ../../etc/passwd:1 must produce a ledger row with that + # string and no filesystem access. + report = "## Must Fix (Critical)\n- **`../../etc/passwd:1`** something.\n" + ids = run.load_rule_ids(run.REPO_ROOT) + result = run.harvest(report, ids) + self.assertEqual(len(result.findings), 1) + self.assertEqual( + result.findings[0]["path"], "../../etc/passwd", + f"expected ../../etc/passwd, got: {result.findings[0]['path']}" + ) + self.assertEqual(result.findings[0]["line"], 1) + + +class TestTraceabilityTableDoesNotContributeRuleIds(unittest.TestCase): + """AC10 Case B regression: rule ids from a traceability table outside sections are not attributed.""" + + def test_traceability_capture_rule_ids_not_from_table(self): + text = (run.BENCH_DIR / "testdata" / "capture-traceability-h4.md").read_text() + ids = run.load_rule_ids(run.REPO_ROOT) + result = run.harvest(text, ids) + + # The 22-row traceability table is outside any severity section. + # No finding should carry a rule_id from that table. + table_ids = re.findall(r"^\| ([a-z][a-z0-9/-]+) \|", text, re.MULTILINE) + self.assertEqual( + len(table_ids), 22, + f"traceability table must have 22 rows, got: {len(table_ids)}" + ) + for finding in result.findings: + if finding.get("rule_id") is not None: + self.assertNotIn( + finding["rule_id"], table_ids, + f"rule_id {finding['rule_id']!r} must not come from traceability table" + ) if __name__ == "__main__": diff --git a/prompts/in-progress/024-spec-005-attribution-from-reviewer-markers.md b/prompts/completed/024-spec-005-attribution-from-reviewer-markers.md similarity index 98% rename from prompts/in-progress/024-spec-005-attribution-from-reviewer-markers.md rename to prompts/completed/024-spec-005-attribution-from-reviewer-markers.md index a20d7b9..3f94628 100644 --- a/prompts/in-progress/024-spec-005-attribution-from-reviewer-markers.md +++ b/prompts/completed/024-spec-005-attribution-from-reviewer-markers.md @@ -1,8 +1,13 @@ --- -status: approved +status: completed spec: [005-bug-bench-harvest-finding-extraction] +summary: 'Implemented positional attribution extraction: rule_id from inline marker (verbatim, no index check) and head-anchored fallback (index-gated), path/line from leading bold reference in four observed shapes, replacing the whole-item membership scan' +execution_id: coding-exec-024-spec-005-attribution-from-reviewer-markers +dark-factory-version: v0.192.9 created: "2026-08-08T11:42:00Z" queued: "2026-08-08T12:14:19Z" +started: "2026-08-08T12:29:43Z" +completed: "2026-08-08T12:33:54Z" --- From ddfd9b437dfddfea852abe8a33a58ca00e21e630 Mon Sep 17 00:00:00 2001 From: Benjamin Borbe Date: Sat, 8 Aug 2026 14:41:44 +0200 Subject: [PATCH 4/6] 025-spec-005-unattributable-item-rejection --- bench/run.py | 78 +++++-- bench/test_review.py | 203 +++++++++++++++--- ...-spec-005-unattributable-item-rejection.md | 6 +- 3 files changed, 247 insertions(+), 40 deletions(-) rename prompts/{in-progress => completed}/025-spec-005-unattributable-item-rejection.md (99%) diff --git a/bench/run.py b/bench/run.py index 6a2a616..dc21f1a 100755 --- a/bench/run.py +++ b/bench/run.py @@ -71,6 +71,7 @@ # Gate constants (frozen invariants — not configurable) NON_REVIEW_MARKER = "NOT A REVIEW" +UNATTRIBUTABLE_MARKER = "UNATTRIBUTABLE FINDING" REJECTION_EXCERPT_BYTES = 2000 # Failure artifact constants (frozen literals — tests grep for them and the README quotes them) @@ -1178,6 +1179,30 @@ def non_review_report(pr_id: str, missing: list[str], stdout_text: str) -> str: ) +def unattributable_report(pr_id: str, items: list, stdout_text: str) -> str: + """Build the multi-line stderr diagnosis for a review carrying unkeyable items. + + Names the PR, then each offending item's severity section and its text + verbatim. The item block is passed through the same bounded excerpt the + NOT A REVIEW gate uses, so a runaway subprocess cannot flood the terminal. + """ + total = len(stdout_text.encode("utf-8")) + item_blocks = [] + for item in items: + item_text = f"[{item['section']}] {item['body']}" + item_blocks.append(rejection_excerpt(item_text)) + + return ( + f"{UNATTRIBUTABLE_MARKER}: {pr_id}\n" + f"{len(items)} unattributable item(s)\n" + + "\n\n".join(item_blocks) + "\n" + f"no ledger row and no row marker were written; this PR is retried on the next run\n" + f"--- subprocess stdout ({total} bytes total) ---\n" + f"{rejection_excerpt(stdout_text)}\n" + f"--- end excerpt ---" + ) + + def list_item_body(stripped_line: str) -> str | None: """Return the item text when stripped_line opens a list item, else None. @@ -1209,13 +1234,11 @@ def _normalize_body(lines: list[str]) -> str: class HarvestResult: """The two-part outcome of harvesting one review report. - findings — items inside a severity section that carry an attribution. - unattributable — items inside a severity section that carry none. Reserved - here and always empty; populated by the unattributable-item - gate, which classifies items once attribution extraction - exists. It is a separate component precisely so a caller - can distinguish "nothing was found" from "something was - found and could not be keyed" (spec 005 AC3). + findings — items inside a severity section that carry a path or rule_id. + unattributable — items inside a severity section that carry neither. Only + items inside a severity section are ever classified; content + outside one (positive notes, traceability table, etc.) opens + no item at all and appears in neither component. """ findings: list unattributable: list @@ -1229,11 +1252,12 @@ def harvest(report_text: str, known_rule_ids: set) -> HarvestResult: has keys: path, line, rule_id, body. """ findings: list = [] + unattributable: list = [] current_section: str | None = None current_finding_lines: list[str] = [] def flush_finding(): - nonlocal current_finding_lines, current_section, findings + nonlocal current_finding_lines, current_section, findings, unattributable if not current_finding_lines or current_section is None: return body = _normalize_body(current_finding_lines) @@ -1242,12 +1266,18 @@ def flush_finding(): current_finding_lines = [] return path, line_num, rule_id = extract_attribution(body, known_rule_ids) - findings.append({ - "path": path, - "line": line_num, - "rule_id": rule_id, - "body": body, - }) + if path is None and rule_id is None: + unattributable.append({ + "section": current_section, + "body": body, + }) + else: + findings.append({ + "path": path, + "line": line_num, + "rule_id": rule_id, + "body": body, + }) current_finding_lines = [] for line, in_fence in iter_report_lines(report_text): @@ -1284,7 +1314,7 @@ def flush_finding(): current_finding_lines.append(stripped) flush_finding() - return HarvestResult(findings=findings, unattributable=[]) + return HarvestResult(findings=findings, unattributable=unattributable) # ---------------------------------------------------------------------- @@ -1522,6 +1552,24 @@ def process_pr(*, entry: dict, coding_repo: pathlib.Path, harvested = harvest(proc.stdout, known_rule_ids) findings = harvested.findings + # 6a. Unattributable-item gate — a finding that cannot be keyed is a parse + # failure, not a body-only finding. Fires *after* the step-5 raw write, + # so the raw capture stays on disk and is re-harvestable after a fix, + # and *before* the ledger append and the row-marker write, so the PR is + # retried next run (the cache check keys on the row marker alone). + if harvested.unattributable: + n = len(harvested.unattributable) + write_failure_artifact( + cache_root, cfg_hash, pr_id, + reason=f"{UNATTRIBUTABLE_MARKER}: {n} unattributable item(s)", + stdout=proc.stdout, stderr=proc.stderr, + ) + print(unattributable_report(pr_id, harvested.unattributable, proc.stdout), + file=sys.stderr) + raise BenchError( + f"{UNATTRIBUTABLE_MARKER}: {pr_id}: {n} unattributable item(s)" + ) + # 7. Build row and append to ledger review_command = shlex.join(argv) # raw_output_ref: relative to REPO_ROOT if under it, else absolute diff --git a/bench/test_review.py b/bench/test_review.py index 99d3845..0160270 100644 --- a/bench/test_review.py +++ b/bench/test_review.py @@ -23,10 +23,10 @@ # ---------------------------------------------------------------------- # Shared test harness helpers # ---------------------------------------------------------------------- -def run_one_pr_with_payload(td: pathlib.Path, payload: str) -> tuple[int, str, pathlib.Path, pathlib.Path]: +def run_one_pr_with_payload(td: pathlib.Path, payload: str) -> tuple[int, str, 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). + Returns (returncode, captured_stdout, captured_stderr, results_dir, cache_root). The stub claude is installed on PATH before the call. """ td = pathlib.Path(td) @@ -65,20 +65,22 @@ def run_one_pr_with_payload(td: pathlib.Path, payload: str) -> tuple[int, str, p plugin_src = testsupport.build_coding_repo(td / "repo") cfg = testsupport.build_verify_config_dir(td / "cfg", plugin_src) + captured_stdout = io.StringIO() 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 + with contextlib.redirect_stdout(captured_stdout): + 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_stdout.getvalue(), captured_stderr.getvalue(), results_dir, cache_root def run_one_pr_with_streams(td: pathlib.Path, *, stdout_text: str = "", @@ -418,7 +420,7 @@ class TestHarvestKeepsFindingWithoutAnyRuleId(unittest.TestCase): def test_harvest_keeps_finding_without_any_rule_id(self): report = """#### Must Fix (Critical) -- This finding has no rule ID at all but should still be kept. +- **`src/x.py:4`** This finding has no rule ID at all but should still be kept. """ ids = run.load_rule_ids(run.REPO_ROOT) result = run.harvest(report, ids) @@ -627,7 +629,7 @@ def test_raw_output_is_cached_verbatim(self): bin_dir = td / "bin" counter = td / "counter" report_text = testsupport.review_report( - must_fix="- `agent-cmd/command-thin`: sample finding at `agents/x.md:12`." + must_fix="- **`agents/x.md:12`** sample finding." ) stub = testsupport.stub_claude(bin_dir, counter, report_text) env = testsupport.with_path(bin_dir) @@ -1072,7 +1074,7 @@ class TestNonReviewOutputIsRejected(unittest.TestCase): 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( + rc, stdout, stderr, results_dir, cache_root = run_one_pr_with_payload( td, "Unknown command: /coding:pr-review" ) @@ -1111,7 +1113,7 @@ def test_section_names_outside_headings_do_not_satisfy_the_gate(self): ) with tempfile.TemporaryDirectory() as td: td = pathlib.Path(td) - rc, stderr, results_dir, cache_root = run_one_pr_with_payload(td, payload) + rc, stdout, stderr, results_dir, cache_root = run_one_pr_with_payload(td, payload) self.assertEqual(rc, 1) self.assertIn(run.NON_REVIEW_MARKER, stderr) @@ -1138,7 +1140,7 @@ 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( + rc, stdout, stderr, results_dir, cache_root = run_one_pr_with_payload( td, testsupport.review_report(nice_to_have=None) ) self.assertEqual(rc, 1) @@ -1162,7 +1164,7 @@ def test_missing_section_names_are_reported_exactly(self): # 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( + rc, stdout, 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) @@ -1201,7 +1203,7 @@ 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( + rc, stdout, stderr, results_dir, cache_root = run_one_pr_with_payload( td, testsupport.review_report(heading_level=level) ) @@ -1976,8 +1978,8 @@ def test_numbered_capture_findings_carry_attribution(self): f"first five findings: expected {expected}, got: {observed}" ) self.assertEqual( - len(result.findings), 7, - f"expected 7 total findings, got: {len(result.findings)}" + len(result.findings), 5, + f"expected 5 total findings, got: {len(result.findings)}" ) @@ -2205,5 +2207,158 @@ def test_traceability_capture_rule_ids_not_from_table(self): ) +# ---------------------------------------------------------------------- +# Tests for the unattributable-item gate (spec 005 AC11, AC12, AC13) +# ---------------------------------------------------------------------- +class TestNiceToHaveBulletsAreReportedUnattributable(unittest.TestCase): + """AC13: the unattributed items in the real capture are reported as such.""" + + def test_nice_to_have_bullets_reported_unattributable(self): + text = (run.BENCH_DIR / "testdata" / "capture-numbered-findings-h3.md").read_text() + ids = run.load_rule_ids(run.REPO_ROOT) + result = run.harvest(text, ids) + + self.assertEqual( + len(result.unattributable), 2, + f"expected 2 unattributable items, got: {len(result.unattributable)}" + ) + for u in result.unattributable: + self.assertEqual( + u["section"], "Nice to Have", + f"section must be 'Nice to Have', got: {u['section']}" + ) + + # Verify verbatim bodies (whitespace-collapsed as harvest does) + self.assertEqual( + result.unattributable[0]["body"], + "Manual Trivy apt-install (update/install/repo-key) duplicates the maintained `aquasecurity/setup-trivy` action — adds ~30-60s/run and maintenance surface with no caching/pinning." + ) + self.assertEqual( + result.unattributable[1]["body"], + "Commit subject `switch build backend to hatchling and add conventional changelog prefixes` is 73 chars (soft cap 50) — FYI only, not in the active rule set." + ) + + # The five Should Fix findings are still present + expected = [ + ("CHANGELOG.md", 18, "changelog/conventional-prefix-required"), + ("README.md", 76, "readme/user-facing-not-agent-context"), + (".github/workflows/ci.yml", 32, None), + ("Makefile.precommit", None, None), + ("Makefile.precommit", None, None), + ] + observed = [(f["path"], f["line"], f["rule_id"]) for f in result.findings[:5]] + self.assertEqual(observed, expected) + + +class TestUnattributableItemFailsThePrLoudly(unittest.TestCase): + """AC11: an unattributable item fails the PR loudly with no row and no cache marker.""" + + def test_unattributable_item_fails_the_pr_loudly(self): + with tempfile.TemporaryDirectory() as td: + td = pathlib.Path(td) + rc, stdout, stderr, results_dir, cache_root = run_one_pr_with_payload( + td, + testsupport.review_report( + should_fix="- an item with neither a file reference nor a rule tag, so it cannot be keyed." + ), + ) + + # Run must fail + self.assertNotEqual(rc, 0, "run must exit non-zero for unattributable item") + + # The frozen literal appears in stderr + self.assertIn("UNATTRIBUTABLE FINDING", stderr) + self.assertEqual( + run.UNATTRIBUTABLE_MARKER, "UNATTRIBUTABLE FINDING", + "UNATTRIBUTABLE_MARKER is a frozen spec invariant" + ) + # PR id and section name in diagnosis + self.assertIn("test#1", stderr) + self.assertIn("Should Fix", stderr) + # Item text verbatim + self.assertIn( + "an item with neither a file reference nor a rule tag", + 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 unattributable review") + + # Exactly one file under reviews_root: the raw stdout, no .json marker + reviews = run.reviews_root(cache_root) + if reviews.exists(): + stdout_files = list(reviews.glob("*.stdout.txt")) + json_files = list(reviews.glob("*.json")) + else: + stdout_files, json_files = [], [] + self.assertEqual( + len(stdout_files), 1, + "exactly one .stdout.txt file expected (the raw capture)" + ) + self.assertEqual( + len(json_files), 0, + "zero .json row markers for unattributable review" + ) + + # Failure artifact exists and contains the item text + failures = run.failures_root(cache_root) + if failures.exists(): + failure_files = list(failures.glob("*")) + else: + failure_files = [] + self.assertGreater( + len(failure_files), 0, + "at least one failure artifact must exist" + ) + # Read first failure artifact and check it contains the item text + failure_text = failure_files[0].read_text() + self.assertIn( + "an item with neither a file reference nor a rule tag", + failure_text, + ) + + # Stdout summary reports 1 failed + self.assertIn("1 failed", stdout) + + +class TestAttributedItemStillProducesARow(unittest.TestCase): + """AC12: an item with a path reference exits 0 and produces a row and cache marker.""" + + def test_attributed_item_still_produces_a_row(self): + with tempfile.TemporaryDirectory() as td: + td = pathlib.Path(td) + rc, stdout, stderr, results_dir, cache_root = run_one_pr_with_payload( + td, + testsupport.review_report( + should_fix="- **`src/x.py:4`** an item with a file reference and no rule tag." + ), + ) + + # Run must succeed + self.assertEqual(rc, 0, "run must exit 0 for attributed item") + + # One ledger row with the finding + ledger = run.ledger_path(results_dir) + rows = [json.loads(ln) for ln in ledger.read_text().splitlines()] + self.assertEqual(len(rows), 1, "exactly 1 ledger row expected") + self.assertEqual(rows[0]["findings"][0]["path"], "src/x.py") + self.assertEqual(rows[0]["findings"][0]["line"], 4) + + # Two files under reviews_root: raw stdout + one json marker + reviews = run.reviews_root(cache_root) + stdout_files = list(reviews.glob("*.stdout.txt")) + json_files = list(reviews.glob("*.json")) + self.assertEqual(len(stdout_files), 1, "one .stdout.txt file expected (the raw capture)") + self.assertEqual( + len(json_files), 1, + "exactly one .json row marker for attributed review" + ) + + if __name__ == "__main__": unittest.main() diff --git a/prompts/in-progress/025-spec-005-unattributable-item-rejection.md b/prompts/completed/025-spec-005-unattributable-item-rejection.md similarity index 99% rename from prompts/in-progress/025-spec-005-unattributable-item-rejection.md rename to prompts/completed/025-spec-005-unattributable-item-rejection.md index 3fc6a8d..fda4bc9 100644 --- a/prompts/in-progress/025-spec-005-unattributable-item-rejection.md +++ b/prompts/completed/025-spec-005-unattributable-item-rejection.md @@ -1,8 +1,12 @@ --- -status: approved +status: completed spec: [005-bug-bench-harvest-finding-extraction] +execution_id: coding-exec-025-spec-005-unattributable-item-rejection +dark-factory-version: v0.192.9 created: "2026-08-08T11:43:00Z" queued: "2026-08-08T12:14:19Z" +started: "2026-08-08T12:33:56Z" +completed: "2026-08-08T12:41:44Z" --- From ca2c3c6cd813932126b17eb24aada3675b2e3e36 Mon Sep 17 00:00:00 2001 From: Benjamin Borbe Date: Sat, 8 Aug 2026 14:45:00 +0200 Subject: [PATCH 5/6] 026-spec-005-harvest-contract-docs-and-changelog --- CHANGELOG.md | 8 ++- bench/README.md | 59 +++++++++++++++---- ...005-harvest-contract-docs-and-changelog.md | 7 ++- 3 files changed, 59 insertions(+), 15 deletions(-) rename prompts/{in-progress => completed}/026-spec-005-harvest-contract-docs-and-changelog.md (98%) diff --git a/CHANGELOG.md b/CHANGELOG.md index c3ca46d..3b4a470 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,9 +10,11 @@ Please choose versions by [Semantic Versioning](http://semver.org/). ## Unreleased -- fix: bench runner — ordered list items (`1.`, `2.`, `10.`) now open findings exactly as unordered items do; items inside fenced code blocks are ignored -- fix: bench runner — item-body normaliser no longer strips one asterisk from a leading bold run, preserving `**path**` references in finding text -- test: bench — add 5 new tests covering ordered/unordered list recognition, fenced-item suppression, and leading-bold-run preservation; add sha256 digest verification for 4 verbatim capture fixtures +- fix: bench runner — a trailing bold-label block (`**Notes:**`, `**Summary:**`) now ends a findings section, so housekeeping bullets after a clean review are no longer invented as findings +- fix: bench runner — numbered/ordered items open a finding exactly as bullets do, so the reviewer's most severe tier is no longer dropped; a list item inside a fenced code block opens nothing; the item's leading bold run survives marker stripping verbatim +- fix: bench runner — `path`, `line` and `rule_id` are read from the reviewer's own markers: the item's leading bold reference for path/line, and the inline `*(rule: \`\`)*` tag for rule id, recorded literally and independent of the runner's `rules/index.json`; the head-anchored legacy backtick shape retained as an index-gated fallback only when no inline tag is present +- fix: bench runner — an item inside a severity section that yields no attribution now fails the PR loudly with `UNATTRIBUTABLE FINDING`, leaving no ledger row and no row marker while preserving the raw capture, in the same class as the existing `NOT A REVIEW` gate +- docs: bench — document the harvest contract in `bench/README.md` § "Reading review output" (what ends a section, what opens a finding, where attribution comes from, when a finding cannot be attributed) and add four verbatim-capture fixtures that lock it down ## v0.35.3 diff --git a/bench/README.md b/bench/README.md index d5eaa99..f93c95b 100644 --- a/bench/README.md +++ b/bench/README.md @@ -94,26 +94,60 @@ The rejection names the PR, names each missing section on its own `missing secti ### 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. +A findings section's content ends at the **next markdown heading of any level**, at a **thematic break**, at a **bold-run line outside a fenced code block** (`**` as the first non-whitespace content of a 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. The terminating heading's level carries no information and is not used: level varies run to run inside a single mode, so relying on it would make boundary determination a property of the template rather than of the output. -> **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. +The two bold-label shapes observed defeating the old boundary were `**Notes:**` and `**Summary**:` — housekeeping blocks appended after a clean review whose trailing bullets were then harvested as findings. + +> **Why a bold label ends a section.** A review whose three severity sections all read `None.` — the correct answer, zero — was harvested as three findings, verbatim the three bullets of a trailing `**Notes:**` block. A bold label introduces a new block, and everything under it belongs to that block. ### 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. +Both list styles open a finding: an **unordered item** (`-` or `*` followed by whitespace) and an **ordered/numbered item** (a run of digits followed by `.` and whitespace). Numbered items need not start at 1 and are not limited to one digit. Subsequent non-list lines extend the item already open. A line inside a fenced code block is ordinary text and opens nothing. Prose appearing in a section **before** the first list item — most importantly the mandated `None.` sentinel — contributes no finding and cannot be extended by anything that follows. Only the list marker is stripped from the body; the item's leading bold run survives intact. -> **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. +> **Why numbered items matter most.** The reviewer writes its more severe tiers as numbered lists. A capture carrying five numbered Should Fix items harvested to two — the two Nice to Have bullets — so the parser lost precisely the findings that matter most, and left no trace of the loss. -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. +### Where attribution comes from -### Fixtures +`path` and `line` are read from the **bold run at the head of the item**, in the shapes the reviewer writes. A path or line appearing only in the item's trailing prose is not used when the leading bold run supplies one; no path is ever inferred by searching the repository and no line is guessed from surrounding text. + +`rule_id` has **two sources, in strict priority order**: + +1. The item's own inline `*(rule: \`\`)*` marker. When present it always wins and is recorded as the literal string the reviewer wrote, **whether or not that id appears in `rules/index.json`**. +2. Only when no inline marker is present, a backticked token at the very head of the item that **is a member of `rules/index.json`**. This legacy shape is what three findings in `bench/testdata/sample-report.md` depend on. It stays index-gated: at the head of an item, an unknown backticked token is far more likely to be a file path or symbol than a rule name. -| Fixture | Origin | Harvests to | +Under either source, an id named in the item's prose, in the item's tail, in a traceability table, or anywhere outside the item is never attributed to it. + +The four observed bold-reference shapes and their resulting `path` and `line` values: + +| Bold reference shape | `path` | `line` | |---|---|---| -| `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 | +| `**bench/run.py:1037**` | `bench/run.py` | `1037` | +| `**README.md**` | `README.md` | `None` | +| `**src/server.go:42**` | `src/server.go` | `42` | +| `**pkg/config.ts:18-24**` | `pkg/config.ts` | `18` | + +> **Why the inline marker is not validated against the rule index.** A tag naming a renamed rule, a rule added since the index was written, or a review run against a different rules revision would otherwise yield `null` — attribution would become a property of the runner's bookkeeping rather than of the reviewer's output. The instrument records what the reviewer claimed; reconciling that against the shipped rule set is the scorer's job, and coupling the two hides rule drift instead of surfacing it. The head-anchored legacy source keeps its index gate because it has no explicit marker to trust: there, membership is the only signal that a backticked token is a rule name at all. + +### When a finding cannot be attributed + +An item inside a severity section that yields neither a `path` nor a `rule_id` cannot be keyed, cannot be matched against a golden set, and is never written as a body-only finding. Such a PR fails with `UNATTRIBUTABLE FINDING` — in the same class as the existing `NOT A REVIEW` gate: no ledger row, no `.json` row marker, the PR listed as failed, remaining PRs still processed, process exits non-zero. + +The two gates differ in what they leave behind. `NOT A REVIEW` fires before the raw-output write and leaves nothing; `UNATTRIBUTABLE FINDING` fires after it, so the `.stdout.txt` stays on disk and the review is re-harvestable after a parser fix without spending tokens again. A both-stream failure artifact is written under `bench/.cache/failures/`. There is no opt-out. + +> **Why a body-only finding is refused.** A finding with no path and no rule id is an unmatchable measurement dressed as a data point. A false rejection costs one operator decision and a re-run; a false acceptance writes an unscoreable row into an append-only ledger. + +### Fixtures + +| Fixture | Origin | `sha256` | Harvests to | +|---|---|---|---| +| `bench/testdata/capture-notes-block-h2.md` | verbatim capture, `node-skeleton#2` short mode, `##` headings, all three sections `None.` with a trailing `**Notes:**` block | `6427028bef301ff822cca6dbf9308896f1899ac5a972ed3fddc276f2216552b9` | 0 findings, 0 unattributable | +| `bench/testdata/capture-numbered-findings-h3.md` | verbatim capture, `python-skeleton#3` full mode, `###` headings, five numbered Should Fix items with inline rule tags plus a positive-notes list | `5530049fa4d116dc5762b69c9c9498ff0865c0ae0c6b1de7b3ae4cc846643e93` | 5 findings, 2 unattributable | +| `bench/testdata/capture-traceability-h4.md` | verbatim capture, `node-skeleton#2` full mode, `####` headings, one bold-headed finding plus a 22-row traceability table | `2922746bb95bdb3a67a683942531362271d8f3ccd558067d910146e054bcfe7c` | 1 finding, 0 unattributable | +| `bench/testdata/capture-summary-trailer-h4.md` | verbatim capture, `github-pr-review-agent#11` short mode, `####` headings, all three sections `None.` with a `**Summary:**` trailer | `36e15eca61133033d81687f87a82b044333c6a7465508d1757f8493361137e79` | 0 findings, 0 unattributable | +| `bench/testdata/sample-report.md` | derived from the review command's Step 5 template, `####` headings | `de40c00e7d3c452fa7475be9fa6541426058a96dba91487460aa53be0bd186ae` | 3 findings | +| `bench/testdata/real-capture-report.md` | verbatim capture of live review output, `##` headings, all three sections `None.`, trailing prose | `be1400f065d6b856910e7ac91c7f4801598b57afb444f55cf2e257a43619f4db` | 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. +Both defects this section documents survived 42 green unit tests because the tests were built from the same template the parser was built from. Every capture checked in before this change carried zero findings — no fixture had ever exercised the parser against a real finding, which is how this defect family reached five occurrences. A fixture for a new defect must be a **capture of real output**, not a transcription of the template. ## Verifying an entry without cloning @@ -135,7 +169,10 @@ These are deliberately not configurable: - **Plugin load path:** the runner hashes the directory named by the isolated config directory's `installed_plugins.json` record (the directory Claude Code really loads from), not the marketplace path; there is no fallback when the record is absent, unreadable, stale, out-of-tree, scope-mismatched, or hash-mismatched - **Two-ref guarantee:** the prepared working copy carries exactly the checked-out head branch and the two synthetic remote-tracking refs for that PR; every other branch, tag and the default-branch symref are removed on every run - **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 +- **List-item markers:** both `-` and `*` unordered items and digit-run ordered items open a finding; prose before the first list item cannot form one; a list item inside a fenced code block opens nothing +- **Section terminators:** next heading of any level, thematic break, bold-run line outside a fence, end of input +- **Inline rule tag:** `*(rule: \`\`)*` is read positionally from the item and recorded verbatim; it is **not** validated against `rules/index.json`; the head-anchored backtick fallback is **index-gated** (only used when no inline tag is present and the token is a known rule id) +- **Unattributable-item rejection:** `UNATTRIBUTABLE FINDING` fires when an item inside a severity section yields neither `path` nor `rule_id`; no ledger row, no row marker, no opt-out - **Stderr excerpt bound:** at most 2,000 bytes of rejected output are printed to stderr (truncation is marked) ## Safety invariant diff --git a/prompts/in-progress/026-spec-005-harvest-contract-docs-and-changelog.md b/prompts/completed/026-spec-005-harvest-contract-docs-and-changelog.md similarity index 98% rename from prompts/in-progress/026-spec-005-harvest-contract-docs-and-changelog.md rename to prompts/completed/026-spec-005-harvest-contract-docs-and-changelog.md index e020fa0..923c3c4 100644 --- a/prompts/in-progress/026-spec-005-harvest-contract-docs-and-changelog.md +++ b/prompts/completed/026-spec-005-harvest-contract-docs-and-changelog.md @@ -1,8 +1,13 @@ --- -status: approved +status: completed spec: [005-bug-bench-harvest-finding-extraction] +summary: 'Documented harvest contract in bench/README.md § Reading review output (4 sub-sections), extended fixtures table to 6 rows, rewrote CHANGELOG.md ## Unreleased with 5 bullets covering the whole of spec 005' +execution_id: coding-exec-026-spec-005-harvest-contract-docs-and-changelog +dark-factory-version: v0.192.9 created: "2026-08-08T11:44:00Z" queued: "2026-08-08T12:14:20Z" +started: "2026-08-08T12:41:45Z" +completed: "2026-08-08T12:45:00Z" --- From ccf8f5344ed4c71560835d392bddcda1271eaa7d Mon Sep 17 00:00:00 2001 From: Benjamin Borbe Date: Sat, 8 Aug 2026 14:55:23 +0200 Subject: [PATCH 6/6] mark spec 005 as verifying --- specs/in-progress/005-bug-bench-harvest-finding-extraction.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/specs/in-progress/005-bug-bench-harvest-finding-extraction.md b/specs/in-progress/005-bug-bench-harvest-finding-extraction.md index 417ceae..aced40c 100644 --- a/specs/in-progress/005-bug-bench-harvest-finding-extraction.md +++ b/specs/in-progress/005-bug-bench-harvest-finding-extraction.md @@ -1,11 +1,12 @@ --- -status: prompted +status: verifying tags: - dark-factory - spec approved: "2026-08-08T11:24:09Z" generating: "2026-08-08T12:14:23Z" prompted: "2026-08-08T12:14:23Z" +verifying: "2026-08-08T12:45:01Z" branch: dark-factory/bug-bench-harvest-finding-extraction ---