Review Axes

Two-axis review of the diff between HEAD and a fixed point the user supplies:

• Standards: does the code conform to this repo's documented coding standards?
• Spec: does the code faithfully implement the originating ticket or spec?

Both axes run as parallel sub-agents so they don't pollute each other's context, then this skill aggregates their findings.

Specs default to local markdown: a spec or ticket file under .scratch/ or docs/, which needs no setup. Only when the spec lives in a remote tracker (GitHub/GitLab) do you need docs/agents/issue-tracker.md; run /setup-skills to configure it.

Process

1. Pin the fixed point

Whatever the user said is the fixed point (a commit SHA, branch name, tag, main, HEAD~5, etc.). If they didn't specify one, ask for it.

Capture the diff command once: git diff <fixed-point>...HEAD (three-dot, so the comparison is against the merge-base). Also note the list of commits via git log <fixed-point>..HEAD --oneline.

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 two parallel sub-agents.

Working-tree mode. When the fixed point is given as the unstaged working tree (the caller says "the unstaged working tree", "my uncommitted changes", or similar) the changes under review were never committed, so no ref can name them:

• The diff command is plain git diff (working tree vs. index). There is no commit list.
• Skip the git rev-parse check; there's no ref to resolve. Still fail on an empty diff.
• Run git add -N . first so newly created files show up in git diff, without it they are invisible and the review silently passes over whole new files.

This is the mode /implement and /delegate-tickets use, since neither commits. In /delegate-tickets the index deliberately holds earlier tickets' work, so git diff isolates exactly the current ticket. Never run git add, git commit, or git reset in this mode beyond the git add -N . above.

2. Identify the spec source

Look for the originating spec, in this order:

1. A path the user passed as an argument.
2. A spec file under .scratch/, docs/, or specs/ matching the branch name or feature: the default home for local-markdown specs.
3. Issue references in the commit messages (#123, Closes #45, GitLab !67, etc.): fetch via the workflow in docs/agents/issue-tracker.md (only when a remote tracker is configured).
4. If nothing is found, ask the user where the spec is. If they say there isn't one, the Spec sub-agent will skip and report "no spec available".

3. Identify the standards sources

Anything in the repo that documents how code should be written, such as CODING_STANDARDS.md or CONTRIBUTING.md.

On top of whatever the repo documents, the Standards axis always carries two fixed baselines below, which apply even when a repo documents nothing: the smell baseline (Fowler code smells, Refactoring, ch.3) and the performance baseline (repeated-work patterns, N+1 chief among them). Two rules bind both:

• The repo overrides. A documented repo standard always wins; where it endorses something a baseline would flag, suppress the finding.
• Always a judgement call. Each entry is a labelled heuristic ("possible Feature Envy", "possible N+1"), never a hard violation. Like any standard here, skip anything tooling already enforces.

Smell baseline

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 honest name comes, the design's murky.
• Duplicated Code: the same logic shape appears in more than one hunk or file in the change, or the diff re-implements something the codebase already has. → extract the shared shape and call it from both, or call the existing implementation. Look for all three shapes: (a) copy-paste between the diff's own hunks; (b) a near-copy with one value or branch changed, which wants a parameter, not a second copy; (c) a helper, validator, mapper, query, or constant that already exists elsewhere in the repo. For (c), search the codebase for the new function's name and for a distinctive line of its body before calling it new.
• Feature Envy: a method that reaches into another object's data more than its own. → move the method onto the data it envies.
• 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.
• Shotgun Surgery: one logical change forces scattered edits across many files in the diff. → gather what changes together into one module.
• Divergent Change: one file or module is edited for several unrelated reasons. → split so each module changes for one reason.
• Speculative Generality: abstraction, parameters, or hooks added for needs the spec doesn't have. → delete it; inline back until a real need shows.
• Message Chains: long a.b().c().d() navigation the caller shouldn't depend on. → hide the walk behind one method on the first object.
• Middle Man: a class or function that mostly just delegates onward. → cut it, call the real target direct.
• Refused Bequest: a subclass or implementer that ignores or overrides most of what it inherits. → drop the inheritance, use composition.

Performance baseline

The same what it is → how to fix shape, aimed at work the change repeats once per row instead of once per request. The tell is always the same: an expensive call (a query, an HTTP request, a file read, a hash or crypto op) sitting inside a loop or a per-item callback whose length is data, not a constant.

• N+1 queries: one query fetches N rows, then each row triggers another query. → fetch the related rows in one round trip (a join, an IN (...), an ORM eager-load such as include / select_related / with / JOIN FETCH) or batch the second query by collected keys. Look for a DB call inside for/forEach/map/comprehension, a lazy relation touched inside a loop or inside a serializer/template/resolver rendering a list, and a GraphQL field resolver that queries per parent instead of going through a batching loader.
• N+1 network calls: the same shape with an HTTP/RPC/queue call per item. → use the bulk endpoint if one exists, otherwise bound the concurrency and gather the results in one pass.
• Missing index for a new access path: the diff filters, joins, sorts, or enforces uniqueness on a column with no supporting index, or adds one migration without the other. → add the index in the same migration, and say which columns and in which order.
• Unbounded result set: a query, scan, or fetch-all with no limit, pagination, or projection, driven by data that grows. → paginate, or select only the columns actually used.
• Repeated work in a loop: a value that doesn't depend on the iteration (a compiled regex, a config lookup, a client construction, a sort, a length query) recomputed each pass. → hoist it above the loop.
• Accidentally quadratic lookup: a nested scan (find/includes/indexOf inside a loop over the same collection) where a map or set would do. → build the index once, look up in constant time.

Each finding must name where the multiplier comes from: which collection is iterated, and what runs per element. A per-item call over a list whose size is a fixed small constant is not a finding.

4. Spawn both sub-agents in parallel

Send a single message with two parallel sub-agent calls (Agent in Claude Code, Task in Cursor). Use the general-purpose / generalPurpose subagent for both.

Pick the model by host (keep this review cheap):

Standards sub-agent prompt should include:

• The full diff command and commit list.
• The list of standards-source files you found in step 3, plus both baselines from step 3 (smell and performance) pasted in full (the sub-agent has no other access to them).
• The brief: "Report, per file/hunk where relevant, (a) every place the diff violates a documented standard: cite the standard (file + the rule); (b) any baseline smell you spot: name it and quote the hunk; and (c) any performance-baseline finding: name it, quote the hunk, and say which collection drives the multiplier and what runs per element. For duplication and for reuse of something that already exists, grep the repo before claiming a hunk is new, and cite the existing implementation's path. Distinguish hard violations from judgement calls: documented-standard breaches can be hard, but baseline findings are always judgement calls, and a documented repo standard overrides the baselines. Skip anything tooling enforces. Under 500 words."

Spec sub-agent prompt should include:

• The diff command and commit list.
• The path or fetched contents of the spec.
• The brief: "Report: (a) requirements the spec asked for that are missing or partial; (b) behaviour in the diff that wasn't asked for (scope creep); (c) requirements that look implemented but where the implementation looks wrong. Quote the spec line for each finding. If the spec has markdown checkboxes (- [ ] / - [x]), also list each checkbox criterion as done or not done based on the code in the diff, not on intent. Under 400 words."

If the spec is missing, skip the Spec sub-agent and note this in the final report.

5. Aggregate

Present the two reports under ## Standards and ## Spec headings, verbatim or lightly cleaned. Do not merge or rerank findings: the two axes are deliberately separate (see Why two axes).

End with a one-line summary: total findings per axis, and the worst issue within each axis (if any). Don't pick a single winner across axes: that's the reranking the separation exists to prevent.

6. Sync acceptance-criteria checkboxes

After the Spec report, update the local markdown spec/ticket source (the file from step 2: typically under .scratch/, docs/, or specs/) so its checkboxes match what the code actually did:

• Flip - [ ] → - [x] only when that criterion is implemented in the diff / codebase (use the Spec sub-agent's done/not-done list; verify against the diff when unsure).
• Leave - [ ] (or flip - [x] → - [ ]) when the criterion is missing, partial, or wrong per Spec.
• Edit existing checkbox lines only: do not add, delete, or rewrite criterion text.
• Skip this step when there is no local markdown spec, the spec has no checkboxes, or the Spec axis was skipped.
• Do not commit these markdown edits unless the user asks.

If the spec/ticket file has a Status: line (see docs/agents/triage-labels.md) and, after the flip above, every acceptance-criteria checkbox is - [x], advance Status: to ready-for-human: implementation is done and the work now needs human review before it merges. Leave Status: untouched when any checkbox remains unchecked, or when the file has no Status: line.

Tell the user briefly which boxes changed (checked / unchecked) and whether Status: was advanced, so they can see progress at a glance.

Why two axes

A change can pass one axis and fail the other:

• Code that follows every standard but implements the wrong thing → Standards pass, Spec fail.
• Code that does exactly what the issue asked but breaks the project's conventions → Spec pass, Standards fail.

Reporting them separately stops one axis from masking the other.