From 6834a81a908fe0e3003d9c97808de1345f22e4d3 Mon Sep 17 00:00:00 2001 From: Shi Qi Date: Wed, 4 Nov 2026 22:39:46 -0800 Subject: [PATCH 1/2] fix: protect validated branch tips during cleanup --- lib/commands/clean.sh | 69 +++++++++++- tests/cmd_clean.bats | 252 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 320 insertions(+), 1 deletion(-) diff --git a/lib/commands/clean.sh b/lib/commands/clean.sh index 0f8d416..090c7ee 100644 --- a/lib/commands/clean.sh +++ b/lib/commands/clean.sh @@ -139,6 +139,61 @@ _clean_branch_matches_pr_state() { return 1 } +# Delete only the exact tip authorized by the provider. A separate tip read +# followed by `git branch -D` would still race a concurrent branch update. +# Usage: _clean_delete_validated_branch repo_root branch branch_tip +_clean_delete_validated_branch() { + local repo_root="$1" branch="$2" branch_tip="$3" + local branch_ref="refs/heads/$branch" worktrees delete_output + + # update-ref does not provide git branch's checked-out branch protection. + if ! worktrees=$(git -C "$repo_root" worktree list --porcelain 2>/dev/null); then + log_warn "Kept branch $branch (could not verify remaining worktrees)" + return 1 + fi + if printf '%s\n' "$worktrees" | grep -Fxq "branch $branch_ref"; then + log_warn "Kept branch $branch (still checked out in another worktree)" + return 1 + fi + + # Rebase/bisect can detach HEAD while still reserving the original branch. + # Preserve git branch's protection for those operations as well. + local common_dir state_file state_branch + if ! common_dir=$(git -C "$repo_root" rev-parse --git-common-dir 2>/dev/null) || \ + [ -z "$common_dir" ]; then + log_warn "Kept branch $branch (could not verify rebase/bisect state)" + return 1 + fi + case "$common_dir" in /*) ;; *) common_dir="$repo_root/$common_dir" ;; esac + for state_file in "$common_dir"/rebase-merge/head-name \ + "$common_dir"/rebase-apply/head-name "$common_dir"/BISECT_START \ + "$common_dir"/worktrees/*/rebase-merge/head-name \ + "$common_dir"/worktrees/*/rebase-apply/head-name "$common_dir"/worktrees/*/BISECT_START; do + [ -e "$state_file" ] || continue + if ! state_branch=$(cat "$state_file" 2>/dev/null) || [ -z "$state_branch" ]; then + log_warn "Kept branch $branch (could not verify rebase/bisect state)" + return 1 + fi + if [ "$state_branch" = "$branch_ref" ] || [ "$state_branch" = "$branch" ]; then + log_warn "Kept branch $branch (rebase or bisect in progress)" + return 1 + fi + done + + # --no-deref also prevents deleting a different branch if this ref becomes + # symbolic between validation and deletion. Never retry without the old tip. + if ! delete_output=$(git -C "$repo_root" update-ref --no-deref -d \ + "$branch_ref" "$branch_tip" 2>&1); then + log_warn "Kept branch $branch (tip changed or deletion failed): $delete_output" + return 1 + fi + + # Match git branch's metadata cleanup only after successful ref deletion. + # A missing section is normal; metadata cleanup must not abort postRemove. + git -C "$repo_root" config --local --remove-section "branch.$branch" 2>/dev/null || true + log_info "Branch deleted: $branch" +} + # Remove worktrees whose PRs/MRs match requested cleanup states (handles squash merges) # Usage: _clean_prs repo_root base_dir prefix yes_mode dry_run [force] [active_worktree_path] [target_ref] [merged_mode] [closed_mode] _clean_prs() { @@ -211,8 +266,20 @@ _clean_prs() { continue fi + # Hooks (or concurrent Git commands) may change the checked-out + # branch or its tip. --force must not override provider validation. + local current_ref current_tip + if ! current_ref=$(git -C "$dir" symbolic-ref --quiet HEAD 2>/dev/null) || \ + [ "$current_ref" != "refs/heads/$branch" ] || \ + ! current_tip=$(git -C "$dir" rev-parse --verify "refs/heads/$branch^{commit}" 2>/dev/null) || \ + [ "$current_tip" != "$branch_tip" ]; then + log_warn "Skipping $branch (branch identity or tip changed, or could not be verified after preRemove)" + skipped=$((skipped + 1)) + continue + fi + if remove_worktree "$dir" "$force"; then - git branch -d "$branch" 2>/dev/null || git branch -D "$branch" 2>/dev/null || true + _clean_delete_validated_branch "$repo_root" "$branch" "$branch_tip" || true removed=$((removed + 1)) if ! run_hooks postRemove \ diff --git a/tests/cmd_clean.bats b/tests/cmd_clean.bats index bc7d87e..90c265b 100644 --- a/tests/cmd_clean.bats +++ b/tests/cmd_clean.bats @@ -238,6 +238,258 @@ teardown() { [ ! -d "$TEST_WORKTREES_DIR/merged-tip" ] } +# Keep Git and hook execution real; only stub the hosting service response. +setup_validated_tip_candidate() { + create_test_worktree "validated-tip" + git -C "$TEST_WT_PATH" commit --allow-empty -m "PR tip" --quiet + CLEAN_VALIDATED_TIP=$(git -C "$TEST_WT_PATH" rev-parse HEAD) + git config branch.validated-tip.description "Keep until branch deletion succeeds" + git config gtr.hook.postRemove 'printf "post\n" >> "$REPO_ROOT/hook-order"' + _clean_detect_provider() { printf "github"; } + ensure_provider_cli() { return 0; } + check_branch_merged() { [ "$2" = validated-tip ] && [ "$4" = "$CLEAN_VALIDATED_TIP" ]; } + check_branch_closed() { check_branch_merged "$@"; } +} + +assert_changed_candidate_is_preserved() { + [ "$status" -eq 0 ] + [ -d "$TEST_WT_PATH" ] + [ "$(cat "$TEST_REPO/hook-order")" = pre ] + [[ "$output" == *"Skipping validated-tip"* ]] + [[ "$output" == *"after preRemove"* ]] + [[ "$output" == *"Removed: 0, Skipped: 1"* ]] +} + +assert_cleanup_preserves_hook_commit() { + setup_validated_tip_candidate + git config gtr.hook.preRemove \ + 'printf "pre\n" >> "$REPO_ROOT/hook-order"; git commit --allow-empty -m "New unmatched work" --quiet' + + run run_clean_with_errexit "$@" --yes + assert_changed_candidate_is_preserved + [ "$(git rev-parse refs/heads/validated-tip)" != "$CLEAN_VALIDATED_TIP" ] + [ -n "$(git config branch.validated-tip.description)" ] +} + +@test "cmd_clean preserves a branch advanced by preRemove" { + assert_cleanup_preserves_hook_commit --merged +} + +@test "cmd_clean --closed --force cannot bypass the post-hook tip check" { + assert_cleanup_preserves_hook_commit --closed --force +} + +@test "cmd_clean skips a hook checkout even when both branches have the same tip" { + setup_validated_tip_candidate + git branch other-tip "$CLEAN_VALIDATED_TIP" + git config gtr.hook.preRemove \ + 'printf "pre\n" >> "$REPO_ROOT/hook-order"; git checkout --quiet other-tip' + + run run_clean_with_errexit --merged --force --yes + assert_changed_candidate_is_preserved + [ "$(git -C "$TEST_WT_PATH" symbolic-ref HEAD)" = refs/heads/other-tip ] + [ "$(git rev-parse refs/heads/validated-tip)" = "$CLEAN_VALIDATED_TIP" ] +} + +@test "cmd_clean skips a hook that detaches HEAD at the validated tip" { + setup_validated_tip_candidate + git config gtr.hook.preRemove \ + 'printf "pre\n" >> "$REPO_ROOT/hook-order"; git checkout --quiet --detach HEAD' + + run run_clean_with_errexit --merged --force --yes + assert_changed_candidate_is_preserved + ! git -C "$TEST_WT_PATH" symbolic-ref --quiet HEAD + [ "$(git rev-parse refs/heads/validated-tip)" = "$CLEAN_VALIDATED_TIP" ] +} + +@test "cmd_clean skips a hook that renames the validated branch" { + setup_validated_tip_candidate + git config gtr.hook.preRemove \ + 'printf "pre\n" >> "$REPO_ROOT/hook-order"; git branch -m renamed-tip' + + run run_clean_with_errexit --closed --force --yes + assert_changed_candidate_is_preserved + [ "$(git -C "$TEST_WT_PATH" symbolic-ref HEAD)" = refs/heads/renamed-tip ] + [ "$(git rev-parse refs/heads/renamed-tip)" = "$CLEAN_VALIDATED_TIP" ] + [ -n "$(git config branch.renamed-tip.description)" ] +} + +@test "cmd_clean fails closed when post-hook branch state cannot be established" { + setup_validated_tip_candidate + git config gtr.hook.preRemove 'printf "pre\n" > "$REPO_ROOT/hook-order"' + git() { + if [ "$1" = -C ] && [ "$2" = "$TEST_WT_PATH" ] && \ + [ "$3" = "$CLEAN_FAIL_COMMAND" ] && [ -e "$TEST_REPO/hook-order" ]; then + printf "%s" "$CLEAN_LOOKUP_OUTPUT" + return "$CLEAN_LOOKUP_STATUS" + fi + command git "$@" + } + + local CLEAN_FAIL_COMMAND CLEAN_LOOKUP_OUTPUT CLEAN_LOOKUP_STATUS + for CLEAN_FAIL_COMMAND in symbolic-ref rev-parse; do + # Failed lookups must fail closed even if they print the expected value. + if [ "$CLEAN_FAIL_COMMAND" = symbolic-ref ]; then + CLEAN_LOOKUP_OUTPUT=refs/heads/validated-tip + else + CLEAN_LOOKUP_OUTPUT="$CLEAN_VALIDATED_TIP" + fi + CLEAN_LOOKUP_STATUS=1 + run run_clean_with_errexit --merged --force --yes + assert_changed_candidate_is_preserved + rm "$TEST_REPO/hook-order" + + CLEAN_LOOKUP_OUTPUT="" + CLEAN_LOOKUP_STATUS=0 + run run_clean_with_errexit --closed --force --yes + assert_changed_candidate_is_preserved + rm "$TEST_REPO/hook-order" + done + [ "$(command git rev-parse refs/heads/validated-tip)" = "$CLEAN_VALIDATED_TIP" ] +} + +@test "cmd_clean removes unchanged validated tips and metadata after hooks" { + local mode + for mode in --merged --closed; do + setup_validated_tip_candidate + git config branch.validated-tip.remote origin + git config branch.validated-tip.merge refs/heads/validated-tip + git config gtr.hook.preRemove \ + 'test -d "$WORKTREE_PATH" && printf "pre\n" > "$REPO_ROOT/hook-order"' + git config gtr.hook.postRemove \ + 'test ! -d "$WORKTREE_PATH" && ! git show-ref --verify --quiet "refs/heads/$BRANCH" && printf "post\n" >> "$REPO_ROOT/hook-order"' + + run run_clean_with_errexit "$mode" --yes + [ "$status" -eq 0 ] + [ ! -d "$TEST_WT_PATH" ] + ! git show-ref --verify --quiet refs/heads/validated-tip + ! git config --get-regexp '^branch\.validated-tip\.' + [ "$(cat "$TEST_REPO/hook-order")" = $'pre\npost' ] + [[ "$output" == *"Removed: 1, Skipped: 0"* ]] + [[ "$output" != *"Post-remove hook failed"* ]] + done +} + +assert_cleanup_preserves_racing_tip() { + setup_validated_tip_candidate + local new_tip + new_tip=$(printf "Concurrent work\n" | git commit-tree "$CLEAN_VALIDATED_TIP^{tree}" -p "$CLEAN_VALIDATED_TIP") + git config gtr.hook.preRemove 'printf "pre\n" > "$REPO_ROOT/hook-order"' + # Inject the update at the final deletion command, after all validation and + # worktree removal. Run the actual Git deletion to exercise its atomic check. + git() { + local args=" $* " + case "$args" in + *" update-ref "*" -d refs/heads/validated-tip "*|*" branch -d validated-tip "*|*" branch -D validated-tip "*) + command git -C "$TEST_REPO" update-ref refs/heads/validated-tip "$new_tip" + printf "raced\n" > "$TEST_REPO/race-injected" + ;; + esac + command git "$@" + } + + run run_clean_with_errexit "$@" --yes + [ "$status" -eq 0 ] + [ -e "$TEST_REPO/race-injected" ] + [ ! -d "$TEST_WT_PATH" ] + [ "$(command git rev-parse refs/heads/validated-tip)" = "$new_tip" ] + [ -n "$(command git config branch.validated-tip.description)" ] + [ "$(cat "$TEST_REPO/hook-order")" = $'pre\npost' ] + [[ "$output" == *"Kept branch validated-tip"* ]] +} + +@test "cmd_clean preserves an update racing the final branch deletion" { + assert_cleanup_preserves_racing_tip --merged +} + +@test "cmd_clean --closed --force cannot bypass the atomic tip check" { + assert_cleanup_preserves_racing_tip --closed --force +} + +@test "cmd_clean keeps a validated branch still checked out in another worktree" { + setup_validated_tip_candidate + git worktree add --quiet --force "$TEST_WORKTREES_DIR/still-active" validated-tip + cd "$TEST_WORKTREES_DIR/still-active" + + run run_clean_with_errexit --merged --force --yes + [ "$status" -eq 0 ] + [ ! -d "$TEST_WT_PATH" ] + [ -d "$TEST_WORKTREES_DIR/still-active" ] + [ "$(git rev-parse refs/heads/validated-tip)" = "$CLEAN_VALIDATED_TIP" ] + [ -n "$(git config branch.validated-tip.description)" ] + [ "$(cat "$TEST_REPO/hook-order")" = post ] + [[ "$output" == *"Kept branch validated-tip (still checked out"* ]] +} + +@test "cmd_clean keeps branch metadata if remaining worktrees cannot be checked" { + setup_validated_tip_candidate + git() { + if [ "$1" = -C ] && [ "$2" = "$TEST_REPO" ] && \ + [ "$3 $4" = "worktree list" ] && [ ! -d "$TEST_WT_PATH" ]; then + return 1 + fi + command git "$@" + } + + run run_clean_with_errexit --closed --force --yes + [ "$status" -eq 0 ] + [ ! -d "$TEST_WT_PATH" ] + [ "$(command git rev-parse refs/heads/validated-tip)" = "$CLEAN_VALIDATED_TIP" ] + [ -n "$(command git config branch.validated-tip.description)" ] + [ "$(cat "$TEST_REPO/hook-order")" = post ] + [[ "$output" == *"Kept branch validated-tip (could not verify remaining worktrees)"* ]] +} + +assert_cleanup_preserves_in_progress_branch() { + local state_file="$1" state_branch="$2" state_path + setup_validated_tip_candidate + git worktree add --quiet --detach "$TEST_WORKTREES_DIR/in-progress" "$CLEAN_VALIDATED_TIP" + # Model Git's branch reservation while HEAD is detached by rebase/bisect. + state_path=$(git -C "$TEST_WORKTREES_DIR/in-progress" rev-parse --git-path "$state_file") + mkdir -p "$(dirname "$state_path")" + printf '%s\n' "$state_branch" > "$state_path" + + run run_clean_with_errexit --merged --force --yes + [ "$status" -eq 0 ] + [ ! -d "$TEST_WT_PATH" ] + [ -d "$TEST_WORKTREES_DIR/in-progress" ] + [ "$(git rev-parse refs/heads/validated-tip)" = "$CLEAN_VALIDATED_TIP" ] + [ -n "$(git config branch.validated-tip.description)" ] + [[ "$output" == *"Kept branch validated-tip (rebase or bisect in progress)"* ]] +} + +@test "cmd_clean preserves branch protection during a merge-backend rebase" { + assert_cleanup_preserves_in_progress_branch rebase-merge/head-name refs/heads/validated-tip +} + +@test "cmd_clean preserves branch protection during an apply-backend rebase" { + assert_cleanup_preserves_in_progress_branch rebase-apply/head-name refs/heads/validated-tip +} + +@test "cmd_clean preserves branch protection during bisect" { + assert_cleanup_preserves_in_progress_branch BISECT_START validated-tip +} + +@test "cmd_clean never follows a symbolic branch ref during final deletion" { + setup_validated_tip_candidate + git branch other-tip "$CLEAN_VALIDATED_TIP" + git() { + case " $* " in + *" update-ref "*" -d refs/heads/validated-tip "*) + command git -C "$TEST_REPO" symbolic-ref refs/heads/validated-tip refs/heads/other-tip + printf "raced\n" > "$TEST_REPO/race-injected" + ;; + esac + command git "$@" + } + + run run_clean_with_errexit --merged --force --yes + [ "$status" -eq 0 ] + [ -e "$TEST_REPO/race-injected" ] + [ ! -d "$TEST_WT_PATH" ] + [ "$(command git rev-parse refs/heads/other-tip)" = "$CLEAN_VALIDATED_TIP" ] +} + # Fail only the candidate's tip lookup; all other Git operations remain real. setup_unresolved_tip_candidate() { create_test_worktree "unresolved-tip" From 882b765de8f75d5dd270dabb52b017e2fc480710 Mon Sep 17 00:00:00 2001 From: Shi Qi Date: Wed, 4 Nov 2026 22:57:34 -0800 Subject: [PATCH 2/2] test: make cleanup absence assertions fail reliably --- tests/cmd_clean.bats | 23 ++++++++++++++++++++--- 1 file changed, 20 insertions(+), 3 deletions(-) diff --git a/tests/cmd_clean.bats b/tests/cmd_clean.bats index 90c265b..5b76add 100644 --- a/tests/cmd_clean.bats +++ b/tests/cmd_clean.bats @@ -251,6 +251,8 @@ setup_validated_tip_candidate() { check_branch_closed() { check_branch_merged "$@"; } } +# Assert cleanup skipped a candidate after preRemove without removing its worktree +# or invoking postRemove. Reads BATS status/output and the fixture's hook log. assert_changed_candidate_is_preserved() { [ "$status" -eq 0 ] [ -d "$TEST_WT_PATH" ] @@ -260,6 +262,8 @@ assert_changed_candidate_is_preserved() { [[ "$output" == *"Removed: 0, Skipped: 1"* ]] } +# Run cleanup with the supplied mode/flags and verify that a preRemove commit +# preserves the worktree, updated branch, and branch metadata. assert_cleanup_preserves_hook_commit() { setup_validated_tip_candidate git config gtr.hook.preRemove \ @@ -298,7 +302,9 @@ assert_cleanup_preserves_hook_commit() { run run_clean_with_errexit --merged --force --yes assert_changed_candidate_is_preserved - ! git -C "$TEST_WT_PATH" symbolic-ref --quiet HEAD + if git -C "$TEST_WT_PATH" symbolic-ref --quiet HEAD >/dev/null; then + false + fi [ "$(git rev-parse refs/heads/validated-tip)" = "$CLEAN_VALIDATED_TIP" ] } @@ -317,6 +323,7 @@ assert_cleanup_preserves_hook_commit() { @test "cmd_clean fails closed when post-hook branch state cannot be established" { setup_validated_tip_candidate git config gtr.hook.preRemove 'printf "pre\n" > "$REPO_ROOT/hook-order"' + # Fail only the selected post-hook lookup; preserve all other Git behavior. git() { if [ "$1" = -C ] && [ "$2" = "$TEST_WT_PATH" ] && \ [ "$3" = "$CLEAN_FAIL_COMMAND" ] && [ -e "$TEST_REPO/hook-order" ]; then @@ -362,14 +369,20 @@ assert_cleanup_preserves_hook_commit() { run run_clean_with_errexit "$mode" --yes [ "$status" -eq 0 ] [ ! -d "$TEST_WT_PATH" ] - ! git show-ref --verify --quiet refs/heads/validated-tip - ! git config --get-regexp '^branch\.validated-tip\.' + if git show-ref --verify --quiet refs/heads/validated-tip; then + false + fi + if git config --get-regexp '^branch\.validated-tip\.' >/dev/null; then + false + fi [ "$(cat "$TEST_REPO/hook-order")" = $'pre\npost' ] [[ "$output" == *"Removed: 1, Skipped: 0"* ]] [[ "$output" != *"Post-remove hook failed"* ]] done } +# Inject an update immediately before branch deletion, then assert cleanup keeps +# the new tip and metadata for the supplied cleanup mode/flags. assert_cleanup_preserves_racing_tip() { setup_validated_tip_candidate local new_tip @@ -423,6 +436,7 @@ assert_cleanup_preserves_racing_tip() { @test "cmd_clean keeps branch metadata if remaining worktrees cannot be checked" { setup_validated_tip_candidate + # Fail worktree enumeration only after the candidate directory is removed. git() { if [ "$1" = -C ] && [ "$2" = "$TEST_REPO" ] && \ [ "$3 $4" = "worktree list" ] && [ ! -d "$TEST_WT_PATH" ]; then @@ -440,6 +454,8 @@ assert_cleanup_preserves_racing_tip() { [[ "$output" == *"Kept branch validated-tip (could not verify remaining worktrees)"* ]] } +# Model the supplied rebase/bisect state file and branch value in another worktree, +# then verify cleanup preserves the reserved branch and its metadata. assert_cleanup_preserves_in_progress_branch() { local state_file="$1" state_branch="$2" state_path setup_validated_tip_candidate @@ -473,6 +489,7 @@ assert_cleanup_preserves_in_progress_branch() { @test "cmd_clean never follows a symbolic branch ref during final deletion" { setup_validated_tip_candidate git branch other-tip "$CLEAN_VALIDATED_TIP" + # Replace the branch with a symbolic ref at the final deletion command. git() { case " $* " in *" update-ref "*" -d refs/heads/validated-tip "*)