diff --git a/.github/cursor-review/README.md b/.github/cursor-review/README.md index 51327c6f..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 (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 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 | 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 | 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,6 +117,16 @@ 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 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 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..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" @@ -367,13 +368,16 @@ def test_it_needs_every_job_it_reads(self): 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:`. + # 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("always()", condition) - self.assertNotIn("!cancelled()", 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 @@ -383,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") @@ -529,7 +538,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 +549,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 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): @@ -684,9 +700,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 +711,43 @@ 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): - """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`).""" + does not declare, reads as '' (see `evaluate`). `workflow_cancelled` is + 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(): 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 +817,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. for result in ("failure", "cancelled"): needs = self.fresh(**{ "round-cap": {"result": result}, @@ -798,6 +832,57 @@ 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 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"}, + "review": {"result": "cancelled"}, + "consolidate": {"result": "cancelled"}, + }) + 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.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(**{ "gate": {"result": "success", "outputs": {"should_run": "false"}}, @@ -1373,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 53979c98..a5661710 100644 --- a/.github/workflows/cursor-review.yml +++ b/.github/workflows/cursor-review.yml @@ -6831,12 +6831,27 @@ jobs: # `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. + # 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, 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. + # + # 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 @@ -6879,14 +6894,45 @@ jobs: 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'))))))) }} 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 @@ -6922,13 +6968,15 @@ jobs: # 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. + # '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 }} @@ -6940,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 4580f029..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 (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 (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 739fe2b8..317d30a3 100644 --- a/docs/callers/cursor-review.md +++ b/docs/callers/cursor-review.md @@ -595,11 +595,22 @@ 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 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 @@ -616,9 +627,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 + 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