diff --git a/.github/cursor-review/README.md b/.github/cursor-review/README.md index d30461d..d35bd4d 100644 --- a/.github/cursor-review/README.md +++ b/.github/cursor-review/README.md @@ -56,6 +56,27 @@ diff-size cap](#over-the-diff-size-cap). With `blocking: true` a final thread is unresolved — see [Optional: make the review blocking](#optional-make-the-review-blocking). +A **Panel integrity** job follows Post review on every run, with no input to +turn it on, and is the check-run an automated merge gate should read for *"was +the panel whole?"* — it reports on every run where a panel was meant to happen, +including the ones where deciding that failed, and stays skipped (so, for a +required check, green) on the ones that deliberately review nothing. See +[Panel integrity](#panel-integrity). + +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. | +| `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`/`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. 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 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's @@ -71,6 +92,57 @@ a fresh runner with a fresh pinned checkout there is nothing tampered left for t minted token to meet. `tests/test_workflow_job_isolation.py` pins the property, and [`pr-size.yml`](../workflows/pr-size.yml) uses the identical split for its comment job. +### Panel integrity + +`Panel integrity` exists because none of the facts above used to reach a +check-run *conclusion*, which is the only surface an automated merge gate reads. +A cell that never submitted left the pre-seeded `status=error` artifact and +exited green; `Aggregate panel findings` reported `Panel: 2/6 cells contributed +findings.` into a log; [`post-review.py`](post-review.py) emitted +`ungated_findings=` as a job output nothing consumed. Measured on one consumer +repo over 92 panel runs, 38 runs had at least one errored leg and 52 of 552 cells +errored — and every leg check in every one of those runs reported `success` +(BE-15554). + +Two changes answer that. Each panel cell now **fails its own leg** when its +artifact does not come back `status=ok`, in a step deliberately placed *after* +the artifact upload so the panel keeps consolidating and the review still posts. +And one `Panel integrity` job reads the panel-level facts back off +`consolidate`'s and `post-review`'s job outputs and goes red on any of: fewer +cells submitted than ran; findings demoted to the review body with no thread; +no review delivered; the judge never adjudicated; `consolidate` or `post-review` +did not succeed at all. It also goes red — rather than skipping — when any job +the panel's decision rests on *failed*, because three of those gate conditions +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. +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 +other. + +What it is **not** is an attestation that six independent reviews happened. A +leg's "did it submit" verdict is the `status` its own `--trust` agent wrote into +`findings.json`, so a prompt-injected cell can green itself; this is an +availability signal against stalls, crashes and step caps — which is what every +observed failure has been — and a forgery-resistant count would need the +submission recorded outside the cell's own writable job. Otherwise it prints one +`::notice::Panel integrity: / cells, anchored finding(s), 0 +unanchored.` It gates no other job — a short panel must not also cost the PR the +findings it did produce — so blocking on it is the caller's call, exactly like +the Blocking gate. + +Nor is a red verdict durable yet. The job is gated on a panel being warranted, +so a later run on the **same head SHA** that takes a deliberate no-panel branch +— a toggled label, or the already-reviewed re-trigger — skips it, and that +newer skipped check run supersedes the red for branch protection, which counts +a skip as a pass. Closing it means reading prior state from outside the run +(the previous conclusion, or the landed review's cell count), which this job +deliberately holds no credential for; tracked as BE-15604, and documented as a +caveat in [the setup +guide](../../docs/callers/cursor-review.md#panel-integrity). + ### Delivery, the body-only fallback, and a throttled POST The review reaches the PR as one `POST /pulls/{n}/reviews`. When GitHub rejects that request over an inline position, the run retries **once** without anchors — the same findings as prose, in one body-only review — and when even that fails, or the run's token cannot write to the PR at all, the review is written to the job summary instead so the findings are never lost outright. A nonzero response is not proof nothing was written, though: GitHub answers a rate limit, a secondary rate limit or abuse detection with **429 or 403** as readily on a request it went on to *serve* as on one it refused. So before the retry the run asks the PR whether the first review actually landed. A confirmed **present** answer is reported as delivered and nothing more is written; a confirmed **absent** posts the fallback. @@ -118,7 +190,9 @@ MCP server. Model prose is never parsed for results: tool schemas validate the records before writing them, which removes formatting drift, markdown fences, truncated JSON, and reformat retries from the result path. If a cell fails (checkout, agent, or tool submission), it still shows up in the panel summary -tagged `error` rather than silently vanishing. +tagged `error` rather than silently vanishing — and, since BE-15554, its own leg +check goes red so the gap is visible in the PR's status rollup and not only in +the consolidated review's panel table. See [Panel integrity](#panel-integrity). ## What's in this directory diff --git a/.github/cursor-review/tests/test_workflow_panel_integrity.py b/.github/cursor-review/tests/test_workflow_panel_integrity.py new file mode 100644 index 0000000..83df238 --- /dev/null +++ b/.github/cursor-review/tests/test_workflow_panel_integrity.py @@ -0,0 +1,696 @@ +#!/usr/bin/env python3 +"""Structural regression tests for cursor-review.yml's panel-integrity signals. + +A reviewer leg whose `cursor-agent` never submits — overwhelmingly the +15-minute `Run cursor review` step cap, absorbed by its `continue-on-error` — +used to leave the pre-seeded `{"status": "error"}` artifact and exit GREEN. +`Aggregate panel findings` counted it (`Panel: 2/6 cells contributed findings.`) +and only short-circuited at zero, and `post-review.py` demoted findings it could +not anchor to the review body and emitted `ungated_findings=` as a job output +nothing consumed. None of that reached a CHECK-RUN CONCLUSION, which is the only +surface an automated merge gate reads: measured on one consumer repo over 92 +panel runs, 38 runs had at least one errored leg, 52 of 552 cells errored, and +every leg check in every one of those runs reported `success` (BE-15554). + +Two jobs answer that now and neither is visible in a diff — a deleted step, a +`continue-on-error: true` added to the leg check, a `needs:` entry dropped from +`panel-integrity`, or a fifth gate condition appearing on its `if:` would each +leave a workflow that parses, lints and runs, and would each restore a green +rollup over a short panel. So the shape is pinned here. + +Deliberately parsed WITHOUT PyYAML, like its sibling suites: this repo is +stdlib-only and CI installs no requirements for these tests, so a `yaml` import +would simply not run. The workflow is uniformly 2-space indented, which is all +the block splitter below needs. + +Run: python3 .github/cursor-review/tests/test_workflow_panel_integrity.py +""" + +import os +import re +import shutil +import subprocess +import unittest + +WORKFLOW = os.path.normpath( + os.path.join( + os.path.dirname(__file__), "..", "..", "workflows", "cursor-review.yml" + ) +) + +JOB_HEADER = re.compile(r"^ ([A-Za-z0-9_-]+):\s*$") +STEP_HEADER = re.compile(r"^ - ") +STEP_NAME = re.compile(r"^ -\s+name:\s*(\S.*)$") +COMMENT = re.compile(r"^\s*#") + +# The leg check and the panel job, by the names their check runs / step titles +# carry. Renaming either is a caller-visible change (a required context name), +# so the literals are pinned rather than matched loosely. +LEG_STEP = "Fail the leg when the cell did not submit" +UPLOAD_STEP = "Upload findings artifact" +PANEL_JOB = "panel-integrity" +PANEL_CONTEXT = "Panel integrity" +UNDECIDED_STEP = "Fail if the panel decision itself did not complete" +REPORT_STEP = "Report panel integrity" + +# Every condition `consolidate` gates on must also gate `panel-integrity`, or it +# reports on runs where no panel was ever supposed to happen. They are NESTED +# inside the upstream-failure disjunct rather than ANDed at the top level (see +# `UndecidedRunFailsClosedTest`), so these are substring assertions on purpose. +GATE_CONDITIONS = ( + "needs.gate.outputs.should_run == 'true'", + "needs.gate.outputs.already_reviewed != 'true'", + "needs.diff-size.outputs.within_cap == 'true'", + "needs.review.result != 'skipped'", +) + +# The five causes the panel job must still be able to see. Each is the exact +# expression its step reads, so a renamed job output fails here rather than +# silently evaluating to the empty string (which every one of these treats as a +# pass) and turning the check into a permanent green. +UNTRUSTED_VALUES = ("OK_COUNT", "TOTAL", "JUDGE_STATUS", "DELIVERED", "UNGATED", "GATED") + +# The jobs whose non-success means "nobody decided whether to review this PR". +# `preflight` and `ledger` are the subtle ones: the review matrix `needs:` BOTH, +# so a non-success in either leaves `needs.review.result == 'skipped'` — +# byte-identical to the deliberate no-panel branches — and gating on that alone +# minted a GREEN required check on a run where not one cell ever started. +# +# `ledger` reads like it cannot fail (every step in it is `continue-on-error`, +# for exactly this reason) — but `Ensure ledger artifact exists` carries none, +# its job cap sits above the sum of its step caps, and cancellation is neither. +# Rare is the wrong bar for a guard that hands out a green required check. +DECISION_RESULTS = ( + "needs.gate.result != 'success'", + "needs.diff-size.result != 'success'", + "needs.preflight.result != 'success'", + "needs.ledger.result != 'success'", +) + +# The job whose `needs:` list DEFINES which jobs are decision jobs. Pinned as a +# relationship rather than a name list, because the way this fail-open was +# re-opened twice was a job being added to the matrix's `needs:` and nobody +# adding it here. +MATRIX_JOB = "review" + +# Causes that read a producing job's RESULT rather than its outputs. An output +# is the empty string when its job failed/was skipped/was cancelled, and every +# output-shaped cause treats empty as a pass. +RESULT_CAUSES = ( + "needs.consolidate.result", + "needs.post-review.result", +) + +CAUSE_READS = ( + "needs.consolidate.outputs.ok_count", + "needs.consolidate.outputs.total", + "needs.consolidate.outputs.degraded", + "needs.post-review.outputs.delivered", + "needs.post-review.outputs.ungated_findings", + "needs.diff-size.outputs.incremental_subset", +) + + +def read_workflow(): + with open(WORKFLOW, encoding="utf-8") as f: + return f.read().split("\n") + + +def split_jobs(lines): + """{job name: [lines]} for the top-level jobs: mapping.""" + try: + start = lines.index("jobs:") + 1 + except ValueError: # pragma: no cover - the file always has one + raise AssertionError("cursor-review.yml has no top-level `jobs:` key") + + jobs, name, body = {}, None, [] + for line in lines[start:]: + match = JOB_HEADER.match(line) + if match: + if name: + jobs[name] = body + name, body = match.group(1), [] + continue + if name is not None: + body.append(line) + if name: + jobs[name] = body + return jobs + + +def split_steps(job_lines): + """[(name, [lines])] for each `- name:` step in a job body. + + A step with no `name:` on its header line comes back as `None`, which is + enough for the ORDERING assertions below without pretending to name it. + """ + steps, name, current = [], None, None + for line in job_lines: + if STEP_HEADER.match(line): + if current is not None: + steps.append((name, current)) + match = STEP_NAME.match(line) + name = match.group(1).strip().strip("'\"") if match else None + current = [line] + elif current is not None: + current.append(line) + if current is not None: + steps.append((name, current)) + return steps + + +def code_lines(block): + """Lines with whole-line comments dropped. + + Every comment block in this file NAMES the steps, jobs and outputs asserted + below — the leg step's own rationale quotes `status=error`, and the panel + job's header quotes `ungated_findings` — so a raw-text scan would stay green + with the code itself deleted. That is the exact failure this suite exists to + prevent, so every assertion runs over code only. + """ + return [line for line in block if not COMMENT.match(line)] + + +def job_scalar(job_lines, key): + """The scalar value of a job-level ` :`, or None when absent.""" + prefix = " %s:" % key + for line in code_lines(job_lines): + if line.startswith(prefix): + return line[len(prefix):].strip() + return None + + +def step_named(job_lines, wanted): + for name, body in split_steps(job_lines): + if name == wanted: + return body + return None + + +def step_order(job_lines): + return [name for name, _ in split_steps(job_lines)] + + +class LegFailsWhenTheCellDidNotSubmitTest(unittest.TestCase): + def setUp(self): + self.jobs = split_jobs(read_workflow()) + # Guard the parser: a splitter that silently stopped matching would make + # every assertion below pass vacuously. + for expected in ("review", "consolidate", "post-review", PANEL_JOB): + self.assertIn(expected, sorted(self.jobs), f"job splitter lost `{expected}`") + self.review = self.jobs["review"] + + def test_the_leg_check_exists(self): + self.assertIsNotNone( + step_named(self.review, LEG_STEP), + f"the `review` job lost its `{LEG_STEP}` step — a cell that never " + "submits is green again and the rollup lies about the panel", + ) + + def test_the_leg_check_runs_even_when_an_earlier_step_failed(self): + # Without `if: always()` the step is SKIPPED on exactly the runs it + # exists for: `Run cursor review` absorbs its own timeout, but a failed + # checkout, CLI install or prompt build fails the job at that step and + # everything after it is skipped by default. + body = code_lines(step_named(self.review, LEG_STEP)) + self.assertIn( + " if: always()", + body, + f"`{LEG_STEP}` is not `if: always()`", + ) + + def test_the_leg_check_runs_after_the_artifact_upload(self): + # Order is the whole reason failing here is free. Before the upload, a + # red leg would take `Upload findings artifact` down with it, the cell + # would vanish from the panel entirely, and `Aggregate panel findings` + # would undercount the matrix — strictly worse than the green-leg bug. + order = step_order(self.review) + self.assertIn(UPLOAD_STEP, order) + self.assertIn(LEG_STEP, order) + self.assertLess( + order.index(UPLOAD_STEP), + order.index(LEG_STEP), + f"`{LEG_STEP}` must come AFTER `{UPLOAD_STEP}`, or a non-submitting " + "cell loses its artifact instead of merely being reported", + ) + + def test_the_leg_check_is_not_itself_absorbed(self): + body = code_lines(step_named(self.review, LEG_STEP)) + self.assertFalse( + any("continue-on-error" in line for line in body), + f"`{LEG_STEP}` carries continue-on-error — it can no longer turn " + "the leg red, which is its only job", + ) + self.assertTrue( + any(line.strip() == "exit 1" for line in body), + f"`{LEG_STEP}` no longer exits non-zero", + ) + + def test_the_run_step_still_absorbs_its_own_cap(self): + # The premise of the split: the cap stays absorbed so the upload still + # runs, and the LEG check is what turns the cell red. Dropping + # `continue-on-error` from `Run cursor review` would fail the job at the + # timeout, skip the upload, and take the cell out of the panel. + body = code_lines(step_named(self.review, "Run cursor review")) + self.assertIn(" continue-on-error: true", body) + + def test_a_red_leg_does_not_cancel_its_sibling_cells(self): + # The other half of "failing here costs the panel nothing", and the only + # half that is not local to the step. With `fail-fast` left at its + # DEFAULT of true, the first cell to exit non-zero CANCELS every sibling + # still running — so the leg check would destroy the panel it exists to + # measure, landing six cells as one red leg and five cancelled ones, and + # `Aggregate panel findings` would undercount a matrix that was merely + # short before. Nothing in this matrix ever failed deliberately until + # the leg check was added (a cell that did not submit exited GREEN), so + # this setting was inert to the panel until this change made it + # load-bearing. That is exactly why it is pinned rather than assumed. + self.assertIn( + " fail-fast: false", + code_lines(self.review), + "the `review` matrix lost `fail-fast: false` — a cell that does not " + "submit now fails its leg deliberately and would CANCEL the sibling " + "cells, turning a short panel into no panel at all", + ) + + +class ConsolidateExposesPanelCountsTest(unittest.TestCase): + def setUp(self): + self.jobs = split_jobs(read_workflow()) + self.consolidate = "\n".join(code_lines(self.jobs["consolidate"])) + + def test_the_panel_counts_are_job_outputs(self): + # `Aggregate panel findings` has always PRINTED these. A job output is + # what makes them readable outside the job's own log. + for key, source in ( + ("ok_count", "steps.aggregate.outputs.ok_count"), + ("total", "steps.aggregate.outputs.total"), + ("degraded", "steps.consolidated.outputs.degraded"), + ("judge_status", "steps.consolidated.outputs.judge_status"), + ): + self.assertIn( + " %s: ${{ %s }}" % (key, source), + self.consolidate, + f"`consolidate` no longer exposes `{key}` from `{source}`", + ) + + def test_the_aggregate_step_still_writes_both_counts(self): + for written in ('g.write(f"ok_count={ok}\\n")', 'g.write(f"total={len(panel)}\\n")'): + self.assertIn(written, self.consolidate) + + +class PanelIntegrityJobTest(unittest.TestCase): + def setUp(self): + self.jobs = split_jobs(read_workflow()) + self.panel = self.jobs[PANEL_JOB] + self.body = "\n".join(code_lines(self.panel)) + + def test_it_publishes_the_documented_context_name(self): + # Callers mark ` / Panel integrity` required, and + # docs/callers/cursor-review.md names that string. Renaming the job's + # `name:` silently un-requires the check on every repo that did. + self.assertEqual(job_scalar(self.panel, "name"), PANEL_CONTEXT) + + def test_it_needs_every_job_it_reads(self): + needs = job_scalar(self.panel, "needs") + self.assertIsNotNone(needs, f"`{PANEL_JOB}` declares no `needs:`") + # Compare PARSED names, not the raw scalar: `review` is a substring of + # `post-review`, so a plain `in` check would stay green with `review` + # dropped from the list — the one entry whose loss this test most needs + # to catch, since `needs.review.result` is what keeps the job from + # reporting on a fork. + declared = {part.strip() for part in needs.strip("[]").split(",") if part.strip()} + for job in ("gate", "diff-size", "review", "consolidate", "post-review"): + self.assertIn( + job, + declared, + f"`{PANEL_JOB}` dropped `{job}` from needs — its outputs then " + "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:`. + condition = job_scalar(self.panel, "if") + self.assertIsNotNone(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 + # trigger label, already reviewed, over the diff-size cap, panel skipped + # — would be red on every PR that deliberately skips the review. + condition = job_scalar(self.panel, "if") + for gate in GATE_CONDITIONS: + self.assertIn(gate, condition, f"`{PANEL_JOB}` lost gate `{gate}`") + + def test_it_holds_no_permissions(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"), "{}") + + def test_it_is_bounded(self): + self.assertEqual(job_scalar(self.panel, "timeout-minutes"), "5") + + def test_it_reads_every_cause(self): + for read in CAUSE_READS: + self.assertIn( + "${{ %s }}" % read, + self.body, + f"`{PANEL_JOB}` no longer reads `{read}`", + ) + + def test_it_fails_and_annotates(self): + self.assertIn("exit 1", self.body) + self.assertIn("::error::", self.body) + self.assertIn("::notice::Panel integrity:", self.body) + + def test_it_flattens_untrusted_values_before_annotating(self): + # JUDGE_STATUS comes from the judge agent's own tool output and the + # counts follow panel-cell artifacts — all written inside jobs that run + # `cursor-agent --trust` over PR code. A newline reaching a + # ::workflow command:: line forges a second command, so every one of + # these must be interpolated through `flatten` and nowhere else. + # + # Two things this had to get right, and originally did not: + # + # 1. The pattern matches the BARE `$VAR` / `${VAR`, with no leading + # quote. Requiring a `"` immediately before the `$` only ever matched + # the already-safe `$(flatten "$VAR")` form — so the unsafe form this + # test exists to catch (`status=$JUDGE_STATUS`, a non-quote character + # before the `$`) never matched at all, while `hits` stayed non-zero + # from the safe occurrences and the assertion passed VACUOUSLY over a + # real workflow-command injection. + # 2. Every line that ECHOES is checked, not only lines containing `::`. + # The runner parses workflow commands on every stdout line, so an + # untrusted value echoed on a plain log line is the same hole. Lines + # that merely TEST a value (`if [ "$OK_COUNT" != "$TOTAL" ]`) reach no + # stdout and are correctly left alone. + self.assertIn("flatten()", self.body) + for var in UNTRUSTED_VALUES: + pattern = re.compile(r"\$\{?%s\b" % var) + hits = 0 + for line in self.body.split("\n"): + if "echo " not in line: # only echoed lines reach the log + continue + for match in pattern.finditer(line): + hits += 1 + # `$(flatten "$VAR"` and `$(flatten "${VAR:-…}"` both leave + # `…$(flatten "` before the match; the bare `$(flatten $VAR` + # form leaves `…$(flatten `. Strip the optional quote, then + # require the call. + prefix = line[: match.start()].rstrip('"') + self.assertTrue( + prefix.endswith("flatten "), + f"`{PANEL_JOB}` echoes ${var} without flatten(): a " + "newline in it forges a second workflow command" + f"\n {line.strip()}", + ) + self.assertTrue(hits, f"`{PANEL_JOB}` no longer reports ${var}") + + def test_it_gates_nothing(self): + # Advisory: red here must not stop the review from posting, or a short + # panel would cost the PR the findings it DID produce. + for name, lines in self.jobs.items(): + if name == PANEL_JOB: + continue + needs = job_scalar(lines, "needs") or "" + self.assertNotIn( + PANEL_JOB, + needs, + f"job `{name}` needs `{PANEL_JOB}` — the check is advisory and " + "must gate no other job", + ) + + +class UndecidedRunFailsClosedTest(unittest.TestCase): + """The job must go RED, not skipped, when the decision jobs did not finish. + + 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 + SKIPS it exactly when `gate`'s dup-check API call errors or `diff-size` + cannot build the diff — and GitHub counts a skipped required check as + PASSING, so a caller that took this PR's advice and required `Panel + 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. + """ + + def setUp(self): + self.jobs = split_jobs(read_workflow()) + self.panel = self.jobs[PANEL_JOB] + self.condition = job_scalar(self.panel, "if") + self.assertIsNotNone(self.condition) + self.declared_needs = { + part.strip() + for part in (job_scalar(self.panel, "needs") or "").strip("[]").split(",") + if part.strip() + } + + def test_the_job_runs_when_an_upstream_decision_job_failed(self): + # Without EVERY disjunct the job is skipped on that failing path and the + # guard step below can never fire, however it is written. + for result in DECISION_RESULTS: + self.assertIn( + result, + self.condition, + f"`{PANEL_JOB}`'s `if:` no longer runs the job on `{result}` — an " + "undecided run skips this check, and a skipped required check is " + "a GREEN merge gate", + ) + + def test_the_guard_step_exists_and_is_first(self): + order = step_order(self.panel) + self.assertIn( + UNDECIDED_STEP, + order, + f"`{PANEL_JOB}` lost its `{UNDECIDED_STEP}` guard — the job now runs " + "on undecided runs and reports causes read from empty outputs", + ) + self.assertIn(REPORT_STEP, order) + self.assertLess( + order.index(UNDECIDED_STEP), + order.index(REPORT_STEP), + f"`{UNDECIDED_STEP}` must come BEFORE `{REPORT_STEP}`: every cause " + "there treats an empty output as a pass, so the report would answer " + "the wrong question on a run that was never decided", + ) + + def test_the_guard_step_fires_on_every_failure_and_exits_nonzero(self): + body = code_lines(step_named(self.panel, UNDECIDED_STEP)) + joined = "\n".join(body) + for result in DECISION_RESULTS: + self.assertIn( + result, + joined, + f"`{UNDECIDED_STEP}` no longer fires on `{result}`", + ) + self.assertFalse( + any("continue-on-error" in line for line in body), + f"`{UNDECIDED_STEP}` carries continue-on-error and can no longer fail the check", + ) + self.assertTrue( + any(line.strip() == "exit 1" for line in body), + f"`{UNDECIDED_STEP}` no longer exits non-zero", + ) + self.assertIn("::error::", joined) + + def test_it_needs_the_decision_jobs_so_it_can_read_their_results(self): + # `needs..result` evaluates to the empty string unless the job is + # declared in `needs:` — and '' != 'success' is TRUE, so dropping one + # from the list would make this check red on EVERY run rather than + # failing open. Loud, but still wrong, and pinned so they stay declared. + for job in ("preflight", "ledger"): + self.assertIn(job, self.declared_needs) + + def test_every_job_the_matrix_needs_is_guarded_here(self): + # THE invariant, and the one both regressions broke. The review matrix + # skips when ANY job it `needs:` does not succeed, and a skipped matrix + # is indistinguishable from the deliberate no-panel branches — so every + # job in the matrix's `needs:` has to be a decision job here, in this + # job's `needs:` AND in the guard's condition. Asserting the RELATIONSHIP + # rather than today's four names is what makes the next job added to the + # matrix fail this suite instead of silently re-opening the fail-open. + matrix_needs = { + part.strip() + for part in (job_scalar(self.jobs[MATRIX_JOB], "needs") or "") + .strip("[]") + .split(",") + if part.strip() + } + self.assertTrue( + matrix_needs, + f"could not read `{MATRIX_JOB}`'s `needs:` — the invariant below is " + "asserting over nothing", + ) + guard = "\n".join(code_lines(step_named(self.panel, UNDECIDED_STEP))) + for job in sorted(matrix_needs): + self.assertIn( + job, + self.declared_needs, + f"`{MATRIX_JOB}` needs `{job}` but `{PANEL_JOB}` does not: a " + f"non-success `{job}` skips the matrix, leaving " + "`needs.review.result == 'skipped'` and a GREEN required check " + "over a run where not one cell reviewed", + ) + self.assertIn( + "needs.%s.result != 'success'" % job, + guard, + f"`{UNDECIDED_STEP}` does not fail closed on a non-success " + f"`{job}`, which skips the review matrix", + ) + self.assertIn( + "needs.%s.result != 'success'" % job, + self.condition, + f"`{PANEL_JOB}`'s `if:` does not run on a non-success `{job}`, " + "so the check SKIPS — and GitHub counts a skipped required " + "check as passing", + ) + + def test_the_causes_read_the_producing_jobs_results_not_only_outputs(self): + # `ok_count`/`total` are BOTH empty when `consolidate` died, and + # unset-vs-unset compares equal, so the completeness comparison alone + # reports a whole panel over a job that never ran. Pair every cause with + # its producer's result, and reject an empty count explicitly. + report = "\n".join(code_lines(step_named(self.panel, REPORT_STEP))) + for cause in RESULT_CAUSES: + self.assertIn( + "${{ %s }}" % cause, + report, + f"`{REPORT_STEP}` no longer reads `{cause}` — an empty output " + "from a dead producer then reads as a pass", + ) + self.assertIn( + '[ -z "$OK_COUNT" ] || [ -z "$TOTAL" ]', + report, + f"`{REPORT_STEP}` no longer rejects empty cell counts: unset-vs-unset " + 'compares EQUAL, so it would print "whole panel" having counted nothing', + ) + + def test_the_deliberate_skips_are_still_skips(self): + # The fail-closed disjunct must not swallow the intentional no-panel + # branches: those are the ones where `gate` and `diff-size` both + # SUCCEEDED and said no review was warranted, and being red on every + # unlabelled PR is what would get this check un-required again. + for gate in GATE_CONDITIONS: + self.assertIn(gate, self.condition, f"`{PANEL_JOB}` lost gate `{gate}`") + + +class FlattenEscapesWorkflowCommandsTest(unittest.TestCase): + """`flatten()` must defeat the runner's OWN unescaping, not just newlines. + + The runner unescapes `%25`, `%0D` and `%0A` inside a workflow command's + message before rendering it. So deleting real newlines is only half the + defence: a value carrying the literal six characters `%0A` arrives here + perfectly single-line and the runner puts the newline back, forging the + second command `tr -d` was meant to prevent. + + Only `JUDGE_STATUS` can actually carry one today — it is whatever string + the judge agent's tool wrote as `status`, unvalidated — but the function is + the shared chokepoint for every value the step echoes, so it is pinned + here rather than at the one call site that needs it. Every other checker in + this repo already escapes `%` before annotating (`check_agents_md.py`, + `check_workflow_pins.py`, `check-org-repo-literals.sh`); this is the same + escape, and the suite executes the real line rather than pattern-matching + it. + """ + + def setUp(self): + self.jobs = split_jobs(read_workflow()) + self.assertIn(PANEL_JOB, self.jobs, "job splitter lost the panel job") + report = step_named(self.jobs[PANEL_JOB], REPORT_STEP) + self.assertIsNotNone(report, f"`{REPORT_STEP}` is gone") + definitions = [ + line.strip() + for line in code_lines(report) + if line.strip().startswith("flatten()") + ] + self.assertEqual( + len(definitions), + 1, + f"expected exactly one `flatten()` definition in `{REPORT_STEP}`, " + f"found {len(definitions)}", + ) + self.definition = definitions[0] + + def _flatten(self, value): + """Run the workflow's ACTUAL flatten() line against `value`.""" + bash = shutil.which("bash") + self.assertIsNotNone(bash, "bash is required to execute flatten()") + script = self.definition + '\nflatten "$1"\n' + done = subprocess.run( + [bash, "-c", script, "flatten-test", value], + capture_output=True, + text=True, + timeout=30, + ) + self.assertEqual( + done.returncode, 0, f"flatten() failed: {done.stderr}" + ) + return done.stdout + + def test_it_escapes_percent_so_the_runner_cannot_re_expand_a_newline(self): + # The attack: no real newline anywhere in the input, so `tr -d` is a + # no-op, and the runner's unescaping supplies the line break. + out = self._flatten("error%0A::stop-commands::tok") + self.assertNotIn( + "%0A::", + out, + "flatten() passed a live `%0A` through: the runner unescapes it " + "into a real newline and the following `::stop-commands::` becomes " + "a genuine workflow command", + ) + self.assertIn( + "%250A", + out, + "flatten() must escape `%` to `%25`, which renders the payload as " + "the inert literal text `%0A`", + ) + # The escape must not silently eat the value it is protecting. + self.assertTrue( + out.startswith("error"), f"flatten() mangled the real status: {out!r}" + ) + + def test_it_still_strips_real_newlines(self): + # The original guarantee, kept: a genuine newline must be DELETED, not + # escaped back into one by the runner. + out = self._flatten("ok\n::error::forged\r\nmore") + self.assertNotIn("\n", out.rstrip("\n")) + self.assertNotIn("\r", out) + self.assertNotIn("%0A", out) + self.assertNotIn("%0D", out) + + def test_it_still_clamps_long_values(self): + out = self._flatten("A" * 500).rstrip("\n") + self.assertLessEqual( + len(out), 64, "flatten() no longer clamps: an agent controls this string" + ) + + def test_the_clamp_cannot_split_an_escape_it_created(self): + # `cut` must run BEFORE the `%` escape. Were it after, a `%25` produced + # at the boundary could be truncated to a bare `%2`/`%` — and, worse, + # a clamp applied to already-escaped text makes the 64-char budget + # depend on attacker-chosen content. + out = self._flatten("%" * 100).rstrip("\n") + self.assertNotIn("%2\n", out) + self.assertEqual( + out, + "%25" * 64, + "each of the 64 clamped `%` must survive as a WHOLE `%25`", + ) + + def test_every_value_the_step_echoes_goes_through_it(self): + # Belt-and-braces against the fix being bypassed rather than reverted: + # a future cause that echoes a raw `$VAR` reintroduces the hole even + # with flatten() intact. `test_it_flattens_untrusted_values_before_ + # annotating` pins the same property structurally; this asserts the + # definition it relies on is the escaping one. + self.assertIn("%25", self.definition, "flatten() no longer escapes `%`") + self.assertIn("tr -d", self.definition, "flatten() no longer strips newlines") + + +if __name__ == "__main__": + unittest.main(verbosity=2) diff --git a/.github/workflows/cursor-review.yml b/.github/workflows/cursor-review.yml index 56af0f1..bac8f57 100644 --- a/.github/workflows/cursor-review.yml +++ b/.github/workflows/cursor-review.yml @@ -1686,7 +1686,15 @@ jobs: # the whole review to 35+ min. Killing the step instead leaves the # pre-seeded status=error artifact for the judge to count as a # non-contributing cell, exactly what the job timeout produced, but - # 15 minutes sooner and green. + # 15 minutes sooner and without CANCELLING the run. + # + # The cap is what this `continue-on-error` absorbs, and that is all it + # absorbs: the LEG is not green on this path. `Fail the leg when the + # cell did not submit` reads the artifact back at the end of the job and + # fails it deliberately, so a cell that never submitted is a red check in + # the PR's rollup instead of a silent hole in the panel count. Keeping + # the absorption HERE rather than dropping `continue-on-error` is what + # lets the artifact upload still run, so the panel keeps consolidating. continue-on-error: true timeout-minutes: 15 env: @@ -1734,6 +1742,74 @@ jobs: if-no-files-found: error retention-days: 7 + - name: Fail the leg when the cell did not submit + # Panel integrity has to be readable from the CHECK-RUN ROLLUP, not only + # from a line in this job's log. Before this step a cell that never + # submitted — overwhelmingly the 15-minute `Run cursor review` cap above, + # absorbed by its `continue-on-error` — left the pre-seeded status=error + # artifact and exited GREEN, so a reader of `statusCheckRollup` saw a + # complete panel while `Aggregate panel findings` was quietly reporting + # "2/6 cells contributed findings". Measured on one consumer repo over 92 + # panel runs: 38 runs had at least one errored leg, 52 of 552 cells + # errored, and every leg check in every one of those runs reported + # `success` (BE-15554). + # + # Deliberately AFTER the upload, so failing here costs the panel nothing: + # the artifact is already published, `consolidate` and `notify-complete` + # gate on `needs.review.result != 'skipped'` (a FAILED matrix is not a + # skipped one), and `post-review` gates on `consolidate`. The + # consolidated review still posts — the only thing that changes is that + # the gap is now visible. `Panel integrity` further down aggregates the + # same fact into one panel-level context. + # + # SCOPE, because the check reads stronger than it is: the verdict is the + # `status` this cell's OWN agent wrote, and that agent runs `--trust` + # with shell access over attacker-authored diff text, so a prompt-injected + # cell can write status=ok with no findings and green both this leg and + # the panel count. This is an AVAILABILITY signal — it catches the stalls, + # crashes and step caps that are what every observed failure has been — + # not an attestation that a review happened. A forgery-resistant count + # needs the submission recorded outside the cell's own writable job. + if: always() + env: + MODEL: ${{ matrix.model }} + REVIEW_TYPE: ${{ matrix.review_type }} + run: | + # findings.json is untrusted: this cell's own agent runs `--trust` + # with shell access in this job, so it can write whatever it likes + # there. Read the status defensively (a missing or unparseable file is + # "did not submit", not a traceback) and sanitize before it reaches a + # ::workflow command:: line. The substitution REPLACES a disallowed + # character rather than dropping it — deleting would let a crafted + # "o k" collapse into the one value that passes. + # + # `ValueError`, not `json.JSONDecodeError`: the file is opened as + # utf-8, so invalid UTF-8 — reachable precisely BECAUSE these bytes + # are agent-written — raises `UnicodeDecodeError`, which is NOT a + # JSONDecodeError. Under `set -e` that traceback would kill the step + # and lose the `::error::` naming which cell failed, turning the one + # case this step exists to report into an unlabelled crash. Both + # exception types subclass `ValueError`, so one clause covers the pair. + status=$(python3 - <<'PY' + import json, re + + try: + with open("/tmp/findings-out/findings.json", encoding="utf-8") as f: + cell = json.load(f) + except (OSError, ValueError): + cell = None + value = cell.get("status") if isinstance(cell, dict) else None + if not isinstance(value, str): + value = "error" + print(re.sub(r"[^A-Za-z0-9_.-]", "_", value)[:32] or "error") + PY + ) + if [ "$status" != "ok" ]; then + echo "::error::Reviewer cell ${REVIEW_TYPE}/${MODEL} did not submit a review (status=$status). Its findings artifact was uploaded and the panel still consolidates; this leg is red so the gap is visible in the rollup." + exit 1 + fi + echo "Reviewer cell ${REVIEW_TYPE}/${MODEL} submitted its findings." + consolidate: # Adjudicates the panel cell artifacts into one consolidated review. # Calls cursor-agent one more time as the judge, with prompt-judge.md. @@ -1782,6 +1858,23 @@ jobs: # below holds pull-requests: write and the bot credentials instead. permissions: contents: read + # Panel-integrity facts, for the `Panel integrity` job at the bottom of this + # file. They are the same numbers the steps below already print; a job output + # is what makes them readable from OUTSIDE this job's log, which is the whole + # point — `Aggregate panel findings` reporting "2/6 cells contributed" in a + # log nobody opens is not a signal a merge gate can act on. + outputs: + ok_count: ${{ steps.aggregate.outputs.ok_count }} + total: ${{ steps.aggregate.outputs.total }} + degraded: ${{ steps.consolidated.outputs.degraded }} + # Untrusted: this is whatever string the judge agent's own tool wrote as + # `status` in /tmp/judge-findings.json, in a job running `cursor-agent + # --trust` over PR-authored text. It is neither validated against an enum + # nor escaped on the way out. Anything that consumes it must flatten AND + # percent-escape it before it reaches a ::workflow command:: line — + # stripping newlines alone is not enough, because the runner unescapes + # `%0A` back into one. `Panel integrity` does both. + judge_status: ${{ steps.consolidated.outputs.judge_status }} steps: - name: Checkout PR repo uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 @@ -2137,8 +2230,10 @@ jobs: # `ledger_download` travels with them because the ledger banner asks two # separate questions — did the JUDGE get prior-round context (this job's # download), and can the poster READ the note (its own download). - # `degraded` is diagnostic: the review's own degradation banner keys on - # `judge_status`, so this only ever reaches the run log. + # `degraded` is diagnostic as far as the REVIEW is concerned: the + # review's own degradation banner keys on `judge_status`, so nothing it + # carries here reaches the PR body. It is also a job output of this job, + # read by `Panel integrity`. env: JUDGE_STATUS: ${{ steps.consolidated.outputs.judge_status }} CONSOLIDATED_COUNT: ${{ steps.consolidated.outputs.consolidated_count }} @@ -2568,6 +2663,236 @@ jobs: --error-message "Judge call failed (status=${JUDGE_STATUS}): ${JUDGE_ERROR}" fi + panel-integrity: + # ONE advisory check-run that says whether the panel that just ran was whole. + # + # Everything it reports was already knowable — and none of it reached a + # check-run conclusion, which is the only surface an automated merge gate + # reads. `Aggregate panel findings` printed "2/6 cells contributed findings" + # into a log; post-review.py emitted `ungated_findings=` as a job output + # nothing consumed; a leg that never submitted exited green. Measured on one + # consumer repo over 92 panel runs: 38 runs had at least one errored leg, 52 + # of 552 cells errored, and every leg check in every one of those runs + # reported `success`, so `statusCheckRollup` was fully green over panels that + # were a third of a panel (BE-15554). + # + # This is the panel-level half of that fix; `Fail the leg when the cell did + # not submit` in the `review` job is the per-cell half. This job is what a + # gate should read, because a matrix leg's context name carries the model id + # and therefore CHANGES whenever the panel list does — a required check on + # `edge-case (kimi-k3-high)` bricks every PR the day that pin moves. + # + # ADVISORY by default, exactly like `Blocking gate`: red here fails no other + # job, gates no merge, and does not gate `post-review` — a caller that wants + # it to block marks ` / Panel integrity` a required check in + # its own ruleset. See docs/callers/cursor-review.md. + # + # `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, + # 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. + # + # 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 + # them are job OUTPUTS, and a job that FAILED has empty outputs — so a gate + # whose dup-check API call errored, or a diff-size that could not build the + # diff at all, would make all four false, skip this job, and mint a GREEN + # required check on a run where whether the PR was reviewed is unknown. The + # fourth, `needs.review.result != 'skipped'`, has the same hole from the + # other end: the matrix declares `needs: [gate, diff-size, preflight, + # ledger]`, so a non-success in ANY of those four skips the whole matrix and + # leaves a 'skipped' result that reads exactly like the deliberate no-panel + # branches — a green check over a run where not one cell ever started. An + # undecided run is not a clean run. So the condition below runs this job + # whenever an upstream DECISION job (`gate`, `diff-size`, `preflight`, + # `ledger`) did not succeed, and `Fail if the panel decision itself did + # not complete` turns that into red — the same fail-closed shape, and the + # same reasoning, as `Blocking gate`'s first two guards. The deliberate + # skips are unchanged: they are the branches where `gate` and `diff-size` + # both SUCCEEDED and said no panel was warranted. + # + # Every job the matrix `needs:` must be listed here. That list is the + # invariant — not the four names that happen to be in it today — because a + # job added to `review`'s `needs:` without being added here re-opens this + # exact fail-open silently, which is how `preflight` and then `ledger` were + # each missed in turn. `test_workflow_panel_integrity.py` pins the two lists + # against each other so the next one cannot be. + name: Panel integrity + needs: [gate, 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.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 + # 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: {} + steps: + - 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 + # job producing them failed — and every one of those causes treats the + # empty string as a pass. Reaching `Report panel integrity` on this path + # would print "no review delivered" over a run whose panel was never + # even decided on, which is a true statement about the wrong question. + # + # This condition is exact GIVEN the job `if:` above, which admits only + # three ways to arrive here with an upstream decision job unfinished: + # `gate` did not succeed; `gate` selected a fresh in-scope review and + # `diff-size` did not succeed; or both succeeded and said review, and + # `preflight` or `ledger` did not succeed. All four are read back + # verbatim. (`within_cap` can only be the string 'true' when `diff-size` + # succeeded, so the deliberate over-cap skip never reaches this step.) + # + # `preflight` and `ledger` are here because they are the decision jobs + # whose failure is invisible from the outputs: `review` needs BOTH, so a + # non-success in either SKIPS the whole matrix, `needs.review.result` is + # then 'skipped' — indistinguishable from the deliberate no-panel + # branches — and gating on that alone skipped this job and minted a + # GREEN required check on a run where NOTHING was reviewed. The job + # `if:` therefore admits a non-success `preflight` or `ledger` under a + # within-cap fresh review, and this step turns it red. A panel that + # never started is not a whole one. + # + # `ledger` is the one that reads like it cannot happen: every step in it + # is `continue-on-error`, precisely so it never skips the matrix. That + # makes a red `ledger` rare, NOT unreachable — its own job comment is + # 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. + # + # 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.diff-size.result != 'success' || needs.preflight.result != 'success' || needs.ledger.result != 'success' }} + env: + GATE_RESULT: ${{ needs.gate.result }} + DIFF_SIZE_RESULT: ${{ needs.diff-size.result }} + PREFLIGHT_RESULT: ${{ needs.preflight.result }} + LEDGER_RESULT: ${{ needs.ledger.result }} + run: | + echo "::error::Panel integrity cannot be judged: the jobs that decide whether a panel should run did not succeed (gate: ${GATE_RESULT}, diff size: ${DIFF_SIZE_RESULT}, preflight: ${PREFLIGHT_RESULT}, ledger: ${LEDGER_RESULT}). Whether this PR was reviewed at all is unknown — re-run the failed job." + exit 1 + + - name: Report panel integrity + env: + OK_COUNT: ${{ needs.consolidate.outputs.ok_count }} + TOTAL: ${{ needs.consolidate.outputs.total }} + DEGRADED: ${{ needs.consolidate.outputs.degraded }} + JUDGE_STATUS: ${{ needs.consolidate.outputs.judge_status }} + DELIVERED: ${{ needs.post-review.outputs.delivered }} + GATED: ${{ needs.post-review.outputs.gated_findings }} + UNGATED: ${{ needs.post-review.outputs.ungated_findings }} + # The RESULTS of the two jobs every value above comes out of. Each + # cause below is phrased against an OUTPUT, and an output is the empty + # string when the job producing it failed, was skipped or was + # cancelled — a state every one of those causes would otherwise read + # as a pass. `OK_COUNT != TOTAL` is the sharpest case: both are empty + # on a dead `consolidate`, so the comparison is unset-vs-unset and + # says "whole panel". That is caught today only because `DELIVERED` + # happens to be empty on those same runs, which is coincidence, not a + # guard — `post-review.py` writes `delivered=true` the moment the POST + # returns and keeps working, so a `post-review` that dies in a LATER + # step leaves delivered=true behind a red job. Read both signals, the + # way `Blocking gate`'s second guard does. + CONSOLIDATE_RESULT: ${{ needs.consolidate.result }} + POST_REVIEW_RESULT: ${{ needs.post-review.result }} + # Declared by `diff-size` only once the incremental-diff fix lands; + # until then this expression is the empty string and the check below + # is inert BY DESIGN. Empty is "not measured", which is a pass — the + # only failing value is a literal 'false', i.e. measured and wrong. + INCREMENTAL_SUBSET: ${{ needs.diff-size.outputs.incremental_subset }} + run: | + # Every value below is a job output, and two of them (`JUDGE_STATUS` + # from the judge agent's own tool output, and the counts that follow a + # panel cell's artifact) originate in agent-written files inside jobs + # that run `--trust` over PR code. They land in ::workflow command:: + # lines here, where a newline would forge a second command, so flatten + # and clamp before echoing. + # + # Escaping `%` is the other half of that, and it is not optional: the + # runner UNESCAPES `%25`, `%0D` and `%0A` in a command's message, so + # deleting real newlines alone is not enough. A judge that writes its + # `status` as `error%0A::stop-commands::x` reaches this line carrying + # no newline at all — and the runner puts one back, forging the second + # command `tr` was supposed to prevent. `%` -> `%25` turns that into + # the inert literal text `%0A`. It is the same escape every checker in + # this repo already applies before annotating (`check_agents_md.py`, + # `check_workflow_pins.py`, `check-org-repo-literals.sh`), and only + # `JUDGE_STATUS` can actually carry a `%` today: the counts are + # python-formatted integers and the rest are job results. + # + # Order is load-bearing at both ends. AFTER `tr`, so a real newline is + # dropped rather than escaped back into one by the runner; and AFTER + # `cut`, so the clamp can never split a `%25` it just created (`cut` + # only ever removes characters, so it cannot forge one either). + flatten() { printf '%s' "$1" | tr -d '\r\n' | cut -c1-64 | sed 's/%/%25/g'; } + + causes=0 + + # Job-result causes FIRST, so a dead producer is named as itself + # rather than inferred from the empty outputs it left behind. + if [ "$CONSOLIDATE_RESULT" != "success" ]; then + echo "::error::The panel was never adjudicated: the Consolidate panel job did not succeed (result=$(flatten "${CONSOLIDATE_RESULT:-}")). Its cell counts and judge status are unavailable, so panel completeness is unknown rather than clean." + causes=$((causes + 1)) + fi + + if [ "$POST_REVIEW_RESULT" != "success" ]; then + echo "::error::The Post review job did not succeed (result=$(flatten "${POST_REVIEW_RESULT:-}")), so whether a review with resolvable threads reached this PR is unknown — delivered=true is written the moment the POST returns, before the rest of that job runs." + causes=$((causes + 1)) + fi + + # An EMPTY count is not a matching count. Guarded separately from the + # comparison below because unset-vs-unset compares EQUAL in `test`, + # which is the one way this check could print "whole panel" having + # counted nothing at all. + if [ -z "$OK_COUNT" ] || [ -z "$TOTAL" ]; then + echo "::error::Panel cell counts are missing (ok_count=$(flatten "${OK_COUNT:-}"), total=$(flatten "${TOTAL:-}")), so how many reviewer cells submitted cannot be established. Treat this as an incomplete panel, not a whole one." + causes=$((causes + 1)) + elif [ "$OK_COUNT" != "$TOTAL" ]; then + echo "::error::Panel incomplete: $(flatten "$OK_COUNT") of $(flatten "$TOTAL") reviewer cells submitted a review. The consolidated review was adjudicated over a short panel — see the red leg checks for which cells did not submit." + causes=$((causes + 1)) + fi + + # Non-empty AND greater than zero: an EMPTY value means post-review + # never got as far as deciding, which `DELIVERED` below reports on its + # own terms rather than as a phantom unanchored finding. + if [ -n "$UNGATED" ] && [ "$UNGATED" != "0" ]; then + echo "::error::$(flatten "$UNGATED") finding(s) could not be anchored to the reviewed diff and were demoted to the review body, where they have no thread to resolve. Read the review body, not just the inline comments." + causes=$((causes + 1)) + fi + + if [ "$DELIVERED" != "true" ]; then + echo "::error::No review carrying resolvable finding threads was delivered to this PR (post-review delivered=$(flatten "${DELIVERED:-}")). The findings, if any, are in the post-review job summary." + causes=$((causes + 1)) + fi + + if [ "$DEGRADED" = "true" ]; then + echo "::error::The judge did not adjudicate this panel (judge status=$(flatten "${JUDGE_STATUS:-?}")); the review was built from the raw union of the cells' findings, so duplicates and false positives were not filtered." + causes=$((causes + 1)) + fi + + if [ "$INCREMENTAL_SUBSET" = "false" ]; then + echo "::error::The incremental \"new since the last reviewed round\" block was not a subset of the diff the panel reviewed, so the cells were prioritized onto hunks that were not in scope." + causes=$((causes + 1)) + fi + + if [ "$causes" -ne 0 ]; then + echo "Panel integrity: $causes problem(s) above." + exit 1 + fi + + echo "::notice::Panel integrity: $(flatten "$OK_COUNT")/$(flatten "$TOTAL") cells, $(flatten "${GATED:-0}") anchored finding(s), 0 unanchored." + blocking-gate: # Opt-in merge gate (inputs.blocking), restored by BE-4691 after #31 # dropped it unintentionally. Fails this check while the PR has unresolved, diff --git a/README.md b/README.md index 4bc0ee5..1d27a21 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)). 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 runner and the panel model list are fixed by the reusable — there are no `runs_on` / `models` inputs (`models` is a job *output*). 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). 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 runner and the panel model list are fixed by the reusable — there are no `runs_on` / `models` inputs (`models` is a job *output*). 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-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) | | [`assign-prs-to-author.yml`](.github/workflows/assign-prs-to-author.yml) | Housekeeping — assigns every open PR with no assignees to its author (bot-authored PRs skipped by default). Run on a schedule from a thin caller; useful when a team tracks PR ownership via assignees. The calling job needs `pull-requests: write` and `issues: write`. | [assign-prs-to-author.md](docs/callers/assign-prs-to-author.md) | diff --git a/docs/callers/cursor-review.md b/docs/callers/cursor-review.md index 7033bf5..acc04da 100644 --- a/docs/callers/cursor-review.md +++ b/docs/callers/cursor-review.md @@ -38,6 +38,12 @@ failure cases besides. The Blocking gate does not have this hole: with `blocking: true` it runs on every event the caller delivers, so its verdict is always a live query of the PR's thread state, never a skip. +**Panel integrity is its own check, and it is always on.** A reviewer cell that +never submits no longer exits green, and a run whose panel came up short, whose +findings could not be anchored, or whose review never landed publishes a red +`Panel integrity` context. It needs no input and blocks nothing by itself — see +[Panel integrity](#panel-integrity). + Prompts and scripts live in [`.github/cursor-review/`](../../.github/cursor-review) — the single source of truth, so your repo carries only a thin caller. @@ -217,6 +223,132 @@ What it does **not** do is make your own group redundant. Keep the caller group **`run_without_label: true` reviews every PR.** On a busy repo that is a large step up in spend. Start label-gated. +## Panel integrity + +` / Panel integrity` (with the caller above, `review / Panel +integrity`) is the context that answers **"was the panel that reviewed this PR a +whole panel?"** — it is the one an automated merge gate should read for that +question, and it runs on every review, with no input to turn on. Read the +**"red, not skipped, when the decision itself failed"** and **"still skips when +no review was warranted"** bullets below before you require it: it is red when +the review ran and came up short *and* when the decision that selects a review +failed, but it is **skipped — and therefore green — on the runs that +deliberately review nothing**. + +It is **advisory**: red here fails no other job, and the consolidated review +still posts. Marking it a required status check in your branch-protection / +ruleset settings is what makes red block a merge — the same two-switch shape as +the Blocking gate, and independent of it. The two answer different questions: +Panel integrity asks whether the review was *complete*, the Blocking gate asks +whether its findings were *addressed*. + +Red means at least one of these, each named on its own `::error::` annotation in +the job log: + +| Cause | What it means | +|---|---| +| Panel incomplete | Fewer cells submitted findings than ran. The consolidated review was adjudicated over a short panel. The individual leg checks (`adversarial ()` / `edge-case ()`) are red for exactly the cells that did not submit — almost always the 15-minute agent cap. | +| Unanchored findings | Findings the review could not anchor to a line of the reviewed diff, so they were demoted to the review **body** and have no thread. The Blocking gate cannot see them; read the body. | +| Nothing delivered | No review carrying resolvable finding threads reached the PR — a read-only token, a rejected inline payload, or a post that could not be confirmed. The findings are in the `Post review` job summary. | +| Judge degraded | The judge model never adjudicated; the review is the raw union of the cells' findings, so duplicates and false positives were not filtered out. | +| Panel never adjudicated / Post review failed | `Consolidate panel` or `Post review` did not succeed, so the values the causes above are read from are absent. Reported as its own cause rather than inferred from the empty outputs, because an absent output is not a clean one — and because `delivered=true` is written the moment the POST returns, so it can survive a `Post review` job that dies in a later step. | +| Cell counts missing | `ok_count`/`total` came back empty. Counted as an incomplete panel: unset-vs-unset compares equal, so without this the check would print "whole panel" having counted nothing. | + +**Your caller job goes red with a failing leg, and that is the point.** A +reusable workflow's caller job takes the aggregate conclusion of the jobs inside +it, so a cell that did not submit now turns `review` red as well as its own leg +— on a measured ~40% of runs, because a stalled `cursor-agent` is common. That +red is the signal: it is what makes an incomplete panel visible to anything +reading `statusCheckRollup`, which is exactly what used to be impossible. Read +it as "this review is partial", not as "the review failed": the consolidated +review still posts, the Blocking gate is unaffected, and re-running the failed +legs or re-triggering the review is what clears it. If you want a *merge* gate, +require `Panel integrity` — do **not** require the caller job itself, which is +red for every unrelated infrastructure failure too. + +**"Re-run failed jobs" posts a second review.** GitHub's re-run-failed-jobs +re-runs every job that *depends* on a failed one, so re-running a red leg also +re-runs `Consolidate panel` and `Post review` — while the green `Gate` is not +re-run and its cached `already_reviewed=false` is reused. `post-review.py`'s +landed-review check only fires when the POST itself *errors*, so a clean re-run +POSTs, and the PR ends up with two consolidated reviews. Prefer **re-running the +whole workflow** (which re-runs `Gate`, whose dup-check sees the review that +already landed) or re-triggering by label. Use re-run-failed-jobs when you +actually want a second, fuller review on the same commit. Both of those +alternatives, though, supersede a red `Panel integrity` with a skip — see the +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. +* **Do not require a leg check instead.** A panel cell's context name carries + the model id (`edge-case (kimi-k3-high)`), so it changes whenever the panel + list does — and a required check whose name no longer exists blocks every PR + in the repo. `Panel integrity` is stable by design. +* **It is red, not skipped, when the decision itself failed.** Panel integrity + is gated on the same four conditions the panel is. Three of them read `Gate`'s + and `Diff size check`'s job *outputs*, which are empty when those jobs + **failed**; the fourth reads the review matrix's *result*, which is `skipped` + whenever **any** job the matrix `needs:` did not succeed — `Preflight — + validate model catalog` or `Prior-review ledger`. Gating on those alone would + skip this check exactly when a dup-check API call errored, the diff could not + be built, or a delisted model stopped the panel before a single cell started — + and GitHub counts a skipped required check as **passing**. So a failed `Gate`, + `Diff size check`, `Preflight` **or `Prior-review ledger`** runs this job and + fails it: an undecided run is not a clean run. (`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.) +* **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 over the diff-size cap, a fork the panel cannot run on — are the ones where + `Gate` and `Diff size check` both *succeeded* and said no panel should run. + This check stays skipped there, and a required skipped check passes. That is + the intended shape: it answers **"was the panel that ran whole?"**, not "was + this PR reviewed at all?" If you need the second question gated too — most + relevantly, if you do not want an over-cap PR merging unreviewed — require the + Blocking gate, which fails closed on over-cap fresh reviews, and keep your own + label policy. Do not read a skipped Panel integrity as "the panel was fine". +* **A later no-panel run on the same commit supersedes a red with a skip.** + That is the previous bullet's sharp edge, and reaching it needs no failure at + all. Once `Panel integrity` is red on a head SHA, **any** subsequent run on + that same SHA which takes a deliberate no-panel branch publishes a *newer* + `Panel integrity` check run in the `skipped` state — and branch protection + reads the latest check run of a given name, counting a skipped one as + passing. An unrelated label toggled, `skip-cursor-review` applied, the trigger + label removed, or the already-reviewed re-trigger all do it — and so does + **re-running the whole workflow**, the remediation recommended above, whose + fresh `Gate` dup-check finds the review that already landed and reports + `already_reviewed=true`. The panel is still short, nothing was re-reviewed, + and the required check is green. Until this is closed (BE-15604), clear a red + `Panel integrity` by pushing a **new commit** — a fresh SHA gets its own + verdict — rather than by re-triggering on the reviewed one, and for a commit + whose review came up short read the check's *run history* rather than only its + latest conclusion. +* **It detects a cell that went missing, not a cell that lied.** "Did this cell + submit" is the `status` field of the artifact the cell itself wrote, and that + cell's agent runs `--trust` with shell access over attacker-authored diff + text. A prompt-injected cell can write `{"status": "ok"}` with zero findings + and green both its own leg and the panel count. That is a real limit, not a + quibble: this check is an availability signal — it catches the stalls, crashes + and caps that make up essentially all of the observed failures — and is **not** + an attestation that six independent reviews happened. Making the count + forgery-resistant needs the submission recorded outside the cell's own + writable job; until then, do not treat a green `Panel integrity` as proof + against an adversarial PR. +* **One listed cause is wired but inert.** The job also reads + `diff-size`'s `incremental_subset` — "were the cells prioritized onto hunks + that were actually in the reviewed diff?" — and fails on a literal `false`. + `Diff size check` does not publish that output yet (it arrives with the + incremental-diff fix), so today the expression is the empty string, which + counts as "not measured", i.e. a pass. Nothing in the rollup changes when it + starts being published; it is documented here so the cause table is not read + as a scope check that is already running. + ## Blocking-gate gotchas Everything in this section applies only once you pass `blocking: true`.