Skip to content

Commit 39856f9

Browse files
Phlogistiqueclaude
Phlogistique
and
claude
committed
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 <base> | .[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) <noreply@anthropic.com>
1 parent 466d7c9 commit 39856f9

3 files changed

Lines changed: 235 additions & 42 deletions

File tree

‎.github/workflows/tests.yml‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ jobs:
1717
bash tests/test_update_pr_stack.sh
1818
bash tests/test_rebase_workflow.sh
1919
bash tests/test_mixed_workflows.sh
20+
bash tests/test_conflict_resolution_resume.sh
2021
2122
e2e-tests:
2223
name: E2E Tests

‎tests/test_conflict_resolution_resume.sh‎

Lines changed: 140 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,140 @@
1+
#!/bin/bash
2+
#
3+
# Tests for the conflict-resolved continuation path (continue_after_resolution).
4+
#
5+
# Focus: the run that resumes after a user pushes a conflict resolution must
6+
# recover its state from the marker left in the conflict comment, and must NOT
7+
# mutate the PR when the recorded state no longer applies (no marker, or the user
8+
# manually retargeted the base). A previous version re-derived the state from
9+
# `gh pr list` and, on a long-lived base, pushed a merge built against a
10+
# non-existent branch before crashing.
11+
12+
set -ueo pipefail
13+
14+
SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
15+
ROOT_DIR="$(cd "$SCRIPT_DIR/.." && pwd)"
16+
17+
PASS=0
18+
fail() { echo "❌ $1"; exit 1; }
19+
ok() { echo "✅ $1"; PASS=$((PASS+1)); }
20+
21+
# Build a configurable gh mock in a temp dir. It records every invocation to
22+
# $CALLS and is driven by env vars set per scenario:
23+
# MOCK_LABELS newline-separated labels returned by `pr view --json labels`
24+
# MOCK_BASE base branch returned by `pr view --json baseRefName`
25+
# MOCK_COMMENTS_FILE file whose contents are returned by `pr view --json comments`
26+
make_mock_gh() {
27+
local dir="$1"
28+
cat > "$dir/mock_gh.sh" <<'EOF'
29+
#!/bin/bash
30+
set -ueo pipefail
31+
echo "gh $*" >> "$CALLS"
32+
if [[ "$1 $2" == "pr view" ]]; then
33+
case "$*" in
34+
*--json\ labels*) printf '%s\n' "${MOCK_LABELS:-}";;
35+
*--json\ baseRefName*) printf '%s\n' "${MOCK_BASE:-}";;
36+
*--json\ comments*) cat "${MOCK_COMMENTS_FILE:-/dev/null}";;
37+
*) echo "unhandled pr view: $*" >&2; exit 1;;
38+
esac
39+
elif [[ "$1 $2" == "pr comment" ]]; then
40+
cat >/dev/null # consume the -F - body
41+
elif [[ "$1 $2" == "pr edit" ]]; then
42+
:
43+
elif [[ "$1 $2" == "pr list" ]]; then
44+
: # no sibling conflicts
45+
elif [[ "$1 $2" == "label create" ]]; then
46+
:
47+
else
48+
echo "unhandled gh: $*" >&2; exit 1
49+
fi
50+
EOF
51+
chmod +x "$dir/mock_gh.sh"
52+
}
53+
54+
# Set up a fresh repo with a bare origin so real pushes are observable.
55+
setup_repo() {
56+
WORK=$(mktemp -d)
57+
ORIGIN=$(mktemp -d)
58+
git init -q --bare "$ORIGIN"
59+
git init -q -b main "$WORK"
60+
cd "$WORK"
61+
git config user.email t@e.com && git config user.name t
62+
git remote add origin "$ORIGIN"
63+
64+
echo base > f.txt && git add f.txt && git commit -qm initial
65+
SQUASH=$(git rev-parse HEAD) # the squash commit lives on main/target
66+
git push -q origin main
67+
68+
git checkout -q -b parent && git push -q origin parent # merged parent branch
69+
git checkout -q -b child # the PR under resolution
70+
echo child >> f.txt && git add f.txt && git commit -qm child
71+
git push -q origin child
72+
CHILD_BEFORE=$(git rev-parse child)
73+
CALLS="$WORK/calls.log"; : > "$CALLS"
74+
MOCK_DIR=$(mktemp -d); make_mock_gh "$MOCK_DIR"
75+
}
76+
77+
run_resume() {
78+
env ACTION_MODE=conflict-resolved PR_BRANCH=child \
79+
GH="$MOCK_DIR/mock_gh.sh" \
80+
MOCK_LABELS="$MOCK_LABELS" MOCK_BASE="$MOCK_BASE" \
81+
MOCK_COMMENTS_FILE="$MOCK_COMMENTS_FILE" CALLS="$CALLS" \
82+
bash "$ROOT_DIR/update-pr-stack.sh" >"$WORK/out.log" 2>&1 || echo "EXIT=$?" >>"$WORK/out.log"
83+
}
84+
85+
marker() { # base target squash
86+
printf '<!-- autorestack-state: base=%s target=%s squash=%s -->' "$1" "$2" "$3"
87+
}
88+
89+
# ---------------------------------------------------------------------------
90+
echo "### Scenario A: user manually retargeted the base -> no mutation"
91+
setup_repo
92+
MOCK_LABELS="autorestack-needs-conflict-resolution"
93+
MOCK_BASE="spark" # human changed it; marker says parent
94+
MOCK_COMMENTS_FILE="$WORK/comments.txt"
95+
{ echo "### conflict"; echo; marker parent main "$SQUASH"; } > "$MOCK_COMMENTS_FILE"
96+
run_resume
97+
98+
grep -q "remove-label autorestack-needs-conflict-resolution" "$CALLS" || fail "A: label not removed"
99+
grep -q "gh pr comment" "$CALLS" || fail "A: no explanatory comment posted"
100+
grep -q -- "--base" "$CALLS" && fail "A: base must NOT be edited"
101+
[[ "$(git -C "$WORK" rev-parse child)" == "$CHILD_BEFORE" ]] || fail "A: child branch was mutated"
102+
[[ "$(git -C "$ORIGIN" rev-parse child)" == "$CHILD_BEFORE" ]] || fail "A: child was pushed"
103+
ok "A: manual retarget detected, no branch mutation, label removed"
104+
105+
# ---------------------------------------------------------------------------
106+
echo "### Scenario B: no state marker -> no mutation"
107+
setup_repo
108+
MOCK_LABELS="autorestack-needs-conflict-resolution"
109+
MOCK_BASE="parent"
110+
MOCK_COMMENTS_FILE="$WORK/comments.txt"
111+
{ echo "### some old conflict comment with no marker"; } > "$MOCK_COMMENTS_FILE"
112+
run_resume
113+
114+
grep -q "remove-label autorestack-needs-conflict-resolution" "$CALLS" || fail "B: label not removed"
115+
grep -q -- "--base" "$CALLS" && fail "B: base must NOT be edited"
116+
[[ "$(git -C "$ORIGIN" rev-parse child)" == "$CHILD_BEFORE" ]] || fail "B: child was pushed"
117+
ok "B: missing marker handled, no branch mutation, label removed"
118+
119+
# ---------------------------------------------------------------------------
120+
echo "### Scenario C: base matches and target exists -> resume, base before label"
121+
setup_repo
122+
# Make child already contain target(main) + squash so update_direct_target is a
123+
# no-op and we exercise the push/retarget/label ordering directly.
124+
git -C "$WORK" merge -q --no-edit main
125+
git -C "$WORK" push -q origin child
126+
MOCK_LABELS="autorestack-needs-conflict-resolution"
127+
MOCK_BASE="parent" # matches marker -> not a manual retarget
128+
MOCK_COMMENTS_FILE="$WORK/comments.txt"
129+
{ echo "### conflict"; echo; marker parent main "$SQUASH"; } > "$MOCK_COMMENTS_FILE"
130+
run_resume
131+
132+
grep -q -- "pr edit child --base main" "$CALLS" || fail "C: base not retargeted to main"
133+
grep -q "remove-label autorestack-needs-conflict-resolution" "$CALLS" || fail "C: label not removed"
134+
base_line=$(grep -n -- "--base main" "$CALLS" | head -1 | cut -d: -f1)
135+
label_line=$(grep -n "remove-label" "$CALLS" | head -1 | cut -d: -f1)
136+
[[ "$base_line" -lt "$label_line" ]] || fail "C: base edit must come before label removal"
137+
ok "C: resume retargets base then removes label"
138+
139+
echo
140+
echo "All conflict-resume tests passed 🎉 ($PASS scenarios)"

‎update-pr-stack.sh‎

Lines changed: 94 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,37 @@ source "$SCRIPT_DIR/command_utils.sh"
2121

2222
CONFLICT_LABEL="autorestack-needs-conflict-resolution"
2323

24+
# Machine-readable marker embedded (invisibly) in the conflict comment so the
25+
# conflict-resolved run can recover the exact stack state. We rely on this rather
26+
# than re-deriving it from `gh pr list --head <base>`: when the base is a
27+
# long-lived integration branch that heuristic returns an unrelated ancient merge
28+
# (e.g. a years-old release merge), and it cannot tell whether a human retargeted
29+
# the PR in the meantime.
30+
STATE_MARKER_PREFIX="<!-- autorestack-state:"
31+
32+
# Args: base-branch target-branch squash-hash. Branch names and hashes contain no
33+
# spaces, so a space-separated key=value list parses back unambiguously.
34+
format_state_marker() {
35+
printf '%s base=%s target=%s squash=%s -->' \
36+
"$STATE_MARKER_PREFIX" "$1" "$2" "$3"
37+
}
38+
39+
# Echoes the most recent state-marker line found in our PR comments, or nothing.
40+
read_state_marker() {
41+
local PR_BRANCH="$1"
42+
gh pr view "$PR_BRANCH" --json comments --jq '.comments[].body' 2>/dev/null \
43+
| { grep -F "$STATE_MARKER_PREFIX" || true; } | tail -n1
44+
}
45+
46+
# Args: a marker line. Echoes "base target squash".
47+
parse_state_marker() {
48+
local LINE="$1"
49+
printf '%s %s %s\n' \
50+
"$(sed -n 's/.* base=\([^ ]*\).*/\1/p' <<<"$LINE")" \
51+
"$(sed -n 's/.* target=\([^ ]*\).*/\1/p' <<<"$LINE")" \
52+
"$(sed -n 's/.* squash=\([^ ]*\).*/\1/p' <<<"$LINE")"
53+
}
54+
2455
# Allow replacing git and gh
2556
[ -v GIT ] && git() { "$GIT" "$@"; }
2657
[ -v GH ] && gh() { "$GH" "$@"; }
@@ -128,6 +159,8 @@ update_direct_target() {
128159
echo '```'
129160
echo
130161
echo "Once you push, this action will resume and finish updating this pull request."
162+
echo
163+
format_state_marker "$MERGED_BRANCH" "$TARGET_BRANCH" "$(git rev-parse SQUASH_COMMIT)"
131164
} | log_cmd gh pr comment "$BRANCH" -F -
132165
# Create the label if it doesn't exist, then add it to the PR
133166
gh label create "$CONFLICT_LABEL" --description "PR needs manual conflict resolution" --color "d73a4a" 2>/dev/null || true
@@ -190,54 +223,73 @@ continue_after_resolution() {
190223

191224
echo "Found conflict label on $PR_BRANCH, continuing stack update..."
192225

193-
# Get the current base branch (the old base that was kept during conflict)
194-
local OLD_BASE
195-
OLD_BASE=$(gh pr view "$PR_BRANCH" --json baseRefName --jq '.baseRefName')
196-
echo "Current base branch: $OLD_BASE"
197-
198226
# The synchronize payload is the child PR, so SQUASH_COMMIT / MERGED_BRANCH /
199227
# TARGET_BRANCH from the original squash-merge run are not in the environment.
200-
# Reconstruct them from the merged parent PR: OLD_BASE is the parent branch,
201-
# and the merged PR whose head is OLD_BASE gives the new target (its base) and
202-
# the squash commit (its merge commit).
203-
local NEW_TARGET SQUASH_HASH
204-
read -r NEW_TARGET SQUASH_HASH < <(gh pr list --head "$OLD_BASE" --state merged \
205-
--json baseRefName,mergeCommit --jq '.[0] | "\(.baseRefName // "") \(.mergeCommit.oid // "")"')
206-
207-
if [[ -z "$NEW_TARGET" || -z "$SQUASH_HASH" ]]; then
208-
echo "⚠️ Could not find where '$OLD_BASE' was merged to; skipping base branch and deletion updates"
209-
# Don't update base or delete old branch - leave things as they are
210-
else
211-
echo "Old base '$OLD_BASE' was merged to '$NEW_TARGET' as $SQUASH_HASH"
212-
213-
# The squash-merge run pushed the base merge and asked the user to resolve
214-
# the pre-squash merge, but it never recorded the squash itself. Finish
215-
# that now: re-run the same merge sequence as the squash-merge path. With
216-
# the user's resolution in place the base merge and pre-squash merge are
217-
# no-ops; only the "-s ours" squash record gets applied, keeping the diff
218-
# against the new base clean. has_squash_commit makes this idempotent.
219-
log_cmd git update-ref SQUASH_COMMIT "$SQUASH_HASH"
220-
MERGED_BRANCH="$OLD_BASE"
221-
TARGET_BRANCH="$NEW_TARGET"
222-
if ! update_direct_target "$PR_BRANCH" "$NEW_TARGET"; then
223-
echo "⚠️ '$PR_BRANCH' still conflicts; re-posted the conflict comment, will retry on next push"
224-
return 1
225-
fi
226-
log_cmd git push origin "$PR_BRANCH"
228+
# Recover them from the marker the squash-merge run left in the conflict
229+
# comment.
230+
local MARKER
231+
MARKER=$(read_state_marker "$PR_BRANCH")
232+
if [[ -z "$MARKER" ]]; then
233+
echo "⚠️ No autorestack state marker on $PR_BRANCH; cannot resume safely. Removing the label."
234+
{ 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."; } \
235+
| log_cmd gh pr comment "$PR_BRANCH" -F -
236+
log_cmd gh pr edit "$PR_BRANCH" --remove-label "$CONFLICT_LABEL"
237+
return
238+
fi
227239

228-
# Remove the conflict label
240+
local RECORDED_BASE NEW_TARGET SQUASH_HASH
241+
read -r RECORDED_BASE NEW_TARGET SQUASH_HASH < <(parse_state_marker "$MARKER")
242+
echo "Recorded state: base=$RECORDED_BASE target=$NEW_TARGET squash=$SQUASH_HASH"
243+
244+
# The base we left the PR on while waiting for conflict resolution was the
245+
# merged parent branch. If it no longer matches, a human retargeted the PR
246+
# (e.g. straight onto the integration branch); we are no longer the authority
247+
# on its base, so we step back without touching the branch. Doing this BEFORE
248+
# any mutation is what stops the failure mode where we pushed a merge built
249+
# against a bogus target and then crashed.
250+
local CURRENT_BASE
251+
CURRENT_BASE=$(gh pr view "$PR_BRANCH" --json baseRefName --jq '.baseRefName')
252+
if [[ "$CURRENT_BASE" != "$RECORDED_BASE" ]]; then
253+
echo "⚠️ Base of $PR_BRANCH changed manually ($RECORDED_BASE -> $CURRENT_BASE); not updating the stack."
254+
{ echo "ℹ️ The base branch of this PR was changed manually, so autorestack stepped back and will not update it automatically."; } \
255+
| log_cmd gh pr comment "$PR_BRANCH" -F -
229256
log_cmd gh pr edit "$PR_BRANCH" --remove-label "$CONFLICT_LABEL"
257+
return
258+
fi
230259

231-
# Update the PR's base branch to the new target
232-
log_cmd gh pr edit "$PR_BRANCH" --base "$NEW_TARGET"
260+
# Defense in depth: never act on a target branch that no longer exists.
261+
if ! git rev-parse --verify --quiet "origin/$NEW_TARGET" >/dev/null; then
262+
echo "⚠️ Recorded target branch '$NEW_TARGET' no longer exists; leaving $PR_BRANCH untouched."
263+
return
264+
fi
233265

234-
# Check if old base branch should be deleted
235-
if has_sibling_conflicts "$OLD_BASE" "$PR_BRANCH"; then
236-
echo "⚠️ Keeping branch '$OLD_BASE' - still referenced by other conflicted PRs"
237-
else
238-
echo "Deleting old base branch '$OLD_BASE' (no other PRs depend on it)"
239-
log_cmd git push origin ":$OLD_BASE" || echo "⚠️ Could not delete '$OLD_BASE' (may already be deleted)"
240-
fi
266+
# The squash-merge run pushed the base merge and asked the user to resolve the
267+
# pre-squash merge, but it never recorded the squash itself. Finish that now:
268+
# re-run the same merge sequence as the squash-merge path. With the user's
269+
# resolution in place the base merge and pre-squash merge are no-ops; only the
270+
# "-s ours" squash record gets applied, keeping the diff against the new base
271+
# clean. has_squash_commit makes this idempotent.
272+
log_cmd git update-ref SQUASH_COMMIT "$SQUASH_HASH"
273+
MERGED_BRANCH="$RECORDED_BASE"
274+
TARGET_BRANCH="$NEW_TARGET"
275+
if ! update_direct_target "$PR_BRANCH" "$NEW_TARGET"; then
276+
echo "⚠️ '$PR_BRANCH' still conflicts; re-posted the conflict comment, will retry on next push"
277+
return 1
278+
fi
279+
280+
# Order matters: push the cleaned-up head, then retarget the base, and only
281+
# then drop the label. The retarget is the step that previously failed; if
282+
# anything here fails the label stays, so the next push resumes.
283+
log_cmd git push origin "$PR_BRANCH"
284+
log_cmd gh pr edit "$PR_BRANCH" --base "$NEW_TARGET"
285+
log_cmd gh pr edit "$PR_BRANCH" --remove-label "$CONFLICT_LABEL"
286+
287+
# Check if old base branch should be deleted
288+
if has_sibling_conflicts "$RECORDED_BASE" "$PR_BRANCH"; then
289+
echo "⚠️ Keeping branch '$RECORDED_BASE' - still referenced by other conflicted PRs"
290+
else
291+
echo "Deleting old base branch '$RECORDED_BASE' (no other PRs depend on it)"
292+
log_cmd git push origin ":$RECORDED_BASE" || echo "⚠️ Could not delete '$RECORDED_BASE' (may already be deleted)"
241293
fi
242294
}
243295

0 commit comments

Comments
 (0)