Comfy-Org/github-workflows

Fork 0

mirror of https://github.com/Comfy-Org/github-workflows.git synced 2026-11-05 00:17:37 +00:00

[PR #363] [MERGED] fix(cursor-approve): withdraw the bot's standing request for changes once auto-approve stops deciding #363

New issue

Closed

opened 2026-10-26 22:39:03 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-26 22:39:03 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/Comfy-Org/github-workflows/pull/363
Author: @mattmillerai
Created: 10/7/2026
Status: ✅ Merged
Merged: 10/7/2026
Merged by: @mattmillerai

Base: main ← Head: matt/be-19492-clear-standing-block

📝 Commits (3)

eac5b2d fix(cursor-approve): withdraw the bot's standing request for changes once auto-approve stops deciding
8c7c2f8 fix(cursor-approve): address panel review on #363 (BE-19492)
8eb10ab fix(cursor-approve): rank the hand-off above a move on the deferred path too (BE-19492)

📊 Changes

10 files changed (+531 additions, -83 deletions)

View changed files

📝 .github/cursor-approve/card.py (+8 -1)
📝 .github/cursor-approve/tests/test_card.py (+3 -3)
📝 .github/cursor-review/README.md (+4 -3)
📝 .github/cursor-review/auto-approve.py (+196 -41)
📝 .github/cursor-review/tests/test_approve_external.py (+101 -4)
📝 .github/cursor-review/tests/test_auto_approve.py (+150 -8)
📝 .github/workflows/cursor-approve.yml (+14 -7)
📝 .github/workflows/cursor-review.yml (+10 -3)
📝 docs/callers/cursor-approve.md (+27 -9)
📝 docs/callers/cursor-review.md (+18 -4)

📄 Description

Builds on #362's standing-block code (card_for_none, post_standing_change_request, external_block), now merged to main.

Problem

#362 makes auto-approve leave a standing REQUEST_CHANGES (as the approver identity) whenever a round withholds approval. Only an approving round ever withdraws it. Some states mean no approving round will come, and in those states the bot's CHANGES_REQUESTED keeps blocking the merge until someone dismisses it by hand:

Veto. The PR is labelled skip-cursor-review. dismiss-stale withdraws approvals but only looks at APPROVED.
Threshold unset. The caller clears approve_max_severity, so decide no longer runs.
Capped. The PR carries needs-human-review. card_for_none's capped branch left the earlier block standing, and approve-external's needs_human outcome posted a new one. The card's next step ("remove the label to reset the round count") clears nothing.

Change

These rules apply only to the bot's own marked, unedited CHANGES_REQUESTED (APPROVE_MARKER in the body, approver login). A human's review and a bot review someone edited are never touched. All selection goes through one new helper, own_change_requests, which dismiss_own_change_requests now uses too.

dismiss-stale (the Dismiss stale auto-approval job runs on every event of an open PR). It now also withdraws the blocks when the live PR carries skip-cursor-review or needs-human-review, or when the new --threshold is passed empty. The workflow passes inputs.approve_max_severity. An absent --threshold (None) checks only the labels. Further details:
- needs-human-review now withdraws every marked approval too, as the veto already did. Otherwise withdrawing the newer block would let an older on-head approval count again.
- The empty-threshold kill switch withdraws no approvals, so it leaves the block standing while any of this identity's approvals still stands.
- A plain push still leaves the block standing, as before (see Chesterton below).
- A failed dismissal turns the job red, the same as a stale approval. Unreadable labels withdraw nothing.
List first, then confirm the label. Every hand-off withdrawal goes through withdraw_handed_off_change_requests. It lists the reviews, then re-reads the label on the live PR before dismissing. A run that finishes late therefore cannot sweep a block a newer round posted after the label came off. This costs one extra PR read, and only when there is a block to withdraw.
decide, capped branch (card_for_none): posts no block, as before, and now also withdraws the earlier ones. The post-write needs-human-review race path does the same:
- The review that raced the label (APPROVE or REQUEST_CHANGES) is dismissed by the id the POST returned, not found by re-listing. Then the earlier blocks go too.
- The hand-off outranks a moved head or base: no standing block is re-posted on a PR handed to a human.
approve-external: needs_human leaves EXTERNAL_BLOCK_OUTCOMES. That outcome and vetoed now withdraw the blocks through EXTERNAL_HANDOFF_MESSAGES instead of posting one. needs_human is classified ahead of superseded, both before and after the POST. not_approved / error / superseded keep posting.
withdraw --approve-gate capped (cursor-approve's start phase). On the round-cap path no cursor-review decide runs, so this phase withdraws the blocks itself. It re-reads needs-human-review off the live PR rather than trusting the caller's gate. A failed dismissal turns the step red. The start-card step now runs under !cancelled() (guarded on a resolved login), so the card is still written.
Card text: NEXT_HUMAN_CAPPED_TEXT now names what actually clears the PR, without claiming the withdrawal succeeded: "the bot withdraws its own request for changes on this hand-off, so a human's review decides this PR — if one still shows, its withdrawal failed (see the run log) and it needs dismissing by hand. Removing the needs-human-review label resets the round count."
Docs: docs/callers/cursor-review.md (Auto-approve and Dismiss sections) and docs/callers/cursor-approve.md ("The status card contract"). The latter drops the "Known gap" paragraph #362 added. Also the cursor-review README and the cursor-approve.yml header.

The ordering rule is unchanged: when a block is posted it is still "post new, then dismiss older by review id". A hand-off posts nothing, so it only dismisses.

Judgment calls

The veto dismisses fail-round blocks too, not only no-decision ones. The ticket suggested restricting this to blocks auto-approve itself posted. I did not, for two reasons. No round runs on a vetoed PR, so a fail block is just as unclearable as a no-decision one. And the label is a human's explicit "the bot does not decide this PR". Restricting it would need a new body marker, and existing blocks would lack it.
needs_human now withdraws instead of posting. That reverses the block #362 added on needs_human. The ticket offered both options. Withdrawing is the one where the capped next step describes an action that actually clears the PR. The card still reports capped / human.
Chesterton. The job comment and module docstring both said "CHANGES_REQUESTED is deliberately NOT dismissed". The reason given was that a push starts no round under a label-triggered caller, so dismissing on push would let any trivial push clear the veto. That still holds, and push behaviour is unchanged (pinned by test_a_push_keeps_the_block). The new dismissals fire only in states where no round can ever come. The old comment (A REQUEST_CHANGES withholds nothing, so it stays.) on decide's label-race check is rewritten to match.

Residual

Round cap plus a plain cursor-review caller (no cursor-approve), when the cap label is applied with GITHUB_TOKEN. A GITHUB_TOKEN label fires no labeled event, and decide does not run on the cap run. The block then stays until the PR's next event, which runs dismiss-stale and sees the label. Under a GitHub App poster the labeled event fires and clears it right away. cursor-approve callers are covered by the start phase. The fix would mean giving the round-cap job the approver identity, which this PR does not do. Proposed as a follow-up from the panel review.
A skip-cursor-review veto applied by someone who cannot dismiss reviews (a triage role, or restricted dismissals) now clears the bot's block. That follows from the Judgment call above. Restricting it is a policy decision, proposed as a follow-up from the panel review.
A caller that computes approve_max_severity per event. For example, ${{ cond && 'low' || '' }}. On every event where it evaluates empty, the new path withdraws that caller's blocks. The docs already describe the input as a static kill switch. No such caller was checked: caller rosters are private secrets and not readable from here.
Not exercised live. Every path is covered by stubbed-gh unit tests only. No real PR was labelled or dismissed against.

Tests

New or rewritten cases in test_auto_approve.py and test_approve_external.py:

dismiss-stale (the new HandedOffBlockTest):
- A veto and needs-human-review each withdraw only the bot's own block, along with every bot approval. A human's REQUEST_CHANGES, an edited bot block and an unmarked one stay.
- A label that is gone by the re-read keeps the block.
- An empty or whitespace threshold clears the block, unless a bot approval still stands. A push, with or without a threshold, does not.
- No approver identity means nothing is touched, and unreadable labels withdraw nothing.
- A failed dismissal goes red, and the job passes --threshold.
decide: the capped round withdraws the earlier block (human and edited ones stay), and its card's next step says so. Both label-race paths withdraw by the posted id. A move together with the label posts no new block, on the posting path and the deferred (--defer-approval) path alike.
approve-external: needs_human posts nothing and withdraws only the bot's block, including when the label lands mid-POST and when the head also moved. A label gone by the re-read keeps the block. vetoed withdraws too. A failed withdrawal is red.
withdraw: on capped it withdraws the block only while the live label is present (on pass or empty it does not). A failed withdrawal is red, and the start-card step keeps !cancelled(). The start phase passes the gate.
Existing tests rewritten for intended behaviour changes: needs_human used to post a block, a REQUEST_CHANGES racing the label used to stay, needs-human-review used to leave an on-head approval, and withdraw used to stay green on a failed withdrawal.

Provenance

Authored by: agent-work loop
Verified:
- Rebased onto main after #362 merged; the PR's own commits applied unchanged
- .github/cursor-review unit suite: 1082 tests OK at the current head; the new moved-and-labelled deferred subtest fails against the previous ordering
- .github/cursor-approve (161), .github/workflow-pins (551) and .github/agents-md-integrity (46) suites OK
- check_workflow_pins.py, check_agents_md.py and check-org-repo-literals.sh pass
- No live GitHub run: every path is covered by stubbed-gh unit tests only
Deviations:
- Two ticket options taken as described under Judgment calls: the veto dismisses all of the bot's blocks, and needs_human withdraws instead of posting.
- Added the withdraw --approve-gate path, which the ticket did not name, to cover the round-cap path.
- From the panel review: needs-human-review also withdraws the bot's approvals, and the start-card step runs under !cancelled().
- From CodeRabbit: the deferred path ranks the hand-off above a move, and test_card.py's raw U+200B lookaheads are escaped.

🤖 Generated with Claude Code

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/Comfy-Org/github-workflows/pull/363 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 10/7/2026 **Status:** ✅ Merged **Merged:** 10/7/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/be-19492-clear-standing-block` --- ### 📝 Commits (3) - [`eac5b2d`](https://github.com/Comfy-Org/github-workflows/commit/eac5b2da8c452e501fca868cf1f7fdffcc471842) fix(cursor-approve): withdraw the bot's standing request for changes once auto-approve stops deciding - [`8c7c2f8`](https://github.com/Comfy-Org/github-workflows/commit/8c7c2f8fab46fbe6f3387d09b00aa011def1fdac) fix(cursor-approve): address panel review on #363 (BE-19492) - [`8eb10ab`](https://github.com/Comfy-Org/github-workflows/commit/8eb10abb7330e1d378ba3c2874b612d8b0a27bee) fix(cursor-approve): rank the hand-off above a move on the deferred path too (BE-19492) ### 📊 Changes **10 files changed** (+531 additions, -83 deletions) <details> <summary>View changed files</summary> 📝 `.github/cursor-approve/card.py` (+8 -1) 📝 `.github/cursor-approve/tests/test_card.py` (+3 -3) 📝 `.github/cursor-review/README.md` (+4 -3) 📝 `.github/cursor-review/auto-approve.py` (+196 -41) 📝 `.github/cursor-review/tests/test_approve_external.py` (+101 -4) 📝 `.github/cursor-review/tests/test_auto_approve.py` (+150 -8) 📝 `.github/workflows/cursor-approve.yml` (+14 -7) 📝 `.github/workflows/cursor-review.yml` (+10 -3) 📝 `docs/callers/cursor-approve.md` (+27 -9) 📝 `docs/callers/cursor-review.md` (+18 -4) </details> ### 📄 Description Builds on #362's standing-block code (`card_for_none`, `post_standing_change_request`, `external_block`), now merged to `main`. ## Problem #362 makes auto-approve leave a standing REQUEST_CHANGES (as the approver identity) whenever a round withholds approval. Only an approving round ever withdraws it. Some states mean no approving round will come, and in those states the bot's CHANGES_REQUESTED keeps blocking the merge until someone dismisses it by hand: 1. **Veto.** The PR is labelled `skip-cursor-review`. `dismiss-stale` withdraws approvals but only looks at `APPROVED`. 2. **Threshold unset.** The caller clears `approve_max_severity`, so `decide` no longer runs. 3. **Capped.** The PR carries `needs-human-review`. `card_for_none`'s capped branch left the earlier block standing, and approve-external's `needs_human` outcome posted a new one. The card's next step ("remove the label to reset the round count") clears nothing. ## Change These rules apply only to the bot's own **marked, unedited** CHANGES_REQUESTED (`APPROVE_MARKER` in the body, approver login). A human's review and a bot review someone edited are never touched. All selection goes through one new helper, `own_change_requests`, which `dismiss_own_change_requests` now uses too. - **`dismiss-stale`** (the `Dismiss stale auto-approval` job runs on every event of an open PR). It now also withdraws the blocks when the live PR carries `skip-cursor-review` or `needs-human-review`, or when the new `--threshold` is passed **empty**. The workflow passes `inputs.approve_max_severity`. An **absent** `--threshold` (None) checks only the labels. Further details: - `needs-human-review` now withdraws every marked approval too, as the veto already did. Otherwise withdrawing the newer block would let an older on-head approval count again. - The empty-threshold kill switch withdraws no approvals, so it leaves the block standing while any of this identity's approvals still stands. - A plain push still leaves the block standing, as before (see Chesterton below). - A failed dismissal turns the job red, the same as a stale approval. Unreadable labels withdraw nothing. - **List first, then confirm the label.** Every hand-off withdrawal goes through `withdraw_handed_off_change_requests`. It lists the reviews, then re-reads the label on the live PR before dismissing. A run that finishes late therefore cannot sweep a block a newer round posted after the label came off. This costs one extra PR read, and only when there is a block to withdraw. - **`decide`, capped branch** (`card_for_none`): posts no block, as before, and now also withdraws the earlier ones. The post-write `needs-human-review` race path does the same: - The review that raced the label (APPROVE or REQUEST_CHANGES) is dismissed by the id the POST returned, not found by re-listing. Then the earlier blocks go too. - The hand-off outranks a moved head or base: no standing block is re-posted on a PR handed to a human. - **`approve-external`**: `needs_human` leaves `EXTERNAL_BLOCK_OUTCOMES`. That outcome and `vetoed` now withdraw the blocks through `EXTERNAL_HANDOFF_MESSAGES` instead of posting one. `needs_human` is classified ahead of `superseded`, both before and after the POST. `not_approved` / `error` / `superseded` keep posting. - **`withdraw --approve-gate capped`** (cursor-approve's start phase). On the round-cap path no cursor-review `decide` runs, so this phase withdraws the blocks itself. It re-reads `needs-human-review` off the live PR rather than trusting the caller's gate. A failed dismissal turns the step red. The start-card step now runs under `!cancelled()` (guarded on a resolved login), so the card is still written. - **Card text**: `NEXT_HUMAN_CAPPED_TEXT` now names what actually clears the PR, without claiming the withdrawal succeeded: "the bot withdraws its own request for changes on this hand-off, so a human's review decides this PR — if one still shows, its withdrawal failed (see the run log) and it needs dismissing by hand. Removing the `needs-human-review` label resets the round count." - **Docs**: `docs/callers/cursor-review.md` (Auto-approve and Dismiss sections) and `docs/callers/cursor-approve.md` ("The status card contract"). The latter drops the "Known gap" paragraph #362 added. Also the `cursor-review` README and the `cursor-approve.yml` header. The ordering rule is unchanged: when a block is posted it is still "post new, then dismiss older by review id". A hand-off posts nothing, so it only dismisses. ### Judgment calls - **The veto dismisses `fail`-round blocks too, not only no-decision ones.** The ticket suggested restricting this to blocks auto-approve itself posted. I did not, for two reasons. No round runs on a vetoed PR, so a `fail` block is just as unclearable as a no-decision one. And the label is a human's explicit "the bot does not decide this PR". Restricting it would need a new body marker, and existing blocks would lack it. - **`needs_human` now withdraws instead of posting.** That reverses the block #362 added on `needs_human`. The ticket offered both options. Withdrawing is the one where the capped next step describes an action that actually clears the PR. The card still reports `capped` / `human`. - **Chesterton.** The job comment and module docstring both said "CHANGES_REQUESTED is deliberately NOT dismissed". The reason given was that a push starts no round under a label-triggered caller, so dismissing on push would let any trivial push clear the veto. That still holds, and push behaviour is unchanged (pinned by `test_a_push_keeps_the_block`). The new dismissals fire only in states where no round can ever come. The old comment `(A REQUEST_CHANGES withholds nothing, so it stays.)` on decide's label-race check is rewritten to match. ## Residual - **Round cap plus a plain cursor-review caller (no cursor-approve), when the cap label is applied with `GITHUB_TOKEN`.** A `GITHUB_TOKEN` label fires no `labeled` event, and `decide` does not run on the cap run. The block then stays until the PR's next event, which runs `dismiss-stale` and sees the label. Under a GitHub App poster the `labeled` event fires and clears it right away. cursor-approve callers are covered by the start phase. The fix would mean giving the `round-cap` job the approver identity, which this PR does not do. Proposed as a follow-up from the panel review. - **A `skip-cursor-review` veto applied by someone who cannot dismiss reviews** (a triage role, or restricted dismissals) now clears the bot's block. That follows from the Judgment call above. Restricting it is a policy decision, proposed as a follow-up from the panel review. - **A caller that computes `approve_max_severity` per event.** For example, `${{ cond && 'low' || '' }}`. On every event where it evaluates empty, the new path withdraws that caller's blocks. The docs already describe the input as a static kill switch. No such caller was checked: caller rosters are private secrets and not readable from here. - **Not exercised live.** Every path is covered by stubbed-`gh` unit tests only. No real PR was labelled or dismissed against. ## Tests New or rewritten cases in `test_auto_approve.py` and `test_approve_external.py`: - **`dismiss-stale`** (the new `HandedOffBlockTest`): - A veto and `needs-human-review` each withdraw only the bot's own block, along with every bot approval. A human's REQUEST_CHANGES, an edited bot block and an unmarked one stay. - A label that is gone by the re-read keeps the block. - An empty or whitespace threshold clears the block, unless a bot approval still stands. A push, with or without a threshold, does not. - No approver identity means nothing is touched, and unreadable labels withdraw nothing. - A failed dismissal goes red, and the job passes `--threshold`. - **`decide`**: the capped round withdraws the earlier block (human and edited ones stay), and its card's next step says so. Both label-race paths withdraw by the posted id. A move together with the label posts no new block, on the posting path and the deferred (`--defer-approval`) path alike. - **`approve-external`**: `needs_human` posts nothing and withdraws only the bot's block, including when the label lands mid-POST and when the head also moved. A label gone by the re-read keeps the block. `vetoed` withdraws too. A failed withdrawal is red. - **`withdraw`**: on `capped` it withdraws the block only while the live label is present (on `pass` or empty it does not). A failed withdrawal is red, and the start-card step keeps `!cancelled()`. The start phase passes the gate. - Existing tests rewritten for intended behaviour changes: `needs_human` used to post a block, a REQUEST_CHANGES racing the label used to stay, `needs-human-review` used to leave an on-head approval, and `withdraw` used to stay green on a failed withdrawal. ## Provenance - **Authored by:** agent-work loop - **Verified:** - Rebased onto `main` after #362 merged; the PR's own commits applied unchanged - `.github/cursor-review` unit suite: 1082 tests OK at the current head; the new moved-and-labelled deferred subtest fails against the previous ordering - `.github/cursor-approve` (161), `.github/workflow-pins` (551) and `.github/agents-md-integrity` (46) suites OK - `check_workflow_pins.py`, `check_agents_md.py` and `check-org-repo-literals.sh` pass - No live GitHub run: every path is covered by stubbed-`gh` unit tests only - **Deviations:** - Two ticket options taken as described under Judgment calls: the veto dismisses all of the bot's blocks, and `needs_human` withdraws instead of posting. - Added the `withdraw --approve-gate` path, which the ticket did not name, to cover the round-cap path. - From the panel review: `needs-human-review` also withdraws the bot's approvals, and the start-card step runs under `!cancelled()`. - From CodeRabbit: the deferred path ranks the hand-off above a move, and `test_card.py`'s raw U+200B lookaheads are escaped. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-26 22:39:03 +00:00

closed this issue
added the
pull-request
label

No labels

cursor-review-catalog-drift

groom

pull-request

No milestone

No project

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

Comfy-Org/github-workflows#363

No description provided.

Rows
Columns