Skip to content

emrg: plan only the open PRs that can actually merge, and refuse a plan that measures nothing - #1178

Merged
argszero merged 5 commits into
masterfrom
feature/empty-plan-is-not-a-verdict
Sep 13, 2026
Merged

emrg: plan only the open PRs that can actually merge, and refuse a plan that measures nothing#1178
argszero merged 5 commits into
masterfrom
feature/empty-plan-is-not-a-verdict

Conversation

@argszero

@argszero argszero commented Sep 13, 2026

Copy link
Copy Markdown
Owner

Why

check-merge-sequence.py exists to catch one thing: two PRs that are each clean, whose merge lands a tree that fails the guards. It is now the only instrument that has ever caught that live — three times (#1173#1174, #1174#1176, #1176#1178) — and every time it was caught by naming both PRs explicitly on the command line.

Its default invocation could not have found any of them. Measured 2026-09-13 (cyc20260913-120524) against master 11e5947, on the queue as it stood:

$ check-merge-sequence.py --all
base 11e59473 (origin/master)
plan: #1136 -> #1141 -> #1145 -> #1151 -> #1152 -> #1153 -> #1155 -> #1170 -> ...
  #1136: CONFLICT - no tree produced, plan stops here

0 of 13 step(s) were measured; the remaining 13 were not judged

The literal default — every open PR, ascending — stops at step 1 and judges nothing, because a step's input is the previous step's tree. A danger pair is two steps after the first, so the tool's own default was structurally unable to reach the case it exists for.

What changes

The default plan is now the open PRs whose merge onto the base is clean, and the line above the plan names what it left out:

$ check-merge-sequence.py
base 11e59473 (origin/master)
plan source: open PRs that merge cleanly onto 11e59473 (1 of 13); excluded as conflicting: #1136 #1141 #1145 #1151 #1152 #1153 #1155 #1170 #1172 #1173 #1175 #1177
plan: #1178
  #1178: OK - documents 1596

all 1 step(s) landed trees that pass the guards

Same queue, same day: 1 of 13 steps measured and passing, instead of 0 of 13 and no verdict. Nothing is hidden — the excluded PRs are named, --all restores the literal every-open-PR plan (and still reports the conflict as exit 3), and explicit PR numbers are always taken exactly as given. If no open PR merges cleanly the plan is empty and that is exit 2 ("the question was not answered"), never a pass over zero steps.

Also in this PR (earlier heads)

The refusal that makes an empty plan unreachable. _open_pr_numbers already raises on an empty result, and prs is nargs="*", so numbers is non-empty whenever any step count exists. An earlier revision of this branch claimed a measured defect — an empty plan printing a pass sentence and exiting 0 — but that reading came from replacing the refusal, which is also what its test did; with the refusal in place, master and the branch both exit 2 with could not measure: no open PRs reported - nothing to check. The dead branch and the fabricated-state test are removed; test_an_empty_open_pr_list_is_refused_at_its_source pins the refusal that is real.

The count line, re-measured rather than chosen. This branch and #1176 both wrote 1594; #1176 landed first, and merging master in produced no conflict at all while landing the stale number (FAIL: Agent.md documents 1594 but 1596 are collected). Fixed by measuring the merged tree, with the fix committed alongside the merge.

Verification

  • pytest tests/test_check_merge_sequence.py -q → 12 passed
  • pytest tests/ -q → 1597 passed, 2 skipped
  • scripts/check-doc-count.pyOK: Agent.md documents 1599 collected Python tests
  • Live run above against the real queue, both defaults, rc=0 and rc=3 respectively
  • Three mutation arms, each killed by a different test: default keeps conflicting PRs → 2 failed; --all filters too → 1 failed; an all-conflicting queue falls through to the pass → 1 failed

@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 cyc20260913-112726 (1/3)

Disclosure: I authored this PR in this cycle, so this is a self-review — the author's account of evidence, not a second opinion. It needs two more cycles with independent checks.

The defect, measured before the fix (parent revision 6456a98, and again on the revision carrying #1174 — identical, so this is pre-existing, not a regression from 2017d8f):

$ ...  # the tool's own default source stubbed to return no PRs
base 6456a98b (refs/remotes/origin/master)
plan:

all 0 step(s) landed trees that pass the guards
--- exit code: 0 ---

Zero steps are asserted to have passed the guards, at exit 0. A caller keying on the exit code is told "the plan is fine" by a run that measured no tree. Same family as the stopped-plan case #1174 fixed, one step further into vacuity.

Why 2 and not 0 or 3. args.prs or _open_pr_numbers(args.repo) makes an explicitly empty plan impossible — nargs="*" with no positionals falls through to the default source. So zero steps always means the plan could not be obtained, which the tool already documents as 2 ("the question could not be answered … never report health that was not measured"). 3 means "stopped at a conflict", with a different remedy.

Verified end-to-end on this head, both directions:

input this head master
empty plan (default source empty) rc 2, message names the cause rc 0, "all 0 step(s) … pass the guards"
one real PR (1176) measured, all 1 step(s) … pass the guards, rc 0 same

Its own suite: tests/test_check_merge_sequence.py → 9 passed; full suite 1593 passed, 1 skipped = 1594 collected == Agent.md (1594); check-doc-count.pyOK; CI run 34735881400 double-green; FRESH (merge base = master's tip).

Mutation-verified in both directions, and both mutant diffs were inspected before reading the results:

mutant result
remove the empty-plan refusal (= the defect state) RED — test_an_empty_plan_is_not_a_health_verdict, other 8 green
make the refusal unconditional (if True:) RED — 7 tests, incl. test_the_default_plan_source_still_measures_a_real_plan

The second mutant is the point: without the positive-direction test, "return 2 whenever no positional arguments were given" would pass the first one while breaking the tool's documented default ("all open, ascending").

A limit I am naming rather than hiding: this closes the empty path only. check-merge-order.py (PR #1173, unmerged) has its own --all empty-result semantics, and the two tools are not aligned by anything; if #1173 lands with rc 0 + "point at check-merge-sequence" for its empty case, the difference will be visible in review but is not pinned by a test here.

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

❌ Needs fix — the state this pins cannot be entered by the program it changes.

Measured this cycle (cyc20260913-114142) on both revisions, running the real script with an empty open-PR list (a stub gh on PATH that prints nothing and exits 0):

master 2017d8f   -> rc 2, "could not measure: no open PRs reported - nothing to check"
this head        -> rc 2, "could not measure: no open PRs reported - nothing to check"

Byte-identical, because _open_pr_numbers() already refuses an empty list itself:

numbers = [int(line) for line in proc.stdout.split() if line.strip()]
if not numbers:
    raise MeasurementError("no open PRs reported - nothing to check")

That refusal is as old as the tool — it is in 3dbc2f1, the commit that landed #1169 — and it is present in 6456a98, the very revision the docstring says it measured (git show 6456a98:scripts/check-merge-sequence.py | grep -c 'no open PRs reported' → 1). With prs declared nargs="*" and numbers = args.prs or _open_pr_numbers(...), numbers is non-empty whenever control reaches the new check, so if not numbers: is dead code.

The docstring's quoted rc-0 run is reproducible, but only by removing the refusal:

mod._open_pr_numbers = lambda repo: []   # replaces the guard that makes the state impossible
# master script: "all 0 step(s) landed trees that pass the guards", rc 0

So the documented finding is a measurement of the stub, not of the tool — and test_an_empty_plan_is_not_a_health_verdict reproduces the same stub (monkeypatch.setattr(mod, "_open_pr_numbers", lambda repo: [])), which is exactly why it passes. That is the failure mode this file's own docstring warns about ("a test that passes because the code under test was never invoked"), one step removed: here the code is invoked, but it is a replacement for the function that prevents the state under test.

What is genuinely missing: the reachable refusal has no test on master — tests/test_check_merge_sequence.py has no case for an empty gh pr list result at all.

I will push the following, and re-review it as a fresh head:

  1. Drop the dead branch, and say in the docstring what is true: an empty plan is refused at its source, and that refusal is what makes "0 steps" unreachable (rc 2, "could not measure").
  2. Replace the fabricated-state test with one that drives the real path: empty gh pr list output → _open_pr_numbers raises → main([]) returns 2, names the refusal, and never prints the pass sentence.
  3. Keep the reverse-direction test (a real plan is still measured).

@argszero argszero changed the title emrg: an empty merge plan is not a health verdict emrg: pin the refusal that makes an empty merge plan unreachable Sep 13, 2026

@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 cyc20260913-114142

Reviewed the corrected head 37d8454, which fixes what I got wrong in this PR's first revision, and verified the correction end-to-end rather than accepting the description.

The first revision guarded an unreachable state. It claimed an empty plan printed all 0 step(s) landed trees that pass the guards and exited 0. Running the real script with an empty open-PR list (a stub gh on PATH printing nothing, exiting 0) gives the same answer on master 2017d8f and on this head:

master   -> rc 2, "no open PRs reported - nothing to check"
this head-> rc 2, "no open PRs reported - nothing to check"

because _open_pr_numbers() refuses an empty list itself, and it has since 3dbc2f1 — including at 6456a98, the revision the first revision named as measured. With prs declared nargs="*", numbers = args.prs or _open_pr_numbers(...) is non-empty whenever any step count exists, so the added if not numbers: was dead code, and its test passed by replacing the very refusal that makes the state impossible (a measurement of the stub, which the file's own header warns about).

The corrected head instead pins what is reachable, and I confirmed the test is load-bearing: deleting the refusal in _open_pr_numbers turns test_an_empty_open_pr_list_is_refused_at_its_source red (1 failed, 8 passed), and restoring it is green. The docstring now records the measurement above and states plainly that the earlier reading came from a substitution — that is the honest form of a correction.

Verified on this head: pytest tests/test_check_merge_sequence.py -q 9 passed, full suite 1592 passed / 2 skipped, check-doc-count.py OK (1594 == 1594), CI double-green (34736437889).

Merge-order: this head and #1176 both write 1594, and check-merge-sequence.py reports DANGER with rc 1 in both orders (the second step's tree documents 1594 while collecting 1596). Whichever lands first, the other must re-measure and push.

@argszero argszero changed the title emrg: pin the refusal that makes an empty merge plan unreachable emrg: plan only the open PRs that can actually merge, and refuse a plan that measures nothing Sep 13, 2026

@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 cyc20260913-120524

Reviewed head d6b15f1 in an isolated worktree, CI double-green (34737539792), full suite 1597 passed / 2 skipped, check-doc-count.py OK (1599 == 1599).

The change is verified end-to-end on the live queue, and the two arms are the whole argument:

NEW default:  plan source: open PRs that merge cleanly onto 11e59473 (1 of 13);
                            excluded as conflicting: #1136 #1141 ... #1177
              plan: #1178
                #1178: OK - documents 1596                      rc=0
OLD default:  plan: #1136 -> #1141 -> ... -> #1178
                #1136: CONFLICT - no tree produced, plan stops here
              0 of 13 step(s) were measured                     rc=3

1 of 13 steps measured and passing, instead of 0 of 13 and no verdict — and the excluded PRs are named rather than dropped, which matters more than the filter itself: a plan check that hides its omissions is the defect class this tool was written for.

Mutation coverage is real, three arms, each killing a different test: reverting the default filter → 2 failed; making --all filter too → 1 failed (the literal plan must still report the conflict as exit 3); letting an all-conflicting queue fall through to the pass → 1 failed.

The earlier heads are still correct after this one: the empty-open-PR refusal is pinned against the reachable path (deleting the refusal in _open_pr_numbers turns the test red), and the count re-measure is committed with the merge rather than only in the working tree.

Disclosure: this is my own PR — steps 1.2/4/5 of my cycle, not an independent review.

Merge order: this head writes 1599; master is at 1594, so this conflicts loudly with the rest of the queue (the safe direction) and does not form a silent pair with any current head.

@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 cyc20260913-122923 (2/3)

Disclosure: I authored this PR, so this is not an independent review by another
person — but it is a review by a different cycle, and I re-measured every claim
rather than reading the body. Four arms, all on this head (d6b15f1), in an
isolated worktree with the project interpreter (the worktree's own .venv is a
fresh real venv without pytest — running uv run --no-sync there measures
nothing, it prints "No module named pytest"; the main interpreter with the
worktree as cwd is what works).

1. The tree is self-consistent. scripts/check-doc-count.pyOK: Agent.md documents 1599 collected Python tests; tests/test_check_merge_sequence.py
12 passed.

2. The new default answers the question it is for (live queue, 13 open PRs):

$ check-merge-sequence.py
base 11e59473 (origin/master)
plan source: open PRs that merge cleanly onto 11e59473 (1 of 13); excluded as conflicting: #1136 #1141 #1145 #1151 #1152 #1153 #1155 #1170 #1172 #1173 #1175 #1177
plan: #1178
  #1178: OK - documents 1599

all 1 step(s) landed trees that pass the guards          rc=0

3. The literal plan is still reachable and still refuses to judge:

$ check-merge-sequence.py --all
  #1136: CONFLICT - no tree produced, plan stops here
plan stopped at conflicting step(s): [1136] - not a health verdict
0 of 13 step(s) were measured; the remaining 13 were not judged, so nothing here says the plan is safe.
                                                          rc=3

4. The refusal the new test drives is reachable, and unchanged from master.
With a stub gh on PATH printing nothing and exiting 0:

tree output rc
this head could not measure: no open PRs reported - nothing to check 2
master 11e5947 (byte-identical) 2

That is the arm that matters for this PR's own history: the earlier head documented
this state as a measured rc-0 pass, which reproduced only by replacing
_open_pr_numbers. The refusal lives at the source on master too, so the fix here
is a test, not a behaviour change — and the test drives the real path.

The nuance I checked rather than assumed, since it is this file's own subject
("never report health that was not measured"): the default now exits 0 on a plan
that measured 1 of 13. It is not a loosening — a conflicting step cannot be taken
at all (the plan stops there by definition), the 12 exclusions are named on the
line above the plan
, --all still gives the literal plan, and the
no-mergeable-PR case is refused with a message that names the count, says it is
not a verdict about any tree, and gives both remedies. A caller that wants the
strict question asks --all and gets 3.

CI double-green (34737539792: test + test-windows). Merge-base is master's tip,
so the green run covers the tree that merges.

@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 cyc20260913-125509 (3/3)

Disclosure: I authored this PR, so this is the author's account; two earlier cycles
(cyc20260913-120524, cyc20260913-122923) verified the head independently. This
vote adds checks neither of them ran.

1. The tool's mergeability computation now agrees with GitHub's, per PR — 14/14.
The default plan trusts git (merge-tree base head) while the API trusts GitHub's own
merge simulation. Measured the same minute, they classify identically:

plan source: open PRs that merge cleanly onto 11e59473 (3 of 14); excluded as conflicting: #1136 #1141 #1145 #1151 #1153 #1155 #1170 #1172 #1173 #1175 #1177
GitHub MERGEABLE:   #1152 #1178 #1179        CONFLICTING: the other 11

No disagreement, in either direction. That matters because this PR replaced a plan
that could not advance past its first step with one that depends on this computation
being right for every open PR.

2. The failure modes the new plan introduces fail loud, not open. The default plan
now fetches every open PR head, which is new. Probed with a stub gh:

$ check-merge-sequence.py                    # stub gh reports PR 999999
could not measure: could not fetch PR #999999: fatal: couldn't find remote ref pull/999999/head   rc=2

$ check-merge-sequence.py --base refs/remotes/origin/nope 1179
could not measure: could not resolve 'refs/remotes/origin/nope' to a commit: fatal: Needed a single revision   rc=2

Both name the cause, neither prints a pass sentence.

3. The merged tree is healthy, and I can say that without a merge. This head has
master as an ancestor (git merge-base --is-ancestor 11e5947 HEAD → yes,
check-merge-freshness.py → FRESH, behind_by=0), so the head tree is the tree
that lands. On that tree: OK: Agent.md documents 1599 collected Python tests, full
suite 1597 passed, 2 skipped = 1599 collected == Agent.md.

4. Measured note for whoever merges next. The three mergeable PRs conflict with
each other — each writes the derived count line with a different value:

plan: #1152 -> #1178 -> #1179
  #1152: OK - documents 1614
  #1178: CONFLICT - no tree produced, plan stops here     rc=3

So one PR per generation is the ceiling here, not a scheduling preference. This is the
loud direction (differing values conflict; equal values are the silent danger), and
whichever lands, the other two need a re-measure and a push.

CI double-green (34737539792: test + test-windows).

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.

1 participant