troykelly/comprehensive-review

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."

Subagent option: For a Claude-like subagent flow, run:

codex-subagent code-reviewer <<'EOF'
[Describe the review scope, issue/PR numbers, and any focus areas.]
EOF

If you run the subagent, use its output and artifact. Otherwise, follow the steps below.

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?

| 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?
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?

| 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:

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 run codex-subagent security-reviewer
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

| Severity | Description | Action | |----------|-------------|--------| | Critical | Security issue, data loss, crash | Must fix before merge | | Major | Significant bug, performance issue | Must fix before merge | | Minor | Style, clarity, small improvement | Should fix before merge |

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)