From c591a6bb628072f67008ffe015dd2668afd92423 Mon Sep 17 00:00:00 2001 From: Tym Rabchuk Date: Sat, 11 Apr 2026 02:47:46 -0400 Subject: [PATCH 1/2] fix(hooks): resolve hook test/path/set-e follow-ups (#73 #74 #75) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three related fixes from the #72 review audit, bundled because the tests cluster in the same file. #73 — hooks_test.sh run_hook silently swallowed exit codes. `OUTPUT=$(…) || true` followed by `EXIT=$?` always saw true's rc=0, so every run_hook caller's exit-code assertion was dead. The suite runs under `set -uo pipefail` (no -e), so the `|| true` was unnecessary; drop it and `$?` now carries the hook's real exit. #74 — post-validate.sh path handling edge cases. Normalize both REPO_ROOT and ABS_PATH through a portable realpath path (python3 with a dirname/pwd fallback) so that: (a) `..` escapes from the repo root no longer match the "$REPO_ROOT"/* glob, (b) symlinked repo roots compare consistently, (c) cwd=subdir + ../in-repo paths still resolve inside the repo. Extend the TMPDIR allowlist to include /private/var/folders/* since realpath resolves /var → /private/var on macOS. New tests pin all four scenarios (subdir cwd, .. escape, TMPDIR, symlinked root). #75 — three cleanup items: 1. security-patterns.sh: add `set -euo pipefail` with the same jq-tolerance guards as rtk-rewrite.sh (post-validate.sh already had it). 2. Per-pattern dedup test: assert that a second, different pattern on the same file still warns, guarding against a key collapse that would flatten the dedup to warn-once-ever. 3. rtk-rewrite rc 3 + rewrite == input test: new shim mode exercises the intersection of the ask-suppression branch and the identity guard. 4. Comment trim: replace "pins the $$→$PPID fix" / "Pins the realpath -m GNU-ism fix…" archaeology with invariant phrasing. Results: 70 passed, 0 failed (was 66 passed, 0 failed). --- hooks/hooks_test.sh | 96 ++++++++++++++++++++++++++++++++++---- hooks/post-validate.sh | 39 ++++++++++++---- hooks/security-patterns.sh | 16 +++++-- 3 files changed, 127 insertions(+), 24 deletions(-) diff --git a/hooks/hooks_test.sh b/hooks/hooks_test.sh index fd1662a..3773888 100644 --- a/hooks/hooks_test.sh +++ b/hooks/hooks_test.sh @@ -45,7 +45,11 @@ fail() { FAIL=$((FAIL + 1)); ERRORS="$ERRORS\n FAIL: $1"; echo " FAIL: $1"; } run_hook() { local script="$1" input="$2" label="$3" expect_json="${4:-false}" - OUTPUT=$(echo "$input" | bash "$HOOK_DIR/$script" 2>/dev/null) || true + # Do NOT add `|| true` here: it would clobber $? with true's exit code + # and the EXIT check below would become dead. This suite runs under + # `set -uo pipefail` (no -e), so a non-zero exit in the command + # substitution does not abort the script — $? captures it faithfully. + OUTPUT=$(echo "$input" | bash "$HOOK_DIR/$script" 2>/dev/null) EXIT=$? # Hook must exit 0 @@ -125,6 +129,7 @@ case "$RTK_SHIM_MODE" in unknown) printf 'rtk %s' "$*"; exit 42 ;; empty-allow) exit 0 ;; noop-rewrite) printf '%s' "$*"; exit 0 ;; # rewrite equals input + ask-noop-rewrite) printf '%s' "$*"; exit 3 ;; # rc 3 (ask) + rewrite equals input *) exit 1 ;; esac SHIM @@ -192,6 +197,16 @@ else fail "rtk-rewrite: identity rewrite should no-op (got: $out)" fi +# rc 3 (ask) + rewrite == input → no-op. Exercises the intersection of +# the ask-suppression branch and the identity-rewrite guard; neither +# should emit a rewrite when the result would be a self-assignment. +out=$(rtk_shim_run ask-noop-rewrite '{"tool_input":{"command":"foo bar","description":""}}') +if [[ -z "$out" ]]; then + pass "rtk-rewrite: rc 3 + rewrite == input → no-op" +else + fail "rtk-rewrite: rc 3 identity rewrite should no-op (got: $out)" +fi + # Empty command → no-op out=$(rtk_shim_run allow '{"tool_input":{"command":"","description":""}}') if [[ -z "$out" ]]; then @@ -237,10 +252,9 @@ else fi # security-patterns.sh — dedup must survive across hook invocations. -# This pins the $$→$PPID fix: $$ is the hook's own bash PID, which is -# brand new every invocation, so dedup was silently disabled and the -# same warning fired on every save. In production Claude Code spawns -# the hook directly via fork+exec, so $PPID is the stable CC session PID. +# Invariant: a repeated (file, pattern) pair warns only on the first call +# within a CC session. Implementation detail: dedup keys off $PPID so the +# key is stable across fork+exec invocations of the hook. # # Topology gotcha: bash's $() command substitution wraps the command in # an intermediate subshell (a real fork), which then becomes the hook's @@ -263,6 +277,18 @@ else fail "security-patterns: dedup not working (first=$sec_first second=$sec_second)" fi +# Per-pattern dedup: a different pattern on the SAME file must still +# warn. Regression guard against a key collapse that flattens the dedup +# key to a global "warn-once-ever" flag. +SEC_PAYLOAD_EVAL='{"tool_name":"Write","tool_input":{"file_path":"dedup_test.py","content":"eval(user_input)"}}' +TMPDIR="$sec_tmpdir" bash "$HOOK_DIR/security-patterns.sh" <<< "$SEC_PAYLOAD_EVAL" \ + > "$sec_tmpdir/out3" 2>/dev/null || true +if [[ -s "$sec_tmpdir/out3" ]]; then + pass "security-patterns: dedup is per-pattern (different pattern, same file, still warns)" +else + fail "security-patterns: per-pattern dedup flattened to warn-once-ever" +fi + echo "" echo "=== PostToolUse Hooks ===" @@ -275,11 +301,8 @@ run_hook "post-validate.sh" \ run_hook "post-validate.sh" "" "post-validate: empty input" false # post-validate.sh — relative path inside the repo must NOT trigger the -# "outside repo" warning. Pins the realpath -m GNU-ism fix: on macOS the -# old implementation's fallback kept $FILE_PATH unresolved, so a benign -# "src/foo.go" failed the "$REPO_ROOT"/* match and generated a false -# warning on every Edit/Write. Run the hook from the repo root with a -# plain relative path and assert no warning is emitted. +# "outside repo" warning. Invariant: in-repo relative paths resolve +# against $(pwd) and match $REPO_ROOT portably on macOS and Linux. out=$(cd "$REPO_ROOT" && printf '%s' \ '{"tool_name":"Write","tool_input":{"file_path":"src/fake.go","content":"package main"}}' \ | bash "$HOOK_DIR/post-validate.sh" 2>/dev/null || true) @@ -299,6 +322,59 @@ else fail "post-validate: absolute out-of-repo path not flagged (got: $out)" fi +# post-validate.sh — cwd is a subdirectory of the repo, FILE_PATH uses +# "../foo" to reach a sibling that's still inside the repo. This is the +# common Claude Code topology (cwd = a subdir, paths written relative +# to it). The path must normalize back into REPO_ROOT, not warn. +out=$(cd "$REPO_ROOT/hooks" && printf '%s' \ + '{"tool_name":"Write","tool_input":{"file_path":"../README.md","content":"x"}}' \ + | bash "$HOOK_DIR/post-validate.sh" 2>/dev/null || true) +if [[ -z "$out" ]]; then + pass "post-validate: cwd=subdir + ../in-repo path → no warning" +else + fail "post-validate: subdir relative path wrongly flagged (got: $out)" +fi + +# post-validate.sh — .. escape from repo root. The resolved absolute +# path is OUTSIDE the repo and must warn. Before the path-normalization +# fix, "$REPO_ROOT/../sibling.go" matched the "$REPO_ROOT"/* glob and +# silently passed. +out=$(cd "$REPO_ROOT" && printf '%s' \ + '{"tool_name":"Write","tool_input":{"file_path":"../devkit-sibling-outside/foo.go","content":"x"}}' \ + | bash "$HOOK_DIR/post-validate.sh" 2>/dev/null || true) +if printf '%s' "$out" | jq -e '.hookSpecificOutput.additionalContext' >/dev/null 2>&1; then + pass "post-validate: .. escape from repo root → warn" +else + fail "post-validate: .. escape not flagged (got: $out)" +fi + +# post-validate.sh — macOS TMPDIR allowlist. /var/folders/... is the +# BSD TMPDIR and must not warn. Also confirms the /tmp allowlist. +out=$(printf '%s' \ + '{"tool_name":"Write","tool_input":{"file_path":"/var/folders/xx/foo.go","content":"package main"}}' \ + | bash "$HOOK_DIR/post-validate.sh" 2>/dev/null || true) +if [[ -z "$out" ]]; then + pass "post-validate: /var/folders (macOS TMPDIR) → no warning" +else + fail "post-validate: TMPDIR path wrongly flagged (got: $out)" +fi + +# post-validate.sh — symlinked repo root. When the user's cwd is +# reached via a symlink, REPO_ROOT (from git rev-parse) and $(pwd) can +# have different prefixes. The hook must normalize both sides so +# in-repo writes via the symlink path don't get mis-classified. +symlink_tmp=$(mktemp -d) +GUARD_TMPS+=("$symlink_tmp") +ln -s "$REPO_ROOT" "$symlink_tmp/link" +out=$(cd "$symlink_tmp/link" && printf '%s' \ + '{"tool_name":"Write","tool_input":{"file_path":"README.md","content":"x"}}' \ + | bash "$HOOK_DIR/post-validate.sh" 2>/dev/null || true) +if [[ -z "$out" ]]; then + pass "post-validate: cwd via symlink to repo → no warning" +else + fail "post-validate: symlinked repo root wrongly flagged (got: $out)" +fi + # slop-detect.sh — should allow clean code run_hook "slop-detect.sh" \ '{"tool_name":"Write","tool_input":{"file_path":"test.go","content":"func Add(a, b int) int { return a + b }"}}' \ diff --git a/hooks/post-validate.sh b/hooks/post-validate.sh index f513744..84c04ac 100755 --- a/hooks/post-validate.sh +++ b/hooks/post-validate.sh @@ -50,25 +50,44 @@ if [ "$TOOL_NAME" = "Edit" ] || [ "$TOOL_NAME" = "Write" ]; then fi fi - # Check for writes outside the git repo. - # realpath -m is GNU-only (not on macOS BSD realpath), so the previous - # implementation silently fell back to the raw FILE_PATH and mis-classified - # every relative path on macOS as "outside repo." Do a portable absolute-path - # conversion instead: absolute inputs stay as-is, relative inputs are - # prefixed with pwd. We don't normalize "." / ".." — the glob match still - # works for in-repo relative paths, and ..-escapes will (correctly) not match. + # Check for writes outside the git repo. Must resolve .. segments and + # symlinks so REPO_ROOT and ABS_PATH compare against a common canonical + # form — GNU `realpath -m` isn't available on macOS BSD realpath, so we + # do it portably here. if [ -n "$FILE_PATH" ]; then REPO_ROOT=$(git rev-parse --show-toplevel 2>/dev/null || true) + if [ -n "$REPO_ROOT" ]; then + # Canonicalize REPO_ROOT so a symlinked repo path (e.g. /Users/x/dev + # → /Volumes/Work/dev) matches $(pwd -P) inside the repo. + REPO_ROOT=$(cd "$REPO_ROOT" 2>/dev/null && pwd -P) || REPO_ROOT="" + fi if [ -n "$REPO_ROOT" ]; then case "$FILE_PATH" in /*) ABS_PATH="$FILE_PATH" ;; *) ABS_PATH="$(pwd)/$FILE_PATH" ;; esac + # Normalize the absolute path: resolve .. / . and symlinks. Prefer + # python3 (ubiquitous on macOS/Linux); fall back to a dirname+pwd + # trick which works whenever the parent directory exists (the common + # case for Write/Edit since the parent must already exist). + if command -v python3 >/dev/null 2>&1; then + NORMALIZED=$(python3 -c 'import os,sys; print(os.path.realpath(sys.argv[1]))' "$ABS_PATH" 2>/dev/null || true) + else + NORMALIZED="" + _dir=$(dirname -- "$ABS_PATH") + _base=$(basename -- "$ABS_PATH") + if [ -d "$_dir" ]; then + NORMALIZED="$(cd -- "$_dir" && pwd -P)/$_base" + fi + fi + [ -n "$NORMALIZED" ] && ABS_PATH="$NORMALIZED" case "$ABS_PATH" in - "$REPO_ROOT"/*) + "$REPO_ROOT"/*|"$REPO_ROOT") ;; # within repo, OK - /tmp/*|/private/tmp/*|/var/folders/*) - ;; # temp files (/var/folders/* is macOS TMPDIR), OK + /tmp/*|/private/tmp/*|/var/folders/*|/private/var/folders/*) + ;; # temp files (/var/folders is macOS TMPDIR; /private/var/folders + # is its realpath-resolved form since /var is a symlink to + # /private/var on macOS), OK *) jq -n --arg file "$FILE_PATH" --arg repo "$REPO_ROOT" '{ hookSpecificOutput: { diff --git a/hooks/security-patterns.sh b/hooks/security-patterns.sh index 76c1f3a..4d23304 100755 --- a/hooks/security-patterns.sh +++ b/hooks/security-patterns.sh @@ -5,10 +5,16 @@ # Catches security anti-patterns at the moment of creation rather than in a later review. # Warns once per file+pattern per session to avoid spam. +set -euo pipefail + +# jq failures on malformed input must not abort the hook — the hook's +# contract is "fail open" (never block Claude Code on parse errors), +# so every jq pipeline gets a `|| true` tolerance guard, matching the +# pattern used in rtk-rewrite.sh. INPUT=$(cat) -TOOL_NAME=$(echo "$INPUT" | jq -r '.tool_name // empty') -FILE_PATH=$(echo "$INPUT" | jq -r '.tool_input.file_path // empty') -NEW_STRING=$(echo "$INPUT" | jq -r '.tool_input.new_string // .tool_input.content // empty') +TOOL_NAME=$(echo "$INPUT" | jq -r '.tool_name // empty' 2>/dev/null || true) +FILE_PATH=$(echo "$INPUT" | jq -r '.tool_input.file_path // empty' 2>/dev/null || true) +NEW_STRING=$(echo "$INPUT" | jq -r '.tool_input.new_string // .tool_input.content // empty' 2>/dev/null || true) # Only check Edit and Write [ "$TOOL_NAME" = "Edit" ] || [ "$TOOL_NAME" = "Write" ] || exit 0 @@ -32,7 +38,9 @@ check_pattern() { if [ -f "$SEEN_FILE" ] && grep -qF "$key" "$SEEN_FILE" 2>/dev/null; then return fi - echo "$key" >> "$SEEN_FILE" 2>/dev/null + # Under set -e, a failed write would abort the hook and turn a + # would-be "ask" warning into a hard exit. Tolerate failures. + echo "$key" >> "$SEEN_FILE" 2>/dev/null || true jq -n --arg reason "$message" '{ hookSpecificOutput: { From 317f183480198bb12923c7f3878725e11c0e26a2 Mon Sep 17 00:00:00 2001 From: Tym Rabchuk Date: Sat, 11 Apr 2026 02:50:24 -0400 Subject: [PATCH 2/2] fix(hooks): windows/git-bash compatibility for post-validate path fix MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three additions so the #74 path-handling fix works on Windows Git Bash and minimal shells that lack python: 1. Absolute-path detection accepts drive-letter forms (C:\foo, C:/foo) in addition to POSIX /foo paths, in case Claude Code emits native-Windows tool_input paths. 2. Path normalization tries python3 → python → cd/pwd → sed-based ./../ collapse. The sed fallback is pure bash + POSIX sed -E so it survives when python is absent AND the parent directory doesn't exist yet (common for Write that creates a new file's parent). Verified manually by shadowing python3/python with /bin/false. 3. The new symlinked-repo-root test skips cleanly (with a SKIP line) on platforms where `ln -s` can't create a real symlink, e.g. Windows Git Bash without developer mode or restricted FUSE mounts. Test suite still: 70 passed, 0 failed on macOS. --- hooks/hooks_test.sh | 19 ++++++++++++------- hooks/post-validate.sh | 37 +++++++++++++++++++++++++++++++------ 2 files changed, 43 insertions(+), 13 deletions(-) diff --git a/hooks/hooks_test.sh b/hooks/hooks_test.sh index 3773888..08db33f 100644 --- a/hooks/hooks_test.sh +++ b/hooks/hooks_test.sh @@ -363,16 +363,21 @@ fi # reached via a symlink, REPO_ROOT (from git rev-parse) and $(pwd) can # have different prefixes. The hook must normalize both sides so # in-repo writes via the symlink path don't get mis-classified. +# Skipped on platforms where `ln -s` doesn't create a real symlink +# (Windows Git Bash without developer mode, some FUSE mounts, etc.). symlink_tmp=$(mktemp -d) GUARD_TMPS+=("$symlink_tmp") -ln -s "$REPO_ROOT" "$symlink_tmp/link" -out=$(cd "$symlink_tmp/link" && printf '%s' \ - '{"tool_name":"Write","tool_input":{"file_path":"README.md","content":"x"}}' \ - | bash "$HOOK_DIR/post-validate.sh" 2>/dev/null || true) -if [[ -z "$out" ]]; then - pass "post-validate: cwd via symlink to repo → no warning" +if ln -s "$REPO_ROOT" "$symlink_tmp/link" 2>/dev/null && [ -L "$symlink_tmp/link" ]; then + out=$(cd "$symlink_tmp/link" && printf '%s' \ + '{"tool_name":"Write","tool_input":{"file_path":"README.md","content":"x"}}' \ + | bash "$HOOK_DIR/post-validate.sh" 2>/dev/null || true) + if [[ -z "$out" ]]; then + pass "post-validate: cwd via symlink to repo → no warning" + else + fail "post-validate: symlinked repo root wrongly flagged (got: $out)" + fi else - fail "post-validate: symlinked repo root wrongly flagged (got: $out)" + echo " SKIP: post-validate: cwd via symlink (ln -s unavailable on this platform)" fi # slop-detect.sh — should allow clean code diff --git a/hooks/post-validate.sh b/hooks/post-validate.sh index 84c04ac..5476a0e 100755 --- a/hooks/post-validate.sh +++ b/hooks/post-validate.sh @@ -62,24 +62,49 @@ if [ "$TOOL_NAME" = "Edit" ] || [ "$TOOL_NAME" = "Write" ]; then REPO_ROOT=$(cd "$REPO_ROOT" 2>/dev/null && pwd -P) || REPO_ROOT="" fi if [ -n "$REPO_ROOT" ]; then + # Absolute-path detection handles POSIX (/foo) and Windows + # drive-letter (C:\foo or C:/foo) forms. On Git Bash / WSL the + # paths are usually MSYS-style (/c/Users/...) which already + # matches the /* arm, but Claude Code may also pass native + # Windows paths depending on how the tool_input was produced. case "$FILE_PATH" in - /*) ABS_PATH="$FILE_PATH" ;; + /*|[A-Za-z]:[/\\]*) ABS_PATH="$FILE_PATH" ;; *) ABS_PATH="$(pwd)/$FILE_PATH" ;; esac # Normalize the absolute path: resolve .. / . and symlinks. Prefer - # python3 (ubiquitous on macOS/Linux); fall back to a dirname+pwd - # trick which works whenever the parent directory exists (the common - # case for Write/Edit since the parent must already exist). + # python3, fall back to python (Windows often ships just `python`), + # then to a dirname+pwd trick which works whenever the parent + # directory exists (the common case for Write/Edit since the + # parent must already exist). + NORMALIZED="" if command -v python3 >/dev/null 2>&1; then NORMALIZED=$(python3 -c 'import os,sys; print(os.path.realpath(sys.argv[1]))' "$ABS_PATH" 2>/dev/null || true) - else - NORMALIZED="" + elif command -v python >/dev/null 2>&1; then + NORMALIZED=$(python -c 'import os,sys; print(os.path.realpath(sys.argv[1]))' "$ABS_PATH" 2>/dev/null || true) + fi + if [ -z "$NORMALIZED" ]; then _dir=$(dirname -- "$ABS_PATH") _base=$(basename -- "$ABS_PATH") if [ -d "$_dir" ]; then NORMALIZED="$(cd -- "$_dir" && pwd -P)/$_base" fi fi + if [ -z "$NORMALIZED" ]; then + # Final fallback: collapse ./ and ../ segments with sed. Won't + # resolve symlinks, but preserves the outside-repo detection + # when neither python nor the parent directory is available + # (pre-mkdir writes, minimal Git Bash installs, etc.). + NORMALIZED="$ABS_PATH" + while : ; do + _prev="$NORMALIZED" + NORMALIZED=$(printf '%s' "$NORMALIZED" | sed -E \ + -e 's|/\./|/|g' \ + -e 's|/[^/]+/\.\./|/|g' \ + -e 's|/[^/]+/\.\.$||' \ + -e 's|/\.$||') + [ "$NORMALIZED" = "$_prev" ] && break + done + fi [ -n "$NORMALIZED" ] && ABS_PATH="$NORMALIZED" case "$ABS_PATH" in "$REPO_ROOT"/*|"$REPO_ROOT")