From ed8e78f4f217f9e7086da7b25fa5b5f8035267b2 Mon Sep 17 00:00:00 2001 From: Glenn Jocher Date: Sat, 5 Sep 2026 21:55:14 +0200 Subject: [PATCH] Review formatted PR commits with local Git context (#897) --- action.yml | 86 +++++++++---------- actions/__init__.py | 2 +- actions/review_pr.py | 147 +++++++++++++++----------------- actions/utils/github_utils.py | 20 ++--- tests/test_first_interaction.py | 45 ++++++---- 5 files changed, 148 insertions(+), 152 deletions(-) diff --git a/action.yml b/action.yml index 4579e2f..f855598 100644 --- a/action.yml +++ b/action.yml @@ -306,49 +306,6 @@ runs: shell: bash continue-on-error: true - # PR automation and first interaction ------------------------------------------------------------------------------ - - name: PR Automation and First Interaction - if: | - (inputs.labels == 'true' && (github.event.action == 'opened' || github.event.action == 'created')) || - ((github.event_name == 'pull_request' || github.event_name == 'pull_request_target') && - github.event.action == 'opened' && (inputs.summary == 'true' || inputs.review == 'true')) - env: - GITHUB_TOKEN: ${{ inputs.token }} - FIRST_ISSUE_RESPONSE: ${{ inputs.first_issue_response }} - BRAVE_API_KEY: ${{ inputs.brave_api_key }} - OPENAI_API_KEY: ${{ inputs.openai_api_key }} - ANTHROPIC_API_KEY: ${{ inputs.anthropic_api_key }} - MODEL: ${{ inputs.model }} - REVIEW_MODEL: ${{ inputs.review_model }} - LABELS: ${{ inputs.labels }} - SUMMARY: ${{ inputs.summary }} - REVIEW: ${{ inputs.review }} - run: | - echo "::group::PR Automation and First Interaction" - trap 'echo "::endgroup::"' EXIT - ultralytics-actions-first-interaction - shell: bash - continue-on-error: true - - # PR Review -------------------------------------------------------------------------------------------------------- - - name: PR Review - if: | - (github.event_name == 'pull_request' || github.event_name == 'pull_request_target') && - github.event.action == 'review_requested' && - github.event.requested_reviewer.login == inputs.github_username - env: - GITHUB_TOKEN: ${{ inputs.token }} - OPENAI_API_KEY: ${{ inputs.openai_api_key }} - ANTHROPIC_API_KEY: ${{ inputs.anthropic_api_key }} - MODEL: ${{ inputs.model }} - REVIEW_MODEL: ${{ inputs.review_model }} - run: | - echo "::group::PR Review" - trap 'echo "::endgroup::"' EXIT - ultralytics-actions-review-pr - shell: bash - continue-on-error: true - # Bun Update --------------------------------------------------------------------------------------------------------- - name: Bun Update if: github.event_name == 'pull_request' && inputs.bun_update == 'true' && github.event.action == 'opened' @@ -404,6 +361,49 @@ runs: shell: bash continue-on-error: true + # PR automation and first interaction ------------------------------------------------------------------------------ + - name: PR Automation and First Interaction + if: | + (inputs.labels == 'true' && (github.event.action == 'opened' || github.event.action == 'created')) || + ((github.event_name == 'pull_request' || github.event_name == 'pull_request_target') && + github.event.action == 'opened' && (inputs.summary == 'true' || inputs.review == 'true')) + env: + GITHUB_TOKEN: ${{ inputs.token }} + FIRST_ISSUE_RESPONSE: ${{ inputs.first_issue_response }} + BRAVE_API_KEY: ${{ inputs.brave_api_key }} + OPENAI_API_KEY: ${{ inputs.openai_api_key }} + ANTHROPIC_API_KEY: ${{ inputs.anthropic_api_key }} + MODEL: ${{ inputs.model }} + REVIEW_MODEL: ${{ inputs.review_model }} + LABELS: ${{ inputs.labels }} + SUMMARY: ${{ inputs.summary }} + REVIEW: ${{ inputs.review }} + run: | + echo "::group::PR Automation and First Interaction" + trap 'echo "::endgroup::"' EXIT + ultralytics-actions-first-interaction + shell: bash + continue-on-error: true + + # PR Review -------------------------------------------------------------------------------------------------------- + - name: PR Review + if: | + (github.event_name == 'pull_request' || github.event_name == 'pull_request_target') && + github.event.action == 'review_requested' && + github.event.requested_reviewer.login == inputs.github_username + env: + GITHUB_TOKEN: ${{ inputs.token }} + OPENAI_API_KEY: ${{ inputs.openai_api_key }} + ANTHROPIC_API_KEY: ${{ inputs.anthropic_api_key }} + MODEL: ${{ inputs.model }} + REVIEW_MODEL: ${{ inputs.review_model }} + run: | + echo "::group::PR Review" + trap 'echo "::endgroup::"' EXIT + ultralytics-actions-review-pr + shell: bash + continue-on-error: true + # Broken links ----------------------------------------------------------------------------------------------------- # lychee-action only ships Linux binaries, so install lychee directly to support macOS runners (Swift repos) too. - name: Broken Link Checker diff --git a/actions/__init__.py b/actions/__init__.py index 3bf86a6..79faca9 100644 --- a/actions/__init__.py +++ b/actions/__init__.py @@ -28,4 +28,4 @@ # ├── test_summarize_pr.py # └── ... -__version__ = "0.3.20" +__version__ = "0.3.21" diff --git a/actions/review_pr.py b/actions/review_pr.py index 5246664..d17a039 100644 --- a/actions/review_pr.py +++ b/actions/review_pr.py @@ -4,6 +4,7 @@ from __future__ import annotations import json import re +import subprocess from fnmatch import fnmatch from pathlib import Path from urllib.parse import quote @@ -46,46 +47,21 @@ def _clip_tool_output(text: str, limit: int = MAX_TOOL_OUTPUT_CHARS) -> str: return text if len(text) <= limit else f"{text[:limit].rstrip()}\n... (truncated)" -def _iter_repo_files(path_glob=None): - """Yield repository files, including hidden files, pruning vendored dirs and paths outside the checkout.""" - root = Path.cwd().resolve() - stack = [root] - while stack: - try: - children = list(stack.pop().iterdir()) - except OSError: - continue - for path in children: - if path.is_dir(): - if path.name not in COMMON_EXCLUDED_DIRS and not path.is_symlink(): - stack.append(path) - continue - rel = path.relative_to(root).as_posix() - if (path_glob and not fnmatch(rel, path_glob)) or should_skip_file(rel): - continue - try: - target = path.resolve() - target.relative_to(root) - except (OSError, ValueError): - continue - if target.is_file(): - yield target, rel - - -def search_repo(query: str, path_glob=None) -> str: - """Search repository text for agent review context.""" - if not query: - return "query is required." - matches = [] - for target, rel in _iter_repo_files(path_glob): - if target.stat().st_size > 500_000: - continue - for line_no, line in enumerate(target.read_text(encoding="utf-8", errors="ignore").splitlines(), 1): - if query in line: - matches.append(f"{rel}:{line_no}:{line}") - if len(matches) >= 200: - return _clip_tool_output("\n".join(matches)) - return _clip_tool_output("\n".join(matches)) if matches else "No matches found." +def _iter_repo_files(head_sha: str, path_glob=None): + """Yield reviewable file paths from the local commit, independent of working-tree edits.""" + result = subprocess.run( + ["git", "ls-tree", "-rlz", head_sha], capture_output=True, text=True, errors="ignore", check=True + ) + for entry in result.stdout.split("\0"): + metadata, _, path = entry.partition("\t") + if ( + path + and metadata.split()[1] == "blob" + and (not path_glob or fnmatch(path, path_glob)) + and not should_skip_file(path) + and not any(part in COMMON_EXCLUDED_DIRS for part in Path(path).parent.parts) + ): + yield path, int(metadata.split()[-1]) def _clip(text: str, limit: int | None) -> str: @@ -170,14 +146,25 @@ def _fetch_head_file(event: Action, sha: str, path: str) -> str | None: def _read_head_file(event: Action, head_sha: str | None, local_checkout: bool, path: str) -> str | None: - """Read one file's text from the PR head: verified local checkout first (no API calls), GitHub API fallback.""" + """Read the reviewed commit from local Git objects when available, otherwise the GitHub API.""" if local_checkout: - root = Path.cwd().resolve() - target = (root / path).resolve() - target.relative_to(root) # raises ValueError for paths or symlinks escaping the checkout - if not target.is_file(): + if Path(path).is_absolute() or ".." in Path(path).parts or "\n" in path: + raise ValueError(path) + result = subprocess.run( + ["git", "cat-file", "--batch", "--follow-symlinks"], + input=f"{head_sha}:{path}\n", + capture_output=True, + text=True, + encoding="utf-8", + errors="ignore", + check=True, + ) + header, _, content = result.stdout.partition("\n") + if header.endswith(" missing"): return None - return target.read_text(encoding="utf-8", errors="ignore") + if " blob " not in header: + raise ValueError(path) + return content[:-1] # cat-file adds one newline after the blob if not (event and head_sha): return None return _fetch_head_file(event, head_sha, path) @@ -205,7 +192,7 @@ def build_review_agent_tools( local_checkout: bool = False, review_history: dict | None = None, ) -> tuple[list[dict], dict]: - """Build read-only tools for the PR review agent, reading files from the PR head via the GitHub API.""" + """Build read-only tools for the review commit, using local Git objects when available.""" diff_files = diff_files or {} review_history = review_history or {} diff_chunks = _split_augmented_diff_by_file(augmented_diff) @@ -219,7 +206,7 @@ def build_review_agent_tools( try: text = _read_head_file(event, head_sha, local_checkout, path) except ValueError: - return f"path must stay inside repository: {path}" + return f"{path} is not a readable file at the PR head." if text is None: return f"{path} does not exist at the PR head." if len(text) > 500_000: @@ -235,7 +222,7 @@ def build_review_agent_tools( def list_files(path_glob=None) -> str: """List files at the PR head matching an optional glob.""" if local_checkout: - files = sorted(rel for _, rel in _iter_repo_files(path_glob)) + files = sorted(path for path, _ in _iter_repo_files(head_sha, path_glob)) return _clip_tool_output("\n".join(files[:300])) if files else "No matching files found." if not (event and head_sha): return "list_files is unavailable: no PR head to list from." @@ -247,6 +234,30 @@ def build_review_agent_tools( files = sorted(p for p in head_tree[0] if (not path_glob or fnmatch(p, path_glob)) and not should_skip_file(p)) return _clip_tool_output("\n".join(files[:300])) if files else "No matching files found." + def search_repo(query: str, path_glob=None) -> str: + """Search committed repository text for agent review context.""" + if not query: + return "query is required." + files = [path for path, size in _iter_repo_files(head_sha, path_glob) if size <= 500_000] + matches = [] + for start in range(0, len(files), 100): + with subprocess.Popen( + ["git", "--literal-pathspecs", "grep", "-nIzFe", query, head_sha, "--", *files[start : start + 100]], + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + text=True, + errors="ignore", + ) as process: + for match in process.stdout: + path, _, rest = match.rstrip("\n").partition("\0") + matches.append(f"{path[len(head_sha) + 1 :]}:{rest.replace(chr(0), ':', 1)}") + if len(matches) >= 200 or sum(map(len, matches)) >= MAX_TOOL_OUTPUT_CHARS: + process.terminate() + return _clip_tool_output("\n".join(matches)) + if process.wait() not in (0, 1): + raise RuntimeError(process.stderr.read()) + return _clip_tool_output("\n".join(matches)) if matches else "No matches found." + def list_changed_files(path_glob=None) -> str: """List changed files in this PR with added/removed line counts.""" rows = [] @@ -479,7 +490,7 @@ def generate_pr_review( diff_text, skipped_files = diff review_history = review_history or {} prior_reviews = review_history.get("reviews") or [] - head_sha = head_sha or (event.get_pr_head_sha() if event else None) + head_sha = head_sha or (event.pr["head"]["sha"] if event else None) if diff_text.startswith("ERROR:"): return {"comments": [], "summary": f"{ERROR_MARKER}: {diff_text}", "head_sha": head_sha} if not diff_text: @@ -804,29 +815,15 @@ def generate_pr_review( return {"comments": [], "summary": summary, "head_sha": head_sha} -def get_local_head_sha() -> str | None: - """Get the current HEAD SHA from local git repo.""" - import subprocess - - try: - result = subprocess.run(["git", "rev-parse", "HEAD"], capture_output=True, text=True, check=True) - return result.stdout.strip() - except Exception as e: - print(f"Failed to get local HEAD SHA: {e}") - return None - - def _verified_local_checkout(head_sha: str | None) -> bool: - """Check the working tree is a clean checkout of the PR head (formatters may dirty it before reviews run).""" - import subprocess - - if not head_sha or get_local_head_sha() != head_sha: - return False - try: - status = subprocess.run(["git", "status", "--porcelain"], capture_output=True, text=True, check=True) - return not status.stdout.strip() - except Exception: - return False + """Check whether the reviewed commit is available locally, regardless of working-tree edits.""" + return ( + bool(head_sha) + and subprocess.run( + ["git", "cat-file", "-e", f"{head_sha}^{{commit}}"], capture_output=True, check=False + ).returncode + == 0 + ) def clear_previous_review(event: Action) -> dict: @@ -947,11 +944,7 @@ def post_review_summary(event: Action, review_data: dict, review_number: int = 1 def run_review(event: Action, pr_title: str, pr_description: str) -> None: """Supersede prior reviews, then generate and publish the next numbered review of the PR head.""" - try: - diff, head_sha = event.get_pr_diff_snapshot() - except RuntimeError as e: - print(f"Skipping stale PR review: {e}") - return + diff, head_sha = event.get_pr_diff(), event.pr["head"]["sha"] history = clear_previous_review(event) review = generate_pr_review(event.repository, diff, pr_title, pr_description, event, head_sha, history) post_review_summary(event, review, review_number=len(history["reviews"]) + 1) diff --git a/actions/utils/github_utils.py b/actions/utils/github_utils.py index 15d6c8c..e8b8ae4 100644 --- a/actions/utils/github_utils.py +++ b/actions/utils/github_utils.py @@ -262,12 +262,13 @@ class Action: return True return False - def get_pr_diff(self, refresh: bool = False) -> tuple[str, list[str]]: - """Retrieve and filter a pull request diff with caching.""" - if self._pr_diff_cache and not refresh: + def get_pr_diff(self) -> tuple[str, list[str]]: + """Retrieve the live PR once and cache its diff pinned to immutable base and head commits.""" + if self._pr_diff_cache: return self._pr_diff_cache - url = f"{GITHUB_API_URL}/repos/{self.repository}/pulls/{self.pr.get('number')}" + self.pr = self.get(f"{GITHUB_API_URL}/repos/{self.repository}/pulls/{self.pr['number']}", hard=True).json() + url = f"{GITHUB_API_URL}/repos/{self.repository}/compare/{self.pr['base']['sha']}...{self.pr['head']['sha']}" response = self.get(url, headers=self.headers_diff) if response.status_code == 200: diff = response.text or "ERROR: EMPTY DIFF, NO CODE CHANGES IN THIS PR." @@ -287,17 +288,6 @@ class Action: return sha return None - def get_pr_diff_snapshot(self) -> tuple[tuple[str, list[str]], str]: - """Fetch a diff whose PR head stayed unchanged for the full request.""" - for _ in range(2): - head_before = self.get_pr_head_sha() - diff = self.get_pr_diff(refresh=True) - head_after = self.get_pr_head_sha() - if head_before and head_before == head_after: - return diff, head_after - print(f"PR head moved while fetching diff ({head_before} -> {head_after}); retrying") - raise RuntimeError("PR head changed repeatedly while fetching the review diff") - def get_repo_data(self, endpoint: str) -> dict: """Fetches repository data from a specified endpoint.""" return self.get(f"{GITHUB_API_URL}/repos/{self.repository}/{endpoint}").json() diff --git a/tests/test_first_interaction.py b/tests/test_first_interaction.py index 20c2507..852f810 100644 --- a/tests/test_first_interaction.py +++ b/tests/test_first_interaction.py @@ -1,5 +1,6 @@ # Ultralytics 🚀 AGPL-3.0 License - https://ultralytics.com/license +import subprocess from unittest.mock import MagicMock, call, patch import pytest @@ -101,7 +102,6 @@ def test_open_pr_review_description(mock_action, mock_content, mock_response, mo event.paginate.return_value = [] event.pr = {"body": body} event.get_pr_diff.return_value = ("diff", []) - event.get_pr_diff_snapshot.return_value = (("review diff", []), "headsha") first_interaction.main() @@ -140,9 +140,12 @@ def test_get_first_interaction_response(mock_get_response, mock_check_links): assert "never assume Python, pip, PyPI" in prompt +@patch("actions.review_pr._read_head_file", return_value=None) @patch("actions.review_pr._verified_local_checkout", return_value=True) @patch("actions.review_pr.get_agent_response") -def test_generate_pr_review_uses_synchronous_response(mock_get_agent_response, mock_checkout, tmp_path, monkeypatch): +def test_generate_pr_review_uses_synchronous_response( + mock_get_agent_response, mock_checkout, mock_read, tmp_path, monkeypatch +): """Test PR reviews avoid background polling for code diffs.""" monkeypatch.chdir(tmp_path) mock_get_agent_response.return_value = {"comments": [], "summary": "LGTM"} @@ -199,11 +202,21 @@ def test_review_agent_search_scans_local_checkout_only(tmp_path, monkeypatch): outside.write_text("outside_secret = True\n", encoding="utf-8") (tmp_path / "linked_secret.py").symlink_to(outside) - tools, handlers = review_pr.build_review_agent_tools(local_checkout=True) + monkeypatch.setenv("GIT_CONFIG_GLOBAL", "/dev/null") + monkeypatch.setenv("GIT_CONFIG_SYSTEM", "/dev/null") + subprocess.run(["git", "init", "-q"], check=True) + subprocess.run(["git", "add", "."], check=True) + subprocess.run( + ["git", "-c", "user.name=Test", "-c", "user.email=test@example.com", "commit", "-qm", "Fixture"], check=True + ) + head_sha = subprocess.check_output(["git", "rev-parse", "HEAD"], text=True).strip() + (tmp_path / "secret.py").write_text("uncommitted = True\n", encoding="utf-8") + assert review_pr._verified_local_checkout(head_sha) + tools, handlers = review_pr.build_review_agent_tools(head_sha=head_sha, local_checkout=True) assert "search_repo" in {t.get("name") for t in tools} assert "1: secret = True" in handlers["read_file"](path="secret.py", start_line=None, end_line=None) - assert "path must stay inside repository" in handlers["read_file"]( + assert "is not a readable file at the PR head" in handlers["read_file"]( path="linked_secret.py", start_line=None, end_line=None ) assert "secret.py" in handlers["list_files"](path_glob=None).splitlines() @@ -235,26 +248,26 @@ def test_pr_head_sha_prefers_live_value(): assert review_pr.Action.get_pr_head_sha(event) is None -def test_review_snapshot_retries_until_diff_and_head_match(): - """Test review snapshots refetch the diff when a push races the first request.""" - event = MagicMock() - event.get_pr_head_sha.side_effect = ["old", "new", "new", "new"] - event.get_pr_diff.side_effect = [("old diff", []), ("new diff", ["generated/client.py"])] - - assert review_pr.Action.get_pr_diff_snapshot(event) == (("new diff", ["generated/client.py"]), "new") - assert event.get_pr_diff.call_args_list == [call(refresh=True), call(refresh=True)] - assert event.get_pr_head_sha.call_count == 4 - - def test_pr_diff_is_filtered_at_retrieval(): """Test every PR diff consumer receives the same filtered diff and skipped-file list.""" - event = MagicMock(repository="org/repo", pr={"number": 1}, headers_diff={}, _pr_diff_cache=None) + event = MagicMock( + repository="org/repo", + pr={"number": 1, "base": {"sha": "base"}, "head": {"sha": "head"}}, + headers_diff={}, + _pr_diff_cache=None, + ) event.get.return_value = MagicMock( status_code=200, text="diff --git a/code.py b/code.py\n+kept\ndiff --git a/generated/client.py b/generated/client.py\n+omitted", ) + event.get.return_value.json.return_value = {"number": 1, "base": {"sha": "base"}, "head": {"sha": "formatted"}} diff, skipped_files = review_pr.Action.get_pr_diff(event) + assert review_pr.Action.get_pr_diff(event) == (diff, skipped_files) + assert event.get.call_args_list == [ + call("https://api.github.com/repos/org/repo/pulls/1", hard=True), + call("https://api.github.com/repos/org/repo/compare/base...formatted", headers={}), + ] assert "+kept" in diff assert "generated/client.py" not in diff