Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 14 additions & 1 deletion AGENTS.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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.
Expand Down
29 changes: 29 additions & 0 deletions changelog.d/3233-already-merged-is-not-a-pending-computation.md
Original file line number Diff line number Diff line change
@@ -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.
84 changes: 80 additions & 4 deletions scripts/check_merge_blockers.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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"
Expand All @@ -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,
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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:
Expand All @@ -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
Expand Down Expand Up @@ -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:
Expand Down Expand Up @@ -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,
Expand All @@ -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]),
),
),
)

Expand All @@ -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]
Expand Down
123 changes: 123 additions & 0 deletions tests/test_merge_blockers.py
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Expand Down
Loading