diff --git a/src/bcbench/collection/__init__.py b/src/bcbench/collection/__init__.py index 90a481326..0d8152905 100644 --- a/src/bcbench/collection/__init__.py +++ b/src/bcbench/collection/__init__.py @@ -1,5 +1,11 @@ """Collection module for gathering dataset entries from GitHub.""" +from bcbench.collection.collect_codereview import collect_codereview_entry from bcbench.collection.collect_gh import ScreeningResult, collect_gh_entry, screen_gh_candidate -__all__ = ["ScreeningResult", "collect_gh_entry", "screen_gh_candidate"] +__all__ = [ + "ScreeningResult", + "collect_codereview_entry", + "collect_gh_entry", + "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..efc7e6b74 --- /dev/null +++ b/src/bcbench/collection/collect_codereview.py @@ -0,0 +1,214 @@ +"""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 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. +""" + +from __future__ import annotations + +from pathlib import Path +from typing import Any + +from bcbench.collection.gh_client import GHClient +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__) + +# 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, Severity | 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 + # 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: + # 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: 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) + ) + if sev_match: + word = sev_match.group(1).lower() + word = _SEVERITY_WORD_ALIASES.get(word, word) + try: + severity = Severity.from_input(word) + 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") + if not 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 + + +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") or "").casefold() != reviewer.casefold(): + continue + if reacted: + 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 + + 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 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)) + + 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: + 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: + 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/src/bcbench/collection/gh_client.py b/src/bcbench/collection/gh_client.py index a113e141a..cf6c80963 100644 --- a/src/bcbench/collection/gh_client.py +++ b/src/bcbench/collection/gh_client.py @@ -58,6 +58,62 @@ 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", + "--slurp", + ], + capture_output=True, + text=True, + encoding="utf-8", + check=True, + ) + # --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.""" + result = subprocess.run( + [ + "gh", + "api", + f"/repos/{self.repo}/pulls/comments/{comment_id}/reactions", + "--paginate", + "--slurp", + ], + capture_output=True, + text=True, + encoding="utf-8", + check=True, + ) + 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 encoded_path = quote(file_path, safe="/") diff --git a/src/bcbench/commands/collect.py b/src/bcbench/commands/collect.py index 217d1b7d7..8a495564e 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,57 @@ 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: 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 + """ + 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}") + + @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..d8819938f --- /dev/null +++ b/tests/test_collect_codereview.py @@ -0,0 +1,164 @@ +"""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: + 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 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. + 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 { + "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" + + +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\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 + + +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"]