merge_pr.py --why cannot see the published merge-authority check, so it says "would merge" while GitHub blocks the merge nf-gwa7 ← Beads

closed priority 2 task unassigned

Twinned from https://github.com/cfsh/ninefold/issues/763 by tools/beads/import_github.py's reconcile pass.

Filed unclaimed. Hit live while merging #759; @aedanpope asked for tooling-friction feedback and this is the one item with a concrete fix.

What happened

@aedanpope approved #759 at 06:43Z. Then:

``` $ ./tools/pr/merge_pr.py --why 759 #759 state=OPEN reviewDecision=(none) reviews={'aedanpope': 'APPROVED'} ledger clean: all non-advisory reviewer feedback on #759 has a later reply -> would merge on HUMAN APPROVAL (perimeter not consulted).

$ ./tools/pr/merge_pr.py 759 authority: human approval (aedanpope) running presubmit --all against head 005fc09 … # ~40s PASS refuse: a GitHub call failed, so the merge cannot be gated safely. X Pull request cfsh/ninefold#759 is not mergeable: the base branch policy prohibits the merge. ```

The cause: the required merge-authority check run was stale. It was published at 02:48 reading *"Needs a human approval"* — true then — and @aedanpope approved at 06:43. Nothing republishes it on an approval, so main's ruleset kept blocking.

Fix took seconds once identified: ./tools/pr/review_now.py 759 --reconcile-onlySUCCESS Approved by aedanpope, then the merge went through.

Why this is a defect and not just a surprise

--why is the command CLAUDE.md sends a session to before merging, and merge_pr.py's own comments state the contract twice:

> --why exists so a session learns what will stop it BEFORE it spends minutes on a build, and a freshness refusal that only appears at merge time is the one it can least afford to discover late.

> --why is where CLAUDE.md's loop sends a session before it spends minutes on a build, so it has to answer the question the merge will actually answer.

That second comment is attached to a fix for this same defect class: STALE_OK was consulted by merge() but not by --why, so --why reported a refusal merge() would not make. This is the same divergence with the arrow reversed — --why reports *no* refusal where the merge is in fact blocked.

⚠️ A precise distinction, because it decides where the fix goes. merge() did not refuse; GitHub did. So the literal invariant — *"every refusal merge() makes, in the order it makes them"* — still holds. What broke is the stated *purpose*: learning what will stop you before spending minutes on a build. merge_pr.py and nflib/authority.py contain zero references to check-runs (grep -n "check-runs\|check_run" → no hits), so the gate recomputes authority from source and never looks at the published artifact that the branch ruleset actually enforces. Those two can only disagree in one window — after a verdict, before a reconcile — and an approval landing after the last dispatcher tick puts you in it every time.

So this fires on the ordinary path, not an exotic one: any PR needing human approval, where the human approves after the reviewer's last sweep. That is the normal shape of an approved PR.

Suggested fix

In --why (and ideally in merge()'s preflight, before presubmit --all spends the 40s), read the head sha's check runs and compare against computed authority:

``` gh api repos/cfsh/ninefold/commits/<head>/check-runs ```

When the published merge-authority conclusion disagrees with what authority.py computes now, say so and name the remedy:

``` ⚠ merge-authority check is STALE — published "Needs a human approval", authority now resolves to human approval (aedanpope). The branch ruleset reads the published check, so the merge WILL be blocked. Fix: ./tools/pr/review_now.py <n> --reconcile-only ```

That turns a two-command diagnosis behind an opaque GitHub string into one line at the moment it is cheap.

⚠️ Do not "fix" this by making merge_pr.py republish the check itself. That would put the gate on both sides of a boundary #704 deliberately split (authority extracted so gate and check *cannot* disagree), and a gate that repairs its own gating artifact is not a gate. Reporting the divergence is the fix; --reconcile-only already exists to repair it.

Smaller, adjacent

review_now.py prints an excellent runtime banner — *"this takes MINUTES … don't kill it"*, plus a pointer to the cheaper --reconcile-only. merge_pr.py carries the equivalent ⏱ warning only in probe_gate()'s docstring (*"THIS MAKES THE MERGE TAKE MINUTES, NOT SECONDS"*), never printed. CLAUDE.md compensates in prose, and the docstring itself argues warnings *"belong in the file a reader is actually in, not only in CLAUDE.md"* — the same argument extends to stdout, since a timeout that kills the gate is set by whoever invokes it, and they are reading the terminal, not the source.

⚠️ One nuance for whoever takes it: --reconcile-only's existing hint is framed as *"Only need the check refreshed after a push?"*. After an approval is the other trigger, and is the one that bit here — worth widening that sentence too.

Refs #734 (--reconcile-only), #704 (authority as a check run), #720 (dispatcher publishes it).

Dependencies

None.

Comments

No comments.

Add a comment