From 8f596a421d4b559924f3bb0490d17faf9730c3a8 Mon Sep 17 00:00:00 2001 From: Johannes du Plessis Date: Wed, 10 Jun 2026 09:54:32 -0700 Subject: [PATCH] feat: prep reviewer repo at init and load repo skills (#1480) * feat: prep reviewer repo at init and load repo skills Clone + checkout the PR head during reviewer agent init so SkillsMiddleware can discover the repo's .agents/skills and .claude/skills from disk at its one-shot scan, and so the LLM no longer narrates the clone mid-run. Co-authored-by: open-swe[bot] * fix: load reviewer skills from trusted base sha and fetch PR pull ref Address review findings: skills are now extracted from the PR base sha via git archive into a dir outside the checkout (prevents PR-authored SKILL.md prompt injection), and repo prep fetches refs/pull//head with a strict checkout so fork PRs fail loudly instead of silently reviewing the default branch. * fix: drop ref from skill-extraction log to satisfy CodeQL --------- Co-authored-by: open-swe[bot] --- agent/reviewer.py | 29 ++++++- agent/utils/repo_prep.py | 160 +++++++++++++++++++++++++++++++++++++++ tests/test_repo_prep.py | 143 ++++++++++++++++++++++++++++++++++ 3 files changed, 331 insertions(+), 1 deletion(-) create mode 100644 agent/utils/repo_prep.py create mode 100644 tests/test_repo_prep.py diff --git a/agent/reviewer.py b/agent/reviewer.py index 2730ac35..eb61b1e9 100644 --- a/agent/reviewer.py +++ b/agent/reviewer.py @@ -69,6 +69,7 @@ from .utils.api_standards_skill import fetch_api_standards_skill from .utils.github_app import get_github_app_installation_token_with_expiry from .utils.github_token import cache_github_token_for_thread from .utils.model import DEFAULT_LLM_REASONING, make_model, provider_model_kwargs +from .utils.repo_prep import materialize_trusted_skills, prepare_review_repo from .utils.sandbox_paths import aresolve_sandbox_work_dir REVIEWER_PROMPT_TEMPLATE = """You are a specialized code reviewer agent. Your job is to review one GitHub PR and publish a single review. @@ -87,12 +88,17 @@ Re-review (user message says "A new commit has been pushed"): GH_TOKEN=dummy gh api repos/{repo_owner}/{repo_name}/compare/... -H "Accept: application/vnd.github.v3.diff" ``` -Clone the repo so you can grep for full file context: +The repo is normally already cloned and checked out at the PR head in +`{working_dir}` — `cd` there and grep for full file context. If that directory +is missing (prep can occasionally fail), clone it yourself: ``` GH_TOKEN=dummy gh repo clone {repo_owner}/{repo_name} && cd {repo_name} && git checkout ``` +If a skills section appears below, the repo ships reviewer-relevant skills. Read +the `SKILL.md` that matches the area you're reviewing and apply it. + Tools: `add_finding`, `update_finding`, `list_findings`, `publish_review`, `resolve_finding_thread`, `reply_to_finding_thread`. Call `publish_review` once at the end. @@ -697,6 +703,26 @@ async def get_reviewer_agent(config: RunnableConfig) -> Pregel: base_sha = str(config["configurable"].get("base_sha", "") or "") head_sha = str(config["configurable"].get("head_sha", "") or "") pr_number = config["configurable"].get("pr_number") + + # Prep the repo on the sandbox before the first model call so the LLM does + # not narrate `gh repo clone`, and so SkillsMiddleware can discover the + # repo's skills at its one-shot scan. Skills are materialized from the PR + # base sha (trusted), never the PR head (author-controlled). + repo_ready = await prepare_review_repo( + sandbox_backend, + work_dir=work_dir, + repo_owner=repo_owner, + repo_name=repo_name, + head_sha=head_sha, + pr_number=pr_number if isinstance(pr_number, int) else None, + base_sha=base_sha, + ) + skill_sources: list[str] = [] + if repo_ready and repo_name: + skill_sources = await materialize_trusted_skills( + sandbox_backend, repo_dir=f"{work_dir}/{repo_name}", trusted_ref=base_sha + ) + pr_url = str(config["configurable"].get("pr_url", "") or "") last_reviewed_sha = str(config["configurable"].get("last_reviewed_sha", "") or "") is_re_review = bool(config["configurable"].get("re_review")) @@ -945,6 +971,7 @@ async def get_reviewer_agent(config: RunnableConfig) -> Pregel: ], subagents=[_general_purpose_subagent(reviewer_subagent_model)], backend=sandbox_backend, + skills=skill_sources or None, middleware=[ SanitizeToolInputsMiddleware(), ModelCallLimitMiddleware(run_limit=MODEL_CALL_RECURSION_LIMIT, exit_behavior="end"), diff --git a/agent/utils/repo_prep.py b/agent/utils/repo_prep.py new file mode 100644 index 00000000..72dad9c3 --- /dev/null +++ b/agent/utils/repo_prep.py @@ -0,0 +1,160 @@ +"""Deterministic repo prep for the reviewer sandbox. + +The reviewer reviews a single PR, so we clone its repo and check out the PR +head during agent init -- before the first model call -- instead of asking the +LLM to narrate ``gh repo clone`` mid-run. Pre-cloning also lets ``SkillsMiddleware`` +discover the repo's ``.agents/skills`` / ``.claude/skills`` at its one-shot +``before_agent`` scan. + +Best-effort: any failure leaves the sandbox usable (the review still works off +the fetched diff) and returns ``False`` so callers can skip skill wiring. +""" + +from __future__ import annotations + +import asyncio +import logging +import posixpath +import shlex +from collections.abc import Sequence + +from deepagents.backends.protocol import SandboxBackendProtocol + +logger = logging.getLogger(__name__) + +CLONE_TIMEOUT_SECONDS = 240 + +DEFAULT_SKILL_DIRS = (".agents/skills", ".claude/skills") + +TRUSTED_SKILLS_DIRNAME = ".review-skills" + + +def _prep_command( + work_dir: str, + repo_owner: str, + repo_name: str, + head_sha: str, + pr_number: int | None, + base_sha: str, +) -> str: + repo_dir = posixpath.join(work_dir, repo_name) + q_work_dir = shlex.quote(work_dir) + q_repo_dir = shlex.quote(repo_dir) + q_full_name = shlex.quote(f"{repo_owner}/{repo_name}") + q_repo_name = shlex.quote(repo_name) + q_head = shlex.quote(head_sha) if head_sha else "" + + lines = [ + "set -e", + f"if [ -d {q_repo_dir}/.git ]; then", + f" cd {q_repo_dir} && GH_TOKEN=dummy git fetch --all --quiet", + "else", + f" cd {q_work_dir} && GH_TOKEN=dummy gh repo clone {q_full_name} && cd {q_repo_name}", + "fi", + ] + if base_sha: + q_base = shlex.quote(base_sha) + lines.append(f"GH_TOKEN=dummy git fetch origin {q_base} --quiet 2>/dev/null || true") + if q_head: + # Direct sha fetch covers same-repo PRs; the pull ref covers fork PRs + # whose head commit is not reachable from origin's branches. + lines.append(f"GH_TOKEN=dummy git fetch origin {q_head} --quiet 2>/dev/null || true") + if pr_number is not None: + pull_ref = shlex.quote(f"refs/pull/{pr_number}/head") + lines.append(f"GH_TOKEN=dummy git fetch origin {pull_ref} --quiet 2>/dev/null || true") + # Strict on purpose: a failed checkout must fail the prep so callers + # know the tree is NOT at the PR head. + lines.append(f"git checkout {q_head} --quiet") + return "\n".join(lines) + + +async def prepare_review_repo( + sandbox_backend: SandboxBackendProtocol, + *, + work_dir: str, + repo_owner: str, + repo_name: str, + head_sha: str, + pr_number: int | None = None, + base_sha: str = "", +) -> bool: + """Clone-or-fetch the repo and check out ``head_sha`` in the sandbox. + + Returns ``True`` only when the repo is prepped at ``work_dir/repo_name`` + and (when ``head_sha`` is given) actually checked out at the PR head. + """ + if not repo_owner or not repo_name: + return False + + command = _prep_command(work_dir, repo_owner, repo_name, head_sha, pr_number, base_sha) + try: + result = await asyncio.to_thread( + sandbox_backend.execute, command, timeout=CLONE_TIMEOUT_SECONDS + ) + except Exception: # noqa: BLE001 + logger.warning("Failed to prep review repo %s/%s", repo_owner, repo_name, exc_info=True) + return False + + exit_code = getattr(result, "exit_code", None) + if exit_code not in (0, None): + logger.warning( + "Review repo prep for %s/%s exited %s: %s", + repo_owner, + repo_name, + exit_code, + getattr(result, "output", ""), + ) + return False + + logger.info( + "Prepped review repo %s/%s at %s (head=%s)", + repo_owner, + repo_name, + posixpath.join(work_dir, repo_name), + head_sha or "", + ) + return True + + +async def materialize_trusted_skills( + sandbox_backend: SandboxBackendProtocol, + *, + repo_dir: str, + trusted_ref: str, + skill_dirs: Sequence[str] = DEFAULT_SKILL_DIRS, +) -> list[str]: + """Extract skill dirs from ``trusted_ref`` into a path outside the checkout. + + Skills are sourced from the PR's base sha -- never the PR head, which the + PR author controls -- so a PR cannot inject instructions into the reviewer + prompt by adding or editing a ``SKILL.md``. Returns the extracted source + dirs (with a trailing slash, as SkillsMiddleware expects). + """ + if not trusted_ref: + return [] + dest_root = posixpath.join(posixpath.dirname(repo_dir), TRUSTED_SKILLS_DIRNAME) + q_repo_dir = shlex.quote(repo_dir) + q_ref = shlex.quote(trusted_ref) + + sources: list[str] = [] + for skill_dir in skill_dirs: + dest = posixpath.join(dest_root, skill_dir) + q_dest = shlex.quote(dest) + q_dir = shlex.quote(skill_dir) + depth = len(skill_dir.split("/")) + command = ( + f"cd {q_repo_dir} && " + f"git cat-file -e {q_ref}:{q_dir} 2>/dev/null && " + f"rm -rf {q_dest} && mkdir -p {q_dest} && " + f"git archive {q_ref} {q_dir} | tar -x --strip-components={depth} -C {q_dest} && " + f"echo {q_dest}" + ) + try: + result = await asyncio.to_thread(sandbox_backend.execute, command) + except Exception: # noqa: BLE001 + logger.warning("Failed to extract trusted skills %s", skill_dir, exc_info=True) + continue + output = getattr(result, "output", "") or "" + if dest in output.splitlines(): + sources.append(f"{dest}/") + return sources diff --git a/tests/test_repo_prep.py b/tests/test_repo_prep.py new file mode 100644 index 00000000..b588d296 --- /dev/null +++ b/tests/test_repo_prep.py @@ -0,0 +1,143 @@ +from __future__ import annotations + +from deepagents.backends.protocol import ExecuteResponse + +from agent.utils.repo_prep import materialize_trusted_skills, prepare_review_repo + + +class _FakeSandboxBackend: + def __init__( + self, + *, + exit_code: int = 0, + raise_exc: bool = False, + output: str = "", + outputs: list[str] | None = None, + ) -> None: + self._exit_code = exit_code + self._raise = raise_exc + self._output = output + self._outputs = outputs + self.commands: list[str] = [] + + @property + def id(self) -> str: + return "fake-sandbox" + + def execute(self, command: str, *, timeout: int | None = None) -> ExecuteResponse: + del timeout + if self._raise: + raise RuntimeError("sandbox unreachable") + self.commands.append(command) + output = self._output + if self._outputs is not None: + output = self._outputs[len(self.commands) - 1] + return ExecuteResponse(output=output, exit_code=self._exit_code, truncated=False) + + +async def test_prepare_review_repo_clones_and_checks_out_head() -> None: + backend = _FakeSandboxBackend() + ok = await prepare_review_repo( + backend, + work_dir="/work", + repo_owner="acme", + repo_name="widget", + head_sha="abc123", + pr_number=42, + base_sha="def456", + ) + assert ok is True + assert len(backend.commands) == 1 + cmd = backend.commands[0] + assert "gh repo clone acme/widget" in cmd + assert "/work/widget/.git" in cmd + assert "git fetch origin def456" in cmd + assert "git fetch origin refs/pull/42/head" in cmd + assert "git checkout abc123 --quiet" in cmd + assert "git checkout abc123 --quiet 2>/dev/null || true" not in cmd + + +async def test_prepare_review_repo_skips_pull_ref_without_pr_number() -> None: + backend = _FakeSandboxBackend() + ok = await prepare_review_repo( + backend, + work_dir="/work", + repo_owner="acme", + repo_name="widget", + head_sha="abc123", + ) + assert ok is True + assert "refs/pull" not in backend.commands[0] + + +async def test_prepare_review_repo_skips_checkout_without_head() -> None: + backend = _FakeSandboxBackend() + ok = await prepare_review_repo( + backend, + work_dir="/work", + repo_owner="acme", + repo_name="widget", + head_sha="", + ) + assert ok is True + assert "git checkout" not in backend.commands[0] + + +async def test_prepare_review_repo_requires_owner_and_name() -> None: + backend = _FakeSandboxBackend() + ok = await prepare_review_repo( + backend, work_dir="/work", repo_owner="", repo_name="widget", head_sha="abc" + ) + assert ok is False + assert backend.commands == [] + + +async def test_prepare_review_repo_returns_false_on_nonzero_exit() -> None: + backend = _FakeSandboxBackend(exit_code=1) + ok = await prepare_review_repo( + backend, work_dir="/work", repo_owner="acme", repo_name="widget", head_sha="abc" + ) + assert ok is False + + +async def test_prepare_review_repo_returns_false_on_exception() -> None: + backend = _FakeSandboxBackend(raise_exc=True) + ok = await prepare_review_repo( + backend, work_dir="/work", repo_owner="acme", repo_name="widget", head_sha="abc" + ) + assert ok is False + + +async def test_materialize_trusted_skills_extracts_from_trusted_ref() -> None: + backend = _FakeSandboxBackend(outputs=["/work/.review-skills/.agents/skills\n", ""]) + sources = await materialize_trusted_skills( + backend, repo_dir="/work/widget", trusted_ref="def456" + ) + assert sources == ["/work/.review-skills/.agents/skills/"] + assert len(backend.commands) == 2 + for cmd in backend.commands: + assert "git cat-file -e def456:" in cmd + assert "git archive def456" in cmd + + +async def test_materialize_trusted_skills_empty_without_ref() -> None: + backend = _FakeSandboxBackend() + sources = await materialize_trusted_skills(backend, repo_dir="/work/widget", trusted_ref="") + assert sources == [] + assert backend.commands == [] + + +async def test_materialize_trusted_skills_empty_when_none_exist() -> None: + backend = _FakeSandboxBackend(output="") + sources = await materialize_trusted_skills( + backend, repo_dir="/work/widget", trusted_ref="def456" + ) + assert sources == [] + + +async def test_materialize_trusted_skills_handles_exception() -> None: + backend = _FakeSandboxBackend(raise_exc=True) + sources = await materialize_trusted_skills( + backend, repo_dir="/work/widget", trusted_ref="def456" + ) + assert sources == []