Code Review Best Practices: Risk, Feedback and Approval
Improve code review with risk-based scope, useful feedback and explicit approval ownership. Includes a worked review record and a reusable pull-request checklist.
Agree what an approval means
A code review should leave the author and reviewer with a shared understanding of the change, its failure risks and the evidence needed to merge it. An approval should not mean that someone glanced at the diff while relying on another person to check the dangerous part.
Define the review boundary explicitly. A reviewer may own correctness, database compatibility, security or user-facing behavior. If several people divide the work, record that division and identify who confirms that the required reviews are complete.
This guide focuses on the daily review decision. Team-wide principles and decision ownership belong in the broader engineering culture guide.
Google's code-review standard favors changes that improve the codebase without demanding perfection. Apply that distinction carefully: an optional naming preference should not hold up a sound change, while an unresolved data-loss risk needs an explicit decision.
Make the change reviewable before asking for review
Keep a pull request focused on a coherent behavior or preparatory change. A fixed line-count cutoff is a poor substitute for judgment. A small authorization change can require deeper review than a large, mechanically generated update.
Separate unrelated formatting from behavior changes. Keep a migration, compatibility layer and its tests together when separating them would hide the transition. For a larger series, explain the dependency order and what can safely merge or deploy at each step.
Give the reviewer a short evidence packet:
| Field | What the author supplies | |---|---| | Intended behavior | The user or system outcome that changes, with a concrete example. | | Scope | Affected entry points, data, consumers and deliberately excluded work. | | Design context | Relevant contract or decision record, including rejected alternatives where useful. | | Failure risks | What can fail through invalid input, concurrency, partial success or incompatible versions. | | Evidence | Tests and manual checks, tied to the current revision and candidate artifact where relevant. | | Release boundary | Feature exposure, migration order and the conditions under which rollback remains safe. | | Review request | Which expertise is needed and which decisions remain open. |
Passing CI belongs in the packet, but it does not explain whether the tests exercise the changed behavior. Link the results and identify meaningful gaps instead of saying only “all tests pass.”
Review the risky paths before polishing the easy ones
Start with the change's purpose and the surrounding code. Trace an input through the authorization, state transition and external effects it can trigger. Then inspect what happens on failure and retry.
Google's reviewer checklist includes design, behavior, concurrency and test quality. The practical question for a test is whether it would fail for the defect you are concerned about.
Match review depth to the change:
- For an API change, examine old callers, denied actions and error responses.
- For a database change, examine existing data and the period when old and new code coexist.
- For a background job, examine duplicate execution, partial completion and recovery.
- For UI behavior, inspect the running interaction at relevant viewport and accessibility states.
- For generated files, review the source and generator configuration, then inspect the generated result for the expected effect.
A reviewer who cannot assess a specialist risk should ask for the relevant owner. An extra approval from someone unfamiliar with the risk is not a replacement.
Worked review: an invoice lookup across tenants
Consider a hypothetical pull request adding an invoice download endpoint. It authenticates the caller and loads an invoice using a request-supplied invoice ID. The happy-path test uses a caller and invoice from the same tenant.
The defect to investigate is whether the caller can retrieve an invoice belonging to another tenant. A successful login alone does not answer that question. The service needs to enforce access to the specific object, as described by OWASP's object-level authorization guidance.
An unhelpful comment is: “This needs better security.”
A review-ready comment is: “Blocking: the lookup uses the supplied invoice ID, but I cannot find a resource-level authorization check before the file is returned. A caller from tenant A may be able to request an invoice from tenant B. Please enforce access using trusted caller context and add a cross-tenant denial test. If that enforcement happens elsewhere, point me to the path this endpoint uses.”
The author can answer with code and evidence rather than guessing what the reviewer wants.
| Review record | Illustrative resolution | |---|---| | Risk | Authenticated caller requests another tenant's invoice. | | Owner | The maintainer responsible for the invoice access policy. | | Required evidence | Authorized access succeeds; cross-tenant and unauthenticated requests are denied under the API's response policy. | | Fix boundary | Enforce policy before file retrieval or signed-link creation, including alternate entry paths. | | Non-blocking feedback | Rename a local variable for clarity if it improves consistency. | | Approval condition | Reviewer checks the final authorization path and the new negative tests on the revised change. |
This record is reusable: replace the risk, owner, evidence and approval condition for the next review. It is an illustrative example, not a report of an Ampity client defect.
Resolve disagreements with a decision owner
Label feedback by the decision it requests. “Blocking” should identify an unmet requirement or a credible failure scenario. “Suggestion” should make clear that the author may choose another sound approach. Questions can reveal missing context without presuming a defect.
In the invoice example, suppose the author says a shared middleware already enforces ownership. The next step is to trace that middleware and demonstrate the denied case. The disagreement is resolved by evidence, not by the seniority of the person leaving the comment.
If two acceptable designs remain, consult the relevant maintainer or technical lead under the team's decision policy. Record the choice and its consequences in the pull request. If a security concern remains unresolved, route it to the security owner rather than relabeling it as a preference to unblock the merge.
A short discussion can replace a long comment exchange. Preserve the outcome in writing so later maintainers can understand it.
Route reviews without creating a permanent queue
Use ownership rules to find reviewers, then check that the rules work. GitHub's CODEOWNERS documentation distinguishes automatic review requests from settings that require code-owner approval. Listing several owners for a path does not, by itself, require every listed owner to approve.
Have a backup owner for absence or overload. Agree how urgent fixes enter the queue and who can make an exception. An emergency path needs a recorded risk decision, bounded scope and follow-up review, not an informal bypass that becomes normal practice.
Review turnaround targets should reflect working hours, risk and available expertise. Track time waiting for the first substantive response separately from time spent revising the change. A slow review may result from an overloaded owner, an unclear change or a difficult design question; each needs a different response.
Avoid ranking engineers by comment count or approval speed. Those numbers can reward superficial activity.
Close the review on the final change
Before merging, check that material revisions received the required review, unresolved blockers have a recorded disposition and the relevant tests cover the version being merged. An earlier approval does not explain the safety of code added afterward.
After release, use defects and review friction to improve the packet or checklist. If reviewers repeatedly ask the same configuration question, make the answer part of the template. If an incident exposes a missing failure test, add that specific case.
Failure conditions and review tradeoffs
A stricter review can reduce avoidable defects while increasing queue time and concentrating authority. A faster review can improve flow while missing cross-boundary risks. Adjust the evidence and reviewer set to the consequence of the change rather than applying one approval rule to every file.
Stop and escalate when the required owner is unavailable, the change cannot be tested at the relevant boundary, or material revisions arrive after approval. For a low-risk change, use a documented backup owner instead of leaving work indefinitely blocked. For security, data-loss or incompatible-contract concerns, do not trade the required evidence for speed.
For a delivery-quality review with an engineering pod, bring a sanitized recent pull request, its review discussion and the release outcome. Use that evidence to agree what the next approval must establish before changing the team's process.