fix(sdd): reject empty or non-descendant BASE..HEAD ranges in review-package - #2136
Open
obra wants to merge 1 commit into
Open
fix(sdd): reject empty or non-descendant BASE..HEAD ranges in review-package#2136obra wants to merge 1 commit into
obra wants to merge 1 commit into
Conversation
…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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
review-packagebuilds a review package from whatever BASE/HEAD range it's handed. When an SDD subagent commits to the wrong branch (#2050's reported failure: an implementer committed to main instead of its worktree), the range is either empty — the reviewer sees nothing and approves 'clean' work — or contains commits that aren't descendants of the expected base. Both silently corrupt the review chain SDD's correctness depends on.Fix
Five lines after the existing BASE/HEAD validation, in the script's own style:
git merge-base --is-ancestor "$base" "$head"— reject with 'HEAD is not a descendant of BASE', exit 3git rev-list --count— reject an empty range, exit 3Exit 3 distinguishes range corruption from usage errors (exit 2). This is the mechanical-enforcement shape requested on #1588; prompt-side wording changes are deliberately out of scope (they need evals).
TDD evidence
Two new assertions in
tests/claude-code/test-sdd-workspace.sh(divergent BASE built viacommit-tree, and BASE==HEAD). RED against the unmodified script (both cases: exit 0, no stderr), GREEN with the guards; all 15 suite assertions pass.Note: this touches
review-packagenear PR #2134's hunk — whichever lands second gets a trivial rebase.Reported by @conradstorz in #2050; guard shape credited to closed PR #2082 (@stantheman0128). Fixes #2050.
Who is submitting
Claude Fable 5 on Claude Code 2.1.228 (implementation subagent + controller review), working the triage build queue directed by @obra, who reviews the diff.
@arittr @ada-sen — review requested.