Structured Review

---
name: structured-code-review
description: Performs a structured five-stage code review covering requirements compliance, correctness, code quality, testing, and security/performance. Each stage uses targeted checklists and categorized feedback (Blocker/Major/Minor/Nit) with actionable suggestions and rationale. Use when the user asks for code review, PR feedback, pull request review, or wants their code checked for bugs, style issues, or vulnerabilities — triggered by phrases like "review my code", "check this PR", "review my changes", "pull request review", or "code feedback".
version: 1.0.0
triggers:
  - code review
  - review this
  - feedback on code
  - PR review
  - review my changes
tags:
  - collaboration
  - code-review
  - quality
  - feedback
difficulty: intermediate
estimatedTime: 15
relatedSkills:
  - collaboration/parallel-investigation
  - planning/verification-gates
---

# Structured Code Review

You are performing a structured, multi-stage code review. This methodology ensures thorough review while providing actionable, constructive feedback.

## Core Principle

**Review in stages. Each stage has a specific focus. Don't mix concerns.**

A structured review catches more issues and provides better feedback than an unstructured scan.

## Review Stages

### Stage 1: Requirements Compliance

First, verify the code meets its requirements.

**Checklist:**
- [ ] Implements stated requirements
- [ ] Handles specified edge cases
- [ ] No scope creep (unexpected additions)
- [ ] No missing functionality

**Feedback at this stage:**
- "This doesn't appear to handle the case when X is empty"
- "The requirement specified Y, but this implements Z"
- "This adds feature F which wasn't requested - is that intentional?"

### Stage 2: Correctness

Next, verify the code works correctly.

**Checklist:**
- [ ] Logic is sound
- [ ] No obvious bugs
- [ ] Error paths are handled
- [ ] No unfinished code (TODOs without tickets)

**Feedback at this stage:**
- "This will throw if `user` is null"
- "The loop exits early before processing all items"
- "What happens when the API call fails?"

### Stage 3: Code Quality

Then, evaluate code quality and maintainability.

**Checklist:**
- [ ] Clear naming
- [ ] Reasonable function/method length
- [ ] No unnecessary complexity
- [ ] Follows project conventions
- [ ] Appropriate abstractions

**Feedback at this stage:**
- "Could you rename `data` to `userProfile` for clarity?"
- "This function is doing three things - consider splitting"
- "We use camelCase for variables in this project"

### Stage 4: Testing

Evaluate test coverage and quality.

**Checklist:**
- [ ] New code has tests
- [ ] Tests cover main paths and edge cases
- [ ] Tests are readable and maintainable
- [ ] Tests don't test implementation details

**Feedback at this stage:**
- "Please add a test for the error case"
- "This test will break if we change the implementation"
- "Consider using a parameterized test for these cases"

### Stage 5: Security & Performance

Finally, check for security and performance concerns.

**Checklist:**
- [ ] No SQL injection, XSS, etc.
- [ ] Secrets not exposed
- [ ] No obvious N+1 queries
- [ ] No unnecessary computation
- [ ] Sensitive data handled correctly

**Feedback at this stage:**
- "This input should be sanitized before use"
- "Consider adding an index for this query"
- "This API key should come from environment variables"

## Writing Good Feedback

### Feedback Levels

| Level | When to Use | Example |
|-------|-------------|---------|
| **Blocker** | Must fix before merge | "Security: This allows SQL injection" |
| **Major** | Should fix, but not critical | "This will fail for empty arrays" |
| **Minor** | Suggestion, nice to have | "Consider renaming for clarity" |
| **Nit** | Trivial, stylistic | "Extra blank line here" |

### Constructive Feedback Template

```
[Level] [Category]: [Issue]

**What:** [Describe the specific issue]
**Why:** [Explain why it matters]
**Suggestion:** [Offer a specific improvement]
```

Example:
```
[Major] Correctness: Null reference possible

**What:** `user.email` is accessed without checking if user exists
**Why:** This will throw TypeError when user is not found
**Suggestion:** Add `if (!user) return null;` before accessing properties
```

## Review Checklist Summary

```markdown
## Review: [PR Title]

### Stage 1: Requirements
- [ ] Implements requirements
- [ ] Handles edge cases
- [ ] Appropriate scope

### Stage 2: Correctness
- [ ] Logic is sound
- [ ] No bugs
- [ ] Errors handled

### Stage 3: Quality
- [ ] Readable
- [ ] Follows conventions
- [ ] Maintainable

### Stage 4: Testing
- [ ] Has tests
- [ ] Tests are good

### Stage 5: Security/Performance
- [ ] No vulnerabilities
- [ ] No performance issues

### Verdict: [ ] Approve [ ] Request Changes [ ] Comment
```

## Integration with Other Skills

- **planning/verification-gates**: Review is a key gate
- **testing/test-patterns**: Evaluate test quality
- **testing/anti-patterns**: Spot testing issues