Engineering Code Review

Code review

Purpose

Most review comments are cheap to make and cheap to ignore: style opinions, naming preferences, and speculative refactors. The expensive defects — a concurrency bug, a missing authorisation check, an unbounded query, a migration that cannot be rolled back — get missed because attention was spent earlier in the diff on things a formatter should have caught. This skill sets the order of attention, the depth appropriate to the risk, and a severity vocabulary that makes "blocking" unambiguous.

Prerequisites

• Inputs: the diff (all commits, not just the latest), the change description stating intent, and the linked ticket or issue.
• Access: ability to read surrounding code, not just the diff hunks — a review confined to changed lines cannot see what the change broke.
• Expected: automated checks (build, tests, lint, type check) already green. If they are red, stop and say so — human review before the machine review passes wastes the reviewer and usually gets redone.

Procedure

1. Establish intent before reading code. State in one sentence what this change is supposed to do and what should be observably different afterwards. If you cannot from the description and ticket, that is the first finding — ask before reviewing. Reviewing a change whose purpose you inferred produces confidently wrong comments.

2. Set review depth from the risk table. Depth is a property of what the code touches, not of the diff size.

   Change touches	Depth	Must include
   Auth, permissions, secrets, crypto	Deep	Second reviewer with security focus; trace every path that reaches the check
   Money, billing, pricing, ledger	Deep	Arithmetic and rounding reviewed explicitly; idempotency of every write
   Personal or regulated data	Deep	Confirm what is logged, stored, and exported
   Schema or data migration	Deep	Rollback path; behaviour while old and new code run together
   Public API or contract	Standard+	Backwards compatibility; versioning; consumers enumerated
   Internal logic, UI, tests, docs	Standard	Correctness and readability
   Generated, vendored, or formatting-only	Light	Confirm it is what it claims; do not read line by line

3. Read in this order and do not reorder it. Each pass is cheap only because the previous one narrowed the field.

   1. Intent match — does the change do what it says, and only that? Scope creep hidden in a diff is how unreviewed changes ship.
   2. Correctness at the boundaries — empty, null, zero, one, maximum, duplicate, out-of-order, concurrent, retried. Most defects live here.
   3. Failure behaviour — what happens when the dependency times out, returns a partial result, or returns success with an empty body? An error that is caught and logged but not propagated is a silent failure, and it is a finding.
   4. Security — untrusted input reaching a query, a path, a template, a deserializer, or a shell; authorisation checked at the resource, not just the route; secrets absent from code and logs.
   5. Operability — can someone diagnose this at 3am? Is the new failure mode visible in monitoring? Is there an unbounded loop, query, or retry?
   6. Tests — do they fail if the change is reverted? A test that passes against both old and new behaviour tests nothing.
   7. Readability — last, and briefly.

4. Apply the severity vocabulary. Every comment carries one, and the author is entitled to act on that meaning.

   Severity	Meaning	Merge
   Blocking	Data loss, security defect, or incorrect behaviour under a reachable input	No
   Should-fix	Real bug or maintainability cost, but bounded and recoverable	Author's call, before merge preferred
   Consider	A better approach exists; the current one is not wrong	Non-blocking
   Nit	Style or preference	Non-blocking; prefix nit: and cap at three per review

   A comment without a severity defaults to Consider. If everything is blocking, nothing is.

5. Write findings so they can be acted on. Each one names the location, what goes wrong, and the concrete input or sequence that makes it go wrong. "This could be a race" is not reviewable. "Two concurrent requests both pass the existence check before either inserts; the second gets a constraint violation instead of the intended no-op" is.

6. Resolve disagreement by cost, not by rank. If author and reviewer disagree twice on the same point, stop commenting and decide: is this reversible cheaply after merge? If yes, merge and file a ticket. If no, escalate to a third party or promote it to an architecture decision. Threads longer than two round trips are a signal to move to a call.

7. State the verdict explicitly. Approve, approve-with-comments, or request-changes, plus one line on what you did not review ("I did not verify the migration against production-sized data"). Unstated review gaps get read as coverage.

Failure modes this skill exists to prevent

• Nitpick saturation. Twenty style comments and no one checked the authorisation path. Step 3's ordering and the three-nit cap exist for this.
• Diff-tunnel vision. The changed lines are correct; the caller three files away now passes an argument that is silently ignored. Read call sites.
• Rubber-stamp on large diffs. A 2,000-line diff gets "LGTM" because it is unreadable. Correct response: ask for it to be split, or declare which parts you reviewed and which you did not.
• Tests that assert the implementation. Mock-heavy tests that mirror the code structure pass through refactors and fail to catch behaviour changes.

Data handling

Classification: Internal. Never approve a change that adds credentials, tokens, keys, or personal data to source, fixtures, or test snapshots — that is Blocking, and if such a value has already been committed, treat it as exposed and say so: it must be rotated, not just deleted. If a reviewer needs production data to evaluate a change, that is a signal the change needs a safe test fixture, not a data export.

Boundaries

• The change has not been written and the question is which approach to take — engineering-decision-record.
• The change is an emergency fix during a live incident — engineering-incident-command sets the reduced bar; this skill applies to the follow-up review.
• The concern is query cost or correctness in SQL specifically — data-analytics-sql-review goes deeper on plans, joins, and scan cost.
• The change requires a controlled production window or CAB approval — it-change-management.

Hand-offs

• Receives from: engineering-decision-record (the accepted approach the change should implement); engineering-incident-postmortem (remediation changes needing deep review).
• Routes to: it-change-management for release scheduling; data-analytics-sql-review for query-heavy diffs.