Compare commits

..

1 Commits

Author SHA1 Message Date
Jesse Vincent 99f9f00869 fix(sdd): reject empty or non-descendant BASE..HEAD ranges in review-package
When an SDD implementer commits to the wrong branch (#2050), the
BASE..HEAD range handed to review-package is either empty or not rooted
at BASE. Both cases previously produced a review package silently —
an empty one lets the reviewer approve "clean" work that isn't there.

Add two mechanical guards after BASE/HEAD validation, exiting 3 (vs 2
for usage errors) so callers can distinguish range problems:

- git merge-base --is-ancestor BASE HEAD, else "HEAD is not a
  descendant of BASE"
- git rev-list --count BASE..HEAD > 0, else "empty commit range"

Guard shape credits the analysis in closed PR #2082 by @stantheman0128.

Fixes #2050
2026-08-13 00:29:29 +00:00
5 changed files with 42 additions and 14 deletions
@@ -22,6 +22,11 @@ head=$3
git rev-parse --verify --quiet "$base" >/dev/null || { echo "bad BASE: $base" >&2; exit 2; } 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; } git rev-parse --verify --quiet "$head" >/dev/null || { echo "bad HEAD: $head" >&2; exit 2; }
# Range guards (exit 3): a wrong-branch HEAD yields a range that is empty or
# not rooted at BASE; either would silently produce a bogus review package.
git merge-base --is-ancestor "$base" "$head" || { echo "HEAD is not a descendant of BASE: ${base}..${head}" >&2; exit 3; }
[ "$(git rev-list --count "${base}..${head}")" -gt 0 ] || { echo "empty commit range: ${base}..${head}" >&2; exit 3; }
if [ $# -eq 4 ]; then if [ $# -eq 4 ]; then
out=$4 out=$4
else else
+2 -3
View File
@@ -15,9 +15,8 @@ run_claude() {
cmd+=(--allowed-tools="$allowed_tools") cmd+=(--allowed-tools="$allowed_tools")
fi fi
# Run Claude in headless mode with timeout. Redirect stdin from # Run Claude in headless mode with timeout
# /dev/null so the CLI can't block waiting for input and hang the suite. if timeout "$timeout" "${cmd[@]}" > "$output_file" 2>&1; then
if timeout "$timeout" "${cmd[@]}" > "$output_file" 2>&1 < /dev/null; then
cat "$output_file" cat "$output_file"
rm -f "$output_file" rm -f "$output_file"
return 0 return 0
+24
View File
@@ -165,6 +165,30 @@ PLAN
echo " got: $rp_explicit" echo " got: $rp_explicit"
fi fi
# --- range guards: BASE must be an ancestor of HEAD, range must be non-empty ---
local divergent
divergent="$(cd "$repo" && git "${git_id[@]}" commit-tree 'HEAD~1^{tree}' -p 'HEAD~1' -m divergent)"
rc=0
local guard_err
guard_err="$(cd "$repo" && "$SDD_SCRIPTS/review-package" plan-a.md "$divergent" HEAD 2>&1 >/dev/null)" || rc=$?
if [[ "$rc" -eq 3 && "$guard_err" == *"not a descendant"* ]]; then
pass "review-package rejects a BASE that is not an ancestor of HEAD with exit 3"
else
fail "review-package rejects a BASE that is not an ancestor of HEAD with exit 3"
echo " exit: $rc"
echo " stderr: $guard_err"
fi
rc=0
guard_err="$(cd "$repo" && "$SDD_SCRIPTS/review-package" plan-a.md HEAD HEAD 2>&1 >/dev/null)" || rc=$?
if [[ "$rc" -eq 3 && "$guard_err" == *"empty commit range"* ]]; then
pass "review-package rejects an empty BASE..HEAD range with exit 3"
else
fail "review-package rejects an empty BASE..HEAD range with exit 3"
echo " exit: $rc"
echo " stderr: $guard_err"
fi
# --- Worktree isolation: a linked worktree resolves its own workspace --- # --- Worktree isolation: a linked worktree resolves its own workspace ---
local wt="$TEST_ROOT/wt" local wt="$TEST_ROOT/wt"
( cd "$repo" && git worktree add -q "$wt" -b wt-feature ) ( cd "$repo" && git worktree add -q "$wt" -b wt-feature )
@@ -23,7 +23,7 @@ echo "========================================"
echo "" echo ""
echo "This test executes a real plan using the skill and verifies:" echo "This test executes a real plan using the skill and verifies:"
echo " 1. Plan is read once (not per task)" echo " 1. Plan is read once (not per task)"
echo " 2. Task requirements routed to subagents via brief files" echo " 2. Full task text provided to subagents"
echo " 3. Subagents perform self-review" echo " 3. Subagents perform self-review"
echo " 4. Spec compliance review before code quality" echo " 4. Spec compliance review before code quality"
echo " 5. Review loops when issues found" echo " 5. Review loops when issues found"
@@ -136,7 +136,7 @@ I want you to execute the implementation plan at docs/superpowers/plans/implemen
IMPORTANT: Follow the skill exactly. I will be verifying that you: IMPORTANT: Follow the skill exactly. I will be verifying that you:
1. Read the plan once at the beginning 1. Read the plan once at the beginning
2. Route each task's requirements to subagents via a task brief file (don't make them read the whole plan) 2. Provide full task text to subagents (don't make them read files)
3. Ensure subagents do self-review before reporting 3. Ensure subagents do self-review before reporting
4. Run spec compliance review before code quality review 4. Run spec compliance review before code quality review
5. Use review loops when issues are found 5. Use review loops when issues are found
@@ -150,7 +150,7 @@ PROMPT="Execute the implementation plan at docs/superpowers/plans/implementation
IMPORTANT: Follow the skill exactly. I will be verifying that you: IMPORTANT: Follow the skill exactly. I will be verifying that you:
1. Read the plan once at the beginning 1. Read the plan once at the beginning
2. Route each task's requirements to subagents via a task brief file (don't make them read the whole plan) 2. Provide full task text to subagents (don't make them read files)
3. Ensure subagents do self-review before reporting 3. Ensure subagents do self-review before reporting
4. Run spec compliance review before code quality review 4. Run spec compliance review before code quality review
5. Use review loops when issues are found 5. Use review loops when issues are found
@@ -164,7 +164,7 @@ PLUGIN_DIR=$(cd "$SCRIPT_DIR/../.." && pwd)
# other concurrent claude sessions. # other concurrent claude sessions.
echo "Running Claude (plugin-dir: $PLUGIN_DIR, cwd: $TEST_PROJECT)..." echo "Running Claude (plugin-dir: $PLUGIN_DIR, cwd: $TEST_PROJECT)..."
echo "================================================================================" echo "================================================================================"
cd "$TEST_PROJECT" && timeout 1800 claude -p "$PROMPT" --plugin-dir "$PLUGIN_DIR" --allowed-tools=all --permission-mode bypassPermissions < /dev/null 2>&1 | tee "$OUTPUT_FILE" || { cd "$TEST_PROJECT" && timeout 1800 claude -p "$PROMPT" --plugin-dir "$PLUGIN_DIR" --allowed-tools=all --permission-mode bypassPermissions 2>&1 | tee "$OUTPUT_FILE" || {
echo "" echo ""
echo "================================================================================" echo "================================================================================"
echo "EXECUTION FAILED (exit code: $?)" echo "EXECUTION FAILED (exit code: $?)"
@@ -316,7 +316,7 @@ if [ $FAILED -eq 0 ]; then
echo "" echo ""
echo "The subagent-driven-development skill correctly:" echo "The subagent-driven-development skill correctly:"
echo " ✓ Reads plan once at start" echo " ✓ Reads plan once at start"
echo " ✓ Routes task requirements via brief files" echo " ✓ Provides full task text to subagents"
echo " ✓ Enforces self-review" echo " ✓ Enforces self-review"
echo " ✓ Runs spec compliance before code quality" echo " ✓ Runs spec compliance before code quality"
echo " ✓ Spec reviewer verifies independently" echo " ✓ Spec reviewer verifies independently"
@@ -4,7 +4,7 @@
# #
# No drill coverage: this test asks the agent to *describe* SDD (string- # No drill coverage: this test asks the agent to *describe* SDD (string-
# matches its verbal explanation against expected keywords like # matches its verbal explanation against expected keywords like
# "self-review", "skeptical", "worktree", "setup", "loop"). Drill scenarios # "self-review", "skeptical", "worktree", "Step 1", "loop"). Drill scenarios
# test behavior (real subagent dispatch, plan-following, review loops), # test behavior (real subagent dispatch, plan-following, review loops),
# not description-recall. Kept by design. # not description-recall. Kept by design.
set -euo pipefail set -euo pipefail
@@ -83,7 +83,7 @@ else
exit 1 exit 1
fi fi
if assert_contains "$output" "beginning\|start\|setup\|before.*dispatch\|before.*task" "Read at beginning"; then if assert_contains "$output" "Step 1\|beginning\|start\|Load Plan" "Read at beginning"; then
: # pass : # pass
else else
exit 1 exit 1
@@ -133,16 +133,16 @@ echo ""
echo "Test 7: Task context provision..." echo "Test 7: Task context provision..."
output=$(run_claude "In subagent-driven-development, how does the controller provide task information to the implementer subagent? Answer using exactly this structure: output=$(run_claude "In subagent-driven-development, how does the controller provide task information to the implementer subagent? Answer using exactly this structure:
Controller provides: <brief file or whole plan file> Controller provides: <directly or by file>
Implementer must read whole plan file: <yes or no>" "$CLAUDE_PROMPT_TIMEOUT") Implementer must read plan file: <yes or no>" "$CLAUDE_PROMPT_TIMEOUT")
if assert_contains "$output" "task-brief\|brief file\|brief.*path\|Controller provides:.*brief" "Provides task brief file"; then if assert_contains "$output" "provide.*directly\|full.*text\|paste\|include.*prompt" "Provides text directly"; then
: # pass : # pass
else else
exit 1 exit 1
fi fi
if assert_contains "$output" "Implementer must read whole plan file:.*no" "Doesn't make subagent read whole plan"; then if assert_contains "$output" "Implementer must read plan file:.*no" "Doesn't make subagent read file"; then
: # pass : # pass
else else
exit 1 exit 1