diff --git a/changelog.d/3232-merge-blockers-already-merged-is-terminal.md b/changelog.d/3232-merge-blockers-already-merged-is-terminal.md new file mode 100644 index 000000000..1deba14bb --- /dev/null +++ b/changelog.d/3232-merge-blockers-already-merged-is-terminal.md @@ -0,0 +1,14 @@ +### Fixed: `check_merge_blockers.py` names an already-merged pull request instead of asking for a re-read that cannot settle + +A pull request that had already merged reported as `merge-state-unknown`, owed by +nobody, with the remedy "Re-read the pull request: mergeability is computed on +demand and settles on a later read". It never settles - a merged pull request is +closed, so `mergeable` stays null for good, and the report described a wait with +no terminating condition while the merge had already landed. Measured on #3219 +and #3230 minutes after they squashed: both `closed` / `merged: true` / +`mergeable: null`, byte-identical in every field the check read to open #3205's +genuine recompute at the same moment. The `merged` key was already in the payload +`resolve_state` fetches and was simply not read. There is now a terminal +`already-merged` outcome, reported ahead of every ruleset rule and alone, because +"0 of 1 approvals" is not a rule the merge left unsatisfied. An open pull +request's null still reads as an uncomputed mergeability, unchanged. diff --git a/scripts/check_merge_blockers.py b/scripts/check_merge_blockers.py index 044360f35..8c02f947b 100644 --- a/scripts/check_merge_blockers.py +++ b/scripts/check_merge_blockers.py @@ -86,7 +86,22 @@ run just after a merge is precisely when it is null. Reported as *gating*, because reading the null as clean is how a conflicted branch is reported as owed by a reviewer -- measured on #1035, which read - ``pusher-only-approval`` while it was ``DIRTY``. Re-read to resolve it. + ``pusher-only-approval`` while it was ``DIRTY``. Re-read to resolve it, + which works only because the pull request is open: the entry below reads the + same null for a reason no read settles. + +``already-merged`` + Nobody, and unlike every entry above there is no state left to reach. A + merged pull request is closed, so ``mergeable`` stays null for good -- the + same null the entry above reads as "still computing", on the one pull + request whose own merge invalidated it. Reported ahead of every ruleset rule + and short-circuiting them: once the change is on the base, "0 of 1 + approvals" is not a rule the merge left unsatisfied, it is a question about + a closed pull request. Measured on #3219 and #3230, which read ``closed`` / + ``merged: true`` / ``mergeable: null`` while open #3205 read ``mergeable: + null`` for the honest reason at the same moment -- byte-identical in every + field this check had been reading, so the ``merged`` key it already fetches + is what separates them (#3231). ``required-check-cancelled`` A **maintainer**, by re-running the run. A cancelled run is not a verdict @@ -222,6 +237,7 @@ def _load_sibling() -> Any: # Outcome names. Ordered here the way they bind in practice, which is also the # order they are reported in: a conflict makes the approval question moot, and # an unresolved thread makes it moot for a different reason. +ALREADY_MERGED = "already-merged" MERGE_CONFLICT = "merge-conflict" MERGE_STATE_UNKNOWN = "merge-state-unknown" DRAFT = "draft" @@ -244,6 +260,7 @@ def _load_sibling() -> Any: ANYONE = "anyone, by attempting the merge" _OWED_BY: dict[str, str] = { + ALREADY_MERGED: NOBODY, MERGE_CONFLICT: AUTHOR, MERGE_STATE_UNKNOWN: NOBODY, DRAFT: AUTHOR, @@ -270,7 +287,12 @@ def _load_sibling() -> Any: # because the cost is asymmetric -- reporting the approval rule as the next # action on a branch that turns out to be conflicted burns an approval, and # reporting it as necessary-but-not-sufficient costs one re-read (#2585). -_GATING: frozenset[str] = frozenset({MERGE_CONFLICT, MERGE_STATE_UNKNOWN, DRAFT}) +# ``already-merged`` gates for the strongest reason of the three: there is no +# rule left to assess rather than one that cannot be assessed yet. It is in this +# set so the report names it as the next action's subject and prints "owed by +# nobody, on `already-merged`" rather than the ungated wording "the answer is not +# in yet", which is the sentence #3231 measured as the misleading one. +_GATING: frozenset[str] = frozenset({ALREADY_MERGED, MERGE_CONFLICT, MERGE_STATE_UNKNOWN, DRAFT}) # The outcomes a scheduled author-side pass can act on without anyone else. @@ -353,6 +375,12 @@ class PullRequestState: mergeable: bool | None merge_state: str unresolved_threads: int + # Whether the pull request has already merged. Lives among the defaulted + # fields for the mechanical reason that the seven above it carry no default, + # not because it is peripheral: it is read before any of them. ``False`` is + # the safe default because it is the only value that leaves every rule below + # evaluated. + merged: bool = False check_conclusions: dict[str, str | None] = field(default_factory=dict) # Each check suite's conclusion on the head, or ``None`` for a census that # was not read. ``()`` is a positive observation -- zero suites exist -- and @@ -472,6 +500,29 @@ def evaluate(state: PullRequestState, rules: Ruleset) -> tuple[Blocker, ...]: """ found: list[Blocker] = [] + # Ahead of every rule below, the draft check included: a merged pull request + # has no unsatisfied rule to name. Separated from the uncomputed + # mergeability further down because the two are indistinguishable in every + # other field this check reads -- a merged pull request is closed, so + # ``mergeable`` stays null for good, and #2586's "re-read, it settles on a + # later read" then prescribes a wait with no terminating condition (#3231). + # + # Returned alone rather than added to ``found`` and gated: gating says the + # rules below cannot be assessed *yet*, and here there is nothing left to + # assess in any future read. + if state.merged: + return ( + Blocker( + ALREADY_MERGED, + "(not a ruleset rule)", + f"The pull request is already merged into {state.base_ref}, so no " + f"rule is outstanding and no later read will change that. Cached " + f"merge state is {state.merge_state or 'unknown'}, and mergeability " + f"is null here because the pull request is closed rather than " + f"because GitHub is still computing it.", + ), + ) + if state.draft: found.append( Blocker( @@ -810,6 +861,9 @@ def resolve_state(repo: str, pr: int, token: str) -> PullRequestState: head_sha=head_sha, base_ref=base_ref, draft=bool(payload.get("draft")), + # Already in this payload and previously unread, which is what let a + # merged pull request report as an uncomputed mergeability (#3231). + merged=bool(payload.get("merged")), mergeable=payload.get("mergeable"), merge_state=str(payload.get("mergeable_state") or ""), unresolved_threads=resolve_unresolved_threads(repo, pr, token), diff --git a/tests/test_merge_blockers.py b/tests/test_merge_blockers.py index 93dcca892..778c06b29 100644 --- a/tests/test_merge_blockers.py +++ b/tests/test_merge_blockers.py @@ -548,6 +548,131 @@ def test_an_unknown_mergeability_does_not_read_as_the_stale_state_case() -> None assert "Attempt the merge" not in rendered +# -------------------------------------------------------------------------- +# The third reading of a null mergeability: the pull request already merged. +# +# #3219 and #3230 were squashed at 12:26:39Z and 12:37:58Z on 2026-09-05, and +# `GET /repos/strands-labs/robots/pulls/{n}` then reported, for both: +# +# state=closed merged=true mergeable=None mergeable_state=unknown +# +# which is what the cluster above reads as "GitHub is still computing". Open +# #3205, read in the same minute, was `merged=false` with an identically null +# `mergeable` -- so the honest transient #2586 was written for is live at the +# same time, and every field the check read before this was the same on all +# three. `merged` is the one that separates them, and it was already in the +# payload `resolve_state` fetches. See issue #3231. +# -------------------------------------------------------------------------- + + +@pytest.mark.parametrize( + ("merged", "expected"), + [ + pytest.param(True, "already-merged", id="merged-3219-and-3230"), + pytest.param(False, "merge-state-unknown", id="open-3205"), + ], +) +def test_the_same_null_mergeability_gets_two_verdicts_by_whether_it_merged(merged: bool, expected: str) -> None: + """The measured pair, differing in the one field that was not being read. + + Pre-fix both rows report ``merge-state-unknown``, so the two are one verdict + and the merged row is told to re-read a value that is already final. + """ + st = state(merged=merged, mergeable=None, merge_state="unknown") + assert outcomes(mod.evaluate(st, MAIN)) == [expected] + + +def test_a_merged_pull_request_names_no_rule_underneath_it() -> None: + """Every rule is moot, not merely unanswerable, so none is listed. + + Deliberately hostile inputs: no approval, two unresolved threads and no + check conclusion at all. On an open pull request each is a blocker the + report must name. On a merged one, naming them invents obligations against a + closed pull request -- the change is already on the base, so there is no + approval that could admit it and no thread whose resolution could. + """ + st = state( + merged=True, + mergeable=None, + merge_state="unknown", + approvers=(), + unresolved_threads=2, + check_conclusions={}, + ) + assert outcomes(mod.evaluate(st, MAIN)) == [mod.ALREADY_MERGED] + + +def test_a_merged_pull_request_is_owed_by_nobody_and_is_not_a_finding() -> None: + """Owed by nobody for the opposite reason to ``required-check-pending``. + + That one is waiting; this one is finished. Not a finding either way, because + the exit status is for what a scheduled author-side pass can act on alone, + and there is nothing left to act on. + """ + blocker = mod.primary(mod.evaluate(state(merged=True, mergeable=None), MAIN)) + assert blocker.outcome == mod.ALREADY_MERGED + assert blocker.owed_by == mod.NOBODY + assert blocker.is_finding is False + assert blocker.is_gating is True + + +def test_a_merged_pull_request_is_not_prescribed_a_read_that_cannot_settle() -> None: + """The measured cost: the remedy that describes a wait with no end. + + A merged pull request is closed, so ``mergeable`` never becomes non-null and + "settles on a later read" is false however many times it is re-read. The + ungated wording is refused for the same reason -- "the answer is not in yet" + is the sentence that read as reassuring while the answer had been in for + eleven minutes -- and so is #2574's merge attempt, which would be a second + merge of a merged branch. + """ + st = state(merged=True, mergeable=None, merge_state="unknown") + rendered = mod.render(st, MAIN, mod.evaluate(st, MAIN), "o/r") + assert "Re-read" not in rendered + assert "the answer is not in yet" not in rendered + assert "Attempt the merge" not in rendered + assert f"Next action is owed by {mod.NOBODY}, on `{mod.ALREADY_MERGED}`" in rendered + assert "already merged" in rendered + + +def test_a_merged_pull_request_is_reported_alone_rather_than_as_a_gate() -> None: + """Gating says "not yet"; this says "never again", so nothing trails it. + + ``_next_action`` appends "an approval is necessary but not sufficient while + it stands" whenever a gating blocker has siblings. On a merged pull request + no approval is necessary at all, so that sentence would be false -- it is + absent because the blocker is returned alone, which this pins rather than + assuming. + """ + st = state(merged=True, mergeable=None, approvers=(), unresolved_threads=1) + rendered = mod.render(st, MAIN, mod.evaluate(st, MAIN), "o/r") + assert "necessary but not sufficient" not in rendered + + +def test_the_merged_verdict_does_not_reach_an_open_pull_request() -> None: + """A guard against fixing this by reporting every null as merged. + + #2586 exists because reading the null as clean handed a ``DIRTY`` branch to + a reviewer (#1035). Reading it as merged would be the same error wearing the + opposite sign, so the whole open cluster above must be untouched. + """ + for mergeable in (True, False, None): + st = state(merged=False, mergeable=mergeable) + assert mod.ALREADY_MERGED not in outcomes(mod.evaluate(st, MAIN)) + + +def test_the_merged_key_is_read_from_the_payload_that_already_carried_it() -> None: + """``resolve_state`` must populate the field, or the report never sees it. + + The seam #3231 measured was not the classifier but the read: ``evaluate`` + cannot separate the two nulls if every state reaching it says ``merged= + False``. Graded on the constructed state rather than the transport, so the + fixture is the REST payload's own key names. + """ + source = _SCRIPT.read_text(encoding="utf-8") + assert 'merged=bool(payload.get("merged"))' in source + + def test_the_next_action_line_names_one_owner_when_a_gating_blocker_is_present() -> None: blockers = mod.evaluate(state(mergeable=False, approvers=()), MAIN) rendered = mod.render(state(mergeable=False, approvers=()), MAIN, blockers, "o/r")