mirror of
https://github.com/obra/superpowers.git
synced 2026-06-12 13:49:05 +08:00
Add review-package script; close fix-dispatch test gap
scripts/review-package generates the reviewer's input deterministically: commit list, stat summary, and net diff with -U10 context, written to a file from an explicit BASE. Live runs showed controllers improvising 'git diff HEAD~1..HEAD', which silently truncates multi-commit tasks, and svelte's five fix dispatches shipped without re-running any tests — fix dispatches now explicitly carry the implementer's re-run-and-report contract.
This commit is contained in:
@@ -112,7 +112,7 @@ single-file mechanical fixes.
|
|||||||
|
|
||||||
Implementer subagents report one of four statuses. Handle each appropriately:
|
Implementer subagents report one of four statuses. Handle each appropriately:
|
||||||
|
|
||||||
**DONE:** Write the task's diff to a file (`git diff BASE..HEAD > /tmp/sdd-task-N.diff`), then dispatch the task reviewer with that path.
|
**DONE:** Generate the review package (`scripts/review-package BASE HEAD /tmp/sdd-task-N.diff`, from this skill's directory; BASE is the commit you recorded before dispatching the implementer — never `HEAD~1`, which silently drops all but the last commit of a multi-commit task), then dispatch the task reviewer with that path.
|
||||||
|
|
||||||
**DONE_WITH_CONCERNS:** The implementer completed the work but flagged doubts. Read the concerns before proceeding. If the concerns are about correctness or scope, address them before review. If they're observations (e.g., "this file is getting large"), note them and proceed to review.
|
**DONE_WITH_CONCERNS:** The implementer completed the work but flagged doubts. Read the concerns before proceeding. If the concerns are about correctness or scope, address them before review. If they're observations (e.g., "this file is getting large"), note them and proceed to review.
|
||||||
|
|
||||||
@@ -153,15 +153,22 @@ final whole-branch review. When you fill a reviewer template:
|
|||||||
- Include the spec/design's global constraints that bind the task (version
|
- Include the spec/design's global constraints that bind the task (version
|
||||||
floors, naming and copy rules, platform requirements) in the requirements
|
floors, naming and copy rules, platform requirements) in the requirements
|
||||||
you paste — a reviewer can only enforce what you hand them.
|
you paste — a reviewer can only enforce what you hand them.
|
||||||
- Hand the reviewer its diff as a file: run
|
- Hand the reviewer its diff as a file: run this skill's
|
||||||
`git diff BASE..HEAD > /tmp/sdd-task-N.diff` (redirected, so the diff
|
`scripts/review-package BASE HEAD /tmp/sdd-task-N.diff` (or, without
|
||||||
never enters your own context) and put that path in the prompt. The
|
bash: `git log --oneline`, `git diff --stat`, and `git diff -U10` for
|
||||||
reviewer then sees the whole change in one Read call instead of
|
the range, redirected to the file). The output never enters your own
|
||||||
re-deriving it with git commands.
|
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.
|
||||||
- Dispatch fix subagents for Critical and Important findings. Record Minor
|
- Dispatch fix subagents for Critical and Important findings. Record Minor
|
||||||
findings and move on — then paste the accumulated Minor findings into the
|
findings and move on — then paste the accumulated Minor findings into the
|
||||||
final whole-branch review dispatch so it can triage which must be fixed
|
final whole-branch review dispatch so it can triage which must be fixed
|
||||||
before merge. A roll-up nobody reads is a silent discard.
|
before merge. A roll-up nobody reads is a silent discard.
|
||||||
|
- Every fix dispatch carries the implementer contract: the fix subagent
|
||||||
|
re-runs the tests covering its change and reports the results. A fix
|
||||||
|
report without test evidence is incomplete — do not re-review on top of
|
||||||
|
it.
|
||||||
|
|
||||||
## Prompt Templates
|
## Prompt Templates
|
||||||
|
|
||||||
@@ -283,9 +290,9 @@ Done!
|
|||||||
- Tell a reviewer what not to flag, or pre-rate a finding's severity in the
|
- 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
|
dispatch prompt ("treat it as Minor at most") — the plan's example code is
|
||||||
a starting point, not evidence that its weaknesses were chosen
|
a starting point, not evidence that its weaknesses were chosen
|
||||||
- Dispatch a task reviewer without a diff file — run
|
- Dispatch a task reviewer without a diff file — generate it first
|
||||||
`git diff BASE..HEAD > /tmp/sdd-task-N.diff` first and name that path in
|
(`scripts/review-package BASE HEAD /tmp/sdd-task-N.diff`) and name that
|
||||||
the prompt
|
path in the prompt
|
||||||
- Move to next task while the review has open Critical/Important issues
|
- Move to next task while the review has open Critical/Important issues
|
||||||
|
|
||||||
**If subagent asks questions:**
|
**If subagent asks questions:**
|
||||||
|
|||||||
37
skills/subagent-driven-development/scripts/review-package
Executable file
37
skills/subagent-driven-development/scripts/review-package
Executable file
@@ -0,0 +1,37 @@
|
|||||||
|
#!/usr/bin/env bash
|
||||||
|
# Generate a task review package: commit list, stat summary, and the net
|
||||||
|
# diff with extended context, written to a file the reviewer reads in one
|
||||||
|
# call. Using the recorded per-task BASE (not HEAD~1) keeps multi-commit
|
||||||
|
# tasks intact.
|
||||||
|
#
|
||||||
|
# Usage: review-package BASE HEAD OUTFILE
|
||||||
|
# Example: review-package a1b2c3d HEAD /tmp/sdd-task-3.diff
|
||||||
|
set -euo pipefail
|
||||||
|
|
||||||
|
if [ $# -ne 3 ]; then
|
||||||
|
echo "usage: review-package BASE HEAD OUTFILE" >&2
|
||||||
|
exit 2
|
||||||
|
fi
|
||||||
|
|
||||||
|
base=$1
|
||||||
|
head=$2
|
||||||
|
out=$3
|
||||||
|
|
||||||
|
git rev-parse --verify --quiet "$base" >/dev/null || { echo "bad BASE: $base" >&2; exit 2; }
|
||||||
|
git rev-parse --verify --quiet "$head" >/dev/null || { echo "bad HEAD: $head" >&2; exit 2; }
|
||||||
|
|
||||||
|
{
|
||||||
|
echo "# Review package: ${base}..${head}"
|
||||||
|
echo
|
||||||
|
echo "## Commits"
|
||||||
|
git log --oneline "${base}..${head}"
|
||||||
|
echo
|
||||||
|
echo "## Files changed"
|
||||||
|
git diff --stat "${base}..${head}"
|
||||||
|
echo
|
||||||
|
echo "## Diff"
|
||||||
|
git diff -U10 "${base}..${head}"
|
||||||
|
} > "$out"
|
||||||
|
|
||||||
|
commits=$(git rev-list --count "${base}..${head}")
|
||||||
|
echo "wrote ${out}: ${commits} commit(s), $(wc -c < "$out" | tr -d ' ') bytes"
|
||||||
@@ -30,9 +30,10 @@ Subagent (general-purpose):
|
|||||||
**Head:** [HEAD_SHA]
|
**Head:** [HEAD_SHA]
|
||||||
**Diff file:** [DIFF_FILE]
|
**Diff file:** [DIFF_FILE]
|
||||||
|
|
||||||
Read the diff file once — that single Read is your view of the change.
|
Read the diff file once — it contains the commit list, a stat summary,
|
||||||
Do not re-run git commands or re-read the files it already shows. If
|
and the full diff with surrounding context, and it is your view of the
|
||||||
the diff file is missing, fetch the diff yourself:
|
change. Do not re-run git commands or re-read the files it already
|
||||||
|
shows. If the diff file is missing, fetch the diff yourself:
|
||||||
`git diff --stat [BASE_SHA]..[HEAD_SHA]` and `git diff [BASE_SHA]..[HEAD_SHA]`.
|
`git diff --stat [BASE_SHA]..[HEAD_SHA]` and `git diff [BASE_SHA]..[HEAD_SHA]`.
|
||||||
Only read files in this diff. Do not crawl the broader codebase. Inspect
|
Only read files in this diff. Do not crawl the broader codebase. Inspect
|
||||||
code outside the diff only to evaluate a concrete risk you can name — and
|
code outside the diff only to evaluate a concrete risk you can name — and
|
||||||
@@ -152,9 +153,9 @@ Subagent (general-purpose):
|
|||||||
- `[DESCRIPTION]` — what the implementer reports they built
|
- `[DESCRIPTION]` — what the implementer reports they built
|
||||||
- `[BASE_SHA]` — commit before this task
|
- `[BASE_SHA]` — commit before this task
|
||||||
- `[HEAD_SHA]` — current commit
|
- `[HEAD_SHA]` — current commit
|
||||||
- `[DIFF_FILE]` — REQUIRED: the path the controller wrote the diff to
|
- `[DIFF_FILE]` — REQUIRED: the path the controller wrote the review
|
||||||
(`git diff BASE..HEAD > /tmp/sdd-task-N.diff`, redirected so it never
|
package to (`scripts/review-package BASE HEAD /tmp/sdd-task-N.diff`,
|
||||||
enters the controller's context)
|
redirected so it never enters the controller's context)
|
||||||
|
|
||||||
**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
|
||||||
|
|||||||
Reference in New Issue
Block a user