docs(pr-flow): make an exhaustive Copilot review loop the step after opening a PR

.claude/skills/pr-flow/SKILL.md

@@ -1,6 +1,6 @@
 ---
 name: pr-flow
-description: Take an issue through to a merged PR in this repo, and what to do at each step. Use when asked to open, create or submit a PR; when a DCO or signoff check fails; when requesting a Copilot review or responding to review comments; when naming a branch; when attaching screenshots to a PR; or when closing out after a merge.
+description: Take an issue through to a merged PR in this repo, and what to do at each step. Use when asked to create a PR for an issue, or to open or submit one; when a DCO or signoff check fails; when running the Copilot review loop after opening a PR or responding to review comments; when naming a branch; when attaching screenshots to a PR; or when closing out after a merge.
 disable-model-invocation: false
 ---
 
@@ -218,12 +218,43 @@ gh pr create --repo modelcontextprotocol/inspector \
 **default branch** (`main`). Because v2 PRs target `v2/main`, `Closes #N` there
 is only a cross-reference — it will **not** create a hard link or close the issue
 on merge. Keep it anyway, so the issues close if/when `v2/main` reaches `main`.
-There is no `gh` flag for manual linking; closing keywords are the only
-mechanism GitHub exposes.
 
-Move the card to **In Review**.
+**So link the PR to its issue explicitly, right after creating it.** The
+`addCloseIssueReferences` GraphQL mutation adds a manual closing reference, the
+same link as the UI's **Development** sidebar, and it works whatever the base
+branch. It is what puts the PR in the card's **Linked pull requests** field,
+which the board shows as a column in table views and as a chip on kanban cards.
+Without it a v2 card shows no PR at all.
 
-## 7. Request a Copilot review
+```sh
+ISSUE_ID=$(gh api graphql -F n=<ISSUE_NUMBER> -f query='query($n:Int!){
+  repository(owner:"modelcontextprotocol",name:"inspector"){issue(number:$n){id}}}' \
+  --jq .data.repository.issue.id)
+PR_ID=$(gh pr view <N> --repo modelcontextprotocol/inspector --json id --jq .id)
+gh api graphql -f query='mutation($i:ID!,$p:[ID!]!){
+  addCloseIssueReferences(input:{issueId:$i, pullRequestIds:$p}){clientMutationId}}' \
+  -f i="$ISSUE_ID" -f p="$PR_ID"
+
+# Verify: the PR should list the issue.
+gh api graphql -F n=<N> -f query='query($n:Int!){
+  repository(owner:"modelcontextprotocol",name:"inspector"){pullRequest(number:$n){
+    closingIssuesReferences(first:10){nodes{number}}}}}' \
+  --jq '[.data.repository.pullRequest.closingIssuesReferences.nodes[].number]'
+```
+
+The link does not change how the issue closes on a v2 merge; that is still
+step 9. `removeCloseIssueReferences` takes the same input and undoes the link.
+
+Move the card to **In Review**, then go straight to step 7.
+
+## 7. Run the Copilot review loop — immediately, every PR
+
+**Opening the PR is not the end of the task.** The next action, without being
+asked, is a Copilot review loop run to exhaustion: request a review, wait for
+the round to land (or for Copilot's session to end), answer it (step 8), and
+request again if anything was pushed. It stops only on one of the exits in 7c.
+
+### 7a. Request a round
 
 Only the GraphQL `requestReviews` mutation with the Copilot **bot id** works —
 REST, `gh pr edit --add-reviewer`, `userIds`, and `copilot-swe-agent` all fail or
@@ -239,17 +270,21 @@ gh api graphql -f query='
   }' -f pr="$PR_ID" -f bot='BOT_kgDOCnlnWA'
 ```
 
-Poll for the review with a `startswith` match — the review login carries a
-`[bot]` suffix. **Put that poll in one backgrounded loop that exits when the
-round lands, and wait for its notification** rather than re-fetching once per
-turn; a review is remote state the harness cannot observe, which is exactly the
-exception described in [Waiting on long-running
-work](../../../AGENTS.md#waiting-on-long-running-work) — and exactly where the
-poll belongs when one is needed.
+### 7b. Wait for it — review posted, or session ended
+
+A round ends one of two ways: Copilot **posts a review**, or its **pending
+request disappears without one** — it failed, or occasionally has nothing to
+say and posts nothing. Waiting only for the review hangs forever on the second
+case, so the wait watches both, plus a hard cap. **Put it in one backgrounded
+loop that exits when the round resolves, and wait for its notification** rather
+than re-fetching once per turn; a review is remote state the harness cannot
+observe, which is exactly the exception described in [Waiting on long-running
+work](../../../AGENTS.md#waiting-on-long-running-work).
 
 ```sh
 EXPECTED=1   # the review COUNT you are waiting to reach — see below
-while :; do
+DEADLINE=$(( $(date +%s) + 1500 ))   # 25 min; rounds normally land in 2–10
+count() {
   # Capture first, so a gh failure stops the loop instead of being swallowed by
   # a pipeline. --slurp cannot be combined with --jq, hence the separate jq.
   raw=$(gh api --paginate --slurp \
@@ -258,7 +293,21 @@ while :; do
   n=$(jq '[.[][] | select(.user.login | startswith("copilot-pull-request-reviewer"))] | length' <<<"$raw") || {
       echo "jq failed ($?) on an unexpected response shape" >&2; exit 1; }
   case $n in '' | *[!0-9]*) echo "not a count: '$n'" >&2; exit 1 ;; esac
-  [ "$n" -ge "$EXPECTED" ] && break
+}
+pending() {
+  p=$(gh api graphql -f query='{repository(owner:"modelcontextprotocol",name:"inspector"){pullRequest(number:<N>){reviewRequests(first:20){nodes{requestedReviewer{... on Bot{login} ... on User{login}}}}}}}' \
+    --jq '[.data.repository.pullRequest.reviewRequests.nodes[].requestedReviewer.login // empty | select(test("copilot";"i"))] | length') || {
+      echo "gh graphql failed ($?)" >&2; exit 1; }
+}
+while :; do
+  count; [ "$n" -ge "$EXPECTED" ] && { echo "ROUND=posted"; break; }
+  pending
+  if [ "$p" = 0 ]; then
+    sleep 30; count   # the request can clear a beat before the review is visible
+    [ "$n" -ge "$EXPECTED" ] && echo "ROUND=posted" || echo "ROUND=ended-without-review"
+    break
+  fi
+  [ "$(date +%s)" -ge "$DEADLINE" ] && { echo "ROUND=timed-out"; break; }
   sleep 30
 done
 ```
@@ -272,8 +321,37 @@ read as a count of `0`; and a `jq` failure on an unexpected shape leaves `n`
 empty, whereupon `[ "" -ge 1 ]` exits non-zero, `break` never fires, and the job
 sleeps and retries forever — the same unbounded wait, reached from the other
 end. A background task that can never succeed is worse than one that never
-started, because it looks like progress. Give the inline comments a further ~60s after the body lands; they
-arrive late (see step 8).
+started, because it looks like progress. On `ROUND=posted`, give the inline
+comments a further ~60s; they arrive late (see step 8).
+
+### 7c. Decide: another round, or stop
+
+Answer the round per step 8 first, then:
+
+| The round…                                                                  | Next                                                                                         |
+| --------------------------------------------------------------------------- | -------------------------------------------------------------------------------------------- |
+| had an in-scope finding you fixed and pushed                                | Request another round (7a), `EXPECTED` + 1.                                                   |
+| was clean — no inline comments, nothing in the body headline or `Suppressed comments` | **Stop.** One clean round is the end — never request a confirming round "just to be sure"; it spends Copilot tokens to re-review code nothing has changed. |
+| held only findings you declined as out of scope (see below)                 | **Stop.** Nothing changed, so another round only re-argues the same scope.                    |
+| `ended-without-review`                                                      | Request once more. Two in a row means Copilot's session on this PR has ended — stop.          |
+| `timed-out`                                                                 | **Stop and report the round as still pending.** The request is still open, so re-running `requestReviews` for the same bot is a no-op and starts nothing new. |
+
+"Clean" means all three channels are empty — inline comments, the body's
+headline sentence, and the `Suppressed comments` block. A zero-comment round
+can still name a real bug in the headline or the suppressed block — read all
+three before calling it clean.
+
+**Weigh every finding against the issue the PR closes.** Fix what is a defect
+_in what this PR added_. Decline, with a reason in the thread, anything that is
+pre-existing behavior, a new capability, or hardening beyond what the issue
+asks for — Copilot does not converge on its own, and every fix it talks you
+into beyond the issue is fresh surface for the next round, so accepting scope
+creep is what makes a review cycle protracted. If a declined finding is a real
+problem worth doing, file it with `/issue-create` and link it in the reply
+rather than growing the PR.
+
+When the loop stops, post a PR-level comment saying the review is closed and
+why (which exit fired), and report the same in your reply to the user.
 
 ## 8. Respond to the review
 

.claude/skills/pr-flow/evals/evals.json

@@ -1,4 +1,12 @@
 [
+  {
+    "prompt": "create a PR for #2463",
+    "expect": "pr-flow"
+  },
+  {
+    "prompt": "Create a PR for #2381.",
+    "expect": "pr-flow"
+  },
   {
     "prompt": "I've finished the fix for issue 2071. Take it through to a pull request.",
     "expect": "pr-flow"
@@ -19,6 +27,10 @@
     "prompt": "My PR is open and green. Walk me through getting it merged and closed out here.",
     "expect": "pr-flow"
   },
+  {
+    "prompt": "Open the PR for #2400, then keep getting Copilot to review it until it has nothing left to say.",
+    "expect": "pr-flow"
+  },
   {
     "prompt": "What does this regex match? /^[a-z]+$/",
     "expect": null

AGENTS.md

@@ -375,15 +375,17 @@ skills; the rules are here.
 - **`Done` means the work shipped.** Exactly two things earn a card a place in Done: its **PR merged**, or it is a **parent whose last sub-issue closed**. Anything else — duplicate, won't fix, not planned, obsolete, superseded — means nothing shipped, so the card is **deleted**. Done is read as the record of what a milestone actually delivered; a duplicate sitting there makes that record wrong in a way nobody can detect later. Deleting a card touches the board only — the issue keeps its labels and comments and stays searchable forever.
 - **When work begins**, create a feature branch and set Status to **In Progress**. **Branch names start with the target version segment** — `v2/fix/2071-oauth-resource-metadata`, `v1/fix/proxy-ssrf-pin` — matching the base branches themselves.
 - **When work is complete**, run `npm run format` then `npm run local:gate`, **sign off every commit** (`git commit -s` — the DCO check is a hard merge gate with no partial credit), open a PR against the matching base branch with **`Closes #<ISSUE_NUMBER>` as the body's first line**, and set Status to **In Review**.
+- **After opening a PR, run a Copilot review loop to exhaustion — unprompted.** Request a review, wait for the round to post _or_ for Copilot's session to end without one, answer every comment, and request again whenever a fix was pushed. Stop on the **first** clean round (no confirming round "just to be sure"), a round holding only out-of-scope findings, or two rounds in a row where Copilot's session ends without posting. **Weigh each finding against the issue the PR closes and decline scope expansion** — pre-existing behavior, new capabilities, and hardening the issue did not ask for — because that is what turns a review cycle into overbuilding. The recipe is the `pr-flow` skill, step 7.
 - **Attach screenshots as proof of functionality** for any web-UI or TUI change. Put them in a **`pr-screenshots/`** folder off the repo root — it is **gitignored**, so the images are staged for upload and never committed — and name them for what they show.
-- ⚠️ Closing keywords only auto-link and auto-close for PRs targeting the **default branch** (`main`). A v2 PR targets `v2/main`, so `Closes #N` there is only a cross-reference. **On merge, manually close the issue and move the card to Done.** Keep the line anyway, so the issues close if/when `v2/main` reaches `main`.
+- ⚠️ Closing keywords only auto-link and auto-close for PRs targeting the **default branch** (`main`). A v2 PR targets `v2/main`, so `Closes #N` there is only a cross-reference and the card shows no linked PR. **Link it explicitly** with the `addCloseIssueReferences` GraphQL mutation right after opening the PR (recipe in `pr-flow`, step 6). **On merge, manually close the issue and move the card to Done.** Keep the line anyway, so the issues close if/when `v2/main` reaches `main`.
 - **If new tasks are discovered during development, create issues** and add them to the board.
 
 ### Responding to Code Reviews
 
 When asked to respond to a code review of a PR:
 
 - it is not necessary to implement all suggestions
+- **judge each suggestion against the issue the PR closes.** Fix defects in what the PR added; decline, with a reason, suggestions that expand the PR beyond what the issue calls for, and file an issue for any that is worth doing on its own
 - you are free to implement suggestions in a different way, or to ignore one if there is a good reason
 - after making the changes, respond to each review comment with what was done (or why it was ignored)
 - **that response goes in the review comment's own thread — a rollup comment does not discharge it.** Each review comment is a discussion thread with its own resolve state, so a bullet posted elsewhere on the page cannot be connected back to the thread it answers: the thread stays open showing a finding and no reply, and the PR reads as though the review were ignored. Replying does not itself **resolve** a thread — that is a separate act and the reviewer's to make — but it is what makes resolving it defensible. Reply inline first, per comment; then post the PR-level summary **in addition**, because inline replies go hidden once the fix is pushed. A finding in the review's "Suppressed comments" block has no thread to reply into, so the summary is the only place it can be answered — that is the one exception. The `gh` calls are in the `pr-flow` skill, step 8.