diff --git a/.github/scripts/pull-request-dashboard/RATIONALE.md b/.github/scripts/pull-request-dashboard/RATIONALE.md index 486e1027a53..51545f13001 100644 --- a/.github/scripts/pull-request-dashboard/RATIONALE.md +++ b/.github/scripts/pull-request-dashboard/RATIONALE.md @@ -172,6 +172,22 @@ 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." 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 @@ -186,9 +202,43 @@ 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 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, 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 + 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 + 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. + but never route one of these PRs to its author: a bot PR whose handoff is + held waits on reviewers instead. ## Copilot Review Gate @@ -199,7 +249,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 @@ -214,11 +264,26 @@ 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 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 + 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 + 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 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/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/copilot_review.py b/.github/scripts/pull-request-dashboard/copilot_review.py index 61363daca7d..03e999f7b06 100644 --- a/.github/scripts/pull-request-dashboard/copilot_review.py +++ b/.github/scripts/pull-request-dashboard/copilot_review.py @@ -17,18 +17,16 @@ 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, + required_checks_settled, +) 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( @@ -58,22 +56,31 @@ def copilot_review_status( return True, bool(latest_review.get("finding_count")) -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 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( @@ -89,7 +96,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 @@ -105,6 +111,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 +134,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..52e104f03f5 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,27 @@ 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 + 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. + or PR creation time. Carried + 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 waiting_since. author_action_review_thread_urls @@ -190,10 +210,11 @@ ) 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, + set_copilot_review_request_needed, ) from dashboard_override import ( apply_dashboard_override, @@ -217,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" @@ -1145,8 +1173,21 @@ 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") + +# 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 ("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: @@ -1162,7 +1203,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("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 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 "" @@ -1176,6 +1225,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 @@ -1282,22 +1344,56 @@ def add_reviewers( ] +def hold_route_until_gates_settle( + facts: dict[str, Any], + route: str, + previous_result: dict[str, Any] | None, + *, + require_clean_copilot_review: bool, +) -> str: + # The required checks and the Copilot review are the author's to clear, so + # 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: + # 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 facts["required_checks_settled"] + or facts["copilot_review_outstanding"] + ) + ) + facts["route_held_for_gates"] = held + return previous_route 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 - # a required clean Copilot review instead of bypassing it. - return 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, - route_pr(facts, pending_actions, required_approvals), - ), - 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, ) @@ -1307,7 +1403,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 {} @@ -1426,6 +1524,7 @@ def build_pr_result( pending_actions, required_approvals, require_clean_copilot_review, + previous_result, ) assign_author_nudge_episode( facts, @@ -1434,7 +1533,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/dashboard_override.py b/.github/scripts/pull-request-dashboard/dashboard_override.py index d9b383dd8de..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 @@ -218,7 +219,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 +232,11 @@ def render_command_reply(reply: dict[str, Any]) -> str: "use `/dashboard route:reviewers`." ) elif kind == "routed": - if reply.get("route") == "copilot": + held_gates = reply.get("held_gates") or "" + if held_gates: message = ( "accepted the reviewer-routing override; the reviewer handoff " - "is waiting on Copilot." + 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", - "route": (result or {}).get("route") or "", + "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 2d6a5224828..af95ef02588 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 { @@ -408,13 +409,14 @@ 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] ] = {} 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,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 {} + 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 []: @@ -458,6 +465,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,13 +561,34 @@ def merge_code_scanning_checks( ] + code_scanning_checks -def include_missing_required_checks( - checks: list[dict[str, Any]] | None, - required_contexts: list[dict[str, Any]] | None, -) -> list[dict[str, Any]] | None: - if checks is None or required_contexts is None: - return None - complete = list(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: + 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 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} + + +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") @@ -580,7 +609,24 @@ def include_missing_required_checks( candidate.get("context") == context for candidate in required_contexts ) == 1 - if reported: + 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 complete.append({ "name": context, @@ -890,9 +936,30 @@ def fetch_pr_routing_raw( ) 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 + 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), + 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/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/pr_status_comment.py b/.github/scripts/pull-request-dashboard/pr_status_comment.py index c72c5ce340e..ca615301401 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,6 +189,7 @@ def author_body( non_blocking_failure_note: str, review_thread_urls: list[str], top_level_feedback_urls: list[str], + held_gates: str = "", ) -> list[str]: noun = "item" if feedback_count == 1 else "items" if failing_count and feedback_count: @@ -213,6 +218,11 @@ def author_body( if non_blocking_failure_note: sentence += f" Note: {non_blocking_failure_note}" return [sentence] + if held_gates: + return [ + 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") return [fallback_next_step] @@ -260,6 +270,11 @@ def render_status_comment( ), review_thread_urls=review_thread_urls, top_level_feedback_urls=top_level_feedback_urls, + 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 c0a5428f833..cb8fb2471c6 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 @@ -111,6 +119,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"): @@ -127,21 +137,38 @@ 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 = { - "copilot-pull-request-reviewer": "Copilot", - "copilot-pull-request-reviewer[bot]": "Copilot", + 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" + + +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 is_copilot_reviewer_login(reviewer.get("login") or ""): + 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 +291,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/route_presentation.py b/.github/scripts/pull-request-dashboard/route_presentation.py index 36c28815f3c..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": { @@ -8,12 +10,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", @@ -56,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/state.py b/.github/scripts/pull-request-dashboard/state.py index d386fd59e06..11afdbc0ff2 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" @@ -31,12 +32,12 @@ # 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 # 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 @@ -263,15 +264,37 @@ 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. The gate facts travel with the + # 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": "approver" if facts.get("is_maintenance_bot") else "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: 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_author_nudge.py b/.github/scripts/pull-request-dashboard/test_author_nudge.py index eabb2ecad48..0763924f2db 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": [], @@ -225,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_copilot_review.py b/.github/scripts/pull-request-dashboard/test_copilot_review.py index 8b04aec909e..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, @@ -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..48993c48295 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,6 +20,7 @@ compute_facts, fetch_pr_raw, group_review_threads, + hold_route_until_gates_settle, 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, } @@ -50,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( @@ -69,6 +72,198 @@ def test_override_reaches_reviewers_when_gate_disabled(self) -> None: self.assertEqual("approver", route) + 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("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, + ) + + def test_author_keeps_the_pr_while_replacement_checks_run(self) -> None: + route = self._hold( + {"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 = 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 = self._hold( + {"ci_failing_count": 0, "ci_pending_count": 0}, + "approver", + {"route": "author"}, + ) + + self.assertEqual("approver", route) + + 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: + route = self._hold({"ci_pending_count": 1}, "approver", 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, + "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 = { + "route_held_for_gates": 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 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_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: @@ -124,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"}] @@ -160,6 +370,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": [], @@ -543,100 +754,111 @@ 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, "copilot_review_exists": False, "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: facts = { + "ci_pending_count": 0, "copilot_review_requested": False, "copilot_review_exists": True, "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: facts = { + "ci_pending_count": 0, "copilot_review_requested": False, "copilot_review_exists": True, "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, "copilot_review_exists": True, "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, "copilot_review_exists": True, "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_disabled_gate_preserves_maintainer_route(self) -> None: + 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, - "maintainer", - enabled=False, - ) + set_copilot_review_request_needed(facts, "approver", enabled=True) + + 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, + } + + set_copilot_review_request_needed(facts, "approver", enabled=True) + + self.assertFalse(facts["copilot_review_request_needed"]) + + def test_author_route_does_not_request_a_re_review(self) -> None: + facts = { + "ci_pending_count": 0, + "copilot_review_requested": False, + "copilot_review_exists": True, + "copilot_review_needed": True, + } + + set_copilot_review_request_needed(facts, "author", enabled=True) + + self.assertFalse(facts["copilot_review_request_needed"]) + + def test_disabled_gate_requests_nothing(self) -> None: + facts = { + "ci_pending_count": 0, + "copilot_review_requested": False, + "copilot_review_exists": True, + "copilot_review_needed": True, + } + + 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..8e912933fd8 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_gates": "the required status checks", "user": "author", }) @@ -171,18 +171,17 @@ 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.", + 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 +481,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 +588,14 @@ 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, + "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 Copilot.\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 6003c4276c5..e55ecca6707 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, ) @@ -478,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, @@ -776,6 +819,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 +844,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 +862,165 @@ 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"]) + + @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( [ @@ -891,6 +1098,59 @@ 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": "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", + paginate=True, + ) + + @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_notify_slack.py b/.github/scripts/pull-request-dashboard/test_notify_slack.py index 93165363e74..d14d3a8be9c 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": "approver", "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..ade644be5a8 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 report;", 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 report;", body) + def test_waiting_on_author_combines_ci_and_review_feedback_reasons(self) -> None: body = pr_status_comment.render_status_comment( self.pr(), @@ -485,7 +517,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_render.py b/.github/scripts/pull-request-dashboard/test_render.py index 17a913aa73c..509762f7e32 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,51 @@ 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_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_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}], + "copilot_review_outstanding": False, + }) + + self.assertEqual("reviewer ✅", cell) + def test_dashboard_does_not_claim_approvers_can_force_refresh(self) -> None: markdown = render_pr_tables([], {}) diff --git a/.github/scripts/pull-request-dashboard/test_route_presentation.py b/.github/scripts/pull-request-dashboard/test_route_presentation.py index ec61977b5c6..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, ) @@ -31,16 +32,19 @@ 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")) + 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 diff --git a/.github/scripts/pull-request-dashboard/test_state.py b/.github/scripts/pull-request-dashboard/test_state.py index c94ae35901c..7834c3ba712 100644 --- a/.github/scripts/pull-request-dashboard/test_state.py +++ b/.github/scripts/pull-request-dashboard/test_state.py @@ -148,6 +148,76 @@ 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", + "ci_pending_count": 0, + }, + }, + "2": { + "route": "copilot", + "facts": {"ci_pending_count": 2}, + }, + "3": { + "route": "copilot", + "facts": { + "ci_pending_count": 0, + "is_maintenance_bot": True, + }, + }, + "4": {"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", + "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", + "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"}, + }, + ) + 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"}) @@ -167,7 +237,7 @@ def test_notification_state_version_is_independent(self) -> None: 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, 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)): diff --git a/.github/scripts/pull-request-dashboard/utils.py b/.github/scripts/pull-request-dashboard/utils.py index 6d4209e4118..dfea082d05c 100644 --- a/.github/scripts/pull-request-dashboard/utils.py +++ b/.github/scripts/pull-request-dashboard/utils.py @@ -64,6 +64,26 @@ 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 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 ""