bench: reject the offending item, not the whole review - #95
Merged
Conversation
A 20-PR Opus pass produced 13 rows and 7 failures - a 35% loss rate - and every failure was an output-shape rejection of a substantive review. The segmentation shows it is not the PRs: the original dev-5, which the harness was iterated against until they passed, lost 0%; the 15 new PRs lost 47%. tts-mcp#10 lost a complete review to ONE unattributable item. Both gates now reject at item granularity: - An unattributable finding is dropped from the row and counted in a new unattributable_count field. Recorded, never silent: removing it quietly would make precision improve for a reason nothing records, which is the defect class this bench exists to catch. - The sanity gate rejects only when EVERY severity section is absent. It exists for D2, where an unknown command's output read as a clean review - that case has no sections at all. Partial sets score and their absent section names are recorded in a new missing_sections field. The guarantee that every scored finding carries a usable matching key is unchanged. This is a change of granularity, not of contract. Two existing tests asserted the old row-level behaviour and were amended to the new contract rather than deleted. The two new behaviour tests were verified to fail against the pre-change code; a third pins the D2 case so loosening the sanity gate cannot silently re-open it.
There was a problem hiding this comment.
I have enough context to perform the in-session adjudication. Let me execute the full analysis now.
Step 4c-sel CLASSIFY:
Candidates from Step 4b-i against CHANGELOG.md, bench/run.py, bench/test_review.py:
| Rule | Owner | Verdict |
|---|---|---|
changelog/conventional-prefix-required |
agent-auditor | Applicable — CHANGELOG.md changed |
python-architecture/main-py-composition-root |
python-architecture-assistant | Skipped — bench/run.py is a benchmark harness CLI script, not an application that wires service objects at module level; the rule targets application composition roots, not tooling |
changelog/unreleased-entry-required |
agent-auditor | Skipped — enforcement_type is script, not judgment; not in candidate set |
No mechanical findings from Step 4a (0 yamls run, 0 findings).
Step 4d-sel ADJUDICATE:
changelog/conventional-prefix-required: All 3 bullets in## Unreleasedusefix:/test:— valid prefixes. ✓- No violations found in the diff for applicable rules.
Manual review of the Python changes confirms correctness:
- Gate change (
if missing:→if len(missing) == len(REQUIRED_SECTION_NAMES)) is logically sound unattributable_countis always set beforebuild_rowis calledmissing_sectionsis always a list (defaulted to[]when None)- New test
TestGatesRejectItemsNotRowsreproduces all three cases correctly
{
"verdict": "approve",
"summary": "The gate-granularity fix correctly rejects only the offending item (unattributable finding or fully-absent sections) rather than the whole review row. All three changelog bullets use valid conventional prefixes. The sanity gate now only fires when ALL severity sections are absent (D2 case), preserving partial-section reviews. Unattributable items are dropped with a count on the row instead of raising BenchError. The three new gate tests and two amended tests accurately assert the new contract.",
"comments": [],
"concerns_addressed": [
"correctness: sanity gate now requires ALL sections absent (len check) — partial section sets score, D2 (no sections) still rejected",
"correctness: unattributable items dropped and counted in `unattributable_count`, row always written",
"correctness: new row fields `unattributable_count` (int, default 0) and `missing_sections` (list, default []) added to `build_row()`",
"tests: two existing tests amended to assert new contract; three new `TestGatesRejectItemsNotRows` test cases cover all gate scenarios"
]
}
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A 20-PR Opus pass on 2026-08-09 produced 13 rows and 7 failures — a 35% loss rate. Every
failure was an output-shape rejection of a substantive review.
The segmentation shows it is not the PRs
Zero failures on the five PRs the gates were tuned against; nearly half rejected on PRs they
were not.
tts-mcp#10lost a complete review to one unattributable item.The change — granularity, not contract
unattributable_countfield, instead of discarding the PR. Recorded, never silent:removing an item quietly would make precision improve for a reason nothing records, which
is exactly the defect class this bench exists to catch.
NOT A REVIEWgate rejects only when every severity section is absent. It existsfor D2 — an unknown command whose output read as a clean review — and that case has no
sections at all. Requiring all three discarded
discord-assistant#5for carrying Should Fixand Nice to Have but not Must Fix. Partial sets now score, with absent names recorded in a
new
missing_sectionsfield.The guarantee that every scored finding carries a usable matching key is unchanged.
Why it matters
[[Curate the Full Benchmark PR Set]] exists to pull 3σ from ~119% of the mean toward ~60% by
quadrupling the fixture. At a 35% loss rate a 20-PR set yields 13 — barely above the fixture
it replaces — so SC4 stayed out of reach. This is the blocker.
Verification
make precommitgreen — 199 testssymptom
AssertionError: 0 != 1(no row produced)defect it was built for — it passes both before and after
contract rather than deleted
The measurement that actually closes the parent task is a re-run of the full 20-PR pass, which
belongs to that task, not this PR.