diff --git a/.github/workflows/pr-risk.yml b/.github/workflows/pr-risk.yml index 54160f7..b9357d8 100644 --- a/.github/workflows/pr-risk.yml +++ b/.github/workflows/pr-risk.yml @@ -14,7 +14,26 @@ 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 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 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 # defaults in scripts/pr-risk/. @@ -141,6 +160,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 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 @@ -256,6 +280,47 @@ 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. + + + 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. + + + 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. + + + 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: 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: @@ -279,6 +344,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 + 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 + state the variable is meant to cover. type: boolean required: false default: false @@ -300,6 +377,64 @@ 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, 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 + # 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. 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 }} + run: | + set -euo pipefail + # 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. + safe_ref=$(printf '%s' "$WORKFLOWS_REF" | tr -c 'A-Za-z0-9._/-' '?') + safe_ref=${safe_ref:0:64} + # 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. + # + # 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 # repo's code at `workflows_ref`, never the graded PR's. - name: Load pr-risk tool @@ -365,6 +500,41 @@ 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 + # 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. + safe_ref=$(printf '%s' "$WORKFLOWS_REF" | tr -c 'A-Za-z0-9._/-' '?') + safe_ref=${safe_ref:0:64} + # 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. + # + # 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 # 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/.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 3fa76c5..ee4af59 100644 --- a/README.md +++ b/README.md @@ -14,7 +14,7 @@ This repo is **public** so any repo — public or private, inside or outside the | [`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-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`. | | [`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-risk.yml`](.github/workflows/pr-risk.yml) | **Advisory PR risk grading (shadow check)** — **off by default** (`enabled: false`); 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.yml`](.github/workflows/pr-risk.yml) | **Advisory PR risk grading (shadow check)** — **off by default** (`enabled: false`); 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. | | [`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`. | | [`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. | | [`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). 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. | 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..728c9a9 --- /dev/null +++ b/scripts/pr-risk/tests/test_pin_contract.sh @@ -0,0 +1,296 @@ +#!/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 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. +# 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. +# * 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 +# 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 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 + +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 } +# `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 } + +GUARD_NAME=' - name: Enforce workflows_ref pin contract' + +# --- 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`. 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. 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 } + /^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" \ + "" "$(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") +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 + 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 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 { next } + /^[[:space:]]*$/ { print > f; next } + /^ / { 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" +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" + +# --- 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 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 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 + +# 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: +# `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" + +# --- 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 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')" +has "the run: block scan found the guard's script body" "$script" 'set -euo pipefail' +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:` +# 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. +# +# 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) } + 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 step's own env binding or a ref: key" "" "$aliased" +# 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 ~ /^ref:.*inputs\.workflows_ref/ { n += 1 } + END { print n+0 } +' "$WF") +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 ]