Conversation
SCAN_START and SCAN_END are assigned around the scan and never referenced again — not in the report, the summary, trustabl.env, or either integration wrapper. Dead since the port from the GitLab component, where the surrounding code did consume them. Beyond the noise, they read as an intent the script does not deliver: a reviewer sees two timestamps bracketing the scan and reasonably expects a duration somewhere in the output. There is none. Removing them rather than wiring them up, since neither integration has anywhere to put a duration today. If one gains a use for it, one line brings it back.
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 Queued with the cleanup changes. 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. |
sairenchristianbuerano
left a comment
sairenchristianbuerano
left a comment
There was a problem hiding this comment.
Thanks @bradAGI — the change looks good, but it needs a rebase.
Several PRs touching scan/trustabl-scan.sh have landed on main since you
opened this, so it no longer merges cleanly. Could you rebase and resolve?
bash test/run-tests.sh will confirm it — 19 passing on main right now.
Ping me when it's up.
2 participants
SCAN_STARTandSCAN_ENDare assigned around the scan and never read again:Two assignments, zero references. Not in the console report, not in
trustabl-summary.md, not intrustabl.env, and not in either integration wrapper. Dead since the port from the GitLab component, where the surrounding code did consume them.Worth removing rather than leaving as harmless noise, because they read as an intent the script does not deliver: a reviewer seeing two timestamps bracket the scan reasonably expects a duration somewhere in the output. There isn't one.
Removing rather than wiring up, since neither integration has anywhere to put a duration today —
trustabl.envis the natural home and adding a field there is a documented-output change that should be its own decision. If one gains a use, one line brings it back.bash -n scan/trustabl-scan.shpasses.