diff --git a/.github/scripts/pull-request-dashboard/RATIONALE.md b/.github/scripts/pull-request-dashboard/RATIONALE.md index 9c030ba608..720db5c824 100644 --- a/.github/scripts/pull-request-dashboard/RATIONALE.md +++ b/.github/scripts/pull-request-dashboard/RATIONALE.md @@ -12,9 +12,9 @@ the implementation understandable and operationally cheap. `repositories.json`. - The top-level workflow resolves target repositories, then calls a reusable per-repository workflow for each target. The per-repository workflow separates - read-only calculation from a repository-wide publisher, so one repository's - update failure does not block delivery for repositories whose updates - succeeded. + read-only calculation from a serialized per-repository publisher, so one + repository's update failure does not block delivery for repositories whose + updates succeeded. - The top-level repository matrix runs one repository at a time. Backfills do not benefit enough from cross-repository parallelism to justify extra aggregate API and LLM demand. @@ -45,9 +45,13 @@ the implementation understandable and operationally cheap. independently, and every accepted webhook still creates a workflow run. - Publishers use one concurrency group per target repository. GitHub preserves the running publisher but may replace an older pending publisher with a newer - one even when `cancel-in-progress` is false. This is safe because accepted - work lives on the state branch and every surviving publisher drains - repository-wide work rather than serving only its triggering invocation. + one even when `cancel-in-progress` is false. Accepted work lives on the state + branch: a targeted publisher limits status-comment and Slack delivery to its + triggering PR. Webhook runs can arrive concurrently for many PRs, so allowing + each publisher to fan out into repository-wide delivery would create long + jobs and put pressure on the GitHub Actions job queue, especially when a new + status-comment revision queues every open PR. The hourly untargeted publisher + is the bounded repository-wide rollout and recovery path. - The top-level hourly health check treats a replaced pending publisher as successful. Matrix failures take precedence over cancellation, so genuine update or delivery failures still open the failure issue. @@ -90,8 +94,11 @@ the implementation understandable and operationally cheap. queue. Incrementing the implementation revision snapshots all open PRs, then hourly runs update at most 50 queued comments until the rollout completes. Dashboard refreshes atomically queue comments only when their persisted result - changes. Every publisher drains up to 50 queued comments, so a newer publisher - also delivers work left by an older pending publisher that GitHub replaced. + changes. A targeted publisher updates only its triggering PR when that PR is + queued and cannot initialize or drain the repository-wide rollout. Untargeted + publishers drain up to 50 queued comments. This confines rollout fan-out to + the hourly path instead of multiplying it across concurrent webhook runs, and + also delivers work left by a pending publisher that GitHub replaced. - Selected PRs are processed one at a time through the same single-PR merge path as targeted refreshes. Each accepted PR update pushes structured state before the next selected PR is processed. @@ -308,11 +315,11 @@ the implementation understandable and operationally cheap. - When a mapped assignee is added after a PR was already notified during the same waiting period, that assignee may wait until the next follow-up cadence instead of receiving an immediate initial notification. -- Every publisher evaluates all accepted repository state for eligible initial - and follow-up notifications. The sent-notification ledger and weekday - 24-hour follow-up cadence bound delivery. Repository-wide evaluation ensures - that a newer publisher drains Slack work whose older pending publisher GitHub - replaced. +- A targeted publisher evaluates only its triggering PR and preserves unrelated + entries in the sent-notification ledger. An untargeted publisher evaluates all + accepted repository state for eligible initial and follow-up notifications, + providing the recovery path for Slack work whose pending publisher GitHub + replaced. The ledger and weekday 24-hour follow-up cadence bound delivery. - Slack notifications are sent only for dashboard state that has already been accepted on the state branch. A newer dashboard update can land after the publisher checks out state, so a notification can be slightly late @@ -328,11 +335,12 @@ the implementation understandable and operationally cheap. - Dashboard publishing is serialized per target repository. The publisher owns target-repository writes for status comments, author reminders, Copilot re-review requests, Slack notifications, and the dashboard issue. -- Each publisher fetches accepted state while holding the publish slot and - drains repository-wide pending work. Status comments are bounded to 50 per - publisher; author reminders and Copilot requests use explicit durable - ledgers; Slack eligibility is reconstructed from accepted dashboard and - notification state. +- Each publisher fetches accepted state while holding the publish slot. A + targeted publisher limits status-comment and Slack delivery to its triggering + PR; an untargeted publisher drains repository-wide work, with status comments + bounded to 50 per run. Author reminders and Copilot requests use explicit + durable ledgers; Slack eligibility is reconstructed from accepted dashboard + and notification state. - The dashboard issue is rendered from `dashboard-state.json` and the target repository's current open PR list after delivery. If another update advances the state branch while a publisher is already working, external views can diff --git a/.github/scripts/pull-request-dashboard/delivery.py b/.github/scripts/pull-request-dashboard/delivery.py index 131f0ad8bd..dce60ad3ec 100644 --- a/.github/scripts/pull-request-dashboard/delivery.py +++ b/.github/scripts/pull-request-dashboard/delivery.py @@ -16,9 +16,12 @@ deliver_dashboard_command_replies, deliver_dashboard_override_requests, ) -from github_cli import detect_repo, list_open_prs, normalize_repo, repo_state_key +from github_cli import detect_repo, gh_api, list_open_prs, normalize_repo, repo_state_key from notify_slack import notify_slack_from_state -from pr_status_comment import update_status_comments_from_state +from pr_status_comment import ( + update_status_comments_from_state, + update_targeted_status_comment_from_state, +) from state import ( author_nudge_state_path, copilot_review_request_state_path, @@ -53,11 +56,20 @@ def deliver_from_state( author_retry_snapshot_path: Path, copilot_retry_snapshot_path: Path, notification_retry_snapshot_path: Path, + pr_number: int | None = None, ) -> list[str]: now = utc_now() errors: list[str] = [] try: - open_prs = list_open_prs(repo) + if pr_number is None: + open_prs = list_open_prs(repo) + else: + pr = gh_api(f"/repos/{repo}/pulls/{pr_number}") + open_prs = ( + [{"number": pr_number, "isDraft": bool(pr.get("draft")), "title": pr.get("title") or ""}] + if pr.get("state") == "open" + else [] + ) except Exception as e: errors.append(f"open pull requests: {e}") open_prs = None @@ -80,7 +92,13 @@ def deliver_from_state( ), errors, ) - if open_prs is not None: + if pr_number is not None: + run_delivery_action( + "status comments", + lambda: update_targeted_status_comment_from_state(repo, pr_number), + errors, + ) + elif open_prs is not None: run_delivery_action( "status comments", lambda: update_status_comments_from_state( @@ -97,11 +115,21 @@ def deliver_from_state( if open_prs is not None: run_delivery_action( "Slack notifications", - lambda: notify_slack_from_state( - repo, - notification_retry_snapshot_path, - open_prs, - now, + lambda: ( + notify_slack_from_state( + repo, + notification_retry_snapshot_path, + open_prs, + now, + {pr_number}, + ) + if pr_number is not None + else notify_slack_from_state( + repo, + notification_retry_snapshot_path, + open_prs, + now, + ) ), errors, ) @@ -112,6 +140,7 @@ def deliver_with_state( repo: str, state_branch_name: str, state_dir: Path, + pr_number: int | None = None, ) -> int: repo_key = repo_state_key(repo) author_retry = runner_temp_path("prior-author-nudge-state.json") @@ -125,6 +154,7 @@ def deliver() -> int: author_retry, copilot_retry, notification_retry, + pr_number, ) return 0 @@ -152,6 +182,7 @@ def deliver() -> int: def main() -> int: parser = argparse.ArgumentParser(description=__doc__) parser.add_argument("--repo", help="target repository name") + parser.add_argument("--pr-number", type=int, help="target pull request number") parser.add_argument("--state-branch", required=True, help="git branch used for workflow state") args = parser.parse_args() repo = normalize_repo(args.repo) if args.repo else detect_repo() @@ -161,6 +192,7 @@ def main() -> int: repo, args.state_branch, state_dir, + args.pr_number, ) diff --git a/.github/scripts/pull-request-dashboard/notify_slack.py b/.github/scripts/pull-request-dashboard/notify_slack.py index d0bd319382..9dea455b77 100644 --- a/.github/scripts/pull-request-dashboard/notify_slack.py +++ b/.github/scripts/pull-request-dashboard/notify_slack.py @@ -41,6 +41,7 @@ def notify_slack_from_state( retry_snapshot_path: Path | None, open_prs: list[dict[str, Any]], now: datetime, + target_pr_numbers: set[int] | None = None, ) -> list[str]: dashboard_state = load_dashboard_state_cache() if dashboard_state is None: @@ -48,19 +49,37 @@ def notify_slack_from_state( return [] open_pr_numbers = {p["number"] for p in open_prs if not p.get("isDraft")} + if target_pr_numbers is not None: + open_pr_numbers &= target_pr_numbers results = results_from_dashboard_state(dashboard_state, open_pr_numbers) current_prs = {p["number"]: p for p in open_prs} for number, result in results.items(): result["pr_title"] = current_prs.get(number, {}).get("title") or "" saved_notifications = load_notifications() + last_notification_state = last_notifications(saved_notifications, retry_snapshot_path) + if target_pr_numbers is not None and last_notification_state is not None: + target_pr_keys = {str(number) for number in target_pr_numbers} + last_notification_state = { + str(number): notification + for number, notification in last_notification_state.items() + if str(number) in target_pr_keys + } updated_notifications, delivery_errors = next_notifications( repo, results, - last_notifications(saved_notifications, retry_snapshot_path), + last_notification_state, now, ) + if target_pr_numbers is not None: + merged_notifications = { + number: notification + for number, notification in (saved_notifications or {}).items() + if str(number) not in target_pr_keys + } + merged_notifications.update(updated_notifications) + updated_notifications = merged_notifications notifications_changed = updated_notifications != (saved_notifications or {}) if not notifications_changed and saved_notifications is not None: print("notifications unchanged", file=sys.stderr) diff --git a/.github/scripts/pull-request-dashboard/pr_status_comment.py b/.github/scripts/pull-request-dashboard/pr_status_comment.py index 62b1dd6ff9..6cf3770b1c 100644 --- a/.github/scripts/pull-request-dashboard/pr_status_comment.py +++ b/.github/scripts/pull-request-dashboard/pr_status_comment.py @@ -364,6 +364,29 @@ def publish_pr_status(repo: str, pr_number: int, dashboard_state: dict[str, Any] upsert_status_comment(repo, pr_number, render_status_comment(pr, result)) +def update_targeted_status_comment_from_state(repo: str, pr_number: int) -> list[str]: + dashboard_state = load_dashboard_state_cache() + if dashboard_state is None: + print("dashboard result state not found; skipping PR status comment", file=sys.stderr) + return [] + + rollout_state = load_status_comment_rollout_state() + pending_pr_numbers = set(rollout_state.get("pending_pr_numbers") or []) + if pr_number not in pending_pr_numbers: + return [] + try: + publish_pr_status(repo, pr_number, dashboard_state) + except Exception as e: + return [f"PR #{pr_number}: {e}"] + + pending_pr_numbers.remove(pr_number) + rollout_state["pending_pr_numbers"] = sorted(pending_pr_numbers) + if not pending_pr_numbers: + rollout_state["completed_revision"] = rollout_state["target_revision"] + save_status_comment_rollout_state(rollout_state) + return [] + + def prepare_rollout_state( rollout_state: dict[str, Any], open_pr_numbers: set[int], diff --git a/.github/scripts/pull-request-dashboard/test_delivery.py b/.github/scripts/pull-request-dashboard/test_delivery.py index c646b8d1ad..7ee2186572 100644 --- a/.github/scripts/pull-request-dashboard/test_delivery.py +++ b/.github/scripts/pull-request-dashboard/test_delivery.py @@ -131,6 +131,47 @@ def test_open_pr_list_failure_skips_dependent_stages(self) -> None: status.assert_not_called() slack.assert_not_called() + def test_targeted_delivery_only_processes_triggering_pr(self) -> None: + with ( + patch.object(delivery, "list_open_prs") as list_open, + patch.object( + delivery, + "gh_api", + return_value={"state": "open", "draft": False, "title": "Seven"}, + ) as gh_api, + patch.object(delivery, "deliver_dashboard_override_requests", return_value=[]), + patch.object(delivery, "deliver_dashboard_command_replies", return_value=[]), + patch.object(delivery, "deliver_prepared_author_nudges", return_value=[]), + patch.object(delivery, "update_status_comments_from_state") as bulk_status, + patch.object( + delivery, + "update_targeted_status_comment_from_state", + return_value=[], + ) as targeted_status, + patch.object(delivery, "deliver_copilot_review_requests", return_value=[]), + patch.object(delivery, "notify_slack_from_state", return_value=[]) as slack, + ): + errors = delivery.deliver_from_state( + "open-telemetry/example", + Path("author"), + Path("copilot"), + Path("slack"), + 7, + ) + + self.assertEqual([], errors) + list_open.assert_not_called() + gh_api.assert_called_once_with("/repos/open-telemetry/example/pulls/7") + bulk_status.assert_not_called() + targeted_status.assert_called_once_with("open-telemetry/example", 7) + slack.assert_called_once_with( + "open-telemetry/example", + ANY, + [{"number": 7, "isDraft": False, "title": "Seven"}], + ANY, + {7}, + ) + @patch.object(delivery.sys, "stderr") @patch.object(delivery, "deliver_from_state", return_value=["status comments: boom"]) @patch.object(delivery.state_branch, "push_state_changes") diff --git a/.github/scripts/pull-request-dashboard/test_notify_slack.py b/.github/scripts/pull-request-dashboard/test_notify_slack.py index 53901f7bdc..93165363e7 100644 --- a/.github/scripts/pull-request-dashboard/test_notify_slack.py +++ b/.github/scripts/pull-request-dashboard/test_notify_slack.py @@ -97,6 +97,127 @@ def test_uncached_pr_does_not_pause_notifications_and_closed_state_is_pruned( } ) + @patch("notify_slack.save_notifications") + @patch("notify_slack.load_notifications") + @patch("notify_slack.load_dashboard_state_cache") + def test_targeted_update_preserves_unrelated_notification_state( + self, + load_dashboard_state_cache, + load_notifications, + save_notifications, + ) -> None: + load_dashboard_state_cache.return_value = { + "prs": {"2": {"pr_number": 2, "route": "author"}} + } + load_notifications.return_value = { + "2": { + "last_notified_at": "2026-07-14T03:00:00Z", + "last_notification_kind": "initial", + }, + "unrelated": { + "last_notified_at": "2026-07-15T03:00:00Z", + "last_notification_kind": "follow-up", + }, + } + + with patch.dict("os.environ", {"SLACK_CHANNEL": "dashboard"}, clear=True): + errors = notify_slack_from_state( + "owner/repo", + None, + [{"number": 2, "isDraft": False, "title": "Open PR"}], + datetime(2026, 7, 20, 2, tzinfo=timezone.utc), + {2}, + ) + + self.assertEqual(errors, []) + save_notifications.assert_called_once_with( + { + "unrelated": { + "last_notified_at": "2026-07-15T03:00:00Z", + "last_notification_kind": "follow-up", + }, + } + ) + + @patch("notify_slack.next_notifications", return_value=({}, [])) + @patch("notify_slack.save_notifications") + @patch("notify_slack.load_notifications", return_value={}) + @patch("notify_slack.load_dashboard_state_cache") + def test_targeted_update_filters_dashboard_results( + self, + load_dashboard_state_cache, + _load_notifications, + _save_notifications, + next_notifications, + ) -> None: + load_dashboard_state_cache.return_value = { + "prs": { + "2": {"pr_number": 2, "route": "author"}, + "3": {"pr_number": 3, "route": "approver"}, + } + } + + with patch.dict("os.environ", {"SLACK_CHANNEL": "dashboard"}, clear=True): + errors = notify_slack_from_state( + "owner/repo", + None, + [ + {"number": 2, "isDraft": False, "title": "Target PR"}, + {"number": 3, "isDraft": False, "title": "Unrelated PR"}, + ], + datetime(2026, 7, 20, 2, tzinfo=timezone.utc), + {2}, + ) + + self.assertEqual(errors, []) + results = next_notifications.call_args.args[1] + self.assertEqual({2}, set(results)) + + @patch("notifications.send_slack_notification") + @patch("notify_slack.save_notifications") + @patch("notify_slack.load_notifications") + @patch("notify_slack.load_dashboard_state_cache") + def test_targeted_update_preserves_uninitialized_notification_state( + self, + load_dashboard_state_cache, + load_notifications, + save_notifications, + send_notification, + ) -> None: + load_dashboard_state_cache.return_value = { + "prs": { + "2": { + "pr_number": 2, + "route": "approver", + "facts": { + "reviewers": [{"login": "reviewer"}], + "waiting_since": "2026-07-20T01:00:00Z", + }, + } + } + } + load_notifications.return_value = None + + with patch.dict( + "os.environ", + { + "SLACK_CHANNEL": "dashboard", + "SLACK_USER_MAP_JSON": '{"reviewer": "U123"}', + }, + clear=True, + ): + errors = notify_slack_from_state( + "owner/repo", + None, + [{"number": 2, "isDraft": False, "title": "Open PR"}], + datetime(2026, 7, 20, 2, tzinfo=timezone.utc), + {2}, + ) + + self.assertEqual(errors, []) + send_notification.assert_not_called() + save_notifications.assert_called_once_with({}) + if __name__ == "__main__": unittest.main() \ No newline at end of file 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 ca1771c555..35883fee24 100644 --- a/.github/scripts/pull-request-dashboard/test_pr_status_comment.py +++ b/.github/scripts/pull-request-dashboard/test_pr_status_comment.py @@ -609,6 +609,66 @@ def test_requires_dashboard_app_identity_and_marker(self, _gh_api: object) -> No class RolloutStateTest(unittest.TestCase): + @patch.object(pr_status_comment, "save_status_comment_rollout_state") + @patch.object(pr_status_comment, "publish_pr_status") + @patch.object(pr_status_comment, "load_dashboard_state_cache", return_value={"prs": {}}) + @patch.object( + pr_status_comment, + "load_status_comment_rollout_state", + return_value={ + "target_revision": 0, + "completed_revision": 0, + "pending_pr_numbers": [12, 34], + }, + ) + def test_targeted_update_only_drains_triggering_pr( + self, + _load_rollout: object, + _load_dashboard: object, + publish_pr_status: Mock, + save_rollout: Mock, + ) -> None: + status = pr_status_comment.update_targeted_status_comment_from_state( + "open-telemetry/example", + 34, + ) + + self.assertEqual([], status) + publish_pr_status.assert_called_once_with("open-telemetry/example", 34, {"prs": {}}) + saved_state = save_rollout.call_args.args[0] + self.assertEqual(0, saved_state["target_revision"]) + self.assertEqual([12], saved_state["pending_pr_numbers"]) + + @patch.object(pr_status_comment, "save_status_comment_rollout_state") + @patch.object(pr_status_comment, "publish_pr_status") + @patch.object(pr_status_comment, "load_dashboard_state_cache", return_value={"prs": {}}) + @patch.object( + pr_status_comment, + "load_status_comment_rollout_state", + return_value={ + "target_revision": 12, + "completed_revision": 11, + "pending_pr_numbers": [34], + }, + ) + def test_targeted_update_completes_drained_rollout( + self, + _load_rollout: object, + _load_dashboard: object, + _publish_pr_status: Mock, + save_rollout: Mock, + ) -> None: + status = pr_status_comment.update_targeted_status_comment_from_state( + "open-telemetry/example", + 34, + ) + + self.assertEqual([], status) + saved_state = save_rollout.call_args.args[0] + self.assertEqual(12, saved_state["target_revision"]) + self.assertEqual(12, saved_state["completed_revision"]) + self.assertEqual([], saved_state["pending_pr_numbers"]) + def test_new_revision_queues_every_open_pr(self) -> None: state = pr_status_comment.prepare_rollout_state( { diff --git a/.github/workflows/pull-request-dashboard-repo.yml b/.github/workflows/pull-request-dashboard-repo.yml index 687b24841e..c0f2d5c4bc 100644 --- a/.github/workflows/pull-request-dashboard-repo.yml +++ b/.github/workflows/pull-request-dashboard-repo.yml @@ -216,10 +216,14 @@ jobs: SLACK_CHANNEL: ${{ inputs.slack_channel }} SLACK_USER_MAP_JSON: ${{ inputs.slack_user_mapping_json }} REPO_NAME: ${{ inputs.repository }} + TRIGGER_PR_NUMBER: ${{ inputs.pr_number }} run: | set -euo pipefail state_branch="${DASHBOARD_STATE_BRANCH_PREFIX}/${REPO_NAME}" delivery_args=(--state-branch "$state_branch" --repo "$REPO_NAME") + if [[ -n "${TRIGGER_PR_NUMBER:-}" ]]; then + delivery_args+=(--pr-number "$TRIGGER_PR_NUMBER") + fi python3 .github/scripts/pull-request-dashboard/delivery.py "${delivery_args[@]}" - name: Publish dashboard issue