lucasfcosta/backpressured/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

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.