OKHP3 Code review
A review is a search for the ways this change fails in production, ranked by how badly. It is not a style pass. Style is the lint job's work — if you spend the review on formatting, you have used up the reader's attention on the cheapest possible findings.
The bar for reporting: could you write the input that makes this go wrong? If not, you have a question, not a finding. Say it as a question.
1. Understand the intent before reading the diff
Read the PR description, linked issue, or ask. Reviewing a change without knowing its goal produces suggestions that fight the author's purpose.
Then decide what should have changed. Files you expected to see and don't are the highest- yield finding in any review — a new state field with no migration, a new branch with no test, a new error path with no logging.
Done when: you can state the intent in a sentence, and you have a list of what you expect the diff to touch.
2. Read the change in dependency order
Not top to bottom, and not alphabetically, the order the tool shows you is meaningless. Start at the data model or interface, then the logic, then the callers, then the tests. Meaning flows that direction and reviewing against the flow means re-reading everything twice.
Done when: you have read every changed hunk, including the ones that look boring.
3. Hunt in priority order
Work down this list. Stop going deeper once you have found something serious enough to block — a review that reports one real bug promptly beats a complete review delivered late.
Correctness
- Off-by-one, inverted condition, wrong default, wrong variable in a copy-pasted block
- Error paths — what happens when the call fails, returns empty, times out, returns partial?
- Boundaries — empty, one, many, null, zero, negative, maximum, duplicate, out of order
- Concurrency — shared mutable state, non-atomic read-modify-write, assumed ordering
Blast radius
- Is it reversible? A schema change, a data migration, or a published API is not
- Does it change behaviour for existing callers or stored data?
- Is it behind a flag, and does the off path actually work?
Interface
- Can this be called wrongly and still compile? Make the wrong call unrepresentable
- Are new failure modes visible to the caller, or swallowed?
- Does the name say what it does, including its surprises?
Maintainability: real, but never blocks a correct change on its own
- Would a stranger understand why, not just what? Comment the why, delete the what
- Is a concept now defined in two places?
Tests
- Does a test exist for the behaviour that changed, and would it fail without the change?
- Does it test through the public surface, or is it welded to internals?
Done when: you have walked all six categories or found a blocking issue.
4. Verify before reporting
For each candidate finding, construct the concrete failure: the input, the state, the resulting wrong behaviour. Then check the surrounding code — a guard three lines up, a validated caller, a type that makes it impossible.
Roughly half of first-pass findings die here. Verifying costs a minute; a wrong finding costs the author twenty and costs you their attention on the next review.
Done when: every finding has a concrete failure path you have checked against the code.
5. Report ranked, with the fix
Order by severity, most severe first. Never by file order — the reader stops partway down, so the ordering decides what actually gets fixed.
For each finding: where, what breaks, the input that breaks it, what to do.
**`parser.py:88` — Off-by-one drops the final record**
`range(len(rows) - 1)` skips the last row. A 3-row upload writes 2 rows, silently.
Use `range(len(rows))`.
Separate what blocks from what doesn't. Label suggestions as suggestions — an unlabelled preference reads as a requirement and turns review into a negotiation.
If you found nothing serious, say that plainly. "No blocking issues; two suggestions below" is a complete and useful review. Inventing findings to look thorough is the fastest way to make people stop asking for reviews.
What not to do
- Don't rewrite the change. Review the change in front of you. "I would have done this differently" is not a finding unless the difference is a defect.
- Don't report what a linter reports. Formatting, unused imports, import order — automate those, ignore them here.
- Don't stack minor findings to look thorough. Fifteen nits bury the one real bug.
- Don't assume the author is wrong. Unfamiliar code is usually unfamiliar, not incorrect. Ask before asserting.