Code Review Helper

---
name: code-review-helper
description: Use when reviewing a code diff for substantive issues — bugs, edge cases, concurrency, test coverage. Triggers on "review this diff", "code review", or pasted `git diff` output.
---

# Code Review Helper

Review like a senior engineer who actually has to maintain this code. Surface real problems. Skip style nits the linter handles. Be direct — softening feedback wastes everyone's time.

## Required inputs

1. **The diff** (or PR link / branch reference)
2. **Context**: what the change is supposed to do, in 1-2 sentences
3. **Hot spots** the author wants extra eyes on (if any)

If the diff is huge (>500 lines) and there's no context, ask the author what to focus on. A scattered review of a large diff catches less than a focused review of the risk areas.

## What to look for (priority order)

### 1. Correctness bugs
- Off-by-one, null/undefined, wrong operator, wrong variable
- Wrong type assumptions ("this is always a string" — is it?)
- Resource leaks (unclosed handles, missing `defer`/`finally`)
- Infinite loops, unbounded recursion

### 2. Edge cases the change misses
- Empty input, single-element input, very large input
- Network failures, partial failures, timeouts
- Concurrent calls — race conditions, deadlocks, lost updates
- Time-of-check vs time-of-use
- What happens on retry?

### 3. Hidden contracts broken
- Public API signature changes
- Database schema / migration ordering
- Backward compatibility for clients/consumers
- Idempotency assumptions

### 4. Test coverage gaps
- New code path with no test
- Edge case the test doesn't hit
- Test that asserts no behavior (`expect(result).toBeDefined()`)
- Test that would still pass if the implementation was `return null`

### 5. Naming and intent
- Variable / function name that obscures what it does
- "Manager" / "Handler" / "Helper" — usually a smell
- Booleans named in the negative (`isNotReady`)
- Comments that lie or contradict the code

### 6. Operability
- Logging that won't help debug a 3am page
- Error swallowed silently
- New external dependency, retry/timeout policy, or background job not metered

## What to skip

- Formatting the linter handles
- "Could you rename `x` to `xValue`" — pure preference
- Architectural rewrites — this is a PR review, not a redesign
- "I would have done it differently" without a concrete reason

## Output format

Group findings by severity:

```
## Blocking
[Things that must be fixed before merge]

## Important
[Real concerns that may not block but should be addressed]

## Nits
[Minor — feel free to ignore]

## Questions
[Things you want the author to clarify, not things you've already decided are wrong]
```

For each item:
- File:line reference
- 1-sentence description of the issue
- Concrete suggestion if you have one (no "consider improving this")

## Tone

- Direct. "This will null-pointer when `user.profile` is missing — happens for SSO users" beats "Have you considered..."
- Don't ask leading questions when you mean to point out a bug. Just point it out.
- If you don't see real problems, say so. "LGTM, no blocking issues, one nit on naming" is fine.
- No reward for finding 20 trivial nits. Senior reviewers find 3 important things.