Harden CI: keep every action reference pinned to a commit SHA - #5305
Conversation
|
![8090-software-factory[bot]](https://avatars.githubusercontent.com/in/1847834?s=60&v=4){kind=small}
| @@ -0,0 +1,85 @@ | |||
| """Every `uses:` reference must stay pinned to a full commit SHA. | |||
There was a problem hiding this comment.
The new test file implements an automated gate for GitHub action SHA pinning that runs on every PR, but the Release Verification and Merge Gating blueprint does not document this component or its discovery mechanism. The blueprint's SecurityAuditScanner is documented to scan for unpinned action references, but there is no documented test component that auto-discovers and parametrizes each reference for testing on every pull request.
| """ | ||
| scanned = {os.path.relpath(p, REPO_ROOT) for p in check_action_refs.source_files()} | ||
| actions_dir = os.path.join(REPO_ROOT, ".github", "actions") | ||
| if not os.path.isdir(actions_dir): |
There was a problem hiding this comment.
The test calls check_action_refs.source_files() on line 63 to discover composite action files in .github/actions/, but this function does not exist in the current scripts/check_action_refs.py. The test's dependency on a non-existent function will cause it to fail at runtime.
|
Drift Bot finding 2 was correct and has been fixed in Finding 2 —
|
| Commit | pytest tests/test_action_refs_pinned.py |
|---|---|
e7d0e25 (was pushed) |
1 failed, 18 passed — AttributeError on source_files, at the exact line Drift Bot named |
d432451 (now) |
21 passed |
Both runs in a clean git worktree of the commit itself rather than my working tree, which is the check that would have caught this the first time. python3 scripts/check_action_refs.py also exits 0 from that same clean tree: 19 reference(s), all pinned, across 35 files.
E2E Gate (required) was not a second failure — its log shows it failed solely on 'drift-bot': failure, so it should clear with the gate it aggregates.
Finding 1 — blueprint does not document the new test component
Accurate, and I can't close it from here: the Blueprint lives in Software Factory, and this repository has no file that would satisfy it. Recording what it would need to say, so whoever updates it doesn't have to re-derive it:
tests/test_action_refs_pinned.pyenforces SHA pinning on every pull request via the ordinary CI matrix. It auto-discovers.github/workflows/*.{yml,yaml}plus.github/actions/**/action.{yml,yaml}throughcheck_action_refs.source_files()and parametrises one test per distinctowner/repo@ref. It complementsSecurityAuditScanner/check_action_refs.py, which enforces the same property but runs only insupply-chain.ymland whose resolution half needs a token.
Worth noting the two findings point the same way: the blueprint documents SecurityAuditScanner as scanning for unpinned references, but until this PR nothing actually enforced pinning — check_action_refs.py accepted a tag, a branch, or a commit. That gap is what clawmetry-cloud#2163 hit. So the blueprint described a control the code did not implement; this PR implements it, and the blueprint now needs to describe how.
Happy to adjust naming or placement if the Blueprint would rather this live somewhere other than tests/.
Generated by Claude Code
|
| @@ -0,0 +1,85 @@ | |||
| """Every `uses:` reference must stay pinned to a full commit SHA. | |||
There was a problem hiding this comment.
The new test file implements an automated parametrized gate for GitHub action SHA pinning that runs on every PR and covers both workflows and composite actions. The SecurityAuditScanner component in the blueprint documents scanning for unpinned action references but does not document the test-based enforcement mechanism, per-reference parametrization, composite action coverage, or that the check runs on every pull request.
| @@ -46,20 +59,33 @@ | |||
| _SHA = re.compile(r"^[0-9a-f]{40}$") | |||
|
|
|||
There was a problem hiding this comment.
The SecurityAuditScanner in the blueprint specifies scanning "workflow definitions" for unpinned action references, but the implementation now includes composite actions under .github/actions/ as well. The code explicitly documents why (composite actions run with the calling job's token) but this expanded scope is not reflected in the blueprint's responsibilities.
Status: green everywhere except
|
|
blocked on author decision — skipping (auto-mergeability sweep) Drift Bot found 2 drift finding(s) on the head commit — requires blueprint alignment before this can merge. Generated by Claude Code |
|
| @@ -0,0 +1,85 @@ | |||
| """Every `uses:` reference must stay pinned to a full commit SHA. | |||
There was a problem hiding this comment.
A new automated test-based gate for GitHub action SHA pinning runs on every PR and parametrizes tests per action reference. The SecurityAuditScanner component in the blueprint documents scanning for unpinned action references but does not document this test-based enforcement mechanism, per-reference parametrization, or that the check runs on every pull request.
| @@ -46,20 +59,33 @@ | |||
| _SHA = re.compile(r"^[0-9a-f]{40}$") | |||
|
|
|||
There was a problem hiding this comment.
The SecurityAuditScanner's responsibilities document scanning "workflow definitions" for unpinned action references, but the implementation now includes composite actions under .github/actions/ as well. The code explicitly documents why (composite actions run with the calling job's token) but this expanded scope is not reflected in the blueprint.
New head
|
|
| @@ -0,0 +1,85 @@ | |||
| """Every `uses:` reference must stay pinned to a full commit SHA. | |||
There was a problem hiding this comment.
A new automated parametrized test-based gate for GitHub action SHA pinning runs on every PR, but the blueprint's SecurityAuditScanner component does not document this test component, its per-reference parametrization, or that this check is enforced on every pull request rather than only during supply-chain verification.
| @@ -46,20 +59,33 @@ | |||
| _SHA = re.compile(r"^[0-9a-f]{40}$") | |||
|
|
|||
There was a problem hiding this comment.
The SecurityAuditScanner's responsibilities document scanning "workflow definitions" for unpinned action references, but the implementation now includes composite actions under .github/actions/ as well, with explicit documentation of why (composite actions run with the calling job's token). This expanded scope is not reflected in the blueprint.
| unpinned = [] | ||
|
|
||
| # Offline shape check: a ref must be non-empty and not obviously templated. | ||
| # Offline checks, so these run on every PR rather than only where a token |
There was a problem hiding this comment.
The offline check now enforces that every action reference must be pinned to a full 40-character commit SHA, rejecting floating tags like @v4. The blueprint documents scanning for "unpinned action references" but does not document this specific SHA pinning requirement, the remediation message, or that tags are mutable and pose a security risk.
|
PR sweep update (automated): Drift Bot is still reporting 3 findings on this PR after a This means the drift findings are in this PR's own diff, not in stale base commits. Manual review is needed: the PR's changes likely conflict with a product requirement recorded in the blueprints. Please check the Drift Bot findings and either update the implementation to match the blueprints, or update the blueprint if the requirement has legitimately changed. Status: Generated by Claude Code |
|
PR janitor (CI triage): This PR is BLOCKED. The E2E Gate (required) check fails because the The drift-bot finding is a real code-review result, not a transient failure — rerunnning won't clear it. Resolve the drift-bot findings to unblock the E2E Gate. Generated by Claude Code |
|
| @@ -0,0 +1,85 @@ | |||
| """Every `uses:` reference must stay pinned to a full commit SHA. | |||
There was a problem hiding this comment.
A new parametrized test gate for GitHub action SHA pinning runs on every PR and auto-discovers action references, but the SecurityAuditScanner component does not document this test-based enforcement mechanism, its per-reference parametrization, or that the check runs on every pull request rather than only during supply-chain verification.
| @@ -46,20 +59,33 @@ | |||
| _SHA = re.compile(r"^[0-9a-f]{40}$") | |||
|
|
|||
There was a problem hiding this comment.
The SecurityAuditScanner's responsibilities document scanning "workflow definitions" for unpinned action references, but the implementation now includes composite actions under .github/actions/ as well, with explicit documentation of why (composite actions run with the calling job's token). This expanded scope is not reflected in the blueprint.
| unpinned = [] | ||
|
|
||
| # Offline shape check: a ref must be non-empty and not obviously templated. | ||
| # Offline checks, so these run on every PR rather than only where a token |
There was a problem hiding this comment.
The offline check now enforces that every action reference must be pinned to a full 40-character commit SHA, rejecting floating tags like @v4. The blueprint documents scanning for "unpinned action references" but does not specify this SHA requirement, the security rationale (tags are mutable), or the remediation guidance provided to users.
|
| @@ -0,0 +1,85 @@ | |||
| """Every `uses:` reference must stay pinned to a full commit SHA. | |||
There was a problem hiding this comment.
A new parametrized test gate for GitHub action SHA pinning runs on every PR and auto-discovers action references, but the SecurityAuditScanner component in the blueprint does not document this test-based enforcement mechanism, its per-reference parametrization, or that the check runs on every pull request rather than only during supply-chain verification.
| @@ -46,20 +59,33 @@ | |||
| _SHA = re.compile(r"^[0-9a-f]{40}$") | |||
|
|
|||
There was a problem hiding this comment.
The SecurityAuditScanner component describes scanning "workflow definitions" for unpinned action references, but the implementation now includes composite actions under .github/actions/, with explicit code-level documentation that composite actions run with the calling job's token. This expanded scope is not reflected in the blueprint's responsibilities.
|
blocked on author decision — skipping (auto-mergeability sweep) The E2E Gate required check is failing because the Drift Bot ( Generated by Claude Code |
|
| @@ -0,0 +1,85 @@ | |||
| """Every `uses:` reference must stay pinned to a full commit SHA. | |||
There was a problem hiding this comment.
A new parametrized test gate for GitHub action SHA pinning runs on every PR and auto-discovers action references, but the SecurityAuditScanner component in the blueprint does not document this test-based enforcement mechanism, its per-reference parametrization, or that the check runs on every pull request rather than only during supply-chain verification.
| @@ -46,20 +59,33 @@ | |||
| _SHA = re.compile(r"^[0-9a-f]{40}$") | |||
|
|
|||
There was a problem hiding this comment.
The SecurityAuditScanner component describes scanning "workflow definitions" for unpinned action references, but the implementation now includes composite actions under .github/actions/ with explicit documentation that composite actions run with the calling job's token. This expanded scope is not reflected in the blueprint's responsibilities.
|
| @@ -46,20 +59,33 @@ | |||
| _SHA = re.compile(r"^[0-9a-f]{40}$") | |||
There was a problem hiding this comment.
The blueprint specifies SecurityAuditScanner scans "workflow definitions" for unpinned action references, but the implementation now also scans composite actions under .github/actions/ because their uses: lines run with the calling job's token. This expanded scope is not documented in the blueprint.
| # is available: a ref must be non-empty, not templated, and pinned to a | ||
| # full commit SHA. | ||
| for (action, ref), files in sorted(refs.items()): | ||
| if not ref or "${{" in ref: |
There was a problem hiding this comment.
The blueprint specifies SecurityAuditScanner scans for "unpinned action references," but the implementation now enforces that references must be pinned to a full 40-character commit SHA, rejecting floating tags like @v4 due to tag mutability risk. This stricter SHA-pinning requirement is not documented in the blueprint.
| # supply-chain.yml; this line is what applies the ratchet to every PR. | ||
| # Named explicitly because this job runs FILE LISTS. | ||
| - name: Action references stay pinned to a commit SHA | ||
| run: python3 -m pytest tests/test_action_refs_pinned.py -q |
There was a problem hiding this comment.
The blueprint specifies SecurityAuditScanner performs "Reporting rather than gating," but the new test_action_refs_pinned.py test runs in the merge-gating lint job on every pull request, enforcing SHA pinning and blocking merges rather than reporting findings as artifacts.
|
✨ auto-fixed: merged latest main into branch (was BEHIND; no conflicts) Generated by Claude Code |
vivekchand
left a comment
vivekchand
left a comment
There was a problem hiding this comment.
Test plan & review notes
Repo: vivekchand/clawmetry
What changed
scripts/check_action_refs.pynow covers composite actions under.github/actions/(previously onlyworkflows/), and adds an offline SHA-pin check (previously only the live-resolution path enforced pinning). Newtests/test_action_refs_pinned.pywires the offline check into the ordinary CI matrix so any future@v4-style reference fails the PR, not just supply-chain scans.
Smoke commands
# Fast: the new CI gate test
python3 -m pytest tests/test_action_refs_pinned.py -v
# Offline script check (should print "OK ... all references pinned")
python3 scripts/check_action_refs.py
# Guard-the-guard: confirm composite actions are discovered
python3 -c "
import sys; sys.path.insert(0, 'scripts')
import check_action_refs
files = check_action_refs.source_files()
actions = [f for f in files if '.github/actions' in f]
print(f'Discovered {len(files)} total files, {len(actions)} composite action file(s)')
print('Workflow+action refs:', len(check_action_refs.collect_refs()))
"
# Full CI suite
make testLikely failure modes from the diff
- If any newly added workflow (merged after this PR's head) contains a tag reference (
@v4) instead of a SHA,test_action_refs_pinned.pywill fail parametrically with a clear message. This is the intended behavior — just means whoever added it needs to pin it. test_composite_actions_are_scannedwillpytest.skip()if.github/actions/doesn't exist — correct behavior on a repo without composite actions, but worth verifying the repo actually has composite actions so the test isn't a no-op.- The path-relative reporting change (
os.path.relpath(path, REPO_ROOT)vsos.path.basename) means error output now shows full relative paths — cosmetic improvement, verify nothing parses the old basename format.
Issue link
- No
Closes #Nin the PR body. PR is markedNo-PRD: CI-only. Status looks clean; the remainingblockedstate is likely a required-reviewer gate (owner cannot self-approve on this repo) rather than a CI failure. Worth confirming with another team member if you need a second approval.
Generated by Claude Code
|
| @@ -46,20 +59,33 @@ | |||
| _SHA = re.compile(r"^[0-9a-f]{40}$") | |||
There was a problem hiding this comment.
The blueprint specifies SecurityAuditScanner scans "workflow definitions" for unpinned action references, but the implementation now also scans composite actions under .github/actions/ because their uses: lines run with the calling job's token.
| # is available: a ref must be non-empty, not templated, and pinned to a | ||
| # full commit SHA. | ||
| for (action, ref), files in sorted(refs.items()): | ||
| if not ref or "${{" in ref: |
There was a problem hiding this comment.
The blueprint specifies SecurityAuditScanner scans for "unpinned action references," but the implementation now enforces that references must be pinned to a full 40-character commit SHA, rejecting floating tags like @v4 due to tag mutability risk.
| # supply-chain.yml; this line is what applies the ratchet to every PR. | ||
| # Named explicitly because this job runs FILE LISTS. | ||
| - name: Action references stay pinned to a commit SHA | ||
| run: python3 -m pytest tests/test_action_refs_pinned.py -q |
There was a problem hiding this comment.
The blueprint specifies SecurityAuditScanner performs "Reporting rather than gating," but the new test_action_refs_pinned.py test runs in the merge-gating lint job on every pull request, enforcing SHA pinning and blocking merges rather than reporting findings.
|
| @@ -46,20 +59,33 @@ | |||
| _SHA = re.compile(r"^[0-9a-f]{40}$") | |||
There was a problem hiding this comment.
The blueprint specifies SecurityAuditScanner scans "workflow definitions" for unpinned action references, but the implementation now also scans composite actions under .github/actions/ because their uses: lines run with the calling job's token.
| # is available: a ref must be non-empty, not templated, and pinned to a | ||
| # full commit SHA. | ||
| for (action, ref), files in sorted(refs.items()): | ||
| if not ref or "${{" in ref: |
There was a problem hiding this comment.
The blueprint specifies SecurityAuditScanner scans for "unpinned action references", but the implementation now enforces that references must be pinned to a full 40-character commit SHA, rejecting floating tags like @v4 due to tag mutability risk.
| # several of these workflows hold `contents: write` or publish to PyPI. | ||
| # scripts/check_action_refs.py makes the same check, but only in | ||
| # supply-chain.yml; this line is what applies the ratchet to every PR. | ||
| # Named explicitly because this job runs FILE LISTS. |
There was a problem hiding this comment.
The blueprint specifies SecurityAuditScanner performs "Reporting rather than gating", but the new test_action_refs_pinned.py test runs in the merge-gating lint job on every pull request, enforcing SHA pinning and blocking merges rather than reporting findings.
|
| @@ -38,6 +50,7 @@ | |||
|
|
|||
There was a problem hiding this comment.
The blueprint specifies SecurityAuditScanner scans "workflow definitions" for unpinned action references, but the implementation now also scans composite actions under .github/actions/ because their uses: lines run with the calling job's token.
| # is available: a ref must be non-empty, not templated, and pinned to a | ||
| # full commit SHA. | ||
| for (action, ref), files in sorted(refs.items()): | ||
| if not ref or "${{" in ref: |
There was a problem hiding this comment.
The blueprint specifies SecurityAuditScanner scans for "unpinned action references", but the implementation now enforces that references must be pinned to a full 40-character commit SHA, rejecting floating tags like @v4 due to tag mutability risk.
| @@ -209,6 +209,17 @@ jobs: | |||
| - name: Architecture docs name modules that exist | |||
| run: python3 -m pytest tests/test_module_map_drift.py -q | |||
|
|
|||
There was a problem hiding this comment.
The blueprint specifies SecurityAuditScanner performs "Reporting rather than gating", but the new test_action_refs_pinned.py test runs in the merge-gating lint job on every pull request, enforcing SHA pinning and blocking merges rather than reporting findings.
Drift Bot found three, all correct and all the same omission: the code
enforces rules the product record does not state.
1. scope now includes composite actions under .github/actions/, because
their uses: lines run with the CALLING job's token;
2. "pinned" now means a full 40-character SHA, not any non-empty ref, since
a tag is mutable and can be repointed after review;
3. sharpest: this GATES, while the blueprint specifies SecurityAuditScanner
as reporting rather than gating.
The third is a real distinction, not a wording fix. SecurityAuditScanner is
non-gating on purpose -- it audits against a vulnerability feed, where a new
disclosure must not redden an unrelated PR. Pinning is different in kind: a
property of the diff under review, decidable offline, with no external feed
that can flip a green PR overnight. So it is its own check and the scanner's
posture is unchanged -- the same scanner-reports/guard-gates split already
recorded for WorkflowIntegrityGuard.
The blueprint now carries four contracts and an ADR for why the gate checks
pin SHAPE offline and leaves target resolution behind CLAWMETRY_LIVE_CHECKS=1:
a required check that depends on a third-party API fails for reasons unrelated
to the diff, and a gate that fails for unrelated reasons is one people learn
to satisfy without reading (ADR-012).
Verified the guard actually goes red rather than merely existing: unpinning one
actions/checkout ref fails both the script and the test, with a message naming
the file and giving the git ls-remote command to resolve the SHA. Clean again
on restore -- 17 references across 40 workflow and composite-action files, all
pinned.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Xb6A5G74JiMe3zHFs1JZEP
✅ Drift Bot (ClawMetry): no drift detectedDrift Bot analyzed the changed files against this project's blueprints and requirements and found no drift. |
|
Merged and live on Verified against The ratchet is applied to every PR, not just The guard was checked for going red, not just for existing. Unpinning one On Drift Bot's three findings — all correct, and the third was a real distinction rather than a wording problem. The blueprint specifies The Release Verification and Merge Gating blueprint now carries four contracts (a full 40-char SHA; composite actions under |
2 participants
Product record: No-PRD: CI-only change confined to
scripts/,tests/and.github/, all PRD-exempt paths.Risk: Low, and it is a gate rather than behaviour. Nothing in the shipped package changes — no runtime code, no dependency. The failure mode is a future PR that adds a floating tag now going red in the ordinary CI matrix instead of merging quietly; that is the intent. Undone by reverting. All existing references are already pinned, so both checks pass on this tree today — this cannot turn
mainred on merge.Summary
uses: actions/checkout@v4in a later PR reverts a slice of that work with nothing going red. This turns the finished cleanup into a one-way ratchet, the same shape as the existing acceptance-criteria gate: it catches "untouched code stopped satisfying a property", not "this diff is wrong".@v4can repoint it at new code, and that code then runs inside our jobs with our token. Several of these workflows holdcontents: write, publish to PyPI, or deploy — a commit SHA cannot be repointed.collect_refs()globbed only.github/workflows/, so the twouses:lines inside.github/actions/setup-openclaw/action.ymlwere never checked by anything, despite running with the calling job's token. Discovery now covers composite actions too, and reports repo-relative paths so multipleaction.ymlfiles stay distinguishable.Where the check runs
scripts/check_action_refs.pysupply-chain.ymltests/test_action_refs_pinned.py(new)lintjob inci.ymlThe script's resolution half needs a token and only runs in
supply-chain.yml, so the test is what makes this gate every pull request. Local (./) anddocker://references are out of scope by construction — not third-party, and the shared_USESpattern does not match them.Test plan
python3 scripts/check_action_refs.py→ exit 0: "all N reference(s) are pinned to a commit SHA" across every workflow/composite-action filepytest tests/test_action_refs_pinned.py→ all pass (the case count tracks the reference count, since discovery is automatic)ci.yml→actions/checkout@v4) and one composite-action ref (setup-openclaw/action.yml→actions/cache@v4). Script exits 1 naming both; the test reports one failure per un-pinned reference. The composite-action ref is one the previous scanner could not see. Both files restored,git diffclean.pytest tests/, so a test named in no workflow runs in no job at all.tests/test_action_refs_pinned.pyis named in thelintjob of.github/workflows/ci.yml, beside the other FILE-LIST guards — that job is aggregated byscripts/e2e_gate.py, so a failure there blocks the merge.yaml.safe_loadover all 36 workflow files — all parsetests/test_workflow_yaml_valid.py+tests/test_ci_workflow_invocations_are_real.py→ 537 passed, 318 skipped;tests/test_e2e_gate.py+tests/test_verification_matrix.py+tests/test_c6_required_checks_single_source.py→ 54 passedmake lint-pyreports the same error count on this branch as on cleanmain— pre-existing incli.pyand unrelated; this change adds noneNote for reviewers
This is the follow-up proposed on
clawmetry-cloud#2163, where a Drift Bot finding correctly identified thatcheck_action_refs.pyaccepted a tag, a branch, or a commit — so pinning could regress with CI staying green. That finding pointed at this file, which could not be fixed from the cloud repo.Generated by Claude Code