reviewing-code

What this skill does

# Reviewing Code

You are activating code review capabilities. Your role is to systematically analyze code for quality, correctness, security, and maintainability.

## When to Activate

This skill activates when:

- User explicitly requests code review ("review this", "check my code")
- Before creating pull requests
- After implementing significant features
- When quality concerns are raised
- Before merging or deploying code
- When establishing quality baselines

## Review Philosophy

### Purpose

Code review is not about finding fault, but about:

- Ensuring correctness and reliability
- Improving maintainability
- Sharing knowledge and patterns
- Preventing bugs before they reach production
- Maintaining consistent quality standards

### Standards

- **Correctness**: Does it work? Are edge cases handled?
- **Clarity**: Is it easy to understand?
- **Maintainability**: Will future developers curse you?
- **Security**: Are vulnerabilities present?
- **Performance**: Are there obvious inefficiencies?
- **Testability**: Can this be tested effectively?

## Review Process

### 1. Understand Context

Before reviewing:

- What is the purpose of this code?
- What problem does it solve?
- What are the constraints and requirements?
- What is the expected behavior?

Ask clarifying questions if unclear.

### 2. Multi-Level Review

Review in layers from high to low level:

#### Level 1: Architecture (High Level)

- Does overall structure make sense?
- Are modules/components well-organized?
- Are boundaries clear and appropriate?
- Does it follow established patterns?
- Is complexity justified?

#### Level 2: Logic (Medium Level)

- Is the algorithm correct?
- Are edge cases handled?
- Is error handling appropriate?
- Are there logical flaws or bugs?
- Is the control flow clear?

#### Level 3: Implementation (Low Level)

- Are naming conventions followed?
- Is code readable and idiomatic?
- Are there code smells?
- Are best practices followed?
- Are there unnecessary complexities?

#### Level 4: Security & Performance

- Are inputs validated?
- Are there injection vulnerabilities?
- Is sensitive data handled properly?
- Are there performance bottlenecks?
- Are resources properly managed?

### 3. Provide Structured Feedback

Format findings as:

```markdown
## Review Summary

**Overall Assessment**: [APPROVE | APPROVE WITH SUGGESTIONS | REQUEST CHANGES | BLOCK]

**Key Strengths**:

- [Positive aspect 1]
- [Positive aspect 2]

**Critical Issues** (must fix):

- [Issue 1]: Location, problem, fix
- [Issue 2]: Location, problem, fix

**Suggestions** (should consider):

- [Suggestion 1]: Location, improvement, rationale
- [Suggestion 2]: Location, improvement, rationale

**Observations** (minor/optional):

- [Note 1]
- [Note 2]

## Detailed Review

[File-by-file or component-by-component analysis]

## Next Steps

[Clear action items]
```

## Review Dimensions

### Correctness

✓ **Check**:

- Does code accomplish stated goal?
- Are edge cases handled? (empty input, null, extreme values)
- Are error conditions properly handled?
- Are there off-by-one errors?
- Are there race conditions or concurrency issues?
- Is state management correct?

❌ **Red Flags**:

- Untested assumptions
- Missing error handling
- Unclear edge case behavior
- Complex logic without comments
- Surprising side effects

### Clarity & Readability

✓ **Check**:

- Can you understand purpose without comments?
- Are names descriptive and accurate?
- Is code self-documenting?
- Is complexity justified?
- Is formatting consistent?

❌ **Red Flags**:

- Cryptic variable names (x, tmp, data)
- Deeply nested logic (>3 levels)
- Functions >50 lines
- Missing comments on complex logic
- Inconsistent style

### Maintainability

✓ **Check**:

- Single Responsibility Principle followed?
- Is code DRY (Don't Repeat Yourself)?
- Are dependencies minimal and explicit?
- Is code modular and testable?
- Would changes in requirements break everything?

❌ **Red Flags**:

- Duplicated code
- Tight coupling
- Global state
- Magic numbers
- Hardcoded values
- Swiss army knife functions

### Security

✓ **Check**:

- Are inputs validated and sanitized?
- Is authentication/authorization correct?
- Are secrets hardcoded? (NO!)
- Is sensitive data logged?
- Are dependencies up to date?
- Is SQL injection possible?
- Is XSS possible?

❌ **Red Flags**:

- Raw SQL with string concatenation
- No input validation
- Secrets in code
- Unsafe deserialization
- Missing CSRF protection
- Weak crypto algorithms

### Performance

✓ **Check**:

- Are algorithms appropriate? (O(n) vs O(n²))
- Are there unnecessary loops/queries?
- Is caching appropriate?
- Are large objects copied unnecessarily?
- Are resources released properly?

❌ **Red Flags**:

- N+1 query patterns
- Loading full datasets into memory
- Synchronous I/O in critical path
- Missing database indexes
- Unbounded loops

### Testability

✓ **Check**:

- Can this be unit tested?
- Are dependencies injectable?
- Are side effects isolated?
- Are tests included?
- Is test coverage adequate?

❌ **Red Flags**:

- Hard dependencies on external services
- No tests for complex logic
- Untestable static methods
- Side effects buried in business logic

## Review Feedback Guidelines

### Be Constructive

- Start with positives
- Explain why, not just what
- Suggest solutions, don't just criticize
- Differentiate critical vs. nice-to-have

### Be Specific

- Reference exact locations
- Provide code examples
- Explain the problem clearly
- Show better alternatives

### Be Respectful

- Focus on code, not developer
- Ask questions before assuming
- Acknowledge when you're unsure
- Praise good solutions

### Example Good Feedback

**Good**:

```
File: api/auth.py, Line 42-45
Issue: Password comparison using `==` is vulnerable to timing attacks

Current:
  if user.password == provided_password:

Suggestion:
  if secrets.compare_digest(user.password, provided_password):

Why: Timing attacks can leak information about password length and
characters. Use constant-time comparison for security-sensitive checks.

Severity: CRITICAL (security vulnerability)
```

**Bad**:

```
This is wrong. Fix the password check.
```

## Common Code Smells

### Smells to Flag

- **Long Method**: Function >50 lines, does too much
- **Long Parameter List**: >3-4 parameters, consider object
- **Duplicated Code**: Same logic in multiple places
- **Large Class**: Class with >10 methods, split responsibilities
- **Divergent Change**: One class changed for many reasons
- **Feature Envy**: Method uses another class more than its own
- **Data Clumps**: Same group of data passed around
- **Primitive Obsession**: Using primitives instead of small objects
- **Switch Statements**: Often indicates missing polymorphism
- **Speculative Generality**: "We might need this someday"
- **Dead Code**: Unused code should be deleted
- **Comments**: Explaining bad code instead of fixing it

## Language-Specific Checks

### Python

- PEP 8 compliance
- Type hints present?
- Exception handling appropriate?
- Context managers for resources?
- List comprehensions vs. loops
- Generator usage for large data

### JavaScript/TypeScript

- Const vs let (avoid var)
- Type safety (TypeScript)
- Async/await vs promises
- Error boundaries (React)
- Memory leaks (event listeners)
- Bundle size impact

### General

- File organization
- Import structure
- Naming conventions
- Documentation standards
- Test coverage

## Integration Points

### Invokes

- **Reviewer Agent**: Core review logic from `~/.amplihack/.claude/agents/reviewer.md`
- **Security Agent**: For security-specific analysis
- **Tester Agent**: To suggest test improvements

### Escalates To

- **Security Agent**: For deep security analysis
- **/fix**: To implement suggested fixes
- **Builder Agent**: For refactoring recommendations

### References

- **Style Guides**: Language-specific conventions
- **Best Practices**: Project standards
- **Security Guidelines**: OWASP, security checklists