leogodin217/review-sprint

Review Sprint

Post-implementation review with fresh eyes. Run after implement-sprint completes.

Purpose

Catch issues that slip through when implementer is too close to the code:

• Dead code and scaffolding
• Test names that don't match test behavior
• Spec-implementation drift
• Uncovered error paths

Context Loading (Minimal)

Load ONLY:

1. CLAUDE.md - Principles (especially #7 and #8)
2. docs/sprints/current/spec.md - What was planned
3. Changed files this sprint (use git diff)
4. Test files for changed code

DO NOT load:

• Full architecture docs
• Historical context
• Capability docs

Fresh eyes means deliberately ignorant of the "why" — only checking the "what."

Review Process

1. Dead Code Scan

# Find scaffolding patterns
grep -rn "# Future:" src/
grep -rn "# TODO:" src/
grep -rn "pass$" src/

# Find unused variables (requires manual review)
# Look for: assigned but never read, precomputed but not used

Flag: Any loop body that only contains pass, continue, or comments.

Flag: Any __init__ that stores data not used by other methods.

2. Test Name Audit

For each test file changed this sprint:

1. Read test function name
2. Read test docstring
3. Read test body
4. Verify: Does the test actually test what the name claims?

Common lies:

• test_X_order that doesn't verify ordering
• test_X_deterministic that only runs once
• test_X_error that doesn't verify the error type/message

3. Coverage Analysis

# Get coverage for new files only
pytest --cov=src/fabulexa --cov-report=term-missing

# Check files added this sprint
git diff --name-only --diff-filter=A main...HEAD

Flag: Any new file with < 100% coverage.

Flag: Uncovered lines that are error conditions (even if "shouldn't happen").

4. Spec-Implementation Comparison

4a. Load Sprint Notes (if available)

Read implementation decisions attached to phase commits:

# Find sprint commits and read their notes
for sha in $(git log --oneline --grep="Sprint" | head -10 | awk '{print $1}'); do
    echo "=== $sha ==="
    git notes --ref refs/notes/agent/sprint show "$sha" 2>/dev/null || echo "(no notes)"
done

If notes exist, use the decisions field to understand why the implementer made specific choices. This enables checking intent-vs-implementation, not just spec-vs-implementation.

4b. Compare Contracts

For each contract in spec.md:

1. Find the implementation
2. Compare signature (params, types, return)
3. Compare docstring (Args, Returns, Raises)
4. Note any improvements or divergences
5. If sprint notes recorded a decision about this contract, verify the stated rationale matches the code

Document: If implementation is better than spec, note it for spec update.

Flag: If implementation is worse than spec, it's a bug.

Flag: If a noted decision contradicts the spec without justification, it's a deviation.

5. Workspace Check

git status --porcelain

Flag: Any untracked files that aren't in .gitignore.

6. Pre-commit Compliance

pre-commit run --all-files

Flag: Any pre-commit failure (formatting, linting, type errors).

7. Demo Verification

Run all demo scripts twice to verify determinism and consistency:

# First run
for demo in docs/sprints/current/demos/phase_*.py; do
    python "$demo"
done

# Second run
for demo in docs/sprints/current/demos/phase_*.py; do
    python "$demo"
done

Flag: Any demo that fails or produces different output between runs.

Output Format

Create review report in docs/sprints/current/review.md:

# Sprint Review: [Name]

**Date:** YYYY-MM-DD
**Reviewer:** Claude (fresh eyes)

## Summary

| Check | Status | Issues |
|-------|--------|--------|
| Dead code | PASS/FAIL | N issues |
| Test names | PASS/FAIL | N issues |
| Coverage | PASS/FAIL | N files < 100% |
| Spec sync | PASS/FAIL | N divergences |
| Workspace | PASS/FAIL | N artifacts |
| Pre-commit | PASS/FAIL | N failures |
| Demos | PASS/FAIL | N failures |

## Issues Found

### 1. [Issue Title]

**Category:** Dead code / Test name / Coverage / Spec sync / Workspace / Demos
**Location:** file:line
**Problem:** [What's wrong]
**Action:** [What to do]

---

## Recommendation

[ ] APPROVED - No blocking issues
[ ] REVISIONS NEEDED - Fix issues and re-review

When to Use

Run review-sprint:

• After all phases of implement-sprint complete
• Before /verify-sprint
• When you suspect quality issues

Review Pipeline

This command checks mechanical properties. Follow it with /verify-sprint for behavioral correctness.

implement-sprint (per phase)
    └── reviewer agent (quick check)

review-sprint (after all phases)
    └── mechanical audit: coverage, linting, dead code, test names

verify-sprint (after review-sprint passes)
    └── spec-fidelity audit: does code match spec's algorithms step-by-step?

/review-sprint catches sloppy code. /verify-sprint catches correct-looking code that does the wrong thing. Both must pass before merge.

Agent Instructions

When invoked, use the reviewer agent with these specific instructions:

1. Load minimal context (see above)
2. Run all 7 checks systematically
3. Document ALL findings (not just blockers)
4. Be skeptical — assume issues exist until proven otherwise
5. Produce the review report
6. Use LSP tools for code navigation — find_definition / find_references / get_incoming_calls to trace code, not Grep for definitions or call sites

The reviewer should NOT:

• Assume the implementer was right
• Skip checks because "it passed CI"
• Accept "it works" as proof of quality
• Grep for def foo or class Bar — use find_definition instead
• Grep a function name across directories to find callers — use find_references or get_incoming_calls