The action-pin check counts a directory named x.yml as a workflow it read

[vladimirrott]

#443 closes the case where assert_action_pins cannot read a workflow file: the loop now refuses and names the path. One entry of the same shape survives it. [ -r "$workflow" ] is true for a directory, grep on a directory exits 2 with Is a directory, and a process substitution's exit status is never checked, so the file count stays where it was and the run reports success over something it never opened.

Measured

On #443's head ce773581, with a probe appended to a copy of the shipped test (the probe is mine, it is not part of that pull request) and the whole script run in a container as uid 1000:

# PROBE (maintainer, not part of the PR): a *directory* named x.yaml passes -r.
mkdir -p "$pin_fixture/dirshaped"
printf '  uses: ./.github/workflows/local.yml\n' > "$pin_fixture/dirshaped/readable.yml"
mkdir "$pin_fixture/dirshaped/blocked.yaml"
if probe_out="$(assert_action_pins "$pin_fixture/dirshaped" 1 2>&1)"; then
    printf 'PROBE: pin check PASSED over a directory named blocked.yaml\n' >&2
else
    printf 'PROBE: pin check refused: %s\n' "$probe_out" >&2
fi

PROBE: pin check PASSED over a directory named blocked.yaml
Release rehearsal contract passed.
TEST rc=0

The readable file clears the floor of 1 on its own, which is the same arrangement #442 used: one entry answers for the directory and nobody notices the other one was skipped.

Severity, stated plainly

Lower than #442. A directory named docs.yml under .github/workflows is not something an ordinary checkout grows, and GitHub would ignore it as well. What makes it worth closing is the shape: this is the third way the same loop reports "asked, found nothing" for "could not ask", after the empty glob (#432) and the unreadable file (#442).

Suggested shape

Fold the file-type test into the arm that #443 added, and keep the message specific about which of the two failed:

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

A single combined condition is shorter and worse: the test then cannot tell the two refusals apart, and a diagnostic that covers both is one a future mutation can satisfy for the wrong reason.

Tests first

Copy the fixture #443 added at the end of tests/release/release-rehearsal.test.sh. Build a directory holding one readable .yml and one mkdir-ed blocked.yaml, call assert_action_pins with a floor of 1, require a non-zero exit and require the exact diagnostic with grep -Fxq. Write it before the fix and watch it print PROBE:-style success, as above.

No root skip is needed for this one: a directory is a directory for uid 0 too, which makes it cheaper to test than #442 was.

Difficulty

easy. Four lines of guard and one fixture, in a file whose test harness already exists. 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.

Land #443 first: this edits the four lines that pull request adds.

[vladimirrott]

@mikevillari, this one is yours if you want it, and I am holding it until you say either way. It came out of reviewing your #443: the arm you added is the right arm, and a directory slips past it for a reason you have already met once.

Take it or leave it with a word here. If you would rather have more to chew on, #441 is the other half of assert_action_pins, where the line grep misses a flow-style mapping, and the fixture that demonstrates it already lives in the file you just edited.

Wait for #443 to land before you start: this edits the four lines it adds.

[vladimirrott]

Status note so nobody duplicates work on this.

PR #446, which replaces the uses: extraction with a YAML parse, changes this behaviour as a side effect. read_text() raises IsADirectoryError, the new except OSError arm catches it, and the refusal names the path. I reproduced both sides against the real contract, one readable block-style workflow clearing a floor of 1 with a directory named blocked.yaml beside it:

=== main-b2c823e6: readable.yml clears the floor, blocked.yaml is a directory ===
PROBE: pin check PASSED over a directory named blocked.yaml (output: grep: /sktmp/tmp.jQBR7ykGJr/dir444/blocked.yaml: Is a directory)
TEST rc=0

=== pr446-968aa9ca: readable.yml clears the floor, blocked.yaml is a directory ===
PROBE: pin check refused: FAIL: cannot read /sktmp/tmp.TiYx4t4ge1/dir444/blocked.yaml: [Errno 21] Is a directory: '/sktmp/tmp.TiYx4t4ge1/dir444/blocked.yaml'
TEST rc=0

The full command and the probe it inserts are in my review on #446.

This issue stays open, because #446 carries no regression fixture for the directory case and fixed behaviour with nothing holding it in place is what comes back. The remaining work is three lines: a mkdir in the unpinned fixture block and an assert_pin_failure on the cannot read diagnostic. I have asked on #446 whether to fold that in there; if it lands with the pin-parsing change, this closes with it, and if not, the fixture on its own is a small standalone PR against whatever is on main at the time.

The hold on this issue is unchanged.