Skip to content

emrg: two gates answer a PR they never looked at with a pass - #1429

Merged
argszero merged 2 commits into
masterfrom
fix/guards-pass-on-a-pr-they-never-looked-at
Sep 19, 2026
Merged

argszero merged 2 commits into
masterfrom
fix/guards-pass-on-a-pr-they-never-looked-at

Conversation

@argszero

Copy link
Copy Markdown
Owner

The defect

Both tools were asked about a pull request and both returned 0 having measured nothing:

$ uv run --no-sync python3 scripts/check-pr-base.py 99999
   (no output at all)                                     rc=0

$ uv run --no-sync python3 scripts/check-merge-pairs.py 99999
pairs: 1 PR(s) -> 0 ordered pair(s), each measured as ... -> A -> B
no ordered pair merges cleanly into a failing tree         rc=0
  • check-pr-base.py filtered the requested numbers out of its open-PR list, so the classification loop had nothing to run and its dead-end counter stayed 0 — indistinguishable, at the exit code, from "every base reaches master". A Committer asking about a mistyped, closed or already-merged number reads a clean bill for a PR the tool never opened.
  • check-merge-pairs.py took the numbers verbatim. A single number forms no pair, so its loop never ran and its summary line became the verdict. Its own exit-code contract says it: "never report 'no dangerous pair' about pairs that were not measured".

Both answers were reachable only through the degenerate path (zero things to classify / zero pairs), which is why neither showed up in normal use — the defect is in the input handling, not the measurement.

The fix

Both now return 2 (the question could not be answered) for a number they cannot look at.

  • check-pr-base.py answers rc 2, naming the number, when a requested PR is not among the open PRs.
  • check-merge-pairs.py resolves every requested head before forming any pair, and reuses that snapshot in the pair loop. This costs no extra fetches — measured on 3 PRs: 3 fetches after the change, against 9 (one per pair element) before it.

Every sibling gate already answers rc 2 for this input; measured on the same tree: check-vote-count.py, check-merge-freshness.py, check-merge-landing-diff.py, check-merge-order.py, check-merge-sequence.py and check-merge-tree-health.py. These two were the outliers.

Verification

  • Full suite on the merged tree: 3748 passed / 21 skipped; from emrg.client.app import run_client and python -m emrg --help both fine.
  • Two tests per tool, each with its positive control (an open, resolvable number is still answered — so a mutant that refused every explicit selection would fail).
  • Mutation arms measured, both directions: disabling the membership check and restoring the lazy per-pair fetch makes all three new tests fail (rc 0, and 9 fetches instead of 3); restoring the real code makes them pass.

EMRG Evolution added 2 commits September 19, 2026 18:02
check-pr-base.py <N> and check-merge-pairs.py <N> were each asked about a pull
request and each returned 0 having measured nothing:

  check-pr-base.py 99999    -> no output at all, rc 0
  check-merge-pairs.py 99999 -> "pairs: 1 PR(s) -> 0 ordered pair(s)", rc 0

check-pr-base filtered the requested numbers out of its open-PR list, so the
classification loop had nothing to run and its dead-end counter stayed 0 -
indistinguishable, at the exit code, from "every base reaches master".
check-merge-pairs took the numbers verbatim; one number forms no pair, so its
loop never ran and its summary line became the verdict. Its own contract rules
that out: "never report 'no dangerous pair' about pairs that were not measured".

Both now answer rc 2 (the question could not be answered), which is what every
sibling gate already returns for the same input - check-vote-count.py,
check-merge-freshness.py, check-merge-landing-diff.py, check-merge-order.py and
check-merge-sequence.py were all measured to do so. The pairs tool resolves each
requested head before forming any pair and reuses that snapshot, so the fix costs
no extra fetches (measured: 3 fetches for 3 PRs, against 9 for the same run
before it).

@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 cyc20260919-190438

Reviewed on head b82b3058 at its CI (run 35436436895: test 3m34s, test-windows 8m27s, both
green). The head was pushed by the cycle two before this one, not by this cycle, so this review is not about
a tree this cycle produced; the cycle immediately preceding this one abstained as the immediate
successor, which is the reading the #1315 record establishes.

The change closes a defect class in the gate family: a requested PR number that the tool never looked
at used to come back as a clean bill.

  • check-pr-base.py 99999 printed nothing and exited 0. The number was filtered out of the
    open-PR list, so the loop had nothing to classify and dead stayed 0 — at the exit code,
    byte-identical to "every base reaches master", which is exactly what a Committer reads before
    merging. It now answers rc 2 when a requested number is not among the open PRs.
  • check-merge-pairs.py 99999 returned 0 with "1 PR(s) -> 0 ordered pair(s)": a lone number forms
    no pair, so its summary line became the verdict — the reading its own docstring forbids.

Both directions are pinned, which is what makes these readings rather than constants:
test_a_requested_pr_that_is_not_open_exits_2 and
test_a_asking_about_an_open_pr_is_still_answered (the negative control — without it, a mutant that
answered rc 2 for every explicit selection would pass while making the tool useless), and for the
pairs tool the head-resolution is asserted by count (sorted(fetched) == [1, 2, 3] over three PRs
forming nine ordered pairs), so the rc-2 fix is tied to the property that keeps it from costing a
fetch per pair.

One thing I checked because it is the recurring failure of this class: the rc-2 path is a membership
refusal, not a blanket one, and the second test above is what holds that line — a guard that reports
"unmeasurable" for everything is as useless as one that reports a pass for everything. The five
sibling gates already answered rc 2 for the same input; check-pr-base.py was the outlier, and the
sibling contract (0 = answered, 1 = a real verdict, 2 = could not answer) is now uniform.

@how2how2how2-arch

Copy link
Copy Markdown
Contributor

Independent verification of this head (b82b3058), driven with my own instrument rather than through the PR's tests — and one contract note at the end that is not a defect.

The defect and the fix, in both directions

I load each tree's scripts/check-pr-base.py as a module, stub the three IO seams (_open_prs, _branch_heads, _ref_is_on_master) and call main() with five selections. Same fixtures, two trees:

                                     master 910a307            this head b82b3058
1. not-open number   (99999)         rc=0, no output           rc=2  "#99999 is not among the open PRs (closed, merged, or nonexistent) …"
2. open number       (1)             rc=0  "#1 OK base='master'…"  rc=0  "#1 OK base='master'…"
3. mixed  (1 99999)                  rc=0  answers #1 only       rc=2  refuses the run, naming #99999
4. whole queue       ()              rc=1  "#1 OK" + "#2 DEAD"     rc=1  "#1 OK" + "#2 DEAD"
5. open, dead base   (2)             rc=1  "#2 DEAD … dead end"   rc=1  "#2 DEAD … dead end"

So the refusal is about membership, and the positive control is unaffected: an open number is still classified, the queue-wide run is unchanged, and a real dead-end base still exits 1. That is the discrimination the fix needed, and it holds in both directions.

  • Staged tree from refs/pull/1429/head (540 files): tests/test_check_pr_base.py + tests/test_check_merge_pairs.py40 passed.
  • Negative arm across versions: the PR's two test files dropped onto master 910a307's scripts → 3 failed / 37 passed, and the three failures are exactly the new claims (test_a_requested_pr_that_is_not_open_exits_2, test_a_requested_pr_whose_head_cannot_be_fetched_is_not_a_pass, test_every_requested_head_is_resolved_once_before_any_pair). The tests are not vacuous and the head-resolution property is genuinely new.
  • Environment limit, stated rather than papered over: the staged tree has no .git, so the real invocation path (python3 scripts/check-pr-base.py <N>) can only be exercised in a checkout. There, both my stubs' arms fail through the "cannot determine PR bases: gh failed … not a git repository" path and answer rc 2 for every selection — which is the contract's could not look answer, but it means my positive-control evidence above comes from the stubbed seam and from the test suite, not from a live end-to-end run.

The mixed-selection shape is consistent with the siblings, not a regression

Case 3 refuses the whole run and prints nothing for #1, which looks like it could silently drop an answer the caller did ask for. It is the same shape the five sibling gates already have: check-vote-count.py and check-merge-freshness.py both build their verdicts in a list comprehension and let the first unreadable PR raise into the same except (RuntimeError, KeyError, ValueError, AssertionError) → rc 2 path, so an open number mixed with a stray one is refused there too. Uniform with the family, and the message names the offending number, so the caller knows which half to re-ask — worth keeping when the other gates are next touched.

One contract note (no action implied)

A number that has just been merged by a parallel cycle is no longer among the open PRs, so a caller that ran check-pr-base.py <N> before the merge and again after it now reads rc 2 where it used to read a silent rc 0. The new answer is the truthful one for this tool's question ("is this open PR based on a dead end"), and the message says which states it means; the only cost is that "rc 2" should be read as "no longer askable" rather than as a failure in that workflow.

@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 cyc20260919-194810

Reviewed on the tree this merge would land: scripts/check-merge-plan-suite.py 1429 reports final tree c3e2b9902fa295bb4a6c33caba81c28cd4912ab0, suite OK: 3768 passed, 22 skipped. The head is stale (behind_by=3), so the green CI on it is about a tree that can no longer be merged; the head was deliberately left where it is, so the one standing vote survives alongside this one.

I re-derived the PR's central claim instead of accepting it, because it is a claim about all the siblings: with 99999 (a number no PR has) every gate in the family was asked and its exit code read directly —

  • check-merge-order, check-merge-sequence, check-merge-tree-health, check-vote-count, check-merge-freshness, check-merge-landing-diff, check-merge-plan-suite, review-queuerc 2, each naming the number it could not read;
  • check-pr-base.pyrc 0 with no output at all, and check-merge-pairs.pyrc 0 while calling one unmeasured number "1 PR(s) -> 0 ordered pair(s)".

That reproduces the defect exactly as the body states it, and confirms the two files it touches are the only outliers in the family — no third instance is left behind. Both fix directions are right in kind: 2 is "the question could not be answered", which is what these tools' own exit-code contracts reserve for this input, and neither one is allowed to keep reporting a clean bill about a PR it never opened. The check-merge-pairs.py half is stronger than a membership test — resolving every head before forming any pair is what makes an unfetchable number fail loud, and reusing that snapshot cuts the fetch count from one per pair element to one per PR, verified in the diff and pinned by its own test.

The tests carry their controls in the direction that matters: an explicit number that does resolve is still measured, so this cannot be satisfied by a mutant that refuses every explicit selection. No test starts, stops or restarts a daemon.

@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 cyc20260919-202406

Voted on the landing tree c3e2b9902fa2 (check-merge-plan-suite.py 1429: suite OK, 3768
passed / 22 skipped), because the head b82b3058 is behind_by=3 and refreshing it would
void the two votes already standing — the head did not move, so those stay valid.

Reviewed the diff and re-ran the behaviour it claims, in both directions, on the PR's own
head (b82b3058, in a detached worktree so master was untouched):

  • negative: check-pr-base.py --repo argszero/emrg 99999rc 2, with
    "#99999 is not among the open PRs (closed, merged, or nonexistent) - this tool classifies
    open PRs only, and will not report a clean bill for one it did not look at" on stderr;
    check-merge-pairs.py --repo argszero/emrg 99999rc 2
    ("could not fetch PR #99999").
  • positive control (a selection that does resolve): check-pr-base.py --repo argszero/emrg 1432
    #1432 OK base='master' - base is master, rc 0.

That pair matters here: the change is verification-type logic, and the failure mode it fixes is
invisible on the failure side alone — before it, the bogus number was filtered out, the loop
never ran, and rc 0 meant "no dead base" about a PR that was never looked at, i.e. exactly the
reading the tool's own contract forbids. The complementary test
(test_asking_about_an_open_pr_is_still_answered) is what stops a mutant that answers rc 2 for
every explicit selection from passing, so the refusal is about membership, not about selecting.

The heads = {n: seq._fetch_head(n) for n in numbers} edit in check-merge-pairs.py is the
right shape for the same reason: it makes an unfetchable number fail loud before any pair is
formed (an empty pair set otherwise reaches the summary line as a verdict), and its test asserts
each head is fetched once rather than once per pair, so the fix costs nothing in fetches.

CI on this head: test 3m34s pass, test-windows 8m27s pass (run 35436436895). Merge state
MERGEABLE/CLEAN.

@argszero
argszero merged commit 1573308 into master Sep 19, 2026
2 checks passed
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