Number PR reviews and carry prior review context forward (#859)
This commit is contained in:
+1
-1
@@ -28,4 +28,4 @@
|
||||
# ├── test_summarize_pr.py
|
||||
# └── ...
|
||||
|
||||
__version__ = "0.2.47"
|
||||
__version__ = "0.3.0"
|
||||
|
||||
@@ -216,14 +216,7 @@ def main(*args, **kwargs):
|
||||
# Automatic PR review after first interaction
|
||||
if AUTO_PR_REVIEW:
|
||||
print("Starting automatic PR review...")
|
||||
try:
|
||||
review_diff, head_sha = event.get_pr_diff_snapshot()
|
||||
except RuntimeError as e:
|
||||
print(f"Skipping stale PR review: {e}")
|
||||
return
|
||||
review_data = review_pr.generate_pr_review(event.repository, review_diff, title, body, event, head_sha)
|
||||
review_pr.post_review_summary(event, review_data)
|
||||
print("PR review completed")
|
||||
review_pr.run_review(event, title, body)
|
||||
return
|
||||
|
||||
# Handle issues and discussions (NOT PRs)
|
||||
|
||||
+186
-28
@@ -32,6 +32,10 @@ MAX_REVIEW_COMMENTS = 8
|
||||
MAX_TOOL_OUTPUT_CHARS = 20000
|
||||
MAX_TOOL_FILE_LINES = 240
|
||||
MAX_AGENT_TURNS = 16
|
||||
MAX_HISTORY_REVIEWS = 5 # prior reviews included in the prompt (the full history stays available via the tool)
|
||||
MAX_HISTORY_ITEM_CHARS = 8000 # per prior review or response, enough for a full summary plus its findings log
|
||||
MAX_HISTORY_CHARS = 20000
|
||||
MAX_FINDING_LOG_CHARS = 500 # per finding logged in the review body, the record that outlives its inline comment
|
||||
REVIEW_COST_SOFT_LIMIT = 2.00 # stop requesting tools after cumulative spend reaches this amount
|
||||
SEVERITY_RANK = {"CRITICAL": 0, "HIGH": 1, "MEDIUM": 2, "LOW": 3, "SUGGESTION": 4, None: 5}
|
||||
|
||||
@@ -83,6 +87,75 @@ def search_repo(query: str, path_glob=None) -> str:
|
||||
return _clip_tool_output("\n".join(matches)) if matches else "No matches found."
|
||||
|
||||
|
||||
def _clip(text: str, limit: int | None) -> str:
|
||||
"""Clip text to at most limit characters, or return it unchanged when no limit is given."""
|
||||
return text if not limit or len(text) <= limit else f"{text[: limit - 1].rstrip()}…"
|
||||
|
||||
|
||||
def _build_review_history(reviews: list[dict], comments: list[dict], bot_username: str | None) -> dict:
|
||||
"""Capture prior bot reviews and the responses to them; each review body carries its own findings log."""
|
||||
parents = {comment.get("id"): comment for comment in comments}
|
||||
owned_ids = {review.get("id") for review in reviews}
|
||||
prior = []
|
||||
for number, review in enumerate(reviews, 1):
|
||||
body = re.sub(rf"{re.escape(REVIEW_MARKER)} *\d*", "", review.get("body") or "").replace(ACTIONS_CREDIT, "")
|
||||
prior.append(
|
||||
{
|
||||
"number": number,
|
||||
"commit": (review.get("commit_id") or "")[:7],
|
||||
"body": body.split("<details><summary>📋 Skipped")[0].strip(),
|
||||
}
|
||||
)
|
||||
|
||||
replies, others = [], []
|
||||
for comment in comments:
|
||||
if (login := comment.get("user", {}).get("login")) == bot_username:
|
||||
continue
|
||||
parent = parents.get(comment.get("in_reply_to_id")) or {}
|
||||
line = comment.get("line") or comment.get("original_line") or 0
|
||||
entry = f"- {comment.get('path')}:{line} @{login} ({comment.get('author_association') or 'NONE'})"
|
||||
body = (comment.get("body") or "").strip()
|
||||
if parent.get("pull_request_review_id") in owned_ids: # a reply to one of our own findings
|
||||
replies.append(f'{entry} on "{_clip((parent.get("body") or "").strip(), 120)}": {body}')
|
||||
else:
|
||||
others.append(f"{entry}: {body}")
|
||||
return {"reviews": prior, "replies": replies, "others": others}
|
||||
|
||||
|
||||
def _fit(entries: list[str], budget: int | None, separator: int = 1) -> tuple[list[str], int]:
|
||||
"""Keep the newest entries that fit the budget, oldest first, with the count of those dropped."""
|
||||
kept, used = [], 0
|
||||
for entry in reversed(entries):
|
||||
cost = len(entry) + (separator if kept else 0) # separators only sit between entries
|
||||
if budget and used + cost > budget:
|
||||
break
|
||||
kept.append(entry)
|
||||
used += cost
|
||||
return kept[::-1], len(entries) - len(kept)
|
||||
|
||||
|
||||
def _format_review_history(
|
||||
history: dict, limit: int | None = None, clip: int | None = None, budget: int | None = None
|
||||
) -> str:
|
||||
"""Render prior reviews and the responses to them as review context, oldest dropped first when over budget."""
|
||||
sections = []
|
||||
share = budget // 4 if budget else None # each response section gets a quarter, leaving half for the reviews
|
||||
for title, key in (("Other review comments on this PR", "others"), ("Replies to your findings", "replies")):
|
||||
if entries := history.get(key):
|
||||
kept, dropped = _fit([_clip(e, share or clip) for e in entries], share) # clipped to fit, never dropped
|
||||
note = f" ({dropped} older comment(s) omitted for length)" if dropped else ""
|
||||
sections.append(f"### {title}{note}\n" + "\n".join(kept))
|
||||
|
||||
reviews = history.get("reviews") or []
|
||||
shown = reviews[-limit:] if limit else reviews
|
||||
rendered = [f"### Review {r['number']} (commit {r['commit']})\n{_clip(r['body'], clip)}" for r in shown]
|
||||
remaining = budget - sum(len(s) + 2 for s in sections) if budget else None
|
||||
kept, dropped = _fit(rendered, remaining, separator=2)
|
||||
if omitted := dropped + len(reviews) - len(shown):
|
||||
kept.insert(0, f"### {omitted} older review(s) omitted for length")
|
||||
return "\n\n".join(kept + sections)
|
||||
|
||||
|
||||
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}"
|
||||
@@ -131,11 +204,14 @@ def build_review_agent_tools(
|
||||
event: Action = None,
|
||||
head_sha: str | None = None,
|
||||
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."""
|
||||
diff_files = diff_files or {}
|
||||
review_history = review_history or {}
|
||||
diff_chunks = _split_augmented_diff_by_file(augmented_diff)
|
||||
head_tree = [] # cached PR head file listing
|
||||
discussion = [] # cached PR discussion comments
|
||||
|
||||
def read_file(path: str, start_line=None, end_line=None) -> str:
|
||||
"""Read a bounded line range from a file at the PR head."""
|
||||
@@ -193,6 +269,21 @@ def build_review_agent_tools(
|
||||
return f"{path} has no diff lines in requested range {start}-{end}."
|
||||
return _clip_tool_output(f"{path} diff:{start}-{end}\n" + "\n".join(lines[start - 1 : end]))
|
||||
|
||||
def read_pr_conversation() -> str:
|
||||
"""Read every prior review of this PR in full, plus the discussion on it."""
|
||||
parts = [_format_review_history(review_history, budget=MAX_TOOL_OUTPUT_CHARS)]
|
||||
if event and (pr_number := (event.pr or {}).get("number")) and not discussion:
|
||||
url = f"{GITHUB_API_URL}/repos/{event.repository}/issues/{pr_number}/comments"
|
||||
discussion.append(
|
||||
[
|
||||
f"@{c.get('user', {}).get('login')}: {_clip((c.get('body') or '').strip(), 1000)}"
|
||||
for c in event.paginate(url, hard=True)
|
||||
]
|
||||
)
|
||||
if discussion and discussion[0]:
|
||||
parts.append("### PR discussion comments\n" + "\n".join(discussion[0]))
|
||||
return _clip_tool_output("\n\n".join(parts))
|
||||
|
||||
tools = [
|
||||
{"type": "web_search"},
|
||||
{
|
||||
@@ -231,6 +322,17 @@ def build_review_agent_tools(
|
||||
},
|
||||
"strict": True,
|
||||
},
|
||||
{
|
||||
"type": "function",
|
||||
"name": "read_pr_conversation",
|
||||
"description": (
|
||||
"Read every earlier review of this PR in full - their summaries, inline findings, and the replies "
|
||||
"to them - plus the PR discussion comments. Use this before repeating, contradicting, or "
|
||||
"reversing an earlier finding, and whenever the PRIOR REVIEWS excerpt in the prompt is truncated."
|
||||
),
|
||||
"parameters": {"type": "object", "properties": {}, "required": [], "additionalProperties": False},
|
||||
"strict": True,
|
||||
},
|
||||
{
|
||||
"type": "function",
|
||||
"name": "read_file",
|
||||
@@ -289,12 +391,16 @@ def build_review_agent_tools(
|
||||
"list_changed_files": list_changed_files,
|
||||
"read_diff": read_diff,
|
||||
"read_file": read_file,
|
||||
"read_pr_conversation": read_pr_conversation,
|
||||
"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"]
|
||||
if not review_history.get("reviews"): # nothing to read back on a first review
|
||||
tools = [t for t in tools if t.get("name") != "read_pr_conversation"]
|
||||
del handlers["read_pr_conversation"]
|
||||
return tools, handlers
|
||||
|
||||
|
||||
@@ -368,8 +474,11 @@ def generate_pr_review(
|
||||
pr_description: str,
|
||||
event: Action = None,
|
||||
head_sha: str | None = None,
|
||||
review_history: dict | None = None,
|
||||
) -> dict:
|
||||
"""Generate comprehensive PR review with line-specific comments and overall assessment."""
|
||||
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)
|
||||
if diff_text.startswith("ERROR:"):
|
||||
return {"comments": [], "summary": f"{ERROR_MARKER}: {diff_text}", "head_sha": head_sha}
|
||||
@@ -404,10 +513,19 @@ def generate_pr_review(
|
||||
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)
|
||||
|
||||
# Carry prior reviews of this PR forward so findings are not repeated, contradicted, or silently reversed
|
||||
history_section = ""
|
||||
if prior_reviews:
|
||||
excerpt = _format_review_history(
|
||||
review_history, limit=MAX_HISTORY_REVIEWS, clip=MAX_HISTORY_ITEM_CHARS, budget=MAX_HISTORY_CHARS
|
||||
)
|
||||
history_section = f"PRIOR REVIEWS OF THIS PR (oldest first):\n{excerpt}\n\n"
|
||||
print(f"Loaded {len(prior_reviews)} prior review(s) ({len(history_section)} chars) for review context")
|
||||
|
||||
# Fetch full file contents for better context if within token budget
|
||||
full_files_section = ""
|
||||
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)
|
||||
file_contents, total_chars = [], len(augmented_diff) + len(guidelines_section) + len(history_section)
|
||||
for file_path in file_list: # already filtered by should_skip_file above
|
||||
text = _read_head_file(event, head_sha, local_checkout, file_path) or ""
|
||||
if not text or len(text) > 100_000: # skip missing and >100KB files entirely
|
||||
@@ -425,7 +543,7 @@ def generate_pr_review(
|
||||
full_files_section = f"FULL FILE CONTENTS:\n{chr(10).join(file_contents)}\n\n"
|
||||
|
||||
# Calculate remaining budget for diff and check if truncation needed
|
||||
diff_budget = max(1000, MAX_PROMPT_CHARS - len(guidelines_section) - len(full_files_section))
|
||||
diff_budget = max(1000, MAX_PROMPT_CHARS - len(guidelines_section) - len(full_files_section) - len(history_section))
|
||||
diff_truncated = len(augmented_diff) > diff_budget
|
||||
is_large_pr = diff_truncated or len(file_list) > 30
|
||||
if is_agent_review_model: # must match the get_agent_response fallback gate
|
||||
@@ -452,6 +570,29 @@ def generate_pr_review(
|
||||
"- If PROJECT GUIDELINES (CLAUDE.md/AGENTS.md) are provided, respect project-specific conventions and standards\n\n"
|
||||
)
|
||||
|
||||
continuity_section = ""
|
||||
if prior_reviews:
|
||||
continuity_section = (
|
||||
f"CONTINUITY - you already reviewed this PR {len(prior_reviews)} time(s), this is review "
|
||||
f"{len(prior_reviews) + 1}:\n"
|
||||
"- PRIOR REVIEWS below carries your earlier summaries and findings plus the replies to them; the diff "
|
||||
"shows the current state\n"
|
||||
"- Never contradict yourself without cause: a change made to satisfy an earlier finding is not a new "
|
||||
"problem, and an earlier finding is reversed only with concrete new evidence, stated as such with the "
|
||||
"review number it reverses\n"
|
||||
"- Drop findings the current diff resolves; repeat an unresolved one only while it still applies, since "
|
||||
"its inline comment was deleted with the superseded review\n"
|
||||
"- A reply under 'Replies to your findings' that rejects or explains one settles it: do not raise it "
|
||||
"again. Everything under 'Other review comments' is context, not a verdict on your findings\n"
|
||||
+ (
|
||||
"- Call read_pr_conversation before repeating or reversing a finding, and whenever the excerpt below "
|
||||
"is truncated\n"
|
||||
if is_agent_review_model
|
||||
else ""
|
||||
)
|
||||
+ "- Open the summary with what changed since the last review: addressed, still open, newly introduced\n\n"
|
||||
)
|
||||
|
||||
content = (
|
||||
"You are an expert code reviewer for Ultralytics. Review code changes and provide inline comments ONLY for genuine issues.\n\n"
|
||||
"WHEN TO COMMENT (priority order):\n"
|
||||
@@ -466,6 +607,7 @@ def generate_pr_review(
|
||||
"- 'Consider using X' without clear benefit\n"
|
||||
"- Issues in unchanged context lines\n\n"
|
||||
f"{visibility_section}"
|
||||
f"{continuity_section}"
|
||||
"QUALITY OVER QUANTITY:\n"
|
||||
"- Zero comments is valid for clean PRs: never invent an issue, never withhold an evidence-backed one\n"
|
||||
"- Each comment must be actionable with clear reasoning\n"
|
||||
@@ -502,6 +644,7 @@ def generate_pr_review(
|
||||
f"TITLE:\n{pr_title}\n\n"
|
||||
f"BODY:\n{remove_html_comments(pr_description or '')[:1000]}\n\n"
|
||||
f"{guidelines_section}"
|
||||
f"{history_section}"
|
||||
f"{full_files_section}"
|
||||
f"DIFF:\n{augmented_diff[:diff_budget]}\n\n"
|
||||
"Now review this diff according to the rules above. Return JSON with comments array and summary."
|
||||
@@ -541,7 +684,9 @@ def generate_pr_review(
|
||||
"additionalProperties": False,
|
||||
}
|
||||
|
||||
tools, tool_handlers = build_review_agent_tools(diff_files, augmented_diff, event, head_sha, local_checkout)
|
||||
tools, tool_handlers = build_review_agent_tools(
|
||||
diff_files, augmented_diff, event, head_sha, local_checkout, review_history
|
||||
)
|
||||
response = get_agent_response(
|
||||
messages,
|
||||
text_format={"format": {"type": "json_schema", "name": "pr_review", "strict": True, "schema": schema}},
|
||||
@@ -686,32 +831,35 @@ def _verified_local_checkout(head_sha: str | None) -> bool:
|
||||
return False
|
||||
|
||||
|
||||
def clear_previous_review(event: Action) -> None:
|
||||
"""Dismiss the bot's active review decisions and delete its superseded inline comments."""
|
||||
def clear_previous_review(event: Action) -> dict:
|
||||
"""Capture the bot's prior reviews, then dismiss their decisions and delete their superseded inline comments."""
|
||||
pr_number, bot_username = event.pr.get("number"), event.get_username()
|
||||
reviews_base = f"{GITHUB_API_URL}/repos/{event.repository}/pulls/{pr_number}/reviews"
|
||||
reviews = event.get(reviews_base, params={"per_page": 100}, hard=True).json()
|
||||
owned_reviews = {
|
||||
review["id"]
|
||||
reviews = event.paginate(reviews_base, hard=True)
|
||||
owned = [
|
||||
review
|
||||
for review in reviews
|
||||
if review.get("user", {}).get("login") == bot_username and REVIEW_MARKER in (review.get("body") or "")
|
||||
}
|
||||
for review in reviews:
|
||||
if review.get("id") in owned_reviews and review.get("state") in ("APPROVED", "CHANGES_REQUESTED"):
|
||||
]
|
||||
comments_base = f"{GITHUB_API_URL}/repos/{event.repository}/pulls/{pr_number}/comments"
|
||||
comments = event.paginate(comments_base, hard=True)
|
||||
history = _build_review_history(owned, comments, bot_username) # capture responses before deleting the comments
|
||||
|
||||
owned_reviews = {review["id"] for review in owned}
|
||||
for review in owned:
|
||||
if review.get("state") in ("APPROVED", "CHANGES_REQUESTED"):
|
||||
event.put(
|
||||
f"{reviews_base}/{review['id']}/dismissals",
|
||||
json={"message": "Superseded by new review"},
|
||||
hard=True,
|
||||
)
|
||||
|
||||
comments_base = f"{GITHUB_API_URL}/repos/{event.repository}/pulls/{pr_number}/comments"
|
||||
comments = event.get(comments_base, params={"per_page": 100}, hard=True).json()
|
||||
for comment in comments:
|
||||
if comment.get("pull_request_review_id") in owned_reviews:
|
||||
event.delete(f"{GITHUB_API_URL}/repos/{event.repository}/pulls/comments/{comment['id']}", hard=True)
|
||||
return history
|
||||
|
||||
|
||||
def post_review_summary(event: Action, review_data: dict) -> None:
|
||||
def post_review_summary(event: Action, review_data: dict, review_number: int = 1) -> None:
|
||||
"""Post overall review summary and inline comments as a single PR review."""
|
||||
if not (pr_number := event.pr.get("number")):
|
||||
return
|
||||
@@ -734,10 +882,17 @@ def post_review_summary(event: Action, review_data: dict) -> None:
|
||||
else "APPROVE"
|
||||
)
|
||||
|
||||
body = f"{REVIEW_MARKER}\n\n{ACTIONS_CREDIT}\n\n{summary[:3000]}\n\n"
|
||||
body = f"{REVIEW_MARKER} {review_number}\n\n{ACTIONS_CREDIT}\n\n{summary[:3000]}\n\n"
|
||||
|
||||
if comments:
|
||||
body += f"💬 Posted {len(comments)} inline comment{'s' if len(comments) != 1 else ''}\n"
|
||||
# Log findings in the review body: inline comments are deleted when the next review supersedes this one
|
||||
findings = "\n".join(
|
||||
f"- {EMOJI_MAP.get(c.get('severity'), '💭')} **{c.get('severity') or 'SUGGESTION'}** "
|
||||
f"`{c.get('file')}:{c.get('line')}` {_clip((c.get('message') or '').strip(), MAX_FINDING_LOG_CHARS)}"
|
||||
for c in comments
|
||||
)
|
||||
summary_text = f"💬 Posted {len(comments)} inline comment{'s' if len(comments) != 1 else ''}"
|
||||
body += f"<details><summary>{summary_text}</summary>\n\n{findings}\n</details>\n"
|
||||
|
||||
if review_data.get("diff_truncated"):
|
||||
body += "\n⚠️ **Large PR**: Review focused on critical issues. Some details may not be covered.\n"
|
||||
@@ -779,6 +934,19 @@ def post_review_summary(event: Action, review_data: dict) -> None:
|
||||
event.post(f"{GITHUB_API_URL}/repos/{event.repository}/pulls/{pr_number}/reviews", json=payload, hard=True)
|
||||
|
||||
|
||||
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
|
||||
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)
|
||||
print("PR review completed")
|
||||
|
||||
|
||||
def main(*args, **kwargs):
|
||||
"""Main entry point for PR review action."""
|
||||
event = Action(*args, **kwargs)
|
||||
@@ -798,17 +966,7 @@ def main(*args, **kwargs):
|
||||
return
|
||||
|
||||
print(f"Starting PR review for #{event.pr['number']}")
|
||||
try:
|
||||
diff, head_sha = event.get_pr_diff_snapshot()
|
||||
except RuntimeError as e:
|
||||
print(f"Skipping stale PR review: {e}")
|
||||
return
|
||||
clear_previous_review(event)
|
||||
review = generate_pr_review(
|
||||
event.repository, diff, event.pr.get("title") or "", event.pr.get("body") or "", event, head_sha
|
||||
)
|
||||
post_review_summary(event, review)
|
||||
print("PR review completed")
|
||||
run_review(event, event.pr.get("title") or "", event.pr.get("body") or "")
|
||||
|
||||
|
||||
if __name__ == "__main__":
|
||||
|
||||
@@ -164,6 +164,16 @@ class Action:
|
||||
"""Performs GET request with error handling."""
|
||||
return self._request("get", url, **kwargs)
|
||||
|
||||
def paginate(self, url, params=None, **kwargs) -> list:
|
||||
"""Fetch every page of a GitHub REST collection."""
|
||||
items = []
|
||||
for page in range(1, 102): # one page past the cap, so an exactly-10,000-item collection still completes
|
||||
page_items = self.get(url, params={**(params or {}), "per_page": 100, "page": page}, **kwargs).json()
|
||||
items.extend(page_items)
|
||||
if len(page_items) < 100:
|
||||
return items
|
||||
raise RuntimeError(f"GitHub collection exceeded 10,000 items: {url}")
|
||||
|
||||
def post(self, url, **kwargs):
|
||||
"""Performs POST request with error handling."""
|
||||
return self._request("post", url, **kwargs)
|
||||
|
||||
@@ -62,8 +62,7 @@ def test_get_event_content_pr():
|
||||
assert action == "opened"
|
||||
|
||||
|
||||
@patch("actions.first_interaction.review_pr.post_review_summary")
|
||||
@patch("actions.first_interaction.review_pr.generate_pr_review")
|
||||
@patch("actions.first_interaction.review_pr.run_review")
|
||||
@patch("actions.first_interaction.get_pr_open_response")
|
||||
@patch("actions.first_interaction.get_event_content")
|
||||
@patch("actions.first_interaction.Action")
|
||||
@@ -80,11 +79,10 @@ def test_get_event_content_pr():
|
||||
("", "Generated summary"),
|
||||
],
|
||||
)
|
||||
def test_open_pr_review_description(mock_action, mock_content, mock_response, mock_review, mock_post, body, expected):
|
||||
def test_open_pr_review_description(mock_action, mock_content, mock_response, mock_review, body, expected):
|
||||
"""Test automatic reviews use the best available author and generated description context."""
|
||||
mock_content.return_value = (456, "node456", "Test PR", body, "testuser", "pull request", "opened")
|
||||
mock_response.return_value = {"summary": "Generated summary", "labels": [], "first_comment": ""}
|
||||
mock_review.return_value = {"head_sha": "headsha", "summary": "LGTM", "comments": []}
|
||||
event = mock_action.return_value
|
||||
event.should_skip_llm.return_value = False
|
||||
event.should_skip_pr_author.return_value = False
|
||||
@@ -94,8 +92,7 @@ def test_open_pr_review_description(mock_action, mock_content, mock_response, mo
|
||||
|
||||
first_interaction.main()
|
||||
|
||||
mock_review.assert_called_once_with(event.repository, "review diff", "Test PR", expected, event, "headsha")
|
||||
mock_post.assert_called_once_with(event, mock_review.return_value)
|
||||
mock_review.assert_called_once_with(event, "Test PR", expected)
|
||||
|
||||
|
||||
@patch("actions.first_interaction.get_response")
|
||||
@@ -240,8 +237,8 @@ def test_review_snapshot_retries_until_diff_and_head_match():
|
||||
assert event.get_pr_head_sha.call_count == 4
|
||||
|
||||
|
||||
def test_post_review_summary_fails_when_github_rejects_review():
|
||||
"""Test review publication is a required operation rather than a silent best effort."""
|
||||
def test_post_review_summary_numbers_reviews_and_logs_findings():
|
||||
"""Test review publication is required and each review is numbered with a findings log that outlives it."""
|
||||
event = MagicMock()
|
||||
event.repository = "org/repo"
|
||||
event.pr = {"number": 7}
|
||||
@@ -250,33 +247,81 @@ def test_post_review_summary_fails_when_github_rejects_review():
|
||||
review_pr.post_review_summary(event, {"head_sha": "abc", "summary": "LGTM", "comments": []})
|
||||
|
||||
assert event.post.call_args.kwargs["hard"] is True
|
||||
assert event.post.call_args.kwargs["json"]["body"].startswith(f"{review_pr.REVIEW_MARKER} 1")
|
||||
|
||||
comment = {"file": "a.py", "line": 3, "side": "RIGHT", "severity": "HIGH", "message": "retry loop never exits"}
|
||||
review_pr.post_review_summary(event, {"head_sha": "abc", "summary": "Fix", "comments": [comment]}, 3)
|
||||
|
||||
body = event.post.call_args.kwargs["json"]["body"]
|
||||
assert body.startswith(f"{review_pr.REVIEW_MARKER} 3")
|
||||
assert "**HIGH** `a.py:3` retry loop never exits" in body
|
||||
|
||||
|
||||
def test_clear_previous_review_preserves_summaries_and_deletes_inline_comments():
|
||||
"""Test replacement reviews invalidate bot decisions and remove only bot inline comments."""
|
||||
def test_clear_previous_review_captures_history_and_deletes_inline_comments():
|
||||
"""Test replacement reviews capture prior context, invalidate bot decisions, and remove only bot inline comments."""
|
||||
event = MagicMock()
|
||||
event.repository = "org/repo"
|
||||
event.pr = {"number": 7}
|
||||
event.get_username.return_value = "review-bot"
|
||||
event.get.side_effect = [
|
||||
MagicMock(
|
||||
json=lambda: [
|
||||
{"id": 1, "state": "APPROVED", "body": review_pr.REVIEW_MARKER, "user": {"login": "review-bot"}},
|
||||
{"id": 2, "state": "COMMENTED", "body": review_pr.REVIEW_MARKER, "user": {"login": "review-bot"}},
|
||||
{"id": 3, "state": "APPROVED", "body": "Human review", "user": {"login": "human"}},
|
||||
{"id": 6, "state": "COMMENTED", "body": "Other automation", "user": {"login": "review-bot"}},
|
||||
]
|
||||
),
|
||||
MagicMock(
|
||||
json=lambda: [
|
||||
{"id": 4, "pull_request_review_id": 1, "user": {"login": "review-bot"}},
|
||||
{"id": 5, "pull_request_review_id": 6, "user": {"login": "review-bot"}},
|
||||
]
|
||||
),
|
||||
event.paginate.side_effect = [
|
||||
[
|
||||
{
|
||||
"id": 1,
|
||||
"state": "APPROVED",
|
||||
"commit_id": "a" * 40,
|
||||
"body": f"{review_pr.REVIEW_MARKER} 1\n\nRetry loop needs a bound",
|
||||
"user": {"login": "review-bot"},
|
||||
},
|
||||
{
|
||||
"id": 2,
|
||||
"state": "COMMENTED",
|
||||
"commit_id": "b" * 40,
|
||||
"body": f"{review_pr.REVIEW_MARKER} 2\n\nBound is correct now",
|
||||
"user": {"login": "review-bot"},
|
||||
},
|
||||
{"id": 3, "state": "APPROVED", "body": "Human review", "user": {"login": "human"}},
|
||||
{"id": 6, "state": "COMMENTED", "body": "Other automation", "user": {"login": "review-bot"}},
|
||||
],
|
||||
[
|
||||
{
|
||||
"id": 4,
|
||||
"pull_request_review_id": 1,
|
||||
"user": {"login": "review-bot"},
|
||||
"path": "a.py",
|
||||
"line": 3,
|
||||
"body": "⚠️ **HIGH**: retry loop never exits",
|
||||
},
|
||||
{"id": 5, "pull_request_review_id": 6, "user": {"login": "review-bot"}},
|
||||
{
|
||||
"id": 8,
|
||||
"pull_request_review_id": 3,
|
||||
"in_reply_to_id": 4,
|
||||
"user": {"login": "human"},
|
||||
"path": "a.py",
|
||||
"line": 3,
|
||||
"body": "intentional, the caller times out",
|
||||
"author_association": "MEMBER",
|
||||
},
|
||||
{
|
||||
"id": 9,
|
||||
"pull_request_review_id": 3,
|
||||
"user": {"login": "human"},
|
||||
"path": "b.py",
|
||||
"line": 9,
|
||||
"body": "unrelated comment on someone else's review",
|
||||
},
|
||||
],
|
||||
]
|
||||
|
||||
review_pr.clear_previous_review(event)
|
||||
history = review_pr.clear_previous_review(event)
|
||||
|
||||
assert [r["number"] for r in history["reviews"]] == [1, 2]
|
||||
assert history["reviews"][1]["commit"] == "b" * 7
|
||||
assert history["reviews"][1]["body"] == "Bound is correct now"
|
||||
assert history["replies"] == [
|
||||
'- a.py:3 @human (MEMBER) on "⚠️ **HIGH**: retry loop never exits": intentional, the caller times out'
|
||||
]
|
||||
assert history["others"] == ["- b.py:9 @human (NONE): unrelated comment on someone else's review"]
|
||||
event.put.assert_called_once_with(
|
||||
"https://api.github.com/repos/org/repo/pulls/7/reviews/1/dismissals",
|
||||
json={"message": "Superseded by new review"},
|
||||
|
||||
@@ -54,6 +54,21 @@ def test_action_request_methods():
|
||||
mock_get.assert_called_once()
|
||||
|
||||
|
||||
def test_action_paginate_walks_every_page():
|
||||
"""Test paginate follows pages until a short one and returns the full collection."""
|
||||
with patch("requests.Session.get") as mock_get:
|
||||
pages = [
|
||||
MagicMock(status_code=200, **{"json.return_value": page, "elapsed.total_seconds.return_value": 0.1})
|
||||
for page in ([{"i": i} for i in range(100)], [{"i": 100}])
|
||||
]
|
||||
mock_get.side_effect = pages
|
||||
|
||||
items = Action(token="test-token").paginate("https://api.github.com/test")
|
||||
|
||||
assert len(items) == 101
|
||||
assert [call.kwargs["params"]["page"] for call in mock_get.call_args_list] == [1, 2]
|
||||
|
||||
|
||||
def test_get_pr_contributors_excludes_bots():
|
||||
"""Test PR contributor credit excludes bot commit users."""
|
||||
action = Action(
|
||||
|
||||
Reference in New Issue
Block a user