fix(hooks): resolve #73 #74 #75 follow-ups (test harness, path handling, set -euo) - #76
Merged
Merged
Conversation
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).
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Bundles the three hook follow-up issues from the #72 review, since the fixes and tests cluster in the same files. Closes #73, #74, #75.
#73 —
run_hookdead exit-code checkhooks_test.shhadOUTPUT=$(…) || truefollowed byEXIT=$?, so$?always sawtrue's rc=0 and everyrun_hookcaller's exit-code assertion was dead code. The suite runs underset -uo pipefail(no-e), so the|| truewas unnecessary cargo — dropping it restores the real hook exit code. Every existingrun_hookcall already expects exit 0 and still passes, so no assertions needed tightening.#74 —
post-validate.shpath handling + test coverageNormalize both
REPO_ROOTand the constructedABS_PATHthrough a portable realpath pipeline before the glob match:REPO_ROOT→ canonicalized viacd && pwd -P(fixes symlinked-repo false positives).ABS_PATH→ python3 → python →cd dirname && pwd -P→ pure-sed./..collapse. The sed tier is the safety net when python is absent and the parent dir doesn't exist yet (pre-mkdir writes, fixtures)./private/var/folders/*because python'srealpathresolves/var → /private/varon macOS.Four new tests pin all four scenarios called out in the issue:
file_path=../in-repo.md→ no warningfile_path=../outside/foo.go→ warn (pins edge case Add scrape command, Jina Reader integration, enhanced PR pipeline #1)/var/folders/xx/foo.go→ no warning (pins the TMPDIR allowlist)#75 — three cleanup items
security-patterns.sh—set -euo pipefailwith|| trueguards on everyjqassignment and the dedupecho >> SEEN_FILE, matching the pattern established inrtk-rewrite.sh. (post-validate.shalready had it.)eval() on the same file used by the existing pickle dedup test. Guards against a key-collapse regression that would flatten dedup to "warn-once-ever".rtk-rewriterc 3 + rewrite == input test — new shim modeask-noop-rewriteexercises the intersection of the ask-suppression branch and the identity-rewrite guard.$()topology comment stays (it prevents a real future bug).Windows / Git Bash compatibility
Second commit adds:
C:\foo/C:/foo) in addition to/foo.python3 → python → cd/pwd → sedfallback chain so the fix works even on minimal Git Bash without python.SKIP:line) whenln -scan't create a real symlink (Windows Git Bash without developer mode, some FUSE mounts).Verified the sed fallback manually by shadowing
python3andpythonwith/bin/false: in-repo paths pass silently,../outsidepaths warn, absolute outside paths warn. On macOS.Test plan
bash hooks/hooks_test.sh→70 passed, 0 failedon macOS/bin/falseln -sis restricted)