Code Review

# Code Review

Produce a senior-level pull-request review from a diff. Focus on the
issues that matter; skip the things a linter already catches.

## When to use

Use this skill when the user:
- Pastes a diff and asks "review this"
- Pastes a PR URL and asks "review this PR"
- Asks "what's wrong with this code?"

## Inputs

Required: A unified diff, file path + content, or PR link.

Optional: Repository context — language, framework, conventions doc. If the
user provides a CONTRIBUTING.md or style guide, weight it heavily.

## Output format

```markdown
### Summary

<one sentence: what this PR does, in user-visible terms>

### Blocking (must fix before merge)

1. **<file>:<line> — <one-line title>**
   <2–3 sentences explaining the issue and the consequence.>
   ```<lang>
   // suggested fix
   ```

### Suggestions (worth considering)

1. **<file>:<line> — <title>**
   <reason>

### Nits (optional)

- <file>:<line> — <one-line note>

### Recommendation

<approve | request changes | needs discussion> — <one-sentence reason>
```

## Review priorities (in order)

1. **Correctness** — does it do what it claims? Off-by-one, null/undefined,
   wrong condition, wrong return type.
2. **Security** — auth bypass, missing input validation, SQL injection, secret
   leakage, SSRF, CSRF, path traversal, unbounded input.
3. **Data integrity** — missing transaction, missing FK, race conditions,
   orphaned rows, broken idempotency.
4. **Tests** — new behavior without tests, tests that don't actually verify
   the new behavior, tests that pass even when the feature is broken.
5. **Performance** — N+1 queries, missing indexes, unbounded loops, memory
   leaks. Only flag if measurable.
6. **API design** — breaking changes, inconsistent naming, exports that should
   be internal.
7. **Readability** — only when it actively impedes future maintenance.

## Things to NOT comment on

- Lint/format issues — that's the linter's job.
- Cosmetic preferences (single vs double quotes, prose comments).
- "I would have done it differently" without a concrete reason.
- Restating what the diff does — the author already knows.
- "Nice work!" / "LGTM!" with no substance.

## Process

1. Read the whole diff before commenting on any single line. Local issues
   sometimes resolve into global ones.
2. For each potential issue, ask: "Will this break in production?" If yes →
   Blocking. If "this is fine but cleaner is X" → Suggestion. If style → Nit
   or skip.
3. For every Blocking item, propose a concrete fix as a code block — not a
   description.
4. End with a clear recommendation: approve, changes requested, or needs
   discussion. Don't hedge.

## Example

```markdown
### Summary

Adds a per-user rate limit to the skill-import endpoint.

### Blocking

1. **lib/ratelimit.ts:42 — Race condition between read and increment**
   The current code does `SELECT count → INCR if under limit` in two
   queries. Two concurrent requests can both see "9 of 10" and both
   increment to 11. Use a single `INCR + EXPIRE` round-trip and check
   the returned value.
   ```ts
   const n = await redis.incr(key)
   if (n === 1) await redis.expire(key, windowSec)
   if (n > limit) return { allowed: false }
   ```

### Suggestions

1. **app/api/skills/import/route.ts:78 — Rate-limit response should
   include a `Retry-After` header**
   Clients can back off cleanly without parsing the body.

### Nits

- lib/ratelimit.ts:18 — Function name `checkRateLimit` is fine but
  `consumeRateLimit` would better signal that this mutates state.

### Recommendation

Changes requested — the race condition will misfire under load.
```

## Hard rules

- Always quote file path and line range. "Somewhere in the route handler"
  is useless.
- Always end with a clear recommendation.
- Never skip the Blocking section — write "_None._" if there are none.
- Never invent code that isn't in the diff.