From a49e087a00dc2202dd9b54477a87cbc569a3fbd1 Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Tue, 4 Aug 2026 21:41:13 -0700 Subject: [PATCH 1/3] feat(pr-risk): require workflows_ref to be an ancestor of upstream main (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. --- .github/workflows/pr-risk.yml | 202 +++++++++++++++++---- README.md | 2 +- docs/callers/pr-risk.md | 2 +- scripts/pr-risk/tests/test_pin_contract.sh | 70 ++++++- 4 files changed, 238 insertions(+), 38 deletions(-) diff --git a/.github/workflows/pr-risk.yml b/.github/workflows/pr-risk.yml index 4aa81d9..e687387 100644 --- a/.github/workflows/pr-risk.yml +++ b/.github/workflows/pr-risk.yml @@ -20,17 +20,33 @@ name: PR Risk Grade (reusable) # `refs/pull/N/head` — so the grading logic cannot change after the caller was reviewed, and # "pin it, don't float it" is machine-checked rather than trusted prose in this header. # -# WHAT SHAPE DOES NOT PROVE is WHICH commit it is. A fork of this PUBLIC repo shares its object -# store, so a fork-authored commit is a perfectly well-shaped 40-hex SHA and would be checked out -# into a job holding the caller's `pull-requests: write` token. Nothing inside this file can close -# that: the check it wants is "`workflows_ref` equals the commit the caller's `uses:` resolved to", -# and the runner does NOT expose that commit to the workflow — `github.workflow_sha` is the -# CALLER's top-level workflow file, and `job_workflow_sha` (the value that would answer this) -# exists only as an OIDC token claim, which would mean an `id-token: write` grant from every -# caller and a token exchange in a job that today holds `permissions: {}`. Until that is designed, -# this is bounded by REVIEW OF THE CALLER on its base branch — which is also the only thing that -# can see the ref the caller actually wrote: `uses:` must name this repo at a full commit SHA and -# `with: workflows_ref:` must be that same SHA written out LITERALLY, never an expression and +# A SECOND AXIS, ANCESTRY, now proves WHOSE commit it is. Shape alone never could: a fork of this +# PUBLIC repo shares its object store, and GitHub serves a fork PR's head objects from this repo's +# own URL to an unauthenticated client — so a fork-authored commit is a perfectly well-shaped +# 40-hex SHA, and it would have been checked out into a job holding the caller's +# `pull-requests: write` token. Every job that checks the ref out now also fetches it from +# `Comfy-Org/github-workflows` (URL written LITERALLY — no context can name this repo from inside a +# reusable workflow) and requires it to be an ANCESTOR OF UPSTREAM `main`. Merged upstream history, +# and nothing else, may be pinned. There is deliberately NO opt-out input: an opt-out would be set +# by the very pin-bump PR the check exists to distrust. Two operational consequences, both by +# design — pinning a NOT-YET-MERGED SHA is now rejected, so merge first and then bump (which is what +# every consumer has always done in practice); and the axis FAILS CLOSED if this repo ever goes +# private or `main` is force-rewritten past a consumer's pin, in which case consumers re-pin to a +# commit that is on `main` to recover. +# +# WHAT ANCESTRY STILL DOES NOT PROVE is that the pin is the CURRENT one. A stale pin, left behind +# when `uses:` moved, is a merged ancestor too and passes. The check that would close that is +# "`workflows_ref` equals the commit the caller's `uses:` resolved to", and the runner does NOT +# expose that commit to the workflow — `github.workflow_sha` is the CALLER's top-level workflow +# file, and `job_workflow_sha` (the value that would answer this) exists only as an OIDC token +# claim, which would mean an `id-token: write` grant from every caller and a token exchange in a job +# that today holds `permissions: {}`. The other residual is narrower and irreducible: a fork commit +# that edits THIS FILE. Such a commit is not an ancestor of upstream `main`, so the axis rejects it +# on the way in — but a caller pinned at one would be running the fork's own copy of this guard, and +# no check written inside a file can bound a revision of that file chosen by an attacker. Both +# residuals are bounded by REVIEW OF THE CALLER on its base branch, which is also the only thing +# that can see the ref the caller actually wrote: `uses:` must name this repo at a full commit SHA +# and `with: workflows_ref:` must be that same SHA written out LITERALLY, never an expression and # never a tag. Call this workflow DIRECTLY; a nested `workflow_call` chain through an org wrapper # is unsupported. See the `workflows_ref` input for the detail. # A consumer repo sharpens the generic defaults by committing @@ -366,32 +382,58 @@ on: with them, since this value is handed to `actions/checkout` verbatim. - THAT IS ONE AXIS, AND IT IS THE ONLY ONE AVAILABLE FROM IN HERE. Shape - proves the ref is IMMUTABLE; it says nothing about WHICH commit it - names. A fork of this PUBLIC repo shares its object store, so a + ITS ANCESTRY IS ENFORCED TOO, in the same step, before the same + checkout: the commit must be reachable from `main` of + Comfy-Org/github-workflows. Shape alone proves only that the ref is + IMMUTABLE, never WHICH repository authored the commit — a fork of this + PUBLIC repo shares its object store, and GitHub serves a fork PR's head + objects from this repo's own URL to an unauthenticated client, so a fork-authored commit is as well-shaped as any other and would be - checked out into a job holding the caller's write token; equally, a pin - left behind when `uses:` moved still passes. The test that would close - both is "equal to the commit `uses:` resolved to for this job" — and - the runner does not expose that commit to the workflow. - `github.workflow_sha` is the CALLER's top-level workflow file, not this - one; the value that would answer it, `job_workflow_sha`, exists only as - an OIDC token claim, which means an `id-token: write` grant from every - caller plus a token exchange in a job that today holds + checked out into a job holding the caller's write token. The step + fetches the pinned SHA and `main` from a LITERAL upstream URL (no + `github` context can name this repo from inside a reusable workflow, so + a variable there would be an alias a fork could point at itself) and + requires `merge-base --is-ancestor`. There is deliberately NO opt-out + input: an opt-out would be set by the very pin-bump PR the check exists + to distrust. + + + TWO OPERATIONAL CONSEQUENCES, BOTH BY DESIGN. Pinning a NOT-YET-MERGED + SHA is now rejected outright — merge to `main` first, then bump the + pin, which is what every consumer has always done in practice. And the + axis FAILS CLOSED: if this repo ever goes private, or `main` is + force-rewritten past a consumer's pin, the fetch or the ancestry test + fails and the run goes red rather than proceeding on an unproven ref. + Recovery is to re-pin to a commit that is on `main`. + + + WHAT ANCESTRY DOES NOT PROVE is that the pin is the CURRENT one: a pin + left behind when `uses:` moved is a merged ancestor too, and passes. + The test that would close that is "equal to the commit `uses:` resolved + to for this job" — and the runner does not expose that commit to the + workflow. `github.workflow_sha` is the CALLER's top-level workflow + file, not this one; the value that would answer it, `job_workflow_sha`, + exists only as an OIDC token claim, which means an `id-token: write` + grant from every caller plus a token exchange in a job that today holds `permissions: {}`. Adding that is a caller-contract change and is - tracked separately — do not assume this input's provenance is checked. + tracked separately — do not assume this pin is proven CURRENT. The + other residual is narrower and irreducible: a fork commit that edits + `pr-risk.yml` itself is not an ancestor of upstream `main`, so it is + rejected on the way in, but a caller pinned at one would be running the + fork's own copy of this guard, and nothing written inside a file can + bound a revision of that file an attacker chose. - SO REVIEW OF THE CALLER IS WHAT BOUNDS IT, and it is the only place - that CAN: the ref the caller actually wrote in `uses:` is never sent - here, so no check inside this workflow can tell a SHA-pinned caller - from a `@v1` one passing whatever that tag currently points at, nor a - written-out SHA from a context expression that re-resolves every run. - WHEN REVIEWING A CALLER, require `uses:` at a full commit SHA of this - repo (org policy already does — a floating one fails the pin-validation - consumer CI runs) and `with: workflows_ref:` set to that same SHA - LITERALLY, character-for-character. An expression of any kind here, and - the contract is decoration. + SO REVIEW OF THE CALLER IS WHAT BOUNDS BOTH RESIDUALS, and it is the + only place that CAN: the ref the caller actually wrote in `uses:` is + never sent here, so no check inside this workflow can tell a SHA-pinned + caller from a `@v1` one passing whatever that tag currently points at, + nor a written-out SHA from a context expression that re-resolves every + run. WHEN REVIEWING A CALLER, require `uses:` at a full commit SHA of + this repo (org policy already does — a floating one fails the + pin-validation consumer CI runs) and `with: workflows_ref:` set to that + same SHA LITERALLY, character-for-character. An expression of any kind + here, and the contract is decoration. CALL IT DIRECTLY — a nested `workflow_call` chain (caller → an org @@ -524,6 +566,53 @@ jobs: echo "::error::workflows_ref must be the FULL 40-hex lowercase commit SHA of Comfy-Org/github-workflows (got '${safe_ref}'). Pin it to the SAME SHA you pin uses: to — see the pr-risk.yml header. Branches, tags and refs/pull/N/head are mutable (and PR-head refs resolve fork-authored code), so they are rejected before the tool checkout." exit 1 fi + # SECOND AXIS — ANCESTRY. Shape proves the value is IMMUTABLE; it does not prove WHICH + # REPOSITORY authored the commit it names. A fork of this PUBLIC repo shares the upstream + # object store, and GitHub serves a fork PR's head objects from THIS repo's URL to an + # UNAUTHENTICATED client — so a fork-authored 40-hex SHA passes the shape test above and is + # then handed to `actions/checkout` and run inside a job holding the caller's write token. + # Assert instead that the commit is REACHABLE FROM UPSTREAM `main`: merged upstream history, + # and nothing else, may be pinned. There is deliberately NO opt-out input for this — an + # opt-out would be set by the very pin-bump PR the check exists to distrust. + # + # THE URL IS A LITERAL ON PURPOSE. Inside a reusable workflow `github.repository` names the + # CALLER, and `github.job_workflow_ref` only repeats the `uses:` string the caller wrote, so + # no context can supply this repo's identity. A variable here would be an alias a fork could + # point back at itself — the exact class of bug this axis closes. Unauthenticated is correct + # (the repo is public), and every failure mode fails CLOSED: a fetch that errors, a `main` + # that cannot be read, or a network outage all leave the step non-zero. + # + # TWO FETCHES, NOT ONE. The depth-1 fetch is what proves the SHA exists in this repo's + # object network at all; the second brings `main` with an explicit refspec (rather than + # relying on remote-tracking config, which a bare-URL remote has none of) so that + # `merge-base --is-ancestor` walks a COMPLETE `main` history. The shallow boundary the first + # fetch leaves behind is harmless: the walk runs from `main` DOWN to the pinned commit and + # never needs that commit's parents. + # + # `git` ECHOES THE REF ITSELF on a failed fetch (`upload-pack: not our ref `), which + # looks like it contradicts the never-emit-the-raw-value rule above. It does not: the shape + # test has already exited on anything that is not exactly 40 characters of `[0-9a-f]`, and + # such a value can carry neither a newline nor a `::`, so it cannot forge a workflow command + # in this PUBLIC log. That ordering is load-bearing — do not move this axis above the shape + # test. + # + # WHAT THIS STILL DOES NOT PROVE is that the pin is the CURRENT one: a stale-but-merged SHA + # is an ancestor too, and lock-step against the commit `uses:` resolved to remains + # unavailable from in here (see the header). Reviewing the caller still bounds that. + scratch=$(mktemp -d) + git init -q "$scratch" + if ! git -C "$scratch" fetch --quiet --depth=1 --no-tags https://github.com/Comfy-Org/github-workflows "$WORKFLOWS_REF"; then + echo "::error::workflows_ref (${safe_ref}) could not be fetched from Comfy-Org/github-workflows, so its provenance cannot be established. Failing closed." + exit 1 + fi + if ! git -C "$scratch" fetch --quiet --no-tags https://github.com/Comfy-Org/github-workflows +refs/heads/main:refs/heads/upstream-main; then + echo "::error::main of Comfy-Org/github-workflows could not be read, so workflows_ref (${safe_ref}) cannot be shown to be merged upstream. Failing closed." + exit 1 + fi + if ! git -C "$scratch" merge-base --is-ancestor "$WORKFLOWS_REF" upstream-main; then + echo "::error::workflows_ref (${safe_ref}) is not an ancestor of Comfy-Org/github-workflows main. A well-shaped SHA can still be a FORK-AUTHORED commit served out of this repo's own object network; only merged upstream commits may be pinned. If this is a genuine just-merged pin, re-run; otherwise treat the pin-bump that introduced it as hostile." + exit 1 + fi # The tool checkout is the same pinned-ref load the grade job does: the resolver is this # repo's code at `workflows_ref`, never the graded PR's. - name: Load pr-risk tool @@ -648,6 +737,53 @@ jobs: echo "::error::workflows_ref must be the FULL 40-hex lowercase commit SHA of Comfy-Org/github-workflows (got '${safe_ref}'). Pin it to the SAME SHA you pin uses: to — see the pr-risk.yml header. Branches, tags and refs/pull/N/head are mutable (and PR-head refs resolve fork-authored code), so they are rejected before the tool checkout." exit 1 fi + # SECOND AXIS — ANCESTRY. Shape proves the value is IMMUTABLE; it does not prove WHICH + # REPOSITORY authored the commit it names. A fork of this PUBLIC repo shares the upstream + # object store, and GitHub serves a fork PR's head objects from THIS repo's URL to an + # UNAUTHENTICATED client — so a fork-authored 40-hex SHA passes the shape test above and is + # then handed to `actions/checkout` and run inside a job holding the caller's write token. + # Assert instead that the commit is REACHABLE FROM UPSTREAM `main`: merged upstream history, + # and nothing else, may be pinned. There is deliberately NO opt-out input for this — an + # opt-out would be set by the very pin-bump PR the check exists to distrust. + # + # THE URL IS A LITERAL ON PURPOSE. Inside a reusable workflow `github.repository` names the + # CALLER, and `github.job_workflow_ref` only repeats the `uses:` string the caller wrote, so + # no context can supply this repo's identity. A variable here would be an alias a fork could + # point back at itself — the exact class of bug this axis closes. Unauthenticated is correct + # (the repo is public), and every failure mode fails CLOSED: a fetch that errors, a `main` + # that cannot be read, or a network outage all leave the step non-zero. + # + # TWO FETCHES, NOT ONE. The depth-1 fetch is what proves the SHA exists in this repo's + # object network at all; the second brings `main` with an explicit refspec (rather than + # relying on remote-tracking config, which a bare-URL remote has none of) so that + # `merge-base --is-ancestor` walks a COMPLETE `main` history. The shallow boundary the first + # fetch leaves behind is harmless: the walk runs from `main` DOWN to the pinned commit and + # never needs that commit's parents. + # + # `git` ECHOES THE REF ITSELF on a failed fetch (`upload-pack: not our ref `), which + # looks like it contradicts the never-emit-the-raw-value rule above. It does not: the shape + # test has already exited on anything that is not exactly 40 characters of `[0-9a-f]`, and + # such a value can carry neither a newline nor a `::`, so it cannot forge a workflow command + # in this PUBLIC log. That ordering is load-bearing — do not move this axis above the shape + # test. + # + # WHAT THIS STILL DOES NOT PROVE is that the pin is the CURRENT one: a stale-but-merged SHA + # is an ancestor too, and lock-step against the commit `uses:` resolved to remains + # unavailable from in here (see the header). Reviewing the caller still bounds that. + scratch=$(mktemp -d) + git init -q "$scratch" + if ! git -C "$scratch" fetch --quiet --depth=1 --no-tags https://github.com/Comfy-Org/github-workflows "$WORKFLOWS_REF"; then + echo "::error::workflows_ref (${safe_ref}) could not be fetched from Comfy-Org/github-workflows, so its provenance cannot be established. Failing closed." + exit 1 + fi + if ! git -C "$scratch" fetch --quiet --no-tags https://github.com/Comfy-Org/github-workflows +refs/heads/main:refs/heads/upstream-main; then + echo "::error::main of Comfy-Org/github-workflows could not be read, so workflows_ref (${safe_ref}) cannot be shown to be merged upstream. Failing closed." + exit 1 + fi + if ! git -C "$scratch" merge-base --is-ancestor "$WORKFLOWS_REF" upstream-main; then + echo "::error::workflows_ref (${safe_ref}) is not an ancestor of Comfy-Org/github-workflows main. A well-shaped SHA can still be a FORK-AUTHORED commit served out of this repo's own object network; only merged upstream commits may be pinned. If this is a genuine just-merged pin, re-run; otherwise treat the pin-bump that introduced it as hostile." + exit 1 + fi - name: Load pr-risk tool # The grader + default map come from THIS workflow's repo (public, pinned # via workflows_ref) — never from the graded PR. No PR code is checked diff --git a/README.md b/README.md index cc79724..18130a9 100644 --- a/README.md +++ b/README.md @@ -20,7 +20,7 @@ complete, copy-pasteable caller. | [`assign-reviewers.yml`](.github/workflows/assign-reviewers.yml) | Auto-requests expertise-aware, load-balanced PR reviewers with new-folk randomization. Matches changed paths against a caller-repo `.github/reviewers.yml` (path-glob → reviewers, plus a `default_pool`), drops the author + `vars.REVIEWER_EXCLUDE`, ranks candidates by open review load (steering off anyone at/over `vars.REVIEWER_LOAD_CAP`), and may swap a slot for a `vars.REVIEWER_GROWTH_POOL` member. Requests go through the CLOUD_CODE_BOT app token so they work on fork PRs. Requires `vars.APP_ID` + `CLOUD_CODE_BOT_PRIVATE_KEY`. | [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) | | [`pr-size.yml`](.github/workflows/pr-size.yml) | PR-size cap — fails (or, in `mode: warn`, only reports) when a PR's net diff exceeds `max_lines` non-generated changed lines, keeping diffs reviewable. Excludes dependency lockfiles, `linguist-generated` files (read from the base ref, so a PR can't exempt itself), Go generated-code markers, and per-repo `extra_lockfiles` / `extra_generated_globs`. A `bypass_label` (default `oversized-ok`) waves through a legitimately large change; a sticky bot comment explains overages when `bot_app_id` + `BOT_APP_PRIVATE_KEY` are supplied (degrades to status + step summary without them). Counting logic + tests live in [`scripts/check-pr-size/`](scripts/check-pr-size). | [pr-size.md](docs/callers/pr-size.md) | -| [`pr-risk.yml`](.github/workflows/pr-risk.yml) | **Advisory PR risk grading (shadow check)** — **automatic grading off by default** (`enabled: false`; a manual `workflow_dispatch` grades regardless, so a repo can trial it before switching on); switch it on with `enabled: true` or by setting the caller repo's `RISK_CONFIG` variable to `{"enabled": true}`, which outranks the input in both directions so `{"enabled": false}` is a no-PR kill switch. Grades every PR into a tier `R0` (safest) .. `R3` (riskiest) and syncs one label (`risk:R0`..`risk:R3`, or `risk:ungraded` when an input was unreadable). The label is the entire product: nothing is gated, routed, commented, or merged. Deterministic (`gh` + `jq`, no LLM): `grade = worst(path_floor, provenance, reversibility)` — path-glob map, what-process-produced-the-diff (registered runbooks with identity + diff-shape assertions; forks are R3 with no exceptions), and revertability (persistent-state mutation, deletions under sensitive classes, did green checks cover the lines). Grader + generic defaults live in [`scripts/pr-risk/`](scripts/pr-risk); a consumer sharpens them with `.github/risk.json` / `.github/risk-runbooks.json`, read from the PR's **base ref** so a PR can't edit the rules that judge it. The job excludes its own run from the check rollup and waits (`wait_for_checks_minutes`) for the rest to settle before labeling. Labels ride the plain `GITHUB_TOKEN` (cannot fire `labeled` triggers — no cascade risk); disagreement is recorded with a human-owned `risk-dispute` label. **Two further publish surfaces are available and are OFF by default**, so an enrolled caller behaves byte-identically until it opts in: `sticky_comment: true` posts ONE comment (created once, updated in place — N pushes leave one comment) carrying the per-file path-axis breakdown, the risk CONCENTRATION sentence ("94% of this diff is R0/R1; the 6% that puts the path floor at R3 is these two files, 40 lines") and a "this grade is wrong" checkbox whose state round-trips into a `risk-grade-disputed` label (distinct from the human-owned `risk-dispute`, which the grader still never touches); `check_run: true` publishes the tier and reason as a Check Run on the head commit — the immutable, timestamped, commit-attached record a mutable label cannot be — from a SEPARATE job, so it is the only surface that needs an extra `checks: write` grant in the caller's block, and only when switched on. Both surfaces are advisory in the same sense as the label: the Check Run's conclusion is hardcoded `neutral`, and every publish failure is an annotation, never a red check. Label text is remappable via `label_map`. `workflows_ref` is **required**, and its **shape is enforced** — every job that checks it out fails the run *before* the tool checkout unless the value is a full 40-hex lowercase commit SHA, so a branch, a tag or a `refs/pull/N/head` is rejected and the grader cannot be loaded from a floating ref after the caller was reviewed. **That is the whole of what is machine-checked, and it is not provenance.** Shape says the ref is immutable, never *which* commit it is: a fork of this public repo shares its object store, so a fork-authored SHA — or a pin left behind when `uses:` moved — is just as well-shaped. The test that would close that is "equal to the commit `uses:` resolved to", and the runner does not expose it to the workflow (`github.workflow_sha` is the *caller's* top-level file; `job_workflow_sha` is an OIDC claim, not a `github` context property, so reading it would need `id-token: write` from every caller). **Reviewing the caller is what bounds it, and it is the only thing that can: require `uses:` at a full commit SHA of this repo and `with: workflows_ref:` set to that same SHA written out literally, character-for-character — never an expression, never a tag.** The guard also runs *before* enablement is resolved (the resolver is itself loaded from `workflows_ref`), so a floating pin fails red even with the `RISK_CONFIG` kill switch set — the switch stops the grading, not a broken enrollment. Call the workflow directly: a nested `workflow_call` chain through an org wrapper is unsupported. Enroll it as its own workflow rather than a job inside an existing CI workflow (the rollup exclusion is per-run). The calling job needs `contents: read` + `issues: write` + `pull-requests: write` + `checks: read` + `actions: read` + `statuses: read`; GitHub rejects a shorter grant at startup (a reusable workflow can only narrow the caller's token, never elevate it), so a caller enrolled from an older copy of this row fails before any step runs. Both writes are the ONE label: repo-side label creation on first use maps to `issues`, and labeling a PR maps to `pull-requests` (the labels endpoint is dual-mapped by what the "issue" is, so `issues: write` alone 403s on a PR). `actions: read` is for the rollup's `CheckRun -> checkSuite -> workflowRun` self-exclusion hop. No secrets. | [pr-risk.md](docs/callers/pr-risk.md) | +| [`pr-risk.yml`](.github/workflows/pr-risk.yml) | **Advisory PR risk grading (shadow check)** — **automatic grading off by default** (`enabled: false`; a manual `workflow_dispatch` grades regardless, so a repo can trial it before switching on); switch it on with `enabled: true` or by setting the caller repo's `RISK_CONFIG` variable to `{"enabled": true}`, which outranks the input in both directions so `{"enabled": false}` is a no-PR kill switch. Grades every PR into a tier `R0` (safest) .. `R3` (riskiest) and syncs one label (`risk:R0`..`risk:R3`, or `risk:ungraded` when an input was unreadable). The label is the entire product: nothing is gated, routed, commented, or merged. Deterministic (`gh` + `jq`, no LLM): `grade = worst(path_floor, provenance, reversibility)` — path-glob map, what-process-produced-the-diff (registered runbooks with identity + diff-shape assertions; forks are R3 with no exceptions), and revertability (persistent-state mutation, deletions under sensitive classes, did green checks cover the lines). Grader + generic defaults live in [`scripts/pr-risk/`](scripts/pr-risk); a consumer sharpens them with `.github/risk.json` / `.github/risk-runbooks.json`, read from the PR's **base ref** so a PR can't edit the rules that judge it. The job excludes its own run from the check rollup and waits (`wait_for_checks_minutes`) for the rest to settle before labeling. Labels ride the plain `GITHUB_TOKEN` (cannot fire `labeled` triggers — no cascade risk); disagreement is recorded with a human-owned `risk-dispute` label. **Two further publish surfaces are available and are OFF by default**, so an enrolled caller behaves byte-identically until it opts in: `sticky_comment: true` posts ONE comment (created once, updated in place — N pushes leave one comment) carrying the per-file path-axis breakdown, the risk CONCENTRATION sentence ("94% of this diff is R0/R1; the 6% that puts the path floor at R3 is these two files, 40 lines") and a "this grade is wrong" checkbox whose state round-trips into a `risk-grade-disputed` label (distinct from the human-owned `risk-dispute`, which the grader still never touches); `check_run: true` publishes the tier and reason as a Check Run on the head commit — the immutable, timestamped, commit-attached record a mutable label cannot be — from a SEPARATE job, so it is the only surface that needs an extra `checks: write` grant in the caller's block, and only when switched on. Both surfaces are advisory in the same sense as the label: the Check Run's conclusion is hardcoded `neutral`, and every publish failure is an annotation, never a red check. Label text is remappable via `label_map`. `workflows_ref` is **required**, and both its **shape and its ancestry are enforced** — every job that checks it out fails the run *before* the tool checkout unless the value is a full 40-hex lowercase commit SHA **and** that commit is an ancestor of `main` of this repo (fetched from a literal upstream URL; no `github` context can name this repo from inside a reusable workflow, so a variable there would be an alias a fork could point at itself). Shape rejects everything mutable — a branch, a tag, a `refs/pull/N/head`. Ancestry rejects everything unmerged: a fork of this public repo shares its object store, and GitHub serves a fork PR's head objects from this repo's own URL unauthenticated, so a fork-authored SHA is just as well-shaped and would otherwise be checked out into a job holding the caller's write token. There is deliberately **no opt-out input** — an opt-out would be set by the very pin-bump PR the check distrusts. Two consequences by design: **pinning a not-yet-merged SHA is rejected** (merge first, then bump), and the axis **fails closed** if this repo ever goes private or `main` is force-rewritten past a consumer's pin (re-pin to a commit on `main` to recover). **What is still not machine-checked is that the pin is the *current* one**: a stale pin left behind when `uses:` moved is a merged ancestor too. The test that would close that is "equal to the commit `uses:` resolved to", and the runner does not expose it to the workflow (`github.workflow_sha` is the *caller's* top-level file; `job_workflow_sha` is an OIDC claim, not a `github` context property, so reading it would need `id-token: write` from every caller). The other residual is irreducible: a fork commit that edits `pr-risk.yml` itself is rejected by ancestry, but a caller pinned at one runs the fork's own copy of the guard, and no in-file check can bound a revision of that file an attacker chose. **Reviewing the caller is what bounds both, and it is the only thing that can: require `uses:` at a full commit SHA of this repo and `with: workflows_ref:` set to that same SHA written out literally, character-for-character — never an expression, never a tag.** The guard also runs *before* enablement is resolved (the resolver is itself loaded from `workflows_ref`), so a floating pin fails red even with the `RISK_CONFIG` kill switch set — the switch stops the grading, not a broken enrollment. Call the workflow directly: a nested `workflow_call` chain through an org wrapper is unsupported. Enroll it as its own workflow rather than a job inside an existing CI workflow (the rollup exclusion is per-run). The calling job needs `contents: read` + `issues: write` + `pull-requests: write` + `checks: read` + `actions: read` + `statuses: read`; GitHub rejects a shorter grant at startup (a reusable workflow can only narrow the caller's token, never elevate it), so a caller enrolled from an older copy of this row fails before any step runs. Both writes are the ONE label: repo-side label creation on first use maps to `issues`, and labeling a PR maps to `pull-requests` (the labels endpoint is dual-mapped by what the "issue" is, so `issues: write` alone 403s on a PR). `actions: read` is for the rollup's `CheckRun -> checkSuite -> workflowRun` self-exclusion hop. No secrets. | [pr-risk.md](docs/callers/pr-risk.md) | | [`stale.yml`](.github/workflows/stale.yml) | Stale-PR sweeper (`actions/stale`) plus a Slack digest of what it touched. PRs inactive for N days are labeled `stale`; still-inactive PRs are closed. The digest header names the source repo so batches from different repos posted to the same channel are unambiguous. Thresholds, messages, exempt labels, and the Slack channel are inputs; the caller owns the schedule + dry-run toggle. The calling job needs `pull-requests: write` and `issues: write`. Optional `SLACK_BOT_TOKEN`. | [stale.md](docs/callers/stale.md) | | [`groom.yml`](.github/workflows/groom.yml) | Scheduled/dispatch org-wide **code-cleanup sweep** (finds only — no commits, no PRs, never merges). A read-only FINDER agent scans a clean default-branch checkout (whole-repo, not a diff) for high-value refactors; an INDEPENDENT VERIFIER agent (fresh session) re-checks each as CONFIRM/DOWNGRADE/REJECT with a stable dedup signature; survivors are deduped against a durable GitHub-issue-state ledger and filed as `groom`-labeled GitHub issues (security-adjacent ones get `groom-security` — investigate, don't auto-implement). Mirrors the cursor-review topology: briefs + ledger live in [`.github/groom/`](.github/groom) as the single source of truth. The finder/verifier/builder agent jobs invoke the Claude CLI directly and mint no GitHub token, so they need nothing beyond `contents: read`; filing runs in a separate job as the bot you configure via `bot_app_id` (Comfy: cloud-code-bot). `dry_run` reports what it would file without opening issues. Runs on a **daily base cron** with a runtime cadence gate: set repo Actions variable `GROOM_INTERVAL_DAYS` (default 7 = weekly) to retune how often a real run happens — weekly → every-3-days → daily — with no workflow-file edit; a tick within the interval no-ops before the finder (`workflow_dispatch` bypasses the interval gate, but the volume gate — when the caller leaves it on — still applies). The calling job must grant `contents: read` + `issues: write` + `pull-requests: read` + `actions: read` — the first three are declared by the `file` / `build_select` jobs (needed even with `bot_app_id` set), and the interval gate needs `actions: read` (reads run history for the last real run); GitHub rejects a shorter grant at startup. Requires `ANTHROPIC_API_KEY` (+ `BOT_APP_PRIVATE_KEY` when `bot_app_id` is set). **Opt-in auto-builder** (`builder: true`, BE-4003): the top `max_prs` (default 5) CONFIRMED, non-security findings become **review-gated PRs** (full CI + cursor-review, **never auto-merged**) instead of issues; a credential-free `build` job emits only a patch artifact and a separate `build_pr` job opens the PR as the bot, preserving the security boundary. The ledger's PR-state (open/merged/closed) stops a built finding being re-proposed. Requires `bot_app_id`. `max_prs` is typed **`string`**, not `number`, so a caller can forward its own `workflow_dispatch` input straight through (`max_prs: ${{ github.event.inputs.max_prs \|\| '1' }}`) and let an operator raise the ceiling for one manual run — no `fromJSON()` cast in the caller, and the parse/clamp (empty → default, non-numeric → 0 PRs + warning, never a failed run) happens once inside the reusable. A build that cannot become a PR (patch over `pr_size_limit`, patch touching CI-privileged paths) **bails** to a `groom` issue so the paid-for work isn't lost — that path lives in `build_pr`, so **`max_findings` does not cap it** and `max_findings: 0` alone does not silence it; set `bail_sink: none` (an operational knob, so `GROOM_CONFIG` can set it with no PR) to file nothing and get a run-log warning + summary line instead. | [groom.md](docs/callers/groom.md) | | [`agents-md-integrity.yml`](.github/workflows/agents-md-integrity.yml) | Enforces the Comfy `AGENTS.md` standard on the caller repo: a top-level `AGENTS.md` must exist and stay under a hard line ceiling (`max_lines`, default 200; warns over `warn_lines`, default 150), a `CLAUDE.md` (if present) must be a thin `@AGENTS.md` shim rather than a divergent copy, no legacy `.cursorrules` (gated `forbid_cursorrules`), every nested monorepo `AGENTS.md` needs a sibling `@AGENTS.md` shim and to be under the ceiling (gated `check_nested`), and `AGENTS.md` should have a CODEOWNERS DRI (`require_codeowners`, warn-only by default). `exclude_paths` (newline-/comma-separated globs, default empty) carves payload subtrees — a repo whose product IS agent instructions, e.g. a plugin marketplace shipping `plugins/**/AGENTS.md` + a real `CLAUDE.md` — out of the nested scan without the all-or-nothing `check_nested: false`; exclusions are applied during the walk (never scanned or line-counted), reported in the log as `EXCLUDED: (matched )`, and a glob that would exclude the ROOT `AGENTS.md`/`CLAUDE.md` — or the whole tree without saying so (`/`, `*`, `*/**`) — is rejected (exit 2). Fails with a non-zero exit + GitHub annotations so it wires in as a required status check. The checker lives in [`.github/agents-md-integrity/`](.github/agents-md-integrity) (pin `workflows_ref` to the same ref as `uses:`); no secrets required. | [agents-md-integrity.md](docs/callers/agents-md-integrity.md) | diff --git a/docs/callers/pr-risk.md b/docs/callers/pr-risk.md index 7d6e9e5..7c7259f 100644 --- a/docs/callers/pr-risk.md +++ b/docs/callers/pr-risk.md @@ -77,7 +77,7 @@ statuses: read | Input | Default | Notes | |---|---|---| -| `workflows_ref` | — (**required**) | Pin to the SAME full commit SHA as `uses:`. No default on purpose: a floating default let a caller SHA-pin `uses:` and still load the grader from HEAD of main. | +| `workflows_ref` | — (**required**) | Pin to the SAME full commit SHA as `uses:`. No default on purpose: a floating default let a caller SHA-pin `uses:` and still load the grader from HEAD of main. Checked before the tool checkout on two axes: it must be a full 40-hex lowercase SHA, **and** that commit must be an ancestor of `main` of this repo. So a branch, a tag, a `refs/pull/N/head` and any **not-yet-merged** SHA all fail the run — **merge the change here first, then bump the pin.** There is no opt-out. | | `fleet_logins` | `mattmillerai` | Logins whose PRs grade provenance `agent-supervised` alongside `agent-coded`. | | `bot_logins` | `github-actions,dependabot,renovate,coderabbitai,cursor,comfy-pr-bot,web-flow` | Extra logins treated as bots. A bot with no runbook entry still grades as human — identity alone buys no trust. | | `label_map` | `''` | Rename the five grader-owned labels as `tier=label` pairs. Tier keys are fixed; only the label text is yours. | diff --git a/scripts/pr-risk/tests/test_pin_contract.sh b/scripts/pr-risk/tests/test_pin_contract.sh index 728c9a9..83f296b 100644 --- a/scripts/pr-risk/tests/test_pin_contract.sh +++ b/scripts/pr-risk/tests/test_pin_contract.sh @@ -41,9 +41,18 @@ # `if [[ -n "$REF" ]] && echo "$REF" >> "$GITHUB_STEP_SUMMARY"` passed), its TRIGGER was itself a # blacklist (so `printenv WORKFLOWS_REF` and `set -euxo pipefail` were invisible), and a `has` # needle proves a test is PRESENT, never that it is the only path to the checkout. A trust -# boundary this small — nine executable lines — is better served by an equality: the body is what -# it is below, or the build is red. Widening it is then a deliberate two-place edit whose diff a -# reviewer sees, which is the property all those scans were reaching for. +# boundary this small — a couple of dozen executable lines — is better served by an equality: the +# body is what it is below, or the build is red. Widening it is then a deliberate two-place edit +# whose diff a reviewer sees, which is the property all those scans were reaching for. +# +# THE NAMED PROPERTIES ARE RESTATEMENTS, NOT THE DEFENCE. A few of the patterns disparaged above +# do appear below — the ancestry anchor, the literal-URL check, "every `::error::` is followed by +# an `exit 1`". They are not a second line of defence and must never be read as one: the equality +# already fails on every mutation they catch, and each of them is exactly as bypassable as the +# earlier drafts showed. They are here so that the COMMON failure — someone editing the guard on +# purpose — reports WHICH invariant they broke instead of only printing a diff of the whole body. +# A new property in this file is worth adding when it names a rule; it is never worth adding as a +# substitute for extending the pinned body. # # STRUCTURE IS PINNED; PROSE IS NOT. The `::error::` message is free to be reworded (it is # canonicalized away before the comparison) but is separately checked to expand nothing but the @@ -177,6 +186,20 @@ expect="$(cat <<'PINNED' echo "::error::" exit 1 fi + scratch=$(mktemp -d) + git init -q "$scratch" + if ! git -C "$scratch" fetch --quiet --depth=1 --no-tags https://github.com/Comfy-Org/github-workflows "$WORKFLOWS_REF"; then + echo "::error::" + exit 1 + fi + if ! git -C "$scratch" fetch --quiet --no-tags https://github.com/Comfy-Org/github-workflows +refs/heads/main:refs/heads/upstream-main; then + echo "::error::" + exit 1 + fi + if ! git -C "$scratch" merge-base --is-ancestor "$WORKFLOWS_REF" upstream-main; then + echo "::error::" + exit 1 + fi PINNED )" # Blank lines are noise here (the slicer keeps them so the byte-identity check above sees them), @@ -201,6 +224,47 @@ else bad "the annotation expands nothing but the sanitized copy" "$errline" fi +# --- the ancestry axis, stated by name --------------------------------------------------------- +# All three assertions below are CONSEQUENCES of the equality above — deleting the ancestry block +# from both copies already fails it. They are restated by name for the same reason the env-binding +# rule is: a failure that says "the guard no longer asserts ancestry" beats one that only prints a +# diff, and the axis is the whole reason a fork-authored SHA cannot be checked out here. Each one +# carries its own coverage self-check, so a stale anchor fails loudly rather than passing vacuously. +ANCESTRY_LINE=' if ! git -C "$scratch" merge-base --is-ancestor "$WORKFLOWS_REF" upstream-main; then' +nancestry=$(grep -cxF "$ANCESTRY_LINE" "$WF") +eq "every copy of the guard asserts the pin is an ancestor of upstream main" "$guards" "$nancestry" + +# THE URL MUST BE A LITERAL. `github.repository` names the CALLER inside a reusable workflow and +# `github.job_workflow_ref` only repeats the caller's `uses:` string, so nothing in the `github` +# context can name THIS repo — which makes any expression in this position an alias a fork could +# point back at itself, turning "is it an ancestor of upstream main?" into "is it an ancestor of +# the attacker's main?". Comments are stripped first so the prose above the axis, which names the +# URL, cannot satisfy this. Tokenizing on spaces and keeping the URL-shaped words is what makes a +# `${{ github.server_url }}/${{ github.repository }}` substitution fail rather than sneak through: +# neither fragment is URL-shaped, so the comparison sees an empty set instead of the literal. +UPSTREAM_URL='https://github.com/Comfy-Org/github-workflows' +fetchlines="$(grep -v '^[[:space:]]*#' "$WF" | grep -F 'git -C "$scratch" fetch')" +nfetch=$(printf '%s\n' "$fetchlines" | grep -cF 'git -C "$scratch" fetch') +if [ "$nfetch" = "$((guards * 2))" ]; then ok "the ancestry fetch scan matched both fetches in every copy ($nfetch)" +else bad "the ancestry fetch scan matched both fetches in every copy" "$nfetch of $((guards * 2)) — anchors are stale, coverage is vacuous"; fi +urltokens="$(printf '%s\n' "$fetchlines" | tr ' ' '\n' | grep -E '://|github\.com' | sort -u | tr '\n' ' ' | sed 's/ $//')" +eq "the ancestry fetch names the upstream repo by literal URL, never an expression" "$UPSTREAM_URL" "$urltokens" + +# EVERY REJECTION PATH IS FATAL. The ancestry axis adds two of them — an unfetchable SHA and a +# non-ancestor one — and a rejection that annotates without exiting lets the checkout proceed on +# exactly the ref the axis just rejected. +fatal="$(printf '%s\n' "$code" | awk ' + /::error::/ { pending = NR; next } + pending && $0 !~ /^[[:space:]]*exit 1[[:space:]]*$/ { print "NOEXIT:" pending } + { pending = 0 } + END { if (pending) print "NOEXIT:" pending } +' | tr '\n' ' ' | sed 's/ $//')" +eq "every ::error:: in the guard is immediately followed by exit 1" "" "$fatal" +nerr=$(printf '%s\n' "$code" | grep -cF '::error::') +if [ "$nerr" -ge 3 ]; then ok "the fatality scan saw every rejection path ($nerr)" +else bad "the fatality scan saw every rejection path" "$nerr — anchors are stale, coverage is vacuous"; fi +no "no path through the guard exits 0 early" "$code" "exit 0" + # --- the guard cannot be neutered while staying byte-identical -------------------------------- # The cheapest way to disarm this without tripping any check above is a step-level key: # `continue-on-error: true` makes the `exit 1` advisory, and an `if:` can switch the whole step From c85f12404ce226fa1987f49c5414e9bbe65a877b Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Wed, 5 Aug 2026 06:37:28 -0700 Subject: [PATCH 2/3] fix(pr-risk): make the ancestry axis survive a bad network minute (BE-6492) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .github/workflows/pr-risk.yml | 174 ++++++++++++++++++--- scripts/pr-risk/tests/test_pin_contract.sh | 88 ++++++++--- 2 files changed, 213 insertions(+), 49 deletions(-) diff --git a/.github/workflows/pr-risk.yml b/.github/workflows/pr-risk.yml index e687387..eeefd0a 100644 --- a/.github/workflows/pr-risk.yml +++ b/.github/workflows/pr-risk.yml @@ -407,6 +407,20 @@ on: Recovery is to re-pin to a commit that is on `main`. + A RED RUN HERE IS NOT A NETWORK BLIP. Because this makes two + unauthenticated fetches a hard precondition of every run, each one is + retried three times with a linear backoff and bounded by git's + low-speed timeout, so a transient github.com error or a secondary rate + limit on shared Actions egress resolves itself rather than reddening + every enrolled consumer at once. What is never retried is the VERDICT. + If the step still fails, read the annotation: it distinguishes a pin + that could not be FETCHED, a `main` that could not be READ, an ancestry + test that could not be COMPLETED (an infrastructure fault, not a + judgement about the pin), and a pin that is genuinely NOT AN ANCESTOR — + which is the only one of the four that says anything about the pin + itself. + + WHAT ANCESTRY DOES NOT PROVE is that the pin is the CURRENT one: a pin left behind when `uses:` moved is a merged ancestor too, and passes. The test that would close that is "equal to the commit `uses:` resolved @@ -582,12 +596,29 @@ jobs: # (the repo is public), and every failure mode fails CLOSED: a fetch that errors, a `main` # that cannot be read, or a network outage all leave the step non-zero. # - # TWO FETCHES, NOT ONE. The depth-1 fetch is what proves the SHA exists in this repo's - # object network at all; the second brings `main` with an explicit refspec (rather than - # relying on remote-tracking config, which a bare-URL remote has none of) so that - # `merge-base --is-ancestor` walks a COMPLETE `main` history. The shallow boundary the first - # fetch leaves behind is harmless: the walk runs from `main` DOWN to the pinned commit and - # never needs that commit's parents. + # TWO FETCHES, NOT ONE, AND BOTH ARE BOUNDED. The depth-1 fetch is what proves the SHA + # exists in this repo's object network at all; the second brings `main` with an explicit + # refspec (rather than relying on remote-tracking config, which a bare-URL remote has none + # of) to walk down to the pin. `--filter=blob:none` holds both to a commit/tree graph — no + # file CONTENT is ever downloaded, because `merge-base` reads none. That bound matters most + # on the REJECTION path: when the pin is not an ancestor there is no shallow boundary + # anywhere in `main`'s ancestry, so the hostile pin is the case that fetches the MOST. + # + # THE SHALLOW BOUNDARY IS REAL, AND HARMLESS. The depth-1 fetch registers a boundary at the + # pinned commit and the server honours it on the second fetch, so `main` arrives TRUNCATED + # there — it is not a complete history and must not be reasoned about as one. What saves it + # is the direction of the walk: `merge-base --is-ancestor` runs from `main` DOWN to the + # pinned commit and stops on reaching it, so it never needs that commit's parents. Do not + # reorder these two fetches, or drop `--depth=1`, on the strength of a completeness the + # second fetch does not actually deliver. + # + # THE FETCH RETRIES; THE VERDICT DOES NOT. Two unauthenticated round-trips are now a hard + # precondition for every run in every enrolled consumer, and there is no opt-out — so a + # transient github.com error or a secondary rate limit on shared Actions egress would + # otherwise turn one bad minute into a fleet-wide red. Three attempts with a linear backoff + # absorb that, and the low-speed knobs turn a stalled connection into a fast failure rather + # than a hang to the job timeout. What does NOT get retried is the ANSWER: a fetch that + # keeps failing, and a commit that is simply not an ancestor, both still fail the step. # # `git` ECHOES THE REF ITSELF on a failed fetch (`upload-pack: not our ref `), which # looks like it contradicts the never-emit-the-raw-value rule above. It does not: the shape @@ -599,17 +630,54 @@ jobs: # WHAT THIS STILL DOES NOT PROVE is that the pin is the CURRENT one: a stale-but-merged SHA # is an ancestor too, and lock-step against the commit `uses:` resolved to remains # unavailable from in here (see the header). Reviewing the caller still bounds that. - scratch=$(mktemp -d) - git init -q "$scratch" - if ! git -C "$scratch" fetch --quiet --depth=1 --no-tags https://github.com/Comfy-Org/github-workflows "$WORKFLOWS_REF"; then - echo "::error::workflows_ref (${safe_ref}) could not be fetched from Comfy-Org/github-workflows, so its provenance cannot be established. Failing closed." + # + # THE SCRATCH CLONE IS CLEANED UP on every path, success or rejection. Hosted runners are + # ephemeral and would not care; self-hosted ones would accumulate one clone of `main` per + # run, per job. + if ! scratch=$(mktemp -d); then + echo "::error::A scratch directory for the workflows_ref (${safe_ref}) ancestry check could not be created, so the pin's provenance cannot be established. This is an infrastructure fault, not evidence about the pin. Failing closed." + exit 1 + fi + trap 'rm -rf "$scratch"' EXIT + if ! git init -q "$scratch"; then + echo "::error::A scratch git repository for the workflows_ref (${safe_ref}) ancestry check could not be initialized, so the pin's provenance cannot be established. This is an infrastructure fault, not evidence about the pin. Failing closed." + exit 1 + fi + # The upstream URL is passed IN by each call site rather than baked in here, so that both + # fetches keep naming the repo literally, in the line a reviewer reads, while sharing one + # retry and transfer-bound policy. See the literal-URL note above for why it is not a var. + fetch_upstream() { + for attempt in 1 2 3; do + if git -C "$scratch" -c http.lowSpeedLimit=1000 -c http.lowSpeedTime=30 fetch --quiet --no-tags --filter=blob:none "$@"; then + return 0 + fi + if [ "$attempt" -lt 3 ]; then + sleep $((attempt * 5)) + fi + done + return 1 + } + if ! fetch_upstream --depth=1 https://github.com/Comfy-Org/github-workflows "$WORKFLOWS_REF"; then + echo "::error::workflows_ref (${safe_ref}) could not be fetched from Comfy-Org/github-workflows after 3 attempts, so its provenance cannot be established. Failing closed." + exit 1 + fi + if ! fetch_upstream https://github.com/Comfy-Org/github-workflows +refs/heads/main:refs/heads/upstream-main; then + echo "::error::main of Comfy-Org/github-workflows could not be read after 3 attempts, so workflows_ref (${safe_ref}) cannot be shown to be merged upstream. Failing closed." exit 1 fi - if ! git -C "$scratch" fetch --quiet --no-tags https://github.com/Comfy-Org/github-workflows +refs/heads/main:refs/heads/upstream-main; then - echo "::error::main of Comfy-Org/github-workflows could not be read, so workflows_ref (${safe_ref}) cannot be shown to be merged upstream. Failing closed." + # `--is-ancestor` HAS THREE OUTCOMES, NOT TWO: 0 is yes, 1 is no, and anything above 1 is + # git failing to answer at all (an unresolvable `upstream-main`, a missing or corrupt + # object, a fetched ref that is not a commit). Collapsing those into the `no` branch would + # report an infrastructure fault to the consumer as "treat the pin-bump that introduced + # this as hostile" — the same cry-wolf the retry above exists to avoid. Both branches still + # fail closed; only the message differs. + ancestry_rc=0 + git -C "$scratch" merge-base --is-ancestor "$WORKFLOWS_REF" upstream-main || ancestry_rc=$? + if [ "$ancestry_rc" -gt 1 ]; then + echo "::error::The ancestry test for workflows_ref (${safe_ref}) could not be COMPLETED — git merge-base failed rather than returning a verdict (see its output above). This is an infrastructure fault, not evidence the pin is hostile. Failing closed." exit 1 fi - if ! git -C "$scratch" merge-base --is-ancestor "$WORKFLOWS_REF" upstream-main; then + if [ "$ancestry_rc" -ne 0 ]; then echo "::error::workflows_ref (${safe_ref}) is not an ancestor of Comfy-Org/github-workflows main. A well-shaped SHA can still be a FORK-AUTHORED commit served out of this repo's own object network; only merged upstream commits may be pinned. If this is a genuine just-merged pin, re-run; otherwise treat the pin-bump that introduced it as hostile." exit 1 fi @@ -753,12 +821,29 @@ jobs: # (the repo is public), and every failure mode fails CLOSED: a fetch that errors, a `main` # that cannot be read, or a network outage all leave the step non-zero. # - # TWO FETCHES, NOT ONE. The depth-1 fetch is what proves the SHA exists in this repo's - # object network at all; the second brings `main` with an explicit refspec (rather than - # relying on remote-tracking config, which a bare-URL remote has none of) so that - # `merge-base --is-ancestor` walks a COMPLETE `main` history. The shallow boundary the first - # fetch leaves behind is harmless: the walk runs from `main` DOWN to the pinned commit and - # never needs that commit's parents. + # TWO FETCHES, NOT ONE, AND BOTH ARE BOUNDED. The depth-1 fetch is what proves the SHA + # exists in this repo's object network at all; the second brings `main` with an explicit + # refspec (rather than relying on remote-tracking config, which a bare-URL remote has none + # of) to walk down to the pin. `--filter=blob:none` holds both to a commit/tree graph — no + # file CONTENT is ever downloaded, because `merge-base` reads none. That bound matters most + # on the REJECTION path: when the pin is not an ancestor there is no shallow boundary + # anywhere in `main`'s ancestry, so the hostile pin is the case that fetches the MOST. + # + # THE SHALLOW BOUNDARY IS REAL, AND HARMLESS. The depth-1 fetch registers a boundary at the + # pinned commit and the server honours it on the second fetch, so `main` arrives TRUNCATED + # there — it is not a complete history and must not be reasoned about as one. What saves it + # is the direction of the walk: `merge-base --is-ancestor` runs from `main` DOWN to the + # pinned commit and stops on reaching it, so it never needs that commit's parents. Do not + # reorder these two fetches, or drop `--depth=1`, on the strength of a completeness the + # second fetch does not actually deliver. + # + # THE FETCH RETRIES; THE VERDICT DOES NOT. Two unauthenticated round-trips are now a hard + # precondition for every run in every enrolled consumer, and there is no opt-out — so a + # transient github.com error or a secondary rate limit on shared Actions egress would + # otherwise turn one bad minute into a fleet-wide red. Three attempts with a linear backoff + # absorb that, and the low-speed knobs turn a stalled connection into a fast failure rather + # than a hang to the job timeout. What does NOT get retried is the ANSWER: a fetch that + # keeps failing, and a commit that is simply not an ancestor, both still fail the step. # # `git` ECHOES THE REF ITSELF on a failed fetch (`upload-pack: not our ref `), which # looks like it contradicts the never-emit-the-raw-value rule above. It does not: the shape @@ -770,17 +855,54 @@ jobs: # WHAT THIS STILL DOES NOT PROVE is that the pin is the CURRENT one: a stale-but-merged SHA # is an ancestor too, and lock-step against the commit `uses:` resolved to remains # unavailable from in here (see the header). Reviewing the caller still bounds that. - scratch=$(mktemp -d) - git init -q "$scratch" - if ! git -C "$scratch" fetch --quiet --depth=1 --no-tags https://github.com/Comfy-Org/github-workflows "$WORKFLOWS_REF"; then - echo "::error::workflows_ref (${safe_ref}) could not be fetched from Comfy-Org/github-workflows, so its provenance cannot be established. Failing closed." + # + # THE SCRATCH CLONE IS CLEANED UP on every path, success or rejection. Hosted runners are + # ephemeral and would not care; self-hosted ones would accumulate one clone of `main` per + # run, per job. + if ! scratch=$(mktemp -d); then + echo "::error::A scratch directory for the workflows_ref (${safe_ref}) ancestry check could not be created, so the pin's provenance cannot be established. This is an infrastructure fault, not evidence about the pin. Failing closed." + exit 1 + fi + trap 'rm -rf "$scratch"' EXIT + if ! git init -q "$scratch"; then + echo "::error::A scratch git repository for the workflows_ref (${safe_ref}) ancestry check could not be initialized, so the pin's provenance cannot be established. This is an infrastructure fault, not evidence about the pin. Failing closed." + exit 1 + fi + # The upstream URL is passed IN by each call site rather than baked in here, so that both + # fetches keep naming the repo literally, in the line a reviewer reads, while sharing one + # retry and transfer-bound policy. See the literal-URL note above for why it is not a var. + fetch_upstream() { + for attempt in 1 2 3; do + if git -C "$scratch" -c http.lowSpeedLimit=1000 -c http.lowSpeedTime=30 fetch --quiet --no-tags --filter=blob:none "$@"; then + return 0 + fi + if [ "$attempt" -lt 3 ]; then + sleep $((attempt * 5)) + fi + done + return 1 + } + if ! fetch_upstream --depth=1 https://github.com/Comfy-Org/github-workflows "$WORKFLOWS_REF"; then + echo "::error::workflows_ref (${safe_ref}) could not be fetched from Comfy-Org/github-workflows after 3 attempts, so its provenance cannot be established. Failing closed." + exit 1 + fi + if ! fetch_upstream https://github.com/Comfy-Org/github-workflows +refs/heads/main:refs/heads/upstream-main; then + echo "::error::main of Comfy-Org/github-workflows could not be read after 3 attempts, so workflows_ref (${safe_ref}) cannot be shown to be merged upstream. Failing closed." exit 1 fi - if ! git -C "$scratch" fetch --quiet --no-tags https://github.com/Comfy-Org/github-workflows +refs/heads/main:refs/heads/upstream-main; then - echo "::error::main of Comfy-Org/github-workflows could not be read, so workflows_ref (${safe_ref}) cannot be shown to be merged upstream. Failing closed." + # `--is-ancestor` HAS THREE OUTCOMES, NOT TWO: 0 is yes, 1 is no, and anything above 1 is + # git failing to answer at all (an unresolvable `upstream-main`, a missing or corrupt + # object, a fetched ref that is not a commit). Collapsing those into the `no` branch would + # report an infrastructure fault to the consumer as "treat the pin-bump that introduced + # this as hostile" — the same cry-wolf the retry above exists to avoid. Both branches still + # fail closed; only the message differs. + ancestry_rc=0 + git -C "$scratch" merge-base --is-ancestor "$WORKFLOWS_REF" upstream-main || ancestry_rc=$? + if [ "$ancestry_rc" -gt 1 ]; then + echo "::error::The ancestry test for workflows_ref (${safe_ref}) could not be COMPLETED — git merge-base failed rather than returning a verdict (see its output above). This is an infrastructure fault, not evidence the pin is hostile. Failing closed." exit 1 fi - if ! git -C "$scratch" merge-base --is-ancestor "$WORKFLOWS_REF" upstream-main; then + if [ "$ancestry_rc" -ne 0 ]; then echo "::error::workflows_ref (${safe_ref}) is not an ancestor of Comfy-Org/github-workflows main. A well-shaped SHA can still be a FORK-AUTHORED commit served out of this repo's own object network; only merged upstream commits may be pinned. If this is a genuine just-merged pin, re-run; otherwise treat the pin-bump that introduced it as hostile." exit 1 fi diff --git a/scripts/pr-risk/tests/test_pin_contract.sh b/scripts/pr-risk/tests/test_pin_contract.sh index 83f296b..bff80e9 100644 --- a/scripts/pr-risk/tests/test_pin_contract.sh +++ b/scripts/pr-risk/tests/test_pin_contract.sh @@ -186,17 +186,41 @@ expect="$(cat <<'PINNED' echo "::error::" exit 1 fi - scratch=$(mktemp -d) - git init -q "$scratch" - if ! git -C "$scratch" fetch --quiet --depth=1 --no-tags https://github.com/Comfy-Org/github-workflows "$WORKFLOWS_REF"; then + if ! scratch=$(mktemp -d); then echo "::error::" exit 1 fi - if ! git -C "$scratch" fetch --quiet --no-tags https://github.com/Comfy-Org/github-workflows +refs/heads/main:refs/heads/upstream-main; then + trap 'rm -rf "$scratch"' EXIT + if ! git init -q "$scratch"; then echo "::error::" exit 1 fi - if ! git -C "$scratch" merge-base --is-ancestor "$WORKFLOWS_REF" upstream-main; then + fetch_upstream() { + for attempt in 1 2 3; do + if git -C "$scratch" -c http.lowSpeedLimit=1000 -c http.lowSpeedTime=30 fetch --quiet --no-tags --filter=blob:none "$@"; then + return 0 + fi + if [ "$attempt" -lt 3 ]; then + sleep $((attempt * 5)) + fi + done + return 1 + } + if ! fetch_upstream --depth=1 https://github.com/Comfy-Org/github-workflows "$WORKFLOWS_REF"; then + echo "::error::" + exit 1 + fi + if ! fetch_upstream https://github.com/Comfy-Org/github-workflows +refs/heads/main:refs/heads/upstream-main; then + echo "::error::" + exit 1 + fi + ancestry_rc=0 + git -C "$scratch" merge-base --is-ancestor "$WORKFLOWS_REF" upstream-main || ancestry_rc=$? + if [ "$ancestry_rc" -gt 1 ]; then + echo "::error::" + exit 1 + fi + if [ "$ancestry_rc" -ne 0 ]; then echo "::error::" exit 1 fi @@ -225,12 +249,12 @@ else fi # --- the ancestry axis, stated by name --------------------------------------------------------- -# All three assertions below are CONSEQUENCES of the equality above — deleting the ancestry block -# from both copies already fails it. They are restated by name for the same reason the env-binding -# rule is: a failure that says "the guard no longer asserts ancestry" beats one that only prints a -# diff, and the axis is the whole reason a fork-authored SHA cannot be checked out here. Each one -# carries its own coverage self-check, so a stale anchor fails loudly rather than passing vacuously. -ANCESTRY_LINE=' if ! git -C "$scratch" merge-base --is-ancestor "$WORKFLOWS_REF" upstream-main; then' +# Every assertion below is a CONSEQUENCE of the equality above — deleting the ancestry block from +# both copies already fails it. They are restated by name for the same reason the env-binding rule +# is: a failure that says "the guard no longer asserts ancestry" beats one that only prints a diff, +# and the axis is the whole reason a fork-authored SHA cannot be checked out here. Each one carries +# its own coverage self-check, so a stale anchor fails loudly rather than passing vacuously. +ANCESTRY_LINE=' git -C "$scratch" merge-base --is-ancestor "$WORKFLOWS_REF" upstream-main || ancestry_rc=$?' nancestry=$(grep -cxF "$ANCESTRY_LINE" "$WF") eq "every copy of the guard asserts the pin is an ancestor of upstream main" "$guards" "$nancestry" @@ -239,30 +263,48 @@ eq "every copy of the guard asserts the pin is an ancestor of upstream main" "$g # context can name THIS repo — which makes any expression in this position an alias a fork could # point back at itself, turning "is it an ancestor of upstream main?" into "is it an ancestor of # the attacker's main?". Comments are stripped first so the prose above the axis, which names the -# URL, cannot satisfy this. Tokenizing on spaces and keeping the URL-shaped words is what makes a -# `${{ github.server_url }}/${{ github.repository }}` substitution fail rather than sneak through: -# neither fragment is URL-shaped, so the comparison sees an empty set instead of the literal. +# URL, cannot satisfy this. +# +# PER LINE, NOT AS A SET — and that distinction is the whole check. An earlier draft `sort -u`'d the +# URL-shaped tokens out of ALL the fetch lines and compared the result to the literal, which made +# the assertion pass as long as ONE line still carried it: swap a single call for +# `${{ github.server_url }}/${{ github.repository }}` (neither fragment URL-shaped, so it +# contributes nothing to the set) and the remaining copies covered for it, with the coverage count +# green too. So it is counted instead: every call line carries the literal, AND no OTHER URL-shaped +# token appears on any of them. UPSTREAM_URL='https://github.com/Comfy-Org/github-workflows' -fetchlines="$(grep -v '^[[:space:]]*#' "$WF" | grep -F 'git -C "$scratch" fetch')" -nfetch=$(printf '%s\n' "$fetchlines" | grep -cF 'git -C "$scratch" fetch') +FETCH_CALL='if ! fetch_upstream ' +fetchlines="$(grep -v '^[[:space:]]*#' "$WF" | grep -F "$FETCH_CALL")" +nfetch=$(printf '%s\n' "$fetchlines" | grep -cF "$FETCH_CALL") if [ "$nfetch" = "$((guards * 2))" ]; then ok "the ancestry fetch scan matched both fetches in every copy ($nfetch)" else bad "the ancestry fetch scan matched both fetches in every copy" "$nfetch of $((guards * 2)) — anchors are stale, coverage is vacuous"; fi +nurl=$(printf '%s\n' "$fetchlines" | grep -cF "$UPSTREAM_URL") +eq "EVERY ancestry fetch names the upstream repo by literal URL, never an expression" "$nfetch" "$nurl" urltokens="$(printf '%s\n' "$fetchlines" | tr ' ' '\n' | grep -E '://|github\.com' | sort -u | tr '\n' ' ' | sed 's/ $//')" -eq "the ancestry fetch names the upstream repo by literal URL, never an expression" "$UPSTREAM_URL" "$urltokens" +eq "no ancestry fetch names any URL other than the upstream one" "$UPSTREAM_URL" "$urltokens" -# EVERY REJECTION PATH IS FATAL. The ancestry axis adds two of them — an unfetchable SHA and a -# non-ancestor one — and a rejection that annotates without exiting lets the checkout proceed on -# exactly the ref the axis just rejected. +# EVERY REJECTION PATH IS FATAL — a rejection that annotates without exiting lets the checkout +# proceed on exactly the ref it just rejected. The `if (pending)` in the first rule is not +# redundant: without it two CONSECUTIVE `::error::` lines would overwrite the first one's line +# number unreported, so a `echo ::error::A; echo ::error::B; exit 1` sequence — one annotation, no +# exit — would read as clean. fatal="$(printf '%s\n' "$code" | awk ' - /::error::/ { pending = NR; next } + /::error::/ { if (pending) print "NOEXIT:" pending; pending = NR; next } pending && $0 !~ /^[[:space:]]*exit 1[[:space:]]*$/ { print "NOEXIT:" pending } { pending = 0 } END { if (pending) print "NOEXIT:" pending } ' | tr '\n' ' ' | sed 's/ $//')" eq "every ::error:: in the guard is immediately followed by exit 1" "" "$fatal" +# The expected count is READ OFF THE PIN, not hardcoded. A literal threshold goes stale silently: +# the previous `-ge 3` was written when the guard had three rejection paths and stayed green as it +# grew to seven, so deleting one would have passed — the exact vacuous pass this line exists to +# stop. Deriving it from `$expect` makes adding or removing a path a one-place edit, with the floor +# below keeping the derivation itself from degenerating. nerr=$(printf '%s\n' "$code" | grep -cF '::error::') -if [ "$nerr" -ge 3 ]; then ok "the fatality scan saw every rejection path ($nerr)" -else bad "the fatality scan saw every rejection path" "$nerr — anchors are stale, coverage is vacuous"; fi +nerr_pinned=$(printf '%s\n' "$expect" | grep -cF '::error::') +if [ "$nerr_pinned" -ge 3 ]; then ok "the pinned body still declares a plausible number of rejection paths ($nerr_pinned)" +else bad "the pinned body still declares a plausible number of rejection paths" "$nerr_pinned — the pin itself has been gutted"; fi +eq "the fatality scan saw every rejection path the pin declares" "$nerr_pinned" "$nerr" no "no path through the guard exits 0 early" "$code" "exit 0" # --- the guard cannot be neutered while staying byte-identical -------------------------------- From 16a32527c63750432b952a308950fd11dc332a9e Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Wed, 5 Aug 2026 06:52:10 -0700 Subject: [PATCH 3/3] docs(pr-risk): the caller grant is all six, and checks: write is not optional (BE-6492) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- README.md | 2 +- docs/callers/pr-risk.md | 27 ++++++++++++++++++++++----- 2 files changed, 23 insertions(+), 6 deletions(-) diff --git a/README.md b/README.md index 18130a9..d351068 100644 --- a/README.md +++ b/README.md @@ -20,7 +20,7 @@ complete, copy-pasteable caller. | [`assign-reviewers.yml`](.github/workflows/assign-reviewers.yml) | Auto-requests expertise-aware, load-balanced PR reviewers with new-folk randomization. Matches changed paths against a caller-repo `.github/reviewers.yml` (path-glob → reviewers, plus a `default_pool`), drops the author + `vars.REVIEWER_EXCLUDE`, ranks candidates by open review load (steering off anyone at/over `vars.REVIEWER_LOAD_CAP`), and may swap a slot for a `vars.REVIEWER_GROWTH_POOL` member. Requests go through the CLOUD_CODE_BOT app token so they work on fork PRs. Requires `vars.APP_ID` + `CLOUD_CODE_BOT_PRIVATE_KEY`. | [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) | | [`pr-size.yml`](.github/workflows/pr-size.yml) | PR-size cap — fails (or, in `mode: warn`, only reports) when a PR's net diff exceeds `max_lines` non-generated changed lines, keeping diffs reviewable. Excludes dependency lockfiles, `linguist-generated` files (read from the base ref, so a PR can't exempt itself), Go generated-code markers, and per-repo `extra_lockfiles` / `extra_generated_globs`. A `bypass_label` (default `oversized-ok`) waves through a legitimately large change; a sticky bot comment explains overages when `bot_app_id` + `BOT_APP_PRIVATE_KEY` are supplied (degrades to status + step summary without them). Counting logic + tests live in [`scripts/check-pr-size/`](scripts/check-pr-size). | [pr-size.md](docs/callers/pr-size.md) | -| [`pr-risk.yml`](.github/workflows/pr-risk.yml) | **Advisory PR risk grading (shadow check)** — **automatic grading off by default** (`enabled: false`; a manual `workflow_dispatch` grades regardless, so a repo can trial it before switching on); switch it on with `enabled: true` or by setting the caller repo's `RISK_CONFIG` variable to `{"enabled": true}`, which outranks the input in both directions so `{"enabled": false}` is a no-PR kill switch. Grades every PR into a tier `R0` (safest) .. `R3` (riskiest) and syncs one label (`risk:R0`..`risk:R3`, or `risk:ungraded` when an input was unreadable). The label is the entire product: nothing is gated, routed, commented, or merged. Deterministic (`gh` + `jq`, no LLM): `grade = worst(path_floor, provenance, reversibility)` — path-glob map, what-process-produced-the-diff (registered runbooks with identity + diff-shape assertions; forks are R3 with no exceptions), and revertability (persistent-state mutation, deletions under sensitive classes, did green checks cover the lines). Grader + generic defaults live in [`scripts/pr-risk/`](scripts/pr-risk); a consumer sharpens them with `.github/risk.json` / `.github/risk-runbooks.json`, read from the PR's **base ref** so a PR can't edit the rules that judge it. The job excludes its own run from the check rollup and waits (`wait_for_checks_minutes`) for the rest to settle before labeling. Labels ride the plain `GITHUB_TOKEN` (cannot fire `labeled` triggers — no cascade risk); disagreement is recorded with a human-owned `risk-dispute` label. **Two further publish surfaces are available and are OFF by default**, so an enrolled caller behaves byte-identically until it opts in: `sticky_comment: true` posts ONE comment (created once, updated in place — N pushes leave one comment) carrying the per-file path-axis breakdown, the risk CONCENTRATION sentence ("94% of this diff is R0/R1; the 6% that puts the path floor at R3 is these two files, 40 lines") and a "this grade is wrong" checkbox whose state round-trips into a `risk-grade-disputed` label (distinct from the human-owned `risk-dispute`, which the grader still never touches); `check_run: true` publishes the tier and reason as a Check Run on the head commit — the immutable, timestamped, commit-attached record a mutable label cannot be — from a SEPARATE job, so it is the only surface that needs an extra `checks: write` grant in the caller's block, and only when switched on. Both surfaces are advisory in the same sense as the label: the Check Run's conclusion is hardcoded `neutral`, and every publish failure is an annotation, never a red check. Label text is remappable via `label_map`. `workflows_ref` is **required**, and both its **shape and its ancestry are enforced** — every job that checks it out fails the run *before* the tool checkout unless the value is a full 40-hex lowercase commit SHA **and** that commit is an ancestor of `main` of this repo (fetched from a literal upstream URL; no `github` context can name this repo from inside a reusable workflow, so a variable there would be an alias a fork could point at itself). Shape rejects everything mutable — a branch, a tag, a `refs/pull/N/head`. Ancestry rejects everything unmerged: a fork of this public repo shares its object store, and GitHub serves a fork PR's head objects from this repo's own URL unauthenticated, so a fork-authored SHA is just as well-shaped and would otherwise be checked out into a job holding the caller's write token. There is deliberately **no opt-out input** — an opt-out would be set by the very pin-bump PR the check distrusts. Two consequences by design: **pinning a not-yet-merged SHA is rejected** (merge first, then bump), and the axis **fails closed** if this repo ever goes private or `main` is force-rewritten past a consumer's pin (re-pin to a commit on `main` to recover). **What is still not machine-checked is that the pin is the *current* one**: a stale pin left behind when `uses:` moved is a merged ancestor too. The test that would close that is "equal to the commit `uses:` resolved to", and the runner does not expose it to the workflow (`github.workflow_sha` is the *caller's* top-level file; `job_workflow_sha` is an OIDC claim, not a `github` context property, so reading it would need `id-token: write` from every caller). The other residual is irreducible: a fork commit that edits `pr-risk.yml` itself is rejected by ancestry, but a caller pinned at one runs the fork's own copy of the guard, and no in-file check can bound a revision of that file an attacker chose. **Reviewing the caller is what bounds both, and it is the only thing that can: require `uses:` at a full commit SHA of this repo and `with: workflows_ref:` set to that same SHA written out literally, character-for-character — never an expression, never a tag.** The guard also runs *before* enablement is resolved (the resolver is itself loaded from `workflows_ref`), so a floating pin fails red even with the `RISK_CONFIG` kill switch set — the switch stops the grading, not a broken enrollment. Call the workflow directly: a nested `workflow_call` chain through an org wrapper is unsupported. Enroll it as its own workflow rather than a job inside an existing CI workflow (the rollup exclusion is per-run). The calling job needs `contents: read` + `issues: write` + `pull-requests: write` + `checks: read` + `actions: read` + `statuses: read`; GitHub rejects a shorter grant at startup (a reusable workflow can only narrow the caller's token, never elevate it), so a caller enrolled from an older copy of this row fails before any step runs. Both writes are the ONE label: repo-side label creation on first use maps to `issues`, and labeling a PR maps to `pull-requests` (the labels endpoint is dual-mapped by what the "issue" is, so `issues: write` alone 403s on a PR). `actions: read` is for the rollup's `CheckRun -> checkSuite -> workflowRun` self-exclusion hop. No secrets. | [pr-risk.md](docs/callers/pr-risk.md) | +| [`pr-risk.yml`](.github/workflows/pr-risk.yml) | **Advisory PR risk grading (shadow check)** — **automatic grading off by default** (`enabled: false`; a manual `workflow_dispatch` grades regardless, so a repo can trial it before switching on); switch it on with `enabled: true` or by setting the caller repo's `RISK_CONFIG` variable to `{"enabled": true}`, which outranks the input in both directions so `{"enabled": false}` is a no-PR kill switch. Grades every PR into a tier `R0` (safest) .. `R3` (riskiest) and syncs one label (`risk:R0`..`risk:R3`, or `risk:ungraded` when an input was unreadable). The label is the entire product: nothing is gated, routed, commented, or merged. Deterministic (`gh` + `jq`, no LLM): `grade = worst(path_floor, provenance, reversibility)` — path-glob map, what-process-produced-the-diff (registered runbooks with identity + diff-shape assertions; forks are R3 with no exceptions), and revertability (persistent-state mutation, deletions under sensitive classes, did green checks cover the lines). Grader + generic defaults live in [`scripts/pr-risk/`](scripts/pr-risk); a consumer sharpens them with `.github/risk.json` / `.github/risk-runbooks.json`, read from the PR's **base ref** so a PR can't edit the rules that judge it. The job excludes its own run from the check rollup and waits (`wait_for_checks_minutes`) for the rest to settle before labeling. Labels ride the plain `GITHUB_TOKEN` (cannot fire `labeled` triggers — no cascade risk); disagreement is recorded with a human-owned `risk-dispute` label. **Two further publish surfaces are available and are OFF by default**, so an enrolled caller behaves byte-identically until it opts in: `sticky_comment: true` posts ONE comment (created once, updated in place — N pushes leave one comment) carrying the per-file path-axis breakdown, the risk CONCENTRATION sentence ("94% of this diff is R0/R1; the 6% that puts the path floor at R3 is these two files, 40 lines") and a "this grade is wrong" checkbox whose state round-trips into a `risk-grade-disputed` label (distinct from the human-owned `risk-dispute`, which the grader still never touches); `check_run: true` publishes the tier and reason as a Check Run on the head commit — the immutable, timestamped, commit-attached record a mutable label cannot be — from a SEPARATE job, the only one holding `checks: write` — a grant every caller makes whether or not it switches the surface on, because GitHub validates every nested job's declared `permissions:` at startup and a job `if:` is a runtime condition (the input decides whether a check is PUBLISHED, not whether the grant is CHECKED). Both surfaces are advisory in the same sense as the label: the Check Run's conclusion is hardcoded `neutral`, and every publish failure is an annotation, never a red check. Label text is remappable via `label_map`. `workflows_ref` is **required**, and both its **shape and its ancestry are enforced** — every job that checks it out fails the run *before* the tool checkout unless the value is a full 40-hex lowercase commit SHA **and** that commit is an ancestor of `main` of this repo (fetched from a literal upstream URL; no `github` context can name this repo from inside a reusable workflow, so a variable there would be an alias a fork could point at itself). Shape rejects everything mutable — a branch, a tag, a `refs/pull/N/head`. Ancestry rejects everything unmerged: a fork of this public repo shares its object store, and GitHub serves a fork PR's head objects from this repo's own URL unauthenticated, so a fork-authored SHA is just as well-shaped and would otherwise be checked out into a job holding the caller's write token. There is deliberately **no opt-out input** — an opt-out would be set by the very pin-bump PR the check distrusts. Two consequences by design: **pinning a not-yet-merged SHA is rejected** (merge first, then bump), and the axis **fails closed** if this repo ever goes private or `main` is force-rewritten past a consumer's pin (re-pin to a commit on `main` to recover). **What is still not machine-checked is that the pin is the *current* one**: a stale pin left behind when `uses:` moved is a merged ancestor too. The test that would close that is "equal to the commit `uses:` resolved to", and the runner does not expose it to the workflow (`github.workflow_sha` is the *caller's* top-level file; `job_workflow_sha` is an OIDC claim, not a `github` context property, so reading it would need `id-token: write` from every caller). The other residual is irreducible: a fork commit that edits `pr-risk.yml` itself is rejected by ancestry, but a caller pinned at one runs the fork's own copy of the guard, and no in-file check can bound a revision of that file an attacker chose. **Reviewing the caller is what bounds both, and it is the only thing that can: require `uses:` at a full commit SHA of this repo and `with: workflows_ref:` set to that same SHA written out literally, character-for-character — never an expression, never a tag.** The guard also runs *before* enablement is resolved (the resolver is itself loaded from `workflows_ref`), so a floating pin fails red even with the `RISK_CONFIG` kill switch set — the switch stops the grading, not a broken enrollment. Call the workflow directly: a nested `workflow_call` chain through an org wrapper is unsupported. Enroll it as its own workflow rather than a job inside an existing CI workflow (the rollup exclusion is per-run). The calling job needs `contents: read` + `issues: write` + `pull-requests: write` + `checks: write` + `actions: read` + `statuses: read`; GitHub rejects a shorter grant at startup (a reusable workflow can only narrow the caller's token, never elevate it), so a caller enrolled from an older copy of this row fails before any step runs. The two label writes are the ONE label: repo-side label creation on first use maps to `issues`, and labeling a PR maps to `pull-requests` (the labels endpoint is dual-mapped by what the "issue" is, so `issues: write` alone 403s on a PR). `checks: write` is the `publish-check` job's declaration, required unconditionally per above — grading itself uses only `checks: read`. `actions: read` is for the rollup's `CheckRun -> checkSuite -> workflowRun` self-exclusion hop. No secrets. | [pr-risk.md](docs/callers/pr-risk.md) | | [`stale.yml`](.github/workflows/stale.yml) | Stale-PR sweeper (`actions/stale`) plus a Slack digest of what it touched. PRs inactive for N days are labeled `stale`; still-inactive PRs are closed. The digest header names the source repo so batches from different repos posted to the same channel are unambiguous. Thresholds, messages, exempt labels, and the Slack channel are inputs; the caller owns the schedule + dry-run toggle. The calling job needs `pull-requests: write` and `issues: write`. Optional `SLACK_BOT_TOKEN`. | [stale.md](docs/callers/stale.md) | | [`groom.yml`](.github/workflows/groom.yml) | Scheduled/dispatch org-wide **code-cleanup sweep** (finds only — no commits, no PRs, never merges). A read-only FINDER agent scans a clean default-branch checkout (whole-repo, not a diff) for high-value refactors; an INDEPENDENT VERIFIER agent (fresh session) re-checks each as CONFIRM/DOWNGRADE/REJECT with a stable dedup signature; survivors are deduped against a durable GitHub-issue-state ledger and filed as `groom`-labeled GitHub issues (security-adjacent ones get `groom-security` — investigate, don't auto-implement). Mirrors the cursor-review topology: briefs + ledger live in [`.github/groom/`](.github/groom) as the single source of truth. The finder/verifier/builder agent jobs invoke the Claude CLI directly and mint no GitHub token, so they need nothing beyond `contents: read`; filing runs in a separate job as the bot you configure via `bot_app_id` (Comfy: cloud-code-bot). `dry_run` reports what it would file without opening issues. Runs on a **daily base cron** with a runtime cadence gate: set repo Actions variable `GROOM_INTERVAL_DAYS` (default 7 = weekly) to retune how often a real run happens — weekly → every-3-days → daily — with no workflow-file edit; a tick within the interval no-ops before the finder (`workflow_dispatch` bypasses the interval gate, but the volume gate — when the caller leaves it on — still applies). The calling job must grant `contents: read` + `issues: write` + `pull-requests: read` + `actions: read` — the first three are declared by the `file` / `build_select` jobs (needed even with `bot_app_id` set), and the interval gate needs `actions: read` (reads run history for the last real run); GitHub rejects a shorter grant at startup. Requires `ANTHROPIC_API_KEY` (+ `BOT_APP_PRIVATE_KEY` when `bot_app_id` is set). **Opt-in auto-builder** (`builder: true`, BE-4003): the top `max_prs` (default 5) CONFIRMED, non-security findings become **review-gated PRs** (full CI + cursor-review, **never auto-merged**) instead of issues; a credential-free `build` job emits only a patch artifact and a separate `build_pr` job opens the PR as the bot, preserving the security boundary. The ledger's PR-state (open/merged/closed) stops a built finding being re-proposed. Requires `bot_app_id`. `max_prs` is typed **`string`**, not `number`, so a caller can forward its own `workflow_dispatch` input straight through (`max_prs: ${{ github.event.inputs.max_prs \|\| '1' }}`) and let an operator raise the ceiling for one manual run — no `fromJSON()` cast in the caller, and the parse/clamp (empty → default, non-numeric → 0 PRs + warning, never a failed run) happens once inside the reusable. A build that cannot become a PR (patch over `pr_size_limit`, patch touching CI-privileged paths) **bails** to a `groom` issue so the paid-for work isn't lost — that path lives in `build_pr`, so **`max_findings` does not cap it** and `max_findings: 0` alone does not silence it; set `bail_sink: none` (an operational knob, so `GROOM_CONFIG` can set it with no PR) to file nothing and get a run-log warning + summary line instead. | [groom.md](docs/callers/groom.md) | | [`agents-md-integrity.yml`](.github/workflows/agents-md-integrity.yml) | Enforces the Comfy `AGENTS.md` standard on the caller repo: a top-level `AGENTS.md` must exist and stay under a hard line ceiling (`max_lines`, default 200; warns over `warn_lines`, default 150), a `CLAUDE.md` (if present) must be a thin `@AGENTS.md` shim rather than a divergent copy, no legacy `.cursorrules` (gated `forbid_cursorrules`), every nested monorepo `AGENTS.md` needs a sibling `@AGENTS.md` shim and to be under the ceiling (gated `check_nested`), and `AGENTS.md` should have a CODEOWNERS DRI (`require_codeowners`, warn-only by default). `exclude_paths` (newline-/comma-separated globs, default empty) carves payload subtrees — a repo whose product IS agent instructions, e.g. a plugin marketplace shipping `plugins/**/AGENTS.md` + a real `CLAUDE.md` — out of the nested scan without the all-or-nothing `check_nested: false`; exclusions are applied during the walk (never scanned or line-counted), reported in the log as `EXCLUDED: (matched )`, and a glob that would exclude the ROOT `AGENTS.md`/`CLAUDE.md` — or the whole tree without saying so (`/`, `*`, `*/**`) — is rejected (exit 2). Fails with a non-zero exit + GitHub annotations so it wires in as a required status check. The checker lives in [`.github/agents-md-integrity/`](.github/agents-md-integrity) (pin `workflows_ref` to the same ref as `uses:`); no secrets required. | [agents-md-integrity.md](docs/callers/agents-md-integrity.md) | diff --git a/docs/callers/pr-risk.md b/docs/callers/pr-risk.md index 7c7259f..aa55d86 100644 --- a/docs/callers/pr-risk.md +++ b/docs/callers/pr-risk.md @@ -50,9 +50,13 @@ jobs: pr-risk: permissions: contents: read - issues: write # the risk label rides the issues API - pull-requests: read - checks: read # the check rollup the reversibility axis reads + issues: write # create the risk:* labels repo-side on first use + pull-requests: write # the label write itself — labeling a PR rides the + # pull-requests permission, not issues (the labels + # endpoint is dual-mapped by what the "issue" is) + checks: write # REQUIRED WHETHER OR NOT you set `check_run: true` — + # see "Grant the whole union" below + actions: read # the rollup's CheckRun -> checkSuite -> workflowRun hop statuses: read uses: Comfy-Org/github-workflows/.github/workflows/pr-risk.yml@ with: @@ -68,11 +72,24 @@ This workflow has no roster yet — it has no fleet of pinned callers to track ```yaml contents: read issues: write -pull-requests: read -checks: read +pull-requests: write +checks: write +actions: read statuses: read ``` +**Grant the whole union, including `checks: write`.** A reusable workflow can +only narrow the caller's token, never elevate it, so GitHub validates *every* +nested job's declared `permissions:` against this block at **startup** — before +any job is scheduled. The `publish-check` job declares `checks: write`, and a +job-level `if:` is a runtime condition, so leaving `check_run` at its default +`false` does not exempt you: a short grant fails the whole run with an opaque +"workflow file issue" and no job-level detail. Grading itself only needs +`checks: read` (the rollup the reversibility axis reads); the write is the +grant, not the behaviour. **Moving an existing pin onto a commit that has this +job? Add `checks: write` to the caller in the same PR** — a pin bump alone will +fail the caller's next run at startup. + ## Inputs | Input | Default | Notes |