Skip to content

fix(scripts): a merged pull request is not a mergeability GitHub is still computing - #3233

Merged
cagataycali merged 1 commit into
strands-labs:mainfrom
cagataycali:fix/merged-pull-request-is-not-a-pending-computation
Sep 5, 2026
Merged

cagataycali merged 1 commit into
strands-labs:mainfrom
cagataycali:fix/merged-pull-request-is-not-a-pending-computation

Conversation

@cagataycali

@cagataycali cagataycali commented Sep 5, 2026

Copy link
Copy Markdown
Member

The report told a merged pull request to wait for something that never arrives

check_merge_blockers.py --pr N classified an already-merged pull request as
merge-state-unknown and prescribed "Re-read the pull request: mergeability is
computed on demand and settles on a later read."
It does not settle. A merged
pull request is closed, so mergeable stays null permanently, and the report's
next-action line reads in the reassuring direction while a merge-and-verify
cycle polls for an answer it already has.

Before / after, on the same pull request (strands-labs/robots#3219, merged 12:26 UTC)

before Outcome: merge-state-unknown
No party owes an action; the answer is not in yet.
Re-read the pull request: mergeability is computed on demand and settles on a later read.
after Outcome: already-merged
No party owes an action, and none will: already-merged.
This is terminal rather than not-in-yet -- no later read changes it.
The pull request is already merged into main, so no rule is outstanding.re-reading this one cannot settle it

mergeable: null had three readings, and the script modelled one

flowchart LR
    N["mergeable: null"] --> Q{"merged?"}
    Q -->|"false, open"| T["merge-state-unknown<br>transient - a re-read settles it<br>(#2585, kept unchanged)"]
    Q -->|"true, closed"| A["already-merged<br>terminal - no read ever settles it<br>(new)"]
    style A fill:#dcffe0,stroke:#1a7f37
    style T fill:#eef2ff,stroke:#3b5bdb
Loading

Measured, GET /repos/strands-labs/robots/pulls/{n}, 2026-09-05

Every field the script read was identical across the merged rows and the open
one, which is why mergeable is None could not carry the distinction. The
transient #2585 was written for is live at the same time, so it must keep its
re-read.

pull request state merged mergeable mergeable_state before after
#3219 (merged 21 min earlier) closed true null unknown merge-state-unknown + re-read already-merged
#3230 (merged 10 min earlier) closed true null unknown merge-state-unknown + re-read already-merged
#3196 (merged 1 day earlier) closed true null unknown merge-state-unknown + re-read already-merged
#2586 (merged 14 days earlier) closed true null unknown merge-state-unknown + re-read already-merged
#3205 (open) open false true blocked missing-approval missing-approval (unchanged)
#2907 (open) open false true blocked pusher-only-approval pusher-only-approval (unchanged)

#2586 is the row that makes "re-read" wrong rather than merely early: fourteen
days on, it still reads null. A second read of #3219 ten seconds later also
returned null.

The change

The field that disambiguates was already in the payload — resolve_state
fetched the whole REST pull request object and took five keys from it, and
merged came back in the same response.

  • PullRequestState.merged, populated from that key.
  • A terminal already-merged outcome, owed by nobody, 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, so the exit status keeps its meaning and no author-side warning
    opens on a pull request whose author has nothing left to do.
  • primary / _next_action gain a terminal tier ahead of the gating one.
    Terminal is 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 every remedy this report can print — re-read, re-run, attempt the merge —
    is wrong for it. Held as a set because both renderers are handed blockers
    alone, and the module's rule is that precedence cannot apply to one report and
    not the other.
  • Docstring: the merge-state-unknown entry's "every open pull request"
    clause no longer implies the merged case is covered.

Reading a null as clean is still wrong (#1035) and is untouched.

Tests

5 cells beside the mergeable=None cluster. The table cell carries the argument:
two rows one field apart, both mergeable=None/merge_state="unknown".

cell pre-fix
…and_an_open_recomputing_one_do_not_read_the_same[already-merged] assert ['merge-state-unknown'] == ['already-merged']
…and_an_open_recomputing_one_do_not_read_the_same[open-and-recomputing] passes — the control: the transient is unchanged
…is_not_offered_a_remedy_that_cannot_terminate assert False is True
…leaves_no_rule_unsatisfied assert ['merge-state-unknown', 'missing-approval'] == ['already-merged']
the_resolved_state_carries_the_merged_key_it_already_fetched assert False is True

Pre-fix (behavioural lines reverted, new symbols kept so the module still
imports): 4 failed, 81 passed. Owner coverage needs no new cell — the
existing AST dataflow grader test_every_outcome_the_report_can_emit_has_an_owner
already fails on an outcome missing from _OWED_BY.

Gate

tests/test_merge_blockers.py 80 passed (base) → 85 passed (+5 == the new cells)
targeted suites (merge_blockers, duplicate_claim_check, closing_reference_gate, checkout_is_pr_head, pull_request_trigger_types) 270 passed
scripts/check_whole_tree_graders.py 13F/4372P/56S/4E in 448s — the standing absent-dependency set, unchanged
ruff check + format --check (CI scope) clean, 1888 files
mypy (CI pin 1.20.2, cold cache both trees) 20 errors / 6 files, error sets byte-identical base vs dev; zero in either changed file
mypy scripts/check_merge_blockers.py Success: no issues found

Not in scope, and measured rather than assumed

A pull request closed without merging does not share the null: #3194 reads
merged: false, mergeable: true, mergeable_state: blocked — it retains its
last computed value. So it is not a second instance of this defect. It is still
a distinct reading (abandoned, not landed) with a different owner, and is left
for a separate decision.

Size

file +
scripts/check_merge_blockers.py 82 2
tests/test_merge_blockers.py 123 0
AGENTS.md 12 3
changelog.d/3233-…md 29 0
total 246 5

Net-positive: 82 production lines for the classification, its precedence tier
and the docstring correction; the rest is the pin, the log and the operator doc.

Closes #3231

AI Contribution Disclosure: This change was generated with AI assistance
using Strands Agents. Every number above
was produced against the live API and this tree.

…till 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 -- strands-labs#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 strands-labs#3219
and strands-labs#3230 squashed, both merged rows and open strands-labs#3205 were identical in every
field the script read, so the genuine transient strands-labs#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 (strands-labs#1035), and the open recomputing path
is unchanged -- pinned by the control row of the new table cell.

Closes strands-labs#3231
@cagataycali
cagataycali force-pushed the fix/merged-pull-request-is-not-a-pending-computation branch from ac432df to ff04842 Compare September 5, 2026 13:07
@cagataycali
cagataycali enabled auto-merge (squash) September 5, 2026 13:08

@yinsong1986 yinsong1986 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

Teaches scripts/check_merge_blockers.py --pr N to distinguish the two situations that share mergeable: null: an open pull request GitHub is still computing (kept as the transient merge-state-unknown with its re-read remedy, per #2585) and a pull request that has already merged, whose null is permanent because mergeability is only computed for open pull requests. The new already-merged outcome reads merged from the same REST payload resolve_state already fetches, short-circuits every ruleset rule (an approval count on a landed change is not an outstanding obligation), and is deliberately not a finding, so the exit-status contract is unchanged. The precedence change is sound: evaluate returns the terminal blocker alone, and the new terminal tier in primary/_next_action is held as a shared set so both renderers apply the same ordering, consistent with the module's own rule. The merged field is keyword-defaulted so existing constructors are untouched, and the sweep path is unaffected by construction (a merged pull request never appears in the open set). Tests pin both directions of the classification (the one-field-apart parametrized pair), the absence of every deferral remedy from the merged report, the short-circuit under three simultaneously-unsatisfied rules with a clean exit, and the resolve_state wiring itself; the existing AST dataflow grader covers the new outcome's _OWED_BY entry without an edit. AGENTS.md's step-8 outcome inventory is updated in the same change, and the changelog fragment follows the <pr-number>-<slug>.md convention. No security, crash, data-integrity, or one-way-door concerns: this is internal triage tooling with its output vocabulary extended additively and its exit semantics preserved.

What's good

  • The control row (open-and-recomputing) is pinned alongside the fix, so the genuine transient #2585 was written for cannot regress silently.
  • The not-in-scope section measures the closed-without-merge case (#3194) rather than assuming it shares the null, and correctly leaves it for a separate decision.
  • Owner coverage rides the existing dataflow grader instead of a copied list, per the repo's derive-don't-restate convention.

@cagataycali
cagataycali merged commit 85d6e65 into strands-labs:main Sep 5, 2026
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

check_merge_blockers.py reports an already-merged pull request as merge-state-unknown and prescribes a re-read that can never settle

2 participants