| name | design-review |
| description | This skill should be used when reviewing implementation plans for anti-patterns, checking design quality before implementation, or performing plan-level quality assessment. Provides 6 anti-pattern detection rules for the designer agent and inline planning. |
| user-invocable | false |
| allowed-tools | Read, Grep, Glob |
Design Review
Anti-pattern detection for implementation plans. Catches structural problems before they become code.
Iron Law
CATCH IT IN THE PLAN, NOT IN THE PR
A plan that implies N+1 queries will produce N+1 queries. A plan that omits error
handling will produce code with no error handling. Review the plan as rigorously as
you review the code — the plan is the blueprint that every implementation decision
follows.
Anti-Pattern Detection Rules
1. N+1 Queries
What to look for in a plan:
- Steps that fetch a list, then loop to fetch related data per item
- "For each X, get Y" language without mentioning batch operations
- JOIN or batch-fetch never mentioned for related data
- "Display X with Y" without specifying how Y is fetched
How to flag:
[N+1 RISK] Step {N}: "{quoted plan text}" — fetches {entity} per iteration.
Resolution: Replace loop with batch query: {proposed batch operation}.
What to suggest instead:
- Identify all IDs in the list first, then batch-fetch related records in one query
- Use ORM eager loading (
.include, .with, JOIN) at the list query level
- Cache frequently accessed related data at the service layer
2. God Functions
What to look for in a plan:
- Single step handling more than 3 distinct responsibilities
- "Validate AND process AND notify AND log AND update" in one step
- Steps described as "the main handler" that touch multiple subsystems
- No clear single-sentence description of what the function does
How to flag:
[GOD FUNCTION RISK] Step {N}: "{quoted plan text}" — handles {n} responsibilities.
Resolution: Split into: {responsibility 1} → {responsibility 2} → {responsibility 3}.
What to suggest instead:
- Each function should have a single, clear sentence description of its purpose
- Extract notification, logging, and side effects to separate functions
- Orchestrate composition at a higher level (service layer calls multiple focused functions)
3. Missing Parallelism
What to look for in a plan:
- Sequential steps with no data dependency between them
- "First do A, then do B, then do C" where B doesn't need A's output
- Multiple external API calls or DB queries listed sequentially
- Fan-out operations (notify all users, update all records) without parallel mention
How to flag:
[SEQUENTIAL BOTTLENECK] Steps {N}-{M}: "{quoted steps}" — no dependency between them.
Resolution: Execute {step A} and {step B} in parallel: {proposed parallel pattern}.
What to suggest instead:
- Use
Promise.all / goroutines / async.gather for independent operations
- Identify the critical path and move independent work off it
- Explicitly note which steps have ordering requirements vs. which are independent
4. Error Handling Gaps
What to look for in a plan:
- Happy path fully described, failure paths absent or vague
- "If error, return error" without specifying what the caller does with it
- Partial operations (multi-step) without rollback plan
- External calls (API, DB, queue) without failure mode specification
- Retry logic absent for transient failures (network timeouts, rate limits)
How to flag:
[ERROR HANDLING GAP] Step {N}: "{quoted plan text}" — failure path unspecified.
Resolution: Add: {on failure scenario} → {specific recovery action}.
What to suggest instead:
- For each external call, specify: what to do on timeout, on 4xx, on 5xx
- For multi-step operations, specify compensation (rollback/undo) for each step
- Use Result types or explicit error returns — no silent failures
5. Missing Caching
What to look for in a plan:
- Expensive operations called on every request without cache mention
- "Fetch configuration on each request", "call third-party API per user action"
- Reference data (categories, settings, permissions) loaded repeatedly
- Computed aggregates recalculated on every read
How to flag:
[CACHING GAP] Step {N}: "{quoted plan text}" — {expensive operation} on every request.
Resolution: Cache {data} for {TTL}: {proposed cache strategy and invalidation}.
What to suggest instead:
- Application-level cache (Redis/Memcached) for cross-request data
- In-memory cache for process-local reference data with short TTL
- Specify cache invalidation: event-driven (on update), time-based (TTL), or both
6. Poor Decomposition
What to look for in a plan:
- Steps combining unrelated concerns (auth + business logic + presentation)
- Unclear ownership — which module/layer owns each step?
- Cross-cutting steps that appear multiple times without abstraction
- "Also update X" or "also send to Y" appended to otherwise focused steps
How to flag:
[DECOMPOSITION ISSUE] Step {N}: "{quoted plan text}" — combines {concern A} and {concern B}.
Resolution: {concern A} belongs in {layer/module}; {concern B} belongs in {layer/module}.
What to suggest instead:
- Assign each step to exactly one layer: presentation, business logic, data access
- Extract cross-cutting concerns (logging, auth, validation) to middleware or decorators
- Each module should have a clear bounded context — changes to one shouldn't require changes to another
Extended References
| Reference | Content |
|---|
references/anti-patterns.md | Before/after plan examples for each anti-pattern |
Severity Guidelines
| Level | Criteria |
|---|
| CRITICAL | Anti-pattern that will cause correctness failures (data loss, security breach) |
| HIGH | Anti-pattern that will cause significant performance or maintainability problems |
| MEDIUM | Anti-pattern that will require architectural rework post-implementation |
| LOW | Anti-pattern that reduces code quality but is easily refactored |
