engineering-handbook/code-review.md

Code Review

Purpose

Code review exists to catch defects, share knowledge, and maintain consistency. It is not a gatekeeping exercise.

What Reviewers Should Look For

Always check

• Does the change do what the PR description says it does?
• Are there obvious bugs, edge cases, or error handling gaps?
• Does it follow naming conventions?
• Are secrets handled correctly per secrets-and-config.md?
• If AWS resources changed, are Lambda defaults correct per aws-infrastructure.md?

Watch for

• Unused code, dead imports, or leftover debug statements
• Missing or outdated README updates for functionality changes
• Hardcoded values that should be in config or Secrets Manager
• Security concerns (input validation, injection, overly broad IAM policies)

Don't nitpick

• Minor formatting differences handled by linters
• Personal style preferences that don't affect correctness
• Naming choices that are reasonable even if you'd pick something different

Giving Feedback

• Be specific. "This might fail if the list is empty" is useful. "Needs work" is not.
• Distinguish between must-fix and suggestions. Prefix optional feedback with "nit:" or "suggestion:"
• Ask questions instead of making assumptions. "Is this intentional?" is better than "This is wrong."
• If a PR is good, say so. A simple "Looks good" is fine.

Deferred Findings

When a reviewer identifies a finding that won't be addressed in the current PR, the PR author must file a Jira ticket in the appropriate project (DEV, PLAT, or SEC) before the PR merges. No exceptions — if it's worth commenting on, it's worth tracking.

Exception: Repos with GitHub Issues enabled (contractor intake repos such as shoc-backend and shoc-frontend-new, and open-source fork repos) may use a GitHub issue instead.

Requirements

• The Jira ticket (or GitHub issue, where applicable) must reference the PR number and link to the specific review comment.
• The PR author must reply to the review comment with a link to the created ticket, acknowledging the deferral.
• This applies to all severity levels: bugs, nits, refactors, missing tests, documentation gaps.

Why

Deferred findings handled informally (retro notes, mental to-do lists, "we'll get to it") fall through the cracks. A ticket in the Jira backlog is the minimum bar for accountability.

Turnaround

• Aim to review within one business day of being requested
• If you can't review in time, say so and suggest another reviewer
• Don't let PRs sit in review for days without feedback

Approving

• Approve when you're confident the change is correct and complete
• If you left suggestions but the PR is otherwise good, approve with comments rather than blocking
• One approval is sufficient for most changes