ultralytics/actions
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Review formatted PR commits with local Git context (#897)

Browse Source
This commit is contained in:
Glenn Jocher
2026-09-05 21:55:14 +02:00
committed by GitHub
parent 3e76669ec3
commit ed8e78f4f2
5 changed files with 148 additions and 152 deletions
Download Patch File Download Diff File Expand all files Collapse all files
action.yml
+43 -43
View File
@@ -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
actions/__init__.py
+1 -1
View File
@@ -28,4 +28,4 @@
# ├── test_summarize_pr.py
# └── ...
__version__ = "0.3.20"
__version__ = "0.3.21"
actions/review_pr.py
+70 -77
View File
@@ -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)
actions/utils/github_utils.py
+5 -15
View File
@@ -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()
tests/test_first_interaction.py
+29 -16
View File
@@ -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", "[email protected]", "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