diff --git a/.github/workflows/copilot-evaluation.yml b/.github/workflows/copilot-evaluation.yml index dd0a697a0..072b786cd 100644 --- a/.github/workflows/copilot-evaluation.yml +++ b/.github/workflows/copilot-evaluation.yml @@ -36,6 +36,11 @@ on: required: false default: true type: boolean + publish-results: + description: "Publish results to the shared dashboard (Braintrust/Kusto + leaderboard). Turn off to run a full experiment (e.g. your own BCQuality) without touching the dashboard." + required: false + default: true + type: boolean al-mcp: description: "Enable AL MCP server" required: false @@ -62,6 +67,16 @@ on: required: false default: "" type: string + bcquality-ref: + description: "Code-review only: BCQuality ref (branch/tag/SHA) to review against. Blank uses the pinned default (microsoft/BCQuality@main)." + required: false + default: "" + type: string + bcquality-repo: + description: "Code-review only: BCQuality repo as owner/name, for testing a fork. Blank uses microsoft/BCQuality." + required: false + default: "" + type: string concurrency: group: copilot-evaluation-${{ inputs.test-run && 'test' || 'full' }}-${{ inputs.category }} @@ -82,6 +97,7 @@ jobs: needs: get-entries outputs: results-dir: ${{ env.EVALUATION_RESULTS_DIR }} + agent-label: ${{ steps.agent-label.outputs.label }} if: needs.get-entries.outputs.entries != '[]' environment: name: ado-read @@ -100,6 +116,13 @@ jobs: - name: Checkout repository uses: actions/checkout@v5 + - name: Resolve agent label + id: agent-label + shell: pwsh + run: | + $label = if ('${{ inputs.category }}' -eq 'code-review') { 'BC PR-Review Engine' } else { 'GitHub Copilot CLI' } + "label=$label" | Out-File -FilePath $env:GITHUB_OUTPUT -Append + - name: Setup BC Container and Repository id: setup-env timeout-minutes: 40 @@ -136,6 +159,12 @@ jobs: env: COPILOT_GITHUB_TOKEN: ${{ github.token }} GH_TOKEN: ${{ github.token }} + # Code-review only. The engine reads these directly; blank refs use the pinned + # defaults (microsoft/BCQuality@main, engine _DEFAULT_ENGINE_REF). A tester sets + # bcquality-ref (and bcquality-repo for a fork) at dispatch to review their own. + BCQUALITY_REF: ${{ inputs.bcquality-ref }} + BCQUALITY_REPO: ${{ inputs.bcquality-repo }} + ENGINE_REPO_TOKEN: ${{ secrets.ENGINE_REPO_TOKEN }} run: | Write-Output "::add-mask::$env:COPILOT_GITHUB_TOKEN" @@ -164,8 +193,8 @@ jobs: with: results-dir: ${{ needs.evaluate-with-copilot-cli.outputs.results-dir }} model: ${{ inputs.model }} - agent: "GitHub Copilot CLI" - mock: ${{ inputs.test-run }} + agent: ${{ needs.evaluate-with-copilot-cli.outputs.agent-label }} + mock: ${{ inputs.test-run || !inputs.publish-results }} category: ${{ inputs.category }} git-ref: ${{ inputs.git-ref || github.ref_name }} secrets: inherit @@ -181,4 +210,4 @@ jobs: workflow-file: copilot-evaluation.yml repeat: ${{ inputs.repeat }} workflow-inputs: | - {"model": "${{ inputs.model }}", "category": "${{ inputs.category }}", "test-run": "${{ inputs.test-run }}", "al-mcp": "${{ inputs.al-mcp }}", "al-lsp": "${{ inputs.al-lsp }}", "git-ref": "${{ inputs.git-ref || github.ref_name }}"} + {"model": "${{ inputs.model }}", "category": "${{ inputs.category }}", "test-run": "${{ inputs.test-run }}", "al-mcp": "${{ inputs.al-mcp }}", "al-lsp": "${{ inputs.al-lsp }}", "git-ref": "${{ inputs.git-ref || github.ref_name }}", "bcquality-ref": "${{ inputs.bcquality-ref }}", "bcquality-repo": "${{ inputs.bcquality-repo }}", "publish-results": "${{ inputs.publish-results }}"} diff --git a/src/bcbench/agent/__init__.py b/src/bcbench/agent/__init__.py index ab30132d1..e53394c23 100644 --- a/src/bcbench/agent/__init__.py +++ b/src/bcbench/agent/__init__.py @@ -3,5 +3,13 @@ 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.engine import code_review_uses_engine, run_engine_review -__all__ = ["BCalBackendConfig", "run_bcal_agent", "run_claude_code", "run_copilot_agent"] +__all__ = [ + "BCalBackendConfig", + "code_review_uses_engine", + "run_bcal_agent", + "run_claude_code", + "run_copilot_agent", + "run_engine_review", +] diff --git a/src/bcbench/agent/engine.py b/src/bcbench/agent/engine.py new file mode 100644 index 000000000..962dd2023 --- /dev/null +++ b/src/bcbench/agent/engine.py @@ -0,0 +1,408 @@ +"""BC PR-Review engine runner for the code-review category (faithful convergence arm). + +Instead of BC-Bench's own review pipeline, this runner invokes the engine's +self-contained local-review entry point (``Invoke-LocalReview.ps1`` from +``microsoft/BC-ALAgents``), which filters a BCQuality checkout and runs the +exact production reviewer against the patched worktree. + +The entry point writes the agent's raw report to ``/_review-report.json`` +(and run stats to ``_run-metrics.json``). BC-Bench normalizes that report into the +flat ``review.json`` its scorer understands. Note this scores the raw agent report; +the full production flow's later post-processing (dedup, volume caps, placement) is +not represented, so this arm measures the reviewer's findings, not the posted set. + +The engine is pinned to a released tag (``_DEFAULT_ENGINE_REF``, override ENGINE_REF) so +BCQuality content stays the only variable under test: point the arm at a BCQuality fork, +branch, or local checkout (``BCQUALITY_REPO`` / ``BCQUALITY_REF`` / ``BCQUALITY_ROOT``) to +measure how a BCQuality change moves the score. +""" + +import base64 +import json +import os +import shutil +import subprocess +import time +from pathlib import Path + +from bcbench.dataset import BaseDatasetEntry +from bcbench.dataset.codereview import Severity +from bcbench.exceptions import AgentError, AgentTimeoutError +from bcbench.logger import get_logger +from bcbench.types import AgentMetrics, ExperimentConfiguration + +logger = get_logger(__name__) + +LOCAL_REVIEW_SCRIPT_NAME = "Invoke-LocalReview.ps1" +REVIEW_REPORT_FILE_NAME = "_review-report.json" +RUN_METRICS_FILE_NAME = "_run-metrics.json" +REVIEW_OUTPUT_FILE = "review.json" +BCQUALITY_REPO_URL = "https://github.com/microsoft/BCQuality.git" +ENGINE_REPO_URL = "https://github.com/microsoft/BC-ALAgents.git" +# The engine's local-review scripts live here inside microsoft/BC-ALAgents. +ENGINE_SCRIPTS_SUBPATH = "agents/ALReviewAgent/scripts" +# Pin a released engine tag by default so BCQuality content is the only variable under +# test; a floating "latest" would let a mid-experiment engine release skew an A/B. Override +# with ENGINE_REF. The exact commit reviewed is still recorded per run via engine_ref. +# 1.19.4 is the first tag that carries the BCQUALITY_CONSUME=plugin path this arm relies on. +_DEFAULT_ENGINE_REF = "1.20.4" +_ENGINE_TIMEOUT_SECONDS = 1800 + + +def _pwsh() -> str: + pwsh = shutil.which("pwsh") + if not pwsh: + raise AgentError("pwsh (PowerShell 7+) is required to run the PR-review engine but was not found on PATH") + return pwsh + + +def _first_env(*names: str) -> str | None: + """Return the first non-empty value among the given environment variables.""" + for name in names: + value = os.environ.get(name) + if value: + return value + return None + + +def code_review_uses_engine() -> bool: + """Whether the code-review category routes through the BC PR-Review engine arm. + + This arm defaults to the engine, so activation lives in code (not the workflow): + dispatching on this branch runs the engine without any workflow env. Set + BCBENCH_CODE_REVIEW_AGENT=copilot to run the plain Copilot reviewer instead (A/B). + """ + return os.environ.get("BCBENCH_CODE_REVIEW_AGENT", "engine").strip().lower() != "copilot" + + +def _git(repo_path: Path, *args: str) -> str: + result = subprocess.run(["git", *args], cwd=repo_path, capture_output=True, text=True, check=False) + if result.returncode != 0: + raise AgentError(f"git {' '.join(args)} failed: {result.stderr.strip()}") + return result.stdout.strip() + + +def _git_head(path: Path) -> str | None: + """Resolve the HEAD commit of a checkout, or None if it is not a git repo.""" + try: + result = subprocess.run(["git", "-C", str(path), "rev-parse", "HEAD"], capture_output=True, text=True, check=False) + except OSError: + return None + return result.stdout.strip() if result.returncode == 0 else None + + +def _commit_patched_worktree(repo_path: Path) -> str: + base_ref = _git(repo_path, "rev-parse", "HEAD") + _git(repo_path, "add", "-A") + _git( + repo_path, + "-c", + "user.name=bcbench", + "-c", + "user.email=bcbench@local", + "-c", + "commit.gpgsign=false", + "commit", + "--allow-empty", + "-m", + "bcbench: apply dataset patch for local review", + ) + return base_ref + + +def _bcquality_repo_url() -> str: + """Resolve the BCQuality repo to review with, honoring the BCQUALITY_REPO override. + + Accepts a full clone URL or a bare ``owner/repo`` (assumed on github.com), so a + CI run can point the arm at a fork without editing the engine. + """ + repo = os.environ.get("BCQUALITY_REPO") + if not repo: + return BCQUALITY_REPO_URL + if repo.startswith(("http://", "https://", "git@")): + return repo + return f"https://github.com/{repo}.git" + + +def _engine_repo_url() -> str: + """Resolve the engine repo to clone, honoring the ENGINE_REPO override. + + Accepts a full clone URL or a bare ``owner/repo`` (assumed on github.com), mirroring + _bcquality_repo_url so the arm can target a fork without editing the workflow. + """ + repo = os.environ.get("ENGINE_REPO") + if not repo: + return ENGINE_REPO_URL + if repo.startswith(("http://", "https://", "git@")): + return repo + return f"https://github.com/{repo}.git" + + +def _clone_at_ref(url: str, ref: str, dest: Path, token: str | None) -> str | None: + """Clone ``url`` into ``dest``, check out ``ref``, and return the resolved SHA. + + When a token is present it is passed via an http.extraheader (like actions/checkout) + so it never lands in the remote URL or an error message; public repos clone with no + token. Returns the checked-out commit for reproducibility. + """ + auth: list[str] = [] + if token: + basic = base64.b64encode(f"x-access-token:{token}".encode()).decode() + auth = ["-c", f"http.https://github.com/.extraheader=AUTHORIZATION: basic {basic}"] + clone = subprocess.run(["git", *auth, "clone", "--quiet", url, str(dest)], capture_output=True, text=True, check=False) + if clone.returncode != 0: + raise AgentError(f"Failed to clone {url}: {clone.stderr.strip()}") + checkout = subprocess.run(["git", "-C", str(dest), "checkout", "--quiet", ref], capture_output=True, text=True, check=False) + if checkout.returncode != 0: + raise AgentError(f"Failed to checkout '{ref}' from {url}: {checkout.stderr.strip()}") + return _git_head(dest) + + +def _prepare_bcquality(work_dir: Path) -> tuple[Path, str | None]: + """Provide a disposable BCQuality checkout for the engine and its resolved commit. + + Invoke-LocalReview.ps1 requires -BCQualityRoot and its filter step DELETES files + in place, so we never point it at a live clone. Prefer a caller-provided checkout + (copied minus .git); otherwise clone microsoft/BCQuality (or BCQUALITY_REPO) at + BCQUALITY_REF. Returns the checkout path and the resolved BCQuality SHA (for + reproducibility), or None when the source is not a git repo. + """ + dest = work_dir / "bcquality" + if dest.exists(): + shutil.rmtree(dest) + + local = os.environ.get("BCQUALITY_ROOT") + if local: + shutil.copytree(local, dest, ignore=shutil.ignore_patterns(".git")) + return dest, _git_head(Path(local)) + + url = _bcquality_repo_url() + ref = os.environ.get("BCQUALITY_REF") or "main" + token = _first_env("BCQUALITY_REPO_TOKEN", "ENGINE_REPO_TOKEN", "GH_TOKEN", "GITHUB_TOKEN") + logger.info(f"Cloning BCQuality @ {ref}") + return dest, _clone_at_ref(url, ref, dest, token) + + +def _prepare_engine(work_dir: Path, engine_scripts_dir: Path | None) -> tuple[Path, str | None]: + """Provide the engine's agent/scripts directory and its resolved commit. + + Prefer a caller/env-provided local checkout (``--engine-scripts-dir`` / ENGINE_SCRIPTS_DIR) + for local dev; otherwise clone microsoft/BC-ALAgents (or ENGINE_REPO) at ENGINE_REF + (default a pinned release tag, see _DEFAULT_ENGINE_REF) and use its agent/scripts subdir. + The exact commit reviewed is returned for reproducibility. + """ + if engine_scripts_dir is not None: + return engine_scripts_dir, _git_head(engine_scripts_dir) + + dest = work_dir / "engine" + if dest.exists(): + shutil.rmtree(dest) + url = _engine_repo_url() + ref = os.environ.get("ENGINE_REF") or _DEFAULT_ENGINE_REF + token = _first_env("ENGINE_REPO_TOKEN", "GH_TOKEN", "GITHUB_TOKEN") + logger.info(f"Cloning PR-review engine @ {ref}") + sha = _clone_at_ref(url, ref, dest, token) + return dest / ENGINE_SCRIPTS_SUBPATH, sha + + +def _ensure_powershell_yaml() -> None: + """Ensure the engine's YAML dependency is installed (Get-BCQualityConfig throws without it).""" + check = subprocess.run( + [_pwsh(), "-NoProfile", "-Command", "if (Get-Module -ListAvailable -Name powershell-yaml) { exit 0 } exit 1"], + capture_output=True, + text=True, + check=False, + ) + if check.returncode == 0: + return + logger.info("Installing powershell-yaml for the engine arm") + install = subprocess.run( + [ + _pwsh(), + "-NoProfile", + "-Command", + "Set-PSRepository PSGallery -InstallationPolicy Trusted; Install-Module powershell-yaml -Scope CurrentUser -Force", + ], + capture_output=True, + text=True, + check=False, + ) + if install.returncode != 0: + raise AgentError(f"Failed to install powershell-yaml: {install.stderr.strip() or install.stdout.strip()}") + + +def _run_local_review( + local_review_script: Path, + repo_path: Path, + bcquality_root: Path, + engine_output_dir: Path, + base_ref: str, + model: str, +) -> None: + # Invoke-LocalReview.ps1 filters the given BCQuality checkout and runs the production + # reviewer, reading GH_TOKEN from the inherited env for Copilot CLI auth. + args = [ + _pwsh(), + "-NoProfile", + "-File", + str(local_review_script), + "-RepoPath", + str(repo_path), + "-BaseRef", + base_ref, + "-BCQualityRoot", + str(bcquality_root), + "-OutputDir", + str(engine_output_dir), + ] + if model: + args += ["-Model", model] + # The engine's Copilot CLI authenticates via GH_TOKEN; bridge it from the tokens the + # generic workflow already exposes (COPILOT_GITHUB_TOKEN / GITHUB_TOKEN) so the arm + # needs no extra workflow env. + env = os.environ.copy() + if not env.get("GH_TOKEN"): + bridged = env.get("COPILOT_GITHUB_TOKEN") or env.get("GITHUB_TOKEN") + if bridged: + env["GH_TOKEN"] = bridged + # Clone-only credentials were already consumed by _prepare_engine/_prepare_bcquality; + # drop them so they are never inherited by the reviewer subprocess (only GH_TOKEN remains). + for clone_only in ("BCQUALITY_REPO_TOKEN", "ENGINE_REPO_TOKEN"): + env.pop(clone_only, None) + # Consume BCQuality as a Copilot CLI plugin (--plugin-dir) rather than a CWD checkout. + # The engine's own default is 'cwd'; a caller can still force cwd by presetting the env. + env.setdefault("BCQUALITY_CONSUME", "plugin") + logger.info(f"Invoking PR-review engine ({env['BCQUALITY_CONSUME']} mode): {local_review_script}") + try: + result = subprocess.run(args, capture_output=True, text=True, timeout=_ENGINE_TIMEOUT_SECONDS, check=False, env=env) + except subprocess.TimeoutExpired as exc: + raise AgentTimeoutError(f"PR-review engine timed out after {_ENGINE_TIMEOUT_SECONDS}s") from exc + if result.stdout: + logger.info(result.stdout) + if result.returncode != 0: + raise AgentError(f"PR-review engine failed (exit {result.returncode}): {result.stderr.strip() or result.stdout.strip()}") + + +def _map_severity(value: str) -> str: + """Map a finding's severity to the shared gold taxonomy (one table for every arm). + + Reuses Severity.from_input so blocker/major/minor/info and the canonical + critical/high/medium/low resolve exactly as the scorer parses gold; an unrecognized + value passes through lowercased instead of failing the run. + """ + try: + return Severity.from_input(value).value + except ValueError: + return value.strip().lower() + + +def _finding_to_comment(finding: dict) -> dict | None: + location = finding.get("location") or {} + file_path = location.get("file") + line_range = location.get("range") or {} + line_start = location.get("line") or line_range.get("start-line") + body = (finding.get("message") or "").strip() + if not file_path or not line_start or not body: + return None + comment: dict = {"file": file_path, "line_start": line_start, "body": body} + line_end = line_range.get("end-line") + if line_end: + comment["line_end"] = line_end + severity = finding.get("severity") + if severity: + comment["severity"] = _map_severity(str(severity)) + return comment + + +def _findings_to_review_comments(payload: dict) -> list[dict]: + comments: list[dict] = [] + for finding in payload.get("findings") or []: + if not isinstance(finding, dict): + continue + comment = _finding_to_comment(finding) + if comment is not None: + comments.append(comment) + return comments + + +def _loads_review_report(text: str) -> dict: + """Parse the engine report, tolerating shell-escaped single quotes. + + When a finding's ``suggested-code`` carries AL Label text emitted through a + single-quoted shell argument, the POSIX close/escape/reopen idiom ``'\\''`` + can leak into ``_review-report.json``. That 4-char sequence is invalid JSON + and breaks ``json.loads``. Collapsing it back to a bare ``'`` is safe (the + sequence can never appear in well-formed JSON). Recent engine builds already + normalize this on disk (microsoft/BC-ALAgents ``Repair-ShellEscapedQuotes``); + this guard keeps the arm robust when pinned to an older engine tag. + """ + try: + return json.loads(text) + except json.JSONDecodeError: + repaired = text.replace("'\\''", "'") + if repaired == text: + raise + return json.loads(repaired) + + +def _write_review_json(engine_output_dir: Path, repo_path: Path) -> int: + report_file = engine_output_dir / REVIEW_REPORT_FILE_NAME + if not report_file.exists(): + raise AgentError(f"Engine did not produce {REVIEW_REPORT_FILE_NAME} at {engine_output_dir}") + payload = _loads_review_report(report_file.read_text(encoding="utf-8")) + comments = _findings_to_review_comments(payload) + (repo_path / REVIEW_OUTPUT_FILE).write_text(json.dumps(comments, indent=2), encoding="utf-8") + logger.info(f"Wrote {len(comments)} review comment(s) to {repo_path / REVIEW_OUTPUT_FILE}") + return len(comments) + + +def _read_run_metrics(engine_output_dir: Path) -> dict: + """Read the engine's _run-metrics.json (token/timing stats), or {} if absent.""" + metrics_file = engine_output_dir / RUN_METRICS_FILE_NAME + if not metrics_file.exists(): + return {} + try: + return json.loads(metrics_file.read_text(encoding="utf-8")) + except (json.JSONDecodeError, OSError): + return {} + + +def run_engine_review( + entry: BaseDatasetEntry, + model: str, + repo_path: Path, + output_dir: Path, + engine_scripts_dir: Path | None = None, +) -> tuple[AgentMetrics | None, ExperimentConfiguration]: + logger.info(f"Running PR-review engine on: {entry.instance_id}") + + output_dir.mkdir(parents=True, exist_ok=True) + engine_output_dir = output_dir / "engine-output" + + engine_scripts_dir, engine_ref = _prepare_engine(output_dir, engine_scripts_dir) + local_review_script = engine_scripts_dir / LOCAL_REVIEW_SCRIPT_NAME + if not local_review_script.exists(): + raise AgentError(f"Engine script not found: {local_review_script}") + + _ensure_powershell_yaml() + bcquality_root, bcquality_sha = _prepare_bcquality(output_dir) + base_ref = _commit_patched_worktree(repo_path) + + started_at = time.monotonic() + _run_local_review(local_review_script, repo_path, bcquality_root, engine_output_dir, base_ref, model) + execution_time = time.monotonic() - started_at + + _write_review_json(engine_output_dir, repo_path) + + raw_metrics = _read_run_metrics(engine_output_dir) + metrics = AgentMetrics( + execution_time=execution_time, + prompt_tokens=raw_metrics.get("prompt_tokens"), + completion_tokens=raw_metrics.get("completion_tokens"), + ) + experiment = ExperimentConfiguration( + custom_agent="bc-pr-review-engine", + engine_ref=engine_ref, + bcquality_sha=bcquality_sha, + ) + return metrics, experiment diff --git a/src/bcbench/commands/evaluate.py b/src/bcbench/commands/evaluate.py index 231989dc7..72d32643b 100644 --- a/src/bcbench/commands/evaluate.py +++ b/src/bcbench/commands/evaluate.py @@ -6,7 +6,14 @@ import typer -from bcbench.agent import BCalBackendConfig, run_bcal_agent, run_claude_code, run_copilot_agent +from bcbench.agent import ( + BCalBackendConfig, + code_review_uses_engine, + run_bcal_agent, + run_claude_code, + run_copilot_agent, + run_engine_review, +) from bcbench.cli_options import ( ClaudeCodeModel, ContainerName, @@ -53,18 +60,32 @@ def evaluate_copilot( run_id: RunId = "copilot_test_run", al_mcp: Annotated[bool, typer.Option("--al-mcp", help="Enable AL MCP server")] = False, al_lsp: Annotated[bool, typer.Option("--al-lsp", help="Enable AL LSP server")] = False, + engine_scripts_dir: Annotated[ + Path | None, + typer.Option( + envvar="ENGINE_SCRIPTS_DIR", + help="Code-review only: use a local engine agent/scripts dir instead of cloning the pinned release. The engine arm is the default; set BCBENCH_CODE_REVIEW_AGENT=copilot to run plain Copilot. Set BCQUALITY_REF/BCQUALITY_ROOT to vary BCQuality.", + ), + ] = None, ) -> None: """ Evaluate GitHub Copilot CLI on single dataset entry. To only run the agent to generate a patch without building/testing, use 'bcbench run copilot' instead. """ + if engine_scripts_dir is not None and category != EvaluationCategory.CODE_REVIEW: + raise typer.BadParameter("--engine-scripts-dir routes through the PR-Review engine and only supports --category 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 GitHub Copilot CLI") - container = ContainerConfig(name=container_name, username=username, password=password) if container_name else None + # code-review defaults to the pinned PR-Review engine arm (BCBENCH_CODE_REVIEW_AGENT=copilot + # opts out); --engine-scripts-dir only overrides the engine's scripts path when it is active. + use_engine = category == EvaluationCategory.CODE_REVIEW and code_review_uses_engine() + agent_name = "BC PR-Review Engine" if use_engine else "GitHub Copilot" + + logger.info(f"Running evaluation on entry {entry_id} with {agent_name}") context = EvaluationContext( entry=entry, @@ -72,24 +93,35 @@ def evaluate_copilot( result_dir=run_dir, container=container, model=model, - agent_name="GitHub Copilot", + agent_name=agent_name, category=category, ) - pipeline = category.pipeline - pipeline.execute( - context, - lambda ctx: run_copilot_agent( - entry=ctx.entry, - repo_path=ctx.repo_path, - category=category, - model=ctx.model, - output_dir=ctx.result_dir, - al_mcp=al_mcp if ctx.container else False, - al_lsp=al_lsp, - container_name=ctx.get_container().name if ctx.container else "", - ), - ) + if use_engine: + context.category.pipeline.execute( + context, + lambda ctx: run_engine_review( + entry=ctx.entry, + model=ctx.model, + repo_path=ctx.repo_path, + output_dir=ctx.result_dir, + engine_scripts_dir=engine_scripts_dir, + ), + ) + else: + category.pipeline.execute( + context, + lambda ctx: run_copilot_agent( + entry=ctx.entry, + repo_path=ctx.repo_path, + category=category, + model=ctx.model, + output_dir=ctx.result_dir, + al_mcp=al_mcp if ctx.container else False, + al_lsp=al_lsp, + container_name=ctx.get_container().name if ctx.container else "", + ), + ) logger.info("Evaluation complete!") logger.info(f"Results saved to: {run_dir}") diff --git a/src/bcbench/dataset/codereview.py b/src/bcbench/dataset/codereview.py index 2048fff41..5c313736c 100644 --- a/src/bcbench/dataset/codereview.py +++ b/src/bcbench/dataset/codereview.py @@ -45,7 +45,9 @@ def from_input(cls, value: str) -> Severity: # findings score correctly instead of coercing to unspecified severity. "blocker": Severity.CRITICAL, "major": Severity.HIGH, - "minor": Severity.LOW, + # minor -> Medium matches the production engine's severity floor + # (Invoke-CopilotPRReview.ps1 $BCQualitySeverityMap); one table for every arm. + "minor": Severity.MEDIUM, } diff --git a/src/bcbench/types.py b/src/bcbench/types.py index da7d32823..8f7611e79 100644 --- a/src/bcbench/types.py +++ b/src/bcbench/types.py @@ -107,13 +107,28 @@ class ExperimentConfiguration(BaseModel): # Plugins loaded for this experiment: "@" (github) or "@local" plugins: list[str] | None = None + # PR-Review engine arm: resolved microsoft/BC-ALAgents engine commit + engine_ref: str | None = None + + # PR-Review engine arm: resolved BCQuality content commit reviewed against + bcquality_sha: str | None = None + def is_empty(self) -> bool: """Check if this configuration has all default/empty values. An empty configuration means no special experiment settings were used. This is useful for comparing with None (no experiment) vs default experiment. """ - return self.mcp_servers is None and self.al_lsp_enabled is False and self.custom_instructions is False and self.skills_enabled is False and self.custom_agent is None and self.plugins is None + return ( + self.mcp_servers is None + and self.al_lsp_enabled is False + and self.custom_instructions is False + and self.skills_enabled is False + and self.custom_agent is None + and self.plugins is None + and self.engine_ref is None + and self.bcquality_sha is None + ) # Where an agent plugin comes from: local, or cloned from GitHub diff --git a/tests/test_codereview.py b/tests/test_codereview.py index 7f8e81fe9..adc845fd2 100644 --- a/tests/test_codereview.py +++ b/tests/test_codereview.py @@ -32,7 +32,7 @@ def test_aliases_map_to_canonical_severities(self): # BCQuality do.md severities emitted by the production engine. assert Severity.from_input("blocker") is Severity.CRITICAL assert Severity.from_input("major") is Severity.HIGH - assert Severity.from_input("minor") is Severity.LOW + assert Severity.from_input("minor") is Severity.MEDIUM def test_unknown_severity_raises(self): with pytest.raises(ValueError, match="Unknown severity"): diff --git a/tests/test_engine.py b/tests/test_engine.py new file mode 100644 index 000000000..f048dfd81 --- /dev/null +++ b/tests/test_engine.py @@ -0,0 +1,276 @@ +import json +import subprocess +from unittest.mock import patch + +import pytest + +from bcbench.agent.engine import ( + _bcquality_repo_url, + _finding_to_comment, + _findings_to_review_comments, + _map_severity, + _prepare_engine, + _read_run_metrics, + _run_local_review, + _write_review_json, + code_review_uses_engine, + run_engine_review, +) +from bcbench.exceptions import AgentError +from tests.conftest import create_codereview_entry + + +class TestEngineFindingMapping: + """Normalization of the engine's _review-report.json findings into review comments.""" + + def test_maps_engine_severity_to_gold_taxonomy(self): + findings = [{"severity": s, "message": "x", "location": {"file": "src/A.al", "line": 3}} for s in ("blocker", "major", "minor", "info")] + got = [c["severity"] for c in _findings_to_review_comments({"findings": findings})] + assert got == ["critical", "high", "medium", "low"] + + def test_uses_location_line_and_range_end(self): + finding = {"message": "x", "location": {"file": "src/A.al", "line": 14, "range": {"start-line": 14, "end-line": 17}}} + comment = _finding_to_comment(finding) + assert comment == {"file": "src/A.al", "line_start": 14, "line_end": 17, "body": "x"} + + def test_falls_back_to_range_start_line_when_line_missing(self): + finding = {"message": "x", "location": {"file": "src/A.al", "range": {"start-line": 9}}} + comment = _finding_to_comment(finding) + assert comment is not None + assert comment["line_start"] == 9 + + def test_skips_findings_missing_file_line_or_message(self): + findings = [ + {"message": "no location", "location": {}}, + {"location": {"file": "src/A.al", "line": 2}}, + {"message": " ", "location": {"file": "src/A.al", "line": 3}}, + ] + assert _findings_to_review_comments({"findings": findings}) == [] + + def test_unknown_severity_passes_through_lowercased(self): + finding = {"severity": "Weird", "message": "x", "location": {"file": "src/A.al", "line": 1}} + comment = _finding_to_comment(finding) + assert comment is not None + assert comment["severity"] == "weird" + + +class TestBcqualityRepoOverride: + def test_defaults_to_upstream(self, monkeypatch): + monkeypatch.delenv("BCQUALITY_REPO", raising=False) + assert _bcquality_repo_url() == "https://github.com/microsoft/BCQuality.git" + + def test_full_url_passthrough(self, monkeypatch): + monkeypatch.setenv("BCQUALITY_REPO", "https://github.com/fork/BCQuality.git") + assert _bcquality_repo_url() == "https://github.com/fork/BCQuality.git" + + def test_owner_repo_shorthand_expands(self, monkeypatch): + monkeypatch.setenv("BCQUALITY_REPO", "fork/BCQuality") + assert _bcquality_repo_url() == "https://github.com/fork/BCQuality.git" + + +class TestRunMetrics: + def test_missing_file_returns_empty(self, tmp_path): + assert _read_run_metrics(tmp_path) == {} + + def test_malformed_json_returns_empty(self, tmp_path): + (tmp_path / "_run-metrics.json").write_text("{not json", encoding="utf-8") + assert _read_run_metrics(tmp_path) == {} + + def test_reads_token_counts(self, tmp_path): + (tmp_path / "_run-metrics.json").write_text(json.dumps({"prompt_tokens": 5, "completion_tokens": 2}), encoding="utf-8") + assert _read_run_metrics(tmp_path) == {"prompt_tokens": 5, "completion_tokens": 2} + + +class TestWriteReviewJson: + def test_raises_when_report_missing(self, tmp_path): + (tmp_path / "repo").mkdir() + with pytest.raises(AgentError): + _write_review_json(tmp_path, tmp_path / "repo") + + def test_repairs_shell_escaped_single_quotes(self, tmp_path): + repo = tmp_path / "repo" + repo.mkdir() + inner = "x" + "'\\''" + "y" # x'\''y -- the POSIX close/escape/reopen idiom + raw = '{"findings": [{"message": "m", "location": {"file": "src/A.al", "line": 5}, "suggested-code": "' + inner + '"}]}' + with pytest.raises(json.JSONDecodeError): + json.loads(raw) # the corruption is genuinely invalid JSON + (tmp_path / "_review-report.json").write_text(raw, encoding="utf-8") + count = _write_review_json(tmp_path, repo) + assert count == 1 + written = json.loads((repo / "review.json").read_text(encoding="utf-8")) + assert written == [{"file": "src/A.al", "line_start": 5, "body": "m"}] + + def test_unrepairable_json_still_raises(self, tmp_path): + repo = tmp_path / "repo" + repo.mkdir() + (tmp_path / "_review-report.json").write_text("{not json", encoding="utf-8") + with pytest.raises(json.JSONDecodeError): + _write_review_json(tmp_path, repo) + + +class TestRunEngineReview: + def _script_dir(self, tmp_path): + scripts = tmp_path / "scripts" + scripts.mkdir() + (scripts / "Invoke-LocalReview.ps1").write_text("# stub", encoding="utf-8") + return scripts + + def test_missing_script_raises(self, tmp_path): + entry = create_codereview_entry() + with pytest.raises(AgentError): + run_engine_review( + entry=entry, + model="claude-haiku-4.5", + repo_path=tmp_path / "repo", + output_dir=tmp_path / "out", + engine_scripts_dir=tmp_path / "missing-scripts", + ) + + def test_orchestrates_and_maps_report_metrics_and_versions(self, tmp_path): + entry = create_codereview_entry() + repo_path = tmp_path / "repo" + repo_path.mkdir() + output_dir = tmp_path / "out" + scripts = self._script_dir(tmp_path) + + def fake_run_local_review(local_review_script, repo, bcquality_root, engine_output_dir, base_ref, model): + engine_output_dir.mkdir(parents=True, exist_ok=True) + (engine_output_dir / "_review-report.json").write_text( + json.dumps({"findings": [{"severity": "major", "message": "boom", "location": {"file": "src/A.al", "line": 5}}]}), + encoding="utf-8", + ) + (engine_output_dir / "_run-metrics.json").write_text( + json.dumps({"prompt_tokens": 111, "completion_tokens": 22, "total_tokens": 133}), + encoding="utf-8", + ) + + with ( + patch("bcbench.agent.engine._ensure_powershell_yaml"), + patch("bcbench.agent.engine._prepare_bcquality", return_value=(tmp_path / "bcq", "bcqsha")), + patch("bcbench.agent.engine._commit_patched_worktree", return_value="base"), + patch("bcbench.agent.engine._run_local_review", side_effect=fake_run_local_review) as run_mock, + patch("bcbench.agent.engine._git_head", return_value="engsha"), + ): + metrics, experiment = run_engine_review( + entry=entry, + model="claude-haiku-4.5", + repo_path=repo_path, + output_dir=output_dir, + engine_scripts_dir=scripts, + ) + + run_mock.assert_called_once() + assert json.loads((repo_path / "review.json").read_text(encoding="utf-8")) == [{"file": "src/A.al", "line_start": 5, "body": "boom", "severity": "high"}] + assert metrics is not None + assert metrics.prompt_tokens == 111 + assert metrics.completion_tokens == 22 + assert metrics.execution_time is not None + assert metrics.execution_time >= 0 + assert experiment.custom_agent == "bc-pr-review-engine" + assert experiment.engine_ref == "engsha" + assert experiment.bcquality_sha == "bcqsha" + assert experiment.is_empty() is False + + +class TestMapSeverity: + """Every arm resolves severity through the one shared alias table (bcbench.dataset.codereview).""" + + def test_reuses_shared_alias_table(self): + assert [_map_severity(s) for s in ("blocker", "major", "minor", "info")] == ["critical", "high", "medium", "low"] + + def test_canonical_is_idempotent(self): + assert _map_severity("Medium") == "medium" + + def test_unknown_passes_through_lowercased(self): + assert _map_severity("Weird") == "weird" + + +class TestCodeReviewUsesEngine: + def test_defaults_to_engine(self, monkeypatch): + monkeypatch.delenv("BCBENCH_CODE_REVIEW_AGENT", raising=False) + assert code_review_uses_engine() is True + + def test_copilot_opts_out(self, monkeypatch): + monkeypatch.setenv("BCBENCH_CODE_REVIEW_AGENT", "copilot") + assert code_review_uses_engine() is False + + +class TestPrepareEngine: + def test_defaults_to_pinned_release(self, tmp_path, monkeypatch): + monkeypatch.delenv("ENGINE_REF", raising=False) + with patch("bcbench.agent.engine._clone_at_ref", return_value="sha") as clone: + _, sha = _prepare_engine(tmp_path, None) + assert clone.call_args.args[1] == "1.20.4" + assert sha == "sha" + + def test_env_ref_overrides_pin(self, tmp_path, monkeypatch): + monkeypatch.setenv("ENGINE_REF", "1.14.4") + with patch("bcbench.agent.engine._clone_at_ref", return_value="sha") as clone: + _prepare_engine(tmp_path, None) + assert clone.call_args.args[1] == "1.14.4" + + def test_local_scripts_dir_skips_clone(self, tmp_path): + with ( + patch("bcbench.agent.engine._clone_at_ref") as clone, + patch("bcbench.agent.engine._git_head", return_value="localsha"), + ): + scripts, sha = _prepare_engine(tmp_path, tmp_path / "local-scripts") + clone.assert_not_called() + assert scripts == tmp_path / "local-scripts" + assert sha == "localsha" + + +class TestRunLocalReviewEnv: + def test_sanitizes_clone_tokens_and_bridges_gh_token(self, tmp_path, monkeypatch): + monkeypatch.setenv("BCQUALITY_REPO_TOKEN", "bcq-secret") + monkeypatch.setenv("ENGINE_REPO_TOKEN", "eng-secret") + monkeypatch.delenv("GH_TOKEN", raising=False) + monkeypatch.setenv("GITHUB_TOKEN", "gh-secret") + captured: dict = {} + + def fake_run(args, **kwargs): + captured["env"] = kwargs["env"] + return subprocess.CompletedProcess(args, 0, stdout="", stderr="") + + with ( + patch("bcbench.agent.engine._pwsh", return_value="pwsh"), + patch("bcbench.agent.engine.subprocess.run", side_effect=fake_run), + ): + _run_local_review(tmp_path / "s.ps1", tmp_path, tmp_path, tmp_path / "out", "base", "claude-haiku-4.5") + + env = captured["env"] + assert "BCQUALITY_REPO_TOKEN" not in env + assert "ENGINE_REPO_TOKEN" not in env + assert env["GH_TOKEN"] == "gh-secret" + + def test_defaults_to_plugin_consumption(self, tmp_path, monkeypatch): + monkeypatch.delenv("BCQUALITY_CONSUME", raising=False) + captured: dict = {} + + def fake_run(args, **kwargs): + captured["env"] = kwargs["env"] + return subprocess.CompletedProcess(args, 0, stdout="", stderr="") + + with ( + patch("bcbench.agent.engine._pwsh", return_value="pwsh"), + patch("bcbench.agent.engine.subprocess.run", side_effect=fake_run), + ): + _run_local_review(tmp_path / "s.ps1", tmp_path, tmp_path, tmp_path / "out", "base", "claude-haiku-4.5") + + assert captured["env"]["BCQUALITY_CONSUME"] == "plugin" + + def test_honors_bcquality_consume_override(self, tmp_path, monkeypatch): + monkeypatch.setenv("BCQUALITY_CONSUME", "cwd") + captured: dict = {} + + def fake_run(args, **kwargs): + captured["env"] = kwargs["env"] + return subprocess.CompletedProcess(args, 0, stdout="", stderr="") + + with ( + patch("bcbench.agent.engine._pwsh", return_value="pwsh"), + patch("bcbench.agent.engine.subprocess.run", side_effect=fake_run), + ): + _run_local_review(tmp_path / "s.ps1", tmp_path, tmp_path, tmp_path / "out", "base", "claude-haiku-4.5") + + assert captured["env"]["BCQUALITY_CONSUME"] == "cwd"