Skip to content

perf(hook-utils): build the telemetry envelope and read file_path with builtins - #3678

Merged
kyle-sexton merged 5 commits into
mainfrom
perf/hook-utils-telemetry-hot-path
Sep 3, 2026
Merged

perf(hook-utils): build the telemetry envelope and read file_path with builtins#3678
kyle-sexton merged 5 commits into
mainfrom
perf/hook-utils-telemetry-hot-path

Conversation

@kyle-sexton

@kyle-sexton kyle-sexton commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

No related issue: phase 4b of the hook-performance program, tracking issue #3623

Summary

With HOOK_TELEMETRY_SINK wired, every guard that reached hook::emit_telemetry paid two jq processes, a mktemp and an rm per run, and hook::read_file_path cost a jq plus four realpath and two cygpath processes on every Write and Edit. On Windows Git Bash each spawn is tens of milliseconds; the phase 4a profile put the telemetry cost at about 1.2 s of the 2.5 s Bash dispatcher wall across five guards.

Both functions now do their work with shell builtins and hand anything they cannot prove to the unchanged jq path.

Fix

hook::emit_telemetry

  • The envelope is assembled in the shell: hook::json_escape_jq_to escapes the string fields exactly as jq does (backslash, quote, \b \f \n \r \t, other C0 bytes and DEL as \u00xx, non-ASCII verbatim) and hook::json_compact_to compacts the caller's data object exactly as jq -c does when that can be proven. jq's own output (pretty or compact), and the compact literal fallbacks the hooks carry, all qualify.
  • Anything not proven (a \u or \/ escape, a raw control byte, a fraction, an exponent, a 16-digit integer, a non-object, invalid JSON) goes to the jq path, which now runs jq -nc so both paths write the same bytes. jq's absence is fail-open only there.
  • One deliberate change in the emitted bytes, flagged for review: the old filter ran jq -n without -c, so today's envelope is jq's pretty-printed multi-line document (CRLF line endings with a Windows jq). The new envelope is one compact line. The sink contract is one JSON document on stdin; the repo's own sink appends a byte-identical record for both forms (proof below), and a compact line is also valid JSONL for a sink that appends raw envelopes. The hook-telemetry convention README now states the compact-line shape and that sinks must parse JSON rather than line layout.
  • No mktemp, no rm, no temp file on the builtin path. The timestamp uses printf -v '%()T' with a date -u fallback when printf binds nothing.
  • Test 2 (jq absent) is rewritten: it asserted "returns 0 with no output" because the envelope needed jq; it now asserts the envelope is delivered with an empty PATH and with a failing jq shadow that is never invoked.

hook::read_file_path

  • stdin is read into the shell (NUL-safe) and .tool_input.file_path is taken by hook::_fast_file_path_to when it can prove jq's answer: the payload is one JSON value by the grammar (every string escape one jq accepts, no raw control bytes), exactly one string in the whole payload decodes to tool_input and exactly one to file_path, tool_input is a direct member of the root object whose value is a flat object (no nested object or array), and file_path is a plain string inside it. Key strings are compared after decoding escapes. A top-level file_path, a tool_input nested elsewhere, two keys, a non-string value, a payload over 64 KiB, a NUL byte, or a non-ASCII \u escape all fall back to the unchanged jq filter.
  • No per-character scanning of the payload: the only whole-payload operations are literal substring replacement, containment tests, IFS splitting and offset slicing (bash regex and %% on a 60 KB payload cost more than the jq process they would replace on this host).
  • The membership comparison resolves the file, the project root and every temp-root candidate with one batched realpath (hook::_physical_prime) into a per-process cache of plain indexed arrays (Bash 3.2 safe), and hook::under_temp_root reads the cache. The file's entry is forgotten after use so a later call in the same process re-resolves it.
  • The git branch uses hook::dirname_to (builtin dirname with the . and / fallbacks).
  • Every existing helper keeps its printing form; _to <var> variants store into a variable so the hot path forks nothing it does not need.

No check was removed. hook::json_escape (a different tool: it drops residual control bytes, which is right for a notice) is unchanged.

Verification

Differential harness under $TEMP/phase4b/ (emit-diff.sh, rfp-diff.sh, exec-count.sh, rfp-time.sh): the OLD lib from git show origin/main:lib/hook-utils.sh and the NEW lib run in separate bash processes on the same inputs.

emit_telemetry (28 cases, 0 diffs, 9 take the jq fallback)

Columns: document is jq -c . of old vs new; compact is the NEW raw bytes vs jq -c . of the OLD (the builtin escaper and compactor reproduce jq's compact rendering byte for byte); sink row is the record .claude/hooks/hook-telemetry-sink.sh appends to hook-events.jsonl for old vs new. timestamp and duration_ms are masked (nondeterministic across two processes) and the timestamp shape is asserted separately.

# case new path document compact sink row note
1 plain builtin identical identical identical
2 empty data {} builtin identical identical identical
3 nested arrays and objects builtin identical identical identical
4 strings with quotes builtin identical identical identical
5 strings with backslashes builtin identical identical identical
6 newlines and tabs builtin identical identical identical
7 control chars (ESC, 0x01, DEL) jq-fallback identical identical identical
8 35 KB blob builtin identical identical identical
9 non-ASCII builtin identical identical identical
10 status with a quote builtin identical identical identical
11 empty hook_id builtin identical identical identical
12 pretty jq -n data (guard shape) builtin identical identical identical
13 compact literal fallback builtin identical identical identical
14 hand-written escaped slash jq-fallback identical identical identical
15 hand-written unicode escape jq-fallback identical identical identical
16 data not an object (array) jq-fallback identical identical no-record sink rejects the envelope on both sides (data.tool lookup on an array)
17 numbers (jq-rendered) jq-fallback identical identical identical
18 numbers (hand-written literals) jq-fallback identical identical identical
19 integers only builtin identical identical identical
20 hook_event with control chars builtin identical identical no-record sink writes no record on both sides
21 all string fields non-ASCII builtin identical identical identical
22 invalid JSON data jq-fallback both-empty both-empty both-empty no envelope on either side
23 data with trailing newline builtin identical identical identical
24 findings with messages (markdownlint shape) builtin identical identical identical
25 raw DEL inside data string jq-fallback identical identical identical
26 pretty data with raw tab indent builtin identical identical identical
27 subject with a Windows path (guard shape) builtin identical identical identical
28 empty data string jq-fallback identical identical identical

read_file_path (42 cases: 41 identical, 1 intended difference, 7 take the jq fallback)

Same stdin, same environment, separate processes; stdout and return code compared.

# case new path old rc/out new rc/out verdict
1 normal Write, in-project (compact) builtin 0/PROJ/plain.md 0/PROJ/plain.md identical
2 Edit shape builtin 0/PROJ/plain.md 0/PROJ/plain.md identical
3 pretty payload (jq -n fixture shape) builtin 0/PROJ/plain.md 0/PROJ/plain.md identical
4 path with spaces builtin 0/PROJ/with space.md 0/PROJ/with space.md identical
5 path with escaped quote (no such file) builtin 1/ 1/ identical
6 Windows drive path, backslashes builtin 1/ 1/ identical
7 Windows drive path, forward slashes builtin 1/ 1/ identical
8 path with A escape builtin 0/PROJ/plain.md 0/PROJ/plain.md identical
9 path with é escape (non-ASCII) jq-fallback 0/PROJ/é.md 0/PROJ/é.md identical
10 path with raw non-ASCII builtin 0/PROJ/é.md 0/PROJ/é.md identical
11 path with \r builtin 0/PROJ/plain.md 0/PROJ/plain.md identical
12 missing file builtin 1/ 1/ identical
13 file outside project builtin 1/ 1/ identical
14 temp file, project outside temp builtin 1/ 1/ identical
15 project under temp builtin 0/HOMEPROJ/scratch/tmpfile.md 0/HOMEPROJ/scratch/tmpfile.md identical
16 in-project file, project outside temp builtin 0/HOMEPROJ/inside.md 0/HOMEPROJ/inside.md identical
17 symlinked file escaping the project builtin 1/ 1/ identical
18 two file_path keys at different depths jq-fallback 0/PROJ/plain.md 0/PROJ/plain.md identical
19 tool_input with a nested object jq-fallback 0/PROJ/plain.md 0/PROJ/plain.md identical
20 tool_input with a nested array of objects jq-fallback 0/PROJ/plain.md 0/PROJ/plain.md identical
21 file_path at top level only builtin 1/ 1/ identical
22 70 KiB payload jq-fallback 0/PROJ/plain.md 0/PROJ/plain.md identical
23 40 KiB payload (under the cap) builtin 0/PROJ/plain.md 0/PROJ/plain.md identical
24 no CLAUDE_PROJECT_DIR, inside a git tree builtin 0/GITREPO/tracked.md 0/GITREPO/tracked.md identical
25 no CLAUDE_PROJECT_DIR, outside any git tree builtin 1/ 1/ identical
26 no CLAUDE_PROJECT_DIR, git absent builtin 1/ 1/ identical
27 empty file_path builtin 1/ 1/ identical
28 file_path is a number builtin 1/ 1/ identical
29 no file_path at all (Bash payload) builtin 1/ 1/ identical
30 tool_input nested in another object builtin 1/ 1/ identical
31 truncated after tool_input builtin 1/ 1/ identical
32 payload with a trailing NUL byte jq-fallback 0/PROJ/plain.md 0/PROJ/plain.md identical
33 key spelled with \u escapes builtin 0/PROJ/plain.md 0/PROJ/plain.md identical
34 duplicate file_path key inside tool_input jq-fallback 0/PROJ/plain.md 0/PROJ/plain.md identical
35 root is an array builtin 1/ 1/ identical
36 empty stdin builtin 1/ 1/ identical
37 invalid JSON (bad literal) builtin 1/ 1/ identical
38 invalid escape in an unrelated string builtin 1/ 1/ identical
39 bad \u hex in an unrelated string builtin 1/ 1/ identical
40 project dir with trailing slash builtin 0/PROJ/plain.md 0/PROJ/plain.md identical
41 prefix sibling of the project builtin 1/ 1/ identical
42 jq absent, in-project Write builtin 1/ 0/PROJ/plain.md intended difference

Cases 38 and 39 are payloads jq rejects outright (an invalid escape anywhere in the text); the skeleton pass validates every escape in every string, so the fast path never proves a value from a text jq would refuse. Case 42 is the one behavior change on this function and it is intended: with jq absent the old reader could never extract a path (the jq filter failed, so every file was skipped); the builtin path needs no jq, so a well-formed payload is admitted. Every hook in this marketplace gates on hook::require_jq or hook::require_jq_blocking before reaching the reader, so no shipped hook observes it.

Exec count and wall time: typos-format PostToolUse Write, sink wired

plugins/typos-format/hooks/typos-format.sh run from a scratch copy of the plugin dir with the OLD and NEW lib swapped in (the carrier copy was never edited), on an in-repo scratch .md, HOOK_TELEMETRY_SINK pointed at a file sink, CLAUDE_PROJECT_DIR set. External programs counted from a PS4='+X ' bash -x trace; wall is the median of 5 runs.

lib external execs wall median (ms) 5 runs (ms) envelopes delivered
old (origin/main) 24 1130 1169, 1130, 1099, 1100, 1307 6
new 17 861 994, 782, 839, 885, 861 6

A second run of the same harness on the final lib, on a busier host: old 24 execs / 1390 ms median, new 17 execs / 994 ms median.

By program, old: jq 7, realpath 4, cygpath 4, tr 3, rm 1, mktemp 1, git 1, basename 1, sink 1, typos 1. New: jq 5, cygpath 4, tr 3, realpath 1, git 1, basename 1, sink 1, typos 1. The remaining jq processes belong to the hook itself (buffer_stdin validation, tool_name, build_data_json, findings), not to the lib; the four cygpath are hook::repo_relative_path and expand_8dot3 on an 8.3-spelled temp candidate, both out of scope here.

hook::read_file_path alone (median of 7, CLAUDE_PROJECT_DIR set, in-project file):

payload old (ms) new (ms)
102 bytes (plain Write) 507 118
40 KB content 567 160
70 KB content (jq fallback) 538 243

Suites (one at a time, this Windows host)

suite result
lib/hook-utils.test.sh PASS=284 FAIL=0 (was 224 on main; new cases: 14b escaper corpus, 14c compactor corpus including invalid escapes, 3b envelope bytes and fallback, 12g fast-path parity, top-level key, invalid escape elsewhere, 64 KiB threshold both sides, 12h dirname_to, Test 2 rewritten)
.claude/hooks/hook-telemetry-sink.test.sh PASS (2 checks), exit 0
lib/rewrite-guard.test.sh 16 passed, 0 failed
plugins/guardrails/hooks/run-guards.test.sh PASS=66 FAIL=0
plugins/guardrails/hooks/block-windows-drive-tmp.test.sh PASS=199 FAIL=15, the 15 are the known host-only /usr/bin/mkdir /tmp/x cases on this Windows machine; the telemetry assertions (now read through jq) pass
plugins/markdown-format/hooks/markdown-format.test.sh PASS=162 FAIL=4, the 4 are the known host-only PATH-shape cases; the unwired path asserts one jq spawn (the stdin probe) now that file_path is read with builtins

First CI run failed on exactly those two suites (they grepped the pretty-envelope spelling and counted two jq spawns) plus a typos hit on two deliberate JSON-typo test literals; all three are fixed in the last commit.

scripts/affected-tests.sh --explain selects 142 suites for this change (not run on this host per the shared-host discipline; CI runs everything).

Gates

  • shellcheck --rcfile .shellcheckrc lib/hook-utils.sh lib/hook-utils.test.sh: clean
  • bash scripts/check-shell-portability.sh origin/main: exit 0 (three \b sites annotated portability-ok, they are bash ANSI-C backspace bytes, not GNU grep word boundaries; one realpath multi-operand site annotated)
  • bash scripts/sync-hook-utils.sh --check: all 17 copies match
  • bash scripts/sync-hook-utils.sh --check-bump origin/main: exit 0 (origin/main re-fetched at 9f07fb5 before the bump)
  • bash scripts/check-changelog-parity.sh --check-bump origin/main: exit 0
  • markdownlint-cli2 plugins/*/CHANGELOG.md: 0 issues
  • em dash count did not grow in any touched file (lib 141 to 140, test 158 to 158)
  • Bash 3.2: no associative arrays, no ${var,,} outside the existing OSTYPE guard, %()T guarded by a date -u fallback

Related

🤖 Generated with Claude Code

kyle-sexton and others added 2 commits September 2, 2026 20:10
…h builtins

Phase 4b of the hook-performance program (#3623). With a telemetry sink
wired, every guard that reached hook::emit_telemetry paid two jq
processes, a mktemp and an rm per run, and hook::read_file_path cost a
jq plus four realpath and two cygpath processes on every Write and Edit.
On Windows Git Bash each spawn is tens of milliseconds.

hook::emit_telemetry now assembles the envelope with shell builtins:
hook::json_escape_jq_to escapes the string fields exactly as jq does and
hook::json_compact_to compacts the caller's data object exactly as jq -c
does, when that can be proven (jq's own output, pretty or compact, and
the compact literal fallbacks the hooks carry all qualify). Anything it
cannot prove (a \u or \/ escape, a raw control byte, a fraction or
exponent, a non-object) goes to the jq path, which now runs jq -nc so
both paths write the same bytes. The envelope is one compact line where
it was jq's pretty-printed document before; the sink contract is one
JSON document on stdin and the repo's own sink appends a byte-identical
record for both forms. jq's absence is fail-open only on the fallback.

hook::read_file_path reads stdin into the shell and takes
.tool_input.file_path with hook::_fast_file_path_to when it can prove
jq's answer: one string decoding to tool_input and one to file_path in
the whole payload, tool_input a direct member of the root object whose
value is a flat object, file_path a plain string inside it. Any other
shape, a payload over 64 KiB, a NUL byte, or a non-ASCII \u escape falls
back to the unchanged jq filter. The membership comparison then resolves
the file, the project root and the temp roots with one batched realpath
(hook::_physical_prime) into a per-process cache of plain arrays, and
hook::under_temp_root reads the cache instead of resolving each
candidate again. dirname is hook::dirname_to.

Differential harness (origin/main lib vs this lib, separate processes):
emit_telemetry 28 cases, 0 diffs (same document, new raw bytes equal
jq -c of old, same sink record; 9 take the jq fallback); read_file_path
39 cases, identical stdout and status on all (7 take the jq fallback).
typos-format PostToolUse Write with a sink: 24 external execs to 17,
wall median 1130 ms to 861 ms. lib/hook-utils.test.sh PASS=278 FAIL=0.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
scripts/sync-hook-utils.sh copies the builtin telemetry envelope and the
builtin file_path reader into every carrying plugin; each carrier's
manifest moves one patch above origin/main so the update cache delivers
the change, with a CHANGELOG entry naming the shared-library change.
Phase 4b of the hook-performance program (#3623).

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 3, 2026

Copy link
Copy Markdown

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-03T00:52:29.782832Z 8a97deb PR opened
ℹ️ About Codex in GitHub

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

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

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

@claude

claude Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

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


Code review complete

  • Gather context (PR body, diff, skip-gate check)
  • Review lib/hook-utils.sh core logic (hook::emit_telemetry, hook::json_escape_jq_to, hook::json_compact_to, hook::read_file_path, hook::_fast_file_path_to, hook::_physical_prime, hook::under_temp_root, hook::dirname_to)
  • Review lib/hook-utils.test.sh new/changed test cases
  • Spot-check carrier sync (plugins/*/hooks/hook-utils.sh, plugin.json bumps, CHANGELOGs)
  • Post summary (no inline findings)

Summary

Traced the new builtin JSON machinery in lib/hook-utils.sh line by line: hook::_json_split/hook::_json_skeleton (offset-preserving quote splitting via the \\@@/\"@@ same-length substitution), hook::_fast_file_path_to (uniqueness + depth-of-1 proof for tool_input, plain-string proof for file_path), hook::json_compact_to (number/escape refusal conditions), and hook::json_escape_jq_to (control-byte escaping). I did not find a correctness defect in the proof conditions or in hook::emit_telemetry's builtin-vs-jq branching — the refusal conditions (nested containers, \u/\/ escapes, raw control bytes, fractions/exponents, 16-digit integers, >64 KiB payloads, duplicate key names) correctly fall back to the unchanged jq path rather than risk a wrong answer, which is the right bias for a hand-rolled parser.

Independently confirmed (not just author-claimed):

  • All 17 plugins/*/hooks/hook-utils.sh copies are byte-identical to lib/hook-utils.sh (diffed two directly).
  • The plugin.json version bumps and CHANGELOG.md entries I spot-checked (typos-format, guardrails) are consistent patch bumps with a "Changed" entry matching the vendored-copy convention used by prior syncs.

Not independently verified — sandboxed to Read/Grep/Glob/git diff/git log in this review lane, and executing shell scripts (even bash script.sh against the library itself) required approval I don't have here — so these remain author-claimed:

  • The differential-harness byte-parity results (28 emit_telemetry cases, 40 read_file_path cases).
  • The exec-count/wall-time measurements.
  • shellcheck, check-shell-portability.sh, sync-hook-utils.sh --check{,-bump}, and the lib/hook-utils.test.sh suite run (PASS=278).

One trivial nit, not worth a blocking comment: the PR body's own verification table (row 30, "tool_input nested in another object") labels the read_file_path result "builtin", but by my hand-trace of hook::_fast_file_path_to that shape ({"outer":{"tool_input":{"file_path":"deep.md"}}}) computes a bracket-depth of 2 for the tool_input key and returns 2 (jq fallback), not a proven verdict — lib/hook-utils.test.sh's own fast_is_jq harness (12g) treats both 1 and 2 as passing for that case, so it can't distinguish the two, and the end result (file not found) is identical either way. This looks like a labeling slip in the PR description, not a code defect.

No blocking findings.

@claude

claude Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Claude finished @kyle-sexton's task in 4m 26s —— View job


Security review (/review:security-review)

  • Gather context (PR body, changed files)
  • Run /review:security-review per custom instructions
  • Post findings / summary

Scope: All 17 hooks/hook-utils.sh copies are byte-identical to lib/hook-utils.sh (same blob SHA 18e4dd5), so I reviewed the one substantive change: lib/hook-utils.sh. plugin.json/CHANGELOG.md changes are routine version bumps from scripts/sync-hook-utils.sh. No GitHub Actions workflow files are touched.

This PR rewrites two security-relevant trust-boundary functions to avoid jq/realpath subprocess spawns:

  • hook::read_file_path / hook::under_temp_root — decides whether a hook is allowed to auto-edit a file (project-membership and temp-scratch gating).
  • hook::_fast_file_path_to — a hand-rolled JSON grammar walker that extracts .tool_input.file_path without jq.

I traced the correctness argument for each rather than relying on the PR's own differential harness:

  • Raw-spelling shortcut in hook::under_temp_root (L654-L683): only engages when the caller has proven target is already a fully-resolved physical path (_HOOK_UTR_TARGET_PHYSICAL, set only by hook::read_file_path after realpath succeeded), and it can only produce an early accept, never an early reject — a non-matching raw prefix always falls through to the full resolved comparison. Given a canonical (symlink-free) target, any literal string-prefix match against an unresolved candidate is provably equivalent to what resolving the candidate would yield, so this can't create a false "under temp root" verdict that a resolved comparison wouldn't also produce. _HOOK_UTR_TARGET_PHYSICAL is reset to 0 immediately after each call site and no caller outside hook::read_file_path sets it, so the shortcut can't leak into unrelated callers.
  • hook::_fast_file_path_to: every ambiguous shape (duplicate key anywhere in the payload, tool_input nested elsewhere, non-flat tool_input, oversized payload, non-ASCII \u escape, NUL byte) explicitly falls back to the unchanged jq filter (return 2) rather than guessing. The 60-byte string-length skip is exactly the maximally-escaped length of tool_input/file_path (10 × 6 bytes), so it can't skip a body that could decode to either name.
  • hook::json_compact_to / hook::json_escape_jq_to (telemetry envelope): the compactor only accepts input that first passes full JSON-grammar validation (hook::_json_skeleton), so re-embedding its output as "data":<value> can't break out of the envelope's JSON structure — it's already balanced, escape-verified JSON. Numbers needing jq's own rendering (fractions, exponents, ≥16-digit integers) are explicitly excluded and deferred to jq.
  • One documented behavior change (jq-absent case in hook::read_file_path now admits a well-formed payload instead of always rejecting): verified unreachable in every shipped hook — each caller (bash-format, markdown-format, typos-format, the three guardrails verify hooks, etc.) calls hook::require_jq first, which exit 0s the whole process when jq is missing (lib/hook-utils.sh L334-L342), so hook::read_file_path is never reached in that state.
  • hook::_physical_prime's batched realpath call is argument-injection-safe (-- separator), rejects any candidate path containing a newline before batching, disables globbing during the IFS=$'\n' split, and discards the whole batch (falling back to per-path resolution) on any line-count mismatch — so a divergent-behavior realpath can't desync the path↔result mapping.

No command injection, path-traversal, or project-membership-bypass issues found in this diff. The refactor consistently chooses "fall back to jq" or "fall back to the pre-existing resolved comparison" over guessing whenever it can't prove its answer matches jq/realpath's, which preserves the original fail-safe posture of both functions.

…e and sinks parse JSON

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@kyle-sexton
kyle-sexton enabled auto-merge (squash) September 3, 2026 00:50
kyle-sexton added a commit that referenced this pull request Sep 3, 2026
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@github-actions

github-actions Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Last security-reviewed head: 67693922790b61b7abe509e551b8508578d0768e. 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

github-actions Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

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

@claude

claude Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Claude finished @kyle-sexton's task in 9m 41s —— View job


Code review — PR #3678

  • Gather context (PR body, diff, CLAUDE.md/AGENTS.md, REVIEW.md)
  • Review lib/hook-utils.sh (canonical; synced identically to 17 plugin copies)
  • Review lib/hook-utils.test.sh
  • Review docs/conventions/hook-telemetry/README.md
  • Sanity-check the sync/changelog/version bump mechanics across plugin copies
  • Post findings (inline + summary)

Summary

This is the code-review lane; per REVIEW.md, security findings are out of scope here since this repo carries a claude-security-review workflow.

I read lib/hook-utils.sh line by line against its own documented contracts — the new builtin JSON skeleton parser/tokenizer (hook::_json_split, hook::_json_skeleton), the .tool_input.file_path fast-path prover (hook::_fast_file_path_to), the jq-parity compactor/escaper (hook::json_compact_to, hook::json_escape_jq_to), and the physical-path cache used by hook::under_temp_root's new raw-spelling shortcut. Traced through the escaping order (\\ then \" substitution preserving byte offsets/length so slices into the original payload stay valid), the depth computation for tool_input membership, the number-literal refusal regex, and the macOS-symlinked-/tmp fallback behavior for the temp-root shortcut. All of it held up against its own stated invariants, and the fallback-to-jq design means anything not provably safe degrades to the pre-existing (safe) jq path rather than risking a wrong answer.

I also independently verified, rather than just taking the PR body's word for it:

  • md5sum lib/hook-utils.sh plugins/*/hooks/hook-utils.sh — all 17 copies are byte-identical to the canonical source.
  • Spot-checked plugins/typos-format/.claude-plugin/plugin.json + CHANGELOG.md — version bump and changelog entry are consistent with the sync.

No inline findings — I did not find a concrete, high-confidence correctness defect in the changed lines that a careful reviewer would block on. The one thing I could not do is execute anything (test suite, shellcheck, the differential harness) — this review lane's sandbox declines to run arbitrary shell/gh/network commands, so the PR body's own verification tables (differential harness results, suite pass counts, gate outputs) are author-claimed and unverified by me; I'm flagging that distinction rather than restating those numbers as independently confirmed.

Worth checking before merge: the top-level ci workflow run for this head SHA currently reports conclusion: failure (run), while claude-security-review, pr-title, managed-files-guard, pr-issue-linkage, and do-not-merge all report success. The CI-status tool only surfaced 30 jobs for that run, all showing success, so I could not pin down which job actually failed — worth a direct look at the Actions run.

kyle-sexton added a commit that referenced this pull request Sep 3, 2026
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…N paths prove anything

jq rejects a text with an invalid escape (\q, \uZZZZ, a trailing
backslash) anywhere in it, so the fast file_path reader must not prove a
value from such a text and the compactor must not splice it. The
skeleton pass now deletes every well-formed escape from each string body
with literal glob substitution and falls back to jq if a backslash
survives. The string cap per payload rises from 800 to 2000 so a large
Edit payload with many quoted strings still qualifies. Copies re-synced.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@claude

claude Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

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


Code review — PR #3678

  • Gather context (PR body, diff, CLAUDE.md/AGENTS.md)
  • Invoke /review:code-review — the skill call errored in this environment (<error>Execute skill: review:code-review</error> on every attempt), so I reviewed manually instead, following the same lane conventions (high-signal correctness/maintainability only, security out of scope since a security lane already reviewed this PR)
  • Hand-trace the delta since the last review pass (commit a832a4b)
  • Post summary (no inline findings)

Summary

This PR was already reviewed three times at head 8a97deb (two code-review passes, one security pass), all with no blocking findings. This invocation is triggered by one new commit since then, a832a4b ("reject invalid string escapes before the builtin JSON paths prove anything"), which hardens hook::_json_skeleton in lib/hook-utils.sh so it rejects strings carrying an escape jq wouldn't accept (\q, bad \u hex, a lone trailing backslash), instead of silently treating any backslash sequence as opaque content.

I hand-traced the new logic (I could not execute shell scripts in this sandbox — same limitation noted in the prior two review passes — so this is static analysis, not a test run):

  • The escape scan runs on the already-@@-neutralized string body (\\ and \" are pre-collapsed by hook::_json_split), so the remaining backslashes can only start \/ \b \f \n \r \t \uXXXX. It removes \uXXXX (exactly 4 hex digits) first, then each single-char escape, and flags anything with a surviving backslash as invalid.
  • Because the removed patterns are exactly the JSON-valid escape forms, a match can never correspond to an actually-invalid sequence — so there's no path to a false "proven valid" verdict. Any backslash that isn't part of a well-formed escape survives and correctly forces return 1 (fallback to jq), which is the safe direction for a proof-or-fallback design.
  • I checked the specific new test case '{"a":"x\\\"}' (a832a4b: lib/hook-utils.test.sh line ~813): this text is actually unterminated (jq: "Unfinished string at EOF"), and I confirmed by hand that it's already rejected upstream by hook::_json_split's odd-quote-count check, independent of the new escape scan. Not a defect — just means that particular test doesn't isolate the new code path — but the new dedicated cases (\q, \uZZZZ, and "invalid escape in an unrelated string" for _fast_file_path_to) do exercise it directly and look correct by hand-trace.
  • Confirmed the real bug this closes: hook::_json_skeleton previously validated brackets/quotes/control-bytes only, with no escape validation, so hook::_fast_file_path_to (which doesn't independently re-validate escapes elsewhere in the payload the way hook::json_compact_to already did) could have proven a file_path from a payload that was actually invalid JSON per jq, purely because of an unrelated field's bad escape. In practice hook::buffer_stdin already validates the whole payload with jq before hook::read_file_path is ever called, so this wasn't reachable through any shipped hook — but it's a correct hardening for the function's standalone contract and for direct unit-test invocation.
  • Verified the 17 carrier copies stay byte-identical to lib/hook-utils.sh after this commit (diff against plugins/guardrails/hooks/hook-utils.sh and plugins/typos-format/hooks/hook-utils.sh, both clean).
  • The 800→2000 string-cap change in hook::_json_split is capacity-only (still bounded by the existing 64 KiB payload cap) and doesn't affect correctness.
  • This commit didn't bump plugin.json/CHANGELOG.md again (only lib/hook-utils.sh + the 17 copies), but per scripts/sync-hook-utils.sh --check-bump's contract that compares against origin/main, one bump per PR (already done at 8a97deb) is sufficient — this isn't a gap.

No blocking findings. Not independently verified here (sandbox has no shell-execution approval in this lane): the differential-harness results, shellcheck, check-shell-portability.sh, and the lib/hook-utils.test.sh run — these remain author-claimed, same caveat as the prior two review passes.

@claude

claude Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

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


Security review (/review:security-review) — re-run on a832a4b

  • Gather context (PR body, HEAD SHA a832a4b, custom instructions)
  • Skip gate: PR open, security-relevant surface present, but head advanced since the last reviewed SHA (8a97deb) — reviewing the delta (a832a4b)
  • Review the new commit's changes to lib/hook-utils.sh / lib/hook-utils.test.sh (17 synced copies verified byte-identical)
  • Post findings / summary

Scope

Only one new commit since the last-reviewed head: a832a4b — "reject invalid string escapes before the builtin JSON paths prove anything." Confirmed via md5sum that all 17 plugins/*/hooks/hook-utils.sh copies remain byte-identical to lib/hook-utils.sh after this commit, so the only substantive review surface is lib/hook-utils.sh#L757-L786 and the corresponding lib/hook-utils.test.sh additions.

What this commit changes

hook::_json_skeleton (used by both hook::_fast_file_path_to and hook::json_compact_to) previously only rejected a string body for a raw control byte, not for an invalid escape (\q, \uZZZZ, a trailing lone backslash). Since \\ and \" are neutralized to @@ before this check runs, any backslash surviving in the neutralized body must start one of \/ \b \f \n \r \t or \uXXXX — the fix strips every well-formed instance of those and rejects (falls back to jq) if a backslash remains.

Trust-boundary analysis

This is exactly the right thing to fix from a security standpoint, and I traced it for correctness rather than taking the commit message at face value:

  • Before this fix, a payload containing a string with an invalid escape (e.g. \q) was not actually valid JSONjq would reject the whole document — but the builtin skeleton parser had no way to detect that, so it could wrongly "prove" a file_path or splice unvalidated bytes into the telemetry envelope from a document jq would refuse to parse at all. That's a real prove/fallback parity gap in a function whose contract (per its own header comment) is "provably safe, or fall back — never close enough." The fix closes it.
  • Soundness of the check: since \\/\" are already collapsed to @@ prior to this scan, every surviving backslash is guaranteed to be the start of a real escape sequence in the original text (JSON escapes are prefix-free — \ + one char, or \u + 4 hex digits — so they can't overlap). The glob-based removal (\uXXXX first, then the six single-char escapes) is a sound one-pass consumption; leftover \U0041 (wrong case U), \a, \B, or a truncated \u12 all correctly leave a dangling backslash and fall back to jq, matching jq's case-sensitive, exactly-4-hex-digit grammar.
  • Direction of failure is safe: every rejection path (return 1 from _json_skeleton) routes to the pre-existing jq fallback in both callers (_fast_file_path_to maps it to return 2; json_compact_to maps it to return 1, both meaning "run jq instead"), not to a wrong answer. I did not find an escape shape that the check would wrongly accept as valid.
  • The companion string-cap increase (800 → 2000 quote chars) is a capacity/performance change with no security implication — it only widens how large a payload can still qualify for the (now more strict) builtin path before deferring to jq.
  • New test cases (compact_refuses "invalid escape", "trailing backslash escape", "bad \u hex digits", and the fast_is_jq cases for an invalid escape elsewhere in the payload) cover the fixed gap, including the case where the bad escape is not in the file_path/tool_input strings themselves — correct, since the whole document must be valid JSON for jq to answer at all.

Result

No security issues found in this delta. The commit is a targeted, correctly-scoped fix that tightens a builtin JSON parser's proof condition to match jq's actual acceptance grammar, and every rejection path degrades to the safe, pre-existing jq fallback rather than risking a wrong trust decision. Nothing here touches GitHub Actions workflows, permissions, or supply-chain pins.

…spawn on the unwired path

The envelope is one compact line now, so block-windows-drive-tmp's suite parses
it instead of matching pretty-printed spacing, markdown-format's unwired case
expects the single stdin-probe jq (file_path is read with builtins), and the
compactor's truncated-literal fixtures carry the spellchecker directive.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@claude

claude Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

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


Code review — PR #3678

  • Gather context (PR body, diff, CLAUDE.md/AGENTS.md)
  • Invoke /review:code-review — the skill call errored again in this environment (<error>Execute skill: review:code-review</error>, same as the prior two review passes on this PR), so I reviewed manually following the same lane conventions (correctness/maintainability only, security out of scope since a security lane already reviewed this PR)
  • Hand-trace the delta since the last-reviewed commit (a832a4b)
  • Post findings (none — summary only)

Summary

This invocation is triggered by one new commit since the last code-review pass, 6769392 — "read the telemetry envelope through jq and count one jq spawn on the unwired path." It's test-and-comment-only; no functional code changed:

  • lib/hook-utils.sh#L789 adds a trailing # spellchecker:disable-line to an existing comment (the one mentioning tr ue) — no code change.
  • lib/hook-utils.test.sh adds the same spellchecker directive to two compact_refuses test-description strings (tru, tr ue) — cosmetic, no assertion logic changed.
  • plugins/guardrails/hooks/block-windows-drive-tmp.test.sh switches three telemetry assertions from raw substring matches against pretty-printed JSON ('"hook": "block-windows-drive-tmp"') to jq -r field extraction (.hook, .status, .data.form, .data.tool, .data.subject), which is the correct fix now that the envelope is one compact line rather than jq's old pretty-printed multi-line output. I checked this against the actual producer: block-windows-drive-tmp.sh's emit_tel() (hooks/block-windows-drive-tmp.sh#L159-L174) builds data as {tool,subject,form} and calls hook::emit_telemetry with top-level hook/status — matching the new jq -r .hook / .status / .data.form / .data.tool / .data.subject paths exactly.
  • plugins/markdown-format/hooks/markdown-format.test.sh updates the unwired-path jq-spawn-count assertion from 2 to 1. I verified this against markdown-format.sh directly: TOOL=$(... | jq -r '.tool_name // empty') at markdown-format.sh#L365 only runs if hook::telemetry_enabled, and hook::read_file_path no longer spawns jq on the builtin path (this PR's core change) — so the unwired default path has exactly one jq spawn left, hook::buffer_stdin's payload-completeness probe. The updated count is correct.

Independently confirmed rather than taken on faith:

  • md5sum lib/hook-utils.sh plugins/*/hooks/hook-utils.sh — all 17 carrier copies remain byte-identical after this commit.
  • No plugin.json/CHANGELOG.md bump in this commit — consistent with the prior review's finding that this PR's single bump (at 8a97deb) covers the whole PR under sync-hook-utils.sh --check-bump's contract.

No blocking findings. This PR has now been reviewed five times across code-review and security-review lanes at three successive heads (8a97deb, a832a4b, 6769392) with no defect found in the core hook::_json_skeleton / hook::_fast_file_path_to / hook::json_compact_to / hook::under_temp_root logic; this delta is test-alignment for the envelope-format and jq-spawn-count changes already reviewed, and it's internally consistent with the hook scripts it tests.

Not independently executed in this sandbox (no shell-execution approval in this review lane, same caveat as every prior pass): the differential harness, shellcheck, check-shell-portability.sh, and running lib/hook-utils.test.sh / block-windows-drive-tmp.test.sh / markdown-format.test.sh themselves.

@claude

claude Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Claude finished @kyle-sexton's task in 44s —— View job


Security review (/review:security-review) — re-run on 6769392

  • Gather context (PR body, HEAD SHA 6769392, custom instructions)
  • Skip gate: PR open, security-relevant surface present, but head advanced since the last reviewed SHA (a832a4b) — reviewing the delta (6769392)
  • Review the new commit's changes
  • Post findings / summary

Scope

One new commit since the last-reviewed head: 6769392 — "read the telemetry envelope through jq and count one jq spawn on the unwired path."

git show --stat 6769392 touches 21 files: lib/hook-utils.sh + its 17 synced carrier copies (1 line each), lib/hook-utils.test.sh, plugins/guardrails/hooks/block-windows-drive-tmp.test.sh, and plugins/markdown-format/hooks/markdown-format.test.sh.

What actually changed

  • lib/hook-utils.sh (and the 17 copies): a single-line change to an existing comment inside hook::_json_skeleton — appends # spellchecker:disable-line. No code, logic, or byte-output change. Confirmed via md5sum lib/hook-utils.sh plugins/*/hooks/hook-utils.sh: all 17 carrier copies remain byte-identical to the canonical source (hash 969936acdf0af8b0e9d3fe2d9eb3f217 everywhere).
  • lib/hook-utils.test.sh: same spellchecker-directive comment added to two existing compact_refuses test-description lines ("bad literal", "split literal"). No assertion logic changed.
  • plugins/guardrails/hooks/block-windows-drive-tmp.test.sh: rewrites telemetry assertions to parse the envelope with jq -r .hook / .status / .data.form / .data.tool / .data.subject instead of substring-matching '"hook": "..."' literals. This is a test adapting to the now-compact (single-line) envelope format introduced earlier in this PR (commit 0f75f60) — it doesn't change what's asserted, only how the value is extracted from JSON, which is more robust than string matching.
  • plugins/markdown-format/hooks/markdown-format.test.sh: updates the expected jq-spawn count on the "unwired" (telemetry sink unset) path from 2 to 1, and its comment, to match hook::read_file_path now resolving file_path via builtins rather than a jq spawn — this is the test catching up to the already-reviewed read_file_path behavior change from earlier commits in this PR, not a new behavior change itself.

Trust-boundary analysis

None of this touches hook::emit_telemetry, hook::read_file_path, hook::_fast_file_path_to, hook::under_temp_root, hook::_physical_prime, hook::json_compact_to, or hook::json_escape_jq_to logic — the only functional line changed in lib/hook-utils.sh is a comment. Everything else is test assertions catching up to already-reviewed behavior (the compact envelope format from 0f75f60, reviewed in the initial security pass; the builtin read_file_path path from 0f75f60, reviewed in the initial security pass; and the escape-validation hardening from a832a4b, reviewed in the prior delta pass).

Result

No security issues found in this delta — it is comment and test-only, with no change to program logic, trust-boundary decisions, or emitted bytes. Nothing here touches GitHub Actions workflows, permissions, or supply-chain pins.

kyle-sexton added a commit that referenced this pull request Sep 3, 2026
…sals

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@kyle-sexton
kyle-sexton merged commit 5e3d749 into main Sep 3, 2026
70 checks passed
@kyle-sexton
kyle-sexton deleted the perf/hook-utils-telemetry-hot-path branch September 3, 2026 01:31
kyle-sexton added a commit that referenced this pull request Sep 3, 2026
#3678 landed

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
kyle-sexton added a commit that referenced this pull request Sep 3, 2026
No related issue: phase 8 of the hook-performance program, tracking
issue #3623

## Summary

The hook-performance program landed nine PRs today (#3621, #3662, #3666,
#3668, #3671, #3675, #3676, #3677, #3678, plus dotfiles #620). This PR
records the outcome where the convention says it lives: the hook-budget
convention gains a dated reference-figures section in spawn-equivalents
with the harness sha256, and the four plugins that lacked a hook budget
accounting row gain one under Requirements. Documentation only; no hook,
script or registration changes.

## Fix

- `docs/conventions/hook-budget/README.md`: a "Reference figures
(2026-09-02, after the hook-performance program)" section with the
harness identity, the before-and-after spawn-equivalents per surface
(before at S = 33 ms, after at S = 18 ms on `main` `5e3d749cb`), the
reference-host conversion, and a plain reading against the budget table:
per-turn rows meet the 500 ms ceiling; per-tool-call rows sit at 1.4 to
1.9 s against the 1 s typical ceiling, with the guardrails dispatcher
named as the whole of the remainder.
- `plugins/typos-format/README.md`, `plugins/eol-normalizer/README.md`,
`plugins/markdown-format/README.md`, `plugins/context-guard/README.md`:
a "Hook budget accounting" section each, carrying the measured rows
their CHANGELOG entries already state (36.3 to 26.0, 41.0 to 21.5, 41.6
to 32.0 spawn-equivalents; 11 to 2, 9 to 4 and 6 to 1 processes for
context-guard), what changed, and the residual. guardrails and
rate-limit-guard already carried one.
- Version bumps with a documentation-only CHANGELOG entry: typos-format
0.6.37, eol-normalizer 0.6.30, markdown-format 0.11.40, context-guard
0.7.36.

## Verification

- Final harness run on the installed cache at `main` `5e3d749cb`,
`--runs 3`, S = 18 ms, valid, quiet host; every measured plugin's cache
directory byte-compared against `origin/main` (0 stale files in 17);
`enabledPlugins` unchanged against the pre-program snapshot; 52
`hooks.json` entries listed, every one `type: command`, no `async` row.
The per-event block, the STATED CHECK and the reading against goal (B)
are in the program's PLAN.md and DEVIATIONS.md on
`perf/hook-performance-program`.
- Per event (ms, slowest hook): PreToolUse:Bash 2,475 before to 1,599
after; PostToolBatch 1,254 to 282; UserPromptSubmit 975 to 297; in-repo
PostToolUse:Write 13,225 to 1,949; in-repo PostToolUse:Edit 17,192 to
3,048.
- `markdownlint-cli2` 0 issues on the five files; em dash counts
unchanged; `scripts/check-changelog-parity.sh --check-bump origin/main`
exit 0; `scripts/affected-tests.sh --explain` selects no suites (every
changed file is a recorded no-suite class covered by a non-shell CI
lane).

## Related

- #3623 (tracking issue)
- Merged today: #3621, #3662, #3666, #3668, #3671, #3675, #3676, #3677,
#3678; dotfiles #620

🤖 Generated with [Claude Code](https://claude.com/claude-code)

---------

Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
kyle-sexton added a commit that referenced this pull request Sep 3, 2026
Pre-prune commit: bac012f (PLAN.md with the restated goal, the constraint 1
mapping table and the final run block; DEVIATIONS.md with every ruling,
finding and evidence run). Durable outcomes graduated to: the outcome and
follow-up comments on issue #3623, docs/conventions/hook-budget/README.md
and the six plugin READMEs (#3679), docs/conventions/hook-telemetry/README.md
(#3678), and issues #3680 to #3685.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
kyle-sexton added a commit that referenced this pull request Sep 5, 2026
…and emit_tel jq (#3732)

<!-- CURSOR_AGENT_PR_BODY_BEGIN -->
No linked issue

## Summary

Phase 1 of a measurement-first shell-script performance pass: cut
remaining process creations on the always-on hook hot path in
`lib/hook-utils.sh` (17 plugin copies) and stop spending `jq` on
wired-sink `{tool,subject,form}` telemetry.

## Fix

Discovery ranked remaining waste after the hook-performance program
(#3623). Guardrails already measured and rejected dispatcher `repo_root`
priming, per-guard `dirname` replacement, and merging
`hardcoded-path-check` git probes. This PR takes the three leftover cuts
that had a drift-immune counter and a matching repo pattern:

1. **`hook::buffer_stdin` timeout/slice wrappers.** GNU Bash runs
command substitution and process substitution in a subshell even for
builtins. The suite already documented those two startup forks. `_to`
helpers write into caller variables with `printf -v`.
2. **`hook::repo_root` `git | tr`.** Same in-shell CR strip
`buffer_stdin` already uses (`${root//$'\\r'/}`).
3. **`hook::json_str_object_to` + seven always-on Bash-guard `emit_tel`
builders.** Compact `{tool,subject,form}` is now builtin and
byte-identical to `jq -nc --arg …`. The envelope was already builtin
(#3678). Unwired default path is unchanged (zero telemetry spawns).

Carrying plugin versions are bumped so consumers receive the synced
copies. After merging `main` twice, those versions were re-bumped past
the numbers `main` shipped in the meantime, and every earlier heading
this branch introduced (including claude-ops `0.41.14`) is kept so
`--check-preserved` holds.

Phase 2 (not in this PR): context-guard resolver in-process;
skill/stale-path grep short-circuit; `affected-tests.sh` manifest cache.

## Verification

Headline counters on this Linux host (wall-clock refused as binding;
Windows Git Bash remains the consumer cost model):

| Subject | Before | After |
|---|---|---|
| `buffer_stdin` timeout/slice | two subshells (`$$` ≠ `$BASHPID`) |
in-process (`$$` = `$BASHPID`) |
| `hook::repo_root` | `spawns=2 [1 git 1 tr]` | `spawns=1 [1 git]` |
| `emit_tel` data object | `spawns=1 [1 jq]` | `spawns=0` |
| emit compact bytes | `{"tool":"Bash","subject":"git status
--short","form":""}` | identical (`cmp`) |

`bash lib/hook-utils.test.sh`: PASS=286 FAIL=0.

Critical guard contracts: block-no-verify 238, block-dangerous-git 479,
block-hook-bypass 576, block-noncanonical-commit 213,
block-convention-violation 70, block-windows-drive-tmp 214,
block-exported-msys-pathconv 127, run-guards 66 — all FAIL=0.

`scripts/affected-tests.sh --run`: 142 shell suites passed; 9 selected
non-shell suites named as NOT RUN (CI lanes). Zero FAIL lines.

Post-merge gates vs `origin/main`: `sync-hook-utils.sh --check` /
`--check-bump`, `check-changelog-parity.sh --check` / `--check-bump` /
`--check-preserved` / `--check-order`, `check-shell-portability.sh`, and
`shellcheck -x lib/hook-utils.test.sh` all OK. Re-ran
`hook-utils.test.sh` (286/0) on the merge.

## Related

Refs #3623, #1403, #3678. Follows
`docs/conventions/hook-budget/README.md` (budget never relaxes) and
`plugins/performance/reference/harness-integrity.md` (spawn count over
two-pass wall-clock).

<!-- CURSOR_AGENT_PR_BODY_END -->

<div><a
href="https://cursor.com/agents/bc-491558a0-20ba-4b4e-ad79-4f386f90c774?cursor_ref=pr_footer&cursor_cta=open_in_web"><picture><source
media="(prefers-color-scheme: dark)"
srcset="https://cursor.com/assets/images/open-in-web-dark.png"><source
media="(prefers-color-scheme: light)"
srcset="https://cursor.com/assets/images/open-in-web-light.png"><img
alt="Open in Web" width="114" height="28"
src="https://cursor.com/assets/images/open-in-web-dark.png"></picture></a>&nbsp;<a
href="https://cursor.com/background-agent?bcId=bc-491558a0-20ba-4b4e-ad79-4f386f90c774&cursor_ref=pr_footer&cursor_cta=open_in_cursor"><picture><source
media="(prefers-color-scheme: dark)"
srcset="https://cursor.com/assets/images/open-in-cursor-dark.png"><source
media="(prefers-color-scheme: light)"
srcset="https://cursor.com/assets/images/open-in-cursor-light.png"><img
alt="Open in Cursor" width="131" height="28"
src="https://cursor.com/assets/images/open-in-cursor-dark.png"></picture></a>&nbsp;</div>

---------

Co-authored-by: Cursor Agent <cursoragent@cursor.com>
Co-authored-by: ksextonmelodic <ksextonmelodic@gmail.com>
kyle-sexton added a commit that referenced this pull request Sep 7, 2026
…ners (#3878)

<!-- CURSOR_AGENT_PR_BODY_BEGIN -->
No linked issue

## Summary

Drop leftover process creations on the hottest Bash paths in this
marketplace: the shared hook library every always-on hook sources, the
always-on formatter Write/Edit paths (typos, ruff, biome, bash,
powershell, go, actionlint, eol, markdown), the always-on
desktop-notification Notification path, always-on guardrails verifiers,
and CI scanners that used to spawn once per file, per plugin, or per
allowlist entry.

## Fix

GNU Bash runs command substitution in a subshell even for builtins
(Command Substitution, [Bash Reference
Manual](https://www.gnu.org/software/bash/manual/html_node/Command-Execution-Environment.html);
[Greg's Wiki](https://mywiki.wooledge.org/CommandSubstitution)).
Cygwin's `fork` is a non-copy-on-write Win32 `CreateProcess` ([Cygwin
User's Guide, Process
Creation](https://ftp.cygwin.com/cygwin-ug-net/highlights.html)): "fork
will almost certainly always be inefficient under Win32."

### Shared hook library (`lib/hook-utils.sh`, synced to 17 carriers,
patch bump)

Same `_to` / in-process pattern as #3838, #3732, and #3678:

- `hook::json_escape_to` deletes residual C0 bytes with parameter
expansion instead of `printf | tr -d`
- `hook::emit_channels` writes through `_to` instead of
`$(hook::json_escape …)`
- Fractional `read -t` landed in bash-4.0-alpha (CHANGES).
`hook::read_supports_fractional_timeout` is `BASH_VERSINFO`; no TMPDIR
probe file
- `hook::notice_once` reads the marker with `read`, creates the
directory only when missing, and prunes stale markers once per process
- `hook::bash_parse_segments` walks `${cmd:i:1}` instead of `read -N1`
from a process substitution, and `$'…'` bodies decode through
`ansi_c_decode_to` (`printf -v`)
- `hook::repo_root_to` / `hook::repo_relative_path_to` write in this
shell so callers skip a leftover capture around git or builtins-only
work

Isolation `$(source …)` forks are unchanged (#3685).

### typos-format (always-on Write|Edit|NotebookEdit)

- Basename via `${FILE##*/}` (plus a backslash trim), not `basename(1)`
- `repo_root_to` / `repo_relative_path_to` instead of capture subshells
- Directory existence check instead of `$(cd && pwd)`
- `command -v typos` is no longer captured; the later exec looks the
name up on PATH

### Remaining always-on formatters (ruff, biome, bash, powershell, go,
actionlint, eol, markdown)

Same leftover class as typos-format, now applied to every always-on
formatter that still captured `_to` helpers or spawned `basename` /
leftover `cd && pwd`:

- `FILE_BASE` is `${FILE##*/}` (and a backslash trim)
- `repo_root_to` / `repo_relative_path_to` write in-process
- `$(cd && pwd)` canonicalize is an existence check on the path git
already answered (ruff, biome, bash-format EditorConfig walk)
- Nested `$(normalize_path "$(physical_path …)")` in powershell-format
uses the `_to` forms
- `command -v ruff|biome|goimports` is no longer captured
- markdown-format keeps physical `pwd -P` containment and config
discovery; leftover helper-capture and membership dirname on the
root-resolution path are gone

### desktop-notification (always-on Notification)

- Field extract fuses into `hook::buffer_stdin_to` so completeness and
`.notification_type` / `.message` share one jq process
- C0 stripping is parameter expansion, not `printf | tr`
- `repo_root_to` writes in-process; OSC 9 / BEL use `printf -v`;
`terminalSequence` uses `json_escape_jq_to`
- `uname` stays so tests can PATH-stub Darwin; git for `repo_root` stays

### guardrails verifiers (always-on PostToolUse / PreToolUse)

- `skill-reference-verify`, `stale-path-verify`, and `cli-flag-verify`
call `repo_root_to` / `repo_relative_path_to` in-process
- `hardcoded-path-check` and `secret-pattern-detection` use
`normalize_path_to` instead of leftover `$(hook::normalize_path)`
captures
- Isolation `$(source …)` forks are unchanged (#3685)

### CI scanners

- Orphaned-fixture scan: one `*.test.*` index, cached `evals.json`
`files[]`, in-shell ERE escape. Unquoted `\\` matches one backslash (a
quoted `'\\'` arm is two chars and leaves `\b` as a word boundary)
- Purged-em-dash scan: one `git ls-files -z` with every `:(glob)`
pathspec; in-process component-wise attribution so `*` cannot cross `/`.
`--list` stdout is byte-identical to origin/main
- Cross-plugin source drift: one `find plugins` plus one `sha256sum` of
2+ cluster paths. Discover stdout is byte-identical to origin/main
- Discriminating-test-skips / silent-skips: one awk per corpus (`FNR` +
`FILENAME`; mawk has no `ENDFILE`)
- Hook-exec-form: one jq over every `hooks.json` and one over every
`plugin.json` (`input_filename` attributes rows). Unreadable
`hooks.json` still fails closed via per-file fallback; unreadable
manifests are still skipped

Hook-specific leftover-fork work already in flight (#3873, #3872, #3871,
#3870, #3869, #3851, #3849, #3779, #3880, #3886) is out of scope here.

## Verification

Independent census re-derived spawn counts from `84adf87b` vs `cdb93f61`
without inheriting implementer figures. Kernel census `strace -f -e
trace=clone,clone3,fork,vfork,execve`; counter over duration; 3
identical trials.

**always-on formatters** (this revision vs `84adf87b`):

| Hook | clones before | clones after | execve before | execve after |
|---|---|---|---|---|
| ruff-format no-config skip | 14 | 10 | 4 | 4 |
| powershell-format no-settings skip | 23 | 16 | 7 | 7 |
| bash-format no-EditorConfig (ShellCheck finding) | 17 | 13 | 6 (1
`basename`) | 5 (0 `basename`) |

**guardrails** (this revision):

| Hook | clones before | clones after | execve |
|---|---|---|---|
| skill-reference-verify Write, no skill refs | 18 | 17 | 8 unchanged |
| secret-pattern-detection clean Write | 10 | 8 | 4 unchanged |

Secret-pattern absolute counts are with `CLAUDE_PLUGIN_ROOT` set (Claude
Code always sets it). Without that env the leftover `PLUGIN_ROOT=$(cd …
&& pwd)` fallback adds one clone on both sides (11→9); the drop of 2 is
the same.

**CI scanners** (successful execve, exclude ENOENT; earlier commits on
this PR):

| Gate | origin/main or prior HEAD | HEAD |
|---|---|---|
| purged-em-dashes `--list` | 478 | 9 |
| cross-plugin-source-drift `--check` | 181 | 4 |
| discriminating-test-skips | 316 (awk 312) | 5 (awk 1) |
| silent-skips | 120 (awk 118) | 4 (awk 2) |
| hook-exec-form `--check` | 196 execve, jq 96, tr 96, clones 292 | 7
execve, jq 2, tr 0, clones 9 |

`--list` / discover stdout for the two listing gates is byte-identical
to origin/main.

**Local `scripts/affected-tests.sh --run`:** 153 shell suites passed or
were skipped; 14 NOT RUN python/mjs ecosystems (exit 3, expected on this
runner). No `FAIL`. Including: `lib/hook-utils.test.sh` PASS=323;
bash-format PASS=54; eol-normalizer PASS=54; markdown-format PASS=174;
powershell-format PASS=17; cli-flag-verify PASS=92; hardcoded-path-check
PASS=118; secret-pattern-detection PASS=86; skill-reference-verify
PASS=140; stale-path-verify PASS=108. ruff/biome/go/actionlint
behavioral cases skipped here (binaries absent); skip-path and source
pins still ran. `session-event-log.test.sh` PASS=53 isolated under the
fan-out.

**CI on `cdb93f61`:** lint, hook-utils, test-linux (0–3), test-windows,
changes, ci-status, and managed-files-guard all succeeded.
https://github.com/melodic-software/claude-code-plugins/actions/runs/34066676378
https://github.com/melodic-software/claude-code-plugins/actions/runs/34066676488

## Related

Refs #3838, #3732, #3678, #1979, #3488, #2891. Same leftover-fork class
as open PRs #3849 / #3851 / #3869 / #3873 / #3872 / #3871 / #3870 /
#3779 / #3880 / #3886 (those stay hook-specific). N/A for a dedicated
issue.

<!-- CURSOR_AGENT_PR_BODY_END -->

<div><a
href="https://cursor.com/agents/bc-fdfdc962-be1b-4c9c-9833-3aec57852330?cursor_ref=pr_footer&cursor_cta=open_in_web"><picture><source
media="(prefers-color-scheme: dark)"
srcset="https://cursor.com/assets/images/open-in-web-dark.png"><source
media="(prefers-color-scheme: light)"
srcset="https://cursor.com/assets/images/open-in-web-light.png"><img
alt="Open in Web" width="114" height="28"
src="https://cursor.com/assets/images/open-in-web-dark.png"></picture></a>&nbsp;<a
href="https://cursor.com/background-agent?bcId=bc-fdfdc962-be1b-4c9c-9833-3aec57852330&cursor_ref=pr_footer&cursor_cta=open_in_cursor"><picture><source
media="(prefers-color-scheme: dark)"
srcset="https://cursor.com/assets/images/open-in-cursor-dark.png"><source
media="(prefers-color-scheme: light)"
srcset="https://cursor.com/assets/images/open-in-cursor-light.png"><img
alt="Open in Cursor" width="131" height="28"
src="https://cursor.com/assets/images/open-in-cursor-dark.png"></picture></a>&nbsp;</div>

---------

Co-authored-by: Cursor Agent <cursoragent@cursor.com>
Co-authored-by: ksextonmelodic <ksextonmelodic@gmail.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.

1 participant