mirror of
https://github.com/Sea-Haven-Industries/engineering-handbook.git
synced 2026-09-30 12:43:14 +00:00
896bfc7501
Some checks are pending
ci / ci / ci (push) Waiting to run
docs: align engineering conventions for Cursor migration (PLAT-62)
71 lines
3.6 KiB
Markdown
71 lines
3.6 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 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.
|
|
|
|
## Security Review Gates
|
|
|
|
Two separate gates apply to security-sensitive changes:
|
|
|
|
**Cross-family review (`cross_review.py`):** Required when the change touches IAM roles, IAM policies, or resource permission boundaries. Run the stateless GPT cross-reviewer via `cross_review.py` in the `security-review` repo. It produces findings-to-verify, not a gospel verdict. After two rounds without convergence, stop and disposition the remainder with Adam. Lambda handler signatures are not a cross-review trigger.
|
|
|
|
**Security review:** Required when the change touches sensitive authentication paths, secrets handling, IaC/IAM definitions, payment flows, or surfaces that accept untrusted input. These surfaces warrant a structured security review pass in addition to standard code review.
|
|
|
|
## 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
|