improvement(review-pr): add thread management — dedup, outdated resolution, human reply handling#4847
Conversation
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. #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) <noreply@anthropic.com>
Hello jbwatenbergscality,My role is to assist you with the merge of this Available options
Available commands
Status report is not available. |
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
Peer approvals must include at least 1 approval from the following list: |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
.claude/skills/review-pr/SKILL.md
Outdated
|
|
||
| - **`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. |
There was a problem hiding this comment.
The unjustified_dismissal category re-challenges humans who resolve or dismiss a bot comment with "done"/"wontfix". This contradicts the "collaborative not adversarial" principle on line 74. When a maintainer consciously dismisses a bot finding, pushing back creates friction and makes the tool argumentative. The "scale to severity" escape clause is vague enough that the LLM may still argue on minor issues in practice. Consider removing this category or limiting it strictly to security/correctness issues with a much higher bar.
— Claude Code
| ``` | ||
| 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. |
There was a problem hiding this comment.
This heuristic only checks whether code changed at the flagged location. Fixes are often applied elsewhere — e.g., fixing a null-check issue by changing the caller, or fixing a security issue by adding validation in a different file. The bot would incorrectly insist the issue persists when the human genuinely fixed it. Consider checking the broader diff (not just the flagged path+line) or softening the reply to acknowledge the fix may be elsewhere.
— Claude Code
|
|
/approve |
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
Peer approvals must include at least 1 approval from the following list: The following options are set: approve |
…ndition 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) <noreply@anthropic.com>
| ``` | ||
| 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. |
There was a problem hiding this comment.
The unjustified_dismissal category treats a human resolving a thread (with no reply) the same as a dismissive reply. Resolving a thread is a deliberate action — it means the author has handled it. Challenging humans who resolve threads contradicts the collaborative not adversarial instruction at line 81. Consider removing the thread resolved by human with no reply condition and only acting on explicitly dismissive replies on unresolved threads.
— Claude Code
| 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). | ||
|
|
There was a problem hiding this comment.
fix_claimed_but_code_unchanged checks if code is unchanged at this location, but fixes can be applied elsewhere — the author may have refactored, moved the code, or fixed it in a different layer. This will produce false positives where the bot incorrectly claims a fix was not applied. Consider also checking whether the flagged code still exists anywhere in the file (not just the same line), or defaulting to silent acceptance when uncertain.
— Claude Code
| gh api -X POST -H "Accept: application/vnd.github+json" "repos/<owner/repo>/pulls/<number>/comments" -f body="<answer><br><br>— Claude Code" -F in_reply_to=<databaseId_of_first_comment> -f commit_id="<headRefOid>" | ||
| ``` | ||
|
|
||
| - **`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. |
There was a problem hiding this comment.
The classification logic has no fallback for ambiguous human replies that do not clearly match any category. Without a default, the bot may misclassify and take the wrong action. Add a fallback (e.g., treat ambiguous replies as justified and add to SKIP SET) to err on the side of being non-adversarial.
— Claude Code
|
3 participants
Problem
Every push triggers a full re-review (workflow fires on
opened+synchronize). Observed on this repo in PR #4820: 15 review submissions, 14 inline comments all fromclaude[bot], many duplicates on the same lines across pushes.Three root causes:
path:lineon every pushSolution
One file changed:
.claude/skills/review-pr/SKILL.mdNew Step 2 inserted between "Fetch PR details" and "Read changed files". It runs before the diff analysis and manages all existing Claude Code threads.
Thread fetch (with pagination)
GraphQL query fetches all threads including
isResolved,isOutdated, full comment history per thread, andpageInfofor pagination. Loops untilhasNextPageis false — handles PRs with more than 100 review threads.Per-thread classification and action
outdatedisOutdated=true,isResolved=falseresolveReviewThreadmutationno_human_replyauthor_asked_questionin_reply_tojustifiedunjustified_dismissalfix_claimed_but_code_unchangedSecurity/correctness dismissals get stricter language. Minor issues may be accepted silently.
SKIP SET deduplication
The SKIP SET holds
{path, line, body_excerpt}. Part A checks it before posting any new comment. Dedup uses body comparison — a genuinely different issue at the samepath:linecan still be posted.Alignment
scality/agent-hub#26— same logic in the centralclaude-code-review.ymlworkflow (affects all repos using that workflow)scality/ring#6462— same approach for the ring repo skill (pagination + body comparison originated there)🤖 Generated with Claude Code