fix(actions-lock): stop advisory findings blocking the lock wrapper - #946
Merged
hyperpolymath merged 2 commits intoSep 22, 2026
Merged
Conversation
`verify_lock_coverage` accepts the known gh-actions-lock v0.1.6
reusable-workflow `stale` false positive, and on the `valid:true` path it
records advisory findings without blocking. The `valid:false` path did not:
every finding that was not the accepted false positive was counted as
blocking, so advisory `sha-as-ref` findings became fatal the moment one real
finding flipped `valid` to false.
Measured on metadatastician/burble 2026-09-22, which carries a genuine
job-level reusable-workflow ref that the released tool cannot see:
without the reusable lock entry -> valid=true, 7 sha-as-ref, wrapper rc 0
with the reusable lock entry -> valid=false, 7 sha-as-ref + 1 stale,
stale accepted, wrapper rc 1
The wrapper printed "Accepted reusable-workflow lock coverage" and failed
anyway. That held burble's workflow-safety job red and, because the step
fail-fasts, its secret-baseline positive/negative controls have been skipped
rather than run.
Discriminate by category, not severity. gh-actions-lock v0.1.6 labels every
finding `severity: warning`, `stale` included, so a severity-based exemption
would swallow real desyncs and pass vacuously. `sha-as-ref` is advisory
because the tool's own authoritative `valid` bit is unaffected by it.
Advisory findings still never explain a `valid:false` result on their own, so
the fail-closed guard is unchanged.
Tests: three cases added to scripts/tests/actions-lock-update-test.sh —
advisory alongside an accepted reusable false positive must pass; a real
stale must still block alongside advisories; advisories alone must still
fail closed. Suite is 12/12. Reverting this fix kills the first; a
severity-based exemption is killed by the existing reusable-acceptance test.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01X3hgXxWm6umMgZkjYyHnnm
Contributor
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (2)
✨ Finishing Touches📝 Generate docstrings
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
hyperpolymath
deleted the
fix/lock-wrapper-advisory-findings-not-blocking
branch
September 22, 2026 09:46
|
hyperpolymath
added a commit
that referenced
this pull request
Sep 22, 2026
…lint baseline, debt re-baseline (#954) Round 2 of the post-#899 fix-forward. #947 merged (7b05a32) but CI on its merge ref exposed 7 failures; this PR fixes the six that are code (the seventh, Scorecard Token-Permissions on propagate-hooks, needs an owner dismiss — the `contents: write` is required for the self-push to `hyperpolymath/standards`). ## What broke on #947 and why 1. **uses-lock**: #947 deleted `asana/push-signed-commits` from `actions.lock` to silence Hypatia's transitive finding — but `.github/actions/signed-push/action.yml:42` really uses it (the ref entered via #946's line, invisible on the PR branch alone). Deleting true lock metadata to satisfy a scanner is gaming; restored verbatim, finding acked honestly instead. 2. **Hypatia gate (latent)**: restoring the metadata re-arms Hypatia's `transitive_dependencies_missing` (asana floats `setup-python@v2` upstream — verified still floating on asana main 2026-09-22). Acked in the baseline (210 -> 211, #951), proven to match with positive + negative controls. 3. **Repo self-tests**: my validator change (empty scan -> valid) fixed the hypatia-gate suite but broke science-ci, which pins empty -> exit 2. The #741 control tested the pre-#771 slurp accident; fail-closed is the documented intent (comment predates #771, both suites now agree). Reverted validator, fixed the stale control. 4. **Validate Hypatia Baseline**: `governance-reusable.yml` validated the new baseline with main's OLD `apply-baseline.sh` (no `warn` severity) — exit 2 on a valid file. Self-lint preference: caller's own script when present, main-pinned fallback for consumers. 5. **Debt ratchet**: three breaches. `deno-residue` counted retirement-doc comments as residue — probe refined to non-comment matches (0, ceiling holds at 1). `gate-scripts` + `todo-fixme` counts had fossilized (runner only ratchets down; #820's own tree already measured 38/79) — re-baselined to measured 40/40 + 80/80 with per-entry declarations (#953). 6. **Exemption ratchet**: baseline 129 -> 210 grew without a trailer on #947. This PR's 210 -> 211 carries `Ratchet-exception: .hypatia-baseline.json`. ## Verified locally (tip worktree) gh verify `valid:true`, uses-lock 0 exceptions, both security suites green (24 gate PASSes), debt structure + run + both ratchets green, registry in sync, baseline ack matches both file variants with a failing negative control. Fixes the six code failures; closes #951 and #953 as implemented (paydown continues in #953). --------- Co-authored-by: hyperpolymath <hyperpolymath@users.noreply.github.com>
hyperpolymath
added a commit
that referenced
this pull request
Sep 22, 2026
…it past #946 (#962) ## What this fixes The lock gate is staged from a **third pin**. There are not two pins in this system, there are three: 1. the caller's `uses: hyperpolymath/standards/...@<sha>` ref, 2. `actions.lock`'s record of that ref, and 3. **a SHA hardcoded inside `governance-reusable.yml`** for its own `actions/checkout` of the lock-gate tooling. Bumping a caller cannot reach the third one. A *called* reusable workflow has no reliable context exposing its own commit (`github.workflow_sha` resolves to the **caller's**), which is what forces the hardcode in the first place. `standards#946` fixed `scripts/update-actions-lock.sh` so advisory findings stop counting toward the blocking tally — and did not bump pin 3. So every caller kept being judged by the **pre-#946** verifier, including [`metadatastician/burble#226`](metadatastician/burble#226), which had bumped its own pin *specifically to pick that fix up* and still went red on `governance / Actions lockfile verify`. Measured, not inferred: the failing job logs `HEAD is now at 4f7f02c`, and the two wrappers disagree on the same tree — | wrapper staged from | `--verify-local` rc | `grep -c is_advisory_category` | |---|---|---| | `4f7f02ca` (what CI ran) | **1** | 0 | | `e977cc67` (post-#946) | **0** | present | ## Why no existing control caught it The pin's **shape** was already guarded, correctly: `tests/test_governance_reusable_shape.sh:63-64` asserts `ref: [0-9a-f]{40}` and refuses `ref: main`, scoped to the `actions-lock-verify` job. Its **currency** was guarded by nothing. A perfectly well-formed 40-hex SHA can point at stale tooling, and this one did for the whole life of #946. That is the guard/consumer trap in its plainest form: **the guard asks "is this 40 hex characters?", the consumer needs "does this contain today's verifier?"** The file already carried a `⚠ BUMP THIS whenever ... changes` comment. A comment is not a gate, and this PR is the difference. Separately, `scripts/tests/governance-reusable-contract-test.sh` bound its checkout assertions only to the step named `Checkout the pinned Standards policy helpers` — the **dupkey** step. The lock gate is a *different* step, `Checkout standards for the lock gate`, and the two share the nouns "checkout", "pinned" and "standards", so a name-match guard written for one proves nothing about the other. It now names the lock-gate step too. ## The predicate, and why it is not the obvious one The obvious assertion — *the pin contains the working tree's helpers* — **deadlocks**. A PR that edits a helper would have to pin to its own merge commit, which does not exist yet. Unsatisfiable-in-PR is the same failure class as a required check that can never report. So the assertion is: > the pinned commit must already contain everything on the **compare** ref, > path-scoped to the step's own `sparse-checkout:` list. - **`pull_request`** → compare is the PR's base SHA. A PR that edits a helper **passes** (its edit is not on base yet). A PR opened while `main` is *already* stale is **forced to bump**, and can, because the needed commit exists. - **`push` to `main`** → compare is `HEAD`. Red exactly when a helper change has just landed and the bump is owed; healed by the very next PR, which the `pull_request` run will not let through unbumped. Under this predicate, **#954 would have been forced to bump after #946 landed**, and burble#226 would have gone green on its first attempt. The pin is therefore **one change behind by construction**. That is inherent, it is acceptable, and the comment at the pin now says so rather than asking a human to remember. Comparison is **path-scoped**, so a rebase or any unrelated commit cannot fail it — only a real divergence in the staged tooling can. **Scope is read out of the step's own `sparse-checkout:` list, never hardcoded**, so adding a file to what the gate stages automatically extends what the guard protects. A hardcoded list here would itself be a guard asking a different question than its consumer. ## Verification `scripts/tests/check-lock-gate-pin-freshness-test.sh` — **10 controls**, each against a throwaway git repo with real commits, fully offline: | control | asserts | |---|---| | stale pin is refused | rc=1, names `scripts/update-actions-lock.sh` | | stale report is path-scoped | never names the unrelated file that also changed | | fresh pin is accepted | rc=0 | | unrelated divergence does not fail it | a rebase must not redden the gate | | `ref: main` is refused | pinning is the point | | abbreviated sha is refused | 40-hex only | | **renamed step fails loudly** | rc=1 — the exact way the contract test lost its subject | | **unresolvable pin fails, not skips** | a skip is indistinguishable from a pass | | missing `ref:` is refused | would follow the default branch | | empty staged scope is refused | nothing to compare is not a free pass | **Meta-mutant.** Removing the path scoping from the guard (`git diff --name-only $pin $compare -- $paths` → without `-- $paths`) kills **exactly the two controls that assert it**, 8 passed / 2 failed. Restored, 10/10. **Contract-test mutants.** `ref: main` → `FAIL: the lock gate is not staged from an immutable 40-hex commit`. Renaming the step → `FAIL: governance workflow has no step named 'Checkout standards for the lock gate'`. Both rc=1. Full suite: **all 51 test files pass** on this branch. The guard **fails the job**. It is not `continue-on-error` and it is not a `::warning::`, which cannot fail a job. ## Notes - `self-test.yml` gains `fetch-depth: 0`. The guard compares two commits and **fails rather than skips** on an unresolvable pin, so the history is a requirement, not an optimisation. This is a `with:` change only — no `uses:` ref moves, so `actions.lock` is untouched. - The new Self Test step passes the base SHA through `env:`, not by interpolating an expression into the `run:` body. The repo's own injection scanner (`tests/test_tag_ruleset_canon.sh`) still passes. - `grep -A N` cannot delimit the step block: it is 19 lines today, so any fixed `N` is either short of the `ref:` or long enough to capture the **next** step's `ref:` and assert against the wrong pin. Both the guard and the contract test take the range from `- name:` to `- name:` with awk. ## ⚠ This does not turn burble#226 green on its own `metadatastician/burble#226` pins standards at `e977cc67`, and **that** copy of `governance-reusable.yml` still carries `4f7f02ca` at the lock-gate step. Pin 3 travels with the pinned YAML. Sequence: **merge this → take the resulting SHA → re-bump burble#226 to it** (all 9 sites plus `actions.lock`, transitive `uses:` list re-extracted against a positive control) → then #226 can go green. ## Out of scope, filed separately The **dupkey** pin `317101e0` is also stale — 45 files differ under `scripts/` versus `main`. Its `sparse-checkout` is the whole `scripts` directory, so the same predicate applied verbatim would be permanently red and useless; it needs a scope narrowed to what that step actually executes. Per the stopping rule that is an issue with acceptance criteria, not scope for this PR. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01X3hgXxWm6umMgZkjYyHnnm Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The defect
scripts/update-actions-lock.shalready knows that gh-actions-lock v0.1.6 cannot seejob-level reusable-workflow
uses:refs, andverify_lock_coverageaccepts the resultingstalefalse positive. On thevalid:truepath it also records advisory findings withoutblocking, exactly as its own comment says it should:
The
valid:falsepath did not honour that. Every finding that was not the accepted falsepositive was counted as blocking, so advisory
sha-as-reffindings became fatal the momentone real finding flipped
validto false.Measured, not inferred
Two arms on
metadatastician/burbleat4a3de2d(2026-09-22), which carries a genuinejob-level reusable-workflow ref the released tool cannot see
(
release.yml:144,slsa-framework/slsa-github-generator/.github/workflows/generator_generic_slsa3.yml@f7dd8c54…):validtruesha-as-reffalsesha-as-ref+ 1 ×staleIn the second arm the wrapper printed
and then failed anyway, on the seven advisories.
Consequence in production. This is what holds burble's
workflow-safety.ymljob red.The lock step fail-fasts, so the step behind it — "Verify secret-baseline positive and
negative controls" — is
skipped, not run. A correct lockfile is currently punished, and asecret-scanning control is suppressed as a side effect.
The fix, and the trap in it
Discriminate by category, never by severity.
gh-actions-lock v0.1.6 labels every finding
"severity": "warning"—staleincluded.A severity-based exemption is the obvious one-liner here and it is wrong: it would swallow
real desyncs and leave a gate that passes vacuously. Measured:
sha-as-refis advisory because the tool's own authoritativevalidbit is unaffected by it(7 of them coexist with
valid: true). That is the criterion, and it is the one used.The fail-closed guard is deliberately unchanged: advisory findings alone still never explain a
valid:falseresult, so they cannot buy approval on their own.Tests
Three cases added to
scripts/tests/actions-lock-update-test.sh, using the existingFAKE_VERIFY_FINDINGseam:stalealongside advisories → must still block (the vacuity guard)valid:false→ must still fail closedSuite: 12/12.
Mutants, both killed:
Stated precisely: the second mutant dies on an existing assertion rather than on new case 2,
so case 2 is a forward regression guard rather than the thing that catches that particular
wrong fix.
Blast radius
Callers are SHA-pinned, so this reaches a repo only when that repo bumps its pin — no
behaviour changes anywhere on merge.
metadatastician/burblewill bump to this commit in itsown lock-resync PR, which is what unblocks its secret-baseline controls.
The underlying tool bug is being reported upstream to
github/gh-actions-lockseparately;this wrapper change is the estate-side mitigation, not a substitute for that fix.
🤖 Generated with Claude Code
https://claude.ai/code/session_01X3hgXxWm6umMgZkjYyHnnm