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 "$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
out=$4
else
+2 -3
View File
@@ -15,9 +15,8 @@ run_claude() {
cmd+=(--allowed-tools="$allowed_tools")
fi
# Run Claude in headless mode with timeout. Redirect stdin from
# /dev/null so the CLI can't block waiting for input and hang the suite.
if timeout "$timeout" "${cmd[@]}" > "$output_file" 2>&1 < /dev/null; then
# Run Claude in headless mode with timeout
if timeout "$timeout" "${cmd[@]}" > "$output_file" 2>&1; then
cat "$output_file"
rm -f "$output_file"
return 0
+24
View File
@@ -165,6 +165,30 @@ PLAN
echo " got: $rp_explicit"
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 ---
local wt="$TEST_ROOT/wt"
( cd "$repo" && git worktree add -q "$wt" -b wt-feature )
@@ -23,7 +23,7 @@ echo "========================================"
echo ""
echo "This test executes a real plan using the skill and verifies:"
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 " 4. Spec compliance review before code quality"
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:
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
4. Run spec compliance review before code quality review
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:
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
4. Run spec compliance review before code quality review
5. Use review loops when issues are found
@@ -164,7 +164,7 @@ PLUGIN_DIR=$(cd "$SCRIPT_DIR/../.." && pwd)
# other concurrent claude sessions.
echo "Running Claude (plugin-dir: $PLUGIN_DIR, cwd: $TEST_PROJECT)..."
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 "EXECUTION FAILED (exit code: $?)"
@@ -316,7 +316,7 @@ if [ $FAILED -eq 0 ]; then
echo ""
echo "The subagent-driven-development skill correctly:"
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 " ✓ Runs spec compliance before code quality"
echo " ✓ Spec reviewer verifies independently"
@@ -4,7 +4,7 @@
#
# No drill coverage: this test asks the agent to *describe* SDD (string-
# 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),
# not description-recall. Kept by design.
set -euo pipefail
@@ -83,7 +83,7 @@ else
exit 1
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
else
exit 1
@@ -133,16 +133,16 @@ echo ""
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:
Controller provides: <brief file or whole plan file>
Implementer must read whole plan file: <yes or no>" "$CLAUDE_PROMPT_TIMEOUT")
Controller provides: <directly or by file>
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
else
exit 1
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
else
exit 1