fix(pr-risk): enforce workflows_ref as a full 40-hex commit SHA before the tool checkout (BE-6307)

.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/.
@@ -151,6 +170,11 @@ name: PR Risk Grade (reusable)
 #         statuses: read
 #       uses: Comfy-Org/github-workflows/.github/workflows/pr-risk.yml@<sha>  # 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: <same sha>
 #         # 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 +290,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:
@@ -301,6 +366,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
@@ -322,6 +399,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
@@ -406,6 +541,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

.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