Skip to content

Let a review comment declare several BCQuality articles - #805

Merged
Wenjie Fan (gggdttt) merged 1 commit into
mainfrom
bench/comment-multi-article
Aug 20, 2026
Merged

Let a review comment declare several BCQuality articles#805
Wenjie Fan (gggdttt) merged 1 commit into
mainfrom
bench/comment-multi-article

Conversation

@gggdttt

Copy link
Copy Markdown
Collaborator

Problem

ReviewComment.article was singular, so an expected comment could be attributed to exactly one BCQuality article. That does not match how the gold answers are actually written: a number of them pair a positive requirement with an explicit false-positive boundary in one comment body, and those are two separate articles in BCQuality.

synthetic__error-handling-errorinfo-actionable-boundary-01
  "Raise ErrorInfo with RecordId = SalesLine.RecordId, PageNo set to the Sales Lines page,
   and a navigation action ... Do not require a Fix-it for Synthetic Save Target: that
   validation occurs on the selectable field itself ..."

synthetic__events-ishandled-reset-boundary-01
  "Start this raise from false and let subscribers opt in. Do not flag ValidateQuantity's
   fresh local or ValidateQuantityAndLocation's second raise ..."

This is deliberate, not incidental — the instance ids literally carry -boundary-. As the article corpus grows, one finding mapping to several articles becomes more common, not less.

Routing the second article through metadata.articles looked like an escape hatch, but it is a dead end. It holds only while no other comment in the entry owns that article:

case 1 (one comment needs A+B):     OK    declared=[A, B]
case 2 (c1 needs A+B, c2 needs B):  FAIL  article(s) [B] declared both per-comment
                                          and in metadata.articles

Case 2 is ordinary — two comments in one entry citing the same article — so the workaround breaks in normal use.

Change

  • ReviewComment.article: ArticleId | None -> ReviewComment.articles: list[ArticleId].
  • Migrated the 17 existing annotations in dataset/codereview.jsonl to the list form. No compatibility alias is kept; the field is days old and the dataset is fully in-repo, so carrying a shim would just be debt.
  • Set extra="forbid" on ReviewComment. Pydantic ignores unknown keys by default, so without this a leftover "article": "..." would be silently dropped — a stale annotation would become invisible coverage loss instead of an error. Verified no comment payload in the dataset carries any other extra key, and both programmatic construction sites (collection/collect_codereview.py, evaluate/review_parsing.py) pass explicit keyword arguments.
  • declared_articles() and the disjointness validator now flatten per-comment lists; metadata.articles keeps its existing false-positive-guard role and the disjointness rule is unchanged.
  • Refreshed the notebook outputs, whose committed run still reported 130 entries (it predates Add second PR reviewer self-improvement gold batch #803). Now 9 articles covered across 17/144 annotated entries; 241 of 250 articles have zero coverage.

Validation

  • Migration audited structurally: parsed all 144 entries before and after, compared every field except the article keys — zero drift; 17 annotations moved across 13 entries, and the article set per entry is unchanged.
  • New tests cover a comment declaring several articles, and that a superseded singular article key is now rejected rather than ignored.
  • ruff format, ruff check clean; pre-commit run --files notebooks/code-review-coverage.ipynb fully green; ty check reports only the pre-existing src/bcbench/redteam.py:150 unused-ignore warning.
  • pytest -q: 740 passed, 2 skipped.

A single expected comment can exercise more than one article, most often when it pairs a positive requirement with an explicit false-positive boundary (synthetic__error-handling-errorinfo-actionable-boundary-01, synthetic__events-ishandled-reset-boundary-01). The singular article field forced such findings to drop one of the two, and routing the overflow through metadata.articles is a dead end: the disjointness validator rejects it as soon as another comment in the same entry legitimately owns that article.

Replace ReviewComment.article with articles: list[ArticleId] and migrate the 17 existing annotations. Set extra=forbid on the model so a leftover singular article key fails loudly instead of being silently ignored, which would otherwise turn a stale annotation into invisible coverage loss. Also refresh the notebook outputs, whose committed run still reported 130 entries.
@gggdttt
Wenjie Fan (gggdttt) merged commit 06bd9ef into main Aug 20, 2026
14 checks passed
@gggdttt
Wenjie Fan (gggdttt) deleted the bench/comment-multi-article branch August 20, 2026 10:59
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.

2 participants