fix(pr-merge): verify the merge outcome before reporting success - #160
Merged
Conversation
`gh pr merge <n> --repo <r> --merge` on a repository with a merge queue prints its usual success line and exits 0 while adding NOTHING to the queue when an auto-merge request is already attached to the PR — the pending request swallows the enqueue. pr-merge.sh relayed that exit status as "queued (--merge, strategy set by the queue)". Measured on one PR: immediately after the call isInMergeQueue was false and the queue was empty; after `gh pr merge <n> --disable-auto` the identical call put it at position 1. Nothing else shows the failure — checks green, mergeStateStatus CLEAN, no timeline event — so the tool asserted a state that did not exist, twice, costing hours. After the merge call succeeds the script now reads the PR back via one GraphQL query (state, isInMergeQueue, autoMergeRequest.enabledBy) and reports only what it observed: merged, queued, or a failure with exit 2. A queue entry registers asynchronously, so the read is retried a bounded five times at 2s rather than trusted on its first answer or waited out with a fixed sleep. A probe that failed is kept apart from a probe that saw nothing: an unreachable API says something about the request, not about the PR, so that case reports the outcome as unknown instead of naming a cause. Where autoMergeRequest is non-null the failure names it and offers `--disable-auto` plus a retry; the script never clears it itself. Adds tests/test_pr_merge_verify_outcome.sh (10 cases, stubbed gh and pr-status.sh, no network) and documents the failure mode in references/pull-request-workflow.md next to its converse, the `autoMergeRequest: null` note. Signed-off-by: Sebastian Mendel <github@sebastianmendel.de>
|
Contributor
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
Member
Author
|
The Copilot review failed twice on |
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.



The defect
gh pr merge <n> --repo <r> --mergeon a repository with a merge queue prints its usual success line and exits 0 while adding nothing to the queue, when an auto-merge request is already attached to the PR — the pending request swallows the enqueue.pr-merge.shrelayed that exit status asqueued (--merge, strategy set by the queue).Observed twice on
netresearch/t3x-nr-llm. Measured on PR #559: immediately after the callisInMergeQueuewasfalseand the merge queue was empty; aftergh pr merge 559 --disable-autothe identical enqueue put it at position 1. Nothing else exposes the failure —gh pr checksis green,mergeStateStatusstaysCLEAN, noadded_to_merge_queuetimeline event is written, and the shell sees a zero exit. The tool asserted a state that did not exist, and that cost several hours of misdiagnosis.The change
After the merge call succeeds, the script reads the PR back with one GraphQL query for
state,isInMergeQueueandautoMergeRequest { enabledBy { login } }, and reports only what it observed:mergedwhen the state says so,queuedwhen the PR really holds a queue entry, and a failure with exit 2 otherwise. The queued branch keys onisInMergeQueuerather than on the repo-levelqueue_activeflag frompr-status.sh— an entry that exists is an entry, whatever the configuration said a moment earlier.A queue entry is registered asynchronously, so the first read can legitimately answer
falsefor an entry that lands a second later. The read is therefore retried a bounded five times at 2s (~8s worst case) rather than trusted on its first answer or waited out with a fixed sleep; it breaks as soon as the outcome is decided, so a successful merge pays one query.A probe that failed is kept apart from a probe that succeeded and saw nothing. An unreachable API says something about the request, not about the PR, so that case reports the outcome as unknown and does not name a cause — naming
--disable-autothere would send the operator to mutate a PR whose state was never read. Both cases exit 2, because the point of the block is to stop reporting an outcome that was never confirmed.Where
autoMergeRequestis non-null, the failure names the enabler and offersgh pr merge <n> --repo <r> --disable-autoplus a retry. The script does not clear the request itself: it reports, and does not mutate state the caller did not ask for.Exit-code semantics are unchanged in shape — 0 merged or queued, 1 the gate is shut and nothing was attempted, 2 needs a human — with 2 extended to cover "the call exited 0 while nothing merged and nothing entered the queue". The header comment says so.
Validation
tests/test_pr_merge_verify_outcome.sh— 10 cases against a stubbedghand a stubbedpr-status.sh, no network and no repo, ~0.6s. It covers the real enqueue, the swallowed enqueue with and without an attached auto-merge request, the non-queue merged and still-open cases, an entry that only appears on the third probe (asserting three GraphQL calls, so the retry is actually exercised), the bounded poll (five attempts, no hang), the unreachable API, and two regression guards that the gate-shut path still exits 1 without callingghand that--dry-runstill calls nothing.Because green on a first run is not evidence, the suite was also run against the pre-fix
pr-merge.shfrommain: every new assertion fails there (case 2 reportsqueuedand exits 0), and the four guards for existing behaviour pass on both.shellcheckclean on the script and the test;pre-commit run --fileson all three paths passes; the fulltests/suite (2 shell, 1 python) passes the waytests.ymlruns it.Docs
references/pull-request-workflow.mdgains the failure mode as a named subsection directly beside its converse, the existing "autoMergeRequest: nulldoes NOT mean not armed" note, with the GraphQL probe and the three readings of it. The## Then Merge: scripts/pr-merge.shsection gains two sentences saying the outcome is now read back and pointing there.