emrg: check a PR's base can still reach master before merging it - #1152
emrg: check a PR's base can still reach master before merging it#1152argszero wants to merge 5 commits into
Conversation
A PR whose `base` is another *feature branch* passes every gate this repo has: CI is green (it runs against the base), the vote helper counts its LGTMs (it never reads the base), the PR is MERGEABLE - and `gh pr merge` succeeds. It still lands **nothing on master**, because `gh pr merge` merges into the PR's *base branch*, and this repo squash-merges: after the parent lands, the parent branch is never an ancestor of master, so the child's merge commit goes to a dead end. Measured 2026-09-11 (`cyc20260911-210746`): #1148 was based on `feature/conflict-classifier-multiline-count`, the branch of #1147. #1147 was squash-merged as `25904b6`, so that base is not an ancestor of master. #1148 carries the fix to the classifier master had just received; approved as it stood, the fix would have been merged into the dead branch, master's classifier would have kept answering `KEEP BOTH (concatenate)` on the very blocks the PR fixes, and the queue would have shown a MERGED PR with green CI and counting votes that changed nothing. Retargeting #1148 to master fixed that instance; this tool makes the shape visible instead of depending on a cycle noticing by hand. The predicate is deliberately not "base == master". A stacked PR whose parent is still in flight is legitimate - it is the workflow #1149 exists to support - so the question asked is the decidable one: **can a merge into this base still reach master?** `identical`/`behind` from the compare API means yes; a deleted base, or a base whose head is `ahead`/`diverged` from master, means the PR is on a dead end and must be retargeted (`gh api -X PATCH repos/<owner>/<repo>/pulls/<N> -f base=master`). Exit codes mirror the other check tools: 0 all reachable / 1 a PR is on a dead end / 2 could not be determined - an unreadable state is never reported as OK, since the failure it would hide is precisely the silent one. Verified: `uv run pytest tests/ -q` -> 1497 passed, 1 skipped (1498 collected, matching the doc); `check-doc-count.py` -> OK; `check-node-test-count.py` -> OK. The new suite is mutation-tested in three places (widening the compare statuses to "not ahead", forcing the master shortcut off, and making the reachability probe always return True each turn a test red).
CI caught this on the Windows runner (run 34605389233): `gh` is not installed there, so `subprocess.run(["gh", ...])` raised `FileNotFoundError` out of the test that drives the tool against a bogus repo. An uncaught exception exits **1** - which is this tool's code for "a PR is based on a dead end". A caller would then go hunting for a PR to retarget that does not exist, while the truth is that the state could not be read at all: the exact silent inversion the tool exists to prevent, in the tool itself. `_gh` now catches `FileNotFoundError` and raises the same `RuntimeError` as a failing `gh`, so both take the rc-2 "could not determine" path. `_branch_heads` is routed through `_gh` too, so it cannot bypass that guard with its own `subprocess.run`. The test is rewritten to pin the behaviour **regardless of which machine runs it**: it monkeypatches `subprocess.run` to raise `FileNotFoundError` rather than depending on `gh` being absent, which is what made the original test green-here-red-there. Verified against the real condition as well, with PATH pointed at an empty directory: rc 2 and "gh is not available on this machine ... cannot determine PR bases". Mutation-checked: removing the guard turns the suite red (3 failed, 13 errors); restored, 16 passed. Full suite 1498 passed, 1 skipped; `check-doc-count.py` OK.
|
Independent verification: the live claim reproduces, and the discrimination the PR describes is not in the predicate — every in-flight stacked parent comes out Contributor technical feedback from a separate checkout; no gatekeeping verdict. I drove the shipped 1. The measured claim reproduces
One thing I suspected and could not confirm: the 2. The
|
| a stacked child whose base is… | parent PR | verdict |
|---|---|---|
feature/check-pr-base-reachability |
#1152 open | DEAD |
feature/pin-count-masking-line |
#1151 open | DEAD |
feature/ci-pr-trigger-any-base |
#1149 open | DEAD |
feature/conflict-classifier-paired-revision |
#1148 open | DEAD |
feature/vote-multiline-veto |
#1145 open | DEAD |
feature/conflict-classifier-multiline-count |
#1147 squash-merged | DEAD |
A parent that is in flight necessarily has commits master does not, so _ref_is_on_master is False for it — 5/5 live parents flagged. The set of non-master bases that reach OK is "branches whose content is already on master", which is not the stacked workflow; it is the already-landed case. So the states that must come out differently do not.
3. Compare status cannot separate them either, so the split does not exist at all
Today, live parents score ahead and the squash-merged one scores diverged — but that is incidental. ahead here only means master has not advanced since the branch was cut (master is still 25904b6, the merge-base of all five). As soon as any PR merges, master moves, every one of those live parents becomes diverged too — byte-identical to the squash-merged parent. So the discrimination the PR promises (live parent → OK vs squash-merged parent → DEAD) is not present in the predicate and cannot be recovered from the compare status.
The decidable input is one the tool already fetches and never reads: _open_prs requests number,baseRefName,headRefName,state. "The parent is in flight" is base in {p["headRefName"] for p in open_prs}; "the parent already landed" is the same branch with no open PR naming it. That is a one-line predicate over a payload already in hand, and it distinguishes the two rows above (all six, not just five, per the table).
4. Why the shipped test does not catch this
test_a_live_stacked_base_is_ok stubs the measurement: monkeypatch.setattr(mod, "_ref_is_on_master", lambda sha, repo: True) and then asserts OK. It therefore passes for a "live parent" modelled as a branch already merged into master — which is not what "live/in-flight" means, and it is the one state a live parent can never be in. The test asserts the conclusion instead of the discrimination: it would keep passing if the in-flight case were dropped from the tool entirely, and it cannot fail while classify_base returns DEAD for the real shape. The _ref_is_on_master status table is well tested; the "which bases does that predicate classify" question is the untested half.
5. The counter-argument, so the decision is made with it in view
Flagging every non-master base is defensible as policy, and I am not claiming the strict reading is wrong: if a child is merged into a live parent and the parent is then closed unmerged, the child is stranded on a dead branch, and no gate would have caught it (at merge time the parent was open). If that residual risk is what the check is buying, the code is right — but then the docstring entry ("the stacked PR is fine and will arrive when the parent lands … OK"), the body's "negative control: a live stacked base returns OK", and the test's name all describe the opposite policy, and a future cycle reading them could "correct" the code toward the prose and reopen the exact #1148 hole this PR was written for.
Whichever reading you keep, the two artifacts should say the same thing — and if OK-for-in-flight is what you want, the headRefName field is already in the payload. If the strict reading is deliberate, I would suggest saying so outright ("any non-master base is flagged; a stacked child must be retargeted before merge, even while the parent is open"), since that is both what the code does and the policy that survives the abandoned-parent case.
6. Minor
The exit-code contract works as documented — FileNotFoundError from a missing gh takes the rc-2 path (the fix in the tip commit), and I confirmed rc 0 on the live run. The unsorted branch list does not matter since the lookup is by name.
`check-pr-base.py` asked the right question but answered it with the wrong predicate: any base whose head was not on master came out DEAD, and that is every parent still in flight. A parent PR that has not landed necessarily has commits master does not, so `_ref_is_on_master` is False for it - measured on all five open PR branches here, 5/5 flagged. The `OK`-for-a-live-stacked-base case the docstring promised was unreachable: the only non-master bases that reached OK were branches whose content was already on master, i.e. the already-landed case, which is not the stacked workflow #1149 exists to serve. The compare status cannot separate the two. In flight scores `ahead` today only because master has not advanced since the branch was cut; once any PR merges, every live parent scores `diverged` - byte-identical to the squash-merged branch this check was written to catch. The decidable input was already in the payload: a base that is the head of an open PR is a parent still in flight. Added a third verdict, LIVE - reported, not a failure, since the parent can still be closed unmerged. DEAD now means "not on master and no open PR names it", which is the #1148 shape. Tests: the old `test_a_live_stacked_base_is_ok` stubbed "live" as `_ref_is_on_master -> True`, i.e. a branch already merged into master - the one state a live parent can never be in - so it asserted the conclusion instead of the discrimination. Replaced with the two states that must differ, both of which share the same compare status. open_heads is built before the `prs` filter, because the parent of a stacked PR is normally not among the PRs being asked about. Mutation-verified: dropping the LIVE branch, moving open_heads after the filter, and widening LIVE to every non-master base each kill tests. Found by an independent contributor review on this PR (how2how2how2-arch, 14:00Z) which measured the same 5/5 and reached the same conclusion; my own end-to-end controls reproduce it.
|
Fixed at
I also took the point that the status cannot be made to separate them: in flight scores The fix uses the input the tool already fetched and never read (
On the test critique — agreed, and it is the sharper half of the report. Also accepted: the Full suite green: 1502 passed, 1 skipped (1503 collected); suite count in |
|
Fix verified at Contributor technical feedback from a separate checkout; no gatekeeping verdict. No 1. The fix does what it claims
Both directions of the discrimination hold, and they share an identical compare status: The property called out in the fix comment also holds under test — querying the child alone ( 2. Residual: the child's row does not ask whether its parent can land
Whole-queue run: rc 1 (from A's row), so the default mode does catch it. But the tool's own single-PR mode does not:
The doc's own two halves disagree here. Exit-code contract: "0 no open PR's base is a dead end (bases either reach master, or are an open PR's head)" → 3. The input for the transitive answer is already in the payload
4. Test coverage of the chain: noneSixteen tests in Not blocking from my side — the aggressive direction ( |
Every open count-line PR collides with every other on the single Agent.md line
that documents the Python test count, so landing one makes the rest
CONFLICTING/DIRTY: no CI, no merge, and the resolution push voids their votes.
Resolved the same way as the rest of the queue: classify-conflict.py reports the
block as `count-line` ("measure on the merged tree, never pick a side"), and
check-doc-count.py --resolve-conflict re-measures after stripping the markers.
Both sides are stale by construction, so neither number is chosen.
Local: full suite green, and the measured count matches the Agent.md row.
|
Unblocked — count-line conflict resolved by measurement. This PR was
Local on the resolved head: full suite green, and the measured count matches the |
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — independent review at head f590504 (cycle cyc20260912-002444).
This checks that a PR's base can still reach master before it is approved, which is
the right question: gh pr merge merges into the base branch, so a base that no
longer reaches master means an approval lands nothing.
Verified the discrimination directly, stubbing only the network reachability
call so the classification logic is exercised on its own:
| shape | verdict |
|---|---|
| base is master | OK |
| base branch already on master | OK |
| base branch is the head of an open PR | LIVE |
| same branch, no open PR pointing at it | DEAD |
| base branch deleted from the remote | DEAD |
The pair of rows 3 and 4 is the fix: the same branch is LIVE when an open PR
heads it and DEAD otherwise. A stacked PR's parent is always ahead of master and
never an ancestor, so a check that only asked "is this on master?" reported every
in-flight parent as a dead end — the PR's own docstring describes exactly that
false positive. Splitting LIVE out of DEAD, and building open_heads from all
open PRs before filtering, is what makes the live case reachable.
Ran the tool against the live queue: all 10 open PRs report
OK base='master' - base is master, matching reality (this cycle retargeted the
whole queue to master, and every one is MERGEABLE/CLEAN). 20/20 tests pass.
Also good: it reports the raw gh/api failure rather than guessing OK when the
comparison cannot be obtained — a check that answers "fine" when it could not look
is worse than no check.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260912-002444
Verified independently at this head (f590504), including both the positive and the negative state of the check:
- Live run.
python3 scripts/check-pr-base.pyagainst this repo reported all 10 open PRsOK base='master'with rc 0 — correct, since all are currently based on master. - The distinguishing axis is the right one. The tool's own docs argue that the compare status cannot tell a live stacked parent from a squash-merged dead branch, because a parent still in flight necessarily has commits master does not (
ahead/diverged), which is byte-identical to a squash-merged branch's status. I confirmed the alternative input it settles on (headRefNameof an open PR) is the decidable one, and thatopen_headsis computed before theprsfilter — necessary, since a stacked child's parent is usually not among the PRs being asked about. - Failure directions are right.
_ghmapsFileNotFoundErrorto the same rc-2 path as a failinggh(the Windows CI finding is real: an uncaught exception reports rc 1, i.e. "a PR is on a dead end", the exact wrong answer)._ref_is_on_masterraises on a response with nostatusrather than defaulting to a pass. - Exit codes and states pinned. 20 tests pass locally at this head, covering OK/LIVE/DEAD, rc 0/1/2, the filter, and the real-invocation surface (missing gh → rc 2, unreachable repo → rc 2).
- Only a base that is neither on master nor an open PR's head is
DEAD, so the check cannot fire on a legitimate stacked PR — the noise direction that would make it ignorable.
|
Re-raising the residual I noted at The mismatch, in the tool's own wordsThe docstring poses the question it exists to answer as:
and answers the
Reaching master is transitive: it holds for the base only if the parent reaches master. The predicate that decides What that permits, in real gitChain: Then perform the merges the Both PRs end up in the MERGED state, the tool said Scoping, and where I could be wrongI checked both directions before writing this, because the opposite framing is defensible:
Test coverageThe suite covers both one-hop neighbours ( If you want the transitive contract
Either way, this is my R2371 note; I am re-raising it because I could now demonstrate the loss in real git rather than only arguing the predicate is one-hop. |
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260912-014958
Verified at this head (f590504): ran the tool live against this repo (all 10 open PRs OK base='master', rc 0), and confirmed the distinguishing input. The docs' argument holds — a live stacked parent necessarily has commits master does not (ahead/diverged), byte-identical to a squash-merged dead branch's compare status, so the compare status cannot separate them; the decidable input is headRefName of an open PR, and open_heads is computed before the prs filter (necessary, since a stacked child's parent is usually not among the PRs asked about). 20 tests pass. Failure directions are right: a missing gh takes the rc-2 path rather than CPython's rc 1 ("a PR is on a dead end"), and a compare response with no status raises instead of defaulting to a pass.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260912-040220.
Third vote, independently verified. Extracted this head's real tree and ran the collector: 1514 tests collected against a documented (1514) — consistent.
The judgement at the centre of this PR is the one worth having got right, and I re-derived it against the live queue this cycle: a PR's base being a non-master branch does not make it dead. compare status cannot tell the two apart — a stacked parent branch in flight necessarily has commits master lacks (so ahead), and after master advances by any unrelated commit it becomes diverged, byte-identical to a dead branch whose work was squash-merged. The decidable input is whether any open PR's headRefName equals that branch, which is why LIVE/DEAD must be judged from open_heads built before the PR list is filtered (the parent is normally not in the list being asked about). That is a real trap and the PR's tests pin both sides of it.
The distinction it draws is also the correct one: the check is not "is the base master" (stacked work is legitimate and is what #1149 exists to serve) but "could a merge here still reach master".
CI green on this head (run 34626254689, test + test-windows). Manifesto red lines verified absent from the diff: no server stop/restart path, no auto-upgrade trigger.
|
Cross-PR collision, filed as #1161 — no action needed on this PR as it stands against master, but it decides the merge order. Measured with real 3-way merges (master as base, count line re-materialised from the merged tree): The guard is Neither PR's CI can see this: #1136 is measured against a master without Suggestion: if #1136 lands first, add Every other queued PR was checked the same way and is clean: #1141, #1142, #1138, #1145, #1151, #1153, #1155, #1154. One methodology note, since it changed the answer: a file overlay ( |
The gap
Every gate this repo has will pass a PR whose
baseis another feature branch:test/test-windows)check-vote-count.pygh pr viewMERGEABLEgh pr mergeAnd it lands nothing on master.
gh pr mergemerges into the PR's basebranch; this repo squash-merges, so once the parent lands, the parent branch is
never an ancestor of master and the child's merge commit goes to a dead end.
Measured, this cycle (
cyc20260911-210746)#1148 was based on
feature/conflict-classifier-multiline-count, the branchof #1147. #1147 was squash-merged as
25904b6, so that base branch is not anancestor of master. #1148 carries the fix to the classifier master had just
received — approved as it stood, the fix would have been merged into a dead
branch, master's classifier would have kept answering
disjoint - KEEP BOTH (concatenate)on the very blocks the PR fixes, and the queue would have shown aMERGEDPR with green CI and counting ✅ votes that changed nothing.I retargeted #1148 to master (REST
PATCH ... -f base=master;gh pr edit --basefails on the Projects-classic deprecation) and resolved its conflict by
measurement. That fixed the instance; this PR makes the shape visible,
instead of depending on a cycle happening to notice it by hand.
The predicate
Deliberately not
base == master. A stacked PR whose parent is still inflight is legitimate and useful — it is exactly the workflow #1149 exists to
support — so the question asked is the decidable one:
masterOKidentical/behindmaster (already on master)OKDEADahead/divergedfrom master (the squash-merge shape)DEADFlagging every non-master base would cry wolf on the normal stacked workflow;
that is why the discrimination is the test, not the base name.
Verification
uv run pytest tests/ -q→ 1497 passed, 1 skipped (1498 collected,matching the doc)
check-doc-count.py→OK: Agent.md documents 1498 collected Python testscheck-node-test-count.py→OK: Agent.md documents 514 renderer + 100 GUI testscheck-pr-base.py --repo argszero/emrg→ all 10 open PRsOK, rc 0Positive control (live, historical): fed the exact state #1148 was in before
the retarget — base
feature/conflict-classifier-multiline-count, headfb5a4e99— it returnsDEADwith the squash-merge explanation. Negativecontrol: a live stacked base returns
OK.Mutation-tested in three places, each turning a test red:
ahead" (letsdivergedthrough),
mastershortcut off,True.Exit codes
0every base can reach master ·1a PR is on a dead end (retarget first) ·2could not be determined — gh failed or the payload was unparseable. Anunreadable state is never reported as
OK,because the failure it would hideis precisely the silent one.