Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 34 additions & 6 deletions .github/scripts/pull-request-dashboard/RATIONALE.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
75 changes: 65 additions & 10 deletions .github/scripts/pull-request-dashboard/copilot_review.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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))

Expand Down Expand Up @@ -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)
)
Expand Down Expand Up @@ -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)
Expand Down
38 changes: 32 additions & 6 deletions .github/scripts/pull-request-dashboard/dashboard.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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 (
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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(
Expand Down
6 changes: 3 additions & 3 deletions .github/scripts/pull-request-dashboard/render.py
Original file line number Diff line number Diff line change
Expand Up @@ -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"):
Expand Down
Loading