[PR #365] [MERGED] feat(cursor-review): re-run a round the head outran, once per PR (BE-19526) #367

New issue

Closed

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

bsakaguchi commented

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

Owner

📋 Pull Request Information

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

Base: main ← Head: matt/be-cursor-approve-auto-retry

📝 Commits (4)

9828f5c feat(cursor-review): re-run a round the head outran, once per PR (BE-19526)
7f4dede Merge remote-tracking branch 'origin/main' into matt/be-cursor-approve-auto-retry
c432a8e fix(cursor-review): relabel from the run's last job, and only once the marked block landed (BE-19526)
4355407 test(workflow-pins): count the auto-retry job's guarded checkout (BE-19526)

📊 Changes

9 files changed (+652 additions, -13 deletions)

View changed files

📝 .github/cursor-approve/card.py (+14 -0)
📝 .github/cursor-review/auto-approve.py (+202 -7)
📝 .github/cursor-review/tests/test_auto_approve.py (+252 -0)
📝 .github/cursor-review/tests/test_workflow_job_isolation.py (+4 -1)
📝 .github/cursor-review/tests/test_workflow_panel_integrity.py (+11 -0)
📝 .github/cursor-review/tests/test_workflow_ref_guard_parity.py (+1 -1)
📝 .github/workflow-pins/tests/test_check_workflow_pins.py (+5 -2)
📝 .github/workflows/cursor-review.yml (+131 -1)
📝 docs/callers/cursor-approve.md (+32 -1)

📄 Description

ELI5

A review panel takes ~45 minutes. If you push during that window, the round ends up judging a commit the PR has already moved past, so it refuses to decide — "the PR head moved while the review ran". That burned a whole panel, and the only way out was for a human to spot the status card and remove/re-add the cursor-review label by hand. Until someone did, the PR sat behind a standing request for changes with nothing coming to clear it.

Now the round re-runs itself. Once per PR.

Why not "review every push"

That was the obvious fix and it's the wrong one — it makes every push to a labelled PR cost a panel, across every consumer repo.

This doesn't react to your push. It reacts to a round that already finished and was wasted. Panels stay bounded by invalidations, not by pushes — a healthy PR runs no extra round however often you push to it. The long panel latency actually helps: by the time the round ends and notices, the pushing has usually stopped, so the retry reviews the settled head instead of chasing a moving one.

When it fires

Only where a fresh round can actually decide differently: the head moved. That's what makes the next round read different content, so the gate's same-SHA dup check can't skip it.

base retarget alongside a head move — rides along
retarget alone, errored panel cell, errored judge, unreadable PR — head unchanged, so the re-run would be deduped away; these still ask for a human, exactly as before
structural causes (findings didn't land as threads, empty diff) — unchanged

The ceiling

One per PR, counted off the PR's own reviews rather than a run-local counter — this job is stateless and the retry's round is a different run. Each retry marks its standing block with , and a marked block still counts after a later round dismisses it, so the bound holds across rounds and runs.

One is deliberate. A second only helps while the author is still pushing, and re-running on every settle is the cost this exists to avoid.

Fails back, never into a loop

unreadable review list, or an unknown approver login → spends the budget rather than retrying blind
the marked standing block did not post (own PR, POST failure) → no retry, and the card keeps the hand-recovery step, since nothing would count the retry
the round is already at max_rounds → no retry (the re-run would cap out and skip its panel)
only an explicit --can-relabel true retries; the GITHUB_TOKEN fallback passes false, because a GITHUB_TOKEN-applied label fires no workflow run (the reason cursor-review-auto-label.yml exists)
failed relabel → the auto-retry job goes red with an ::error:: naming the label; the card already says "if no new round starts, re-run it by hand"

Where the relabel runs

Not in decide. decide sets auto_retry=true, and the relabel runs in a new last job, auto-retry ("Re-run the round (head moved)"). That job needs post-review, panel-integrity, blocking-gate and notify-complete under !cancelled(). The unlabeled/labeled events it fires land in the caller's cancel-in-progress group, so if decide relabelled itself it would cancel the run's own Blocking gate and Panel integrity checks. The DELETE URL-quotes the label and tolerates a 404, and the re-add is tried twice.

The contract marker stays relabel, so an agent reading the card waits for the new round either way — only the prose changes, so nobody re-does by hand what already fired.

Caller impact

None required. Whichever token a caller already configures (APPROVER_TOKEN, or the bot_app_id App) needs label write on the repo; a caller on neither simply never auto-retries.

Provenance

Found while diagnosing why two live PRs were sitting on standing blocks nobody had noticed — in both cases a branch update during the panel. Documented in docs/callers/cursor-approve.md.

Verified:

cursor-review script suite green on the current head (1144 tests), including new coverage for the posted-id gate, round-cap room, fail-closed --can-relabel, label quoting, the 404-tolerant DELETE and the retried re-add
cursor-approve script suite green (161 tests); workflow-pins suite green
check_workflow_pins.py, check_agents_md.py --root . and check-org-repo-literals.sh pass

Deviations:

The relabel moved from decide into a new final auto-retry job. Four structural workflow tests were updated for it: the cursor-review guard count went 10→11 and the workflow-pins guarded-site count went 21→22, the bot-key allowlist gained auto-retry, and panel-integrity's "gates nothing" test now allows this one ordering-only dependent and pins that it runs under !cancelled() and never reads panel-integrity's result

Tests

python3 -m unittest discover -s .github/cursor-review/tests -p 'test_*.py' — 1144 pass
python3 -m unittest discover -s .github/cursor-approve/tests -p 'test_*.py' — 161 pass
python3 -m unittest discover -s .github/workflow-pins/tests -p 'test_*.py' — pass
python3 .github/workflow-pins/check_workflow_pins.py — OK
python3 .github/agents-md-integrity/check_agents_md.py --root . — passed
bash .github/lint/check-org-repo-literals.sh — OK

🤖 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/365 **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-cursor-approve-auto-retry` --- ### 📝 Commits (4) - [`9828f5c`](https://github.com/Comfy-Org/github-workflows/commit/9828f5c993c340d4aa73d49e5e1c659f1b71ae89) feat(cursor-review): re-run a round the head outran, once per PR (BE-19526) - [`7f4dede`](https://github.com/Comfy-Org/github-workflows/commit/7f4dede8dab067a391e65c0caccdadab833c0013) Merge remote-tracking branch 'origin/main' into matt/be-cursor-approve-auto-retry - [`c432a8e`](https://github.com/Comfy-Org/github-workflows/commit/c432a8e8130b76dedaa5962820cb1d6598a31047) fix(cursor-review): relabel from the run's last job, and only once the marked block landed (BE-19526) - [`4355407`](https://github.com/Comfy-Org/github-workflows/commit/43554072d7ebbf9e144c3b644403263726c95c91) test(workflow-pins): count the auto-retry job's guarded checkout (BE-19526) ### 📊 Changes **9 files changed** (+652 additions, -13 deletions) <details> <summary>View changed files</summary> 📝 `.github/cursor-approve/card.py` (+14 -0) 📝 `.github/cursor-review/auto-approve.py` (+202 -7) 📝 `.github/cursor-review/tests/test_auto_approve.py` (+252 -0) 📝 `.github/cursor-review/tests/test_workflow_job_isolation.py` (+4 -1) 📝 `.github/cursor-review/tests/test_workflow_panel_integrity.py` (+11 -0) 📝 `.github/cursor-review/tests/test_workflow_ref_guard_parity.py` (+1 -1) 📝 `.github/workflow-pins/tests/test_check_workflow_pins.py` (+5 -2) 📝 `.github/workflows/cursor-review.yml` (+131 -1) 📝 `docs/callers/cursor-approve.md` (+32 -1) </details> ### 📄 Description ## ELI5 A review panel takes ~45 minutes. If you push during that window, the round ends up judging a commit the PR has already moved past, so it refuses to decide — "the PR head moved while the review ran". That burned a whole panel, and the only way out was for a human to spot the status card and remove/re-add the `cursor-review` label by hand. Until someone did, the PR sat behind a standing request for changes with nothing coming to clear it. Now the round re-runs itself. Once per PR. ## Why not "review every push" That was the obvious fix and it's the wrong one — it makes every push to a labelled PR cost a panel, across every consumer repo. This doesn't react to your push. It reacts to a round that already finished and was wasted. **Panels stay bounded by invalidations, not by pushes** — a healthy PR runs no extra round however often you push to it. The long panel latency actually helps: by the time the round ends and notices, the pushing has usually stopped, so the retry reviews the settled head instead of chasing a moving one. ## When it fires Only where a fresh round can actually decide differently: **the head moved**. That's what makes the next round read different content, so the gate's same-SHA `dup` check can't skip it. - base retarget *alongside* a head move — rides along - retarget alone, errored panel cell, errored judge, unreadable PR — head unchanged, so the re-run would be deduped away; these still ask for a human, exactly as before - structural causes (findings didn't land as threads, empty diff) — unchanged ## The ceiling One per PR, counted off the PR's own reviews rather than a run-local counter — this job is stateless and the retry's round is a different run. Each retry marks its standing block with ``, and a marked block still counts after a later round dismisses it, so the bound holds across rounds and runs. One is deliberate. A second only helps while the author is still pushing, and re-running on every settle is the cost this exists to avoid. ## Fails back, never into a loop - unreadable review list, or an unknown approver login → spends the budget rather than retrying blind - the marked standing block did not post (own PR, POST failure) → no retry, and the card keeps the hand-recovery step, since nothing would count the retry - the round is already at `max_rounds` → no retry (the re-run would cap out and skip its panel) - only an explicit `--can-relabel true` retries; the `GITHUB_TOKEN` fallback passes `false`, because a `GITHUB_TOKEN`-applied label fires no workflow run (the reason `cursor-review-auto-label.yml` exists) - failed relabel → the `auto-retry` job goes red with an `::error::` naming the label; the card already says "if no new round starts, re-run it by hand" ## Where the relabel runs Not in decide. decide sets `auto_retry=true`, and the relabel runs in a new last job, `auto-retry` ("Re-run the round (head moved)"). That job `needs` post-review, panel-integrity, blocking-gate and notify-complete under `!cancelled()`. The `unlabeled`/`labeled` events it fires land in the caller's `cancel-in-progress` group, so if decide relabelled itself it would cancel the run's own Blocking gate and Panel integrity checks. The DELETE URL-quotes the label and tolerates a 404, and the re-add is tried twice. The contract marker stays `relabel`, so an agent reading the card waits for the new round either way — only the prose changes, so nobody re-does by hand what already fired. ## Caller impact None required. Whichever token a caller already configures (`APPROVER_TOKEN`, or the `bot_app_id` App) needs **label write** on the repo; a caller on neither simply never auto-retries. ## Provenance Found while diagnosing why two live PRs were sitting on standing blocks nobody had noticed — in both cases a branch update during the panel. Documented in `docs/callers/cursor-approve.md`. **Verified:** - cursor-review script suite green on the current head (1144 tests), including new coverage for the posted-id gate, round-cap room, fail-closed `--can-relabel`, label quoting, the 404-tolerant DELETE and the retried re-add - cursor-approve script suite green (161 tests); workflow-pins suite green - `check_workflow_pins.py`, `check_agents_md.py --root .` and `check-org-repo-literals.sh` pass **Deviations:** - The relabel moved from decide into a new final `auto-retry` job. Four structural workflow tests were updated for it: the cursor-review guard count went 10→11 and the workflow-pins guarded-site count went 21→22, the bot-key allowlist gained `auto-retry`, and panel-integrity's "gates nothing" test now allows this one ordering-only dependent and pins that it runs under `!cancelled()` and never reads panel-integrity's result ## Tests `python3 -m unittest discover -s .github/cursor-review/tests -p 'test_*.py'` — 1144 pass `python3 -m unittest discover -s .github/cursor-approve/tests -p 'test_*.py'` — 161 pass `python3 -m unittest discover -s .github/workflow-pins/tests -p 'test_*.py'` — pass `python3 .github/workflow-pins/check_workflow_pins.py` — OK `python3 .github/agents-md-integrity/check_agents_md.py --root .` — passed `bash .github/lint/check-org-repo-literals.sh` — OK 🤖 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:05 +00:00