diff --git a/.github/scripts/pull-request-dashboard/RATIONALE.md b/.github/scripts/pull-request-dashboard/RATIONALE.md index 50fc6a863c3..ac1d5a27990 100644 --- a/.github/scripts/pull-request-dashboard/RATIONALE.md +++ b/.github/scripts/pull-request-dashboard/RATIONALE.md @@ -318,12 +318,40 @@ the implementation understandable and operationally cheap. started, not from the comment count on its review. A review's comment count never shrinks, so it keeps counting feedback the author has since addressed and holds the PR on work that is already done. -- The gate's re-request path is deliberately narrow: it triggers only when the - current head has no Copilot review, because a push is the one change a - re-review can respond to. Findings on the current head sit on unchanged code, - so asking Copilot to look at it again would reach the same verdict and be - requested again on the next pass; those threads clear when the author resolves - them or pushes a fix, which is a re-request in its own right. +- The gate's re-request path covers two states. A stale review means the author + pushed, which is the one change a re-review can respond to. Findings on the + current head sit on unchanged code, so asking Copilot to look at it again + would reach the same verdict and be requested again on the next pass; those + threads clear when the author resolves them or pushes a fix, which is a + re-request in its own right. +- The other state is a first review that never arrived. The gate otherwise + relies entirely on automatic Copilot code review to produce it, so when GitHub + silently never starts one, the pull request waits on its author forever for a + review nobody has asked for, and only manual intervention recovers it. +- That wait is timed from `copilot_first_review_missing_since`, set when the + gate first observes a non-draft pull request with no Copilot review and + carried forward across passes. Becoming a draft resets it, because GitHub + starts the automatic review when a pull request becomes ready rather than when + it is opened. A push deliberately does not reset it: GitHub does not + automatically review a pull request it has never reviewed, so restarting the + wait on every push would leave an actively developed pull request waiting + forever — exactly the case the recovery exists for. +- One hour is the grace period. Observed first reviews normally land within + twenty minutes and have been seen as late as forty, so an hour clears the + normal spread without waiting through another full review cycle. Nothing + signals the expiry itself: a pull request stalled on a missing review produces + no activity, so no webhook fires and the hourly backfill is what notices. + Recording and delivery share a run, so recovery lands within roughly one to + two hours of the pull request becoming ready, against a failure that is + otherwise unbounded. +- A first-review request is reachable only where a re-request already is — the + pull request would otherwise route to reviewers or maintainers, Copilot is not + already a pending requested reviewer, and the required checks have settled — + so the recovery cannot spend a review on code CI is about to reject. +- Delivery re-validates the review state against live data and discards the + 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. - 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 d6da31b34c6..dbaea33a3e2 100644 --- a/.github/scripts/pull-request-dashboard/copilot_review.py +++ b/.github/scripts/pull-request-dashboard/copilot_review.py @@ -2,7 +2,7 @@ from __future__ import annotations -from datetime import datetime +from datetime import datetime, timedelta from pathlib import Path import sys from typing import Any @@ -21,10 +21,19 @@ actor_login, format_ts, is_copilot_reviewer_login, + parse_ts, required_checks_settled, + utc_now, ) +# How long the automatic first review is given to arrive before the dashboard +# requests one itself. Measured against observed first reviews, which normally +# land within twenty minutes and have been seen as late as forty; an hour +# clears that without waiting through the whole first review cycle again. +FIRST_REVIEW_GRACE = timedelta(hours=1) + + def is_copilot_reviewer(obj: dict[str, Any] | None) -> bool: return is_copilot_reviewer_login(actor_login(obj)) @@ -76,22 +85,64 @@ def copilot_review_outstanding(facts: dict[str, Any], *, enabled: bool) -> bool: ) +def set_copilot_first_review_missing_since( + facts: dict[str, Any], + previous_result: dict[str, Any] | None, + *, + enabled: bool, + now: datetime, +) -> None: + # How long this pull request has been waiting on a first review that GitHub + # was expected to start automatically. The clock runs only while the wait is + # real: the gate applies, the pull request is out of draft, and Copilot has + # never reviewed it. Draft resets it because GitHub starts the automatic + # review when a pull request becomes ready, not when it is opened. A push + # deliberately does not reset it, because GitHub does not automatically + # review a pull request it has never reviewed, so restarting the wait on + # every push would leave an actively developed pull request waiting forever. + previous_facts = (previous_result or {}).get("facts") or {} + if not enabled or facts.get("is_draft") or facts.get("copilot_review_exists"): + facts.pop("copilot_first_review_missing_since", None) + return + facts["copilot_first_review_missing_since"] = str( + previous_facts.get("copilot_first_review_missing_since") or format_ts(now) + ) + + +def copilot_first_review_overdue(facts: dict[str, Any], now: datetime) -> bool: + missing_since = parse_ts(facts.get("copilot_first_review_missing_since")) + if missing_since is None: + return False + return now - missing_since >= FIRST_REVIEW_GRACE + + def set_copilot_review_request_needed( facts: dict[str, Any], route: str, *, enabled: bool, + now: datetime | None = None, ) -> None: # Requesting a re-review before the checks report would spend it on code CI - # is about to reject, and a PR GitHub has never reviewed is already queued - # for the automatic first review. Only a stale review is worth re-running: - # findings on the current head are unchanged code, so a re-review cannot - # clear them and would be requested again on every pass. + # is about to reject. Only two states are worth a request. A stale review + # means the author pushed, which is the one change a re-review can respond + # to; findings on the current head sit on unchanged code, so re-reviewing + # would reach the same verdict and be requested again on every pass. A + # review GitHub should have started automatically and never did is the + # other: the gate would otherwise hold the pull request on its author + # indefinitely, waiting for a review nobody has asked for. + now = now or utc_now() + review_missing = not facts.get("copilot_review_exists") facts["copilot_review_request_needed"] = ( enabled and route in ("approver", "maintainer") - and bool(facts.get("copilot_review_exists")) - and bool(facts.get("copilot_review_stale")) + and ( + ( + bool(facts.get("copilot_review_exists")) + and bool(facts.get("copilot_review_stale")) + ) + or (review_missing and copilot_first_review_overdue(facts, now)) + ) and not facts.get("copilot_review_requested") and required_checks_settled(facts) ) @@ -217,11 +268,15 @@ def deliver_copilot_review_requests( current_head, raw.get("review_threads") or [], ) - if not review_exists or not review_stale: + # A missing review is a reason to request, not to discard: the + # request was recorded precisely because the automatic first review + # never arrived. Only a review that already covers the current head + # makes the request pointless, which is what happens when one lands + # between the observation and this delivery. + if review_exists and not review_stale: print( f"discarding Copilot review request for PR #{pr_number}: " - f"Copilot review exists={review_exists} " - f"stale={review_stale} for head {current_head}", + f"Copilot review already covers head {current_head}", file=sys.stderr, ) requests.pop(key, None) diff --git a/.github/scripts/pull-request-dashboard/dashboard.py b/.github/scripts/pull-request-dashboard/dashboard.py index c54ac8e6530..d71bedf28ff 100644 --- a/.github/scripts/pull-request-dashboard/dashboard.py +++ b/.github/scripts/pull-request-dashboard/dashboard.py @@ -130,11 +130,22 @@ current head, so a re-review would see unreviewed code. False when Copilot has never - reviewed, because that PR - awaits the automatic first - review rather than a - re-request. Only a stale - review is worth re-requesting. + reviewed; that PR is tracked + by + copilot_first_review_missing_since + instead. + copilot_first_review_missing_since + str (iso) When the gate first observed + this non-draft PR with no + Copilot review at all. Carried + forward across passes, and + absent once a review exists, + the PR is a draft, or the gate + does not apply. Once it is + older than the grace period, + the automatic first review is + presumed lost and the + dashboard requests one. copilot_review_needed bool The review is stale or Copilot owns open review threads, meaning unresolved threads @@ -238,6 +249,7 @@ copilot_review_status, is_copilot_reviewer, record_copilot_review_observation, + set_copilot_first_review_missing_since, set_copilot_review_request_needed, ) from dashboard_override import ( @@ -1408,7 +1420,9 @@ def resolve_pr_route( required_approvals: int, require_clean_copilot_review: bool, previous_result: dict[str, Any] | None = None, + now: datetime | None = None, ) -> str: + now = now or utc_now() route = route_pr(facts, pending_actions, required_approvals) previous_facts = (previous_result or {}).get("facts") or {} override_cleared_actions = bool( @@ -1442,8 +1456,14 @@ def resolve_pr_route( copilot_review_gate_enabled = ( require_clean_copilot_review and not manual_reviewer_handoff ) + set_copilot_first_review_missing_since( + facts, + previous_result, + enabled=copilot_review_gate_enabled, + now=now, + ) set_copilot_review_request_needed( - facts, route, enabled=copilot_review_gate_enabled + facts, route, enabled=copilot_review_gate_enabled, now=now ) return hold_route_until_gates_settle( facts, @@ -1471,6 +1491,12 @@ def preserve_override_state_after_failure( previous_facts.get("copilot_review_bypassed_by_override") and same_overridden_head ) + # A failed pass must not restart the first-review clock, or a repeatedly + # failing classification would keep the wait permanently under the grace. + if previous_facts.get("copilot_first_review_missing_since"): + facts["copilot_first_review_missing_since"] = previous_facts[ + "copilot_first_review_missing_since" + ] def assign_author_nudge_episode( diff --git a/.github/scripts/pull-request-dashboard/render.py b/.github/scripts/pull-request-dashboard/render.py index d07c124890e..840fe1b3775 100644 --- a/.github/scripts/pull-request-dashboard/render.py +++ b/.github/scripts/pull-request-dashboard/render.py @@ -154,9 +154,9 @@ def copilot_review_pending(facts: dict[str, Any]) -> bool: # Copilot earns a place only where its review holds the pull request. That # scope comes first, and within it a requested review qualifies, as does a # pull request Copilot has never reviewed, because the automatic first - # review is never requested and the hold it causes would otherwise have - # nothing on the row to explain it. A hold is not enough on its own: - # unsettled checks hold a route too. + # review is not requested through the dashboard and the hold it causes + # would otherwise have nothing on the row to explain it. A hold is not + # enough on its own: unsettled checks hold a route too. if not facts.get("copilot_review_outstanding"): return False if facts.get("copilot_review_requested"): diff --git a/.github/scripts/pull-request-dashboard/test_copilot_review.py b/.github/scripts/pull-request-dashboard/test_copilot_review.py index f8a42cfada4..7898a714be6 100644 --- a/.github/scripts/pull-request-dashboard/test_copilot_review.py +++ b/.github/scripts/pull-request-dashboard/test_copilot_review.py @@ -8,8 +8,11 @@ from datetime import datetime, timezone from copilot_review import ( + copilot_first_review_overdue, deliver_copilot_review_requests, record_copilot_review_observation, + set_copilot_first_review_missing_since, + set_copilot_review_request_needed, stale_request_reason, ) @@ -17,6 +20,175 @@ NOW = datetime(2026, 7, 20, 2, tzinfo=timezone.utc) +class CopilotFirstReviewMissingSinceTest(unittest.TestCase): + def test_starts_clock_when_review_is_missing(self) -> None: + facts: dict = {} + + set_copilot_first_review_missing_since( + facts, None, enabled=True, now=NOW + ) + + self.assertEqual( + "2026-07-20T02:00:00+00:00", + facts["copilot_first_review_missing_since"], + ) + + def test_carries_clock_forward_across_passes(self) -> None: + facts: dict = {} + previous = { + "facts": {"copilot_first_review_missing_since": "2026-07-20T00:00:00+00:00"} + } + + set_copilot_first_review_missing_since( + facts, previous, enabled=True, now=NOW + ) + + self.assertEqual( + "2026-07-20T00:00:00+00:00", + facts["copilot_first_review_missing_since"], + ) + + def test_push_does_not_restart_clock(self) -> None: + # GitHub does not automatically review a PR it has never reviewed, so a + # push must not reset the wait or an active PR would never recover. + facts: dict = {"head_sha": "new-head"} + previous = { + "facts": { + "head_sha": "old-head", + "copilot_first_review_missing_since": "2026-07-20T00:00:00+00:00", + } + } + + set_copilot_first_review_missing_since( + facts, previous, enabled=True, now=NOW + ) + + self.assertEqual( + "2026-07-20T00:00:00+00:00", + facts["copilot_first_review_missing_since"], + ) + + def test_draft_clears_clock(self) -> None: + facts: dict = {"is_draft": True} + previous = { + "facts": {"copilot_first_review_missing_since": "2026-07-20T00:00:00+00:00"} + } + + set_copilot_first_review_missing_since( + facts, previous, enabled=True, now=NOW + ) + + self.assertNotIn("copilot_first_review_missing_since", facts) + + def test_existing_review_clears_clock(self) -> None: + facts: dict = {"copilot_review_exists": True} + previous = { + "facts": {"copilot_first_review_missing_since": "2026-07-20T00:00:00+00:00"} + } + + set_copilot_first_review_missing_since( + facts, previous, enabled=True, now=NOW + ) + + self.assertNotIn("copilot_first_review_missing_since", facts) + + def test_disabled_gate_clears_clock(self) -> None: + facts: dict = {} + previous = { + "facts": {"copilot_first_review_missing_since": "2026-07-20T00:00:00+00:00"} + } + + set_copilot_first_review_missing_since( + facts, previous, enabled=False, now=NOW + ) + + self.assertNotIn("copilot_first_review_missing_since", facts) + + def test_overdue_only_after_the_grace_period(self) -> None: + self.assertFalse(copilot_first_review_overdue({}, NOW)) + self.assertFalse( + copilot_first_review_overdue( + {"copilot_first_review_missing_since": "2026-07-20T01:01:00+00:00"}, + NOW, + ) + ) + self.assertTrue( + copilot_first_review_overdue( + {"copilot_first_review_missing_since": "2026-07-20T01:00:00+00:00"}, + NOW, + ) + ) + + +class CopilotFirstReviewRequestTest(unittest.TestCase): + def base_facts(self, **overrides) -> dict: + facts = { + "copilot_review_exists": False, + "copilot_review_stale": False, + "copilot_review_requested": False, + "ci_pending_count": 0, + } + facts.update(overrides) + return facts + + def test_within_grace_does_not_request(self) -> None: + facts = self.base_facts( + copilot_first_review_missing_since="2026-07-20T01:30:00+00:00", + ) + + set_copilot_review_request_needed( + facts, "approver", enabled=True, now=NOW + ) + + self.assertFalse(facts["copilot_review_request_needed"]) + + def test_past_grace_requests_the_first_review(self) -> None: + facts = self.base_facts( + copilot_first_review_missing_since="2026-07-20T00:30:00+00:00", + ) + + set_copilot_review_request_needed( + facts, "approver", enabled=True, now=NOW + ) + + self.assertTrue(facts["copilot_review_request_needed"]) + + def test_pending_request_is_not_duplicated(self) -> None: + facts = self.base_facts( + copilot_review_requested=True, + copilot_first_review_missing_since="2026-07-20T00:30:00+00:00", + ) + + set_copilot_review_request_needed( + facts, "approver", enabled=True, now=NOW + ) + + self.assertFalse(facts["copilot_review_request_needed"]) + + def test_unsettled_checks_hold_the_first_review_request(self) -> None: + facts = self.base_facts( + ci_pending_count=1, + copilot_first_review_missing_since="2026-07-20T00:30:00+00:00", + ) + + set_copilot_review_request_needed( + facts, "approver", enabled=True, now=NOW + ) + + self.assertFalse(facts["copilot_review_request_needed"]) + + def test_author_route_does_not_request(self) -> None: + facts = self.base_facts( + copilot_first_review_missing_since="2026-07-20T00:30:00+00:00", + ) + + set_copilot_review_request_needed( + facts, "author", enabled=True, now=NOW + ) + + self.assertFalse(facts["copilot_review_request_needed"]) + + class CopilotReviewRequestStateTest(unittest.TestCase): @patch("copilot_review.save_copilot_review_requests") @patch("copilot_review.load_copilot_review_requests", return_value={}) @@ -130,7 +302,7 @@ def test_clears_request_when_no_longer_needed(self, _load_requests, save_request @patch("copilot_review.save_copilot_review_requests") @patch("copilot_review.load_copilot_review_requests", return_value={}) - def test_initial_automatic_review_does_not_enqueue_request( + def test_missing_first_review_within_grace_does_not_enqueue_request( self, _load_requests, save_requests, @@ -347,7 +519,12 @@ def test_drops_request_when_live_routing_inputs_changed( return_value="accepted-fingerprint", ) @patch("copilot_review.request_copilot_review") - @patch("copilot_review.fetch_pr_reviews", return_value=[]) + @patch( + "copilot_review.fetch_pr_reviews", + return_value=[ + {"user": {"login": "Copilot"}, "commit_id": "current-head"}, + ], + ) @patch( "copilot_review.fetch_current_pr_routing_inputs", return_value=( @@ -393,10 +570,62 @@ def test_drops_request_when_copilot_review_no_longer_needed( save_requests.assert_called_once_with({}) self.assertIn( "discarding Copilot review request for PR #7: Copilot review " - "exists=False stale=False for head current-head", + "already covers head current-head", stderr.getvalue(), ) + @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_delivers_request_for_missing_first_review( + self, + _load_requests, + save_requests, + _fetch_current_state, + _fetch_reviews, + request_review, + _fingerprint, + ) -> None: + errors = deliver_copilot_review_requests("open-telemetry/example", NOW) + + self.assertEqual([], errors) + request_review.assert_called_once_with("PR_node") + save_requests.assert_called_once_with({ + "7": { + "head_sha": "current-head", + "observed_at": "2026-07-20T01:00:00+00:00", + "requested_at": "2026-07-20T02:00:00+00:00", + "routing_input_fingerprint": "accepted-fingerprint", + }, + }) + class StaleRequestReasonTest(unittest.TestCase): ENTRY = { diff --git a/.github/scripts/pull-request-dashboard/test_dashboard.py b/.github/scripts/pull-request-dashboard/test_dashboard.py index 0c987c86ffb..f61329b6072 100644 --- a/.github/scripts/pull-request-dashboard/test_dashboard.py +++ b/.github/scripts/pull-request-dashboard/test_dashboard.py @@ -1199,12 +1199,13 @@ def test_waits_for_automatic_initial_copilot_review(self) -> None: self.assertFalse(facts["copilot_review_exists"]) self.assertFalse(facts["copilot_review_needed"]) - def test_initial_automatic_review_needs_no_request(self) -> None: + def test_pending_first_review_request_is_not_duplicated(self) -> None: facts = { "ci_pending_count": 0, "copilot_review_requested": True, "copilot_review_exists": False, "copilot_review_stale": False, + "copilot_first_review_missing_since": "2020-01-01T00:00:00+00:00", } set_copilot_review_request_needed(facts, "approver", enabled=True) diff --git a/pull-request-dashboard/README.md b/pull-request-dashboard/README.md index f1aeef125de..c3ef3935241 100644 --- a/pull-request-dashboard/README.md +++ b/pull-request-dashboard/README.md @@ -20,7 +20,7 @@ The dashboard groups open non-draft pull requests by who is expected to act next - 💬 has an open (unresolved) review thread on the PR - 📌 has tracked top-level feedback that still needs author action - 🔴 requested changes - - ⏳ a review is in flight (only used for Copilot: a requested re-review, or the automatic first review while it holds the PR) + - ⏳ a review is in flight (only used for Copilot: a requested review, or a first review that has not arrived yet while it holds the PR) - Icons combine when multiple states apply. For example, 💬📌 means the reviewer has both an unresolved inline thread and tracked top-level feedback; ✅ may accompany either or both. - **CI** — Aggregate check status across the PR's required status checks. Optional checks do not affect this column: - ✅ all required checks passing @@ -72,16 +72,18 @@ 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 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. 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. | `labels_to_display` only controls which labels are shown. It does not filter pull requests or affect dashboard routing, notifications, or status comments. All matching labels are displayed in the order returned by GitHub; a label matching more than one configured pattern is shown once. -For each listed branch, `require_clean_copilot_review_branches` relies on automatic Copilot -code review for the initial review, so list only branches where automatic Copilot -code review is enabled. The dashboard requests later reviews using its GitHub App +For each listed branch, `require_clean_copilot_review_branches` expects automatic Copilot +code review to produce the initial review, so list only branches where automatic Copilot +code review is enabled. When no automatic review has arrived within an hour of the PR +becoming ready, the dashboard requests one itself rather than holding the PR on its author +indefinitely. The dashboard requests reviews using its GitHub App installation token with pull-request write permission. Leave **Review new pushes** disabled if the dashboard should request re-reviews only when a PR is ready to return to reviewers or maintainers.