Serve PR review tools from the PR head via GitHub API (#792)
This commit is contained in:
+1
-1
@@ -28,4 +28,4 @@
|
||||
# ├── test_summarize_pr.py
|
||||
# └── ...
|
||||
|
||||
__version__ = "0.2.18"
|
||||
__version__ = "0.2.19"
|
||||
|
||||
@@ -0,0 +1,77 @@
|
||||
# Ultralytics 🚀 AGPL-3.0 License - https://ultralytics.com/license
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import os
|
||||
import subprocess
|
||||
|
||||
# Mirrors the formatting steps in action.yml (source of truth if these drift); tested in tests/test_format_code.py
|
||||
RUFF_CHECK = [
|
||||
"ruff",
|
||||
"check",
|
||||
"--fix",
|
||||
"--unsafe-fixes",
|
||||
"--extend-select=F,I,D,UP,RUF,FA",
|
||||
"--target-version=py39",
|
||||
"--ignore=D100,D104,D203,D205,D212,D213,D401,D406,D407,D413,RUF001,RUF002,RUF012",
|
||||
".",
|
||||
]
|
||||
RUFF_FORMAT = ["ruff", "format", "--line-length=120", "."]
|
||||
DOCSTRINGS = ["ultralytics-actions-format-python-docstrings", "."]
|
||||
PRETTIER = """
|
||||
npm install -g [email protected] prettier-plugin-sh
|
||||
ultralytics-actions-update-markdown-code-blocks
|
||||
npx prettier --write --list-different --print-width 120 "**/*.{js,jsx,ts,tsx,css,less,scss,json,yml,yaml,html,vue,svelte}" '!**/*lock.{json,yaml,yml}' '!**/*.lock' '!**/model.json' '!**/*.min.js' '!**/*.min.css'
|
||||
if find . -name "*.sh" -type f | grep -q .; then
|
||||
npx prettier --write --list-different --print-width 120 --plugin=$(npm root -g)/prettier-plugin-sh/lib/index.cjs "**/*.sh"
|
||||
fi
|
||||
# Handle Markdown separately
|
||||
find . -name "*.md" ! -path "*/docs/*" -exec npx prettier --write --list-different --print-width 120 {} +
|
||||
if [ -d "./docs" ]; then
|
||||
find ./docs -name "*.md" ! -path "*/reference/*" -exec npx prettier --tab-width 4 --print-width 120 --write --list-different {} +
|
||||
fi
|
||||
"""
|
||||
CODESPELL = [
|
||||
"codespell",
|
||||
"--builtin",
|
||||
"clear,rare,informal,en-GB_to_en-US",
|
||||
"--write-changes",
|
||||
"--uri-ignore-words-list",
|
||||
"*",
|
||||
"--ignore-regex",
|
||||
r"\b[a-z]+[A-Z][a-zA-Z]*\b",
|
||||
"--ignore-words-list",
|
||||
"nin,cancelled,MapPin,couldn,grey,writeable,RepResNet,Idenfy,WIT,Smoot,EHR,ROUGE,ALS,Carmel,FPR,Hach,Calle,ore,COO,MOT,crate,nd,ned,strack,dota,ane,segway,fo,gool,winn,commend,bloc,nam,afterall,skelton,goin,cann,CANN",
|
||||
"--skip",
|
||||
"*.pt,*.pth,*.torchscript,*.onnx,*.tflite,*.pb,*.bin,*.param,*.mlmodel,*.engine,*.npy,*.data*,*.csv,*pnnx*,*venv*,*translat*,*lock*,__pycache__*,*.ico,*.jpg,*.png,*.webp,*.avif,*.mp4,*.mov,/runs,/.git,./docs/??/*.md,./docs/mkdocs_??.yml,action.yml",
|
||||
]
|
||||
|
||||
|
||||
def _enabled(name: str) -> bool:
|
||||
"""Check an INPUTS_* env flag, defaulting to enabled."""
|
||||
return os.getenv(name, "true").lower() == "true"
|
||||
|
||||
|
||||
def _run(cmd, shell: bool = False) -> None:
|
||||
"""Run one formatter, continuing on failure like the composite action steps."""
|
||||
try:
|
||||
subprocess.run(cmd, shell=shell, check=True)
|
||||
except Exception as e:
|
||||
print(f"Formatter {'script' if shell else cmd[0]} failed but continuing: {e}")
|
||||
|
||||
|
||||
def main():
|
||||
"""Format the current directory with the Ultralytics Actions formatters (Python, Prettier, Codespell)."""
|
||||
if _enabled("INPUTS_PYTHON"):
|
||||
_run(RUFF_CHECK)
|
||||
_run(RUFF_FORMAT)
|
||||
if _enabled("INPUTS_PYTHON_DOCSTRINGS"):
|
||||
_run(DOCSTRINGS)
|
||||
if _enabled("INPUTS_PRETTIER"):
|
||||
_run(PRETTIER, shell=True)
|
||||
if _enabled("INPUTS_SPELLING"):
|
||||
_run(CODESPELL)
|
||||
|
||||
|
||||
if __name__ == "__main__":
|
||||
main()
|
||||
+156
-88
@@ -6,6 +6,7 @@ import json
|
||||
import re
|
||||
from fnmatch import fnmatch
|
||||
from pathlib import Path
|
||||
from urllib.parse import quote
|
||||
|
||||
from .utils import (
|
||||
ACTIONS_CREDIT,
|
||||
@@ -40,38 +41,6 @@ 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 _resolve_repo_path(path: str) -> Path:
|
||||
"""Resolve a repo-relative path without allowing access outside the checkout."""
|
||||
root = Path.cwd().resolve()
|
||||
target = (root / (path or ".")).resolve()
|
||||
try:
|
||||
target.relative_to(root)
|
||||
except ValueError as e:
|
||||
raise ValueError(f"path must stay inside repository: {path}") from e
|
||||
return target
|
||||
|
||||
|
||||
def read_file(path: str, start_line=None, end_line=None) -> str:
|
||||
"""Read a bounded range from a repository file for agent review context."""
|
||||
try:
|
||||
target = _resolve_repo_path(path)
|
||||
except ValueError as e:
|
||||
return str(e)
|
||||
rel = target.relative_to(Path.cwd().resolve()).as_posix()
|
||||
if not target.is_file():
|
||||
return f"{path} is not a file."
|
||||
if should_skip_file(rel) or target.stat().st_size > 500_000:
|
||||
return f"{path} is skipped because it is generated, vendored, or too large."
|
||||
|
||||
lines = target.read_text(encoding="utf-8", errors="ignore").splitlines()
|
||||
start = max(1, int(start_line or 1))
|
||||
end = min(len(lines), int(end_line or len(lines)), start + MAX_TOOL_FILE_LINES - 1)
|
||||
if end < start:
|
||||
return f"{path} has no lines in requested range {start}-{end}."
|
||||
numbered = "\n".join(f"{i:>5}: {lines[i - 1]}" for i in range(start, end + 1))
|
||||
return _clip_tool_output(f"{rel}:{start}-{end}\n{numbered}")
|
||||
|
||||
|
||||
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()
|
||||
@@ -98,14 +67,6 @@ def _iter_repo_files(path_glob=None):
|
||||
yield target, rel
|
||||
|
||||
|
||||
def list_files(path_glob=None) -> str:
|
||||
"""List repository files matching an optional glob."""
|
||||
files = [rel for _, rel in _iter_repo_files(path_glob)]
|
||||
if not files:
|
||||
return "No matching files found."
|
||||
return _clip_tool_output("\n".join(sorted(files)[:300]))
|
||||
|
||||
|
||||
def search_repo(query: str, path_glob=None) -> str:
|
||||
"""Search repository text for agent review context."""
|
||||
if not query:
|
||||
@@ -122,6 +83,49 @@ def search_repo(query: str, path_glob=None) -> str:
|
||||
return _clip_tool_output("\n".join(matches)) if matches else "No matches found."
|
||||
|
||||
|
||||
def _pr_head_sha(event: Action | None) -> str | None:
|
||||
"""Return the live PR head SHA (payload fallback) so reviews read the same code the diff describes."""
|
||||
if event is None or not event.pr:
|
||||
return None
|
||||
if (number := event.pr.get("number")) and event.repository:
|
||||
try:
|
||||
response = event.get(f"{GITHUB_API_URL}/repos/{event.repository}/pulls/{number}")
|
||||
if response.status_code == 200 and (sha := (response.json().get("head") or {}).get("sha")):
|
||||
return sha
|
||||
except Exception as e:
|
||||
print(f"Live PR head lookup failed: {e}")
|
||||
sha = (event.pr.get("head") or {}).get("sha")
|
||||
return sha if isinstance(sha, str) else None
|
||||
|
||||
|
||||
def _fetch_head_file(event: Action, sha: str, path: str) -> str | None:
|
||||
"""Fetch one file's text from the PR head via the GitHub contents API; None only if it does not exist (404)."""
|
||||
url = f"{GITHUB_API_URL}/repos/{event.repository}/contents/{quote(path)}?ref={sha}"
|
||||
headers = {**event.headers, "Accept": "application/vnd.github.raw+json"}
|
||||
response = event.get(url, headers=headers, expected_status=[200, 404])
|
||||
if response.status_code == 200:
|
||||
return response.text
|
||||
if response.status_code == 404:
|
||||
return None
|
||||
raise RuntimeError(f"fetch failed for {path} at PR head: HTTP {response.status_code}")
|
||||
|
||||
|
||||
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."""
|
||||
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():
|
||||
return None
|
||||
if target.stat().st_size > 500_000:
|
||||
raise RuntimeError(f"{path} is too large to read")
|
||||
return target.read_text(encoding="utf-8", errors="ignore")
|
||||
if not (event and head_sha):
|
||||
return None
|
||||
return _fetch_head_file(event, head_sha, path)
|
||||
|
||||
|
||||
def _split_augmented_diff_by_file(augmented_diff: str) -> dict[str, list[str]]:
|
||||
"""Split an augmented diff into per-file chunks."""
|
||||
chunks, current_file = {}, None
|
||||
@@ -136,10 +140,52 @@ def _split_augmented_diff_by_file(augmented_diff: str) -> dict[str, list[str]]:
|
||||
return chunks
|
||||
|
||||
|
||||
def build_review_agent_tools(diff_files: dict | None = None, augmented_diff: str = "") -> tuple[list[dict], dict]:
|
||||
"""Build read-only tools for the PR review agent."""
|
||||
def build_review_agent_tools(
|
||||
diff_files: dict | None = None,
|
||||
augmented_diff: str = "",
|
||||
event: Action = None,
|
||||
head_sha: str | None = None,
|
||||
local_checkout: bool = False,
|
||||
) -> tuple[list[dict], dict]:
|
||||
"""Build read-only tools for the PR review agent, reading files from the PR head via the GitHub API."""
|
||||
diff_files = diff_files or {}
|
||||
diff_chunks = _split_augmented_diff_by_file(augmented_diff)
|
||||
head_tree = [] # cached PR head file listing
|
||||
|
||||
def read_file(path: str, start_line=None, end_line=None) -> str:
|
||||
"""Read a bounded line range from a file at the PR head."""
|
||||
if should_skip_file(path):
|
||||
return f"{path} is skipped because it is generated, vendored, or too large."
|
||||
try:
|
||||
text = _read_head_file(event, head_sha, local_checkout, path)
|
||||
except ValueError:
|
||||
return f"path must stay inside repository: {path}"
|
||||
if text is None:
|
||||
return f"{path} does not exist at the PR head."
|
||||
if len(text) > 500_000:
|
||||
return f"{path} is skipped because it is generated, vendored, or too large."
|
||||
lines = text.splitlines()
|
||||
start = max(1, int(start_line or 1))
|
||||
end = min(len(lines), int(end_line or len(lines)), start + MAX_TOOL_FILE_LINES - 1)
|
||||
if end < start:
|
||||
return f"{path} has no lines in requested range {start}-{end}."
|
||||
numbered = "\n".join(f"{i:>5}: {lines[i - 1]}" for i in range(start, end + 1))
|
||||
return _clip_tool_output(f"{path}:{start}-{end}\n{numbered}")
|
||||
|
||||
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))
|
||||
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."
|
||||
if not head_tree:
|
||||
response = event.get(f"{GITHUB_API_URL}/repos/{event.repository}/git/trees/{head_sha}?recursive=1")
|
||||
if response.status_code != 200:
|
||||
return f"list_files failed: HTTP {response.status_code}." # don't cache failures
|
||||
head_tree.append([t["path"] for t in response.json().get("tree", []) if t.get("type") == "blob"])
|
||||
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 list_changed_files(path_glob=None) -> str:
|
||||
"""List changed files in this PR with added/removed line counts."""
|
||||
@@ -204,9 +250,9 @@ def build_review_agent_tools(diff_files: dict | None = None, augmented_diff: str
|
||||
"type": "function",
|
||||
"name": "read_file",
|
||||
"description": (
|
||||
"Read a bounded line range from a repository file in the checked-out PR head, including unchanged "
|
||||
"files such as pyproject.toml, tests, configs, and shared helpers. Use this to verify changed code or "
|
||||
"nearby definitions before making a review finding."
|
||||
"Read a bounded line range from a repository file at the PR head, including unchanged files such as "
|
||||
"pyproject.toml, tests, configs, and shared helpers. Use this to verify changed code or nearby "
|
||||
"definitions before making a review finding."
|
||||
),
|
||||
"parameters": {
|
||||
"type": "object",
|
||||
@@ -254,32 +300,37 @@ def build_review_agent_tools(diff_files: dict | None = None, augmented_diff: str
|
||||
"strict": True,
|
||||
},
|
||||
]
|
||||
return tools, {
|
||||
handlers = {
|
||||
"list_changed_files": list_changed_files,
|
||||
"read_diff": read_diff,
|
||||
"read_file": read_file,
|
||||
"search_repo": search_repo,
|
||||
"list_files": list_files,
|
||||
}
|
||||
if not local_checkout: # repo text search needs a checkout of the PR head; drop it instead of returning empty
|
||||
tools = [t for t in tools if t.get("name") != "search_repo"]
|
||||
del handlers["search_repo"]
|
||||
return tools, handlers
|
||||
|
||||
|
||||
def get_repo_guidelines(model: str = "") -> str:
|
||||
"""Read guidelines from the repository root if they exist (one agent file + CONTRIBUTING.md)."""
|
||||
def get_repo_guidelines(
|
||||
model: str = "", event: Action = None, head_sha: str | None = None, local_checkout: bool = False
|
||||
) -> str:
|
||||
"""Read repository guidelines (one agent file + CONTRIBUTING.md) from the PR head."""
|
||||
guidelines = []
|
||||
# Prefer CLAUDE.md for Anthropic models, AGENTS.md for others; load only one, never both
|
||||
agent_prefs = ("CLAUDE.md", "AGENTS.md") if "claude" in model.lower() else ("AGENTS.md", "CLAUDE.md")
|
||||
for filename in ("CONTRIBUTING.md", *agent_prefs):
|
||||
try:
|
||||
p = Path(filename)
|
||||
if p.is_file() and p.stat().st_size <= 100_000:
|
||||
content = p.read_text(encoding="utf-8", errors="ignore")[:MAX_CONTEXT_FILE_CHARS]
|
||||
if content:
|
||||
guidelines.append(f"### {filename}\n~~~\n{content}\n~~~")
|
||||
print(f"Loaded {filename} ({len(content)} chars) for review context")
|
||||
if filename in agent_prefs:
|
||||
break # Only load one agent guidelines file
|
||||
content = (_read_head_file(event, head_sha, local_checkout, filename) or "")[:MAX_CONTEXT_FILE_CHARS]
|
||||
except Exception as e:
|
||||
print(f"Failed to read {filename}: {e}")
|
||||
continue
|
||||
if content:
|
||||
guidelines.append(f"### {filename}\n~~~\n{content}\n~~~")
|
||||
print(f"Loaded {filename} ({len(content)} chars) for review context")
|
||||
if filename in agent_prefs:
|
||||
break # Only load one agent guidelines file
|
||||
return f"PROJECT GUIDELINES:\n{chr(10).join(guidelines)}\n\n" if guidelines else ""
|
||||
|
||||
|
||||
@@ -354,37 +405,35 @@ def generate_pr_review(
|
||||
file_list = list(diff_files.keys())
|
||||
lines_changed = sum(len(sides["RIGHT"]) + len(sides["LEFT"]) for sides in diff_files.values())
|
||||
|
||||
# Read model-appropriate guidelines from repo root for project-specific review context
|
||||
# Read model-appropriate guidelines from the PR head for project-specific review context
|
||||
review_model = get_review_model()
|
||||
is_agent_review_model = not _is_anthropic_model(review_model)
|
||||
guidelines_section = get_repo_guidelines(review_model)
|
||||
head_sha = _pr_head_sha(event)
|
||||
local_checkout = _verified_local_checkout(head_sha)
|
||||
if head_sha:
|
||||
print(f"Reviewing PR head {head_sha[:7]} ({'local checkout' if local_checkout else 'via GitHub API'})")
|
||||
guidelines_section = get_repo_guidelines(review_model, event, head_sha, local_checkout)
|
||||
|
||||
# Fetch full file contents for better context if within token budget
|
||||
full_files_section = ""
|
||||
if event and not is_agent_review_model and len(file_list) <= 10: # Reasonable file count limit
|
||||
if event and head_sha and not is_agent_review_model and len(file_list) <= 10: # Reasonable file count limit
|
||||
file_contents, total_chars = [], len(augmented_diff) + len(guidelines_section)
|
||||
for file_path in file_list:
|
||||
for file_path in file_list: # already filtered by should_skip_file above
|
||||
try:
|
||||
p = Path(file_path)
|
||||
if not p.is_file():
|
||||
continue
|
||||
# Skip files matching SKIP_PATTERNS and files >100KB
|
||||
if should_skip_file(file_path) or p.stat().st_size > 100_000:
|
||||
continue
|
||||
snippet = p.read_text(encoding="utf-8", errors="ignore")[:MAX_CONTEXT_FILE_CHARS]
|
||||
if not snippet:
|
||||
continue
|
||||
if len(snippet) == MAX_CONTEXT_FILE_CHARS:
|
||||
snippet = f"{snippet.rstrip()}\n... (truncated)"
|
||||
# Only include if within budget, include buffer for Markdown noise
|
||||
estimated_cost = len(snippet) + 200
|
||||
if total_chars + estimated_cost < MAX_PROMPT_CHARS:
|
||||
file_contents.append(f"### {file_path}\n```\n{snippet}\n```")
|
||||
total_chars += estimated_cost
|
||||
else:
|
||||
break # Stop when we hit budget limit
|
||||
text = _read_head_file(event, head_sha, local_checkout, file_path) or ""
|
||||
except Exception:
|
||||
continue
|
||||
if not text or len(text) > 100_000: # skip missing and >100KB files entirely
|
||||
continue
|
||||
snippet = text[:MAX_CONTEXT_FILE_CHARS]
|
||||
if len(snippet) == MAX_CONTEXT_FILE_CHARS:
|
||||
snippet = f"{snippet.rstrip()}\n... (truncated)"
|
||||
# Only include if within budget, include buffer for Markdown noise
|
||||
estimated_cost = len(snippet) + 200
|
||||
if total_chars + estimated_cost >= MAX_PROMPT_CHARS:
|
||||
break # Stop when we hit budget limit
|
||||
file_contents.append(f"### {file_path}\n```\n{snippet}\n```")
|
||||
total_chars += estimated_cost
|
||||
if file_contents:
|
||||
full_files_section = f"FULL FILE CONTENTS:\n{chr(10).join(file_contents)}\n\n"
|
||||
|
||||
@@ -397,19 +446,24 @@ def generate_pr_review(
|
||||
"CONTEXT AND VERIFICATION:\n"
|
||||
"- Start from the diff, then use tools to inspect full files or related code when context affects a finding\n"
|
||||
"- Verify suspected issues before commenting: read the enclosing function, and check definitions, callers, and existing patterns\n"
|
||||
"- Assume the author is knowledgeable about new package versions, dependencies, and codebase architecture unless tool evidence proves otherwise\n"
|
||||
"- Do NOT flag version updates; check imports or references to code outside the diff with tools instead of flagging them\n"
|
||||
"- If still unsure after checking, assume the author knows what they're doing\n"
|
||||
"- Never claim a name, import, or reference is missing without reading the relevant file first\n"
|
||||
"- Do NOT flag version updates\n"
|
||||
"- Report every issue the tool evidence confirms; drop a suspicion only when the evidence shows the code is correct\n"
|
||||
"- If PROJECT GUIDELINES (CLAUDE.md/AGENTS.md) are provided, respect project-specific conventions and standards\n\n"
|
||||
"AVAILABLE TOOLS:\n"
|
||||
"- list_changed_files: list every changed file in the PR with added/removed counts; use this for large "
|
||||
"PRs or when the initial diff is truncated\n"
|
||||
"- read_diff: inspect the line-numbered diff for a changed file; use this to review files not visible in "
|
||||
"the initial prompt and to recover exact R/L line numbers for comments\n"
|
||||
"- read_file: inspect bounded line ranges from repository files, including unchanged files; the checkout is "
|
||||
"- read_file: inspect bounded line ranges from repository files, including unchanged files; contents are "
|
||||
"the PR head, so file line numbers match RIGHT-side diff line numbers\n"
|
||||
"- search_repo: find related definitions, dependencies, tests, config, or existing patterns across the checkout\n"
|
||||
"- list_files: locate repository files by glob\n"
|
||||
+ (
|
||||
"- search_repo: find related definitions, dependencies, tests, config, or existing patterns across "
|
||||
"the repository\n"
|
||||
if local_checkout
|
||||
else ""
|
||||
)
|
||||
+ "- list_files: locate repository files by glob\n"
|
||||
"- web_search: verify current public docs, package docs, release notes, issues, or vendor behavior when a "
|
||||
"review finding depends on external behavior\n"
|
||||
"- Tool turns and cost are budgeted: batch independent tool calls in the same turn, e.g. read several "
|
||||
@@ -441,7 +495,7 @@ def generate_pr_review(
|
||||
"- Issues in unchanged context lines\n\n"
|
||||
f"{visibility_section}"
|
||||
"QUALITY OVER QUANTITY:\n"
|
||||
"- Zero comments is valid for clean PRs - don't invent issues\n"
|
||||
"- Zero comments is valid for clean PRs - don't invent issues, but don't withhold genuine verified ones\n"
|
||||
"- Each comment must be actionable with clear reasoning\n"
|
||||
"- Combine related issues into one comment\n"
|
||||
f"- Hard cap: {MAX_REVIEW_COMMENTS} comments maximum\n\n"
|
||||
@@ -516,7 +570,7 @@ def generate_pr_review(
|
||||
"additionalProperties": False,
|
||||
}
|
||||
|
||||
tools, tool_handlers = build_review_agent_tools(diff_files, augmented_diff)
|
||||
tools, tool_handlers = build_review_agent_tools(diff_files, augmented_diff, event, head_sha, local_checkout)
|
||||
response = get_agent_response(
|
||||
messages,
|
||||
text_format={"format": {"type": "json_schema", "name": "pr_review", "strict": True, "schema": schema}},
|
||||
@@ -617,6 +671,7 @@ def generate_pr_review(
|
||||
"diff_files": diff_files,
|
||||
"diff_truncated": diff_truncated,
|
||||
"skipped_files": skipped_files,
|
||||
"head_sha": head_sha,
|
||||
}
|
||||
)
|
||||
print(f"Valid comments after filtering: {len(response['comments'])}")
|
||||
@@ -675,13 +730,26 @@ def get_local_head_sha() -> str | None:
|
||||
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
|
||||
|
||||
|
||||
def post_review_summary(event: Action, review_data: dict, review_number: int) -> None:
|
||||
"""Post overall review summary and inline comments as a single PR review."""
|
||||
if not (pr_number := event.pr.get("number")):
|
||||
return
|
||||
|
||||
# Use local HEAD SHA to avoid "Line could not be resolved" errors when auto-format pushed new commits
|
||||
commit_sha = get_local_head_sha() or event.pr.get("head", {}).get("sha")
|
||||
# Anchor to the exact head the review was generated from; fall back to the local checkout, then live head
|
||||
commit_sha = review_data.get("head_sha") or get_local_head_sha() or _pr_head_sha(event)
|
||||
if not commit_sha:
|
||||
return
|
||||
|
||||
|
||||
@@ -90,6 +90,7 @@ ultralytics-actions-summarize-pr = "actions.summarize_pr:main"
|
||||
ultralytics-actions-summarize-release = "actions.summarize_release:main"
|
||||
ultralytics-actions-update-markdown-code-blocks = "actions.update_markdown_code_blocks:main"
|
||||
ultralytics-actions-headers = "actions.update_file_headers:main"
|
||||
ultralytics-actions-format = "actions.format_code:main"
|
||||
ultralytics-actions-format-python-docstrings = "actions.format_python_docstrings:main"
|
||||
ultralytics-actions-dependabot = "actions.dependabot:run"
|
||||
ultralytics-actions-failed-actions = "actions.failed_scheduled_actions:run"
|
||||
|
||||
@@ -3,6 +3,7 @@
|
||||
# Import CLI command modules
|
||||
from actions import (
|
||||
first_interaction,
|
||||
format_code,
|
||||
format_python_docstrings,
|
||||
summarize_pr,
|
||||
summarize_release,
|
||||
@@ -14,6 +15,7 @@ def test_importable_modules():
|
||||
"""Test that all modules can be imported without errors."""
|
||||
# This is a simple test to ensure modules can be imported successfully
|
||||
assert hasattr(first_interaction, "main")
|
||||
assert hasattr(format_code, "main")
|
||||
assert hasattr(summarize_pr, "main")
|
||||
assert hasattr(summarize_release, "main")
|
||||
assert hasattr(update_markdown_code_blocks, "main")
|
||||
|
||||
@@ -2,6 +2,8 @@
|
||||
|
||||
from unittest.mock import MagicMock, patch
|
||||
|
||||
import pytest
|
||||
|
||||
from actions import review_pr
|
||||
from actions.first_interaction import get_event_content, get_first_interaction_response, get_relevant_labels
|
||||
|
||||
@@ -99,12 +101,18 @@ def test_get_first_interaction_response(mock_get_response):
|
||||
mock_get_response.assert_called_once()
|
||||
|
||||
|
||||
@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, tmp_path, monkeypatch):
|
||||
def test_generate_pr_review_uses_synchronous_response(mock_get_agent_response, mock_checkout, tmp_path, monkeypatch):
|
||||
"""Test PR reviews avoid background polling for code diffs."""
|
||||
monkeypatch.chdir(tmp_path)
|
||||
(tmp_path / "test.py").write_text("old = False\n", encoding="utf-8")
|
||||
mock_get_agent_response.return_value = {"comments": [], "summary": "LGTM"}
|
||||
mock_event = MagicMock()
|
||||
mock_event.repository = "ultralytics/actions"
|
||||
mock_event.pr = {"number": 1, "head": {"sha": "headsha"}}
|
||||
api_response = MagicMock(status_code=200, text="")
|
||||
api_response.json.return_value = {"head": {"sha": "headsha"}}
|
||||
mock_event.get.return_value = api_response
|
||||
diff = """diff --git a/test.py b/test.py
|
||||
--- a/test.py
|
||||
+++ b/test.py
|
||||
@@ -113,7 +121,7 @@ def test_generate_pr_review_uses_synchronous_response(mock_get_agent_response, t
|
||||
+old = False
|
||||
"""
|
||||
|
||||
review = review_pr.generate_pr_review("ultralytics/actions", diff, "Test PR", "", event=MagicMock())
|
||||
review = review_pr.generate_pr_review("ultralytics/actions", diff, "Test PR", "", event=mock_event)
|
||||
|
||||
assert review["summary"] == "LGTM"
|
||||
mock_get_agent_response.assert_called_once()
|
||||
@@ -134,10 +142,9 @@ def test_generate_pr_review_uses_synchronous_response(mock_get_agent_response, t
|
||||
assert "FULL FILE CONTENTS" not in mock_get_agent_response.call_args.args[0][1]["content"]
|
||||
|
||||
|
||||
def test_review_agent_tools_can_read_repo_but_not_outside(tmp_path, monkeypatch):
|
||||
"""Test local review tools can inspect unchanged repo files without escaping the checkout."""
|
||||
def test_review_agent_search_scans_local_checkout_only(tmp_path, monkeypatch):
|
||||
"""Test repo search scans the local checkout without escaping it and is dropped without a checkout."""
|
||||
monkeypatch.chdir(tmp_path)
|
||||
(tmp_path / "changed.py").write_text("target = True\n", encoding="utf-8")
|
||||
(tmp_path / "secret.py").write_text("secret = True\n", encoding="utf-8")
|
||||
(tmp_path / "patterns.py").write_text("literal = '(a+)+$'\n", encoding="utf-8")
|
||||
workflow = tmp_path / ".github" / "workflows" / "format.yml"
|
||||
@@ -146,17 +153,15 @@ def test_review_agent_tools_can_read_repo_but_not_outside(tmp_path, monkeypatch)
|
||||
outside = tmp_path.parent / "outside_secret.py"
|
||||
outside.write_text("outside_secret = True\n", encoding="utf-8")
|
||||
(tmp_path / "linked_secret.py").symlink_to(outside)
|
||||
(tmp_path / "linked_internal.py").symlink_to(tmp_path / "secret.py")
|
||||
|
||||
_, handlers = review_pr.build_review_agent_tools()
|
||||
tools, handlers = review_pr.build_review_agent_tools(local_checkout=True)
|
||||
|
||||
assert "target = True" in handlers["read_file"](path="changed.py", start_line=None, end_line=None)
|
||||
assert "secret = True" in handlers["read_file"](path="secret.py", start_line=None, end_line=None)
|
||||
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"](
|
||||
path="linked_secret.py", start_line=None, end_line=None
|
||||
)
|
||||
listed_files = handlers["list_files"](path_glob=None).splitlines()
|
||||
assert "secret.py" in listed_files
|
||||
assert "secret.py" in handlers["list_files"](path_glob=None).splitlines()
|
||||
assert ".github/workflows/format.yml" in handlers["list_files"](path_glob=".github/**").splitlines()
|
||||
assert "secret.py:1:secret = True" in handlers["search_repo"](query="secret = True", path_glob=None)
|
||||
assert "No matches found." == handlers["search_repo"](query="secret.*True", path_glob=None)
|
||||
@@ -164,6 +169,73 @@ def test_review_agent_tools_can_read_repo_but_not_outside(tmp_path, monkeypatch)
|
||||
assert ".github/workflows/format.yml:1:workflow = True" in handlers["search_repo"](
|
||||
query="workflow = True", path_glob=".github/**"
|
||||
)
|
||||
assert "No matches found." == handlers["search_repo"](query="outside_secret = True", path_glob=None)
|
||||
|
||||
tools, handlers = review_pr.build_review_agent_tools(local_checkout=False)
|
||||
assert "search_repo" not in handlers
|
||||
assert "search_repo" not in {t.get("name") for t in tools}
|
||||
|
||||
|
||||
def test_pr_head_sha_prefers_live_value():
|
||||
"""Test head SHA resolution prefers the live PR value and falls back to the event payload."""
|
||||
event = MagicMock()
|
||||
event.repository = "org/repo"
|
||||
event.pr = {"number": 5, "head": {"sha": "old"}}
|
||||
live = MagicMock(status_code=200)
|
||||
live.json.return_value = {"head": {"sha": "new"}}
|
||||
event.get.return_value = live
|
||||
assert review_pr._pr_head_sha(event) == "new"
|
||||
event.get.return_value = MagicMock(status_code=500)
|
||||
assert review_pr._pr_head_sha(event) == "old"
|
||||
assert review_pr._pr_head_sha(None) is None
|
||||
|
||||
|
||||
def test_review_agent_tools_read_pr_head_via_api(tmp_path, monkeypatch):
|
||||
"""Test review tools serve PR-head content via the GitHub API regardless of local checkout."""
|
||||
monkeypatch.chdir(tmp_path)
|
||||
event = MagicMock()
|
||||
event.repository = "org/repo"
|
||||
event.headers = {"Authorization": "Bearer x"}
|
||||
file_response = MagicMock(status_code=200, text="import torch\ntorch.zeros(1)\n")
|
||||
missing_response = MagicMock(status_code=404)
|
||||
tree_response = MagicMock(status_code=200)
|
||||
tree_response.json.return_value = {
|
||||
"tree": [{"path": "tests/test_python.py", "type": "blob"}, {"path": "docs", "type": "tree"}]
|
||||
}
|
||||
|
||||
def fake_get(url, **kwargs):
|
||||
if "/git/trees/" in url:
|
||||
return tree_response
|
||||
if "missing.py" in url:
|
||||
return missing_response
|
||||
return MagicMock(status_code=500) if "flaky.py" in url else file_response
|
||||
|
||||
event.get.side_effect = fake_get
|
||||
|
||||
_, handlers = review_pr.build_review_agent_tools({}, "", event, "a" * 40, local_checkout=False)
|
||||
|
||||
excerpt = handlers["read_file"](path="tests/test_python.py", start_line=1, end_line=1)
|
||||
assert "1: import torch" in excerpt
|
||||
assert handlers["read_file"](path="missing.py", start_line=None, end_line=None).endswith(
|
||||
"does not exist at the PR head."
|
||||
)
|
||||
with pytest.raises(RuntimeError): # transient API errors must not read as "file missing"
|
||||
handlers["read_file"](path="flaky.py", start_line=None, end_line=None)
|
||||
|
||||
tree_response.status_code = 500
|
||||
assert handlers["list_files"](path_glob=None) == "list_files failed: HTTP 500." # failures are not cached
|
||||
tree_response.status_code = 200
|
||||
assert handlers["list_files"](path_glob=None).splitlines() == ["tests/test_python.py"]
|
||||
|
||||
|
||||
@patch("actions.review_pr._fetch_head_file")
|
||||
def test_get_repo_guidelines_fetches_pr_head(mock_fetch):
|
||||
"""Test guidelines are fetched from the PR head via the GitHub API."""
|
||||
mock_fetch.side_effect = lambda event, sha, path: "Be nice" if path == "AGENTS.md" else None
|
||||
section = review_pr.get_repo_guidelines("gpt-5.5", event=MagicMock(), head_sha="abc")
|
||||
assert "AGENTS.md" in section and "Be nice" in section
|
||||
assert "CONTRIBUTING.md" not in section
|
||||
assert review_pr.get_repo_guidelines("gpt-5.5", event=None, head_sha="abc") == ""
|
||||
|
||||
|
||||
def test_review_agent_tools_can_list_and_read_changed_file_diffs():
|
||||
|
||||
@@ -0,0 +1,38 @@
|
||||
# Ultralytics 🚀 AGPL-3.0 License - https://ultralytics.com/license
|
||||
|
||||
from pathlib import Path
|
||||
from unittest.mock import call, patch
|
||||
|
||||
from actions import format_code
|
||||
|
||||
ACTION_YML = Path(__file__).parents[1] / "action.yml"
|
||||
|
||||
|
||||
def test_format_commands_match_action_yml():
|
||||
"""Test packaged formatter commands stay aligned with the composite action steps."""
|
||||
text = ACTION_YML.read_text(encoding="utf-8")
|
||||
for arg in [*format_code.RUFF_CHECK[1:], *format_code.RUFF_FORMAT[1:], format_code.DOCSTRINGS[0]]:
|
||||
assert arg == "." or arg.replace("=", " ") in text, f"ruff arg not in action.yml: {arg}"
|
||||
for line in format_code.PRETTIER.strip().splitlines():
|
||||
assert line.strip() in text, f"prettier line not in action.yml: {line.strip()}"
|
||||
for arg in format_code.CODESPELL[1:]:
|
||||
assert arg == "*" or arg in text, f"codespell arg not in action.yml: {arg}"
|
||||
|
||||
|
||||
@patch("actions.format_code.subprocess.run")
|
||||
def test_format_main_runs_all_formatters(mock_run, monkeypatch):
|
||||
"""Test format CLI runs every formatter by default and respects INPUTS_* opt-outs."""
|
||||
format_code.main()
|
||||
assert mock_run.call_args_list == [
|
||||
call(format_code.RUFF_CHECK, shell=False, check=True),
|
||||
call(format_code.RUFF_FORMAT, shell=False, check=True),
|
||||
call(format_code.DOCSTRINGS, shell=False, check=True),
|
||||
call(format_code.PRETTIER, shell=True, check=True),
|
||||
call(format_code.CODESPELL, shell=False, check=True),
|
||||
]
|
||||
|
||||
mock_run.reset_mock()
|
||||
monkeypatch.setenv("INPUTS_PYTHON", "false")
|
||||
monkeypatch.setenv("INPUTS_SPELLING", "false")
|
||||
format_code.main()
|
||||
assert mock_run.call_args_list == [call(format_code.PRETTIER, shell=True, check=True)]
|
||||
Reference in New Issue
Block a user