Surgical Code Review

# Surgical Code Review A review's job is to find the bug that ships, not to relitigate style. Read in priority order and label every comment so the author knows what blocks merge. ## Process 1. **Read the intent first.** PR description, linked issue, commit messages. If you can't state what the change is supposed to do, ask before reviewing — comments written against a guessed intent waste everyone's time. 2. **Map the diff.** List the touched files and sort them: production logic first, then schema/config, then tests, then docs/generated files. Review in that order. 3. **Correctness pass.** For each logic change, ask: - What inputs make this branch misbehave? (empty, null, zero, negative, duplicate, unicode, huge) - Are error paths handled or silently swallowed? - Off-by-one on every loop bound and slice; timezone and DST on every date; race on every shared mutable read-then-write. - Does the test diff actually exercise the changed lines? Run the tests if you can; "tests exist" is not "tests cover this." 4. **Security pass.** Unsanitized input reaching a query, shell, or template; missing authorization on new endpoints; secrets or tokens in the diff. 5. **Design pass.** Does this change fight the existing architecture? Is there a function 20 lines away that already does this? Flag, but mark severity honestly — design comments are rarely blockers on a small diff. 6. **Style pass — last and lightest.** If a linter could have caught it, suggest configuring the linter instead of commenting. Never let style comments outnumber correctness comments. ## Severity labels (prefix every comment) - **[blocker]** — bug, data loss, security hole, broken contract. Must be fixed before merge. - **[should]** — likely problem or meaningful debt; fix now or file a tracked follow-up. - **[nit]** — preference; author may ignore without replying. - **[question]** — you need information; not a request for change. - **[praise]** — call out one thing done well per review. It calibrates the rest. ## Rules - Quote the exact lines you're commenting on; never comment on the file in general. - Propose a concrete alternative for every [blocker] and [should]. "This is wrong" without "do X instead" is half a comment. - Review the tests as hard as the code: assertions that can't fail, mocks that hide the bug, deleted coverage. - Large diff (>400 changed lines of logic)? Say so and request a split rather than skimming. A rubber-stamped giant diff is worse than a delayed one. - Check what's *missing* from the diff, not just what's in it: the caller that wasn't updated, the second place the copied constant lives, the migration without a rollback, the new config absent from the deploy environment. - End with an explicit verdict: approve, approve-with-nits, or request-changes, plus the one-line reason. ## Self-check before submitting the review - [ ] You can state the PR's intent in one sentence — and your comments address that intent. - [ ] Every [blocker] includes the failing scenario and a proposed fix. - [ ] Correctness comments outnumber style comments. - [ ] You ran the tests, or said explicitly that you reviewed statically only. - [ ] Verdict line present.