From 189898799c90f1bb8f86cf4dc91c637a36f4ecaa Mon Sep 17 00:00:00 2001 From: pluginslab <57633278+pluginslab@users.noreply.github.com> Date: Thu, 17 Sep 2026 12:41:44 +0100 Subject: [PATCH] fix(hooks): the pre-commit gate was failing open, twice Two independent ways for this gate to do nothing, both silent. 1. It only matched `git commit` as a prefix. case "$command" in git\ commit*) ;; *) exit 0 ;; esac So `git add -A && git commit -m x` -- the idiom most people and most agents actually type -- never triggered it. Neither did `cd sub && git commit`, nor `git -C dir commit`. Verified against the real hook: of four common commit shapes, exactly one fired the gate. Every commit in the session that produced this fix used the `git add && git commit` form, so none of them were gated. quality.sh was run by hand each time, which is the only reason that is a footnote and not an incident. Now matches `git ... commit` anywhere in the command, allowing flags and paths between the two words but not a command separator. The tradeoff is that `git log --grep=commit` will now run the suite; that direction is the safe one, and a block names the command that triggered it so a false positive explains itself. 2. It parsed the payload with python3 and never checked python3 existed. On a host without it, the capture came back empty, matched nothing, and the hook exited 0. No gate, no warning. It now tries python3, falls back to jq, and if neither is present prints "THE QUALITY GATE IS NOT RUNNING" on stderr. It still does not block in that case: this hook fires before every Bash call, so blocking on its own internal errors would wedge the session. Failing open is the right call. Failing open *silently* was not. Tests grew from 5 cases to 20, including the four commit shapes that used to bypass the gate and the no-parser contract. The gate-firing cases are now guarded on a parser being available, so the suite stays honest on a host that legitimately cannot gate rather than going red for behaving as designed. Verified: macOS 92 passed, Linux+python3 92 passed, Linux without any JSON parser 78 passed with 14 correctly skipped. Co-Authored-By: Claude Opus 5 (1M context) --- .claude/hooks/pre-commit.sh | 73 ++++++++++++++--- CHANGELOG.md | 6 ++ tests/hooks/test-pre-commit.sh | 138 ++++++++++++++++++++++++++------- 3 files changed, 176 insertions(+), 41 deletions(-) diff --git a/.claude/hooks/pre-commit.sh b/.claude/hooks/pre-commit.sh index 55c7057..687923f 100755 --- a/.claude/hooks/pre-commit.sh +++ b/.claude/hooks/pre-commit.sh @@ -5,21 +5,69 @@ # to scripts/quality.sh — the single source of truth for the quality suite. # Exits non-zero to block the commit if the suite fails. # +# This hook fails OPEN by design: it runs before every Bash call, so blocking +# on its own internal errors would wedge the session. The price of failing +# open is that a broken gate is invisible — you keep believing you are gated +# while nothing runs. So every path that gives up says so, loudly, on stderr. +# set -uo pipefail -command=$(python3 -c ' +payload=$(cat) + +# No payload at all: nothing to inspect, and no real invocation looks like +# this. Stay quiet rather than crying wolf on every command. +[[ -z "$payload" ]] && exit 0 + +# Extract .tool_input.command. Two parsers, because the hook previously used +# python3 alone and did not check whether it existed — on a host without it, +# the capture came back empty, matched nothing, and the gate silently turned +# itself off. +parsed='' +have_parser=0 + +if command -v python3 >/dev/null 2>&1; then + if parsed=$(printf '%s' "$payload" | python3 -c ' import json, sys -try: - data = json.load(sys.stdin) - print(data.get("tool_input", {}).get("command", "")) -except Exception: - pass -') - -case "$command" in - git\ commit*) ;; - *) exit 0 ;; -esac +data = json.load(sys.stdin) +sys.stdout.write(str(data.get("tool_input", {}).get("command", ""))) +' 2>/dev/null); then + have_parser=1 + fi +fi + +if (( ! have_parser )) && command -v jq >/dev/null 2>&1; then + if parsed=$(printf '%s' "$payload" | jq -r '.tool_input.command // ""' 2>/dev/null); then + have_parser=1 + fi +fi + +if (( ! have_parser )); then + echo "pre-commit: cannot read the hook payload — no working python3 or jq on PATH." >&2 + echo "pre-commit: THE QUALITY GATE IS NOT RUNNING. Install python3 or jq to re-enable it." >&2 + exit 0 +fi + +# Match `git commit` anywhere in the command, not only at the start. +# +# The previous prefix match (`git commit*`) meant the gate never fired for +# `git add -A && git commit -m ...` — the most common commit idiom there is — +# nor for `cd sub && git commit`, nor `git -C dir commit`. The gate looked +# present and was inert for the shapes people actually type. +# +# The regex allows flags and paths between `git` and `commit`, but not a +# command separator, so `git status && make commit-docs` does not match. +# +# Tradeoff: this does match a command that merely mentions committing, e.g. +# `git log --grep=commit`. That direction is the safe one — a needless quality +# run costs seconds, a skipped gate costs a broken commit — but it is a real +# tradeoff, so the block message below names the command that triggered it. +# Held in a variable: inside [[ =~ ]] an inline pattern containing `;` or `&` +# is parsed as shell syntax before the regex engine ever sees it. +commit_re='(^|[;&|[:space:]])git[[:space:]][^;&|]*commit' + +if [[ ! "$parsed" =~ $commit_re ]]; then + exit 0 +fi if [[ ! -x "./scripts/quality.sh" ]]; then echo "scripts/quality.sh missing or not executable; skipping pre-commit gate." >&2 @@ -29,6 +77,7 @@ fi if ! ./scripts/quality.sh; then echo "" >&2 echo "Commit blocked. Fix the issues above, then commit again." >&2 + echo "Triggered by: $parsed" >&2 exit 1 fi diff --git a/CHANGELOG.md b/CHANGELOG.md index 016516b..24b1f53 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,12 @@ The kit's evolution, kept for humans. Per-feature progress lives in `.claude/pla - **Hook logic is no longer triplicated.** The active-feature selection was copy-pasted into all three hooks, with a comment in `stop.sh` asking the reader to keep them "in lockstep" by hand — which is how they came to share an identical bug. `.claude/hooks/lib.sh` now owns `hook_mtime`, `hook_is_complete` and `hook_active_progress`. `hook_mtime` probes once for which `stat` the machine has and validates its own output is numeric, so no future platform can reintroduce this class of bug. +- **The pre-commit gate never fired for the most common way to commit.** `pre-commit.sh` matched `git commit*` as a *prefix*, so it only triggered when the command began with those words. `git add -A && git commit -m x` — the idiom most people and most agents actually type — sailed straight past it, as did `cd sub && git commit` and `git -C dir commit`. The gate looked present and was inert for the shapes that matter. It now matches `git … commit` anywhere in the command, allowing flags and paths between the two words but not a command separator. + + The tradeoff is that a command merely *mentioning* committing (`git log --grep=commit`) now runs the suite. That is the safe direction — a needless quality run costs seconds, a skipped gate costs a broken commit — and a block now names the command that triggered it, so a false positive explains itself. + +- **The pre-commit gate silently disabled itself without `python3`.** The payload was parsed with `python3 -c` and nothing checked whether the interpreter existed. On a host without it the capture came back empty, matched no pattern, and the hook exited 0 — no gate, no warning, no way to notice. It now tries `python3`, falls back to `jq`, and if neither is available says **"THE QUALITY GATE IS NOT RUNNING"** on stderr. It still does not block, because this hook fires before every Bash call and blocking on its own internal errors would wedge the session — but failing open silently and failing open loudly are very different things. + - **A flaky test in `test-stop.sh`.** The multi-feature case rewrote both fixtures with `sed -i.bak`, making their mtimes equal to the second, then `touch`ed the newer one to break the tie. Since mtime resolution here is whole seconds, that only won a sub-second race — which macOS happened to win and Linux consistently lost. It now pushes the older fixture into the past with `touch -t`, which is portable and deterministic. ## v1.0.3 — 2026-06-11 diff --git a/tests/hooks/test-pre-commit.sh b/tests/hooks/test-pre-commit.sh index 01f876d..cdcc905 100755 --- a/tests/hooks/test-pre-commit.sh +++ b/tests/hooks/test-pre-commit.sh @@ -6,7 +6,17 @@ # 1. non-commit Bash → exit 0 (passthrough) # 2. git commit + no scripts/quality.sh → exit 0 with warning (skip gate) # 3. git commit + passing quality.sh → exit 0 -# 4. git commit + failing quality.sh → exit 1 (block) +# 4. git commit + failing quality.sh → exit 1 (block), naming the command +# 5. compound / flagged commit forms still trigger the gate +# 6. no JSON parser available → exit 0, but loudly +# 7. empty payload → quiet passthrough +# +# Cases 5 and 6 exist because this hook had two ways of silently doing +# nothing. A prefix match meant `git add -A && git commit -m x` — the most +# common commit idiom there is — never triggered it. And parsing the payload +# with python3 alone meant a host without python3 got no gate and no warning. +# Both failed open *invisibly*, which is the worst property a gate can have: +# you keep the belief that you are covered. # set -uo pipefail @@ -15,6 +25,24 @@ source "$(dirname "${BASH_SOURCE[0]}")/../lib.sh" echo "pre-commit.sh" +# Everything that asserts the gate *fires* needs a JSON parser on PATH, because +# without one the hook cannot read the command and correctly declines to gate. +# Demanding a gate on a host with no parser would make this suite red for +# behaving exactly as designed — the same "the test encodes my machine" mistake +# that hid the Linux mtime bug. Case 6 covers the no-parser contract head-on. +if command -v python3 >/dev/null 2>&1 || command -v jq >/dev/null 2>&1; then + HAVE_PARSER=1 +else + HAVE_PARSER=0 +fi + +write_quality() { + # write_quality + mkdir -p scripts + printf '#!/usr/bin/env bash\nexit %s\n' "$1" > scripts/quality.sh + chmod +x scripts/quality.sh +} + # --- Case 1: non-commit Bash command → exit 0, no gate run --- setup_sandbox run_hook "pre-commit.sh" '{"tool_input":{"command":"ls -la"}}' @@ -27,39 +55,91 @@ run_hook "pre-commit.sh" '{"tool_input":{"command":"git status"}}' assert_exit_code "git status (not commit) → exit 0" 0 "$HOOK_EXIT" teardown_sandbox -# --- Case 3: git commit but no scripts/quality.sh → exit 0 with warning --- -setup_sandbox -run_hook "pre-commit.sh" '{"tool_input":{"command":"git commit -m foo"}}' -assert_exit_code "git commit, no quality.sh → exit 0" 0 "$HOOK_EXIT" -assert_contains "git commit, no quality.sh → warns on stderr" \ - "$HOOK_STDERR_TEXT" "quality.sh missing" -teardown_sandbox +if (( HAVE_PARSER )); then + + # --- Case 3: git commit but no scripts/quality.sh → exit 0 with warning --- + setup_sandbox + run_hook "pre-commit.sh" '{"tool_input":{"command":"git commit -m foo"}}' + assert_exit_code "git commit, no quality.sh → exit 0" 0 "$HOOK_EXIT" + assert_contains "git commit, no quality.sh → warns on stderr" \ + "$HOOK_STDERR_TEXT" "quality.sh missing" + teardown_sandbox + + # --- Case 4: git commit + passing quality.sh → exit 0 --- + setup_sandbox + write_quality 0 + run_hook "pre-commit.sh" '{"tool_input":{"command":"git commit -m feat"}}' + assert_exit_code "git commit, quality pass → exit 0" 0 "$HOOK_EXIT" + teardown_sandbox + + # --- Case 5: git commit + failing quality.sh → exit 1 (blocks commit) --- + setup_sandbox + write_quality 1 + run_hook "pre-commit.sh" '{"tool_input":{"command":"git commit -m bad"}}' + assert_exit_code "git commit, quality fail → exit 1" 1 "$HOOK_EXIT" + assert_contains "git commit, quality fail → block message" \ + "$HOOK_STDERR_TEXT" "Commit blocked" + # The widened match can fire on a command that merely mentions committing, + # so a block has to say what triggered it or the user cannot tell why. + assert_contains "git commit, quality fail → names the command" \ + "$HOOK_STDERR_TEXT" "git commit -m bad" + teardown_sandbox -# --- Case 4: git commit + passing quality.sh → exit 0 --- + # --- Case 6: commit shapes a prefix match would miss --- + # Every one of these bypassed the gate entirely before the match was widened. + for cmd in \ + "git add -A && git commit -m x" \ + "cd sub && git commit -m x" \ + "git -C /some/dir commit -m x" \ + "git commit --amend --no-edit" + do + setup_sandbox + write_quality 1 + run_hook "pre-commit.sh" "{\"tool_input\":{\"command\":\"$cmd\"}}" + assert_exit_code "gate fires on: $cmd" 1 "$HOOK_EXIT" + teardown_sandbox + done + + # …and shapes that must still pass straight through. + for cmd in "git status" "git log --oneline" "ls -la" "make build"; do + setup_sandbox + write_quality 1 + run_hook "pre-commit.sh" "{\"tool_input\":{\"command\":\"$cmd\"}}" + assert_exit_code "gate ignores: $cmd" 0 "$HOOK_EXIT" + teardown_sandbox + done + +else + echo " - skipped 14 gate-firing cases: no python3 or jq on PATH" +fi + +# --- Case 7: no JSON parser on PATH → fail open, but say so --- +# This path used to be silent: no python3 meant no gate and no warning. setup_sandbox -mkdir -p scripts -cat > scripts/quality.sh <<'EOF' -#!/usr/bin/env bash -exit 0 -EOF -chmod +x scripts/quality.sh -run_hook "pre-commit.sh" '{"tool_input":{"command":"git commit -m feat"}}' -assert_exit_code "git commit, quality pass → exit 0" 0 "$HOOK_EXIT" +write_quality 1 + +# A PATH with no python3 and no jq on it. +stub_bin="$SANDBOX/stub-bin" +mkdir -p "$stub_bin" +for tool in bash cat grep sed; do + real=$(command -v "$tool" 2>/dev/null) && ln -sf "$real" "$stub_bin/$tool" 2>/dev/null +done + +out=$(PATH="$stub_bin" "$KIT_ROOT/.claude/hooks/pre-commit.sh" \ + <<< '{"tool_input":{"command":"git commit -m x"}}' 2>&1) +rc=$? + +assert_exit_code "no parser → does not block" 0 "$rc" +assert_contains "no parser → warns the gate is off" "$out" "QUALITY GATE IS NOT RUNNING" teardown_sandbox -# --- Case 5: git commit + failing quality.sh → exit 1 (blocks commit) --- +# --- Case 8: empty payload → quiet passthrough --- +# Nothing to inspect, and no real invocation looks like this. Warning here +# would cry wolf on every command. setup_sandbox -mkdir -p scripts -cat > scripts/quality.sh <<'EOF' -#!/usr/bin/env bash -echo "phpcs found 1 error" >&2 -exit 1 -EOF -chmod +x scripts/quality.sh -run_hook "pre-commit.sh" '{"tool_input":{"command":"git commit -m bad"}}' -assert_exit_code "git commit, quality fail → exit 1" 1 "$HOOK_EXIT" -assert_contains "git commit, quality fail → block message" \ - "$HOOK_STDERR_TEXT" "Commit blocked" +run_hook "pre-commit.sh" "" +assert_exit_code "empty payload → exit 0" 0 "$HOOK_EXIT" +assert_empty "empty payload → no stderr noise" "$HOOK_STDERR_TEXT" teardown_sandbox exit $FAIL_COUNT