From f0c1bc40708615d5b423922c08c1f27e6cf96259 Mon Sep 17 00:00:00 2001 From: Craig Jennings Date: Fri, 24 Jul 2026 12:28:18 -0500 Subject: fix(hooks): the secret scan no longer passes when git fails Every bundle built its scan input as `git diff --cached ... | grep ... || true`. The `|| true` has to stay, since grep exits 1 when it matches nothing and that is the ordinary case. But with no pipefail it also swallowed a failure of git itself, so an empty result made the scan search nothing, find nothing, and report clean with a real credential staged. A gate that passes without having looked. .emacs.d found it in elisp. It was in all five: bash, elisp, go, python and typescript, eleven sites once each bundle's staged-file list is counted. Two of those bundles are ones I wrote yesterday by copying bash, so I propagated it while closing an unrelated gap in the same file. Each site now reads the diff on its own and aborts if git fails, leaving the greps their `|| true`. Verified per bundle on three axes: refuses when the diff cannot be read, still blocks a real staged secret, still passes a clean commit. The cross-bundle test suite had its own version of the same disease. Its VARIANTS list read "elisp bash go" while python and typescript also shipped hooks, so every "in every variant" assertion had quietly skipped two bundles since the day they were added. VARIANTS is now discovered from the tree. Two fail-closed assertions join it, and .emacs.d's elisp suite is adopted here beside the canonical hook, because a test living in the consuming project cannot fail when the canonical regresses. Also removes the validate-el auto-test cap, Craig's call. Above 20 matching test files the runner skipped everything and exited 0 with no output. The premise was speed and it did not hold: a whole family runs in about a second. The cap was also hiding a real cross-test pollution bug that only surfaces when a family runs in one process. --- working/hook-fail-open/rulesets-hook-note.txt | 31 --------------------------- 1 file changed, 31 deletions(-) delete mode 100644 working/hook-fail-open/rulesets-hook-note.txt (limited to 'working/hook-fail-open/rulesets-hook-note.txt') diff --git a/working/hook-fail-open/rulesets-hook-note.txt b/working/hook-fail-open/rulesets-hook-note.txt deleted file mode 100644 index 1c7a813..0000000 --- a/working/hook-fail-open/rulesets-hook-note.txt +++ /dev/null @@ -1,31 +0,0 @@ -Two hook fixes from .emacs.d, both the same defect class: a quality gate that reports success without having looked. Both files are rulesets-owned, so these local edits revert on the next sync — please take them into the canonicals. - -FILE 1: languages/elisp/githooks/pre-commit - -The secret scan built its input like this: - - added_lines="$(git diff --cached -U0 --diff-filter=AM | grep '^+' | grep -v '^+++' || true)" - -No pipefail, and || true swallows everything. Any git failure yields an empty added_lines, so the scan searches nothing, finds nothing, reports clean, and the commit proceeds with a real secret in it. Reproduced with a stub git that fails on the staged-diff call: the hook exited 0 with an AWS-shaped key staged. - -The || true cannot simply be removed — grep exits 1 when it matches nothing, which is the ordinary case for a commit with no added lines. The fix separates the two: read the diff on its own and abort on a git failure, then let the greps keep their || true. - -The staged-file list feeding the paren check (line 40 in the old file) had the identical hole, so a git failure there silently skipped paren validation. Fixed the same way. - -Note the local copy was already identical to the canonical before this change, so there is no other drift to reconcile. - -FILE 2: the elisp bundle's .claude/hooks/validate-el.sh - -The auto-test runner is gated on `[ "$count" -ge 1 ] && [ "$count" -le "$MAX_AUTO_TEST_FILES" ]` with no else branch. Above the cap the block is skipped, nothing is printed, and the hook exits 0 — indistinguishable from a passing run. Live for the three largest families in .emacs.d: calendar-sync 63 test files, music 45, ai-term 35. Every edit to those ran parens and byte-compile and zero tests, silently, and this has already let a wrong test assertion through more than once. - -The cap is not the problem and I did not change it. The silence is. Added a notice_json helper (reports via additionalContext + stderr, exits 0 rather than 2) and an elif that names the count, states plainly that it is NOT a pass, and gives the make test-file command. Live output on a calendar-sync edit: - - TESTS SKIPPED: 63 test files match calendar-sync.el — over the auto-run cap of 20, so none ran. This is NOT a pass. Run them explicitly, e.g. make test-file FILE=test-calendar-sync--add-days.el - -TESTS: both files now have bats coverage, attached as tests/test-pre-commit-hook.bats and tests/test-validate-el-hook.bats (8 + 4 tests, all green, shellcheck clean at warning level). They were written red-first: the fail-open tests drive a stubbed failing git, and the over-cap tests build a synthetic project with 21 test files. If rulesets wants these tests alongside the canonical hooks rather than in the consuming project, they should move — your call on where they belong, since the hooks are yours and the tests currently live in ours. - -One caveat on the pre-commit bats file: it deliberately assembles its credential fixtures at runtime (AKIA + tail) rather than embedding literals, because a literal key-shaped string trips the very hook under test on every commit touching that file. Worth preserving if you adopt it. - -CONTEXT: found during a skeptical review pass. The pre-commit one is the more interesting find — it surfaced *because* a reviewer was checking an unrelated one-line fix to the same file and noticed the fixed instance was the harmless one while the dangerous one three lines down was untouched. This is the fifth instance in .emacs.d of the family "a gate that enumerates or caps or swallows instead of discovering" — the earlier ones being a Makefile wildcard bug that ran zero integration tests for months, stale .elc masking unparseable source, and a hardcoded benchmark list. - -No reply needed unless you want the tests re-homed or the wording changed. -- cgit v1.2.3