diff options
| author | Craig Jennings <c@cjennings.net> | 2026-07-24 12:28:18 -0500 |
|---|---|---|
| committer | Craig Jennings <c@cjennings.net> | 2026-07-24 12:28:18 -0500 |
| commit | f0c1bc40708615d5b423922c08c1f27e6cf96259 (patch) | |
| tree | 5fc8ebe7fde6381f4cd91b5de15fe4a2521889c8 /working/hook-fail-open/cap-note.txt | |
| parent | 781fa0786e2096797e630dcb81ffbc62ed98460b (diff) | |
| download | rulesets-f0c1bc40708615d5b423922c08c1f27e6cf96259.tar.gz rulesets-f0c1bc40708615d5b423922c08c1f27e6cf96259.zip | |
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.
Diffstat (limited to 'working/hook-fail-open/cap-note.txt')
| -rw-r--r-- | working/hook-fail-open/cap-note.txt | 35 |
1 files changed, 0 insertions, 35 deletions
diff --git a/working/hook-fail-open/cap-note.txt b/working/hook-fail-open/cap-note.txt deleted file mode 100644 index ba92101..0000000 --- a/working/hook-fail-open/cap-note.txt +++ /dev/null @@ -1,35 +0,0 @@ -SUPERSEDES the validate-el.sh I sent an hour ago (inbox 2026-07-24-0902). That version added a loud skip notice above the cap. Craig chose to remove the cap instead, and the reason is worth passing on. - -WHY THE CAP WENT - -The comment said "skip if more matches than this (large test suites)" — i.e. speed. Measured on this machine, running a whole family as the hook would: - - ai-term 35 files 1.0s 208 tests - music 45 files 1.7s 403 tests - calendar-sync 63 files 0.9s 633 tests - -Under two seconds each. The cap was not buying anything. - -Worse, it was concealing a real failure. calendar-sync exits 1 when its 63 files run in one process: test-calendar-sync--sync-calendar-skips-when-in-flight marks a calendar "proton" as syncing to exercise the dispatcher's in-flight guard, and never resets it — it cleared the module's state hash on entry but not on exit. test-calendar-sync--sync-dispatch-normal-ics-fetcher, in a sibling file, dispatches a calendar also named "proton", hits the leftover guard, and the dispatch it asserts never happens. ERT's alphabetical order puts the polluter first. - -That bug was invisible from both directions: `make test` runs each file in its own Emacs, and the hook skipped the family for being over the cap. So the cap was not just useless, it was hiding the one thing that would have caught the defect. - -CHANGES TO validate-el.sh (attached) - -- Removed MAX_AUTO_TEST_FILES and the upper bound on the gate; it is now `if [ "$count" -ge 1 ]`. -- Removed the notice_json helper and the over-cap elif from the earlier submission — with no cap they are unreachable, and leaving dead code in a hook whose whole problem was a silent path seemed wrong. - -The pre-commit fix from the earlier note is unaffected and still stands as sent. - -BEFORE YOU ADOPT THIS - -Removing the cap means the hook runs every stem-matched test file on each edit. Two things to weigh for other consuming projects: - -1. The timings above are this machine and this project. A project with genuinely slow tests (network, sleeps, big fixtures) could feel it. The :slow tag filter is already applied, which covers the usual offenders. -2. It surfaces cross-test pollution that per-file runs hide. That is a feature, but it means a project adopting this may go red on first use in a way it was not before. That is what happened here, and finding it was the point. - -I checked the other big families in this project as stem groups before removing the cap: ai-term, music, calendar-sync, dirvish and the org-agenda-config / org-agenda-frame groups all pass. There is a separate pollution bug between test-org-agenda-frame.el and test-org-agenda-config-commands.el, but no single stem matches both, so the hook never produces that combination. Filed locally. - -TESTS: test-validate-el-hook.bats attached, rewritten for the no-cap behavior (6 tests). Two of them stage a deliberately failing test file, so a quiet pass proves the run actually happened rather than being skipped — that is the property the old cap violated. - -No reply needed. |
