How to review

A review is the last cheap moment you get to move correctness out of the author's head and into the shape of the code, before the diff turns into the thing somebody maintains at 2am who knows less than the author does right now. So review for structure, not taste. A change is good when the wrong thing it could have done is no longer something you can even express in the code, and not because it reads nicely. This is MANIFESTO.md turned into a reading order, and what it's trying to do is the rubric. Does this change minimise what a tired engineer has to hold in their head, while keeping a bounded blast radius for anything an attacker or an unlucky caller controls? Each lens below tells you what you're hunting for and which tenet it serves. When two findings pull against each other, sort it out by who controls the input and how wide the blast radius gets, and split every finding into must-fix and consider before you post it. None of it is free.

Read for locality first

Ask how far from the changed lines you had to look to convince yourself they're correct; the answer is the change's largest tax.

Action-at-a-distance is the heaviest cost there is on changing code later, so it's the first thing to go looking for (I). If convincing yourself the diff is correct meant you had to open the constructor, and the caller, and a global that gets mutated somewhere else, then that distance is the finding, and the next person to touch this pays it all over again. Flag behaviour that depends on context you can't see, and call out the change that derives everything from inputs it actually declares.

Frontend: a component reading props and local state passes, unlike one whose behaviour turns on a useEffect three files away mutating shared context.
Backend: a handler taking request-scoped config passes, unlike one reaching into a process-global the middleware mutated.
The tell: you had to open four files to approve a ten-line change. That distance is the finding.

Tension: This pulls against DRY. Don't demand every helper be inlined to be local; a named, owned shared fact living elsewhere with an explicit dependency is correct (XIV). The finding is hidden distance, not declared distance.

Ask yourself: To approve this, how far from the diff did I have to look, and is that distance the design, or an accident the author can remove?

Audit every boundary the change touches

Find each place control, data, or trust changes hands, and check it parses once, bounds size before allocating, re-verifies authority, and won't break an old consumer.

A boundary the author didn't design is one their bugs went and designed for them. At every crossing, so a request body, a queue message, a deserialisation point, a public response, confirm the input gets parsed into a typed value once (III) and that the value, even once it's well-formed, still got treated as hostile (IV). Size capped before allocation. Identity re-derived from the verified token and not from whatever the payload claims. Array lengths bounded. Then check the contract evolves safely (XV). It has to reject what violates an invariant it depends on, but ignore the field it merely doesn't recognise, and an old client hitting it unchanged must not break.

API: the public response is pinned by a test that fails when a refactor drops a field, unlike the ORM shape leaking straight into the JSON.
Supply chain: the new dependency is pinned and checksummed. A typosquat or poisoned transitive dep is a boundary crossing (IV), not a convenience.
The tell: a userId read from the request body and trusted, a privilege escalation the type can't catch.

Tension: This cuts the other way against locality (I, IV). Don't demand re-validation at every internal hop; that's clutter and a latency tax. Insist on the parse at each real crossing, and on the parsed type carried inward. Internal callers trust the type, not the wire.

Ask yourself: What is the worst single value, or the worst package, the other side could send here, and did the author bound it and re-verify authority before acting?

III

Hunt the silent swallow

The empty catch, the ignored error return, the bare except:: find the path where a failure disappears, because that is a wrong state that has learned to hide.

The most dangerous thing in any diff is the failure you can't see, because there's no stack trace for an error that already got swallowed (XIII). Scan for handling that just throws the failure away: a caught exception with an empty body, an err that gets assigned and then never checked, a promise with no rejection path, a fallback that hides the fault it papered over. Failure has to be visible and impossible to ignore in whatever idiom the language gives you, and where it is handled, it needs to be handled where the context to actually decide lives, not soaked up three frames too early.

Go-style: every (value, err) has the err confronted at the call site, unlike an _ that drops it.
Scripting: set -euo pipefail halts the chain, unlike a failed command ignored while the script charges on, corrupting the next step.
The tell: a try that wraps fifty lines and catches everything into one log-and-continue.

Tension: This is the fail-fast versus degrade balance (XIX). Not every caught error is a bug; a service should degrade rather than crash. The finding is the silent swallow, failure absorbed with no signal and no decision, not a deliberate, logged, observable fallback.

Ask yourself: Is there any path in this diff where a failure vanishes with no error surfaced, no signal emitted, and no decision made?

Find the unbounded thing the caller controls

Look for anything whose size or count an attacker or an unlucky caller sets and the code does not cap. That's a DoS and an OOM, fixable on sight without a profiler.

Most of "measure before optimising" holds up, but there's a carve-out the reviewer has to enforce. Complexity that goes super-linear in caller-controlled or attacker-controlled input isn't a performance question at all, it's a correctness bug and not a tuning question (XVII), because the size is caller- or attacker-controlled (IV) and the ceiling is (VII)'s to set. Find the unbounded allocation that scales off a request field, the array with no length cap, the recursion with no depth limit, the retry with no ceiling. "Dev data was fine" is exactly how it ships. And catch the reliable antipatterns where no profile is needed: the N+1 round-trip, the per-row loop over an op you could vectorise, the per-frame allocation.

Databases: the innocent loop issuing 500 SELECTs per request: batch it into one query; no profiler required.
Backend: a buffer sized from an unbounded request field, the 2 GB body that parses fine and kills the heap.
The tell: a limit that exists for the year's twelve months but not for the user-supplied list.

Tension: This is paid for in measure-first (XVII). Don't let this lens become a licence to demand clever rewrites of cold, readable code nobody profiled; that spends clarity for nothing. It governs only caller-controlled blowups and known antipatterns; the cold 95% stays legible.

Ask yourself: Is any cost here proportional to a size the caller controls with no ceiling, and is it a known antipattern I can ask them to fix without a benchmark?

Find the check-then-act window and the unsafe replay

Where shared state is read then written, ask whether two actors can interleave; where an operation can be retried, ask whether running it twice corrupts.

The race stays invisible in review unless you go and look for it, but it's devastating once it's in production (IX). For every if exists / then create, every if balance / then debit, every "reserve the last one", ask whether concurrent requests, or multiple workers, or overlapping runs can slip in between the check and the act, and if they can, demand atomicity, or serialisation, or an act that's commutative, and chosen where you can see it. Separately, in a world where acks get lost and things get redelivered, check the mutation is keyed by a stable id so a retry hands back the original result instead of charging twice (X). And flag time that's being used as if you could trust it: a wall-clock deadline that can step backwards under an NTP correction, or an ordering that compares timestamps across hosts (XXIII). "It's single-threaded" is the assumption that's wrong most often once you're at scale.

Databases: INSERT ... ON CONFLICT or a unique constraint, unlike SELECT then INSERT with a window between.
Payments: an idempotency key returns the original result on retry, unlike a second charge.
The tell: a "processing" flag two overlapping cron runs both read as false.

Tension: This is waste in single-run code. In genuinely single-actor, single-run code there is no window, so the heavy machinery is just waste. Spend it where two actors can actually interleave, or where a repeat touches money, state, or the outside world, and stay sceptical of "can't happen here".

Ask yourself: Can two actors actually run this between the read and the act, and if a retry replays this exact operation, is the result identical to running it once?

Trace every acquire to its release, and every registration to its teardown

For each resource the change acquires and each listener it registers, find the line that frees it on every exit path, including error and panic, or it's an orphan.

Memory gets reclaimed for you. External resources never do (VII, VIII). So walk each task, timer, subprocess, subscription, file handle, and lock the diff acquires, and confirm that exactly one owner releases it on every path out - the happy return, the early error, the panic. Shutdown is an exit path too, and it's easy to miss: confirm in-flight obligations are drained, buffers flushed, and messages acked before the process goes, rather than left abandoned mid-flight (XII). Then check that each listener registered on setup is torn down on teardown, because otherwise it fires callbacks into a thing that no longer exists. And check that completion is learned from the event where one is guaranteed, with a reconciler sitting behind it for the edge a crash or disconnect emits no event for.

Frontend: the diff adds an addEventListener/subscribe with no matching remove in the same hunk, the missing cleanup keeping a dead component alive and updating.
Backend: defer/try-finally/RAII releasing the pool slot on the error path too, unlike a handle leaked when the function returns early.
The tell: a subscription with no corresponding teardown anywhere in the diff.

Tension: Events are sharp but lossy (VIII). Don't demand a poll where an event is guaranteed delivered; subscribe, for the latency. But where completion is absence - a crash, a hang - insist on a level-triggered reconciler, because a missed edge across a boundary wedges forever.

Ask yourself: Does every resource this acquires get released on every exit path, and does every registration have a teardown, with a reconciler behind any edge that might be missed?

VII

Judge the blast radius and the reversibility

Ask what this change can touch if it's wrong or compromised (XVI), whether failure stays contained (XIX), whether undoing it in a year is a scalpel or a demolition (XX), and whether you would even see the invariant break (XVIII).

Two diffs can be equally correct today and worlds apart in what they cost when wrong. Check least privilege (XVI): if this component were fully compromised, or simply buggy, what's the most it could touch, and does it actually need all of that? Check containment (XIX): when its dependency dies, does it lose a feature or the whole system, and is one tenant's bad day walled off from the rest? Check reversibility (XX): is it shipped behind a flag it can retreat through, sequenced expand-then-contract, or is it a one-shot cutover with no losing move? And is the change observable enough to know when its invariant breaks (XVIII)?

Privilege: the job scoped to one bucket, unlike a broad * grant that turns a compromised dep into an exfiltration.
Reversibility: expand, backfill, switch, then drop, unlike the drop-and-rename in one transaction, a cliff.
The tell: a new flag with no owner and no expiry, a future zombie (XX).

Tension: This trades against simplicity (XXI). Fallbacks, bulkheads, and seams are real complexity, and an untested degraded path is just a second bug. Demand the tier or the seam only where the blast radius justifies it and someone will actually exercise it, not speculative flexibility for a future that may never come.

Ask yourself: If this were fully compromised or simply wrong, what's the most it touches, does failure stay contained, and is the eventual undo a scalpel or a demolition?

VIII

Demand the test of the contract

A change to behaviour with no test that pins it is correctness living in the author's head; require a test of the contract, the boundary and the decision, not the private shape.

A bug that escaped is a missing test, not just a bad commit (XXIV), so where the diff fixes a bug or adds a behaviour, ask for the executable check that re-runs the rule for everyone forever, including call sites that don't exist yet. But insist that it pins the contract - the boundary (XV) and the decision (XI) - not the private shape, or the test turns into a tax that punishes the very refactors this house style wants cheap.

Backend: a test that fails the instant a refactor drops a public field, unlike a test asserting a private helper's internals, which punishes cleanup.
Bugfix: a regression test that reproduces the original report, unlike "fixed it, trust me".
The tell: a behaviour change with a green build and no new or changed test.

Tension: This pulls against refactor cost (XXIV, XX). Tests coupled to implementation tax the refactors the house style wants cheap. Require behaviour pinned at the boundary and the decision, not the internals. A missing test is a real finding, but so is a brittle one.

Ask yourself: Is this new behaviour enforced by something that re-runs without the author, and does the test pin the contract and the decision rather than the private shape?

The through-line

Every lens here is the manifesto's one move read backwards. Verify that correctness was pushed out of the author's vigilance and into the structure: the local function, the parsed boundary, the visible failure, the bounded resource, the released handle, the contained blast radius, the contract pinned by a test. The review's job isn't to admire the diff. It's to confirm the wrong thing can no longer be expressed in it, and where it still can, to say so in the order of blast radius, not the order you happened to notice things.

Review for the person who will maintain this at 2am with a pager going off, knowing less than the author knows now. Block what an attacker or an unlucky caller could turn into a catastrophe; for the rest, name the cost and let it ship, and never wave a slogan where the tiebreaker is who controls the input.