diff --git a/agent/reviewer.py b/agent/reviewer.py index e3df0d0f..41784165 100644 --- a/agent/reviewer.py +++ b/agent/reviewer.py @@ -17,6 +17,7 @@ agent for code review only: import asyncio import logging +import posixpath import re import warnings @@ -90,13 +91,7 @@ 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" ``` -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 -``` +{repo_checkout_note} 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. @@ -294,12 +289,55 @@ The dataset expects 1-5 comments per PR (mean ~2). """ +_REPO_READY_NOTE = """The repo is already cloned and checked out at the PR head in +`{working_dir}` — `cd` there and grep for full file context.""" + +_REPO_NOT_READY_NOTE = """Repo prep FAILED: the checkout in `{working_dir}` may be missing or — worse — +present but stale (at an old commit). Do NOT trust local files until you have +re-prepped the tree yourself. Run: + +``` +cd {working_dir} || {{ cd {parent_dir} && GH_TOKEN=dummy gh repo clone {repo_owner}/{repo_name} && cd {repo_name}; }} +GH_TOKEN=dummy git fetch origin {head_sha_or_placeholder} --quiet || GH_TOKEN=dummy git fetch origin refs/pull/{pr_number}/head --quiet +git checkout --force {head_sha_or_placeholder} --quiet +``` + +and verify `git rev-parse HEAD` matches the PR head before reading local +files. If you cannot get the tree onto the PR head, rely exclusively on the +diff and `gh api` file contents (`GH_TOKEN=dummy gh api +repos/{repo_owner}/{repo_name}/contents/?ref=`) — never on +the local checkout.""" + + +def _repo_checkout_note( + *, + repo_ready: bool, + working_dir: str, + repo_owner: str, + repo_name: str, + pr_number: int | str, + head_sha: str, +) -> str: + if repo_ready: + return _REPO_READY_NOTE.format(working_dir=working_dir) + return _REPO_NOT_READY_NOTE.format( + working_dir=working_dir, + parent_dir=posixpath.dirname(working_dir) or working_dir, + repo_owner=repo_owner or "", + repo_name=repo_name or "", + pr_number=pr_number if pr_number != "" else "", + head_sha_or_placeholder=head_sha or "", + ) + + def _reviewer_system_prompt( working_dir: str, *, repo_owner: str, repo_name: str, pr_number: int | str, + repo_ready: bool = True, + head_sha: str = "", reviewer_eval: bool = False, org_guidelines: str | None = None, repo_style_prompt: str | None = None, @@ -311,6 +349,14 @@ def _reviewer_system_prompt( repo_owner=repo_owner or "", repo_name=repo_name or "", pr_number=pr_number if pr_number != "" else "", + repo_checkout_note=_repo_checkout_note( + repo_ready=repo_ready, + working_dir=working_dir, + repo_owner=repo_owner, + repo_name=repo_name, + pr_number=pr_number, + head_sha=head_sha, + ), ) if reviewer_eval: prompt = f"{prompt}\n{REVIEWER_EVAL_PROMPT_SUFFIX}" @@ -965,6 +1011,8 @@ async def get_reviewer_agent(config: RunnableConfig) -> Pregel: repo_owner=repo_owner, repo_name=repo_name, pr_number=pr_number if isinstance(pr_number, int) else "", + repo_ready=repo_ready, + head_sha=head_sha, reviewer_eval=reviewer_eval, org_guidelines=org_guidelines, repo_style_prompt=repo_style_prompt, diff --git a/agent/utils/repo_prep.py b/agent/utils/repo_prep.py index 72dad9c3..0c5e26b4 100644 --- a/agent/utils/repo_prep.py +++ b/agent/utils/repo_prep.py @@ -47,7 +47,9 @@ def _prep_command( lines = [ "set -e", f"if [ -d {q_repo_dir}/.git ]; then", - f" cd {q_repo_dir} && GH_TOKEN=dummy git fetch --all --quiet", + # Tolerate fetch-all failures: the targeted head/base fetches below + # are what the checkout actually needs. + f" cd {q_repo_dir} && {{ GH_TOKEN=dummy git fetch --all --quiet || true; }}", "else", f" cd {q_work_dir} && GH_TOKEN=dummy gh repo clone {q_full_name} && cd {q_repo_name}", "fi", @@ -62,9 +64,12 @@ def _prep_command( 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") + # --force: a reused sandbox can have a dirty worktree from a previous + # run, which would otherwise block the checkout and silently leave the + # tree at the old head. 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 --force {q_head} --quiet") + lines.append(f'[ "$(git rev-parse HEAD)" = {q_head} ]') return "\n".join(lines) diff --git a/tests/test_repo_prep.py b/tests/test_repo_prep.py index b588d296..5b343aee 100644 --- a/tests/test_repo_prep.py +++ b/tests/test_repo_prep.py @@ -53,8 +53,10 @@ async def test_prepare_review_repo_clones_and_checks_out_head() -> None: 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 + assert "git checkout --force abc123 --quiet" in cmd + assert "git checkout --force abc123 --quiet 2>/dev/null || true" not in cmd + assert '[ "$(git rev-parse HEAD)" = abc123 ]' in cmd + assert "git fetch --all --quiet || true" in cmd async def test_prepare_review_repo_skips_pull_ref_without_pr_number() -> None: diff --git a/tests/test_reviewer.py b/tests/test_reviewer.py index 5d92be9e..7afca801 100644 --- a/tests/test_reviewer.py +++ b/tests/test_reviewer.py @@ -24,6 +24,45 @@ def test_reviewer_system_prompt_formats_without_keyerror() -> None: assert "at least 1 finding" not in prompt.lower() +def test_reviewer_system_prompt_repo_ready_note() -> None: + prompt = reviewer._reviewer_system_prompt( + "/workspace/repo", + repo_owner="acme", + repo_name="repo", + pr_number=42, + repo_ready=True, + ) + assert "already cloned and checked out at the PR head" in prompt + assert "Repo prep FAILED" not in prompt + + +def test_reviewer_system_prompt_repo_not_ready_warns_stale() -> None: + prompt = reviewer._reviewer_system_prompt( + "/workspace/repo", + repo_owner="acme", + repo_name="repo", + pr_number=42, + repo_ready=False, + head_sha="abc123", + ) + assert "Repo prep FAILED" in prompt + assert "stale" in prompt + assert "git checkout --force abc123" in prompt + assert "git rev-parse HEAD" in prompt + assert "already cloned and checked out at the PR head" not in prompt + + +def test_reviewer_system_prompt_repo_not_ready_without_head_sha() -> None: + prompt = reviewer._reviewer_system_prompt( + "/workspace/repo", + repo_owner="acme", + repo_name="repo", + pr_number=42, + repo_ready=False, + ) + assert "git checkout --force " in prompt + + def test_reviewer_system_prompt_includes_repo_style_section() -> None: prompt = reviewer._reviewer_system_prompt( "/workspace/repo",