Stop treating gh API failures as negative answers (#53)

Phlogistique · web-flow · commit 066613a47e5c · 2026-07-03T14:26:46.000+02:00
Two spots swallowed a failed PR listing (gh api .../pulls) and acted on the empty answer (each verified destructive against the pre-fix script with the new test scenarios): - has_sibling_conflicts: a failure read as "no siblings", so the resume deleted the kept parent branch while another conflicted PR may still need it for its resolution. - The list_child_prs callers in main(): the process substitutions consuming it drop its exit status, so a failure read as "no children" and the run deleted the merged branch under the children it never saw, without updating them. has_sibling_conflicts now dies on the failed listing; the list_child_prs callers capture its output and die on failure. https://claude.ai/code/session_01STkeSJ7cLrmrNn4aTDYkwH
diff --git a/tests/mock_gh.sh b/tests/mock_gh.sh
@@ -4,6 +4,10 @@
 # Only direct children are queried now (no recursive updates of indirect children).
 
 if [[ "$1" == "api" && "$2" == repos/*"/pulls?base="* ]]; then
+    if [[ "${MOCK_PR_LIST_FAIL:-}" == 1 ]]; then
+        echo "mock gh: pr list API down" >&2
+        exit 1
+    fi
     # Open PRs based on a branch (already --jq filtered to "<number> <head>").
     base="${2#*pulls\?base=}"
     base="${base%%&*}"
diff --git a/tests/test_conflict_resolution_resume.sh b/tests/test_conflict_resolution_resume.sh
@@ -23,6 +23,7 @@ ok() { echo "✅ $1"; PASS=$((PASS+1)); }
 #   MOCK_LABELS         newline-separated labels returned by `pr view --json labels`
 #   MOCK_COMMENTS_FILE  file served as the body of our own PR comments
 #   MOCK_LABELS_FAIL    set to 1 to make `pr view --json labels` fail
+#   MOCK_PR_LIST_FAIL   set to 1 to make `pr list` fail
 # The PR's base branch is not mocked: the script must take it from PR_BASE
 # (event payload), so a baseRefName query is an unhandled call and fails.
 make_mock_gh() {
@@ -48,6 +49,7 @@ elif [[ "$1 $2" == "pr comment" ]]; then
 elif [[ "$1 $2" == "pr edit" ]]; then
     :
 elif [[ "$1" == "api" ]]; then
+    [[ "${MOCK_PR_LIST_FAIL:-}" == 1 ]] && { echo "mock gh: pr list API down" >&2; exit 1; }
     :  # no sibling conflicts
 elif [[ "$1 $2" == "label create" ]]; then
     :
@@ -100,6 +102,7 @@ run_resume() {
         GH="$MOCK_DIR/mock_gh.sh" GIT="$MOCK_DIR/mock_git.sh" \
         MOCK_LABELS="$MOCK_LABELS" MOCK_LABELS_FAIL="${MOCK_LABELS_FAIL:-}" \
         MOCK_COMMENTS_FILE="$MOCK_COMMENTS_FILE" CALLS="$CALLS" \
+        MOCK_PR_LIST_FAIL="${MOCK_PR_LIST_FAIL:-}" \
         bash "$ROOT_DIR/update-pr-stack.sh" >"$WORK/out.log" 2>&1 || echo "EXIT=$?" >>"$WORK/out.log"
 }
 
@@ -256,5 +259,26 @@ grep -q "remove-label" "$CALLS" && fail "H: label must NOT be touched on an API
 [[ "$(git -C "$ORIGIN" rev-parse child)" == "$CHILD_BEFORE" ]] || fail "H: child was pushed"
 ok "H: labels API failure fails the run instead of skipping the resume"
 
+# ---------------------------------------------------------------------------
+echo "### Scenario I: listing sibling conflicts fails -> keep the old base branch"
+setup_repo
+# Same successful-resume setup as scenario C, but the sibling listing that
+# decides whether the old base branch can be deleted fails. Answering "no
+# siblings" there would delete a branch another conflicted PR still needs.
+git -C "$WORK" merge -q --no-edit main
+git -C "$WORK" push -q origin child
+MOCK_LABELS="autorestack-needs-conflict-resolution"
+MOCK_LABELS_FAIL=""
+PR_BASE="parent"
+MOCK_PR_LIST_FAIL=1
+MOCK_COMMENTS_FILE="$WORK/comments.txt"
+{ echo "### conflict"; echo; marker parent main "$SQUASH"; } > "$MOCK_COMMENTS_FILE"
+run_resume
+
+grep -q "EXIT=" "$WORK/out.log" || fail "I: run should have failed"
+git -C "$ORIGIN" rev-parse --verify -q parent >/dev/null || fail "I: old base branch was deleted on an API failure"
+grep -q -- "push origin :parent" "$CALLS" && fail "I: deletion must not be attempted"
+ok "I: sibling-listing API failure keeps the old base branch"
+
 echo
 echo "All conflict-resume tests passed 🎉 ($PASS scenarios)"
diff --git a/tests/test_update_pr_stack.sh b/tests/test_update_pr_stack.sh
@@ -212,6 +212,31 @@ else
     exit 1
 fi
 
+# A failed children listing must fail the run before any mutation: silently
+# treating it as "no children" would delete the merged branch under the
+# children it never saw.
+echo -e "\nRunning update script with a failing pr list..."
+FAIL_LOG="$TEST_REPO/update_fail_run.log"
+if log_cmd env \
+    SQUASH_COMMIT=$SQUASH_COMMIT \
+    MERGED_BRANCH=feature1 \
+    PR_NUMBER=1 \
+    TARGET_BRANCH=main \
+    MOCK_PR_LIST_FAIL=1 \
+    GH="$SCRIPT_DIR/mock_gh.sh" \
+    GIT="$SCRIPT_DIR/mock_git.sh" \
+    $SCRIPT_DIR/../update-pr-stack.sh > "$FAIL_LOG" 2>&1; then
+    echo "❌ run must fail when the children cannot be listed"
+    cat "$FAIL_LOG"
+    exit 1
+fi
+if grep -q "git push origin :feature1" "$FAIL_LOG"; then
+    echo "❌ merged branch must not be deleted when the children cannot be listed"
+    cat "$FAIL_LOG"
+    exit 1
+fi
+echo "✅ Failing pr list fails the run without deleting the merged branch"
+
 echo -e "\nAll tests passed! 🎉"
 
 # Clean up
diff --git a/update-pr-stack.sh b/update-pr-stack.sh
@@ -46,6 +46,9 @@ format_state_marker() {
 }
 
 # Echoes the most recent state-marker line found in our PR comments, or nothing.
+# Dies when the comments cannot be read at all: an API failure must not pass
+# for "no marker", which the caller treats as a reason to give up the resume
+# and remove the conflict label for good.
 read_state_marker() {
     local PR_NUMBER="$1"
     local BODIES
@@ -149,9 +152,10 @@ is_rebase_merge() {
 }
 
 # Echoes "<number> <head branch>" for each open PR based on the merged branch.
-# try, not run: callers consume this through a process substitution, which
-# swallows the exit status either way; a die in here would only leave that
-# subshell. Making the callers notice the failure is a separate concern.
+# try, not run: callers run this in a command substitution, where a die would
+# only leave the subshell, so they capture the output and die themselves. An
+# unhandled failure here must not pass for "no children": the caller would
+# then delete the merged branch under the children it never saw.
 list_child_prs() {
     try gh api "repos/{owner}/{repo}/pulls?base=$MERGED_BRANCH&state=open&per_page=100" \
         --paginate --jq '.[] | "\(.number) \(.head.ref)"'
@@ -252,14 +256,17 @@ pr_has_conflict_label() {
 
 # Check if any other PRs with conflict label still depend on a given base branch
 # Returns 0 (true) if siblings exist, 1 (false) if no siblings
+# Dies when the PRs cannot be listed: answering "no siblings" on an API failure
+# makes the caller delete a branch a sibling may still need for its resolution.
 has_sibling_conflicts() {
     local BASE_BRANCH="$1"
     local EXCLUDE_BRANCH="$2"
 
     # Find all open PRs with the conflict label that are based on BASE_BRANCH
     local CONFLICTED_SIBLINGS
     CONFLICTED_SIBLINGS=$(gh api "repos/{owner}/{repo}/pulls?base=$BASE_BRANCH&state=open&per_page=100" \
-        --paginate --jq ".[] | select(any(.labels[]; .name == \"$CONFLICT_LABEL\")) | .head.ref" 2>/dev/null || echo "")
+        --paginate --jq ".[] | select(any(.labels[]; .name == \"$CONFLICT_LABEL\")) | .head.ref") \
+        || die "could not list conflicted PRs based on $BASE_BRANCH"
 
     for SIBLING in $CONFLICTED_SIBLINGS; do
         if [[ "$SIBLING" != "$EXCLUDE_BRANCH" ]]; then
@@ -395,10 +402,11 @@ main() {
     # children and delete the merged branch.
     if git rev-parse --verify --quiet SQUASH_COMMIT^2 >/dev/null; then
         echo "✓ '$MERGED_BRANCH' was merged with a merge commit, not squashed; retargeting children without touching their heads"
+        CHILDREN=$(list_child_prs) || die "could not list the PRs based on $MERGED_BRANCH"
         while read -r NUMBER BRANCH; do
             [[ -n "$BRANCH" ]] || continue
             run gh pr edit "$NUMBER" --base "$TARGET_BRANCH"
-        done < <(list_child_prs)
+        done <<<"$CHILDREN"
         # Deleting a PR's base branch closes the PR, so the retargets come first.
         run git push origin ":$MERGED_BRANCH"
         return 0
@@ -410,21 +418,23 @@ main() {
     # the intermediate copies. Tell the children and leave everything alone.
     if is_rebase_merge "$PR_NUMBER"; then
         echo "⚠️ '$MERGED_BRANCH' looks rebase-merged; rebase merges are not supported, leaving the stack alone"
+        CHILDREN=$(list_child_prs) || die "could not list the PRs based on $MERGED_BRANCH"
         while read -r NUMBER BRANCH; do
             [[ -n "$BRANCH" ]] || continue
             run gh pr comment "$NUMBER" --body "ℹ️ The base branch \`$MERGED_BRANCH\` of this PR was merged with \"Rebase and merge\", which autorestack does not support. Update this PR manually. \`$MERGED_BRANCH\` was kept so this PR stays open."
-        done < <(list_child_prs)
+        done <<<"$CHILDREN"
         return 0
     fi
 
     # Find all PRs directly targeting the merged PR's head
+    CHILDREN=$(list_child_prs) || die "could not list the PRs based on $MERGED_BRANCH"
     INITIAL_NUMBERS=()
     INITIAL_TARGETS=()
     while read -r NUMBER BRANCH; do
         [[ -n "$BRANCH" ]] || continue
         INITIAL_NUMBERS+=("$NUMBER")
         INITIAL_TARGETS+=("$BRANCH")
-    done < <(list_child_prs)
+    done <<<"$CHILDREN"
 
     # Track successfully updated vs conflicted branches separately
     UPDATED_TARGETS=()
