---
name: forge-code-review
description: Reviewing pull requests well. What to look at first, what to skip, severity prefixes, when to block vs approve, frame feedback as questions, never rewrite-as-review, reviewing AI-generated code specifically. Contains the severity-prefix vocabulary + a reviewer's mental checklist. Use whenever you are reviewing someone else's code.
license: MIT
---

# forge-code-review

You are reviewing a pull request. Default code-review behavior is either rubber-stamp ("LGTM 🚀") or rewrite ("here is how I would have done it" with a 50-comment teardown). Both fail the team. This skill exists to do code review that catches real bugs, teaches when appropriate, and ships code.

The mental model: **a review is a conversation about a specific change, not an audit of the entire codebase.** The reviewer's job is to ensure the change is safe to ship and the codebase remains coherent. Anything else is scope creep.

## Quick reference (the things you must never do)

1. Approve without reading the diff.
2. Block on subjective style covered by the formatter.
3. Rewrite the entire function as a "suggestion."
4. Leave 30 inline nits with no priority.
5. Comment "I would have done X" with no functional reason.
6. Approve immediately on a friend's PR without scrutiny.
7. Block immediately on a stranger's PR for taste alone.
8. Demand the author refactor unrelated code.
9. Make the author rebase the design decision over the PR.
10. Use the review to relitigate technology choices made elsewhere.

## Hard rules

### Where to look first

**1. Read the PR description before the diff.** What is this change for? Without context, the diff is just code.

**2. Read tests before implementation.** Tests reveal intent. A new feature with no tests is a structural problem; a new feature with thorough tests tells you exactly what the author thinks the behavior should be.

**3. Look at file structure first.** What files were touched? Is the scope what you expected from the description? An auth fix that touches a billing handler is a red flag.

**4. Then read the diff in dependency order or as GitHub presents it.**

### What to look for (in order)

**5. Correctness.** Does it do what the description says? Edge cases (empty inputs, null, unauthorized, race conditions, retries)?

**6. Tests.** Do they actually test the behavior? Mocking too much defeats the test. Tautological assertions defeat the test. Skipped tests committed without justification are a smell.

**7. Naming and clarity.** Will someone reading this in 18 months understand it? Bad names compound.

**8. Scope.** Did the PR do what it said, and nothing else? A "fix the bug" PR that also renames 30 files is going to ship the bug after the rename gets reverted.

**9. Security.** Is user input validated? Are secrets handled correctly? Are auth checks present where state mutates?

**10. Concurrency.** Is shared state accessed safely? Race conditions possible? Does retry-on-failure preserve idempotency?

**11. Performance.** N+1 query? Unbounded loop? Catastrophically backtracking regex?

**12. Observability.** Will you be able to debug this when it fails at 2am? Are errors logged with context? Is success measurable?

### What NOT to comment on

**13. Style covered by the formatter.** Prettier / Black / gofmt should handle this. If they did not run, the comment is "please run the formatter," not 12 inline nits.

**14. Personal preference dressed as principle.** "I would have used a `for` loop here" - if both work, let the author choose.

**15. Refactoring the whole file.** A PR fixing one bug is not the place to restructure unrelated code. Open a follow-up.

**16. The technology choice.** "Why use React Query instead of SWR?" - either it was decided already, or this PR is not the place.

### How to phrase feedback

**17. Praise specifically when something is well done.** "The way you handled the retry-with-backoff here is exactly right; particularly the dead-letter step." Specific praise teaches and motivates.

**18. Frame critical feedback as a question first.**

```
GOOD:  "What happens if `userId` is null here?"
BAD:   "You forgot to handle null."
```

The question invites; the assertion accuses.

**19. State the severity. Prefix when unclear.**

| Prefix | Meaning | Author's action |
| --- | --- | --- |
| `nit:` | Cosmetic, optional | Fix if easy; can ignore |
| `question:` | Genuine inquiry | Answer in a comment |
| `suggestion:` | Optional improvement | Consider; pass or take |
| `blocker:` | Must fix before merge | Fix and request re-review |

Without prefixes, every comment carries the same weight and important ones get lost.

**20. Suggest a fix only when clearly better.** Otherwise, name the problem and let the author solve it. They have more context than you.

**21. For non-trivial feedback, explain why.** "Move this validation to the schema parse" + one sentence on why is teaching. Without the why, it's an assertion of authority.

### When to block

**22. Block for:** incorrect behavior, security holes, missing tests for non-trivial logic, broken builds. Objective; author should fix before merging.

**23. Do not block for:** subjective style, "I would have done it differently," refactor preferences, minor nits. Use "approve with comments" or just leave the comments and approve.

**24. A blocked PR has a path forward.** Not "block: this is bad." Instead:

```
blocker: this introduces SQL injection.
Use a parameterized query:
  db.query('SELECT * FROM users WHERE id = $1', [userId])
```

### When to approve

**25. Approve when the change is correct and ships value, even if not perfect.** Perfect is the enemy of merged.

**26. Approve with comments for non-blocking suggestions.** Author can pick them up in this PR or follow up.

**27. Do not approve PRs you have not actually read.** Rubber stamps destroy the value of review.

### Large PRs

**28. A PR over ~500 lines is hard to review well.** Ask the author to split if possible.

```
question: this is a lot to review well. Could the schema change be a separate PR before the handler change?
```

**29. If split is not possible, review in chunks.** Read the schema first, then the migrations, then the handlers, then the tests. Mark progress in a comment.

**30. Get a second reviewer.** Two pairs of eyes catch what one misses.

### Specific patterns to flag

| Pattern | Why | Action |
| --- | --- | --- |
| `try { ... } catch { /* ignore */ }` | Swallows errors | blocker |
| `as any`, `// @ts-ignore` | Escapes type system | blocker without an issue link |
| Magic numbers | Unmaintainable | suggestion: extract a constant |
| Hardcoded URLs / paths | Will break in another env | suggestion: env var |
| Inputs unvalidated at boundary | Security risk | blocker |
| DB query built by string concat | SQL injection | blocker |
| `console.log` left in | Will spam prod | blocker |
| `TODO` without issue link | Rots | nit: link an issue or remove |
| `it.skip(...)` committed | Dead test | blocker without an issue link |

### Reviewing in different contexts

**Junior author.** More teaching, more "why." Explain principles, not just rules. Approve smaller correct work to build confidence. Pair on the response if multiple things need changing.

**Senior author.** Less teaching, more peer-level feedback. Higher bar for blocking; assume the author thought about it. Discuss architectural choices if you disagree, defer when reasonable.

**Yourself, six months later.** Future-you is a stranger. The review of your own past code is informative.

**LLM-generated code.** The patterns matter more than the lines. AI is good at producing plausible code; reviewers must check for the failure modes specific to AI: hallucinated APIs, fabricated imports, made-up library functions, security defaults wrong by omission, "// rest of the file" truncations, tests that look correct but test nothing.

## Common AI-output patterns to reject (in the PR itself)

| Pattern | Why slop | Action |
| --- | --- | --- |
| `// rest of implementation` | Truncation | blocker: write the full code |
| Hallucinated import (`from "some-lib"`) | Library doesn't exist | blocker: verify or remove |
| `expect(true).toBe(true)` | Tautological test | blocker: real assertion |
| `try { ... } catch (e) { console.log(e) }` | Silent failure | blocker: structured logger + rethrow or Result |
| Schema validation in handler AND in DTO AND in repo | Triple-check | nit: keep one at the boundary |
| `any` in TypeScript | Opts out of types | blocker: `unknown` + narrow |
| New CSS gradient `from-indigo to-violet` | AI fingerprint | suggestion: solid brand color |
| Generated tests that pass without the new code | Tests are wrong | blocker: tests must fail without code |

## Worked example: a real review

PR description: "Adds cursor pagination to GET /orders."

Reviewer comments:

> ✅ **Praise:** The cursor encoding here is solid - opaque, JSON-base64 round-trip with a try/catch that returns a typed error. This is exactly the pattern from forge-api-design rule 10.
>
> **question:** What happens if `limit=10000` is passed? I see the schema caps it at 200 but I want to confirm the SQL doesn't allow unbounded reads.
>
> **suggestion:** The `decodeCursor` return type could be `Result<Cursor, "invalid_cursor">` instead of `Cursor | null`. The current `null` overloads "missing" and "malformed."
>
> **nit:** `lines 142-145` could be hoisted out of the loop, but it's not hot path. Take or pass.
>
> **blocker:** I don't see a test for what happens on a malformed cursor. The handler returns 400 from `decodeCursor` failure but there's no `it("rejects malformed cursor with 400")` case.

What this review does right: opens with specific praise (rule 17); each comment is prefixed with severity (rule 19); blocker has a clear path to resolution (rule 24); critical comments are questions (rule 18); does not rewrite the function (rule 14); stays in scope of "is this safe to ship" (rules 5-12); LLM-generated test gap caught (specific to forge-tests rule 28).

## Workflow

When reviewing a PR:

1. **Read the description and the linked issue.**
2. **Pull the branch and run it if the change is non-trivial.**
3. **Read tests first.**
4. **Read the diff in a sensible order. Take notes.**
5. **Leave comments with severity prefixes.**
6. **Decide: approve, approve with comments, request changes.**
7. **State the decision clearly.** "Approving; please address the nits before merge if easy."

## Verification

This skill is about behavior; verification by review. Manual checklist for completed reviews:

- [ ] Read the description before the diff.
- [ ] Read tests before implementation.
- [ ] Comments have severity prefixes where useful.
- [ ] Suggestions explain "why" when non-obvious.
- [ ] Critical feedback framed as questions where possible.
- [ ] Specific praise on something done well.
- [ ] Clear decision: approve, approve with comments, or block.

## When to skip this skill

- Bot-generated PRs (Dependabot, Renovate) - skim, approve if green.
- Auto-generated code (codegen output) - review the codegen config, not the output.
- Drafts marked WIP - they are not asking for review yet.

## Related skills

- [`forge-pr-description`](../forge-pr-description/SKILL.md) - the PR description you are reading first.
- [`forge-commit-messages`](../forge-commit-messages/SKILL.md) - commit hygiene to flag in review.
- [`forge-tests`](../../testing/forge-tests/SKILL.md) - test patterns to flag in review.
- [`forge-naming`](../forge-naming/SKILL.md) - naming smells to flag.