Review Verification Protocol
This protocol MUST be followed before reporting any code review finding. Skipping these steps leads to false positives that waste developer time and erode trust in reviews.
Hard gates (sequence)
Apply once per finding before it may appear in the review. If a gate fails, omit the finding, downgrade to Informational (per Severity Calibration), or rephrase as a question—do not ship soft accusations.
| Step |
What you do |
Pass condition (objective) |
| 1. Anchor |
Read the full enclosing symbol or module, not only the diff hunk. |
You can state file path and line range (or symbol name + file) you are judging. |
| 2. Evidence |
For this finding’s type, run the checks in Verification by Issue Type. |
Each required check has an artifact: pasted tool output, file:line citation, or explicit "none" / "N matches" after a repo search—not a claim you "looked." |
| 3. Severity |
Assign severity using Severity Calibration. |
Label matches the table; requests for net-new code that did not exist in scope are Informational only. |
| 4. Format |
Draft the finding for the report. |
Matches [FILE:LINE] ISSUE_TITLE; Informational items do not add to the actionable count. |
Style-only or preference items must fail gate 2 or map to Do NOT Flag At All—they do not get a severity.
Pre-Report Verification Checklist
Before flagging ANY issue, verify (these items are what gate 2 must produce evidence for):
Verification by Issue Type
"Unused Variable/Function"
Before flagging, you MUST:
- Search for ALL references in the codebase (grep/find)
- Check if it's exported and used by external consumers
- Check if it's used via reflection, decorators, or dynamic dispatch
- Verify it's not a callback passed to a framework
Common false positives:
- State setters in React (may trigger re-renders even if value appears unused)
- Variables used in templates/JSX
- Exports used by consuming packages
"Missing Validation/Error Handling"
Before flagging, you MUST:
- Check if validation exists at a higher level (caller, middleware, route handler)
- Check if the framework provides validation (Pydantic, Zod, TypeScript)
- Verify the "missing" check isn't present in a different form
Common false positives:
- Framework already validates (FastAPI + Pydantic, React Hook Form)
- Parent component validates before passing props
- Error boundary catches at higher level
"Type Assertion/Unsafe Cast"
Before flagging, you MUST:
- Confirm it's actually an assertion, not an annotation
- Check if the type is narrowed by runtime checks before the point
- Verify if framework guarantees the type (loader data, form data)
Valid patterns often flagged incorrectly:
// Type annotation, NOT assertion
const data: UserData = await loader()
// Type narrowing makes this safe
if (isUser(data)) {
data.name // TypeScript knows this is User
}
"Potential Memory Leak/Race Condition"
Before flagging, you MUST:
- Verify cleanup function is actually missing (not just in a different location)
- Check if AbortController signal is checked after awaits
- Confirm the component can actually unmount during the async operation
Common false positives:
- Cleanup exists in useEffect return
- Signal is checked (code reviewer missed it)
- Operation completes before unmount is possible
"Performance Issue"
Before flagging, you MUST:
- Confirm the code runs frequently enough to matter (render vs click handler)
- Verify the optimization would have measurable impact
- Check if the framework already optimizes this (React compiler, memoization)
Do NOT flag:
- Functions created in click handlers (runs once per click)
- Array methods on small arrays (< 100 items)
- Object creation in event handlers
Severity Calibration
Critical (Block Merge)
ONLY use for:
- Security vulnerabilities (injection, auth bypass, data exposure)
- Data corruption bugs
- Crash-causing bugs in happy path
- Breaking changes to public APIs
Major (Should Fix)
Use for:
- Logic bugs that affect functionality
- Missing error handling that causes poor UX
- Performance issues with measurable impact
- Accessibility violations
Minor (Consider Fixing)
Use for:
- Code clarity improvements
- Documentation gaps
- Inconsistent style (within reason)
- Non-critical test coverage gaps
Informational (No Action Required)
Use for:
- Improvements that require adding new dependencies or modules
- Suggestions for net-new code that didn't exist in the codebase before (new modules, test suites, abstractions)
- Architectural ideas for future consideration
- Test infrastructure suggestions (new mock libraries, behaviour extraction)
- Optimizations without measurable impact in the current context
These are NOT review blockers. They should be noted for the author's awareness but must not appear in the actionable issue count. The Verdict should ignore informational items entirely.
Do NOT Flag At All
- Style preferences where both approaches are valid
- Optimizations with no measurable benefit
- Test code not meeting production standards (intentionally simpler)
- Library/framework internal code (shadcn components, generated code)
- Hypothetical issues that require unlikely conditions
Valid Patterns (Do NOT Flag)
TypeScript
| Pattern |
Why It's Valid |
map.get(key) || [] |
Map.get() returns T | undefined, fallback is correct |
| Class exports without separate type export |
Classes work as both value and type |
as const on literal arrays |
Creates readonly tuple types |
| Type annotation on variable declaration |
Not a type assertion |
satisfies instead of as |
Type checking without assertion |
React
| Pattern |
Why It's Valid |
| Array index as key (static list) |
Valid when: items don't reorder, list is static, no item identity needed |
| Inline arrow in onClick |
Valid for non-performance-critical handlers (runs once per click) |
| State that appears unused |
May be set via refs, external callbacks, or triggers re-renders |
| Empty dependency array with refs |
Refs are stable, don't need to be dependencies |
| Non-null assertion after check |
TypeScript narrowing may not track through all patterns |
Testing
| Pattern |
Why It's Valid |
toHaveTextContent without regex |
Handles nested text correctly |
| Mock at module level |
Defined once, not duplicated |
| Index-based test data |
Tests don't need stable identity |
| Simplified error messages |
Test clarity over production polish |
General
| Pattern |
Why It's Valid |
+? lazy quantifier in regex |
Prevents over-matching, correct for many patterns |
| Direct string concatenation |
Simpler than template literals for simple cases |
| Multiple returns in function |
Can improve readability |
| Comments explaining "why" |
Better than no comments |
Context-Sensitive Rules
React Keys
Flag array index as key ONLY IF ALL of these are true:
useEffect Dependencies
Flag missing dependency ONLY IF:
Error Handling
Flag missing try/catch ONLY IF:
Before Submitting Review
Final verification:
- Each finding passed Hard gates (sequence) (anchor, evidence with artifacts, severity, format).
- Re-read each finding and ask: "Did I verify this is actually an issue?"
- For each finding, can you point to the specific line that proves the issue exists?
- Would a domain expert agree this is a problem, or is it a style preference?
- Does fixing this provide real value, or is it busywork?
- Format every finding as:
[FILE:LINE] ISSUE_TITLE
- For each finding, ask: "Does this fix existing code, or does it request entirely new code that didn't exist before?" If the latter, downgrade to Informational.
- If this is a re-review: ONLY verify previous fixes. Do not introduce new findings.
If uncertain about any finding, either:
- Remove it from the review
- Mark it as a question rather than an issue
- Verify by reading more code context
1---2name: review-verification-protocol-33description: Mandatory verification steps for all code reviews to reduce false positives. Load this skill before reporting ANY code review findings.4---5
6# Review Verification Protocol
7
8This protocol MUST be followed before reporting any code review finding. Skipping these steps leads to false positives that waste developer time and erode trust in reviews.
9
10## Hard gates (sequence)
11
12Apply **once per finding** before it may appear in the review. If a gate fails, **omit** the finding, **downgrade** to Informational (per [Severity Calibration](#severity-calibration)), or **rephrase** as a question—do not ship soft accusations.
13
14| Step | What you do | Pass condition (objective) |
15|------|----------------|----------------------------|
16| **1. Anchor** | Read the full enclosing symbol or module, not only the diff hunk. | You can state **file path** and **line range** (or symbol name + file) you are judging. |
17| **2. Evidence** | For this finding’s type, run the checks in [Verification by Issue Type](#verification-by-issue-type). | Each required check has an **artifact**: pasted tool output, **file:line** citation, or explicit **"none"** / **"N matches"** after a repo search—not a claim you "looked." |
18| **3. Severity** | Assign severity using [Severity Calibration](#severity-calibration). | Label matches the table; requests for net-new code that did not exist in scope are **Informational** only. |
19| **4. Format** | Draft the finding for the report. | Matches `[FILE:LINE] ISSUE_TITLE`; Informational items do not add to the actionable count. |
20
21Style-only or preference items must fail gate 2 or map to **Do NOT Flag At All**—they do not get a severity.
22
23## Pre-Report Verification Checklist
24
25Before flagging ANY issue, verify (these items are **what gate 2 must produce evidence for**):
26
27- [ ] **I read the actual code** - Not just the diff context, but the full function/class
28- [ ] **I searched for usages** - Before claiming "unused", searched all references
29- [ ] **I checked surrounding code** - The issue may be handled elsewhere (guards, earlier checks)
30- [ ] **I verified syntax against current docs** - Framework syntax evolves (Tailwind v4, TS 5.x, React 19)
31- [ ] **I distinguished "wrong" from "different style"** - Both approaches may be valid
32- [ ] **I considered intentional design** - Checked comments, CLAUDE.md, architectural context
33
34## Verification by Issue Type
35
36### "Unused Variable/Function"
37
38**Before flagging**, you MUST:
391. Search for ALL references in the codebase (grep/find)
402. Check if it's exported and used by external consumers
413. Check if it's used via reflection, decorators, or dynamic dispatch
424. Verify it's not a callback passed to a framework
43
44**Common false positives:**
45- State setters in React (may trigger re-renders even if value appears unused)
46- Variables used in templates/JSX
47- Exports used by consuming packages
48
49### "Missing Validation/Error Handling"
50
51**Before flagging**, you MUST:
521. Check if validation exists at a higher level (caller, middleware, route handler)
532. Check if the framework provides validation (Pydantic, Zod, TypeScript)
543. Verify the "missing" check isn't present in a different form
55
56**Common false positives:**
57- Framework already validates (FastAPI + Pydantic, React Hook Form)
58- Parent component validates before passing props
59- Error boundary catches at higher level
60
61### "Type Assertion/Unsafe Cast"
62
63**Before flagging**, you MUST:
641. Confirm it's actually an assertion, not an annotation
652. Check if the type is narrowed by runtime checks before the point
663. Verify if framework guarantees the type (loader data, form data)
67
68**Valid patterns often flagged incorrectly:**
69```typescript
70// Type annotation, NOT assertion
71const data: UserData = await loader()
72
73// Type narrowing makes this safe
74if (isUser(data)) {
75 data.name // TypeScript knows this is User
76}
77```
78
79### "Potential Memory Leak/Race Condition"
80
81**Before flagging**, you MUST:
821. Verify cleanup function is actually missing (not just in a different location)
832. Check if AbortController signal is checked after awaits
843. Confirm the component can actually unmount during the async operation
85
86**Common false positives:**
87- Cleanup exists in useEffect return
88- Signal is checked (code reviewer missed it)
89- Operation completes before unmount is possible
90
91### "Performance Issue"
92
93**Before flagging**, you MUST:
941. Confirm the code runs frequently enough to matter (render vs click handler)
952. Verify the optimization would have measurable impact
963. Check if the framework already optimizes this (React compiler, memoization)
97
98**Do NOT flag:**
99- Functions created in click handlers (runs once per click)
100- Array methods on small arrays (< 100 items)
101- Object creation in event handlers
102
103## Severity Calibration
104
105### Critical (Block Merge)
106
107**ONLY use for:**
108- Security vulnerabilities (injection, auth bypass, data exposure)
109- Data corruption bugs
110- Crash-causing bugs in happy path
111- Breaking changes to public APIs
112
113### Major (Should Fix)
114
115**Use for:**
116- Logic bugs that affect functionality
117- Missing error handling that causes poor UX
118- Performance issues with measurable impact
119- Accessibility violations
120
121### Minor (Consider Fixing)
122
123**Use for:**
124- Code clarity improvements
125- Documentation gaps
126- Inconsistent style (within reason)
127- Non-critical test coverage gaps
128
129### Informational (No Action Required)
130
131**Use for:**
132- Improvements that require adding new dependencies or modules
133- Suggestions for net-new code that didn't exist in the codebase before (new modules, test suites, abstractions)
134- Architectural ideas for future consideration
135- Test infrastructure suggestions (new mock libraries, behaviour extraction)
136- Optimizations without measurable impact in the current context
137
138**These are NOT review blockers.** They should be noted for the author's awareness but must not appear in the actionable issue count. The Verdict should ignore informational items entirely.
139
140### Do NOT Flag At All
141
142- Style preferences where both approaches are valid
143- Optimizations with no measurable benefit
144- Test code not meeting production standards (intentionally simpler)
145- Library/framework internal code (shadcn components, generated code)
146- Hypothetical issues that require unlikely conditions
147
148## Valid Patterns (Do NOT Flag)
149
150### TypeScript
151
152| Pattern | Why It's Valid |
153|---------|----------------|
154| `map.get(key) \|\| []` | `Map.get()` returns `T \| undefined`, fallback is correct |
155| Class exports without separate type export | Classes work as both value and type |
156| `as const` on literal arrays | Creates readonly tuple types |
157| Type annotation on variable declaration | Not a type assertion |
158| `satisfies` instead of `as` | Type checking without assertion |
159
160### React
161
162| Pattern | Why It's Valid |
163|---------|----------------|
164| Array index as key (static list) | Valid when: items don't reorder, list is static, no item identity needed |
165| Inline arrow in onClick | Valid for non-performance-critical handlers (runs once per click) |
166| State that appears unused | May be set via refs, external callbacks, or triggers re-renders |
167| Empty dependency array with refs | Refs are stable, don't need to be dependencies |
168| Non-null assertion after check | TypeScript narrowing may not track through all patterns |
169
170### Testing
171
172| Pattern | Why It's Valid |
173|---------|----------------|
174| `toHaveTextContent` without regex | Handles nested text correctly |
175| Mock at module level | Defined once, not duplicated |
176| Index-based test data | Tests don't need stable identity |
177| Simplified error messages | Test clarity over production polish |
178
179### General
180
181| Pattern | Why It's Valid |
182|---------|----------------|
183| `+?` lazy quantifier in regex | Prevents over-matching, correct for many patterns |
184| Direct string concatenation | Simpler than template literals for simple cases |
185| Multiple returns in function | Can improve readability |
186| Comments explaining "why" | Better than no comments |
187
188## Context-Sensitive Rules
189
190### React Keys
191
192Flag array index as key **ONLY IF ALL** of these are true:
193- [ ] Items CAN be reordered (sortable list, drag-drop)
194- [ ] Items CAN be inserted/removed from middle
195- [ ] Items HAVE stable identifiers available (id, uuid)
196- [ ] The list is NOT completely replaced atomically
197
198### useEffect Dependencies
199
200Flag missing dependency **ONLY IF**:
201- [ ] The value actually changes during component lifetime
202- [ ] Stale closure would cause incorrect behavior
203- [ ] The value is NOT a ref (refs are stable)
204- [ ] The value is NOT a stable callback (useCallback with empty deps)
205
206### Error Handling
207
208Flag missing try/catch **ONLY IF**:
209- [ ] No error boundary catches this at a higher level
210- [ ] The framework doesn't handle errors (loader errorElement)
211- [ ] The error would cause a crash, not just a failed operation
212- [ ] User needs specific feedback for this error type
213
214## Before Submitting Review
215
216Final verification:
2171. Each finding passed [Hard gates (sequence)](#hard-gates-sequence) (anchor, evidence with artifacts, severity, format).
2182. Re-read each finding and ask: "Did I verify this is actually an issue?"
2193. For each finding, can you point to the specific line that proves the issue exists?
2204. Would a domain expert agree this is a problem, or is it a style preference?
2215. Does fixing this provide real value, or is it busywork?
2226. Format every finding as: `[FILE:LINE] ISSUE_TITLE`
2237. For each finding, ask: "Does this fix existing code, or does it request entirely new code that didn't exist before?" If the latter, downgrade to Informational.
2248. If this is a re-review: ONLY verify previous fixes. Do not introduce new findings.
225
226If uncertain about any finding, either:
227- Remove it from the review
228- Mark it as a question rather than an issue
229- Verify by reading more code context
majiayu000