[PR #236] [MERGED] ci(cursor-review): post the review from a job that never checks out PR code #242

New issue

Closed

opened 2026-10-26 22:37:42 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-26 22:37:42 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/Comfy-Org/github-workflows/pull/236
Author: @mattmillerai
Created: 8/27/2026
Status: ✅ Merged
Merged: 8/27/2026
Merged by: @mattmillerai

Base: main ← Head: matt/be-9569-split-post-review-job

📝 Commits (2)

95d9cf8 ci(cursor-review): post the review from a job that never checks out PR code
6b08fb7 fix(cursor-review): close the post-review gate and harden the isolation test (BE-9569)

📊 Changes

10 files changed (+711 additions, -44 deletions)

View changed files

📝 .github/cursor-review/README.md (+15 -0)
📝 .github/cursor-review/build-ledger.py (+8 -6)
➕ .github/cursor-review/tests/test_workflow_job_isolation.py (+350 -0)
📝 .github/workflow-pins/tests/test_check_workflow_pins.py (+5 -2)
📝 .github/workflows/cursor-review.yml (+310 -22)
📝 .github/workflows/test-cursor-review-scripts.yml (+5 -4)
📝 CONTRIBUTING.md (+7 -4)
📝 README.md (+1 -1)
📝 SECURITY.md (+7 -4)
📝 docs/callers/cursor-review.md (+3 -1)

📄 Description

ELI-5

The judge model that writes our PR reviews used to run in the same job that held the review bot's private key. That judge reads the PR's diff — attacker-authored text — and Cursor's --print mode can use write and shell tools, so a diff that talked the judge into rewriting a script on disk would have been rewriting a script that ran moments later with a GitHub App token in the environment. This moves the token mint and the review post into their own job on a fresh runner that never checks out the PR. Nothing about the review a user sees changes.

What changed

consolidate and post-review are now two jobs, the same shape pr-size.yml already uses for its comment job and cursor-review-catalog-drift.yml uses for its issues: write job.

consolidate keeps the fetch-depth: 0 PR checkout and the judge, and drops pull-requests: write — it is contents: read only now. Two new steps at the end write a small meta JSON (judge_status, consolidated_count, degraded, triggered_by, and the ledger download's outcome) and upload it alongside consolidated.json + judge-findings.json as the cursor-review-consolidated artifact.
post-review (new) does no actions/checkout of the PR repo — only Comfy-Org/github-workflows@${{ inputs.workflows_ref }} with persist-credentials: false, behind the same Require a pinned workflows_ref guard every other checkout site carries. It downloads the payload, mints the App token, renders the ledger note and runs post-review.py exactly as before. permissions: contents: read + pull-requests: write, timeout-minutes: 10.
The reviewed diff post-review.py anchors inline comments against is downloaded from diff-size's own pr-diff artifact, not re-uploaded from the judge's job — so the copy the poster reads is the one the panel reviewed, not one the judge had shell access to.
The judge's step cap stays where it was; the job cap stays 40m on consolidate.

Verified: post-review.py shells out to gh and nothing else — no git, no dependence on a PR checkout. Every value it needs arrives as a CLI arg or via the artifact.

The invariant, stated once: no job in cursor-review.yml both checks out PR code and holds a write-scoped credential. .github/cursor-review/tests/test_workflow_job_isolation.py (new, stdlib-only, 9 tests) pins that structurally — it parses the workflow, classifies each job by whether any actions/checkout step resolves to something other than the pinned assets repo, and asserts no such job carries a write-scoped permissions: grant or references BOT_APP_PRIVATE_KEY. Every assertion was confirmed non-vacuous by injecting the matching regression and watching it fire.

Carried along, because the split makes them wrong otherwise:

notify-complete's "One consolidated review is on the PR" Slack DM now keys on the post job's result rather than consolidate's, and distinguishes the two failures — a green consolidate no longer justifies claiming the review landed.
The guarded-ref-checkout census in .github/workflow-pins/tests/ moves 16 → 17 for the new job's assets checkout. check_workflow_pins.py confirms it carries its guard (0 exempt).
SECURITY.md, CONTRIBUTING.md and build-ledger.py's two docstrings each carried an explicit statement of the pre-split boundary ("It is not absolute for cursor-review's judge…", "a workflow whose consolidate job holds pull-requests: write", "consolidate renders a banner"). All four now describe the split.
Doc sync: root README.md catalog row, .github/cursor-review/README.md, docs/callers/cursor-review.md (the "don't use Consolidate panel as a required check" gotcha now says the same of Post review, which needs: it and skips in all the same cases plus the failure cases).

Judgment calls

if: !cancelled() && needs.consolidate.result == 'success'. Both halves are load-bearing. !cancelled() rather than always() because the documented caller sets cancel-in-progress: true and always() runs a job even when the run is cancelled — in the common supersede ordering consolidate has already finished and uploaded, so a needs-level != 'cancelled' test passes and the poster would post against a superseded head SHA alongside the new run's review. Splitting the post into its own job widened that window, since cancellation used to kill the post step mid-job. Same reasoning over-cap-comment already documents for its own !cancelled(). And == 'success' rather than "ran at all" because this job's entire input is a downloaded artifact: artifact names are run-global, a prompt-injected panel cell can claim cursor-review-consolidated first, v4+ immutability then fails the real upload and reds consolidate — so a gate that accepted a failed consolidate would post the spoofed payload under the bot's write token.
A failed consolidate skips this job rather than producing a second red one. Consequence of the above, and a reversal of this PR's first draft. The failure stays visible as consolidate's own red status and in notify-complete's DM, which names which half broke; there was never anything worth posting on that path. Require the consolidated payload is still there as the diagnostic for a transient download failure, and now also asserts each of the three expected files landed — if-no-files-found: error fires only when ZERO paths match, so a partial bundle uploads clean. A judge that never submitted is not this case: its step is continue-on-error, so consolidate still succeeds and the degraded panel-union review still posts.
Ledger note. Rather than re-uploading ledger-note.txt out of the judge's job, post-review downloads the pristine cursor-review-ledger artifact itself. The banner's delivery check then covers both questions instead of one, and reports them separately, because they mean different things to the author: if the judge's download failed the panel really did review without prior-round context ("findings may repeat earlier rounds"); if only the poster's download failed — a case that exists only because this job fetches the note itself — the panel and judge had the context and only the header could not be rendered, so saying "findings may repeat" there would be false.
triggered_by is validated on the way out of the meta file (^[A-Za-z0-9-]{1,39}(\[bot\])?$, [bot] because Resolve triggerer legitimately emits it). It is the one review-header field post-review.py interpolates raw rather than through neutralize_mentions, which was safe while the value came from github.actor in-job and stopped being safe once it started arriving out of the prompt-injectable job. A rejected value degrades to no attribution line, not to a wrong one.
needs: [consolidate, ledger] rather than also gate. Nothing in the job reads gate's outputs, and a skipped gate already skips consolidate, which skips this. ledger is needed for needs.ledger.outputs.status.
No consumer callers are bumped here; the bumper workflow moves the SHA and consumers pick this up on their next bump.
No capability is denied by this change — the review still posts, on every path it posted on before. The one behaviour change on a failure path (a skipped post-review instead of a red one) fires only where the old code already produced no review.

Residual

Acceptance criterion 1 is met in substance but not as literally worded. The ticket asks that grep -n show secrets.BOT_APP_PRIVATE_KEY only in post-review. It also appears in over-cap-comment, a job that did not exist when the ticket was written. That job checks out nothing at all, so the boundary the criterion protects holds — but if you grep expecting one job, you will find two. The test asserts the accurate form (holders == {over-cap-comment, post-review}, neither with a PR checkout) rather than the literal one.
notify-complete's success DM can still overstate what landed, and this is deferred to a follow-up rather than fixed here. post-review.py deliberately exits 0 on a read-only-token 403 after writing the review only to $GITHUB_STEP_SUMMARY (and does the same on several other POST_FAILED_SUMMARY_NOTE paths), so a green post-review job does not strictly mean the review is on the PR. This PR did not introduce the gap — keying the DM on consolidate beforehand was wrong in exactly the same way — it only moved which job the DM reads. Closing it means changing post-review.py's exit contract to emit a posted-versus-degraded value, surfacing it as a job output, and extending test_post_review.py; that is a separate concern from the isolation boundary.
The end-to-end criterion is unexercised: no labeled PR on a consumer repo has run this code. It cannot be, before merge. This repo's own ci-cursor-review.yml deliberately pins a merged-main SHA rather than a local path (AGENTS.md: "do not 'simplify' them to a path"), so the cursor-review run on this PR executes the old, unsplit workflow; no consumer can reach the new shape until this lands and their SHA bump merges. What has been verified instead: the workflow parses, actionlint reports no new finding, every run: block passes bash -n and shellcheck, all three new/changed inline step bodies were executed locally against the real text extracted from the YAML (the payload-completeness guard, all 10 LEDGER_NOTE branches, and 10 triggered_by inputs including markdown-link and team-mention attempts), and every isolation assertion was fired by injecting its regression. The first real consumer run after the bump is the acceptance test, and the thing to watch is the Post review job: an artifact-plumbing mistake surfaces there as a red job with no review posted, not as a silently wrong review.
The ticket's plan referenced a file state this repo has moved past (it cites @676fbb6 L1503–1900, --approve-mcps, and a ledger-note.txt upload). Two plan items turned out to be no-ops against the current tree and were not done: post-review.py needs no change (it never shells out to git), and tests/test_wire_bot_identity.py asserts on the caller-wiring helper, not on job names or steps, so there was nothing in it to update. The new isolation suite is what covers the job structure instead.
.github/coderabbit-config's suite was not run: it needs jsonschema from its pinned requirements.txt, which is not installed in this environment. That directory is untouched by this diff, so this is a not-run, not a known failure.
Not measured, and worth knowing before this fans out: the four other reusables in this repo that mint an App token were not audited for the same PR-checkout-plus-write-token shape. The new test is scoped to cursor-review.yml alone. groom.yml is documented as already credential-split and pr-size.yml is the pattern this copies, but neither was re-verified here, and pr-area-label.yml, pr-derisk.yml and linear-ticket.yml were not looked at.

Provenance

Authored by: agent-work loop
Verified: python3 -m unittest discover -s .github/cursor-review/tests: 314 passed, 0 failed (305 on base, +9 isolation tests); .github/workflow-pins/tests: 308 passed; .github/groom: 365; .github/public-repo-hygiene: 196; .github/refresh-reviewers: 48; .github/agents-md-integrity: 46 — all OK. python3 .github/workflow-pins/check_workflow_pins.py: OK, 11 workflows, 0 exempt; python3 .github/agents-md-integrity/check_agents_md.py --root .: passed (1 pre-existing CODEOWNERS warning); actionlint .github/workflows/cursor-review.yml: 1 finding, byte-identical to the one on origin/main (job.workflow_sha, in the untouched ledger job); shellcheck + bash -n clean on every run: block in the file. Isolation suite proven non-vacuous against 7 injected regressions (permissions block deleted; pull-requests: 'write' # comment restored on consolidate; permissions: write-all; a workflow-level grant; the assets checkout switched to ${{ github.repository }}; a run:-block forgery of the repository: exemption; the judge invocation removed).
Deviations: see "Judgment calls" and "Residual" above — chiefly that acceptance criterion 1 holds in substance but names one job where the tree now has two; that a failed consolidate now skips the poster rather than reddening it; that the notify-complete DM's posted-versus-degraded gap is deferred to a follow-up; and that the end-to-end consumer-run criterion cannot be exercised before merge.

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/Comfy-Org/github-workflows/pull/236 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 8/27/2026 **Status:** ✅ Merged **Merged:** 8/27/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/be-9569-split-post-review-job` --- ### 📝 Commits (2) - [`95d9cf8`](https://github.com/Comfy-Org/github-workflows/commit/95d9cf8be93d87062e74f8efe6c7089fb2ea0ebd) ci(cursor-review): post the review from a job that never checks out PR code - [`6b08fb7`](https://github.com/Comfy-Org/github-workflows/commit/6b08fb77db9c3123679678ce338c8ddcb200ee41) fix(cursor-review): close the post-review gate and harden the isolation test (BE-9569) ### 📊 Changes **10 files changed** (+711 additions, -44 deletions) <details> <summary>View changed files</summary> 📝 `.github/cursor-review/README.md` (+15 -0) 📝 `.github/cursor-review/build-ledger.py` (+8 -6) ➕ `.github/cursor-review/tests/test_workflow_job_isolation.py` (+350 -0) 📝 `.github/workflow-pins/tests/test_check_workflow_pins.py` (+5 -2) 📝 `.github/workflows/cursor-review.yml` (+310 -22) 📝 `.github/workflows/test-cursor-review-scripts.yml` (+5 -4) 📝 `CONTRIBUTING.md` (+7 -4) 📝 `README.md` (+1 -1) 📝 `SECURITY.md` (+7 -4) 📝 `docs/callers/cursor-review.md` (+3 -1) </details> ### 📄 Description ## ELI-5 The judge model that writes our PR reviews used to run in the same job that held the review bot's private key. That judge reads the PR's diff — attacker-authored text — and Cursor's `--print` mode can use write and shell tools, so a diff that talked the judge into rewriting a script on disk would have been rewriting a script that ran moments later *with a GitHub App token in the environment*. This moves the token mint and the review post into their own job on a fresh runner that never checks out the PR. Nothing about the review a user sees changes. ## What changed `consolidate` and `post-review` are now two jobs, the same shape [`pr-size.yml`](.github/workflows/pr-size.yml) already uses for its `comment` job and [`cursor-review-catalog-drift.yml`](.github/workflows/cursor-review-catalog-drift.yml) uses for its `issues: write` job. - **`consolidate`** keeps the `fetch-depth: 0` PR checkout and the judge, and **drops `pull-requests: write`** — it is `contents: read` only now. Two new steps at the end write a small meta JSON (`judge_status`, `consolidated_count`, `degraded`, `triggered_by`, and the ledger download's outcome) and upload it alongside `consolidated.json` + `judge-findings.json` as the `cursor-review-consolidated` artifact. - **`post-review`** (new) does **no `actions/checkout` of the PR repo** — only `Comfy-Org/github-workflows@${{ inputs.workflows_ref }}` with `persist-credentials: false`, behind the same `Require a pinned workflows_ref` guard every other checkout site carries. It downloads the payload, mints the App token, renders the ledger note and runs `post-review.py` exactly as before. `permissions: contents: read` + `pull-requests: write`, `timeout-minutes: 10`. - The reviewed diff `post-review.py` anchors inline comments against is downloaded from `diff-size`'s own `pr-diff` artifact, **not** re-uploaded from the judge's job — so the copy the poster reads is the one the panel reviewed, not one the judge had shell access to. - The judge's step cap stays where it was; the job cap stays 40m on `consolidate`. Verified: `post-review.py` shells out to `gh` and nothing else — no `git`, no dependence on a PR checkout. Every value it needs arrives as a CLI arg or via the artifact. **The invariant, stated once:** no job in `cursor-review.yml` both checks out PR code and holds a write-scoped credential. `.github/cursor-review/tests/test_workflow_job_isolation.py` (new, stdlib-only, 9 tests) pins that structurally — it parses the workflow, classifies each job by whether any `actions/checkout` step resolves to something other than the pinned assets repo, and asserts no such job carries a write-scoped `permissions:` grant or references `BOT_APP_PRIVATE_KEY`. Every assertion was confirmed non-vacuous by injecting the matching regression and watching it fire. Carried along, because the split makes them wrong otherwise: - `notify-complete`'s "One consolidated review is on the PR" Slack DM now keys on the **post** job's result rather than `consolidate`'s, and distinguishes the two failures — a green consolidate no longer justifies claiming the review landed. - The guarded-ref-checkout census in `.github/workflow-pins/tests/` moves 16 → 17 for the new job's assets checkout. `check_workflow_pins.py` confirms it carries its guard (`0 exempt`). - `SECURITY.md`, `CONTRIBUTING.md` and `build-ledger.py`'s two docstrings each carried an explicit statement of the *pre-split* boundary ("It is **not** absolute for cursor-review's judge…", "a workflow whose `consolidate` job holds `pull-requests: write`", "consolidate renders a banner"). All four now describe the split. - Doc sync: root `README.md` catalog row, `.github/cursor-review/README.md`, `docs/callers/cursor-review.md` (the "don't use `Consolidate panel` as a required check" gotcha now says the same of `Post review`, which `needs:` it and skips in all the same cases plus the failure cases). ## Judgment calls - **`if: !cancelled() && needs.consolidate.result == 'success'`.** Both halves are load-bearing. `!cancelled()` rather than `always()` because the documented caller sets `cancel-in-progress: true` and `always()` runs a job even when the *run* is cancelled — in the common supersede ordering `consolidate` has already finished and uploaded, so a needs-level `!= 'cancelled'` test passes and the poster would post against a superseded head SHA alongside the new run's review. Splitting the post into its own job widened that window, since cancellation used to kill the post step mid-job. Same reasoning `over-cap-comment` already documents for its own `!cancelled()`. And `== 'success'` rather than "ran at all" because this job's entire input is a downloaded artifact: artifact names are run-global, a prompt-injected panel cell can claim `cursor-review-consolidated` first, v4+ immutability then fails the *real* upload and reds `consolidate` — so a gate that accepted a failed consolidate would post the spoofed payload under the bot's write token. - **A failed `consolidate` skips this job rather than producing a second red one.** Consequence of the above, and a reversal of this PR's first draft. The failure stays visible as `consolidate`'s own red status and in `notify-complete`'s DM, which names which half broke; there was never anything worth posting on that path. `Require the consolidated payload` is still there as the diagnostic for a transient download failure, and now also asserts each of the three expected files landed — `if-no-files-found: error` fires only when ZERO paths match, so a partial bundle uploads clean. A judge that never submitted is **not** this case: its step is `continue-on-error`, so `consolidate` still succeeds and the degraded panel-union review still posts. - **Ledger note.** Rather than re-uploading `ledger-note.txt` out of the judge's job, `post-review` downloads the pristine `cursor-review-ledger` artifact itself. The banner's delivery check then covers **both** questions instead of one, and reports them **separately**, because they mean different things to the author: if the *judge's* download failed the panel really did review without prior-round context ("findings may repeat earlier rounds"); if only the *poster's* download failed — a case that exists only because this job fetches the note itself — the panel and judge had the context and only the header could not be rendered, so saying "findings may repeat" there would be false. - **`triggered_by` is validated on the way out of the meta file** (`^[A-Za-z0-9-]{1,39}(\[bot\])?$`, `[bot]` because `Resolve triggerer` legitimately emits it). It is the one review-header field `post-review.py` interpolates raw rather than through `neutralize_mentions`, which was safe while the value came from `github.actor` in-job and stopped being safe once it started arriving out of the prompt-injectable job. A rejected value degrades to no attribution line, not to a wrong one. - **`needs: [consolidate, ledger]`** rather than also `gate`. Nothing in the job reads `gate`'s outputs, and a skipped `gate` already skips `consolidate`, which skips this. `ledger` is needed for `needs.ledger.outputs.status`. - **No consumer callers are bumped here**; the bumper workflow moves the SHA and consumers pick this up on their next bump. - No capability is denied by this change — the review still posts, on every path it posted on before. The one behaviour change on a failure path (a skipped `post-review` instead of a red one) fires only where the old code already produced no review. ## Residual - **Acceptance criterion 1 is met in substance but not as literally worded.** The ticket asks that `grep -n` show `secrets.BOT_APP_PRIVATE_KEY` *only* in `post-review`. It also appears in **`over-cap-comment`**, a job that did not exist when the ticket was written. That job checks out nothing at all, so the boundary the criterion protects holds — but if you grep expecting one job, you will find two. The test asserts the accurate form (`holders == {over-cap-comment, post-review}`, neither with a PR checkout) rather than the literal one. - **`notify-complete`'s success DM can still overstate what landed**, and this is **deferred to a follow-up rather than fixed here**. `post-review.py` deliberately exits 0 on a read-only-token 403 after writing the review only to `$GITHUB_STEP_SUMMARY` (and does the same on several other `POST_FAILED_SUMMARY_NOTE` paths), so a green `post-review` job does not strictly mean the review is on the PR. This PR did not introduce the gap — keying the DM on `consolidate` beforehand was wrong in exactly the same way — it only moved which job the DM reads. Closing it means changing `post-review.py`'s exit contract to emit a posted-versus-degraded value, surfacing it as a job output, and extending `test_post_review.py`; that is a separate concern from the isolation boundary. - **The end-to-end criterion is unexercised: no labeled PR on a consumer repo has run this code.** It cannot be, before merge. This repo's own `ci-cursor-review.yml` deliberately pins a merged-`main` SHA rather than a local path (AGENTS.md: "do not 'simplify' them to a path"), so the cursor-review run on *this* PR executes the **old**, unsplit workflow; no consumer can reach the new shape until this lands and their SHA bump merges. What has been verified instead: the workflow parses, `actionlint` reports no new finding, every `run:` block passes `bash -n` and `shellcheck`, all three new/changed inline step bodies were executed locally against the real text extracted from the YAML (the payload-completeness guard, all 10 `LEDGER_NOTE` branches, and 10 `triggered_by` inputs including markdown-link and team-mention attempts), and every isolation assertion was fired by injecting its regression. **The first real consumer run after the bump is the acceptance test**, and the thing to watch is the `Post review` job: an artifact-plumbing mistake surfaces there as a red job with no review posted, not as a silently wrong review. - **The ticket's plan referenced a file state this repo has moved past** (it cites `@676fbb6` L1503–1900, `--approve-mcps`, and a `ledger-note.txt` upload). Two plan items turned out to be no-ops against the current tree and were not done: `post-review.py` needs no change (it never shells out to `git`), and `tests/test_wire_bot_identity.py` asserts on the *caller-wiring helper*, not on job names or steps, so there was nothing in it to update. The new isolation suite is what covers the job structure instead. - **`.github/coderabbit-config`'s suite was not run**: it needs `jsonschema` from its pinned `requirements.txt`, which is not installed in this environment. That directory is untouched by this diff, so this is a not-run, not a known failure. - **Not measured, and worth knowing before this fans out:** the four other reusables in this repo that mint an App token were not audited for the same PR-checkout-plus-write-token shape. The new test is scoped to `cursor-review.yml` alone. `groom.yml` is documented as already credential-split and `pr-size.yml` is the pattern this copies, but neither was re-verified here, and `pr-area-label.yml`, `pr-derisk.yml` and `linear-ticket.yml` were not looked at. ## Provenance - **Authored by:** agent-work loop - **Verified:** `python3 -m unittest discover -s .github/cursor-review/tests`: 314 passed, 0 failed (305 on base, +9 isolation tests); `.github/workflow-pins/tests`: 308 passed; `.github/groom`: 365; `.github/public-repo-hygiene`: 196; `.github/refresh-reviewers`: 48; `.github/agents-md-integrity`: 46 — all OK. `python3 .github/workflow-pins/check_workflow_pins.py`: OK, 11 workflows, 0 exempt; `python3 .github/agents-md-integrity/check_agents_md.py --root .`: passed (1 pre-existing CODEOWNERS warning); `actionlint .github/workflows/cursor-review.yml`: 1 finding, byte-identical to the one on `origin/main` (`job.workflow_sha`, in the untouched ledger job); `shellcheck` + `bash -n` clean on every `run:` block in the file. Isolation suite proven non-vacuous against 7 injected regressions (permissions block deleted; `pull-requests: 'write' # comment` restored on `consolidate`; `permissions: write-all`; a workflow-level grant; the assets checkout switched to `${{ github.repository }}`; a `run:`-block forgery of the `repository:` exemption; the judge invocation removed). - **Deviations:** see "Judgment calls" and "Residual" above — chiefly that acceptance criterion 1 holds in substance but names one job where the tree now has two; that a failed `consolidate` now skips the poster rather than reddening it; that the `notify-complete` DM's posted-versus-degraded gap is deferred to a follow-up; and that the end-to-end consumer-run criterion cannot be exercised before merge. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-26 22:37:42 +00:00