perf(guardrails): parse each hook payload in one jq process, not two or three

[kyle-sexton]

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):

   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.

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):

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.)

Every sample — isolated parse block, 3 fields to 1 (N=100)

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

Every sample — isolated parse block, 2 fields to 1 (N=100)

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

Payload-level differential vs origin/main — all 62 cases

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

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

• fix(guardrails): stop blocking read-only python open() and cat > /dev/null #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.
• perf(hook-utils): cut three subprocess spawns per hook invocation #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
  perf(hook-utils): cut three subprocess spawns per hook invocation #1979 itself used. An earlier revision of this description claimed no shared-lib edit; that
  is no longer true and is corrected here.
• guardrails: recover the PreToolUse spawn-reduction work from the closed #1385 #1403 — "recover the PreToolUse spawn-reduction work from the closed fix(guardrails): restore PreToolUse enforcement — guards were killed at their timeout #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 fix(guardrails): restore PreToolUse enforcement — guards were killed at their timeout #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.
• ci: no Windows runner anywhere — green checks carry no signal for platform-sensitive shell changes #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.

[cursor]

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

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

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b2f08cb590

ℹ️ 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".

[claude]

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

---

Code review — perf(guardrails): parse each hook payload in one jq process, not two or three

• Gather context (PR body, diff, review comments)
• Check for REVIEW.md / lane split
• Review each changed hook script for correctness, error handling, maintainability
• Check test coverage claims vs actual committed tests
• Post inline findings
• Post summary

Scope note: this repo has a dedicated .github/workflows/claude-security-review.yml lane, so per REVIEW.md this pass omits security findings — including the NUL-byte fail-open issue Codex already flagged on secret-pattern-detection.sh:63. That one belongs to the security lane, not here.

What I checked

Diffed all 10 converted hooks against origin/main and hand-verified every HOOK_JQ_FIELDS[n] index against the hook::jq_fields call order in each file (block-convention-violation.sh, block-hook-bypass.sh, block-noncanonical-commit.sh, cli-flag-verify.sh, flag-commit-pr-skill-bypass.sh, hardcoded-path-check.sh, secret-pattern-detection.sh, skill-reference-verify.sh, stale-path-verify.sh, workflow-resilience-check.sh). All ten map correctly — no off-by-one index bugs. The .tool_name // "Bash" default move to the shell side, and the replace_all // false | tostring in-filter placement, are both applied consistently with what the PR body describes. Failure semantics (hook::jq_fields … || exit 0) do land on the same skip the old empty-field guards produced in every file I read.

I could not independently run shellcheck, shfmt, the contract test suites, or scripts/check-changelog-parity.sh in this sandbox (no execution approval for those) — the PR body's tool-output and pass/fail counts are author-claimed and unverified by me. I did independently grep the committed *.test.sh suites (not the PR's own claims about them).

Finding — test coverage gap for the regression class this PR is built to avoid

The PR body cites #1385/#1403: the prior hook::jq_fields-style extraction attempt regressed on multi-line command/content values, and that's exactly the risk class re-verified here via a 62-case differential harness. That harness is real but not committed — the PR body says so explicitly in its #1403 bullet ("the differential here is a working harness, not committed test coverage").

I confirmed the gap directly: the content-bearing hooks' committed suites have no multi-line Write/Edit content case —
hardcoded-path-check.test.sh and
secret-pattern-detection.test.sh have zero multiline/multi-line hits — and
skill-reference-verify.test.sh and
stale-path-verify.test.sh only exercise replace_all: true, never replace_all: false/absent — the exact case the new in-filter // false | tostring placement was written to keep byte-identical.

By contrast, block-noncanonical-commit.test.sh already has extensive multi-line -m coverage (dozens of cases), so the Bash-command hooks are in better shape here than the Write/Edit content hooks.

Since hook::jq_fields is now load-bearing for 12 hooks, and the one time this exact conversion pattern was tried before it silently regressed on multi-line content, I'd suggest folding a handful of the differential harness's cases (multi-line Write/Edit content at minimum; replace_all: false ideally) into the committed suites as a follow-up, so the next person to touch hook::jq_fields or these hooks gets a CI signal instead of relying on someone re-running an uncommitted script. Not a blocker for this PR — it's an accurate description of pre-existing suite composition, not something this PR broke — but it's worth tracking, possibly against the still-open #1403.

Nits (posted inline)

• skill-reference-verify.sh:76 and stale-path-verify.sh:78 — the careful "" vs "false" preservation for replace_all is currently unobservable: both files only ever compare REPLACE_ALL against the literal "true", so the distinction the comment describes doesn't change any branch today. Minor comment-accuracy nit, not a behavior issue.

Everything else

The mechanical conversion itself — field ordering, the TOOL_NAME default relocation, the hardcoded-path-check/secret-pattern-detection content-serialized-before-exclusions trade, and the workflow-resilience-check two-field merge — all check out against the code as written. Changelog entry and the 0.22.0 → 0.22.1 patch bump match a behavior-preserving internal change (Keep a Changelog "Changed" section, no "Added"/"Removed").

[claude]

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

---

Security review — PR #2120

Scope: the 12 changed files (10 guardrail hooks + plugin.json + CHANGELOG.md), diffed against origin/main. lib/hook-utils.sh (where hook::jq_fields is defined) is unchanged by this PR and is referenced only as supporting context for the root cause below.

• Gather context (PR diff, changed hook files)
• Review each changed hook for injection / auth / trust-boundary issues
• Check the NUL-byte hook::jq_fields delimiter collision flagged by Codex (security angle)
• Post findings

---

IMPORTANT — NUL byte in a payload field silently disables the guard it's inside (fail-open), across all 10 converted hooks

Confidence: High (confirms and generalizes the Codex bot comment on secret-pattern-detection.sh:63)

hook::jq_fields (lib/hook-utils.sh#L642-L677) batches N jq filters into one process and separates the N output values with a NUL byte (-j join mode, (., "") per value — L653-L655), then reconstructs the array with read -r -d '' (L660) and requires the reconstructed count to equal the requested field count (L675).

This is not collision-safe: if any field's own value contains a NUL (a JSON string can carry one via ``, and jq -j reproduces it as a raw byte on the pipe), that byte is indistinguishable from the record separator. `read -d ''` splits the value there, the array ends up with more elements than filters, the count check fails, `hook::jq_fields` returns 1, and every call site does `hook::jq_fields "$INPUT" ... || exit 0` — the entire guard exits as if the field were simply absent, not as "malformed, treat cautiously."

That fail-open is a regression specifically for this PR's two content-scanning security guards:

• secret-pattern-detection.sh (L61-L63) — a Write/Edit/NotebookEdit content/new_string/new_source containing `` now skips secret scanning entirely for that call, including the text before and after the NUL. The prior per-field $( ... ) command substitution silently dropped the NUL byte itself (documented bash behavior) but kept scanning the rest of the content — so this is a strict loosening, not a pre-existing gap being carried forward.
• hardcoded-path-check.sh (L68-L70) — same mechanism, same fail-open on a NUL anywhere in the written content.

The same collision also lets a NUL inside tool_input.command fully bypass the command-inspecting guards — not just skip a sub-check, but skip the whole hook, on the token that actually gets executed (a NUL in a JSON string typically truncates the string the OS-level exec layer sees, so the pre-NUL prefix is what runs, and that prefix escapes inspection entirely):

• block-noncanonical-commit.sh:123, block-convention-violation.sh:77, block-hook-bypass.sh:78, flag-commit-pr-skill-bypass.sh:95, cli-flag-verify.sh:81, skill-reference-verify.sh:80, stale-path-verify.sh:82, workflow-resilience-check.sh:52.

Note block-dangerous-git.sh and block-no-verify.sh already carried this exact flaw from #2007 (unchanged here), so the underlying bug in hook::jq_fields isn't new — but this PR is what extends the exposure to the secret/hardcoded-path scanners, which is the highest-impact pair to have fail open.

Suggested direction (matches the Codex comment): don't use a byte that can also occur inside a field's own value as the delimiter. E.g. length-prefix each value, or use jq's own array/NUL-terminated JSON output (jq -c array with a real JSON parse on the bash side is a heavier fix); the minimal fix is likely to detect the delimiter-corruption case explicitly (count mismatch is already visible) and route it to the same skip-if-can't-parse semantics only for genuinely malformed JSON — while still scanning whatever content is recoverable, or at minimum failing closed (block/warn) rather than open for the two scanning guards specifically. That trade-off is a design decision for the author, not prescribed here.

Fix this →

---

No other security-relevant findings

Reviewed the remaining diff surface across all ten hooks: the batched filters passed to hook::jq_fields are static string literals baked into the script (no payload-derived jq-filter injection); the .tool_name // "Bash" default relocation and the replace_all // false | tostring in-filter placement (to dodge hook::jq_fields's // "" wrapper treating boolean false as empty) are both behavior-preserving per the PR's own differential testing; array indices into HOOK_JQ_FIELDS match the requested filter order and count in every hook; and the || exit 0 fail-open semantics for a missing/unparsable payload are unchanged from main (this was already the failure mode for hook::require_jq/empty-field cases pre-PR — the NUL-collision case above is what turns a previously-narrow gap into a much easier-to-trigger one for content-scanning guards). plugin.json/CHANGELOG.md changes are version/metadata only, no security surface.

No GitHub Actions workflow files are touched by this PR, so nothing here falls under zizmor's advisory lane.

[github-actions]

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

[kyle-sexton]

I was sent to address the two REPLACE_ALL comment nits (PRRT_kwDOTCGFQM6XtQ59, PRRT_kwDOTCGFQM6XtQ88). You had already done it — both threads are resolved and outdated, and the wording in both files now reads "kept for parity with the pre-conversion output, not because a branch depends on it". So the main thing I owe you is a report, not a change.

The premise held, verified independently at your current head

I enumerated the consumer set rather than taking the nit's word for it, at 9fb8383d:

skill-reference-verify.sh:430   [[ "$REPLACE_ALL" == "true" ]] || ((${#offs[@]} == 1))
skill-reference-verify.sh:462   [[ "$REPLACE_ALL" == "true" ]] || ((${#offs[@]} == 1)) || continue
stale-path-verify.sh:251        if [[ "$REPLACE_ALL" == "true" ]]; then

Three comparisons, all one-way. HOOK_JQ_FIELDS[3] is assigned exactly once per file and only to REPLACE_ALL; REPLACE_ALL is never exported; nothing outside these two files references either name; and neither test suite asserts the ""/"false" distinction — both only exercise replace_all:true. So "" and "false" really are indistinguishable to every current consumer, and the softening was warranted rather than a reviewer's guess.

Two residuals, offered as one comment-only commit you can ignore

Branch: fix/guardrails-replace-all-comment-2120, cut from 9fb8383d (re-resolved with git ls-remote immediately before pushing and again just now — unchanged). One commit: 0a08156a5093a612683e9d3d4aaa8bdb94a3f794.

1. The re-wrap left # Failure orphaned on a line of its own, in both files, ahead of # semantics are unchanged:.

2. "Unobservable" now reads as "harmless", and it is not. This is the part I think is worth your attention. With the // false in the filter, a well-formed payload can no longer produce "" — so a "" would mean the extraction itself failed, and all three consumers above read that as the permissive branch. The current wording is accurate about there being no live branch, but a reader who takes "no branch depends on it" as "so "" is fine" would be wrong in the one direction that matters for a guard.

The commit adds a clause saying exactly that and pointing at #2126, and it deliberately does not decide what "" should mean — #2126 argues that closing the gap is a real decision (fail closed? validate at the assignment site? document the permissive choice as intended?), and settling it in a comment here would pre-empt that. So this does not close #2126 and should not be read as doing so.

Evidence

Comment-only; no behaviour changes. Same host, both suites, at 9fb8383d and with the commit applied:

                                   base 9fb8383d   with 0a08156a
skill-reference-verify.test.sh     PASS=96 FAIL=0  PASS=96 FAIL=0
stale-path-verify.test.sh          PASS=87 FAIL=0  PASS=87 FAIL=0

shellcheck -x -S warning and scripts/check-shell-portability.sh --paths clean on both files.

I did not push to perf/guardrails-jq-fields — it is under active edit and the choice to take this is yours. I have not resolved or re-opened any thread, and I would not; both were already resolved by your own change, correctly. I also deliberately did not touch plugins/guardrails/hooks/hook-utils.sh, which is under concurrent edit for #2122.

[claude]

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

---

Code review — perf(guardrails): parse each hook payload in one jq process, not two or three (re-review after synchronize, head 6b7aa1b)

• Gather context (new commits since last review, PR body, diff, review thread history)
• Review new commits for correctness / architecture / maintainability
• Check test coverage for the new changes
• Post inline findings
• Post summary

Scope note: this repo has a dedicated .github/workflows/claude-security-review.yml lane, so per REVIEW.md this pass omits security findings. The NUL-delimiter fail-open/bypass mechanics (already covered exhaustively in this thread — Codex, the earlier security-lane pass, and the author's own extended investigation into command-guard truncation behavior) belong there, not here. This pass is scoped to correctness, architecture, error handling, test coverage, and maintainability of what changed since the last full review (commit 9fb8383, the NUL-strip fix, plus the two merge commits that followed it).

🔴 Finding — the origin/main merge (6b7aa1b) silently reverts session-flow to a stale version, deleting a shipped fix

Verified directly with git show at both refs, not from any claimed tool output:

$ git show origin/main:plugins/session-flow/.claude-plugin/plugin.json | grep version
  "version": "0.22.1",
$ git show HEAD:plugins/session-flow/.claude-plugin/plugin.json | grep version
  "version": "0.22.0",

origin/main (via #2116) carries session-flow at 0.22.1 with a changelog entry for a real bug fix ("a background continuation that COMPLETED is no longer read as one that failed"). This branch's merge of origin/main (6b7aa1b) lands with that plugin back at 0.22.0 and the ## [0.22.1] entry gone from plugins/session-flow/CHANGELOG.md — a net +1/-27 across the two files, even though session-flow has no hook::jq_fields call site and is otherwise untouched by this PR's actual work. Left as-is, merging this PR moves main's session-flow backward and silently drops that fix's changelog record.

scripts/check-changelog-parity.sh has a check built for exactly this shape (VERSION REGRESSION: … merging would move the version BACKWARD, scripts/check-changelog-parity.sh#L311) — the PR description's "passes" claim for that script was run at 9fb8383, before the origin/main merge that introduced this regression landed in 6b7aa1b, so it hasn't been re-run since. Inline comments posted on both affected files with a fix link.

Everything else new since the last review

The 9fb8383 NUL-strip fix itself (hook::jq_fields now removes NUL from each value jq-side via split("") | join("") before framing) is correctly scoped: mechanism, regression tests (lib/hook-utils.test.sh, secret-pattern-detection.test.sh, hardcoded-path-check.test.sh), and the mutation-tested proof in the PR description all check out against the code as written. The replace_all comment softening in skill-reference-verify.sh / stale-path-verify.sh matches the earlier review nit correctly — content-wise. Two small things:

• Nit (posted inline): the comment re-wrap left # Failure orphaned on its own line in both files (skill-reference-verify.sh:79, stale-path-verify.sh:81) — cosmetic only.
• Test-coverage observation, not a new finding: the added regression tests cover the two content-scanning hooks (secret-pattern-detection, hardcoded-path-check) only. The command-guard question raised later in this same review thread — whether a NUL-then-trailing-text command payload is handled consistently across the command-parsing hooks — has no committed test either way. That's already tracked as an open design question with a companion branch (fix/hook-utils-nul-fail-open / guardrails: a NUL byte in a payload value makes block-no-verify and block-dangerous-git fail open #2122) per the thread, so I'm not re-deriving it here; flagging only that it remains untested in this branch as of 6b7aa1b.

No other files in the diff (the 10 converted hooks, the 15 other synced hook-utils.sh copies, plugin.json/CHANGELOG.md version bumps for the guardrails-related plugins) show issues beyond what the earlier review round already covered and this PR has since addressed.

View job

[claude]

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

---

Security review — PR #2120

• Gather context (PR diff at current HEAD 6b7aa1b, prior review threads)
• Review lib/hook-utils.sh NUL-handling fix (hook::jq_fields) for the disposition already debated in-thread
• Review each converted guardrails hook for injection / trust-boundary issues
• Check jq-filter construction across all 12 hook::jq_fields call sites for injection
• Post findings

Scope: the diff between origin/main and 6b7aa1b (12 hook/lib files + manifests/changelogs across 16 plugins). No .github/workflows/* files are touched by this PR, so nothing here falls under zizmor's lane.

---

IMPORTANT — the NUL strip in hook::jq_fields is correct for content scanners but can synthesize a non-matching token in command-blocklist guards

Confidence: Medium — the code-level mechanism is confirmed by reading; real-world exploitability depends on how the harness delivers tool_input.command bytes to the actual shell, which this repo cannot see and which the PR's own review thread twice measured, then retracted (nobody has traced the actual path end-to-end).

lib/hook-utils.sh#L663 strips every NUL from every field's value before it reaches any caller: split("") | join(""). That's the right fix for the content-scanning guards this PR converts (secret-pattern-detection.sh, hardcoded-path-check.sh) — the full pre- and post-NUL text is still scanned, matching the PR's own mutation-tested regression cases.

But the same strip is now applied uniformly to the command-blocklist guards, which match an exact token against a fixed string — e.g. block-no-verify.sh#L199: [[ "$x" == "--no-verify" ]]. A tool_input.command value containing --no-verify + NUL + any trailing byte(s) strips to --no-verifyx, which fails that comparison — the guard evaluates a token that was never the one an attacker intends to run and lets the segment through unblocked. The same shape applies to every other exact-match blocklist token this PR's hooks and the two pre-existing hook::jq_fields callers check (core.hookspath=, --force, hook-manager env prefixes, etc. in block-dangerous-git.sh, block-hook-bypass.sh, block-noncanonical-commit.sh, block-convention-violation.sh, flag-commit-pr-skill-bypass.sh).

Why this isn't a clean regression, and why it's still worth flagging:

• For the two hooks already on hook::jq_fields before this PR (block-no-verify.sh, block-dangerous-git.sh, both unchanged in this diff but behaviorally affected because they source the patched hook-utils.sh), origin/main's current behavior is strictly worse: any NUL anywhere in the command already trips the field-count mismatch and the caller's || exit 0 skips the guard entirely, unconditionally allowing the real command through untouched. This PR narrows that to requiring the NUL to land precisely inside the matched token — a strict improvement for those two callers, not a new hole.
• For the ten hooks newly converted by this PR, the strip is claimed (and differentially/mutation-tested) byte-identical to the old per-field $( ) substitution's NUL handling, so no regression there either versus each hook's own prior behavior.
• What is new is scope: this PR grows the set of hook::jq_fields callers from 2 to 12, six of which own a block (exit 2) verdict, and none of the 12 consult any signal that a value contained a NUL — confirmed by grep, there is no HOOK_JQ_FIELDS_NUL-style flag anywhere in the current lib/hook-utils.sh. A hook has no way to choose "refuse rather than evaluate a possibly-corrupted token" today, for content or command fields alike.

This exact issue, including the strip-vs-truncate trade-off and the flag-prerequisite point, has already been identified, reproduced, and discussed at length in this PR's own comment thread (including two retracted claims about which mechanism the shell actually uses to consume a NUL-bearing command). The author has linked a companion fix (#2122) that adds a flag and states the flag-check is a prerequisite for every converted hook, sequencing left as an author call. I'm not aware of anything in that existing analysis that's wrong — flagging here mainly to record, from an independent pass at the current HEAD, that: (a) the residual is real and still unaddressed at 6b7aa1b, (b) it is a narrowing not a widening of the pre-existing main fail-open, and (c) the blast radius is 12 callers / 6 verdict-owners, not just the one hook the inline thread centers on.

Fix this →

---

No other security-relevant findings

• No jq-filter injection: independently grepped every hook::jq_fields call site across all 12 callers — every filter argument is a static string literal baked into the script; none are built from tool_input/payload data, so there's no path for a crafted payload to inject additional jq filter syntax into the batched program.
• Field-index mapping: spot-checked secret-pattern-detection.sh (.tool_name→0, .file_path→1, .content→2, .new_string→3, .new_source→4, consumed at L125-127) and skill-reference-verify.sh's replace_all handling — both correct against the documented filter order, consistent with the exhaustive index check already posted by the earlier review pass on this PR.
• Scope/path guards in secret-pattern-detection.sh / hardcoded-path-check.sh (CLAUDE_PROJECT_DIR prefix check) are unchanged logic, just reordered relative to the new batched jq call per the PR's stated trade-off (content is now extracted before the git check-ignore skip) — that's a coverage trade (scans slightly more, never less), not a security weakening.
• No CI/workflow files are touched by this PR — nothing here falls under the Actions-specific advisory lane (pull_request_target, permissions, unpinned actions, etc.).
• plugin.json / CHANGELOG.md changes across all 16 carrying plugins are version/metadata only.
  · branch perf/guardrails-jq-fields

[claude]

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

---

Security review — PR #2120 (HEAD 62e2270)

• Gather context (diff since the last security-lane pass at 6b7aa1b, full thread history)
• Check disposition of the NUL-byte fail-open / strip-vs-truncate debate at current HEAD
• Check disposition of the session-flow version-regression finding
• Re-check all hook::jq_fields call sites for the flag-check prerequisite
• Post findings

Scope confirmed: git diff 6b7aa1b..62e2270 -- lib/hook-utils.sh plugins/guardrails/hooks/ is empty — the only commit since the last full security-lane pass (6b7aa1b, posted 04:56) is 62e2270, which touches only plugin.json/CHANGELOG.md metadata for 29 unrelated plugins (the session-flow version-regression fix flagged in that same pass, now resolved — confirmed no session-flow files remain reverted). No security-relevant code changed. This pass independently re-verifies the prior finding against current HEAD rather than re-deriving it from scratch, since the extensive in-thread investigation (Codex, two prior security-lane passes, and the author's own measurement-and-retraction sequence) already did that work.

---

IMPORTANT — hook::jq_fields's NUL strip is unconditional and unsignaled, so a NUL inside a blocklisted command token can still evade the six command-matching guards

Confidence: Medium (unchanged from the prior pass — the mechanism is confirmed by reading; real-world exploitability depends on an execution path nobody in this thread established, including two retracted claims about it)

Verified directly against current HEAD: lib/hook-utils.sh#L652-L667 strips every NUL byte from every field's value inside the jq filter (split("") | join("")) before values reach any caller, and exposes no flag or signal that a strip happened — confirmed by grepping the whole tree for any NUL-flag mechanism (JQ_FIELDS_NUL, jq_fields_nul, HOOK_NUL, etc.): none exists anywhere in this repo at 62e2270.

This disposition is correct and tested for the two content-scanning callers (secret-pattern-detection.sh, hardcoded-path-check.sh) — the full pre- and post-NUL text is still scanned, matching the PR's own mutation-tested regression cases. But the same strip applies uniformly to the six command-blocklist guards that also call the helper — block-no-verify.sh:82,199, block-dangerous-git.sh, block-hook-bypass.sh, block-noncanonical-commit.sh, block-convention-violation.sh, flag-commit-pr-skill-bypass.sh — each of which does an exact-string match against a blocklisted token (e.g. [[ "$x" == "--no-verify" ]]). A tool_input.command value with a NUL byte landing inside the blocklisted token (--no-verify<NUL>x) strips to --no-verifyx, which fails every such comparison and lets the segment through, unblocked, by a guard that now reasons about a string that isn't the one it received.

I independently re-counted the caller census rather than trusting the thread's own numbers: grep -c 'hook::jq_fields "\$INPUT"' across plugins/guardrails/hooks/*.sh returns 12 callers (matches the thread), and grep -c 'exit 2' per file confirms 8 of the 12 own a block verdict (block-convention-violation, block-dangerous-git, block-hook-bypass, block-noncanonical-commit, block-no-verify, cli-flag-verify, hardcoded-path-check, secret-pattern-detection) — matching the "eight own a block verdict" figure the thread converged on. None of the 12 consults any NUL signal, because none exists to consult.

Why this is genuinely unresolved rather than a clear regression: the thread's own investigation (three rounds of measurement and two retractions, 04:19–04:29) established that nobody has traced the actual path a tool_input.command string travels from the hook payload to whatever ultimately executes it — Node's child_process refuses a NUL-bearing argument outright on the measured paths, which would mean the command never reaches a parsing shell at all, in which case this specific bypass shape may not be reachable in practice. That is exactly why "fail-closed on the mere presence of a NUL" — regardless of disposition — was the position the thread converged on, and it's the one piece still missing from HEAD: the companion change (fix/hook-utils-nul-fail-open, ccp#2122) that would add a flag and have callers check it has not landed in this branch or been merged into it.

Net assessment: not a new issue introduced since the last security-lane pass (code is byte-identical to what 6b7aa1b already reviewed), and not something this specific PR needs to solve — the flag-check prerequisite is explicitly sequenced as a separate, joint decision with #2122 per the thread. Recording it here because it is still live at the commit this review is scoped to, and because a security review that omits it on the grounds that "it was already mentioned" would be incomplete for anyone relying on this pass in isolation.

Fix this →

---

No new security-relevant findings since the last pass

• session-flow version regression is resolved, not a security issue in itself — confirmed plugins/session-flow/.claude-plugin/plugin.json and CHANGELOG.md no longer show the downgrade the prior pass flagged at 6b7aa1b.
• No jq-filter injection: re-confirmed all hook::jq_fields filter arguments across all 12 call sites remain static string literals, none built from payload data.
• Content-scanning NUL fix (secret-pattern-detection.sh, hardcoded-path-check.sh) is unchanged since the 9fb8383 fix already reviewed and is still correct: full content before and after a NUL is scanned, not truncated or dropped.
• No .github/workflows/* files are touched by this PR — nothing here falls under zizmor's advisory lane.
• No other files in the diff (10 converted hooks, 15 synced hook-utils.sh copies, plugin.json/CHANGELOG.md bumps across 16 plugins) show security-relevant changes beyond what prior passes already covered.
  · branch perf/guardrails-jq-fields

[claude]

Claude finished @kyle-sexton's task in 8m 33s —— View job

---

Code review — perf(guardrails): parse each hook payload in one jq process, not two or three (re-review after synchronize, head 9cd1837)

• Gather context (new commits since last full code-review pass at 6b7aa1b/62e2270)
• Review new commits for correctness / architecture / maintainability
• Check test coverage for anything new
• Post inline findings
• Post summary

Scope note: security findings (the NUL-delimiter fail-open/bypass discussion already covered exhaustively in this thread) are left to the dedicated security lane per REVIEW.md.

What's new since the last full code-review pass (6b7aa1b)

• 62e2270 — the changelog-parity fix responding to my prior session-flow version-regression finding.
• 0b097eb — not this PR's own work. It's a separate, already-merged origin/main commit (fix(guardrails): pin the reconstruction scan's locale so it stops decoding a literal search #2127, a skill-reference-verify locale fix) pulled in by the 9cd1837 merge; git diff origin/main...HEAD confirms it contributes nothing to this PR's actual diff.
• d20e072 — the comment reflow fixing the orphaned # Failure line in skill-reference-verify.sh / stale-path-verify.sh. Verified directly against the diff: matches exactly what was posted inline earlier in this thread, comment-only, no behavior change.

I independently re-verified the session-flow fix rather than trusting the claim: plugins/session-flow/.claude-plugin/plugin.json is 0.22.2 (above origin/main's 0.22.1) and CHANGELOG.md carries both the [0.22.1] entry (the real #2116 fix) and [0.22.2] on top — resolved correctly. I then checked every version-bump pair this PR touches (git diff origin/main...HEAD -- '**/plugin.json', all 44 files) and confirmed every one is old-version-then-strictly-higher, so the specific regression I flagged before is gone everywhere.

🔴 New finding — the changelog-restore fix introduced a different bug: spurious version bumps + duplicated entries on plugins this PR has no business touching

While re-checking the restore, I read the actual changelog content rather than just the version ordering, and found that for plugins not among the 16 real hook::jq_fields/hook-utils.sh carriers (actionlint, autonomy, bash-format, biome-format, claude-ops, context-guard, desktop-notification, eol-normalizer, go-format, guardrails, markdown-format, powershell-format, rate-limit-guard, ruff-format, source-control, typos-format), the "restored" top entry is a byte-for-byte duplicate of the entry already sitting one version below it on origin/main — i.e. these plugins got a version bump and a changelog entry for no actual change at all.

Verified directly (diffed each plugin's full directory against origin/main, not just the changelog) — confirmed as the plugin's entire PR diff, in all four cases:

• architecture/CHANGELOG.md:6 — [0.5.1] duplicates [0.5.0]
• adhd/CHANGELOG.md:6 — [0.4.1] duplicates [0.4.0]
• discovery/CHANGELOG.md:3 — [0.13.2] duplicates [0.13.1] (itself from unrelated fix(discovery): one exhaustive surface is not an artifact inventory #2116, merged via origin/main)
• firecrawl/CHANGELOG.md:6 — [0.5.1] duplicates [0.5.0]

Inline comments posted on all four. I also found a partial version of the same thing in a real carrier — markdown-format/CHANGELOG.md:21 — where the new [0.11.2] entry correctly adds the genuine NUL-fix bullet, but then also re-pastes the entire [0.11.1] "host without git" paragraph that's already recorded one section down (inline comment posted).

Why this passed the author's stated check-order/check-bump runs: I read scripts/check-changelog-parity.sh directly — --check-order (L114-L155) only flags a repeated version number, never repeated content under two different numbers, and --check-bump only checks that the version moved forward. Neither check can see this class of bug, so a clean run of both (which I have no reason to doubt) is fully consistent with this still being present.

Root cause, as best I can tell: the 62e2270 rebuild logic pulled origin/main's current top entry forward as "this branch's own entry" for every plugin whose local changelog differed from origin/main, even for plugins where the only reason it differed was the earlier bad merge resolution stripping it back to a stale state — not because this branch (a guardrails-focused perf PR) ever had a legitimate change of its own for that plugin.

Scope, stated honestly: I spot-checked 4 non-carrier plugins and found the pattern in all 4, plus one partial instance in a real carrier. I did not check the remaining ~24 non-carrier plugins this PR's diff touches (ai-briefing, bug-report, claude-config, claude-memory, code-tidying, codebase-health, context7, debugging, discipline, disk-hygiene, docs-hygiene, domain-driven-design, dometrain, education, evals, event-storming, github, implementation, kindle-dedrm, knowledge, machine-health, mcp-tools, miro, naming, session-flow already separately confirmed clean) one by one — given a 4-for-4 hit rate on files whose only PR diff is plugin.json + CHANGELOG.md, I'd treat the same as likely for the rest of that set rather than assume they're fine, and recommend an explicit audit rather than my sampling standing in for one.

Suggested fix: for any plugin whose only diff in this PR is a plugin.json bump + CHANGELOG.md entry with no corresponding hooks/hook-utils.sh change (or other real functional diff), restore both files to origin/main's content exactly (e.g. git checkout origin/main -- <path>/plugin.json <path>/CHANGELOG.md) rather than carrying a phantom release.

Everything else

lib/hook-utils.test.sh's new NUL regression case (jf_nul_input, built via jq's [0] | implode so no literal NUL escape lives in the test source) correctly asserts the join behavior (beforeafter) rather than truncation, matching the strip disposition implemented in hook::jq_fields and the extended design discussion in this thread. No issues there.
· branch perf/guardrails-jq-fields

[claude]

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

---

Security review — PR #2120 (HEAD 9cd1837)

• Gather context (diff since last security-lane pass at 62e2270)
• Review new commits (0b097eb, d20e072, 9cd1837) for security-relevant changes
• Check disposition of previously-flagged NUL-byte fail-open issue
• Post findings

Scope check. git diff origin/main...HEAD at this head totals 1272 insertions / 231 deletions, matching the PR's own stated diff size exactly — confirms nothing besides this PR's own commits is in scope. Compared against the diff already reviewed at the last security-lane pass (6b7aa1b/62e2270), the security-relevant surface (lib/hook-utils.sh and the ten converted plugins/guardrails/hooks/*.sh) is byte-identical — git diff 62e2270..9cd1837 -- lib/hook-utils.sh is empty.

Two commits landed since that pass:

• d20e072 — pure comment re-wrap (fixes the orphaned # Failure line flagged as a nit in the code-review lane). No code change, no security surface.
• 0b097eb (fix(guardrails): pin the reconstruction scan's locale, fix(guardrails): pin the reconstruction scan's locale so it stops decoding a literal search #2127) — this arrived via the origin/main merge in 9cd1837 and is already on origin/main (confirmed: git rev-parse origin/main == 0b097eb), so it is common ancestry, not part of this PR's diff, and out of this review's scope per the task instructions (review only files changed in this PR, base origin/main). Noting for completeness since it does touch skill-reference-verify.sh: it adds local +x LC_ALL=C inside reconstruct_partial_edit(). The +x (unexported) is deliberate and documented in the diff itself — child processes (grep -oE, etc.) keep running in the caller's original locale, only this function's own bash-native %%/[[:space:]] matching is pinned. No injection surface: the value is a fixed literal (C), not derived from payload or environment data.

No new security-relevant findings

The previously-reported and still-open item — hook::jq_fields's unconditional, unsignaled NUL-strip letting a NUL land inside a blocklisted token and evade the six exact-match command guards (block-no-verify.sh, block-dangerous-git.sh, block-hook-bypass.sh, block-noncanonical-commit.sh, block-convention-violation.sh, flag-commit-pr-skill-bypass.sh) — is unchanged at this head, since its code (lib/hook-utils.sh) hasn't moved since the last pass. It's already been reported in full (three prior security-lane passes on this thread) and is tracked in the companion PR fix/hook-utils-nul-fail-open (ccp#2122), which the author has stated needs to land and reconcile before/alongside this PR. Not re-reporting it as a fresh finding here — nothing changed about it since 6b7aa1b.

No jq-filter injection, no new command-guard bypass paths, no CI/workflow files touched, and the session-flow version-regression from the earlier pass remains fixed (confirmed no session-flow files in this diff).
· branch perf/guardrails-jq-fields

[claude]

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

---

Security review — PR #2120 (HEAD 6051543)

• Gather context (diff since last security-lane pass at 9cd1837)
• Check what changed in the "drop spurious bumps" commit (6051543)
• Re-verify disposition of previously-flagged NUL-byte fail-open issue
• Post findings

Scope check. git diff origin/main...HEAD --stat totals 860 insertions / 202 deletions across 62 files, matching the PR's own stated diff size exactly. git diff 9cd1837..6051543 -- lib/hook-utils.sh plugins/guardrails/hooks/ is empty — the only commit since the last full security-lane pass (9cd1837) is 6051543, and it touches only plugin.json version numbers and CHANGELOG.md entries across 29 non-carrier plugins (adhd, ai-briefing, architecture, bug-report, claude-config, claude-memory, code-tidying, codebase-health, context7, debugging, discipline, discovery, disk-hygiene, docs-hygiene, domain-driven-design, dometrain, education, evals, event-storming, firecrawl, github, implementation, kindle-dedrm, knowledge, machine-health, markdown-format, mcp-tools, miro, naming, session-flow) — it drops the spurious version bump + duplicated changelog entry the code-review lane flagged (e.g. architecture [0.5.1] duplicating [0.5.0]), restoring them to origin/main's content. No hooks/ or lib/ files are touched. Pure metadata/version cleanup, no security surface.

lib/hook-utils.sh (and its 16 synced plugin copies) is byte-identical to what was reviewed at 9cd1837, 62e2270, and 6b7aa1b in this thread's prior security-lane passes.

No new security-relevant findings

Nothing security-relevant changed since the last pass. The previously-reported and still-open item stands, unchanged, and is not being re-derived here since its analysis (mechanism, reproduction, and the extended strip-vs-truncate design discussion between the author and this lane) is already fully recorded on this thread:

hook::jq_fields's NUL strip is unconditional and unsignaled (lib/hook-utils.sh#L652-L667) — correct and tested for the two content-scanning callers (secret-pattern-detection.sh, hardcoded-path-check.sh), but a NUL landing inside a blocklisted token in tool_input.command can still evade the six exact-match command guards (block-no-verify.sh, block-dangerous-git.sh, block-hook-bypass.sh, block-noncanonical-commit.sh, block-convention-violation.sh, flag-commit-pr-skill-bypass.sh), because no caller consults any NUL-occurred signal (none exists to consult). Severity IMPORTANT, confidence Medium — real exploitability depends on an execution path this thread's own investigation could not establish (Node's child_process appears to refuse NUL-bearing arguments outright on the measured paths, which would make the bypass shape unreachable in practice; this was measured and partially retracted twice in-thread). Tracked as a joint prerequisite with the companion change fix/hook-utils-nul-fail-open (#2122), which needs to land and be reconciled with this branch — not something this PR alone needs to resolve.

No jq-filter injection (all hook::jq_fields filter arguments remain static string literals across all 12 call sites), no CI/workflow files touched by this PR (nothing here falls under zizmor's advisory lane), and the session-flow version-regression and the spurious duplicate-changelog-entry issues from earlier passes are both resolved as of this commit.
· branch perf/guardrails-jq-fields

[claude]

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

---

Code review — perf(guardrails): parse each hook payload in one jq process, not two or three (re-review after synchronize, head 6051543)

• Gather context (new commits since last full review at 9cd1837)
• Verify the changelog-bump fix (6051543) resolves the spurious-duplicate findings from the last pass
• Review any other new changes
• Post findings
• Post summary

Scope note: per REVIEW.md, this pass omits security findings — the NUL-delimiter fail-open/bypass discussion is exhaustively covered elsewhere in this thread and belongs to the dedicated security lane.

What's new since the last full code-review pass (6b7aa1b/9cd1837)

Only one commit changed anything: 6051543 (fix(changelog): drop the spurious bumps my restore introduced), which is the author's response to the prior pass's finding that the 62e2270 changelog-restore had given a phantom version bump + duplicated entry to architecture, adhd, discovery, firecrawl, and a partial duplicate to markdown-format.

I independently re-verified the fix rather than trusting the commit message:

• All 5 previously-flagged plugins now match origin/main exactly. git diff origin/main -- plugins/architecture/ plugins/adhd/ plugins/discovery/ plugins/firecrawl/ produces no output — the phantom bumps and duplicate entries are gone.
• markdown-format's partial duplicate is resolved too — its [0.11.2] section now contains only the new NUL-fix bullet; the [0.11.1] "host without git" content one section down is untouched, not restated.
• The overall diff is back to exactly the PR's claimed scope: git diff origin/main...HEAD --stat now shows only lib/hook-utils.sh/lib/hook-utils.test.sh, the 10 converted guardrails hooks + 2 test files, and plugin.json+CHANGELOG.md+hooks/hook-utils.sh for exactly the 16 real hook::jq_fields-carrying plugins (860/-202, matching the PR's own stated additions/deletions). session-flow — flagged as wrongly touched two passes ago — no longer appears in the diff at all, confirmed it carries no hooks/hook-utils.sh copy and has no business being here.
• Every one of the 16 carriers' version bumps is a clean single patch increment (git diff origin/main...HEAD -- '**/plugin.json'): e.g. actionlint 0.8.0→0.8.1, guardrails 0.23.0→0.23.1, source-control 0.51.1→0.51.2 — no regressions, no skips.

🔴 New finding — one instance of the same duplicate-entry bug survived the cleanup, in source-control

Posted inline on plugins/source-control/CHANGELOG.md:21-30. Summary: comparing line counts across the 14 "plain" carriers (git diff origin/main...HEAD --numstat -- '**/CHANGELOG.md'), 13 of them add exactly +16 lines (one clean NUL-fix bullet); source-control adds +26. The extra 10 lines are a byte-identical duplicate of the "PR-body linkage gate … deadlocked on a maximum-length PR body" bullet already sitting under [0.51.1] ten lines below it — an entry already on origin/main, describing a fix (pr-body-linkage-gate.sh's < <(printf '%s\n' "$body")) that this PR's diff doesn't touch at all (git diff origin/main...HEAD --stat -- plugins/source-control/ shows only plugin.json, CHANGELOG.md, hooks/hook-utils.sh). source-control is a legitimate carrier so the version bump and the NUL-fix bullet itself are correct — only the second, duplicated bullet needs to come out.

Root cause is the same one the prior pass already diagnosed for the other 5 plugins: scripts/check-changelog-parity.sh --check-order only flags a repeated version number, never repeated content under two different numbers, so this class of bug isn't caught by the automated gate the author's checklist relies on. guardrails's own [0.23.1] section (+49 lines) was checked too and is clean — all its content is genuinely new (the NUL-fix, the replace_all comment softening, and the headline jq-batching change), not a restatement of anything already on origin/main.

Everything else

No hook, lib, or test file changed since the last full review pass — the mechanical conversion, the NUL-strip fix, and the replace_all comment wording were all reviewed in prior passes on this thread and are unchanged at this head. I did not re-run shellcheck/shfmt/the contract suites since no .sh file is in this commit's diff.
· branch perf/guardrails-jq-fields

[claude]

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

---

Security review — PR #2120 (HEAD 84a51af)

• Gather context (diff since last security-lane pass at 6051543)
• Check what changed in 84a51af
• Re-verify disposition of previously-flagged NUL-byte fail-open issue
• Post findings

Scope check. git diff origin/main...HEAD --stat totals 850 insertions / 202 deletions across 62 files, matching the PR's own stated diff size exactly. git diff 6051543..84a51af -- lib/hook-utils.sh plugins/guardrails/hooks/ is empty — the only commit since the last security-lane pass (6051543) is 84a51af, and it touches exactly one file:

--- a/plugins/source-control/CHANGELOG.md
+++ b/plugins/source-control/CHANGELOG.md
@@ -18,16 +18,6 @@ ### Fixed
-- **The PR-body linkage gate and validator deadlocked on a maximum-length PR body.** ...
-  (10-line bullet, byte-identical to the one still present under [0.51.1] below)

This removes the duplicated [0.51.2] bullet the code-review lane flagged (a restatement of a bullet already on origin/main under [0.51.1], describing a pr-body-linkage-gate.sh fix this PR doesn't otherwise touch). Pure changelog text, no code, no security surface.

No new security-relevant findings

Nothing in the security-relevant surface (lib/hook-utils.sh and the ten converted plugins/guardrails/hooks/*.sh) changed since the last full security-lane pass at 6051543, so its conclusions still stand at this head without re-derivation:

hook::jq_fields's NUL strip is unconditional and unsignaled (lib/hook-utils.sh#L652-L667) — correct and tested for the two content-scanning callers (secret-pattern-detection.sh, hardcoded-path-check.sh), but a NUL landing inside a blocklisted token in tool_input.command can still evade the six exact-match command guards (block-no-verify.sh, block-dangerous-git.sh, block-hook-bypass.sh, block-noncanonical-commit.sh, block-convention-violation.sh, flag-commit-pr-skill-bypass.sh), because no caller consults any NUL-occurred signal (none exists to consult). Severity IMPORTANT, confidence Medium — extensively discussed in this thread (Codex, three prior security-lane passes, and the author's own measurement-and-retraction sequence), and tracked as a joint prerequisite with the companion change fix/hook-utils-nul-fail-open (#2122), which needs to land and be reconciled with this branch. Not re-derived here since nothing about it changed.

No jq-filter injection (all hook::jq_fields filter arguments remain static string literals across all 12 call sites, unchanged since the last pass), no CI/workflow files touched by this PR (nothing here falls under zizmor's advisory lane), and the previously-flagged session-flow version-regression and spurious-duplicate-changelog issues remain resolved as of this commit.
· branch perf/guardrails-jq-fields

[claude]

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

---

Code review — re-review after synchronize (head 84a51af)

• Gather context (new commit since last full review at 6051543)
• Verify the source-control duplicate-bullet fix
• Check for any other regressions introduced by 84a51af
• Post inline findings (none needed)
• Post summary

Scope note: per REVIEW.md, this pass omits security findings — the NUL-delimiter fail-open/bypass discussion is exhaustively covered elsewhere in this thread and belongs to the dedicated security lane.

What changed since the last full code-review pass (6051543)

Exactly one commit, 84a51af (fix(changelog): drop the last restated bullet from source-control), touching exactly one file: plugins/source-control/CHANGELOG.md (-10 lines, no additions). This addresses the finding from the prior review pass that source-control's new [0.51.2] section restated the "PR-body linkage gate … deadlocked" bullet verbatim from [0.51.1] below it.

Verified directly, not just by re-reading the claim:

• git show 84a51af — the deleted hunk is exactly the duplicated bullet (the one starting "The PR-body linkage gate and validator deadlocked…"), nothing else touched.
• Read the resulting file (plugins/source-control/CHANGELOG.md#L1-L22) — [0.51.2] now contains only the NUL-fix bullet, [0.51.1] still has its original PR-body-linkage entry intact below it (untouched, matches origin/main), and the section boundary is clean (single blank line, no orphaned Markdown).
• git diff origin/main...HEAD --numstat -- '**/CHANGELOG.md' — all 15 non-guardrails carriers (including source-control) now show a uniform +16/-0, matching the "one clean bullet" pattern the other 14 plain carriers already had; source-control was the one outlier at +26 before this commit and is no longer. guardrails remains at +49 (its own real, larger changelog entry), as expected.
• Re-checked markdown-format (the one file the previous pass flagged as a partial duplicate rather than a full spurious plugin) — still correctly trimmed, [0.11.2] carries only its own NUL-fix bullet, [0.11.1] retains its original content untouched.
• Spot-checked the other 14 plain-carrier changelogs' diffs for the same "verbatim bullet repeated one section down" shape this commit's message claims to have swept for — found none. No new instance of the pattern surfaced anywhere in the 16-carrier set at this head.

No hook script, plugin.json, or lib/hook-utils.sh content changed in this commit — pure changelog cleanup, so nothing here revisits the mechanical-conversion or NUL-strip correctness already reviewed in prior passes on this thread.

No findings

This commit does exactly what it claims, is correctly scoped, and resolves the last open item from the previous code-review pass. Nothing else changed since then.
· branch perf/guardrails-jq-fields