From 1f957ffe015970a04e91593eea8859b46e09df7d Mon Sep 17 00:00:00 2001 From: cliffhall Date: Wed, 23 Sep 2026 20:10:20 -0400 Subject: [PATCH 1/3] docs(pr-flow): make an exhaustive Copilot review loop the step after opening a PR (#2464) AGENTS.md now requires the loop, unprompted, after every PR, and requires findings to be weighed against the issue with scope expansion declined. pr-flow step 7 becomes the loop: request, wait for a posted review or for Copilot's pending request to lapse without one (plus a 25-minute cap), then a stop/continue table. The first clean round is the stopping point; no confirming round. The description leads with "create a PR for an issue" and eval cases pin that prompt shape. Co-Authored-By: Claude Opus 5.5 (1M context) Signed-off-by: cliffhall --- .claude/skills/pr-flow/SKILL.md | 81 ++++++++++++++++++++----- .claude/skills/pr-flow/evals/evals.json | 12 ++++ AGENTS.md | 2 + 3 files changed, 81 insertions(+), 14 deletions(-) diff --git a/.claude/skills/pr-flow/SKILL.md b/.claude/skills/pr-flow/SKILL.md index 08947b67e..86eb792d3 100644 --- a/.claude/skills/pr-flow/SKILL.md +++ b/.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 --- @@ -221,9 +221,16 @@ 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**. +Move the card to **In Review**, then go straight to step 7. -## 7. Request a Copilot review +## 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 +246,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 +269,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:){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 +297,36 @@ 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` or `timed-out` | Request once more. Two in a row means Copilot's session on this PR has ended — stop. | + +"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 diff --git a/.claude/skills/pr-flow/evals/evals.json b/.claude/skills/pr-flow/evals/evals.json index d8d87f3a4..e2685d464 100644 --- a/.claude/skills/pr-flow/evals/evals.json +++ b/.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 diff --git a/AGENTS.md b/AGENTS.md index 1c0b7d906..814ef0780 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -375,6 +375,7 @@ 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 #` 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`. - **If new tasks are discovered during development, create issues** and add them to the board. @@ -384,6 +385,7 @@ skills; the rules are here. 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. From 3be44dce4fcf720ad8f0eac276491782fd9dd500 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Wed, 23 Sep 2026 20:22:16 -0400 Subject: [PATCH 2/3] docs(pr-flow): stop on a timed-out round instead of re-requesting (#2466 review) A timeout fires only while the request is still pending, and requestReviews with union:true is a no-op for an already-requested bot, so a retry would start nothing new. Co-Authored-By: Claude Opus 5.5 (1M context) Signed-off-by: cliffhall --- .claude/skills/pr-flow/SKILL.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/.claude/skills/pr-flow/SKILL.md b/.claude/skills/pr-flow/SKILL.md index 86eb792d3..721e1b040 100644 --- a/.claude/skills/pr-flow/SKILL.md +++ b/.claude/skills/pr-flow/SKILL.md @@ -309,7 +309,8 @@ Answer the round per step 8 first, then: | 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` or `timed-out` | Request once more. Two in a row means Copilot's session on this PR has ended — stop. | +| `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 From 26231673a2b8641e9553bbc1bbb0e0459f5a9bfd Mon Sep 17 00:00:00 2001 From: cliffhall Date: Wed, 23 Sep 2026 20:23:21 -0400 Subject: [PATCH 3/3] docs(pr-flow): link a v2 PR to its issue with addCloseIssueReferences (#2464) Closing keywords do not link a PR whose base is not the default branch, so a v2 card showed no linked PR. The GraphQL mutation adds a manual closing reference, which populates the card's Linked pull requests field. Verified on #2466/#2464 with the manual link removed first. Co-Authored-By: Claude Opus 5.5 (1M context) Signed-off-by: cliffhall --- .claude/skills/pr-flow/SKILL.md | 28 ++++++++++++++++++++++++++-- AGENTS.md | 2 +- 2 files changed, 27 insertions(+), 3 deletions(-) diff --git a/.claude/skills/pr-flow/SKILL.md b/.claude/skills/pr-flow/SKILL.md index 721e1b040..77e1ebf32 100644 --- a/.claude/skills/pr-flow/SKILL.md +++ b/.claude/skills/pr-flow/SKILL.md @@ -218,8 +218,32 @@ 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. + +**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. + +```sh +ISSUE_ID=$(gh api graphql -F n= -f query='query($n:Int!){ + repository(owner:"modelcontextprotocol",name:"inspector"){issue(number:$n){id}}}' \ + --jq .data.repository.issue.id) +PR_ID=$(gh pr view --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= -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. diff --git a/AGENTS.md b/AGENTS.md index 814ef0780..f599795f4 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -377,7 +377,7 @@ skills; the rules are here. - **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 #` 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