Use after implementing features - 7-criteria code review with MANDATORY artifact posting to GitHub issue; blocks PR creation until complete
allowed-tools
["Read","Grep","Glob","mcp__github__*"]
model
opus
Comprehensive Review
Overview
Review code against 7 criteria before considering it complete.
Core principle: Self-review catches issues before they reach others.
HARD REQUIREMENT: Review artifact MUST be posted to the GitHub issue. This is enforced by hooks.
Announce at start: "I'm performing a comprehensive code review."
Review Artifact Requirement
This is not optional. Before a PR can be created:
Complete review against all 7 criteria
Document all findings
Post artifact to issue comment using EXACT format below
Address all findings (fix or defer with tracking issues)
Update artifact to show "Unaddressed: 0"
The review-gate skill and PreToolUse hook will BLOCK PR creation without this artifact.
The 7 Criteria
1. Blindspots
Question: What am I missing?
Check
Ask Yourself
Edge cases
What happens at boundaries? Empty input? Max values?
Error paths
What if external services fail? Network issues?
Concurrency
Multiple users/threads? Race conditions?
State
What if called in wrong order? Invalid state?
Dependencies
What if dependency behavior changes?
// Blindspot example: What if items is empty?functioncalculateAverage(items: number[]): number {
return items.reduce((a, b) => a + b, 0) / items.length;
// Blindspot: Division by zero when items is empty!
}
// FixedfunctioncalculateAverage(items: number[]): number {
if (items.length === 0) {
thrownewError('Cannot calculate average of empty array');
}
return items.reduce((a, b) => a + b, 0) / items.length;
}
2. Clarity/Consistency
Question: Will someone else understand this?
Check
Ask Yourself
Names
Do names describe what things do/are?
Structure
Is code organized logically?
Complexity
Can this be simplified?
Patterns
Does this match existing patterns?
Surprises
Would anything surprise a reader?
3. Maintainability
Question: Can this be changed safely?
Check
Ask Yourself
Coupling
Is this tightly bound to other code?
Cohesion
Does this do one thing well?
Duplication
Is logic repeated anywhere?
Tests
Do tests cover this adequately?
Extensibility
Can new features be added easily?
4. Security Risks
Question: Can this be exploited?
Check
Ask Yourself
Input validation
Is all input validated and sanitized?
Authentication
Is access properly controlled?
Authorization
Are permissions checked?
Data exposure
Is sensitive data protected?
Injection
SQL, XSS, command injection possible?
Dependencies
Are dependencies secure and updated?
NOTE: If security-sensitive files are changed (auth, api, middleware, etc.), invoke security-review skill for deeper analysis.
5. Performance Implications
Question: Will this scale?
Check
Ask Yourself
Algorithms
Is complexity appropriate? O(n²) when O(n) possible?
Database
N+1 queries? Missing indexes? Full table scans?
Memory
Large objects in memory? Memory leaks?
Network
Unnecessary requests? Large payloads?
Caching
Should results be cached?
6. Documentation
Question: Is this documented adequately?
Check
Ask Yourself
Public APIs
Are all public functions documented?
Parameters
Are parameter types and purposes clear?
Returns
Is return value documented?
Errors
Are thrown errors documented?
Examples
Are complex usages demonstrated?
Why
Are non-obvious decisions explained?
See inline-documentation skill for documentation standards.
7. Standards and Style
Question: Does this follow project conventions?
Check
Ask Yourself
Naming
Follows project naming conventions?
Formatting
Matches project formatting?
Patterns
Uses established patterns?
Types
Fully typed (no any)?
Language
Uses inclusive language?
IPv6-first
Network code uses IPv6 by default? IPv4 only for documented legacy?
Linting
Passes all linters?
See style-guide-adherence, strict-typing, inclusive-language, ipv6-first skills.
Review Process
Step 1: Prepare
# Get list of changed files
git diff --name-only HEAD~1
# Get full diff
git diff HEAD~1
# Check for security-sensitive files
git diff --name-only HEAD~1 | grep -E '(auth|security|middleware|api|password|token|secret)'# If matches found, security-review skill is MANDATORY
Step 2: Review Each Criterion
For each of the 7 criteria:
Review all changed code
Note any issues found
Determine severity (Critical/Major/Minor)
Step 3: Check Security-Sensitive
If ANY security-sensitive files were changed:
Invoke security-review skill OR security-reviewer subagent
Include security review results in artifact
Mark "Security-Sensitive: YES" in artifact
Step 4: Document Findings
## Code Review Findings### 1. Blindspots- [ ] **Critical**: No handling for empty array in `calculateAverage()`- [ ] **Minor**: Missing null check in `formatUser()`### 2. Clarity/Consistency- [ ] **Major**: Variable `x` should have descriptive name
### 3. Maintainability- [x] No issues found
### 4. Security Risks- [ ] **Critical**: SQL injection possible in `findUser()`### 5. Performance Implications- [ ] **Major**: N+1 query in `getOrdersWithUsers()`### 6. Documentation- [ ] **Minor**: Missing JSDoc on `processOrder()`### 7. Standards and Style- [x] Passes all checks
Step 5: Address All Findings
Use apply-all-findings skill to address every issue.
For findings that cannot be fixed:
Use deferred-finding skill to create tracking issue
Link tracking issue in artifact
"Deferred without tracking issue" is NOT PERMITTED
Step 6: Post Artifact to Issue (MANDATORY)
Post review artifact as comment on the GitHub issue:
