[PR #369] [MERGED] fix(cursor-approve): the remedy #366 advised does not exist — move the head instead (BE-19527) #370

New issue

Closed

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

bsakaguchi commented

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

Owner

📋 Pull Request Information

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

Base: main ← Head: matt/be-19527-fix-remedy

📝 Commits (2)

136d4c7 fix(cursor-approve): the remedy #366 advised does not exist — move the head instead (BE-19527)
b43672b fix(cursor-approve): route the remaining unchanged-head paths to push_then_relabel (BE-19527)

📊 Changes

9 files changed (+203 additions, -93 deletions)

View changed files

📝 .github/cursor-approve/card.py (+41 -18)
📝 .github/cursor-approve/tests/test_card.py (+17 -2)
📝 .github/cursor-review/auto-approve.py (+64 -29)
📝 .github/cursor-review/post-review.py (+4 -1)
📝 .github/cursor-review/tests/test_approve_external.py (+16 -6)
📝 .github/cursor-review/tests/test_auto_approve.py (+32 -20)
📝 .github/cursor-review/tests/test_post_review.py (+2 -2)
📝 .github/workflows/cursor-approve.yml (+1 -1)
📝 docs/callers/cursor-approve.md (+26 -14)

📄 Description

Follow-up to #366, which merged with its review findings unaddressed. Its diagnosis was right; its remedy was not.

The bug #366 got right

On a round whose head never moved, "remove and re-add the label" is a no-op — the gate's dup step skips any head already carrying a bot-posted consolidated review, and a degraded round posts one too.

The bug #366 introduced

It pointed at the dup step's state != "DISMISSED" clause and told people to dismiss that review. They can't. post-review.py submits it with "event": "COMMENT" (4 call sites), and GitHub dismisses only APPROVED/CHANGES_REQUESTED — 422 otherwise, no Dismiss control in the UI. That clause is unreachable for the one review it's matched against. One impossible remedy swapped for another.

What actually clears the dedupe is moving the head. So: push_then_relabel, "push a commit — an empty one is enough — then re-run the round." The relabel stays in the text because a label-gated caller no-ops on synchronize.

Reachable everywhere now, not from one site

no_decision_next was the only path that selected the new value. These still hardcoded relabel on an unchanged head:

the deferred-approval failure path
the failed decision-POST path
the base-only leg of the head/base-moved race (a retarget never moves the head)
card_for_none's card.py-unloadable fallback
card.py's own DECIDE_* maps, for an axes error and for an unknown outcome
cursor-approve's decide, when the base alone was retargeted: that is now its own retargeted outcome (before the POST and in the post-POST race) instead of superseded, which still means a head move and keeps relabel
post-review.py's error review, which told people to relabel the head it sits on

All now share next_for_unchanged_head(), and an unknown outcome falls back to the advice that works whether or not the head moved. The opposite case is handled too: when the deferred-approval path's re-check saw the head move, it advises a plain relabel, not an empty commit.

Two more corrections

The relabel gate was over-narrow. It keyed on auto_retry_eligible, which also requires every reason to be retryable — so a round where the head moved and the judge flaked got sent down the push path, although the live head carried no review and a plain relabel would have started a round. Now keyed on REASON_HEAD_MOVED.

#366's body trim is reverted (now with the same threshold guard render_standing_body has). Dropping the scope note and threshold was my cosmetic preference and it cost real information: under delta scope _trusted_decision can APPROVE with findings above the threshold, and the scope note was the only durable on-PR record that it waived them; and the axes path renders no threshold on its card, so for those blocks the line moved to neither artifact.

Contract

cursor-approve-next is now documented as an open set — treat an unrecognised value as human, so the next addition isn't a break either. push_then_relabel replaces dismiss_then_relabel, which was on main for minutes and which no consumer has pinned (the bump PRs from #366 are unmerged).

Provenance

Every finding here came from #366's own panel review. I verified the High against post-review.py before acting on it. This PR's own panel review (8 findings) is addressed in b43672b. 7 were fixed. One was refuted with live evidence: older consolidated COMMENT reviews keep their own commit_id across pushes, so moving the head does clear the dedupe.

Verified:

python3 -m unittest discover -s .github/cursor-review/tests -p 'test_*.py': 1147 pass
python3 -m unittest discover -s .github/cursor-approve/tests -p 'test_*.py': 162 pass
The new tests for a base-only retarget and a deferred head move fail against the code before the fix
check_workflow_pins.py (plus its unit tests), check_agents_md.py and check-org-repo-literals.sh: pass

Deviations:

Adds a retargeted value to approve-external's internal step output outcome. It is not a workflow_call output; the card and the docs map it.

🤖 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/369 **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-19527-fix-remedy` --- ### 📝 Commits (2) - [`136d4c7`](https://github.com/Comfy-Org/github-workflows/commit/136d4c7ad7f6aee63450cbdf1cd68aea81add1df) fix(cursor-approve): the remedy #366 advised does not exist — move the head instead (BE-19527) - [`b43672b`](https://github.com/Comfy-Org/github-workflows/commit/b43672bbbb401d9bafa1ad3deecc4520f0c4d276) fix(cursor-approve): route the remaining unchanged-head paths to push_then_relabel (BE-19527) ### 📊 Changes **9 files changed** (+203 additions, -93 deletions) <details> <summary>View changed files</summary> 📝 `.github/cursor-approve/card.py` (+41 -18) 📝 `.github/cursor-approve/tests/test_card.py` (+17 -2) 📝 `.github/cursor-review/auto-approve.py` (+64 -29) 📝 `.github/cursor-review/post-review.py` (+4 -1) 📝 `.github/cursor-review/tests/test_approve_external.py` (+16 -6) 📝 `.github/cursor-review/tests/test_auto_approve.py` (+32 -20) 📝 `.github/cursor-review/tests/test_post_review.py` (+2 -2) 📝 `.github/workflows/cursor-approve.yml` (+1 -1) 📝 `docs/callers/cursor-approve.md` (+26 -14) </details> ### 📄 Description Follow-up to #366, which merged with its review findings unaddressed. Its diagnosis was right; its remedy was not. ## The bug #366 got right On a round whose head never moved, "remove and re-add the label" is a **no-op** — the gate's `dup` step skips any head already carrying a bot-posted consolidated review, and a degraded round posts one too. ## The bug #366 introduced It pointed at the dup step's `state != "DISMISSED"` clause and told people to dismiss that review. **They can't.** `post-review.py` submits it with `"event": "COMMENT"` (4 call sites), and GitHub dismisses only `APPROVED`/`CHANGES_REQUESTED` — 422 otherwise, no Dismiss control in the UI. That clause is unreachable for the one review it's matched against. One impossible remedy swapped for another. What actually clears the dedupe is **moving the head**. So: `push_then_relabel`, *"push a commit — an empty one is enough — then re-run the round."* The relabel stays in the text because a label-gated caller no-ops on `synchronize`. ## Reachable everywhere now, not from one site `no_decision_next` was the only path that selected the new value. These still hardcoded `relabel` on an unchanged head: - the deferred-approval failure path - the failed decision-POST path - the base-only leg of the head/base-moved race (a retarget never moves the head) - `card_for_none`'s card.py-unloadable fallback - `card.py`'s own `DECIDE_*` maps, for an axes error and for an unknown outcome - cursor-approve's decide, when the base alone was retargeted: that is now its own `retargeted` outcome (before the POST and in the post-POST race) instead of `superseded`, which still means a head move and keeps `relabel` - `post-review.py`'s error review, which told people to relabel the head it sits on All now share `next_for_unchanged_head()`, and an unknown outcome falls back to the advice that works whether or not the head moved. The opposite case is handled too: when the deferred-approval path's re-check saw the **head** move, it advises a plain relabel, not an empty commit. ## Two more corrections **The relabel gate was over-narrow.** It keyed on `auto_retry_eligible`, which also requires *every* reason to be retryable — so a round where the head moved **and** the judge flaked got sent down the push path, although the live head carried no review and a plain relabel would have started a round. Now keyed on `REASON_HEAD_MOVED`. **#366's body trim is reverted** (now with the same threshold guard `render_standing_body` has). Dropping the scope note and threshold was my cosmetic preference and it cost real information: under `delta` scope `_trusted_decision` can APPROVE with findings above the threshold, and the scope note was the only durable on-PR record that it waived them; and the axes path renders no threshold on its card, so for those blocks the line moved to *neither* artifact. ## Contract `cursor-approve-next` is now documented as an **open set** — treat an unrecognised value as `human`, so the next addition isn't a break either. `push_then_relabel` replaces `dismiss_then_relabel`, which was on main for minutes and which no consumer has pinned (the bump PRs from #366 are unmerged). ## Provenance Every finding here came from #366's own panel review. I verified the High against `post-review.py` before acting on it. This PR's own panel review (8 findings) is addressed in b43672b. 7 were fixed. One was refuted with live evidence: older consolidated COMMENT reviews keep their own `commit_id` across pushes, so moving the head does clear the dedupe. **Verified:** - `python3 -m unittest discover -s .github/cursor-review/tests -p 'test_*.py'`: 1147 pass - `python3 -m unittest discover -s .github/cursor-approve/tests -p 'test_*.py'`: 162 pass - The new tests for a base-only retarget and a deferred head move fail against the code before the fix - `check_workflow_pins.py` (plus its unit tests), `check_agents_md.py` and `check-org-repo-literals.sh`: pass **Deviations:** - Adds a `retargeted` value to approve-external's internal step output `outcome`. It is not a `workflow_call` output; the card and the docs map it. 🤖 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:06 +00:00