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. --- todo.org | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) (limited to 'todo.org') diff --git a/todo.org b/todo.org index bd5f16e..023f991 100644 --- a/todo.org +++ b/todo.org @@ -55,10 +55,15 @@ Grading: Minor severity (nothing broken now; the exposure is a future regression Fix: add =claude-templates/bin/*= to the =check_hook= loop, the same shape =languages/*/githooks/*= already uses for extensionless files. A no-op today by design — it passes immediately — which is exactly what a guard should do. -** VERIFY [#A] Parked: the secret-scan pre-commit fails open in ALL FIVE bundles (from .emacs.d) +** DONE [#A] Applied: the secret-scan pre-commit fails open in ALL FIVE bundles (from .emacs.d) :PROPERTIES: :LAST_REVIEWED: 2026-07-24 :END: +CLOSED: [2026-07-24 Fri] +Applied 2026-07-24 across all five bundles (11 sites: the secret-scan input in each, plus each bundle's staged-file list, typescript having two). Verified on all five: refuses when the diff can't be read, still blocks a real staged secret, still passes a clean commit. Adopted .emacs.d's elisp bats suite (8 tests) and extended the repo-level cross-bundle suite with two fail-closed assertions. + +Separate defect found while wiring that up: the cross-bundle suite's VARIANTS list read "elisp bash go" while python and typescript also shipped hooks, so every "in every variant" assertion had silently skipped two bundles since I added them. VARIANTS is now discovered from the tree rather than enumerated — the same failure class the tests exist to catch. + What arrived: .emacs.d found that =languages/elisp/githooks/pre-commit= builds its scan input as =added_lines="$(git diff --cached -U0 ... | grep '^+' | grep -v '^+++' || true)"=. With no pipefail and =|| true= swallowing everything, any git failure yields an empty string, so the scan searches nothing, finds nothing, reports clean, and the commit proceeds with the secret in it. The staged-file list feeding the paren check has the identical hole. *Verified independently, and it is worse than reported.* I reproduced the fail-open with a stub git that fails only the staged-diff call: exit 0 with an AWS-shaped key staged. Then I checked the other bundles, which the sender did not: *bash, go, python, and typescript all carry the same pattern and all fail open the same way.* Confirmed live on all five. Two of those (python, typescript) are hooks I wrote on 2026-07-24 by copying the bash one, so I propagated the defect while closing a different gap. @@ -70,12 +75,14 @@ The sender's fix is correct and I verified all three axes on it: refuses to proc Decision needed: the fix as sent covers elisp only. It should be applied to all five bundles, which is my scope expansion rather than the sender's proposal — hence a VERIFY rather than a silent apply. Also unresolved: where the two attached bats suites live, since the hooks are rulesets-owned and the tests currently sit in the consuming project. Prepared: [[file:working/hook-fail-open/pre-commit.diff]], plus =test-pre-commit-hook.bats= (8 tests) in the same dir. -Say "approve the hook fail-open fix" and it gets applied across all five bundles. -** VERIFY [#B] Parked: remove the validate-el auto-test cap (from .emacs.d, Craig's call) +** DONE [#B] Applied: remove the validate-el auto-test cap (from .emacs.d, Craig's call) :PROPERTIES: :LAST_REVIEWED: 2026-07-24 :END: +CLOSED: [2026-07-24 Fri] +Applied 2026-07-24. Cap and the unreachable notice helper removed; the gate is now count -ge 1. Adopted the rewritten bats suite (6 tests), with its hook path corrected from the installed .claude/hooks/ layout to the bundle's claude/hooks/ — the re-homing mismatch the sender flagged. + What arrived: a superseding handoff. The first proposed a loud notice when the test count exceeds =MAX_AUTO_TEST_FILES=20= (above the cap the block was skipped, nothing printed, exit 0 — indistinguishable from a pass). Craig chose in the .emacs.d session to remove the cap instead. The reasoning is the valuable part. Measured, a whole family runs in under two seconds (calendar-sync 63 files / 633 tests / 0.9s), so the cap bought nothing. Worse, it was *concealing* a real cross-test pollution bug: calendar-sync exits 1 when its 63 files run in one process, because one test marks a calendar as syncing and never resets it, and a sibling file's test then hits the stale guard. Invisible from both directions — =make test= runs each file in its own Emacs, and the hook skipped the family for being over the cap. @@ -85,7 +92,6 @@ Verified the superseding file drops the cap entirely (zero references, gate is n Since Craig already made this call, the remaining decision is only adoption scope: removing the cap means other consuming projects run every stem-matched test file per edit, and one may go red on first use by surfacing pollution that per-file runs hid. The sender flags that as intended. Prepared: [[file:working/hook-fail-open/validate-el.diff]], plus =test-validate-el-hook.bats= (6 tests, two of which stage a deliberately failing test so a quiet pass proves the run happened). -Say "approve the cap removal" and it gets applied. ** TODO [#B] cj-remove-block still can delete the WRONG cj block :bug: :PROPERTIES: -- cgit v1.2.3