mirror of
https://github.com/obra/superpowers.git
synced 2026-07-23 18:54:07 +08:00
rename(sdd): re-review -> fix review, a scoped review of the fix diff
Responds to maintainer review: "re-review" names the action badly — it
reads as "review again," which is the exact failure observed in the
field (fix reviewers re-running full reviews and package-wide suites
against explicit scope instructions). "Fix review" names the artifact
under review — the fix diff since the previous review — and makes the
scope self-enforcing.
Pure vocabulary substitution: re-review-prompt.md becomes
fix-review-prompt.md, SKILL.md and the review-package --role value
follow, and the term is introduced once as "a scoped fix review — a
review of the fix diff, not a fresh review." No behavioral rules
changed. Historical records (docs/superpowers/plans, specs,
RELEASE-NOTES) keep the old vocabulary; other skills' generic verb
usage ("no need to re-review") is untouched.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
@@ -59,7 +59,7 @@ digraph process {
|
||||
"Finding conflicts with plan text?" [shape=diamond];
|
||||
"Ask human partner which governs" [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];
|
||||
"Dispatch scoped fix review (./fix-review-prompt.md)" [shape=box];
|
||||
"All findings addressed?" [shape=diamond];
|
||||
"R = 5?" [shape=diamond];
|
||||
"Adjudicate each open finding" [shape=box];
|
||||
@@ -72,7 +72,7 @@ digraph process {
|
||||
"Setup: worktree, ledger check, read plan, pre-flight review" [shape=box];
|
||||
"More tasks remain?" [shape=diamond];
|
||||
"Dispatch final code reviewer (../requesting-code-review/code-reviewer.md)" [shape=box];
|
||||
"Final findings? ONE fix dispatch, one scoped re-review, adjudicate residuals" [shape=box];
|
||||
"Final findings? ONE fix dispatch, one scoped fix review, adjudicate residuals" [shape=box];
|
||||
"Final review clean: delete this plan's workspace" [shape=box];
|
||||
"Use superpowers:finishing-a-development-branch" [shape=box style=filled fillcolor=lightgreen];
|
||||
|
||||
@@ -88,8 +88,8 @@ digraph process {
|
||||
"Finding conflicts with plan text?" -> "Ask human partner which governs" [label="yes"];
|
||||
"Ask human partner which governs" -> "Fix round R of 5: R≤3 resume implementer; R≥4 fresh implementer, more capable model";
|
||||
"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?";
|
||||
"Fix round R of 5: R≤3 resume implementer; R≥4 fresh implementer, more capable model" -> "Dispatch scoped fix review (./fix-review-prompt.md)";
|
||||
"Dispatch scoped fix review (./fix-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"];
|
||||
@@ -101,8 +101,8 @@ digraph process {
|
||||
"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 final code reviewer (../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";
|
||||
"Final findings? ONE fix dispatch, one scoped re-review, adjudicate residuals" -> "Final review clean: delete this plan's workspace";
|
||||
"Dispatch final code reviewer (../requesting-code-review/code-reviewer.md)" -> "Final findings? ONE fix dispatch, one scoped fix review, adjudicate residuals";
|
||||
"Final findings? ONE fix dispatch, one scoped fix review, adjudicate residuals" -> "Final review clean: delete this plan's workspace";
|
||||
"Final review clean: delete this plan's workspace" -> "Use superpowers:finishing-a-development-branch";
|
||||
}
|
||||
```
|
||||
@@ -174,7 +174,7 @@ capable available model, not the session default.
|
||||
|
||||
**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
|
||||
most capable model; a subtle concurrency change does. Scoped re-reviews of
|
||||
most capable model; a subtle concurrency change does. Scoped fix reviews of
|
||||
small fix diffs take a cheap-to-mid tier.
|
||||
|
||||
**Fix-loop escalation (rounds 4-5)**: use a model at least one tier above
|
||||
@@ -323,7 +323,8 @@ Before the loop starts, two routes leave it immediately:
|
||||
Do not dismiss the finding because the plan mandates it, and do not
|
||||
dispatch a fix that contradicts the plan without asking.
|
||||
Everything else enters the loop. A fix round is one fix dispatch plus one
|
||||
scoped re-review. Five rounds maximum per task:
|
||||
scoped fix review — a review of the fix diff, not a fresh review. Five
|
||||
rounds maximum per task:
|
||||
|
||||
**Rounds 1-3 — resume the original implementer.** Send it the open findings
|
||||
verbatim. Its context is intact: it knows the task, the code, and its own
|
||||
@@ -342,14 +343,14 @@ own problem — fresh eyes and a capability bump in one move.
|
||||
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
|
||||
output; dispatch the fix 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 --role re-review PLAN_FILE FIX_BASE HEAD`
|
||||
**The fix review is scoped.** Run `scripts/review-package --role fix-review PLAN_FILE FIX_BASE HEAD`
|
||||
where FIX_BASE is the head the previous review saw, and dispatch
|
||||
[re-review-prompt.md](re-review-prompt.md) with the findings list, the
|
||||
brief, the report file, and the printed diff path. The re-reviewer verdicts
|
||||
[fix-review-prompt.md](fix-review-prompt.md) with the findings list, the
|
||||
brief, the report file, and the printed diff path. The fix reviewer verdicts
|
||||
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
|
||||
@@ -361,7 +362,7 @@ minors — they never extend the loop.
|
||||
Never fix findings yourself in the controller session — your context stays
|
||||
clean for coordination, and controller fixes skip review.
|
||||
|
||||
**The breaker.** When round 5's re-review still leaves findings open, stop
|
||||
**The breaker.** When round 5's fix review still leaves findings open, stop
|
||||
dispatching. Adjudicate each open finding yourself — you hold the plan and
|
||||
the cross-task context the reviewer lacks:
|
||||
|
||||
@@ -411,9 +412,9 @@ 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 --role re-review PLAN_FILE FIX_BASE HEAD` over the fix range,
|
||||
[re-review-prompt.md](re-review-prompt.md)).
|
||||
Then run exactly one scoped fix review of the fix wave
|
||||
(`scripts/review-package --role fix-review PLAN_FILE FIX_BASE HEAD` over the fix range,
|
||||
[fix-review-prompt.md](fix-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
|
||||
@@ -444,9 +445,9 @@ Use superpowers:finishing-a-development-branch.
|
||||
| "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. |
|
||||
| "The reviewer will just find something new anyway" | Scoped fix 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. |
|
||||
| "The fix was small, skip the fix review" | Unreviewed fixes are how regressions land. Every round ends with a scoped fix 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. |
|
||||
| "This new finding is real — one more wave" | Real findings are infinite under a strong reviewer. The completed wave is the exit; adjudicate and route. |
|
||||
@@ -500,8 +501,8 @@ Task reviewer: Spec ❌:
|
||||
Implementer: Added progress reporting, extracted PROGRESS_INTERVAL constant.
|
||||
Re-ran test/recovery.test.js — 10/10 passing. Fix report appended.
|
||||
|
||||
[Run review-package --role re-review PLAN_FILE FIX_BASE HEAD; dispatch scoped re-review]
|
||||
Re-reviewer: Missing progress reporting — ADDRESSED (src/recovery.js:41).
|
||||
[Run review-package --role fix-review PLAN_FILE FIX_BASE HEAD; dispatch scoped fix review]
|
||||
Fix reviewer: Missing progress reporting — ADDRESSED (src/recovery.js:41).
|
||||
Magic number — ADDRESSED (src/recovery.js:7). New breakage: none.
|
||||
Verdict: all findings addressed.
|
||||
|
||||
|
||||
@@ -1,7 +1,7 @@
|
||||
# Scoped Re-Review Prompt Template
|
||||
# Scoped Fix 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
|
||||
Use this template when dispatching a fix review after a fix round. The
|
||||
fix 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
|
||||
@@ -9,11 +9,11 @@ that the fix itself broke nothing.
|
||||
|
||||
```
|
||||
Subagent (general-purpose):
|
||||
description: "Re-review Task N fix round R"
|
||||
description: "Fix review Task N 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
|
||||
You are 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.
|
||||
|
||||
@@ -47,7 +47,7 @@ Subagent (general-purpose):
|
||||
|
||||
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
|
||||
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.
|
||||
@@ -93,14 +93,14 @@ Subagent (general-purpose):
|
||||
|
||||
**Placeholders:**
|
||||
- `[MODEL]` — REQUIRED: reviewer model per SKILL.md Model Selection; scoped
|
||||
re-reviews of small fix diffs take a cheap-to-mid tier
|
||||
fix 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 PLAN_FILE FIX_BASE HEAD` printed
|
||||
- `[DIFF_FILE]` — the path `scripts/review-package --role fix-review PLAN_FILE FIX_BASE HEAD` printed
|
||||
|
||||
**Re-reviewer returns:** per-finding verdicts (ADDRESSED / NOT ADDRESSED),
|
||||
**Fix reviewer returns:** per-finding verdicts (ADDRESSED / NOT ADDRESSED),
|
||||
new breakage in the fix diff, out-of-scope observations, and a round verdict.
|
||||
@@ -4,9 +4,9 @@
|
||||
# call. Using the recorded per-task BASE (not HEAD~1) keeps multi-commit
|
||||
# tasks intact.
|
||||
#
|
||||
# Usage: review-package [--role task-review|re-review|final-review] PLAN_FILE BASE HEAD [OUTFILE]
|
||||
# Usage: review-package [--role task-review|fix-review|final-review] PLAN_FILE BASE HEAD [OUTFILE]
|
||||
# Default OUTFILE: <repo-root>/.superpowers/sdd/<plan-basename>/review-<base7>..<head7>.diff
|
||||
# (named per range, so a re-review after fixes gets a distinct fresh file).
|
||||
# (named per range, so a fix review after fixes gets a distinct fresh file).
|
||||
#
|
||||
# The trailing dispatch hint rides this output because the controller reads it
|
||||
# immediately before spawning the reviewer; skill text loaded at session start
|
||||
@@ -17,22 +17,18 @@ script_dir=$(cd "$(dirname "$0")" && pwd)
|
||||
|
||||
role=task-review
|
||||
if [ "${1:-}" = "--role" ]; then
|
||||
[ $# -ge 2 ] || { echo "usage: review-package [--role task-review|re-review|final-review] PLAN_FILE BASE HEAD [OUTFILE]" >&2; exit 2; }
|
||||
[ $# -ge 2 ] || { echo "usage: review-package [--role task-review|fix-review|final-review] PLAN_FILE BASE HEAD [OUTFILE]" >&2; exit 2; }
|
||||
role=$2
|
||||
shift 2
|
||||
fi
|
||||
|
||||
case "$role" in
|
||||
task-review) hint_key=task-review ;;
|
||||
# re-review maps onto the fix-review hints entry; the --role value itself
|
||||
# is renamed in the next commit.
|
||||
re-review) hint_key=fix-review ;;
|
||||
final-review) hint_key=final-review ;;
|
||||
*) echo "bad --role: ${role} (task-review|re-review|final-review)" >&2; exit 2 ;;
|
||||
task-review|fix-review|final-review) hint_key=$role ;;
|
||||
*) echo "bad --role: ${role} (task-review|fix-review|final-review)" >&2; exit 2 ;;
|
||||
esac
|
||||
|
||||
if [ $# -lt 3 ] || [ $# -gt 4 ]; then
|
||||
echo "usage: review-package [--role task-review|re-review|final-review] PLAN_FILE BASE HEAD [OUTFILE]" >&2
|
||||
echo "usage: review-package [--role task-review|fix-review|final-review] PLAN_FILE BASE HEAD [OUTFILE]" >&2
|
||||
exit 2
|
||||
fi
|
||||
|
||||
|
||||
Reference in New Issue
Block a user