aboutsummaryrefslogtreecommitdiff
path: root/working/hook-fail-open/rulesets-hook-note.txt
diff options
context:
space:
mode:
Diffstat (limited to 'working/hook-fail-open/rulesets-hook-note.txt')
-rw-r--r--working/hook-fail-open/rulesets-hook-note.txt31
1 files changed, 31 insertions, 0 deletions
diff --git a/working/hook-fail-open/rulesets-hook-note.txt b/working/hook-fail-open/rulesets-hook-note.txt
new file mode 100644
index 0000000..1c7a813
--- /dev/null
+++ b/working/hook-fail-open/rulesets-hook-note.txt
@@ -0,0 +1,31 @@
+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.