feat(cursor-review): fail the review when workflows_ref differs from the uses: pin - #328
mattmillerai wants to merge 1 commit into
Conversation
…the uses: pin
Each of the eight `Require a pinned workflows_ref` steps proved the ref was
non-empty and warned when it was not 40-hex, but nothing ever compared it to
the commit the caller's `uses:` line actually resolved to. A caller could pin
`uses:` to one commit and load this workflow's prompts, scripts and model list
from another — the review then runs one version's workflow against a different
version's judging logic, with both pins looking plausible in the caller.
Port the equality guard public-repo-hygiene.yml already runs: bind
`job.workflow_sha` (computed by the runner from the `uses:` pin, so a caller
cannot set it) and fail on a mismatch. All eight copies stay byte-identical;
a new parity suite pins that, asserts the count, and pins the ledger's
exemption from the other side.
Two deliberate differences from the hygiene guard, both documented inline:
* an empty `job.workflow_sha` (Actions runner older than v2.334.0) WARNS
rather than failing. Hygiene fails closed there because it is the checker
whose own integrity rests on the equality; here a property nothing could
evaluate should not take the whole review down.
* both sides are lower-cased before the comparison. Hygiene normalises only
`$SHA` because its 40-hex test hard-fails an upper-case ref several lines
earlier; the same test here only warns, so an upper-case ref naming the
RIGHT commit reaches the comparison and must not fail.
The exempt `Prior-review ledger` job — which must never fail, since the review
matrix `needs:` it — gets the same cross-check as a `::warning::` with no exit,
so a caller whose guarded jobs are all skipped still has the split recorded
somewhere.
|
Warning Review paused — included plan limit reachedKeep your review moving with free on-demand reviews.
On-demand reviews are free for the next 21 days.
Reviews can continue after your included limit without a manual trigger. An admin must approve usage-based billing. Promotion and pricing detailsOn-demand reviews are free for the next 21 days. After that, they cost $0.25 per reviewed file. Review limit detailsOr wait 49 minutes for your next included review. Limit details: You’ve used the included review currently available. Your 139 included PR review attempts over the past 7 days set your current allowance at 1 review per hour. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Team Run ID: 📒 Files selected for processing (4)
Comment |
mattmillerai
added
agent-coded
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔍 Cursor Review — Consolidated panel
Triggered by @mattmillerai.
Every reviewer in the matrix failed to contribute — see the panel summary for which cells errored, and the run logs for the underlying cause.
Panel: 0/6 reviewers contributed findings.
Reviewers that did not contribute: claude-opus-5-thinking-max:adversarial (error), gpt-5.6-sol-max:adversarial (error), kimi-k3-high:adversarial (error), claude-opus-5-thinking-max:edge-case (error), gpt-5.6-sol-max:edge-case (error), kimi-k3-high:edge-case (error)
2 participants
ELI-5
A caller of
cursor-review.ymlpins it twice:uses:says which workflow to run, andworkflows_ref:says which commit to load the review prompts and scripts from. They are supposed to name the same commit — the header has said so for a year — but nothing checked it. If they drift, the review runs one version's workflow against a different version's judging logic, and both pins look perfectly plausible in the caller file.public-repo-hygiene.ymlalready solved this: compareworkflows_refagainstjob.workflow_sha, which the runner computes from theuses:line, so a caller cannot forge it. This ports that guard into all eight of cursor-review'sRequire a pinned workflows_refsteps and fails the job on a mismatch.What changed
WORKFLOW_SHA: ${{ job.workflow_sha }}inenv:and an equality test after the existing empty/40-hex tests. They remain byte-identical to one another (there is no anchor, no step-leveluses: ./, and no job-levelrun:inworkflow_call, so eight literal copies is the only spelling available).Prior-review ledgerjob gets the same cross-check as a warning with noexit. That job must never fail — the review matrixneeds:it — but it runs for every caller, including one whose guarded jobs are all skipped by the label gate, so its log is sometimes the only place a split pin would be recorded.workflows_refrow in the setup guide, one line in the workflow's header example, and a newworkflows_refmust equal theuses:pin section in.github/cursor-review/README.mdcovering the rule, the warn-on-empty branch, and how to fix a red run.test_workflow_ref_guard_parity.pyasserts the count is exactly 8 (so a ninth guarded job is an explicit opt-in), that all eight blocks are byte-identical, that the guard binds the bare input rather than the|| job.workflow_shafallback, that the mismatch branch exits, that the empty-job.workflow_shabranch does not, and — from the other side — that the ledger readsjob.workflow_shaand never exits. Picked up automatically by the suite'sunittest discover; no workflow wiring needed.Blast radius — measured, not assumed
I swept every caller GitHub code search can see and compared the two pins in each:
Comfy-Org/comfy-typescript-sdk,ebfd9e4vsffcc3f5)workflows_refvalues that are not 40-hexci-cursor-review.yml425c154, and it runs the pre-change reusable, so it does not exercise this guardThe guard is a true positive in the wild today. The identical check in
public-repo-hygiene.ymlfailed a live run this morning —comfy-clirun35427331671, on branchdependabot/github_actions/…, withworkflows_ref (3b2c8ca…) is not the commit this workflow was called at (a404fd4…). Dependabot had bumpeduses:alone. That same run log is the empirical proof thatjob.workflow_shais populated on real GitHub-hosted runners inside aworkflow_callreusable (it printed a realgithub-workflowsmain SHA), which is the one premise that would otherwise make this change a false-fail machine. A separate hygiene run oncomfy-cli(35428909227) is green, so the guard is not simply failing everything.Dependabot is the mechanism that will actually trip this.
bump-callers.shrewrites both pins together, so a fleet bump keeps them equal; Dependabot rewrites onlyuses:, which splits them. Of the 27 external callers, 8 have agithub-actionsDependabot ecosystem with noignoreentry forComfy-Org/github-workflowsand are therefore exposed — 5 public (ComfyUI_frontend,comfy-cli,comfy-mcp,comfy-python-sdk,comfy-typescript-sdk, each confirmed public via the API before naming it here) and 3 non-public, which I have deliberately not named. 3 callers already have the ignore, 15 have nodependabot.ymlat all, and 1 has nogithub-actionsecosystem. Expect cursor-review to start going red on Dependabot PRs in those 8 the same way public-repo-hygiene already does. That is the intended signal, and the error message names the fix.Judgment calls
job.workflow_shabranch warns; hygiene fails closed. Hygiene's rationale is that it is the checker whose own integrity rests on the equality, so a runner that cannot supply the value cannot run it. cursor-review is not in that position, and failing eight jobs over a property nothing could evaluate would take a review down on a caller that changed nothing. Every job here isruns-on: ubuntu-latest, so only a runner group shadowing that label reaches the branch. Flagging it explicitly because the two guards are meant to converge.REFalone because "the 40-hex grep already fixes case". That is true in hygiene, where the shape test hard-fails an upper-case ref several lines earlier, and false here, where the same test only warns. So an upper-case-but-correct ref reaches the comparison, and comparing raw would fail all eight jobs of a caller whose two pins genuinely agree — with an error message stating something false.$REFitself is untouched, so the messages still quote what the caller wrote. Verified: an upper-case 40-hex ref equal to$SHAnow exits 0 with only the pre-existing shape warning.comfy-typescript-sdk#153), so the message would have pointed maintainers at an unmerged precedent; and the name is not on.github/lint/org-repo-allowlist.txt, so publishing it into a workflow file would sidestep the ratification step that allowlist exists to be. The message keeps the actionable half: add adependabot.ymlignore forComfy-Org/github-workflows.workflows_refnow hard-fails — not via the shape test, which I left as a::warning::exactly as instructed, but as a consequence of the equality test, sincemainis not a 40-hex SHA. This is unavoidable for any equality guard and matches hygiene. It affects nobody today: 0 of 27 callers use a non-SHAworkflows_ref.tr 'A-Z' 'a-z'draws shellcheckSC2018/SC2019infos. Kept verbatim from hygiene —[:upper:]/[:lower:]would add accent handling that is wrong for byte-exact hex, and nothing in CI shellchecks workflowrun:blocks.Sequencing
The related port into the other six reusables (
agents-md-integrity,linear-ticket,pr-area-label,pr-size,coderabbit-config-validate,refresh-reviewers) is tracked separately and deliberately untouched here — this PR iscursor-review.ymlonly. The two ports should converge on one guard text; judgment calls 1 and 2 above are the places where this one had to diverge from the hygiene original, and both reasons are file-specific rather than preferences.comfy-typescript-sdk#153re-syncs the one split caller and adds its Dependabot ignore; it is still open. Merging after it lands is tidier but not required:#153re-syncs to425c154, which predates this commit, so that repo's cursor-review does not go red the moment this merges — the guard only executes once itsuses:is bumped to a commit containing it, and abump-callersbump moves both pins together.Open PRs #320/#321/#322 also edit
cursor-review.yml; I confirmed none of them touches a guard step, aWORKFLOWS_REFbinding, or adds a guarded job (which would needEXPECTED_GUARDSbumped).Residual
ignoreentry forComfy-Org/github-workflowsin its owndependabot.yml; that is a cross-repo change and out of scope here. Until then, the first Dependabot bump after this merges will turn their cursor-review red. The 5 public ones are named above; the 3 non-public ones are identifiable by re-running the sweep against theCURSOR_REVIEW_CALLERSroster.CURSOR_REVIEW_CALLERSroster was not audited. The sweep above is GitHub code search, which sees only what the search index covers and only default branches. Roster secrets are write-only, so I could not check the sweep against the canonical caller list in either direction — a caller missing from code search would not appear in any number here.run:body under a local shell, plus the live evidence from the identical hygiene guard. This repo's ownci-cursor-review.ymlpins the pre-change reusable, so it cannot exercise it; the first real execution is the next fleet bump.check_workflow_pins.pyaccepting the hygiene shape,ci-cursor-review.ymlpinned to425c154, and none of feat(cursor-review): make panel integrity a check-run conclusion #320/fix(cursor-review): build the incremental hunks block as a subset of the reviewed diff #321/fix(cursor-review): record each round's merge base and pin the incremental block's OLD patch to it #322 touching the guard. Anything else in that comment is unexercised.Provenance
python3 .github/workflow-pins/check_workflow_pins.py→OK — 11 workflow(s) … every ref checkout guarded, exit 0;unittest discover .github/cursor-review/tests→ 611 passed, 0 failed (11 of them the new parity suite);unittest discover .github/workflow-pins/tests→ 499 passed, 32 skipped, 0 failed;check_agents_md.py --root .→ passed (2 pre-existing warnings);check-org-repo-literals.sh→ OK, plusshellcheck -xon it; PyYAML parse ofcursor-review.ymlconfirming 8 guard steps with the expectedenv:and the ledger's two bindings; extracted-guard shell runs covering equal / differing / empty-WORKFLOW_SHA/ empty-ref / whitespace-only-ref / branch-ref / upper-case-ref, and the extracted ledger body across the same matrix confirming it never exits non-zero; four mutation tests (drop one copy's binding, remove theexit, add anexitto the ledger, add a 9th guard step) each confirming the new suite fails.