Quality & Team

Code Review

Foundational

Code review builds quality, security, and shared understanding into the team. It is not a gate to argue past. Done well, it catches bugs early, spreads knowledge, and raises everyone's standard. Done badly, it is either a rubber stamp or a fight. Review the code, not the person. Review as if you will maintain it yourself.

A review has two equally important jobs: improve the change, and improve the team. The reviewer is a second pair of eyes on correctness, security, and design. The reviewer is also a teacher who explains the why, so the lesson sticks. The author helps by sending small, clear, self-explained changes. The goal is shared ownership of quality. Once code is merged, it belongs to all of us.

Review is also a real security control. It is where a missing tenant filter, an unparameterised query, a fail-open path, or a leaked secret gets caught before production. So security-relevant changes need careful, specific attention, not a quick LGTM. Keep the tone kind and collaborative. Reviews that feel like attacks make people defensive and hide problems.

Review well

DoReview for correctness, security, and design first. Let automated tools handle formatting and linting, so people focus on what matters.
DoGive specific attention to the high-risk parts: authorisation and tenant scoping, input validation, fail-closed behaviour, secrets, and money or regulated logic.
DoExplain the why behind each comment and teach, do not just point. Mark which issues block the merge and which are optional suggestions (for example, 'nit:').
DoBe kind and specific. Critique the code, assume good intent, and praise good work as often as you flag problems.
ConsiderPulling the branch and running it for anything subtle. Some bugs only show when you actually run the change.
AlwaysBlock the merge on a real security, correctness, or compliance defect. A fail-open path, missing tenant scope, or leaked secret is never a 'fix later'.

The rubber stamp

Reviewer: "LGTM 👍"  (1,200-line PR, mixed refactor + new feature,
          a new query with no tenant filter buried in the middle)

The change is too big to review properly, so it is approved without real checking. A cross-tenant data leak ships because nobody read the query. The review added no protection.

Specific, teaching, blocking where it matters

"blocking: this query is missing the TenantId predicate — as written
 it returns other tenants' customers. We enforce tenant scope server-side
 (see Multi-Tenancy). Suggest routing through TenantRepository."
"nit: name could be clearer, non-blocking."

The serious issue is clearly blocked and explained, with the why and a fix. The small issue is marked optional. The author learns, and the leak never ships.

Make your change reviewable

DoKeep changes small and focused, with one logical change per PR, so they can be reviewed properly rather than skimmed.
DoWrite a clear description: what changed, why, how you tested it, and anything the reviewer should look at closely.
DoRespond to feedback calmly, and follow up on every comment. Resolve it, fix it, or discuss it, but do not ignore it.
ConsiderReviewing your own diff first. You will catch the obvious things and leave the reviewer free for the important ones.
Do notSend large, mixed-purpose PRs that reviewers can only skim. Big diffs get rubber-stamped, and that is where bugs slip through.
NeverApprove a change you do not understand, or merge code that has not passed review and the automated security and quality checks.

Self-review checklist

AskAs reviewer: do I understand this change well enough to approve it?
AskHave I specifically checked the security-critical parts: authorisation, tenant scope, validation, and secrets?
AskAs author: is this PR small and clear enough to be reviewed properly?
AskIs my feedback about the code, explained, and kind? Or could it read as an attack?

Why it matters: Review is one of the cheapest places to catch expensive mistakes. A security flaw found in a PR costs a comment; the same flaw in production costs an incident. Review is also how standards and knowledge spread across the team. Careful, kind, real review builds quality over time. A culture of rubber stamps quietly lets defects through.