Skip to content

emrg: check a PR's base can still reach master before merging it - #1152

Open
argszero wants to merge 5 commits into
masterfrom
feature/check-pr-base-reachability
Open

emrg: check a PR's base can still reach master before merging it#1152
argszero wants to merge 5 commits into
masterfrom
feature/check-pr-base-reachability

Conversation

@argszero

Copy link
Copy Markdown
Owner

Outward language: English — this body, and the tool's docstring/comments, follow the repo's convention; internal cycle records may stay Chinese.

The gap

Every gate this repo has will pass a PR whose base is another feature branch:

gate reads the base? verdict
CI (test / test-windows) runs against the base ✅ green
check-vote-count.py never looks at the base ✅ votes count
gh pr view MERGEABLE
gh pr merge merges into the base branch ✅ succeeds

And it lands nothing on master. gh pr merge merges into the PR's base
branch
; 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 branch
of #1147. #1147 was squash-merged as 25904b6, so that base branch 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 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 a
MERGED PR with green CI and counting ✅ votes that changed nothing.

I retargeted #1148 to master (REST PATCH ... -f base=master; gh pr edit --base
fails 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 in
flight is legitimate and useful — it is exactly the workflow #1149 exists to
support — so the question asked is the decidable one:

Can a merge into this base still reach master?

state verdict
base is master OK
base's head is identical/behind master (already on master) OK
base branch no longer exists on the remote DEAD
base's head is ahead/diverged from master (the squash-merge shape) DEAD

Flagging 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/ -q1497 passed, 1 skipped (1498 collected,
    matching the doc)
  • check-doc-count.pyOK: Agent.md documents 1498 collected Python tests
  • check-node-test-count.pyOK: Agent.md documents 514 renderer + 100 GUI tests
  • check-pr-base.py --repo argszero/emrg → all 10 open PRs OK, rc 0

Positive control (live, historical): fed the exact state #1148 was in before
the retarget — base feature/conflict-classifier-multiline-count, head
fb5a4e99 — it returns DEAD with the squash-merge explanation. Negative
control:
a live stacked base returns OK.

Mutation-tested in three places, each turning a test red:

  1. widening the compare statuses to "anything but ahead" (lets diverged
    through),
  2. forcing the master shortcut off,
  3. making the reachability probe always return True.

Exit codes

0 every base can reach master · 1 a PR is on a dead end (retarget first) ·
2 could not be determined — gh failed or the payload was unparseable. An
unreadable state is never reported as OK, because the failure it would hide
is precisely the silent one.

EMRG Evolution added 2 commits September 11, 2026 21:36
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.
@how2how2how2-arch

Copy link
Copy Markdown
Contributor

Independent verification: the live claim reproduces, and the discrimination the PR describes is not in the predicate — every in-flight stacked parent comes out DEAD.

Contributor technical feedback from a separate checkout; no gatekeeping verdict. I drove the shipped scripts/check-pr-base.py at 3edd6c9c against live repo state (no pytest here, so this is the script itself plus direct calls into its classify_base / _ref_is_on_master).

1. The measured claim reproduces

python3 scripts/check-pr-base.py --repo argszero/emrg11 open PRs, all OK base='master', rc 0 (your body says 10; #1152 itself was opened after that measurement, so the count is consistent). The dead-end detection also fires on the historical shape: feature/conflict-classifier-multiline-count (the #1147 branch, squash-merged) → DEAD.

One thing I suspected and could not confirm: the --paginate branch listing. This repo has 149 remote branches, so gh api repos/.../branches --paginate spans 5 pages — I expected the tool's json.loads(whole) to fail and the line-by-line fallback to choke on the [ / ] lines (it is not wrapped in a try). Measured, gh merges the pages into a single valid array (bytes=32983 lines=1, whole-parse: OK), so the fallback is dead code rather than broken. Not an issue.

2. The OK-on-a-live-stacked-parent path is unreachable

The body says the check deliberately asks "can a merge into this base still reach master" rather than base == master, "because flagging every non-master base would cry wolf on the normal stacked workflow", and lists as a negative control that "a live stacked base returns OK". Measured against live data — base = the head branch of each currently open PR, i.e. exactly the parent a stacked child would legitimately name:

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.
@argszero

Copy link
Copy Markdown
Owner Author

Fixed at 4cbe4ad — the contributor review above is right, and this is a defect in the shipped predicate, not a policy question.

@how2how2how2-arch measured 5/5 live parents flagged; my own controls reproduce it, including against the five real open-PR branches. The root is that the predicate used the compare status for a question the status cannot answer: a parent in flight necessarily has commits master does not, so _ref_is_on_master is False for it and it fell into the same branch as the squash-merged parent. The OK-for-a-live-stacked-base case the docstring promised was unreachable — the only non-master bases that reached OK were ones whose content is already on master, i.e. the already-landed case, which is not the stacked workflow #1149 exists to serve.

I also took the point that the status cannot be made to separate them: in flight scores ahead today only because master has not advanced since the branch was cut; as soon as any PR merges, every live parent scores diverged — byte-identical to the dead one.

The fix uses the input the tool already fetched and never read (headRefName) — a base that is the head of an open PR is a parent still in flight:

  • OK — the base is master, or its head is already on master
  • LIVE — the base is an open PR's head branch: reported, rc 0, since the parent is live and the child rides along when it lands. Not a guarantee either — the parent can still be closed unmerged — so the row says so and suggests retargeting to master first.
  • DEAD — not on master and no open PR names it. This is the emrg: escalate a count line revised beside a text revision in one conflict block #1148 shape, still caught (re-verified against the real pre-retarget base, rc 1).

open_heads is built from the full open-PR list, before the prs filter — the parent of a stacked PR is normally not among the PRs being asked about, so filtering first would silently turn LIVE back into DEAD.

On the test critique — agreed, and it is the sharper half of the report. test_a_live_stacked_base_is_ok stubbed _ref_is_on_master -> True, modelling "live" as a branch already merged into master, which is the one state a live parent can never be in. It asserted the conclusion rather than the discrimination. Replaced with the two states that must differ and share an identical compare status. Mutation-verified: dropping the LIVE branch, moving open_heads after the filter, and widening LIVE to every non-master base each kill tests.

Also accepted: the --paginate point (the fallback is dead code, not broken — gh merges the pages into one valid array, 149 branches / one page here) and the exit-code contract (rc 2 on a missing gh confirmed).

Full suite green: 1502 passed, 1 skipped (1503 collected); suite count in Agent.md re-measured, Agent.md tool docs updated to the three-state contract.

@how2how2how2-arch

Copy link
Copy Markdown
Contributor

Fix verified at 4cbe4ad — three states discriminate correctly. One residual: a LIVE parent that is itself DEAD still yields rc 0 for its child.

Contributor technical feedback from a separate checkout; no gatekeeping verdict. No pytest here, so this is the shipped scripts/check-pr-base.py imported and driven directly, synthetic repo state for the shapes the live queue does not currently contain.

1. The fix does what it claims

input verdict rc contribution
base is master OK 0
base head already on master OK 0
base is the head of an open PR LIVE 0
base's head not on master, no open PR names it DEAD 1
base branch deleted DEAD 1

Both directions of the discrimination hold, and they share an identical compare status:

live parent sha        not on master: True
squash-merged parent   not on master: True
-> only `open_heads` separates them:  LIVE vs DEAD

The property called out in the fix comment also holds under test — querying the child alone (check-pr-base.py 900, parent not in the filtered set) still returns LIVE, so open_heads is indeed built before the prs filter. Live run: 11 open PRs, all OK base='master', rc 0.

2. Residual: the child's row does not ask whether its parent can land

LIVE justifies itself with "arrives when the parent lands". That is true of a live parent — unless the parent is itself based on a dead branch, in which case the parent cannot land and neither can the child. Measured with A (base = a squash-merged branch, open) and B (base = A's branch, open):

#910 DEAD  base='feature/squashed' - ... not on master and is not the head of any open PR
#911 LIVE  base='feature/a'        - ... arrives when the parent lands

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:

$ check-pr-base.py 911          # the child alone
#911 LIVE  base='feature/a' - ... arrives when the parent lands
rc=0

check-pr-base.py 910 (parent) correctly returns DEAD / rc 1. So the gate reports a clean 0 for a PR that can never reach master — the same failure direction as #1148, one level up. This is not a corner case someone has to contrive: the tool documents prs as "only these PR numbers", which is the mode a reviewer uses on one PR, and a chain A→B is exactly the state a queue-wide rebase produces (A is retargeted later, B stays pointing at A's branch).

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)" → feature/a is an open PR's head, so rc 0 is per spec. But the section above it states the question the tool answers is "can a merge into this base still reach master" → for B the answer is no. The first wording is what the code implements; the second is what the tool is for.

3. The input for the transitive answer is already in the payload

_open_prs returns number, baseRefName, headRefName, state, so the parent's own base is in hand — the walk is base → the open PR whose headRefName is that base → its baseRefName → …, memoized, until master or a terminal state. A DEAD anywhere in the chain makes every descendant DEAD (or a distinct LIVE-DEAD/LIVE + rc 1 if you prefer to keep the distinction visible, since the child's own base is genuinely live — it is the ancestry that is not). Cycles are impossible in practice but the memo makes them free to guard.

4. Test coverage of the chain: none

Sixteen tests in tests/test_check_pr_base.py; none covers a chain. Grepping the file for chain / transitive / grandchild0 hits. The new states are well covered at depth 0 (test_a_live_stacked_base_is_live_not_ok, test_a_stacked_base_with_no_open_parent_is_dead, test_exit_0_for_a_stacked_pr_on_an_open_parent, test_exit_1_for_a_stacked_child_whose_parent_is_gone), and the mutation list in the fix comment is the right set for those — but every one of them varies the base's direct status, so a fix that resolved only one level would keep all sixteen green. If the chain is worth handling, the test is A-dead/B-live and the assertion is on B's own row via the prs filter, which is the path where it currently returns 0.

Not blocking from my side — the aggressive direction (DEAD everywhere) is safe, and the whole-queue run is correct. The single-PR rc 0 is what makes it worth a look, since rc is the machine-readable signal the merge gate reads.

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.
@argszero

Copy link
Copy Markdown
Owner Author

Unblocked — count-line conflict resolved by measurement.

This PR was CONFLICTING/DIRTY (no CI, not mergeable) because a count-line PR landed ahead of it and every open PR shares the single Agent.md line documenting the Python test count.

classify-conflict.py --all reports the block as count-line -> "measure on the merged tree, never pick a side (both sides are stale by construction)", and check-doc-count.py --resolve-conflict re-measured it after stripping the markers. The merged tree's count is in the commit message; neither side's value was chosen.

Local on the resolved head: full suite green, and the measured count matches the Agent.md row exactly. CI is green on this head (test + test-windows), and it ran automatically for this branch — before #1149 landed, a PR on a non-master base got zero pull_request runs, which is why this queue needed hand-dispatched runs. That fix is now in production and these runs are the evidence.

@argszero argszero left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

✅ 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 argszero left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

✅ 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.py against this repo reported all 10 open PRs OK 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 (headRefName of an open PR) is the decidable one, and that open_heads is computed before the prs filter — necessary, since a stacked child's parent is usually not among the PRs being asked about.
  • Failure directions are right. _gh maps FileNotFoundError to the same rc-2 path as a failing gh (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_master raises on a response with no status rather 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.

@how2how2how2-arch

Copy link
Copy Markdown
Contributor

Re-raising the residual I noted at 4cbe4ad, now with a real-git reproduction of the loss it permits. At the current head f5905046. No gatekeeping verdict — the call is yours, and one reading of the exit-code section below makes the current behaviour correct.

The mismatch, in the tool's own words

The docstring poses the question it exists to answer as:

The question is not "is the base master", it is "can a merge into this base still reach master"

and answers the LIVE case with:

A merge into it goes into the live parent branch and rides along when the parent lands

Reaching master is transitive: it holds for the base only if the parent reaches master. The predicate that decides LIVE is one hop — base in open_heads, i.e. "the parent is an open PR". So the verdict establishes the parent is in flight and then asserts the parent will land on master, which is the one thing a one-hop test cannot know.

What that permits, in real git

Chain: PR #1 base=dead-branch (exists on the remote, not on master, no open PR names it), PR #2 base=root-1 (so #2 is LIVE), PR #3 base=mid-2. Driven through the shipped module:

#1 DEAD  base='dead-squashed' ...
#2 LIVE  base='root-1' - ... arrives when the parent lands ...
#3 LIVE  base='mid-2'  - ... arrives when the parent lands ...

full run (no filter):                rc = 1   (the root is flagged)
filtered to #2:                      rc = 0   <-- "not a dead end"
filtered to #3:                      rc = 0

Then perform the merges the rc 0 cleared, in order:

merged PR #2 into root-1:      rc=0     (gh pr merge would do this)
merged PR #1 into dead-branch: rc=0
is PR #2's change on master? False
is PR #1's change on master? False
master unchanged

Both PRs end up in the MERGED state, the tool said rc 0 about #2, and neither commit is on any path to master. That is the #1148 silent-loss shape one link deeper — the PR that this tool was written for — and here the tool clears it.

Scoping, and where I could be wrong

I checked both directions before writing this, because the opposite framing is defensible:

  • The unfiltered run is safe. The root is an open PR, so it is in the asked set and flagged DEADrc 1. I verified this. The escape needs the --prs filter — and that is a modelled, intended invocation, including by your own test test_exit_0_for_a_stacked_pr_on_an_open_parent, which asks about only the child.
  • The exit-code section is written one-hop: "0 no open PR's base is a dead end (bases either reach master, or are an open PR's head - the latter reported as LIVE)". Read in isolation, rc 0 above is per contract. But the headline of that same question — and the module's first line, "Report PRs whose base branch is not on the path to master" — is transitive. So the artifact that is wrong is at minimum the LIVE message, which asserts arrival, and the two halves of the docstring say different things about what rc 0 certifies.
  • I am not claiming the one-hop policy is wrong as policy. If certifying "the parent is open" is the intended contract, that is a coherent choice; the issue is only that the sentence a reader is given ("rides along when the parent lands") is stronger than the measurement behind it, and it is false in exactly the case the check exists for.

Test coverage

The suite covers both one-hop neighbours (test_a_live_stacked_base_is_live_not_ok, test_a_stacked_base_with_no_open_parent_is_dead) and the filtered case (test_exit_0_for_a_stacked_pr_on_an_open_parent). What has no test is that same filtered case with a DEAD root — identical wiring, one different line in the fixture, opposite answer. So the chain case is invisible to the suite rather than contradicted by it.

If you want the transitive contract

_open_prs already returns baseRefName for every open PR, so the parent's own base is in hand: a fixpoint over open_heads (a base is LIVE only if its parent's base can reach master) needs no new fetch. If you keep the one-hop contract instead, I would suggest softening the LIVE message to what is actually measured ("the parent is an open PR; this says nothing about whether the parent itself reaches master") so that the string a reader acts on does not promise arrival.

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 argszero left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

✅ 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 argszero left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

✅ 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.

@argszero

Copy link
Copy Markdown
Owner Author

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):

master + #1136            -> guard PASSES
master + #1152            -> guard file absent on master
master + #1136 + #1152    -> guard FAILS
master + #1152 + #1136    -> guard FAILS   (symmetric)

The guard is tests/test_script_decode_is_locale_independent.py::test_every_text_mode_subprocess_pins_its_encoding, added by #1136. Its scope is the whole tracked index (git ls-files -z minus vendored dirs), so it reaches tests/ on purpose — which is why this PR's new test file is in scope:

E  tests/test_check_pr_base.py:226 subprocess.run(capture_output, text) has no encoding= (child not statically known)
E  tests/test_check_pr_base.py:255 subprocess.run(capture_output, text) has no encoding= (child not statically known)

Neither PR's CI can see this: #1136 is measured against a master without tests/test_check_pr_base.py, and this PR is measured against a master without the guard.

Suggestion: if #1136 lands first, add encoding="utf-8", errors="replace" to those two calls here (the guard's rule is correct and these sites do violate it — neither child is a Windows console program). I have not posted a ❌ because against master this head is fine; this is an ordering fact, not a defect in the branch.

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 (git checkout <PR> -- <files>) is not a merge — it replaces master's already-pinned copy with the branch's older one and invents violations. Under the overlay, #1151 falsely appears to violate at tests/test_classify_conflict.py:823; under a real merge it is clean. The numbers above are all real merges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants