fix(hook-utils): stop a NUL in a payload value from voiding two blocking guards

[kyle-sexton]

Closes #2122

Update — main moved under this PR, and the disposition changed with it

#2120 merged (fd075c27), and it fixed the same function with the opposite value disposition:
it STRIPS every NUL out of a value where this branch TRUNCATED at the first one. The PR went
DIRTY. Resolved by merging origin/main into the branch — never a rebase, since force-push is
blocked here twice over.

The resolution keeps main's strip and this branch's flag plus fail-closed guards. That is
additive over main rather than a choice between the two sides, and it is what this body already
argued for in its own words: the disposition is immaterial for this PR's own two callers, which
refuse on the flag before reading a value, while main now carries the ten scanner-class callers
#2120 converted, none of which consults the flag. Truncating would have hidden a credential placed
after a NUL from secret-pattern-detection and hardcoded-path-check. Everything below that
says "truncate" describes the pre-merge branch; the shipped behaviour is strip + flag.

The textual merge git produced was silently fatal, and was not taken

git auto-merged the function body into a hybrid carrying BOTH main's per-filter
split("\u0000") | join("") and this branch's array-level explode | .[0:(index(0) // length)] | implode. Strip runs first, so index(0) looked at a value with no NUL left in it and the flag
read 0 on every payload — the guards would never have fired, with no conflict marker and no test
of the pre-merge branch able to see it. The flag is now computed from the untouched values with the
strip applied after, and both the library and the guard comments say the ordering is load-bearing,
because it is exactly what the next textual merge will get wrong again.

Why the flag and the guards are still needed after #2120

#2120 closed the fail-open for the CONTENT guards. It did not close the COMMAND guards: stripping
SPLICES the bytes either side of the NUL into a token the payload never carried contiguously, and
the guards then match against that token. Re-measured at the hook boundary, origin/main at
fd075c27 versus this tree, same script, same host, on fixtures whose NUL is a real byte — verified
by decoding each fixture and counting the byte (jq -j .tool_input.command | tr -dc '\u0000' | wc -c =

1. rather than trusting that the escape survived construction:

payload	main	this change
git commit --no-verify<NUL>x	0 ALLOWED	2 blocked
git push --force<NUL>x	0 ALLOWED	2 blocked
a lone NUL	0 ALLOWED	2 blocked
a trailing NUL	0 ALLOWED	2 blocked
git commit --no-veri<NUL>fy	2 blocked	2 blocked
clean --no-verify	2	2
clean --force	2	2
harmless (git status)	0	0

Identical on both guards. The fifth row is stated, not counted: the splice happens to reassemble
a real --no-verify there, so main already blocks it and it evidences nothing about this change.
The live rows are the first four, and the first two are the ones that matter — a real --no-verify
and a real --force that main waves through. No clean command changed verdict in either
direction.

Tests re-pointed rather than deleted

Every assertion this branch wrote against truncation was measuring a value the helper no longer
produces, so each was rewritten for strip and two new cases were added: the splice
(--no-verify<NUL>x -> the single token --no-verifyx), and an ALL-NUL value, which strips to
empty — that case, and not a leading NUL, is the real reason both guards consult the flag ahead of
their empty-command skip. The guard suites keep every NUL row at exit 2; the verdict never depended
on the disposition, only its justification did, and one mislabelled row was corrected accordingly.

Conflicts and versions

• lib/hook-utils.sh — header comment and jq program, resolved by hand.
• The 16 vendored copies were regenerated with scripts/sync-hook-utils.sh, not hand-resolved;
  --check reports 16/16 byte-identical.
• 16 CHANGELOGs where both sides claimed the same version: this branch's entry moves up one patch
  above main's and is rewritten for the resolved design.
• All 16 plugin.json files had auto-merged to main's number, leaving no bump at all — no
  conflict, only --check-bump catches it, exactly the trap flagged below. Re-bumped:
  guardrails 0.23.1 -> 0.23.2, markdown-format 0.11.2 -> 0.11.3,
  source-control 0.51.2 -> 0.51.3, patch bumps for the other 13.
• Coordination with fix(markdown-format): resolve the repo root from the filesystem so nested files reach the root config without git #2130: it also bumps markdown-format to 0.11.3. Whichever merges second
  must re-bump.

main moved twice more: three merges, and one of them was silently lossy

main landed #2147, then #2140 and #2149, while this PR sat. Three merge passes, no rebase at any
point. Second pass: #2147 took guardrails to 0.24.0 and edited
block-dangerous-git.sh, which this branch also edits — resolved by keeping main's three-field
hook::jq_fields "$INPUT" '.tool_input.command' '.cwd' '.tool_name' call verbatim and appending this
branch's NUL block after it. Third pass: one changelog conflict on source-control. Every plugin
manifest had auto-merged to main's number with no bump on both passes.

The second pass exposed a defect this branch had introduced, and it is worth reading even if you
skip the rest. An earlier commit here accidentally wrote a real NUL byte into
plugins/guardrails/CHANGELOG.md — a \u0000 that was meant to be literal text in a prose
description of the fixtures. git classifies any file containing a NUL as binary, so the textual
three-way merge never ran on that changelog: it kept ours wholesale and silently discarded main's
entire 0.24.0 section, with no conflict marker and nothing in git status to distinguish it from
a file that merged cleanly. It was caught by counting NUL bytes across the touched files, not by
reading the diff. The byte is gone, the section is restored, and the changelog's 0.24.1 entry now
sits above main's 0.24.0.

That is a mistake this PR made, not a pre-existing one, and it is reported rather than quietly fixed
because the failure mode generalises: a NUL in a tracked text file turns every future merge of that
file into a silent take-ours. In a repository whose CHANGELOGs are the merge-conflict surface for
every shared-library change, that is worth knowing independently of this fix.

Incidental, and relevant to the "what I could NOT verify" list below

While posting a review reply, the harness itself refused a tool call whose command field
carried a stray control character, with command contains control characters that would be hidden in the approval dialog. That is a live observation of the validation the list below names as unverified
— it fires, and it fires on the command field. It is not the discriminating probe: it says
nothing about whether that validation runs before or after PreToolUse hooks, and nothing about
whether the rejected class includes NUL specifically rather than the control characters it does
cover. Recorded as an observation, not as evidence that the guards are unreachable. Nothing in this
change leans on it in either direction.

Gates re-run after the merge

sync-hook-utils.sh --check (16/16) - sync-hook-utils.sh --check-bump origin/main -
check-changelog-parity.sh --check / --check-bump origin/main / --check-order -
shellcheck -x with no severity floor on lib/hook-utils.sh, the bash-format vendored copy,
both guards and all three test files (rc 0 — this is what the two open review threads reported
failing; the jq-variable spelling they flagged is gone from the current program text) -
shfmt -d -i 2 (rc 0).

Suite results after the merge are in the thread below.

The defect

hook::jq_fields frames its fields with a NUL delimiter drawn from the same byte space as the
values it separates. A JSON NUL escape inside a value splits that value in two, the cardinality
check ((${#values[@]} == $#)) || return 1 fires, and both real callers spell that || exit 0 —
a PreToolUse ALLOW, emitted with no diagnostic of any kind.

One correction to the issue's mechanism, because it moves where the fix belongs. The collision is
reliably detected, not intermittently: every NUL adds exactly one record, so the count is always
N + k for k >= 1 and the check never misses. The defect therefore never lived in the library's
return value. It lives in one exit path serving two conditions with opposite correct responses —
"jq is absent or cannot parse this" (where allowing is the documented, deliberate behaviour) and
"this payload carries a NUL" (where allowing is wrong). Separating those two is the fix.

Design

jq truncates each value at its first NUL and reports the fact; the caller owns the verdict.

• lib/hook-utils.sh — each filter becomes ... | explode | .[0:(index(0) // length)] | implode.
  The separator then cannot occur inside a value, so the record count no longer depends on what a
  parseable payload holds.
• A leading record carries the NUL flag, computed from the untruncated values and emitted by the
  same jq program, so reporting it costs no second spawn. It surfaces as HOOK_JQ_FIELDS_NUL,
  assigned in the same unconditional block that resets HOOK_JQ_FIELDS — above all three return
  paths, so no early return can leak a stale 1, which in a guard would mean blocking a clean
  payload on the strength of an earlier one.
• block-no-verify.sh and block-dangerous-git.sh fail CLOSED on that flag, before their
  empty-command skip, because the helper truncates at the first NUL and a leading one therefore
  leaves an empty value that would otherwise be waved through as "no command".

Why fail CLOSED, and why that argument does not depend on the executor

No executor-fidelity claim is made here, in either direction. Two behaviours were measured and
they disagree, and which of them a hook payload actually reaches has not been traced by anyone:

measured	result
bash parsing a command it reads (stdin, script file)	discards the NUL — echo ha<NUL>rd prints hard, and --no-verify<NUL>x becomes --no-verifyx
a NUL inside an argv word handed to execve	the string simply ends there
Node v24.18.0 child_process — argv, shell: true, and execSync	refuses outright, ERR_INVALID_ARG_VALUE: must be a string without null bytes, while the same calls with a clean string run normally

An earlier draft of this PR argued that truncation was right because the executor truncates. That
was wrong — it generalised the argv case to a path that is not known to be the one in use. The
correct argument is that the design does not need it: failing closed on the flag is correct under
deletion, under truncation, and under refusal alike, so it cannot be invalidated by tracing the path
later. That is the whole case for it. Matching the value would need the trace; refusing does not.

Truncate rather than delete, on grounds that appeal to no shell

Truncation never fabricates a token the payload did not carry contiguously, and when a caller
forgets the flag it is the content class that degrades rather than the command class — a matcher
sees a prefix rather than a joined token that matches nothing. For this PR's own two callers the
choice is immaterial: they refuse on the flag before reading a value at all. It is the
conservative default, not the accurate one, and the flag is the load-bearing part.

Why the library does not block on its own

It is sourced by 15 other plugins, formatters among them, for which exiting 2 would be wrong; and a
sourced library calling exit on its caller's behalf is hidden control flow. Policy stays with the
caller and the library only reports the fact.

Rejected alternatives

Alternative	Why not
Delete the NUL (map(select(. != 0)))	Fabricates contiguity the payload did not have, and inverts which caller class degrades unsafely when a hook forgets the flag; see above. Not rejected on executor grounds.
gsub / split+join on a NUL	Both work on jq 1.8.2 here, but each puts a NUL inside the jq program text — a regex pattern and a string literal. A construct whose behaviour varied across jq builds would fail EVERY payload: a universal fail-open, strictly worse than the payload-dependent one. explode/implode use integer comparison only, with no NUL anywhere in the program. This is a reason, not a measurement — see the unverified list.
Length-prefixed framing	Needs read -N (bash 4.1+); this lib supports 3.2+.
An explicit emitted count	Redundant once the separator is absent from the value space.
Per-field @base64	Needs a base64 binary; only jq is a documented prerequisite.
@sh + eval	Puts payload-derived text through eval.
Fail closed inside the library	Impossible without the library exiting on its caller's behalf, which is wrong for the 15 other plugins.

Scope

This is a shared-library change, and the repo's own gate makes it 55 files.
plugins/guardrails/hooks/hook-utils.sh is a vendored copy; lib/hook-utils.sh is the source of
truth. CI enforces scripts/sync-hook-utils.sh --check (all 16 copies byte-identical) and
--check-bump (every carrying plugin bumped when the lib changes), so editing only the guardrails
copy would fail CI. Precedent: 9b90e35, 50 files. Hence 16 vendored copies, 16 plugin.json bumps
and 16 changelog entries, plus the lib, its test, the two guards, their two test files and the
guardrails README.

hook::jq_field — SINGULAR — is untouched. It is a separate two-line function; there is no
shared internal the two route through. grep -rn "hook::jq_field " --include=*.sh plugins/, with the
vendored copies excluded, finds 22 call sites across 12 files in claude-ops, context-guard
and source-control. None of them are touched. git diff origin/main -- lib/hook-utils.sh mentions
hook::jq_field on exactly two lines, both of them the same doc-comment cross-reference inside the
plural function's header ("Values are CR-stripped, as in hook::jq_field"); the singular
function's own body appears nowhere in the diff. Blast radius is exactly the two guards.

No other plugin is affected by the truncation. grep -rn "hook::jq_fields" --include=*.sh .,
excluding the 16 vendored copies and lib/hook-utils.*, returns exactly two call sites — both in
this PR. Every other hit across the 16 plugins is the doc comment in the vendored library. Nothing
round-trips a value into a file, and nothing compares a length or hash against one.

Versions, taken against origin/main at the time of the last rebase: guardrails 0.23.0 -> 0.23.1, markdown-format 0.11.1 -> 0.11.2, source-control 0.51.1 -> 0.51.2, and plain patch bumps
for the other 13. Worth flagging for anyone rebasing a sibling branch: when a plugin's version moved
on main mid-flight, git auto-merged the manifest to main's number, silently leaving no bump
at all — no conflict, and only sync-hook-utils.sh --check-bump catches it. That happened three
times here. #2120 is still open against the same guardrails files and owes a re-bump.

Two caller classes want opposite dispositions — which is why there is a flag

This is the strongest argument for the design, and it is demonstrated rather than theoretical.
#2120 has independently fixed the same function with the opposite disposition: at its head
9fb8383d, hook::jq_fields does ... | tostring | split("<NUL>") | join("") — it strips.

Neither disposition is simply right, because the two caller classes disagree:

payload	under strip	under truncate
content: harmless<NUL>aws_secret=AKIA… (a scanner)	secret is joined and scanned	secret is cut off and invisible
command: --no-verify<NUL>x (a guard)	joins to --no-verifyx, matches nothing, allowed	leaves --no-verify, blocked

(Which of those two readings the executor would agree with is untraced, and is not the argument —
see above. The point is only that a caller ignoring the flag degrades unsafely in one class or the
other, depending which disposition the helper picks.)

Both halves measured. The command half is the boundary table below. The content half I measured by
driving the helper directly, since no shipped hook reads .tool_input.content through it on main:

payload: .tool_input.content = "harmless preamble<NUL>aws_secret=AKIA…"
this branch (truncate)  rc=0  flag=1  value=[harmless preamble]   credential NOT visible
468bb2d9    (base)      rc=1  flag=-  value=[<none>]              credential NOT visible

So yes — truncation loses post-NUL content for a scanning caller. Stated plainly because it is a
real consequence of this design. It is not a regression (the base loses it too, and additionally
allows), and truncation is still the chosen default: it keeps the command class safe when a caller
ignores the flag, where strip keeps the content class safe instead. Strip inverts which class fails
unsafely; it does not remove the failure. Neither is chosen on executor grounds.

A single disposition cannot serve both callers. The flag is what resolves it — the helper
reports, and each caller decides: a command guard refuses outright, a content scanner refuses the
write rather than scanning a value it knows is incomplete. Either way the credential never lands.

The count, measured on 9fb8383d

Every one of the ten hooks #2120 converts calls hook::jq_fields. Zero of them consult any NUL
signal. Six own an exit 2 verdict:

hook	jq_fields calls	flag checks	exit 2 paths
secret-pattern-detection	2	0	2
hardcoded-path-check	2	0	2
block-convention-violation	2	0	3
block-hook-bypass	2	0	2
block-noncanonical-commit	2	0	5
cli-flag-verify	2	0	1
skill-reference-verify	3	0	0
stale-path-verify	3	0	0
flag-commit-pr-skill-bypass	2	0	0
workflow-resilience-check	2	0	0

Zero flag checks is expected — the flag does not exist on their branch. The point is what it implies
for whichever of us merges second: merge order does not rescue it. This PR first, then their
rebase, and the scanning hooks receive truncated values with no flag check. Theirs first, then this
one, and the same is true the moment strip becomes truncate. A reader must not conclude that this
PR makes that conversion safe. It does not. Adding the flag checks to those ten hooks is a
prerequisite for the conversion, not a follow-up — and it is theirs to do, since those hooks exist in
converted form only on their branch. This PR deliberately does not touch them.

hardcoded-path-check.sh is a third caller class worth calling out: it reads .tool_input.content,
.new_string and .new_source and owns two exit 2 paths, so it is both scanner and guard.

Per-field reachability was checked separately and holds: at their head, both
secret-pattern-detection.sh and hardcoded-path-check.sh reach exit 2 through .content and
through .new_string. (hardcoded-path-check.sh returns early unless CLAUDE_PROJECT_DIR is set,
so a probe without it exits 0 on every payload and looks exactly like "not reachable".)

#2123 needs nothing — its diff introduces zero hook::jq_fields call sites.

Merge coordination: #2120 now also edits lib/hook-utils.sh, so this is a direct conflict on the
same function rather than only on the manifest and changelog. Whoever merges second must keep both
correctness properties — the flag and the fail-closed guards from here, and the scanning-caller
requirement from there — rather than resolving by taking one side of the hunk.

Evidence

Hook boundary, before and after

Real hooks, payload piped on stdin, exit code read. BEFORE is a git archive of origin/main at
468bb2d9 — re-measured after #2123 merged, because #2123 changed
plugins/guardrails/lib/powershell/ps-command.sh, which both guards source. AFTER is this branch.
Same script, same host.

case	before	after
clean git push --no-verify / git reset --hard	2	2
clean harmless (echo hi / git status)	0	0
trailing NUL	0	2
NUL splitting the flag (--no-veri<NUL>fy)	0	2
NUL then junk (--no-verify<NUL>x)	0	2
leading NUL	0	2
NUL in an otherwise harmless command	0	2

Identical for both guards. No row where a clean command changed verdict. The <NUL>x row is the one
that matters most: it is the payload that executes as the dangerous command.

The leading-NUL row blocks for the right reason

Identical truncated content, opposite verdicts, so the flag decides rather than incidental matching:

payload	exit
"command": "" (empty, no NUL)	0
command field absent entirely	0
leading NUL, truncates to empty	2
a lone NUL and nothing else	2

Same on both guards.

Test suites, same host, baseline vs branch

Both arms ran in full, serially, on an uncontended host: every *.test.sh under
plugins/guardrails/hooks/ plus lib/hook-utils.test.sh — 14 suites, every one of them listed
below. BASELINE is the same 468bb2d9 tree used for the boundary table; BRANCH is this tip.

suite	baseline	branch	delta
lib/hook-utils.test.sh	156 / 0	162 / 0	+6 new cases
block-dangerous-git.test.sh	341 / 0	346 / 0	+5 new cases
block-no-verify.test.sh	120 / 0	127 / 0	+7 new cases
block-convention-violation.test.sh	31 / 0	31 / 0	—
block-hook-bypass.test.sh	260 / 0	260 / 0	—
block-noncanonical-commit.test.sh	202 / 0	202 / 0	—
cli-flag-verify.test.sh	52 / 0	52 / 0	—
flag-commit-pr-skill-bypass.test.sh	29 / 0	29 / 0	—
hardcoded-path-check.test.sh	94 / 0	94 / 0	—
require-jq-notice-isolation.test.sh	2 / 0	2 / 0	—
secret-pattern-detection.test.sh	52 / 0	52 / 0	—
skill-reference-verify.test.sh	96 / 0	96 / 0	—
stale-path-verify.test.sh	87 / 0	87 / 0	—
workflow-resilience-check.test.sh	16 / 0	16 / 0	—
total	1538 / 0	1556 / 0	+18, 0 failures either side

Every suite that does not exercise the new path is byte-identical across the two arms, so the +18 is
entirely the new cases. No pre-existing failure to disambiguate.

Two of the new library tests look redundant and are not: HOOK_JQ_FIELDS_NUL is checked both after
a clean payload and after an early return, each running a NUL payload first, because a
single-call test cannot observe a stale flag however it is written, and two of the three return
paths fire before any NUL could be seen.

Other gates, all re-run after the rebase

sync-hook-utils.sh --check (16/16) - sync-hook-utils.sh --check-bump origin/main -
check-changelog-parity.sh --check / --check-bump origin/main / --check-order -
check-silent-skips.sh - check-contract-clause-coverage.py -
check-cross-plugin-source-drift.sh --check - check-hook-userconfig-argv.sh -
check-plugin-manifest-presence.sh - sync-parse-concern-value.sh --check -
sync-resolve-convention-pattern.sh --check - sync-standards-contract.sh --check -
check-skill-leaf-names.sh --check - check-shell-portability.sh --paths -
shellcheck -x -S warning (rc 0) - shfmt -d -i 2 (rc 0) - markdownlint-cli2 (0 issues) -
check-manifest-duplicate-keys.py.

What this PR does NOT fix, stated rather than implied

A payload jq cannot parse still returns 1 and is still allowed. Malformed JSON, a wrongly typed
field or an empty buffer all reach the same || exit 0, exactly as before this change. Process
substitution also means jq's own exit status is never observed. That path is untouched here and out
of scope, and the header comment now says so instead of claiming — as an earlier draft of this very
fix did — that nothing a payload contains can reach it. That claim is the same reasoning shape that
produced #2122, and it should not ship inside its fix.

What I could NOT verify

• How a command actually travels from hook payload to execution. Nobody traced it. Two shell
  behaviours were measured and they disagree, and Node refuses NUL-bearing strings on every shape
  tried, so the command may never reach a shell parser at all. The design is built so this does not
  matter: fail-closed is right under deletion, truncation, and refusal alike. An earlier draft of
  this PR did lean on it, in one direction and then the other; both are gone, from the body and from
  the code comments, the README and the changelog.
• Whether the harness's control-character validation runs before or after PreToolUse hooks, and
  whether the class it rejects includes NUL specifically. The discriminating probe is
  bypass-shaped and was deliberately not run. The guard that exists is worded "contains control
  characters that would be hidden in the approval dialog" — approval-surface anti-spoofing, covering
  command / script / url only, with no equivalent on content / new_string / file_text. It
  is an implementation detail, not a documented guarantee, and nothing here leans on it in either
  direction.
• Behaviour on jq builds other than 1.8.2, and on bash other than 5.3.9 (Cygwin). The chosen
  construct uses only explode, implode, index, array slicing and any — core since jq 1.5 —
  precisely to keep that risk low, but it was not executed against an older jq. The repo's
  hook-utils-windows job exercises Git Bash on windows-2025 in CI.
• Any performance claim. The spawn count is unchanged at one, which is structural. Measured
  per-field cost of the sanitiser was below spawn noise on this host — the no-op control benchmarked
  slower than all three candidates — so no number is claimed.
• Whether a NUL payload can reach a PreToolUse hook through the harness's own serialization.
  Being settled separately. This fix does not depend on the answer: failing open on a parse failure
  is wrong regardless of how the parse came to fail.

Related

• Closes guardrails: a NUL byte in a payload value makes block-no-verify and block-dangerous-git fail open #2122 — the reproduction and the shipped-hook measurements this PR fixes.
• Refs perf(guardrails): parse each hook payload in one jq process, not two or three #2120 (perf/guardrails-jq-fields) — converts ten further guardrails hooks to
  hook::jq_fields and, at 9fb8383d, independently fixes the same function by stripping NULs.
  Direct conflict on lib/hook-utils.sh; whoever merges second must keep both correctness
  properties rather than taking one side of the hunk, and those ten hooks need HOOK_JQ_FIELDS_NUL
  checks as a prerequisite. Not closed by this PR.
• Refs fix(guardrails): stop the here-string deadlock that voided two blocking guards #2123 — merged mid-work; changed plugins/guardrails/lib/powershell/ps-command.sh, which
  both guards source, so the boundary table was re-measured against it. Introduces no
  hook::jq_fields call site, so it needs nothing from this change.
• Refs 9b90e35 (perf(hook-utils): cut three subprocess spawns per hook invocation #1979) — the precedent for a shared-lib change costing a version bump and a
  changelog entry in every carrying plugin.

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

[claude]

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

---

Security review complete

• Gather context (PR diff, description, custom instructions)
• Review lib/hook-utils.sh (hook::jq_fields logic change)
• Review block-no-verify.sh / block-dangerous-git.sh (fail-closed NUL flag usage)
• Review vendored copies for drift / inconsistency
• Check test files, README, changelog for anything security-relevant
• Post final findings as a review

Scope: reviewed only the files this PR touches — lib/hook-utils.sh, block-no-verify.sh, block-dangerous-git.sh, the 16 vendored hook-utils.sh copies, and the associated tests/README/changelogs. Per the review's lane split, unpinned-action risk, workflow trigger patterns, permissions, and template injection are deferred to zizmor — this diff touches no .github/ workflow, settings, or permissions file at all, so that lane is moot here.

No security issues found. Specifically checked for and ruled out:

• jq program injection — the prog string is built by concatenating filter arguments (lib/hook-utils.sh#L699-L702), but every call site passes only static literals ('.tool_input.command', '.tool_name') hardcoded in the two guard scripts, never payload-derived text. No attacker-controlled input reaches the jq program text itself.
• Fail-open regression via the new cardinality check — the check moved from ${#values[@]} == $# to == $# + 1 (lib/hook-utils.sh#L724) to account for the new leading flag record. Verified this can't be satisfied by a crafted payload that both trips the NUL flag and produces an output count that spuriously matches: the flag is computed from the untruncated $v array before any splitting occurs, so it accurately reflects every requested field regardless of how many records the truncation step later emits.
• False-negative on the NUL flag — traced how a raw JSON escape becomes an actual NUL codepoint after jq parses it (strings pass through `tostring` as a no-op), so `explode | index(0)` reliably detects it. A appearing only inside a re-serialized nested object/array (via tostring on non-string values) stays textually escaped rather than a real NUL byte, so it can't smuggle a literal NUL past the flag check or later split the wire separator — consistent, not a bypass.
• Ordering of the fail-closed check — confirmed in both guards that HOOK_JQ_FIELDS_NUL is tested before the empty-command skip (block-no-verify.sh#L99-L106, block-dangerous-git.sh#L101-L108), so a leading-NUL payload that truncates to an empty command can't slip through the "no command" allow path.
• Blast radius claims in the PR body — independently re-ran the greps: hook::jq_fields (plural) has exactly two call sites, both in this PR's two guards; hook::jq_field (singular, untouched) has 12 unrelated call sites across claude-ops, source-control, context-guard, none touched by this diff. Confirmed.
• Vendored copy drift — diffed lib/hook-utils.sh against the guardrails and source-control vendored copies; byte-identical, as CI's sync gate requires.
• Information disclosure — the new BLOCKED: stderr messages are static text, never echo the payload's command/content back, so no payload-derived data leaks through the hook-feedback channel.

One thing worth naming since it's easy to mistake for a gap but isn't: the flag only causes a hard block in the two guards this PR modifies. The ten hooks converted in the separate, still-open #2120 don't consult the flag at all — but that's explicitly out of scope for this diff (no files from that set are touched here), and the PR body already discloses it at length rather than leaving it implicit. Not treating it as a finding against this PR.

[claude]

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

---

Review complete

Scope reviewed: lib/hook-utils.sh (source of truth), lib/hook-utils.test.sh, plugins/guardrails/hooks/{block-no-verify,block-dangerous-git}.sh + their .test.sh files, plugins/guardrails/README.md, and the 16 vendored hook-utils.sh copies + manifest/changelog bumps. Security review (claude-security-review) is a separate lane and out of scope here.

Findings

1. CI is currently red on two independent gates — inline comment posted on lib/hook-utils.sh L702-704: the plugin-gate job's ShellCheck step fails (exit 1) on SC2016 (info) for the two new prog+='...' lines that build the jq program ($v is a jq variable, not meant to bash-expand — correct code, but ShellCheck flags it and CI doesn't pass -S warning the way the PR body's manual check did). This also fails hygiene's aggregate step, which depends on plugin-gate. Verified directly from the CI job log, not from the PR body's own claims.

2. pr-issue-linkage is failing (verified via job 93367835320): the PR body has a Closes #2122 line but no ## Related section at all, which CLAUDE.md's linkage contract requires (## Related non-empty, or N/A). This is a required check per the repo's own PR-linkage gate — the body needs a ## Related section added (N/A is fine if nothing applies) and CI re-run.

3. Minor / nit — stale figure in the PR description, not the code. The body states "guardrails 0.22.1 -> 0.22.2" but the actual diff bumps plugins/guardrails/.claude-plugin/plugin.json 0.23.0 -> 0.23.1 (source-control's 0.51.1 -> 0.51.2 claim is correct). Likely left over from before the rebase onto fix(guardrails): pin the reconstruction scan's locale so it stops decoding a literal search #2127 (which took guardrails to 0.23.0 first). Doesn't affect correctness — plugin.json and CHANGELOG.md agree with each other for all 16 plugins (independently verified) — just worth fixing in the description since the rest of the body leans heavily on exact version numbers as evidence.

What I independently verified (not just author-claimed)

• All 16 vendored hook-utils.sh copies are byte-identical to lib/hook-utils.sh (md5sum match).
• plugin.json version bumps and the corresponding CHANGELOG.md new-entry headers agree, for all 16 plugins.
• hook::jq_field (singular) body is untouched — confirmed via diff grep, no hits outside doc comments.
• Traced the hook::jq_fields jq program logic by hand: the flag record ("1"/"0") is emitted once, computed from untruncated explode arrays, ahead of N truncated value records; the cardinality check ($# + 1) correctly rejects both under- and over-counts (e.g. two concatenated JSON docs); HOOK_JQ_FIELDS_NUL is reset in the same unconditional block as HOOK_JQ_FIELDS, ahead of all three early-return paths, so no return path can leak a stale flag. No correctness bug found in the truncation/flag logic itself.
• Both guards (block-no-verify.sh, block-dangerous-git.sh) consult the flag and exit 2 before the empty-command skip, in the same order, with set -uo pipefail posing no unbound-variable risk since the flag is always set before it's read.
• Test coverage in lib/hook-utils.test.sh and both guard .test.sh files exercises leading/trailing/mid-token NUL, NUL in a non-command field, flag-staleness across calls, and error-message content — no coverage gaps found for the stated scope.

Not independently verified (author-claimed only)

• The exact pass/fail counts in the PR body's "Test suites" and "Hook boundary" tables, and the executor-behavior measurements (bash NUL-discard, Node child_process NUL-refusal) — I did not have shell/jq/bash-script execution available in this review environment (bash, jq, shellcheck all required approval that wasn't grantable here), so these are the author's own numbers, not independently reproduced. If this is a recurring limitation for this review lane, allowlisting bash, jq, and shellcheck would let a future review actually re-run the test suites and lint gates directly instead of relying on CI's own job logs.

[github-actions]

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

[claude]

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

---

Security review complete

• Gather context (PR diff, changed files)
• Review lib/hook-utils.sh (hook::jq_fields change — now strip+flag, not the truncate design in the PR body)
• Review block-no-verify.sh / block-dangerous-git.sh fail-closed logic
• Check vendored copies for drift (byte-identical, confirmed via sha256sum)
• Check for injection surfaces, GH Actions risk, secret handling
• Post final findings

Scope: lib/hook-utils.sh, lib/hook-utils.test.sh, plugins/guardrails/hooks/{block-no-verify,block-dangerous-git}.sh + tests, plugins/guardrails/README.md, and the 16 vendored hook-utils.sh copies + manifest/changelog bumps. No .github/ workflow file is touched by this diff, so the unpinned-action / trigger / permissions / template-injection lane is out of scope here (deferred to zizmor per instructions).

Note: the code on this branch has moved past what the PR description narrates. The description argues for a truncate design (explode/implode), but the tip commit (re-point the NUL cases at strip semantics) replaced that with strip (split("")|join("")) to reconcile with #2120, which merged to main first (8446af4) with its own strip-based fix. I reviewed the code as it actually is at d3af1b3, not the superseded prose.

Findings

1. IMPORTANT / confidence: CONFIRMED (fact) + PLAUSIBLE (security impact) — the PR's blast-radius claim is stale, and the gap it describes as hypothetical is already live on main.

The PR body states (and a prior review round trusted): "hook::jq_fields, excluding vendored copies, returns exactly two call sites — both in this PR" and frames #2120 as "still open", with the flag-check gap for its ten converted hooks as a future merge-order risk ("Whoever merges second must keep both correctness properties").

That's no longer accurate. git log shows #2120 (8446af4, "parse each hook payload in one jq process") already merged to main, and this branch already pulled it in via 11dcc3b. Right now, on this exact branch (and on main), there are 12 non-vendored call sites, not 2 — confirmed by direct grep, not the stale count in the description. Of those, five perform command/argv-pattern matching with a documented exit 2 block path and do not consult HOOK_JQ_FIELDS_NUL:

• block-hook-bypass.sh:78
• block-noncanonical-commit.sh:123
• block-convention-violation.sh:77
• cli-flag-verify.sh:81
• workflow-resilience-check.sh:52

(the remaining five — hardcoded-path-check.sh, secret-pattern-detection.sh, skill-reference-verify.sh, stale-path-verify.sh, flag-commit-pr-skill-bypass.sh — are content-scanning, which the PR body's own reasoning treats as lower-risk under strip semantics since splicing preserves byte order/adjacency rather than truncating).

This isn't a defect this diff introduces — the strip behavior and the unflagged callers both predate it, arriving via #2120. But this PR is the one that performed the #2120 merge integration (11dcc3b → d3af1b3), had full visibility into the resulting state, and is the natural place either to add the same fail-closed flag check to the five matcher hooks above or to correct the description's scope claims so a reader isn't relying on a "two call sites, still hypothetical" picture that's no longer true. I did not verify per-hook exploitability for all five (that requires tracing each hook's own matching logic, which is outside this diff) — I spot-checked block-hook-bypass.sh, which matches substrings/patterns rather than exact argv-token equality, so it's less exposed to the specific --no-verify<NUL>x-style splice than block-no-verify.sh's exact-flag check was. The other four were not individually traced.

Suggested fix: either (a) add the same if ((HOOK_JQ_FIELDS_NUL)); then ... exit 2; fi guard used in block-no-verify.sh/block-dangerous-git.sh to the five matcher hooks listed above, or (b) if that's intentionally deferred to a follow-up, update the PR description's "exactly two call sites" / "#2120 still open" claims to reflect that #2120 has already merged and the gap is live now, not contingent on merge order.

What I checked and found clean

• jq program injection — filter values are only ever hardcoded literals from the two guard call sites; no payload-derived text reaches the jq program text.
• The rewritten hook::jq_fields — flag computed from map(explode | index(0) != null) | any) over the unstripped array before the per-value split("")|join(""); HOOK_JQ_FIELDS_NUL=0 is reset unconditionally ahead of all three early-return paths ((($#)), missing jq, cardinality mismatch), so no early return can leak a stale 1 or a stale 0. Cardinality check ($# + 1) correctly accounts for the new leading flag record.
• The ShellCheck SC2016 finding from the earlier review round is resolved — the rewritten program no longer uses a jq-side $v variable inside single-quoted bash literals (confirmed via grep -n '\$v\b', no matches), so that CI failure no longer applies to this code.
• Ordering in both guards — HOOK_JQ_FIELDS_NUL is checked before the empty-command skip in both block-no-verify.sh and block-dangerous-git.sh, so a leading-NUL payload that strips to empty can't slip through the "no command" allow path.
• Vendored-copy drift — all 16 copies are byte-identical to lib/hook-utils.sh (sha256sum match).
• No .github/ workflow, settings, or permissions file is touched by this diff.
• No secrets/credentials handling — the new BLOCKED: stderr messages are static text, never echo payload content back.
  · branch fix/hook-utils-nul-fail-open

[claude]

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

---

Code review complete

• Gather context (PR diff at latest sync, CLAUDE.md guidelines)
• Review lib/hook-utils.sh diff since last review round
• Review lib/hook-utils.test.sh diff
• Review the two guard scripts + their test files
• Check CI status for previously-flagged ShellCheck SC2016 issue
• Post findings

Scope: this review is scoped to correctness, architecture, error handling, test coverage, and maintainability per CLAUDE.md/the lane split — security review is left to claude-security-review, already posted separately. I reviewed the full diff against origin/main, with extra attention to what changed since the prior review round: the two newest commits (11dcc3b merging origin/main — which brought in #2120's independently-landed strip fix for the same function — and d3af1b3 re-pointing tests at strip semantics).

Finding

plugins/guardrails/CHANGELOG.md line 16 contains a raw, unescaped NUL byte, and it is the live cause of this PR's ci failure. Permalink

The sentence "...is a real byte decoded from a JSON ` ` escape:" was clearly meant to show the escape sequence as literal text between backticks, but an actual 0x00 byte landed there instead of its escaped representation. I could not post this as an inline PR comment — GitHub rejects it because the file's diff has no resolvable line/hunk (see next paragraph) — so it's reported here instead, independently verified three ways in this checkout:

1. file plugins/guardrails/CHANGELOG.md → data, not UTF-8 text — every other file this PR touches, including all 15 sibling CHANGELOG.md files, reports clean UTF-8 text.
2. git diff renders this file as Binary files a/plugins/guardrails/CHANGELOG.md and b/plugins/guardrails/CHANGELOG.md differ — no textual diff at all, so this addition isn't reviewable on GitHub's normal file-diff view either.
3. Traced it directly to the currently-failing ci run (31394028937, job hygiene, editorconfig-checker step): plugins/guardrails/CHANGELOG.md: Wrong character encoding ("binary" instead of "utf-8"). That's the only failing check right now.

Fix: replace the raw NUL byte with the literal escaped text (e.g. ``) so the line reads as prose again, then re-run editorconfig-checker locally to confirm.

What I verified and found correct

• The previously-flagged ShellCheck SC2016 failure (two earlier review-round comments) is resolved, not by adding a shellcheck disable, but because the merge with perf(guardrails): parse each hook payload in one jq process, not two or three #2120 replaced the explode/implode-with-$v-jq-variable truncate construct with a split("") | join("") strip construct that never puts a jq variable inside a single-quoted bash literal. Confirmed no $v remains anywhere in lib/hook-utils.sh, and all 16 vendored copies are still byte-identical to it.
• pr-issue-linkage now passes (previously flagged as failing for a missing ## Related section).
• hook::jq_fields logic is correct under the merged strip semantics: HOOK_JQ_FIELDS_NUL is computed from the untruncated array before the strip (ordering is load-bearing and is called out in a comment), reset unconditionally ahead of all three early-return paths so no stale flag can leak, and the $# + 1 cardinality check can't be spuriously satisfied since NUL is fully absent from every stripped value.
• Both guards (block-no-verify.sh, block-dangerous-git.sh) check the flag before the empty-command skip, in the same order, so an all-NUL command (which strips to an empty string) can't slip through as "no command."
• Test coverage is thorough for the new behavior: leading/trailing/mid-token/all-NUL cases, the new splice case (--no-verify<NUL>x → --no-verifyx, flagged), a literal `` text field that must not trip the flag, and flag-staleness across both a clean call and an early-return call. No gaps found for the stated scope.
• The guardrails CHANGELOG.md/plugin.json (0.23.2) content is otherwise accurate and matches the current merged code, not the pre-merge design — good, since the PR body itself (written before the origin/main merge) still describes the superseded truncate design and doesn't mention that perf(guardrails): parse each hook payload in one jq process, not two or three #2120 landed with strip and this branch merged on top of it. Worth a description update for anyone reading it fresh, but not a code defect.

Not independently verified

• The PR body's test-suite pass/fail counts and boundary-table measurements — I don't have bash/jq execution available in this review environment, so these remain author-claimed, not independently reproduced here.
  · branch fix/hook-utils-nul-fail-open

[claude]

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

---

Security review complete

• Gather context (PR diff via gh pr diff, prior review rounds on this PR)
• Review lib/hook-utils.sh diff since last security review round
• Review the two guard scripts (block-no-verify.sh, block-dangerous-git.sh)
• Check vendored copies for drift
• Check CHANGELOGs / README for the previously-flagged raw NUL byte issue
• Post findings

Scope: all 55 changed files at HEAD 435b2fe — lib/hook-utils.sh (source of truth) and its test, the 16 vendored hooks/hook-utils.sh copies + plugin.json version bumps + CHANGELOG.md entries, plugins/guardrails/README.md, and the two guards (block-no-verify.sh, block-dangerous-git.sh) + their test files. No .github/ workflow, settings, or permissions file is touched by this diff, so unpinned-action risk, trigger patterns, permissions, and template injection are out of scope for this lane (deferred to zizmor).

No security issues found

This round re-verifies the code as it now sits after two merges from origin/main (pulling in #2120's independently-landed strip fix and #2123's here-string fix) plus the d3af1b3 test re-point commit. The design landed is strip + fail-closed flag, not the truncate design the PR description originally argued for — I reviewed the actual code, not the superseded prose.

• Prior CI-blocking finding is resolved. The two earlier review rounds flagged a live ShellCheck SC2016 failure from a jq-side $v variable inside single-quoted bash literals (prog+='... $v ...'). That construct is gone: the merge with perf(guardrails): parse each hook payload in one jq process, not two or three #2120 replaced it with A | (X, Y), feeding the same array literal to both the flag computation and the value stripping, with no jq variable anywhere in the program text. Confirmed via grep -n '\$v\b' lib/hook-utils.sh — no matches.
• Prior CI-blocking finding is resolved. The raw, unescaped NUL byte previously found in plugins/guardrails/CHANGELOG.md (line 16), which made that file data/binary and failed the editorconfig-checker step, is gone. file lib/hook-utils.sh plugins/guardrails/CHANGELOG.md plugins/actionlint/CHANGELOG.md plugins/bash-format/CHANGELOG.md now reports clean Unicode text, UTF-8 text for all of them.
• jq program injection — filter arguments to hook::jq_fields are still only ever hardcoded string literals from the two guard call sites ('.tool_input.command', '.cwd', '.tool_name'); no payload-derived text reaches the jq program text.
• Fail-open regression via the cardinality check — ((${#values[@]} == $# + 1)) correctly accounts for the new leading flag record. The flag is computed from the untruncated array (map(explode | index(0) != null) | any) strictly before the per-value split("") | join(""), and the code comment explicitly calls out that this ordering is load-bearing — the exact hazard a naive textual merge would reintroduce. Traced this by hand against the current program text (lib/hook-utils.sh around the hook::jq_fields body) and it holds.
• HOOK_JQ_FIELDS_NUL cannot leak stale state — reset to 0 unconditionally in the same block as HOOK_JQ_FIELDS=(), ahead of all three early-return paths (no filters, missing jq, cardinality mismatch), so a guard can never inherit a stale 1 from a prior invocation nor silently keep a stale 0 after a NUL-bearing call.
• Guard ordering — both block-no-verify.sh and block-dangerous-git.sh check HOOK_JQ_FIELDS_NUL and exit 2 before the empty-command skip, so a leading/all-NUL command that strips to an empty string can't slip through the "no command" allow path.
• Vendored-copy drift — all 16 plugins/*/hooks/hook-utils.sh copies share the identical blob SHA dcb0e820f... with lib/hook-utils.sh per the changed-files listing (byte-identical), consistent with the repo's sync gate.
• Test files (lib/hook-utils.test.sh, both guard .test.sh files) build NUL-bearing fixtures entirely inside jq -n ([0] | implode), never via a raw byte or shell interpolation in the test source itself — no injection surface there, and no payload content is echoed back in the new BLOCKED: stderr messages (static text only).
• CHANGELOG / README / plugin.json content — plain prose and version bumps, no injection surface, consistent across all 16 plugins.

This is the fifth automated review pass on this PR; the substantive findings from the first four rounds (stale blast-radius count relative to #2120, the ShellCheck SC2016 CI failure, the missing ## Related PR-body section, and the raw-NUL-byte CHANGELOG file) have each already been addressed in the commits since. Nothing new surfaced in this pass.
· branch fix/hook-utils-nul-fail-open

[claude]

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

---

Code review complete

• Gather context (gh pr diff, CLAUDE.md, prior review rounds)
• Review lib/hook-utils.sh diff (post-sync commit)
• Review lib/hook-utils.test.sh diff
• Review the two guard scripts + their test files
• Review plugins/guardrails/README.md and CHANGELOG entries
• Check vendored-copy sync consistency
• Post findings as inline comments + summary

Scope: correctness, architecture, error handling, test coverage, maintainability — per the lane split, security implications (e.g. the blast-radius/unflagged-caller question already raised in the security lane's latest round) are left to claude-security-review. Reviewed the tip of this branch (435b2fe, which merges origin/main a second time), not the pre-merge design the PR body originally described.

Traced correctness of the core change

Walked the rewritten hook::jq_fields jq program by hand against lib/hook-utils.sh L698-L750:

• The flag record (map(explode | index(0) != null) | any) is computed over the raw tostring'd array before the per-value split("")|join("") strip — ordering matches what the header comment (L664-L668) mandates, and I confirmed there's no $v jq-variable left anywhere that a textual merge could reorder around.
• HOOK_JQ_FIELDS_NUL=0 and HOOK_JQ_FIELDS=() are both reset unconditionally at the top of the function, ahead of all three early-return paths ((($#)), missing jq, cardinality mismatch) — no path leaks a stale flag from a prior call.
• Cardinality check is $# + 1 (flag record + N values), which correctly accounts for the new leading record without reopening the original over/under-count bug.
• Both guards (block-no-verify.sh [L99-L106], block-dangerous-git.sh L125-L130) check HOOK_JQ_FIELDS_NUL before their empty-command skip, so an all-NUL command (strips to "") can't slip through as "no command."

This matches the design the PR body argues for, and I didn't find a logic error in it.

Two items from prior review rounds are now resolved on this tip, confirmed directly (not re-flagged)

• The ShellCheck SC2016 finding from the two earlier inline review comments (on $v-inside-single-quotes) no longer applies — grep -n '\$v\b' lib/hook-utils.sh returns no matches on this tip; the strip/flag construct doesn't use a jq-side $v variable at all.
• The raw-NUL-byte-in-CHANGELOG.md finding from the previous review round is fixed — file plugins/guardrails/CHANGELOG.md now reports Unicode text, UTF-8 text, and the line in question reads as escaped `` prose.
• Current CI (mcp__github_ci__get_ci_status) shows 4/6 checks passed, 0 failed, 2 still in progress (ci, this review) — consistent with those fixes holding.

Finding (inline, minor)

Posted on block-dangerous-git.test.sh:849-863: block-dangerous-git.sh emits the identical three-line BLOCKED:/What a guard can.../Fix: reissue... stderr message as block-no-verify.sh (byte-for-byte, confirmed via grep), but only block-no-verify.test.sh asserts on the message content — block-dangerous-git.test.sh's run_nul cases check exit codes only. A future edit to one guard's wording wouldn't be caught by the other's test suite. Low severity, easy parity fix.

What I did not independently verify

The PR body's test-suite pass/fail counts and boundary-measurement tables — no bash/jq execution tool is granted to this review lane (consistent with what earlier review rounds reported), so I traced the jq program logic by hand and via diff inspection rather than running the suites myself. Treat those counts as author-claimed. CI's own ci run (in progress at time of this review) is the independent signal for pass/fail.

Everything else checked and found consistent

• All 16 vendored hook-utils.sh copies carry the identical git blob SHA (dcb0e820f...) per the PR's file list — byte-identical, as the sync gate requires.
• plugin.json/CHANGELOG.md version bumps line up across all 16 plugins spot-checked (actionlint 0.8.2→0.8.3, guardrails 0.24.0→0.24.1).
• README.md and CHANGELOG.md prose for guardrails accurately describes the strip (not truncate) design actually shipped on this tip, unlike the PR body's original (now-superseded, and now corrected) narration.
  · branch fix/hook-utils-nul-fail-open

[claude]

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

---

Security review complete

• Gather context (gh pr diff / git diff against origin/main, prior review rounds)
• Diff since last security review round (435b2fe → 043d60c)
• Review lib/hook-utils.sh / vendored copies for drift
• Review the two guard scripts for regressions
• Post findings

Scope: all 55 files this PR changes relative to origin/main — lib/hook-utils.sh (source of truth) + its test, the 16 vendored hooks/hook-utils.sh copies + plugin.json bumps + CHANGELOG.md entries, plugins/guardrails/README.md, and the two guards (block-no-verify.sh, block-dangerous-git.sh) + their test files. No .github/ workflow, settings, or permissions file is touched, so unpinned-action risk, trigger patterns, permissions, and template injection stay out of scope for this lane (deferred to zizmor).

No security issues found

This is a synchronize trigger on a PR that has already been through five prior automated security-review passes on this thread. I diffed the current tip (043d60c) against the last commit a security review actually ran on (435b2fe) and confirmed no security-relevant file changed in that window:

• git diff 435b2fe..HEAD touching lib/, any vendored hooks/hook-utils.sh, any plugin.json, any CHANGELOG.md, or the two guard scripts / their tests — empty. The hook::jq_fields logic, the fail-closed HOOK_JQ_FIELDS_NUL checks in both guards, and all 16 vendored copies are byte-for-byte what the prior round (which found no issues) already reviewed.
• The only content that changed in this window is plugins/guardrails/README.md (8 lines) — a pure prose fix renaming /pull-request create → /source-control:pull-request create and /commit → /source-control:commit in documentation text, no code or security-relevant behavior.
• Everything else in the 043d60c merge commit (docs/, plugins/claude-ops/, plugins/planning/, plugins/source-control/skills/commit/scripts/exec-bit-check.sh, etc.) came in from origin/main via the merge and predates the merge-base (cf743d6) — confirmed via git diff cf743d6..HEAD --stat, which reproduces exactly this PR's own 55-file set with no unrelated files present. None of it is this PR's own contribution.

Re-affirming what the prior rounds already established and which still holds on this tip:

• jq program injection — filter args to hook::jq_fields are hardcoded literals only, never payload-derived.
• Fail-open regression via the cardinality check — $# + 1 correctly accounts for the leading flag record; the flag is computed from the untruncated/unstripped array strictly before the strip, and a code comment marks that ordering as load-bearing.
• HOOK_JQ_FIELDS_NUL can't leak stale state — reset unconditionally ahead of all three early-return paths.
• Both guards check the flag before their empty-command skip — an all-NUL command can't slip through as "no command."
• Vendored-copy drift — all 16 copies share the identical blob SHA with lib/hook-utils.sh.
• No secrets/credential handling — the BLOCKED: stderr messages are static text, never echo payload content.
  · branch fix/hook-utils-nul-fail-open

[claude]

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

---

Code review complete

• Gather context (gh pr diff against origin/main, prior review rounds on this PR)
• Review lib/hook-utils.sh diff since the last review round (now at 043d60c, a third origin/main merge)
• Review lib/hook-utils.test.sh diff
• Review the two guard scripts + their test files
• Review plugins/guardrails/README.md / CHANGELOG entries
• Check vendored-copy sync consistency
• Check CI status
• Post findings

Scope: correctness, architecture, error handling, test coverage, maintainability — security is left to the claude-security-review lane, already posted separately for this HEAD. Reviewed the full 55-file diff against origin/main at tip 043d60c.

No new findings

This sync (435b2fe → 043d60c) is a third merge of origin/main into the branch, adding one unrelated commit (cf743d6, a source-control exec-bit-check fix). I diffed 435b2fe..043d60c for every file this PR's own scope touches and confirmed none of the core logic changed: lib/hook-utils.sh, lib/hook-utils.test.sh, both guard scripts, and their test files are byte-identical to what the previous review round (posted 2026-08-10T14:26–14:30Z) already covered. The only content delta in scope is plugins/guardrails/README.md picking up two unrelated skill-name updates (/pull-request create → /source-control:pull-request create) from origin/main — documentation only, no behavioral surface.

Independently re-verified that the two CI-blocking issues from earlier rounds are still resolved at this tip:

• No jq-side $v variable remains in lib/hook-utils.sh (grep -n '\$v\b' — no matches), so the ShellCheck SC2016 false-positive is gone for good, not just patched around.
• plugins/guardrails/CHANGELOG.md and lib/hook-utils.sh both report clean Unicode text, UTF-8 text via file — the earlier raw-NUL-byte corruption has not recurred.
• Spot-checked guardrails: plugin.json version (0.24.1) matches the CHANGELOG.md top entry (## [0.24.1]).

gh/CI status is green: ci, pr-title, pr-issue-linkage, and do-not-merge all passed; claude-security-review completed successfully for this tip.

Still outstanding (already flagged, unchanged, non-blocking)

The minor test-coverage-parity gap from the previous round's inline comment on block-dangerous-git.test.sh is still present: block-dangerous-git.sh emits the identical three-line BLOCKED: stderr message as block-no-verify.sh, but only block-no-verify.test.sh asserts on the message content — block-dangerous-git.test.sh's run_nul cases (L849-L861) still check exit codes only. Not re-posting a duplicate inline comment since the file is unchanged since that finding was raised — the existing thread already covers it.

Not independently verified

bash/jq/shellcheck execution is not available to this review lane in this environment (approval not grantable for running scripts), so the PR body's test-suite pass/fail counts and boundary-measurement tables remain author-claimed, not independently reproduced here. CI's own green ci run is the independent signal for pass/fail at this tip.
· branch fix/hook-utils-nul-fail-open

[kyle-sexton]

Suite and boundary results after the three merges

Scoped honestly: this is what was re-run on this host after the merges, and what was not. The host is
heavily contended (several agents running concurrently), which is why the list is shorter than the
14-suite table in the body — that table was measured before main moved and should be read as
historical, not current.

Re-run and green

suite	result	note
lib/hook-utils.test.sh	PASS=172 FAIL=0	includes the re-pointed NUL cases and the two new ones
plugins/guardrails/hooks/block-no-verify.test.sh	PASS=128 FAIL=0	measured at the second-merge tree; the third merge touched only plugins/source-control/CHANGELOG.md

shellcheck -x with no severity floor over lib/hook-utils.sh, plugins/bash-format/hooks/hook-utils.sh,
both guards and all three test files: rc 0. shfmt -d -i 2: rc 0.
sync-hook-utils.sh --check: 16/16 byte-identical. sync-hook-utils.sh --check-bump origin/main,
check-changelog-parity.sh --check / --check-bump origin/main / --check-order: all clean.

Not re-run — stated rather than implied

• block-dangerous-git.test.sh did not finish on this host. It was started three times and
  killed or outrun each time by machine contention. Its NUL rows are covered directly by the
  boundary measurement below, but the suite's own 346-case run is unverified after the merges.
• The other eleven guardrails suites were not re-run. Those are the hooks perf(guardrails): parse each hook payload in one jq process, not two or three #2120 converted to this
  helper. sync-hook-utils.sh --check proves the sixteen vendored copies are byte-identical to
  lib/hook-utils.sh; it proves nothing about those hooks' behaviour against the changed library.
  The library change they see is additive — a new global, and the flag computed before the same
  strip they already had — but that is an argument, not a measurement.

Boundary, re-measured against CURRENT main

The body's table cites main at fd075c27. #2147 has since rewritten how block-dangerous-git
reads its payload fields, so the two live rows were re-measured against main at 04822fc4:

row	main @ 04822fc4	this branch @ 043d60ce
git commit --no-verify<NUL>x (block-no-verify)	0 ALLOWED	2 blocked
git push --force<NUL>x (block-dangerous-git)	0 ALLOWED	2 blocked
clean --no-verify	2	2
clean --force	2	2
harmless git status	0	0

Both bypasses are still live on today's main and still closed here. No clean command changed
verdict in either direction.

Fixture integrity was checked rather than assumed: each payload is built with jq -n carrying a
JSON \u0000 escape, and every fixture was confirmed to decode to exactly one real NUL byte
(jq -j .tool_input.command | tr -dc '\0' | wc -c = 1) before any exit code was read. A NUL fixture
that silently fails to construct reads as a clean pass and would mean nothing.

git commit --no-veri<NUL>fy is deliberately excluded from the live rows: stripping reassembles
a real --no-verify there, so main already blocks it and it evidences nothing about this change.

[claude]

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

---

Security review complete

• Gather context (gh pr diff / prior review rounds on this thread)
• Diff since last security review round (043d60c → 10f8da3)
• Re-verify no security-relevant surface changed
• Post findings

Scope: all 55 files this PR changes relative to origin/main — lib/hook-utils.sh (source of truth) + its test, the 16 vendored hooks/hook-utils.sh copies + plugin.json bumps + CHANGELOG.md entries, plugins/guardrails/README.md, and the two guards (block-no-verify.sh, block-dangerous-git.sh) + their test files. No .github/ workflow, settings, or permissions file is touched, so unpinned-action risk, trigger patterns, permissions, and template injection stay out of scope for this lane (deferred to zizmor).

No security issues found

This is a synchronize trigger. The only change since the last security-review pass (043d60c) is a single commit, 10f8da3:

plugins/guardrails/hooks/block-dangerous-git.test.sh | 14 ++++++++++++++
1 file changed, 14 insertions(+)

It adds two assert_contains checks (stderr message content: "NUL byte" and "reissue the tool call without the embedded NUL") to block-dangerous-git.test.sh's NUL cases, mirroring the assertions already present in block-no-verify.test.sh. This directly resolves the test-coverage-parity gap the code-review lane flagged in an earlier round — it's test-only, adds no new code path, and doesn't touch lib/hook-utils.sh, either guard script, any vendored copy, or any plugin.json/CHANGELOG.md. No security surface changed.

Re-affirming what prior rounds on this thread already established and which still holds at 10f8da3:

• jq program injection — filter args to hook::jq_fields are hardcoded literals only, never payload-derived.
• Fail-open regression via the cardinality check — $# + 1 correctly accounts for the leading flag record; the flag is computed from the untruncated/unstripped array strictly before the strip, and a code comment marks that ordering as load-bearing.
• HOOK_JQ_FIELDS_NUL can't leak stale state — reset unconditionally ahead of all three early-return paths.
• Both guards check the flag before their empty-command skip — an all-NUL command can't slip through as "no command."
• Vendored-copy drift — all 16 copies share the identical blob SHA with lib/hook-utils.sh.
• No secrets/credential handling — the BLOCKED: stderr messages (now asserted identically in both test files) are static text, never echo payload content.
  · branch fix/hook-utils-nul-fail-open

[claude]

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

---

Code review complete

• Gather context (git diff origin/main...HEAD, prior review rounds on this PR)
• Review what changed since the last review round (043d60c → 10f8da3)
• Check CI status
• Post findings

Scope: this sync adds exactly one commit, touching exactly one file: plugins/guardrails/hooks/block-dangerous-git.test.sh (+14 lines). Everything else in the 55-file diff is unchanged since the previous round's code-review pass (which found no issues in it), so this review is scoped to that one addition.

The change resolves the parity gap flagged in the prior round

The previous code-review pass on this PR left an inline comment: block-dangerous-git.sh emits the identical three-line BLOCKED:/What a guard can.../Fix: reissue... stderr message as block-no-verify.sh, but only block-no-verify.test.sh asserted on the message content — block-dangerous-git.test.sh's NUL cases checked exit codes only, so a wording drift between the two guards wouldn't be caught by either suite.

This commit adds a nul_stderr helper plus two assert_contains checks to block-dangerous-git.test.sh, mirroring the existing pattern in block-no-verify.test.sh line-for-line (nul_stderr helper, same jq -n --arg h --arg t payload construction, same two assertions). Traced by hand against the actual guard source:

• plugins/guardrails/hooks/block-dangerous-git.sh:126 emits "BLOCKED: the payload carries a NUL byte, which a command cannot reliably carry." — matches the new assertion's "NUL byte" substring.
• plugins/guardrails/hooks/block-dangerous-git.sh:128 emits "Fix: reissue the tool call without the embedded NUL." — matches the new assertion's "reissue the tool call without the embedded NUL" substring.
• The fixture head (git reset --hard) is a pattern block-dangerous-git.sh actually recognizes as dangerous, so the NUL-carrying payload reaches the block path this test means to exercise rather than falling through on a pattern miss.
• HOOK and assert_contains are both already in scope from the file's existing header (guardrails-test-helpers.sh source, HOOK="$HOOK_DIR/block-dangerous-git.sh"), so the addition is self-contained and consistent with the surrounding file's conventions.

No correctness issues found in the addition. The new comment block explaining why the duplication is intentional (both guards share wording by design; exit-code-only coverage can't see a message-only drift) is a good call — it preempts a future reader from "deduplicating" the assertion into a shared helper and losing the parity check the duplication exists to provide.

Not independently executed

bash/jq execution isn't available to this review lane in this environment (approval isn't grantable here), so I traced the assertion logic against the guard's actual stderr text and the sibling test file by hand/grep rather than running the suite. CI (ci job) was still queued at review time; pr-title, pr-issue-linkage, do-not-merge, and claude-security-review had already completed successfully with no failures.
· branch fix/hook-utils-nul-fail-open

[claude]

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

---

Security review complete

• Gather context (PR diff, prior review rounds already on this thread)
• Diff current HEAD against last-reviewed commit (10f8da3 → faff1a4)
• Review the new content for security issues
• Post findings

Scope: all 55 files this PR changes relative to origin/main — lib/hook-utils.sh (source of truth) + its test, the 16 vendored hooks/hook-utils.sh copies + plugin.json bumps + CHANGELOG.md entries, plugins/guardrails/README.md, and the two guards (block-no-verify.sh, block-dangerous-git.sh) + their test files. No .github/ workflow, settings, or permissions file is touched, so unpinned-action risk, trigger patterns, permissions, and template injection stay out of scope for this lane (deferred to zizmor).

No security issues found

This is a synchronize trigger. The only change since the last security-review pass (10f8da3) is a single commit, faff1a4:

plugins/guardrails/hooks/block-dangerous-git.test.sh | 9 +++++++++
plugins/guardrails/hooks/block-no-verify.test.sh     | 9 +++++++++
2 files changed, 18 insertions(+)

It adds one paired test case to each guard's suite — an all-NUL command (nul_stderr '' '') asserted to be blocked for the NUL reason, and an empty command with no NUL asserted to be allowed (exit 0) via the existing empty-command skip. This pins the ordering already reviewed and confirmed in prior rounds (HOOK_JQ_FIELDS_NUL checked before the empty-command skip in both guards) with an explicit test pair rather than changing it. Test-only: no change to lib/hook-utils.sh, either guard script, any vendored copy, or any plugin.json/CHANGELOG.md. No new security surface.

Re-affirming what prior rounds on this thread already established and which still holds at faff1a4:

• jq program injection — filter args to hook::jq_fields are hardcoded literals only, never payload-derived.
• Fail-open regression via the cardinality check — $# + 1 correctly accounts for the leading flag record; the flag is computed from the untruncated/unstripped array strictly before the strip, and a code comment marks that ordering as load-bearing.
• HOOK_JQ_FIELDS_NUL can't leak stale state — reset unconditionally ahead of all three early-return paths.
• Both guards check the flag before their empty-command skip — an all-NUL command can't slip through as "no command" (now also pinned by the new test pair above).
• Vendored-copy drift — all 16 copies share the identical blob SHA with lib/hook-utils.sh.
• No secrets/credential handling — the BLOCKED: stderr messages are static text, never echo payload content.
  · branch fix/hook-utils-nul-fail-open

[claude]

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

---

Security review complete

• Gather context (git log/git show against prior review rounds on this thread)
• Diff current HEAD (ceedf32) against last-reviewed commit
• Re-verify no new security-relevant surface changed
• Post findings

Scope: all 55 files this PR changes relative to origin/main — lib/hook-utils.sh (source of truth) + its test, the 16 vendored hooks/hook-utils.sh copies + plugin.json bumps + CHANGELOG.md entries, plugins/guardrails/README.md, and the two guards (block-no-verify.sh, block-dangerous-git.sh) + their test files. No .github/ workflow, settings, or permissions file is touched, so unpinned-action risk, trigger patterns, permissions, and template injection stay out of scope for this lane (deferred to zizmor).

No security issues found

This is a synchronize trigger. The tip commit, ceedf32, is test-only:

plugins/guardrails/hooks/block-dangerous-git.test.sh | 9 +++++++++
plugins/guardrails/hooks/block-no-verify.test.sh     | 9 +++++++++
2 files changed, 18 insertions(+)

It adds one pinning pair to each guard suite: an all-NUL command asserted to be blocked with the NUL reason on stderr, and an empty command with no NUL asserted to still take the existing empty-command skip. This is the same content already reviewed and cleared in the immediately preceding security-review round on this thread (posted against the equivalent commit) — verified by diffing the two, byte-for-byte identical addition, just carried forward. No change to lib/hook-utils.sh, either guard script, any vendored copy, or any plugin.json/CHANGELOG.md. No new security surface.

Re-affirming what prior rounds on this thread already established and which still holds at ceedf32:

• jq program injection — filter args to hook::jq_fields are hardcoded literals only, never payload-derived.
• Fail-open regression via the cardinality check — $# + 1 correctly accounts for the leading flag record; the flag is computed from the untruncated/unstripped array strictly before the strip, and a code comment marks that ordering as load-bearing.
• HOOK_JQ_FIELDS_NUL can't leak stale state — reset unconditionally ahead of all three early-return paths.
• Both guards check the flag before their empty-command skip — an all-NUL command can't slip through as "no command" (now pinned by an explicit ordering-boundary test pair in both suites, added by this exact commit).
• Vendored-copy drift — all 16 copies share the identical blob SHA with lib/hook-utils.sh.
• No secrets/credential handling — the BLOCKED: stderr messages are static text, never echo payload content.
  · branch fix/hook-utils-nul-fail-open

[claude]

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

---

Security review complete

• Gather context (gh pr diff / prior review rounds already on this thread)
• Diff current HEAD (4aff456) against the last fully-reviewed commit (043d60c)
• Re-verify no new security-relevant surface changed
• Post findings

Scope: all 55 files this PR changes relative to origin/main — lib/hook-utils.sh (source of truth) + its test, the 16 vendored hooks/hook-utils.sh copies + plugin.json bumps + CHANGELOG.md entries, plugins/guardrails/README.md, and the two guards (block-no-verify.sh, block-dangerous-git.sh) + their test files. No .github/ workflow, settings, or permissions file is touched, so unpinned-action risk, trigger patterns, permissions, and template injection stay out of scope for this lane (deferred to zizmor).

No security issues found

git diff 043d60c..HEAD --stat confirms exactly two files changed since the last full security-review pass on this thread, both test-only:

plugins/guardrails/hooks/block-dangerous-git.test.sh | 23 ++++++++++++++++++++
plugins/guardrails/hooks/block-no-verify.test.sh     |  9 +++++++++
2 files changed, 32 insertions(+)

The two commits since then (10f8da3, 4aff456) add:

1. assert_contains checks in block-dangerous-git.test.sh for the BLOCKED: stderr message content on the NUL path — mirroring assertions already present in block-no-verify.test.sh, closing a coverage-parity gap the code-review lane flagged earlier (message-content drift between the two guards wasn't caught by either suite before).
2. A pinning pair in both test files: an all-NUL command asserted blocked with the NUL reason, paired with a NUL-free empty command asserted still allowed via the pre-existing empty-command skip. This pins the ordering already reviewed and confirmed correct in prior rounds (HOOK_JQ_FIELDS_NUL checked before the empty-command skip in both guards) with an explicit test, rather than changing behavior.

No change to lib/hook-utils.sh, either guard script, any vendored copy, or any plugin.json/CHANGELOG.md. No new security surface — this diff only adds assertions against already-shipped, already-reviewed behavior.

Re-affirming what prior rounds on this thread already established and which still holds at 4aff456:

• jq program injection — filter args to hook::jq_fields are hardcoded literals only, never payload-derived.
• Fail-open regression via the cardinality check — $# + 1 correctly accounts for the leading flag record; the flag is computed from the untruncated/unstripped array strictly before the strip, and a code comment marks that ordering as load-bearing.
• HOOK_JQ_FIELDS_NUL can't leak stale state — reset unconditionally ahead of all three early-return paths.
• Both guards check the flag before their empty-command skip — an all-NUL command can't slip through as "no command" (now pinned by an explicit ordering-boundary test pair in both suites, confirmed in this round's diff).
• Vendored-copy drift — all 16 copies share the identical blob SHA with lib/hook-utils.sh.
• No secrets/credential handling — the BLOCKED: stderr messages are static text, never echo payload content.
  · branch fix/hook-utils-nul-fail-open

[claude]

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

---

Security review complete

• Gather context (prior review rounds already on this thread)
• Diff current HEAD (854b649) against last-reviewed commit (4aff456)
• Re-verify no new security-relevant surface changed
• Post findings

Scope: all 55 files this PR changes relative to origin/main — lib/hook-utils.sh (source of truth) + its test, the 16 vendored hooks/hook-utils.sh copies + plugin.json bumps + CHANGELOG.md entries, plugins/guardrails/README.md, and the two guards (block-no-verify.sh, block-dangerous-git.sh) + their test files. No .github/ workflow, settings, or permissions file is touched by this diff, so unpinned-action risk, trigger patterns, permissions, and template injection stay out of scope for this lane (deferred to zizmor).

No security issues found

This is a synchronize trigger. git diff origin/main...HEAD --stat confirms the PR's own file set is unchanged — still exactly the same 55 files as the last fully-reviewed round, 2114 insertions(+), 496 deletions(-).

The tip commit, 854b649, is Merge remote-tracking branch 'origin/main' into HEAD. git diff 4aff456..HEAD -- <every security-relevant file in this PR's scope> (lib/hook-utils.sh, lib/hook-utils.test.sh, both guards + their tests, the guardrails vendored copy, plugin.json, CHANGELOG.md) returns empty — none of it changed. Everything the merge actually brought in (plugins/songwriting/**, the new plugins/mutation-testing/** plugin, plugins/tdd/**, plugins/verification/**, scripts/check-changelog-parity.sh, .github/workflows/ci.yml, .claude-plugin/marketplace.json, etc. — 51 files, +2724/-222) landed from origin/main and is not this PR's own contribution; none of it is in this PR's diff against origin/main.

Re-affirming what prior rounds on this thread already established and which still holds at 854b649:

• jq program injection — filter args to hook::jq_fields are hardcoded literals only, never payload-derived.
• Fail-open regression via the cardinality check — $# + 1 correctly accounts for the leading flag record; the flag is computed from the untruncated/unstripped array strictly before the strip, and a code comment marks that ordering as load-bearing.
• HOOK_JQ_FIELDS_NUL can't leak stale state — reset unconditionally ahead of all three early-return paths.
• Both guards check the flag before their empty-command skip — an all-NUL command can't slip through as "no command" (pinned by explicit ordering-boundary tests in both suites).
• Vendored-copy drift — all 16 copies share the identical blob SHA with lib/hook-utils.sh.
• No secrets/credential handling — the BLOCKED: stderr messages are static text, never echo payload content.
  · branch fix/hook-utils-nul-fail-open