Skip to content

fix(scripts): a standing request for changes is a blocker owed by the account that made it - #3268

Merged
cagataycali merged 2 commits into
strands-labs:mainfrom
cagataycali:fix/merge-blockers-name-a-standing-request-for-changes
Sep 6, 2026
Merged

cagataycali merged 2 commits into
strands-labs:mainfrom
cagataycali:fix/merge-blockers-name-a-standing-request-for-changes

Conversation

@cagataycali

Copy link
Copy Markdown
Member

What

scripts/check_merge_blockers.py modelled only the approval side of the review decision. A pull request sitting at reviewDecision: CHANGES_REQUESTED was therefore reported as missing-approval, owed by a reviewer other than the pusher -- and that party's approval cannot clear it.

With required reviews in force, a standing request for changes holds the merge until its own author approves or dismisses it. Another account's approval satisfies required_approving_review_count and the pull request stays BLOCKED. So this is the #1905 presentation reached from the review-decision side rather than the last-push side: a state that reads as ordinary reviewer latency, needs one specific named account, and gets a round spent on a reviewer who cannot move it.

Adds a changes-requested outcome, owed by the reviewer who requested changes, reported ahead of the approval rules it is not answerable by.

Why nothing caught it

Every sweep this repository has reads clean on the shape. Measured on #3205:

signal reading while it was blocked
reviewDecision CHANGES_REQUESTED
mergeStateStatus BLOCKED
review threads 1, resolved
check_thread_is_answered.py --pr 3205 nothing-owed
check_merge_blockers.py missing-approval, owed by a reviewer who could not clear it
check_pr_head_is_current.py current
call-test-lint / Test and Lint SUCCESS

Thread resolution and the review decision are separate objects: resolving the thread does not retract the review, so the sweep that answers "does this owe me anything" was correct to say no while the decision went on blocking. And the requester's own follow-up reply is a COMMENTED review, which expresses no position and supersedes nothing -- so the one account that could clear it had already said everything it was going to say.

It stood 15h44m, of which 12h51m was after the fix had landed and the thread was resolved.

Shape

  • Standing has one owner. current_change_requesters lives beside current_approvers in check_last_push_approval.py and both read one shared _latest_positions, so the rule that a COMMENTED review retracts nothing cannot hold on one side and lapse on the other. The sweep binds the sibling's resolver rather than deriving standing itself, exactly as it already does for approvals.
  • Reported, not gated. changes-requested is deliberately absent from _FINDINGS, on the same reasoning that keeps missing-approval out: it is owed by a reviewer, and a finding that fires whenever a review is in progress fires on the ordinary state and stops meaning anything.
  • Not gating either. It does not claim the rules beside it are unassessable -- a failing required check is independently the author's to fix, and _next_action's gating language would misdescribe that.
  • Scoped to the rule. Only reported where the ruleset actually requires approving reviews, for the same reason the pusher discount is: reporting it unconditionally would invent a blocker on a branch whose ruleset does not hold the merge for a review at all.
  • Names the account, not just the role. the reviewer who requested changes is not actionable on its own; the detail line names each holding account, since on a pull request with several reviewers the report is the only place that is written down.

Tests

13 new cells, all 13 fail on pre-fix code (7 over the sweep's verdict, 6 over the shared resolver):

scripts reverted to main, new pins run:  13 failed, 139 passed
with the change:                         152 passed

The #3205 cell is the counterfactual itself -- it asserts the primary blocker is changes-requested owed by REQUESTING_REVIEWER, and explicitly that it is not OTHER_REVIEWER, which was the pre-fix answer. The third-party-approval cell is the one that shows the two questions are genuinely different rather than two spellings of one: with an eligible approval present there is no approval rule left to report, so a check modelling only approvals reports nothing at all and the reader concludes the branch is ready.

Gate

check result
pytest tests/test_merge_blockers.py tests/test_last_push_approval.py 152 passed (was 139)
new pins against pre-fix scripts 13 failed, as intended
ruff check clean
ruff format --check clean
mypy scripts/check_merge_blockers.py scripts/check_last_push_approval.py Success: no issues found in 2 source files
whole-tree graders (104), base vs head 0 attributable -- 110 failed / 2139 passed on both, node-id sets identical in both directions
non-ASCII sweep on changed Python clean
behind_by vs main 0

The 110 failures and 178 collection errors in the grader roster are environmental in this sandbox (no torch, mujoco, zenoh) and reproduce byte-identically on the unmerged base, which is why the claim is the delta and not the count.

Docs

AGENTS.md gains CHANGES_REQUESTED as the fourth reading of reviewDecision, beside the existing null / APPROVED / REVIEW_REQUIRED ones, and the sweep's enumeration of parties gains this outcome so the file cannot drift from the tool.

Advances the #1905 class. Claims no issue.

Round log

  • Round 1 -- opened. Draft first so the changelog fragment lands before a review can arrive, per PR Workflow step 3.

… account that made it

`check_merge_blockers.py` modelled only the approval side of the review
decision, so a pull request sitting at `CHANGES_REQUESTED` was reported as
`missing-approval` owed by "a reviewer other than the pusher". That party
cannot clear it. With required reviews in force a standing request for changes
holds the merge until its own author approves or dismisses it, so another
account's approval satisfies `required_approving_review_count` and the pull
request stays `BLOCKED` -- the strands-labs#1905 presentation reached from the
review-decision side rather than the last-push side.

Measured on strands-labs#3205: `CHANGES_REQUESTED` stood 15h44m with its one review thread
resolved, `call-test-lint` SUCCESS, and `check_thread_is_answered.py` reading
`nothing-owed`. 12h51m of that was after the fix had landed. Thread resolution
and review decision are separate objects, and the requester's own follow-up
reply is a `COMMENTED` review, which expresses no position and supersedes
nothing -- so every sweep the repository has read clean while the decision went
on blocking.

The new `changes-requested` outcome is reported ahead of the approval rules it
is not answerable by, names the holding accounts in its detail rather than only
the role, and is scoped to a ruleset that actually requires reviews. It is
deliberately not a finding: it is owed by a reviewer, and a finding that fires
whenever a review is in progress fires on the ordinary state.

Standing is resolved by the sibling check rather than re-derived, so the rule
that a `COMMENTED` review retracts nothing has one owner across both questions.

Pinned by 13 cells that fail on pre-fix code, 7 of them over the sweep's
verdict and 6 over the shared resolver.
@cagataycali
cagataycali marked this pull request as ready for review September 6, 2026 17:38

@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 changes-requested outcome to scripts/check_merge_blockers.py, owed by the reviewer who requested changes, reported ahead of the approval rules it is not answerable by -- closing the gap where a CHANGES_REQUESTED review decision was reported as missing-approval owed by a party whose approval could not merge anything (the #1905 presentation reached from the review-decision side, measured on #3205). Standing gets one owner: current_change_requesters lives beside current_approvers in check_last_push_approval.py over a shared _latest_positions resolver, so the COMMENTED-retracts-nothing rule cannot hold on one side and lapse on the other, and the sweep binds the sibling's resolver rather than re-deriving it. I verified the counterfactual claim independently: with scripts/ reverted to main, the 13 new pins fail (13 failed / 139 passed) and with the change all 152 pass. Nothing here is a security surface, a wire format, or a public API one-way door -- these are repository maintenance scripts with an outcome vocabulary that this PR also keeps in sync with AGENTS.md.

What's good

  • Every new pin fails on pre-fix code, per AGENTS.md > Review Learnings (#85) > Testing > "Pin regression tests for reviewed fixes" -- reproduced locally, not taken on trust.
  • CHANGES_REQUESTED is deliberately absent from _FINDINGS and _GATING, consistent with the module's own reasoning for keeping missing-approval out; the third-party-approval test is the cell that shows the two questions are genuinely distinct.
  • Scoped to required_approving_review_count, so the outcome cannot be invented on a branch whose ruleset does not hold the merge for a review -- same discipline as the existing pusher discount.
  • The relevant whole-tree graders (test-name behaviour, ASCII tool strings, host paths, docstring xrefs, review-archaeology) all pass on the head tree; ruff check and ruff format --check clean on the four changed Python files; changelog fragment named with the PR number per step 3.
  • DISMISSED was already in POSITION_STATES on main, so the _latest_positions extraction is a pure refactor of the approval side -- the 139 pre-existing tests passing unmodified confirms it.

@cagataycali
cagataycali merged commit 074372b into strands-labs:main Sep 6, 2026
10 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.

2 participants