---
name: implementation-review-checklist
description: MUST run after superpowers plan execution completes, before finishing-a-development-branch. Use after implementing code to catch reinvented utilities, pattern mismatches, type safety issues, and convention violations. Triggers on "review for reuse", "check implementation quality", finishing plan execution, or before merging.
---

# Implementation Review Checklist

## Overview

A structured review that catches bugs the standard code-quality review misses — specifically the "reinvented wheel" and "pattern mismatch" categories that account for most post-implementation fixes in agent-written code.

**Core principle:** Every new utility, pattern, or convention introduced by the implementation is suspect until proven necessary. The codebase already has solutions — find them.

**Announce at start:** "I'm using the implementation-review-checklist to verify this code against codebase conventions."

## When to Use

- After implementing a feature, before committing or during code review
- As an addition to superpowers' code-quality-reviewer — run AFTER their standard review
- When user says "review for reuse", "check what I missed", "did I reinvent anything"
- After executing a superpowers implementation plan

**Do NOT use for:**
- Pure refactoring (no new functionality)
- Config/dependency changes
- Documentation-only changes

## The Checklist

Run each check. Report PASS or FAIL with file:line references for failures.

### 1. Reuse Check

**For each new function, utility, or helper introduced by this implementation:**

- [ ] Grep the codebase for functions that do the same thing
- [ ] Check the module's existing imports — do sibling files import something similar?
- [ ] Check `common/utils`, `common/hooks`, shared packages

**How to search:**
```
# Search for the concept, not the exact name
grep -r "buildPayload\|createPayload\|Payload" --include="*.ts" path/to/module/
grep -r "ErrorResponse\|errorText\|parseError" --include="*.ts" path/to/app/
```

**FAIL if:** An existing utility does the same thing and the new code doesn't use it.

### 2. Sibling Consistency

**For each UI pattern in the implementation (error display, loading states, empty states, banners):**

- [ ] Find the nearest sibling component that handles the same concern
- [ ] Compare: component used, variant, styling, placement
- [ ] Check that error/loading/empty states match the sibling's approach

**How to check:**
1. List files in the same directory/module
2. Open the closest sibling (same type of component)
3. Compare the specific pattern

**FAIL if:** The implementation uses a different variant, styling, or component than its siblings for the same concern.

### 3. Type Safety

- [ ] No `any` types in function signatures, hook return types, or public interfaces
- [ ] No `as` type casts that suppress real type errors (casts for test mocks are OK)
- [ ] No `Record<string, any>` — use proper typed records
- [ ] Form value types are concrete union types, not `any`

**How to check:**
```
grep -n "any\b" path/to/new/files.ts
grep -n "\bas\b" path/to/new/files.ts
```

**FAIL if:** `any` appears in non-test code, or `as` casts hide real type mismatches.

### 4. Race Conditions

- [ ] Async form submission has a double-submit guard (useRef or disabled state)
- [ ] No stale closures in async callbacks (check useCallback dependencies)
- [ ] Concurrent API calls are handled (abort controller, or sequential by design)

**How to check:**
1. Find all `async` functions in hooks
2. Check if there's a guard (useRef `isSaving`, `isLoading` check at start)
3. Verify useCallback dependency arrays include all referenced state

**FAIL if:** A save/submit handler can be called twice in rapid succession without guard.

### 5. Numeric Precision

- [ ] Price/percentage calculations use the project's precision library (Decimal.js, BigNumber, etc.)
- [ ] No raw float arithmetic for money: `price * (1 - discount / 100)` is a bug
- [ ] Rounding is explicit, not implicit via float truncation

**How to check:**
```
grep -n "price\|Price\|discount\|Discount\|percent" path/to/new/files.ts
# Check if any arithmetic uses raw * / + - on numeric price values
```

**FAIL if:** Money math uses raw JavaScript arithmetic instead of a precision library.

### 6. Internationalization

- [ ] No hardcoded English strings in user-facing UI or error messages
- [ ] Error messages use `intl.formatMessage` or the module's i18n pattern
- [ ] If the module uses an i18n system, new strings are added to message files

**How to check:**
```
grep -n "Error\|Failed\|Success\|Please\|Cannot" path/to/new/files.ts
# Check if these are wrapped in intl.formatMessage or equivalent
```

**FAIL if:** A user-visible string is a raw English literal not going through i18n.

### 7. Validation Wiring

For implementations that include forms:

- [ ] If a validation function exists, it's actually wired to the form (`validate` prop on `<Form>`)
- [ ] Required fields have validation, not just `required` HTML attribute
- [ ] Validation errors are displayed (not silently swallowed)

**How to check:**
1. Find all `validate` functions exported from the module
2. Grep for where they're imported
3. Verify they're passed to `<Form validate={...}>` or equivalent

**FAIL if:** A validation function exists but isn't connected to the form component.

### 8. Test Robustness

- [ ] No locale-dependent assertions (no `$`, `EUR`, specific decimal separators)
- [ ] Price/number assertions use regex patterns or `toMatch(/1[.,]40/)` instead of exact strings
- [ ] Test mocks don't hide real integration issues (mock at boundaries, not internals)
- [ ] New hooks/components used in existing test files have their mocks added

**How to check:**
```
grep -n "€\|\\$\|\£\|USD\|EUR" path/to/new/*.spec.ts
grep -n "toHaveTextContent.*[0-9]" path/to/new/*.spec.ts
```

**FAIL if:** Tests assert on locale-specific formatting that would break in a different locale.

## Output Format

```markdown
## Implementation Review: [Feature Name]

| # | Check | Status | Details |
|---|-------|--------|---------|
| 1 | Reuse | PASS/FAIL | [specifics] |
| 2 | Sibling consistency | PASS/FAIL | [specifics] |
| 3 | Type safety | PASS/FAIL | [specifics] |
| 4 | Race conditions | PASS/FAIL | [specifics] |
| 5 | Numeric precision | PASS/FAIL | [specifics] |
| 6 | i18n | PASS/FAIL | [specifics] |
| 7 | Validation wiring | PASS/FAIL | [specifics] |
| 8 | Test robustness | PASS/FAIL | [specifics] |

### Failures

#### [Check name]: [file:line]
**Found:** [what the code does]
**Expected:** [what it should do, with reference to existing code]
**Fix:** [specific action]
```

## Calibration

**Critical failures** (must fix before merge):
- Reuse: existing utility ignored, manual reimplementation
- Type safety: `any` in public interfaces
- Race conditions: no double-submit guard on save handler
- Numeric precision: raw float math on money values

**Important failures** (should fix):
- Sibling consistency: different UI pattern than neighbors
- i18n: hardcoded strings
- Validation wiring: validator exists but not connected

**Minor failures** (note for follow-up):
- Test robustness: locale-dependent assertions
- Reuse: utility exists but in a distant module (questionable import)

## Integration with Superpowers

This skill complements superpowers' code-quality-reviewer. The standard reviewer checks:
- File responsibility and decomposition
- Clean code and maintainability
- Test coverage

This checklist adds:
- Codebase-aware reuse verification
- Pattern consistency with siblings
- Domain-specific checks (precision, i18n, race conditions)

**Recommended flow:**
```
superpowers spec-reviewer → superpowers code-quality-reviewer → implementation-review-checklist
```

If a discovery report exists (from codebase-discovery skill), use its "Constraints for the Plan" section as the ground truth for the reuse check — every "DO use" constraint should be verified in the implementation.