perf(hooks): cut leftover Bash parse and dirname tax on the hot path - #3788
Merged
Conversation
The ~41 KB PowerShell classifier returns 0 immediately on Bash, so a file-scope source and the dispatcher's --lib were parse tax. Isolation subshells do not inherit the include guard, so both the dispatcher skip and the per-guard lazy source are required. Spawn census stays at 5 (3 git + 2 jq). bash -x source of ps-command.sh: 5 → 0. Wall p50 51.9 → 46.8 ms on this measurable host. Co-authored-by: ksextonmelodic <ksextonmelodic@gmail.com>
Default zone_hook_mode is advisory, so the gate is inert. Parsing the shared library to discover that was the entire cost of the default path. The MODE check now sits above every source, the same shape as the kill-switch hoist. Spawn census stays at 0. bash -x source of hook-utils.sh: 1 → 0. Wall p50 4.8 → 1.4 ms on this measurable host. Blocking mode is unchanged. Co-authored-by: ksextonmelodic <ksextonmelodic@gmail.com>
Production hooks still used source "$(dirname "${BASH_SOURCE[0]}")/…"
and several formatters walked parents with $(dirname). GNU Bash forks a
subshell for every command substitution even when the body is a builtin.
The house pattern is ${BASH_SOURCE[0]%/*} / ${FILE%/*} with the
bare-filename and root fallbacks.
bash-format matching .sh Write, spawn census: 9 → 4 (5 dirname gone).
What each hook checks is unchanged.
Co-authored-by: ksextonmelodic <ksextonmelodic@gmail.com>
ps-command.sh is no longer sourced on Bash, so PS_HERESTRING_PLACEHOLDER is unset. The -m newline scan compared against it anyway, and set -u turned a single-line git commit -m into exit 1. Compare the token only when the classifier defined it. Co-authored-by: ksextonmelodic <ksextonmelodic@gmail.com>
.claude/hooks/hook-telemetry-sink.sh is a copy of the plugin sink with a different hook-utils path. It now uses the same BASH_SOURCE strip as the plugin copy, and the drift test strips source \$HOOK_DIR/ as well as the old dirname form. Co-authored-by: ksextonmelodic <ksextonmelodic@gmail.com>
kyle-sexton
marked this pull request as ready for review
September 6, 2026 02:56
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. |
kyle-sexton
enabled auto-merge (squash)
September 6, 2026 06:37
Resolve changelog and version collisions with main's path-placeholder releases: guardrails 0.32.9, source-control 0.55.57, claude-ops 0.42.16. Keep main's 0.32.8 / 0.55.56 / 0.42.15 entries and move this branch's notes onto new headings so changelog-parity stays strictly above main. Co-authored-by: ksextonmelodic <ksextonmelodic@gmail.com>
The strip pattern matches a literal source "$HOOK_DIR/" line, so the dollars must stay single-quoted. ShellCheck treated that as an expansion bug and failed the lint lane. Co-authored-by: ksextonmelodic <ksextonmelodic@gmail.com>
kyle-sexton
added a commit
that referenced
this pull request
Sep 6, 2026
No related issue: a documentation inaccuracy found by a post-merge verification of this topic's four PRs, small enough to fix directly rather than file. ## Summary #3769 (claude-ops 0.42.11) moved the reference sink's routing to the envelope spine, `.session_id // .data.session_id`, and left the file header saying the route is "decided by the envelope's `data.session_id`". That header is the sink's only description of its own routing rule, so a reader of the file got the pre-1.1 answer. `docs/conventions/hook-telemetry/README.md` states it correctly, which is what makes this an oversight rather than a disagreement about intent. ## Fix The header now names the spine key first and the `data.session_id` fallback second, matching line 62, and states the spine key as **optional**: contract 1.1 has a producer carry `session_id` when its payload held a well-formed one and omit it otherwise, in which case this sink takes the legacy route. Applied to both copies, `plugins/claude-ops/hooks/hook-telemetry-sink.sh` and the repo-local `.claude/hooks/hook-telemetry-sink.sh`. Those two files are not byte-identical and are not meant to be — the repo-local copy reaches `hook-utils.sh` and `session-log-lib.sh` by relative path because they are not colocated with it, and says so in its own comment. The stale header had drifted identically in both, so both needed the same correction. Comment only. No behavior change. claude-ops 0.42.16 to **0.42.17**. ## Review findings, both addressed - **Codex (P2):** the first correction said `session_id` is "carried by every 1.1 producer", which overstates a contract that makes the four correlation keys optional and has `hook::emit_telemetry` omit absent or malformed ones. Both citations checked before changing anything; reworded in `a1713a1f`, thread resolved. - **A fresh-context verifier gating this PR** found the version bump had collided: `main` shipped claude-ops 0.42.16 in #3788 while this PR was open. Renumbered to 0.42.17 in the merge commit, with #3788's released 0.42.16 entry kept intact beneath this change's. ## Verification - Comment-only proven three ways rather than asserted: comment-and-blank-stripped executable bodies compare byte-identical; `shfmt -mn` (a real shell parser) gives matching SHA-256 per file pair across base and head; every one of the 8 changed lines begins with `#`. File modes unchanged. - Behavior driven, not inferred: the sink itself run 8 times across {base, head} × {plugin copy, repo-local copy} × {spine-only envelope, data-only envelope}, each into a fresh project dir. Every base/head pair produced a byte-identical JSONL line and the same route. - `plugins/claude-ops/hooks/hook-telemetry-sink.test.sh`: PASS=41 FAIL=0. `scripts/check-changelog-parity.sh --check-bump origin/main`, `--check`, `--check-preserved origin/main`, `scripts/validate-plugins.sh`, `scripts/sync-hook-utils.sh --check`, `scripts/check-hook-wiring-liveness.sh`: all pass. shellcheck clean on both files. - `scripts/affected-tests.sh --run`: only pre-existing failures, `cache-content-check.test.sh`'s two "process budget" cases, which reproduce on unmodified `main`. ## Related - Refs #3769, which introduced the spine-first route and left this header behind; #3758, the contract it serves; #3788, which took 0.42.16. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_019DaWEB8Daq1xAXy2Xj1Pme --------- Co-authored-by: Claude <noreply@anthropic.com>
kyle-sexton
added a commit
that referenced
this pull request
Sep 6, 2026
<!-- CURSOR_AGENT_PR_BODY_BEGIN --> No linked issue ## Summary Phase 1 of a measured leftover after the hook-performance program: the always-on Bash dispatcher no longer spends git processes on a command that cannot be a commit. Isolation forks (#3685) are unchanged. ## Fix - `block-noncanonical-commit` skips persisted `git config --get alias.<sub>` when `<sub>` is a current **non-deprecated** git builtin that already existed in git 2.25. git-config ignores aliases that hide existing Git commands except for deprecated commands (https://git-scm.com/docs/git-config, fetched 2026-09-06). git.c marks `whatchanged` and `pack-redundant` `DEPRECATED`; git 2.51+ honors `alias.whatchanged = commit` (`t/t0014-alias.sh`). Those names stay probed. Names added after 2.25 (`bugreport` 2.27, `maintenance` 2.31, `diagnose` 2.38) stay probed so an older git can still alias them. Asking the installed git for `git --list-cmds=builtins` would put a spawn back on every `git status`. Empirical on git 2.43: `alias.status = commit` still runs `status`; `alias.ci` of the same expansion creates the commit. A leftover `alias.status = commit` can no longer false-block `git status`. Non-builtin aliases (`git ci`, `git qc`) still resolve as before. - `block-convention-violation` loads the convention pair (`git rev-parse --show-toplevel` plus the resolver) on first commit or `gh pr create`, not on every Bash fire. Telemetry subject extraction moves inside `emit_tel` after the sink check. What this phase does **not** do: - Per-guard `$(source …)` isolation forks (#3685) - The remaining two primed `jq` parses on the dispatcher - Splitting `hook-utils.sh` - Exec-form hooks (#3686) ## Verification Host qualified with `plugins/performance/lib/spawn_noise.py`: measurable (min 0.4 ms, spread 2.26×). Same-session before is `origin/main` at `5101f5a2` (the two guards only). `HOOK_TELEMETRY_SINK` unset. Spawn census through a stable PATH shim (`plugins/performance/scripts/spawn-census.sh`). Unchanged after dropping `whatchanged` / post-2.25 names from the skip list. | Subject | Counter | before | after | |---|---|---|---| | Bash dispatcher (`git status --short`) | PATH-shim spawns | 5 (`3 git`, `2 jq`) | 2 (`2 jq`) | | Bash dispatcher (`echo hello`) | PATH-shim spawns | 3 (`1 git`, `2 jq`) | 2 (`2 jq`) | | `git status --short` | wall p50 / p95 (n=20) | 58 / 59 ms | 42 / 43 ms | | `echo hello` | wall p50 / p95 (n=20) | 53 / 55 ms | 41 / 42 ms | | `git config --get alias.status[.command]` | `bash -x` | 2 | 0 | | `git rev-parse --show-toplevel` on `echo hello` | `bash -x` | 1 | 0 | The milliseconds are context on this cheap-spawn host. The durable claim is the three git processes that disappeared. - `plugins/guardrails/hooks/block-noncanonical-commit.test.sh` PASS=216 FAIL=0 - `plugins/guardrails/hooks/block-convention-violation.test.sh` PASS=78 FAIL=0 - `plugins/guardrails/hooks/run-guards.test.sh` PASS=101 FAIL=0 - `scripts/affected-tests.sh --run` on the earlier revision: 7 selected suites, 0 failed - `scripts/check-changelog-parity.sh --check` and `--check-bump origin/main` clean ## Related Refs #3685 Refs #3788 Refs #3686 <!-- CURSOR_AGENT_PR_BODY_END --> <div><a href="https://cursor.com/agents/bc-33b90393-8f81-4181-8909-024a610fdce6?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-33b90393-8f81-4181-8909-024a610fdce6&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>
![https://cursor.com/assets/images/open-in-web-dark.png"><source](https://cursor.com/assets/images/open-in-web-dark.png"><source){kind=link}
![https://cursor.com/assets/images/open-in-web-light.png"><img](https://cursor.com/assets/images/open-in-web-light.png"><img){kind=link}
![https://cursor.com/assets/images/open-in-web-dark.png"></picture></a> <a](https://cursor.com/assets/images/open-in-web-dark.png"></picture></a> <a){kind=link}
![https://cursor.com/assets/images/open-in-cursor-dark.png"><source](https://cursor.com/assets/images/open-in-cursor-dark.png"><source){kind=link}
![https://cursor.com/assets/images/open-in-cursor-light.png"><img](https://cursor.com/assets/images/open-in-cursor-light.png"><img){kind=link}
![https://cursor.com/assets/images/open-in-cursor-dark.png"></picture></a> </div](https://cursor.com/assets/images/open-in-cursor-dark.png"></picture></a> </div){kind=link}
This was referenced Sep 6, 2026
Open
kyle-sexton
pushed a commit
that referenced
this pull request
Sep 7, 2026
…acement (#3871) Closes #3509 ## Summary `pr-body-linkage-gate.sh` timed out on **every** recorded run in the measurement window — 423 timeouts against a 15 s ceiling, the worst blocked count in the #3508 campaign. A gate that is killed before it renders a verdict protects nothing, so its cost is part of its contract. Its MCP-surface sibling `pr-linkage-mcp-gate.sh` and the validator both share is fixed here too. **The parent's stated cause does not hold, and this PR does not act on it.** #3508 attributes the cost to per-field `jq` forks needing a new shared helper in `lib/hook-utils.sh`. Shard #3520 (PR #3779) established the real mechanism, and merged PR #3788 fixed 34 hook scripts across 17 plugins while touching `lib/hook-utils.sh` **zero times**. This PR follows that precedent: **`lib/hook-utils.sh` and every `plugins/*/hooks/hook-utils.sh` copy are untouched** (unmerged PRs #3740 and #3838 own that file). The fix is entirely in-file. The mechanism, re-verified on this host before any edit was made: | Form | clone-family | `execve` | | --- | --- | --- | | `V=$(jq . f)` | 1 | 1 | | `V=$(jq . f 2>/dev/null)` | **2** | 1 | | `{ V=$(jq . f); } 2>/dev/null` | **1** | 1 | | `V=$(printf \| jq \| tr)` | 4 | 2 | | `V=$(cat -- f)` | 1 | 1 | | `V=$(<f)` | **0** | **0** | | `V=$(shellfunc)` | 1 | 0 | | out-variable call | **0** | 0 | | `read < <(printf …)` | 1 | 0 | Bash elides the extra fork and execs in the command substitution's own subshell only when the command carries no redirection of its own. A `2>/dev/null`, a `<<<`, or a pipeline inside the substitution defeats that. ## Fix Four shapes, all in-file: 1. **Per-field `jq` batched into one process.** Both gates read payload fields through `printf '%s' "$INPUT" | jq -r … 2>/dev/null | tr -d '\r'`, once per field — 4 clones and 2 execs each, **five times over** on the MCP surface, all asking about one buffered string. One `hook::jq_fields` call (the library's existing batched reader, called not changed) answers every field, and CR-strips exactly as the `tr` did. 2. **Redirection hoisted onto the enclosing group.** `ORIGIN=$(git … 2>/dev/null || true)` became `{ ORIGIN=$(git …) || ORIGIN=""; } 2>/dev/null`. The group holds exactly one command, so nothing beyond that git call is silenced. 3. **The shared validator's helpers write into a caller-named variable.** `strip_html_comments`, `mask_markdown_code`, `section_content` and `trim` were each read through `$(…)` over a `< <(printf …)` line reader: **12 forks and zero extra `execve` per judged body** — pure process-creation latency. They now use `printf -v` and an in-shell line split. `linkage::chomp_to` reproduces the trailing-newline strip that command substitution performed, which is the one thing a naive out-variable conversion gets wrong. 4. **`$(<file)` for the `--body-file` read, `printf -v '%q'` for wrapper re-quoting, `hook::json_str_object_to` for the telemetry envelope** — replacing a `cat`, a `printf` substitution, and a `jq -n` that bash can do itself. The `$(dirname …)` source line was already fixed in both gates by #3771. `pr-body-linkage-gate.sh` and `pr-linkage-validator.sh` now invoke **no external command of their own at all**; every remaining spawn on their path belongs to `lib/hook-utils.sh`. `pr-linkage-mcp-gate.sh` keeps three, each justified: the origin-remote scope guard, the `jq -e` defer-guard on a repo's own `settings.json` (behind a `[[ -f ]]` probe), and a carriage-return fallback described below. ### Where the MCP reader's behaviour could change, and what it does about each `hook::jq_fields` CR-strips every value it returns; the validator only strips a CR at end of line. A body with a **mid-line** CR would therefore be judged against different text — and stripping is the **permissive** direction: `## Sum<CR>mary` becomes a section that was previously missing, turning a BLOCK into a silent ALLOW. The batch therefore also reports whether the raw body holds a CR at all, and the MCP body is re-read losslessly with its own `jq` only in that case, which no real payload hits. Review of the first revision found two field shapes where that batched reader itself flipped a per-field DENY to ALLOW; both are fixed in the second commit (`fe3fd235`), and the differential below was re-run over them: - **The CR probe was not type-safe.** `contains("\r")` errors on a non-string body, one erroring filter fails the whole batch, and a failed batch exited 0 — so a body of `5`, `true`, `{"a":1}` or `["x"]`, which the per-field reader rendered as text and blocked, was allowed. The probe now goes through `tostring` first, so it is total over every JSON type. A batch that **still** fails (a `tool_input` or payload root that is not an object) falls back to the per-field reads it replaced instead of allowing outright, so a batch failure is now exactly as fail-closed as the per-field reader was: a determinable bad body still blocks. That fallback is unreachable for any object `tool_input`, so the fenced spawn counts do not move. - **Trailing newlines survived on the exact-match fields.** `$( )` chomped them from every per-field read, so `"owner": "acme-corp\n"` or a tool name with a trailing newline matched the guards and was gated; `hook::jq_fields` keeps the newline and both slipped past. `TOOL`, `HOOK_CWD`, `T_OWNER`, `T_REPO` and `BODY` are now chomped in-shell, byte-identical to their `$(jq -r …)` form whenever they carry no CR. **One accepted stricter change remains.** A CR *inside* `tool_name`, `owner` or `repo` used to stay in the value, so it never matched and the call was allowed; `hook::jq_fields` strips it, the value matches, and the body is judged. Neither GitHub nor the MCP server produces such a value; the stricter direction is kept and named in the CHANGELOG rather than asserted away. ## Verification **Behaviour, proven by differential rather than argued.** - **End-to-end, 154 payloads (85 Bash, 69 MCP), exit code, stdout and stderr compared byte-for-byte** between the merge-base gates and these, after `fe3fd235`. Bash surface: every body-flag spelling (`--body`, `-b`, `--body=`, `--body-file`, `-F`, attached forms), stdin and substitution heredocs, multiple-heredoc and unterminated cases, missing body files, `--repo`/`-R` and `cd` escapes, `env -S` / `env` / `sudo` wrappers, `gh.exe` and `./gh`, CRLF and mid-line-CR bodies, NUL bytes, NBSP/BOM/ZWSP/U+2028/U+2029, `--fill`/`--web`, absent and non-string `command`, trailing-newline and CR `cwd`. MCP surface: the tool x owner x body matrix, absent/empty/null body, non-string bodies (`5`, `true`, `{"a":1}`, `["x"]`, `false`), absent and non-object `tool_input`, NUL and CR in `owner`, trailing newline and CR in `owner`/`repo`/`tool_name`/`cwd`. **Bash gate: 85/85 verdict-identical. MCP gate: 67/69 verdict-identical; the remaining 2 are the CR-in-`owner` and CR-in-`tool_name` cases above, ALLOW to DENY.** Before `fe3fd235` the same run showed 7 DENY-to-ALLOW mismatches (the four non-string bodies, `owner\n`, `repo\n`, `tool_name\n`); all 7 are gone and no new one appeared. Bash's own "ignored null byte" warning line, whose text carries the script path, is excluded from the byte comparison. Denies: 69 at the merge base, 71 here. - **Validator differential, 425 bodies**, hand cases plus a seeded fuzz corpus over heading/fence/comment/CR/NBSP/backtick-run tokens. **0 mismatches.** - **Trailing-newline invariance** proved separately, because `hook::jq_fields` preserves trailing newlines where `$(printf | jq)` stripped them: 24 validator cases and 9 hook cases, 0 mismatches; the MCP gate's own fields are now chomped as well. - Contract suites: `pr-body-linkage-gate.test.sh` **146/146**, `pr-linkage-mcp-gate.test.sh` **37/37** (nine new cases: the four non-string bodies block, `owner\n` / `repo\n` / `tool_name\n` still gate, CR-in-owner gates as the accepted stricter case, a string `tool_input` takes the fallback and allows). **Cost, measured with `strace -f -e trace=clone,clone3,fork,vfork,execve`** — not an xtrace command count, which reads source positions rather than kernel spawns (#3520 measured xtrace at 2 against 8 real spawns on one script). Telemetry sink off; re-measured after `fe3fd235`, unchanged: | Path | clones before | clones after | `execve` before | `execve` after | | --- | --- | --- | --- | --- | | Bash gate, a `gh` call with no `pr` | 8 | **7** | 3 | **2** | | Bash gate, `gh pr create` with a body (ALLOW) | 28 | **11** | 6 | **3** | | Bash gate, `gh pr create` with a body (BLOCK) | 28 | **11** | 6 | **3** | | MCP gate, unrelated tool | 7 | **7** | 2 | **2** | | MCP gate, create (ALLOW) | 37 | **11** | 9 | **4** | | MCP gate, create (BLOCK) | 37 | **11** | 9 | **4** | Two components: the validator refactor alone removes **12 clones with `execve` unchanged** — that half is pure latency, no work removed. The rest is genuinely duplicated work removed: six `jq` processes re-parsing one buffered payload, plus two `tr` calls deleting a byte class bash rewrites in place. **No wall-clock figure is claimed.** This is a Linux host where a spawn costs ~3-5 ms; the campaign's host measures 0.3-0.9 s and is bimodal at 501 concurrent processes. A timing here would say nothing about there, so the process count is reported as the proxy, per #3508's own correction. **New gate: `hooks/pr-linkage-spawn-budget.test.sh`.** Ceilings are the measured counts with **no headroom**, per `hook-budget.md` rule 2. It refuses to report a pass it has not earned: - a self-check first proves the harness can distinguish `$(cmd 2>/dev/null)` from `{ …; } 2>/dev/null` at the kernel level, and **SKIPs** rather than passing if it cannot (no ptrace, no strace); - three mutants must each raise the count above the ceiling or **the suite fails itself**: a redirect moved back inside a substitution (11 -> 12 clones), one field split back out of the batch (11 -> 14 clones, 4 -> 5 execve), a validator helper re-forking (11 -> 13 clones); - it asserts both gates still exit 2 on a failing body, so a budget of zero spawns cannot pass as a no-op. Pointed at HEAD's hooks the new suite reports **13 failures**; against this branch, 23/23 pass. **Gates run** (re-run after `fe3fd235`). `scripts/affected-tests.sh --run`: 151 shell suites pass, 14 selected suites belong to ecosystems the runner does not execute (reported NOT RUN, not skipped), one pre-existing failure noted below; `scripts/check-changelog-parity.sh` in all four modes (`--check`, `--check-order`, `--check-bump origin/main`, `--check-preserved origin/main`) all rc=0; `shellcheck -x` and `shfmt -d` clean on all five scripts; `markdownlint-cli2` and `editorconfig-checker` clean on the changed files; `check-shell-portability.sh`, `check-purged-em-dashes.sh`, `check-silent-skips.sh`, `check-discriminating-test-skips.sh`, `check-killswitch-hoist.sh`, `check-hook-exec-form.sh`, `check-fixture-git-isolation.sh` all rc=0 on the first revision. Manifest bumped 0.55.58 -> **0.55.60** with the matching CHANGELOG entry (see Related for why not 0.55.59), and the README carries the measured share per `hook-budget.md` rule 1. **Pre-existing failures, not from this branch** (both reproduce on a clean `HEAD` checkout, and this branch touches no file either reads): `plugins/claude-ops/skills/plugins/scripts/cache-content-check.test.sh` (2 cases, its own xtrace-based budget probe) and `plugins/session-flow/scripts/tests/test_save_point.py::test_new_origin_falls_back_to_directory_name`. ### Acceptance criteria: two are not fully met, stated plainly | # | Criterion | Status | | --- | --- | --- | | 1 | No more than 2 external spawns on the common path (own shell + at most one `jq`) | **NOT MET — over by one.** The common path is now the hook's own shell, one `jq -e .`, and one batched `jq`. The batched `jq` is the criterion's allowance; the `jq -e .` is `hook::buffer_stdin`'s payload validation inside `lib/hook-utils.sh`, which this PR is fenced off from. Removing it is that library's change to make, in #3740/#3838. | | 2 | `grep`/`sed`/`cut`/`tr`/`basename`/`dirname` on the hot path replaced with builtins | **MET.** None appears in any of the three scripts; `cat` is gone too. | | 3 | Matcher or early guard exits before any spawn for non-matching invocations | **PARTIALLY MET.** The `if: Bash(*gh *)` filter (pre-existing) removes the hook process entirely for non-`gh` calls, and the two in-hook guards short-circuit before any repo I/O. They cannot run before `hook::buffer_stdin`, whose `jq` is the same library spawn as row 1 — the hook must read stdin before it can know what it is looking at. | | 4 | Existing behavioural tests still pass; the guard still blocks what it blocked before | **MET, with one named exception.** 183 contract cases plus the 154-payload differential: every merge-base DENY is reproduced, and two merge-base ALLOWs (a CR inside `owner` or `tool_name`) are now DENY, the stricter direction, recorded in the CHANGELOG. | | 5 | Under 2 s for a single run on a Windows host | **NOT VERIFIED.** No Windows host available; the process-count proxy above is offered instead, and no timing figure is invented. | ## Related - Parent: #3508 (Windows process-creation tax). Its stated cause — per-field `jq` needing a new shared `hook-utils.sh` helper — is not what this PR acts on; see Summary. - Precedent: #3520 / PR #3779 (established redirection placement as the real mechanism) and merged PR #3788 (34 scripts, 17 plugins, `lib/hook-utils.sh` untouched). - **Version and overlap with #3838.** `main` is at `source-control` 0.55.58. Open, ready PR #3838 (`cursor/shell-script-perf-phase1-bb5b`) bumps this plugin to **0.55.59** with its own `## [0.55.59]` heading, and edits **both gate files this PR touches** (`pr-body-linkage-gate.sh`, `pr-linkage-mcp-gate.sh`, introducing `hook::buffer_stdin_to`) plus the three worktree gates and the synced `hook-utils.sh`. This PR therefore takes **0.55.60**, verified against `main`, #3838, #3774 (0.55.58, no bump) and #3740 (tops at 0.55.54). Whichever of #3838 and this PR lands second needs a **rebase of the two gate files, not just a rebump**; that is the merge lane's call, flagged here so it is not a surprise. Sibling #3510 is in flight against this plugin and takes the next version after this one. - Fenced off: unmerged PRs #3740 and #3838 own `lib/hook-utils.sh`; #3510 owns `worktree-add-containment-gate.sh`, `worktree-add-claim-gate.sh` and `worktree-create-gate.sh`, which share this plugin's `CHANGELOG.md` and manifest. Neither set is touched here. - Prior art acknowledged: #1403 / PR #1385, whose revival bar (four contract-test regressions, two failing open) is what the deny-preserving differential above is aimed at. - Budget authority: `docs/conventions/hook-budget/README.md` (#1809), surfaced by `.claude/rules/hook-budget.md`. 🤖 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>
cursor Bot
pushed a commit
that referenced
this pull request
Sep 7, 2026
… off substitutions The Stop-event unsurfaced-failure detector timed out 113 times in the #3508 window at a 21.9 s average, roughly 44x the 500 ms the hook-budget convention gives the whole always-on per-turn set. The parent issue blames per-field jq forks needing a shared helper; that diagnosis is wrong here, as shard #3520 and PR #3788 established twice. The cost is redirection placement. Bash runs the command of a command substitution in the substitution's own subshell and skips the extra fork ONLY when that command carries no redirection of its own. `$(wc -c <file 2>/dev/null)` is two processes for one byte count; `$(cat -- file 2>/dev/null)` is two for a file read; `{ V=$(cmd); } 2>/dev/null` is one. Those forks are invisible to `bash -x`, which reads command positions, which is why the campaign kept mis-attributing the cost to the exec count. Six sites in hook-failure-audit.sh change and no shared library does: - `wc` names the transcript instead of redirecting it in, and `read` drops the filename column along with the padding some wc builds add. - The `read_window` helper is gone. Under the tail cap grep opens the transcript itself rather than being fed by `cat` through a function call that was a second subshell on top of the substitution's own. - The marker read is `$(<file)`, which forks nothing and execs nothing. - The marker write strips carriage returns in the shell instead of piping through `tr`. - The marker directory is probed with `-d` before `mkdir -p` spawns. - The two payload fields come from one `hook::jq_fields` pass instead of two `hook::jq_field` calls. Over the tail cap the `tail | sed | grep` pipeline and its redirect placement are untouched: a pipeline element forks either way, and hoisting the redirect onto a group would newly silence sed and grep for no saving. The `printf | jq` feeding the structural selection also stays a pipeline, because a here-string at or above the pipe capacity deadlocks before jq is exec'd and that payload routinely clears it; it is off the common path regardless. Common path (a turn with no failure recorded, under the cap): 18 process creations and 6 execs before, 9 and 4 after. Over the cap: 19 and 7 before, 12 and 6 after. A turn that warns: 34 and 17 before, 24 and 14 after. Measured with `strace -ff -e trace=clone,clone3,fork,vfork,execve`. No wall-clock figure is claimed: this Linux host says nothing about the Windows spawn tax the budget binds to, and on the #3508 host one creation costs 180-2,841 ms. Behaviour is unchanged, which for this hook is the whole point: it exists to surface failures that otherwise pass unnoticed, so a hoisted `2>/dev/null` could silence exactly the diagnostic it is for. Every group holds one command, and the one stream now silenced that was not before is grep's, which `cat`'s own redirect already discarded. Pre- and post-change stdout, stderr, exit codes and marker contents were compared byte for byte across ten scenarios: first warn, dedup, a new failing registration re-warning while the warned one stays muted, all three classification branches, the tail cap, a clean transcript, a missing transcript, the kill switch, and no data directory. Identical. The contract test gains an strace budget assertion on both counts, mutation- checked: moving either silenced redirect back inside its substitution adds a fork with no new exec, leaves every behavioural assertion green, and trips the creation ceiling. The README states the measured share per hook-budget Rule 1, as a process count, and says plainly that the Windows parallel-wall figure is still owed. Two acceptance criteria are not met and are called out in the PR: the hook cannot exit before any spawn, since reading the transcript is how it learns there is nothing to report, and the 500 ms parallel-wall figure for the three-plugin per-turn set is cross-plugin and Windows-only. Refs #3508. Precedent #3779, #3788. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ViPsHkL3ng9xWt2GjEQJob
kyle-sexton
added a commit
that referenced
this pull request
Sep 7, 2026
…ach (#3873) Closes #3510 ## Summary The three `source-control` worktree gates spawned four processes to read each hook payload field. The parent campaign (#3508) blames per-field `jq` forks needing a shared helper; that diagnosis is wrong and this is the fourth PR to show it. Part of the cost is a **fork that never execs**, part is a `| tr -d '\r'` stage behind every read, and all of it is in-file: > `$(cmd)` = 1 process. `$(cmd 2>/dev/null)` = 2. `$(a | b | c)` = 4. `{ v=$(cmd); } 2>/dev/null` = 1. `$(printf '%s' "$X" | cmd)` = 3. Measured on this branch with `strace -f -e trace=clone,clone3,fork,vfork,execve`, confirming the mechanism before touching anything. `lib/hook-utils.sh` and its 17 synced plugin copies are **untouched**. Reported at **418 timeouts**, worst hook in the set after #3511. A scout counted roughly 7 in-substitution redirect/pipe sites; the actual count is 8: three in `worktree-add-containment-gate.sh`, four in `worktree-add-claim-gate.sh`, one in `worktree-create-gate.sh`. **`worktree-create-gate.sh` was partly clean already.** It carries the fixed `${BASH_SOURCE[0]%/*}` form (from #3788), and `hooks.json` registers it under `WorktreeCreate` at timeout 60, firing once per worktree creation. It is *not* on the `PreToolUse:Bash` path that produced the 418 timeouts, so none of those were its. Its two field reads were still costing 19 process creations, so they are fixed here, but the win is bookkeeping, not the timeout. **Review fix, second revision.** The first revision of this PR fed the whole payload to `jq` and `sed` by **here-string** (`{ V=$(jq …); } <<<"$INPUT"`), which is the exact form `lib/hook-utils.sh` forbids at `hook::json_complete` ("`printf | jq`, never `jq <<< "$1"`"). The reason is #1587 (`dfda6ec3`): bash fills a here-string's pipe itself, so a payload at or above the pipe capacity, **65536 bytes, traced hanging bash indefinitely on Git Bash**, blocks the shell before `jq` is ever exec'd. On these hooks that hang is the 15 s timeout, and on a containment gate that is a stall **and** a fail-open, the failure mode this campaign exists to remove. It does not reproduce on Linux bash 5.2 (the temp-file fallback engages at 65536 to 70000 bytes, verified in this session), which is how it slipped through. This revision adopts the library's own form and re-measures; the numbers below are the honest new ones, and several are worse than the first revision's. ## Fix In-file only, all mechanical: 1. **Both Bash gates' payload reads** (2 sites in containment, 3 in claim): `$(printf '%s' "$INPUT" | jq … 2>/dev/null | tr -d '\r')` becomes `$(printf '%s' "$INPUT" | jq … 2>/dev/null)` plus `V="${V//$'\r'/}"`. The feed is **kept as `printf '%s' "$INPUT" | jq`**, byte-for-byte the feed line inside `hook::jq_field`, the form the library prescribes for a hook payload; the jq program text is unchanged (`// empty`, no `gsub`), which is what keeps every differential below byte-identical to `main` on a non-string `.cwd`. Only the `tr` stage goes. `hook::jq_field` itself was measured too: it costs 4 creations to the inline form's 3 and adds a string-only `gsub` that changes the non-string-field behaviour, so it is not used. A here-string was measured at 1 creation and is not used either, for the reason above. 2. **`git_unlocated`**: `2>/dev/null` moves off the `git` command onto the enclosing subshell, so bash execs git in that subshell instead of forking for it. `unset` writes nothing to stderr, so the same stream is silenced and no other. 3. **`configured_root`**: `git config --get-all | tail -n 1 | tr -d '\r'` becomes one `git` plus `${r##*$'\n'}` and `${r//$'\r'/}`, on a single-command group (no payload involved). Multi-value last-wins verified A/B. 4. **Claim gate's stderr temp file**: `mktemp` + `2>"$err_file"` + `rm -f` where the file was **never read**. Replaced with `2>/dev/null` on a one-command group. This also removes the `|| continue` that silently skipped a claim whenever `TMPDIR` was unwritable, a fail-open now gone (`TMPDIR=/nonexistent` reproduced the lost claim on `main`). `claim_rc=$?` stays outside the group so it is still the helper's status. 5. **Create gate's `json_field`** becomes a `_to` form (the convention `hook-utils.sh` documents on its own `_to` helpers), dropping the two substitutions that wrapped each read; each rung is its own `printf '%s' "$payload" | jq` or `printf '%s' "$payload" | sed` pipeline. The jq program text is byte-for-byte `hook::jq_field`'s, `gsub` included, on purpose: `gsub` is string-only, so a numeric or object `.name` fails jq and falls through to the string-shaped `sed` rung, which yields nothing and refuses. 6. Nothing else. Every remaining `jq`, `git` and `sed` call is the same call with the same arguments. The two `<<<` left in the Bash gates are `IFS='/' read -r -a segs <<<"$rest"`, a builtin fed one path string, pre-existing on `main`; bash fills no pipe for a builtin, so that is not the hazard. ### Counts, per file, before → after Process creations (clone/clone3/fork/vfork) and `execve`, counted separately, re-measured after the review fix. No wall-clock figure: this Linux host is nothing like the campaign's contended Windows hosts, so a time here would be misleading. | Hook | Path | creations | `execve` | | --- | --- | --- | --- | | `worktree-add-containment-gate.sh` | a `worktree` command that is not an `add` (the hot path) | 8 → **7** | 3 → **2** | | | an `add` outside every repository (allow) | 22 → **18** | 7 → **5** | | | an `add` into a working tree (**block**) | 21 → **18** | 7 → **5** | | | an `add` into a `.git` directory (**block**) | 27 → **22** | 9 → **7** | | | `git -C <repo> worktree add sub/x` (**block**, names the root) | 26 → **20** | 10 → **6** | | | dynamic / post-`cd` / `echo git worktree add` (allow) | 13 → **11** | 5 → **3** | | `worktree-add-claim-gate.sh` | a `worktree` command that is not an `add` | 8 → **7** | 3 → **2** | | | a parsed `add` target | 28 → **22** | 11 → **6** | | `worktree-create-gate.sh` | payload with no `.name` (before the helper runs) | 19 → **13** | 6 → **4** | | | disabled by the kill switch | 0 → **0** | 0 → **0** | For the record, the first revision's here-string counts were 5 / 14 / 14 / 18 / 16 / 7 / 5 / 16 / 7 creations on those rows. The `printf | jq` form gives back exactly 2 creations per field read (3 where a here-string was 1), and no `execve`: the `execve` column is unchanged from the first revision. **`execve` dropping is not removed work.** Every drop is a named program replaced by a bash builtin or removed as dead code: | Program | Replacement | | --- | --- | | `tr -d '\r'` (×6 across the two Bash gates, 3 + 3; ×1 in the create gate; 7 removed) | `${v//$'\r'/}` | | `tail -n 1` (containment `configured_root`) | `${r##*$'\n'}` | | `head -n 1` (create gate fallback rung) | `${v%%$'\n'*}` | | `mktemp` + `rm` (claim gate) | removed; the temp file was written and deleted, never read | Four of the seven creations left on the hot path belong to `lib/hook-utils.sh` (the `hook::buffer_stdin` substitution and `hook::json_complete`'s `printf | jq -e .`), which is fenced off here. The gate's own share is the one `printf | jq` field read: 3 creations, 1 `execve`. ## Verification **Behaviour is the risk** on these gates: a swallowed non-zero status turns a block into a silent allow, which here means an unclaimed or out-of-tree worktree gets created. So every deny path and its near misses were A/B'd on **rc, full stdout and full stderr** against a pristine `git archive origin/main` copy of the plugin, re-run in full after the review fix: - **Containment, 52 probes**: 11 payloads × 4 root-resolution environments (nothing configured, plugin option, plugin data dir, `melodic.worktreeroot` git key) = 44, plus the kill switch, an empty stdin, a malformed payload, four non-string `.tool_input.command` values (number, object, `null`, `true`), and a non-string `.cwd`. Payloads: out-of-containment target, target inside a working tree, target inside a `.git` directory, `git -C`-composed relative target, valid external creation, `..`-escape out of the repo, dynamic `$HOME` target, post-`cd` target, `echo git worktree add`, an unrelated command, a CR-bearing path. **Byte-identical, all 52.** - **`configured_root` precedence** separately: git key beats plugin option beats data dir; multi-value `--get-all` last-wins; single value. Identical. - **Claim gate, real state**: a freshly-added unlocked worktree (claimed, correct `additionalContext`), a worktree already carrying **another session's** live claim (helper rc 4, not rewritten, reason string preserved verbatim), a target that was never created (nothing claimed), and a payload with no `session_id`. Identical, including the emitted JSON, modulo fixture path and timestamp. - **Create gate, 18 probes** including the disabled path, empty stdin, illegal branch name, missing root, non-repository cwd, unexpanded `${user_config}` placeholder, `name` 4242 / object / `null` with jq present, and a **jq-absent PATH** exercising the `sed` fallback rung with `name` 4242, `null` and a string. Identical apart from fixture paths and a fixture commit SHA. - **Existing suites**: `worktree-add-containment-gate.test.sh` (41 cases), `worktree-add-claim-gate.test.sh` (24), `worktree-create-gate.test.sh` (37). All pass. ### Permissive normalization The sibling shard's finding, that CR stripping can be the *permissive* direction, was checked here in both directions, with CR, BOM, zero-width space, U+2028, U+2029, NBSP, and trailing newline in the target path and in `.cwd`. **A/B identical on all 15 probes.** No batching was introduced, so neither of the two flip mechanisms is reachable: each field keeps its own `$(…)` and its own `// empty`, and the only string-only jq filter in the diff is the create gate's **pre-existing** `gsub("\r";"")`, kept byte-for-byte on purpose (see Fix 5). `name:4242`, `name:{"a":1}` and `name:null` are all still refused with the identical message, with jq present and absent. ### Pre-existing CR containment bypass (not fixed here; predates this work) Recorded precisely enough to act on without rediscovery, because a security finding should not live only in a PR body. **This PR does not fix it, and it is present identically on `origin/main`**; this lane does not file work items. - **Payload shape.** A Bash tool call whose command is `git worktree add ..<CR>/../outside/x` from a cwd inside a repository, where `<CR>` is the raw byte 0x0D immediately after a `..` segment. On the wire the harness JSON-escapes it, so the payload reads `"command":"git worktree add ..\r/../outside/x"`; that is the real `PreToolUse:Bash` payload path, nothing downstream re-parses the command, and the hook's tokenizer keeps the word whole. - **What the hook does.** `worktree-add-containment-gate.sh` strips every CR from the command before resolving, so it sees `../../outside/x`, resolves it to a path outside the repository, and returns rc 0 (allow). Plain `../outside/x` from the same cwd is blocked (rc 2, target `<repo>/outside/x`); `a<CR>b/../../outside/x` is also blocked, since a CR mid-segment leaves the nearest-existing-ancestor walk on the repository. The `..`-adjacent position is the one that flips. - **What git then does.** git does not strip the CR: `..<CR>` is an ordinary directory name, so `..<CR>/../outside/x` resolves to `<cwd>/outside/x`, and `git worktree add` creates it there, **inside the repository** (`<repo>/sub/outside/x`, rc 0, confirmed by `git worktree list`; a literal `..<CR>` directory is left in `<repo>/sub`). From the repository root, `..<CR>/../.git/wt` lands **inside `.git/`** (`<repo>/.git/wt`, created, rc 0). - **`worktree-add-claim-gate.sh`** (PostToolUse) returns rc 0 on the same command as well; it has no containment role, so nothing after the PreToolUse gate catches it. - **CR-only.** Quoting does not matter: unquoted, double-quoted and single-quoted CR words all allow in the hook and all create the directory in git. TAB is not in the class: an unquoted `..<TAB>/../outside/x` splits into two words for the shell and for the hook's tokenizer alike, and git then fails (rc 128, nothing created); a quoted `"..<TAB>/../outside/x"` stays one word, the hook does not strip TAB, resolves it lexically to `<repo>/sub/outside/x`, and **blocks**. Only CR is stripped before resolution, so only CR diverges from what git will do. - **Severity, the reviewer's read: low.** The gate is documented best-effort (the header declares fail-open on anything it cannot resolve statically) and already allows any `$VAR`-carrying target, so `git worktree add $PWD/x` reaches the same place with no CR at all. The CR shape grants nothing that `$PWD/x` does not. It is still a real fail-open of the class #3871 named, in shipped code, and the fix belongs with whoever owns the strip (strip CR only from the ends of the word, or resolve the un-stripped word and compare). ### New test `plugins/source-control/hooks/worktree-gates-spawn-budget.test.sh` (15 assertions) holds the counts as ceilings. It uses **strace, not xtrace and not a PATH shim**: both are blind to a fork that never execs. Upper bounds rather than equalities, so a later library change that removes more work does not fail it. **The ceilings cannot see a here-string regression**, because a here-string *lowers* the count; the suite's header says so, and three new cases grep each gate for `<<<` on `$INPUT` or `$payload` so that regression fails the suite anyway. It skips as a suite where `strace` is absent or cannot ptrace, rather than asserting on empty trace output. **Proven non-vacuous against seven mutants**, one per change, each reverted alone in a scratch copy and re-run after the review fix. A restored `tr` stage costs exactly +1 creation and +1 `execve` per field, which the exact ceilings catch: | Mutant | Suite | Cases it fails | | --- | --- | --- | | control (no mutation) | pass | none | | M1 containment `COMMAND` read, `tr` restored | fail | all 3 containment cases | | M2 containment `HOOK_CWD` read, `tr` restored | fail | 2 containment cases | | M3 containment `git_unlocated` | fail | 2 containment cases | | M4 containment `configured_root` | fail | the block case | | M5 claim's three field reads, `tr` restored | fail | both claim cases | | M6 claim `err_file` temp | fail | the parsed-target case | | M7 create `json_field_to` rungs (back to `hook::jq_field` + `sed \| head \| tr`) | fail | the create case | ### Gates | Gate | Result | | --- | --- | | `scripts/affected-tests.sh --run` | **exit 0**, all 4 selected suites pass; no unmapped file | | `check-changelog-parity.sh --check` | pass | | `check-changelog-parity.sh --check-order` | pass (91 changelogs, no duplicate heading) | | `check-changelog-parity.sh --check-bump origin/main` | pass | | `check-changelog-parity.sh --check-preserved origin/main` | pass (234 headings compared) | | `shellcheck -x` on all four scripts | clean | | `shfmt -d` (EditorConfig-driven) | clean | | `markdownlint-cli2` on README + CHANGELOG | 0 issues | | `check-killswitch-hoist.sh` | pass (31 hooks) | | `check-silent-skips.sh` | pass | | `check-discriminating-test-skips.sh` | pass | | `check-hook-exec-form.sh` | pass | | `check-fixture-git-isolation.sh` | pass (128 isolated) | ### Acceptance criteria, stated plainly | Criterion | Status | | --- | --- | | No more than 2 external spawns on the common path (own shell + at most one `jq`) | **Met for this hook's own share**: the gate execs exactly one `jq` of its own (3 creations, because the payload rides in on `printf \| jq` rather than a here-string, by library rule). **Not met literally**: a second `jq -e .` remains, and it belongs to `hook::json_complete` inside `lib/hook-utils.sh`, which is fenced off from this change (17 synced copies; unmerged #3740 and #3838 both target it). | | `grep`/`sed`/`cut`/`tr`/`basename`/`dirname` on the hot path replaced with builtins | **Met.** `tr`, `tail` and `head` are gone; `dirname` went in #3788. The one remaining `sed` is the create gate's jq-absent fallback rung, off the hot path, and is deliberately kept. | | Early exit before any spawn for non-matching invocations | **Already met on `main`** by the `if: Bash(*worktree*)` registration filter (#3621) plus each hook's own jq-free regex pre-filter. Unchanged here. | | Existing behavioural tests pass; the guard still blocks what it blocked | **Met.** 102 existing cases plus the A/B above. | | Under 2 s for a single run on a Windows host | **NOT VERIFIED.** No Windows host available. The budget doc's ceiling is stated as parallel wall time and this host's spawn cost is nothing like the campaign's, so a figure from here would mislead. Process creations are reported instead, which is the quantity that maps to the tax. | ### Reproduction ```bash strace -f -qq -e trace=clone,clone3,fork,vfork,execve -o t.txt \ bash plugins/source-control/hooks/worktree-add-containment-gate.sh <payload.json grep -cE '(clone3?|v?fork)\(' t.txt # creations grep -cE 'execve\(' t.txt # minus 1 for the traced program itself ``` ## Related - **#3508**: parent (Windows process-creation tax). Its stated cause, per-field `jq` forks needing a shared helper, is disproved again here: this PR touches `lib/hook-utils.sh` zero times. - **#1587** (`dfda6ec3`): the here-string deadlock trace this revision defers to; `lib/hook-utils.sh:1380` is the rule. - **#3779** (issue #3520): the precedent that first established the redirect-placement mechanism. - **#3788** (merged): fixed 34 scripts with zero `lib/hook-utils.sh` edits, and is where these three gates got their `${BASH_SOURCE[0]%/*}` form. - **#3871** (issue #3509): **sibling sharing this plugin's version chain**, and the source of the permissive-normalization class checked above. Its files (`pr-body-linkage-gate.sh`, `pr-linkage-mcp-gate.sh`, `pr-linkage-validator.sh`) are untouched here and its changelog entry is preserved verbatim. Its README hunk sits ~40 lines above this PR's, in the `pr-body-linkage-gate` section. - **#3838**: **overlaps this PR's files.** It edits all three worktree gates, replacing `INPUT=$(hook::buffer_stdin)` with the new `hook::buffer_stdin_to` form. The hunks do not overlap this PR's (its edits are on the `buffer_stdin` line; this PR's start below it), and a three-way merge was clean, but **the merge lane should expect a rebase rather than only a rebump**. The two changes are complementary: `buffer_stdin_to` removes one more creation from the same hot path, which is exactly why this PR's budget test asserts upper bounds rather than equalities. - **Version chain.** `main` is `0.55.58`; #3838 takes `0.55.59`; #3871 takes `0.55.59` and is being renumbered to `0.55.60`; **this PR takes `0.55.61`**, verified against `main` and every open `source-control` PR (#3774 and #3740 both claim versions at or below `main` and are stale, so they do not contend for 59 to 61). - **#3740**: unmerged `lib/hook-utils.sh` change; fenced off here, as is #3838's copy of it. - **#1403 / #1385**: prior art whose revival must first clear four contract-test regressions, two of which failed open. Not revived here. 🤖 Generated with [Claude Code](https://claude.com/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>
kyle-sexton
added a commit
that referenced
this pull request
Sep 7, 2026
…3870) Closes #3515 > [!IMPORTANT] > **Which of #3515's acceptance criteria this PR meets, and which it does not.** > > | Criterion | Status | > |---|---| > | No more than 2 external process spawns on the common path (own shell + at most one `jq`) | **Met for the common path**, which is every interactive Stop: the hook's shell plus one `uname -s`. The one spawn is `uname`, not `jq`; it is the managed-settings platform primitive the trust design rests on (`$OSTYPE` is a variable a repo env block can set) and was left alone on purpose. **Not met for an enabled lane's stop**: that path launches 5 programs (4 `jq`, 1 `uname`), down from 18. One of those `jq` is in the fenced shared library's `hook::buffer_stdin` (its payload validation pass); the other three read three different inputs (payload, settings file, block decision). | > | `grep`/`sed`/`cut`/`tr`/`basename`/`dirname` replaced with builtins on the hot path | **Met.** None launches on either traced path; the suite pins their absence. `cksum` remains on the marker-consumption path only (it keys the ledger, and changing the key would orphan existing ledger entries). | > | An early guard exits before any spawn for non-matching invocations | **Partially met.** The payload-free pre-filter exits before stdin and before `jq`, but it spawns `uname` to know which fixed managed-settings path to test. Removing it would mean testing all three platform paths, one of which is cwd-relative on Linux, so it stays. | > | Existing behavioural tests pass; the guard blocks what it blocked before | **Met**, with one disclosed divergence in the stricter direction (below). | > | Measured on a Windows host, a single run completes in under 2 s | **Not measured.** This runner is Linux; wall-clock is not the right proxy here and no figure is invented. The hook-budget convention's per-turn 500 ms bar (the corrected criterion from the issue comments) also still needs the Windows measurement. | > > `Closes #3515` is kept because the `pr-issue-linkage` gate wants a native closing keyword. A reviewer may prefer to reopen #3515 on merge for the Windows measurement and the enabled-lane count, or split those into a follow-up. ## Summary `hooks/lane-stop-gate.sh` is the autonomy plugin's `Stop` hook. It fires on every turn of every session, gated or not, and #3515 recorded it at a 27.6 s average with 73 timeouts against its 15 s budget on the #3508 hosts. It carried the largest in-file fork signature of the campaign: about 12 redirections and 11 pipes written inside command substitutions, plus a `$( )` capture around every lib helper that is nothing but parameter expansion. Following #3779's diagnosis rather than #3508's original framing: the cost was redirection placement and per-helper subshells, not per-field `jq` needing a shared helper. `lib/hook-utils.sh` and all 17 `plugins/*/hooks/hook-utils.sh` copies are untouched (`git diff origin/main --name-only` shows only the six autonomy files). The one shared-library facility used, `hook::jq_fields`, already existed on `main`. ## Fix All in `plugins/autonomy/hooks/lane-stop-gate.sh` and `lane-stop-gate-lib.sh`: | Site | Before | After | |---|---|---| | Lib path helpers (`gate_data_dir`, `gate_trusted_data_dir`, `gate_arm_record_path`, `gate_arm_claim_path`, `gate_user_settings_file`) | `v=$(gate_x)`: one fork each, for a function that only expands parameters | `gate_x_to <var>` forms via `printf -v`; print forms delegate, so `lane-stop-gate-arm.sh` and the lib-level tests read exactly what they read before | | Pre-filter settings scan | `grep -q lane_stop_gate "$f" 2>/dev/null` per file | builtin NUL-chunk `read` + substring test (`gate_file_mentions`), with `2>/dev/null` written **before** the input redirection so an existing-but-unreadable settings file stays as silent as `grep` was (bash applies redirections left to right; see case 51) | | Managed-files list | `done < <(gate_managed_settings_files)` with `$(uname -s 2>/dev/null)` inside: 3 creations per call, called up to 4 times per stop | `gate_managed_settings_files_load` fills an array in-process; `{ platform=$(uname -s); } 2>/dev/null` is 1 creation; loaded once per stop and reused by option resolution | | Payload fields (`EVENT`, `SESSION_ID`, `CWD`, `STOP_ACTIVE`, `LAST`) | five `printf \| jq \| tr` pipelines, 3-4 creations each | one `hook::jq_fields` pass (3 creations: process substitution, printf writer, jq); values chomped of trailing newlines so each reads byte-for-byte as the old `$( )` capture did | | Settings options (3 keys x managed files + user file) | one `$(jq … <file 2>/dev/null)` per key per file, each behind two more captures (8 creations per key) | `gate_settings_options_to <file> <key>...`: one jq per file for every key, NUL-separated through a process substitution with the redirections on the enclosing group (1 creation per file); resolved once, answered from memory (`gate_option_to`) | | Arm record | `jq -ec .` validation + `printf \| jq` per field + `printf \| jq` per option: 8 creations plus 6 on lookup | one jq pass over the record file yielding armed_at, session_id, sentinel, marker, compact json (1 creation); `EPOCHSECONDS` for the TTL clock with `date` as the pre-5.0 fallback | | Sentinel escape and match | `$(printf \| sed …)` then `grep -qE … <<<"$LAST"` | shell loop over the same fifteen metacharacters; `[[ $LAST =~ (^\|\n)[[:space:]]*TOKEN[[:space:]]*(\n\|$) ]]`, which agrees with grep's per-line verdict on every message (argument in the code comment) | | Telemetry data object | `$(jq -nc --arg … 2>/dev/null)` on every evaluated stop | assembled in the shell from the closed vocabulary: identical bytes | | Marker ledger | `$(printf \| cksum \| tr -cd '0-9')`, `$(stat … \|\| stat … \|\| printf '')`, `$(dirname …)` | `{ key=$(cksum); } < <(printf '%s' "$path")` with a parameter-expansion digit filter (same key as before, verified), per-rung `{ …; } 2>/dev/null` stat groups, `${ledger%/*}` | | Post-nudge branch read | `$(git … 2>/dev/null \| tr -d '\000-\037')` | `{ BRANCH=$(git …); } 2>/dev/null` + `${BRANCH//[[:cntrl:]]/}` (a superset strip; git refuses every such byte in a ref name, and `lane::notify` strips C0 again) | | `gate_resolve_plugin_name` (unanchored installs) | `$(jq … <"$manifest" 2>/dev/null)` | group-hoisted redirections | Left alone on purpose: `uname -s` as the platform primitive; `hook::buffer_stdin` (shared library; its `$( )` capture, read-slice probe and `printf | jq -e` validation are 4 of the enabled path's remaining 10 creations); the `gate_arm_owned` subshell (one fork, and the cleanest scope for `umask`/`noclobber`); the final `jq -nc` block decision. **Disclosed divergence (one degenerate config):** a configured sentinel that itself holds a newline. `grep -E` read that newline as a pattern separator and authorized a stop on any line matching either half of the token. The shell match treats the token as one pattern, so only the whole token standing alone authorizes, which is what the block reason instructs the agent to emit. No shipped launcher writes such a token. Pinned by new case 50 and named in the 0.22.30 changelog entry. Group redirections were checked for over-suppression: every `{ …; } 2>/dev/null` here wraps exactly one command, and the two process-substitution loops wrap a `read` builtin (no stderr) plus the one jq whose stderr the old code already suppressed. ## Verification **Before/after, measured here** (`strace -f -e trace=clone,clone3,fork,vfork,execve`, staged `plugins/cache/<m>/<n>/<v>/hooks` install, hook launched by the harness so its own shell is not in the count): | Path | Creations before | After | Launches (`execve`) before | After | |---|---|---|---|---| | Default: no gate footprint anywhere (every interactive `Stop`) | 4 | **1** | 2 (`grep`, `uname`) | **1** (`uname`) | | Enabled by user settings, first stop, no signal (block) | 48 | **10** | 18 | **5** (`jq`, `jq`, `uname`, `jq`, `jq`) | | Enabled, sentinel on its own line (allow) | 44 | 9 | 16 | 4 | | Enabled, second stop after the nudge (allow, notify muted) | 54 | 10 | 21 | 5 | | Enabled, marker present (allow, consume) | 51 | 12 | 19 | 6 | | Env-only enable claim (once-per-session notice) | 31 | 22 | 11 | 8 (remainder is `hook::notice_once` in the shared library) | | Armed by the launcher, first stop (block) | 60 | 11 | 20 | 5 | Launches did not stay flat, and that is expected here: unlike #3779, this issue explicitly asks for the `grep`/`sed`/`tr`/`dirname` helpers to become builtins, so those launches are gone; every `jq` that reads a distinct input is still launched, and the payload/settings/arm-record `jq` calls that were batched read the same inputs once instead of several times. **Wall-clock is not measured and no figure is invented.** A spawn is about 1 ms on this Linux runner; #3508 measures 180 to 2,841 ms per spawn (median 1,108 ms at 501 concurrent processes) on the affected Windows hosts. Process creations by trace are the drift-immune proxy #3508's corrected criteria ask for. The plugin README now carries the table above under a "Hook cost" heading, per hook-budget Rule 1, and says the Windows wall-clock share still needs measuring. **Proof the verdicts did not change.** - All 89 pre-existing assertions in `lane-stop-gate.test.sh` pass unmodified; the suite is now 103 with the additions below (102 where `chmod 000` denies nothing and case 51 skips visibly). - A black-box differential harness (scratch, not committed) ran the `origin/main` hook and this branch's hook over 95 scenarios from identical fresh staged installs, comparing rc, stdout, and marker/ledger side effects: every settings shape the reader distinguishes (boolean/string/number/null values, `options` as string/array, `pluginConfigs` as array/string, null entry, other marketplace, malformed, two documents), sentinel edge cases (CRLF, tabs, blank lines, inline mention, substring, trailing text, 120 KB messages with early and no sentinel, invalid UTF-8 around the token, metacharacter sentinels, empty sentinel), marker absolute/relative/consumed/stale-ledger/undeletable, env-only claims, and the arm record's every parse and claim path (null/false/number/string/empty/malformed, missing or non-numeric or float `armed_at`, expired, legacy `session_id` match/mismatch/numeric, taken/empty/directory claim files, missing session id). **93 identical; the 2 differences are the two halves of the disclosed newline-sentinel case.** - The ledger key derivation and the sentinel escape were also compared directly against the old pipelines on sample inputs: identical. **New regression tests, verified non-vacuous.** - Case 49: `gate_settings_options_to` answers three keys from one pass with the single-key verdicts (including the trailing-newline chomp and the all-or-nothing `options`-not-an-object case). - Case 50: a newline-bearing sentinel authorizes only as a whole block (the disclosed divergence, pinned). - Case 51: an existing-but-unreadable settings file produces **no stderr** and does not change the verdict. Pins the redirection order in `gate_file_mentions` (commit `fd0e7578`): the first draft wrote `done <"$1" 2>/dev/null`, which attempts the open before stderr is silenced and prints `Permission denied` per turn where `grep -q … 2>/dev/null` was silent. Run as an unprivileged user the case passes on the fixed tree and fails with that exact line on the old ordering; where `chmod 000` denies nothing (root, or a filesystem without POSIX modes) it skips visibly rather than passing vacuously. - Trace budget: exactly 1 creation and 1 launch (`uname`) on the default path; ceilings of 10 creations and 5 launches on the enabled block path (a ceiling so a shared-library saving in #3740/#3838 lowers the count without failing here, while a regression in this plugin's files raises it and does); and no `dirname`, `tr`, `sed`, `grep`, `cksum` or `date` on either path. Skips where `strace` is unavailable; the CI Linux lane does not skip. Re-measured rather than assumed after the `fd0e7578` redirection swap: both pins hold, and the enabled path's four `jq` are still `jq -e .` (`hook::buffer_stdin`), the `jq -j` payload pass, the `jq -j` settings pass and the `jq -nc` block decision, in that order with `uname` third. - Mutation 1 (move the `uname` redirection back inside its substitution): `FAIL: default path creates 2 processes, budget is 1` and `FAIL: enabled block path creates 11 processes, ceiling is 10`. Mutation 2 (put the `sed` escape pipeline back): 13 creations, 6 launches, and the named-helper check all fail. Both on scratch copies; the committed tree passes. **Commands run in the foreground on the pushed head `fd0e7578`, with actual results** (the `lane-notify` / lane-launcher / `check-shell-portability` lines are from the `1ca98cbf` run; the second commit touches neither those files nor their inputs, and the full `affected-tests.sh --run` below was re-run on `fd0e7578`): ``` bash plugins/autonomy/hooks/lane-stop-gate.test.sh -> PASS=103 FAIL=0 (unprivileged) -> PASS=102 FAIL=0 as root, case 51 skips visibly bash plugins/autonomy/hooks/lane-notify.test.sh -> rc=0 bash plugins/claude-ops/skills/lanes/scripts/lane-launcher.test.sh -> rc=0 (calls lane-stop-gate-arm.sh) bash plugins/claude-ops/skills/lanes/scripts/restart-consumer.test.sh -> rc=0 bash scripts/affected-tests.sh --explain -> the R4 transitive walk from lane-stop-gate.sh fans out to the whole shell corpus (170 suites); plugin.json, CHANGELOG.md, README.md recorded as no-suite (non-shell lanes) bash scripts/affected-tests.sh --run (foreground) -> rc=1: one failing suite, plugins/claude-ops/skills/plugins/scripts/cache-content-check.test.sh ("process budget: the trace probe actually counted something ... measured -1, the pid-stamped PS4 did not reach the traced shell"). Reproduced identically on a pristine `git archive origin/main` export: pre-existing and environmental, not from this change. Every other selected shell suite passed. shellcheck <3 changed .sh files> -> clean shfmt -d <3 changed .sh files> -> clean bash scripts/check-shell-portability.sh origin/main -> No unexcused GNU-only constructs in 3 shell file(s). node_modules/.bin/markdownlint-cli2 README.md CHANGELOG.md -> 0 issues typos <5 changed files> -> clean scripts/check-hook-exec-form.sh, check-killswitch-hoist.sh, check-silent-skips.sh, check-discriminating-test-skips.sh, check-hook-wiring-liveness.sh, check-purged-em-dashes.sh -> all rc=0 bash scripts/check-changelog-parity.sh --check -> rc=0 bash scripts/check-changelog-parity.sh --check-order -> rc=0 bash scripts/check-changelog-parity.sh --check-bump origin/main -> rc=0 bash scripts/check-changelog-parity.sh --check-preserved origin/main -> rc=0 (78 headings compared) ``` Manifest bumped 0.22.29 to 0.22.30 with a matching changelog entry. Not mine and pre-existing: `test_save_point.py::test_new_origin_falls_back_to_directory_name` (Python, in the NOT RUN ecosystem list) and the `cache-content-check.test.sh` trace-probe failure above. ## Related - Parent campaign: #3508 (Windows process-creation tax). Its corrected criteria ask for spawn counts before and after by trace, and the hook-budget bar; both are reported above. - Precedent for the in-file approach: #3779 (context-guard shard #3520), which located the cost in redirection placement rather than per-field `jq`, and merged PR #3788, which applied it across 34 hooks without touching `lib/hook-utils.sh`. - #3740 and #3838 own `lib/hook-utils.sh` and its 17 synced copies; this PR does not touch them. The enabled path's remaining shared-library share (4 of 10 creations) is theirs; the trace test's ceiling leaves it room. - Prior art #1403 / #1385 (`hook::jq_fields`, `strip_quoted_spans`, the 65-spawn census). `hook::jq_fields` is used here as it already exists on `main`; nothing from #1385 is revived. - Budget authority: `docs/conventions/hook-budget/README.md`, surfaced by `.claude/rules/hook-budget.md`. 🤖 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>
cursor Bot
pushed a commit
that referenced
this pull request
Sep 7, 2026
… off substitutions The Stop-event unsurfaced-failure detector timed out 113 times in the #3508 window at a 21.9 s average, roughly 44x the 500 ms the hook-budget convention gives the whole always-on per-turn set. The parent issue blames per-field jq forks needing a shared helper; that diagnosis is wrong here, as shard #3520 and PR #3788 established twice. The cost is redirection placement. Bash runs the command of a command substitution in the substitution's own subshell and skips the extra fork ONLY when that command carries no redirection of its own. `$(wc -c <file 2>/dev/null)` is two processes for one byte count; `$(cat -- file 2>/dev/null)` is two for a file read; `{ V=$(cmd); } 2>/dev/null` is one. Those forks are invisible to `bash -x`, which reads command positions, which is why the campaign kept mis-attributing the cost to the exec count. Six sites in hook-failure-audit.sh change and no shared library does: - `wc` names the transcript instead of redirecting it in, and `read` drops the filename column along with the padding some wc builds add. - The `read_window` helper is gone. Under the tail cap grep opens the transcript itself rather than being fed by `cat` through a function call that was a second subshell on top of the substitution's own. - The marker read is `$(<file)`, which forks nothing and execs nothing. - The marker write strips carriage returns in the shell instead of piping through `tr`. - The marker directory is probed with `-d` before `mkdir -p` spawns. - The two payload fields come from one `hook::jq_fields` pass instead of two `hook::jq_field` calls. Over the tail cap the `tail | sed | grep` pipeline and its redirect placement are untouched: a pipeline element forks either way, and hoisting the redirect onto a group would newly silence sed and grep for no saving. The `printf | jq` feeding the structural selection also stays a pipeline, because a here-string at or above the pipe capacity deadlocks before jq is exec'd and that payload routinely clears it; it is off the common path regardless. Common path (a turn with no failure recorded, under the cap): 18 process creations and 6 execs before, 9 and 4 after. Over the cap: 19 and 7 before, 12 and 6 after. A turn that warns: 34 and 17 before, 24 and 14 after. Measured with `strace -ff -e trace=clone,clone3,fork,vfork,execve`. No wall-clock figure is claimed: this Linux host says nothing about the Windows spawn tax the budget binds to, and on the #3508 host one creation costs 180-2,841 ms. Behaviour is unchanged, which for this hook is the whole point: it exists to surface failures that otherwise pass unnoticed, so a hoisted `2>/dev/null` could silence exactly the diagnostic it is for. Every group holds one command, and the one stream now silenced that was not before is grep's, which `cat`'s own redirect already discarded. Pre- and post-change stdout, stderr, exit codes and marker contents were compared byte for byte across ten scenarios: first warn, dedup, a new failing registration re-warning while the warned one stays muted, all three classification branches, the tail cap, a clean transcript, a missing transcript, the kill switch, and no data directory. Identical. The contract test gains an strace budget assertion on both counts, mutation- checked: moving either silenced redirect back inside its substitution adds a fork with no new exec, leaves every behavioural assertion green, and trips the creation ceiling. The README states the measured share per hook-budget Rule 1, as a process count, and says plainly that the Windows parallel-wall figure is still owed. Two acceptance criteria are not met and are called out in the PR: the hook cannot exit before any spawn, since reading the transcript is how it learns there is nothing to report, and the 500 ms parallel-wall figure for the three-plugin per-turn set is cross-plugin and Windows-only. Refs #3508. Precedent #3779, #3788. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ViPsHkL3ng9xWt2GjEQJob
kyle-sexton
pushed a commit
that referenced
this pull request
Sep 7, 2026
…3513) (#3869) Closes #3513 ## Summary `block-hook-bypass.sh` was the largest process creator of any guard in the plugin on a benign Bash call: 7 creations, 0 execs. Every exec-based census (the PATH-shim spawn census, `run-guards.test.sh`'s dirname/sed pin, an xtrace command-position count) reads a fork-without-exec as free, which is why the issue's numbers and the "per-field jq" cause never matched what the kernel does. Six of the seven were in this file; the seventh is `$(hook::buffer_stdin)`. Measured with `strace -f -e trace=clone,clone3,fork,vfork,execve` on the real dispatched path (`run-guards.sh block-hook-bypass.sh`), `HOOK_TELEMETRY_SINK` unset, guard share = count minus a no-op guard dispatched the same way, three identical repeats: | Path | creations before -> after | execve before -> after | |---|---|---| | Guard share, benign `git status --short` | 7 -> 1 | 0 -> 0 | | Guard share, blocked `echo hi > notes.md` | 10 -> 5 | 1 -> 1 | | Whole Bash dispatcher, benign | 36 -> 30 | 3 -> 3 | | Guard alone under the dispatcher, wall p50 / p95 (n=20, interleaved) | 27.4 / 29.2 ms -> 24.3 / 27.5 ms | | | Whole Bash dispatcher, wall p50 / p95 (n=20, interleaved) | 51.6 / 60.4 ms -> 48.2 / 49.6 ms | | The execve column does not move: this is latency, not removed work. **On the issue's "at most two spawns" line.** The one creation left in the guard's own share is `$(hook::buffer_stdin)`. Its fork-free form (`hook::buffer_stdin_to <var>`) belongs to `lib/hook-utils.sh`, which is fenced by unmerged #3740 and #3838, so it is not touched here. Whether #3513 counts as met depends on what one counts: the guard's in-file contribution is now 1 creation and 0 execs, and the dispatcher's own `$(source …)` isolation fork is the dispatcher's, not this file's. `Closes` here means the in-file work is done; the remaining fork is the hook-utils item. ## Fix Five in-file creations removed, one left in place: 1. `:159` eager `SUBJECT=$(hook::extract_bash_subject …)` at file scope. Only `emit_tel` read it, and `emit_tel` is gated on the start stamp and the opt-in sink. The subject is now derived inside `emit_tel` behind both gates. Verdicts never read it. 2. `:478` `EXECUTABLE=$(strip_literals "$COMMAND")`. Now `strip_literals_to EXECUTABLE "$COMMAND"`, assigning through a nameref; every trailing newline is stripped, as `$(…)` stripped them. 3. `:474`, `:1305`, `:1330`, `:1403` the four `done < <(printf '%s\n' …)` loops. One fork-free splitter, `split_lines_to`, does a sentinel-prefixed `IFS=$'\n'` word split under `set -f` (globbing restored to whatever the caller had). The sentinel prefix is what keeps blank lines and runs of newlines from collapsing, and a leading or trailing empty line from disappearing, so the array is exactly what `read` delivered, which `strip_literals` needs to keep its heredoc and open-quote state aligned with physical lines. `NORMALIZED_SEGMENTS` becomes an array filled once in `normalize_segments`; the three per-segment scans iterate it. `return` and `continue 2` inside those loops reach the same scopes as before because neither loop shape runs its body in a subshell. A here-string was not an option: at 65536-65663 bytes bash blocks forever writing it into the pipe (documented in `lib/path-detection/hardcoded-path-patterns.sh`). 4. `:112` `INPUT=$(hook::buffer_stdin)` is left as is. It is the fenced hook-utils item, and #3740 modifies this exact rc-handling block; the diff stays off it so the two do not fight at merge. Manifest 0.32.10 -> **0.32.12**, CHANGELOG entry, README "Hook budget accounting" entry with the method and table above. 0.32.11 is taken by #3849 (issue #3511), which bumps the same manifest off the same 0.32.10 base; this branch takes the next free number so the two do not collide on `plugins/guardrails/CHANGELOG.md`. Two documentation corrections carried in the same renumber commit: - The CHANGELOG entry and the new README row said "the 602-case contract suite passes". With the pins this change set adds the suite is 611 cases, which is what it reports. - The comment moved onto `emit_tel` carried two em dashes over from the file-scope comment it replaced. `.claude/rules/vendor-docs-are-not-style.md` bars them from this repo's instruction surfaces, and `check-purged-em-dashes.sh` scans markdown only, so nothing caught them. They are parentheses now. No other comment is touched, and no guard logic changed. ## Verification - **Deny paths still deny, compared against `origin/main`.** 244 paired runs (61 commands x Bash/PowerShell payloads x standalone/dispatched, plus a 70 KiB single-line command and a 3000-line command on each side) agree on exit code and first stderr line. The corpus covers every documented deny form (cat/echo/printf redirects, no-space forms, heredoc openers with a trailing redirect, `2>&1`/`>&2` before the file, prefix modifiers `command`/`exec --`/`FOO=bar`, group and `if` headers, leading redirects, discard-then-real-file `> /dev/null > real.txt`, python write indicators, staged `mv`/`cp`, quoted operands with embedded separators, `\;` and backslash-newline escapes, `ec""ho` / `ec"xy"ho` splices) and the allow forms (dup-only redirects, producer-scoped `bash x.sh > out && echo done`, quoted prose in commit and PR bodies, `command -v`, comments, blank-line-only and newline-padded commands, glob characters). - `split_lines_to` checked directly against `while IFS= read -r` over `printf '%s\n'` on: empty, single line, trailing newline, leading newline, runs of blank lines, whitespace-only lines, glob characters, the sentinel byte itself at line starts and ends, IFS characters, trailing backslashes, UTF-8, a 70 KiB line, 3001 lines. Identical arrays in every case; `set -f` preserved when the caller had it and restored off when it did not. - **New strace-based budget test** in `block-hook-bypass.test.sh`: pins the guard's benign share at exactly 1 creation and 0 execve on two benign payloads (single segment; multi-line, multi-segment), and that `echo > file` still exits 2 under the tracer. Skips visibly on a host without a working strace. It runs the real dispatched path, not the standalone guard, and subtracts a no-op guard so dispatcher forks do not leak into the pin. **Mutation-checked**: unmutated control 9/9; each of the four changes reverted alone (eager SUBJECT restored; splitter through `mapfile < <(…)`; strip through `$(…)`; segments through `mapfile < <(…)`) fails the pin on both payloads. - **The census was re-taken after the renumber commit**, since a documentation-only change must not move it, and it does not: whole Bash dispatcher on the benign payload reads 36 creations at `origin/main` against 30 on this branch, 3/3 identical repeats, execve flat at 3 on both; the blocked-path guard share is 5 creations and 1 execve; the contract suite's exact benign-share pin (1 creation, 0 execve) passes. - `scripts/affected-tests.sh --run`: 5 suites selected (`block-hook-bypass.test.sh` 611/611, `flag-commit-pr-skill-bypass.test.sh`, `require-jq-posture.test.sh`, `run-guards.test.sh`, `scripts/check-shell-portability.test.sh`), all passed, exit 0; the manifest, CHANGELOG and README map to the recorded no-suite allowlist. - `scripts/check-changelog-parity.sh` **all four modes** the script defines, on the committed tree: `--check` (pass), `--check-order` (pass, 91 changelogs read newest-first with no duplicate versions), `--check-bump origin/main` (pass), `--check-preserved origin/main` (pass, 155 headings preserved). `--check-order` is the mode that catches a doubled `## [<version>]` after a bad conflict resolution, which is the failure the renumber exists to avoid; an earlier revision of this PR ran only the other three. - `shellcheck -x` clean on the guard and the test; `shfmt -d` no diff; markdownlint-cli2 0 issues; editorconfig-checker clean; `check-purged-em-dashes.sh` clean; ai-slop detector 0 findings on the README and CHANGELOG. - Pre-existing and not touched: `test_save_point.py::test_new_origin_falls_back_to_directory_name`. ## Related - #3508 parent campaign. Its stated cause (per-field `jq` forks needing a shared helper) does not describe this guard: since #3788 the dispatcher primes the fields once, and this guard contributed 0 execs before this change. - #3779 precedent (#3520): an xtrace command-position count read 2 where the kernel made 8; the same instrument gap is why the shim-based budget test could not see any of these seven. - #3740 adjacent: modifies this script's `buffer_stdin` rc handling. This diff stays off that block. #3838 is the other fence on `lib/hook-utils.sh`. - #3849 (issue #3511) holds guardrails 0.32.11; this PR now takes 0.32.12, so the two no longer collide. - Credit note for the record: this file's `dirname` exec died in `a5f5e799` (#3781); #3788 never touched this file. 🤖 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>
cursor Bot
pushed a commit
that referenced
this pull request
Sep 7, 2026
… off substitutions The Stop-event unsurfaced-failure detector timed out 113 times in the #3508 window at a 21.9 s average, roughly 44x the 500 ms the hook-budget convention gives the whole always-on per-turn set. The parent issue blames per-field jq forks needing a shared helper; that diagnosis is wrong here, as shard #3520 and PR #3788 established twice. The cost is redirection placement. Bash runs the command of a command substitution in the substitution's own subshell and skips the extra fork ONLY when that command carries no redirection of its own. `$(wc -c <file 2>/dev/null)` is two processes for one byte count; `$(cat -- file 2>/dev/null)` is two for a file read; `{ V=$(cmd); } 2>/dev/null` is one. Those forks are invisible to `bash -x`, which reads command positions, which is why the campaign kept mis-attributing the cost to the exec count. Six sites in hook-failure-audit.sh change and no shared library does: - `wc` names the transcript instead of redirecting it in, and `read` drops the filename column along with the padding some wc builds add. - The `read_window` helper is gone. Under the tail cap grep opens the transcript itself rather than being fed by `cat` through a function call that was a second subshell on top of the substitution's own. - The marker read is `$(<file)`, which forks nothing and execs nothing. - The marker write strips carriage returns in the shell instead of piping through `tr`. - The marker directory is probed with `-d` before `mkdir -p` spawns. - The two payload fields come from one `hook::jq_fields` pass instead of two `hook::jq_field` calls. Over the tail cap the `tail | sed | grep` pipeline and its redirect placement are untouched: a pipeline element forks either way, and hoisting the redirect onto a group would newly silence sed and grep for no saving. The `printf | jq` feeding the structural selection also stays a pipeline, because a here-string at or above the pipe capacity deadlocks before jq is exec'd and that payload routinely clears it; it is off the common path regardless. Common path (a turn with no failure recorded, under the cap): 18 process creations and 6 execs before, 9 and 4 after. Over the cap: 19 and 7 before, 12 and 6 after. A turn that warns: 34 and 17 before, 24 and 14 after. Measured with `strace -ff -e trace=clone,clone3,fork,vfork,execve`. No wall-clock figure is claimed: this Linux host says nothing about the Windows spawn tax the budget binds to, and on the #3508 host one creation costs 180-2,841 ms. Behaviour is unchanged, which for this hook is the whole point: it exists to surface failures that otherwise pass unnoticed, so a hoisted `2>/dev/null` could silence exactly the diagnostic it is for. Every group holds one command, and the one stream now silenced that was not before is grep's, which `cat`'s own redirect already discarded. Pre- and post-change stdout, stderr, exit codes and marker contents were compared byte for byte across ten scenarios: first warn, dedup, a new failing registration re-warning while the warned one stays muted, all three classification branches, the tail cap, a clean transcript, a missing transcript, the kill switch, and no data directory. Identical. The contract test gains an strace budget assertion on both counts, mutation- checked: moving either silenced redirect back inside its substitution adds a fork with no new exec, leaves every behavioural assertion green, and trips the creation ceiling. The README states the measured share per hook-budget Rule 1, as a process count, and says plainly that the Windows parallel-wall figure is still owed. Two acceptance criteria are not met and are called out in the PR: the hook cannot exit before any spawn, since reading the transcript is how it learns there is nothing to report, and the 500 ms parallel-wall figure for the three-plugin per-turn set is cross-plugin and Windows-only. Refs #3508. Precedent #3779, #3788. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ViPsHkL3ng9xWt2GjEQJob
kyle-sexton
added a commit
that referenced
this pull request
Sep 7, 2026
…stitutions (#3849) Closes #3511 ## Summary `plugins/guardrails/hooks/block-convention-violation.sh` carried three `2>/dev/null` redirects **inside** command substitutions. GNU Bash execs the body of a command substitution in the substitution's own subshell, instead of forking a second time, only when that body carries no redirection of its own (Command Substitution, Bash Reference Manual; https://mywiki.wooledge.org/CommandSubstitution). So `v=$(cmd 2>/dev/null)` costs two process creations to run one program where `{ v=$(cmd); } 2>/dev/null` costs one. On the Windows Git Bash hosts #3508 measures, a fork is a full process creation at 0.3-0.9 s. **The parent's stated cause does not apply to this file.** #3508 attributes the cost to per-field `jq` forks needing a shared helper. This guard already batches all three payload fields through `hook::jq_fields`, already derives its own directory with `${BASH_SOURCE[0]%/*}` instead of `$(dirname …)`, and since #3788 is `source`d in-process by `run-guards.sh`, which buffers stdin and primes the jq fields once for the whole batch. What was left is redirection placement, the same cause shard #3520 established in PR #3779 and PR #3788 fixed across 34 scripts without touching `lib/hook-utils.sh`. This PR touches no shared library and no dispatcher: the change is in-file. ## Fix Three redirects hoisted onto single-command groups: | Site | Reached on | |---|---| | `bash "$RESOLVER" …` convention resolver (two calls) | first commit or `gh pr create` after a cache miss | | `git rev-parse --absolute-git-dir` sequencer probe | every stdin-form commit | | `git config --get alias.<sub>` alias probe | every non-builtin git subcommand, so this one is on the per-tool-call path | Each group holds **exactly one command**, so the group's status is still that command's. That matters here specifically because this is a *gate*: hoisting `2>/dev/null` over a multi-command group can swallow a non-zero status and turn a block into a silent allow. `|| conv_val=""` and `|| return 1` fire on exactly the failures they fired on before. ## Verification **Process counts, from the kernel** (`strace -f -e trace=clone,clone3,fork,vfork,execve`, `HOOK_TELEMETRY_SINK` unset). A PATH shim cannot see a fork that never execs, and `bash -x` prints one line whether a command costs one process or two, so neither existing counter in this repo can measure this. Guard invoked directly: | Scenario | creations before | after | execve before | after | |---|---|---|---|---| | `echo hello` | 11 | 11 | 3 | 3 | | `git status --short` | 11 | 11 | 3 | 3 | | `git wibble --x` (alias probe) | 14 | **13** | 4 | 4 | | stdin-form commit, cold cache | 34 | **31** | 16 | 16 | | stdin-form commit, warm cache | 19 | **18** | 5 | 5 | Guard's incremental cost inside `run-guards.sh`, the way it actually runs (full Bash matcher guard list, measured with and without this guard in the list, warm convention cache): | Scenario | guard's added creations before | after | guard's added execve | |---|---|---|---| | `git status --short` | 3 | 3 | 0 | | `git wibble --x` | 6 | **5** | 1 | | stdin-form commit | 11 | **10** | 2 | `execve` is unchanged everywhere, which is the evidence this removes latency rather than removing work. Wall-clock is not the claim: this Linux host's spawn floor is nothing like the contended Windows host in #3508, and inventing a millisecond figure from it would mislead. **Deny paths still deny.** All three touched sites were exercised against a fixture repo carrying a tracked `subject_pattern`: - violating commit subject blocked (exit 2), conforming allowed (exit 0) — proves the resolver site still returns a pattern - violating `gh pr create --title` blocked (exit 2) - `git qc` where `alias.qc = commit` blocked (exit 2) — proves the alias-probe site still resolves the alias, which fails **open** if it breaks - commit during an in-progress merge (`MERGE_HEAD` present) allowed (exit 0) — proves the sequencer-probe site still detects the sequencer, which is the exemption the probe exists for **New test, mutation-checked.** `block-convention-violation.test.sh` gains a per-site kernel-trace budget assertion: for each external command this file starts, the process that execs it must have a parent that itself execve'd. A parent that never execs and has exactly one child is the wasted fork. Each site must appear in the trace, so a scenario that stops reaching a site fails rather than passing by absence. Moving each redirect back inside its substitution, one at a time, failed exactly its own assertion and no other (3 runs, `PASS=80 FAIL=1` each). The block skips loudly, printing `skip: strace is unavailable…`, where `strace` is absent or not permitted, so it is a Linux-CI guard and not a Windows one. **Gates.** `scripts/affected-tests.sh --run` exit 0, all 5 selected suites pass (`block-convention-violation.test.sh` PASS=81 FAIL=0, `run-guards.test.sh` PASS=101, `require-jq-posture.test.sh`, `lib/resolve-convention-pattern.test.sh`, `commit-msg-convention.test.sh` PASS=15). `shellcheck -x` clean, `shfmt -d` clean, `check-shell-portability.sh --paths` clean, `check-killswitch-hoist.sh` clean, `check-changelog-parity.sh` clean in all four modes (`--check`, `--check-order`, `--check-bump origin/main`, `--check-preserved origin/main`). Manifest 0.32.10 → 0.32.11 with the matching CHANGELOG entry. The prettier warning on `plugins/guardrails/CHANGELOG.md` is pre-existing at `origin/main` and untouched. ### Acceptance criteria not fully met — stated plainly | Criterion from #3511 | Status | |---|---| | Subprocess `grep`/`sed`/`cut`/`tr`/`basename`/`dirname` on the hot path replaced with builtins | **Already true before this PR.** None of them appear on any path of this file. | | Early guard clause exits before any spawn for non-matching invocations | **Already true before this PR** (#3820 deferred the convention load). `git status --short` and `echo hello` add **zero** execs from this guard. | | Existing behavioural tests pass; the guard blocks what it blocked before | **Met.** | | No more than 2 external process spawns on its common path (own shell plus at most one `jq`) | **Not met as literally written, and not achievable in-file.** Run standalone the guard costs 3 execs on the common path: 2 `jq` and the telemetry sink, all inside `lib/hook-utils.sh`. In the batch it actually runs in, it adds **0** execs to the common path and 3 pure forks, and those forks are the dispatcher's per-guard `$(source …)` isolation fork plus `hook-utils` internals. Both files are out of scope here: `hook-utils.sh` is synced across 17 plugin copies with unmerged work in flight (#3740, #3838), and the isolation fork is #3685. | | A single run completes in under 2 s on a Windows host | **Unverified.** No Windows host in this session. The comment thread on #3511 also supersedes this figure with the repo's own budget (`docs/conventions/hook-budget/README.md`: <= 1 s typical / <= 2 s worst case per tool call, parallel wall). | This PR does not on its own bring the guard under the hook budget on the host in #3508. It removes the cost this file owns and can fix without touching a fenced shared library. ## Related - Parent: #3508 (Windows process-creation tax), whose stated cause is corrected above - Precedent for the diagnosis: #3520, PR #3779 (found redirection placement is the real cost), PR #3788 (fixed 34 scripts across 17 plugins, `lib/hook-utils.sh` untouched) - Prior work on this file: #3820 (deferred the convention load off the benign path) - Out of scope and named above: #3685 (per-guard isolation fork), #3740 and #3838 (unmerged `hook-utils.sh` work) - Siblings sharing this plugin's manifest and CHANGELOG: #3521, #3529 🤖 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>
kyle-sexton
added a commit
that referenced
this pull request
Sep 7, 2026
… off substitutions (#3851) Closes #3512 ## Summary The Stop-event unsurfaced-failure detector (`plugins/claude-ops/hooks/hook-failure-audit.sh`) timed out 113 times in the #3508 window at a 21.9 s average, roughly 44x the 500 ms the hook-budget convention gives the whole always-on per-turn set. **The parent issue's stated cause is wrong for this hook.** #3508 blames per-field `jq` forks needing a new shared helper. Shard #3520 (PR #3779) established that the real cost is *redirection placement*, and merged PR #3788 fixed 34 hook scripts across 17 plugins while touching `lib/hook-utils.sh` zero times. This shard applies the same diagnosis. **No shared library is touched** — the fence around `lib/hook-utils.sh` and its 17 synced copies (unmerged #3740, #3838) holds. The mechanism: bash runs the command of a command substitution in the substitution's own subshell and skips the extra fork **only when that command carries no redirection of its own**. `$(wc -c <file 2>/dev/null)` is two processes for one byte count; `$(cat -- file 2>/dev/null)` is two for a file read; `{ V=$(cmd); } 2>/dev/null` is one; `$(<file)` is zero. Those forks are invisible to `bash -x`, which reads command positions — which is why the campaign kept mis-attributing the cost to the exec count. ## Fix Six in-file sites, all in `hook-failure-audit.sh`: | Site | Before | After | |---|---|---| | Transcript size | `SIZE=$(wc -c <"$TRANSCRIPT" 2>/dev/null)` | `wc` names the file, `read` drops the filename column and any padding, `2>/dev/null` rides a single-command group | | Pre-filter, under the cap | `read_window \| grep -F …`, where `read_window` was `cat -- file 2>/dev/null` | `{ RECORDS=$(grep -F … -- "$TRANSCRIPT"); } 2>/dev/null` — grep opens the transcript itself; the helper function was a second subshell on top of the substitution's own | | Marker read | `$(cat -- "$MARKER" 2>/dev/null)` | `$(<"$MARKER")` — no subshell, no exec | | Marker write | `jq … 2>/dev/null \| tr -d '\r' >>"$MARKER"` | `$(jq …)` then in-shell `${VAR//$'\r'/}` with a builtin `printf` append | | Marker directory | `mkdir -p "$MARKER_DIR" 2>/dev/null` every warned turn | `[[ -d … ]] ||` first; `mkdir -p` on an existing directory was a whole process to reach the same no-op | | Payload fields | two `hook::jq_field` calls | one `hook::jq_fields` call (the batching helper the library already ships, used by 10 other hooks) | The CHANGELOG entry originally said "Five sites" over that same list of six; it now says six. Deliberately **not** changed, both documented in the file: - **Over the tail cap**, `tail | sed '1d' | grep` and its `2>/dev/null` on `tail` are untouched. A pipeline element forks either way, so hoisting the redirect onto a group would newly silence `sed` and `grep` for no saving, and merging `sed` into `grep` would change the matching semantics. - **`printf '%s' "$RECORDS" | jq -cRs`** stays a pipeline rather than becoming a here-string, and the in-file comment now states the grounds as they actually stand. `hook::jq_field` in the shared library documents this hazard and refuses the here-string form for it: bash fills a here-string's pipe itself, so a payload at or above the pipe capacity can block before `jq` is exec'd. The trace behind that note (#1587: 65536 bytes hung indefinitely while 65000 returned at once) comes from this repo's **Windows Git Bash** hosts. It does **not** reproduce on Linux bash 5.2 — re-checked here at 65535, 65536, 65537, 200 kB and 2 MB, each returning immediately, including under an unwritable `TMPDIR`. The call keeps the library's conservative form anyway rather than bet the hazard is Linux-only: the forgone saving is one fork on the warning path only, since a turn with no failure record exits before that line. Earlier revisions of this PR and of the comment asserted the deadlock as universal fact, which is more than is known. ## Verification **Process counts**, measured with `strace -ff -qq -e trace=clone,clone3,fork,vfork,execve` (`-ff` so no syscall line is split across an `<unfinished>`/`<resumed>` pair), successful `execve` only, the harness's own top-level `bash <hook>` exec excluded: | Path | Creations before | after | execs before | after | |---|---|---|---|---| | No failure recorded, under the tail cap (the common case) | 18 | **9** | 6 | **4** | | No failure recorded, over the tail cap | 19 | **12** | 7 | **6** | | A turn that warns | 34 | **24** | 17 | **14** | The remaining 9 creations on the common path are 4 execs (`wc`, `grep`, and two `jq` passes inside the synced `hook-utils.sh`) plus subshell forks inside that same fenced library. The `execve` drop is one batched `jq` and two removed helper processes — removed *processes*, not removed work: every input is still read and every record still classified. **No wall-clock figure is claimed.** This Linux host says nothing about the Windows spawn tax the budget binds to, and on the #3508 host one process creation costs 180-2,841 ms (median 1,108 ms at 501 concurrent). The process count is the honest proxy and the README says so. **Behaviour is unchanged, proven, not asserted.** This hook's whole job is surfacing failures that otherwise pass unnoticed, so a carelessly hoisted `2>/dev/null` could silence exactly the diagnostic it exists to emit. Every hoisted group holds one command. The only stream now silenced that was not before is `grep`'s own stderr, and `cat`'s own `2>/dev/null` already discarded that same stream on the same path. Pre- and post-change stdout, stderr, exit codes **and marker-file contents** were compared byte for byte across ten scenarios — first warn, dedup, a new failing registration re-warning while the already-warned one stays muted, all three classification branches (launch / ambiguous / completed), the tail cap, a clean transcript, a missing transcript, the kill switch, and no data directory. Identical. **Budget test, mutation-checked.** `hook-failure-audit.test.sh` gains an strace-based assertion on both the creation and exec ceilings, skipping cleanly where strace is unavailable or not permitted. Non-vacuity was demonstrated, not assumed: moving either silenced redirect back inside its substitution — `SIZE=$(wc -c <"$TRANSCRIPT" 2>/dev/null)`, or `RECORDS=$(grep -F … -- "$TRANSCRIPT" 2>/dev/null)` — pushes creations from 9 to 10 with **no change to the exec count**, leaves all 93 behavioural assertions green, and fails the budget assertion. That is precisely the fork xtrace cannot see. **Gates, all foreground:** | Gate | Result | |---|---| | `scripts/affected-tests.sh --run` | exit 0, both selected suites pass (`hook-failure-audit.test.sh` 94/94, `audit-session-id.test.sh` 27/27) | | `shellcheck -x` on both changed shell files | clean | | `shfmt -d -i 2 -ci` | clean | | `scripts/check-changelog-parity.sh` `--check` / `--check-order` / `--check-bump origin/main` / `--check-preserved origin/main` | all four pass | | `markdownlint-cli2` on the changed markdown | clean | | `scripts/check-purged-em-dashes.sh` | pass | | `scripts/check-shell-portability.sh origin/main` | pass | | `scripts/check-silent-skips.sh`, `check-killswitch-hoist.sh`, `check-hook-exec-form.sh`, `check-hook-wiring-liveness.sh`, `check-cross-plugin-source-drift.sh`, `validate-plugins.sh` | pass | The wording-only follow-up commit re-ran the test suites, all four parity modes, `shellcheck -x`, `shfmt -d -i 2 -ci` and `markdownlint-cli2`: unchanged, including the 9-creation / 4-exec budget assertions, which is the expected result for a comment-and-prose change. `test_save_point.py::test_new_origin_falls_back_to_directory_name` is pre-existing and not in this diff's selection. ### Acceptance criteria not fully met Two of the issue's criteria are **not** satisfied, and no amount of in-file work would satisfy them: - **"A turn with no hook failures exits before any external process spawn."** Not met, and not reachable. The hook learns there is nothing to report *by reading the transcript*; there is no cheaper oracle. The floor without touching the fenced `hook-utils.sh` or abandoning the O(cap) tail bound is one `wc`, one `grep`, and the library's own payload parse — which is what this PR reaches. If the criterion is to be met literally it needs a different design (a marker written by the failing hook, or consolidation with #3515/#3516), not a further micro-optimization here. - **"The always-on per-turn set stays within <= 500 ms parallel wall."** Not measured. That figure is cross-plugin (this hook shares the budget with #3515 `autonomy` and #3516 `disk-hygiene`) and binds to Windows Git Bash, which this runner is not. The README records the figure as owed rather than implying it was taken. A third is met in a different form than specified: the issue asks for a **PATH shim** spawn census. This uses **strace** instead, which is strictly stronger for this defect — a PATH shim sees only programs that are `exec`'d, and the whole cost here is subshell forks that never exec anything. ## Related - Parent: #3508 (the campaign; its stated cause is corrected above for this hook) - Precedent: #3520 / PR #3779 (established redirection placement as the real cost), PR #3788 (34 scripts, 17 plugins, zero library edits) - Siblings on the same 500 ms per-turn budget: #3515 (`autonomy`), #3516 (`disk-hygiene`) - Fenced, deliberately untouched: #3740, #3838 (`lib/hook-utils.sh` and its 17 synced copies); the here-string note above cites `hook::jq_field` there but changes nothing in it - **Adjacency checked, no edit-surface overlap:** open PR #3769 adds correlation keys (`session_id`, `prompt_id`, `tool_use_id`, `agent_id`) to the hook-telemetry envelope spine inside `hook::emit_telemetry`. It does **not** touch `hook-failure-audit.sh`. This diff leaves that hook's `SESSION_ID` extraction and its `data.session_id` envelope key exactly as they are, so nothing here collides with the key migration. The two PRs both bump `plugins/claude-ops/.claude-plugin/plugin.json` and prepend to its CHANGELOG, so whichever lands second takes a trivial version/heading rebase. 🤖 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>
kyle-sexton
added a commit
that referenced
this pull request
Sep 7, 2026
…it (#3529) (#3872) Closes #3529 ## Summary `block-dangerous-git.sh` is 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, and `run-guards.sh` are untouched. ## Fix - **Common path (every Bash/PowerShell call):** the eager `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 inside `emit_tel`, behind the start-stamp and sink gates. - **Lease path (`--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. The `2>&1` cannot move onto an outer group here (git's stderr is the diagnostic the block message quotes, and an outer `2>&1` would send it to the hook's stdout), so the body is now `exec 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 now `printf -v`; `$(effective_dir ...)` around a builtins-only function is now `effective_dir_to`, a nameref assignment (the default base is read before the nameref is written, so the caller's scratch variable then assigns `HOOK_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 when `CLAUDE_PLUGIN_ROOT` is unset, which Claude Code never leaves unset. - Guardrails **0.32.13** (`main` is 0.32.10; #3849 takes 0.32.11 and #3869 takes 0.32.12, re-verified against `origin/main` and 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.sh` minus a no-op guard dispatched the same way; this repository as cwd, `HOOK_TELEMETRY_SINK` unset, `CLAUDE_PROJECT_DIR` empty; 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. | Scenario | creations before → after | execve before → after | |---|---|---| | Guard share, benign `git status --short` / `echo hello` | 3 → 2 | 0 → 0 | | Guard share, blocked `git push --force origin main` / `git reset --hard` | 3 → 2 | 0 → 0 | | Guard share, lease `--force-with-lease=main:<40-hex>` | 5 → 3 | 1 → 1 | | Guard share, `!` alias no trailing args | 5 → 3 | 0 → 0 | | Guard share, `!` alias with three trailing args | 8 → 3 | 0 → 0 | | Guard share, `!` alias whose body carries a lease | 7 → 4 | 1 → 1 | | Guard share, PowerShell `git status` | 15 → 14 | 3 → 3 | | Standalone `bash block-dangerous-git.sh`, benign | 9 → 8 | 3 → 3 | | Whole Bash dispatcher line from hooks.json, benign | 35 → 34 | 3 → 3 | **Deny paths still deny.** A/B against a pristine `origin/main` export 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-run` lease spellings, `reset --hard`/`--h`, `clean -f/-fd/-fdx/--force`, `checkout .`/`:/`/`-f`/`--pathspec-from-file`/exclude-only, `restore .`, `switch --discard-changes`; their near-miss safe variants `reset --keep`, `clean -n`, `checkout -- file`, `restore --staged .`, `branch -D`, `filter-branch`, quoted text; `!` and inline aliases, `bash -c`/`sh -c`/`env`/`sudo` wrappers, 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 `-C` dir (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 `git` on `PATH`. On a `PATH` carrying no `git` the lookup now fails inside the `exec` rather than around it, so the diagnostic the block message quotes changes wording: - before: `<hook>: line 341: git: command not found` - after: `<hook>: line 363: exec: git: not found` Measured directly on both trees, running the real hook against a `PATH` shadow that excludes `git` and nothing else: both exit **2**, both emit the same `BLOCKED: ... 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: a `git` on `PATH` that is present but not executable is rc **126** on both trees, and the `exec` form appends a second `cannot execute: Permission denied` line.) 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-prefixed `git`, a zero-width space glued to `--force`, and U+2028 glued to `--force` are 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.sh` 479 → 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 the `git rev-parse --show-object-format` exec'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); `exec` removed → 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 except `plugins/claude-ops/skills/plugins/scripts/cache-content-check.test.sh` (2 of 24, PS4 trace probe), which fails identically on a clean `origin/main` export, 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.sh` passes. `check-purged-em-dashes.sh` passes. 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 ...)`, both `lib/hook-utils.sh` (#3740, #3838) and outside this PR's fence, so that line is not closed from inside this file. (3) "A non-`git` command exits before any spawn": this guard spawns nothing of its own on any command; the dispatcher's two `jq` execs 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 - Refs #3508 (parent). Its "per-field jq" diagnosis does not apply to this guard: since #3788 the dispatcher buffers stdin and primes jq once per batch, so no individual guard contributes a jq spawn; the remaining cost here was forks that never exec. - Precedent for the mechanism and the strace pin: #3779 (context-guard), #3849 (guardrails 0.32.11), #3869 (guardrails 0.32.12), #3851, #3870, #3871. - Version-chain siblings: #3849, #3869 (entries kept verbatim; this PR adds only 0.32.13). - Out of fence, left for their owners: `$(hook::buffer_stdin)` fork-free form in #3740 / #3838; the PowerShell lane's 14 creations live in `lib/powershell/ps-command.sh`. 🤖 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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No related issue: Phase 1 of a measured leftover after the hook-performance program; does not close the isolation-fork or shebang-hop work.
Summary
Highest-impact leftover on the always-on Bash path after #3781: the guardrails dispatcher still parsed
ps-command.sh(~41 KB) five times on every Bash tool call, advisoryzone-gate.shstill parsedhook-utils.shto discover it was inert, and remaining production hooks still located siblings with$(dirname). Those parses and execs are gone. Guard decisions are unchanged.Fix
run-guards.shdefers--libuntil.tool_nameis known and skips it when that name isBash. Each git/commit guard that still runs alone sourcesps-command.shonly insideif [[ "$TOOL_NAME" == "PowerShell" ]], the same shapeblock-hook-bypassalready used. Isolation subshells do not inherit the include guard unless the parent sourced first, so both skips are required.block-noncanonical-commitcompares-mvalues againstPS_HERESTRING_PLACEHOLDERonly when the classifier defined it, so a Bash single-line-mdoes not abort underset -u.zone-gate.shinlines the default-advisory MODE check above everysource, the same shape as the kill-switch hoist.${BASH_SOURCE[0]%/*}/${FILE%/*}with the bare-filename and root fallbacks, including formatter parent walks. GNU Bash forks a subshell for every command substitution even when the body is a builtin (Command Substitution, Bash Reference Manual; https://mywiki.wooledge.org/CommandSubstitution). The repo-local.claudetelemetry sink stays byte-identical with the plugin copy modulo itshook-utilspath.What this phase does not do:
$(source …)isolation forks (guardrails dispatcher is the whole per-tool-call budget miss after the hook-performance program #3685) — high correctness riskhook-utils.shgitcommandaffected-tests.sh/check-shell-portability.sh/ context-budget Node hookif:— documented as a correctness residual, not a leftoverVerification
Host qualified with
plugins/performance/lib/spawn_noise.py: measurable (min 0.5 ms, spread 1.78×).HOOK_TELEMETRY_SINKunset. Spawn census through a stable PATH shim (plugins/performance/scripts/spawn-census.sh).git status --short)3 git,2 jq)3 git,2 jq)source …/ps-command.sh(bash -x)source hook-utils.sh(bash -x).shWrite5 dirname, …)dirname0)The milliseconds are context on this cheap-spawn host. The durable claims are the five classifier parses, the advisory library parse, and the five
dirnameexecs.plugins/guardrails/hooks/run-guards.test.shPASS=101 FAIL=0plugins/guardrails/hooks/block-no-verify.test.shPASS=240 FAIL=0plugins/guardrails/hooks/block-noncanonical-commit.test.shPASS=213 FAIL=0plugins/context-guard/hooks/zone-gate.test.shPASS=26 FAIL=0.claude/hooks/hook-telemetry-sink.test.shPASSscripts/check-changelog-parity.sh --checkand--check-bump origin/maincleanscripts/check-killswitch-hoist.sh31 scriptsRelated
Refs #3685
Refs #3781
Refs #3623
Refs #3749