fix: identify an orphan's PR by head SHA, not by branch name - #144
Merged
Conversation
The orphan-cleanup safety net could not actually clean anything. Its squash-merge check was `git diff trunk...branch` being empty, which only holds for a branch that has been *restacked* onto current trunk — and an orphan is by definition not restacked, so the diff still shows its own changes long after they landed. Verified against two real squash-merged orphans: both were reported as "not merged" and skipped. A safety net that never fires is a no-op. The check that does work is identity. `get_pr_statuses_for` resolves `headRefName`, so it returns whatever PR most recently used a branch *name* — that is why its `merged` flag could not be trusted on its own. Selecting `headRefOid` alongside it turns the name match into an identity match: if the merged PR's head commit is this branch's tip, it really is this branch's PR. - `PrInfo` gains `head_oid`, populated from GraphQL, `gh pr view`, and the REST shape; empty where the source does not report one, which never counts as proof. - `orphan_work_is_in_trunk` is a pure function so the rule is directly testable, including the case that motivated it: a merged PR whose head is some other commit is not evidence about the branch in this worktree. Re-verified end to end: both real orphans are now cleaned, and a worktree with uncommitted changes is still protected.
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
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.
Follow-up to #143. The orphan-cleanup safety net it added could not actually clean anything.
The bug
The squash-merge arm of the merged check was
git diff trunk...branchbeing empty. That range meansdiff(merge-base(trunk, branch), branch)— it goes empty only for a branch that has been restacked onto current trunk, because then the merge-base is the trunk tip. An orphan is by definition not restacked, so its merge-base is the old fork point and the diff still shows all of its own changes long after they landed on trunk.Verified against the two real orphans this feature exists for — both squash-merged via #141 and #142:
is_ancestorfalse (squash rewrites the commits), diff non-empty (not restacked) → never cleaned. A safety net that never fires is a no-op.The fix
The right check is identity, not content. #143 correctly established that
get_pr_statuses_forresolvesheadRefNameand so returns whatever PR most recently used a branch name — which is why itsmergedflag can't be trusted alone. SelectingheadRefOidalongside it converts that name match into an identity match: if the merged PR's head commit is this branch's tip, it really is this branch's PR.PrInfogainshead_oid, populated from all three sources (GraphQL aliased query,gh pr view, REST). Empty where a source doesn't report one, which never counts as proof.orphan_work_is_in_trunkis pure, so the rule is unit-testable — including the case that motivated the whole guard: a merged PR whose head is a different commit is not evidence about the branch in this worktree.This keeps every safety property from #143's review and adds the one that makes the feature work.
Verification
Built and run against the actual orphans:
A worktree with uncommitted changes in the same run was still correctly refused.
491 unit tests + all integration suites green, clippy clean. Test fixtures asserting the
gh pr view --jsonfield list were updated for the added field.