Skill: code-reviewer

Trigger: "review this" · "review [file]" · "check this code" · "code review"

Steps

Step 0 — Load project context (before looking at any code)

Check for each of these files and read them if they exist:

memory/lessons.md

• Extract any lines flagged as mistakes, anti-patterns, or corrections
• Add each one as a custom checklist item under a ### Project Lessons section
• Example: "never use addRow on IDENTITY tables" → becomes a checklist item

memory/decisions.md

• Extract locked architectural decisions the code must follow
• Add violations of these as automatic failures regardless of the standard checklist

memory/complexity_profile.md (written by map-codebase)

• Read the detected stack — use it to enable the right checklist sections:
  • SQL / ORM detected → enable Data Layer section
  • Auth middleware detected → enable API / Endpoints section
  • Frontend JS detected → enable JS Patterns section
  • Python detected → enable Python Patterns section
  • Java detected → enable Java Patterns section

If none of these files exist yet: skip Step 0, run the standard checklist only, and note at the top: No project context loaded — running generic checklist. Run map-codebase and a session to sharpen this.

Step 1 — Read the file(s)

Read the file(s) being reviewed in full. If a file is too large, grep for the patterns most likely to fail first.

Step 2 — Run checklist

Run every applicable section. Report every issue with file + line number. Report clean sections explicitly — never skip them silently.

Standard Checklist

Security

• No SQL / command injection — user input is quoted or parameterized before DB/shell use
• No secrets or API keys hardcoded
• Auth checks present on protected routes/endpoints
• User input validated at system boundaries (not trusted internally)

Correctness

• Null / undefined / empty checks before dereferencing
• Array bounds safe — no off-by-one
• Error paths return or throw — never fall through silently
• Async operations awaited or .then()-chained correctly (JS) / try-catch present (Java/Python)

Data Layer (enabled if SQL/ORM detected)

• No SELECT * — explicit column list
• Single-row queries check for results before accessing
• Loop queries don't issue N+1 DB calls — pre-load with a map if needed
• INSERT/UPDATE includes required audit fields (created_at, created_by, org/tenant id)

API / Endpoints (enabled if auth/routing detected)

• Public endpoints registered correctly (auth-bypass list if applicable)
• Both success and error paths return a valid response
• Error messages don't expose stack traces or internal details
• Response shape matches what the frontend expects

JS Patterns (enabled if frontend JS detected)

• No var in module scope leaking globals unintentionally
• DOM access uses getElementById / querySelector — not jQuery unless project uses it
• No inline event handlers in dynamically generated HTML
• User data escaped before innerHTML — never raw interpolation

Java Patterns (enabled if Java detected)

• No raw types — generics used properly
• Resources closed in finally or try-with-resources
• No swallowed exceptions (catch (Exception e) {})
• Null checks before dereferencing

Python Patterns (enabled if Python detected)

• No mutable default arguments (def f(x=[]))
• with used for file/connection handling
• No bare except: — always catch specific exceptions

Production Survival (always on — AI-generated code fails here most)

• Silent error swallowing — catch blocks that return null, log nothing, and send no alert. Failures must surface. return null in a catch is a red flag.
• Race conditions — concurrent writes to shared state, unguarded async flows, double-submission on payment/booking/signup. Check: can two requests hit this at the same time?
• Naive retry logic — retries with no backoff (fixed for i < 3 loop). Under load this hammers a degraded API. Requires: exponential backoff + jitter + max delay cap.
• Missing idempotency — charge/webhook/insert that can run twice and cause a double-charge or duplicate record. Check: is there a uniqueness guard, a processed flag, or an idempotency key?
• State assumptions — code that assumes clean initial state (empty arrays, zero counters, fresh DB). Production has 3 years of dirty edge cases. Check: what happens if this runs on an existing record?

Code Quality

• No duplicate logic that already exists elsewhere — reuse helpers
• No hardcoded values that should be constants or config
• Exceptions caught and handled — no swallowed errors
• Resources closed after use

Project Lessons (populated from lessons.md at runtime)

This section is built dynamically from your project's memory. Empty on first session.

Report Format

Context loaded: lessons.md (N patterns), decisions.md (N decisions), stack: [detected]

❌ [file]:[line] — [issue]
   Fix: [specific fix]

✅ [section] — clean

End with: N issues found or Clean — no issues.

Notes

• Security and correctness first — style is secondary
• If no project context exists yet, say so at the top — don't silently skip it
• After finding a new pattern, suggest adding it to lessons.md via /learn