perf(guardrails): drop the forks that never exec in block-dangerous-git (#3529) - #3872
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Claude finished @kyle-sexton's task in 4m 54s —— View job Code review complete
SummaryNo blocking findings. This is a well-scoped, carefully-argued perf change and I couldn't find a correctness defect in it. Things I specifically checked and found sound:
One thing I could not independently verify: the strace-measured fork/exec counts (the before/after tables in the PR body, CHANGELOG, and README, and the new strace-based pins in the test file). My sandbox doesn't allow running Non-blocking observation: the CHANGELOG/ |
Independent review and the correction it produced — flipped to readyPosted by the autonomous worker lane ( The
|
| Check | Result |
|---|---|
SUBJECT deferral |
Safe — only emit_tel reads it; COMMAND is rewritten only in the PowerShell lane, where the helper never reads it |
printf -v quoting |
32 inputs, 0 diffs — quotes, backslash, newline, tab, glob metas, ~ $ ! # { ( ; | < >, empty, non-ASCII, BOM, ZWSP, U+2028, CR, control byte, invalid UTF-8, trailing newlines |
| Nameref | Single call site, no reachable circular case |
| Deny paths | 176 paired runs, 170 identical; 108 blocked / 68 allowed |
execve |
Flat everywhere, reproduced twice each: benign 3→2 / 0→0, alias 3-arg 8→3 / 0→0, lease 5→3 / 1→1, dispatcher 35→34 / 3→3 |
| Normalization | Clean — 12 CR/BOM/ZWSP/U+2028 probes, 0 flips |
| Mutation pins | Three reproduce; suite 492/0 |
| Version 0.32.13 | Clear across all 10 open guardrails PRs |
The claim that could not have been true
The body said the differential agreed on "full stderr" across 190 runs. A PATH with no git is exactly where the text diverges, and none of those 190 runs had that — the reviewer's own set found 6 diffs, all this case.
Verified independently rather than transcribed: a PATH shadow directory of symlinks to every executable except git, run end-to-end against both trees. Old emitted git: command not found, new emits exec: git: not found; both exit 2, both carry the identical BLOCKED: frame and remedy line, probe status 127 on both. The non-executable case gives rc 126 on both, with the exec form adding a second cannot execute line.
The body, the 0.32.13 CHANGELOG entry, and plugins/guardrails/README.md:395 — which carried the same claim — now say the runs agree on exit code and stderr with one input they did not cover, and name the exact strings. Correcting the README was one file beyond the fix brief; leaving a known-false claim in a shipped doc to stay inside scope would have been the wrong trade.
One wording call left for a reviewer
The CHANGELOG and README say "the 479-case contract suite passes" while the body says the suite went 479 → 492. Defensible as "the 479 pre-existing cases still pass", but a reader checking the suite finds 492. Worth tightening; not blocking.
Criterion (2), "at most 2 spawns", stays unmet: the remaining two forks are $(hook::buffer_stdin) and the parser's < <(printf), both lib/hook-utils.sh work fenced behind #3740 and #3838. The body says so plainly.
Generated by Claude Code
|
Claude finished @kyle-sexton's task in 1m 42s —— View job Security review
No security issues found. This PR touches
Nothing in this diff weakens the block/allow decision logic, widens what's permitted, or introduces a new command/argument-injection path. Consistent with the PR's own A/B verification (190 paired runs, 0 exit-code/stderr differences against Per this lane's scope, GitHub Actions hardening findings are deferred to zizmor and not re-reported here; this diff doesn't touch workflow files regardless. |
|
Last security-reviewed head: |
|
Claude has reviewed this PR 1 time. The lane skips further automatic reviews after 5; deleting this comment resets the count. |
…ners (#3878) <!-- CURSOR_AGENT_PR_BODY_BEGIN --> 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. <!-- CURSOR_AGENT_PR_BODY_END --> <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>
) 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>
0172c45 to
96f201b
Compare
…l-commit (#3514) (#3886) Closes #3514 ## 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 #3869 and #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. ## Related - Refs #3508 (parent: Windows process-creation tax) - Refs #3878 (merged into this branch at `1b681862`; removed the shared parser fork that held the benign figure at 1) - Refs #3740, #3838 (`lib/hook-utils.sh` fork-free forms) - Refs #3869, #3872, #3849 (sibling guardrails spawn PRs; same `emit_tel` shape, and the version numbers they hold) - Refs #1403 / #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.com/claude-code) https://claude.ai/code/session_01ViPsHkL3ng9xWt2GjEQJob --------- Co-authored-by: Kyle Sexton <ksextonclaude@outlook.com> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
fd5f1ae to
c384bbe
Compare
…it (#3529) On every Bash and PowerShell call the guard created three processes of its own and executed none, so the PATH-shim census read it as free. One of the three was this file's: an eager SUBJECT=$(hook::extract_bash_subject) at file scope feeding a telemetry envelope that is off by default and that the verdict never reads. It is now derived inside emit_tel, behind the start-stamp and sink gates. Off the common path, three more sites paid a fork for a string: - the hash-width probe $(git ... rev-parse --show-object-format 2>&1) cost two creations for one exec, because bash execs a substitution's body in its own subshell only when that body carries no redirection. The 2>&1 has to stay inside (git's stderr is the diagnostic the block message quotes), so the body is now `exec git ...`; - a `!` alias reparse spent one $(printf '%q') per trailing argument, now printf -v; - its $(effective_dir ...) around a builtins-only function is now effective_dir_to, a nameref assignment. Kernel census (strace -f -e trace=clone,clone3,fork,vfork,execve, guard share = dispatched minus a no-op guard dispatched the same way): benign and blocked Bash calls 3 -> 2 creations, execve 0 -> 0; lease with a full-width oid 5 -> 3, execve 1 -> 1; `!` alias with three trailing args 8 -> 3; whole Bash dispatcher 35 -> 34, execve 3 -> 3. The two creations left are $(hook::buffer_stdin) and the shared parser's < <(printf ...), both lib/hook-utils.sh work (#3740, #3838), and the PowerShell lane's fourteen live in lib/powershell/ps-command.sh. Verdicts are unchanged: 190 paired runs against origin/main (87 Bash and 8 PowerShell commands, standalone and dispatched, three repository shapes, CR/BOM/zero-width/U+2028 variants) agree on exit code and full stderr; the contract suite passes at 492 and now pins each site by the same instrument, each pin checked by reverting its change alone. Guardrails 0.32.13 (0.32.11 and 0.32.12 are taken by #3849 and #3869). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ViPsHkL3ng9xWt2GjEQJob
…PATH The 0.32.13 CHANGELOG entry and the README hook-budget entry both said the 190 paired A/B runs agree on exit code and "full stderr". They did not cover a PATH carrying no git, and that is the one input where the text moves: the failed lookup now happens inside the exec rather than around it, so the diagnostic the block message quotes reads "exec: git: not found" where it read "git: command not found". Verified directly on both trees with a PATH shadow that excludes git only: old and new both exit 2 with the same BLOCKED message frame and the same remedy line, and the probe status is 127 on both, so the verdict, the branch taken and the guard's behaviour are unchanged. Only the wording of that quoted error differs. Both surfaces now say so instead of claiming byte identity the runs never established. Docs only. No change to block-dangerous-git.sh; the contract suite still passes 492/0 with the strace pins intact. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ViPsHkL3ng9xWt2GjEQJob
origin/main already landed hook::buffer_stdin_to and the parser's fork-free line split, so the two remaining creations this pin named are gone. Move the benign share from 2 to 0 and the !-alias reparse from +1 to +0. The hash-width probe is still one creation over benign. Co-authored-by: ksextonmelodic <ksextonmelodic@gmail.com>
7725b07 to
758a8c9
Compare
Closes #3529
Summary
block-dangerous-git.shis the guard with the highest external-command call-site count in the marketplace, but on the dispatched path it executes nothing: its cost is forks that never exec, which a PATH shim cannot see. On every Bash and PowerShell call the guard created three such processes of its own. This PR removes the one that was this file's to remove on the common path, and three more on the alias and lease paths, in-file only.lib/hook-utils.sh, its per-plugin copies, andrun-guards.share untouched.Fix
SUBJECT=$(hook::extract_bash_subject ...)at file scope fed a telemetry envelope that is off by default and that the verdict never reads. It is now derived insideemit_tel, behind the start-stamp and sink gates.--force-with-lease=<ref>:<hex>):out="$(git ... rev-parse --show-object-format 2>&1)"cost two creations for one exec, because bash execs a substitution's body in the substitution's own subshell only when the body carries no redirection of its own. The2>&1cannot move onto an outer group here (git's stderr is the diagnostic the block message quotes, and an outer2>&1would send it to the hook's stdout), so the body is nowexec git ...: the subshell becomes git. A missing git still lands in the same*error branch, with bash's own "not found" text captured, and the wording of that captured text changes: see the stderr note under Verification.!alias reparse: one$(printf '%q')per trailing argument is nowprintf -v;$(effective_dir ...)around a builtins-only function is noweffective_dir_to, a nameref assignment (the default base is read before the nameref is written, so the caller's scratch variable then assignsHOOK_EFFECTIVE_BASE).$(dirname ...)was already gone from this file before this PR (${BASH_SOURCE[0]%/*}); the scout's note was stale. The PowerShell lane's$(cd "$_HOOK_SELF/.." && pwd)is left alone: it runs only whenCLAUDE_PLUGIN_ROOTis unset, which Claude Code never leaves unset.mainis 0.32.10; perf(guardrails): hoist three convention-gate redirects off their substitutions #3849 takes 0.32.11 and perf(guardrails): block-hook-bypass drops six forks that never exec (#3513) #3869 takes 0.32.12, re-verified againstorigin/mainand the open PR list immediately before opening this). README hook-budget accounting entry added per hook-budget Rule 1.Verification
Kernel census (
strace -f -e trace=clone,clone3,fork,vfork,execve; guard share =run-guards.sh block-dangerous-git.shminus a no-op guard dispatched the same way; this repository as cwd,HOOK_TELEMETRY_SINKunset,CLAUDE_PROJECT_DIRempty; three identical repeats). strace rather than a PATH shim or xtrace because the subject is a fork that never execs, which neither of those can see. execve reported separately: it does not move anywhere, which is the evidence this is latency, not removed work.git status --short/echo hellogit push --force origin main/git reset --hard--force-with-lease=main:<40-hex>!alias no trailing args!alias with three trailing args!alias whose body carries a leasegit statusbash block-dangerous-git.sh, benignDeny paths still deny. A/B against a pristine
origin/mainexport on exit code and stderr: 190 paired runs, 0 differences. 87 Bash commands (every form the guard matches:--force,-f,+refspec,--mirror, bare/=ref/=ref:movable/=ref:<40-hex>/=ref:<64-hex>/--force-if-includes/--no-force-with-lease/--dry-runlease spellings,reset --hard/--h,clean -f/-fd/-fdx/--force,checkout ./://-f/--pathspec-from-file/exclude-only,restore .,switch --discard-changes; their near-miss safe variantsreset --keep,clean -n,checkout -- file,restore --staged .,branch -D,filter-branch, quoted text;!and inline aliases,bash -c/sh -c/env/sudowrappers, multi-segment lines) and 8 PowerShell commands, standalone and dispatched, across a SHA-1 repo, a SHA-256 repo, a non-repo and a nonexistent-Cdir (probe-failure path). Verdict tally on the new tree: 51 blocked / 36 allowed Bash, 5 / 3 PowerShell.One stderr string is not identical, and it is not covered by those 190 runs. Every run above had
gitonPATH. On aPATHcarrying nogitthe lookup now fails inside theexecrather than around it, so the diagnostic the block message quotes changes wording:<hook>: line 341: git: command not found<hook>: line 363: exec: git: not foundMeasured directly on both trees, running the real hook against a
PATHshadow that excludesgitand nothing else: both exit 2, both emit the sameBLOCKED: ... hash format could not be determined (...)frame and the same remedy line, and the probe's own status is 127 on both, so the same*branch runs and the push is blocked either way. The claim this section previously made, "full stderr", overstated that: verdicts and stderr agree everywhere the 190 runs reached, and on the git-absent probe the only thing that moves is the wording of a quoted error message on a path that still denies. (Related, same mechanism: agitonPATHthat is present but not executable is rc 126 on both trees, and theexecform appends a secondcannot execute: Permission deniedline.) The 0.32.13 CHANGELOG entry and the README hook-budget entry state the same qualification.Permissive-normalization check. Field reads are not touched by this PR. Probed anyway: a CR mid-token (
git push --for\rce origin main) and after the token are both blocked, identically on standalone and dispatched paths and on both trees, so the CR strip already lives in the library's field read rather than in the dispatcher cache, and for this guard it acts in the restrictive direction. BOM-prefixedgit, a zero-width space glued to--force, and U+2028 glued to--forceare allowed on both trees (the bytes make the token something bash would hand to git verbatim, and git rejects it); no divergence introduced.Contract suite.
block-dangerous-git.test.sh479 → 492, all passing (re-run at the tip of this branch:PASS=492 FAIL=0, strace pins included). The 13 new assertions are strace-based pins: benign share exactly 2 creations / 0 execve; blocked share equals benign;!alias reparse is benign + 1 (the shared parser's re-entry); trailing alias arguments add 0; lease probe is benign + 1 creation and exactly 1 execve; and a per-process check that thegit rev-parse --show-object-formatexec's parent itself exec'd. Skips visibly where strace is absent. Non-vacuity proven by mutation, each change reverted alone: eager subject → benign pin fails (3);execremoved → lease pins fail (4,extra-fork);$(printf)restored → trailing-args pin fails (6);$(effective_dir)restored → alias pin fails (4).Gates.
scripts/affected-tests.sh --run: every selected suite passes exceptplugins/claude-ops/skills/plugins/scripts/cache-content-check.test.sh(2 of 24, PS4 trace probe), which fails identically on a cleanorigin/mainexport, so it is pre-existing and host-specific, not this change.check-changelog-parity.sh--check,--check-order,--check-bump origin/main,--check-preserved origin/main: all four pass.check-killswitch-hoist.shpasses.check-purged-em-dashes.shpasses. shellcheck and shfmt clean on both shell files; markdownlint-cli2 clean on CHANGELOG and README.Acceptance criteria in #3529, stated plainly. (1) Spawn count before/after: reported above, by kernel trace, a superset of the PATH-shim count the issue asked for. (2) "No more than 2 external process spawns on the common path": this guard's own share is now 0 execs and 2 forks; the two forks are
$(hook::buffer_stdin)and the shared parser's< <(printf ...), bothlib/hook-utils.sh(#3740, #3838) and outside this PR's fence, so that line is not closed from inside this file. (3) "A non-gitcommand exits before any spawn": this guard spawns nothing of its own on any command; the dispatcher's twojqexecs are batch-wide since #3788 and not attributable to this guard. (4) Behavioural tests pass and every form blocked before is still blocked (190/190 A/B on verdict). (5) Wall-clock alongside concurrent-process count: not reported. This Linux host's spawn floor is under a millisecond, so a timing here would not transfer to the Windows spawn tax the parent describes; the process count is the durable figure.Related
$(hook::buffer_stdin)fork-free form in 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; the PowerShell lane's 14 creations live inlib/powershell/ps-command.sh.🤖 Generated with Claude Code
https://claude.ai/code/session_01ViPsHkL3ng9xWt2GjEQJob
Generated by Claude Code