Read the pickup event, so a dead request stops looking like patience by ptr727 · Pull Request #527 · ptr727/ProjectTemplate

ptr727 · 2026-08-03T13:21:15Z

A live stall on another repository's pull request exposed a third wait state and two wrong readings behind it. The pull request had been sitting for thirteen and a half hours reading as waiting on the reviewer. The request was pending, and nothing was acting on it.

The state

Copilot raises copilot_work_started within about half a minute of accepting a request, and reviews a few minutes later. A request that never draws one stays that way indefinitely. From the reviews alone it is identical to a slow round, and elapsed time cannot separate them either, since a slow round also shows no review. Only the pickup event says whether anything is working.

wait now exits 50 on a pending request with no pickup after it, naming the request's timestamp. The pickup is read before the timeout, so the stall reports as itself rather than as PENDING once the clock runs out, and only after --pickup-grace (five minutes) rather than per poll, since inside that grace a pending request is simply a review in progress. It is the one reading here taken over REST, because no GraphQL timeline item carries the event.

The two wrong readings

gh pr view --json reviewRequests omits a Bot reviewer entirely. It returned an empty set while GraphQL returned totalCount: 1 with the reviewer in it. This is what makes the state so easy to misdiagnose, and it did: on the strength of that projection the stall was reported to the maintainer as "no request was ever made", which was wrong. The pending set is now read through GraphQL.

The runbook called removal impossible, on the grounds that no removePullRequestFromReviewRequest mutation exists. That much is true and the conclusion drawn from it was not: requestReviews replaces the reviewer set when union is false, which the schema states outright, so an empty botIds clears the request. That is the clear half of the recovery, and the runbook had ruled it out.

Recovery

Clear the request, then request again. The pull request UI offers no re-request control while a request is pending, and union: true adds a reviewer already in the set, which changes nothing. Both mutations are in the runbook with the caveat that union: false replaces the whole set, so the pending set is read first lest a human reviewer be dropped alongside the bot. Run against the stalled pull request, this drew copilot_work_started 35 seconds later, on a request that had produced nothing for thirteen and a half hours, and the review that followed raised a real finding.

Recovery stays out of pr_review.py, which holds its no-mutation contract: the digest names the state, and the mutations stay visible to the write guard and to review.

Verification

The stuck-request case was watched failing against a build with the pickup read removed, where it runs the wait out and reports the stall as PENDING. It fails rather than hangs, which took a second pass: the first version of the case spun for the whole timeout instead of failing, and a case that hangs gates nothing.

scripts/test_pr_review.py 54 pass, scripts/test_prose_lint.py 153 pass, spec/audit.py --selftest pass, spec/validate.py clean, repo_gate.py clean, both prose_lint.py invocations clean over the diff and over every touched file. The new digest was also run against two live pull requests, one with a pending request and one without.

🤖 Generated with Claude Code

A request the reviewer never picks up produces exactly what a slow review produces, which is nothing, and one sat for thirteen and a half hours while the pull request read as waiting on the reviewer. Elapsed time cannot separate them, since a genuinely slow round shows no review either, so the wait reads `copilot_work_started` instead of the clock. A request with no pickup after it ends the wait with exit 50 and the timestamp, because waiting on does not start what nothing is acting on. Two readings this depended on were wrong. `gh pr view --json reviewRequests` omits a Bot reviewer outright, reporting an empty set while the reviewer sits in it, so the pending set is read through GraphQL. And the runbook called removal impossible for want of a named mutation, where `requestReviews` replaces the set when `union` is false, which is the clear half of the recovery it now documents. Recovery stays out of the script, which keeps its no-mutation contract. The digest names the state and the runbook carries the two mutations. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Copilot

Pull request overview

This PR enhances scripts/pr_review.py to detect and report a third “wait” state where a Copilot review request is pending but never gets picked up (no copilot_work_started), adding a new exit code (50) and updating the runbook and tests accordingly.

Changes:

Add GraphQL visibility into pending review requests (reviewRequests) and incorporate it into the digest/wait output.
Add a REST timeline reader to detect a “requested but never picked up” state and surface it as status=REQUEST_NOT_PICKED_UP with exit code 50.
Expand test coverage and update documentation/runbook to describe the new state and recovery steps.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File	Description
scripts/pr_review.py	Adds pending-request detection via GraphQL, pickup detection via REST timeline, and new wait/digest signaling with exit code 50.
scripts/test_pr_review.py	Extends fixtures and adds tests for pickup/stalled-request detection and pending-set reading.
scripts/README.md	Documents the new wait state, exit code 50, and the rationale for checking pickup events.
.github/copilot-instructions.md	Updates the runbook to include the “stuck pending request” state and the clear/re-request recovery flow.

The comment and the README said the read avoids running per poll, and past the grace it ran on every one. A forty-minute wait against a picked up request turned one REST reader into thirty-two, which is the rate limit exposure the design was written to avoid, and the documentation described the design rather than the code. The read now waits an interval between readings, the grace serving as both the first delay and the spacing. One reading settles the request in front of it, and the next covers a request a push raises mid-wait, so the later readings are not redundant. A case counts the readings against a stepped clock and was watched failing at thirty-two against the bound. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Copilot

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

Suppressed comments (1)

scripts/pr_review.py:96

timeline() uses gh api --paginate without increasing per_page, so on PRs/issues with long timelines it may require many REST requests per pickup check (default page size is 30). Since this runs repeatedly during wait, it increases rate-limit exposure. Consider requesting the maximum page size to reduce the number of API calls.

    r = subprocess.run(
        ['gh', 'api', '--paginate', f'repos/{owner}/{repo}/issues/{num}/timeline',
         '--jq', '.[] | select(.event == "copilot_work_started" or .event == "review_requested")'
                 ' | "\\(.event) \\(.created_at)"'],
        capture_output=True, text=True)

`--paginate` walks the default page size of 30, so the reader's cost scales with how long a pull request has run rather than with what it reads. The 168-event timeline this state was found on takes six requests a reading at 30 and two at 100, against a reader that repeats for the life of a wait. Two cases cover it, one holding the request to the largest page and to being a read, the other holding a failed read to raising rather than returning no events, since an empty list reads as no request pending, which is the false clean this reader exists to close. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

ptr727 · 2026-08-03T13:34:13Z

Suppressed comments (1) from review #527 (review), confirmed and fixed in 93b3701.

scripts/pr_review.py:96
timeline() uses gh api --paginate without increasing per_page, so on PRs/issues with long timelines it may require many REST requests per pickup check (default page size is 30). Since this runs repeatedly during wait, it increases rate-limit exposure. Consider requesting the maximum page size to reduce the number of API calls.

Fixed in 93b3701. Correct, and it lands on the same nerve as the threaded finding from the previous round, which is what makes it worth more than a one-line change. Both are the reader costing more requests than its design admits: that one ran per poll instead of per interval, this one walks the pull request's whole history 30 events at a time. The interval fix bounded how often the reader runs and left how much it costs each time untouched.

Measured on the pull request this state was actually found on rather than in the abstract. ptr727/PhotoCleaner#35 carries a 168-event timeline, which is six requests a reading at the default page size and two at the maximum. That pull request is also exactly the profile where the pickup reader matters, since a long-running review loop is what accumulates both the timeline and the stalls.

Two cases went in with it. One holds the request to the largest page and to being a read, asserting no -X or --method reaches the one REST call in this script, which keeps the no-mutation contract honest now that not every call is GraphQL. The other holds a failed read to raising rather than returning no events, because an empty list reads as no request pending, and that is the false clean this reader exists to close, arriving one level up inside the reader itself.

Also verified live: the reader now returns 69 request and pickup events from that timeline and correctly reports the current request as picked up rather than stalled.

Copilot

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

Suppressed comments (2)

scripts/pr_review.py:97

timeline() treats every review_requested event as a Copilot request. The REST timeline event payload includes requested_reviewer, and other review requests (e.g., a human reviewer requested after Copilot) can occur later; using the newest request timestamp regardless of who was requested can incorrectly flag REQUEST_NOT_PICKED_UP even if Copilot was picked up normally.

Filter review_requested events to only those where requested_reviewer.login is Copilot (or at least starts with copilot-pull-request-reviewer) before feeding them to never_picked_up().

        ['gh', 'api', '--paginate', f'repos/{owner}/{repo}/issues/{num}/timeline?per_page=100',
         '--jq', '.[] | select(.event == "copilot_work_started" or .event == "review_requested")'
                 ' | "\\(.event) \\(.created_at)"'],

scripts/pr_review.py:314

--pickup-grace accepts negative values, which makes next_pickup stay behind elapsed and effectively forces a timeline read on every poll iteration (reintroducing the per-poll REST-call pattern) due to elapsed > next_pickup always being true. Reject negative values at argument-parse time.

    ap.add_argument('--pickup-grace', type=int, default=300,
                    help='seconds before the first pickup read, and between reads (default 5m)')
    a = ap.parse_args(argv)
    owner, repo = a.repo.split('/', 1)

Every `review_requested` event counted as the reviewer's, so a human requested after a review landed became the newest request with no pickup after it, and a picked-up review reported as never picked up. That is a false stop rather than a missed one, on a pull request whose review is sitting there. The timeline spells the reviewer a third way, login `Copilot` with type `Bot`, against GraphQL's `copilot-pull-request-reviewer` and the `[bot]` suffix REST user objects carry. Filtering on either of those two selects nothing, so the predicate is the type plus a loose login match, and the runbook records the third form beside the other two. A case drives the real jq over a crafted timeline rather than asserting on the filter text, which would pass on a filter matching nothing. A negative `--pickup-grace` is also rejected, since it leaves the next reading behind the clock and reinstates the per-poll reads by argument. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

ptr727 · 2026-08-03T13:42:57Z

Suppressed comments (2) from review #527 (review), both confirmed and fixed in 3766943. The earlier block still in the digest is answered above.

scripts/pr_review.py:97
timeline() treats every review_requested event as a Copilot request. The REST timeline event payload includes requested_reviewer, and other review requests (e.g., a human reviewer requested after Copilot) can occur later; using the newest request timestamp regardless of who was requested can incorrectly flag REQUEST_NOT_PICKED_UP even if Copilot was picked up normally.
Filter review_requested events to only those where requested_reviewer.login is Copilot (or at least starts with copilot-pull-request-reviewer) before feeding them to never_picked_up().

Fixed in 3766943, though not with the filter as suggested, and the difference is worth stating because the suggested one fails silently.

The diagnosis is exactly right and the failure is the bad kind: requesting a human reviewer after a review lands makes that the newest request, nothing picks it up because nothing is meant to, and a picked-up review reports as REQUEST_NOT_PICKED_UP. A false stop on a pull request whose review is sitting right there, which is worse than the missed stop this feature was written to catch.

The suggested predicate does not work here. The timeline spells the reviewer a third way: requested_reviewer is login Copilot with type Bot, against GraphQL's copilot-pull-request-reviewer and the [bot] suffix REST user objects carry. Measured on ptr727/PhotoCleaner#35, which has four such events:

startswith("copilot-pull-request-reviewer")  ->  0 events
type == "Bot" and login matches /copilot/i   ->  71 events, newest correct

A filter keyed to either of the documented spellings selects nothing, so every request disappears, never_picked_up sees no requests at all, and the state reports clean forever. That is the same false clean one level up, and it would have been invisible: the feature would simply never fire again. The predicate is now the type plus a loose login match, which holds across all three spellings, and the runbook records the third form beside the other two.

The case for it drives the real jq over a crafted timeline carrying a bot request, a human request, and another bot's request, rather than asserting on the filter's text. A text assertion passes just as happily on a filter that matches nothing, which is precisely the failure being guarded.

scripts/pr_review.py:314
--pickup-grace accepts negative values, which makes next_pickup stay behind elapsed and effectively forces a timeline read on every poll iteration (reintroducing the per-poll REST-call pattern) due to elapsed > next_pickup always being true. Reject negative values at argument-parse time.

Fixed in 3766943. Correct, and it is the previous round's finding walking back in through the argument parser: the interval bounds the reads only while the interval is positive. Rejected at parse time with a case covering it.

Copilot

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

…ckup-event tooling to main (#528) Promotes four commits to `main`. Two of them change **carried** files, so downstream repos read the stale text until this lands, which is what makes the promotion the delivery step rather than bookkeeping. ## What this delivers to the fleet **Carried rule text**, picked up by every repo on its next re-vendor: | PR | File and section | Change | | --- | --- | --- | | #526 | `AGENTS.md`, Context and Delegation Discipline (**verbatim**) | A wait separates three outcomes and says which one it reached: run the command in the foreground before backgrounding it, never let `\|\| echo '[]'`, `\|\| true` or `2>/dev/null` stand in for a failure, emit on failure, and bound the wait. The session rule also stops ending a session on a third review round, which read as license to leave a loop open while it was still producing defects | | #526 | `GOVERNANCE.md`, Verification Discipline (**verbatim**) | A launched process is not a result, and a cause nobody observed is not a diagnosis | | #526 | `GOVERNANCE.md`, PR Review Etiquette (**verbatim**) | Every finding ends in one of five actions rather than at a round count: fixed, disproven with proof the reviewer can read, deferred against a filed issue, declined with the maintainer's explicit answer, or fixed as a class where the code keeps earning it | | #520 | `CODESTYLE.md` and the prose gate | The comment rules the gate now reaches, carried to the fleet through a public composite action | | #526, #527 | `.github/copilot-instructions.md` | A quota or rate-limit answer is terminal rather than pending; a request pending with no pickup is a third state with a recovery recipe; three corrections below | **Runbook corrections**, each one a path an agent followed to a wrong answer this week: - `gh pr view --json reviewRequests` **omits a Bot reviewer entirely**, reporting an empty set while Copilot sits in it. This is how a live stall was misdiagnosed to the maintainer as no request having been made. - Removal was called impossible for want of a named mutation. `requestReviews` **replaces** the reviewer set when `union` is false, which is the clear half of the recovery, and it resolved a real thirteen-and-a-half-hour stall in 35 seconds. - The reviewer login has a **third** spelling. A timeline `review_requested` carries login `Copilot` with type `Bot`, against GraphQL's `copilot-pull-request-reviewer` and the `[bot]` suffix REST user objects add. A filter keyed to either documented form selects nothing there. **Hub-only tooling** (`scripts/`, not carried): `pr_review.py` gains exit `40` for a reviewer answer that carries no commit and exit `50` for a request nothing picked up, with the window and interval guards those needed. `prose_lint.py` and the gate queue changes from #520 and #522 ride along. ## Why the rules moved Three stalls in one day, each reported as waiting on the reviewer, none of them that. A CI watcher whose command did not exist on the installed `gh` and whose fallback turned every error into "nothing yet". A stall explained afterwards with a throttle that appears nowhere in the record. And a review request that was pending while nothing acted on it. The common shape is a wait that cannot tell "not yet" from "never", and a report of patience standing in for a reading nobody took. ## Verification `scripts/test_pr_review.py` 59 pass, `scripts/test_prose_lint.py` 153 pass, `scripts/test_repo_gate.py` 23 pass, `spec/audit.py --selftest` pass, `spec/validate.py` clean, `repo_gate.py` clean, both `prose_lint.py` invocations clean, `editorconfig-checker` clean. Both source pull requests were driven to a clean Copilot round: #526 over seven rounds and ten findings, #527 over four rounds and five findings, every one of the fifteen real and answered. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

Copilot AI review requested due to automatic review settings August 3, 2026 13:21

Copilot started reviewing on behalf of ptr727 August 3, 2026 13:21 View session

Copilot AI reviewed Aug 3, 2026

View reviewed changes

Comment thread scripts/pr_review.py

Copilot AI review requested due to automatic review settings August 3, 2026 13:28

Copilot started reviewing on behalf of ptr727 August 3, 2026 13:29 View session

Copilot AI reviewed Aug 3, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings August 3, 2026 13:33

Copilot started reviewing on behalf of ptr727 August 3, 2026 13:34 View session

Copilot AI reviewed Aug 3, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings August 3, 2026 13:42

Copilot started reviewing on behalf of ptr727 August 3, 2026 13:43 View session

Copilot AI reviewed Aug 3, 2026

View reviewed changes

ptr727 merged commit 302d7a1 into develop Aug 3, 2026
7 checks passed

ptr727 deleted the feature/stuck-review-request branch August 3, 2026 13:48

ptr727 mentioned this pull request Aug 3, 2026

Promote the wait failure clause, the five review outcomes, and the pickup-event tooling to main #528

Merged

Navigation Menu

Read the pickup event, so a dead request stops looking like patience - #527

Read the pickup event, so a dead request stops looking like patience#527
ptr727 merged 4 commits into
developfrom
feature/stuck-review-request

ptr727 commented Aug 3, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

ptr727 commented Aug 3, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

ptr727 commented Aug 3, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

ptr727 commented Aug 3, 2026

The state

The two wrong readings

Recovery

Verification

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

ptr727 commented Aug 3, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

ptr727 commented Aug 3, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants