feat(hook-telemetry): carry the correlation keys on the envelope spine at 1.1 - #3769
Merged
Conversation
… predicate per extension The Write|Edit PostToolUse row spawned the dispatcher, loaded the library and parsed the payload on every write, and all three verifiers early-exited unless the file was one of five extensions. The row now carries one handler per extension with an if: Edit(*.<ext>) predicate that Claude Code evaluates before spawning: 86.1 ms (26.9 S) to 0 processes on an in-repo .txt, the .md row unchanged at 95.8 ms (N = 15, Linux CI host). run-guards.test.sh pins the predicate set to the union of the verifiers' own case gates in both directions. Guardrails 0.32.3 to 0.32.4. The issue's typos-format and eol-normalizer rows take no predicate: typos scans every file type by design (its README records the absence of an extension gate as deliberate) and eol-normalizer resolves every path through .gitattributes with no extension list. Recorded in the README budget entry. Closes #3751 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019DaWEB8Daq1xAXy2Xj1Pme
The shell-portability lint rejects the GNU-only \s; the comment filter now uses [[:space:]]. Same verdicts, 77/77. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019DaWEB8Daq1xAXy2Xj1Pme
… fills The per-session observability report's "Rewrote" block reads a changed boolean the sink copies from data.changed, and no producer sent it. The shared rewrite guard now records a byte verdict at the take (HOOK_REWRITE_CHANGED: true, false, or unknown when it could not snapshot), its six carriers add the key to their telemetry data, and markdown-format and typos-format derive it from their own rewrite signals (the fix-count line, applied corrections). biome, go, powershell and ruff took the disclosure after emitting on some arms; those arms now take first, with stdout unchanged. bash-format takes unconditionally before its emit so a run where shfmt never ran reports false rather than nothing. Every data schema gains the optional key; each suite pins it on a rewriting run and a no-op run; the observability fixture carries one changed row and the case that asserted the block empty now asserts the file is named. Nine patch bumps: bash 0.7.37, biome 0.6.35, eol 0.6.36, go 0.3.39, powershell 0.7.38, ruff 0.6.36, markdown 0.11.45, typos 0.6.43, claude-ops 0.42.7. Closes #3755 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019DaWEB8Daq1xAXy2Xj1Pme
…session-id-ji6rl5
typos-format's clean arm reached typos and knows the answer, so it now sends changed: false instead of omitting the key; an all-clean session reads "nothing rewritten" rather than "no data". ruff-format's tool-break arm emitted telemetry before taking the disclosure, so a rewrite the fix or format pass had already written was not recorded; the take now precedes the emit as on every other arm. One suite case pins the clean verdict. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019DaWEB8Daq1xAXy2Xj1Pme
…a-changed-ji6rl5 # Conflicts: # plugins/claude-ops/CHANGELOG.md
…e at 1.1 hook::emit_telemetry now reads the payload the producer buffered (INPUT, or HOOK_TELEMETRY_PAYLOAD when set) and copies session_id, prompt_id, tool_use_id and agent_id onto the envelope between duration_ms and data, each only when present as a plain id, on both the builtin and the jq path. schema_version reads 1.1; the envelope schema, the contract README and its changelog record the four optional keys. The claude-ops reference sink and its repo-local copy route on the spine session_id first and fall back to data.session_id, so every fleet producer lands in the per-session file with no change to the hooks themselves. The library suite pins both paths, the malformed-id and no-payload cases, and the payload override; the sink suite pins the spine route and its precedence; every envelope assertion moves to 1.1. Seventeen carrier plugins take a patch bump for the synced library. Closes #3758 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019DaWEB8Daq1xAXy2Xj1Pme
…metry-session-id-ji6rl5 # Conflicts: # plugins/claude-ops/.claude-plugin/plugin.json # plugins/claude-ops/CHANGELOG.md
…ion-id-ji6rl5 # Conflicts: # docs/conventions/hook-telemetry/README.md
Contributor
Author
|
Version-collision heads-up on Your notice listed If you keep 0.42.10, Nothing else in your list overlaps with mine. I touch no file under Generated by Claude Code |
…ion-id-ji6rl5 # Conflicts: # plugins/actionlint/CHANGELOG.md # plugins/autonomy/CHANGELOG.md # plugins/bash-format/.claude-plugin/plugin.json # plugins/bash-format/CHANGELOG.md # plugins/biome-format/.claude-plugin/plugin.json # plugins/biome-format/CHANGELOG.md # plugins/claude-ops/CHANGELOG.md # plugins/context-guard/CHANGELOG.md # plugins/eol-normalizer/.claude-plugin/plugin.json # plugins/eol-normalizer/CHANGELOG.md # plugins/go-format/.claude-plugin/plugin.json # plugins/go-format/CHANGELOG.md # plugins/guardrails/CHANGELOG.md # plugins/guardrails/README.md # plugins/instruction-placement/CHANGELOG.md # plugins/markdown-format/.claude-plugin/plugin.json # plugins/markdown-format/CHANGELOG.md # plugins/powershell-format/.claude-plugin/plugin.json # plugins/powershell-format/CHANGELOG.md # plugins/rate-limit-guard/CHANGELOG.md # plugins/ruff-format/.claude-plugin/plugin.json # plugins/ruff-format/CHANGELOG.md # plugins/source-control/CHANGELOG.md # plugins/typos-format/.claude-plugin/plugin.json # plugins/typos-format/CHANGELOG.md
kyle-sexton
marked this pull request as ready for review
September 5, 2026 22:08
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Contributor
|
Claude finished @kyle-sexton's task in 6m 27s —— View job Security review —
|
Contributor
|
Claude finished @kyle-sexton's task in 5m 54s —— View job Code review (
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c4ea4466ec
ℹ️ 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".
Contributor
|
Last security-reviewed head: |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Contributor
|
Claude has reviewed this PR 1 time. The lane skips further automatic reviews after 5; deleting this comment resets the count. |
…only
The search was unanchored, so it took the leftmost match anywhere in the
payload. Two ways that goes wrong, both reproduced before the fix:
tool_input.options.prompt_id was read as the envelope's prompt_id when the
payload carried no root prompt_id, joining telemetry to the wrong prompt;
a session_id nested ahead of the real one won outright, filing the row under
another session's id. That one is worse than a miss: the row lands in a file
that belongs to a different session.
The search is now cut at the first nested container, so a key inside
tool_input or tool_response can never be reached. The cut is deliberately
conservative: a `:{` or `:[` inside a string value truncates early and a root
key after the first container is then omitted, which the field's existing
"omitted, never guessed" rule already allows, and an omitted session_id routes
to the shared log — recoverable, where a wrong one is not.
Cutting first also shortens every one of the four searches from the whole
payload to a short prefix, so this is cheaper than what it replaces. That
matters: staying off jq here is why the emitter meets its latency budget.
Three cases pin it, and they discriminate — against the un-cut code the two
nested cases fail (PASS=298 FAIL=2) while the documented-shape case still
passes, so the cut costs nothing on a real payload.
Also corrects the hook-telemetry README: the first spine-aware reference sink
ships as claude-ops 0.42.11, not 0.42.10, which reads only data.session_id.
Both reported by Codex on #3769.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019DaWEB8Daq1xAXy2Xj1Pme
…ncation bce36d6 stopped the nested-key capture by cutting the search at the first nested container. That was wrong in a way its own test hid: the documented payload places tool_use_id AFTER tool_input, so the cut dropped it on every tool event. session-event-log.sh already says as much in its early-stop note, "tool_input closed, tool_use_id still to come", and the fixture added with the cut listed the four ids up front, so it passed while the real payload shape silently lost one. The keys are now selected by depth instead. Escapes are neutralized, the payload is split on the quote character, and the alternating fields are walked: even fields are structure, odd fields are string bodies. A string body is kept only at depth 1, so nested objects collapse to brace-and-colon rubble carrying no quotes and no nested key can match, while a root key sitting after a container is still reached. The walk costs one step per string rather than one per byte, and what it renders is small, so the four searches run over a short string whatever the payload size. Two things this gets right that the truncation did not: the documented PreToolUse payload yields all of session_id, prompt_id and tool_use_id, and a nested session_id ahead of the real one now loses to the ROOT value rather than suppressing it. Bounded, because the neutralizing passes are superlinear in the escape count: measured 4 / 13 / 38 / 486 ms per emit at 16 KiB / 64 KiB / 128 KiB / 512 KiB of escape-bearing payload. Past 64 KiB the walk is skipped and the head cut is used instead — still safe, since nothing nested is reachable, but not complete: a root key after the first container is omitted up there. With the gate, an escaped 512 KiB payload costs 22 ms instead of 486. The suite discriminates against both wrong versions: 3 failures against the un-anchored original, and 3 against the truncating fix. Answers the security review's unbounded-scan finding, whose second item is the nesting one this closes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019DaWEB8Daq1xAXy2Xj1Pme
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019DaWEB8Daq1xAXy2Xj1Pme
Contributor
Author
|
Both security-review items are addressed, and chasing the first one surfaced a defect the fix for the second had introduced. Head is now Item 2 (raw text scan, not structure-aware) was real, and worse than "speculative." Two reproductions before any change, driving
You correctly identified why the existing decoy test missed it — it plants the decoy inside an escaped string, which the literal match can't reach anyway. The first attempt at that fix was wrong, and your note about key ordering is why. Cutting the scan at the first nested container stopped the nested capture but dropped Keys are now selected by depth: escapes neutralized, payload split on the quote character, alternating fields walked, string bodies kept only at depth 1. Nested objects collapse to brace-and-colon rubble carrying no quotes, so no nested key can match, while a root key after a container is still reached. The documented PreToolUse payload now yields Item 1 (unbounded scan) was real too, and the depth walk initially made it worse. The neutralizing passes are superlinear in escape count. Measured per emit on this container, escape-bearing payloads:
The walk is gated at 65536 bytes; past it the payload takes the head cut instead. That stays safe — nothing nested is reachable at any size — but it is not complete: a root key after the first container is omitted up there, so The suite discriminates against both wrong versions: 3 failures against the un-anchored original, 3 against the truncating fix, 303/303 on the current one. New cases cover the documented key order, a nested decoy with no root key, a nested key ahead of the root one, decoys inside a root array, a multi-megabyte payload, and the size-gate boundary. Your other conclusions I re-checked and agree with: the Generated by Claude Code |
…ion-id-ji6rl5 # Conflicts: # plugins/guardrails/CHANGELOG.md
… be omitted The contract said the four keys are "copied verbatim from the hook payload when present", which is what a consumer reads to decide what a present key means. Two properties that review settled were missing from it. Root-only is a guarantee worth stating: a key of the same name nested inside tool_input or tool_response is never taken, so a tool whose own parameter is called tool_use_id or agent_id cannot put a value on the spine. A consumer can treat a present key as the harness's own rather than as tool-supplied input. Absence above 65536 bytes is a limit worth stating: selecting by depth costs more than the emitter can spend on a payload carrying a whole file, so a large payload reads only the region ahead of the first nested container, which omits tool_use_id. Absent still means absent, never guessed and never borrowed from elsewhere; session_id and prompt_id lead the payload and are unaffected. #3784 tracks closing the gap. Recorded in the contract README, the contract changelog, and the claude-ops entry, since the per-session report is what a reader sees the effect in. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019DaWEB8Daq1xAXy2Xj1Pme
kyle-sexton
pushed a commit
that referenced
this pull request
Sep 6, 2026
…er, forward from one anchor Closes #3784 `hook::emit_telemetry` selects the four spine ids by walking the payload's quote-split fields and keeping a string body only at depth 1. That walk is exact, but it was gated at 65536 bytes because the two passes that neutralize `\\` and `\"` before the split are superlinear in the escape count — 4 / 10 / 24 / 235 / 6670 ms per emit for a 16 KiB / 64 KiB / 128 KiB / 512 KiB / 2 MB escape-bearing payload, measured here. Above the gate the payload was cut at its first nested container instead: safe, but it omitted every root key sitting after that container, which is where `tool_use_id` and `agent_id` sit. So on any payload over 64 KiB — every Write or Edit of a real file — the envelope carried no per-tool-call join at all. Above the gate the payload is now read in a 16384-byte window at each END, and BOTH windows are walked forward from the payload's first byte: the head window literally, the tail window by CARRYING the head's string-and-depth state across the middle. Anchoring is the whole of the safety argument. A walk anchored at the first byte knows, for every byte it reads, whether it is inside a string and how deep it is, because a JSON document begins outside one at depth 0; nothing in the payload has to be trusted for that, and a truncated payload only ends the walk early. Reading the tail BACKWARD from the last byte is cheaper, and was written that way first. It is not sound. It needs the payload to end by closing its root, and no bounded check establishes that: a payload cut off mid-write can end in a `}` that closes a NESTED object, or in one that is really string content, and a walk that believes such an end rebuilds the raw text one quote out of step — where `tool_input`'s own `tool_use_id` matches. hook::buffer_stdin returns what it read when stdin goes idle, and the cut point moves with how much content the tool sent, so that is a reachable spoof of an audit-trail id, not a theoretical one. Both shapes are now cases in the suite. The carry is taken only when the middle proves it: the head window must end inside a string, and the middle must hold no unescaped quote and must not leave either seam inside a backslash escape. A middle that cannot leave its string carries the head's depth unchanged whatever braces it spells, because they are string content. Proving it costs one scan of the middle, so the middle is capped at 262144 bytes — a 294912-byte payload — and above the cap the head window runs alone. An unbounded scan is what the review of #3769 refused. Two further guards fell out of the rewrite and are kept: a field the walk reads as structure must hold only what can legally sit between two JSON strings, which catches an out-of-step walk however it got that way; and a depth back to 0 ends the walk, so a second concatenated document cannot contribute a root key. Splitting the whole payload instead — repairing escaped quotes in the split rather than neutralizing them first, the approach #3784 favoured — was implemented and measured before this: it does remove both superlinear passes, but it yields one field per quote, and bash reads an array element in ~4 us, so a 512 KiB escape-bearing payload cost 495 ms in the walk alone. Per emit, before against after, on the same escape-bearing payloads: 16 KiB 4/6 ms, 64 KiB 4/9 ms, 128 KiB 5/13 ms, 512 KiB 17/16 ms, 2 MB 88/59 ms. Payloads from 64 KiB to 288 KiB now carry all four ids where the old code carried two; above the cap they carry the same two, no slower. A payload within the gate is still walked whole and is unaffected, except that a malformed one (unbalanced quote) now yields no key rather than falling through to the head cut, which is what the code always said it did. lib/hook-utils.test.sh: 312 pass, with new cases for the documented order over the gate, both truncation spoofs, the carry cap's two sides, a middle that leaves its string, a payload that does not end outside a string, 5000 bytes of trailing whitespace, and the keys stranded between the windows. Synced to all 17 carrier plugins with a patch bump and a CHANGELOG entry each. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019DaWEB8Daq1xAXy2Xj1Pme
kyle-sexton
added a commit
that referenced
this pull request
Sep 6, 2026
No related issue: a documentation inaccuracy found by a post-merge verification of this topic's four PRs, small enough to fix directly rather than file. ## Summary #3769 (claude-ops 0.42.11) moved the reference sink's routing to the envelope spine, `.session_id // .data.session_id`, and left the file header saying the route is "decided by the envelope's `data.session_id`". That header is the sink's only description of its own routing rule, so a reader of the file got the pre-1.1 answer. `docs/conventions/hook-telemetry/README.md` states it correctly, which is what makes this an oversight rather than a disagreement about intent. ## Fix The header now names the spine key first and the `data.session_id` fallback second, matching line 62, and states the spine key as **optional**: contract 1.1 has a producer carry `session_id` when its payload held a well-formed one and omit it otherwise, in which case this sink takes the legacy route. Applied to both copies, `plugins/claude-ops/hooks/hook-telemetry-sink.sh` and the repo-local `.claude/hooks/hook-telemetry-sink.sh`. Those two files are not byte-identical and are not meant to be — the repo-local copy reaches `hook-utils.sh` and `session-log-lib.sh` by relative path because they are not colocated with it, and says so in its own comment. The stale header had drifted identically in both, so both needed the same correction. Comment only. No behavior change. claude-ops 0.42.16 to **0.42.17**. ## Review findings, both addressed - **Codex (P2):** the first correction said `session_id` is "carried by every 1.1 producer", which overstates a contract that makes the four correlation keys optional and has `hook::emit_telemetry` omit absent or malformed ones. Both citations checked before changing anything; reworded in `a1713a1f`, thread resolved. - **A fresh-context verifier gating this PR** found the version bump had collided: `main` shipped claude-ops 0.42.16 in #3788 while this PR was open. Renumbered to 0.42.17 in the merge commit, with #3788's released 0.42.16 entry kept intact beneath this change's. ## Verification - Comment-only proven three ways rather than asserted: comment-and-blank-stripped executable bodies compare byte-identical; `shfmt -mn` (a real shell parser) gives matching SHA-256 per file pair across base and head; every one of the 8 changed lines begins with `#`. File modes unchanged. - Behavior driven, not inferred: the sink itself run 8 times across {base, head} × {plugin copy, repo-local copy} × {spine-only envelope, data-only envelope}, each into a fresh project dir. Every base/head pair produced a byte-identical JSONL line and the same route. - `plugins/claude-ops/hooks/hook-telemetry-sink.test.sh`: PASS=41 FAIL=0. `scripts/check-changelog-parity.sh --check-bump origin/main`, `--check`, `--check-preserved origin/main`, `scripts/validate-plugins.sh`, `scripts/sync-hook-utils.sh --check`, `scripts/check-hook-wiring-liveness.sh`: all pass. shellcheck clean on both files. - `scripts/affected-tests.sh --run`: only pre-existing failures, `cache-content-check.test.sh`'s two "process budget" cases, which reproduce on unmodified `main`. ## Related - Refs #3769, which introduced the spine-first route and left this header behind; #3758, the contract it serves; #3788, which took 0.42.16. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_019DaWEB8Daq1xAXy2Xj1Pme --------- Co-authored-by: Claude <noreply@anthropic.com>
kyle-sexton
added a commit
that referenced
this pull request
Sep 7, 2026
…ons to 3 (#3779) Closes #3520 > [!IMPORTANT] > **#3520's first acceptance criterion is NOT met by this PR.** It asks for "no more than 2 external > process spawns on its common path (its own shell plus at most one `jq`)". This branch runs **4 > `execve`** on that path: the hook's own shell, one `jq` for the envelope fields, `bash` for > `scripts/context-zone.sh`, and one `jq` inside the resolver. What this PR reduces is **process > creations** (`clone`/`fork`/`vfork`), 8 to 3; the program launches are deliberately unchanged, and > the branch does not get to 2. > > Two reasons it stops at 4, both structural rather than oversight: > > 1. **Collapsing the resolver's `bash` + `jq` into the hook's own `jq` needs the shared > `lib/hook-utils.sh` helper**, which #3740 has in flight and which this shard is fenced off from. > Re-implementing the band lookup inside the hook instead would fork the single band authority > that `context-zone.sh` exists to be. > 2. **`payload.sh`'s drain loop is kept on purpose** (see the Fix section). It costs no process, but > keeping it means the payload is re-fed to `jq` rather than `jq` reading the hook's stdin > directly, which is what forces the here-string disclosed below. > > `Closes #3520` is retained only because the repo's `pr-issue-linkage` gate requires a native > closing keyword and `Refs` fails it. **A reviewer may reasonably prefer to reopen #3520 on merge, > or split the remaining 4 → 2 work into a follow-up blocked on #3740, rather than accept a partial > close.** No argument is made here for accepting it; the figure is stated so the decision is made on > the real number. ## Summary `zone-crossing-inject.sh` fires on `PostToolBatch` **and** `UserPromptSubmit`, so it draws on both the per-tool-call and the per-turn ceiling in `docs/conventions/hook-budget/README.md`. It was reported at a 73.0 s average with 10 timeouts. The plugin's own hook-cost accounting already claimed a 3-process steady fire. That claim was measured by counting commands in **command position**, which counts `jq` and `bash` *invocations*, not processes. Under `strace -f` the same path was creating **8** processes. The gap is a bash detail, not a script-logic problem. Bash elides the extra fork inside `$(...)` and execs the command in the substitution's own subshell — but only when that command carries no redirection of its own. A `2>/dev/null`, a `<<<`, or a pipeline written *inside* the substitution defeats the elision, so bash forks the subshell and forks again to run the command. A fork that never execs never reaches a command position, which is exactly why the existing budget test read **2** and passed while the kernel was making 8. Measured on this Linux host, `V=$(jq . f)` costs 1 process creation; `V=$(jq . f 2>/dev/null)` costs 2; `V=$(printf %s "$X" | jq .)` costs 3. Hoisting the redirection onto an enclosing group restores the elision: `{ V=$(jq . f); } 2>/dev/null` is back to 1. This confirms the campaign's stated cause holds for this script, but locates it more precisely than "per-field `jq` forks": there was never more than one `jq` per pass here. The cost was redirection placement around calls that were already batched. Notably, **no `hook::jq_fields` batching was needed**, so the fenced `lib/hook-utils.sh` is untouched. ## Fix Three call sites move their redirection onto an enclosing `{ ...; }` group, and the stdin payload is assigned in-process instead of captured through a command substitution: | Site | File | Before | After | |---|---|---|---| | Payload read | `hooks/payload.sh`, `hooks/zone-crossing-inject.sh` | `INPUT=$(cg::read_payload)` — a subshell to move a string between two copies of the same shell | new `cg::read_payload_to` assigns via `printf -v`; `cg::read_payload` stays and delegates, so there is one drain loop | | Envelope fields | `hooks/zone-crossing-inject.sh` | `$(printf '%s' "$INPUT" \| jq … 2>/dev/null)` — 3 processes | `{ FIELDS=$(jq …); } 2>/dev/null <<<"$INPUT"` — 1 | | Resolver call | `hooks/zone-crossing-inject.sh` | `$(bash "$RESOLVER" … 2>/dev/null)` — 2 | `{ zone=$(bash "$RESOLVER" …); } 2>/dev/null` — 1 | | Snapshot pass | `scripts/context-zone.sh` | `$(jq … "$snap" 2>/dev/null)` — 2 | group form — 1 | | `zones.json` pass | `scripts/context-zone.sh` | `$(jq … "$zones" 2>/dev/null)` — 2 | group form — 1 (off the common path) | ### Which hooks actually benefit Two, not three: - **`zone-crossing-inject.sh`** itself, on both of its routes (`PostToolBatch` and `UserPromptSubmit`). - **The `PreToolUse` zone gate**, `hooks/zone-gate.sh`. It calls `scripts/context-zone.sh` at line 91 and so inherits the resolver's internal saving (its own resolver call still carries an inner redirect and is not touched here). **The `PostCompact` marker does NOT benefit, and an earlier revision of this body and of `CHANGELOG.md` wrongly said it did.** `hooks/post-compact-mark.sh` never calls `scripts/context-zone.sh` — the resolver appears nowhere in the file — and it still reads its payload with `INPUT=$(cg::read_payload)` at line 50, a command substitution. Its process count is unchanged by this PR. Both the changelog entry and the README now say so explicitly. ### Disclosed cost: a temp file on payloads over 64KiB The saving is not free, and the charge is disk rather than CPU. Two of the five removed process creations come from replacing `printf '%s' "$INPUT" | jq` with `jq` fed by `<<<"$INPUT"`, and **a here-string is not a pipe**. Bash 5.1+ delivers one through the pipe buffer only while it fits; at or above 64KiB it writes the string to a temp file (`/tmp/sh-thd.*`) and hands `jq` that descriptor. Measured here on bash 5.2.21 under `strace -f -e trace=openat`: | Here-string size | `/tmp/sh-thd.*` opens | |---|---| | 60,000 bytes | 0 | | 65,536 bytes | 2 (create, then read) | | 204,800 bytes | 2 | The pipeline this replaced never touched disk at any size. The extracted fields are byte-identical either way, so no output changes. But a `PostToolBatch` payload carries every serialized tool result and routinely clears 64KiB, so a large fire now performs a temp-file write and read it did not perform before. **That lands on the platform this work is for:** the #3508 hosts run Defender real-time protection, which scans temp-file writes, and the 0.4.8 measurement already recorded in the plugin README attributes 22.0 s on that platform to it. The trade taken is one process creation saved on **every** fire against disk I/O on the fires that exceed the buffer, on hosts where a process creation costs 180–2,841 ms. Disclosed in three places: the plugin README's hook-cost accounting (per hook-budget Rule 1 and `.claude/rules/hook-budget.md`), the `0.7.44` changelog entry, and the code comment at the call site — which previously claimed the group rewrite "changes nothing else". ### Why the drain loop is kept `payload.sh`'s drain loop is **kept**, not bypassed. An earlier revision of this body justified that by saying piping stdin straight to `jq` "would have removed another process". **That is no longer true and has been corrected:** after the `cg::read_payload_to` conversion the loop is `read` builtins only and costs zero processes. What piping stdin straight to `jq` would actually avoid is the here-string temp file above. The decision stands on its real merit: the loop's bounded `read -t 5` caps a stalled pipe at five seconds instead of letting it block to the harness timeout, which is the very symptom this campaign is about. Disk I/O on oversized payloads is the smaller of the two costs. `lib/hook-utils.sh` and every `plugins/*/hooks/hook-utils.sh` copy are untouched — verified in CI-visible form by `git diff origin/main --name-only`. ## Verification **Before/after, measured here** (`strace -f -e trace=clone,clone3,fork,vfork,execve`, steady non-crossing path, primed state dir): | Steady fire | Process creations | Program launches (`execve`) | |---|---|---| | `origin/main` | **8** | 4 | | This branch | **3** | 4 | **Program launches are unchanged at 4** — the same `jq`, `bash` and `jq` still run over the same inputs. That is the evidence this is a latency fix and not a work-removal fix: only the fork overhead around the calls is gone. It is also why #3520's 2-spawn criterion is unmet; see the note at the top. **Wall-clock is not measurable on this host, and I am not inventing a figure.** The cost in #3508 is Windows process-creation tax (180–2,841 ms per spawn, median 1,108 ms at 501 concurrent processes, against ~1% user CPU). This runner is Linux, where a spawn is ~1 ms, so the whole effect is below noise. The proxy reported is the drift-immune criterion #3508 itself specifies: **process-creation count by trace**. At the #3508 median spawn cost, 5 fewer process creations is on the order of 5.5 s per fire; at that issue's floor, ~0.9 s. Both need confirming on the affected Windows host before any acceptance criterion can be signed off — this PR does not claim that measurement, and it does not net out the temp-file I/O disclosed above. **Proof the behaviour did not change:** - All 144 pre-existing assertions across the two directly affected suites still pass unmodified (63 in `zone-crossing-inject.test.sh`, 81 in `context-zone.test.sh`), including partial-write recovery, the armed-rank hysteresis, fail-open paths, and the malformed-`zones.json` stderr notices. - `execve` count and targets are byte-identical before and after. - New behaviour tests target the specific risk this change introduces. A group redirect covers more than an inner one, so the hazard is over-suppression or a resolver notice leaking into the hook's stdout and corrupting the single JSON document the harness parses. A malformed `zones.json` is the one input that drives the resolver's stderr on this route: it pins that the notice reaches **neither** of the hook's streams, that stdout stays one parseable JSON document, and that shipped default bands still resolve and inject. An unparseable payload pins that the payload pass's nonzero status still propagates out of its new enclosing group rather than being absorbed by it. **New regression test, verified non-vacuous.** `strace -f` pins process creations at exactly 3 and program launches at exactly 4, so a fork saving cannot be confused with work removed. Mutating a copy of the tree to move one redirect back inside its substitution takes the count to 4 and fails with `steady path creates 4 processes, budget is 3`, while the old command-position budget passes silently. The assertion skips where `strace` is unavailable (containers, macOS), and the pre-existing command-position budget still runs there. **Commands run in the foreground on the current head, with actual results:** ``` bash scripts/affected-tests.sh --explain -> selects 4 suites (post-compact-mark, zone-crossing-inject, zone-gate, context-zone) 3 no-suite files recorded in scripts/affected-tests-no-suite.txt (plugin.json, CHANGELOG.md, README.md) bash scripts/affected-tests.sh --run (foreground) -> PASS: plugins/context-guard/hooks/post-compact-mark.test.sh PASS: plugins/context-guard/hooks/zone-crossing-inject.test.sh PASS: plugins/context-guard/hooks/zone-gate.test.sh PASS: plugins/context-guard/scripts/context-zone.test.sh All 4 selected suites passed or were skipped. EXIT=0 bash plugins/context-guard/hooks/zone-crossing-inject.test.sh -> PASS=72 FAIL=0 bash plugins/context-guard/scripts/context-zone.test.sh -> PASS=81 FAIL=0 bash scripts/check-changelog-parity.sh --check -> rc=0 bash scripts/check-changelog-parity.sh --check-order -> rc=0 (89 changelogs) bash scripts/check-changelog-parity.sh --check-bump origin/main -> rc=0 bash scripts/check-changelog-parity.sh --check-preserved origin/main -> rc=0 (68 headings compared) shellcheck <4 changed shell files> -> clean shfmt -d <4 changed shell files> -> clean bash scripts/check-shell-portability.sh origin/main -> No unexcused GNU-only constructs in 4 shell file(s). ``` Manifest bumped 0.7.43 → 0.7.44 with a matching changelog entry; the accuracy corrections land inside that same unreleased entry, so no further bump was needed (`--check-bump` and `--check-preserved` both confirm). Not mine and pre-existing: `test_save_point.py::test_new_origin_falls_back_to_directory_name` fails identically on pristine `origin/main`. ## Related - Parent campaign: #3508 — Windows process-creation tax. This is the one shard of thirteen with zero `hook::buffer_stdin` call sites, so it was fixable in-file without serializing behind the shared `lib/hook-utils.sh` work. - #3740 has `lib/hook-utils.sh` changes in flight; this PR deliberately does not touch that file or any of its 17 synced copies. **It is also the blocker on the remaining 4 → 2 `execve` work** described at the top of this body. - Prior art #1403 / #1385 (`hook::jq_fields`, `strip_quoted_spans`, the 65-spawn census). Not revived here and not needed: the batching those introduce was already present in this hook, and the remaining cost was redirection placement rather than call count. - Checked for collision against #3769 (hook-telemetry envelope keys) and #3762 (guardrails PostToolUse dispatcher) — neither touches any `context-guard` file. - Budget authority: `docs/conventions/hook-budget/README.md`, surfaced by `.claude/rules/hook-budget.md`. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01ViPsHkL3ng9xWt2GjEQJob --------- Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: ksextonmelodic <ksextonmelodic@gmail.com>
kyle-sexton
added a commit
that referenced
this pull request
Sep 7, 2026
… off substitutions (#3851) Closes #3512 ## Summary The Stop-event unsurfaced-failure detector (`plugins/claude-ops/hooks/hook-failure-audit.sh`) timed out 113 times in the #3508 window at a 21.9 s average, roughly 44x the 500 ms the hook-budget convention gives the whole always-on per-turn set. **The parent issue's stated cause is wrong for this hook.** #3508 blames per-field `jq` forks needing a new shared helper. Shard #3520 (PR #3779) established that the real cost is *redirection placement*, and merged PR #3788 fixed 34 hook scripts across 17 plugins while touching `lib/hook-utils.sh` zero times. This shard applies the same diagnosis. **No shared library is touched** — the fence around `lib/hook-utils.sh` and its 17 synced copies (unmerged #3740, #3838) holds. The mechanism: bash runs the command of a command substitution in the substitution's own subshell and skips the extra fork **only when that command carries no redirection of its own**. `$(wc -c <file 2>/dev/null)` is two processes for one byte count; `$(cat -- file 2>/dev/null)` is two for a file read; `{ V=$(cmd); } 2>/dev/null` is one; `$(<file)` is zero. Those forks are invisible to `bash -x`, which reads command positions — which is why the campaign kept mis-attributing the cost to the exec count. ## Fix Six in-file sites, all in `hook-failure-audit.sh`: | Site | Before | After | |---|---|---| | Transcript size | `SIZE=$(wc -c <"$TRANSCRIPT" 2>/dev/null)` | `wc` names the file, `read` drops the filename column and any padding, `2>/dev/null` rides a single-command group | | Pre-filter, under the cap | `read_window \| grep -F …`, where `read_window` was `cat -- file 2>/dev/null` | `{ RECORDS=$(grep -F … -- "$TRANSCRIPT"); } 2>/dev/null` — grep opens the transcript itself; the helper function was a second subshell on top of the substitution's own | | Marker read | `$(cat -- "$MARKER" 2>/dev/null)` | `$(<"$MARKER")` — no subshell, no exec | | Marker write | `jq … 2>/dev/null \| tr -d '\r' >>"$MARKER"` | `$(jq …)` then in-shell `${VAR//$'\r'/}` with a builtin `printf` append | | Marker directory | `mkdir -p "$MARKER_DIR" 2>/dev/null` every warned turn | `[[ -d … ]] ||` first; `mkdir -p` on an existing directory was a whole process to reach the same no-op | | Payload fields | two `hook::jq_field` calls | one `hook::jq_fields` call (the batching helper the library already ships, used by 10 other hooks) | The CHANGELOG entry originally said "Five sites" over that same list of six; it now says six. Deliberately **not** changed, both documented in the file: - **Over the tail cap**, `tail | sed '1d' | grep` and its `2>/dev/null` on `tail` are untouched. A pipeline element forks either way, so hoisting the redirect onto a group would newly silence `sed` and `grep` for no saving, and merging `sed` into `grep` would change the matching semantics. - **`printf '%s' "$RECORDS" | jq -cRs`** stays a pipeline rather than becoming a here-string, and the in-file comment now states the grounds as they actually stand. `hook::jq_field` in the shared library documents this hazard and refuses the here-string form for it: bash fills a here-string's pipe itself, so a payload at or above the pipe capacity can block before `jq` is exec'd. The trace behind that note (#1587: 65536 bytes hung indefinitely while 65000 returned at once) comes from this repo's **Windows Git Bash** hosts. It does **not** reproduce on Linux bash 5.2 — re-checked here at 65535, 65536, 65537, 200 kB and 2 MB, each returning immediately, including under an unwritable `TMPDIR`. The call keeps the library's conservative form anyway rather than bet the hazard is Linux-only: the forgone saving is one fork on the warning path only, since a turn with no failure record exits before that line. Earlier revisions of this PR and of the comment asserted the deadlock as universal fact, which is more than is known. ## Verification **Process counts**, measured with `strace -ff -qq -e trace=clone,clone3,fork,vfork,execve` (`-ff` so no syscall line is split across an `<unfinished>`/`<resumed>` pair), successful `execve` only, the harness's own top-level `bash <hook>` exec excluded: | Path | Creations before | after | execs before | after | |---|---|---|---|---| | No failure recorded, under the tail cap (the common case) | 18 | **9** | 6 | **4** | | No failure recorded, over the tail cap | 19 | **12** | 7 | **6** | | A turn that warns | 34 | **24** | 17 | **14** | The remaining 9 creations on the common path are 4 execs (`wc`, `grep`, and two `jq` passes inside the synced `hook-utils.sh`) plus subshell forks inside that same fenced library. The `execve` drop is one batched `jq` and two removed helper processes — removed *processes*, not removed work: every input is still read and every record still classified. **No wall-clock figure is claimed.** This Linux host says nothing about the Windows spawn tax the budget binds to, and on the #3508 host one process creation costs 180-2,841 ms (median 1,108 ms at 501 concurrent). The process count is the honest proxy and the README says so. **Behaviour is unchanged, proven, not asserted.** This hook's whole job is surfacing failures that otherwise pass unnoticed, so a carelessly hoisted `2>/dev/null` could silence exactly the diagnostic it exists to emit. Every hoisted group holds one command. The only stream now silenced that was not before is `grep`'s own stderr, and `cat`'s own `2>/dev/null` already discarded that same stream on the same path. Pre- and post-change stdout, stderr, exit codes **and marker-file contents** were compared byte for byte across ten scenarios — first warn, dedup, a new failing registration re-warning while the already-warned one stays muted, all three classification branches (launch / ambiguous / completed), the tail cap, a clean transcript, a missing transcript, the kill switch, and no data directory. Identical. **Budget test, mutation-checked.** `hook-failure-audit.test.sh` gains an strace-based assertion on both the creation and exec ceilings, skipping cleanly where strace is unavailable or not permitted. Non-vacuity was demonstrated, not assumed: moving either silenced redirect back inside its substitution — `SIZE=$(wc -c <"$TRANSCRIPT" 2>/dev/null)`, or `RECORDS=$(grep -F … -- "$TRANSCRIPT" 2>/dev/null)` — pushes creations from 9 to 10 with **no change to the exec count**, leaves all 93 behavioural assertions green, and fails the budget assertion. That is precisely the fork xtrace cannot see. **Gates, all foreground:** | Gate | Result | |---|---| | `scripts/affected-tests.sh --run` | exit 0, both selected suites pass (`hook-failure-audit.test.sh` 94/94, `audit-session-id.test.sh` 27/27) | | `shellcheck -x` on both changed shell files | clean | | `shfmt -d -i 2 -ci` | clean | | `scripts/check-changelog-parity.sh` `--check` / `--check-order` / `--check-bump origin/main` / `--check-preserved origin/main` | all four pass | | `markdownlint-cli2` on the changed markdown | clean | | `scripts/check-purged-em-dashes.sh` | pass | | `scripts/check-shell-portability.sh origin/main` | pass | | `scripts/check-silent-skips.sh`, `check-killswitch-hoist.sh`, `check-hook-exec-form.sh`, `check-hook-wiring-liveness.sh`, `check-cross-plugin-source-drift.sh`, `validate-plugins.sh` | pass | The wording-only follow-up commit re-ran the test suites, all four parity modes, `shellcheck -x`, `shfmt -d -i 2 -ci` and `markdownlint-cli2`: unchanged, including the 9-creation / 4-exec budget assertions, which is the expected result for a comment-and-prose change. `test_save_point.py::test_new_origin_falls_back_to_directory_name` is pre-existing and not in this diff's selection. ### Acceptance criteria not fully met Two of the issue's criteria are **not** satisfied, and no amount of in-file work would satisfy them: - **"A turn with no hook failures exits before any external process spawn."** Not met, and not reachable. The hook learns there is nothing to report *by reading the transcript*; there is no cheaper oracle. The floor without touching the fenced `hook-utils.sh` or abandoning the O(cap) tail bound is one `wc`, one `grep`, and the library's own payload parse — which is what this PR reaches. If the criterion is to be met literally it needs a different design (a marker written by the failing hook, or consolidation with #3515/#3516), not a further micro-optimization here. - **"The always-on per-turn set stays within <= 500 ms parallel wall."** Not measured. That figure is cross-plugin (this hook shares the budget with #3515 `autonomy` and #3516 `disk-hygiene`) and binds to Windows Git Bash, which this runner is not. The README records the figure as owed rather than implying it was taken. A third is met in a different form than specified: the issue asks for a **PATH shim** spawn census. This uses **strace** instead, which is strictly stronger for this defect — a PATH shim sees only programs that are `exec`'d, and the whole cost here is subshell forks that never exec anything. ## Related - Parent: #3508 (the campaign; its stated cause is corrected above for this hook) - Precedent: #3520 / PR #3779 (established redirection placement as the real cost), PR #3788 (34 scripts, 17 plugins, zero library edits) - Siblings on the same 500 ms per-turn budget: #3515 (`autonomy`), #3516 (`disk-hygiene`) - Fenced, deliberately untouched: #3740, #3838 (`lib/hook-utils.sh` and its 17 synced copies); the here-string note above cites `hook::jq_field` there but changes nothing in it - **Adjacency checked, no edit-surface overlap:** open PR #3769 adds correlation keys (`session_id`, `prompt_id`, `tool_use_id`, `agent_id`) to the hook-telemetry envelope spine inside `hook::emit_telemetry`. It does **not** touch `hook-failure-audit.sh`. This diff leaves that hook's `SESSION_ID` extraction and its `data.session_id` envelope key exactly as they are, so nothing here collides with the key migration. The two PRs both bump `plugins/claude-ops/.claude-plugin/plugin.json` and prepend to its CHANGELOG, so whichever lands second takes a trivial version/heading rebase. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01ViPsHkL3ng9xWt2GjEQJob --- _Generated by [Claude Code](https://claude.ai/code/session_01ViPsHkL3ng9xWt2GjEQJob)_ --------- Co-authored-by: Claude <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #3758
Its two bases (#3765, #3762) have merged,
mainis merged in, and the diff is reduced to this change alone.Summary
The reference sink files an envelope per session only when the envelope carries a session id, and only the nine claude-ops audit hooks sent one, so the per-session report's "hooks fired" table never listed the formatters or the guards. The library now puts the payload's correlation keys on the envelope spine for every producer, at contract 1.1.
Fix
lib/hook-utils.sh(synced into its 17 carriers):hook::emit_telemetryreads the payload the producer buffered (HOOK_TELEMETRY_PAYLOADwhen set, elseINPUT, the variable every fleet hook assigns fromhook::buffer_stdin) and copiessession_id,prompt_id,tool_use_idandagent_idonto the envelope betweenduration_msanddata, each only when present as a plain id ([A-Za-z0-9._-]+), on both the builtin and the jq path. No jq, no subprocess, no producer change.schema_versionreads1.1.docs/conventions/hook-telemetry:envelope.schema.jsongains the four optional properties; the README gains a "Correlation keys" section and rewrites the sink-routing note; the contract CHANGELOG records 1.1 as an additive minor.plugins/claude-ops/hooks/hook-telemetry-sink.shand the repo-local.claude/hooks/copy): routes on the spinesession_idfirst and falls back todata.session_id, so envelopes from 1.0 producers keep their route.The keys are selected by depth
Review found that searching the raw payload takes the leftmost match anywhere in it. Two failures, both reproduced against the first version of this branch:
{"session_id":"sess-real","tool_input":{"options":{"prompt_id":"NESTED-WRONG"}}}emitted"prompt_id":"NESTED-WRONG".{"tool_input":{"n":{"session_id":"NESTED-WRONG"}},"session_id":"sess-real"}emitted"session_id":"NESTED-WRONG"— not a mis-join, the row lands in another session'ssessions/<id>.jsonl.Cutting the search at the first nested container fixed those and broke something else: the documented payload places
tool_use_idaftertool_input(session-event-log.shsays so in its early-stop note, "tool_input closed, tool_use_id still to come"), so the cut dropped it on every tool event, and the fixture added alongside listed the four ids up front so it passed anyway.So the keys are selected by depth instead. Escapes are neutralized, the payload is split on the quote character, and the alternating fields are walked — even fields structure, odd fields string bodies. A string body is kept only at depth 1, so nested objects collapse to brace-and-colon rubble carrying no quotes and no nested key can match, while a root key after a container is still reached. The walk costs one step per string, not one per byte, and what it renders is short, so the four searches run over a small string whatever the payload size.
Bounded, and the gap is filed
The neutralizing passes are superlinear in escape count. Per emit, escape-bearing payload, this container:
The walk is gated at 65536 bytes; past it the payload takes the head cut. That stays safe at any size — nothing nested is reachable — but it is not complete: a root key after the first container is omitted above the gate, so
tool_use_idis dropped on payloads over 64 KiB.session_idandprompt_idlead the payload, so routing is unaffected. #3784 carries that gap with these measurements and two candidate approaches, and the code comment points at it.Verification
lib/hook-utils.test.sh303/303. The suite discriminates against both wrong versions: 3 failures against the un-anchored original and 3 against the truncating fix. New cases cover the documented key order (tool_use_idaftertool_input), a nested decoy with no root key, a nested key ahead of the root one, decoys inside a root array, a multi-megabyte payload, and the size-gate boundary.hook-telemetry-sink.test.sh41/41,api-error-audit.test.sh11/11,run-guards.test.sh99/99,claude-observability.test.sh60/60,lib/rewrite-guard.test.sh22/22.scripts/affected-tests.sh --run: one failure, not this PR's —block-hook-bypass.test.sh, "symlink: a genuine temp write in the same root stays allowed". Reproduced identically on unmodifiedmainat73eb4d98in a clean worktree (PASS=601 FAIL=1).scripts/sync-hook-utils.sh --check,scripts/sync-rewrite-guard.sh --check,scripts/check-changelog-parity.sh --check-bump origin/main,--check,--check-preserved origin/main,scripts/validate-plugins.sh,scripts/check-shell-portability.sh origin/main,scripts/check-killswitch-hoist.sh: all pass. shellcheck clean; the one shfmt hunk is pre-existing (maincarries 49, this branch 1, and it is not in the changed region).Renumbered three times
#3770 (fleet-wide prompt audit) bumped 64 plugins and took eight of the numbers this branch claimed; #3762 and #3765 then took guardrails 0.32.5 and claude-ops 0.42.10; #3781 then took guardrails 0.32.6. Every one of the seventeen was re-derived as the next patch above
origin/mainand verified againstgit show origin/main:plugins/<p>/.claude-plugin/plugin.jsonafter each move. Each CHANGELOG conflict was resolved by keeping the released entry at its own heading and lifting this change's entry above it;--check-preserved origin/mainpassing is what proves no released entry was dropped.Related
🤖 Generated with Claude Code
https://claude.ai/code/session_019DaWEB8Daq1xAXy2Xj1Pme