From 5da5f30307635487c18216384cea36f048979eea Mon Sep 17 00:00:00 2001 From: Eeshu-Yadav Date: Fri, 8 May 2026 23:00:02 +0530 Subject: [PATCH 1/9] [fix] Stop stale-PR bot from closing PRs blocked only by bot reviews --- .../actions/bot-autoassign/stale_pr_bot.py | 42 ++- .../bot-autoassign/tests/test_stale_pr_bot.py | 275 +++++++++++++++++- 2 files changed, 302 insertions(+), 15 deletions(-) diff --git a/.github/actions/bot-autoassign/stale_pr_bot.py b/.github/actions/bot-autoassign/stale_pr_bot.py index 9404909c..4f604899 100644 --- a/.github/actions/bot-autoassign/stale_pr_bot.py +++ b/.github/actions/bot-autoassign/stale_pr_bot.py @@ -32,11 +32,21 @@ def _get_last_author_activity( 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 + author_date = commit.commit.author.date if commit.commit.author else None + committer_date = ( + commit.commit.committer.date if commit.commit.committer else None + ) + if author_date and committer_date: + commit_date = max(author_date, committer_date) + else: + commit_date = author_date or committer_date + if not commit_date or commit_date <= after_date: + continue + author_login = commit.author.login if commit.author else None + committer_login = commit.committer.login if commit.committer else None + if author_login == pr_author or committer_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: @@ -160,13 +170,25 @@ 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" + latest_per_reviewer = {} + for r in all_reviews: + if not r.user or not r.submitted_at: + continue + if r.user.type == "Bot": + continue + if r.state == "COMMENTED": + continue + current = latest_per_reviewer.get(r.user.login) + if current is None or r.submitted_at > current.submitted_at: + latest_per_reviewer[r.user.login] = r + blocking = [ + r + for r in latest_per_reviewer.values() + if r.state == "CHANGES_REQUESTED" ] - if not changes_requested_reviews: + if not blocking: return None - changes_requested_reviews.sort(key=lambda r: r.submitted_at, reverse=True) - return changes_requested_reviews[0].submitted_at + return max(blocking, key=lambda r: r.submitted_at).submitted_at except Exception as e: print("Error getting reviews" f" for PR #{pr.number}: {e}") return None 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..f905b8f2 100644 --- a/.github/actions/bot-autoassign/tests/test_stale_pr_bot.py +++ b/.github/actions/bot-autoassign/tests/test_stale_pr_bot.py @@ -77,6 +77,144 @@ 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 + class TestGetDaysSinceActivity: @patch("stale_pr_bot.datetime") @@ -92,7 +230,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 +243,63 @@ 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 + class TestIsWaitingForMaintainer: def _make_pr(self, author="contributor"): @@ -123,7 +320,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 +333,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() @@ -152,7 +353,9 @@ def test_contributor_responded_maintainer_commented(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 = "maintainer" @@ -176,7 +379,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 +399,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,14 +421,20 @@ 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 @@ -369,16 +582,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 +605,55 @@ 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() + class TestRun: def test_no_github_client(self, bot_env): From 8ec665cb23b991a871c8710485cf6d2b93064520 Mon Sep 17 00:00:00 2001 From: Eeshu-Yadav Date: Fri, 8 May 2026 23:44:13 +0530 Subject: [PATCH 2/9] [fix] Address review feedback on stale-PR bot fix --- .../actions/bot-autoassign/stale_pr_bot.py | 74 +++++++++++-------- .../bot-autoassign/tests/test_stale_pr_bot.py | 21 ++++++ docs/developer/reusable-github-utils.rst | 27 +++++++ 3 files changed, 92 insertions(+), 30 deletions(-) diff --git a/.github/actions/bot-autoassign/stale_pr_bot.py b/.github/actions/bot-autoassign/stale_pr_bot.py index 4f604899..b2ff5320 100644 --- a/.github/actions/bot-autoassign/stale_pr_bot.py +++ b/.github/actions/bot-autoassign/stale_pr_bot.py @@ -15,6 +15,25 @@ def __init__(self): self.DAYS_BEFORE_UNASSIGN = 14 self.DAYS_BEFORE_CLOSE = 60 + @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, @@ -32,21 +51,11 @@ def _get_last_author_activity( return None last_activity = None for commit in pr.get_commits(): - author_date = commit.commit.author.date if commit.commit.author else None - committer_date = ( - commit.commit.committer.date if commit.commit.committer else None - ) - if author_date and committer_date: - commit_date = max(author_date, committer_date) - else: - commit_date = author_date or committer_date + commit_date = self._commit_activity_date_for_author(commit, pr_author) if not commit_date or commit_date <= after_date: continue - author_login = commit.author.login if commit.author else None - committer_login = commit.committer.login if commit.committer else None - if author_login == pr_author or committer_login == pr_author: - if not last_activity or commit_date > last_activity: - last_activity = commit_date + 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: @@ -167,28 +176,33 @@ def is_waiting_for_maintainer( return False def get_last_changes_requested(self, pr, all_reviews=None): + """Timestamp of the latest CHANGES_REQUESTED that still represents + a human reviewer's current stance, or ``None``. + """ try: if all_reviews is None: all_reviews = list(pr.get_reviews()) + # Bot reviews are advisory; COMMENTED does not change stance. latest_per_reviewer = {} - for r in all_reviews: - if not r.user or not r.submitted_at: - continue - if r.user.type == "Bot": - continue - if r.state == "COMMENTED": + 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(r.user.login) - if current is None or r.submitted_at > current.submitted_at: - latest_per_reviewer[r.user.login] = r - blocking = [ - r - for r in latest_per_reviewer.values() - if r.state == "CHANGES_REQUESTED" - ] - if not blocking: - return None - return max(blocking, key=lambda r: r.submitted_at).submitted_at + 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, + ) except Exception as e: print("Error getting reviews" f" for PR #{pr.number}: {e}") return None 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 f905b8f2..e163889d 100644 --- a/.github/actions/bot-autoassign/tests/test_stale_pr_bot.py +++ b/.github/actions/bot-autoassign/tests/test_stale_pr_bot.py @@ -300,6 +300,27 @@ def test_both_unlinked_commit_skipped(self, mock_datetime, bot_env): 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"): diff --git a/docs/developer/reusable-github-utils.rst b/docs/developer/reusable-github-utils.rst index 26ab4329..aaa61698 100644 --- a/docs/developer/reusable-github-utils.rst +++ b/docs/developer/reusable-github-utils.rst @@ -73,6 +73,33 @@ OpenWISP repositories. The bot provides the following features: - **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 a maintainer (``OWNER``, ``MEMBER`` or + ``COLLABORATOR``) has commented or reviewed after the contributor's + last action, the PR is skipped — the ball is in the maintainer's court. +4. **Action by days inactive:** + + - **≥ 7 days:** posts a stale-warning comment. + - **≥ 14 days:** adds the ``stale`` label and unassigns the contributor + from linked issues. Pushing commits or replying revives the PR. + - **≥ 60 days:** closes the PR. Linked issues remain unassigned; + reopening the PR reassigns them and removes the ``stale`` label. + + Each stage posts at most once per blocking review cycle. + **Secrets** These secrets are used by the workflow to generate a ``GITHUB_TOKEN`` via From cf6f686ae9dc38430b25f9fd50aca3bdb0ccf47c Mon Sep 17 00:00:00 2001 From: Eeshu-Yadav Date: Sat, 9 May 2026 17:24:38 +0530 Subject: [PATCH 3/9] [change] Drop auto-close from stale-PR bot in favor of final follow-up --- .../actions/bot-autoassign/stale_pr_bot.py | 123 ++++++------------ .../bot-autoassign/tests/test_stale_pr_bot.py | 57 ++++---- docs/developer/reusable-github-utils.rst | 13 +- 3 files changed, 84 insertions(+), 109 deletions(-) diff --git a/.github/actions/bot-autoassign/stale_pr_bot.py b/.github/actions/bot-autoassign/stale_pr_bot.py index b2ff5320..a4ce141d 100644 --- a/.github/actions/bot-autoassign/stale_pr_bot.py +++ b/.github/actions/bot-autoassign/stale_pr_bot.py @@ -13,7 +13,7 @@ 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 @staticmethod def _commit_activity_date_for_author(commit, pr_author): @@ -244,75 +244,31 @@ def unassign_linked_issues(self, pr): 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") - return True + 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 = [ - "", - 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! 💙" - ), + followup_lines = [ + "", + f"Hi @{pr_author},", "", - ("**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): @@ -353,14 +309,6 @@ 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) @@ -469,27 +417,40 @@ def process_stale_prs(self): " 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, + ), + ) + 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 + if action(pr, days_inactive): + processed_count += 1 except Exception as e: print(f"Error processing" f" PR #{pr.number}: {e}") continue 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 e163889d..1f156ed2 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: @@ -563,34 +563,15 @@ def test_success(self, bot_env): mock_issue.remove_from_assignees.assert_called_once_with("testuser") -@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() @@ -675,6 +656,36 @@ def test_skips_pr_with_superseded_changes_requested(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_pr_first_processed_past_60_days_marks_stale_and_followup( + 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.get_labels.return_value = [] + 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 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() + 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 aaa61698..fb503d87 100644 --- a/docs/developer/reusable-github-utils.rst +++ b/docs/developer/reusable-github-utils.rst @@ -69,7 +69,8 @@ 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. @@ -92,11 +93,13 @@ The Stale PR job runs daily. For each open PR: last action, the PR is skipped — the ball is in the maintainer's court. 4. **Action by days inactive:** - - **≥ 7 days:** posts a stale-warning comment. + - **7–13 days:** posts a stale-warning comment. - **≥ 14 days:** adds the ``stale`` label and unassigns the contributor - from linked issues. Pushing commits or replying revives the PR. - - **≥ 60 days:** closes the PR. Linked issues remain unassigned; - reopening the PR reassigns them and removes the ``stale`` label. + from linked issues. An author comment on the PR clears the label and + reassigns the issues; a push alone only resets the inactivity timer. + - **≥ 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. From 6ae57a034b560bc88df73a8eada03077397cb485 Mon Sep 17 00:00:00 2001 From: Eeshu-Yadav Date: Sun, 10 May 2026 01:18:18 +0530 Subject: [PATCH 4/9] [fix] Refine stale-PR bot semantics per review feedback --- .../actions/bot-autoassign/stale_pr_bot.py | 149 ++++++++++-------- .../bot-autoassign/tests/test_stale_pr_bot.py | 83 ++++++++-- docs/developer/reusable-github-utils.rst | 7 +- 3 files changed, 160 insertions(+), 79 deletions(-) diff --git a/.github/actions/bot-autoassign/stale_pr_bot.py b/.github/actions/bot-autoassign/stale_pr_bot.py index a4ce141d..0a3591ae 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"}) @@ -14,6 +19,7 @@ def __init__(self): self.DAYS_BEFORE_STALE_WARNING = 7 self.DAYS_BEFORE_UNASSIGN = 14 self.DAYS_BEFORE_FINAL_FOLLOWUP = 60 + self.bot_login = os.environ.get("BOT_USERNAME", "openwisp-companion") + "[bot]" @staticmethod def _commit_activity_date_for_author(commit, pr_author): @@ -38,6 +44,7 @@ def _get_last_author_activity( self, pr, after_date, + commits=None, issue_comments=None, all_reviews=None, review_comments=None, @@ -50,7 +57,9 @@ def _get_last_author_activity( if not pr_author: return None last_activity = None - for commit in pr.get_commits(): + 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 @@ -86,6 +95,7 @@ def get_days_since_activity( self, pr, last_changes_requested, + commits=None, issue_comments=None, all_reviews=None, review_comments=None, @@ -96,6 +106,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, @@ -104,60 +115,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: @@ -172,8 +158,8 @@ 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): """Timestamp of the latest CHANGES_REQUESTED that still represents @@ -204,14 +190,12 @@ def get_last_changes_requested(self, pr, all_reviews=None): default=None, ) except Exception as e: - print("Error getting reviews" f" for PR #{pr.number}: {e}") + print(f"Error getting reviews for PR #{pr.number}: {e}") return 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. - - Uses HTML markers. If ``after_date`` is provided, - only considers comments posted after that date. + """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: @@ -220,7 +204,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: @@ -228,21 +212,45 @@ 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) + ) + + def _clear_stale_label(self, pr): + try: + if "stale" not in {label.name for label in pr.get_labels()}: + return False + pr.remove_from_labels("stale") + return True except Exception as e: - print(f"Error processing linked issues for PR #{pr.number}: {e}") - return 0 + 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: @@ -251,7 +259,7 @@ def send_final_followup(self, pr, days_inactive): return False followup_lines = [ "", - f"Hi @{pr_author},", + f"Hi @{pr_author} 👋,", "", ( f"This PR has been inactive for **{days_inactive} days**" @@ -310,20 +318,19 @@ def mark_pr_stale(self, pr, days_inactive): " We're happy to help! 🤝" ), ] - 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} as stale after {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): @@ -372,7 +379,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): @@ -394,27 +401,34 @@ def process_stale_prs(self): 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 stages = ( @@ -437,6 +451,7 @@ def process_stale_prs(self): self.send_final_followup, ), ) + posted_stale_this_run = False for low, high, marker, action in stages: if days_inactive < low: continue @@ -449,17 +464,19 @@ def process_stale_prs(self): issue_comments=issue_comments, ): continue + # Don't post final-followup in the same run as stale. + if marker == "final_followup" and posted_stale_this_run: + continue if action(pr, days_inactive): processed_count += 1 + if marker == "stale": + posted_stale_this_run = True 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}") @@ -467,7 +484,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 1f156ed2..4f08e67f 100644 --- a/.github/actions/bot-autoassign/tests/test_stale_pr_bot.py +++ b/.github/actions/bot-autoassign/tests/test_stale_pr_bot.py @@ -367,8 +367,8 @@ 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) @@ -384,7 +384,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.""" @@ -460,6 +460,13 @@ def test_many_events_does_not_miss_contributor_activity(self, bot_env): 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): @@ -497,23 +504,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) @@ -521,7 +536,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] @@ -562,6 +577,16 @@ 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() + class TestSendFinalFollowup: def test_success(self, bot_env): @@ -657,7 +682,7 @@ def test_skips_pr_with_superseded_changes_requested(self, mock_datetime, bot_env mock_pr.edit.assert_not_called() @patch("stale_pr_bot.datetime") - def test_pr_first_processed_past_60_days_marks_stale_and_followup( + 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) @@ -681,11 +706,49 @@ def test_pr_first_processed_past_60_days_marks_stale_and_followup( 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 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_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.get_labels.return_value = [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() + 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 fb503d87..4025a6fd 100644 --- a/docs/developer/reusable-github-utils.rst +++ b/docs/developer/reusable-github-utils.rst @@ -88,9 +88,10 @@ The Stale PR job runs daily. For each open PR: 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 a maintainer (``OWNER``, ``MEMBER`` or - ``COLLABORATOR``) has commented or reviewed after the contributor's - last action, the PR is skipped — the ball is in the maintainer's court. +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. From 299ebe3a2650198a0a1d7310f0405d7c7e891ae5 Mon Sep 17 00:00:00 2001 From: Eeshu-Yadav Date: Wed, 13 May 2026 13:04:18 +0530 Subject: [PATCH 5/9] [fix] Address review feedback on stale-PR bot fix --- .../actions/bot-autoassign/stale_pr_bot.py | 18 +++++------ .../bot-autoassign/tests/test_stale_pr_bot.py | 31 +++++++++++++++++++ docs/developer/reusable-github-utils.rst | 5 +-- 3 files changed, 43 insertions(+), 11 deletions(-) diff --git a/.github/actions/bot-autoassign/stale_pr_bot.py b/.github/actions/bot-autoassign/stale_pr_bot.py index 0a3591ae..1da14a0e 100644 --- a/.github/actions/bot-autoassign/stale_pr_bot.py +++ b/.github/actions/bot-autoassign/stale_pr_bot.py @@ -296,10 +296,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." ), "", ( @@ -325,7 +324,7 @@ def mark_pr_stale(self, pr, days_inactive): except Exception as e: print(f"Could not add stale label: {e}") print( - f"Marked PR #{pr.number} as stale after {days_inactive} days," + f"Marked PR #{pr.number} stale at {days_inactive} days," f" unassigned {unassigned_count} issues" ) return True @@ -398,6 +397,9 @@ 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()) @@ -411,8 +413,7 @@ def process_stale_prs(self): 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, @@ -427,8 +428,7 @@ def process_stale_prs(self): 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 stages = ( 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 4f08e67f..ade9803f 100644 --- a/.github/actions/bot-autoassign/tests/test_stale_pr_bot.py +++ b/.github/actions/bot-autoassign/tests/test_stale_pr_bot.py @@ -749,6 +749,37 @@ def test_clears_stale_label_when_contributor_responds(self, mock_datetime, bot_e 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.get_labels.return_value = [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 4025a6fd..d8e7e226 100644 --- a/docs/developer/reusable-github-utils.rst +++ b/docs/developer/reusable-github-utils.rst @@ -96,8 +96,9 @@ The Stale PR job runs daily. For each open PR: - **7–13 days:** posts a stale-warning comment. - **≥ 14 days:** adds the ``stale`` label and unassigns the contributor - from linked issues. An author comment on the PR clears the label and - reassigns the issues; a push alone only resets the inactivity timer. + 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. From 9a3f8d0c885060e25d13c2f651277e92317b74cb Mon Sep 17 00:00:00 2001 From: Eeshu-Yadav Date: Wed, 13 May 2026 13:57:52 +0530 Subject: [PATCH 6/9] [fix] Address CodeRabbit review on stale-PR bot fix --- .../actions/bot-autoassign/stale_pr_bot.py | 58 ++++++++++--------- .../bot-autoassign/tests/test_stale_pr_bot.py | 34 +++++++++++ 2 files changed, 64 insertions(+), 28 deletions(-) diff --git a/.github/actions/bot-autoassign/stale_pr_bot.py b/.github/actions/bot-autoassign/stale_pr_bot.py index 1da14a0e..4c3527d4 100644 --- a/.github/actions/bot-autoassign/stale_pr_bot.py +++ b/.github/actions/bot-autoassign/stale_pr_bot.py @@ -19,7 +19,10 @@ def __init__(self): self.DAYS_BEFORE_STALE_WARNING = 7 self.DAYS_BEFORE_UNASSIGN = 14 self.DAYS_BEFORE_FINAL_FOLLOWUP = 60 - self.bot_login = os.environ.get("BOT_USERNAME", "openwisp-companion") + "[bot]" + 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): @@ -164,34 +167,33 @@ def is_waiting_for_maintainer( def get_last_changes_requested(self, pr, all_reviews=None): """Timestamp of the latest CHANGES_REQUESTED that still represents a human reviewer's current stance, or ``None``. + + Errors propagate so the caller can distinguish "no active block" + from "couldn't determine" and skip the PR. """ - try: - 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, - ) - except Exception as e: - print(f"Error getting reviews for PR #{pr.number}: {e}") - return None + 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, + ) 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 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 ade9803f..9eb3f2f9 100644 --- a/.github/actions/bot-autoassign/tests/test_stale_pr_bot.py +++ b/.github/actions/bot-autoassign/tests/test_stale_pr_bot.py @@ -711,6 +711,40 @@ def test_pr_first_processed_past_60_days_marks_stale_only( mock_pr.add_to_labels.assert_called_once_with("stale") mock_pr.edit.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.get_labels.return_value = [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) From ccc287a5e0d99315df6b4dd9861b87195e2215f6 Mon Sep 17 00:00:00 2001 From: Federico Capoano Date: Wed, 13 May 2026 10:45:25 -0300 Subject: [PATCH 7/9] [chores] Added test_propagates_exception --- .github/actions/bot-autoassign/tests/test_stale_pr_bot.py | 7 +++++++ 1 file changed, 7 insertions(+) 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 9eb3f2f9..58871312 100644 --- a/.github/actions/bot-autoassign/tests/test_stale_pr_bot.py +++ b/.github/actions/bot-autoassign/tests/test_stale_pr_bot.py @@ -215,6 +215,13 @@ def test_review_without_submitted_at_skipped(self, bot_env): 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") From 908da14e3b8ead6a94f420984582d5f1ff1f8bf8 Mon Sep 17 00:00:00 2001 From: Federico Capoano Date: Wed, 13 May 2026 10:53:15 -0300 Subject: [PATCH 8/9] [chores] Avoided 1 HTTP request to get labels (already pre-fetched) --- .github/actions/bot-autoassign/stale_pr_bot.py | 3 ++- .github/actions/bot-autoassign/tests/test_stale_pr_bot.py | 8 ++++---- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/.github/actions/bot-autoassign/stale_pr_bot.py b/.github/actions/bot-autoassign/stale_pr_bot.py index 4c3527d4..bba9c85e 100644 --- a/.github/actions/bot-autoassign/stale_pr_bot.py +++ b/.github/actions/bot-autoassign/stale_pr_bot.py @@ -228,8 +228,9 @@ def unassign_linked_issues(self, pr): ) 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.get_labels()}: + if "stale" not in {label.name for label in pr.labels}: return False pr.remove_from_labels("stale") return True 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 58871312..5f82a099 100644 --- a/.github/actions/bot-autoassign/tests/test_stale_pr_bot.py +++ b/.github/actions/bot-autoassign/tests/test_stale_pr_bot.py @@ -708,7 +708,7 @@ def test_pr_first_processed_past_60_days_marks_stale_only( mock_pr.get_commits.return_value = [] mock_pr.get_issue_comments.return_value = [] mock_pr.get_review_comments.return_value = [] - mock_pr.get_labels.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] @@ -737,7 +737,7 @@ def test_final_followup_fires_after_prior_stale_run(self, mock_datetime, bot_env mock_pr.get_review_comments.return_value = [] stale_label = Mock() stale_label.name = "stale" - mock_pr.get_labels.return_value = [stale_label] + mock_pr.labels = [stale_label] prior_stale = Mock() prior_stale.user.login = bot.bot_login prior_stale.body = " previous run" @@ -775,7 +775,7 @@ def test_clears_stale_label_when_contributor_responds(self, mock_datetime, bot_e mock_pr.get_commits.return_value = [commit] stale_label = Mock() stale_label.name = "stale" - mock_pr.get_labels.return_value = [stale_label] + mock_pr.labels = [stale_label] mock_pr.get_issue_comments.return_value = [] mock_pr.get_review_comments.return_value = [] mock_issue = Mock() @@ -808,7 +808,7 @@ def test_clears_stale_label_when_no_active_block(self, mock_datetime, bot_env): mock_pr.get_reviews.return_value = [approve] stale_label = Mock() stale_label.name = "stale" - mock_pr.get_labels.return_value = [stale_label] + mock_pr.labels = [stale_label] mock_issue = Mock() mock_issue.number = 42 mock_issue.pull_request = None From 2610bfbb1922fd6610b9cff02d7e995e6626eb1f Mon Sep 17 00:00:00 2001 From: Federico Capoano Date: Wed, 13 May 2026 11:02:26 -0300 Subject: [PATCH 9/9] [chores] Handled cornern case (mark_pr_stale fails mid-execution) --- .../actions/bot-autoassign/stale_pr_bot.py | 9 ++++-- .../bot-autoassign/tests/test_stale_pr_bot.py | 32 +++++++++++++++++++ 2 files changed, 39 insertions(+), 2 deletions(-) diff --git a/.github/actions/bot-autoassign/stale_pr_bot.py b/.github/actions/bot-autoassign/stale_pr_bot.py index bba9c85e..3ebb50f6 100644 --- a/.github/actions/bot-autoassign/stale_pr_bot.py +++ b/.github/actions/bot-autoassign/stale_pr_bot.py @@ -470,10 +470,15 @@ def process_stale_prs(self): # 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 - if marker == "stale": - posted_stale_this_run = True except Exception as e: print(f"Error processing PR #{pr.number}: {e}") continue 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 5f82a099..4cc926d7 100644 --- a/.github/actions/bot-autoassign/tests/test_stale_pr_bot.py +++ b/.github/actions/bot-autoassign/tests/test_stale_pr_bot.py @@ -718,6 +718,38 @@ def test_pr_first_processed_past_60_days_marks_stale_only( 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)