Article chapter 05 of 08
Review the riskiest behaviour first
Start with the task and its acceptance criteria, then follow the requested behaviour into the code. A diff read on its own can't tell you whether it does what was approved.
For an authorisation change, I'd look at the identity and scope checks before any interface text. For a migration, I'd look at reversibility, locks and whether data survives before I looked at generated model updates. For an external action, I'd check idempotency and how it handles uncertain outcomes before I cared about the success response.
Read the tests like production code. Check that they fail for the original defect or actually protect the requested behaviour. Look out for assertions that just repeat whatever the new implementation outputs. And check the nearby opposite cases: unauthorised users, rejected inputs, duplicate events, empty state, stale versions and boundary values.
An order that wastes less attention:
- Confirm the task is still needed and the scope matches it.
- Check the automated results from a clean environment.
- Look at the highest-risk behaviour and its tests.
- Review the design and how it fits with the code around it.
- Go through mechanical and generated changes.
- Run the manual or deployed-environment checks the task names.
- Accept it, ask for a bounded revision, or close it.
Try not to drip-feed small comments while a big design problem is still open. Raise the blocking issue first and say what evidence you'd need to accept it. Agents can respond to lots of comments very quickly, so it's easy to get round after round of revisions that never settle the decision.
For unfamiliar code, bring in an owner who knows how it runs in practice. Automated analysis can point out patterns and inconsistencies, but it might not know why some awkward constraint is there. If that knowledge isn't in the repository, write it down during review so the next task starts from a better place.