The action-pin check reports success over a workflow it could not read

[vladimirrott]

#432 taught tests/release/release-rehearsal.test.sh to refuse when it extracted too few uses: lines, which closes the case where the extraction returns nothing. A second path to the same outcome survives: a workflow the extraction cannot read is counted as zero and the run still passes, as long as the remaining files clear the floor.

Measured

On b2c823e, one file made unreadable, nothing else changed:

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

The invariant in the comment above that loop says every uses: in every workflow pins a 40-hex SHA. That run verified one fewer workflow than it claimed, and said so only on stderr, where nothing reads it.

The floor does not catch it because the other workflows still total more than 20. It would only catch a host where enough files were unreadable to drop the whole count below the floor.

The obvious cause is the wrong one

assert_action_pins ends its loop with:

done < <(grep -E '^[[:space:]]*(-[[:space:]]+)?uses:' "$workflow" || true)

The || true looks like the culprit and is not. Removing it changes nothing, measured:

clean run without '|| true':            rc=0
unreadable workflow, no '|| true':      rc=0, "Release rehearsal contract passed."

The exit status of a process substitution is never checked, by set -e or anything else, so grep exiting 2 is invisible either way. The || true is redundant, not load-bearing.

Suggested shape

Assert readability per file, before the loop body, so "could not ask" is separated from "asked, found nothing":

[ -r "$workflow" ] || {
    printf 'FAIL: cannot read %s\n' "$workflow" >&2
    return 1
}

A negative twin belongs with it, and it is cheap: create a fixture directory, chmod 000 a file in it, call assert_action_pins and require the failure to name the file. Note that a test doing this has to skip under a real root, where the mode is not enforced.

While you are in there

The glob assertion has no negative twin either. Deleting it leaves the whole suite green, because the fixture directory always contains missed.yml:

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

It does fire when reached, with a better message than the floor gives, so this is a missing test rather than a broken arm.

Tests first

Two directions, and the second is the one this issue is really about.

Break what the guard protects. Build a fixture directory holding two or three
workflow files, chmod 000 one of them, call assert_action_pins against that
directory, and require it to exit non-zero naming the file it could not read.
Write that test before the fix and watch it fail: on b2c823e6 the function
returns 0 and prints the contract as passed.

A test that does this has to skip under a real root, where mode 000 is not
enforced. tests/release/action-steps.test.sh already carries a geteuid() == 0
guard you can copy the shape of, and #433 is the issue about ci-local staying
quiet when such a skip happens.

Break the guard's own input. The glob assertion has no negative twin, so
deleting it changes nothing:

$ grep -c 'no workflow files matched' tests/release/release-rehearsal.test.sh
0
$ bash tests/release/release-rehearsal.test.sh; echo "rc=$?"
Release rehearsal contract passed.
rc=0

Point assert_action_pins at an empty directory and require the failure, so the
arm that fires when discovery selects nothing is itself covered. It does fire
when reached, and with a better message than the floor gives; what is missing is
anything that would notice if it stopped.

Do not fix the readability case by widening the floor. The floor answers "the
extraction broke wholesale" and this is "one file was skipped in silence"; a
number large enough to catch the second would be a number that fails on an
ordinary day.

Difficulty

easy. One file, under ten lines of change, and the work is the two tests
rather than the fix. No Rust, no VM, no daemon, no credentials.

Getting started

CONTRIBUTING.md
has the build and test commands. The only one you need here is
bash tests/release/release-rehearsal.test.sh. No CLA and no copyright waiver.
The project is MIT.

[vladimirrott]

@mikevillari this one is yours if you want it. It replaces the #431 offer I have just withdrawn, which a PR overtook a day after I made it.

Why this one rather than another. #399 was a CI step reporting success over a set it never counted, and your fix separated "the producer failed" from "the producer returned nothing" so the two could not read alike. This is the third member of that family, in a release gate rather than a workflow: release-rehearsal.test.sh treats a workflow it cannot read as one containing zero uses: lines, and passes.

I reproduced it at b2c823e6 this morning. The grep wrote one line to stderr naming the file, and I have trimmed the absolute path off the front of it:

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

The floor #432 added does not catch it, because the remaining workflows still clear twenty on their own. The || true at the end of that loop looks like the cause and is not, and the body explains why with the measurement.

The second half of the issue is a missing negative twin rather than a broken check, and I confirmed it holds by deleting the assertion:

$ grep -c 'no workflow files matched' tests/release/release-rehearsal.test.sh
0
$ bash tests/release/release-rehearsal.test.sh; echo "rc=$?"
Release rehearsal contract passed.
rc=0

The whole suite stays green with that arm gone, because the fixture directory always contains a file. (I restored the file afterwards; git status --porcelain | wc -l was 0.)

No VM, no daemon, no credentials, and one file. A test that does this has to skip under a real root, where mode 000 is not enforced.

No deadline, and no is a complete answer.

[mikevillari]

Taking you up on #442. The new unreadable-file regression fails on the current implementation, and the four-line readability guard makes it pass. The empty-directory case uses a zero floor so the count guard cannot hide a missing discovery check. Removing either guard independently turns the test red.

I am finishing the diagnostic mutation checks and validation report before opening the PR. Please assign this one to me so the claim is visible to other contributors.