From ab71f8c24907fc329036ba704af1d48af2374b77 Mon Sep 17 00:00:00 2001 From: Jose Alvarez Date: Tue, 27 Oct 2026 15:09:35 +0100 Subject: [PATCH 01/11] Updated state_issue_pr_ping script to use codeowners --- .github/scripts/stale_issue_pr_ping.py | 61 +++++++++-------- .github/tests/test_stale_issue_pr_ping.py | 81 ++++++++++------------- 2 files changed, 69 insertions(+), 73 deletions(-) diff --git a/.github/scripts/stale_issue_pr_ping.py b/.github/scripts/stale_issue_pr_ping.py index 9c865213ade..772b2b0b6fb 100644 --- a/.github/scripts/stale_issue_pr_ping.py +++ b/.github/scripts/stale_issue_pr_ping.py @@ -18,6 +18,7 @@ from github import Auth, Github, GithubException from github.Issue import Issue from github.IssueComment import IssueComment +from github.Repository import Repository PING_COMMENT = ( @@ -28,25 +29,34 @@ PINGED_LABEL = "requested-info" -def get_team_members(g: Github, org: str, team_slug: str) -> set[str]: - """Fetch active team member usernames.""" - try: - org_obj = g.get_organization(org) - team = org_obj.get_team_by_slug(team_slug) - return {m.login for m in team.get_members()} - except GithubException as exc: - if exc.status in (403, 404): - print( - f"ERROR: Failed to fetch team members for {org}/{team_slug} " - f"(HTTP {exc.status}). Check that the token has the 'read:org' " - f"scope and that the team slug '{team_slug}' is correct." - ) - else: - print(f"ERROR: Failed to fetch team members for {org}/{team_slug}: {exc}") - sys.exit(1) - except Exception as exc: - print(f"ERROR: Failed to fetch team members for {org}/{team_slug}: {exc}") - sys.exit(1) +def get_maintainers(repo: Repository) -> set[str]: + """Fetch maintainer usernames from CODEOWNERS on the default branch.""" + codeowners = repo.get_contents( + ".github/CODEOWNERS", + ref=repo.default_branch, + ) + + if isinstance(codeowners, list): + raise RuntimeError("Expected CODEOWNERS to be a file") + + content = codeowners.decoded_content.decode("utf-8") + owners = set() + + for line in content.splitlines(): + parts = line.split() + if not parts or parts[0] not in {"/python/", "/dotnet/"}: + continue + + owners.update( + owner.removeprefix("@").lower() + for owner in parts[1:] + if owner.startswith("@") + ) + + if not owners: + raise RuntimeError("No maintainers found in CODEOWNERS") + + return owners def find_last_team_comment( @@ -163,11 +173,6 @@ def main() -> None: print("ERROR: GITHUB_REPOSITORY environment variable is required") sys.exit(1) - team_slug = os.environ.get("TEAM_SLUG") - if not team_slug: - print("ERROR: TEAM_SLUG environment variable is required") - sys.exit(1) - days_threshold_raw = os.environ.get("DAYS_THRESHOLD", "4") try: days_threshold = int(days_threshold_raw) @@ -184,9 +189,13 @@ def main() -> None: g = Github(auth=Auth.Token(token)) repo = g.get_repo(repository) - print(f"Fetching team members for {org}/{team_slug}...") - team_members = get_team_members(g, org, team_slug) + print("Fetching maintainers...") + team_members = get_maintainers(repo) print(f"Found {len(team_members)} team members.\n") + for maintainer in sorted(team_members): + print(f" @{maintainer}") + + return now = datetime.now(timezone.utc) pinged = [] diff --git a/.github/tests/test_stale_issue_pr_ping.py b/.github/tests/test_stale_issue_pr_ping.py index b9a7ad5d434..7e68c7e25d3 100644 --- a/.github/tests/test_stale_issue_pr_ping.py +++ b/.github/tests/test_stale_issue_pr_ping.py @@ -20,7 +20,7 @@ TRIGGER_LABEL, author_replied_after, find_last_team_comment, - get_team_members, + get_maintainers, main, ping, should_ping, @@ -227,48 +227,45 @@ def test_all_retries_fail(self, mock_sleep): # --------------------------------------------------------------------------- -# get_team_members +# get_maintainers # --------------------------------------------------------------------------- -class TestGetTeamMembers: - def test_success(self): - g = MagicMock() - member = MagicMock() - member.login = "alice" - g.get_organization.return_value.get_team_by_slug.return_value.get_members.return_value = [member] - assert get_team_members(g, "org", "my-team") == {"alice"} +class TestGetMaintainers: + def test_fetches_and_parses_unique_python_and_dotnet_owners(self): + repo = MagicMock() + repo.default_branch = "feature-branch" + codeowners = MagicMock() + codeowners.decoded_content = b""" +/python/ @Alice @shared +/dotnet/ @BOB @Shared +/docs/ @ignored + """ + repo.get_contents.return_value = codeowners + + assert get_maintainers(repo) == {"alice", "bob", "shared"} + repo.get_contents.assert_called_once_with( + ".github/CODEOWNERS", + ref="feature-branch", + ) - def test_403_error_message(self, capsys): - from github import GithubException + def test_rejects_directory_response(self): + repo = MagicMock() + repo.get_contents.return_value = [MagicMock()] - g = MagicMock() - g.get_organization.return_value.get_team_by_slug.side_effect = GithubException( - 403, {"message": "Forbidden"}, None - ) - with pytest.raises(SystemExit): - get_team_members(g, "org", "my-team") - out = capsys.readouterr().out - assert "read:org" in out - assert "403" in out + with pytest.raises(RuntimeError, match="Expected CODEOWNERS to be a file"): + get_maintainers(repo) - def test_404_error_message(self, capsys): - from github import GithubException + def test_rejects_codeowners_without_matching_owners(self): + repo = MagicMock() + codeowners = MagicMock() + codeowners.decoded_content = b""" +/docs/ @docs-owner +/python/** @python-owner + """ + repo.get_contents.return_value = codeowners - g = MagicMock() - g.get_organization.return_value.get_team_by_slug.side_effect = GithubException( - 404, {"message": "Not Found"}, None - ) - with pytest.raises(SystemExit): - get_team_members(g, "org", "bad-slug") - out = capsys.readouterr().out - assert "read:org" in out - assert "bad-slug" in out - - def test_generic_error(self, capsys): - g = MagicMock() - g.get_organization.side_effect = RuntimeError("boom") - with pytest.raises(SystemExit): - get_team_members(g, "org", "team") + with pytest.raises(RuntimeError, match="No maintainers found in CODEOWNERS"): + get_maintainers(repo) # --------------------------------------------------------------------------- @@ -279,19 +276,9 @@ class TestMain: @patch.dict(os.environ, { "GITHUB_TOKEN": "tok", "GITHUB_REPOSITORY": "org/repo", - "TEAM_SLUG": "my-team", "DAYS_THRESHOLD": "abc", }, clear=True) def test_invalid_days_threshold(self, capsys): with pytest.raises(SystemExit): main() assert "numeric" in capsys.readouterr().out - - @patch.dict(os.environ, { - "GITHUB_TOKEN": "tok", - "GITHUB_REPOSITORY": "org/repo", - }, clear=True) - def test_missing_team_slug(self, capsys): - with pytest.raises(SystemExit): - main() - assert "TEAM_SLUG" in capsys.readouterr().out From b15a77a06b12baf506d690d7313531df7896ec70 Mon Sep 17 00:00:00 2001 From: Jose Alvarez Date: Tue, 27 Oct 2026 15:17:17 +0100 Subject: [PATCH 02/11] Removed workflow reference to TEAM_SLUG which is no longer used --- .github/scripts/stale_issue_pr_ping.py | 2 -- .github/workflows/stale-issue-pr-ping.yml | 1 - .gitignore | 2 +- 3 files changed, 1 insertion(+), 4 deletions(-) diff --git a/.github/scripts/stale_issue_pr_ping.py b/.github/scripts/stale_issue_pr_ping.py index 772b2b0b6fb..86e3084b0c1 100644 --- a/.github/scripts/stale_issue_pr_ping.py +++ b/.github/scripts/stale_issue_pr_ping.py @@ -195,8 +195,6 @@ def main() -> None: for maintainer in sorted(team_members): print(f" @{maintainer}") - return - now = datetime.now(timezone.utc) pinged = [] failed = [] diff --git a/.github/workflows/stale-issue-pr-ping.yml b/.github/workflows/stale-issue-pr-ping.yml index ce983390757..8904c1cba68 100644 --- a/.github/workflows/stale-issue-pr-ping.yml +++ b/.github/workflows/stale-issue-pr-ping.yml @@ -61,6 +61,5 @@ jobs: run: python .github/scripts/stale_issue_pr_ping.py env: GITHUB_TOKEN: ${{ steps.github-auth.outputs.token }} - TEAM_SLUG: ${{ secrets.DEVELOPER_TEAM }} DAYS_THRESHOLD: ${{ github.event.inputs.days_threshold || '4' }} DRY_RUN: ${{ github.event.inputs.dry_run || 'false' }} diff --git a/.gitignore b/.gitignore index 34bf943e24a..802ec0c0bf7 100644 --- a/.gitignore +++ b/.gitignore @@ -135,7 +135,7 @@ celerybeat.pid .env .venv env/ -venv/ +**/.venv*/ # Foundry agent CLI (contains secrets, auto-generated) .foundry-agent.json From 7c2e255cf2456b7e8a4ef9be770b6a7dc4af6f3d Mon Sep 17 00:00:00 2001 From: Jose Alvarez Date: Wed, 28 Oct 2026 13:23:39 +0100 Subject: [PATCH 03/11] Simplified should_ping policy and applying label to relevant issues --- .github/scripts/stale_issue_pr_ping.py | 91 +++++++++--- .github/tests/test_stale_issue_pr_ping.py | 163 +++++++++++++++++----- 2 files changed, 194 insertions(+), 60 deletions(-) diff --git a/.github/scripts/stale_issue_pr_ping.py b/.github/scripts/stale_issue_pr_ping.py index 86e3084b0c1..a117c1e7848 100644 --- a/.github/scripts/stale_issue_pr_ping.py +++ b/.github/scripts/stale_issue_pr_ping.py @@ -1,11 +1,10 @@ # Copyright (c) Microsoft. All rights reserved. -"""Scan open issues and PRs labeled 'waiting-for-author' for stale follow-ups. +"""Classify open issues and PRs waiting for external authors and ping stale items. -Team members manually add the 'waiting-for-author' label when they need a -response from the external author. If the author hasn't replied within -DAYS_THRESHOLD days of the last team comment, post a reminder and add the -'requested-info' label to prevent duplicate pings. +When a maintainer leaves the latest conversation comment on an external item, +the automation adds 'waiting-for-author'. On later runs, inactive items receive +a reminder and the 'requested-info' label to prevent duplicate pings. """ from __future__ import annotations @@ -83,6 +82,30 @@ def author_replied_after( return False +def should_mark_waiting(issue: Issue, team_members: set[str]) -> bool: + """Return whether an external item should gain 'waiting-for-author'. + + Policy: a CODEOWNER's latest conversation comment marks the item, and newly + marked items are not pinged until a later scheduled run. + """ + if issue.user.login in team_members: + return False + if any(label.name == TRIGGER_LABEL for label in issue.labels): + return False + if issue.comments == 0: + return False + + comments = list(issue.get_comments()) + if not comments: + return False + + latest_comment = comments[-1] + return bool( + latest_comment.user + and latest_comment.user.login in team_members + ) + + def should_ping( issue: Issue, team_members: set[str], @@ -106,27 +129,33 @@ def should_ping( if any(label.name == PINGED_LABEL for label in issue.labels): return False - # Skip if no comments at all - if issue.comments == 0: - return False + # Check if the issue has been inactive long enough + days_since = (now - issue.updated_at.astimezone(timezone.utc)).days + return days_since >= days_threshold - # Fetch comments once for both lookups - comments = list(issue.get_comments()) - # Find last team member comment - last_team_comment = find_last_team_comment(comments, team_members) - if last_team_comment is None: - return False +def mark_waiting(issue: Issue, dry_run: bool) -> bool: + """Add the 'waiting-for-author' label. Returns True on success.""" + author = issue.user.login + kind = "PR" if issue.pull_request else "Issue" - # Skip if author replied after the last team comment - if author_replied_after(comments, author, last_team_comment.created_at): - return False + if dry_run: + print( + f" [DRY RUN] Would mark {kind} #{issue.number} " + f"as waiting for @{author}" + ) + return True - # Check if enough days have passed - days_since = (now - last_team_comment.created_at.astimezone(timezone.utc)).days - if days_since < days_threshold: + try: + issue.add_to_labels(TRIGGER_LABEL) + except Exception as exc: + print( + f" ERROR: Failed to mark {kind} #{issue.number} " + f"as waiting for @{author}: {exc}" + ) return False + print(f" Marked {kind} #{issue.number} as waiting for @{author}") return True @@ -196,22 +225,38 @@ def main() -> None: print(f" @{maintainer}") now = datetime.now(timezone.utc) + marked = [] pinged = [] failed = [] scanned = 0 - print(f"Scanning open issues and PRs labeled '{TRIGGER_LABEL}' (threshold: {days_threshold} days)...\n") + print( + "Scanning open issues and PRs " + f"(stale threshold: {days_threshold} days)...\n" + ) - for issue in repo.get_issues(state="open", labels=[TRIGGER_LABEL]): + for issue in repo.get_issues(state="open"): scanned += 1 + if should_mark_waiting(issue, team_members): + if mark_waiting(issue, dry_run): + marked.append(issue.number) + else: + failed.append(issue.number) + continue + if should_ping(issue, team_members, days_threshold, now): if ping(issue, dry_run): pinged.append(issue.number) else: failed.append(issue.number) - print(f"\nDone. Scanned {scanned} items, pinged {len(pinged)}, failed {len(failed)}.") + print( + f"\nDone. Scanned {scanned} items, marked {len(marked)}, " + f"pinged {len(pinged)}, failed {len(failed)}." + ) + if marked: + print(f"Marked waiting: {', '.join(f'#{n}' for n in marked)}") if pinged: print(f"Pinged: {', '.join(f'#{n}' for n in pinged)}") if failed: diff --git a/.github/tests/test_stale_issue_pr_ping.py b/.github/tests/test_stale_issue_pr_ping.py index 7e68c7e25d3..e779c35ec05 100644 --- a/.github/tests/test_stale_issue_pr_ping.py +++ b/.github/tests/test_stale_issue_pr_ping.py @@ -22,7 +22,9 @@ find_last_team_comment, get_maintainers, main, + mark_waiting, ping, + should_mark_waiting, should_ping, ) @@ -59,6 +61,7 @@ def _make_issue( comments: list[MagicMock] | None = None, pull_request: bool = False, number: int = 42, + updated_at: datetime = NOW, ) -> MagicMock: issue = MagicMock() issue.user = MagicMock() @@ -70,6 +73,7 @@ def _make_issue( issue.labels = [_make_label(n) for n in labels] issue.comments = comment_count issue.pull_request = MagicMock() if pull_request else None + issue.updated_at = updated_at if comments is not None: issue.get_comments.return_value = comments return issue @@ -129,58 +133,90 @@ def test_deleted_user_comment(self): assert author_replied_after([c1], "external", after) is False +# --------------------------------------------------------------------------- +# should_mark_waiting +# --------------------------------------------------------------------------- + +class TestShouldMarkWaiting: + def test_mark_when_latest_comment_is_from_maintainer(self): + comments = [ + _make_comment("external", NOW - timedelta(days=2)), + _make_comment("alice", NOW - timedelta(days=1)), + ] + issue = _make_issue(labels=[], comments=comments, comment_count=2) + + assert should_mark_waiting(issue, TEAM) is True + + def test_skip_maintainer_author(self): + issue = _make_issue( + author="alice", + labels=[], + comments=[_make_comment("bob", NOW)], + ) + + assert should_mark_waiting(issue, TEAM) is False + issue.get_comments.assert_not_called() + + def test_skip_already_waiting_item(self): + issue = _make_issue( + comments=[_make_comment("alice", NOW)], + ) + + assert should_mark_waiting(issue, TEAM) is False + issue.get_comments.assert_not_called() + + def test_skip_item_without_comments(self): + issue = _make_issue(labels=[], comment_count=0) + + assert should_mark_waiting(issue, TEAM) is False + issue.get_comments.assert_not_called() + + def test_skip_when_latest_comment_is_not_from_maintainer(self): + comments = [ + _make_comment("alice", NOW - timedelta(days=2)), + _make_comment("external", NOW - timedelta(days=1)), + ] + issue = _make_issue(labels=[], comments=comments, comment_count=2) + + assert should_mark_waiting(issue, TEAM) is False + + # --------------------------------------------------------------------------- # should_ping # --------------------------------------------------------------------------- class TestShouldPing: - def test_should_ping_stale_issue(self): - team_comment = _make_comment("alice", NOW - timedelta(days=5)) - issue = _make_issue(comments=[team_comment], comment_count=1) - assert should_ping(issue, TEAM, 4, NOW) is True - - def test_skip_team_member_author(self): - issue = _make_issue(author="alice", labels=[TRIGGER_LABEL], comment_count=1) + def test_skip_recently_updated_issue(self): + issue = _make_issue(updated_at=NOW - timedelta(days=3)) assert should_ping(issue, TEAM, 4, NOW) is False - def test_skip_already_pinged(self): - issue = _make_issue(labels=[TRIGGER_LABEL, PINGED_LABEL], comment_count=1) - assert should_ping(issue, TEAM, 4, NOW) is False + def test_ping_issue_updated_at_threshold(self): + issue = _make_issue(updated_at=NOW - timedelta(days=4)) + assert should_ping(issue, TEAM, 4, NOW) is True - def test_skip_no_comments(self): - issue = _make_issue(comment_count=0) + def test_skip_maintainer_author(self): + issue = _make_issue(author="alice", updated_at=NOW - timedelta(days=4)) assert should_ping(issue, TEAM, 4, NOW) is False - def test_skip_no_team_comment(self): - c = _make_comment("external", NOW - timedelta(days=5)) - issue = _make_issue(comments=[c], comment_count=1) + def test_skip_already_warned_issue(self): + issue = _make_issue( + labels=[TRIGGER_LABEL, PINGED_LABEL], + updated_at=NOW - timedelta(days=4), + ) assert should_ping(issue, TEAM, 4, NOW) is False - def test_skip_author_replied(self): - team_c = _make_comment("alice", NOW - timedelta(days=5)) - author_c = _make_comment("external", NOW - timedelta(days=3)) - issue = _make_issue(comments=[team_c, author_c], comment_count=2) - assert should_ping(issue, TEAM, 4, NOW) is False - def test_skip_not_enough_days(self): - team_comment = _make_comment("alice", NOW - timedelta(days=2)) - issue = _make_issue(comments=[team_comment], comment_count=1) - assert should_ping(issue, TEAM, 4, NOW) is False +# --------------------------------------------------------------------------- +# mark_waiting +# --------------------------------------------------------------------------- - def test_aware_datetime_handled(self): - """Timezone-aware datetimes should not be mangled by astimezone.""" - aware_dt = (NOW - timedelta(days=5)).replace(tzinfo=timezone.utc) - team_comment = _make_comment("alice", aware_dt) - issue = _make_issue(comments=[team_comment], comment_count=1) - assert should_ping(issue, TEAM, 4, NOW) is True +class TestMarkWaiting: + def test_dry_run_does_not_apply_label(self, capsys): + issue = _make_issue(labels=[]) - def test_naive_datetime_handled(self): - """Naive datetimes (pre-PyGithub 2.x) should be handled by astimezone.""" - naive_dt = (NOW - timedelta(days=5)).replace(tzinfo=None) - team_comment = _make_comment("alice", naive_dt) - issue = _make_issue(comments=[team_comment], comment_count=1) - # astimezone on naive datetime treats it as local time; just verify no crash - should_ping(issue, TEAM, 4, NOW) + assert mark_waiting(issue, dry_run=True) is True + issue.add_to_labels.assert_not_called() + assert "DRY RUN" in capsys.readouterr().out # --------------------------------------------------------------------------- @@ -282,3 +318,56 @@ def test_invalid_days_threshold(self, capsys): with pytest.raises(SystemExit): main() assert "numeric" in capsys.readouterr().out + + @patch.dict(os.environ, { + "GITHUB_TOKEN": "tok", + "GITHUB_REPOSITORY": "org/repo", + }, clear=True) + @patch("stale_issue_pr_ping.get_maintainers", return_value=TEAM) + @patch("stale_issue_pr_ping.Github") + def test_scans_all_open_items_and_defers_newly_marked( + self, + mock_github, + mock_get_maintainers, + capsys, + ): + issue = _make_issue( + labels=[], + comments=[_make_comment("alice", NOW)], + ) + repo = mock_github.return_value.get_repo.return_value + repo.get_issues.return_value = [issue] + + main() + + mock_get_maintainers.assert_called_once_with(repo) + repo.get_issues.assert_called_once_with(state="open") + issue.add_to_labels.assert_called_once_with(TRIGGER_LABEL) + issue.create_comment.assert_not_called() + assert "marked 1, pinged 0" in capsys.readouterr().out + + @patch.dict(os.environ, { + "GITHUB_TOKEN": "tok", + "GITHUB_REPOSITORY": "org/repo", + }, clear=True) + @patch("stale_issue_pr_ping.datetime") + @patch("stale_issue_pr_ping.ping", return_value=True) + @patch("stale_issue_pr_ping.get_maintainers", return_value=TEAM) + @patch("stale_issue_pr_ping.Github") + def test_existing_waiting_item_continues_to_ping_check( + self, + mock_github, + mock_get_maintainers, + mock_ping, + mock_datetime, + ): + issue = _make_issue(updated_at=NOW - timedelta(days=4)) + repo = mock_github.return_value.get_repo.return_value + repo.get_issues.return_value = [issue] + mock_datetime.now.return_value = NOW + + main() + + mock_get_maintainers.assert_called_once_with(repo) + issue.get_comments.assert_not_called() + mock_ping.assert_called_once_with(issue, False) From cdae2eb18610f2acdf1d2211f67c986c07045f08 Mon Sep 17 00:00:00 2001 From: Jose Alvarez Date: Fri, 30 Oct 2026 10:30:46 +0100 Subject: [PATCH 04/11] add warn and closure to script. and default to dry run --- .github/scripts/stale_issue_pr_ping.py | 299 ++++++----- .github/tests/test_stale_issue_pr_ping.py | 626 ++++++++++++++-------- .github/workflows/stale-issue-pr-ping.yml | 14 +- 3 files changed, 571 insertions(+), 368 deletions(-) diff --git a/.github/scripts/stale_issue_pr_ping.py b/.github/scripts/stale_issue_pr_ping.py index a117c1e7848..c724918206b 100644 --- a/.github/scripts/stale_issue_pr_ping.py +++ b/.github/scripts/stale_issue_pr_ping.py @@ -1,10 +1,10 @@ # Copyright (c) Microsoft. All rights reserved. -"""Classify open issues and PRs waiting for external authors and ping stale items. +"""Warn and close stale external-authored pull requests. -When a maintainer leaves the latest conversation comment on an external item, -the automation adds 'waiting-for-author'. On later runs, inactive items receive -a reminder and the 'requested-info' label to prevent duplicate pings. +After a CODEOWNER's latest conversation comment, warn and apply +'requested-info' after the warning threshold, then close after the closing +threshold without activity. CODEOWNER-authored pull requests are exempt. """ from __future__ import annotations @@ -14,18 +14,23 @@ import time from datetime import datetime, timezone -from github import Auth, Github, GithubException -from github.Issue import Issue -from github.IssueComment import IssueComment +from github import Auth, Github +from github.PullRequest import PullRequest from github.Repository import Repository -PING_COMMENT = ( - "@{author}, friendly reminder — this issue is waiting on your response. " - "Please share any updates when you get a chance. (This is an automated message.)" +REQUESTED_INFO_LABEL = "requested-info" +WARNING_THRESHOLD_DAYS = 4 +CLOSING_THRESHOLD_DAYS = 4 +WARNING_COMMENT = ( + "@{author}, this pull request is waiting on your response. " + "If there is no further activity within {closing_days} days, " + "it will be closed automatically. (This is an automated message.)" +) +CLOSING_COMMENT = ( + "Closing this pull request because there was no further activity " + "after the automated warning. (This is an automated message.)" ) -TRIGGER_LABEL = "waiting-for-author" -PINGED_LABEL = "requested-info" def get_maintainers(repo: Repository) -> set[str]: @@ -58,139 +63,168 @@ def get_maintainers(repo: Repository) -> set[str]: return owners -def find_last_team_comment( - comments: list[IssueComment], team_members: set[str] -) -> IssueComment | None: - """Return the most recent comment from a team member, or None.""" - for comment in reversed(comments): - if comment.user and comment.user.login in team_members: - return comment - return None - - -def author_replied_after( - comments: list[IssueComment], author: str, after: datetime +def should_warn( + pull_request: PullRequest, + maintainers: set[str], + now: datetime, + warning_threshold_days: int, ) -> bool: - """Check if the issue author commented after the given timestamp.""" - for comment in comments: - if ( - comment.user - and comment.user.login == author - and comment.created_at > after - ): - return True - return False - - -def should_mark_waiting(issue: Issue, team_members: set[str]) -> bool: - """Return whether an external item should gain 'waiting-for-author'. - - Policy: a CODEOWNER's latest conversation comment marks the item, and newly - marked items are not pinged until a later scheduled run. - """ - if issue.user.login in team_members: + """Return whether an external pull request is ready for its warning.""" + if pull_request.user.login.lower() in maintainers: return False - if any(label.name == TRIGGER_LABEL for label in issue.labels): + if any(label.name == REQUESTED_INFO_LABEL for label in pull_request.labels): return False - if issue.comments == 0: + days_inactive = ( + now - pull_request.updated_at.astimezone(timezone.utc) + ).days + if days_inactive < warning_threshold_days: return False - comments = list(issue.get_comments()) + comments = list(pull_request.get_issue_comments()) if not comments: return False latest_comment = comments[-1] return bool( latest_comment.user - and latest_comment.user.login in team_members + and latest_comment.user.login.lower() in maintainers ) -def should_ping( - issue: Issue, - team_members: set[str], - days_threshold: int, +def should_close( + pull_request: PullRequest, + maintainers: set[str], now: datetime, + closing_threshold_days: int, ) -> bool: - """Determine whether this issue/PR should be pinged. - - Only issues/PRs carrying the 'waiting-for-author' label are candidates. - """ - author = issue.user.login - - # Skip if the trigger label is not present - if not any(label.name == TRIGGER_LABEL for label in issue.labels): + """Return whether a warned external pull request is ready to close.""" + if pull_request.user.login.lower() in maintainers: return False - # Skip if author is a team member - if author in team_members: + if not any( + label.name == REQUESTED_INFO_LABEL + for label in pull_request.labels + ): return False - # Skip if already pinged - if any(label.name == PINGED_LABEL for label in issue.labels): - return False + days_inactive = ( + now - pull_request.updated_at.astimezone(timezone.utc) + ).days + return days_inactive >= closing_threshold_days - # Check if the issue has been inactive long enough - days_since = (now - issue.updated_at.astimezone(timezone.utc)).days - return days_since >= days_threshold - -def mark_waiting(issue: Issue, dry_run: bool) -> bool: - """Add the 'waiting-for-author' label. Returns True on success.""" - author = issue.user.login - kind = "PR" if issue.pull_request else "Issue" +def warn( + pull_request: PullRequest, + dry_run: bool, + closing_threshold_days: int, +) -> bool: + """Post the warning and add 'requested-info'. Returns True on success.""" + author = pull_request.user.login if dry_run: print( - f" [DRY RUN] Would mark {kind} #{issue.number} " - f"as waiting for @{author}" + f" [DRY RUN] Would warn PR #{pull_request.number} (@{author}) " + f"and add '{REQUESTED_INFO_LABEL}'" ) return True - try: - issue.add_to_labels(TRIGGER_LABEL) - except Exception as exc: - print( - f" ERROR: Failed to mark {kind} #{issue.number} " - f"as waiting for @{author}: {exc}" - ) - return False - - print(f" Marked {kind} #{issue.number} as waiting for @{author}") - return True + max_retries = 3 + commented = False + labeled = False + for attempt in range(1, max_retries + 1): + try: + if not commented: + pull_request.create_issue_comment( + WARNING_COMMENT.format( + author=author, + closing_days=closing_threshold_days, + ) + ) + commented = True + if not labeled: + pull_request.add_to_labels(REQUESTED_INFO_LABEL) + labeled = True + print(f" Warned PR #{pull_request.number} (@{author})") + return True + except Exception as exc: + if attempt < max_retries: + wait = 2 ** attempt # 2s, 4s + print( + f" WARN: Warning attempt {attempt}/{max_retries} failed " + f"for PR #{pull_request.number}: {exc}. " + f"Retrying in {wait}s..." + ) + time.sleep(wait) + else: + print( + f" ERROR: Failed to warn PR #{pull_request.number} after " + f"{max_retries} attempts: {exc}" + ) + return False -def ping(issue: Issue, dry_run: bool) -> bool: - """Post a reminder comment and add the 'requested-info' label. Returns True on success.""" - author = issue.user.login - kind = "PR" if issue.pull_request else "Issue" +def close(pull_request: PullRequest, dry_run: bool) -> bool: + """Post the closing message and close the PR. Returns True on success.""" + author = pull_request.user.login if dry_run: - print(f" [DRY RUN] Would ping {kind} #{issue.number} (@{author})") + print( + f" [DRY RUN] Would comment on and close " + f"PR #{pull_request.number} (@{author})" + ) return True max_retries = 3 commented = False - labeled = False for attempt in range(1, max_retries + 1): try: if not commented: - issue.create_comment(PING_COMMENT.format(author=author)) + pull_request.create_issue_comment(CLOSING_COMMENT) commented = True - if not labeled: - issue.add_to_labels(PINGED_LABEL) - labeled = True - print(f" Pinged {kind} #{issue.number} (@{author})") + pull_request.edit(state="closed") + print(f" Closed PR #{pull_request.number} (@{author})") return True except Exception as exc: if attempt < max_retries: wait = 2 ** attempt # 2s, 4s - print(f" WARN: Attempt {attempt}/{max_retries} failed for {kind} #{issue.number}: {exc}. Retrying in {wait}s...") + print( + f" WARN: Closing attempt {attempt}/{max_retries} failed " + f"for PR #{pull_request.number}: {exc}. " + f"Retrying in {wait}s..." + ) time.sleep(wait) else: - print(f" ERROR: Failed to ping {kind} #{issue.number} after {max_retries} attempts: {exc}") + print( + f" ERROR: Failed to close PR #{pull_request.number} after " + f"{max_retries} attempts: {exc}" + ) return False +def get_runtime_thresholds() -> tuple[int, int]: + """Return warning and closing thresholds, applying DAYS_THRESHOLD to both.""" + override = os.environ.get("DAYS_THRESHOLD") + if override is None: + return WARNING_THRESHOLD_DAYS, CLOSING_THRESHOLD_DAYS + + try: + threshold = int(override) + except ValueError: + print( + "ERROR: DAYS_THRESHOLD must be a non-negative integer, " + f"got '{override}'" + ) + sys.exit(1) + + if threshold < 0: + print( + "ERROR: DAYS_THRESHOLD must be a non-negative integer, " + f"got '{override}'" + ) + sys.exit(1) + + return threshold, threshold + + def main() -> None: token = os.environ.get("GITHUB_TOKEN") if not token: @@ -202,63 +236,72 @@ def main() -> None: print("ERROR: GITHUB_REPOSITORY environment variable is required") sys.exit(1) - days_threshold_raw = os.environ.get("DAYS_THRESHOLD", "4") - try: - days_threshold = int(days_threshold_raw) - except ValueError: - print(f"ERROR: DAYS_THRESHOLD must be a numeric value, got '{days_threshold_raw}'") - sys.exit(1) + warning_threshold_days, closing_threshold_days = get_runtime_thresholds() dry_run = os.environ.get("DRY_RUN", "false").lower() == "true" - org = repository.split("/")[0] - if dry_run: - print("Running in DRY RUN mode — no comments or labels will be applied.\n") + print("Running in DRY RUN mode — no comments, labels, or closures will be applied.\n") g = Github(auth=Auth.Token(token)) repo = g.get_repo(repository) print("Fetching maintainers...") - team_members = get_maintainers(repo) - print(f"Found {len(team_members)} team members.\n") - for maintainer in sorted(team_members): + maintainers = get_maintainers(repo) + print(f"Found {len(maintainers)} maintainers.\n") + for maintainer in sorted(maintainers): print(f" @{maintainer}") now = datetime.now(timezone.utc) - marked = [] - pinged = [] + warned = [] + closed = [] failed = [] scanned = 0 print( - "Scanning open issues and PRs " - f"(stale threshold: {days_threshold} days)...\n" + "Scanning open pull requests " + f"(warning: {warning_threshold_days} days, " + f"closing: {closing_threshold_days} days)...\n" ) - for issue in repo.get_issues(state="open"): + for pull_request in repo.get_pulls(state="open"): scanned += 1 - if should_mark_waiting(issue, team_members): - if mark_waiting(issue, dry_run): - marked.append(issue.number) + if should_close( + pull_request, + maintainers, + now, + closing_threshold_days, + ): + if close(pull_request, dry_run): + closed.append(pull_request.number) else: - failed.append(issue.number) + failed.append(pull_request.number) continue - if should_ping(issue, team_members, days_threshold, now): - if ping(issue, dry_run): - pinged.append(issue.number) + if should_warn( + pull_request, + maintainers, + now, + warning_threshold_days, + ): + if warn( + pull_request, + dry_run, + closing_threshold_days, + ): + warned.append(pull_request.number) else: - failed.append(issue.number) + failed.append(pull_request.number) + continue print( - f"\nDone. Scanned {scanned} items, marked {len(marked)}, " - f"pinged {len(pinged)}, failed {len(failed)}." + f"\nDone. Scanned {scanned} PRs, warned {len(warned)}, " + f"closed {len(closed)}, failed {len(failed)}." ) - if marked: - print(f"Marked waiting: {', '.join(f'#{n}' for n in marked)}") - if pinged: - print(f"Pinged: {', '.join(f'#{n}' for n in pinged)}") + if warned: + print(f"Warned: {', '.join(f'#{n}' for n in warned)}") + if closed: + print(f"Closed: {', '.join(f'#{n}' for n in closed)}") if failed: print(f"Failed: {', '.join(f'#{n}' for n in failed)}") sys.exit(1) diff --git a/.github/tests/test_stale_issue_pr_ping.py b/.github/tests/test_stale_issue_pr_ping.py index e779c35ec05..5771a905003 100644 --- a/.github/tests/test_stale_issue_pr_ping.py +++ b/.github/tests/test_stale_issue_pr_ping.py @@ -6,265 +6,331 @@ import os import sys -from datetime import datetime, timezone, timedelta -from unittest.mock import MagicMock, patch +from datetime import datetime, timedelta, timezone +from unittest.mock import MagicMock, call, patch import pytest +from github import GithubException # Ensure the script directory is importable sys.path.insert(0, os.path.join(os.path.dirname(__file__), "..", "scripts")) from stale_issue_pr_ping import ( - PINGED_LABEL, - PING_COMMENT, - TRIGGER_LABEL, - author_replied_after, - find_last_team_comment, + CLOSING_COMMENT, + CLOSING_THRESHOLD_DAYS, + REQUESTED_INFO_LABEL, + WARNING_COMMENT, + WARNING_THRESHOLD_DAYS, + close, get_maintainers, + get_runtime_thresholds, main, - mark_waiting, - ping, - should_mark_waiting, - should_ping, + should_close, + should_warn, + warn, ) -TEAM = {"alice", "bob"} +MAINTAINERS = {"alice", "bob"} NOW = datetime(2026, 3, 15, 12, 0, 0, tzinfo=timezone.utc) -# --------------------------------------------------------------------------- -# Helpers -# --------------------------------------------------------------------------- - -def _make_comment(login: str | None, created_at: datetime) -> MagicMock: - """Create a mock IssueComment.""" - c = MagicMock() +def _make_comment(login: str | None) -> MagicMock: + comment = MagicMock() if login is None: - c.user = None + comment.user = None else: - c.user = MagicMock() - c.user.login = login - c.created_at = created_at - return c + comment.user = MagicMock() + comment.user.login = login + return comment def _make_label(name: str) -> MagicMock: - lbl = MagicMock() - lbl.name = name - return lbl + label = MagicMock() + label.name = name + return label -def _make_issue( +def _make_pull_request( author: str = "external", labels: list[str] | None = None, - comment_count: int = 1, comments: list[MagicMock] | None = None, - pull_request: bool = False, number: int = 42, updated_at: datetime = NOW, ) -> MagicMock: - issue = MagicMock() - issue.user = MagicMock() - issue.user.login = author - issue.number = number - # Default to having the trigger label, since the API query pre-filters. - if labels is None: - labels = [TRIGGER_LABEL] - issue.labels = [_make_label(n) for n in labels] - issue.comments = comment_count - issue.pull_request = MagicMock() if pull_request else None - issue.updated_at = updated_at - if comments is not None: - issue.get_comments.return_value = comments - return issue - - -# --------------------------------------------------------------------------- -# find_last_team_comment -# --------------------------------------------------------------------------- - -class TestFindLastTeamComment: - def test_returns_last_team_comment(self): - c1 = _make_comment("alice", datetime(2026, 3, 1, tzinfo=timezone.utc)) - c2 = _make_comment("external", datetime(2026, 3, 2, tzinfo=timezone.utc)) - c3 = _make_comment("bob", datetime(2026, 3, 3, tzinfo=timezone.utc)) - assert find_last_team_comment([c1, c2, c3], TEAM) is c3 - - def test_returns_none_when_no_team_comments(self): - c1 = _make_comment("external", datetime(2026, 3, 1, tzinfo=timezone.utc)) - assert find_last_team_comment([c1], TEAM) is None - - def test_returns_none_for_empty_list(self): - assert find_last_team_comment([], TEAM) is None - - def test_skips_deleted_user(self): - c1 = _make_comment(None, datetime(2026, 3, 1, tzinfo=timezone.utc)) - c2 = _make_comment("alice", datetime(2026, 3, 2, tzinfo=timezone.utc)) - assert find_last_team_comment([c1, c2], TEAM) is c2 - - def test_only_deleted_users(self): - c1 = _make_comment(None, datetime(2026, 3, 1, tzinfo=timezone.utc)) - assert find_last_team_comment([c1], TEAM) is None - - -# --------------------------------------------------------------------------- -# author_replied_after -# --------------------------------------------------------------------------- - -class TestAuthorRepliedAfter: - def test_author_replied(self): - after = datetime(2026, 3, 1, tzinfo=timezone.utc) - c1 = _make_comment("external", datetime(2026, 3, 2, tzinfo=timezone.utc)) - assert author_replied_after([c1], "external", after) is True - - def test_author_not_replied(self): - after = datetime(2026, 3, 5, tzinfo=timezone.utc) - c1 = _make_comment("external", datetime(2026, 3, 2, tzinfo=timezone.utc)) - assert author_replied_after([c1], "external", after) is False - - def test_different_user_replied(self): - after = datetime(2026, 3, 1, tzinfo=timezone.utc) - c1 = _make_comment("someone_else", datetime(2026, 3, 2, tzinfo=timezone.utc)) - assert author_replied_after([c1], "external", after) is False - - def test_deleted_user_comment(self): - after = datetime(2026, 3, 1, tzinfo=timezone.utc) - c1 = _make_comment(None, datetime(2026, 3, 2, tzinfo=timezone.utc)) - assert author_replied_after([c1], "external", after) is False - - -# --------------------------------------------------------------------------- -# should_mark_waiting -# --------------------------------------------------------------------------- - -class TestShouldMarkWaiting: - def test_mark_when_latest_comment_is_from_maintainer(self): - comments = [ - _make_comment("external", NOW - timedelta(days=2)), - _make_comment("alice", NOW - timedelta(days=1)), - ] - issue = _make_issue(labels=[], comments=comments, comment_count=2) + pull_request = MagicMock() + pull_request.user = MagicMock() + pull_request.user.login = author + pull_request.number = number + pull_request.labels = [ + _make_label(name) + for name in (labels or []) + ] + pull_request.updated_at = updated_at + pull_request.get_issue_comments.return_value = comments or [] + return pull_request + + +class TestPolicyThresholds: + def test_default_constants(self): + assert WARNING_THRESHOLD_DAYS == 4 + assert CLOSING_THRESHOLD_DAYS == 4 + + @patch.dict(os.environ, {}, clear=True) + def test_runtime_defaults(self): + assert get_runtime_thresholds() == (4, 4) + + @patch.dict(os.environ, {"DAYS_THRESHOLD": "7"}, clear=True) + def test_runtime_override_applies_to_both_stages(self): + assert get_runtime_thresholds() == (7, 7) + + @patch.dict(os.environ, {"DAYS_THRESHOLD": "0"}, clear=True) + def test_zero_threshold_is_valid(self): + assert get_runtime_thresholds() == (0, 0) + + @patch.dict(os.environ, {"DAYS_THRESHOLD": "invalid"}, clear=True) + def test_invalid_runtime_override_is_rejected(self, capsys): + with pytest.raises(SystemExit): + get_runtime_thresholds() + + assert "non-negative integer" in capsys.readouterr().out + + @patch.dict(os.environ, {"DAYS_THRESHOLD": "-1"}, clear=True) + def test_negative_runtime_override_is_rejected(self, capsys): + with pytest.raises(SystemExit): + get_runtime_thresholds() + + assert "non-negative integer" in capsys.readouterr().out + + +class TestShouldWarn: + def test_warns_at_threshold_for_latest_maintainer_comment(self): + pull_request = _make_pull_request( + comments=[ + _make_comment("external"), + _make_comment("alice"), + ], + updated_at=NOW - timedelta(days=4), + ) + + assert should_warn(pull_request, MAINTAINERS, NOW, 4) is True + pull_request.get_issue_comments.assert_called_once_with() - assert should_mark_waiting(issue, TEAM) is True + def test_recent_activity_resets_warning_clock(self): + pull_request = _make_pull_request( + comments=[_make_comment("alice")], + updated_at=NOW - timedelta(days=3), + ) + + assert should_warn(pull_request, MAINTAINERS, NOW, 4) is False + pull_request.get_issue_comments.assert_not_called() + + def test_maintainer_authored_pr_is_exempt(self): + pull_request = _make_pull_request( + author="Alice", + comments=[_make_comment("bob")], + updated_at=NOW - timedelta(days=4), + ) + + assert should_warn(pull_request, MAINTAINERS, NOW, 4) is False + pull_request.get_issue_comments.assert_not_called() - def test_skip_maintainer_author(self): - issue = _make_issue( - author="alice", - labels=[], - comments=[_make_comment("bob", NOW)], + def test_requested_info_pr_is_not_warned_again(self): + pull_request = _make_pull_request( + labels=[REQUESTED_INFO_LABEL], + comments=[_make_comment("alice")], + updated_at=NOW - timedelta(days=4), ) - assert should_mark_waiting(issue, TEAM) is False - issue.get_comments.assert_not_called() + assert should_warn(pull_request, MAINTAINERS, NOW, 4) is False + pull_request.get_issue_comments.assert_not_called() - def test_skip_already_waiting_item(self): - issue = _make_issue( - comments=[_make_comment("alice", NOW)], + def test_pr_without_regular_conversation_comments_is_skipped(self): + pull_request = _make_pull_request( + comments=[], + updated_at=NOW - timedelta(days=4), ) - assert should_mark_waiting(issue, TEAM) is False - issue.get_comments.assert_not_called() + assert should_warn(pull_request, MAINTAINERS, NOW, 4) is False + pull_request.get_issue_comments.assert_called_once_with() - def test_skip_item_without_comments(self): - issue = _make_issue(labels=[], comment_count=0) + def test_latest_external_comment_is_skipped(self): + pull_request = _make_pull_request( + comments=[ + _make_comment("alice"), + _make_comment("external"), + ], + updated_at=NOW - timedelta(days=4), + ) - assert should_mark_waiting(issue, TEAM) is False - issue.get_comments.assert_not_called() + assert should_warn(pull_request, MAINTAINERS, NOW, 4) is False - def test_skip_when_latest_comment_is_not_from_maintainer(self): - comments = [ - _make_comment("alice", NOW - timedelta(days=2)), - _make_comment("external", NOW - timedelta(days=1)), + def test_deleted_latest_comment_author_is_skipped(self): + pull_request = _make_pull_request( + comments=[_make_comment(None)], + updated_at=NOW - timedelta(days=4), + ) + + assert should_warn(pull_request, MAINTAINERS, NOW, 4) is False + + +class TestWarn: + def test_dry_run_mutates_nothing(self, capsys): + pull_request = _make_pull_request() + + assert warn(pull_request, dry_run=True, closing_threshold_days=6) is True + pull_request.create_issue_comment.assert_not_called() + pull_request.add_to_labels.assert_not_called() + output = capsys.readouterr().out + assert "DRY RUN" in output + assert REQUESTED_INFO_LABEL in output + + def test_posts_warning_then_applies_requested_info(self): + pull_request = _make_pull_request() + + assert warn(pull_request, dry_run=False, closing_threshold_days=6) is True + assert pull_request.method_calls == [ + call.create_issue_comment( + WARNING_COMMENT.format( + author="external", + closing_days=6, + ) + ), + call.add_to_labels(REQUESTED_INFO_LABEL), ] - issue = _make_issue(labels=[], comments=comments, comment_count=2) - assert should_mark_waiting(issue, TEAM) is False + @patch("stale_issue_pr_ping.time.sleep") + def test_comment_retry_succeeds(self, mock_sleep): + pull_request = _make_pull_request() + pull_request.create_issue_comment.side_effect = [ + Exception("network error"), + None, + ] + assert warn(pull_request, dry_run=False, closing_threshold_days=4) is True + assert pull_request.create_issue_comment.call_count == 2 + pull_request.add_to_labels.assert_called_once_with(REQUESTED_INFO_LABEL) + mock_sleep.assert_called_once() -# --------------------------------------------------------------------------- -# should_ping -# --------------------------------------------------------------------------- + @patch("stale_issue_pr_ping.time.sleep") + def test_label_retry_does_not_repeat_warning(self, mock_sleep): + pull_request = _make_pull_request() + pull_request.add_to_labels.side_effect = [ + Exception("label error"), + None, + ] -class TestShouldPing: - def test_skip_recently_updated_issue(self): - issue = _make_issue(updated_at=NOW - timedelta(days=3)) - assert should_ping(issue, TEAM, 4, NOW) is False + assert warn(pull_request, dry_run=False, closing_threshold_days=4) is True + pull_request.create_issue_comment.assert_called_once() + assert pull_request.add_to_labels.call_count == 2 + mock_sleep.assert_called_once() - def test_ping_issue_updated_at_threshold(self): - issue = _make_issue(updated_at=NOW - timedelta(days=4)) - assert should_ping(issue, TEAM, 4, NOW) is True + @patch("stale_issue_pr_ping.time.sleep") + def test_warning_failure_never_applies_label(self, mock_sleep): + pull_request = _make_pull_request() + pull_request.create_issue_comment.side_effect = Exception("comment error") + + assert warn(pull_request, dry_run=False, closing_threshold_days=4) is False + assert pull_request.create_issue_comment.call_count == 3 + pull_request.add_to_labels.assert_not_called() + + @patch("stale_issue_pr_ping.time.sleep") + def test_final_label_failure_reports_failure_without_repeating_warning( + self, + mock_sleep, + ): + pull_request = _make_pull_request() + pull_request.add_to_labels.side_effect = Exception("label error") - def test_skip_maintainer_author(self): - issue = _make_issue(author="alice", updated_at=NOW - timedelta(days=4)) - assert should_ping(issue, TEAM, 4, NOW) is False + assert warn(pull_request, dry_run=False, closing_threshold_days=4) is False + pull_request.create_issue_comment.assert_called_once() + assert pull_request.add_to_labels.call_count == 3 + assert mock_sleep.call_count == 2 - def test_skip_already_warned_issue(self): - issue = _make_issue( - labels=[TRIGGER_LABEL, PINGED_LABEL], + +class TestShouldClose: + def test_closes_requested_info_pr_at_threshold(self): + pull_request = _make_pull_request( + labels=[REQUESTED_INFO_LABEL], updated_at=NOW - timedelta(days=4), ) - assert should_ping(issue, TEAM, 4, NOW) is False + assert should_close(pull_request, MAINTAINERS, NOW, 4) is True -# --------------------------------------------------------------------------- -# mark_waiting -# --------------------------------------------------------------------------- + def test_recent_activity_resets_closing_clock(self): + pull_request = _make_pull_request( + labels=[REQUESTED_INFO_LABEL], + updated_at=NOW - timedelta(days=3), + ) -class TestMarkWaiting: - def test_dry_run_does_not_apply_label(self, capsys): - issue = _make_issue(labels=[]) + assert should_close(pull_request, MAINTAINERS, NOW, 4) is False - assert mark_waiting(issue, dry_run=True) is True - issue.add_to_labels.assert_not_called() - assert "DRY RUN" in capsys.readouterr().out + def test_maintainer_authored_pr_is_exempt(self): + pull_request = _make_pull_request( + author="Alice", + labels=[REQUESTED_INFO_LABEL], + updated_at=NOW - timedelta(days=4), + ) + assert should_close(pull_request, MAINTAINERS, NOW, 4) is False -# --------------------------------------------------------------------------- -# ping -# --------------------------------------------------------------------------- + def test_unlabeled_pr_is_not_closed(self): + pull_request = _make_pull_request( + updated_at=NOW - timedelta(days=4), + ) + + assert should_close(pull_request, MAINTAINERS, NOW, 4) is False -class TestPing: - def test_dry_run(self, capsys): - issue = _make_issue() - assert ping(issue, dry_run=True) is True - issue.create_comment.assert_not_called() + +class TestClose: + def test_dry_run_mutates_nothing(self, capsys): + pull_request = _make_pull_request() + + assert close(pull_request, dry_run=True) is True + pull_request.create_issue_comment.assert_not_called() + pull_request.edit.assert_not_called() assert "DRY RUN" in capsys.readouterr().out - def test_success(self, capsys): - issue = _make_issue() - assert ping(issue, dry_run=False) is True - issue.create_comment.assert_called_once() - issue.add_to_labels.assert_called_once_with(PINGED_LABEL) + def test_posts_closing_comment_then_closes_pr(self): + pull_request = _make_pull_request() + + assert close(pull_request, dry_run=False) is True + assert pull_request.method_calls == [ + call.create_issue_comment(CLOSING_COMMENT), + call.edit(state="closed"), + ] @patch("stale_issue_pr_ping.time.sleep") - def test_retry_on_failure(self, mock_sleep): - issue = _make_issue() - issue.create_comment.side_effect = [Exception("net error"), None] - assert ping(issue, dry_run=False) is True - assert issue.create_comment.call_count == 2 + def test_close_retry_does_not_repeat_comment(self, mock_sleep): + pull_request = _make_pull_request() + pull_request.edit.side_effect = [Exception("close error"), None] + + assert close(pull_request, dry_run=False) is True + pull_request.create_issue_comment.assert_called_once_with(CLOSING_COMMENT) + assert pull_request.edit.call_count == 2 mock_sleep.assert_called_once() @patch("stale_issue_pr_ping.time.sleep") - def test_idempotent_retry_skips_comment_on_label_failure(self, mock_sleep): - """If create_comment succeeds but add_to_labels fails, retry should not re-comment.""" - issue = _make_issue() - issue.add_to_labels.side_effect = [Exception("label error"), None] - assert ping(issue, dry_run=False) is True - # Comment should only be created once even though there were 2 attempts - assert issue.create_comment.call_count == 1 - assert issue.add_to_labels.call_count == 2 + def test_comment_failure_never_closes_pr(self, mock_sleep): + pull_request = _make_pull_request() + pull_request.create_issue_comment.side_effect = Exception("comment error") + + assert close(pull_request, dry_run=False) is False + assert pull_request.create_issue_comment.call_count == 3 + pull_request.edit.assert_not_called() @patch("stale_issue_pr_ping.time.sleep") - def test_all_retries_fail(self, mock_sleep): - issue = _make_issue() - issue.create_comment.side_effect = Exception("permanent error") - assert ping(issue, dry_run=False) is False - assert issue.create_comment.call_count == 3 + def test_final_close_failure_reports_failure_without_repeating_comment( + self, + mock_sleep, + ): + pull_request = _make_pull_request() + pull_request.edit.side_effect = Exception("close error") + assert close(pull_request, dry_run=False) is False + pull_request.create_issue_comment.assert_called_once_with(CLOSING_COMMENT) + assert pull_request.edit.call_count == 3 + assert mock_sleep.call_count == 2 -# --------------------------------------------------------------------------- -# get_maintainers -# --------------------------------------------------------------------------- class TestGetMaintainers: def test_fetches_and_parses_unique_python_and_dotnet_owners(self): @@ -275,7 +341,7 @@ def test_fetches_and_parses_unique_python_and_dotnet_owners(self): /python/ @Alice @shared /dotnet/ @BOB @Shared /docs/ @ignored - """ +""" repo.get_contents.return_value = codeowners assert get_maintainers(repo) == {"alice", "bob", "shared"} @@ -297,77 +363,171 @@ def test_rejects_codeowners_without_matching_owners(self): codeowners.decoded_content = b""" /docs/ @docs-owner /python/** @python-owner - """ +""" repo.get_contents.return_value = codeowners with pytest.raises(RuntimeError, match="No maintainers found in CODEOWNERS"): get_maintainers(repo) + def test_propagates_codeowners_fetch_failure(self): + repo = MagicMock() + error = GithubException(404, {"message": "Not Found"}, None) + repo.get_contents.side_effect = error + + with pytest.raises(GithubException) as exc_info: + get_maintainers(repo) + + assert exc_info.value is error -# --------------------------------------------------------------------------- -# main – env var validation -# --------------------------------------------------------------------------- class TestMain: - @patch.dict(os.environ, { - "GITHUB_TOKEN": "tok", - "GITHUB_REPOSITORY": "org/repo", - "DAYS_THRESHOLD": "abc", - }, clear=True) - def test_invalid_days_threshold(self, capsys): + @patch.dict(os.environ, {}, clear=True) + def test_missing_github_token(self, capsys): + with pytest.raises(SystemExit): + main() + + assert "GITHUB_TOKEN" in capsys.readouterr().out + + @patch.dict(os.environ, {"GITHUB_TOKEN": "tok"}, clear=True) + def test_missing_github_repository(self, capsys): + with pytest.raises(SystemExit): + main() + + assert "GITHUB_REPOSITORY" in capsys.readouterr().out + + @patch.dict( + os.environ, + { + "GITHUB_TOKEN": "tok", + "GITHUB_REPOSITORY": "org/repo", + "DAYS_THRESHOLD": "invalid", + }, + clear=True, + ) + @patch("stale_issue_pr_ping.Github") + def test_invalid_days_threshold_stops_before_github(self, mock_github, capsys): with pytest.raises(SystemExit): main() - assert "numeric" in capsys.readouterr().out - @patch.dict(os.environ, { - "GITHUB_TOKEN": "tok", - "GITHUB_REPOSITORY": "org/repo", - }, clear=True) - @patch("stale_issue_pr_ping.get_maintainers", return_value=TEAM) + mock_github.assert_not_called() + assert "non-negative integer" in capsys.readouterr().out + + @patch.dict( + os.environ, + { + "GITHUB_TOKEN": "tok", + "GITHUB_REPOSITORY": "org/repo", + "DAYS_THRESHOLD": "7", + }, + clear=True, + ) + @patch("stale_issue_pr_ping.datetime") + @patch("stale_issue_pr_ping.get_maintainers", return_value=MAINTAINERS) @patch("stale_issue_pr_ping.Github") - def test_scans_all_open_items_and_defers_newly_marked( + def test_scans_only_open_prs_and_defers_new_warning( self, mock_github, mock_get_maintainers, + mock_datetime, capsys, ): - issue = _make_issue( - labels=[], - comments=[_make_comment("alice", NOW)], + pull_request = _make_pull_request( + comments=[_make_comment("alice")], + updated_at=NOW - timedelta(days=7), ) repo = mock_github.return_value.get_repo.return_value - repo.get_issues.return_value = [issue] + repo.get_pulls.return_value = [pull_request] + mock_datetime.now.return_value = NOW main() mock_get_maintainers.assert_called_once_with(repo) - repo.get_issues.assert_called_once_with(state="open") - issue.add_to_labels.assert_called_once_with(TRIGGER_LABEL) - issue.create_comment.assert_not_called() - assert "marked 1, pinged 0" in capsys.readouterr().out - - @patch.dict(os.environ, { - "GITHUB_TOKEN": "tok", - "GITHUB_REPOSITORY": "org/repo", - }, clear=True) + repo.get_pulls.assert_called_once_with(state="open") + repo.get_issues.assert_not_called() + pull_request.create_issue_comment.assert_called_once_with( + WARNING_COMMENT.format( + author="external", + closing_days=7, + ) + ) + pull_request.add_to_labels.assert_called_once_with(REQUESTED_INFO_LABEL) + pull_request.edit.assert_not_called() + assert "Scanned 1 PRs, warned 1, closed 0" in capsys.readouterr().out + + @patch.dict( + os.environ, + { + "GITHUB_TOKEN": "tok", + "GITHUB_REPOSITORY": "org/repo", + "DAYS_THRESHOLD": "7", + }, + clear=True, + ) @patch("stale_issue_pr_ping.datetime") - @patch("stale_issue_pr_ping.ping", return_value=True) - @patch("stale_issue_pr_ping.get_maintainers", return_value=TEAM) + @patch("stale_issue_pr_ping.get_maintainers", return_value=MAINTAINERS) @patch("stale_issue_pr_ping.Github") - def test_existing_waiting_item_continues_to_ping_check( + def test_existing_requested_info_pr_closes_before_warning_check( self, mock_github, mock_get_maintainers, - mock_ping, mock_datetime, + capsys, ): - issue = _make_issue(updated_at=NOW - timedelta(days=4)) + pull_request = _make_pull_request( + labels=[REQUESTED_INFO_LABEL], + updated_at=NOW - timedelta(days=7), + ) repo = mock_github.return_value.get_repo.return_value - repo.get_issues.return_value = [issue] + repo.get_pulls.return_value = [pull_request] mock_datetime.now.return_value = NOW main() mock_get_maintainers.assert_called_once_with(repo) - issue.get_comments.assert_not_called() - mock_ping.assert_called_once_with(issue, False) + pull_request.get_issue_comments.assert_not_called() + pull_request.create_issue_comment.assert_called_once_with(CLOSING_COMMENT) + pull_request.edit.assert_called_once_with(state="closed") + output = capsys.readouterr().out + assert "Scanned 1 PRs, warned 0, closed 1, failed 0" in output + assert "Closed: #42" in output + + @patch.dict( + os.environ, + { + "GITHUB_TOKEN": "tok", + "GITHUB_REPOSITORY": "org/repo", + }, + clear=True, + ) + @patch("stale_issue_pr_ping.datetime") + @patch("stale_issue_pr_ping.warn", return_value=False) + @patch("stale_issue_pr_ping.get_maintainers", return_value=MAINTAINERS) + @patch("stale_issue_pr_ping.Github") + def test_failed_warning_is_reported_and_exits( + self, + mock_github, + mock_get_maintainers, + mock_warn, + mock_datetime, + capsys, + ): + pull_request = _make_pull_request( + comments=[_make_comment("alice")], + updated_at=NOW - timedelta(days=WARNING_THRESHOLD_DAYS), + ) + repo = mock_github.return_value.get_repo.return_value + repo.get_pulls.return_value = [pull_request] + mock_datetime.now.return_value = NOW + + with pytest.raises(SystemExit): + main() + + mock_get_maintainers.assert_called_once_with(repo) + mock_warn.assert_called_once_with( + pull_request, + False, + CLOSING_THRESHOLD_DAYS, + ) + output = capsys.readouterr().out + assert "Scanned 1 PRs, warned 0, closed 0, failed 1" in output + assert "Failed: #42" in output diff --git a/.github/workflows/stale-issue-pr-ping.yml b/.github/workflows/stale-issue-pr-ping.yml index 8904c1cba68..a3ee0b654c7 100644 --- a/.github/workflows/stale-issue-pr-ping.yml +++ b/.github/workflows/stale-issue-pr-ping.yml @@ -1,4 +1,4 @@ -name: Stale issue and PR ping +name: Stale pull request lifecycle on: schedule: @@ -6,11 +6,11 @@ on: workflow_dispatch: inputs: days_threshold: - description: 'Days of silence before pinging the author' + description: 'Days of inactivity for both warning and closing periods' required: false default: '4' dry_run: - description: 'Log what would be pinged without taking action' + description: 'Log pull request warnings and closures without taking action' required: false default: 'false' type: choice @@ -23,8 +23,8 @@ concurrency: cancel-in-progress: true jobs: - ping_stale: - name: "Ping stale issues and PRs" + manage_stale: + name: "Manage stale pull requests" runs-on: ubuntu-latest environment: github-app-auth permissions: @@ -57,9 +57,9 @@ jobs: - name: Install dependencies run: pip install PyGithub==2.6.0 - - name: Run stale issue/PR ping + - name: Run stale pull request lifecycle run: python .github/scripts/stale_issue_pr_ping.py env: GITHUB_TOKEN: ${{ steps.github-auth.outputs.token }} DAYS_THRESHOLD: ${{ github.event.inputs.days_threshold || '4' }} - DRY_RUN: ${{ github.event.inputs.dry_run || 'false' }} + DRY_RUN: ${{ github.event.inputs.dry_run || 'true' }} From 22eff9b7dffea0de6d8b7b406ffbb83d802d31ef Mon Sep 17 00:00:00 2001 From: Jose Alvarez Date: Fri, 30 Oct 2026 10:45:17 +0100 Subject: [PATCH 05/11] Added dont-close label escape hatch --- .github/scripts/stale_issue_pr_ping.py | 8 +++++++- .github/tests/test_stale_issue_pr_ping.py | 19 +++++++++++++++++++ 2 files changed, 26 insertions(+), 1 deletion(-) diff --git a/.github/scripts/stale_issue_pr_ping.py b/.github/scripts/stale_issue_pr_ping.py index c724918206b..4cd14bb6716 100644 --- a/.github/scripts/stale_issue_pr_ping.py +++ b/.github/scripts/stale_issue_pr_ping.py @@ -4,7 +4,8 @@ After a CODEOWNER's latest conversation comment, warn and apply 'requested-info' after the warning threshold, then close after the closing -threshold without activity. CODEOWNER-authored pull requests are exempt. +threshold without activity. CODEOWNER-authored and 'dont-close'-labeled pull +requests are exempt. """ from __future__ import annotations @@ -20,6 +21,7 @@ REQUESTED_INFO_LABEL = "requested-info" +DONT_CLOSE_LABEL = "dont-close" WARNING_THRESHOLD_DAYS = 4 CLOSING_THRESHOLD_DAYS = 4 WARNING_COMMENT = ( @@ -72,6 +74,8 @@ def should_warn( """Return whether an external pull request is ready for its warning.""" if pull_request.user.login.lower() in maintainers: return False + if any(label.name == DONT_CLOSE_LABEL for label in pull_request.labels): + return False if any(label.name == REQUESTED_INFO_LABEL for label in pull_request.labels): return False days_inactive = ( @@ -100,6 +104,8 @@ def should_close( """Return whether a warned external pull request is ready to close.""" if pull_request.user.login.lower() in maintainers: return False + if any(label.name == DONT_CLOSE_LABEL for label in pull_request.labels): + return False if not any( label.name == REQUESTED_INFO_LABEL for label in pull_request.labels diff --git a/.github/tests/test_stale_issue_pr_ping.py b/.github/tests/test_stale_issue_pr_ping.py index 5771a905003..e395dcd6a6f 100644 --- a/.github/tests/test_stale_issue_pr_ping.py +++ b/.github/tests/test_stale_issue_pr_ping.py @@ -18,6 +18,7 @@ from stale_issue_pr_ping import ( CLOSING_COMMENT, CLOSING_THRESHOLD_DAYS, + DONT_CLOSE_LABEL, REQUESTED_INFO_LABEL, WARNING_COMMENT, WARNING_THRESHOLD_DAYS, @@ -144,6 +145,16 @@ def test_requested_info_pr_is_not_warned_again(self): assert should_warn(pull_request, MAINTAINERS, NOW, 4) is False pull_request.get_issue_comments.assert_not_called() + def test_dont_close_pr_is_not_warned(self): + pull_request = _make_pull_request( + labels=[DONT_CLOSE_LABEL], + comments=[_make_comment("alice")], + updated_at=NOW - timedelta(days=4), + ) + + assert should_warn(pull_request, MAINTAINERS, NOW, 4) is False + pull_request.get_issue_comments.assert_not_called() + def test_pr_without_regular_conversation_comments_is_skipped(self): pull_request = _make_pull_request( comments=[], @@ -280,6 +291,14 @@ def test_unlabeled_pr_is_not_closed(self): assert should_close(pull_request, MAINTAINERS, NOW, 4) is False + def test_dont_close_pr_is_not_closed(self): + pull_request = _make_pull_request( + labels=[REQUESTED_INFO_LABEL, DONT_CLOSE_LABEL], + updated_at=NOW - timedelta(days=4), + ) + + assert should_close(pull_request, MAINTAINERS, NOW, 4) is False + class TestClose: def test_dry_run_mutates_nothing(self, capsys): From bf4054c401158fab550bfefd102a4e416208a612 Mon Sep 17 00:00:00 2001 From: Jose Alvarez Date: Fri, 30 Oct 2026 11:21:29 +0100 Subject: [PATCH 06/11] workflow and scripts renamed --- ...tale_issue_pr_ping.py => stale_pr_ping.py} | 0 ...issue_pr_ping.py => test_stale_pr_ping.py} | 40 +++++++++---------- ...le-issue-pr-ping.yml => stale-pr-ping.yml} | 2 +- 3 files changed, 21 insertions(+), 21 deletions(-) rename .github/scripts/{stale_issue_pr_ping.py => stale_pr_ping.py} (100%) rename .github/tests/{test_stale_issue_pr_ping.py => test_stale_pr_ping.py} (94%) rename .github/workflows/{stale-issue-pr-ping.yml => stale-pr-ping.yml} (97%) diff --git a/.github/scripts/stale_issue_pr_ping.py b/.github/scripts/stale_pr_ping.py similarity index 100% rename from .github/scripts/stale_issue_pr_ping.py rename to .github/scripts/stale_pr_ping.py diff --git a/.github/tests/test_stale_issue_pr_ping.py b/.github/tests/test_stale_pr_ping.py similarity index 94% rename from .github/tests/test_stale_issue_pr_ping.py rename to .github/tests/test_stale_pr_ping.py index e395dcd6a6f..a22e3211cec 100644 --- a/.github/tests/test_stale_issue_pr_ping.py +++ b/.github/tests/test_stale_pr_ping.py @@ -1,6 +1,6 @@ # Copyright (c) Microsoft. All rights reserved. -"""Tests for stale_issue_pr_ping.py.""" +"""Tests for stale_pr_ping.py.""" from __future__ import annotations @@ -15,7 +15,7 @@ # Ensure the script directory is importable sys.path.insert(0, os.path.join(os.path.dirname(__file__), "..", "scripts")) -from stale_issue_pr_ping import ( +from stale_pr_ping import ( CLOSING_COMMENT, CLOSING_THRESHOLD_DAYS, DONT_CLOSE_LABEL, @@ -209,7 +209,7 @@ def test_posts_warning_then_applies_requested_info(self): call.add_to_labels(REQUESTED_INFO_LABEL), ] - @patch("stale_issue_pr_ping.time.sleep") + @patch("stale_pr_ping.time.sleep") def test_comment_retry_succeeds(self, mock_sleep): pull_request = _make_pull_request() pull_request.create_issue_comment.side_effect = [ @@ -222,7 +222,7 @@ def test_comment_retry_succeeds(self, mock_sleep): pull_request.add_to_labels.assert_called_once_with(REQUESTED_INFO_LABEL) mock_sleep.assert_called_once() - @patch("stale_issue_pr_ping.time.sleep") + @patch("stale_pr_ping.time.sleep") def test_label_retry_does_not_repeat_warning(self, mock_sleep): pull_request = _make_pull_request() pull_request.add_to_labels.side_effect = [ @@ -235,7 +235,7 @@ def test_label_retry_does_not_repeat_warning(self, mock_sleep): assert pull_request.add_to_labels.call_count == 2 mock_sleep.assert_called_once() - @patch("stale_issue_pr_ping.time.sleep") + @patch("stale_pr_ping.time.sleep") def test_warning_failure_never_applies_label(self, mock_sleep): pull_request = _make_pull_request() pull_request.create_issue_comment.side_effect = Exception("comment error") @@ -244,7 +244,7 @@ def test_warning_failure_never_applies_label(self, mock_sleep): assert pull_request.create_issue_comment.call_count == 3 pull_request.add_to_labels.assert_not_called() - @patch("stale_issue_pr_ping.time.sleep") + @patch("stale_pr_ping.time.sleep") def test_final_label_failure_reports_failure_without_repeating_warning( self, mock_sleep, @@ -318,7 +318,7 @@ def test_posts_closing_comment_then_closes_pr(self): call.edit(state="closed"), ] - @patch("stale_issue_pr_ping.time.sleep") + @patch("stale_pr_ping.time.sleep") def test_close_retry_does_not_repeat_comment(self, mock_sleep): pull_request = _make_pull_request() pull_request.edit.side_effect = [Exception("close error"), None] @@ -328,7 +328,7 @@ def test_close_retry_does_not_repeat_comment(self, mock_sleep): assert pull_request.edit.call_count == 2 mock_sleep.assert_called_once() - @patch("stale_issue_pr_ping.time.sleep") + @patch("stale_pr_ping.time.sleep") def test_comment_failure_never_closes_pr(self, mock_sleep): pull_request = _make_pull_request() pull_request.create_issue_comment.side_effect = Exception("comment error") @@ -337,7 +337,7 @@ def test_comment_failure_never_closes_pr(self, mock_sleep): assert pull_request.create_issue_comment.call_count == 3 pull_request.edit.assert_not_called() - @patch("stale_issue_pr_ping.time.sleep") + @patch("stale_pr_ping.time.sleep") def test_final_close_failure_reports_failure_without_repeating_comment( self, mock_sleep, @@ -423,7 +423,7 @@ def test_missing_github_repository(self, capsys): }, clear=True, ) - @patch("stale_issue_pr_ping.Github") + @patch("stale_pr_ping.Github") def test_invalid_days_threshold_stops_before_github(self, mock_github, capsys): with pytest.raises(SystemExit): main() @@ -440,9 +440,9 @@ def test_invalid_days_threshold_stops_before_github(self, mock_github, capsys): }, clear=True, ) - @patch("stale_issue_pr_ping.datetime") - @patch("stale_issue_pr_ping.get_maintainers", return_value=MAINTAINERS) - @patch("stale_issue_pr_ping.Github") + @patch("stale_pr_ping.datetime") + @patch("stale_pr_ping.get_maintainers", return_value=MAINTAINERS) + @patch("stale_pr_ping.Github") def test_scans_only_open_prs_and_defers_new_warning( self, mock_github, @@ -482,9 +482,9 @@ def test_scans_only_open_prs_and_defers_new_warning( }, clear=True, ) - @patch("stale_issue_pr_ping.datetime") - @patch("stale_issue_pr_ping.get_maintainers", return_value=MAINTAINERS) - @patch("stale_issue_pr_ping.Github") + @patch("stale_pr_ping.datetime") + @patch("stale_pr_ping.get_maintainers", return_value=MAINTAINERS) + @patch("stale_pr_ping.Github") def test_existing_requested_info_pr_closes_before_warning_check( self, mock_github, @@ -518,10 +518,10 @@ def test_existing_requested_info_pr_closes_before_warning_check( }, clear=True, ) - @patch("stale_issue_pr_ping.datetime") - @patch("stale_issue_pr_ping.warn", return_value=False) - @patch("stale_issue_pr_ping.get_maintainers", return_value=MAINTAINERS) - @patch("stale_issue_pr_ping.Github") + @patch("stale_pr_ping.datetime") + @patch("stale_pr_ping.warn", return_value=False) + @patch("stale_pr_ping.get_maintainers", return_value=MAINTAINERS) + @patch("stale_pr_ping.Github") def test_failed_warning_is_reported_and_exits( self, mock_github, diff --git a/.github/workflows/stale-issue-pr-ping.yml b/.github/workflows/stale-pr-ping.yml similarity index 97% rename from .github/workflows/stale-issue-pr-ping.yml rename to .github/workflows/stale-pr-ping.yml index a3ee0b654c7..6afe760f53b 100644 --- a/.github/workflows/stale-issue-pr-ping.yml +++ b/.github/workflows/stale-pr-ping.yml @@ -58,7 +58,7 @@ jobs: run: pip install PyGithub==2.6.0 - name: Run stale pull request lifecycle - run: python .github/scripts/stale_issue_pr_ping.py + run: python .github/scripts/stale_pr_ping.py env: GITHUB_TOKEN: ${{ steps.github-auth.outputs.token }} DAYS_THRESHOLD: ${{ github.event.inputs.days_threshold || '4' }} From 25249828df46aa4f9b871c9a374b30239efcbf24 Mon Sep 17 00:00:00 2001 From: Jose Alvarez Date: Fri, 30 Oct 2026 12:15:14 +0100 Subject: [PATCH 07/11] Reverted workflow dry_run default and .gitignore entry --- .github/workflows/stale-pr-ping.yml | 2 +- .gitignore | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/stale-pr-ping.yml b/.github/workflows/stale-pr-ping.yml index 6afe760f53b..b9257546ad1 100644 --- a/.github/workflows/stale-pr-ping.yml +++ b/.github/workflows/stale-pr-ping.yml @@ -62,4 +62,4 @@ jobs: env: GITHUB_TOKEN: ${{ steps.github-auth.outputs.token }} DAYS_THRESHOLD: ${{ github.event.inputs.days_threshold || '4' }} - DRY_RUN: ${{ github.event.inputs.dry_run || 'true' }} + DRY_RUN: ${{ github.event.inputs.dry_run || 'false' }} diff --git a/.gitignore b/.gitignore index 802ec0c0bf7..34bf943e24a 100644 --- a/.gitignore +++ b/.gitignore @@ -135,7 +135,7 @@ celerybeat.pid .env .venv env/ -**/.venv*/ +venv/ # Foundry agent CLI (contains secrets, auto-generated) .foundry-agent.json From e402c58f18918c88c477eca9f1f1912239a5b124 Mon Sep 17 00:00:00 2001 From: Jose Alvarez Date: Fri, 30 Oct 2026 13:39:54 +0100 Subject: [PATCH 08/11] Recover stale PR lifecycle state --- .github/scripts/stale_pr_ping.py | 117 ++++++++- .github/tests/test_stale_pr_ping.py | 358 +++++++++++++++++++++++++--- 2 files changed, 425 insertions(+), 50 deletions(-) diff --git a/.github/scripts/stale_pr_ping.py b/.github/scripts/stale_pr_ping.py index 4cd14bb6716..d84a928c8a3 100644 --- a/.github/scripts/stale_pr_ping.py +++ b/.github/scripts/stale_pr_ping.py @@ -16,18 +16,21 @@ from datetime import datetime, timezone from github import Auth, Github +from github.IssueComment import IssueComment from github.PullRequest import PullRequest from github.Repository import Repository REQUESTED_INFO_LABEL = "requested-info" DONT_CLOSE_LABEL = "dont-close" +WARNING_MARKER = "" WARNING_THRESHOLD_DAYS = 4 CLOSING_THRESHOLD_DAYS = 4 WARNING_COMMENT = ( "@{author}, this pull request is waiting on your response. " "If there is no further activity within {closing_days} days, " "it will be closed automatically. (This is an automated message.)" + f"\n\n{WARNING_MARKER}" ) CLOSING_COMMENT = ( "Closing this pull request because there was no further activity " @@ -65,16 +68,33 @@ def get_maintainers(repo: Repository) -> set[str]: return owners +def get_latest_comment(pull_request: PullRequest) -> IssueComment | None: + """Fetch only the newest page of regular PR conversation comments.""" + return next(iter(pull_request.get_issue_comments().reversed), None) + + +def has_warning_marker(comment: IssueComment | None) -> bool: + """Return whether a comment is the automated warning.""" + return bool(comment and WARNING_MARKER in (comment.body or "")) + + +def is_exempt(pull_request: PullRequest, maintainers: set[str]) -> bool: + """Return whether a pull request is exempt from the lifecycle.""" + return ( + pull_request.user.login.lower() in maintainers + or any(label.name == DONT_CLOSE_LABEL for label in pull_request.labels) + ) + + def should_warn( pull_request: PullRequest, + latest_comment: IssueComment | None, maintainers: set[str], now: datetime, warning_threshold_days: int, ) -> bool: """Return whether an external pull request is ready for its warning.""" - if pull_request.user.login.lower() in maintainers: - return False - if any(label.name == DONT_CLOSE_LABEL for label in pull_request.labels): + if is_exempt(pull_request, maintainers): return False if any(label.name == REQUESTED_INFO_LABEL for label in pull_request.labels): return False @@ -84,33 +104,30 @@ def should_warn( if days_inactive < warning_threshold_days: return False - comments = list(pull_request.get_issue_comments()) - if not comments: - return False - - latest_comment = comments[-1] return bool( - latest_comment.user + latest_comment + and latest_comment.user and latest_comment.user.login.lower() in maintainers ) def should_close( pull_request: PullRequest, + latest_comment: IssueComment | None, maintainers: set[str], now: datetime, closing_threshold_days: int, ) -> bool: """Return whether a warned external pull request is ready to close.""" - if pull_request.user.login.lower() in maintainers: - return False - if any(label.name == DONT_CLOSE_LABEL for label in pull_request.labels): + if is_exempt(pull_request, maintainers): return False if not any( label.name == REQUESTED_INFO_LABEL for label in pull_request.labels ): return False + if not has_warning_marker(latest_comment): + return False days_inactive = ( now - pull_request.updated_at.astimezone(timezone.utc) @@ -118,6 +135,56 @@ def should_close( return days_inactive >= closing_threshold_days +def clear_requested_info(pull_request: PullRequest, dry_run: bool) -> bool: + """Remove stale lifecycle state after newer conversation activity.""" + if dry_run: + print( + f" [DRY RUN] Would remove '{REQUESTED_INFO_LABEL}' " + f"from PR #{pull_request.number}" + ) + return True + + try: + pull_request.remove_from_labels(REQUESTED_INFO_LABEL) + except Exception as exc: + print( + f" ERROR: Failed to remove '{REQUESTED_INFO_LABEL}' " + f"from PR #{pull_request.number}: {exc}" + ) + return False + + print( + f" Removed '{REQUESTED_INFO_LABEL}' from PR #{pull_request.number} " + "after newer conversation activity" + ) + return True + + +def restore_requested_info(pull_request: PullRequest, dry_run: bool) -> bool: + """Restore lifecycle state when the warning comment already exists.""" + if dry_run: + print( + f" [DRY RUN] Would restore '{REQUESTED_INFO_LABEL}' " + f"on PR #{pull_request.number}" + ) + return True + + try: + pull_request.add_to_labels(REQUESTED_INFO_LABEL) + except Exception as exc: + print( + f" ERROR: Failed to restore '{REQUESTED_INFO_LABEL}' " + f"on PR #{pull_request.number}: {exc}" + ) + return False + + print( + f" Restored '{REQUESTED_INFO_LABEL}' on PR #{pull_request.number} " + "from the warning marker" + ) + return True + + def warn( pull_request: PullRequest, dry_run: bool, @@ -269,11 +336,34 @@ def main() -> None: f"closing: {closing_threshold_days} days)...\n" ) - for pull_request in repo.get_pulls(state="open"): + pull_requests = list(repo.get_pulls(state="open")) + + for pull_request in pull_requests: scanned += 1 + if is_exempt(pull_request, maintainers): + continue + + latest_comment = get_latest_comment(pull_request) + has_requested_info = any( + label.name == REQUESTED_INFO_LABEL + for label in pull_request.labels + ) + has_marker = has_warning_marker(latest_comment) + + if has_requested_info and not has_marker: + if not clear_requested_info(pull_request, dry_run): + failed.append(pull_request.number) + continue + + if has_marker and not has_requested_info: + if not restore_requested_info(pull_request, dry_run): + failed.append(pull_request.number) + continue + if should_close( pull_request, + latest_comment, maintainers, now, closing_threshold_days, @@ -286,6 +376,7 @@ def main() -> None: if should_warn( pull_request, + latest_comment, maintainers, now, warning_threshold_days, diff --git a/.github/tests/test_stale_pr_ping.py b/.github/tests/test_stale_pr_ping.py index a22e3211cec..721ac1c25cc 100644 --- a/.github/tests/test_stale_pr_ping.py +++ b/.github/tests/test_stale_pr_ping.py @@ -21,11 +21,16 @@ DONT_CLOSE_LABEL, REQUESTED_INFO_LABEL, WARNING_COMMENT, + WARNING_MARKER, WARNING_THRESHOLD_DAYS, + clear_requested_info, close, + get_latest_comment, get_maintainers, get_runtime_thresholds, + has_warning_marker, main, + restore_requested_info, should_close, should_warn, warn, @@ -35,13 +40,14 @@ NOW = datetime(2026, 3, 15, 12, 0, 0, tzinfo=timezone.utc) -def _make_comment(login: str | None) -> MagicMock: +def _make_comment(login: str | None, body: str = "") -> MagicMock: comment = MagicMock() if login is None: comment.user = None else: comment.user = MagicMock() comment.user.login = login + comment.body = body return comment @@ -67,10 +73,46 @@ def _make_pull_request( for name in (labels or []) ] pull_request.updated_at = updated_at - pull_request.get_issue_comments.return_value = comments or [] + issue_comments = MagicMock() + issue_comments.reversed = iter(reversed(comments or [])) + pull_request.get_issue_comments.return_value = issue_comments return pull_request +def _make_warning_comment() -> MagicMock: + return _make_comment("github-actions[bot]", WARNING_MARKER) + + +class TestLatestComment: + def test_fetches_only_newest_reversed_comment_page(self): + older_comment = _make_comment("external") + latest_comment = _make_comment("alice") + pull_request = _make_pull_request( + comments=[older_comment, latest_comment], + ) + issue_comments = pull_request.get_issue_comments.return_value + issue_comments.__iter__.side_effect = AssertionError( + "forward history should not be fetched" + ) + + assert get_latest_comment(pull_request) is latest_comment + pull_request.get_issue_comments.assert_called_once_with() + + def test_returns_none_without_comments(self): + pull_request = _make_pull_request() + + assert get_latest_comment(pull_request) is None + + +class TestWarningMarker: + def test_detects_warning_marker(self): + assert has_warning_marker(_make_warning_comment()) is True + + def test_rejects_regular_or_missing_comment(self): + assert has_warning_marker(_make_comment("alice", "regular comment")) is False + assert has_warning_marker(None) is False + + class TestPolicyThresholds: def test_default_constants(self): assert WARNING_THRESHOLD_DAYS == 4 @@ -105,83 +147,118 @@ def test_negative_runtime_override_is_rejected(self, capsys): class TestShouldWarn: def test_warns_at_threshold_for_latest_maintainer_comment(self): + latest_comment = _make_comment("alice") pull_request = _make_pull_request( - comments=[ - _make_comment("external"), - _make_comment("alice"), - ], updated_at=NOW - timedelta(days=4), ) - assert should_warn(pull_request, MAINTAINERS, NOW, 4) is True - pull_request.get_issue_comments.assert_called_once_with() + assert should_warn( + pull_request, + latest_comment, + MAINTAINERS, + NOW, + 4, + ) is True def test_recent_activity_resets_warning_clock(self): + latest_comment = _make_comment("alice") pull_request = _make_pull_request( - comments=[_make_comment("alice")], updated_at=NOW - timedelta(days=3), ) - assert should_warn(pull_request, MAINTAINERS, NOW, 4) is False - pull_request.get_issue_comments.assert_not_called() + assert should_warn( + pull_request, + latest_comment, + MAINTAINERS, + NOW, + 4, + ) is False def test_maintainer_authored_pr_is_exempt(self): + latest_comment = _make_comment("bob") pull_request = _make_pull_request( author="Alice", - comments=[_make_comment("bob")], updated_at=NOW - timedelta(days=4), ) - assert should_warn(pull_request, MAINTAINERS, NOW, 4) is False - pull_request.get_issue_comments.assert_not_called() + assert should_warn( + pull_request, + latest_comment, + MAINTAINERS, + NOW, + 4, + ) is False def test_requested_info_pr_is_not_warned_again(self): + latest_comment = _make_comment("alice") pull_request = _make_pull_request( labels=[REQUESTED_INFO_LABEL], - comments=[_make_comment("alice")], updated_at=NOW - timedelta(days=4), ) - assert should_warn(pull_request, MAINTAINERS, NOW, 4) is False - pull_request.get_issue_comments.assert_not_called() + assert should_warn( + pull_request, + latest_comment, + MAINTAINERS, + NOW, + 4, + ) is False def test_dont_close_pr_is_not_warned(self): + latest_comment = _make_comment("alice") pull_request = _make_pull_request( labels=[DONT_CLOSE_LABEL], - comments=[_make_comment("alice")], updated_at=NOW - timedelta(days=4), ) - assert should_warn(pull_request, MAINTAINERS, NOW, 4) is False - pull_request.get_issue_comments.assert_not_called() + assert should_warn( + pull_request, + latest_comment, + MAINTAINERS, + NOW, + 4, + ) is False def test_pr_without_regular_conversation_comments_is_skipped(self): pull_request = _make_pull_request( - comments=[], updated_at=NOW - timedelta(days=4), ) - assert should_warn(pull_request, MAINTAINERS, NOW, 4) is False - pull_request.get_issue_comments.assert_called_once_with() + assert should_warn( + pull_request, + None, + MAINTAINERS, + NOW, + 4, + ) is False def test_latest_external_comment_is_skipped(self): + latest_comment = _make_comment("external") pull_request = _make_pull_request( - comments=[ - _make_comment("alice"), - _make_comment("external"), - ], updated_at=NOW - timedelta(days=4), ) - assert should_warn(pull_request, MAINTAINERS, NOW, 4) is False + assert should_warn( + pull_request, + latest_comment, + MAINTAINERS, + NOW, + 4, + ) is False def test_deleted_latest_comment_author_is_skipped(self): + latest_comment = _make_comment(None) pull_request = _make_pull_request( - comments=[_make_comment(None)], updated_at=NOW - timedelta(days=4), ) - assert should_warn(pull_request, MAINTAINERS, NOW, 4) is False + assert should_warn( + pull_request, + latest_comment, + MAINTAINERS, + NOW, + 4, + ) is False class TestWarn: @@ -265,7 +342,13 @@ def test_closes_requested_info_pr_at_threshold(self): updated_at=NOW - timedelta(days=4), ) - assert should_close(pull_request, MAINTAINERS, NOW, 4) is True + assert should_close( + pull_request, + _make_warning_comment(), + MAINTAINERS, + NOW, + 4, + ) is True def test_recent_activity_resets_closing_clock(self): pull_request = _make_pull_request( @@ -273,7 +356,13 @@ def test_recent_activity_resets_closing_clock(self): updated_at=NOW - timedelta(days=3), ) - assert should_close(pull_request, MAINTAINERS, NOW, 4) is False + assert should_close( + pull_request, + _make_warning_comment(), + MAINTAINERS, + NOW, + 4, + ) is False def test_maintainer_authored_pr_is_exempt(self): pull_request = _make_pull_request( @@ -282,14 +371,26 @@ def test_maintainer_authored_pr_is_exempt(self): updated_at=NOW - timedelta(days=4), ) - assert should_close(pull_request, MAINTAINERS, NOW, 4) is False + assert should_close( + pull_request, + _make_warning_comment(), + MAINTAINERS, + NOW, + 4, + ) is False def test_unlabeled_pr_is_not_closed(self): pull_request = _make_pull_request( updated_at=NOW - timedelta(days=4), ) - assert should_close(pull_request, MAINTAINERS, NOW, 4) is False + assert should_close( + pull_request, + _make_warning_comment(), + MAINTAINERS, + NOW, + 4, + ) is False def test_dont_close_pr_is_not_closed(self): pull_request = _make_pull_request( @@ -297,7 +398,72 @@ def test_dont_close_pr_is_not_closed(self): updated_at=NOW - timedelta(days=4), ) - assert should_close(pull_request, MAINTAINERS, NOW, 4) is False + assert should_close( + pull_request, + _make_warning_comment(), + MAINTAINERS, + NOW, + 4, + ) is False + + def test_manual_requested_info_without_marker_is_not_closed(self): + pull_request = _make_pull_request( + labels=[REQUESTED_INFO_LABEL], + updated_at=NOW - timedelta(days=4), + ) + + assert should_close( + pull_request, + _make_comment("alice", "manual comment"), + MAINTAINERS, + NOW, + 4, + ) is False + + +class TestRequestedInfoRecovery: + def test_clear_requested_info(self): + pull_request = _make_pull_request(labels=[REQUESTED_INFO_LABEL]) + + assert clear_requested_info(pull_request, dry_run=False) is True + pull_request.remove_from_labels.assert_called_once_with( + REQUESTED_INFO_LABEL + ) + + def test_clear_requested_info_dry_run_mutates_nothing(self, capsys): + pull_request = _make_pull_request(labels=[REQUESTED_INFO_LABEL]) + + assert clear_requested_info(pull_request, dry_run=True) is True + pull_request.remove_from_labels.assert_not_called() + assert "DRY RUN" in capsys.readouterr().out + + def test_clear_requested_info_failure_is_explicit(self, capsys): + pull_request = _make_pull_request(labels=[REQUESTED_INFO_LABEL]) + pull_request.remove_from_labels.side_effect = Exception("remove error") + + assert clear_requested_info(pull_request, dry_run=False) is False + assert "ERROR" in capsys.readouterr().out + + def test_restore_requested_info_without_reposting_warning(self): + pull_request = _make_pull_request() + + assert restore_requested_info(pull_request, dry_run=False) is True + pull_request.add_to_labels.assert_called_once_with(REQUESTED_INFO_LABEL) + pull_request.create_issue_comment.assert_not_called() + + def test_restore_requested_info_dry_run_mutates_nothing(self, capsys): + pull_request = _make_pull_request() + + assert restore_requested_info(pull_request, dry_run=True) is True + pull_request.add_to_labels.assert_not_called() + assert "DRY RUN" in capsys.readouterr().out + + def test_restore_requested_info_failure_is_explicit(self, capsys): + pull_request = _make_pull_request() + pull_request.add_to_labels.side_effect = Exception("label error") + + assert restore_requested_info(pull_request, dry_run=False) is False + assert "ERROR" in capsys.readouterr().out class TestClose: @@ -494,22 +660,140 @@ def test_existing_requested_info_pr_closes_before_warning_check( ): pull_request = _make_pull_request( labels=[REQUESTED_INFO_LABEL], + comments=[_make_warning_comment()], updated_at=NOW - timedelta(days=7), ) repo = mock_github.return_value.get_repo.return_value - repo.get_pulls.return_value = [pull_request] + materialized = {"complete": False} + + def pull_requests(): + yield pull_request + materialized["complete"] = True + + def assert_materialized(**kwargs): + assert materialized["complete"] is True + assert kwargs == {"state": "closed"} + + repo.get_pulls.return_value = pull_requests() + pull_request.edit.side_effect = assert_materialized mock_datetime.now.return_value = NOW main() mock_get_maintainers.assert_called_once_with(repo) - pull_request.get_issue_comments.assert_not_called() + pull_request.get_issue_comments.assert_called_once_with() pull_request.create_issue_comment.assert_called_once_with(CLOSING_COMMENT) pull_request.edit.assert_called_once_with(state="closed") output = capsys.readouterr().out assert "Scanned 1 PRs, warned 0, closed 1, failed 0" in output assert "Closed: #42" in output + @patch.dict( + os.environ, + { + "GITHUB_TOKEN": "tok", + "GITHUB_REPOSITORY": "org/repo", + }, + clear=True, + ) + @patch("stale_pr_ping.datetime") + @patch("stale_pr_ping.get_maintainers", return_value=MAINTAINERS) + @patch("stale_pr_ping.Github") + def test_manual_requested_info_is_cleared_instead_of_closed( + self, + mock_github, + mock_get_maintainers, + mock_datetime, + ): + pull_request = _make_pull_request( + labels=[REQUESTED_INFO_LABEL], + comments=[_make_comment("external", "author update")], + updated_at=NOW - timedelta(days=CLOSING_THRESHOLD_DAYS), + ) + repo = mock_github.return_value.get_repo.return_value + repo.get_pulls.return_value = [pull_request] + mock_datetime.now.return_value = NOW + + main() + + mock_get_maintainers.assert_called_once_with(repo) + pull_request.remove_from_labels.assert_called_once_with( + REQUESTED_INFO_LABEL + ) + pull_request.create_issue_comment.assert_not_called() + pull_request.edit.assert_not_called() + + @patch.dict( + os.environ, + { + "GITHUB_TOKEN": "tok", + "GITHUB_REPOSITORY": "org/repo", + }, + clear=True, + ) + @patch("stale_pr_ping.datetime") + @patch("stale_pr_ping.get_maintainers", return_value=MAINTAINERS) + @patch("stale_pr_ping.Github") + def test_warning_marker_restores_missing_label_without_reposting( + self, + mock_github, + mock_get_maintainers, + mock_datetime, + ): + pull_request = _make_pull_request( + comments=[_make_warning_comment()], + updated_at=NOW - timedelta(days=CLOSING_THRESHOLD_DAYS), + ) + repo = mock_github.return_value.get_repo.return_value + repo.get_pulls.return_value = [pull_request] + mock_datetime.now.return_value = NOW + + main() + + mock_get_maintainers.assert_called_once_with(repo) + pull_request.add_to_labels.assert_called_once_with(REQUESTED_INFO_LABEL) + pull_request.create_issue_comment.assert_not_called() + pull_request.edit.assert_not_called() + + @patch.dict( + os.environ, + { + "GITHUB_TOKEN": "tok", + "GITHUB_REPOSITORY": "org/repo", + "DRY_RUN": "true", + }, + clear=True, + ) + @patch("stale_pr_ping.datetime") + @patch("stale_pr_ping.get_maintainers", return_value=MAINTAINERS) + @patch("stale_pr_ping.Github") + def test_recovery_dry_run_mutates_nothing( + self, + mock_github, + mock_get_maintainers, + mock_datetime, + ): + manual_label = _make_pull_request( + labels=[REQUESTED_INFO_LABEL], + comments=[_make_comment("external", "author update")], + number=41, + ) + missing_label = _make_pull_request( + comments=[_make_warning_comment()], + number=42, + ) + repo = mock_github.return_value.get_repo.return_value + repo.get_pulls.return_value = [manual_label, missing_label] + mock_datetime.now.return_value = NOW + + main() + + mock_get_maintainers.assert_called_once_with(repo) + manual_label.remove_from_labels.assert_not_called() + missing_label.add_to_labels.assert_not_called() + manual_label.edit.assert_not_called() + missing_label.edit.assert_not_called() + @patch.dict( os.environ, { From b01970f75a02b8af9ced8c4da36430a028e44ea1 Mon Sep 17 00:00:00 2001 From: Jose Alvarez Date: Fri, 30 Oct 2026 14:36:28 +0100 Subject: [PATCH 09/11] Isolate stale PR comment lookup failures --- .github/scripts/stale_pr_ping.py | 37 +++++++++- .github/tests/test_stale_pr_ping.py | 102 ++++++++++++++++++++++++++++ 2 files changed, 137 insertions(+), 2 deletions(-) diff --git a/.github/scripts/stale_pr_ping.py b/.github/scripts/stale_pr_ping.py index d84a928c8a3..af1485819e9 100644 --- a/.github/scripts/stale_pr_ping.py +++ b/.github/scripts/stale_pr_ping.py @@ -15,7 +15,7 @@ import time from datetime import datetime, timezone -from github import Auth, Github +from github import Auth, Github, GithubException from github.IssueComment import IssueComment from github.PullRequest import PullRequest from github.Repository import Repository @@ -73,6 +73,33 @@ def get_latest_comment(pull_request: PullRequest) -> IssueComment | None: return next(iter(pull_request.get_issue_comments().reversed), None) +def get_latest_comment_with_retry( + pull_request: PullRequest, +) -> tuple[bool, IssueComment | None]: + """Fetch the latest comment, retrying transient GitHub API failures.""" + max_retries = 3 + for attempt in range(1, max_retries + 1): + try: + return True, get_latest_comment(pull_request) + except GithubException as exc: + if attempt < max_retries: + wait = 2 ** attempt # 2s, 4s + print( + f" WARN: Latest-comment lookup attempt " + f"{attempt}/{max_retries} failed for " + f"PR #{pull_request.number}: {exc}. " + f"Retrying in {wait}s..." + ) + time.sleep(wait) + else: + print( + f" ERROR: Failed to fetch the latest comment for " + f"PR #{pull_request.number} after {max_retries} " + f"attempts: {exc}" + ) + return False, None + + def has_warning_marker(comment: IssueComment | None) -> bool: """Return whether a comment is the automated warning.""" return bool(comment and WARNING_MARKER in (comment.body or "")) @@ -344,7 +371,13 @@ def main() -> None: if is_exempt(pull_request, maintainers): continue - latest_comment = get_latest_comment(pull_request) + lookup_succeeded, latest_comment = get_latest_comment_with_retry( + pull_request + ) + if not lookup_succeeded: + failed.append(pull_request.number) + continue + has_requested_info = any( label.name == REQUESTED_INFO_LABEL for label in pull_request.labels diff --git a/.github/tests/test_stale_pr_ping.py b/.github/tests/test_stale_pr_ping.py index 721ac1c25cc..09bc5c78c3f 100644 --- a/.github/tests/test_stale_pr_ping.py +++ b/.github/tests/test_stale_pr_ping.py @@ -26,6 +26,7 @@ clear_requested_info, close, get_latest_comment, + get_latest_comment_with_retry, get_maintainers, get_runtime_thresholds, has_warning_marker, @@ -103,6 +104,49 @@ def test_returns_none_without_comments(self): assert get_latest_comment(pull_request) is None + @patch("stale_pr_ping.time.sleep") + def test_transient_lookup_failure_succeeds_on_retry( + self, + mock_sleep, + capsys, + ): + latest_comment = _make_comment("alice") + pull_request = _make_pull_request(comments=[latest_comment]) + issue_comments = pull_request.get_issue_comments.return_value + pull_request.get_issue_comments.side_effect = [ + GithubException(502, {"message": "Bad Gateway"}, None), + issue_comments, + ] + + assert get_latest_comment_with_retry(pull_request) == ( + True, + latest_comment, + ) + assert pull_request.get_issue_comments.call_count == 2 + mock_sleep.assert_called_once_with(2) + assert "lookup attempt 1/3 failed for PR #42" in capsys.readouterr().out + + @patch("stale_pr_ping.time.sleep") + def test_final_lookup_failure_is_reported( + self, + mock_sleep, + capsys, + ): + pull_request = _make_pull_request() + pull_request.get_issue_comments.side_effect = GithubException( + 502, + {"message": "Bad Gateway"}, + None, + ) + + assert get_latest_comment_with_retry(pull_request) == (False, None) + assert pull_request.get_issue_comments.call_count == 3 + assert mock_sleep.call_args_list == [call(2), call(4)] + assert ( + "Failed to fetch the latest comment for PR #42 after 3 attempts" + in capsys.readouterr().out + ) + class TestWarningMarker: def test_detects_warning_marker(self): @@ -834,3 +878,61 @@ def test_failed_warning_is_reported_and_exits( output = capsys.readouterr().out assert "Scanned 1 PRs, warned 0, closed 0, failed 1" in output assert "Failed: #42" in output + + @patch.dict( + os.environ, + { + "GITHUB_TOKEN": "tok", + "GITHUB_REPOSITORY": "org/repo", + }, + clear=True, + ) + @patch("stale_pr_ping.time.sleep") + @patch("stale_pr_ping.datetime") + @patch("stale_pr_ping.get_maintainers", return_value=MAINTAINERS) + @patch("stale_pr_ping.Github") + def test_lookup_failure_is_recorded_and_later_pr_is_processed( + self, + mock_github, + mock_get_maintainers, + mock_datetime, + mock_sleep, + capsys, + ): + failed_pull_request = _make_pull_request(number=41) + failed_pull_request.get_issue_comments.side_effect = GithubException( + 502, + {"message": "Bad Gateway"}, + None, + ) + later_pull_request = _make_pull_request( + comments=[_make_comment("alice")], + number=42, + updated_at=NOW - timedelta(days=WARNING_THRESHOLD_DAYS), + ) + repo = mock_github.return_value.get_repo.return_value + repo.get_pulls.return_value = [ + failed_pull_request, + later_pull_request, + ] + mock_datetime.now.return_value = NOW + + with pytest.raises(SystemExit): + main() + + mock_get_maintainers.assert_called_once_with(repo) + assert failed_pull_request.get_issue_comments.call_count == 3 + assert mock_sleep.call_args_list == [call(2), call(4)] + later_pull_request.create_issue_comment.assert_called_once_with( + WARNING_COMMENT.format( + author="external", + closing_days=CLOSING_THRESHOLD_DAYS, + ) + ) + later_pull_request.add_to_labels.assert_called_once_with( + REQUESTED_INFO_LABEL + ) + output = capsys.readouterr().out + assert "Scanned 2 PRs, warned 1, closed 0, failed 1" in output + assert "Warned: #42" in output + assert "Failed: #41" in output From 157d0892e6ec4e56392f50cf27a497c335fbeee0 Mon Sep 17 00:00:00 2001 From: Jose Alvarez Date: Tue, 3 Nov 2026 11:20:15 +0100 Subject: [PATCH 10/11] Use GitHub member association for stale PRs --- .github/scripts/stale_pr_ping.py | 63 ++------- .github/tests/test_stale_pr_ping.py | 212 +++++++++++++++------------- 2 files changed, 121 insertions(+), 154 deletions(-) diff --git a/.github/scripts/stale_pr_ping.py b/.github/scripts/stale_pr_ping.py index af1485819e9..032a332d033 100644 --- a/.github/scripts/stale_pr_ping.py +++ b/.github/scripts/stale_pr_ping.py @@ -2,10 +2,10 @@ """Warn and close stale external-authored pull requests. -After a CODEOWNER's latest conversation comment, warn and apply -'requested-info' after the warning threshold, then close after the closing -threshold without activity. CODEOWNER-authored and 'dont-close'-labeled pull -requests are exempt. +After a repository or organization member's latest conversation comment, warn +and apply 'requested-info' after the warning threshold, then close after the +closing threshold without activity. Member-authored and 'dont-close'-labeled +pull requests are exempt. Membership uses GitHub's exact MEMBER association. """ from __future__ import annotations @@ -18,9 +18,9 @@ from github import Auth, Github, GithubException from github.IssueComment import IssueComment from github.PullRequest import PullRequest -from github.Repository import Repository +MEMBER_ASSOCIATION = "MEMBER" REQUESTED_INFO_LABEL = "requested-info" DONT_CLOSE_LABEL = "dont-close" WARNING_MARKER = "" @@ -38,36 +38,6 @@ ) -def get_maintainers(repo: Repository) -> set[str]: - """Fetch maintainer usernames from CODEOWNERS on the default branch.""" - codeowners = repo.get_contents( - ".github/CODEOWNERS", - ref=repo.default_branch, - ) - - if isinstance(codeowners, list): - raise RuntimeError("Expected CODEOWNERS to be a file") - - content = codeowners.decoded_content.decode("utf-8") - owners = set() - - for line in content.splitlines(): - parts = line.split() - if not parts or parts[0] not in {"/python/", "/dotnet/"}: - continue - - owners.update( - owner.removeprefix("@").lower() - for owner in parts[1:] - if owner.startswith("@") - ) - - if not owners: - raise RuntimeError("No maintainers found in CODEOWNERS") - - return owners - - def get_latest_comment(pull_request: PullRequest) -> IssueComment | None: """Fetch only the newest page of regular PR conversation comments.""" return next(iter(pull_request.get_issue_comments().reversed), None) @@ -105,10 +75,10 @@ def has_warning_marker(comment: IssueComment | None) -> bool: return bool(comment and WARNING_MARKER in (comment.body or "")) -def is_exempt(pull_request: PullRequest, maintainers: set[str]) -> bool: +def is_exempt(pull_request: PullRequest) -> bool: """Return whether a pull request is exempt from the lifecycle.""" return ( - pull_request.user.login.lower() in maintainers + pull_request.author_association == MEMBER_ASSOCIATION or any(label.name == DONT_CLOSE_LABEL for label in pull_request.labels) ) @@ -116,12 +86,11 @@ def is_exempt(pull_request: PullRequest, maintainers: set[str]) -> bool: def should_warn( pull_request: PullRequest, latest_comment: IssueComment | None, - maintainers: set[str], now: datetime, warning_threshold_days: int, ) -> bool: """Return whether an external pull request is ready for its warning.""" - if is_exempt(pull_request, maintainers): + if is_exempt(pull_request): return False if any(label.name == REQUESTED_INFO_LABEL for label in pull_request.labels): return False @@ -133,20 +102,18 @@ def should_warn( return bool( latest_comment - and latest_comment.user - and latest_comment.user.login.lower() in maintainers + and latest_comment.author_association == MEMBER_ASSOCIATION ) def should_close( pull_request: PullRequest, latest_comment: IssueComment | None, - maintainers: set[str], now: datetime, closing_threshold_days: int, ) -> bool: """Return whether a warned external pull request is ready to close.""" - if is_exempt(pull_request, maintainers): + if is_exempt(pull_request): return False if not any( label.name == REQUESTED_INFO_LABEL @@ -345,12 +312,6 @@ def main() -> None: g = Github(auth=Auth.Token(token)) repo = g.get_repo(repository) - print("Fetching maintainers...") - maintainers = get_maintainers(repo) - print(f"Found {len(maintainers)} maintainers.\n") - for maintainer in sorted(maintainers): - print(f" @{maintainer}") - now = datetime.now(timezone.utc) warned = [] closed = [] @@ -368,7 +329,7 @@ def main() -> None: for pull_request in pull_requests: scanned += 1 - if is_exempt(pull_request, maintainers): + if is_exempt(pull_request): continue lookup_succeeded, latest_comment = get_latest_comment_with_retry( @@ -397,7 +358,6 @@ def main() -> None: if should_close( pull_request, latest_comment, - maintainers, now, closing_threshold_days, ): @@ -410,7 +370,6 @@ def main() -> None: if should_warn( pull_request, latest_comment, - maintainers, now, warning_threshold_days, ): diff --git a/.github/tests/test_stale_pr_ping.py b/.github/tests/test_stale_pr_ping.py index 09bc5c78c3f..3a5e000c74a 100644 --- a/.github/tests/test_stale_pr_ping.py +++ b/.github/tests/test_stale_pr_ping.py @@ -19,6 +19,7 @@ CLOSING_COMMENT, CLOSING_THRESHOLD_DAYS, DONT_CLOSE_LABEL, + MEMBER_ASSOCIATION, REQUESTED_INFO_LABEL, WARNING_COMMENT, WARNING_MARKER, @@ -27,7 +28,6 @@ close, get_latest_comment, get_latest_comment_with_retry, - get_maintainers, get_runtime_thresholds, has_warning_marker, main, @@ -37,11 +37,14 @@ warn, ) -MAINTAINERS = {"alice", "bob"} NOW = datetime(2026, 3, 15, 12, 0, 0, tzinfo=timezone.utc) -def _make_comment(login: str | None, body: str = "") -> MagicMock: +def _make_comment( + login: str | None, + body: str = "", + author_association: str = "CONTRIBUTOR", +) -> MagicMock: comment = MagicMock() if login is None: comment.user = None @@ -49,6 +52,7 @@ def _make_comment(login: str | None, body: str = "") -> MagicMock: comment.user = MagicMock() comment.user.login = login comment.body = body + comment.author_association = author_association return comment @@ -60,6 +64,7 @@ def _make_label(name: str) -> MagicMock: def _make_pull_request( author: str = "external", + author_association: str = "CONTRIBUTOR", labels: list[str] | None = None, comments: list[MagicMock] | None = None, number: int = 42, @@ -68,6 +73,7 @@ def _make_pull_request( pull_request = MagicMock() pull_request.user = MagicMock() pull_request.user.login = author + pull_request.author_association = author_association pull_request.number = number pull_request.labels = [ _make_label(name) @@ -190,8 +196,11 @@ def test_negative_runtime_override_is_rejected(self, capsys): class TestShouldWarn: - def test_warns_at_threshold_for_latest_maintainer_comment(self): - latest_comment = _make_comment("alice") + def test_warns_at_threshold_for_latest_member_comment(self): + latest_comment = _make_comment( + "alice", + author_association=MEMBER_ASSOCIATION, + ) pull_request = _make_pull_request( updated_at=NOW - timedelta(days=4), ) @@ -199,13 +208,15 @@ def test_warns_at_threshold_for_latest_maintainer_comment(self): assert should_warn( pull_request, latest_comment, - MAINTAINERS, NOW, 4, ) is True def test_recent_activity_resets_warning_clock(self): - latest_comment = _make_comment("alice") + latest_comment = _make_comment( + "alice", + author_association=MEMBER_ASSOCIATION, + ) pull_request = _make_pull_request( updated_at=NOW - timedelta(days=3), ) @@ -213,28 +224,32 @@ def test_recent_activity_resets_warning_clock(self): assert should_warn( pull_request, latest_comment, - MAINTAINERS, NOW, 4, ) is False - def test_maintainer_authored_pr_is_exempt(self): - latest_comment = _make_comment("bob") + def test_member_authored_pr_is_exempt(self): + latest_comment = _make_comment( + "bob", + author_association=MEMBER_ASSOCIATION, + ) pull_request = _make_pull_request( - author="Alice", + author_association=MEMBER_ASSOCIATION, updated_at=NOW - timedelta(days=4), ) assert should_warn( pull_request, latest_comment, - MAINTAINERS, NOW, 4, ) is False def test_requested_info_pr_is_not_warned_again(self): - latest_comment = _make_comment("alice") + latest_comment = _make_comment( + "alice", + author_association=MEMBER_ASSOCIATION, + ) pull_request = _make_pull_request( labels=[REQUESTED_INFO_LABEL], updated_at=NOW - timedelta(days=4), @@ -243,13 +258,15 @@ def test_requested_info_pr_is_not_warned_again(self): assert should_warn( pull_request, latest_comment, - MAINTAINERS, NOW, 4, ) is False def test_dont_close_pr_is_not_warned(self): - latest_comment = _make_comment("alice") + latest_comment = _make_comment( + "alice", + author_association=MEMBER_ASSOCIATION, + ) pull_request = _make_pull_request( labels=[DONT_CLOSE_LABEL], updated_at=NOW - timedelta(days=4), @@ -258,7 +275,6 @@ def test_dont_close_pr_is_not_warned(self): assert should_warn( pull_request, latest_comment, - MAINTAINERS, NOW, 4, ) is False @@ -271,13 +287,22 @@ def test_pr_without_regular_conversation_comments_is_skipped(self): assert should_warn( pull_request, None, - MAINTAINERS, NOW, 4, ) is False - def test_latest_external_comment_is_skipped(self): - latest_comment = _make_comment("external") + @pytest.mark.parametrize( + "author_association", + ["OWNER", "COLLABORATOR", "CONTRIBUTOR"], + ) + def test_non_member_latest_comment_is_skipped( + self, + author_association, + ): + latest_comment = _make_comment( + "external", + author_association=author_association, + ) pull_request = _make_pull_request( updated_at=NOW - timedelta(days=4), ) @@ -285,13 +310,12 @@ def test_latest_external_comment_is_skipped(self): assert should_warn( pull_request, latest_comment, - MAINTAINERS, NOW, 4, ) is False def test_deleted_latest_comment_author_is_skipped(self): - latest_comment = _make_comment(None) + latest_comment = _make_comment(None, author_association="NONE") pull_request = _make_pull_request( updated_at=NOW - timedelta(days=4), ) @@ -299,11 +323,34 @@ def test_deleted_latest_comment_author_is_skipped(self): assert should_warn( pull_request, latest_comment, - MAINTAINERS, NOW, 4, ) is False + @pytest.mark.parametrize( + "author_association", + ["OWNER", "COLLABORATOR", "CONTRIBUTOR"], + ) + def test_non_member_pr_author_is_not_exempt( + self, + author_association, + ): + latest_comment = _make_comment( + "alice", + author_association=MEMBER_ASSOCIATION, + ) + pull_request = _make_pull_request( + author_association=author_association, + updated_at=NOW - timedelta(days=4), + ) + + assert should_warn( + pull_request, + latest_comment, + NOW, + 4, + ) is True + class TestWarn: def test_dry_run_mutates_nothing(self, capsys): @@ -389,7 +436,6 @@ def test_closes_requested_info_pr_at_threshold(self): assert should_close( pull_request, _make_warning_comment(), - MAINTAINERS, NOW, 4, ) is True @@ -403,14 +449,13 @@ def test_recent_activity_resets_closing_clock(self): assert should_close( pull_request, _make_warning_comment(), - MAINTAINERS, NOW, 4, ) is False - def test_maintainer_authored_pr_is_exempt(self): + def test_member_authored_pr_is_exempt(self): pull_request = _make_pull_request( - author="Alice", + author_association=MEMBER_ASSOCIATION, labels=[REQUESTED_INFO_LABEL], updated_at=NOW - timedelta(days=4), ) @@ -418,11 +463,31 @@ def test_maintainer_authored_pr_is_exempt(self): assert should_close( pull_request, _make_warning_comment(), - MAINTAINERS, NOW, 4, ) is False + @pytest.mark.parametrize( + "author_association", + ["OWNER", "COLLABORATOR", "CONTRIBUTOR"], + ) + def test_non_member_pr_author_is_not_exempt( + self, + author_association, + ): + pull_request = _make_pull_request( + author_association=author_association, + labels=[REQUESTED_INFO_LABEL], + updated_at=NOW - timedelta(days=4), + ) + + assert should_close( + pull_request, + _make_warning_comment(), + NOW, + 4, + ) is True + def test_unlabeled_pr_is_not_closed(self): pull_request = _make_pull_request( updated_at=NOW - timedelta(days=4), @@ -431,7 +496,6 @@ def test_unlabeled_pr_is_not_closed(self): assert should_close( pull_request, _make_warning_comment(), - MAINTAINERS, NOW, 4, ) is False @@ -445,7 +509,6 @@ def test_dont_close_pr_is_not_closed(self): assert should_close( pull_request, _make_warning_comment(), - MAINTAINERS, NOW, 4, ) is False @@ -459,7 +522,6 @@ def test_manual_requested_info_without_marker_is_not_closed(self): assert should_close( pull_request, _make_comment("alice", "manual comment"), - MAINTAINERS, NOW, 4, ) is False @@ -561,54 +623,6 @@ def test_final_close_failure_reports_failure_without_repeating_comment( assert mock_sleep.call_count == 2 -class TestGetMaintainers: - def test_fetches_and_parses_unique_python_and_dotnet_owners(self): - repo = MagicMock() - repo.default_branch = "feature-branch" - codeowners = MagicMock() - codeowners.decoded_content = b""" -/python/ @Alice @shared -/dotnet/ @BOB @Shared -/docs/ @ignored -""" - repo.get_contents.return_value = codeowners - - assert get_maintainers(repo) == {"alice", "bob", "shared"} - repo.get_contents.assert_called_once_with( - ".github/CODEOWNERS", - ref="feature-branch", - ) - - def test_rejects_directory_response(self): - repo = MagicMock() - repo.get_contents.return_value = [MagicMock()] - - with pytest.raises(RuntimeError, match="Expected CODEOWNERS to be a file"): - get_maintainers(repo) - - def test_rejects_codeowners_without_matching_owners(self): - repo = MagicMock() - codeowners = MagicMock() - codeowners.decoded_content = b""" -/docs/ @docs-owner -/python/** @python-owner -""" - repo.get_contents.return_value = codeowners - - with pytest.raises(RuntimeError, match="No maintainers found in CODEOWNERS"): - get_maintainers(repo) - - def test_propagates_codeowners_fetch_failure(self): - repo = MagicMock() - error = GithubException(404, {"message": "Not Found"}, None) - repo.get_contents.side_effect = error - - with pytest.raises(GithubException) as exc_info: - get_maintainers(repo) - - assert exc_info.value is error - - class TestMain: @patch.dict(os.environ, {}, clear=True) def test_missing_github_token(self, capsys): @@ -651,17 +665,20 @@ def test_invalid_days_threshold_stops_before_github(self, mock_github, capsys): clear=True, ) @patch("stale_pr_ping.datetime") - @patch("stale_pr_ping.get_maintainers", return_value=MAINTAINERS) @patch("stale_pr_ping.Github") def test_scans_only_open_prs_and_defers_new_warning( self, mock_github, - mock_get_maintainers, mock_datetime, capsys, ): pull_request = _make_pull_request( - comments=[_make_comment("alice")], + comments=[ + _make_comment( + "alice", + author_association=MEMBER_ASSOCIATION, + ) + ], updated_at=NOW - timedelta(days=7), ) repo = mock_github.return_value.get_repo.return_value @@ -670,7 +687,6 @@ def test_scans_only_open_prs_and_defers_new_warning( main() - mock_get_maintainers.assert_called_once_with(repo) repo.get_pulls.assert_called_once_with(state="open") repo.get_issues.assert_not_called() pull_request.create_issue_comment.assert_called_once_with( @@ -693,12 +709,10 @@ def test_scans_only_open_prs_and_defers_new_warning( clear=True, ) @patch("stale_pr_ping.datetime") - @patch("stale_pr_ping.get_maintainers", return_value=MAINTAINERS) @patch("stale_pr_ping.Github") def test_existing_requested_info_pr_closes_before_warning_check( self, mock_github, - mock_get_maintainers, mock_datetime, capsys, ): @@ -724,7 +738,6 @@ def assert_materialized(**kwargs): main() - mock_get_maintainers.assert_called_once_with(repo) pull_request.get_issue_comments.assert_called_once_with() pull_request.create_issue_comment.assert_called_once_with(CLOSING_COMMENT) pull_request.edit.assert_called_once_with(state="closed") @@ -741,12 +754,10 @@ def assert_materialized(**kwargs): clear=True, ) @patch("stale_pr_ping.datetime") - @patch("stale_pr_ping.get_maintainers", return_value=MAINTAINERS) @patch("stale_pr_ping.Github") def test_manual_requested_info_is_cleared_instead_of_closed( self, mock_github, - mock_get_maintainers, mock_datetime, ): pull_request = _make_pull_request( @@ -760,7 +771,6 @@ def test_manual_requested_info_is_cleared_instead_of_closed( main() - mock_get_maintainers.assert_called_once_with(repo) pull_request.remove_from_labels.assert_called_once_with( REQUESTED_INFO_LABEL ) @@ -776,12 +786,10 @@ def test_manual_requested_info_is_cleared_instead_of_closed( clear=True, ) @patch("stale_pr_ping.datetime") - @patch("stale_pr_ping.get_maintainers", return_value=MAINTAINERS) @patch("stale_pr_ping.Github") def test_warning_marker_restores_missing_label_without_reposting( self, mock_github, - mock_get_maintainers, mock_datetime, ): pull_request = _make_pull_request( @@ -794,7 +802,6 @@ def test_warning_marker_restores_missing_label_without_reposting( main() - mock_get_maintainers.assert_called_once_with(repo) pull_request.add_to_labels.assert_called_once_with(REQUESTED_INFO_LABEL) pull_request.create_issue_comment.assert_not_called() pull_request.edit.assert_not_called() @@ -809,12 +816,10 @@ def test_warning_marker_restores_missing_label_without_reposting( clear=True, ) @patch("stale_pr_ping.datetime") - @patch("stale_pr_ping.get_maintainers", return_value=MAINTAINERS) @patch("stale_pr_ping.Github") def test_recovery_dry_run_mutates_nothing( self, mock_github, - mock_get_maintainers, mock_datetime, ): manual_label = _make_pull_request( @@ -832,7 +837,6 @@ def test_recovery_dry_run_mutates_nothing( main() - mock_get_maintainers.assert_called_once_with(repo) manual_label.remove_from_labels.assert_not_called() missing_label.add_to_labels.assert_not_called() manual_label.edit.assert_not_called() @@ -848,18 +852,21 @@ def test_recovery_dry_run_mutates_nothing( ) @patch("stale_pr_ping.datetime") @patch("stale_pr_ping.warn", return_value=False) - @patch("stale_pr_ping.get_maintainers", return_value=MAINTAINERS) @patch("stale_pr_ping.Github") def test_failed_warning_is_reported_and_exits( self, mock_github, - mock_get_maintainers, mock_warn, mock_datetime, capsys, ): pull_request = _make_pull_request( - comments=[_make_comment("alice")], + comments=[ + _make_comment( + "alice", + author_association=MEMBER_ASSOCIATION, + ) + ], updated_at=NOW - timedelta(days=WARNING_THRESHOLD_DAYS), ) repo = mock_github.return_value.get_repo.return_value @@ -869,7 +876,6 @@ def test_failed_warning_is_reported_and_exits( with pytest.raises(SystemExit): main() - mock_get_maintainers.assert_called_once_with(repo) mock_warn.assert_called_once_with( pull_request, False, @@ -889,12 +895,10 @@ def test_failed_warning_is_reported_and_exits( ) @patch("stale_pr_ping.time.sleep") @patch("stale_pr_ping.datetime") - @patch("stale_pr_ping.get_maintainers", return_value=MAINTAINERS) @patch("stale_pr_ping.Github") def test_lookup_failure_is_recorded_and_later_pr_is_processed( self, mock_github, - mock_get_maintainers, mock_datetime, mock_sleep, capsys, @@ -906,7 +910,12 @@ def test_lookup_failure_is_recorded_and_later_pr_is_processed( None, ) later_pull_request = _make_pull_request( - comments=[_make_comment("alice")], + comments=[ + _make_comment( + "alice", + author_association=MEMBER_ASSOCIATION, + ) + ], number=42, updated_at=NOW - timedelta(days=WARNING_THRESHOLD_DAYS), ) @@ -920,7 +929,6 @@ def test_lookup_failure_is_recorded_and_later_pr_is_processed( with pytest.raises(SystemExit): main() - mock_get_maintainers.assert_called_once_with(repo) assert failed_pull_request.get_issue_comments.call_count == 3 assert mock_sleep.call_args_list == [call(2), call(4)] later_pull_request.create_issue_comment.assert_called_once_with( From 5533282bad3ec36b8c1292583ba81541179c4ca0 Mon Sep 17 00:00:00 2001 From: Jose Alvarez Date: Tue, 3 Nov 2026 13:20:36 +0100 Subject: [PATCH 11/11] Refactor stale PR workflow tests --- .github/tests/test_stale_pr_ping.py | 766 ++++++++++------------------ 1 file changed, 272 insertions(+), 494 deletions(-) diff --git a/.github/tests/test_stale_pr_ping.py b/.github/tests/test_stale_pr_ping.py index 3a5e000c74a..ef65c9b92c1 100644 --- a/.github/tests/test_stale_pr_ping.py +++ b/.github/tests/test_stale_pr_ping.py @@ -7,6 +7,7 @@ import os import sys from datetime import datetime, timedelta, timezone +from types import SimpleNamespace from unittest.mock import MagicMock, call, patch import pytest @@ -15,6 +16,7 @@ # Ensure the script directory is importable sys.path.insert(0, os.path.join(os.path.dirname(__file__), "..", "scripts")) +import stale_pr_ping from stale_pr_ping import ( CLOSING_COMMENT, CLOSING_THRESHOLD_DAYS, @@ -30,6 +32,7 @@ get_latest_comment_with_retry, get_runtime_thresholds, has_warning_marker, + is_exempt, main, restore_requested_info, should_close, @@ -41,25 +44,23 @@ def _make_comment( - login: str | None, + login: str | None = "external", body: str = "", author_association: str = "CONTRIBUTOR", ) -> MagicMock: comment = MagicMock() - if login is None: - comment.user = None - else: - comment.user = MagicMock() - comment.user.login = login + comment.user = None if login is None else MagicMock(login=login) comment.body = body comment.author_association = author_association return comment -def _make_label(name: str) -> MagicMock: - label = MagicMock() - label.name = name - return label +def _member_comment(login: str = "member") -> MagicMock: + return _make_comment(login, author_association=MEMBER_ASSOCIATION) + + +def _warning_comment() -> MagicMock: + return _make_comment("github-actions[bot]", WARNING_MARKER) def _make_pull_request( @@ -71,12 +72,11 @@ def _make_pull_request( updated_at: datetime = NOW, ) -> MagicMock: pull_request = MagicMock() - pull_request.user = MagicMock() - pull_request.user.login = author + pull_request.user = MagicMock(login=author) pull_request.author_association = author_association pull_request.number = number pull_request.labels = [ - _make_label(name) + SimpleNamespace(name=name) for name in (labels or []) ] pull_request.updated_at = updated_at @@ -86,16 +86,30 @@ def _make_pull_request( return pull_request -def _make_warning_comment() -> MagicMock: - return _make_comment("github-actions[bot]", WARNING_MARKER) +@pytest.fixture +def main_context(monkeypatch): + github = MagicMock() + repo = github.return_value.get_repo.return_value + mock_datetime = MagicMock() + mock_datetime.now.return_value = NOW + monkeypatch.setattr(stale_pr_ping, "Github", github) + monkeypatch.setattr(stale_pr_ping, "datetime", mock_datetime) + monkeypatch.setenv("GITHUB_TOKEN", "tok") + monkeypatch.setenv("GITHUB_REPOSITORY", "org/repo") + monkeypatch.delenv("DAYS_THRESHOLD", raising=False) + monkeypatch.delenv("DRY_RUN", raising=False) + return SimpleNamespace( + github=github, + repo=repo, + datetime=mock_datetime, + ) class TestLatestComment: def test_fetches_only_newest_reversed_comment_page(self): - older_comment = _make_comment("external") - latest_comment = _make_comment("alice") + latest_comment = _member_comment() pull_request = _make_pull_request( - comments=[older_comment, latest_comment], + comments=[_make_comment(), latest_comment], ) issue_comments = pull_request.get_issue_comments.return_value issue_comments.__iter__.side_effect = AssertionError( @@ -106,9 +120,7 @@ def test_fetches_only_newest_reversed_comment_page(self): pull_request.get_issue_comments.assert_called_once_with() def test_returns_none_without_comments(self): - pull_request = _make_pull_request() - - assert get_latest_comment(pull_request) is None + assert get_latest_comment(_make_pull_request()) is None @patch("stale_pr_ping.time.sleep") def test_transient_lookup_failure_succeeds_on_retry( @@ -116,7 +128,7 @@ def test_transient_lookup_failure_succeeds_on_retry( mock_sleep, capsys, ): - latest_comment = _make_comment("alice") + latest_comment = _member_comment() pull_request = _make_pull_request(comments=[latest_comment]) issue_comments = pull_request.get_issue_comments.return_value pull_request.get_issue_comments.side_effect = [ @@ -154,202 +166,172 @@ def test_final_lookup_failure_is_reported( ) -class TestWarningMarker: - def test_detects_warning_marker(self): - assert has_warning_marker(_make_warning_comment()) is True - - def test_rejects_regular_or_missing_comment(self): - assert has_warning_marker(_make_comment("alice", "regular comment")) is False - assert has_warning_marker(None) is False - - -class TestPolicyThresholds: - def test_default_constants(self): - assert WARNING_THRESHOLD_DAYS == 4 - assert CLOSING_THRESHOLD_DAYS == 4 - - @patch.dict(os.environ, {}, clear=True) - def test_runtime_defaults(self): - assert get_runtime_thresholds() == (4, 4) - - @patch.dict(os.environ, {"DAYS_THRESHOLD": "7"}, clear=True) - def test_runtime_override_applies_to_both_stages(self): - assert get_runtime_thresholds() == (7, 7) - - @patch.dict(os.environ, {"DAYS_THRESHOLD": "0"}, clear=True) - def test_zero_threshold_is_valid(self): - assert get_runtime_thresholds() == (0, 0) - - @patch.dict(os.environ, {"DAYS_THRESHOLD": "invalid"}, clear=True) - def test_invalid_runtime_override_is_rejected(self, capsys): - with pytest.raises(SystemExit): - get_runtime_thresholds() - - assert "non-negative integer" in capsys.readouterr().out - - @patch.dict(os.environ, {"DAYS_THRESHOLD": "-1"}, clear=True) - def test_negative_runtime_override_is_rejected(self, capsys): - with pytest.raises(SystemExit): - get_runtime_thresholds() +class TestPolicy: + @pytest.mark.parametrize( + ("association", "labels", "expected"), + [ + (MEMBER_ASSOCIATION, [], True), + ("OWNER", [], False), + ("COLLABORATOR", [], False), + ("CONTRIBUTOR", [], False), + ("CONTRIBUTOR", [DONT_CLOSE_LABEL], True), + ], + ) + def test_exemptions(self, association, labels, expected): + pull_request = _make_pull_request( + author_association=association, + labels=labels, + ) - assert "non-negative integer" in capsys.readouterr().out + assert is_exempt(pull_request) is expected + @pytest.mark.parametrize( + ("comment", "expected"), + [ + (_warning_comment(), True), + (_make_comment(body="regular comment"), False), + (None, False), + ], + ) + def test_warning_marker(self, comment, expected): + assert has_warning_marker(comment) is expected -class TestShouldWarn: def test_warns_at_threshold_for_latest_member_comment(self): - latest_comment = _make_comment( - "alice", - author_association=MEMBER_ASSOCIATION, - ) pull_request = _make_pull_request( - updated_at=NOW - timedelta(days=4), + updated_at=NOW - timedelta(days=WARNING_THRESHOLD_DAYS), ) assert should_warn( pull_request, - latest_comment, + _member_comment(), NOW, - 4, + WARNING_THRESHOLD_DAYS, ) is True def test_recent_activity_resets_warning_clock(self): - latest_comment = _make_comment( - "alice", - author_association=MEMBER_ASSOCIATION, - ) pull_request = _make_pull_request( - updated_at=NOW - timedelta(days=3), + updated_at=NOW - timedelta(days=WARNING_THRESHOLD_DAYS - 1), ) assert should_warn( pull_request, - latest_comment, + _member_comment(), NOW, - 4, + WARNING_THRESHOLD_DAYS, ) is False - def test_member_authored_pr_is_exempt(self): - latest_comment = _make_comment( - "bob", - author_association=MEMBER_ASSOCIATION, - ) + def test_requested_info_pr_is_not_warned_again(self): pull_request = _make_pull_request( - author_association=MEMBER_ASSOCIATION, - updated_at=NOW - timedelta(days=4), + labels=[REQUESTED_INFO_LABEL], + updated_at=NOW - timedelta(days=WARNING_THRESHOLD_DAYS), ) assert should_warn( pull_request, - latest_comment, + _member_comment(), NOW, - 4, + WARNING_THRESHOLD_DAYS, ) is False - def test_requested_info_pr_is_not_warned_again(self): - latest_comment = _make_comment( - "alice", - author_association=MEMBER_ASSOCIATION, - ) + @pytest.mark.parametrize( + "latest_comment", + [ + None, + _make_comment(author_association="OWNER"), + _make_comment(author_association="COLLABORATOR"), + _make_comment(author_association="CONTRIBUTOR"), + _make_comment(None, author_association="NONE"), + ], + ) + def test_non_member_or_missing_latest_comment_is_skipped( + self, + latest_comment, + ): pull_request = _make_pull_request( - labels=[REQUESTED_INFO_LABEL], - updated_at=NOW - timedelta(days=4), + updated_at=NOW - timedelta(days=WARNING_THRESHOLD_DAYS), ) assert should_warn( pull_request, latest_comment, NOW, - 4, + WARNING_THRESHOLD_DAYS, ) is False - def test_dont_close_pr_is_not_warned(self): - latest_comment = _make_comment( - "alice", - author_association=MEMBER_ASSOCIATION, - ) + def test_closes_requested_info_pr_at_threshold(self): pull_request = _make_pull_request( - labels=[DONT_CLOSE_LABEL], - updated_at=NOW - timedelta(days=4), + labels=[REQUESTED_INFO_LABEL], + updated_at=NOW - timedelta(days=CLOSING_THRESHOLD_DAYS), ) - assert should_warn( + assert should_close( pull_request, - latest_comment, + _warning_comment(), NOW, - 4, - ) is False + CLOSING_THRESHOLD_DAYS, + ) is True - def test_pr_without_regular_conversation_comments_is_skipped(self): + def test_recent_activity_resets_closing_clock(self): pull_request = _make_pull_request( - updated_at=NOW - timedelta(days=4), + labels=[REQUESTED_INFO_LABEL], + updated_at=NOW - timedelta(days=CLOSING_THRESHOLD_DAYS - 1), ) - assert should_warn( + assert should_close( pull_request, - None, + _warning_comment(), NOW, - 4, + CLOSING_THRESHOLD_DAYS, ) is False @pytest.mark.parametrize( - "author_association", - ["OWNER", "COLLABORATOR", "CONTRIBUTOR"], + ("labels", "comment"), + [ + ([], _warning_comment()), + ([REQUESTED_INFO_LABEL], _make_comment(body="manual comment")), + ], ) - def test_non_member_latest_comment_is_skipped( - self, - author_association, - ): - latest_comment = _make_comment( - "external", - author_association=author_association, - ) + def test_closing_requires_label_and_warning_marker(self, labels, comment): pull_request = _make_pull_request( - updated_at=NOW - timedelta(days=4), + labels=labels, + updated_at=NOW - timedelta(days=CLOSING_THRESHOLD_DAYS), ) - assert should_warn( + assert should_close( pull_request, - latest_comment, + comment, NOW, - 4, + CLOSING_THRESHOLD_DAYS, ) is False - def test_deleted_latest_comment_author_is_skipped(self): - latest_comment = _make_comment(None, author_association="NONE") - pull_request = _make_pull_request( - updated_at=NOW - timedelta(days=4), - ) - assert should_warn( - pull_request, - latest_comment, - NOW, - 4, - ) is False +class TestThresholds: + def test_default_constants(self): + assert WARNING_THRESHOLD_DAYS == 4 + assert CLOSING_THRESHOLD_DAYS == 4 @pytest.mark.parametrize( - "author_association", - ["OWNER", "COLLABORATOR", "CONTRIBUTOR"], + ("environment", "expected"), + [ + ({}, (4, 4)), + ({"DAYS_THRESHOLD": "7"}, (7, 7)), + ({"DAYS_THRESHOLD": "0"}, (0, 0)), + ], ) - def test_non_member_pr_author_is_not_exempt( - self, - author_association, - ): - latest_comment = _make_comment( - "alice", - author_association=MEMBER_ASSOCIATION, - ) - pull_request = _make_pull_request( - author_association=author_association, - updated_at=NOW - timedelta(days=4), - ) + def test_runtime_thresholds(self, environment, expected): + with patch.dict(os.environ, environment, clear=True): + assert get_runtime_thresholds() == expected + + @pytest.mark.parametrize("value", ["invalid", "-1"]) + def test_invalid_runtime_override_is_rejected(self, value, capsys): + with patch.dict( + os.environ, + {"DAYS_THRESHOLD": value}, + clear=True, + ), pytest.raises(SystemExit): + get_runtime_thresholds() - assert should_warn( - pull_request, - latest_comment, - NOW, - 4, - ) is True + assert "non-negative integer" in capsys.readouterr().out class TestWarn: @@ -359,11 +341,9 @@ def test_dry_run_mutates_nothing(self, capsys): assert warn(pull_request, dry_run=True, closing_threshold_days=6) is True pull_request.create_issue_comment.assert_not_called() pull_request.add_to_labels.assert_not_called() - output = capsys.readouterr().out - assert "DRY RUN" in output - assert REQUESTED_INFO_LABEL in output + assert REQUESTED_INFO_LABEL in capsys.readouterr().out - def test_posts_warning_then_applies_requested_info(self): + def test_posts_warning_before_applying_label(self): pull_request = _make_pull_request() assert warn(pull_request, dry_run=False, closing_threshold_days=6) is True @@ -388,7 +368,7 @@ def test_comment_retry_succeeds(self, mock_sleep): assert warn(pull_request, dry_run=False, closing_threshold_days=4) is True assert pull_request.create_issue_comment.call_count == 2 pull_request.add_to_labels.assert_called_once_with(REQUESTED_INFO_LABEL) - mock_sleep.assert_called_once() + mock_sleep.assert_called_once_with(2) @patch("stale_pr_ping.time.sleep") def test_label_retry_does_not_repeat_warning(self, mock_sleep): @@ -401,176 +381,89 @@ def test_label_retry_does_not_repeat_warning(self, mock_sleep): assert warn(pull_request, dry_run=False, closing_threshold_days=4) is True pull_request.create_issue_comment.assert_called_once() assert pull_request.add_to_labels.call_count == 2 - mock_sleep.assert_called_once() - - @patch("stale_pr_ping.time.sleep") - def test_warning_failure_never_applies_label(self, mock_sleep): - pull_request = _make_pull_request() - pull_request.create_issue_comment.side_effect = Exception("comment error") - - assert warn(pull_request, dry_run=False, closing_threshold_days=4) is False - assert pull_request.create_issue_comment.call_count == 3 - pull_request.add_to_labels.assert_not_called() + mock_sleep.assert_called_once_with(2) + @pytest.mark.parametrize("failure_stage", ["comment", "label"]) @patch("stale_pr_ping.time.sleep") - def test_final_label_failure_reports_failure_without_repeating_warning( + def test_final_failure_preserves_partial_state( self, mock_sleep, + failure_stage, ): pull_request = _make_pull_request() - pull_request.add_to_labels.side_effect = Exception("label error") + if failure_stage == "comment": + pull_request.create_issue_comment.side_effect = Exception("comment") + else: + pull_request.add_to_labels.side_effect = Exception("label") assert warn(pull_request, dry_run=False, closing_threshold_days=4) is False - pull_request.create_issue_comment.assert_called_once() - assert pull_request.add_to_labels.call_count == 3 - assert mock_sleep.call_count == 2 - - -class TestShouldClose: - def test_closes_requested_info_pr_at_threshold(self): - pull_request = _make_pull_request( - labels=[REQUESTED_INFO_LABEL], - updated_at=NOW - timedelta(days=4), - ) - - assert should_close( - pull_request, - _make_warning_comment(), - NOW, - 4, - ) is True + assert mock_sleep.call_args_list == [call(2), call(4)] + if failure_stage == "comment": + assert pull_request.create_issue_comment.call_count == 3 + pull_request.add_to_labels.assert_not_called() + else: + pull_request.create_issue_comment.assert_called_once() + assert pull_request.add_to_labels.call_count == 3 - def test_recent_activity_resets_closing_clock(self): - pull_request = _make_pull_request( - labels=[REQUESTED_INFO_LABEL], - updated_at=NOW - timedelta(days=3), - ) - assert should_close( - pull_request, - _make_warning_comment(), - NOW, - 4, - ) is False - - def test_member_authored_pr_is_exempt(self): - pull_request = _make_pull_request( - author_association=MEMBER_ASSOCIATION, - labels=[REQUESTED_INFO_LABEL], - updated_at=NOW - timedelta(days=4), - ) +class TestRequestedInfoRecovery: + @pytest.mark.parametrize( + ("function", "method_name", "label"), + [ + (clear_requested_info, "remove_from_labels", REQUESTED_INFO_LABEL), + (restore_requested_info, "add_to_labels", REQUESTED_INFO_LABEL), + ], + ) + def test_updates_requested_info(self, function, method_name, label): + pull_request = _make_pull_request(labels=[REQUESTED_INFO_LABEL]) - assert should_close( - pull_request, - _make_warning_comment(), - NOW, - 4, - ) is False + assert function(pull_request, dry_run=False) is True + getattr(pull_request, method_name).assert_called_once_with(label) @pytest.mark.parametrize( - "author_association", - ["OWNER", "COLLABORATOR", "CONTRIBUTOR"], + ("function", "method_name"), + [ + (clear_requested_info, "remove_from_labels"), + (restore_requested_info, "add_to_labels"), + ], ) - def test_non_member_pr_author_is_not_exempt( + def test_dry_run_mutates_nothing( self, - author_association, + function, + method_name, + capsys, ): - pull_request = _make_pull_request( - author_association=author_association, - labels=[REQUESTED_INFO_LABEL], - updated_at=NOW - timedelta(days=4), - ) - - assert should_close( - pull_request, - _make_warning_comment(), - NOW, - 4, - ) is True - - def test_unlabeled_pr_is_not_closed(self): - pull_request = _make_pull_request( - updated_at=NOW - timedelta(days=4), - ) - - assert should_close( - pull_request, - _make_warning_comment(), - NOW, - 4, - ) is False - - def test_dont_close_pr_is_not_closed(self): - pull_request = _make_pull_request( - labels=[REQUESTED_INFO_LABEL, DONT_CLOSE_LABEL], - updated_at=NOW - timedelta(days=4), - ) - - assert should_close( - pull_request, - _make_warning_comment(), - NOW, - 4, - ) is False - - def test_manual_requested_info_without_marker_is_not_closed(self): - pull_request = _make_pull_request( - labels=[REQUESTED_INFO_LABEL], - updated_at=NOW - timedelta(days=4), - ) - - assert should_close( - pull_request, - _make_comment("alice", "manual comment"), - NOW, - 4, - ) is False - - -class TestRequestedInfoRecovery: - def test_clear_requested_info(self): pull_request = _make_pull_request(labels=[REQUESTED_INFO_LABEL]) - assert clear_requested_info(pull_request, dry_run=False) is True - pull_request.remove_from_labels.assert_called_once_with( - REQUESTED_INFO_LABEL - ) - - def test_clear_requested_info_dry_run_mutates_nothing(self, capsys): - pull_request = _make_pull_request(labels=[REQUESTED_INFO_LABEL]) - - assert clear_requested_info(pull_request, dry_run=True) is True - pull_request.remove_from_labels.assert_not_called() + assert function(pull_request, dry_run=True) is True + getattr(pull_request, method_name).assert_not_called() assert "DRY RUN" in capsys.readouterr().out - def test_clear_requested_info_failure_is_explicit(self, capsys): + @pytest.mark.parametrize( + ("function", "method_name"), + [ + (clear_requested_info, "remove_from_labels"), + (restore_requested_info, "add_to_labels"), + ], + ) + def test_failure_is_explicit( + self, + function, + method_name, + capsys, + ): pull_request = _make_pull_request(labels=[REQUESTED_INFO_LABEL]) - pull_request.remove_from_labels.side_effect = Exception("remove error") + getattr(pull_request, method_name).side_effect = Exception("failure") - assert clear_requested_info(pull_request, dry_run=False) is False + assert function(pull_request, dry_run=False) is False assert "ERROR" in capsys.readouterr().out - def test_restore_requested_info_without_reposting_warning(self): + def test_restore_does_not_repeat_warning(self): pull_request = _make_pull_request() assert restore_requested_info(pull_request, dry_run=False) is True - pull_request.add_to_labels.assert_called_once_with(REQUESTED_INFO_LABEL) pull_request.create_issue_comment.assert_not_called() - def test_restore_requested_info_dry_run_mutates_nothing(self, capsys): - pull_request = _make_pull_request() - - assert restore_requested_info(pull_request, dry_run=True) is True - pull_request.add_to_labels.assert_not_called() - assert "DRY RUN" in capsys.readouterr().out - - def test_restore_requested_info_failure_is_explicit(self, capsys): - pull_request = _make_pull_request() - pull_request.add_to_labels.side_effect = Exception("label error") - - assert restore_requested_info(pull_request, dry_run=False) is False - assert "ERROR" in capsys.readouterr().out - class TestClose: def test_dry_run_mutates_nothing(self, capsys): @@ -581,7 +474,7 @@ def test_dry_run_mutates_nothing(self, capsys): pull_request.edit.assert_not_called() assert "DRY RUN" in capsys.readouterr().out - def test_posts_closing_comment_then_closes_pr(self): + def test_posts_comment_before_closing(self): pull_request = _make_pull_request() assert close(pull_request, dry_run=False) is True @@ -598,130 +491,97 @@ def test_close_retry_does_not_repeat_comment(self, mock_sleep): assert close(pull_request, dry_run=False) is True pull_request.create_issue_comment.assert_called_once_with(CLOSING_COMMENT) assert pull_request.edit.call_count == 2 - mock_sleep.assert_called_once() - - @patch("stale_pr_ping.time.sleep") - def test_comment_failure_never_closes_pr(self, mock_sleep): - pull_request = _make_pull_request() - pull_request.create_issue_comment.side_effect = Exception("comment error") - - assert close(pull_request, dry_run=False) is False - assert pull_request.create_issue_comment.call_count == 3 - pull_request.edit.assert_not_called() + mock_sleep.assert_called_once_with(2) + @pytest.mark.parametrize("failure_stage", ["comment", "close"]) @patch("stale_pr_ping.time.sleep") - def test_final_close_failure_reports_failure_without_repeating_comment( + def test_final_failure_preserves_partial_state( self, mock_sleep, + failure_stage, ): pull_request = _make_pull_request() - pull_request.edit.side_effect = Exception("close error") + if failure_stage == "comment": + pull_request.create_issue_comment.side_effect = Exception("comment") + else: + pull_request.edit.side_effect = Exception("close") assert close(pull_request, dry_run=False) is False - pull_request.create_issue_comment.assert_called_once_with(CLOSING_COMMENT) - assert pull_request.edit.call_count == 3 - assert mock_sleep.call_count == 2 + assert mock_sleep.call_args_list == [call(2), call(4)] + if failure_stage == "comment": + assert pull_request.create_issue_comment.call_count == 3 + pull_request.edit.assert_not_called() + else: + pull_request.create_issue_comment.assert_called_once() + assert pull_request.edit.call_count == 3 class TestMain: - @patch.dict(os.environ, {}, clear=True) - def test_missing_github_token(self, capsys): - with pytest.raises(SystemExit): + @pytest.mark.parametrize( + ("environment", "message"), + [ + ({}, "GITHUB_TOKEN"), + ({"GITHUB_TOKEN": "tok"}, "GITHUB_REPOSITORY"), + ], + ) + def test_required_environment(self, environment, message, capsys): + with patch.dict(os.environ, environment, clear=True), pytest.raises( + SystemExit + ): main() - assert "GITHUB_TOKEN" in capsys.readouterr().out - - @patch.dict(os.environ, {"GITHUB_TOKEN": "tok"}, clear=True) - def test_missing_github_repository(self, capsys): - with pytest.raises(SystemExit): - main() + assert message in capsys.readouterr().out - assert "GITHUB_REPOSITORY" in capsys.readouterr().out + def test_invalid_threshold_stops_before_github( + self, + main_context, + monkeypatch, + capsys, + ): + monkeypatch.setenv("DAYS_THRESHOLD", "invalid") - @patch.dict( - os.environ, - { - "GITHUB_TOKEN": "tok", - "GITHUB_REPOSITORY": "org/repo", - "DAYS_THRESHOLD": "invalid", - }, - clear=True, - ) - @patch("stale_pr_ping.Github") - def test_invalid_days_threshold_stops_before_github(self, mock_github, capsys): with pytest.raises(SystemExit): main() - mock_github.assert_not_called() + main_context.github.assert_not_called() assert "non-negative integer" in capsys.readouterr().out - @patch.dict( - os.environ, - { - "GITHUB_TOKEN": "tok", - "GITHUB_REPOSITORY": "org/repo", - "DAYS_THRESHOLD": "7", - }, - clear=True, - ) - @patch("stale_pr_ping.datetime") - @patch("stale_pr_ping.Github") - def test_scans_only_open_prs_and_defers_new_warning( + def test_scans_only_open_prs_and_warns( self, - mock_github, - mock_datetime, + main_context, + monkeypatch, capsys, ): + monkeypatch.setenv("DAYS_THRESHOLD", "7") pull_request = _make_pull_request( - comments=[ - _make_comment( - "alice", - author_association=MEMBER_ASSOCIATION, - ) - ], + comments=[_member_comment()], updated_at=NOW - timedelta(days=7), ) - repo = mock_github.return_value.get_repo.return_value - repo.get_pulls.return_value = [pull_request] - mock_datetime.now.return_value = NOW + main_context.repo.get_pulls.return_value = [pull_request] main() - repo.get_pulls.assert_called_once_with(state="open") - repo.get_issues.assert_not_called() + main_context.repo.get_pulls.assert_called_once_with(state="open") + main_context.repo.get_issues.assert_not_called() pull_request.create_issue_comment.assert_called_once_with( - WARNING_COMMENT.format( - author="external", - closing_days=7, - ) + WARNING_COMMENT.format(author="external", closing_days=7) ) pull_request.add_to_labels.assert_called_once_with(REQUESTED_INFO_LABEL) pull_request.edit.assert_not_called() assert "Scanned 1 PRs, warned 1, closed 0" in capsys.readouterr().out - @patch.dict( - os.environ, - { - "GITHUB_TOKEN": "tok", - "GITHUB_REPOSITORY": "org/repo", - "DAYS_THRESHOLD": "7", - }, - clear=True, - ) - @patch("stale_pr_ping.datetime") - @patch("stale_pr_ping.Github") - def test_existing_requested_info_pr_closes_before_warning_check( + def test_materializes_prs_before_closing( self, - mock_github, - mock_datetime, + main_context, + monkeypatch, capsys, ): + monkeypatch.setenv("DAYS_THRESHOLD", "7") pull_request = _make_pull_request( labels=[REQUESTED_INFO_LABEL], - comments=[_make_warning_comment()], + comments=[_warning_comment()], updated_at=NOW - timedelta(days=7), ) - repo = mock_github.return_value.get_repo.return_value materialized = {"complete": False} def pull_requests(): @@ -732,42 +592,24 @@ def assert_materialized(**kwargs): assert materialized["complete"] is True assert kwargs == {"state": "closed"} - repo.get_pulls.return_value = pull_requests() + main_context.repo.get_pulls.return_value = pull_requests() pull_request.edit.side_effect = assert_materialized - mock_datetime.now.return_value = NOW main() - pull_request.get_issue_comments.assert_called_once_with() pull_request.create_issue_comment.assert_called_once_with(CLOSING_COMMENT) pull_request.edit.assert_called_once_with(state="closed") output = capsys.readouterr().out assert "Scanned 1 PRs, warned 0, closed 1, failed 0" in output assert "Closed: #42" in output - @patch.dict( - os.environ, - { - "GITHUB_TOKEN": "tok", - "GITHUB_REPOSITORY": "org/repo", - }, - clear=True, - ) - @patch("stale_pr_ping.datetime") - @patch("stale_pr_ping.Github") - def test_manual_requested_info_is_cleared_instead_of_closed( - self, - mock_github, - mock_datetime, - ): + def test_clears_manual_or_post_warning_state(self, main_context): pull_request = _make_pull_request( labels=[REQUESTED_INFO_LABEL], - comments=[_make_comment("external", "author update")], + comments=[_make_comment(body="author update")], updated_at=NOW - timedelta(days=CLOSING_THRESHOLD_DAYS), ) - repo = mock_github.return_value.get_repo.return_value - repo.get_pulls.return_value = [pull_request] - mock_datetime.now.return_value = NOW + main_context.repo.get_pulls.return_value = [pull_request] main() @@ -777,28 +619,12 @@ def test_manual_requested_info_is_cleared_instead_of_closed( pull_request.create_issue_comment.assert_not_called() pull_request.edit.assert_not_called() - @patch.dict( - os.environ, - { - "GITHUB_TOKEN": "tok", - "GITHUB_REPOSITORY": "org/repo", - }, - clear=True, - ) - @patch("stale_pr_ping.datetime") - @patch("stale_pr_ping.Github") - def test_warning_marker_restores_missing_label_without_reposting( - self, - mock_github, - mock_datetime, - ): + def test_restores_missing_label_without_reposting(self, main_context): pull_request = _make_pull_request( - comments=[_make_warning_comment()], + comments=[_warning_comment()], updated_at=NOW - timedelta(days=CLOSING_THRESHOLD_DAYS), ) - repo = mock_github.return_value.get_repo.return_value - repo.get_pulls.return_value = [pull_request] - mock_datetime.now.return_value = NOW + main_context.repo.get_pulls.return_value = [pull_request] main() @@ -806,34 +632,25 @@ def test_warning_marker_restores_missing_label_without_reposting( pull_request.create_issue_comment.assert_not_called() pull_request.edit.assert_not_called() - @patch.dict( - os.environ, - { - "GITHUB_TOKEN": "tok", - "GITHUB_REPOSITORY": "org/repo", - "DRY_RUN": "true", - }, - clear=True, - ) - @patch("stale_pr_ping.datetime") - @patch("stale_pr_ping.Github") def test_recovery_dry_run_mutates_nothing( self, - mock_github, - mock_datetime, + main_context, + monkeypatch, ): + monkeypatch.setenv("DRY_RUN", "true") manual_label = _make_pull_request( labels=[REQUESTED_INFO_LABEL], - comments=[_make_comment("external", "author update")], + comments=[_make_comment(body="author update")], number=41, ) missing_label = _make_pull_request( - comments=[_make_warning_comment()], + comments=[_warning_comment()], number=42, ) - repo = mock_github.return_value.get_repo.return_value - repo.get_pulls.return_value = [manual_label, missing_label] - mock_datetime.now.return_value = NOW + main_context.repo.get_pulls.return_value = [ + manual_label, + missing_label, + ] main() @@ -842,36 +659,19 @@ def test_recovery_dry_run_mutates_nothing( manual_label.edit.assert_not_called() missing_label.edit.assert_not_called() - @patch.dict( - os.environ, - { - "GITHUB_TOKEN": "tok", - "GITHUB_REPOSITORY": "org/repo", - }, - clear=True, - ) - @patch("stale_pr_ping.datetime") - @patch("stale_pr_ping.warn", return_value=False) - @patch("stale_pr_ping.Github") def test_failed_warning_is_reported_and_exits( self, - mock_github, - mock_warn, - mock_datetime, + main_context, + monkeypatch, capsys, ): pull_request = _make_pull_request( - comments=[ - _make_comment( - "alice", - author_association=MEMBER_ASSOCIATION, - ) - ], + comments=[_member_comment()], updated_at=NOW - timedelta(days=WARNING_THRESHOLD_DAYS), ) - repo = mock_github.return_value.get_repo.return_value - repo.get_pulls.return_value = [pull_request] - mock_datetime.now.return_value = NOW + main_context.repo.get_pulls.return_value = [pull_request] + mock_warn = MagicMock(return_value=False) + monkeypatch.setattr(stale_pr_ping, "warn", mock_warn) with pytest.raises(SystemExit): main() @@ -885,22 +685,10 @@ def test_failed_warning_is_reported_and_exits( assert "Scanned 1 PRs, warned 0, closed 0, failed 1" in output assert "Failed: #42" in output - @patch.dict( - os.environ, - { - "GITHUB_TOKEN": "tok", - "GITHUB_REPOSITORY": "org/repo", - }, - clear=True, - ) - @patch("stale_pr_ping.time.sleep") - @patch("stale_pr_ping.datetime") - @patch("stale_pr_ping.Github") - def test_lookup_failure_is_recorded_and_later_pr_is_processed( + def test_lookup_failure_does_not_block_later_pr( self, - mock_github, - mock_datetime, - mock_sleep, + main_context, + monkeypatch, capsys, ): failed_pull_request = _make_pull_request(number=41) @@ -910,33 +698,23 @@ def test_lookup_failure_is_recorded_and_later_pr_is_processed( None, ) later_pull_request = _make_pull_request( - comments=[ - _make_comment( - "alice", - author_association=MEMBER_ASSOCIATION, - ) - ], + comments=[_member_comment()], number=42, updated_at=NOW - timedelta(days=WARNING_THRESHOLD_DAYS), ) - repo = mock_github.return_value.get_repo.return_value - repo.get_pulls.return_value = [ + main_context.repo.get_pulls.return_value = [ failed_pull_request, later_pull_request, ] - mock_datetime.now.return_value = NOW + mock_sleep = MagicMock() + monkeypatch.setattr(stale_pr_ping.time, "sleep", mock_sleep) with pytest.raises(SystemExit): main() assert failed_pull_request.get_issue_comments.call_count == 3 assert mock_sleep.call_args_list == [call(2), call(4)] - later_pull_request.create_issue_comment.assert_called_once_with( - WARNING_COMMENT.format( - author="external", - closing_days=CLOSING_THRESHOLD_DAYS, - ) - ) + later_pull_request.create_issue_comment.assert_called_once() later_pull_request.add_to_labels.assert_called_once_with( REQUESTED_INFO_LABEL )