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:

1. Complete review against all 7 criteria
2. Document all findings
3. Post artifact to issue comment using EXACT format below
4. Address all findings (fix or defer with tracking issues)
5. 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?

// Blindspot example: What if items is empty?
function calculateAverage(items: number[]): number {
  return items.reduce((a, b) => a + b, 0) / items.length;
  // Blindspot: Division by zero when items is empty!
}

// Fixed
function calculateAverage(items: number[]): number {
  if (items.length === 0) {
    throw new Error('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?

3. Maintainability

Question: Can this be changed safely?

4. Security Risks

Question: Can this be exploited?

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?

6. Documentation

Question: Is this documented adequately?

See inline-documentation skill for documentation standards.

7. Standards and Style

Question: Does this follow project conventions?

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:

1. Review all changed code
2. Note any issues found
3. Determine severity (Critical/Major/Minor)

Step 3: Check Security-Sensitive

If ANY security-sensitive files were changed:

1. Invoke security-review skill OR security-reviewer subagent
2. Include security review results in artifact
3. 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:

1. Use deferred-finding skill to create tracking issue
2. Link tracking issue in artifact
3. "Deferred without tracking issue" is NOT PERMITTED

Step 6: Post Artifact to Issue (MANDATORY)

Post review artifact as comment on the GitHub issue:

ISSUE_NUMBER=123
gh issue comment $ISSUE_NUMBER --body "$(cat <<'EOF'
<!-- REVIEW:START -->
## Code Review Complete

| Property | Value |
|----------|-------|
| Worker | `[WORKER_ID]` |
| Issue | #123 |
| Scope | [MINOR|MAJOR] |
| Security-Sensitive | [YES|NO] |
| Reviewed | [ISO_TIMESTAMP] |

### Criteria Results

| # | Criterion | Status | Findings |
|---|-----------|--------|----------|
| 1 | Blindspots | [✅ PASS|✅ FIXED|⚠️ DEFERRED] | [N] |
| 2 | Clarity | [✅ PASS|✅ FIXED|⚠️ DEFERRED] | [N] |
| 3 | Maintainability | [✅ PASS|✅ FIXED|⚠️ DEFERRED] | [N] |
| 4 | Security | [✅ PASS|✅ FIXED|⚠️ DEFERRED|N/A] | [N] |
| 5 | Performance | [✅ PASS|✅ FIXED|⚠️ DEFERRED] | [N] |
| 6 | Documentation | [✅ PASS|✅ FIXED|⚠️ DEFERRED] | [N] |
| 7 | Style | [✅ PASS|✅ FIXED|⚠️ DEFERRED] | [N] |

### Findings Fixed in This PR

| # | Severity | Finding | Resolution |
|---|----------|---------|------------|
| 1 | [SEVERITY] | [DESCRIPTION] | [HOW_FIXED] |

### Findings Deferred (With Tracking Issues)

| # | Severity | Finding | Tracking Issue | Justification |
|---|----------|---------|----------------|---------------|
| 1 | [SEVERITY] | [DESCRIPTION] | #[ISSUE] | [WHY] |

### Summary

| Category | Count |
|----------|-------|
| Fixed in PR | [N] |
| Deferred (with tracking) | [N] |
| Unaddressed | 0 |

**Review Status:** ✅ COMPLETE
<!-- REVIEW:END -->
EOF
)"

CRITICAL: "Unaddressed" MUST be 0. "Review Status" MUST be "COMPLETE".

Severity Levels

Checklist

Complete for every code review:

• Blindspots: Edge cases, errors, concurrency checked
• Clarity: Names, structure, complexity reviewed
• Maintainability: Coupling, cohesion, tests evaluated
• Security: Input, auth, injection, exposure checked (MANDATORY for sensitive files)
• Performance: Algorithms, queries, memory reviewed
• Documentation: Public APIs documented
• Style: Conventions followed
• All findings documented
• All findings addressed OR deferred with tracking issues
• Review artifact posted to issue (exact format)
• "Unaddressed: 0" in artifact
• "Review Status: COMPLETE" in artifact

Integration

This skill is called by:

• issue-driven-development - Step 9

This skill uses:

• review-scope - Determine review breadth
• apply-all-findings - Address issues
• security-review - For security-sensitive changes
• deferred-finding - For creating tracking issues

This skill is enforced by:

• review-gate - Verifies artifact before PR
• PreToolUse hook - Blocks PR without artifact

This skill references:

• inline-documentation - Documentation standards
• strict-typing - Type requirements
• style-guide-adherence - Style requirements
• inclusive-language - Language requirements
• ipv6-first - Network code requirements (IPv6 primary, IPv4 legacy)