mirror of
https://github.com/Sea-Haven-Industries/engineering-handbook.git
synced 2026-09-30 08:03:16 +00:00
7d5d04985a
Require PR authors to create a GitHub issue for any review finding deferred past the current PR, and link it in the review thread before merging. Prevents informal tracking from dropping items.
61 lines
2.5 KiB
Markdown
61 lines
2.5 KiB
Markdown
# 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](naming-conventions.md)?
|
|
- Are secrets handled correctly per [secrets-and-config.md](secrets-and-config.md)?
|
|
- If AWS resources changed, are Lambda defaults correct per [aws-infrastructure.md](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 create a GitHub issue for it before the PR merges. No exceptions -- if it's worth commenting on, it's worth tracking.
|
|
|
|
### Requirements
|
|
|
|
- The GitHub issue 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 issue, 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. An issue in the 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
|