lacs-project / lacs-project/sysknife
The action-pin check reports success over a workflow it could not read
- Dominant language
- Rust
- Stars
- 12
- Forks
- 19
- Avg merge
- 18h 57m
- Merged PRs (30d)
- 116
Description
#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:
```bash
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":
```bash
[ -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`:
```bash
((${#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.
Contributor guide
Research direction
Start in tests/release/release-rehearsal.test.sh at assert_action_pins and read the surrounding workflow-discovery checks. Run the script, then add coverage for an unreadable workflow and for a directory with no matching workflow files; verify failures name the relevant path, skipping the permission-mode case when running as root.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- bash
- Domain
- release, testing-qa
- Issue type
- Bug
- Difficulty
- 3/5
- Estimated time
- 1-2 days
- Activity status
- Active
- Clarity
- Clearly specified
- Newbie friendliness
- 78/100