fix(guardrails): stop blocking read-only python open() and cat > /dev/null - #2007
Merged
Conversation
…t/PR advisory `block-hook-bypass` refused a READ-ONLY inline `open()` and a `cat > /dev/null` discard; `cli-flag-verify` reported npm's global config flags as hallucinated; and `flag-commit-pr-skill-bypass` was registered at `timeout: 10` against a 12-19s runtime, so the advisory never completed on any firing. - `_py_write` no longer treats a bare `open(` as a write indicator. `open(f,'w')` and `open(f)` differ only by an argument, so `open(` now counts only alongside an argument-position write-mode literal (a quoted mode token carrying w/a/x/+, after a comma or `mode=`). The test is CO-OCCURRENCE, not position: bash ERE has no lazy quantifier, so a positional `open\([^)]*'w'` would fail open on `open(os.path.join(a,b),'w')`. Accepted residual, fail-closed: a read-only `open()` beside an unrelated argument-position mode-shaped literal still blocks. Four PowerShell fixtures that used a bare `open(` as their stand-in write indicator are respelled to `open(f,'w')` so they keep testing the mention-over-block contract. - The `cat >` lane gets the `/dev/null` discard exemption the echo/printf lane already had, segment-scoped so `cat > /dev/null && cat > real.txt` still blocks. Both lanes now share one segment splitter (`normalize_segments`). - `npm` joins `git` and `npx` as an excluded `cli-flag-verify` binary: every npm config key is a flag on every subcommand, and none appears in `npm <subcmd> --help` or `npm --help`. A top-level `--help` fallback was measured against the repro and rejected -- it would not have closed it. - `flag-commit-pr-skill-bypass` moves to `timeout: 60`, its five siblings' value on the same matcher. Per the hooks reference (https://code.claude.com/docs/en/hooks, fetched 2026-08-08) the documented `command` default is 600 and hook entries merge additively across settings levels, so no consumer could raise the plugin's 10 locally. - `block-dangerous-git` and `block-no-verify` adopt `hook::jq_fields`, taking both payload fields from one jq process instead of two. guardrails 0.19.2 -> 0.19.3. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
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 encountered an error after 0s —— View job I'll analyze this and get back to you. |
|
Claude encountered an error after 0s —— View job I'll analyze this and get back to you. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 02c23adc21
ℹ️ 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 finished @kyle-sexton's task in 6m 4s —— View job Security review — PR #2007
|
… /dev/null Review caught that the new `cat > /dev/null` exemption was a bypass of the guard it was added to. It fired on any `/dev/null` redirect anywhere in the segment, but bash applies redirections LEFT TO RIGHT, so `cat > /dev/null > real.txt` writes to real.txt and `cat >/dev/null 1>real.txt` does too. Write the discard first and the real file second, inside one segment, and the whole guard was one token away from being switched off. The exemption now resolves the segment's EFFECTIVE stdout destination — the last stdout redirect's target — and exempts only when that is /dev/null. The echo/printf lane carried the identical order-blind presence test, so it moves to the same helper; that half was pre-existing rather than introduced here. The first version of this fix excluded any redirect preceded by a digit, which also dropped `1>` — and `1>file` is stdout exactly as `>file` is. The new `cat >/dev/null 1>real.txt` case caught it. The scan now admits the explicit fd-1 spelling while still excluding other fds (`2>`, `21>`), the combined form (`&>`), and fd dups (`>&1`, whose target class excludes `&`). Seven regression cases pin both directions, including the inverse order (`cat > real.txt > /dev/null`), which is a genuine discard and must stay allowed. The helper sets a global rather than echoing: it runs per segment on every Bash call, and a command substitution would add a fork to each one. Also updates README's documented default binary set, which still advertised `npm` after this PR removed it, and states why it is excluded and how to re-add it via `cli_flag_verify_bins`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Claude finished @kyle-sexton's task in 5m 17s —— View job Code review — PR #2007
Scope note: this repo has a Tooling note: Finding (posted inline on
|
|
Claude finished @kyle-sexton's task in 4m 59s —— View job Security review — PR #2007 (re-run at
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
Claude has reviewed this PR 2 times. The lane skips further automatic reviews after 5; deleting this comment resets the count. |
…code The bullet was written for the first commit and never revisited after the fix that followed it, so it named `_echo_devnull` — which no longer exists — and omitted the left-to-right effective-stdout-target resolution entirely. That omission is the part that matters: the first commit's exemption was a presence test, and a presence test is a one-token bypass of the whole guard (`cat > /dev/null > real.txt` writes to real.txt). An entry describing the superseded behavior would have left the record claiming the weaker check shipped. It now states the effective-target rule, that `_echo_devnull` was the same order-blind test on the echo/printf lane and was pre-existing rather than introduced here, which fd spellings the scan admits and excludes, and why the helper sets a global instead of echoing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Claude finished @kyle-sexton's task in 1m 20s —— View job Code review — PR #2007 (re-run at
|
|
Claude finished @kyle-sexton's task in 1m 39s —— View job Security review — PR #2007 (re-run at
|
kyle-sexton
added a commit
that referenced
this pull request
Aug 9, 2026
…oth lanes (#2008) No linked issue Found while reviewing #2007 after it merged, not reported by a consumer. Review of this PR then turned up a second, unrelated defect in the same function — see the third section. ## `block-hook-bypass` never caught the explicit stdout redirect `cat 1>real.txt` and `echo x 1>real.txt` write files exactly as `cat >real.txt` and `echo x >real.txt` do. Neither was caught. Verified against the shipped hook on `main` before changing anything: ```text cat 1>real.txt -> exit 0 # write, allowed echo x 1>real.txt -> exit 0 # write, allowed cat > real.txt -> exit 2 # control, blocked ``` One character defeated both lanes of the guard. **Cause.** Both detection patterns only ever admitted the bare `>`: - `_cat_redir` required `cat[[:space:]]*>` - `_echo_file_out` excluded any digit-prefixed operator, in order to keep `2>` out — and that exclusion took the legitimate fd-1 spelling with it Both now admit an optional `1` before the operator. Other fds stay out: `_echo_file_out` still rejects a digit-prefixed operator apart from that `1`, so `2>` and `21>` do not match; and in the `cat` lane the `[[:space:]]*1?>` sequence cannot match `cat 2>err`. The fd-1 discard (`cat 1>/dev/null`) is still exempt, because the exemption reads the *effective stdout target* rather than the operator spelling. ## Pre-existing — but #2007 is why it surfaced This is not a 0.19.3 regression. 0.19.3 widened the same `1?>` spelling on the **exemption** side (`set_last_stdout_target`, so `cat >/dev/null 1>real.txt` could not sneak a write past the discard check) and did not touch detection. The two sides then disagreed about what a stdout redirect looks like: the exemption understood a spelling the detection never looked for. That asymmetry is what made the hole visible — worth noting as a review heuristic, since the same shape (widen one side of a guard, leave the other) will recur. ## Second defect, found by this PR's review: `echo x >&2` was blocked on bash 5.2+ A reviewer argued the `\x01`/`\x02` sentinel exclusions in `_redir_scan` were dead code. Testing that claim falsified it and exposed a live false positive instead. Removing the exclusions as suggested produced `PASS=252 FAIL=3` — failing on exactly `cat 1>&2`, `cat 1>&-`, and `cat >&2`. Dumping the stored segment showed why: ```text DBG_SEG=$'cat 1>\0012' # the sentinel survives normalization ``` **Cause: a substitution replacement that stopped meaning what it said.** `normalize_segments` protects a redirect `&` with a `\x01` sentinel so an fd dup is not split as a control operator, then restores it with `${normalized//"$soh"/&}`. Since **bash 5.2**, an unquoted `&` in a substitution *replacement* expands to the text the pattern just matched — the `sed` rule — so that line restored the sentinel to itself: ```bash soh=$'\x01'; n="cat 1>${soh}2"; n="${n//"$soh"/&}"; printf '%q\n' "$n" # $'cat 1>\0012' # bash 5.3.15 — no-op ``` A silent no-op on bash ≥ 5.2, still correct on older bash: the guard quietly behaved differently depending on the interpreter running it. The surviving `\x01` then matched `_echo_file_out`'s target class, and the producer lane — unlike the `cat` lane — has no emptiness skip, so an empty effective target fell straight through to a block. Verified against a real `main` worktree: | command | `main` | this PR | |---|---|---| | `echo x >&2` | **2 (blocked)** | 0 | | `printf x >&2` | **2 (blocked)** | 0 | | `echo x >&2 > real.txt` | 2 | 2 | Writing to stderr is not a file write. The last row confirms the fix opens no hole: bash applies redirections left to right, so the file is still the effective stdout target. **Fix.** Restore with `\&`. Two defenses are kept against the same class of regression: the sentinel exclusions in `_redir_scan` stay (a dup is rejected whichever byte reaches the scan), and `producer_redirect_bypass` gains the `cat` lane's emptiness skip — whose absence is what turned an empty effective target into a block. Both are unreachable while `_echo_file_out` and `_redir_scan` agree, which is precisely the equivalence that failed silently here. ## Verification | Check | Result | |---|---| | `block-hook-bypass.test.sh` | **PASS=260 FAIL=0** (255 before; 5 new cases) | | `shellcheck` | clean | | `check-changelog-parity.sh --check-bump origin/main` | pass | | `markdownlint-cli2` | 0 issues | New cases pin every direction. Write forms (`cat 1>`, `cat 1>>`, `cat 1> ` spaced, `echo x 1>`, `printf x 1>`) block; fd-1 discards (`cat 1>/dev/null`, `echo x 1>/dev/null`) stay allowed; other-fd forms (`cat 2>err.log`, `cat 21>err.log`) are not swept in; and the new stderr-dup cases (`echo x >&2`, `printf x >&2`, `echo x 1>&2`, `echo x >&-`) are allowed while `echo x >&2 > real.txt` still blocks. `check-orphaned-fixtures.sh` was not run locally; it exceeds a 300s timeout on this machine. CI covers it. `origin/main` was merged in to resolve a version collision — #2047 took 0.19.4 while this sat open. `guardrails` 0.19.4 → **0.19.5**. ## Related - #2007 — widened `1?>` on the exemption side; the asymmetry it created is what exposed the first defect. - #2047 — took 0.19.4 on `main`; this PR rebases onto it as 0.19.5. - Inbox item `20260730-182801-guardrails-hook-false-positives-and-ungated-commit-pr-hook` — the consumer report behind #2007. Neither defect here is part of it; neither was reported. --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This was referenced Aug 10, 2026
kyle-sexton
added a commit
that referenced
this pull request
Aug 10, 2026
…ng guards (#2123) No linked issue ## Summary Two BLOCKING PreToolUse guards — `secret-pattern-detection` and `hardcoded-path-check` — returned **no verdict at all** for a Write/Edit payload of **65536–65663 bytes inclusive**. Not slow: deadlocked. A live-shape AWS access-key id inside such a payload produced nothing; the same token in a small payload exits 2. Both hooks are registered at `timeout: 60`, so the harness cancels the guard and the verdict is lost — a fail-open reachable by any agent that controls the size of what it writes. Same class as #1587, which fixed `hook-utils.sh`'s JSON path and stopped at that one call site. This PR sweeps the class instead of patching only the two reported files. ## The defect Bash delivers a here-string by filling a pipe **itself**, before the reader is `exec`'d, and it appends a newline. A payload in that band puts the write 1–128 bytes past the 65536-byte pipe capacity and bash blocks forever. At ≥129 bytes over, bash spills to a temp file and it works again — so the window is **closed on both sides**, which is exactly why no ordinary size ever caught it. Reproduced standalone, outside the plugin (`bash 5.3.15(1)`, MINGW64): ``` C=$(head -c 65600 /dev/zero | tr '\0' b) timeout 15 bash -c 'grep -qE "Users" <<<"$1"' _ "$C"; echo $? # 124 (hung) # 65535 -> 1 65536 -> 124 65600 -> 124 65663 -> 124 65664 -> 1 ``` The `while … done <<<"$var"` shape hangs identically (rc 124 at 65536 / 65600 / 65663), which is what pulled the command-scanning guards into scope. ## Boundary measurements, through the real hooks Payload piped to the hook on stdin — never `<<<`, which would hang the measurement itself. `rc 124` = killed at the bound, i.e. never answered. All numbers from this Windows host (Git Bash + Defender), under concurrent agent load. `secret-pattern-detection`, exact content bytes, AWS access-key id at the tail: | content bytes | case | BEFORE rc | BEFORE secs | AFTER rc | AFTER secs | | --- | --- | --- | --- | --- | --- | | 65535 | clean | 0 | 10 | 0 | 19 | | 65535 | AWS key | 2 | 43 | 2 | 63 | | 65536 | clean | **124** | 202 (bound) | **0** | 14 | | 65536 | AWS key | **124** | 204 (bound) | **2** | 51 | | 65600 | clean | **124** | 151 (bound) | **0** | 12 | | 65600 | AWS key | **124** | 151 (bound) | **2** | 67 | | 65663 | clean | **124** | 156 (bound) | **0** | 24 | | 65663 | AWS key | **124** | 155 (bound) | **2** | 41 | | 65664 | clean | 0 | 20 | 0 | 19 | | 65664 | AWS key | 2 | 44 | 2 | 59 | The BEFORE hangs were taken at a **200-second** bound first, then 150 — well past the legitimate slow path (41–67 s) — so these are deadlocks, not slowness. `hardcoded-path-check`, clean payloads. The pre-filter gate runs on **every** write, so the clean column is the stronger claim: nothing in the window got a verdict, violating or not. | content bytes | BEFORE rc | BEFORE secs | AFTER rc | | --- | --- | --- | --- | | 65535 | 0 | 34 | 0 | | 65536 | **124** | 152 (bound) | **0** | | 65600 | **124** | 151 (bound) | **0** | | 65663 | **124** | 207 (bound) | **0** | | 65664 | 0 | 44 | 0 | ## Why not `printf … | grep -q` — the pipefail inversion is real `hardcoded-path-patterns.sh:73-75` carried a comment *justifying* the here-string, and the justification was half right. Measured under `set -o pipefail`, with the match on line 1 so `grep -q` can exit before the writer finishes (a single-line payload never reproduces this — grep must read it all, so `printf` never gets SIGPIPE'd): | shape | `set +o pipefail` | `set -o pipefail` | | --- | --- | --- | | `grep -qE pat <<<"$C"` | 0 | 0 — but deadlocks in the window | | `printf … \| grep -qE pat` | 0 | **141** ← inversion | | `printf … \| grep -E pat >/dev/null` | 0 | 0 | | `grep -qE pat < <(printf …)` | 0 | **0** ← chosen | Both hooks run under `set -uo pipefail` (`set -e` is off), so the inversion is live — and it is worse than a wrong status, because both gate sites are written `if ! grep -q …`: > `grep -q` matches → exits 0 → SIGPIPEs `printf` → `pipefail` reports **141** → `if ! 141` is > **true** → the gate early-returns **clean**. A fail-open on the very payload that contained the > secret. **Chosen idiom: process substitution.** It keeps the writer *outside* the pipeline, so `pipefail` can never see its SIGPIPE, while preserving the `-q` early exit the gate exists for — and it never blocks. Verified at all six sizes in both the match and no-match directions. Two shapes, chosen by whether the reader drains its input. The pattern lib previously **contradicted `hook-utils.sh` inside the same plugin** — it told readers to PREFER a here-string over `printf | grep`, while `hook-utils.sh` told them a whole payload must never go through `<<<`. The lib now states the same rule and cites it: - reader drains (`jq`, `grep` without `-q`) → `printf … | reader` - reader may exit early (`grep -q`) → `reader < <(printf …)` For `while … done` loops the substitution is `< <(printf '%s\n' …)`. The `\n` is mandatory and makes it byte-identical to the here-string it replaces (`<<<` appends a newline unconditionally), so no loop can drop its final line. ## Repo-wide sweep of `<<<` — every site, with a verdict 876 occurrences total; 329 outside `*.test.sh`. Fix criterion: **the string can reach 65536–65663 bytes from an agent- or attacker-controlled source, AND a hang loses a security verdict.** ### Fixed (18 sites) | site | input | why | | --- | --- | --- | | `guardrails/lib/path-detection/hardcoded-path-patterns.sh:76,83` | whole Write/Edit payload | reported; blocking | | `guardrails/hooks/secret-pattern-detection.sh:158,207` | whole Write/Edit payload | reported; blocking | | `guardrails/hooks/hardcoded-path-check.sh:219` | `$VIOLATIONS` | **not in the original report.** `$VIOLATIONS` embeds each MATCHED LINE verbatim, and the lib's `head -3` bounds the line COUNT, not bytes — so one 65KB minified line carrying a hardcoded path makes it payload-sized. It deadlocks on the **blocked** path, after the stderr message but before `exit 2`. Measured separately below | | `guardrails/hooks/block-convention-violation.sh:132,158` | `$cmd` (Bash/PowerShell command) | blocking guard; loop shape hangs identically | | `guardrails/hooks/block-hook-bypass.sh:248,497,566` | `$cmd`, `$NORMALIZED_SEGMENTS` (derived from `$COMMAND`) | blocking guard | | `guardrails/hooks/flag-commit-pr-skill-bypass.sh:229` | `$cmd` | same command stripper | | `guardrails/lib/powershell/ps-command.sh:143,671` | `$cmd`, `$norm` | shared lib behind the blocking PowerShell guards | | `guardrails/hooks/workflow-resilience-check.sh:72,79` | `$SCRIPT` (inline Workflow script, or a `scriptPath` file read) | advisory, so no verdict is lost — but a hang wedges the Workflow call until the harness cancels | | `source-control/hooks/pr-body-linkage-gate.sh:194` | `$text` (PR body) | **blocking** gate. GitHub caps a PR body at exactly **65536 characters** — the documented maximum lands inside the hang window | | `source-control/hooks/pr-linkage-validator.sh:76,113` | `$body` | same input, same cap | `source-control` is deliberately in scope rather than left as a half-fix; it costs the second plugin bump in this PR. ### Judged safe — no fix, with reason | site | reason | | --- | --- | | `guardrails/lib/verification/verify-cli-flag.sh:159,161` (`$HELP_OUTPUT`) | local CLI `--help` output; not attacker-influenced. A **latent hang**, not a security hole — noted, not fixed | | `guardrails/hooks/cli-flag-verify.sh:188,337` (`read -ra` on `$seg`/`$chainstr`) | advisory PostToolUse; a single ≥64KB fragment is possible in principle, but no verdict is at stake. Latent hang, noted | | `guardrails/hooks/flag-commit-pr-skill-bypass.sh:196` (`$keys`), `skill-reference-verify.sh:160` (`$declared`) | jq-derived plugin/settings key names; bounded by manifest size, not by any payload | | `guardrails/hooks/block-no-verify.sh:102` | a `userConfig` option value (administrator-provided scalar) | | `biome-format:211,237,251`, `ruff-format:250,275` (`$OUTPUT`) | formatter/linter output. Same mechanism, **different consequence class** — no security verdict at stake, only a wedged formatter. Recommended follow-up, kept out to keep this PR reviewable | | `claude-ops-paths.sh:19,68`, `worktree-create.sh:373,412`, `babysit-readiness-gate.sh:286,293`, `check-plugin-manifest-presence.sh:75` | `IFS=… read -ra` splits of a path or a short CSV; cannot approach 64KB | | `source-control/skills/pull-request/scripts/fetch-annotations.sh:177` (`$FILTERED`) | jq-filtered CI annotation records in a skill script, not a hook gate; no blocking verdict | | `claude-config/skills/audit/scripts/*`, `claude-ops/skills/lanes/scripts/*`, `work-items` adapters | skill-invoked scripts over jq-bounded JSON, not a hook payload; no blocking verdict | | ~547 sites in `*.test.sh` | fixed small fixtures authored in-repo; reported as one class | `lib/hook-utils.sh` itself was already clean (#1587), and the stdin→`CONTENT` path in both hooks is `printf '%s' "$INPUT" | jq -r` throughout — verified, because otherwise the payload would have hung upstream and this fix would have changed nothing. ### The `$VIOLATIONS` site, measured A BEFORE run cannot reach line 219 through the hook (the gate deadlocks first), so it is measured directly, and its reachability is confirmed on the patched hook: ```text $VIOLATIONS as hpp::scan_text builds it: the label, then "<lineno>:<line>" with the matched line embedded VERBATIM, then the block terminator. bytes = 65628 PRE-FIX grep -E 'detected:$' <<<"$VIOLATIONS" -> rc=124 (deadlock, 60s bound) POST-FIX grep -E 'detected:$' < <(printf '%s' …) -> rc=0 (4s under load) ``` ## Tests Neither suite had a **single** payload-size case before this (`grep -n '65536\|head -c'` returned nothing). Added to **both** `secret-pattern-detection.test.sh` and `hardcoded-path-check.test.sh`: - clean payloads at **65535, 65536, 65600, 65663, 65664** — the window and both shoulders; - a **real detectable secret / hardcoded path inside the window** (65536 and 65600) that must exit 2; - the empty-content case, pinning that `printf '%s' ""` (zero bytes) matches the old `<<<""` (one empty line) in outcome; - a stderr assertion that the `grep -q` early-exit leaks no `Broken pipe` noise onto the hook's user-facing channel. Every case is bounded by `timeout 150` so a regression **fails loudly instead of hanging CI**, and asserts the **exact** expected code — `124` is reported as its own named failure. A "non-zero means blocked" assertion would have accepted the hang and would not have caught this defect. The payload is piped, never fed to the hook with `<<<`, which would hang the test itself at exactly these sizes. In the path suite the detect payload separates the filler from the home path with a **space**: the slash-rooted macOS/Linux bodies require a left boundary, so a path glued straight onto filler bytes legitimately does not match — and the "must block" case would have passed for the wrong reason. That was caught by a measurement script that omitted the space and returned 0 where 2 was expected. ## Known follow-up (not fixed here) On this Windows host the **patched** detect path measured **41–67 s** against the `timeout: 60` registration in `hooks.json`. So on Git Bash under Defender a large payload can still lose its verdict to the harness — now by slowness rather than deadlock. The cost is process spawns, not matching: on a hit, itemization runs 12 patterns × 5 processes. That is a separate defect with a separate fix (batch the itemization), deliberately out of scope here, and stated rather than left for the next auditor to "discover" as a half-fix. ## Verification - `shellcheck -x` clean on all 17 changed files. - `shfmt -d` clean on every changed file. The pre-existing drift in the two `.test.sh` files is unchanged by this PR — confirmed byte-identical at `origin/main` — and does not touch the added block. - `scripts/check-shell-portability.sh --paths …` — no unexcused GNU-only constructs in 27 shell files. - `scripts/check-changelog-parity.sh --check-bump origin/main` — passes. - `scripts/sync-hook-utils.sh --check` and `--check-bump origin/main` — pass. `lib/hook-utils.sh` is left **byte-identical to `main`** on purpose. Its guidance was already correct (only its upper bound was imprecise), and the sync gate requires a version bump plus a changelog entry for **all fourteen** other plugins carrying the shared lib in exchange for a comment-only edit — churn that would bury a security fix. The contradiction is resolved on the guardrails side, which is where the wrong advice lived. - `markdownlint-cli2` — 0 issues. - `secret-pattern-detection.test.sh` — **PASS=52 FAIL=0** - `hardcoded-path-check.test.sh` — **PASS=94 FAIL=0** **Version bumps are patch, deliberately.** A reviewer may reach for minor on "payloads that were allowed are now blocked". Nothing legitimate becomes refused that the guard did not already intend to refuse — the fix restores the documented contract rather than widening it, which matches this repo's practice (`source-control` 0.49.3 shipped a behavior-changing `exec-bit-check` fix at patch level; the `guardrails` 0.21.0 minor was called out specifically for an *acceptance* change that could refuse previously-allowed legitimate work). ## Rider The README hook table listed all six guards registered under the `Bash|PowerShell` matcher as `PreToolUse · Bash`; no row named PowerShell at all. Verified row-by-row against `hooks.json` (6 rows, 6 hooks, exact match) and corrected. ## Related - Refs #1587 — fixed this same deadlock class in `hook-utils.sh`'s JSON path and stopped at that one call site; this PR sweeps the rest. - Refs #2007 — prior `guardrails` fix in the same review stream. --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
kyle-sexton
added a commit
that referenced
this pull request
Aug 10, 2026
…or three (#2120) No linked issue `hook::jq_fields` landed in #1979 and got its first two adopters in #2007 (`block-dangerous-git`, `block-no-verify`). The other **ten** guardrails hooks were still parsing their PreToolUse/PostToolUse payload with a separate `printf '%s' "$INPUT" | jq -r … | tr -d '\r'` pipeline **per field**, over the same already-buffered stdin envelope. This converts all ten. ## Survey — what was still forking per field Counting only `jq` **execs against the buffered payload**. `jq -n` envelope builders, `jq -R | jq -s` finding serializers, and jq reading a file from disk are out of scope and untouched. | hook | jq execs on payload, `main` | after | fields | | --- | --- | --- | --- | | `block-noncanonical-commit` | 3 | 1 | `command`, `cwd`, `tool_name` | | `block-convention-violation` | 3 | 1 | `tool_name`, `command`, `cwd` | | `hardcoded-path-check` | 3 | 1 | `tool_name`, `file_path`, `content`/`new_string`/`new_source` | | `secret-pattern-detection` | 3 | 1 | same as above | | `skill-reference-verify` | 3 (Edit) / 2 (Write) | 1 | `tool_name`, `new_string`, `replace_all` / `content` | | `stale-path-verify` | 3 (Edit) / 2 (Write) | 1 | same as above | | `block-hook-bypass` | 2 | 1 | `command`, `tool_name` | | `flag-commit-pr-skill-bypass` | 2 | 1 | `command`, `tool_name` | | `cli-flag-verify` | 2 | 1 | `tool_name`, `new_string`/`content` | | `workflow-resilience-check` | 2 | 1 | `script`, `scriptPath` | | `block-dangerous-git` | 1 | 1 | already converted by #2007 | | `block-no-verify` | 1 | 1 | already converted by #2007 | Collateral, not claimed as the headline: each old line is three process creations (`$( )` subshell + `jq` + `tr`), so a 3-field hook went 9 → 3 and a 2-field hook 6 → 3 — the `tr -d '\r'` per field disappears too, because `hook::jq_fields` strips CR shell-side. ### Deliberately NOT converted **`hook::read_file_path`** — `cli-flag-verify`, `skill-reference-verify` and `stale-path-verify` each still pay one jq exec there. Folding `file_path` into the batched call would mean either duplicating or restructuring that helper's existence + project-membership validation, and it lives in the synced shared lib (`lib/hook-utils.sh` → 13 plugin copies + the CI drift check), so the blast radius reaches every plugin for one exec. Left alone on purpose. **`flag-commit-pr-skill-bypass`'s `enabledPlugins` reads** (two jq calls at L145/L155) read a settings **file**, not the payload. Different input, not batchable here. ## How the fields were kept byte-identical Two spots would have changed behavior under a naive conversion, and both are handled: 1. **`.tool_name // "Bash"`** — the default moves to the shell side (`TOOL_NAME="${HOOK_JQ_FIELDS[n]:-Bash}"`), matching `block-dangerous-git`. 2. **`replace_all`** keeps `// false | tostring` **inside** the filter. `hook::jq_fields` wraps every filter in `// ""`, and jq's `//` treats the boolean `false` as empty — so a bare `.tool_input.replace_all` returns `""` where the old call returned `"false"`. Verified against all three input shapes (absent / `false` / `true`): ```text value=null filter=.tool_input.replace_all new=[] old=[false] value=null filter=.tool_input.replace_all // false | tostring new=[false] old=[false] value=false filter=.tool_input.replace_all new=[] old=[false] value=false filter=.tool_input.replace_all // false | tostring new=[false] old=[false] value=true filter=.tool_input.replace_all new=[true] old=[true] value=true filter=.tool_input.replace_all // false | tostring new=[true] old=[true] ``` **Failure semantics are unchanged in every hook.** `hook::jq_fields … || exit 0` lands on exactly the skip the old empty-field guard produced — each hook's statement right after its first old jq call was already `[[ -n "$X" ]] || exit 0` or a `case … *) exit 0`. `hook::require_jq` still runs first and still makes a missing jq visible once per session. **One trade stated plainly.** In `hardcoded-path-check` and `secret-pattern-detection` the per-tool content field is now serialized in the first call, i.e. BEFORE the file-path exclusions and the `git check-ignore` skip that used to precede it. On a skipped write that is one extra copy out of jq of a payload already buffered in memory, traded for one fewer process on every path. Process creation, not jq's parse, is the cost centre on the host this targets. ## Measurement **Method.** Two checkouts — arm A at `origin/main`, arm B this branch — with the arms **interleaved inside one loop**, alternating which runs first each iteration, so both arms share one load sample. Compared as **paired deltas** (`B_i − A_i`), summarized by median and quartiles. Never "50× A, then 50× B": one instrumented fork on this host has been recorded swinging 93 ms → 3234 ms, so a single sequential before/after pair proves nothing. **Machine load — every number below was taken under load, and is labelled as such.** This box runs several agents concurrently. Snapshot during the runs: `cpu_pct_avg=20.3`, `procs_total=405`, `bash_procs=18`, `free_mem_gb=31.4`. Windows 11, Git Bash (`GNU bash 5.3.15 x86_64-pc-cygwin`), `jq-1.8.2`. Load is why absolute per-arm times below run into seconds; it is also why the **medians are inflated relative to a quiet box** and the conservative statistics are the headline. **Headline, conservative — p75 (least-favourable quartile) of the paired deltas:** | conversion shape | p75 | median | min-of-arms floor | NEW faster in | | --- | --- | --- | --- | --- | | 3 fields → 1 (run 1, N=100) | **-404 ms** | -1033 ms | — | 91/100 | | 3 fields → 1 (run 2, N=100) | **-449 ms** | -991 ms | -394 ms | 95/100 | | 2 fields → 1 (N=100) | **-194 ms** | -274 ms | -192 ms | 87/100 | Run 1 was reproduced by run 2 to within 45 ms at p75 and 42 ms at the median — the point the task brief makes about a "PASS=154 FAIL=0" claim from a single run that did not reproduce. Run 1's raw samples were not retained to a file (its summary line is quoted above); **runs 2 and the 2-field run have every sample below**, and either alone carries the claim. The p75 and the independently-computed floor (fastest observed A minus fastest observed B, i.e. the least-contended sample of each arm) agree to within 10 ms in both shapes. Two conservative estimators converging is the strongest claim here; the medians are the same effect amplified by contention. **End-to-end, whole-hook** — `block-noncanonical-commit.sh` invoked as a process, N=60 interleaved: **median paired delta -687 ms**, range -13039 ms to +11774 ms. Reported deliberately even though it is noisier and *smaller* than the isolated 3-field median: the parse block cannot recover more than the whole hook does, and omitting the weaker own-number is what makes a stronger one look selected. **Against the prior model.** A previous session's model predicted ~280 ms recovered and the handoff recorded "the measured-versus-model gap says expect LESS." Stated plainly: the conservative 2-field number (**-194 ms**) is **under** that model, and the conservative 3-field number (**-404 ms**) is **over** it. The model was a single figure for a range of shapes. Every sample is in the collapsed sections below. ## Behavior verification ### Payload-level differential vs `origin/main` — 62/62 identical Issue #1403 records that the previous extraction attempt (#1385) regressed on **multi-line command values** — four suites failed, all on multi-line payloads. That is the exact risk class for this change, so it is tested directly: the same payload fed to the `origin/main` copy and the converted copy of each hook, requiring **identical exit code, identical stdout and identical stderr**. Cases: plain command, backslash-newline continuation (`git commit --no\<newline>verify`), multi-line `-m` body, escaped quotes, embedded tab, PowerShell here-string, stdout-redirect write, `gh pr create`, empty command; Write/Edit/NotebookEdit multi-line content, unmatched tool, empty content; `replace_all` true/false; Workflow inline-script / `scriptPath`-only / neither. **Result: `DIFFERENTIAL PASS=62 FAIL=0`.** This is a deterministic comparison of outputs, not a timing measurement, so it does not carry the reproducibility caveat the numbers above do. ### Contract suites — run STRICTLY one at a time Their wall-clock assertions corrupt under contention, so the runner is serial by construction. **Re-run after the NUL fix** (this is the authoritative set; the pre-fix tallies below it are kept for the record). `lib/hook-utils.test.sh` is included because that is where the helper and its new regression case live. ```text lib/hook-utils.test.sh rc=0 PASS=155 FAIL=0 secret-pattern-detection rc=0 PASS=44 FAIL=0 hardcoded-path-check rc=0 PASS=86 FAIL=0 skill-reference-verify rc=0 PASS=96 FAIL=0 stale-path-verify rc=0 PASS=87 FAIL=0 block-noncanonical-commit rc=0 passed: 202 failed: 0 ``` `secret-pattern-detection` and `hardcoded-path-check` each gained exactly **+2** assertions — the two added by the NUL regression case in each file. That is visible directly rather than by subtraction: under mutation (the `split | join` reverted, tests kept) the same trees report `PASS=42 FAIL=2` and `PASS=155 → 154 FAIL=1`, failing on precisely those assertions and nothing else. `skill-reference-verify` reads higher than the pre-fix table below because `main` was merged in between; no case was added to it here. **On the `block-noncanonical-commit` promise.** This description previously said that suite "was still running when this PR was opened" and that "its result will be posted as a comment." No such comment was ever posted, so it is settled here instead: the suite was re-run after the NUL fix and passes, **202/0**. Worth stating because it nearly went into this description as a false negative — that suite reports `passed: N failed: N`, not the `PASS=N FAIL=N` every other guardrails suite uses, so the first run's output filter matched nothing and the run looked like an abort. It was not; the filter was wrong. The tally above is from an unfiltered re-run. **Not re-run, and why.** The remaining guardrails suites (`block-hook-bypass`, `block-convention-violation`, `flag-commit-pr-skill-bypass`, `cli-flag-verify`, `workflow-resilience-check`, plus the two already-converted git guards) and the 15 non-guardrails plugins were not re-run for the NUL fix. The strip is a no-op for any payload without a NUL, and `grep -rln 'hook::jq_fields' plugins/*/hooks/*.sh` returns guardrails files only — the other 15 plugins carry the lib text and a version bump but have no call site. Their pre-fix tallies stand. **Pre-fix tallies** (the original `hook::jq_fields` conversion, before the NUL fix): ```text workflow-resilience-check rc=0 PASS=16 FAIL=0 35s block-convention-violation rc=0 PASS=31 FAIL=0 320s secret-pattern-detection rc=0 PASS=42 FAIL=0 314s flag-commit-pr-skill-bypass rc=0 PASS=29 FAIL=0 303s cli-flag-verify rc=0 PASS=52 FAIL=0 549s skill-reference-verify rc=0 PASS=68 FAIL=0 900s hardcoded-path-check rc=0 PASS=84 FAIL=0 1501s stale-path-verify rc=0 PASS=87 FAIL=0 1600s stale-path-verify rc=0 PASS=87 FAIL=0 1571s block-hook-bypass rc=0 PASS=260 FAIL=0 2278s ``` One caveat from that run, stated rather than hidden: `hardcoded-path-check` and `stale-path-verify` each show **two** lines because a background runner believed killed had survived, so a second copy of each ran concurrently. Both copies of both suites returned the same tally. Contention can only produce spurious *failures* in a wall-clock assertion, never a spurious pass, so a green result under contention is the stronger reading. (The first `hardcoded-path-check` line's tally column is a `grep` artifact — its log ends `PASS=84 FAIL=0`.) <details><summary>Every sample — isolated parse block, 3 fields to 1 (N=100)</summary> ```text payload=payload-ls.json iterations=100 fields=3 sample old_ms new_ms delta_ms 1 704 303 -401 2 1247 262 -985 3 648 223 -425 4 616 265 -351 5 650 252 -398 6 640 758 118 7 653 253 -400 8 627 223 -404 9 633 237 -396 10 1506 423 -1083 11 1664 481 -1183 12 1739 462 -1277 13 2018 1015 -1003 14 1852 796 -1056 15 838 958 120 16 1212 265 -947 17 704 260 -444 18 673 263 -410 19 722 258 -464 20 849 331 -518 21 1429 357 -1072 22 891 869 -22 23 1345 367 -978 24 1318 322 -996 25 663 807 144 26 1192 324 -868 27 1308 887 -421 28 1827 816 -1011 29 3450 782 -2668 30 6323 1474 -4849 31 8060 4150 -3910 32 7464 1999 -5465 33 3327 1541 -1786 34 1791 337 -1454 35 4003 869 -3134 36 11638 1056 -10582 37 4819 2220 -2599 38 5149 885 -4264 39 1270 858 -412 40 1215 799 -416 41 7450 1903 -5547 42 7951 1636 -6315 43 4620 4181 -439 44 4524 1083 -3441 45 2888 1642 -1246 46 2639 824 -1815 47 1870 276 -1594 48 1249 831 -418 49 1793 793 -1000 50 2296 251 -2045 51 2730 757 -1973 52 2241 1294 -947 53 5852 2494 -3358 54 5935 2120 -3815 55 3870 1551 -2319 56 5200 820 -4380 57 1205 265 -940 58 2331 242 -2089 59 4141 1333 -2808 60 1943 305 -1638 61 1910 328 -1582 62 1288 280 -1008 63 1252 266 -986 64 1202 271 -931 65 2896 253 -2643 66 4351 935 -3416 67 6699 894 -5805 68 2085 843 -1242 69 1278 291 -987 70 1218 251 -967 71 1220 258 -962 72 636 238 -398 73 1188 249 -939 74 674 749 75 75 617 223 -394 76 1170 253 -917 77 1751 222 -1529 78 4921 820 -4101 79 5530 796 -4734 80 1895 808 -1087 81 1953 838 -1115 82 1794 251 -1543 83 1193 295 -898 84 1723 809 -914 85 1713 269 -1444 86 1194 255 -939 87 652 769 117 88 1158 808 -350 89 4942 1893 -3049 90 4702 3793 -909 91 1890 1441 -449 92 1167 257 -910 93 1178 269 -909 94 649 269 -380 95 704 249 -455 96 1194 284 -910 97 1188 759 -429 98 1139 250 -889 99 2270 256 -2014 100 7879 1671 -6208 median_paired_delta_ms=-991 p25=-2014 p75=-449 (negative = NEW is faster) iterations_where_NEW_faster=95/100 ``` </details> <details><summary>Every sample — isolated parse block, 2 fields to 1 (N=100)</summary> ```text payload=payload-ls.json iterations=100 fields=2 sample old_ms new_ms delta_ms 1 2995 2056 -939 2 1664 268 -1396 3 1030 300 -730 4 491 285 -206 5 963 266 -697 6 484 793 309 7 427 253 -174 8 494 251 -243 9 432 741 309 10 414 249 -165 11 448 252 -196 12 434 230 -204 13 472 222 -250 14 949 225 -724 15 436 235 -201 16 441 235 -206 17 444 234 -210 18 444 250 -194 19 432 239 -193 20 446 234 -212 21 1021 251 -770 22 447 253 -194 23 485 256 -229 24 459 272 -187 25 1224 788 -436 26 1176 436 -740 27 6119 984 -5135 28 2840 1012 -1828 29 1183 1052 -131 30 552 287 -265 31 531 811 280 32 523 267 -256 33 479 808 329 34 463 266 -197 35 1016 269 -747 36 475 250 -225 37 477 269 -208 38 997 256 -741 39 469 259 -210 40 508 267 -241 41 1178 838 -340 42 1726 877 -849 43 5537 3620 -1917 44 4843 1005 -3838 45 2414 1547 -867 46 1708 271 -1437 47 1003 312 -691 48 450 281 -169 49 499 804 305 50 530 866 336 51 512 273 -239 52 1002 252 -750 53 468 295 -173 54 2921 926 -1995 55 1729 938 -791 56 1736 2165 429 57 1086 834 -252 58 484 957 473 59 1022 281 -741 60 1030 857 -173 61 2762 787 -1975 62 10295 1980 -8315 63 4781 3332 -1449 64 2104 968 -1136 65 1226 902 -324 66 1143 860 -283 67 1061 288 -773 68 1084 834 -250 69 1038 287 -751 70 466 259 -207 71 1215 815 -400 72 2466 4496 2030 73 3997 3347 -650 74 5594 1707 -3887 75 2973 2085 -888 76 1729 1418 -311 77 2138 1415 -723 78 978 252 -726 79 940 741 -199 80 973 261 -712 81 982 253 -729 82 1011 247 -764 83 456 794 338 84 1016 299 -717 85 1010 272 -738 86 1007 1324 317 87 2535 2307 -228 88 1007 266 -741 89 1013 822 -191 90 1011 831 -180 91 919 269 -650 92 956 241 -715 93 946 758 -188 94 1002 1804 802 95 2150 831 -1319 96 1590 1345 -245 97 2952 3790 838 98 6564 5253 -1311 0 [main] bash 433945 dofork: child -1 - forked process 52068 died unexpectedly, retry 0, exit code 0xC0000142, errno 11 parsebench.sh: fork: retry: Resource temporarily unavailable 99 14516 9057 -5459 100 3374 1022 -2352 median_paired_delta_ms=-274 p25=-750 p75=-194 (negative = NEW is faster) iterations_where_NEW_faster=87/100 ``` </details> <details><summary>Payload-level differential vs origin/main — all 62 cases</summary> ```text ok: block-hook-bypass plain-ls (rc=0) ok: block-hook-bypass backslash-newline-continuation (rc=0) ok: block-hook-bypass multiline-m (rc=0) ok: block-hook-bypass escaped-quotes (rc=0) ok: block-hook-bypass embedded-tab (rc=0) ok: block-hook-bypass ps-herestring (rc=0) ok: block-hook-bypass redirect-write (rc=2) ok: block-hook-bypass gh-pr-create (rc=0) ok: block-hook-bypass empty-command (rc=0) ok: block-noncanonical-commit plain-ls (rc=0) ok: block-noncanonical-commit backslash-newline-continuation (rc=0) ok: block-noncanonical-commit multiline-m (rc=2) ok: block-noncanonical-commit escaped-quotes (rc=0) ok: block-noncanonical-commit embedded-tab (rc=0) ok: block-noncanonical-commit ps-herestring (rc=2) ok: block-noncanonical-commit redirect-write (rc=0) ok: block-noncanonical-commit gh-pr-create (rc=0) ok: block-noncanonical-commit empty-command (rc=0) ok: block-convention-violation plain-ls (rc=0) ok: block-convention-violation backslash-newline-continuation (rc=0) ok: block-convention-violation multiline-m (rc=0) ok: block-convention-violation escaped-quotes (rc=0) ok: block-convention-violation embedded-tab (rc=0) ok: block-convention-violation ps-herestring (rc=0) ok: block-convention-violation redirect-write (rc=0) ok: block-convention-violation gh-pr-create (rc=0) ok: block-convention-violation empty-command (rc=0) ok: flag-commit-pr-skill-bypass plain-ls (rc=0) ok: flag-commit-pr-skill-bypass backslash-newline-continuation (rc=0) ok: flag-commit-pr-skill-bypass multiline-m (rc=0) ok: flag-commit-pr-skill-bypass escaped-quotes (rc=0) ok: flag-commit-pr-skill-bypass embedded-tab (rc=0) ok: flag-commit-pr-skill-bypass ps-herestring (rc=0) ok: flag-commit-pr-skill-bypass redirect-write (rc=0) ok: flag-commit-pr-skill-bypass gh-pr-create (rc=0) ok: flag-commit-pr-skill-bypass empty-command (rc=0) ok: hardcoded-path-check write-multiline (rc=0) ok: hardcoded-path-check edit-multiline (rc=0) ok: hardcoded-path-check notebook-multiline (rc=0) ok: hardcoded-path-check unmatched-tool (rc=0) ok: hardcoded-path-check empty-content (rc=0) ok: secret-pattern-detection write-multiline (rc=0) ok: secret-pattern-detection edit-multiline (rc=0) ok: secret-pattern-detection notebook-multiline (rc=0) ok: secret-pattern-detection unmatched-tool (rc=0) ok: secret-pattern-detection empty-content (rc=0) ok: cli-flag-verify write-multiline (rc=0) ok: cli-flag-verify edit-multiline (rc=0) ok: cli-flag-verify unmatched-tool (rc=0) ok: skill-reference-verify write-multiline (rc=0) ok: skill-reference-verify edit-multiline (rc=0) ok: skill-reference-verify unmatched-tool (rc=0) ok: stale-path-verify write-multiline (rc=0) ok: stale-path-verify edit-multiline (rc=0) ok: stale-path-verify unmatched-tool (rc=0) ok: skill-reference-verify replace_all=true (rc=0) ok: stale-path-verify replace_all=true (rc=0) ok: skill-reference-verify replace_all=false (rc=0) ok: stale-path-verify replace_all=false (rc=0) ok: workflow-resilience-check workflow-inline-multiline (rc=0) ok: workflow-resilience-check workflow-scriptpath-only (rc=0) ok: workflow-resilience-check workflow-neither (rc=0) DIFFERENTIAL PASS=62 FAIL=0 ``` </details> ## Review follow-up — the NUL fail-open (P1) Review found a **fail-open this PR introduced**, and it reproduces. `hook::jq_fields` delimits its batched fields with a NUL byte. JSON may legitimately encode a NUL inside a string, and a `Write`/`Edit`/`NotebookEdit` `content` field is exactly where one arrives — jq emitted the raw byte, the read split that value in two, the cardinality check saw one value too many, the helper returned non-zero, and the hook's `|| exit 0` skipped detection **entirely**. The per-field command substitution this PR replaced discarded the NUL and scanned the rest, so this was a regression, not a pre-existing gap. **Reproduction** — one payload, `tool_input.content` = `harmless first line` + NUL + `aws_key = AKIA…`, fed to `secret-pattern-detection.sh` at both refs: | arm | exit | note | | --- | --- | --- | | `origin/main` | **2** (blocked) | stderr also carries bash's own `warning: command substitution: ignored null byte in input` — the old path saw the NUL, dropped it, and scanned the rest | | this branch, before the fix | **0** (allowed) | secret passes unblocked | | this branch, after the fix | **2** (blocked) | | **The framing scheme, and why this one.** Each value is now NUL-stripped **inside the jq filter** (`split("<NUL>") | join("")`, the 1-arity plain-string split — not `gsub`, which would put a NUL inside an Oniguruma pattern), so the delimiter provably cannot occur in a value. The three options weighed: - **Length-prefix framing** is collision-proof but needs `read -N` (Bash 4.1+); this lib supports 3.2+ and says so. - **`@base64` / `@json` encoding** costs a decode per field shell-side — a spawn each, which undoes the whole PR — and still cannot deliver the byte, see below. - **Stripping** is not the lesser option, it is the **only representable** one: a bash variable cannot hold a NUL byte, so *no* scheme delivers one into `HOOK_JQ_FIELDS`. It is also byte-for-byte what the pre-conversion `$( )` did. Content **after** the NUL is returned and scanned exactly as before. **On "rather than failing open".** The mismatch policy is unchanged and deliberately so: `return 1` + the caller's `|| exit 0` is the documented jq-absent fail-open (`hook::require_jq` makes it visible once per session) and matches the pre-conversion empty-field guard. What changed is that the **cause** of the spurious mismatch is gone — a mid-stream jq filter error is now the only way to trip it, exactly as on `main`. **Regression cases** (all three go red on reverting the strip, green with it): - `lib/hook-utils.test.sh` — a NUL-bearing value keeps its slot and its post-NUL content. Mutated: `PASS=154 FAIL=1`. Fixed: `PASS=155 FAIL=0`. - `plugins/guardrails/hooks/secret-pattern-detection.test.sh` — a secret **after** a NUL exits 2. - `plugins/guardrails/hooks/hardcoded-path-check.test.sh` — a machine path **after** a NUL exits 2. Payloads are built with jq's `[0] | implode`, so no literal escape sequence for the byte lives in any test file's source. **Blast radius.** The fix is in the synced shared lib, so `scripts/sync-hook-utils.sh` ran and all 16 carrying plugins take a patch bump with an identical `### Fixed` entry — the mechanism #1979 used for the same file. `guardrails` additionally documents the guard-level regression and the comment softening below. ### Review nits — `replace_all` comment (both files) `skill-reference-verify.sh` and `stale-path-verify.sh` now say the `// false | tostring` is kept for parity with the pre-conversion output, **not** because a branch depends on it: every consumer tests `== "true"`, which `""` and `"false"` fail alike. Comment only; behavior unchanged. ## Checks run locally - `shellcheck -x` clean on every changed `.sh` file (the ten hooks, the shared lib, the three test files). - `shfmt -d` clean on the same set. - `npx --no-install markdownlint-cli2 plugins/guardrails/CHANGELOG.md` — 0 issues. - `bash scripts/check-changelog-parity.sh --check-bump origin/main` — passes (`guardrails` `0.22.0` → `0.22.2` plus a patch bump on all 15 other carrying plugins, each with its own new `## [<version>]` entry). - `bash scripts/sync-hook-utils.sh --check` — all 16 plugin copies match `lib/hook-utils.sh`; `--check-bump origin/main` — every carrying plugin bumped. - No `printf '%s' "$INPUT" | jq` remains anywhere under `plugins/guardrails/hooks/`. ## Related - #2007 — introduced this helper's first two adopters (`block-dangerous-git`, `block-no-verify`) and set the pattern this PR follows; the `// "Bash"` shell-side default is copied from it verbatim. - #1979 — added `hook::jq_fields` to `lib/hook-utils.sh`. The review follow-up above **does** edit that shared lib (the NUL strip), so `scripts/sync-hook-utils.sh` ran, all 16 plugin copies were re-synced, and every carrying plugin took a patch bump — the same mechanism #1979 itself used. An earlier revision of this description claimed no shared-lib edit; that is no longer true and is corrected here. - #1403 — "recover the PreToolUse spawn-reduction work from the closed #1385". This PR discharges **one** part of it: the `hook::jq_fields` conversion across the remaining guards, verified against the multi-line regression class that sank #1385 (precondition 1, differential above). It does **not** discharge: `strip_quoted_spans` in `flag-commit-pr-skill-bypass`, the deferred `git rev-parse --is-inside-work-tree` probe in `hardcoded-path-check`, committed multi-line regression **cases in the suites** (precondition 3 — the differential here is a working harness, not committed test coverage), or the unreviewed `hook_latency_report.py`. Left open. - #1414 — CI has no Windows runner, so a green `plugin-gate` carries no signal for a change whose whole point is MSYS fork-emulation cost. That is why this PR carries local Windows measurements and a payload-level differential rather than leaning on CI. ## Reproducing the numbers The harnesses are scratch scripts, not committed. To re-derive: clone `origin/main` and this branch side by side, then for each iteration time one invocation of each arm back to back (alternating order), and take the median/p75 of `B_i − A_i`. State the machine load with any number produced — on a quiet box the absolute times will be far lower than those above, and the recovery should land nearer the min-of-arms floor (-394 ms for 3 fields, -192 ms for 2) than the loaded medians. --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No linked issue
Consumer report drained from the handoff inbox:
20260730-182801-guardrails-hook-false-positives-and-ungated-commit-pr-hook. Two false positives were reproduced verbatim at HEAD before anything was changed.1a — a read-only
open()was blockedReproduced first:
python3 -c "import json; d=json.load(open('x.json'))"→ exit 2._py_writematchedopen[[:space:]]*\(with no write-mode discrimination, so every inline Pythonopen()read as a write.The discrimination boundary, stated because it is a design call and not obvious. A bare
open(is no longer a write indicator at all.open(counts as a write only when an argument-position write-mode literal occurs in the same command — a quoted token built solely from mode characters[rwaxbtu+], containing at least one ofw/a/x/+, appearing immediately after a comma or aftermode=.The test is co-occurrence, not positional, deliberately. Bash ERE has no lazy quantifier: a positional
open\([^)]*'w'stops at the first)and would fail open on a realopen(os.path.join(a,b),'w'), while a greedy.*reaches into unrelated text. This is the same mangle-resistant co-occurrence shape the PowerShell lane already uses. The argument-position requirement — rather than "a mode literal anywhere" — is what keeps common read shapes clear:json.load(open('p'))['a']has its'a'preceded by[, not by a comma.Residual, in the fail-closed direction and pinned by a test: a read-only
open()in a command that separately carries an argument-position'w'/'a'/'x'/'+'literal (e.g.print(open('f').read(), 'a')) still blocks. Accepted over the alternative.Second residual, left alone: a bare
pathlibmention is still an indicator on its own, so read-only inline Python that merely importspathlibstill blocks. That indicator is what currently carries.write_text(/.write_bytes(—\.write[[:space:]]*\(does not match.write_text(— so narrowing it requires introducing an explicit write-call set. Out of scope here, recorded in the CHANGELOG.Fixtures respelled, not relaxed — called out so it does not read as test-fitting. Four PowerShell-lane cases used a bare
open(as their stand-in write indicator to assert mention-over-block and here-string inertness. Sinceopen(is no longer an indicator, those inputs were respelled toopen(f,'w')so they keep testing their actual contract, and a new case asserts that the same mention with a READ-mode open is now allowed.1b —
cat > /dev/nullwas blockedA discard is not a write. Added
_cat_devnull, the exemption the echo/printf lane already had.It is segment-scoped, not command-scoped: a whole-command exemption would let
cat > /dev/null && cat > real.txtthrough, which is now a pinned regression floor. Thecatand echo/printf scans now share one splitter — the segmentation block was extracted fromproducer_redirect_bypassintonormalize_segments(called once, setsNORMALIZED_SEGMENTS) so the two lanes cannot drift on escaped separators or the2>&1fd-dup sentinel. Quoted spellings (cat > "/dev/null",cat > /dev/"null") fall out of the existing redirect-operand handling instrip_literals; asserted.2 —
flag-commit-pr-skill-bypasstimeout 10 → 60It was the only guardrails hook not at 60, against five
Bash|PowerShellsiblings that are. Per the hooks page fetched this session (https://code.claude.com/docs/en/hooks):Nothing pushes a
PreToolUsehook to 10 — the 10 was authored here. Same page, on why a consumer cannot work around it locally:3a —
hook::jq_fieldsadopted byblock-dangerous-gitandblock-no-verifyTwo
jqspawns collapsed to one per invocation; first adopters in the fleet. Failure semantics are unchanged: rc 1 from the helper exits 0 exactly as the old empty-COMMANDskip did, afterhook::require_jqhas already surfaced the degraded state. The cross-hookrequire-jq-notice-isolationcontract still passes over both adopters.3b —
cli-flag-verifyglobal-flag false positiveThe obvious fix was a chain-fallback to top-level
--help, and it was measured and rejected:npm --helpdoes not list--prefixeither (verify-cli-flag.sh npm --prefix→ rc 1), so it would not have closed the repro.Root cause is structural.
npm --helpstates:Every config key is a flag on every subcommand, so per-subcommand help is non-exhaustive by design, and the authoritative list (
npm config ls -l) printsprefix = "…", not--prefix, so no generic--helpparser can consume it.npmtherefore joinsgitandnpxin the exclusion, on the rationale already recorded in that file. Consumers re-add it viacli_flag_verify_bins.Verification
block-hook-bypass.test.shblock-dangerous-git.test.shblock-no-verify.test.shcli-flag-verify.test.shrequire-jq-notice-isolation.test.shshellcheck(6 changed files)shfmt -d(5 of 6)check-shell-portability.sh --paths(6 files)check-silent-skips.shmarkdownlint-cli2check-changelog-parity.sh --check-bump origin/maingit ls-files -schanged.sh100755Suites were run strictly one at a time — their wall-clock ceilings fail spuriously under concurrency. The first
block-hook-bypassrun surfaced 3 failures (the PowerShellopen(fixtures); those were respelled and the suite re-run to green twice, so the 233 tally is the shipped file.Both directions of each fix are covered: read-only
open()feedingjson.load→ exit 0,open(nested-call, 'w')→ exit 2 (the fail-open floor),cat > /dev/nullvariants → exit 0,cat > /dev/null && cat > real.txtandcat >> real.txt 2>&1→ exit 2 (the segment-scoping floors).cli-flag-verify.test.shfails bareshfmt -d— pre-existing and identical atorigin/main(theX=$(…); RC=$?one-liner idiom used throughout). The diff hunks stop around line 239 and this PR's addition starts at 264, so the added lines are shfmt-clean.mainbeing green means bareshfmt -dis evidently not the gate CI applies to.test.shhere.check-orphaned-fixtures.shwas not run locally; it exceeds a 300s timeout on this machine. CI covers it.guardrails0.19.2 → 0.19.3.Deliberately not done
workflow-resilience-check.shremains attimeout: 10—Workflowmatcher, not exercised by the report, flagged out of scope by the item itself.cat >&2still blocks. An fd-dup is not a file write — the same class as the/dev/nullFP fixed here — but it is pre-existing and not the reported defect. Flagged rather than folded in.Surfaced, not fixed
plugins/guardrails/README.md's hook table lists "PreToolUse · Bash" for six guards whose registered matcher inhooks.jsonisBash|PowerShell— a table-wide doc/manifest mismatch, not introduced here.Related
hook::jq_fields, which this PR is the first to adopt.context-guard, same defect class and same fetched doc line.20260730-182801-guardrails-hook-false-positives-and-ungated-commit-pr-hook— the consumer report.