Code review
Review the change, not the author, and report only what you can defend with
evidence from the code in front of you.
Method
- Understand the intent first. Read the description, the tests, and the
shape of the diff before judging a line. A "bug" that is deliberate
behavior wastes everyone's time; state the intent back in one sentence
before your first finding.
- Read in execution order, not file order. Follow a request, event, or
call from entry to exit through the changed code. Bugs live on the paths
between hunks, not inside them.
- Hunt in this priority order:
- Correctness: logic inversions, off-by-one, unhandled error and
null
paths, race conditions, resource leaks, broken invariants.
- Security: unvalidated input reaching queries, paths, URLs, or shell;
secrets in code; authz checks missing on new surfaces.
- Reliability: what happens when the network call fails, the file is
missing, the input is empty, the list has one item, or two callers
arrive at once.
- Maintainability: misleading names, dead code, duplication of an
existing helper, missing tests for the risky branch.
- Verify before you report. For each candidate finding, construct the
concrete failing scenario: the input or state that triggers it and the
wrong result that follows. If you cannot construct one, it is a question,
not a finding: ask it as a question.
- Rank by severity, worst first. Blocker (data loss, security,
crash), Major (wrong behavior on realistic input), Minor (confusing but
correct), Nit (style). Never bury a Blocker under ten Nits.
Reporting format
For each finding: file:line: one-sentence defect: the failing scenario,
a suggested fix. Lead with the verdict: "N findings, worst is X" so the
reader knows the stakes in the first line. If the change is good, say so
plainly and stop; inventing findings to look thorough is a defect in the
review, not the code.
Boundaries
- Do not review style a formatter or linter already enforces.
- Do not demand rewrites of working code that is merely not how you would
write it; that is preference, and it goes at Nit level or not at all.
- If the diff is too large to follow execution paths, say that first: "this
needs splitting to review honestly" is a legitimate top finding.
1---2name: code-review3description: Review code changes for correctness, security, and maintainability with severity-ranked, evidence-based findings. Use when asked to review a diff, a pull request, or a file before merge.4---56# Code review78Review the change, not the author, and report only what you can defend with9evidence from the code in front of you.1011## Method12131. **Understand the intent first.** Read the description, the tests, and the14 shape of the diff before judging a line. A "bug" that is deliberate15 behavior wastes everyone's time; state the intent back in one sentence16 before your first finding.172. **Read in execution order, not file order.** Follow a request, event, or18 call from entry to exit through the changed code. Bugs live on the paths19 between hunks, not inside them.203. **Hunt in this priority order:**21 - Correctness: logic inversions, off-by-one, unhandled error and `null`22 paths, race conditions, resource leaks, broken invariants.23 - Security: unvalidated input reaching queries, paths, URLs, or shell;24 secrets in code; authz checks missing on new surfaces.25 - Reliability: what happens when the network call fails, the file is26 missing, the input is empty, the list has one item, or two callers27 arrive at once.28 - Maintainability: misleading names, dead code, duplication of an29 existing helper, missing tests for the risky branch.304. **Verify before you report.** For each candidate finding, construct the31 concrete failing scenario: the input or state that triggers it and the32 wrong result that follows. If you cannot construct one, it is a question,33 not a finding: ask it as a question.345. **Rank by severity, worst first.** Blocker (data loss, security,35 crash), Major (wrong behavior on realistic input), Minor (confusing but36 correct), Nit (style). Never bury a Blocker under ten Nits.3738## Reporting format3940For each finding: **file:line: one-sentence defect: the failing scenario,41a suggested fix.** Lead with the verdict: "N findings, worst is X" so the42reader knows the stakes in the first line. If the change is good, say so43plainly and stop; inventing findings to look thorough is a defect in the44review, not the code.4546## Boundaries4748- Do not review style a formatter or linter already enforces.49- Do not demand rewrites of working code that is merely not how you would50 write it; that is preference, and it goes at Nit level or not at all.51- If the diff is too large to follow execution paths, say that first: "this52 needs splitting to review honestly" is a legitimate top finding.
Amey-Thakur