diff --git a/src/pr_af/app.py b/src/pr_af/app.py index 2c05c9c..b112704 100644 --- a/src/pr_af/app.py +++ b/src/pr_af/app.py @@ -61,18 +61,34 @@ def _extract_pr_number(pr_url: str) -> int | None: def _checkout_pr_branch(target_dir: str, pr_number: int) -> None: git_env = {**os.environ, "GIT_TERMINAL_PROMPT": "0", "GIT_ASKPASS": "echo"} - subprocess.run( - ["git", "-C", target_dir, "fetch", "--depth", "1", "origin", f"pull/{pr_number}/head:pr-review"], + # Fetch the PR head into FETCH_HEAD rather than directly into a local + # ``pr-review`` branch. When the workspace is reused across reviews, the + # previous run leaves ``pr-review`` checked out, and + # ``git fetch origin pull/N/head:pr-review`` is *refused* by git + # ("refusing to fetch into branch '...' checked out at ..."). That failure + # was previously swallowed (capture_output, no return-code check), so every + # PR after the first in a workspace's lifetime got reviewed against the + # first PR's tree — producing silent "no findings" reviews. Fetching to + # FETCH_HEAD always succeeds, and ``checkout -B`` then (re)points + # ``pr-review`` at it even when it is the currently checked-out branch. + fetch = subprocess.run( + ["git", "-C", target_dir, "fetch", "--depth", "1", "origin", f"pull/{pr_number}/head"], env=git_env, timeout=300, capture_output=True, + text=True, ) - subprocess.run( - ["git", "-C", target_dir, "checkout", "pr-review"], + if fetch.returncode != 0: + raise ValueError(f"git fetch of PR #{pr_number} head failed: {fetch.stderr.strip()}") + checkout = subprocess.run( + ["git", "-C", target_dir, "checkout", "-B", "pr-review", "FETCH_HEAD"], env=git_env, timeout=30, capture_output=True, + text=True, ) + if checkout.returncode != 0: + raise ValueError(f"git checkout of PR #{pr_number} (pr-review) failed: {checkout.stderr.strip()}") def _resolve_repo(repo_path: str | None, pr_url: str | None) -> str: diff --git a/tests/test_resolve_repo.py b/tests/test_resolve_repo.py new file mode 100644 index 0000000..d297f09 --- /dev/null +++ b/tests/test_resolve_repo.py @@ -0,0 +1,107 @@ +"""Behavior tests for PR-head checkout into the pr-af workspace. + +Maps to the validation contract for ``_checkout_pr_branch``: + +* fresh workspace -> working tree reflects the PR head +* REUSED workspace (``pr-review`` already checked out from a prior PR) -> + working tree updates to the *new* PR head, not left on the prior PR's tree. + This is the regression test for the silent "no findings" bug: a reused + workspace kept reviewing every PR after the first against the first PR's tree. +* unfetchable PR ref -> raises, instead of silently leaving a stale tree. + +Uses real temporary git repositories (a local path acts as the ``origin`` +remote with ``refs/pull//head`` refs); no network and no mocking of the +function's internals. +""" + +from __future__ import annotations + +import subprocess +from pathlib import Path + +import pytest + +from pr_af.app import _checkout_pr_branch + + +def _git(*args: str, cwd: Path) -> str: + return subprocess.run( + ["git", *args], cwd=cwd, check=True, capture_output=True, text=True + ).stdout.strip() + + +def _make_upstream(path: Path) -> None: + path.mkdir() + _git("init", "-q", "-b", "main", cwd=path) + _git("config", "user.email", "test@example.com", cwd=path) + _git("config", "user.name", "test", cwd=path) + _git("config", "commit.gpgsign", "false", cwd=path) + (path / "marker.txt").write_text("main\n") + _git("add", "-A", cwd=path) + _git("commit", "-qm", "initial", cwd=path) + + +def _publish_pr_ref(upstream: Path, pr_number: int, content: str) -> None: + """Create ``refs/pull//head`` in ``upstream`` with marker.txt=content.""" + _git("checkout", "-q", "-b", f"_pr{pr_number}", "main", cwd=upstream) + (upstream / "marker.txt").write_text(content + "\n") + _git("commit", "-qam", f"pr{pr_number}", cwd=upstream) + sha = _git("rev-parse", "HEAD", cwd=upstream) + _git("update-ref", f"refs/pull/{pr_number}/head", sha, cwd=upstream) + _git("checkout", "-q", "main", cwd=upstream) + _git("branch", "-qD", f"_pr{pr_number}", cwd=upstream) + + +def _clone_workspace(upstream: Path, target: Path) -> None: + """Mirror pr-af's clone: shallow, no tags, no default-branch checkout.""" + _git( + "clone", "--depth", "1", "--no-tags", "--no-checkout", + str(upstream), str(target), cwd=upstream.parent, + ) + + +def test_checkout_reused_workspace_updates_to_new_pr_head(tmp_path: Path) -> None: + """A reused workspace must review the *current* PR, not the previous one.""" + upstream = tmp_path / "upstream" + _make_upstream(upstream) + _publish_pr_ref(upstream, 1, "pr1") + + target = tmp_path / "workspace" + _clone_workspace(upstream, target) + marker = target / "marker.txt" + + # First PR in this workspace: working tree reflects PR #1. + _checkout_pr_branch(str(target), 1) + assert marker.read_text() == "pr1\n" + assert _git("rev-parse", "--abbrev-ref", "HEAD", cwd=target) == "pr-review" + + # A second PR arrives; the workspace is reused (pr-review still checked out). + _publish_pr_ref(upstream, 2, "pr2") + _git("fetch", "--all", cwd=target) # what _resolve_repo does for a reused dir + + _checkout_pr_branch(str(target), 2) + # Regression: before the fix this stayed on PR #1's tree ("pr1"). + assert marker.read_text() == "pr2\n" + + +def test_checkout_fresh_workspace_reflects_pr_head(tmp_path: Path) -> None: + upstream = tmp_path / "upstream" + _make_upstream(upstream) + _publish_pr_ref(upstream, 7, "pr7") + + target = tmp_path / "workspace" + _clone_workspace(upstream, target) + + _checkout_pr_branch(str(target), 7) + assert (target / "marker.txt").read_text() == "pr7\n" + + +def test_checkout_raises_on_unfetchable_pr_ref(tmp_path: Path) -> None: + upstream = tmp_path / "upstream" + _make_upstream(upstream) + + target = tmp_path / "workspace" + _clone_workspace(upstream, target) + + with pytest.raises(ValueError, match="PR #999"): + _checkout_pr_branch(str(target), 999)