From 2384bbba790c78aa1c53761fb3449f8faf943108 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 9 Jun 2026 22:06:20 +0000 Subject: [PATCH] Stop treating gh API failures as negative answers Two helpers swallowed gh errors with `|| echo ""` and returned the negative answer instead (verified against the previous script version with the new test scenarios): - has_sibling_conflicts: a failed `gh pr list` read as "no siblings", so the resume deleted the kept parent branch while another conflicted PR may still need it for its resolution. - pr_has_conflict_label: a failed `gh pr view` read as "no label", ending the run green without resuming anything. Both now die on command failure, so the branch and label survive and the next push retries. read_state_marker gets the same explicit handling; there the gh failure already aborted the run (pipefail propagated it to an errexit-active caller), but silently, with the error message hidden by 2>/dev/null. Also reorders main() to push the updated heads before retargeting their PRs, matching the conflict-resolved path: if the run fails between the two steps, a retargeted PR with a stale head sits conflicting with its new base, GitHub suppresses its CI, and no label exists to resume it. https://claude.ai/code/session_01STkeSJ7cLrmrNn4aTDYkwH --- tests/test_conflict_resolution_resume.sh | 63 +++++++++++++++++++++++- update-pr-stack.sh | 46 +++++++++++------ 2 files changed, 93 insertions(+), 16 deletions(-) diff --git a/tests/test_conflict_resolution_resume.sh b/tests/test_conflict_resolution_resume.sh index 933611d..5740352 100644 --- a/tests/test_conflict_resolution_resume.sh +++ b/tests/test_conflict_resolution_resume.sh @@ -23,6 +23,8 @@ ok() { echo "✅ $1"; PASS=$((PASS+1)); } # MOCK_LABELS newline-separated labels returned by `pr view --json labels` # MOCK_BASE base branch returned by `pr view --json baseRefName` # MOCK_COMMENTS_FILE file whose contents are returned by `pr view --json 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 make_mock_gh() { local dir="$1" cat > "$dir/mock_gh.sh" <<'EOF' @@ -31,7 +33,9 @@ set -ueo pipefail echo "gh $*" >> "$CALLS" if [[ "$1 $2" == "pr view" ]]; then case "$*" in - *--json\ labels*) printf '%s\n' "${MOCK_LABELS:-}";; + *--json\ labels*) + [[ "${MOCK_LABELS_FAIL:-}" == 1 ]] && { echo "mock gh: labels API down" >&2; exit 1; } + printf '%s\n' "${MOCK_LABELS:-}";; *--json\ baseRefName*) printf '%s\n' "${MOCK_BASE:-}";; *--json\ comments*) cat "${MOCK_COMMENTS_FILE:-/dev/null}";; *) echo "unhandled pr view: $*" >&2; exit 1;; @@ -41,6 +45,7 @@ elif [[ "$1 $2" == "pr comment" ]]; then elif [[ "$1 $2" == "pr edit" ]]; then : elif [[ "$1 $2" == "pr list" ]]; 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 : @@ -92,6 +97,7 @@ run_resume() { GH="$MOCK_DIR/mock_gh.sh" GIT="$MOCK_DIR/mock_git.sh" \ MOCK_LABELS="$MOCK_LABELS" MOCK_BASE="$MOCK_BASE" \ MOCK_COMMENTS_FILE="$MOCK_COMMENTS_FILE" CALLS="$CALLS" \ + MOCK_LABELS_FAIL="${MOCK_LABELS_FAIL:-}" 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" } @@ -197,5 +203,60 @@ grep -q -- "--base" "$CALLS" && fail "E: base must NOT be edited" [[ "$(git -C "$ORIGIN" rev-parse child)" == "$CHILD_BEFORE" ]] || fail "E: child was pushed" ok "E: missing base branch detected, no crash, label removed" +# --------------------------------------------------------------------------- +echo "### Scenario F: reading the PR comments fails -> fail the run, keep the label" +setup_repo +# A transient API failure while reading the comments must not pass for "no +# state marker": that path removes the conflict label and permanently cancels +# the auto-resume. The run must fail loudly instead, so the next push retries. +MOCK_LABELS="autorestack-needs-conflict-resolution" +MOCK_BASE="parent" +MOCK_COMMENTS_FILE="$WORK/does-not-exist" # makes the mock gh fail on pr view --json comments +run_resume + +grep -q "EXIT=" "$WORK/out.log" || fail "F: run should have failed" +grep -q "remove-label" "$CALLS" && fail "F: label must NOT be removed on an API failure" +grep -q "gh pr comment" "$CALLS" && fail "F: no comment must be posted on an API failure" +[[ "$(git -C "$ORIGIN" rev-parse child)" == "$CHILD_BEFORE" ]] || fail "F: child was pushed" +ok "F: API failure fails the run and keeps the resume armed" + +# --------------------------------------------------------------------------- +echo "### Scenario G: reading the labels fails -> fail the run, do nothing" +setup_repo +# An API failure must not pass for "no conflict label": that ends the run +# green without resuming anything. +MOCK_LABELS="" +MOCK_LABELS_FAIL=1 +MOCK_BASE="parent" +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 "G: run should have failed, not ended green" +grep -q "remove-label" "$CALLS" && fail "G: label must NOT be touched on an API failure" +[[ "$(git -C "$ORIGIN" rev-parse child)" == "$CHILD_BEFORE" ]] || fail "G: child was pushed" +ok "G: labels API failure fails the run instead of skipping the resume" + +# --------------------------------------------------------------------------- +echo "### Scenario H: 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="" +MOCK_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 "H: run should have failed" +git -C "$ORIGIN" rev-parse --verify -q parent >/dev/null || fail "H: old base branch was deleted on an API failure" +grep -q -- "push origin :parent" "$CALLS" && fail "H: deletion must not be attempted" +ok "H: sibling-listing API failure keeps the old base branch" + echo echo "All conflict-resume tests passed 🎉 ($PASS scenarios)" diff --git a/update-pr-stack.sh b/update-pr-stack.sh index 2b1a0b7..4769067 100755 --- a/update-pr-stack.sh +++ b/update-pr-stack.sh @@ -39,10 +39,15 @@ 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_BRANCH="$1" - gh pr view "$PR_BRANCH" --json comments --jq '.comments[].body' 2>/dev/null \ - | { grep -F "$STATE_MARKER_PREFIX" || true; } | tail -n1 + local COMMENTS + COMMENTS=$(gh pr view "$PR_BRANCH" --json comments --jq '.comments[].body') \ + || die "could not read the comments of $PR_BRANCH" + grep -F "$STATE_MARKER_PREFIX" <<<"$COMMENTS" | tail -n1 || true } # Args: a marker line. Echoes "base target squash". @@ -185,23 +190,29 @@ See $GITHUB_SERVER_URL/$GITHUB_REPOSITORY/actions/runs/$GITHUB_RUN_ID" return 0 } -# Check if a PR has the conflict resolution label +# Check if a PR has the conflict resolution label. Dies when the labels cannot +# be read, rather than answering "no" and letting the run end green without +# resuming anything. pr_has_conflict_label() { local BRANCH="$1" local LABELS - LABELS=$(gh pr view "$BRANCH" --json labels --jq '.labels[].name' 2>/dev/null || echo "") - echo "$LABELS" | grep -q "^${CONFLICT_LABEL}$" + LABELS=$(gh pr view "$BRANCH" --json labels --jq '.labels[].name') \ + || die "could not read the labels of $BRANCH" + grep -q "^${CONFLICT_LABEL}$" <<<"$LABELS" } # 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 pr list --base "$BASE_BRANCH" --label "$CONFLICT_LABEL" --json headRefName --jq '.[].headRefName' 2>/dev/null || echo "") + CONFLICTED_SIBLINGS=$(gh pr list --base "$BASE_BRANCH" --label "$CONFLICT_LABEL" --json headRefName --jq '.[].headRefName') \ + || die "could not list conflicted PRs based on $BASE_BRANCH" for SIBLING in $CONFLICTED_SIBLINGS; do if [[ "$SIBLING" != "$EXCLUDE_BRANCH" ]]; then @@ -242,7 +253,7 @@ continue_after_resolution() { # Recover them from the marker the squash-merge run left in the conflict # comment. local MARKER - MARKER=$(read_state_marker "$PR_BRANCH") + MARKER=$(read_state_marker "$PR_BRANCH") || die "could not read the state marker of $PR_BRANCH" if [[ -z "$MARKER" ]]; then echo "⚠ī¸ No autorestack state marker on $PR_BRANCH; cannot resume safely. Removing the label." abandon_resume "$PR_BRANCH" "ℹī¸ autorestack could not find its state marker on this PR, so it will not update the stack automatically. If this PR still needs its base updated, update its base manually." @@ -345,20 +356,25 @@ main() { fi done - # Only update base branches for successfully updated PRs + # Push the updated heads before retargeting their PRs, so each head already + # contains TARGET_BRANCH when the base flips to it and the PR stays + # mergeable (GitHub suppresses CI on a PR that conflicts with its base). + # Same ordering as the conflict-resolved path; it also keeps a failure + # between the two steps recoverable, since a retargeted PR with a stale + # head would sit conflicting with its new base with no label to resume it. + if [[ "${#UPDATED_TARGETS[@]}" -gt 0 ]]; then + run git push origin "${UPDATED_TARGETS[@]}" + fi + for BRANCH in "${UPDATED_TARGETS[@]}"; do run gh pr edit "$BRANCH" --base "$TARGET_BRANCH" done - # Push updated branches; only delete merged branch if no conflicts + # Delete the merged branch last: open PRs must no longer be based on it + # when it goes away, and conflicted PRs still need it for their resolution. if [[ "${#CONFLICTED_TARGETS[@]}" -eq 0 ]]; then - # No conflicts - safe to delete merged branch - run git push origin ":$MERGED_BRANCH" "${UPDATED_TARGETS[@]}" + run git push origin ":$MERGED_BRANCH" else - # Some conflicts - keep merged branch for reference during manual resolution - if [[ "${#UPDATED_TARGETS[@]}" -gt 0 ]]; then - run git push origin "${UPDATED_TARGETS[@]}" - fi echo "⚠ī¸ Keeping branch '$MERGED_BRANCH' - still referenced by conflicted PRs: ${CONFLICTED_TARGETS[*]}" fi }