From ff0484202024a1256a36b2deeb254e2c8624ac2b Mon Sep 17 00:00:00 2001 From: cagataycali Date: Sat, 5 Sep 2026 09:06:41 -0400 Subject: [PATCH] fix(scripts): a merged pull request is not a mergeability GitHub is still computing `check_merge_blockers.py --pr N` reported an already-merged pull request 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". No later read settles it. A merged pull request is closed, so `mergeable` stays null permanently -- #2586 still read `null`/`unknown` fourteen days after it squashed -- and the advice describes a wait with no terminating condition, in the reassuring direction: "no party owes an action" is literally true while the answer is "you already merged it". `mergeable is None` was ambiguous between "GitHub is still computing" and "there is nothing left to compute". Measured on 2026-09-05 right after #3219 and #3230 squashed, both merged rows and open #3205 were identical in every field the script read, so the genuine transient #2585 was written for is live at the same time as the terminal state and must keep its re-read. The field that separates them was already in the payload: `resolve_state` fetched the whole REST pull request object and read five keys from it, and `merged` came back in the same response. - `PullRequestState` gains `merged`, populated from that key. - A terminal `already-merged` outcome, owed by nobody, reported ahead of every rule and short-circuiting them: on a change already on the base, "0 of 1 approvals" is not an unsatisfied rule. Not a finding, so the exit status keeps its meaning. - `primary` and `_next_action` gain a terminal tier ahead of the gating one, so a merged pull request cannot get "the answer is not in yet". - The `merge-state-unknown` docstring no longer implies its "every open pull request" clause covers the merged case. Reading a null as clean is still wrong (#1035), and the open recomputing path is unchanged -- pinned by the control row of the new table cell. Closes #3231 --- AGENTS.md | 15 ++- ...ady-merged-is-not-a-pending-computation.md | 29 +++++ scripts/check_merge_blockers.py | 84 +++++++++++- tests/test_merge_blockers.py | 123 ++++++++++++++++++ 4 files changed, 246 insertions(+), 5 deletions(-) create mode 100644 changelog.d/3233-already-merged-is-not-a-pending-computation.md diff --git a/AGENTS.md b/AGENTS.md index 562e55749..d5d10ae9c 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -1287,7 +1287,8 @@ hatch run format # ruff check --fix, ruff format absent because the head carries no check suite at all (also a maintainer, but by closing and reopening: there is no held run to approve and no suite to re-run), a check still running (nobody), a - mergeability GitHub has not finished computing (nobody, until a re-read), or + mergeability GitHub has not finished computing (nobody, until a re-read), a + pull request that has already merged (nobody, terminally), or no unsatisfied rule at all, which is the #2574 case and the one worth saying out loud. A conflict, a draft, or an uncomputed mergeability is reported as *gating*: the rules behind it cannot be assessed, so an approval there is @@ -1302,6 +1303,18 @@ hatch run format # ruff check --fix, ruff format request in the same state read `no-unsatisfied-rule`, whose printed remedy is to attempt the merge. Both are now `merge-state-unknown`. See #2585. + Read the null against `merged`, though, because "every open pull request" is + the whole of that claim. The pull request whose *own* merge invalidated the + value is not open, and for it the null never resolves: #2586 still read + `mergeable: null` / `mergeable_state: unknown` fourteen days after it + squashed. So a merged pull request reports `already-merged`, terminal and + ahead of every rule, rather than the re-read -- which on a closed pull request + describes a wait with no terminating condition, and reads in the reassuring + direction while a merge-and-verify cycle polls for an answer it already has. + A pull request closed *without* merging is a different reading again and does + not share the null: measured on #3194, it retains its last computed + `mergeable` (`true`/`blocked`). See #3231. + It composes `check_last_push_approval.py` rather than restating it, so what counts as a current approval has one owner. Neither script gates a merge. Pinned by tests/test_merge_blockers.py. diff --git a/changelog.d/3233-already-merged-is-not-a-pending-computation.md b/changelog.d/3233-already-merged-is-not-a-pending-computation.md new file mode 100644 index 000000000..df788a466 --- /dev/null +++ b/changelog.d/3233-already-merged-is-not-a-pending-computation.md @@ -0,0 +1,29 @@ +### Fixed: an already-merged pull request is no longer reported as a mergeability still being computed + +`scripts/check_merge_blockers.py --pr N` reported a pull request that had +already merged 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". No later read settles it -- a merged pull request is closed, so +`mergeable` stays null permanently, and #2586 still read `null`/`unknown` +fourteen days after it squashed. The advice describes a wait with no +terminating condition, and it reads in the reassuring direction: "no party owes +an action" is literally true, which is exactly what a caller polling for "can I +merge yet" sees while the answer is "you already did". + +`mergeable is None` was ambiguous between "GitHub is still computing" and +"there is nothing left to compute", and the genuine transient #2585 named is +live at the same time: measured immediately after #3219 and #3230 squashed, +both merged pull requests and open #3205 were identical in every field the +script read. The field that separates them was already in the payload -- +`resolve_state` fetched the whole REST pull request object for five keys, and +`merged` came back in the same response. + +`PullRequestState` now carries `merged`, and a terminal `already-merged` +outcome is reported ahead of every rule and short-circuits them: once the +change is on the base, "0 of 1 approvals" is not an unsatisfied rule but a +question about a closed pull request. It is owed by nobody, is deliberately not +a finding so the exit status keeps its meaning, and carries none of the +report's deferral language. `primary` and `_next_action` gained a terminal tier +ahead of the gating one, so a merged pull request can no longer be told "the +answer is not in yet". Reading a null as clean is still wrong (#1035) and the +open recomputing path is unchanged. diff --git a/scripts/check_merge_blockers.py b/scripts/check_merge_blockers.py index 044360f35..ded1eb431 100644 --- a/scripts/check_merge_blockers.py +++ b/scripts/check_merge_blockers.py @@ -82,12 +82,26 @@ Nobody, for now, and the same shape as the entry above one field over. ``mergeable`` is ``bool | None``: GitHub computes it on demand and returns null while it works, which is neither "conflicts" nor "merges cleanly". A - merge into the base invalidates it for every open pull request, so a sweep + merge into the base invalidates it for every *open* pull request, so a sweep 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. + Scoped to an open pull request deliberately: the pull request whose own + merge invalidated the value is not open, and for it the null never resolves + at all. That case is ``already-merged`` below, not this one. + +``already-merged`` + Nobody, terminally. A merged pull request is closed, so ``mergeable`` stays + null permanently -- measured on #2586, still ``null``/``unknown`` fourteen + days after it squashed -- and the re-read above describes a wait with no + terminating condition. Read from ``merged``, which the same response + already carries. Reported ahead of every rule and short-circuiting them: + once the change is on the base, "0 of 1 approvals" is not an unsatisfied + rule, it is a question about a closed pull request. Not a finding, because + there is nothing for an author-side pass to act on. + ``required-check-cancelled`` A **maintainer**, by re-running the run. A cancelled run is not a verdict about the tree; it is what the concurrency group leaves behind when a @@ -222,6 +236,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 +259,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, @@ -272,6 +288,15 @@ def _load_sibling() -> Any: # reporting it as necessary-but-not-sufficient costs one re-read (#2585). _GATING: frozenset[str] = frozenset({MERGE_CONFLICT, MERGE_STATE_UNKNOWN, DRAFT}) +# An outcome that answers the question rather than deferring it. Distinct from +# gating: a gating blocker is one the rules below it wait on, and it clears. A +# terminal one has nothing below it and never clears, so the remedy language +# every other outcome carries -- re-read, re-run, attempt the merge -- is wrong +# for it. Held as a set rather than checked at the two render sites because +# ``primary`` and ``_next_action`` are handed blockers alone, and the module's +# own rule is that precedence cannot be applied to one report and not the other. +_TERMINAL: frozenset[str] = frozenset({ALREADY_MERGED}) + # The outcomes a scheduled author-side pass can act on without anyone else. # These are the ones that get misread as reviewer bandwidth, so these are the @@ -328,6 +353,15 @@ def is_gating(self) -> bool: """Whether the rules after this one cannot be assessed until it clears.""" return self.outcome in _GATING + @property + def is_terminal(self) -> bool: + """Whether this answers the question rather than deferring it. + + A terminal outcome is not waiting on a person, a clock or a later read, + so it takes precedence over every other and must not carry a remedy. + """ + return self.outcome in _TERMINAL + @dataclass(frozen=True) class Ruleset: @@ -353,6 +387,12 @@ class PullRequestState: mergeable: bool | None merge_state: str unresolved_threads: int + # Whether the change is already on the base. Read because ``mergeable`` is + # null for a merged pull request exactly as it is for one GitHub is still + # computing, and the two need opposite reports: one is terminal, the other + # settles on a re-read. Defaults to ``False`` so the ordinary open case is + # unchanged, and every caller names its fields. + 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 @@ -469,7 +509,25 @@ def evaluate(state: PullRequestState, rules: Ruleset) -> tuple[Blocker, ...]: is a single ``no-unsatisfied-rule`` blocker rather than an empty tuple: the caller asked why a pull request is blocked, and "no reason found" is an answer with a remedy, not an absence of one. + + A merged pull request short-circuits every rule below. Its ``mergeable`` is + null for good, so falling through would report the transient + ``merge-state-unknown`` and prescribe a re-read that cannot settle; and an + approval count on a change already on the base is not an unsatisfied rule. """ + if state.merged: + return ( + Blocker( + ALREADY_MERGED, + "(not a ruleset rule)", + f"The pull request is already merged into {state.base_ref or 'its base'}, " + f"so no rule is outstanding. Its cached merge state reads " + f"{state.merge_state or 'unknown'} and stays that way: mergeability is " + f"computed for open pull requests only, so re-reading this one cannot " + f"settle it and no party owes an action.", + ), + ) + found: list[Blocker] = [] if state.draft: @@ -812,6 +870,7 @@ def resolve_state(repo: str, pr: int, token: str) -> PullRequestState: draft=bool(payload.get("draft")), mergeable=payload.get("mergeable"), merge_state=str(payload.get("mergeable_state") or ""), + merged=bool(payload.get("merged")), unresolved_threads=resolve_unresolved_threads(repo, pr, token), check_conclusions=resolve_check_conclusions(repo, head_sha, token) if head_sha else {}, check_suite_conclusions=resolve_check_suites(repo, head_sha, token) if head_sha else None, @@ -836,14 +895,20 @@ def primary(blockers: Sequence[Blocker]) -> Blocker: reviewer now, and reporting it as owed by nobody would park it. Only if no blocker is owed by anyone does the earliest-binding one stand. + A terminal blocker precedes even a gating one: gating asks the reader to wait + for something, and there is nothing left to wait for. + Both renderers and both warning paths route through this, so precedence cannot be applied to one report and not the other. """ return next( - (b for b in blockers if b.is_gating), + (b for b in blockers if b.is_terminal), next( - (b for b in blockers if b.is_finding), - next((b for b in blockers if b.owed_by != NOBODY), blockers[0]), + (b for b in blockers if b.is_gating), + next( + (b for b in blockers if b.is_finding), + next((b for b in blockers if b.owed_by != NOBODY), blockers[0]), + ), ), ) @@ -857,7 +922,18 @@ def _next_action(blockers: Sequence[Blocker]) -> list[str]: merge anything. Non-gating blockers genuinely are parallel -- a pending check and a missing approval wait on different people at once -- so those are listed together. + + A terminal blocker is reported first and alone, and says so in as many + words. The fall-through line below it -- "the answer is not in yet" -- is + the reassuring reading a merged pull request must not get: it is true only + of an outcome that a later read can change. """ + terminal = next((b for b in blockers if b.is_terminal), None) + if terminal is not None: + return [ + f"No party owes an action, and none will: `{terminal.outcome}`.", + "This is terminal rather than not-in-yet -- no later read changes it.", + ] gating = primary(blockers) if any(b.is_gating for b in blockers) else None if gating is not None: trailing = [b for b in blockers if b is not gating] diff --git a/tests/test_merge_blockers.py b/tests/test_merge_blockers.py index 93dcca892..08e9bf0a6 100644 --- a/tests/test_merge_blockers.py +++ b/tests/test_merge_blockers.py @@ -548,6 +548,129 @@ def test_an_unknown_mergeability_does_not_read_as_the_stale_state_case() -> None assert "Attempt the merge" not in rendered +# -------------------------------------------------------------------------- +# A merged pull request. ``mergeable`` is null here for the same reason it is +# null while GitHub computes, and the two need opposite reports. Measured on +# 2026-09-05 immediately after #3219 and #3230 squashed: both read +# ``state: closed``, ``merged: true``, ``mergeable: null``, +# ``mergeable_state: unknown``, byte-identical to open #3205 in every field this +# script read, and all three reported ``merge-state-unknown`` owed by nobody +# with the remedy "re-read ... settles on a later read". It does not settle: +# #2586 still read null fourteen days after it merged. +# -------------------------------------------------------------------------- + + +@pytest.mark.parametrize( + ("merged", "expected"), + [(False, mod.MERGE_STATE_UNKNOWN), (True, mod.ALREADY_MERGED)], + ids=["open-and-recomputing", "already-merged"], +) +def test_a_merged_pull_request_and_an_open_recomputing_one_do_not_read_the_same(merged: bool, expected: str) -> None: + """One field apart, and it is the field that decides whether waiting helps. + + Both rows are ``mergeable=None``, ``merge_state="unknown"`` -- the whole + observable difference is ``merged``, which the pull request payload already + carries beside the two keys the script was reading. The open row is the + genuine transient #2585 was written for and must keep its re-read; the + merged row must not get it, because no read will ever change it. + """ + st = state(merged=merged, mergeable=None, merge_state="unknown") + assert outcomes(mod.evaluate(st, MAIN)) == [expected] + + +def test_a_merged_pull_request_is_not_offered_a_remedy_that_cannot_terminate() -> None: + """The cost was a polling cycle: the reassuring reading is also the wrong one. + + "No party owes an action; the answer is not in yet" is literally true of a + merged pull request and describes the opposite situation -- the answer is + in, and it is that the change already landed. Every deferral this report can + print is asserted absent, because each one sends the caller back to a wait + with no terminating condition. + """ + st = state(merged=True, mergeable=None, merge_state="unknown") + blockers = mod.evaluate(st, MAIN) + rendered = mod.render(st, MAIN, blockers, "o/r") + + assert mod.primary(blockers).is_terminal is True + assert "already merged into main" in rendered + assert "terminal rather than not-in-yet" in rendered + for deferral in ( + "settles on a later read", + "Re-read the pull request", + "Attempt the merge", + "the answer is not in yet", + "necessary but not sufficient", + mod.MERGE_STATE_UNKNOWN, + ): + assert deferral not in rendered, f"a merged pull request was told to {deferral!r}" + + +def test_a_merged_pull_request_leaves_no_rule_unsatisfied( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """Every rule below is short-circuited, and the exit status stays clean. + + Posed with three rules unsatisfied at once, because that is what makes the + short-circuit the point rather than an artefact of a quiet fixture: on a + change already sitting on the base, "0 of 1 approvals" is not an + outstanding obligation and a red check on a superseded head is not a + finding. A finding here would exit 1 and open an author-side warning on a + pull request whose author has nothing left to do. + """ + st = state( + merged=True, + mergeable=None, + merge_state="unknown", + approvers=(), + unresolved_threads=2, + check_conclusions={REQUIRED: "failure"}, + ) + blockers = mod.evaluate(st, MAIN) + + assert outcomes(blockers) == [mod.ALREADY_MERGED] + assert mod.primary(blockers).owed_by == mod.NOBODY + assert mod.primary(blockers).is_finding is False + + monkeypatch.setattr(mod, "resolve_ruleset", lambda repo, ref, token: MAIN) + monkeypatch.setattr(mod, "resolve_state", lambda repo, pr, token: st) + assert mod.main(["--repo", "o/r", "--pr", "1", "--token", "t"]) == 0 + + +def test_the_resolved_state_carries_the_merged_key_it_already_fetched( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """The read is the fix; the fixtures above cannot see a key nobody reads. + + ``resolve_state`` fetched the whole pull request object and took five keys + from it. ``merged`` was in the same response the whole time, so this pins + the wiring rather than the classification: without it the field keeps its + ``False`` default and every merged pull request reports the transient again. + """ + monkeypatch.setattr( + mod, + "_get", + lambda url, token: { + "head": {"sha": "b6c49eca"}, + "base": {"ref": "main"}, + "draft": False, + "merged": True, + "mergeable": None, + "mergeable_state": "unknown", + }, + ) + monkeypatch.setattr(mod, "resolve_reviews", lambda *a: []) + monkeypatch.setattr(mod, "resolve_unresolved_threads", lambda *a: 0) + monkeypatch.setattr(mod, "resolve_check_conclusions", lambda *a: {}) + monkeypatch.setattr(mod, "resolve_check_suites", lambda *a: ()) + monkeypatch.setattr(mod, "resolve_pusher", lambda *a: "the-author") + + resolved = mod.resolve_state("o/r", 3219, "t") + + assert resolved.merged is True + # And it lands where the classification reads it. + assert outcomes(mod.evaluate(resolved, MAIN)) == [mod.ALREADY_MERGED] + + 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")