lacs-project / lacs-project/sysknife

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

Open
#442 0 comments 0 reactions 0 assignees View on GitHub
bug easy help wanted
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.