From f504b89b5677cdc00be5e7817e01d591cd46d552 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sun, 6 Sep 2026 12:15:11 -0400 Subject: [PATCH 1/3] docs: reply to each review comment in its own thread, not in a rollup (#2254) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit pr-flow §8's "mirror each round at PR level" was being read as permission to skip the inline reply, so the rollup got posted and the per-thread replies never happened. A bullet elsewhere on the page cannot be connected back to the thread it answers: the thread stays open showing a finding and silence, and by round three matching bullets to comments is reconstruction rather than reading. - AGENTS.md, "Responding to Code Reviews": says where the response goes, which was the ambiguity being resolved the wrong way — inline in the comment's own thread, with the rollup in addition, never instead. - pr-flow §8 reorders the two so the primary is unmistakable, and keeps the reason the mirror exists (threads go outdated once the fix is pushed) as a rationale for it rather than an alternative to the inline reply. - Both name the exception: a finding in the "Suppressed comments" block has no comment id and so no thread to reply into, making the mirror its only home. - §8 gains the two mechanics, so the easy path is not the only documented one: the reply endpoint keyed by comment id, and fetching comments per review by review id, since the unpaginated /reviews listing hides later rounds behind your own replies. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_017nLLLTGc37hQiC8s3mNyhq Signed-off-by: cliffhall --- .claude/skills/pr-flow/SKILL.md | 33 ++++++++++++++++++++++++++++----- AGENTS.md | 1 + 2 files changed, 29 insertions(+), 5 deletions(-) diff --git a/.claude/skills/pr-flow/SKILL.md b/.claude/skills/pr-flow/SKILL.md index 1cc421f7..786b5c37 100644 --- a/.claude/skills/pr-flow/SKILL.md +++ b/.claude/skills/pr-flow/SKILL.md @@ -279,11 +279,34 @@ arrive late (see step 8). - It is **not** necessary to implement every suggestion. Implementing one a different way, or declining it with a reason, is fine. -- After making the changes, **respond to each comment** with what was done, or - why it was ignored. -- ⚠️ **Inline replies go hidden once the fix is pushed** (the threads become - outdated), so **mirror each round at PR level** as a summary comment, and always - read the "Suppressed comments" block. +- After making the changes, **reply to each review comment in its own thread** + with what was done, or why it was declined. That inline reply is the primary + response and it is not optional — each review comment is a discussion thread + with its own resolve state, and only a reply _in_ the thread can resolve it or + show a reviewer reading it that the finding was answered. + + ```sh + # Fetch the round's comments by REVIEW id — the unpaginated /reviews listing + # hides later rounds behind your own replies. + gh api repos/modelcontextprotocol/inspector/pulls//reviews//comments \ + --jq '.[]|"\(.id) \(.path):\(.line)\n\(.body)"' + + # Reply into one thread, keyed by the comment id from above. + gh api repos/modelcontextprotocol/inspector/pulls//comments//replies \ + -f body='Fixed in — …' + ``` + +- ⚠️ **Then mirror the round at PR level, in addition — never instead.** Inline + replies go hidden once the fix is pushed, because the threads become outdated, + so a summary comment is what keeps the round readable afterwards. It does + **not** discharge the per-comment replies: a rollup bullet cannot be connected + back to the thread it answers, so the thread stays open with a finding and + silence in it, and by round three matching bullets to comments is + reconstruction rather than reading. +- ⚠️ Always read the **"Suppressed comments"** block in the review body. Those + findings have no comment id, so they have no thread to reply into — the + PR-level mirror is the only place they can be answered, and it is the one case + where answering there is the whole response. - ⚠️ **Copilot's inline comments lag its review body.** The body's "generated N comments" count lands first; fetch by recency and reconcile. Repeated re-review silence means the session ended. diff --git a/AGENTS.md b/AGENTS.md index 2d8d3ffd..ca30281a 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -372,6 +372,7 @@ When asked to respond to a code review of a PR: - it is not necessary to implement all suggestions - 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. 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. ## Always test new or modified code From 5a1b5900cf1c7daa3ff76a9924f68f493db7d416 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sun, 6 Sep 2026 12:20:35 -0400 Subject: [PATCH 2/3] docs: a reply does not resolve a review thread MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Copilot review round 1 on #2273: the text said only a reply in the thread "can resolve it", which is wrong — resolution is a separate act (the Resolve conversation button, or the resolveReviewThread GraphQL mutation) and it is the reviewer's to make. Reading it as satisfied by the documented REST reply is exactly the wrong conclusion for a PR whose whole point is what a reply does. State what the reply actually buys: it is the only thing a reviewer reading that thread sees, and it is what makes resolving the thread defensible. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_017nLLLTGc37hQiC8s3mNyhq Signed-off-by: cliffhall --- .claude/skills/pr-flow/SKILL.md | 7 +++++-- AGENTS.md | 2 +- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/.claude/skills/pr-flow/SKILL.md b/.claude/skills/pr-flow/SKILL.md index 786b5c37..d7d1a06e 100644 --- a/.claude/skills/pr-flow/SKILL.md +++ b/.claude/skills/pr-flow/SKILL.md @@ -282,8 +282,11 @@ arrive late (see step 8). - After making the changes, **reply to each review comment in its own thread** with what was done, or why it was declined. That inline reply is the primary response and it is not optional — each review comment is a discussion thread - with its own resolve state, and only a reply _in_ the thread can resolve it or - show a reviewer reading it that the finding was answered. + with its own resolve state, and a reply _in_ the thread is the only thing a + reviewer reading that thread sees. It does **not** resolve the thread: + resolving is a separate act — the "Resolve conversation" button, or the + `resolveReviewThread` GraphQL mutation — and it is the reviewer's to make. The + reply is what makes resolving it defensible. ```sh # Fetch the round's comments by REVIEW id — the unpaginated /reviews listing diff --git a/AGENTS.md b/AGENTS.md index ca30281a..a416da3f 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -372,7 +372,7 @@ When asked to respond to a code review of a PR: - it is not necessary to implement all suggestions - 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. 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. +- **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. ## Always test new or modified code From ed5e98b643c11531deee4d9764a5ecb830390a92 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sun, 6 Sep 2026 12:28:01 -0400 Subject: [PATCH 3/3] docs: paginate the review-comments fetch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Copilot review round 2 on #2273: the list endpoint pages at 30, so a large review round would silently drop later comments — and a workflow whose whole point is replying to every comment cannot be documented with a command that returns only the first thirty. --paginate combines with --jq fine (only --slurp does not), verified against a live review. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_017nLLLTGc37hQiC8s3mNyhq Signed-off-by: cliffhall --- .claude/skills/pr-flow/SKILL.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/.claude/skills/pr-flow/SKILL.md b/.claude/skills/pr-flow/SKILL.md index d7d1a06e..08947b67 100644 --- a/.claude/skills/pr-flow/SKILL.md +++ b/.claude/skills/pr-flow/SKILL.md @@ -291,7 +291,9 @@ arrive late (see step 8). ```sh # Fetch the round's comments by REVIEW id — the unpaginated /reviews listing # hides later rounds behind your own replies. - gh api repos/modelcontextprotocol/inspector/pulls//reviews//comments \ + # --paginate: this endpoint returns 30 per page, and a round you only half + # fetch is a round you only half answer. + gh api --paginate repos/modelcontextprotocol/inspector/pulls//reviews//comments \ --jq '.[]|"\(.id) \(.path):\(.line)\n\(.body)"' # Reply into one thread, keyed by the comment id from above.