diff --git a/actions/__init__.py b/actions/__init__.py index 983acc3..e42a94a 100644 --- a/actions/__init__.py +++ b/actions/__init__.py @@ -28,4 +28,4 @@ # ├── test_summarize_pr.py # └── ... -__version__ = "0.2.47" +__version__ = "0.3.0" diff --git a/actions/first_interaction.py b/actions/first_interaction.py index 7a7976c..337bc71 100644 --- a/actions/first_interaction.py +++ b/actions/first_interaction.py @@ -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) diff --git a/actions/review_pr.py b/actions/review_pr.py index 2309608..f3003d3 100644 --- a/actions/review_pr.py +++ b/actions/review_pr.py @@ -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("
📋 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"
{summary_text}\n\n{findings}\n
\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__": diff --git a/actions/utils/github_utils.py b/actions/utils/github_utils.py index 05b4f8b..289440b 100644 --- a/actions/utils/github_utils.py +++ b/actions/utils/github_utils.py @@ -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) diff --git a/tests/test_first_interaction.py b/tests/test_first_interaction.py index 351f120..d232dac 100644 --- a/tests/test_first_interaction.py +++ b/tests/test_first_interaction.py @@ -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"}, diff --git a/tests/test_github_utils.py b/tests/test_github_utils.py index 81b8d38..0265dc6 100644 --- a/tests/test_github_utils.py +++ b/tests/test_github_utils.py @@ -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(