fix(hooks): three silent-failure bugs found during hook audit - #72
Merged
Conversation
rtk-rewrite: update to rtk's 4-value exit code protocol. The binary
embeds the protocol (0 allow, 1 no-equiv, 2 deny, 3 ask) but its own
--help text still documents the old `|| exit 0` contract, so the hook
was treating exit 3 ("rewrite available, ask user") as failure and
silently discarding every rewrite. Default `rtk` rules match common
commands (ls, git, find, ...) with ask, so input-side token savings
were effectively dead. New hook handles each exit code correctly and
maps rtk's ask decision to Claude Code's `permissionDecision: "ask"`.
security-patterns: fix $$ → $PPID for the per-session dedup file. $$
is the hook's own bash PID, which is brand new for every invocation,
so the "warn once per file+pattern per session" behavior was silently
disabled — every save wrote to a unique dedup file and re-fired the
same warning. $PPID is the Claude Code process that spawned the hook,
which is stable for the session.
post-validate: replace GNU-only `realpath -m` with portable absolute-
path conversion. On macOS BSD realpath has no -m flag, so the previous
`|| echo "$FILE_PATH"` fallback left relative paths unresolved, and
every benign `src/foo.go` save wrongly matched the "outside repo"
branch and emitted a noise warning. Also whitelists macOS's
/var/folders/* TMPDIR alongside /tmp.
Tests: 8 new cases covering (1) all four rtk exit codes via a PATH
shim so the tests are deterministic regardless of the installed rtk
version, (2) security-patterns dedup across two real invocations
(with a note on the bash $() subshell PPID gotcha so the test
topology matches production), (3) post-validate relative-in-repo
and absolute-out-of-repo path handling.
All 60 hook smoke tests pass; Go engine tests unchanged.
Two follow-ups on the rtk-rewrite hook based on PR #72 review and user feedback: 1. Collapse rtk exit 3 → "allow" instead of "ask". rtk's default rules match common commands (ls, git, find, …) with ask, so the previous behavior prompted on every Bash call. devkit's own safety-check.sh already fires earlier in the PreToolUse chain for destructive operations, so a second rtk-side prompt is redundant noise. Both rc 0 and rc 3 now auto-apply the rewrite. 2. Guard jq calls with `|| true`. Under `set -euo pipefail`, a jq failure on malformed stdin (or any CC protocol hiccup) would propagate non-zero out of the command substitution and CC would treat the hook as blocked. The hook now degrades to empty $COMMAND and exits 0 cleanly on bad input, matching pre-PR silent no-op behavior. Tests: 4 new cases pin the new contract and edge coverage: - rc 0 with empty stdout → no-op (pins the empty-rewrite guard) - rc unknown (42) → pass through (fail-open) - rewrite equal to input → no-op (pins the equality guard) - malformed JSON stdin → exit 0 silent no-op (regression test) Existing rc 3 test updated to assert decision="allow" instead of "ask". 64 hook tests pass, 0 failures.
This was referenced Apr 11, 2026
5uck1ess
added a commit
that referenced
this pull request
Apr 11, 2026
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).
4 tasks
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
1 participant
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
Three silent-failure bugs in the devkit hook stack that were all producing wrong behavior without any visible error. Found while diagnosing why
rtkrewrites never seemed to land.1.
rtk-rewrite.sh— every rewrite silently discardedThe hook was written against rtk's old contract (
|| exit 0fromrtk rewrite --help), but the binary has since evolved to a 4-value protocol that's embedded in the binary but not yet in--help:Default rtk rules match
ls,git,find, … with ask, so the binary was correctly producing rewrites and exiting 3, and the hook's|| exit 0was discarding every single one. Input-side token savings were effectively dead.New hook handles all four codes and maps rtk's ask signal to Claude Code's
permissionDecision: "ask".2.
security-patterns.sh— per-session dedup silently disabledSEEN_FILE="/tmp/devkit-security-seen-$$"used$$, which is the hook's own bash PID — a brand-new process every invocation. So the "warn once per file+pattern per session" behavior was broken: every save wrote to a unique dedup file and re-fired the same warning. Fixed by switching to$PPID(the Claude Code process that spawned the hook, stable per session).3.
post-validate.sh— false "outside repo" warning on every macOS saverealpath -mis a GNU coreutils flag — macOS BSDrealpathdoesn't have it. The|| echo "$FILE_PATH"fallback kept relative paths unresolved, sosrc/foo.gofailed the"$REPO_ROOT"/*glob match and generated noise warnings on every Edit/Write. Replaced with a portable shell-only absolute-path conversion, and also whitelisted/var/folders/*(macOSTMPDIR) alongside/tmp.Test plan
hooks/hooks_test.sh:$()subshell PPID gotcha so the test topology matches production)bash hooks/hooks_test.shsuite: 60 passed, 0 failed (was 52 before)go test ./...insrc/: all packages greenrtk0.35.0 on macOS: ask decisions now produce rewritten commands withpermissionDecision: "ask"as intended