From c693d77e9539e2127e4242055b126c7d96a2f097 Mon Sep 17 00:00:00 2001 From: Trask Stalnaker Date: Tue, 4 Aug 2026 07:04:28 -0700 Subject: [PATCH] Stop posting a first status comment on untracked closed pull requests A `pull_request_review` webhook for a pull request that merged weeks ago still dispatches a targeted refresh. `build_pr_result` returns `None` for a closed pull request, and `merge_dashboard_update_with_latest_state` then reported a state change even when the pull request had never been in the dashboard cache, so there was nothing to drop. That queued a status comment, and `upsert_status_comment` created one, announcing "Merged" on a pull request the dashboard had never commented on. Report no change when nothing was cached for the pull request, and never create a status comment for a terminal state. A terminal status only exists to move an already published comment to its final state. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: ca21dec5-33e3-4724-b588-a70626f5435d --- .../pull-request-dashboard/dashboard.py | 8 +++ .../pr_status_comment.py | 29 +++++++++- .../pull-request-dashboard/test_dashboard.py | 45 +++++++++++++++ .../test_pr_status_comment.py | 57 +++++++++++++++++++ 4 files changed, 137 insertions(+), 2 deletions(-) diff --git a/.github/scripts/pull-request-dashboard/dashboard.py b/.github/scripts/pull-request-dashboard/dashboard.py index 919ee478b1e..a4af774fd27 100644 --- a/.github/scripts/pull-request-dashboard/dashboard.py +++ b/.github/scripts/pull-request-dashboard/dashboard.py @@ -1698,6 +1698,14 @@ def merge_dashboard_update_with_latest_state( return replace(calculation, results=results, dashboard_state=dashboard_state), True else: dashboard_state = calculation.dashboard_state + previous_pr_result = calculation.starting_pr_result + if previous_pr_result is None: + # Nothing is cached for this PR, so there is no routed result to + # drop. Reporting a change here would queue a status comment for a + # PR the dashboard never tracked, which is how an event on a + # long-merged PR ends up posting a first status comment on it. + results = results_from_dashboard_state(dashboard_state, open_pr_numbers) + return replace(calculation, results=results, dashboard_state=dashboard_state), True dashboard_state = update_dashboard_state_for_pr(dashboard_state, pr_number, None) results = results_from_dashboard_state(dashboard_state, open_pr_numbers) return replace(calculation, results=results, dashboard_state=dashboard_state), False diff --git a/.github/scripts/pull-request-dashboard/pr_status_comment.py b/.github/scripts/pull-request-dashboard/pr_status_comment.py index ce6d8b1c45e..8710c7db8dc 100644 --- a/.github/scripts/pull-request-dashboard/pr_status_comment.py +++ b/.github/scripts/pull-request-dashboard/pr_status_comment.py @@ -221,6 +221,10 @@ def author_body( return [fallback_next_step] +def is_terminal_pr(pr: dict[str, Any]) -> bool: + return bool(pr.get("merged")) or (pr.get("state") or "").lower() == "closed" + + def render_status_comment( pr: dict[str, Any], result: dict[str, Any] | None, @@ -373,7 +377,13 @@ def managed_status_comments(repo: str, pr_number: int) -> list[dict[str, Any]]: ] -def upsert_status_comment(repo: str, pr_number: int, body: str) -> None: +def upsert_status_comment( + repo: str, + pr_number: int, + body: str, + *, + create: bool = True, +) -> None: comments = managed_status_comments(repo, pr_number) if comments: comment = comments[0] @@ -396,6 +406,13 @@ def upsert_status_comment(repo: str, pr_number: int, body: str) -> None: ]) return + if not create: + print( + f"PR #{pr_number} has no status comment to update; skipping creation", + file=sys.stderr, + ) + return + print(f"creating PR #{pr_number} status comment", file=sys.stderr) run_gh([ "gh", "api", "--method", "POST", @@ -407,7 +424,15 @@ def upsert_status_comment(repo: str, pr_number: int, body: str) -> None: def publish_pr_status(repo: str, pr_number: int, dashboard_state: dict[str, Any]) -> None: pr = gh_api(f"/repos/{repo}/pulls/{pr_number}") result = (dashboard_state.get("prs") or {}).get(str(pr_number)) - upsert_status_comment(repo, pr_number, render_status_comment(pr, result)) + # A terminal status only exists to move an already published comment to its + # final state. Creating one instead would announce a merge or close on a + # pull request the dashboard never commented on. + upsert_status_comment( + repo, + pr_number, + render_status_comment(pr, result), + create=not is_terminal_pr(pr), + ) def update_targeted_status_comment_from_state(repo: str, pr_number: int) -> list[str]: diff --git a/.github/scripts/pull-request-dashboard/test_dashboard.py b/.github/scripts/pull-request-dashboard/test_dashboard.py index 6371eceafaa..8eeebd59ac7 100644 --- a/.github/scripts/pull-request-dashboard/test_dashboard.py +++ b/.github/scripts/pull-request-dashboard/test_dashboard.py @@ -22,6 +22,7 @@ group_review_threads, hold_route_until_gates_settle, main, + merge_dashboard_update_with_latest_state, remove_cached_dashboard_prs, resolve_pr_route, route_pr, @@ -1227,6 +1228,50 @@ def test_unchanged_targeted_state_does_not_enqueue_status_comment( prepare_due=False, ) + @patch( + "dashboard.load_dashboard_state_cache", + return_value={"prs": {"34": {"route": "author"}}}, + ) + def test_untracked_closed_pr_reports_no_state_change( + self, _load_state: Mock + ) -> None: + calculation = DashboardUpdate( + results={}, + dashboard_state={"prs": {"34": {"route": "author"}}}, + trigger_pr_result=None, + starting_pr_result=None, + used_cached_dashboard_state=True, + ) + + _merged, dashboard_state_unchanged = merge_dashboard_update_with_latest_state( + calculation, 12, {34} + ) + + self.assertTrue(dashboard_state_unchanged) + + @patch( + "dashboard.load_dashboard_state_cache", + return_value={"prs": {"12": {"route": "author"}}}, + ) + def test_tracked_closed_pr_still_reports_a_state_change( + self, _load_state: Mock + ) -> None: + starting_pr_result = {"route": "author"} + calculation = DashboardUpdate( + results={}, + dashboard_state={"prs": {"12": starting_pr_result}}, + trigger_pr_result=None, + starting_pr_result=starting_pr_result, + used_cached_dashboard_state=True, + ) + + merged, dashboard_state_unchanged = merge_dashboard_update_with_latest_state( + calculation, 12, set() + ) + + self.assertFalse(dashboard_state_unchanged) + self.assertEqual({}, merged.dashboard_state["prs"]) + class RequiredCiRoutingTest(unittest.TestCase): def test_non_blocking_check_failures_use_deterministic_casefold_tiebreaker(self) -> None: diff --git a/.github/scripts/pull-request-dashboard/test_pr_status_comment.py b/.github/scripts/pull-request-dashboard/test_pr_status_comment.py index d63d99991ea..7c17ed67957 100644 --- a/.github/scripts/pull-request-dashboard/test_pr_status_comment.py +++ b/.github/scripts/pull-request-dashboard/test_pr_status_comment.py @@ -590,6 +590,63 @@ def test_updates_comment_and_deletes_duplicates(self, _comments: object) -> None self.assertEqual(["PATCH", "DELETE"], [command[3] for command in self.commands]) + @patch.object(pr_status_comment, "managed_status_comments", return_value=[]) + def test_does_not_create_comment_when_creation_is_disabled( + self, _comments: object + ) -> None: + pr_status_comment.upsert_status_comment( + "open-telemetry/example", 1, "body", create=False + ) + + self.assertEqual([], self.commands) + + @patch.object( + pr_status_comment, + "managed_status_comments", + return_value=[{"id": 7, "body": " old"}], + ) + def test_still_updates_existing_comment_when_creation_is_disabled( + self, _comments: object + ) -> None: + pr_status_comment.upsert_status_comment( + "open-telemetry/example", 1, "body", create=False + ) + + self.assertEqual(["PATCH"], [command[3] for command in self.commands]) + + +class PublishPrStatusTest(unittest.TestCase): + @patch.object(pr_status_comment, "upsert_status_comment") + @patch.object(pr_status_comment, "gh_api") + def test_terminal_pr_never_creates_a_status_comment( + self, gh_api: Mock, upsert: Mock + ) -> None: + for pr in ( + {"number": 1, "state": "closed", "merged": True}, + {"number": 1, "state": "closed", "merged": False}, + ): + with self.subTest(merged=pr["merged"]): + gh_api.return_value = pr + + pr_status_comment.publish_pr_status( + "open-telemetry/example", 1, {"prs": {}} + ) + + self.assertFalse(upsert.call_args.kwargs["create"]) + + @patch.object(pr_status_comment, "upsert_status_comment") + @patch.object( + pr_status_comment, + "gh_api", + return_value={"number": 1, "state": "open", "merged": False}, + ) + def test_open_pr_still_creates_a_status_comment( + self, _gh_api: Mock, upsert: Mock + ) -> None: + pr_status_comment.publish_pr_status("open-telemetry/example", 1, {"prs": {}}) + + self.assertTrue(upsert.call_args.kwargs["create"]) + class ManagedStatusCommentsTest(unittest.TestCase): @patch.object(