From 39856f972a90a8da0941818b8a15362a339deee1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?No=C3=A9=20Rubinstein?= Date: Mon, 1 Jun 2026 18:02:35 +0200 Subject: [PATCH 1/7] Stop corrupting PRs whose base changed during conflict resolution When a PR sat in autorestack-needs-conflict-resolution and a human changed its base instead of resolving the conflict, the resume run rebuilt its state with `gh pr list --head | .[0]`. On a long-lived base like `spark` that returns an unrelated ancient merge: in run 26764629741 it picked a 2022 release merge into the deleted branch `spark-v1.13.0-rc-dev`, pushed a merge commit built against it onto the PR, removed the conflict label, then crashed setting the base to the missing branch. The resume now recovers base/target/squash from a marker the squash-merge run embeds in the conflict comment, and bails before any mutation when the marker is missing or the PR's current base no longer matches the one we left it on (the manual-retarget case). Target existence is checked as a backstop, and the base retarget runs before the label is dropped so a failure leaves the PR resumable. Co-Authored-By: Claude Opus 4.8 (1M context) --- .github/workflows/tests.yml | 1 + tests/test_conflict_resolution_resume.sh | 140 +++++++++++++++++++++++ update-pr-stack.sh | 136 +++++++++++++++------- 3 files changed, 235 insertions(+), 42 deletions(-) create mode 100644 tests/test_conflict_resolution_resume.sh diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index 83ff8a2..c524c79 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -17,6 +17,7 @@ jobs: bash tests/test_update_pr_stack.sh bash tests/test_rebase_workflow.sh bash tests/test_mixed_workflows.sh + bash tests/test_conflict_resolution_resume.sh e2e-tests: name: E2E Tests diff --git a/tests/test_conflict_resolution_resume.sh b/tests/test_conflict_resolution_resume.sh new file mode 100644 index 0000000..2a4cd59 --- /dev/null +++ b/tests/test_conflict_resolution_resume.sh @@ -0,0 +1,140 @@ +#!/bin/bash +# +# Tests for the conflict-resolved continuation path (continue_after_resolution). +# +# Focus: the run that resumes after a user pushes a conflict resolution must +# recover its state from the marker left in the conflict comment, and must NOT +# mutate the PR when the recorded state no longer applies (no marker, or the user +# manually retargeted the base). A previous version re-derived the state from +# `gh pr list` and, on a long-lived base, pushed a merge built against a +# non-existent branch before crashing. + +set -ueo pipefail + +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +ROOT_DIR="$(cd "$SCRIPT_DIR/.." && pwd)" + +PASS=0 +fail() { echo "❌ $1"; exit 1; } +ok() { echo "✅ $1"; PASS=$((PASS+1)); } + +# Build a configurable gh mock in a temp dir. It records every invocation to +# $CALLS and is driven by env vars set per scenario: +# 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` +make_mock_gh() { + local dir="$1" + cat > "$dir/mock_gh.sh" <<'EOF' +#!/bin/bash +set -ueo pipefail +echo "gh $*" >> "$CALLS" +if [[ "$1 $2" == "pr view" ]]; then + case "$*" in + *--json\ labels*) 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;; + esac +elif [[ "$1 $2" == "pr comment" ]]; then + cat >/dev/null # consume the -F - body +elif [[ "$1 $2" == "pr edit" ]]; then + : +elif [[ "$1 $2" == "pr list" ]]; then + : # no sibling conflicts +elif [[ "$1 $2" == "label create" ]]; then + : +else + echo "unhandled gh: $*" >&2; exit 1 +fi +EOF + chmod +x "$dir/mock_gh.sh" +} + +# Set up a fresh repo with a bare origin so real pushes are observable. +setup_repo() { + WORK=$(mktemp -d) + ORIGIN=$(mktemp -d) + git init -q --bare "$ORIGIN" + git init -q -b main "$WORK" + cd "$WORK" + git config user.email t@e.com && git config user.name t + git remote add origin "$ORIGIN" + + echo base > f.txt && git add f.txt && git commit -qm initial + SQUASH=$(git rev-parse HEAD) # the squash commit lives on main/target + git push -q origin main + + git checkout -q -b parent && git push -q origin parent # merged parent branch + git checkout -q -b child # the PR under resolution + echo child >> f.txt && git add f.txt && git commit -qm child + git push -q origin child + CHILD_BEFORE=$(git rev-parse child) + CALLS="$WORK/calls.log"; : > "$CALLS" + MOCK_DIR=$(mktemp -d); make_mock_gh "$MOCK_DIR" +} + +run_resume() { + env ACTION_MODE=conflict-resolved PR_BRANCH=child \ + GH="$MOCK_DIR/mock_gh.sh" \ + MOCK_LABELS="$MOCK_LABELS" MOCK_BASE="$MOCK_BASE" \ + MOCK_COMMENTS_FILE="$MOCK_COMMENTS_FILE" CALLS="$CALLS" \ + bash "$ROOT_DIR/update-pr-stack.sh" >"$WORK/out.log" 2>&1 || echo "EXIT=$?" >>"$WORK/out.log" +} + +marker() { # base target squash + printf '' "$1" "$2" "$3" +} + +# --------------------------------------------------------------------------- +echo "### Scenario A: user manually retargeted the base -> no mutation" +setup_repo +MOCK_LABELS="autorestack-needs-conflict-resolution" +MOCK_BASE="spark" # human changed it; marker says parent +MOCK_COMMENTS_FILE="$WORK/comments.txt" +{ echo "### conflict"; echo; marker parent main "$SQUASH"; } > "$MOCK_COMMENTS_FILE" +run_resume + +grep -q "remove-label autorestack-needs-conflict-resolution" "$CALLS" || fail "A: label not removed" +grep -q "gh pr comment" "$CALLS" || fail "A: no explanatory comment posted" +grep -q -- "--base" "$CALLS" && fail "A: base must NOT be edited" +[[ "$(git -C "$WORK" rev-parse child)" == "$CHILD_BEFORE" ]] || fail "A: child branch was mutated" +[[ "$(git -C "$ORIGIN" rev-parse child)" == "$CHILD_BEFORE" ]] || fail "A: child was pushed" +ok "A: manual retarget detected, no branch mutation, label removed" + +# --------------------------------------------------------------------------- +echo "### Scenario B: no state marker -> no mutation" +setup_repo +MOCK_LABELS="autorestack-needs-conflict-resolution" +MOCK_BASE="parent" +MOCK_COMMENTS_FILE="$WORK/comments.txt" +{ echo "### some old conflict comment with no marker"; } > "$MOCK_COMMENTS_FILE" +run_resume + +grep -q "remove-label autorestack-needs-conflict-resolution" "$CALLS" || fail "B: label not removed" +grep -q -- "--base" "$CALLS" && fail "B: base must NOT be edited" +[[ "$(git -C "$ORIGIN" rev-parse child)" == "$CHILD_BEFORE" ]] || fail "B: child was pushed" +ok "B: missing marker handled, no branch mutation, label removed" + +# --------------------------------------------------------------------------- +echo "### Scenario C: base matches and target exists -> resume, base before label" +setup_repo +# Make child already contain target(main) + squash so update_direct_target is a +# no-op and we exercise the push/retarget/label ordering directly. +git -C "$WORK" merge -q --no-edit main +git -C "$WORK" push -q origin child +MOCK_LABELS="autorestack-needs-conflict-resolution" +MOCK_BASE="parent" # matches marker -> not a manual retarget +MOCK_COMMENTS_FILE="$WORK/comments.txt" +{ echo "### conflict"; echo; marker parent main "$SQUASH"; } > "$MOCK_COMMENTS_FILE" +run_resume + +grep -q -- "pr edit child --base main" "$CALLS" || fail "C: base not retargeted to main" +grep -q "remove-label autorestack-needs-conflict-resolution" "$CALLS" || fail "C: label not removed" +base_line=$(grep -n -- "--base main" "$CALLS" | head -1 | cut -d: -f1) +label_line=$(grep -n "remove-label" "$CALLS" | head -1 | cut -d: -f1) +[[ "$base_line" -lt "$label_line" ]] || fail "C: base edit must come before label removal" +ok "C: resume retargets base then removes label" + +echo +echo "All conflict-resume tests passed 🎉 ($PASS scenarios)" diff --git a/update-pr-stack.sh b/update-pr-stack.sh index c000f41..f35d366 100755 --- a/update-pr-stack.sh +++ b/update-pr-stack.sh @@ -21,6 +21,37 @@ source "$SCRIPT_DIR/command_utils.sh" CONFLICT_LABEL="autorestack-needs-conflict-resolution" +# Machine-readable marker embedded (invisibly) in the conflict comment so the +# conflict-resolved run can recover the exact stack state. We rely on this rather +# than re-deriving it from `gh pr list --head `: when the base is a +# long-lived integration branch that heuristic returns an unrelated ancient merge +# (e.g. a years-old release merge), and it cannot tell whether a human retargeted +# the PR in the meantime. +STATE_MARKER_PREFIX="' \ + "$STATE_MARKER_PREFIX" "$1" "$2" "$3" +} + +# Echoes the most recent state-marker line found in our PR comments, or nothing. +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 +} + +# Args: a marker line. Echoes "base target squash". +parse_state_marker() { + local LINE="$1" + printf '%s %s %s\n' \ + "$(sed -n 's/.* base=\([^ ]*\).*/\1/p' <<<"$LINE")" \ + "$(sed -n 's/.* target=\([^ ]*\).*/\1/p' <<<"$LINE")" \ + "$(sed -n 's/.* squash=\([^ ]*\).*/\1/p' <<<"$LINE")" +} + # Allow replacing git and gh [ -v GIT ] && git() { "$GIT" "$@"; } [ -v GH ] && gh() { "$GH" "$@"; } @@ -128,6 +159,8 @@ update_direct_target() { echo '```' echo echo "Once you push, this action will resume and finish updating this pull request." + echo + format_state_marker "$MERGED_BRANCH" "$TARGET_BRANCH" "$(git rev-parse SQUASH_COMMIT)" } | log_cmd gh pr comment "$BRANCH" -F - # Create the label if it doesn't exist, then add it to the PR gh label create "$CONFLICT_LABEL" --description "PR needs manual conflict resolution" --color "d73a4a" 2>/dev/null || true @@ -190,54 +223,73 @@ continue_after_resolution() { echo "Found conflict label on $PR_BRANCH, continuing stack update..." - # Get the current base branch (the old base that was kept during conflict) - local OLD_BASE - OLD_BASE=$(gh pr view "$PR_BRANCH" --json baseRefName --jq '.baseRefName') - echo "Current base branch: $OLD_BASE" - # The synchronize payload is the child PR, so SQUASH_COMMIT / MERGED_BRANCH / # TARGET_BRANCH from the original squash-merge run are not in the environment. - # Reconstruct them from the merged parent PR: OLD_BASE is the parent branch, - # and the merged PR whose head is OLD_BASE gives the new target (its base) and - # the squash commit (its merge commit). - local NEW_TARGET SQUASH_HASH - read -r NEW_TARGET SQUASH_HASH < <(gh pr list --head "$OLD_BASE" --state merged \ - --json baseRefName,mergeCommit --jq '.[0] | "\(.baseRefName // "") \(.mergeCommit.oid // "")"') - - if [[ -z "$NEW_TARGET" || -z "$SQUASH_HASH" ]]; then - echo "⚠️ Could not find where '$OLD_BASE' was merged to; skipping base branch and deletion updates" - # Don't update base or delete old branch - leave things as they are - else - echo "Old base '$OLD_BASE' was merged to '$NEW_TARGET' as $SQUASH_HASH" - - # The squash-merge run pushed the base merge and asked the user to resolve - # the pre-squash merge, but it never recorded the squash itself. Finish - # that now: re-run the same merge sequence as the squash-merge path. With - # the user's resolution in place the base merge and pre-squash merge are - # no-ops; only the "-s ours" squash record gets applied, keeping the diff - # against the new base clean. has_squash_commit makes this idempotent. - log_cmd git update-ref SQUASH_COMMIT "$SQUASH_HASH" - MERGED_BRANCH="$OLD_BASE" - TARGET_BRANCH="$NEW_TARGET" - if ! update_direct_target "$PR_BRANCH" "$NEW_TARGET"; then - echo "⚠️ '$PR_BRANCH' still conflicts; re-posted the conflict comment, will retry on next push" - return 1 - fi - log_cmd git push origin "$PR_BRANCH" + # Recover them from the marker the squash-merge run left in the conflict + # comment. + local MARKER + MARKER=$(read_state_marker "$PR_BRANCH") + if [[ -z "$MARKER" ]]; then + echo "⚠️ No autorestack state marker on $PR_BRANCH; cannot resume safely. Removing the label." + { echo "ℹ️ 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, do it manually."; } \ + | log_cmd gh pr comment "$PR_BRANCH" -F - + log_cmd gh pr edit "$PR_BRANCH" --remove-label "$CONFLICT_LABEL" + return + fi - # Remove the conflict label + local RECORDED_BASE NEW_TARGET SQUASH_HASH + read -r RECORDED_BASE NEW_TARGET SQUASH_HASH < <(parse_state_marker "$MARKER") + echo "Recorded state: base=$RECORDED_BASE target=$NEW_TARGET squash=$SQUASH_HASH" + + # The base we left the PR on while waiting for conflict resolution was the + # merged parent branch. If it no longer matches, a human retargeted the PR + # (e.g. straight onto the integration branch); we are no longer the authority + # on its base, so we step back without touching the branch. Doing this BEFORE + # any mutation is what stops the failure mode where we pushed a merge built + # against a bogus target and then crashed. + local CURRENT_BASE + CURRENT_BASE=$(gh pr view "$PR_BRANCH" --json baseRefName --jq '.baseRefName') + if [[ "$CURRENT_BASE" != "$RECORDED_BASE" ]]; then + echo "⚠️ Base of $PR_BRANCH changed manually ($RECORDED_BASE -> $CURRENT_BASE); not updating the stack." + { echo "ℹ️ The base branch of this PR was changed manually, so autorestack stepped back and will not update it automatically."; } \ + | log_cmd gh pr comment "$PR_BRANCH" -F - log_cmd gh pr edit "$PR_BRANCH" --remove-label "$CONFLICT_LABEL" + return + fi - # Update the PR's base branch to the new target - log_cmd gh pr edit "$PR_BRANCH" --base "$NEW_TARGET" + # Defense in depth: never act on a target branch that no longer exists. + if ! git rev-parse --verify --quiet "origin/$NEW_TARGET" >/dev/null; then + echo "⚠️ Recorded target branch '$NEW_TARGET' no longer exists; leaving $PR_BRANCH untouched." + return + fi - # Check if old base branch should be deleted - if has_sibling_conflicts "$OLD_BASE" "$PR_BRANCH"; then - echo "⚠️ Keeping branch '$OLD_BASE' - still referenced by other conflicted PRs" - else - echo "Deleting old base branch '$OLD_BASE' (no other PRs depend on it)" - log_cmd git push origin ":$OLD_BASE" || echo "⚠️ Could not delete '$OLD_BASE' (may already be deleted)" - fi + # The squash-merge run pushed the base merge and asked the user to resolve the + # pre-squash merge, but it never recorded the squash itself. Finish that now: + # re-run the same merge sequence as the squash-merge path. With the user's + # resolution in place the base merge and pre-squash merge are no-ops; only the + # "-s ours" squash record gets applied, keeping the diff against the new base + # clean. has_squash_commit makes this idempotent. + log_cmd git update-ref SQUASH_COMMIT "$SQUASH_HASH" + MERGED_BRANCH="$RECORDED_BASE" + TARGET_BRANCH="$NEW_TARGET" + if ! update_direct_target "$PR_BRANCH" "$NEW_TARGET"; then + echo "⚠️ '$PR_BRANCH' still conflicts; re-posted the conflict comment, will retry on next push" + return 1 + fi + + # Order matters: push the cleaned-up head, then retarget the base, and only + # then drop the label. The retarget is the step that previously failed; if + # anything here fails the label stays, so the next push resumes. + log_cmd git push origin "$PR_BRANCH" + log_cmd gh pr edit "$PR_BRANCH" --base "$NEW_TARGET" + log_cmd gh pr edit "$PR_BRANCH" --remove-label "$CONFLICT_LABEL" + + # Check if old base branch should be deleted + if has_sibling_conflicts "$RECORDED_BASE" "$PR_BRANCH"; then + echo "⚠️ Keeping branch '$RECORDED_BASE' - still referenced by other conflicted PRs" + else + echo "Deleting old base branch '$RECORDED_BASE' (no other PRs depend on it)" + log_cmd git push origin ":$RECORDED_BASE" || echo "⚠️ Could not delete '$RECORDED_BASE' (may already be deleted)" fi } From e84e732ee91056c4cbbe6be780f44757beaf36f6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?No=C3=A9=20Rubinstein?= Date: Tue, 2 Jun 2026 09:05:28 +0200 Subject: [PATCH 2/7] Reword test header: name the root cause, not the symptom Co-Authored-By: Claude Opus 4.8 (1M context) --- tests/test_conflict_resolution_resume.sh | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/test_conflict_resolution_resume.sh b/tests/test_conflict_resolution_resume.sh index 2a4cd59..b59759b 100644 --- a/tests/test_conflict_resolution_resume.sh +++ b/tests/test_conflict_resolution_resume.sh @@ -5,9 +5,9 @@ # Focus: the run that resumes after a user pushes a conflict resolution must # recover its state from the marker left in the conflict comment, and must NOT # mutate the PR when the recorded state no longer applies (no marker, or the user -# manually retargeted the base). A previous version re-derived the state from -# `gh pr list` and, on a long-lived base, pushed a merge built against a -# non-existent branch before crashing. +# manually retargeted the base). A previous version re-derived the base pull +# request from the target branch, which made it sensitive to humans doing +# unexpected things such as changing the base branch manually. set -ueo pipefail From f7f7d03c85cb7f8066d191f0ef4870776ead9bb6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?No=C3=A9=20Rubinstein?= Date: Tue, 2 Jun 2026 09:10:14 +0200 Subject: [PATCH 3/7] Simplify state-marker rationale comment Co-Authored-By: Claude Opus 4.8 (1M context) --- update-pr-stack.sh | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/update-pr-stack.sh b/update-pr-stack.sh index f35d366..cd65222 100755 --- a/update-pr-stack.sh +++ b/update-pr-stack.sh @@ -22,11 +22,9 @@ source "$SCRIPT_DIR/command_utils.sh" CONFLICT_LABEL="autorestack-needs-conflict-resolution" # Machine-readable marker embedded (invisibly) in the conflict comment so the -# conflict-resolved run can recover the exact stack state. We rely on this rather -# than re-deriving it from `gh pr list --head `: when the base is a -# long-lived integration branch that heuristic returns an unrelated ancient merge -# (e.g. a years-old release merge), and it cannot tell whether a human retargeted -# the PR in the meantime. +# conflict-resolved run can recover the exact stack state it recorded, instead of +# re-deriving the parent PR from the PR's current base branch (which breaks when +# anything about that base changes, e.g. a human retargeting the PR manually). STATE_MARKER_PREFIX="