Review Implementation

---
name: review-implementation
description: Review a code implementation for correctness, quality, test coverage, security, performance, and spec alignment.
---

# Review Implementation

Audits a code implementation and reports findings across eight categories: correctness, code quality, test coverage, security, performance, spec alignment, reversibility, and forward compatibility.
Each finding is prioritized with 🔴 MUST fix, 🟡 SHOULD fix, or 🟢 MAY fix.
A 🔴 MUST finding must reach `L3 - Executed` or be marked `unproven` with the runtime evidence it needed and why that was infeasible.

## Prerequisites

- Apply the shared SDLC conventions in `skills/sdlc/references/shared.md`.
- Apply the shared evidence standard in [`skills/sdlc/references/evidence.md`](../sdlc/references/evidence.md): label every finding with a level in the form `L<n> - <Name>` plus an artifact.
- If no argument is provided, locate the feature directory under `.sdlc/features/` whose frontmatter `issue` field references `$ISSUE_NUMBER`.
- Code to review provided in context, as file paths to read, or as a diff
- Specification or acceptance criteria (optional, improves alignment check)
- `.sdlc/features/N-<slug>/lifecycle.md` (optional, if a lifecycle document was produced): verify state machines, transition guards, invariants, and retention policies are implemented correctly
- `.sdlc/features/N-<slug>/telemetry.md` (optional, if a telemetry plan was produced): verify analytics events are implemented correctly
- `.sdlc/features/N-<slug>/observability.md` (optional, if an observability plan was produced): verify logging, metrics, tracing, and health checks are implemented correctly

## Steps

1. Read the code thoroughly.
2. Cross-reference against the specification or acceptance criteria if provided.
3. Identify issues in each category below.
4. Prioritize each finding: 🔴 MUST, 🟡 SHOULD, 🟢 MAY.
5. Label each finding with an evidence level in the form `L<n> - <Name>` plus an artifact, per the shared evidence standard.
6. Report findings using the output format. Omit categories with no findings.
7. Write the findings to `.sdlc/features/N-<slug>/review-implementation.md` with frontmatter `artifact: implementation`, `verdict` (`approved` if there are no blocking findings, `changes-requested` if the author must address findings, `rejected` for a fundamental flaw), and `reviewed_at: <ISO date>`, and the findings as the body, per `skills/sdlc/references/shared.md`. Record any unresolved open questions in the findings body.

## Review Checklist

### Correctness
- Does the implementation meet all acceptance criteria?
- Are edge cases and error conditions handled?
- Are there logic errors, off-by-one errors, or incorrect conditionals?
- Is state managed correctly (no races, no stale data)?

### Code Quality
- Are names clear and consistent with codebase conventions?
- Is the code DRY without premature abstraction?
- Is complexity appropriate — are there simpler alternatives?
- Is dead code or commented-out code absent?

### Test Coverage
- Delegate the coverage analysis to [`/analyze-test-coverage`](../analyze-test-coverage/SKILL.md): invoke it with the implementation diff, and embed its three tables (introduced tests, change coverage, uncovered code) into the Test Coverage section below. Raise its findings in the findings body.
- Are tests verifying behavior rather than implementation details?
- Do tests cover error paths and edge cases?

### Security
- Is user input validated and sanitized?
- Are there hardcoded secrets or credentials?
- Are authorization checks in place for protected operations?
- Are SQL queries parameterized?

### Performance
- Are there N+1 queries or unnecessary repeated work?
- Is caching used appropriately?
- Are there blocking operations that should be async?

### Spec Alignment
- Does the implementation match the API contract (field names, types, status codes)?
- Are all specified behaviors implemented?
- Are there behaviors implemented that are not in the spec (scope creep)?
- If a lifecycle document exists, are all states, transitions, guard conditions, and invariants implemented correctly?
- If a lifecycle document exists, are retention and expiry policies enforced as specified?
- If a telemetry plan exists, are all analytics events emitted at the right locations with correct properties?
- If an observability plan exists, are all metrics, logs, traces, and health checks implemented per the plan?

### Reversibility
- Can we undo this cleanly if the change needs to be reverted?
- Are there irreversible side effects (destructive migrations, permanent data loss, one-way API transformations)?
- Are one-way-door design decisions called out explicitly?

### Forward Compatibility
- Can contracts and persisted data accept future additions without breaking (unknown fields tolerated, unknown enum values handled, additive-only changes)?
- Is there a versioning strategy so future evolution does not force coordinated upgrades on all consumers?
- Are extension points provided for known likely future change, or does the code assume a fixed set?

## Output Format

```markdown
## Summary

🔴 / 🟢 <Overall assessment in one sentence.>

## Correctness

<Findings with 🔴/🟡/🟢 priority, each with `Evidence: L<n> - <Name>, <artifact>`, or "No issues found.">

## Code Quality

<Findings (each with `Evidence: L<n> - <Name>, <artifact>`) or "No issues found.">

## Test Coverage

*Populated by [`/analyze-test-coverage`](../analyze-test-coverage/SKILL.md). Embed its three tables below and append any findings.*

### Introduced tests

| Test file | Test(s) | What it tests |
|---|---|---|
| <path> | <test name> | <behavior verified> |

### Change coverage

| Changed file | Behavior changed | Covered by test? | Gap |
|---|---|---|---|
| <path> | <description> | Yes / No | <gap or em-dash> |

### Uncovered code

| File | Function / branch / path | Why it matters |
|---|---|---|
| <path> | <description> | <risk if this code regresses silently> |

<Findings (each with `Evidence: L<n> - <Name>, <artifact>`) or "No issues found.">

## Security

<Findings (each with `Evidence: L<n> - <Name>, <artifact>`) or "No issues found.">

## Performance

<Findings (each with `Evidence: L<n> - <Name>, <artifact>`) or "No issues found.">

## Spec Alignment

<Findings (each with `Evidence: L<n> - <Name>, <artifact>`) or "No issues found.">

## Reversibility

<Findings (each with `Evidence: L<n> - <Name>, <artifact>`) or "No issues found.">

## Forward Compatibility

<Findings (each with `Evidence: L<n> - <Name>, <artifact>`) or "No issues found.">
```

## Outcome

If `$OUTCOME_YAML` is set, emit your verdict there per `skills/sdlc/references/shared.md`:

| Verdict | When |
|---|---|
| `approved` | No blocking findings; the subject passes review |
| `changes-requested` | Findings the author must address before it passes |
| `rejected` | Fundamental flaw requiring rework or stopping |

In the same emission, list the findings file under `artifacts:` (`.sdlc/features/N-<slug>/review-implementation.md`).

## Example Usage

**Scenario 1: Missing error handling**
Handler returns 500 for all errors instead of specific codes defined in the spec.
🔴 MUST fix.

**Scenario 2: N+1 query**
A loop fetches user data individually for each item in a list.
🟡 SHOULD fix with a batch query.

**Scenario 3: Variable naming**
Variable `d` used instead of `discount_rate`.
🟢 MAY improve.

## Next Step

Once all 🔴 MUST findings are resolved, continue with `/create-documentation`, then `/validate-implementation` to capture visual proof and get user sign-off, then `/create-pr` (opens the PR as a draft) and `/promote-pr` (reviews it and marks it ready only when it clears the review bar).