mirror of
https://github.com/obra/superpowers.git
synced 2026-07-17 23:44:04 +08:00
Compare commits
12 Commits
sdd-fix-lo
...
skill-detr
| Author | SHA1 | Date | |
|---|---|---|---|
|
|
67714e036c | ||
|
|
75ef64f3df | ||
|
|
0aff5d6c64 | ||
|
|
12960fbaf7 | ||
|
|
10f4868e4d | ||
|
|
2e039d0229 | ||
|
|
3adac24a6c | ||
|
|
232993b395 | ||
|
|
dee15bcb9e | ||
|
|
447edeea29 | ||
|
|
880b9b84d5 | ||
|
|
37d9a63d45 |
File diff suppressed because it is too large
Load Diff
@@ -1,196 +0,0 @@
|
|||||||
# SDD Fix-Loop Redesign — Design Spec
|
|
||||||
|
|
||||||
**Status:** Approved design (brainstormed with Jesse 2026-07-15); implementation
|
|
||||||
plan to follow.
|
|
||||||
**Objective:** make the subagent-driven-development skill's review-fix loop
|
|
||||||
convergent and autonomous, and make the document readable, without rewriting
|
|
||||||
its eval-tuned language.
|
|
||||||
**Hard invariant:** existing eval-tuned sentences move; they do not get
|
|
||||||
reworded. New machinery ships with drill evidence.
|
|
||||||
|
|
||||||
## Problems
|
|
||||||
|
|
||||||
Four, all observed in real sessions:
|
|
||||||
|
|
||||||
1. **Pathological review loops.** The loop is literally "Repeat until
|
|
||||||
approved" — no round cap. Each re-review is a fresh full review of the
|
|
||||||
whole diff, so a nondeterministic frontier reviewer surfaces new findings
|
|
||||||
every round instead of verifying fixes. Result: implement, review, fix,
|
|
||||||
review, review, fix, review, fix — with no circuit breaker. The
|
|
||||||
strict-cost spec (2026-06-10) independently measured review-loop count as
|
|
||||||
the biggest run-to-run cost variance.
|
|
||||||
2. **Contradictory fix policy.** The process diagram and "Constructing
|
|
||||||
Reviewer Prompts" dispatch dedicated fix subagents; Red Flags says
|
|
||||||
"Implementer (same subagent) fixes them"; implementer-prompt.md's "After
|
|
||||||
Review Findings" section assumes the implementer will be re-engaged. Three
|
|
||||||
answers to "who fixes?" in one skill.
|
|
||||||
3. **Accreted structure.** Thirteen top-level sections; guidance for one
|
|
||||||
activity is scattered across four of them. "Constructing Reviewer Prompts"
|
|
||||||
is a grab-bag holding reviewer guidance, fix policy, final-review policy,
|
|
||||||
and plan-conflict adjudication.
|
|
||||||
4. **Red Flags format.** Seven sibling skills use the `| Excuse | Reality |`
|
|
||||||
rationalization table; SDD carries a 17-bullet "Never" list plus three
|
|
||||||
"If X" mini-blocks.
|
|
||||||
|
|
||||||
## Design Decisions
|
|
||||||
|
|
||||||
| # | Decision | Rationale |
|
|
||||||
|---|----------|-----------|
|
|
||||||
| 1 | The original implementer fixes its own review findings — resume it in place. | It already holds the task context; ownership beats a drive-by patcher. Fresh "fix subagents" rebuild context per finding and lack the task frame. |
|
|
||||||
| 2 | Re-reviews are scoped to the findings. | Fresh full reviews each round are the churn engine. Scoped re-reviews make the loop structurally convergent; the final whole-branch review remains the broad safety net. |
|
|
||||||
| 3 | Circuit breaker at five fix rounds: three resumes, then two fresh dispatches on a more capable model. | Jesse's call. A loop that survives three resumes usually means the implementer cannot see its own problem — the fresh capable dispatch de-anchors and capability-bumps in one move. |
|
|
||||||
| 4 | At trip, the controller adjudicates and routes. No new human checkpoint — structural failures reach the existing BLOCKED stop. | SDD's point is autonomous execution. The controller holds the plan and cross-task context the reviewer lacks; the existing text already sanctions it ("adjudicate it in the review loop") without ever specifying the mechanism. |
|
|
||||||
| 5 | Reorganize SKILL.md by lifecycle, preserving tuned sentences. | Fixes "hard to follow" at the root. Content moves to its point of use, matching the house direction (recent commits fold recap sections into points of use). |
|
|
||||||
| 6 | Convert Red Flags to a `| Excuse | Reality |` rationalization table; relocate hard rules to their points of use. | Matches the other seven skills. Excuses get rebuttals; rules get enforced where the reader acts. |
|
|
||||||
|
|
||||||
## The Fix Loop
|
|
||||||
|
|
||||||
Trigger: a task review returns spec ❌ or any Critical/Important finding.
|
|
||||||
|
|
||||||
**Rounds 1–3 — resume the original implementer.** Send the findings verbatim
|
|
||||||
(Critical/Important plus spec gaps). The implementer fixes, re-runs the
|
|
||||||
covering tests, appends the fix report to its existing report file, and
|
|
||||||
returns the short contract. On a harness without agent resume, a "resume" is
|
|
||||||
a fresh dispatch carrying the brief, the report file, and the findings — the
|
|
||||||
report file is the persistent memory either way.
|
|
||||||
|
|
||||||
**Rounds 4–5 — fresh implementer, more capable model.** Full task context:
|
|
||||||
brief, report file, open findings, and the framing "a prior implementer
|
|
||||||
attempted this N times; you own the task now."
|
|
||||||
|
|
||||||
**Every round's re-review is scoped.** The re-reviewer receives the brief,
|
|
||||||
the updated report, the original findings list, and a fix-scoped diff package
|
|
||||||
(`review-package FIX_BASE HEAD`, where FIX_BASE is the head the reviewer
|
|
||||||
last reviewed; the script already takes arbitrary ranges).
|
|
||||||
It verdicts each finding addressed / not addressed and flags new breakage in
|
|
||||||
the fix diff only. Novel findings on code the fix did not touch are reported
|
|
||||||
as non-blocking; the controller ledgers them for the final review.
|
|
||||||
|
|
||||||
**Fix-report completeness gate (existing rule, kept):** before dispatching a
|
|
||||||
re-review, confirm the fix report names the covering tests, the command run,
|
|
||||||
and the output.
|
|
||||||
|
|
||||||
**No early exit.** The controller never adjudicates before the cap — an early
|
|
||||||
exit reopens the "pre-judge findings to spare yourself a review loop" hole
|
|
||||||
the current content deliberately closed. One exception, unchanged from
|
|
||||||
today: a finding that conflicts with what the plan's text mandates goes to
|
|
||||||
the human immediately (plan authority, not loop churn).
|
|
||||||
|
|
||||||
**Minor findings** never enter the loop: ledger them as they arrive (existing
|
|
||||||
rule, kept).
|
|
||||||
|
|
||||||
### Adjudication at Trip
|
|
||||||
|
|
||||||
After round five fails, the controller stops dispatching and judges each open
|
|
||||||
finding against the brief, the plan, and cross-task context:
|
|
||||||
|
|
||||||
- **Contested or wrong** → ledger with a one-line adjudication ("controller:
|
|
||||||
reviewer wrong because X"), continue. The final review sees both sides.
|
|
||||||
- **Real, not load-bearing** → ledger as known-open, continue. Later
|
|
||||||
dispatches touching that area carry a pointer to the entry.
|
|
||||||
- **Real and load-bearing** (later tasks build on it, or it reveals a plan
|
|
||||||
defect) → the existing BLOCKED stop. Park-and-continue defers a structural
|
|
||||||
failure to the most expensive point and lets dependents build on it, so
|
|
||||||
structural failures stop the run — through the stop condition that already
|
|
||||||
exists, not a new checkpoint.
|
|
||||||
|
|
||||||
Every adjudication is a ledger entry. Silent discards stay forbidden.
|
|
||||||
|
|
||||||
## Document Restructure
|
|
||||||
|
|
||||||
New skeleton, in execution order:
|
|
||||||
|
|
||||||
1. Intro — why subagents, core principle, narration, continuous execution
|
|
||||||
2. When to Use — unchanged, including the decision graph
|
|
||||||
3. The Process — diagram updated for the new loop
|
|
||||||
4. Setup — worktree, ledger check/resume, pre-flight plan review, todos
|
|
||||||
5. Model Selection — stays one cross-cutting section; every dispatch
|
|
||||||
consults it, so folding it into points of use would repeat it five times
|
|
||||||
6. The Task Loop — five numbered steps:
|
|
||||||
1. Dispatch the implementer (task-brief script, five-part dispatch
|
|
||||||
composition, model line required)
|
|
||||||
2. Handle the report (DONE / DONE_WITH_CONCERNS / NEEDS_CONTEXT / BLOCKED)
|
|
||||||
3. Review the task (review-package script, reviewer dispatch composition,
|
|
||||||
constraints lens, no pre-judging, ⚠️ handling)
|
|
||||||
4. Fix loop (the machinery above)
|
|
||||||
5. Complete the task (ledger append, todo update)
|
|
||||||
7. Final Review — package, model pin, one fix wave, one scoped re-review,
|
|
||||||
adjudication
|
|
||||||
8. Finish — finishing-a-development-branch
|
|
||||||
9. Common Rationalizations — the table
|
|
||||||
10. Example Workflow — updated to show a resume-based fix round and the
|
|
||||||
breaker not tripping
|
|
||||||
|
|
||||||
"Constructing Reviewer Prompts," "File Handoffs," and "Durable Progress"
|
|
||||||
dissolve into the steps where each rule applies. Every eval-tuned sentence
|
|
||||||
lands in exactly one new location; a move map in the implementation plan
|
|
||||||
tracks source → destination so review can verify nothing was dropped or
|
|
||||||
reworded.
|
|
||||||
|
|
||||||
## Rationalization Table
|
|
||||||
|
|
||||||
Excuse-shaped Never items convert to rows; new rows cover the loop
|
|
||||||
pathology. Draft rows (final wording at implementation):
|
|
||||||
|
|
||||||
| Excuse | Reality |
|
|
||||||
|--------|---------|
|
|
||||||
| "Close enough on spec compliance" | Reviewer found gaps = not done. |
|
|
||||||
| "I'll fix it myself, dispatching is overhead" | Controller fixes pollute your context and skip review. Resume the implementer. |
|
|
||||||
| "One more round will converge" | Past the cap, rounds don't converge. Adjudicate. |
|
|
||||||
| "The reviewer will just find something new anyway" | Scoped re-reviews check fixes, not taste. New findings on untouched code go to the ledger, not the loop. |
|
|
||||||
| "This finding is obviously wrong, I'll drop it" | You adjudicate only at the cap, and every adjudication is a ledger entry. Silent discards are forbidden. |
|
|
||||||
| "The fix was small, skip the re-review" | Unreviewed fixes are how regressions land. |
|
|
||||||
|
|
||||||
Hard rules that are not excuses (never parallel implementers, never dispatch
|
|
||||||
a reviewer without a diff file, model line required, never re-dispatch
|
|
||||||
ledger-complete tasks) move to their points of use.
|
|
||||||
|
|
||||||
## Prompt Templates
|
|
||||||
|
|
||||||
- **implementer-prompt.md** — "After Review Findings" rewritten for resume
|
|
||||||
semantics: you will be resumed with findings; fix, re-run covering tests,
|
|
||||||
append to your report file, return the short contract.
|
|
||||||
- **task-reviewer-prompt.md** — initial review only; the trailing re-review
|
|
||||||
sentence moves out.
|
|
||||||
- **re-review-prompt.md (new)** — the scoped re-review contract: inputs are
|
|
||||||
brief, updated report, original findings, fix-scoped diff package; output
|
|
||||||
is a per-finding verdict (addressed / not addressed), new breakage in the
|
|
||||||
fix diff, and non-blocking observations outside it. A separate template
|
|
||||||
because it is a different contract — overloading the full-review template
|
|
||||||
produced the current ambiguity.
|
|
||||||
- **Takeover dispatch (rounds 4–5)** — composed from implementer-prompt.md
|
|
||||||
plus SKILL.md guidance (brief, report path, open findings, takeover
|
|
||||||
framing); no new template file.
|
|
||||||
|
|
||||||
## Final Review Loop
|
|
||||||
|
|
||||||
Unchanged: merge-base package, most capable model, ONE fixer with the
|
|
||||||
complete findings list. New: exactly one scoped re-review of the fix wave,
|
|
||||||
then controller adjudication. Residual load-bearing findings surface at
|
|
||||||
finishing-a-development-branch, where the human already is. The end of the
|
|
||||||
branch gets a bounded loop too.
|
|
||||||
|
|
||||||
## Evals
|
|
||||||
|
|
||||||
Three new drill scenarios in `evals/`:
|
|
||||||
|
|
||||||
1. **Resume, don't re-dispatch:** a task review returns findings; the
|
|
||||||
controller must resume the same implementer rather than dispatch a fix
|
|
||||||
subagent.
|
|
||||||
2. **Breaker trips:** a seeded never-satisfied reviewer; the controller must
|
|
||||||
stop dispatching after the fifth round fails, adjudicate, ledger, and
|
|
||||||
continue — not loop.
|
|
||||||
3. **Structural finding stops:** a load-bearing finding (later tasks depend
|
|
||||||
on it); the controller must stop via BLOCKED rather than park.
|
|
||||||
|
|
||||||
Plus before/after runs of the existing SDD scenarios to catch regressions
|
|
||||||
from the reorganization.
|
|
||||||
|
|
||||||
## Non-Goals
|
|
||||||
|
|
||||||
- Ledger session-scoping — PR #1943 owns it. This work touches the same
|
|
||||||
sections, so the implementation plan notes the collision risk.
|
|
||||||
- Script changes — task-brief and review-package already do what the new
|
|
||||||
loop needs.
|
|
||||||
- Changes to executing-plans or requesting-code-review beyond the final-
|
|
||||||
review pointer continuing to resolve.
|
|
||||||
@@ -51,85 +51,41 @@ digraph process {
|
|||||||
subgraph cluster_per_task {
|
subgraph cluster_per_task {
|
||||||
label="Per Task";
|
label="Per Task";
|
||||||
"Dispatch implementer subagent (./implementer-prompt.md)" [shape=box];
|
"Dispatch implementer subagent (./implementer-prompt.md)" [shape=box];
|
||||||
"Implementer asks questions?" [shape=diamond];
|
"Implementer subagent asks questions?" [shape=diamond];
|
||||||
"Answer questions, provide context" [shape=box];
|
"Answer questions, provide context" [shape=box];
|
||||||
"Implementer implements, tests, commits, self-reviews" [shape=box];
|
"Implementer subagent implements, tests, commits, self-reviews" [shape=box];
|
||||||
"Generate review package, dispatch task reviewer (./task-reviewer-prompt.md)" [shape=box];
|
"Write diff file, dispatch task reviewer subagent (./task-reviewer-prompt.md)" [shape=box];
|
||||||
"Spec ✅ and quality approved?" [shape=diamond];
|
"Task reviewer reports spec ✅ and quality approved?" [shape=diamond];
|
||||||
"Finding conflicts with plan text?" [shape=diamond];
|
"Dispatch fix subagent for Critical/Important findings" [shape=box];
|
||||||
"Ask human partner which governs" [shape=box];
|
"Mark task complete in todo list and progress ledger" [shape=box];
|
||||||
"Fix round R of 5: R≤3 resume implementer; R≥4 fresh implementer, more capable model" [shape=box];
|
|
||||||
"Dispatch scoped re-review (./re-review-prompt.md)" [shape=box];
|
|
||||||
"All findings addressed?" [shape=diamond];
|
|
||||||
"R = 5?" [shape=diamond];
|
|
||||||
"Adjudicate each open finding" [shape=box];
|
|
||||||
"Any load-bearing finding?" [shape=diamond];
|
|
||||||
"STOP: report BLOCKED to human partner" [shape=box];
|
|
||||||
"Park findings in ledger with rulings" [shape=box];
|
|
||||||
"Append completion to ledger, mark todo complete" [shape=box];
|
|
||||||
}
|
}
|
||||||
|
|
||||||
"Setup: worktree, ledger check, read plan, pre-flight review" [shape=box];
|
"Read plan, note context and global constraints, create todos" [shape=box];
|
||||||
"More tasks remain?" [shape=diamond];
|
"More tasks remain?" [shape=diamond];
|
||||||
"Dispatch final code reviewer (../requesting-code-review/code-reviewer.md)" [shape=box];
|
"Dispatch final code reviewer subagent (../requesting-code-review/code-reviewer.md)" [shape=box];
|
||||||
"Final findings? ONE fix dispatch, one scoped re-review, adjudicate residuals" [shape=box];
|
|
||||||
"Use superpowers:finishing-a-development-branch" [shape=box style=filled fillcolor=lightgreen];
|
"Use superpowers:finishing-a-development-branch" [shape=box style=filled fillcolor=lightgreen];
|
||||||
|
|
||||||
"Setup: worktree, ledger check, read plan, pre-flight review" -> "Dispatch implementer subagent (./implementer-prompt.md)";
|
"Read plan, note context and global constraints, create todos" -> "Dispatch implementer subagent (./implementer-prompt.md)";
|
||||||
"Dispatch implementer subagent (./implementer-prompt.md)" -> "Implementer asks questions?";
|
"Dispatch implementer subagent (./implementer-prompt.md)" -> "Implementer subagent asks questions?";
|
||||||
"Implementer asks questions?" -> "Answer questions, provide context" [label="yes"];
|
"Implementer subagent asks questions?" -> "Answer questions, provide context" [label="yes"];
|
||||||
"Answer questions, provide context" -> "Implementer implements, tests, commits, self-reviews";
|
"Answer questions, provide context" -> "Dispatch implementer subagent (./implementer-prompt.md)";
|
||||||
"Implementer asks questions?" -> "Implementer implements, tests, commits, self-reviews" [label="no"];
|
"Implementer subagent asks questions?" -> "Implementer subagent implements, tests, commits, self-reviews" [label="no"];
|
||||||
"Implementer implements, tests, commits, self-reviews" -> "Generate review package, dispatch task reviewer (./task-reviewer-prompt.md)";
|
"Implementer subagent implements, tests, commits, self-reviews" -> "Write diff file, dispatch task reviewer subagent (./task-reviewer-prompt.md)";
|
||||||
"Generate review package, dispatch task reviewer (./task-reviewer-prompt.md)" -> "Spec ✅ and quality approved?";
|
"Write diff file, dispatch task reviewer subagent (./task-reviewer-prompt.md)" -> "Task reviewer reports spec ✅ and quality approved?";
|
||||||
"Spec ✅ and quality approved?" -> "Append completion to ledger, mark todo complete" [label="yes"];
|
"Task reviewer reports spec ✅ and quality approved?" -> "Dispatch fix subagent for Critical/Important findings" [label="no"];
|
||||||
"Spec ✅ and quality approved?" -> "Finding conflicts with plan text?" [label="no"];
|
"Dispatch fix subagent for Critical/Important findings" -> "Write diff file, dispatch task reviewer subagent (./task-reviewer-prompt.md)" [label="re-review"];
|
||||||
"Finding conflicts with plan text?" -> "Ask human partner which governs" [label="yes"];
|
"Task reviewer reports spec ✅ and quality approved?" -> "Mark task complete in todo list and progress ledger" [label="yes"];
|
||||||
"Ask human partner which governs" -> "Fix round R of 5: R≤3 resume implementer; R≥4 fresh implementer, more capable model";
|
"Mark task complete in todo list and progress ledger" -> "More tasks remain?";
|
||||||
"Finding conflicts with plan text?" -> "Fix round R of 5: R≤3 resume implementer; R≥4 fresh implementer, more capable model" [label="no"];
|
|
||||||
"Fix round R of 5: R≤3 resume implementer; R≥4 fresh implementer, more capable model" -> "Dispatch scoped re-review (./re-review-prompt.md)";
|
|
||||||
"Dispatch scoped re-review (./re-review-prompt.md)" -> "All findings addressed?";
|
|
||||||
"All findings addressed?" -> "Append completion to ledger, mark todo complete" [label="yes"];
|
|
||||||
"All findings addressed?" -> "R = 5?" [label="no"];
|
|
||||||
"R = 5?" -> "Fix round R of 5: R≤3 resume implementer; R≥4 fresh implementer, more capable model" [label="no - next round"];
|
|
||||||
"R = 5?" -> "Adjudicate each open finding" [label="yes - breaker trips"];
|
|
||||||
"Adjudicate each open finding" -> "Any load-bearing finding?";
|
|
||||||
"Any load-bearing finding?" -> "STOP: report BLOCKED to human partner" [label="yes"];
|
|
||||||
"Any load-bearing finding?" -> "Park findings in ledger with rulings" [label="no"];
|
|
||||||
"Park findings in ledger with rulings" -> "Append completion to ledger, mark todo complete";
|
|
||||||
"Append completion to ledger, mark todo complete" -> "More tasks remain?";
|
|
||||||
"More tasks remain?" -> "Dispatch implementer subagent (./implementer-prompt.md)" [label="yes"];
|
"More tasks remain?" -> "Dispatch implementer subagent (./implementer-prompt.md)" [label="yes"];
|
||||||
"More tasks remain?" -> "Dispatch final code reviewer (../requesting-code-review/code-reviewer.md)" [label="no"];
|
"More tasks remain?" -> "Dispatch final code reviewer subagent (../requesting-code-review/code-reviewer.md)" [label="no"];
|
||||||
"Dispatch final code reviewer (../requesting-code-review/code-reviewer.md)" -> "Final findings? ONE fix dispatch, one scoped re-review, adjudicate residuals";
|
"Dispatch final code reviewer subagent (../requesting-code-review/code-reviewer.md)" -> "Use superpowers:finishing-a-development-branch";
|
||||||
"Final findings? ONE fix dispatch, one scoped re-review, adjudicate residuals" -> "Use superpowers:finishing-a-development-branch";
|
|
||||||
}
|
}
|
||||||
```
|
```
|
||||||
|
|
||||||
## Setup
|
## Pre-Flight Plan Review
|
||||||
|
|
||||||
Ensure the work happens in an isolated workspace: use
|
Ensure the work happens in an isolated workspace: use
|
||||||
superpowers:using-git-worktrees to create one or verify the existing one.
|
superpowers:using-git-worktrees to create one or verify the existing one.
|
||||||
Never start implementation on a main/master branch without your human
|
|
||||||
partner's explicit consent.
|
|
||||||
|
|
||||||
Conversation memory does not survive compaction. In real sessions,
|
|
||||||
controllers that lost their place have re-dispatched entire completed task
|
|
||||||
sequences — the single most expensive failure observed. Track progress in
|
|
||||||
a ledger file, not only in todos.
|
|
||||||
|
|
||||||
- At skill start, check for a ledger:
|
|
||||||
`cat "$(git rev-parse --show-toplevel)/.superpowers/sdd/progress.md"`. Tasks with
|
|
||||||
a `Task <N>: complete` line are DONE — do not re-dispatch them; resume at
|
|
||||||
the first task without one. A task whose last line is a fix round is
|
|
||||||
mid-loop: resume the loop at the next round.
|
|
||||||
- The ledger is your recovery map: the commits it names exist in git even
|
|
||||||
when your context no longer remembers creating them. After compaction,
|
|
||||||
trust the ledger and `git log` over your own recollection.
|
|
||||||
- `git clean -fdx` will destroy the ledger (it's git-ignored scratch); if
|
|
||||||
that happens, recover from `git log`.
|
|
||||||
|
|
||||||
Read the plan once, note its context and Global Constraints, and create a
|
|
||||||
todo per task.
|
|
||||||
|
|
||||||
Before dispatching Task 1, scan the plan once for conflicts:
|
Before dispatching Task 1, scan the plan once for conflicts:
|
||||||
|
|
||||||
@@ -157,11 +113,7 @@ capable available model, not the session default.
|
|||||||
|
|
||||||
**Review tasks**: choose the model with the same judgment, scaled to the
|
**Review tasks**: choose the model with the same judgment, scaled to the
|
||||||
diff's size, complexity, and risk. A small mechanical diff does not need the
|
diff's size, complexity, and risk. A small mechanical diff does not need the
|
||||||
most capable model; a subtle concurrency change does. Scoped re-reviews of
|
most capable model; a subtle concurrency change does.
|
||||||
small fix diffs take a cheap-to-mid tier.
|
|
||||||
|
|
||||||
**Fix-loop escalation (rounds 4-5)**: use a model at least one tier above
|
|
||||||
the implementer that got stuck.
|
|
||||||
|
|
||||||
**Always specify the model explicitly when dispatching a subagent.** An
|
**Always specify the model explicitly when dispatching a subagent.** An
|
||||||
omitted model inherits your session's model — often the most capable and
|
omitted model inherits your session's model — often the most capable and
|
||||||
@@ -180,47 +132,7 @@ that implementer. Single-file mechanical fixes also take the cheapest tier.
|
|||||||
- Touches multiple files with integration concerns → standard model
|
- Touches multiple files with integration concerns → standard model
|
||||||
- Requires design judgment or broad codebase understanding → most capable model
|
- Requires design judgment or broad codebase understanding → most capable model
|
||||||
|
|
||||||
## The Task Loop
|
## Handling Implementer Status
|
||||||
|
|
||||||
Everything you paste into a dispatch prompt — and everything a subagent
|
|
||||||
prints back — stays resident in your context for the rest of the session
|
|
||||||
and is re-read on every later turn. Hand artifacts over as files.
|
|
||||||
|
|
||||||
### 1. Dispatch the implementer
|
|
||||||
|
|
||||||
Record BASE (`git rev-parse HEAD`) before dispatching — the review package
|
|
||||||
and fix-round diffs need it.
|
|
||||||
|
|
||||||
- **Task brief:** before dispatching an implementer, run this skill's
|
|
||||||
`scripts/task-brief PLAN_FILE N` — it extracts the task's full text to a
|
|
||||||
uniquely named file and prints the path. Compose the dispatch so the
|
|
||||||
brief stays the single source of
|
|
||||||
requirements. Your dispatch should contain: (1) one line on where this
|
|
||||||
task fits in the project; (2) the brief path, introduced as "read this
|
|
||||||
first — it is your requirements, with the exact values to use verbatim";
|
|
||||||
(3) interfaces and decisions from earlier tasks that the brief cannot
|
|
||||||
know; (4) your resolution of any ambiguity you noticed in the brief;
|
|
||||||
(5) the report-file path and report contract. Exact values (numbers,
|
|
||||||
magic strings, signatures, test cases) appear only in the brief. Never
|
|
||||||
make a subagent read the whole plan file.
|
|
||||||
- **Report file:** name the implementer's report file after the brief
|
|
||||||
(brief `…/task-N-brief.md` → report `…/task-N-report.md`) and put it in
|
|
||||||
the dispatch prompt. The implementer writes the full report there and
|
|
||||||
returns only status, commits, a one-line test summary, and concerns.
|
|
||||||
- A dispatch prompt describes one task, not the session's history. Do not
|
|
||||||
paste accumulated prior-task summaries ("state after Tasks 1-3") into
|
|
||||||
later dispatches — a real session's dispatch hit 42k chars of which 99%
|
|
||||||
was pasted history. A fresh subagent needs its task, the interfaces it
|
|
||||||
touches, and the global constraints. Nothing else.
|
|
||||||
- If an earlier task parked a finding in the area this task touches, carry
|
|
||||||
a pointer to that ledger entry in the dispatch.
|
|
||||||
- Record the implementer's agent identity from the dispatch result —
|
|
||||||
fix-loop rounds 1-3 resume this agent.
|
|
||||||
- Never dispatch multiple implementation subagents in parallel (conflicts).
|
|
||||||
|
|
||||||
Template: [implementer-prompt.md](implementer-prompt.md)
|
|
||||||
|
|
||||||
### 2. Handle the report
|
|
||||||
|
|
||||||
Implementer subagents report one of four statuses. Handle each appropriately:
|
Implementer subagents report one of four statuses. Handle each appropriately:
|
||||||
|
|
||||||
@@ -238,37 +150,20 @@ Implementer subagents report one of four statuses. Handle each appropriately:
|
|||||||
|
|
||||||
**Never** ignore an escalation or force the same model to retry without changes. If the implementer said it's stuck, something needs to change.
|
**Never** ignore an escalation or force the same model to retry without changes. If the implementer said it's stuck, something needs to change.
|
||||||
|
|
||||||
If the implementer asks questions — before starting or mid-task — answer
|
## Handling Reviewer ⚠️ Items
|
||||||
clearly and completely, provide additional context if needed, and don't
|
|
||||||
rush it into implementation.
|
|
||||||
|
|
||||||
### 3. Review the task
|
The task reviewer may report "⚠️ Cannot verify from diff" items — requirements
|
||||||
|
that live in unchanged code or span tasks. These do not block the rest of the
|
||||||
|
review, but you must resolve each one yourself before marking the task
|
||||||
|
complete: you hold the plan and cross-task context the reviewer
|
||||||
|
lacks. If you confirm an item is a real gap, treat it as a failed spec
|
||||||
|
review — send it back to the implementer and re-review.
|
||||||
|
|
||||||
|
## Constructing Reviewer Prompts
|
||||||
|
|
||||||
Per-task reviews are task-scoped gates. The broad review happens once, at the
|
Per-task reviews are task-scoped gates. The broad review happens once, at the
|
||||||
final whole-branch review. Never skip the task review, and never accept a
|
final whole-branch review. When you fill a reviewer template:
|
||||||
report missing either verdict — spec compliance AND task quality are both
|
|
||||||
required. Implementer self-review never replaces the task review; both are
|
|
||||||
needed.
|
|
||||||
|
|
||||||
- Hand the reviewer its diff as a file: run this skill's
|
|
||||||
`scripts/review-package BASE HEAD` and pass the reviewer the file path
|
|
||||||
it prints (or, without bash: `git log --oneline`, `git diff --stat`,
|
|
||||||
and `git diff -U10` for the range, redirected to one uniquely named
|
|
||||||
file). The output never enters your own context, and the reviewer sees
|
|
||||||
the commit list, stat summary, and full diff with context in one Read
|
|
||||||
call. Use the BASE you recorded before dispatching the implementer —
|
|
||||||
never `HEAD~1`, which silently truncates multi-commit tasks. Never
|
|
||||||
dispatch a task reviewer without a diff file.
|
|
||||||
- **Reviewer inputs:** the task reviewer gets three paths — the same brief
|
|
||||||
file, the report file, and the review package — plus the global
|
|
||||||
constraints that bind the task.
|
|
||||||
- The global-constraints block you hand the reviewer is its attention
|
|
||||||
lens. Copy the binding requirements verbatim from the plan's Global
|
|
||||||
Constraints section or the spec: exact values, exact formats, and the
|
|
||||||
stated relationships between components ("same layout as X", "matches
|
|
||||||
Y"). The reviewer's template already carries the process rules (YAGNI,
|
|
||||||
test hygiene, review method) — the constraints block is for what THIS
|
|
||||||
project's spec demands.
|
|
||||||
- Do not add open-ended directives like "check all uses" or "run race tests
|
- Do not add open-ended directives like "check all uses" or "run race tests
|
||||||
if useful" without a concrete, task-specific reason
|
if useful" without a concrete, task-specific reason
|
||||||
- Do not ask a reviewer to re-run tests the implementer already ran on the
|
- Do not ask a reviewer to re-run tests the implementer already ran on the
|
||||||
@@ -279,153 +174,111 @@ needed.
|
|||||||
loop. If the prompt you are writing contains "do not flag," "don't treat X
|
loop. If the prompt you are writing contains "do not flag," "don't treat X
|
||||||
as a defect," "at most Minor," or "the plan chose" — stop: you are
|
as a defect," "at most Minor," or "the plan chose" — stop: you are
|
||||||
pre-judging, usually to spare yourself a review loop.
|
pre-judging, usually to spare yourself a review loop.
|
||||||
|
- The global-constraints block you hand the reviewer is its attention
|
||||||
The task reviewer may report "⚠️ Cannot verify from diff" items — requirements
|
lens. Copy the binding requirements verbatim from the plan's Global
|
||||||
that live in unchanged code or span tasks. These do not block the rest of the
|
Constraints section or the spec: exact values, exact formats, and the
|
||||||
review, but you must resolve each one yourself before marking the task
|
stated relationships between components ("same layout as X", "matches
|
||||||
complete: you hold the plan and cross-task context the reviewer
|
Y"). The reviewer's template already carries the process rules (YAGNI,
|
||||||
lacks. If you confirm an item is a real gap, treat it as a failed spec
|
test hygiene, review method) — the constraints block is for what THIS
|
||||||
review — it enters the fix loop with the other findings.
|
project's spec demands.
|
||||||
|
- Hand the reviewer its diff as a file: run this skill's
|
||||||
Template: [task-reviewer-prompt.md](task-reviewer-prompt.md)
|
`scripts/review-package BASE HEAD` and pass the reviewer the file path
|
||||||
|
it prints (or, without bash: `git log --oneline`, `git diff --stat`,
|
||||||
### 4. The fix loop
|
and `git diff -U10` for the range, redirected to one uniquely named
|
||||||
|
file). The output never enters your own context, and the reviewer sees
|
||||||
The loop triggers when the review reports spec ❌, any Critical or Important
|
the commit list, stat summary, and full diff with context in one Read
|
||||||
finding, or a ⚠️ item you confirmed as a real gap.
|
call. Use the BASE you recorded before dispatching the implementer —
|
||||||
|
never `HEAD~1`, which silently truncates multi-commit tasks.
|
||||||
Before the loop starts, two routes leave it immediately:
|
- A dispatch prompt describes one task, not the session's history. Do not
|
||||||
|
paste accumulated prior-task summaries ("state after Tasks 1-3") into
|
||||||
- Record Minor findings in the progress ledger as you go
|
later dispatches — a real session's dispatch hit 42k chars of which 99%
|
||||||
(`Task <N>: minor (deferred): <one-liner>`), and point the final
|
was pasted history. A fresh subagent needs its task, the interfaces it
|
||||||
|
touches, and the global constraints. Nothing else.
|
||||||
|
- Dispatch fix subagents for Critical and Important findings. Record Minor
|
||||||
|
findings in the progress ledger as you go, and point the final
|
||||||
whole-branch review at that list so it can triage which must be fixed
|
whole-branch review at that list so it can triage which must be fixed
|
||||||
before merge. A roll-up nobody reads is a silent discard. Minor findings
|
before merge. A roll-up nobody reads is a silent discard.
|
||||||
never enter the loop.
|
|
||||||
- A finding labeled plan-mandated — or any finding that conflicts with
|
- A finding labeled plan-mandated — or any finding that conflicts with
|
||||||
what the plan's text requires — is the human's decision, like any plan
|
what the plan's text requires — is the human's decision, like any plan
|
||||||
contradiction: present the finding and the plan text, ask which governs.
|
contradiction: present the finding and the plan text, ask which governs.
|
||||||
Do not dismiss the finding because the plan mandates it, and do not
|
Do not dismiss the finding because the plan mandates it, and do not
|
||||||
dispatch a fix that contradicts the plan without asking.
|
dispatch a fix that contradicts the plan without asking.
|
||||||
|
- The final whole-branch review gets a package too: run
|
||||||
|
`scripts/review-package MERGE_BASE HEAD` (MERGE_BASE = the commit the
|
||||||
|
branch started from, e.g. `git merge-base main HEAD`) and include the
|
||||||
|
printed path in the final review dispatch, so the final reviewer reads
|
||||||
|
one file instead of re-deriving the branch diff with git commands.
|
||||||
|
- Every fix dispatch carries the implementer contract: the fix subagent
|
||||||
|
re-runs the tests covering its change and reports the results. Name the
|
||||||
|
covering test files in the dispatch — a one-line fix does not need the
|
||||||
|
whole suite. Before re-dispatching the reviewer, confirm the fix report
|
||||||
|
contains the covering tests, the command run, and the output; dispatch
|
||||||
|
the re-review once all three are present.
|
||||||
|
- If the final whole-branch review returns findings, dispatch ONE fix
|
||||||
|
subagent with the complete findings list — not one fixer per finding.
|
||||||
|
Per-finding fixers each rebuild context and re-run suites; a real
|
||||||
|
session's final-review fix wave cost more than all its tasks combined.
|
||||||
|
|
||||||
Everything else enters the loop. A fix round is one fix dispatch plus one
|
## File Handoffs
|
||||||
scoped re-review. Five rounds maximum per task:
|
|
||||||
|
|
||||||
**Rounds 1-3 — resume the original implementer.** Send it the open findings
|
Everything you paste into a dispatch prompt — and everything a subagent
|
||||||
verbatim. Its context is intact: it knows the task, the code, and its own
|
prints back — stays resident in your context for the rest of the session
|
||||||
choices. If your harness cannot send another message to a live subagent,
|
and is re-read on every later turn. Hand artifacts over as files:
|
||||||
dispatch a fresh implementer carrying the brief path, the report-file path,
|
|
||||||
and the findings — the report file is the persistent memory either way.
|
|
||||||
|
|
||||||
**Rounds 4-5 — dispatch a fresh implementer on a more capable model** (per
|
- **Task brief:** before dispatching an implementer, run this skill's
|
||||||
Model Selection), with the brief path, the report-file path, the open
|
`scripts/task-brief PLAN_FILE N` — it extracts the task's full text to a
|
||||||
findings, and this framing: "A prior implementer attempted this task
|
uniquely named file and prints the path. Compose the dispatch so the
|
||||||
[N] times; you own it now. Read the report file for what was tried." A loop
|
brief stays the single source of requirements. Your dispatch should
|
||||||
that survives three resumes usually means the implementer cannot see its
|
contain: (1) one line on where this task fits in the project; (2) the
|
||||||
own problem — fresh eyes and a capability bump in one move.
|
brief path, introduced as "read this first — it is your requirements,
|
||||||
|
with the exact values to use verbatim"; (3) interfaces and decisions
|
||||||
|
from earlier tasks that the brief cannot know; (4) your resolution of
|
||||||
|
any ambiguity you noticed in the brief; (5) the report-file path and
|
||||||
|
report contract. Exact values (numbers, magic strings, signatures, test
|
||||||
|
cases) appear only in the brief.
|
||||||
|
- **Report file:** name the implementer's report file after the brief
|
||||||
|
(brief `…/task-N-brief.md` → report `…/task-N-report.md`) and put it in
|
||||||
|
the dispatch prompt. The implementer writes the full report there and
|
||||||
|
returns only status, commits, a one-line test summary, and concerns.
|
||||||
|
- **Reviewer inputs:** the task reviewer gets three paths — the same brief
|
||||||
|
file, the report file, and the review package — plus the global
|
||||||
|
constraints that bind the task.
|
||||||
|
- Fix dispatches append their fix report (with test results) to the same
|
||||||
|
report file and return a short summary; re-reviews read the updated file.
|
||||||
|
|
||||||
**Every round, either way:** the implementer fixes, re-runs the tests
|
## Durable Progress
|
||||||
covering the amended code, appends its fix report to the same report file,
|
|
||||||
and returns the short contract. Before re-dispatching the reviewer, confirm
|
|
||||||
the fix report contains the covering tests, the command run, and the
|
|
||||||
output; dispatch the re-review once all three are present. Name the
|
|
||||||
covering test files in the fix message — a one-line fix does not need the
|
|
||||||
whole suite.
|
|
||||||
|
|
||||||
**The re-review is scoped.** Run `scripts/review-package FIX_BASE HEAD`
|
Conversation memory does not survive compaction. In real sessions,
|
||||||
where FIX_BASE is the head the previous review saw, and dispatch
|
controllers that lost their place have re-dispatched entire completed task
|
||||||
[re-review-prompt.md](re-review-prompt.md) with the findings list, the
|
sequences — the single most expensive failure observed. Track progress in
|
||||||
brief, the report file, and the printed diff path. The re-reviewer verdicts
|
a ledger file, not only in todos.
|
||||||
each finding ADDRESSED or NOT ADDRESSED and flags new breakage in the fix
|
|
||||||
diff only. New Critical/Important breakage in the fix diff joins the open
|
|
||||||
findings list. Out-of-scope observations go to the ledger as deferred
|
|
||||||
minors — they never extend the loop.
|
|
||||||
|
|
||||||
**After each round,** append to the ledger:
|
- At skill start, check for a ledger:
|
||||||
`Task <N>: fix round <R>/5 (<X> addressed, <Y> open — <finding one-liners>; commits <a7>..<b7>)`
|
`cat "$(git rev-parse --show-toplevel)/.superpowers/sdd/progress.md"`. Tasks listed there
|
||||||
|
as complete are DONE — do not re-dispatch them; resume at the first task
|
||||||
|
not marked complete.
|
||||||
|
- When a task's review comes back clean, append one line to the ledger in
|
||||||
|
the same message as your other bookkeeping:
|
||||||
|
`Task N: complete (commits <base7>..<head7>, review clean)`.
|
||||||
|
- The ledger is your recovery map: the commits it names exist in git even
|
||||||
|
when your context no longer remembers creating them. After compaction,
|
||||||
|
trust the ledger and `git log` over your own recollection.
|
||||||
|
- `git clean -fdx` will destroy the ledger (it's git-ignored scratch); if
|
||||||
|
that happens, recover from `git log`.
|
||||||
|
|
||||||
Never fix findings yourself in the controller session — your context stays
|
## Prompt Templates
|
||||||
clean for coordination, and controller fixes skip review.
|
|
||||||
|
|
||||||
**The breaker.** When round 5's re-review still leaves findings open, stop
|
- [implementer-prompt.md](implementer-prompt.md) - Dispatch implementer subagent
|
||||||
dispatching. Adjudicate each open finding yourself — you hold the plan and
|
- [task-reviewer-prompt.md](task-reviewer-prompt.md) - Dispatch task reviewer subagent (spec compliance + code quality)
|
||||||
the cross-task context the reviewer lacks:
|
- Final whole-branch review: use superpowers:requesting-code-review's [code-reviewer.md](../requesting-code-review/code-reviewer.md)
|
||||||
|
|
||||||
- **The reviewer is wrong, or the point is contestable:** park it —
|
|
||||||
`Task <N>: parked — <finding> — ruling: <why the code stands>`. The final
|
|
||||||
review sees both sides.
|
|
||||||
- **Real, but nothing downstream builds on it:** park it the same way, with
|
|
||||||
a ruling that says it's real and deferred.
|
|
||||||
- **Real and load-bearing** — a later task builds on it, or it reveals a
|
|
||||||
plan defect: STOP. Append `Task <N>: BLOCKED — <reason>` and report to
|
|
||||||
your human partner with the finding, the plan text it collides with, and
|
|
||||||
the fix history. Parking a structural failure lets every dependent task
|
|
||||||
build on it and hands the final review a problem it cannot fix either.
|
|
||||||
|
|
||||||
Adjudicate only at the cap. Adjudicating earlier to end a loop is
|
|
||||||
pre-judging with a different name. Every adjudication is a ledger entry —
|
|
||||||
a silent discard is forbidden.
|
|
||||||
|
|
||||||
### 5. Complete the task
|
|
||||||
|
|
||||||
When the review comes back clean — or every open finding is parked with a
|
|
||||||
ruling at the cap — append the completion line to the ledger in the same
|
|
||||||
message as your other bookkeeping:
|
|
||||||
|
|
||||||
- `Task <N>: complete (commits <base7>..<head7>, review clean)`
|
|
||||||
- `Task <N>: complete (commits <base7>..<head7>, <K> parked)` after a
|
|
||||||
tripped breaker
|
|
||||||
|
|
||||||
Then mark the todo complete and move on. Never move to the next task while
|
|
||||||
the review has open Critical/Important issues that are neither fixed nor
|
|
||||||
parked-with-ruling at the cap.
|
|
||||||
|
|
||||||
## Final Review
|
|
||||||
|
|
||||||
The final whole-branch review gets a package too: run
|
|
||||||
`scripts/review-package MERGE_BASE HEAD` (MERGE_BASE = the commit the
|
|
||||||
branch started from, e.g. `git merge-base main HEAD`) and include the
|
|
||||||
printed path in the final review dispatch, so the final reviewer reads
|
|
||||||
one file instead of re-deriving the branch diff with git commands. Dispatch
|
|
||||||
on the most capable available model (see Model Selection), using
|
|
||||||
superpowers:requesting-code-review's
|
|
||||||
[code-reviewer.md](../requesting-code-review/code-reviewer.md). Point it at
|
|
||||||
the ledger's deferred-minor and parked lines so it can triage which must be
|
|
||||||
fixed before merge.
|
|
||||||
|
|
||||||
If the final whole-branch review returns findings, dispatch ONE fix subagent
|
|
||||||
with the complete findings list — not one fixer per finding.
|
|
||||||
Per-finding fixers each rebuild context and re-run suites; a real
|
|
||||||
session's final-review fix wave cost more than all its tasks combined.
|
|
||||||
Then run exactly one scoped re-review of the fix wave
|
|
||||||
(`scripts/review-package` over the fix range, [re-review-prompt.md](re-review-prompt.md)).
|
|
||||||
Adjudicate any residual findings as in the task loop's breaker: park with
|
|
||||||
rulings, or stop on load-bearing ones. There is no second fix wave —
|
|
||||||
residual load-bearing findings surface to your human partner when
|
|
||||||
finishing-a-development-branch presents the options.
|
|
||||||
|
|
||||||
## Finish
|
|
||||||
|
|
||||||
Use superpowers:finishing-a-development-branch.
|
|
||||||
|
|
||||||
## Common Rationalizations
|
|
||||||
|
|
||||||
| Excuse | Reality |
|
|
||||||
|--------|---------|
|
|
||||||
| "Close enough on spec compliance" | Reviewer found spec gaps = not done. Fix or hit the cap and adjudicate — those are the only exits. |
|
|
||||||
| "I'll fix it myself, dispatching is overhead" | Controller fixes pollute your context and skip review. Resume the implementer. |
|
|
||||||
| "One more round will converge" | Past the cap, rounds don't converge — the failure is structural. Adjudicate and route. |
|
|
||||||
| "The reviewer will just find something new anyway" | Scoped re-reviews verify fixes; they cannot wander. New findings on untouched code go to the ledger, not the loop. |
|
|
||||||
| "This finding is obviously wrong, I'll drop it" | You adjudicate only at the cap, and every ruling is a ledger entry. Silent discards are forbidden. |
|
|
||||||
| "The fix was small, skip the re-review" | Unreviewed fixes are how regressions land. Every round ends with a scoped re-review. |
|
|
||||||
| "Reviews slow the loop down" | The loop without reviews is just unverified churn. Reviews are the loop's brakes and steering. |
|
|
||||||
| "Ledger bookkeeping is overhead" | The ledger is what survives compaction. Controllers without one have re-dispatched entire completed task sequences. |
|
|
||||||
|
|
||||||
## Example Workflow
|
## Example Workflow
|
||||||
|
|
||||||
```
|
```
|
||||||
You: I'm using Subagent-Driven Development to execute this plan.
|
You: I'm using Subagent-Driven Development to execute this plan.
|
||||||
|
|
||||||
[Setup: worktree verified, no ledger found, read plan, created todos]
|
[Read plan file once: docs/superpowers/plans/feature-plan.md]
|
||||||
|
[Create todos for all tasks]
|
||||||
|
|
||||||
Task 1: Hook installation script
|
Task 1: Hook installation script
|
||||||
|
|
||||||
@@ -435,7 +288,8 @@ Implementer: "Before I begin - should the hook be installed at user or system le
|
|||||||
|
|
||||||
You: "User level (~/.config/superpowers/hooks/)"
|
You: "User level (~/.config/superpowers/hooks/)"
|
||||||
|
|
||||||
Implementer: [Later]
|
Implementer: "Got it. Implementing now..."
|
||||||
|
[Later] Implementer:
|
||||||
- Implemented install-hook command
|
- Implemented install-hook command
|
||||||
- Added tests, 5/5 passing
|
- Added tests, 5/5 passing
|
||||||
- Self-review: Found I missed --force flag, added it
|
- Self-review: Found I missed --force flag, added it
|
||||||
@@ -445,39 +299,77 @@ Implementer: [Later]
|
|||||||
Task reviewer: Spec ✅ - all requirements met, nothing extra.
|
Task reviewer: Spec ✅ - all requirements met, nothing extra.
|
||||||
Strengths: Good test coverage, clean. Issues: None. Task quality: Approved.
|
Strengths: Good test coverage, clean. Issues: None. Task quality: Approved.
|
||||||
|
|
||||||
[Ledger: Task 1: complete (commits a1b2c3d..d4e5f6a, review clean)]
|
[Mark Task 1 complete]
|
||||||
|
|
||||||
Task 2: Recovery modes
|
Task 2: Recovery modes
|
||||||
|
|
||||||
[Run task-brief for Task 2; dispatch implementer with brief + report paths + context]
|
[Run task-brief for Task 2; dispatch implementer with brief + report paths + context]
|
||||||
|
|
||||||
Implementer: [No questions]
|
Implementer: [No questions, proceeds]
|
||||||
|
Implementer:
|
||||||
- Added verify/repair modes
|
- Added verify/repair modes
|
||||||
- 8/8 tests passing
|
- 8/8 tests passing
|
||||||
|
- Self-review: All good
|
||||||
- Committed
|
- Committed
|
||||||
|
|
||||||
[Run review-package, dispatch task reviewer with the printed path]
|
[Run review-package, dispatch task reviewer with the printed path]
|
||||||
Task reviewer: Spec ❌:
|
Task reviewer: Spec ❌:
|
||||||
- Missing: Progress reporting (spec says "report every 100 items")
|
- Missing: Progress reporting (spec says "report every 100 items")
|
||||||
|
- Extra: Added --json flag (not requested)
|
||||||
Issues (Important): Magic number (100)
|
Issues (Important): Magic number (100)
|
||||||
|
|
||||||
[Fix round 1: resume the implementer with both findings]
|
[Dispatch fix subagent with all findings]
|
||||||
Implementer: Added progress reporting, extracted PROGRESS_INTERVAL constant.
|
Fixer: Removed --json flag, added progress reporting, extracted PROGRESS_INTERVAL constant
|
||||||
Re-ran test/recovery.test.js — 10/10 passing. Fix report appended.
|
|
||||||
|
|
||||||
[Run review-package FIX_BASE HEAD; dispatch scoped re-review]
|
[Task reviewer reviews again]
|
||||||
Re-reviewer: Missing progress reporting — ADDRESSED (src/recovery.js:41).
|
Task reviewer: Spec ✅. Task quality: Approved.
|
||||||
Magic number — ADDRESSED (src/recovery.js:7). New breakage: none.
|
|
||||||
Verdict: all findings addressed.
|
|
||||||
|
|
||||||
[Ledger: Task 2: fix round 1/5 (2 addressed, 0 open; commits d4e5f6a..b7c8d9e)]
|
[Mark Task 2 complete]
|
||||||
[Ledger: Task 2: complete (commits d4e5f6a..b7c8d9e, review clean)]
|
|
||||||
|
|
||||||
...
|
...
|
||||||
|
|
||||||
[After all tasks]
|
[After all tasks]
|
||||||
[Run review-package MERGE_BASE HEAD; dispatch final code-reviewer, most capable model]
|
[Dispatch final code-reviewer]
|
||||||
Final reviewer: All requirements met. Deferred minors triaged: none block merge.
|
Final reviewer: All requirements met, ready to merge
|
||||||
|
|
||||||
Done! Using superpowers:finishing-a-development-branch.
|
Done!
|
||||||
```
|
```
|
||||||
|
|
||||||
|
## Red Flags
|
||||||
|
|
||||||
|
**Never:**
|
||||||
|
- Start implementation on main/master branch without explicit user consent
|
||||||
|
- Skip task review, or accept a report missing either verdict (spec compliance AND task quality are both required)
|
||||||
|
- Proceed with unfixed issues
|
||||||
|
- Dispatch multiple implementation subagents in parallel (conflicts)
|
||||||
|
- Make a subagent read the whole plan file (hand it its task brief —
|
||||||
|
`scripts/task-brief` — instead)
|
||||||
|
- Skip scene-setting context (subagent needs to understand where task fits)
|
||||||
|
- Ignore subagent questions (answer before letting them proceed)
|
||||||
|
- Accept "close enough" on spec compliance (reviewer found spec issues = not done)
|
||||||
|
- Skip review loops (reviewer found issues = implementer fixes = review again)
|
||||||
|
- Let implementer self-review replace actual review (both are needed)
|
||||||
|
- Tell a reviewer what not to flag, or pre-rate a finding's severity in the
|
||||||
|
dispatch prompt ("treat it as Minor at most") — the plan's example code is
|
||||||
|
a starting point, not evidence that its weaknesses were chosen
|
||||||
|
- Dispatch a task reviewer without a diff file — generate it first
|
||||||
|
(`scripts/review-package BASE HEAD`) and name the printed path in the
|
||||||
|
prompt
|
||||||
|
- Move to next task while the review has open Critical/Important issues
|
||||||
|
- Re-dispatch a task the progress ledger already marks complete — check
|
||||||
|
the ledger (and `git log`) after any compaction or resume
|
||||||
|
|
||||||
|
**If subagent asks questions:**
|
||||||
|
- Answer clearly and completely
|
||||||
|
- Provide additional context if needed
|
||||||
|
- Don't rush them into implementation
|
||||||
|
|
||||||
|
**If reviewer finds issues:**
|
||||||
|
- Implementer (same subagent) fixes them
|
||||||
|
- Reviewer reviews again
|
||||||
|
- Repeat until approved
|
||||||
|
- Don't skip the re-review
|
||||||
|
|
||||||
|
**If subagent fails task:**
|
||||||
|
- Dispatch fix subagent with specific instructions
|
||||||
|
- Don't try to fix manually (context pollution)
|
||||||
|
|||||||
@@ -106,12 +106,9 @@ Subagent (general-purpose):
|
|||||||
|
|
||||||
## After Review Findings
|
## After Review Findings
|
||||||
|
|
||||||
If the task review finds issues, you will be resumed with the findings.
|
If a reviewer finds issues and you fix them, re-run the tests that cover
|
||||||
Fix them, re-run the tests that cover the amended code, and append a fix
|
the amended code and append the results to your report file. Reviewers
|
||||||
report to your report file: what you changed, the covering tests you
|
will not re-run tests for you — your report is the test evidence.
|
||||||
ran, the command, and the output. Reviewers will not re-run tests for
|
|
||||||
you — your report is the test evidence. Then reply with the same short
|
|
||||||
status contract as your first report.
|
|
||||||
|
|
||||||
## Report Format
|
## Report Format
|
||||||
|
|
||||||
|
|||||||
@@ -1,106 +0,0 @@
|
|||||||
# Scoped Re-Review Prompt Template
|
|
||||||
|
|
||||||
Use this template when dispatching a re-review after a fix round. The
|
|
||||||
re-reviewer verifies the findings were addressed and checks the fix diff for
|
|
||||||
new breakage. It is not a fresh review — the full review already happened.
|
|
||||||
|
|
||||||
**Purpose:** Verify each finding from the previous review was addressed, and
|
|
||||||
that the fix itself broke nothing.
|
|
||||||
|
|
||||||
```
|
|
||||||
Subagent (general-purpose):
|
|
||||||
description: "Re-review Task N fix round R"
|
|
||||||
model: [MODEL — REQUIRED: choose per SKILL.md Model Selection; an omitted
|
|
||||||
model silently inherits the session's most expensive one]
|
|
||||||
prompt: |
|
|
||||||
You are re-reviewing one task's fix round. A previous review produced
|
|
||||||
findings; an implementer has attempted to fix them. Your job is to
|
|
||||||
verdict each finding and inspect the fix diff — nothing else.
|
|
||||||
|
|
||||||
## The Task
|
|
||||||
|
|
||||||
Read the task brief: [BRIEF_FILE]
|
|
||||||
|
|
||||||
## The Findings Under Verification
|
|
||||||
|
|
||||||
[FINDINGS]
|
|
||||||
|
|
||||||
## The Fix
|
|
||||||
|
|
||||||
Read the implementer's report (fix reports are appended at the end):
|
|
||||||
[REPORT_FILE]
|
|
||||||
|
|
||||||
**Fix base:** [FIX_BASE_SHA] (the head the previous review saw)
|
|
||||||
**Head:** [HEAD_SHA]
|
|
||||||
**Diff file:** [DIFF_FILE]
|
|
||||||
|
|
||||||
Read the diff file once — it contains the fix commits, a stat summary,
|
|
||||||
and the fix diff with surrounding context. Do not re-run git commands.
|
|
||||||
If the diff file is missing, fetch the diff yourself:
|
|
||||||
`git diff --stat [FIX_BASE_SHA]..[HEAD_SHA]` and
|
|
||||||
`git diff [FIX_BASE_SHA]..[HEAD_SHA]`.
|
|
||||||
|
|
||||||
Your review is read-only on this checkout. Do not mutate the working
|
|
||||||
tree, the index, HEAD, or branch state in any way.
|
|
||||||
|
|
||||||
## Scope
|
|
||||||
|
|
||||||
Your scope is the findings list and the fix diff. Verdict every finding.
|
|
||||||
Inspect the fix diff for new problems the fix itself introduced. Do NOT
|
|
||||||
re-review code the fix did not touch: if you notice an issue entirely
|
|
||||||
outside the fix diff, report it under Out-of-Scope Observations — it
|
|
||||||
does not block this task and does not extend the loop. A broad
|
|
||||||
whole-branch review happens after all tasks are complete.
|
|
||||||
|
|
||||||
## Tests
|
|
||||||
|
|
||||||
The implementer re-ran the tests covering the amended code and appended
|
|
||||||
the results to the report file. Treat the report as unverified claims:
|
|
||||||
confirm the fix report names the covering tests and shows their output,
|
|
||||||
and verify the claims against the diff. Do not re-run the suite to
|
|
||||||
confirm their report. Run a test only when reading the code raises a
|
|
||||||
specific doubt that no existing run answers — and then a focused test,
|
|
||||||
never a package-wide suite.
|
|
||||||
|
|
||||||
## Output Format
|
|
||||||
|
|
||||||
Your final message is the report itself: begin directly with the first
|
|
||||||
finding's verdict. Every line is a verdict, a finding with file:line,
|
|
||||||
or a check you ran — no preamble, no process narration.
|
|
||||||
|
|
||||||
### Finding Verdicts
|
|
||||||
|
|
||||||
For each finding in The Findings Under Verification, in order:
|
|
||||||
- **[finding one-liner]** — ADDRESSED | NOT ADDRESSED, with file:line
|
|
||||||
evidence. "Attempted" is not addressed: the specific defect must no
|
|
||||||
longer exist.
|
|
||||||
|
|
||||||
### New Breakage in the Fix Diff
|
|
||||||
|
|
||||||
Anything the fix itself broke or introduced, with severity
|
|
||||||
(Critical/Important/Minor) and file:line. "None" if clean.
|
|
||||||
|
|
||||||
### Out-of-Scope Observations
|
|
||||||
|
|
||||||
Issues you noticed entirely outside the fix diff. Non-blocking; the
|
|
||||||
controller ledgers these for the final review. "None" if none.
|
|
||||||
|
|
||||||
### Verdict
|
|
||||||
|
|
||||||
**Fix round:** [All findings addressed, no new Critical/Important
|
|
||||||
breakage | Findings remain open] — list the open ones.
|
|
||||||
```
|
|
||||||
|
|
||||||
**Placeholders:**
|
|
||||||
- `[MODEL]` — REQUIRED: reviewer model per SKILL.md Model Selection; scoped
|
|
||||||
re-reviews of small fix diffs take a cheap-to-mid tier
|
|
||||||
- `[BRIEF_FILE]` — the task brief file (same file the implementer worked from)
|
|
||||||
- `[FINDINGS]` — the Critical/Important findings and spec gaps from the
|
|
||||||
previous review, copied verbatim, one per bullet
|
|
||||||
- `[REPORT_FILE]` — the implementer's report file (fix reports appended)
|
|
||||||
- `[FIX_BASE_SHA]` — the head the previous review saw
|
|
||||||
- `[HEAD_SHA]` — current commit
|
|
||||||
- `[DIFF_FILE]` — the path `scripts/review-package FIX_BASE HEAD` printed
|
|
||||||
|
|
||||||
**Re-reviewer returns:** per-finding verdicts (ADDRESSED / NOT ADDRESSED),
|
|
||||||
new breakage in the fix diff, out-of-scope observations, and a round verdict.
|
|
||||||
@@ -183,3 +183,6 @@ Subagent (general-purpose):
|
|||||||
|
|
||||||
**Reviewer returns:** Spec Compliance verdict (✅/❌/⚠️), Strengths, Issues
|
**Reviewer returns:** Spec Compliance verdict (✅/❌/⚠️), Strengths, Issues
|
||||||
(Critical/Important/Minor), Task quality verdict
|
(Critical/Important/Minor), Task quality verdict
|
||||||
|
|
||||||
|
A fix dispatch can address spec gaps and quality findings together;
|
||||||
|
re-review after fixes covers both verdicts.
|
||||||
|
|||||||
@@ -7,7 +7,7 @@ Add to your Codex config (`~/.codex/config.toml`):
|
|||||||
multi_agent = true
|
multi_agent = true
|
||||||
```
|
```
|
||||||
|
|
||||||
This enables `spawn_agent`, `wait_agent`, and `close_agent` for skills like `dispatching-parallel-agents` and `subagent-driven-development`. When using subagent-driven-development, close reviewer subagents when their review returns. Keep each implementer subagent open until its task's review passes — the fix loop resumes the implementer — then close it. If your harness cannot send another message to a spawned agent, dispatch each fix round as a fresh implementer carrying the brief, the report file, and the findings.
|
This enables `spawn_agent`, `wait_agent`, and `close_agent` for skills like `dispatching-parallel-agents` and `subagent-driven-development`. When using subagent-driven-development, you should always close implementer and reviewer subagents when they have finished all their work.
|
||||||
|
|
||||||
## Environment Detection
|
## Environment Detection
|
||||||
|
|
||||||
|
|||||||
Reference in New Issue
Block a user