From 99f9f00869e3e774e9abe9a29ccfccb8957ec5da Mon Sep 17 00:00:00 2001
From: Jesse Vincent <jesse@fsck.com>
Date: Thu, 13 Aug 2026 00:29:29 +0000
Subject: [PATCH] fix(sdd): reject empty or non-descendant BASE..HEAD ranges in
 review-package
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

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
---
 .../scripts/review-package                    |  5 ++++
 tests/claude-code/test-sdd-workspace.sh       | 24 +++++++++++++++++++
 2 files changed, 29 insertions(+)

diff --git a/skills/subagent-driven-development/scripts/review-package b/skills/subagent-driven-development/scripts/review-package
index 31852e2a..7af8dbe4 100755
--- a/skills/subagent-driven-development/scripts/review-package
+++ b/skills/subagent-driven-development/scripts/review-package
@@ -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
diff --git a/tests/claude-code/test-sdd-workspace.sh b/tests/claude-code/test-sdd-workspace.sh
index 84172301..681cba08 100755
--- a/tests/claude-code/test-sdd-workspace.sh
+++ b/tests/claude-code/test-sdd-workspace.sh
@@ -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 )