Merge pull request #405 from mattpocock/rename/review-to-code-review

Rename review skill to code-review, promote to engineering
This commit is contained in:
Matt Pocock 2026-07-01 12:06:56 +01:00 committed by GitHub
commit 26512d74b1
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
17 changed files with 71 additions and 15 deletions

View file

@ -57,7 +57,7 @@ Optional. A short, checkable list of the observable signals that tell the reader
Always present. Situate the skill in the system in a sentence or two: Always present. Situate the skill in the system in a sentence or two:
- **Role.** Name it: a **chain step** (`grill-with-docs → to-prd → to-issues → tdd`), a **run-once setup** (`setup-matt-pocock-skills`), **periodic maintenance** (`improve-codebase-architecture`, "every few days"), or a **reach-for-it-anytime standalone** (`diagnosing-bugs`, `prototype`, `handoff`). A standalone's map is one honest sentence — far better than omitting the section. - **Role.** Name it: a **chain step** (`grill-with-docs → to-prd → to-issues → implement → code-review`), a **run-once setup** (`setup-matt-pocock-skills`), **periodic maintenance** (`improve-codebase-architecture`, "every few days"), or a **reach-for-it-anytime standalone** (`diagnosing-bugs`, `prototype`, `handoff`). A standalone's map is one honest sentence — far better than omitting the section.
- **Neighbours.** The one or two siblings that matter, each with a because-clause, linked absolutely. - **Neighbours.** The one or two siblings that matter, each with a because-clause, linked absolutely.
- **The map.** Point to [ask-matt](https://aihero.dev/skills-ask-matt), the router over the whole set, so this page stays a node and never has to redraw the graph. - **The map.** Point to [ask-matt](https://aihero.dev/skills-ask-matt), the router over the whole set, so this page stays a node and never has to redraw the graph.

View file

@ -0,0 +1,5 @@
---
"mattpocock-skills": minor
---
Rename the in-progress **`review`** skill to **`code-review`** and promote it from `in-progress/` to the `engineering/` bucket. It now ships in the plugin, is listed in the top-level and Engineering READMEs (Model-invoked), and has a human-facing docs page at `docs/engineering/code-review.md`. The `/implement` skill and docs now point at `/code-review`.

View file

@ -2,4 +2,4 @@
"mattpocock-skills": patch "mattpocock-skills": patch
--- ---
Give the in-progress **`review`** skill an always-on Fowler smell baseline on its Standards axis. A curated ~12 high-signal "Bad Smells in Code" (Mysterious Name, Duplicated Code, Feature Envy, Data Clumps, Primitive Obsession, Repeated Switches, Shotgun Surgery, Divergent Change, Speculative Generality, Message Chains, Middle Man, Refused Bequest) are inlined into `SKILL.md` as a fixed baseline alongside whatever the repo documents — not a new third axis. Two binding rules keep it safe: a documented repo standard overrides the baseline, and every smell is reported as a judgement call, never a hard violation. Give the in-progress **`code-review`** skill an always-on Fowler smell baseline on its Standards axis. A curated ~12 high-signal "Bad Smells in Code" (Mysterious Name, Duplicated Code, Feature Envy, Data Clumps, Primitive Obsession, Repeated Switches, Shotgun Surgery, Divergent Change, Speculative Generality, Message Chains, Middle Man, Refused Bequest) are inlined into `SKILL.md` as a fixed baseline alongside whatever the repo documents — not a new third axis. Two binding rules keep it safe: a documented repo standard overrides the baseline, and every smell is reported as a judgement call, never a hard violation.

View file

@ -13,6 +13,7 @@
"./skills/engineering/prototype", "./skills/engineering/prototype",
"./skills/engineering/domain-modeling", "./skills/engineering/domain-modeling",
"./skills/engineering/codebase-design", "./skills/engineering/codebase-design",
"./skills/engineering/code-review",
"./skills/productivity/grill-me", "./skills/productivity/grill-me",
"./skills/productivity/grilling", "./skills/productivity/grilling",
"./skills/productivity/handoff", "./skills/productivity/handoff",

View file

@ -164,6 +164,7 @@ Skills I use daily for code work.
- **[tdd](./skills/engineering/tdd/SKILL.md)** — Test-driven development with a red-green-refactor loop. Builds features or fixes bugs one vertical slice at a time. - **[tdd](./skills/engineering/tdd/SKILL.md)** — Test-driven development with a red-green-refactor loop. Builds features or fixes bugs one vertical slice at a time.
- **[domain-modeling](./skills/engineering/domain-modeling/SKILL.md)** — Actively build and sharpen a project's domain model — challenge terms against the glossary, stress-test with edge-case scenarios, and update `CONTEXT.md` and ADRs inline. - **[domain-modeling](./skills/engineering/domain-modeling/SKILL.md)** — Actively build and sharpen a project's domain model — challenge terms against the glossary, stress-test with edge-case scenarios, and update `CONTEXT.md` and ADRs inline.
- **[codebase-design](./skills/engineering/codebase-design/SKILL.md)** — Shared discipline and vocabulary for designing deep modules: a lot of behaviour behind a small interface, placed at a clean seam, testable through that interface. - **[codebase-design](./skills/engineering/codebase-design/SKILL.md)** — Shared discipline and vocabulary for designing deep modules: a lot of behaviour behind a small interface, placed at a clean seam, testable through that interface.
- **[code-review](./skills/engineering/code-review/SKILL.md)** — Two-axis review of the diff since a fixed point: **Standards** (does it follow the repo's coding standards, plus a Fowler smell baseline?) and **Spec** (does it faithfully implement the originating issue/PRD?), run as parallel sub-agents so neither pollutes the other.
### Productivity ### Productivity

View file

@ -24,7 +24,7 @@ Reach for it whenever you're unsure which skill or flow a situation calls for: y
## Flows, not just skills ## Flows, not just skills
The idea `ask-matt` gives you to think with is the **flow** — a path *through* the skills rather than a single one. Most work runs along one **main flow** (idea → ship: grill → PRD → issues → implement), two **on-ramps** merge onto it (a triage lane for incoming bugs and requests; a codebase-health lane that generates ideas), and everything else is a **standalone** you reach for on its own. Ask a question and you get placed on the right flow, at the right step — not just handed a tool. The idea `ask-matt` gives you to think with is the **flow** — a path *through* the skills rather than a single one. Most work runs along one **main flow** (idea → ship: grill → PRD → issues → implement → review), two **on-ramps** merge onto it (a triage lane for incoming bugs and requests; a codebase-health lane that generates ideas), and everything else is a **standalone** you reach for on its own. Ask a question and you get placed on the right flow, at the right step — not just handed a tool.
## Where it fits ## Where it fits

View file

@ -0,0 +1,47 @@
Quickstart:
```bash
npx skills add mattpocock/skills --skill=code-review
```
```bash
npx skills update code-review
```
[Source](https://github.com/mattpocock/skills/tree/main/skills/engineering/code-review)
## What it does
`code-review` reviews the diff between `HEAD` and a fixed point you supply — a commit, branch, tag, or merge-base — along two separate axes: **Standards** (does the code follow this repo's documented conventions?) and **Spec** (does it implement what the originating issue or PRD asked for?). It runs each axis as its own parallel sub-agent and reports them side by side. It never merges or re-ranks the two sets of findings — keeping them separate is the whole point, because a change can pass one axis and fail the other, and a single blended verdict lets one mask the other.
## When to reach for it
Type `/code-review`, or the agent reaches for it automatically when you ask to review a branch, a PR, work-in-progress changes, or anything "since X".
Reach for this when there is a diff to judge against a known-good point and you want the two questions — *is it built right?* and *is it the right thing?* — answered independently. It runs at the end of the build loop; for actually writing the code test-first, use [tdd](https://aihero.dev/skills-tdd), and for building a whole spec into code use [implement](https://aihero.dev/skills-implement), which runs its own `/code-review` pass before committing.
## Prerequisites
The **Spec** axis needs somewhere to find the originating spec — an issue reference in the commit messages, a path you pass in, or a PRD under `docs/`/`specs/`. That issue-tracker wiring comes from [setup-matt-pocock-skills](https://aihero.dev/skills-setup-matt-pocock-skills); without a spec the Spec axis simply skips and says so. The **Standards** axis needs nothing set up — it always carries a built-in Fowler smell baseline even in a repo that documents no conventions.
## Two axes, never merged
The defining idea is the **two axes**. **Standards** asks whether the diff conforms to how this repo writes code — its `CODING_STANDARDS.md` or `CONTRIBUTING.md`, plus a fixed baseline of ~12 Fowler code smells (Mysterious Name, Duplicated Code, Feature Envy, Data Clumps, …). Two rules keep the baseline safe: a documented repo standard always overrides it, and every smell is a judgement call, never a hard violation. **Spec** asks the orthogonal question — does the code do what the issue or PRD actually asked, without missing requirements or smuggling in scope creep?
They run as parallel sub-agents so neither pollutes the other's context, and the final report presents them under separate `## Standards` and `## Spec` headings with a per-axis summary. There is deliberately no single winner across axes.
## It's working if
- It pins and confirms the fixed point first (`git rev-parse`), failing fast on a bad ref or empty diff rather than inside the sub-agents.
- Standards and Spec findings arrive in two distinct blocks, each citing its source — a repo standard or baseline smell for one, a quoted spec line for the other.
- When no spec can be found, the Spec axis reports "no spec available" instead of inventing requirements.
## Where it fits
`code-review` is the review step at the tail of the main build chain:
```txt
grill-with-docs → to-prd → to-issues → implement → code-review
```
Its closest neighbour is [implement](https://aihero.dev/skills-implement), which drives the build and calls this as its own review pass before committing; upstream, the spec it checks against is produced by [to-prd](https://aihero.dev/skills-to-prd) and [to-issues](https://aihero.dev/skills-to-issues). When you're unsure which skill or flow fits, [ask-matt](https://aihero.dev/skills-ask-matt) routes you.

View file

@ -44,7 +44,7 @@ What makes this variant its own skill is where the answers go. As the grill runs
`grill-with-docs` is the opening step of the main build chain: `grill-with-docs` is the opening step of the main build chain:
```txt ```txt
grill-with-docs → to-prd → to-issues → implement grill-with-docs → to-prd → to-issues → implement → code-review
``` ```
It comes first, before anything is written down as a spec: it produces the shared understanding and settled vocabulary that [to-prd](https://aihero.dev/skills-to-prd) then synthesises into a PRD without re-interviewing you. Its close neighbours are [grilling](https://aihero.dev/skills-grilling), the same interview without the docs, and [domain-modeling](https://aihero.dev/skills-domain-modeling), the glossary-and-ADR discipline it drives. When you're unsure which skill or flow fits, [ask-matt](https://aihero.dev/skills-ask-matt) routes you. It comes first, before anything is written down as a spec: it produces the shared understanding and settled vocabulary that [to-prd](https://aihero.dev/skills-to-prd) then synthesises into a PRD without re-interviewing you. Its close neighbours are [grilling](https://aihero.dev/skills-grilling), the same interview without the docs, and [domain-modeling](https://aihero.dev/skills-domain-modeling), the glossary-and-ADR discipline it drives. When you're unsure which skill or flow fits, [ask-matt](https://aihero.dev/skills-ask-matt) routes you.

View file

@ -30,10 +30,10 @@ Around that core it keeps the loop tight — typecheck often, run single test fi
## Where it fits ## Where it fits
`implement` is the build step at the end of the main chain: `implement` is the build step near the end of the main chain, just before the review:
```txt ```txt
grill-with-docs → to-prd → to-issues → implement grill-with-docs → to-prd → to-issues → implement → code-review
``` ```
Reach for it after the work has been specced and sequenced, not before. Its key neighbours are [to-issues](https://aihero.dev/skills-to-issues), which produces the independently-grabbable tickets it works through, and [tdd](https://aihero.dev/skills-tdd), which it drives internally to write the tests at each seam before running its own `/review` pass and committing. When you're unsure which skill or flow fits, [ask-matt](https://aihero.dev/skills-ask-matt) routes you. Reach for it after the work has been specced and sequenced, not before. Its key neighbours are [to-issues](https://aihero.dev/skills-to-issues), which produces the independently-grabbable tickets it works through, and [tdd](https://aihero.dev/skills-tdd), which it drives internally to write the tests at each seam before running its own [code-review](https://aihero.dev/skills-code-review) pass and committing. When you're unsure which skill or flow fits, [ask-matt](https://aihero.dev/skills-ask-matt) routes you.

View file

@ -41,7 +41,7 @@ Refactoring only happens once the suite is green; never while red.
`tdd` is the red-green loop the main build chain runs to write code: `tdd` is the red-green loop the main build chain runs to write code:
```txt ```txt
grill-with-docs → to-prd → to-issues → implement grill-with-docs → to-prd → to-issues → implement → code-review
``` ```
[implement](https://aihero.dev/skills-implement) is the chain's final step, and it drives `tdd` internally to build each ticket test-first — so `tdd` is the engine inside that step rather than a step of its own. You can also reach for it directly, whenever there's a concrete behaviour to build without a full spec. Its other neighbour is [codebase-design](https://aihero.dev/skills-codebase-design), which it leans on to find deep-module seams worth testing at. When you're unsure which skill or flow fits, [ask-matt](https://aihero.dev/skills-ask-matt) routes you. [implement](https://aihero.dev/skills-implement) is the chain's build step, and it drives `tdd` internally to build each ticket test-first before handing off to [code-review](https://aihero.dev/skills-code-review) — so `tdd` is the engine inside that step rather than a step of its own. You can also reach for it directly, whenever there's a concrete behaviour to build without a full spec. Its other neighbour is [codebase-design](https://aihero.dev/skills-codebase-design), which it leans on to find deep-module seams worth testing at. When you're unsure which skill or flow fits, [ask-matt](https://aihero.dev/skills-ask-matt) routes you.

View file

@ -37,7 +37,7 @@ Before slicing, `to-issues` looks for prefactoring — "make the change easy, th
`to-issues` is a step in the main build chain: `to-issues` is a step in the main build chain:
```txt ```txt
grill-with-docs → to-prd → to-issues → implement grill-with-docs → to-prd → to-issues → implement → code-review
``` ```
It sits between [to-prd](https://aihero.dev/skills-to-prd), which hands it a settled spec with user stories to slice against, and [implement](https://aihero.dev/skills-implement), which builds each independently-grabbable issue, driving [tdd](https://aihero.dev/skills-tdd) internally to write the tests test-first. When you're unsure which skill or flow fits, [ask-matt](https://aihero.dev/skills-ask-matt) routes you. It sits between [to-prd](https://aihero.dev/skills-to-prd), which hands it a settled spec with user stories to slice against, and [implement](https://aihero.dev/skills-implement), which builds each independently-grabbable issue, driving [tdd](https://aihero.dev/skills-tdd) internally to write the tests test-first, before its [code-review](https://aihero.dev/skills-code-review) pass. When you're unsure which skill or flow fits, [ask-matt](https://aihero.dev/skills-ask-matt) routes you.

View file

@ -53,7 +53,7 @@ That matters for agentic development: a good interface gives tests something dur
`to-prd` is a step in the main build chain: `to-prd` is a step in the main build chain:
```txt ```txt
grill-with-docs → to-prd → to-issues → implement grill-with-docs → to-prd → to-issues → implement → code-review
``` ```
Reach for it after the plan and domain language are resolved, and before you break the work into implementation tickets. Its key neighbours are [grill-with-docs](https://aihero.dev/skills-grill-with-docs), which sharpens the context so the PRD is precise, and [to-issues](https://aihero.dev/skills-to-issues), which turns the PRD into independently-grabbable issues for [implement](https://aihero.dev/skills-implement) to build. When you're unsure which skill or flow fits, [ask-matt](https://aihero.dev/skills-ask-matt) routes you. Reach for it after the plan and domain language are resolved, and before you break the work into implementation tickets. Its key neighbours are [grill-with-docs](https://aihero.dev/skills-grill-with-docs), which sharpens the context so the PRD is precise, and [to-issues](https://aihero.dev/skills-to-issues), which turns the PRD into independently-grabbable issues for [implement](https://aihero.dev/skills-implement) to build. When you're unsure which skill or flow fits, [ask-matt](https://aihero.dev/skills-ask-matt) routes you.

View file

@ -24,3 +24,4 @@ Model- or user-reachable (rich trigger phrasing so the model can reach for them)
- **[tdd](./tdd/SKILL.md)** — Test-driven development with a red-green-refactor loop. Builds features or fixes bugs one vertical slice at a time. - **[tdd](./tdd/SKILL.md)** — Test-driven development with a red-green-refactor loop. Builds features or fixes bugs one vertical slice at a time.
- **[domain-modeling](./domain-modeling/SKILL.md)** — Actively build and sharpen a project's domain model — challenge terms, stress-test with scenarios, update `CONTEXT.md` and ADRs inline. - **[domain-modeling](./domain-modeling/SKILL.md)** — Actively build and sharpen a project's domain model — challenge terms, stress-test with scenarios, update `CONTEXT.md` and ADRs inline.
- **[codebase-design](./codebase-design/SKILL.md)** — Shared discipline and vocabulary for designing deep modules: small interfaces, clean seams, testable through the interface. - **[codebase-design](./codebase-design/SKILL.md)** — Shared discipline and vocabulary for designing deep modules: small interfaces, clean seams, testable through the interface.
- **[code-review](./code-review/SKILL.md)** — Two-axis review of the diff since a fixed point: **Standards** (does it follow the repo's coding standards, plus a Fowler smell baseline?) and **Spec** (does it faithfully implement the originating issue/PRD?), run as parallel sub-agents.

View file

@ -23,6 +23,8 @@ The route most work travels. You have an idea and want it built.
- **Yes****`/to-prd`** (turn the thread into a PRD) → **`/to-issues`** (split the PRD into independently-grabbable issues). Because the issues are independent, **clear context between each one**: start a fresh session per issue and kick off **`/implement`** by passing it the PRD and the single issue to work on. - **Yes****`/to-prd`** (turn the thread into a PRD) → **`/to-issues`** (split the PRD into independently-grabbable issues). Because the issues are independent, **clear context between each one**: start a fresh session per issue and kick off **`/implement`** by passing it the PRD and the single issue to work on.
- **No****`/implement`** right here, in the same context window. - **No****`/implement`** right here, in the same context window.
Either way, `/implement` closes out by running **`/code-review`** — a two-axis review (Standards + Spec) of the diff — before committing. Reach for **`/code-review`** on its own whenever you want to review a branch or PR against a fixed point.
### Context hygiene ### Context hygiene
Keep steps 13 in **one unbroken context window** — don't compact or clear until after `/to-issues` — so the grilling, PRD, and issues all build on the same thinking. Each `/implement` then starts fresh, working from the issue. Keep steps 13 in **one unbroken context window** — don't compact or clear until after `/to-issues` — so the grilling, PRD, and issues all build on the same thinking. Each `/implement` then starts fresh, working from the issue.

View file

@ -1,5 +1,5 @@
--- ---
name: review name: code-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". 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".
--- ---

View file

@ -10,6 +10,6 @@ Use /tdd where possible, at pre-agreed seams.
Run typechecking regularly, single test files regularly, and the full test suite once at the end. Run typechecking regularly, single test files regularly, and the full test suite once at the end.
Once done, use /review to review the work. Once done, use /code-review to review the work.
Commit your work to the current branch. Commit your work to the current branch.

View file

@ -4,7 +4,6 @@ Skills that are still being developed. They're not ready to ship — expect roug
- **[decision-mapping](./decision-mapping/SKILL.md)** — Turn a loose idea into a sequenced map of investigation tickets, then drive them to resolution one at a time. User-invoked. - **[decision-mapping](./decision-mapping/SKILL.md)** — Turn a loose idea into a sequenced map of investigation tickets, then drive them to resolution one at a time. User-invoked.
- **[loop-me](./loop-me/SKILL.md)** — Grill yourself into implementable workflow specs over multiple sessions, using the current directory as a stateful workspace. User-invoked. - **[loop-me](./loop-me/SKILL.md)** — Grill yourself into implementable workflow specs over multiple sessions, using the current directory as a stateful workspace. User-invoked.
- **[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?).
- **[wizard](./wizard/SKILL.md)** — Generate an interactive bash wizard that walks a human through a manual procedure (setup, a one-off migration, a state transition) — opening URLs, capturing values, writing `.env` and GitHub Actions secrets. User-invoked. - **[wizard](./wizard/SKILL.md)** — Generate an interactive bash wizard that walks a human through a manual procedure (setup, a one-off migration, a state transition) — opening URLs, capturing values, writing `.env` and GitHub Actions secrets. User-invoked.
- **[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-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-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.