perf(guardrails): block-hook-bypass drops six forks that never exec (#3513)

[claude]

Closes #3513

Summary

block-hook-bypass.sh was the largest process creator of any guard in the plugin on a benign Bash call: 7 creations, 0 execs. Every exec-based census (the PATH-shim spawn census, run-guards.test.sh's dirname/sed pin, an xtrace command-position count) reads a fork-without-exec as free, which is why the issue's numbers and the "per-field jq" cause never matched what the kernel does. Six of the seven were in this file; the seventh is $(hook::buffer_stdin).

Measured with strace -f -e trace=clone,clone3,fork,vfork,execve on the real dispatched path (run-guards.sh block-hook-bypass.sh), HOOK_TELEMETRY_SINK unset, guard share = count minus a no-op guard dispatched the same way, three identical repeats:

Path	creations before -> after	execve before -> after
Guard share, benign git status --short	7 -> 1	0 -> 0
Guard share, blocked echo hi > notes.md	10 -> 5	1 -> 1
Whole Bash dispatcher, benign	36 -> 30	3 -> 3
Guard alone under the dispatcher, wall p50 / p95 (n=20, interleaved)	27.4 / 29.2 ms -> 24.3 / 27.5 ms
Whole Bash dispatcher, wall p50 / p95 (n=20, interleaved)	51.6 / 60.4 ms -> 48.2 / 49.6 ms

The execve column does not move: this is latency, not removed work.

On the issue's "at most two spawns" line. The one creation left in the guard's own share is $(hook::buffer_stdin). Its fork-free form (hook::buffer_stdin_to <var>) belongs to lib/hook-utils.sh, which is fenced by unmerged #3740 and #3838, so it is not touched here. Whether #3513 counts as met depends on what one counts: the guard's in-file contribution is now 1 creation and 0 execs, and the dispatcher's own $(source …) isolation fork is the dispatcher's, not this file's. Closes here means the in-file work is done; the remaining fork is the hook-utils item.

Fix

Five in-file creations removed, one left in place:

1. :159 eager SUBJECT=$(hook::extract_bash_subject …) at file scope. Only emit_tel read it, and emit_tel is gated on the start stamp and the opt-in sink. The subject is now derived inside emit_tel behind both gates. Verdicts never read it.
2. :478 EXECUTABLE=$(strip_literals "$COMMAND"). Now strip_literals_to EXECUTABLE "$COMMAND", assigning through a nameref; every trailing newline is stripped, as $(…) stripped them.
3. :474, :1305, :1330, :1403 the four done < <(printf '%s\n' …) loops. One fork-free splitter, split_lines_to, does a sentinel-prefixed IFS=$'\n' word split under set -f (globbing restored to whatever the caller had). The sentinel prefix is what keeps blank lines and runs of newlines from collapsing, and a leading or trailing empty line from disappearing, so the array is exactly what read delivered, which strip_literals needs to keep its heredoc and open-quote state aligned with physical lines. NORMALIZED_SEGMENTS becomes an array filled once in normalize_segments; the three per-segment scans iterate it. return and continue 2 inside those loops reach the same scopes as before because neither loop shape runs its body in a subshell. A here-string was not an option: at 65536-65663 bytes bash blocks forever writing it into the pipe (documented in lib/path-detection/hardcoded-path-patterns.sh).
4. :112 INPUT=$(hook::buffer_stdin) is left as is. It is the fenced hook-utils item, and fix(hook-utils): a hook payload cut short at EOF is a loud allow; a stall stays a block #3740 modifies this exact rc-handling block; the diff stays off it so the two do not fight at merge.

Manifest 0.32.10 -> 0.32.12, CHANGELOG entry, README "Hook budget accounting" entry with the method and table above. 0.32.11 is taken by #3849 (issue #3511), which bumps the same manifest off the same 0.32.10 base; this branch takes the next free number so the two do not collide on plugins/guardrails/CHANGELOG.md.

Two documentation corrections carried in the same renumber commit:

• The CHANGELOG entry and the new README row said "the 602-case contract suite passes". With the pins this change set adds the suite is 611 cases, which is what it reports.
• The comment moved onto emit_tel carried two em dashes over from the file-scope comment it replaced. .claude/rules/vendor-docs-are-not-style.md bars them from this repo's instruction surfaces, and check-purged-em-dashes.sh scans markdown only, so nothing caught them. They are parentheses now. No other comment is touched, and no guard logic changed.

Verification

• Deny paths still deny, compared against origin/main. 244 paired runs (61 commands x Bash/PowerShell payloads x standalone/dispatched, plus a 70 KiB single-line command and a 3000-line command on each side) agree on exit code and first stderr line. The corpus covers every documented deny form (cat/echo/printf redirects, no-space forms, heredoc openers with a trailing redirect, 2>&1/>&2 before the file, prefix modifiers command/exec --/FOO=bar, group and if headers, leading redirects, discard-then-real-file > /dev/null > real.txt, python write indicators, staged mv/cp, quoted operands with embedded separators, \; and backslash-newline escapes, ec""ho / ec"xy"ho splices) and the allow forms (dup-only redirects, producer-scoped bash x.sh > out && echo done, quoted prose in commit and PR bodies, command -v, comments, blank-line-only and newline-padded commands, glob characters).
• split_lines_to checked directly against while IFS= read -r over printf '%s\n' on: empty, single line, trailing newline, leading newline, runs of blank lines, whitespace-only lines, glob characters, the sentinel byte itself at line starts and ends, IFS characters, trailing backslashes, UTF-8, a 70 KiB line, 3001 lines. Identical arrays in every case; set -f preserved when the caller had it and restored off when it did not.
• New strace-based budget test in block-hook-bypass.test.sh: pins the guard's benign share at exactly 1 creation and 0 execve on two benign payloads (single segment; multi-line, multi-segment), and that echo > file still exits 2 under the tracer. Skips visibly on a host without a working strace. It runs the real dispatched path, not the standalone guard, and subtracts a no-op guard so dispatcher forks do not leak into the pin. Mutation-checked: unmutated control 9/9; each of the four changes reverted alone (eager SUBJECT restored; splitter through mapfile < <(…); strip through $(…); segments through mapfile < <(…)) fails the pin on both payloads.
• The census was re-taken after the renumber commit, since a documentation-only change must not move it, and it does not: whole Bash dispatcher on the benign payload reads 36 creations at origin/main against 30 on this branch, 3/3 identical repeats, execve flat at 3 on both; the blocked-path guard share is 5 creations and 1 execve; the contract suite's exact benign-share pin (1 creation, 0 execve) passes.
• scripts/affected-tests.sh --run: 5 suites selected (block-hook-bypass.test.sh 611/611, flag-commit-pr-skill-bypass.test.sh, require-jq-posture.test.sh, run-guards.test.sh, scripts/check-shell-portability.test.sh), all passed, exit 0; the manifest, CHANGELOG and README map to the recorded no-suite allowlist.
• scripts/check-changelog-parity.sh all four modes the script defines, on the committed tree: --check (pass), --check-order (pass, 91 changelogs read newest-first with no duplicate versions), --check-bump origin/main (pass), --check-preserved origin/main (pass, 155 headings preserved). --check-order is the mode that catches a doubled ## [<version>] after a bad conflict resolution, which is the failure the renumber exists to avoid; an earlier revision of this PR ran only the other three.
• shellcheck -x clean on the guard and the test; shfmt -d no diff; markdownlint-cli2 0 issues; editorconfig-checker clean; check-purged-em-dashes.sh clean; ai-slop detector 0 findings on the README and CHANGELOG.
• Pre-existing and not touched: test_save_point.py::test_new_origin_falls_back_to_directory_name.

Related

• perf(hooks): Windows process-creation tax makes the hook set unusable - 1,778 timeouts / 20.6 hours blocked over 9 days #3508 parent campaign. Its stated cause (per-field jq forks needing a shared helper) does not describe this guard: since perf(hooks): cut leftover Bash parse and dirname tax on the hot path #3788 the dispatcher primes the fields once, and this guard contributed 0 execs before this change.
• perf(context-guard): cut the zone-crossing fire from 8 process creations to 3 #3779 precedent (perf(context-guard): PostToolBatch: context-zone crossing averages 73.0s and timed out 10x - reduce process spawns #3520): an xtrace command-position count read 2 where the kernel made 8; the same instrument gap is why the shim-based budget test could not see any of these seven.
• fix(hook-utils): a hook payload cut short at EOF is a loud allow; a stall stays a block #3740 adjacent: modifies this script's buffer_stdin rc handling. This diff stays off that block. perf(hooks): fuse stdin jq completeness with field extract #3838 is the other fence on lib/hook-utils.sh.
• perf(guardrails): hoist three convention-gate redirects off their substitutions #3849 (issue perf(guardrails): block-convention-violation.sh averages 70.5s (Bash) / 81.8s (PS) and timed out 207x - reduce process spawns #3511) holds guardrails 0.32.11; this PR now takes 0.32.12, so the two no longer collide.
• Credit note for the record: this file's dirname exec died in a5f5e799 (perf(guardrails): drop dirname and sed execs from the always-on path #3781); perf(hooks): cut leftover Bash parse and dirname tax on the hot path #3788 never touched this file.

🤖 Generated with Claude Code

https://claude.ai/code/session_01ViPsHkL3ng9xWt2GjEQJob

---

Generated by Claude Code

[chatgpt-codex-connector]

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-09-06T19:55:56.066012Z	c1bb271	Draft marked ready

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

[claude]

Claude finished @kyle-sexton's task in 5m 10s —— View job

---

Claude finished the code review

• Gather context (PR body, changed files, origin/main diff)
• Invoke /review:code-review and follow its instructions
• Post inline findings
• Post summary review comment

Scope

.github/workflows/claude-security-review.yml exists in this repo, so security findings are out of scope for this lane (per REVIEW.md); none were assessed here. Reviewed the full diff (git diff origin/main...HEAD, 5 files) for correctness and maintainability.

Findings

1 issue posted inline on plugins/guardrails/README.md:382 — the new "Hook budget accounting" row is still headed 0.32.11, but the renumber commit (c1bb271) bumped the manifest and CHANGELOG.md heading to 0.32.12 and left this README heading untouched (confirmed via git show c1bb271 -- plugins/guardrails/README.md, which only edits the "602→611 case" line, not the version label). 0.32.11 is reserved by an unrelated PR (#3849 / issue #3511); scripts/check-changelog-parity.sh never reads README.md, so nothing catches this. See the inline comment for a one-line fix.

What I checked but did not flag

• split_lines_to's sentinel-prefixed IFS=$'\n' splitting trick (avoiding the "newline is IFS whitespace, so runs collapse and edges get stripped" pitfall by prefixing every line with \x1f so no field is ever empty): traced the logic by hand against the described edge cases (embedded sentinel bytes, leading/trailing blank lines, empty input) and it holds up. I did not independently run the differential test the PR describes against while IFS= read -r (no shell-execution tool available in this lane — see below), so treat that verification as author-claimed.
• The four while read/process-substitution loops converted to for seg in "${NORMALIZED_SEGMENTS[@]}" / for line in "${_bbh_lines[@]}": confirmed neither loop shape ran in a subshell before or after, so return and continue 2 (in producer_redirect_bypass) still reach the same scopes.
• Moving SUBJECT derivation into emit_tel: confirmed TOOL_NAME and COMMAND are assigned exactly once each, at file scope, before any call site of emit_tel, so lazy derivation can't observe a different value than the old eager one.
• The nameref out-params (strip_literals_to, split_lines_to): checked for the classic bash nameref self-collision hazard (a local variable name inside the callee matching the caller's target variable name) — no collisions between the two functions' local variable names.
• The new strace-based budget test in block-hook-bypass.test.sh: logic (regex matching clone/fork/execve completions including the <unfinished>/<resumed> split form, subtracting a no-op-guard baseline) reads correctly; not independently executed.
• Version-bump mechanics: scripts/check-changelog-parity.sh doesn't check README, confirmed by reading the script; the CHANGELOG's jump from 0.32.12 straight to 0.32.10 (no local 0.32.11) is expected given perf(guardrails): hoist three convention-gate redirects off their substitutions #3849 owns that number on a parallel branch, not a defect.

Limitations

This lane's tool grant doesn't include general shell execution — I could not run shellcheck, shfmt, the test suite, or the strace-based census myself, so the PR's test/verification claims (611/611 pass, mutation checks, the 244 paired runs, all four check-changelog-parity.sh modes) are author-claimed and unverified by me, not independently confirmed.

[kyle-sexton]

Independent review and the merge-blocker it caught — flipped to ready

Posted by the autonomous worker lane (vm-worker-202609050257). Commit c1bb271a. This flip is the lane's handoff; the merge lane owns it from here. This lane does not merge.

The blocker, found and fixed

This PR originally took guardrails 0.32.11 — already claimed by PR #3849, off the same 0.32.10 base. git merge-tree confirmed the conflict on CHANGELOG.md. Renumbered to 0.32.12; git merge-tree origin/main HEAD now exits 0. #3529 is an issue rather than a PR, and no open PR claims 0.32.12.

The parity mode that was skipped is the one that catches this. The prior pass ran three modes and reported the script defines three. It defines four — --check-order verifies each changelog reads newest-first with no repeated version, which is exactly what a bad resolution of a doubled heading produces. All four now pass, including --check-order across 91 changelogs.

Verified by independent reproduction

Check	Result
SUBJECT deferral	Safe — zero remaining references; emit_tel is the only consumer and its three callers all run after COMMAND/TOOL_NAME are set once and never reassigned. No verdict path reads it
Splitter vs read	17 cases identical — empty, lone/leading/trailing/run-of newlines, whitespace-only, backslashes, glob metacharacters with matching files present, CRLF, sentinel byte at line start and end, tabs, control bytes, UTF-8 — plus 70 KiB, the 65600-byte band, 3000 lines. set -f restored to the caller's prior state either way; IFS restored; clean under set -u
Nameref	Safe — the circular case is passing the function's own local, and no call site does
Deny paths	224 paired runs, 0 differences in exit code or first stderr line; 30 of 56 Bash commands deny on HEAD
Measurements	Reproduced 3/3: benign share 7 → 1 (execve 0 → 0), blocked 10 → 5 (1 → 1), whole lane 36 → 30 (3 → 3)
Subtraction method	Sound — the no-op guard rides the same dispatcher, payload and env, so those forks cancel; before and after use identical subtraction
Mutation pin	Reproduces on all four single reverts
#3740 overlap	Clean — its only hunk here is the buffer_stdin rc block at :91; this diff starts at :150, and :112 is byte-identical to main

Two accuracy fixes

The changelog and README said "the 602-case contract suite"; it is 611 with the new pins. And two em dashes had been carried into an added comment — check-purged-em-dashes.sh is green only because it scans markdown, not shell, so the house-style rule went unenforced there. Both corrected.

One honest note from the re-census: measuring with the guard tree copied outside the repo gives 36 → 30, while the in-repo script path reads 31. That is a location artifact of the measurement, not of the change.

Not closable in-file

The issue's "≤2 spawns" criterion cannot be met while $(hook::buffer_stdin) at :112 stays fenced behind unmerged #3740 and #3838. The body says so plainly.

---

Generated by Claude Code

[claude]

Claude finished @kyle-sexton's task in 3m 9s —— View job

---

Security review complete

Task: Run /review:security-review on PR #3869 (claude/3513-hook-bypass-perf @ c1bb271).

• Gather context (diff, changed files)
• Invoke /review:security-review skill and follow its instructions
• Post findings

---

This PR is a performance refactor of plugins/guardrails/hooks/block-hook-bypass.sh (a Bash-write-bypass security guard), replacing several fork-per-call constructs ($(cmd), < <(printf …) line loops) with fork-free equivalents (nameref-based strip_literals_to, and a new split_lines_to helper backing an array-typed NORMALIZED_SEGMENTS). Since this touches the guard's own detection logic rather than unrelated code, I reviewed it for behavior-preservation bugs that could weaken the deny/bypass detection (a logic regression here is the security-relevant risk class for this file), not just style.

No security issues found. Specifics I checked:

• split_lines_to (the new line-splitter replacing while IFS= read -r line; do … done < <(printf '%s\n' …)): traced the sentinel-prefix (\x1f) construction by hand. Because every original newline is replaced with \n + sentinel, and the whole string is sentinel-prefixed at the start, the transformed text never has adjacent newlines or leading/trailing newlines — so bash's IFS-whitespace collapse/strip behavior on IFS=$'\n' splitting can't merge blank lines or drop leading/trailing empty fields, which is the correctness property strip_literals_to's heredoc/open-quote-span tracking depends on (misaligning physical lines there is exactly the kind of bug that could let a quoted/heredoc span swallow or fail to swallow the wrong tokens and change a verdict).
• Sentinel collision: the new \x1f sentinel is local to split_lines_to and distinct from the pre-existing \x01–\x04 marks used internally by strip_literals_to/normalize_segments; confirmed only one occurrence of \x1f in the file. A command containing a literal \x1f byte is preserved correctly (only the one prepended sentinel is stripped via # shortest-match removal, so a pre-existing sentinel byte in the input survives).
• EXECUTABLE=""; strip_literals_to EXECUTABLE "$COMMAND" (L524) replacing $(strip_literals "$COMMAND"): the old path relied on command-substitution's stripping of all trailing newlines; the new function replicates that explicitly with while [[ "$result" == *$'\n' ]]; do result="${result%$'\n'}"; done (L521) rather than a single % strip — correct, and necessary, since without it a trailing-newline difference could change EXEC_LC matching.
• NORMALIZED_SEGMENTS changed from a newline-joined string to an array (split_lines_to NORMALIZED_SEGMENTS "$normalized", L755); grepped all three consumers (staged_write_move_bypass, cat_redirect_bypass, producer_redirect_bypass) and confirmed every one was updated from while IFS= read -r seg … done < <(printf …) to for seg in "${NORMALIZED_SEGMENTS[@]}", so no stale scalar read of the old string form remains.
• set -f save/restore in split_lines_to: prior noglob state is captured before set -f and restored only if it wasn't already set, and IFS is scoped local to the function — no leakage into caller state that could re-enable globbing (and thus glob-expand a payload) partway through detection.
• Removed globals (SUBJECT, string-form NORMALIZED_SEGMENTS) have no leftover references anywhere in the guard or its test file.
• CHANGELOG/README/manifest version-bump changes are documentation/metadata only.

This matches the PR's own extensive verification narrative (244 paired deny/allow runs vs. origin/main, direct fuzzing of split_lines_to against read, and mutation testing on the 611-case suite) — the described edge-case coverage (sentinel byte itself, blank line runs, leading/trailing newlines, 70 KiB lines) lines up with the actual failure modes this kind of splitter refactor risks. Per the security-review skill's scope, this is a logic/trust-boundary review; no Actions-workflow files are touched, so zizmor's lane doesn't apply here. I was not able to execute the shell test suite directly in this environment (tool restrictions), so this is a static/code-reading verification, not a fresh test run.

[github-actions]

Last security-reviewed head: c1bb271ab9512fa558dafacda168215febee35a4. On the next push, the relevance gate compares only the commits since this SHA; delete this comment to force a full re-review.

[github-actions]

Claude has reviewed this PR 1 time. The lane skips further automatic reviews after 5; deleting this comment resets the count.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c1bb271ab9

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[kyle-sexton]

Lane babysit-loop, instance ccr-session-babysit-loop-20260906, claimed at head c1bb271a (mergeable_state=dirty, ~70 min since last activity). Conflicts resolved and a stale perf pin corrected. Head moves c1bb271a -> 2f014eaa.

Conflicts (3, all one shape)

block-hook-bypass.sh — the actual subject of this PR — merged cleanly. All three conflicts were the guardrails version stack: main published 0.32.11 while this branch had already claimed 0.32.12.

file	resolution
.claude-plugin/plugin.json	keep 0.32.12, strictly above main's 0.32.11
CHANGELOG.md	main's 0.32.11 entry verbatim; this branch's 0.32.12 stacked above it
README.md	both sides titled a perf row 0.32.11; main's stays verbatim, this branch's renumbered to 0.32.12 to match its manifest

All four changelog-parity modes pass, as do plugin-options-docs, purged-em-dashes and shellcheck -x.

The merge made this PR's own claim out of date

Merging main brought in #3838, which landed the fork-free hook::buffer_stdin_to in lib/hook-utils.sh. This guard now calls it. That matters because this PR documented seven forks removed except the seventh, $(hook::buffer_stdin), which it explicitly left to lib/hook-utils.sh.

The contract suite caught it, exactly as designed. Its own comment said:

The one creation left is the guard's own $(hook::buffer_stdin) … when that lands this figure drops to 0 and the pin moves with it.

It has landed, so two strace cases failed with expected '1', got '0' — the guard is now cheaper than its own test allowed. Corrected in 2f014eaa:

• the pin is 0, with the comment recording why;
• the CHANGELOG and README rows that named $(hook::buffer_stdin) as the remaining creation now say all seven are gone;
• figures re-measured on the merged tree with the same instrument the README documents (strace -f -e trace=clone,clone3,fork,vfork,execve, guard share = count minus a no-op guard, HOOK_TELEMETRY_SINK unset), stable across three repeats: benign guard share 7 -> 0 creations, blocked 10 -> 4.

The whole-dispatcher and wall-clock rows were measured on the CI host against the pre-#3838 base. I did not restate them from this container; the README now says so, and points at the 0.32.11 row for the dispatcher's own reduction.

Test status

scripts/affected-tests.sh --run is clean except one case, which is not branch-owned:

FAIL: symlink: a genuine temp write in the same root stays allowed: expected exit 0, got 2

It reproduces identically on unmodified origin/main in this container (PASS=601 FAIL=1), so it is a sandbox symlink-resolution artifact, not a regression. block-noncanonical-commit.test.sh is 216/216.

(One measurement note for anyone reproducing this locally: run the suites after committing the merge. With MERGE_HEAD still set, block-noncanonical-commit correctly exempts commits during an in-progress merge and 36 of its cases report expected exit 2, got 0. That is the guard working, not a break.)

Lane action: advanced. Now awaiting CI on 2f014eaa. Merge not attempted — the gh merge gate cannot read thread-resolution state under this session's pinned-GraphQL restriction, so readiness stays unproven from here. Claim released.

---

Generated by Claude Code