@@ -110,18 +110,18 @@ Small, focused changes are easier to review, faster to merge, and safer to deplo
...
@@ -110,18 +110,18 @@ Small, focused changes are easier to review, faster to merge, and safer to deplo
~1000 lines changed → Too large. Split it.
~1000 lines changed → Too large. Split it.
```
```
**Watch file size, not just diff size.** A small diff can still push a file past a healthy boundary — around 1000 *total* lines in a single file (distinct from the ~1000 *changed*-lines threshold above) is a common inspection signal, not a hard cap. When a change materially grows an already-large file, ask whether to extract helpers, subcomponents, or modules *first*, before piling more on. Decompose, then add.
**Watch file size, not just diff size.** A small diff can still push a file past a healthy boundary — around 1000 _total_ lines in a single file (distinct from the ~1000 _changed_-lines threshold above) is a common inspection signal, not a hard cap. When a change materially grows an already-large file, ask whether to extract helpers, subcomponents, or modules _first_, before piling more on. Decompose, then add.
**What counts as "one change":** A single self-contained modification that addresses one thing, includes related tests, and keeps the system functional after submission. One part of a feature — not the whole feature.
**What counts as "one change":** A single self-contained modification that addresses one thing, includes related tests, and keeps the system functional after submission. One part of a feature — not the whole feature.
**Splitting strategies when a change is too large:**
**Splitting strategies when a change is too large:**
| **Vertical**| Break into smaller full-stack slices of the feature | Feature work |
| **Vertical** | Break into smaller full-stack slices of the feature | Feature work |
**When large changes are acceptable:** Complete file deletions and automated refactoring where the reviewer only needs to verify intent, not every line.
**When large changes are acceptable:** Complete file deletions and automated refactoring where the reviewer only needs to verify intent, not every line.
...
@@ -178,17 +178,17 @@ For each file changed:
...
@@ -178,17 +178,17 @@ For each file changed:
Label every comment with its severity so the author knows what's required vs optional:
Label every comment with its severity so the author knows what's required vs optional:
| **Optional:** / **Consider:** | Suggestion | Worth considering but not required |
| **Optional:** / **Consider:** | Suggestion | Worth considering but not required |
| **FYI**| Informational only | No action needed — context for future reference |
| **FYI** | Informational only | No action needed — context for future reference |
This prevents authors from treating all feedback as mandatory and wasting time on optional suggestions.
This prevents authors from treating all feedback as mandatory and wasting time on optional suggestions.
**Lead with what matters.** Order findings by leverage: correctness and security first, then structural regressions and missed simplifications, then everything else. Don't bury a real issue under cosmetic nits — a few high-conviction comments beat a long list. If you have one structural problem and ten nits, the structural problem *is* the review.
**Lead with what matters.** Order findings by leverage: correctness and security first, then structural regressions and missed simplifications, then everything else. Don't bury a real issue under cosmetic nits — a few high-conviction comments beat a long list. If you have one structural problem and ten nits, the structural problem _is_ the review.
### Step 5: Verify the Verification
### Step 5: Verify the Verification
...
@@ -222,6 +222,7 @@ Human makes the final call
...
@@ -222,6 +222,7 @@ Human makes the final call
This catches issues that a single model might miss — different models have different blind spots.
This catches issues that a single model might miss — different models have different blind spots.
**Example prompt for a review agent:**
**Example prompt for a review agent:**
```
```
Review this code change for correctness, security, and adherence to
Review this code change for correctness, security, and adherence to
our project conventions. The spec says [X]. The change should [Y].
our project conventions. The spec says [X]. The change should [Y].
...
@@ -281,6 +282,7 @@ When reviewing code — whether written by you, another agent, or a human:
...
@@ -281,6 +282,7 @@ When reviewing code — whether written by you, another agent, or a human:
Part of code review is dependency review:
Part of code review is dependency review:
**Before adding any dependency:**
**Before adding any dependency:**
1. Does the existing stack solve this? (Often it does.)
1. Does the existing stack solve this? (Often it does.)
2. How large is the dependency? (Check bundle impact.)
2. How large is the dependency? (Check bundle impact.)
3. Is it actively maintained? (Check last commit, open issues.)
3. Is it actively maintained? (Check last commit, open issues.)
...
@@ -293,11 +295,11 @@ Part of code review is dependency review:
...
@@ -293,11 +295,11 @@ Part of code review is dependency review:
1.**Read the changelog, not just the version number.** Semver is a promise the maintainer may not have kept — a "patch" can carry a behavioral change. For a major bump, read the migration notes and find what breaks.
1.**Read the changelog, not just the version number.** Semver is a promise the maintainer may not have kept — a "patch" can carry a behavioral change. For a major bump, read the migration notes and find what breaks.
2.**One dependency per change.** Upgrade and merge them individually (or in small related groups). When a bulk bump breaks the build, you've lost which package did it; a single-package change makes the cause obvious and the revert clean.
2.**One dependency per change.** Upgrade and merge them individually (or in small related groups). When a bulk bump breaks the build, you've lost which package did it; a single-package change makes the cause obvious and the revert clean.
3.**Let the tests decide.** The upgrade is verified by a green suite before *and* after, not by "it installed." If coverage around the dependency's behavior is thin, that gap is the real finding — add a test first.
3.**Let the tests decide.** The upgrade is verified by a green suite before _and_ after, not by "it installed." If coverage around the dependency's behavior is thin, that gap is the real finding — add a test first.
4.**Mind the transitive graph.** Most installed packages are ones nobody chose directly. Review the lockfile diff, not just `package.json`; a single direct bump can pull in dozens of indirect changes.
4.**Mind the transitive graph.** Most installed packages are ones nobody chose directly. Review the lockfile diff, not just `package.json`; a single direct bump can pull in dozens of indirect changes.
5.**Keep the lockfile honest.** Commit it, review its diff, and never hand-edit it. The lockfile is the thing that actually pins what ships.
5.**Keep the lockfile honest.** Commit it, review its diff, and never hand-edit it. The lockfile is the thing that actually pins what ships.
For triaging `npm audit` findings and supply-chain risk (typosquatting, compromised maintainers), follow the `security-and-hardening` skill — this section covers the upgrade *workflow*, that one covers the security verdict.
For triaging `npm audit` findings and supply-chain risk (typosquatting, compromised maintainers), follow the `security-and-hardening` skill — this section covers the upgrade _workflow_, that one covers the security verdict.
## The Review Checklist
## The Review Checklist
...
@@ -305,20 +307,24 @@ For triaging `npm audit` findings and supply-chain risk (typosquatting, compromi
...
@@ -305,20 +307,24 @@ For triaging `npm audit` findings and supply-chain risk (typosquatting, compromi
## Review: [PR/Change title]
## Review: [PR/Change title]
### Context
### Context
-[] I understand what this change does and why
-[] I understand what this change does and why
### Correctness
### Correctness
-[] Change matches spec/task requirements
-[] Change matches spec/task requirements
-[] Edge cases handled
-[] Edge cases handled
-[] Error paths handled
-[] Error paths handled
-[] Tests cover the change adequately
-[] Tests cover the change adequately
### Readability
### Readability
-[] Names are clear and consistent
-[] Names are clear and consistent
-[] Logic is straightforward
-[] Logic is straightforward
-[] No unnecessary complexity
-[] No unnecessary complexity
### Architecture
### Architecture
-[] Follows existing patterns
-[] Follows existing patterns
-[] No unnecessary coupling or dependencies
-[] No unnecessary coupling or dependencies
-[] Appropriate abstraction level
-[] Appropriate abstraction level
...
@@ -326,6 +332,7 @@ For triaging `npm audit` findings and supply-chain risk (typosquatting, compromi
...
@@ -326,6 +332,7 @@ For triaging `npm audit` findings and supply-chain risk (typosquatting, compromi
-[] No feature logic in shared modules; file stays within a healthy size
-[] No feature logic in shared modules; file stays within a healthy size
### Security
### Security
-[] No secrets in code
-[] No secrets in code
-[] Input validated at boundaries
-[] Input validated at boundaries
-[] No injection vulnerabilities
-[] No injection vulnerabilities
...
@@ -333,19 +340,23 @@ For triaging `npm audit` findings and supply-chain risk (typosquatting, compromi
...
@@ -333,19 +340,23 @@ For triaging `npm audit` findings and supply-chain risk (typosquatting, compromi
-[] External data sources treated as untrusted
-[] External data sources treated as untrusted
### Performance
### Performance
-[] No N+1 patterns
-[] No N+1 patterns
-[] No unbounded operations
-[] No unbounded operations
-[] Pagination on list endpoints
-[] Pagination on list endpoints
### Verification
### Verification
-[] Tests pass
-[] Tests pass
-[] Build succeeds
-[] Build succeeds
-[] Manual verification done (if applicable)
-[] Manual verification done (if applicable)
### Verdict
### Verdict
-[]**Approve** — Ready to merge
-[]**Approve** — Ready to merge
-[]**Request changes** — Issues must be addressed
-[]**Request changes** — Issues must be addressed
```
```
## See Also
## See Also
- For detailed security review guidance, see `../../references/security-checklist.md`
- For detailed security review guidance, see `../../references/security-checklist.md`
...
@@ -353,17 +364,17 @@ For triaging `npm audit` findings and supply-chain risk (typosquatting, compromi
...
@@ -353,17 +364,17 @@ For triaging `npm audit` findings and supply-chain risk (typosquatting, compromi
| "It works, that's good enough" | Working code that's unreadable, insecure, or architecturally wrong creates debt that compounds. |
| "It works, that's good enough" | Working code that's unreadable, insecure, or architecturally wrong creates debt that compounds. |
| "I wrote it, so I know it's correct" | Authors are blind to their own assumptions. Every change benefits from another set of eyes. |
| "I wrote it, so I know it's correct" | Authors are blind to their own assumptions. Every change benefits from another set of eyes. |
| "We'll clean it up later" | Later never comes. The review is the quality gate — use it. Require cleanup before merge, not after. |
| "We'll clean it up later" | Later never comes. The review is the quality gate — use it. Require cleanup before merge, not after. |
| "AI-generated code is probably fine" | AI code needs more scrutiny, not less. It's confident and plausible, even when wrong. |
| "AI-generated code is probably fine" | AI code needs more scrutiny, not less. It's confident and plausible, even when wrong. |
| "The tests pass, so it's good" | Tests are necessary but not sufficient. They don't catch architecture problems, security issues, or readability concerns. |
| "The tests pass, so it's good" | Tests are necessary but not sufficient. They don't catch architecture problems, security issues, or readability concerns. |
| "The refactor makes it cleaner" | Relocating complexity isn't reducing it. If the reader still holds the same number of concepts, the structure didn't improve — look for the version where branches disappear. |
| "The refactor makes it cleaner" | Relocating complexity isn't reducing it. If the reader still holds the same number of concepts, the structure didn't improve — look for the version where branches disappear. |
| "It's only a small addition to this file" | Small diffs still push files past a healthy size and bolt branches onto unrelated flows. Judge the resulting structure, not the diff size. |
| "It's only a small addition to this file" | Small diffs still push files past a healthy size and bolt branches onto unrelated flows. Judge the resulting structure, not the diff size. |
| "It's just a version bump" | A bump is a behavior change you didn't write. Read the changelog; semver doesn't guarantee no breakage. |
| "It's just a version bump" | A bump is a behavior change you didn't write. Read the changelog; semver doesn't guarantee no breakage. |
| "I'll upgrade everything in one PR to save time" | A bulk bump that breaks the build hides which package did it. One dependency per change keeps the cause and the revert clean. |
| "I'll upgrade everything in one PR to save time" | A bulk bump that breaks the build hides which package did it. One dependency per change keeps the cause and the revert clean. |