ci: run shellcheck over the scanner and its tests - #22
Conversation
The scanner is the whole product here — it resolves a release, verifies it, runs the engine, scales the score, and decides whether the build fails — and until now nothing checked any of it. A regression in the gate logic or in the jq that reads the engine's ScanResult would ship silently. The suite drives the real scan/trustabl-scan.sh. Each test builds an actual gzipped release tarball containing a stub engine and a real checksums.txt over it, then puts a stub curl on PATH that serves that directory by URL basename. The download, sha256 verification, extraction and invocation therefore all run unmodified; only the network and the engine binary are substituted. The fixtures are unmodified output from a real `trustabl scan` (engine v0.1.7), so the assertions pin the scanner against the ScanResult shape the engine actually emits rather than a hand-written approximation of it.
The scanner is the product here and it is shell, with no static analysis over it. shellcheck catches the class of defect that stays invisible until a specific input hits it — an unquoted expansion, a masked exit status, a read of an unset variable. Run at --severity=warning so it gates on substance rather than style. Two fixes were needed to make it pass, both of which the tool found on its own: scan/trustabl-scan.sh:112 SC2034: SCAN_START appears unused scan/trustabl-scan.sh:142 SC2034: SCAN_END appears unused which is the dead code trustabl#18 removes, carried here so this job is green on its own; and SCAN_OUT/SCAN_EXIT in the harness, which are set in harness.sh and read by the test functions in run-tests.sh — a real cross-file reference shellcheck cannot see, so that one is a disable with the reason recorded next to it rather than a change. Validated on a real runner before opening: the job failed with exactly those four SC2034s, and passes with them addressed.
|
Thanks @bradAGI. Flagging that this is being read rather than sitting unlooked-at. We are reviewing all 52 open PRs together instead of one at a time. 31 of them edit Reading against #47, which also adds linting. The order we are working to: the test harness in #1 first, since twelve PRs depend on it and nothing is verifiable without it, then the fixes that close fail-open paths, then behaviour changes, then docs. A verdict on this one follows once its cluster is read. Apologies for the wait, and thanks for the contribution. |
|
Thanks @bradAGI — closing this one as a duplicate, and I want to be clear it is not a judgement on the change. Getting shellcheck onto a repo whose whole product is a shell script is overdue, and you moved first on it. Going with #47 instead: #47 lints the CI configs as well as the scanner, so it covers this plus a bit more. Nothing here was wrong. Two people found the same problem within days of each other, which is a good sign for the repo and a bad sign for how long that bug had been sitting there. Sorry for the wasted effort, and thanks for taking the time. |
2 participants
The scanner is the product, and it is shell, with no static analysis over it. shellcheck catches the class of defect that stays invisible until a specific input hits it — an unquoted expansion, a masked exit status, a read of an unset variable.
Run at
--severity=warningso it gates on substance rather than style.It found real dead code on the first run
The job failed initially with exactly four
SC2034s, and two of them were in the product:That is the dead code #18 removes — arrived at independently, which is a decent argument for the job paying for itself. The removal is carried here too so this job is green standalone; if #18 merges first, this hunk disappears.
The other two —
SCAN_OUT/SCAN_EXIT— are set inharness.shand read by the test functions inrun-tests.sh. That is a genuine cross-file reference shellcheck analyses per-file and cannot see, so it gets adisablewith the reason recorded next to it, not a silent suppression:Validated on a real runner before opening
Not asserted — run. On a fork PR against the ubuntu runner:
SC2034s aboveBoth the
test (ubuntu-latest)/test (macos-latest)jobs and the newshellcheckjob are green.