Rename review skill to code-review and promote to engineering
Rename the in-progress `review` skill to `code-review` and move it from `skills/in-progress/` into the promoted `skills/engineering/` bucket. - Add it to `.claude-plugin/plugin.json` and the Model-invoked sections of the top-level and Engineering READMEs; drop it from the in-progress README. - Add a docs page at `docs/engineering/code-review.md`. - Point `/implement` and its docs at `/code-review`. - Update the smell-baseline changeset reference; add a changeset for the move. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
parent
5a4191541c
commit
14c13c5bf9
10 changed files with 59 additions and 5 deletions
5
.changeset/code-review-promote.md
Normal file
5
.changeset/code-review-promote.md
Normal 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`.
|
||||||
|
|
@ -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.
|
||||||
|
|
|
||||||
|
|
@ -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",
|
||||||
|
|
|
||||||
|
|
@ -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
|
||||||
|
|
||||||
|
|
|
||||||
47
docs/engineering/code-review.md
Normal file
47
docs/engineering/code-review.md
Normal 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.
|
||||||
|
|
@ -36,4 +36,4 @@ Around that core it keeps the loop tight — typecheck often, run single test fi
|
||||||
grill-with-docs → to-prd → to-issues → implement
|
grill-with-docs → to-prd → to-issues → implement
|
||||||
```
|
```
|
||||||
|
|
||||||
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` pass and committing. When you're unsure which skill or flow fits, [ask-matt](https://aihero.dev/skills-ask-matt) routes you.
|
||||||
|
|
|
||||||
|
|
@ -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.
|
||||||
|
|
|
||||||
|
|
@ -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".
|
||||||
---
|
---
|
||||||
|
|
||||||
|
|
@ -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.
|
||||||
|
|
|
||||||
|
|
@ -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.
|
||||||
|
|
|
||||||
Loading…
Add table
Add a link
Reference in a new issue