From 8024f692216f18f002121c83a43239cd3f8a377a Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Tue, 4 Aug 2026 02:51:54 -0700 Subject: [PATCH 1/8] fix(pr-risk): enforce workflows_ref as a full 40-hex commit SHA before the tool checkout (BE-6307) workflows_ref supplies the code that runs in the gate and grade jobs, and the grade job holds a GH_TOKEN carrying pull-requests: write. The "pin it to the same full SHA as uses:" contract was prose only, so a caller could pass main, a tag, or refs/pull/N/head (fork-authored, mutable) and the run succeeded. Both jobs that check out workflows_ref now reject anything but a full 40-hex commit SHA, before the checkout. Enforced in the workflow rather than in a script: the scripts are what the ref loads, so a script-side check would sit inside the blast radius it is meant to bound. --- .github/workflows/pr-risk.yml | 54 ++++++++++++++++++++++++++++++++++- README.md | 2 +- 2 files changed, 54 insertions(+), 2 deletions(-) diff --git a/.github/workflows/pr-risk.yml b/.github/workflows/pr-risk.yml index e80a3f7..949088f 100644 --- a/.github/workflows/pr-risk.yml +++ b/.github/workflows/pr-risk.yml @@ -14,7 +14,11 @@ name: PR Risk Grade (reusable) # # The grader and its default risk map load from THIS repo at the pinned `workflows_ref`, # never from the graded PR's checkout (no PR code is checked out at all) — a PR cannot edit -# the rules that judge it. A consumer repo sharpens the generic defaults by committing +# the rules that judge it. `workflows_ref` has deliberately no default and is now ENFORCED as a +# full 40-hex commit SHA: every job that checks it out fails before the checkout on anything +# mutable (a branch, a tag, `refs/pull/N/head`), so the "pin it to the same SHA as `uses:`" +# contract is machine-checked rather than trusted prose in this header. A consumer repo sharpens +# the generic defaults by committing # `.github/risk.json` (map) / `.github/risk-runbooks.json` (runbook registry), which are read # from the PR's BASE ref: present-but-invalid fails the run loudly; absent falls back to the # defaults in scripts/pr-risk/. @@ -151,6 +155,9 @@ name: PR Risk Grade (reusable) # statuses: read # uses: Comfy-Org/github-workflows/.github/workflows/pr-risk.yml@ # v1 # with: +# # ENFORCED, not merely asked for: a full 40-hex commit SHA, or the run fails before the +# # tool checkout. A branch, a tag, or `refs/pull/N/head` is rejected — see the paragraph +# # about the pinned ref at the top of this header for why. # workflows_ref: # # OFF BY DEFAULT. Enrolling and switching on are two decisions: land the caller, get # # it reviewed, then start grading. Either pin it on here, or leave this out and set @@ -266,6 +273,12 @@ on: so the two halves of one tool drifted apart and the grading logic stayed mutable after review. A supply chain with a floating link in it is not a chain. + + + ENFORCED: the run fails before the tool checkout unless this matches + `^[0-9a-f]{40}$`. Branches, tags and `refs/pull/N/head` are mutable — + and a PR-head ref of this PUBLIC repo resolves fork-authored code — so + they are rejected rather than documented against. type: string required: true enabled: @@ -322,6 +335,31 @@ jobs: outputs: enabled: ${{ steps.resolve.outputs.enabled }} steps: + # THE PIN CONTRACT IS ENFORCED, NOT DOCUMENTED. The ref below supplies the code that runs + # in this job — a mutable ref (branch, tag, `refs/pull/N/head`) means that code can change + # after the caller was reviewed, and PR-head refs of this PUBLIC repo resolve fork-authored + # code. Only a full commit SHA is immutable, so anything else fails here, before the + # checkout. Enforced in the WORKFLOW, not in a script: the scripts are what the ref loads, + # so a script-side check would sit inside the blast radius it is meant to bound. EVERY job + # that checks out `workflows_ref` re-asserts this itself — `grade` does not inherit its + # safety from this job's step, so neither one can be made unsafe by a later `if:` or a + # re-ordering of the graph. This is also the FIRST checkout in the run, so a bad pin is + # reported before any of this repo's code has executed anywhere. + # + # The value arrives via `env:` and is never interpolated into the script body — inline + # `${{ }}` of the very input being validated is a shell-injection vector. `[[ =~ ]]` rather + # than `grep -Eq '^[0-9a-f]{40}$'` because grep anchors per LINE: a multi-line value + # carrying one SHA-shaped line would pass it and then be handed to checkout in full. + # The two copies of this step are byte-identical on purpose; keep them that way. + - name: Enforce workflows_ref pin contract + env: + WORKFLOWS_REF: ${{ inputs.workflows_ref }} + run: | + set -euo pipefail + if [[ ! "$WORKFLOWS_REF" =~ ^[0-9a-f]{40}$ ]]; then + echo "::error::workflows_ref must be the FULL 40-hex commit SHA of Comfy-Org/github-workflows (got '${WORKFLOWS_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 # 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 @@ -406,6 +444,20 @@ jobs: BASE_REF: ${{ (inputs.pr_number || inputs.pr_numbers) == '' && github.event.pull_request.base.ref || '' }} GH_TOKEN: ${{ github.token }} steps: + # The same guard the `gate` job applies, restated here rather than inherited from it. This + # job is where the stakes are: the checked-out code runs holding the job's `GH_TOKEN`, which + # carries `pull-requests: write`, and public repos enroll via `pull_request_target` so that + # token is present on fork-authored events. A job must not depend on ANOTHER job's step for + # its own trust boundary — see the `gate` job above for the full rationale. + - name: Enforce workflows_ref pin contract + env: + WORKFLOWS_REF: ${{ inputs.workflows_ref }} + run: | + set -euo pipefail + if [[ ! "$WORKFLOWS_REF" =~ ^[0-9a-f]{40}$ ]]; then + echo "::error::workflows_ref must be the FULL 40-hex commit SHA of Comfy-Org/github-workflows (got '${WORKFLOWS_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 - 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 2e603df..9ec2d8a 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. Label text is remappable via `label_map`. `workflows_ref` is **required** — pin it to the same full commit SHA as `uses:`, so the grader cannot be loaded from a floating ref after the caller was reviewed. 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. Label text is remappable via `label_map`. `workflows_ref` is **required** and its format is **ENFORCED** — pin it to the same full commit SHA as `uses:`, so the grader cannot be loaded from a floating ref after the caller was reviewed; every job that checks it out fails the run *before* the tool checkout unless it is a full 40-hex commit SHA, so a branch, a tag, or a `refs/pull/N/head` is rejected rather than trusted. 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. | [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) | From 48eb512200131f68e0dc8b1a9d5c6ed558edd13d Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Tue, 4 Aug 2026 03:20:03 -0700 Subject: [PATCH 2/8] fix(pr-risk): tie workflows_ref to the SHA `uses:` resolved to, not just to SHA shape (BE-6307) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The shape check alone did not deliver the guarantee its own comment claimed. GitHub serves a fork PR's head objects from the upstream repo, so a fork-authored commit of this PUBLIC repo has a perfectly well-formed 40-hex SHA: `refs/pull/N/head` was rejected while the identical commit, spelled as a SHA, was checked out into a job holding the caller's `pull-requests: write` token. A stale pin left behind when `uses:` moved passed for the same reason. Both guard copies now also require the value to equal `github.job_workflow_sha` — the commit `uses:` actually resolved to for that job, supplied by the runner and unforgeable by any input. That is what turns "pin it to the same SHA as `uses:`" from prose into a check. An empty context warns and falls back to the shape test rather than red-checking every consumer on a platform regression; it is always populated for a `workflow_call` job and this workflow has no other trigger. Also from the review panel: - accept a mixed-case SHA (`[0-9a-fA-F]`, case-insensitive compare). Git parses object IDs case-insensitively, so rejecting one told a caller to pin the SHA it had already pinned. - never echo the raw value into a `::error::`. A multi-line value — precisely what the `[[ =~ ]]` choice exists to catch — ended the annotation at the first newline and left the rest to be re-parsed as workflow commands (`::stop-commands::`, a forged `::notice::`) in a public log. Non-ref characters become `?` and the result is truncated to one bounded line. - add scripts/pr-risk/tests/test_pin_contract.sh. The invariants "every job that checks out the ref re-asserts this itself" and "the two copies are byte-identical" were held up by a comment and hand-copying, with nothing to fail if either broke. Structural, over the workflow text, because the guard cannot live in a script: a script-side check would sit inside the blast radius it exists to bound. --- .github/workflows/pr-risk.yml | 118 ++++++++++++++++----- .github/workflows/test-pr-risk.yml | 10 +- README.md | 2 +- scripts/pr-risk/tests/test_pin_contract.sh | 109 +++++++++++++++++++ 4 files changed, 211 insertions(+), 28 deletions(-) create mode 100644 scripts/pr-risk/tests/test_pin_contract.sh diff --git a/.github/workflows/pr-risk.yml b/.github/workflows/pr-risk.yml index 949088f..dcb1782 100644 --- a/.github/workflows/pr-risk.yml +++ b/.github/workflows/pr-risk.yml @@ -14,10 +14,13 @@ name: PR Risk Grade (reusable) # # The grader and its default risk map load from THIS repo at the pinned `workflows_ref`, # never from the graded PR's checkout (no PR code is checked out at all) — a PR cannot edit -# the rules that judge it. `workflows_ref` has deliberately no default and is now ENFORCED as a -# full 40-hex commit SHA: every job that checks it out fails before the checkout on anything -# mutable (a branch, a tag, `refs/pull/N/head`), so the "pin it to the same SHA as `uses:`" -# contract is machine-checked rather than trusted prose in this header. A consumer repo sharpens +# the rules that judge it. `workflows_ref` has deliberately no default and is now ENFORCED: every +# job that checks it out fails BEFORE the checkout unless the value is a full 40-hex commit SHA +# *and* equals `github.job_workflow_sha` — the commit `uses:` actually resolved to. Anything +# mutable (a branch, a tag, `refs/pull/N/head`) is rejected by the first test; a well-shaped SHA +# that is not the running revision — a fork-authored commit, or a stale pin left behind when +# `uses:` moved — is rejected by the second. The "pin it to the same SHA as `uses:`" contract is +# machine-checked rather than trusted prose in this header. A consumer repo sharpens # the generic defaults by committing # `.github/risk.json` (map) / `.github/risk-runbooks.json` (runbook registry), which are read # from the PR's BASE ref: present-but-invalid fails the run loudly; absent falls back to the @@ -155,9 +158,10 @@ name: PR Risk Grade (reusable) # statuses: read # uses: Comfy-Org/github-workflows/.github/workflows/pr-risk.yml@ # v1 # with: -# # ENFORCED, not merely asked for: a full 40-hex commit SHA, or the run fails before the -# # tool checkout. A branch, a tag, or `refs/pull/N/head` is rejected — see the paragraph -# # about the pinned ref at the top of this header for why. +# # ENFORCED, not merely asked for: it must be a full 40-hex commit SHA *and* the same +# # commit the `uses:` above resolves to, or the run fails before the tool checkout. A +# # branch, a tag, `refs/pull/N/head`, or a SHA left behind when `uses:` moved is +# # rejected — see the paragraph about the pinned ref at the top of this header for why. # workflows_ref: # # OFF BY DEFAULT. Enrolling and switching on are two decisions: land the caller, get # # it reviewed, then start grading. Either pin it on here, or leave this out and set @@ -275,10 +279,13 @@ on: is not a chain. - ENFORCED: the run fails before the tool checkout unless this matches - `^[0-9a-f]{40}$`. Branches, tags and `refs/pull/N/head` are mutable — - and a PR-head ref of this PUBLIC repo resolves fork-authored code — so - they are rejected rather than documented against. + ENFORCED, on two axes, before the tool checkout: it must match + `^[0-9a-fA-F]{40}$` (branches, tags and `refs/pull/N/head` are mutable, + so they are rejected rather than documented against) AND equal + `github.job_workflow_sha`, the commit `uses:` resolved to. The second + test is what makes it THIS repo's reviewed code: a PR-head ref of this + PUBLIC repo resolves fork-authored code whose SHA is just as + well-shaped as any other. type: string required: true enabled: @@ -336,28 +343,61 @@ jobs: enabled: ${{ steps.resolve.outputs.enabled }} steps: # THE PIN CONTRACT IS ENFORCED, NOT DOCUMENTED. The ref below supplies the code that runs - # in this job — a mutable ref (branch, tag, `refs/pull/N/head`) means that code can change - # after the caller was reviewed, and PR-head refs of this PUBLIC repo resolve fork-authored - # code. Only a full commit SHA is immutable, so anything else fails here, before the - # checkout. Enforced in the WORKFLOW, not in a script: the scripts are what the ref loads, - # so a script-side check would sit inside the blast radius it is meant to bound. EVERY job - # that checks out `workflows_ref` re-asserts this itself — `grade` does not inherit its - # safety from this job's step, so neither one can be made unsafe by a later `if:` or a - # re-ordering of the graph. This is also the FIRST checkout in the run, so a bad pin is - # reported before any of this repo's code has executed anywhere. + # in this job, so it is checked on TWO axes before any checkout happens: + # 1. SHAPE — a mutable ref (branch, tag, `refs/pull/N/head`) means that code can change + # after the caller was reviewed. Only a full commit SHA is immutable. + # 2. IDENTITY — shape alone does not say whose commit it is. GitHub serves a fork PR's + # head objects from this upstream repo, so a fork-authored 40-hex SHA is a perfectly + # well-shaped ref. So the value must EQUAL `github.job_workflow_sha`, the commit + # `uses:` resolved to for this job — runner-supplied and unforgeable by any input. + # That is what turns "pin it to the same SHA as `uses:`" from prose into a check. + # Enforced in the WORKFLOW, not in a script: the scripts are what the ref loads, so a + # script-side check would sit inside the blast radius it is meant to bound. EVERY job that + # checks out `workflows_ref` re-asserts this itself — `grade` does not inherit its safety + # from this job's step, so neither one can be made unsafe by a later `if:` or a re-ordering + # of the graph. This is also the FIRST checkout in the run, so a bad pin is reported before + # any of this repo's code has executed anywhere. # # The value arrives via `env:` and is never interpolated into the script body — inline # `${{ }}` of the very input being validated is a shell-injection vector. `[[ =~ ]]` rather - # than `grep -Eq '^[0-9a-f]{40}$'` because grep anchors per LINE: a multi-line value + # than `grep -Eq '^[0-9a-fA-F]{40}$'` because grep anchors per LINE: a multi-line value # carrying one SHA-shaped line would pass it and then be handed to checkout in full. - # The two copies of this step are byte-identical on purpose; keep them that way. + # The two copies of this step are byte-identical on purpose, and + # scripts/pr-risk/tests/test_pin_contract.sh fails the build if they drift apart or if a + # job grows a `workflows_ref` checkout without one. - name: Enforce workflows_ref pin contract env: WORKFLOWS_REF: ${{ inputs.workflows_ref }} + # The commit `uses:` actually resolved to for THIS job. The runner sets it from the + # call graph; no input, caller expression, or fork PR can forge it — which is what + # makes the lock-step comparison below a real check rather than a restatement of the + # same untrusted value. + JOB_WORKFLOW_SHA: ${{ github.job_workflow_sha }} run: | set -euo pipefail - if [[ ! "$WORKFLOWS_REF" =~ ^[0-9a-f]{40}$ ]]; then - echo "::error::workflows_ref must be the FULL 40-hex commit SHA of Comfy-Org/github-workflows (got '${WORKFLOWS_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." + # Never interpolate the raw value into a `::error::`. A multi-line value — exactly what + # the `[[ =~ ]]` choice exists to catch — would end the annotation at the first newline + # and leave the remainder to be re-parsed by the runner as workflow commands + # (`::add-mask::`, `::stop-commands::`, a forged `::notice::`) in a PUBLIC log. Anything + # outside the ref alphabet becomes `?` and the result is truncated, so what is echoed is + # always one bounded line. + safe_ref=$(printf '%s' "$WORKFLOWS_REF" | tr -c 'A-Za-z0-9._/-' '?') + safe_ref=${safe_ref:0:64} + if [[ ! "$WORKFLOWS_REF" =~ ^[0-9a-fA-F]{40}$ ]]; then + echo "::error::workflows_ref must be the FULL 40-hex 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 + # Shape proves the ref is IMMUTABLE; it proves nothing about WHOSE commit it names. + # GitHub serves a fork PR's head objects from this upstream repo, so a fork-authored + # 40-hex SHA passes the regex and would be checked out into a job holding the caller's + # token. The pin only means "the reviewed code of this repo" if it is the very commit + # this job is already running from, so that is what is asserted. + if [[ -z "$JOB_WORKFLOW_SHA" ]]; then + echo "::warning::github.job_workflow_sha was empty, so workflows_ref could be checked for SHA shape only, not for lock-step with uses:. That context is always populated for a workflow_call job and this workflow has no other trigger — treat an empty value as a platform regression, not a supported mode." + exit 0 + fi + if [[ "${WORKFLOWS_REF,,}" != "${JOB_WORKFLOW_SHA,,}" ]]; then + echo "::error::workflows_ref (${safe_ref}) is not the commit uses: resolved to (${JOB_WORKFLOW_SHA}), so the grader would load from a different revision of Comfy-Org/github-workflows than the one reviewed alongside this caller. Set workflows_ref to the SAME SHA as the uses: line calling this workflow (a local './' call must pass that run's own commit SHA)." exit 1 fi # The tool checkout is the same pinned-ref load the grade job does: the resolver is this @@ -452,10 +492,36 @@ jobs: - name: Enforce workflows_ref pin contract env: WORKFLOWS_REF: ${{ inputs.workflows_ref }} + # The commit `uses:` actually resolved to for THIS job. The runner sets it from the + # call graph; no input, caller expression, or fork PR can forge it — which is what + # makes the lock-step comparison below a real check rather than a restatement of the + # same untrusted value. + JOB_WORKFLOW_SHA: ${{ github.job_workflow_sha }} run: | set -euo pipefail - if [[ ! "$WORKFLOWS_REF" =~ ^[0-9a-f]{40}$ ]]; then - echo "::error::workflows_ref must be the FULL 40-hex commit SHA of Comfy-Org/github-workflows (got '${WORKFLOWS_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." + # Never interpolate the raw value into a `::error::`. A multi-line value — exactly what + # the `[[ =~ ]]` choice exists to catch — would end the annotation at the first newline + # and leave the remainder to be re-parsed by the runner as workflow commands + # (`::add-mask::`, `::stop-commands::`, a forged `::notice::`) in a PUBLIC log. Anything + # outside the ref alphabet becomes `?` and the result is truncated, so what is echoed is + # always one bounded line. + safe_ref=$(printf '%s' "$WORKFLOWS_REF" | tr -c 'A-Za-z0-9._/-' '?') + safe_ref=${safe_ref:0:64} + if [[ ! "$WORKFLOWS_REF" =~ ^[0-9a-fA-F]{40}$ ]]; then + echo "::error::workflows_ref must be the FULL 40-hex 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 + # Shape proves the ref is IMMUTABLE; it proves nothing about WHOSE commit it names. + # GitHub serves a fork PR's head objects from this upstream repo, so a fork-authored + # 40-hex SHA passes the regex and would be checked out into a job holding the caller's + # token. The pin only means "the reviewed code of this repo" if it is the very commit + # this job is already running from, so that is what is asserted. + if [[ -z "$JOB_WORKFLOW_SHA" ]]; then + echo "::warning::github.job_workflow_sha was empty, so workflows_ref could be checked for SHA shape only, not for lock-step with uses:. That context is always populated for a workflow_call job and this workflow has no other trigger — treat an empty value as a platform regression, not a supported mode." + exit 0 + fi + if [[ "${WORKFLOWS_REF,,}" != "${JOB_WORKFLOW_SHA,,}" ]]; then + echo "::error::workflows_ref (${safe_ref}) is not the commit uses: resolved to (${JOB_WORKFLOW_SHA}), so the grader would load from a different revision of Comfy-Org/github-workflows than the one reviewed alongside this caller. Set workflows_ref to the SAME SHA as the uses: line calling this workflow (a local './' call must pass that run's own commit SHA)." exit 1 fi - name: Load pr-risk tool diff --git a/.github/workflows/test-pr-risk.yml b/.github/workflows/test-pr-risk.yml index 876fc16..c76f48c 100644 --- a/.github/workflows/test-pr-risk.yml +++ b/.github/workflows/test-pr-risk.yml @@ -48,7 +48,7 @@ jobs: persist-credentials: false - name: shellcheck - run: shellcheck -x grade-pr-risk.sh apply-risk-label.sh grade-targets.sh resolve-enabled.sh tests/test_grade_pr_risk.sh tests/test_apply_risk_label.sh tests/test_grade_targets.sh tests/test_resolve_enabled.sh + run: shellcheck -x grade-pr-risk.sh apply-risk-label.sh grade-targets.sh resolve-enabled.sh tests/test_grade_pr_risk.sh tests/test_apply_risk_label.sh tests/test_grade_targets.sh tests/test_resolve_enabled.sh tests/test_pin_contract.sh - name: default map + registry parse and validate # The shipped defaults must pass the grader's own structural validation: @@ -69,6 +69,14 @@ jobs: # every call it receives is logged, so the suite asserts on which requests were made. run: bash tests/test_grade_targets.sh + - name: pin-contract suite + # The `workflows_ref` guard in pr-risk.yml itself — the trust boundary that decides which + # revision of this repo's grader runs, and the one piece of logic that CANNOT live in a + # script (a script-side check would sit inside the blast radius it bounds). Structural, + # over the workflow text: every job that checks out the ref is guarded, the hand-copied + # copies have not drifted, and neither enforcement axis has been dropped. + run: bash tests/test_pin_contract.sh + - name: enablement suite # The `enabled` switch: that `vars.RISK_CONFIG` outranks the reviewed input in BOTH # directions, and that a malformed variable degrades to the reviewed value rather than diff --git a/README.md b/README.md index 9ec2d8a..1dfac31 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. Label text is remappable via `label_map`. `workflows_ref` is **required** and its format is **ENFORCED** — pin it to the same full commit SHA as `uses:`, so the grader cannot be loaded from a floating ref after the caller was reviewed; every job that checks it out fails the run *before* the tool checkout unless it is a full 40-hex commit SHA, so a branch, a tag, or a `refs/pull/N/head` is rejected rather than trusted. 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. Label text is remappable via `label_map`. `workflows_ref` is **required** and **ENFORCED** — pin it to the same full commit SHA as `uses:`, so the grader cannot be loaded from a floating ref after the caller was reviewed. Every job that checks it out fails the run *before* the tool checkout unless the value is a full 40-hex commit SHA **and** equals `github.job_workflow_sha`, the commit `uses:` actually resolved to: a branch, a tag or a `refs/pull/N/head` is rejected by the first test, and a well-shaped SHA that is not the running revision — a fork-authored commit of this public repo, or a stale pin left behind when `uses:` moved — by the second. 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. | [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/scripts/pr-risk/tests/test_pin_contract.sh b/scripts/pr-risk/tests/test_pin_contract.sh new file mode 100644 index 0000000..0177cb1 --- /dev/null +++ b/scripts/pr-risk/tests/test_pin_contract.sh @@ -0,0 +1,109 @@ +#!/usr/bin/env bash +# Almost every literal below is a fragment of the YAML under inspection, so `${{ }}` and +# `${VAR}` are the text being tested and must NOT expand. File-wide, hence up here. +# shellcheck disable=SC2016 +# test_pin_contract.sh — hermetic structural tests for the `workflows_ref` pin guard in +# .github/workflows/pr-risk.yml. No network, no Actions: this reads the workflow file as text. +# +# The guard is the trust boundary of the whole workflow — it is what stops the grader being +# loaded from a mutable ref, or from a fork-authored commit of this PUBLIC repo, into a job +# holding the caller's `pull-requests: write` token. It lives in the WORKFLOW rather than in a +# script (a script-side check would sit inside the blast radius it bounds), which means the +# normal script suites cannot cover it. Nothing else in CI would notice these regressions: +# +# * A NEW JOB CHECKS OUT `workflows_ref` WITHOUT THE GUARD. The invariant "every job that +# checks it out re-asserts this itself" is stated in a comment and held up by hand-copying. +# A job added later that skips the guard silently loses the boundary for that job. +# * THE COPIES DRIFT. They are byte-identical on purpose; one-character drift between them +# (a loosened regex, a dropped `exit 1`) would leave one job weaker than the other with no +# visible symptom. +# * THE GUARD STOPS BEING FIRST. It only bounds what it precedes — a guard after the checkout +# it protects is decoration. +# * AN AXIS IS DROPPED. Shape alone proves the ref is immutable, not whose commit it is; the +# `github.job_workflow_sha` comparison is what proves it is this repo's reviewed code. +# +# bash tests/test_pin_contract.sh # exit 0 = all green +set -uo pipefail + +SELF_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +WF="$SELF_DIR/../../../.github/workflows/pr-risk.yml" +[ -f "$WF" ] || { echo "FATAL: $WF not found" >&2; exit 1; } + +PASS=0; FAIL=0 +ok() { PASS=$((PASS+1)); printf 'ok %s\n' "$1"; } +bad() { FAIL=$((FAIL+1)); printf 'FAIL %s\n got: %s\n' "$1" "${2:-}"; } +eq() { if [ "$2" = "$3" ]; then ok "$1"; else bad "$1 (expected '$2')" "$3"; fi } + +GUARD_NAME=' - name: Enforce workflows_ref pin contract' +CHECKOUT_REF=' ref: ${{ inputs.workflows_ref }}' + +# --- every `workflows_ref` checkout is preceded, in its own job, by the guard ----------------- +# Job boundaries are the 2-space-indented `:` keys under `jobs:`; the guard flag resets at +# each one, so a guard in `gate` cannot vouch for a checkout in `grade`. +unguarded="$(awk -v guard="$GUARD_NAME" -v ref="$CHECKOUT_REF" ' + /^jobs:$/ { injobs = 1; next } + injobs && /^ [A-Za-z0-9_-]+:[[:space:]]*$/ { job = $1; guarded = 0 } + injobs && $0 == guard { guarded = 1 } + injobs && $0 == ref { if (!guarded) print job } +' "$WF")" +eq "every workflows_ref checkout sits behind the guard, in its own job" "" "$unguarded" + +guards=$(grep -cxF "$GUARD_NAME" "$WF") +checkouts=$(grep -cxF "$CHECKOUT_REF" "$WF") +eq "one guard per workflows_ref checkout" "$checkouts" "$guards" +if [ "$guards" -ge 2 ]; then + ok "the guard is restated per job rather than centralized ($guards copies)" +else + bad "the guard is restated per job rather than centralized" "$guards copies" +fi + +# --- the copies have not drifted -------------------------------------------------------------- +# Slice each guard from its `- name:` line up to the next step OR the comment block introducing +# it (both live at 6-space indent; the guard's own body is indented deeper), then compare every +# copy against the first. +copies="$(mktemp -d "${TMPDIR:-/tmp}/pr-risk-pin.XXXXXX")" +trap 'rm -rf "$copies"' EXIT +awk -v guard="$GUARD_NAME" -v out="$copies" ' + $0 == guard { n += 1; f = out "/guard." n; inguard = 1; print > f; next } + inguard && /^ [-#]/ { inguard = 0 } + inguard { print > f } +' "$WF" +drift="" +first="$copies/guard.1" +for f in "$copies"/guard.*; do + cmp -s "$first" "$f" || drift="$drift $(basename "$f")" +done +eq "all copies of the guard step are byte-identical" "" "$drift" + +# --- both axes are still enforced, and enforced fatally --------------------------------------- +body="$(cat "$first")" +case "$body" in + *'^[0-9a-fA-F]{40}$'*) ok "axis 1: the ref must be shaped like a full 40-hex commit SHA" ;; + *) bad "axis 1: the ref must be shaped like a full 40-hex commit SHA" "regex missing//changed" ;; +esac +case "$body" in + *'JOB_WORKFLOW_SHA: ${{ github.job_workflow_sha }}'*) + ok "axis 2: the runner-supplied resolved SHA is read into the guard" ;; + *) bad "axis 2: the runner-supplied resolved SHA is read into the guard" "env var missing" ;; +esac +case "$body" in + *'"${WORKFLOWS_REF,,}" != "${JOB_WORKFLOW_SHA,,}"'*) + ok "axis 2: the pin is compared against it, case-insensitively" ;; + *) bad "axis 2: the pin is compared against it, case-insensitively" "comparison missing" ;; +esac +eq "each rejection is fatal (both axes exit 1)" "2" "$(printf '%s\n' "$body" | grep -c 'exit 1')" + +# --- the input itself is never interpolated into the shell or echoed raw ---------------------- +# `${{ inputs.workflows_ref }}` inline in `run:` would be a shell-injection vector, and echoing +# the raw value into a `::error::` lets a multi-line value forge workflow commands in a public log. +case "$body" in + *'run: |'*'${{'*) bad "the input reaches the script only via env:" "\${{ }} inside run:" ;; + *) ok "the input reaches the script only via env:" ;; +esac +case "$body" in + *'${WORKFLOWS_REF}'*) bad "only the sanitized value is echoed into annotations" "raw value echoed" ;; + *) ok "only the sanitized value is echoed into annotations" ;; +esac + +printf '\n%s passed, %s failed\n' "$PASS" "$FAIL" +[ "$FAIL" -eq 0 ] From dabdfe1792cd41b1611c57c49f3c164e4fccd333 Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Tue, 4 Aug 2026 03:37:21 -0700 Subject: [PATCH 3/8] fix(pr-risk): fail closed when job_workflow_sha is unreadable, and assert the guard by property (BE-6307) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Second review round, all of it fair. FAIL CLOSED. The empty-`job_workflow_sha` path warned and exited 0, so the lock-step axis failed open. An unrecognized `github` context property evaluates to the empty string, which means a rename or removal upstream would have turned the whole boundary into a silent permanent no-op with no red check anywhere — the one failure mode a guard like this must not have. Both copies now `exit 1` with an `::error::`. The blast radius I was protecting against is small (an advisory shadow check with a handful of callers); the one I was creating was not. NARROW THE CLAIM. `github.job_workflow_sha` is whatever the CALLER's own `uses:` resolved to, so a caller that points `uses:` itself at a fork commit is already running that fork's copy of this file and satisfies both axes. No check inside the called workflow can reach that; reviewing the caller on its base branch is what bounds it. What the axis does prove is that the two halves of the tool cannot disagree — a stale pin left behind when `uses:` moved, or a fork-authored SHA handed to `workflows_ref` while `uses:` stayed upstream. Header, input description, step comment and README row now say that and no more. TEST ASSERTS PROPERTIES, NOT COUNTS. The suite pinned a literal count of two `exit 1` lines, so it would have gone red on the fix above — a test that blocks its own subject's hardening teaches people to delete the test. It now asserts that no rejection path exits non-fatally (no `exit 0` at all) and that every `::error::` is paired with a failing exit, over a comment-stripped body. Also fixed there: - the raw-echo check matched only the braced `${WORKFLOWS_REF}`, missing the likelier unbraced regression it exists to catch; - checkout detection was a byte-exact line match, so a requoted, reindented or respaced `ref:` was invisible to both the coverage walk and the count; - `/^jobs:$/` and the job-key anchor were exact enough that a trailing space or comment made the primary assertion pass vacuously with zero coverage — the awk now reports what it matched and the suite fails if that is nothing; - the drift slicer only closed on a sibling step, so a guard that is the last step of a job absorbed the next job and reported spurious drift; - `mktemp -d` failure is checked before the `rm -rf` trap is installed. Mutation-checked: reverting the fail-open, echoing the unbraced value, dropping the lock-step compare, loosening one copy's regex, adding an unguarded checkout, and moving a guard after the checkout it protects each fail the suite; a respelled `${{inputs.workflows_ref}}`, a trailing comment on a job key, and a trailing space on `jobs:` no longer do. --- .github/workflows/pr-risk.yml | 69 +++++++---- README.md | 2 +- scripts/pr-risk/tests/test_pin_contract.sh | 134 +++++++++++++-------- 3 files changed, 136 insertions(+), 69 deletions(-) diff --git a/.github/workflows/pr-risk.yml b/.github/workflows/pr-risk.yml index dcb1782..846a3ef 100644 --- a/.github/workflows/pr-risk.yml +++ b/.github/workflows/pr-risk.yml @@ -16,12 +16,18 @@ name: PR Risk Grade (reusable) # never from the graded PR's checkout (no PR code is checked out at all) — a PR cannot edit # the rules that judge it. `workflows_ref` has deliberately no default and is now ENFORCED: every # job that checks it out fails BEFORE the checkout unless the value is a full 40-hex commit SHA -# *and* equals `github.job_workflow_sha` — the commit `uses:` actually resolved to. Anything -# mutable (a branch, a tag, `refs/pull/N/head`) is rejected by the first test; a well-shaped SHA -# that is not the running revision — a fork-authored commit, or a stale pin left behind when -# `uses:` moved — is rejected by the second. The "pin it to the same SHA as `uses:`" contract is -# machine-checked rather than trusted prose in this header. A consumer repo sharpens -# the generic defaults by committing +# *and* equals `github.job_workflow_sha` — the commit `uses:` resolved to for that job. The first +# test rejects anything mutable (a branch, a tag, `refs/pull/N/head`); the second rejects a +# well-shaped SHA that is not the revision already running — a stale pin left behind when `uses:` +# moved, or a fork-authored commit of this PUBLIC repo handed to `workflows_ref` while `uses:` +# stayed upstream. So the two halves of the tool cannot disagree, and "pin it to the same SHA as +# `uses:`" is machine-checked rather than trusted prose in this header. +# +# What that does NOT prove — and cannot, from in here — is that the running revision is upstream +# code: `job_workflow_sha` is whatever the CALLER's own `uses:` resolved to, so a caller pointing +# `uses:` itself at a fork commit is already running the fork's copy of this very file. That case +# is bounded by review of the caller on its base branch, not by any check this workflow can make. +# A consumer repo sharpens the generic defaults by committing # `.github/risk.json` (map) / `.github/risk-runbooks.json` (runbook registry), which are read # from the PR's BASE ref: present-but-invalid fails the run loudly; absent falls back to the # defaults in scripts/pr-risk/. @@ -283,9 +289,12 @@ on: `^[0-9a-fA-F]{40}$` (branches, tags and `refs/pull/N/head` are mutable, so they are rejected rather than documented against) AND equal `github.job_workflow_sha`, the commit `uses:` resolved to. The second - test is what makes it THIS repo's reviewed code: a PR-head ref of this - PUBLIC repo resolves fork-authored code whose SHA is just as - well-shaped as any other. + test is what stops the two halves of the tool disagreeing — a stale pin + left behind when `uses:` moved, or a fork-authored commit of this + PUBLIC repo passed here while `uses:` stayed upstream, both being just + as well-shaped as any other SHA. It does not, and cannot, prove the + running revision is upstream: that is what reviewing the caller's + `uses:` line on its base branch is for. type: string required: true enabled: @@ -346,11 +355,15 @@ jobs: # in this job, so it is checked on TWO axes before any checkout happens: # 1. SHAPE — a mutable ref (branch, tag, `refs/pull/N/head`) means that code can change # after the caller was reviewed. Only a full commit SHA is immutable. - # 2. IDENTITY — shape alone does not say whose commit it is. GitHub serves a fork PR's + # 2. LOCK-STEP — shape alone does not say WHICH commit it is. GitHub serves a fork PR's # head objects from this upstream repo, so a fork-authored 40-hex SHA is a perfectly # well-shaped ref. So the value must EQUAL `github.job_workflow_sha`, the commit # `uses:` resolved to for this job — runner-supplied and unforgeable by any input. - # That is what turns "pin it to the same SHA as `uses:`" from prose into a check. + # That is what turns "pin it to the same SHA as `uses:`" from prose into a check, and + # it is scoped to exactly that: the grader cannot come from a different revision than + # the one running. It does NOT prove the running revision is upstream — a caller whose + # own `uses:` points at a fork commit is already running that fork's copy of this file, + # which no check inside this file can reach. Reviewing the caller bounds that one. # Enforced in the WORKFLOW, not in a script: the scripts are what the ref loads, so a # script-side check would sit inside the blast radius it is meant to bound. EVERY job that # checks out `workflows_ref` re-asserts this itself — `grade` does not inherit its safety @@ -387,14 +400,21 @@ jobs: echo "::error::workflows_ref must be the FULL 40-hex 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 - # Shape proves the ref is IMMUTABLE; it proves nothing about WHOSE commit it names. + # Shape proves the ref is IMMUTABLE; it proves nothing about WHICH commit it names. # GitHub serves a fork PR's head objects from this upstream repo, so a fork-authored # 40-hex SHA passes the regex and would be checked out into a job holding the caller's - # token. The pin only means "the reviewed code of this repo" if it is the very commit - # this job is already running from, so that is what is asserted. + # token. The tool is only the revision that was reviewed if it is the very commit this + # job is already running from, so that is what is asserted. + # + # FAILS CLOSED. An unrecognized `github` context property evaluates to the empty string, + # so a rename or removal upstream would otherwise turn this whole boundary into a silent + # permanent no-op with no red check anywhere — the one failure mode a security guard + # must not have. The context is always populated for a `workflow_call` job and this + # workflow has no other trigger, so empty means the platform changed under us: stop, + # loudly, rather than check out a ref whose provenance cannot be established. if [[ -z "$JOB_WORKFLOW_SHA" ]]; then - echo "::warning::github.job_workflow_sha was empty, so workflows_ref could be checked for SHA shape only, not for lock-step with uses:. That context is always populated for a workflow_call job and this workflow has no other trigger — treat an empty value as a platform regression, not a supported mode." - exit 0 + echo "::error::github.job_workflow_sha is empty, so workflows_ref cannot be checked for lock-step with the commit uses: resolved to. That context is always populated for a workflow_call job and this workflow has no other trigger, so this is a platform regression (or a renamed context property), not a supported mode — failing closed rather than checking out a ref whose provenance cannot be established." + exit 1 fi if [[ "${WORKFLOWS_REF,,}" != "${JOB_WORKFLOW_SHA,,}" ]]; then echo "::error::workflows_ref (${safe_ref}) is not the commit uses: resolved to (${JOB_WORKFLOW_SHA}), so the grader would load from a different revision of Comfy-Org/github-workflows than the one reviewed alongside this caller. Set workflows_ref to the SAME SHA as the uses: line calling this workflow (a local './' call must pass that run's own commit SHA)." @@ -511,14 +531,21 @@ jobs: echo "::error::workflows_ref must be the FULL 40-hex 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 - # Shape proves the ref is IMMUTABLE; it proves nothing about WHOSE commit it names. + # Shape proves the ref is IMMUTABLE; it proves nothing about WHICH commit it names. # GitHub serves a fork PR's head objects from this upstream repo, so a fork-authored # 40-hex SHA passes the regex and would be checked out into a job holding the caller's - # token. The pin only means "the reviewed code of this repo" if it is the very commit - # this job is already running from, so that is what is asserted. + # token. The tool is only the revision that was reviewed if it is the very commit this + # job is already running from, so that is what is asserted. + # + # FAILS CLOSED. An unrecognized `github` context property evaluates to the empty string, + # so a rename or removal upstream would otherwise turn this whole boundary into a silent + # permanent no-op with no red check anywhere — the one failure mode a security guard + # must not have. The context is always populated for a `workflow_call` job and this + # workflow has no other trigger, so empty means the platform changed under us: stop, + # loudly, rather than check out a ref whose provenance cannot be established. if [[ -z "$JOB_WORKFLOW_SHA" ]]; then - echo "::warning::github.job_workflow_sha was empty, so workflows_ref could be checked for SHA shape only, not for lock-step with uses:. That context is always populated for a workflow_call job and this workflow has no other trigger — treat an empty value as a platform regression, not a supported mode." - exit 0 + echo "::error::github.job_workflow_sha is empty, so workflows_ref cannot be checked for lock-step with the commit uses: resolved to. That context is always populated for a workflow_call job and this workflow has no other trigger, so this is a platform regression (or a renamed context property), not a supported mode — failing closed rather than checking out a ref whose provenance cannot be established." + exit 1 fi if [[ "${WORKFLOWS_REF,,}" != "${JOB_WORKFLOW_SHA,,}" ]]; then echo "::error::workflows_ref (${safe_ref}) is not the commit uses: resolved to (${JOB_WORKFLOW_SHA}), so the grader would load from a different revision of Comfy-Org/github-workflows than the one reviewed alongside this caller. Set workflows_ref to the SAME SHA as the uses: line calling this workflow (a local './' call must pass that run's own commit SHA)." diff --git a/README.md b/README.md index 1dfac31..fa545ca 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. Label text is remappable via `label_map`. `workflows_ref` is **required** and **ENFORCED** — pin it to the same full commit SHA as `uses:`, so the grader cannot be loaded from a floating ref after the caller was reviewed. Every job that checks it out fails the run *before* the tool checkout unless the value is a full 40-hex commit SHA **and** equals `github.job_workflow_sha`, the commit `uses:` actually resolved to: a branch, a tag or a `refs/pull/N/head` is rejected by the first test, and a well-shaped SHA that is not the running revision — a fork-authored commit of this public repo, or a stale pin left behind when `uses:` moved — by the second. 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. Label text is remappable via `label_map`. `workflows_ref` is **required** and **ENFORCED** — pin it to the same full commit SHA as `uses:`, so the grader cannot be loaded from a floating ref after the caller was reviewed. Every job that checks it out fails the run *before* the tool checkout unless the value is a full 40-hex commit SHA **and** equals `github.job_workflow_sha`, the commit `uses:` resolved to: a branch, a tag or a `refs/pull/N/head` is rejected by the first test, and a well-shaped SHA that is not the revision already running — a stale pin left behind when `uses:` moved, or a fork-authored commit of this public repo handed to `workflows_ref` while `uses:` stayed upstream — by the second. Scoped to exactly that: it stops the two halves of the tool disagreeing, and does not (and from inside the called workflow cannot) prove the running revision is upstream — reviewing the caller's `uses:` line is what bounds that. 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. | [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/scripts/pr-risk/tests/test_pin_contract.sh b/scripts/pr-risk/tests/test_pin_contract.sh index 0177cb1..02fc5a1 100644 --- a/scripts/pr-risk/tests/test_pin_contract.sh +++ b/scripts/pr-risk/tests/test_pin_contract.sh @@ -6,10 +6,10 @@ # .github/workflows/pr-risk.yml. No network, no Actions: this reads the workflow file as text. # # The guard is the trust boundary of the whole workflow — it is what stops the grader being -# loaded from a mutable ref, or from a fork-authored commit of this PUBLIC repo, into a job -# holding the caller's `pull-requests: write` token. It lives in the WORKFLOW rather than in a -# script (a script-side check would sit inside the blast radius it bounds), which means the -# normal script suites cannot cover it. Nothing else in CI would notice these regressions: +# loaded from a mutable ref, or from a revision other than the one running, into a job holding +# the caller's `pull-requests: write` token. It lives in the WORKFLOW rather than in a script (a +# script-side check would sit inside the blast radius it bounds), which means the normal script +# suites cannot cover it. Nothing else in CI would notice these regressions: # # * A NEW JOB CHECKS OUT `workflows_ref` WITHOUT THE GUARD. The invariant "every job that # checks it out re-asserts this itself" is stated in a comment and held up by hand-copying. @@ -19,8 +19,16 @@ # visible symptom. # * THE GUARD STOPS BEING FIRST. It only bounds what it precedes — a guard after the checkout # it protects is decoration. -# * AN AXIS IS DROPPED. Shape alone proves the ref is immutable, not whose commit it is; the -# `github.job_workflow_sha` comparison is what proves it is this repo's reviewed code. +# * AN AXIS IS DROPPED, OR STOPS BEING FATAL. Shape alone proves the ref is immutable, not +# which commit it is; the `github.job_workflow_sha` comparison is what proves it is the +# revision already running. Either axis degraded to a warning is a silent no-op. +# +# ASSERT PROPERTIES, NOT COUNTS. An earlier draft pinned a literal number of `exit 1` lines, +# which would have gone red on the very next hardening of the guard — a test that blocks its own +# subject's improvement teaches people to delete the test. What is asserted here instead is that +# no rejection path can be non-fatal (no `exit 0` at all) and that every `::error::` is paired +# with an exit. The awk passes also self-check that they matched anything, so a brittle anchor +# fails loudly rather than passing vacuously with zero coverage. # # bash tests/test_pin_contract.sh # exit 0 = all green set -uo pipefail @@ -33,24 +41,46 @@ PASS=0; FAIL=0 ok() { PASS=$((PASS+1)); printf 'ok %s\n' "$1"; } bad() { FAIL=$((FAIL+1)); printf 'FAIL %s\n got: %s\n' "$1" "${2:-}"; } eq() { if [ "$2" = "$3" ]; then ok "$1"; else bad "$1 (expected '$2')" "$3"; fi } +has() { case "$2" in *"$3"*) ok "$1" ;; *) bad "$1" "not found: $3" ;; esac } +no() { case "$2" in *"$3"*) bad "$1" "present: $3" ;; *) ok "$1" ;; esac } GUARD_NAME=' - name: Enforce workflows_ref pin contract' -CHECKOUT_REF=' ref: ${{ inputs.workflows_ref }}' # --- every `workflows_ref` checkout is preceded, in its own job, by the guard ----------------- # Job boundaries are the 2-space-indented `:` keys under `jobs:`; the guard flag resets at -# each one, so a guard in `gate` cannot vouch for a checkout in `grade`. -unguarded="$(awk -v guard="$GUARD_NAME" -v ref="$CHECKOUT_REF" ' - /^jobs:$/ { injobs = 1; next } - injobs && /^ [A-Za-z0-9_-]+:[[:space:]]*$/ { job = $1; guarded = 0 } - injobs && $0 == guard { guarded = 1 } - injobs && $0 == ref { if (!guarded) print job } +# each one, so a guard in `gate` cannot vouch for a checkout in `grade`. Checkout detection is +# deliberately NOT a byte-exact line match: a future job that quotes the expression, drops the +# inner spaces, indents differently, or adds a trailing comment must still be seen, or an +# unguarded checkout ships green and defeats the point of this file. Match on the whitespace- +# stripped form instead. The awk also reports how many jobs and how many refs it saw, so a +# pattern that silently matches nothing fails below rather than printing a vacuous `ok`. +scan="$(awk -v guard="$GUARD_NAME" ' + function squash(s) { gsub(/[[:space:]]/, "", s); return s } + /^jobs:[[:space:]]*(#.*)?$/ { injobs = 1; next } + !injobs { next } + /^ [A-Za-z0-9_-]+:[[:space:]]*(#.*)?$/ { job = $1; jobs += 1; guarded = 0; next } + $0 == guard { guarded = 1; next } + squash($0) ~ /^ref:\$\{\{inputs\.workflows_ref\}\}$/ { + refs += 1 + if (!guarded) print "UNGUARDED:" job + } + END { print "JOBS:" jobs+0; print "REFS:" refs+0 } ' "$WF")" -eq "every workflows_ref checkout sits behind the guard, in its own job" "" "$unguarded" + +eq "every workflows_ref checkout sits behind the guard, in its own job" \ + "" "$(printf '%s\n' "$scan" | grep '^UNGUARDED:' | tr '\n' ' ' | sed 's/ $//')" + +# The two coverage self-checks: without them the assertion above is `ok` when the anchors match +# nothing at all, which is the failure mode a structural test is most prone to. +njobs="$(printf '%s\n' "$scan" | sed -n 's/^JOBS://p')" +nrefs="$(printf '%s\n' "$scan" | sed -n 's/^REFS://p')" +if [ "${njobs:-0}" -ge 2 ]; then ok "the job scan matched the workflow's jobs ($njobs)" +else bad "the job scan matched the workflow's jobs" "$njobs — anchors are stale, coverage is vacuous"; fi +if [ "${nrefs:-0}" -ge 2 ]; then ok "the checkout scan matched the workflows_ref checkouts ($nrefs)" +else bad "the checkout scan matched the workflows_ref checkouts" "$nrefs — anchors are stale, coverage is vacuous"; fi guards=$(grep -cxF "$GUARD_NAME" "$WF") -checkouts=$(grep -cxF "$CHECKOUT_REF" "$WF") -eq "one guard per workflows_ref checkout" "$checkouts" "$guards" +eq "one guard per workflows_ref checkout" "$nrefs" "$guards" if [ "$guards" -ge 2 ]; then ok "the guard is restated per job rather than centralized ($guards copies)" else @@ -58,15 +88,19 @@ else fi # --- the copies have not drifted -------------------------------------------------------------- -# Slice each guard from its `- name:` line up to the next step OR the comment block introducing -# it (both live at 6-space indent; the guard's own body is indented deeper), then compare every -# copy against the first. -copies="$(mktemp -d "${TMPDIR:-/tmp}/pr-risk-pin.XXXXXX")" +# Slice each guard from its `- name:` line to the first following line that is NOT indented +# deeper than it — the next step, the comment block introducing one, or the next job's key. A +# terminator that only recognized sibling steps would run past the end of a job whose LAST step +# is the guard and swallow the following job. +copies="$(mktemp -d "${TMPDIR:-/tmp}/pr-risk-pin.XXXXXX")" || { echo "FATAL: mktemp failed" >&2; exit 1; } +[ -d "$copies" ] || { echo "FATAL: mktemp produced no directory" >&2; exit 1; } trap 'rm -rf "$copies"' EXIT awk -v guard="$GUARD_NAME" -v out="$copies" ' $0 == guard { n += 1; f = out "/guard." n; inguard = 1; print > f; next } - inguard && /^ [-#]/ { inguard = 0 } - inguard { print > f } + !inguard { next } + /^[[:space:]]*$/ { print > f; next } + /^ / { print > f; next } + { inguard = 0 } ' "$WF" drift="" first="$copies/guard.1" @@ -75,35 +109,41 @@ for f in "$copies"/guard.*; do done eq "all copies of the guard step are byte-identical" "" "$drift" -# --- both axes are still enforced, and enforced fatally --------------------------------------- +# --- both axes are still enforced, and no rejection path can be non-fatal --------------------- body="$(cat "$first")" -case "$body" in - *'^[0-9a-fA-F]{40}$'*) ok "axis 1: the ref must be shaped like a full 40-hex commit SHA" ;; - *) bad "axis 1: the ref must be shaped like a full 40-hex commit SHA" "regex missing//changed" ;; -esac -case "$body" in - *'JOB_WORKFLOW_SHA: ${{ github.job_workflow_sha }}'*) - ok "axis 2: the runner-supplied resolved SHA is read into the guard" ;; - *) bad "axis 2: the runner-supplied resolved SHA is read into the guard" "env var missing" ;; -esac -case "$body" in - *'"${WORKFLOWS_REF,,}" != "${JOB_WORKFLOW_SHA,,}"'*) - ok "axis 2: the pin is compared against it, case-insensitively" ;; - *) bad "axis 2: the pin is compared against it, case-insensitively" "comparison missing" ;; -esac -eq "each rejection is fatal (both axes exit 1)" "2" "$(printf '%s\n' "$body" | grep -c 'exit 1')" +# Comments are stripped before any assertion about control flow, so an `exit 1` or an `exit 0` +# quoted in prose neither satisfies nor breaks a check. +code="$(printf '%s\n' "$body" | sed 's/[[:space:]]*#.*$//')" + +has "axis 1: the ref must be shaped like a full 40-hex commit SHA" \ + "$code" '^[0-9a-fA-F]{40}$' +has "axis 2: the runner-supplied resolved SHA is read into the guard" \ + "$body" 'JOB_WORKFLOW_SHA: ${{ github.job_workflow_sha }}' +has "axis 2: the pin is compared against it, case-insensitively" \ + "$code" '"${WORKFLOWS_REF,,}" != "${JOB_WORKFLOW_SHA,,}"' +has "axis 2: an unreadable job_workflow_sha is itself a rejection (fail closed)" \ + "$code" '[[ -z "$JOB_WORKFLOW_SHA" ]]' + +# Every rejection is fatal, expressed without pinning a literal count: nothing in the guard may +# exit successfully mid-way, and each error annotation must be paired with a failing exit. A new +# axis therefore extends this cleanly instead of turning it red. +no "no rejection path exits non-fatally" "$code" "exit 0" +errors=$(printf '%s\n' "$code" | grep -c '::error::') +fatals=$(printf '%s\n' "$code" | grep -c 'exit 1') +eq "every ::error:: is paired with a failing exit" "$errors" "$fatals" +if [ "$errors" -ge 3 ]; then ok "all three rejection paths are present (shape, unreadable, mismatch)" +else bad "all three rejection paths are present (shape, unreadable, mismatch)" "$errors ::error:: lines"; fi # --- the input itself is never interpolated into the shell or echoed raw ---------------------- # `${{ inputs.workflows_ref }}` inline in `run:` would be a shell-injection vector, and echoing -# the raw value into a `::error::` lets a multi-line value forge workflow commands in a public log. -case "$body" in - *'run: |'*'${{'*) bad "the input reaches the script only via env:" "\${{ }} inside run:" ;; - *) ok "the input reaches the script only via env:" ;; -esac -case "$body" in - *'${WORKFLOWS_REF}'*) bad "only the sanitized value is echoed into annotations" "raw value echoed" ;; - *) ok "only the sanitized value is echoed into annotations" ;; -esac +# the raw value into a `::error::` lets a multi-line value forge workflow commands in a public +# log. The echo check matches the value in ANY spelling — `$WORKFLOWS_REF` unbraced is the more +# likely regression than `${WORKFLOWS_REF}`, so it must not be the one the test misses. +script="$(printf '%s\n' "$code" | sed -n '/run: |/,$p')" +no "the input reaches the script only via env:" "$script" '${{' +echoed="$(printf '%s\n' "$code" | grep -E '^[[:space:]]*echo ' | grep -E '\$\{?WORKFLOWS_REF' || true)" +eq "only the sanitized value is echoed into annotations" "" "$echoed" +has "the sanitized value is what the annotations use" "$code" '${safe_ref}' printf '\n%s passed, %s failed\n' "$PASS" "$FAIL" [ "$FAIL" -eq 0 ] From b68a2cb6be3511b589696afcc6600d91ab7cf689 Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Tue, 4 Aug 2026 03:55:02 -0700 Subject: [PATCH 4/8] fix(pr-risk): close the guard's neuter paths, and make the suite assert positions not sums (BE-6307) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Third review round, on the guard and on the suite that guards it. WORKFLOW - The mismatch error told a caller that a local `./` call must pass its own run's SHA. Unfollowable: the checkout below hardcodes `repository: Comfy-Org/github-workflows`, so taking that advice swaps a guard failure for a checkout failure on a SHA that does not exist there. Replaced with the remedy that does apply — including the tag case, where "the same SHA as `uses:`" is not actionable because `uses:` holds no SHA. - Documented the corollary rather than leaving it implicit: a tag- or branch-pinned `uses:` cannot satisfy the lock-step axis, since `job_workflow_sha` is then whatever the tag currently points at and a force-moved `v1` would change the tool under an already-reviewed caller. That is intended, and it is org policy already — `uses:` must be a full commit SHA or it fails the pin-validation consumer CI runs. SUITE — it was checkable in ways it claimed not to be - Nothing stopped the cheapest neuter of all: `continue-on-error: true` makes every `exit 1` advisory, and an `if:` switches the step off. Applied to both copies they stay byte-identical and every other assertion still passes while the checkout proceeds on an unvalidated ref. Both are now rejected. - Comparing TOTAL `::error::` and `exit 1` counts proved the sums match, not the pairing the assertion's name claimed — one path could degrade to log-and-continue while another gained a spare exit. Now positional: each `::error::` must be followed by `exit 1` as the next non-blank line. - `has`/`no` matched with `case` globbing, so the axis-1 needle `^[0-9a-fA-F]{40}$` was read as a glob (bracket class, literal braces) and could be satisfied by text that is not that regex. Now `grep -F`. - Checkout detection was anchored to one squashed spelling, so `ref: "${{ inputs.workflows_ref }}"` or a trailing comment was invisible — the very spellings the comment promised were covered. Now any `ref:` key naming `inputs.workflows_ref`, deliberately over-inclusive. - The raw-value ban inspected only `echo` lines. The runner re-parses any step output, so `printf`, a line continuation, and a redirect into `$GITHUB_STEP_SUMMARY` were all open. Now every line naming the value must consume it, never emit it, in any spelling. - The axis-2 env assertion read the un-stripped body, so a commented-out mapping would have satisfied it; and the drift loop had no `nullglob` guard, so an empty slicer would have `cmp`ed a nonexistent file instead of failing cleanly. Each of the above is mutation-verified, including the two evasions the panel described: an unguarded checkout written in the quoted spelling, and one rejection path degraded to log-and-continue while another gains a spare exit. --- .github/workflows/pr-risk.yml | 12 ++- scripts/pr-risk/tests/test_pin_contract.sh | 105 ++++++++++++++++----- 2 files changed, 89 insertions(+), 28 deletions(-) diff --git a/.github/workflows/pr-risk.yml b/.github/workflows/pr-risk.yml index 846a3ef..3342784 100644 --- a/.github/workflows/pr-risk.yml +++ b/.github/workflows/pr-risk.yml @@ -295,6 +295,14 @@ on: as well-shaped as any other SHA. It does not, and cannot, prove the running revision is upstream: that is what reviewing the caller's `uses:` line on its base branch is for. + + + COROLLARY: a tag- or branch-pinned `uses:` cannot satisfy this, since + `job_workflow_sha` is then whatever the tag currently points at and a + force-moved `v1` would silently change the tool under a reviewed + caller. That is intentional and matches org policy — `uses:` must be a + full commit SHA (a floating one fails the pin-validation consumer CI + already runs). Pin `uses:` to a SHA and pass the same SHA here. type: string required: true enabled: @@ -417,7 +425,7 @@ jobs: exit 1 fi if [[ "${WORKFLOWS_REF,,}" != "${JOB_WORKFLOW_SHA,,}" ]]; then - echo "::error::workflows_ref (${safe_ref}) is not the commit uses: resolved to (${JOB_WORKFLOW_SHA}), so the grader would load from a different revision of Comfy-Org/github-workflows than the one reviewed alongside this caller. Set workflows_ref to the SAME SHA as the uses: line calling this workflow (a local './' call must pass that run's own commit SHA)." + echo "::error::workflows_ref (${safe_ref}) is not the commit uses: resolved to (${JOB_WORKFLOW_SHA}), so the grader would load from a different revision of Comfy-Org/github-workflows than the one running. Set BOTH to that same full commit SHA. If the uses: line calling this workflow is pinned to a TAG or a BRANCH, fix that first: it must be a full commit SHA (org policy — a floating uses: fails the pin-validation consumer CI runs anyway), and this input takes the same SHA." exit 1 fi # The tool checkout is the same pinned-ref load the grade job does: the resolver is this @@ -548,7 +556,7 @@ jobs: exit 1 fi if [[ "${WORKFLOWS_REF,,}" != "${JOB_WORKFLOW_SHA,,}" ]]; then - echo "::error::workflows_ref (${safe_ref}) is not the commit uses: resolved to (${JOB_WORKFLOW_SHA}), so the grader would load from a different revision of Comfy-Org/github-workflows than the one reviewed alongside this caller. Set workflows_ref to the SAME SHA as the uses: line calling this workflow (a local './' call must pass that run's own commit SHA)." + echo "::error::workflows_ref (${safe_ref}) is not the commit uses: resolved to (${JOB_WORKFLOW_SHA}), so the grader would load from a different revision of Comfy-Org/github-workflows than the one running. Set BOTH to that same full commit SHA. If the uses: line calling this workflow is pinned to a TAG or a BRANCH, fix that first: it must be a full commit SHA (org policy — a floating uses: fails the pin-validation consumer CI runs anyway), and this input takes the same SHA." exit 1 fi - name: Load pr-risk tool diff --git a/scripts/pr-risk/tests/test_pin_contract.sh b/scripts/pr-risk/tests/test_pin_contract.sh index 02fc5a1..1c3d012 100644 --- a/scripts/pr-risk/tests/test_pin_contract.sh +++ b/scripts/pr-risk/tests/test_pin_contract.sh @@ -22,13 +22,21 @@ # * AN AXIS IS DROPPED, OR STOPS BEING FATAL. Shape alone proves the ref is immutable, not # which commit it is; the `github.job_workflow_sha` comparison is what proves it is the # revision already running. Either axis degraded to a warning is a silent no-op. +# * THE STEP IS NEUTERED WHOLESALE. `continue-on-error: true` makes every `exit 1` advisory +# and an `if:` switches the step off — added to both copies they stay byte-identical, every +# assertion above still holds, and the checkout proceeds on an unvalidated ref. +# * THE RAW VALUE IS EMITTED AGAIN. The runner re-parses any line of step output, so a +# multi-line ref can forge workflow commands in a public log. # -# ASSERT PROPERTIES, NOT COUNTS. An earlier draft pinned a literal number of `exit 1` lines, -# which would have gone red on the very next hardening of the guard — a test that blocks its own -# subject's improvement teaches people to delete the test. What is asserted here instead is that -# no rejection path can be non-fatal (no `exit 0` at all) and that every `::error::` is paired -# with an exit. The awk passes also self-check that they matched anything, so a brittle anchor -# fails loudly rather than passing vacuously with zero coverage. +# ASSERT PROPERTIES, NOT COUNTS, AND POSITIONS, NOT SUMS. An earlier draft pinned a literal +# number of `exit 1` lines, which would have gone red on the very next hardening of the guard — +# a test that blocks its own subject's improvement teaches people to delete the test. A later +# one compared total `::error::` and `exit 1` counts, which one path could satisfy on another's +# behalf. What is asserted here is that no rejection path can be non-fatal (no `exit 0` at all) +# and that each `::error::` is FOLLOWED by an exit. The awk passes also self-check that they +# matched anything, so a brittle anchor fails loudly rather than passing vacuously with zero +# coverage — and the patterns are deliberately over-inclusive, since a false positive here costs +# a puzzled minute and a false negative ships an unguarded checkout. # # bash tests/test_pin_contract.sh # exit 0 = all green set -uo pipefail @@ -41,8 +49,10 @@ PASS=0; FAIL=0 ok() { PASS=$((PASS+1)); printf 'ok %s\n' "$1"; } bad() { FAIL=$((FAIL+1)); printf 'FAIL %s\n got: %s\n' "$1" "${2:-}"; } eq() { if [ "$2" = "$3" ]; then ok "$1"; else bad "$1 (expected '$2')" "$3"; fi } -has() { case "$2" in *"$3"*) ok "$1" ;; *) bad "$1" "not found: $3" ;; esac } -no() { case "$2" in *"$3"*) bad "$1" "present: $3" ;; *) ok "$1" ;; esac } +# `grep -F`, not `case` globbing: half these needles are regex text (`^[0-9a-fA-F]{40}$`), whose +# brackets and braces a glob would happily reinterpret into something laxer than it reads. +has() { if printf '%s\n' "$2" | grep -qF -- "$3"; then ok "$1"; else bad "$1" "not found: $3"; fi } +no() { if printf '%s\n' "$2" | grep -qF -- "$3"; then bad "$1" "present: $3"; else ok "$1"; fi } GUARD_NAME=' - name: Enforce workflows_ref pin contract' @@ -51,8 +61,10 @@ GUARD_NAME=' - name: Enforce workflows_ref pin contract' # each one, so a guard in `gate` cannot vouch for a checkout in `grade`. Checkout detection is # deliberately NOT a byte-exact line match: a future job that quotes the expression, drops the # inner spaces, indents differently, or adds a trailing comment must still be seen, or an -# unguarded checkout ships green and defeats the point of this file. Match on the whitespace- -# stripped form instead. The awk also reports how many jobs and how many refs it saw, so a +# unguarded checkout ships green and defeats the point of this file. So: strip whitespace, then +# match any `ref:` key mentioning `inputs.workflows_ref` ANYWHERE in the value. Deliberately +# over-inclusive — a false positive here costs one puzzled minute, a false negative ships an +# unguarded checkout. The awk also reports how many jobs and how many refs it saw, so a # pattern that silently matches nothing fails below rather than printing a vacuous `ok`. scan="$(awk -v guard="$GUARD_NAME" ' function squash(s) { gsub(/[[:space:]]/, "", s); return s } @@ -60,7 +72,7 @@ scan="$(awk -v guard="$GUARD_NAME" ' !injobs { next } /^ [A-Za-z0-9_-]+:[[:space:]]*(#.*)?$/ { job = $1; jobs += 1; guarded = 0; next } $0 == guard { guarded = 1; next } - squash($0) ~ /^ref:\$\{\{inputs\.workflows_ref\}\}$/ { + squash($0) ~ /^ref:.*inputs\.workflows_ref/ { refs += 1 if (!guarded) print "UNGUARDED:" job } @@ -102,15 +114,26 @@ awk -v guard="$GUARD_NAME" -v out="$copies" ' /^ / { print > f; next } { inguard = 0 } ' "$WF" +# Without `set -e` and without `nullglob`, a slicer that produced nothing would leave the loop +# below iterating the literal glob and `cmp`-ing a nonexistent file — the byte-identity check +# would dissolve into a confusing message instead of a clean failure. Count first. +nslices=$(find "$copies" -maxdepth 1 -name 'guard.*' -type f | wc -l | tr -d ' ') +if [ "$nslices" -ge 2 ]; then ok "the guard slicer produced one slice per copy ($nslices)" +else bad "the guard slicer produced one slice per copy" "$nslices — nothing to compare"; fi + drift="" first="$copies/guard.1" -for f in "$copies"/guard.*; do - cmp -s "$first" "$f" || drift="$drift $(basename "$f")" -done +if [ -f "$first" ]; then + for f in "$copies"/guard.*; do + cmp -s "$first" "$f" || drift="$drift $(basename "$f")" + done +else + drift="no slices" +fi eq "all copies of the guard step are byte-identical" "" "$drift" # --- both axes are still enforced, and no rejection path can be non-fatal --------------------- -body="$(cat "$first")" +body="$(cat "$first" 2>/dev/null)" # Comments are stripped before any assertion about control flow, so an `exit 1` or an `exit 0` # quoted in prose neither satisfies nor breaks a check. code="$(printf '%s\n' "$body" | sed 's/[[:space:]]*#.*$//')" @@ -118,31 +141,61 @@ code="$(printf '%s\n' "$body" | sed 's/[[:space:]]*#.*$//')" has "axis 1: the ref must be shaped like a full 40-hex commit SHA" \ "$code" '^[0-9a-fA-F]{40}$' has "axis 2: the runner-supplied resolved SHA is read into the guard" \ - "$body" 'JOB_WORKFLOW_SHA: ${{ github.job_workflow_sha }}' + "$code" 'JOB_WORKFLOW_SHA: ${{ github.job_workflow_sha }}' has "axis 2: the pin is compared against it, case-insensitively" \ "$code" '"${WORKFLOWS_REF,,}" != "${JOB_WORKFLOW_SHA,,}"' has "axis 2: an unreadable job_workflow_sha is itself a rejection (fail closed)" \ "$code" '[[ -z "$JOB_WORKFLOW_SHA" ]]' # Every rejection is fatal, expressed without pinning a literal count: nothing in the guard may -# exit successfully mid-way, and each error annotation must be paired with a failing exit. A new +# exit successfully mid-way, and each error annotation must be followed by a failing exit. A new # axis therefore extends this cleanly instead of turning it red. no "no rejection path exits non-fatally" "$code" "exit 0" +# Positionally, not by totals. Equal SUMS would let one path degrade to log-and-continue so long +# as another gained a spare `exit 1` — which is exactly the regression this is here to catch, so +# each `::error::` must be followed by `exit 1` as the next non-blank line. +unpaired="$(printf '%s\n' "$code" | awk ' + /^[[:space:]]*$/ { next } + pending && $0 !~ /^[[:space:]]*exit 1[[:space:]]*$/ { print "UNPAIRED:" NR; pending = 0 } + { pending = /::error::/ } + END { if (pending) print "UNPAIRED:eof" } +' | tr '\n' ' ' | sed 's/ $//')" +eq "every ::error:: is followed by a failing exit" "" "$unpaired" errors=$(printf '%s\n' "$code" | grep -c '::error::') -fatals=$(printf '%s\n' "$code" | grep -c 'exit 1') -eq "every ::error:: is paired with a failing exit" "$errors" "$fatals" if [ "$errors" -ge 3 ]; then ok "all three rejection paths are present (shape, unreadable, mismatch)" else bad "all three rejection paths are present (shape, unreadable, mismatch)" "$errors ::error:: lines"; fi -# --- the input itself is never interpolated into the shell or echoed raw ---------------------- -# `${{ inputs.workflows_ref }}` inline in `run:` would be a shell-injection vector, and echoing -# the raw value into a `::error::` lets a multi-line value forge workflow commands in a public -# log. The echo check matches the value in ANY spelling — `$WORKFLOWS_REF` unbraced is the more -# likely regression than `${WORKFLOWS_REF}`, so it must not be the one the test misses. +# --- 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 +# off. Added to BOTH copies they stay identical, every error stays paired, and the checkout +# proceeds with an unvalidated ref. So the step must carry neither. +no "the guard is not softened by continue-on-error" "$code" "continue-on-error" +gatedon="$(printf '%s\n' "$code" | grep -E '^ if:' || true)" +eq "the guard is not conditional (no step-level if:)" "" "$gatedon" + +# --- the input itself is never interpolated into the shell nor emitted raw -------------------- +# `${{ inputs.workflows_ref }}` inline in `run:` would be a shell-injection vector, and emitting +# the raw value lets a multi-line value forge workflow commands in a public log — the runner +# re-parses ANY line of step output, so this is not only about `echo` and not only about lines +# that themselves contain `::`. Every line naming the value must therefore be one that consumes +# it (a `[[ ]]` test, or the assignment that sanitizes it), never one that emits it: no bare +# `echo`/`printf`, no workflow command, no redirect into an Actions file, and no continuation of +# a line that was doing one of those. script="$(printf '%s\n' "$code" | sed -n '/run: |/,$p')" no "the input reaches the script only via env:" "$script" '${{' -echoed="$(printf '%s\n' "$code" | grep -E '^[[:space:]]*echo ' | grep -E '\$\{?WORKFLOWS_REF' || true)" -eq "only the sanitized value is echoed into annotations" "" "$echoed" +emitted="$(printf '%s\n' "$script" | awk ' + { line = $0; sub(/^[[:space:]]+/, "", line) } + line ~ /\$\{?WORKFLOWS_REF/ { + if (cont) { print "CONTINUATION:" NR } + else if (line ~ /::/) { print "WORKFLOW-COMMAND:" NR } + else if (line ~ /GITHUB_(STEP_SUMMARY|OUTPUT|ENV|PATH)/) { print "ACTIONS-FILE:" NR } + else if (line ~ /^(echo|printf)[[:space:]]/) { print "BARE-EMIT:" NR } + else if (line ~ /[^0-9a-zA-Z_]>>?[[:space:]]*[\$\/"]/) { print "REDIRECT:" NR } + } + { cont = (line ~ /\\$/) } +' | tr '\n' ' ' | sed 's/ $//')" +eq "the raw value is consumed, never emitted, in any spelling" "" "$emitted" has "the sanitized value is what the annotations use" "$code" '${safe_ref}' printf '\n%s passed, %s failed\n' "$PASS" "$FAIL" From f314614c9128901134b2baeef37cd35335b7e079 Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Tue, 4 Aug 2026 07:16:41 -0700 Subject: [PATCH 5/8] fix(pr-risk): correct the pin contract's claims, sanitize the runner SHA, and close the suite's bypass holes (BE-6307) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-ups on the `workflows_ref` guard. No change to what the guard accepts or rejects; the changes are to what it CLAIMS, what it echoes, and what the structural suite can actually catch. Docs, where the header overstated the guarantee: - The COROLLARY said a tag- or branch-pinned `uses:` "cannot satisfy this". It can: `job_workflow_sha` arrives already RESOLVED, so a `@v1` caller passing the SHA that tag points at clears both axes. Proving `uses:` was SHA-pinned needs the caller's original ref, which the called workflow never sees. Reworded to say so, and to state the real consequence — the inverse of the old text — that every sanctioned `git tag -f v1 ` move takes such callers red. - State that nesting is unsupported: in a caller -> org wrapper -> pr-risk chain `job_workflow_sha` names one commit, so no single `workflows_ref` satisfies both this guard and the wrapper's pin. Call the workflow directly. - Document that the guard precedes enablement and is not subject to it (the resolver is itself loaded from `workflows_ref`), so a drifted pin fails red even with the `RISK_CONFIG` kill switch set. The switch stops the grading, not a broken enrollment. README's pr-risk row carries all three. Guard: - Sanitize `JOB_WORKFLOW_SHA` through the same tr/truncate as the ref before it reaches the mismatch annotation. It was only ever tested for emptiness, never for shape, while the comment beside it argues the platform could reshape that property out from under us — so a multi-line value could have ended the annotation early and left the rest to be re-parsed as workflow commands in a public log. Both copies stay byte-identical. Suite, each hole verified by mutating the workflow and watching the named assertion go red: - Assert `WORKFLOWS_REF: ${{ inputs.workflows_ref }}`. Rebinding it to `${{ github.job_workflow_sha }}` made the lock-step test a tautology that passed while the checkout used the unvalidated input. - Assert the PROTECTED step, not just the guard: no `if:` / `continue-on-error` on `Load pr-risk tool`, and no job-level `continue-on-error`. `if: always()` on the checkout ran it after the guard exited 1, guard byte-identical. - Forbid aliasing the input out of the two shapes the scans understand, so an `env:` binding, a composite-action `with:`, or a `git fetch` in a `run:` step cannot route around the "every checkout is guarded" scan. - Invert the emit scan to a whitelist: the raw value may be sanitized or tested and nothing else. The blacklist missed one hop (`raw=$WORKFLOWS_REF` then `echo "$raw"`) and every spelling nobody enumerated (`export`, `declare`, `read <<<`, `case`). - Give the `/run: |/` anchor a coverage self-check; reshaping the block scalar emptied `$script` and both following assertions passed over nothing. - Strip only WHOLE-LINE comments. The trailing-`#` strip truncated any line carrying a `#` in a string (`github-workflows#NN`), which could have hidden the `$WORKFLOWS_REF` mention the emit scan exists to inspect. --- .github/workflows/pr-risk.yml | 75 ++++++++++--- README.md | 2 +- scripts/pr-risk/tests/test_pin_contract.sh | 116 ++++++++++++++++++--- 3 files changed, 166 insertions(+), 27 deletions(-) diff --git a/.github/workflows/pr-risk.yml b/.github/workflows/pr-risk.yml index 3342784..e755b74 100644 --- a/.github/workflows/pr-risk.yml +++ b/.github/workflows/pr-risk.yml @@ -25,8 +25,13 @@ name: PR Risk Grade (reusable) # # What that does NOT prove — and cannot, from in here — is that the running revision is upstream # code: `job_workflow_sha` is whatever the CALLER's own `uses:` resolved to, so a caller pointing -# `uses:` itself at a fork commit is already running the fork's copy of this very file. That case -# is bounded by review of the caller on its base branch, not by any check this workflow can make. +# `uses:` itself at a fork commit is already running the fork's copy of this very file. Nor does +# it prove the caller SHA-pinned `uses:` at all: the value arrives already resolved, so a +# `uses: ...@v1` caller passing the SHA that tag points at satisfies both axes (it just goes red +# the next time `v1` is force-moved). Both cases are bounded by review of the caller on its base +# branch, not by any check this workflow can make. Call this workflow DIRECTLY — a nested +# `workflow_call` chain through an org wrapper is unsupported, because `job_workflow_sha` names +# one commit and the wrapper's is not this file's; see the `workflows_ref` input for the detail. # A consumer repo sharpens the generic defaults by committing # `.github/risk.json` (map) / `.github/risk-runbooks.json` (runbook registry), which are read # from the PR's BASE ref: present-but-invalid fails the run loudly; absent falls back to the @@ -297,12 +302,32 @@ on: `uses:` line on its base branch is for. - COROLLARY: a tag- or branch-pinned `uses:` cannot satisfy this, since - `job_workflow_sha` is then whatever the tag currently points at and a - force-moved `v1` would silently change the tool under a reviewed - caller. That is intentional and matches org policy — `uses:` must be a - full commit SHA (a floating one fails the pin-validation consumer CI - already runs). Pin `uses:` to a SHA and pass the same SHA here. + WHAT IT CANNOT SEE is the caller's ORIGINAL `uses:` ref. + `job_workflow_sha` is already the RESOLVED commit, so a `uses: ...@v1` + caller that passes whatever `v1` currently points at satisfies both + axes: this guard does NOT reject a tag- or branch-pinned `uses:`, and + no check inside the called workflow can — proving that needs the ref + the caller wrote, which is never sent. Reviewing the caller's `uses:` + line on its base branch is what bounds it, and org policy already + requires a full commit SHA there (a floating one fails the + pin-validation consumer CI runs). + + + What a floating `uses:` does buy is a LOUD failure on the next + sanctioned `git tag -f v1 ` move: the tag resolves somewhere new, + this hand-written pin does not follow, and every tag-pinned caller goes + red until it is repinned. That is the intended direction of the + failure — better a red check than a tool silently swapped under a + reviewed caller — but it is a cost the tag-pinned caller pays, not a + rejection at the door. Pin `uses:` to a SHA and pass the same SHA here. + + + CALL IT DIRECTLY — a nested `workflow_call` chain (caller → an org + wrapper workflow → this one) is NOT supported. `job_workflow_sha` names + ONE commit for the job, and a wrapper living in another repo (or at + another commit of this one) is a different commit than this file, so no + single `workflows_ref` value can satisfy both this guard and the + wrapper's own pin. Name `pr-risk.yml` in the caller's `uses:`. type: string required: true enabled: @@ -338,6 +363,18 @@ on: empty, not an object, or carries no boolean `enabled` — a malformed variable must never be the reason a repo silently stops grading, so it degrades to this value and says so in an annotation. + + + SCOPE OF THE KILL SWITCH: it stops the GRADING, not a broken + enrollment. The `workflows_ref` pin contract is enforced BEFORE this + resolves and is not subject to it — the resolver script is itself + loaded from `workflows_ref`, so there is no point in the run at which a + mismatched pin could be read as "switched off" without first checking + out the very ref under suspicion. A caller whose two pins have drifted + therefore fails red on every PR even with `{"enabled": false}` set. The + fix is the one-line repin (or dropping the caller) — both PRs, but a + caller that cannot say which revision of the tool it runs is not in a + state the variable is meant to cover. type: boolean required: false default: false @@ -396,14 +433,19 @@ jobs: JOB_WORKFLOW_SHA: ${{ github.job_workflow_sha }} run: | set -euo pipefail - # Never interpolate the raw value into a `::error::`. A multi-line value — exactly what + # Never interpolate a raw value into a `::error::`. A multi-line value — exactly what # the `[[ =~ ]]` choice exists to catch — would end the annotation at the first newline # and leave the remainder to be re-parsed by the runner as workflow commands # (`::add-mask::`, `::stop-commands::`, a forged `::notice::`) in a PUBLIC log. Anything # outside the ref alphabet becomes `?` and the result is truncated, so what is echoed is - # always one bounded line. + # always one bounded line. The RUNNER-supplied SHA gets the identical treatment, not + # because a caller can forge it, but because it is only ever tested for emptiness and + # never for shape — and the block below argues the platform could rename or reshape that + # property out from under this guard. Neither value is trusted to be one tidy line. safe_ref=$(printf '%s' "$WORKFLOWS_REF" | tr -c 'A-Za-z0-9._/-' '?') safe_ref=${safe_ref:0:64} + safe_job_sha=$(printf '%s' "$JOB_WORKFLOW_SHA" | tr -c 'A-Za-z0-9._/-' '?') + safe_job_sha=${safe_job_sha:0:64} if [[ ! "$WORKFLOWS_REF" =~ ^[0-9a-fA-F]{40}$ ]]; then echo "::error::workflows_ref must be the FULL 40-hex 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 @@ -425,7 +467,7 @@ jobs: exit 1 fi if [[ "${WORKFLOWS_REF,,}" != "${JOB_WORKFLOW_SHA,,}" ]]; then - echo "::error::workflows_ref (${safe_ref}) is not the commit uses: resolved to (${JOB_WORKFLOW_SHA}), so the grader would load from a different revision of Comfy-Org/github-workflows than the one running. Set BOTH to that same full commit SHA. If the uses: line calling this workflow is pinned to a TAG or a BRANCH, fix that first: it must be a full commit SHA (org policy — a floating uses: fails the pin-validation consumer CI runs anyway), and this input takes the same SHA." + echo "::error::workflows_ref (${safe_ref}) is not the commit uses: resolved to (${safe_job_sha}), so the grader would load from a different revision of Comfy-Org/github-workflows than the one running. Set BOTH to that same full commit SHA. If the uses: line calling this workflow is pinned to a TAG or a BRANCH, fix that first: it must be a full commit SHA (org policy — a floating uses: fails the pin-validation consumer CI runs anyway), and this input takes the same SHA." exit 1 fi # The tool checkout is the same pinned-ref load the grade job does: the resolver is this @@ -527,14 +569,19 @@ jobs: JOB_WORKFLOW_SHA: ${{ github.job_workflow_sha }} run: | set -euo pipefail - # Never interpolate the raw value into a `::error::`. A multi-line value — exactly what + # Never interpolate a raw value into a `::error::`. A multi-line value — exactly what # the `[[ =~ ]]` choice exists to catch — would end the annotation at the first newline # and leave the remainder to be re-parsed by the runner as workflow commands # (`::add-mask::`, `::stop-commands::`, a forged `::notice::`) in a PUBLIC log. Anything # outside the ref alphabet becomes `?` and the result is truncated, so what is echoed is - # always one bounded line. + # always one bounded line. The RUNNER-supplied SHA gets the identical treatment, not + # because a caller can forge it, but because it is only ever tested for emptiness and + # never for shape — and the block below argues the platform could rename or reshape that + # property out from under this guard. Neither value is trusted to be one tidy line. safe_ref=$(printf '%s' "$WORKFLOWS_REF" | tr -c 'A-Za-z0-9._/-' '?') safe_ref=${safe_ref:0:64} + safe_job_sha=$(printf '%s' "$JOB_WORKFLOW_SHA" | tr -c 'A-Za-z0-9._/-' '?') + safe_job_sha=${safe_job_sha:0:64} if [[ ! "$WORKFLOWS_REF" =~ ^[0-9a-fA-F]{40}$ ]]; then echo "::error::workflows_ref must be the FULL 40-hex 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 @@ -556,7 +603,7 @@ jobs: exit 1 fi if [[ "${WORKFLOWS_REF,,}" != "${JOB_WORKFLOW_SHA,,}" ]]; then - echo "::error::workflows_ref (${safe_ref}) is not the commit uses: resolved to (${JOB_WORKFLOW_SHA}), so the grader would load from a different revision of Comfy-Org/github-workflows than the one running. Set BOTH to that same full commit SHA. If the uses: line calling this workflow is pinned to a TAG or a BRANCH, fix that first: it must be a full commit SHA (org policy — a floating uses: fails the pin-validation consumer CI runs anyway), and this input takes the same SHA." + echo "::error::workflows_ref (${safe_ref}) is not the commit uses: resolved to (${safe_job_sha}), so the grader would load from a different revision of Comfy-Org/github-workflows than the one running. Set BOTH to that same full commit SHA. If the uses: line calling this workflow is pinned to a TAG or a BRANCH, fix that first: it must be a full commit SHA (org policy — a floating uses: fails the pin-validation consumer CI runs anyway), and this input takes the same SHA." exit 1 fi - name: Load pr-risk tool diff --git a/README.md b/README.md index fa545ca..8d1ef66 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. Label text is remappable via `label_map`. `workflows_ref` is **required** and **ENFORCED** — pin it to the same full commit SHA as `uses:`, so the grader cannot be loaded from a floating ref after the caller was reviewed. Every job that checks it out fails the run *before* the tool checkout unless the value is a full 40-hex commit SHA **and** equals `github.job_workflow_sha`, the commit `uses:` resolved to: a branch, a tag or a `refs/pull/N/head` is rejected by the first test, and a well-shaped SHA that is not the revision already running — a stale pin left behind when `uses:` moved, or a fork-authored commit of this public repo handed to `workflows_ref` while `uses:` stayed upstream — by the second. Scoped to exactly that: it stops the two halves of the tool disagreeing, and does not (and from inside the called workflow cannot) prove the running revision is upstream — reviewing the caller's `uses:` line is what bounds that. 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. Label text is remappable via `label_map`. `workflows_ref` is **required** and **ENFORCED** — pin it to the same full commit SHA as `uses:`, so the grader cannot be loaded from a floating ref after the caller was reviewed. Every job that checks it out fails the run *before* the tool checkout unless the value is a full 40-hex commit SHA **and** equals `github.job_workflow_sha`, the commit `uses:` resolved to: a branch, a tag or a `refs/pull/N/head` is rejected by the first test, and a well-shaped SHA that is not the revision already running — a stale pin left behind when `uses:` moved, or a fork-authored commit of this public repo handed to `workflows_ref` while `uses:` stayed upstream — by the second. Scoped to exactly that: it stops the two halves of the tool disagreeing. It does not (and from inside the called workflow cannot) prove the running revision is upstream, nor that `uses:` was SHA-pinned rather than tag-pinned — the value arrives already resolved, so a tag-pinned caller passing the SHA the tag points at satisfies both axes and simply goes red the next time that tag is force-moved; reviewing the caller's `uses:` line is what bounds both. The guard also runs *before* enablement is resolved (the resolver is itself loaded from `workflows_ref`), so a drifted 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. | [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/scripts/pr-risk/tests/test_pin_contract.sh b/scripts/pr-risk/tests/test_pin_contract.sh index 1c3d012..7b90242 100644 --- a/scripts/pr-risk/tests/test_pin_contract.sh +++ b/scripts/pr-risk/tests/test_pin_contract.sh @@ -22,11 +22,21 @@ # * AN AXIS IS DROPPED, OR STOPS BEING FATAL. Shape alone proves the ref is immutable, not # which commit it is; the `github.job_workflow_sha` comparison is what proves it is the # revision already running. Either axis degraded to a warning is a silent no-op. +# * AN AXIS BECOMES A TAUTOLOGY. Rebind `WORKFLOWS_REF` to `${{ github.job_workflow_sha }}` +# and the lock-step test compares the resolved SHA with itself — always green, while the +# checkout below still uses the unvalidated input. Nothing about the guard's SHAPE changes. # * THE STEP IS NEUTERED WHOLESALE. `continue-on-error: true` makes every `exit 1` advisory # and an `if:` switches the step off — added to both copies they stay byte-identical, every -# assertion above still holds, and the checkout proceeds on an unvalidated ref. +# assertion above still holds, and the checkout proceeds on an unvalidated ref. Or the +# cheaper version, which leaves the guard untouched and edits its VICTIM: `if: always()` on +# the checkout, or `continue-on-error` at job level. +# * THE INPUT IS ALIASED PAST THE SCAN. The unguarded-checkout scan matches `ref:` keys naming +# `inputs.workflows_ref`. Bind it to an `env:` key first, forward it to a composite action, +# or hand it to a `git fetch` in a `run:` step, and the scan has nothing left to see. # * THE RAW VALUE IS EMITTED AGAIN. The runner re-parses any line of step output, so a -# multi-line ref can forge workflow commands in a public log. +# multi-line ref can forge workflow commands in a public log — including one hop through +# another variable, which is why the emit scan whitelists what may touch the value rather +# than blacklisting the emit shapes someone thought of. # # ASSERT PROPERTIES, NOT COUNTS, AND POSITIONS, NOT SUMS. An earlier draft pinned a literal # number of `exit 1` lines, which would have gone red on the very next hardening of the guard — @@ -134,10 +144,22 @@ eq "all copies of the guard step are byte-identical" "" "$drift" # --- both axes are still enforced, and no rejection path can be non-fatal --------------------- body="$(cat "$first" 2>/dev/null)" -# Comments are stripped before any assertion about control flow, so an `exit 1` or an `exit 0` -# quoted in prose neither satisfies nor breaks a check. -code="$(printf '%s\n' "$body" | sed 's/[[:space:]]*#.*$//')" +# WHOLE-LINE comments are dropped before any assertion about control flow, so an `exit 1` or an +# `exit 0` quoted in prose neither satisfies nor breaks a check. Deliberately NOT a trailing-`#` +# strip: `#` has no special meaning inside a shell string, and this repo routinely writes +# `github-workflows#NN` and the like in exactly the annotation lines below — a naive +# `s/[[:space:]]*#.*$//` would truncate such a line and could silently drop the `$WORKFLOWS_REF` +# mention that the emit scan exists to inspect. The residual cost runs the other, harmless way: +# a trailing comment that happens to say `exit 0` trips a check. False positive, one puzzled +# minute — the trade this file makes everywhere. +code="$(printf '%s\n' "$body" | grep -v '^[[:space:]]*#')" +# The value under test must be the caller's INPUT, not a restatement of the runner's own. Rebind +# it to `${{ github.job_workflow_sha }}` and the lock-step comparison compares the resolved SHA +# with itself — a tautology that always passes while the checkout below still uses the +# unvalidated input, with every other assertion in this file staying green. +has "the guarded value is the caller's input, not the resolved SHA restated" \ + "$code" 'WORKFLOWS_REF: ${{ inputs.workflows_ref }}' has "axis 1: the ref must be shaped like a full 40-hex commit SHA" \ "$code" '^[0-9a-fA-F]{40}$' has "axis 2: the runner-supplied resolved SHA is read into the guard" \ @@ -174,6 +196,30 @@ no "the guard is not softened by continue-on-error" "$code" "continue-on-error" gatedon="$(printf '%s\n' "$code" | grep -E '^ if:' || true)" eq "the guard is not conditional (no step-level if:)" "" "$gatedon" +# --- nor can the step it PROTECTS be made to run anyway --------------------------------------- +# Everything above inspects the guard. The cheaper bypass leaves the guard byte-identical and +# edits its victim instead: `if: always()` (or `success() || failure()`) on the checkout makes it +# run after the guard has exited 1, and a job-level `continue-on-error: true` demotes the guard's +# failure for the whole job. Either one and the checkout proceeds on an unvalidated ref with +# every assertion in this file still green — so the protected step is checked too. +CHECKOUT_NAME=' - name: Load pr-risk tool' +protected="$(awk -v step="$CHECKOUT_NAME" ' + $0 == step { instep = 1; print; next } + !instep { next } + /^[[:space:]]*$/ { next } + /^ / { print; next } + { instep = 0 } +' "$WF" | grep -v '^[[:space:]]*#')" +nprotected=$(printf '%s\n' "$protected" | grep -cxF "$CHECKOUT_NAME") +if [ "$nprotected" = "$nrefs" ]; then ok "the protected-checkout scan matched every guarded checkout ($nprotected)" +else bad "the protected-checkout scan matched every guarded checkout" "$nprotected of $nrefs — anchors are stale, coverage is vacuous"; fi +no "the protected checkout is not run-anyway (no step-level if:)" "$protected" "if:" +no "the protected checkout is not softened by continue-on-error" "$protected" "continue-on-error" +# Job level, where one key covers the guard and its checkout at once. `if:` at this indent is +# legitimate (`grade` is gated on enablement); `continue-on-error` never is. +jobsoft="$(grep -nE '^ continue-on-error:' "$WF" | tr '\n' ' ' | sed 's/ $//')" +eq "no job demotes its own failures wholesale (no job-level continue-on-error)" "" "$jobsoft" + # --- the input itself is never interpolated into the shell nor emitted raw -------------------- # `${{ inputs.workflows_ref }}` inline in `run:` would be a shell-injection vector, and emitting # the raw value lets a multi-line value forge workflow commands in a public log — the runner @@ -183,20 +229,66 @@ eq "the guard is not conditional (no step-level if:)" "" "$gatedon" # `echo`/`printf`, no workflow command, no redirect into an Actions file, and no continuation of # a line that was doing one of those. script="$(printf '%s\n' "$code" | sed -n '/run: |/,$p')" -no "the input reaches the script only via env:" "$script" '${{' +# Unlike every other anchor here, this one had no coverage self-check: reshape the block scalar +# (`run: >-`, or a `run:` with a trailing comment) and `$script` comes back EMPTY, at which point +# both assertions below pass over nothing at all — the vacuous-pass mode this file's header +# claims to have eliminated. Anchor on a line the guard's script must contain. +has "the run: block scan found the guard's script body" "$script" 'set -euo pipefail' +# WHITELIST, not blacklist. The named categories below are diagnostics — they say WHICH way a +# line leaks — but the verdict is the `else`: the raw value may be read by the sanitizing +# assignment and by the `[[ ]]` tests, and by NOTHING else. A blacklist of emit-shapes has a +# one-hop hole (`raw=$WORKFLOWS_REF` on one line, `echo "$raw"` on the next, and every shape rule +# sees nothing) and an open-ended tail of spellings to keep chasing — `export`, `local`, `read`, +# a herestring, a here-doc. Inverting it is what lets the assertion's name be true: any new way +# of touching the value is reported until someone widens this list on purpose. emitted="$(printf '%s\n' "$script" | awk ' { line = $0; sub(/^[[:space:]]+/, "", line) } line ~ /\$\{?WORKFLOWS_REF/ { - if (cont) { print "CONTINUATION:" NR } - else if (line ~ /::/) { print "WORKFLOW-COMMAND:" NR } - else if (line ~ /GITHUB_(STEP_SUMMARY|OUTPUT|ENV|PATH)/) { print "ACTIONS-FILE:" NR } - else if (line ~ /^(echo|printf)[[:space:]]/) { print "BARE-EMIT:" NR } - else if (line ~ /[^0-9a-zA-Z_]>>?[[:space:]]*[\$\/"]/) { print "REDIRECT:" NR } + sanctioned = (!cont && (line ~ /^safe_ref=\$\(printf/ || line ~ /^(el)?if \[\[ /)) + if (!sanctioned) { + if (cont) { print "CONTINUATION:" NR } + else if (line ~ /::/) { print "WORKFLOW-COMMAND:" NR } + else if (line ~ /GITHUB_(STEP_SUMMARY|OUTPUT|ENV|PATH)/) { print "ACTIONS-FILE:" NR } + else if (line ~ /^(echo|printf)[[:space:]]/) { print "BARE-EMIT:" NR } + else if (line ~ /[^0-9a-zA-Z_]>>?[[:space:]]*[\$\/"]/) { print "REDIRECT:" NR } + else { print "UNSANCTIONED:" NR } + } } { cont = (line ~ /\\$/) } ' | tr '\n' ' ' | sed 's/ $//')" -eq "the raw value is consumed, never emitted, in any spelling" "" "$emitted" +eq "the raw value is only sanitized or tested, never emitted or copied elsewhere" "" "$emitted" +no "the input reaches the script only via env:" "$script" '${{' has "the sanitized value is what the annotations use" "$code" '${safe_ref}' +# --- ...and the input is never aliased out from under the checkout scan ------------------------ +# The "every workflows_ref checkout sits behind the guard" scan at the top matches literal `ref:` +# keys naming `inputs.workflows_ref`. Bind the input to something else first — `env: REF: ${{ +# inputs.workflows_ref }}` then `ref: ${{ env.REF }}`, or forward it to a composite action's +# `with:`, or hand it to a `git fetch`/`gh api` in a `run:` step — and that scan sees no `ref:` +# to check, `REFS` stays where it was, and an unguarded fetch of an unvalidated ref ships green. +# Closing that by teaching the scan to follow aliases is a dataflow problem; forbidding the alias +# is a grep. So: OUTSIDE comments, `inputs.workflows_ref` may appear only as the guard's own env +# binding or as a `ref:` key — the two shapes the scans above actually understand. A future job +# that legitimately needs it elsewhere adds the shape here, deliberately, rather than by accident. +aliased="$(awk ' + { line = $0; sub(/^[[:space:]]*/, "", line) } + line ~ /^#/ { next } + line !~ /inputs\.workflows_ref/ { next } + { squashed = line; gsub(/[[:space:]]/, "", squashed) } + squashed ~ /^WORKFLOWS_REF:\$\{\{inputs\.workflows_ref\}\}$/ { next } + squashed ~ /^ref:/ { next } + { print "ALIASED:" NR } +' "$WF" | tr '\n' ' ' | sed 's/ $//')" +eq "the input is referenced only as the guard's env binding or a ref: key" "" "$aliased" +nmentions=$(awk ' + { line = $0; sub(/^[[:space:]]*/, "", line) } + line ~ /^#/ { next } + line ~ /inputs\.workflows_ref/ { n += 1 } + END { print n+0 } +' "$WF") +want=$((nrefs * 2)) +if [ "$nmentions" -ge "$want" ]; then ok "the alias scan saw one env binding and one ref: per guarded job ($nmentions)" +else bad "the alias scan saw one env binding and one ref: per guarded job" "$nmentions, expected >= $want — anchors are stale, coverage is vacuous"; fi + printf '\n%s passed, %s failed\n' "$PASS" "$FAIL" [ "$FAIL" -eq 0 ] From c77b30b684ccf2b6f1c3bee74520dfd2d700c7a6 Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Tue, 4 Aug 2026 07:51:22 -0700 Subject: [PATCH 6/8] fix(pr-risk): scope the alias scan to the guard step, judge emits per statement, cover the runner SHA (BE-6307) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review round 4 on the `workflows_ref` pin contract. Three holes in the suite's own claimed guarantees, and one thing the guard cannot see that the docs did not name. - ALIAS SCAN, STEP OWNERSHIP. Sanctioning the env-binding SHAPE anywhere in the file left the bypass open one step to the right: a later step binding `WORKFLOWS_REF` of its own and running `git fetch origin "$WORKFLOWS_REF"` adds no `ref:` key, so every count stayed put and an unguarded fetch of an unvalidated ref shipped green. The binding is now sanctioned only inside a guard step, and the mention count is `-eq` rather than `-ge` (still a property — it is nrefs*2, so a third guarded job moves both sides) so a spare mention cannot ride along unexamined. - EMIT SCAN, PER STATEMENT. The whitelist cleared a whole LINE on its prefix, so `if [[ -n "$WORKFLOWS_REF" ]]; then echo "$WORKFLOWS_REF" >> "$GITHUB_STEP_SUMMARY"; fi` opened with a sanctioned `if [[ ` and cleared in full, raw emit and all. Lines are split on `;` and each statement judged alone. - EMIT SCAN, BOTH VALUES. It covered only `WORKFLOWS_REF`, though the guard's own comment insists the runner-supplied SHA gets identical treatment because it is only ever tested for emptiness. Echoing raw `$JOB_WORKFLOW_SHA` into an annotation would have stayed green. Now covered, plus a coverage self-check so a renamed env key fails loudly instead of passing vacuously, and the `${safe_job_sha}` annotation assertion its sibling already had. Each of the three is mutation-tested: injecting the bypass turns the suite red. - DOCS: `github.job_workflow_sha` also resolves in the CALLER's `with:`, so `workflows_ref: ${{ github.job_workflow_sha }}` equals itself by construction and passes both axes forever while `uses:` sits on `@v1`. Worse than the tag-pinned case already documented, because the passthrough never needs repinning and so never goes red. The guard cannot see it — the value arrives identical either way — so it is named in the header, the input description and the README, with the reviewer instruction it implies: require a LITERAL 40-hex SHA in the caller's `with:`, never an expression. Guard body unchanged; the two copies stay byte-identical. --- .github/workflows/pr-risk.yml | 22 +++++- README.md | 2 +- scripts/pr-risk/tests/test_pin_contract.sh | 79 ++++++++++++++++------ 3 files changed, 81 insertions(+), 22 deletions(-) diff --git a/.github/workflows/pr-risk.yml b/.github/workflows/pr-risk.yml index e755b74..01526f8 100644 --- a/.github/workflows/pr-risk.yml +++ b/.github/workflows/pr-risk.yml @@ -28,8 +28,12 @@ name: PR Risk Grade (reusable) # `uses:` itself at a fork commit is already running the fork's copy of this very file. Nor does # it prove the caller SHA-pinned `uses:` at all: the value arrives already resolved, so a # `uses: ...@v1` caller passing the SHA that tag points at satisfies both axes (it just goes red -# the next time `v1` is force-moved). Both cases are bounded by review of the caller on its base -# branch, not by any check this workflow can make. Call this workflow DIRECTLY — a nested +# the next time `v1` is force-moved) — and a caller that passes the `github.job_workflow_sha` +# CONTEXT here rather than a written-out SHA satisfies them permanently, since the passthrough +# re-resolves on every run and so never needs repinning. That is the shape to look for when +# reviewing a caller: `workflows_ref` must be a literal 40-hex SHA in the caller's `with:`, equal +# to the one in `uses:`, never an expression. Both cases are bounded by review of the caller on +# its base branch, not by any check this workflow can make. Call this workflow DIRECTLY — a nested # `workflow_call` chain through an org wrapper is unsupported, because `job_workflow_sha` names # one commit and the wrapper's is not this file's; see the `workflows_ref` input for the detail. # A consumer repo sharpens the generic defaults by committing @@ -313,6 +317,20 @@ on: pin-validation consumer CI runs). + NOR CAN IT SEE THAT THIS VALUE WAS WRITTEN OUT. `job_workflow_sha` + also resolves in the CALLER's own `with:` block, so a caller can hand + this input that context expression instead of a SHA. It then equals + itself by construction and passes both axes on every run, forever, + while `uses:` sits on `@v1` or `@main` — the value that arrives here + is identical either way, so the guard has nothing to distinguish. This + is strictly worse than the tag-pinned case below, because the + passthrough never needs repinning and so never produces the loud + failure that would otherwise surface the drift. WHEN REVIEWING A + CALLER, require a LITERAL 40-hex SHA in `with: workflows_ref:`, + character-for-character the one in `uses:` — an expression of any + kind, and this whole contract is decoration. + + What a floating `uses:` does buy is a LOUD failure on the next sanctioned `git tag -f v1 ` move: the tag resolves somewhere new, this hand-written pin does not follow, and every tag-pinned caller goes diff --git a/README.md b/README.md index 8d1ef66..fe95ebc 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. Label text is remappable via `label_map`. `workflows_ref` is **required** and **ENFORCED** — pin it to the same full commit SHA as `uses:`, so the grader cannot be loaded from a floating ref after the caller was reviewed. Every job that checks it out fails the run *before* the tool checkout unless the value is a full 40-hex commit SHA **and** equals `github.job_workflow_sha`, the commit `uses:` resolved to: a branch, a tag or a `refs/pull/N/head` is rejected by the first test, and a well-shaped SHA that is not the revision already running — a stale pin left behind when `uses:` moved, or a fork-authored commit of this public repo handed to `workflows_ref` while `uses:` stayed upstream — by the second. Scoped to exactly that: it stops the two halves of the tool disagreeing. It does not (and from inside the called workflow cannot) prove the running revision is upstream, nor that `uses:` was SHA-pinned rather than tag-pinned — the value arrives already resolved, so a tag-pinned caller passing the SHA the tag points at satisfies both axes and simply goes red the next time that tag is force-moved; reviewing the caller's `uses:` line is what bounds both. The guard also runs *before* enablement is resolved (the resolver is itself loaded from `workflows_ref`), so a drifted 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. Label text is remappable via `label_map`. `workflows_ref` is **required** and **ENFORCED** — pin it to the same full commit SHA as `uses:`, so the grader cannot be loaded from a floating ref after the caller was reviewed. Every job that checks it out fails the run *before* the tool checkout unless the value is a full 40-hex commit SHA **and** equals `github.job_workflow_sha`, the commit `uses:` resolved to: a branch, a tag or a `refs/pull/N/head` is rejected by the first test, and a well-shaped SHA that is not the revision already running — a stale pin left behind when `uses:` moved, or a fork-authored commit of this public repo handed to `workflows_ref` while `uses:` stayed upstream — by the second. Scoped to exactly that: it stops the two halves of the tool disagreeing. It does not (and from inside the called workflow cannot) prove the running revision is upstream, nor that `uses:` was SHA-pinned rather than tag-pinned — the value arrives already resolved, so a tag-pinned caller passing the SHA the tag points at satisfies both axes and simply goes red the next time that tag is force-moved — and a caller that passes the `github.job_workflow_sha` *context expression* here instead of a written-out SHA equals itself by construction and passes forever, never repinning and so never going red at all. **When reviewing a caller, require a literal 40-hex SHA in `with: workflows_ref:`, character-for-character the one in `uses:`** — reviewing that line is what bounds all of these. The guard also runs *before* enablement is resolved (the resolver is itself loaded from `workflows_ref`), so a drifted 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. | [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/scripts/pr-risk/tests/test_pin_contract.sh b/scripts/pr-risk/tests/test_pin_contract.sh index 7b90242..159b466 100644 --- a/scripts/pr-risk/tests/test_pin_contract.sh +++ b/scripts/pr-risk/tests/test_pin_contract.sh @@ -220,7 +220,7 @@ no "the protected checkout is not softened by continue-on-error" "$protected" "c jobsoft="$(grep -nE '^ continue-on-error:' "$WF" | tr '\n' ' ' | sed 's/ $//')" eq "no job demotes its own failures wholesale (no job-level continue-on-error)" "" "$jobsoft" -# --- the input itself is never interpolated into the shell nor emitted raw -------------------- +# --- neither raw value is interpolated into the shell nor emitted raw ------------------------- # `${{ inputs.workflows_ref }}` inline in `run:` would be a shell-injection vector, and emitting # the raw value lets a multi-line value forge workflow commands in a public log — the runner # re-parses ANY line of step output, so this is not only about `echo` and not only about lines @@ -228,6 +228,13 @@ eq "no job demotes its own failures wholesale (no job-level continue-on-error)" # it (a `[[ ]]` test, or the assignment that sanitizes it), never one that emits it: no bare # `echo`/`printf`, no workflow command, no redirect into an Actions file, and no continuation of # a line that was doing one of those. +# +# BOTH values, not just the caller's. `JOB_WORKFLOW_SHA` is runner-supplied and so cannot be +# forged by a caller — but the guard's own comment argues the platform could reshape or rename +# that property out from under it, which is exactly why the guard never tests it for shape and +# does sanitize it before echoing. A scan that covered only `WORKFLOWS_REF` would stay green +# while a future edit echoed raw `$JOB_WORKFLOW_SHA` into an annotation, contradicting the +# comment it sits under. Same whitelist, one more sanctioned assignment. script="$(printf '%s\n' "$code" | sed -n '/run: |/,$p')" # Unlike every other anchor here, this one had no coverage self-check: reshape the block scalar # (`run: >-`, or a `run:` with a trailing comment) and `$script` comes back EMPTY, at which point @@ -241,24 +248,44 @@ has "the run: block scan found the guard's script body" "$script" 'set -euo pipe # sees nothing) and an open-ended tail of spellings to keep chasing — `export`, `local`, `read`, # a herestring, a here-doc. Inverting it is what lets the assertion's name be true: any new way # of touching the value is reported until someone widens this list on purpose. +# +# PER STATEMENT, not per line. Sanctioning a whole LINE on its prefix hands back everything the +# whitelist just bought: `if [[ -n "$WORKFLOWS_REF" ]]; then echo "$WORKFLOWS_REF" >> \ +# "$GITHUB_STEP_SUMMARY"; fi` opens with a sanctioned `if [[ ` and would clear in full, raw emit +# and all. So the line is split on `;` first and each statement judged on its own — the shell's +# own separator, so a leak has to hide inside a command substitution (where it is captured, not +# logged) rather than merely after a semicolon. A `;` inside a quoted string over-splits, which +# costs a false positive, never a false negative: the trade this file makes everywhere. emitted="$(printf '%s\n' "$script" | awk ' - { line = $0; sub(/^[[:space:]]+/, "", line) } - line ~ /\$\{?WORKFLOWS_REF/ { - sanctioned = (!cont && (line ~ /^safe_ref=\$\(printf/ || line ~ /^(el)?if \[\[ /)) - if (!sanctioned) { - if (cont) { print "CONTINUATION:" NR } - else if (line ~ /::/) { print "WORKFLOW-COMMAND:" NR } - else if (line ~ /GITHUB_(STEP_SUMMARY|OUTPUT|ENV|PATH)/) { print "ACTIONS-FILE:" NR } - else if (line ~ /^(echo|printf)[[:space:]]/) { print "BARE-EMIT:" NR } - else if (line ~ /[^0-9a-zA-Z_]>>?[[:space:]]*[\$\/"]/) { print "REDIRECT:" NR } - else { print "UNSANCTIONED:" NR } + { + raw = $0 + n = split(raw, stmt, ";") + for (i = 1; i <= n; i++) { + line = stmt[i]; sub(/^[[:space:]]+/, "", line); sub(/[[:space:]]+$/, "", line) + if (line !~ /\$\{?(WORKFLOWS_REF|JOB_WORKFLOW_SHA)/) continue + sanctioned = (!cont && (line ~ /^safe_(ref|job_sha)=\$\(printf/ || line ~ /^(el)?if \[\[ /)) + if (!sanctioned) { + if (cont) { print "CONTINUATION:" NR } + else if (line ~ /::/) { print "WORKFLOW-COMMAND:" NR } + else if (line ~ /GITHUB_(STEP_SUMMARY|OUTPUT|ENV|PATH)/) { print "ACTIONS-FILE:" NR } + else if (line ~ /^(echo|printf)[[:space:]]/) { print "BARE-EMIT:" NR } + else if (line ~ /[^0-9a-zA-Z_]>>?[[:space:]]*[\$\/"]/) { print "REDIRECT:" NR } + else { print "UNSANCTIONED:" NR } + } } + cont = (raw ~ /\\$/) } - { cont = (line ~ /\\$/) } ' | tr '\n' ' ' | sed 's/ $//')" -eq "the raw value is only sanitized or tested, never emitted or copied elsewhere" "" "$emitted" +eq "neither raw value is emitted or copied elsewhere, only sanitized or tested" "" "$emitted" +# Coverage self-check for the scan above, in the same spirit as the ones on the other anchors: if +# the trigger pattern matched nothing at all — a renamed env key, a reshaped script — the `eq` +# passes vacuously over an empty scan and reports a green boundary that was never inspected. +ntouch=$(printf '%s\n' "$script" | grep -c '\${\?\(WORKFLOWS_REF\|JOB_WORKFLOW_SHA\)') +if [ "$ntouch" -ge 5 ]; then ok "the emit scan saw both guarded values ($ntouch lines)" +else bad "the emit scan saw both guarded values" "$ntouch lines — anchors are stale, coverage is vacuous"; fi no "the input reaches the script only via env:" "$script" '${{' -has "the sanitized value is what the annotations use" "$code" '${safe_ref}' +has "the sanitized input is what the annotations use" "$code" '${safe_ref}' +has "the sanitized runner SHA is what the annotations use" "$code" '${safe_job_sha}' # --- ...and the input is never aliased out from under the checkout scan ------------------------ # The "every workflows_ref checkout sits behind the guard" scan at the top matches literal `ref:` @@ -270,16 +297,31 @@ has "the sanitized value is what the annotations use" "$code" '${safe_ref}' # is a grep. So: OUTSIDE comments, `inputs.workflows_ref` may appear only as the guard's own env # binding or as a `ref:` key — the two shapes the scans above actually understand. A future job # that legitimately needs it elsewhere adds the shape here, deliberately, rather than by accident. -aliased="$(awk ' +# +# WHICH STEP OWNS THE BINDING MATTERS. Sanctioning the env-binding SHAPE anywhere in the file +# leaves the hole open one step to the right: a later step that binds `WORKFLOWS_REF: ${{ +# inputs.workflows_ref }}` of its own and then runs `git fetch origin "$WORKFLOWS_REF"` adds no +# `ref:` key, so `REFS` and the guard count both stay put, the binding clears on shape, and an +# unguarded fetch of an unvalidated ref ships green — the exact bypass this scan exists to close. +# So the binding is sanctioned only INSIDE a guard step: step ownership is tracked by resetting +# at every step key (`- ` at step indent) and every job key, and set only by the guard's own +# `- name:` line, which the byte-identity check above already pins. +aliased="$(awk -v guard="$GUARD_NAME" ' + $0 == guard { inguard = 1; next } + /^ - / { inguard = 0 } + /^ [A-Za-z0-9_-]+:/ { inguard = 0 } { line = $0; sub(/^[[:space:]]*/, "", line) } line ~ /^#/ { next } line !~ /inputs\.workflows_ref/ { next } { squashed = line; gsub(/[[:space:]]/, "", squashed) } - squashed ~ /^WORKFLOWS_REF:\$\{\{inputs\.workflows_ref\}\}$/ { next } + inguard && squashed ~ /^WORKFLOWS_REF:\$\{\{inputs\.workflows_ref\}\}$/ { next } squashed ~ /^ref:/ { next } { print "ALIASED:" NR } ' "$WF" | tr '\n' ' ' | sed 's/ $//')" -eq "the input is referenced only as the guard's env binding or a ref: key" "" "$aliased" +eq "the input is referenced only as the guard step's own env binding or a ref: key" "" "$aliased" +# EXACTLY two mentions per guarded job, not "at least". `-ge` let a spare mention ride along +# unexamined; the assertion's own name claims one binding and one `ref:` apiece, so it is an +# equality. Still a property rather than a literal: a third guarded job moves both sides at once. nmentions=$(awk ' { line = $0; sub(/^[[:space:]]*/, "", line) } line ~ /^#/ { next } @@ -287,8 +329,7 @@ nmentions=$(awk ' END { print n+0 } ' "$WF") want=$((nrefs * 2)) -if [ "$nmentions" -ge "$want" ]; then ok "the alias scan saw one env binding and one ref: per guarded job ($nmentions)" -else bad "the alias scan saw one env binding and one ref: per guarded job" "$nmentions, expected >= $want — anchors are stale, coverage is vacuous"; fi +eq "the alias scan saw one env binding and one ref: per guarded job ($nmentions)" "$want" "$nmentions" printf '\n%s passed, %s failed\n' "$PASS" "$FAIL" [ "$FAIL" -eq 0 ] From 4efc5e7b4796324a283a287ea691e7fbe834cb47 Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Tue, 4 Aug 2026 07:54:10 -0700 Subject: [PATCH 7/8] test(pr-risk): use ERE for the emit-scan coverage grep (BE-6307) `\|` and `\?` in a BRE are GNU extensions; `grep -cE` says the same thing portably and reads as the ERE it already was. --- scripts/pr-risk/tests/test_pin_contract.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/pr-risk/tests/test_pin_contract.sh b/scripts/pr-risk/tests/test_pin_contract.sh index 159b466..7d594d7 100644 --- a/scripts/pr-risk/tests/test_pin_contract.sh +++ b/scripts/pr-risk/tests/test_pin_contract.sh @@ -280,7 +280,7 @@ eq "neither raw value is emitted or copied elsewhere, only sanitized or tested" # Coverage self-check for the scan above, in the same spirit as the ones on the other anchors: if # the trigger pattern matched nothing at all — a renamed env key, a reshaped script — the `eq` # passes vacuously over an empty scan and reports a green boundary that was never inspected. -ntouch=$(printf '%s\n' "$script" | grep -c '\${\?\(WORKFLOWS_REF\|JOB_WORKFLOW_SHA\)') +ntouch=$(printf '%s\n' "$script" | grep -cE '\$\{?(WORKFLOWS_REF|JOB_WORKFLOW_SHA)') if [ "$ntouch" -ge 5 ]; then ok "the emit scan saw both guarded values ($ntouch lines)" else bad "the emit scan saw both guarded values" "$ntouch lines — anchors are stale, coverage is vacuous"; fi no "the input reaches the script only via env:" "$script" '${{' From 936e2752d611d7f6d58a34a623c47dc3b10b072e Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Tue, 4 Aug 2026 08:27:14 -0700 Subject: [PATCH 8/8] =?UTF-8?q?fix(pr-risk):=20drop=20the=20lock-step=20ax?= =?UTF-8?q?is=20=E2=80=94=20github.job=5Fworkflow=5Fsha=20does=20not=20exi?= =?UTF-8?q?st=20(BE-6307)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The lock-step half of the pin guard read `${{ github.job_workflow_sha }}`, which is not a property of the `github` context — it exists only as an OIDC token claim. It therefore expanded to the empty string, and the fail-closed `-z` branch added right below it would have aborted EVERY invocation, correctly pinned callers included, before the tool checkout. `actionlint` rejects the expression outright ("property \"job_workflow_sha\" is not defined in object type ...") and GitHub's context reference lists only `workflow_ref` / `workflow_sha`, neither of which names the reusable workflow's own commit. Reported by 2 of 8 review models. So the axis is removed rather than repaired: reading the claim would need an `id-token: write` grant from every caller plus a token exchange in a job that holds `permissions: {}`, which is a caller-contract change, not a fix. The header, the input description and the README now say what is actually enforced — shape, not provenance — and name review of the caller as the only thing that can bound which commit the ref points at. The surviving shape axis is tightened while it is in hand: a length test plus a "contains a character outside [0-9a-f]" glob replaces `[[ =~ ^[0-9a-fA-F]{40}$ ]]`, which drops the anchoring question entirely (whether ERE `$` also matches before a trailing newline is libc-dependent) and rejects uppercase hex, which the regex accepted and then handed to `actions/checkout` verbatim. test_pin_contract.sh now PINS the guard's executable body verbatim instead of characterizing it. Four findings this round were holes in the emit-scan whitelist — it cleared a whole `;`-statement on its prefix, its trigger was itself a blacklist, and a `has` needle proves a test is present, never that it is the only path to the checkout. An equality closes all of them at once: verified red against a permissive wrapper branch, an `&&`-chained raw emit, `printenv`, `set -euxo pipefail`, a warning-instead-of-error, a dropped `exit 1`, a loosened character class, a dropped length test, a raw value in the annotation, and an `if: false` on the step — and green against a pure rewording of the message, which is canonicalized out and separately checked to expand nothing but the sanitized copy. The `nrefs * 2` mention count becomes two per-category checks. --- .github/workflows/pr-risk.yml | 272 +++++++++------------ README.md | 2 +- scripts/pr-risk/tests/test_pin_contract.sh | 217 +++++++--------- 3 files changed, 202 insertions(+), 289 deletions(-) diff --git a/.github/workflows/pr-risk.yml b/.github/workflows/pr-risk.yml index 01526f8..946ef93 100644 --- a/.github/workflows/pr-risk.yml +++ b/.github/workflows/pr-risk.yml @@ -14,28 +14,25 @@ name: PR Risk Grade (reusable) # # The grader and its default risk map load from THIS repo at the pinned `workflows_ref`, # never from the graded PR's checkout (no PR code is checked out at all) — a PR cannot edit -# the rules that judge it. `workflows_ref` has deliberately no default and is now ENFORCED: every -# job that checks it out fails BEFORE the checkout unless the value is a full 40-hex commit SHA -# *and* equals `github.job_workflow_sha` — the commit `uses:` resolved to for that job. The first -# test rejects anything mutable (a branch, a tag, `refs/pull/N/head`); the second rejects a -# well-shaped SHA that is not the revision already running — a stale pin left behind when `uses:` -# moved, or a fork-authored commit of this PUBLIC repo handed to `workflows_ref` while `uses:` -# stayed upstream. So the two halves of the tool cannot disagree, and "pin it to the same SHA as -# `uses:`" is machine-checked rather than trusted prose in this header. +# the rules that judge it. `workflows_ref` has deliberately no default and its SHAPE is now +# ENFORCED: every job that checks it out fails BEFORE the checkout unless the value is a full +# 40-hex lowercase commit SHA. That rejects everything MUTABLE — a branch, a tag, +# `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 that does NOT prove — and cannot, from in here — is that the running revision is upstream -# code: `job_workflow_sha` is whatever the CALLER's own `uses:` resolved to, so a caller pointing -# `uses:` itself at a fork commit is already running the fork's copy of this very file. Nor does -# it prove the caller SHA-pinned `uses:` at all: the value arrives already resolved, so a -# `uses: ...@v1` caller passing the SHA that tag points at satisfies both axes (it just goes red -# the next time `v1` is force-moved) — and a caller that passes the `github.job_workflow_sha` -# CONTEXT here rather than a written-out SHA satisfies them permanently, since the passthrough -# re-resolves on every run and so never needs repinning. That is the shape to look for when -# reviewing a caller: `workflows_ref` must be a literal 40-hex SHA in the caller's `with:`, equal -# to the one in `uses:`, never an expression. Both cases are bounded by review of the caller on -# its base branch, not by any check this workflow can make. Call this workflow DIRECTLY — a nested -# `workflow_call` chain through an org wrapper is unsupported, because `job_workflow_sha` names -# one commit and the wrapper's is not this file's; see the `workflows_ref` input for the detail. +# 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 +# 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 # `.github/risk.json` (map) / `.github/risk-runbooks.json` (runbook registry), which are read # from the PR's BASE ref: present-but-invalid fails the run loudly; absent falls back to the @@ -173,10 +170,11 @@ name: PR Risk Grade (reusable) # statuses: read # uses: Comfy-Org/github-workflows/.github/workflows/pr-risk.yml@ # v1 # with: -# # ENFORCED, not merely asked for: it must be a full 40-hex commit SHA *and* the same -# # commit the `uses:` above resolves to, or the run fails before the tool checkout. A -# # branch, a tag, `refs/pull/N/head`, or a SHA left behind when `uses:` moved is -# # rejected — see the paragraph about the pinned ref at the top of this header for why. +# # ENFORCED, not merely asked for: it must be a full 40-hex LOWERCASE commit SHA or the +# # run fails before the tool checkout — a branch, a tag or `refs/pull/N/head` is rejected. +# # Write out the SAME SHA as the `uses:` above, LITERALLY: that the two agree is what +# # review of this caller checks, and it is not something the workflow can see. See the +# # paragraph about the pinned ref at the top of this header for why. # workflows_ref: # # OFF BY DEFAULT. Enrolling and switching on are two decisions: land the caller, get # # it reviewed, then start grading. Either pin it on here, or leave this out and set @@ -294,58 +292,45 @@ on: is not a chain. - ENFORCED, on two axes, before the tool checkout: it must match - `^[0-9a-fA-F]{40}$` (branches, tags and `refs/pull/N/head` are mutable, - so they are rejected rather than documented against) AND equal - `github.job_workflow_sha`, the commit `uses:` resolved to. The second - test is what stops the two halves of the tool disagreeing — a stale pin - left behind when `uses:` moved, or a fork-authored commit of this - PUBLIC repo passed here while `uses:` stayed upstream, both being just - as well-shaped as any other SHA. It does not, and cannot, prove the - running revision is upstream: that is what reviewing the caller's - `uses:` line on its base branch is for. + ITS SHAPE IS ENFORCED before the tool checkout: exactly 40 characters, + every one of them in `[0-9a-f]`. Branches, tags and `refs/pull/N/head` + are mutable, so they are rejected rather than documented against; a + multi-line value, trailing whitespace and uppercase hex are rejected + with them, since this value is handed to `actions/checkout` verbatim. - WHAT IT CANNOT SEE is the caller's ORIGINAL `uses:` ref. - `job_workflow_sha` is already the RESOLVED commit, so a `uses: ...@v1` - caller that passes whatever `v1` currently points at satisfies both - axes: this guard does NOT reject a tag- or branch-pinned `uses:`, and - no check inside the called workflow can — proving that needs the ref - the caller wrote, which is never sent. Reviewing the caller's `uses:` - line on its base branch is what bounds it, and org policy already - requires a full commit SHA there (a floating one fails the - pin-validation consumer CI runs). + 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 + 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 + `permissions: {}`. Adding that is a caller-contract change and is + tracked separately — do not assume this input's provenance is checked. - NOR CAN IT SEE THAT THIS VALUE WAS WRITTEN OUT. `job_workflow_sha` - also resolves in the CALLER's own `with:` block, so a caller can hand - this input that context expression instead of a SHA. It then equals - itself by construction and passes both axes on every run, forever, - while `uses:` sits on `@v1` or `@main` — the value that arrives here - is identical either way, so the guard has nothing to distinguish. This - is strictly worse than the tag-pinned case below, because the - passthrough never needs repinning and so never produces the loud - failure that would otherwise surface the drift. WHEN REVIEWING A - CALLER, require a LITERAL 40-hex SHA in `with: workflows_ref:`, - character-for-character the one in `uses:` — an expression of any - kind, and this whole contract is decoration. - - - What a floating `uses:` does buy is a LOUD failure on the next - sanctioned `git tag -f v1 ` move: the tag resolves somewhere new, - this hand-written pin does not follow, and every tag-pinned caller goes - red until it is repinned. That is the intended direction of the - failure — better a red check than a tool silently swapped under a - reviewed caller — but it is a cost the tag-pinned caller pays, not a - rejection at the door. Pin `uses:` to a SHA and pass the same SHA here. + 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. CALL IT DIRECTLY — a nested `workflow_call` chain (caller → an org - wrapper workflow → this one) is NOT supported. `job_workflow_sha` names - ONE commit for the job, and a wrapper living in another repo (or at - another commit of this one) is a different commit than this file, so no - single `workflows_ref` value can satisfy both this guard and the - wrapper's own pin. Name `pr-risk.yml` in the caller's `uses:`. + wrapper workflow → this one) is NOT supported: the wrapper's own pin + and this input name different files, and nothing reconciles them. + Name `pr-risk.yml` in the caller's `uses:`. type: string required: true enabled: @@ -387,8 +372,8 @@ on: enrollment. The `workflows_ref` pin contract is enforced BEFORE this resolves and is not subject to it — the resolver script is itself loaded from `workflows_ref`, so there is no point in the run at which a - mismatched pin could be read as "switched off" without first checking - out the very ref under suspicion. A caller whose two pins have drifted + mutable pin could be read as "switched off" without first checking out + the very ref under suspicion. A caller pinned to a branch or a tag therefore fails red on every PR even with `{"enabled": false}` set. The fix is the one-line repin (or dropping the caller) — both PRs, but a caller that cannot say which revision of the tool it runs is not in a @@ -415,18 +400,21 @@ jobs: enabled: ${{ steps.resolve.outputs.enabled }} steps: # THE PIN CONTRACT IS ENFORCED, NOT DOCUMENTED. The ref below supplies the code that runs - # in this job, so it is checked on TWO axes before any checkout happens: - # 1. SHAPE — a mutable ref (branch, tag, `refs/pull/N/head`) means that code can change - # after the caller was reviewed. Only a full commit SHA is immutable. - # 2. LOCK-STEP — shape alone does not say WHICH commit it is. GitHub serves a fork PR's - # head objects from this upstream repo, so a fork-authored 40-hex SHA is a perfectly - # well-shaped ref. So the value must EQUAL `github.job_workflow_sha`, the commit - # `uses:` resolved to for this job — runner-supplied and unforgeable by any input. - # That is what turns "pin it to the same SHA as `uses:`" from prose into a check, and - # it is scoped to exactly that: the grader cannot come from a different revision than - # the one running. It does NOT prove the running revision is upstream — a caller whose - # own `uses:` points at a fork commit is already running that fork's copy of this file, - # which no check inside this file can reach. Reviewing the caller bounds that one. + # in this job, so its SHAPE is checked before any checkout happens: a mutable ref (branch, + # tag, `refs/pull/N/head`) means that code can change after the caller was reviewed, and + # only a full commit SHA is immutable. + # + # SHAPE IS THE ONLY AXIS THIS FILE CAN CHECK, so do not read it as more. It does not say + # WHICH commit the ref names: a fork of this PUBLIC repo shares its object store, so a + # fork-authored 40-hex SHA is a perfectly well-shaped ref, and a pin left behind when + # `uses:` moved is too. Both would be closed by "equal to the commit `uses:` resolved to + # for this job" — but the runner does not hand that commit to the workflow. + # `github.workflow_sha` is the CALLER's top-level workflow file; `job_workflow_sha`, which + # is the value that would answer it, is an OIDC token CLAIM, not a `github` context + # property (`actionlint` rejects `github.job_workflow_sha`, and reading the claim needs + # `id-token: write` from every caller plus an exchange in a job that holds `permissions: {}` + # today). Reviewing the caller is what bounds that, and it is the only thing that can. + # # Enforced in the WORKFLOW, not in a script: the scripts are what the ref loads, so a # script-side check would sit inside the blast radius it is meant to bound. EVERY job that # checks out `workflows_ref` re-asserts this itself — `grade` does not inherit its safety @@ -435,57 +423,38 @@ jobs: # any of this repo's code has executed anywhere. # # The value arrives via `env:` and is never interpolated into the script body — inline - # `${{ }}` of the very input being validated is a shell-injection vector. `[[ =~ ]]` rather - # than `grep -Eq '^[0-9a-fA-F]{40}$'` because grep anchors per LINE: a multi-line value - # carrying one SHA-shaped line would pass it and then be handed to checkout in full. - # The two copies of this step are byte-identical on purpose, and - # scripts/pr-risk/tests/test_pin_contract.sh fails the build if they drift apart or if a - # job grows a `workflows_ref` checkout without one. + # `${{ }}` of the very input being validated is a shell-injection vector. The two copies of + # this step are byte-identical on purpose, and scripts/pr-risk/tests/test_pin_contract.sh + # pins this step's executable body verbatim, fails the build if the copies drift apart, and + # fails it if a job grows a `workflows_ref` checkout without one. - name: Enforce workflows_ref pin contract env: WORKFLOWS_REF: ${{ inputs.workflows_ref }} - # The commit `uses:` actually resolved to for THIS job. The runner sets it from the - # call graph; no input, caller expression, or fork PR can forge it — which is what - # makes the lock-step comparison below a real check rather than a restatement of the - # same untrusted value. - JOB_WORKFLOW_SHA: ${{ github.job_workflow_sha }} run: | set -euo pipefail - # Never interpolate a raw value into a `::error::`. A multi-line value — exactly what - # the `[[ =~ ]]` choice exists to catch — would end the annotation at the first newline - # and leave the remainder to be re-parsed by the runner as workflow commands + # Never interpolate a raw value into a `::error::`. A multi-line value — exactly what the + # length/character test below exists to catch — would end the annotation at the first + # newline and leave the remainder to be re-parsed by the runner as workflow commands # (`::add-mask::`, `::stop-commands::`, a forged `::notice::`) in a PUBLIC log. Anything # outside the ref alphabet becomes `?` and the result is truncated, so what is echoed is - # always one bounded line. The RUNNER-supplied SHA gets the identical treatment, not - # because a caller can forge it, but because it is only ever tested for emptiness and - # never for shape — and the block below argues the platform could rename or reshape that - # property out from under this guard. Neither value is trusted to be one tidy line. + # always one bounded line. safe_ref=$(printf '%s' "$WORKFLOWS_REF" | tr -c 'A-Za-z0-9._/-' '?') safe_ref=${safe_ref:0:64} - safe_job_sha=$(printf '%s' "$JOB_WORKFLOW_SHA" | tr -c 'A-Za-z0-9._/-' '?') - safe_job_sha=${safe_job_sha:0:64} - if [[ ! "$WORKFLOWS_REF" =~ ^[0-9a-fA-F]{40}$ ]]; then - echo "::error::workflows_ref must be the FULL 40-hex 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 - # Shape proves the ref is IMMUTABLE; it proves nothing about WHICH commit it names. - # GitHub serves a fork PR's head objects from this upstream repo, so a fork-authored - # 40-hex SHA passes the regex and would be checked out into a job holding the caller's - # token. The tool is only the revision that was reviewed if it is the very commit this - # job is already running from, so that is what is asserted. + # LENGTH + CHARACTER CLASS, not `grep -Eq '^[0-9a-f]{40}$'` and not `[[ =~ ]]`: both raise + # an anchoring question this form does not have. `grep` anchors `^…$` per LINE, so a + # multi-line value carrying one SHA-shaped line passes it and is then handed to checkout + # in full; `[[ =~ ]]` anchors the whole string, but whether `$` may ALSO match just before + # a trailing newline is a libc-dependent detail no trust boundary should rest on. A length + # test plus "contains a character outside [0-9a-f]" has no anchors at all — a newline, a + # space or an uppercase letter is simply a character outside the class. # - # FAILS CLOSED. An unrecognized `github` context property evaluates to the empty string, - # so a rename or removal upstream would otherwise turn this whole boundary into a silent - # permanent no-op with no red check anywhere — the one failure mode a security guard - # must not have. The context is always populated for a `workflow_call` job and this - # workflow has no other trigger, so empty means the platform changed under us: stop, - # loudly, rather than check out a ref whose provenance cannot be established. - if [[ -z "$JOB_WORKFLOW_SHA" ]]; then - echo "::error::github.job_workflow_sha is empty, so workflows_ref cannot be checked for lock-step with the commit uses: resolved to. That context is always populated for a workflow_call job and this workflow has no other trigger, so this is a platform regression (or a renamed context property), not a supported mode — failing closed rather than checking out a ref whose provenance cannot be established." - exit 1 - fi - if [[ "${WORKFLOWS_REF,,}" != "${JOB_WORKFLOW_SHA,,}" ]]; then - echo "::error::workflows_ref (${safe_ref}) is not the commit uses: resolved to (${safe_job_sha}), so the grader would load from a different revision of Comfy-Org/github-workflows than the one running. Set BOTH to that same full commit SHA. If the uses: line calling this workflow is pinned to a TAG or a BRANCH, fix that first: it must be a full commit SHA (org policy — a floating uses: fails the pin-validation consumer CI runs anyway), and this input takes the same SHA." + # LOWERCASE ONLY, deliberately. The value is handed to `actions/checkout` VERBATIM, so + # accepting a spelling the fetch may not resolve would trade this step's clear message for + # an obscure failure one step later — the opposite of the point. Everything that emits a + # SHA (`git rev-parse`, `gh`, the REST API, the UI) emits lowercase, so a false reject + # here is loud and fixed by lowercasing. + if [[ ${#WORKFLOWS_REF} -ne 40 || "$WORKFLOWS_REF" == *[!0-9a-f]* ]]; then + 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 # The tool checkout is the same pinned-ref load the grade job does: the resolver is this @@ -580,48 +549,31 @@ jobs: - name: Enforce workflows_ref pin contract env: WORKFLOWS_REF: ${{ inputs.workflows_ref }} - # The commit `uses:` actually resolved to for THIS job. The runner sets it from the - # call graph; no input, caller expression, or fork PR can forge it — which is what - # makes the lock-step comparison below a real check rather than a restatement of the - # same untrusted value. - JOB_WORKFLOW_SHA: ${{ github.job_workflow_sha }} run: | set -euo pipefail - # Never interpolate a raw value into a `::error::`. A multi-line value — exactly what - # the `[[ =~ ]]` choice exists to catch — would end the annotation at the first newline - # and leave the remainder to be re-parsed by the runner as workflow commands + # Never interpolate a raw value into a `::error::`. A multi-line value — exactly what the + # length/character test below exists to catch — would end the annotation at the first + # newline and leave the remainder to be re-parsed by the runner as workflow commands # (`::add-mask::`, `::stop-commands::`, a forged `::notice::`) in a PUBLIC log. Anything # outside the ref alphabet becomes `?` and the result is truncated, so what is echoed is - # always one bounded line. The RUNNER-supplied SHA gets the identical treatment, not - # because a caller can forge it, but because it is only ever tested for emptiness and - # never for shape — and the block below argues the platform could rename or reshape that - # property out from under this guard. Neither value is trusted to be one tidy line. + # always one bounded line. safe_ref=$(printf '%s' "$WORKFLOWS_REF" | tr -c 'A-Za-z0-9._/-' '?') safe_ref=${safe_ref:0:64} - safe_job_sha=$(printf '%s' "$JOB_WORKFLOW_SHA" | tr -c 'A-Za-z0-9._/-' '?') - safe_job_sha=${safe_job_sha:0:64} - if [[ ! "$WORKFLOWS_REF" =~ ^[0-9a-fA-F]{40}$ ]]; then - echo "::error::workflows_ref must be the FULL 40-hex 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 - # Shape proves the ref is IMMUTABLE; it proves nothing about WHICH commit it names. - # GitHub serves a fork PR's head objects from this upstream repo, so a fork-authored - # 40-hex SHA passes the regex and would be checked out into a job holding the caller's - # token. The tool is only the revision that was reviewed if it is the very commit this - # job is already running from, so that is what is asserted. + # LENGTH + CHARACTER CLASS, not `grep -Eq '^[0-9a-f]{40}$'` and not `[[ =~ ]]`: both raise + # an anchoring question this form does not have. `grep` anchors `^…$` per LINE, so a + # multi-line value carrying one SHA-shaped line passes it and is then handed to checkout + # in full; `[[ =~ ]]` anchors the whole string, but whether `$` may ALSO match just before + # a trailing newline is a libc-dependent detail no trust boundary should rest on. A length + # test plus "contains a character outside [0-9a-f]" has no anchors at all — a newline, a + # space or an uppercase letter is simply a character outside the class. # - # FAILS CLOSED. An unrecognized `github` context property evaluates to the empty string, - # so a rename or removal upstream would otherwise turn this whole boundary into a silent - # permanent no-op with no red check anywhere — the one failure mode a security guard - # must not have. The context is always populated for a `workflow_call` job and this - # workflow has no other trigger, so empty means the platform changed under us: stop, - # loudly, rather than check out a ref whose provenance cannot be established. - if [[ -z "$JOB_WORKFLOW_SHA" ]]; then - echo "::error::github.job_workflow_sha is empty, so workflows_ref cannot be checked for lock-step with the commit uses: resolved to. That context is always populated for a workflow_call job and this workflow has no other trigger, so this is a platform regression (or a renamed context property), not a supported mode — failing closed rather than checking out a ref whose provenance cannot be established." - exit 1 - fi - if [[ "${WORKFLOWS_REF,,}" != "${JOB_WORKFLOW_SHA,,}" ]]; then - echo "::error::workflows_ref (${safe_ref}) is not the commit uses: resolved to (${safe_job_sha}), so the grader would load from a different revision of Comfy-Org/github-workflows than the one running. Set BOTH to that same full commit SHA. If the uses: line calling this workflow is pinned to a TAG or a BRANCH, fix that first: it must be a full commit SHA (org policy — a floating uses: fails the pin-validation consumer CI runs anyway), and this input takes the same SHA." + # LOWERCASE ONLY, deliberately. The value is handed to `actions/checkout` VERBATIM, so + # accepting a spelling the fetch may not resolve would trade this step's clear message for + # an obscure failure one step later — the opposite of the point. Everything that emits a + # SHA (`git rev-parse`, `gh`, the REST API, the UI) emits lowercase, so a false reject + # here is loud and fixed by lowercasing. + if [[ ${#WORKFLOWS_REF} -ne 40 || "$WORKFLOWS_REF" == *[!0-9a-f]* ]]; then + 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 - name: Load pr-risk tool diff --git a/README.md b/README.md index fe95ebc..adb548e 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. Label text is remappable via `label_map`. `workflows_ref` is **required** and **ENFORCED** — pin it to the same full commit SHA as `uses:`, so the grader cannot be loaded from a floating ref after the caller was reviewed. Every job that checks it out fails the run *before* the tool checkout unless the value is a full 40-hex commit SHA **and** equals `github.job_workflow_sha`, the commit `uses:` resolved to: a branch, a tag or a `refs/pull/N/head` is rejected by the first test, and a well-shaped SHA that is not the revision already running — a stale pin left behind when `uses:` moved, or a fork-authored commit of this public repo handed to `workflows_ref` while `uses:` stayed upstream — by the second. Scoped to exactly that: it stops the two halves of the tool disagreeing. It does not (and from inside the called workflow cannot) prove the running revision is upstream, nor that `uses:` was SHA-pinned rather than tag-pinned — the value arrives already resolved, so a tag-pinned caller passing the SHA the tag points at satisfies both axes and simply goes red the next time that tag is force-moved — and a caller that passes the `github.job_workflow_sha` *context expression* here instead of a written-out SHA equals itself by construction and passes forever, never repinning and so never going red at all. **When reviewing a caller, require a literal 40-hex SHA in `with: workflows_ref:`, character-for-character the one in `uses:`** — reviewing that line is what bounds all of these. The guard also runs *before* enablement is resolved (the resolver is itself loaded from `workflows_ref`), so a drifted 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. 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) | | [`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. | [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/scripts/pr-risk/tests/test_pin_contract.sh b/scripts/pr-risk/tests/test_pin_contract.sh index 7d594d7..728c9a9 100644 --- a/scripts/pr-risk/tests/test_pin_contract.sh +++ b/scripts/pr-risk/tests/test_pin_contract.sh @@ -19,12 +19,11 @@ # visible symptom. # * THE GUARD STOPS BEING FIRST. It only bounds what it precedes — a guard after the checkout # it protects is decoration. -# * AN AXIS IS DROPPED, OR STOPS BEING FATAL. Shape alone proves the ref is immutable, not -# which commit it is; the `github.job_workflow_sha` comparison is what proves it is the -# revision already running. Either axis degraded to a warning is a silent no-op. -# * AN AXIS BECOMES A TAUTOLOGY. Rebind `WORKFLOWS_REF` to `${{ github.job_workflow_sha }}` -# and the lock-step test compares the resolved SHA with itself — always green, while the -# checkout below still uses the unvalidated input. Nothing about the guard's SHAPE changes. +# * THE CHECK IS WEAKENED, IN ANY OF THE WAYS A PATTERN SCAN CANNOT ENUMERATE. Loosen the +# character class, drop the length test, wrap the strict test in a permissive branch +# (`if <7-hex> then accept; else ; fi`), rebind `WORKFLOWS_REF` to +# something other than the input, emit the raw value in a new spelling (`printenv`, `env`, +# `declare -p`, `set -x`), or add a second emitting statement after a sanctioned prefix. # * THE STEP IS NEUTERED WHOLESALE. `continue-on-error: true` makes every `exit 1` advisory # and an `if:` switches the step off — added to both copies they stay byte-identical, every # assertion above still holds, and the checkout proceeds on an unvalidated ref. Or the @@ -33,20 +32,26 @@ # * THE INPUT IS ALIASED PAST THE SCAN. The unguarded-checkout scan matches `ref:` keys naming # `inputs.workflows_ref`. Bind it to an `env:` key first, forward it to a composite action, # or hand it to a `git fetch` in a `run:` step, and the scan has nothing left to see. -# * THE RAW VALUE IS EMITTED AGAIN. The runner re-parses any line of step output, so a -# multi-line ref can forge workflow commands in a public log — including one hop through -# another variable, which is why the emit scan whitelists what may touch the value rather -# than blacklisting the emit shapes someone thought of. # -# ASSERT PROPERTIES, NOT COUNTS, AND POSITIONS, NOT SUMS. An earlier draft pinned a literal -# number of `exit 1` lines, which would have gone red on the very next hardening of the guard — -# a test that blocks its own subject's improvement teaches people to delete the test. A later -# one compared total `::error::` and `exit 1` counts, which one path could satisfy on another's -# behalf. What is asserted here is that no rejection path can be non-fatal (no `exit 0` at all) -# and that each `::error::` is FOLLOWED by an exit. The awk passes also self-check that they -# matched anything, so a brittle anchor fails loudly rather than passing vacuously with zero -# coverage — and the patterns are deliberately over-inclusive, since a false positive here costs -# a puzzled minute and a false negative ships an unguarded checkout. +# THE GUARD'S EXECUTABLE BODY IS PINNED VERBATIM, and that is deliberate. Earlier drafts of this +# file tried to characterize the body instead — assert both axes are present, assert every +# `::error::` is followed by an `exit 1`, whitelist which statements may touch the value — and +# each round of review found another way through, because every one of them was a pattern over an +# open-ended language: the whitelist cleared a whole `;`-delimited statement on its PREFIX (so +# `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. +# +# 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 +# sanitized copy — otherwise a wording tweak would fail this test for no security reason, while +# `echo "::error::$WORKFLOWS_REF"` would slip past a prose-blind pin. Everything AROUND the body — +# which jobs have the guard, whether it precedes its checkout, whether the copies agree, whether +# either has been neutered — stays a property assertion, and each of those scans self-checks that +# it matched anything, so a stale anchor fails loudly rather than passing vacuously. # # bash tests/test_pin_contract.sh # exit 0 = all green set -uo pipefail @@ -59,8 +64,8 @@ PASS=0; FAIL=0 ok() { PASS=$((PASS+1)); printf 'ok %s\n' "$1"; } bad() { FAIL=$((FAIL+1)); printf 'FAIL %s\n got: %s\n' "$1" "${2:-}"; } eq() { if [ "$2" = "$3" ]; then ok "$1"; else bad "$1 (expected '$2')" "$3"; fi } -# `grep -F`, not `case` globbing: half these needles are regex text (`^[0-9a-fA-F]{40}$`), whose -# brackets and braces a glob would happily reinterpret into something laxer than it reads. +# `grep -F`, not `case` globbing: these needles are workflow and shell syntax (`${{`, `[[ `), +# whose brackets and braces a glob would happily reinterpret into something laxer than it reads. has() { if printf '%s\n' "$2" | grep -qF -- "$3"; then ok "$1"; else bad "$1" "not found: $3"; fi } no() { if printf '%s\n' "$2" | grep -qF -- "$3"; then bad "$1" "present: $3"; else ok "$1"; fi } @@ -142,50 +147,59 @@ else fi eq "all copies of the guard step are byte-identical" "" "$drift" -# --- both axes are still enforced, and no rejection path can be non-fatal --------------------- +# --- the guard's executable body is exactly this ----------------------------------------------- body="$(cat "$first" 2>/dev/null)" # WHOLE-LINE comments are dropped before any assertion about control flow, so an `exit 1` or an # `exit 0` quoted in prose neither satisfies nor breaks a check. Deliberately NOT a trailing-`#` # strip: `#` has no special meaning inside a shell string, and this repo routinely writes # `github-workflows#NN` and the like in exactly the annotation lines below — a naive # `s/[[:space:]]*#.*$//` would truncate such a line and could silently drop the `$WORKFLOWS_REF` -# mention that the emit scan exists to inspect. The residual cost runs the other, harmless way: -# a trailing comment that happens to say `exit 0` trips a check. False positive, one puzzled -# minute — the trade this file makes everywhere. +# mention the pin below exists to inspect. The residual cost runs the other, harmless way: a +# trailing comment that happens to say `exit 1` becomes part of the pinned text. False positive, +# one puzzled minute — the trade this file makes everywhere. code="$(printf '%s\n' "$body" | grep -v '^[[:space:]]*#')" -# The value under test must be the caller's INPUT, not a restatement of the runner's own. Rebind -# it to `${{ github.job_workflow_sha }}` and the lock-step comparison compares the resolved SHA -# with itself — a tautology that always passes while the checkout below still uses the -# unvalidated input, with every other assertion in this file staying green. -has "the guarded value is the caller's input, not the resolved SHA restated" \ - "$code" 'WORKFLOWS_REF: ${{ inputs.workflows_ref }}' -has "axis 1: the ref must be shaped like a full 40-hex commit SHA" \ - "$code" '^[0-9a-fA-F]{40}$' -has "axis 2: the runner-supplied resolved SHA is read into the guard" \ - "$code" 'JOB_WORKFLOW_SHA: ${{ github.job_workflow_sha }}' -has "axis 2: the pin is compared against it, case-insensitively" \ - "$code" '"${WORKFLOWS_REF,,}" != "${JOB_WORKFLOW_SHA,,}"' -has "axis 2: an unreadable job_workflow_sha is itself a rejection (fail closed)" \ - "$code" '[[ -z "$JOB_WORKFLOW_SHA" ]]' +# THE EQUALITY. Every property the scans in earlier drafts reached for — the input is the value +# under test and not a restatement of something else, the shape test is the ONLY path to the +# checkout rather than merely a present one, no branch accepts early, no statement emits the raw +# value under any spelling, `set -euo pipefail` has not grown an `x` — is a consequence of the +# body being exactly this and nothing else. See the header for why an equality rather than yet +# another pattern. To CHANGE the guard, change it here too: that second edit is the point. +expect="$(cat <<'PINNED' + - name: Enforce workflows_ref pin contract + env: + WORKFLOWS_REF: ${{ inputs.workflows_ref }} + run: | + set -euo pipefail + safe_ref=$(printf '%s' "$WORKFLOWS_REF" | tr -c 'A-Za-z0-9._/-' '?') + safe_ref=${safe_ref:0:64} + if [[ ${#WORKFLOWS_REF} -ne 40 || "$WORKFLOWS_REF" == *[!0-9a-f]* ]]; then + echo "::error::" + exit 1 + fi +PINNED +)" +# Blank lines are noise here (the slicer keeps them so the byte-identity check above sees them), +# and the annotation's PROSE is canonicalized to `` so a rewording is not a test failure. +actual="$(printf '%s\n' "$code" | grep -v '^[[:space:]]*$' | sed 's/\(::error::\).*/\1"/')" +if [ "$actual" = "$expect" ]; then + ok "the guard's executable body is exactly the pinned one" +else + bad "the guard's executable body is exactly the pinned one" \ + "$(diff <(printf '%s\n' "$expect") <(printf '%s\n' "$actual") | tr '\n' '~')" +fi -# Every rejection is fatal, expressed without pinning a literal count: nothing in the guard may -# exit successfully mid-way, and each error annotation must be followed by a failing exit. A new -# axis therefore extends this cleanly instead of turning it red. -no "no rejection path exits non-fatally" "$code" "exit 0" -# Positionally, not by totals. Equal SUMS would let one path degrade to log-and-continue so long -# as another gained a spare `exit 1` — which is exactly the regression this is here to catch, so -# each `::error::` must be followed by `exit 1` as the next non-blank line. -unpaired="$(printf '%s\n' "$code" | awk ' - /^[[:space:]]*$/ { next } - pending && $0 !~ /^[[:space:]]*exit 1[[:space:]]*$/ { print "UNPAIRED:" NR; pending = 0 } - { pending = /::error::/ } - END { if (pending) print "UNPAIRED:eof" } -' | tr '\n' ' ' | sed 's/ $//')" -eq "every ::error:: is followed by a failing exit" "" "$unpaired" -errors=$(printf '%s\n' "$code" | grep -c '::error::') -if [ "$errors" -ge 3 ]; then ok "all three rejection paths are present (shape, unreadable, mismatch)" -else bad "all three rejection paths are present (shape, unreadable, mismatch)" "$errors ::error:: lines"; fi +# The one thing the canonicalization above deliberately stops seeing. `` hides the +# annotation's text, so it must be checked here that the text expands NOTHING but the sanitized +# copy — otherwise `echo "::error::$WORKFLOWS_REF"` would read as a mere rewording, and a +# multi-line ref would forge workflow commands in a PUBLIC log exactly as before. +errline="$(printf '%s\n' "$code" | grep -F '::error::')" +stripped="${errline//\$\{safe_ref\}/}" +if [ -n "$errline" ] && [ "${stripped#*\$}" = "$stripped" ]; then + ok "the annotation expands nothing but the sanitized copy" +else + bad "the annotation expands nothing but the sanitized copy" "$errline" +fi # --- 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: @@ -220,72 +234,14 @@ no "the protected checkout is not softened by continue-on-error" "$protected" "c jobsoft="$(grep -nE '^ continue-on-error:' "$WF" | tr '\n' ' ' | sed 's/ $//')" eq "no job demotes its own failures wholesale (no job-level continue-on-error)" "" "$jobsoft" -# --- neither raw value is interpolated into the shell nor emitted raw ------------------------- -# `${{ inputs.workflows_ref }}` inline in `run:` would be a shell-injection vector, and emitting -# the raw value lets a multi-line value forge workflow commands in a public log — the runner -# re-parses ANY line of step output, so this is not only about `echo` and not only about lines -# that themselves contain `::`. Every line naming the value must therefore be one that consumes -# it (a `[[ ]]` test, or the assignment that sanitizes it), never one that emits it: no bare -# `echo`/`printf`, no workflow command, no redirect into an Actions file, and no continuation of -# a line that was doing one of those. -# -# BOTH values, not just the caller's. `JOB_WORKFLOW_SHA` is runner-supplied and so cannot be -# forged by a caller — but the guard's own comment argues the platform could reshape or rename -# that property out from under it, which is exactly why the guard never tests it for shape and -# does sanitize it before echoing. A scan that covered only `WORKFLOWS_REF` would stay green -# while a future edit echoed raw `$JOB_WORKFLOW_SHA` into an annotation, contradicting the -# comment it sits under. Same whitelist, one more sanctioned assignment. +# --- the input reaches the shell only through env: --------------------------------------------- +# `${{ inputs.workflows_ref }}` interpolated inline into a `run:` body would be a shell-injection +# vector — the runner substitutes the text BEFORE bash ever sees it, so quoting inside the script +# cannot help. The pinned body above already spells the binding out, but this states the rule by +# name so a failure says WHICH rule broke rather than just showing a diff. script="$(printf '%s\n' "$code" | sed -n '/run: |/,$p')" -# Unlike every other anchor here, this one had no coverage self-check: reshape the block scalar -# (`run: >-`, or a `run:` with a trailing comment) and `$script` comes back EMPTY, at which point -# both assertions below pass over nothing at all — the vacuous-pass mode this file's header -# claims to have eliminated. Anchor on a line the guard's script must contain. has "the run: block scan found the guard's script body" "$script" 'set -euo pipefail' -# WHITELIST, not blacklist. The named categories below are diagnostics — they say WHICH way a -# line leaks — but the verdict is the `else`: the raw value may be read by the sanitizing -# assignment and by the `[[ ]]` tests, and by NOTHING else. A blacklist of emit-shapes has a -# one-hop hole (`raw=$WORKFLOWS_REF` on one line, `echo "$raw"` on the next, and every shape rule -# sees nothing) and an open-ended tail of spellings to keep chasing — `export`, `local`, `read`, -# a herestring, a here-doc. Inverting it is what lets the assertion's name be true: any new way -# of touching the value is reported until someone widens this list on purpose. -# -# PER STATEMENT, not per line. Sanctioning a whole LINE on its prefix hands back everything the -# whitelist just bought: `if [[ -n "$WORKFLOWS_REF" ]]; then echo "$WORKFLOWS_REF" >> \ -# "$GITHUB_STEP_SUMMARY"; fi` opens with a sanctioned `if [[ ` and would clear in full, raw emit -# and all. So the line is split on `;` first and each statement judged on its own — the shell's -# own separator, so a leak has to hide inside a command substitution (where it is captured, not -# logged) rather than merely after a semicolon. A `;` inside a quoted string over-splits, which -# costs a false positive, never a false negative: the trade this file makes everywhere. -emitted="$(printf '%s\n' "$script" | awk ' - { - raw = $0 - n = split(raw, stmt, ";") - for (i = 1; i <= n; i++) { - line = stmt[i]; sub(/^[[:space:]]+/, "", line); sub(/[[:space:]]+$/, "", line) - if (line !~ /\$\{?(WORKFLOWS_REF|JOB_WORKFLOW_SHA)/) continue - sanctioned = (!cont && (line ~ /^safe_(ref|job_sha)=\$\(printf/ || line ~ /^(el)?if \[\[ /)) - if (!sanctioned) { - if (cont) { print "CONTINUATION:" NR } - else if (line ~ /::/) { print "WORKFLOW-COMMAND:" NR } - else if (line ~ /GITHUB_(STEP_SUMMARY|OUTPUT|ENV|PATH)/) { print "ACTIONS-FILE:" NR } - else if (line ~ /^(echo|printf)[[:space:]]/) { print "BARE-EMIT:" NR } - else if (line ~ /[^0-9a-zA-Z_]>>?[[:space:]]*[\$\/"]/) { print "REDIRECT:" NR } - else { print "UNSANCTIONED:" NR } - } - } - cont = (raw ~ /\\$/) - } -' | tr '\n' ' ' | sed 's/ $//')" -eq "neither raw value is emitted or copied elsewhere, only sanitized or tested" "" "$emitted" -# Coverage self-check for the scan above, in the same spirit as the ones on the other anchors: if -# the trigger pattern matched nothing at all — a renamed env key, a reshaped script — the `eq` -# passes vacuously over an empty scan and reports a green boundary that was never inspected. -ntouch=$(printf '%s\n' "$script" | grep -cE '\$\{?(WORKFLOWS_REF|JOB_WORKFLOW_SHA)') -if [ "$ntouch" -ge 5 ]; then ok "the emit scan saw both guarded values ($ntouch lines)" -else bad "the emit scan saw both guarded values" "$ntouch lines — anchors are stale, coverage is vacuous"; fi -no "the input reaches the script only via env:" "$script" '${{' -has "the sanitized input is what the annotations use" "$code" '${safe_ref}' -has "the sanitized runner SHA is what the annotations use" "$code" '${safe_job_sha}' +no "the input reaches the script only via env:" "$script" '${{' # --- ...and the input is never aliased out from under the checkout scan ------------------------ # The "every workflows_ref checkout sits behind the guard" scan at the top matches literal `ref:` @@ -319,17 +275,22 @@ aliased="$(awk -v guard="$GUARD_NAME" ' { print "ALIASED:" NR } ' "$WF" | tr '\n' ' ' | sed 's/ $//')" eq "the input is referenced only as the guard step's own env binding or a ref: key" "" "$aliased" -# EXACTLY two mentions per guarded job, not "at least". `-ge` let a spare mention ride along -# unexamined; the assertion's own name claims one binding and one `ref:` apiece, so it is an -# equality. Still a property rather than a literal: a third guarded job moves both sides at once. -nmentions=$(awk ' - { line = $0; sub(/^[[:space:]]*/, "", line) } +# Coverage self-check, PER CATEGORY rather than against a `nrefs * 2` total. The total encoded +# today's exact shape twice over — one binding AND one `ref:` per checkout — so a job that +# legitimately checked the tool out twice, or a consolidation that bound the input once for two +# steps, failed with the misleading "anchors are stale" message even though coverage was intact. +# What actually needs proving is that each scan matched at all: one binding per guard copy, and +# the `ref:` keys the top-of-file scan already counted. The alias scan above is what forbids +# anything else, so these two need not add up to the mentions. +nbind=$(grep -cE '^ *WORKFLOWS_REF: \$\{\{ inputs\.workflows_ref \}\}$' "$WF") +eq "each copy of the guard binds the input exactly once" "$guards" "$nbind" +nrefkeys=$(awk ' + { line = $0; gsub(/[[:space:]]/, "", line) } line ~ /^#/ { next } - line ~ /inputs\.workflows_ref/ { n += 1 } + line ~ /^ref:.*inputs\.workflows_ref/ { n += 1 } END { print n+0 } ' "$WF") -want=$((nrefs * 2)) -eq "the alias scan saw one env binding and one ref: per guarded job ($nmentions)" "$want" "$nmentions" +eq "every counted checkout names the input on its own ref: key" "$nrefs" "$nrefkeys" printf '\n%s passed, %s failed\n' "$PASS" "$FAIL" [ "$FAIL" -eq 0 ]