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] <open-swe@users.noreply.github.com>

* 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/<n>/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] <open-swe@users.noreply.github.com>

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/<last_reviewed_sha>...<head_sha> -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 <head_sha>
 ```
 
+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"),

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 "<none>",
+    )
+    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

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 == []