dkenge123-oss Code Review
Find the bugs that matter. A review that buries one real defect under twenty nitpicks has failed.
Protocol
- Understand the intent first. Read the PR description, linked issue, or commit messages. You cannot judge correctness without knowing what the change is supposed to do.
- Read the diff twice.
- Pass one: what changed, at the level of behavior
- Pass two: hunt for defects using the checklist below
- Trace, don't assume. For each suspicious line, open the surrounding file. Most false positives come from reviewing a diff hunk without its context.
- Rank findings by severity:
CRITICAL: data loss, security hole, crash on a common pathHIGH: wrong behavior on a realistic input, race condition, resource leakMEDIUM: wrong behavior on an edge case, misleading error handlingLOW: naming, structure, or clarity issues that will cause future bugs
- For every finding, state the failure scenario. "This breaks when X happens and produces Y" beats "this looks wrong". If you cannot construct a concrete failure scenario, downgrade or drop the finding.
Defect checklist
- Off-by-one at boundaries, empty inputs, null or undefined flows
- Error paths: swallowed exceptions, missing rollback, partial writes
- Concurrency: shared state mutated without coordination
- Security: unsanitized input reaching a query, shell, or template
- Contracts: callers of a changed function that were not updated
- Tests: does the new test actually fail without the fix?
Never
- Never pad the review with style opinions unless the author asked for them.
- Never approve a change you did not fully read.
- Never report a finding you could verify but chose not to.
Done means
Every finding has a file, a line, a severity, and a concrete failure scenario. Zero findings is a valid outcome and should be stated with confidence.