Code review

Code Review

Purpose

Review changes the way a senior engineer does: correctness first, then security, then performance, then design, then style. Every finding carries a severity, a reason, and a fix.

When to Use

• Reviewing a pull request or a local diff before pushing.
• Auditing a branch before merge to a protected branch.
• Reviewing someone else's code in an unfamiliar area of the codebase.

Capabilities

• Correctness analysis: edge cases, off-by-one, null and error paths, concurrency.
• Security review: injection, authorization gaps, secret exposure, unsafe deserialization.
• Performance review: N+1 queries, unbounded allocations, blocking calls on hot paths.
• Design review: coupling, layering violations, missing abstractions, premature ones.
• Test review: whether the tests would actually fail if the code were wrong.

Inputs

• A diff, PR number, or branch reference.
• The surrounding code, not just the changed lines.
• The intent of the change: linked issue, PR description, or a one-line summary.

Outputs

A review with findings grouped by severity:

• Blocking — Correctness, security, or data-loss defects. Must be fixed before merge.
• Should fix — Design or performance problems that will cost more later than now.
• Consider — Improvements that are genuinely optional.

Each finding states the file and line, what is wrong, why it matters, and a suggested fix.

Workflow

1. Understand the intent — Read the PR description and the issue. A change that does something other than what it claims is the first finding.
2. Read the diff in context — Open the surrounding functions. Most defects live in the interaction between changed and unchanged code.
3. Trace the failure paths — For each new branch: what happens when the input is empty, null, malformed, or hostile? What happens when the network call fails?
4. Check the boundaries — Anything crossing a trust boundary (HTTP, queue, file, DB) must be validated on the way in and encoded on the way out.
5. Assess the tests — Would the new tests fail if the implementation were subtly wrong? If not, they are coverage, not verification.
6. Rank and report — Sort findings by severity. Lead with the ones that block.

Best Practices

• Review the code, not the author. Write "this returns before closing the file", not "you forgot to close the file".
• Do not report style issues that a formatter should own. Fix the formatter config instead.
• One blocking finding stated clearly beats twenty nitpicks that bury it.
• If the design is wrong, say so early and stop line-by-line reviewing. Detailed feedback on code that should be deleted wastes everyone's time.
• Approve when the change is better than what exists. Perfection is not the bar; regression is.
• Quote the exact line. A finding without a location is not actionable.

Examples

A well-formed finding:

Blocking — api/orders.py:88 total is computed from request.json["items"] without validating quantities, so a negative quantity produces a negative total and a credit to the customer's card. Validate quantity >= 1 in the request schema, and assert total > 0 before charging.

A finding that is not actionable:

"The order logic feels a bit fragile."

Notes

• Automated tools should catch formatting, unused imports, and obvious lint violations before a human reads the diff. If they are reaching review, fix the pipeline, not the PR.
• Large PRs receive worse reviews. If a diff exceeds roughly 400 lines, reviewing it as one unit is unreliable — ask for it to be split.
• A review that finds nothing is a valid outcome, but say what you checked.