From 285ed428c855242c4854f9ef06d33c213ea8e175 Mon Sep 17 00:00:00 2001 From: Trask Stalnaker Date: Thu, 30 Jul 2026 08:00:51 -0700 Subject: [PATCH 01/16] Do not hand a PR off to reviewers before its required checks report --- .../pull-request-dashboard/RATIONALE.md | 36 ++++ .../pull-request-dashboard/copilot_review.py | 28 ++++ .../pull-request-dashboard/dashboard.py | 45 ++++- .../pull-request-dashboard/github_cli.py | 41 +++++ .../scripts/pull-request-dashboard/state.py | 4 +- .../test_author_nudge.py | 1 + .../test_copilot_review.py | 55 +++++- .../pull-request-dashboard/test_dashboard.py | 156 ++++++++++++++++++ .../pull-request-dashboard/test_github_cli.py | 96 ++++++++++- .../pull-request-dashboard/test_state.py | 4 +- 10 files changed, 453 insertions(+), 13 deletions(-) diff --git a/.github/scripts/pull-request-dashboard/RATIONALE.md b/.github/scripts/pull-request-dashboard/RATIONALE.md index 486e1027a53..45b0e6b296b 100644 --- a/.github/scripts/pull-request-dashboard/RATIONALE.md +++ b/.github/scripts/pull-request-dashboard/RATIONALE.md @@ -172,6 +172,20 @@ the implementation understandable and operationally cheap. - Classic branch-protection required status checks are not discovered when they have not reported. This is an accepted limitation because configured OpenTelemetry repositories use rulesets for required status checks. +- The rollup is read from the PR's last commit, which can still be the previous + head just after a push. That commit's checks are already complete, so they + would read as a settled result for code that is no longer proposed. The + rollup therefore carries its commit oid and is discarded when it does not + match the PR head, leaving check facts unavailable until the rollup catches + up. +- A required context is only pending while the app that owns it may still + report. Check suites for the head commit are consulted, and a context is + dropped from the pending set once every suite its app created has completed, + because the app has then reported everything it is going to. Without this a + ruleset context that no workflow produces — an obsolete or conditionally + skipped job — would be reported as permanently pending. Such a PR cannot + merge either way; the difference is that the dashboard stops treating it as + "still running." - A `code_scanning` ruleset rule holds the merge on a check that the code scanning app publishes per configured tool, named after that tool, which GitHub never marks as required. Those checks are matched by app and by the @@ -186,6 +200,18 @@ the implementation understandable and operationally cheap. Repository-configured `non_blocking_check_patterns` identify failed optional checks in a note alongside this action, without changing required-check facts or routing. +- A PR already routed to its author stays there until the required checks + report again. A push clears the failing count before the replacement checks + produce a result, so the PR would otherwise be handed off on evidence that + does not exist yet and handed back minutes later when the same check fails. + Only the author route is held, because it is the only one a check result can + restore; a PR moving between reviewer and maintainer routes is not waiting on + CI. Unavailable check results hold the route for the same reason a pending + one does, and resolve on a later run. +- A held route also holds its wait age. Recomputing it would read the push as + the end of the CI failure and fall back to the last approver activity, which + is usually far older, so a PR the author had just pushed to would sort to the + top of the waiting-on-authors section as the stalest item on the board. - Maintenance-bot PRs retain maintainer-oriented routing because the bot cannot respond to a dashboard action. Pending required checks affect the CI column but do not change who owns the next action. @@ -214,6 +240,16 @@ the implementation understandable and operationally cheap. gate or produces fresh actionable threads that route the PR back to the author, so re-requesting an unchanged commit is self-correcting rather than a re-request loop. +- The gate only fires once the required checks have settled. A route computed + while checks are still running is provisional: a failure that has not + completed yet cannot route the PR to its author, so the PR looks ready for + reviewers and the gate would spend a Copilot review on code CI is about to + reject. Unavailable check results are treated the same as running ones, + because both mean the routing decision cannot be trusted yet. +- Delivery re-validates the required checks against live data rather than + relying on the routing fingerprint alone. The fingerprint only detects + change, so checks that were unsettled when the request was recorded and are + still unsettled at delivery would otherwise pass through unnoticed. - "Clean" means no inline comments on the current head, counted from the review, not from the classifier's actionability judgment. Accepted limitation: if Copilot leaves comments the classifier treats as diff --git a/.github/scripts/pull-request-dashboard/copilot_review.py b/.github/scripts/pull-request-dashboard/copilot_review.py index 61363daca7d..8974c8b684c 100644 --- a/.github/scripts/pull-request-dashboard/copilot_review.py +++ b/.github/scripts/pull-request-dashboard/copilot_review.py @@ -58,6 +58,14 @@ def copilot_review_status( return True, bool(latest_review.get("finding_count")) +def required_checks_settled(facts: dict[str, Any]) -> bool: + # A route computed while checks are still running is provisional: route_pr + # can only see a failure once the check has completed. + if "ci_pending_count" not in facts: + return False + return not facts.get("ci_pending_count", 0) + + def apply_copilot_review_gate( facts: dict[str, Any], route: str, @@ -67,6 +75,8 @@ def apply_copilot_review_gate( facts["copilot_review_request_needed"] = False if not enabled or route not in ("approver", "maintainer"): return route + if not required_checks_settled(facts): + return route if not facts.get("copilot_review_exists"): return "copilot" if not facts.get("copilot_review_needed"): @@ -105,6 +115,13 @@ def record_copilot_review_observation( save_copilot_review_requests(requests) +def named_checks(checks: list[dict[str, Any]]) -> str: + names = sorted(check.get("name") or "" for check in checks) + if len(names) > 3: + return f"{', '.join(names[:3])} and {len(names) - 3} more" + return ", ".join(names) + + def stale_request_reason( entry: dict[str, Any], pr: dict[str, Any], @@ -121,6 +138,17 @@ def stale_request_reason( f"head is {current_head or '(missing)'} " f"but {entry.get('head_sha') or '(missing)'} was observed" ) + # The fingerprint below only detects change, so checks that were unsettled + # when the request was recorded and still are would otherwise pass through. + checks = raw.get("checks") + if checks is None: + return "required check results are unavailable" + failing = [check for check in checks if check.get("bucket") in ("fail", "cancel")] + if failing: + return f"required checks are failing: {named_checks(failing)}" + pending = [check for check in checks if check.get("bucket") == "pending"] + if pending: + return f"required checks have not completed: {named_checks(pending)}" if not entry.get("routing_input_fingerprint"): return "no routing fingerprint was observed" if current_routing_fingerprint != entry["routing_input_fingerprint"]: diff --git a/.github/scripts/pull-request-dashboard/dashboard.py b/.github/scripts/pull-request-dashboard/dashboard.py index ec0f4c2d6ea..ac78fe5cdf6 100644 --- a/.github/scripts/pull-request-dashboard/dashboard.py +++ b/.github/scripts/pull-request-dashboard/dashboard.py @@ -111,7 +111,9 @@ current required failures. ci_pending_count int Merge-blocking checks only; absent when checks could not be - fetched. + fetched, and excludes required + contexts whose app has already + finished reporting. conflicts str "yes" | "no" | "unknown". created_at str (iso) last_activity_at str (iso) Latest substantive activity by a @@ -121,9 +123,14 @@ last_approver_activity_at str (iso) Stage 2 — add_wait_age_facts (depends on routing + pending actions): + author_route_held_for_checks bool Author route retained because + the required checks have not + reported since a push. waiting_since str (iso) Oldest pending discussion, or route-appropriate fallback, - or PR creation time. + or PR creation time. Carried + forward while the author route + is held. waiting_age_basis str Which heuristic chose waiting_since. author_action_review_thread_urls @@ -194,6 +201,7 @@ copilot_review_status, is_copilot_reviewer, record_copilot_review_observation, + required_checks_settled, ) from dashboard_override import ( apply_dashboard_override, @@ -1162,7 +1170,15 @@ def add_wait_age_facts( facts: dict[str, Any], route: str, pending_actions: dict[str, dict[str, Any]], + previous_result: dict[str, Any] | None = None, ) -> None: + previous_facts = (previous_result or {}).get("facts") or {} + # A held route was not re-evaluated, so its wait continues uninterrupted + # rather than restarting from whatever the incomplete facts now imply. + if facts.get("author_route_held_for_checks") and previous_facts.get("waiting_since"): + facts["waiting_since"] = previous_facts["waiting_since"] + facts["waiting_age_basis"] = previous_facts.get("waiting_age_basis") or "" + return actions = ROUTE_DISCUSSION_ACTIONS.get(route) wait_ts = oldest_pending_action_ts(pending_actions, actions) if actions else None basis = "oldest_pending_thread" if wait_ts else "" @@ -1282,11 +1298,27 @@ def add_reviewers( ] +def hold_author_route_until_checks_report( + facts: dict[str, Any], + route: str, + previous_result: dict[str, Any] | None, +) -> str: + # A push clears the failing count before the replacement checks report, so + # the author would otherwise hand the PR off on evidence that does not exist + # yet. Routes that do not depend on a check result move freely. + held = (previous_result or {}).get( + "route" + ) == "author" and not required_checks_settled(facts) + facts["author_route_held_for_checks"] = held + return "author" if held else route + + def resolve_pr_route( facts: dict[str, Any], pending_actions: dict[str, dict[str, Any]], required_approvals: int, require_clean_copilot_review: bool, + previous_result: dict[str, Any] | None = None, ) -> str: # Apply the manual reviewer-routing override before the Copilot review gate # so an overridden route (for example author -> reviewers) is still held for @@ -1295,7 +1327,11 @@ def resolve_pr_route( facts, apply_dashboard_override( facts, - route_pr(facts, pending_actions, required_approvals), + hold_author_route_until_checks_report( + facts, + route_pr(facts, pending_actions, required_approvals), + previous_result, + ), ), enabled=require_clean_copilot_review, ) @@ -1426,6 +1462,7 @@ def build_pr_result( pending_actions, required_approvals, require_clean_copilot_review, + previous_result, ) assign_author_nudge_episode( facts, @@ -1434,7 +1471,7 @@ def build_pr_result( raw.get("issue_comments") or [], ) append_route_noop_reply(raw, facts, route) - add_wait_age_facts(facts, route, pending_actions) + add_wait_age_facts(facts, route, pending_actions, previous_result) facts["author_action_review_thread_urls"] = author_action_discussion_urls( review_threads, pending_actions ) diff --git a/.github/scripts/pull-request-dashboard/github_cli.py b/.github/scripts/pull-request-dashboard/github_cli.py index 2d6a5224828..0e843ceafe4 100644 --- a/.github/scripts/pull-request-dashboard/github_cli.py +++ b/.github/scripts/pull-request-dashboard/github_cli.py @@ -308,6 +308,7 @@ def fetch_pr_reviews(owner: str, repo_name: str, number: int) -> list[dict[str, commits(last: 1) { nodes { commit { + oid statusCheckRollup { contexts(first: 100, after: $after) { nodes { @@ -415,6 +416,7 @@ def gh_pr_check_rollup( ] = {} code_scanning_by_identity: dict[tuple[str, int | None], dict[str, Any]] = {} after: str | None = None + head_oid = "" try: while True: data = gh_graphql(PR_CHECKS_QUERY, {"id": pr_id, "after": after}) @@ -422,6 +424,7 @@ def gh_pr_check_rollup( commits = pull_request.get("commits") or {} commit_nodes = commits.get("nodes") or [] commit = (commit_nodes[0] if commit_nodes else {}).get("commit") or {} + head_oid = commit.get("oid") or head_oid rollup = commit.get("statusCheckRollup") or {} contexts = rollup.get("contexts") or {} for node in contexts.get("nodes") or []: @@ -458,6 +461,7 @@ def gh_pr_check_rollup( return None checks = list(checks_by_identity.values()) return { + "head_oid": head_oid, "required": [check for check, is_required in checks if is_required], "non_blocking_failures": [ check @@ -553,9 +557,31 @@ def merge_code_scanning_checks( ] + code_scanning_checks +def settled_check_suite_app_ids(repo: str, head_sha: str) -> set[int]: + # An app is settled once every check suite it created for this head has + # completed, which means it has reported every context it is going to. + if not head_sha: + return set() + try: + payload = gh_api( + f"/repos/{repo}/commits/{head_sha}/check-suites?per_page=100" + ) + except RuntimeError: + return set() + settled: dict[int, bool] = {} + for suite in (payload or {}).get("check_suites") or []: + app_id = (suite.get("app") or {}).get("id") + if app_id is None: + continue + completed = suite.get("status") == "completed" + settled[app_id] = settled.get(app_id, True) and completed + return {app_id for app_id, completed in settled.items() if completed} + + def include_missing_required_checks( checks: list[dict[str, Any]] | None, required_contexts: list[dict[str, Any]] | None, + settled_app_ids: set[int] | None = None, ) -> list[dict[str, Any]] | None: if checks is None or required_contexts is None: return None @@ -582,6 +608,9 @@ def include_missing_required_checks( ) == 1 if reported: continue + # A settled app will never report this context, so it cannot be pending. + if integration_id is not None and integration_id in (settled_app_ids or set()): + continue complete.append({ "name": context, "state": "EXPECTED", @@ -888,11 +917,23 @@ def fetch_pr_routing_raw( repo, pr.get("baseRefName") or "", ) + settled_app_ids_future = pool.submit( + settled_check_suite_app_ids, + repo, + pr.get("headRefOid") or "", + ) check_rollup = check_rollup_future.result() branch_rules = branch_rules_future.result() + # commits(last: 1) can lag headRefOid, and the previous head's checks + # are already complete, so a mismatch has to read as no check data. + if check_rollup is not None and check_rollup["head_oid"] != ( + pr.get("headRefOid") or "" + ): + check_rollup = None checks = include_missing_required_checks( None if check_rollup is None else check_rollup["required"], required_check_contexts(branch_rules), + settled_app_ids_future.result(), ) if checks is not None and check_rollup is not None: checks = merge_code_scanning_checks( diff --git a/.github/scripts/pull-request-dashboard/state.py b/.github/scripts/pull-request-dashboard/state.py index d386fd59e06..e1095a33c23 100644 --- a/.github/scripts/pull-request-dashboard/state.py +++ b/.github/scripts/pull-request-dashboard/state.py @@ -23,7 +23,7 @@ # current vector, ordinary state loaders may regenerate mismatched disposable # caches. Every constant ending in _STATE_VERSION or _REVISION is included. # dashboard-state.json: accepted PR routing results and backfill readiness. -DASHBOARD_STATE_VERSION = 6 +DASHBOARD_STATE_VERSION = 7 # backfill-state.json: round-robin cursor used by full dashboard refreshes. BACKFILL_STATE_VERSION = 3 # notification-state.json: pending and delivered Slack notification records. @@ -31,7 +31,7 @@ # author-nudge-state.json: waiting episodes and delivered author reminders. AUTHOR_NUDGE_STATE_VERSION = 2 # copilot-review-request-state.json: pending and delivered review requests. -COPILOT_REVIEW_REQUEST_STATE_VERSION = 3 +COPILOT_REVIEW_REQUEST_STATE_VERSION = 4 # status-comment-rollout-state.json: target/completed renderer revisions and queue. STATUS_COMMENT_ROLLOUT_STATE_VERSION = 1 # Rendered status-comment behavior. Increment when existing comments need to diff --git a/.github/scripts/pull-request-dashboard/test_author_nudge.py b/.github/scripts/pull-request-dashboard/test_author_nudge.py index eabb2ecad48..a9b598b09ab 100644 --- a/.github/scripts/pull-request-dashboard/test_author_nudge.py +++ b/.github/scripts/pull-request-dashboard/test_author_nudge.py @@ -113,6 +113,7 @@ def test_routing_fingerprint_tracks_base_branch(self) -> None: @patch( "github_cli.gh_pr_check_rollup", return_value={ + "head_oid": "current-head", "required": [{"name": "build", "bucket": "fail"}], "non_blocking_failures": [], "code_scanning": [], diff --git a/.github/scripts/pull-request-dashboard/test_copilot_review.py b/.github/scripts/pull-request-dashboard/test_copilot_review.py index 8b04aec909e..10df27964c2 100644 --- a/.github/scripts/pull-request-dashboard/test_copilot_review.py +++ b/.github/scripts/pull-request-dashboard/test_copilot_review.py @@ -184,7 +184,7 @@ def test_delivers_request_for_current_stale_review( "headRefOid": "current-head", "id": "PR_node_id", } - fetch_current_state.return_value = (pr, {}) + fetch_current_state.return_value = (pr, {"checks": []}) fetch_reviews.return_value = [{ "id": 20, "commit_id": "reviewed-head", @@ -247,6 +247,7 @@ def test_pending_request_is_acknowledged_from_pull_response( fetch_current_state.return_value = ( pr, { + "checks": [], "review_requests": [ { "__typename": "Bot", @@ -288,7 +289,7 @@ def test_pending_request_is_acknowledged_from_pull_response( "isDraft": False, "headRefOid": "current-head", }, - {}, + {"checks": []}, ), ) @patch("copilot_review.save_copilot_review_requests") @@ -356,7 +357,7 @@ def test_drops_request_when_live_routing_inputs_changed( "isDraft": False, "headRefOid": "current-head", }, - {}, + {"checks": []}, ), ) @patch("copilot_review.save_copilot_review_requests") @@ -411,13 +412,14 @@ def reason( pr: dict | None = None, current_head: str = "current-head", current_routing_fingerprint: str = "accepted-fingerprint", + raw: dict | None = None, ) -> str: return stale_request_reason( self.ENTRY if entry is None else entry, self.OPEN_PR if pr is None else pr, current_head, current_routing_fingerprint, - {}, + {"checks": []} if raw is None else raw, ) def test_current_request_is_not_stale(self) -> None: @@ -447,6 +449,51 @@ def test_reports_missing_observed_fingerprint(self) -> None: self.reason(entry={"head_sha": "current-head"}), ) + def test_reports_unavailable_check_results(self) -> None: + self.assertEqual( + "required check results are unavailable", + self.reason(raw={}), + ) + + def test_reports_failing_required_checks(self) -> None: + self.assertEqual( + "required checks are failing: build", + self.reason( + raw={ + "checks": [ + {"name": "build", "bucket": "fail"}, + {"name": "lint", "bucket": "pass"}, + ], + }, + ), + ) + + def test_reports_pending_required_checks(self) -> None: + self.assertEqual( + "required checks have not completed: build", + self.reason( + raw={ + "checks": [ + {"name": "build", "bucket": "pending"}, + {"name": "lint", "bucket": "pass"}, + ], + }, + ), + ) + + def test_summarizes_long_lists_of_unsettled_checks(self) -> None: + self.assertEqual( + "required checks have not completed: a, b, c and 2 more", + self.reason( + raw={ + "checks": [ + {"name": name, "bucket": "pending"} + for name in ("e", "d", "c", "b", "a") + ], + }, + ), + ) + def test_fingerprint_mismatch_reports_component_digests(self) -> None: reason = self.reason(current_routing_fingerprint="new-fingerprint") diff --git a/.github/scripts/pull-request-dashboard/test_dashboard.py b/.github/scripts/pull-request-dashboard/test_dashboard.py index 7a22edec160..60b5a98d962 100644 --- a/.github/scripts/pull-request-dashboard/test_dashboard.py +++ b/.github/scripts/pull-request-dashboard/test_dashboard.py @@ -20,6 +20,7 @@ compute_facts, fetch_pr_raw, group_review_threads, + hold_author_route_until_checks_report, main, remove_cached_dashboard_prs, resolve_pr_route, @@ -35,6 +36,7 @@ def _author_route_facts(self, **overrides: object) -> dict[str, object]: # A failing required check routes a human-authored PR to the author. facts: dict[str, object] = { "ci_failing_count": 1, + "ci_pending_count": 0, "dashboard_override_label_applied": True, "dashboard_override_requested": False, } @@ -69,6 +71,103 @@ def test_override_reaches_reviewers_when_gate_disabled(self) -> None: self.assertEqual("approver", route) + def test_override_reaches_reviewers_while_checks_are_running(self) -> None: + facts = self._author_route_facts(ci_failing_count=0, ci_pending_count=1) + + route = resolve_pr_route(facts, {}, 1, False, {"route": "author"}) + + self.assertEqual("approver", route) + + +class AuthorRouteHoldTest(unittest.TestCase): + def test_author_keeps_the_pr_while_replacement_checks_run(self) -> None: + route = hold_author_route_until_checks_report( + {"ci_failing_count": 0, "ci_pending_count": 1}, + "approver", + {"route": "author"}, + ) + + self.assertEqual("author", route) + + def test_author_keeps_the_pr_while_check_results_are_unavailable(self) -> None: + route = hold_author_route_until_checks_report( + {}, + "maintainer", + {"route": "author"}, + ) + + self.assertEqual("author", route) + + def test_settled_checks_release_the_pr_to_reviewers(self) -> None: + route = hold_author_route_until_checks_report( + {"ci_failing_count": 0, "ci_pending_count": 0}, + "approver", + {"route": "author"}, + ) + + self.assertEqual("approver", route) + + def test_reviewer_route_moves_on_while_checks_run(self) -> None: + route = hold_author_route_until_checks_report( + {"ci_pending_count": 1}, + "maintainer", + {"route": "approver"}, + ) + + self.assertEqual("maintainer", route) + + def test_first_observation_of_a_pr_is_not_held(self) -> None: + route = hold_author_route_until_checks_report( + {"ci_pending_count": 1}, + "approver", + None, + ) + + self.assertEqual("approver", route) + + def test_held_route_carries_the_previous_wait_forward(self) -> None: + facts = { + "author_route_held_for_checks": True, + "ci_failing_count": 0, + "last_approver_activity_at": "2026-07-10T01:00:00+00:00", + } + + add_wait_age_facts( + facts, + "author", + {}, + { + "route": "author", + "facts": { + "waiting_since": "2026-07-20T01:00:00+00:00", + "waiting_age_basis": "ci_failure", + }, + }, + ) + + self.assertEqual("2026-07-20T01:00:00+00:00", facts["waiting_since"]) + self.assertEqual("ci_failure", facts["waiting_age_basis"]) + + def test_released_route_recomputes_the_wait(self) -> None: + facts = { + "author_route_held_for_checks": False, + "ci_failing_count": 0, + "last_approver_activity_at": "2026-07-10T01:00:00+00:00", + } + + add_wait_age_facts( + facts, + "author", + {}, + { + "route": "author", + "facts": {"waiting_since": "2026-07-20T01:00:00+00:00"}, + }, + ) + + self.assertEqual("2026-07-10T01:00:00+00:00", facts["waiting_since"]) + self.assertEqual("last_approver_activity", facts["waiting_age_basis"]) + class AuthorNudgeEpisodeTest(unittest.TestCase): def test_preserves_episode_while_route_remains_author(self) -> None: @@ -160,6 +259,7 @@ def gh_api(path: str, paginate: bool) -> list[dict]: patch( "github_cli.gh_pr_check_rollup", return_value={ + "head_oid": "", "required": [], "non_blocking_failures": [], "code_scanning": [], @@ -545,6 +645,7 @@ def test_waits_for_automatic_initial_copilot_review(self) -> None: def test_initial_automatic_review_blocks_human_handoff(self) -> None: facts = { + "ci_pending_count": 0, "copilot_review_requested": True, "copilot_review_exists": False, "copilot_review_needed": False, @@ -561,6 +662,7 @@ def test_initial_automatic_review_blocks_human_handoff(self) -> None: def test_marks_re_review_needed_after_push_since_clean_review(self) -> None: facts = { + "ci_pending_count": 0, "copilot_review_requested": False, "copilot_review_exists": True, "copilot_review_needed": True, @@ -577,6 +679,7 @@ def test_marks_re_review_needed_after_push_since_clean_review(self) -> None: def test_marks_re_review_needed_before_reviewer_handoff(self) -> None: facts = { + "ci_pending_count": 0, "copilot_review_requested": False, "copilot_review_exists": True, "copilot_review_needed": True, @@ -593,6 +696,7 @@ def test_marks_re_review_needed_before_reviewer_handoff(self) -> None: def test_pending_re_review_waits_without_duplicate_request(self) -> None: facts = { + "ci_pending_count": 0, "copilot_review_requested": True, "copilot_review_exists": True, "copilot_review_needed": True, @@ -609,6 +713,7 @@ def test_pending_re_review_waits_without_duplicate_request(self) -> None: def test_current_head_clean_review_moves_to_maintainers(self) -> None: facts = { + "ci_pending_count": 0, "copilot_review_requested": False, "copilot_review_exists": True, "copilot_review_needed": False, @@ -623,8 +728,59 @@ def test_current_head_clean_review_moves_to_maintainers(self) -> None: self.assertEqual(route, "maintainer") self.assertFalse(facts["copilot_review_request_needed"]) + def test_running_checks_hold_re_review_request(self) -> None: + facts = { + "ci_pending_count": 1, + "copilot_review_requested": False, + "copilot_review_exists": True, + "copilot_review_needed": True, + } + + route = apply_copilot_review_gate( + facts, + "approver", + enabled=True, + ) + + self.assertEqual(route, "approver") + self.assertFalse(facts["copilot_review_request_needed"]) + + def test_unavailable_check_results_hold_re_review_request(self) -> None: + facts = { + "copilot_review_requested": False, + "copilot_review_exists": True, + "copilot_review_needed": True, + } + + route = apply_copilot_review_gate( + facts, + "approver", + enabled=True, + ) + + self.assertEqual(route, "approver") + self.assertFalse(facts["copilot_review_request_needed"]) + + def test_running_checks_hold_initial_review_handoff(self) -> None: + facts = { + "ci_pending_count": 2, + "copilot_review_requested": False, + "copilot_review_exists": False, + "copilot_review_needed": False, + } + + route = apply_copilot_review_gate( + facts, + "approver", + enabled=True, + ) + + self.assertEqual(route, "approver") + self.assertFalse(facts["copilot_review_request_needed"]) + def test_disabled_gate_preserves_maintainer_route(self) -> None: facts = { + "ci_pending_count": 0, "copilot_review_requested": False, "copilot_review_exists": True, "copilot_review_needed": True, diff --git a/.github/scripts/pull-request-dashboard/test_github_cli.py b/.github/scripts/pull-request-dashboard/test_github_cli.py index 6003c4276c5..7fca71392db 100644 --- a/.github/scripts/pull-request-dashboard/test_github_cli.py +++ b/.github/scripts/pull-request-dashboard/test_github_cli.py @@ -21,6 +21,7 @@ request_copilot_review, required_check_contexts, required_code_scanning_checks, + settled_check_suite_app_ids, ) @@ -776,6 +777,7 @@ def test_check_rollup_separates_code_scanning_results(self, graphql) -> None: @patch( "github_cli.gh_pr_check_rollup", return_value={ + "head_oid": "current-head", "required": [{"name": "build", "bucket": "pass", "integration_id": 1}], "non_blocking_failures": [], "code_scanning": [ @@ -800,7 +802,11 @@ def test_routing_raw_reports_unsatisfied_code_scanning_rule( _rollup, _branch_rules, ) -> None: - gh_pr_view.return_value = {"id": "PR_node", "baseRefName": "main"} + gh_pr_view.return_value = { + "id": "PR_node", + "baseRefName": "main", + "headRefOid": "current-head", + } raw = fetch_pr_routing_raw( "open-telemetry/example", @@ -814,6 +820,48 @@ def test_routing_raw_reports_unsatisfied_code_scanning_rule( [(check["name"], check["bucket"]) for check in raw["checks"]], ) + @patch("github_cli.gh_branch_rules", return_value=[]) + @patch( + "github_cli.gh_pr_check_rollup", + return_value={ + "head_oid": "previous-head", + "required": [{"name": "build", "bucket": "pass", "integration_id": 1}], + "non_blocking_failures": [], + "code_scanning": [], + }, + ) + @patch("github_cli.fetch_review_threads", return_value=[]) + @patch("github_cli.fetch_review_requests", return_value=[]) + @patch("github_cli.fetch_pr_reviews", return_value=[]) + @patch("github_cli.fetch_pr_issue_comments", return_value=[]) + @patch("github_cli.gh_api", return_value=[]) + @patch("github_cli.gh_pr_view") + def test_routing_raw_discards_checks_from_a_superseded_head( + self, + gh_pr_view, + _gh_api, + _issue_comments, + _reviews, + _review_requests, + _review_threads, + _rollup, + _branch_rules, + ) -> None: + gh_pr_view.return_value = { + "id": "PR_node", + "baseRefName": "main", + "headRefOid": "current-head", + } + + raw = fetch_pr_routing_raw( + "open-telemetry/example", + "open-telemetry", + "example", + 7, + ) + + self.assertIsNone(raw["checks"]) + def test_missing_required_checks_are_pending(self) -> None: self.assertEqual( [ @@ -891,6 +939,52 @@ def test_check_fetch_failure_remains_unknown(self) -> None: )) self.assertIsNone(include_missing_required_checks([], None)) + def test_settled_app_never_reports_its_missing_required_check(self) -> None: + self.assertEqual( + [], + include_missing_required_checks( + [], + [{"context": "windows-unittest", "integration_id": 15368}], + {15368}, + ), + ) + + def test_unsettled_app_still_owes_its_missing_required_check(self) -> None: + checks = include_missing_required_checks( + [], + [{"context": "windows-unittest", "integration_id": 15368}], + {17893}, + ) + + self.assertEqual( + [("windows-unittest", "pending")], + [(check["name"], check["bucket"]) for check in checks or []], + ) + + @patch("github_cli.gh_api") + def test_settled_app_ids_require_every_suite_to_complete(self, gh_api) -> None: + gh_api.return_value = { + "check_suites": [ + {"status": "completed", "app": {"id": 15368}}, + {"status": "in_progress", "app": {"id": 15368}}, + {"status": "completed", "app": {"id": 17893}}, + ], + } + + self.assertEqual({17893}, settled_check_suite_app_ids("owner/repo", "head")) + gh_api.assert_called_once_with( + "/repos/owner/repo/commits/head/check-suites?per_page=100" + ) + + @patch("github_cli.gh_api") + def test_settled_app_ids_are_empty_without_a_head_sha(self, gh_api) -> None: + self.assertEqual(set(), settled_check_suite_app_ids("owner/repo", "")) + gh_api.assert_not_called() + + @patch("github_cli.gh_api", side_effect=RuntimeError("boom")) + def test_settled_app_ids_are_empty_when_suites_cannot_be_read(self, _gh_api) -> None: + self.assertEqual(set(), settled_check_suite_app_ids("owner/repo", "head")) + @patch("github_cli.gh_graphql") def test_fetch_pr_reviews_normalizes_paginated_reviews(self, graphql) -> None: graphql.side_effect = [ diff --git a/.github/scripts/pull-request-dashboard/test_state.py b/.github/scripts/pull-request-dashboard/test_state.py index c94ae35901c..20021070a1b 100644 --- a/.github/scripts/pull-request-dashboard/test_state.py +++ b/.github/scripts/pull-request-dashboard/test_state.py @@ -164,10 +164,10 @@ def test_dashboard_state_save_writes_explicit_shape(self) -> None: def test_notification_state_version_is_independent(self) -> None: self.assertEqual(BACKFILL_STATE_VERSION, 3) self.assertEqual(NOTIFICATION_STATE_VERSION, 3) - self.assertEqual(DASHBOARD_STATE_VERSION, 6) + self.assertEqual(DASHBOARD_STATE_VERSION, 7) self.assertEqual(STATUS_COMMENT_ROLLOUT_STATE_VERSION, 1) self.assertEqual(AUTHOR_NUDGE_STATE_VERSION, 2) - self.assertEqual(COPILOT_REVIEW_REQUEST_STATE_VERSION, 3) + self.assertEqual(COPILOT_REVIEW_REQUEST_STATE_VERSION, 4) def test_author_nudge_state_round_trip(self) -> None: with tempfile.TemporaryDirectory() as temp_dir, patch("state._state_dir", Path(temp_dir)): From 3c1ff37d7e365638a0b73c9390fa74c1094bba9d Mon Sep 17 00:00:00 2001 From: Trask Stalnaker Date: Thu, 30 Jul 2026 08:23:14 -0700 Subject: [PATCH 02/16] Do not restart the reviewers' wait clock on an author push --- .../pull-request-dashboard/RATIONALE.md | 6 ++ .../pull-request-dashboard/dashboard.py | 24 +++++- .../pull-request-dashboard/test_dashboard.py | 78 +++++++++++++++++++ 3 files changed, 106 insertions(+), 2 deletions(-) diff --git a/.github/scripts/pull-request-dashboard/RATIONALE.md b/.github/scripts/pull-request-dashboard/RATIONALE.md index 45b0e6b296b..c575c4379c5 100644 --- a/.github/scripts/pull-request-dashboard/RATIONALE.md +++ b/.github/scripts/pull-request-dashboard/RATIONALE.md @@ -212,6 +212,12 @@ the implementation understandable and operationally cheap. the end of the CI failure and fall back to the last approver activity, which is usually far older, so a PR the author had just pushed to would sort to the top of the waiting-on-authors section as the stalest item on the board. +- While a PR stays on a route where someone other than the author owes it a + response, its wait age only moves back, never forward. The fallback for those + routes is the last author activity, so a push would otherwise restart the + clock and present a review nobody has done in a week as brand new. A handoff + from the author route does start a fresh wait, because that push is what put + the PR in front of reviewers. - Maintenance-bot PRs retain maintainer-oriented routing because the bot cannot respond to a dashboard action. Pending required checks affect the CI column but do not change who owns the next action. diff --git a/.github/scripts/pull-request-dashboard/dashboard.py b/.github/scripts/pull-request-dashboard/dashboard.py index ac78fe5cdf6..625b47e4805 100644 --- a/.github/scripts/pull-request-dashboard/dashboard.py +++ b/.github/scripts/pull-request-dashboard/dashboard.py @@ -130,7 +130,9 @@ route-appropriate fallback, or PR creation time. Carried forward while the author route - is held. + is held, and never moves + forward while the PR stays on + a reviewer route. waiting_age_basis str Which heuristic chose waiting_since. author_action_review_thread_urls @@ -1153,8 +1155,13 @@ def oldest_pending_action_ts( return min(timestamps) if timestamps else None +# Routes where the PR is out of the author's hands and someone else owes it a +# response, so an author push does not change whose turn it is. +REVIEWER_ROUTES = ("approver", "maintainer", "copilot") + + def fallback_wait_ts(route: str, facts: dict[str, Any]) -> tuple[datetime | None, str]: - if route in ("approver", "maintainer", "copilot"): + if route in REVIEWER_ROUTES: return parse_ts(facts.get("last_author_activity_at") or ""), "last_author_activity" if route == "author": if facts.get("ci_failing_count", 0) > 0: @@ -1192,6 +1199,19 @@ def add_wait_age_facts( if wait_ts is None: wait_ts = parse_ts(facts.get("created_at") or "") basis = "created" + previous_wait_ts = parse_ts(previous_facts.get("waiting_since") or "") + # Reviewers have been waiting since the PR reached them, so while it stays + # with them the clock only moves back, never forward: an author push is not + # a fresh start for a review that has not happened yet. + if ( + route in REVIEWER_ROUTES + and (previous_result or {}).get("route") in REVIEWER_ROUTES + and previous_wait_ts is not None + and wait_ts is not None + and previous_wait_ts < wait_ts + ): + wait_ts = previous_wait_ts + basis = previous_facts.get("waiting_age_basis") or "" facts["waiting_since"] = format_ts(wait_ts) facts["waiting_age_basis"] = basis diff --git a/.github/scripts/pull-request-dashboard/test_dashboard.py b/.github/scripts/pull-request-dashboard/test_dashboard.py index 60b5a98d962..7d0d65979eb 100644 --- a/.github/scripts/pull-request-dashboard/test_dashboard.py +++ b/.github/scripts/pull-request-dashboard/test_dashboard.py @@ -169,6 +169,84 @@ def test_released_route_recomputes_the_wait(self) -> None: self.assertEqual("last_approver_activity", facts["waiting_age_basis"]) +class ReviewerWaitTest(unittest.TestCase): + def test_author_push_does_not_restart_the_reviewer_wait(self) -> None: + facts = {"last_author_activity_at": "2026-07-30T01:00:00+00:00"} + + add_wait_age_facts( + facts, + "approver", + {}, + { + "route": "approver", + "facts": { + "waiting_since": "2026-07-23T01:00:00+00:00", + "waiting_age_basis": "last_author_activity", + }, + }, + ) + + self.assertEqual("2026-07-23T01:00:00+00:00", facts["waiting_since"]) + self.assertEqual("last_author_activity", facts["waiting_age_basis"]) + + def test_wait_survives_the_copilot_review_detour(self) -> None: + facts = {"last_author_activity_at": "2026-07-30T01:00:00+00:00"} + + add_wait_age_facts( + facts, + "approver", + {}, + { + "route": "copilot", + "facts": { + "waiting_since": "2026-07-23T01:00:00+00:00", + "waiting_age_basis": "last_author_activity", + }, + }, + ) + + self.assertEqual("2026-07-23T01:00:00+00:00", facts["waiting_since"]) + + def test_handoff_from_the_author_starts_a_new_wait(self) -> None: + facts = {"last_author_activity_at": "2026-07-30T01:00:00+00:00"} + + add_wait_age_facts( + facts, + "approver", + {}, + { + "route": "author", + "facts": {"waiting_since": "2026-07-23T01:00:00+00:00"}, + }, + ) + + self.assertEqual("2026-07-30T01:00:00+00:00", facts["waiting_since"]) + self.assertEqual("last_author_activity", facts["waiting_age_basis"]) + + def test_older_evidence_still_moves_the_wait_back(self) -> None: + facts = {"last_author_activity_at": "2026-07-10T01:00:00+00:00"} + + add_wait_age_facts( + facts, + "approver", + {}, + { + "route": "approver", + "facts": {"waiting_since": "2026-07-23T01:00:00+00:00"}, + }, + ) + + self.assertEqual("2026-07-10T01:00:00+00:00", facts["waiting_since"]) + self.assertEqual("last_author_activity", facts["waiting_age_basis"]) + + def test_first_observation_computes_the_wait(self) -> None: + facts = {"last_author_activity_at": "2026-07-30T01:00:00+00:00"} + + add_wait_age_facts(facts, "approver", {}, None) + + self.assertEqual("2026-07-30T01:00:00+00:00", facts["waiting_since"]) + + class AuthorNudgeEpisodeTest(unittest.TestCase): def test_preserves_episode_while_route_remains_author(self) -> None: facts: dict[str, object] = {} From 14d5bdd5ae8383abb3af6b851add1faf4c19da11 Mon Sep 17 00:00:00 2001 From: Trask Stalnaker Date: Thu, 30 Jul 2026 08:30:28 -0700 Subject: [PATCH 03/16] Address review comment from copilot-pull-request-reviewer: clear the held-route flag when the override wins --- .github/scripts/pull-request-dashboard/dashboard.py | 7 ++++++- .github/scripts/pull-request-dashboard/test_dashboard.py | 1 + 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/.github/scripts/pull-request-dashboard/dashboard.py b/.github/scripts/pull-request-dashboard/dashboard.py index 625b47e4805..b7aa841edfe 100644 --- a/.github/scripts/pull-request-dashboard/dashboard.py +++ b/.github/scripts/pull-request-dashboard/dashboard.py @@ -1343,7 +1343,7 @@ def resolve_pr_route( # Apply the manual reviewer-routing override before the Copilot review gate # so an overridden route (for example author -> reviewers) is still held for # a required clean Copilot review instead of bypassing it. - return apply_copilot_review_gate( + route = apply_copilot_review_gate( facts, apply_dashboard_override( facts, @@ -1355,6 +1355,11 @@ def resolve_pr_route( ), enabled=require_clean_copilot_review, ) + if route != "author": + # The override outranks the hold, and a route the PR did not keep has no + # wait to carry forward. + facts["author_route_held_for_checks"] = False + return route def assign_author_nudge_episode( diff --git a/.github/scripts/pull-request-dashboard/test_dashboard.py b/.github/scripts/pull-request-dashboard/test_dashboard.py index 7d0d65979eb..e2fd8d3e3ac 100644 --- a/.github/scripts/pull-request-dashboard/test_dashboard.py +++ b/.github/scripts/pull-request-dashboard/test_dashboard.py @@ -77,6 +77,7 @@ def test_override_reaches_reviewers_while_checks_are_running(self) -> None: route = resolve_pr_route(facts, {}, 1, False, {"route": "author"}) self.assertEqual("approver", route) + self.assertFalse(facts["author_route_held_for_checks"]) class AuthorRouteHoldTest(unittest.TestCase): From 396ac380b4e1b7310132c92a1c5f962abd7fa013 Mon Sep 17 00:00:00 2001 From: Trask Stalnaker Date: Thu, 30 Jul 2026 10:34:05 -0700 Subject: [PATCH 04/16] Hold the reviewer handoff with the author until CI and Copilot report Removes the copilot route: the required checks and the Copilot review are gates the author owns, so an outstanding one keeps the PR waiting on its author instead of moving it to a route nobody acts on. --- .../pull-request-dashboard/RATIONALE.md | 43 +++-- .../pull-request-dashboard/copilot_review.py | 36 ++-- .../pull-request-dashboard/dashboard.py | 74 ++++---- .../dashboard_override.py | 8 +- .../pr_status_comment.py | 7 + .../route_presentation.py | 6 - .../test_copilot_review.py | 8 +- .../pull-request-dashboard/test_dashboard.py | 162 ++++++------------ .../test_dashboard_override.py | 18 +- .../test_notify_slack.py | 5 +- .../test_pr_status_comment.py | 1 - .../test_route_presentation.py | 6 - 12 files changed, 171 insertions(+), 203 deletions(-) diff --git a/.github/scripts/pull-request-dashboard/RATIONALE.md b/.github/scripts/pull-request-dashboard/RATIONALE.md index c575c4379c5..a87be01bcb6 100644 --- a/.github/scripts/pull-request-dashboard/RATIONALE.md +++ b/.github/scripts/pull-request-dashboard/RATIONALE.md @@ -200,14 +200,20 @@ the implementation understandable and operationally cheap. Repository-configured `non_blocking_check_patterns` identify failed optional checks in a note alongside this action, without changing required-check facts or routing. -- A PR already routed to its author stays there until the required checks - report again. A push clears the failing count before the replacement checks - produce a result, so the PR would otherwise be handed off on evidence that - does not exist yet and handed back minutes later when the same check fails. - Only the author route is held, because it is the only one a check result can - restore; a PR moving between reviewer and maintainer routes is not waiting on - CI. Unavailable check results hold the route for the same reason a pending - one does, and resolve on a later run. +- A PR the dashboard would hand to reviewers stays with its author until the + required checks report. Clearing the checks is the author's job, so an + outstanding one is not yet a reason to spend reviewer attention, and a push + clears the failing count before the replacement checks produce a result, so + the PR would otherwise be handed off on evidence that does not exist yet and + handed back minutes later when the same check fails. A PR that has already + reached reviewers is never pulled back, because a check result alone does not + make it the author's turn again. Unavailable check results hold the handoff + for the same reason a pending one does, and resolve on a later run. +- A held PR is presented as waiting on its author rather than on the robot it + is waiting for. The CI column and the reviewers column already name what is + outstanding, and the live status comment tells the author the handoff happens + once those are clean, so a separate route would add a section that nobody is + expected to act on. - A held route also holds its wait age. Recomputing it would read the push as the end of the CI failure and fall back to the last approver activity, which is usually far older, so a PR the author had just pushed to would sort to the @@ -231,7 +237,7 @@ the implementation understandable and operationally cheap. - The setting lists the base branches to gate rather than a single on/off switch, because automatic Copilot review is itself configured per branch (often only the default branch). Gating a branch with no automatic review - would park every ready PR on the copilot route waiting for a review that never + would hold every ready PR with its author waiting for a review that never runs, so only branches with automatic review are listed and PRs targeting other branches route normally. - Copilot findings normally return a PR to the author through ordinary @@ -246,12 +252,17 @@ the implementation understandable and operationally cheap. gate or produces fresh actionable threads that route the PR back to the author, so re-requesting an unchanged commit is self-correcting rather than a re-request loop. -- The gate only fires once the required checks have settled. A route computed - while checks are still running is provisional: a failure that has not - completed yet cannot route the PR to its author, so the PR looks ready for - reviewers and the gate would spend a Copilot review on code CI is about to - reject. Unavailable check results are treated the same as running ones, - because both mean the routing decision cannot be trusted yet. +- An outstanding Copilot review holds the reviewer handoff the same way an + unsettled required check does, including when a reviewer-routing override + asked for the handoff. The override says the author is done with the + discussion, not that the robots have finished, and a handoff made before they + report would only change back. +- The gate withholds the re-review request until the required checks have + settled. A route computed while checks are still running is provisional: a + failure that has not completed yet cannot route the PR to its author, so the + PR looks ready for reviewers and the gate would spend a Copilot review on code + CI is about to reject. Unavailable check results are treated the same as + running ones, because both mean the routing decision cannot be trusted yet. - Delivery re-validates the required checks against live data rather than relying on the routing fingerprint alone. The fingerprint only detects change, so checks that were unsettled when the request was recorded and are @@ -260,7 +271,7 @@ the implementation understandable and operationally cheap. review, not from the classifier's actionability judgment. Accepted limitation: if Copilot leaves comments the classifier treats as non-actionable while they stay unresolved, routing sits at reviewers but the - gate holds the PR on the copilot route and re-requests until Copilot returns a + handoff stays held with the author and re-requests until Copilot returns a comment-free review or the author pushes. The strict count is intentional — the gate is a conservative "Copilot had nothing to say about this exact code" check, and folding in classifier judgment could let a real-but-non-actionable diff --git a/.github/scripts/pull-request-dashboard/copilot_review.py b/.github/scripts/pull-request-dashboard/copilot_review.py index 8974c8b684c..777bec44962 100644 --- a/.github/scripts/pull-request-dashboard/copilot_review.py +++ b/.github/scripts/pull-request-dashboard/copilot_review.py @@ -66,24 +66,31 @@ def required_checks_settled(facts: dict[str, Any]) -> bool: return not facts.get("ci_pending_count", 0) -def apply_copilot_review_gate( +def copilot_review_outstanding(facts: dict[str, Any], *, enabled: bool) -> bool: + if not enabled: + return False + return not facts.get("copilot_review_exists") or bool( + facts.get("copilot_review_needed") + ) + + +def set_copilot_review_request_needed( facts: dict[str, Any], route: str, *, enabled: bool, -) -> str: - facts["copilot_review_request_needed"] = False - if not enabled or route not in ("approver", "maintainer"): - return route - if not required_checks_settled(facts): - return route - if not facts.get("copilot_review_exists"): - return "copilot" - if not facts.get("copilot_review_needed"): - return route - if not facts.get("copilot_review_requested"): - facts["copilot_review_request_needed"] = True - return "copilot" +) -> None: + # Requesting a re-review before the checks report would spend it on code CI + # is about to reject, and a PR GitHub has never reviewed is already queued + # for the automatic first review. + facts["copilot_review_request_needed"] = ( + enabled + and route in ("approver", "maintainer") + and bool(facts.get("copilot_review_exists")) + and bool(facts.get("copilot_review_needed")) + and not facts.get("copilot_review_requested") + and required_checks_settled(facts) + ) def record_copilot_review_observation( @@ -99,7 +106,6 @@ def record_copilot_review_observation( if ( not result or result.get("failed") - or result.get("route") != "copilot" or not facts.get("copilot_review_request_needed") or not head_sha or not routing_fingerprint diff --git a/.github/scripts/pull-request-dashboard/dashboard.py b/.github/scripts/pull-request-dashboard/dashboard.py index b7aa841edfe..eb1020e47d1 100644 --- a/.github/scripts/pull-request-dashboard/dashboard.py +++ b/.github/scripts/pull-request-dashboard/dashboard.py @@ -123,14 +123,15 @@ last_approver_activity_at str (iso) Stage 2 — add_wait_age_facts (depends on routing + pending actions): - author_route_held_for_checks bool Author route retained because - the required checks have not - reported since a push. + route_held_for_gates bool Reviewer handoff withheld + because the required checks or + the Copilot review are still + outstanding. waiting_since str (iso) Oldest pending discussion, or route-appropriate fallback, or PR creation time. Carried - forward while the author route - is held, and never moves + forward while the handoff is + held, and never moves forward while the PR stays on a reviewer route. waiting_age_basis str Which heuristic chose @@ -199,11 +200,12 @@ ) from author_nudge import record_author_nudge_observation, routing_input_fingerprint from copilot_review import ( - apply_copilot_review_gate, + copilot_review_outstanding, copilot_review_status, is_copilot_reviewer, record_copilot_review_observation, required_checks_settled, + set_copilot_review_request_needed, ) from dashboard_override import ( apply_dashboard_override, @@ -1157,7 +1159,7 @@ def oldest_pending_action_ts( # Routes where the PR is out of the author's hands and someone else owes it a # response, so an author push does not change whose turn it is. -REVIEWER_ROUTES = ("approver", "maintainer", "copilot") +REVIEWER_ROUTES = ("approver", "maintainer") def fallback_wait_ts(route: str, facts: dict[str, Any]) -> tuple[datetime | None, str]: @@ -1182,7 +1184,7 @@ def add_wait_age_facts( previous_facts = (previous_result or {}).get("facts") or {} # A held route was not re-evaluated, so its wait continues uninterrupted # rather than restarting from whatever the incomplete facts now imply. - if facts.get("author_route_held_for_checks") and previous_facts.get("waiting_since"): + if facts.get("route_held_for_gates") and previous_facts.get("waiting_since"): facts["waiting_since"] = previous_facts["waiting_since"] facts["waiting_age_basis"] = previous_facts.get("waiting_age_basis") or "" return @@ -1318,18 +1320,27 @@ def add_reviewers( ] -def hold_author_route_until_checks_report( +def hold_route_until_gates_settle( facts: dict[str, Any], route: str, previous_result: dict[str, Any] | None, + *, + require_clean_copilot_review: bool, ) -> str: - # A push clears the failing count before the replacement checks report, so - # the author would otherwise hand the PR off on evidence that does not exist - # yet. Routes that do not depend on a check result move freely. - held = (previous_result or {}).get( - "route" - ) == "author" and not required_checks_settled(facts) - facts["author_route_held_for_checks"] = held + # The required checks and the Copilot review are the author's to clear, so + # an outstanding one keeps the PR with them. A PR that already reached + # reviewers is never pulled back, so a push cannot change whose turn it is. + held = ( + route in REVIEWER_ROUTES + and (previous_result or {}).get("route") not in REVIEWER_ROUTES + and ( + not required_checks_settled(facts) + or copilot_review_outstanding( + facts, enabled=require_clean_copilot_review + ) + ) + ) + facts["route_held_for_gates"] = held return "author" if held else route @@ -1340,26 +1351,21 @@ def resolve_pr_route( require_clean_copilot_review: bool, previous_result: dict[str, Any] | None = None, ) -> str: - # Apply the manual reviewer-routing override before the Copilot review gate - # so an overridden route (for example author -> reviewers) is still held for - # a required clean Copilot review instead of bypassing it. - route = apply_copilot_review_gate( + # The override hands the PR to reviewers the same way automatic routing + # does, so the gates hold it the same way rather than being bypassed. + route = apply_dashboard_override( facts, - apply_dashboard_override( - facts, - hold_author_route_until_checks_report( - facts, - route_pr(facts, pending_actions, required_approvals), - previous_result, - ), - ), - enabled=require_clean_copilot_review, + route_pr(facts, pending_actions, required_approvals), + ) + set_copilot_review_request_needed( + facts, route, enabled=require_clean_copilot_review + ) + return hold_route_until_gates_settle( + facts, + route, + previous_result, + require_clean_copilot_review=require_clean_copilot_review, ) - if route != "author": - # The override outranks the hold, and a route the PR did not keep has no - # wait to carry forward. - facts["author_route_held_for_checks"] = False - return route def assign_author_nudge_episode( diff --git a/.github/scripts/pull-request-dashboard/dashboard_override.py b/.github/scripts/pull-request-dashboard/dashboard_override.py index d9b383dd8de..1b3fc446786 100644 --- a/.github/scripts/pull-request-dashboard/dashboard_override.py +++ b/.github/scripts/pull-request-dashboard/dashboard_override.py @@ -218,7 +218,6 @@ def override_ack_marker(comment_id: int) -> str: ROUTE_ALREADY_ROUTED_PHRASE = { "approver": "already waiting on reviewers", "maintainer": "already past review and waiting on maintainers", - "copilot": "waiting on an automated Copilot review", } @@ -232,10 +231,11 @@ def render_command_reply(reply: dict[str, Any]) -> str: "use `/dashboard route:reviewers`." ) elif kind == "routed": - if reply.get("route") == "copilot": + if reply.get("held_for_gates"): message = ( "accepted the reviewer-routing override; the reviewer handoff " - "is waiting on Copilot." + "is waiting on the required status checks and the Copilot " + "review." ) else: message = "routed this pull request to reviewers." @@ -422,7 +422,7 @@ def deliver_dashboard_override_requests(repo: str) -> list[str]: { "comment_id": facts["dashboard_override_command_id"], "kind": "routed", - "route": (result or {}).get("route") or "", + "held_for_gates": bool(facts.get("route_held_for_gates")), "user": facts.get("dashboard_override_command_user") or "", }, ) diff --git a/.github/scripts/pull-request-dashboard/pr_status_comment.py b/.github/scripts/pull-request-dashboard/pr_status_comment.py index c72c5ce340e..d4daa60f34e 100644 --- a/.github/scripts/pull-request-dashboard/pr_status_comment.py +++ b/.github/scripts/pull-request-dashboard/pr_status_comment.py @@ -185,6 +185,7 @@ def author_body( non_blocking_failure_note: str, review_thread_urls: list[str], top_level_feedback_urls: list[str], + held_for_gates: bool = False, ) -> list[str]: noun = "item" if feedback_count == 1 else "items" if failing_count and feedback_count: @@ -213,6 +214,11 @@ def author_body( if non_blocking_failure_note: sentence += f" Note: {non_blocking_failure_note}" return [sentence] + if held_for_gates: + return [ + "Wait for the required status checks and the Copilot review to " + "finish; this pull request moves to reviewers once they are clean." + ] _, fallback_next_step = route_status_summary("author") return [fallback_next_step] @@ -260,6 +266,7 @@ def render_status_comment( ), review_thread_urls=review_thread_urls, top_level_feedback_urls=top_level_feedback_urls, + held_for_gates=bool(facts.get("route_held_for_gates")), ) else: _, next_step = route_status_summary(route) diff --git a/.github/scripts/pull-request-dashboard/route_presentation.py b/.github/scripts/pull-request-dashboard/route_presentation.py index 36c28815f3c..e9cedf3e2fd 100644 --- a/.github/scripts/pull-request-dashboard/route_presentation.py +++ b/.github/scripts/pull-request-dashboard/route_presentation.py @@ -8,12 +8,6 @@ "status_waiting_on": "Maintainers", "status_next_step": "Merge when ready.", }, - "copilot": { - "dashboard_label": "Waiting on Copilot", - "status_headline": "Waiting on Copilot", - "status_waiting_on": "Copilot", - "status_next_step": "Wait for the pending review to complete.", - }, "approver": { "dashboard_label": "Waiting on reviewers", "status_headline": "Waiting on reviewers", diff --git a/.github/scripts/pull-request-dashboard/test_copilot_review.py b/.github/scripts/pull-request-dashboard/test_copilot_review.py index 10df27964c2..4afdcdea0f0 100644 --- a/.github/scripts/pull-request-dashboard/test_copilot_review.py +++ b/.github/scripts/pull-request-dashboard/test_copilot_review.py @@ -24,7 +24,7 @@ def test_records_request_for_current_head(self, _load_requests, save_requests) - record_copilot_review_observation( 7, { - "route": "copilot", + "route": "approver", "facts": { "head_sha": "current-head", "copilot_review_request_needed": True, @@ -52,7 +52,7 @@ def test_new_head_replaces_previous_request(self, _load_requests, save_requests) record_copilot_review_observation( 7, { - "route": "copilot", + "route": "approver", "facts": { "head_sha": "current-head", "copilot_review_request_needed": True, @@ -89,7 +89,7 @@ def test_same_head_request_needed_resets_acknowledgement( record_copilot_review_observation( 7, { - "route": "copilot", + "route": "approver", "facts": { "head_sha": "current-head", "copilot_review_request_needed": True, @@ -138,7 +138,7 @@ def test_initial_automatic_review_does_not_enqueue_request( record_copilot_review_observation( 7, { - "route": "copilot", + "route": "approver", "facts": { "head_sha": "current-head", "copilot_review_exists": False, diff --git a/.github/scripts/pull-request-dashboard/test_dashboard.py b/.github/scripts/pull-request-dashboard/test_dashboard.py index e2fd8d3e3ac..119004e06d8 100644 --- a/.github/scripts/pull-request-dashboard/test_dashboard.py +++ b/.github/scripts/pull-request-dashboard/test_dashboard.py @@ -7,7 +7,7 @@ import unittest from unittest.mock import ANY, Mock, call, patch -from copilot_review import apply_copilot_review_gate +from copilot_review import set_copilot_review_request_needed from dashboard import ( BACKFILL_RECORDED_FAILURE_STATUS, DashboardUpdate, @@ -20,7 +20,7 @@ compute_facts, fetch_pr_raw, group_review_threads, - hold_author_route_until_checks_report, + hold_route_until_gates_settle, main, remove_cached_dashboard_prs, resolve_pr_route, @@ -52,7 +52,8 @@ def test_override_is_still_gated_by_required_copilot_review(self) -> None: route = resolve_pr_route(facts, {}, 1, True) - self.assertEqual("copilot", route) + self.assertEqual("author", route) + self.assertTrue(facts["route_held_for_gates"]) def test_override_reaches_reviewers_when_copilot_review_is_clean(self) -> None: facts = self._author_route_facts( @@ -71,18 +72,32 @@ def test_override_reaches_reviewers_when_gate_disabled(self) -> None: self.assertEqual("approver", route) - def test_override_reaches_reviewers_while_checks_are_running(self) -> None: + def test_override_is_held_while_checks_are_running(self) -> None: facts = self._author_route_facts(ci_failing_count=0, ci_pending_count=1) route = resolve_pr_route(facts, {}, 1, False, {"route": "author"}) - self.assertEqual("approver", route) - self.assertFalse(facts["author_route_held_for_checks"]) + self.assertEqual("author", route) + self.assertTrue(facts["route_held_for_gates"]) + +class GateHoldTest(unittest.TestCase): + def _hold( + self, + facts: dict[str, object], + route: str, + previous_result: dict[str, object] | None, + require_clean_copilot_review: bool = False, + ) -> str: + return hold_route_until_gates_settle( + facts, + route, + previous_result, + require_clean_copilot_review=require_clean_copilot_review, + ) -class AuthorRouteHoldTest(unittest.TestCase): def test_author_keeps_the_pr_while_replacement_checks_run(self) -> None: - route = hold_author_route_until_checks_report( + route = self._hold( {"ci_failing_count": 0, "ci_pending_count": 1}, "approver", {"route": "author"}, @@ -91,16 +106,22 @@ def test_author_keeps_the_pr_while_replacement_checks_run(self) -> None: self.assertEqual("author", route) def test_author_keeps_the_pr_while_check_results_are_unavailable(self) -> None: - route = hold_author_route_until_checks_report( - {}, - "maintainer", + route = self._hold({}, "maintainer", {"route": "author"}) + + self.assertEqual("author", route) + + def test_author_keeps_the_pr_while_a_copilot_review_is_outstanding(self) -> None: + route = self._hold( + {"ci_pending_count": 0, "copilot_review_exists": False}, + "approver", {"route": "author"}, + require_clean_copilot_review=True, ) self.assertEqual("author", route) def test_settled_checks_release_the_pr_to_reviewers(self) -> None: - route = hold_author_route_until_checks_report( + route = self._hold( {"ci_failing_count": 0, "ci_pending_count": 0}, "approver", {"route": "author"}, @@ -109,26 +130,18 @@ def test_settled_checks_release_the_pr_to_reviewers(self) -> None: self.assertEqual("approver", route) def test_reviewer_route_moves_on_while_checks_run(self) -> None: - route = hold_author_route_until_checks_report( - {"ci_pending_count": 1}, - "maintainer", - {"route": "approver"}, - ) + route = self._hold({"ci_pending_count": 1}, "maintainer", {"route": "approver"}) self.assertEqual("maintainer", route) - def test_first_observation_of_a_pr_is_not_held(self) -> None: - route = hold_author_route_until_checks_report( - {"ci_pending_count": 1}, - "approver", - None, - ) + def test_a_pr_that_never_reached_reviewers_is_held(self) -> None: + route = self._hold({"ci_pending_count": 1}, "approver", None) - self.assertEqual("approver", route) + self.assertEqual("author", route) def test_held_route_carries_the_previous_wait_forward(self) -> None: facts = { - "author_route_held_for_checks": True, + "route_held_for_gates": True, "ci_failing_count": 0, "last_approver_activity_at": "2026-07-10T01:00:00+00:00", } @@ -151,7 +164,7 @@ def test_held_route_carries_the_previous_wait_forward(self) -> None: def test_released_route_recomputes_the_wait(self) -> None: facts = { - "author_route_held_for_checks": False, + "route_held_for_gates": False, "ci_failing_count": 0, "last_approver_activity_at": "2026-07-10T01:00:00+00:00", } @@ -190,24 +203,6 @@ def test_author_push_does_not_restart_the_reviewer_wait(self) -> None: self.assertEqual("2026-07-23T01:00:00+00:00", facts["waiting_since"]) self.assertEqual("last_author_activity", facts["waiting_age_basis"]) - def test_wait_survives_the_copilot_review_detour(self) -> None: - facts = {"last_author_activity_at": "2026-07-30T01:00:00+00:00"} - - add_wait_age_facts( - facts, - "approver", - {}, - { - "route": "copilot", - "facts": { - "waiting_since": "2026-07-23T01:00:00+00:00", - "waiting_age_basis": "last_author_activity", - }, - }, - ) - - self.assertEqual("2026-07-23T01:00:00+00:00", facts["waiting_since"]) - def test_handoff_from_the_author_starts_a_new_wait(self) -> None: facts = {"last_author_activity_at": "2026-07-30T01:00:00+00:00"} @@ -722,7 +717,7 @@ def test_waits_for_automatic_initial_copilot_review(self) -> None: self.assertFalse(facts["copilot_review_exists"]) self.assertFalse(facts["copilot_review_needed"]) - def test_initial_automatic_review_blocks_human_handoff(self) -> None: + def test_initial_automatic_review_needs_no_request(self) -> None: facts = { "ci_pending_count": 0, "copilot_review_requested": True, @@ -730,13 +725,8 @@ def test_initial_automatic_review_blocks_human_handoff(self) -> None: "copilot_review_needed": False, } - route = apply_copilot_review_gate( - facts, - "approver", - enabled=True, - ) + set_copilot_review_request_needed(facts, "approver", enabled=True) - self.assertEqual(route, "copilot") self.assertFalse(facts["copilot_review_request_needed"]) def test_marks_re_review_needed_after_push_since_clean_review(self) -> None: @@ -747,13 +737,8 @@ def test_marks_re_review_needed_after_push_since_clean_review(self) -> None: "copilot_review_needed": True, } - route = apply_copilot_review_gate( - facts, - "maintainer", - enabled=True, - ) + set_copilot_review_request_needed(facts, "maintainer", enabled=True) - self.assertEqual(route, "copilot") self.assertTrue(facts["copilot_review_request_needed"]) def test_marks_re_review_needed_before_reviewer_handoff(self) -> None: @@ -764,16 +749,11 @@ def test_marks_re_review_needed_before_reviewer_handoff(self) -> None: "copilot_review_needed": True, } - route = apply_copilot_review_gate( - facts, - "approver", - enabled=True, - ) + set_copilot_review_request_needed(facts, "approver", enabled=True) - self.assertEqual(route, "copilot") self.assertTrue(facts["copilot_review_request_needed"]) - def test_pending_re_review_waits_without_duplicate_request(self) -> None: + def test_pending_re_review_is_not_requested_twice(self) -> None: facts = { "ci_pending_count": 0, "copilot_review_requested": True, @@ -781,16 +761,11 @@ def test_pending_re_review_waits_without_duplicate_request(self) -> None: "copilot_review_needed": True, } - route = apply_copilot_review_gate( - facts, - "maintainer", - enabled=True, - ) + set_copilot_review_request_needed(facts, "maintainer", enabled=True) - self.assertEqual(route, "copilot") self.assertFalse(facts["copilot_review_request_needed"]) - def test_current_head_clean_review_moves_to_maintainers(self) -> None: + def test_current_head_clean_review_needs_no_request(self) -> None: facts = { "ci_pending_count": 0, "copilot_review_requested": False, @@ -798,13 +773,8 @@ def test_current_head_clean_review_moves_to_maintainers(self) -> None: "copilot_review_needed": False, } - route = apply_copilot_review_gate( - facts, - "maintainer", - enabled=True, - ) + set_copilot_review_request_needed(facts, "maintainer", enabled=True) - self.assertEqual(route, "maintainer") self.assertFalse(facts["copilot_review_request_needed"]) def test_running_checks_hold_re_review_request(self) -> None: @@ -815,13 +785,8 @@ def test_running_checks_hold_re_review_request(self) -> None: "copilot_review_needed": True, } - route = apply_copilot_review_gate( - facts, - "approver", - enabled=True, - ) + set_copilot_review_request_needed(facts, "approver", enabled=True) - self.assertEqual(route, "approver") self.assertFalse(facts["copilot_review_request_needed"]) def test_unavailable_check_results_hold_re_review_request(self) -> None: @@ -831,33 +796,23 @@ def test_unavailable_check_results_hold_re_review_request(self) -> None: "copilot_review_needed": True, } - route = apply_copilot_review_gate( - facts, - "approver", - enabled=True, - ) + set_copilot_review_request_needed(facts, "approver", enabled=True) - self.assertEqual(route, "approver") self.assertFalse(facts["copilot_review_request_needed"]) - def test_running_checks_hold_initial_review_handoff(self) -> None: + def test_author_route_does_not_request_a_re_review(self) -> None: facts = { - "ci_pending_count": 2, + "ci_pending_count": 0, "copilot_review_requested": False, - "copilot_review_exists": False, - "copilot_review_needed": False, + "copilot_review_exists": True, + "copilot_review_needed": True, } - route = apply_copilot_review_gate( - facts, - "approver", - enabled=True, - ) + set_copilot_review_request_needed(facts, "author", enabled=True) - self.assertEqual(route, "approver") self.assertFalse(facts["copilot_review_request_needed"]) - def test_disabled_gate_preserves_maintainer_route(self) -> None: + def test_disabled_gate_requests_nothing(self) -> None: facts = { "ci_pending_count": 0, "copilot_review_requested": False, @@ -865,13 +820,8 @@ def test_disabled_gate_preserves_maintainer_route(self) -> None: "copilot_review_needed": True, } - route = apply_copilot_review_gate( - facts, - "maintainer", - enabled=False, - ) + set_copilot_review_request_needed(facts, "maintainer", enabled=False) - self.assertEqual(route, "maintainer") self.assertFalse(facts["copilot_review_request_needed"]) diff --git a/.github/scripts/pull-request-dashboard/test_dashboard_override.py b/.github/scripts/pull-request-dashboard/test_dashboard_override.py index 5e07368b6df..62abe6d9750 100644 --- a/.github/scripts/pull-request-dashboard/test_dashboard_override.py +++ b/.github/scripts/pull-request-dashboard/test_dashboard_override.py @@ -150,10 +150,10 @@ def test_renders_command_replies(self) -> None: routed = dashboard_override.render_command_reply( {"comment_id": 4, "kind": "routed", "user": "author"} ) - copilot_gated = dashboard_override.render_command_reply({ + gate_held = dashboard_override.render_command_reply({ "comment_id": 5, "kind": "routed", - "route": "copilot", + "held_for_gates": True, "user": "author", }) @@ -171,18 +171,18 @@ def test_renders_command_replies(self) -> None: self.assertIn(dashboard_override.command_reply_marker(4), routed) self.assertIn(dashboard_override.override_ack_marker(4), routed) self.assertIn("@author routed this pull request to reviewers.", routed) - self.assertIn(dashboard_override.command_reply_marker(5), copilot_gated) + self.assertIn(dashboard_override.command_reply_marker(5), gate_held) self.assertIn( "@author accepted the reviewer-routing override; the reviewer " - "handoff is waiting on Copilot.", - copilot_gated, + "handoff is waiting on the required status checks and the Copilot " + "review.", + gate_held, ) def test_renders_already_routed_replies_per_route(self) -> None: cases = { "approver": "already waiting on reviewers", "maintainer": "already past review and waiting on maintainers", - "copilot": "waiting on an automated Copilot review", } for route, phrase in cases.items(): with self.subTest(route=route): @@ -482,7 +482,6 @@ def test_command_only_overrides_pre_review_routes(self) -> None: ("author", "approver", True, False), ("approver", "approver", False, True), ("maintainer", "maintainer", False, True), - ("copilot", "copilot", False, True), ): with self.subTest(route=route): facts = { @@ -590,11 +589,12 @@ def test_does_not_create_label_without_pending_command(self, _load_state, run_gh return_value={ "prs": { "7": { - "route": "copilot", + "route": "author", "facts": { "dashboard_override_requested": True, "dashboard_override_command_id": 3, "dashboard_override_command_user": "author", + "route_held_for_gates": True, } }, "8": {"facts": {"dashboard_override_requested": False}}, @@ -633,7 +633,7 @@ def test_delivers_pending_override_label_and_acknowledges_command( call([ "gh", "api", "--method", "POST", "repos/open-telemetry/example/issues/7/comments", - "-f", "body=\n\n@author accepted the reviewer-routing override; the reviewer handoff is waiting on Copilot.\n", + "-f", "body=\n\n@author accepted the reviewer-routing override; the reviewer handoff is waiting on the required status checks and the Copilot review.\n", ]), ], run_gh.call_args_list, diff --git a/.github/scripts/pull-request-dashboard/test_notify_slack.py b/.github/scripts/pull-request-dashboard/test_notify_slack.py index 93165363e74..cb4b0c5a428 100644 --- a/.github/scripts/pull-request-dashboard/test_notify_slack.py +++ b/.github/scripts/pull-request-dashboard/test_notify_slack.py @@ -10,12 +10,13 @@ class NotifySlackTest(unittest.TestCase): @patch("notifications.send_slack_notification") - def test_copilot_route_does_not_notify_reviewers(self, send_notification) -> None: + def test_gate_held_pr_does_not_notify_reviewers(self, send_notification) -> None: results = { 7: { "pr_number": 7, - "route": "copilot", + "route": "author", "facts": { + "route_held_for_gates": True, "reviewers": [{"login": "reviewer"}], "waiting_since": "2026-07-20T01:00:00Z", }, 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 65d4ae18c24..50ff5695b1b 100644 --- a/.github/scripts/pull-request-dashboard/test_pr_status_comment.py +++ b/.github/scripts/pull-request-dashboard/test_pr_status_comment.py @@ -485,7 +485,6 @@ def test_routes_render_one_status_sentence(self) -> None: expected_summaries = { "approver": ("Waiting on reviewers", "Review the latest changes."), "maintainer": ("Waiting on maintainers", "Merge when ready."), - "copilot": ("Waiting on Copilot", "Wait for the pending review to complete."), "transient-failure": ("Waiting on the pull request dashboard maintainers", "Determine the next action."), "unknown": ("Waiting on the pull request dashboard maintainers", "Determine the next action."), } diff --git a/.github/scripts/pull-request-dashboard/test_route_presentation.py b/.github/scripts/pull-request-dashboard/test_route_presentation.py index ec61977b5c6..f2a1d47e1d9 100644 --- a/.github/scripts/pull-request-dashboard/test_route_presentation.py +++ b/.github/scripts/pull-request-dashboard/test_route_presentation.py @@ -31,12 +31,6 @@ def test_author_status_does_not_mention_login(self) -> None: route_status_summary("author"), ) - def test_copilot_status_identifies_pending_review(self) -> None: - self.assertEqual( - ("Copilot", "Wait for the pending review to complete."), - route_status_summary("copilot"), - ) - def test_unrecognized_route_uses_unknown_presentation(self) -> None: self.assertEqual(route_label("unknown"), route_label("other")) self.assertEqual(route_status_summary("unknown"), route_status_summary("other")) From e0e435893a6910ddf0ef8a6381f2e9fbfe5a6c21 Mon Sep 17 00:00:00 2001 From: Trask Stalnaker Date: Thu, 30 Jul 2026 10:36:38 -0700 Subject: [PATCH 05/16] Read every page of check suites for the head commit --- .../pull-request-dashboard/github_cli.py | 18 ++++++++------- .../pull-request-dashboard/test_github_cli.py | 23 ++++++++++++------- 2 files changed, 25 insertions(+), 16 deletions(-) diff --git a/.github/scripts/pull-request-dashboard/github_cli.py b/.github/scripts/pull-request-dashboard/github_cli.py index 0e843ceafe4..465917a66f9 100644 --- a/.github/scripts/pull-request-dashboard/github_cli.py +++ b/.github/scripts/pull-request-dashboard/github_cli.py @@ -563,18 +563,20 @@ def settled_check_suite_app_ids(repo: str, head_sha: str) -> set[int]: if not head_sha: return set() try: - payload = gh_api( - f"/repos/{repo}/commits/{head_sha}/check-suites?per_page=100" + pages = gh_api( + f"/repos/{repo}/commits/{head_sha}/check-suites?per_page=100", + paginate=True, ) except RuntimeError: return set() settled: dict[int, bool] = {} - for suite in (payload or {}).get("check_suites") or []: - app_id = (suite.get("app") or {}).get("id") - if app_id is None: - continue - completed = suite.get("status") == "completed" - settled[app_id] = settled.get(app_id, True) and completed + for page in pages or []: + for suite in page.get("check_suites") or []: + app_id = (suite.get("app") or {}).get("id") + if app_id is None: + continue + completed = suite.get("status") == "completed" + settled[app_id] = settled.get(app_id, True) and completed return {app_id for app_id, completed in settled.items() if completed} diff --git a/.github/scripts/pull-request-dashboard/test_github_cli.py b/.github/scripts/pull-request-dashboard/test_github_cli.py index 7fca71392db..a30f3d23737 100644 --- a/.github/scripts/pull-request-dashboard/test_github_cli.py +++ b/.github/scripts/pull-request-dashboard/test_github_cli.py @@ -963,17 +963,24 @@ def test_unsettled_app_still_owes_its_missing_required_check(self) -> None: @patch("github_cli.gh_api") def test_settled_app_ids_require_every_suite_to_complete(self, gh_api) -> None: - gh_api.return_value = { - "check_suites": [ - {"status": "completed", "app": {"id": 15368}}, - {"status": "in_progress", "app": {"id": 15368}}, - {"status": "completed", "app": {"id": 17893}}, - ], - } + gh_api.return_value = [ + { + "check_suites": [ + {"status": "completed", "app": {"id": 15368}}, + {"status": "completed", "app": {"id": 17893}}, + ], + }, + { + "check_suites": [ + {"status": "in_progress", "app": {"id": 15368}}, + ], + }, + ] self.assertEqual({17893}, settled_check_suite_app_ids("owner/repo", "head")) gh_api.assert_called_once_with( - "/repos/owner/repo/commits/head/check-suites?per_page=100" + "/repos/owner/repo/commits/head/check-suites?per_page=100", + paginate=True, ) @patch("github_cli.gh_api") From 04c1867bcfe5fb2d655b6d6b00ef75889feb911c Mon Sep 17 00:00:00 2001 From: Trask Stalnaker Date: Thu, 30 Jul 2026 10:52:15 -0700 Subject: [PATCH 06/16] Hold a PR from advancing, not just from reaching reviewers An approver-routed PR is no longer handed to maintainers while the required checks or the Copilot review are outstanding; only movement back toward the author is always allowed. --- .../pull-request-dashboard/RATIONALE.md | 19 ++++++------- .../pull-request-dashboard/dashboard.py | 27 +++++++++++++------ .../pull-request-dashboard/test_dashboard.py | 15 ++++++++++- 3 files changed, 43 insertions(+), 18 deletions(-) diff --git a/.github/scripts/pull-request-dashboard/RATIONALE.md b/.github/scripts/pull-request-dashboard/RATIONALE.md index a87be01bcb6..5a3483d1417 100644 --- a/.github/scripts/pull-request-dashboard/RATIONALE.md +++ b/.github/scripts/pull-request-dashboard/RATIONALE.md @@ -200,14 +200,15 @@ the implementation understandable and operationally cheap. Repository-configured `non_blocking_check_patterns` identify failed optional checks in a note alongside this action, without changing required-check facts or routing. -- A PR the dashboard would hand to reviewers stays with its author until the - required checks report. Clearing the checks is the author's job, so an - outstanding one is not yet a reason to spend reviewer attention, and a push - clears the failing count before the replacement checks produce a result, so - the PR would otherwise be handed off on evidence that does not exist yet and - handed back minutes later when the same check fails. A PR that has already - reached reviewers is never pulled back, because a check result alone does not - make it the author's turn again. Unavailable check results hold the handoff +- A PR does not advance toward merge while the required checks are unsettled: + an author waiting on CI keeps the PR, and a PR already with approvers is not + handed to maintainers to merge. Clearing the checks is the author's job, so + an outstanding one is not yet a reason to spend anyone else's attention, and a + push clears the failing count before the replacement checks produce a result, + so the PR would otherwise move forward on evidence that does not exist yet and + move back minutes later when the same check fails. Moving back toward the + author is never held, because a failing check or new author-owned discussion + is evidence the gates cannot undo. Unavailable check results hold the handoff for the same reason a pending one does, and resolve on a later run. - A held PR is presented as waiting on its author rather than on the robot it is waiting for. The CI column and the reviewers column already name what is @@ -252,7 +253,7 @@ the implementation understandable and operationally cheap. gate or produces fresh actionable threads that route the PR back to the author, so re-requesting an unchanged commit is self-correcting rather than a re-request loop. -- An outstanding Copilot review holds the reviewer handoff the same way an +- An outstanding Copilot review holds the PR where it is the same way an unsettled required check does, including when a reviewer-routing override asked for the handoff. The override says the author is done with the discussion, not that the robots have finished, and a handoff made before they diff --git a/.github/scripts/pull-request-dashboard/dashboard.py b/.github/scripts/pull-request-dashboard/dashboard.py index eb1020e47d1..b0e8b22bf24 100644 --- a/.github/scripts/pull-request-dashboard/dashboard.py +++ b/.github/scripts/pull-request-dashboard/dashboard.py @@ -123,9 +123,10 @@ last_approver_activity_at str (iso) Stage 2 — add_wait_age_facts (depends on routing + pending actions): - route_held_for_gates bool Reviewer handoff withheld - because the required checks or - the Copilot review are still + route_held_for_gates bool The PR did not advance to the + route it computed, because + the required checks or the + Copilot review are still outstanding. waiting_since str (iso) Oldest pending discussion, or route-appropriate fallback, @@ -1161,6 +1162,14 @@ def oldest_pending_action_ts( # response, so an author push does not change whose turn it is. REVIEWER_ROUTES = ("approver", "maintainer") +# How far a PR has travelled toward merge. An unsettled gate stops it from +# advancing, but never from moving back toward its author. +ROUTE_PROGRESSION = ("author", "approver", "maintainer") + + +def route_progress(route: str) -> int: + return ROUTE_PROGRESSION.index(route) if route in ROUTE_PROGRESSION else 0 + def fallback_wait_ts(route: str, facts: dict[str, Any]) -> tuple[datetime | None, str]: if route in REVIEWER_ROUTES: @@ -1328,11 +1337,13 @@ def hold_route_until_gates_settle( require_clean_copilot_review: bool, ) -> str: # The required checks and the Copilot review are the author's to clear, so - # an outstanding one keeps the PR with them. A PR that already reached - # reviewers is never pulled back, so a push cannot change whose turn it is. + # a PR does not advance while one is outstanding. Moving back toward the + # author is always allowed, because those are decisions a gate cannot undo. + previous_route = (previous_result or {}).get("route") or "" + if previous_route not in ROUTE_PROGRESSION: + previous_route = "author" held = ( - route in REVIEWER_ROUTES - and (previous_result or {}).get("route") not in REVIEWER_ROUTES + route_progress(route) > route_progress(previous_route) and ( not required_checks_settled(facts) or copilot_review_outstanding( @@ -1341,7 +1352,7 @@ def hold_route_until_gates_settle( ) ) facts["route_held_for_gates"] = held - return "author" if held else route + return previous_route if held else route def resolve_pr_route( diff --git a/.github/scripts/pull-request-dashboard/test_dashboard.py b/.github/scripts/pull-request-dashboard/test_dashboard.py index 119004e06d8..d6a18588134 100644 --- a/.github/scripts/pull-request-dashboard/test_dashboard.py +++ b/.github/scripts/pull-request-dashboard/test_dashboard.py @@ -129,9 +129,22 @@ def test_settled_checks_release_the_pr_to_reviewers(self) -> None: self.assertEqual("approver", route) - def test_reviewer_route_moves_on_while_checks_run(self) -> None: + def test_reviewers_do_not_hand_off_to_maintainers_while_checks_run(self) -> None: route = self._hold({"ci_pending_count": 1}, "maintainer", {"route": "approver"}) + self.assertEqual("approver", route) + + def test_a_pr_still_moves_back_to_its_author_while_checks_run(self) -> None: + facts: dict[str, object] = {"ci_pending_count": 1, "ci_failing_count": 1} + + route = self._hold(facts, "author", {"route": "maintainer"}) + + self.assertEqual("author", route) + self.assertFalse(facts["route_held_for_gates"]) + + def test_an_unchanged_route_is_not_held(self) -> None: + route = self._hold({"ci_pending_count": 1}, "maintainer", {"route": "maintainer"}) + self.assertEqual("maintainer", route) def test_a_pr_that_never_reached_reviewers_is_held(self) -> None: From 09c97d859fb0f0abb1de297c890d0d1090fc00db Mon Sep 17 00:00:00 2001 From: Trask Stalnaker Date: Thu, 30 Jul 2026 10:57:24 -0700 Subject: [PATCH 07/16] Show an outstanding Copilot review as a pending reviewer --- .../pull-request-dashboard/RATIONALE.md | 11 ++++--- .../pull-request-dashboard/dashboard.py | 12 ++++++-- .../scripts/pull-request-dashboard/render.py | 26 +++++++++++++++-- .../pull-request-dashboard/test_render.py | 29 +++++++++++++++++-- 4 files changed, 66 insertions(+), 12 deletions(-) diff --git a/.github/scripts/pull-request-dashboard/RATIONALE.md b/.github/scripts/pull-request-dashboard/RATIONALE.md index 5a3483d1417..6a43c637ede 100644 --- a/.github/scripts/pull-request-dashboard/RATIONALE.md +++ b/.github/scripts/pull-request-dashboard/RATIONALE.md @@ -211,10 +211,13 @@ the implementation understandable and operationally cheap. is evidence the gates cannot undo. Unavailable check results hold the handoff for the same reason a pending one does, and resolve on a later run. - A held PR is presented as waiting on its author rather than on the robot it - is waiting for. The CI column and the reviewers column already name what is - outstanding, and the live status comment tells the author the handoff happens - once those are clean, so a separate route would add a section that nobody is - expected to act on. + is waiting for, so a separate route would add a section that nobody is + expected to act on. What it waits for is named in the columns instead: the CI + column already shows running checks, and an outstanding Copilot review is + listed in the reviewers column with the same pending icon. Copilot otherwise + joins that column only once it has reviewed, so without this the Copilot gate + would hold a PR with nothing on the row to explain why. The live status + comment tells the author the handoff happens once both are clean. - A held route also holds its wait age. Recomputing it would read the push as the end of the CI failure and fall back to the last approver activity, which is usually far older, so a PR the author had just pushed to would sort to the diff --git a/.github/scripts/pull-request-dashboard/dashboard.py b/.github/scripts/pull-request-dashboard/dashboard.py index b0e8b22bf24..4966b56d04b 100644 --- a/.github/scripts/pull-request-dashboard/dashboard.py +++ b/.github/scripts/pull-request-dashboard/dashboard.py @@ -123,6 +123,11 @@ last_approver_activity_at str (iso) Stage 2 — add_wait_age_facts (depends on routing + pending actions): + copilot_review_outstanding bool The Copilot review gate applies + to this PR and its review is + missing or stale, so the + reviewers column shows Copilot + as pending. route_held_for_gates bool The PR did not advance to the route it computed, because the required checks or the @@ -1342,13 +1347,14 @@ def hold_route_until_gates_settle( previous_route = (previous_result or {}).get("route") or "" if previous_route not in ROUTE_PROGRESSION: previous_route = "author" + facts["copilot_review_outstanding"] = copilot_review_outstanding( + facts, enabled=require_clean_copilot_review + ) held = ( route_progress(route) > route_progress(previous_route) and ( not required_checks_settled(facts) - or copilot_review_outstanding( - facts, enabled=require_clean_copilot_review - ) + or facts["copilot_review_outstanding"] ) ) facts["route_held_for_gates"] = held diff --git a/.github/scripts/pull-request-dashboard/render.py b/.github/scripts/pull-request-dashboard/render.py index c0a5428f833..66f4903314a 100644 --- a/.github/scripts/pull-request-dashboard/render.py +++ b/.github/scripts/pull-request-dashboard/render.py @@ -111,6 +111,8 @@ def age_cell(facts: dict[str, Any]) -> str: def reviewer_icon(reviewer: dict[str, Any]) -> str: discussion_icons = [] + if reviewer.get("pending_review"): + discussion_icons.append("⏳") if reviewer.get("open_thread"): discussion_icons.append("💬") if reviewer.get("top_level_feedback"): @@ -138,10 +140,27 @@ def reviewer_display_name(login: str) -> str: return REVIEWER_DISPLAY_NAMES.get(login, login) +COPILOT_REVIEWER_LOGIN = "copilot-pull-request-reviewer" + + +def display_reviewers(facts: dict[str, Any]) -> list[dict[str, Any]]: + # Copilot only joins the reviewer list once it has reviewed, so an + # outstanding review has to be added for the wait to be visible at all. + reviewers = [dict(reviewer) for reviewer in facts.get("reviewers") or []] + if not facts.get("copilot_review_outstanding"): + return reviewers + for reviewer in reviewers: + if reviewer_display_name(reviewer.get("login") or "") == "Copilot": + reviewer["pending_review"] = True + return reviewers + reviewers.append({"login": COPILOT_REVIEWER_LOGIN, "pending_review": True}) + reviewers.sort(key=lambda reviewer: str(reviewer.get("login") or "").lower()) + return reviewers + + def reviewers_cell_text(facts: dict[str, Any]) -> str: - reviewers = facts.get("reviewers") or [] parts = [] - for reviewer in reviewers: + for reviewer in display_reviewers(facts): login = markdown_escape(reviewer_display_name(reviewer.get("login") or "")) if not login: continue @@ -264,7 +283,8 @@ def render_pr_tables( ) reviewers_note = ( "Reviewers column: ✅ approved · ✔️ approved (non-code-owner) · " - "💬 open review thread · 📌 top-level feedback needs author action · 🔴 changes requested." + "⏳ review pending · 💬 open review thread · 📌 top-level feedback needs author action · " + "🔴 changes requested." ) out: list[str] = [ "> [!NOTE]", diff --git a/.github/scripts/pull-request-dashboard/test_render.py b/.github/scripts/pull-request-dashboard/test_render.py index 17a913aa73c..a2a935cfb91 100644 --- a/.github/scripts/pull-request-dashboard/test_render.py +++ b/.github/scripts/pull-request-dashboard/test_render.py @@ -2,7 +2,7 @@ import unittest -from render import render_diagnostics_section, render_pr_tables +from render import render_diagnostics_section, render_pr_tables, reviewers_cell_text class RenderTest(unittest.TestCase): @@ -122,10 +122,35 @@ def test_reviewer_legend_includes_top_level_feedback(self) -> None: markdown = render_pr_tables([], {}) self.assertIn( - "💬 open review thread · 📌 top-level feedback needs author action · 🔴 changes requested.", + "⏳ review pending · 💬 open review thread · " + "📌 top-level feedback needs author action · 🔴 changes requested.", markdown, ) + def test_outstanding_copilot_review_is_listed_as_pending(self) -> None: + cell = reviewers_cell_text({ + "reviewers": [{"login": "reviewer", "approved": True}], + "copilot_review_outstanding": True, + }) + + self.assertEqual("Copilot ⏳
reviewer ✅", cell) + + def test_outstanding_copilot_review_marks_its_existing_entry(self) -> None: + cell = reviewers_cell_text({ + "reviewers": [{"login": "copilot-pull-request-reviewer", "open_thread": True}], + "copilot_review_outstanding": True, + }) + + self.assertEqual("Copilot ⏳\u2060💬", cell) + + def test_clean_copilot_review_is_not_listed_as_pending(self) -> None: + cell = reviewers_cell_text({ + "reviewers": [{"login": "reviewer", "approved": True}], + "copilot_review_outstanding": False, + }) + + self.assertEqual("reviewer ✅", cell) + def test_dashboard_does_not_claim_approvers_can_force_refresh(self) -> None: markdown = render_pr_tables([], {}) From 97aee51e1ca12b212a88a192bbab889eda3f9e74 Mon Sep 17 00:00:00 2001 From: Trask Stalnaker Date: Thu, 30 Jul 2026 11:10:01 -0700 Subject: [PATCH 08/16] Address review comment from copilot-pull-request-reviewer: keep the dashboard state and migrate the retired route --- .../scripts/pull-request-dashboard/state.py | 13 +++++++-- .../pull-request-dashboard/test_state.py | 27 ++++++++++++++++++- 2 files changed, 37 insertions(+), 3 deletions(-) diff --git a/.github/scripts/pull-request-dashboard/state.py b/.github/scripts/pull-request-dashboard/state.py index e1095a33c23..5ad499037dc 100644 --- a/.github/scripts/pull-request-dashboard/state.py +++ b/.github/scripts/pull-request-dashboard/state.py @@ -23,7 +23,7 @@ # current vector, ordinary state loaders may regenerate mismatched disposable # caches. Every constant ending in _STATE_VERSION or _REVISION is included. # dashboard-state.json: accepted PR routing results and backfill readiness. -DASHBOARD_STATE_VERSION = 7 +DASHBOARD_STATE_VERSION = 6 # backfill-state.json: round-robin cursor used by full dashboard refreshes. BACKFILL_STATE_VERSION = 3 # notification-state.json: pending and delivered Slack notification records. @@ -263,15 +263,24 @@ def enqueue_status_comment_update(pr_number: int) -> None: save_status_comment_rollout_state(state) +def migrated_pr_record(record: Any) -> Any: + # The retired copilot route held a PR while its review was outstanding, + # which is now part of the author's turn. + if not isinstance(record, dict) or record.get("route") != "copilot": + return record + return {**record, "route": "author"} + + def load_dashboard_state_cache() -> dict[str, Any] | None: state = load_state_file(dashboard_state_path(), DASHBOARD_STATE_VERSION) if state is None: return None prs = state.get("prs") + prs = prs if isinstance(prs, dict) else {} return { "version": DASHBOARD_STATE_VERSION, INITIAL_BACKFILL_COMPLETE_KEY: initial_backfill_complete(state), - "prs": prs if isinstance(prs, dict) else {}, + "prs": {number: migrated_pr_record(record) for number, record in prs.items()}, } diff --git a/.github/scripts/pull-request-dashboard/test_state.py b/.github/scripts/pull-request-dashboard/test_state.py index 20021070a1b..67d87108364 100644 --- a/.github/scripts/pull-request-dashboard/test_state.py +++ b/.github/scripts/pull-request-dashboard/test_state.py @@ -148,6 +148,31 @@ def test_state_specific_loaders_own_payload_defaults(self) -> None: {"version": BACKFILL_STATE_VERSION, "cursor": {}}, ) + def test_dashboard_state_migrates_the_retired_copilot_route(self) -> None: + with tempfile.TemporaryDirectory() as temp_dir, patch("state._state_dir", Path(temp_dir)): + dashboard_state_path().write_text( + json.dumps({ + "version": DASHBOARD_STATE_VERSION, + "initial_backfill_complete": True, + "prs": { + "1": {"route": "copilot", "facts": {"waiting_since": "2026-07-01T00:00:00Z"}}, + "2": {"route": "approver"}, + }, + }), + encoding="utf-8", + ) + + state = load_dashboard_state_cache() + + self.assertTrue(state["initial_backfill_complete"]) + self.assertEqual( + state["prs"], + { + "1": {"route": "author", "facts": {"waiting_since": "2026-07-01T00:00:00Z"}}, + "2": {"route": "approver"}, + }, + ) + def test_dashboard_state_save_writes_explicit_shape(self) -> None: with tempfile.TemporaryDirectory() as temp_dir, patch("state._state_dir", Path(temp_dir)): save_dashboard_state_cache({"unknown": "discard me"}) @@ -164,7 +189,7 @@ def test_dashboard_state_save_writes_explicit_shape(self) -> None: def test_notification_state_version_is_independent(self) -> None: self.assertEqual(BACKFILL_STATE_VERSION, 3) self.assertEqual(NOTIFICATION_STATE_VERSION, 3) - self.assertEqual(DASHBOARD_STATE_VERSION, 7) + self.assertEqual(DASHBOARD_STATE_VERSION, 6) self.assertEqual(STATUS_COMMENT_ROLLOUT_STATE_VERSION, 1) self.assertEqual(AUTHOR_NUDGE_STATE_VERSION, 2) self.assertEqual(COPILOT_REVIEW_REQUEST_STATE_VERSION, 4) From 11a5facaa12808b39d0b7b7723b3182e110b9716 Mon Sep 17 00:00:00 2001 From: Trask Stalnaker Date: Thu, 30 Jul 2026 11:25:15 -0700 Subject: [PATCH 09/16] Address low-confidence review findings Never route a held maintenance-bot PR to its author, name only the gates that are actually outstanding, recognize the short Copilot login, and reject a check rollup whose pages describe different commits. --- .../pull-request-dashboard/RATIONALE.md | 3 +- .../pull-request-dashboard/dashboard.py | 10 ++++- .../dashboard_override.py | 13 ++++-- .../pull-request-dashboard/github_cli.py | 6 ++- .../pr_status_comment.py | 20 ++++++--- .../scripts/pull-request-dashboard/render.py | 1 + .../route_presentation.py | 14 +++++++ .../pull-request-dashboard/test_dashboard.py | 9 ++++ .../test_dashboard_override.py | 9 ++-- .../pull-request-dashboard/test_github_cli.py | 42 +++++++++++++++++++ .../test_pr_status_comment.py | 32 ++++++++++++++ .../pull-request-dashboard/test_render.py | 8 ++++ 12 files changed, 149 insertions(+), 18 deletions(-) diff --git a/.github/scripts/pull-request-dashboard/RATIONALE.md b/.github/scripts/pull-request-dashboard/RATIONALE.md index 6a43c637ede..d66fd78de9b 100644 --- a/.github/scripts/pull-request-dashboard/RATIONALE.md +++ b/.github/scripts/pull-request-dashboard/RATIONALE.md @@ -230,7 +230,8 @@ the implementation understandable and operationally cheap. the PR in front of reviewers. - Maintenance-bot PRs retain maintainer-oriented routing because the bot cannot respond to a dashboard action. Pending required checks affect the CI column - but do not change who owns the next action. + but never route one of these PRs to its author: a bot PR whose handoff is + held waits on reviewers instead. ## Copilot Review Gate diff --git a/.github/scripts/pull-request-dashboard/dashboard.py b/.github/scripts/pull-request-dashboard/dashboard.py index 4966b56d04b..bbacb379b55 100644 --- a/.github/scripts/pull-request-dashboard/dashboard.py +++ b/.github/scripts/pull-request-dashboard/dashboard.py @@ -133,6 +133,10 @@ the required checks or the Copilot review are still outstanding. + required_checks_settled bool Every required check has + reported on the current head, + so the computed route is not + provisional. waiting_since str (iso) Oldest pending discussion, or route-appropriate fallback, or PR creation time. Carried @@ -1346,14 +1350,16 @@ def hold_route_until_gates_settle( # author is always allowed, because those are decisions a gate cannot undo. previous_route = (previous_result or {}).get("route") or "" if previous_route not in ROUTE_PROGRESSION: - previous_route = "author" + # A maintenance bot has no author route to fall back to. + previous_route = "approver" if facts.get("is_maintenance_bot") else "author" facts["copilot_review_outstanding"] = copilot_review_outstanding( facts, enabled=require_clean_copilot_review ) + facts["required_checks_settled"] = required_checks_settled(facts) held = ( route_progress(route) > route_progress(previous_route) and ( - not required_checks_settled(facts) + not facts["required_checks_settled"] or facts["copilot_review_outstanding"] ) ) diff --git a/.github/scripts/pull-request-dashboard/dashboard_override.py b/.github/scripts/pull-request-dashboard/dashboard_override.py index 1b3fc446786..e7b2d28ff54 100644 --- a/.github/scripts/pull-request-dashboard/dashboard_override.py +++ b/.github/scripts/pull-request-dashboard/dashboard_override.py @@ -7,6 +7,7 @@ from urllib.parse import quote from github_cli import gh_api, run_gh +from route_presentation import outstanding_gate_phrase from state import load_dashboard_state_cache from utils import actor_login @@ -231,11 +232,11 @@ def render_command_reply(reply: dict[str, Any]) -> str: "use `/dashboard route:reviewers`." ) elif kind == "routed": - if reply.get("held_for_gates"): + held_gates = reply.get("held_gates") or "" + if held_gates: message = ( "accepted the reviewer-routing override; the reviewer handoff " - "is waiting on the required status checks and the Copilot " - "review." + f"is waiting on {held_gates}." ) else: message = "routed this pull request to reviewers." @@ -422,7 +423,11 @@ def deliver_dashboard_override_requests(repo: str) -> list[str]: { "comment_id": facts["dashboard_override_command_id"], "kind": "routed", - "held_for_gates": bool(facts.get("route_held_for_gates")), + "held_gates": ( + outstanding_gate_phrase(facts) + if facts.get("route_held_for_gates") + else "" + ), "user": facts.get("dashboard_override_command_user") or "", }, ) diff --git a/.github/scripts/pull-request-dashboard/github_cli.py b/.github/scripts/pull-request-dashboard/github_cli.py index 465917a66f9..17d9ecd7ef2 100644 --- a/.github/scripts/pull-request-dashboard/github_cli.py +++ b/.github/scripts/pull-request-dashboard/github_cli.py @@ -424,7 +424,11 @@ def gh_pr_check_rollup( commits = pull_request.get("commits") or {} commit_nodes = commits.get("nodes") or [] commit = (commit_nodes[0] if commit_nodes else {}).get("commit") or {} - head_oid = commit.get("oid") or head_oid + page_oid = commit.get("oid") or "" + if head_oid and page_oid and page_oid != head_oid: + # The pages describe two different commits, so neither is whole. + return None + head_oid = page_oid or head_oid rollup = commit.get("statusCheckRollup") or {} contexts = rollup.get("contexts") or {} for node in contexts.get("nodes") or []: diff --git a/.github/scripts/pull-request-dashboard/pr_status_comment.py b/.github/scripts/pull-request-dashboard/pr_status_comment.py index d4daa60f34e..49c76f4e0a8 100644 --- a/.github/scripts/pull-request-dashboard/pr_status_comment.py +++ b/.github/scripts/pull-request-dashboard/pr_status_comment.py @@ -13,7 +13,11 @@ run_gh, ) from dashboard_override import PRE_REVIEW_ROUTES -from route_presentation import route_status_summary, status_headline +from route_presentation import ( + outstanding_gate_phrase, + route_status_summary, + status_headline, +) from state import ( STATUS_COMMENT_REVISION, load_dashboard_state_cache, @@ -185,7 +189,7 @@ def author_body( non_blocking_failure_note: str, review_thread_urls: list[str], top_level_feedback_urls: list[str], - held_for_gates: bool = False, + held_gates: str = "", ) -> list[str]: noun = "item" if feedback_count == 1 else "items" if failing_count and feedback_count: @@ -214,10 +218,10 @@ def author_body( if non_blocking_failure_note: sentence += f" Note: {non_blocking_failure_note}" return [sentence] - if held_for_gates: + if held_gates: return [ - "Wait for the required status checks and the Copilot review to " - "finish; this pull request moves to reviewers once they are clean." + f"Wait for {held_gates} to finish; this pull request moves to " + "reviewers once the results are clean." ] _, fallback_next_step = route_status_summary("author") return [fallback_next_step] @@ -266,7 +270,11 @@ def render_status_comment( ), review_thread_urls=review_thread_urls, top_level_feedback_urls=top_level_feedback_urls, - held_for_gates=bool(facts.get("route_held_for_gates")), + held_gates=( + outstanding_gate_phrase(facts) + if facts.get("route_held_for_gates") + else "" + ), ) else: _, next_step = route_status_summary(route) diff --git a/.github/scripts/pull-request-dashboard/render.py b/.github/scripts/pull-request-dashboard/render.py index 66f4903314a..d35d995ce14 100644 --- a/.github/scripts/pull-request-dashboard/render.py +++ b/.github/scripts/pull-request-dashboard/render.py @@ -131,6 +131,7 @@ def reviewer_icon(reviewer: dict[str, Any]) -> str: # Friendlier display names for bot reviewers whose login is verbose. REVIEWER_DISPLAY_NAMES = { + "copilot": "Copilot", "copilot-pull-request-reviewer": "Copilot", "copilot-pull-request-reviewer[bot]": "Copilot", } diff --git a/.github/scripts/pull-request-dashboard/route_presentation.py b/.github/scripts/pull-request-dashboard/route_presentation.py index e9cedf3e2fd..7bbe6c6cc59 100644 --- a/.github/scripts/pull-request-dashboard/route_presentation.py +++ b/.github/scripts/pull-request-dashboard/route_presentation.py @@ -1,5 +1,7 @@ from __future__ import annotations +from typing import Any + ROUTE_PRESENTATION = { "maintainer": { @@ -50,3 +52,15 @@ def route_status_summary(route: str) -> tuple[str, str]: def status_headline(route: str) -> str: return ROUTE_PRESENTATION.get(route, ROUTE_PRESENTATION["unknown"])["status_headline"] + + +def outstanding_gate_phrase(facts: dict[str, Any]) -> str: + # Only one gate has to be outstanding for a PR to be held, and a branch + # without the Copilot gate never has that one, so naming both would tell + # the author to wait for work that is finished or never happens. + gates = [] + if not facts.get("required_checks_settled"): + gates.append("the required status checks") + if facts.get("copilot_review_outstanding"): + gates.append("the Copilot review") + return " and ".join(gates) diff --git a/.github/scripts/pull-request-dashboard/test_dashboard.py b/.github/scripts/pull-request-dashboard/test_dashboard.py index d6a18588134..e8664aebb84 100644 --- a/.github/scripts/pull-request-dashboard/test_dashboard.py +++ b/.github/scripts/pull-request-dashboard/test_dashboard.py @@ -152,6 +152,15 @@ def test_a_pr_that_never_reached_reviewers_is_held(self) -> None: self.assertEqual("author", route) + def test_a_held_maintenance_bot_pr_is_never_routed_to_its_author(self) -> None: + route = self._hold( + {"ci_pending_count": 1, "is_maintenance_bot": True}, + "maintainer", + None, + ) + + self.assertEqual("approver", route) + def test_held_route_carries_the_previous_wait_forward(self) -> None: facts = { "route_held_for_gates": True, diff --git a/.github/scripts/pull-request-dashboard/test_dashboard_override.py b/.github/scripts/pull-request-dashboard/test_dashboard_override.py index 62abe6d9750..8e912933fd8 100644 --- a/.github/scripts/pull-request-dashboard/test_dashboard_override.py +++ b/.github/scripts/pull-request-dashboard/test_dashboard_override.py @@ -153,7 +153,7 @@ def test_renders_command_replies(self) -> None: gate_held = dashboard_override.render_command_reply({ "comment_id": 5, "kind": "routed", - "held_for_gates": True, + "held_gates": "the required status checks", "user": "author", }) @@ -174,8 +174,7 @@ def test_renders_command_replies(self) -> None: self.assertIn(dashboard_override.command_reply_marker(5), gate_held) self.assertIn( "@author accepted the reviewer-routing override; the reviewer " - "handoff is waiting on the required status checks and the Copilot " - "review.", + "handoff is waiting on the required status checks.", gate_held, ) @@ -595,6 +594,8 @@ def test_does_not_create_label_without_pending_command(self, _load_state, run_gh "dashboard_override_command_id": 3, "dashboard_override_command_user": "author", "route_held_for_gates": True, + "required_checks_settled": True, + "copilot_review_outstanding": True, } }, "8": {"facts": {"dashboard_override_requested": False}}, @@ -633,7 +634,7 @@ def test_delivers_pending_override_label_and_acknowledges_command( call([ "gh", "api", "--method", "POST", "repos/open-telemetry/example/issues/7/comments", - "-f", "body=\n\n@author accepted the reviewer-routing override; the reviewer handoff is waiting on the required status checks and the Copilot review.\n", + "-f", "body=\n\n@author accepted the reviewer-routing override; the reviewer handoff is waiting on the Copilot review.\n", ]), ], run_gh.call_args_list, diff --git a/.github/scripts/pull-request-dashboard/test_github_cli.py b/.github/scripts/pull-request-dashboard/test_github_cli.py index a30f3d23737..bc3cc984214 100644 --- a/.github/scripts/pull-request-dashboard/test_github_cli.py +++ b/.github/scripts/pull-request-dashboard/test_github_cli.py @@ -479,6 +479,48 @@ def test_check_rollup_separates_configured_non_blocking_failures(self, graphql) [check["name"] for check in rollup["non_blocking_failures"]], ) + @patch("github_cli.gh_graphql") + def test_check_rollup_rejects_pages_from_different_commits(self, graphql) -> None: + def page(oid: str, name: str, has_next: bool) -> dict: + return { + "data": { + "node": { + "commits": { + "nodes": [{ + "commit": { + "oid": oid, + "statusCheckRollup": { + "contexts": { + "nodes": [{ + "__typename": "CheckRun", + "name": name, + "status": "COMPLETED", + "conclusion": "SUCCESS", + "url": "https://github.com/open-telemetry/example/runs/1", + "isRequired": True, + }], + "pageInfo": { + "hasNextPage": has_next, + "endCursor": "cursor-1", + }, + }, + }, + }, + }], + }, + }, + }, + } + + graphql.side_effect = [ + page("stale-head", "build", True), + page("current-head", "test", False), + ] + + rollup = gh_pr_check_rollup("open-telemetry/example", "PR_id", []) + + self.assertIsNone(rollup) + @patch("github_cli.gh_graphql") def test_check_rollup_keeps_required_and_non_blocking_attempts_separate( self, 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 50ff5695b1b..7973c0365e2 100644 --- a/.github/scripts/pull-request-dashboard/test_pr_status_comment.py +++ b/.github/scripts/pull-request-dashboard/test_pr_status_comment.py @@ -226,6 +226,38 @@ def test_waiting_on_author_names_required_ci_failure(self) -> None: self.assertNotIn("### Review feedback", body) self.assertNotIn(pr_status_comment.RESPONSE_EXAMPLES, body) + def test_held_pr_names_only_the_outstanding_check_gate(self) -> None: + body = pr_status_comment.render_status_comment( + self.pr(), + { + "route": "author", + "facts": { + "author": "alice", + "route_held_for_gates": True, + "required_checks_settled": False, + "copilot_review_outstanding": False, + }, + }, + ) + + self.assertIn("Wait for the required status checks to finish;", body) + + def test_held_pr_names_only_the_outstanding_copilot_gate(self) -> None: + body = pr_status_comment.render_status_comment( + self.pr(), + { + "route": "author", + "facts": { + "author": "alice", + "route_held_for_gates": True, + "required_checks_settled": True, + "copilot_review_outstanding": True, + }, + }, + ) + + self.assertIn("Wait for the Copilot review to finish;", body) + def test_waiting_on_author_combines_ci_and_review_feedback_reasons(self) -> None: body = pr_status_comment.render_status_comment( self.pr(), diff --git a/.github/scripts/pull-request-dashboard/test_render.py b/.github/scripts/pull-request-dashboard/test_render.py index a2a935cfb91..506eaf26c30 100644 --- a/.github/scripts/pull-request-dashboard/test_render.py +++ b/.github/scripts/pull-request-dashboard/test_render.py @@ -143,6 +143,14 @@ def test_outstanding_copilot_review_marks_its_existing_entry(self) -> None: self.assertEqual("Copilot ⏳\u2060💬", cell) + def test_outstanding_copilot_review_marks_its_short_login_entry(self) -> None: + cell = reviewers_cell_text({ + "reviewers": [{"login": "copilot", "open_thread": True}], + "copilot_review_outstanding": True, + }) + + self.assertEqual("Copilot ⏳\u2060💬", cell) + def test_clean_copilot_review_is_not_listed_as_pending(self) -> None: cell = reviewers_cell_text({ "reviewers": [{"login": "reviewer", "approved": True}], From ad34da0fd30745f8f5ec00b6ed4b187448bccd96 Mon Sep 17 00:00:00 2001 From: Trask Stalnaker Date: Thu, 30 Jul 2026 12:37:45 -0700 Subject: [PATCH 10/16] Address low-confidence review findings Roll out the status comment change, and stop reminding people while a PR is held for a robot gate. --- .../scripts/pull-request-dashboard/RATIONALE.md | 5 +++++ .../pull-request-dashboard/author_nudge.py | 14 ++++++++++++-- .../scripts/pull-request-dashboard/dashboard.py | 4 +++- .../pull-request-dashboard/notifications.py | 7 +++++++ .github/scripts/pull-request-dashboard/state.py | 2 +- .../pull-request-dashboard/test_author_nudge.py | 13 +++++++++++++ .../pull-request-dashboard/test_dashboard.py | 15 +++++++++++++++ .../pull-request-dashboard/test_notify_slack.py | 2 +- 8 files changed, 57 insertions(+), 5 deletions(-) diff --git a/.github/scripts/pull-request-dashboard/RATIONALE.md b/.github/scripts/pull-request-dashboard/RATIONALE.md index d66fd78de9b..81eec72e6c0 100644 --- a/.github/scripts/pull-request-dashboard/RATIONALE.md +++ b/.github/scripts/pull-request-dashboard/RATIONALE.md @@ -222,6 +222,11 @@ the implementation understandable and operationally cheap. the end of the CI failure and fall back to the last approver activity, which is usually far older, so a PR the author had just pushed to would sort to the top of the waiting-on-authors section as the stalest item on the board. +- A held PR sends no reminder in either direction. The author nudge and the + reviewer Slack notification both ask a person to respond, and while the hold + lasts the response is owed by a robot that is already working. The author's + waiting episode ends when the hold begins, so a later handoff back to the + author starts a fresh one instead of resuming a wait the author has answered. - While a PR stays on a route where someone other than the author owes it a response, its wait age only moves back, never forward. The fallback for those routes is the last author activity, so a push would otherwise restart the diff --git a/.github/scripts/pull-request-dashboard/author_nudge.py b/.github/scripts/pull-request-dashboard/author_nudge.py index 4be719cb44a..b21454ea8ef 100644 --- a/.github/scripts/pull-request-dashboard/author_nudge.py +++ b/.github/scripts/pull-request-dashboard/author_nudge.py @@ -104,6 +104,16 @@ def fetch_current_pr_routing_inputs( return raw["pr"], raw +def waiting_on_author(result: dict[str, Any] | None) -> bool: + # A held PR shows the author route only because a robot gate has not + # reported yet, and the author has nothing to answer while it runs. + facts = (result or {}).get("facts") or {} + return ( + (result or {}).get("route") == "author" + and not facts.get("route_held_for_gates") + ) + + def plan_nudge( result: dict[str, Any] | None, previous: dict[str, Any] | None, @@ -116,7 +126,7 @@ def plan_nudge( or result.get("route") in ("transient-failure", "unknown") ): return False, entry or None - if not result or result.get("route") != "author": + if not waiting_on_author(result): return False, None if nudged_at: return False, entry @@ -252,7 +262,7 @@ def deliver_prepared_author_nudges( continue pr_number = int(key) result = dashboard_prs.get(key) - if not result or result.get("route") != "author": + if not waiting_on_author(result): _due, reset_entry = plan_nudge(result, entry, now) if reset_entry is None: updated.pop(key, None) diff --git a/.github/scripts/pull-request-dashboard/dashboard.py b/.github/scripts/pull-request-dashboard/dashboard.py index bbacb379b55..17d8365b1ef 100644 --- a/.github/scripts/pull-request-dashboard/dashboard.py +++ b/.github/scripts/pull-request-dashboard/dashboard.py @@ -1397,7 +1397,9 @@ def assign_author_nudge_episode( previous_result: dict[str, Any] | None, issue_comments: list[dict[str, Any]], ) -> None: - if route != "author": + # A held PR only shows the author route because a gate has not reported, + # so the author's waiting episode ended when the route was computed. + if route != "author" or facts.get("route_held_for_gates"): facts.pop("author_nudge_episode_id", None) return previous_facts = (previous_result or {}).get("facts") or {} diff --git a/.github/scripts/pull-request-dashboard/notifications.py b/.github/scripts/pull-request-dashboard/notifications.py index 8044ee0e651..48d07c863f1 100644 --- a/.github/scripts/pull-request-dashboard/notifications.py +++ b/.github/scripts/pull-request-dashboard/notifications.py @@ -216,6 +216,13 @@ def next_notifications( continue facts = result.get("facts") or {} + # A held PR did not reach the reviewers who would be reminded here; it + # is only waiting for a robot gate to report. + if facts.get("route_held_for_gates"): + if last_pr_notification: + updated_notifications[pr_key] = last_pr_notification + continue + mapped_reviewers = [ (reviewer, slack_user_map[reviewer.lower()]) for reviewer in reviewer_logins_for_notification(facts) diff --git a/.github/scripts/pull-request-dashboard/state.py b/.github/scripts/pull-request-dashboard/state.py index 5ad499037dc..7707ac65c3b 100644 --- a/.github/scripts/pull-request-dashboard/state.py +++ b/.github/scripts/pull-request-dashboard/state.py @@ -36,7 +36,7 @@ STATUS_COMMENT_ROLLOUT_STATE_VERSION = 1 # Rendered status-comment behavior. Increment when existing comments need to # adopt a change; hourly runs durably roll it out to all open PRs. -STATUS_COMMENT_REVISION = 14 +STATUS_COMMENT_REVISION = 15 INITIAL_BACKFILL_COMPLETE_KEY = "initial_backfill_complete" _state_dir: Path | None = None diff --git a/.github/scripts/pull-request-dashboard/test_author_nudge.py b/.github/scripts/pull-request-dashboard/test_author_nudge.py index a9b598b09ab..0763924f2db 100644 --- a/.github/scripts/pull-request-dashboard/test_author_nudge.py +++ b/.github/scripts/pull-request-dashboard/test_author_nudge.py @@ -226,6 +226,19 @@ def test_leaving_author_route_resets_unnudged_clock(self) -> None: self.assertFalse(due) self.assertIsNone(entry) + def test_gate_held_route_resets_clock_and_does_not_nudge(self) -> None: + held = author_result() + held["facts"]["route_held_for_gates"] = True + + due, entry = author_nudge.plan_nudge( + held, + {"waiting_since": "2026-07-10T00:00:00+00:00", "nudged_at": ""}, + NOW, + ) + + self.assertFalse(due) + self.assertIsNone(entry) + def test_returning_to_author_route_starts_new_episode(self) -> None: previous = { "waiting_since": "2026-07-01T00:00:00+00:00", diff --git a/.github/scripts/pull-request-dashboard/test_dashboard.py b/.github/scripts/pull-request-dashboard/test_dashboard.py index e8664aebb84..48993c48295 100644 --- a/.github/scripts/pull-request-dashboard/test_dashboard.py +++ b/.github/scripts/pull-request-dashboard/test_dashboard.py @@ -319,6 +319,21 @@ def test_recovers_episode_from_status_comment_after_cache_loss(self) -> None: self.assertEqual("abc123", facts["author_nudge_episode_id"]) + def test_ends_episode_while_route_is_held_for_gates(self) -> None: + facts: dict[str, object] = {"route_held_for_gates": True} + + assign_author_nudge_episode( + facts, + "author", + { + "route": "author", + "facts": {"author_nudge_episode_id": "abc123"}, + }, + [], + ) + + self.assertNotIn("author_nudge_episode_id", facts) + class FetchPrRawTest(unittest.TestCase): def test_uses_graphql_issue_comments_without_rest_join(self) -> None: issue_comments = [{"id": 101, "body": "GraphQL comment"}] diff --git a/.github/scripts/pull-request-dashboard/test_notify_slack.py b/.github/scripts/pull-request-dashboard/test_notify_slack.py index cb4b0c5a428..d14d3a8be9c 100644 --- a/.github/scripts/pull-request-dashboard/test_notify_slack.py +++ b/.github/scripts/pull-request-dashboard/test_notify_slack.py @@ -14,7 +14,7 @@ def test_gate_held_pr_does_not_notify_reviewers(self, send_notification) -> None results = { 7: { "pr_number": 7, - "route": "author", + "route": "approver", "facts": { "route_held_for_gates": True, "reviewers": [{"login": "reviewer"}], From dca07847f2efa86157aef222a2b9927da193a9be Mon Sep 17 00:00:00 2001 From: Trask Stalnaker Date: Thu, 30 Jul 2026 13:06:09 -0700 Subject: [PATCH 11/16] Address low-confidence review findings Correct the rollup return annotation, and read check suites only when an app-owned required context has not reported. --- .../pull-request-dashboard/RATIONALE.md | 4 +- .../pull-request-dashboard/github_cli.py | 56 ++++++--- .../pull-request-dashboard/test_github_cli.py | 117 ++++++++++++++++++ 3 files changed, 158 insertions(+), 19 deletions(-) diff --git a/.github/scripts/pull-request-dashboard/RATIONALE.md b/.github/scripts/pull-request-dashboard/RATIONALE.md index 81eec72e6c0..51545f13001 100644 --- a/.github/scripts/pull-request-dashboard/RATIONALE.md +++ b/.github/scripts/pull-request-dashboard/RATIONALE.md @@ -185,7 +185,9 @@ the implementation understandable and operationally cheap. ruleset context that no workflow produces — an obsolete or conditionally skipped job — would be reported as permanently pending. Such a PR cannot merge either way; the difference is that the dashboard stops treating it as - "still running." + "still running." The suites are read only when an app-owned required context + has not reported, so the refreshes that cannot use the answer do not pay for + it. - A `code_scanning` ruleset rule holds the merge on a check that the code scanning app publishes per configured tool, named after that tool, which GitHub never marks as required. Those checks are matched by app and by the diff --git a/.github/scripts/pull-request-dashboard/github_cli.py b/.github/scripts/pull-request-dashboard/github_cli.py index 17d9ecd7ef2..af95ef02588 100644 --- a/.github/scripts/pull-request-dashboard/github_cli.py +++ b/.github/scripts/pull-request-dashboard/github_cli.py @@ -409,7 +409,7 @@ def gh_pr_check_rollup( repo: str, pr_id: str, non_blocking_check_patterns: list[str], -) -> dict[str, list[dict[str, Any]]] | None: +) -> dict[str, Any] | None: del repo checks_by_identity: dict[ tuple[str, int | None, bool], tuple[dict[str, Any], bool] @@ -584,14 +584,11 @@ def settled_check_suite_app_ids(repo: str, head_sha: str) -> set[int]: return {app_id for app_id, completed in settled.items() if completed} -def include_missing_required_checks( - checks: list[dict[str, Any]] | None, - required_contexts: list[dict[str, Any]] | None, - settled_app_ids: set[int] | None = None, -) -> list[dict[str, Any]] | None: - if checks is None or required_contexts is None: - return None - complete = list(checks) +def unreported_required_contexts( + checks: list[dict[str, Any]], + required_contexts: list[dict[str, Any]], +) -> list[dict[str, Any]]: + unreported: list[dict[str, Any]] = [] for requirement in required_contexts: context = requirement["context"] integration_id = requirement.get("integration_id") @@ -612,8 +609,22 @@ def include_missing_required_checks( candidate.get("context") == context for candidate in required_contexts ) == 1 - if reported: - continue + if not reported: + unreported.append(requirement) + return unreported + + +def include_missing_required_checks( + checks: list[dict[str, Any]] | None, + required_contexts: list[dict[str, Any]] | None, + settled_app_ids: set[int] | None = None, +) -> list[dict[str, Any]] | None: + if checks is None or required_contexts is None: + return None + complete = list(checks) + for requirement in unreported_required_contexts(checks, required_contexts): + context = requirement["context"] + integration_id = requirement.get("integration_id") # A settled app will never report this context, so it cannot be pending. if integration_id is not None and integration_id in (settled_app_ids or set()): continue @@ -923,11 +934,6 @@ def fetch_pr_routing_raw( repo, pr.get("baseRefName") or "", ) - settled_app_ids_future = pool.submit( - settled_check_suite_app_ids, - repo, - pr.get("headRefOid") or "", - ) check_rollup = check_rollup_future.result() branch_rules = branch_rules_future.result() # commits(last: 1) can lag headRefOid, and the previous head's checks @@ -936,10 +942,24 @@ def fetch_pr_routing_raw( pr.get("headRefOid") or "" ): check_rollup = None + required_contexts = required_check_contexts(branch_rules) + # The check suites only say whether an app-owned context that has not + # reported can still arrive, so nothing else has to pay for that read. + settled_app_ids: set[int] = set() + if check_rollup is not None and required_contexts is not None and any( + requirement.get("integration_id") is not None + for requirement in unreported_required_contexts( + check_rollup["required"], required_contexts + ) + ): + settled_app_ids = settled_check_suite_app_ids( + repo, + pr.get("headRefOid") or "", + ) checks = include_missing_required_checks( None if check_rollup is None else check_rollup["required"], - required_check_contexts(branch_rules), - settled_app_ids_future.result(), + required_contexts, + settled_app_ids, ) if checks is not None and check_rollup is not None: checks = merge_code_scanning_checks( diff --git a/.github/scripts/pull-request-dashboard/test_github_cli.py b/.github/scripts/pull-request-dashboard/test_github_cli.py index bc3cc984214..e55ecca6707 100644 --- a/.github/scripts/pull-request-dashboard/test_github_cli.py +++ b/.github/scripts/pull-request-dashboard/test_github_cli.py @@ -904,6 +904,123 @@ def test_routing_raw_discards_checks_from_a_superseded_head( self.assertIsNone(raw["checks"]) + @patch("github_cli.settled_check_suite_app_ids", return_value=set()) + @patch( + "github_cli.gh_branch_rules", + return_value=[{ + "type": "required_status_checks", + "parameters": { + "required_status_checks": [{"context": "build", "integration_id": 1}], + }, + }], + ) + @patch( + "github_cli.gh_pr_check_rollup", + return_value={ + "head_oid": "current-head", + "required": [{"name": "build", "bucket": "pass", "integration_id": 1}], + "non_blocking_failures": [], + "code_scanning": [], + }, + ) + @patch("github_cli.fetch_review_threads", return_value=[]) + @patch("github_cli.fetch_review_requests", return_value=[]) + @patch("github_cli.fetch_pr_reviews", return_value=[]) + @patch("github_cli.fetch_pr_issue_comments", return_value=[]) + @patch("github_cli.gh_api", return_value=[]) + @patch("github_cli.gh_pr_view") + def test_routing_raw_skips_check_suites_when_every_context_reported( + self, + gh_pr_view, + _gh_api, + _issue_comments, + _reviews, + _review_requests, + _review_threads, + _rollup, + _branch_rules, + settled_app_ids, + ) -> None: + gh_pr_view.return_value = { + "id": "PR_node", + "baseRefName": "main", + "headRefOid": "current-head", + } + + raw = fetch_pr_routing_raw( + "open-telemetry/example", + "open-telemetry", + "example", + 7, + ) + + self.assertEqual( + [("build", "pass")], + [(check["name"], check["bucket"]) for check in raw["checks"]], + ) + settled_app_ids.assert_not_called() + + @patch("github_cli.settled_check_suite_app_ids", return_value={2}) + @patch( + "github_cli.gh_branch_rules", + return_value=[{ + "type": "required_status_checks", + "parameters": { + "required_status_checks": [ + {"context": "build", "integration_id": 1}, + {"context": "windows", "integration_id": 2}, + ], + }, + }], + ) + @patch( + "github_cli.gh_pr_check_rollup", + return_value={ + "head_oid": "current-head", + "required": [{"name": "build", "bucket": "pass", "integration_id": 1}], + "non_blocking_failures": [], + "code_scanning": [], + }, + ) + @patch("github_cli.fetch_review_threads", return_value=[]) + @patch("github_cli.fetch_review_requests", return_value=[]) + @patch("github_cli.fetch_pr_reviews", return_value=[]) + @patch("github_cli.fetch_pr_issue_comments", return_value=[]) + @patch("github_cli.gh_api", return_value=[]) + @patch("github_cli.gh_pr_view") + def test_routing_raw_reads_check_suites_for_an_unreported_context( + self, + gh_pr_view, + _gh_api, + _issue_comments, + _reviews, + _review_requests, + _review_threads, + _rollup, + _branch_rules, + settled_app_ids, + ) -> None: + gh_pr_view.return_value = { + "id": "PR_node", + "baseRefName": "main", + "headRefOid": "current-head", + } + + raw = fetch_pr_routing_raw( + "open-telemetry/example", + "open-telemetry", + "example", + 7, + ) + + self.assertEqual( + [("build", "pass")], + [(check["name"], check["bucket"]) for check in raw["checks"]], + ) + settled_app_ids.assert_called_once_with( + "open-telemetry/example", "current-head" + ) + def test_missing_required_checks_are_pending(self) -> None: self.assertEqual( [ From 5e2a2c78826a1e75d8aa412586853de1e8061dca Mon Sep 17 00:00:00 2001 From: Trask Stalnaker Date: Thu, 30 Jul 2026 13:29:47 -0700 Subject: [PATCH 12/16] Address low-confidence review findings Match the Copilot reviewer by login from one shared list, and say the held gates have to report rather than finish. --- .../pull-request-dashboard/copilot_review.py | 11 ++--------- .../pull-request-dashboard/pr_status_comment.py | 2 +- .github/scripts/pull-request-dashboard/render.py | 16 +++++++++++----- .../test_pr_status_comment.py | 4 ++-- .../pull-request-dashboard/test_render.py | 8 ++++++++ .github/scripts/pull-request-dashboard/utils.py | 12 ++++++++++++ 6 files changed, 36 insertions(+), 17 deletions(-) diff --git a/.github/scripts/pull-request-dashboard/copilot_review.py b/.github/scripts/pull-request-dashboard/copilot_review.py index 777bec44962..e918bc29591 100644 --- a/.github/scripts/pull-request-dashboard/copilot_review.py +++ b/.github/scripts/pull-request-dashboard/copilot_review.py @@ -17,18 +17,11 @@ request_copilot_review, ) from state import load_copilot_review_requests, save_copilot_review_requests -from utils import actor_login, format_ts - - -_COPILOT_REVIEWER_LOGINS = { - "copilot", - "copilot-pull-request-reviewer", - "copilot-pull-request-reviewer[bot]", -} +from utils import actor_login, format_ts, is_copilot_reviewer_login def is_copilot_reviewer(obj: dict[str, Any] | None) -> bool: - return actor_login(obj).lower() in _COPILOT_REVIEWER_LOGINS + return is_copilot_reviewer_login(actor_login(obj)) def copilot_review_status( diff --git a/.github/scripts/pull-request-dashboard/pr_status_comment.py b/.github/scripts/pull-request-dashboard/pr_status_comment.py index 49c76f4e0a8..ca615301401 100644 --- a/.github/scripts/pull-request-dashboard/pr_status_comment.py +++ b/.github/scripts/pull-request-dashboard/pr_status_comment.py @@ -220,7 +220,7 @@ def author_body( return [sentence] if held_gates: return [ - f"Wait for {held_gates} to finish; this pull request moves to " + f"Wait for {held_gates} to report; this pull request moves to " "reviewers once the results are clean." ] _, fallback_next_step = route_status_summary("author") diff --git a/.github/scripts/pull-request-dashboard/render.py b/.github/scripts/pull-request-dashboard/render.py index d35d995ce14..51fbffbc9ba 100644 --- a/.github/scripts/pull-request-dashboard/render.py +++ b/.github/scripts/pull-request-dashboard/render.py @@ -5,7 +5,15 @@ from typing import Any from route_presentation import ROUTE_ORDER, route_label -from utils import actor_login, activity_age, markdown_escape, parse_ts, seconds_since +from utils import ( + COPILOT_REVIEWER_LOGINS, + actor_login, + activity_age, + is_copilot_reviewer_login, + markdown_escape, + parse_ts, + seconds_since, +) # Mirrors dashboard_override.DASHBOARD_OVERRIDE_LABEL; duplicated here to keep @@ -131,9 +139,7 @@ def reviewer_icon(reviewer: dict[str, Any]) -> str: # Friendlier display names for bot reviewers whose login is verbose. REVIEWER_DISPLAY_NAMES = { - "copilot": "Copilot", - "copilot-pull-request-reviewer": "Copilot", - "copilot-pull-request-reviewer[bot]": "Copilot", + login: "Copilot" for login in COPILOT_REVIEWER_LOGINS } @@ -151,7 +157,7 @@ def display_reviewers(facts: dict[str, Any]) -> list[dict[str, Any]]: if not facts.get("copilot_review_outstanding"): return reviewers for reviewer in reviewers: - if reviewer_display_name(reviewer.get("login") or "") == "Copilot": + if is_copilot_reviewer_login(reviewer.get("login") or ""): reviewer["pending_review"] = True return reviewers reviewers.append({"login": COPILOT_REVIEWER_LOGIN, "pending_review": True}) 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 7973c0365e2..ade644be5a8 100644 --- a/.github/scripts/pull-request-dashboard/test_pr_status_comment.py +++ b/.github/scripts/pull-request-dashboard/test_pr_status_comment.py @@ -240,7 +240,7 @@ def test_held_pr_names_only_the_outstanding_check_gate(self) -> None: }, ) - self.assertIn("Wait for the required status checks to finish;", body) + self.assertIn("Wait for the required status checks to report;", body) def test_held_pr_names_only_the_outstanding_copilot_gate(self) -> None: body = pr_status_comment.render_status_comment( @@ -256,7 +256,7 @@ def test_held_pr_names_only_the_outstanding_copilot_gate(self) -> None: }, ) - self.assertIn("Wait for the Copilot review to finish;", body) + self.assertIn("Wait for the Copilot review to report;", body) def test_waiting_on_author_combines_ci_and_review_feedback_reasons(self) -> None: body = pr_status_comment.render_status_comment( diff --git a/.github/scripts/pull-request-dashboard/test_render.py b/.github/scripts/pull-request-dashboard/test_render.py index 506eaf26c30..509762f7e32 100644 --- a/.github/scripts/pull-request-dashboard/test_render.py +++ b/.github/scripts/pull-request-dashboard/test_render.py @@ -151,6 +151,14 @@ def test_outstanding_copilot_review_marks_its_short_login_entry(self) -> None: self.assertEqual("Copilot ⏳\u2060💬", cell) + def test_outstanding_copilot_review_marks_its_api_cased_entry(self) -> None: + cell = reviewers_cell_text({ + "reviewers": [{"login": "Copilot", "open_thread": True}], + "copilot_review_outstanding": True, + }) + + self.assertEqual("Copilot ⏳\u2060💬", cell) + def test_clean_copilot_review_is_not_listed_as_pending(self) -> None: cell = reviewers_cell_text({ "reviewers": [{"login": "reviewer", "approved": True}], diff --git a/.github/scripts/pull-request-dashboard/utils.py b/.github/scripts/pull-request-dashboard/utils.py index 6d4209e4118..349d6f5c9d4 100644 --- a/.github/scripts/pull-request-dashboard/utils.py +++ b/.github/scripts/pull-request-dashboard/utils.py @@ -64,6 +64,18 @@ def actor_login(obj: dict[str, Any] | None) -> str: return ((obj or {}).get("login") or "").strip() +# Every login GitHub has used for the Copilot reviewer, lowercased. +COPILOT_REVIEWER_LOGINS = frozenset({ + "copilot", + "copilot-pull-request-reviewer", + "copilot-pull-request-reviewer[bot]", +}) + + +def is_copilot_reviewer_login(login: str) -> bool: + return (login or "").strip().lower() in COPILOT_REVIEWER_LOGINS + + def format_ts(ts: datetime | None) -> str: return ts.isoformat() if ts else "" From 405d3b7f2dc3bc05e3fdfdd584ac955ab7f7628c Mon Sep 17 00:00:00 2001 From: Trask Stalnaker Date: Thu, 30 Jul 2026 13:51:33 -0700 Subject: [PATCH 13/16] Address low-confidence review findings Look up the reviewer display name the same way the Copilot reviewer itself is matched. --- .github/scripts/pull-request-dashboard/render.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/.github/scripts/pull-request-dashboard/render.py b/.github/scripts/pull-request-dashboard/render.py index 51fbffbc9ba..cb8fb2471c6 100644 --- a/.github/scripts/pull-request-dashboard/render.py +++ b/.github/scripts/pull-request-dashboard/render.py @@ -137,14 +137,15 @@ def reviewer_icon(reviewer: dict[str, Any]) -> str: return WORD_JOINER.join(discussion_icons) -# Friendlier display names for bot reviewers whose login is verbose. +# Friendlier display names for bot reviewers whose login is verbose, keyed by +# the lowercased login so they match the same way the reviewer itself does. REVIEWER_DISPLAY_NAMES = { login: "Copilot" for login in COPILOT_REVIEWER_LOGINS } def reviewer_display_name(login: str) -> str: - return REVIEWER_DISPLAY_NAMES.get(login, login) + return REVIEWER_DISPLAY_NAMES.get((login or "").strip().lower(), login) COPILOT_REVIEWER_LOGIN = "copilot-pull-request-reviewer" From f4fdc9e3d870257c57472b7abe103015dcd63a0b Mon Sep 17 00:00:00 2001 From: Trask Stalnaker Date: Thu, 30 Jul 2026 14:26:28 -0700 Subject: [PATCH 14/16] Address low-confidence review findings Cover the combined held-gate phrase directly so the wording shared by the status comment and the dashboard override stays asserted when both gates are outstanding. --- .../pull-request-dashboard/test_route_presentation.py | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/.github/scripts/pull-request-dashboard/test_route_presentation.py b/.github/scripts/pull-request-dashboard/test_route_presentation.py index f2a1d47e1d9..bcc60a52cfe 100644 --- a/.github/scripts/pull-request-dashboard/test_route_presentation.py +++ b/.github/scripts/pull-request-dashboard/test_route_presentation.py @@ -5,6 +5,7 @@ from route_presentation import ( ROUTE_ORDER, ROUTE_PRESENTATION, + outstanding_gate_phrase, route_label, route_status_summary, ) @@ -35,6 +36,15 @@ def test_unrecognized_route_uses_unknown_presentation(self) -> None: self.assertEqual(route_label("unknown"), route_label("other")) self.assertEqual(route_status_summary("unknown"), route_status_summary("other")) + def test_both_outstanding_gates_are_named_in_one_phrase(self) -> None: + self.assertEqual( + "the required status checks and the Copilot review", + outstanding_gate_phrase({ + "required_checks_settled": False, + "copilot_review_outstanding": True, + }), + ) + if __name__ == "__main__": unittest.main() \ No newline at end of file From 6bf37d008a13f59572288167568bcbe184ce5098 Mon Sep 17 00:00:00 2001 From: Trask Stalnaker Date: Thu, 30 Jul 2026 14:53:16 -0700 Subject: [PATCH 15/16] Address low-confidence review findings Carry the held-gate facts through the retired copilot route migration so a cached record renders as held instead of telling the author to answer feedback that does not exist, and share the required-check settlement predicate through utils so the migration derives it the same way a fresh routing pass does. --- .../pull-request-dashboard/copilot_review.py | 15 ++++---- .../pull-request-dashboard/dashboard.py | 10 ++++-- .../scripts/pull-request-dashboard/state.py | 16 +++++++-- .../pull-request-dashboard/test_state.py | 36 ++++++++++++++++--- .../scripts/pull-request-dashboard/utils.py | 8 +++++ 5 files changed, 68 insertions(+), 17 deletions(-) diff --git a/.github/scripts/pull-request-dashboard/copilot_review.py b/.github/scripts/pull-request-dashboard/copilot_review.py index e918bc29591..03e999f7b06 100644 --- a/.github/scripts/pull-request-dashboard/copilot_review.py +++ b/.github/scripts/pull-request-dashboard/copilot_review.py @@ -17,7 +17,12 @@ request_copilot_review, ) from state import load_copilot_review_requests, save_copilot_review_requests -from utils import actor_login, format_ts, is_copilot_reviewer_login +from utils import ( + actor_login, + format_ts, + is_copilot_reviewer_login, + required_checks_settled, +) def is_copilot_reviewer(obj: dict[str, Any] | None) -> bool: @@ -51,14 +56,6 @@ def copilot_review_status( return True, bool(latest_review.get("finding_count")) -def required_checks_settled(facts: dict[str, Any]) -> bool: - # A route computed while checks are still running is provisional: route_pr - # can only see a failure once the check has completed. - if "ci_pending_count" not in facts: - return False - return not facts.get("ci_pending_count", 0) - - def copilot_review_outstanding(facts: dict[str, Any], *, enabled: bool) -> bool: if not enabled: return False diff --git a/.github/scripts/pull-request-dashboard/dashboard.py b/.github/scripts/pull-request-dashboard/dashboard.py index 17d8365b1ef..52e104f03f5 100644 --- a/.github/scripts/pull-request-dashboard/dashboard.py +++ b/.github/scripts/pull-request-dashboard/dashboard.py @@ -214,7 +214,6 @@ copilot_review_status, is_copilot_reviewer, record_copilot_review_observation, - required_checks_settled, set_copilot_review_request_needed, ) from dashboard_override import ( @@ -239,7 +238,14 @@ update_dashboard_state_for_pr, ) import state_branch -from utils import actor_login, format_ts, parse_ts, truncate, utc_now +from utils import ( + actor_login, + format_ts, + parse_ts, + required_checks_settled, + truncate, + utc_now, +) # --- CLI defaults ---------------------------------------------------------- DEFAULT_MODEL = "gpt-5.4-mini" diff --git a/.github/scripts/pull-request-dashboard/state.py b/.github/scripts/pull-request-dashboard/state.py index 7707ac65c3b..7daf36345b0 100644 --- a/.github/scripts/pull-request-dashboard/state.py +++ b/.github/scripts/pull-request-dashboard/state.py @@ -8,6 +8,7 @@ from github_cli import detect_repo, normalize_repo, repo_state_key import state_branch +from utils import required_checks_settled DASHBOARD_MARKDOWN_FILE = "pull-request-dashboard.md" @@ -265,10 +266,21 @@ def enqueue_status_comment_update(pr_number: int) -> None: def migrated_pr_record(record: Any) -> Any: # The retired copilot route held a PR while its review was outstanding, - # which is now part of the author's turn. + # which is now part of the author's turn. The gate facts travel with the + # route so cached records read as held until the PR is refreshed. if not isinstance(record, dict) or record.get("route") != "copilot": return record - return {**record, "route": "author"} + facts = record.get("facts") or {} + return { + **record, + "route": "author", + "facts": { + **facts, + "route_held_for_gates": True, + "copilot_review_outstanding": True, + "required_checks_settled": required_checks_settled(facts), + }, + } def load_dashboard_state_cache() -> dict[str, Any] | None: diff --git a/.github/scripts/pull-request-dashboard/test_state.py b/.github/scripts/pull-request-dashboard/test_state.py index 67d87108364..01921532955 100644 --- a/.github/scripts/pull-request-dashboard/test_state.py +++ b/.github/scripts/pull-request-dashboard/test_state.py @@ -155,8 +155,18 @@ def test_dashboard_state_migrates_the_retired_copilot_route(self) -> None: "version": DASHBOARD_STATE_VERSION, "initial_backfill_complete": True, "prs": { - "1": {"route": "copilot", "facts": {"waiting_since": "2026-07-01T00:00:00Z"}}, - "2": {"route": "approver"}, + "1": { + "route": "copilot", + "facts": { + "waiting_since": "2026-07-01T00:00:00Z", + "ci_pending_count": 0, + }, + }, + "2": { + "route": "copilot", + "facts": {"ci_pending_count": 2}, + }, + "3": {"route": "approver"}, }, }), encoding="utf-8", @@ -168,8 +178,26 @@ def test_dashboard_state_migrates_the_retired_copilot_route(self) -> None: self.assertEqual( state["prs"], { - "1": {"route": "author", "facts": {"waiting_since": "2026-07-01T00:00:00Z"}}, - "2": {"route": "approver"}, + "1": { + "route": "author", + "facts": { + "waiting_since": "2026-07-01T00:00:00Z", + "ci_pending_count": 0, + "route_held_for_gates": True, + "copilot_review_outstanding": True, + "required_checks_settled": True, + }, + }, + "2": { + "route": "author", + "facts": { + "ci_pending_count": 2, + "route_held_for_gates": True, + "copilot_review_outstanding": True, + "required_checks_settled": False, + }, + }, + "3": {"route": "approver"}, }, ) diff --git a/.github/scripts/pull-request-dashboard/utils.py b/.github/scripts/pull-request-dashboard/utils.py index 349d6f5c9d4..dfea082d05c 100644 --- a/.github/scripts/pull-request-dashboard/utils.py +++ b/.github/scripts/pull-request-dashboard/utils.py @@ -76,6 +76,14 @@ def is_copilot_reviewer_login(login: str) -> bool: return (login or "").strip().lower() in COPILOT_REVIEWER_LOGINS +def required_checks_settled(facts: dict[str, Any]) -> bool: + # A route computed while checks are still running is provisional: route_pr + # can only see a failure once the check has completed. + if "ci_pending_count" not in facts: + return False + return not facts.get("ci_pending_count", 0) + + def format_ts(ts: datetime | None) -> str: return ts.isoformat() if ts else "" From 409e4fecb321c55214b1ed9828e77235c230c130 Mon Sep 17 00:00:00 2001 From: Trask Stalnaker Date: Thu, 30 Jul 2026 15:52:37 -0700 Subject: [PATCH 16/16] Address low-confidence review findings Migrate a maintenance bot's retired copilot route to the approver route so the cached record and every later hold keep the fallback the bot would get from a fresh routing pass instead of parking it on an author route it can never act on. --- .../scripts/pull-request-dashboard/state.py | 6 ++++-- .../pull-request-dashboard/test_state.py | 21 +++++++++++++++++-- 2 files changed, 23 insertions(+), 4 deletions(-) diff --git a/.github/scripts/pull-request-dashboard/state.py b/.github/scripts/pull-request-dashboard/state.py index 7daf36345b0..11afdbc0ff2 100644 --- a/.github/scripts/pull-request-dashboard/state.py +++ b/.github/scripts/pull-request-dashboard/state.py @@ -267,13 +267,15 @@ def enqueue_status_comment_update(pr_number: int) -> None: def migrated_pr_record(record: Any) -> Any: # The retired copilot route held a PR while its review was outstanding, # which is now part of the author's turn. The gate facts travel with the - # route so cached records read as held until the PR is refreshed. + # route so cached records read as held until the PR is refreshed, and the + # held route picks the same fallback a maintenance bot gets, because it has + # no author to act. if not isinstance(record, dict) or record.get("route") != "copilot": return record facts = record.get("facts") or {} return { **record, - "route": "author", + "route": "approver" if facts.get("is_maintenance_bot") else "author", "facts": { **facts, "route_held_for_gates": True, diff --git a/.github/scripts/pull-request-dashboard/test_state.py b/.github/scripts/pull-request-dashboard/test_state.py index 01921532955..7834c3ba712 100644 --- a/.github/scripts/pull-request-dashboard/test_state.py +++ b/.github/scripts/pull-request-dashboard/test_state.py @@ -166,7 +166,14 @@ def test_dashboard_state_migrates_the_retired_copilot_route(self) -> None: "route": "copilot", "facts": {"ci_pending_count": 2}, }, - "3": {"route": "approver"}, + "3": { + "route": "copilot", + "facts": { + "ci_pending_count": 0, + "is_maintenance_bot": True, + }, + }, + "4": {"route": "approver"}, }, }), encoding="utf-8", @@ -197,7 +204,17 @@ def test_dashboard_state_migrates_the_retired_copilot_route(self) -> None: "required_checks_settled": False, }, }, - "3": {"route": "approver"}, + "3": { + "route": "approver", + "facts": { + "ci_pending_count": 0, + "is_maintenance_bot": True, + "route_held_for_gates": True, + "copilot_review_outstanding": True, + "required_checks_settled": True, + }, + }, + "4": {"route": "approver"}, }, )