diff --git a/skills/subagent-driven-development/SKILL.md b/skills/subagent-driven-development/SKILL.md index 468a71a2..35095194 100644 --- a/skills/subagent-driven-development/SKILL.md +++ b/skills/subagent-driven-development/SKILL.md @@ -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. diff --git a/skills/subagent-driven-development/re-review-prompt.md b/skills/subagent-driven-development/fix-review-prompt.md similarity index 85% rename from skills/subagent-driven-development/re-review-prompt.md rename to skills/subagent-driven-development/fix-review-prompt.md index 18b0fb8a..e5c88b84 100644 --- a/skills/subagent-driven-development/re-review-prompt.md +++ b/skills/subagent-driven-development/fix-review-prompt.md @@ -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. diff --git a/skills/subagent-driven-development/scripts/review-package b/skills/subagent-driven-development/scripts/review-package index b988d424..b45dc255 100755 --- a/skills/subagent-driven-development/scripts/review-package +++ b/skills/subagent-driven-development/scripts/review-package @@ -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: /.superpowers/sdd//review-...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 diff --git a/tests/claude-code/test-sdd-workspace.sh b/tests/claude-code/test-sdd-workspace.sh index 0e322140..406638ab 100755 --- a/tests/claude-code/test-sdd-workspace.sh +++ b/tests/claude-code/test-sdd-workspace.sh @@ -184,13 +184,13 @@ PLAN echo " got: $rp_hint" fi - local rp_rereview - rp_rereview="$(cd "$repo" && env -u CLAUDECODE "$SDD_SCRIPTS/review-package" --role re-review plan-a.md HEAD~1 HEAD)" - if [[ "$rp_rereview" == *"dispatch (spawn_agent): fork_turns=none model=gpt-5.6-terra reasoning_effort=medium"* ]]; then - pass "review-package --role re-review relays the medium-effort hint" + local rp_fixreview + rp_fixreview="$(cd "$repo" && env -u CLAUDECODE "$SDD_SCRIPTS/review-package" --role fix-review plan-a.md HEAD~1 HEAD)" + if [[ "$rp_fixreview" == *"dispatch (spawn_agent): fork_turns=none model=gpt-5.6-terra reasoning_effort=medium"* ]]; then + pass "review-package --role fix-review relays the medium-effort hint" else - fail "review-package --role re-review relays the medium-effort hint" - echo " got: $rp_rereview" + fail "review-package --role fix-review relays the medium-effort hint" + echo " got: $rp_fixreview" fi local rp_final