Code Review Guidelines¶

Status: 🟢 Active  |  Owner: Engineering Enablement  |  Last Reviewed: 2025-Q4

Purpose¶

Code review is a collaborative quality tool, not a gatekeeping exercise. Its purposes, in order of importance, are:

1. Knowledge sharing — reviewing code is one of the best ways to spread understanding of the codebase across the team.
2. Early defect detection — catching logic errors, edge cases, and security issues before they reach production.
3. Standards enforcement — ensuring consistency with the team's agreed coding standards and architectural patterns.
4. Learning and mentorship — both reviewers and authors learn through the review process.

Code review should never be used as a mechanism for control, for exerting technical authority, or for demonstrating superiority. A review culture where engineers fear submitting code is a dysfunctional review culture.

Reviewer Responsibilities¶

What to Review¶

What Not to Block Merges On¶

• Pure style preferences that aren't covered by the style guide. If it's not in the standard, it's not a blocker.
• "I would have done it differently" — if the author's approach is correct and consistent with standards, personal preference is not a blocker.
• Minor naming suggestions that don't affect clarity significantly. Raise them as non-blocking comments.

Comment Classification¶

Use prefixes to communicate the urgency and blocking status of your comments:

# Examples
blocker: This will throw a NullPointerException if `order.getCustomer()` returns null.
         Add a null check or use Optional.

suggestion: Consider extracting this into a private method to improve readability.

nit: Minor — `customerData` could be renamed `customer` here since the type makes it clear.

praise: Really clean implementation of the retry logic — much better than the previous approach.

Author Responsibilities¶

Before requesting review:

• Self-review the diff — catch your own issues before asking others.
• Ensure CI is green.
• PR description is complete — what it does, why, how to test it.
• PR size is reasonable (see Pull Request Standards).
• Add inline comments on any code that might look confusing to a reviewer.

When responding to review comments:

• Acknowledge every comment — even if you disagree, respond to each one.
• If you make a change in response to a comment, mark it resolved after pushing.
• If you disagree with a blocker, discuss it in the PR comment thread — not in Slack. Keep the decision auditable.
• Do not merge while there are unresolved blocker: or concern: comments.

Reviewer Behaviour Standards¶

Response Time¶

• Reviews must be started within 1 business day of being requested.
• A reviewer who cannot complete a review in time should reassign it or communicate.
• If no review after 1 business day, the author may tag the team in #engineering to find an available reviewer.

Tone¶

• Be kind. Review the code, not the person.
• Use "we" and "the code" rather than "you": "This function does X" rather than "You did X wrong".
• Ask questions rather than making accusations: "Could this throw if the list is empty?" rather than "This will throw if the list is empty and you haven't handled that."
• When making a suggestion, explain why — not just what. "Extract this into a constant (magic numbers make it harder to understand the business intent)" is more useful than "extract this constant".

Approve When Ready¶

Do not sit on an approval. If the code is ready to merge (or ready after minor changes you trust the author to make), approve it. Waiting for the "perfect" solution at the cost of the author's time is a tax on delivery.

Team Code Review Norms¶

Pair on Complex Reviews¶

For large, complex, or security-sensitive PRs, consider pairing with the author on a short screen share rather than asynchronous written review. This is faster and produces better outcomes for complex changes.

Two-Review Rule for Critical Services¶

Services marked as "critical" in the service registry require a minimum of 2 approvals before merge. The second reviewer must be independent of the PR author's direct team.

No Self-Merge (except in specific conditions)¶

Engineers may not approve and merge their own PRs except: - Emergency hotfixes during an active incident (must be reviewed post-facto within 24 hours). - Trivial documentation updates with no code changes. - Automated dependency update PRs that have passed full CI.

Automated Review Assistance¶

The following automated checks run on every PR and are not a substitute for human review, but should be reviewed alongside the PR:

References¶

• Pull Request Standards
• Git Standards & Commit Conventions
• Coding Standards

Last reviewed: 2025-Q4  |  Owner: Engineering Enablement