address-review

# Address PR Review Comments

Load this skill when:
- A PR has review comments that need addressing
- User says "address review", "check PR comments", "handle review feedback"
- After pushing code and wanting to check for automated reviewer feedback

---

## Core Principle: Critical Assessment First

**DO NOT blindly implement every suggestion.** Review comments—especially from automated reviewers like Copilot—vary widely in quality. Your job is to:

1. **Assess** each comment critically
2. **Decide** whether it improves the code
3. **Act** appropriately (implement, push back, or discuss)

This aligns with CLAUDE.md: *"Push back if something feels wrong"*

---

## Workflow

### Step 1: Fetch ALL Review Comments

**IMPORTANT**: Reviews come from TWO different sources. You must check BOTH:

```bash
# Get PR number from current branch
PR_NUMBER=$(gh pr view --json number -q '.number' 2>/dev/null)

# 1. Fetch inline review comments (Copilot posts here)
gh api repos/{owner}/{repo}/pulls/$PR_NUMBER/comments

# 2. Fetch issue comments (Claude posts here via GitHub Actions)
gh pr view $PR_NUMBER --comments --json comments
```

**Why two sources?**
- **Pull request comments** (`/pulls/.../comments`): Inline code review comments attached to specific lines. Copilot uses this.
- **Issue comments** (`--comments`): General PR comments not attached to code lines. Claude (via GitHub Actions) posts here.

If you only check one source, you WILL miss reviews. Always check both.

### Step 2: Assess Each Comment

For each comment, evaluate:

| Question | Assessment |
|----------|------------|
| Does this fix a real bug? | High value if yes |
| Does this improve readability significantly? | Medium value if yes |
| Does this improve maintainability? | Medium value if yes |
| Is this a style nitpick with no functional benefit? | Low value |
| Could this suggestion make things worse? | Negative value - push back |
| Is this context-dependent and the reviewer lacks context? | Discuss or push back |

### Step 3: Categorize

Assign each comment to one of:

#### IMPLEMENT
- Fixes actual bugs
- Prevents real security issues
- Significantly improves clarity
- Adds missing error handling that matters

#### PUSH BACK
- Style nitpicks with no functional benefit
- Suggestions that reduce debuggability (e.g., combining assertions)
- Over-engineering for hypothetical scenarios
- Changes that contradict project patterns
- Automated suggestions that lack context

#### DISCUSS
- Architectural decisions that need human input
- Trade-offs where both options are valid
- Changes that might affect other parts of the codebase

---

## Assessment Criteria by Comment Type

### Code Style Comments
```
"Consider renaming X to Y"
"This could be more concise"
```
**Usually PUSH BACK** unless the current name is genuinely confusing.

### Assertion/Test Comments
```
"Combine these assertions"
"Simplify this test"
```
**Often PUSH BACK** — Separate assertions give better failure messages. Don't sacrifice debuggability for brevity.

### Error Handling Comments
```
"Add error handling for X"
"Handle the case where Y is null"
```
**ASSESS carefully** — Is this a real scenario? Don't add defensive code for impossible cases.

### Documentation Comments
```
"Add a docstring"
"Document this behavior"
```
**IMPLEMENT** if the code is genuinely unclear. **PUSH BACK** if the code is self-documenting.

### Security Comments
```
"Validate input X"
"Sanitize before using"
```
**IMPLEMENT** if at a trust boundary. **PUSH BACK** if internal code where input is already validated.

### Performance Comments
```
"This could be optimized by..."
"Consider caching X"
```
**PUSH BACK** unless there's evidence of a real performance problem. Premature optimization is the root of all evil.

---

## Response Templates

### For IMPLEMENT
```
Implementing: [brief description]
Reason: [why this improves the code]
```
Then make the change.

### For PUSH BACK
Draft a response for the PR:
```
Thanks for the suggestion. I'm going to keep the current implementation because:
- [Concrete reason 1]
- [Concrete reason 2]

[Optional: explanation of trade-off considered]
```

### For DISCUSS
Ask the user:
```
Comment suggests: [summary]
Trade-offs:
- Option A: [pros/cons]
- Option B: [pros/cons]
How would you like to proceed?
```

---

## Example Assessment

**Comment**: "These three assertions check for the same constraint. Consider combining them into a single assertion."

**Assessment**:
- Does it fix a bug? No
- Does it improve readability? Marginally
- Does it improve maintainability? No
- Could it make things worse? **YES** — Combined assertion loses specificity. If the test fails, you won't know which pattern was found.

**Decision**: PUSH BACK

**Response**: "Keeping separate assertions for better failure diagnostics. When a test fails, we want to know exactly which forbidden pattern was detected, not just that 'one of three patterns' was found."

---

## After Processing All Comments

Provide a summary:

```
## Review Assessment Summary

**PR**: #194
**Total comments**: 6

| # | Comment | Assessment | Action |
|---|---------|------------|--------|
| 1 | Combine assertions | Low value - loses debug info | PUSH BACK |
| 2 | Add user feedback | Medium value - UX improvement | IMPLEMENT |
| 3 | Rename variable | Nitpick | PUSH BACK |
...

**Implementing**: 2 comments
**Pushing back**: 3 comments
**Discussing**: 1 comment

Shall I proceed with implementing the valuable changes and drafting push-back responses?
```

---

## Common Automated Reviewer Patterns to Watch For

### Copilot Tends To:
- Suggest combining code that's intentionally separate
- Flag "redundancy" that's actually clarity
- Miss project-specific patterns
- Suggest over-abstraction

### Claude (as reviewer) Tends To:
- Be more context-aware but sometimes over-thorough
- Suggest documentation where code is self-documenting
- Sometimes miss that simpler is better
- **Miss structural/syntactic bugs** while praising high-level architecture
- Give false confidence ("LGTM") while bugs exist — don't treat approval as validation

---

## Comparing Copilot vs Claude Reviews

When both reviewers have commented on a PR, generate a comparison summary.

### How to Identify Reviewer Source

```bash
# Copilot comments have user.login = "Copilot" or similar bot identifier
# Claude comments may come from a GitHub Action or specific bot account

gh api repos/{owner}/{repo}/pulls/$PR_NUMBER/comments | jq '
  group_by(.user.login) |
  map({reviewer: .[0].user.login, count: length, comments: map(.body[:80])})
'
```

### Comparison Summary Template

```
## Reviewer Comparison: PR #194

### Overview
| Metric | Copilot | Claude |
|--------|---------|--------|
| Total comments | 4 | 6 |
| High value | 1 | 3 |
| Low value/nitpicks | 3 | 2 |
| Overlapping concerns | 2 | 2 |

### Agreement (Both flagged)
These issues were caught by both reviewers — higher confidence they matter:
- [ ] Issue X: [brief description]
- [ ] Issue Y: [brief description]

### Copilot Only
Issues only Copilot raised:
- [ ] [description] — Assessment: [IMPLEMENT/PUSH BACK/DISCUSS]

### Claude Only
Issues only Claude raised:
- [ ] [description] — Assessment: [IMPLEMENT/PUSH BACK/DISCUSS]

### Contradictions
Where reviewers disagree or suggest opposite approaches:
- Copilot says: [X]
- Claude says: [Y]
- **Recommendation**: [which to follow and why]

### Complementarity Analysis
- **Copilot strengths this PR**: [e.g., caught syntax issues, import redundancy]
- **Claude strengths this PR**: [e.g., caught logic issues, UX concerns]
- **Blind spots both missed**: [if any obvious issues neither caught]

### Summary
[1-2 sentences on overall review quality and recommended actions]
```

### Interpreting Agreement/Disagreement

| Scenario | Interpretation | Action |
|----------|---------------|--------|
| Both flag same issue | High confidence it matters | Likely IMPLEMENT |
| Only Copilot flags | Often a pattern/style nitpick | Assess carefully, often PUSH BACK |
| Only Claude flags | Often contextual/architectural | Assess carefully, often valuable |
| They contradict | Need human judgment | DISCUSS with user |

### Example Comparison Output

```
## Reviewer Comparison: PR #194

### Overview
| Metric | Copilot | Claude |
|--------|---------|--------|
| Total comments | 6 | 4 |
| High value | 1 | 2 |
| Low value/nitpicks | 4 | 1 |
| Overlapping concerns | 1 | 1 |

### Agreement
- Silent return when no metrics (both caught) → IMPLEMENT

### Copilot Only
- Combine redundant assertions → PUSH BACK (loses debug info)
- Remove "inline polling" from message → PUSH BACK (nitpick)
- Use sys.modules instead of import → IMPLEMENT (valid)
- Simplify test structure → PUSH BACK (over-abstraction)

### Claude Only
- Consider retry logic for flaky API → DISCUSS (scope creep?)
- Add integration test coverage → IMPLEMENT (good catch)
- Type hints on callback → PUSH BACK (internal function)

### Complementarity
- **Copilot**: Good at catching import/syntax patterns
- **Claude**: Better at catching missing functionality and UX issues
- **Together**: Reasonable coverage, but both over-index on style

### Summary
6 of 10 comments are low-value nitpicks. Implement 3 (silent return,
sys.modules, integration test), push back on 6, discuss 1 (retry logic).
```

---

## Key Reminders

1. **Quality over compliance** — A clean review with 0 comments addressed can be better than implementing bad suggestions
2. **Explain push-backs** — Don't just ignore; respond with reasoning
3. **Trust your judgment** — You've read the code; automated reviewers often haven't
4. **Ask when uncertain** — Use DISCUSS for genuinely ambiguous cases
5. **Batch similar decisions** — If pushing back on multiple similar comments, explain once

---

## When Done

After addressing all comments:
1. Commit any implemented changes
2. Push to update the PR
3. Post push-back responses as PR comments (or suggest user does)
4. Advise whether to request re-review