From 061a2ca2484d06ff1dc642d09187d75d1b03361e Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Wed, 21 Oct 2026 23:30:29 +0000 Subject: [PATCH 1/2] fix(cursor-review): skip Panel integrity when the run is cancelled 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 --- .github/cursor-review/README.md | 10 +- .../tests/test_workflow_panel_integrity.py | 102 +++++++++++++++--- .github/workflows/cursor-review.yml | 48 ++++++--- README.md | 2 +- docs/callers/cursor-review.md | 27 +++-- 5 files changed, 151 insertions(+), 38 deletions(-) diff --git a/.github/cursor-review/README.md b/.github/cursor-review/README.md index 51327c6f..09194b59 100644 --- a/.github/cursor-review/README.md +++ b/.github/cursor-review/README.md @@ -68,13 +68,13 @@ The jobs, in the order they report: | Job / check run | Runs when | Red means | |---|---|---| | `Gate` | always | The trigger decision itself failed (a label read or the dedupe API call), so whether the PR should be reviewed is unknown. | -| `Prior-review ledger` | reviewing | Designed never to fail the run: the review matrix `needs:` it, so it degrades to an empty ledger rather than erroring. Rare is not never — if it does go red (job timeout, cancellation, a lost runner), the matrix skips and `Panel integrity` reports that rather than passing. | +| `Prior-review ledger` | reviewing | Designed never to fail the run: the review matrix `needs:` it, so it degrades to an empty ledger rather than erroring. Rare is not never — if it does go red (a job timeout or a lost runner, both a `cancelled` job result while the run is still going), the matrix skips and `Panel integrity` reports that rather than passing. A run cancelled by `cancel-in-progress` skips `Panel integrity` instead; that run was superseded, and the replacement reports. | | `Diff size check` | reviewing | The `BASE...HEAD` diff could not be built at all. An over-cap PR is not a failure — it is a skip plus a PR comment. | | `Preflight — validate model catalog` | reviewing | A pinned panel model is delisted. The panel is skipped rather than quietly running a lab short. | | ` ()` — one per cell | reviewing | **That cell did not submit a review.** Its artifact is still uploaded and the panel still consolidates; the leg is red so the gap reaches `statusCheckRollup`. Never require one of these: the context name carries the model id and changes whenever the panel list does. | | `Consolidate panel` | reviewing | The judge job failed outright (a hung judge is absorbed and falls back to the panel union instead). | | `Post review` | Consolidate panel succeeded | The POST failed, or succeeded without the run being able to confirm it. The findings are written to the job summary in that case — see [Delivery, the body-only fallback, and a throttled POST](#delivery-the-body-only-fallback-and-a-throttled-post). | -| `Panel integrity` | reviewing, **or** the decision to review failed | The panel was short, findings went unanchored, nothing was delivered, the judge never adjudicated — or `Gate`/`Round cap`/`Diff size check`/`Preflight`/`Prior-review ledger` failed, leaving whether the PR was reviewed unknown. Skipped, not green-by-verdict, when no review was warranted (including a head stopped by the `max_rounds` cap). Advisory unless a caller marks it required. | +| `Panel integrity` | reviewing, **or** the decision to review failed, and the run was not cancelled | The panel was short, findings went unanchored, nothing was delivered, the judge never adjudicated — or `Gate`/`Round cap`/`Diff size check`/`Preflight`/`Prior-review ledger` failed, leaving whether the PR was reviewed unknown. Skipped, not green-by-verdict, when no review was warranted (including a head stopped by the `max_rounds` cap) and when `cancel-in-progress` cancelled the run. Advisory unless a caller marks it required. | | `Blocking gate` | `blocking: true` | Unresolved, non-outdated finding threads — or a round that should have produced them and did not. Opt-in. | **Post review is its own job, and that is a security boundary.** No job both @@ -117,6 +117,12 @@ read a job output (empty when the job producing it failed) and the fourth reads the matrix result, which is `skipped` whenever ANY job the matrix `needs:` (`gate`, `diff-size`, `preflight`, `ledger`) did not succeed, while a skipped required check passes: the same fail-closed guard the Blocking gate carries. +A workflow cancelled by `cancel-in-progress` is not that failure. The job's +`if:` is `!cancelled()`, so a superseded run skips this check instead of +failing it, and the run that replaced it is the one that reports. A decision +job that failed — or whose own `timeout-minutes` GitHub reports as a +`cancelled` job result without cancelling the workflow — still fails the check. + The invariant is *every* job the matrix depends on, not the four that happen to be on that list today — it was re-opened twice by a dependency being added to the matrix and not here, so the test suite now pins the two lists against each diff --git a/.github/cursor-review/tests/test_workflow_panel_integrity.py b/.github/cursor-review/tests/test_workflow_panel_integrity.py index 02f0a089..f70db56f 100644 --- a/.github/cursor-review/tests/test_workflow_panel_integrity.py +++ b/.github/cursor-review/tests/test_workflow_panel_integrity.py @@ -365,15 +365,23 @@ def test_it_needs_every_job_it_reads(self): "evaluate to the empty string, which every cause treats as a pass", ) - def test_it_reports_on_cancellation(self): - # `always()`, not `!cancelled()`: GitHub counts a SKIPPED required check - # as PASSING, so a cancelled run that skipped this job would mint a green - # context over a panel that never finished — the same fail-open the - # blocking gate documents at its own `if:`. + def test_a_cancelled_run_skips_this_job(self): + # `!cancelled()`, not `always()`. `always()` runs the job even when + # cancel-in-progress has cancelled the RUN, the decision jobs come back + # `cancelled`, and the guard then fails the check ("re-run the failed + # job") on a run that was torn down before it decided anything. That is + # the false red. `post-review` and `over-cap-comment` use `!cancelled()` + # for the same interaction. + # + # A job result of `cancelled` on a run that was NOT cancelled — a + # `timeout-minutes`, which GitHub reports as `cancelled` without + # cancelling the workflow — still matches `!= 'success'` and still + # fails closed. `cancelled()` is the workflow flag, not that result. + # The Blocking gate keeps `always()`; this check does not. condition = job_scalar(self.panel, "if") self.assertIsNotNone(condition) - self.assertIn("always()", condition) - self.assertNotIn("!cancelled()", condition) + self.assertIn("!cancelled()", condition) + self.assertNotIn("always()", condition) def test_it_is_gated_exactly_like_consolidate(self): # Reporting on a run where the panel was never supposed to happen — no @@ -529,7 +537,8 @@ def test_it_gates_nothing(self): class UndecidedRunFailsClosedTest(unittest.TestCase): - """The job must go RED, not skipped, when the decision jobs did not finish. + """The job must go RED, not skipped, when a decision job did not succeed + and the run itself was not cancelled. Every gate condition on `panel-integrity` reads a job OUTPUT, and a job that FAILED has empty outputs. Gating the job on those outputs alone therefore @@ -539,6 +548,12 @@ class UndecidedRunFailsClosedTest(unittest.TestCase): integrity` would get a green merge gate over a run that never decided whether to review the PR at all. `Blocking gate` closes the same hole with the same two guards; this suite pins that they stay closed here too. + + A run cancelled by concurrency is the exception, pinned in + `RoundCapIsADecisionJobTest`: every decision job then comes back + `cancelled` because the run was torn down, and this check skips rather + than failing on a supersede. A `cancelled` job result while the workflow + is still running (`timeout-minutes`) is not that exception. """ def setUp(self): @@ -684,9 +699,9 @@ def test_the_deliberate_skips_are_still_skips(self): CONTEXT_REF = re.compile(r"\b(?:needs|inputs|steps|vars|matrix|github|secrets)(?:\.[A-Za-z0-9_-]+)+") -def evaluate(expression, context): +def evaluate(expression, context, workflow_cancelled=False): """Evaluate an expression built only from context reads, string literals, - `always()`, `==`/`!=`, `&&`/`||` and parentheses. + `always()`, `cancelled()`, `==`/`!=`, `&&`/`||` and parentheses. `context` maps a dotted read (`needs.gate.result`, `inputs.`, `steps..outputs.`, `matrix.`) to its value. A read it does @@ -695,28 +710,39 @@ def evaluate(expression, context): `a == 'true' && inputs.b || ''` yields a string. Anything outside that small grammar fails the translation loudly rather than evaluating to a guess. + + `workflow_cancelled` is GitHub's `cancelled()` — the workflow was + cancelled — and is distinct from a job result of `cancelled` (a + `timeout-minutes`, which does not cancel the workflow). It defaults to + false: the suites that build a `needs` map are describing a run that was + not itself cancelled. """ expr = expression.strip() if expr.startswith("${{") and expr.endswith("}}"): expr = expr[3:-2].strip() py = CONTEXT_REF.sub(lambda match: repr(context.get(match.group(0), "")), expr) + # `!cancelled()` before `cancelled()`, or the negation is left as a `!`. + flag = "True" if workflow_cancelled else "False" + py = py.replace("!cancelled()", "not " + flag) + py = py.replace("cancelled()", flag) py = py.replace("always()", "True").replace("&&", " and ").replace("||", " or ") - leftover = re.sub(r"'[^']*'|\b(True|False|and|or)\b|==|!=|[()\s]", "", py) + leftover = re.sub(r"'[^']*'|\b(True|False|and|or|not)\b|==|!=|[()\s]", "", py) if leftover: # pragma: no cover - only on a grammar this helper does not know raise AssertionError("cannot evaluate expression remainder %r" % leftover) return eval(py, {"__builtins__": {}}, {}) # noqa: S307 - grammar checked above -def evaluate_if(condition, needs): +def evaluate_if(condition, needs, workflow_cancelled=False): """Evaluate a job `if:` against `needs`, which maps a job to `{"result": ..., "outputs": {...}}`. A job absent from it, or an output it - does not declare, reads as '' (see `evaluate`).""" + does not declare, reads as '' (see `evaluate`). `workflow_cancelled` is + GitHub's `cancelled()` and defaults to false.""" context = {} for job, entry in needs.items(): context["needs.%s.result" % job] = entry.get("result", "") for name, value in entry.get("outputs", {}).items(): context["needs.%s.outputs.%s" % (job, name)] = value - return evaluate(condition, context) + return evaluate(condition, context, workflow_cancelled=workflow_cancelled) class RoundCapIsADecisionJobTest(unittest.TestCase): @@ -786,6 +812,9 @@ def test_max_rounds_off_leaves_capped_empty_and_still_reports(self): self.assertFalse(evaluate_if(self.guard_if, needs)) def test_a_failed_round_cap_is_red_not_skipped(self): + # `cancelled` here is a job result (timeout-minutes), not a cancelled + # workflow. The run is still going, so the check must run and the + # guard must fire. A workflow-level cancel is the next test. for result in ("failure", "cancelled"): needs = self.fresh(**{ "round-cap": {"result": result}, @@ -798,6 +827,51 @@ def test_a_failed_round_cap_is_red_not_skipped(self): self.assertTrue(evaluate_if(self.condition, needs), result) self.assertTrue(evaluate_if(self.guard_if, needs), result) + def test_a_superseded_run_skips_rather_than_failing(self): + # cancel-in-progress: the RUN was cancelled, so every decision job's + # result is `cancelled` even though none of them failed. The job must + # not run — a skip, not the guard's red "re-run the failed job". + # Same needs with the workflow still going (the previous test's + # `cancelled` result) must keep running; only `cancelled()` changes. + needs = self.fresh(**{ + "gate": {"result": "cancelled"}, + "round-cap": {"result": "cancelled"}, + "diff-size": {"result": "cancelled"}, + "preflight": {"result": "cancelled"}, + "ledger": {"result": "cancelled"}, + "review": {"result": "cancelled"}, + "consolidate": {"result": "cancelled"}, + "post-review": {"result": "cancelled"}, + }) + self.assertFalse( + evaluate_if(self.condition, needs, workflow_cancelled=True), + "a concurrency-cancelled run still runs Panel integrity", + ) + self.assertTrue( + evaluate_if(self.condition, needs, workflow_cancelled=False), + "the same all-cancelled results on a live run must still fail closed " + "(a timeout reports `cancelled` without cancelling the workflow)", + ) + self.assertTrue(evaluate_if(self.guard_if, needs, workflow_cancelled=False)) + + def test_a_cancel_mid_panel_skips_too(self): + # The decision jobs finished and the panel was in flight when the run + # was superseded. `review` / `consolidate` come back `cancelled`, which + # is not `skipped`, so the old `always()` condition ran the report step + # and failed the check. The workflow flag skips it; the replacement run + # reports. The same shape on a live run — a matrix cancelled without + # the workflow being cancelled — still runs the check. + needs = self.fresh(**{ + "review": {"result": "cancelled"}, + "consolidate": {"result": "cancelled"}, + "post-review": {"result": "skipped"}, + }) + self.assertFalse(evaluate_if(self.condition, needs, workflow_cancelled=True)) + self.assertTrue(evaluate_if(self.condition, needs, workflow_cancelled=False)) + # Decision jobs succeeded, so the guard stays quiet; the report step + # owns the red on the live-run path. + self.assertFalse(evaluate_if(self.guard_if, needs, workflow_cancelled=False)) + def test_the_other_deliberate_stops_still_skip(self): unlabelled = self.fresh(**{ "gate": {"result": "success", "outputs": {"should_run": "false"}}, diff --git a/.github/workflows/cursor-review.yml b/.github/workflows/cursor-review.yml index 53979c98..e694016a 100644 --- a/.github/workflows/cursor-review.yml +++ b/.github/workflows/cursor-review.yml @@ -6828,15 +6828,34 @@ jobs: # and gated on `approve_gate`, which already fails closed; see # docs/callers/cursor-approve.md ("A red panel cell must not skip the axes"). # - # `always()`, and the same four gate conditions `consolidate` uses, so this - # reports on exactly the runs where a panel was supposed to happen and stays - # skipped on the ones where nothing was (no trigger label, already reviewed, - # capped by `max_rounds`, over the diff-size cap, panel skipped). `always()` rather than - # `!cancelled()` for `Blocking gate`'s reason: GitHub counts a SKIPPED - # required check as passing, so green-on-cancel would be a fail-open for any - # caller that requires this. A cancelled run therefore reports red here — - # under the documented `cancel-in-progress` caller that check lands on the - # superseded head SHA, not the new one. + # `!cancelled()`, and the same four gate conditions `consolidate` uses, so + # this reports on exactly the runs where a panel was supposed to happen and + # stays skipped on the ones where nothing was (no trigger label, already + # reviewed, capped by `max_rounds`, over the diff-size cap, panel skipped). + # + # `!cancelled()` rather than `always()`, for the same reason `post-review` + # documents at its own `if:` (and `over-cap-comment` before it): the + # documented caller sets `cancel-in-progress: true`, and `always()` runs a + # job even when the RUN is cancelled. A superseded run's decision jobs come + # back `cancelled`, not `failure`, and the guard below then fails this + # check — "re-run the failed job" — on a run that was torn down before it + # decided anything. That is a false failure: nothing failed, and the run + # that replaced this one is the one that reports. Skipping leaves the + # superseded run without a Panel integrity failure. The same skip covers a + # cancel that lands mid-panel: `consolidate` / `post-review` come back + # `cancelled` and the report step would otherwise fail the check for a + # panel the run was not allowed to finish. + # + # Fail-closed for a run that was NOT cancelled is unchanged, and that is + # the property the old `always()` was protecting where it still matters. + # `cancelled()` is the workflow flag, not a job result. A decision job that + # failed, or that GitHub ended for its own `timeout-minutes` (the job + # result is `cancelled`, the workflow is not, so `cancelled()` is false), + # still satisfies `!= 'success'`, this job still runs, and the guard still + # exits 1. A skipped required check passes, so this skip is a pass for the + # superseded run only — the same reading as the deliberate no-panel + # branches, not a claim the panel was whole. The Blocking gate is untouched + # and still uses `always()`, so a blocking caller stays red on cancel. # # That same "skipped counts as passing" rule is why those four conditions # cannot be read STRAIGHT off `gate`, `diff-size` and the matrix. Three of @@ -6877,7 +6896,7 @@ jobs: # hits the cap again. name: Panel integrity needs: [gate, round-cap, diff-size, preflight, ledger, review, consolidate, post-review] - if: ${{ always() && (needs.gate.result != 'success' || (needs.gate.outputs.should_run == 'true' && needs.gate.outputs.already_reviewed != 'true' && (needs.round-cap.result != 'success' || (needs.round-cap.outputs.capped != 'true' && (needs.diff-size.result != 'success' || (needs.diff-size.outputs.within_cap == 'true' && (needs.preflight.result != 'success' || needs.ledger.result != 'success' || needs.review.result != 'skipped'))))))) }} + if: ${{ !cancelled() && (needs.gate.result != 'success' || (needs.gate.outputs.should_run == 'true' && needs.gate.outputs.already_reviewed != 'true' && (needs.round-cap.result != 'success' || (needs.round-cap.outputs.capped != 'true' && (needs.diff-size.result != 'success' || (needs.diff-size.outputs.within_cap == 'true' && (needs.preflight.result != 'success' || needs.ledger.result != 'success' || needs.review.result != 'skipped'))))))) }} runs-on: ubuntu-latest # Reads job outputs and prints. No API call, no checkout, no network. timeout-minutes: 5 @@ -6921,9 +6940,12 @@ jobs: # written around the consequence. `Ensure ledger artifact exists` carries # no `continue-on-error`; the job's 20-minute cap sits above only 15 # minutes of step caps, so the two untimed steps can trip the JOB - # timeout; and a cancelled or infrastructure-lost `ledger` is neither - # 'success' nor a step outcome at all. Rare is the wrong bar for a guard - # whose failure mode is handing out a green required check. + # timeout; and an infrastructure-lost `ledger` is neither 'success' nor + # a step outcome at all. Both surface as result `cancelled` on a run + # that is still going. A workflow-level cancel never reaches this step: + # the job `if:` is `!cancelled()`, so a superseded run is skipped rather + # than judged here. Rare is the wrong bar for a guard whose failure + # mode is handing out a green required check. # # Note these are read as RESULTS, not as a reason to believe the matrix # ran: if `review` ran at all, all four succeeded, so this guard can only diff --git a/README.md b/README.md index 4580f029..0d8f991f 100644 --- a/README.md +++ b/README.md @@ -16,7 +16,7 @@ complete, copy-pasteable caller. |---|---|---| | [`detect-unreviewed-merge.yml`](.github/workflows/detect-unreviewed-merge.yml) | SOC 2 compliance — detects PRs merged without prior approval and opens a tracking issue in [`Comfy-Org/unreviewed-merges`](https://github.com/Comfy-Org/unreviewed-merges). | [detect-unreviewed-merge.md](docs/callers/detect-unreviewed-merge.md) | | [`linear-ticket.yml`](.github/workflows/linear-ticket.yml) | **Requires a Linear issue that Linear has LINKED to the exact PR when the PR targets a protected base branch** — not a `TEAM-123`-shaped string in author text. The validator reads GitHub's branch `protected` property, so any number of branches protected by branch protection or rulesets are covered without a caller-maintained list; unprotected targets publish no `Linear ticket` status and do not query Linear (a success status would be unsafe because GitHub scopes statuses to a SHA, not a PR). For protected targets, the only thing that passes is an attachment Linear returns for the PR's canonical `html_url` (`attachmentsForURL`, PR URL passed as a GraphQL **variable**) whose issue satisfies the configured team (`team-keys`, matched on the resolved API `team.key`, never a prefix) and state (`require-open-issue` rejects `completed`/`canceled` via `state.type`) policy; a PR linked to several tickets passes when at least one satisfies policy. Identifiers mined from branch/title/body are **diagnostics only** (capped at 20, one batched lookup) — they explain a red check, never turn it green. Fork- and Dependabot-safe by a two-stage split: an unprivileged, checks-out-nothing **signal** workflow on `pull_request`, then a privileged validator on `workflow_run` that GitHub grants the secret + write token. It publishes the required **`Linear ticket`** commit status on the refetched PR head SHA (its own default-branch job is *not* the merge check), refetches the head SHA and base branch before every terminal write so a superseded or retargeted run can't overwrite a newer result, maintains one marker diagnostic comment, and **fails closed** on unreadable branch protection or Linear auth/schema/timeout/rate-limit errors (an infrastructure failure, never a green pass) with bounded retry for Linear's async-link race. `enforce: false` is warn-only — a failing verdict never exits the job nonzero (a broken run still does), and by default (`soft-fail: true`) it still publishes the **red** status so the pilot is visible on the PR while blocking nothing, since blocking is branch protection requiring the context, not this workflow's call (`soft-fail: false` restores the silent always-green variant); one `exempt-label` (e.g. `linear-exempt`) waives it per-PR, and an opt-in `exempt-actors` list (empty by default) skips bot PRs (e.g. `dependabot[bot]`) with no built-in bypass. An opt-in `exempt-paths` list exempts a PR whose changed files **all** match (config-only chores arriving from many different authors); it is decided IN the validator — reading the GitHub-owned changed-file list, failing closed past that endpoint's silent 3000-file cap — precisely so an exempt PR still publishes the green required status, which a `paths-ignore:` on the signal workflow could not (an unpublished required context leaves the PR pending forever). Logic + hermetic tests live in [`scripts/linear-ticket/`](scripts/linear-ticket); `workflows_ref` is **required**, shape- and ancestry-checked before checkout. Requires `LINEAR_API_TOKEN` + the two-workflow caller. | [linear-ticket.md](docs/callers/linear-ticket.md) | -| [`cursor-review.yml`](.github/workflows/cursor-review.yml) | Label-triggered multi-model code review. A 3-lab × 2-review-type cursor-agent panel runs adversarial + edge-case passes, a judge model consolidates them into one PR review with per-finding severity badges, and the triggerer gets Slack start/complete DMs. Advisory by default: it posts a review, it does not gate. Opt-in `blocking: true` adds a fail-closed **Blocking gate** check — red while any finding thread is unresolved and non-outdated, and red whenever the round that should have produced those threads did not land (the trigger job failed, the post exited zero without reaching the PR, every finding was demoted to the review body, the diff was over `diff_size_cap`, or the run was cancelled) — which blocks merge once the caller's ruleset marks it required (two switches; see [the setup guide](docs/callers/cursor-review.md)). Separately and **always on**, a **Panel integrity** check reports whether the panel that reviewed the PR was a *whole* panel — red when fewer reviewer cells submitted than ran, when findings could not be anchored to the diff and were demoted to the review body, when no review carrying resolvable threads reached the PR, when the judge never adjudicated, or when the jobs that decide whether to review at all did not succeed (an undecided run is not a clean run). It takes no input and gates nothing by itself; ` / Panel integrity` is the stable context to mark required if you want completeness gated — **not** an individual panel leg, whose name carries the model id and changes whenever the pins move. Two consequences to read before bumping onto it: a cell that does not submit now fails its own leg instead of exiting green, so **your caller job goes red** on a measured ~40% of runs (that red is the signal — the consolidated review still posts, and the Blocking gate is unaffected); and the deliberate no-panel branches leave it *skipped*, which a required check counts as passing, so a skip answers "no panel was warranted", never "the panel was fine". Scope and limits — including forgeability and same-SHA supersession — are in [the setup guide](docs/callers/cursor-review.md). Prompts and scripts live in [`.github/cursor-review/`](.github/cursor-review) — the single source of truth, so consumer repos carry only a thin caller. Generated code is excluded from BOTH the diff-size gate and the diff sent to the panel via the shared [`check-pr-size`](scripts/check-pr-size) classifier (base-ref `linguist-generated`, base-blob Go generated markers — a PR cannot self-exempt a file from review, lockfiles, plus `extra_generated_globs` / `extra_lockfiles`) — the same detection as `pr-size.yml`, so codegen never blows the review cap or wastes review signal. Blank and comment-only lines are also discounted from the size count by default (`ignore_comments`) so AI comment volume doesn't trip the cap and skip the review — comments still reach the panel, only the count ignores them. The panel model list is fixed by the reusable — there is no `models` input (`models` is a job *output*); an optional `runs_on` routes only the credential-free heavy jobs (`diff-size`, `preflight`, panel, judge) to a caller's runner pool, while every write-credential job stays on GitHub-hosted `ubuntu-latest`. Least privilege by construction: no job both checks out PR code and holds a write-scoped credential — every job that checks out PR code and runs `cursor-agent` over it (every panel cell and the judge) holds `contents: read` only, and hands its result to a separate, checkout-free `Post review` job via an artifact; that job and the over-the-cap comment job are the only holders of `pull-requests: write`, and neither checks out PR code. Requires `CURSOR_API_KEY` (+ optional `SLACK_BOT_TOKEN`). | [cursor-review.md](docs/callers/cursor-review.md) | +| [`cursor-review.yml`](.github/workflows/cursor-review.yml) | Label-triggered multi-model code review. A 3-lab × 2-review-type cursor-agent panel runs adversarial + edge-case passes, a judge model consolidates them into one PR review with per-finding severity badges, and the triggerer gets Slack start/complete DMs. Advisory by default: it posts a review, it does not gate. Opt-in `blocking: true` adds a fail-closed **Blocking gate** check — red while any finding thread is unresolved and non-outdated, and red whenever the round that should have produced those threads did not land (the trigger job failed, the post exited zero without reaching the PR, every finding was demoted to the review body, the diff was over `diff_size_cap`, or the run was cancelled) — which blocks merge once the caller's ruleset marks it required (two switches; see [the setup guide](docs/callers/cursor-review.md)). Separately and **always on**, a **Panel integrity** check reports whether the panel that reviewed the PR was a *whole* panel — red when fewer reviewer cells submitted than ran, when findings could not be anchored to the diff and were demoted to the review body, when no review carrying resolvable threads reached the PR, when the judge never adjudicated, or when the jobs that decide whether to review at all did not succeed on a run that was not itself cancelled (an undecided run is not a clean run; a concurrency cancel skips this check rather than failing it, while a job timeout — also reported as `cancelled` — still fails closed). It takes no input and gates nothing by itself; ` / Panel integrity` is the stable context to mark required if you want completeness gated — **not** an individual panel leg, whose name carries the model id and changes whenever the pins move. Two consequences to read before bumping onto it: a cell that does not submit now fails its own leg instead of exiting green, so **your caller job goes red** on a measured ~40% of runs (that red is the signal — the consolidated review still posts, and the Blocking gate is unaffected); and the deliberate no-panel branches leave it *skipped*, which a required check counts as passing, so a skip answers "no panel was warranted", never "the panel was fine". Scope and limits — including forgeability and same-SHA supersession — are in [the setup guide](docs/callers/cursor-review.md). Prompts and scripts live in [`.github/cursor-review/`](.github/cursor-review) — the single source of truth, so consumer repos carry only a thin caller. Generated code is excluded from BOTH the diff-size gate and the diff sent to the panel via the shared [`check-pr-size`](scripts/check-pr-size) classifier (base-ref `linguist-generated`, base-blob Go generated markers — a PR cannot self-exempt a file from review, lockfiles, plus `extra_generated_globs` / `extra_lockfiles`) — the same detection as `pr-size.yml`, so codegen never blows the review cap or wastes review signal. Blank and comment-only lines are also discounted from the size count by default (`ignore_comments`) so AI comment volume doesn't trip the cap and skip the review — comments still reach the panel, only the count ignores them. The panel model list is fixed by the reusable — there is no `models` input (`models` is a job *output*); an optional `runs_on` routes only the credential-free heavy jobs (`diff-size`, `preflight`, panel, judge) to a caller's runner pool, while every write-credential job stays on GitHub-hosted `ubuntu-latest`. Least privilege by construction: no job both checks out PR code and holds a write-scoped credential — every job that checks out PR code and runs `cursor-agent` over it (every panel cell and the judge) holds `contents: read` only, and hands its result to a separate, checkout-free `Post review` job via an artifact; that job and the over-the-cap comment job are the only holders of `pull-requests: write`, and neither checks out PR code. Requires `CURSOR_API_KEY` (+ optional `SLACK_BOT_TOKEN`). | [cursor-review.md](docs/callers/cursor-review.md) | | [`cursor-approve.yml`](.github/workflows/cursor-approve.yml) + [`axis-correctness.yml`](.github/workflows/axis-correctness.yml) / [`axis-conformance.yml`](.github/workflows/axis-conformance.yml) / [`axis-business.yml`](.github/workflows/axis-business.yml) / [`axis-design.yml`](.github/workflows/axis-design.yml) / [`axis-completeness.yml`](.github/workflows/axis-completeness.yml) | Per-axis approval after a passing cursor-review round (`approve_gate == 'pass'`). Each axis is its own reusable declaring only the secrets it needs (correctness/conformance: `CURSOR_API_KEY` only; business/design/completeness add optional read-only Linear/Notion tokens, held by a local read-only MCP proxy the agent never sees the tokens of, private repos only), runs one read-only `cursor-agent` judgement over the PR head via the shared [`cursor-axis-base.yml`](.github/workflows/cursor-axis-base.yml), and uploads one verdict artifact. `cursor-approve.yml`, called with `phase: start` and `phase: decide`, keeps one live status card on the PR and approves through `auto-approve.py approve-external` (head re-read, reviewed-SHA marker, own stale approvals withdrawn, never over `needs-human-review` or `skip-cursor-review`, both re-read live). Prompts and the aggregator live in [`.github/cursor-approve/`](.github/cursor-approve). Requires `CURSOR_API_KEY` + `APPROVER_TOKEN`. | [cursor-approve.md](docs/callers/cursor-approve.md) | | [`cursor-review-auto-label.yml`](.github/workflows/cursor-review-auto-label.yml) | Companion to `cursor-review.yml`. On PR assignment, applies the review label for an opted-in reviewer (via the CLOUD_CODE_BOT app token, so the label actually triggers the review). The opt-in roster lives in the caller's `vars.CURSOR_REVIEW_OPTED_IN_LOGINS` — no roster is baked into the workflow. Requires `vars.APP_ID` + `CLOUD_CODE_BOT_PRIVATE_KEY`, or the optional `bot_app_id` + `BOT_APP_PRIVATE_KEY` pair to mint from a narrower App instead. | [cursor-review-auto-label.md](docs/callers/cursor-review-auto-label.md) | | [`assign-reviewers.yml`](.github/workflows/assign-reviewers.yml) | Assigns PR owners using configured paths and recent human approval history. Chooses complementary file coverage before workload, with an explained one-owner default fallback and no random substitutions. Optionally generates a shared history artifact on schedule/manual dispatch and reuses fresh snapshots across PRs. Reads configuration from the base SHA, respects manual assignments, and preserves opt-in `REVIEWER_AUTHOR_ALLOWLIST` scoping. Requires `vars.APP_ID` + `CLOUD_CODE_BOT_PRIVATE_KEY`; callers skip forks and Dependabot triggers. | [assign-reviewers.md](docs/callers/assign-reviewers.md) | diff --git a/docs/callers/cursor-review.md b/docs/callers/cursor-review.md index 739fe2b8..8d5f40d9 100644 --- a/docs/callers/cursor-review.md +++ b/docs/callers/cursor-review.md @@ -595,11 +595,20 @@ supersession shape below before you reach for either to clear one. The remaining shapes to expect before you require it: -* **A cancelled run reports red.** GitHub counts a *skipped* required check as - passing, so this job runs on cancellation rather than handing a superseded run - a free green — the same reasoning the Blocking gate documents. Under the - `cancel-in-progress` caller above that red lands on the head SHA that was - superseded, not on the new one. +* **A superseded run skips this check.** `cancel-in-progress` cancels the + earlier run while its decision jobs are still going; their results are + `cancelled`, not `failure`. The job used to run anyway (`always()`) and fail + with "re-run the failed job", which painted that superseded run red. Its + `if:` is now `!cancelled()`, the same choice `Post review` makes so a + cancelled run does not keep working, so the superseded run skips Panel + integrity instead of failing it. The run that replaced it is the one that + reports. A skipped required check passes, and that pass means "this run has + no verdict", the same reading as the deliberate no-panel branches below — it + does not say the panel was whole. This is only the workflow-cancelled path. + A decision job that **failed**, or that hit its own `timeout-minutes` + (GitHub reports that job result as `cancelled` without cancelling the + workflow), still runs this check and fails it. The Blocking gate is separate + and still goes red on cancellation. * **Do not require a leg check instead.** A panel cell's context name carries the model id (`edge-case (kimi-k3-max)`), so it changes whenever the panel list does — and a required check whose name no longer exists blocks every PR @@ -616,9 +625,11 @@ The remaining shapes to expect before you require it: `Round cap`, `Diff size check`, `Preflight` **or `Prior-review ledger`** runs this job and fails it: an undecided run is not a clean run. (`Round cap` sits upstream of `Diff size check`, so a red one skips the panel just the same.) (`Prior-review ledger` is built - never to fail — every step in it is `continue-on-error` — but a job timeout, - a cancellation or a lost runner is not a step outcome, and "rare" is the wrong - bar for something that would otherwise hand you a green merge gate.) + never to fail — every step in it is `continue-on-error` — but a job timeout + or a lost runner is not a step outcome. GitHub reports both as a `cancelled` + job result while the workflow itself is still running, and "rare" is the wrong + bar for something that would otherwise hand you a green merge gate. A workflow + cancelled by `cancel-in-progress` is the previous bullet, not this one.) * **It still skips when no review was warranted, and a skip is green.** The deliberate no-panel branches — no trigger label, an already-reviewed commit, a PR that hit its `max_rounds` cap, a PR over the diff-size cap, a fork the panel From 4c51389a17cc3f5e645d1cef269813b131444c3c Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Wed, 21 Oct 2026 23:59:24 +0000 Subject: [PATCH 2/2] fix(cursor-review): keep Panel integrity on always(), pass a cancel only 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 --- .github/cursor-review/README.md | 18 +- .../tests/test_workflow_panel_integrity.py | 220 +++++++++++++----- .github/workflows/cursor-review.yml | 107 +++++---- README.md | 2 +- docs/callers/cursor-review.md | 34 +-- 5 files changed, 256 insertions(+), 125 deletions(-) diff --git a/.github/cursor-review/README.md b/.github/cursor-review/README.md index 09194b59..cf9c3715 100644 --- a/.github/cursor-review/README.md +++ b/.github/cursor-review/README.md @@ -68,13 +68,13 @@ The jobs, in the order they report: | Job / check run | Runs when | Red means | |---|---|---| | `Gate` | always | The trigger decision itself failed (a label read or the dedupe API call), so whether the PR should be reviewed is unknown. | -| `Prior-review ledger` | reviewing | Designed never to fail the run: the review matrix `needs:` it, so it degrades to an empty ledger rather than erroring. Rare is not never — if it does go red (a job timeout or a lost runner, both a `cancelled` job result while the run is still going), the matrix skips and `Panel integrity` reports that rather than passing. A run cancelled by `cancel-in-progress` skips `Panel integrity` instead; that run was superseded, and the replacement reports. | +| `Prior-review ledger` | reviewing | Designed never to fail the run: the review matrix `needs:` it, so it degrades to an empty ledger rather than erroring. Rare is not never — if it does go red (a job timeout or a lost runner, both a `cancelled` job result while the run is still going), the matrix skips and `Panel integrity` reports that rather than passing. A run cancelled after a newer push passes `Panel integrity` without judging it — that head was superseded; any other cancel stays red. | | `Diff size check` | reviewing | The `BASE...HEAD` diff could not be built at all. An over-cap PR is not a failure — it is a skip plus a PR comment. | | `Preflight — validate model catalog` | reviewing | A pinned panel model is delisted. The panel is skipped rather than quietly running a lab short. | | ` ()` — one per cell | reviewing | **That cell did not submit a review.** Its artifact is still uploaded and the panel still consolidates; the leg is red so the gap reaches `statusCheckRollup`. Never require one of these: the context name carries the model id and changes whenever the panel list does. | | `Consolidate panel` | reviewing | The judge job failed outright (a hung judge is absorbed and falls back to the panel union instead). | | `Post review` | Consolidate panel succeeded | The POST failed, or succeeded without the run being able to confirm it. The findings are written to the job summary in that case — see [Delivery, the body-only fallback, and a throttled POST](#delivery-the-body-only-fallback-and-a-throttled-post). | -| `Panel integrity` | reviewing, **or** the decision to review failed, and the run was not cancelled | The panel was short, findings went unanchored, nothing was delivered, the judge never adjudicated — or `Gate`/`Round cap`/`Diff size check`/`Preflight`/`Prior-review ledger` failed, leaving whether the PR was reviewed unknown. Skipped, not green-by-verdict, when no review was warranted (including a head stopped by the `max_rounds` cap) and when `cancel-in-progress` cancelled the run. Advisory unless a caller marks it required. | +| `Panel integrity` | reviewing, **or** the decision to review failed | The panel was short, findings went unanchored, nothing was delivered, the judge never adjudicated — or `Gate`/`Round cap`/`Diff size check`/`Preflight`/`Prior-review ledger` failed, leaving whether the PR was reviewed unknown. Skipped, not green-by-verdict, when no review was warranted (including a head stopped by the `max_rounds` cap). On a cancelled run it passes only when the PR head has since moved; a cancel on an unmoved head is red. Advisory unless a caller marks it required. | | `Blocking gate` | `blocking: true` | Unresolved, non-outdated finding threads — or a round that should have produced them and did not. Opt-in. | **Post review is its own job, and that is a security boundary.** No job both @@ -117,11 +117,15 @@ read a job output (empty when the job producing it failed) and the fourth reads the matrix result, which is `skipped` whenever ANY job the matrix `needs:` (`gate`, `diff-size`, `preflight`, `ledger`) did not succeed, while a skipped required check passes: the same fail-closed guard the Blocking gate carries. -A workflow cancelled by `cancel-in-progress` is not that failure. The job's -`if:` is `!cancelled()`, so a superseded run skips this check instead of -failing it, and the run that replaced it is the one that reports. A decision -job that failed — or whose own `timeout-minutes` GitHub reports as a -`cancelled` job result without cancelling the workflow — still fails the check. +A cancelled run is told apart inside the job, not by skipping it: the `if:` +stays `always()`, because `cancelled()` is true for a manual cancel too, and a +same-SHA replacement can decline to review, so a skip there would be a green +required check with nothing behind it. On a cancelled run the first step, +`Judge a cancelled run`, re-reads the PR's live head: if it moved past the SHA +the run was reviewing, the run was superseded by a push and the job passes +without judging (a required check is read on the new head, not this one); otherwise — head unmoved, or +unreadable — the job fails closed. A decision job that failed or timed out on +a live run still fails the check through the guard. The invariant is *every* job the matrix depends on, not the four that happen to be on that list today — it was re-opened twice by a dependency being added to diff --git a/.github/cursor-review/tests/test_workflow_panel_integrity.py b/.github/cursor-review/tests/test_workflow_panel_integrity.py index f70db56f..891283d8 100644 --- a/.github/cursor-review/tests/test_workflow_panel_integrity.py +++ b/.github/cursor-review/tests/test_workflow_panel_integrity.py @@ -52,6 +52,7 @@ UPLOAD_STEP = "Upload findings artifact" PANEL_JOB = "panel-integrity" PANEL_CONTEXT = "Panel integrity" +CANCEL_STEP = "Judge a cancelled run" UNDECIDED_STEP = "Fail if the panel decision itself did not complete" REPORT_STEP = "Report panel integrity" @@ -365,23 +366,18 @@ def test_it_needs_every_job_it_reads(self): "evaluate to the empty string, which every cause treats as a pass", ) - def test_a_cancelled_run_skips_this_job(self): - # `!cancelled()`, not `always()`. `always()` runs the job even when - # cancel-in-progress has cancelled the RUN, the decision jobs come back - # `cancelled`, and the guard then fails the check ("re-run the failed - # job") on a run that was torn down before it decided anything. That is - # the false red. `post-review` and `over-cap-comment` use `!cancelled()` - # for the same interaction. - # - # A job result of `cancelled` on a run that was NOT cancelled — a - # `timeout-minutes`, which GitHub reports as `cancelled` without - # cancelling the workflow — still matches `!= 'success'` and still - # fails closed. `cancelled()` is the workflow flag, not that result. - # The Blocking gate keeps `always()`; this check does not. + def test_it_reports_on_cancellation(self): + # `always()`, not `!cancelled()`: GitHub counts a SKIPPED required check + # as PASSING, and `cancelled()` is true for EVERY workflow cancel — a + # manual one has no replacement run, and a same-SHA replacement can + # decline to review. A skip there mints a green context over a panel + # that never finished, the fail-open the blocking gate documents at its + # own `if:`. The supersede case is told apart INSIDE the job by + # `Judge a cancelled run`, not by skipping it. condition = job_scalar(self.panel, "if") self.assertIsNotNone(condition) - self.assertIn("!cancelled()", condition) - self.assertNotIn("always()", condition) + self.assertIn("always()", condition) + self.assertNotIn("cancelled()", condition) def test_it_is_gated_exactly_like_consolidate(self): # Reporting on a run where the panel was never supposed to happen — no @@ -391,11 +387,16 @@ def test_it_is_gated_exactly_like_consolidate(self): for gate in GATE_CONDITIONS: self.assertIn(gate, condition, f"`{PANEL_JOB}` lost gate `{gate}`") - def test_it_holds_no_permissions(self): + def test_it_holds_only_pull_requests_read(self): # An ABSENT block is not "no permissions": a workflow_call reusable # INHERITS the caller job's, and the documented caller grants - # `pull-requests: write`. - self.assertEqual(job_scalar(self.panel, "permissions"), "{}") + # `pull-requests: write`. The one API read (the live PR head, on a + # cancelled run) needs `pull-requests: read` and nothing more. + self.assertEqual(job_scalar(self.panel, "permissions"), "") + self.assertEqual( + block_mapping(self.panel, " permissions:", 6), + {"pull-requests": "read"}, + ) def test_it_is_bounded(self): self.assertEqual(job_scalar(self.panel, "timeout-minutes"), "5") @@ -549,11 +550,11 @@ class UndecidedRunFailsClosedTest(unittest.TestCase): whether to review the PR at all. `Blocking gate` closes the same hole with the same two guards; this suite pins that they stay closed here too. - A run cancelled by concurrency is the exception, pinned in - `RoundCapIsADecisionJobTest`: every decision job then comes back - `cancelled` because the run was torn down, and this check skips rather - than failing on a supersede. A `cancelled` job result while the workflow - is still running (`timeout-minutes`) is not that exception. + A run cancelled after a newer push is the one exception, and it is not a + skip: the job still runs, and `Judge a cancelled run` passes it only after + reading that the PR's live head moved (`CancelledRunTest`). A cancel with + the head unmoved, or a `cancelled` job result on a live run + (`timeout-minutes`), stays red. """ def setUp(self): @@ -732,12 +733,16 @@ def evaluate(expression, context, workflow_cancelled=False): return eval(py, {"__builtins__": {}}, {}) # noqa: S307 - grammar checked above -def evaluate_if(condition, needs, workflow_cancelled=False): - """Evaluate a job `if:` against `needs`, which maps a job to +def evaluate_if(condition, needs, workflow_cancelled=False, steps=None): + """Evaluate a job or step `if:` against `needs`, which maps a job to `{"result": ..., "outputs": {...}}`. A job absent from it, or an output it does not declare, reads as '' (see `evaluate`). `workflow_cancelled` is - GitHub's `cancelled()` and defaults to false.""" + GitHub's `cancelled()` and defaults to false. `steps` maps an earlier + step id to its `{output: value}`.""" context = {} + for step, outputs in (steps or {}).items(): + for name, value in outputs.items(): + context["steps.%s.outputs.%s" % (step, name)] = value for job, entry in needs.items(): context["needs.%s.result" % job] = entry.get("result", "") for name, value in entry.get("outputs", {}).items(): @@ -814,7 +819,7 @@ def test_max_rounds_off_leaves_capped_empty_and_still_reports(self): def test_a_failed_round_cap_is_red_not_skipped(self): # `cancelled` here is a job result (timeout-minutes), not a cancelled # workflow. The run is still going, so the check must run and the - # guard must fire. A workflow-level cancel is the next test. + # guard must fire. for result in ("failure", "cancelled"): needs = self.fresh(**{ "round-cap": {"result": result}, @@ -827,50 +832,56 @@ def test_a_failed_round_cap_is_red_not_skipped(self): self.assertTrue(evaluate_if(self.condition, needs), result) self.assertTrue(evaluate_if(self.guard_if, needs), result) - def test_a_superseded_run_skips_rather_than_failing(self): - # cancel-in-progress: the RUN was cancelled, so every decision job's - # result is `cancelled` even though none of them failed. The job must - # not run — a skip, not the guard's red "re-run the failed job". - # Same needs with the workflow still going (the previous test's - # `cancelled` result) must keep running; only `cancelled()` changes. + def report_if(self): + for line in code_lines(step_named(self.panel, REPORT_STEP)): + if line.strip().startswith("if:"): + return line.strip()[len("if:"):].strip() + return None + + def test_a_cancelled_run_still_runs_the_job(self): + # Every decision job `cancelled`, whether the RUN was cancelled or a + # job timed out on a live one: the job runs either way. A skip here is + # a green required check, and `cancelled()` cannot tell a supersede + # from a manual cancel. + needs = self.fresh(**{ + job: {"result": "cancelled"} + for job in ("gate", "round-cap", "diff-size", "preflight", "ledger", + "review", "consolidate", "post-review") + }) + for cancelled in (True, False): + self.assertTrue(evaluate_if(self.condition, needs, workflow_cancelled=cancelled), cancelled) + # Not superseded (the head did not move, or this is a live run): the + # guard still fires. + self.assertTrue(evaluate_if(self.guard_if, needs)) + self.assertTrue(evaluate_if(self.guard_if, needs, steps={"cancel": {}})) + + def test_a_superseded_run_stands_down_after_the_head_check(self): + # `Judge a cancelled run` set `superseded`: the guard and the report + # step both stand down, so the job ends on that step's pass. needs = self.fresh(**{ "gate": {"result": "cancelled"}, - "round-cap": {"result": "cancelled"}, - "diff-size": {"result": "cancelled"}, - "preflight": {"result": "cancelled"}, - "ledger": {"result": "cancelled"}, "review": {"result": "cancelled"}, "consolidate": {"result": "cancelled"}, - "post-review": {"result": "cancelled"}, }) - self.assertFalse( - evaluate_if(self.condition, needs, workflow_cancelled=True), - "a concurrency-cancelled run still runs Panel integrity", - ) - self.assertTrue( - evaluate_if(self.condition, needs, workflow_cancelled=False), - "the same all-cancelled results on a live run must still fail closed " - "(a timeout reports `cancelled` without cancelling the workflow)", - ) - self.assertTrue(evaluate_if(self.guard_if, needs, workflow_cancelled=False)) - - def test_a_cancel_mid_panel_skips_too(self): - # The decision jobs finished and the panel was in flight when the run - # was superseded. `review` / `consolidate` come back `cancelled`, which - # is not `skipped`, so the old `always()` condition ran the report step - # and failed the check. The workflow flag skips it; the replacement run - # reports. The same shape on a live run — a matrix cancelled without - # the workflow being cancelled — still runs the check. + superseded = {"cancel": {"superseded": "true"}} + self.assertFalse(evaluate_if(self.guard_if, needs, workflow_cancelled=True, steps=superseded)) + report_if = self.report_if() + self.assertIsNotNone(report_if, f"`{REPORT_STEP}` must stand down on a superseded run") + self.assertFalse(evaluate_if(report_if, needs, workflow_cancelled=True, steps=superseded)) + self.assertTrue(evaluate_if(report_if, needs, workflow_cancelled=True, steps={"cancel": {}})) + + def test_a_cancel_mid_panel_still_runs_the_job(self): + # Decision jobs finished and the panel was in flight. `review` / + # `consolidate` come back `cancelled`, not `skipped`, so the job runs, + # and on an unmoved head the report step owns the red. needs = self.fresh(**{ "review": {"result": "cancelled"}, "consolidate": {"result": "cancelled"}, "post-review": {"result": "skipped"}, }) - self.assertFalse(evaluate_if(self.condition, needs, workflow_cancelled=True)) - self.assertTrue(evaluate_if(self.condition, needs, workflow_cancelled=False)) - # Decision jobs succeeded, so the guard stays quiet; the report step - # owns the red on the live-run path. - self.assertFalse(evaluate_if(self.guard_if, needs, workflow_cancelled=False)) + self.assertTrue(evaluate_if(self.condition, needs, workflow_cancelled=True)) + self.assertFalse(evaluate_if(self.guard_if, needs)) + self.assertTrue(evaluate_if(self.report_if(), needs)) def test_the_other_deliberate_stops_still_skip(self): unlabelled = self.fresh(**{ @@ -1447,5 +1458,92 @@ def test_the_upload_cannot_reach_the_judge_or_fail_the_cell(self): # the aggregator would then flag the whole panel as inconsistent. self.assertEqual(step_scalar(self.step, "continue-on-error"), "true") +class CancelledRunTest(unittest.TestCase): + """`Judge a cancelled run`, EXECUTED against a stub `gh`. + + It is the only thing standing between a cancelled run and a red check, so + each way it can pass is pinned by running it: it passes ONLY when the live + PR head moved past the SHA this run reviewed. A manual cancel, a same-SHA + supersede, an event with no PR head and an unreadable head all stay red — + a skip, or a pass, there would be a green required check over a panel that + never finished. + """ + + RUN_HEAD = "a" * 40 + NEW_HEAD = "b" * 40 + + def setUp(self): + self.panel = split_jobs(read_workflow())[PANEL_JOB] + self.step = step_named(self.panel, CANCEL_STEP) + self.assertIsNotNone(self.step, f"`{PANEL_JOB}` lost its `{CANCEL_STEP}` step") + self.workdir = tempfile.mkdtemp() + self.addCleanup(shutil.rmtree, self.workdir) + + def test_it_is_first_runs_only_on_cancel_and_gates_the_rest(self): + order = step_order(self.panel) + self.assertEqual(order[0], CANCEL_STEP) + lines = [line.strip() for line in code_lines(self.step)] + self.assertIn("if: ${{ cancelled() }}", lines) + self.assertIn("id: cancel", lines) + self.assertFalse(any("continue-on-error" in line for line in lines)) + for name in (UNDECIDED_STEP, REPORT_STEP): + body = "\n".join(code_lines(step_named(self.panel, name))) + self.assertIn("steps.cancel.outputs.superseded != 'true'", body, name) + + def run_cancel(self, live_head, pr_number="7", run_head=RUN_HEAD, gh_fails=False): + bindir = os.path.join(self.workdir, "bin") + os.makedirs(bindir, exist_ok=True) + gh = os.path.join(bindir, "gh") + with open(gh, "w", encoding="utf-8") as f: + f.write("#!/bin/sh\n") + f.write('echo "$@" >> "%s/gh-calls"\n' % self.workdir) + f.write("exit 1\n" if gh_fails else "echo '%s'\n" % live_head) + os.chmod(gh, 0o755) + output = os.path.join(self.workdir, "github-output") + open(output, "w", encoding="utf-8").close() + context = { + "github.token": "t", + "github.repository": "o/r", + "github.event.pull_request.number": pr_number, + "github.event.pull_request.head.sha": run_head, + } + env = {"PATH": bindir + os.pathsep + os.environ.get("PATH", ""), "GITHUB_OUTPUT": output} + for name, value in block_mapping(self.step, " env:", 10).items(): + env[name] = render(value, context) + done = subprocess.run( + [shutil.which("bash"), "-e", "-c", run_block(self.step)], + env=env, cwd=self.workdir, capture_output=True, text=True, timeout=60, + ) + with open(output, encoding="utf-8") as f: + return done, f.read() + + def test_a_moved_head_passes_and_marks_superseded(self): + done, output = self.run_cancel(self.NEW_HEAD) + self.assertEqual(done.returncode, 0, done.stdout + done.stderr) + self.assertIn("superseded=true", output) + with open(os.path.join(self.workdir, "gh-calls"), encoding="utf-8") as f: + self.assertIn("repos/o/r/pulls/7", f.read()) + + def test_an_unmoved_head_fails(self): + # A manual cancel, or a same-SHA supersede: nothing is known to report + # on this head, so the check stays red. + done, output = self.run_cancel(self.RUN_HEAD) + self.assertEqual(done.returncode, 1) + self.assertIn("::error::", done.stdout) + self.assertNotIn("superseded", output) + + def test_an_unreadable_head_fails(self): + for kwargs in ({"gh_fails": True}, {"live_head": ""}): + done, output = self.run_cancel(kwargs.pop("live_head", self.NEW_HEAD), **kwargs) + self.assertEqual(done.returncode, 1, kwargs) + self.assertNotIn("superseded", output) + + def test_no_pull_request_head_fails(self): + for kwargs in ({"pr_number": ""}, {"run_head": ""}): + done, output = self.run_cancel(self.NEW_HEAD, **kwargs) + self.assertEqual(done.returncode, 1, kwargs) + self.assertNotIn("superseded", output) + + if __name__ == "__main__": unittest.main(verbosity=2) diff --git a/.github/workflows/cursor-review.yml b/.github/workflows/cursor-review.yml index e694016a..a5661710 100644 --- a/.github/workflows/cursor-review.yml +++ b/.github/workflows/cursor-review.yml @@ -6828,34 +6828,30 @@ jobs: # and gated on `approve_gate`, which already fails closed; see # docs/callers/cursor-approve.md ("A red panel cell must not skip the axes"). # - # `!cancelled()`, and the same four gate conditions `consolidate` uses, so - # this reports on exactly the runs where a panel was supposed to happen and - # stays skipped on the ones where nothing was (no trigger label, already - # reviewed, capped by `max_rounds`, over the diff-size cap, panel skipped). + # `always()`, and the same four gate conditions `consolidate` uses, so this + # reports on exactly the runs where a panel was supposed to happen and stays + # skipped on the ones where nothing was (no trigger label, already reviewed, + # capped by `max_rounds`, over the diff-size cap, panel skipped). # - # `!cancelled()` rather than `always()`, for the same reason `post-review` - # documents at its own `if:` (and `over-cap-comment` before it): the - # documented caller sets `cancel-in-progress: true`, and `always()` runs a - # job even when the RUN is cancelled. A superseded run's decision jobs come - # back `cancelled`, not `failure`, and the guard below then fails this - # check — "re-run the failed job" — on a run that was torn down before it - # decided anything. That is a false failure: nothing failed, and the run - # that replaced this one is the one that reports. Skipping leaves the - # superseded run without a Panel integrity failure. The same skip covers a - # cancel that lands mid-panel: `consolidate` / `post-review` come back - # `cancelled` and the report step would otherwise fail the check for a - # panel the run was not allowed to finish. + # `always()` rather than `!cancelled()` for `Blocking gate`'s reason: GitHub + # counts a SKIPPED required check as passing, and `cancelled()` is true for + # EVERY workflow cancel, not just a `cancel-in-progress` supersede. A manual + # cancel (UI, API, `gh run cancel`, which anyone with Actions write can do) + # has no replacement run at all, and a same-SHA replacement can itself + # decline to review (label removed, `skip-cursor-review`). A `!cancelled()` + # skip would hand that head SHA a green completeness gate over a panel that + # never finished. # - # Fail-closed for a run that was NOT cancelled is unchanged, and that is - # the property the old `always()` was protecting where it still matters. - # `cancelled()` is the workflow flag, not a job result. A decision job that - # failed, or that GitHub ended for its own `timeout-minutes` (the job - # result is `cancelled`, the workflow is not, so `cancelled()` is false), - # still satisfies `!= 'success'`, this job still runs, and the guard still - # exits 1. A skipped required check passes, so this skip is a pass for the - # superseded run only — the same reading as the deliberate no-panel - # branches, not a claim the panel was whole. The Blocking gate is untouched - # and still uses `always()`, so a blocking caller stays red on cancel. + # The supersede case is still not red, though: `Judge a cancelled run` + # below runs only when the run was cancelled and re-reads the PR's LIVE + # head. If it moved past the SHA this run was reviewing, a newer push + # superseded it — this check sits on a head nobody can merge any more, the + # new head is judged by its own checks, and the job passes without judging. If the + # head did NOT move (or cannot be read), nothing is known to have replaced + # this run on this SHA, so the job fails closed exactly as before. A job + # that hit its own `timeout-minutes` lands on the same fail-closed path + # whichever way GitHub reports `cancelled()` for it: either the guard + # reads its `cancelled` result, or the head check finds the head unmoved. # # That same "skipped counts as passing" rule is why those four conditions # cannot be read STRAIGHT off `gate`, `diff-size` and the matrix. Three of @@ -6896,16 +6892,47 @@ jobs: # hits the cap again. name: Panel integrity needs: [gate, round-cap, diff-size, preflight, ledger, review, consolidate, post-review] - if: ${{ !cancelled() && (needs.gate.result != 'success' || (needs.gate.outputs.should_run == 'true' && needs.gate.outputs.already_reviewed != 'true' && (needs.round-cap.result != 'success' || (needs.round-cap.outputs.capped != 'true' && (needs.diff-size.result != 'success' || (needs.diff-size.outputs.within_cap == 'true' && (needs.preflight.result != 'success' || needs.ledger.result != 'success' || needs.review.result != 'skipped'))))))) }} + if: ${{ always() && (needs.gate.result != 'success' || (needs.gate.outputs.should_run == 'true' && needs.gate.outputs.already_reviewed != 'true' && (needs.round-cap.result != 'success' || (needs.round-cap.outputs.capped != 'true' && (needs.diff-size.result != 'success' || (needs.diff-size.outputs.within_cap == 'true' && (needs.preflight.result != 'success' || needs.ledger.result != 'success' || needs.review.result != 'skipped'))))))) }} runs-on: ubuntu-latest - # Reads job outputs and prints. No API call, no checkout, no network. + # Reads job outputs and prints. No checkout. One API read (the live PR + # head), and only on a cancelled run. timeout-minutes: 5 - # Nothing at all: this job reads `needs` outputs and writes annotations. - # An empty block is not the same as an absent one — a `workflow_call` - # reusable with no `permissions:` INHERITS the caller job's, and the - # documented caller grants `pull-requests: write`. - permissions: {} + # `pull-requests: read` for that one read and nothing else. Never leave the + # block absent — a `workflow_call` reusable with no `permissions:` INHERITS + # the caller job's, and the documented caller grants `pull-requests: write`. + permissions: + pull-requests: read steps: + - name: Judge a cancelled run + # Only on a cancelled run; see the job comment. Passes ONLY when the + # PR's live head has moved past the SHA this run was reviewing, and + # then sets `superseded` so the two steps below stand down. Every other + # outcome — head unchanged, no PR head in the event, the read failing — + # exits 1, which skips the steps below and leaves the check red. + id: cancel + if: ${{ cancelled() }} + env: + GH_TOKEN: ${{ github.token }} + REPO: ${{ github.repository }} + PR_NUMBER: ${{ github.event.pull_request.number }} + RUN_HEAD: ${{ github.event.pull_request.head.sha }} + run: | + if [ -z "$PR_NUMBER" ] || [ -z "$RUN_HEAD" ]; then + echo "::error::Panel integrity: this run was cancelled and carries no pull-request head to compare, so nothing shows it was superseded. Re-run the workflow." + exit 1 + fi + live_head=$(gh api "repos/${REPO}/pulls/${PR_NUMBER}" --jq '.head.sha') || live_head="" + if [ -z "$live_head" ]; then + echo "::error::Panel integrity: this run was cancelled and the PR's current head could not be read, so whether a newer push superseded it is unknown. Re-run the workflow." + exit 1 + fi + if [ "$live_head" = "$RUN_HEAD" ]; then + echo "::error::Panel integrity: this run was cancelled before its panel finished, and the PR head is still ${RUN_HEAD} — nothing has superseded it. Re-run the workflow, or re-apply the review label." + exit 1 + fi + echo "::notice::Panel integrity: this run was cancelled after the PR head moved from ${RUN_HEAD} to ${live_head}. Required checks are read on the newer head; nothing is judged here." + echo "superseded=true" >> "$GITHUB_OUTPUT" + - name: Fail if the panel decision itself did not complete # Fail-closed guard, and it has to come FIRST because every cause in the # step below is phrased in terms of job outputs that are EMPTY when the @@ -6940,17 +6967,16 @@ jobs: # written around the consequence. `Ensure ledger artifact exists` carries # no `continue-on-error`; the job's 20-minute cap sits above only 15 # minutes of step caps, so the two untimed steps can trip the JOB - # timeout; and an infrastructure-lost `ledger` is neither 'success' nor - # a step outcome at all. Both surface as result `cancelled` on a run - # that is still going. A workflow-level cancel never reaches this step: - # the job `if:` is `!cancelled()`, so a superseded run is skipped rather - # than judged here. Rare is the wrong bar for a guard whose failure - # mode is handing out a green required check. + # timeout; and a cancelled or infrastructure-lost `ledger` is neither + # 'success' nor a step outcome at all. (A run cancelled after a newer + # push stands down in `Judge a cancelled run` and never gets here.) + # Rare is the wrong bar for a guard whose failure mode is handing out a + # green required check. # # Note these are read as RESULTS, not as a reason to believe the matrix # ran: if `review` ran at all, all four succeeded, so this guard can only # fire on runs where the panel never started. - if: ${{ needs.gate.result != 'success' || needs.round-cap.result != 'success' || needs.diff-size.result != 'success' || needs.preflight.result != 'success' || needs.ledger.result != 'success' }} + if: ${{ steps.cancel.outputs.superseded != 'true' && (needs.gate.result != 'success' || needs.round-cap.result != 'success' || needs.diff-size.result != 'success' || needs.preflight.result != 'success' || needs.ledger.result != 'success') }} env: GATE_RESULT: ${{ needs.gate.result }} ROUND_CAP_RESULT: ${{ needs.round-cap.result }} @@ -6962,6 +6988,7 @@ jobs: exit 1 - name: Report panel integrity + if: ${{ steps.cancel.outputs.superseded != 'true' }} env: OK_COUNT: ${{ needs.consolidate.outputs.ok_count }} TOTAL: ${{ needs.consolidate.outputs.total }} diff --git a/README.md b/README.md index 0d8f991f..5fca45bb 100644 --- a/README.md +++ b/README.md @@ -16,7 +16,7 @@ complete, copy-pasteable caller. |---|---|---| | [`detect-unreviewed-merge.yml`](.github/workflows/detect-unreviewed-merge.yml) | SOC 2 compliance — detects PRs merged without prior approval and opens a tracking issue in [`Comfy-Org/unreviewed-merges`](https://github.com/Comfy-Org/unreviewed-merges). | [detect-unreviewed-merge.md](docs/callers/detect-unreviewed-merge.md) | | [`linear-ticket.yml`](.github/workflows/linear-ticket.yml) | **Requires a Linear issue that Linear has LINKED to the exact PR when the PR targets a protected base branch** — not a `TEAM-123`-shaped string in author text. The validator reads GitHub's branch `protected` property, so any number of branches protected by branch protection or rulesets are covered without a caller-maintained list; unprotected targets publish no `Linear ticket` status and do not query Linear (a success status would be unsafe because GitHub scopes statuses to a SHA, not a PR). For protected targets, the only thing that passes is an attachment Linear returns for the PR's canonical `html_url` (`attachmentsForURL`, PR URL passed as a GraphQL **variable**) whose issue satisfies the configured team (`team-keys`, matched on the resolved API `team.key`, never a prefix) and state (`require-open-issue` rejects `completed`/`canceled` via `state.type`) policy; a PR linked to several tickets passes when at least one satisfies policy. Identifiers mined from branch/title/body are **diagnostics only** (capped at 20, one batched lookup) — they explain a red check, never turn it green. Fork- and Dependabot-safe by a two-stage split: an unprivileged, checks-out-nothing **signal** workflow on `pull_request`, then a privileged validator on `workflow_run` that GitHub grants the secret + write token. It publishes the required **`Linear ticket`** commit status on the refetched PR head SHA (its own default-branch job is *not* the merge check), refetches the head SHA and base branch before every terminal write so a superseded or retargeted run can't overwrite a newer result, maintains one marker diagnostic comment, and **fails closed** on unreadable branch protection or Linear auth/schema/timeout/rate-limit errors (an infrastructure failure, never a green pass) with bounded retry for Linear's async-link race. `enforce: false` is warn-only — a failing verdict never exits the job nonzero (a broken run still does), and by default (`soft-fail: true`) it still publishes the **red** status so the pilot is visible on the PR while blocking nothing, since blocking is branch protection requiring the context, not this workflow's call (`soft-fail: false` restores the silent always-green variant); one `exempt-label` (e.g. `linear-exempt`) waives it per-PR, and an opt-in `exempt-actors` list (empty by default) skips bot PRs (e.g. `dependabot[bot]`) with no built-in bypass. An opt-in `exempt-paths` list exempts a PR whose changed files **all** match (config-only chores arriving from many different authors); it is decided IN the validator — reading the GitHub-owned changed-file list, failing closed past that endpoint's silent 3000-file cap — precisely so an exempt PR still publishes the green required status, which a `paths-ignore:` on the signal workflow could not (an unpublished required context leaves the PR pending forever). Logic + hermetic tests live in [`scripts/linear-ticket/`](scripts/linear-ticket); `workflows_ref` is **required**, shape- and ancestry-checked before checkout. Requires `LINEAR_API_TOKEN` + the two-workflow caller. | [linear-ticket.md](docs/callers/linear-ticket.md) | -| [`cursor-review.yml`](.github/workflows/cursor-review.yml) | Label-triggered multi-model code review. A 3-lab × 2-review-type cursor-agent panel runs adversarial + edge-case passes, a judge model consolidates them into one PR review with per-finding severity badges, and the triggerer gets Slack start/complete DMs. Advisory by default: it posts a review, it does not gate. Opt-in `blocking: true` adds a fail-closed **Blocking gate** check — red while any finding thread is unresolved and non-outdated, and red whenever the round that should have produced those threads did not land (the trigger job failed, the post exited zero without reaching the PR, every finding was demoted to the review body, the diff was over `diff_size_cap`, or the run was cancelled) — which blocks merge once the caller's ruleset marks it required (two switches; see [the setup guide](docs/callers/cursor-review.md)). Separately and **always on**, a **Panel integrity** check reports whether the panel that reviewed the PR was a *whole* panel — red when fewer reviewer cells submitted than ran, when findings could not be anchored to the diff and were demoted to the review body, when no review carrying resolvable threads reached the PR, when the judge never adjudicated, or when the jobs that decide whether to review at all did not succeed on a run that was not itself cancelled (an undecided run is not a clean run; a concurrency cancel skips this check rather than failing it, while a job timeout — also reported as `cancelled` — still fails closed). It takes no input and gates nothing by itself; ` / Panel integrity` is the stable context to mark required if you want completeness gated — **not** an individual panel leg, whose name carries the model id and changes whenever the pins move. Two consequences to read before bumping onto it: a cell that does not submit now fails its own leg instead of exiting green, so **your caller job goes red** on a measured ~40% of runs (that red is the signal — the consolidated review still posts, and the Blocking gate is unaffected); and the deliberate no-panel branches leave it *skipped*, which a required check counts as passing, so a skip answers "no panel was warranted", never "the panel was fine". Scope and limits — including forgeability and same-SHA supersession — are in [the setup guide](docs/callers/cursor-review.md). Prompts and scripts live in [`.github/cursor-review/`](.github/cursor-review) — the single source of truth, so consumer repos carry only a thin caller. Generated code is excluded from BOTH the diff-size gate and the diff sent to the panel via the shared [`check-pr-size`](scripts/check-pr-size) classifier (base-ref `linguist-generated`, base-blob Go generated markers — a PR cannot self-exempt a file from review, lockfiles, plus `extra_generated_globs` / `extra_lockfiles`) — the same detection as `pr-size.yml`, so codegen never blows the review cap or wastes review signal. Blank and comment-only lines are also discounted from the size count by default (`ignore_comments`) so AI comment volume doesn't trip the cap and skip the review — comments still reach the panel, only the count ignores them. The panel model list is fixed by the reusable — there is no `models` input (`models` is a job *output*); an optional `runs_on` routes only the credential-free heavy jobs (`diff-size`, `preflight`, panel, judge) to a caller's runner pool, while every write-credential job stays on GitHub-hosted `ubuntu-latest`. Least privilege by construction: no job both checks out PR code and holds a write-scoped credential — every job that checks out PR code and runs `cursor-agent` over it (every panel cell and the judge) holds `contents: read` only, and hands its result to a separate, checkout-free `Post review` job via an artifact; that job and the over-the-cap comment job are the only holders of `pull-requests: write`, and neither checks out PR code. Requires `CURSOR_API_KEY` (+ optional `SLACK_BOT_TOKEN`). | [cursor-review.md](docs/callers/cursor-review.md) | +| [`cursor-review.yml`](.github/workflows/cursor-review.yml) | Label-triggered multi-model code review. A 3-lab × 2-review-type cursor-agent panel runs adversarial + edge-case passes, a judge model consolidates them into one PR review with per-finding severity badges, and the triggerer gets Slack start/complete DMs. Advisory by default: it posts a review, it does not gate. Opt-in `blocking: true` adds a fail-closed **Blocking gate** check — red while any finding thread is unresolved and non-outdated, and red whenever the round that should have produced those threads did not land (the trigger job failed, the post exited zero without reaching the PR, every finding was demoted to the review body, the diff was over `diff_size_cap`, or the run was cancelled) — which blocks merge once the caller's ruleset marks it required (two switches; see [the setup guide](docs/callers/cursor-review.md)). Separately and **always on**, a **Panel integrity** check reports whether the panel that reviewed the PR was a *whole* panel — red when fewer reviewer cells submitted than ran, when findings could not be anchored to the diff and were demoted to the review body, when no review carrying resolvable threads reached the PR, when the judge never adjudicated, or when the jobs that decide whether to review at all did not succeed (an undecided run is not a clean run; a run cancelled after a newer push passes it without judging, while any other cancel stays red). It takes no input and gates nothing by itself; ` / Panel integrity` is the stable context to mark required if you want completeness gated — **not** an individual panel leg, whose name carries the model id and changes whenever the pins move. Two consequences to read before bumping onto it: a cell that does not submit now fails its own leg instead of exiting green, so **your caller job goes red** on a measured ~40% of runs (that red is the signal — the consolidated review still posts, and the Blocking gate is unaffected); and the deliberate no-panel branches leave it *skipped*, which a required check counts as passing, so a skip answers "no panel was warranted", never "the panel was fine". Scope and limits — including forgeability and same-SHA supersession — are in [the setup guide](docs/callers/cursor-review.md). Prompts and scripts live in [`.github/cursor-review/`](.github/cursor-review) — the single source of truth, so consumer repos carry only a thin caller. Generated code is excluded from BOTH the diff-size gate and the diff sent to the panel via the shared [`check-pr-size`](scripts/check-pr-size) classifier (base-ref `linguist-generated`, base-blob Go generated markers — a PR cannot self-exempt a file from review, lockfiles, plus `extra_generated_globs` / `extra_lockfiles`) — the same detection as `pr-size.yml`, so codegen never blows the review cap or wastes review signal. Blank and comment-only lines are also discounted from the size count by default (`ignore_comments`) so AI comment volume doesn't trip the cap and skip the review — comments still reach the panel, only the count ignores them. The panel model list is fixed by the reusable — there is no `models` input (`models` is a job *output*); an optional `runs_on` routes only the credential-free heavy jobs (`diff-size`, `preflight`, panel, judge) to a caller's runner pool, while every write-credential job stays on GitHub-hosted `ubuntu-latest`. Least privilege by construction: no job both checks out PR code and holds a write-scoped credential — every job that checks out PR code and runs `cursor-agent` over it (every panel cell and the judge) holds `contents: read` only, and hands its result to a separate, checkout-free `Post review` job via an artifact; that job and the over-the-cap comment job are the only holders of `pull-requests: write`, and neither checks out PR code. Requires `CURSOR_API_KEY` (+ optional `SLACK_BOT_TOKEN`). | [cursor-review.md](docs/callers/cursor-review.md) | | [`cursor-approve.yml`](.github/workflows/cursor-approve.yml) + [`axis-correctness.yml`](.github/workflows/axis-correctness.yml) / [`axis-conformance.yml`](.github/workflows/axis-conformance.yml) / [`axis-business.yml`](.github/workflows/axis-business.yml) / [`axis-design.yml`](.github/workflows/axis-design.yml) / [`axis-completeness.yml`](.github/workflows/axis-completeness.yml) | Per-axis approval after a passing cursor-review round (`approve_gate == 'pass'`). Each axis is its own reusable declaring only the secrets it needs (correctness/conformance: `CURSOR_API_KEY` only; business/design/completeness add optional read-only Linear/Notion tokens, held by a local read-only MCP proxy the agent never sees the tokens of, private repos only), runs one read-only `cursor-agent` judgement over the PR head via the shared [`cursor-axis-base.yml`](.github/workflows/cursor-axis-base.yml), and uploads one verdict artifact. `cursor-approve.yml`, called with `phase: start` and `phase: decide`, keeps one live status card on the PR and approves through `auto-approve.py approve-external` (head re-read, reviewed-SHA marker, own stale approvals withdrawn, never over `needs-human-review` or `skip-cursor-review`, both re-read live). Prompts and the aggregator live in [`.github/cursor-approve/`](.github/cursor-approve). Requires `CURSOR_API_KEY` + `APPROVER_TOKEN`. | [cursor-approve.md](docs/callers/cursor-approve.md) | | [`cursor-review-auto-label.yml`](.github/workflows/cursor-review-auto-label.yml) | Companion to `cursor-review.yml`. On PR assignment, applies the review label for an opted-in reviewer (via the CLOUD_CODE_BOT app token, so the label actually triggers the review). The opt-in roster lives in the caller's `vars.CURSOR_REVIEW_OPTED_IN_LOGINS` — no roster is baked into the workflow. Requires `vars.APP_ID` + `CLOUD_CODE_BOT_PRIVATE_KEY`, or the optional `bot_app_id` + `BOT_APP_PRIVATE_KEY` pair to mint from a narrower App instead. | [cursor-review-auto-label.md](docs/callers/cursor-review-auto-label.md) | | [`assign-reviewers.yml`](.github/workflows/assign-reviewers.yml) | Assigns PR owners using configured paths and recent human approval history. Chooses complementary file coverage before workload, with an explained one-owner default fallback and no random substitutions. Optionally generates a shared history artifact on schedule/manual dispatch and reuses fresh snapshots across PRs. Reads configuration from the base SHA, respects manual assignments, and preserves opt-in `REVIEWER_AUTHOR_ALLOWLIST` scoping. Requires `vars.APP_ID` + `CLOUD_CODE_BOT_PRIVATE_KEY`; callers skip forks and Dependabot triggers. | [assign-reviewers.md](docs/callers/assign-reviewers.md) | diff --git a/docs/callers/cursor-review.md b/docs/callers/cursor-review.md index 8d5f40d9..317d30a3 100644 --- a/docs/callers/cursor-review.md +++ b/docs/callers/cursor-review.md @@ -595,20 +595,22 @@ supersession shape below before you reach for either to clear one. The remaining shapes to expect before you require it: -* **A superseded run skips this check.** `cancel-in-progress` cancels the - earlier run while its decision jobs are still going; their results are - `cancelled`, not `failure`. The job used to run anyway (`always()`) and fail - with "re-run the failed job", which painted that superseded run red. Its - `if:` is now `!cancelled()`, the same choice `Post review` makes so a - cancelled run does not keep working, so the superseded run skips Panel - integrity instead of failing it. The run that replaced it is the one that - reports. A skipped required check passes, and that pass means "this run has - no verdict", the same reading as the deliberate no-panel branches below — it - does not say the panel was whole. This is only the workflow-cancelled path. - A decision job that **failed**, or that hit its own `timeout-minutes` - (GitHub reports that job result as `cancelled` without cancelling the - workflow), still runs this check and fails it. The Blocking gate is separate - and still goes red on cancellation. +* **A cancelled run passes only if the PR head has moved.** The job still + runs on a cancelled run (`always()`): GitHub counts a *skipped* required + check as passing, and a cancel is not always a supersede — a manual cancel + from the UI, the API or `gh run cancel` has no replacement run, and a + same-SHA replacement can decline to review (the label removed, + `skip-cursor-review` added). So on a cancelled run it first re-reads the + PR's live head. If a newer push moved it past the SHA this run was + reviewing, the check passes without judging: it sits on a head nobody can + merge, and required checks are read on the new head. If the head did **not** move, or + cannot be read, the check fails — re-run the workflow or re-apply the + review label. That includes a same-SHA re-trigger under `cancel-in-progress` + (re-applying the label while a panel is running): the cancelled run goes + red on that SHA, and the replacement run's later Panel integrity result is + the one to read. A decision job that **failed** or timed out on a run that + was not cancelled still fails the check through the guard. The Blocking gate + is separate and still goes red on cancellation. * **Do not require a leg check instead.** A panel cell's context name carries the model id (`edge-case (kimi-k3-max)`), so it changes whenever the panel list does — and a required check whose name no longer exists blocks every PR @@ -628,8 +630,8 @@ The remaining shapes to expect before you require it: never to fail — every step in it is `continue-on-error` — but a job timeout or a lost runner is not a step outcome. GitHub reports both as a `cancelled` job result while the workflow itself is still running, and "rare" is the wrong - bar for something that would otherwise hand you a green merge gate. A workflow - cancelled by `cancel-in-progress` is the previous bullet, not this one.) + bar for something that would otherwise hand you a green merge gate. A + cancelled workflow is the previous bullet, not this one.) * **It still skips when no review was warranted, and a skip is green.** The deliberate no-panel branches — no trigger label, an already-reviewed commit, a PR that hit its `max_rounds` cap, a PR over the diff-size cap, a fork the panel