From 4c6e90edd1566853a3a3ff4496e27c4e74bcbf89 Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Fri, 18 Sep 2026 20:15:28 +0000 Subject: [PATCH 1/9] feat(cursor-review): make panel integrity a check-run conclusion MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A reviewer leg whose cursor-agent never submits — overwhelmingly the 15-minute "Run cursor review" step cap, absorbed by its continue-on-error — left the pre-seeded {"status":"error"} artifact and exited GREEN. "Aggregate panel findings" counted it into a log line and only short-circuited at zero, and post-review.py emitted ungated_findings= as a job output nothing consumed. None of it 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 and 52 of 552 cells errored, while every leg check in every one of those runs reported `success`. Two halves: * Each panel cell now fails its own leg when its artifact does not come back status=ok. The step sits AFTER the artifact upload on purpose, so the panel still consolidates and the review still posts — 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 status is read defensively and sanitized: the cell's own agent runs --trust with shell access in that job, so findings.json is untrusted bytes headed for a ::workflow command:: line. * One advisory `Panel integrity` job aggregates the panel-level facts — ok_count/total/degraded/judge_status, newly exposed as consolidate job outputs, plus post-review's delivered/gated/ungated — and goes red with one ::error:: per cause. It gates no other job; a caller that wants it to block marks the context required, exactly like the Blocking gate. Its context name is stable, unlike a matrix leg's, which carries the model id. Documented in the setup guide next to the Blocking gate (including that a failing leg now turns the caller job red, which is the signal) and in the directory README's new job table. Pinned by a new workflow-shape suite. --- .github/cursor-review/README.md | 47 ++- .../tests/test_workflow_panel_integrity.py | 348 ++++++++++++++++++ .github/workflows/cursor-review.yml | 185 +++++++++- docs/callers/cursor-review.md | 60 +++ 4 files changed, 636 insertions(+), 4 deletions(-) create mode 100644 .github/cursor-review/tests/test_workflow_panel_integrity.py diff --git a/.github/cursor-review/README.md b/.github/cursor-review/README.md index d30461d..e8fb350 100644 --- a/.github/cursor-review/README.md +++ b/.github/cursor-review/README.md @@ -56,6 +56,24 @@ 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 — 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 | Never fails the run: the review matrix `needs:` it, so it degrades to an empty ledger rather than erroring. | +| `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 | The panel was short, findings went unanchored, nothing was delivered, or the judge never adjudicated. 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 +89,31 @@ 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. 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. See [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 +161,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..c86d0ee --- /dev/null +++ b/.github/cursor-review/tests/test_workflow_panel_integrity.py @@ -0,0 +1,348 @@ +#!/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 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" + +# Every condition `consolidate` gates on must also gate `panel-integrity`, or it +# reports on runs where no panel was ever supposed to happen. +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") + +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) + + +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:`") + for job in ("gate", "diff-size", "review", "consolidate", "post-review"): + self.assertIn( + job, + needs, + 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. + 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 "::" not in line: # only the annotation lines interpolate + continue + for match in pattern.finditer(line): + hits += 1 + self.assertTrue( + line[: match.start()].endswith("flatten "), + f"`{PANEL_JOB}` interpolates ${var} into an annotation " + "without flatten(): a newline in it forges a workflow " + f"command\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", + ) + + +if __name__ == "__main__": + unittest.main(verbosity=2) diff --git a/.github/workflows/cursor-review.yml b/.github/workflows/cursor-review.yml index 56af0f1..99822e4 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,57 @@ 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. + 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. + 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, json.JSONDecodeError): + 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 +1841,19 @@ 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: written by the judge agent's own tool output. Anything that + # consumes it must flatten it before it reaches a ::workflow command:: + # line — `Panel integrity` does. + judge_status: ${{ steps.consolidated.outputs.judge_status }} steps: - name: Checkout PR repo uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 @@ -2137,8 +2209,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 +2642,111 @@ 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. + name: Panel integrity + needs: [gate, diff-size, review, consolidate, post-review] + if: ${{ always() && needs.gate.outputs.should_run == 'true' && needs.gate.outputs.already_reviewed != 'true' && needs.diff-size.outputs.within_cap == 'true' && 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: 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 }} + # 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. + flatten() { printf '%s' "$1" | tr -d '\r\n' | cut -c1-64; } + + causes=0 + + if [ "$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/docs/callers/cursor-review.md b/docs/callers/cursor-review.md index 7033bf5..7e7b011 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,60 @@ 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 this PR actually reviewed by a +whole panel?"** — it is the one an automated merge gate should read, and it runs +on every review, with no input to turn on. + +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. | + +**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. + +Three more 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. +* **A failed `Gate` job skips this check rather than failing it.** Panel + integrity is gated on the same four conditions the panel is, and all four read + `Gate`'s outputs — which are empty when that job failed. There is no panel to + report on in that case, and the `Gate` context is itself red, so the rollup + still carries the signal; the fail-closed guard for a *required* check lives on + the Blocking gate. + ## Blocking-gate gotchas Everything in this section applies only once you pass `blocking: true`. From 1ad01efe3f7fbed35b576465b81ba4b7a26e4c3e Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Fri, 18 Sep 2026 20:30:19 +0000 Subject: [PATCH 2/9] fix(cursor-review): fail Panel integrity closed when the decision jobs did not succeed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 SKIPPED it exactly when `gate`'s dup-check API call errored or `diff-size` could not build the diff — and GitHub counts a skipped required check as PASSING. A caller that took this PR's own advice and required `Panel integrity` as its merge gate would have gotten a green check over a run that never decided whether to review the PR at all. Run the job when an upstream decision job did not succeed, and fail it there, with the guard FIRST so the report step never answers the wrong question off empty outputs. This is the same fail-closed shape, and the same reasoning, as `Blocking gate`'s first two guards. The deliberate no-panel skips are unchanged: they are the branches where `gate` and `diff-size` both SUCCEEDED and said no panel was warranted. Docs said the opposite, so both surfaces are corrected, and the caller guide now states plainly that a skipped Panel integrity is green and answers "was the panel whole?", not "was this PR reviewed at all?". Also parse `needs:` into job names before asserting membership: `review` is a substring of `post-review`, so the old check stayed green with `review` dropped from the list. Addresses CodeRabbit review threads on #320. --- .github/cursor-review/README.md | 13 ++- .../tests/test_workflow_panel_integrity.py | 90 ++++++++++++++++++- .github/workflows/cursor-review.yml | 37 +++++++- docs/callers/cursor-review.md | 33 +++++-- 4 files changed, 158 insertions(+), 15 deletions(-) diff --git a/.github/cursor-review/README.md b/.github/cursor-review/README.md index e8fb350..87a5582 100644 --- a/.github/cursor-review/README.md +++ b/.github/cursor-review/README.md @@ -57,7 +57,10 @@ 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 — see +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: @@ -71,7 +74,7 @@ The jobs, in the order they report: | ` ()` — 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 | The panel was short, findings went unanchored, nothing was delivered, or the judge never adjudicated. 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`/`Diff size check` 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 @@ -107,7 +110,11 @@ 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. Otherwise it prints one +no review delivered; the judge never adjudicated. It also goes red — rather than +skipping — when `gate` or `diff-size` *failed*, because every one of those +conditions reads a job output, an output is empty when the job producing it +failed, and a skipped required check passes: the same fail-closed guard the +Blocking gate carries. 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 diff --git a/.github/cursor-review/tests/test_workflow_panel_integrity.py b/.github/cursor-review/tests/test_workflow_panel_integrity.py index c86d0ee..6250552 100644 --- a/.github/cursor-review/tests/test_workflow_panel_integrity.py +++ b/.github/cursor-review/tests/test_workflow_panel_integrity.py @@ -48,9 +48,13 @@ 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. +# 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'", @@ -258,10 +262,16 @@ def test_it_publishes_the_documented_context_name(self): 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, - needs, + declared, f"`{PANEL_JOB}` dropped `{job}` from needs — its outputs then " "evaluate to the empty string, which every cause treats as a pass", ) @@ -344,5 +354,81 @@ def test_it_gates_nothing(self): ) +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) + + def test_the_job_runs_when_an_upstream_decision_job_failed(self): + # Without BOTH disjuncts the job is skipped on the failing path and the + # guard step below can never fire, however it is written. + for result in ("needs.gate.result != 'success'", "needs.diff-size.result != 'success'"): + 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_either_failure_and_exits_nonzero(self): + body = code_lines(step_named(self.panel, UNDECIDED_STEP)) + joined = "\n".join(body) + for result in ("needs.gate.result != 'success'", "needs.diff-size.result != 'success'"): + 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_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}`") + + if __name__ == "__main__": unittest.main(verbosity=2) diff --git a/.github/workflows/cursor-review.yml b/.github/workflows/cursor-review.yml index 99822e4..b715961 100644 --- a/.github/workflows/cursor-review.yml +++ b/.github/workflows/cursor-review.yml @@ -2675,9 +2675,22 @@ jobs: # 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` and `diff-size`. Every one of them is a + # job OUTPUT, 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. An undecided + # run is not a clean run. So the condition below runs this job whenever an + # upstream DECISION job 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. name: Panel integrity needs: [gate, diff-size, review, consolidate, post-review] - if: ${{ always() && needs.gate.outputs.should_run == 'true' && needs.gate.outputs.already_reviewed != 'true' && needs.diff-size.outputs.within_cap == 'true' && needs.review.result != 'skipped' }} + 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.review.result != 'skipped')))) }} runs-on: ubuntu-latest # Reads job outputs and prints. No API call, no checkout, no network. timeout-minutes: 5 @@ -2687,6 +2700,28 @@ jobs: # 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 + # two ways to arrive here with an upstream decision job unfinished: + # `gate` did not succeed, or `gate` selected a fresh in-scope review and + # `diff-size` did not succeed. Both 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.) + if: ${{ needs.gate.result != 'success' || needs.diff-size.result != 'success' }} + env: + GATE_RESULT: ${{ needs.gate.result }} + DIFF_SIZE_RESULT: ${{ needs.diff-size.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}). 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 }} diff --git a/docs/callers/cursor-review.md b/docs/callers/cursor-review.md index 7e7b011..c5bde93 100644 --- a/docs/callers/cursor-review.md +++ b/docs/callers/cursor-review.md @@ -226,9 +226,13 @@ step up in spend. Start label-gated. ## Panel integrity ` / Panel integrity` (with the caller above, `review / Panel -integrity`) is the context that answers **"was this PR actually reviewed by a -whole panel?"** — it is the one an automated merge gate should read, and it runs -on every review, with no input to turn on. +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 two +bullets at the end of this section 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 / @@ -270,12 +274,23 @@ Three more shapes to expect before you require it: 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. -* **A failed `Gate` job skips this check rather than failing it.** Panel - integrity is gated on the same four conditions the panel is, and all four read - `Gate`'s outputs — which are empty when that job failed. There is no panel to - report on in that case, and the `Gate` context is itself red, so the rollup - still carries the signal; the fail-closed guard for a *required* check lives on - the Blocking gate. +* **It is red, not skipped, when the decision itself failed.** Panel integrity + is gated on the same four conditions the panel is, and all four read `Gate`'s + and `Diff size check`'s job *outputs* — which are empty when those jobs + **failed**. Gating on them alone would skip this check exactly when a dup-check + API call errored or the diff could not be built, and GitHub counts a skipped + required check as **passing**. So a failed `Gate` or `Diff size check` runs + this job and fails it: an undecided run is not a clean run. +* **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". ## Blocking-gate gotchas From e4e287da0e265e24476bc01540f88f7b4ed58e22 Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Fri, 18 Sep 2026 20:50:44 +0000 Subject: [PATCH 3/9] fix(cursor-review): fail Panel integrity closed on a dead preflight and on absent producer results MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-ups on the panel-integrity check. Three fail-opens and one vacuous test, each verified by executing the affected `run:` block. A failed `preflight` skipped the check and minted a GREEN required context. `review` needs `preflight`, so a delisted model leaves the whole matrix 'skipped' — byte-identical to the deliberate no-panel branches the job correctly stays quiet on — and not one cell ever started. `preflight` joins `needs:`, the job `if:` admits its non-success under a within-cap fresh review, and the existing first guard turns that red. The same hole the gate/diff-size disjunct already closed, from the other end. Every cause read a job OUTPUT and treated the empty string as a pass, so a producer that died left a clean-looking report. `post-review.py` writes delivered=true the moment the POST returns and keeps working: executing the old block with delivered=true and post-review=failure exits 0 and prints "Panel integrity: 6/6 cells". `ok_count`/`total` are the sharper case — both empty on a dead `consolidate`, and unset-vs-unset compares EQUAL. Each cause is now paired with its producer's `result`, and an empty count is rejected explicitly rather than compared. The leg check caught json.JSONDecodeError but not UnicodeDecodeError, which `json.load` raises on the invalid UTF-8 an agent-writable file can hold — under `set -e` that traceback killed the step and lost the `::error::` naming the cell. Both subclass ValueError. The flatten assertion required a double quote immediately before the `$`, so it only ever inspected the already-safe `$(flatten "$VAR")` form; the unsafe `status=$JUDGE_STATUS` never matched while hits stayed non-zero from the safe occurrences. It now matches the bare form and checks every echoed line, since the runner parses workflow commands on all stdout. Docs: the decision-failure bullet claimed all four gate conditions read Gate's outputs — three read outputs, the fourth reads the matrix result, and preflight is why that matters. Adds the re-run-failed-jobs double-post warning (re-running a red leg re-runs consolidate and post-review while the cached already_reviewed=false is reused), scopes the leg check to an availability signal rather than an attestation a --trust cell cannot forge, and documents the inert incremental_subset cause. Verified: 623 cursor-review tests, 499 workflow-pins, 46 agents-md-integrity, check_workflow_pins.py and check_agents_md.py all pass. Seven workflow mutations each turn the suite red, including the unflattened echo the old pattern missed. Both `run:` blocks executed against their input states. Co-Authored-By: Claude Opus 5 --- .github/cursor-review/README.md | 21 +++- .../tests/test_workflow_panel_integrity.py | 91 +++++++++++++-- .github/workflows/cursor-review.yml | 110 ++++++++++++++---- docs/callers/cursor-review.md | 46 +++++++- 4 files changed, 224 insertions(+), 44 deletions(-) diff --git a/.github/cursor-review/README.md b/.github/cursor-review/README.md index 87a5582..ac918bf 100644 --- a/.github/cursor-review/README.md +++ b/.github/cursor-review/README.md @@ -74,7 +74,7 @@ The jobs, in the order they report: | ` ()` — 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` 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. | +| `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` 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 @@ -110,11 +110,20 @@ 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. It also goes red — rather than -skipping — when `gate` or `diff-size` *failed*, because every one of those -conditions reads a job output, an output is empty when the job producing it -failed, and a skipped required check passes: the same fail-closed guard the -Blocking gate carries. Otherwise it prints one +no review delivered; the judge never adjudicated; `consolidate` or `post-review` +did not succeed at all. It also goes red — rather than skipping — when `gate`, +`diff-size` or `preflight` *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 (`skipped` when `preflight` failed, since the matrix `needs:` it), +while a skipped required check passes: the same fail-closed guard the Blocking +gate carries. + +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 diff --git a/.github/cursor-review/tests/test_workflow_panel_integrity.py b/.github/cursor-review/tests/test_workflow_panel_integrity.py index 6250552..3defd40 100644 --- a/.github/cursor-review/tests/test_workflow_panel_integrity.py +++ b/.github/cursor-review/tests/test_workflow_panel_integrity.py @@ -68,6 +68,25 @@ # pass) and turning the check into a permanent green. UNTRUSTED_VALUES = ("OK_COUNT", "TOTAL", "JUDGE_STATUS", "DELIVERED", "UNGATED", "GATED") +# The three jobs whose non-success means "nobody decided whether to review this +# PR". `preflight` is the subtle one: the review matrix `needs:` it, so a failed +# preflight 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. +DECISION_RESULTS = ( + "needs.gate.result != 'success'", + "needs.diff-size.result != 'success'", + "needs.preflight.result != 'success'", +) + +# 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", @@ -322,20 +341,40 @@ def test_it_flattens_untrusted_values_before_annotating(self): # `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) + pattern = re.compile(r"\$\{?%s\b" % var) hits = 0 for line in self.body.split("\n"): - if "::" not in line: # only the annotation lines interpolate + 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( - line[: match.start()].endswith("flatten "), - f"`{PANEL_JOB}` interpolates ${var} into an annotation " - "without flatten(): a newline in it forges a workflow " - f"command\n {line.strip()}", + 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}") @@ -374,9 +413,9 @@ def setUp(self): self.assertIsNotNone(self.condition) def test_the_job_runs_when_an_upstream_decision_job_failed(self): - # Without BOTH disjuncts the job is skipped on the failing path and the + # 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 ("needs.gate.result != 'success'", "needs.diff-size.result != 'success'"): + for result in DECISION_RESULTS: self.assertIn( result, self.condition, @@ -402,10 +441,10 @@ def test_the_guard_step_exists_and_is_first(self): "the wrong question on a run that was never decided", ) - def test_the_guard_step_fires_on_either_failure_and_exits_nonzero(self): + 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 ("needs.gate.result != 'success'", "needs.diff-size.result != 'success'"): + for result in DECISION_RESULTS: self.assertIn( result, joined, @@ -421,6 +460,38 @@ def test_the_guard_step_fires_on_either_failure_and_exits_nonzero(self): ) self.assertIn("::error::", joined) + def test_it_needs_preflight_so_it_can_read_its_result(self): + # `needs.preflight.result` evaluates to the empty string unless the job + # is declared in `needs:` — and '' != 'success' is TRUE, so dropping it + # from the list would make this check red on EVERY run rather than + # failing open. Loud, but still wrong, and pinned so it stays declared. + declared = { + part.strip() + for part in (job_scalar(self.panel, "needs") or "").strip("[]").split(",") + if part.strip() + } + self.assertIn("preflight", declared) + + 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 diff --git a/.github/workflows/cursor-review.yml b/.github/workflows/cursor-review.yml index b715961..0bf95de 100644 --- a/.github/workflows/cursor-review.yml +++ b/.github/workflows/cursor-review.yml @@ -1761,6 +1761,15 @@ jobs: # 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 }} @@ -1773,13 +1782,21 @@ jobs: # ::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, json.JSONDecodeError): + except (OSError, ValueError): cell = None value = cell.get("status") if isinstance(cell, dict) else None if not isinstance(value, str): @@ -2677,20 +2694,25 @@ jobs: # 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` and `diff-size`. Every one of them is a - # job OUTPUT, 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. An undecided - # run is not a clean run. So the condition below runs this job whenever an - # upstream DECISION job 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. + # 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: `review` needs `preflight`, so a FAILED preflight 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`) 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. name: Panel integrity - needs: [gate, diff-size, 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.review.result != 'skipped')))) }} + needs: [gate, diff-size, preflight, 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.review.result != 'skipped'))))) }} runs-on: ubuntu-latest # Reads job outputs and prints. No API call, no checkout, no network. timeout-minutes: 5 @@ -2709,17 +2731,28 @@ jobs: # even decided on, which is a true statement about the wrong question. # # This condition is exact GIVEN the job `if:` above, which admits only - # two ways to arrive here with an upstream decision job unfinished: - # `gate` did not succeed, or `gate` selected a fresh in-scope review and - # `diff-size` did not succeed. Both 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.) - if: ${{ needs.gate.result != 'success' || needs.diff-size.result != 'success' }} + # 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` did not succeed. All three 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` is here because it is the THIRD decision job, and the one + # whose failure is invisible from the outputs: `review` needs it, so a + # red `preflight` 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` under a within-cap fresh review, and + # this step turns it red. A panel that never started is not a whole one. + if: ${{ needs.gate.result != 'success' || needs.diff-size.result != 'success' || needs.preflight.result != 'success' }} env: GATE_RESULT: ${{ needs.gate.result }} DIFF_SIZE_RESULT: ${{ needs.diff-size.result }} + PREFLIGHT_RESULT: ${{ needs.preflight.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}). Whether this PR was reviewed at all is unknown — re-run the failed job." + 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}). Whether this PR was reviewed at all is unknown — re-run the failed job." exit 1 - name: Report panel integrity @@ -2731,6 +2764,20 @@ jobs: 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 @@ -2747,7 +2794,26 @@ jobs: causes=0 - if [ "$OK_COUNT" != "$TOTAL" ]; then + # 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 diff --git a/docs/callers/cursor-review.md b/docs/callers/cursor-review.md index c5bde93..8f32c13 100644 --- a/docs/callers/cursor-review.md +++ b/docs/callers/cursor-review.md @@ -250,6 +250,8 @@ the job log: | 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 @@ -263,6 +265,16 @@ 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. + Three more shapes to expect before you require it: * **A cancelled run reports red.** GitHub counts a *skipped* required check as @@ -275,12 +287,15 @@ Three more shapes to expect before you require it: 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, and all four read `Gate`'s - and `Diff size check`'s job *outputs* — which are empty when those jobs - **failed**. Gating on them alone would skip this check exactly when a dup-check - API call errored or the diff could not be built, and GitHub counts a skipped - required check as **passing**. So a failed `Gate` or `Diff size check` runs - this job and fails it: an undecided run is not a clean run. + 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` + when `Preflight — validate model catalog` failed, because the matrix `needs:` + it. 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` **or `Preflight`** + runs this job and fails it: an undecided run is not a clean run. * **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 @@ -291,6 +306,25 @@ Three more shapes to expect before you require it: 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". +* **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 From 2bf8e31092dd811a91a3027a1387b29795c1f3a5 Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Fri, 18 Sep 2026 21:05:21 +0000 Subject: [PATCH 4/9] fix(cursor-review): fail Panel integrity closed on a dead ledger too MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `review` declares `needs: [gate, diff-size, preflight, ledger]`, and a non-success in ANY of those four skips the whole matrix — leaving `needs.review.result == 'skipped'`, byte-identical to the deliberate no-panel branches. `panel-integrity` guarded three of the four. A red `ledger` therefore skipped the check, and GitHub counts a skipped required check as PASSING: a green merge gate over a run where not one cell reviewed. Same fail-open as the `preflight` one closed a round earlier, one dependency over. `ledger` reads like it cannot fail — every step in it is `continue-on-error` precisely so it never skips the matrix — but that makes it rare, not unreachable: `Ensure ledger artifact exists` carries no `continue-on-error`, 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 job is not a step outcome at all. Its own job comment is written around this consequence. "Rare" is the wrong bar for a guard whose failure mode is handing out a green gate. `ledger` now joins `needs:`, the job `if:` and the guard step's condition and message, mirroring `preflight` exactly. Pinned as a RELATIONSHIP rather than a name list: the new `test_every_job_the_matrix_needs_is_guarded_here` reads `review`'s `needs:` and asserts every entry appears in `panel-integrity`'s `needs:`, its `if:` and the guard's `if:`. This fail-open was re-opened twice by a job being added to the matrix's dependencies and not here; pinning the two lists against each other is what makes the next one fail the suite instead. Four mutations verified red against it (dropping `ledger` from each of the three sites, and adding a phantom job to the matrix's `needs:`); control green. Docs corrected in the same commit: the README check-run table, its `Prior-review ledger` row (which claimed it "never fails the run"), and the caller guide's "red, not skipped, when the decision itself failed" bullet all enumerated the old three. --- .github/cursor-review/README.md | 20 +++-- .../tests/test_workflow_panel_integrity.py | 81 ++++++++++++++++--- .github/workflows/cursor-review.yml | 64 ++++++++++----- docs/callers/cursor-review.md | 16 ++-- 4 files changed, 135 insertions(+), 46 deletions(-) diff --git a/.github/cursor-review/README.md b/.github/cursor-review/README.md index ac918bf..f1f8cb3 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 | Never fails the run: the review matrix `needs:` it, so it degrades to an empty ledger rather than erroring. | +| `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` 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. | +| `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 @@ -111,12 +111,16 @@ 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 `gate`, -`diff-size` or `preflight` *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 (`skipped` when `preflight` failed, since the matrix `needs:` it), -while a skipped required check passes: the same fail-closed guard the Blocking -gate carries. +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 diff --git a/.github/cursor-review/tests/test_workflow_panel_integrity.py b/.github/cursor-review/tests/test_workflow_panel_integrity.py index 3defd40..32054ce 100644 --- a/.github/cursor-review/tests/test_workflow_panel_integrity.py +++ b/.github/cursor-review/tests/test_workflow_panel_integrity.py @@ -68,17 +68,29 @@ # pass) and turning the check into a permanent green. UNTRUSTED_VALUES = ("OK_COUNT", "TOTAL", "JUDGE_STATUS", "DELIVERED", "UNGATED", "GATED") -# The three jobs whose non-success means "nobody decided whether to review this -# PR". `preflight` is the subtle one: the review matrix `needs:` it, so a failed -# preflight 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. +# 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. @@ -411,6 +423,11 @@ def setUp(self): 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 @@ -460,17 +477,57 @@ def test_the_guard_step_fires_on_every_failure_and_exits_nonzero(self): ) self.assertIn("::error::", joined) - def test_it_needs_preflight_so_it_can_read_its_result(self): - # `needs.preflight.result` evaluates to the empty string unless the job - # is declared in `needs:` — and '' != 'success' is TRUE, so dropping it + 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 it stays declared. - declared = { + # 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.panel, "needs") or "").strip("[]").split(",") + for part in (job_scalar(self.jobs[MATRIX_JOB], "needs") or "") + .strip("[]") + .split(",") if part.strip() } - self.assertIn("preflight", declared) + 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 diff --git a/.github/workflows/cursor-review.yml b/.github/workflows/cursor-review.yml index 0bf95de..b28bce6 100644 --- a/.github/workflows/cursor-review.yml +++ b/.github/workflows/cursor-review.yml @@ -2700,19 +2700,27 @@ jobs: # 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: `review` needs `preflight`, so a FAILED preflight 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`) did not succeed, and `Fail if the panel decision itself did + # 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, 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.review.result != 'skipped'))))) }} + 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 @@ -2734,25 +2742,41 @@ jobs: # 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` did not succeed. All three are read back verbatim. - # (`within_cap` can only be the string 'true' when `diff-size` + # `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` is here because it is the THIRD decision job, and the one - # whose failure is invisible from the outputs: `review` needs it, so a - # red `preflight` 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` under a within-cap fresh review, and - # this step turns it red. A panel that never started is not a whole one. - if: ${{ needs.gate.result != 'success' || needs.diff-size.result != 'success' || needs.preflight.result != 'success' }} + # `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}). Whether this PR was reviewed at all is unknown — re-run the failed job." + 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 diff --git a/docs/callers/cursor-review.md b/docs/callers/cursor-review.md index 8f32c13..b701fcf 100644 --- a/docs/callers/cursor-review.md +++ b/docs/callers/cursor-review.md @@ -290,12 +290,16 @@ Three more shapes to expect before you require it: 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` - when `Preflight — validate model catalog` failed, because the matrix `needs:` - it. 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` **or `Preflight`** - runs this job and fails it: an undecided run is not a clean run. + 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 From 73557d76fbd81f2888483f17f97dcaafca49cd4c Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Fri, 18 Sep 2026 21:31:42 +0000 Subject: [PATCH 5/9] docs(cursor-review): name the Panel integrity bullets instead of counting them MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two stale positional cross-references in the setup guide's `Panel integrity` section, both left behind by the list growing across the three review rounds: - "Three more shapes to expect before you require it:" introduced SIX bullets. It was accurate when written; `red, not skipped, when the decision itself failed`, `it detects a cell that went missing` and `one listed cause is wired but inert` were each added after it. - "Read the two bullets at the end of this section" pointed at the two bullets that describe red-vs-skipped — which are now bullets three and four of six, not the last two. A caller following that instruction today reads the forgeability limit and the inert `incremental_subset` cause instead of the two shapes the sentence goes on to summarize, and those two are exactly the ones that decide whether requiring this check does what they expect. Fixed the way the same problem was fixed in the workflow a commit earlier: pin the RELATIONSHIP, not the count. The intro now names the two bullets it means, so it survives the list growing again, and the lead-in drops the number rather than restating it as "six" for the next round to falsify. Docs-only, and deliberately NO `Skip-caller-bump: true` trailer: that trailer declares the WHOLE squashed PR bump-irrelevant for every fleet, and this PR carries the behavioral `cursor-review.yml` change that every caller does need to bump to. Verified on the patched tree: cursor-review 624 passed, workflow-pins 499 passed / 32 skipped, agents-md-integrity 46 passed; `check_workflow_pins.py`, `check_agents_md.py --root .` and `check-org-repo-literals.sh` all rc=0. No test couples to either edited string. Co-Authored-By: Claude Opus 5 --- docs/callers/cursor-review.md | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/docs/callers/cursor-review.md b/docs/callers/cursor-review.md index b701fcf..17d1322 100644 --- a/docs/callers/cursor-review.md +++ b/docs/callers/cursor-review.md @@ -228,9 +228,10 @@ step up in spend. Start label-gated. ` / 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 two -bullets at the end of this section before you require it: it is red when the -review ran and came up short *and* when the decision that selects a review +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**. @@ -275,7 +276,7 @@ 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. -Three more shapes to expect before you require it: +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 From 3d0b4775d980e2168287e99ef8655b4e755dac35 Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Sat, 19 Sep 2026 04:58:05 +0000 Subject: [PATCH 6/9] docs(cursor-review): caveat that a later no-panel run supersedes a red Panel integrity MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The setup guide tells a caller to require `Panel integrity` as a merge gate and lists the shapes to expect before they do, but not this one: the job's `if:` is gated on a panel being warranted, so any later run on the SAME head SHA that takes a deliberate no-panel branch publishes a newer check run in the `skipped` state, and branch protection reads the latest check run of a name while counting a skip as a pass. A red verdict is therefore cleared by a toggled label, an applied `skip-cursor-review`, a removed trigger label, or the already-reviewed re-trigger — with the panel still short and nothing re-reviewed. The guide's own recommended remediation is the sharpest case: it says to prefer re-running the whole workflow over re-run-failed-jobs, and that fresh `Gate` dup-check finds the consolidated review already landed at that SHA, reports `already_reviewed=true`, and skips this job. So the page recommended the action that turns the red green. Documents the caveat on both surfaces and points at pushing a new commit as the way to clear a red instead. No behaviour change: closing the hole needs prior state from outside the run (the previous conclusion, or the landed review's cell count) and a credential this job deliberately holds none of, which is a design decision tracked as BE-15604 rather than a patch for this PR. --- .github/cursor-review/README.md | 11 ++++++++++- docs/callers/cursor-review.md | 20 +++++++++++++++++++- 2 files changed, 29 insertions(+), 2 deletions(-) diff --git a/.github/cursor-review/README.md b/.github/cursor-review/README.md index f1f8cb3..d35bd4d 100644 --- a/.github/cursor-review/README.md +++ b/.github/cursor-review/README.md @@ -131,7 +131,16 @@ 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. See [the setup +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 diff --git a/docs/callers/cursor-review.md b/docs/callers/cursor-review.md index 17d1322..acc04da 100644 --- a/docs/callers/cursor-review.md +++ b/docs/callers/cursor-review.md @@ -274,7 +274,9 @@ 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. +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: @@ -311,6 +313,22 @@ The remaining shapes to expect before you require it: 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 From 4b6c9a8d80b5579a5ae1696e8050f8929f94e715 Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Sat, 19 Sep 2026 17:21:15 +0000 Subject: [PATCH 7/9] test(cursor-review): pin `fail-fast: false` so a red leg cannot cancel the panel MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The leg check's premise — "failing here costs the panel nothing" — has two halves. The local half (fail AFTER the artifact upload, keep the run step's `continue-on-error`) is pinned by the tests beside this one. The non-local half is not: with `fail-fast` 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 — six cells landing as one red leg and five cancelled ones, with `Aggregate panel findings` undercounting a matrix that was merely short before. That setting was inert to the panel until this branch: a cell that never submitted used to exit GREEN, so nothing in the matrix ever failed deliberately. This change is what makes it load-bearing, so pin it rather than assume it. Test only — no workflow or behaviour change. Verified by mutation: flipping `fail-fast` to true fails the new test and nothing else. Co-Authored-By: Claude Opus 5 --- .../tests/test_workflow_panel_integrity.py | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/.github/cursor-review/tests/test_workflow_panel_integrity.py b/.github/cursor-review/tests/test_workflow_panel_integrity.py index 32054ce..794e554 100644 --- a/.github/cursor-review/tests/test_workflow_panel_integrity.py +++ b/.github/cursor-review/tests/test_workflow_panel_integrity.py @@ -252,6 +252,25 @@ def test_the_run_step_still_absorbs_its_own_cap(self): 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): From bec0638e5b4463f04e427c40d620b134924a72fd Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Sat, 19 Sep 2026 17:43:37 +0000 Subject: [PATCH 8/9] fix(cursor-review): percent-escape untrusted values in Panel integrity annotations MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `flatten()` deleted CR/LF before interpolating a value into an `::error::` line, on the stated reasoning that "a newline would forge a second command". That is only half the defence. The runner UNESCAPES `%25`, `%0D` and `%0A` in a workflow command's message before rendering it, so a value carrying the six literal characters `%0A` arrives perfectly single-line — `tr -d` is a no-op on it — and the runner puts the newline back, forging exactly the second command the function exists to prevent. `JUDGE_STATUS` is the reachable one, and it is reachable today. It is whatever string the judge agent's own tool wrote as `status` into /tmp/judge-findings.json, read out with a bare `.get('status','?')` in a job running `cursor-agent --trust` over PR-authored text: no enum validation, no escaping on the way out, and it flows through a job output straight into this annotation. A judge writing `error%0A::stop-commands::tok` — 27 characters, well under the 64-char clamp — lands a live `::stop-commands::` on the check run and suppresses the real causes echoed after it. The other values cannot carry a `%`: `ok_count`/`total` are python-formatted integers and the rest are job results. This does not let a panel forge the check's CONCLUSION — that is driven by the `causes` counter and the step's exit code, not by annotations — so it is a reporting-integrity fix, not a merge-gate one. It is also strictly narrower than the forgeability limit this PR already documents. Fixing it anyway because it is one `sed` and because every other checker in this repo already applies precisely this escape before annotating: `check_agents_md.py`, `check_workflow_pins.py`, `check_public_repo_hygiene.py`, `check_coderabbit_config.py` and, in shell, `check-org-repo-literals.sh`. The new code was the one annotating surface that did not. Order is load-bearing at both ends: the escape runs AFTER `tr`, so a real newline is dropped rather than escaped back into one, and AFTER `cut`, so the clamp cannot split a `%25` it just created. Also corrects the `judge_status` job-output comment, which told consumers to flatten the value and stopped there. Verified: the real `flatten()` line is extracted from the workflow and executed under bash against adversarial input, rather than pattern-matched; the whole `Report panel integrity` block was executed with `JUDGE_STATUS=error%0A::stop-commands::pwn` and emits the inert `%250A`. Mutation-tested — reverting the escape (3 failures), moving it before the clamp (1) and dropping the newline strip (2) each turn the suite red against a green 30-test control, and the tree restores byte-identical. Full suites on the merged tree: cursor-review 630 passed, workflow-pins 499 passed / 32 skipped, agents-md-integrity 46 passed, refresh-reviewers 108 passed; `check_workflow_pins.py`, `check_agents_md.py` and `check-org-repo-literals.sh` all rc=0. Co-Authored-By: Claude Opus 5 --- .../tests/test_workflow_panel_integrity.py | 115 ++++++++++++++++++ .github/workflows/cursor-review.yml | 29 ++++- 2 files changed, 140 insertions(+), 4 deletions(-) diff --git a/.github/cursor-review/tests/test_workflow_panel_integrity.py b/.github/cursor-review/tests/test_workflow_panel_integrity.py index 794e554..83df238 100644 --- a/.github/cursor-review/tests/test_workflow_panel_integrity.py +++ b/.github/cursor-review/tests/test_workflow_panel_integrity.py @@ -28,6 +28,8 @@ import os import re +import shutil +import subprocess import unittest WORKFLOW = os.path.normpath( @@ -577,5 +579,118 @@ def test_the_deliberate_skips_are_still_skips(self): 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 b28bce6..bac8f57 100644 --- a/.github/workflows/cursor-review.yml +++ b/.github/workflows/cursor-review.yml @@ -1867,9 +1867,13 @@ jobs: ok_count: ${{ steps.aggregate.outputs.ok_count }} total: ${{ steps.aggregate.outputs.total }} degraded: ${{ steps.consolidated.outputs.degraded }} - # Untrusted: written by the judge agent's own tool output. Anything that - # consumes it must flatten it before it reaches a ::workflow command:: - # line — `Panel integrity` does. + # 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 @@ -2814,7 +2818,24 @@ 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. - flatten() { printf '%s' "$1" | tr -d '\r\n' | cut -c1-64; } + # + # 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 From 0ca980056677fb33cbf14fe9e8f3fb33db67388c Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Sat, 19 Sep 2026 17:59:40 +0000 Subject: [PATCH 9/9] docs(cursor-review): carry Panel integrity into the public catalog row MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The three doc surfaces this PR updated are the two it owns — the setup guide and `.github/cursor-review/README.md` — plus the workflow comments. The ROOT README is the public catalog, and its `cursor-review.yml` row still enumerated the check surface as it was before this branch: advisory, with `blocking: true` as the one switch that adds a check. After this PR that row is wrong by omission in the two ways a consumer deciding whether to bump would most want to know about: - there is now a second, ALWAYS-ON check context (`Panel integrity`), and it is the one a completeness gate should require — not a panel leg, whose context name carries the model id and moves when the pins do; - the caller job goes RED on a measured ~40% of runs, because a cell that did not submit now fails its own leg instead of exiting green. That is the single most surprising consequence of this branch, and the catalog is where an adopter meets the workflow first. Both are already written up in `docs/callers/cursor-review.md`; this is the catalog-level summary plus the pointer, not a second copy of it. The skip semantics are stated in the same breath, because "skipped counts as passing" is what makes a half-read of this feature unsafe: a skip answers "no panel was warranted", never "the panel was fine". The forgeability and same-SHA supersession limits are named and pointed at rather than restated, so the catalog cannot drift from the guide on the details — AGENTS.md's standing warning about this file having drifted once already. Docs only, root README only. Deliberately NO `Skip-caller-bump: true` trailer: the trailer declares the WHOLE PR bump-irrelevant for every fleet, and this PR is behavioral. It would also be moot — the cursor-review fleet watches `.github/workflows/cursor-review.yml`, `.github/cursor-review/**` and `scripts/check-pr-size`, none of which is the root README. Verified: table row still parses (4 cells, matching the header and separator rows) and the added prose contains no `|`; cursor-review 630 passed, workflow-pins 499 passed / 32 skipped, agents-md-integrity 46 passed; `check_workflow_pins.py`, `check_agents_md.py` and `check-org-repo-literals.sh` all rc=0. The ~40% figure is the one already sourced in the workflow comment and the guide (38 of 92 runs had at least one errored leg, BE-15554). Co-Authored-By: Claude Opus 5 --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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) |