From 049cba89ebabf6bce701d150fe7db40b0ea2d27e Mon Sep 17 00:00:00 2001 From: wenjiefan Date: Thu, 23 Jul 2026 11:38:40 +0200 Subject: [PATCH 1/6] Add 'collect codereview' to build dataset entries from real PRs Harvest a real PR diff as the patch and its inline comments as expected_comments; --reviewer keeps one author's findings, --reacted keeps positively-reacted ones (drops thumbs-down as FP guards by omission). Relax ReviewComment.file to allow spaces in paths. Serves ADO 643360 + 643361. --- src/bcbench/collection/__init__.py | 10 +- src/bcbench/collection/collect_codereview.py | 194 +++++++++++++++++++ src/bcbench/collection/gh_client.py | 53 +++++ src/bcbench/commands/collect.py | 48 ++++- src/bcbench/dataset/codereview.py | 2 +- tests/test_collect_codereview.py | 94 +++++++++ 6 files changed, 398 insertions(+), 3 deletions(-) create mode 100644 src/bcbench/collection/collect_codereview.py create mode 100644 tests/test_collect_codereview.py diff --git a/src/bcbench/collection/__init__.py b/src/bcbench/collection/__init__.py index 90a481326..36c4ca1ee 100644 --- a/src/bcbench/collection/__init__.py +++ b/src/bcbench/collection/__init__.py @@ -1,5 +1,13 @@ """Collection module for gathering dataset entries from GitHub.""" +from bcbench.collection.collect_codereview import build_expected_comments, collect_codereview_entry, parse_domain_severity from bcbench.collection.collect_gh import ScreeningResult, collect_gh_entry, screen_gh_candidate -__all__ = ["ScreeningResult", "collect_gh_entry", "screen_gh_candidate"] +__all__ = [ + "ScreeningResult", + "build_expected_comments", + "collect_codereview_entry", + "collect_gh_entry", + "parse_domain_severity", + "screen_gh_candidate", +] diff --git a/src/bcbench/collection/collect_codereview.py b/src/bcbench/collection/collect_codereview.py new file mode 100644 index 000000000..6733640f1 --- /dev/null +++ b/src/bcbench/collection/collect_codereview.py @@ -0,0 +1,194 @@ +"""Build a code-review dataset entry from a real GitHub pull request. + +Turns a PR (its diff) plus a set of expected review comments into a CodeReviewEntry. +Two ways to decide which of the PR's inline comments are the *expected* findings: + +- ``reviewer``: keep comments authored by a given login (e.g. a human reviewer who + wrote the gold findings directly on the PR). +- ``reacted``: keep the reviewer-bot comments that received a positive reaction + (thumbs-up / heart / ...). Comments with only a thumbs-down are dropped, which + turns them into false-positive guards by omission (the construct stays in the + patch but is absent from ``expected_comments``, so flagging it costs precision). + +If neither is given, every top-level inline comment on the PR is used. +""" + +from __future__ import annotations + +from pathlib import Path +from typing import Any + +import typer + +from bcbench.collection.gh_client import GHClient +from bcbench.config import get_config +from bcbench.dataset import CodeReviewEntry, ReviewComment, Severity +from bcbench.dataset.dataset_entry import EntryMetadata +from bcbench.exceptions import CollectionError +from bcbench.logger import get_logger + +logger = get_logger(__name__) +_config = get_config() + +# GitHub reaction contents that mark a comment as a confirmed (good) finding. +POSITIVE_REACTIONS = frozenset({"+1", "heart", "hooray", "rocket"}) + +_SEVERITY_WORDS = ("critical", "high", "medium", "low", "error", "warning", "suggestion", "info", "major", "minor") +# Words the ReviewComment severity enum doesn't know, mapped to ones it does. +_SEVERITY_WORD_ALIASES = {"major": "high", "minor": "low"} + + +def parse_domain_severity(body: str) -> tuple[str | None, str | None]: + """Best-effort extraction of (domain, severity) from a review-comment body. + + Recognises both the reviewer-bot header (``... Medium Severity - Performance ...``) + and the ``**Domain - title** _(major)_`` shape humans write. Both fields are + optional on ReviewComment, so anything unparsed simply stays ``None``. + """ + import re + + domain: str | None = None + # Bot header: " Severity - " + match = re.search(r"Severity\s*[\u2014-]\s*([A-Za-z][A-Za-z /]+)", body) + if match: + domain = match.group(1).strip() + else: + # Human header: leading "** - ...**" + match = re.match(r"\s*\*\*\s*([A-Za-z][A-Za-z ]+?)\s*[\u2014-]", body) + if match: + domain = match.group(1).strip() + + severity: str | None = None + sev_match = re.search(r"\b(" + "|".join(_SEVERITY_WORDS) + r")\b", body, re.IGNORECASE) + if sev_match: + word = sev_match.group(1).lower() + word = _SEVERITY_WORD_ALIASES.get(word, word) + try: + severity = Severity.from_input(word).value + except ValueError: + severity = None + + return domain, severity + + +def _comment_line_span(comment: dict[str, Any]) -> tuple[int, int | None] | None: + """Return (line_start, line_end) for a RIGHT-side comment, or None if unplaceable.""" + if comment.get("in_reply_to_id"): + return None # thread reply, not a standalone finding + if (comment.get("side") or "RIGHT") != "RIGHT": + return None # LEFT-side comments reference base lines, not the reviewed diff + line = comment.get("line") or comment.get("original_line") + if not line: + return None # outdated / unanchored comment + start = comment.get("start_line") or comment.get("original_start_line") + if start and start != line: + return int(start), int(line) + return int(line), None + + +def build_expected_comments( + comments: list[dict[str, Any]], + *, + reviewer: str | None, + reacted: bool, + reactions_by_id: dict[int, list[dict[str, Any]]] | None = None, +) -> list[ReviewComment]: + """Select and convert PR inline comments into expected ReviewComments.""" + reactions_by_id = reactions_by_id or {} + selected: list[ReviewComment] = [] + + for comment in comments: + if reviewer is not None and (comment.get("user") or {}).get("login") != reviewer: + continue + if reacted: + contents = {r.get("content") for r in reactions_by_id.get(comment.get("id"), [])} + if not (contents & POSITIVE_REACTIONS): + continue + + span = _comment_line_span(comment) + if span is None: + logger.debug("Skipping comment %s (unplaceable / reply / left-side)", comment.get("id")) + continue + + body = (comment.get("body") or "").strip() + if not body: + continue + + domain, severity = parse_domain_severity(body) + selected.append( + ReviewComment( + file=comment["path"], + line_start=span[0], + line_end=span[1], + body=body, + domain=domain, + severity=severity, + ) + ) + + return selected + + +def collect_codereview_entry( + pr_number: int, + output: Path, + environment_setup_version: str, + repo: str = "microsoft/BCApps", + reviewer: str | None = None, + reacted: bool = False, + area: str | None = None, +) -> CodeReviewEntry: + gh_client = GHClient(repo) + + try: + logger.info("Collecting code-review entry for PR #%s from %s", pr_number, repo) + + pr_data: dict[str, Any] = gh_client.get_pr_info(pr_number) + base_ref = pr_data.get("baseRefOid", "") + head_ref = pr_data.get("headRefOid", "") + if not base_ref or not head_ref: + raise CollectionError("Unable to determine base/head commit from PR data") + + base_commit = gh_client.get_merge_base(base_ref, head_ref) + if not base_commit: + raise CollectionError("Unable to determine merge-base commit for PR") + + patch = gh_client.get_pr_diff(pr_number) + if not patch.strip(): + raise CollectionError("PR diff is empty") + + comments = gh_client.get_pr_review_comments(pr_number) + reactions_by_id: dict[int, list[dict[str, Any]]] = {} + if reacted: + for comment in comments: + cid = comment.get("id") + if cid is not None: + reactions_by_id[cid] = gh_client.get_review_comment_reactions(cid) + + expected_comments = build_expected_comments(comments, reviewer=reviewer, reacted=reacted, reactions_by_id=reactions_by_id) + logger.info("Selected %d expected comment(s) from %d PR comment(s)", len(expected_comments), len(comments)) + + entry = CodeReviewEntry( + repo=repo, + instance_id=f"{repo.replace('/', '__')}-{pr_number}", + base_commit=base_commit, + created_at=pr_data.get("createdAt", ""), + environment_setup_version=environment_setup_version, + patch=patch, + metadata=EntryMetadata(area=area), + expected_comments=expected_comments, + ) + except CollectionError: + raise + except Exception as exc: + logger.exception("Failed to collect code-review entry") + raise typer.Exit(code=1) from exc + + try: + entry.save_to_file(output) + except OSError as exc: + logger.exception("Failed to write dataset entry") + raise typer.Exit(code=1) from exc + + logger.info("Saved code-review entry %s to %s", entry.instance_id, output) + return entry diff --git a/src/bcbench/collection/gh_client.py b/src/bcbench/collection/gh_client.py index a113e141a..7637ca949 100644 --- a/src/bcbench/collection/gh_client.py +++ b/src/bcbench/collection/gh_client.py @@ -58,6 +58,59 @@ def get_pr_diff(self, pr_number: int) -> str: ) return result.stdout + def get_merge_base(self, base: str, head: str) -> str: + """Return the merge-base SHA of `base` and `head`. + + This is the commit `gh pr diff` (a three-dot diff) is computed against, so a + code-review entry that stores it as base_commit can apply the PR patch cleanly. + """ + result = subprocess.run( + [ + "gh", + "api", + f"/repos/{self.repo}/compare/{base}...{head}", + "--jq", + ".merge_base_commit.sha", + ], + capture_output=True, + text=True, + encoding="utf-8", + check=True, + ) + return result.stdout.strip() + + def get_pr_review_comments(self, pr_number: int) -> list[dict[str, Any]]: + """Return all inline review comments on a PR (paginated).""" + result = subprocess.run( + [ + "gh", + "api", + f"/repos/{self.repo}/pulls/{pr_number}/comments", + "--paginate", + ], + capture_output=True, + text=True, + encoding="utf-8", + check=True, + ) + return json.loads(result.stdout) + + def get_review_comment_reactions(self, comment_id: int) -> list[dict[str, Any]]: + """Return reactions on a single inline review comment.""" + result = subprocess.run( + [ + "gh", + "api", + f"/repos/{self.repo}/pulls/comments/{comment_id}/reactions", + "--paginate", + ], + capture_output=True, + text=True, + encoding="utf-8", + check=True, + ) + return json.loads(result.stdout) + def get_file_content(self, file_path: str, ref: str) -> str: # URL-encode the file path to handle spaces and special characters encoded_path = quote(file_path, safe="/") diff --git a/src/bcbench/commands/collect.py b/src/bcbench/commands/collect.py index 217d1b7d7..b06a6f894 100644 --- a/src/bcbench/commands/collect.py +++ b/src/bcbench/commands/collect.py @@ -5,7 +5,7 @@ import typer from typing_extensions import Annotated -from bcbench.collection import ScreeningResult, collect_gh_entry, screen_gh_candidate +from bcbench.collection import ScreeningResult, collect_codereview_entry, collect_gh_entry, screen_gh_candidate from bcbench.config import get_config from bcbench.exceptions import CollectionError @@ -38,6 +38,52 @@ def collect_gh( collect_gh_entry(pr_number=pr_number, output=output, repo=repo, environment_setup_version=environment_setup_version) +@collect_app.command("codereview") +def collect_codereview( + pr_number: Annotated[int, typer.Argument(help="Pull request number to collect")], + environment_setup_version: Annotated[ + str, + typer.Option("--environment-setup-version", help="BC environment version to record on the entry (e.g. 27.0)"), + ], + output: Annotated[Path, typer.Option(help="Path to output dataset file")] = _config.paths.dataset_dir / "codereview.jsonl", + repo: Annotated[str, typer.Option(help="GitHub repository in OWNER/REPO format")] = "microsoft/BCApps", + reviewer: Annotated[ + str | None, + typer.Option(help="Only use inline comments authored by this GitHub login as expected findings"), + ] = None, + reacted: Annotated[ + bool, + typer.Option("--reacted", help="Only use comments with a positive reaction (thumbs-up / heart / ...) as expected findings"), + ] = False, + area: Annotated[str | None, typer.Option(help="Value to record in metadata.area")] = None, +) -> None: + """ + Build a code-review dataset entry from a real GitHub pull request. + + The PR diff becomes the review patch; its inline comments become the expected + findings. Choose which comments count as expected with --reviewer or --reacted + (default: all top-level inline comments). + + Example usage: + + # A PR whose expected findings a reviewer wrote directly as inline comments + bcbench collect codereview 9553 --reviewer WaelAbuSeada --environment-setup-version 27.0 --area privacy + + # Harvest online-eval reactions: only reviewer-bot comments that got a thumbs-up + bcbench collect codereview 9315 --reacted --environment-setup-version 27.0 + """ + entry = collect_codereview_entry( + pr_number=pr_number, + output=output, + environment_setup_version=environment_setup_version, + repo=repo, + reviewer=reviewer, + reacted=reacted, + area=area, + ) + typer.echo(f"Saved {entry.instance_id} with {len(entry.expected_comments)} expected comment(s) to {output}") + + @collect_app.command("screen") def screen( pr_number: Annotated[int, typer.Argument(help="Pull request number to screen")], diff --git a/src/bcbench/dataset/codereview.py b/src/bcbench/dataset/codereview.py index 36afd81d7..df89fd0f3 100644 --- a/src/bcbench/dataset/codereview.py +++ b/src/bcbench/dataset/codereview.py @@ -47,7 +47,7 @@ def from_input(cls, value: str) -> Severity: class ReviewComment(BaseModel): model_config = ConfigDict(frozen=True) - file: Annotated[str, Field(pattern=r"^[A-Za-z0-9][A-Za-z0-9./_-]*\.(al|json)$")] + file: Annotated[str, Field(pattern=r"^[A-Za-z0-9][A-Za-z0-9 ./_-]*\.(al|json)$")] line_start: Annotated[int, Field(ge=1)] line_end: Annotated[int, Field(ge=1)] | None = None domain: str | None = None diff --git a/tests/test_collect_codereview.py b/tests/test_collect_codereview.py new file mode 100644 index 000000000..6147c362a --- /dev/null +++ b/tests/test_collect_codereview.py @@ -0,0 +1,94 @@ +"""Tests for building code-review dataset entries from GitHub PRs.""" + +from bcbench.collection.collect_codereview import ( + build_expected_comments, + parse_domain_severity, +) + + +class TestParseDomainSeverity: + def test_parses_reviewer_bot_header(self): + body = "$\\textbf{Medium Severity \u2014 Performance}$ SetAutoCalcFields would help here." + domain, severity = parse_domain_severity(body) + assert domain == "Performance" + assert severity == "medium" + + def test_parses_human_header_with_major(self): + body = "**Privacy \u2014 field-level DataClassification** _(major)_ add DataClassification." + domain, severity = parse_domain_severity(body) + assert domain == "Privacy" + assert severity == "high" # major -> high + + def test_minor_maps_to_low(self): + _, severity = parse_domain_severity("**Style \u2014 Locked labels** _(minor)_") + assert severity == "low" + + def test_unparseable_returns_none(self): + domain, severity = parse_domain_severity("please take another look at this") + assert domain is None + assert severity is None + + +def _comment(cid, path, *, login="alice", line=10, start_line=None, body="finding", side="RIGHT", reply=None): + return { + "id": cid, + "path": path, + "user": {"login": login}, + "line": line, + "start_line": start_line, + "body": body, + "side": side, + "in_reply_to_id": reply, + } + + +class TestBuildExpectedComments: + def test_reviewer_filter_keeps_only_that_author(self): + comments = [ + _comment(1, "src/A.al", login="wael"), + _comment(2, "src/B.al", login="bot"), + ] + result = build_expected_comments(comments, reviewer="wael", reacted=False) + assert [c.file for c in result] == ["src/A.al"] + + def test_no_filter_keeps_all_placeable(self): + comments = [_comment(1, "src/A.al"), _comment(2, "src/B.al")] + result = build_expected_comments(comments, reviewer=None, reacted=False) + assert len(result) == 2 + + def test_reacted_keeps_positive_drops_thumbs_down(self): + comments = [_comment(1, "src/Good.al"), _comment(2, "src/Bad.al")] + reactions = {1: [{"content": "+1"}], 2: [{"content": "-1"}]} + result = build_expected_comments(comments, reviewer=None, reacted=True, reactions_by_id=reactions) + assert [c.file for c in result] == ["src/Good.al"] + + def test_reviewer_and_reacted_compose(self): + comments = [ + _comment(1, "src/A.al", login="bot"), + _comment(2, "src/B.al", login="bot"), + _comment(3, "src/C.al", login="human"), + ] + reactions = {1: [{"content": "+1"}], 2: [], 3: [{"content": "+1"}]} + result = build_expected_comments(comments, reviewer="bot", reacted=True, reactions_by_id=reactions) + assert [c.file for c in result] == ["src/A.al"] + + def test_skips_replies_and_left_side_and_unanchored(self): + comments = [ + _comment(1, "src/Reply.al", reply=99), + _comment(2, "src/Left.al", side="LEFT"), + _comment(3, "src/NoLine.al", line=None), + _comment(4, "src/Ok.al"), + ] + result = build_expected_comments(comments, reviewer=None, reacted=False) + assert [c.file for c in result] == ["src/Ok.al"] + + def test_multiline_span_sets_start_and_end(self): + comments = [_comment(1, "src/A.al", line=20, start_line=15)] + result = build_expected_comments(comments, reviewer=None, reacted=False) + assert result[0].line_start == 15 + assert result[0].line_end == 20 + + def test_path_with_spaces_is_accepted(self): + comments = [_comment(1, "src/W1/1.Setup Data/Foo.Codeunit.al")] + result = build_expected_comments(comments, reviewer=None, reacted=False) + assert result[0].file == "src/W1/1.Setup Data/Foo.Codeunit.al" From 6d07259fb578604f5c33fd144cd6053cf778b3b6 Mon Sep 17 00:00:00 2001 From: wenjiefan Date: Thu, 23 Jul 2026 14:28:05 +0200 Subject: [PATCH 2/6] Read domain from agent_domain marker (LaTeX-escaped headers unparseable) Real reviewer-bot comments escape the severity/domain header (Severity\ -\ Performance), so the header regex missed domain. Prefer the marker the bot always appends; fall back to header parsing for human comments. --- src/bcbench/collection/collect_codereview.py | 19 +++++++++++++------ tests/test_collect_codereview.py | 12 ++++++++++++ 2 files changed, 25 insertions(+), 6 deletions(-) diff --git a/src/bcbench/collection/collect_codereview.py b/src/bcbench/collection/collect_codereview.py index 6733640f1..9c722a6af 100644 --- a/src/bcbench/collection/collect_codereview.py +++ b/src/bcbench/collection/collect_codereview.py @@ -48,15 +48,22 @@ def parse_domain_severity(body: str) -> tuple[str | None, str | None]: import re domain: str | None = None - # Bot header: " Severity - " - match = re.search(r"Severity\s*[\u2014-]\s*([A-Za-z][A-Za-z /]+)", body) - if match: - domain = match.group(1).strip() + # Preferred: the explicit metadata marker the review bot appends. It survives + # LaTeX-escaped headers (e.g. "Severity\ \u2014\ Performance") that the header + # regex below cannot read. + marker = re.search(r"", body) + if marker: + domain = marker.group(1).strip() else: - # Human header: leading "** - ...**" - match = re.match(r"\s*\*\*\s*([A-Za-z][A-Za-z ]+?)\s*[\u2014-]", body) + # Bot header: " Severity - " + match = re.search(r"Severity\s*[\u2014-]\s*([A-Za-z][A-Za-z /]+)", body) if match: domain = match.group(1).strip() + else: + # Human header: leading "** - ...**" + match = re.match(r"\s*\*\*\s*([A-Za-z][A-Za-z ]+?)\s*[\u2014-]", body) + if match: + domain = match.group(1).strip() severity: str | None = None sev_match = re.search(r"\b(" + "|".join(_SEVERITY_WORDS) + r")\b", body, re.IGNORECASE) diff --git a/tests/test_collect_codereview.py b/tests/test_collect_codereview.py index 6147c362a..ae1ef0077 100644 --- a/tests/test_collect_codereview.py +++ b/tests/test_collect_codereview.py @@ -28,6 +28,18 @@ def test_unparseable_returns_none(self): assert domain is None assert severity is None + def test_agent_domain_marker_wins_over_latex_escaped_header(self): + # Real bot comments escape the header (Severity\ \u2014\ Performance), which the + # header regex cannot read, but always append an agent_domain marker. + body = ( + "$\\textbf{\U0001f7e1\\ Medium\\ Severity\\ \u2014\\ Performance}$\n" + "### CheckJobTaskIsPosting calls JobTask.Get without SetLoadFields.\n" + "" + ) + domain, severity = parse_domain_severity(body) + assert domain == "performance" + assert severity == "medium" + def _comment(cid, path, *, login="alice", line=10, start_line=None, body="finding", side="RIGHT", reply=None): return { From 447600c79e1d64bb61387671d20a4af81e9db557 Mon Sep 17 00:00:00 2001 From: wenjiefan Date: Thu, 23 Jul 2026 15:58:11 +0200 Subject: [PATCH 3/6] Address PR review: fix pagination, drop outdated-comment fallback, case-insensitive reviewer, remove unused global - gh_client: add --slurp so multi-page comment/reaction results parse (json.loads previously failed on concatenated page arrays). - _comment_line_span: use only current line/start_line; drop original_line fallback that re-admitted outdated comments at stale coordinates. - build_expected_comments: case-insensitive reviewer login match. - Remove unused module-level _config (CodeQL); clarify --reacted is any-author (combine with --reviewer for bot-only). Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 74500e3e-0905-4f26-a2f4-3e6208c4fec2 --- src/bcbench/collection/collect_codereview.py | 19 +++++++++---------- src/bcbench/collection/gh_client.py | 7 +++++-- src/bcbench/commands/collect.py | 3 ++- 3 files changed, 16 insertions(+), 13 deletions(-) diff --git a/src/bcbench/collection/collect_codereview.py b/src/bcbench/collection/collect_codereview.py index 9c722a6af..48c120e2f 100644 --- a/src/bcbench/collection/collect_codereview.py +++ b/src/bcbench/collection/collect_codereview.py @@ -5,10 +5,11 @@ - ``reviewer``: keep comments authored by a given login (e.g. a human reviewer who wrote the gold findings directly on the PR). -- ``reacted``: keep the reviewer-bot comments that received a positive reaction - (thumbs-up / heart / ...). Comments with only a thumbs-down are dropped, which - turns them into false-positive guards by omission (the construct stays in the - patch but is absent from ``expected_comments``, so flagging it costs precision). +- ``reacted``: keep comments that received a positive reaction (thumbs-up / heart + / ...) from any author; combine with ``reviewer`` to restrict to the review bot. + Comments with only a thumbs-down are dropped, which turns them into false-positive + guards by omission (the construct stays in the patch but is absent from + ``expected_comments``, so flagging it costs precision). If neither is given, every top-level inline comment on the PR is used. """ @@ -21,14 +22,12 @@ import typer from bcbench.collection.gh_client import GHClient -from bcbench.config import get_config from bcbench.dataset import CodeReviewEntry, ReviewComment, Severity from bcbench.dataset.dataset_entry import EntryMetadata from bcbench.exceptions import CollectionError from bcbench.logger import get_logger logger = get_logger(__name__) -_config = get_config() # GitHub reaction contents that mark a comment as a confirmed (good) finding. POSITIVE_REACTIONS = frozenset({"+1", "heart", "hooray", "rocket"}) @@ -84,10 +83,10 @@ def _comment_line_span(comment: dict[str, Any]) -> tuple[int, int | None] | None return None # thread reply, not a standalone finding if (comment.get("side") or "RIGHT") != "RIGHT": return None # LEFT-side comments reference base lines, not the reviewed diff - line = comment.get("line") or comment.get("original_line") + line = comment.get("line") if not line: - return None # outdated / unanchored comment - start = comment.get("start_line") or comment.get("original_start_line") + return None # outdated / unanchored: current line no longer exists in the patch + start = comment.get("start_line") if start and start != line: return int(start), int(line) return int(line), None @@ -105,7 +104,7 @@ def build_expected_comments( selected: list[ReviewComment] = [] for comment in comments: - if reviewer is not None and (comment.get("user") or {}).get("login") != reviewer: + if reviewer is not None and ((comment.get("user") or {}).get("login") or "").casefold() != reviewer.casefold(): continue if reacted: contents = {r.get("content") for r in reactions_by_id.get(comment.get("id"), [])} diff --git a/src/bcbench/collection/gh_client.py b/src/bcbench/collection/gh_client.py index 7637ca949..cf6c80963 100644 --- a/src/bcbench/collection/gh_client.py +++ b/src/bcbench/collection/gh_client.py @@ -87,13 +87,15 @@ def get_pr_review_comments(self, pr_number: int) -> list[dict[str, Any]]: "api", f"/repos/{self.repo}/pulls/{pr_number}/comments", "--paginate", + "--slurp", ], capture_output=True, text=True, encoding="utf-8", check=True, ) - return json.loads(result.stdout) + # --paginate --slurp wraps each page (itself an array) into an outer array. + return [item for page in json.loads(result.stdout) for item in page] def get_review_comment_reactions(self, comment_id: int) -> list[dict[str, Any]]: """Return reactions on a single inline review comment.""" @@ -103,13 +105,14 @@ def get_review_comment_reactions(self, comment_id: int) -> list[dict[str, Any]]: "api", f"/repos/{self.repo}/pulls/comments/{comment_id}/reactions", "--paginate", + "--slurp", ], capture_output=True, text=True, encoding="utf-8", check=True, ) - return json.loads(result.stdout) + return [item for page in json.loads(result.stdout) for item in page] def get_file_content(self, file_path: str, ref: str) -> str: # URL-encode the file path to handle spaces and special characters diff --git a/src/bcbench/commands/collect.py b/src/bcbench/commands/collect.py index b06a6f894..fef9802d6 100644 --- a/src/bcbench/commands/collect.py +++ b/src/bcbench/commands/collect.py @@ -69,7 +69,8 @@ def collect_codereview( # A PR whose expected findings a reviewer wrote directly as inline comments bcbench collect codereview 9553 --reviewer WaelAbuSeada --environment-setup-version 27.0 --area privacy - # Harvest online-eval reactions: only reviewer-bot comments that got a thumbs-up + # Harvest online-eval reactions: any comment that got a thumbs-up (add + # --reviewer to restrict to the review bot) bcbench collect codereview 9315 --reacted --environment-setup-version 27.0 """ entry = collect_codereview_entry( From b58368890ead1a1f617114e319547eccb47dc77c Mon Sep 17 00:00:00 2001 From: wenjiefan Date: Thu, 23 Jul 2026 16:12:25 +0200 Subject: [PATCH 4/6] Only re-export the collect entry-point from collection package build_expected_comments and parse_domain_severity are internal helpers (unit-tested directly via the submodule, no package-level consumer). Match the collect_gh convention of exporting only public entry-points. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 74500e3e-0905-4f26-a2f4-3e6208c4fec2 --- src/bcbench/collection/__init__.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/bcbench/collection/__init__.py b/src/bcbench/collection/__init__.py index 36c4ca1ee..0d8152905 100644 --- a/src/bcbench/collection/__init__.py +++ b/src/bcbench/collection/__init__.py @@ -1,13 +1,11 @@ """Collection module for gathering dataset entries from GitHub.""" -from bcbench.collection.collect_codereview import build_expected_comments, collect_codereview_entry, parse_domain_severity +from bcbench.collection.collect_codereview import collect_codereview_entry from bcbench.collection.collect_gh import ScreeningResult, collect_gh_entry, screen_gh_candidate __all__ = [ "ScreeningResult", - "build_expected_comments", "collect_codereview_entry", "collect_gh_entry", - "parse_domain_severity", "screen_gh_candidate", ] From 2d309d437347e389fe4595c239b66146ccae5c59 Mon Sep 17 00:00:00 2001 From: wenjiefan Date: Fri, 24 Jul 2026 12:28:41 +0200 Subject: [PATCH 5/6] Address review feedback in codereview collector - parse_domain_severity: prefer an explicit (minor)/(major) tag or a ' Severity' header over the first severity-like word in prose, so a word inside a title (e.g. 'high' in 'high-impact') no longer overrides the annotated severity. - collect_codereview_entry: in --reacted mode, pre-filter comments (reviewer, placeable line span, non-empty body) before calling the reactions API, so review-heavy PRs no longer spend a request per comment and risk rate limits. - collect codereview command: catch CollectionError and exit(1) with a clean message instead of a traceback, matching the screen command. - tests: cover collect_codereview_entry orchestration with a mocked GHClient (call sequence + reload) and the explicit-tag severity case. --- src/bcbench/collection/collect_codereview.py | 24 ++++++- src/bcbench/commands/collect.py | 22 ++++--- tests/test_collect_codereview.py | 66 ++++++++++++++++++++ 3 files changed, 100 insertions(+), 12 deletions(-) diff --git a/src/bcbench/collection/collect_codereview.py b/src/bcbench/collection/collect_codereview.py index 48c120e2f..464d4f9a4 100644 --- a/src/bcbench/collection/collect_codereview.py +++ b/src/bcbench/collection/collect_codereview.py @@ -65,7 +65,15 @@ def parse_domain_severity(body: str) -> tuple[str | None, str | None]: domain = match.group(1).strip() severity: str | None = None - sev_match = re.search(r"\b(" + "|".join(_SEVERITY_WORDS) + r")\b", body, re.IGNORECASE) + words = "|".join(_SEVERITY_WORDS) + # Prefer an explicit annotation over a bare severity-like word that may appear in + # prose (e.g. "high" inside "high-impact" in a title). Order: an explicit "(minor)" + # tag, then the bot " Severity" header, then a broad first-word fallback. + sev_match = ( + re.search(r"\(\s*(" + words + r")\s*\)", body, re.IGNORECASE) + or re.search(r"\b(" + words + r")\s+severity\b", body, re.IGNORECASE) + or re.search(r"\b(" + words + r")\b", body, re.IGNORECASE) + ) if sev_match: word = sev_match.group(1).lower() word = _SEVERITY_WORD_ALIASES.get(word, word) @@ -168,8 +176,18 @@ def collect_codereview_entry( if reacted: for comment in comments: cid = comment.get("id") - if cid is not None: - reactions_by_id[cid] = gh_client.get_review_comment_reactions(cid) + if cid is None: + continue + # Only spend a reactions API call on comments that can actually become a + # finding: skip other reviewers, replies, LEFT-side/unplaceable and empty + # bodies (build_expected_comments applies the same filters again). + if reviewer is not None and ((comment.get("user") or {}).get("login") or "").casefold() != reviewer.casefold(): + continue + if _comment_line_span(comment) is None: + continue + if not (comment.get("body") or "").strip(): + continue + reactions_by_id[cid] = gh_client.get_review_comment_reactions(cid) expected_comments = build_expected_comments(comments, reviewer=reviewer, reacted=reacted, reactions_by_id=reactions_by_id) logger.info("Selected %d expected comment(s) from %d PR comment(s)", len(expected_comments), len(comments)) diff --git a/src/bcbench/commands/collect.py b/src/bcbench/commands/collect.py index fef9802d6..8a495564e 100644 --- a/src/bcbench/commands/collect.py +++ b/src/bcbench/commands/collect.py @@ -73,15 +73,19 @@ def collect_codereview( # --reviewer to restrict to the review bot) bcbench collect codereview 9315 --reacted --environment-setup-version 27.0 """ - entry = collect_codereview_entry( - pr_number=pr_number, - output=output, - environment_setup_version=environment_setup_version, - repo=repo, - reviewer=reviewer, - reacted=reacted, - area=area, - ) + try: + entry = collect_codereview_entry( + pr_number=pr_number, + output=output, + environment_setup_version=environment_setup_version, + repo=repo, + reviewer=reviewer, + reacted=reacted, + area=area, + ) + except CollectionError as exc: + typer.echo(f"Error: {exc}", err=True) + raise typer.Exit(code=1) from exc typer.echo(f"Saved {entry.instance_id} with {len(entry.expected_comments)} expected comment(s) to {output}") diff --git a/tests/test_collect_codereview.py b/tests/test_collect_codereview.py index ae1ef0077..64ce558d8 100644 --- a/tests/test_collect_codereview.py +++ b/tests/test_collect_codereview.py @@ -1,9 +1,13 @@ """Tests for building code-review dataset entries from GitHub PRs.""" +from unittest.mock import MagicMock, patch + from bcbench.collection.collect_codereview import ( build_expected_comments, + collect_codereview_entry, parse_domain_severity, ) +from bcbench.dataset.codereview import CodeReviewEntry class TestParseDomainSeverity: @@ -28,6 +32,11 @@ def test_unparseable_returns_none(self): assert domain is None assert severity is None + def test_explicit_severity_tag_wins_over_word_in_prose(self): + # "high" appears only inside "high-impact"; the explicit tag is (minor). + _, severity = parse_domain_severity("high-impact change _(minor)_") + assert severity == "low" + def test_agent_domain_marker_wins_over_latex_escaped_header(self): # Real bot comments escape the header (Severity\ \u2014\ Performance), which the # header regex cannot read, but always append an agent_domain marker. @@ -104,3 +113,60 @@ def test_path_with_spaces_is_accepted(self): comments = [_comment(1, "src/W1/1.Setup Data/Foo.Codeunit.al")] result = build_expected_comments(comments, reviewer=None, reacted=False) assert result[0].file == "src/W1/1.Setup Data/Foo.Codeunit.al" + + +def _gh_double(comments, *, merge_base="a" * 40): + gh = MagicMock() + gh.get_pr_info.return_value = { + "baseRefOid": "b" * 40, + "headRefOid": "c" * 40, + "createdAt": "2026-01-01T00:00:00Z", + } + gh.get_merge_base.return_value = merge_base + gh.get_pr_diff.return_value = ( + "diff --git a/src/Foo.al b/src/Foo.al\n" + "new file mode 100644\n--- /dev/null\n+++ b/src/Foo.al\n" + "@@ -0,0 +1 @@\n+codeunit 50000 Foo { }\n" + ) + gh.get_pr_review_comments.return_value = comments + gh.get_review_comment_reactions.side_effect = lambda cid: [{"content": "+1"}] if cid == 1 else [] + return gh + + +class TestCollectCodereviewEntry: + def test_orchestration_writes_reloadable_entry(self, tmp_path): + out = tmp_path / "codereview.jsonl" + gh = _gh_double([_comment(1, "src/Foo.al", login="bot")]) + with patch("bcbench.collection.collect_codereview.GHClient", return_value=gh): + entry = collect_codereview_entry( + pr_number=9999, + output=out, + environment_setup_version="27.0", + repo="microsoft/BCApps", + reviewer="bot", + ) + gh.get_merge_base.assert_called_once_with("b" * 40, "c" * 40) + assert entry.base_commit == "a" * 40 + reloaded = CodeReviewEntry.load(out) + assert len(reloaded) == 1 + assert reloaded[0].instance_id == "microsoft__BCApps-9999" + assert [c.file for c in reloaded[0].expected_comments] == ["src/Foo.al"] + + def test_reacted_fetches_reactions_only_for_candidates(self, tmp_path): + out = tmp_path / "codereview.jsonl" + comments = [ + _comment(1, "src/Ok.al"), # candidate -> reaction fetched + _comment(2, "src/Reply.al", reply=99), # reply -> skipped, no API call + _comment(3, "src/Left.al", side="LEFT"), # left side -> skipped, no API call + ] + gh = _gh_double(comments) + with patch("bcbench.collection.collect_codereview.GHClient", return_value=gh): + entry = collect_codereview_entry( + pr_number=1, + output=out, + environment_setup_version="27.0", + reacted=True, + ) + fetched = [call.args[0] for call in gh.get_review_comment_reactions.call_args_list] + assert fetched == [1] # only the placeable candidate, not the reply / left-side + assert [c.file for c in entry.expected_comments] == ["src/Ok.al"] From 0722f9c95056dcbbd95fc758d88da4c84b462011 Mon Sep 17 00:00:00 2001 From: wenjiefan Date: Fri, 24 Jul 2026 13:15:06 +0200 Subject: [PATCH 6/6] Fix collector type errors and error-handling layering - Return Severity (not str) from parse_domain_severity and annotate the local, resolving the ty mismatch against ReviewComment.severity. - Guard the reactions lookup against a None comment id. - Raise CollectionError instead of typer.Exit from collect_codereview_entry so the CLI command formats the error consistently; drop the now-unused typer import. --- src/bcbench/collection/collect_codereview.py | 22 ++++++++------------ tests/test_collect_codereview.py | 12 ++--------- 2 files changed, 11 insertions(+), 23 deletions(-) diff --git a/src/bcbench/collection/collect_codereview.py b/src/bcbench/collection/collect_codereview.py index 464d4f9a4..efc7e6b74 100644 --- a/src/bcbench/collection/collect_codereview.py +++ b/src/bcbench/collection/collect_codereview.py @@ -19,8 +19,6 @@ from pathlib import Path from typing import Any -import typer - from bcbench.collection.gh_client import GHClient from bcbench.dataset import CodeReviewEntry, ReviewComment, Severity from bcbench.dataset.dataset_entry import EntryMetadata @@ -37,7 +35,7 @@ _SEVERITY_WORD_ALIASES = {"major": "high", "minor": "low"} -def parse_domain_severity(body: str) -> tuple[str | None, str | None]: +def parse_domain_severity(body: str) -> tuple[str | None, Severity | None]: """Best-effort extraction of (domain, severity) from a review-comment body. Recognises both the reviewer-bot header (``... Medium Severity - Performance ...``) @@ -64,21 +62,19 @@ def parse_domain_severity(body: str) -> tuple[str | None, str | None]: if match: domain = match.group(1).strip() - severity: str | None = None + severity: Severity | None = None words = "|".join(_SEVERITY_WORDS) # Prefer an explicit annotation over a bare severity-like word that may appear in # prose (e.g. "high" inside "high-impact" in a title). Order: an explicit "(minor)" # tag, then the bot " Severity" header, then a broad first-word fallback. sev_match = ( - re.search(r"\(\s*(" + words + r")\s*\)", body, re.IGNORECASE) - or re.search(r"\b(" + words + r")\s+severity\b", body, re.IGNORECASE) - or re.search(r"\b(" + words + r")\b", body, re.IGNORECASE) + re.search(r"\(\s*(" + words + r")\s*\)", body, re.IGNORECASE) or re.search(r"\b(" + words + r")\s+severity\b", body, re.IGNORECASE) or re.search(r"\b(" + words + r")\b", body, re.IGNORECASE) ) if sev_match: word = sev_match.group(1).lower() word = _SEVERITY_WORD_ALIASES.get(word, word) try: - severity = Severity.from_input(word).value + severity = Severity.from_input(word) except ValueError: severity = None @@ -115,7 +111,9 @@ def build_expected_comments( if reviewer is not None and ((comment.get("user") or {}).get("login") or "").casefold() != reviewer.casefold(): continue if reacted: - contents = {r.get("content") for r in reactions_by_id.get(comment.get("id"), [])} + cid = comment.get("id") + reactions = reactions_by_id.get(cid, []) if cid is not None else [] + contents = {r.get("content") for r in reactions} if not (contents & POSITIVE_REACTIONS): continue @@ -205,14 +203,12 @@ def collect_codereview_entry( except CollectionError: raise except Exception as exc: - logger.exception("Failed to collect code-review entry") - raise typer.Exit(code=1) from exc + raise CollectionError(f"Failed to collect code-review entry for PR #{pr_number}: {exc}") from exc try: entry.save_to_file(output) except OSError as exc: - logger.exception("Failed to write dataset entry") - raise typer.Exit(code=1) from exc + raise CollectionError(f"Failed to write dataset entry to {output}: {exc}") from exc logger.info("Saved code-review entry %s to %s", entry.instance_id, output) return entry diff --git a/tests/test_collect_codereview.py b/tests/test_collect_codereview.py index 64ce558d8..d8819938f 100644 --- a/tests/test_collect_codereview.py +++ b/tests/test_collect_codereview.py @@ -40,11 +40,7 @@ def test_explicit_severity_tag_wins_over_word_in_prose(self): def test_agent_domain_marker_wins_over_latex_escaped_header(self): # Real bot comments escape the header (Severity\ \u2014\ Performance), which the # header regex cannot read, but always append an agent_domain marker. - body = ( - "$\\textbf{\U0001f7e1\\ Medium\\ Severity\\ \u2014\\ Performance}$\n" - "### CheckJobTaskIsPosting calls JobTask.Get without SetLoadFields.\n" - "" - ) + body = "$\\textbf{\U0001f7e1\\ Medium\\ Severity\\ \u2014\\ Performance}$\n### CheckJobTaskIsPosting calls JobTask.Get without SetLoadFields.\n" domain, severity = parse_domain_severity(body) assert domain == "performance" assert severity == "medium" @@ -123,11 +119,7 @@ def _gh_double(comments, *, merge_base="a" * 40): "createdAt": "2026-01-01T00:00:00Z", } gh.get_merge_base.return_value = merge_base - gh.get_pr_diff.return_value = ( - "diff --git a/src/Foo.al b/src/Foo.al\n" - "new file mode 100644\n--- /dev/null\n+++ b/src/Foo.al\n" - "@@ -0,0 +1 @@\n+codeunit 50000 Foo { }\n" - ) + gh.get_pr_diff.return_value = "diff --git a/src/Foo.al b/src/Foo.al\nnew file mode 100644\n--- /dev/null\n+++ b/src/Foo.al\n@@ -0,0 +1 @@\n+codeunit 50000 Foo { }\n" gh.get_pr_review_comments.return_value = comments gh.get_review_comment_reactions.side_effect = lambda cid: [{"content": "+1"}] if cid == 1 else [] return gh