fix(hooks): fail closed when pre-push scanner missing (#11)

README.md

@@ -42,6 +42,10 @@ The global mode sets `git config --global core.hooksPath ~/.config/git/hooks`. S
 its own local `core.hooksPath` overrides the global hook — install per-repo there. See memory
 `reference_global_security_review_hook`.
 
+If `review.sh` is missing or not executable at the configured path, the pre-push hook **fails closed**
+(nonzero exit) instead of skipping the gate. Repair by setting `SH_REVIEW_SH` to the scanner path, or
+reinstall with `./install-hooks.sh --global`.
+
 - `--global` also creates the machine-level suppressions dir
   (`${SH_SECURITY_SUPPRESSIONS_DIR:-~/.config/sea-haven/security-review}`): the per-repo file
   `<dir>/<repo-basename>/suppressions.json` is preferred by the hooks over repo-local

hooks/pre-push

@@ -8,26 +8,28 @@ set -uo pipefail
 REPO_ROOT="$(git rev-parse --show-toplevel 2>/dev/null)" || exit 0
 [ -f "$REPO_ROOT/.security-review-skip" ] && exit 0
 REVIEW_SH="${SH_REVIEW_SH:-$HOME/Documents/repositories/seahaven/security-review/review.sh}"
-if [ -f "$REVIEW_SH" ]; then
-  SUP=()
-  # Suppressions: prefer a MACHINE-LEVEL file kept out of repo history
-  # (<dir>/<repo-basename>/suppressions.json), else fall back to a repo-local
-  # .security-review/suppressions.json. Keyed by repo basename — adequate for the
-  # current single-namespace layout under ~/Documents/repositories.
-  MACHINE_SUP="${SH_SECURITY_SUPPRESSIONS_DIR:-$HOME/.config/sea-haven/security-review}/$(basename "$REPO_ROOT")/suppressions.json"
-  if [ -f "$MACHINE_SUP" ]; then
-    SUP=(--suppressions "$MACHINE_SUP")
-  elif [ -f "$REPO_ROOT/.security-review/suppressions.json" ]; then
-    SUP=(--suppressions "$REPO_ROOT/.security-review/suppressions.json")
-  fi
-  echo "security-review: scanning $REPO_ROOT (scanners-only) before push..." >&2
-  # ${SUP[@]+"${SUP[@]}"} = bash-3.2-safe expansion of a possibly-empty array under set -u.
-  if ! bash "$REVIEW_SH" --scanners-only ${SUP[@]+"${SUP[@]}"} "$REPO_ROOT"; then
-    echo "security-review: BLOCKED (confirmed crit/high). Fix it, suppress with justification, or 'git push --no-verify' to override." >&2
-    exit 1
-  fi
-else
-  echo "security-review: review.sh not found at $REVIEW_SH (set SH_REVIEW_SH) — skipping gate" >&2
+if [ ! -x "$REVIEW_SH" ]; then
+  echo "security-review: review.sh is missing or not executable at $REVIEW_SH" >&2
+  echo "Repair: set SH_REVIEW_SH to the scanner path, or reinstall with:" >&2
+  echo "  ${HOME}/Documents/repositories/seahaven/security-review/install-hooks.sh --global" >&2
+  exit 1
+fi
+SUP=()
+# Suppressions: prefer a MACHINE-LEVEL file kept out of repo history
+# (<dir>/<repo-basename>/suppressions.json), else fall back to a repo-local
+# .security-review/suppressions.json. Keyed by repo basename — adequate for the
+# current single-namespace layout under ~/Documents/repositories.
+MACHINE_SUP="${SH_SECURITY_SUPPRESSIONS_DIR:-$HOME/.config/sea-haven/security-review}/$(basename "$REPO_ROOT")/suppressions.json"
+if [ -f "$MACHINE_SUP" ]; then
+  SUP=(--suppressions "$MACHINE_SUP")
+elif [ -f "$REPO_ROOT/.security-review/suppressions.json" ]; then
+  SUP=(--suppressions "$REPO_ROOT/.security-review/suppressions.json")
+fi
+echo "security-review: scanning $REPO_ROOT (scanners-only) before push..." >&2
+# ${SUP[@]+"${SUP[@]}"} = bash-3.2-safe expansion of a possibly-empty array under set -u.
+if ! bash "$REVIEW_SH" --scanners-only ${SUP[@]+"${SUP[@]}"} "$REPO_ROOT"; then
+  echo "security-review: BLOCKED (confirmed crit/high). Fix it, suppress with justification, or 'git push --no-verify' to override." >&2
+  exit 1
 fi
 # Don't silently disable a repo-local pre-push hook: chain to it if present.
 LOCAL_HOOK="$REPO_ROOT/.git/hooks/pre-push"

install-hooks.sh

@@ -64,6 +64,7 @@ case "${1:-}" in
 
     echo
     echo "Done. Every repo on this machine is now gated by review.sh --scanners-only before push."
+    echo "A missing or non-executable review.sh fails the push closed; repair with this installer or SH_REVIEW_SH."
     echo "Caveats: a repo that sets its OWN local core.hooksPath (e.g. husky) overrides this global hook"
     echo "         — run 'install-hooks.sh <that-repo>' to gate it per-repo. Skip a repo with a"
     echo "         .security-review-skip file at its root; bypass once with 'git push --no-verify'."

tests/test_pre_push_hook.py

@@ -0,0 +1,99 @@
+"""Regression tests for the global pre-push security hook fail-closed behavior."""
+
+from __future__ import annotations
+
+import os
+import subprocess
+from pathlib import Path
+
+import pytest
+
+REPO_ROOT = Path(__file__).resolve().parents[1]
+PRE_PUSH = REPO_ROOT / "hooks" / "pre-push"
+
+
+@pytest.fixture
+def temp_git_repo(tmp_path: Path) -> Path:
+    """Minimal git repo so the hook's git rev-parse succeeds."""
+    repo = tmp_path / "repo"
+    repo.mkdir()
+    subprocess.run(["git", "init"], cwd=repo, check=True, capture_output=True)
+    subprocess.run(
+        ["git", "config", "user.email", "test@example.com"],
+        cwd=repo,
+        check=True,
+        capture_output=True,
+    )
+    subprocess.run(
+        ["git", "config", "user.name", "Test"],
+        cwd=repo,
+        check=True,
+        capture_output=True,
+    )
+    (repo / "README").write_text("x\n", encoding="utf-8")
+    subprocess.run(["git", "add", "README"], cwd=repo, check=True, capture_output=True)
+    subprocess.run(
+        ["git", "commit", "-m", "init"],
+        cwd=repo,
+        check=True,
+        capture_output=True,
+    )
+    return repo
+
+
+def _run_hook(
+    repo: Path, review_sh: str | Path, *, env_extra: dict[str, str] | None = None
+) -> subprocess.CompletedProcess[str]:
+    env = os.environ.copy()
+    env["SH_REVIEW_SH"] = str(review_sh)
+    if env_extra:
+        env.update(env_extra)
+    return subprocess.run(
+        ["bash", str(PRE_PUSH)],
+        cwd=repo,
+        env=env,
+        capture_output=True,
+        text=True,
+        check=False,
+    )
+
+
+def test_missing_scanner_fails_closed(temp_git_repo: Path, tmp_path: Path) -> None:
+    missing = tmp_path / "no-such-review.sh"
+    result = _run_hook(temp_git_repo, missing)
+    assert result.returncode == 1
+    assert str(missing) in result.stderr
+    assert "missing or not executable" in result.stderr
+    assert "install-hooks.sh --global" in result.stderr
+
+
+def test_non_executable_scanner_fails_closed(
+    temp_git_repo: Path, tmp_path: Path
+) -> None:
+    stub = tmp_path / "review.sh"
+    stub.write_text("#!/usr/bin/env bash\nexit 0\n", encoding="utf-8")
+    stub.chmod(0o644)
+    result = _run_hook(temp_git_repo, stub)
+    assert result.returncode == 1
+    assert str(stub) in result.stderr
+    assert "missing or not executable" in result.stderr
+    assert "install-hooks.sh --global" in result.stderr
+
+
+def test_scanner_success_allows_push(temp_git_repo: Path, tmp_path: Path) -> None:
+    stub = tmp_path / "review.sh"
+    stub.write_text("#!/usr/bin/env bash\nexit 0\n", encoding="utf-8")
+    stub.chmod(0o755)
+    result = _run_hook(temp_git_repo, stub)
+    assert result.returncode == 0
+    assert "BLOCKED" not in result.stderr
+    assert "missing or not executable" not in result.stderr
+
+
+def test_scanner_block_blocks_push(temp_git_repo: Path, tmp_path: Path) -> None:
+    stub = tmp_path / "review.sh"
+    stub.write_text("#!/usr/bin/env bash\nexit 1\n", encoding="utf-8")
+    stub.chmod(0o755)
+    result = _run_hook(temp_git_repo, stub)
+    assert result.returncode == 1
+    assert "BLOCKED" in result.stderr