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/README.md b/README.md index f65c111..f55449e 100644 --- a/README.md +++ b/README.md @@ -36,9 +36,10 @@ When a merge conflict occurs during the automatic update: After you manually resolve the conflict and push: 1. The push triggers the `synchronize` event -2. The action detects the conflict label and removes it +2. The action detects the conflict label 3. Updates the PR's base branch to trunk -4. Deletes the old base branch (if no other conflicted PRs still depend on it) +4. Removes the conflict label +5. Deletes the old base branch (if no other conflicted PRs still depend on it) --- diff --git a/tests/test_conflict_resolution_resume.sh b/tests/test_conflict_resolution_resume.sh new file mode 100644 index 0000000..3f0956a --- /dev/null +++ b/tests/test_conflict_resolution_resume.sh @@ -0,0 +1,171 @@ +#!/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 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 + +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" +} + +make_mock_git() { + local dir="$1" + cat > "$dir/mock_git.sh" <<'EOF' +#!/bin/bash +set -ueo pipefail +echo "git $*" >> "$CALLS" +exec git "$@" +EOF + chmod +x "$dir/mock_git.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" + make_mock_git "$MOCK_DIR" +} + +run_resume() { + env ACTION_MODE=conflict-resolved PR_BRANCH=child \ + 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" \ + 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, push before 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 -- "git push origin child" "$CALLS" || fail "C: child not pushed" +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" +push_line=$(grep -n -- "git push origin child" "$CALLS" | head -1 | cut -d: -f1) +base_line=$(grep -n -- "--base main" "$CALLS" | head -1 | cut -d: -f1) +label_line=$(grep -n "remove-label" "$CALLS" | head -1 | cut -d: -f1) +[[ "$push_line" -lt "$base_line" ]] || fail "C: push must come before base edit" +[[ "$base_line" -lt "$label_line" ]] || fail "C: base edit must come before label removal" +ok "C: resume pushes, retargets base, then removes label" + +# --------------------------------------------------------------------------- +echo "### Scenario D: recorded target branch is gone -> give up cleanly" +setup_repo +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 ghost-target "$SQUASH"; } > "$MOCK_COMMENTS_FILE" +run_resume + +grep -q "remove-label autorestack-needs-conflict-resolution" "$CALLS" || fail "D: label not removed" +grep -q "gh pr comment" "$CALLS" || fail "D: no explanatory comment posted" +grep -q -- "--base" "$CALLS" && fail "D: base must NOT be edited" +[[ "$(git -C "$ORIGIN" rev-parse child)" == "$CHILD_BEFORE" ]] || fail "D: child was pushed" +ok "D: missing target detected, no branch mutation, label removed" + +echo +echo "All conflict-resume tests passed 🎉 ($PASS scenarios)" diff --git a/update-pr-stack.sh b/update-pr-stack.sh index c000f41..3676a9e 100755 --- a/update-pr-stack.sh +++ b/update-pr-stack.sh @@ -21,6 +21,35 @@ 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 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="' \ + "$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 +157,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 @@ -176,6 +207,16 @@ has_sibling_conflicts() { return 1 # No siblings with same base } +# Give up on resuming the stack update: tell the user why on the PR, then drop +# the conflict label so this action stops re-triggering. Used for the dead-end +# cases where we cannot or must not finish automatically. +abandon_resume() { + local PR_BRANCH="$1" + local MESSAGE="$2" + echo "$MESSAGE" | log_cmd gh pr comment "$PR_BRANCH" -F - + log_cmd gh pr edit "$PR_BRANCH" --remove-label "$CONFLICT_LABEL" +} + # Continue processing after user manually resolved conflicts continue_after_resolution() { check_env_var "PR_BRANCH" @@ -190,54 +231,75 @@ 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." + 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." + return + fi + + local OLD_BASE NEW_TARGET SQUASH_HASH + read -r OLD_BASE NEW_TARGET SQUASH_HASH < <(parse_state_marker "$MARKER") + echo "Recorded state: base=$OLD_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. This runs before + # any mutation: once the base diverges, the recorded target is stale and a + # merge built against it would be wrong. + local CURRENT_BASE + CURRENT_BASE=$(gh pr view "$PR_BRANCH" --json baseRefName --jq '.baseRefName') + if [[ "$CURRENT_BASE" != "$OLD_BASE" ]]; then + echo "⚠ī¸ Base of $PR_BRANCH changed manually ($OLD_BASE -> $CURRENT_BASE); not updating the stack." + abandon_resume "$PR_BRANCH" "ℹī¸ The base branch of this PR was changed manually, so autorestack stepped back and will not update it automatically." + return + fi - # Remove the conflict label - log_cmd gh pr edit "$PR_BRANCH" --remove-label "$CONFLICT_LABEL" + # Defense in depth: never act on a target branch that no longer exists. The + # action checks out with full history (fetch-depth: 0), so a missing origin + # ref means the branch is really gone, not just unfetched; no future resume + # can succeed, so give up cleanly rather than stranding the PR under the label. + if ! git rev-parse --verify --quiet "origin/$NEW_TARGET" >/dev/null; then + echo "⚠ī¸ Recorded target branch '$NEW_TARGET' no longer exists; abandoning resume of $PR_BRANCH." + 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." + return + fi - # Update the PR's base branch to the new target - log_cmd gh pr edit "$PR_BRANCH" --base "$NEW_TARGET" + # 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 - # 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 + # Drop the label last: it is what re-triggers this action, so while any + # earlier step can still fail it must stay on to let the next push resume. + # Push the cleaned-up head before retargeting so the head already contains + # NEW_TARGET when the base flips to it, keeping the PR mergeable (GitHub + # suppresses CI on a PR that conflicts with its base). + 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 "$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 }