code-auditor — a real code review, not a vibe check

When to use this skill

Trigger when the user wants a judgment call on code quality. Strong signals:

• "review this", "audit this codebase", "find bugs", "what's wrong with this PR"
• A pasted diff with no further instructions
• After a long implementation session, before commit
• "is this safe to ship?"

Do not trigger for: pure style/formatting (linters do that), generating fixes (use refactor-master), or for security-only reviews where the user explicitly wants OWASP coverage (use security-sentinel).

The output contract

A report with:

1. Verdict — one line: safe to ship / needs changes / blocks merge
2. Critical issues — bugs, security, data loss risks. Each with file:line, what's wrong, how to fix, why it matters
3. High-priority — performance, correctness edge cases, broken contracts
4. Medium — maintainability problems that will hurt the next person
5. Notes — stylistic suggestions, optional improvements

If there are zero criticals and zero highs, say so. Don't manufacture findings to fill the report.

Workflow

1 — Scope

Ask, or infer:

• Is this a diff review (compare against main) or a full-file audit?
• Which areas matter most: correctness, security, performance, maintainability?
• Is there a target reader (the author, the team lead, a release gate)?

If reviewing a diff, run git diff <base>...HEAD first. Don't audit the whole repo when the user only changed one function.

2 — Read the code with intent

Walk the changed code with these lenses:

Correctness lens — what does this function promise, and does it deliver?

• Off-by-one in loops or slicing
• Null/undefined paths the type system doesn't cover (e.g. JSON parse, regex match)
• Async without await; awaits in loops where Promise.all should be
• Mutated inputs (especially arrays and objects passed by reference)
• Race conditions: shared state, parallel writes, double-clicks, retry on top of side effects
• Date/time/timezone handling
• Float comparison without epsilon

Security lens — what's the attack surface?

• User input flowing into SQL, shell, file paths, HTML, regex, JSON, YAML
• Auth checks missing on new endpoints
• Secrets in code, env files committed, logs, error messages
• CORS too permissive; CSRF on state-changing forms
• Open redirects, SSRF via user-controlled URLs
• Trust boundaries crossed without revalidation

Performance lens — what's wasteful?

• N+1 queries (any .map(async ...) over IDs that hits the DB)
• Unbounded loops, recursion without depth limit
• Synchronous file/IO on hot paths
• Unmemoized renders, re-instantiated event handlers in JSX
• Loading entire collections to compute a count/exists check

Maintainability lens — will the next engineer cry?

• Functions over ~60 lines or with > 3 levels of nesting
• Boolean flags that should be enums
• Comments explaining what (delete) vs why (keep)
• Dead code; unreachable branches; commented-out blocks
• Names that lie (getUser that creates a user, validate that mutates)

3 — Cross-check assumptions

For each finding, before writing it up:

• Read the surrounding context. Sometimes the "bug" is intentional.
• Search for callers. A function that looks unsafe may only be called from a safe site.
• Check tests. If the behavior is covered, the finding may be by design.

4 — Rank and write

Order findings by blast radius, not by where they appear in the file. A silent data-loss bug on line 412 beats a missing prop type on line 3.

For each finding, write:

[SEVERITY] file:line — one-line title
  What: the actual problem in 1–2 sentences
  Why it matters: the user-visible impact (data loss, security, perf)
  Fix: the smallest change that resolves it (or a sketch in 3 lines of code)

Don't suggest rewrites unless the file is genuinely beyond repair. Patch-shaped feedback gets fixed; rewrite suggestions get ignored.

5 — Verdict

End with the one-line verdict. Be decisive. "Looks fine, ship it" is a valid output. So is "Don't merge — issue #2 will corrupt user data on retry."

Patterns and anti-patterns

✅ Do:

• Cite file:line. Reviews without citations don't get acted on.
• Group findings by file when there are many. Easier to address as a batch.
• Compliment the good parts when they're genuinely good — anchors the trust the rest of the review needs.

❌ Don't:

• Don't pad with nits to look thorough. Three real findings beat thirty fake ones.
• Don't review style if the repo has a formatter. The formatter will do it.
• Don't make confident security claims without tracing the data flow. Hedge if you're not sure: "this may be SQL injection — please verify the ORM is parameterizing here."
• Don't suggest a refactor in a bug-fix review. Note it separately.

Example invocation

User: "Review this PR for the new password reset endpoint." [pastes diff]

1. Scope: a diff review, security-sensitive, ~80 lines across 3 files.
2. Read with correctness + security lenses prioritized.
3. Findings:
   • [CRITICAL] src/auth/reset.ts:42 — token comparison uses ===, not crypto.timingSafeEqual. Timing-attack vulnerable. Use crypto.timingSafeEqual(Buffer.from(a), Buffer.from(b)).
   • [HIGH] src/auth/reset.ts:67 — no rate limit on the request endpoint. Attacker can enumerate emails. Add the existing rateLimit('auth.reset', { max: 5, window: '15m' }) middleware.
   • [MED] src/auth/reset.ts:88 — error message reveals whether the email exists. Return the same response for "sent" and "not found".
   • [NOTE] Token TTL is hard-coded to 1 hour. Consider extracting to config/auth.ts so it's tunable.
4. Verdict: blocks merge — fix the timing-safe compare and the rate limit. Other items can land in a follow-up.

See also

• security-sentinel — when the review is purely security-focused (OWASP sweep)
• refactor-master — to actually apply the structural fixes the audit suggests
• test-architect — to add coverage for the edge cases the audit found