Stop the Copilot review gate from holding a pull request forever - #255
Draft
trask wants to merge 7 commits into
Draft
Stop the Copilot review gate from holding a pull request forever#255trask wants to merge 7 commits into
trask wants to merge 7 commits into
Conversation
…livered Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
trask
changed the base branch from
main
to
trask-recover-missing-copilot-first-review
August 16, 2026 15:44
Contributor
There was a problem hiding this comment.
Pull request overview
Confirms GitHub recorded Copilot review requests before marking them delivered.
Changes:
- Retries confirmation reads and accepts pending or completed reviews as proof.
- Tracks repeated unconfirmed requests and fails after three.
- Documents behavior and expands tests.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
pull-request-dashboard/README.md |
Documents delivery confirmation. |
RATIONALE.md |
Explains retry and failure policy. |
copilot_review.py |
Implements confirmation and counters. |
github_cli.py |
Clarifies mutation semantics. |
state.py |
Merges confirmation counters across retries. |
test_copilot_review.py |
Tests confirmation behavior. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
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
trask
changed the title
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
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated no new comments.
Suppressed comments (3)
.github/scripts/pull-request-dashboard/copilot_review.py:332
requested_atnow 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 instate.py:20-34,COPILOT_REVIEW_REQUEST_STATE_VERSIONmust 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)}
.github/scripts/pull-request-dashboard/dashboard.py:2030
- The half-pass cap is bypassed when priority PRs are contiguous in rotation order. For example, with a cursor at 4 and PRs 5–8 all waiting,
priorityis[5, 6], butremainingstarts[7, 8, ...], so all four slots still go to waiting PRs and ordinary rotation does not advance. Exclude every priority number from the rotation portion, then use deferred priorities only when there are not enough non-priority PRs to fill the pass.
remaining = [number for number in rotation if number not in set(priority)]
selected_numbers = (priority + remaining)[:max_prs]
.github/scripts/pull-request-dashboard/dashboard.py:1458
- These new hold-clock facts change the persisted
dashboard-state.jsonshape, butDASHBOARD_STATE_VERSIONremains 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)
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
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>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated no new comments.
Suppressed comments (1)
pull-request-dashboard/README.md:75
- 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). |
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>
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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The dashboard holds a pull request on its author until the required status checks and the Copilot review report on the current head. Neither wait had a limit, so a gate that never reported held the pull request for as long as it stayed open, and it looked like a pull request waiting normally.
open-telemetry/opentelemetry-java-instrumentation#19053has been held since 2026-07-18 because two required checks have no check runs on its head.A gate may now keep a pull request off its reviewers for four hours. Past that it routes anyway, and its status comment says which gate the dashboard gave up on:
An expired hold is reported as a delivery failure, which opens the tracking issue the dashboard already uses. This replaces the per-request escalation this branch originally added.