perf(guardrails): drop the forks that never exec in block-noncanonical-commit (#3514) by kyle-sexton · Pull Request #3886 · melodic-software/claude-code-plugins

kyle-sexton · 2026-09-06T22:04:41Z

Summary

block-noncanonical-commit.sh created processes that never exec'd, so every exec-counting census (PATH shim, run-guards.test.sh, an xtrace count) read it as free while a Windows host paid a full process creation for each. This PR removes the forks that belong to this file: the eager telemetry subject at file scope on every Bash and PowerShell call, the two command substitutions around builtins-only functions on the blocked multi-line commit path, one $(printf '%q') per trailing argument on a ! alias reparse, and the $(cd … && pwd) plugin-root probe on the PowerShell lane. Verdicts are unchanged.

The branch merges main at 1b681862 (#3878), which replaced the shared parser's < <(printf …) fork in lib/hook-utils.sh with a ${cmd:i:1} walk. With that gone, this guard's own share on a benign Bash call is now 0 creations / 0 execve, and every figure below is measured against that main. Guardrails is bumped to 0.32.15: main carries 0.32.14 (#3878), and the open PRs #3849, #3869 and #3872 hold 0.32.12 and 0.32.13.

Scope is the one hook this issue names. lib/hook-utils.sh and its synced copies are untouched by this PR (the guardrails copy is byte-identical to main's), as are the sibling guards (#3517, #3518, #3519, #3521).

Fix

In plugins/guardrails/hooks/block-noncanonical-commit.sh:

SUBJECT=$(hook::extract_bash_subject …) moves from file scope into emit_tel, behind the start-stamp and hook::telemetry_enabled gates. Only the envelope reads it, and the envelope is off by default. Same shape as block-windows-drive-tmp.sh and the sibling PRs perf(guardrails): block-hook-bypass drops six forks that never exec (#3513) #3869 and perf(guardrails): drop the forks that never exec in block-dangerous-git (#3529) #3872.
effective_dir becomes effective_dir_to <var> … (nameref assignment) at its three call sites; explicit_global / explicit_git_dir become explicit_global_to / explicit_git_dir_to. Both were $(…) around builtins-only bodies on the blocked path.
The two ! alias reparse loops quote trailing arguments with printf -v instead of $(printf '%q' …).
The PowerShell lane sets PLUGIN_ROOT="${CLAUDE_PLUGIN_ROOT:-$_HOOK_SELF/..}"; source resolves that path to the same file the canonicalised spelling named, and nothing else reads PLUGIN_ROOT. One tradeoff, recorded in a comment: the kernel resolves .. physically where cd resolved it logically, so a hooks/ directory that is itself a symlink out of the plugin root needs CLAUDE_PLUGIN_ROOT set. Spaces and relative invocation are unaffected.

Not changed, deliberately: repo_git_probe's $(git … 2>/dev/null; printf 'x') still costs two creations for one exec. The compound body exists to keep a trailing newline in a repository path byte-exact (documented in the function), and neither hoisting the redirect nor exec git preserves that, so it stays.

No matcher predicate was added ahead of the shared parser. A safe text pre-filter is constructible: the parser only removes or decodes characters, and decoding requires $', so a predicate that admits any command containing $' or a g…i…t run with only quote, backslash and newline characters between the letters must hold for every word the parser can turn into git (the review verified zero fail-open across 32 DENY payloads, including g''it, $'\x67it', "git", \git, g"i"t, g<LF>it, bash -c / -lc / sh -c wrappers and an inline ! alias). What it would buy is small: it skips only commands with no git-shaped substring at all, and the single fork it would have saved on those was the library parser's, which main has now removed. So criterion 3 of #3514 is moot rather than impossible, and this PR adds no predicate.

Verification

Everything in this section except the wall-clock paragraph was re-run after the merge of main at 1b681862, on the merged tree at 5d3102b1. The two arms are git archive exports: pristine = origin/main at 1b681862, modified = 5d3102b1. A content-hash comparison of the two exports (3767 files each) shows exactly five files differing, all under plugins/guardrails/: .claude-plugin/plugin.json, CHANGELOG.md, README.md, hooks/block-noncanonical-commit.sh and hooks/block-noncanonical-commit.test.sh. lib/hook-utils.sh, the guardrails hooks/hook-utils.sh, hooks/run-guards.sh and lib/powershell/ps-command.sh are byte-identical across the arms, and the pristine guard is byte-identical to the 1b681862 blob.

Kernel census (strace -f -e trace=clone,clone3,fork,vfork,execve, dispatched through run-guards.sh, guard share = count minus a no-op guard dispatched the same way, this repository as cwd, HOOK_TELEMETRY_SINK unset, CLAUDE_PROJECT_DIR empty, three repeats each, identical every time; re-run post-merge on the two exports above, every figure reproduced). Against the pre-#3878 main (c0fba152) every creation count read one higher on both sides, the parser's.

Payload	creations before → after	execve before → after
Bash `git status --short` (benign)	1 → 0	0 → 0
Bash `git commit -m 'feat: x'` (single-line, allowed)	1 → 0	0 → 0
Bash multi-line `-m` (blocked, rc 2)	5 → 2	1 → 1
Bash `git wibble` (persisted-alias probe)	6 → 4	2 → 2
Bash inline `!` alias to multi-line `-m` (blocked)	10 → 6	3 → 3
PowerShell `git status`	14 → 12	3 → 3
Whole eight-guard Bash matcher, `git status --short` (absolute)	22 → 21	2 → 2

The execve column is unchanged everywhere, which is what makes this latency rather than removed work. A benign call now creates nothing in this guard; the remaining PowerShell creations are in lib/powershell/ps-command.sh.

Wall clock (measured before the merge, against c0fba152, not re-run), Linux, bash -c : floor p50 3.0 ms / p95 7.2 ms, n=20 after 2 warmup, sides interleaved, guard alone under the dispatcher: benign p50 23.8 → 21.2 ms (p95 25.9 → 30.9 ms, noise at this scale); blocked p50 32.2 → 29.9 ms (p95 75.8 → 44.7 ms). The milliseconds are context; the process counts are the host-independent figure.

A/B differential (re-run post-merge on the two exports above, in a fresh harness): 96 payloads (85 Bash, 9 PowerShell, 2 Write-tool) × 2 modes (standalone bash <guard> and dispatched run-guards.sh --lib lib/powershell/ps-command.sh <guard>) = 192 paired runs, plus 13 payloads × 2 modes = 26 paired runs under a PATH that carries no git at all (command -v git confirmed empty under that PATH; the blocked payloads in those runs still produce the full 452-byte stderr message, 429 bytes on the PowerShell lane, on both arms, so stderr is genuinely compared there rather than trivially empty) = 218 paired runs, 218 identical on exit code, stdout and stderr (byte compare with cmp). Verdicts: 50 deny / 46 allow per arm, identical on both arms in both modes; no payload the pristine arm denied is allowed by the modified arm. The corpus covers every -m spelling the guard matches (-m, -am, -m<attached>, -qm, --message=, --message, --mess=, --m, $'…'), the exempt forms (--amend, --fixup, -F <path>, -F - heredoc, repeated single-line -m, bare git commit, an in-progress merge with MERGE_HEAD via cwd and via --git-dir=), case variants (GIT, Git), /usr/bin/git, git.exe, g''it, $'\x67it', "git", \git, g"i"t, git split across a backslash-newline, bash -c / bash -lc / sh -c wrappers, env, NAME=value, sudo and sudo FOO=1 prefixes, cd sub && …, ;, |, &&, ||, bare newline, CRLF, U+2028 in the message, a leading BOM, zero-width space inside git and inside the message, near-miss spellings that must not be caught (gitt, mygit, git-commit, a quoted string mention, a comment), eval, variable indirection, backtick, inline -c alias.* (git alias, ! shell alias, ! alias with trailing arguments), persisted aliases (multi-line commit, single-line, ! shell, two-hop chain to commit -F -, undefined git wibble), -C ., -C sub, --git-dir=, repeated --git-dir, --work-tree=, --trailer, --no-verify, literal \n, $(printf …) message, nohup … &, tab separators, a grep -m false friend, a 70 KiB benign command, a 20 KiB multi-line message, an empty command, a control character in the message, an empty cwd, nine PowerShell forms (status, backtick-n, real newline, here-string -m, here-string piped to -F -, & call operator, Invoke-Expression, single-line, near-miss gitt), and two Write-tool payloads. Additionally 11 telemetry envelopes captured through a stub sink (HOOK_TELEMETRY_SINK set; the sink is fire-and-forget, so the capture waits for its write) agree on status, tool, subject and form between the two arms, across benign, blocked, inline ! alias, persisted-alias probe, empty-cwd and PowerShell benign and blocked payloads; subject is the field the lazy SUBJECT change touches. No payload is skipped by any new predicate, because none was added.

Contract suite on the merged tree: block-noncanonical-commit.test.sh PASS=227 FAIL=0 (216 on pristine main, plus 11 new assertions). The new section pins, by the same strace instrument: benign share exactly 0 creations / 0 execve; single-line -m equal to benign; blocked multi-line -m exactly +2 creations / 1 execve over benign; trailing ! alias arguments add zero creations. It skips visibly where strace is absent.

Mutation checks (re-run post-merge on 5d3102b1; apply, run the suite, revert, worktree verified clean after each): restoring the eager file-scope SUBJECT fails the benign pin (expected 0, got 1); putting the explicit --git-dir back through a $(…) on the block path fails the blocked delta (expected 2, got 3); restoring a $(printf '%q') per trailing alias argument fails the trailing-args pin (expected 4, got 7, three trailing arguments). Each mutant reads PASS=226 FAIL=1 with only its named pin failing. The deltas do not depend on the library parser. All three revert clean.

Gates on the merged tree (5d3102b1, re-run post-merge): bash scripts/affected-tests.sh --run selected 7 shell suites (resolve-convention-pattern, block-convention-violation, block-noncanonical-commit, flag-commit-pr-skill-bypass, require-jq-posture, run-guards, commit-msg-convention), "All 7 selected suites passed or were skipped", exit 0. plugins/claude-ops/skills/plugins/scripts/cache-content-check.test.sh is not in that selection and fails two process-budget assertions on the clean 1b681862 export as well, so it is pre-existing and unrelated. check-changelog-parity.sh --check, --check-order, --check-bump origin/main, --check-preserved origin/main all exit 0 against main at 1b681862. sync-plugin-options-docs.py --check up to date. ShellCheck clean on the guard and its test. No em dash in any added line.

Unmet, stated plainly: acceptance criterion 5, "measured on a Windows host, under 2 s", is not met by this PR. Everything above was measured on Linux. The substitute evidence is the host-independent process-creation and execve census (the Windows cost is a per-spawn multiplier on those counts) and Linux wall clock against a bash -c : floor. Criterion 1's "at most 2 spawns" is met on the common path on both counters (0 creations, 0 execve) once #3878 is in the base.

Refs perf(hooks): Windows process-creation tax makes the hook set unusable - 1,778 timeouts / 20.6 hours blocked over 9 days #3508 (parent: Windows process-creation tax)
Refs perf: drop leftover forks in hook-utils, always-on hooks, and CI scanners #3878 (merged into this branch at 1b681862; removed the shared parser fork that held the benign figure at 1)
Refs fix(hook-utils): a hook payload cut short at EOF is a loud allow; a stall stays a block #3740, perf(hooks): fuse stdin jq completeness with field extract #3838 (lib/hook-utils.sh fork-free forms)
Refs perf(guardrails): block-hook-bypass drops six forks that never exec (#3513) #3869, perf(guardrails): drop the forks that never exec in block-dangerous-git (#3529) #3872, perf(guardrails): hoist three convention-gate redirects off their substitutions #3849 (sibling guardrails spawn PRs; same emit_tel shape, and the version numbers they hold)
Refs guardrails: recover the PreToolUse spawn-reduction work from the closed #1385 #1403 / fix(guardrails): restore PreToolUse enforcement — guards were killed at their timeout #1385 (prior spawn-reduction art)
Refs docs/conventions/hook-budget/README.md (budget accounting entry added to the guardrails README)

🤖 Generated with Claude Code

https://claude.ai/code/session_01ViPsHkL3ng9xWt2GjEQJob

…ners (#3878)  No linked issue ## Summary Drop leftover process creations on the hottest Bash paths in this marketplace: the shared hook library every always-on hook sources, the always-on formatter Write/Edit paths (typos, ruff, biome, bash, powershell, go, actionlint, eol, markdown), the always-on desktop-notification Notification path, always-on guardrails verifiers, and CI scanners that used to spawn once per file, per plugin, or per allowlist entry. ## Fix GNU Bash runs command substitution in a subshell even for builtins (Command Substitution, [Bash Reference Manual](https://www.gnu.org/software/bash/manual/html_node/Command-Execution-Environment.html); [Greg's Wiki](https://mywiki.wooledge.org/CommandSubstitution)). Cygwin's `fork` is a non-copy-on-write Win32 `CreateProcess` ([Cygwin User's Guide, Process Creation](https://ftp.cygwin.com/cygwin-ug-net/highlights.html)): "fork will almost certainly always be inefficient under Win32." ### Shared hook library (`lib/hook-utils.sh`, synced to 17 carriers, patch bump) Same `_to` / in-process pattern as #3838, #3732, and #3678: - `hook::json_escape_to` deletes residual C0 bytes with parameter expansion instead of `printf | tr -d` - `hook::emit_channels` writes through `_to` instead of `$(hook::json_escape …)` - Fractional `read -t` landed in bash-4.0-alpha (CHANGES). `hook::read_supports_fractional_timeout` is `BASH_VERSINFO`; no TMPDIR probe file - `hook::notice_once` reads the marker with `read`, creates the directory only when missing, and prunes stale markers once per process - `hook::bash_parse_segments` walks `${cmd:i:1}` instead of `read -N1` from a process substitution, and `$'…'` bodies decode through `ansi_c_decode_to` (`printf -v`) - `hook::repo_root_to` / `hook::repo_relative_path_to` write in this shell so callers skip a leftover capture around git or builtins-only work Isolation `$(source …)` forks are unchanged (#3685). ### typos-format (always-on Write|Edit|NotebookEdit) - Basename via `${FILE##*/}` (plus a backslash trim), not `basename(1)` - `repo_root_to` / `repo_relative_path_to` instead of capture subshells - Directory existence check instead of `$(cd && pwd)` - `command -v typos` is no longer captured; the later exec looks the name up on PATH ### Remaining always-on formatters (ruff, biome, bash, powershell, go, actionlint, eol, markdown) Same leftover class as typos-format, now applied to every always-on formatter that still captured `_to` helpers or spawned `basename` / leftover `cd && pwd`: - `FILE_BASE` is `${FILE##*/}` (and a backslash trim) - `repo_root_to` / `repo_relative_path_to` write in-process - `$(cd && pwd)` canonicalize is an existence check on the path git already answered (ruff, biome, bash-format EditorConfig walk) - Nested `$(normalize_path "$(physical_path …)")` in powershell-format uses the `_to` forms - `command -v ruff|biome|goimports` is no longer captured - markdown-format keeps physical `pwd -P` containment and config discovery; leftover helper-capture and membership dirname on the root-resolution path are gone ### desktop-notification (always-on Notification) - Field extract fuses into `hook::buffer_stdin_to` so completeness and `.notification_type` / `.message` share one jq process - C0 stripping is parameter expansion, not `printf | tr` - `repo_root_to` writes in-process; OSC 9 / BEL use `printf -v`; `terminalSequence` uses `json_escape_jq_to` - `uname` stays so tests can PATH-stub Darwin; git for `repo_root` stays ### guardrails verifiers (always-on PostToolUse / PreToolUse) - `skill-reference-verify`, `stale-path-verify`, and `cli-flag-verify` call `repo_root_to` / `repo_relative_path_to` in-process - `hardcoded-path-check` and `secret-pattern-detection` use `normalize_path_to` instead of leftover `$(hook::normalize_path)` captures - Isolation `$(source …)` forks are unchanged (#3685) ### CI scanners - Orphaned-fixture scan: one `*.test.*` index, cached `evals.json` `files[]`, in-shell ERE escape. Unquoted `\\` matches one backslash (a quoted `'\\'` arm is two chars and leaves `\b` as a word boundary) - Purged-em-dash scan: one `git ls-files -z` with every `:(glob)` pathspec; in-process component-wise attribution so `*` cannot cross `/`. `--list` stdout is byte-identical to origin/main - Cross-plugin source drift: one `find plugins` plus one `sha256sum` of 2+ cluster paths. Discover stdout is byte-identical to origin/main - Discriminating-test-skips / silent-skips: one awk per corpus (`FNR` + `FILENAME`; mawk has no `ENDFILE`) - Hook-exec-form: one jq over every `hooks.json` and one over every `plugin.json` (`input_filename` attributes rows). Unreadable `hooks.json` still fails closed via per-file fallback; unreadable manifests are still skipped Hook-specific leftover-fork work already in flight (#3873, #3872, #3871, #3870, #3869, #3851, #3849, #3779, #3880, #3886) is out of scope here. ## Verification Independent census re-derived spawn counts from `84adf87b` vs `cdb93f61` without inheriting implementer figures. Kernel census `strace -f -e trace=clone,clone3,fork,vfork,execve`; counter over duration; 3 identical trials. **always-on formatters** (this revision vs `84adf87b`): | Hook | clones before | clones after | execve before | execve after | |---|---|---|---|---| | ruff-format no-config skip | 14 | 10 | 4 | 4 | | powershell-format no-settings skip | 23 | 16 | 7 | 7 | | bash-format no-EditorConfig (ShellCheck finding) | 17 | 13 | 6 (1 `basename`) | 5 (0 `basename`) | **guardrails** (this revision): | Hook | clones before | clones after | execve | |---|---|---|---| | skill-reference-verify Write, no skill refs | 18 | 17 | 8 unchanged | | secret-pattern-detection clean Write | 10 | 8 | 4 unchanged | Secret-pattern absolute counts are with `CLAUDE_PLUGIN_ROOT` set (Claude Code always sets it). Without that env the leftover `PLUGIN_ROOT=$(cd … && pwd)` fallback adds one clone on both sides (11→9); the drop of 2 is the same. **CI scanners** (successful execve, exclude ENOENT; earlier commits on this PR): | Gate | origin/main or prior HEAD | HEAD | |---|---|---| | purged-em-dashes `--list` | 478 | 9 | | cross-plugin-source-drift `--check` | 181 | 4 | | discriminating-test-skips | 316 (awk 312) | 5 (awk 1) | | silent-skips | 120 (awk 118) | 4 (awk 2) | | hook-exec-form `--check` | 196 execve, jq 96, tr 96, clones 292 | 7 execve, jq 2, tr 0, clones 9 | `--list` / discover stdout for the two listing gates is byte-identical to origin/main. **Local `scripts/affected-tests.sh --run`:** 153 shell suites passed or were skipped; 14 NOT RUN python/mjs ecosystems (exit 3, expected on this runner). No `FAIL`. Including: `lib/hook-utils.test.sh` PASS=323; bash-format PASS=54; eol-normalizer PASS=54; markdown-format PASS=174; powershell-format PASS=17; cli-flag-verify PASS=92; hardcoded-path-check PASS=118; secret-pattern-detection PASS=86; skill-reference-verify PASS=140; stale-path-verify PASS=108. ruff/biome/go/actionlint behavioral cases skipped here (binaries absent); skip-path and source pins still ran. `session-event-log.test.sh` PASS=53 isolated under the fan-out. **CI on `cdb93f61`:** lint, hook-utils, test-linux (0–3), test-windows, changes, ci-status, and managed-files-guard all succeeded. https://github.com/melodic-software/claude-code-plugins/actions/runs/34066676378 https://github.com/melodic-software/claude-code-plugins/actions/runs/34066676488 ## Related Refs #3838, #3732, #3678, #1979, #3488, #2891. Same leftover-fork class as open PRs #3849 / #3851 / #3869 / #3873 / #3872 / #3871 / #3870 / #3779 / #3880 / #3886 (those stay hook-specific). N/A for a dedicated issue.  <div><a href="https://cursor.com/agents/bc-fdfdc962-be1b-4c9c-9833-3aec57852330?cursor_ref=pr_footer&cursor_cta=open_in_web"><picture><source media="(prefers-color-scheme: dark)" srcset="https://cursor.com/assets/images/open-in-web-dark.png"><source media="(prefers-color-scheme: light)" srcset="https://cursor.com/assets/images/open-in-web-light.png"><img alt="Open in Web" width="114" height="28" src="https://cursor.com/assets/images/open-in-web-dark.png"></picture></a> <a href="https://cursor.com/background-agent?bcId=bc-fdfdc962-be1b-4c9c-9833-3aec57852330&cursor_ref=pr_footer&cursor_cta=open_in_cursor"><picture><source media="(prefers-color-scheme: dark)" srcset="https://cursor.com/assets/images/open-in-cursor-dark.png"><source media="(prefers-color-scheme: light)" srcset="https://cursor.com/assets/images/open-in-cursor-light.png"><img alt="Open in Cursor" width="131" height="28" src="https://cursor.com/assets/images/open-in-cursor-dark.png"></picture></a> </div> --------- Co-authored-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: ksextonmelodic <ksextonmelodic@gmail.com>

chatgpt-codex-connector · 2026-09-07T00:54:45Z

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-07T00:58:03.171586Z	`5d3102b`	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 · 2026-09-07T00:54:55Z

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

Code review complete

Load /review:code-review skill instructions
Check skip gate (draft/closed/trivial/already-reviewed) — none apply; PR is open, ready for review, and non-trivial
Gather PR diff (origin/main...HEAD)
Review plugins/guardrails/hooks/block-noncanonical-commit.sh
Review plugins/guardrails/hooks/block-noncanonical-commit.test.sh
Review docs/version bump files (plugin.json, CHANGELOG.md, README.md)
Post inline findings and summary

Findings

One inline finding, posted on both docs it applies to:

plugins/guardrails/CHANGELOG.md#L24 / plugins/guardrails/README.md#L396 — Both persisted docs state "136 paired runs (65 payloads ..., plus six on a PATH with no git)" and "seven telemetry envelopes." The PR body's own Verification section describes a later re-run, done after the main merge, at 218 paired runs (96 + 13 payloads, 13 in the no-git-PATH slice) and 11 telemetry envelopes. Since the CHANGELOG/README are what remains in the repo after the PR closes, worth reconciling these figures with the final verification numbers before merge.

Code walkthrough (no other issues found)

Traced the four fork removals in block-noncanonical-commit.sh against the shared-nameref pattern used throughout the codebase's _to helpers:

explicit_global_to / explicit_git_dir_to chain namerefs correctly — explicit_git_dir_to passes the name (not a nameref) through to explicit_global_to, which is the standard way to avoid a nameref-to-nameref indirection bug in Bash. Verified the intermediate frame declares no local shadowing the target name, so dynamic-scope resolution lands on the caller's variable at every recursion depth of check_segment.
effective_dir_to correctly reads HOOK_EFFECTIVE_BASE into base before writing the nameref target, so a caller naming that global as the destination wouldn't self-clobber (not currently exercised, but the invariant holds).
emit_tel's lazily-computed subject is safe for the PowerShell lane despite COMMAND being rewritten (PS_SAFE_COMMAND) before emit_tel runs: hook::extract_bash_subject (lib/hook-utils.sh:1896) returns the bare tool name for any non-Bash tool, ignoring cmd entirely, so the rewrite is moot for the subject value.
The printf -v quoted_arg / printf -v alias-reparse changes and the PowerShell PLUGIN_ROOT change preserve existing control flow; the symlink-hooks/-directory tradeoff on the latter is disclosed in the added comment.
run-guards.sh already supports an absolute guard path via its */* branch, so the new test helper's guard_share dispatching $TEST_TMPDIR/noop-guard.sh through it is valid.

No correctness bugs found in the fork-removal refactor itself. Security review is out of scope for this lane per plugins/review/skills/code-review/SKILL.md.

claude · 2026-09-07T00:55:21Z

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

Security review complete

Gather context (PR diff, changed files)
Run skip-gate check
Review block-noncanonical-commit.sh diff for security-relevant changes
Review block-noncanonical-commit.test.sh diff
Adversarially validate any candidate findings
Post final results

No security issues found.

This PR is a mechanical performance refactor of plugins/guardrails/hooks/block-noncanonical-commit.sh that removes forks without changing the guard's verdict logic. I read the full diff (gh pr diff) and traced each change for injection or fail-open risk, since this file is itself a security guard:

explicit_global() / explicit_git_dir() / effective_dir() → *_to nameref variants (block-noncanonical-commit.sh#L227-L258, #L541-L557): replaces $(...) command substitution with local -n nameref out-parameters. Checked every call site (seg_dir, explicit_gd) against each function's own locals (opt, i, n, arg, found, a, base) for the classic nameref self-collision gotcha — no collisions, and the destination names are hardcoded identifiers, never derived from parsed command text, so there's no path for attacker-controlled input to influence which variable a nameref binds to. The functions have no global side effects, so running them in-process instead of a subshell doesn't leak new state.
$(printf '%q' "$a") → printf -v quoted_arg '%q' "$a" on the ! alias reparse paths (#L826-L831, #L900-L906): the quoted value is only ever appended into reparse/preparse via += (an assignment context, not a command word), so neither the old nor the new form is subject to word-splitting or globbing — behavior is identical. Confirmed reparse/preparse only ever feed back into hook::bash_parse_segments and alias_reexpand_admit (the guard's own tokenizer), never eval/bash -c, so quoting fidelity here doesn't open an execution path either way.
SUBJECT moved from file scope into emit_tel() (#L177-L193): worth double-checking since the PowerShell lane rewrites COMMAND later in the script, so a lazily-computed subject could in principle capture the rewritten value. Verified against hook::extract_bash_subject (lib/hook-utils.sh:1896): for any tool != "Bash" it returns the bare tool name and never inspects cmd, so the PowerShell case is unaffected by the rewrite either way. No behavior or privacy change to the telemetry subject.
PowerShell PLUGIN_ROOT from $(cd … && pwd) to $_HOOK_SELF/.. (#L1060): _HOOK_SELF derives from BASH_SOURCE[0], not attacker-controlled input, and the only consumer is source. The symlink-resolution tradeoff is already called out in the added comment. No new trust boundary crossed.

Verdict logic (git-subcommand matching, alias reparsing, sequencer detection) is untouched; only how intermediate values move between functions changed. The extensive A/B differential in the PR description (218 paired runs, identical exit code/stdout/stderr) and the contract suite (227/227) are consistent with what the diff shows.

Branch: claude/3514-noncanonical-commit-spawns

github-actions · 2026-09-07T00:58:53Z

Last security-reviewed head: 5d3102b199a77ec2935491393fb153087ecd8e6e. 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 · 2026-09-07T01:00:46Z

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

…s, take 0.32.16 Review of #3895 (#3528) returned one MEDIUM and three NITs; this is that fix pass, nothing more. - Version 0.32.16, not 0.32.15. Main is at 0.32.14 and #3886 was renumbered to 0.32.15 after this branch opened; no other open PR head carries anything above main. plugin.json, the CHANGELOG heading, and the README budget note move together; all four parity modes are green against origin/main. - run-guards.sh resolved `.hook_event_name` as RUN_GUARDS_VALUES[9], the field's position in PRIME_FILTERS. A filter added ahead of it would have put the neighbouring value into every dispatcher abort notice. The event is now looked up by name in the primed filters. The suite gains an rg-shift case (a filter inserted ahead, the event still named) that the positional code fails (expected PreToolUse, got ''), and a static check that PRIME_FILTERS still carries the field. - Bash holds one EXIT trap per shell, so a guard that later ran its own `trap ... EXIT` replaced the boundary and was back to the bare rc 1 this PR removes. The suite now fails on any `trap` naming EXIT in a registered hook or a library it sources (hook-utils.sh, the --lib paths); a copy of block-no-verify given one fails that assertion by name. The library header states the supported way to chain exit-time work (a slot in this library, never a second trap); no hook needs one today. - abort-boundary.test.sh is 100755 like its seventeen siblings. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ViPsHkL3ng9xWt2GjEQJob

) Closes #3528 ## Summary A guardrails hook that died between its first line and its own final `exit` (an unbound variable under `set -u`, a helper that no longer existed, a `source hook-utils.sh` that failed) ended with whatever status the last command had, usually 1, and wrote nothing of its own. Claude Code treats any status other than 0 and 2 as a non-blocking error: the tool call proceeds and the transcript records "exit 1, stderr: (none)". For a PreToolUse blocking guard that is enforcement silently skipped, which is what the issue recorded for `block-windows-drive-tmp` and `cli-flag-verify`. Every registered guardrails hook and the dispatcher now install a shared abort boundary that turns that outcome into a deliberate, visible one. Enforcement is unchanged: every hook declares fail-open, which is the status quo on abort; the notice is the only delta. ## Fix - New `plugins/guardrails/hooks/abort-boundary.sh` (a sibling library in `hooks/`, not `lib/hook-utils.sh`, which is contended by #3740 and #3838 and syncs to 17 copies). `guard::abort_boundary <name> <event> <open|closed> <chosen-status>...` installs an EXIT trap that passes the chosen statuses through untouched and turns any other into one stderr line naming the hook and the status plus a `systemMessage` / `additionalContext` document, then exits with the declared posture (`open`: exit 0, document on stdout; `closed`: exit 2, notice on stderr). The handler clears its own trap first, is builtins-only, and calls nothing from `hook-utils.sh`, so it still reports the case where that library failed to load. Stream choice was checked against the installed CLI (2.1.258: "Failed with non-blocking status code" on non-0/2, `hook_non_blocking_error`) and the repo's hook-observability doctrine: on exit 0 stderr reaches only the debug log and the stdout document is what the operator and agent see, so the notice goes on both. - All 14 registered hooks: `source abort-boundary.sh`, a per-hook posture comment beside the install line, and `source hook-utils.sh || exit 70` (a non-chosen status, so a failed library load is reported instead of limping through undefined `hook::` calls). Blocking guards choose `0 2`; advisory hooks choose `0`. `block-hook-bypass`'s bespoke handler (#3130 F5) is replaced by the shared one. - `run-guards.sh`: installs the same boundary around its own prologue and merge (an abort there skipped every guard of the event), primes `.hook_event_name` in its existing jq call so its notice names the event, and releases the trap before its deliberate aggregated `exit "$RC"` so a non-block status a guard returns still surfaces exactly as before. The event is looked up by NAME among the primed filters (review fix pass): the first cut read `RUN_GUARDS_VALUES[9]`, the field's position in `PRIME_FILTERS`, so a filter added ahead of it would have put the neighbouring value into every dispatcher notice. - One EXIT trap per shell (review fix pass): bash holds a single EXIT trap, so a guard that later runs its own `trap ... EXIT` replaces the boundary and is back to the bare rc 1 this PR removes (verified: with `trap ':' EXIT` after the install line a forced abort is rc 1, empty stdout, bash's own line only; without it, exit 0 plus the document). The suite now fails on any `trap` naming EXIT in a registered hook or in a library it sources (`hook-utils.sh`, the `--lib` paths). The library header states the supported way to chain exit-time work: a slot in the library that the handler calls before it decides, added with a suite case in the same change, never a second trap. No hook needs one today. - Version 0.32.16. Main is at 0.32.14; #3886 was renumbered to 0.32.15 after this branch opened (this body previously said it held 0.32.14, which was stale). Determined by reading `plugins/guardrails/.claude-plugin/plugin.json` at current `origin/main` and at the head of all 31 open PRs: #3886 is the only head above main, so 0.32.16 is the first free number. #3872 (0.32.13), #3849 and #3869 (0.32.12) and the rest sit at or below main and will need re-bumps of their own; whichever guardrails PR lands after this one takes 0.32.17. A CHANGELOG conflict at merge time is still expected. Exit-status contract mapped before the change (unchanged by it): guards exit 0 allow (including `hook::check_enabled` and `hook::require_jq`) and 2 deny (including `hook::require_jq_blocking`), nothing else on purpose (`cli-flag-verify`'s `exit 1`/`exit 2` strings are comment prose); `hook::buffer_stdin_to` returns 0 payload, 1 empty, 2 stalled/malformed, and the dispatcher exits 0 on rc 1 before any guard is sourced; the dispatcher exits 2 if any guard exited 2, else the highest non-zero guard status, else 0; guards run in `$( )` subshells, which do not inherit the parent's EXIT trap (verified empirically on bash 5.2, so the two boundaries never fire for one exit). ## Verification - `hooks/abort-boundary.test.sh`, 232 assertions, green (mode 100755, like its 17 siblings). It reads the registered set from `hooks.json` (15 scripts, never enumerated), asserts each sources the library and installs under its own name with a posture literal, asserts no registered hook and no sourced library (`hooks/hook-utils.sh`, `lib/powershell/ps-command.sh`) installs an EXIT trap of its own, asserts `PRIME_FILTERS` still carries `.hook_event_name`, forces a `set -u` abort in every registered guard on a plugin copy (exit 0, one stderr line naming the guard and `rc=1`, a JSON document whose `hookEventName` is an event the guard is registered for), forces the abort mid-hook through a failing shared helper on `block-windows-drive-tmp` (on a `D:/tmp` write the shipped guard still denies) and `cli-flag-verify`, checks the dispatched path keeps a sibling's deny (exit 2) beside an aborting guard and merges two notices into one document, checks the dispatcher's own boundary before and after priming plus its release, checks a prime filter inserted ahead of `.hook_event_name` on a copy still yields a notice naming `PreToolUse`, checks chosen statuses and the kill switch pass through silently, and checks the handler ends the process once when its own body fails. - Non-vacuity, one mutation at a time, suite re-run, reverted: handler ignores the chosen-status check (137 failures); `stale-path-verify` loses its install line (2); open posture exits 1 (27); dispatcher forgets its release (2); `block-hook-bypass` loses `|| exit 70` (1); closed posture exits 0 (1). Fix pass: the new suite against the previous positional `run-guards.sh` fails exactly the new shift assertion ("expected 'PreToolUse', got ''", 231/1); a copy of `block-no-verify` given `trap 'rm -f "$TMP_SCRATCH"' EXIT` after its install line fails the new trap assertion by file and line. One mutation is NOT caught: removing `trap - EXIT` from the handler (0 failures), because bash itself never re-runs an EXIT trap. The review confirmed this independently: that line survived 15 attack shapes, so on bash it is redundant defense in depth rather than load-bearing; the re-entry test proves the observable property (one notice, terminates), not that line. - Review confirmation of the enforcement claim: the reviewer independently ran 234 cases (22 payloads standalone and dispatched, plus 6 forced-abort modes: `hook-utils.sh` missing, `hook-utils.sh` unparseable, an injected `set -u` abort in every guard, a missing `hook::` helper, jq off PATH, stdin closed) and found no ALLOW/DENY flip anywhere. - A/B against a pristine `git archive origin/main` export of the plugin. (a) Suite level: all 17 existing guardrails suites produce identical `ok:`/`FAIL:` assertion lines on both trees (3,206 assertions on the pristine side), with two explained differences: `require-jq-posture` lists the new library in its census (now excluded, like `hook-utils.sh`), and `block-hook-bypass`'s "symlink: genuine temp write" case fails on EITHER tree when the suite runs from under `/tmp` and passes on either tree from outside it (location artefact, verified both ways). (b) Explicit corpus: 71 payloads (36 Bash, 5 PowerShell, 24 Write/Edit, 6 PostToolUse, 1 Workflow, plus non-JSON and empty stdin) run standalone through every guard and through the dispatcher as `hooks.json` registers it, on both trees: exit code, stdout, and stderr byte-identical across all 71 (branch census: 38 deny, 32 allow at the dispatcher, 1 Workflow-only). No DENY-to-ALLOW flip anywhere. - `bash scripts/affected-tests.sh --run --shard N/4`, N in 0..3, after the fix pass: 153 shell suites PASS (every guardrails suite among them, `abort-boundary.test.sh` at 232/0 under the runner), 1 FAIL: `plugins/claude-ops/skills/plugins/scripts/cache-content-check.test.sh` (process-budget trace probe, 2 of 24 cases), reproduced identically on a clean `git archive origin/main` export, so pre-existing and unrelated. Shards 0, 2, 3 rc 3; shard 1 rc 1 is that one suite. 14 suites in other ecosystems reported NOT RUN by the shell runner. - Parity, all four modes green against current `origin/main`: `--check`, `--check-order`, `--check-bump origin/main`, `--check-preserved origin/main` (159 headings preserved). - ShellCheck 0.11.0 clean on every changed shell file (`-x`, repo `.shellcheckrc`); `check-silent-skips.sh` clean; `check-shell-portability.sh origin/main` clean (22 files); `sync-hook-utils.sh --check`: all 17 copies match, `lib/hook-utils.sh` byte-identical to main; no em dash in any added line; no here-string introduced in production code. - Budget: `strace -f -e trace=clone,clone3,fork,vfork,execve` of the whole Bash dispatcher on a benign command, three repeats per side: creations 23 -> 23, execve 2 -> 2. Wall p50 moved about 2 to 5 ms on this host (recorded in the README budget section). The name lookup is a loop over ten array entries with builtins, no process. Not measured on Windows. Known consequence, stated plainly (verified against the 2.1.258 binary during review): exit 0 plus a valid JSON document IS read (`systemMessage` as a meta message, `additionalContext` accepted for `hookEventName: "PreToolUse"`), while stderr is never read on exit 0, so the stdout document was the correct channel. The trade-off is that `plugins/claude-ops/hooks/hook-failure-audit.sh:146` greps only `hook_non_blocking_error`, so these aborts leave NO hook failure record for that detector, and the bash error line that pristine surfaced through the exit-1 warning is now debug-log only (`claude --debug`). The notice in the transcript and the agent's context replaces the failure record; nothing else does. The issue's second verification item (that detector reporting zero records over a live session with a forced abort) was therefore not run and would hold by construction rather than by observation. The dispatcher's notice names the event only when the payload carries `hook_event_name` (Claude Code's payloads do; a payload without it gets `systemMessage` only). Adjacent and untouched: #3861 (guard fail-open when no interpreter resolves) is a different path; nothing here changes interpreter resolution. ## Related - Refs #3713 (same defect class on four hooks in other plugins; not reached by a guardrails-local library) - Refs #3507, #3130 F5 (the `block-hook-bypass` handler this generalizes) - Refs #3740, #3838 (open `lib/hook-utils.sh` PRs; deliberately not touched) - Refs #3886 (0.32.15, the number below this one), #3849, #3869, #3872 (open guardrails PRs at or below main; CHANGELOG conflict expected) - Refs #3861 (adjacent fail-open path, needs-human, not part of this change) - Refs #3508, #3512, #3517 (environmental trigger and latency work, out of scope) - docs/adr/0004-rightsize-instruction-surfaces-by-incumbent-first-arbitration.md (killed PreToolUse hook yields no decision) 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01ViPsHkL3ng9xWt2GjEQJob --- _Generated by [Claude Code](https://claude.ai/code/session_01ViPsHkL3ng9xWt2GjEQJob)_ --------- Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: ksextonmelodic <ksextonmelodic@gmail.com>

…l-commit (#3514) Derive the telemetry subject inside emit_tel behind the sink gate instead of at file scope; assign effective_dir and the explicit --git-dir through namerefs instead of command substitutions; quote ! alias trailing args with printf -v; name the plugin root by path on the PowerShell lane. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ViPsHkL3ng9xWt2GjEQJob

…kernel trace; 0.32.14 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ViPsHkL3ng9xWt2GjEQJob

…fter #3878 main at 1b68186 (#3878) replaced the shared parser's `< <(printf ...)` with a `${cmd:i:1}` walk, so the one creation the benign pin still allowed for is gone: strace -f -e trace=clone,clone3,fork,vfork,execve now reads 0 creations / 0 execve for this guard's own share on a benign Bash call. Move the pin to 0, re-census every row against the new main (benign 1 -> 0, single-line -m 1 -> 0, blocked multi-line -m 5 -> 2, git wibble 6 -> 4, inline ! alias 10 -> 6, PowerShell 14 -> 12, whole eight-guard matcher 22 -> 21; execve unchanged everywhere), and reword the CHANGELOG, README and test comments that named the parser fork as the remaining creation. Record in a comment that `$_HOOK_SELF/..` resolves `..` physically where `$(cd ... && pwd)` resolved it logically, so a symlinked hooks/ needs CLAUDE_PLUGIN_ROOT. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ViPsHkL3ng9xWt2GjEQJob

cursor Bot mentioned this pull request Sep 6, 2026

perf: drop leftover forks in hook-utils, always-on hooks, and CI scanners #3878

Merged

kyle-sexton marked this pull request as ready for review September 7, 2026 00:54

claude Bot reviewed Sep 7, 2026

View reviewed changes

Comment thread plugins/guardrails/CHANGELOG.md Outdated

claude Bot reviewed Sep 7, 2026

View reviewed changes

Comment thread plugins/guardrails/README.md Outdated

kyle-sexton mentioned this pull request Sep 7, 2026

fix(guardrails): every hook says so when it could not run (#3528) #3895

Merged

Kyle Sexton and others added 3 commits September 7, 2026 13:38

test(guardrails): pin block-noncanonical-commit process creations by …

f649543

…kernel trace; 0.32.14 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ViPsHkL3ng9xWt2GjEQJob

cursor Bot force-pushed the claude/3514-noncanonical-commit-spawns branch from 5d3102b to c2e8519 Compare September 7, 2026 13:41

kyle-sexton merged commit e9fd1de into main Sep 7, 2026
12 checks passed

kyle-sexton deleted the claude/3514-noncanonical-commit-spawns branch September 7, 2026 14:03

Navigation Menu

Uh oh!

perf(guardrails): drop the forks that never exec in block-noncanonical-commit (#3514) - #3886

perf(guardrails): drop the forks that never exec in block-noncanonical-commit (#3514)#3886
kyle-sexton merged 3 commits into
mainfrom
claude/3514-noncanonical-commit-spawns

kyle-sexton commented Sep 6, 2026 •

edited

Loading

Uh oh!

chatgpt-codex-connector Bot commented Sep 7, 2026 •

edited

Loading

Uh oh!

claude Bot commented Sep 7, 2026 •

edited

Loading

Uh oh!

claude Bot commented Sep 7, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Sep 7, 2026

Uh oh!

Uh oh!

Uh oh!

github-actions Bot commented Sep 7, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

kyle-sexton commented Sep 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Fix

Verification

Related

Uh oh!

chatgpt-codex-connector Bot commented Sep 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codex Review Summary

Uh oh!

claude Bot commented Sep 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code review complete

Findings

Code walkthrough (no other issues found)

Uh oh!

claude Bot commented Sep 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Security review complete

Uh oh!

github-actions Bot commented Sep 7, 2026

Uh oh!

Uh oh!

Uh oh!

github-actions Bot commented Sep 7, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

kyle-sexton commented Sep 6, 2026 •

edited

Loading

chatgpt-codex-connector Bot commented Sep 7, 2026 •

edited

Loading

claude Bot commented Sep 7, 2026 •

edited

Loading

claude Bot commented Sep 7, 2026 •

edited

Loading