lacs-project / lacs-project/sysknife
The ShellCheck job passes when a search root disappears, and it never lints the two git hooks
- Dominant language
- Rust
- Stars
- 12
- Forks
- 19
- Avg merge
- 18h 57m
- Merged PRs (30d)
- 116
Description
`.github/workflows/e2e.yml:77-81` is the only ShellCheck gate in CI, and it
reads its file list from the left-hand side of a pipe:
```yaml
- name: ShellCheck maintained scripts
run: |
find tests/e2e tests/release scripts assets/demo \
-type f -name '*.sh' -print0 \
| xargs -0 shellcheck --severity=warning
```
GitHub runs a `run:` block as `bash -e {0}`, and `e2e.yml` sets no
`defaults.run.shell`, so there is no `pipefail`. `find`'s exit status is
discarded and the step's verdict comes from `xargs` alone. Rename or move one of
those four roots and the scan quietly shrinks to the survivors.
## Why it matters
Reproduced in a scratch tree, running the step body exactly as the workflow
writes it. `bad.sh` carries three warnings and lives under `tests/release`:
```
$ d=/tmp/sk-shellcheck-repro; mkdir -p "$d"/{tests/e2e,tests/release,scripts,assets/demo}
$ printf '#!/usr/bin/env bash\nfoo=1\ncd $undefined_and_unquoted\necho "$fooo"\n' > "$d/tests/release/bad.sh"
$ printf '#!/usr/bin/env bash\necho ok\n' > "$d/scripts/good.sh"
$ cd "$d"
$ out="$(bash -e -c "find tests/e2e tests/release scripts assets/demo -type f -name '*.sh' -print0 | xargs -0 shellcheck --severity=warning" 2>&1)"; rc=$?
$ echo "rc=$rc"; printf '%s\n' "$out" | head -6
rc=123
In tests/release/bad.sh line 2:
foo=1
^-^ SC2034 (warning): foo appears unused. Verify use (or export if used externally).
$ mv tests/release tests/release-suite
$ out="$(bash -e -c "find tests/e2e tests/release scripts assets/demo -type f -name '*.sh' -print0 | xargs -0 shellcheck --severity=warning" 2>&1)"; rc=$?
$ echo "rc=$rc"; printf '%s\n' "$out"
rc=0
find: ‘tests/release’: No such file or directory
```
Green, with a file carrying three warnings never opened. The whole trace is one
line on stderr inside a passing step.
Two facts bound this honestly, and both are worth knowing before you start.
**The local mirror already catches it.** `scripts/ci-local.sh:254-258` runs the
identical `find | xargs`, inside a script whose line 2 is `set -euo pipefail`,
and the subshell inherits it:
```
$ out="$(bash -eo pipefail -c "find tests/e2e tests/release scripts assets/demo -type f -name '*.sh' -print0 | xargs -0 shellcheck --severity=warning" 2>&1)"; rc=$?
$ echo "rc=$rc"; printf '%s\n' "$out"
rc=1
find: ‘tests/release’: No such file or directory
```
So CI is the looser of the two copies, which is the wrong way round.
**The empty case is fail-closed, so this is about a stale path rather than a
vanished list:**
```
$ out="$(printf '' | xargs -0 shellcheck --severity=warning 2>&1)"; rc=$?
$ echo "rc=$rc"; printf '%s\n' "$out" | head -1
rc=123
No files specified.
```
## The second gap in the same two lines
The roots miss the two hooks that gate every contributor's commit and push.
194 tracked shell files, 192 scanned:
```
$ git ls-files > /tmp/sk-tracked.txt
$ python3 - <<'PY' > /tmp/sk-all-sh.txt
import pathlib
for line in open('/tmp/sk-tracked.txt'):
p = line.rstrip('\n')
f = pathlib.Path(p)
if not f.is_file():
continue
if p.endswith('.sh'):
print(p); continue
try:
first = f.open('rb').readline().decode('utf-8', 'replace')
except Exception:
continue
if first.startswith('#!') and ('bash' in first or first.rstrip().endswith('sh')):
print(p)
PY
$ sort -o /tmp/sk-all-sh.txt /tmp/sk-all-sh.txt; wc -l < /tmp/sk-all-sh.txt
194
$ find tests/e2e tests/release scripts assets/demo -type f -name '*.sh' | sort > /tmp/sk-scanned.txt; wc -l < /tmp/sk-scanned.txt
192
$ comm -23 /tmp/sk-all-sh.txt /tmp/sk-scanned.txt
.githooks/pre-commit
.githooks/pre-push
```
Neither has a `.sh` extension, which is why `-name '*.sh'` skips them.
`.githooks/pre-commit` is the script that runs `scripts/check_no_secrets.sh`
before every commit, and it is unlinted.
## Scope
- Read the list into an array and refuse an empty one, then lint from the
array: `mapfile -d '' files < <(find ...)`, `(( ${#files[@]} > 0 ))` or exit
non-zero, `shellcheck --severity=warning "${files[@]}"`.
- Add `.githooks` to the search roots, and select on the shebang rather than
the extension so a new extensionless hook is covered by construction.
- Make the same change in `scripts/ci-local.sh:254-258` so the two copies stay
the same scan. They are meant to be identical; the comment at `:253` says so.
- Do not reach for `set -o pipefail` in the workflow as the whole fix. It turns
this case red, and it leaves the next reader with the same two lines to
reason about.
## Tests first
`tests/release/*.test.sh` is where the runnable gates live, and #331 is about
wiring every one of them to a gate, so put the assertion there. A
`shellcheck-coverage.test.sh` that derives both sets and asserts every tracked
shell file appears in the scanned set, plus a floor on the scanned count.
Prove it bites twice, in a fixture copy rather than in the tree:
1. Rename one search root. The test must fail naming the missing root, not
pass on the survivors.
2. Point the search at an empty directory. The test must refuse rather than
report success over zero files.
## Difficulty
`easy`. Two shell edits and one new test script, no Rust and no VM.
## Getting started
[CONTRIBUTING.md](https://github.com/lacs-project/sysknife/blob/main/CONTRIBUTING.md)
has the build and test commands. `shellcheck` comes from `apt install shellcheck`.
No CLA and no copyright waiver. The project is MIT.
Contributor guide
Assessment
This issue has not been assessed yet.