review-pr

What this skill does

# Review Pull Request

Perform a comprehensive code review of a pull request.

## Instructions

1. Get PR information: `gh pr view $ARGUMENTS --json title,body,files,additions,deletions`
2. Review each changed file
3. Provide structured feedback

## Review Checklist

### Code Quality
- [ ] Code is readable and well-organized
- [ ] Functions are appropriately sized
- [ ] No code duplication
- [ ] Meaningful variable/function names

### Functionality
- [ ] Logic is correct
- [ ] Edge cases handled
- [ ] Error handling is comprehensive
- [ ] No obvious bugs

### Security
- [ ] No hardcoded secrets
- [ ] Input validation present
- [ ] No injection vulnerabilities
- [ ] Authorization checks in place

### Testing
- [ ] Tests added for new code
- [ ] Existing tests still pass
- [ ] Edge cases tested

### Documentation
- [ ] Code is self-documenting or commented
- [ ] README updated if needed
- [ ] API changes documented

## Output Format

```markdown
## PR Review: #[number] - [title]

### Summary
[1-2 sentence overview]

### Approval Status
[ ] Approved
[ ] Approved with suggestions
[ ] Changes requested

### Findings

#### Critical (Must Fix)
- [ ] [Issue description] - `file:line`

#### Suggestions (Should Consider)
- [ ] [Improvement] - `file:line`

#### Nitpicks (Optional)
- [ ] [Minor suggestion] - `file:line`

### Positive Highlights
- [What's done well]

### Questions
- [Clarifications needed]
```

## Usage

```
/review-pr 123
/review-pr https://github.com/owner/repo/pull/123
```

---

## Advanced: Multi-Agent Review

For production-grade reviews requiring specialized perspectives and anti-hallucination safeguards.

### Pre-flight Check

Before reviewing, check if this is a follow-up pass to avoid repeating suggestions:

```bash
# Detect if Claude already reviewed this PR
git log --oneline -10 | grep "Co-Authored-By: Claude"
```

If detected, note: "This appears to be a follow-up pass. I'll focus on new issues and avoid repeating previous suggestions."

### Scope Drift Detection

Cross-reference the PR diff against the original plan to catch unintended changes.

```bash
# Detect current branch
BRANCH=$(git branch --show-current)

# Search for a plan file associated with this branch
ls ~/.claude/plans/ 2>/dev/null | grep -i "$BRANCH" | head -3

# Files actually changed in this PR
git diff --stat origin/main...HEAD | head -30
```

If a plan file exists for this branch:
1. Read the plan file: what was the stated scope?
2. Compare stated scope vs actual `git diff --stat`
3. Flag files changed that were NOT mentioned in the plan

Output format:
```
SCOPE DRIFT CHECK
─────────────────────────────────────────
Plan scope:    [what the plan said would change]
Actual diff:   [files actually changed]
Drift:         [files changed outside plan scope, if any]
Verdict:       IN SCOPE / DRIFT DETECTED
```

If no plan file exists: note "No plan file found for this branch, skipping scope drift check."

### Multi-Agent Specialization

Launch 3 parallel specialized agents (see [Split Role Sub-Agents](../../guide/ultimate-guide.md#split-role-sub-agents)):

**Agent 1: Consistency Auditor**
```
Focus: DRY violations, duplicate logic, pattern inconsistencies
Check for:
- Duplicated code blocks (>5 lines similar)
- Inconsistent naming conventions
- Pattern violations (if project uses X pattern, enforce it)
```

**Agent 2: SOLID Principles Analyst**
```
Focus: Single Responsibility Principle violations, complexity
Check for:
- Functions >50 lines (likely doing too much)
- Nested conditionals >3 levels deep
- Cyclomatic complexity >10
- Mixed concerns in single component
```

**Agent 3: Defensive Code Auditor**
```
Focus: Silent failures, masked bugs, hidden fallbacks, LLM output trust boundary
Check for:
- Empty catch blocks: try { } catch (e) { } // swallows error
- Silent fallbacks: return data || DEFAULT // hides missing data
- Unchecked null/undefined: user.name without validation
- Ignored promise rejections: async fn without .catch()

LLM Output Trust Boundary (especially relevant in AI-assisted codebases):
- LLM-generated values (emails, URLs, names, IDs) written to DB or passed to
  downstream functions without format validation; add lightweight guards
  (email regex, URL parsing, .trim()) before persisting
- Structured tool output (arrays, objects from AI tools) accepted without
  type/shape checks before database writes or rendering
- AI-generated SQL or code strings executed without sanitization
```

### Anti-Hallucination Rules

**Verify before asserting**:
- Use `Grep` or `Glob` to verify patterns before recommending them
- If suggesting "use existing UserService pattern", confirm UserService exists first
- Never claim "project uses X" without checking actual codebase

**Occurrence rule**:
- Pattern with >10 occurrences = established (Suggestion level)
- Pattern with <3 occurrences = not established (Can Skip or ask maintainer)
- Read full file context, not just diff lines

**Uncertainty markers**:
- Use "❓ To verify:" when unsure about project conventions
- Use "💡 Consider:" for optional improvements
- Use "🔴 Must fix:" only for critical bugs/security

### Reconciliation

After agents report findings:

1. **Deduplicate**: Remove overlapping suggestions across agents
2. **Prioritize existing patterns**: If codebase uses pattern X, recommend X (not ideal pattern Y)
3. **Mark skipped suggestions**: "Skipping [suggestion] because project uses [alternative pattern]"
4. **Track reasoning**: Document why suggestion was kept or skipped

### Severity Classification

```
🔴 Must Fix (Blockers)
- Security vulnerabilities
- Data loss risks
- Breaking changes without migration
- Silent failures masking bugs

🟡 Should Fix (Improvements)
- SOLID violations causing maintenance issues
- DRY violations (>3 duplicates)
- Performance bottlenecks (N+1 queries)
- Missing error handling for critical paths

🟢 Can Skip (Nice-to-haves)
- Style inconsistencies (if no linter)
- Minor naming improvements
- Overly nested code (if <3 levels)
- Documentation gaps (if code self-documenting)
```

### Fix-First Heuristic

Determine whether to auto-fix each finding or surface it for user decision.

```
AUTO-FIX (apply without asking):          ASK (needs human judgment):
├─ Dead code / unused variables            ├─ Security changes (auth, XSS, injection)
├─ N+1 queries (missing eager loading)     ├─ Race conditions
├─ Stale comments contradicting code       ├─ Design decisions
├─ Magic numbers → named constants         ├─ Large fixes (>20 lines changed)
├─ Missing import / path mismatches        ├─ Enum completeness
├─ Variables assigned but never read       ├─ Anything removing functionality
└─ Obvious version/doc mismatches          └─ User-visible behavior changes
```

**Rule**: If a senior engineer would apply the fix in 30 seconds without discussion, it's AUTO-FIX. If reasonable engineers could disagree, it's ASK.

After agents report findings:
1. Apply all AUTO-FIX items immediately with minimal targeted edits
2. Batch all ASK items into a single user decision (not one question per item)

### Auto-Fix Loop (Optional)

For automated convergence:

```
Review → Identify issues → Fix → Re-review → Repeat until minimal changes

Safeguards:
- Max 3 iterations to prevent infinite loops
- Run tsc/lint check before each iteration
- Skip auto-fix for protected files (package.json, migrations, etc.)
```

**Example prompt**:
```
Review this PR with auto-fix enabled:
1. Review using 3 agents above
2. Fix all 🔴 Must Fix issues
3. Re-review to verify fixes
4. Repeat for 🟡 Should Fix (max 2 more iterations)
5. Stop when only 🟢 Can Skip remain
```

### Conditional Context Loading

Load additional context based on diff content (stack-agnostic):

| If diff contains... | Then check... |
|---------------------|---------------|
| Database queries | Indexes, N+1 patterns, query optimization |
| API endpoints | Auth middleware, input validation, rate limiting |
| Authentication logic | Password hashing,