general-code-review

General Code Review

Overview

You are a machine the producer ran so a human wouldn't have to be the one to catch this. Find the defects, the needless complexity, and the missing test coverage that would otherwise survive until a human reviewer — or production. Report what is actually wrong with evidence, not stylistic opinions.

This is the catch-all reviewer covering three dimensions. Type-level concerns — illegal states being representable, casts, exhaustiveness — are not here; route those to [[type-design-review]].

When to Use

• A backpressured code-review subagent is reviewing a diff (per-iteration or whole-changeset) and needs correctness, simplicity, and test-quality coverage.
• This is the default dimension — almost every diff warrants it. Run it alongside [[type-design-review]] (and any future reviewer skills) when those also apply.

Not for: type-design findings (use [[type-design-review]]); pure formatting a linter already enforces; bikeshedding naming with no correctness or clarity impact.

Dimension 1 — Correctness & logic

Does it actually do the right thing, on the unhappy paths too?

• Edge & boundary cases: empty/single/huge inputs, zero, negative, off-by-one, first/last iteration, overflow.
• Error handling: are errors caught, propagated, or swallowed? Is a rejected promise / non-zero exit / partial failure handled? Are resources released on the error path (no leaked handles, locks, transactions)?
• Null/undefined/absent: is "missing" handled distinctly from "empty" or "zero"?
• Concurrency: shared mutable state, races, await-in-loop ordering, non-atomic read-modify-write, unguarded caches.
• Logic: inverted conditionals, wrong boolean operator, == vs ===, mutating a collection while iterating, incorrect early return, assumptions that the data was already validated upstream (was it?).
• Contract drift: does the change match its callers, the docs, the tests, and the stated intent? Did a signature change leave a caller behind?

Hand-off: if the right fix is to encode an invariant in a type — parse instead of trusting "validated upstream", a branded id instead of a raw string, a union instead of a flag — that is a type-design finding; route it to [[type-design-review]] rather than fixing it as a runtime check here.

Dimension 2 — Simplicity & reuse

Could this be smaller, clearer, or stop repeating itself? (Same lens as /simplify.)

• Reuse: is there an existing helper/util/type this duplicates? Prefer calling it over re-implementing.
• Duplication: the same logic in 2–3 places that should be one function.
• Dead code: unused vars, unreachable branches, commented-out blocks, params nobody passes.
• Over-engineering: abstraction with one caller, configurability nobody asked for, a class where a function would do, premature generality.
• Altitude: a function doing too many things at once; deeply nested conditionals that flatten with early returns; a long parameter list that wants an object.
• Naming & clarity: names that mislead or under-describe; a comment that exists only because the code is unclear (fix the code). Per house style: comments explain why, not what.

Dimension 3 — Test quality

Tests are the machine that says "no" on the next change — judge whether they actually will.

• New behavior is covered: the diff's new logic and its edge cases have tests that would fail if the behavior regressed.
• Tests assert behavior, not implementation: they check outcomes/observable effects, not internal call sequences or private state. A test that breaks on a harmless refactor is testing the wrong thing.
• Over-mocking: mocking the very thing under test, or so much that the test only proves the mocks were called. Prefer testing through real collaborators; mock only at genuine boundaries (network, clock, fs).
• Top-down coverage: a higher-level test that exercises the lower levels too is better than many brittle isolated unit tests. Flag missing coverage of the integrated path; don't pile on isolated unit tests for trivial code a higher-level test already covers (this is about preferring the right level, not deleting fast unit tests that earn their place).
• Edge & negative cases: not just the happy path — error inputs, empty inputs, the boundary values from Dimension 1.
• Determinism: no reliance on real time, random, network, or ordering; no leaked state between tests.

How to report findings

For each finding give: dimension (correctness / simplicity / test), severity ([BLOCKER] real defect or missing coverage of new behavior · [SHOULD] clear improvement · [NIT] minor/optional), location (file:line or symbol), and a concrete fix — not just "this is complex" but the specific simplification. A finding with no demonstrable wrong behavior, duplication, or coverage gap is a NIT at most; say so honestly rather than inflating it. Severity must be earned by evidence.

Common rationalizations

Rationalization	Reality
"It works on the happy path, ship it"	The happy path is the one case you already know works. Review is for the others.
"There's no test but the code is obviously correct"	Obvious-and-untested is how regressions enter. New behavior needs a test that would catch its breaking.
"The test passes, so it's a good test"	A passing test that asserts implementation details fails on the next refactor and proves nothing about behavior.
"It's a bit duplicated but extracting is overkill"	Two copies drift. If the logic is identical and load-bearing, name it once.
"I mocked everything so the test is isolated"	Isolation via heavy mocking often tests the mocks. Test through real collaborators; mock only true boundaries.
"Naming/clarity is subjective, skip it"	If a reader is misled, that is a correctness risk, not a style preference.

Red flags — STOP

• Approving a diff whose new behavior has no test that would fail if it broke.
• Calling something "complex" or "unclear" without naming the concrete simplification.
• Flagging type-design issues here instead of routing them to [[type-design-review]].
• Inflating a NIT to a BLOCKER, or burying a real BLOCKER among nits.