Stop corrupting PRs whose base changed during conflict resolution#39
Merged
Merged
Conversation
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>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The missing-target backstop returned silently, leaving the PR labeled with no explanation and no path to ever resume. It now posts a comment and drops the label like the other two dead-end cases, routed through a shared abandon_resume helper.
Both ordering comments narrated the bug being prevented ("the step that
previously failed", "the failure mode where we pushed a merge against a bogus
target and crashed"). Restate them as the rules a fresh reader needs: the base
check runs before any mutation because a diverged base makes the recorded
target stale, and the label drops last so any earlier failure stays resumable.
The marker is a new source for the base the PR is moving off of, not a new concept, so renaming OLD_BASE to RECORDED_BASE was only diff churn. It reads fine next to CURRENT_BASE.
The conflict-resolved path relies on pushing the cleaned-up head before retargeting the base, and retargeting before dropping the retry label. The focused resume test now records git commands as well as gh calls so that ordering is enforced, and the README happy path matches the same sequence.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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 likesparkthat returns an unrelated ancient merge: in run 26764629741 it picked a 2022 release merge into the deleted branchspark-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.shcovers 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.