| name | code-review |
| description | Review code against Conor's style guide — naming, progressive disclosure, function design, error handling, agentic patterns, and token economics. Use this skill whenever the user asks for a code review, says things like "review this", "check my code", "look over this PR", "roast this", asks for feedback on a diff/branch/file, or after finishing an implementation and wants a pass before commit. Also trigger proactively after substantial code changes where a self-review would catch issues. Produces a structured findings report grouped by severity (blocker / major / minor / nit) with file:line references and concrete suggested fixes — not generic platitudes. |
Code Review
Review code against the style guide in references/style-guide.md. Produce a structured, severity-tagged findings report that a human can skim in 30 seconds and an agent can act on directly.
What gets reviewed
Default scope, in order of preference:
- Explicit target — if the user names a file, PR, branch, or diff, review exactly that
- Current branch vs
main — git diff main...HEAD when reviewing "my changes" / "this branch"
- Uncommitted changes —
git diff HEAD + git diff --staged when reviewing "what I just wrote"
- Ask — only if genuinely ambiguous. Don't ping-pong.
Skim the diff first, then read surrounding context for any file with non-trivial changes. A review that misses the calling context misses half the bugs.
Output format
Always use this structure. Omit sections that are empty — don't pad with "no issues found".
# Code Review — <what was reviewed>
**Summary**: <1-2 sentences. What changed, overall shape of the feedback.>
## Blockers
<Bugs, security issues, broken contracts, data loss risks. Must fix before merge.>
- **<file>:<line>** — <what's wrong>. <why it matters>. <concrete fix>.
## Major
<Style-guide violations with real cost: hidden dependencies, swallowed errors, leaky boundaries, wrong abstraction.>
## Minor
<Naming, guard clauses, section ordering, missing types. Worth fixing, not urgent.>
## Nits
<Taste-level preferences. Author may ignore.>
## Praise
<1-3 specific things done well. Not filler — skip if there's nothing genuine.>
Each finding is one bullet, one issue, one fix. If you're writing a paragraph, split it.
What to look for
Work through these lenses. Not every lens applies to every review — skip ones that aren't relevant rather than manufacture findings.
1. Correctness (blocker-tier)
- Off-by-one, null/undefined handling, race conditions, unawaited promises
- Silent error swallowing —
catch {}, discarded Result.error, ignored rejections
- Mutations on shared/aliased state
- Idempotency: will a retry duplicate work or corrupt state?
- Boundary validation: is untrusted input validated before it enters the type system?
2. Style-guide adherence
Cross-reference references/style-guide.md. The high-value checks:
- Naming: vague (
data, result, temp), missing units (timeout vs timeoutMs), abbreviations (cfg, cust), booleans without prefix (valid vs isValid), nouns where verbs belong (email() vs sendEmail())
- Guard clauses: nested conditionals that could be flattened, happy path buried >2 levels deep
- Progressive disclosure: helpers above public API, types scattered instead of grouped, file that doesn't answer "what is this?" in first 10 lines
- Explicit dependencies: globals, singletons, ambient imports; things a function uses that aren't in its signature
- Error handling:
throw where Result fits, any/unknown leaking, opaque messages ("Validation failed")
- Single responsibility: functions doing two things,
and in the name, >50 lines without a clear reason
3. Agentic / context hygiene
This is where most reviews miss things:
- Leaky context boundaries: module imports from 4+ siblings to do one job
- Token bloat: functions returning the kitchen sink when callers want a summary
- Unstructured outputs: prose where a discriminated union or typed object belongs
- Missing machine-parseable errors: error strings instead of
{ code, retryable }
- Convention drift: new code that doesn't match established patterns in the same module
4. Minimalism
- Premature abstraction (1-2 call sites, not 3+)
- Speculative generality (options/flags nothing uses)
- Dead code, unused exports, commented-out blocks
- Comments that explain what instead of why (rename the thing instead)
- Backwards-compat shims for code that isn't shipped yet
5. Testing
- Missing tests on new behaviour; tests that assert implementation not behaviour
- Tests coupled to internals (mocking private methods, asserting call counts)
- One test doing five things
- Golden-path-only: no failure case coverage
6. Security & side effects (blocker if present)
- SQL/command/template injection, unescaped user input reaching a sink
- Secrets in code, logs, or commit
- Auth/authz checks missing or bypassable
- Broad
catch that hides security-relevant failures
- Unintended network calls, file writes, or process spawns
7. Commit & PR hygiene (if reviewing a branch/PR)
- Multiple unrelated concerns in one commit/PR
- Commit messages that say what not why
- Drive-by refactors mixed with the core change
Calibrating severity
Err toward fewer, sharper findings over exhaustive lists. A 40-item review gets ignored; a 6-item review gets acted on.
- Blocker: ship this and something breaks — data, auth, contracts, runtime
- Major: real maintenance cost, will bite within weeks
- Minor: cumulative drag — worth a cleanup pass
- Nit: preference, subjective, author's call
If you find yourself marking everything Major, recalibrate. If everything is Nit, you're not looking hard enough.
What NOT to do
- Don't rewrite the code yourself unless the user asked. Point at the problem and suggest the fix direction — let the author own the edit.
- Don't file-by-file walk through everything. Group findings by severity, not by file.
- Don't invent issues to pad the review. Empty sections are fine. "Looks good" is a valid outcome.
- Don't quote huge blocks of the reviewed code back at the author. They wrote it. A
file:line ref is enough.
- Don't moralize. "This is bad practice because..." reads worse than "This will break when X happens — do Y instead."
- Don't review stylistic preferences outside the guide as if they were rules. Mark taste items as Nits and move on.
Process
- Establish scope: which files/diff. If unclear, default to
git diff main...HEAD.
- Skim the full diff once before forming opinions. Avoid first-impression tunnel vision.
- Read surrounding context for any file with meaningful changes — the call sites, the types, the tests.
- Consult
references/style-guide.md when a finding touches a specific principle; cite the section so the author can drill in.
- Draft findings, then cut by 30% — remove anything that wouldn't survive the author asking "so what?".
- Output the report in the format above.
The goal: the author reads the review, knows exactly what to do next, and feels the reviewer actually understood the code.
