diff --git a/.github/actions/bot-autoassign/stale_pr_bot.py b/.github/actions/bot-autoassign/stale_pr_bot.py index dfecc7fd..98e3ee1a 100644 --- a/.github/actions/bot-autoassign/stale_pr_bot.py +++ b/.github/actions/bot-autoassign/stale_pr_bot.py @@ -1,10 +1,12 @@ import time -from collections import deque from datetime import datetime, timezone from base import GitHubBot from utils import unassign_linked_issues_helper +# GitHub author_association values that represent project maintainers. +MAINTAINER_ROLES = frozenset({"OWNER", "MEMBER", "COLLABORATOR"}) + class StalePRBot(GitHubBot): def __init__(self): @@ -13,6 +15,54 @@ def __init__(self): self.DAYS_BEFORE_UNASSIGN = 14 self.DAYS_BEFORE_CLOSE = 60 + def _get_last_author_activity( + self, + pr, + after_date, + issue_comments=None, + all_reviews=None, + review_comments=None, + ): + """Return the datetime of the PR author's latest activity after *after_date*. + + Returns ``None`` when the author has not acted since *after_date*. + """ + pr_author = pr.user.login if pr.user else None + 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 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: + comment_date = comment.created_at + if comment_date > after_date: + if not last_activity or comment_date > last_activity: + last_activity = comment_date + 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: + comment_date = comment.created_at + if comment_date > after_date: + if not last_activity or comment_date > last_activity: + last_activity = comment_date + if all_reviews is None: + all_reviews = list(pr.get_reviews()) + for review in all_reviews: + if review.user and review.user.login == pr_author: + review_date = review.submitted_at + if review_date and review_date > after_date: + if not last_activity or review_date > last_activity: + last_activity = review_date + return last_activity + def get_days_since_activity( self, pr, @@ -23,78 +73,95 @@ def get_days_since_activity( ): if not last_changes_requested: return 0 + try: + last_author_activity = self._get_last_author_activity( + pr, + last_changes_requested, + issue_comments, + all_reviews, + review_comments, + ) + reference_date = last_author_activity or last_changes_requested + now = datetime.now(timezone.utc) + return (now - reference_date).days + except Exception as e: + print("Error calculating activity" f" for PR #{pr.number}: {e}") + return 0 + + def is_waiting_for_maintainer( + self, + pr, + last_changes_requested, + 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. + """ try: pr_author = pr.user.login if pr.user else None if not pr_author: - return 0 - last_author_activity = None - commits = deque(pr.get_commits(), maxlen=50) - for commit in commits: - commit_date = commit.commit.author.date - if commit_date > last_changes_requested: - if commit.author and commit.author.login == pr_author: - if ( - not last_author_activity - or commit_date > last_author_activity - ): - last_author_activity = commit_date + return False + last_author_activity = self._get_last_author_activity( + pr, + last_changes_requested, + 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()) - comments = ( - issue_comments[-20:] if len(issue_comments) > 20 else issue_comments - ) - for comment in comments: - if comment.user and comment.user.login == pr_author: - comment_date = comment.created_at - if comment_date > last_changes_requested: - if ( - not last_author_activity - or comment_date > last_author_activity - ): - last_author_activity = comment_date + 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()) - all_review_comments = review_comments - review_comments = ( - all_review_comments[-20:] - if len(all_review_comments) > 20 - else all_review_comments - ) for comment in review_comments: - if comment.user and comment.user.login == pr_author: - comment_date = comment.created_at - if comment_date > last_changes_requested: - if ( - not last_author_activity - or comment_date > last_author_activity - ): - last_author_activity = comment_date + 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()) - reviews = all_reviews[-20:] if len(all_reviews) > 20 else all_reviews - for review in reviews: - if review.user and review.user.login == pr_author: - review_date = review.submitted_at - if review_date and review_date > last_changes_requested: - if ( - not last_author_activity - or review_date > last_author_activity - ): - last_author_activity = review_date - reference_date = last_author_activity or last_changes_requested - now = datetime.now(timezone.utc) - return (now - reference_date).days + for review in all_reviews: + if ( + review.user + and review.user.login != pr_author + and review.user.type != "Bot" + and getattr(review, "author_association", None) in MAINTAINER_ROLES + and review.submitted_at + and review.submitted_at > last_author_activity + ): + return False + return True except Exception as e: - print("Error calculating activity" f" for PR #{pr.number}: {e}") - return 0 + print("Error checking maintainer activity" f" for PR #{pr.number}: {e}") + return False def get_last_changes_requested(self, pr, all_reviews=None): try: if all_reviews is None: all_reviews = list(pr.get_reviews()) - reviews = all_reviews[-50:] if len(all_reviews) > 50 else all_reviews changes_requested_reviews = [ - r for r in reviews if r.state == "CHANGES_REQUESTED" + r for r in all_reviews if r.state == "CHANGES_REQUESTED" ] if not changes_requested_reviews: return None @@ -348,6 +415,18 @@ def process_stale_prs(self): f"PR #{pr.number}: {days_inactive}" " days since contributor activity" ) + if self.is_waiting_for_maintainer( + pr, + last_changes_requested, + issue_comments, + all_reviews, + review_comments, + ): + print( + 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 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 20892a3a..32c5ffbb 100644 --- a/.github/actions/bot-autoassign/tests/test_stale_pr_bot.py +++ b/.github/actions/bot-autoassign/tests/test_stale_pr_bot.py @@ -1,6 +1,6 @@ import os import sys -from datetime import datetime, timezone +from datetime import datetime, timedelta, timezone from unittest.mock import Mock, patch # Add the parent directory to path for importing bot modules @@ -104,6 +104,129 @@ def test_no_last_changes(self, bot_env): assert bot.get_days_since_activity(mock_pr, None) == 0 +class TestIsWaitingForMaintainer: + def _make_pr(self, author="contributor"): + mock_pr = Mock() + mock_pr.number = 1 + mock_pr.user.login = author + mock_pr.get_commits.return_value = [] + mock_pr.get_issue_comments.return_value = [] + mock_pr.get_review_comments.return_value = [] + mock_pr.get_reviews.return_value = [] + return mock_pr + + def test_contributor_responded_no_maintainer_since(self, bot_env): + """Contributor pushed after changes requested, no maintainer response.""" + bot = StalePRBot() + pr = self._make_pr() + last_cr = datetime(2024, 1, 1, tzinfo=timezone.utc) + # Contributor pushed a commit after changes were requested + commit = Mock() + commit.commit.author.date = datetime(2024, 1, 5, tzinfo=timezone.utc) + commit.author.login = "contributor" + pr.get_commits.return_value = [commit] + assert bot.is_waiting_for_maintainer(pr, last_cr) is True + + def test_contributor_responded_maintainer_reviewed(self, bot_env): + """Contributor pushed, then maintainer submitted a review.""" + 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.author.login = "contributor" + pr.get_commits.return_value = [commit] + # Maintainer reviewed after contributor's commit + review = Mock() + review.user.login = "maintainer" + review.user.type = "User" + review.author_association = "MEMBER" + review.submitted_at = datetime(2024, 1, 7, tzinfo=timezone.utc) + 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.""" + 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.author.login = "contributor" + pr.get_commits.return_value = [commit] + comment = Mock() + comment.user.login = "maintainer" + comment.user.type = "User" + 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 + + def test_contributor_never_responded(self, bot_env): + """No contributor activity after changes requested → not waiting.""" + bot = StalePRBot() + pr = self._make_pr() + last_cr = datetime(2024, 1, 1, tzinfo=timezone.utc) + assert bot.is_waiting_for_maintainer(pr, last_cr) is False + + def test_bot_comments_are_ignored(self, bot_env): + """Bot comments should not count as maintainer activity.""" + 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.author.login = "contributor" + pr.get_commits.return_value = [commit] + # Only a bot comment exists after contributor's activity + bot_comment = Mock() + bot_comment.user.login = "github-actions[bot]" + bot_comment.user.type = "Bot" + bot_comment.author_association = "NONE" + bot_comment.created_at = datetime(2024, 1, 6, tzinfo=timezone.utc) + pr.get_issue_comments.return_value = [bot_comment] + assert bot.is_waiting_for_maintainer(pr, last_cr) is True + + def test_non_maintainer_comment_ignored(self, bot_env): + """A random community member commenting should not count as maintainer.""" + 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.author.login = "contributor" + pr.get_commits.return_value = [commit] + comment = Mock() + comment.user.login = "random_user" + comment.user.type = "User" + comment.author_association = "NONE" + 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 True + + def test_many_events_does_not_miss_contributor_activity(self, bot_env): + """Contributor activity must be found even with many subsequent events.""" + bot = StalePRBot() + pr = self._make_pr() + last_cr = datetime(2024, 1, 1, tzinfo=timezone.utc) + # Contributor pushed a commit early on + contributor_commit = Mock() + contributor_commit.commit.author.date = datetime( + 2024, 1, 2, tzinfo=timezone.utc + ) + contributor_commit.author.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.author.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 + + class TestUnassignLinkedIssues: def test_success(self, bot_env): bot = StalePRBot() @@ -236,6 +359,36 @@ def test_already_closed(self, bot_env): mock_pr.edit.assert_not_called() +class TestProcessStalePrs: + @patch("stale_pr_bot.datetime") + def test_skips_pr_waiting_for_maintainer(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.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), + ) + 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.author.login = "contributor" + mock_pr.get_commits.return_value = [commit] + # No maintainer activity + 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() + # PR should not be warned, staled, or closed + mock_pr.create_issue_comment.assert_not_called() + mock_pr.edit.assert_not_called() + + class TestRun: def test_no_github_client(self, bot_env): bot = StalePRBot()