Skip to content

fix(scripts): a merged pull request is not a mergeability still being computed - #3232

Closed
cagataycali wants to merge 2 commits into
strands-labs:mainfrom
cagataycali:merge-blockers-already-merged-is-terminal
Closed

cagataycali wants to merge 2 commits into
strands-labs:mainfrom
cagataycali:merge-blockers-already-merged-is-terminal

Conversation

@cagataycali

Copy link
Copy Markdown
Member

Closes #3231

The defect

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. The report described a wait with no terminating condition, and it read in the
reassuring direction: "No party owes an action; the answer is not in yet" is what a
caller polling "can I merge yet" saw while the answer was "you already did".

Measured

GET /repos/strands-labs/robots/pulls/{n}, 2026-09-05, minutes after #3219 and #3230
squashed:

pull request state merged mergeable mergeable_state before after
#3219 closed true null unknown merge-state-unknown, "Re-read" already-merged
#3230 closed true null unknown merge-state-unknown, "Re-read" already-merged
#3205 open false null unknown merge-state-unknown, "Re-read" unchanged - correct here

The third row is the reason this is not a dead state with a one-line special case: the
genuine transient #2586 was written for is live at the same moment, on the same fields.
All three rows were byte-identical in every field this check read, so mergeable is None
was ambiguous between "still computing" and "nothing left to compute".

Why this line

resolve_state fetches the whole REST pull request object and read five keys from it -
head, base, draft, mergeable, mergeable_state. The same response carries
merged, which answers the question outright. This is the "fetched and not read" failure
AGENTS.md names as the recurring root cause, in the one script whose stated purpose is
"ask for the verdict rather than re-deriving it".

The script's own docstring stated the assumption that fails, and it is now corrected:

A 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.

True, and it is why merge-state-unknown exists. But the pull request whose own merge
invalidated the value is not open, and nothing downstream re-checked that.

This is not a revert of #2586. Reading the null as clean is still wrong (#1035 read
pusher-only-approval on a DIRTY branch). The null simply has a third reading #2585 did
not have to separate - the same shape as reviewDecision: null meaning two things in
AGENTS.md. A control cell pins the open cluster untouched across all three mergeable
values.

The change

Four edits in the seam that already existed:

  • PullRequestState gains merged: bool = False, and resolve_state populates it from the
    key already fetched.
  • A terminal already-merged outcome, owed by NOBODY, and not a finding - the exit status
    is for what an author-side pass can act on alone, and there is nothing left to act on.
  • evaluate() returns it alone, ahead of every rule including the draft check, rather
    than adding it to the gating list. Gating means the rules below cannot be assessed yet;
    here there is nothing left to assess in any future read, and naming "0 of 1 approvals"
    beside it would invent an obligation against a closed pull request.
  • It is in _GATING so the report prints Next action is owed by nobody, on `already-merged` rather than falling through to the ungated wording "the answer is not
    in yet", which is the sentence that was measured as misleading.

No new transport, no new query, no signature change.

Tests

tests/test_merge_blockers.py, beside the existing mergeable=None cluster, with the
measured triple as the fixture header.

The core cell is table-driven over the one field that was not being read:
test_the_same_null_mergeability_gets_two_verdicts_by_whether_it_merged, parametrized
merged-3219-and-3230 / open-3205. Also added: no rule is named underneath a merged pull
request (hostile inputs - no approval, two unresolved threads, no check conclusion); the
verdict is owed by nobody and is not a finding; neither the re-read remedy, the ungated
"answer is not in yet" wording, nor #2574's merge attempt is printed; the blocker is
reported alone so "necessary but not sufficient" cannot appear; and two guards - the merged
verdict never reaches an open pull request at any mergeable value, and resolve_state
actually reads the key.

The existing AST dataflow cell test_every_outcome_the_report_can_emit_has_an_owner grades
the new outcome for free, which is why _OWED_BY is not asserted by spelling.

Pre-fix control (the two behavioural edits reverted, tests and docstrings kept):

FAILED ...::test_the_same_null_mergeability_gets_two_verdicts_by_whether_it_merged[merged-3219-and-3230]
            - AssertionError: assert ['merge-state-unknown'] == ['already-merged']
FAILED ...::test_a_merged_pull_request_names_no_rule_underneath_it
            - AssertionError: assert ['merge-state...ing-approval'] == ['already-merged']
FAILED ...::test_a_merged_pull_request_is_owed_by_nobody_and_is_not_a_finding
FAILED ...::test_a_merged_pull_request_is_not_prescribed_a_read_that_cannot_settle
            - AssertionError: assert 'Re-read' not in '## Merge bl...rrying them.'
FAILED ...::test_a_merged_pull_request_is_reported_alone_rather_than_as_a_gate
FAILED ...::test_the_merged_key_is_read_from_the_payload_that_already_carried_it
6 failed, 82 passed

Exactly the six graders fail; the open-3205 row and the open-pull-request guard stay
green, so they are guards rather than restatements of the fix. The second failure is worth
reading twice - pre-fix a merged pull request was reported as merge-state-unknown plus
missing-approval
, so it was also asking a reviewer to approve a landed change.

Gate

check result
ruff check + format --check clean, 2 files
mypy scripts/check_merge_blockers.py (pinned <2.0) Success: no issues found
pytest tests/test_merge_blockers.py base 80 passed -> 88 passed, +8 = exactly the new cells
pre-fix 6 failed / 82 passed, both guards green
live, merged #3219 and #3230 already-merged, owed by nobody, exit 0
live, open #3205 missing-approval owed by any reviewer - no leak

Deliberately out of scope

A pull request that is closed but not merged reads the same mergeable: null. That is
a fourth verdict with a different meaning again - abandoned rather than landed - and
plausibly a different owner, so it deserves separating. It is a distinct decision from this
one and is not what cost a cycle here, so merged is read rather than state.

The --all-open sweep filters to open pull requests and is unaffected; this lands on the
single-PR path a merge-and-verify cycle uses.

Size: 3 files. Production: one field, one short-circuit, one payload key; the rest is
the regression table, the corrected docstring entry and the changelog fragment.


AI Contribution Disclosure: This pull request was generated with AI assistance using Strands Agents. All code changes have been reviewed for correctness and tested, and every measurement reported above was taken against the live API or a local run.

… computed

check_merge_blockers.py reported 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". A merged pull request is
closed, so mergeable stays null for good and no later read settles anything.

Measured on strands-labs#3219 and strands-labs#3230 right after they squashed: both closed,
merged=true, mergeable=null, mergeable_state=unknown -- byte-identical, in
every field this check read, to open strands-labs#3205's honest recompute at the same
moment. The disambiguating key was already in the payload resolve_state
fetches and was simply not read.

Adds a terminal already-merged outcome, owed by nobody, returned ahead of
every ruleset rule rather than gating them: once the change is on the base,
"0 of 1 approvals" is not a rule the merge left unsatisfied. strands-labs#2586's
treatment of an open pull request's null is untouched, and a control cell
pins that.

Refs strands-labs#3231

@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

Adds a terminal already-merged outcome to scripts/check_merge_blockers.py so a pull request that has already merged is named as such instead of reporting merge-state-unknown with a "re-read, it settles later" remedy that can never settle (a merged PR is closed, so mergeable stays null permanently). The fix reads the merged key already present in the REST payload resolve_state fetches, short-circuits evaluate() ahead of every ruleset rule so no obligation (approval, thread resolution) is invented against a closed PR, and places the outcome in _GATING so the report prints an owner (nobody) rather than the ungated "the answer is not in yet" wording. It is not a finding, so exit-status semantics are unchanged. Verified at head: the full tests/test_merge_blockers.py suite passes (88), ruff check/format clean, all three changed files ASCII-only.

What's good

  • The open-PR cluster is pinned untouched across all three mergeable values (test_the_merged_verdict_does_not_reach_an_open_pull_request), so this is not a revert of #2586 and cannot leak the merged verdict onto a DIRTY branch.
  • The --all-open sweep filters to open pull requests, so the new short-circuit is unreachable there by construction; the one place it can fire (a PR that merges between the list read and the state read) now reports a non-finding instead of a stale verdict, which is strictly better.
  • Hostile-input test (no approval, unresolved threads, no check conclusions) pins that no rule is named underneath a merged PR, which is the half of the pre-fix behaviour worth fixing most - it was also asking a reviewer to approve a landed change.
  • Changelog fragment named per convention (3232-<slug>.md), pushed with the PR; the closed-but-not-merged fourth reading is explicitly scoped out rather than silently conflated.

@cagataycali

Copy link
Copy Markdown
Member Author

Closing in favour of #3233, which implements the same fix and is a superset of this diff. Both were written against #3231 independently -- the duplicate shape #3169 records.

#3232 (this) #3233
merged read in resolve_state yes yes
short-circuit ahead of every rule yes yes
tier the outcome sits in reuses _GATING new _TERMINAL, ahead of gating
_next_action wording the gating line states it is terminal, not not-in-yet
AGENTS.md untouched outcome list + the unqualified re-read prose corrected
closed-unmerged asserted to share the null measured on #3194: it does not, it keeps its last computed mergeable

The two that decide it:

  1. AGENTS.md would be left contradicting the code. It enumerates this script's outcomes and their owners in prose (~L1287) and states the null-mergeability rule as "a merge into the base invalidates it for every open pull request ... re-read to resolve it" (~L1303). With this branch merged, already-merged is absent from that list and the re-read claim stays unqualified -- so the change would need a second pull request to be finished.
  2. _GATING documents an invariant this branch makes false. Its own comment is that the rules behind a gating blocker cannot be assessed "until that clears", and _next_action renders that sentence. already-merged never clears. Nothing renders wrong today only because the short-circuit returns a single blocker, so trailing is empty -- the misreport is latent, waiting for the first caller that reads is_gating as "clears later".

The scoping note this branch added to the merge-state-unknown docstring entry says the same thing as #3233's, so nothing here is lost. No follow-up is needed once #3233 lands.

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