ci: merge only after the required checks have actually reported (#1966)

[justinchuby]

Addresses #1966. Does not change repository configuration — that decision is #1966's and not mine to take unilaterally. This makes the safe invocation the easy one in the meantime.

The mechanism, with the missing half

gh pr merge --auto means "merge as soon as the mergeable state allows", not "merge when the required checks are green". For an actor covered by the merge rules bypass (RepositoryRole id 5, bypass_mode: always) the required contexts are advisory. But the bypass alone does not explain the observed behaviour — two PRs armed by bypassing actors behaved differently:

PR	armed	required contexts at arm time	outcome
#1958	08:05	not yet created on 05742109f	merged 08:05:37 — four minutes before its own Fast even started, and it then failed
#1942	~07:09	created, pending	merged 07:25:25, after both reported pass

The bypass does not fire through a pending check. It fires when there is nothing to see. Under saturation a context takes 15–60 minutes to be created, and --auto armed inside that window merges immediately.

I nearly shipped this myself today. I rebased #1957 with --auto still armed. A force-push resets the head SHA and the old contexts do not carry over — the armed auto-merge does. For as long as babc43018 had no contexts, that PR was one scheduler tick from merging with zero CI. It did not. That was luck. I have since disarmed --auto everywhere and merged #1957 and #1967 through this script instead.

What the script establishes

every context the ruleset names as required is present in the rollup, for this head commit, and every one concluded SUCCESS.

Each of those three words is a defect this repo has already shipped in another costume:

• present — absent is not pending, pending is not pass. gh pr checks | grep -v fail reports success for a check that does not exist, because there is no row to grep. That is the vacuous-guard class (Vacuous-guard class: five independent instances of a test that ran, reported ok, and asserted nothing #1817) relocated into the gate that guards everything else.
• this — the head is pinned at start; if it moves the wait is void (exit 4). Directly from the fix(build): name the unpopulated cpuinfo submodule instead of failing in cmake (#1816) #1957 near-miss above.
• ruleset — the required set is read from the API at run time. A hardcoded list drifts silently into a shorter one, and the failure mode of that is a green report.

Fails closed: an undetermined required set is exit 5, never a merge, because an empty required set reads identically to a satisfied one.

Distinct exit codes, because "did not merge" is not one outcome and a caller that cannot tell timeout from failure retries the wrong ones: 0 merged, 2 a required check failed, 3 timed out (pending or never appeared), 4 head moved, 5 required set undeterminable, 6 not about checks (closed/draft/refused).

There is no --admin flag and no code path that could add one; --admin from the caller is rejected as an unknown option.

Evidence

• scripts/merge_when_green_test.sh — 27/27, hermetic (gh shimmed, backed by fixtures, recording its own argv so "never --admin" is asserted behaviourally rather than by grepping the source).
• Mutants killed 5/5:

mutant	cells that went red
absent counts as success	3
head-move check removed	2
empty required set proceeds	3
pending counts as pass	3
required set hardcoded instead of read from the API	4

• shellcheck -x clean on both scripts.
• Used in anger: fix(build): name the unpopulated cpuinfo submodule instead of failing in cmake (#1816) #1957 (b3a9f6cb6) and fix(hostlock): let run --owner certify the rows it wraps (#1929) #1967 (8d6377f00) were both merged by this script, after all required contexts reported SUCCESS on the exact head SHA.

Wiring

Added to Rust quality beside the existing (self-test) steps. The suite is hermetic and costs about a second. It belongs in a required lane specifically because this script's failure mode is a green report: if it starts treating an absent check as a passing one, nothing downstream notices, because the evidence it was meant to collect is exactly what is missing.

The live-API probe is opt-in (MWG_LIVE=1) so CI has no network dependency; the offline branch asserts the property that makes the fixtures meaningful — that the required set is read from the ruleset API rather than listed in the script. Both branches assert exactly one cell, so the count pin is invariant across environments.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 81.02%. Comparing base (b3a9f6c) to head (c594d3b).
⚠️ Report is 11 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1970      +/-   ##
==========================================
+ Coverage   80.25%   81.02%   +0.77%     
==========================================
  Files         407      424      +17     
  Lines      188524   207556   +19032     
  Branches   188524   207556   +19032     
==========================================
+ Hits       151293   168179   +16886     
- Misses      31822    33741    +1919     
- Partials     5409     5636     +227     

Flag	Coverage Δ
cli-ort-linux	72.51% <ø> (?)
cli-ort-windows	72.01% <ø> (?)
mlas	85.33% <ø> (?)
offline	81.18% <ø> (+0.93%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.
see 69 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[justinchuby]

Opus review found two ways this script could merge when it should not — the exact defect it exists to eliminate. Both fixed in 1b9dc763c, both now covered.

1. Greenness was inferred from the absence of non-green rows. The decision was [ -z "$notgreen" ], which an empty verdict satisfies. Reproduced end to end: a statusCheckRollup containing a non-object entry makes the parser throw, it writes nothing to stdout, and the script printed "every required check reported SUCCESS" and merged. A required set that was present but all-whitespace reached the same place by a different route.

Fixed three ways, because the empty verdict had three sources: the parser's exit status is now checked (exit 5), the required set is filtered for non-blank lines before the emptiness test, and the decision counts SUCCESS rows against the required count instead of inferring from an empty complement.

2. Duplicate rollup entries were keyed last-wins. For a required context appearing twice, the array order decided whether we merged — ordering GitHub does not document as chronological, and a workflow re-run produces exactly this shape. Confirmed against the pre-fix script: FAILURE then SUCCESS → merged; the reverse → refused. Now folded pessimistically — a name is green only if every entry carrying it is.

Also taken: ERROR and STALE join the fast-fail set (neither will ever go green, so timing out reports the wrong exit code); and five consecutive unreadable polls is now an environment fault (exit 1) rather than a slow check (exit 3), so nobody retries a revoked token as if it were a queue.

Cells 27 → 38. New: non-object rollup entry; whitespace-only required set; duplicate name in both orders; duplicate all-green still merges; legacy ERROR fails fast; unreadable PR is exit 1.

Mutants: 8 run, 6 killed — and I am reporting the two survivors rather than the ratio that flatters.

mutant	result
absent counts as success	KILLED (4 RED)
head-move check removed	KILLED (2 RED)
empty required set proceeds	KILLED (5 RED)
required set hardcoded	KILLED (5 RED)
rollup parse failure ignored	KILLED (15 RED)
duplicates keyed last-wins	KILLED (2 RED)
pending counts as pass	SURVIVED
greenness inferred, not counted	SURVIVED

Both survivors are redundancy by design: each is covered by the other guard, so breaking one alone changes no outcome. I could have removed the redundant layer to make the table read 8/8, and chose not to — a merge gate is where belt-and-braces is justified — but the comment at the decision now says plainly which layer is load-bearing and which is belt, so nobody later mistakes an uncovered line for a tested one. That distinction is the whole subject of #1817.

Not taken, with reasons:

• Scoping the required set to rulesets whose ref_name matches the base branch. Today both active branch rulesets target ~DEFAULT_BRANCH and main carries zero checks, so the union is exactly the two required contexts. Over-collecting is the safe direction for merge safety and only costs liveness; under-collecting is the direction that merges something. If a release/* ruleset with its own checks ever appears, this waits forever and exits 3 — loudly, and not by merging.
• The offline anti-vacuity cell pins source text rather than behaviour. True, and it is the weakest cell here; a behaviour-preserving refactor would turn it red. It is what remains testable with no network, the live probe (MWG_LIVE=1) covers the real API, and a cell that fails loudly for the wrong reason is a cost I will take over one that passes quietly for the wrong reason.

Reviewer also confirmed no gh failure mode produces a false green (auth/404/5xx/rate-limit each land on exit 5, 1 or 3), that a missing python3 or a malformed blob exits before the merge path, and that the CI step has no network, gh, git or pre-existing-target/ dependency.

[justinchuby]

Re-review found the fix commit's own test was vacuous. Fixed in c594d3b97, and I am correcting a claim I made in the last commit message.

The finding. The cell named a pull request that cannot be read at all is an environment fault did not exercise the code it was named after. The shim failed every gh pr view, so the opening snapshot failed and the pre-existing pre-loop die produced exit 1. The loop was never entered — deleting the entire misses block left the suite at 38/38, and mutating its exit 1 to exit 77 changed nothing.

So the behaviour that commit actually added — five consecutive in-loop read failures being an environment fault rather than a slow check — had zero coverage, while a green cell carried its name. That is the vacuous-guard shape, in the suite for the script whose subject is the vacuous-guard shape. Reviewer was right to lead with it.

The fix. The shim now fails a window of reads rather than all of them, so a cell can let the opening read succeed and break the connection afterwards. Three cells, 38 → 41:

• the pre-loop failure, renamed to say what it actually tests;
• a read that breaks mid-wait → exit 1;
• four dropped reads then a recovery must still merge — otherwise the counter has quietly turned every transient 502 into a refusal, which is a different way to be wrong.

Both mutants now die: retry counter removed and threshold raised beyond any run, each 1 RED on the mid-wait cell. Neither was detectable before.

Correcting my own commit message. I claimed rollup parse failure ignored was killed with 15 RED. That is wrong, and the reviewer's version is right. My mutant inverted the guard so it fired always, which naturally reddens everything; removing it changes no test outcome, because every parser fault raises before printing a row and the rows != want count then refuses with the same exit 5. The guard is a third redundant layer, not a uniquely covered one. It earns its place on the diagnostic — parser exit N rather than a row-count mismatch — not on coverage.

Honest count for this file: 11 mutants run, 8 killed, 3 disclosed survivors, all three redundancy by design. I would rather publish that than a ratio that flatters, for the same reason the survivors are in the commit message.

Live evidence for the THIS head property, unplanned. The waiter was running against 1b9dc763c when I pushed the fix:

-- 11:57:33 --
  Fast (Linux x86_64)          COMPLETED    SUCCESS
  Rust quality                 QUEUED       -
merge_when_green.sh: head moved 1b9dc763c -> c594d3b97 while waiting.
merge_when_green.sh: the checks observed so far were for a commit that is no longer
merge_when_green.sh: the tip. Refusing to merge; re-run against the new head.

Fast (Linux x86_64) was green — for a commit that no longer existed at the tip. Exit 4, no merge. That is precisely the #1957 near-miss this PR was written after, reproduced by accident against the real API, and refused.

Reviewer also confirmed the two previously disclosed survivors are genuinely redundant rather than a rationalisation: given the rows == want gate, green == want and [ -z "$notgreen" ] are logically equivalent, so no reachable input separates them.