feat(skills,pipelines): make the review node review, and make a long run followable and affordable

Closes #24.

**The review node ran nothing.** Both templates declared
`/superpowers:requesting-code-review` — the skill about how to HAND OVER a
review, not one that reviews anything — so `agenthub lanes node review`
performed no review at all. They now declare the bare `/code-review`, Claude
Code's own. It carries no `plugin:` prefix, so `requiredSkillPlugins` skips it
and the doctor's required-plugin row is unchanged (verified against the live
function: still exactly the five superpowers skills the templates name).

**The rest is the brainstorm → implement → review span, made cheaper to run and
harder to lose track of.** `ship-feature` gains four sections it never had:

- **Right-size the run.** The pipeline is not free — every dispatching phase
  costs a model call and minutes, and the fix-loop pays it again each lap. A
  typo does not enter the pipeline at all; a small single-layer change skips the
  plan-debate, independent-test and gate agents and lets the user's acceptance
  be the gate; medium/large runs everything. Sizing is a claim made out loud,
  and a task that grows re-sizes upward.
- **Dispatching a judgement.** The fresh-context/FOREGROUND/never-`fork`/
  hand-only-verifiable-artifacts/one-parseable-line contract, written once
  instead of re-argued in three phases — plus the rule that was missing
  entirely: never dispatch what a command answers. An agent is for judgement,
  not for typing.
- **Resuming after a compaction.** Long runs outlive the context window; the
  lane skill had this and this one did not. Re-derive position from the declared
  stage and the diff, and treat a phase whose evidence you cannot find as one
  that did not happen.
- **A `Leave when:` line on every phase.** Objective exit criteria are what stop
  both the silent skip and the endless polish: intake leaves when the answers
  are in hand, review leaves when the latest pass over the CURRENT diff is
  clean, the gate leaves when verification really ran AND the user said yes.

And, narrower: intake asks only what the repo cannot answer (a question the code
could have answered spends the user's round and earns a worse answer);
implement works the approved plan task by task, re-checks each acceptance
criterion, heartbeats on long work, and treats a plan that turns out wrong as a
return to `plan` rather than a quiet substitution; tests runs the suite the diff
needs rather than both by reflex.
This commit is contained in:
2026-09-18 10:39:01 +07:00
parent e217314248
commit eab7c437df
7 changed files with 133 additions and 33 deletions
+1 -1
View File
File diff suppressed because one or more lines are too long
+7 -5
View File
@@ -556,6 +556,7 @@ The lane is resolved from the working directory, so no id is needed (pass one to
agenthub lanes node # list every node of this lane's pipeline and its command
agenthub lanes node intake # → claude "/superpowers:brainstorming"
agenthub lanes node intake the export button # extra words are appended to the prompt
agenthub lanes node review # → claude "/code-review"
agenthub lanes node gates # → agenthub lanes hook ci-gate
agenthub lanes node pr-open --print # show the argv, run nothing
```
@@ -1070,11 +1071,12 @@ command: it's whichever of the fields above the matching rule's `tool` carried.
### Superpowers skill invocations
The built-in `default` pipeline's `plan`, `implement`, `review`, and `ship`
nodes each carry a `{"tool": "Skill", "match": "..."}` rule matching the
Superpowers workflow skill names (`brainstorming`/`writing-plans`,
`executing-plans`/`subagent-driven-development`, `code-review`/
`requesting-code-review`, `finishing-a-development-branch`). Invoking one of
The built-in `default` pipeline's `intake`, `plan`, `implement`, `review`, and
`ship` nodes each carry a `{"tool": "Skill", "match": "..."}` rule matching the
workflow skill names they expect (`brainstorming`, `writing-plans`,
`executing-plans`/`subagent-driven-development`, Claude Code's own
`code-review` plus `requesting-code-review`/`receiving-code-review`,
`finishing-a-development-branch`). Invoking one of
these skills is a much stronger signal than a matched Bash command, but it is
still detection, not declaration — it renders dashed amber and never `done`,
same as every other detected stage.
+4 -2
View File
@@ -312,8 +312,10 @@ string a template names — as the pipeline's own `skill` or as one node's
`command` — has to come from an installed, enabled plugin. Today that means
**`superpowers`**, which `/agenthub:ship-feature` and `/agenthub:ship-feature-lane`
drive step by step (`brainstorming`, `writing-plans`,
`test-driven-development`, `requesting-code-review`,
`verification-before-completion`, `finishing-a-development-branch`). Install it
`test-driven-development`, `verification-before-completion`,
`finishing-a-development-branch`). The `review` node is the exception: it names
the bare `/code-review`, Claude Code's own review skill, which carries no
`plugin:` prefix and so requires nothing to be installed. Install it
with `claude plugin install superpowers@claude-plugins-official`.
The row is a **WARN, never a FAIL**: the dashboard, hooks and CLI all work
+1 -1
View File
@@ -107,7 +107,7 @@
"label": "Review",
"icon": "👀",
"gate": true,
"command": "/superpowers:requesting-code-review",
"command": "/code-review",
"aliases": [
"reviewing",
"code-review",
+1 -1
View File
@@ -142,7 +142,7 @@
"label": "Code review",
"icon": "👀",
"gate": true,
"command": "/superpowers:requesting-code-review",
"command": "/code-review",
"aliases": [
"reviewing",
"code-review",
+1
View File
@@ -78,6 +78,7 @@ Do NOT jump to code. Understand the requirement first.
- **Restate + quick scan.** Restate the requirement. Do a fast targeted scan of the relevant code (use the `Explore` agent for breadth) so your questions are grounded in what actually exists.
- **The brainstorm is mandatory, not conditional.** Run the **superpowers:brainstorming** skill here on every run — not only "if the requirement is fuzzy". A one-line requirement always leaves scope, edge cases and acceptance criteria unstated, and this stage is the only place left to find that out cheaply.
- **Ask only what the repo cannot answer.** Read first, ask second: a question whose answer is in the code, the tests, `CLAUDE.md` or the existing UI spends the human's one interactive round on something you could have looked up. What belongs to them is intent — scope, priorities, trade-offs, what "done" means — never facts.
- **Frontloaded Q&A — asked with `AskUserQuestion`, and you WAIT for the answers.** Put the questions to the human with the `AskUserQuestion` tool (batched, up to 4 per round, each with real options — the way plan mode asks), not as prose they may not answer. Ask **every** clarifying question in ONE batch: acceptance criteria, scope / non-goals, UI/UX specifics, data shapes, edge cases, which existing flows it touches. You may not answer them on the human's behalf and you may not go on to Stage 0b until they are answered — deciding for them here is what makes a whole autonomous pipeline build the wrong feature. Only if the human explicitly says to decide yourself do you proceed, and then you write the assumptions into the lane spec. **Sibling-surface check (mandatory):** if your scan shows the app has N parallel surfaces of the pattern the requirement touches (e.g. several foldered areas, several list pages sharing a component) and the requirement names fewer than N, explicitly ask "this exists in [all N places] — apply to all, or only [the named ones]?" A missed sibling here costs a full second pipeline pass when a reviewer catches it on the PR. Then activate a clean feature slot for this run: `agenthub feature activate <slug> --title "<short title>"` — Task B's `activate` archives whatever feature was previously active on this lane automatically, so this run's dashboard state starts clean without a separate "init" step. Then mark intake: `agenthub stage intake --status running`. After this the human is asked exactly twice more — to approve the plan (Stage 0b) and to accept the work before the PR is published (Stage 7) — plus any `blocked` escalation. Nothing else is interactive.
### 0b — Investigate & plan *(autonomous)*
+118 -23
View File
@@ -53,6 +53,68 @@ backgrounded Agent or helper finishes. So:
- Never narrate a wait you are not actually running. If you cannot sustain one,
say so plainly and stop.
## The shape of a run
Forward: `intake → plan → implement → tests → review → gate → ship → done`.
Two back-edges, and they are the point of the pipeline, not its failure mode:
- **`tests`/`review` → `implement`** — a failure or a finding sends you to the
code, then forward through `tests` and `review` again.
- **`gate` → `plan`** — a NO-GO about the APPROACH (wrong scope, wrong shape,
the requirement meant something else) replans and re-earns the user's
approval. A NO-GO about a DEFECT goes to `implement` instead.
Going back is a declaration: re-run `agenthub stage <node>` for the node you
dropped to. Detection cannot move a lane backward on its own, so a silent
rework leaves the dashboard showing a stage you already left, and the nodes
ahead keep the previous pass's evidence.
## Right-size the run before you start it
This pipeline is not free: every phase that dispatches an agent costs a model
call and minutes of wall clock, and the fix-loop pays it again on each lap.
Match its weight to the work, and say in one line which weight you picked.
| Work | What runs |
|---|---|
| **Trivial** — typo, rename, one-line fix, a question | Not this skill. Do it, run the touched suite, say what you did. |
| **Small** — one layer, a few files, no new interface | `intake` questions → `implement` → `tests` → `review` with `code-review` alone → `gate` (the user's acceptance IS the gate). Skip the plan-debate agent, the independent-test agent and the gate agent. |
| **Medium / large** — multi-file, new route/schema/interface, cross-layer, or anything you would defend in a review | Every phase below, in full. |
Sizing is a claim you make out loud, not a shortcut you take quietly. When
torn, take the heavier one. A small task that grows re-sizes upward mid-run —
re-declare the phase you re-enter and pick up the skipped agents from there.
## Dispatching a judgement (used by `tests`, `review`, `gate`)
One contract, written once, so the phases below don't each re-argue it:
- **Fresh context, FOREGROUND, never `fork`.** A fork inherits your context —
precisely the bias the dispatch exists to escape. Foreground because the phase
gates on the answer, and the agent's completion is what re-invokes you.
- **Hand only what the agent can verify for itself**: the repo path, the
requirement/plan, the `git diff` range, the commands to run. Never your
conclusions — "I already checked X", "this is safe because…" — those are the
very claims it is there to test.
- **One parseable final line** is the verdict (`UNIT-TESTS:`, `REVIEW:`,
`BACKEND-REVIEW:`, `FRONTEND-REVIEW:`, `GATE:`). Read that line; never infer a
pass from prose around it.
- **Never dispatch what a command answers.** Running a suite, reading a log,
grepping a path — do it yourself. An agent is for judgement, not for typing.
- Agents for the same phase go out in **one message** so they run concurrently.
## Resuming after a compaction
A long run outlives the context window. Re-derive your position, don't guess it:
- `agenthub lanes` — the lane's declared stage, status and notes.
- `git status` + `git diff --stat <base>...` — what is actually built.
- The plan file — what was actually approved.
Resume at the declared stage and re-run it from the top; every `agenthub`
command is idempotent. **A phase whose evidence you cannot find did not
happen** — re-run it rather than assuming a past self did.
## Phases
### intake — brainstorm WITH the user, always
@@ -66,6 +128,11 @@ backgrounded Agent or helper finishes. So:
"seems clear" is the single most common way this pipeline builds the wrong
thing: a requirement typed in one line always leaves scope, edge cases and
acceptance criteria unstated.
- **Ask only what the repo cannot answer.** Read first, ask second: a question
whose answer is in the code, the tests, `CLAUDE.md` or the existing UI costs
the user's time and earns you a worse answer than reading would. What belongs
to the user is intent — scope, priorities, trade-offs, what "done" means —
never facts you could have looked up.
- **Ask the user, and wait for the answer.** Put the open questions to them with
`AskUserQuestion` — batched in one round (up to 4 at a time), each with real
options, exactly the way plan mode asks. Questions that change what you build:
@@ -76,6 +143,9 @@ backgrounded Agent or helper finishes. So:
- The one exception: the user explicitly said to skip the questions and decide
yourself ("cứ làm đi", "no questions", "you decide"). Then state the
assumptions you are proceeding under, in writing, and go on.
- **Leave when:** the user's answers are in hand (or their explicit "decide
yourself" is), and you can state the scope, the non-goals and what "done"
means in your own words without a guess in any of them.
### plan — a written plan, challenged, then APPROVED by the user
- `agenthub stage plan`
@@ -99,6 +169,8 @@ backgrounded Agent or helper finishes. So:
- Approval covers the plan as presented. Work that turns out to need a
materially different approach comes BACK here (`agenthub stage plan` again)
for a fresh approval, rather than drifting under the old one.
- **Leave when:** a written plan exists, it survived the challenge, and the user
approved THIS version of it in this turn.
### implement — smallest coherent diff
- `agenthub stage implementing`
@@ -107,22 +179,36 @@ backgrounded Agent or helper finishes. So:
- **superpowers:test-driven-development**: failing test → minimal code → green.
- Smallest diff that satisfies the requirement. Every changed line traces to it.
- **file-headers** applies to every source file you create or edit.
- Work the approved plan task by task, in its order. Before leaving this phase,
re-read the plan's acceptance criteria and check each one against what you
actually built — that list, not your memory of it, is what `gate` will ask
about.
- **The plan turning out wrong is a result, not a detour.** If implementing
shows the approach cannot work, or the requirement means something else, stop
and go back to `plan` (re-declare it, present the new plan, get it approved
again). Quietly building a different thing than the one approved is the
failure this phase has to avoid.
- Long implementation? Re-declare `agenthub stage implementing` as you go — it
doubles as the heartbeat, so the dashboard doesn't read a working lane as
stalled.
- **Leave when:** every plan task is done, every acceptance criterion has
something concrete behind it, and the diff contains nothing that traces to
neither.
### tests — this repo's verification, not a claim
- `agenthub stage tests --status running`
- **Dispatch the judgement, run the commands yourself.** Only work that needs an
unbiased pair of eyes is worth an agent; a deterministic command is not.
- **Independent unit tests → a fresh-context agent** (Agent tool,
`subagent_type: general-purpose` — FOREGROUND, and **never `fork`**: a fork
inherits your context, which is the bias you are trying to escape). Hand it
only what it can verify itself — repo path, the spec/requirement, `git diff`
range, the commands — never your own conclusions. It reads the requirement
- **Independent unit tests → a fresh-context agent** (`general-purpose`; see
*Dispatching a judgement*). It reads the requirement
and the diff cold and writes the tests the implementation is *missing* —
happy, negative and boundary, in the repo's existing style. It may not touch
non-test files nor weaken existing tests. Final line:
`UNIT-TESTS: PASS — <n> added` or `UNIT-TESTS: FAIL — <test> :: <defect>`.
- **Suite run → plain Bash, here.** Backend changed → `npm run test:server`.
Frontend → `npm run test:client`. Nothing is judged, so nothing is
- **Suite run → plain Bash, here.** Run the suite(s) the diff actually needs:
backend changed → `npm run test:server`; frontend changed →
`npm run test:client`; both → both. A docs-only change needs neither, and
running the other layer's suite proves nothing about this diff. Nothing is judged, so nothing is
delegated: an agent for this buys a second model's tokens to read output you
can read for free. A UI snapshot diff is reported for review, **never
blindly regenerated** (`cd client && npx vitest run -u` is yours to run,
@@ -133,6 +219,9 @@ backgrounded Agent or helper finishes. So:
- Record the outcome: `agenthub stage tests --evidence "<what passed>"`, or
`--result fail` with what failed. A step you could not run is reported as not
run, never as passed.
- **Leave when:** the suites the diff needs are green in output you actually
read, and the independent-test agent (when the size called for one) returned
`PASS`.
### review — the native review, plus the domains this diff actually touched
- `agenthub stage review --status running`
@@ -145,16 +234,15 @@ backgrounded Agent or helper finishes. So:
three times the tokens for one opinion. Add the domain reviewer(s) once the
change is big enough to have seams: multiple files in a layer, a route or
schema or hook touched, or anything you would want a second reader on.
- **Always: Claude Code's own `code-review` skill** (the official plugin's, run
as `/code-review` on the working diff). It is not optional and is not replaced
by an agent reviewing "in its own words" — run it in a **fresh-context agent**
(`subagent_type: Code Reviewer`, FOREGROUND, never `fork`; a reviewer that
watched you write the code is not a reviewer), whose job is to invoke that
skill and report its findings. See **superpowers:requesting-code-review** for
what to hand over. Final line: `REVIEW: CLEAN` or `REVIEW: FINDINGS — <n>`.
- **Domain reviewers, conditional on the touched layers**, launched in parallel
with the above (same message, separate Agent calls) — FOREGROUND, fresh
context, never `fork`:
- **Always: Claude Code's own `code-review` skill** (`/code-review`, on the
working diff — it is also what the `review` node declares). Not optional, and
not replaced by an agent reviewing "in its own words": run it in a
fresh-context `Code Reviewer` agent whose job is to invoke that skill and
report what it found (a reviewer that watched you write the code is not a
reviewer). See **superpowers:requesting-code-review** for what to hand over.
Final line: `REVIEW: CLEAN` or `REVIEW: FINDINGS — <n>`.
- **Domain reviewers, conditional on the touched layers**, in the same message
as the above so they run concurrently:
- Any path under `server/` (routes, lib, hooks, db, CLI-facing code) →
`agenthub:backend-reviewer`. No backend path changed → do not launch it.
- Any path under `client/` (components, pages, i18n, styles) →
@@ -173,16 +261,19 @@ backgrounded Agent or helper finishes. So:
is a loop, not a checkpoint: it ends when a pass over the current diff comes
back clean (or with only findings you have explicitly and defensibly
rejected), not when one pass has been performed.
- Record it: `agenthub stage review --evidence "<n> findings across <who ran>,
resolved"`.
- **Leave when:** the latest pass over the CURRENT diff is clean or
argued-through — not when a pass has merely happened.
### gate — evidence, then the user's acceptance
- **superpowers:verification-before-completion**. Commands actually run, output
actually read. This is the phase that stops "should work" from shipping.
- For a change of any real size, the gate is a **fresh-context agent** too
(`general-purpose`, FOREGROUND, never `fork`): hand it the requirement, the
diff range and the verification commands — not your evidence — and let it
re-run and re-check them. Final line: `GATE: GO` or `GATE: NO-GO — <what is
missing>`. A one-line fix does not need it; anything you would defend in a
review does.
- At medium/large size the gate is a **fresh-context agent** too
(`general-purpose`): hand it the requirement, the diff range and the
verification commands — not your evidence — and let it re-run and re-check
them. Final line: `GATE: GO` or `GATE: NO-GO — <what is missing>`. At small
size skip it; the user's acceptance below is the gate.
- **The gate is not yours to wave through. Ask the user.** Present, in a few
lines: what was built, what was verified and how (the commands and their real
outcome), what review found and what you did about it, and anything you could
@@ -203,6 +294,8 @@ backgrounded Agent or helper finishes. So:
`ship` with a note about it.
- Only once the user has accepted: `agenthub stage gate --evidence "<what was
verified, and that the user accepted it>"`
- **Leave when:** the verification really ran, its output was really read, and
the user said yes. Two of three is a NO-GO.
### ship — docs, then the user's call
- `agenthub stage ship`
@@ -211,6 +304,8 @@ backgrounded Agent or helper finishes. So:
deferred, not "if asked".
- Commit / push / PR **only when the user asks in this turn**. Finishing an
implementation is not authorization to commit.
- **Leave when:** the docs that the change made wrong are right again, and you
have reported plainly what shipped, what was verified, and what you skipped.
- `agenthub stage done --status passed --evidence "<what shipped>"`
## This repository's own rules