michaelsvanbeek/code-review

Code Review Standards

When to Use

• Reviewing a pull request or diff
• Preparing your own code for review
• Writing constructive review comments
• Establishing team review conventions
• Checking code against project standards before merge

---

Review Priorities

Review in this order — stop at the first category with issues before nitpicking lower ones:

| Priority | Category | Questions to ask | |----------|----------|-----------------| | 1 | Correctness | Does it do what it claims? Are edge cases handled? | | 2 | Security | Input validation? Secret exposure? Injection? Access control? | | 3 | Design | Right abstraction level? Follows existing patterns? Over-engineered? | | 4 | Reliability | Error handling? Logging? Graceful degradation? | | 5 | Performance | Obvious N+1 queries, unbounded loops, or missing indexes? | | 6 | Readability | Clear names? Minimal complexity? Would a new team member understand? | | 7 | Style | Formatting, naming conventions, import order (should be caught by linters) |

Don't review style manually — that's what linters and formatters enforce in CI. If CI passes lint, style is not a review concern.

---

What CI Should Catch (Not Reviewers)

These should fail the build automatically — don't waste review time on them:

| Check | CI tool | Reference | |-------|---------|-----------| | Code formatting | Ruff format / Prettier | linting skills | | Lint violations | Ruff / ESLint | linting skills | | Type errors | mypy / TypeScript strict | python / typescript skills | | Test failures | pytest / vitest | testing skill | | Coverage regression | pytest-cov --cov-fail-under | cicd skill | | Known vulnerabilities | pip audit / npm audit | security skill |

---

Reviewer Checklist

Use as a mental model during review. Not every point applies to every PR.

Logic & Correctness

• [ ] Changes match the stated intent (PR description / issue).
• [ ] Edge cases are handled (empty inputs, nulls, boundary values, concurrent access).
• [ ] Error paths log meaningful context and return appropriate codes.
• [ ] No silent failures — errors are either handled or propagated, never swallowed.

Security

• [ ] No hardcoded secrets, API keys, or credentials.
• [ ] External input is validated and sanitized.
• [ ] SQL queries use parameterized statements (no string interpolation).
• [ ] File paths are not constructed from user input without validation.
• [ ] New dependencies don't introduce known vulnerabilities.

Design

• [ ] Changes follow existing patterns in the codebase — not a new pattern for one use.
• [ ] No unnecessary abstractions (no new helper class for one call site).
• [ ] Public API surface is minimal — don't expose internals that could change.
• [ ] Side effects are explicit and contained (not hidden in utility functions).

Testing

• [ ] New behavior has tests. Bug fixes include a regression test.
• [ ] Tests verify behavior, not implementation details.
• [ ] Mocks are used for external dependencies only — not for the code under test.
• [ ] Test names describe the expected behavior as a specification.

Operations

• [ ] Logging is present for significant operations and error paths.
• [ ] Configuration uses environment variables with sensible defaults.
• [ ] Database migrations have a down/rollback path.
• [ ] Deploy is backward-compatible (no breaking change without migration plan).

---

Writing Review Comments

Be Specific

# Bad
"This doesn't look right."

# Good
"This will throw a KeyError if `user_data` doesn't contain 'email'.
Consider using `user_data.get('email')` or validating the field upstream."

Categorize Comments

Prefix comments to signal intent:

| Prefix | Meaning | |--------|---------| | blocker: | Must fix before merge. Correctness, security, or data loss risk. | | suggestion: | Improvement idea. Take it or leave it. | | question: | Genuine question — I don't understand the intent. | | nit: | Minor style preference. Not worth blocking on. | | praise: | Something done well. Positive reinforcement matters. |

Rules for Reviewers

• Approve when it's good enough, not when it's perfect. Perfect is the enemy of shipped.
• Limit to one round of blocker feedback — if you keep finding new blockers, the PR is too large.
• Don't rewrite their code — describe the problem and trust the author to solve it.
• Respond within one business day — stale PRs are costly.
• Prefer questions over commands — "Have you considered X?" is better than "Do X."

Rules for Authors

• Keep PRs small — under 400 lines of meaningful diff. Split large changes into stacked PRs.
• Write a clear PR description — what changed, why, and how to test it.
• Self-review before requesting — read your own diff. Catch the obvious stuff yourself.
• Don't force-push after review — reviewers lose context. Use fixup commits, squash at merge.

---

PR Size Guidelines

| Lines changed | Assessment | Action | |--------------|------------|--------| | < 100 | Ideal | Fast to review, easy to understand | | 100 – 400 | Acceptable | Should have a clear description | | 400 – 800 | Too large | Split if possible | | > 800 | Unacceptable | Must be split. No reviewer can effectively review this. |

Exceptions: auto-generated code, migrations, dependency lockfile updates.

---

PR Description Template

## What

Brief description of the change.

## Why

Link to issue or explain the motivation.

## How

Key design decisions and approach taken.

## Testing

How to verify the change works:
- [ ] Unit tests pass
- [ ] Manual testing steps (if applicable)

## Checklist

- [ ] Lint / format passes
- [ ] Tests added for new behavior
- [ ] No secrets in code
- [ ] Documentation updated (if user-facing)