# AI Code Review — The 5-Layer Review Process

AI-generated code optimizes for "looks good," not "works correctly." It generates plausible
code from patterns; plausible is not correct. **AI code requires more scrutiny than human
code, not less** — treat it like an intern's PR: potentially brilliant, definitely needs
review. You are the senior engineer who catches the landmines before they explode.

## 1. Discovery — look, don't interrogate

Find the code under review yourself. Run **all four** — do not stop at the first that yields:

```bash
git diff --staged          # staged changes — the usual pre-commit case
git diff                   # unstaged working-tree changes
git diff HEAD~1            # the AI's last commit, if it already committed
git status --porcelain     # NEW/UNTRACKED files — invisible to all three diffs above
```

**The untracked trap.** `git diff` cannot see a file that has never been added. AI assistants
create brand-new files constantly, so the most important code in a change is routinely the code
git will not show you. A diff that is mostly prose or config, plus one new untracked module, is
the normal shape of an AI change — and reviewing only the diff means reviewing the packaging and
declaring the engine fine.

Two rules follow:

- **A diff that adds a *reference* to a file pulls that file into scope**, even if git can't show
  it: a new `npm` script, a CI step, an import, a `require`. Follow the reference and review the
  target.
- **If the change adds a runnable entrypoint, run it.** Check first whether it has a read-only or
  `--dry-run` mode; if it does, there is no excuse not to. "It compiles" is not Layer 1. Layer 1
  is "it runs, and it does the thing."

Then read the surrounding context so Layer 2 has something to compare against:

- Read the full body of every file the diff touches (a diff hunk hides the missing try/catch
  three lines above it).
- Glob for `CLAUDE.md`, `CONTRIBUTING.md`, `.eslintrc*`, `.editorconfig`, `tsconfig.json` —
  these are the project's stated conventions.
- List the existing `utils/`, `lib/`, `helpers/` modules. Layer 2's top red flag is
  "reinvents utilities you already have," and you cannot see it without the inventory.

**Only if all three git commands come back empty** ask the user for the code — one message:
"No diff found. Paste the code, or tell me the file/branch to review."

## 2. The 5-Layer Review Process

Review from surface to deep. Run **every** layer — the layers you skip are the layers that
bite. Full checklist in `references/checklist.md`.

### Layer 1: Does It Run? — 30 seconds
Verify: does it compile/parse? · are imports correct? · do the functions it calls exist? ·
can you run the happy path?
Red flags: imports from non-existent libraries · calls to undefined methods · syntax errors
(rare, but they happen) · type errors in strongly-typed languages.

### Layer 2: Does It Follow Patterns? — 2–3 minutes
Verify: does it match the existing file structure? · does it follow naming conventions? ·
does it use established patterns (error handling, logging)? · does it integrate with existing
modules correctly?
**Pattern drift = these four red flags, specifically:** reinvents utilities you already
have · different naming style than the rest of the codebase · ignores existing error-handling
patterns · bypasses established abstractions. Do not report "pattern drift" without naming
which of the four, and the file it drifted from.
This is where **The Copy-Paste Architect** gets caught.

### Layer 3: Does It Handle Errors? — 5 minutes
The critical layer AI most often fails. Check each against its known AI failure mode:

| Check | What to Look For | Common AI Failure |
|---|---|---|
| Null/undefined | Guards against null values | Assumes inputs are always valid |
| Network calls | Try/catch on async operations | Happy path only |
| Database ops | Transaction handling, rollbacks | No error recovery |
| User input | Validation and sanitization | Trusts all input |
| File operations | Checks file exists, handles read errors | Assumes success |

Then test with bad inputs — ask yourself, of the code in front of you:

```
- What if this API returns 500?
- What if the user sends an empty string?
- What if the database connection drops mid-query?
- What if the file doesn't exist?
- What if two users do this simultaneously?
```

Answer all five in writing against the actual diff. "N/A" is a legitimate answer; silence is not.
This is where **The Trusting Optimist** gets caught.

### Layer 4: Is It Secure? — 5–10 minutes
AI generates "typical" code, and a lot of typical code on the internet is insecure. **Always
assume AI output needs security hardening.**

- **SQL Injection:** Does it use parameterized queries or string concatenation?
- **XSS:** Does it sanitize user input before rendering?
- **Authentication:** Does it check permissions before operations?
- **Secrets:** Are API keys/passwords hardcoded, or environment variables?
- **CSRF:** Are state-changing operations protected?

### Layer 5: Is It Maintainable? — 5 minutes
Future-you will need to understand this.

- **Comments:** Are complex sections explained?
- **Function length:** Are functions under 50 lines?
- **Cyclomatic complexity:** Too many nested ifs/loops?
- **Magic numbers:** Are constants defined with names?
- **Testability:** Can you unit test this easily?

This is where **The Magic String Lover** and **The Synchronous Blocker** get caught.

## 3. The four AI code smells

Every review pass explicitly checks for all four. Each has a before/after code pair in
`references/ai-code-smells.md` — quote the "should be" form as the fix, don't paraphrase it.

| Smell | What it is | Caught in |
|---|---|---|
| **The Trusting Optimist** | Assumes every fetch / DB call / file read succeeds | Layer 3 |
| **The Magic String Lover** | Hardcodes `'admin'`, `'success'`, raw status strings instead of constants | Layer 5 |
| **The Copy-Paste Architect** | Duplicates similar blocks instead of extracting shared functions | Layer 2 |
| **The Synchronous Blocker** | `readFileSync` / sync DB calls in async contexts, freezing the event loop | Layer 5 |

## 4. Severity — the Fast Fail Signals

These are the sorting criterion. Anything matching a Fast Fail Signal is **Must Fix**; nothing
else is. Everything remaining is **Should Fix**.

| Fast Fail Signal | Severity | Consequence |
|---|---|---|
| Hardcoded secrets | **Must Fix** | Immediate rejection |
| No error handling | **Must Fix** | Must fix before accepting |
| SQL string concatenation | **Must Fix** | Security risk |
| **The change doesn't do its own job** | **Must Fix** | See below |
| Sync file operations **in an async / request-serving context** | **Should Fix** | Performance issue |

**The fourth signal is the one reviewers miss.** The first three are *code-shape* signals — they
catch bad lines. They are blind to a change that is internally clean and still defeats its own
purpose. Fire this signal when:

- the change **ships a stale build artifact** — it edits a source, and the generated/committed
  output the user actually consumes was never regenerated;
- the change **adds a check that nothing runs** — a linter, a test, a gate that is wired into no
  build, no CI, no `npm` script. It is dead code the day it lands;
- the change **only half-applies** — the new rule covers the case in front of it and silently
  skips its siblings.

Ask it as one question: *"If this merges exactly as written, does the stated goal actually hold?"*
A build script whose purpose is to stop shipping broken downloads, which itself ships broken
downloads, is a **Must Fix** no matter how clean its internals are.

Scope note on sync file ops: a top-level CLI or build script is *supposed* to be synchronous —
there is no event loop to starve. Flagging every `readFileSync` there is cargo-cult, and produces
a wall of false positives. Fire it only where a blocked thread costs someone a response.

Verdict rule: **one or more Must Fix ⇒ `❌ Do not ship`.** Zero Must Fix, some Should Fix ⇒
`⚠️ Ship with follow-ups`. Clean across all five layers ⇒ `✅ Ship`.

## 5. Automated tools

Run whichever the project already has configured, before the manual pass — they clear the
noise so your 15–20 minutes go to logic:

- **ESLint / Pylint** — code style and common bugs
- **TypeScript** — type safety catches many AI errors
- **SonarQube** — security vulnerabilities and code smells
- **Snyk** — dependency vulnerabilities
- **Jest / Pytest** — run tests, check coverage

**But don't rely on tools alone. They catch syntax issues, not logical flaws.** A green test
suite is not a review.

## 6. Output contract

Emit exactly this, filled. **Every finding cites `file:line` and names its layer. No `...`
lines, no empty sections — write "None found." if a section is genuinely empty.** A findings
table with no line numbers is a memo, not a review.

Worked calibration example (the AI login endpoint, end to end): `references/worked-example.md`.

```text
# AI Code Review — src/routes/auth.js (2 files, +48 −3)

## Verdict
❌ Do not ship. Major rewrites needed. 3 Must Fix (2 security, 1 error handling).

## Layer Pass
| Layer | Budget | Result |
|---|---|---|
| 1 · Execution      | 30 sec   | ✅ Syntax correct, imports assumed present. |
| 2 · Patterns       | 2–3 min  | ⚠️  No input validation middleware. Other routes use it. |
| 3 · Error handling | 5 min    | ❌ No try/catch, no user-exists check. |
| 4 · Security       | 5–10 min | ❌ Plaintext password compare; JWT secret hardcoded. |
| 5 · Maintainability| 5 min    | ⚠️  Short but missing comments on security requirements. |

## Must Fix
| file:line | Layer | Finding | Fix |
|---|---|---|---|
| src/routes/auth.js:6 | 4 · Security | JWT secret hardcoded as `'secret123'` — Fast Fail Signal: hardcoded secrets → immediate rejection. | `jwt.sign({ id: user.id }, process.env.JWT_SECRET, { expiresIn: '24h' })` |
| src/routes/auth.js:5 | 4 · Security | Plaintext password comparison `user.password === password`; no hashing. | `const isValid = await bcrypt.compare(password, user.passwordHash);` |
| src/routes/auth.js:3 | 3 · Error handling | The Trusting Optimist — no try/catch on `await User.findOne`, and `user.password` throws on a missing user. Bad-input probe "what if the DB drops mid-query?" is unanswered. | Wrap the handler body in try/catch → `logger.error` + 500; add `if (!user) return res.status(401).json({ error: 'Invalid credentials' });` |

## Should Fix
| file:line | Layer | Finding | Fix |
|---|---|---|---|
| src/routes/auth.js:2 | 2 · Patterns | Bypasses established abstraction: every other route in `src/routes/` mounts `validateInput(schema)`; this one parses `req.body` raw. | Mount `validateInput(loginSchema)` as middleware. |
| src/routes/auth.js:1 | 4 · Security | No rate limiting — brute force possible. | `rateLimit({ max: 5, window: '15m' })` |
| src/routes/auth.js:5 | 4 · Security | Timing attack: different response times for a valid vs. invalid email. | Return the same 401 body/latency on both branches. |
| src/routes/auth.js:1 | 5 · Maintainability | No comments on the security requirements a future editor must not break. | Comment the rate-limit and constant-response intent inline. |

## AI Code Smells Found
- **The Trusting Optimist** — `src/routes/auth.js:3` (unguarded `await`, no `!user` check).
- The Magic String Lover — none found.
- The Copy-Paste Architect — none found.
- The Synchronous Blocker — none found.

## Tests To Add
- `POST /login` with an email that does not exist → expect 401, not a 500 crash.
- `POST /login` with an empty-string password → expect 401.
- `POST /login` 6× in 15 min from one IP → expect 429.
- `User.findOne` rejects (DB down) → expect 500 + a logged error, not an unhandled rejection.
```

## Hard Rules

- **The 10-Minute Rule.** Spend at least 10 minutes on **Layer 4 (security)** alone, regardless
  of how simple the code looks. Most critical bugs hide in "obviously correct" code, and the
  security layer is where they bite hardest in production. **A full pass across all five layers
  takes 15–20 minutes total** — a review that took 90 seconds did not happen.
- **Do not rubber-stamp an AI diff because the tests pass.** Tools catch syntax issues, not
  logical flaws.
- **Hardcoded secrets → immediate rejection.** No "fix it in a follow-up." The verdict is ❌.
- **Never report a finding without `file:line`.** Concrete location or it doesn't go in the table.
- **Functions under 50 lines** is the threshold. Flag anything over it in Layer 5.
- **A human makes the final call on security-sensitive changes.** Your verdict is a
  recommendation; auth, payments, and data-deletion diffs get a named human reviewer.
- All five bad-input probes get an answer in Layer 3. Unanswered = the layer is not done.

## Asking the AI to review its own code

Useful as a *second* pass, never as a replacement for yours:

```text
"Review this code for:
1. Error handling gaps
2. Security vulnerabilities
3. Edge cases not handled
4. Performance issues
5. Maintainability concerns

Be critical. What could go wrong?"
```
