fix(guardrails): restore PreToolUse enforcement — guards were killed at their timeout - #1385
fix(guardrails): restore PreToolUse enforcement — guards were killed at their timeout#1385kyle-sexton wants to merge 2 commits into
Conversation
…at their timeout (#1345) The guardrails PreToolUse guards were not slow; they never finished. Every one was killed by the harness at its declared `timeout`, and a cancelled hook contributes no `permissionDecision`, so the guarded tool call proceeded without the guard's verdict. Measured over 3,923 PreToolUse runs, the overall non-enforcement rate was 86.1%; individual guards ran 45-100% killed and several never completed once. Two compounding causes, both addressed: Process-spawn cost. Each guard spawned 7-18 external processes through MSYS fork emulation under Defender real-time scanning - 65 for a single Bash tool call. The per-field `printf | jq | tr` pipelines collapse into one `jq` process per guard via a new `hook::jq_fields`; `hook::buffer_stdin`'s CR strip becomes parameter expansion; `flag-commit-pr-skill-bypass`'s per-line `sed` pair becomes a whole-string parameter-expansion pass; `hardcoded-path-check` answers two gates with one `git` process. Exact census, identical payloads: 65 -> 35 spawns per Bash tool call, and 24 -> 11 across the two content guards. Declared timeouts far below the platform default. Per current docs the `timeout` default is 600s for command hooks, lowered only by UserPromptSubmit and MessageDisplay - not PreToolUse. The 10s/15s values were a 60x/40x reduction with no recorded rationale. Raised to 60s/90s, still 10x and 6.7x below the platform default; derivation is in the guardrails CHANGELOG. No detection logic changed. The `strip_literals` rewrite - the one edit that could silently weaken a guard - was proved byte-identical to the previous `sed` pair on 20,041 inputs, including 20,000 fuzzed over a quote-heavy alphabet. `hook::jq_fields` strips U+001E from every value inside jq before joining, so payload text cannot shift field alignment and cause a guard to scan the wrong bytes. The shared lib change reaches all 13 carrying plugins through `scripts/sync-hook-utils.sh`; each carrier is version-bumped so consumers actually receive it.
|
Claude encountered an error after 7m 9s —— View job PR Review: fix(guardrails): restore PreToolUse enforcement
|
|
Claude encountered an error after 6m 22s —— View job Security Review in Progress
|
Three branch-owned hygiene sub-checks failed on this PR: - typos: `mis-assign` in the hook::jq_fields rationale split on the hyphen, so `mis` was flagged as `miss`/`mist` in lib/hook-utils.sh and all 13 vendored plugin copies. Dropped the hyphen (`misassign`) rather than allowlisting: _typos.toml is synced verbatim from the root-canonical policy and documents itself as not locally editable. Propagated with scripts/sync-hook-utils.sh; no version bumps needed, since every carrying plugin already bumped on this branch. - shellcheck: SC1003 on the `'\')` case pattern in flag-commit-pr-skill-bypass.sh's quote stripper. Replaced with the unquoted `\)`. Both forms match exactly one literal backslash, so the escape-skip branch is unchanged; `'\')` would have matched two and silently broken the stripper. - exec-bit: hook_latency_report.py carried a shebang at mode 100644. Set 100755, matching every other shebanged .py script in the repo. Verified locally: typos clean, shellcheck clean over all 349 tracked scripts, sync-hook-utils.sh --check and check-cross-plugin-source-drift.sh both clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Claude finished @kyle-sexton's task in 4m 47s —— View job PR Review: fix(guardrails): restore PreToolUse enforcement
VerdictThe diff is correct and safe to merge. The central fix is sound: the previous 10 s / 15 s timeouts were 60× / 40× below the platform default with no recorded rationale, hooks were killed before they ran, and a killed PreToolUse hook fails open (confirmed from the docs — exit-from-timeout is a non-blocking outcome). No detection logic is weakened anywhere. Two items follow: one unclosed concern from the PR's own evidence list (MEDIUM), one new LOW finding. Core correctness — verified
Traced edge cases manually against
No detection logic weakened Confirmed across all hooks in scope. Regex patterns, block conditions, allow-list paths, and per-field extraction values are semantically identical to pre-change code. The changes are limited to: (a) collapsing per-field jq calls into Timeout arithmetic From the CHANGELOG derivation: worst-case guard is 12 spawns (flag-commit-pr-skill-bypass) × 437 ms upper bound + 380 ms sourcing ≈ 5.6 s. × 3 concurrent-hook-density effect ≈ 16.9 s. × 3 headroom ≈ 50.6 s → 60 s (smallest multiple of 10 covering it). 1.5 ratio → 90 s for content-scanning guards. The multipliers (3× density, 3× headroom) are measured upper bounds documented as estimates; the resulting values stay 10× / 6.7× below the platform default. Conservative and defensible. Sync delivery — verified All 14 copies of CLAUDE.md design-rule compliance All rules pass: repo-agnostic (uses Security analysisNo new injection surfaces. Specific checks:
FindingsMEDIUM — plugins/guardrails/hooks/flag-commit-pr-skill-bypass.sh#L205 The CHANGELOG claims byte-identical output on 20,041 inputs (41 hand-written + 20,000 fuzz), but neither the test runner nor the corpus is in the PR. Code analysis is consistent with equivalence (see above), and the blast radius is advisory-only — a divergence would cause an advisory to fire or not fire, not a block/allow outcome. Whether to require the harness before merge or as follow-up is an owner call; both are defensible. Fix this → LOW — plugins/guardrails/scripts/hook_latency_report.py#L33 for line in open(path, encoding='utf-8', errors='replace'):
INFO — CONTENT parsed before skip guards in Write hooks
Operator ratification items (unchanged from PR's disclosure)
|
|
Claude finished @kyle-sexton's task in 6m 16s —— View job Security Review — PR #1385
Security scope: This review covers only the files changed in this PR. Findings marked with severity (CRITICAL / IMPORTANT / SUGGESTION) and confidence level. Primary change: timeout restorationThe core fix — raising timeouts from 10 s / 15 s to 60 s / 90 s so hooks complete before the harness kills them — is a security improvement. Timed-out PreToolUse hooks yield no block decision and the tool call proceeds unguarded. The prior timeouts made enforcement structurally impossible. No security concerns with this change.
|
![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: d54180fe42
ℹ️ 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".
| 'tool_use_id': att.get('toolUseID'), | ||
| 'date': ts[:10], | ||
| }) | ||
| fanout[(path, att.get('toolUseID'))] += 1 |
There was a problem hiding this comment.
When a transcript contains both PreToolUse and another hook event for the same tool use, fanout is incremented for all attachments before rows is filtered by --event, so the default PreToolUse report combines sequential PostToolUse hooks into its hooks-per-tool-call histogram. This overstates the concurrent hook density used to diagnose latency; derive the histogram from the event-filtered rows instead.
Useful? React with 👍 / 👎.
| def pct(values, p): | ||
| values = sorted(values) | ||
| return values[min(len(values) - 1, int(len(values) * p))] |
There was a problem hiding this comment.
For sample sizes divisible by ten, int(len(values) * .9) selects the observation after the 90th-percentile nearest rank; for example, ten completed durations numbered 1–10 report a p90 of 10 rather than 9. This systematically overstates p90 for common run counts and can distort timeout tuning, so use ceil(n * p) - 1 or an explicit standard quantile implementation.
Useful? React with 👍 / 👎.
|
Closing this rather than merging it. Recording why, because the diagnosis behind it is sound and the What settled itA fresh-context verification of this PR against the guardrails contract suites returned DO NOT
On a safety layer, absence of evidence is itself the finding. Four contract-test regressions were also measured on Windows against the post-change tree, two of plus What did land, and what did notThe timeout half of the fix landed as #1379, which is now merged. #1379 touches only The spawn-reduction half did not land, and #1379 alone does not fix the defect — it narrows the Not carried forward silentlyTwo things in this diff are recorded on #1403 rather than lost: Verified before closing: #1345 is closed (by #1379), so #1403 is the live home for the remaining |
|
Closed — see the disposition comment above. The spawn-reduction work is tracked as #1403. |
The execution record described lanes by what sat uncommitted in a worktree, which is unreachable from a clone and therefore not a citation. Every lane has since published, so each row now names the PR or issue that carries its result: L2 as #1343, L3 as #1349 with the UNBACKED-to-OPINION mapping stated so the claim set does not need to survive as a separate list, L4 as #1385 closed with recovery on #1403, L1 as #1286 closed with the fold on #1271 and gate defects on #1404. The source article is named with its URL rather than left as "a practitioner article", so a reader auditing this ADR's premises can reach the thing the digests measured. The digests themselves prune with the contract slice, which is why the pointer replaces them rather than supplementing them.
…or three (#2120) No linked issue `hook::jq_fields` landed in #1979 and got its first two adopters in #2007 (`block-dangerous-git`, `block-no-verify`). The other **ten** guardrails hooks were still parsing their PreToolUse/PostToolUse payload with a separate `printf '%s' "$INPUT" | jq -r … | tr -d '\r'` pipeline **per field**, over the same already-buffered stdin envelope. This converts all ten. ## Survey — what was still forking per field Counting only `jq` **execs against the buffered payload**. `jq -n` envelope builders, `jq -R | jq -s` finding serializers, and jq reading a file from disk are out of scope and untouched. | hook | jq execs on payload, `main` | after | fields | | --- | --- | --- | --- | | `block-noncanonical-commit` | 3 | 1 | `command`, `cwd`, `tool_name` | | `block-convention-violation` | 3 | 1 | `tool_name`, `command`, `cwd` | | `hardcoded-path-check` | 3 | 1 | `tool_name`, `file_path`, `content`/`new_string`/`new_source` | | `secret-pattern-detection` | 3 | 1 | same as above | | `skill-reference-verify` | 3 (Edit) / 2 (Write) | 1 | `tool_name`, `new_string`, `replace_all` / `content` | | `stale-path-verify` | 3 (Edit) / 2 (Write) | 1 | same as above | | `block-hook-bypass` | 2 | 1 | `command`, `tool_name` | | `flag-commit-pr-skill-bypass` | 2 | 1 | `command`, `tool_name` | | `cli-flag-verify` | 2 | 1 | `tool_name`, `new_string`/`content` | | `workflow-resilience-check` | 2 | 1 | `script`, `scriptPath` | | `block-dangerous-git` | 1 | 1 | already converted by #2007 | | `block-no-verify` | 1 | 1 | already converted by #2007 | Collateral, not claimed as the headline: each old line is three process creations (`$( )` subshell + `jq` + `tr`), so a 3-field hook went 9 → 3 and a 2-field hook 6 → 3 — the `tr -d '\r'` per field disappears too, because `hook::jq_fields` strips CR shell-side. ### Deliberately NOT converted **`hook::read_file_path`** — `cli-flag-verify`, `skill-reference-verify` and `stale-path-verify` each still pay one jq exec there. Folding `file_path` into the batched call would mean either duplicating or restructuring that helper's existence + project-membership validation, and it lives in the synced shared lib (`lib/hook-utils.sh` → 13 plugin copies + the CI drift check), so the blast radius reaches every plugin for one exec. Left alone on purpose. **`flag-commit-pr-skill-bypass`'s `enabledPlugins` reads** (two jq calls at L145/L155) read a settings **file**, not the payload. Different input, not batchable here. ## How the fields were kept byte-identical Two spots would have changed behavior under a naive conversion, and both are handled: 1. **`.tool_name // "Bash"`** — the default moves to the shell side (`TOOL_NAME="${HOOK_JQ_FIELDS[n]:-Bash}"`), matching `block-dangerous-git`. 2. **`replace_all`** keeps `// false | tostring` **inside** the filter. `hook::jq_fields` wraps every filter in `// ""`, and jq's `//` treats the boolean `false` as empty — so a bare `.tool_input.replace_all` returns `""` where the old call returned `"false"`. Verified against all three input shapes (absent / `false` / `true`): ```text value=null filter=.tool_input.replace_all new=[] old=[false] value=null filter=.tool_input.replace_all // false | tostring new=[false] old=[false] value=false filter=.tool_input.replace_all new=[] old=[false] value=false filter=.tool_input.replace_all // false | tostring new=[false] old=[false] value=true filter=.tool_input.replace_all new=[true] old=[true] value=true filter=.tool_input.replace_all // false | tostring new=[true] old=[true] ``` **Failure semantics are unchanged in every hook.** `hook::jq_fields … || exit 0` lands on exactly the skip the old empty-field guard produced — each hook's statement right after its first old jq call was already `[[ -n "$X" ]] || exit 0` or a `case … *) exit 0`. `hook::require_jq` still runs first and still makes a missing jq visible once per session. **One trade stated plainly.** In `hardcoded-path-check` and `secret-pattern-detection` the per-tool content field is now serialized in the first call, i.e. BEFORE the file-path exclusions and the `git check-ignore` skip that used to precede it. On a skipped write that is one extra copy out of jq of a payload already buffered in memory, traded for one fewer process on every path. Process creation, not jq's parse, is the cost centre on the host this targets. ## Measurement **Method.** Two checkouts — arm A at `origin/main`, arm B this branch — with the arms **interleaved inside one loop**, alternating which runs first each iteration, so both arms share one load sample. Compared as **paired deltas** (`B_i − A_i`), summarized by median and quartiles. Never "50× A, then 50× B": one instrumented fork on this host has been recorded swinging 93 ms → 3234 ms, so a single sequential before/after pair proves nothing. **Machine load — every number below was taken under load, and is labelled as such.** This box runs several agents concurrently. Snapshot during the runs: `cpu_pct_avg=20.3`, `procs_total=405`, `bash_procs=18`, `free_mem_gb=31.4`. Windows 11, Git Bash (`GNU bash 5.3.15 x86_64-pc-cygwin`), `jq-1.8.2`. Load is why absolute per-arm times below run into seconds; it is also why the **medians are inflated relative to a quiet box** and the conservative statistics are the headline. **Headline, conservative — p75 (least-favourable quartile) of the paired deltas:** | conversion shape | p75 | median | min-of-arms floor | NEW faster in | | --- | --- | --- | --- | --- | | 3 fields → 1 (run 1, N=100) | **-404 ms** | -1033 ms | — | 91/100 | | 3 fields → 1 (run 2, N=100) | **-449 ms** | -991 ms | -394 ms | 95/100 | | 2 fields → 1 (N=100) | **-194 ms** | -274 ms | -192 ms | 87/100 | Run 1 was reproduced by run 2 to within 45 ms at p75 and 42 ms at the median — the point the task brief makes about a "PASS=154 FAIL=0" claim from a single run that did not reproduce. Run 1's raw samples were not retained to a file (its summary line is quoted above); **runs 2 and the 2-field run have every sample below**, and either alone carries the claim. The p75 and the independently-computed floor (fastest observed A minus fastest observed B, i.e. the least-contended sample of each arm) agree to within 10 ms in both shapes. Two conservative estimators converging is the strongest claim here; the medians are the same effect amplified by contention. **End-to-end, whole-hook** — `block-noncanonical-commit.sh` invoked as a process, N=60 interleaved: **median paired delta -687 ms**, range -13039 ms to +11774 ms. Reported deliberately even though it is noisier and *smaller* than the isolated 3-field median: the parse block cannot recover more than the whole hook does, and omitting the weaker own-number is what makes a stronger one look selected. **Against the prior model.** A previous session's model predicted ~280 ms recovered and the handoff recorded "the measured-versus-model gap says expect LESS." Stated plainly: the conservative 2-field number (**-194 ms**) is **under** that model, and the conservative 3-field number (**-404 ms**) is **over** it. The model was a single figure for a range of shapes. Every sample is in the collapsed sections below. ## Behavior verification ### Payload-level differential vs `origin/main` — 62/62 identical Issue #1403 records that the previous extraction attempt (#1385) regressed on **multi-line command values** — four suites failed, all on multi-line payloads. That is the exact risk class for this change, so it is tested directly: the same payload fed to the `origin/main` copy and the converted copy of each hook, requiring **identical exit code, identical stdout and identical stderr**. Cases: plain command, backslash-newline continuation (`git commit --no\<newline>verify`), multi-line `-m` body, escaped quotes, embedded tab, PowerShell here-string, stdout-redirect write, `gh pr create`, empty command; Write/Edit/NotebookEdit multi-line content, unmatched tool, empty content; `replace_all` true/false; Workflow inline-script / `scriptPath`-only / neither. **Result: `DIFFERENTIAL PASS=62 FAIL=0`.** This is a deterministic comparison of outputs, not a timing measurement, so it does not carry the reproducibility caveat the numbers above do. ### Contract suites — run STRICTLY one at a time Their wall-clock assertions corrupt under contention, so the runner is serial by construction. **Re-run after the NUL fix** (this is the authoritative set; the pre-fix tallies below it are kept for the record). `lib/hook-utils.test.sh` is included because that is where the helper and its new regression case live. ```text lib/hook-utils.test.sh rc=0 PASS=155 FAIL=0 secret-pattern-detection rc=0 PASS=44 FAIL=0 hardcoded-path-check rc=0 PASS=86 FAIL=0 skill-reference-verify rc=0 PASS=96 FAIL=0 stale-path-verify rc=0 PASS=87 FAIL=0 block-noncanonical-commit rc=0 passed: 202 failed: 0 ``` `secret-pattern-detection` and `hardcoded-path-check` each gained exactly **+2** assertions — the two added by the NUL regression case in each file. That is visible directly rather than by subtraction: under mutation (the `split | join` reverted, tests kept) the same trees report `PASS=42 FAIL=2` and `PASS=155 → 154 FAIL=1`, failing on precisely those assertions and nothing else. `skill-reference-verify` reads higher than the pre-fix table below because `main` was merged in between; no case was added to it here. **On the `block-noncanonical-commit` promise.** This description previously said that suite "was still running when this PR was opened" and that "its result will be posted as a comment." No such comment was ever posted, so it is settled here instead: the suite was re-run after the NUL fix and passes, **202/0**. Worth stating because it nearly went into this description as a false negative — that suite reports `passed: N failed: N`, not the `PASS=N FAIL=N` every other guardrails suite uses, so the first run's output filter matched nothing and the run looked like an abort. It was not; the filter was wrong. The tally above is from an unfiltered re-run. **Not re-run, and why.** The remaining guardrails suites (`block-hook-bypass`, `block-convention-violation`, `flag-commit-pr-skill-bypass`, `cli-flag-verify`, `workflow-resilience-check`, plus the two already-converted git guards) and the 15 non-guardrails plugins were not re-run for the NUL fix. The strip is a no-op for any payload without a NUL, and `grep -rln 'hook::jq_fields' plugins/*/hooks/*.sh` returns guardrails files only — the other 15 plugins carry the lib text and a version bump but have no call site. Their pre-fix tallies stand. **Pre-fix tallies** (the original `hook::jq_fields` conversion, before the NUL fix): ```text workflow-resilience-check rc=0 PASS=16 FAIL=0 35s block-convention-violation rc=0 PASS=31 FAIL=0 320s secret-pattern-detection rc=0 PASS=42 FAIL=0 314s flag-commit-pr-skill-bypass rc=0 PASS=29 FAIL=0 303s cli-flag-verify rc=0 PASS=52 FAIL=0 549s skill-reference-verify rc=0 PASS=68 FAIL=0 900s hardcoded-path-check rc=0 PASS=84 FAIL=0 1501s stale-path-verify rc=0 PASS=87 FAIL=0 1600s stale-path-verify rc=0 PASS=87 FAIL=0 1571s block-hook-bypass rc=0 PASS=260 FAIL=0 2278s ``` One caveat from that run, stated rather than hidden: `hardcoded-path-check` and `stale-path-verify` each show **two** lines because a background runner believed killed had survived, so a second copy of each ran concurrently. Both copies of both suites returned the same tally. Contention can only produce spurious *failures* in a wall-clock assertion, never a spurious pass, so a green result under contention is the stronger reading. (The first `hardcoded-path-check` line's tally column is a `grep` artifact — its log ends `PASS=84 FAIL=0`.) <details><summary>Every sample — isolated parse block, 3 fields to 1 (N=100)</summary> ```text payload=payload-ls.json iterations=100 fields=3 sample old_ms new_ms delta_ms 1 704 303 -401 2 1247 262 -985 3 648 223 -425 4 616 265 -351 5 650 252 -398 6 640 758 118 7 653 253 -400 8 627 223 -404 9 633 237 -396 10 1506 423 -1083 11 1664 481 -1183 12 1739 462 -1277 13 2018 1015 -1003 14 1852 796 -1056 15 838 958 120 16 1212 265 -947 17 704 260 -444 18 673 263 -410 19 722 258 -464 20 849 331 -518 21 1429 357 -1072 22 891 869 -22 23 1345 367 -978 24 1318 322 -996 25 663 807 144 26 1192 324 -868 27 1308 887 -421 28 1827 816 -1011 29 3450 782 -2668 30 6323 1474 -4849 31 8060 4150 -3910 32 7464 1999 -5465 33 3327 1541 -1786 34 1791 337 -1454 35 4003 869 -3134 36 11638 1056 -10582 37 4819 2220 -2599 38 5149 885 -4264 39 1270 858 -412 40 1215 799 -416 41 7450 1903 -5547 42 7951 1636 -6315 43 4620 4181 -439 44 4524 1083 -3441 45 2888 1642 -1246 46 2639 824 -1815 47 1870 276 -1594 48 1249 831 -418 49 1793 793 -1000 50 2296 251 -2045 51 2730 757 -1973 52 2241 1294 -947 53 5852 2494 -3358 54 5935 2120 -3815 55 3870 1551 -2319 56 5200 820 -4380 57 1205 265 -940 58 2331 242 -2089 59 4141 1333 -2808 60 1943 305 -1638 61 1910 328 -1582 62 1288 280 -1008 63 1252 266 -986 64 1202 271 -931 65 2896 253 -2643 66 4351 935 -3416 67 6699 894 -5805 68 2085 843 -1242 69 1278 291 -987 70 1218 251 -967 71 1220 258 -962 72 636 238 -398 73 1188 249 -939 74 674 749 75 75 617 223 -394 76 1170 253 -917 77 1751 222 -1529 78 4921 820 -4101 79 5530 796 -4734 80 1895 808 -1087 81 1953 838 -1115 82 1794 251 -1543 83 1193 295 -898 84 1723 809 -914 85 1713 269 -1444 86 1194 255 -939 87 652 769 117 88 1158 808 -350 89 4942 1893 -3049 90 4702 3793 -909 91 1890 1441 -449 92 1167 257 -910 93 1178 269 -909 94 649 269 -380 95 704 249 -455 96 1194 284 -910 97 1188 759 -429 98 1139 250 -889 99 2270 256 -2014 100 7879 1671 -6208 median_paired_delta_ms=-991 p25=-2014 p75=-449 (negative = NEW is faster) iterations_where_NEW_faster=95/100 ``` </details> <details><summary>Every sample — isolated parse block, 2 fields to 1 (N=100)</summary> ```text payload=payload-ls.json iterations=100 fields=2 sample old_ms new_ms delta_ms 1 2995 2056 -939 2 1664 268 -1396 3 1030 300 -730 4 491 285 -206 5 963 266 -697 6 484 793 309 7 427 253 -174 8 494 251 -243 9 432 741 309 10 414 249 -165 11 448 252 -196 12 434 230 -204 13 472 222 -250 14 949 225 -724 15 436 235 -201 16 441 235 -206 17 444 234 -210 18 444 250 -194 19 432 239 -193 20 446 234 -212 21 1021 251 -770 22 447 253 -194 23 485 256 -229 24 459 272 -187 25 1224 788 -436 26 1176 436 -740 27 6119 984 -5135 28 2840 1012 -1828 29 1183 1052 -131 30 552 287 -265 31 531 811 280 32 523 267 -256 33 479 808 329 34 463 266 -197 35 1016 269 -747 36 475 250 -225 37 477 269 -208 38 997 256 -741 39 469 259 -210 40 508 267 -241 41 1178 838 -340 42 1726 877 -849 43 5537 3620 -1917 44 4843 1005 -3838 45 2414 1547 -867 46 1708 271 -1437 47 1003 312 -691 48 450 281 -169 49 499 804 305 50 530 866 336 51 512 273 -239 52 1002 252 -750 53 468 295 -173 54 2921 926 -1995 55 1729 938 -791 56 1736 2165 429 57 1086 834 -252 58 484 957 473 59 1022 281 -741 60 1030 857 -173 61 2762 787 -1975 62 10295 1980 -8315 63 4781 3332 -1449 64 2104 968 -1136 65 1226 902 -324 66 1143 860 -283 67 1061 288 -773 68 1084 834 -250 69 1038 287 -751 70 466 259 -207 71 1215 815 -400 72 2466 4496 2030 73 3997 3347 -650 74 5594 1707 -3887 75 2973 2085 -888 76 1729 1418 -311 77 2138 1415 -723 78 978 252 -726 79 940 741 -199 80 973 261 -712 81 982 253 -729 82 1011 247 -764 83 456 794 338 84 1016 299 -717 85 1010 272 -738 86 1007 1324 317 87 2535 2307 -228 88 1007 266 -741 89 1013 822 -191 90 1011 831 -180 91 919 269 -650 92 956 241 -715 93 946 758 -188 94 1002 1804 802 95 2150 831 -1319 96 1590 1345 -245 97 2952 3790 838 98 6564 5253 -1311 0 [main] bash 433945 dofork: child -1 - forked process 52068 died unexpectedly, retry 0, exit code 0xC0000142, errno 11 parsebench.sh: fork: retry: Resource temporarily unavailable 99 14516 9057 -5459 100 3374 1022 -2352 median_paired_delta_ms=-274 p25=-750 p75=-194 (negative = NEW is faster) iterations_where_NEW_faster=87/100 ``` </details> <details><summary>Payload-level differential vs origin/main — all 62 cases</summary> ```text ok: block-hook-bypass plain-ls (rc=0) ok: block-hook-bypass backslash-newline-continuation (rc=0) ok: block-hook-bypass multiline-m (rc=0) ok: block-hook-bypass escaped-quotes (rc=0) ok: block-hook-bypass embedded-tab (rc=0) ok: block-hook-bypass ps-herestring (rc=0) ok: block-hook-bypass redirect-write (rc=2) ok: block-hook-bypass gh-pr-create (rc=0) ok: block-hook-bypass empty-command (rc=0) ok: block-noncanonical-commit plain-ls (rc=0) ok: block-noncanonical-commit backslash-newline-continuation (rc=0) ok: block-noncanonical-commit multiline-m (rc=2) ok: block-noncanonical-commit escaped-quotes (rc=0) ok: block-noncanonical-commit embedded-tab (rc=0) ok: block-noncanonical-commit ps-herestring (rc=2) ok: block-noncanonical-commit redirect-write (rc=0) ok: block-noncanonical-commit gh-pr-create (rc=0) ok: block-noncanonical-commit empty-command (rc=0) ok: block-convention-violation plain-ls (rc=0) ok: block-convention-violation backslash-newline-continuation (rc=0) ok: block-convention-violation multiline-m (rc=0) ok: block-convention-violation escaped-quotes (rc=0) ok: block-convention-violation embedded-tab (rc=0) ok: block-convention-violation ps-herestring (rc=0) ok: block-convention-violation redirect-write (rc=0) ok: block-convention-violation gh-pr-create (rc=0) ok: block-convention-violation empty-command (rc=0) ok: flag-commit-pr-skill-bypass plain-ls (rc=0) ok: flag-commit-pr-skill-bypass backslash-newline-continuation (rc=0) ok: flag-commit-pr-skill-bypass multiline-m (rc=0) ok: flag-commit-pr-skill-bypass escaped-quotes (rc=0) ok: flag-commit-pr-skill-bypass embedded-tab (rc=0) ok: flag-commit-pr-skill-bypass ps-herestring (rc=0) ok: flag-commit-pr-skill-bypass redirect-write (rc=0) ok: flag-commit-pr-skill-bypass gh-pr-create (rc=0) ok: flag-commit-pr-skill-bypass empty-command (rc=0) ok: hardcoded-path-check write-multiline (rc=0) ok: hardcoded-path-check edit-multiline (rc=0) ok: hardcoded-path-check notebook-multiline (rc=0) ok: hardcoded-path-check unmatched-tool (rc=0) ok: hardcoded-path-check empty-content (rc=0) ok: secret-pattern-detection write-multiline (rc=0) ok: secret-pattern-detection edit-multiline (rc=0) ok: secret-pattern-detection notebook-multiline (rc=0) ok: secret-pattern-detection unmatched-tool (rc=0) ok: secret-pattern-detection empty-content (rc=0) ok: cli-flag-verify write-multiline (rc=0) ok: cli-flag-verify edit-multiline (rc=0) ok: cli-flag-verify unmatched-tool (rc=0) ok: skill-reference-verify write-multiline (rc=0) ok: skill-reference-verify edit-multiline (rc=0) ok: skill-reference-verify unmatched-tool (rc=0) ok: stale-path-verify write-multiline (rc=0) ok: stale-path-verify edit-multiline (rc=0) ok: stale-path-verify unmatched-tool (rc=0) ok: skill-reference-verify replace_all=true (rc=0) ok: stale-path-verify replace_all=true (rc=0) ok: skill-reference-verify replace_all=false (rc=0) ok: stale-path-verify replace_all=false (rc=0) ok: workflow-resilience-check workflow-inline-multiline (rc=0) ok: workflow-resilience-check workflow-scriptpath-only (rc=0) ok: workflow-resilience-check workflow-neither (rc=0) DIFFERENTIAL PASS=62 FAIL=0 ``` </details> ## Review follow-up — the NUL fail-open (P1) Review found a **fail-open this PR introduced**, and it reproduces. `hook::jq_fields` delimits its batched fields with a NUL byte. JSON may legitimately encode a NUL inside a string, and a `Write`/`Edit`/`NotebookEdit` `content` field is exactly where one arrives — jq emitted the raw byte, the read split that value in two, the cardinality check saw one value too many, the helper returned non-zero, and the hook's `|| exit 0` skipped detection **entirely**. The per-field command substitution this PR replaced discarded the NUL and scanned the rest, so this was a regression, not a pre-existing gap. **Reproduction** — one payload, `tool_input.content` = `harmless first line` + NUL + `aws_key = AKIA…`, fed to `secret-pattern-detection.sh` at both refs: | arm | exit | note | | --- | --- | --- | | `origin/main` | **2** (blocked) | stderr also carries bash's own `warning: command substitution: ignored null byte in input` — the old path saw the NUL, dropped it, and scanned the rest | | this branch, before the fix | **0** (allowed) | secret passes unblocked | | this branch, after the fix | **2** (blocked) | | **The framing scheme, and why this one.** Each value is now NUL-stripped **inside the jq filter** (`split("<NUL>") | join("")`, the 1-arity plain-string split — not `gsub`, which would put a NUL inside an Oniguruma pattern), so the delimiter provably cannot occur in a value. The three options weighed: - **Length-prefix framing** is collision-proof but needs `read -N` (Bash 4.1+); this lib supports 3.2+ and says so. - **`@base64` / `@json` encoding** costs a decode per field shell-side — a spawn each, which undoes the whole PR — and still cannot deliver the byte, see below. - **Stripping** is not the lesser option, it is the **only representable** one: a bash variable cannot hold a NUL byte, so *no* scheme delivers one into `HOOK_JQ_FIELDS`. It is also byte-for-byte what the pre-conversion `$( )` did. Content **after** the NUL is returned and scanned exactly as before. **On "rather than failing open".** The mismatch policy is unchanged and deliberately so: `return 1` + the caller's `|| exit 0` is the documented jq-absent fail-open (`hook::require_jq` makes it visible once per session) and matches the pre-conversion empty-field guard. What changed is that the **cause** of the spurious mismatch is gone — a mid-stream jq filter error is now the only way to trip it, exactly as on `main`. **Regression cases** (all three go red on reverting the strip, green with it): - `lib/hook-utils.test.sh` — a NUL-bearing value keeps its slot and its post-NUL content. Mutated: `PASS=154 FAIL=1`. Fixed: `PASS=155 FAIL=0`. - `plugins/guardrails/hooks/secret-pattern-detection.test.sh` — a secret **after** a NUL exits 2. - `plugins/guardrails/hooks/hardcoded-path-check.test.sh` — a machine path **after** a NUL exits 2. Payloads are built with jq's `[0] | implode`, so no literal escape sequence for the byte lives in any test file's source. **Blast radius.** The fix is in the synced shared lib, so `scripts/sync-hook-utils.sh` ran and all 16 carrying plugins take a patch bump with an identical `### Fixed` entry — the mechanism #1979 used for the same file. `guardrails` additionally documents the guard-level regression and the comment softening below. ### Review nits — `replace_all` comment (both files) `skill-reference-verify.sh` and `stale-path-verify.sh` now say the `// false | tostring` is kept for parity with the pre-conversion output, **not** because a branch depends on it: every consumer tests `== "true"`, which `""` and `"false"` fail alike. Comment only; behavior unchanged. ## Checks run locally - `shellcheck -x` clean on every changed `.sh` file (the ten hooks, the shared lib, the three test files). - `shfmt -d` clean on the same set. - `npx --no-install markdownlint-cli2 plugins/guardrails/CHANGELOG.md` — 0 issues. - `bash scripts/check-changelog-parity.sh --check-bump origin/main` — passes (`guardrails` `0.22.0` → `0.22.2` plus a patch bump on all 15 other carrying plugins, each with its own new `## [<version>]` entry). - `bash scripts/sync-hook-utils.sh --check` — all 16 plugin copies match `lib/hook-utils.sh`; `--check-bump origin/main` — every carrying plugin bumped. - No `printf '%s' "$INPUT" | jq` remains anywhere under `plugins/guardrails/hooks/`. ## Related - #2007 — introduced this helper's first two adopters (`block-dangerous-git`, `block-no-verify`) and set the pattern this PR follows; the `// "Bash"` shell-side default is copied from it verbatim. - #1979 — added `hook::jq_fields` to `lib/hook-utils.sh`. The review follow-up above **does** edit that shared lib (the NUL strip), so `scripts/sync-hook-utils.sh` ran, all 16 plugin copies were re-synced, and every carrying plugin took a patch bump — the same mechanism #1979 itself used. An earlier revision of this description claimed no shared-lib edit; that is no longer true and is corrected here. - #1403 — "recover the PreToolUse spawn-reduction work from the closed #1385". This PR discharges **one** part of it: the `hook::jq_fields` conversion across the remaining guards, verified against the multi-line regression class that sank #1385 (precondition 1, differential above). It does **not** discharge: `strip_quoted_spans` in `flag-commit-pr-skill-bypass`, the deferred `git rev-parse --is-inside-work-tree` probe in `hardcoded-path-check`, committed multi-line regression **cases in the suites** (precondition 3 — the differential here is a working harness, not committed test coverage), or the unreviewed `hook_latency_report.py`. Left open. - #1414 — CI has no Windows runner, so a green `plugin-gate` carries no signal for a change whose whole point is MSYS fork-emulation cost. That is why this PR carries local Windows measurements and a payload-level differential rather than leaning on CI. ## Reproducing the numbers The harnesses are scratch scripts, not committed. To re-derive: clone `origin/main` and this branch side by side, then for each iteration time one invocation of each arm back to back (alternating order), and take the median/p75 of `B_i − A_i`. State the machine load with any number produced — on a quiet box the absolute times will be far lower than those above, and the recovery should land nearer the min-of-arms floor (-394 ms for 3 fields, -192 ms for 2) than the loaded medians. --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
1 participant
Summary
The
guardrailsPreToolUse hooks were not slow — they never finished. Each was killed at itsdeclared
timeout, and a killed PreToolUse hook yieldsoutcome:"cancelled"with nopermissionDecision, so the tool call proceeded unguarded.Measured over the transcript window before this change:
block-dangerous-gitenforced on ~0% of callsThis is a security defect first and a latency defect second. Restoring enforcement is the point;
the performance work exists to make enforcement affordable.
Fix
Root-cause only. No detection logic is weakened, no matcher is narrowed, nothing is disabled —
all three were explicitly ruled out.
Verification status — read this before merging
This branch has an incomplete evidence trail, and I am disclosing that rather than implying
otherwise. The agent that authored the change was interrupted before recording:
documentation — the claim was read from the decompiled v2.1.219 binary, not from docs
hook-utils.shchange was delivered throughscripts/sync-hook-utils.shrather than hand-edited, which matters because that file is a cross-plugin cluster
An independent fresh-context review is in progress and its verdict gates this PR. Do not merge
on the strength of the commit subject.
Tradeoff for the reviewer to ratify
If declared timeouts are raised, worst-case waits get longer. Leaving them low keeps waits short
but preserves a silent fail-open. The diagnosis recommends raising them, on the grounds that a
guard which does not run is worse than a guard which is slow. That call belongs to the owner, not
to the change.
Related
Closes #1345