[PR #320] [MERGED] feat(cursor-review): make panel integrity a check-run conclusion #324

New issue

Closed

opened 2026-10-26 22:38:30 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-26 22:38:30 +00:00

Owner

📋 Pull Request Information

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

Base: main ← Head: matt/be-15559-panel-integrity-check

📝 Commits (10+)

4c6e90e feat(cursor-review): make panel integrity a check-run conclusion
1ad01ef fix(cursor-review): fail Panel integrity closed when the decision jobs did not succeed
e4e287d fix(cursor-review): fail Panel integrity closed on a dead preflight and on absent producer results
10090cb Merge remote-tracking branch 'origin/main' into matt/be-15559-panel-integrity-check
2bf8e31 fix(cursor-review): fail Panel integrity closed on a dead ledger too
73557d7 docs(cursor-review): name the Panel integrity bullets instead of counting them
5d8d430 Merge remote-tracking branch 'origin/main' into matt/be-15559-panel-integrity-check
3d0b477 docs(cursor-review): caveat that a later no-panel run supersedes a red Panel integrity
4b6c9a8 test(cursor-review): pin fail-fast: false so a red leg cannot cancel the panel
99be8b7 Merge remote-tracking branch 'origin/main' into matt/be-15559-panel-integrity-check

📊 Changes

5 files changed (+1453 additions, -5 deletions)

View changed files

📝 .github/cursor-review/README.md (+75 -1)
➕ .github/cursor-review/tests/test_workflow_panel_integrity.py (+875 -0)
📝 .github/workflows/cursor-review.yml (+363 -3)
📝 README.md (+1 -1)
📝 docs/callers/cursor-review.md (+139 -0)

📄 Description

ELI-5

When the review panel runs, six AI reviewers each look at your PR and one judge writes up what they found. If one of those six gets stuck and never answers, its check still turned green — so anything reading the PR's check rollup saw a complete panel when it was really a third of a panel. This PR makes that visible: a reviewer that didn't answer turns its own check red, and one new Panel integrity check says whether the panel as a whole was whole. The review still posts either way — nothing new blocks a merge unless a repo chooses to require the new check.

What changed

1. A leg that didn't submit is red. The review job gets a final step, Fail the leg when the cell did not submit, which reads /tmp/findings-out/findings.json back and exits 1 when its status is not ok. It sits after Upload findings artifact on purpose: the artifact is already published, consolidate and notify-complete gate on needs.review.result != 'skipped' (a failed matrix is not a skipped one) and post-review gates on consolidate, so the consolidated review still posts. The only thing that changes is that the gap reaches the rollup. The Run cursor review comment block is updated to say the step cap avoids a cancelled run, and that the leg is now deliberately failed when the cell did not submit.

2. A panel-level Panel integrity context. consolidate now exposes ok_count, total (from the aggregate step), degraded and judge_status as job outputs. A new panel-integrity job — needs: [gate, round-cap, diff-size, preflight, ledger, review, consolidate, post-review] (preflight and ledger joined in the second and third review rounds, round-cap when #345's max_rounds cap landed on main — see Judgment calls), always() plus the same four gate conditions consolidate uses (nested under an upstream-failure disjunct, so a failed decision job is red rather than skipped), permissions: {}, timeout-minutes: 5 — emits one ::error:: per cause and exits 1 when any of: fewer cells submitted than ran; ungated_findings is non-empty and greater than zero; delivered != 'true'; degraded == 'true'. A false incremental_subset is annotated as a ::warning:: and is deliberately not a cause — see Judgment calls. Otherwise it prints one ::notice::Panel integrity: <ok>/<total> cells, <n> anchored finding(s), 0 unanchored. and exits 0. It gates no other job — a short panel must not also cost the PR the findings it did produce.

3. Caller-facing docs. A ## Panel integrity section in the setup guide next to the Blocking gate (what red means, that it is the context an automated merge gate should read, and the two-switch required-check shape), plus a job/check-run table and a matching section in the directory README. Both surfaces scope the claim to "was the panel whole?" and state plainly that the deliberate no-panel branches stay skipped, that a required skipped check passes, and that over-cap in particular wants the Blocking gate. The root README.md catalog row is updated too (0ca9800): it enumerated the check surface as Blocking-gate-only, so after this branch it was wrong by omission on the two things an adopter most needs — that there is now a second, always-on Panel integrity context and that it, not a panel leg, is the stable one to require; and that the caller job goes red on ~40% of runs. The skip semantics are stated in the same breath; forgeability and same-SHA supersession are pointed at rather than restated, so the catalog cannot drift from the guide on the details.

No caller change is required; the shared caller bumper propagates the workflow SHA. No Skip-caller-bump trailer — this is a behavioral change and every fleet caller should bump.

Judgment calls

The leg check reads the artifact defensively rather than as a one-liner. That cell's own agent runs --trust with shell access in the same job, so findings.json is untrusted bytes and its status lands in a ::workflow command:: line. A missing or unparseable file now reads as "did not submit" and produces the proper ::error:: instead of a traceback, and the value is sanitized by replacing disallowed characters rather than dropping them — deleting them would let a crafted "o k" collapse into the one value that passes. Panel integrity flattens every value it interpolates for the same reason (a newline forges a second workflow command) and, since bec0638, percent-escapes them too — the runner unescapes %0A back into a newline, so stripping real newlines alone left JUDGE_STATUS (an unvalidated string from the judge agent's own tool) able to forge a command without containing one. Both are covered by tests.
always(), not !cancelled(), on panel-integrity. GitHub counts a skipped required check as passing, so green-on-cancel would be a fail-open for any repo that requires this — the same reasoning the Blocking gate documents at its own if:. A cancelled run therefore reports red; under the documented cancel-in-progress caller that red lands on the superseded head SHA, not the new one. Documented.
The caller job now goes red on a failing leg, on a measured ~40% of runs. A reusable's caller job takes the aggregate conclusion of the jobs inside it, so this is unavoidable given (1) — and it is the point, since the rollup is the surface the whole ticket is about. Called out explicitly in the setup guide, with the instruction to require Panel integrity rather than the caller job.
A failed Gate, Diff size check or Preflight fails Panel integrity, it does not skip it. ~~Originally this skipped, on the reasoning that the Gate context is itself red so the rollup still carries the signal.~~ That was a fail-open, caught in review: all four gate conditions read job outputs, an output is empty when the job producing it failed, so the check skipped on exactly those runs — and GitHub counts a skipped required check as passing, which would have handed a green merge gate to any caller that took this PR's own advice and required it. The job if: now also runs on needs.gate.result != 'success' and on a fresh in-scope review whose diff-size did not succeed, and a first step Fail if the panel decision itself did not complete turns that into red — the same fail-closed shape as Blocking gate's first two guards. Guard first, so the report step never evaluates its causes off empty outputs. A second review round found that fix had closed only two of the three decision jobs: review declares needs: preflight, so a failed preflight skips the whole matrix and leaves needs.review.result == 'skipped' — indistinguishable from the deliberate no-panel branches — and the check skipped over a run where not one cell started. preflight now joins needs: and the same guard covers it. A third round found the same fail-open one dependency further over: the matrix declares needs: [gate, diff-size, preflight, ledger], and ledger was the one job of the four still unguarded — a red ledger skips the matrix, review reports skipped, and the check skipped into a green required check. ledger now joins needs:, the job if: and the guard as well. It reads like it cannot fail (every step in it is continue-on-error, precisely so it never skips the matrix) but that makes it rare, not unreachable: Ensure ledger artifact exists carries no continue-on-error, its 20-minute job cap sits above only 15 minutes of step caps, and a cancellation or lost runner is not a step outcome at all. The pin is now the RELATIONSHIP — a test reads review's needs: and requires every entry to appear in all three places — because this hole was re-opened twice by a job being added to the matrix and not to the check. The deliberate skips are untouched: they are the branches where gate and diff-size both succeeded and said no panel was warranted. Over-cap stays a skip on purpose and is routed to the Blocking gate, whose own second guard I verified fails closed on exactly that path. round-cap (#345, merged from main) is the one decision job the matrix does not need directly — it sits upstream of diff-size, whose own if: skips on capped == 'true' — so it is read on both sides: a non-success round-cap joins the guard and goes red, and a successful capped head is a deliberate skip like over-cap (the Blocking gate already fails closed on it). Before that, a capped head went red telling the caller to re-run a job that would just hit the cap again; the tests now evaluate the job and guard if: expressions over the capped / uncapped / max_rounds-off / failed-cap / unlabelled / over-cap cases rather than substring-matching them.
Every cause is paired with its producing job's result, not just its outputs. An output is the empty string when the job producing it failed, and every cause treated empty as a pass. Executing the pre-fix run: block with delivered=true (which post-review.py writes the moment the POST returns, before the rest of that job runs) and a failed post-review exits 0 and prints Panel integrity: 6/6 cells — a green panel signal over a red delivery job. ok_count/total are sharper still: both empty on a dead consolidate, and unset-vs-unset compares equal. The step now reads needs.consolidate.result and needs.post-review.result (the env names blocking-gate and notify-complete already use) and rejects an empty count explicitly instead of comparing it.
The leg check is scoped as an availability signal, not an attestation. The "did this cell submit" verdict is the status the cell's own --trust agent wrote, so a prompt-injected cell can green itself and the panel count with it. That limit is now stated in the workflow comment, the caller guide and the README, along with what would fix it (recording the submission outside the cell's own writable job). It catches the stalls, crashes and step caps that are what every observed failure has been; it is not proof against an adversarial PR, and the docs no longer read as though it were.
A discarded incremental block warns; it is deliberately not a cause. This started as a failing cause, written while diff-size declared no such output. Merging #321 (84db66a) both published the output and inverted what a false means, without touching a line of this job. #321 made the subset check fail closed: a block that does not verify is discarded whole and the panel reviews the full reviewed diff alone. So the cells were never "prioritized onto hunks that were not in scope" as the ::error:: claimed — they got no prioritization hint at all, which is byte-identical input to a run that never built a block (round 1, and every "could not build one" branch), and those report true. Reddening the one context callers are told to mark required, over a run whose reviewed content is indistinguishable from a normal one and where nothing was skipped and no finding suppressed, is a merge-blocking false positive — and it is the one cause that never indicated a short panel, which is the only question this check answers. It now emits a ::warning:: naming what actually happened and pointing at the Incremental diff discarded warning on Diff size check for the counts. A non-subset block is still a real builder defect worth seeing; it is not a panel-integrity failure. The other five causes are untouched.
A new sibling test file rather than edits to test_workflow_job_isolation.py. That suite pins the credential split; this one pins the integrity signals. Same house style: stdlib only, no PyYAML, comments stripped before every assertion so a comment naming a step can never satisfy it.

Capability check

The diff adds exit 1 / ::error:: paths, which is the shape that usually means a capability is being denied — it is not one here, and I verified that rather than asserting it. Nothing the workflow could do before is refused: the new check is advisory, gates no job, and the consolidated review still posts over a short panel. I confirmed that by reading every downstream if: — consolidate and notify-complete key on needs.review.result != 'skipped', post-review on needs.consolidate.result == 'success', blocking-gate does not needs: the panel at all — and a repo-wide grep for needs.review returns only those three sites, so no path keys on the panel having succeeded. A test asserts no job needs: panel-integrity.

Provenance

Authored by: agent-work loop
Verified: at head 432fa41 (merge of origin/main cfca794, #355–#363 — no textual conflict; those touch auto-approve/approve-external, post-review's needs: and diff-size's new incremental_state output, none of which Panel integrity reads; incremental_subset, which it does read, is still emitted unchanged):
- cursor-review (1119 tests), cursor-approve (161) and workflow-pins (551, 43 skipped) unittest suites pass on that tree, including test_workflow_panel_integrity.py.
- check_workflow_pins.py OK (167 uses: refs SHA-pinned); check_agents_md.py --root . passes with 2 pre-existing warnings; check-org-repo-literals.sh OK.
- Job graph pinned by tests: panel-integrity's needs: is a superset of review's decision jobs plus round-cap, every output it reads is declared upstream, nothing needs: it, permissions: {} intact; the if: truth-table tests cover capped / uncapped / failed-cap / over-cap cases.
- PR CI on the previous head 55ede56: all 8 checks pass; CI on 432fa41 pending at time of writing; no merge_group run exists for this PR.
- Not done: no live panel run (see Residual).
Deviations:
- Three fail-opens fixed in review: failed decision jobs (gate, diff-size, preflight, ledger) now fail the check rather than skip it, pinned as a relationship to review's needs:; causes are paired with producing-job results, not outputs alone.
- % → %25 escape added to flatten() so agent-written JUDGE_STATUS cannot inject workflow commands (bec0638).
- incremental_subset == 'false' demoted from a failing cause to a ::warning:: after #321 made a discarded block mean "panel read the full diff" (76aa262) — a deviation from the plan, whose premise no longer holds.
- Docs: setup guide, directory README and root README.md catalog row updated (3d0b477, 0ca9800), including the same-head-SHA supersession caveat.
- Semantic conflict with #345 fixed (8c786ef): round-cap added as a decision job, and a max_rounds-capped head now skips the check instead of going red.
- Answered but not actioned, threads left open on purpose for the merger: staging the per-leg exit 1, and same-head-SHA supersession (design change, filed as a follow-up).
Deviation detail (carried forward)

none against the plan as specified. Three corrections after review, all fail-opens the plan did not anticipate: (1) "a failed Gate skips this check" became fail-closed, extended in the second round to preflight, the third decision job — the matrix needs: it, so its failure was reaching the check as an ordinary skipped review; (2) causes read job outputs only, so a producer that died left a clean-looking report; (3) the round-2 fix named preflight specifically rather than the invariant behind it, leaving ledger — the fourth job the matrix needs: — still able to skip the check into a green required check. That one is now pinned as a relationship between review's needs: and this job's, so the class is closed rather than the third instance of it. The docs asserting the superseded behaviour were corrected in the same commits. A fourth correction landed in bec0638, and it is a reporting-integrity hole rather than a fail-open in the gate: flatten() stripped real newlines before interpolating untrusted values into ::error:: lines, but the runner unescapes %0A in a command's message, so a judge writing its status as error%0A::stop-commands::tok reaches the annotation with no newline in it and the runner supplies one. JUDGE_STATUS is the one reachable value (unvalidated agent text; the counts are python-formatted integers and the rest are job results). It cannot forge the check's conclusion, which is driven by the causes counter and the exit code — and it is strictly narrower than the forgeability limit already documented — but it was the one annotating surface in this repo not applying the % -> %25 escape that check_agents_md.py, check_workflow_pins.py, check_public_repo_hygiene.py, check_coderabbit_config.py and check-org-repo-literals.sh all do. Two findings are answered but not actioned, and both threads are left open on purpose: staging the per-leg exit 1 behind an input, and a fourth fail-open found during the reviews pass — a no-panel run on the same head SHA supersedes a red Panel integrity with a skip. The second is a design change, not a patch, and is filed as a follow-up; its documentation half did land, in 3d0b477, because the setup guide was not merely silent on that hole — its "Re-run failed jobs" paragraph actively recommended the whole-workflow re-run that triggers it. See Residual. A fifth correction landed in 0ca9800, docs-only and on a surface the earlier rounds did not touch: the PR updated the setup guide and the .github/cursor-review/ directory README, but not the root README.md public catalog, whose cursor-review.yml row still described the check surface as it was before this branch. AGENTS.md carries a standing warning that this catalog has drifted once already. No Skip-caller-bump: true trailer on it — the trailer declares the whole PR bump-irrelevant for every fleet and this PR is behavioral, and it would be moot anyway, since the cursor-review fleet watches .github/workflows/cursor-review.yml, .github/cursor-review/** and scripts/check-pr-size, not the root README. A sixth correction landed in 76aa262, and it is the first that is behavioral rather than docs-only: Panel integrity failed on incremental_subset == 'false' with an ::error:: asserting the cells had been prioritized onto out-of-scope hunks. Merging #321 made that assertion false — the block is discarded whole and the panel reads the full reviewed diff — so the check was reddening a required-eligible context over a run indistinguishable from a normal one, while the setup guide and the env comment still described the cause as inert. The cause is now a non-failing ::warning:: with a true message, and the relationship is pinned by test, because publishing an output flipped this job's meaning without editing it and nothing else would catch the next such flip. This is a deviation from the plan as specified, which listed incremental_subset as a failing cause; the premise it rested on no longer holds. A seventh correction is to this description rather than to the tree: What changed §2 still enumerated the new job as needs: [gate, diff-size, review, consolidate, post-review], the five-job list from the first round. The second and third review rounds added preflight and ledger — the fail-open fixes that are the substance of the Judgment calls entry on decision jobs — so the summary contradicted both the merged file and its own rationale, on precisely the list that whole saga is about. Corrected to the seven the file declares; no code changed.

Residual

Not fixed here, and worth its own follow-up:

A no-panel run on the same head SHA supersedes a red verdict with a skip. Found while self-reviewing, after the three rounds above; thread left open at #discussion_r4050781260. The three corrections above closed the decision-job half of the fail-open — a job dying and skipping the check. This is the other half and needs no failure at all: once run A on SHA X reports red, any later event on X where the gate says no panel (an unrelated label toggled, skip-cursor-review applied, the trigger label removed, or the whole-workflow re-run this PR's own setup guide recommends, which sets already_reviewed=true) makes this job's if: false at its second disjunct. The job is skipped, a skipped job still publishes a Panel integrity check run newer than run A's, branch protection reads the latest of a given name, and a skipped required check passes. The panel is still short and the gate is green. Not patched here, deliberately: the verdict lives only in consolidate/post-review job outputs, which do not exist on a run where the panel did not run, so making it event-independent means reading prior state from outside the run (checks: read on the previous conclusion, or pull-requests: read on the landed review) — and this job is pinned by test at permissions: {} with "no API call, no checkout, no network" as a stated property. blocking-gate is the model and shows why it is a design decision rather than a patch: its job if: is unconditional precisely so its verdict is event-independent, and its exemptions for those same two branches are safe only because they fall through to a live thread query. Until it lands, the guide's "require Panel integrity" advice is a weaker gate than that section reads — worth weighing at merge, though a red that a label toggle can clear is still strictly more signal than today's silent green. The docs half is now landed (3d0b477): supersession is a named shape in the setup guide's "before you require it" list (trigger set, and "push a new commit" as the way to clear a red instead of re-triggering on the reviewed one), a matching limit sits beside the forgeability one in the README, both name BE-15604 so the caveat expires with the fix, and the "Re-run failed jobs" paragraph now points at that shape rather than silently recommending into it. The check's behaviour is unchanged; what changed is that the page no longer advises the action that hides the signal it tells you to require.
No live panel run exercised this. The plan's third verification step — trigger the review label on a test PR, confirm Panel integrity green with all six legs submitting, then force a leg failure (e.g. timeout-minutes: 1 on a branch of the reusable pinned via workflows_ref) and confirm that leg's context and Panel integrity go red while the consolidated review still posts — was not run. It needs a live CURSOR_API_KEY panel on a consumer repo, which this change cannot reach; and this repo's own ci-cursor-review.yml pins a merged-main SHA rather than the branch, so the panel that reviews this PR runs the old code. Everything asserted above is therefore static (workflow-shape tests) plus direct execution of the two run: blocks against fabricated inputs — it is not evidence from a real run. The first live panel after the caller bump is the real acceptance test; watch for Panel integrity red-on-cancel noise and for the ~40% caller-job-red rate landing as expected.
~~incremental_subset is unverified because it does not exist yet.~~ Resolved. The sibling ticket landed as #321 and the output exists; the name and the literal false value were both confirmed against diff-size. The branch is no longer dead code — but confirming it also showed the cause was asserting a consequence #321 had made impossible, so it was demoted from a failure to a ::warning:: (Judgment calls). What remains unverified is the same thing as every other cause here: no live panel run has exercised it.
notify-complete still does not mention a short panel. The Slack complete DM reports judge degradation but not the cell count, so the person who triggered the review is told the review is done without being told a third of the panel never answered. Out of scope here (the ticket scoped this to check-run conclusions), but it is the same gap in the other notification channel.
The measurements quoted in the code comments and docs are the investigation's, not re-measured here. The 92-run / 38-run / 52-of-552-cell figures come from the spike this change implements; I did not re-run that sweep, and I have no access to the run history to do so. They are quoted in an anonymized form (no repo named) because this repo is public. If the underlying timeout rate has moved since, the "~40% of runs" figure in the setup guide is what would need updating.
No caller-side audit was possible for the ~40% caller-job-red rate. Review raised that the per-leg exit 1 turns every consumer's caller job red on a measured ~40% of runs the moment this SHA is bumped, with no input to stage it. The setup guide's answer is to require Panel integrity and not the caller job, but whether any consumer currently marks the caller job a required check cannot be checked from here — rosters are repo secrets. That one-time audit belongs before the fleet bump, not in this diff, and the thread is left unresolved so it is visible at merge time.
The panel count is forgeable by the panel. A cell's status is written by its own --trust agent, so a prompt-injected cell can green its leg and the count. Now documented as a limit in all three surfaces rather than fixed: making it forgery-resistant needs the submission recorded outside the cell's own writable job, which is a change to the submission path, not to this check.
Aggregate panel findings has the same json.JSONDecodeError gap. consolidate's artifact read (cursor-review.yml ~line 1985) catches JSONDecodeError but not UnicodeDecodeError, on files agents write. Pre-existing and untouched here; it fails closed (the step dies, consolidate goes red, and Panel integrity is now red on that via the new CONSOLIDATE_RESULT cause), so it is visible rather than silent.
Nothing here reduces the leg-failure rate. The change makes a stalled cursor-agent visible; it does not make it rarer. Explicitly out of scope per the plan, but the ~40% caller-job-red rate is a strong argument for tackling the timeout rate next, and it is the thing most likely to make reviewers start ignoring the red.

_{🔄 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/320 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 9/18/2026 **Status:** ✅ Merged **Merged:** 10/7/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/be-15559-panel-integrity-check` --- ### 📝 Commits (10+) - [`4c6e90e`](https://github.com/Comfy-Org/github-workflows/commit/4c6e90edd1566853a3a3ff4496e27c4e74bcbf89) feat(cursor-review): make panel integrity a check-run conclusion - [`1ad01ef`](https://github.com/Comfy-Org/github-workflows/commit/1ad01efe3f7fbed35b576465b81ba4b7a26e4c3e) fix(cursor-review): fail Panel integrity closed when the decision jobs did not succeed - [`e4e287d`](https://github.com/Comfy-Org/github-workflows/commit/e4e287da0e265e24476bc01540f88f7b4ed58e22) fix(cursor-review): fail Panel integrity closed on a dead preflight and on absent producer results - [`10090cb`](https://github.com/Comfy-Org/github-workflows/commit/10090cbb269b4b9b7b7cc1829a983b4f19434b21) Merge remote-tracking branch 'origin/main' into matt/be-15559-panel-integrity-check - [`2bf8e31`](https://github.com/Comfy-Org/github-workflows/commit/2bf8e31092dd811a91a3027a1387b29795c1f3a5) fix(cursor-review): fail Panel integrity closed on a dead ledger too - [`73557d7`](https://github.com/Comfy-Org/github-workflows/commit/73557d76fbd81f2888483f17f97dcaafca49cd4c) docs(cursor-review): name the Panel integrity bullets instead of counting them - [`5d8d430`](https://github.com/Comfy-Org/github-workflows/commit/5d8d430ebbe0d88b54113332f1b0c8768157917f) Merge remote-tracking branch 'origin/main' into matt/be-15559-panel-integrity-check - [`3d0b477`](https://github.com/Comfy-Org/github-workflows/commit/3d0b4775d980e2168287e99ef8655b4e755dac35) docs(cursor-review): caveat that a later no-panel run supersedes a red Panel integrity - [`4b6c9a8`](https://github.com/Comfy-Org/github-workflows/commit/4b6c9a8d80b5579a5ae1696e8050f8929f94e715) test(cursor-review): pin `fail-fast: false` so a red leg cannot cancel the panel - [`99be8b7`](https://github.com/Comfy-Org/github-workflows/commit/99be8b776764bccdc805ccdfbe82fd2488713cf5) Merge remote-tracking branch 'origin/main' into matt/be-15559-panel-integrity-check ### 📊 Changes **5 files changed** (+1453 additions, -5 deletions) <details> <summary>View changed files</summary> 📝 `.github/cursor-review/README.md` (+75 -1) ➕ `.github/cursor-review/tests/test_workflow_panel_integrity.py` (+875 -0) 📝 `.github/workflows/cursor-review.yml` (+363 -3) 📝 `README.md` (+1 -1) 📝 `docs/callers/cursor-review.md` (+139 -0) </details> ### 📄 Description ## ELI-5 When the review panel runs, six AI reviewers each look at your PR and one judge writes up what they found. If one of those six gets stuck and never answers, its check still turned **green** — so anything reading the PR's check rollup saw a complete panel when it was really a third of a panel. This PR makes that visible: a reviewer that didn't answer turns its own check **red**, and one new `Panel integrity` check says whether the panel as a whole was whole. The review still posts either way — nothing new blocks a merge unless a repo chooses to require the new check. ## What changed **1. A leg that didn't submit is red.** The `review` job gets a final step, `Fail the leg when the cell did not submit`, which reads `/tmp/findings-out/findings.json` back and exits 1 when its `status` is not `ok`. It sits *after* `Upload findings artifact` on purpose: the artifact is already published, `consolidate` and `notify-complete` gate on `needs.review.result != 'skipped'` (a **failed** matrix is not a skipped one) and `post-review` gates on `consolidate`, so the consolidated review still posts. The only thing that changes is that the gap reaches the rollup. The `Run cursor review` comment block is updated to say the step cap avoids a *cancelled* run, and that the leg is now deliberately *failed* when the cell did not submit. **2. A panel-level `Panel integrity` context.** `consolidate` now exposes `ok_count`, `total` (from the aggregate step), `degraded` and `judge_status` as job outputs. A new `panel-integrity` job — `needs: [gate, round-cap, diff-size, preflight, ledger, review, consolidate, post-review]` (`preflight` and `ledger` joined in the second and third review rounds, `round-cap` when #345's `max_rounds` cap landed on `main` — see Judgment calls), `always()` plus the same four gate conditions `consolidate` uses (nested under an upstream-failure disjunct, so a failed decision job is red rather than skipped), `permissions: {}`, `timeout-minutes: 5` — emits one `::error::` per cause and exits 1 when any of: fewer cells submitted than ran; `ungated_findings` is non-empty and greater than zero; `delivered != 'true'`; `degraded == 'true'`. A `false` `incremental_subset` is annotated as a `::warning::` and is deliberately **not** a cause — see Judgment calls. Otherwise it prints one `::notice::Panel integrity: <ok>/<total> cells, <n> anchored finding(s), 0 unanchored.` and exits 0. It gates no other job — a short panel must not also cost the PR the findings it *did* produce. **3. Caller-facing docs.** A `## Panel integrity` section in the setup guide next to the Blocking gate (what red means, that it is the context an automated merge gate should read, and the two-switch required-check shape), plus a job/check-run table and a matching section in the directory README. Both surfaces scope the claim to *"was the panel whole?"* and state plainly that the deliberate no-panel branches stay **skipped**, that a required skipped check passes, and that over-cap in particular wants the Blocking gate. The root [`README.md`](README.md) catalog row is updated too (`0ca9800`): it enumerated the check surface as Blocking-gate-only, so after this branch it was wrong by omission on the two things an adopter most needs — that there is now a second, always-on `Panel integrity` context and that it, not a panel leg, is the stable one to require; and that the caller job goes red on ~40% of runs. The skip semantics are stated in the same breath; forgeability and same-SHA supersession are pointed at rather than restated, so the catalog cannot drift from the guide on the details. No caller change is required; the shared caller bumper propagates the workflow SHA. No `Skip-caller-bump` trailer — this is a behavioral change and every fleet caller should bump. ## Judgment calls - **The leg check reads the artifact defensively rather than as a one-liner.** That cell's own agent runs `--trust` with shell access in the same job, so `findings.json` is untrusted bytes and its `status` lands in a `::workflow command::` line. A missing or unparseable file now reads as "did not submit" and produces the proper `::error::` instead of a traceback, and the value is sanitized by **replacing** disallowed characters rather than dropping them — deleting them would let a crafted `"o k"` collapse into the one value that passes. `Panel integrity` flattens every value it interpolates for the same reason (a newline forges a second workflow command) and, since `bec0638`, percent-escapes them too — the runner unescapes `%0A` back into a newline, so stripping real newlines alone left `JUDGE_STATUS` (an unvalidated string from the judge agent's own tool) able to forge a command without containing one. Both are covered by tests. - **`always()`, not `!cancelled()`, on `panel-integrity`.** GitHub counts a *skipped* required check as passing, so green-on-cancel would be a fail-open for any repo that requires this — the same reasoning the Blocking gate documents at its own `if:`. A cancelled run therefore reports red; under the documented `cancel-in-progress` caller that red lands on the superseded head SHA, not the new one. Documented. - **The caller job now goes red on a failing leg, on a measured ~40% of runs.** A reusable's caller job takes the aggregate conclusion of the jobs inside it, so this is unavoidable given (1) — and it is the point, since the rollup is the surface the whole ticket is about. Called out explicitly in the setup guide, with the instruction to require `Panel integrity` rather than the caller job. - **A failed `Gate`, `Diff size check` or `Preflight` fails `Panel integrity`, it does not skip it.** ~~Originally this skipped, on the reasoning that the `Gate` context is itself red so the rollup still carries the signal.~~ That was a fail-open, caught in review: all four gate conditions read job *outputs*, an output is empty when the job producing it failed, so the check skipped on exactly those runs — and GitHub counts a skipped required check as **passing**, which would have handed a green merge gate to any caller that took this PR's own advice and required it. The job `if:` now also runs on `needs.gate.result != 'success'` and on a fresh in-scope review whose `diff-size` did not succeed, and a first step `Fail if the panel decision itself did not complete` turns that into red — the same fail-closed shape as `Blocking gate`'s first two guards. Guard first, so the report step never evaluates its causes off empty outputs. A second review round found that fix had closed only two of the three decision jobs: `review` declares `needs: preflight`, so a failed `preflight` skips the whole matrix and leaves `needs.review.result == 'skipped'` — indistinguishable from the deliberate no-panel branches — and the check skipped over a run where not one cell started. `preflight` now joins `needs:` and the same guard covers it. A **third** round found the same fail-open one dependency further over: the matrix declares `needs: [gate, diff-size, preflight, ledger]`, and `ledger` was the one job of the four still unguarded — a red `ledger` skips the matrix, `review` reports `skipped`, and the check skipped into a green required check. `ledger` now joins `needs:`, the job `if:` and the guard as well. It reads like it cannot fail (every step in it is `continue-on-error`, precisely so it never skips the matrix) but that makes it rare, not unreachable: `Ensure ledger artifact exists` carries no `continue-on-error`, its 20-minute job cap sits above only 15 minutes of step caps, and a cancellation or lost runner is not a step outcome at all. The pin is now the RELATIONSHIP — a test reads `review`'s `needs:` and requires every entry to appear in all three places — because this hole was re-opened twice by a job being added to the matrix and not to the check. The deliberate skips are untouched: they are the branches where `gate` and `diff-size` both *succeeded* and said no panel was warranted. Over-cap stays a skip on purpose and is routed to the Blocking gate, whose own second guard I verified fails closed on exactly that path. `round-cap` (#345, merged from `main`) is the one decision job the matrix does not need directly — it sits upstream of `diff-size`, whose own `if:` skips on `capped == 'true'` — so it is read on both sides: a non-success `round-cap` joins the guard and goes red, and a successful `capped` head is a deliberate skip like over-cap (the Blocking gate already fails closed on it). Before that, a capped head went red telling the caller to re-run a job that would just hit the cap again; the tests now *evaluate* the job and guard `if:` expressions over the capped / uncapped / `max_rounds`-off / failed-cap / unlabelled / over-cap cases rather than substring-matching them. - **Every cause is paired with its producing job's `result`, not just its outputs.** An output is the empty string when the job producing it failed, and every cause treated empty as a pass. Executing the pre-fix `run:` block with `delivered=true` (which `post-review.py` writes the moment the POST returns, before the rest of that job runs) and a failed `post-review` exits **0** and prints `Panel integrity: 6/6 cells` — a green panel signal over a red delivery job. `ok_count`/`total` are sharper still: both empty on a dead `consolidate`, and unset-vs-unset compares *equal*. The step now reads `needs.consolidate.result` and `needs.post-review.result` (the env names `blocking-gate` and `notify-complete` already use) and rejects an empty count explicitly instead of comparing it. - **The leg check is scoped as an availability signal, not an attestation.** The "did this cell submit" verdict is the `status` the cell's own `--trust` agent wrote, so a prompt-injected cell can green itself and the panel count with it. That limit is now stated in the workflow comment, the caller guide and the README, along with what would fix it (recording the submission outside the cell's own writable job). It catches the stalls, crashes and step caps that are what every observed failure has been; it is not proof against an adversarial PR, and the docs no longer read as though it were. - **A discarded incremental block warns; it is deliberately not a cause.** This started as a failing cause, written while `diff-size` declared no such output. Merging #321 (`84db66a`) both published the output and inverted what a `false` means, without touching a line of this job. #321 made the subset check fail **closed**: a block that does not verify is discarded whole and the panel reviews the full reviewed diff alone. So the cells were never "prioritized onto hunks that were not in scope" as the `::error::` claimed — they got no prioritization hint at all, which is byte-identical input to a run that never built a block (round 1, and every "could not build one" branch), and those report `true`. Reddening the one context callers are told to mark **required**, over a run whose reviewed content is indistinguishable from a normal one and where nothing was skipped and no finding suppressed, is a merge-blocking false positive — and it is the one cause that never indicated a short panel, which is the only question this check answers. It now emits a `::warning::` naming what actually happened and pointing at the `Incremental diff discarded` warning on `Diff size check` for the counts. A non-subset block is still a real *builder* defect worth seeing; it is not a panel-integrity failure. The other five causes are untouched. - **A new sibling test file rather than edits to `test_workflow_job_isolation.py`.** That suite pins the credential split; this one pins the integrity signals. Same house style: stdlib only, no PyYAML, comments stripped before every assertion so a comment naming a step can never satisfy it. ## Capability check The diff adds `exit 1` / `::error::` paths, which is the shape that usually means a capability is being denied — it is not one here, and I verified that rather than asserting it. Nothing the workflow could do before is refused: the new check is advisory, gates no job, and the consolidated review still posts over a short panel. I confirmed that by reading every downstream `if:` — `consolidate` and `notify-complete` key on `needs.review.result != 'skipped'`, `post-review` on `needs.consolidate.result == 'success'`, `blocking-gate` does not `needs:` the panel at all — and a repo-wide grep for `needs.review` returns only those three sites, so no path keys on the panel having *succeeded*. A test asserts no job `needs: panel-integrity`. ## Provenance - **Authored by:** agent-work loop - **Verified:** at head `432fa41` (merge of `origin/main` `cfca794`, #355–#363 — no textual conflict; those touch auto-approve/approve-external, `post-review`'s `needs:` and `diff-size`'s new `incremental_state` output, none of which `Panel integrity` reads; `incremental_subset`, which it does read, is still emitted unchanged): - cursor-review (1119 tests), cursor-approve (161) and workflow-pins (551, 43 skipped) unittest suites pass on that tree, including `test_workflow_panel_integrity.py`. - `check_workflow_pins.py` OK (167 `uses:` refs SHA-pinned); `check_agents_md.py --root .` passes with 2 pre-existing warnings; `check-org-repo-literals.sh` OK. - Job graph pinned by tests: `panel-integrity`'s `needs:` is a superset of `review`'s decision jobs plus `round-cap`, every output it reads is declared upstream, nothing `needs:` it, `permissions: {}` intact; the `if:` truth-table tests cover capped / uncapped / failed-cap / over-cap cases. - PR CI on the previous head `55ede56`: all 8 checks pass; CI on `432fa41` pending at time of writing; no `merge_group` run exists for this PR. - Not done: no live panel run (see Residual). - **Deviations:** - Three fail-opens fixed in review: failed decision jobs (`gate`, `diff-size`, `preflight`, `ledger`) now fail the check rather than skip it, pinned as a relationship to `review`'s `needs:`; causes are paired with producing-job results, not outputs alone. - `%` → `%25` escape added to `flatten()` so agent-written `JUDGE_STATUS` cannot inject workflow commands (`bec0638`). - `incremental_subset == 'false'` demoted from a failing cause to a `::warning::` after #321 made a discarded block mean "panel read the full diff" (`76aa262`) — a deviation from the plan, whose premise no longer holds. - Docs: setup guide, directory README and root `README.md` catalog row updated (`3d0b477`, `0ca9800`), including the same-head-SHA supersession caveat. - Semantic conflict with #345 fixed (`8c786ef`): `round-cap` added as a decision job, and a `max_rounds`-capped head now skips the check instead of going red. - Answered but not actioned, threads left open on purpose for the merger: staging the per-leg `exit 1`, and same-head-SHA supersession (design change, filed as a follow-up). <details><summary>Deviation detail (carried forward)</summary> none against the plan as specified. Three corrections after review, all fail-opens the plan did not anticipate: (1) "a failed `Gate` skips this check" became fail-closed, extended in the second round to `preflight`, the third decision job — the matrix `needs:` it, so its failure was reaching the check as an ordinary `skipped` review; (2) causes read job outputs only, so a producer that died left a clean-looking report; (3) the round-2 fix named `preflight` specifically rather than the invariant behind it, leaving `ledger` — the fourth job the matrix `needs:` — still able to skip the check into a green required check. That one is now pinned as a relationship between `review`'s `needs:` and this job's, so the class is closed rather than the third instance of it. The docs asserting the superseded behaviour were corrected in the same commits. A fourth correction landed in `bec0638`, and it is a reporting-integrity hole rather than a fail-open in the gate: `flatten()` stripped real newlines before interpolating untrusted values into `::error::` lines, but the runner unescapes `%0A` in a command's message, so a judge writing its `status` as `error%0A::stop-commands::tok` reaches the annotation with no newline in it and the runner supplies one. `JUDGE_STATUS` is the one reachable value (unvalidated agent text; the counts are python-formatted integers and the rest are job results). It cannot forge the check's *conclusion*, which is driven by the `causes` counter and the exit code — and it is strictly narrower than the forgeability limit already documented — but it was the one annotating surface in this repo not applying the `%` -> `%25` escape that `check_agents_md.py`, `check_workflow_pins.py`, `check_public_repo_hygiene.py`, `check_coderabbit_config.py` and `check-org-repo-literals.sh` all do. Two findings are answered but **not** actioned, and both threads are left open on purpose: staging the per-leg `exit 1` behind an input, and a fourth fail-open found during the reviews pass — a no-panel run on the same head SHA supersedes a red `Panel integrity` with a skip. The second is a design change, not a patch, and is filed as a follow-up; its *documentation* half did land, in `3d0b477`, because the setup guide was not merely silent on that hole — its "Re-run failed jobs" paragraph actively recommended the whole-workflow re-run that triggers it. See Residual. A fifth correction landed in `0ca9800`, docs-only and on a surface the earlier rounds did not touch: the PR updated the setup guide and the `.github/cursor-review/` directory README, but not the root `README.md` public catalog, whose `cursor-review.yml` row still described the check surface as it was before this branch. AGENTS.md carries a standing warning that this catalog has drifted once already. No `Skip-caller-bump: true` trailer on it — the trailer declares the whole PR bump-irrelevant for every fleet and this PR is behavioral, and it would be moot anyway, since the cursor-review fleet watches `.github/workflows/cursor-review.yml`, `.github/cursor-review/**` and `scripts/check-pr-size`, not the root README. A sixth correction landed in `76aa262`, and it is the first that is behavioral rather than docs-only: `Panel integrity` failed on `incremental_subset == 'false'` with an `::error::` asserting the cells had been prioritized onto out-of-scope hunks. Merging #321 made that assertion false — the block is discarded whole and the panel reads the full reviewed diff — so the check was reddening a required-eligible context over a run indistinguishable from a normal one, while the setup guide and the env comment still described the cause as inert. The cause is now a non-failing `::warning::` with a true message, and the relationship is pinned by test, because publishing an output flipped this job's meaning without editing it and nothing else would catch the next such flip. This is a deviation from the plan as specified, which listed `incremental_subset` as a failing cause; the premise it rested on no longer holds. A seventh correction is to this description rather than to the tree: **What changed** §2 still enumerated the new job as `needs: [gate, diff-size, review, consolidate, post-review]`, the five-job list from the first round. The second and third review rounds added `preflight` and `ledger` — the fail-open fixes that are the substance of the Judgment calls entry on decision jobs — so the summary contradicted both the merged file and its own rationale, on precisely the list that whole saga is about. Corrected to the seven the file declares; no code changed. </details> ## Residual Not fixed here, and worth its own follow-up: - **A no-panel run on the same head SHA supersedes a red verdict with a skip.** Found while self-reviewing, after the three rounds above; thread left open at [`#discussion_r4050781260`](https://github.com/Comfy-Org/github-workflows/pull/320#discussion_r4050781260). The three corrections above closed the *decision-job* half of the fail-open — a job dying and skipping the check. This is the other half and needs no failure at all: once run A on SHA `X` reports red, any later event on `X` where the gate says no panel (an unrelated label toggled, `skip-cursor-review` applied, the trigger label removed, or the whole-workflow re-run this PR's own setup guide recommends, which sets `already_reviewed=true`) makes this job's `if:` false at its second disjunct. The job is skipped, a skipped job still publishes a `Panel integrity` check run newer than run A's, branch protection reads the latest of a given name, and a skipped required check passes. The panel is still short and the gate is green. Not patched here, deliberately: the verdict lives only in `consolidate`/`post-review` job outputs, which do not exist on a run where the panel did not run, so making it event-independent means reading prior state from outside the run (`checks: read` on the previous conclusion, or `pull-requests: read` on the landed review) — and this job is pinned by test at `permissions: {}` with "no API call, no checkout, no network" as a stated property. `blocking-gate` is the model and shows why it is a design decision rather than a patch: its job `if:` is unconditional precisely so its verdict is event-independent, and its exemptions for those same two branches are safe only because they fall through to a live thread query. Until it lands, the guide's "require `Panel integrity`" advice is a weaker gate than that section reads — worth weighing at merge, though a red that a label toggle can clear is still strictly more signal than today's silent green. The **docs half is now landed** (`3d0b477`): supersession is a named shape in the setup guide's "before you require it" list (trigger set, and "push a new commit" as the way to clear a red instead of re-triggering on the reviewed one), a matching limit sits beside the forgeability one in the README, both name BE-15604 so the caveat expires with the fix, and the "Re-run failed jobs" paragraph now points at that shape rather than silently recommending into it. The check's behaviour is unchanged; what changed is that the page no longer advises the action that hides the signal it tells you to require. - **No live panel run exercised this.** The plan's third verification step — trigger the review label on a test PR, confirm `Panel integrity` green with all six legs submitting, then force a leg failure (e.g. `timeout-minutes: 1` on a branch of the reusable pinned via `workflows_ref`) and confirm that leg's context and `Panel integrity` go red while the consolidated review still posts — was **not run**. It needs a live `CURSOR_API_KEY` panel on a consumer repo, which this change cannot reach; and this repo's own `ci-cursor-review.yml` pins a merged-main SHA rather than the branch, so the panel that reviews *this* PR runs the old code. Everything asserted above is therefore static (workflow-shape tests) plus direct execution of the two `run:` blocks against fabricated inputs — it is not evidence from a real run. The first live panel after the caller bump is the real acceptance test; watch for `Panel integrity` red-on-cancel noise and for the ~40% caller-job-red rate landing as expected. - ~~**`incremental_subset` is unverified because it does not exist yet.**~~ **Resolved.** The sibling ticket landed as #321 and the output exists; the name and the literal `false` value were both confirmed against `diff-size`. The branch is no longer dead code — but confirming it also showed the cause was asserting a consequence #321 had made impossible, so it was demoted from a failure to a `::warning::` (Judgment calls). What remains unverified is the same thing as every other cause here: no live panel run has exercised it. - **`notify-complete` still does not mention a short panel.** The Slack complete DM reports judge degradation but not the cell count, so the person who triggered the review is told the review is done without being told a third of the panel never answered. Out of scope here (the ticket scoped this to check-run conclusions), but it is the same gap in the other notification channel. - **The measurements quoted in the code comments and docs are the investigation's, not re-measured here.** The 92-run / 38-run / 52-of-552-cell figures come from the spike this change implements; I did not re-run that sweep, and I have no access to the run history to do so. They are quoted in an anonymized form (no repo named) because this repo is public. If the underlying timeout rate has moved since, the "~40% of runs" figure in the setup guide is what would need updating. - **No caller-side audit was possible for the ~40% caller-job-red rate.** Review raised that the per-leg `exit 1` turns every consumer's caller job red on a measured ~40% of runs the moment this SHA is bumped, with no input to stage it. The setup guide's answer is to require `Panel integrity` and not the caller job, but **whether any consumer currently marks the caller job a required check cannot be checked from here** — rosters are repo secrets. That one-time audit belongs before the fleet bump, not in this diff, and the thread is left unresolved so it is visible at merge time. - **The panel count is forgeable by the panel.** A cell's `status` is written by its own `--trust` agent, so a prompt-injected cell can green its leg and the count. Now documented as a limit in all three surfaces rather than fixed: making it forgery-resistant needs the submission recorded outside the cell's own writable job, which is a change to the submission path, not to this check. - **`Aggregate panel findings` has the same `json.JSONDecodeError` gap.** `consolidate`'s artifact read (`cursor-review.yml` ~line 1985) catches `JSONDecodeError` but not `UnicodeDecodeError`, on files agents write. Pre-existing and untouched here; it fails closed (the step dies, `consolidate` goes red, and `Panel integrity` is now red on that via the new `CONSOLIDATE_RESULT` cause), so it is visible rather than silent. - **Nothing here reduces the leg-failure rate.** The change makes a stalled `cursor-agent` visible; it does not make it rarer. Explicitly out of scope per the plan, but the ~40% caller-job-red rate is a strong argument for tackling the timeout rate next, and it is the thing most likely to make reviewers start ignoring the red. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-26 22:38:30 +00:00