feat(skill): /review-loop — supervised discover/plan/spec/tests/code/integrate/deploy loop with MiniMax review at every stage
Codifies the 2026-09-13 agent-runtimes program (item 824222cb): MiniMax verification + reviews, Sonnet spec-ahead tests, MiniMax coders in git worktrees, tests/spec untouched gate, test-merge integration, live verify. Claude-Session: https://claude.ai/code/session_018LRh1S2bHJYhEgrMu3UJx8
This commit is contained in:
Paul O'Reilly
152
skills/review-loop/SKILL.md
Normal file
152
skills/review-loop/SKILL.md
Normal file
@@ -0,0 +1,152 @@
|
||||
---
|
||||
name: review-loop
|
||||
description: >
|
||||
Supervised delivery loop for planner-backed projects: verify the work item is still
|
||||
real, then plan, spec, tests, code, integrate, deploy, with MiniMax reviewing every
|
||||
artefact against best-practices plus gotchas until no CRITICAL/HIGH, Sonnet writing
|
||||
the spec-ahead tests, MiniMax implementing in its own git worktree, and the supervisor
|
||||
gating that tests/ and spec/ are untouched before merging. Pass one or more work-item
|
||||
UUIDs (and optionally the entry stage) as arguments.
|
||||
user_invocable: true
|
||||
allowed-tools: Read, Write, Edit, Agent, SendMessage, Bash(date *), Bash(test *), Bash(ls *), Bash(cat *), Bash(mkdir *), Bash(chmod *), Bash(setsid *), Bash(pgrep *), Bash(python3 *), Bash(git status*), Bash(git rev-parse *), Bash(git log *), Bash(git diff *), Bash(git add *), Bash(git commit *), Bash(git pull *), Bash(git push), Bash(git merge *), Bash(git worktree *), Bash(git checkout main -- *), Bash(git -C *), Bash(mm-worktree-task *), Bash(ask-minimax-run *), Bash(review-minimax-run *), Bash(gac *), Bash(kubectl *), Bash(ssh-add *)
|
||||
---
|
||||
|
||||
<command-name>review-loop</command-name>
|
||||
|
||||
# /review-loop Skill
|
||||
|
||||
Drive a planner work item (or a small batch) from "is this still a real issue?"
|
||||
to "deployed and verified live" with an independent MiniMax review of every
|
||||
artefact. The loop is what shipped WIA-17c..f and PDIV on 2026-09-13 (program
|
||||
item `824222cb` in agent-runtimes holds every round as a worked example).
|
||||
|
||||
## Pre-gathered context
|
||||
|
||||
### Current date
|
||||
!`date +%Y-%m-%d`
|
||||
|
||||
### Tooling (check via tool calls before step 0)
|
||||
Run these with the Bash tool and stop if any is MISSING:
|
||||
`test -f "$HOME/.claude-oreillyit-minimax/provider.env"` (MiniMax profile),
|
||||
`test -x "$HOME/sbin/mm-worktree-task"`, and
|
||||
`test -x "$HOME/dev/claude/projects/small-scripts/scripts/review-minimax-run"`.
|
||||
|
||||
### Planner + repo
|
||||
!`git rev-parse --abbrev-ref HEAD`
|
||||
!`git status --short | grep -v uv.lock | head -5`
|
||||
|
||||
## Arguments
|
||||
|
||||
`$ARGUMENTS` = one or more work-item UUIDs, optionally followed by `--from <stage>`
|
||||
(`discover|plan|spec|tests|code|integrate|deploy`). Without `--from`, the skill
|
||||
runs discovery first and decides the entry stage from what exists.
|
||||
|
||||
## Roles
|
||||
|
||||
| Role | Who | Never does |
|
||||
|---|---|---|
|
||||
| Supervisor | this session | write tests or code MiniMax should write; skip a gate |
|
||||
| Verifier / reviewer | MiniMax M3 via `ask-minimax-run` (detached) | modify repo files |
|
||||
| Test author | Sonnet subagent (`Agent`, model sonnet) | touch spec/, controlplane/, lib/ |
|
||||
| Coder | MiniMax via `mm-worktree-task` in its own git worktree | touch tests/ or spec/ |
|
||||
|
||||
Each work item gets its own worktree and branch (`mm-<slug>` / `mm/<slug>`);
|
||||
nothing is developed on main. (The tooling uses git *worktrees*; a git subtree
|
||||
is a different mechanism and is not used.)
|
||||
|
||||
## The loop
|
||||
|
||||
Every artefact goes: draft → MiniMax review → adjudicate (accept / reject with a
|
||||
source-cited reason) → apply → re-review, until a round has zero CRITICAL and
|
||||
zero HIGH. Cap 4 rounds; record residue on the item and move on. Reject up front
|
||||
the "symbol/module does not exist yet" class for spec-ahead artefacts, or it
|
||||
dominates every round.
|
||||
|
||||
### 0. Discover (per UUID)
|
||||
1. `python3 -m lib.cp_cli planner items get <uuid>` + `documents list/get` into the scratchpad.
|
||||
2. MiniMax verification prompt: "is the described defect still present in the
|
||||
code on main? cite file:line or commit; verdict RESOLVED / STILL OPEN /
|
||||
PARTIAL". Spot-check one citation with git yourself.
|
||||
3. RESOLVED → `planner items patch <uuid> --state completed --description "<evidence>"`.
|
||||
PARTIAL / OPEN → attach the verdict as `verification-<date>.md` and continue.
|
||||
|
||||
### 1. Plan (only for items that need a decision)
|
||||
Write the plan to the scratchpad; upload as `plan-v1.md` on the item; MiniMax
|
||||
review with spec-driven-development, test-driven-development, security-
|
||||
architecture, api-design, validation, kubernetes + the project's
|
||||
`memory/gotchas-*.md`. The plan MUST carry an ID-allocation table (every new
|
||||
requirement ID checked against the spec files for collisions) and a
|
||||
decision record `{options, choice, rationale, rejected, follow_ups}`.
|
||||
|
||||
### 2. Spec
|
||||
Amend the owning `spec/*.md` in place (amend, don't add, when reversing prior
|
||||
text; add `Revokes prior text:` notes). Every requirement gets: normative
|
||||
statement, **Why**, **Test handles** with exact test names and file, and a
|
||||
module-layout row for every new/changed file. Pin cross-module seams to exact
|
||||
signatures and the real attribute names — read the source first; the reviewer
|
||||
will catch invented names but only after a wasted round.
|
||||
Review prompt inputs: the `git diff spec/...` file, mechanical-test-generation,
|
||||
spec-driven-development, api-design, security-architecture, gotchas-specs,
|
||||
gotchas-python. Commit `spec: <IDs> (<item uuids>)` when clean.
|
||||
|
||||
### 3. Tests (Sonnet)
|
||||
One subagent per independent area. Brief: read the spec block + every cited
|
||||
source file; create exactly the named handles in the named files; every new test
|
||||
`@pytest.mark.xfail(strict=True, reason="<ID> not implemented")`; tests that
|
||||
already pass today stay unmarked and are reported; access not-yet-existing
|
||||
symbols inside the test body (never `from x import New` at top, never
|
||||
`importorskip`); must not modify spec/ or source; run the files and the
|
||||
`--co` of the whole suite. Gate: 0 xpassed, 0 errors. Then MiniMax reviews the
|
||||
test files against the spec handles + test-driven-development +
|
||||
mechanical-test-generation + gotchas-python (mock-target divergence,
|
||||
`object.__new__` fixtures that skip `__init__`, `AsyncMock` on sync calls,
|
||||
assertions that pass today for the wrong reason). Send accepted findings back to
|
||||
the same Sonnet agent with `SendMessage`. Commit `test: … — xfail strict`.
|
||||
|
||||
### 4. Code (MiniMax in a worktree)
|
||||
Create a child task item under the program item; attach `brief.md` (spec IDs,
|
||||
the exact test files, the hard rules). Launch detached (max 2 concurrent — the
|
||||
host runs CI builders):
|
||||
```
|
||||
mm-worktree-task <uuid> --docs all --timeout 3600 \
|
||||
--focus "Make the xfail(strict) tests in brief.md pass; never modify tests/ or spec/; run the named files with --runxfail and the whole test directory before reporting"
|
||||
```
|
||||
Collect with `mm-worktree-task <uuid> --collect`.
|
||||
**Gates, in order:**
|
||||
1. `git diff main...mm/<slug> -- tests/ spec/` is EMPTY. If not: `git checkout main -- tests/ spec/` in the worktree, note the violation, re-run.
|
||||
2. In the worktree: target files with `--runxfail` all pass; the whole test directory has no new failures (strict XPASS of the new tests is the expected signal).
|
||||
3. A test that cannot pass is a test or spec bug → fix it on main via the Sonnet agent, commit, `git -C <worktree> merge main`.
|
||||
4. MiniMax code review of `git diff main...<branch>` (correctness vs spec text, security, gotchas-python, wire-shape changes, no test edits). Small accepted fixes: apply yourself in the worktree and commit; large ones: re-run the coder with a `review-r1-fixes.md` brief.
|
||||
|
||||
### 5. Integrate
|
||||
```
|
||||
git worktree add -f /tmp/claude-1000/test-merge -b test-merge main
|
||||
git -C /tmp/claude-1000/test-merge merge --no-edit mm/<slug> # each branch
|
||||
```
|
||||
Strip the xfail markers there (per-test decorators, `_XFAIL` constants,
|
||||
module-level `pytestmark`, `pytest.param(..., marks=...)` rows), run the FULL
|
||||
suite, fast-forward main (`git merge --ff-only test-merge`), push, remove the
|
||||
worktree. Regenerate `openapi/` + SDKs only if a route/schema changed (the local
|
||||
generator produces version-noise churn otherwise — revert it).
|
||||
|
||||
### 6. Deploy and verify live
|
||||
Empty commit `[CI BUILD] [DEPLOY] deploy: …`; watch with
|
||||
`GITEA_TOKEN_FILE=~/dev/claude/secrets/gitea/ai_admin/token gac tail <owner/repo> --until run-complete`
|
||||
(detached; a later push cancels an in-flight run — re-trigger). Then
|
||||
`kubectl rollout status`, confirm the image tag, and **reproduce the original
|
||||
bug's exact user-visible action** plus a smoke of unrelated reads. Only then
|
||||
`planner items patch <uuid> --state completed --description "<evidence>"`.
|
||||
If reads look wrong: roll back first (deploy-repo `newTag` + ArgoCD hard
|
||||
refresh), diagnose second, file an incident item with an RCA.
|
||||
|
||||
## Bookkeeping
|
||||
- Every review result → `documents put <program-item> <artefact>-review-r<N>.md`.
|
||||
- Follow-ups discovered in review → new `future`/`task` items under the program item, never left in prose.
|
||||
- End: `/log`, CONTEXT.md cache, commit + push both the project and any secondary repo touched (e.g. `agent-runtime-framework`).
|
||||
|
||||
## Hard rules and gotchas
|
||||
- Launch every MiniMax run through a `setsid nohup <script>` launcher; never `pkill -f` on the model name (it kills the invoking shell). Build prompt files with Python, not `sed` (scratchpad paths contain `/`).
|
||||
- Memoising helpers need an ordering test that drives the real lifespan — unit tests with a bare `app_state` cannot see `create_app()` vs lifespan ordering (incident aa98d9fe).
|
||||
- "Verified live" means the original repro, not just a 200 on an unrelated route.
|
||||
- Never `cd` into a worktree in a way that moves the session cwd; use `git -C` or `(cd … && …)`.
|
||||
- The supervisor owns marker removal, merges, and pushes; agents never commit on main.
|
||||
Reference in New Issue
Block a user