Skip to content
Merged
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
43 changes: 41 additions & 2 deletions .github/copilot-instructions.md
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ gh api repos/<owner>/<repo>/pulls/<N>/reviews --jq \

**Round 1 is normally auto-seeded, so poll for it before trying to self-trigger.** Auto-review-on-open supplies the first review with no `botIds` call needed, but it can lag one to three minutes. After opening a PR (or the first push), **poll** for a Copilot review on the head SHA (see [Verify Review Covered Current Head](#verify-review-covered-current-head)) before concluding none ran. The `requestReviews` mutation below is for **re-requesting on later pushes** (a new head SHA). By then a prior review exists, so its bot node id is readable. A missing bot node id on round 1 therefore means "the auto-review has not landed yet - wait and poll," **not** "ask the maintainer to kick it off."

> **The reviewer login differs by API.** In **GraphQL** (`gh api graphql` and `gh pr view --json reviews`, which is GraphQL-backed) the `Bot.login` is `copilot-pull-request-reviewer`, with **no `[bot]` suffix**. In the **REST** API (`gh api repos/.../issues|pulls/...`) the same account's `user.login` is `copilot-pull-request-reviewer[bot]`, **with** the suffix. Each query below uses the correct form for its API, so match the API, not a single spelling, when adapting them.
> **The reviewer login differs by API, in three forms rather than two.** In **GraphQL** (`gh api graphql` and `gh pr view --json reviews`, which is GraphQL-backed) the `Bot.login` is `copilot-pull-request-reviewer`, with **no `[bot]` suffix**. In the **REST** API (`gh api repos/.../issues|pulls/...`) the same account's `user.login` is `copilot-pull-request-reviewer[bot]`, **with** the suffix. In a REST **timeline** `review_requested` event the `requested_reviewer` is a third spelling again, login `Copilot` with `type` `Bot`, so a filter written against either of the other two selects nothing there and reports a pull request with requests as having none. Match on the type plus a loose login test rather than on any one spelling, and each query below uses the correct form for its API.

```sh
# 1. PR node id + the Copilot reviewer's bot node id (read from any existing
Expand Down Expand Up @@ -116,7 +116,8 @@ Known non-working request paths (don't rely on them, and use the `requestReviews
- `copilot-pull-request-reviewer` as a requested reviewer slug returns 422.
- `requestReviews` with the reviewer's bot node id in **`userIds`** fails with `Could not resolve to User node`, because the Copilot reviewer is a **Bot**, so its node id goes in **`botIds`** (as in the mutation above), never `userIds`.
- `suggestedActors(capabilities: [CAN_BE_ASSIGNED])` lists `copilot-swe-agent` (the coding agent), not `copilot-pull-request-reviewer`, so do not source the reviewer's bot node id there. Read it from an existing review per step 1 above.
- There is no `removePullRequestFromReviewRequest` mutation, and removing the reviewer to force a fresh pass is unnecessary anyway, since `requestReviews` with `union: true` re-fires the review on the current head.
- There is no `removePullRequestFromReviewRequest` mutation, but removal is not therefore impossible: `requestReviews` **replaces** the reviewer set when `union` is false (the schema describes `union` as "add users to the set rather than replace"), so an empty `botIds` with `union: false` removes the pending request. Reach for it only in the stuck case below, since `union: true` re-fires a review on the current head without it.
- `gh pr view --json reviewRequests` **omits a Bot reviewer entirely**, reporting an empty set while Copilot sits in it. Read the pending set through GraphQL `reviewRequests`, which returns the `Bot` node, because the REST-backed projection makes a pending request read as no request at all.

### Verify Review Covered Current Head

Expand Down Expand Up @@ -150,6 +151,44 @@ This path is only for a **genuinely missing** review, meaning no Copilot review

**Bound each wait, and read what Copilot actually posted before opening another one.** A poll that widens forever is indistinguishable from a poll that has stopped, and "still pending" is the honest report for exactly as long as evidence supports it. Two readings decide whether waiting again is warranted. Compare the request's timestamp against the newest Copilot activity of **any** kind on the pull request, since a reviewer that has already answered on a later head, or that posted an issue comment instead of a formal review, is not a reviewer running late, and a wait that keeps reporting "pending" against a landed review is a broken wait rather than a slow reviewer. Then read that newest response, because a Copilot answer naming a quota or a rate limit is a **terminal** outcome rather than a pending one: no formal review will land, so path (1) never matches the head and path (2) is correctly never confirmed, both paths behave exactly as specified, and the agent waits for something that is not coming. The fix is account-side and re-requesting does not change it, so report it to the maintainer and stop waiting. Where the newest response is neither a review nor a refusal you recognize, that too goes to the maintainer with its text, rather than being waited through.

**A pending request nothing picked up is a third state, and it is the one that looks most like patience.** Copilot raises a `copilot_work_started` timeline event within about half a minute of accepting a request, and submits its review a few minutes later. A request that never draws one is not a slow review, it is a request nothing is acting on, and it stays that way indefinitely: one sat for thirteen and a half hours while the pull request read as waiting on the reviewer. Elapsed time cannot tell the two apart, since a genuinely slow round also shows no review, so read the event rather than the clock. `copilot_work_started` appears in the REST timeline only, and no GraphQL timeline item carries it:

```sh
# The pending set (GraphQL, since the `gh pr view` projection cannot see a Bot reviewer).
gh api graphql -f query='
{ repository(owner:"<owner>",name:"<repo>"){ pullRequest(number:<N>){
reviewRequests(first:10){ totalCount
nodes{ requestedReviewer{ __typename ... on Bot{login} ... on User{login} } } } } } }'

# The request and pickup events, newest last. A `review_requested` with no later
# `copilot_work_started` is the stuck state. Requests are filtered to the reviewer's own,
# since a human requested afterwards is a different request and reading it as this one
# reports a picked-up review as never picked up. `per_page` is the pagination cost.
gh api --paginate 'repos/<owner>/<repo>/issues/<N>/timeline?per_page=100' \
--jq '.[] | select(.event == "copilot_work_started" or (.event == "review_requested"
and .requested_reviewer.type == "Bot"
and ((.requested_reviewer.login // "") | ascii_downcase | test("copilot"))))
| "\(.event) \(.created_at)"'
```

**Recover it by clearing the request and requesting again**, because the pull request UI offers no re-request control while a request is pending, and `requestReviews` with `union: true` adds a reviewer already in the set, which changes nothing. Read the pending set first, since `union: false` replaces the whole set and would drop a human reviewer requested alongside the bot. Where the clear-and-request does not draw a `copilot_work_started` within a minute or so, push a commit instead, since a new head raises a fresh request rather than poking a stale one.

```sh
PR_NODE=$(gh pr view <N> --json id --jq '.id')
# 1. Clear. `union: false` replaces the set, so an empty botIds removes the pending request.
gh api graphql -f query='
mutation($pr: ID!) {
requestReviews(input: { pullRequestId: $pr, botIds: [], union: false }) {
pullRequest { reviewRequests(first: 10) { totalCount } } }
}' -F pr="$PR_NODE"
# 2. Request again, against a now-empty set, with $BOT_ID read as in "Triggering and Polling".
gh api graphql -f query='
mutation($pr: ID!, $bot: ID!) {
requestReviews(input: { pullRequestId: $pr, botIds: [$bot], union: true }) {
pullRequest { reviewRequests(first: 10) { totalCount } } }
}' -F pr="$PR_NODE" -F bot="$BOT_ID"
```

If a review did not run on the current head, retry:

1. Wait briefly and check head-SHA coverage (see above).
Expand Down
4 changes: 3 additions & 1 deletion scripts/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,9 @@ python3 scripts/pr_review.py wait 452 --timeout 2700

`wait` exits `40` when Copilot answers the request with a plain comment rather than a review, meaning a comment of its own that postdates its newest review on the pull request. The test is the **shape** of that answer and not its cause, which the script reads nothing of: a comment carries no commit, so it satisfies no coverage check whatever it says, and a wait reading formal reviews alone treats it as an unmet condition and then polls out its whole timeout against an answer that already arrived. A refusal is the case that makes this worth catching, a quota or rate-limit message among them, and `40` neither asserts nor detects one. The comment prints whole because its wording is the only thing separating a refusal, which is terminal since no review follows it and re-requesting does not clear it, from an ordinary remark that is not, so `40` ends the wait and hands the text to the reader who can tell them apart. A comment **older** than the newest review is spent rather than terminal, because the review it preceded did land. Every connection reads the newest `WINDOW` nodes rather than the reviewer's own, since GraphQL offers no author filter, so ordinary traffic is what pushes theirs out of reach. `window_blind` is the one guard over both sides, and each side fails differently. Blind on **comments** means an answer could be back there unseen, which reads as `answered_outside_review=unknown` rather than `no`. Blind on **reviews** is worse, because the newest review in view is then not the newest there is, and an empty baseline dates every comment as newer so each one reads as an answer: a false `40` that stops the loop on a pull request whose review actually landed. That case reports nothing and lets the wait keep polling, since a wait that runs on is visible where a wrong terminal is not.

Everything else is decidable and says so. One of the reviewer's own nodes in view, even a **spent** one, settles the question, because nodes arrive in creation order, so anything behind the window is older than everything inside it. A window holding every node the pull request has is settled too, which is why the guard reads `pageInfo.hasPreviousPage` rather than the node count: a full window and a complete one are the same length, so length alone would report a gap where none exists. Cases hold `WINDOW` equal across all four windows and hold all four to asking for `hasPreviousPage`, since a connection that stops asking reports `no` instead of `unknown`, the silent narrowing one level up. The timeout path prints the full digest for the same reason, as a bare `PENDING` line reports a slow reviewer and a broken poll identically, which is the reading that turns a stalled watcher into a watcher nobody notices is stalled.
Everything else is decidable and says so. One of the reviewer's own nodes in view, even a **spent** one, settles the question, because nodes arrive in creation order, so anything behind the window is older than everything inside it. A window holding every node the pull request has is settled too, which is why the guard reads `pageInfo.hasPreviousPage` rather than the node count: a full window and a complete one are the same length, so length alone would report a gap where none exists. Cases hold `WINDOW` equal across all four windows and hold all four to asking for `hasPreviousPage`, since a connection that stops asking reports `no` instead of `unknown`, the silent narrowing one level up. `wait` exits `50` when the reviewer sits in the pending request set and no `copilot_work_started` follows the newest request, meaning nothing is acting on it and waiting on will not start it. That state is invisible from the reviews alone and indistinguishable from patience: one request sat thirteen and a half hours while the pull request read as waiting on the reviewer. Elapsed time cannot separate it from a slow round either, so the pickup event decides. It is the one thing here read over REST, since no GraphQL timeline item carries it, and it runs on its own interval rather than per poll: the first read comes after `--pickup-grace` (default five minutes), because inside that window a pending request is simply a review being worked on, and each later read waits another interval. One reading settles the request in front of it, and the next covers a request a push raises mid-wait, so a long wait costs a handful of REST calls instead of one per poll. The pickup is checked **before** the timeout, so the stall reports as itself instead of as `PENDING` once the clock runs out. Recovery stays out of this script, which holds its no-mutation contract: the digest names the state and the runbook carries the two mutations that clear and re-raise the request. The pending set is read through GraphQL rather than `gh pr view --json reviewRequests`, which omits a Bot reviewer outright and reports an empty set while Copilot sits in it.

The timeout path prints the full digest for the same reason, as a bare `PENDING` line reports a slow reviewer and a broken poll identically, which is the reading that turns a stalled watcher into a watcher nobody notices is stalled.

The digest also reports the **suppressed findings** a review body collapses into a `<details>` block. Those reach no review thread, so a loop that polls threads alone reports a clean pass while they stand, and the [merge gate][governance] counts them as outstanding findings either way. `suppressed=N` counts findings rather than blocks, reading the `(N)` the heading carries, since one body holds one block per round and counting blocks reports two findings as one. It covers **every** round rather than the current head, because a suppressed finding has no resolved state for a push to retire: head-scoping read "superseded by a push" as "answered", and a finding nobody replied to left the digest the moment the branch moved, so the run reported zero. That is how four rounds went unanswered across three pull requests in one day, each found by the maintainer rather than by this script. The summary line splits the count as `suppressed=N (on_head=N earlier=N)` and each block is marked with the round that raised it, since a finding on an older round may since be moot and deciding that is the reader's call rather than one the count should make for them. Each block prints whole where a thread body truncates, because a thread can be re-read at its id and a suppressed finding cannot, and it prints under a marker naming what closing it takes: no thread exists to reply on or resolve, so the answer goes in the PR conversation.

Expand Down
Loading
Loading