diff --git a/.github/scripts/pull-request-dashboard/RATIONALE.md b/.github/scripts/pull-request-dashboard/RATIONALE.md index ac1d5a27990..56f7b7ae417 100644 --- a/.github/scripts/pull-request-dashboard/RATIONALE.md +++ b/.github/scripts/pull-request-dashboard/RATIONALE.md @@ -156,6 +156,13 @@ the implementation understandable and operationally cheap. next run continues after it in sorted PR-number order, wrapping when needed. Failed PR numbers are stored beside the cursor and are removed after a later successful refresh. +- The rotation is not reordered to favor PRs that are waiting on something. A + wait ends with nothing changing on the PR — a check completes, a review is + filed — so a missed event is only noticed when the rotation comes round again. + But a full sweep takes two passes on most repositories and four on the largest, + which is no longer than the gate hold that bounds the wait anyway, so + refreshing waiting PRs first would buy about an hour on one repository and + nothing on the rest. - Initial-backfill completion is stored in dashboard state and becomes true in the same accepted state commit that attempts the final missing open non-draft PR. Failed PR data is not accepted into dashboard state, but a recorded failed @@ -293,10 +300,60 @@ the implementation understandable and operationally cheap. 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. +- A handoff the gates held starts its wait when the gates release it, not at the + push. The push and the handoff were the same moment before the gates existed, + which is why the fallback dates a reviewer's wait from the last author + activity. Now the gates sit between the two, so a PR whose checks took an hour + would reach reviewers already an hour old, and one released after a stalled + gate would arrive older still. Either way the age blames reviewers for a wait + they could not have answered, and sorts the PR above ones they really have + been sitting on. +- Only the handoff from the author restarts the wait. A held PR that was already + with reviewers and is released to maintainers keeps its wait, because it never + left the people who owe it a response, and restarting there would present an + approval a week old as a merge request that just arrived. - Maintenance-bot PRs retain maintainer-oriented routing because the bot cannot respond to a dashboard action. Pending required checks affect the CI column but never route one of these PRs to its author: a bot PR whose handoff is held waits on reviewers instead. +- A hold has a time limit, and past it the PR routes anyway. Every gate waits on + something outside the dashboard, and each one has been seen never to arrive: a + required check with no check run on the head, a Copilot review GitHub never + started, a review request GitHub accepted and dropped. The dashboard does not + decide whether a PR may merge, branch protection does, so a hold that never + ends protects nobody. It only keeps the PR away from the people who could look + at the missing gate, and it hides the failure, because a held PR looks exactly + like a PR that is waiting normally. +- The limit is four hours: longer than a slow check suite or a queued Copilot + review, short enough that a gate which is never going to report costs the PR + part of a day rather than the rest of its life. +- The clock starts when a gate first holds the PR back, and then runs on its own + for as long as the same head still has an outstanding gate. Carrying it that + way is what lets the hold give up without the stall looking resolved a moment + later, and it keeps the status comment able to say which gate the dashboard + stopped waiting for. Starting it only on a real hold is what keeps a slow + check suite on a PR that was already with its reviewers from looking like a + stalled handoff. A push clears the clock, because new code means new checks + and a review that has to run again. +- Only a gate that has reported nothing on the current head runs the clock. A + Copilot review that covers the head but left open findings holds the PR, yet + it is not missing: it answered, and the threads it left are the author's to + clear. Because the clock is carried across a trip back to the author, counting + those findings would make every author who takes more than four hours over + review comments look like a gate GitHub lost, and the report would say a + review never arrived when it is sitting on the PR. A review that is missing or + that only covers older code still counts, because Copilot has said nothing + about the code being reviewed. +- An expired hold is reported as a delivery failure on whole-repository passes, + which opens the same tracking issue as any other dashboard failure. This is + the only alarm the gates raise. Each way a gate can go missing has its own + cause and none of them can be told apart from the dashboard's side, so + reporting them separately would mean a new alarm for every new way GitHub + finds to lose something. The hold expiring is the one symptom they all share. +- The report stays active while the stall does, the same way a PR that keeps + failing to refresh keeps the hourly failure active. A gate that will never + report is usually a repository misconfiguration — a required check with no + workflow to produce it — and it needs a person, not a reminder that stops. ## Copilot Review Gate @@ -352,6 +409,20 @@ the implementation understandable and operationally cheap. request only when a review already covers the current head, which is what happens when one lands between the observation and the delivery. A review that is still missing is a reason to request, not to discard. +- A request counts as delivered only once GitHub shows Copilot as a pending + reviewer. The mutation answers with success even when GitHub records nothing: + on one pull request it accepted the same request every hour for nineteen + hours and created no review request at all, and the pull request stayed held + on its author until a person requested the review by hand. Stamping the + request from the mutation's own answer hides that completely, because success + is silent and only discards are logged. +- The read back is retried a few times, because GitHub takes a moment to record + a request it did accept. A Copilot review of the current head is accepted as + the same proof, since a short review can finish and take Copilot out of the + pending requests again before the read. +- A request GitHub dropped is left undelivered, so the next pass sends it again + and logs the miss. It needs no alarm of its own: the pull request stays held + while the review is missing, and the hold expiring is what reports it. - The reviewers column marks Copilot pending only where the gate applies and a review is genuinely in flight — a requested re-review, or the automatic first review on a PR the Copilot gate is holding because Copilot has never reviewed diff --git a/.github/scripts/pull-request-dashboard/copilot_review.py b/.github/scripts/pull-request-dashboard/copilot_review.py index dbaea33a3e2..2deade80ae8 100644 --- a/.github/scripts/pull-request-dashboard/copilot_review.py +++ b/.github/scripts/pull-request-dashboard/copilot_review.py @@ -14,7 +14,9 @@ ) from github_cli import ( fetch_pr_reviews, + fetch_review_requests, request_copilot_review, + sleep_for_retry, ) from state import load_copilot_review_requests, save_copilot_review_requests from utils import ( @@ -34,6 +36,12 @@ FIRST_REVIEW_GRACE = timedelta(hours=1) +# How many times the pull request is read back before a request counts as +# missing. GitHub takes a moment to record a reviewer it did accept, so a +# single empty read proves nothing. +REQUEST_CONFIRMATION_ATTEMPTS = 3 + + def is_copilot_reviewer(obj: dict[str, Any] | None) -> bool: return is_copilot_reviewer_login(actor_login(obj)) @@ -85,6 +93,19 @@ def copilot_review_outstanding(facts: dict[str, Any], *, enabled: bool) -> bool: ) +def copilot_review_unreported(facts: dict[str, Any], *, enabled: bool) -> bool: + # Whether the gate is still waiting for Copilot to say anything about the + # current head. Findings are an answer, not a silence: the threads they + # leave are the author's to clear, and the dashboard already routes the + # pull request to the author for them. Only a review that is missing or + # that covers older code is a report that has not arrived. + if not enabled: + return False + return not facts.get("copilot_review_exists") or bool( + facts.get("copilot_review_stale") + ) + + def set_copilot_first_review_missing_since( facts: dict[str, Any], previous_result: dict[str, Any] | None, @@ -222,6 +243,32 @@ def stale_request_reason( return "" +def copilot_review_request_landed( + owner: str, + repo_name: str, + pr_number: int, + head_sha: str, +) -> bool: + """Report whether GitHub recorded the Copilot review request just sent.""" + for attempt in range(REQUEST_CONFIRMATION_ATTEMPTS): + if attempt: + sleep_for_retry(attempt - 1) + if any( + is_copilot_reviewer(request) + for request in fetch_review_requests(owner, repo_name, pr_number) or [] + ): + return True + # Copilot can finish a short review before the last read, which takes it + # back out of the pending requests. A review of the current head proves the + # request landed just as well as a pending one does. + review_exists, review_stale, _findings = copilot_review_status( + fetch_pr_reviews(owner, repo_name, pr_number) or [], + head_sha, + [], + ) + return review_exists and not review_stale + + def deliver_copilot_review_requests( repo: str, now: datetime, @@ -285,9 +332,25 @@ def deliver_copilot_review_requests( if not pull_request_id: raise RuntimeError(f"GitHub did not return a node ID for PR #{pr_number}") request_copilot_review(pull_request_id) + landed = copilot_review_request_landed( + owner, + repo_name, + pr_number, + current_head, + ) except Exception as e: errors.append(f"PR #{pr_number}: {e}") continue - requests[key] = {**entry, "requested_at": format_ts(now)} + if landed: + requests[key] = {**entry, "requested_at": format_ts(now)} + continue + # Leaving the request undelivered keeps the next pass trying. Nothing + # escalates from here: a request that keeps going missing leaves the + # pull request held, and the hold is what reports the stall. + print( + f"GitHub did not record the Copilot review request for " + f"PR #{pr_number} on head {current_head}", + file=sys.stderr, + ) save_copilot_review_requests(requests) return errors \ No newline at end of file diff --git a/.github/scripts/pull-request-dashboard/dashboard.py b/.github/scripts/pull-request-dashboard/dashboard.py index d71bedf28ff..545c42120e4 100644 --- a/.github/scripts/pull-request-dashboard/dashboard.py +++ b/.github/scripts/pull-request-dashboard/dashboard.py @@ -163,6 +163,15 @@ to this PR and its review is missing or stale, so the route is held. + copilot_review_unreported bool The Copilot review gate applies + and Copilot has said nothing + about the current head, so the + gate is still waiting to + report. False once a review + covers this head, even when it + left open findings, because + those are the author's to + clear. route_held_for_gates bool The PR did not advance to the route it computed, because the required checks or the @@ -172,11 +181,23 @@ reported on the current head, so the computed route is not provisional. + route_held_since str (iso) When the gates first kept this + PR off its reviewers on this + head. Cleared once every gate + has reported or the author + pushes. + route_hold_expired bool A gate has reported nothing on + this head for longer than + GATE_HOLD_LIMIT, so the PR + routes anyway and the stall + is reported. waiting_since str (iso) Oldest pending discussion, or route-appropriate fallback, or PR creation time. Carried forward while the handoff is - held, and never moves + held, restarted when a held + handoff reaches reviewers, + and never moves forward while the PR stays on a reviewer route. waiting_age_basis str Which heuristic chose @@ -222,7 +243,7 @@ import uuid from concurrent.futures import ThreadPoolExecutor from dataclasses import dataclass, replace -from datetime import datetime +from datetime import datetime, timedelta from pathlib import Path from typing import Any, TypedDict @@ -247,6 +268,7 @@ from copilot_review import ( copilot_review_outstanding, copilot_review_status, + copilot_review_unreported, is_copilot_reviewer, record_copilot_review_observation, set_copilot_first_review_missing_since, @@ -1222,6 +1244,12 @@ def oldest_pending_action_ts( # advancing, but never from moving back toward its author. ROUTE_PROGRESSION = ("author", "approver", "maintainer") +# How long a gate may keep a pull request off its reviewers. Long enough that a +# slow check suite or a queued Copilot review finishes first, short enough that +# a gate which is never going to report costs the pull request part of a day +# rather than the rest of its life. +GATE_HOLD_LIMIT = timedelta(hours=4) + def route_progress(route: str) -> int: return ROUTE_PROGRESSION.index(route) if route in ROUTE_PROGRESSION else 0 @@ -1245,6 +1273,7 @@ def add_wait_age_facts( route: str, pending_actions: dict[str, dict[str, Any]], previous_result: dict[str, Any] | None = None, + now: datetime | None = None, ) -> None: previous_facts = (previous_result or {}).get("facts") or {} # A held route was not re-evaluated, so its wait continues uninterrupted @@ -1253,6 +1282,35 @@ def add_wait_age_facts( facts["waiting_since"] = previous_facts["waiting_since"] facts["waiting_age_basis"] = "gate_hold" return + # Reviewers have been waiting since the gates let the PR reach them, which + # is not the push. The fallback below dates a reviewer's wait from the last + # author activity, and that was the same moment until the gates started + # sitting between the two: now a PR whose checks took an hour would arrive + # already an hour old, and one released after a stalled gate would arrive + # older still, blaming reviewers for a wait they could not have answered. + if ( + route in REVIEWER_ROUTES + and previous_facts.get("route_held_for_gates") + and (previous_result or {}).get("route") == "author" + ): + facts["waiting_since"] = format_ts(now or utc_now()) + facts["waiting_age_basis"] = "gate_release" + return + # The release above only happens on the pass that hands the PR over, so + # without carrying it the next pass falls back to the author's push and + # presents the very wait the release exists to discard. The guard below + # cannot catch that, because it only stops the wait moving forward. + if ( + route in REVIEWER_ROUTES + and (previous_result or {}).get("route") in REVIEWER_ROUTES + and previous_facts.get("waiting_age_basis") == "gate_release" + and previous_facts.get("waiting_since") + and facts.get("head_sha") + and facts.get("head_sha") == previous_facts.get("head_sha") + ): + facts["waiting_since"] = previous_facts["waiting_since"] + facts["waiting_age_basis"] = "gate_release" + 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 "" @@ -1385,12 +1443,57 @@ def add_reviewers( ] +def gate_hold_expired(facts: dict[str, Any], now: datetime) -> bool: + held_since = parse_ts(facts.get("route_held_since")) + if held_since is None: + return False + return now - held_since >= GATE_HOLD_LIMIT + + +def set_gate_hold_clock( + facts: dict[str, Any], + previous_result: dict[str, Any] | None, + route: str, + *, + unreported_gates: bool, + would_hold: bool, + now: datetime, +) -> None: + # How long the gates have been keeping this pull request off the reviewers + # it would otherwise be with. It starts when a gate first holds the pull + # request back, and then runs on its own for as long as the same head still + # has a gate that has not reported. Carrying it that way is what lets the + # hold give up without the stall looking resolved a moment later, and it is + # why a trip back to the author does not stop it: the author owes the pull + # request something, but the gate is still missing on the same code, so + # letting the round trip clear the clock would hand that gate four more + # hours the moment the author answers. Starting it only on a real hold is + # what keeps a slow check suite on a pull request that was already with its + # reviewers from looking like one. A push clears it, because new code means + # new checks and a review that has to run again. + previous_facts = (previous_result or {}).get("facts") or {} + head_sha = str(facts.get("head_sha") or "") + carried = ( + str(previous_facts.get("route_held_since") or "") + if head_sha and head_sha == previous_facts.get("head_sha") + else "" + ) + if not ( + unreported_gates + and (carried or (route in REVIEWER_ROUTES and would_hold)) + ): + facts.pop("route_held_since", None) + return + facts["route_held_since"] = carried or format_ts(now) + + def hold_route_until_gates_settle( facts: dict[str, Any], route: str, previous_result: dict[str, Any] | None, *, require_clean_copilot_review: bool, + now: datetime, ) -> 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 @@ -1402,14 +1505,37 @@ def hold_route_until_gates_settle( facts["copilot_review_outstanding"] = copilot_review_outstanding( facts, enabled=require_clean_copilot_review ) + facts["copilot_review_unreported"] = copilot_review_unreported( + 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"] - ) + gates_outstanding = ( + not facts["required_checks_settled"] or facts["copilot_review_outstanding"] + ) + # Only a gate that has reported nothing on this head can stall. A Copilot + # review that left findings did report, and clearing those findings is the + # author's own work, so counting it would turn every author who takes more + # than four hours over review comments into a missing gate. + unreported_gates = ( + not facts["required_checks_settled"] or facts["copilot_review_unreported"] ) + would_hold = route_progress(route) > route_progress(previous_route) + set_gate_hold_clock( + facts, + previous_result, + route, + unreported_gates=unreported_gates, + would_hold=would_hold, + now=now, + ) + # A gate can stay outstanding forever: a required check that never reports, + # a review GitHub never runs. The dashboard cannot block a merge, so an + # endless hold protects nobody and only keeps the pull request away from + # the people who could move it. Past the limit it routes the pull request + # anyway and says which gate it stopped waiting for. + expired = gate_hold_expired(facts, now) + facts["route_hold_expired"] = expired + held = would_hold and gates_outstanding and not expired facts["route_held_for_gates"] = held return previous_route if held else route @@ -1470,6 +1596,7 @@ def resolve_pr_route( route, previous_result, require_clean_copilot_review=copilot_review_gate_enabled, + now=now, ) diff --git a/.github/scripts/pull-request-dashboard/delivery.py b/.github/scripts/pull-request-dashboard/delivery.py index ed5d5c6620f..b7378d7f031 100644 --- a/.github/scripts/pull-request-dashboard/delivery.py +++ b/.github/scripts/pull-request-dashboard/delivery.py @@ -15,6 +15,7 @@ from dashboard_override import deliver_dashboard_command_replies from github_cli import detect_repo, gh_api, list_open_prs, normalize_repo, repo_state_key from notify_slack import notify_slack_from_state +from route_presentation import unreported_gate_phrase from pr_status_comment import ( update_status_comments_from_state, update_targeted_status_comment_from_state, @@ -23,6 +24,7 @@ author_nudge_state_path, claim_delivery_versions, copilot_review_request_state_path, + load_dashboard_state_cache, notification_state_path, set_state_dir, ) @@ -49,6 +51,33 @@ def run_delivery_action( errors.append(f"{label}: {e}") +def report_stalled_gates(open_pr_numbers: set[int]) -> list[str]: + # The gates are the one place where the dashboard waits on someone else. + # When a wait outlasts its limit the dashboard has already routed the pull + # request, so nothing on the pull request itself is broken and nobody would + # notice. Reporting it here is what turns a silent stall into a failure a + # person sees, whatever the gate was and whatever went missing. + state = load_dashboard_state_cache() + if state is None: + return [] + stalled: list[str] = [] + for key, result in (state.get("prs") or {}).items(): + facts = (result or {}).get("facts") or {} + if not facts.get("route_hold_expired"): + continue + try: + number = int(key) + except ValueError: + continue + if number not in open_pr_numbers: + continue + gates = unreported_gate_phrase(facts) + if not gates: + continue + stalled.append(f"PR #{number}: {gates} never reported on head {facts.get('head_sha') or 'unknown'}") + return sorted(stalled) + + def deliver_from_state( repo: str, author_retry_snapshot_path: Path, @@ -126,6 +155,16 @@ def deliver_from_state( ), errors, ) + if pr_number is None and open_prs is not None: + # Last, so a stalled gate is reported but never keeps the real work + # from being delivered. Only whole-repository passes report it: a + # single pull request refresh has no business failing over another + # pull request's stall. + run_delivery_action( + "stalled gates", + lambda: report_stalled_gates({pr["number"] for pr in open_prs}), + errors, + ) return errors diff --git a/.github/scripts/pull-request-dashboard/github_cli.py b/.github/scripts/pull-request-dashboard/github_cli.py index 2a9748010af..0288a67c03f 100644 --- a/.github/scripts/pull-request-dashboard/github_cli.py +++ b/.github/scripts/pull-request-dashboard/github_cli.py @@ -118,6 +118,9 @@ def gh_api(path: str, paginate: bool = False, token: str | None = None) -> Any: def request_copilot_review(pull_request_id: str) -> None: + # Success here only means GitHub accepted the mutation. It does not mean + # GitHub recorded the reviewer, so callers have to read the pull request + # back to find out whether the request landed. gh_graphql( REQUEST_COPILOT_REVIEW_MUTATION, { diff --git a/.github/scripts/pull-request-dashboard/pr_status_comment.py b/.github/scripts/pull-request-dashboard/pr_status_comment.py index 8710c7db8dc..248984dff62 100644 --- a/.github/scripts/pull-request-dashboard/pr_status_comment.py +++ b/.github/scripts/pull-request-dashboard/pr_status_comment.py @@ -14,6 +14,7 @@ ) from dashboard_override import PRE_REVIEW_ROUTES, uncleared_ci_failing_count from route_presentation import ( + abandoned_gate_note, outstanding_gate_phrase, route_status_summary, status_headline, @@ -277,6 +278,13 @@ def render_status_comment( else: _, next_step = route_status_summary(route) body = [next_step] + abandoned_gates = ( + abandoned_gate_note(facts) + if facts.get("route_hold_expired") + else "" + ) + if abandoned_gates: + body.extend(["", abandoned_gates]) if failing_count: check_summary = ( "1 required status check is failing." diff --git a/.github/scripts/pull-request-dashboard/route_presentation.py b/.github/scripts/pull-request-dashboard/route_presentation.py index 7bbe6c6cc59..7d1ab2670b7 100644 --- a/.github/scripts/pull-request-dashboard/route_presentation.py +++ b/.github/scripts/pull-request-dashboard/route_presentation.py @@ -64,3 +64,28 @@ def outstanding_gate_phrase(facts: dict[str, Any]) -> str: if facts.get("copilot_review_outstanding"): gates.append("the Copilot review") return " and ".join(gates) + + +def unreported_gate_phrase(facts: dict[str, Any]) -> str: + # Which gate has said nothing at all about the current head. This is not + # the same as the gate that is holding the PR: a Copilot review that left + # findings holds it but has reported, so naming it would send the reader + # after a gate that arrived. + gates = [] + if not facts.get("required_checks_settled"): + gates.append("the required status checks") + if facts.get("copilot_review_unreported"): + gates.append("the Copilot review") + return " and ".join(gates) + + +def abandoned_gate_note(facts: dict[str, Any]) -> str: + # Said once the dashboard has stopped waiting, so the reader knows the + # missing gate is not something they are supposed to sit and wait for. + gates = unreported_gate_phrase(facts) + if not gates: + return "" + return ( + f"The dashboard stopped waiting for {gates} to report, " + "and routed this pull request anyway." + ) diff --git a/.github/scripts/pull-request-dashboard/state.py b/.github/scripts/pull-request-dashboard/state.py index 5c8a1d67b49..bded40ec309 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 = 8 # 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 = 3 # copilot-review-request-state.json: pending and delivered review requests. -COPILOT_REVIEW_REQUEST_STATE_VERSION = 4 +COPILOT_REVIEW_REQUEST_STATE_VERSION = 5 # 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_copilot_review.py b/.github/scripts/pull-request-dashboard/test_copilot_review.py index 7898a714be6..e33fc20730b 100644 --- a/.github/scripts/pull-request-dashboard/test_copilot_review.py +++ b/.github/scripts/pull-request-dashboard/test_copilot_review.py @@ -8,6 +8,7 @@ from datetime import datetime, timezone from copilot_review import ( + REQUEST_CONFIRMATION_ATTEMPTS, copilot_first_review_overdue, deliver_copilot_review_requests, record_copilot_review_observation, @@ -218,7 +219,12 @@ def test_records_request_for_current_head(self, _load_requests, save_requests) - @patch("copilot_review.save_copilot_review_requests") @patch( "copilot_review.load_copilot_review_requests", - return_value={"7": {"head_sha": "old-head", "requested_at": "old-request"}}, + return_value={ + "7": { + "head_sha": "old-head", + "requested_at": "old-request", + } + }, ) def test_new_head_replaces_previous_request(self, _load_requests, save_requests) -> None: record_copilot_review_observation( @@ -283,7 +289,12 @@ def test_same_head_request_needed_resets_acknowledgement( @patch("copilot_review.save_copilot_review_requests") @patch( "copilot_review.load_copilot_review_requests", - return_value={"7": {"head_sha": "current-head", "requested_at": ""}}, + return_value={ + "7": { + "head_sha": "current-head", + "requested_at": "", + } + }, ) def test_clears_request_when_no_longer_needed(self, _load_requests, save_requests) -> None: record_copilot_review_observation( @@ -322,6 +333,10 @@ def test_missing_first_review_within_grace_does_not_enqueue_request( save_requests.assert_called_once_with({}) + @patch( + "copilot_review.fetch_review_requests", + return_value=[{"__typename": "Bot", "login": "copilot-pull-request-reviewer"}], + ) @patch( "copilot_review.routing_input_fingerprint", return_value="accepted-fingerprint", @@ -349,6 +364,7 @@ def test_delivers_request_for_current_stale_review( fetch_reviews, request_review, _fingerprint, + fetch_pending_requests, ) -> None: pr = { "state": "OPEN", @@ -374,6 +390,7 @@ def test_delivers_request_for_current_stale_review( fetch_current_state.assert_called_once_with("open-telemetry/example", 7) fetch_reviews.assert_called_once_with("open-telemetry", "example", 7) request_review.assert_called_once_with("PR_node_id") + fetch_pending_requests.assert_called_once_with("open-telemetry", "example", 7) save_requests.assert_called_once_with({ "7": { "head_sha": "current-head", @@ -574,6 +591,10 @@ def test_drops_request_when_copilot_review_no_longer_needed( stderr.getvalue(), ) + @patch( + "copilot_review.fetch_review_requests", + return_value=[{"__typename": "Bot", "login": "copilot-pull-request-reviewer"}], + ) @patch( "copilot_review.routing_input_fingerprint", return_value="accepted-fingerprint", @@ -612,6 +633,7 @@ def test_delivers_request_for_missing_first_review( _fetch_reviews, request_review, _fingerprint, + _fetch_pending_requests, ) -> None: errors = deliver_copilot_review_requests("open-telemetry/example", NOW) @@ -626,6 +648,141 @@ def test_delivers_request_for_missing_first_review( }, }) + @patch("copilot_review.sleep_for_retry") + @patch("copilot_review.fetch_review_requests", return_value=[]) + @patch( + "copilot_review.routing_input_fingerprint", + return_value="accepted-fingerprint", + ) + @patch("copilot_review.request_copilot_review") + @patch("copilot_review.fetch_pr_reviews", return_value=[]) + @patch( + "copilot_review.fetch_current_pr_routing_inputs", + return_value=( + { + "id": "PR_node", + "state": "OPEN", + "isDraft": False, + "headRefOid": "current-head", + }, + {"checks": []}, + ), + ) + @patch("copilot_review.save_copilot_review_requests") + @patch( + "copilot_review.load_copilot_review_requests", + return_value={ + "7": { + "head_sha": "current-head", + "observed_at": "2026-07-20T01:00:00+00:00", + "requested_at": "", + "routing_input_fingerprint": "accepted-fingerprint", + } + }, + ) + def test_dropped_request_is_not_recorded_as_delivered( + self, + _load_requests, + save_requests, + _fetch_current_state, + _fetch_reviews, + _request_review, + _fingerprint, + fetch_pending_requests, + _sleep, + ) -> None: + stderr = io.StringIO() + + with redirect_stderr(stderr): + errors = deliver_copilot_review_requests("open-telemetry/example", NOW) + + self.assertEqual([], errors) + self.assertEqual( + REQUEST_CONFIRMATION_ATTEMPTS, + fetch_pending_requests.call_count, + ) + save_requests.assert_called_once_with({ + "7": { + "head_sha": "current-head", + "observed_at": "2026-07-20T01:00:00+00:00", + "requested_at": "", + "routing_input_fingerprint": "accepted-fingerprint", + }, + }) + self.assertIn( + "GitHub did not record the Copilot review request for PR #7 on " + "head current-head", + stderr.getvalue(), + ) + + @patch("copilot_review.sleep_for_retry") + @patch("copilot_review.fetch_review_requests", return_value=[]) + @patch( + "copilot_review.routing_input_fingerprint", + return_value="accepted-fingerprint", + ) + @patch("copilot_review.request_copilot_review") + @patch( + "copilot_review.fetch_pr_reviews", + side_effect=[ + [], + [ + { + "id": 20, + "commit_id": "current-head", + "user": {"login": "copilot-pull-request-reviewer"}, + "submitted_at": "2026-07-20T02:00:00Z", + } + ], + ], + ) + @patch( + "copilot_review.fetch_current_pr_routing_inputs", + return_value=( + { + "id": "PR_node", + "state": "OPEN", + "isDraft": False, + "headRefOid": "current-head", + }, + {"checks": []}, + ), + ) + @patch("copilot_review.save_copilot_review_requests") + @patch( + "copilot_review.load_copilot_review_requests", + return_value={ + "7": { + "head_sha": "current-head", + "observed_at": "2026-07-20T01:00:00+00:00", + "requested_at": "", + "routing_input_fingerprint": "accepted-fingerprint", + } + }, + ) + def test_review_that_arrives_before_the_read_counts_as_delivered( + self, + _load_requests, + save_requests, + _fetch_current_state, + _fetch_reviews, + _request_review, + _fingerprint, + _fetch_pending_requests, + _sleep, + ) -> None: + errors = deliver_copilot_review_requests("open-telemetry/example", NOW) + + self.assertEqual([], errors) + save_requests.assert_called_once_with({ + "7": { + "head_sha": "current-head", + "observed_at": "2026-07-20T01:00:00+00:00", + "requested_at": "2026-07-20T02:00:00+00:00", + "routing_input_fingerprint": "accepted-fingerprint", + }, + }) + class StaleRequestReasonTest(unittest.TestCase): ENTRY = { diff --git a/.github/scripts/pull-request-dashboard/test_dashboard.py b/.github/scripts/pull-request-dashboard/test_dashboard.py index f61329b6072..08dcbca1a91 100644 --- a/.github/scripts/pull-request-dashboard/test_dashboard.py +++ b/.github/scripts/pull-request-dashboard/test_dashboard.py @@ -2,6 +2,7 @@ from argparse import Namespace from copy import deepcopy +from datetime import datetime, timedelta, timezone from pathlib import Path import tempfile import unittest @@ -10,6 +11,7 @@ from copilot_review import set_copilot_review_request_needed from dashboard import ( BACKFILL_RECORDED_FAILURE_STATUS, + GATE_HOLD_LIMIT, DashboardUpdate, add_wait_age_facts, apply_targeted_dashboard_update, @@ -357,18 +359,22 @@ def test_completed_author_reply_without_approval_waits_on_reviewers(self) -> Non class GateHoldTest(unittest.TestCase): + START = datetime(2026, 8, 16, 12, 0, tzinfo=timezone.utc) + def _hold( self, facts: dict[str, object], route: str, previous_result: dict[str, object] | None, require_clean_copilot_review: bool = False, + now: datetime | None = None, ) -> str: return hold_route_until_gates_settle( facts, route, previous_result, require_clean_copilot_review=require_clean_copilot_review, + now=now or self.START, ) def test_author_keeps_the_pr_while_replacement_checks_run(self) -> None: @@ -436,6 +442,237 @@ def test_a_held_maintenance_bot_pr_is_never_routed_to_its_author(self) -> None: self.assertEqual("approver", route) + def test_a_held_pr_starts_the_hold_clock(self) -> None: + facts: dict[str, object] = {"ci_pending_count": 1, "head_sha": "abc"} + + self._hold(facts, "approver", None, now=self.START) + + self.assertEqual("2026-08-16T12:00:00+00:00", facts["route_held_since"]) + self.assertFalse(facts["route_hold_expired"]) + + def test_the_hold_clock_keeps_running_on_the_same_head(self) -> None: + facts: dict[str, object] = {"ci_pending_count": 1, "head_sha": "abc"} + + self._hold( + facts, + "approver", + { + "route": "author", + "facts": {"head_sha": "abc", "route_held_since": "2026-08-16T09:00:00+00:00"}, + }, + now=self.START, + ) + + self.assertEqual("2026-08-16T09:00:00+00:00", facts["route_held_since"]) + + def test_a_push_restarts_the_hold_clock(self) -> None: + facts: dict[str, object] = {"ci_pending_count": 1, "head_sha": "def"} + + self._hold( + facts, + "approver", + { + "route": "author", + "facts": {"head_sha": "abc", "route_held_since": "2026-08-16T09:00:00+00:00"}, + }, + now=self.START, + ) + + self.assertEqual("2026-08-16T12:00:00+00:00", facts["route_held_since"]) + + def test_settled_gates_clear_the_hold_clock(self) -> None: + facts: dict[str, object] = { + "ci_failing_count": 0, + "ci_pending_count": 0, + "head_sha": "abc", + } + + self._hold( + facts, + "approver", + { + "route": "author", + "facts": {"head_sha": "abc", "route_held_since": "2026-08-16T09:00:00+00:00"}, + }, + now=self.START, + ) + + self.assertNotIn("route_held_since", facts) + self.assertFalse(facts["route_hold_expired"]) + + def test_a_gate_that_never_reports_stops_holding_the_pr(self) -> None: + facts: dict[str, object] = {"ci_pending_count": 1, "head_sha": "abc"} + held_since = self.START - GATE_HOLD_LIMIT + + route = self._hold( + facts, + "approver", + { + "route": "author", + "facts": { + "head_sha": "abc", + "route_held_since": held_since.isoformat(), + }, + }, + now=self.START, + ) + + self.assertEqual("approver", route) + self.assertFalse(facts["route_held_for_gates"]) + self.assertTrue(facts["route_hold_expired"]) + + def test_a_gate_still_missing_after_release_stays_reported(self) -> None: + # Releasing the pull request does not make the stall look resolved: the + # clock carries on while the same head has an outstanding gate. + facts: dict[str, object] = {"ci_pending_count": 1, "head_sha": "abc"} + held_since = self.START - GATE_HOLD_LIMIT - timedelta(hours=1) + + route = self._hold( + facts, + "approver", + { + "route": "approver", + "facts": { + "head_sha": "abc", + "route_held_since": held_since.isoformat(), + }, + }, + now=self.START, + ) + + self.assertEqual("approver", route) + self.assertTrue(facts["route_hold_expired"]) + + def test_checks_that_never_held_the_pr_do_not_start_the_clock(self) -> None: + # An approved pull request whose author pushes is already with its + # reviewers, so a slow check suite is not a stalled handoff. + facts: dict[str, object] = {"ci_pending_count": 1, "head_sha": "abc"} + + self._hold( + facts, + "maintainer", + {"route": "maintainer", "facts": {"head_sha": "abc"}}, + now=self.START, + ) + + self.assertNotIn("route_held_since", facts) + self.assertFalse(facts["route_hold_expired"]) + + def test_a_pr_sent_back_to_its_author_keeps_the_clock(self) -> None: + # The author owes this PR something, but the gate is still missing on + # the same code, so the round trip must not buy it a fresh four hours. + facts: dict[str, object] = { + "ci_pending_count": 1, + "ci_failing_count": 1, + "head_sha": "abc", + } + held_since = self.START - GATE_HOLD_LIMIT - timedelta(hours=1) + + route = self._hold( + facts, + "author", + { + "route": "approver", + "facts": { + "head_sha": "abc", + "route_held_since": held_since.isoformat(), + }, + }, + now=self.START, + ) + + self.assertEqual("author", route) + self.assertEqual(held_since.isoformat(), facts["route_held_since"]) + self.assertTrue(facts["route_hold_expired"]) + self.assertFalse(facts["route_held_for_gates"]) + + def test_an_author_round_trip_does_not_restart_an_expired_hold(self) -> None: + # The author replies without pushing, so the same never-reporting gate + # would otherwise hold the PR for another four hours. + facts: dict[str, object] = {"ci_pending_count": 1, "head_sha": "abc"} + held_since = self.START - GATE_HOLD_LIMIT - timedelta(hours=1) + + route = self._hold( + facts, + "approver", + { + "route": "author", + "facts": { + "head_sha": "abc", + "route_held_since": held_since.isoformat(), + }, + }, + now=self.START + timedelta(minutes=20), + ) + + self.assertEqual("approver", route) + self.assertEqual(held_since.isoformat(), facts["route_held_since"]) + self.assertTrue(facts["route_hold_expired"]) + + def test_copilot_findings_on_the_current_head_do_not_stall(self) -> None: + # Copilot reported on this head and left findings, so the PR is with + # its author over review comments, not waiting on a gate GitHub lost. + facts: dict[str, object] = { + "ci_failing_count": 0, + "ci_pending_count": 0, + "head_sha": "abc", + "copilot_review_exists": True, + "copilot_review_stale": False, + "copilot_review_needed": True, + } + held_since = self.START - GATE_HOLD_LIMIT - timedelta(hours=1) + + route = self._hold( + facts, + "author", + { + "route": "approver", + "facts": { + "head_sha": "abc", + "route_held_since": held_since.isoformat(), + }, + }, + require_clean_copilot_review=True, + now=self.START, + ) + + self.assertEqual("author", route) + self.assertTrue(facts["copilot_review_outstanding"]) + self.assertFalse(facts["copilot_review_unreported"]) + self.assertNotIn("route_held_since", facts) + self.assertFalse(facts["route_hold_expired"]) + + def test_a_review_that_only_covers_older_code_still_stalls(self) -> None: + # Copilot has said nothing about this head, so the wait for it is real + # even though an older review exists. + facts: dict[str, object] = { + "ci_failing_count": 0, + "ci_pending_count": 0, + "head_sha": "abc", + "copilot_review_exists": True, + "copilot_review_stale": True, + "copilot_review_needed": True, + } + held_since = self.START - GATE_HOLD_LIMIT + + route = self._hold( + facts, + "approver", + { + "route": "author", + "facts": { + "head_sha": "abc", + "route_held_since": held_since.isoformat(), + }, + }, + require_clean_copilot_review=True, + now=self.START, + ) + + self.assertEqual("approver", route) + self.assertTrue(facts["copilot_review_unreported"]) + self.assertTrue(facts["route_hold_expired"]) + def test_held_route_carries_the_previous_wait_forward(self) -> None: facts = { "route_held_for_gates": True, @@ -479,6 +716,102 @@ def test_released_route_recomputes_the_wait(self) -> None: self.assertEqual("2026-07-10T01:00:00+00:00", facts["waiting_since"]) self.assertEqual("last_approver_activity", facts["waiting_age_basis"]) + def test_reviewers_start_waiting_when_the_gates_release_the_pr(self) -> None: + # The push was four hours ago; the reviewers could not have answered + # any of it, because the gates held the PR on its author throughout. + facts = { + "route_held_for_gates": False, + "last_author_activity_at": "2026-08-16T08:00:00+00:00", + } + + add_wait_age_facts( + facts, + "approver", + {}, + { + "route": "author", + "facts": { + "route_held_for_gates": True, + "waiting_since": "2026-08-16T08:00:00+00:00", + }, + }, + now=self.START, + ) + + self.assertEqual("2026-08-16T12:00:00+00:00", facts["waiting_since"]) + self.assertEqual("gate_release", facts["waiting_age_basis"]) + + def test_the_gate_release_wait_survives_the_next_refresh(self) -> None: + # The release only happens once, so the wait it started has to be + # carried, or the very next refresh dates the PR from the push again. + facts = { + "route_held_for_gates": False, + "head_sha": "abc", + "last_author_activity_at": "2026-08-16T08:00:00+00:00", + } + + add_wait_age_facts( + facts, + "approver", + {}, + { + "route": "approver", + "facts": { + "head_sha": "abc", + "waiting_since": "2026-08-16T12:00:00+00:00", + "waiting_age_basis": "gate_release", + }, + }, + now=self.START + timedelta(hours=1), + ) + + self.assertEqual("2026-08-16T12:00:00+00:00", facts["waiting_since"]) + self.assertEqual("gate_release", facts["waiting_age_basis"]) + + def test_an_unheld_handoff_still_dates_from_the_push(self) -> None: + facts = { + "route_held_for_gates": False, + "last_author_activity_at": "2026-08-16T08:00:00+00:00", + } + + add_wait_age_facts( + facts, + "approver", + {}, + { + "route": "author", + "facts": {"waiting_since": "2026-08-10T01:00:00+00:00"}, + }, + now=self.START, + ) + + self.assertEqual("2026-08-16T08:00:00+00:00", facts["waiting_since"]) + self.assertEqual("last_author_activity", facts["waiting_age_basis"]) + + def test_a_release_to_maintainers_keeps_the_reviewer_wait(self) -> None: + # This PR never left the people who owe it a response, so the merge + # request is as old as the review that produced it. + facts = { + "route_held_for_gates": False, + "last_author_activity_at": "2026-08-16T08:00:00+00:00", + } + + add_wait_age_facts( + facts, + "maintainer", + {}, + { + "route": "approver", + "facts": { + "route_held_for_gates": True, + "waiting_since": "2026-08-10T01:00:00+00:00", + }, + }, + now=self.START, + ) + + self.assertEqual("2026-08-10T01:00:00+00:00", facts["waiting_since"]) + class ReviewerWaitTest(unittest.TestCase): def test_author_push_does_not_restart_the_reviewer_wait(self) -> None: diff --git a/.github/scripts/pull-request-dashboard/test_delivery.py b/.github/scripts/pull-request-dashboard/test_delivery.py index acf0122c62d..b04eceb9d2f 100644 --- a/.github/scripts/pull-request-dashboard/test_delivery.py +++ b/.github/scripts/pull-request-dashboard/test_delivery.py @@ -9,6 +9,7 @@ class DeliveryTest(unittest.TestCase): + @patch.object(delivery, "report_stalled_gates", return_value=[]) @patch.object(delivery, "notify_slack_from_state", return_value=[]) @patch.object(delivery, "deliver_copilot_review_requests", return_value=[]) @patch.object(delivery, "deliver_prepared_author_nudges", return_value=[]) @@ -30,6 +31,7 @@ def test_runs_all_repository_deliveries_in_order( author_nudges, copilot_reviews, slack, + stalled_gates, ) -> None: order = Mock() @@ -42,6 +44,7 @@ def record(label: str) -> list[str]: author_nudges.side_effect = lambda *_args: record("author") copilot_reviews.side_effect = lambda *_args: record("copilot") slack.side_effect = lambda *_args: record("slack") + stalled_gates.side_effect = lambda *_args: record("stalled") errors = delivery.deliver_from_state( "open-telemetry/example", Path("author"), @@ -52,7 +55,14 @@ def record(label: str) -> list[str]: self.assertEqual([], errors) _list_open.assert_called_once_with("open-telemetry/example") self.assertEqual( - [call("replies"), call("author"), call("status"), call("copilot"), call("slack")], + [ + call("replies"), + call("author"), + call("status"), + call("copilot"), + call("slack"), + call("stalled"), + ], order.call_args_list, ) status_comments.assert_called_once_with( @@ -68,7 +78,9 @@ def record(label: str) -> list[str]: ], ANY, ) + stalled_gates.assert_called_once_with({7, 8}) + @patch.object(delivery, "report_stalled_gates", return_value=[]) @patch.object(delivery, "notify_slack_from_state", return_value=[]) @patch.object(delivery, "deliver_copilot_review_requests", return_value=[]) @patch.object(delivery, "deliver_prepared_author_nudges", return_value=[]) @@ -87,6 +99,7 @@ def test_failure_does_not_block_later_deliveries( author_nudges, copilot_reviews, slack, + stalled_gates, ) -> None: errors = delivery.deliver_from_state( "open-telemetry/example", @@ -100,6 +113,7 @@ def test_failure_does_not_block_later_deliveries( author_nudges.assert_called_once() copilot_reviews.assert_called_once() slack.assert_called_once() + stalled_gates.assert_called_once_with({7}) def test_open_pr_list_failure_skips_dependent_stages(self) -> None: with ( @@ -164,6 +178,110 @@ def test_targeted_delivery_only_processes_triggering_pr(self) -> None: {7}, ) + def test_a_stalled_gate_is_reported_when_the_whole_repository_runs(self) -> None: + state = { + "prs": { + "7": { + "facts": { + "route_hold_expired": True, + "copilot_review_outstanding": True, + "copilot_review_unreported": True, + "required_checks_settled": True, + "head_sha": "abc", + } + }, + "8": {"facts": {"route_held_for_gates": True}}, + } + } + + with patch.object(delivery, "load_dashboard_state_cache", return_value=state): + errors = delivery.report_stalled_gates({7, 8}) + + self.assertEqual( + ["PR #7: the Copilot review never reported on head abc"], + errors, + ) + + def test_a_copilot_review_that_reported_is_not_named_as_the_stall(self) -> None: + # The checks are what went missing. Copilot answered on this head, so + # naming it would send the reader after a gate that is not missing. + state = { + "prs": { + "7": { + "facts": { + "route_hold_expired": True, + "copilot_review_outstanding": True, + "copilot_review_unreported": False, + "required_checks_settled": False, + "head_sha": "abc", + } + } + } + } + + with patch.object(delivery, "load_dashboard_state_cache", return_value=state): + errors = delivery.report_stalled_gates({7}) + + self.assertEqual( + ["PR #7: the required status checks never reported on head abc"], + errors, + ) + + def test_an_expired_hold_with_every_gate_reported_is_not_reported(self) -> None: + state = { + "prs": { + "7": { + "facts": { + "route_hold_expired": True, + "copilot_review_unreported": False, + "required_checks_settled": True, + "head_sha": "abc", + } + } + } + } + + with patch.object(delivery, "load_dashboard_state_cache", return_value=state): + errors = delivery.report_stalled_gates({7}) + + self.assertEqual([], errors) + + def test_a_stalled_gate_on_a_closed_pr_is_not_reported(self) -> None: + state = {"prs": {"7": {"facts": {"route_hold_expired": True}}}} + + with patch.object(delivery, "load_dashboard_state_cache", return_value=state): + errors = delivery.report_stalled_gates(set()) + + self.assertEqual([], errors) + + def test_a_targeted_delivery_does_not_report_stalled_gates(self) -> None: + with ( + patch.object( + delivery, + "gh_api", + return_value={"state": "open", "draft": False, "title": "Seven"}, + ), + patch.object(delivery, "deliver_dashboard_command_replies", return_value=[]), + patch.object(delivery, "deliver_prepared_author_nudges", return_value=[]), + patch.object( + delivery, + "update_targeted_status_comment_from_state", + return_value=[], + ), + patch.object(delivery, "deliver_copilot_review_requests", return_value=[]), + patch.object(delivery, "notify_slack_from_state", return_value=[]), + patch.object(delivery, "report_stalled_gates", return_value=[]) as stalled, + ): + delivery.deliver_from_state( + "open-telemetry/example", + Path("author"), + Path("copilot"), + Path("slack"), + 7, + ) + + stalled.assert_not_called() + @patch.object(delivery.sys, "stderr") @patch.object(delivery, "deliver_from_state", return_value=["status comments: boom"]) @patch.object(delivery, "claim_delivery_versions", return_value=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 7c17ed67957..54b7073e9e9 100644 --- a/.github/scripts/pull-request-dashboard/test_pr_status_comment.py +++ b/.github/scripts/pull-request-dashboard/test_pr_status_comment.py @@ -258,6 +258,28 @@ def test_held_pr_names_only_the_outstanding_copilot_gate(self) -> None: self.assertIn("Wait for the Copilot review to report;", body) + def test_a_pr_released_from_a_stalled_gate_says_so(self) -> None: + body = pr_status_comment.render_status_comment( + self.pr(), + { + "route": "approver", + "facts": { + "author": "alice", + "route_held_for_gates": False, + "route_hold_expired": True, + "required_checks_settled": True, + "copilot_review_outstanding": True, + "copilot_review_unreported": True, + }, + }, + ) + + self.assertIn( + "The dashboard stopped waiting for the Copilot review to report, " + "and routed this pull request anyway.", + 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_state.py b/.github/scripts/pull-request-dashboard/test_state.py index 3d87ca02f5f..4555bd0a176 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, 7) + self.assertEqual(DASHBOARD_STATE_VERSION, 8) self.assertEqual(STATUS_COMMENT_ROLLOUT_STATE_VERSION, 1) self.assertEqual(AUTHOR_NUDGE_STATE_VERSION, 3) - self.assertEqual(COPILOT_REVIEW_REQUEST_STATE_VERSION, 4) + self.assertEqual(COPILOT_REVIEW_REQUEST_STATE_VERSION, 5) 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/pull-request-dashboard/README.md b/pull-request-dashboard/README.md index c3ef3935241..1ac5a72bf05 100644 --- a/pull-request-dashboard/README.md +++ b/pull-request-dashboard/README.md @@ -72,7 +72,7 @@ Fields: | `required_approvals` | no | Number of approvals required for an open PR to be marked ready to merge. Defaults to `1`. | | `labels_to_display` | no | Case-sensitive shell-style label name patterns to display inline after PR titles. Exact names such as `breaking change` and wildcard patterns such as `size/*` are supported. Defaults to `[]`, which displays no labels. | | `non_blocking_check_patterns` | no | Check-name globs for non-required checks whose failures should be identified in the live PR status comment. When the PR is waiting on the author, matching failures are reported only when at least one required check is failing and are noted alongside those failures. On other routes, matching failures are shown separately. Matching checks remain informational and do not affect routing or the dashboard CI column. | -| `require_clean_copilot_review_branches` | no | List of base branch names for which a Copilot review of the current head with no open Copilot review threads is required before automatically routing a PR to reviewers or maintainers. An effective `/dashboard route:reviewers` override bypasses this gate. A thread counts as open while it is unresolved and GitHub has not marked it outdated, so a thread whose code the author has since rewritten stops holding the PR even if nobody resolved it. The dashboard re-requests Copilot review when a push has left the previous review stale, and requests the first review itself if automatic Copilot code review has not produced one within an hour of the PR becoming ready. It does not duplicate a pending request. List only branches where automatic Copilot code review is enabled (typically `["main"]`); PRs targeting any other branch are never gated, so they cannot stall waiting for a review that never runs. Defaults to `[]` (no branches gated). | +| `require_clean_copilot_review_branches` | no | List of base branch names for which a Copilot review of the current head with no open Copilot review threads is required before automatically routing a PR to reviewers or maintainers. An effective `/dashboard route:reviewers` override bypasses this gate. A thread counts as open while it is unresolved and GitHub has not marked it outdated, so a thread whose code the author has since rewritten stops holding the PR even if nobody resolved it. The dashboard re-requests Copilot review when a push has left the previous review stale, and requests the first review itself if automatic Copilot code review has not produced one within an hour of the PR becoming ready. It does not duplicate a pending request. A request counts as delivered only once GitHub confirms Copilot is a pending reviewer or finds a completed Copilot review of the current head, so one that GitHub accepts but does not record in either form is sent again on the next pass. A gate that never reports holds the PR for at most four hours; after that the PR routes anyway, its status comment says which gate the dashboard stopped waiting for, and the run reports the stall. List only branches where automatic Copilot code review is enabled (typically `["main"]`); PRs targeting any other branch are never gated, so they cannot stall waiting for a review that never runs. Defaults to `[]` (no branches gated). | | `slack_channel` | no | Slack channel for notifications. Omit to skip Slack processing for this repository. | | `slack_user_mapping` | no | Map of GitHub login to Slack user ID for at-mentions. | | `large_repo` | no | If `true`, apply rendering presets that keep the dashboard body under GitHub's 65,536-character issue-body limit: cap each section (each *Waiting on …* table, the *Draft pull requests* table, and the *Diagnostics* block) at 100 rows, and omit the *Draft pull requests* section entirely. Truncated sections get a `_More X PRs not shown_` footer. Defaults to `false` (no cap, drafts shown). Enable this for very large repos with hundreds of PRs. |