diff --git a/.github/workflows/copilot-evaluation.yml b/.github/workflows/copilot-evaluation.yml index 7d9065537..43f693bd4 100644 --- a/.github/workflows/copilot-evaluation.yml +++ b/.github/workflows/copilot-evaluation.yml @@ -139,7 +139,36 @@ jobs: - name: Install evaluation CLIs uses: $/.github/actions/install-eval-clis + # code-review is evaluated through the BC-ALAgents review engine (the real PROD + # generate half + BCQuality). Check it out only for that category. To evaluate the + # engine at a different ref, change `ref` below on your private branch. + - name: Checkout BC-ALAgents review engine + if: ${{ inputs.category == 'code-review' }} + uses: actions/checkout@v5 + with: + repository: microsoft/BC-ALAgents + ref: main + path: bc-alagents-engine + token: ${{ github.token }} + + - name: Run code-review engine for entry ${{ matrix.entry }} + if: ${{ inputs.category == 'code-review' }} + timeout-minutes: 120 + shell: pwsh + env: + COPILOT_GITHUB_TOKEN: ${{ github.token }} + GH_TOKEN: ${{ github.token }} + BC_PR_REVIEW_ROOT: ${{ github.workspace }}/bc-alagents-engine + run: | + Write-Output "::add-mask::$env:COPILOT_GITHUB_TOKEN" + + uv run bcbench evaluate code-review "${{ matrix.entry }}" ` + --model "${{ inputs.model }}" ` + --repo-path "${{ steps.setup-env.outputs.repo_path }}" ` + --output-dir "${{ env.EVALUATION_RESULTS_DIR }}" + - name: Run GitHub Copilot CLI for entry ${{ matrix.entry }} + if: ${{ inputs.category != 'code-review' }} timeout-minutes: 120 shell: pwsh env: @@ -173,7 +202,7 @@ jobs: with: results-dir: ${{ needs.evaluate-with-copilot-cli.outputs.results-dir }} model: ${{ inputs.model }} - agent: "GitHub Copilot CLI" + agent: ${{ inputs.category == 'code-review' && 'BC PR Review' || 'GitHub Copilot CLI' }} mock: ${{ inputs.test-run }} category: ${{ inputs.category }} git-ref: ${{ inputs.git-ref || github.ref_name }} diff --git a/src/bcbench/agent/__init__.py b/src/bcbench/agent/__init__.py index ab30132d1..984cb7115 100644 --- a/src/bcbench/agent/__init__.py +++ b/src/bcbench/agent/__init__.py @@ -4,4 +4,8 @@ from bcbench.agent.claude import run_claude_code from bcbench.agent.copilot import run_copilot_agent +# The AI harnesses are the top-level backends. The code-review category is NOT a fourth +# harness: it runs the Copilot-powered BC-ALAgents review engine, whose backend lives under +# the copilot package (bcbench.agent.copilot.pr_review.run_pr_review_agent) and is reached +# only through the dedicated `code-review` command, not by picking a harness here. __all__ = ["BCalBackendConfig", "run_bcal_agent", "run_claude_code", "run_copilot_agent"] diff --git a/src/bcbench/agent/copilot/pr_review/__init__.py b/src/bcbench/agent/copilot/pr_review/__init__.py new file mode 100644 index 000000000..c5871df5e --- /dev/null +++ b/src/bcbench/agent/copilot/pr_review/__init__.py @@ -0,0 +1,3 @@ +from bcbench.agent.copilot.pr_review.agent import run_pr_review_agent + +__all__ = ["run_pr_review_agent"] diff --git a/src/bcbench/agent/copilot/pr_review/agent.py b/src/bcbench/agent/copilot/pr_review/agent.py new file mode 100644 index 000000000..db1ea696c --- /dev/null +++ b/src/bcbench/agent/copilot/pr_review/agent.py @@ -0,0 +1,269 @@ +"""Run the BC-ALAgents review engine (generate half) as a BC-Bench agent. + +The code-review category runs the engine's own generate shell +(``Invoke-PRReviewShell.ps1 -GenerateOnly``) in local mode against the entry's changes, +so BC-Bench measures the real PROD engine + BCQuality rather than a divergent +re-implementation. The BC-Bench ``--model`` threads straight through to the single +Copilot the engine spawns (``COPILOT_MODEL``). + +The engine writes ``agent-output.txt`` (the harvested findings report); we map it to +``review.json`` in the repo root so the existing code-review scorer runs unchanged. +""" + +import json +import os +import shutil +import subprocess +import time +from pathlib import Path +from typing import Any + +import yaml + +from bcbench.agent.copilot.pr_review.review_output import engine_report_to_review_comments, load_engine_report +from bcbench.config import get_config +from bcbench.dataset import BaseDatasetEntry +from bcbench.dataset.codereview import CodeReviewEntry +from bcbench.exceptions import AgentError, AgentTimeoutError +from bcbench.logger import get_logger +from bcbench.types import AgentMetrics, EvaluationCategory, ExperimentConfiguration + +logger = get_logger(__name__) +_config = get_config() + +_AGENT_OUTPUT_FILE = "agent-output.txt" +_REVIEW_OUTPUT_FILE = "review.json" +_PREPARE_BCQUALITY_SCRIPT = Path(__file__).parent / "scripts" / "Prepare-BCQualityRoot.ps1" + + +def _load_pr_review_settings() -> dict[str, Any]: + config_file = _config.paths.agent_share_dir / "config.yaml" + data = yaml.safe_load(config_file.read_text()) or {} + return data.get("pr_review") or {} + + +def _resolve_pr_review_root(settings: dict[str, Any]) -> Path: + raw = os.environ.get("BC_PR_REVIEW_ROOT") or settings.get("path") + if not raw: + raise AgentError("Engine root not configured. Set 'pr_review.path' in the shared agent config.yaml or the BC_PR_REVIEW_ROOT environment variable.") + root = Path(raw).expanduser() + shell = root / "agents" / "ALReviewAgent" / "scripts" / "Invoke-PRReviewShell.ps1" + if not shell.exists(): + raise AgentError(f"Engine review shell not found at {shell}. Check 'pr_review.path' points at a BC-ALAgents checkout.") + return root + + +def _resolve_engine_revision(engine_root: Path) -> str: + """Resolve the engine checkout's git revision (with a dirty marker) for provenance.""" + head = subprocess.run(["git", "-C", str(engine_root), "rev-parse", "HEAD"], capture_output=True, text=True, check=False) + if head.returncode != 0 or not head.stdout.strip(): + return "unknown" + sha = head.stdout.strip() + dirty = subprocess.run(["git", "-C", str(engine_root), "status", "--porcelain"], capture_output=True, text=True, check=False) + if dirty.returncode == 0 and dirty.stdout.strip(): + return f"{sha}-dirty" + return sha + + +def _resolve_pwsh() -> str: + pwsh = shutil.which("pwsh") + if not pwsh: + raise AgentError("PowerShell (pwsh) not found in PATH. The BC-ALAgents engine requires PowerShell 7+.") + return pwsh + + +def _resolve_gh_token() -> str: + token = os.environ.get("GH_TOKEN") or os.environ.get("GITHUB_TOKEN") + if token: + return token + gh = shutil.which("gh") + if gh: + result = subprocess.run([gh, "auth", "token"], capture_output=True, text=True, check=False) + if result.returncode == 0 and result.stdout.strip(): + return result.stdout.strip() + raise AgentError("No GitHub token available for Copilot CLI auth. Set GH_TOKEN or run `gh auth login`.") + + +def _git(args: list[str], cwd: Path) -> subprocess.CompletedProcess[str]: + return subprocess.run(["git", *args], cwd=str(cwd), capture_output=True, text=True, check=True) + + +def _commit_patch_as_head(repo_path: Path) -> None: + """Commit the applied working-tree patch so the engine can diff base..HEAD. + + The code-review pipeline applies the entry patch as uncommitted changes (and + marks new files intent-to-add). The engine's local mode diffs a committed + ``BASE_REF...HEAD`` range, so materialize the changes as a head commit on top + of the base commit (which is the current HEAD). + """ + _git(["add", "-A"], repo_path) + status = _git(["status", "--porcelain"], repo_path) + if not status.stdout.strip(): + raise AgentError("No changes to review: the entry patch produced an empty working tree diff.") + _git( + ["-c", "user.name=bcbench", "-c", "user.email=bcbench@local", "commit", "-q", "--no-verify", "-m", "bcbench review head"], + repo_path, + ) + + +def _init_trusted_workspace(path: Path) -> Path: + path.mkdir(parents=True, exist_ok=True) + _git(["init", "-q"], path) + _git(["-c", "user.name=bcbench", "-c", "user.email=bcbench@local", "commit", "-q", "--allow-empty", "-m", "trusted"], path) + return path + + +def _prepare_bcquality_root( + engine_root: Path, + pwsh: str, + dest: Path, + bcquality_ref: str | None, + bcquality_repo: str | None = None, + bcquality_local_path: str | None = None, +) -> tuple[Path, str | None]: + env = {**os.environ} + if bcquality_repo: + env["BCQUALITY_REPO"] = bcquality_repo + if bcquality_ref: + env["BCQUALITY_REF"] = bcquality_ref + args = [pwsh, "-NoProfile", "-File", str(_PREPARE_BCQUALITY_SCRIPT), "-EngineRoot", str(engine_root), "-Root", str(dest)] + if bcquality_local_path: + args += ["-LocalPath", bcquality_local_path] + result = subprocess.run( + args, + capture_output=True, + text=True, + env=env, + check=False, + ) + if result.returncode != 0: + logger.error(f"BCQuality preparation failed:\n{result.stdout}\n{result.stderr}") + raise AgentError(f"Failed to prepare BCQuality root (exit {result.returncode}).") + root: Path | None = None + sha: str | None = None + for line in result.stdout.splitlines(): + if line.startswith("root="): + root = Path(line[len("root=") :].strip()) + elif line.startswith("sha="): + sha = line[len("sha=") :].strip() + if root is None or not root.exists(): + raise AgentError("BCQuality preparation did not report a valid root.") + return root, sha + + +def _write_review_json(output_dir: Path, repo_path: Path) -> int: + agent_output = output_dir / _AGENT_OUTPUT_FILE + if not agent_output.exists(): + raise AgentError(f"Engine did not produce {_AGENT_OUTPUT_FILE} in {output_dir}.") + report = load_engine_report(agent_output.read_text(encoding="utf-8")) + if report is None: + raise AgentError(f"Engine {_AGENT_OUTPUT_FILE} was empty or not a valid findings report; refusing to score it as a clean review.") + if not isinstance(report.get("findings"), list): + raise AgentError(f"Engine report in {_AGENT_OUTPUT_FILE} has no findings list (got {type(report.get('findings')).__name__}); refusing to score it as a clean review.") + comments = engine_report_to_review_comments(report) + (repo_path / _REVIEW_OUTPUT_FILE).write_text(json.dumps(comments, indent=2), encoding="utf-8") + return len(comments) + + +def run_pr_review_agent( + entry: BaseDatasetEntry, + model: str, + category: EvaluationCategory, + repo_path: Path, + output_dir: Path, + bcquality_ref: str | None = None, + bcquality_repo: str | None = None, + bcquality_local_path: str | None = None, + min_severity: str | None = None, +) -> tuple[AgentMetrics | None, ExperimentConfiguration]: + """Run the engine's generate half on a code-review entry and write review.json. + + Separate from run_copilot_agent by design: this spawns the PROD BC-ALAgents + PowerShell orchestrator (Copilot is spawned inside the engine, not here), so it + owns none of the copilot-harness prompt/MCP/LSP wiring and takes engine-specific + inputs (BCQuality source, min severity) for the code-review category only. + + Returns: + Tuple of (AgentMetrics, ExperimentConfiguration). + """ + if category is not EvaluationCategory.CODE_REVIEW: + raise AgentError(f"The engine agent only supports the code-review category, got {category.value}.") + if not isinstance(entry, CodeReviewEntry): + raise AgentError(f"The engine agent requires a CodeReviewEntry, got {type(entry).__name__}.") + + settings = _load_pr_review_settings() + engine_root = _resolve_pr_review_root(settings) + pwsh = _resolve_pwsh() + gh_token = _resolve_gh_token() + agent_version = str(settings.get("agent_version", "0.0.0")) + severity = min_severity or settings.get("min_severity") or "Low" + bcquality_cfg = settings.get("bcquality") or {} + bcquality_repo = bcquality_repo or bcquality_cfg.get("repo") + bcquality_ref = bcquality_ref or bcquality_cfg.get("ref") + bcquality_local_path = bcquality_local_path or bcquality_cfg.get("local_path") + + output_dir.mkdir(parents=True, exist_ok=True) + logger.info(f"Running BC-ALAgents review engine on: {entry.instance_id}") + + _commit_patch_as_head(repo_path) + trusted_workspace = _init_trusted_workspace(output_dir / "trusted") + bcquality_root, bcquality_sha = _prepare_bcquality_root( + engine_root, + pwsh, + output_dir / "bcquality", + bcquality_ref, + bcquality_repo, + bcquality_local_path, + ) + + shell = engine_root / "agents" / "ALReviewAgent" / "scripts" / "Invoke-PRReviewShell.ps1" + env = { + **os.environ, + "REVIEW_SOURCE": "local", + "BASE_REF": entry.base_commit, + "REVIEW_TARGET_WORKSPACE": str(repo_path), + "REVIEW_WORKSPACE": str(trusted_workspace), + "REVIEW_OUTPUT_DIR": str(output_dir), + "BCQUALITY_ROOT": str(bcquality_root), + "COPILOT_MODEL": model, + "COPILOT_REVIEW_AGENT_VERSION": agent_version, + "AGENT_MINIMUM_SEVERITY": severity, + "GH_TOKEN": gh_token, + } + + plugins = [f"bc-review-engine@{_resolve_engine_revision(engine_root)}"] + if bcquality_sha: + plugins.append(f"BCQuality@{bcquality_sha}") + config = ExperimentConfiguration( + custom_agent="bc-review-engine", + plugins=plugins, + ) + + start = time.monotonic() + try: + result = subprocess.run( + [pwsh, "-NoProfile", "-File", str(shell), "-GenerateOnly", "-OutputDir", str(output_dir)], + cwd=str(repo_path), + env=env, + capture_output=True, + text=True, + timeout=_config.timeout.agent_execution, + check=True, + ) + logger.debug(f"Engine stdout:\n{result.stdout}") + if result.stderr: + logger.debug(f"Engine stderr:\n{result.stderr}") + count = _write_review_json(output_dir, repo_path) + logger.info(f"Engine review complete for {entry.instance_id}: wrote {count} comment(s) to {_REVIEW_OUTPUT_FILE}") + except subprocess.TimeoutExpired: + logger.exception(f"Engine review timed out after {_config.timeout.agent_execution} seconds") + metrics = AgentMetrics(execution_time=_config.timeout.agent_execution) + raise AgentTimeoutError("Engine review timed out", metrics=metrics, config=config) from None + except subprocess.CalledProcessError as e: + logger.exception(f"Engine review failed (exit {e.returncode}):\n{e.stdout}\n{e.stderr}") + raise AgentError(f"Engine review execution failed: {e}") from None + except Exception: + logger.exception("Unexpected error running engine review") + raise + else: + return AgentMetrics(execution_time=time.monotonic() - start), config diff --git a/src/bcbench/agent/copilot/pr_review/review_output.py b/src/bcbench/agent/copilot/pr_review/review_output.py new file mode 100644 index 000000000..ab0c86add --- /dev/null +++ b/src/bcbench/agent/copilot/pr_review/review_output.py @@ -0,0 +1,106 @@ +"""Map the engine's findings report onto BC-Bench's review.json schema. + +The BC-ALAgents engine writes ``agent-output.txt`` (the harvested +``_review-report.json``) in its output directory. Each finding is shaped like:: + + { + "severity": "Critical|High|Medium|Low", + "location": { "file": "", "line": }, + "message": "", + "domain": "", + ... + } + +BC-Bench's code-review scorer instead reads ``review.json`` from the repo root as +a flat list of ``{file, line_start, line_end, severity, body}`` objects (see +``bcbench.evaluate.review_parsing.parse_review_output``). This module performs the +one transform between the two so the engine's generate half plugs into the +existing scoring pipeline unchanged. +""" + +import json +from pathlib import PurePosixPath +from typing import Any + +from bcbench.logger import get_logger + +logger = get_logger(__name__) + +__all__ = ["engine_report_to_review_comments", "load_engine_report"] + + +def load_engine_report(raw_output: str) -> dict[str, Any] | None: + """Parse the engine's ``agent-output.txt`` text into a report dict. + + Returns ``None`` when the text is empty or not a JSON object. + """ + if not raw_output.strip(): + return None + try: + report = json.loads(raw_output) + except json.JSONDecodeError: + logger.warning("Engine agent-output.txt is not valid JSON") + return None + if not isinstance(report, dict): + logger.warning(f"Engine report is not a JSON object (got {type(report).__name__})") + return None + return report + + +def _normalize_path(file_value: str) -> str: + return PurePosixPath(file_value.strip().replace("\\", "/")).as_posix().removeprefix("./") + + +def engine_report_to_review_comments(report: dict[str, Any]) -> list[dict[str, Any]]: + """Convert an engine findings report into review.json comment dicts. + + Findings without a file, a positive line, or body text are dropped — those + cannot be scored as located comments and mirror what the engine's own + renderer would skip. + """ + findings = report.get("findings") + if not isinstance(findings, list): + return [] + + comments: list[dict[str, Any]] = [] + for finding in findings: + if not isinstance(finding, dict): + continue + + location = finding.get("location") + location = location if isinstance(location, dict) else {} + file_value = location.get("file") + line_value = location.get("line") + + # The finding's human text: prefer the rendered message, then the + # structured issue/recommendation the signature is built from. + body = finding.get("message") or finding.get("issue") or finding.get("recommendation") + + if not isinstance(file_value, str) or not file_value.strip(): + continue + if not isinstance(line_value, (int, str)) or isinstance(line_value, bool): + continue + try: + line = int(line_value) + except (TypeError, ValueError): + continue + if line <= 0: + continue + if not isinstance(body, str) or not body.strip(): + continue + + severity = finding.get("severity") + domain = finding.get("domain") + + comments.append( + { + "file": _normalize_path(file_value), + "line_start": line, + "line_end": line, + "severity": str(severity).strip().lower() if isinstance(severity, str) and severity.strip() else None, + "domain": domain.strip() if isinstance(domain, str) and domain.strip() else None, + "body": body.strip(), + } + ) + + return comments diff --git a/src/bcbench/agent/copilot/pr_review/scripts/Prepare-BCQualityRoot.ps1 b/src/bcbench/agent/copilot/pr_review/scripts/Prepare-BCQualityRoot.ps1 new file mode 100644 index 000000000..7b224453f --- /dev/null +++ b/src/bcbench/agent/copilot/pr_review/scripts/Prepare-BCQualityRoot.ps1 @@ -0,0 +1,85 @@ +<# +.SYNOPSIS + Clone/copy + filter BCQuality into a root the BC-ALAgents engine can consume. + +.DESCRIPTION + Mirrors the `resolve` -> "Fetch and filter BCQuality" step of the engine's + reusable review.yml so BC-Bench prepares BCQUALITY_ROOT exactly the way PROD + does: read the engine's pinned repo/ref via Get-BCQualityConfig.ps1, shallow + fetch that ref, then run Invoke-BCQualityFilter.ps1 over the checkout. + + Honors the same BCQUALITY_* environment overrides Get-BCQualityConfig reads + (e.g. BCQUALITY_REPO, BCQUALITY_REF), so a caller can pin a different content + repo/ref (such as a private branch or fork) without editing the engine. + + Pass -LocalPath to iterate on a local BCQuality checkout without pushing: the + checkout is COPIED into -Root (excluding .git) and filtered there, so the + original working tree is never modified (the filter deletes files). This is the + fast inner loop for optimizing BCQuality structure and re-scoring in BC-Bench. + + Emits the resolved root and SHA as `root=` / `sha=` lines on + stdout for the Python caller to parse. +#> +[CmdletBinding()] +param( + [Parameter(Mandatory)] [string] $EngineRoot, + [Parameter(Mandatory)] [string] $Root, + [string] $LocalPath +) + +Set-StrictMode -Version Latest +$ErrorActionPreference = 'Stop' + +$scripts = Join-Path $EngineRoot 'agents' 'ALReviewAgent' 'scripts' +$getConfig = Join-Path $scripts 'Get-BCQualityConfig.ps1' +$filter = Join-Path $scripts 'Invoke-BCQualityFilter.ps1' + +if (-not (Test-Path $getConfig)) { throw "Get-BCQualityConfig.ps1 not found under '$scripts' - check engine path." } + +if (-not (Get-Module -ListAvailable -Name powershell-yaml)) { + Install-Module powershell-yaml -Scope CurrentUser -Force -AllowClobber +} + +# Get-BCQualityConfig honors BCQUALITY_REPO/REF and the knowledge/layer overrides; +# the resulting $cfg drives both the fetch (repo/ref) and the filter (allow/deny). +$cfg = & $getConfig + +if ($LocalPath) { + $src = (Resolve-Path -LiteralPath $LocalPath).Path + if (-not (Test-Path -LiteralPath $src)) { throw "BCQuality -LocalPath does not exist: $src" } + + Write-Host "Copying local BCQuality from $src into $Root (excluding .git)" + if (Test-Path -LiteralPath $Root) { Remove-Item -LiteralPath $Root -Recurse -Force } + New-Item -ItemType Directory -Force -Path $Root | Out-Null + Get-ChildItem -LiteralPath $src -Force | Where-Object { $_.Name -ne '.git' } | ForEach-Object { + Copy-Item -LiteralPath $_.FullName -Destination $Root -Recurse -Force + } + + # Provenance: use the source checkout's HEAD sha when it is a git repo. + $resolvedSha = 'local' + $headSha = (& git -C $src rev-parse HEAD 2>$null) + if ($LASTEXITCODE -eq 0 -and $headSha) { $resolvedSha = "local:$($headSha.Trim())" } + Write-Host "BCQuality local source SHA: $resolvedSha" +} +else { + $repo = $cfg.bcquality.repo + $ref = $cfg.bcquality.ref + + Write-Host "Fetching BCQuality from $repo@$ref into $Root" + if (Test-Path -LiteralPath $Root) { Remove-Item -LiteralPath $Root -Recurse -Force } + New-Item -ItemType Directory -Force -Path $Root | Out-Null + git -C $Root init -q + git -C $Root remote add origin $repo + git -C $Root fetch --depth=1 origin "$ref" + if ($LASTEXITCODE -ne 0) { throw "git fetch of BCQuality ref '$ref' failed (exit $LASTEXITCODE)" } + git -C $Root checkout -q FETCH_HEAD + if ($LASTEXITCODE -ne 0) { throw "git checkout of BCQuality ref '$ref' failed (exit $LASTEXITCODE)" } + + $resolvedSha = (& git -C $Root rev-parse HEAD).Trim() + Write-Host "BCQuality resolved SHA: $resolvedSha" +} + +& $filter -BCQualityRoot $Root -Config $cfg | Out-Null + +"root=$Root" +"sha=$resolvedSha" diff --git a/src/bcbench/agent/shared/config.yaml b/src/bcbench/agent/shared/config.yaml index 54e3a0495..5cd213cd4 100644 --- a/src/bcbench/agent/shared/config.yaml +++ b/src/bcbench/agent/shared/config.yaml @@ -77,22 +77,6 @@ prompt: Extensibility request: {{task}} - code-review-template: | - /review - - Review only the uncommitted working-tree changes (git diff HEAD); do not compare commits such as HEAD~1..HEAD or origin/main. - - Your only deliverable is a file named review.json in the repository root. You MUST write it before finishing; if you do not, your review is lost and counts as no output. - - review.json must contain a single JSON array. Each finding is an object with these fields: - - file: repo-relative path of the file the finding refers to (string, required) - - line_start: 1-based line number where the issue starts (integer, required) - - line_end: line number where the issue ends (integer, optional) - - severity: one of critical, high, medium, or low (optional, defaults to medium) - - body: concise description of the issue (string, required) - - If there are no findings, write an empty array. Write only valid JSON to review.json, with no surrounding markdown or commentary. - # controls: # 1. whether to copy custom instructions from `src/bcbench/agent/shared/instructions//` # - Copilot: copies to repo/.github/ and renames AGENTS.md to copilot-instructions.md @@ -175,3 +159,22 @@ mcp: # type: "stdio" # command: "npx" # args: ["-y", "@modelcontextprotocol/server-filesystem", "{{repo_path}}"] + +# BC-ALAgents review engine settings. The code-review category runs the engine's own +# generate half (Invoke-PRReviewShell.ps1 -GenerateOnly) in local mode instead of a +# bespoke review prompt, so it measures the real PROD engine + BCQuality. These knobs +# live here in the shared config so they are easy to tweak on a private branch. +# path: local microsoft/BC-ALAgents checkout, e.g. "C:/depot/BC-ALAgents". Left +# null in the repo; CI sets BC_PR_REVIEW_ROOT (which takes precedence). +# For a local run, set that env var or put your checkout path here on your +# own branch (do not commit a machine-specific path). +# bcquality: content source for the engine run; all optional (null = engine's pinned +# repo/ref). Set local_path to iterate on a local BCQuality checkout without +# pushing (it is copied and filtered; the original is never modified). +# CLI flags (--bcquality-repo/-ref/-local-path) override these. +pr_review: + path: null + bcquality: + repo: null + ref: null + local_path: null diff --git a/src/bcbench/cli_options.py b/src/bcbench/cli_options.py index 3361d50c2..78b9d1c01 100644 --- a/src/bcbench/cli_options.py +++ b/src/bcbench/cli_options.py @@ -23,6 +23,17 @@ EvaluationCategoryOption = Annotated[EvaluationCategory, typer.Option(help="Category of evaluation to perform")] + +def reject_code_review(category: EvaluationCategory, verb: str) -> None: + """Guard general harness commands (copilot/claude) against the code-review category. + + code-review is not a harness choice: it is always served by the dedicated engine + command, so a general harness must refuse it rather than run a divergent review. + """ + if category is EvaluationCategory.CODE_REVIEW: + raise typer.BadParameter(f"code-review is not available under a general harness; use 'bcbench {verb} code-review' instead.") + + CopilotModel = Annotated[ Literal[ "claude-sonnet-5", diff --git a/src/bcbench/commands/evaluate.py b/src/bcbench/commands/evaluate.py index a8b59ad37..4af96b754 100644 --- a/src/bcbench/commands/evaluate.py +++ b/src/bcbench/commands/evaluate.py @@ -7,6 +7,7 @@ import typer from bcbench.agent import BCalBackendConfig, run_bcal_agent, run_claude_code, run_copilot_agent +from bcbench.agent.copilot.pr_review import run_pr_review_agent from bcbench.cli_options import ( ClaudeCodeModel, ContainerName, @@ -17,6 +18,7 @@ OutputDir, RepoPath, RunId, + reject_code_review, ) from bcbench.config import get_config from bcbench.dataset import BaseDatasetEntry, NL2ALEntry @@ -40,6 +42,59 @@ def _prepare_run_dir(output_dir: Path, run_id: str) -> Path: return run_dir +def _run_pr_review_evaluation( + entry_id: str, + model: str, + repo_path: Path, + output_dir: Path, + run_id: str, + bcquality_ref: str | None = None, + bcquality_repo: str | None = None, + bcquality_local_path: str | None = None, + min_severity: str | None = None, +) -> None: + """Evaluate a code-review entry through the BC-ALAgents review engine. + + Backs the dedicated 'evaluate code-review' command: the code-review category always + runs the engine's own generate half (the real PROD path), so callers never drive a + bespoke review prompt. BCQuality source and severity default to the engine config when + not overridden. + """ + category = EvaluationCategory.CODE_REVIEW + entry = category.entry_class.load(category.dataset_path, entry_id=entry_id)[0] + run_dir = _prepare_run_dir(output_dir, run_id) + + logger.info(f"Running evaluation on entry {entry_id} with the BC-ALAgents review engine") + + context = EvaluationContext( + entry=entry, + repo_path=repo_path, + result_dir=run_dir, + container=None, + model=model, + agent_name=AgentHarness.PR_REVIEW, + category=category, + ) + + category.pipeline.execute( + context, + lambda ctx: run_pr_review_agent( + entry=ctx.entry, + repo_path=ctx.repo_path, + category=category, + model=ctx.model, + output_dir=ctx.result_dir, + bcquality_ref=bcquality_ref, + bcquality_repo=bcquality_repo, + bcquality_local_path=bcquality_local_path, + min_severity=min_severity, + ), + ) + + logger.info("Evaluation complete!") + logger.info(f"Results saved to: {run_dir}") + + @evaluate_app.command("copilot") def evaluate_copilot( entry_id: Annotated[str, typer.Argument(help="Entry ID to run")], @@ -59,6 +114,7 @@ def evaluate_copilot( To only run the agent to generate a patch without building/testing, use 'bcbench run copilot' instead. """ + reject_code_review(category, "evaluate") entry = category.entry_class.load(category.dataset_path, entry_id=entry_id)[0] run_dir = _prepare_run_dir(output_dir, run_id) @@ -114,6 +170,7 @@ def evaluate_claude_code( To only run the agent to generate a patch without building/testing, use 'bcbench run claude' instead. """ + reject_code_review(category, "evaluate") entry = category.entry_class.load(category.dataset_path, entry_id=entry_id)[0] run_dir = _prepare_run_dir(output_dir, run_id) @@ -150,6 +207,41 @@ def evaluate_claude_code( logger.info(f"Results saved to: {run_dir}") +@evaluate_app.command("code-review") +def evaluate_code_review( + entry_id: Annotated[str, typer.Argument(help="Entry ID to run")], + model: CopilotModel = "claude-sonnet-5", + repo_path: RepoPath = _config.paths.testbed_path, + output_dir: OutputDir = _config.paths.evaluation_results_path, + run_id: RunId = "pr_review_test_run", + bcquality_ref: Annotated[str | None, typer.Option(help="Override the BCQuality ref (defaults to the engine's pinned ref)")] = None, + bcquality_repo: Annotated[str | None, typer.Option(help="Override the BCQuality repo, e.g. a private fork (defaults to config/engine)")] = None, + bcquality_local_path: Annotated[str | None, typer.Option(help="Use a local BCQuality checkout (copied + filtered, never modified) instead of fetching")] = None, + min_severity: Annotated[str | None, typer.Option(help="AGENT_MINIMUM_SEVERITY floor (defaults to config)")] = None, +) -> None: + """ + Evaluate the code-review category on a single entry via the BC-ALAgents review engine. + + code-review is not a general harness choice: it always runs the engine's own generate + shell in local mode - the real PROD generate path - then scores the resulting + review.json with the standard code-review judge. Requires a local BC-ALAgents checkout + (pr_review.path in config.yaml or BC_PR_REVIEW_ROOT), PowerShell 7+, and GH_TOKEN. + + To only generate review.json without scoring, use 'bcbench run code-review' instead. + """ + _run_pr_review_evaluation( + entry_id, + model=model, + repo_path=repo_path, + output_dir=output_dir, + run_id=run_id, + bcquality_ref=bcquality_ref, + bcquality_repo=bcquality_repo, + bcquality_local_path=bcquality_local_path, + min_severity=min_severity, + ) + + @evaluate_app.command("bcal") def evaluate_bcal( entry_id: Annotated[str, typer.Argument(help="Entry ID to run")], diff --git a/src/bcbench/commands/run.py b/src/bcbench/commands/run.py index cb8e79acc..eaf3b8a92 100644 --- a/src/bcbench/commands/run.py +++ b/src/bcbench/commands/run.py @@ -1,5 +1,6 @@ """CLI commands for running agents.""" +from pathlib import Path from typing import Annotated, cast import typer @@ -7,6 +8,7 @@ from bcbench.agent.bcal import BCalBackendConfig, run_bcal_agent from bcbench.agent.claude import run_claude_code from bcbench.agent.copilot import run_copilot_agent +from bcbench.agent.copilot.pr_review import run_pr_review_agent from bcbench.cli_options import ( ClaudeCodeModel, ContainerName, @@ -14,6 +16,7 @@ EvaluationCategoryOption, OutputDir, RepoPath, + reject_code_review, ) from bcbench.config import get_config from bcbench.dataset import NL2ALEntry @@ -26,6 +29,39 @@ run_app = typer.Typer(help="Run agents on single dataset entry") +def _run_pr_review( + entry_id: str, + model: str, + repo_path: Path, + output_dir: Path, + bcquality_ref: str | None = None, + bcquality_repo: str | None = None, + bcquality_local_path: str | None = None, + min_severity: str | None = None, +) -> None: + """Generate review.json for a code-review entry via the BC-ALAgents review engine. + + Backs the dedicated 'run code-review' command: code-review always runs the engine's + real generate half, never a bespoke prompt. BCQuality source and severity default to + the engine config when not overridden. + """ + category = EvaluationCategory.CODE_REVIEW + entry = category.entry_class.load(category.dataset_path, entry_id=entry_id)[0] + category.pipeline.setup_workspace(entry, repo_path) + + run_pr_review_agent( + entry=entry, + repo_path=repo_path, + model=model, + category=category, + output_dir=output_dir, + bcquality_ref=bcquality_ref, + bcquality_repo=bcquality_repo, + bcquality_local_path=bcquality_local_path, + min_severity=min_severity, + ) + + @run_app.command("copilot") def run_copilot( entry_id: Annotated[str, typer.Argument(help="Entry ID to run")], @@ -45,6 +81,7 @@ def run_copilot( Example: uv run bcbench run copilot microsoft__BCApps-5633 --category bug-fix --repo-path /path/to/BCApps """ + reject_code_review(category, "run") entry = category.entry_class.load(category.dataset_path, entry_id=entry_id)[0] category.pipeline.setup_workspace(entry, repo_path) @@ -79,6 +116,7 @@ def run_claude( Example: uv run bcbench run claude microsoft__BCApps-5633 --category bug-fix --repo-path /path/to/BCApps """ + reject_code_review(category, "run") entry = category.entry_class.load(category.dataset_path, entry_id=entry_id)[0] category.pipeline.setup_workspace(entry, repo_path) @@ -94,6 +132,41 @@ def run_claude( ) +@run_app.command("code-review") +def run_code_review( + entry_id: Annotated[str, typer.Argument(help="Entry ID to run")], + model: CopilotModel = "claude-sonnet-5", + repo_path: RepoPath = _config.paths.testbed_path, + output_dir: OutputDir = _config.paths.evaluation_results_path, + bcquality_ref: Annotated[str | None, typer.Option(help="Override the BCQuality ref (defaults to the engine's pinned ref)")] = None, + bcquality_repo: Annotated[str | None, typer.Option(help="Override the BCQuality repo, e.g. a private fork (defaults to config/engine)")] = None, + bcquality_local_path: Annotated[str | None, typer.Option(help="Use a local BCQuality checkout (copied + filtered, never modified) instead of fetching")] = None, + min_severity: Annotated[str | None, typer.Option(help="AGENT_MINIMUM_SEVERITY floor (defaults to config)")] = None, +) -> None: + """ + Run the code-review category on a single entry via the BC-ALAgents review engine. + + code-review is not a general harness choice: it always runs the engine's real generate + half (never a bespoke prompt), so it has its own command instead of a copilot/claude + sub-command. Writes review.json in the repo root without scoring; for full evaluation + use 'bcbench evaluate code-review'. Requires a local BC-ALAgents checkout + (pr_review.path in config.yaml or BC_PR_REVIEW_ROOT), PowerShell 7+, and GH_TOKEN. + + Example: + uv run bcbench run code-review synthetic__style-018 --repo-path /path/to/testbed + """ + _run_pr_review( + entry_id, + model=model, + repo_path=repo_path, + output_dir=output_dir, + bcquality_ref=bcquality_ref, + bcquality_repo=bcquality_repo, + bcquality_local_path=bcquality_local_path, + min_severity=min_severity, + ) + + @run_app.command("bcal") def run_bcal( entry_id: Annotated[str, typer.Argument(help="Entry ID to run")], diff --git a/src/bcbench/types.py b/src/bcbench/types.py index 00b7fb7ff..3b2ccde62 100644 --- a/src/bcbench/types.py +++ b/src/bcbench/types.py @@ -167,6 +167,7 @@ class AgentHarness(StrEnum): CLAUDE = "Claude Code" BCAL = "BCal" MOCK = "mock-agent" + PR_REVIEW = "BC PR Review" @property def expected_metrics(self) -> frozenset[str]: @@ -193,7 +194,7 @@ def expected_metrics(self) -> frozenset[str]: completion_tokens=None, tool_usage=None, ) - case AgentHarness.BCAL: + case AgentHarness.BCAL | AgentHarness.PR_REVIEW: expected = AgentMetrics(execution_time=None) case _: raise ValueError(f"Unknown AgentHarness: {self}") diff --git a/tests/test_pr_review_agent.py b/tests/test_pr_review_agent.py new file mode 100644 index 000000000..33e0f30e2 --- /dev/null +++ b/tests/test_pr_review_agent.py @@ -0,0 +1,60 @@ +import json +from pathlib import Path + +import pytest + +from bcbench.agent.copilot.pr_review.agent import _write_review_json +from bcbench.exceptions import AgentError + + +def _dirs(tmp_path: Path) -> tuple[Path, Path]: + out = tmp_path / "out" + out.mkdir() + repo = tmp_path / "repo" + repo.mkdir() + return out, repo + + +def _write_output(output_dir: Path, text: str) -> None: + (output_dir / "agent-output.txt").write_text(text, encoding="utf-8") + + +def test_valid_empty_findings_is_a_clean_review(tmp_path: Path) -> None: + out, repo = _dirs(tmp_path) + _write_output(out, json.dumps({"outcome": "completed", "outcome-reason": "", "findings": []})) + assert _write_review_json(out, repo) == 0 + assert json.loads((repo / "review.json").read_text(encoding="utf-8")) == [] + + +def test_findings_are_mapped(tmp_path: Path) -> None: + out, repo = _dirs(tmp_path) + report = { + "outcome": "completed", + "findings": [{"severity": "High", "location": {"file": "src/Foo.al", "line": 42}, "message": "x", "domain": "ui"}], + } + _write_output(out, json.dumps(report)) + assert _write_review_json(out, repo) == 1 + + +def test_missing_agent_output_raises(tmp_path: Path) -> None: + out, repo = _dirs(tmp_path) + with pytest.raises(AgentError, match="did not produce"): + _write_review_json(out, repo) + + +@pytest.mark.parametrize("text", ["", " ", "not json", "[]"]) +def test_invalid_output_raises_instead_of_clean_review(tmp_path: Path, text: str) -> None: + out, repo = _dirs(tmp_path) + _write_output(out, text) + with pytest.raises(AgentError, match="empty or not a valid"): + _write_review_json(out, repo) + assert not (repo / "review.json").exists() + + +@pytest.mark.parametrize("report", [{"outcome": "failed"}, {"outcome": "dispatch", "findings": None}, {"findings": "nope"}]) +def test_malformed_report_raises_instead_of_clean_review(tmp_path: Path, report: dict) -> None: + out, repo = _dirs(tmp_path) + _write_output(out, json.dumps(report)) + with pytest.raises(AgentError, match="no findings list"): + _write_review_json(out, repo) + assert not (repo / "review.json").exists() diff --git a/tests/test_pr_review_output.py b/tests/test_pr_review_output.py new file mode 100644 index 000000000..8b56bc702 --- /dev/null +++ b/tests/test_pr_review_output.py @@ -0,0 +1,112 @@ +import json + +from bcbench.agent.copilot.pr_review.review_output import engine_report_to_review_comments, load_engine_report +from bcbench.evaluate.review_parsing import parse_review_output + + +def _report(findings: list[dict]) -> dict: + return {"outcome": "completed", "outcome-reason": "", "findings": findings} + + +def test_load_engine_report_parses_object() -> None: + report = load_engine_report(json.dumps(_report([]))) + assert report is not None + assert report["findings"] == [] + + +def test_load_engine_report_rejects_empty_or_invalid() -> None: + assert load_engine_report("") is None + assert load_engine_report(" ") is None + assert load_engine_report("not json") is None + # A bare JSON array is not the engine report shape. + assert load_engine_report("[]") is None + + +def test_maps_nested_location_and_message() -> None: + report = _report( + [ + { + "severity": "High", + "location": {"file": "src/Foo.al", "line": 42}, + "message": "Missing ToolTip on field.", + "domain": "ui", + } + ] + ) + comments = engine_report_to_review_comments(report) + assert comments == [ + { + "file": "src/Foo.al", + "line_start": 42, + "line_end": 42, + "severity": "high", + "domain": "ui", + "body": "Missing ToolTip on field.", + } + ] + + +def test_normalizes_path_and_lowercases_severity() -> None: + report = _report( + [ + { + "severity": "CRITICAL", + "location": {"file": ".\\src\\Bar.al", "line": 7}, + "message": "Unchecked Get.", + "domain": "error-handling", + } + ] + ) + (comment,) = engine_report_to_review_comments(report) + assert comment["file"] == "src/Bar.al" + assert comment["severity"] == "critical" + + +def test_falls_back_to_issue_then_recommendation_for_body() -> None: + report = _report( + [ + {"location": {"file": "a.al", "line": 1}, "issue": "issue text"}, + {"location": {"file": "b.al", "line": 2}, "recommendation": "rec text"}, + ] + ) + comments = engine_report_to_review_comments(report) + assert [c["body"] for c in comments] == ["issue text", "rec text"] + + +def test_drops_findings_missing_file_line_or_body() -> None: + report = _report( + [ + {"location": {"line": 5}, "message": "no file"}, + {"location": {"file": "c.al"}, "message": "no line"}, + {"location": {"file": "d.al", "line": 0}, "message": "non-positive line"}, + {"location": {"file": "e.al", "line": 3}, "message": " "}, + {"location": {"file": "f.al", "line": 3}}, + ] + ) + assert engine_report_to_review_comments(report) == [] + + +def test_missing_or_non_list_findings_yields_empty() -> None: + assert engine_report_to_review_comments({"outcome": "completed"}) == [] + assert engine_report_to_review_comments({"findings": None}) == [] + assert engine_report_to_review_comments({"findings": "nope"}) == [] + + +def test_output_is_consumable_by_review_parser() -> None: + report = _report( + [ + { + "severity": "Medium", + "location": {"file": "src/Baz.al", "line": 10}, + "message": "Some finding.", + "domain": "performance", + } + ] + ) + comments = engine_report_to_review_comments(report) + parsed = parse_review_output(json.dumps(comments)) + assert parsed is not None + assert len(parsed) == 1 + assert parsed[0].file == "src/Baz.al" + assert parsed[0].line_start == 10 + assert parsed[0].body == "Some finding."