Add neutral ignored_comments bucket to code-review scoring

[Wenjie Fan (gggdttt)]

What

Adds an optional ignored_comments list to each code-review gold entry (CodeReviewEntry). A generated comment that matches an ignored comment is treated as neutral: it is dropped from scoring entirely — it earns no recall and does not count against precision.

This gives the code-review category a third comment state alongside the two it has today:

Bucket	Field	Effect on scoring
Required	expected_comments	Must be found (drives recall); a match is correct (precision).
Acceptable / neutral (new)	ignored_comments	Neither required nor penalized — dropped from scoring.
Everything else	(omission)	Any comment not matching an expected one is a false positive (precision).

The neutral bucket captures legitimate-but-debatable or out-of-scope findings that we do not want to force the agent to raise, but also do not want to punish it for raising (the long tail of "maintainer-judgment / accepted-noise" comments).

How it scores

1. Expected and ignored comments are structurally matched (file + line proximity). Ignored is matched only against the generated comments left over after expected structural matching, so expected takes precedence.
2. Both structural buckets are validated in a single LLM judge pass (judge_comment_matches_split), then split back apart by bucket - one judge subprocess per evaluation, so no extra run-to-run nondeterminism, no second calibration target, and no stale verdict-file reuse.
3. Ignored matches are removed from the scored generated set:
   • precision = matched / (generated - ignored)
   • recall is unchanged (ignored comments never grant recall).
4. Expected always takes precedence — ignored only ever sees the structural leftovers, so a generated comment lands in at most one bucket.

precision_recall(0, 0, N) already returns precision 1.0 (the "correct silence" convention), so a positive task whose only output is an ignored comment scores precision 1.0 / recall 0.0 — it is treated as effectively silent, and macro precision keys on scorable comments so it is not rewarded as if it had commented correctly.

Surfacing

• ignored_comment_count is added to CodeReviewResult, CodeReviewResultSummary, and category_metrics.
• Micro precision in the summary subtracts the ignored total so aggregate precision stays consistent with per-task precision.
• An Ignored column is added to the markdown and console comment-count tables.
• docs/code-review.md documents the neutral bucket and the new column.

Backward compatibility

ignored_comments defaults to empty on every existing gold entry, so ignored_count = 0 and scoring is identical to today. All 81 gold entries are unaffected.

Tests / validation

• New tests cover: ignored is neutral (no recall, no precision hit), ignored does not grant recall, expected precedence, empty ignored = baseline, micro precision excludes ignored, and a positive-task-with-only-ignored macro-precision case.
• ruff format + ruff check clean, ty clean, full suite 678 passed / 1 skipped (was 668 + 10 new tests).

Open decisions (safe defaults — flagging for review)

1. Judge-gated ignored matching — ignored matches go through the LLM judge, mirroring expected matching, for tight semantics. Both buckets share a single judge pass (judge_comment_matches_split), so this adds no extra LLM call. Alternative would be structural-only.
2. Ignored counts surfaced on dashboards — added to summary tables + category_metrics for transparency. Easy to hide if we'd rather keep them internal.

Happy to adjust either based on how we want to author gold entries.

[Copilot]

🟡 Not ready to approve

The second judge pass can reuse stale expected-match verdicts and produce incorrect scores.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Pull request overview

Adds neutral ignored_comments support to code-review scoring.

Changes:

• Matches ignored comments after expected comments and excludes them from precision.
• Surfaces ignored counts in metrics and reports.
• Adds model, documentation, and scoring tests.

File summaries

File	Description
src/bcbench/dataset/codereview.py	Defines ignored gold comments.
src/bcbench/evaluate/codereview.py	Adds ignored-comment judge pass.
src/bcbench/results/codereview.py	Updates scoring, aggregation, and reporting.
tests/conftest.py	Extends code-review test factories.
tests/test_codereview.py	Tests neutral scoring and aggregation.
docs/code-review.md	Documents ignored comments.

Review details

• Files reviewed: 6/6 changed files
• Comments generated: 3
• Review effort level: Balanced

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[Copilot]

🟡 Changes recommended

Ignored matching excludes structurally paired comments before expected matches are semantically validated, causing incorrect precision penalties.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Review details

Suppressed comments (1)

src/bcbench/evaluate/codereview.py:80

• leftover is computed before the semantic judge confirms the expected pairs. Because match_comments can structurally pair any same-file comment, a generated finding that semantically matches an ignored comment can first be paired with an unrelated expected comment, receive a false expected verdict, and then never be considered for the ignored bucket. It is therefore counted as an incorrect comment instead of neutral. Run expected judging first and derive leftovers from the validated expected pairs before matching/judging ignored comments (or implement equivalent reassignment while preserving expected precedence).

            leftover = unmatched_generated(generated_comments, structural_matches)
            ignored_structural = match_comments(context.entry.ignored_comments, leftover)

• Files reviewed: 7/7 changed files
• Comments generated: 0 new
• Review effort level: Balanced

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[Copilot]

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.

[Sun Haoran (haoranpb)]

Looks good, would be great to get test coverage on your critical metrics calculation path

[Wenjie Fan (gggdttt)]

Thanks Sun Haoran (@haoranpb) - round 2 pushed (f156cbe). Summary:

Test coverage (the blocking asks)

• unmatched_generated now has TestUnmatchedGenerated (incl. identity-not-value-equality).
• Extracted the scoring math into a pure _score_counts helper with TestScoreCounts (normal split, all-ignored, ignored-reduces-scored-set).
• Added precedence tests to TestJudgeExpectedAndIgnored.

Naming / dedup

• leftover -> unmatched_generated_comments; judge_comment_matches removed; judge_comment_matches_split -> judge_expected_and_ignored. The only functions with an LLM subprocess are now judge_verdicts and judge_expected_and_ignored.
• ignored_comment_count -> ignored_matched_comment_count (it always counted matched ignored comments, not dataset size).

Correctness (from the bot thread)

• Ignored comments are matched against all generated comments and expected precedence is applied after the judge, so a finding whose expected pair the judge rejects can still be neutralized as ignored instead of counting as a false positive. Single judge pass preserved.

Gauntlet green: ruff format/check + ty clean, 682 passed / 1 skipped (+10 new tests).

[Sun Haoran (haoranpb)]

Looks good overall.

One design question: the distinction between ignored_comments and severity is not immediately clear to me. My understanding is that severity represents impact, while ignored_comments represents whether a finding is required for benchmark scoring. A low-severity expected comment still affects recall, whereas an ignored comment is neutral. Could we document that rationale and the criteria for choosing one bucket over the other?

This also highlighted that the core F score weights every expected finding equally: missing a critical issue has the same effect as missing a low-severity issue. Is that intentional? If impact should affect the leaderboard, severity-weighted precision/recall could provide a more general model, potentially ignored_comments can simplify be zero-weight findings => much easier.

[Sun Haoran (haoranpb)]

Thanks Sun Haoran (Sun Haoran (@haoranpb)) - round 2 pushed (f156cbe). Summary:

Test coverage (the blocking asks)

• unmatched_generated now has TestUnmatchedGenerated (incl. identity-not-value-equality).
• Extracted the scoring math into a pure _score_counts helper with TestScoreCounts (normal split, all-ignored, ignored-reduces-scored-set).
• Added precedence tests to TestJudgeExpectedAndIgnored.

Naming / dedup

• leftover -> unmatched_generated_comments; judge_comment_matches removed; judge_comment_matches_split -> judge_expected_and_ignored. The only functions with an LLM subprocess are now judge_verdicts and judge_expected_and_ignored.
• ignored_comment_count -> ignored_matched_comment_count (it always counted matched ignored comments, not dataset size).

Correctness (from the bot thread)

• Ignored comments are matched against all generated comments and expected precedence is applied after the judge, so a finding whose expected pair the judge rejects can still be neutralized as ignored instead of counting as a false positive. Single judge pass preserved.

Gauntlet green: ruff format/check + ty clean, 682 passed / 1 skipped (+10 new tests).

Good work, see above comment for the challenge on the design here. Wenjie Fan (@gggdttt)