From 4b3e62718fd6991b52b66305acea7712eaaeeb68 Mon Sep 17 00:00:00 2001 From: Trask Stalnaker Date: Sun, 16 Aug 2026 08:44:11 -0700 Subject: [PATCH 01/11] Confirm GitHub recorded a Copilot review request before calling it delivered Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../pull-request-dashboard/RATIONALE.md | 17 ++ .../pull-request-dashboard/copilot_review.py | 85 +++++- .../pull-request-dashboard/github_cli.py | 3 + .../scripts/pull-request-dashboard/state.py | 13 +- .../test_copilot_review.py | 267 +++++++++++++++++- pull-request-dashboard/README.md | 2 +- 6 files changed, 380 insertions(+), 7 deletions(-) diff --git a/.github/scripts/pull-request-dashboard/RATIONALE.md b/.github/scripts/pull-request-dashboard/RATIONALE.md index ac1d5a27990..87b3c06f6a0 100644 --- a/.github/scripts/pull-request-dashboard/RATIONALE.md +++ b/.github/scripts/pull-request-dashboard/RATIONALE.md @@ -352,6 +352,23 @@ 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 the entry counts how many have gone missing on the current head. Three in + a row fail the run, which opens the hourly failure issue and closes it again + once a request lands. Failing on the first miss would report GitHub's ordinary + lag as a breakage; never failing would repeat that nineteen-hour wait with + nobody watching. - 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..d38f51f21fe 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,19 @@ 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 + + +# How many requests GitHub may drop before the run fails. GitHub has answered +# the mutation with success and recorded nothing for the same pull request +# every hour for a day. The pull request stays held on a review nobody is going +# to run, and only a person can unstick it, so the failure has to reach one. +UNCONFIRMED_REQUEST_LIMIT = 3 + + def is_copilot_reviewer(obj: dict[str, Any] | None) -> bool: return is_copilot_reviewer_login(actor_login(obj)) @@ -167,11 +182,21 @@ def record_copilot_review_observation( ): requests.pop(key, None) else: + # The count of requests GitHub has dropped belongs to the head they + # were sent for, so it survives a fresh observation of that same head + # and starts over on a new one. + previous = requests.get(key) or {} + unconfirmed = ( + int(previous.get("unconfirmed_request_count") or 0) + if previous.get("head_sha") == head_sha + else 0 + ) requests[key] = { "head_sha": head_sha, "observed_at": format_ts(observed_at), "requested_at": "", "routing_input_fingerprint": routing_fingerprint, + "unconfirmed_request_count": unconfirmed, } save_copilot_review_requests(requests) @@ -222,6 +247,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, @@ -260,7 +311,11 @@ def deliver_copilot_review_requests( is_copilot_reviewer(request) for request in (raw.get("review_requests") or []) ): - requests[key] = {**entry, "requested_at": format_ts(now)} + requests[key] = { + **entry, + "requested_at": format_ts(now), + "unconfirmed_request_count": 0, + } continue reviews = fetch_pr_reviews(owner, repo_name, pr_number) or [] review_exists, review_stale, _review_findings = copilot_review_status( @@ -285,9 +340,35 @@ 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), + "unconfirmed_request_count": 0, + } + continue + # Leaving the request undelivered keeps the next pass trying, which is + # what recovered the one pull request this was seen on. The count is + # what turns an hour of bad luck into a failure someone acts on. + unconfirmed = int(entry.get("unconfirmed_request_count") or 0) + 1 + requests[key] = {**entry, "unconfirmed_request_count": unconfirmed} + message = ( + f"GitHub did not record the Copilot review request for " + f"PR #{pr_number} on head {current_head}; " + f"{unconfirmed} in a row have gone missing" + ) + if unconfirmed >= UNCONFIRMED_REQUEST_LIMIT: + errors.append(f"PR #{pr_number}: {message}") + else: + print(message, file=sys.stderr) save_copilot_review_requests(requests) return errors \ No newline at end of file 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/state.py b/.github/scripts/pull-request-dashboard/state.py index 5c8a1d67b49..9ef405c2306 100644 --- a/.github/scripts/pull-request-dashboard/state.py +++ b/.github/scripts/pull-request-dashboard/state.py @@ -413,12 +413,19 @@ def union_merge_copilot_review_requests( merged = dict(baseline_requests) for key, retry_entry in retry_snapshot_requests.items(): baseline_entry = merged.get(key) or {} - if ( - (retry_entry or {}).get("requested_at") - and retry_entry.get("head_sha") == baseline_entry.get("head_sha") + retry_entry = retry_entry or {} + if not ( + retry_entry.get("head_sha") == baseline_entry.get("head_sha") and retry_entry.get("observed_at") and retry_entry.get("observed_at") == baseline_entry.get("observed_at") ): + continue + # A delivered request has to survive so the next attempt does not send + # it again, and so does a count of requests GitHub dropped, because + # losing it would restart the wait for a request that never lands. + if retry_entry.get("requested_at") or int( + retry_entry.get("unconfirmed_request_count") or 0 + ) > int(baseline_entry.get("unconfirmed_request_count") or 0): merged[key] = retry_entry return merged diff --git a/.github/scripts/pull-request-dashboard/test_copilot_review.py b/.github/scripts/pull-request-dashboard/test_copilot_review.py index 7898a714be6..8120cef6677 100644 --- a/.github/scripts/pull-request-dashboard/test_copilot_review.py +++ b/.github/scripts/pull-request-dashboard/test_copilot_review.py @@ -8,6 +8,8 @@ from datetime import datetime, timezone from copilot_review import ( + REQUEST_CONFIRMATION_ATTEMPTS, + UNCONFIRMED_REQUEST_LIMIT, copilot_first_review_overdue, deliver_copilot_review_requests, record_copilot_review_observation, @@ -212,13 +214,20 @@ def test_records_request_for_current_head(self, _load_requests, save_requests) - "observed_at": "2026-07-20T02:00:00+00:00", "requested_at": "", "routing_input_fingerprint": "accepted-fingerprint", + "unconfirmed_request_count": 0, }, }) @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", + "unconfirmed_request_count": 2, + } + }, ) def test_new_head_replaces_previous_request(self, _load_requests, save_requests) -> None: record_copilot_review_observation( @@ -240,6 +249,7 @@ def test_new_head_replaces_previous_request(self, _load_requests, save_requests) "observed_at": "2026-07-20T02:00:00+00:00", "requested_at": "", "routing_input_fingerprint": "accepted-fingerprint", + "unconfirmed_request_count": 0, }, }) @@ -277,6 +287,46 @@ def test_same_head_request_needed_resets_acknowledgement( "observed_at": "2026-07-20T02:00:00+00:00", "requested_at": "", "routing_input_fingerprint": "accepted-fingerprint", + "unconfirmed_request_count": 0, + }, + }) + + @patch("copilot_review.save_copilot_review_requests") + @patch( + "copilot_review.load_copilot_review_requests", + return_value={ + "7": { + "head_sha": "current-head", + "requested_at": "", + "unconfirmed_request_count": 2, + } + }, + ) + def test_same_head_keeps_count_of_dropped_requests( + self, + _load_requests, + save_requests, + ) -> None: + record_copilot_review_observation( + 7, + { + "route": "approver", + "facts": { + "head_sha": "current-head", + "copilot_review_request_needed": True, + "routing_input_fingerprint": "accepted-fingerprint", + }, + }, + NOW, + ) + + save_requests.assert_called_once_with({ + "7": { + "head_sha": "current-head", + "observed_at": "2026-07-20T02:00:00+00:00", + "requested_at": "", + "routing_input_fingerprint": "accepted-fingerprint", + "unconfirmed_request_count": 2, }, }) @@ -322,6 +372,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 +403,7 @@ def test_delivers_request_for_current_stale_review( fetch_reviews, request_review, _fingerprint, + fetch_pending_requests, ) -> None: pr = { "state": "OPEN", @@ -374,12 +429,14 @@ 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", "observed_at": "2026-07-20T01:00:00+00:00", "requested_at": "2026-07-20T02:00:00+00:00", "routing_input_fingerprint": "accepted-fingerprint", + "unconfirmed_request_count": 0, }, }) @@ -444,6 +501,7 @@ def test_pending_request_is_acknowledged_from_pull_response( "observed_at": "2026-07-20T01:00:00+00:00", "requested_at": "2026-07-20T02:00:00+00:00", "routing_input_fingerprint": "accepted-fingerprint", + "unconfirmed_request_count": 0, }, }) @@ -574,6 +632,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 +674,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) @@ -623,6 +686,208 @@ def test_delivers_request_for_missing_first_review( "observed_at": "2026-07-20T01:00:00+00:00", "requested_at": "2026-07-20T02:00:00+00:00", "routing_input_fingerprint": "accepted-fingerprint", + "unconfirmed_request_count": 0, + }, + }) + + @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", + "unconfirmed_request_count": 1, + }, + }) + self.assertIn( + "GitHub did not record the Copilot review request for PR #7 on " + "head current-head; 1 in a row have gone missing", + 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", 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", + "unconfirmed_request_count": UNCONFIRMED_REQUEST_LIMIT - 1, + } + }, + ) + def test_repeatedly_dropped_request_fails_the_run( + 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( + [ + f"PR #7: GitHub did not record the Copilot review request for " + f"PR #7 on head current-head; {UNCONFIRMED_REQUEST_LIMIT} in a " + f"row have gone missing" + ], + errors, + ) + 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", + "unconfirmed_request_count": UNCONFIRMED_REQUEST_LIMIT, + }, + }) + + @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", + "unconfirmed_request_count": 0, }, }) diff --git a/pull-request-dashboard/README.md b/pull-request-dashboard/README.md index c3ef3935241..db0987e13ac 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, so one that GitHub accepts but does not record is sent again on the next pass. 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. | From 47bbd0f3c0be7e4114f2b7f1b169ce2b946bde37 Mon Sep 17 00:00:00 2001 From: Trask Stalnaker Date: Sun, 16 Aug 2026 14:38:41 -0700 Subject: [PATCH 02/11] Stop the Copilot review gate from holding a pull request forever The gate holds a pull request on its author until the required checks and the Copilot review report. Each of those waits was unbounded, so any gate that never reported held the pull request for as long as it stayed open. Three ways that happens turned up in three days: GitHub never started the automatic first review, GitHub accepted a review request and recorded nothing, and a request was never made because the pull request was last refreshed while CI was still running and nothing looked at it again. Bound the hold. A gate may keep a pull request off its reviewers for four hours; past that the pull request routes anyway and its status comment says which gate the dashboard stopped waiting for. The dashboard cannot block a merge, so an endless hold protects nobody and only hides the pull request. Refresh waiting pull requests first. The hourly pass rotated through open pull requests by number, so on a repository with more of them than one pass holds, a wait that ended with nothing changing on the pull request went unnoticed for hours. Pull requests whose stored facts show an unfinished wait now go first, capped at half the pass so the rotation cannot starve. Report an expired hold once, as a delivery failure, which opens the tracking issue the dashboard already uses. Each way a gate goes missing has its own cause and none can be told apart from here, so this replaces the per-request escalation added for dropped review requests. A dropped request is still logged and still retried; the hold expiring is what reports it. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: e669e830-2655-44d1-b960-7d3d63a4e2a0 --- .../pull-request-dashboard/RATIONALE.md | 52 ++++- .../pull-request-dashboard/copilot_review.py | 47 +--- .../pull-request-dashboard/dashboard.py | 126 ++++++++++- .../pull-request-dashboard/delivery.py | 37 ++++ .../pr_status_comment.py | 8 + .../route_presentation.py | 12 ++ .../scripts/pull-request-dashboard/state.py | 13 +- .../test_copilot_review.py | 110 +--------- .../pull-request-dashboard/test_dashboard.py | 203 ++++++++++++++++++ .../pull-request-dashboard/test_delivery.py | 59 +++++ .../test_pr_status_comment.py | 21 ++ pull-request-dashboard/README.md | 2 +- 12 files changed, 517 insertions(+), 173 deletions(-) diff --git a/.github/scripts/pull-request-dashboard/RATIONALE.md b/.github/scripts/pull-request-dashboard/RATIONALE.md index 87b3c06f6a0..a02b7ac7cbe 100644 --- a/.github/scripts/pull-request-dashboard/RATIONALE.md +++ b/.github/scripts/pull-request-dashboard/RATIONALE.md @@ -156,6 +156,20 @@ 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. +- A PR whose stored facts show it waiting on something — a held route, a hold + that expired, a Copilot review request still to send — is refreshed first, + before the rotation spends the rest of the budget. The rotation exists because + webhooks handle everything that has just changed, but a wait ends with nothing + changing on the PR at all: a check completes, a review is filed, and if that + event is missed nothing else will bring the dashboard back. On a repository + with more open PRs than one pass can hold, the rotation alone leaves such a PR + waiting for hours, which is how one sat with every check green and no review + requested from one evening to the next. +- The waiting PRs take at most half a pass, so a repository where many are + waiting cannot stop the rotation from reaching the rest. They are taken in + rotation order, which spreads the ones that do not fit across later passes + instead of cutting off the same tail every time, and leaves the cursor on a + rotation PR so the next pass carries on from there. - 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 @@ -297,6 +311,35 @@ the implementation understandable and operationally cheap. 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. +- 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 @@ -363,12 +406,9 @@ the implementation understandable and operationally cheap. 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 the entry counts how many have gone missing on the current head. Three in - a row fail the run, which opens the hourly failure issue and closes it again - once a request lands. Failing on the first miss would report GitHub's ordinary - lag as a breakage; never failing would repeat that nineteen-hour wait with - nobody watching. +- 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 d38f51f21fe..d81446ac073 100644 --- a/.github/scripts/pull-request-dashboard/copilot_review.py +++ b/.github/scripts/pull-request-dashboard/copilot_review.py @@ -42,13 +42,6 @@ REQUEST_CONFIRMATION_ATTEMPTS = 3 -# How many requests GitHub may drop before the run fails. GitHub has answered -# the mutation with success and recorded nothing for the same pull request -# every hour for a day. The pull request stays held on a review nobody is going -# to run, and only a person can unstick it, so the failure has to reach one. -UNCONFIRMED_REQUEST_LIMIT = 3 - - def is_copilot_reviewer(obj: dict[str, Any] | None) -> bool: return is_copilot_reviewer_login(actor_login(obj)) @@ -182,21 +175,11 @@ def record_copilot_review_observation( ): requests.pop(key, None) else: - # The count of requests GitHub has dropped belongs to the head they - # were sent for, so it survives a fresh observation of that same head - # and starts over on a new one. - previous = requests.get(key) or {} - unconfirmed = ( - int(previous.get("unconfirmed_request_count") or 0) - if previous.get("head_sha") == head_sha - else 0 - ) requests[key] = { "head_sha": head_sha, "observed_at": format_ts(observed_at), "requested_at": "", "routing_input_fingerprint": routing_fingerprint, - "unconfirmed_request_count": unconfirmed, } save_copilot_review_requests(requests) @@ -311,11 +294,7 @@ def deliver_copilot_review_requests( is_copilot_reviewer(request) for request in (raw.get("review_requests") or []) ): - requests[key] = { - **entry, - "requested_at": format_ts(now), - "unconfirmed_request_count": 0, - } + requests[key] = {**entry, "requested_at": format_ts(now)} continue reviews = fetch_pr_reviews(owner, repo_name, pr_number) or [] review_exists, review_stale, _review_findings = copilot_review_status( @@ -350,25 +329,15 @@ def deliver_copilot_review_requests( errors.append(f"PR #{pr_number}: {e}") continue if landed: - requests[key] = { - **entry, - "requested_at": format_ts(now), - "unconfirmed_request_count": 0, - } + requests[key] = {**entry, "requested_at": format_ts(now)} continue - # Leaving the request undelivered keeps the next pass trying, which is - # what recovered the one pull request this was seen on. The count is - # what turns an hour of bad luck into a failure someone acts on. - unconfirmed = int(entry.get("unconfirmed_request_count") or 0) + 1 - requests[key] = {**entry, "unconfirmed_request_count": unconfirmed} - message = ( + # 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}; " - f"{unconfirmed} in a row have gone missing" + f"PR #{pr_number} on head {current_head}", + file=sys.stderr, ) - if unconfirmed >= UNCONFIRMED_REQUEST_LIMIT: - errors.append(f"PR #{pr_number}: {message}") - else: - print(message, 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..b9b9e119d40 100644 --- a/.github/scripts/pull-request-dashboard/dashboard.py +++ b/.github/scripts/pull-request-dashboard/dashboard.py @@ -172,6 +172,15 @@ 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 the gates + clear or the author pushes. + route_hold_expired bool The gates have been + outstanding past + 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 @@ -222,7 +231,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 @@ -1222,6 +1231,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 @@ -1385,12 +1400,54 @@ 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, + *, + gates_outstanding: 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 an outstanding gate. Carrying it that way is what lets the hold give + # up without the stall looking resolved a moment later. 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 ( + gates_outstanding + and route in REVIEWER_ROUTES + and (carried or 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 @@ -1403,13 +1460,26 @@ def hold_route_until_gates_settle( 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"] + ) + would_hold = route_progress(route) > route_progress(previous_route) + set_gate_hold_clock( + facts, + previous_result, + route, + gates_outstanding=gates_outstanding, + 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 +1540,7 @@ def resolve_pr_route( route, previous_result, require_clean_copilot_review=copilot_review_gate_enabled, + now=now, ) @@ -1889,6 +1960,32 @@ class BackfillSelection: cached_pr_numbers_to_remove: set[int] +# How much of a pass the unfinished waits may take. They go first because the +# rotation alone can leave one waiting for hours, but a repository where every +# pull request is waiting must not stop the rotation from reaching the rest. +BACKFILL_PRIORITY_SHARE = 0.5 + + +def backfill_priority_pr_numbers(dashboard_state: dict[str, Any]) -> set[int]: + # A pull request whose stored facts show it waiting on something is the one + # the dashboard is most likely to be wrong about: the event that ends the + # wait may never arrive, and until someone looks again nothing moves. + numbers: set[int] = set() + for key, result in (dashboard_state.get("prs") or {}).items(): + facts = (result or {}).get("facts") or {} + if not ( + facts.get("route_held_for_gates") + or facts.get("route_hold_expired") + or facts.get("copilot_review_request_needed") + ): + continue + try: + numbers.add(int(key)) + except ValueError: + continue + return numbers + + def select_backfill_prs( prs: list[dict[str, Any]], dashboard_state: dict[str, Any], @@ -1900,7 +1997,20 @@ def select_backfill_prs( open_number_set = set(open_numbers) cached_numbers = dashboard_state_pr_numbers(dashboard_state) cached_pr_numbers_to_remove = cached_numbers - open_number_set - selected_numbers = round_robin_numbers(open_numbers, backfill_cursor_pr_number(backfill_state))[:max_prs] + rotation = round_robin_numbers( + open_numbers, backfill_cursor_pr_number(backfill_state) + ) + priority_budget = int(max_prs * BACKFILL_PRIORITY_SHARE) + priority_numbers = backfill_priority_pr_numbers(dashboard_state) & open_number_set + # Taking them in rotation order spreads the ones that do not fit across + # later passes instead of always cutting off the same tail. The rotation + # itself follows, so the cursor lands on a rotation pull request and the + # next pass carries on from there. + priority = [number for number in rotation if number in priority_numbers][ + :priority_budget + ] + remaining = [number for number in rotation if number not in set(priority)] + selected_numbers = (priority + remaining)[:max_prs] return BackfillSelection( [open_prs_by_number[number] for number in selected_numbers], cached_pr_numbers_to_remove, diff --git a/.github/scripts/pull-request-dashboard/delivery.py b/.github/scripts/pull-request-dashboard/delivery.py index ed5d5c6620f..3ae03ce8dea 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 outstanding_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,31 @@ 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 = outstanding_gate_phrase(facts) or "a gate" + 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 +153,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/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..8f0c8c2d7a3 100644 --- a/.github/scripts/pull-request-dashboard/route_presentation.py +++ b/.github/scripts/pull-request-dashboard/route_presentation.py @@ -64,3 +64,15 @@ 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 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 = outstanding_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 9ef405c2306..5c8a1d67b49 100644 --- a/.github/scripts/pull-request-dashboard/state.py +++ b/.github/scripts/pull-request-dashboard/state.py @@ -413,19 +413,12 @@ def union_merge_copilot_review_requests( merged = dict(baseline_requests) for key, retry_entry in retry_snapshot_requests.items(): baseline_entry = merged.get(key) or {} - retry_entry = retry_entry or {} - if not ( - retry_entry.get("head_sha") == baseline_entry.get("head_sha") + if ( + (retry_entry or {}).get("requested_at") + and retry_entry.get("head_sha") == baseline_entry.get("head_sha") and retry_entry.get("observed_at") and retry_entry.get("observed_at") == baseline_entry.get("observed_at") ): - continue - # A delivered request has to survive so the next attempt does not send - # it again, and so does a count of requests GitHub dropped, because - # losing it would restart the wait for a request that never lands. - if retry_entry.get("requested_at") or int( - retry_entry.get("unconfirmed_request_count") or 0 - ) > int(baseline_entry.get("unconfirmed_request_count") or 0): merged[key] = retry_entry return merged diff --git a/.github/scripts/pull-request-dashboard/test_copilot_review.py b/.github/scripts/pull-request-dashboard/test_copilot_review.py index 8120cef6677..e33fc20730b 100644 --- a/.github/scripts/pull-request-dashboard/test_copilot_review.py +++ b/.github/scripts/pull-request-dashboard/test_copilot_review.py @@ -9,7 +9,6 @@ from copilot_review import ( REQUEST_CONFIRMATION_ATTEMPTS, - UNCONFIRMED_REQUEST_LIMIT, copilot_first_review_overdue, deliver_copilot_review_requests, record_copilot_review_observation, @@ -214,7 +213,6 @@ def test_records_request_for_current_head(self, _load_requests, save_requests) - "observed_at": "2026-07-20T02:00:00+00:00", "requested_at": "", "routing_input_fingerprint": "accepted-fingerprint", - "unconfirmed_request_count": 0, }, }) @@ -225,7 +223,6 @@ def test_records_request_for_current_head(self, _load_requests, save_requests) - "7": { "head_sha": "old-head", "requested_at": "old-request", - "unconfirmed_request_count": 2, } }, ) @@ -249,7 +246,6 @@ def test_new_head_replaces_previous_request(self, _load_requests, save_requests) "observed_at": "2026-07-20T02:00:00+00:00", "requested_at": "", "routing_input_fingerprint": "accepted-fingerprint", - "unconfirmed_request_count": 0, }, }) @@ -287,7 +283,6 @@ def test_same_head_request_needed_resets_acknowledgement( "observed_at": "2026-07-20T02:00:00+00:00", "requested_at": "", "routing_input_fingerprint": "accepted-fingerprint", - "unconfirmed_request_count": 0, }, }) @@ -298,43 +293,9 @@ def test_same_head_request_needed_resets_acknowledgement( "7": { "head_sha": "current-head", "requested_at": "", - "unconfirmed_request_count": 2, } }, ) - def test_same_head_keeps_count_of_dropped_requests( - self, - _load_requests, - save_requests, - ) -> None: - record_copilot_review_observation( - 7, - { - "route": "approver", - "facts": { - "head_sha": "current-head", - "copilot_review_request_needed": True, - "routing_input_fingerprint": "accepted-fingerprint", - }, - }, - NOW, - ) - - save_requests.assert_called_once_with({ - "7": { - "head_sha": "current-head", - "observed_at": "2026-07-20T02:00:00+00:00", - "requested_at": "", - "routing_input_fingerprint": "accepted-fingerprint", - "unconfirmed_request_count": 2, - }, - }) - - @patch("copilot_review.save_copilot_review_requests") - @patch( - "copilot_review.load_copilot_review_requests", - 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( 7, @@ -436,7 +397,6 @@ def test_delivers_request_for_current_stale_review( "observed_at": "2026-07-20T01:00:00+00:00", "requested_at": "2026-07-20T02:00:00+00:00", "routing_input_fingerprint": "accepted-fingerprint", - "unconfirmed_request_count": 0, }, }) @@ -501,7 +461,6 @@ def test_pending_request_is_acknowledged_from_pull_response( "observed_at": "2026-07-20T01:00:00+00:00", "requested_at": "2026-07-20T02:00:00+00:00", "routing_input_fingerprint": "accepted-fingerprint", - "unconfirmed_request_count": 0, }, }) @@ -686,7 +645,6 @@ def test_delivers_request_for_missing_first_review( "observed_at": "2026-07-20T01:00:00+00:00", "requested_at": "2026-07-20T02:00:00+00:00", "routing_input_fingerprint": "accepted-fingerprint", - "unconfirmed_request_count": 0, }, }) @@ -749,79 +707,14 @@ def test_dropped_request_is_not_recorded_as_delivered( "observed_at": "2026-07-20T01:00:00+00:00", "requested_at": "", "routing_input_fingerprint": "accepted-fingerprint", - "unconfirmed_request_count": 1, }, }) self.assertIn( "GitHub did not record the Copilot review request for PR #7 on " - "head current-head; 1 in a row have gone missing", + "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", 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", - "unconfirmed_request_count": UNCONFIRMED_REQUEST_LIMIT - 1, - } - }, - ) - def test_repeatedly_dropped_request_fails_the_run( - 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( - [ - f"PR #7: GitHub did not record the Copilot review request for " - f"PR #7 on head current-head; {UNCONFIRMED_REQUEST_LIMIT} in a " - f"row have gone missing" - ], - errors, - ) - 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", - "unconfirmed_request_count": UNCONFIRMED_REQUEST_LIMIT, - }, - }) - @patch("copilot_review.sleep_for_retry") @patch("copilot_review.fetch_review_requests", return_value=[]) @patch( @@ -887,7 +780,6 @@ def test_review_that_arrives_before_the_read_counts_as_delivered( "observed_at": "2026-07-20T01:00:00+00:00", "requested_at": "2026-07-20T02:00:00+00:00", "routing_input_fingerprint": "accepted-fingerprint", - "unconfirmed_request_count": 0, }, }) diff --git a/.github/scripts/pull-request-dashboard/test_dashboard.py b/.github/scripts/pull-request-dashboard/test_dashboard.py index f61329b6072..5b9b41a76bd 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, @@ -27,6 +29,7 @@ remove_cached_dashboard_prs, resolve_pr_route, route_pr, + select_backfill_prs, set_backfill_pr_failed, update_dashboard_for_backfill, write_initial_backfill_output, @@ -357,18 +360,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 +443,146 @@ 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_stops_the_clock(self) -> None: + 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) + + self._hold( + facts, + "author", + { + "route": "approver", + "facts": { + "head_sha": "abc", + "route_held_since": held_since.isoformat(), + }, + }, + now=self.START, + ) + + self.assertNotIn("route_held_since", facts) + self.assertFalse(facts["route_hold_expired"]) + def test_held_route_carries_the_previous_wait_forward(self) -> None: facts = { "route_held_for_gates": True, @@ -1360,6 +1507,62 @@ def test_head_sha_prefers_pr_head_ref_oid_over_truncated_commits(self) -> None: self.assertFalse(facts["copilot_review_needed"]) +class BackfillSelectionTest(unittest.TestCase): + def _prs(self, count: int) -> list[dict[str, object]]: + return [{"number": number} for number in range(1, count + 1)] + + def _numbers( + self, + selection_state: dict[str, object], + cursor: int | None = None, + max_prs: int = 4, + count: int = 10, + ) -> list[int]: + backfill_state = {"cursor": {"last_pr_number": cursor}} if cursor else {} + selection = select_backfill_prs( + self._prs(count), + selection_state, + backfill_state, + max_prs, + ) + return [pr["number"] for pr in selection.selected_prs] + + def _held(self, *numbers: int) -> dict[str, object]: + return { + "prs": { + str(number): {"facts": {"route_held_for_gates": True}} + for number in numbers + } + } + + def test_rotation_alone_when_nothing_is_waiting(self) -> None: + self.assertEqual([5, 6, 7, 8], self._numbers({"prs": {}}, cursor=4)) + + def test_a_waiting_pr_is_refreshed_before_its_turn(self) -> None: + # The one the rotation would not have reached for several passes. + self.assertEqual([2, 5, 6, 7], self._numbers(self._held(2), cursor=4)) + + def test_waiting_prs_cannot_take_the_whole_pass(self) -> None: + numbers = self._numbers(self._held(1, 2, 3, 4, 9, 10), cursor=4) + + self.assertEqual([9, 10, 5, 6], numbers) + + def test_a_needed_copilot_review_request_also_earns_a_refresh(self) -> None: + state = {"prs": {"2": {"facts": {"copilot_review_request_needed": True}}}} + + self.assertEqual([2, 5, 6, 7], self._numbers(state, cursor=4)) + + def test_an_expired_hold_keeps_earning_a_refresh(self) -> None: + state = {"prs": {"2": {"facts": {"route_hold_expired": True}}}} + + self.assertEqual([2, 5, 6, 7], self._numbers(state, cursor=4)) + + def test_a_closed_waiting_pr_is_not_selected(self) -> None: + numbers = self._numbers(self._held(2, 99), cursor=4) + + self.assertEqual([2, 5, 6, 7], numbers) + + class InitialBackfillCompletionTest(unittest.TestCase): def test_marks_complete_only_after_all_open_prs_are_cached(self) -> None: state = {"initial_backfill_complete": False, "prs": {"1": {}}} diff --git a/.github/scripts/pull-request-dashboard/test_delivery.py b/.github/scripts/pull-request-dashboard/test_delivery.py index acf0122c62d..548508af87c 100644 --- a/.github/scripts/pull-request-dashboard/test_delivery.py +++ b/.github/scripts/pull-request-dashboard/test_delivery.py @@ -164,6 +164,65 @@ 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, + "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_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..eafb6fc043c 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,27 @@ 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, + }, + }, + ) + + 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/pull-request-dashboard/README.md b/pull-request-dashboard/README.md index db0987e13ac..9fcc8728377 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. A request counts as delivered only once GitHub confirms Copilot is a pending reviewer, so one that GitHub accepts but does not record is sent again on the next pass. 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, so one that GitHub accepts but does not record 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. | From c91224827beb360ea75bedcbf3d24092fd9ac78e Mon Sep 17 00:00:00 2001 From: Trask Stalnaker Date: Sun, 16 Aug 2026 14:52:51 -0700 Subject: [PATCH 03/11] Start a reviewer's wait when the gates hand the pull request over A reviewer's wait age is dated from the last author activity. That was the same moment as the handoff before the gates existed: the author pushed and the pull request went straight to reviewers. Now the gates sit between the two, so a pull request whose checks take an hour reaches reviewers already an hour old, and one released after a gate stalls arrives four hours old. Either way the age charges reviewers for a wait they could not have answered, and sorts the pull request above ones they really have been sitting on. A handoff the gates held now starts its wait when the gates release it. Only the handoff from the author restarts the wait. A held pull request 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. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: e669e830-2655-44d1-b960-7d3d63a4e2a0 --- .../pull-request-dashboard/RATIONALE.md | 12 ++++ .../pull-request-dashboard/dashboard.py | 19 ++++- .../pull-request-dashboard/test_dashboard.py | 69 +++++++++++++++++++ 3 files changed, 99 insertions(+), 1 deletion(-) diff --git a/.github/scripts/pull-request-dashboard/RATIONALE.md b/.github/scripts/pull-request-dashboard/RATIONALE.md index a02b7ac7cbe..3c943d2d2fa 100644 --- a/.github/scripts/pull-request-dashboard/RATIONALE.md +++ b/.github/scripts/pull-request-dashboard/RATIONALE.md @@ -307,6 +307,18 @@ 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 diff --git a/.github/scripts/pull-request-dashboard/dashboard.py b/.github/scripts/pull-request-dashboard/dashboard.py index b9b9e119d40..2a2478a7e6e 100644 --- a/.github/scripts/pull-request-dashboard/dashboard.py +++ b/.github/scripts/pull-request-dashboard/dashboard.py @@ -185,7 +185,9 @@ 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 @@ -1260,6 +1262,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 @@ -1268,6 +1271,20 @@ 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 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 "" diff --git a/.github/scripts/pull-request-dashboard/test_dashboard.py b/.github/scripts/pull-request-dashboard/test_dashboard.py index 5b9b41a76bd..474811f1467 100644 --- a/.github/scripts/pull-request-dashboard/test_dashboard.py +++ b/.github/scripts/pull-request-dashboard/test_dashboard.py @@ -626,6 +626,75 @@ 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_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: From ef49605f63e652710d3360bb5bc37581d3cc8ffa Mon Sep 17 00:00:00 2001 From: Trask Stalnaker Date: Sun, 16 Aug 2026 15:17:02 -0700 Subject: [PATCH 04/11] Leave the backfill rotation alone Refreshing pull requests with an unfinished wait ahead of the rotation was meant to shorten how long a missed check completion or review goes unnoticed. It does not buy enough to keep. A full sweep takes two hourly passes on most repositories and four on the largest, which is no longer than the four-hour gate hold that already bounds the wait, so the reordering saved about an hour on one repository and nothing on the rest. The cost was a second selection rule naming three facts, which anyone adding a gate later would have had to remember to extend. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: e669e830-2655-44d1-b960-7d3d63a4e2a0 --- .../pull-request-dashboard/RATIONALE.md | 21 +++---- .../pull-request-dashboard/dashboard.py | 41 +------------ .../pull-request-dashboard/test_dashboard.py | 57 ------------------- 3 files changed, 8 insertions(+), 111 deletions(-) diff --git a/.github/scripts/pull-request-dashboard/RATIONALE.md b/.github/scripts/pull-request-dashboard/RATIONALE.md index 3c943d2d2fa..b5b45e015fb 100644 --- a/.github/scripts/pull-request-dashboard/RATIONALE.md +++ b/.github/scripts/pull-request-dashboard/RATIONALE.md @@ -156,20 +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. -- A PR whose stored facts show it waiting on something — a held route, a hold - that expired, a Copilot review request still to send — is refreshed first, - before the rotation spends the rest of the budget. The rotation exists because - webhooks handle everything that has just changed, but a wait ends with nothing - changing on the PR at all: a check completes, a review is filed, and if that - event is missed nothing else will bring the dashboard back. On a repository - with more open PRs than one pass can hold, the rotation alone leaves such a PR - waiting for hours, which is how one sat with every check green and no review - requested from one evening to the next. -- The waiting PRs take at most half a pass, so a repository where many are - waiting cannot stop the rotation from reaching the rest. They are taken in - rotation order, which spreads the ones that do not fit across later passes - instead of cutting off the same tail every time, and leaves the cursor on a - rotation PR so the next pass carries on from there. +- 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 diff --git a/.github/scripts/pull-request-dashboard/dashboard.py b/.github/scripts/pull-request-dashboard/dashboard.py index 2a2478a7e6e..7d0ae638fa8 100644 --- a/.github/scripts/pull-request-dashboard/dashboard.py +++ b/.github/scripts/pull-request-dashboard/dashboard.py @@ -1977,32 +1977,6 @@ class BackfillSelection: cached_pr_numbers_to_remove: set[int] -# How much of a pass the unfinished waits may take. They go first because the -# rotation alone can leave one waiting for hours, but a repository where every -# pull request is waiting must not stop the rotation from reaching the rest. -BACKFILL_PRIORITY_SHARE = 0.5 - - -def backfill_priority_pr_numbers(dashboard_state: dict[str, Any]) -> set[int]: - # A pull request whose stored facts show it waiting on something is the one - # the dashboard is most likely to be wrong about: the event that ends the - # wait may never arrive, and until someone looks again nothing moves. - numbers: set[int] = set() - for key, result in (dashboard_state.get("prs") or {}).items(): - facts = (result or {}).get("facts") or {} - if not ( - facts.get("route_held_for_gates") - or facts.get("route_hold_expired") - or facts.get("copilot_review_request_needed") - ): - continue - try: - numbers.add(int(key)) - except ValueError: - continue - return numbers - - def select_backfill_prs( prs: list[dict[str, Any]], dashboard_state: dict[str, Any], @@ -2014,20 +1988,7 @@ def select_backfill_prs( open_number_set = set(open_numbers) cached_numbers = dashboard_state_pr_numbers(dashboard_state) cached_pr_numbers_to_remove = cached_numbers - open_number_set - rotation = round_robin_numbers( - open_numbers, backfill_cursor_pr_number(backfill_state) - ) - priority_budget = int(max_prs * BACKFILL_PRIORITY_SHARE) - priority_numbers = backfill_priority_pr_numbers(dashboard_state) & open_number_set - # Taking them in rotation order spreads the ones that do not fit across - # later passes instead of always cutting off the same tail. The rotation - # itself follows, so the cursor lands on a rotation pull request and the - # next pass carries on from there. - priority = [number for number in rotation if number in priority_numbers][ - :priority_budget - ] - remaining = [number for number in rotation if number not in set(priority)] - selected_numbers = (priority + remaining)[:max_prs] + selected_numbers = round_robin_numbers(open_numbers, backfill_cursor_pr_number(backfill_state))[:max_prs] return BackfillSelection( [open_prs_by_number[number] for number in selected_numbers], cached_pr_numbers_to_remove, diff --git a/.github/scripts/pull-request-dashboard/test_dashboard.py b/.github/scripts/pull-request-dashboard/test_dashboard.py index 474811f1467..27ef8f0dc8e 100644 --- a/.github/scripts/pull-request-dashboard/test_dashboard.py +++ b/.github/scripts/pull-request-dashboard/test_dashboard.py @@ -29,7 +29,6 @@ remove_cached_dashboard_prs, resolve_pr_route, route_pr, - select_backfill_prs, set_backfill_pr_failed, update_dashboard_for_backfill, write_initial_backfill_output, @@ -1576,62 +1575,6 @@ def test_head_sha_prefers_pr_head_ref_oid_over_truncated_commits(self) -> None: self.assertFalse(facts["copilot_review_needed"]) -class BackfillSelectionTest(unittest.TestCase): - def _prs(self, count: int) -> list[dict[str, object]]: - return [{"number": number} for number in range(1, count + 1)] - - def _numbers( - self, - selection_state: dict[str, object], - cursor: int | None = None, - max_prs: int = 4, - count: int = 10, - ) -> list[int]: - backfill_state = {"cursor": {"last_pr_number": cursor}} if cursor else {} - selection = select_backfill_prs( - self._prs(count), - selection_state, - backfill_state, - max_prs, - ) - return [pr["number"] for pr in selection.selected_prs] - - def _held(self, *numbers: int) -> dict[str, object]: - return { - "prs": { - str(number): {"facts": {"route_held_for_gates": True}} - for number in numbers - } - } - - def test_rotation_alone_when_nothing_is_waiting(self) -> None: - self.assertEqual([5, 6, 7, 8], self._numbers({"prs": {}}, cursor=4)) - - def test_a_waiting_pr_is_refreshed_before_its_turn(self) -> None: - # The one the rotation would not have reached for several passes. - self.assertEqual([2, 5, 6, 7], self._numbers(self._held(2), cursor=4)) - - def test_waiting_prs_cannot_take_the_whole_pass(self) -> None: - numbers = self._numbers(self._held(1, 2, 3, 4, 9, 10), cursor=4) - - self.assertEqual([9, 10, 5, 6], numbers) - - def test_a_needed_copilot_review_request_also_earns_a_refresh(self) -> None: - state = {"prs": {"2": {"facts": {"copilot_review_request_needed": True}}}} - - self.assertEqual([2, 5, 6, 7], self._numbers(state, cursor=4)) - - def test_an_expired_hold_keeps_earning_a_refresh(self) -> None: - state = {"prs": {"2": {"facts": {"route_hold_expired": True}}}} - - self.assertEqual([2, 5, 6, 7], self._numbers(state, cursor=4)) - - def test_a_closed_waiting_pr_is_not_selected(self) -> None: - numbers = self._numbers(self._held(2, 99), cursor=4) - - self.assertEqual([2, 5, 6, 7], numbers) - - class InitialBackfillCompletionTest(unittest.TestCase): def test_marks_complete_only_after_all_open_prs_are_cached(self) -> None: state = {"initial_backfill_complete": False, "prs": {"1": {}}} From bc582ccfb4f677aa217bb98ccde45784136282d3 Mon Sep 17 00:00:00 2001 From: Trask Stalnaker Date: Sun, 16 Aug 2026 15:30:15 -0700 Subject: [PATCH 05/11] Address Copilot review comments: bump persisted state versions Copilot comment: This adds stored state and changes delivery semantics, but `COPILOT_REVIEW_REQUEST_STATE_VERSION` remains `4`. The compatibility contract at `state.py:20-24` requires that version to increase for either change; otherwise `claim_delivery_versions()` treats older in-flight workers as compatible, allowing one to apply the old mutation-success behavior and erase/bypass this counter. Bump the Copilot request state version and its version assertion. Copilot comment: `requested_at` now means that GitHub confirmed the request rather than merely accepted the mutation, so this is a persisted semantic and delivery-behavior change. Per the versioning contract in `state.py:20-34`, `COPILOT_REVIEW_REQUEST_STATE_VERSION` must be incremented; otherwise an older concurrent delivery run remains compatible and can still stamp an unconfirmed request as delivered after the new behavior is active. ``` if landed: requests[key] = {**entry, "requested_at": format_ts(now)} ``` Copilot comment: These new hold-clock facts change the persisted `dashboard-state.json` shape, but `DASHBOARD_STATE_VERSION` remains unchanged. The repository explicitly requires a version increment for stored-shape or delivered-behavior changes (`state.py:20-26`); without it, an older in-flight delivery worker is considered compatible with the new state and can process it using the pre-expiration behavior. Bump the dashboard state version as part of this change. ``` facts["route_held_since"] = carried or format_ts(now) ``` Analysis: Both changes alter the delivery compatibility contract. Incrementing the Copilot request version prevents an older worker from recording mutation success as confirmed delivery. Incrementing the dashboard version prevents an older worker from processing hold-clock state without the expiration behavior. The counter named in the first comment was removed later, but the request semantic change still requires the version increment. Upsides: Concurrent workers reject stale delivery behavior, and the version assertions document both new contracts. Downsides: Existing disposable dashboard and Copilot request caches are regenerated once under the new versions. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .github/scripts/pull-request-dashboard/state.py | 4 ++-- .github/scripts/pull-request-dashboard/test_state.py | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) 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_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)): From d8d9998798feac3578b4e6717d5df39e42903889 Mon Sep 17 00:00:00 2001 From: Trask Stalnaker Date: Sun, 16 Aug 2026 15:38:41 -0700 Subject: [PATCH 06/11] Address Copilot review comment: document completed review confirmation Copilot comment: This says a request is confirmed only by a pending-reviewer record, but `copilot_review_request_landed()` also treats a completed Copilot review of the current head as confirmation. Document that second confirmation path so the configuration reference matches the implemented delivery semantics. ``` | `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, so one that GitHub accepts but does not record 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). | ``` Analysis: The implementation accepts either a pending Copilot review request or a completed Copilot review of the current head. The configuration reference named only the first path, so it could mislead operators when a short review finishes before the confirmation read. Upsides: The configuration reference now matches both confirmation paths and explains when a request will be retried. Downsides: The already long configuration description grows by one short clause. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- pull-request-dashboard/README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pull-request-dashboard/README.md b/pull-request-dashboard/README.md index 9fcc8728377..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. A request counts as delivered only once GitHub confirms Copilot is a pending reviewer, so one that GitHub accepts but does not record 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). | +| `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. | From d01e1b587d4f74fb02ebe644226218d7216d50f3 Mon Sep 17 00:00:00 2001 From: Trask Stalnaker Date: Sun, 16 Aug 2026 15:46:07 -0700 Subject: [PATCH 07/11] Address Copilot review comment: cover stalled gate delivery stage Copilot comment: This new stage makes `test_runs_all_repository_deliveries_in_order` fail: that whole-repository test does not mock `report_stalled_gates()` or initialize `state._state_dir`, so `load_dashboard_state_cache()` raises and `deliver_from_state()` returns a `stalled gates: state directory has not been initialized` error instead of `[]`. Please update the existing whole-repository delivery tests to mock/record this stage (and include it in the expected order where appropriate). Analysis: The whole-repository tests mocked every earlier delivery stage but left the new stalled-gate stage connected to uninitialized state. Mocking it at the same boundary isolates the orchestration tests and lets the order test verify that stalled-gate reporting runs last. Upsides: The delivery test suite passes, the stage order is covered, and the failure-continuation test proves stalled-gate reporting still runs after an earlier stage fails. Downsides: The whole-repository tests add one mock parameter and assertion each. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../pull-request-dashboard/test_delivery.py | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/.github/scripts/pull-request-dashboard/test_delivery.py b/.github/scripts/pull-request-dashboard/test_delivery.py index 548508af87c..a0e816c3c74 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 ( From a654e4f03557125127db5f1f3e3ce1b61e0b7b6a Mon Sep 17 00:00:00 2001 From: Trask Stalnaker Date: Sun, 16 Aug 2026 19:01:53 -0700 Subject: [PATCH 08/11] Address review finding: carry the gate-release wait forward Review finding: The gate-release wait reset survives only one refresh. add_wait_age_facts sets waiting_since to now with basis gate_release when the previous accepted result was a held author route. On the next pass the previous route is a reviewer route and route_held_for_gates is false, so that branch no longer applies, the approver fallback returns last_author_activity_at, and the monotonic guard below only clamps when the new value is later than the stored one. The push is earlier than the release, so the guard does not fire and waiting_since goes back to the push time. Probe: two consecutive add_wait_age_facts calls with last_author_activity_at 2026-08-16T08:00:00+00:00 give 2026-08-16T12:00:00+00:00 with basis gate_release on the release pass, and 2026-08-16T08:00:00+00:00 with basis last_author_activity on the very next pass. That is the age this PR sets out to stop showing, so a pull request released after a four-hour stalled gate is again presented to reviewers as four hours old one refresh later. Fix: persist the release, for example by carrying the gate_release waiting_since and basis forward while the pull request stays on a reviewer route on the same head, so the fallback cannot pull the wait back to the push. Analysis: the release branch recognises exactly one transition, from a held author route to a reviewer route. Every later refresh sees a reviewer route on both sides, so it falls through to the fallback, which for a reviewer route is the author's last activity. The monotonic guard underneath only refuses to move the wait forward, and the push is earlier than the release, so it accepts the older time and the release is lost. Carrying the release the same way a held route carries its wait keeps it until something else genuinely changes whose turn it is. The carry is limited to the head it was recorded on. After a push the guard already preserves the release, because the new push is later than it. Upsides: the wait the release starts is the one reviewers actually see, instead of one that lasts a single refresh. A pull request freed from a four-hour stalled gate no longer reappears four hours old, and no longer sorts above pull requests reviewers have really been sitting on. Downsides: reviewer waits now depend on one more carried fact, so a lost or rejected state commit drops the release and falls back to the push time. A discussion pending on reviewers from before the release no longer sets the wait while the pull request stays on that head, which understates that thread's own age. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../pull-request-dashboard/dashboard.py | 15 ++++++++++ .../pull-request-dashboard/test_dashboard.py | 29 ++++++++++++++++++- 2 files changed, 43 insertions(+), 1 deletion(-) diff --git a/.github/scripts/pull-request-dashboard/dashboard.py b/.github/scripts/pull-request-dashboard/dashboard.py index 7d0ae638fa8..994581e99ee 100644 --- a/.github/scripts/pull-request-dashboard/dashboard.py +++ b/.github/scripts/pull-request-dashboard/dashboard.py @@ -1285,6 +1285,21 @@ def add_wait_age_facts( 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 "" diff --git a/.github/scripts/pull-request-dashboard/test_dashboard.py b/.github/scripts/pull-request-dashboard/test_dashboard.py index 27ef8f0dc8e..ba23d8a9298 100644 --- a/.github/scripts/pull-request-dashboard/test_dashboard.py +++ b/.github/scripts/pull-request-dashboard/test_dashboard.py @@ -650,7 +650,34 @@ def test_reviewers_start_waiting_when_the_gates_release_the_pr(self) -> None: 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: + 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"]) + + facts = { "route_held_for_gates": False, "last_author_activity_at": "2026-08-16T08:00:00+00:00", From 02ba98fc30d51146450a1b76c0b0c7358a277f52 Mon Sep 17 00:00:00 2001 From: Trask Stalnaker Date: Sun, 16 Aug 2026 19:03:51 -0700 Subject: [PATCH 09/11] Address review finding: keep an expired gate hold across an author round trip Review finding: An expired hold restarts for another four hours when the same head goes back to the author and returns. set_gate_hold_clock drops route_held_since whenever the pre-hold route is author, without regard to the head, so a reviewer comment that puts the pull request back on its author clears the expiry. When the author replies on that same head, would_hold is true again and the clock starts from now, so a gate the dashboard already proved absent on that head holds the pull request for four more hours, and report_stalled_gates goes quiet in between because route_hold_expired is false. Probe on head abc: the expiry pass routes to approver with route_hold_expired true, the author pass clears route_held_since, and the reply pass twenty minutes later returns route author with route_held_since set to that moment. The RATIONALE this PR adds says the clock runs on its own for as long as the same head still has an outstanding gate, and that a push clears it, so the code and the stated rule disagree. Fix: keep the expiry for the head, for example by carrying route_held_since across an author-route pass while the head is unchanged, so a bounce to the author cannot buy a missing gate another four hours. Analysis: two different things were tied to the same condition. Starting the clock has to wait for a real handoff, or a slow check suite on a pull request that was already with its reviewers would look like a stalled one. Keeping a clock that is already running does not, because the gate it is waiting for is missing from the same code whichever route the pull request is on. Requiring a reviewer route for both meant a comment that needs an author reply erased the evidence, and the reply then looked like a first handoff. Splitting the condition keeps the new-clock rule as it was and carries an existing clock for as long as the head and the outstanding gate last. Holding is unaffected, because it still requires the route to advance, and the author route never does. Upsides: a gate the dashboard has already given up on cannot hold the pull request again on the same code, however often it goes back and forth with its author. The stall stays reported for as long as it lasts, instead of going silent every time the author owes a reply, which is when a missing gate is easiest to miss. Downsides: a stall is now reported while the pull request sits with its author, so a run fails over a pull request nobody is waiting on. The report also outlives the handoff it started with, so a required check that is merely slow keeps being named until the author pushes. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../pull-request-dashboard/dashboard.py | 15 ++++---- .../pull-request-dashboard/test_dashboard.py | 35 ++++++++++++++++--- 2 files changed, 40 insertions(+), 10 deletions(-) diff --git a/.github/scripts/pull-request-dashboard/dashboard.py b/.github/scripts/pull-request-dashboard/dashboard.py index 994581e99ee..59a948413af 100644 --- a/.github/scripts/pull-request-dashboard/dashboard.py +++ b/.github/scripts/pull-request-dashboard/dashboard.py @@ -1452,10 +1452,14 @@ def set_gate_hold_clock( # 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 an outstanding gate. Carrying it that way is what lets the hold give - # up without the stall looking resolved a moment later. 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. + # 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 = ( @@ -1465,8 +1469,7 @@ def set_gate_hold_clock( ) if not ( gates_outstanding - and route in REVIEWER_ROUTES - and (carried or would_hold) + and (carried or (route in REVIEWER_ROUTES and would_hold)) ): facts.pop("route_held_since", None) return diff --git a/.github/scripts/pull-request-dashboard/test_dashboard.py b/.github/scripts/pull-request-dashboard/test_dashboard.py index ba23d8a9298..7a507ca1934 100644 --- a/.github/scripts/pull-request-dashboard/test_dashboard.py +++ b/.github/scripts/pull-request-dashboard/test_dashboard.py @@ -558,7 +558,9 @@ def test_checks_that_never_held_the_pr_do_not_start_the_clock(self) -> None: self.assertNotIn("route_held_since", facts) self.assertFalse(facts["route_hold_expired"]) - def test_a_pr_sent_back_to_its_author_stops_the_clock(self) -> None: + 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, @@ -566,7 +568,7 @@ def test_a_pr_sent_back_to_its_author_stops_the_clock(self) -> None: } held_since = self.START - GATE_HOLD_LIMIT - timedelta(hours=1) - self._hold( + route = self._hold( facts, "author", { @@ -579,8 +581,33 @@ def test_a_pr_sent_back_to_its_author_stops_the_clock(self) -> None: now=self.START, ) - self.assertNotIn("route_held_since", facts) - self.assertFalse(facts["route_hold_expired"]) + 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_held_route_carries_the_previous_wait_forward(self) -> None: facts = { From e79638fc82e914a5b890f21abcc9314afc1c862e Mon Sep 17 00:00:00 2001 From: Trask Stalnaker Date: Sun, 16 Aug 2026 20:21:17 -0700 Subject: [PATCH 10/11] Address review finding: restore the test the gate-release commit overwrote Review finding: Commit a654e4f0355 deleted the def line of an existing test instead of adding a new one above it. The diff of that commit replaces `- def test_an_unheld_handoff_still_dates_from_the_push(self) -> None:` with `+ def test_the_gate_release_wait_survives_the_next_refresh(self) -> None:` plus its whole body, so the old test's body (lines 708-725: the unheld-handoff scenario asserting waiting_since falls back to last_author_activity) is now an orphaned continuation of the new test, separated only by a stray double blank line. Two effects: the named regression test test_an_unheld_handoff_still_dates_from_the_push no longer exists, so the suite silently lost a test and the failure it reports no longer names the scenario that broke; and the unheld-handoff assertions now run only when the carry-forward assertions above them pass, so one regression masks the other. The method name also no longer describes what the method checks. Fix: restore `def test_an_unheld_handoff_still_dates_from_the_push(self) -> None:` with its original comment immediately before the orphaned `facts = {` at line 708, and drop the extra blank line, so the two scenarios are two independent tests again. Analysis: the new test was written over the def line of the old one, so its body ran on as a second scenario inside the new method. Python accepts that, which is why nothing failed and the loss was silent. The two scenarios check opposite things: one that a gate release is carried across the next refresh, the other that a handoff the gates never held still dates from the author's push. Giving the second its def line back separates them, so each names itself when it fails and neither hides the other. Upsides: the suite has both tests again rather than one, and it now runs 96 tests where it ran 95. A regression in the unheld-handoff fallback reports its own name instead of a carry-forward failure, and it is no longer masked when the carry-forward assertions fail first. Downsides: No material downside identified. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .github/scripts/pull-request-dashboard/test_dashboard.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/scripts/pull-request-dashboard/test_dashboard.py b/.github/scripts/pull-request-dashboard/test_dashboard.py index 7a507ca1934..ffc2ca37c61 100644 --- a/.github/scripts/pull-request-dashboard/test_dashboard.py +++ b/.github/scripts/pull-request-dashboard/test_dashboard.py @@ -704,7 +704,7 @@ def test_the_gate_release_wait_survives_the_next_refresh(self) -> None: 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", From 62f94de6ff4131363dcb8af31273b3d34b3a8a0b Mon Sep 17 00:00:00 2001 From: Trask Stalnaker Date: Sun, 16 Aug 2026 20:24:02 -0700 Subject: [PATCH 11/11] Address review finding: only a gate that reported nothing can expire the hold Review finding: report_stalled_gates reports 'the Copilot review never reported' for a pull request whose Copilot review did report. copilot_review_outstanding() (copilot_review.py:88) is true when copilot_review_needed is set, and dashboard.py:633 sets copilot_review_needed from `copilot_review_stale or copilot_review_findings`, so a Copilot review that covers the current head but left open findings keeps the gate outstanding. set_gate_hold_clock() carries route_held_since across a trip back to the author whenever gates_outstanding is true and the head is unchanged, so the clock keeps running while the author works on those findings. Walk it through: pass one has checks pending and no Copilot review, so the PR is held on approver and the clock starts at T. Ten minutes later Copilot posts a review of head abc with unresolved findings, the checks finish, and the PR routes to author; gates_outstanding stays true through copilot_review_needed, so the carried clock keeps running from T. At T+4h the author has not resolved the threads and has not pushed, gate_hold_expired() is true, and the hourly whole-repository delivery emits 'PR #N: the Copilot review never reported on head abc', which run_delivery_action turns into a delivery failure and opens the tracking issue. Nothing is stalled: Copilot reported, and the dashboard is already routing the PR to its author correctly. An author taking more than four hours to answer Copilot is ordinary, so this fires repeatedly on healthy pull requests, and RATIONALE.md states the report is meant for a repository misconfiguration that needs a person. Fix: stop counting a Copilot review that has reported on the current head as a gate the hold clock can expire on, for example by excluding the case where copilot_review_exists is true and the review is not stale from the gates_outstanding value that set_gate_hold_clock and gate_hold_expired use, while leaving a missing or stale review counted. Analysis: the hold and the stall clock were asking the same question, but they need different ones. The hold asks whether a gate is blocking the handoff, and open Copilot findings do block it. The clock asks whether a gate has gone missing, and findings are the opposite of missing: Copilot answered. The two only came apart because the clock is deliberately carried across a trip back to the author, so it kept running through exactly the state that findings produce. copilot_review_unreported now names the narrower question, true only when Copilot has said nothing about the current head, which means no review at all or one that covers older code. hold_route_until_gates_settle keeps the full gate set for the hold and passes the narrower set to the clock. unreported_gate_phrase reports from the same fact, so the status comment and the stall report can no longer name a gate that arrived, and report_stalled_gates now skips a pull request with no unreported gate rather than calling it "a gate". Upsides: an author who takes more than four hours over Copilot's comments no longer opens an hourly tracking issue, so the alarm keeps meaning what RATIONALE says it means. The stall report and the status comment name only a gate that produced nothing, so a reader is never sent after a review that is sitting on the pull request. A missing review, a stale one, and a required check with no check run all still expire the hold exactly as before. Downsides: a Copilot review that arrives with findings clears the carried clock, so if the author later pushes nothing and Copilot never re-reports on that head, the four hours start again from the next real hold rather than continuing. That is the same fresh start any newly reported gate gets, and the pull request is with its author throughout, so nobody is waiting on the dashboard for it. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../pull-request-dashboard/RATIONALE.md | 9 +++ .../pull-request-dashboard/copilot_review.py | 13 ++++ .../pull-request-dashboard/dashboard.py | 53 ++++++++++----- .../pull-request-dashboard/delivery.py | 6 +- .../route_presentation.py | 15 ++++- .../pull-request-dashboard/test_dashboard.py | 64 +++++++++++++++++++ .../pull-request-dashboard/test_delivery.py | 45 +++++++++++++ .../test_pr_status_comment.py | 1 + 8 files changed, 187 insertions(+), 19 deletions(-) diff --git a/.github/scripts/pull-request-dashboard/RATIONALE.md b/.github/scripts/pull-request-dashboard/RATIONALE.md index b5b45e015fb..56f7b7ae417 100644 --- a/.github/scripts/pull-request-dashboard/RATIONALE.md +++ b/.github/scripts/pull-request-dashboard/RATIONALE.md @@ -335,6 +335,15 @@ the implementation understandable and operationally cheap. 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 diff --git a/.github/scripts/pull-request-dashboard/copilot_review.py b/.github/scripts/pull-request-dashboard/copilot_review.py index d81446ac073..2deade80ae8 100644 --- a/.github/scripts/pull-request-dashboard/copilot_review.py +++ b/.github/scripts/pull-request-dashboard/copilot_review.py @@ -93,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, diff --git a/.github/scripts/pull-request-dashboard/dashboard.py b/.github/scripts/pull-request-dashboard/dashboard.py index 59a948413af..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 @@ -174,10 +183,11 @@ provisional. route_held_since str (iso) When the gates first kept this PR off its reviewers on this - head. Cleared once the gates - clear or the author pushes. - route_hold_expired bool The gates have been - outstanding past + 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. @@ -258,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, @@ -1444,22 +1455,22 @@ def set_gate_hold_clock( previous_result: dict[str, Any] | None, route: str, *, - gates_outstanding: bool, + 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 an outstanding gate. 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. + # 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 = ( @@ -1468,7 +1479,7 @@ def set_gate_hold_clock( else "" ) if not ( - gates_outstanding + unreported_gates and (carried or (route in REVIEWER_ROUTES and would_hold)) ): facts.pop("route_held_since", None) @@ -1494,16 +1505,26 @@ 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) 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, - gates_outstanding=gates_outstanding, + unreported_gates=unreported_gates, would_hold=would_hold, now=now, ) diff --git a/.github/scripts/pull-request-dashboard/delivery.py b/.github/scripts/pull-request-dashboard/delivery.py index 3ae03ce8dea..b7378d7f031 100644 --- a/.github/scripts/pull-request-dashboard/delivery.py +++ b/.github/scripts/pull-request-dashboard/delivery.py @@ -15,7 +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 outstanding_gate_phrase +from route_presentation import unreported_gate_phrase from pr_status_comment import ( update_status_comments_from_state, update_targeted_status_comment_from_state, @@ -71,7 +71,9 @@ def report_stalled_gates(open_pr_numbers: set[int]) -> list[str]: continue if number not in open_pr_numbers: continue - gates = outstanding_gate_phrase(facts) or "a gate" + 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) diff --git a/.github/scripts/pull-request-dashboard/route_presentation.py b/.github/scripts/pull-request-dashboard/route_presentation.py index 8f0c8c2d7a3..7d1ab2670b7 100644 --- a/.github/scripts/pull-request-dashboard/route_presentation.py +++ b/.github/scripts/pull-request-dashboard/route_presentation.py @@ -66,10 +66,23 @@ def outstanding_gate_phrase(facts: dict[str, Any]) -> str: 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 = outstanding_gate_phrase(facts) + gates = unreported_gate_phrase(facts) if not gates: return "" return ( diff --git a/.github/scripts/pull-request-dashboard/test_dashboard.py b/.github/scripts/pull-request-dashboard/test_dashboard.py index ffc2ca37c61..08dcbca1a91 100644 --- a/.github/scripts/pull-request-dashboard/test_dashboard.py +++ b/.github/scripts/pull-request-dashboard/test_dashboard.py @@ -609,6 +609,70 @@ def test_an_author_round_trip_does_not_restart_an_expired_hold(self) -> None: 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, diff --git a/.github/scripts/pull-request-dashboard/test_delivery.py b/.github/scripts/pull-request-dashboard/test_delivery.py index a0e816c3c74..b04eceb9d2f 100644 --- a/.github/scripts/pull-request-dashboard/test_delivery.py +++ b/.github/scripts/pull-request-dashboard/test_delivery.py @@ -185,6 +185,7 @@ def test_a_stalled_gate_is_reported_when_the_whole_repository_runs(self) -> None "facts": { "route_hold_expired": True, "copilot_review_outstanding": True, + "copilot_review_unreported": True, "required_checks_settled": True, "head_sha": "abc", } @@ -201,6 +202,50 @@ def test_a_stalled_gate_is_reported_when_the_whole_repository_runs(self) -> None 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}}}} 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 eafb6fc043c..54b7073e9e9 100644 --- a/.github/scripts/pull-request-dashboard/test_pr_status_comment.py +++ b/.github/scripts/pull-request-dashboard/test_pr_status_comment.py @@ -269,6 +269,7 @@ def test_a_pr_released_from_a_stalled_gate_says_so(self) -> None: "route_hold_expired": True, "required_checks_settled": True, "copilot_review_outstanding": True, + "copilot_review_unreported": True, }, }, )