Skip to content

improvement(review-pr): add thread management — dedup, outdated resolution, human reply handling #4847

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
JBWatenbergScality wants to merge 2 commits into
base: development/133.0
Choose a base branch
from
+49 −5
Open

improvement(review-pr): add thread management — dedup, outdated resolution, human reply handling #4847

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
761dfe9
improvement(review-pr): add thread management to review skill
JBWatenbergScality Mar 27, 2026
2357b0e
fix(review-pr): correct OR/AND precedence in unjustified_dismissal co…
JBWatenbergScality Mar 27, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 49 additions & 5 deletions .claude/skills/review-pr/SKILL.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,9 +32,50 @@ gh pr view <number> --repo <owner/repo> --json title,body,headRefOid,author,file
gh pr diff <number> --repo <owner/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:"<owner>",name:"<repo>"){pullRequest(number:<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:"<endCursor>"` to `reviewThreads(first:100, after:"<endCursor>")` 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:"<id>"}){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/<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.
Copy link
Copy Markdown

@claude claude bot Mar 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


- **`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/<owner/repo>/pulls/<number>/comments" -f body="Issue still present in latest commit. Could you share your reasoning or clarify how it's been mitigated?<br><br>— Claude Code" -F in_reply_to=<databaseId_of_first_comment> -f commit_id="<headRefOid>"
```
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.
Copy link
Copy Markdown

@claude claude bot Mar 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown

@claude claude bot Mar 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

```bash
gh api -X POST -H "Accept: application/vnd.github+json" "repos/<owner/repo>/pulls/<number>/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?<br><br>— Claude Code" -F in_reply_to=<databaseId_of_first_comment> -f commit_id="<headRefOid>"
```

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).

Copy link
Copy Markdown

@claude claude bot Mar 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

4. **Analyze the changes** against these criteria:

| Area | What to check |
|------|---------------|
Expand All @@ -55,13 +96,16 @@ gh pr diff <number> --repo <owner/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/<owner/repo>/pulls/<number>/comments" -f body="Your comment<br><br>— Claude Code" -f path="path/to/file" -F line=<line_number> -f side="RIGHT" -f commit_id="<headRefOid>"
Expand Down Expand Up @@ -104,7 +148,7 @@ Do not describe or summarize the PR. For each issue, state the problem on one li
- <suggestion>
```

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

Expand Down
Loading