diff --git a/.github/actions/bot-autoassign/stale_pr_bot.py b/.github/actions/bot-autoassign/stale_pr_bot.py index 9404909c..3ebb50f6 100644 --- a/.github/actions/bot-autoassign/stale_pr_bot.py +++ b/.github/actions/bot-autoassign/stale_pr_bot.py @@ -1,8 +1,13 @@ +import os import time from datetime import datetime, timezone from base import GitHubBot -from utils import unassign_linked_issues_helper +from utils import ( + extract_linked_issues, + get_valid_linked_issues, + unassign_linked_issues_helper, +) # GitHub author_association values that represent project maintainers. MAINTAINER_ROLES = frozenset({"OWNER", "MEMBER", "COLLABORATOR"}) @@ -13,12 +18,36 @@ def __init__(self): super().__init__() self.DAYS_BEFORE_STALE_WARNING = 7 self.DAYS_BEFORE_UNASSIGN = 14 - self.DAYS_BEFORE_CLOSE = 60 + self.DAYS_BEFORE_FINAL_FOLLOWUP = 60 + bot_username = os.environ.get("BOT_USERNAME", "openwisp-companion") + self.bot_login = ( + bot_username if bot_username.endswith("[bot]") else f"{bot_username}[bot]" + ) + + @staticmethod + def _commit_activity_date_for_author(commit, pr_author): + dates = [] + if ( + commit.author + and commit.author.login == pr_author + and commit.commit.author + and commit.commit.author.date + ): + dates.append(commit.commit.author.date) + if ( + commit.committer + and commit.committer.login == pr_author + and commit.commit.committer + and commit.commit.committer.date + ): + dates.append(commit.commit.committer.date) + return max(dates, default=None) def _get_last_author_activity( self, pr, after_date, + commits=None, issue_comments=None, all_reviews=None, review_comments=None, @@ -31,12 +60,14 @@ def _get_last_author_activity( if not pr_author: return None last_activity = None - for commit in pr.get_commits(): - commit_date = commit.commit.author.date - if commit_date > after_date: - if commit.author and commit.author.login == pr_author: - if not last_activity or commit_date > last_activity: - last_activity = commit_date + if commits is None: + commits = pr.get_commits() + for commit in commits: + commit_date = self._commit_activity_date_for_author(commit, pr_author) + if not commit_date or commit_date <= after_date: + continue + if not last_activity or commit_date > last_activity: + last_activity = commit_date if issue_comments is None: issue_comments = list(pr.get_issue_comments()) for comment in issue_comments: @@ -67,6 +98,7 @@ def get_days_since_activity( self, pr, last_changes_requested, + commits=None, issue_comments=None, all_reviews=None, review_comments=None, @@ -77,6 +109,7 @@ def get_days_since_activity( last_author_activity = self._get_last_author_activity( pr, last_changes_requested, + commits, issue_comments, all_reviews, review_comments, @@ -85,60 +118,35 @@ def get_days_since_activity( now = datetime.now(timezone.utc) return (now - reference_date).days except Exception as e: - print("Error calculating activity" f" for PR #{pr.number}: {e}") + print(f"Error calculating activity for PR #{pr.number}: {e}") return 0 def is_waiting_for_maintainer( self, pr, last_changes_requested, + commits=None, issue_comments=None, all_reviews=None, review_comments=None, ): - """Return True when the contributor has responded but no maintainer has acted since. - - The bot should not warn, mark stale, or close a PR when the ball - is in the maintainers' court. + """True when the contributor has responded but no maintainer review + has followed. Comments don't count; errors fail closed (skip). """ try: pr_author = pr.user.login if pr.user else None if not pr_author: - return False + return True last_author_activity = self._get_last_author_activity( pr, last_changes_requested, + commits, issue_comments, all_reviews, review_comments, ) if not last_author_activity: return False - # Check for maintainer activity after the contributor's last action. - # Only OWNER / MEMBER / COLLABORATOR responses count; random - # community comments and bot messages do not. - if issue_comments is None: - issue_comments = list(pr.get_issue_comments()) - for comment in issue_comments: - if ( - comment.user - and comment.user.login != pr_author - and comment.user.type != "Bot" - and getattr(comment, "author_association", None) in MAINTAINER_ROLES - and comment.created_at > last_author_activity - ): - return False - if review_comments is None: - review_comments = list(pr.get_review_comments()) - for comment in review_comments: - if ( - comment.user - and comment.user.login != pr_author - and comment.user.type != "Bot" - and getattr(comment, "author_association", None) in MAINTAINER_ROLES - and comment.created_at > last_author_activity - ): - return False if all_reviews is None: all_reviews = list(pr.get_reviews()) for review in all_reviews: @@ -153,29 +161,43 @@ def is_waiting_for_maintainer( return False return True except Exception as e: - print("Error checking maintainer activity" f" for PR #{pr.number}: {e}") - return False + print(f"Error checking maintainer activity for PR #{pr.number}: {e}") + return True def get_last_changes_requested(self, pr, all_reviews=None): - try: - if all_reviews is None: - all_reviews = list(pr.get_reviews()) - changes_requested_reviews = [ - r for r in all_reviews if r.state == "CHANGES_REQUESTED" - ] - if not changes_requested_reviews: - return None - changes_requested_reviews.sort(key=lambda r: r.submitted_at, reverse=True) - return changes_requested_reviews[0].submitted_at - except Exception as e: - print("Error getting reviews" f" for PR #{pr.number}: {e}") - return None + """Timestamp of the latest CHANGES_REQUESTED that still represents + a human reviewer's current stance, or ``None``. - def has_bot_comment(self, pr, comment_type, after_date=None, issue_comments=None): - """Check if PR already has a specific type of bot comment. + Errors propagate so the caller can distinguish "no active block" + from "couldn't determine" and skip the PR. + """ + if all_reviews is None: + all_reviews = list(pr.get_reviews()) + # Bot reviews are advisory; COMMENTED does not change stance. + latest_per_reviewer = {} + for review in all_reviews: + if ( + not review.user + or not review.submitted_at + or review.user.type == "Bot" + or review.state == "COMMENTED" + ): + continue + current = latest_per_reviewer.get(review.user.login) + if current is None or review.submitted_at > current.submitted_at: + latest_per_reviewer[review.user.login] = review + return max( + ( + review.submitted_at + for review in latest_per_reviewer.values() + if review.state == "CHANGES_REQUESTED" + ), + default=None, + ) - Uses HTML markers. If ``after_date`` is provided, - only considers comments posted after that date. + def has_bot_comment(self, pr, comment_type, after_date=None, issue_comments=None): + """Check if this bot has already posted a comment with the given + marker. If ``after_date`` is set, only later comments count. """ try: if issue_comments is None: @@ -184,7 +206,7 @@ def has_bot_comment(self, pr, comment_type, after_date=None, issue_comments=None for comment in issue_comments: if ( comment.user - and comment.user.type == "Bot" + and comment.user.login == self.bot_login and marker in comment.body ): if after_date and comment.created_at <= after_date: @@ -192,91 +214,72 @@ def has_bot_comment(self, pr, comment_type, after_date=None, issue_comments=None return True return False except Exception as e: - print("Error checking bot comments" f" for PR #{pr.number}: {e}") + print(f"Error checking bot comments for PR #{pr.number}: {e}") return False def unassign_linked_issues(self, pr): - try: - pr_author = pr.user.login if pr.user else None - if not pr_author: - return False - unassigned_issues = unassign_linked_issues_helper( + pr_author = pr.user.login if pr.user else None + if not pr_author: + return 0 + return len( + unassign_linked_issues_helper( self.repo, self.repository_name, pr.body or "", pr_author ) - return len(unassigned_issues) - except Exception as e: - print(f"Error processing linked issues for PR #{pr.number}: {e}") - return 0 + ) - def close_stale_pr(self, pr, days_inactive): - # TEMPORARY: auto-close disabled. The stale-detection heuristic - # has been closing PRs that are merely blocked by bot reviews - # (or by reviews the same reviewer later approved). The proper - # fix lives in PR #668; until it lands, no PR is auto-closed. - print(f"Auto-close currently disabled, skipping PR #{pr.number}") - return False - if pr.state == "closed": - print(f"PR #{pr.number} is already closed, skipping") + def _clear_stale_label(self, pr): + # pr.labels comes from the list-pulls payload, so no extra request. + try: + if "stale" not in {label.name for label in pr.labels}: + return False + pr.remove_from_labels("stale") return True + except Exception as e: + print(f"Could not clear stale label from PR #{pr.number}: {e}") + return False + + def _reassign_unassigned_linked_issues(self, pr): + pr_author = pr.user.login if pr.user else None + if not pr_author: + return + try: + linked = extract_linked_issues(pr.body or "") + for _, issue in get_valid_linked_issues( + self.repo, self.repository_name, linked + ): + try: + if not issue.assignees: + issue.add_to_assignees(pr_author) + except Exception as e: + print(f"Error reassigning issue #{issue.number}: {e}") + except Exception as e: + print(f"Error iterating linked issues for PR #{pr.number}: {e}") + + def send_final_followup(self, pr, days_inactive): try: pr_author = pr.user.login if pr.user else None if not pr_author: return False - close_lines = [ - "", + followup_lines = [ + "", f"Hi @{pr_author} 👋,", "", ( - "This pull request has been automatically" - " closed due to" - f" **{days_inactive} days of inactivity**." - " After changes were requested," - " the PR remained inactive." - ), - "", - ( - "We understand that life gets busy," - " and we appreciate your initial" - " contribution! 💙" - ), - "", - ("**The door is always open**" " for you to come back:"), - ( - "- You can **reopen this PR** at any time" - " if you'd like to continue working on it" - ), - ("- Feel free to push new commits" " addressing the requested changes"), - ( - "- If you reopen the PR, the linked issue" - " will be reassigned to you" + f"This PR has been inactive for **{days_inactive} days**" + " since changes were requested. Are you still working on it?" ), "", ( - "If you have any questions or need help," - " don't hesitate to reach out." - " We're here to support you!" + "If yes, push new commits or reply to let us know." + " If you've moved on, please close the PR or comment" + " so another contributor can pick it up." ), - "", - ("Thank you for your interest in" " contributing to OpenWISP! 🙏"), ] - try: - pr.create_issue_comment("\n".join(close_lines)) - except Exception as comment_error: - print( - f"Warning: Could not post closing comment" - f" on PR #{pr.number}: {comment_error}" - ) - finally: - pr.edit(state="closed") - unassigned_count = self.unassign_linked_issues(pr) - print( - f"Closed PR #{pr.number} after" - f" {days_inactive} days of inactivity," - f" unassigned {unassigned_count} issues" - ) + pr.create_issue_comment("\n".join(followup_lines)) + print(f"Sent final follow-up for PR #{pr.number}") return True except Exception as e: - print(f"Error closing PR #{pr.number}: {e}") + print(f"Error sending final follow-up for PR #{pr.number}: {e}") return False def mark_pr_stale(self, pr, days_inactive): @@ -296,10 +299,9 @@ def mark_pr_stale(self, pr, days_inactive): ), "", ( - "As a result, **the linked issue(s)" - " have been unassigned** from you" - " to allow other contributors" - " to work on it." + "As a result, **any linked issues are being" + " unassigned** from you so other contributors" + " can pick them up." ), "", ( @@ -317,29 +319,20 @@ def mark_pr_stale(self, pr, days_inactive): " let us know." " We're happy to help! 🤝" ), - "", - ( - "If there's no further activity within" - f" **{self.DAYS_BEFORE_CLOSE - days_inactive}" - " more days**, this PR will be" - " automatically closed" - " (but can be reopened anytime)." - ), ] - pr.create_issue_comment("\n".join(unassign_lines)) unassigned_count = self.unassign_linked_issues(pr) + pr.create_issue_comment("\n".join(unassign_lines)) try: pr.add_to_labels("stale") except Exception as e: print(f"Could not add stale label: {e}") print( - f"Marked PR #{pr.number} as stale after" - f" {days_inactive} days," + f"Marked PR #{pr.number} stale at {days_inactive} days," f" unassigned {unassigned_count} issues" ) return True except Exception as e: - print(f"Error marking PR #{pr.number}" f" as stale: {e}") + print(f"Error marking PR #{pr.number} as stale: {e}") return False def send_stale_warning(self, pr, days_inactive): @@ -388,7 +381,7 @@ def send_stale_warning(self, pr, days_inactive): print(f"Sent stale warning for PR #{pr.number}") return True except Exception as e: - print("Error sending warning" f" for PR #{pr.number}: {e}") + print(f"Error sending warning for PR #{pr.number}: {e}") return False def process_stale_prs(self): @@ -407,62 +400,91 @@ def process_stale_prs(self): pr, all_reviews ) if not last_changes_requested: + # No active block — unwind any prior stale state. + if self._clear_stale_label(pr): + self._reassign_unassigned_linked_issues(pr) continue issue_comments = list(pr.get_issue_comments()) review_comments = list(pr.get_review_comments()) + commits = list(pr.get_commits()) days_inactive = self.get_days_since_activity( pr, last_changes_requested, + commits, issue_comments, all_reviews, review_comments, ) print( - f"PR #{pr.number}: {days_inactive}" - " days since contributor activity" + f"PR #{pr.number}: {days_inactive} days since contributor activity" ) if self.is_waiting_for_maintainer( pr, last_changes_requested, + commits, issue_comments, all_reviews, review_comments, ): + # If we previously marked the PR stale, unwind + # that state now that the contributor has acted. + if self._clear_stale_label(pr): + self._reassign_unassigned_linked_issues(pr) print( - f"PR #{pr.number}: waiting for" - " maintainer review, skipping" + f"PR #{pr.number}: waiting for maintainer review, skipping" ) continue - if days_inactive >= self.DAYS_BEFORE_CLOSE: - if self.close_stale_pr(pr, days_inactive): - processed_count += 1 - elif days_inactive >= self.DAYS_BEFORE_UNASSIGN: - if not self.has_bot_comment( - pr, + stages = ( + ( + self.DAYS_BEFORE_STALE_WARNING, + self.DAYS_BEFORE_UNASSIGN, + "stale_warning", + self.send_stale_warning, + ), + ( + self.DAYS_BEFORE_UNASSIGN, + None, "stale", - after_date=last_changes_requested, - issue_comments=issue_comments, - ): - if self.mark_pr_stale(pr, days_inactive): - processed_count += 1 - elif days_inactive >= self.DAYS_BEFORE_STALE_WARNING: - if not self.has_bot_comment( + self.mark_pr_stale, + ), + ( + self.DAYS_BEFORE_FINAL_FOLLOWUP, + None, + "final_followup", + self.send_final_followup, + ), + ) + posted_stale_this_run = False + for low, high, marker, action in stages: + if days_inactive < low: + continue + if high is not None and days_inactive >= high: + continue + if self.has_bot_comment( pr, - "stale_warning", + marker, after_date=last_changes_requested, issue_comments=issue_comments, ): - if self.send_stale_warning(pr, days_inactive): - processed_count += 1 + continue + # Don't post final-followup in the same run as stale. + if marker == "final_followup" and posted_stale_this_run: + continue + # Flag the attempt, not the success: a failed stale + # post must still suppress final-followup this run, + # otherwise the contributor gets the follow-up with + # no prior stale notice. The stale stage will retry + # on the next daily run. + if marker == "stale": + posted_stale_this_run = True + if action(pr, days_inactive): + processed_count += 1 except Exception as e: - print(f"Error processing" f" PR #{pr.number}: {e}") + print(f"Error processing PR #{pr.number}: {e}") continue finally: time.sleep(0.5) - print( - f"Checked {pr_count} open PRs," - f" processed {processed_count} stale PRs" - ) + print(f"Checked {pr_count} open PRs, processed {processed_count} stale PRs") return True except Exception as e: print(f"Error in process_stale_prs: {e}") @@ -470,7 +492,7 @@ def process_stale_prs(self): def run(self): if not self.github or not self.repo: - print("GitHub client not properly initialized," " cannot proceed") + print("GitHub client not properly initialized, cannot proceed") return False print("Stale PR Management Bot starting...") try: diff --git a/.github/actions/bot-autoassign/tests/test_stale_pr_bot.py b/.github/actions/bot-autoassign/tests/test_stale_pr_bot.py index 78307452..4cc926d7 100644 --- a/.github/actions/bot-autoassign/tests/test_stale_pr_bot.py +++ b/.github/actions/bot-autoassign/tests/test_stale_pr_bot.py @@ -43,7 +43,7 @@ def test_thresholds(self, bot_env): bot = StalePRBot() assert bot.DAYS_BEFORE_STALE_WARNING == 7 assert bot.DAYS_BEFORE_UNASSIGN == 14 - assert bot.DAYS_BEFORE_CLOSE == 60 + assert bot.DAYS_BEFORE_FINAL_FOLLOWUP == 60 class TestGetLastChangesRequested: @@ -77,6 +77,151 @@ def test_no_changes_requested(self, bot_env): ] assert bot.get_last_changes_requested(mock_pr) is None + def _make_review(self, state, submitted_at, login="alice", user_type="User"): + review = Mock() + review.state = state + review.submitted_at = submitted_at + review.user.login = login + review.user.type = user_type + return review + + def test_bot_changes_requested_ignored(self, bot_env): + bot = StalePRBot() + mock_pr = Mock() + mock_pr.get_reviews.return_value = [ + self._make_review( + "CHANGES_REQUESTED", + datetime(2024, 1, 2, tzinfo=timezone.utc), + login="coderabbitai[bot]", + user_type="Bot", + ), + ] + assert bot.get_last_changes_requested(mock_pr) is None + + def test_bot_changes_requested_then_bot_approved(self, bot_env): + bot = StalePRBot() + mock_pr = Mock() + mock_pr.get_reviews.return_value = [ + self._make_review( + "CHANGES_REQUESTED", + datetime(2024, 1, 1, tzinfo=timezone.utc), + login="coderabbitai[bot]", + user_type="Bot", + ), + self._make_review( + "APPROVED", + datetime(2024, 1, 2, tzinfo=timezone.utc), + login="coderabbitai[bot]", + user_type="Bot", + ), + ] + assert bot.get_last_changes_requested(mock_pr) is None + + def test_human_changes_requested_then_same_human_approved(self, bot_env): + bot = StalePRBot() + mock_pr = Mock() + mock_pr.get_reviews.return_value = [ + self._make_review( + "CHANGES_REQUESTED", + datetime(2024, 1, 1, tzinfo=timezone.utc), + ), + self._make_review( + "APPROVED", + datetime(2024, 1, 2, tzinfo=timezone.utc), + ), + ] + assert bot.get_last_changes_requested(mock_pr) is None + + def test_human_changes_requested_then_dismissed(self, bot_env): + bot = StalePRBot() + mock_pr = Mock() + mock_pr.get_reviews.return_value = [ + self._make_review( + "CHANGES_REQUESTED", + datetime(2024, 1, 1, tzinfo=timezone.utc), + ), + self._make_review( + "DISMISSED", + datetime(2024, 1, 3, tzinfo=timezone.utc), + ), + ] + assert bot.get_last_changes_requested(mock_pr) is None + + def test_commented_does_not_supersede_changes_requested(self, bot_env): + bot = StalePRBot() + mock_pr = Mock() + mock_pr.get_reviews.return_value = [ + self._make_review( + "CHANGES_REQUESTED", + datetime(2024, 1, 1, tzinfo=timezone.utc), + ), + self._make_review( + "COMMENTED", + datetime(2024, 1, 5, tzinfo=timezone.utc), + ), + ] + assert bot.get_last_changes_requested(mock_pr) == datetime( + 2024, 1, 1, tzinfo=timezone.utc + ) + + def test_bot_review_after_human_block_does_not_dominate(self, bot_env): + bot = StalePRBot() + mock_pr = Mock() + human_block = datetime(2024, 1, 1, tzinfo=timezone.utc) + mock_pr.get_reviews.return_value = [ + self._make_review("CHANGES_REQUESTED", human_block, login="alice"), + self._make_review( + "CHANGES_REQUESTED", + datetime(2024, 2, 1, tzinfo=timezone.utc), + login="coderabbitai[bot]", + user_type="Bot", + ), + ] + assert bot.get_last_changes_requested(mock_pr) == human_block + + def test_one_human_blocks_other_approves(self, bot_env): + bot = StalePRBot() + mock_pr = Mock() + block_time = datetime(2024, 1, 5, tzinfo=timezone.utc) + mock_pr.get_reviews.return_value = [ + self._make_review( + "APPROVED", + datetime(2024, 1, 4, tzinfo=timezone.utc), + login="alice", + ), + self._make_review( + "CHANGES_REQUESTED", + block_time, + login="bob", + ), + ] + assert bot.get_last_changes_requested(mock_pr) == block_time + + def test_review_without_user_skipped(self, bot_env): + bot = StalePRBot() + mock_pr = Mock() + bad_review = Mock( + state="CHANGES_REQUESTED", + submitted_at=datetime(2024, 1, 1, tzinfo=timezone.utc), + ) + bad_review.user = None + mock_pr.get_reviews.return_value = [bad_review] + assert bot.get_last_changes_requested(mock_pr) is None + + def test_review_without_submitted_at_skipped(self, bot_env): + bot = StalePRBot() + mock_pr = Mock() + pending = self._make_review("CHANGES_REQUESTED", None) + mock_pr.get_reviews.return_value = [pending] + assert bot.get_last_changes_requested(mock_pr) is None + + def test_propagates_exception(self, bot_env): + bot = StalePRBot() + mock_pr = Mock() + mock_pr.get_reviews.side_effect = RuntimeError("transient API error") + with pytest.raises(RuntimeError, match="transient API error"): + bot.get_last_changes_requested(mock_pr) + class TestGetDaysSinceActivity: @patch("stale_pr_bot.datetime") @@ -92,7 +237,9 @@ def test_with_author_commit(self, mock_datetime, bot_env): mock_pr.get_reviews.return_value = [] mock_commit = Mock() mock_commit.commit.author.date = datetime(2024, 1, 5, tzinfo=timezone.utc) + mock_commit.commit.committer.date = datetime(2024, 1, 5, tzinfo=timezone.utc) mock_commit.author.login = "testuser" + mock_commit.committer.login = "testuser" mock_pr.get_commits.return_value = [mock_commit] last_cr = datetime(2024, 1, 1, tzinfo=timezone.utc) result = bot.get_days_since_activity(mock_pr, last_cr) @@ -103,6 +250,84 @@ def test_no_last_changes(self, bot_env): mock_pr = Mock() assert bot.get_days_since_activity(mock_pr, None) == 0 + @patch("stale_pr_bot.datetime") + def test_force_push_uses_committer_date(self, mock_datetime, bot_env): + mock_datetime.now.return_value = datetime(2024, 1, 20, tzinfo=timezone.utc) + mock_datetime.side_effect = lambda *a, **kw: datetime(*a, **kw) + bot = StalePRBot() + mock_pr = Mock() + mock_pr.user.login = "testuser" + mock_pr.get_issue_comments.return_value = [] + mock_pr.get_review_comments.return_value = [] + mock_pr.get_reviews.return_value = [] + mock_commit = Mock() + mock_commit.commit.author.date = datetime(2023, 12, 1, tzinfo=timezone.utc) + mock_commit.commit.committer.date = datetime(2024, 1, 15, tzinfo=timezone.utc) + mock_commit.author.login = "testuser" + mock_commit.committer.login = "testuser" + mock_pr.get_commits.return_value = [mock_commit] + last_cr = datetime(2024, 1, 1, tzinfo=timezone.utc) + assert bot.get_days_since_activity(mock_pr, last_cr) == 5 + + @patch("stale_pr_bot.datetime") + def test_unlinked_author_falls_back_to_committer(self, mock_datetime, bot_env): + mock_datetime.now.return_value = datetime(2024, 1, 10, tzinfo=timezone.utc) + mock_datetime.side_effect = lambda *a, **kw: datetime(*a, **kw) + bot = StalePRBot() + mock_pr = Mock() + mock_pr.user.login = "testuser" + mock_pr.get_issue_comments.return_value = [] + mock_pr.get_review_comments.return_value = [] + mock_pr.get_reviews.return_value = [] + mock_commit = Mock() + mock_commit.commit.author.date = datetime(2024, 1, 5, tzinfo=timezone.utc) + mock_commit.commit.committer.date = datetime(2024, 1, 5, tzinfo=timezone.utc) + mock_commit.author = None + mock_commit.committer.login = "testuser" + mock_pr.get_commits.return_value = [mock_commit] + last_cr = datetime(2024, 1, 1, tzinfo=timezone.utc) + assert bot.get_days_since_activity(mock_pr, last_cr) == 5 + + @patch("stale_pr_bot.datetime") + def test_both_unlinked_commit_skipped(self, mock_datetime, bot_env): + mock_datetime.now.return_value = datetime(2024, 1, 10, tzinfo=timezone.utc) + mock_datetime.side_effect = lambda *a, **kw: datetime(*a, **kw) + bot = StalePRBot() + mock_pr = Mock() + mock_pr.user.login = "testuser" + mock_pr.get_issue_comments.return_value = [] + mock_pr.get_review_comments.return_value = [] + mock_pr.get_reviews.return_value = [] + mock_commit = Mock() + mock_commit.commit.author.date = datetime(2024, 1, 5, tzinfo=timezone.utc) + mock_commit.commit.committer.date = datetime(2024, 1, 5, tzinfo=timezone.utc) + mock_commit.author = None + mock_commit.committer = None + mock_pr.get_commits.return_value = [mock_commit] + last_cr = datetime(2024, 1, 1, tzinfo=timezone.utc) + assert bot.get_days_since_activity(mock_pr, last_cr) == 9 + + @patch("stale_pr_bot.datetime") + def test_maintainer_rebase_does_not_count_as_author_activity( + self, mock_datetime, bot_env + ): + mock_datetime.now.return_value = datetime(2024, 3, 1, tzinfo=timezone.utc) + mock_datetime.side_effect = lambda *a, **kw: datetime(*a, **kw) + bot = StalePRBot() + mock_pr = Mock() + mock_pr.user.login = "contributor" + mock_pr.get_issue_comments.return_value = [] + mock_pr.get_review_comments.return_value = [] + mock_pr.get_reviews.return_value = [] + mock_commit = Mock() + mock_commit.commit.author.date = datetime(2023, 12, 1, tzinfo=timezone.utc) + mock_commit.commit.committer.date = datetime(2024, 2, 25, tzinfo=timezone.utc) + mock_commit.author.login = "contributor" + mock_commit.committer.login = "maintainer" + mock_pr.get_commits.return_value = [mock_commit] + last_cr = datetime(2024, 1, 1, tzinfo=timezone.utc) + assert bot.get_days_since_activity(mock_pr, last_cr) == 60 + class TestIsWaitingForMaintainer: def _make_pr(self, author="contributor"): @@ -123,7 +348,9 @@ def test_contributor_responded_no_maintainer_since(self, bot_env): # Contributor pushed a commit after changes were requested commit = Mock() commit.commit.author.date = datetime(2024, 1, 5, tzinfo=timezone.utc) + commit.commit.committer.date = datetime(2024, 1, 5, tzinfo=timezone.utc) commit.author.login = "contributor" + commit.committer.login = "contributor" pr.get_commits.return_value = [commit] assert bot.is_waiting_for_maintainer(pr, last_cr) is True @@ -134,7 +361,9 @@ def test_contributor_responded_maintainer_reviewed(self, bot_env): last_cr = datetime(2024, 1, 1, tzinfo=timezone.utc) commit = Mock() commit.commit.author.date = datetime(2024, 1, 5, tzinfo=timezone.utc) + commit.commit.committer.date = datetime(2024, 1, 5, tzinfo=timezone.utc) commit.author.login = "contributor" + commit.committer.login = "contributor" pr.get_commits.return_value = [commit] # Maintainer reviewed after contributor's commit review = Mock() @@ -145,14 +374,16 @@ def test_contributor_responded_maintainer_reviewed(self, bot_env): pr.get_reviews.return_value = [review] assert bot.is_waiting_for_maintainer(pr, last_cr) is False - def test_contributor_responded_maintainer_commented(self, bot_env): - """Contributor pushed, then maintainer left an issue comment.""" + def test_maintainer_comment_does_not_count_as_review(self, bot_env): + """A maintainer comment is not a review; the PR is still waiting.""" bot = StalePRBot() pr = self._make_pr() last_cr = datetime(2024, 1, 1, tzinfo=timezone.utc) commit = Mock() commit.commit.author.date = datetime(2024, 1, 5, tzinfo=timezone.utc) + commit.commit.committer.date = datetime(2024, 1, 5, tzinfo=timezone.utc) commit.author.login = "contributor" + commit.committer.login = "contributor" pr.get_commits.return_value = [commit] comment = Mock() comment.user.login = "maintainer" @@ -160,7 +391,7 @@ def test_contributor_responded_maintainer_commented(self, bot_env): comment.author_association = "COLLABORATOR" comment.created_at = datetime(2024, 1, 7, tzinfo=timezone.utc) pr.get_issue_comments.return_value = [comment] - assert bot.is_waiting_for_maintainer(pr, last_cr) is False + assert bot.is_waiting_for_maintainer(pr, last_cr) is True def test_contributor_never_responded(self, bot_env): """No contributor activity after changes requested → not waiting.""" @@ -176,7 +407,9 @@ def test_bot_comments_are_ignored(self, bot_env): last_cr = datetime(2024, 1, 1, tzinfo=timezone.utc) commit = Mock() commit.commit.author.date = datetime(2024, 1, 5, tzinfo=timezone.utc) + commit.commit.committer.date = datetime(2024, 1, 5, tzinfo=timezone.utc) commit.author.login = "contributor" + commit.committer.login = "contributor" pr.get_commits.return_value = [commit] # Only a bot comment exists after contributor's activity bot_comment = Mock() @@ -194,7 +427,9 @@ def test_non_maintainer_comment_ignored(self, bot_env): last_cr = datetime(2024, 1, 1, tzinfo=timezone.utc) commit = Mock() commit.commit.author.date = datetime(2024, 1, 5, tzinfo=timezone.utc) + commit.commit.committer.date = datetime(2024, 1, 5, tzinfo=timezone.utc) commit.author.login = "contributor" + commit.committer.login = "contributor" pr.get_commits.return_value = [commit] comment = Mock() comment.user.login = "random_user" @@ -214,18 +449,31 @@ def test_many_events_does_not_miss_contributor_activity(self, bot_env): contributor_commit.commit.author.date = datetime( 2024, 1, 2, tzinfo=timezone.utc ) + contributor_commit.commit.committer.date = datetime( + 2024, 1, 2, tzinfo=timezone.utc + ) contributor_commit.author.login = "contributor" + contributor_commit.committer.login = "contributor" # 60 subsequent commits from CI/other (not from contributor) base = datetime(2024, 1, 3, tzinfo=timezone.utc) other_commits = [] for i in range(60): c = Mock() c.commit.author.date = base + timedelta(days=i) + c.commit.committer.date = base + timedelta(days=i) c.author.login = "ci-bot" + c.committer.login = "ci-bot" other_commits.append(c) pr.get_commits.return_value = [contributor_commit] + other_commits assert bot.is_waiting_for_maintainer(pr, last_cr) is True + def test_fails_closed_on_exception(self, bot_env): + bot = StalePRBot() + pr = self._make_pr() + pr.get_commits.side_effect = RuntimeError("transient API error") + last_cr = datetime(2024, 1, 1, tzinfo=timezone.utc) + assert bot.is_waiting_for_maintainer(pr, last_cr) is True + class TestUnassignLinkedIssues: def test_success(self, bot_env): @@ -263,23 +511,31 @@ def test_finds_marker(self, bot_env): bot = StalePRBot() mock_pr = Mock() mock_comment = Mock() - mock_comment.user.type = "Bot" + mock_comment.user.login = bot.bot_login mock_comment.body = " This is a stale warning" mock_comment.created_at = datetime(2024, 1, 10, tzinfo=timezone.utc) mock_pr.get_issue_comments.return_value = [mock_comment] assert bot.has_bot_comment(mock_pr, "stale") assert not bot.has_bot_comment(mock_pr, "closed") + def test_ignores_marker_from_other_bot(self, bot_env): + bot = StalePRBot() + mock_pr = Mock() + mock_comment = Mock() + mock_comment.user.login = "some-other-bot[bot]" + mock_comment.body = " quoted from elsewhere" + mock_comment.created_at = datetime(2024, 1, 10, tzinfo=timezone.utc) + mock_pr.get_issue_comments.return_value = [mock_comment] + assert not bot.has_bot_comment(mock_pr, "stale") + def test_ignores_old_marker_before_after_date(self, bot_env): - """Old markers from a previous cycle should be ignored.""" bot = StalePRBot() mock_pr = Mock() mock_comment = Mock() - mock_comment.user.type = "Bot" + mock_comment.user.login = bot.bot_login mock_comment.body = " old warning" mock_comment.created_at = datetime(2024, 1, 5, tzinfo=timezone.utc) mock_pr.get_issue_comments.return_value = [mock_comment] - # The marker is from Jan 5 but changes re-requested Jan 8 after_date = datetime(2024, 1, 8, tzinfo=timezone.utc) assert not bot.has_bot_comment(mock_pr, "stale_warning", after_date=after_date) @@ -287,7 +543,7 @@ def test_finds_recent_marker_after_date(self, bot_env): bot = StalePRBot() mock_pr = Mock() mock_comment = Mock() - mock_comment.user.type = "Bot" + mock_comment.user.login = bot.bot_login mock_comment.body = " new warning" mock_comment.created_at = datetime(2024, 1, 15, tzinfo=timezone.utc) mock_pr.get_issue_comments.return_value = [mock_comment] @@ -328,35 +584,26 @@ def test_success(self, bot_env): mock_pr.add_to_labels.assert_called_once_with("stale") mock_issue.remove_from_assignees.assert_called_once_with("testuser") + def test_no_comment_when_unassign_raises(self, bot_env): + bot = StalePRBot() + mock_pr = Mock() + mock_pr.user.login = "testuser" + mock_pr.number = 1 + bot.unassign_linked_issues = Mock(side_effect=RuntimeError("transient")) + assert bot.mark_pr_stale(mock_pr, 14) is False + mock_pr.create_issue_comment.assert_not_called() + mock_pr.add_to_labels.assert_not_called() + -@pytest.mark.skip(reason="Auto-close temporarily disabled; see close_stale_pr stub.") -class TestCloseStalePR: +class TestSendFinalFollowup: def test_success(self, bot_env): bot = StalePRBot() mock_pr = Mock() - mock_pr.body = "Fixes #123" mock_pr.user.login = "testuser" - mock_pr.state = "open" - mock_assignee = Mock() - mock_assignee.login = "testuser" - mock_issue = Mock() - mock_issue.pull_request = None - mock_issue.assignees = [mock_assignee] - mock_issue.repository.full_name = "openwisp/openwisp-utils" - bot_env["repo"].get_issue.return_value = mock_issue - assert bot.close_stale_pr(mock_pr, 60) + assert bot.send_final_followup(mock_pr, 60) mock_pr.create_issue_comment.assert_called_once() comment = mock_pr.create_issue_comment.call_args[0][0] - assert "" in comment - mock_pr.edit.assert_called_once_with(state="closed") - mock_issue.remove_from_assignees.assert_called_once_with("testuser") - - def test_already_closed(self, bot_env): - bot = StalePRBot() - mock_pr = Mock() - mock_pr.state = "closed" - assert bot.close_stale_pr(mock_pr, 60) - mock_pr.create_issue_comment.assert_not_called() + assert "" in comment mock_pr.edit.assert_not_called() @@ -369,16 +616,19 @@ def test_skips_pr_waiting_for_maintainer(self, mock_datetime, bot_env): mock_pr = Mock() mock_pr.number = 42 mock_pr.user.login = "contributor" - # Changes requested on Jan 1 - review = Mock( - state="CHANGES_REQUESTED", - submitted_at=datetime(2024, 1, 1, tzinfo=timezone.utc), - ) + # Maintainer requested changes on Jan 1 + review = Mock() + review.state = "CHANGES_REQUESTED" + review.submitted_at = datetime(2024, 1, 1, tzinfo=timezone.utc) + review.user.login = "maintainer" + review.user.type = "User" mock_pr.get_reviews.return_value = [review] # Contributor pushed on Jan 5 commit = Mock() commit.commit.author.date = datetime(2024, 1, 5, tzinfo=timezone.utc) + commit.commit.committer.date = datetime(2024, 1, 5, tzinfo=timezone.utc) commit.author.login = "contributor" + commit.committer.login = "contributor" mock_pr.get_commits.return_value = [commit] # No maintainer activity mock_pr.get_issue_comments.return_value = [] @@ -389,6 +639,220 @@ def test_skips_pr_waiting_for_maintainer(self, mock_datetime, bot_env): mock_pr.create_issue_comment.assert_not_called() mock_pr.edit.assert_not_called() + @patch("stale_pr_bot.datetime") + def test_skips_pr_with_only_bot_changes_requested(self, mock_datetime, bot_env): + mock_datetime.now.return_value = datetime(2024, 5, 10, tzinfo=timezone.utc) + mock_datetime.side_effect = lambda *a, **kw: datetime(*a, **kw) + bot = StalePRBot() + mock_pr = Mock() + mock_pr.number = 1235 + mock_pr.user.login = "contributor" + bot_review = Mock() + bot_review.state = "CHANGES_REQUESTED" + bot_review.submitted_at = datetime(2024, 2, 1, tzinfo=timezone.utc) + bot_review.user.login = "coderabbitai[bot]" + bot_review.user.type = "Bot" + mock_pr.get_reviews.return_value = [bot_review] + mock_pr.get_commits.return_value = [] + mock_pr.get_issue_comments.return_value = [] + mock_pr.get_review_comments.return_value = [] + bot_env["repo"].get_pulls.return_value = [mock_pr] + bot.process_stale_prs() + mock_pr.create_issue_comment.assert_not_called() + mock_pr.edit.assert_not_called() + + @patch("stale_pr_bot.datetime") + def test_skips_pr_with_superseded_changes_requested(self, mock_datetime, bot_env): + mock_datetime.now.return_value = datetime(2024, 5, 10, tzinfo=timezone.utc) + mock_datetime.side_effect = lambda *a, **kw: datetime(*a, **kw) + bot = StalePRBot() + mock_pr = Mock() + mock_pr.number = 99 + mock_pr.user.login = "contributor" + cr_review = Mock() + cr_review.state = "CHANGES_REQUESTED" + cr_review.submitted_at = datetime(2024, 2, 1, tzinfo=timezone.utc) + cr_review.user.login = "maintainer" + cr_review.user.type = "User" + approve_review = Mock() + approve_review.state = "APPROVED" + approve_review.submitted_at = datetime(2024, 2, 5, tzinfo=timezone.utc) + approve_review.user.login = "maintainer" + approve_review.user.type = "User" + mock_pr.get_reviews.return_value = [cr_review, approve_review] + mock_pr.get_commits.return_value = [] + mock_pr.get_issue_comments.return_value = [] + mock_pr.get_review_comments.return_value = [] + bot_env["repo"].get_pulls.return_value = [mock_pr] + bot.process_stale_prs() + mock_pr.create_issue_comment.assert_not_called() + mock_pr.edit.assert_not_called() + + @patch("stale_pr_bot.datetime") + def test_pr_first_processed_past_60_days_marks_stale_only( + self, mock_datetime, bot_env + ): + mock_datetime.now.return_value = datetime(2024, 5, 10, tzinfo=timezone.utc) + mock_datetime.side_effect = lambda *a, **kw: datetime(*a, **kw) + bot = StalePRBot() + mock_pr = Mock() + mock_pr.body = "" + mock_pr.number = 7 + mock_pr.user.login = "contributor" + cr_review = Mock() + cr_review.state = "CHANGES_REQUESTED" + cr_review.submitted_at = datetime(2024, 2, 1, tzinfo=timezone.utc) + cr_review.user.login = "maintainer" + cr_review.user.type = "User" + mock_pr.get_reviews.return_value = [cr_review] + mock_pr.get_commits.return_value = [] + mock_pr.get_issue_comments.return_value = [] + mock_pr.get_review_comments.return_value = [] + mock_pr.labels = [] + bot_env["repo"].get_pulls.return_value = [mock_pr] + bot.process_stale_prs() + bodies = [c[0][0] for c in mock_pr.create_issue_comment.call_args_list] + assert any("" in b for b in bodies) + assert not any("" in b for b in bodies) + assert not any("" in b for b in bodies) + mock_pr.add_to_labels.assert_called_once_with("stale") + mock_pr.edit.assert_not_called() + + @patch("stale_pr_bot.datetime") + def test_first_run_past_60_days_failed_stale_skips_followup( + self, mock_datetime, bot_env + ): + # Sustained API outage: mark_pr_stale returns False. The + # final-followup must still be suppressed this run, otherwise the + # contributor sees an out-of-context follow-up with no prior stale + # notice. + mock_datetime.now.return_value = datetime(2024, 5, 10, tzinfo=timezone.utc) + mock_datetime.side_effect = lambda *a, **kw: datetime(*a, **kw) + bot = StalePRBot() + mock_pr = Mock() + mock_pr.body = "" + mock_pr.number = 8 + mock_pr.user.login = "contributor" + cr_review = Mock() + cr_review.state = "CHANGES_REQUESTED" + cr_review.submitted_at = datetime(2024, 2, 1, tzinfo=timezone.utc) + cr_review.user.login = "maintainer" + cr_review.user.type = "User" + mock_pr.get_reviews.return_value = [cr_review] + mock_pr.get_commits.return_value = [] + mock_pr.get_issue_comments.return_value = [] + mock_pr.get_review_comments.return_value = [] + mock_pr.labels = [] + bot.mark_pr_stale = Mock(return_value=False) + bot.send_final_followup = Mock(return_value=True) + bot_env["repo"].get_pulls.return_value = [mock_pr] + bot.process_stale_prs() + bot.mark_pr_stale.assert_called_once() + bot.send_final_followup.assert_not_called() + + @patch("stale_pr_bot.datetime") + def test_final_followup_fires_after_prior_stale_run(self, mock_datetime, bot_env): + mock_datetime.now.return_value = datetime(2024, 5, 10, tzinfo=timezone.utc) + mock_datetime.side_effect = lambda *a, **kw: datetime(*a, **kw) + bot = StalePRBot() + mock_pr = Mock() + mock_pr.body = "" + mock_pr.number = 7 + mock_pr.user.login = "contributor" + cr_review = Mock() + cr_review.state = "CHANGES_REQUESTED" + cr_review.submitted_at = datetime(2024, 2, 1, tzinfo=timezone.utc) + cr_review.user.login = "maintainer" + cr_review.user.type = "User" + mock_pr.get_reviews.return_value = [cr_review] + mock_pr.get_commits.return_value = [] + mock_pr.get_review_comments.return_value = [] + stale_label = Mock() + stale_label.name = "stale" + mock_pr.labels = [stale_label] + prior_stale = Mock() + prior_stale.user.login = bot.bot_login + prior_stale.body = " previous run" + prior_stale.created_at = datetime(2024, 5, 1, tzinfo=timezone.utc) + mock_pr.get_issue_comments.return_value = [prior_stale] + bot_env["repo"].get_pulls.return_value = [mock_pr] + bot.process_stale_prs() + bodies = [c[0][0] for c in mock_pr.create_issue_comment.call_args_list] + assert any("" in b for b in bodies) + assert not any( + "" in b and "previous run" not in b for b in bodies + ) + mock_pr.edit.assert_not_called() + + @patch("stale_pr_bot.datetime") + def test_clears_stale_label_when_contributor_responds(self, mock_datetime, bot_env): + mock_datetime.now.return_value = datetime(2024, 2, 1, tzinfo=timezone.utc) + mock_datetime.side_effect = lambda *a, **kw: datetime(*a, **kw) + bot = StalePRBot() + mock_pr = Mock() + mock_pr.body = "Fixes #42" + mock_pr.number = 1 + mock_pr.user.login = "contributor" + cr_review = Mock() + cr_review.state = "CHANGES_REQUESTED" + cr_review.submitted_at = datetime(2024, 1, 1, tzinfo=timezone.utc) + cr_review.user.login = "maintainer" + cr_review.user.type = "User" + mock_pr.get_reviews.return_value = [cr_review] + commit = Mock() + commit.commit.author.date = datetime(2024, 1, 20, tzinfo=timezone.utc) + commit.commit.committer.date = datetime(2024, 1, 20, tzinfo=timezone.utc) + commit.author.login = "contributor" + commit.committer.login = "contributor" + mock_pr.get_commits.return_value = [commit] + stale_label = Mock() + stale_label.name = "stale" + mock_pr.labels = [stale_label] + mock_pr.get_issue_comments.return_value = [] + mock_pr.get_review_comments.return_value = [] + mock_issue = Mock() + mock_issue.number = 42 + mock_issue.pull_request = None + mock_issue.assignees = [] + mock_issue.repository.full_name = "openwisp/openwisp-utils" + bot_env["repo"].get_issue.return_value = mock_issue + bot_env["repo"].get_pulls.return_value = [mock_pr] + bot.process_stale_prs() + mock_pr.remove_from_labels.assert_called_once_with("stale") + mock_issue.add_to_assignees.assert_called_once_with("contributor") + mock_pr.create_issue_comment.assert_not_called() + + @patch("stale_pr_bot.datetime") + def test_clears_stale_label_when_no_active_block(self, mock_datetime, bot_env): + mock_datetime.now.return_value = datetime(2024, 2, 1, tzinfo=timezone.utc) + mock_datetime.side_effect = lambda *a, **kw: datetime(*a, **kw) + bot = StalePRBot() + mock_pr = Mock() + mock_pr.body = "Fixes #42" + mock_pr.number = 1 + mock_pr.user.login = "contributor" + # Reviewer approved after their CR → no active CHANGES_REQUESTED. + approve = Mock() + approve.state = "APPROVED" + approve.submitted_at = datetime(2024, 1, 20, tzinfo=timezone.utc) + approve.user.login = "maintainer" + approve.user.type = "User" + mock_pr.get_reviews.return_value = [approve] + stale_label = Mock() + stale_label.name = "stale" + mock_pr.labels = [stale_label] + mock_issue = Mock() + mock_issue.number = 42 + mock_issue.pull_request = None + mock_issue.assignees = [] + mock_issue.repository.full_name = "openwisp/openwisp-utils" + bot_env["repo"].get_issue.return_value = mock_issue + bot_env["repo"].get_pulls.return_value = [mock_pr] + bot.process_stale_prs() + mock_pr.remove_from_labels.assert_called_once_with("stale") + mock_issue.add_to_assignees.assert_called_once_with("contributor") + mock_pr.create_issue_comment.assert_not_called() + class TestRun: def test_no_github_client(self, bot_env): diff --git a/docs/developer/reusable-github-utils.rst b/docs/developer/reusable-github-utils.rst index 26ab4329..d8e7e226 100644 --- a/docs/developer/reusable-github-utils.rst +++ b/docs/developer/reusable-github-utils.rst @@ -69,10 +69,42 @@ OpenWISP repositories. The bot provides the following features: assigned, the bot responds with contributing guidelines explaining that no assignment is needed — just open a PR. - **Stale PR management**: Warns PR authors after 7 days of inactivity, - marks stale and unassigns after 14 days, and closes after 60 days. + marks stale and unassigns after 14 days, and posts a final follow-up + encouragement after 60 days. The bot does not auto-close PRs. - **PR reopen reassignment**: When a stale PR is reopened, linked issues are reassigned back to the author. +**How Stale PR Detection Works** + +The Stale PR job runs daily. For each open PR: + +1. **Trigger condition.** The PR is processed only when at least one human + reviewer's latest non-COMMENTED review is ``CHANGES_REQUESTED``. Bot + reviews and reviews later superseded by ``APPROVED`` or ``DISMISSED`` + from the same reviewer do not block. +2. **Inactivity** is the time since the more recent of: the PR author's + latest commit, issue comment, review comment, or review after the + blocking review; or the blocking review's timestamp if the author has + not acted since. For commits the date is taken from whichever identity + (author or committer) matches the PR author, so a maintainer rebasing + the contributor's commits does not reset the clock. +3. **Maintainer-court skip.** If the contributor has responded but no + maintainer (``OWNER``, ``MEMBER`` or ``COLLABORATOR``) has submitted a + review since, the PR is skipped — the ball is in the maintainer's + court. Comments are not reviews. +4. **Action by days inactive:** + + - **7–13 days:** posts a stale-warning comment. + - **≥ 14 days:** adds the ``stale`` label and unassigns the contributor + from linked issues. Any subsequent author activity (push or comment) + unwinds the label and reassigns linked issues on the next daily run; + an author comment also triggers the immediate recovery bot. + - **≥ 60 days:** posts a final follow-up comment asking whether the + contributor is still working on it. The PR is not auto-closed; + maintainers may close manually if needed. + + Each stage posts at most once per blocking review cycle. + **Secrets** These secrets are used by the workflow to generate a ``GITHUB_TOKEN`` via