Skip to content

Guard stacked PRs from silent closure when deleting merged base branches - #2633

Merged
Flix6x merged 7 commits into
mainfrom
copilot/fix-merged-pr-closure-issue
Oct 1, 2026
Merged

Flix6x merged 7 commits into
mainfrom
copilot/fix-merged-pr-closure-issue

Conversation

Copilot AI commented Oct 1, 2026 •

Copy link
Copy Markdown
Contributor

Merging a stacked PR with branch deletion can silently close other open PRs that target the deleted branch as base. This updates the branch-sync guidance to prevent that failure mode and documents the correct recovery order when it already happened.

  • Pre-delete safety check for stacked branches

    • Added an explicit precondition in feature-branch-sync.instructions.md: before deleting a merged branch, check for open PRs targeting it as base.
    • Added the operational rule: if dependents exist, do not use --delete-branch; retarget or restack first.
  • Recovery path for accidentally closed stacked PRs

    • Added guidance that recovery requires restoring the deleted base branch first, then reopening the closed PR.
    • Clarified that reopening alone leaves the PR mis-based and does not restore restacking preconditions.
  • Changelog update

    • Added an Infrastructure / Support entry in documentation/changelog.rst describing the new branch-deletion safeguard.
# Pre-delete check before merging with branch cleanup
gh pr list --base <branch> --state open

Context:
- Merging with --delete-branch can silently close open PRs targeting that base branch.

Change:
- Added a pre-delete check with gh pr list and explicit guidance to skip --delete-branch when dependents exist.
- Added recovery order for accidentally closed stacked PRs: restore base branch, then reopen.

Co-authored-by: Flix6x <30658763+Flix6x@users.noreply.github.com>
@read-the-docs-community

read-the-docs-community Bot commented Oct 1, 2026 •

Copy link
Copy Markdown

Documentation build overview

📚 flexmeasures | 🛠️ Build #34884757 | 📁 Comparing 7c70b97 against latest (d0903d0)

  🔍 Preview build  

1 file changed
± api/v3_0.html

Context:
- Branch-deletion guidance changed for stacked pull requests.

Change:
- Added an Infrastructure/Support changelog item describing the new pre-delete check and why it prevents silent PR closure.

Co-authored-by: Flix6x <30658763+Flix6x@users.noreply.github.com>
Copilot AI changed the title [WIP] Fix issue with merging stacked PRs closing unrelated PRs Guard stacked PRs from silent closure when deleting merged base branches Oct 1, 2026
Copilot AI requested a review from Flix6x October 1, 2026 12:34

@Flix6x Flix6x left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't a Claude hook be appropriate, too?

Please also read #2632 (comment) for finetuning documentation

@Flix6x
Flix6x marked this pull request as ready for review October 1, 2026 18:00
@Flix6x Flix6x added the AI Infra label Oct 1, 2026
Copilot AI and others added 2 commits October 1, 2026 18:05
Context:
- Deleting in the same merge command can race stacked-PR retargeting and close dependent PRs.

Change:
- Added a PreToolUse hook that blocks gh pr merge when --delete-branch is present.
- Registered the hook in .claude/settings.json with a targeted matcher.

Co-authored-by: Flix6x <30658763+Flix6x@users.noreply.github.com>
Context:
- Follow-up feedback clarified that the failure mode is a race between retargeting and immediate branch deletion.

Change:
- Updated the branch-deletion guidance to require merging without --delete-branch, checking dependent PRs, and deleting only after the base has no dependents.
- Updated the changelog entry to reflect this refined guidance and the new Claude guard hook.

Co-authored-by: Flix6x <30658763+Flix6x@users.noreply.github.com>

Copilot AI commented Oct 1, 2026

Copy link
Copy Markdown
Contributor Author

Wouldn't a Claude hook be appropriate, too?

Please also read #2632 (comment) for finetuning documentation

Implemented in 0d35764 and 69d927f.

  • Added a Claude PreToolUse hook that blocks gh pr merge ... --delete-branch (.claude/hooks/merge-delete-branch-guard.sh) and registered it in .claude/settings.json.
  • Refined .github/instructions/feature-branch-sync.instructions.md using the race-based explanation from your linked issue comment: merge first, verify gh pr list --base <branch> --state open is empty, then delete (or let auto-delete run).

Copilot AI requested a review from Flix6x October 1, 2026 18:06
`-d` is the documented short form of `--delete-branch` on `gh pr merge`, and pflag bundles short booleans, so `-sd` and `-ds` say the same thing.
Matching only the long spelling let every short one straight through: `gh pr merge 1234 --squash -d` was allowed, which is the command the guard exists to stop.

Verified against the hook as committed: the long form, `-d`, `-sd` and `-ds` all block now, while a plain merge, `--admin`, `-t 'deadline fix'` (a value containing the letter, not a flag) and `git branch -d` are all still allowed.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015He1XE7G7HN1svS7Nxgth4
Signed-off-by: F.N. Claessen <claessen@seita.nl>

@Flix6x Flix6x left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed. Both of your requests are met: the Claude hook is here, and the instructions now carry the refined mechanism from #2632's follow-up — that merging retargets dependents and --delete-branch races that retarget, rather than deletion closing them outright. The recovery order (restore the base, then reopen) is right, and so is the note that reopening alone leaves the PR based on a branch that is still gone.

One hole, found and fixed in b8a7295

The guard matched only the long spelling. -d is the documented short form of --delete-branch on gh pr merge, and pflag bundles short booleans, so -sd and -ds say the same thing. Against the hook as it stood:

rc=2  gh pr merge 1234 --squash --delete-branch   BLOCKED
rc=0  gh pr merge 1234 --squash -d                allowed   <-- the command the guard exists to stop

I have pushed the fix rather than asking for it, since it is one line. The match now covers the long form, -d, and any bundled cluster containing d, and I checked it does not over-reach:

BLOCKED: --delete-branch, -d, -sd, -ds
allowed: plain merge, --admin, -t 'deadline fix'   (a value containing the letter, not a flag)
allowed: git branch -d x                           (not a gh pr merge at all)

One deliberate bluntness, which I would keep

The hook blocks --delete-branch unconditionally, rather than checking whether any PR actually depends on the branch. That is stricter than the rule it enforces. I would leave it: the alternative needs a gh pr list inside a 10-second hook, which means a network call that fails open exactly when the network is slow — and failing open is the whole problem. Being told to merge in two steps costs one extra command; the message says which two. Worth a line in the script saying that is a choice, not an oversight.

One question rather than a request

Does this belong in the changelog? It is an agent instruction plus a .claude hook — nothing a FlexMeasures user or operator can observe, where the entries around it in Infrastructure / Support are things like the /api/dev move. I would drop the entry and keep release notes about the product, but it is your call and I have left it in place.

Approving on the strength of the rest. Two things you have to do yourself, though: your Changes requested review still blocks the merge and only you can dismiss or supersede it, and the repository requires an approval that is not the author's — mine counts here since the author is the Copilot agent, but note I also pushed the fix above, so you may prefer to look at that one line before clearing your review.

@Flix6x Flix6x left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed remove the changelog entry.

The change is a `.claude` hook and an instruction for coding agents. Nothing a FlexMeasures user or operator can observe, where the entries around it in Infrastructure / Support are things like the `/api/dev` move. Release notes stay about the product.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015He1XE7G7HN1svS7Nxgth4
Signed-off-by: F.N. Claessen <claessen@seita.nl>
@Flix6x
Flix6x merged commit 9b211a0 into main Oct 1, 2026
13 checks passed
@Flix6x
Flix6x deleted the copilot/fix-merged-pr-closure-issue branch October 1, 2026 19:39
@Flix6x Flix6x added this to the 1.1.0 milestone Oct 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Merging a stacked PR with --delete-branch silently closes the PRs that targeted the deleted branch

2 participants