claudeframework: Skill for Claude Code

Review the current implementation changes and produce a written review report.

IMPORTANT: For best results, run this skill in a fresh session (/clear first) so the review is unbiased by the implementation context.

Use a subagent to do the file investigation so the reading does not consume the main session's context. The subagent should return its findings and you write the report from those findings.

Steps:

1. Read CLAUDE.md for project context and conventions
2. If $ARGUMENTS is provided, treat it as a spec filename and read .claude/specs/<name>.md to understand what was intended
3. Phase awareness: check if .claude/specs/<name>-phases.md exists. If it does:
   • Read it and identify the current phase (the phase with status in-progress, or the latest done phase)
   • Scope all review lenses to the current phase's requirements and its assigned contract items only
   • Requirements belonging to later phases (status: pending) are explicitly out of scope — do not flag them as missing in Correctness, PM, or Spec validation
   • State at the top of the review report: "Reviewing Phase N of M — later-phase items are out of scope"
   • On the final phase (no pending phases remain), review against the full spec to catch cross-phase integration issues
4. Read all files in .claude/context/ if the directory exists — for additional project context when evaluating correctness
5. Detect review mode — check if .claude/reviews/<name>-review.md already exists from a previous cycle:
   • If no previous review exists → full review mode (steps 6-8 below)
   • If a previous review exists → delta review mode (step 9 below)

Full Review Mode (first review cycle)

1. Identify what changed: use the project's version control system (e.g. git diff main or git diff HEAD~1); if not in a VCS repo, ask the user which files to review
2. Read each changed file in full
3. Evaluate against these criteria (multi-role review — address each lens):
   • Correctness: does it match the spec requirements? Are there logic bugs or missing cases?
   • Code quality: does it follow existing patterns? Is anything over-engineered or unnecessarily complex?
   • Security: any injection risks, exposed secrets, or missing input validation at system boundaries? OWASP top-10 considerations?
   • Tests / QA: are the relevant cases covered? Are there obvious edge-case gaps? Any regression risk to existing behavior? Are component-level rendering tests present (not just pure-function tests)?
   • UX / Minimal impact: does the change touch only what's necessary? Any scope creep? Are any fixes patches hiding a root-cause problem that should be fixed properly? For components displaying computed numbers, check for floating-point precision issues (use .toFixed() or Math.round()). For components with reactive state (useEffect + Zustand/Redux), check that dismissals, resets, and one-time actions cannot be re-triggered by unrelated state changes.
   • PM: does the change deliver business value? Does it align with the stated goal in the spec? Is anything built that wasn't asked for?
   • DevOps: any CI/CD implications? Environment variables, build config, or deployment steps affected? Any observability gaps (missing logs, metrics)?
   • Contract validation: read the spec's ## Contract section and give a verdict per item, never a single global one. This skill has no Bash tool and therefore does not execute the verify commands — running them is /ship Step 5's job. What you check here is whether each binding is real:
     • ✓ bound — the command's target exists (the named test file and test function are present in the diff or already in the codebase; the e2e spec path resolves) and its assertions actually cover the stated criterion. A test whose name matches the filter but asserts something else is ✗, not ✓.
     • ✗ unbound — the verify names a test that does not exist, a filter that would match nothing, or a test that asserts something other than the criterion. Severity: major. This is worse than an admitted gap: the Step 5 gate reports green and everyone downstream believes the criterion was proven. Also ✗ at major severity: a type: e2e item with no env field, or one bound to env: deployed whose criterion covers behaviour this diff introduces — that behaviour is not deployed at gate time, so the binding would fail correct code (or, once the old deployment coincidentally satisfies it, pass without proving anything).
     • manual — the item declares manual. Check whether the stated reason is still true (it claims no snapshot harness exists — has one appeared since?). If the reason has become false, flag minor: a criterion is being hand-waved that could now be automated.