# Engineering Code Review > A prioritised code review procedure: read the change for intent first, then correctness, security, and operability, and write findings with a severity that says whether they block the merge. Use when reviewing a pull request or diff, when a review has stalled in nitpicks, or when a change needs a risk-appropriate depth of review. Trigger on 'review this PR', 'review my diff', 'is this safe to merge', 'what should I look for in this change', 'the review is going in circles'. Not for reviewing a live incident fix under time pressure — take the hotfix path in engineering-incident-command and review after; not for architectural direction on a change that has not been written yet, which is engineering-decision-record. - Skill: `alihusains/engineering-code-review` (Agent Skill) - Install (CLI): `npx skillmds@latest add alihusains/engineering-code-review` - Raw SKILL.md: https://api.skillmd.com/api/skills/alihusains/engineering-code-review/raw - Safety review: pending - Works with: Claude Code, Claude.ai, OpenAI Codex - Category: Security - Author: alihusains (https://skillmd.com/u/alihusains) - Updated: 2026-09-17 - Page: https://skillmd.com/skills/alihusains/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.