Skip to content

fix(bench): harvester counts prose as findings and drops numbered ones - #85

Merged
bborbe merged 6 commits into
masterfrom
feature/bench-harvest-extraction
Aug 8, 2026
Merged

fix(bench): harvester counts prose as findings and drops numbered ones#85
bborbe merged 6 commits into
masterfrom
feature/bench-harvest-extraction

Conversation

@bborbe

@bborbe bborbe commented Aug 8, 2026

Copy link
Copy Markdown
Owner

Spec 005-bug-bench-harvest-finding-extraction (kind: bug). The HARVEST layer both invented findings that did not exist and dropped findings that did — simultaneously. Test count 72 → 103; make precommit 30/30 green.

The defect

A node-skeleton#2 review whose three severity sections all read None. was recorded as 3 findings — verbatim the three bullets under a trailing **Notes:** block. Meanwhile a python-skeleton#3 review with five numbered Should Fix items, each carrying a bold path reference and an inline rule tag, yielded 0 of them: the parser matched - bullets only, and the reviewer uses numbered lists for its more severe tiers.

Measured across every run before the fix:

mode     findings  path     line     rule_id
short    27        9/27     9/27     0/27
full      3        0/3      0/3      0/3

Precision 3/6, recall 3/8, attribution 0/6. For a measurement instrument a plausible wrong number is worse than a crash — this one had already produced a published wrong conclusion, and it blocked the downstream golden-set work that needs a stable finding key.

Four root causes

  1. Sections were not terminated by a bold label. **Notes:** is not a heading, so its bullets were absorbed into the preceding section.
  2. Numbered items were invisible. Pure recall hole on the most severe tier.
  3. Rule tags existed in the output and were ignored. *(rule: ...)* markers were present; nothing parsed them.
  4. _normalize_body corrupted the body, stripping one leading * from **No test coverage for ... at exactly the position the path reference lives.

Governing rule

The harvester extracts what the review contract defines, or refuses loudly. A finding it cannot attribute is a parse failure, not a finding — reported with the frozen literal UNATTRIBUTABLE FINDING, in the same class as the existing NOT A REVIEW gate. There is deliberately no opt-out; an escape hatch on this goal is the regression the spec exists to close.

Fixture provenance

Four real captures are committed under bench/testdata/ and gated by published sha256, not by content literals. Five defects reached production in this codebase because fixtures were written from the same template as the parser — tests and code agreed with each other and both disagreed with the model's real output. A hash anchors provenance without letting the fixture be reconstructed from the spec.

Verification

  • make precommit — 30/30 checks, 103 tests, green
  • Live end-to-end run over the 5-PR fixture: 5 ok, 0 cache hit, 0 failed
  • Clean review with a trailing **Notes:** block → 0 findings (was 3)
  • Five numbered Should Fix items → 5 extracted, with paths and rule ids (was 0)
  • Every finding produced by the live run carries a real path and line

The stale review cache was cleared before the live run: the config identity is f(rules+commands, model, effort, mode, prs_version) and does not include bench/run.py, so a harvester change would otherwise have been served old rows.

@ben-s-pull-request-reviewer ben-s-pull-request-reviewer Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now I have sufficient context to perform the in-session classification and adjudication. Let me proceed with Steps 4c-sel and 4d-sel.

Step 4c-sel: CLASSIFY

Changed files: CHANGELOG.md, bench/README.md, bench/run.py, bench/test_review.py, bench/testdata/*.md, prompts/**, specs/**

Candidate rules from rules/index.json with enforcement_type=judgment and matching triggers:

  • changelog/conventional-prefix-required — trigger: CHANGELOG.mdapplicable (CHANGELOG.md changed)
  • changelog/preamble-frozen — trigger: CHANGELOG.mdapplicable (CHANGELOG.md changed)
  • changelog/unreleased-entry-required — trigger: CHANGELOG.mdapplicable (CHANGELOG.md changed)

All three agent-auditor rules are applicable. No other rules have glob triggers matching the changed file set (no bench/**, prompts/**, specs/** in triggers).

Step 4d-sel: ADJUDICATE

Reading the relevant rule blocks from changelog-guide.md:

  • changelog/conventional-prefix-required: Every ## Unreleased bullet must start with feat:, fix:, refactor:, test:, docs:, chore:, or perf:. All four new bullets: fix: bench runner — … (×4) ✓
  • changelog/preamble-frozen: No content above # Changelog; no ## Unreleased inside the preamble block. New ## Unreleased is placed after the last MAJOR/MINOR/PATCH bullet, before ## v0.35.3
  • changelog/unreleased-entry-required: At least one non-vendored file changed, ## Unreleased has bullets. ✓

No violations.

Code review (bench/run.py changes):

The four root-cause fixes are correctly implemented:

  1. ORDERED_ITEM_RE added; list_item_body now handles both unordered and ordered list items
  2. BOLD_RUN_START_RE added; bold-run lines terminate a findings section
  3. extract_attribution with RULE_TAG_RE, LEADING_BOLD_RE, PATH_LINE_RE — priority-ordered attribution from item's own markers
  4. UNATTRIBUTABLE_MARKER gate in process_pr fails the PR loudly with no ledger row

The _normalize_body fix (removing the inline bullet-stripping that corrupted leading bold runs) is correct.

Test quality: 31 new tests (72→103) with proper subTest patterns, AC-named classes, corpus-based assertions using real verbatim-capture fixtures. No issues.

Mechanical funnel: 0 findings across 74 YAMLs.


{
  "verdict": "approve",
  "summary": "Bugfix correctly implements four root-cause repairs to the bench runner's harvest layer: ordered list items now open findings, bold-run lines terminate sections, attribution is read from the item's own markers in priority order, and unattributable items fail the PR loudly. All three changelog entries use correct conventional prefixes and are placed correctly relative to the frozen preamble. 31 new tests (72→103) with AC-named classes and verbatim-capture fixtures. Mechanical funnel clean.",
  "comments": [],
  "concerns_addressed": [
    "correctness: ordered list items now open findings in bench/run.py:275 (ORDERED_ITEM_RE + list_item_body)",
    "correctness: bold-run lines terminate sections in bench/run.py:1299 (BOLD_RUN_START_RE check)",
    "correctness: _normalize_body no longer strips leading asterisk from bold path references in bench/run.py:1223",
    "correctness: unattributable gate fires loudly in bench/run.py:1560 (UNATTRIBUTABLE_MARKER)",
    "tests: test count 72->103 with 4 verbatim-capture fixtures covering all root causes"
  ]
}

@bborbe
bborbe merged commit 8ecaae4 into master Aug 8, 2026
1 check passed
@bborbe
bborbe deleted the feature/bench-harvest-extraction branch August 8, 2026 13:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant