review-pr

What this skill does

# 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:

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

### 2. Fetch Review Comments

Fetch comments with their IDs:

```bash
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:

| Category | Criteria | Response |
|----------|----------|----------|
| **Already addressed** | Issue was fixed in a subsequent commit, or reviewer misread the code | Explain what the code actually does |
| **Fix** | Valid bug, security issue, or typo that exists in current code | Implement the fix |
| **Won't fix** | Over-engineering, style preference, feature request, or would break consistency | Explain reasoning |

### 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:

```bash
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:

```bash
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:

```bash
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:

```bash
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:

```bash
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

| Reason | Response template |
|--------|-------------------|
| **Matches existing pattern** | "Won't fix — this matches the exact pattern of X, Y, and Z in the same file. Changing just this would be inconsistent." |
| **Over-engineering** | "Won't fix — [the failure mode] will fail obviously / self-heal on retry / is handled by [existing mechanism]." |
| **Feature request** | "Won't fix — this is a feature request rather than a bug. The current flow works: [what it does]. Out of scope for this PR." |
| **Product decision** | "Won't fix — this is a product decision, not a bug. [Current behavior] is intentional because [reason]." |
| **Would break things** | "Won't fix — this would break [existing behavior / consistency with X]." |

---

## Common Reviewer Misunderstandings

### Inverted conditions
Reviewers often misread boolean logic. Example:
```typescript
// "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,