[PR #389] [MERGED] fix(cursor-approve): keep a red panel cell from skipping the axes #388

New issue

Closed

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

bsakaguchi commented

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

Owner

📋 Pull Request Information

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

Base: main ← Head: matt/cursor-approve-axes-skip-on-red-panel

📝 Commits (2)

5e12cae fix(cursor-approve): keep a red panel cell from skipping the axes
b3db43e fix(cursor-approve): pin the caller guards as exact conjuncts, and say what !cancelled() gives up

📊 Changes

5 files changed (+347 additions, -18 deletions)

View changed files

➕ .github/cursor-approve/tests/test_caller_doc_pattern.py (+227 -0)
📝 .github/workflows/cursor-review.yml (+23 -4)
📝 .github/workflows/test-cursor-review-scripts.yml (+19 -4)
📝 docs/callers/cursor-approve.md (+74 -9)
📝 docs/callers/cursor-review.md (+4 -1)

📄 Description

What this fixes

cursor-review's called job goes red whenever a panel cell errors — the leg fails by design ("Fail the leg when the cell did not submit") and Panel integrity fails alongside it. approve_max_failed_reviewers does not change that: it governs the approve_gate verdict, not the job's conclusion.

The caller pattern this repo publishes chained approve-start and every axis off that job with a bare needs:. So GitHub's default rule skipped them on the red job, the axes never ran, and cursor-approve's decide — which runs on always() — rendered every axis as ⚠️ no result and withheld the approval.

A caller that sets approve_max_failed_reviewers: 1 precisely to tolerate one dead cell therefore never got the tolerance it configured. One errored cell out of six withheld every round, and nothing on the card said why.

Observed

A consumer repo's PR (a private caller, so not linked here):

cursor-review / edge-case (kimi-k3-high)   failure   ← RetriableError: [resource_exhausted]
cursor-review / Panel integrity            failure   ← 3 of 4 reviewer cells submitted
approve-start                              skipped
axis-correctness                           skipped
axis-conformance                           skipped
cursor-approve (decide)                    success   ← always(), so it ran with nothing to read

The cell died on Cursor-side capacity after three reconnects. It had already written two medium findings to findings.json, but never reached the MCP submit tool, so status stayed error and the leg went red.

approve_gate was pass the whole time — decide's own if: requires it, and decide ran — so the gate had already tolerated the cell exactly as the caller asked. Only the needs: propagation one job earlier threw that away.

Across the 81 preceding completed panel runs on that consumer, approve-start had never once run.

The change

docs/callers/cursor-approve.md — !cancelled() on the start phase and all five axes. !cancelled() rather than always() so a superseded run never goes on to decide.
The axes also restate needs.approve-start.result == 'success', which !cancelled() would otherwise drop. The documented invariant is unchanged: a start phase that failed to withdraw cursor-review's standing approval still skips them, and decide reads that as "no result".
A new ### A red panel cell must not skip the axes section explains why the guard is load-bearing rather than stylistic.
docs/callers/cursor-review.md's next: approve_gate example gets the same !cancelled() guard.
.github/cursor-approve/tests/test_caller_doc_pattern.py pins the guards as exact top-level conjuncts, including the exact approve_gate == 'pass'/'capped' comparison. A top-level ||, a != 'pass', a bare truthy gate, and a stale second copy of a job all fail. It covers every occurrence of each job across all yaml blocks, and cursor-review.md's next: example too.
Both caller docs join test-cursor-review-scripts.yml's path filter, so a docs-only revert runs the test.
Two comments corrected: Panel integrity described itself as "advisory — red here fails no other job", which is true of check-runs and of this workflow but not of a caller's needs:; and approve_max_failed_reviewers promised a round "decided normally" without saying what the caller must do for that to hold (now including that !cancelled() drops the implicit success requirement on other needs:, which must be restated). Step 1 of cursor-approve.md's "What it does" no longer says a failed run skips the start phase.

What this does and does not change about approval

approve_gate remains the only authority on whether a round may be decided, and it already fails closed to untrusted on a degraded judge, more errored cells than the caller tolerates, an undelivered review, a moved head, or any upstream decision job that did not succeed. That covers every Panel integrity failure that is not simply a short panel. Reading the gate off a red cursor-review job cannot approve a round the gate itself rejected.

It does give up one incidental backstop, and the doc now says so. The gate trusts each cell's self-reported status. Artifact names are run-global, so a prompt-injected cell could upload a forged clean record under another cell's name: that cell's leg goes red while consolidate reads the forgery. A bare needs: used to skip the axes on that red leg. Now the axes still review the change independently, and decide still withholds on any red axis. Closing the gap at the source (consolidate cross-checking each leg's conclusion) is a runtime change to cursor-review, so it is deferred to a follow-up.

Dropping Panel integrity instead would not have helped: a red matrix leg fails the caller's job on its own.

Scope

No runtime behaviour in this repo changes — the workflow edits are comments. The fix each caller needs is the if: guards.

Follow-up, not in this PR: the one consumer running the axes today needs the same guards applied to its approve-start / axis-correctness / axis-conformance jobs (this repo's own ci-cursor-review.yml does not run them).

Separately worth noting: kimi-k3-high resource_exhausted is recurring, not a one-off — four other recent consumer panels went red on kimi cells, twice with both legs.

Tests

python3 -m unittest discover -s .github/cursor-review/tests -p 'test_*.py'    # 1172 OK
python3 -m unittest discover -s .github/cursor-approve/tests -p 'test_*.py'   #  174 OK
python3 -m unittest discover -s .github/workflow-pins/tests -p 'test_*.py'    #  551 OK
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

Provenance

Authored by: agent-work loop
Verified:

cursor-review / cursor-approve / workflow-pins unittest suites pass locally at b3db43e (counts above). Pin lint, AGENTS.md integrity and the org-repo-literal lint pass.
test_caller_doc_pattern.py was mutation-checked on b3db43e. Each of these fails the suite: || for &&, != 'pass', the start phase's parentheses dropped, and a stale bare if: on a second copy of a job.
PR CI on b3db43e: every GitHub Actions and Socket check passed, and CodeRabbit completed with no actionable comments. The cursor-review panel last ran on 5e12cae, and all six of its findings are answered on b3db43e. Every review thread is resolved.
A local merge of b3db43e with main c37492c is conflict-free, and the cursor-review (1210) and cursor-approve (174) suites, the pin lint and the AGENTS.md integrity check pass on that merged tree.
Deviations:
Body redacted to drop a private consumer repo's name, PR and workflow path (public repo). The first commit message (5e12cae) still names it, and this repo's squash message is built from the commit messages, so whoever merges should delete that reference from the squash message (it is not rewritten here to avoid force-pushing the branch).
A review finding (a forged cell artifact behind a red leg) is answered with a doc caveat here plus a deferred follow-up, not a runtime fix.

🤖 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/389 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 10/9/2026 **Status:** ✅ Merged **Merged:** 10/9/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/cursor-approve-axes-skip-on-red-panel` --- ### 📝 Commits (2) - [`5e12cae`](https://github.com/Comfy-Org/github-workflows/commit/5e12caed8c0e28ed67d0186afd0dbfefdcc31bba) fix(cursor-approve): keep a red panel cell from skipping the axes - [`b3db43e`](https://github.com/Comfy-Org/github-workflows/commit/b3db43e878218348960d97e851b72ba55a490c8c) fix(cursor-approve): pin the caller guards as exact conjuncts, and say what `!cancelled()` gives up ### 📊 Changes **5 files changed** (+347 additions, -18 deletions) <details> <summary>View changed files</summary> ➕ `.github/cursor-approve/tests/test_caller_doc_pattern.py` (+227 -0) 📝 `.github/workflows/cursor-review.yml` (+23 -4) 📝 `.github/workflows/test-cursor-review-scripts.yml` (+19 -4) 📝 `docs/callers/cursor-approve.md` (+74 -9) 📝 `docs/callers/cursor-review.md` (+4 -1) </details> ### 📄 Description ## What this fixes cursor-review's called job goes **red whenever a panel cell errors** — the leg fails by design ("Fail the leg when the cell did not submit") and `Panel integrity` fails alongside it. `approve_max_failed_reviewers` does not change that: it governs the `approve_gate` **verdict**, not the job's **conclusion**. The caller pattern this repo publishes chained `approve-start` and every axis off that job with a bare `needs:`. So GitHub's default rule skipped them on the red job, the axes never ran, and `cursor-approve`'s decide — which runs on `always()` — rendered every axis as `⚠️ no result` and withheld the approval. A caller that sets `approve_max_failed_reviewers: 1` precisely to tolerate one dead cell therefore never got the tolerance it configured. One errored cell out of six withheld every round, and nothing on the card said why. ## Observed A consumer repo's PR (a private caller, so not linked here): ``` cursor-review / edge-case (kimi-k3-high) failure ← RetriableError: [resource_exhausted] cursor-review / Panel integrity failure ← 3 of 4 reviewer cells submitted approve-start skipped axis-correctness skipped axis-conformance skipped cursor-approve (decide) success ← always(), so it ran with nothing to read ``` The cell died on Cursor-side capacity after three reconnects. It had already written two medium findings to `findings.json`, but never reached the MCP submit tool, so status stayed `error` and the leg went red. `approve_gate` was `pass` the whole time — decide's own `if:` requires it, and decide ran — so the gate had already tolerated the cell exactly as the caller asked. Only the `needs:` propagation one job earlier threw that away. Across the 81 preceding completed panel runs on that consumer, `approve-start` had **never once run**. ## The change - `docs/callers/cursor-approve.md` — `!cancelled()` on the start phase and all five axes. `!cancelled()` rather than `always()` so a superseded run never goes on to decide. - The axes also restate `needs.approve-start.result == 'success'`, which `!cancelled()` would otherwise drop. The documented invariant is unchanged: a start phase that failed to withdraw cursor-review's standing approval still skips them, and decide reads that as "no result". - A new `### A red panel cell must not skip the axes` section explains why the guard is load-bearing rather than stylistic. - `docs/callers/cursor-review.md`'s `next:` approve_gate example gets the same `!cancelled()` guard. - `.github/cursor-approve/tests/test_caller_doc_pattern.py` pins the guards as **exact top-level conjuncts**, including the exact `approve_gate == 'pass'`/`'capped'` comparison. A top-level `||`, a `!= 'pass'`, a bare truthy gate, and a stale second copy of a job all fail. It covers every occurrence of each job across all yaml blocks, and cursor-review.md's `next:` example too. - Both caller docs join `test-cursor-review-scripts.yml`'s path filter, so a docs-only revert runs the test. - Two comments corrected: `Panel integrity` described itself as "advisory — red here fails no other job", which is true of check-runs and of this workflow but not of a caller's `needs:`; and `approve_max_failed_reviewers` promised a round "decided normally" without saying what the caller must do for that to hold (now including that `!cancelled()` drops the implicit success requirement on other `needs:`, which must be restated). Step 1 of cursor-approve.md's "What it does" no longer says a failed run skips the start phase. ## What this does and does not change about approval `approve_gate` remains the only authority on whether a round may be decided, and it already fails closed to `untrusted` on a degraded judge, more errored cells than the caller tolerates, an undelivered review, a moved head, or **any upstream decision job that did not succeed**. That covers every `Panel integrity` failure that is not simply a short panel. Reading the gate off a red cursor-review job cannot approve a round the gate itself rejected. It does give up one *incidental* backstop, and the doc now says so. The gate trusts each cell's self-reported `status`. Artifact names are run-global, so a prompt-injected cell could upload a forged clean record under another cell's name: that cell's leg goes red while consolidate reads the forgery. A bare `needs:` used to skip the axes on that red leg. Now the axes still review the change independently, and decide still withholds on any red axis. Closing the gap at the source (consolidate cross-checking each leg's conclusion) is a runtime change to cursor-review, so it is deferred to a follow-up. Dropping `Panel integrity` instead would not have helped: a red matrix leg fails the caller's job on its own. ## Scope No runtime behaviour in this repo changes — the workflow edits are comments. The fix each caller needs is the `if:` guards. **Follow-up, not in this PR:** the one consumer running the axes today needs the same guards applied to its `approve-start` / `axis-correctness` / `axis-conformance` jobs (this repo's own `ci-cursor-review.yml` does not run them). Separately worth noting: `kimi-k3-high` `resource_exhausted` is recurring, not a one-off — four other recent consumer panels went red on kimi cells, twice with both legs. ## Tests ``` python3 -m unittest discover -s .github/cursor-review/tests -p 'test_*.py' # 1172 OK python3 -m unittest discover -s .github/cursor-approve/tests -p 'test_*.py' # 174 OK python3 -m unittest discover -s .github/workflow-pins/tests -p 'test_*.py' # 551 OK 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 ``` ## Provenance **Authored by:** agent-work loop **Verified:** - cursor-review / cursor-approve / workflow-pins unittest suites pass locally at `b3db43e` (counts above). Pin lint, AGENTS.md integrity and the org-repo-literal lint pass. - `test_caller_doc_pattern.py` was mutation-checked on `b3db43e`. Each of these fails the suite: `||` for `&&`, `!= 'pass'`, the start phase's parentheses dropped, and a stale bare `if:` on a second copy of a job. - PR CI on `b3db43e`: every GitHub Actions and Socket check passed, and CodeRabbit completed with no actionable comments. The cursor-review panel last ran on `5e12cae`, and all six of its findings are answered on `b3db43e`. Every review thread is resolved. - A local merge of `b3db43e` with main `c37492c` is conflict-free, and the cursor-review (1210) and cursor-approve (174) suites, the pin lint and the AGENTS.md integrity check pass on that merged tree. **Deviations:** - Body redacted to drop a private consumer repo's name, PR and workflow path (public repo). The first commit message (`5e12cae`) still names it, and this repo's squash message is built from the commit messages, so **whoever merges should delete that reference from the squash message** (it is not rewritten here to avoid force-pushing the branch). - A review finding (a forged cell artifact behind a red leg) is answered with a doc caveat here plus a deferred follow-up, not a runtime fix. 🤖 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:17 +00:00