fix(ci): make ShellCheck coverage fail closed

[ITSMERNB]

Summary

• make the ShellCheck file enumeration fail closed on missing roots and an empty scan
• derive maintained shell files by shebang so extensionless hooks are covered
• include .githooks/pre-commit and .githooks/pre-push
• keep GitHub CI and ci-local on the same enumerator
• add a release coverage gate over every tracked shell file with a count floor

Validation

• bash -n scripts/shellcheck-files.sh tests/release/shellcheck-coverage.test.sh scripts/ci-local.sh
• bash tests/release/shellcheck-coverage.test.sh -> 195 tracked shell files covered
• missing-root fixture -> refused and names the missing root
• empty-root fixture -> refused instead of passing over zero files
• .githooks/pre-commit and .githooks/pre-push present in the derived scan
• git diff --check

Closes #409

[vladimirrott]

Reviewed at 2f425847547483acd2f7ca5ea298ac7e38108c2d.

The discovery script is right, and two of its guards are genuinely mutation-proved rather than asserted. shellcheck-files.sh refuses a missing root and refuses an empty result:

((${#files[@]} > 0)) || {
    printf 'shellcheck-files: no shell files found\n' >&2
    exit 1
}

and your test drives both failing paths through the real script with a temp directory, so a scan that finds nothing can no longer report success. Extending coverage to .githooks is the right instinct too: pre-push runs ci-local.sh --fast, so it is the script contributors actually depend on, and it was never linted.

I checked the thing most likely to make this red on merge, which is whether any tracked .sh file has a shebang your predicate misses. It does not:

$ while read -r f; do first=$(head -1 "$f"); \
    [[ "$first" == '#!'* && ( "$first" == *bash* || "$first" == */sh ) ]] || echo "MISS: $f"; \
  done < <(git ls-files '*.sh'); echo done
total .sh missed by predicate: 0

and the only extensionless shell files in the tree are the two .githooks hooks you hardcoded assertions for. So this passes today.

The blocking item

.githooks is in the scan roots but not in the workflow triggers, so a change to a git hook still never reaches the linter.

scripts/shellcheck-files.sh:7:

    roots=(tests/e2e tests/release scripts assets/demo .githooks)

.github/workflows/e2e.yml paths:, both the push and pull_request copies, unchanged by this PR:

      - "tests/e2e/**"
      - "tests/release/**"
      - "scripts/**"
      - "assets/demo/**"
      - "apps/sysknife-cli/**"
      - "crates/**"
      - ".github/workflows/e2e.yml"

scripts-lint in e2e.yml is the only CI job that runs shellcheck. Your new ci.yml step runs shellcheck-coverage.test.sh, which asserts membership in a list and never lints anything. So for a PR that touches only .githooks/pre-push, the coverage test goes green claiming the file is covered, scripts-lint is never dispatched, and no linter ever reads it.

What makes this worth blocking on rather than waving through is that e2e.yml already documents this exact bug, twice, in the comments right above the list you would be editing:

This job shellchecks tests/release too, so a change there has to be able to trigger it. It could not: the release-gate scripts were linted only when something else in the list happened to change alongside them.

Adding - ".githooks/**" to both paths: blocks fixes the instance. If you want to close the class, have shellcheck-coverage.test.sh assert that every root in shellcheck-files.sh appears in both of e2e.yml's paths: lists. Then the next person who adds a root cannot reintroduce it.

Not blocking

The extensionless branch of your test uses the same predicate as production, character for character:

# scripts/shellcheck-files.sh:25
    if [[ "$first" == '#!'* && ("$first" == *bash* || "$first" == */sh) ]]; then
# tests/release/shellcheck-coverage.test.sh:34
    elif [[ "$first" == '#!'* && ("$first" == *bash* || "$first" == */sh) ]]; then

For *.sh files the test has an independent oracle (the extension), so that half is fine. For extensionless files the two can never disagree, which means a #!/usr/bin/env sh or #!/bin/dash script would escape both the scan and the test that exists to catch it. No such file exists today, so this is latent rather than live. Worth fixing while you are in here.

Also: dropping -name '*.sh' from the find means you now open and read the first line of every file under five roots, including the demo GIFs and the JSON cassettes. Harmless, but a -name prefilter or a tracked-files source would be tidier.

One coordination note that is not your fault: #427 rewrites the same run_hygiene_group block to discover these tests by glob, so the two conflict. I confirmed it with git merge-tree. The resolution is to take #427's run_shell_tests, which picks your new test up automatically. I will handle the ordering, and whichever lands second gets a trivial rebase.

Requesting changes on the .githooks trigger gap only. The rest is solid work and the empty-set guards are exactly what I want to see.

[vladimirrott]

One heads-up on merge order, unrelated to the .githooks finding above.

#427 is approved and green, and it replaces the hardcoded list in run_hygiene_group with a glob over tests/release/*.test.sh. Your branch adds a line to that same block, so the two conflict:

$ git merge --no-edit origin/pull/427/head
Auto-merging scripts/ci-local.sh
CONFLICT (content): Merge conflict in scripts/ci-local.sh

When you rebase, resolve it in #427's favour and drop your ci-local.sh hunk: its glob picks up shellcheck-coverage.test.sh on its own, so you end up with less code and the same coverage. Nothing else in your diff touches a file #427 changes.

I will land #427 first, which means the conflict is already there when you next push rather than appearing after you think you are done.

[ITSMERNB]

Added .githooks/** to both the push and pull_request path filters and made the coverage test derive the default scan roots via --print-roots, then require every root to appear in both trigger lists. Rebased on current main first; local coverage now reports 201 tracked shell files and the syntax/diff checks are clean. I have left the #427 overlap alone while it is still open, as requested.

[ITSMERNB]

Thanks, Vladimir. I rebased #423 onto current main after #427 landed and resolved the overlap in #427's favour, so the old ci-local.sh hunk is gone.

The blocking trigger gap is fixed now: .githooks/** is present in both the push and pull_request path filters. I also added a guard that reads the ShellCheck root list from shellcheck-files.sh and requires every root to appear in both workflow trigger lists, so adding a future scan root without making it trigger the linter fails the release test.

Local checks after the rebase:

• bash -n on the touched shell scripts
• bash tests/release/shellcheck-coverage.test.sh -> 203 tracked shell files covered
• missing/empty roots still fail closed
• git diff --check

Pushed as 430fc0f on top of current main.

[ITSMERNB]

Addressed the blocking .githooks trigger gap and rebased cleanly after #427 landed. The old ci-local.sh hunk is gone in favor of #427's release-test glob, both e2e path filters now include .githooks/**, and the coverage test checks that every ShellCheck root appears in both trigger lists. Current head is 430fc0f; the full CI set is green, including scripts-lint and docs-and-hygiene.

[vladimirrott]

Reviewed at 430fc0f77b9ca9766b4f1a26abcfb660f204bc4d.

You closed the class rather than the instance, which is more than I asked for in so many words. --print-roots plus the trigger_count == 2 loop means the next person who adds a scan root cannot forget the workflow trigger, and two copies is the right granularity: a root wired into push but not pull_request is still a hole, and your loop says so.

I ran everything in a container against a clone of your head. maintainer screen says DO NOT EXECUTE for a PR touching .github/workflows/** and shell that the gates run directly, so nothing here ran on my machine.

Clean:

$ podman run --rm --network=none -v /home/entropia/.local/state/sysknife-maint/trees/rv423a:/repo:z -w /repo -e HOME=/tmp -e GIT_CONFIG_COUNT=1 -e GIT_CONFIG_KEY_0=safe.directory -e GIT_CONFIG_VALUE_0=/repo localhost/sk-pygit:1 bash -c 'bash tests/release/shellcheck-coverage.test.sh; echo "rc=$?"'
ok: ShellCheck covers 203 tracked shell files and refuses missing/empty roots
rc=0

Reverting the production hunk, both .githooks/** lines out of e2e.yml, turns your new assertion red and names the root:

FAIL: ShellCheck root .githooks must trigger both push and pull_request e2e jobs
rc=1

Removing one of the two, which is the shape somebody ships by accident, fails the same way:

$ git diff --stat
 .github/workflows/e2e.yml | 1 -
 1 file changed, 1 deletion(-)
FAIL: ShellCheck root .githooks must trigger both push and pull_request e2e jobs
rc=1

ShellCheck is clean over the exact list your enumerator produces, both hooks included:

$ xargs -0 shellcheck --severity=warning < .scan-list.bin; echo "shellcheck-over-discovered rc=$?"
shellcheck-over-discovered rc=0

Blocking, and both are small

1. The roots loop can inspect nothing

--print-roots feeds the loop and nothing counts what came out. Make that side silent and the assertion iterates zero times while the test still reports success:

$ git diff --stat scripts/shellcheck-files.sh
 scripts/shellcheck-files.sh | 1 -
 1 file changed, 1 deletion(-)
ok: ShellCheck covers 203 tracked shell files and refuses missing/empty roots
rc=0

The one deleted line is printf '%s\0' "${default_roots[@]}" inside the --print-roots branch, so the flag exits 0 having printed nothing. Your other two guards refuse an empty result by hand, which is why this one stands out. A count around the loop closes it:

roots_seen=0
while IFS= read -r -d '' root; do
    roots_seen=$((roots_seen + 1))
    ...
done <"$tmp/roots"
((roots_seen > 0)) || fail 'shellcheck-files.sh --print-roots produced no roots'

2. ci-local.sh is no longer on the same enumerator

Your PR body lists "keep GitHub CI and ci-local on the same enumerator" as done. At this head it is not, and I think the rebase over #427 is where it went. scripts/ci-local.sh:263-270 still carries the old find, under a comment that has become untrue:

# Same scan as e2e.yml's "ShellCheck maintained scripts" step -- kept here too
# since every script this task adds/touches must stay shellcheck-clean.
hygiene_shellcheck() (
    cd "$repo_root" || exit 1
    find tests/e2e tests/release scripts assets/demo \
        -type f -name '*.sh' -print0 \
        | xargs -0 shellcheck --severity=warning
)

Two files apart, and they are the two files this PR is about:

$ echo "ci-local: $(wc -l < /tmp/cilocal.txt)  enumerator: $(wc -l < /tmp/enum.s.txt)"
ci-local: 201  enumerator: 203
$ comm -13 /tmp/cilocal.txt /tmp/enum.s.txt
.githooks/pre-commit
.githooks/pre-push

So somebody edits .githooks/pre-push, runs scripts/ci-local.sh, sees green, pushes, and CI lints a file their local run never read. Pointing hygiene_shellcheck at scripts/shellcheck-files.sh fixes the instance. One more assertion in shellcheck-coverage.test.sh stops it drifting back: ci-local.sh has to invoke the enumerator, and must not carry its own find ... -name '*.sh'.

Not blocking

Your validation section says 195 tracked shell files; I measure 203. The tree has held 201 tracked *.sh plus the two extensionless hooks since 61b3a87, so nothing moved under you between those runs and the difference is on your side. Both numbers clear the > 150 floor, so this changes no verdict. I mention it because a validation number that nobody recounts is how a wrong one survives.

The extensionless branch of your test still uses the production predicate character for character (scripts/shellcheck-files.sh:25 and tests/release/shellcheck-coverage.test.sh:34), so for a file with no .sh suffix the two can never disagree. I raised it last round and it is still latent, since the only extensionless shell files in the tree are the two hooks you assert on by name. Fine to leave.

After this

Push those two changes and I will merge on the next pass, on conditions I would rather state now than apply without saying so: no production or workflow diff beyond the two hunks, the mutation above re-run green-then-red against your new head, a clean board on ci and e2e both, and the approval still standing. Anything else landing in .github/** or crates/** sends it back through a full read.

You keep finding guards that answer a question they could not ask, which is the failure this repo has shipped more often than any other. When #409 closes I would like to hand you something in that family rather than something adjacent. Say whether you would rather stay on CI plumbing or move toward the daemon side, and I will hold whichever fits.

One invitation, not a condition. Anyone this careful about which shell files get linted is running these scripts on real machines. SysKnife's read-only surface costs nothing to try: sysknife "show me failed services" or sysknife doctor plans and previews before it touches anything. If you do run it on a box you administer, I would rather hear what it got wrong than not hear.

[vladimirrott]

Re-reviewed at 65b6e6189796f577220efac1c36148f7a11fad38, four minutes after the last one. Both items are done and done the way I would have done them.

The mutation that passed last round now fails:

$ git diff --stat
 scripts/shellcheck-files.sh | 1 -
 1 file changed, 1 deletion(-)
FAIL: shellcheck-files.sh --print-roots produced no roots
rc=1

ci-local.sh put back on its own find:

$ git diff --stat
 scripts/ci-local.sh | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)
FAIL: ci-local shellcheck must use scripts/shellcheck-files.sh
rc=1

The second half of that assertion earns its keep on its own. Enumerator call left in place, one stray find ... -name '*.sh' added beside it, which is how this drifts back in real life rather than by someone deleting the call:

$ git diff --stat
 scripts/ci-local.sh | 1 +
 1 file changed, 1 insertion(+)
FAIL: ci-local shellcheck must not maintain its own '*.sh' find
rc=1

And the .githooks trigger still bites at this head:

FAIL: ShellCheck root .githooks must trigger both push and pull_request e2e jobs
rc=1

Clean, and ShellCheck over the three changed scripts:

ok: ShellCheck covers 203 tracked shell files and refuses missing/empty roots
rc=0
$ shellcheck --severity=warning scripts/ci-local.sh scripts/shellcheck-files.sh tests/release/shellcheck-coverage.test.sh; echo "shellcheck rc=$?"
shellcheck rc=0

The merge gate has its receipt:

$ maintainer-merge verify 423 65b6e6189796f577220efac1c36148f7a11fad38 "tests/release/shellcheck-coverage.test.sh" '/^      - "\.githooks\/\*\*"$/d' shell
  suite: shell (docker.io/library/python:3.12 under podman)
  running 'tests/release/shellcheck-coverage.test.sh' unmutated
  shell suite: evidence of 1 executed unit(s), passing unmutated
  applying the mutation and re-running
  receipt recorded for #423 at 65b6e618 (observed: clean pass, mutated fail)

Approved. One thing stands between this and main, and it is procedural rather than anything about your work: a pull request touching .github/workflows/** gets a human read of the workflow diff before it merges, and that is the control that keeps a fork from reshaping CI. Yours is two paths: entries and a rewritten ShellCheck step, and I have read both, but the second pair of eyes is the point of the rule. Analyze (rust) is also still running.

So this merges as soon as those two land, with nothing more from you.

Approving a first-timer's queued workflow run is the other thing I keep manual, and it is why your earlier pushes sat. That delay is mine.

On what comes next: you said nothing yet about CI plumbing against the daemon side, and the offer stands either way. #433 is the CI-shaped one, a ci-local run reporting PASS after a root-only assertion skipped itself with nothing comparing what ran against what was meant to run, which is this same argument one layer up. I have offered that to @vsolano9 first since it grew out of their #407. If they pass, it is yours. Say which direction you want and I will hold something in it.

[ITSMERNB]

Thanks for the fast re-review and for reproducing both mutations independently. Daemon side next, please. I would like to stay in the same fail-closed / evidence-boundary family, but closer to the actual execution and authorization path rather than another CI-only guard. I will leave #423 with the workflow second-read/merge process from here.