Repository navigation
fix(cursor-review): pass Panel integrity on a cancelled run only when a push superseded it - #440
Conversation
cancel-in-progress leaves the decision jobs `cancelled`. Panel integrity's
`always()` then ran and failed the check ("re-run the failed job") on a run
that was superseded. `!cancelled()` skips that run. A failed decision job, or
a job timeout (result `cancelled` while the workflow is still running), still
fails the check.
Co-authored-by: Matt Miller <mattmillerai@users.noreply.github.com>
|
Warning Review limit reached
This review includes 5 billable files and costs up to $1.25.
Reviews can continue after your included limit without a manual trigger. An admin must approve usage-based billing. Or wait 2 minutes for your next included review. View limit details
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Priority: ⬇️ Low Merge Risk: 🟡 Moderate · up to
|
| Check name | Status | Explanation |
|---|---|---|
| Linked Issues check |  |
Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | |
Check skipped because no linked issues were found for this pull request. |
✨ Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
✨ Simplify code
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Comment @coderabbitai help to get the list of available commands.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔍 Cursor Review — Consolidated panel
Triggered by @mattmillerai.
Found 3 finding(s).
| Severity | Count |
|---|---|
| 🟠 High | 1 |
| 🟢 Low | 2 |
Panel: 6/6 reviewers contributed findings.
…nly after the head moved `!cancelled()` skipped the check on EVERY workflow cancel. A manual cancel has no replacement run, and a same-SHA replacement can decline to review, so the skip handed that head a green required check over an unfinished panel. The job is back on `always()`. A new first step, `Judge a cancelled run`, runs only on a cancelled run and re-reads the PR's live head: it passes (and the guard and report steps stand down) only when the head moved past the SHA the run was reviewing. Head unchanged, no PR head in the event, or an unreadable head all exit 1. The job now holds `pull-requests: read` for that one read. The supersede case stays green and the fail-open is closed. A job timeout fails closed whichever way GitHub reports `cancelled()` for it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
mattmillerai
changed the title
ELI5
When a code-review run gets cancelled because someone pushed newer code, the review check no longer shows a confusing red "re-run the failed job". A run cancelled for any other reason, such as a manual cancel, still fails. A required check can't quietly go green when nobody finished the review.
Before
Panel integrityusedif: always() && (…).always()still starts the job aftercancel-in-progresscancels the run. The decision jobs (gate,round-cap,diff-size,preflight,ledger) then reportcancelled, notfailure, and the guard failed the check:That is a false failure when the run was superseded by a newer push. The same red happened when a cancel landed mid-panel, because
consolidate/post-reviewcome backcancelled.After
The job stays on
always(). Skipping it with!cancelled()(this PR's first revision) would be a fail-open:cancelled()is true for every workflow cancel. A manual cancel has no replacement run, and a same-SHA replacement can decline to review. A skipped required check passes, so that skip would hand the head a green completeness gate over an unfinished panel.Instead, a new first step,
Judge a cancelled run, runs only when the run was cancelled and re-reads the PR's live head:superseded. The guard and report steps stand down, so the check is green on a head nobody can merge any more, and required checks are read on the new head.The job's permissions go from
{}topull-requests: readfor that one API read.Fail-closed is unchanged for a run that was not cancelled
failurestill satisfies!= 'success', and the guard still exits 1.timeout-minutesfails closed whichever way GitHub reportscancelled()for it. Either the guard reads itscancelledresult, or the head check finds the head unmoved.max_rounds, over the diff-size cap) still skip.A same-SHA re-trigger under
cancel-in-progress(re-applying the label while a panel runs) still paints the cancelled run red on that SHA, which is the fail-closed side. The replacement run's later result is the one to read. The Blocking gate is untouched and still usesalways().Not in this PR
Two runs a few seconds apart for one event is the caller (or the app delivery) starting the workflow twice. This reusable's concurrency group already cancels the earlier one, which is what stops both panels from posting. Collapsing the duplicate trigger is not something the reusable can do safely; letting both runs finish would post two reviews.
Provenance
Verified:
python3 -m unittest discover -s .github/cursor-review/tests -p 'test_*.py': 1740 tests OK, including the newCancelledRunTest, which runs the step against a stubgh(head moved / unmoved / unreadable / no PR head)check_workflow_pins.pyOKcheck_agents_md.pypassed, shellcheck on the new step's script OK, workflow YAML parsesDeviations:
!cancelled()job skip with an in-job head-moved check, per the review panel's High finding (raised by all six reviewers) that the skip was a fail-open on a manual or same-SHA cancelpull-requests: readinstead of no permissions