Code Review Guidelines
What reviewers look for in Python pull requests - correctness, tests, security, operability, and team conventions - without turning review into style nitpicks ruff already enforces.
Recipe
Reviewer 5-minute pass:
- Read PR description - ticket link, migration note, rollout plan.
- Scan file list - unexpected deps, lockfile, alembic, infra.
- Read tests first - behavior change covered?
- Spot-check implementation - edge cases, errors, types.
- Comment with questions, not demands - approve when trust + CI green.
When to reach for this:
- Author preparing PR for review
- Reviewer opening notification queue
- Tech lead calibrating review culture
- Retro after escaped defect
Working Example
Good PR description:
## [BILL-91] Apply VAT to EU orders
- Adds TaxService with Decimal math
- Alembic revision `a1b2c3` adds `vat_amount` column (nullable, backfill in follow-up)
- Feature flag `billing.vat_enabled` default off
## Test plan
- [x] uv run pytest tests/billing/test_tax.py
- [x] Manual: create order DE address, flag on, see VAT line
## Rollback
Disable flag; migration reversible via downgrade (drops column - OK pre-prod)Good review comment:
**Question:** What happens for zero-quantity line items - divide by zero or skip?
Suggest asserting in `test_tax_zero_quantity` - happy to approve after that.What this demonstrates:
- Description links ticket, migration, flag, rollback
- Test plan mirrors CI commands
- Review asks question + concrete test suggestion
- Decimal and feature flag called out explicitly
Deep Dive
How It Works
- Author responsibility - Small PRs, self-review diff, green CI before requesting review.
- Reviewer responsibility - Timely response, prioritize risk, approve when bar met.
- CODEOWNERS - Auto-assign domain experts for sensitive paths.
- CI as first reviewer - ruff, pytest, mypy, security scan.
Review Lens (priority order)
| Priority | Check |
|---|---|
| P0 | Correctness, security, data loss risk |
| P1 | Tests, migrations, API contract breaks |
| P2 | Performance, observability (logs/metrics) |
| P3 | Style (if CI missed), naming, docs |
Python-Specific Focus
- Async - Blocking calls in
async def? - Money - float used for currency?
- SQL - Parameterized queries only?
- Pickle/yaml load - Unsafe deserialization?
- Dependencies - New transitive packages justified in lockfile diff?
- Typing - Public API annotated?
Gotchas
- Rubber-stamping green CI - Tests may not cover branch. Fix: read test assertions, not just coverage %.
- Style comments ruff enforces - Wastes author time. Fix: comment only if rule should change in pyproject.
- Blocking on preference - "I would use dict not dataclass". Fix: ask for ADR if architectural.
- Huge PRs - 50 files unreviewable. Fix: author splits; reviewer requests split before deep read.
- Missing migration review - Deploy fails on duplicate revision. Fix: check alembic chain and downgrade note.
- Unkind tone - Drives bus factor. Fix: collaborative phrasing; praise good patterns.
Alternatives
| Alternative | Use When | Don't Use When |
|---|---|---|
| Pair programming | Complex risky change | Async review timezone spread |
| AI review bot | First pass nitpicks | Sole reviewer for security |
| Design doc before code | Large architecture shift | One-line bugfix |
| No review (solo) | Prototype throwaway | Production main |
FAQs
How fast should review happen?
Target first response < 1 business day; SLA per team wiki.
Approve with nits?
Yes if nits are non-blocking and tracked in follow-up ticket optional.
Request changes vs comment?
Request changes when merge would cause prod risk; comment for educational or optional.
Junior reviews senior?
Encouraged - questions improve docs; senior still CODEOWNER on critical paths.
Auto-merge on green?
OK for dependabot patch bumps with policy; not for migrations without human.
Review notebooks?
Prefer paired .py from Jupytext or review outputs-stripped diff only.
Security review trigger?
Auth, crypto, new external input parsers, dependency major bumps - tag security buddy.
Performance review trigger?
Hot path queries, new N+1 ORM loops, large pandas in request path.
Who merges?
Author after approval + green CI unless release manager role for train.
Post-merge monitoring?
Author watches dashboards 30 min after deploy for their change - note in PR template.
Related
- Conventions & Style Guide - idioms
- Git Best Practices - PR hygiene
- Code Review Culture - leadership
- 30 Security Rules - security lens
Stack versions: This page was written for Python 3.14.0 (stable 3.14, maintenance 3.13), FastAPI 0.115+, Django 5.2, Flask 3.1, Pydantic 2, PyTorch 2.6+, pandas 2.2+, Polars 1.x, ruff 0.9+, and uv 0.6+.