review-changes

Review Changes

Two axes, never merged. Code can pass one and fail the other, and merging lets one mask the other.

Inputs

• The diff: no argument -> working tree vs the default diff base; file/function -> that target and its impact; branch -> vs main/master; PR number -> the checked-out PR branch or local diff.
• .safe-code/context/code-standards.md (documented repo standards).
• The active spec, if any: .safe-code/context/feature-specs/<NN>-*.md with status: in-progress (or the one the user names).
• Graph accelerators when a codegraph index is ready (codegraph sync first): codegraph impact <symbol> for high-risk changes, codegraph callers <symbol> for changed signatures, codegraph affected <changed files> for the tests the diff reaches (over-reports — the safe side). Without them: git diff, rg, direct reads. Read full source only for changed or high-impact files.

Axis 1 — Standards

Does the change follow the documented standards, and does it avoid the smell baseline?

Smell baseline (Fowler), always carried against the diff: Mysterious Name · Duplicated Code · Feature Envy · Data Clumps · Primitive Obsession · Repeated Switches · Shotgun Surgery · Divergent Change · Speculative Generality · Message Chains · Middle Man · Refused Bequest. Plus two structural smells: a pure helper living in a heavy module drags that module into every importer (move it to a leaf module); two branches that check the same state with near-identical conditions leave a gap between them.

Binding rules: a documented repo standard always overrides the baseline; every smell is a labelled judgement call ("possible Feature Envy"), never a hard violation; skip anything tooling (lint, type-check, formatter) already enforces.

Test shapes to reject on this axis: implementation-coupled (mocks internal collaborators or asserts through a side channel — the tell is that it breaks on refactor while behaviour is unchanged); tautological (the assertion recomputes the expected value the way the code does, so it passes by construction — expected values must come from an independent source: a known-good literal, a worked example, the spec); horizontally sliced (all tests first, then all implementation — it verifies imagined behaviour); substring-matched (contains("valid") also passes on "invalid", or fails on a formatting change — assert the exact value or structure); simulated-only (fake timers or jsdom can hide bugs that exist only in the real runtime — ask for one real-runtime probe).

Evidence checks:

• A performance claim ("faster", "lighter") needs a before-baseline measured under the same conditions; without one it is labelled hypothesis. "Fewer lines" is not performance evidence.
• A changed setting that several paths share (timeout, pool size, retry count) is checked against each path's time budget using real latency, not just the path that motivated the change.
• Test depth by change size is allowed only when the project declares tiers in code-standards.md; never pick a lighter tier yourself — a short review compresses the report, never the verification.

Axis 2 — Spec

Only when an active spec exists; otherwise report Spec: no active spec. Does the change implement what the spec asked?

• Missing: a requirement in the spec with no corresponding change.
• Scope creep: a change no requirement asked for (note it; it may be prefactoring, which is fine if labelled).
• Wrongly implemented: the requirement is addressed but the behaviour differs from the spec's Design / Verify When Done.
• Reinvention: the spec or change adds a new module, table, or harness where something existing already does most of the job — name the existing one.

Output

## Standards
- [severity] path:line - issue, impact, fix
Worst issue: <one line, or none>

## Spec
- [missing | creep | wrong | reinvention] <requirement or change> - detail
Worst issue: <one line, or "no active spec">

## Summary
- Risk: <low|medium|high>
- Blast radius: <files/functions/flows>
- Tests checked: <commands or graph coverage>
- Open questions: <or none>

Findings first, ordered by severity within each axis; never re-rank across axes. Report only actionable findings.