mirror of
https://github.com/Sea-Haven-Industries/engineering-handbook.git
synced 2026-09-30 09:13:14 +00:00
735b407017
Partner engineering teams need the standards without access to this repo, which contains internal repo names, account identifiers, and migration history. Adds eight simplified pages, one per Confluence page, plus an internal README with source mapping and exclusions. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UP6j3hYgoVqjKZwC3XB9ay
105 lines
3.1 KiB
Markdown
105 lines
3.1 KiB
Markdown
# Pull Requests and Code Review
|
|
|
|
## Scope
|
|
|
|
One PR is one logical change. If the title needs the word "and", split it.
|
|
|
|
| Good scope | Too broad |
|
|
|---|---|
|
|
| Add receipt parser Lambda | Add receipt parser and refactor auth middleware |
|
|
| Fix timeout in payment processor | Fix timeout and update dependencies |
|
|
|
|
## Title
|
|
|
|
Use the commit format plus the Jira key at the end:
|
|
|
|
```
|
|
type(scope): description (DEV-123)
|
|
```
|
|
|
|
- Valid Jira keys start with `DEV`, `PLAT`, or `SEC`.
|
|
- Keep the title at or under 120 characters.
|
|
- Describe the change, not the ticket.
|
|
|
|
| Good | Bad |
|
|
|---|---|
|
|
| `feat(parser): add retry logic for upstream failures (DEV-42)` | `DEV-42` |
|
|
| `fix(auth): correct null check in session handler (PLAT-7)` | `Bug fix` |
|
|
|
|
Only automated dependency-update PRs may leave out the Jira key.
|
|
|
|
## Description
|
|
|
|
Use exactly these four sections, in this order. If a section has nothing to say, write `None.` instead of deleting it.
|
|
|
|
```markdown
|
|
## Summary
|
|
What changed and why, in 1-3 sentences.
|
|
|
|
## Validation
|
|
How you verified it works: steps, commands, screenshots for UI changes.
|
|
|
|
## Tests
|
|
Tests added, updated, or run. If there are no automated tests, describe the manual testing.
|
|
|
|
## Notes
|
|
Deploy order, migrations, follow-ups, breaking changes. Otherwise `None.`
|
|
```
|
|
|
|
State facts a reviewer can check. Do not add AI-tool attribution footers.
|
|
|
|
## When to open a PR
|
|
|
|
Open a PR when the work is ready for review and you have verified it works. Do not use PRs to park unfinished work.
|
|
|
|
## Reviewing
|
|
|
|
### Always check
|
|
|
|
- Does the change do what the description says?
|
|
- Are there bugs, missed edge cases, or gaps in error handling?
|
|
- Are naming and secrets handling correct?
|
|
- Are there hardcoded values that belong in configuration?
|
|
- Are IAM permissions as narrow as they can be? Is input validated at the boundaries?
|
|
- Is the README updated when behavior changed?
|
|
- Are tests appropriate for the change?
|
|
|
|
### Do not nitpick
|
|
|
|
- Formatting that linters handle
|
|
- Personal style preferences
|
|
- Reasonable names you would have chosen differently
|
|
|
|
### Finding categories
|
|
|
|
Label each review comment with a category:
|
|
|
|
| Category | Meaning | Author must act? |
|
|
|---|---|---|
|
|
| BLOCK | Must fix before merge | Yes, the PR cannot merge |
|
|
| FIX | Strongly recommended | Yes, unless deferred to a ticket |
|
|
| NIT | Optional improvement | No, author's choice |
|
|
| QUESTION | Reviewer needs clarification | Yes, answer it |
|
|
|
|
### Giving feedback
|
|
|
|
- Be specific. "This fails when the list is empty" helps. "Needs work" does not.
|
|
- Ask rather than assume: "Is this intentional?"
|
|
- If the PR is good, say so.
|
|
|
|
### Deferred findings
|
|
|
|
If a finding will not be fixed in the current PR, the author files a Jira ticket before merging, links it from the review comment, and references the PR in the ticket. This applies to every finding category.
|
|
|
|
### Turnaround
|
|
|
|
- Review within one business day of the request.
|
|
- If you cannot, say so and suggest another reviewer.
|
|
- One approval is enough for most changes.
|
|
|
|
## Merging
|
|
|
|
- Squash-merge branches with messy interim commits.
|
|
- Use a regular merge for branches whose commit history is clean and meaningful.
|
|
- The PR must have green CI and an approval.
|
|
- Delete the branch after merging.
|