From 9da0faf6e39eb1640f8646c5cf54a75bc54965e2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dawid=20Ci=C4=99=C5=BCarkiewicz?= Date: Fri, 24 Jul 2026 15:52:38 -0700 Subject: [PATCH] perf(shellcheck): batch pre-commit script checks ### Summary Run ShellCheck once for all matching tracked text scripts instead of starting one process per file. Preserve the existing severity, file-list membership, output, and failure behavior while making the empty-script case an explicit no-op. Add structural generated-hook coverage for invocation count and argument safety. ### Details Build a Bash array by reading the existing newline-delimited `FLAKEBOX_GIT_LS_TEXT` representation and selecting `.sh` suffixes. Quoted array expansion preserves spaces and glob characters, then passes `--severity=warning` and all paths to one ShellCheck process. This intentionally does not redesign the generic file-list representation, add NUL transport, or add ARG_MAX chunking. Use a fake ShellCheck in the generated-hook integration check to prove zero scripts causes no invocation, two scripts cause exactly one invocation with exact argv, and a failing ShellCheck still fails the generated hook. An untracked path matching a tracked literal bracket expression makes unsafe pathname expansion fail the test. Regenerate the checked-in pre-commit artifact and Flakebox root identity. ### Reviews An independent review found that the initial bracketed-path fixture had no matching directory entry and therefore could not detect unsafe pathname expansion. The strengthened fixture adds an untracked matching path while tracking the bracketed name with literal Git pathspec semantics. Follow-up review passed with no findings. ### Validation - `nix develop -c treefmt` - focused `gitHooks` Nix check - `nix develop -c selfci check` - `nix develop -c selfci check --candidate swkkprpu` --- .config/flakebox/id | 2 +- checks/git-hooks.nix | 48 ++++++++++++++++++++++++++++++++++++++ lib/modules/shellcheck.nix | 13 ++++++++--- misc/git-hooks/pre-commit | 13 ++++++++--- 4 files changed, 69 insertions(+), 7 deletions(-) diff --git a/.config/flakebox/id b/.config/flakebox/id index 402fe1c..51ec345 100644 --- a/.config/flakebox/id +++ b/.config/flakebox/id @@ -1 +1 @@ -b3c9ea32bd238c00ccfd8b74c3b0c683441ebb2d67d4c0a47a0065404d27dbf5e33ade808310e029032c69e0d5418ac285a19dee4c331484b51a0924b27fcb50 +d65b7ff6c41916c949fb5e2b5511d6ce5b951e3e878db458c8012df8bc027a93a773b72a53ea93e1533daaf4142447f7f88c8970cc53853baa108eb2dbde40ae diff --git a/checks/git-hooks.nix b/checks/git-hooks.nix index 9e5c92d..6b147e2 100644 --- a/checks/git-hooks.nix +++ b/checks/git-hooks.nix @@ -64,6 +64,10 @@ let ''; }; + shellcheckHooks = mkHookFixture { + inherit (flakeboxLib.config.git.pre-commit.hooks) shellcheck; + }; + stashHooks = mkHookFixture { stash-probe = '' grep -qx staged tracked.txt @@ -344,6 +348,50 @@ pkgs.runCommand "git-hooks-tests" kill "$background_pid" 2>/dev/null || true cd .. + # ShellCheck is skipped for an empty script set, then receives every tracked + # script in one argument-safe invocation. Its failure still fails the hook. + mkdir shellcheck-repo + cd shellcheck-repo + git init -q + mkdir fake-shellcheck-bin + cat > fake-shellcheck-bin/shellcheck <<'EOF' + #!${pkgs.bash}/bin/bash + printf 'invocation\n' >> "$FLAKEBOX_SHELLCHECK_LOG" + printf '%s\n' "$@" > "$FLAKEBOX_SHELLCHECK_ARGS" + exit "''${FLAKEBOX_SHELLCHECK_STATUS:-0}" + EOF + chmod +x fake-shellcheck-bin/shellcheck + export PATH="$PWD/fake-shellcheck-bin:$PATH" + export FLAKEBOX_SHELLCHECK_LOG="$PWD/shellcheck.log" + export FLAKEBOX_SHELLCHECK_ARGS="$PWD/shellcheck.args" + + printf 'not a script\n' > tracked.txt + git add tracked.txt + NO_STASH=1 bash ${shellcheckHooks}/misc/git-hooks/pre-commit + [ ! -e "$FLAKEBOX_SHELLCHECK_LOG" ] + + printf '#!/bin/sh\n' > 'alpha script.sh' + printf '#!/bin/sh\n' > 'literal[glob].sh' + # This untracked path makes an unsafe expansion of literal[glob].sh match. + printf '#!/bin/sh\n' > literalg.sh + git --literal-pathspecs add 'alpha script.sh' 'literal[glob].sh' + NO_STASH=1 bash ${shellcheckHooks}/misc/git-hooks/pre-commit + [ "$(grep -c '^invocation$' "$FLAKEBOX_SHELLCHECK_LOG")" -eq 1 ] + printf '%s\n' \ + '--severity=warning' \ + 'alpha script.sh' \ + 'literal[glob].sh' > expected-shellcheck.args + cmp expected-shellcheck.args "$FLAKEBOX_SHELLCHECK_ARGS" + + set +e + FLAKEBOX_SHELLCHECK_STATUS=23 NO_STASH=1 \ + bash ${shellcheckHooks}/misc/git-hooks/pre-commit + shellcheck_failure_status=$? + set -e + [ "$shellcheck_failure_status" -ne 0 ] + [ "$(grep -c '^invocation$' "$FLAKEBOX_SHELLCHECK_LOG")" -eq 2 ] + cd .. + # Private patch restoration exposes indexed content to checks and restores # tracked worktree changes afterwards without touching the user stash. mkdir stash-repo diff --git a/lib/modules/shellcheck.nix b/lib/modules/shellcheck.nix index dc6e028..9116389 100644 --- a/lib/modules/shellcheck.nix +++ b/lib/modules/shellcheck.nix @@ -21,9 +21,16 @@ config = lib.mkIf config.shellcheck.enable { git.pre-commit.hooks = { shellcheck = '' - for path in $(echo "$FLAKEBOX_GIT_LS_TEXT" | grep -E '.*\.sh$'); do - shellcheck --severity=warning "$path" - done + shellcheck_paths=() + while IFS= read -r path; do + if [[ "$path" == *.sh ]]; then + shellcheck_paths+=("$path") + fi + done <<< "$FLAKEBOX_GIT_LS_TEXT" + + if [[ "''${#shellcheck_paths[@]}" -ne 0 ]]; then + shellcheck --severity=warning "''${shellcheck_paths[@]}" + fi ''; }; diff --git a/misc/git-hooks/pre-commit b/misc/git-hooks/pre-commit index 7b50aec..0330c00 100755 --- a/misc/git-hooks/pre-commit +++ b/misc/git-hooks/pre-commit @@ -107,9 +107,16 @@ export -f check_semgrep function check_shellcheck() { set -euo pipefail - for path in $(echo "$FLAKEBOX_GIT_LS_TEXT" | grep -E '.*\.sh$'); do - shellcheck --severity=warning "$path" - done + shellcheck_paths=() + while IFS= read -r path; do + if [[ "$path" == *.sh ]]; then + shellcheck_paths+=("$path") + fi + done <<< "$FLAKEBOX_GIT_LS_TEXT" + + if [[ "${#shellcheck_paths[@]}" -ne 0 ]]; then + shellcheck --severity=warning "${shellcheck_paths[@]}" + fi } export -f check_shellcheck