Skip to content

Add per-article BCQuality coverage for code-review dataset (AB#646153) - #776

Merged
Wenjie Fan (gggdttt) merged 11 commits into
mainfrom
bench/per-article-coverage
Aug 19, 2026
Merged

Add per-article BCQuality coverage for code-review dataset (AB#646153)#776
Wenjie Fan (gggdttt) merged 11 commits into
mainfrom
bench/per-article-coverage

Conversation

@gggdttt

@gggdttt Wenjie Fan (gggdttt) commented Aug 7, 2026

Copy link
Copy Markdown
Collaborator

What

Adds per-article BCQuality coverage tracking to the code-review category so we can see, per BCQuality knowledge article, which articles have gold coverage and which have none. Follow-up from the 2026-08-06 PR Review Agent sync (AB#646153).

Why

Today gold entries are classified only by coarse domain/area. We cannot tell which specific BCQuality article an entry exercises, nor which articles have zero gold coverage. This blocks Wael's self-improving-agent pipeline from targeting coverage gaps.

Changes

  • Schema (optional, backward-compatible) in dataset/codereview.py:
    • ArticleId — shared <domain>/<slug> constrained alias (e.g. security/secrettext-for-credentials).
    • ReviewComment.article — the article a required finding derives from.
    • CodeReviewEntryMetadata.articles — entry-level list for false-positive-guard entries (expected_comments = []) that test an article by omission and so have no per-comment article. Category-specific metadata lives on this EntryMetadata subclass rather than the shared base.
    • A model validator keeps the two annotation sources disjoint, and declared_articles() returns their union.
    • Existing entries without annotations load unchanged (counted as unannotated).
  • analysis/bcquality_article_coverage.py — pure analysis logic: aggregate declared articles, enumerate the BCQuality article inventory from a checkout, compute covered / zero-coverage / unknown-slug / unannotated. Kept out of dataset, which stays the schema/model layer.
  • notebooks/code-review-coverage.ipynb — the coverage report surface, mirroring the existing notebooks/dataset.ipynb. Inventory comes from $BCQUALITY_ROOT; when absent it degrades to declared-only (zero-coverage undetermined). Rendered outputs are committed so the analysis is readable directly on GitHub.
  • Backfill: the security domain is annotated as a worked example (17 of its 20 entries → 9 distinct articles). Remaining domains are intentionally left for follow-up backfill; the report surfaces them as unannotated.

Example output (committed in the notebook)

9 articles covered across 17/130 annotated entries; 241 of 250 articles have zero coverage
Domain Covered Gold entries Inventory Zero-cov
security 9 17 24 15
performance 0 0 47 47
style 0 0 35 35
ui 0 0 29 29
...

The low overall ratio is expected at this stage: this PR delivers the mechanism plus a single worked domain, so 8 of 9 dataset areas are still unannotated by design.

Validation

ruff format/check clean (including the notebook), ty clean (the one remaining diagnostic is a pre-existing unused-ignore in redteam.py from main, untouched here), pytest 738 passed / 2 skipped. Dataset still loads all 130 entries; the JSONL diff touches only the security lines. Notebook executed end-to-end against BCQuality main, with non-deterministic execution timestamps stripped so the committed outputs stay reviewable.

Notes

  • Additive — no existing scoring/behavior changes.
  • No CLI subcommand: this is ad-hoc analysis, so it lives in notebooks/ per review feedback.
  • Ties into the neutral ignored_comments work (Add neutral ignored_comments bucket to code-review scoring #768): FP-guard gold cases can be classified by the article whose false positive they guard.

AB#646153

Annotate code-review gold entries with the BCQuality knowledge article(s) they exercise and report per-article coverage.

- ReviewComment.article and metadata.articles (both optional, backward-compatible) carry the <domain>/<slug> association; FP-guard entries (expected_comments=[]) use the entry-level list.

- New dataset/coverage.py aggregates declared articles and, given a BCQuality checkout (--bcquality-root / BCQUALITY_ROOT), flags zero-coverage and unknown slugs; degrades to declared-only when absent.

- 'bcbench dataset coverage' renders per-domain coverage with a JSON --github-output.

- Backfill the security domain (18 entries, 10 articles) as a worked example; 671 passed / 1 skipped.
Copilot AI balanced review requested due to automatic review settings August 7, 2026 09:49

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Adds per-article BCQuality coverage tracking for code-review datasets.

Changes:

  • Extends dataset schemas with optional article annotations.
  • Adds inventory comparison and coverage reporting.
  • Adds a CLI command, tests, and security-domain annotations.

Reviewed changes

Copilot reviewed 6 out of 7 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
dataset/codereview.jsonl Backfills security article annotations.
src/bcbench/commands/dataset.py Adds the coverage CLI command.
src/bcbench/dataset/__init__.py Exports coverage APIs.
src/bcbench/dataset/codereview.py Adds comment-level article support.
src/bcbench/dataset/coverage.py Implements coverage aggregation and inventory comparison.
src/bcbench/dataset/dataset_entry.py Adds entry-level article metadata.
tests/test_codereview_coverage.py Tests coverage logic and inventory handling.

Comment thread dataset/codereview.jsonl Outdated
Comment thread dataset/codereview.jsonl Outdated
Comment thread src/bcbench/dataset/codereview.py Outdated
Comment thread src/bcbench/commands/dataset.py Outdated
Comment thread dataset/codereview.jsonl Outdated
…erage

# Conflicts:
#	src/bcbench/dataset/__init__.py
@gggdttt
Wenjie Fan (gggdttt) marked this pull request as ready for review August 11, 2026 13:46
@haoranpb

Copy link
Copy Markdown
Collaborator

Wenjie Fan (@gggdttt) Quick question, is this function for ad-hoc analysis only? Or would you need it for downstream consumption?

Comment thread src/bcbench/dataset/dataset_entry.py Outdated
Comment thread src/bcbench/dataset/codereview.py Outdated
Comment thread src/bcbench/commands/dataset.py Outdated
wenjiefan added 4 commits August 13, 2026 17:53
- Move code-review-only 'articles' off base EntryMetadata into a CodeReviewEntryMetadata subclass (haoran #1)

- Share an ArticleId type between ReviewComment.article and metadata.articles (haoran #3)

- Add a model validator keeping per-comment and entry-level article annotations disjoint so they cannot drift (haoran #2)

- Replace the ad-hoc dataset coverage CLI command with a notebook, matching the bug-fix precedent (haoran #4)
- Add cell ids so the notebook validates under nbformat 4.5 (matches dataset.ipynb)

- Assert the CodeReviewEntry validator rejects an article declared both per-comment and in metadata.articles
- Remap the hardcoded-secret findings in security-001/009 from isolatedstorage-setencrypted-for-sensitive-values (which covers Set vs SetEncrypted) to secrettext-for-credentials, which covers secrets embedded in source

- Drop inherent-permissions-minimal-grant tags from security-013: those findings are object-level permissionset InherentPermissions/Entitlements properties and specific-table grant lines, not the method-level [InherentPermissions(...)] attribute the article covers (security-006 keeps the tag; its finding is a genuine method-level attribute)

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

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

Suppressed comments (2)

notebooks/code-review-coverage.ipynb:68

  • This sums article-entry associations rather than unique gold entries. If one entry declares two articles in the same domain, it is counted twice in the “Gold entries” column. Deduplicate the covered entry_ids before taking the count.
    "    row = {\"Domain\": domain, \"Covered\": len(covered), \"Gold entries\": sum(c.count for c in covered)}\n",

src/bcbench/dataset/coverage.py:106

  • The advertised bcbench dataset coverage entry point is missing. src/bcbench/commands/dataset.py still registers only list, view, and version and never imports these helpers, so users get “No such command” and cannot use the documented --bcquality-root or --github-output options. Please register the coverage command and cover its console/JSON output paths.
def build_coverage_report(
    entries: Sequence[CodeReviewEntry],
    inventory: Iterable[str] | None = None,
) -> CoverageReport:

@haoranpb Sun Haoran (haoranpb) left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Also noticed the notebook has no commited outputs, you can probably include that as well.

Comment thread src/bcbench/dataset/codereview.py
Comment thread src/bcbench/dataset/coverage.py Outdated
Comment thread src/bcbench/dataset/coverage.py Outdated
Comment thread src/bcbench/analysis/bcquality_article_coverage.py
Wenjie Fan (gggdttt) and others added 2 commits August 18, 2026 13:19
Move BCQuality-specific coverage reporting into bcbench.analysis, strengthen article identifier typing, and commit deterministic notebook outputs.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@gggdttt

Copy link
Copy Markdown
Collaborator Author

Committed the executed notebook outputs in b0a0e91. The rendered report now shows 9 covered articles across 17 of 130 annotated entries, a 186-article BCQuality inventory with 177 zero-coverage articles, per-domain totals, covered article-to-entry mappings, and the current gap lists. Execution metadata was normalized so the committed notebook contains no timestamps, local checkout paths, credentials, or transient errors.

resolve_bcquality_root already falls back to BCQUALITY_ROOT, so the notebook no longer reads the environment variable itself.

Rename tests/test_codereview_coverage.py to match the analysis module it covers.

@haoranpb Sun Haoran (haoranpb) left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Have you read through the results in the notebook, just wondering if that coverage is by design, looks a bit surprising tbh.

9 articles covered across 17/130 annotated entries; 177 of 186 articles have zero coverage

Otherwise looks good

The previous outputs were produced from a BCQuality checkout that sat on a feature branch 44 commits behind main, so the inventory was understated at 186 articles and four domains (appsource, data-modeling, query, telemetry) were missing entirely.

Regenerated against BCQuality main: 250 articles, 241 with zero coverage. Dataset-side figures are unchanged at 9 covered articles across 17 of 130 annotated entries.
@gggdttt
Wenjie Fan (gggdttt) merged commit f3a3a63 into main Aug 19, 2026
9 checks passed
@gggdttt
Wenjie Fan (gggdttt) deleted the bench/per-article-coverage branch August 19, 2026 12:16
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.

3 participants