From 98472c0208a46d1fdd791b241d9fecd2c6e099de Mon Sep 17 00:00:00 2001 From: Chao Wang <26245345+ChaoWao@users.noreply.github.com> Date: Sun, 26 Jul 2026 20:02:05 -0700 Subject: [PATCH] Fix: read all three PR comment surfaces in the fix-pr skill fetch-comments queried only reviewThreads, so review summary bodies and PR conversation comments were never fetched. A CHANGES_REQUESTED rationale or a plain "please rebase" comment could therefore sit unread while the fix-pr loop reported the PR clean. - Fetch reviewThreads, reviews[].body, and issue comments in one query - Track handled review bodies and conversation comments in a per-PR ledger, since neither surface carries an isResolved bit and both would otherwise re-present as unaddressed every iteration - Filter bot status/walkthrough noise, and dedup summaries that only restate their own inline threads - Answer non-thread feedback with a batched PR conversation comment; resolveReviewThread stays thread-only, as it rejects review and issue-comment node IDs - Require no unhandled feedback on any surface before the loop exits --- .claude/lib/github/README.md | 4 +- .claude/lib/github/fetch-comments.md | 103 +++++++++++++++++--- .claude/lib/github/reply-and-resolve.md | 41 +++++++- .claude/skills/fix-pr/SKILL.md | 124 ++++++++++++++++-------- 4 files changed, 213 insertions(+), 59 deletions(-) diff --git a/.claude/lib/github/README.md b/.claude/lib/github/README.md index 4588ee9e8f..a8cffa773e 100644 --- a/.claude/lib/github/README.md +++ b/.claude/lib/github/README.md @@ -10,8 +10,8 @@ Reusable procedures for GitHub PR workflows. | [lookup-pr](lookup-pr.md) | Find PR by number, branch, or list all | All | | [detect-permission](detect-permission.md) | Check push access to PR | fix-pr | | [commit-and-push](commit-and-push.md) | Squash commits, rebase, and push | github-pr, fix-pr | -| [fetch-comments](fetch-comments.md) | Get unresolved PR review comments | fix-pr | -| [reply-and-resolve](reply-and-resolve.md) | Reply to and resolve review threads | fix-pr | +| [fetch-comments](fetch-comments.md) | Get all PR feedback — inline threads, review bodies, conversation comments | fix-pr | +| [reply-and-resolve](reply-and-resolve.md) | Reply to each feedback surface; resolve review threads | fix-pr | | [branch-naming](branch-naming.md) | Generate branch name from commit | github-pr | | [common-issues](common-issues.md) | Troubleshooting reference | All | diff --git a/.claude/lib/github/fetch-comments.md b/.claude/lib/github/fetch-comments.md index 1e3477af31..f93c8b7f3c 100644 --- a/.claude/lib/github/fetch-comments.md +++ b/.claude/lib/github/fetch-comments.md @@ -1,9 +1,19 @@ -# Fetch Unresolved PR Comments +# Fetch PR Feedback (all three comment surfaces) -Fetches all unresolved review threads for a given PR. +A PR carries feedback on **three separate surfaces**. Fetching only the inline +review threads silently drops the other two — which is where "please rebase", +"this whole approach is wrong", and most bot summaries actually land. + +| # | Surface | GraphQL field | Resolvable? | Reply with | +| - | ------- | ------------- | ----------- | ---------- | +| A | Inline review threads (anchored to a file/line) | `reviewThreads.nodes` | Yes (`resolveReviewThread`) | thread reply on the comment's `databaseId` | +| B | Review summary bodies (text submitted with APPROVE / REQUEST_CHANGES / COMMENT) | `reviews.nodes[].body` | No | a PR conversation comment | +| C | PR conversation comments (issue comments, incl. `@`-mentions and most bot posts) | `comments.nodes` | No | a PR conversation comment | **Important:** Inline values directly into the GraphQL query string. Do NOT use `-f`/`-F` flags with GraphQL `$variables` — bash mangles the `$` signs. See [common-issues](./common-issues.md) for details. +## One query for all three + ```bash gh api graphql -f query=' query { @@ -13,6 +23,7 @@ query { nodes { id isResolved + isOutdated comments(first: 50) { nodes { id @@ -28,26 +39,94 @@ query { } } } + reviews(first: 100) { + nodes { + id + databaseId + state + body + author { login } + submittedAt + } + } + comments(first: 100) { + nodes { + id + databaseId + body + author { login } + createdAt + } + } } } -}' +}' > /tmp/pr-feedback.json ``` Replace `OWNER`, `REPO`, `NUMBER` with actual values (e.g., `"hw-native-sys"`, `"simpler"`, `276`). -Use `--jq` to filter unresolved threads: +Split the surfaces out of the saved file (`jq` on a *file* avoids the `gh --jq` +`$`-quoting pitfalls): ```bash -gh api graphql -f query='...' \ - --jq '[.data.repository.pullRequest.reviewThreads.nodes[] | select(.isResolved == false)]' +P='.data.repository.pullRequest' + +# A — unresolved inline threads +jq "[${P}.reviewThreads.nodes[] | select(.isResolved == false)]" /tmp/pr-feedback.json + +# B — review bodies that actually say something (empty body = drive-by approve) +jq "[${P}.reviews.nodes[] | select(.body != \"\")]" /tmp/pr-feedback.json + +# C — conversation comments +jq "${P}.comments.nodes" /tmp/pr-feedback.json ``` -**Limits:** +**Limits / pagination:** each connection is capped at `first: 100` (and +`comments(first: 50)` per thread). Add `pageInfo { hasNextPage endCursor }` and +re-query with `after: ""` on any connection reporting +`hasNextPage: true`. + +## Surfaces B and C have no resolved bit — track them yourself + +Only surface A carries `isResolved`. A review body or conversation comment stays +in the API response forever, so re-fetching it every iteration makes the same +feedback look perpetually unaddressed. Keep a handled-ID ledger per PR: + +```bash +LEDGER="/tmp/pr--handled.txt" # one comment/review node id per line +touch "$LEDGER" + +# Filter out what was handled in an earlier iteration +jq -r "${P}.comments.nodes[].id" /tmp/pr-feedback.json | grep -Fxv -f "$LEDGER" + +# After replying to (or consciously skipping) one, record it +echo "$NODE_ID" >> "$LEDGER" +``` + +`createdAt` / `submittedAt` is the fallback when the ledger is missing: anything +predating the current HEAD commit's push was, in practice, already seen. + +## Filtering bot noise (surface C) + +Bots post status and walkthrough comments that carry no request. Treat a +conversation comment as **non-actionable** when its body matches any of: + +- `` +- `` without an `Actionable comments posted: ` where `n > 0` +- `Review skipped` / `Pre-merge checks` status blocks +- A pure CI/deploy status echo + +Everything else on surface C is real feedback and must be classified, including +short human one-liners (`please rebase onto main`, `can you split this?`) that +never appear as an inline thread. -- `reviewThreads(first: 100)`: Fetches up to 100 threads. -- `comments(first: 50)`: Fetches up to 50 comments per thread. +A bot's *actionable* findings usually arrive as surface A threads, with B/C +holding only the summary — so **dedup B/C against A** before presenting: if a +review body just enumerates its own inline threads, address the threads and +treat the body as informational. -Output: JSON array of unresolved threads. Each thread has: +Output per surface: -- `id` — GraphQL node ID (for resolving via mutation) -- `comments.nodes[].databaseId` — REST API ID (for replying) +- A — `id` (GraphQL node ID, for `resolveReviewThread`), `comments.nodes[].databaseId` (REST ID, for threaded replies), `path`/`line` for context +- B — `state` (`CHANGES_REQUESTED` outranks `COMMENTED`), `body`, `author` +- C — `databaseId`, `body`, `author`, `createdAt` diff --git a/.claude/lib/github/reply-and-resolve.md b/.claude/lib/github/reply-and-resolve.md index c040f39d6a..cf2dadf5cd 100644 --- a/.claude/lib/github/reply-and-resolve.md +++ b/.claude/lib/github/reply-and-resolve.md @@ -1,23 +1,29 @@ # Reply and Resolve -For each comment, reply then resolve the thread. Both steps are required. +How to answer each of the three feedback surfaces from +[fetch-comments](./fetch-comments.md). Every piece of feedback gets a reply; +only surface A can additionally be resolved. -## Step 1: Reply to the comment +## Surface A — inline review threads + +Both steps are required. + +### Step 1: Reply to the comment ```bash gh api "repos/${PR_REPO_OWNER}/${PR_REPO_NAME}/pulls/${PR_NUMBER}/comments/${COMMENT_DATABASE_ID}/replies" \ - -f body='Fixed in — description' + -f body='Fixed — description' ``` **Important:** Always use single quotes for `-f body='...'` to avoid bash history expansion issues with `!` and other special characters. **Response templates:** -- Fixed: `'Fixed in — description'` +- Fixed: `'Fixed — description'` - Skip: `'Current code follows .claude/rules/'` - Acknowledged: `'Acknowledged, thank you!'` -## Step 2: Resolve the thread +### Step 2: Resolve the thread Use the thread `id` (the GraphQL node ID from [fetch-comments](./fetch-comments.md), NOT the `databaseId`): @@ -34,3 +40,28 @@ mutation ResolveThread($threadId: ID!) { `$THREAD_ID` is the `id` field from the `reviewThreads.nodes[]` returned by [fetch-comments](./fetch-comments.md). **Both steps are mandatory.** Do not skip the resolve step. + +## Surfaces B and C — review bodies and conversation comments + +These have **no resolve mutation** — `resolveReviewThread` accepts only a +review-thread node ID and errors on a review or issue-comment ID. Answer them +with a PR conversation comment and record the ID in the ledger. + +```bash +gh pr comment "$PR_NUMBER" --body "@${AUTHOR} Fixed — description" +echo "$NODE_ID" >> "/tmp/pr-${PR_NUMBER}-handled.txt" +``` + +- **Address the author by `@login`** — a conversation comment is not threaded, so without the mention it is unclear which feedback it answers. Quote the point being answered when several are outstanding. +- **Batch one reply per iteration.** Answering five conversation comments with five separate posts spams the PR; one comment with a short bullet per point is the norm. +- **Recording the ID is what makes the reply stick.** Skip it and the next iteration re-presents the same feedback as unaddressed. +- Feedback that is purely informational (bot walkthrough, "nice work") needs no reply — just record its ID. + +Template for a batched reply: + +```text +@reviewer Addressed: +- Rebased onto main. +- Null check added on the predicate path. +- Kept the existing enum style — follows .claude/rules/codestyle.md §2. +``` diff --git a/.claude/skills/fix-pr/SKILL.md b/.claude/skills/fix-pr/SKILL.md index 2f35fcdf0b..8173e6be4e 100644 --- a/.claude/skills/fix-pr/SKILL.md +++ b/.claude/skills/fix-pr/SKILL.md @@ -1,6 +1,6 @@ --- name: fix-pr -description: Fix GitHub PR issues — address review comments and resolve CI failures in a loop until the PR is fully clean. Fetches CI errors online and triages review feedback. Use when fixing PR problems, addressing review comments, or resolving CI failures. +description: Fix GitHub PR issues — address all PR feedback (inline review threads, review summary bodies, and conversation comments) and resolve CI failures in a loop until the PR is fully clean. Fetches CI errors online and triages review feedback. Use when fixing PR problems, addressing review comments, or resolving CI failures. --- # Fix PR Workflow @@ -15,7 +15,7 @@ Create tasks to track progress through this workflow: 2. Detect & classify issues 3. Get user confirmation 4. Fix issues & push (fold into one commit — amend/squash, never append) -5. Resolve comment threads +5. Reply to all feedback; resolve the threads that support it 6. Re-check (loop until clean) ## Input @@ -52,40 +52,42 @@ Validate PR state: OPEN (continue), CLOSED (warn), MERGED (exit). ### Step 2: Detect Issues (run in parallel) -**A) Review comments:** +**A) All PR feedback — three surfaces, not just inline threads:** -Run [fetch-comments](../../lib/github/fetch-comments.md). +Run [fetch-comments](../../lib/github/fetch-comments.md) and fetch **all three** +surfaces in one query: + +| Surface | What it is | Why it is easy to miss | +| ------- | ---------- | ---------------------- | +| **A** inline review threads | `reviewThreads.nodes` — anchored to file/line | — (the only one with `isResolved`) | +| **B** review summary bodies | `reviews.nodes[].body` — text submitted with CHANGES_REQUESTED / COMMENTED / APPROVED | Not a thread; never appears in a `reviewThreads` query | +| **C** PR conversation comments | `comments.nodes` — issue comments, `@`-mentions, most bot posts | Not a thread; holds "please rebase", scope objections, maintainer instructions | + +**Fetching only surface A is the classic failure of this workflow** — a +maintainer's "this approach is wrong, see my comment above" lives on B or C and +gets silently skipped while the PR is declared clean. ```bash OWNER=$(gh repo view --json owner -q '.owner.login') NAME=$(gh repo view --json name -q '.name') +P='.data.repository.pullRequest' + +# Save the combined query from fetch-comments.md to a file, then jq the file +# (see pitfalls below) +gh api graphql -f query='...' > /tmp/pr-feedback.json + +# Count each surface +jq "[${P}.reviewThreads.nodes[] | select(.isResolved == false)] | length" /tmp/pr-feedback.json +jq "[${P}.reviews.nodes[] | select(.body != \"\")] | length" /tmp/pr-feedback.json +jq "${P}.comments.nodes | length" /tmp/pr-feedback.json -# Fetch review threads — save to file, then grep (see pitfalls below) -gh api graphql \ - -F owner="$OWNER" -F name="$NAME" -F number= \ - -f query=' -query($owner: String!, $name: String!, $number: Int!) { - repository(owner: $owner, name: $name) { - pullRequest(number: $number) { - reviewThreads(first: 100) { - nodes { - id isResolved - comments(last: 1) { - nodes { id databaseId body author { login } path line } - } - } - pageInfo { hasNextPage endCursor } - } - } - } -}' > /tmp/threads.json - -# Count unresolved threads (use whitespace-tolerant pattern) -grep -Ec '"isResolved":[[:space:]]*false' /tmp/threads.json - -# Paginate: if hasNextPage is true, re-run with -F cursor="" until done +# Paginate: if any connection's hasNextPage is true, re-query with after: "" ``` +Surfaces B and C carry no `isResolved` bit — filter them against the +handled-ID ledger (`/tmp/pr--handled.txt`, see fetch-comments) so +already-answered feedback is not re-presented every iteration. + **B) CI status:** ```bash @@ -99,9 +101,11 @@ gh pr checks - Also see [common-issues](../../lib/github/common-issues.md) for `gh api` quoting pitfalls (use single quotes for `--jq` and `-f body=`) - Use `grep -c` for simple counts; save to a temp file first if complex parsing is needed -Present: "**Iteration N** — Found X unresolved comments and Y failed/pending checks." +Present: "**Iteration N** — Found X unresolved review threads, Y unhandled review bodies / conversation comments, and Z failed/pending checks." -**Exit condition:** All checks green AND no unresolved comments → done. Pending checks do NOT count as clean. +**Exit condition:** All checks green AND no unresolved threads AND no unhandled +review bodies or conversation comments → done. Pending checks do NOT count as +clean, and an unanswered conversation comment does NOT count as clean. ### Step 3: Detect Permission @@ -109,18 +113,26 @@ Run [detect-permission](../../lib/github/detect-permission.md) to determine push ### Step 4: Classify Issues -**Review comments** — filter `isResolved: false`, classify: +**Feedback** — take unresolved surface-A threads plus unhandled surface-B/C +items, and classify **every** item by content, regardless of which surface it +arrived on: | Category | Description | Examples | | -------- | ----------- | -------- | -| **A: Actionable** | Code changes required | Bugs, missing validation, race conditions, incorrect logic | +| **A: Actionable** | Code changes required | Bugs, missing validation, race conditions, incorrect logic, "rebase onto main", "split this PR" | | **B: Discussable** | May skip if follows `.claude/rules/` | Style preferences, premature optimizations | -| **C: Informational** | Resolve without changes | Acknowledgments, "optional" suggestions | +| **C: Informational** | Answer without changes | Acknowledgments, "optional" suggestions, bot walkthroughs | Treat bot reviewers (CodeRabbit, Copilot, Gemini) same as human — classify by content. For Category B, explain why code may already comply with `.claude/rules/`. +Surface-specific handling before classifying: + +- **B (review bodies):** a `CHANGES_REQUESTED` body is actionable by default — it blocks merge even when every inline thread is resolved. A body that only enumerates its own inline threads is informational; address the threads instead. +- **C (conversation comments):** drop bot status/walkthrough noise per the filter list in [fetch-comments](../../lib/github/fetch-comments.md); classify everything else. Short human one-liners here are frequently the most consequential feedback on the PR. +- **Dedup across surfaces** — the same point raised inline and repeated in a summary is one item, addressed once. + **CI failures:** ```bash @@ -151,12 +163,19 @@ For large logs: `gh run view --job --log-failed 2>&1 | grep -E "error:| Present ALL issues in a numbered list: +Label each item with its surface so the user can see nothing was dropped: + ```text -Review Comments: +Inline review threads: 1. [A] src/foo.cpp:42 — Missing null check (reviewer: alice) 2. [B] src/bar.py:15 — Style suggestion (reviewer: coderabbitai) +Review bodies: + 3. [A] CHANGES_REQUESTED — "split the ring resize out of this PR" (reviewer: bob) +Conversation comments: + 4. [A] "please rebase onto main, base moved" (reviewer: alice) + 5. [C] CodeRabbit walkthrough — no action CI Failures: - 3. [CI] build — error: 'Foo' is not a member of 'pto2' + 6. [CI] build — error: 'Foo' is not a member of 'pto2' ``` Ask which to address/skip: @@ -244,9 +263,12 @@ missing null check on the predicate. Update the body to note the guard, keep the `feat(runtime): add predicated dispatch` subject — not `fix(pr): address comments`, not the untouched original. -### Step 7: Reply and Resolve Comment Threads +### Step 7: Reply and Resolve -For each comment, **both steps are mandatory** (see [reply-and-resolve](../../lib/github/reply-and-resolve.md)): +Every item presented in Step 5 gets an answer. How depends on its surface (see +[reply-and-resolve](../../lib/github/reply-and-resolve.md)). + +**Surface A — inline review threads.** Both steps are mandatory: 1. **Reply** using the comment's `databaseId`: @@ -266,6 +288,22 @@ For each comment, **both steps are mandatory** (see [reply-and-resolve](../../li **Important:** The thread `id` comes from `reviewThreads.nodes[].id` in the fetch-comments GraphQL response. Each thread contains comments — use the thread's `id` to resolve, and the comment's `databaseId` to reply. +**Surfaces B and C — review bodies and conversation comments.** There is no +resolve mutation for these (`resolveReviewThread` takes only a review-thread +node ID). Reply with a single batched PR conversation comment, then record each +handled ID in the ledger: + +```bash +gh pr comment "$PR_NUMBER" --body "@alice Addressed: +- Rebased onto main. +- Null check added on the predicate path." + +echo "$NODE_ID" >> "/tmp/pr-${PR_NUMBER}-handled.txt" # one line per item, incl. skipped/informational +``` + +Recording the ID is what makes the answer stick — skip it and Step 2 re-presents +the same feedback next iteration and the loop never converges. + Reply templates: - **Fixed** → "Fixed — description of change" (do **not** cite a commit SHA: the PR commit is amended/squashed and force-pushed each iteration, so any SHA you quote is immediately stale) @@ -304,15 +342,21 @@ Then loop back to Step 2. ## Checklist - [ ] PR matched and validated -- [ ] Review comments and CI status fetched -- [ ] ALL issues presented to user for selection +- [ ] **All three feedback surfaces fetched** — inline threads, review bodies, conversation comments — plus CI status +- [ ] ALL issues presented to user for selection, labelled by surface - [ ] Fixes folded into the PR (amended/squashed — **no appended `fix(pr)` commit**) - [ ] Verified `git rev-list HEAD --not "$BASE_REF" --count` == 1 before pushing - [ ] Changes force-pushed with `--force-with-lease` (single commit) -- [ ] Review comment threads replied to and resolved +- [ ] Inline threads replied to **and** resolved +- [ ] Review bodies / conversation comments replied to and recorded in the handled-ID ledger - [ ] Waited for CI/reviews and re-checked - [ ] Loop exited: all clean OR max iterations reached ## Remember **Not all comments require code changes.** Evaluate against `.claude/rules/` first. When in doubt, consult user. + +**Feedback is not only inline review threads.** A review summary body or a plain +conversation comment carries no `isResolved` flag and no file anchor, but it can +be the most consequential thing on the PR. Fetch all three surfaces every +iteration, or the loop will report "clean" over unread feedback.