Skip to content

Commit e80c278

Browse files
Phlogistiqueclaude
Phlogistique
and
claude
authored
Stop corrupting PRs whose base changed during conflict resolution (#39)
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](https://github.com/scortexio/sensei/actions/runs/26764629741/job/78887430408) 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. New unit test `tests/test_conflict_resolution_resume.sh` covers the three dead-end paths (missing marker, manual retarget, missing target) and the happy-path ordering: push the cleaned-up head, retarget the base, then remove the label. --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent 466d7c9 commit e80c278

4 files changed

Lines changed: 280 additions & 45 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

‎README.md‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -36,9 +36,10 @@ When a merge conflict occurs during the automatic update:
3636
After you manually resolve the conflict and push:
3737

3838
1. The push triggers the `synchronize` event
39-
2. The action detects the conflict label and removes it
39+
2. The action detects the conflict label
4040
3. Updates the PR's base branch to trunk
41-
4. Deletes the old base branch (if no other conflicted PRs still depend on it)
41+
4. Removes the conflict label
42+
5. Deletes the old base branch (if no other conflicted PRs still depend on it)
4243

4344
---
4445

‎tests/test_conflict_resolution_resume.sh‎

Lines changed: 171 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,171 @@
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 base pull
9+
# request from the target branch, which made it sensitive to humans doing
10+
# unexpected things such as changing the base branch manually.
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+
make_mock_git() {
55+
local dir="$1"
56+
cat > "$dir/mock_git.sh" <<'EOF'
57+
#!/bin/bash
58+
set -ueo pipefail
59+
echo "git $*" >> "$CALLS"
60+
exec git "$@"
61+
EOF
62+
chmod +x "$dir/mock_git.sh"
63+
}
64+
65+
# Set up a fresh repo with a bare origin so real pushes are observable.
66+
setup_repo() {
67+
WORK=$(mktemp -d)
68+
ORIGIN=$(mktemp -d)
69+
git init -q --bare "$ORIGIN"
70+
git init -q -b main "$WORK"
71+
cd "$WORK"
72+
git config user.email t@e.com && git config user.name t
73+
git remote add origin "$ORIGIN"
74+
75+
echo base > f.txt && git add f.txt && git commit -qm initial
76+
SQUASH=$(git rev-parse HEAD) # the squash commit lives on main/target
77+
git push -q origin main
78+
79+
git checkout -q -b parent && git push -q origin parent # merged parent branch
80+
git checkout -q -b child # the PR under resolution
81+
echo child >> f.txt && git add f.txt && git commit -qm child
82+
git push -q origin child
83+
CHILD_BEFORE=$(git rev-parse child)
84+
CALLS="$WORK/calls.log"; : > "$CALLS"
85+
MOCK_DIR=$(mktemp -d)
86+
make_mock_gh "$MOCK_DIR"
87+
make_mock_git "$MOCK_DIR"
88+
}
89+
90+
run_resume() {
91+
env ACTION_MODE=conflict-resolved PR_BRANCH=child \
92+
GH="$MOCK_DIR/mock_gh.sh" GIT="$MOCK_DIR/mock_git.sh" \
93+
MOCK_LABELS="$MOCK_LABELS" MOCK_BASE="$MOCK_BASE" \
94+
MOCK_COMMENTS_FILE="$MOCK_COMMENTS_FILE" CALLS="$CALLS" \
95+
bash "$ROOT_DIR/update-pr-stack.sh" >"$WORK/out.log" 2>&1 || echo "EXIT=$?" >>"$WORK/out.log"
96+
}
97+
98+
marker() { # base target squash
99+
printf '<!-- autorestack-state: base=%s target=%s squash=%s -->' "$1" "$2" "$3"
100+
}
101+
102+
# ---------------------------------------------------------------------------
103+
echo "### Scenario A: user manually retargeted the base -> no mutation"
104+
setup_repo
105+
MOCK_LABELS="autorestack-needs-conflict-resolution"
106+
MOCK_BASE="spark" # human changed it; marker says parent
107+
MOCK_COMMENTS_FILE="$WORK/comments.txt"
108+
{ echo "### conflict"; echo; marker parent main "$SQUASH"; } > "$MOCK_COMMENTS_FILE"
109+
run_resume
110+
111+
grep -q "remove-label autorestack-needs-conflict-resolution" "$CALLS" || fail "A: label not removed"
112+
grep -q "gh pr comment" "$CALLS" || fail "A: no explanatory comment posted"
113+
grep -q -- "--base" "$CALLS" && fail "A: base must NOT be edited"
114+
[[ "$(git -C "$WORK" rev-parse child)" == "$CHILD_BEFORE" ]] || fail "A: child branch was mutated"
115+
[[ "$(git -C "$ORIGIN" rev-parse child)" == "$CHILD_BEFORE" ]] || fail "A: child was pushed"
116+
ok "A: manual retarget detected, no branch mutation, label removed"
117+
118+
# ---------------------------------------------------------------------------
119+
echo "### Scenario B: no state marker -> no mutation"
120+
setup_repo
121+
MOCK_LABELS="autorestack-needs-conflict-resolution"
122+
MOCK_BASE="parent"
123+
MOCK_COMMENTS_FILE="$WORK/comments.txt"
124+
{ echo "### some old conflict comment with no marker"; } > "$MOCK_COMMENTS_FILE"
125+
run_resume
126+
127+
grep -q "remove-label autorestack-needs-conflict-resolution" "$CALLS" || fail "B: label not removed"
128+
grep -q -- "--base" "$CALLS" && fail "B: base must NOT be edited"
129+
[[ "$(git -C "$ORIGIN" rev-parse child)" == "$CHILD_BEFORE" ]] || fail "B: child was pushed"
130+
ok "B: missing marker handled, no branch mutation, label removed"
131+
132+
# ---------------------------------------------------------------------------
133+
echo "### Scenario C: base matches and target exists -> resume, push before base before label"
134+
setup_repo
135+
# Make child already contain target(main) + squash so update_direct_target is a
136+
# no-op and we exercise the push/retarget/label ordering directly.
137+
git -C "$WORK" merge -q --no-edit main
138+
git -C "$WORK" push -q origin child
139+
MOCK_LABELS="autorestack-needs-conflict-resolution"
140+
MOCK_BASE="parent" # matches marker -> not a manual retarget
141+
MOCK_COMMENTS_FILE="$WORK/comments.txt"
142+
{ echo "### conflict"; echo; marker parent main "$SQUASH"; } > "$MOCK_COMMENTS_FILE"
143+
run_resume
144+
145+
grep -q -- "git push origin child" "$CALLS" || fail "C: child not pushed"
146+
grep -q -- "pr edit child --base main" "$CALLS" || fail "C: base not retargeted to main"
147+
grep -q "remove-label autorestack-needs-conflict-resolution" "$CALLS" || fail "C: label not removed"
148+
push_line=$(grep -n -- "git push origin child" "$CALLS" | head -1 | cut -d: -f1)
149+
base_line=$(grep -n -- "--base main" "$CALLS" | head -1 | cut -d: -f1)
150+
label_line=$(grep -n "remove-label" "$CALLS" | head -1 | cut -d: -f1)
151+
[[ "$push_line" -lt "$base_line" ]] || fail "C: push must come before base edit"
152+
[[ "$base_line" -lt "$label_line" ]] || fail "C: base edit must come before label removal"
153+
ok "C: resume pushes, retargets base, then removes label"
154+
155+
# ---------------------------------------------------------------------------
156+
echo "### Scenario D: recorded target branch is gone -> give up cleanly"
157+
setup_repo
158+
MOCK_LABELS="autorestack-needs-conflict-resolution"
159+
MOCK_BASE="parent" # matches marker -> not a manual retarget
160+
MOCK_COMMENTS_FILE="$WORK/comments.txt"
161+
{ echo "### conflict"; echo; marker parent ghost-target "$SQUASH"; } > "$MOCK_COMMENTS_FILE"
162+
run_resume
163+
164+
grep -q "remove-label autorestack-needs-conflict-resolution" "$CALLS" || fail "D: label not removed"
165+
grep -q "gh pr comment" "$CALLS" || fail "D: no explanatory comment posted"
166+
grep -q -- "--base" "$CALLS" && fail "D: base must NOT be edited"
167+
[[ "$(git -C "$ORIGIN" rev-parse child)" == "$CHILD_BEFORE" ]] || fail "D: child was pushed"
168+
ok "D: missing target detected, no branch mutation, label removed"
169+
170+
echo
171+
echo "All conflict-resume tests passed 🎉 ($PASS scenarios)"

‎update-pr-stack.sh‎

Lines changed: 105 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,35 @@ 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 it recorded, instead of
26+
# re-deriving the parent PR from the PR's current base branch (which breaks when
27+
# anything about that base changes, e.g. a human retargeting the PR manually).
28+
STATE_MARKER_PREFIX="<!-- autorestack-state:"
29+
30+
# Args: base-branch target-branch squash-hash. Branch names and hashes contain no
31+
# spaces, so a space-separated key=value list parses back unambiguously.
32+
format_state_marker() {
33+
printf '%s base=%s target=%s squash=%s -->' \
34+
"$STATE_MARKER_PREFIX" "$1" "$2" "$3"
35+
}
36+
37+
# Echoes the most recent state-marker line found in our PR comments, or nothing.
38+
read_state_marker() {
39+
local PR_BRANCH="$1"
40+
gh pr view "$PR_BRANCH" --json comments --jq '.comments[].body' 2>/dev/null \
41+
| { grep -F "$STATE_MARKER_PREFIX" || true; } | tail -n1
42+
}
43+
44+
# Args: a marker line. Echoes "base target squash".
45+
parse_state_marker() {
46+
local LINE="$1"
47+
printf '%s %s %s\n' \
48+
"$(sed -n 's/.* base=\([^ ]*\).*/\1/p' <<<"$LINE")" \
49+
"$(sed -n 's/.* target=\([^ ]*\).*/\1/p' <<<"$LINE")" \
50+
"$(sed -n 's/.* squash=\([^ ]*\).*/\1/p' <<<"$LINE")"
51+
}
52+
2453
# Allow replacing git and gh
2554
[ -v GIT ] && git() { "$GIT" "$@"; }
2655
[ -v GH ] && gh() { "$GH" "$@"; }
@@ -128,6 +157,8 @@ update_direct_target() {
128157
echo '```'
129158
echo
130159
echo "Once you push, this action will resume and finish updating this pull request."
160+
echo
161+
format_state_marker "$MERGED_BRANCH" "$TARGET_BRANCH" "$(git rev-parse SQUASH_COMMIT)"
131162
} | log_cmd gh pr comment "$BRANCH" -F -
132163
# Create the label if it doesn't exist, then add it to the PR
133164
gh label create "$CONFLICT_LABEL" --description "PR needs manual conflict resolution" --color "d73a4a" 2>/dev/null || true
@@ -176,6 +207,16 @@ has_sibling_conflicts() {
176207
return 1 # No siblings with same base
177208
}
178209

210+
# Give up on resuming the stack update: tell the user why on the PR, then drop
211+
# the conflict label so this action stops re-triggering. Used for the dead-end
212+
# cases where we cannot or must not finish automatically.
213+
abandon_resume() {
214+
local PR_BRANCH="$1"
215+
local MESSAGE="$2"
216+
echo "$MESSAGE" | log_cmd gh pr comment "$PR_BRANCH" -F -
217+
log_cmd gh pr edit "$PR_BRANCH" --remove-label "$CONFLICT_LABEL"
218+
}
219+
179220
# Continue processing after user manually resolved conflicts
180221
continue_after_resolution() {
181222
check_env_var "PR_BRANCH"
@@ -190,54 +231,75 @@ continue_after_resolution() {
190231

191232
echo "Found conflict label on $PR_BRANCH, continuing stack update..."
192233

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-
198234
# The synchronize payload is the child PR, so SQUASH_COMMIT / MERGED_BRANCH /
199235
# 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"
236+
# Recover them from the marker the squash-merge run left in the conflict
237+
# comment.
238+
local MARKER
239+
MARKER=$(read_state_marker "$PR_BRANCH")
240+
if [[ -z "$MARKER" ]]; then
241+
echo "⚠️ No autorestack state marker on $PR_BRANCH; cannot resume safely. Removing the label."
242+
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."
243+
return
244+
fi
245+
246+
local OLD_BASE NEW_TARGET SQUASH_HASH
247+
read -r OLD_BASE NEW_TARGET SQUASH_HASH < <(parse_state_marker "$MARKER")
248+
echo "Recorded state: base=$OLD_BASE target=$NEW_TARGET squash=$SQUASH_HASH"
249+
250+
# The base we left the PR on while waiting for conflict resolution was the
251+
# merged parent branch. If it no longer matches, a human retargeted the PR
252+
# (e.g. straight onto the integration branch); we are no longer the authority
253+
# on its base, so we step back without touching the branch. This runs before
254+
# any mutation: once the base diverges, the recorded target is stale and a
255+
# merge built against it would be wrong.
256+
local CURRENT_BASE
257+
CURRENT_BASE=$(gh pr view "$PR_BRANCH" --json baseRefName --jq '.baseRefName')
258+
if [[ "$CURRENT_BASE" != "$OLD_BASE" ]]; then
259+
echo "⚠️ Base of $PR_BRANCH changed manually ($OLD_BASE -> $CURRENT_BASE); not updating the stack."
260+
abandon_resume "$PR_BRANCH" "ℹ️ The base branch of this PR was changed manually, so autorestack stepped back and will not update it automatically."
261+
return
262+
fi
227263

228-
# Remove the conflict label
229-
log_cmd gh pr edit "$PR_BRANCH" --remove-label "$CONFLICT_LABEL"
264+
# Defense in depth: never act on a target branch that no longer exists. The
265+
# action checks out with full history (fetch-depth: 0), so a missing origin
266+
# ref means the branch is really gone, not just unfetched; no future resume
267+
# can succeed, so give up cleanly rather than stranding the PR under the label.
268+
if ! git rev-parse --verify --quiet "origin/$NEW_TARGET" >/dev/null; then
269+
echo "⚠️ Recorded target branch '$NEW_TARGET' no longer exists; abandoning resume of $PR_BRANCH."
270+
abandon_resume "$PR_BRANCH" "ℹ️ The branch this PR was being retargeted onto (\`$NEW_TARGET\`) no longer exists, so autorestack stepped back. If this PR still needs its base updated, update its base manually."
271+
return
272+
fi
230273

231-
# Update the PR's base branch to the new target
232-
log_cmd gh pr edit "$PR_BRANCH" --base "$NEW_TARGET"
274+
# The squash-merge run pushed the base merge and asked the user to resolve the
275+
# pre-squash merge, but it never recorded the squash itself. Finish that now:
276+
# re-run the same merge sequence as the squash-merge path. With the user's
277+
# resolution in place the base merge and pre-squash merge are no-ops; only the
278+
# "-s ours" squash record gets applied, keeping the diff against the new base
279+
# clean. has_squash_commit makes this idempotent.
280+
log_cmd git update-ref SQUASH_COMMIT "$SQUASH_HASH"
281+
MERGED_BRANCH="$OLD_BASE"
282+
TARGET_BRANCH="$NEW_TARGET"
283+
if ! update_direct_target "$PR_BRANCH" "$NEW_TARGET"; then
284+
echo "⚠️ '$PR_BRANCH' still conflicts; re-posted the conflict comment, will retry on next push"
285+
return 1
286+
fi
233287

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
288+
# Drop the label last: it is what re-triggers this action, so while any
289+
# earlier step can still fail it must stay on to let the next push resume.
290+
# Push the cleaned-up head before retargeting so the head already contains
291+
# NEW_TARGET when the base flips to it, keeping the PR mergeable (GitHub
292+
# suppresses CI on a PR that conflicts with its base).
293+
log_cmd git push origin "$PR_BRANCH"
294+
log_cmd gh pr edit "$PR_BRANCH" --base "$NEW_TARGET"
295+
log_cmd gh pr edit "$PR_BRANCH" --remove-label "$CONFLICT_LABEL"
296+
297+
# Check if old base branch should be deleted
298+
if has_sibling_conflicts "$OLD_BASE" "$PR_BRANCH"; then
299+
echo "⚠️ Keeping branch '$OLD_BASE' - still referenced by other conflicted PRs"
300+
else
301+
echo "Deleting old base branch '$OLD_BASE' (no other PRs depend on it)"
302+
log_cmd git push origin ":$OLD_BASE" || echo "⚠️ Could not delete '$OLD_BASE' (may already be deleted)"
241303
fi
242304
}
243305

0 commit comments

Comments
 (0)