[PR #366] [MERGED] fix(cursor-approve): stop advising a relabel that does nothing, and state the decision once (BE-19527) #368

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/366
Author: @mattmillerai
Created: 10/7/2026
Status: ✅ Merged
Merged: 10/7/2026
Merged by: @mattmillerai

Base: main ← Head: matt/be-cursor-approve-honest-remedy

📝 Commits (1)

5fb09d8 fix(cursor-approve): stop advising a relabel that does nothing, and state the decision once (BE-19527)

📊 Changes

5 files changed (+89 additions, -25 deletions)

View changed files

📝 .github/cursor-approve/card.py (+14 -1)
📝 .github/cursor-review/auto-approve.py (+22 -8)
📝 .github/cursor-review/tests/test_approve_external.py (+2 -1)
📝 .github/cursor-review/tests/test_auto_approve.py (+32 -12)
📝 docs/callers/cursor-approve.md (+19 -3)

📄 Description

Stacked on #365 — based on matt/be-cursor-approve-auto-retry, retarget to main once that merges. It reuses the auto_retry_eligible / RETRYABLE_CAUSES split #365 introduces.

ELI5

Two things the status card and the standing block got wrong, both visible in the same pair of comments on a PR.

1. The remedy was often a no-op

The gate's dup step skips any head that already carries a bot-posted consolidated review — and a degraded round posts one too. So for a round the head never moved under, "remove and re-add the cursor-review label" starts a run that no-ops, and the PR stays exactly where it was. The card has been printing advice that silently does nothing, which is worse than printing none.

That's every transient cause except a moved head:

cause	head moved?	relabel alone works?
the head moved	yes	yes — new content, dedupe can't skip it
a panel cell errored	no	no
the judge errored	no	no
the PR could not be read	no	no
the base alone was retargeted	no	no

Those now report dismiss_then_relabel and name the dup step's own documented escape hatch: dismiss that consolidated review first, then re-run.

A new contract value rather than new prose under relabel, because an agent reading the marker and following relabel there would start nothing either — the bug exists at the machine level too.

2. The decision was rendered twice

The review body repeated the scope note and the threshold that the card already carries. The card's scope note is the richer of the two (it has a per-finding "why it gated" the body never had), and the headline already names the threshold. Both lines come off both review bodies.

What stays is what a reader who only gets the email notification needs: the verdict and the findings that drove it. render_external_body — the axes path — has always been a headline plus a card link; this is the same trade, one step less far.

Caller impact

dismiss_then_relabel is additive to card.NEXTS. Anything switching exhaustively on the contract marker should learn it; anything matching known values and falling through is unaffected.

Provenance

Both found reading the pair of comments on a live PR and asking why they say the same thing twice — and then why the next step they both printed wouldn't have worked.

Tests

python3 -m unittest discover -s .github/cursor-review/tests -p 'test_*.py' — 1097 pass
python3 -m unittest discover -s .github/cursor-approve/tests -p 'test_*.py' — 161 pass
python3 .github/workflow-pins/check_workflow_pins.py — OK
python3 .github/agents-md-integrity/check_agents_md.py --root . — passed

🤖 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/366 **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-honest-remedy` --- ### 📝 Commits (1) - [`5fb09d8`](https://github.com/Comfy-Org/github-workflows/commit/5fb09d808fc7de854ab5d13efd9ac9d0a0a03eb7) fix(cursor-approve): stop advising a relabel that does nothing, and state the decision once (BE-19527) ### 📊 Changes **5 files changed** (+89 additions, -25 deletions) <details> <summary>View changed files</summary> 📝 `.github/cursor-approve/card.py` (+14 -1) 📝 `.github/cursor-review/auto-approve.py` (+22 -8) 📝 `.github/cursor-review/tests/test_approve_external.py` (+2 -1) 📝 `.github/cursor-review/tests/test_auto_approve.py` (+32 -12) 📝 `docs/callers/cursor-approve.md` (+19 -3) </details> ### 📄 Description > Stacked on #365 — based on `matt/be-cursor-approve-auto-retry`, retarget to `main` once that merges. It reuses the `auto_retry_eligible` / `RETRYABLE_CAUSES` split #365 introduces. ## ELI5 Two things the status card and the standing block got wrong, both visible in the same pair of comments on a PR. ### 1. The remedy was often a no-op The gate's `dup` step skips any head that already carries a bot-posted consolidated review — and a *degraded* round posts one too. So for a round the head never moved under, "remove and re-add the `cursor-review` label" starts a run that no-ops, and the PR stays exactly where it was. The card has been printing advice that silently does nothing, which is worse than printing none. That's every transient cause except a moved head: | cause | head moved? | relabel alone works? | |---|---|---| | the head moved | yes | yes — new content, dedupe can't skip it | | a panel cell errored | no | **no** | | the judge errored | no | **no** | | the PR could not be read | no | **no** | | the base alone was retargeted | no | **no** | Those now report **`dismiss_then_relabel`** and name the dup step's own documented escape hatch: dismiss that consolidated review first, then re-run. A new contract value rather than new prose under `relabel`, because an agent reading the marker and following `relabel` there would start nothing either — the bug exists at the machine level too. ### 2. The decision was rendered twice The review body repeated the scope note and the threshold that the card already carries. The card's scope note is the richer of the two (it has a per-finding "why it gated" the body never had), and the headline already names the threshold. Both lines come off both review bodies. What stays is what a reader who only gets the email notification needs: the verdict and the findings that drove it. `render_external_body` — the axes path — has always been a headline plus a card link; this is the same trade, one step less far. ## Caller impact `dismiss_then_relabel` is additive to `card.NEXTS`. Anything switching exhaustively on the contract marker should learn it; anything matching known values and falling through is unaffected. ## Provenance Both found reading the pair of comments on a live PR and asking why they say the same thing twice — and then why the next step they both printed wouldn't have worked. ## Tests `python3 -m unittest discover -s .github/cursor-review/tests -p 'test_*.py'` — 1097 pass `python3 -m unittest discover -s .github/cursor-approve/tests -p 'test_*.py'` — 161 pass `python3 .github/workflow-pins/check_workflow_pins.py` — OK `python3 .github/agents-md-integrity/check_agents_md.py --root .` — passed 🤖 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