From 761dfe90bccee00af36e8643365fca5392ec7653 Mon Sep 17 00:00:00 2001 From: Jean-Baptiste Watenberg Date: Fri, 27 Mar 2026 15:27:14 +0100 Subject: [PATCH 1/2] improvement(review-pr): add thread management to review skill MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds Step 2 to the review skill covering all existing Claude Code threads before the diff analysis runs. This fixes the three problems observed on active PRs (e.g. scality/metalk8s#4820: 15 review submissions, 14 inline comments, many duplicates from claude[bot]): 1. Duplicate comments: SKIP SET built from active threads prevents re-posting the same issue at the same path+line on every synchronize event. 2. Stale outdated threads: threads whose code has changed are auto-resolved via resolveReviewThread GraphQL mutation. 3. Human replies ignored: threads are classified by conversation history: - no_human_reply → skip set only - author_asked_question → Claude answers inline via in_reply_to - justified → accepted silently (substantive technical reasoning) - unjustified_dismissal → challenge reply asking for reasoning; stricter language for security/correctness issues - fix_claimed_but_code_unchanged → reply noting issue persists Dedup uses {path, line, body_excerpt} comparison so a genuinely different issue at the same path+line can still be raised. Pagination handles PRs with more than 100 review threads. Aligned with scality/agent-hub#26 (central workflow) and ring#6462. Co-Authored-By: Claude Sonnet 4.6 (1M context) --- .claude/skills/review-pr/SKILL.md | 54 ++++++++++++++++++++++++++++--- 1 file changed, 49 insertions(+), 5 deletions(-) diff --git a/.claude/skills/review-pr/SKILL.md b/.claude/skills/review-pr/SKILL.md index afabacf258..12be291f46 100644 --- a/.claude/skills/review-pr/SKILL.md +++ b/.claude/skills/review-pr/SKILL.md @@ -32,9 +32,50 @@ gh pr view --repo --json title,body,headRefOid,author,file gh pr diff --repo ``` -2. **Read changed files** to understand the full context around each change (not just the diff hunks). +2. **Manage existing Claude Code threads** (run before reviewing the diff): -3. **Analyze the changes** against these criteria: + Fetch all review threads with full pagination — loop until `hasNextPage` is false, merging nodes across pages: + + ```bash + gh api graphql -f query='{repository(owner:"",name:""){pullRequest(number:){reviewThreads(first:100){pageInfo{hasNextPage endCursor}nodes{id isResolved isOutdated resolvedBy{login}comments(first:20){nodes{databaseId author{login}body path line}}}}}}}' + ``` + + For pagination add `after:""` to `reviewThreads(first:100, after:"")` and repeat until `hasNextPage` is false. + + For each thread whose **first comment** contains `— Claude Code`: + + **OUTDATED** (`isOutdated=true`, `isResolved=false`) → resolve silently, then skip to next thread: + ```bash + gh api graphql -f query='mutation{resolveReviewThread(input:{threadId:""}){thread{isResolved}}}' + ``` + + **ACTIVE** — read ALL comments, find human replies (`author.login != "claude[bot]"`), classify: + + - **`no_human_reply`** — no human comments, not resolved → add `{path, line, body_excerpt}` to **SKIP SET**. No further action. + + - **`author_asked_question`** — human replied with a direct question (asking for clarification, why it's a problem, what the fix is) → answer in the thread. Add to SKIP SET. + ```bash + gh api -X POST -H "Accept: application/vnd.github+json" "repos//pulls//comments" -f body="

— Claude Code" -F in_reply_to= -f commit_id="" + ``` + + - **`justified`** — human reply contains substantive technical reasoning (why issue isn't applicable, acknowledged trade-off, relevant constraint) → accept silently. Add to SKIP SET. Optionally resolve thread if still open. + + - **`unjustified_dismissal`** — thread resolved by human with no reply, OR reply is dismissive with no technical reasoning (`"done"`, `"ok"`, `"wontfix"` with nothing substantive) AND flagged code still unchanged in new diff → reply in thread (do NOT post new top-level comment). Add to SKIP SET. + ```bash + gh api -X POST -H "Accept: application/vnd.github+json" "repos//pulls//comments" -f body="Issue still present in latest commit. Could you share your reasoning or clarify how it's been mitigated?

— Claude Code" -F in_reply_to= -f commit_id="" + ``` + For security/correctness issues add: `"This is a security/correctness concern — a brief explanation helps reviewers and future contributors."` Scale to severity: minor issues may be accepted silently. + + - **`fix_claimed_but_code_unchanged`** — human claimed fix but code unchanged in new diff → reply in thread. Add to SKIP SET. + ```bash + gh api -X POST -H "Accept: application/vnd.github+json" "repos//pulls//comments" -f body="Flagged code still appears at this location in the latest commit. Could you double-check, or clarify if the fix is elsewhere?

— Claude Code" -F in_reply_to= -f commit_id="" + ``` + + All replies: collaborative not adversarial. 1-2 sentences. Name the specific construct. + +3. **Read changed files** to understand the full context around each change (not just the diff hunks). + +4. **Analyze the changes** against these criteria: | Area | What to check | |------|---------------| @@ -55,13 +96,16 @@ gh pr diff --repo | Test coverage | New logic paths have corresponding unit tests; changed functions have updated tests; no test files deleted without replacement, no orphan test that would be never run | | Test impact | Changes to shared code (`map.jinja`, `constants.py`, CRD types, API types) are reflected in dependent tests across all affected layers | | BDD / integration | New user-facing behavior has matching behave scenarios in `tests/`; modified Salt orchestrations have updated integration steps | -4. **Deliver your review:** + +5. **Deliver your review:** ### If CI mode: post to GitHub #### Part A: Inline file comments -For each specific issue, post a comment on the exact file and line: +For each issue found, **check the SKIP SET from step 2 first**. The SKIP SET contains `{path, line, body_excerpt}` entries. If an entry exists with the same `path` and `line` AND the `body_excerpt` covers the same root issue → skip. A genuinely different issue at the same path+line may still be posted. + +Otherwise post a comment on the exact file and line: ```bash gh api -X POST -H "Accept: application/vnd.github+json" "repos//pulls//comments" -f body="Your comment

— Claude Code" -f path="path/to/file" -F line= -f side="RIGHT" -f commit_id="" @@ -104,7 +148,7 @@ Do not describe or summarize the PR. For each issue, state the problem on one li - ``` -If no issues: just say "LGTM". End with: `Review by Claude Code` +If no new issues and no thread replies needed: just say "LGTM". End with: `Review by Claude Code` ### If local mode: output the review as text From 2357b0e225fc980f7cad1752e8b1ea1d7b2126b5 Mon Sep 17 00:00:00 2001 From: Jean-Baptiste Watenberg Date: Fri, 27 Mar 2026 15:44:12 +0100 Subject: [PATCH 2/2] fix(review-pr): correct OR/AND precedence in unjustified_dismissal condition As written, the condition read: (resolved with no reply) OR (dismissive reply AND code unchanged). This meant if someone fixed the code and resolved the thread without commenting, Claude would still challenge them. Fix: add parentheses so the code-unchanged guard applies to both branches: (resolved with no reply OR dismissive reply) AND code unchanged in new diff Caught by claude[bot] review on scality/agent-hub#26. Co-Authored-By: Claude Sonnet 4.6 (1M context) --- .claude/skills/review-pr/SKILL.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.claude/skills/review-pr/SKILL.md b/.claude/skills/review-pr/SKILL.md index 12be291f46..1a93a1cb9d 100644 --- a/.claude/skills/review-pr/SKILL.md +++ b/.claude/skills/review-pr/SKILL.md @@ -60,7 +60,7 @@ gh pr diff --repo - **`justified`** — human reply contains substantive technical reasoning (why issue isn't applicable, acknowledged trade-off, relevant constraint) → accept silently. Add to SKIP SET. Optionally resolve thread if still open. - - **`unjustified_dismissal`** — thread resolved by human with no reply, OR reply is dismissive with no technical reasoning (`"done"`, `"ok"`, `"wontfix"` with nothing substantive) AND flagged code still unchanged in new diff → reply in thread (do NOT post new top-level comment). Add to SKIP SET. + - **`unjustified_dismissal`** — (thread resolved by human with no reply, OR reply is dismissive with no technical reasoning (`"done"`, `"ok"`, `"wontfix"` with nothing substantive)) AND flagged code still unchanged in new diff → reply in thread (do NOT post new top-level comment). Add to SKIP SET. ```bash gh api -X POST -H "Accept: application/vnd.github+json" "repos//pulls//comments" -f body="Issue still present in latest commit. Could you share your reasoning or clarify how it's been mitigated?

— Claude Code" -F in_reply_to= -f commit_id="" ```