[PR #362] [MERGED] feat(cursor-approve): status card and a standing request-changes on every round outcome (BE-19489) #365

New issue

Closed

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

bsakaguchi commented

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

Owner

📋 Pull Request Information

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

Base: main ← Head: matt/be-19489-status-card-every-outcome

📝 Commits (4)

bacfab6 feat(cursor-approve): status card and a standing request-changes on every round outcome (BE-19489)
3c4914d feat(cursor-approve): block on a withheld axes decision; name the caller's review label (BE-19489)
6705c70 fix(cursor-approve): address panel review on #362 (BE-19489)
5a89473 fix(cursor-approve): leave the standing block when decide's own review is withdrawn or fails (BE-19489)

📊 Changes

10 files changed (+1354 additions, -80 deletions)

View changed files

📝 .github/cursor-approve/card.py (+193 -18)
📝 .github/cursor-approve/tests/test_card.py (+132 -0)
📝 .github/cursor-review/README.md (+1 -1)
📝 .github/cursor-review/auto-approve.py (+469 -25)
📝 .github/cursor-review/tests/test_approve_external.py (+106 -12)
📝 .github/cursor-review/tests/test_auto_approve.py (+341 -14)
📝 .github/workflows/cursor-approve.yml (+7 -3)
📝 .github/workflows/cursor-review.yml (+15 -3)
📝 docs/callers/cursor-approve.md (+71 -1)
📝 docs/callers/cursor-review.md (+19 -3)

📄 Description

ELI-5

When the robot reviewer finished a round without approving a PR, the PR often showed nothing at all. That happened most when the robot couldn't decide, for example because one of its reviewers crashed. Authors, and the agents that drive PRs to merge, couldn't tell whether they had to do anything. Now every round updates one status comment. It says what happened, why, and what to do next. It also has two hidden tags that agents can read. When the robot can't decide, it also leaves a "changes requested" review, so a PR whose threads are all resolved doesn't look approved. The next round that approves removes that review.

What changed

The status card is written on every round where auto-approve applies (approve_max_severity is set and the author passes approve_authors): pass, changes_requested, no_decision and capped. An author approve_authors doesn't list gets no card. The card is still one comment, found by  and edited in place.
Card content
- A state line and the reasons from decide_gate, verbatim.
- For changes_requested, one line per gating finding: severity, file:line, a thread link (#discussion_r<id>, matched from the open-thread read decide already makes) and why it gated. The reason is delta, a live repeat, High/Critical, or full scope.
- The reviewed SHA.
- A next-step line on every card that isn't approved.
- Two machine-readable markers on lines 2 and 3:  and .
- The start and decide phases of cursor-approve also emit the markers. The decide outcomes map onto the same contract.
- The contract is documented as stable in docs/callers/cursor-approve.md#the-status-card-contract.
Next step for no_decision
- A transient cause (a reviewer or the judge errored, the head or base moved, the PR couldn't be read) gives relabel.
- A structural cause (findings didn't land as threads, findings reached the review body only, the reviewed diff is empty) gives human, and the card names the cause.
Standing REQUEST_CHANGES on a no-decision round
- For opted-in authors, a NONE outcome now posts a REQUEST_CHANGES as the approver identity. The body gives the reasons and the next step.
- It carries the usual cursor-review-auto-approve marker, reviewed-SHA marker and base marker.
- A repeated NONE posts the new review first and then dismisses this identity's older marked ones, so reviews don't stack. If the dismissal fails, two block reviews stay, never zero.
- An approving round withdraws it through dismiss_own_change_requests. That covers decide's --defer-approval path (already the case), decide's posted APPROVE (new: an explicit dismissal, not just the supersede) and cursor-approve's approve-external (new).
- capped is unchanged: no new request-changes. An unlisted author gets nothing. Behaviour is the same with defer_approval on or off.
- A NONE caused only by an earlier round's open above-threshold thread is treated the same way. The card state is changes_requested, with those threads linked and next step resolve_then_relabel.
Wiring
- post-review now also needs: round-cap. diff-size already depended on it, so the ordering doesn't change.
- The decide step passes --card true --round --max-rounds --run-url.
- Card writes go through auto-approve.py's gh. They never change the decision, and a failed write is only a warning.
- No workflow inputs changed.
The decide phase blocks when it withholds (defer_approval path):
- approve-external now leaves the same standing REQUEST_CHANGES on not_approved, error, superseded and needs_human. The body has the decide card's reasons (shared through card.decide_reasons) and next step, plus the reviewed-SHA marker.
- Same rule against stacking: post the new review, then dismiss the older ones.
- vetoed, own_pr and an approval post none.
- A superseded decide that finishes after a newer run already approved the live head posts none. As the approver's latest review, it would override that approval.
- This is what keeps a red axis visibly blocking the PR after the passing cursor-review round has dismissed the earlier block.
The caller's label is in the next-step text: the cards and blocks cursor-review writes name its review_label input instead of a hard-coded cursor-review. The value is passed as --review-label "$REVIEW_LABEL". A value that isn't a plain label name is not echoed, and the line says "the review label" instead, so it never names a label the repo may not have. cursor-approve.yml has no review_label input, so the cards its own start and decide phases write, and its decide-phase block, keep the default cursor-review text.
Docs: auto-approve.py docstring, .github/cursor-review/README.md, docs/callers/cursor-review.md and docs/callers/cursor-approve.md.

Design choice (card placement)

cursor-review's decide writes the card. phase: start doesn't run on more outcomes.

Only decide has the data. The gating findings (consolidated.json), the open threads and their comment ids, decide_gate's reasons and the scope's per-finding gating reason all live in the post-review job. Sending them to cursor-approve would mean new structured reusable-workflow outputs. Those are flat strings, so we'd have to serialise and re-validate model output across a trust boundary.
Callers need no new wiring. The documented caller gates approve-start on approve_gate == 'pass' || 'capped'. Running start on more outcomes would mean every caller rewrites its if: and passes more inputs before a single not-approved card appears. Comfy-Org/cloud#12306 is the motivating case: no job after cursor-review ran at all.
It runs on every outcome. decide runs whenever a round is delivered, including when the cursor-approve jobs would be skipped, cancelled or failed. So the card can't go stale behind a skipped job.
It doesn't break the existing flow. On pass, cursor-approve's start phase still overwrites the card with the axes table, and its decide phase writes the verdicts. Both write as the APPROVER_TOKEN identity, which is how they find the same comment. The docs say to pass the same APPROVER_TOKEN to both. The axes still run only on pass.

Testing

Command	Result
`cd .github/cursor-review && python3 -m unittest discover -s tests -q`	1061 tests, OK
`cd .github/cursor-approve && python3 -m unittest discover -s tests -q`	161 tests, OK
`cd .github/workflow-pins && python3 -m unittest discover -s tests -q`	551 tests, OK (43 skipped)
`bash .github/bump-callers/tests/*.sh`	485 / 197 / 445 passed, 0 failed
`actionlint` on `cursor-review.yml` and `cursor-approve.yml`	Only the existing `job.workflow_sha` and SC2018/SC2019 findings. The count is unchanged from `main`.

New tests:

StatusCardAndStandingBlockTest, NoDecisionNextTest and ThreadUrlTest in test_auto_approve.py.
Approve-external cases in test_approve_external.py.
ContractMarkers in test_card.py.

What they cover:

One case per outcome, checking the card text, both markers and the next-step line.
Reasons from decide_gate render verbatim.
An unlisted author gets no review and no card.
A second round edits the same comment.
NONE posts REQUEST_CHANGES with the reason, the next step and the SHA marker.
A repeated NONE replaces the earlier review, and an edited one is left alone.
A later passing round withdraws it, both posted and deferred, and so does approve-external.
capped posts no new review.
A red axis in approve-external posts REQUEST_CHANGES carrying the card's reasons and next step and the SHA marker, a later approval withdraws it, and vetoed and own_pr post none. Model-written axis reasons are sanitized, and a late superseded run doesn't override a newer approval.
review_label is used in the next-step text; an unsafe value is not echoed and the line falls back to "the review label".
Eight existing approve-external tests now expect a block where they used to expect none.
A forged marker can't come in through a reason or a path.
The workflow wiring.

All of the new cases fail when run against origin/main sources: 23 in cursor-review and 17 in cursor-approve. The exceptions are the guards for an unlisted author and for a non-approval keeping its review, which already held. Five existing tests were updated, because they asserted that an untrusted round posts no review.

Not exercised end to end on a live PR. The behaviour is covered by the unit harnesses, which fake gh.

Provenance

Authored by: agent-work loop
Verified:
- cursor-review (1061), cursor-approve (161) and workflow-pins (551, 43 skipped) unit suites pass at the current head.
- check_workflow_pins.py, check_agents_md.py and the org-repo-literal lint pass.
- All PR CI checks green; all 11 review threads (cursor-review panel + CodeRabbit) resolved by the two follow-up commits.
Deviations:
- Not exercised end to end on a live PR; behaviour is covered by unit harnesses that fake gh.
- The known gap (only an approving round withdraws a standing block) is documented in docs/callers/cursor-approve.md and tracked separately.

🤖 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/362 **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-19489-status-card-every-outcome` --- ### 📝 Commits (4) - [`bacfab6`](https://github.com/Comfy-Org/github-workflows/commit/bacfab6701fa6b34fc7415577fee5c8a6e74a469) feat(cursor-approve): status card and a standing request-changes on every round outcome (BE-19489) - [`3c4914d`](https://github.com/Comfy-Org/github-workflows/commit/3c4914dafa89f6b588b91699f6074433007342c6) feat(cursor-approve): block on a withheld axes decision; name the caller's review label (BE-19489) - [`6705c70`](https://github.com/Comfy-Org/github-workflows/commit/6705c70ff0330c9c7634fe4642cf937ddd85c1cc) fix(cursor-approve): address panel review on #362 (BE-19489) - [`5a89473`](https://github.com/Comfy-Org/github-workflows/commit/5a894736b090a1be3b5bd4ef73bff77b3d7568bb) fix(cursor-approve): leave the standing block when decide's own review is withdrawn or fails (BE-19489) ### 📊 Changes **10 files changed** (+1354 additions, -80 deletions) <details> <summary>View changed files</summary> 📝 `.github/cursor-approve/card.py` (+193 -18) 📝 `.github/cursor-approve/tests/test_card.py` (+132 -0) 📝 `.github/cursor-review/README.md` (+1 -1) 📝 `.github/cursor-review/auto-approve.py` (+469 -25) 📝 `.github/cursor-review/tests/test_approve_external.py` (+106 -12) 📝 `.github/cursor-review/tests/test_auto_approve.py` (+341 -14) 📝 `.github/workflows/cursor-approve.yml` (+7 -3) 📝 `.github/workflows/cursor-review.yml` (+15 -3) 📝 `docs/callers/cursor-approve.md` (+71 -1) 📝 `docs/callers/cursor-review.md` (+19 -3) </details> ### 📄 Description ## ELI-5 When the robot reviewer finished a round without approving a PR, the PR often showed nothing at all. That happened most when the robot couldn't decide, for example because one of its reviewers crashed. Authors, and the agents that drive PRs to merge, couldn't tell whether they had to do anything. Now every round updates one status comment. It says what happened, why, and what to do next. It also has two hidden tags that agents can read. When the robot can't decide, it also leaves a "changes requested" review, so a PR whose threads are all resolved doesn't look approved. The next round that approves removes that review. ## What changed - **The status card is written on every round** where auto-approve applies (`approve_max_severity` is set and the author passes `approve_authors`): `pass`, `changes_requested`, `no_decision` and `capped`. An author `approve_authors` doesn't list gets no card. The card is still one comment, found by `` and edited in place. - **Card content** - A state line and the reasons from `decide_gate`, verbatim. - For `changes_requested`, one line per gating finding: severity, `file:line`, a thread link (`#discussion_r<id>`, matched from the open-thread read decide already makes) and why it gated. The reason is delta, a live repeat, High/Critical, or `full` scope. - The reviewed SHA. - A next-step line on every card that isn't approved. - Two machine-readable markers on lines 2 and 3: `` and ``. - The start and decide phases of cursor-approve also emit the markers. The decide outcomes map onto the same contract. - The contract is documented as stable in `docs/callers/cursor-approve.md#the-status-card-contract`. - **Next step for `no_decision`** - A transient cause (a reviewer or the judge errored, the head or base moved, the PR couldn't be read) gives `relabel`. - A structural cause (findings didn't land as threads, findings reached the review body only, the reviewed diff is empty) gives `human`, and the card names the cause. - **Standing REQUEST_CHANGES on a no-decision round** - For opted-in authors, a NONE outcome now posts a REQUEST_CHANGES as the approver identity. The body gives the reasons and the next step. - It carries the usual `cursor-review-auto-approve` marker, reviewed-SHA marker and base marker. - A repeated NONE posts the new review first and then dismisses this identity's older marked ones, so reviews don't stack. If the dismissal fails, two block reviews stay, never zero. - An approving round withdraws it through `dismiss_own_change_requests`. That covers decide's `--defer-approval` path (already the case), decide's posted APPROVE (new: an explicit dismissal, not just the supersede) and cursor-approve's `approve-external` (new). - `capped` is unchanged: no new request-changes. An unlisted author gets nothing. Behaviour is the same with `defer_approval` on or off. - A NONE caused only by an earlier round's open above-threshold thread is treated the same way. The card state is `changes_requested`, with those threads linked and next step `resolve_then_relabel`. - **Wiring** - `post-review` now also `needs: round-cap`. `diff-size` already depended on it, so the ordering doesn't change. - The decide step passes `--card true --round --max-rounds --run-url`. - Card writes go through `auto-approve.py`'s `gh`. They never change the decision, and a failed write is only a warning. - No workflow inputs changed. - **The decide phase blocks when it withholds (defer_approval path):** - `approve-external` now leaves the same standing REQUEST_CHANGES on `not_approved`, `error`, `superseded` and `needs_human`. The body has the decide card's reasons (shared through `card.decide_reasons`) and next step, plus the reviewed-SHA marker. - Same rule against stacking: post the new review, then dismiss the older ones. - `vetoed`, `own_pr` and an approval post none. - A `superseded` decide that finishes after a newer run already approved the live head posts none. As the approver's latest review, it would override that approval. - This is what keeps a red axis visibly blocking the PR after the passing cursor-review round has dismissed the earlier block. - **The caller's label is in the next-step text:** the cards and blocks cursor-review writes name its `review_label` input instead of a hard-coded `cursor-review`. The value is passed as `--review-label "$REVIEW_LABEL"`. A value that isn't a plain label name is not echoed, and the line says "the review label" instead, so it never names a label the repo may not have. `cursor-approve.yml` has no `review_label` input, so the cards its own start and decide phases write, and its decide-phase block, keep the default `cursor-review` text. - **Docs:** `auto-approve.py` docstring, `.github/cursor-review/README.md`, `docs/callers/cursor-review.md` and `docs/callers/cursor-approve.md`. ## Design choice (card placement) **cursor-review's `decide` writes the card. `phase: start` doesn't run on more outcomes.** - **Only decide has the data.** The gating findings (`consolidated.json`), the open threads and their comment ids, `decide_gate`'s reasons and the scope's per-finding gating reason all live in the `post-review` job. Sending them to cursor-approve would mean new structured reusable-workflow outputs. Those are flat strings, so we'd have to serialise and re-validate model output across a trust boundary. - **Callers need no new wiring.** The documented caller gates `approve-start` on `approve_gate == 'pass' || 'capped'`. Running start on more outcomes would mean every caller rewrites its `if:` and passes more inputs before a single not-approved card appears. Comfy-Org/cloud#12306 is the motivating case: no job after cursor-review ran at all. - **It runs on every outcome.** decide runs whenever a round is delivered, including when the cursor-approve jobs would be skipped, cancelled or failed. So the card can't go stale behind a skipped job. - **It doesn't break the existing flow.** On `pass`, cursor-approve's start phase still overwrites the card with the axes table, and its decide phase writes the verdicts. Both write as the `APPROVER_TOKEN` identity, which is how they find the same comment. The docs say to pass the same `APPROVER_TOKEN` to both. The axes still run only on `pass`. ## Testing | Command | Result | |---|---| | `cd .github/cursor-review && python3 -m unittest discover -s tests -q` | 1061 tests, OK | | `cd .github/cursor-approve && python3 -m unittest discover -s tests -q` | 161 tests, OK | | `cd .github/workflow-pins && python3 -m unittest discover -s tests -q` | 551 tests, OK (43 skipped) | | `bash .github/bump-callers/tests/*.sh` | 485 / 197 / 445 passed, 0 failed | | `actionlint` on `cursor-review.yml` and `cursor-approve.yml` | Only the existing `job.workflow_sha` and SC2018/SC2019 findings. The count is unchanged from `main`. | New tests: - `StatusCardAndStandingBlockTest`, `NoDecisionNextTest` and `ThreadUrlTest` in `test_auto_approve.py`. - Approve-external cases in `test_approve_external.py`. - `ContractMarkers` in `test_card.py`. What they cover: - One case per outcome, checking the card text, both markers and the next-step line. - Reasons from `decide_gate` render verbatim. - An unlisted author gets no review and no card. - A second round edits the same comment. - NONE posts REQUEST_CHANGES with the reason, the next step and the SHA marker. - A repeated NONE replaces the earlier review, and an edited one is left alone. - A later passing round withdraws it, both posted and deferred, and so does `approve-external`. - `capped` posts no new review. - A red axis in `approve-external` posts REQUEST_CHANGES carrying the card's reasons and next step and the SHA marker, a later approval withdraws it, and `vetoed` and `own_pr` post none. Model-written axis reasons are sanitized, and a late `superseded` run doesn't override a newer approval. - `review_label` is used in the next-step text; an unsafe value is not echoed and the line falls back to "the review label". - Eight existing approve-external tests now expect a block where they used to expect none. - A forged marker can't come in through a reason or a path. - The workflow wiring. All of the new cases fail when run against `origin/main` sources: 23 in cursor-review and 17 in cursor-approve. The exceptions are the guards for an unlisted author and for a non-approval keeping its review, which already held. Five existing tests were updated, because they asserted that an untrusted round posts no review. Not exercised end to end on a live PR. The behaviour is covered by the unit harnesses, which fake `gh`. ## Provenance - **Authored by:** agent-work loop - **Verified:** - cursor-review (1061), cursor-approve (161) and workflow-pins (551, 43 skipped) unit suites pass at the current head. - `check_workflow_pins.py`, `check_agents_md.py` and the org-repo-literal lint pass. - All PR CI checks green; all 11 review threads (cursor-review panel + CodeRabbit) resolved by the two follow-up commits. - **Deviations:** - Not exercised end to end on a live PR; behaviour is covered by unit harnesses that fake `gh`. - The known gap (only an approving round withdraws a standing block) is documented in `docs/callers/cursor-approve.md` and tracked separately. 🤖 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:04 +00:00