richtabor/agent-skills/review-pr

Review PR Comments

Fetch, evaluate, and address PR review comments from GitHub. Many reviewer comments reference outdated code or misunderstand the logic — always verify against the actual codebase before acting.

Process

This is a single-pass flow. Fetch comments, read files, evaluate, present assessment, reply to all comments, resolve all threads, and show the final summary — all without pausing for user confirmation.

1. Find the PR

If no PR number is provided, find the PR for the current branch:

gh pr list --head $(git branch --show-current) --json number,title --jq '.[0]'

2. Fetch Review Comments

Fetch comments with their IDs:

gh api repos/{owner}/{repo}/pulls/{number}/comments --jq '.[] | {id: .id, path: .path, line: (.line // .original_line), body: .body, in_reply_to_id: .in_reply_to_id}'

Filter to only top-level comments (where in_reply_to_id is null).

3. Read Referenced Files (Critical Step)

Before evaluating any comment, read the actual current code. Reviewers often:

• Reference line numbers from an older commit
• Misread the logic
• Comment on code that's already been fixed

For each unique file path, read the file and verify:

• Does the line number match what the reviewer is describing?
• Has the issue already been addressed?
• Is the reviewer's understanding of the code correct?

4. Evaluate All Comments

Categorize each comment:

5. Present Assessment, Reply, and Resolve

Do all of this in one pass — no pausing for user confirmation. Present your assessment, reply to each comment, and resolve all threads immediately.

Present ALL comments with your assessment:

## PR #[number]: [title]
 
Found [X] review comments. Here's my assessment:
 
---
 
### 1. `path/file.ts:42` — **Already addressed**
> [comment body]
 
**Issue**: Reviewer concerned about X
**Actual code**: Lines 26-34 already handle this — [brief explanation]
 
---
 
### 2. `path/other.ts:15` — **Won't fix**
> [comment body]
 
**Issue**: Reviewer wants X
**Why skip**: [Over-engineering / Matches existing pattern / Feature request / etc]
 
---
 
### 3. `path/file.ts:88` — **Fix**
> [comment body]
 
**Issue**: Typo in user-facing string
**Proposed change**: Change "Jumpope" to "Jump Rope"
 
---
 
## Summary
- **Already addressed**: 6 comments
- **Won't fix**: 8 comments
- **Fix**: 2 comments

Then immediately reply to all comments and resolve all threads (steps 6-7 below). Don't wait for user input.

6. Reply to Comments

For Already addressed comments, reply explaining the current state:

gh api repos/{owner}/{repo}/pulls/{pr_number}/comments/{comment_id}/replies \
  -X POST \
  -f body="This is already handled — lines 26-34 fetch the authenticated user and verify user.id === state before proceeding. If they don't match, it rejects with a CSRF error."

For Won't fix comments, reply with concise reasoning:

gh api repos/{owner}/{repo}/pulls/{pr_number}/comments/{comment_id}/replies \
  -X POST \
  -f body="Won't fix — this matches the exact pattern used for the other integrations in the same file. Changing just this one would be inconsistent."

For Fix comments, implement the fix first, then reply:

gh api repos/{owner}/{repo}/pulls/{pr_number}/comments/{comment_id}/replies \
  -X POST \
  -f body="Fixed in abc1234"

7. Resolve All Threads

First, get all unresolved thread IDs:

cat << 'QUERY' | gh api graphql --input - --jq '.data.repository.pullRequest.reviewThreads.nodes[] | select(.isResolved == false) | {threadId: .id, commentId: .comments.nodes[0].databaseId}'
{"query": "query($owner: String!, $repo: String!, $pr: Int!) { repository(owner: $owner, name: $repo) { pullRequest(number: $pr) { reviewThreads(first: 100) { nodes { id isResolved comments(first: 1) { nodes { databaseId } } } } } } }", "variables": {"owner": "OWNER", "repo": "REPO", "pr": 123}}
QUERY

Then resolve each thread by ID:

cat << 'QUERY' | gh api graphql --input -
{"query": "mutation { resolveReviewThread(input: {threadId: \"THREAD_ID_HERE\"}) { thread { isResolved } } }"}
QUERY

8. Final Summary

After replying and resolving, show the final summary:

## Review Complete
 
All 16 conversations resolved.
 
### Already addressed (6)
| Comment | Explanation |
|---------|-------------|
| State validation | Already validates user.id === state at lines 26-34 |
| HR zones docs | Comments already say "milliseconds", not "percentage" |
 
### Won't fix (8)
| Comment | Reason |
|---------|--------|
| Form semantics | Matches existing pattern for other integrations |
| Rate limiting | Self-heals on next cron run, over-engineering |
 
### Fixed (2)
| Commit | Fix |
|--------|-----|
| `abc123` | Fixed typo "Jumpope" → "Jump Rope" |

Important: The entire flow (assess → reply → resolve → summarize) happens in one pass. Don't pause to ask "should I proceed?" — just do it.

Evaluation Guidelines

Already Addressed — Explain, don't fix

Common patterns where the reviewer is wrong:

• Reviewer references wrong line numbers: Code has changed since they reviewed
• Reviewer misread the logic: e.g., thinks condition is inverted when it's correct
• Issue was already fixed: Subsequent commits addressed it
• Code exists elsewhere: The check/validation happens in a different function

Response template:

"This is already handled — [specific location] does [what it does]. [Brief explanation of why it's correct]."

Fix — Implement and commit

Only fix issues that:

• Actually exist in the current code
• Are bugs, security issues, or typos
• Can be verified by reading the file

Won't Fix — Explain reasoning

Common Reviewer Misunderstandings

Inverted conditions

Reviewers often misread boolean logic. Example:

// "Refresh if expires in less than 5 minutes"
if (expiry.getTime() - Date.now() > 5 * 60 * 1000) {
  return integration.access_token!; // Token is fresh, return early
}
// Proceed to refresh...

Reviewer says: "Condition is inverted, tokens never refresh" Reality: Condition is correct — returns early when fresh, refreshes when stale.

Missing validation that exists

Reviewer says: "State parameter not validated" Reality: Lines 26-34 already validate it. Always check if the validation exists before agreeing.

Tokens in error messages

Reviewer says: "Sensitive tokens logged in error messages" Reality: The error logs the API's error response, not our tokens. Read what's actually in the error string.

Code that doesn't exist at that line

Reviewer references line 174, but file only has 161 lines. The file has changed since the review.

Key Principles

1. Single-pass execution: Don't pause to ask "should I proceed?" — assess, reply, resolve, and summarize in one flow
2. Verify before acting: Always read the actual file before evaluating a comment
3. Trust the codebase: If existing patterns work, new code should match them
4. Explain, don't just dismiss: Skipped comments deserve clear explanations
5. Resolve everything: All threads should be resolved, whether fixed or explained
6. Be concise: Responses should be 1-2 sentences, not paragraphs