feat(pr-risk): require workflows_ref to be an ancestor of upstream main (BE-6492) - #139
feat(pr-risk): require workflows_ref to be an ancestor of upstream main (BE-6492)#139mattmillerai wants to merge 3 commits into
Conversation
…in (BE-6492) Shape (40-hex) proves the pinned ref is immutable; it never proved WHICH repository authored the commit. A fork of this public repo shares the upstream object store, and GitHub serves a fork PR head objects from the upstream URL to an unauthenticated client, so a fork-authored SHA passed the shape test and would be checked out into a job holding the caller pull-requests: write token. Both byte-identical copies of the pin guard now also fetch the pinned SHA and main from a literal upstream URL and require merge-base --is-ancestor. No opt-out input: an opt-out would be set by the very pin-bump PR the check distrusts. Every failure mode is annotated and fails closed. Updates the three honesty locations (workflow header, workflows_ref input description, README row) plus the caller setup guide, and extends the pin contract suite: the pinned body grows the new block, and the ancestry anchor, the literal-URL rule and the fatality of every rejection path are restated as named properties with coverage self-checks.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 45 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughChangesWorkflow reference provenance
Sequence Diagram(s)sequenceDiagram
participant GateOrGrade
participant TemporaryGitRepository
participant UpstreamMain
GateOrGrade->>TemporaryGitRepository: Initialize temporary repository
GateOrGrade->>TemporaryGitRepository: Fetch pinned workflows_ref
GateOrGrade->>UpstreamMain: Fetch main from literal upstream URL
UpstreamMain-->>TemporaryGitRepository: Return main history
GateOrGrade->>TemporaryGitRepository: Check workflows_ref ancestry
TemporaryGitRepository-->>GateOrGrade: Return verdict or infrastructure error
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
![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.
Found 9 finding(s).
| Severity | Count |
|---|---|
| 🟡 Medium | 2 |
| 🟢 Low | 4 |
| ⚪ Nit | 3 |
Panel: 8/8 reviewers contributed findings.
…-6492) Review follow-ups on the ancestry guard. The axis itself is unchanged — a pin must still be a 40-hex SHA and an ancestor of upstream main, with no opt-out — but making two unauthenticated fetches a hard precondition of every run in every enrolled consumer needs the fetches to be bounded and the failures to be distinguishable. - Retry each fetch three times with a linear backoff, under git's low-speed timeout. A transient github.com error or a secondary rate limit on shared Actions egress no longer reddens the whole fleet at once, and a stalled connection fails fast instead of hanging to the job timeout. The VERDICT is never retried. - Bound both fetches with --filter=blob:none. merge-base reads no file content, and the rejection path — a pin with no shallow boundary anywhere in main's ancestry — was the case that fetched the most: 3.3M -> 424K measured. - Distinguish merge-base's three outcomes. rc > 1 is git failing to answer, not a verdict; reporting it as "treat the pin-bump as hostile" was cry-wolf. Both branches still fail closed. - Clean up the scratch clone with a trap on every path, and annotate the mktemp/git-init faults like every other rejection path. - Correct the shallow-boundary comment: the depth-1 fetch DOES truncate main at the pin, so the second fetch is not a complete history. What makes it safe is the direction of the walk, which the comment now says. Test side, all three of which were vacuous passes: - The literal-URL check sort -u'd tokens into a SET, so replacing ONE of four fetch URLs with an expression still passed. Counted per line now. - The rejection-path count was -ge 3 while the guard had grown to seven, so deleting a path stayed green. The threshold is read off the pin. - The fatality awk overwrote an unreported pending line, so two consecutive ::error:: lines with one exit read as clean. Each of the three is mutation-tested to fail on exactly the edit it missed before.
mattmillerai
added
cursor-review
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/callers/pr-risk.md`:
- Line 80: Update the caller permissions documentation near the workflows_ref
entry to include pull-requests: write, actions: read, and checks: write
alongside the existing required permissions, ensuring the documented grade and
publish-check requirements are complete.
In `@README.md`:
- Line 23: The README caller permissions list must require checks: write
unconditionally, because the reusable workflow’s publish-check job declares that
permission and GitHub validates nested permissions before execution. Update the
permissions list in the pr-risk workflow documentation to include checks: write
alongside the existing grants, without making it conditional on check_run.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: e423aad4-b3ed-48a9-87a2-cdb20e384b08
📒 Files selected for processing (4)
.github/workflows/pr-risk.ymlREADME.mddocs/callers/pr-risk.mdscripts/pr-risk/tests/test_pin_contract.sh
…optional (BE-6492) The workflow header has been authoritative since the two-job split: GitHub validates EVERY nested job's declared `permissions:` against the caller's block at startup, and a job-level `if:` is a runtime condition — so `publish-check`'s `checks: write` is required of every caller whether or not `check_run` is switched on. Both docs said otherwise, in different ways: - README's row claimed `check_run: true` "is the only surface that needs an extra `checks: write` grant ... and only when switched on", then listed a six-item grant with `checks: read`. - docs/callers/pr-risk.md was staler still — its copy-pasteable caller and its Required-permissions block both said `pull-requests: read` (the grade job declares `write`; the labels endpoint 403s without it) and omitted `actions: read` and `checks: write` entirely. Either page, copied as written, fails the caller's first run at startup with the opaque "workflow file issue" and no job-level detail. Both now match the header's caller block, and the caller guide explains the union rule where an enroller reads it, including the pin-bump case. Docs only — no workflow or script behaviour changes.
There was a problem hiding this comment.
🔍 Cursor Review — Consolidated panel
Triggered by @mattmillerai.
Round 2 — ledger: 9 prior finding(s) across 1 round(s) (0 never answered).
Found 10 finding(s).
| Severity | Count |
|---|---|
| 🟡 Medium | 1 |
| 🟢 Low | 6 |
| ⚪ Nit | 3 |
Panel: 8/8 reviewers contributed findings.
.github/workflows/pr-risk.yml:650 — 🟡 Medium — The retry budget can exceed the gate job's timeout-minutes: 5: each fetch_upstream allows 3 attempts bounded only by http.lowSpeedTime=30 plus 5s+10s of backoff (~105s), and the guard calls it twice (~210s) before actions/checkout and the resolver even start — and the low-speed knobs bound only a transfer that has started, so a blackholed connect can hang for libcurl's 300s default with no connect timeout at all. When the job is killed at the cap the run is red with NONE of the ::error:: annotations the input description at line 416 tells the consumer to read, which is the cry-wolf the retry was added to prevent. Give each attempt a wall-clock bound (e.g. timeout 45 git ...) or raise gate's timeout to cover the worst case. Same at line 876. Raised by 3 of 8 reviewers (claude-opus-5-thinking-max edge-case, claude-opus-5-thinking-max adversarial, gpt-5.6-sol-max adversarial).
.github/workflows/pr-risk.yml:677 — 🟢 Low — merge-base --is-ancestor peels annotated tags and does not prove the supplied object is itself a commit, and the rc > 1 branch's own comment lists "a fetched ref that is not a commit" as a cause while its annotation tells the consumer this is an infrastructure fault. A well-shaped 40-hex workflows_ref naming a tag/tree/blob (GitHub serves any object by SHA) is fetchable and lands in that branch permanently, so the consumer re-runs forever on a deterministic condition. Add git -C "$scratch" cat-file -t "$WORKFLOWS_REF" must equal commit before the ancestry test, and give it its own annotation. Same at line 902. Raised by 2 of 8 reviewers (claude-opus-5-thinking-max edge-case, gpt-5.6-sol-max edge-case).
scripts/pr-risk/tests/test_pin_contract.sh:277 — 🟢 Low — Extracting the fetch into fetch_upstream moved the line that actually runs git ... fetch out of the scan's reach: fetchlines now matches only if ! fetch_upstream call sites, so both the new per-line nurl assertion and the urltokens set comparison inspect lines that merely pass a URL as an argument, never the line that consumes it. A URL added inside the helper body (line 651/876 of the workflow) is invisible to the property whose comment claims "no OTHER URL-shaped token appears". Include the helper's git ... fetch line in fetchlines, or separately assert the helper body contains no URL-shaped token. Raised by 1 of 8 reviewers (claude-opus-5-thinking-max edge-case).
.github/workflows/pr-risk.yml:664 — 🟢 Low — The round-1 reply declined --shallow-since because a time bound turns a legitimately old pin into a false REJECT — that reasoning does not extend to --filter=tree:0, which bounds the payload strictly further without bounding ancestry at all, since merge-base reads no trees either. On the rejection path this main fetch still downloads the complete tree graph for all of history, which is the unauthenticated, no-opt-out path that grows as the repo ages. Raised by 2 of 8 reviewers (claude-opus-5-thinking-max adversarial, gpt-5.6-sol-max adversarial).
↩︎ re-raise of #139 (comment) (round 1)
.github/workflows/pr-risk.yml:675 — 🟢 Low — --filter=blob:none registers the upstream URL as a promisor remote in the scratch repo, so this merge-base can itself trigger an on-demand lazy fetch if an object it walks is absent from the partial packs. That third round-trip sits outside the fetch_upstream retry/low-speed policy and outside any timeout, so a partial-clone gap becomes an unbounded hang instead of the clean rc>1 infrastructure-fault path directly below. Set GIT_NO_LAZY_FETCH=1 for this invocation so a gap surfaces as rc>1. Same at line 900. Raised by 1 of 8 reviewers (claude-opus-5-thinking-max adversarial).
.github/workflows/pr-risk.yml:649 — 🟢 Low — fetch_upstream retries every failure identically, including definitive ones: a not-yet-merged or fork-authored pin fails with upload-pack: not our ref on all three attempts, spending 15s of sleeps and two extra requests for a permanent answer. The rate-limit case is worse — a 429 is retried three times on a fixed 5s/10s backoff, tripling request volume against github.com precisely when it is shedding load, simultaneously across every enrolled consumer with no opt-out. Restrict retries to transport-level failure, or at least skip the retry when git reports not our ref. Raised by 1 of 8 reviewers (claude-opus-5-thinking-max adversarial).
.github/workflows/pr-risk.yml:530 — 🟢 Low — This gate-job comment block is now stale: it still states that SHAPE IS THE ONLY AXIS THIS FILE CAN CHECK and that the fork-authored 40-hex SHA case cannot be closed from here — but the same step ~50 lines below now enforces ancestry, which does close the fork case and leaves only the stale-pin residual. The header, input description, README and docs page were all rewritten for the new axis; this block was missed, and in this file the prose is the spec a maintainer reasons from when editing or reordering the guard. Raised by 1 of 8 reviewers (kimi-k3-max edge-case).
.github/workflows/pr-risk.yml:416 — ⚪ Nit — The input description enumerates four annotations the consumer can get, but the guard now emits six — the scratch-directory and git init faults added this round (lines 638 and 643) are not in the list. Someone who hits one of those reads a documented enumeration that does not contain their annotation, the same footgun as documenting an input that does not exist. Also, "each one is retried three times" overstates by one: for attempt in 1 2 3 is three TOTAL attempts, which is how the step's own messages phrase it ("after 3 attempts"). Raised by 2 of 8 reviewers (claude-opus-5-thinking-max edge-case, kimi-k3-max edge-case).
.github/workflows/pr-risk.yml:654 — ⚪ Nit — if [ "$attempt" -lt 3 ] re-derives the loop bound from for attempt in 1 2 3 rather than reading it, and "after 3 attempts" is hardcoded a third and fourth time in the annotations at lines 661/665. Extending the loop to 1 2 3 4 would silently drop the backoff between the added attempts and leave the consumer-facing message wrong; hoist the count into one variable used by the loop, the guard and the messages. Raised by 1 of 8 reviewers (claude-opus-5-thinking-max edge-case).
scripts/pr-risk/tests/test_pin_contract.sh:307 — ⚪ Nit — $code is already asserted byte-identical to $expect at line 232, and both nerr and nerr_pinned are grep -c '::error::' over exactly those two strings, so this eq cannot fail unless the equality has already failed, and the sanctioned two-place edit moves both counts in lockstep. The gain over the old -ge 3 is a clearer failure message, not new coverage — the comment at 298-302 ("deleting one would have passed — the exact vacuous pass this line exists to stop") overstates it, and the -ge 3 floor still sits four paths below the seven the pin now declares. Raised by 2 of 8 reviewers (claude-opus-5-thinking-max edge-case, claude-opus-5-thinking-max adversarial).
(Inline comments could not be anchored to the diff; listed above instead.)
|
🤖 The reviews loop filed Linear follow-up ticket(s) for review thread(s) deferred as out of scope for this PR:
The following carry
|
2 participants
ELI-5
The pin guard already checked that
workflows_reflooks like a commit SHA. It never checked whose commit it was. Because this repo is public, GitHub will hand a fork's commits to anyone who asks for them from this repo's own URL — so a commit written by a fork was just as well-shaped as a real one, and would have been checked out and run inside a job holding the caller'spull-requests: writetoken. This PR adds a second question: is this commit actually part of ourmain? If not, the run stops before anything is checked out.What changed
Both byte-identical copies of the
Enforce workflows_ref pin contractstep (thegatejob and thegradejob) gain an ancestry check after the shape check and still before theLoad pr-risk toolcheckout: fetch the pinned SHA depth-1 from a literalhttps://github.com/Comfy-Org/github-workflows, fetchmainwith an explicit+refs/heads/main:refs/heads/upstream-mainrefspec, then requiregit merge-base --is-ancestor. The URL is a literal on purpose — inside a reusable workflowgithub.repositorynames the caller andgithub.job_workflow_refjust repeats the caller'suses:string, so no context can name this repo, and a variable there would be an alias a fork could point back at itself. There is no opt-out input, deliberately: an opt-out would be set by the very pin-bump PR the check exists to distrust. All three rejection paths are annotated and fail closed.Docs updated at the three honesty locations the previous PR established (workflow header,
workflows_refinput description, READMEpr-riskrow) plus the caller setup guide, stating both the new claim and the two residuals honestly.Verification
I ran the real thing against the real upstream, unauthenticated, before shipping it — this is a network shape, and the suites are hermetic so they cannot prove it works:
maintip97a280dmain(same serving mechanism as a fork PR head)bash scripts/pr-risk/tests/test_pin_contract.sh→ 26 passed, 0 failed. All six pr-risk suites green (26 / 63 / 48 / 105 / 27 / 67).shellcheck -xclean across every pr-risk script. AGENTS.md integrity checker passes.All five required mutations individually go red, each on the property that names it: ancestry deleted from one copy → drift + anchor coverage; from both → anchor coverage + literal-URL;
exit 1→exit 0→ fatality + no-exit 0; literal URL →${{ github.server_url }}/${{ github.repository }}→ literal-URL check;::error::with no following exit → fatality.Judgment calls — please read these
1. The ticket's premise about "axis 2" is not true on
main, so the code deviates. BE-6492 says the guard "currently enforces two axes: shape and lock-step (== github.job_workflow_sha)" and instructs me to test$JOB_WORKFLOW_SHAbecause "axis 2 already proved them equal". There is no lock-step axis. PR #118 shipped shape only, and its own comments explain why lock-step is not implementable here:job_workflow_shais an OIDC token claim, not agithubcontext property, so reading it needsid-token: writefrom every caller. No$JOB_WORKFLOW_SHAor${safe_job_sha}variable exists to read. I used$WORKFLOWS_REFand the existing${safe_ref}. This is not merely a forced substitution — it is the valueactions/checkoutactually consumes, so validating it directly is at least as sound as validating a proxy proven equal to it. For the same reason the in-file comment calls this the second axis, not the third.2. The residual is different from what the ticket says, and I documented mine, not its. The ticket says the only residual left is "a fork commit that EDITS
pr-risk.ymlitself". That would be true if lock-step existed. Since it does not, a stale-but-merged pin is also still unproven — it is an ancestor ofmaintoo. The docs state both residuals.3. Test items 1–3 are implemented; item 4 does not apply. The ticket describes a suite with an emit-scan whitelist and an "every
::error::is followed by an exit" property. That suite no longer exists: it was deliberately replaced with a verbatim equality on the guard's executable body, and the file's header explicitly records that the pattern-scan approach was abandoned because review kept finding ways through it. So I extended the pinned body (mandatory — otherwise the suite fails), and added the ancestry anchor, literal-URL and fatality checks as named restatements with coverage self-checks. I added a header paragraph making clear these are for better failure messages and are not a second line of defence, so nobody later mistakes them for the guarantee.4. I added a fourth rejection path the ticket did not specify. The ticket's snippet leaves the
mainfetch bare, so a failure there exits 128 with no annotation — against this step's own doctrine that a rejection is never an obscure failure one step later. I wrapped it in the sameif ! … ::error:: … exit 1shape. The two-fetch shape and the explicit refspec are unchanged.5. Blast radius — the one claim I could not independently re-verify. I confirmed directly that nothing in this repo produces a non-ancestor pin:
pr-risk.ymlhas no caller here (so this repo's CI never self-invokes it), andbump-pr-risk-callers.ymltriggers onlyon: push: branches: [main]and writesgithub.shaafter assertingmain_tip == GITHUB_SHA— so the bumper can only ever write a main-tip SHA. What I could not re-verify is the ticket's claim that all 4 historical pins across the live consumers are ancestors, because the roster is a private repo variable and this repo is public. That is BE-6491's evidence, taken on trust. Worth a reviewer's glance, especially as #137 (seeding the pr-risk roster) is still open.6. Capability-denial falsification. This diff denies a capability (pinning an unmerged SHA), so I went looking for a legitimate user rather than assuming there is none — items in 5 above, plus the pre-merge testing path: a change to pr-risk can still be validated before merge by the six hermetic suites in
scripts/pr-risk/tests/, which do not need a live pin. The redirect for anyone who was relying on an unmerged pin is "merge here first, then bump", which is what every consumer has done in practice.7. No retry on transient network failure. A GitHub blip fails the run closed rather than retrying; recovery is a re-run, and the error text says so. I judged a retry loop not worth the added non-determinism on a trust boundary, but it is a deliberate choice, not an oversight.
Cost
Two small fetches of a few-hundred-commit repo per guarded job, ~1–2s each, on
gateandgrade.