perf(claude-ops): halve hook-failure-audit spawns by moving redirects off substitutions

[kyle-sexton]

Closes #3512

Summary

The Stop-event unsurfaced-failure detector (plugins/claude-ops/hooks/hook-failure-audit.sh) timed out 113 times in the #3508 window at a 21.9 s average, roughly 44x the 500 ms the hook-budget convention gives the whole always-on per-turn set.

The parent issue's stated cause is wrong for this hook. #3508 blames per-field jq forks needing a new shared helper. Shard #3520 (PR #3779) established that the real cost is redirection placement, and merged PR #3788 fixed 34 hook scripts across 17 plugins while touching lib/hook-utils.sh zero times. This shard applies the same diagnosis. No shared library is touched — the fence around lib/hook-utils.sh and its 17 synced copies (unmerged #3740, #3838) holds.

The mechanism: bash runs the command of a command substitution in the substitution's own subshell and skips the extra fork only when that command carries no redirection of its own. $(wc -c <file 2>/dev/null) is two processes for one byte count; $(cat -- file 2>/dev/null) is two for a file read; { V=$(cmd); } 2>/dev/null is one; $(<file) is zero. Those forks are invisible to bash -x, which reads command positions — which is why the campaign kept mis-attributing the cost to the exec count.

Fix

Six in-file sites, all in hook-failure-audit.sh:

Site	Before	After
Transcript size	SIZE=$(wc -c <"$TRANSCRIPT" 2>/dev/null)	wc names the file, read drops the filename column and any padding, 2>/dev/null rides a single-command group
Pre-filter, under the cap	read_window | grep -F …, where read_window was cat -- file 2>/dev/null	{ RECORDS=$(grep -F … -- "$TRANSCRIPT"); } 2>/dev/null — grep opens the transcript itself; the helper function was a second subshell on top of the substitution's own
Marker read	$(cat -- "$MARKER" 2>/dev/null)	$(<"$MARKER") — no subshell, no exec
Marker write	jq … 2>/dev/null | tr -d '\r' >>"$MARKER"	$(jq …) then in-shell ${VAR//$'\r'/} with a builtin printf append
Marker directory	mkdir -p "$MARKER_DIR" 2>/dev/null every warned turn	`[[ -d … ]]
Payload fields	two hook::jq_field calls	one hook::jq_fields call (the batching helper the library already ships, used by 10 other hooks)

The CHANGELOG entry originally said "Five sites" over that same list of six; it now says six.

Deliberately not changed, both documented in the file:

• Over the tail cap, tail | sed '1d' | grep and its 2>/dev/null on tail are untouched. A pipeline element forks either way, so hoisting the redirect onto a group would newly silence sed and grep for no saving, and merging sed into grep would change the matching semantics.
• printf '%s' "$RECORDS" | jq -cRs stays a pipeline rather than becoming a here-string, and the in-file comment now states the grounds as they actually stand. hook::jq_field in the shared library documents this hazard and refuses the here-string form for it: bash fills a here-string's pipe itself, so a payload at or above the pipe capacity can block before jq is exec'd. The trace behind that note (fix(hook-utils): read hook stdin in chunks so a large payload is not blocked #1587: 65536 bytes hung indefinitely while 65000 returned at once) comes from this repo's Windows Git Bash hosts. It does not reproduce on Linux bash 5.2 — re-checked here at 65535, 65536, 65537, 200 kB and 2 MB, each returning immediately, including under an unwritable TMPDIR. The call keeps the library's conservative form anyway rather than bet the hazard is Linux-only: the forgone saving is one fork on the warning path only, since a turn with no failure record exits before that line. Earlier revisions of this PR and of the comment asserted the deadlock as universal fact, which is more than is known.

Verification

Process counts, measured with strace -ff -qq -e trace=clone,clone3,fork,vfork,execve (-ff so no syscall line is split across an <unfinished>/<resumed> pair), successful execve only, the harness's own top-level bash <hook> exec excluded:

Path	Creations before	after	execs before	after
No failure recorded, under the tail cap (the common case)	18	9	6	4
No failure recorded, over the tail cap	19	12	7	6
A turn that warns	34	24	17	14

The remaining 9 creations on the common path are 4 execs (wc, grep, and two jq passes inside the synced hook-utils.sh) plus subshell forks inside that same fenced library. The execve drop is one batched jq and two removed helper processes — removed processes, not removed work: every input is still read and every record still classified.

No wall-clock figure is claimed. This Linux host says nothing about the Windows spawn tax the budget binds to, and on the #3508 host one process creation costs 180-2,841 ms (median 1,108 ms at 501 concurrent). The process count is the honest proxy and the README says so.

Behaviour is unchanged, proven, not asserted. This hook's whole job is surfacing failures that otherwise pass unnoticed, so a carelessly hoisted 2>/dev/null could silence exactly the diagnostic it exists to emit. Every hoisted group holds one command. The only stream now silenced that was not before is grep's own stderr, and cat's own 2>/dev/null already discarded that same stream on the same path. Pre- and post-change stdout, stderr, exit codes and marker-file contents were compared byte for byte across ten scenarios — first warn, dedup, a new failing registration re-warning while the already-warned one stays muted, all three classification branches (launch / ambiguous / completed), the tail cap, a clean transcript, a missing transcript, the kill switch, and no data directory. Identical.

Budget test, mutation-checked. hook-failure-audit.test.sh gains an strace-based assertion on both the creation and exec ceilings, skipping cleanly where strace is unavailable or not permitted. Non-vacuity was demonstrated, not assumed: moving either silenced redirect back inside its substitution — SIZE=$(wc -c <"$TRANSCRIPT" 2>/dev/null), or RECORDS=$(grep -F … -- "$TRANSCRIPT" 2>/dev/null) — pushes creations from 9 to 10 with no change to the exec count, leaves all 93 behavioural assertions green, and fails the budget assertion. That is precisely the fork xtrace cannot see.

Gates, all foreground:

Gate	Result
scripts/affected-tests.sh --run	exit 0, both selected suites pass (hook-failure-audit.test.sh 94/94, audit-session-id.test.sh 27/27)
shellcheck -x on both changed shell files	clean
shfmt -d -i 2 -ci	clean
scripts/check-changelog-parity.sh --check / --check-order / --check-bump origin/main / --check-preserved origin/main	all four pass
markdownlint-cli2 on the changed markdown	clean
scripts/check-purged-em-dashes.sh	pass
scripts/check-shell-portability.sh origin/main	pass
scripts/check-silent-skips.sh, check-killswitch-hoist.sh, check-hook-exec-form.sh, check-hook-wiring-liveness.sh, check-cross-plugin-source-drift.sh, validate-plugins.sh	pass

The wording-only follow-up commit re-ran the test suites, all four parity modes, shellcheck -x, shfmt -d -i 2 -ci and markdownlint-cli2: unchanged, including the 9-creation / 4-exec budget assertions, which is the expected result for a comment-and-prose change.

test_save_point.py::test_new_origin_falls_back_to_directory_name is pre-existing and not in this diff's selection.

Acceptance criteria not fully met

Two of the issue's criteria are not satisfied, and no amount of in-file work would satisfy them:

• "A turn with no hook failures exits before any external process spawn." Not met, and not reachable. The hook learns there is nothing to report by reading the transcript; there is no cheaper oracle. The floor without touching the fenced hook-utils.sh or abandoning the O(cap) tail bound is one wc, one grep, and the library's own payload parse — which is what this PR reaches. If the criterion is to be met literally it needs a different design (a marker written by the failing hook, or consolidation with perf(autonomy): Stop hook: lane-stop gate averages 27.6s and timed out 73x - reduce process spawns #3515/perf(disk-hygiene): Stop-event guard_launch_monitor times out 52x via run-python-hook.sh launcher overhead #3516), not a further micro-optimization here.
• "The always-on per-turn set stays within <= 500 ms parallel wall." Not measured. That figure is cross-plugin (this hook shares the budget with perf(autonomy): Stop hook: lane-stop gate averages 27.6s and timed out 73x - reduce process spawns #3515 autonomy and perf(disk-hygiene): Stop-event guard_launch_monitor times out 52x via run-python-hook.sh launcher overhead #3516 disk-hygiene) and binds to Windows Git Bash, which this runner is not. The README records the figure as owed rather than implying it was taken.

A third is met in a different form than specified: the issue asks for a PATH shim spawn census. This uses strace instead, which is strictly stronger for this defect — a PATH shim sees only programs that are exec'd, and the whole cost here is subshell forks that never exec anything.

Related

• Parent: perf(hooks): Windows process-creation tax makes the hook set unusable - 1,778 timeouts / 20.6 hours blocked over 9 days #3508 (the campaign; its stated cause is corrected above for this hook)
• Precedent: perf(context-guard): PostToolBatch: context-zone crossing averages 73.0s and timed out 10x - reduce process spawns #3520 / PR perf(context-guard): cut the zone-crossing fire from 8 process creations to 3 #3779 (established redirection placement as the real cost), PR perf(hooks): cut leftover Bash parse and dirname tax on the hot path #3788 (34 scripts, 17 plugins, zero library edits)
• Siblings on the same 500 ms per-turn budget: perf(autonomy): Stop hook: lane-stop gate averages 27.6s and timed out 73x - reduce process spawns #3515 (autonomy), perf(disk-hygiene): Stop-event guard_launch_monitor times out 52x via run-python-hook.sh launcher overhead #3516 (disk-hygiene)
• Fenced, deliberately untouched: fix(hook-utils): a hook payload cut short at EOF is a loud allow; a stall stays a block #3740, perf(hooks): fuse stdin jq completeness with field extract #3838 (lib/hook-utils.sh and its 17 synced copies); the here-string note above cites hook::jq_field there but changes nothing in it
• Adjacency checked, no edit-surface overlap: open PR feat(hook-telemetry): carry the correlation keys on the envelope spine at 1.1 #3769 adds correlation keys (session_id, prompt_id, tool_use_id, agent_id) to the hook-telemetry envelope spine inside hook::emit_telemetry. It does not touch hook-failure-audit.sh. This diff leaves that hook's SESSION_ID extraction and its data.session_id envelope key exactly as they are, so nothing here collides with the key migration. The two PRs both bump plugins/claude-ops/.claude-plugin/plugin.json and prepend to its CHANGELOG, so whichever lands second takes a trivial version/heading rebase.

🤖 Generated with Claude Code

https://claude.ai/code/session_01ViPsHkL3ng9xWt2GjEQJob

---

Generated by Claude Code

[chatgpt-codex-connector]

Codex Review Summary

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

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-09-06T19:21:46.744730Z	b8ae268	Draft marked ready

ℹ️ About Codex in GitHub

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

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

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

[kyle-sexton]

Independent review and the corrections it produced — flipped to ready

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

Verified independently, not read

Check	Result
mkdir TOCTOU	Safe. The gate is [[ -d ]] || mkdir -p … 2>/dev/null, so a race loser still runs the idempotent -p form. 200 concurrent fresh-dir invocations, 0 failures; a failure would only leave MARKER empty (re-warn path), never break the hook
Detection unchanged	13 scenarios × 4 artefacts (stdout, stderr, rc, marker bytes) = 64 comparisons, 0 diffs against origin/main — including tail cap, missing transcript, kill switch, no data dir, whitespace transcript path, pre-existing CRLF marker
wc naming vs redirect	Equivalent on whitespace path, newline-in-name, and missing file
$(<file) vs $(cat)	Identical on empty, absent, trailing-newline, CRLF, and unreadable (tested as uid 65534)
grep window	Byte-identical under cap; boundary probes at cap−1/cap/cap+1 and mid-line cuts select the same lines
execve drops accounted	common 6→4 = cat + one batched jq; warn 17→14 = cat, tr, one jq. grep, wc, sed, tail, find unchanged — forks elided, no work removed
Mutation	Reproduces: either redirect back inside its substitution gives creations 9→10, execs 4→4, failing only the ceiling
Fences and #3769	Clean; all 17 synced copies byte-identical to lib, no SESSION_ID line in the diff

One divergence found and it is an improvement: jq_fields now keeps "0" for a non-string session_id where the old code fell to no-session because gsub on a number failed. Unreachable with real UUID payloads.

The correction worth recording

The code comment asserted as universal fact that a here-string at pipe capacity deadlocks before jq execs — the stated reason for leaving a printf | jq fork in place. The reviewer could not reproduce it on Linux bash 5.2 at 65535, 65536, 65537 bytes, 200 kB or 2 MB, even with an unwritable TMPDIR.

Traced to source: the claim originates in dfda6ec3 (#1587), whose trace (65536 hung indefinitely, 65000 returned at once) was measured entirely on Git Bash. The shared library's own comment says only "on this platform". The comment now says exactly that — the hazard is documented and reproduced in that context, does not reproduce on Linux bash 5.2, and the pipeline is kept because the forgone saving is one fork on the warning path only, not because the hazard is universal.

The decision was right; the justification was overstated. A comment asserting an unverified platform-specific hazard as universal outlives the PR and can stop a future worker from taking a real saving.

Also fixed: CHANGELOG.md:16 said "Five sites" over a list of six.

Not fully met, stated plainly

Two acceptance criteria are not met and the body says so under its own heading: the hook cannot exit before any spawn, because reading the transcript is how it learns there is nothing to report (1 wc + 1 grep + the fenced library's parse is the floor); and the 500 ms parallel-wall figure is cross-plugin and Windows-only, recorded in the README as owed. The spawn census uses strace rather than the issue's PATH shim, which is strictly stronger — a shim cannot see forks that never exec.

Gates: affected-tests.sh --run exit 0 (94/94 and 27/27, budget assertions still 9 creations / 4 execs), shellcheck, shfmt -d -i 2 -ci, four changelog-parity modes, silent-skips, killswitch-hoist, hook-exec-form, hook-wiring-liveness, em-dash purge — all clean.

---

Generated by Claude Code

[claude]

Claude finished @kyle-sexton's task in 6m 21s —— View job

---

Code review — /review:code-review

• Gather context (PR body, diff, changed files)
• Run /review:code-review per instructions
• Post inline findings (if any)
• Post final summary

Skip gate: PR is open, not draft, and the change has real review surface (five files, the core logic in hook-failure-audit.sh), so a full review applies. No prior successful review from this lane exists on this head.

Scope reviewed: git diff origin/main...HEAD (5 files: plugin.json, CHANGELOG.md, README.md, hook-failure-audit.sh, hook-failure-audit.test.sh). Security concerns are out of scope for this lane.

What I checked line by line in hook-failure-audit.sh:

• hook::jq_fields batching (lines 71–79): correctly handles the documented difference from hook::jq_field — an absent field arrives as "" with rc 0 rather than a non-zero return, and the new code spells out [[ -n "$TRANSCRIPT" ... ]] / [[ -n "$SESSION" ]] explicitly instead of riding on ||. This is a real (and correctly handled) semantic difference from the two-call version, not a copy-paste risk.
• SIZE=""; { read -r SIZE _ < <(wc -c -- "$TRANSCRIPT"); } 2>/dev/null (lines 97–99): the 2>/dev/null on the enclosing group redirects the current shell's fd 2 before the process substitution forks, so the child wc inherits the already-redirected fd — the stderr silencing is preserved. Failure (missing/unreadable file) leaves SIZE empty via read's EOF, caught by the same [[ -n "$SIZE" ]] || exit 0 the original had.
• Pre-filter split into over-cap / under-cap branches (lines 164–173): under-cap now has grep open the transcript directly instead of cat | grep; over-cap pipeline is left untouched, matching the stated rationale (a pipeline element forks either way, so hoisting there buys nothing).
• Marker read { WARNED=$(<"$MARKER"); } 2>/dev/null (line 230): $(<file) is bash's zero-fork file-read form; behavior on a missing/unreadable file matches the "any bookkeeping failure leaves WARNED empty" contract already documented above it.
• Marker write (lines 314–317): FINGERPRINTS=$(jq ...) strips only the trailing newline; printf '%s\n' puts exactly one back, and jq's internal newlines between multiple fingerprint lines survive the round-trip through command substitution, so multi-hookName output still lands one-fingerprint-per-line in the marker file — same on-disk shape as the old tr-piped version.
• mkdir -p guarded by [[ -d "$MARKER_DIR" ]] || first (line 224): equivalent — an existing-but-unwritable directory fell through the old mkdir -p the same way it falls through -d now, and still fails at the write below.
• No dangling references to the removed read_window helper anywhere in the plugin; hook-failure-audit.sh isn't synced into any other plugin, so no cross-plugin drift risk from this change.

Docs/version: README's relative link to ../../docs/conventions/hook-budget/README.md resolves; CHANGELOG's "six sites" list matches the six sites actually touched in the diff; plugin.json version bump (0.42.20 → 0.42.21) matches the new CHANGELOG heading.

Test file (hook-failure-audit.test.sh): the new strace-based budget block gates cleanly on strace availability/permission before asserting. One low-confidence observation, not raised as a finding: prog_count() (used for the exact grep/cat/wc assertions) matches any execve line for that program regardless of exit status, while the EXECS/PROGS diagnostics above it filter to = 0$ (successful) only. In practice bash resolves a full path before issuing a single execve, so this shouldn't produce spurious failed-attempt lines for standard coreutils — not blocking.

Not independently verified by me: this lane's sandbox restricts Bash to read-only git/gh inspection, so I read the diff and library code but did not execute scripts/affected-tests.sh, shellcheck, shfmt, the test suite, or strace myself. The PR body's process-creation counts (18→9, 6→4), the "93 behavioural assertions green," and the four changelog-parity/lint gates are author-claimed and unverified by me — noted here rather than restated as confirmed.

Result: no high-confidence correctness or maintainability findings. The six optimization sites are semantically equivalent to what they replace, including the edge cases (empty-field extraction, missing/unreadable files, multi-record marker writes), and the change stays scoped to the one file it targets plus its test and docs.

[claude]

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

---

Security review (/review:security-review)

• Gather context (PR diff via gh pr diff 3851, read changed files)
• Skip-gate check
• Review changed files for security-relevant surface
• Report

Skip gate: PR is open; the diff does touch file I/O (hook-failure-audit.sh reads a transcript path and writes a marker file), so I did not stop at the skip gate — reviewed in full.

Scope reviewed: all 5 changed files — hook-failure-audit.sh, hook-failure-audit.test.sh, plugin.json, README.md, CHANGELOG.md.

Finding: none.

This is a pure performance refactor of hook-failure-audit.sh (redirection placement to cut subshell forks); no new inputs, no new file writes, no new external calls, and no behavior change was introduced beyond process count (the PR's own byte-for-byte marker/stdout/stderr comparison across ten scenarios backs this). Specifically checked:

• Path handling: TRANSCRIPT and the marker path are unchanged in provenance from before this diff. SESSION is still passed through the same ${SESSION//[^A-Za-z0-9_-]/-} allowlist before being used in MARKER="$MARKER_DIR/${SESSION}" (hook-failure-audit.sh#L97), so no path-traversal surface opened up.
• Flag/argument injection: the two sites that switched from redirection to a file-argument form both add -- before the filename — wc -c -- "$TRANSCRIPT" (L94) and grep -F '"hook_non_blocking_error"' -- "$TRANSCRIPT" (L166) — so a transcript path beginning with - still can't be parsed as an option.
• Quoting: every substitution touched ($(<"$MARKER"), $(jq …), the read -r … < <(wc …) process substitution) stays double-quoted where expansion happens; no new word-splitting/glob surface.
• Fail-open discipline preserved: every new guard ([[ -n "$SIZE" ]], [[ -n "$TRANSCRIPT" && -f "$TRANSCRIPT" ]], the mkdir -p/-d fallback) still exits 0 or falls through to "treat as new" on any bookkeeping failure, matching the hook's existing advisory/fail-open contract — no path where a read/parse failure would silently suppress a real warning.
• No shared-library edit: lib/hook-utils.sh and its synced copies are untouched, consistent with the PR's stated fence.
• Test file changes are strace-based budget assertions over fixed, test-controlled fixtures — no untrusted input, no injection surface.

No CRITICAL/IMPORTANT/SUGGESTION findings to report.
· branch claude/3512-failure-audit-perf

[github-actions]

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

[github-actions]

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

[kyle-sexton]

Lane babysit-loop, instance ccr-session-babysit-loop-20260906, claimed at head b8ae268b (mergeable_state=dirty, ~1.7 h since last activity). Conflict resolved. Head moves b8ae268b -> 5abf82b3.

The conflict

One file, plugins/claude-ops/CHANGELOG.md. Both sides had written a different entry under the same heading 0.42.21 — a version collision, not a stack:

• main's 0.42.21 is the perf(hooks): fuse stdin jq completeness with field extract #3838 hook::buffer_stdin_to sync;
• this branch's 0.42.21 is the failure-audit fork reduction.

Resolved by the convention this cluster already uses: main's entry stays verbatim at the number main published it under, and this branch's entry is renumbered above it. This branch's entry becomes 0.42.22 and plugins/claude-ops/.claude-plugin/plugin.json moves to 0.42.22 to match.

hooks/hook-failure-audit.sh, the subject of this PR, merged cleanly. No README version reference needed renumbering.

Verification

• All four changelog-parity modes pass, including --check-bump origin/main and --check-order across 91 changelogs.

• scripts/affected-tests.sh --run: no failures.

• hook-failure-audit.test.sh run explicitly (the merge diff is docs plus manifest, so the suite is not otherwise selected): PASS=94 FAIL=0, including the strace budget assertions this PR added:

  ok: budget: one grep pre-filter, reading the transcript directly (1)
  ok: budget: no cat feeding the pre-filter (0)
  ok: budget: one wc for the tail-cap decision (1)
  

Worth noting because the sibling shard #3869 did not survive this merge unchanged — main's buffer_stdin_to dropped its guard's last fork and invalidated an exact pin there. This PR's budget assertions still hold as written.

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

---

Generated by Claude Code