From 9fecab929abb904c68ce3366a1781df31ab22832 Mon Sep 17 00:00:00 2001 From: Matt Pocock Date: Sun, 10 May 2026 20:59:48 +0100 Subject: [PATCH] Add review skill to facilitate two-axis code reviews --- skills/in-progress/README.md | 1 + skills/in-progress/review/SKILL.md | 78 ++++++++++++++++++++++++++++++ 2 files changed, 79 insertions(+) create mode 100644 skills/in-progress/review/SKILL.md diff --git a/skills/in-progress/README.md b/skills/in-progress/README.md index 1e634f5..8db2399 100644 --- a/skills/in-progress/README.md +++ b/skills/in-progress/README.md @@ -3,6 +3,7 @@ Skills that are still being developed. They're not ready to ship — expect rough edges, breaking changes, and abandoned experiments. They're excluded from the plugin and the top-level README until they graduate to a stable bucket. - **[handoff](./handoff/SKILL.md)** — Compact the current conversation into a handoff document so another agent can continue the work. +- **[review](./review/SKILL.md)** — Review changes since a fixed point along two parallel axes: **Standards** (does the diff follow the repo's coding standards?) and **Spec** (does the diff faithfully implement the originating issue/PRD?). - **[writing-beats](./writing-beats/SKILL.md)** — Shape an article as a journey of beats, choose-your-own-adventure style. Pick a starting beat, write only that beat, then pivot to the next, until the article reaches a natural end. - **[writing-fragments](./writing-fragments/SKILL.md)** — Grilling session that mines you for fragments — heterogeneous nuggets of writing — and appends them to a single document as raw material for a future article. - **[writing-shape](./writing-shape/SKILL.md)** — Take a markdown file of raw material and shape it into an article paragraph by paragraph, arguing format choices at each step. diff --git a/skills/in-progress/review/SKILL.md b/skills/in-progress/review/SKILL.md new file mode 100644 index 0000000..7507a36 --- /dev/null +++ b/skills/in-progress/review/SKILL.md @@ -0,0 +1,78 @@ +--- +name: review +description: Review the changes since a fixed point (commit, branch, tag, or merge-base) along two axes — Standards (does the code follow this repo's documented coding standards?) and Spec (does the code match what the originating issue/PRD asked for?). Runs both reviews in parallel sub-agents and reports them side by side. Use when the user wants to review a branch, a PR, work-in-progress changes, or asks to "review since X". +--- + +# Review + +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 issue / PRD / spec? + +Both axes run as **parallel sub-agents** so they don't pollute each other's context, then this skill aggregates their findings. + +The issue tracker should have been provided to you — run `/setup-matt-pocock-skills` if `docs/agents/issue-tracker.md` is missing. + +## Process + +### 1. Pin the fixed point + +Whatever the user said is the fixed point — a commit SHA, branch name, tag, `main`, `HEAD~5`, etc. Don't be opinionated; pass it through. If they didn't specify one, ask: "Review against what — a branch, a commit, or `main`?" Don't proceed until you have it. + +Capture the diff command once: `git diff ...HEAD` (three-dot, so the comparison is against the merge-base). Also note the list of commits via `git log ..HEAD --oneline`. + +### 2. Identify the spec source + +Look for the originating spec, in this order: + +1. Issue references in the commit messages (`#123`, `Closes #45`, GitLab `!67`, etc.) — fetch via the workflow in `docs/agents/issue-tracker.md`. +2. A path the user passed as an argument. +3. A PRD/spec file under `docs/`, `specs/`, or `.scratch/` matching the branch name or feature. +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. Common locations: + +- `CLAUDE.md`, `AGENTS.md` +- `CONTRIBUTING.md` +- `CONTEXT.md`, `CONTEXT-MAP.md`, per-context `CONTEXT.md` files +- `docs/adr/` (architectural decisions are standards) +- `.editorconfig`, `eslint.config.*`, `biome.json`, `prettier.config.*`, `tsconfig.json` (machine-enforced standards — note them but don't re-check what tooling already checks) +- Any `STYLE.md`, `STANDARDS.md`, `STYLEGUIDE.md`, or similar at the repo root or under `docs/` + +Collect the list of files. The **Standards** sub-agent will read them. + +### 4. Spawn both sub-agents in parallel + +Send a single message with two `Agent` tool calls. Use the `general-purpose` subagent for both. + +**Standards sub-agent prompt** — include: + +- The full diff command and commit list. +- The list of standards-source files you found in step 3. +- The brief: "Read the standards docs. Then read the diff. Report — per file/hunk where relevant — every place the diff violates a documented standard. Cite the standard (file + the rule). Distinguish hard violations from judgement calls. Skip anything tooling enforces. Under 400 words." + +**Spec sub-agent prompt** — include: + +- The diff command and commit list. +- The path or fetched contents of the spec. +- The brief: "Read the spec. Then read the diff. 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. 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 so the user can see them independently. + +End with a one-line summary: total findings per axis, and the worst single issue (if any) flagged. + +## 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.