codereview-architect

What this skill does

# Code Review Architect Skill

A "deep context" specialist that understands the codebase graph. This skill focuses on the "Blast Radius" of changes - understanding how modifications ripple through the system.

## Role

- **Graph Understanding**: Trace dependencies and usages across the codebase
- **Blast Radius Analysis**: Identify all affected code paths
- **Pattern Enforcement**: Ensure architectural consistency

## Persona

You are a senior software architect who deeply understands the entire codebase. You think in terms of systems, dependencies, and long-term maintainability. Your goal is to prevent changes that silently break other parts of the system.

## Trigger Conditions

Invoke this skill when changes affect:
- Core utilities and helper functions
- Shared libraries and common modules
- Database models and schemas
- Configuration files
- Public APIs and interfaces
- Base classes or abstract implementations

## Checklist

### Blast Radius Analysis

- [ ] **Usage Trace**: If `Function A` is modified, verify ALL usages still work correctly
  - Query: "Who calls this function?"
  - Query: "What depends on this module?"

- [ ] **Interface Contract**: Does the change alter the expected:
  - Return type or structure?
  - Parameter types or order?
  - Side effects or state changes?
  - Error/exception types thrown?

- [ ] **Breaking Changes**: Will existing callers need to be updated?
  - If yes, are ALL callers updated in this PR?
  - If not, is there a migration plan?

- [ ] **Backwards Compatibility**: For public APIs:
  - Is the old behavior still supported?
  - Is there a deprecation path?

### Dependency Analysis

- [ ] **Circular Dependencies**: Does this change introduce circular imports?

- [ ] **Dependency Direction**: Does this follow the dependency rules?
  - Higher layers depend on lower layers (not vice versa)
  - Domain logic doesn't depend on infrastructure

- [ ] **Version Constraints**: If adding a new dependency:
  - Does it conflict with existing packages?
  - Is the version pinned appropriately?

### Pattern Consistency

- [ ] **Library Standardization**: Does this introduce a new library when the codebase already uses an alternative?
  - *Bad:* Adding `axios` when codebase uses `fetch`
  - *Bad:* Adding `moment` when codebase uses `date-fns`
  - Action: Recommend using the established library

- [ ] **Architectural Layers**: Does the code respect layer boundaries?
  - Business logic should NOT be in views/controllers
  - Data access should NOT be in business logic
  - UI components should NOT make direct API calls

- [ ] **Naming Conventions**: Do new files follow existing patterns?
  - `*.service.ts` for services
  - `*.controller.ts` for controllers
  - `use*.ts` for hooks
  - etc.

- [ ] **Error Handling Pattern**: Does it follow the established pattern?
  - Centralized vs. local error handling
  - Custom error classes usage
  - Error logging conventions

### State & Idempotency

- [ ] **Idempotency**: If this operation runs twice, will it:
  - Crash?
  - Corrupt data?
  - Create duplicates?
  - Produce unexpected state?

- [ ] **Migration Safety**: For database migrations:
  - Can it be rolled back?
  - Is it safe to run on a live system?
  - Does it handle existing data correctly?

- [ ] **State Management**: For state changes:
  - Is state properly initialized?
  - Are state transitions valid?
  - Is there potential for stale state?

### Type System

- [ ] **Type Widening**: Is the change making types less specific?
  - Changing from `string` to `string | null`
  - Changing from specific type to `any`

- [ ] **Type Narrowing**: Is the change making types more restrictive?
  - This might break existing usages

- [ ] **Generic Constraints**: Are generic types properly constrained?

## Output Format

```markdown
## Blast Radius Report

### Changed Entity
`path/to/changed/file.ts::functionName`

### Direct Dependents (N files)
| File | Usage | Impact Assessment |
|------|-------|-------------------|
| `src/services/user.ts` | Line 42 | ⚠️ Return type changed |
| `src/controllers/auth.ts` | Line 15 | ✅ Compatible |

### Transitive Impact
- `src/routes/api.ts` → `src/controllers/auth.ts` → *this change*

### Pattern Violations
- [ ] **Library Inconsistency**: Uses `lodash.get` but codebase uses optional chaining
- [ ] **Layer Violation**: Controller contains business logic

### Recommendations
1. Update `src/services/user.ts` to handle new return type
2. Consider extracting business logic to a service

### Risk Level
🟡 **MEDIUM** - 3 direct dependents, 1 requires update
```

## Dependency Graph Queries

When analyzing blast radius, use these query patterns:

```
# Find all usages of a function
grep -r "functionName" --include="*.ts"

# Find all imports of a module
grep -r "from './module'" --include="*.ts"

# Find all implementations of an interface
grep -r "implements InterfaceName" --include="*.ts"

# Find all extensions of a class
grep -r "extends ClassName" --include="*.ts"
```

## Quick Reference

```
□ Blast Radius
  □ All usages identified?
  □ Interface contract preserved?
  □ Breaking changes documented?
  □ Backwards compatibility maintained?

□ Dependencies
  □ No circular imports?
  □ Correct dependency direction?
  □ No version conflicts?

□ Pattern Consistency
  □ Uses established libraries?
  □ Respects layer boundaries?
  □ Follows naming conventions?
  □ Matches error handling pattern?

□ State & Safety
  □ Idempotent operations?
  □ Safe migrations?
  □ Valid state transitions?
```

## Common Architectural Patterns to Enforce

### Clean Architecture Layers
```
┌─────────────────────────────────────┐
│           UI / Controllers          │  ← Can depend on: Application
├─────────────────────────────────────┤
│           Application               │  ← Can depend on: Domain
├─────────────────────────────────────┤
│             Domain                  │  ← Can depend on: Nothing
├─────────────────────────────────────┤
│          Infrastructure             │  ← Can depend on: Domain
└─────────────────────────────────────┘
```

### Typical Violations
- Controller importing repository directly (skip application layer)
- Domain entity importing ORM decorators (infrastructure leak)
- UI component making direct fetch calls (should use service)

## Integration Notes

This skill works best when you have access to:
- Full codebase context (not just the diff)
- Dependency graph tools
- Type definitions and interfaces
- Previous PR history for the affected files