Code Review Recent Changes

Code Review: Recent Changes

Review the diff between HEAD and a fixed point the user supplies, along three independent axes:

• Standards - does the code conform to this repo's documented conventions?
• Spec - does the code faithfully implement the originating issue / PRD / spec?
• Maintainability - is the change structurally healthy, or did it leave the codebase harder to change?

Each axis runs as its own parallel sub-agent so they don't pollute each other's context, then this skill aggregates their findings. Keep them separate: a change can pass one axis and fail another - code that follows every convention but implements the wrong thing (Standards pass, Spec fail), or does exactly what the issue asked while leaving the codebase messier (Spec pass, Maintainability fail). Separate reporting stops one axis from masking another.

Review stance

Two ideas shape how the sub-agents work, so build them into the briefs:

• Read outward from the diff. An agent handed a diff tends to treat it as the edge of the world. Tell each sub-agent to look past it: for a changed symbol, read its surrounding function/file and the modules that call it or that it calls. A hunk that looks fine in isolation can duplicate an existing helper, contradict a sibling module's pattern, or leave a half-finished migration two files over.
• Be ambitious on Maintainability; precise on Standards and Spec. Standards and Spec are close to binary - a documented rule is violated or it isn't, a requirement is met or it isn't - so favour precision and don't manufacture findings. Maintainability is where the valuable, easy-to-miss findings live, so favour recall: propose a restructuring even when you're not fully sure, because a wrong suggestion costs the reader one quick "no", while a worthwhile one you never raise is one nobody gets to consider. To keep the wrong ones cheap to dismiss, every Maintainability finding carries a confidence label and concrete evidence.

Process

1. Pin the fixed point

Whatever the user named is the fixed point - a commit SHA, branch name, tag, main, HEAD~5. Pass it through; don't be opinionated. If they didn't give one, ask: "Review against what - a branch, a commit, or main?" Don't proceed without it.

Capture three things once and reuse them across all sub-agents:

• Diff: git diff <fixed-point>...HEAD (three-dot, so the comparison is against the merge-base).
• Commits: git log <fixed-point>..HEAD --oneline.
• Size deltas: git diff <fixed-point>...HEAD --stat - the Maintainability axis uses this to spot files crossing a size boundary.

Before going further, confirm the fixed point resolves (git rev-parse <fixed-point>) and the diff is non-empty. A bad ref or empty diff should fail here, not inside the parallel sub-agents.

2. Check for a spec

Scan the captured commit list and the user's request for a spec signal: an issue reference (#123, Closes #45, !67), a spec/PRD path the user passed, or an issue/PRD the user named. This is a cheap check - the commit list is already in context.

• A signal is present -> read references/spec-review.md and follow it to locate the spec and build the Spec sub-agent brief.
• No signal -> skip the Spec axis. Don't open the reference; note "no spec available" in the final report.

3. Identify the standards sources

Anything in the repo that documents how code should be written:

• CLAUDE.md, AGENTS.md, CONTRIBUTING.md
• CONTEXT.md / CONTEXT-MAP.md (including per-directory ones); docs/adr/ (architectural decisions are standards)
• STYLE.md, STANDARDS.md, STYLEGUIDE.md at the repo root or under docs/
• .editorconfig, eslint.config.*, biome.json, prettier.config.*, tsconfig.json - machine-enforced; note them but don't re-check what tooling already enforces.

Collect the list; the Standards sub-agent reads them.

On top of whatever the repo documents, the Standards axis always carries a smell baseline - a fixed set of Fowler code smells (Refactoring, ch.3) that applies even when the repo documents nothing. Two rules bind it:

• The repo overrides. A documented repo standard always wins; where it endorses something the baseline would flag, suppress the smell.
• Always a judgement call. Each smell is a labelled judgement call ("possible Primitive Obsession"), never a hard violation - and, like any standard here, skip anything tooling already enforces.

Each smell reads what it is -> how to fix; match it against the diff:

• Mysterious Name - a function, variable, or type whose name doesn't reveal what it does or holds. -> rename it; if no clear name comes, the design's murky.
• Data Clumps - the same few fields or params keep travelling together (a type wanting to be born). -> bundle them into one type, pass that.
• Primitive Obsession - a primitive or string standing in for a domain concept that deserves its own type. -> give the concept its own small type.
• Repeated Switches - the same switch/if-cascade on the same type recurs across the change. -> replace with polymorphism, or one map both sites share.
• Message Chains - long a.b().c().d() navigation the caller shouldn't depend on. -> hide the walk behind one method on the first object.

The structural smells from the full Fowler baseline (Duplicated Code, Feature Envy, Shotgun Surgery, Divergent Change, Speculative Generality, Middle Man, Refused Bequest) live on the Maintainability axis instead, so they aren't double-reported.