Skip to content

perf(hook-utils): cut three subprocess spawns per hook invocation - #1979

Merged
kyle-sexton merged 3 commits into
mainfrom
perf/hook-utils-spawn-reduction
Aug 8, 2026
Merged

perf(hook-utils): cut three subprocess spawns per hook invocation#1979
kyle-sexton merged 3 commits into
mainfrom
perf/hook-utils-spawn-reduction

Conversation

@kyle-sexton

Copy link
Copy Markdown
Contributor

Closes #1978

Summary

Every hook in this marketplace sources lib/hook-utils.sh, and buffering the hook payload spawned
three external processes to do work bash can do in-process. On Windows Git Bash, where process
creation is fork() emulation, each spawn costs roughly 140 ms — paid on every tool call, in all 16
plugins that carry the library.

Fix

  • hook::resolve_read_slice: the awk float division becomes fixed-point shell arithmetic,
    printing the same three-decimal form read -t is given. printf -v, not $( ), because a command
    substitution forks the shell even for a builtin — the fork is the cost being removed.
  • hook::buffer_stdin: printf | tr -d '\r' becomes ${input//$'\r'/}, and the post-loop
    jq -e . validity probe is skipped when hook::json_complete already parsed the identical
    CR-stripped buffer with jq inside the read loop. json_complete returns non-zero both for an
    incomplete buffer and for absent/broken jq, so the flag is set only on its success path and the
    jq-absent fail-open is untouched.
  • New hook::jq_fields: extracts several fields from one payload in a single jq process, for
    hooks that read two or three fields from the same envelope and currently pay a fork plus an exec
    for each. It uses // "" rather than // empty so an absent field keeps its slot instead of
    silently shifting every later index onto the wrong filter, reads NUL-separated values through a
    process substitution (command substitution strips NUL), and strips CR after the read — the
    Windows jq build writes stdout in text mode and expands every LF it emits to CRLF, so a value
    cleaned inside jq arrives dirty anyway.

No hook call sites change in this PR. The plugins that read a second field already gate it behind an
early exit or a telemetry probe, so converting them would add work on the common path; the batch
helper's win is in the guardrails git guards, which read .tool_input.command and .tool_name
unconditionally — and those files are in flight in #1974. The helper ships now because the lib sync
gate makes every library change cost a version bump in all 16 carrying plugins; adding it later would
pay that a second time.

Verification

Measured, quiet box, 15 alternating pairs of the same block-dangerous-git invocation against
each library version (alternating so machine-load drift hits both arms equally):

lib mean min max
main 1672 ms 1316 ms 2443 ms
this branch 1401 ms 1120 ms 1760 ms

~270 ms per invocation, and the slow tail shrinks with the mean. That is less than the
3 × 140 ms the spawn-count model predicts; the measured number is the one to trust.

Gates run locally:

  • lib/hook-utils.test.sh — new coverage for the slice format (including the fallbacks a
    non-numeric bound and a 0.000 quotient must take) and for hook::jq_fields (multi-line and
    CR-carrying values, absent-field slot retention, unparsable payload, no-filter call,
    non-string values). The two buffer_stdin timing assertions that fail intermittently here fail
    the same way on main (1–3 failures per run on both sides) — they are wall-clock-ceiling tests
    on a loaded Windows box, the same class as the ceilings tracked for
    block-noncanonical-commit.test.sh.
  • plugins/guardrails/hooks/block-dangerous-git.test.sh — the black-box hook contract suite, run
    serially (never concurrently: its wall-clock assertions fail spuriously under parallelism).
  • scripts/sync-hook-utils.sh --check — all 16 plugin copies match.
  • scripts/sync-hook-utils.sh --check-bump origin/main — every carrying plugin bumped.
  • scripts/check-changelog-parity.sh --check-bump origin/main and --check-order.
  • scripts/check-shell-portability.sh --paths, shellcheck -x, shfmt -d -i 2,
    markdownlint-cli2, scripts/check-manifest-duplicate-keys.py.

Fresh-docs mandate: no WebFetch was required for this change and none was performed. The edit is
internal implementation of a shell library — it touches no hook contract surface, no manifest field
beyond the mechanical version bumps the sync gate itself demands, and no documented harness
behavior. The 16 touched manifests are version lines only.

Related

Every hook in this marketplace sources the shared library and buffers its
stdin through hook::buffer_stdin, which spawned three external processes to
do work bash can do in-process:

  * awk, to divide the read timeout by the slice count — one float division.
  * printf | tr -d '\r', a fork and an exec to delete one byte class from a
    string parameter expansion rewrites in place.
  * a second jq -e . validity probe over the very buffer hook::json_complete
    had already parsed with jq inside the read loop.

On Windows Git Bash process creation is fork() emulation and each spawn costs
~140 ms, so this was pure overhead on every tool call, in all 16 plugins that
carry the library.

Measured on a quiet box, 15 alternating pairs of the same block-dangerous-git
invocation against each library version: 1672 ms mean (1316–2443) before,
1401 ms mean (1120–1760) after — and the slow tail shrinks with the mean.

Behavior is held fixed. The slice keeps the exact three-decimal form read -t
is given (a quotient landing on an exact half-millisecond now rounds half-up
rather than through awk's binary approximation — 1 ms in an idle bound that is
itself an approximation). The buffer is CR-stripped as before. The
completeness verdict is reused only when jq itself produced it, so a host with
no jq still reaches the same fail-open branch.

Also adds hook::jq_fields, which extracts several fields from one payload in a
single jq process — for the hooks that read two or three fields from the same
envelope and pay a fork plus an exec for each. It uses `// ""` rather than
`// empty` so an absent field keeps its slot instead of shifting every later
index, and strips CR after the read because the Windows jq build writes stdout
in text mode and expands every LF it emits to CRLF.

Closes #1978

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@cursor

cursor Bot commented Aug 8, 2026

Copy link
Copy Markdown

Bugbot is not enabled for your account, so this pull request was not reviewed.

Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs.

@claude

claude Bot commented Aug 8, 2026

Copy link
Copy Markdown

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


Review of PR #1979perf(hook-utils): cut three subprocess spawns per hook invocation

  • Gather context (PR body, custom instructions, CLAUDE.md)
  • Fetch diff via git diff origin/main...HEAD
  • Review lib/hook-utils.sh core changes (resolve_read_slice, buffer_stdin, new jq_fields)
  • Review lib/hook-utils.test.sh coverage
  • Verify parity across the 15 plugin mirror copies + manifest/changelog bumps
  • Post final review

Summary

Solid, well-verified change. I traced the resolve_read_slice fixed-point arithmetic by hand against the old awk "%.3f", t/n output for every case in the new test table (t=2→0.500, t=10→2.500, t=0.5→0.125, t=0.004→0.001, t=0.000→0.000 1 fallback, non-numeric→fallback) — all match, including the documented half-millisecond rounding seam vs. awk. I also confirmed all 15 plugins/*/hooks/hook-utils.sh copies are byte-identical to lib/hook-utils.sh, every carrying plugin's plugin.json took a correct patch bump, and the CHANGELOG entries are consistent across the sampled plugins (guardrails, rate-limit-guard, typos-format). The hook::buffer_stdin reuse of the json_complete verdict via the new validated flag is correct: the string checked and the string later assigned to input after CR-stripping are byte-identical, since no more input is read between the check and the reassignment.

Finding

hook::jq_fields silently collapses a literal false field to the same empty-string sentinel as an absent/null field, contradicting its own doc comment.

lib/hook-utils.sh#L651:

prog+="((${filter}) // \"\" | tostring)"

jq's // alternative operator treats false as falsy exactly like null — not just "absent/error." Verified directly:

$ echo '{"a":false,"b":true}' | jq -c '{a: ((.a) // "" | tostring), b: ((.b) // "" | tostring)}'
{"a":"","b":"true"}

The function's own doc comment (lines ~620-624) says: "An absent, null, or empty field becomes the EMPTY STRING and keeps its slot... Emptiness stays the caller's decision." A literal false isn't absent, null, or empty — but it's silently indistinguishable from an absent field under the current expression. This is the same pre-existing quirk in hook::jq_field (which uses // empty), but it's worth catching here because hook::jq_fields's whole design rationale (// "" instead of // empty) is specifically about not letting a field's value get lossily conflated with absence — and this one case still does.

Impact today is latent: the PR explicitly notes no call sites use this helper yet, and the current candidate future callers (.tool_input.command, .tool_name) are string fields. But it's new, heavily-documented library surface that future hooks (e.g. a boolean flag like a success/enabled field) will build on without knowing about this gap, and the test suite covers null/number/absent but not a false value.

Suggested fix: replace the alternative-operator form with an explicit null check, e.g. ((${filter}) as $v | if $v == null then "" else ($v | tostring) end), and add a test case with a false-valued field.

Fix this →

Everything else

  • No architectural concerns — shipping hook::jq_fields now without call sites is explicitly justified by the sync-gate's per-version-bump cost, and that reasoning holds.
  • Fresh-docs mandate self-assessment (no WebFetch needed) is correct: this touches no hook contract surface, and the 16 manifest diffs are mechanical version-only bumps.
  • hook::buffer_stdin's switch from printf | tr -d '\r' to ${input//$'\r'/} and the read_slice awk removal are behavior-preserving and correctly reasoned.

@claude

claude Bot commented Aug 8, 2026

Copy link
Copy Markdown

Claude finished @kyle-sexton's task in 2m 11s —— View job


Security review

  • Gather context (gh pr diff, read changed files)
  • Review lib/hook-utils.sh changes for injection / logic flaws
  • Review lib/hook-utils.test.sh for issues
  • Review the 16 plugin-synced copies + manifest/changelog bumps
  • Post findings

Scope reviewed: lib/hook-utils.sh (source of truth), lib/hook-utils.test.sh, and the 16 synced plugins/*/hooks/hook-utils.sh copies + their plugin.json/CHANGELOG.md version bumps. No workflow files are touched by this PR.

No security issues found. This is a narrowly-scoped internal perf refactor with no new external inputs, no privilege changes, and no injection surfaces introduced. Specifics I checked and ruled out:

  • Bash-arithmetic injection in hook::resolve_read_slice (lib/hook-utils.sh#L463-L498): $t (from the stdin_read_timeout userConfig, CLAUDE_PLUGIN_OPTION_STDIN_READ_TIMEOUT) is only used inside a [[ ... =~ ^([0-9]+)(\.([0-9]+))?$ ]] match. The $((...)) arithmetic that follows uses only ${BASH_REMATCH[1]}/${BASH_REMATCH[3]} — values the anchored, digits-only regex guarantees contain nothing but 0-9. Bash arithmetic recursively expands variable references, which is a classic injection vector when the raw attacker-influenced string reaches $(( )) directly, but that never happens here — only the pre-validated digit-only captures do. Overflow/garbage inputs fall through to the documented unsliced "<t> 1" form rather than misbehaving.
  • hook::buffer_stdin's new validated short-circuit (lib/hook-utils.sh#L500-L596): this is the security-relevant path, since guard hooks like block-dangerous-git rely on hook::buffer_stdin's validity verdict before parsing .tool_input.command. Traced it end-to-end: validated=1 is only set immediately before a break, so no further mutation of input can occur between the hook::json_complete check and the final ((validated == 0)) && ... gate — the post-loop input="${input//$'\r'/}" re-strip is idempotent on an already CR-stripped string. The skip reuses a verdict jq already produced on the exact same bytes; it doesn't weaken the fail-open behavior for a jq-absent host (untouched code path) or let malformed JSON through.
  • New hook::jq_fields (lib/hook-utils.sh#L614-L677): the untrusted hook payload is passed to jq as stdin data (printf '%s' "$input" | jq -j "$prog"), never interpolated into the jq program string — the filters concatenated into $prog are literal strings supplied by call sites, not derived from the payload, so there's no jq-filter-injection path. Confirmed via grep that no hook in this PR calls hook::jq_fields yet (it ships unused, per the PR description, for a future PR) — so there's no live call site to assess for a tainted-filter misuse today. Worth keeping in mind for whichever PR wires up the first caller: hook::jq_fields must only ever be called with static, hardcoded jq filter strings, never a filter built from hook input — but that's a note for future review, not a finding against this PR.
  • NUL-separated field parsing: read via process substitution (not $( ), which strips NULs) with a ((${#values[@]} == $#)) || return 1 count check — an embedded literal `` inside a JSON string value (the one documented residual) would desync the count and fail closed rather than silently return misaligned/wrong field values.
  • Manifest / changelog changes: all 16 plugin.json diffs are version-line-only bumps (no permissions, no new commands/MCP servers, no config surface changes); all 16 hooks/hook-utils.sh copies are byte-identical to the lib/hook-utils.sh source (matching SHAs), consistent with the sync-gate guarantee. No secrets or credentials present in the CHANGELOG additions I spot-checked.

No CRITICAL, IMPORTANT, or SUGGESTION findings to report.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 5fae68cf1d

ℹ️ About Codex in GitHub

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

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

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

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

Comment thread lib/hook-utils.sh
@github-actions

github-actions Bot commented Aug 8, 2026

Copy link
Copy Markdown

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

…-reduction

# Conflicts:
#	plugins/source-control/.claude-plugin/plugin.json
#	plugins/source-control/CHANGELOG.md
@claude

claude Bot commented Aug 8, 2026

Copy link
Copy Markdown

Claude finished @kyle-sexton's task in 2m 36s —— View job


Review of PR #1979perf(hook-utils): cut three subprocess spawns per hook invocation (re-run on synchronize)

  • Gather context (PR body, CLAUDE.md, prior review comments)
  • Diff origin/main...HEAD to see what changed since the last review pass
  • Confirm lib/hook-utils.sh content is unchanged since commit 5fae68c (the merge commit only reconciles plugins/source-control/.claude-plugin/plugin.json)
  • Re-check the previously-flagged hook::jq_fields finding and Codex's new nounset comment
  • Post updated review

What changed since the last review

Nothing in lib/hook-utils.sh or lib/hook-utils.test.sh. git diff 5fae68c..HEAD -- lib/hook-utils.sh is empty — the second commit (fedcde6) is a merge of origin/main that only pulls in an unrelated plugins/source-control version bump. The correctness/test-coverage finding from my earlier pass is therefore still open:

hook::jq_fields still collapses a literal false field to the same empty-string sentinel as an absent field, at lib/hook-utils.sh#L651:

prog+="((${filter}) // \"\" | tostring)"

This contradicts the function's own doc comment (L620-623), which promises only an absent, null, or empty field becomes "". jq's // treats false as falsy too, so a real false value is indistinguishable from an absent field. Test coverage in lib/hook-utils.test.sh#L1598-L1602 exercises number and null but still has no false case. Not a regression risk today (no call sites exist yet, per the PR description), but it's new library surface a future boolean-field caller would build on without knowing about the gap.

Fix this →

On Codex's nounset comment (lib/hook-utils.sh#L675)

I looked into this and believe it's a false positive. The pre-4.4 Bash nounset bug is specific to expanding array elements"${arr[@]}"/"${arr[*]}" on a zero-element array raises "unbound variable" under set -u. It does not apply to ${#arr[@]}, the length expansion, which is well-defined as 0 for an empty or unset array at any Bash version, including 3.2. The repo's own documented workaround in plugins/markdown-format/hooks/markdown-format.sh:444-447 makes exactly this distinction — it guards "${arr[@]}" expansions but leaves ${#arr[@]} checks on possibly-empty arrays unguarded throughout that same file (e.g. lines 527, 661, 684: while ((${#queue[@]} > 0)), if ((${#MODULE_FILES[@]} > 0)), if ((${#RISK_CONFIGS[@]} > 0))). If ${#arr[@]} itself aborted under set -u on empty arrays, those pre-existing unguarded call sites would already be broken on the same Bash range this comment worries about. hook::jq_fields's own test cases at lines 1582 and 1590 (unparsable payload, no-filter call) already exercise the values array staying empty and reaching line 675 under this test file's own set -uo pipefail (line 7) without failing, consistent with that reading. I'd treat this as no action needed, but flagging so the author can weigh in if there's a specific 3.2-4.3 Windows/macOS host that showed the abort in practice.

Everything else

  • No other changes since my prior pass: hook::resolve_read_slice's fixed-point arithmetic, the printf -v (not $( )) discipline, and hook::buffer_stdin's validated-flag reuse of the json_complete verdict all check out as before.
  • All 15 plugin-synced hook-utils.sh copies remain byte-identical to lib/hook-utils.sh; the only file touched by the merge commit outside lib/ and plugins/source-control is the version-bump reconciliation already covered by scripts/sync-hook-utils.sh --check-bump.
  • Architecture, error-handling, and test-coverage decisions (shipping hook::jq_fields ahead of call sites, the NUL-separated wire format, the CR-stripped-after-jq ordering) remain well-justified and unchanged from the version already reviewed.
    ·

@claude

claude Bot commented Aug 8, 2026

Copy link
Copy Markdown

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


Security review of PR #1979

  • Gather context (gh pr diff, read changed files)
  • Review lib/hook-utils.sh changes for injection / logic flaws
  • Review lib/hook-utils.test.sh for issues
  • Review the 16 plugin-synced copies + manifest/changelog bumps
  • Post findings

Scope reviewed: lib/hook-utils.sh (source of truth), lib/hook-utils.test.sh, and the 16 synced plugins/*/hooks/hook-utils.sh copies + their plugin.json/CHANGELOG.md version bumps. No workflow files are touched by this PR.

No new security-relevant changes since the last full review pass. I diffed the current head (fedcde6) against the commit already reviewed in depth (5fae68c): lib/hook-utils.sh and lib/hook-utils.test.sh are byte-identical between the two — the only new commit is a merge of origin/main, which touches none of the security-scoped files. The prior review's findings stand:

  • hook::resolve_read_slice (lib/hook-utils.sh#L463-L498): the timeout value only ever reaches $(( )) through ${BASH_REMATCH[1]}/${BASH_REMATCH[3]}, captured by an anchored digits-only regex — no path for the raw, less-trusted string to reach shell arithmetic directly. No injection.
  • hook::buffer_stdin's reuse of the hook::json_complete verdict via the new validated flag (lib/hook-utils.sh#L500-L596): validated=1 is set only immediately before the break that ends the loop on the exact same CR-stripped bytes checked; nothing mutates input in between. Guard hooks that gate on this function's validity verdict (e.g. block-dangerous-git) are unaffected — fail-open behavior for a jq-absent host is untouched.
  • New hook::jq_fields (lib/hook-utils.sh#L614-L677): the untrusted hook payload is passed to jq as stdin data, never spliced into the jq program string — filters concatenated into prog are static strings from call sites, not payload-derived. Confirmed again: no hook in the repo calls hook::jq_fields yet, so there is no live call site with an injection surface today.
  • NUL-separated field parsing: read via process substitution (not $( ), which would strip NULs), with a count check that fails closed (not mistaking a partial read for a complete one) rather than silently returning misaligned field values.
  • All 16 plugins/*/hooks/hook-utils.sh copies remain byte-identical to lib/hook-utils.sh (matching SHAs in the diff), consistent with the sync-gate guarantee — no drifted copy could diverge in security-relevant behavior.

One non-security note for the author to double check, not a finding: plugins/source-control/.claude-plugin/plugin.json shows the worktree_create_gate_enabled userConfig property removed in this PR's diff, which looks like a merge-artifact from pulling in origin/main (worktree_root/worktree_create_gate_enabled land as part of an unrelated PR, #1970) rather than anything this PR intends. It isn't a security issue either way — worktree-create-gate.sh reads the flag as ${CLAUDE_PLUGIN_OPTION_WORKTREE_CREATE_GATE_ENABLED:-true}, so the gate stays fail-safe enabled even without the manifest declaring the option — but it's worth confirming the merge didn't silently drop a config surface that belongs on main.

No CRITICAL, IMPORTANT, or SUGGESTION security findings to report.

kyle-sexton added a commit that referenced this pull request Aug 8, 2026
…ed --sessions (#1981)

Closes #1980

## Summary

Two ways `retro`'s chain-scoped path produced a wrong answer with no
error signal: a comma-joined
`--sessions` list resolved to nothing, and a chain walk that terminated
early was indistinguishable
from a genuinely short chain.

## Fix

**`--sessions` comma splitting.** The option is declared `nargs="+"`, so
`--sessions a,b,c` was
consumed as one literal token that matched no transcript, and the run
reported `0 with transcript`
for a chain whose transcripts all existed. Tokens are split on `,` after
parsing — a session id
never contains one, so the split cannot change the meaning of a
correctly space-separated
invocation. Empty fragments (`a,,b`, a trailing comma) are dropped
rather than passed on as an id
that cannot exist; a value resolving to no ids at all reaches the
existing usage error (exit 2).

**`chain_coverage`.** Multi-session output gains `requested` / `found` /
`available` / `ratio`.
`available` counts the transcripts present in the base directory — the
per-project transcript
directory — which is the denominator the `previous_handoff` walk
structurally cannot see. It is
coverage evidence for a reader, not a filter: some sibling transcripts
will belong to other work,
which is exactly why the skill surfaces the ratio rather than the parser
widening the chain. The
same ratio also rides in the human-readable `summary`, so it is visible
without reading the
structured field. An unreadable base directory degrades `available` to
`null` instead of failing
the parse.

**Skill contract.** `retro`'s SKILL.md and `context/session.md` now
require stating the discovery
basis, and forbid presenting a low-coverage chain retrospective
silently: below a ratio of ~0.5,
name `found` and `available` and offer `--sessions` with the ids
enumerated.

## Verification

- `plugins/session-flow/skills/retro/scripts/test_parse_transcript.py` —
35 passed (was 30). New
cases: comma-joined list resolves the same list as the space-separated
form and keeps its order
(first id = current session); mixed separators with empty fragments; a
`--sessions ,` value that
yields no ids exits 2; coverage reported as 2-of-5 with `ratio` 0.4 and
the ratio present in
  `summary`; full coverage reports `ratio` 1.0.
- `plugins/session-flow/skills/retro/scripts/parse-transcript.test.sh` —
passes.
- `ruff check` and `ruff format --check` — clean.
- `scripts/check-changed-skills.sh origin/main` — `retro` PASS, 0
errors.
- `scripts/check-changelog-parity.sh --check-bump origin/main` and
`--check-order`, and
  `markdownlint-cli2` on the three touched markdown files — clean.

**Fresh-docs mandate**: no WebFetch was required and none was performed.
This changes a script's own
CLI behavior, its JSON output shape, and skill prose — no plugin
manifest field beyond the `version`
bump, no hook contract, no documented harness behavior.

## Related

- Refs #1979 and #1974 — other fixes draining the same audit inbox. No
file overlap with either:
`session-flow` carries no copy of the shared hook library, so it is
untouched by the 16-plugin
  version bump in #1979.
- The same audit report's third finding (a subagent completion-signal
contract in `orchestrate`)
overlaps an audit item resolved separately and is deliberately out of
scope here.

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
…-reduction

Resolves the version-line collisions this branch's 16-plugin lib bump has
with three PRs that landed on main first (#1974 guardrails, #1981
session-flow, #1983 claude-ops).

- guardrails: main released 0.19.1 (#1974's PowerShell sink fixes), so the
  shared-lib entry re-heads as 0.19.2.
- claude-ops: main released 0.27.2 (#1983's $HOME spelling fix, which also
  escaped the manifest's em-dashes), so the shared-lib entry re-heads as
  0.27.3 and the manifest keeps main's escaped description.
- Both changelogs keep BOTH entries, each under its own version.

Gates: sync-hook-utils.sh --check (all 16 copies match) and --check-bump
origin/main (lib changed and every carrying plugin bumped) both pass.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@claude

claude Bot commented Aug 8, 2026

Copy link
Copy Markdown

Claude finished @kyle-sexton's task in 1m 50s —— View job


Review of PR #1979perf(hook-utils): cut three subprocess spawns per hook invocation (re-run on synchronize, HEAD 6ba4eb9)

  • Gather context (PR body, CLAUDE.md, prior review comments)
  • Diff origin/main...HEAD and fedcde6..6ba4eb9 to see what changed since the last full review pass
  • Confirm lib/hook-utils.sh / lib/hook-utils.test.sh content is unchanged since the last review (the two new commits are merges of origin/main)
  • Audit the merge commits for unintended drift in the files they touch
  • Post updated review

What changed since the last review

lib/hook-utils.sh and lib/hook-utils.test.sh are byte-identical between fedcde6 (last reviewed) and 6ba4eb9 (current HEAD) — the two new commits are merges of origin/main (#1981, session-flow) with no direct edits to this PR's own source files. The previously-reported finding is therefore still open:

hook::jq_fields still collapses a literal false field to the same empty-string sentinel as an absent field, at lib/hook-utils.sh#L651 (prog+="((${filter}) // \"\" | tostring)"). jq's // treats false as falsy the same as null, contradicting the function's own doc comment at L620-623 ("An absent, null, or empty field becomes the EMPTY STRING"). Test coverage still has no false-valued case. Not a regression risk today (no call sites exist yet), but it's new library surface a future boolean-field caller will build on without knowing about the gap.

The Codex nounset comment on L675 remains resolved as a false positive per the author's own reply — ${#values[@]} is the nounset-safe length form, the repo already relies on that exact distinction elsewhere (markdown-format.sh), and the array is non-empty by construction at that point since hook::jq_fields returns early on $# == 0. Agreed, no action needed.

New finding (code-review lane): merge dropped a userConfig property that origin/main added and this plugin's own hook still depends on

plugins/source-control/.claude-plugin/plugin.json no longer declares worktree_create_gate_enabled, even though origin/main (PR #1970, merged into this branch at commit fedcde6) added it, and the hook script + its test suite still reference it.

  • git diff origin/main...HEAD -- plugins/source-control/.claude-plugin/plugin.json shows the property being removed relative to origin/main — this branch's merge silently dropped it while bumping the manifest version past it (0.47.00.47.1).
  • plugins/source-control/hooks/worktree-create-gate.sh#L38 and #L62 still read CLAUDE_PLUGIN_OPTION_WORKTREE_CREATE_GATE_ENABLED, and worktree-create-gate.test.sh still sets it directly — so the code and tests assume the manifest declares this knob.
  • plugins/source-control/.claude-plugin/plugin.json#L169-L178 shows babysit_worktree_root and worktree_root still present, with worktree_create_gate_enabled missing between them (it sat there in origin/main's 0.47.0).
  • The 0.47.1 CHANGELOG entry only documents the hook-utils.sh sync change, not this removal — so it isn't an intentional deprecation, it's a merge-conflict-resolution artifact.

This is squarely a code-review-lane concern (manifest correctness / maintainability, CLAUDE.md's "Configurable without editing the plugin" rule — userConfig is how a consumer discovers and sets this option without forking), not a security issue: the hook's ${CLAUDE_PLUGIN_OPTION_WORKTREE_CREATE_GATE_ENABLED:-true} still fails safe (enabled) at runtime either way, so nothing is exploitable — but a consumer can no longer see or set the opt-out through the plugin's declared config surface, and scripts/sync-hook-utils.sh --check-bump / check-changelog-parity.sh wouldn't catch this since it's unrelated to the hook-utils sync they gate.

Fix this →

Everything else

  • Architecture, error-handling, and test-coverage decisions in lib/hook-utils.sh (resolve_read_slice fixed-point arithmetic, printf -v discipline, buffer_stdin's validated-flag reuse of the json_complete verdict, NUL-separated wire format for jq_fields, CR-stripped-after-jq ordering) remain well-justified and unchanged from the version already reviewed in depth.
  • All 15 plugin-synced hook-utils.sh copies remain byte-identical to lib/hook-utils.sh; manifest/changelog version bumps for those 15 plugins are consistent, mechanical, and correctly gated.

@claude

claude Bot commented Aug 8, 2026

Copy link
Copy Markdown

Claude finished @kyle-sexton's task in 1m 2s —— View job


Security review of PR #1979 (re-run at 6ba4eb9)

  • Diff current HEAD against last-reviewed commit (fedcde6) to see what's new
  • Review any new security-relevant changes
  • Post findings

Scope reviewed: lib/hook-utils.sh, lib/hook-utils.test.sh, and the 16 synced plugins/*/hooks/hook-utils.sh copies + their plugin.json/CHANGELOG.md bumps — the same scope as the prior two security passes on this PR. No workflow files are touched.

No new security-relevant changes since the last full review. git diff fedcde6..6ba4eb9 -- lib/ 'plugins/*/hooks/hook-utils.sh' is empty — every security-scoped file is byte-identical to the version already reviewed. The new commit (6ba4eb9) is a merge of origin/main (bringing in #1981 session-flow work and unrelated claude-ops/guardrails/session-flow bumps); confirmed via git diff origin/main...HEAD --stat that none of those merged-in files are part of this PR's actual diff — they land on main independently and this branch is just catching up.

Findings from the prior passes stand, unchanged:

  • hook::resolve_read_slice (lib/hook-utils.sh#L463-L498): the timeout value only reaches $(( )) through ${BASH_REMATCH[1]}/${BASH_REMATCH[3]}, captured by an anchored digits-only regex. No path for the raw, less-trusted string to reach shell arithmetic directly. No injection.
  • hook::buffer_stdin's validated-flag reuse of the hook::json_complete verdict (lib/hook-utils.sh#L500-L596): the flag is set only immediately before the loop's break, on the exact same CR-stripped bytes; nothing mutates input in between. Guard hooks (e.g. block-dangerous-git) that gate on this verdict are unaffected, and jq-absent fail-open behavior is untouched.
  • New hook::jq_fields (lib/hook-utils.sh#L614-L677): the untrusted payload is passed to jq as stdin data, never spliced into the jq program string — filters are static, call-site-supplied literals. Still no call sites exist anywhere in the repo, so there's no live injection surface today; any future caller must keep passing only static filter strings, never payload-derived ones.
  • NUL-separated field parsing reads via process substitution (not $( )) and fails closed on a field-count mismatch rather than returning misaligned values.
  • All 16 plugins/*/hooks/hook-utils.sh copies remain byte-identical to lib/hook-utils.sh.

One pre-existing non-security note, unchanged from before: plugins/source-control/.claude-plugin/plugin.json still shows worktree_create_gate_enabled absent relative to main (a merge-artifact from #1970 landing on main after this branch's base) — not a security issue, since worktree-create-gate.sh reads the flag with a fail-safe ${CLAUDE_PLUGIN_OPTION_WORKTREE_CREATE_GATE_ENABLED:-true} default, but worth the author double-checking the rebase carried it correctly.

No CRITICAL, IMPORTANT, or SUGGESTION security findings to report.

@kyle-sexton
kyle-sexton merged commit 9b90e35 into main Aug 8, 2026
33 checks passed
@kyle-sexton
kyle-sexton deleted the perf/hook-utils-spawn-reduction branch August 8, 2026 05:48
kyle-sexton added a commit that referenced this pull request Aug 8, 2026
…ed tail (#2001)

No linked issue

Consumer report drained from the handoff inbox:
`20260730-182801-context-guard-zone-crossing-hook-times-out-100-percent`.
The zone-crossing hook was reported timing out 100% of the time; all
four registrations sat at `timeout: 10`.

## The number was authored here, not inherited

Per the hooks page fetched this session
(<https://code.claude.com/docs/en/hooks>):

> `timeout` | no | Seconds before canceling. Defaults: 600 for
`command`, `http`, and `mcp_tool`; 30 for `prompt`; 60 for `agent`.
`UserPromptSubmit` lowers the `command`, `http`, and `mcp_tool` default
to 30, and `MessageDisplay` lowers it to 10.

Unit is **seconds**. The applicable defaults are 600 (`PostToolBatch`,
`PreToolUse`, `PostCompact`) and 30 (`UserPromptSubmit`). So `10` was a
deliberate narrowing to 1/60th of the default, not something inherited —
and it is below what the hooks actually take on Windows.

## Measured, post-#1979

The prior per-invocation saving (`9b90e351`) is on `main`, so the
report's timings were stale. Re-measured with a harness invoking each
script exactly as the hook would — real payload on stdin, `HOME` /
`CLAUDE_PLUGIN_DATA` / `CLAUDE_PLUGIN_ROOT` set, snapshot present so the
resolver does real work. Two runs, 18 samples per path:

| Path | min | max |
|---|---|---|
| `zone-crossing-inject.sh` — PostToolBatch, 150 KB payload | 2.21 s |
6.68 s |
| `zone-crossing-inject.sh` — UserPromptSubmit, small payload | 2.24 s |
**22.01 s** |
| `zone-gate.sh` — PreToolUse | 0.45 s | 2.83 s |
| `post-compact-mark.sh` — PostCompact | 1.27 s | **12.37 s** |

**These are noisy and are presented as such.** CPU load was 14% before
run 1 and 68% during run 2; 523 processes; Defender real-time protection
enabled. Identical work spanned 3.3 s → 22.0 s, so the means are
unreliable and only the maxima carry the decision.

Three things the measurement establishes:

1. **`post-compact-mark.sh` reached 12.4 s — over the old 10 s cap.**
The report flagged this one as *inferred, not measured*, and as the most
consequential, since sibling plugins read its marker. It is now measured
fact.
2. **`zone-gate.sh` peaked at 2.83 s with no observed overrun.** It is
raised for uniformity and tail-safety, not because it was failing —
stated plainly rather than folded into a "they were all broken" claim.
3. **The tail is environmental, not payload-scaling.** The *small*
UserPromptSubmit payload (22.0 s) beat the 150 KB PostToolBatch one (6.7
s). Sizing has to survive an AV-stalled process spawn, not just the
median.

## Why 60 and not the report's suggested 30

22.0 s is a **floor, not a p100**: the harness times the script alone,
excluding the harness's own hook-launch overhead, and it never sets
`HOOK_TELEMETRY_SINK`, so `hook::emit_telemetry` short-circuits and its
per-invocation `jq -n` + sink exec are excluded too (spawns cost ~140 ms
each here, per the 0.4.6 entry). 30 would leave under 8 s of margin on
an already-optimistic number. 60 gives ~2.7× while staying an order of
magnitude under the 600 s default, so a genuinely hung hook still cannot
stall a session for ten minutes. `guardrails` and `disk-hygiene` already
declare 60 in this marketplace. A timeout is a cap, not a cost.

## What #1988 changed about the edit surface

`.claude-plugin/plugin.json` no longer carries a `hooks` key at all —
#1988 removed the redeclaration of the default-discovered path. The
manifest was re-read at HEAD rather than trusted from the report.
Consequence: `hooks/hooks.json` is now unambiguously the single
declaration site for a timeout, so this change touches one file plus the
version bump. The report's four-registration table is still accurate.

## Deliberately not asserted

The page says only "Seconds before canceling" — what a cancelled hook
reports, whether partial output is discarded, and whether sibling hooks
continue are **not documented**, so none of it is claimed here. Likewise
the page states 30 as `UserPromptSubmit`'s *default* and does not say
whether it also **caps** a larger explicit value; 60 is declared
regardless, which is harmless if clamped, since 22.0 s still clears 30.

## Verification

| Check | Result |
|---|---|
| `zone-crossing-inject.test.sh` | PASS=18 FAIL=0 |
| `zone-gate.test.sh` | PASS=24 FAIL=0 |
| `post-compact-mark.test.sh` | PASS=16 FAIL=0 |
| `hooks.json` parses | OK — timeouts `[60, 60, 60, 60]` |
| `plugin.json` parses | OK — 0.4.8, no `hooks` key |
| `markdownlint-cli2` | 0 issues |
| `check-changelog-parity.sh --check-bump origin/main` | pass |
| shellcheck / shfmt / shell-portability / exec-bit | N/A — no `.sh`
changed (2 JSON + 1 MD) |

`check-orphaned-fixtures.sh` was not run locally; it exceeds a 300s
timeout on this machine. CI covers it.

`context-guard` 0.4.7 → **0.4.8**.

## Out of scope, surfaced not fixed

- Profiling the hot path (the report's suggestions 2–3) is the durable
fix for the *cost*; this PR fixes the *cap*. The Windows per-invocation
cost is still real and still open.
- A timeout-observability notice is already served: every hook emits
`duration_ms` via `hook::emit_telemetry`, opt-in through
`HOOK_TELEMETRY_SINK`.
- Same defect class elsewhere, unmeasured: `rate-limit-guard` declares
`timeout: 10`, `guardrails` has two entries at `10`, and `claude-ops`
declares `timeout: 5` on **eight** registrations — the tightest in the
marketplace, notable given the spawn tail measured above.
- `plugins/context-guard/CHANGELOG.md` cites `(#1985)` for 0.4.7 and
`(#1978)` for 0.4.6, but those landed as `6b5c1b96 (#1988)` and
`9b90e351 (#1979)`. Consistent pattern, likely issue-vs-PR numbering;
unverified and not touched.

## Related

- #1988 — removed the manifest's hooks redeclaration; the reason the
manifest was re-read at HEAD rather than trusted from the report.
- #1979 — the per-invocation saving that made the report's original
timings stale and forced a re-measure.
- Inbox item
`20260730-182801-context-guard-zone-crossing-hook-times-out-100-percent`
— the consumer report.

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
kyle-sexton added a commit that referenced this pull request Aug 8, 2026
…/null (#2007)

No linked issue

Consumer report drained from the handoff inbox:
`20260730-182801-guardrails-hook-false-positives-and-ungated-commit-pr-hook`.
Two false positives were reproduced verbatim at HEAD before anything was
changed.

## 1a — a read-only `open()` was blocked

Reproduced first: `python3 -c "import json;
d=json.load(open('x.json'))"` → exit 2.

`_py_write` matched `open[[:space:]]*\(` with no write-mode
discrimination, so every inline Python `open()` read as a write.

**The discrimination boundary, stated because it is a design call and
not obvious.** A bare `open(` is no longer a write indicator at all.
`open(` counts as a write **only when an argument-position write-mode
literal occurs in the same command** — a quoted token built solely from
mode characters `[rwaxbtu+]`, containing at least one of
`w`/`a`/`x`/`+`, appearing immediately after a comma or after `mode=`.

The test is **co-occurrence, not positional, deliberately.** Bash ERE
has no lazy quantifier: a positional `open\([^)]*'w'` stops at the first
`)` and would **fail open** on a real `open(os.path.join(a,b),'w')`,
while a greedy `.*` reaches into unrelated text. This is the same
mangle-resistant co-occurrence shape the PowerShell lane already uses.
The **argument-position** requirement — rather than "a mode literal
anywhere" — is what keeps common read shapes clear:
`json.load(open('p'))['a']` has its `'a'` preceded by `[`, not by a
comma.

**Residual, in the fail-closed direction and pinned by a test:** a
read-only `open()` in a command that separately carries an
argument-position `'w'`/`'a'`/`'x'`/`'+'` literal (e.g.
`print(open('f').read(), 'a')`) still blocks. Accepted over the
alternative.

**Second residual, left alone:** a bare `pathlib` mention is still an
indicator on its own, so read-only inline Python that merely imports
`pathlib` still blocks. That indicator is what currently carries
`.write_text(` / `.write_bytes(` — `\.write[[:space:]]*\(` does not
match `.write_text(` — so narrowing it requires introducing an explicit
write-call set. Out of scope here, recorded in the CHANGELOG.

**Fixtures respelled, not relaxed — called out so it does not read as
test-fitting.** Four PowerShell-lane cases used a bare `open(` as their
stand-in write indicator to assert mention-over-block and here-string
inertness. Since `open(` is no longer an indicator, those inputs were
respelled to `open(f,'w')` so they keep testing their actual contract,
and a new case asserts that the same mention with a READ-mode open is
now allowed.

## 1b — `cat > /dev/null` was blocked

A discard is not a write. Added `_cat_devnull`, the exemption the
echo/printf lane already had.

It is **segment-scoped, not command-scoped**: a whole-command exemption
would let `cat > /dev/null && cat > real.txt` through, which is now a
pinned regression floor. The `cat` and echo/printf scans now share one
splitter — the segmentation block was extracted from
`producer_redirect_bypass` into `normalize_segments` (called once, sets
`NORMALIZED_SEGMENTS`) so the two lanes cannot drift on escaped
separators or the `2>&1` fd-dup sentinel. Quoted spellings (`cat >
"/dev/null"`, `cat > /dev/"null"`) fall out of the existing
redirect-operand handling in `strip_literals`; asserted.

## 2 — `flag-commit-pr-skill-bypass` timeout 10 → 60

It was the only guardrails hook not at 60, against five
`Bash|PowerShell` siblings that are. Per the hooks page fetched this
session (<https://code.claude.com/docs/en/hooks>):

> Seconds before canceling. Defaults: 600 for `command`, `http`, and
`mcp_tool`; 30 for `prompt`; 60 for `agent`.

Nothing pushes a `PreToolUse` hook to 10 — the 10 was authored here.
Same page, on why a consumer cannot work around it locally:

> Hook entries merge across settings levels rather than replacing each
other: user, project, and local settings add their own hooks without
removing managed ones, and the `disableAllHooks` setting can't disable
managed hooks from outside managed settings.

## 3a — `hook::jq_fields` adopted by `block-dangerous-git` and
`block-no-verify`

Two `jq` spawns collapsed to one per invocation; first adopters in the
fleet. Failure semantics are unchanged: rc 1 from the helper exits 0
exactly as the old empty-`COMMAND` skip did, after `hook::require_jq`
has already surfaced the degraded state. The cross-hook
`require-jq-notice-isolation` contract still passes over both adopters.

## 3b — `cli-flag-verify` global-flag false positive

**The obvious fix was a chain-fallback to top-level `--help`, and it was
measured and rejected:** `npm --help` does not list `--prefix` either
(`verify-cli-flag.sh npm --prefix` → rc 1), so it would not have closed
the repro.

Root cause is structural. `npm --help` states:

> Specify configs in the ini-formatted file … or on the command line
via: `npm <command> --key=value`

Every config key is a flag on every subcommand, so per-subcommand help
is non-exhaustive **by design**, and the authoritative list (`npm config
ls -l`) prints `prefix = "…"`, not `--prefix`, so no generic `--help`
parser can consume it. `npm` therefore joins `git` and `npx` in the
exclusion, on the rationale already recorded in that file. Consumers
re-add it via `cli_flag_verify_bins`.

## Verification

| Check | Result |
|---|---|
| `block-hook-bypass.test.sh` | PASS=233 FAIL=0 |
| `block-dangerous-git.test.sh` | PASS=329 FAIL=0 |
| `block-no-verify.test.sh` | PASS=120 FAIL=0 |
| `cli-flag-verify.test.sh` | PASS=52 FAIL=0 |
| `require-jq-notice-isolation.test.sh` | PASS=2 FAIL=0 |
| `shellcheck` (6 changed files) | clean |
| `shfmt -d` (5 of 6) | clean |
| `check-shell-portability.sh --paths` (6 files) | No unexcused GNU-only
constructs |
| `check-silent-skips.sh` | No silent prerequisite skips found |
| `markdownlint-cli2` | 0 issues |
| `check-changelog-parity.sh --check-bump origin/main` | pass |
| `git ls-files -s` changed `.sh` | all `100755` |

Suites were run strictly one at a time — their wall-clock ceilings fail
spuriously under concurrency. The first `block-hook-bypass` run surfaced
3 failures (the PowerShell `open(` fixtures); those were respelled and
the suite re-run to green twice, so the 233 tally is the shipped file.

Both directions of each fix are covered: read-only `open()` feeding
`json.load` → exit 0, `open(nested-call, 'w')` → exit 2 (the fail-open
floor), `cat > /dev/null` variants → exit 0, `cat > /dev/null && cat >
real.txt` and `cat >> real.txt 2>&1` → exit 2 (the segment-scoping
floors).

`cli-flag-verify.test.sh` fails bare `shfmt -d` — **pre-existing and
identical at `origin/main`** (the `X=$(…); RC=$?` one-liner idiom used
throughout). The diff hunks stop around line 239 and this PR's addition
starts at 264, so the added lines are shfmt-clean. `main` being green
means bare `shfmt -d` is evidently not the gate CI applies to `.test.sh`
here.

`check-orphaned-fixtures.sh` was not run locally; it exceeds a 300s
timeout on this machine. CI covers it.

`guardrails` 0.19.2 → **0.19.3**.

## Deliberately not done

- **F2 (out-of-repo plugin-data append blocked)** — not among the three
confirmed-live findings; prior triage left it unreproduced.
- **F1 suggestions 2–3 (command-gating the hook, profiling the 12–19
s)** — the report says explicitly these are *not* superseded by a
timeout fix, and that is right: a hook taking 12–19 s on every shell
call is still expensive. Scoped out here, still open.
- **`workflow-resilience-check.sh` remains at `timeout: 10`** —
`Workflow` matcher, not exercised by the report, flagged out of scope by
the item itself.
- **`cat >&2` still blocks.** An fd-dup is not a file write — the same
class as the `/dev/null` FP fixed here — but it is pre-existing and not
the reported defect. Flagged rather than folded in.

## Surfaced, not fixed

- `plugins/guardrails/README.md`'s hook table lists "PreToolUse · Bash"
for six guards whose registered matcher in `hooks.json` is
`Bash|PowerShell` — a table-wide doc/manifest mismatch, not introduced
here.
- The inbox item's front-matter title still says "guardrails 0.18.1"
while its triage audited 0.19.0 and this lands at 0.19.3; stale on
version, but its findings verified live at HEAD.

## Related

- #1979 — the shared-hook-lib work that introduced `hook::jq_fields`,
which this PR is the first to adopt.
- #2001 — the sibling timeout fix in `context-guard`, same defect class
and same fetched doc line.
- Inbox item
`20260730-182801-guardrails-hook-false-positives-and-ungated-commit-pr-hook`
— the consumer report.

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
kyle-sexton added a commit that referenced this pull request Aug 10, 2026
…or three (#2120)

No linked issue

`hook::jq_fields` landed in #1979 and got its first two adopters in
#2007
(`block-dangerous-git`, `block-no-verify`). The other **ten** guardrails
hooks were still parsing
their PreToolUse/PostToolUse payload with a separate `printf '%s'
"$INPUT" | jq -r … | tr -d '\r'`
pipeline **per field**, over the same already-buffered stdin envelope.
This converts all ten.

## Survey — what was still forking per field

Counting only `jq` **execs against the buffered payload**. `jq -n`
envelope builders, `jq -R | jq -s`
finding serializers, and jq reading a file from disk are out of scope
and untouched.

| hook | jq execs on payload, `main` | after | fields |
| --- | --- | --- | --- |
| `block-noncanonical-commit` | 3 | 1 | `command`, `cwd`, `tool_name` |
| `block-convention-violation` | 3 | 1 | `tool_name`, `command`, `cwd` |
| `hardcoded-path-check` | 3 | 1 | `tool_name`, `file_path`,
`content`/`new_string`/`new_source` |
| `secret-pattern-detection` | 3 | 1 | same as above |
| `skill-reference-verify` | 3 (Edit) / 2 (Write) | 1 | `tool_name`,
`new_string`, `replace_all` / `content` |
| `stale-path-verify` | 3 (Edit) / 2 (Write) | 1 | same as above |
| `block-hook-bypass` | 2 | 1 | `command`, `tool_name` |
| `flag-commit-pr-skill-bypass` | 2 | 1 | `command`, `tool_name` |
| `cli-flag-verify` | 2 | 1 | `tool_name`, `new_string`/`content` |
| `workflow-resilience-check` | 2 | 1 | `script`, `scriptPath` |
| `block-dangerous-git` | 1 | 1 | already converted by #2007 |
| `block-no-verify` | 1 | 1 | already converted by #2007 |

Collateral, not claimed as the headline: each old line is three process
creations
(`$( )` subshell + `jq` + `tr`), so a 3-field hook went 9 → 3 and a
2-field hook 6 → 3 — the
`tr -d '\r'` per field disappears too, because `hook::jq_fields` strips
CR shell-side.

### Deliberately NOT converted

**`hook::read_file_path`** — `cli-flag-verify`, `skill-reference-verify`
and `stale-path-verify`
each still pay one jq exec there. Folding `file_path` into the batched
call would mean either
duplicating or restructuring that helper's existence +
project-membership validation, and it lives
in the synced shared lib (`lib/hook-utils.sh` → 13 plugin copies + the
CI drift check), so the blast
radius reaches every plugin for one exec. Left alone on purpose.

**`flag-commit-pr-skill-bypass`'s `enabledPlugins` reads** (two jq calls
at L145/L155) read a
settings **file**, not the payload. Different input, not batchable here.

## How the fields were kept byte-identical

Two spots would have changed behavior under a naive conversion, and both
are handled:

1. **`.tool_name // "Bash"`** — the default moves to the shell side
(`TOOL_NAME="${HOOK_JQ_FIELDS[n]:-Bash}"`), matching
`block-dangerous-git`.
2. **`replace_all`** keeps `// false | tostring` **inside** the filter.
`hook::jq_fields` wraps every filter in `// ""`, and jq's `//` treats
the boolean `false` as
empty — so a bare `.tool_input.replace_all` returns `""` where the old
call returned `"false"`.
   Verified against all three input shapes (absent / `false` / `true`):

   ```text
value=null filter=.tool_input.replace_all new=[] old=[false]
value=null filter=.tool_input.replace_all // false | tostring
new=[false] old=[false]
value=false filter=.tool_input.replace_all new=[] old=[false]
value=false filter=.tool_input.replace_all // false | tostring
new=[false] old=[false]
value=true filter=.tool_input.replace_all new=[true] old=[true]
value=true filter=.tool_input.replace_all // false | tostring new=[true]
old=[true]
   ```

**Failure semantics are unchanged in every hook.** `hook::jq_fields … ||
exit 0` lands on exactly
the skip the old empty-field guard produced — each hook's statement
right after its first old jq call
was already `[[ -n "$X" ]] || exit 0` or a `case … *) exit 0`.
`hook::require_jq` still runs first and
still makes a missing jq visible once per session.

**One trade stated plainly.** In `hardcoded-path-check` and
`secret-pattern-detection` the per-tool
content field is now serialized in the first call, i.e. BEFORE the
file-path exclusions and the
`git check-ignore` skip that used to precede it. On a skipped write that
is one extra copy out of jq
of a payload already buffered in memory, traded for one fewer process on
every path. Process
creation, not jq's parse, is the cost centre on the host this targets.

## Measurement

**Method.** Two checkouts — arm A at `origin/main`, arm B this branch —
with the arms **interleaved
inside one loop**, alternating which runs first each iteration, so both
arms share one load sample.
Compared as **paired deltas** (`B_i − A_i`), summarized by median and
quartiles. Never "50× A, then
50× B": one instrumented fork on this host has been recorded swinging 93
ms → 3234 ms, so a single
sequential before/after pair proves nothing.

**Machine load — every number below was taken under load, and is
labelled as such.** This box runs
several agents concurrently. Snapshot during the runs:
`cpu_pct_avg=20.3`, `procs_total=405`,
`bash_procs=18`, `free_mem_gb=31.4`. Windows 11, Git Bash (`GNU bash
5.3.15 x86_64-pc-cygwin`),
`jq-1.8.2`. Load is why absolute per-arm times below run into seconds;
it is also why the
**medians are inflated relative to a quiet box** and the conservative
statistics are the headline.

**Headline, conservative — p75 (least-favourable quartile) of the paired
deltas:**

| conversion shape | p75 | median | min-of-arms floor | NEW faster in |
| --- | --- | --- | --- | --- |
| 3 fields → 1 (run 1, N=100) | **-404 ms** | -1033 ms | — | 91/100 |
| 3 fields → 1 (run 2, N=100) | **-449 ms** | -991 ms | -394 ms | 95/100
|
| 2 fields → 1 (N=100) | **-194 ms** | -274 ms | -192 ms | 87/100 |

Run 1 was reproduced by run 2 to within 45 ms at p75 and 42 ms at the
median — the point the task
brief makes about a "PASS=154 FAIL=0" claim from a single run that did
not reproduce. Run 1's raw
samples were not retained to a file (its summary line is quoted above);
**runs 2 and the 2-field run
have every sample below**, and either alone carries the claim.

The p75 and the independently-computed floor (fastest observed A minus
fastest observed B, i.e. the
least-contended sample of each arm) agree to within 10 ms in both
shapes. Two conservative estimators
converging is the strongest claim here; the medians are the same effect
amplified by contention.

**End-to-end, whole-hook** — `block-noncanonical-commit.sh` invoked as a
process, N=60 interleaved:
**median paired delta -687 ms**, range -13039 ms to +11774 ms. Reported
deliberately even though it
is noisier and *smaller* than the isolated 3-field median: the parse
block cannot recover more than
the whole hook does, and omitting the weaker own-number is what makes a
stronger one look selected.

**Against the prior model.** A previous session's model predicted ~280
ms recovered and the handoff
recorded "the measured-versus-model gap says expect LESS." Stated
plainly: the conservative 2-field
number (**-194 ms**) is **under** that model, and the conservative
3-field number (**-404 ms**) is
**over** it. The model was a single figure for a range of shapes.

Every sample is in the collapsed sections below.

## Behavior verification

### Payload-level differential vs `origin/main` — 62/62 identical

Issue #1403 records that the previous extraction attempt (#1385)
regressed on **multi-line command
values** — four suites failed, all on multi-line payloads. That is the
exact risk class for this
change, so it is tested directly: the same payload fed to the
`origin/main` copy and the converted
copy of each hook, requiring **identical exit code, identical stdout and
identical stderr**.

Cases: plain command, backslash-newline continuation (`git commit
--no\<newline>verify`), multi-line
`-m` body, escaped quotes, embedded tab, PowerShell here-string,
stdout-redirect write, `gh pr
create`, empty command; Write/Edit/NotebookEdit multi-line content,
unmatched tool, empty content;
`replace_all` true/false; Workflow inline-script / `scriptPath`-only /
neither.

**Result: `DIFFERENTIAL PASS=62 FAIL=0`.** This is a deterministic
comparison of outputs, not a
timing measurement, so it does not carry the reproducibility caveat the
numbers above do.

### Contract suites — run STRICTLY one at a time

Their wall-clock assertions corrupt under contention, so the runner is
serial by construction.

**Re-run after the NUL fix** (this is the authoritative set; the pre-fix
tallies below it are kept
for the record). `lib/hook-utils.test.sh` is included because that is
where the helper and its new
regression case live.

```text
lib/hook-utils.test.sh               rc=0   PASS=155 FAIL=0
secret-pattern-detection             rc=0   PASS=44  FAIL=0
hardcoded-path-check                 rc=0   PASS=86  FAIL=0
skill-reference-verify               rc=0   PASS=96  FAIL=0
stale-path-verify                    rc=0   PASS=87  FAIL=0
block-noncanonical-commit            rc=0   passed: 202 failed: 0
```

`secret-pattern-detection` and `hardcoded-path-check` each gained
exactly **+2** assertions — the two
added by the NUL regression case in each file. That is visible directly
rather than by subtraction:
under mutation (the `split | join` reverted, tests kept) the same trees
report `PASS=42 FAIL=2` and
`PASS=155 → 154 FAIL=1`, failing on precisely those assertions and
nothing else.
`skill-reference-verify` reads higher than the pre-fix table below
because `main` was merged in
between; no case was added to it here.

**On the `block-noncanonical-commit` promise.** This description
previously said that suite "was
still running when this PR was opened" and that "its result will be
posted as a comment." No such
comment was ever posted, so it is settled here instead: the suite was
re-run after the NUL fix and
passes, **202/0**. Worth stating because it nearly went into this
description as a false negative —
that suite reports `passed: N failed: N`, not the `PASS=N FAIL=N` every
other guardrails suite uses,
so the first run's output filter matched nothing and the run looked like
an abort. It was not; the
filter was wrong. The tally above is from an unfiltered re-run.

**Not re-run, and why.** The remaining guardrails suites
(`block-hook-bypass`,
`block-convention-violation`, `flag-commit-pr-skill-bypass`,
`cli-flag-verify`,
`workflow-resilience-check`, plus the two already-converted git guards)
and the 15
non-guardrails plugins were not re-run for the NUL fix. The strip is a
no-op for any
payload without a NUL, and
`grep -rln 'hook::jq_fields' plugins/*/hooks/*.sh` returns guardrails
files only — the other 15
plugins carry the lib text and a version bump but have no call site.
Their pre-fix tallies stand.

**Pre-fix tallies** (the original `hook::jq_fields` conversion, before
the NUL fix):

```text
workflow-resilience-check            rc=0   PASS=16 FAIL=0               35s
block-convention-violation           rc=0   PASS=31 FAIL=0               320s
secret-pattern-detection             rc=0   PASS=42 FAIL=0               314s
flag-commit-pr-skill-bypass          rc=0   PASS=29 FAIL=0               303s
cli-flag-verify                      rc=0   PASS=52 FAIL=0               549s
skill-reference-verify               rc=0   PASS=68 FAIL=0               900s
hardcoded-path-check                 rc=0   PASS=84 FAIL=0               1501s
stale-path-verify                    rc=0   PASS=87 FAIL=0               1600s
stale-path-verify                    rc=0   PASS=87 FAIL=0               1571s
block-hook-bypass                    rc=0   PASS=260 FAIL=0              2278s
```

One caveat from that run, stated rather than hidden:
`hardcoded-path-check` and `stale-path-verify`
each show **two** lines because a background runner believed killed had
survived, so a second copy
of each ran concurrently. Both copies of both suites returned the same
tally. Contention can only
produce spurious *failures* in a wall-clock assertion, never a spurious
pass, so a green result
under contention is the stronger reading. (The first
`hardcoded-path-check` line's tally column is a
`grep` artifact — its log ends `PASS=84 FAIL=0`.)

<details><summary>Every sample — isolated parse block, 3 fields to 1
(N=100)</summary>

```text
payload=payload-ls.json iterations=100 fields=3
sample old_ms new_ms delta_ms
1 704 303 -401
2 1247 262 -985
3 648 223 -425
4 616 265 -351
5 650 252 -398
6 640 758 118
7 653 253 -400
8 627 223 -404
9 633 237 -396
10 1506 423 -1083
11 1664 481 -1183
12 1739 462 -1277
13 2018 1015 -1003
14 1852 796 -1056
15 838 958 120
16 1212 265 -947
17 704 260 -444
18 673 263 -410
19 722 258 -464
20 849 331 -518
21 1429 357 -1072
22 891 869 -22
23 1345 367 -978
24 1318 322 -996
25 663 807 144
26 1192 324 -868
27 1308 887 -421
28 1827 816 -1011
29 3450 782 -2668
30 6323 1474 -4849
31 8060 4150 -3910
32 7464 1999 -5465
33 3327 1541 -1786
34 1791 337 -1454
35 4003 869 -3134
36 11638 1056 -10582
37 4819 2220 -2599
38 5149 885 -4264
39 1270 858 -412
40 1215 799 -416
41 7450 1903 -5547
42 7951 1636 -6315
43 4620 4181 -439
44 4524 1083 -3441
45 2888 1642 -1246
46 2639 824 -1815
47 1870 276 -1594
48 1249 831 -418
49 1793 793 -1000
50 2296 251 -2045
51 2730 757 -1973
52 2241 1294 -947
53 5852 2494 -3358
54 5935 2120 -3815
55 3870 1551 -2319
56 5200 820 -4380
57 1205 265 -940
58 2331 242 -2089
59 4141 1333 -2808
60 1943 305 -1638
61 1910 328 -1582
62 1288 280 -1008
63 1252 266 -986
64 1202 271 -931
65 2896 253 -2643
66 4351 935 -3416
67 6699 894 -5805
68 2085 843 -1242
69 1278 291 -987
70 1218 251 -967
71 1220 258 -962
72 636 238 -398
73 1188 249 -939
74 674 749 75
75 617 223 -394
76 1170 253 -917
77 1751 222 -1529
78 4921 820 -4101
79 5530 796 -4734
80 1895 808 -1087
81 1953 838 -1115
82 1794 251 -1543
83 1193 295 -898
84 1723 809 -914
85 1713 269 -1444
86 1194 255 -939
87 652 769 117
88 1158 808 -350
89 4942 1893 -3049
90 4702 3793 -909
91 1890 1441 -449
92 1167 257 -910
93 1178 269 -909
94 649 269 -380
95 704 249 -455
96 1194 284 -910
97 1188 759 -429
98 1139 250 -889
99 2270 256 -2014
100 7879 1671 -6208
median_paired_delta_ms=-991 p25=-2014 p75=-449 (negative = NEW is faster)
iterations_where_NEW_faster=95/100
```

</details>

<details><summary>Every sample — isolated parse block, 2 fields to 1
(N=100)</summary>

```text
payload=payload-ls.json iterations=100 fields=2
sample old_ms new_ms delta_ms
1 2995 2056 -939
2 1664 268 -1396
3 1030 300 -730
4 491 285 -206
5 963 266 -697
6 484 793 309
7 427 253 -174
8 494 251 -243
9 432 741 309
10 414 249 -165
11 448 252 -196
12 434 230 -204
13 472 222 -250
14 949 225 -724
15 436 235 -201
16 441 235 -206
17 444 234 -210
18 444 250 -194
19 432 239 -193
20 446 234 -212
21 1021 251 -770
22 447 253 -194
23 485 256 -229
24 459 272 -187
25 1224 788 -436
26 1176 436 -740
27 6119 984 -5135
28 2840 1012 -1828
29 1183 1052 -131
30 552 287 -265
31 531 811 280
32 523 267 -256
33 479 808 329
34 463 266 -197
35 1016 269 -747
36 475 250 -225
37 477 269 -208
38 997 256 -741
39 469 259 -210
40 508 267 -241
41 1178 838 -340
42 1726 877 -849
43 5537 3620 -1917
44 4843 1005 -3838
45 2414 1547 -867
46 1708 271 -1437
47 1003 312 -691
48 450 281 -169
49 499 804 305
50 530 866 336
51 512 273 -239
52 1002 252 -750
53 468 295 -173
54 2921 926 -1995
55 1729 938 -791
56 1736 2165 429
57 1086 834 -252
58 484 957 473
59 1022 281 -741
60 1030 857 -173
61 2762 787 -1975
62 10295 1980 -8315
63 4781 3332 -1449
64 2104 968 -1136
65 1226 902 -324
66 1143 860 -283
67 1061 288 -773
68 1084 834 -250
69 1038 287 -751
70 466 259 -207
71 1215 815 -400
72 2466 4496 2030
73 3997 3347 -650
74 5594 1707 -3887
75 2973 2085 -888
76 1729 1418 -311
77 2138 1415 -723
78 978 252 -726
79 940 741 -199
80 973 261 -712
81 982 253 -729
82 1011 247 -764
83 456 794 338
84 1016 299 -717
85 1010 272 -738
86 1007 1324 317
87 2535 2307 -228
88 1007 266 -741
89 1013 822 -191
90 1011 831 -180
91 919 269 -650
92 956 241 -715
93 946 758 -188
94 1002 1804 802
95 2150 831 -1319
96 1590 1345 -245
97 2952 3790 838
98 6564 5253 -1311
      0 [main] bash 433945 dofork: child -1 - forked process 52068 died unexpectedly, retry 0, exit code 0xC0000142, errno 11
parsebench.sh: fork: retry: Resource temporarily unavailable
99 14516 9057 -5459
100 3374 1022 -2352
median_paired_delta_ms=-274 p25=-750 p75=-194 (negative = NEW is faster)
iterations_where_NEW_faster=87/100
```

</details>

<details><summary>Payload-level differential vs origin/main — all 62
cases</summary>

```text
ok:   block-hook-bypass  plain-ls  (rc=0)
ok:   block-hook-bypass  backslash-newline-continuation  (rc=0)
ok:   block-hook-bypass  multiline-m  (rc=0)
ok:   block-hook-bypass  escaped-quotes  (rc=0)
ok:   block-hook-bypass  embedded-tab  (rc=0)
ok:   block-hook-bypass  ps-herestring  (rc=0)
ok:   block-hook-bypass  redirect-write  (rc=2)
ok:   block-hook-bypass  gh-pr-create  (rc=0)
ok:   block-hook-bypass  empty-command  (rc=0)
ok:   block-noncanonical-commit  plain-ls  (rc=0)
ok:   block-noncanonical-commit  backslash-newline-continuation  (rc=0)
ok:   block-noncanonical-commit  multiline-m  (rc=2)
ok:   block-noncanonical-commit  escaped-quotes  (rc=0)
ok:   block-noncanonical-commit  embedded-tab  (rc=0)
ok:   block-noncanonical-commit  ps-herestring  (rc=2)
ok:   block-noncanonical-commit  redirect-write  (rc=0)
ok:   block-noncanonical-commit  gh-pr-create  (rc=0)
ok:   block-noncanonical-commit  empty-command  (rc=0)
ok:   block-convention-violation  plain-ls  (rc=0)
ok:   block-convention-violation  backslash-newline-continuation  (rc=0)
ok:   block-convention-violation  multiline-m  (rc=0)
ok:   block-convention-violation  escaped-quotes  (rc=0)
ok:   block-convention-violation  embedded-tab  (rc=0)
ok:   block-convention-violation  ps-herestring  (rc=0)
ok:   block-convention-violation  redirect-write  (rc=0)
ok:   block-convention-violation  gh-pr-create  (rc=0)
ok:   block-convention-violation  empty-command  (rc=0)
ok:   flag-commit-pr-skill-bypass  plain-ls  (rc=0)
ok:   flag-commit-pr-skill-bypass  backslash-newline-continuation  (rc=0)
ok:   flag-commit-pr-skill-bypass  multiline-m  (rc=0)
ok:   flag-commit-pr-skill-bypass  escaped-quotes  (rc=0)
ok:   flag-commit-pr-skill-bypass  embedded-tab  (rc=0)
ok:   flag-commit-pr-skill-bypass  ps-herestring  (rc=0)
ok:   flag-commit-pr-skill-bypass  redirect-write  (rc=0)
ok:   flag-commit-pr-skill-bypass  gh-pr-create  (rc=0)
ok:   flag-commit-pr-skill-bypass  empty-command  (rc=0)
ok:   hardcoded-path-check  write-multiline  (rc=0)
ok:   hardcoded-path-check  edit-multiline  (rc=0)
ok:   hardcoded-path-check  notebook-multiline  (rc=0)
ok:   hardcoded-path-check  unmatched-tool  (rc=0)
ok:   hardcoded-path-check  empty-content  (rc=0)
ok:   secret-pattern-detection  write-multiline  (rc=0)
ok:   secret-pattern-detection  edit-multiline  (rc=0)
ok:   secret-pattern-detection  notebook-multiline  (rc=0)
ok:   secret-pattern-detection  unmatched-tool  (rc=0)
ok:   secret-pattern-detection  empty-content  (rc=0)
ok:   cli-flag-verify  write-multiline  (rc=0)
ok:   cli-flag-verify  edit-multiline  (rc=0)
ok:   cli-flag-verify  unmatched-tool  (rc=0)
ok:   skill-reference-verify  write-multiline  (rc=0)
ok:   skill-reference-verify  edit-multiline  (rc=0)
ok:   skill-reference-verify  unmatched-tool  (rc=0)
ok:   stale-path-verify  write-multiline  (rc=0)
ok:   stale-path-verify  edit-multiline  (rc=0)
ok:   stale-path-verify  unmatched-tool  (rc=0)
ok:   skill-reference-verify  replace_all=true  (rc=0)
ok:   stale-path-verify  replace_all=true  (rc=0)
ok:   skill-reference-verify  replace_all=false  (rc=0)
ok:   stale-path-verify  replace_all=false  (rc=0)
ok:   workflow-resilience-check  workflow-inline-multiline  (rc=0)
ok:   workflow-resilience-check  workflow-scriptpath-only  (rc=0)
ok:   workflow-resilience-check  workflow-neither  (rc=0)
DIFFERENTIAL PASS=62 FAIL=0
```

</details>

## Review follow-up — the NUL fail-open (P1)

Review found a **fail-open this PR introduced**, and it reproduces.
`hook::jq_fields` delimits its
batched fields with a NUL byte. JSON may legitimately encode a NUL
inside a string, and a
`Write`/`Edit`/`NotebookEdit` `content` field is exactly where one
arrives — jq emitted the raw
byte, the read split that value in two, the cardinality check saw one
value too many, the helper
returned non-zero, and the hook's `|| exit 0` skipped detection
**entirely**. The per-field command
substitution this PR replaced discarded the NUL and scanned the rest, so
this was a regression, not
a pre-existing gap.

**Reproduction** — one payload, `tool_input.content` = `harmless first
line` + NUL +
`aws_key = AKIA…`, fed to `secret-pattern-detection.sh` at both refs:

| arm | exit | note |
| --- | --- | --- |
| `origin/main` | **2** (blocked) | stderr also carries bash's own
`warning: command substitution: ignored null byte in input` — the old
path saw the NUL, dropped it, and scanned the rest |
| this branch, before the fix | **0** (allowed) | secret passes
unblocked |
| this branch, after the fix | **2** (blocked) | |

**The framing scheme, and why this one.** Each value is now NUL-stripped
**inside the jq filter**
(`split("<NUL>") | join("")`, the 1-arity plain-string split — not
`gsub`, which would put a NUL
inside an Oniguruma pattern), so the delimiter provably cannot occur in
a value. The three options
weighed:

- **Length-prefix framing** is collision-proof but needs `read -N` (Bash
4.1+); this lib supports
  3.2+ and says so.
- **`@base64` / `@json` encoding** costs a decode per field shell-side —
a spawn each, which undoes
  the whole PR — and still cannot deliver the byte, see below.
- **Stripping** is not the lesser option, it is the **only
representable** one: a bash variable
cannot hold a NUL byte, so *no* scheme delivers one into
`HOOK_JQ_FIELDS`. It is also byte-for-byte
what the pre-conversion `$( )` did. Content **after** the NUL is
returned and scanned exactly as
  before.

**On "rather than failing open".** The mismatch policy is unchanged and
deliberately so: `return 1`
+ the caller's `|| exit 0` is the documented jq-absent fail-open
(`hook::require_jq` makes it visible
once per session) and matches the pre-conversion empty-field guard. What
changed is that the
**cause** of the spurious mismatch is gone — a mid-stream jq filter
error is now the only way to
trip it, exactly as on `main`.

**Regression cases** (all three go red on reverting the strip, green
with it):

- `lib/hook-utils.test.sh` — a NUL-bearing value keeps its slot and its
post-NUL content.
  Mutated: `PASS=154 FAIL=1`. Fixed: `PASS=155 FAIL=0`.
- `plugins/guardrails/hooks/secret-pattern-detection.test.sh` — a secret
**after** a NUL exits 2.
- `plugins/guardrails/hooks/hardcoded-path-check.test.sh` — a machine
path **after** a NUL exits 2.

Payloads are built with jq's `[0] | implode`, so no literal escape
sequence for the byte lives in
any test file's source.

**Blast radius.** The fix is in the synced shared lib, so
`scripts/sync-hook-utils.sh` ran and all
16 carrying plugins take a patch bump with an identical `### Fixed`
entry — the mechanism #1979 used
for the same file. `guardrails` additionally documents the guard-level
regression and the comment
softening below.

### Review nits — `replace_all` comment (both files)

`skill-reference-verify.sh` and `stale-path-verify.sh` now say the `//
false | tostring` is kept for
parity with the pre-conversion output, **not** because a branch depends
on it: every consumer tests
`== "true"`, which `""` and `"false"` fail alike. Comment only; behavior
unchanged.

## Checks run locally

- `shellcheck -x` clean on every changed `.sh` file (the ten hooks, the
shared lib, the
  three test files).
- `shfmt -d` clean on the same set.
- `npx --no-install markdownlint-cli2 plugins/guardrails/CHANGELOG.md` —
0 issues.
- `bash scripts/check-changelog-parity.sh --check-bump origin/main` —
passes
(`guardrails` `0.22.0` → `0.22.2` plus a patch bump on all 15 other
carrying plugins,
  each with its own new `## [<version>]` entry).
- `bash scripts/sync-hook-utils.sh --check` — all 16 plugin copies match
`lib/hook-utils.sh`;
  `--check-bump origin/main` — every carrying plugin bumped.
- No `printf '%s' "$INPUT" | jq` remains anywhere under
`plugins/guardrails/hooks/`.

## Related

- #2007 — introduced this helper's first two adopters
(`block-dangerous-git`, `block-no-verify`) and
set the pattern this PR follows; the `// "Bash"` shell-side default is
copied from it verbatim.
- #1979 — added `hook::jq_fields` to `lib/hook-utils.sh`. The review
follow-up above **does**
edit that shared lib (the NUL strip), so `scripts/sync-hook-utils.sh`
ran, all 16 plugin
copies were re-synced, and every carrying plugin took a patch bump — the
same mechanism
#1979 itself used. An earlier revision of this description claimed no
shared-lib edit; that
  is no longer true and is corrected here.
- #1403 — "recover the PreToolUse spawn-reduction work from the closed
#1385". This PR discharges
**one** part of it: the `hook::jq_fields` conversion across the
remaining guards, verified against
the multi-line regression class that sank #1385 (precondition 1,
differential above). It does
**not** discharge: `strip_quoted_spans` in
`flag-commit-pr-skill-bypass`, the deferred
`git rev-parse --is-inside-work-tree` probe in `hardcoded-path-check`,
committed multi-line
regression **cases in the suites** (precondition 3 — the differential
here is a working harness,
not committed test coverage), or the unreviewed
`hook_latency_report.py`. Left open.
- #1414 — CI has no Windows runner, so a green `plugin-gate` carries no
signal for a change whose
whole point is MSYS fork-emulation cost. That is why this PR carries
local Windows measurements
  and a payload-level differential rather than leaning on CI.

## Reproducing the numbers

The harnesses are scratch scripts, not committed. To re-derive: clone
`origin/main` and this branch
side by side, then for each iteration time one invocation of each arm
back to back (alternating
order), and take the median/p75 of `B_i − A_i`. State the machine load
with any number produced —
on a quiet box the absolute times will be far lower than those above,
and the recovery should land
nearer the min-of-arms floor (-394 ms for 3 fields, -192 ms for 2) than
the loaded medians.

---------

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
kyle-sexton added a commit that referenced this pull request Aug 10, 2026
…ing guards (#2135)

Closes #2122

## Update — `main` moved under this PR, and the disposition changed with
it

**#2120 merged (`fd075c27`), and it fixed the same function with the
opposite value disposition:
it STRIPS every NUL out of a value where this branch TRUNCATED at the
first one.** The PR went
`DIRTY`. Resolved by merging `origin/main` into the branch — never a
rebase, since force-push is
blocked here twice over.

**The resolution keeps `main`'s strip and this branch's flag plus
fail-closed guards.** That is
additive over `main` rather than a choice between the two sides, and it
is what this body already
argued for in its own words: the disposition is *immaterial for this
PR's own two callers*, which
refuse on the flag before reading a value, while `main` now carries the
ten scanner-class callers
#2120 converted, none of which consults the flag. Truncating would have
hidden a credential placed
after a NUL from `secret-pattern-detection` and `hardcoded-path-check`.
**Everything below that
says "truncate" describes the pre-merge branch; the shipped behaviour is
strip + flag.**

### The textual merge git produced was silently fatal, and was not taken

git auto-merged the function body into a hybrid carrying BOTH `main`'s
per-filter
`split("\u0000") | join("")` **and** this branch's array-level `explode
| .[0:(index(0) // length)]
| implode`. Strip runs first, so `index(0)` looked at a value with no
NUL left in it and **the flag
read `0` on every payload** — the guards would never have fired, with no
conflict marker and no test
of the pre-merge branch able to see it. The flag is now computed from
the untouched values with the
strip applied after, and both the library and the guard comments say the
ordering is load-bearing,
because it is exactly what the next textual merge will get wrong again.

### Why the flag and the guards are still needed after #2120

#2120 closed the fail-open for the CONTENT guards. It did not close the
COMMAND guards: stripping
SPLICES the bytes either side of the NUL into a token the payload never
carried contiguously, and
the guards then match against that token. Re-measured at the hook
boundary, `origin/main` at
`fd075c27` versus this tree, same script, same host, on fixtures whose
NUL is a real byte — verified
by decoding each fixture and counting the byte (`jq -j
.tool_input.command | tr -dc '\u0000' | wc -c` =
1) rather than trusting that the escape survived construction:

| payload | `main` | this change |
| --- | --- | --- |
| `git commit --no-verify<NUL>x` | **0 ALLOWED** | **2 blocked** |
| `git push --force<NUL>x` | **0 ALLOWED** | **2 blocked** |
| a lone NUL | **0 ALLOWED** | **2 blocked** |
| a trailing NUL | **0 ALLOWED** | **2 blocked** |
| `git commit --no-veri<NUL>fy` | 2 blocked | 2 blocked |
| clean `--no-verify` | 2 | 2 |
| clean `--force` | 2 | 2 |
| harmless (`git status`) | 0 | 0 |

Identical on both guards. **The fifth row is stated, not counted:** the
splice happens to reassemble
a real `--no-verify` there, so `main` already blocks it and it evidences
nothing about this change.
The live rows are the first four, and the first two are the ones that
matter — a real `--no-verify`
and a real `--force` that `main` waves through. No clean command changed
verdict in either
direction.

### Tests re-pointed rather than deleted

Every assertion this branch wrote against truncation was measuring a
value the helper no longer
produces, so each was rewritten for strip and two new cases were added:
the splice
(`--no-verify<NUL>x` -> the single token `--no-verifyx`), and an ALL-NUL
value, which strips to
empty — that case, and not a leading NUL, is the real reason both guards
consult the flag ahead of
their empty-command skip. The guard suites keep every NUL row at exit 2;
the verdict never depended
on the disposition, only its justification did, and one mislabelled row
was corrected accordingly.

### Conflicts and versions

- `lib/hook-utils.sh` — header comment and jq program, resolved by hand.
- The 16 vendored copies were **regenerated with
`scripts/sync-hook-utils.sh`**, not hand-resolved;
  `--check` reports 16/16 byte-identical.
- 16 CHANGELOGs where both sides claimed the same version: this branch's
entry moves up one patch
  above `main`'s and is rewritten for the resolved design.
- **All 16 `plugin.json` files had auto-merged to `main`'s number,
leaving no bump at all** — no
conflict, only `--check-bump` catches it, exactly the trap flagged
below. Re-bumped:
  `guardrails 0.23.1 -> 0.23.2`, `markdown-format 0.11.2 -> 0.11.3`,
  `source-control 0.51.2 -> 0.51.3`, patch bumps for the other 13.
- **Coordination with #2130:** it also bumps `markdown-format` to
`0.11.3`. Whichever merges second
  must re-bump.

### `main` moved twice more: three merges, and one of them was silently
lossy

`main` landed #2147, then #2140 and #2149, while this PR sat. Three
merge passes, no rebase at any
point. Second pass: #2147 took `guardrails` to `0.24.0` and edited
`block-dangerous-git.sh`, which this branch also edits — resolved by
keeping main's three-field
`hook::jq_fields "$INPUT" '.tool_input.command' '.cwd' '.tool_name'`
call verbatim and appending this
branch's NUL block after it. Third pass: one changelog conflict on
`source-control`. Every plugin
manifest had auto-merged to main's number with no bump on **both**
passes.

**The second pass exposed a defect this branch had introduced, and it is
worth reading even if you
skip the rest.** An earlier commit here accidentally wrote a **real NUL
byte** into
`plugins/guardrails/CHANGELOG.md` — a `\u0000` that was meant to be
literal text in a prose
description of the fixtures. git classifies any file containing a NUL as
**binary**, so the textual
three-way merge never ran on that changelog: it kept ours wholesale and
**silently discarded main's
entire `0.24.0` section**, with no conflict marker and nothing in `git
status` to distinguish it from
a file that merged cleanly. It was caught by counting NUL bytes across
the touched files, not by
reading the diff. The byte is gone, the section is restored, and the
changelog's `0.24.1` entry now
sits above main's `0.24.0`.

That is a mistake this PR made, not a pre-existing one, and it is
reported rather than quietly fixed
because the failure mode generalises: **a NUL in a tracked text file
turns every future merge of that
file into a silent take-ours.** In a repository whose CHANGELOGs are the
merge-conflict surface for
every shared-library change, that is worth knowing independently of this
fix.

### Incidental, and relevant to the "what I could NOT verify" list below

While posting a review reply, the **harness itself refused a tool call**
whose `command` field
carried a stray control character, with `command contains control
characters that would be hidden in
the approval dialog`. That is a live observation of the validation the
list below names as unverified
— it fires, and it fires on the `command` field. It is **not** the
discriminating probe: it says
nothing about whether that validation runs before or after PreToolUse
hooks, and nothing about
whether the rejected class includes NUL specifically rather than the
control characters it does
cover. Recorded as an observation, not as evidence that the guards are
unreachable. Nothing in this
change leans on it in either direction.

### Gates re-run after the merge

`sync-hook-utils.sh --check` (16/16) - `sync-hook-utils.sh --check-bump
origin/main` -
`check-changelog-parity.sh --check` / `--check-bump origin/main` /
`--check-order` -
`shellcheck -x` with **no severity floor** on `lib/hook-utils.sh`, the
`bash-format` vendored copy,
both guards and all three test files (rc 0 — this is what the two open
review threads reported
failing; the jq-variable spelling they flagged is gone from the current
program text) -
`shfmt -d -i 2` (rc 0).

Suite results after the merge are in the thread below.

## The defect

`hook::jq_fields` frames its fields with a NUL delimiter drawn from the
same byte space as the
values it separates. A JSON NUL escape inside a value splits that value
in two, the cardinality
check `((${#values[@]} == $#)) || return 1` fires, and both real callers
spell that `|| exit 0` —
a PreToolUse **ALLOW**, emitted with no diagnostic of any kind.

One correction to the issue's mechanism, because it moves where the fix
belongs. The collision is
**reliably detected**, not intermittently: every NUL adds exactly one
record, so the count is always
`N + k` for `k >= 1` and the check never misses. The defect therefore
never lived in the library's
return value. It lives in **one exit path serving two conditions with
opposite correct responses** —
"jq is absent or cannot parse this" (where allowing is the documented,
deliberate behaviour) and
"this payload carries a NUL" (where allowing is wrong). Separating those
two is the fix.

## Design

**jq truncates each value at its first NUL and reports the fact; the
caller owns the verdict.**

- `lib/hook-utils.sh` — each filter becomes `... | explode |
.[0:(index(0) // length)] | implode`.
The separator then cannot occur inside a value, so the record count no
longer depends on what a
  parseable payload holds.
- A leading record carries the NUL flag, computed from the untruncated
values and emitted by the
**same** jq program, so reporting it costs no second spawn. It surfaces
as `HOOK_JQ_FIELDS_NUL`,
assigned in the same unconditional block that resets `HOOK_JQ_FIELDS` —
above all three return
paths, so no early return can leak a stale `1`, which in a guard would
mean blocking a clean
  payload on the strength of an earlier one.
- `block-no-verify.sh` and `block-dangerous-git.sh` fail **CLOSED** on
that flag, **before** their
empty-command skip, because the helper truncates at the first NUL and a
leading one therefore
leaves an empty value that would otherwise be waved through as "no
command".

### Why fail CLOSED, and why that argument does not depend on the
executor

**No executor-fidelity claim is made here, in either direction.** Two
behaviours were measured and
they disagree, and which of them a hook payload actually reaches has
**not been traced by anyone**:

| measured | result |
| --- | --- |
| bash parsing a command it reads (stdin, script file) | **discards**
the NUL — `echo ha<NUL>rd` prints `hard`, and `--no-verify<NUL>x`
becomes `--no-verifyx` |
| a NUL inside an argv word handed to `execve` | the string simply ends
there |
| Node v24.18.0 `child_process` — argv, `shell: true`, and `execSync` |
**refuses** outright, `ERR_INVALID_ARG_VALUE: must be a string without
null bytes`, while the same calls with a clean string run normally |

An earlier draft of this PR argued that truncation was right *because
the executor truncates*. That
was wrong — it generalised the argv case to a path that is not known to
be the one in use. **The
correct argument is that the design does not need it:** failing closed
on the flag is correct under
deletion, under truncation, and under refusal alike, so it cannot be
invalidated by tracing the path
later. That is the whole case for it. Matching the value would need the
trace; refusing does not.

### Truncate rather than delete, on grounds that appeal to no shell

Truncation never fabricates a token the payload did not carry
contiguously, and when a caller
forgets the flag it is the *content* class that degrades rather than the
command class — a matcher
sees a prefix rather than a joined token that matches nothing. **For
this PR's own two callers the
choice is immaterial: they refuse on the flag before reading a value at
all.** It is the
conservative default, not the accurate one, and the flag is the
load-bearing part.

### Why the library does not block on its own

It is sourced by 15 other plugins, formatters among them, for which
exiting 2 would be wrong; and a
sourced library calling `exit` on its caller's behalf is hidden control
flow. Policy stays with the
caller and the library only reports the fact.

### Rejected alternatives

| Alternative | Why not |
| --- | --- |
| Delete the NUL (`map(select(. != 0))`) | Fabricates contiguity the
payload did not have, and inverts which caller class degrades unsafely
when a hook forgets the flag; see above. Not rejected on executor
grounds. |
| `gsub` / `split`+`join` on a NUL | Both work on jq 1.8.2 here, but
each puts a NUL inside the jq **program** text — a regex pattern and a
string literal. A construct whose behaviour varied across jq builds
would fail EVERY payload: a universal fail-open, strictly worse than the
payload-dependent one. `explode`/`implode` use integer comparison only,
with no NUL anywhere in the program. This is a reason, not a measurement
— see the unverified list. |
| Length-prefixed framing | Needs `read -N` (bash 4.1+); this lib
supports 3.2+. |
| An explicit emitted count | Redundant once the separator is absent
from the value space. |
| Per-field `@base64` | Needs a `base64` binary; only `jq` is a
documented prerequisite. |
| `@sh` + `eval` | Puts payload-derived text through `eval`. |
| Fail closed inside the library | Impossible without the library
exiting on its caller's behalf, which is wrong for the 15 other plugins.
|

## Scope

**This is a shared-library change, and the repo's own gate makes it 55
files.**
`plugins/guardrails/hooks/hook-utils.sh` is a **vendored copy**;
`lib/hook-utils.sh` is the source of
truth. CI enforces `scripts/sync-hook-utils.sh --check` (all 16 copies
byte-identical) and
`--check-bump` (every carrying plugin bumped when the lib changes), so
editing only the guardrails
copy would fail CI. Precedent: 9b90e35, 50 files. Hence 16 vendored
copies, 16 `plugin.json` bumps
and 16 changelog entries, plus the lib, its test, the two guards, their
two test files and the
guardrails README.

**`hook::jq_field` — SINGULAR — is untouched.** It is a separate
two-line function; there is no
shared internal the two route through. `grep -rn "hook::jq_field "
--include=*.sh plugins/`, with the
vendored copies excluded, finds **22 call sites across 12 files** in
`claude-ops`, `context-guard`
and `source-control`. None of them are touched. `git diff origin/main --
lib/hook-utils.sh` mentions
`hook::jq_field` on exactly two lines, both of them the same doc-comment
cross-reference inside the
*plural* function's header ("Values are CR-stripped, as in
`hook::jq_field`"); the singular
function's own body appears nowhere in the diff. **Blast radius is
exactly the two guards.**

**No other plugin is affected by the truncation.** `grep -rn
"hook::jq_fields" --include=*.sh .`,
excluding the 16 vendored copies and `lib/hook-utils.*`, returns exactly
two call sites — both in
this PR. Every other hit across the 16 plugins is the doc comment in the
vendored library. Nothing
round-trips a value into a file, and nothing compares a length or hash
against one.

**Versions**, taken against `origin/main` at the time of the last
rebase: `guardrails 0.23.0 ->
0.23.1`, `markdown-format 0.11.1 -> 0.11.2`, `source-control 0.51.1 ->
0.51.2`, and plain patch bumps
for the other 13. Worth flagging for anyone rebasing a sibling branch:
when a plugin's version moved
on `main` mid-flight, `git` **auto-merged the manifest to main's
number**, silently leaving no bump
at all — no conflict, and only `sync-hook-utils.sh --check-bump` catches
it. That happened three
times here. #2120 is still open against the same guardrails files and
owes a re-bump.

## Two caller classes want opposite dispositions — which is why there is
a flag

This is the strongest argument for the design, and it is demonstrated
rather than theoretical.
#2120 has independently fixed the same function with the **opposite**
disposition: at its head
`9fb8383d`, `hook::jq_fields` does `... | tostring | split("<NUL>") |
join("")` — it **strips**.

Neither disposition is simply right, because the two caller classes
disagree:

| payload | under strip | under truncate |
| --- | --- | --- |
| `content: harmless<NUL>aws_secret=AKIA…` (a scanner) | secret is
joined and **scanned** | secret is cut off and **invisible** |
| `command: --no-verify<NUL>x` (a guard) | joins to `--no-verifyx`,
matches nothing, **allowed** | leaves `--no-verify`, **blocked** |

(Which of those two readings the executor would agree with is untraced,
and is not the argument —
see above. The point is only that a caller ignoring the flag degrades
unsafely in one class or the
other, depending which disposition the helper picks.)

Both halves measured. The command half is the boundary table below. The
content half I measured by
driving the helper directly, since no shipped hook reads
`.tool_input.content` through it on `main`:

```
payload: .tool_input.content = "harmless preamble<NUL>aws_secret=AKIA…"
this branch (truncate)  rc=0  flag=1  value=[harmless preamble]   credential NOT visible
468bb2d    (base)      rc=1  flag=-  value=[<none>]              credential NOT visible
```

**So yes — truncation loses post-NUL content for a scanning caller.**
Stated plainly because it is a
real consequence of this design. It is not a regression (the base loses
it too, and additionally
allows), and truncation is still the chosen default: it keeps the
*command* class safe when a caller
ignores the flag, where strip keeps the *content* class safe instead.
Strip inverts which class fails
unsafely; it does not remove the failure. Neither is chosen on executor
grounds.

**A single disposition cannot serve both callers. The flag is what
resolves it** — the helper
reports, and each caller decides: a command guard refuses outright, a
content scanner refuses the
write rather than scanning a value it knows is incomplete. Either way
the credential never lands.

### The count, measured on `9fb8383d`

**Every one of the ten hooks #2120 converts calls `hook::jq_fields`.
Zero of them consult any NUL
signal. Six own an `exit 2` verdict:**

| hook | `jq_fields` calls | flag checks | `exit 2` paths |
| --- | --- | --- | --- |
| `secret-pattern-detection` | 2 | **0** | 2 |
| `hardcoded-path-check` | 2 | **0** | 2 |
| `block-convention-violation` | 2 | **0** | 3 |
| `block-hook-bypass` | 2 | **0** | 2 |
| `block-noncanonical-commit` | 2 | **0** | 5 |
| `cli-flag-verify` | 2 | **0** | 1 |
| `skill-reference-verify` | 3 | **0** | 0 |
| `stale-path-verify` | 3 | **0** | 0 |
| `flag-commit-pr-skill-bypass` | 2 | **0** | 0 |
| `workflow-resilience-check` | 2 | **0** | 0 |

Zero flag checks is expected — the flag does not exist on their branch.
The point is what it implies
for whichever of us merges second: **merge order does not rescue it.**
This PR first, then their
rebase, and the scanning hooks receive truncated values with no flag
check. Theirs first, then this
one, and the same is true the moment strip becomes truncate. **A reader
must not conclude that this
PR makes that conversion safe. It does not.** Adding the flag checks to
those ten hooks is a
prerequisite for the conversion, not a follow-up — and it is theirs to
do, since those hooks exist in
converted form only on their branch. This PR deliberately does not touch
them.

`hardcoded-path-check.sh` is a **third** caller class worth calling out:
it reads `.tool_input.content`,
`.new_string` and `.new_source` **and** owns two `exit 2` paths, so it
is both scanner and guard.

Per-field reachability was checked separately and holds: at their head,
both
`secret-pattern-detection.sh` and `hardcoded-path-check.sh` reach `exit
2` through `.content` and
through `.new_string`. (`hardcoded-path-check.sh` returns early unless
`CLAUDE_PROJECT_DIR` is set,
so a probe without it exits 0 on every payload and looks exactly like
"not reachable".)

#2123 needs nothing — its diff introduces zero `hook::jq_fields` call
sites.

**Merge coordination:** #2120 now also edits `lib/hook-utils.sh`, so
this is a direct conflict on the
same function rather than only on the manifest and changelog. Whoever
merges second must **keep both
correctness properties** — the flag and the fail-closed guards from
here, and the scanning-caller
requirement from there — rather than resolving by taking one side of the
hunk.

## Evidence

### Hook boundary, before and after

Real hooks, payload piped on stdin, exit code read. BEFORE is a `git
archive` of `origin/main` at
`468bb2d9` — re-measured after #2123 merged, because #2123 changed
`plugins/guardrails/lib/powershell/ps-command.sh`, which both guards
source. AFTER is this branch.
Same script, same host.

| case | before | after |
| --- | --- | --- |
| clean `git push --no-verify` / `git reset --hard` | 2 | 2 |
| clean harmless (`echo hi` / `git status`) | 0 | 0 |
| trailing NUL | **0** | **2** |
| NUL splitting the flag (`--no-veri<NUL>fy`) | **0** | **2** |
| NUL then junk (`--no-verify<NUL>x`) | **0** | **2** |
| leading NUL | **0** | **2** |
| NUL in an otherwise harmless command | **0** | **2** |

Identical for both guards. No row where a clean command changed verdict.
The `<NUL>x` row is the one
that matters most: it is the payload that executes as the dangerous
command.

### The leading-NUL row blocks for the right reason

Identical truncated content, opposite verdicts, so the flag decides
rather than incidental matching:

| payload | exit |
| --- | --- |
| `"command": ""` (empty, no NUL) | 0 |
| `command` field absent entirely | 0 |
| leading NUL, truncates to empty | **2** |
| a lone NUL and nothing else | **2** |

Same on both guards.

### Test suites, same host, baseline vs branch

**Both arms ran in full**, serially, on an uncontended host: every
`*.test.sh` under
`plugins/guardrails/hooks/` plus `lib/hook-utils.test.sh` — 14 suites,
every one of them listed
below. BASELINE is the same `468bb2d9` tree used for the boundary table;
BRANCH is this tip.

| suite | baseline | branch | delta |
| --- | --- | --- | --- |
| `lib/hook-utils.test.sh` | 156 / 0 | **162 / 0** | +6 new cases |
| `block-dangerous-git.test.sh` | 341 / 0 | **346 / 0** | +5 new cases |
| `block-no-verify.test.sh` | 120 / 0 | **127 / 0** | +7 new cases |
| `block-convention-violation.test.sh` | 31 / 0 | 31 / 0 | — |
| `block-hook-bypass.test.sh` | 260 / 0 | 260 / 0 | — |
| `block-noncanonical-commit.test.sh` | 202 / 0 | 202 / 0 | — |
| `cli-flag-verify.test.sh` | 52 / 0 | 52 / 0 | — |
| `flag-commit-pr-skill-bypass.test.sh` | 29 / 0 | 29 / 0 | — |
| `hardcoded-path-check.test.sh` | 94 / 0 | 94 / 0 | — |
| `require-jq-notice-isolation.test.sh` | 2 / 0 | 2 / 0 | — |
| `secret-pattern-detection.test.sh` | 52 / 0 | 52 / 0 | — |
| `skill-reference-verify.test.sh` | 96 / 0 | 96 / 0 | — |
| `stale-path-verify.test.sh` | 87 / 0 | 87 / 0 | — |
| `workflow-resilience-check.test.sh` | 16 / 0 | 16 / 0 | — |
| **total** | **1538 / 0** | **1556 / 0** | **+18, 0 failures either
side** |

Every suite that does not exercise the new path is byte-identical across
the two arms, so the +18 is
entirely the new cases. No pre-existing failure to disambiguate.

Two of the new library tests look redundant and are not:
`HOOK_JQ_FIELDS_NUL` is checked both after
a clean payload and after an **early return**, each running a NUL
payload first, because a
single-call test cannot observe a stale flag however it is written, and
two of the three return
paths fire before any NUL could be seen.

### Other gates, all re-run after the rebase

`sync-hook-utils.sh --check` (16/16) - `sync-hook-utils.sh --check-bump
origin/main` -
`check-changelog-parity.sh --check` / `--check-bump origin/main` /
`--check-order` -
`check-silent-skips.sh` - `check-contract-clause-coverage.py` -
`check-cross-plugin-source-drift.sh --check` -
`check-hook-userconfig-argv.sh` -
`check-plugin-manifest-presence.sh` - `sync-parse-concern-value.sh
--check` -
`sync-resolve-convention-pattern.sh --check` -
`sync-standards-contract.sh --check` -
`check-skill-leaf-names.sh --check` - `check-shell-portability.sh
--paths` -
`shellcheck -x -S warning` (rc 0) - `shfmt -d -i 2` (rc 0) -
`markdownlint-cli2` (0 issues) -
`check-manifest-duplicate-keys.py`.

## What this PR does NOT fix, stated rather than implied

**A payload jq cannot parse still returns 1 and is still allowed.**
Malformed JSON, a wrongly typed
field or an empty buffer all reach the same `|| exit 0`, exactly as
before this change. Process
substitution also means jq's own exit status is never observed. That
path is untouched here and out
of scope, and the header comment now says so instead of claiming — as an
earlier draft of this very
fix did — that nothing a payload contains can reach it. That claim is
the same reasoning shape that
produced #2122, and it should not ship inside its fix.

## What I could NOT verify

- **How a command actually travels from hook payload to execution.**
Nobody traced it. Two shell
behaviours were measured and they disagree, and Node refuses NUL-bearing
strings on every shape
tried, so the command may never reach a shell parser at all. The design
is built so this does not
matter: fail-closed is right under deletion, truncation, and refusal
alike. An earlier draft of
this PR did lean on it, in one direction and then the other; both are
gone, from the body and from
  the code comments, the README and the changelog.
- **Whether the harness's control-character validation runs before or
after PreToolUse hooks**, and
**whether the class it rejects includes NUL specifically.** The
discriminating probe is
bypass-shaped and was deliberately not run. The guard that exists is
worded *"contains control
characters that would be hidden in the approval dialog"* —
approval-surface anti-spoofing, covering
`command` / `script` / `url` only, with no equivalent on `content` /
`new_string` / `file_text`. It
is an implementation detail, not a documented guarantee, and nothing
here leans on it in either
  direction.
- **Behaviour on jq builds other than 1.8.2, and on bash other than
5.3.9 (Cygwin).** The chosen
construct uses only `explode`, `implode`, `index`, array slicing and
`any` — core since jq 1.5 —
precisely to keep that risk low, but it was not executed against an
older jq. The repo's
  `hook-utils-windows` job exercises Git Bash on windows-2025 in CI.
- **Any performance claim.** The spawn count is unchanged at one, which
is structural. Measured
per-field cost of the sanitiser was below spawn noise on this host — the
no-op control benchmarked
  *slower* than all three candidates — so no number is claimed.
- **Whether a NUL payload can reach a PreToolUse hook through the
harness's own serialization.**
Being settled separately. This fix does not depend on the answer:
failing open on a parse failure
  is wrong regardless of how the parse came to fail.

## Related

- Closes #2122 — the reproduction and the shipped-hook measurements this
PR fixes.
- Refs #2120 (`perf/guardrails-jq-fields`) — converts ten further
guardrails hooks to
`hook::jq_fields` and, at `9fb8383d`, independently fixes the same
function by **stripping** NULs.
Direct conflict on `lib/hook-utils.sh`; whoever merges second must keep
both correctness
properties rather than taking one side of the hunk, and those ten hooks
need `HOOK_JQ_FIELDS_NUL`
  checks as a prerequisite. Not closed by this PR.
- Refs #2123 — merged mid-work; changed
`plugins/guardrails/lib/powershell/ps-command.sh`, which
both guards source, so the boundary table was re-measured against it.
Introduces no
  `hook::jq_fields` call site, so it needs nothing from this change.
- Refs 9b90e35 (#1979) — the precedent for a shared-lib change costing
a version bump and a
  changelog entry in every carrying plugin.

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

perf(hook-utils): cut the per-invocation subprocess spawns in the shared hook library

1 participant