fix(guardrails): fail closed when jq is missing in the irreversible-operation guards

[kyle-sexton]

What

hook::require_jq was command -v jq && return 0, else a once-per-session notice and exit 0 —
the whole hook skipped, the tool call proceeds. Measured against origin/main, with the jq-present
column as the discrimination control:

                       jq PRESENT   jq HIDDEN
  dangerous push       DENY         ALLOW      <-- the guard was skipped entirely
  safe command         ALLOW        ALLOW

The same two scripts fail closed on the other input they cannot parse: above MAX_COMMAND_LEN
(16384) a command is treated as obfuscation and blocked unread. Two opposite postures toward "I
cannot read this input" in one file — so an author who could not fit a dangerous command under the
ceiling could simply be on a machine without jq.

The disposition: fail CLOSED, scoped to the irreversible-operation guards

After (same harness, same machine):

  block-dangerous-git.sh        jq PRESENT   jq HIDDEN
    dangerous push              DENY         DENY
    safe command                ALLOW        DENY

  block-no-verify.sh            jq PRESENT   jq HIDDEN
    commit --no-verify          DENY         DENY
    safe command                ALLOW        DENY

  posture control (unchanged)
    block-convention-violation.sh   jq PRESENT=ALLOW  jq HIDDEN=ALLOW
    block-hook-bypass.sh            jq PRESENT=ALLOW  jq HIDDEN=ALLOW
    block-noncanonical-commit.sh    jq PRESENT=ALLOW  jq HIDDEN=ALLOW

The (safe, jq HIDDEN) = DENY cell is a real cost, not an oversight. These guards run on every
Bash/PowerShell tool call; without jq they cannot read the command at all, so they cannot tell a
dangerous one from a safe one and deny both. On a jq-less machine every matched tool call is
blocked until jq is installed or the guard's kill switch is set. That is the hard dependency
option 2 named. Option 3 (a jq-free substring pre-check) was rejected and is not implemented.

The kill switch is still a real exit: hook::check_enabled runs before the gate, so
block_dangerous_git_enabled=false bypasses the guard on a jq-less machine. Asserted.

Which hooks are in the class — mechanical, not a taste judgement

The criterion is internal consistency: a hook is fail-closed iff it already fails closed on
another unparsable-input condition. Today that is a MAX_COMMAND_LEN ceiling, and repo-wide that
is exactly two files — block-dangerous-git.sh and block-no-verify.sh, the two the issue names.
That is not a coincidence: intra-script contradiction is what #2146 reports, and a script with no
length-ceiling posture has no contradiction to resolve.

Considered and deliberately excluded, so this is not a silent scoping choice:

Hook	Why not
block-hook-bypass	exits 2 and carries the same "the only supported deliberate bypass is the kill switch" sentence — but it guards a file write (cat > path), trivially reversible, and has no length ceiling
block-noncanonical-commit	guards a message shape; a mangled message is recoverable by --amend
secret-pattern-detection, hardcoded-path-check, block-convention-violation	all guard reversible file writes
other-plugin blocking require_jq callers (context-guard/zone-gate, source-control/pr-*-gate, autonomy/lane-stop-gate)	checked repo-wide; none carries a length ceiling

Severity is a slope; "already fails closed elsewhere in the same script" is a line.
require-jq-posture.test.sh pins the membership both ways, so a hook that grows a ceiling and keeps
the fail-open gate fails, and so does a hook that adopts the blocking gate without one.

Helper design: a sibling function, not a parameter

hook::require_jq_blocking alongside the unchanged hook::require_jq.

Why not a flag on the existing function: a parameter's omitted value has to default to
something, and the safe-looking default (fail open, today's behaviour) means a guard that should
fail closed but whose flag someone forgot fails open silently — which is the exact defect this
PR fixes, reintroduced at the API. Two names make the posture greppable, make the fail-closed path
impossible to reach by accident, and make omission a visible choice.

Why not branch at the call sites: the issue's own acceptance says the reasoning belongs at the
helper, and a call-site branch leaves the decision point still unexplained. (It also would not have
avoided the 16 plugin bumps: sync-hook-utils.sh --check-bump is content-based, so even a
comment-only lib edit requires them.)

The reasoning is at the helper. One TWO POSTURES, AND WHY THERE ARE TWO block sits above both
functions — why fail-open is the default, why a minority must not be, the membership criterion, the
exclusions, the disclosed cost, and why two functions rather than a flag. The call-site comments now
say "this asserts the behaviour; that explains it", and the posture test asserts the block is
actually there.

The control that FAILS against current main

require-jq-posture.test.sh was run unchanged against origin/main's guardrails plugin
(git archive origin/main plugins/guardrails, hashes verified equal to origin/main's blobs):

FAIL: block-dangerous-git.sh defines MAX_COMMAND_LEN but does not call hook::require_jq_blocking
FAIL: block-no-verify.sh defines MAX_COMMAND_LEN but does not call hook::require_jq_blocking
FAIL: hook-utils.sh's posture block mentions 'TWO POSTURES'
FAIL: jq HIDDEN, dangerous push: DENY ... : expected 'DENY', got 'ALLOW'
FAIL: jq HIDDEN, commit --no-verify: DENY ... : expected 'DENY', got 'ALLOW'
...
PASS=21 FAIL=15
SUITE EXIT: 1

with the four cells against main reproducing the issue's table exactly (DENY/ALLOW over
ALLOW/ALLOW). Against this branch: PASS=36 FAIL=0.

How jq was hidden — and how that measurement was kept honest

A BASH_ENV file defines a command shell function that reports jq absent and forwards every
other lookup to the real builtin, plus a jq function that fails like a missing binary. PATH is
untouched. Stripping PATH directories also removes git, which these guards invoke, and a guard
that cannot find git produces the same ALLOW for an entirely unrelated reason.

The suite prints a precondition line measured inside the hidden environment and refuses to read a
verdict until it holds:

PRECONDITION (measured inside the jq-hidden environment): jq=hidden git=visible bash=visible path-to-jq=intact

path-to-jq=intact is builtin command -v jq still resolving — proof the lookup was hidden and
the tool was not removed. Every jq probe in hook-utils.sh is a command -v jq (verified: 7
sites, all of that form), so the override reaches all of them. A second check sources the real
hook-utils.sh inside the hidden environment and asserts the gate's own predicate sees no jq,
plus the inverse without the override.

The harness caught itself once. The first origin/main run archived only
plugins/guardrails/hooks, so block-dangerous-git.sh could not source its bundled PowerShell
classifier from <plugin-root>/lib and exited early — producing ALLOW in all four cells,
including jq PRESENT / dangerous push. The jq-present discrimination control is what flagged it as
a broken harness rather than a measured result. Fixed by archiving the whole plugin. This is the
failure mode the issue says invalidated three prior attempts.

Proof the fixture reached the path under test

Under jq hidden, the denial is asserted to be the new path and not some unrelated failure:

• names jq as the missing prerequisite;
• carries the documented install route https://jqlang.org/download/;
• names the guard's own kill switch (block_dangerous_git_enabled / block_no_verify_enabled);
• is not the fail-open skip notice (hook skipped for this session asserted absent).

And the advisory control asserts the inverse — that the fail-closed denial text is absent from every
advisory hook's stderr.

Blast radius

sync-hook-utils.sh --check-bump requires every carrying plugin to bump when the shared lib changes,
so all 16 are bumped with a CHANGELOG entry (the precedent set by b20e70a / #2147). The 15
non-guardrails entries state honestly that the lib gained a fail-closed sibling with no behaviour
change in that plugin — nothing outside guardrails calls it. guardrails takes a minor bump
(0.24.1 → 0.25.0), not a patch: it now denies calls it previously allowed.

origin/main moved under this branch mid-flight — #2135 landed the same 16-plugin lib bump for its
NUL-byte fix, so every version collided. Resolved by taking main's side of every CHANGELOG and
manifest wholesale and re-applying this change's entry and bump on top, so main's entries survive
intact and every plugin here is strictly above what main now carries. block-no-verify.sh conflicted
textually on the very comment both changes rewrote; both survive (see the merge commit message).

require-jq-notice-isolation.test.sh needed one adjustment: its discovery matched
hook::require_jq as a substring and so would have swept in hook::require_jq_blocking, whose
callers have no notice key to collide. The match is now anchored, and the test's own subject is
documented as not applying to the blocking gate.

Verification run on this branch

Check	Result
require-jq-posture.test.sh (new)	PASS=36 FAIL=0
same suite vs origin/main	PASS=21 FAIL=15, exit 1
require-jq-notice-isolation.test.sh	PASS=2 FAIL=0
block-no-verify.test.sh	see CI
block-dangerous-git.test.sh	see CI
scripts/sync-hook-utils.sh --check / --check-bump origin/main	pass
scripts/check-changelog-parity.sh --check / --check-bump origin/main	pass
scripts/check-silent-skips.sh	pass
scripts/check-shell-portability.sh --paths <changed>	pass
shellcheck -x -S warning <changed>	clean

Merge-order note

This PR and #2167 both bump plugins/source-control (this one because it carries the shared
hook-utils.sh). Main is at 0.51.5; #2167 now claims 0.51.6 and this PR claims 0.51.7, so
it stays strictly greater either way. Merge #2167 first — it is one plugin and cheaper to redo.
If this one lands first instead, #2167 must re-bump to 0.51.8.

Closes #2146

Related

• fix(guardrails): block-dangerous-git still clears an unsafe --force-with-lease via payload cwd and env -S #2124 / fix(guardrails): read the payload cwd, and stop env -S hiding commands from every git guard #2147 — a separate live bypass in the same guard, and the precedent for a 16-plugin lib bump
• docs(guardrails): block-dangerous-git's contract line says Bash-only; the matcher is Bash|PowerShell #2145 — the contract-line inaccuracy in the same file
• Triage the remaining stranded P2 review findings from the post-merge sweep #1938 — the stranded post-merge review-findings sweep
• docs(source-control): scope exec-bit-check's content-determinism claim to the A/C arms #2167 — the other PR in this pair; collides with this one on the source-control version bump

[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 1m 54s —— View job

---

Security review complete

Todo list

• Gather context: gh pr diff for PR fix(guardrails): fail closed when jq is missing in the irreversible-operation guards #2178
• Read changed hook scripts (hook-utils.sh, block-dangerous-git.sh, block-no-verify.sh, new test files)
• Analyze for injection surfaces, auth/access-control gaps, fail-open/fail-closed logic flaws
• Check the 15 mechanically-bumped plugins for unintended behavior changes
• Post findings as PR review

Scope

Reviewed the full diff (gh pr diff 2178): the new hook::require_jq_blocking gate and posture-doc block in lib/hook-utils.sh, the call-site swap in block-dangerous-git.sh and block-no-verify.sh, the new require-jq-posture.test.sh, the anchoring fix in require-jq-notice-isolation.test.sh, and the mechanical 16-plugin hook-utils.sh sync + version/CHANGELOG bumps.

Findings

No security issues found.

Notes from the review (informational, not findings):

• Ordering is correct for the fail-closed gate not to become a trap. In both block-dangerous-git.sh and block-no-verify.sh, hook::check_enabled (a plain env-var lookup, jq-free) runs before hook::require_jq_blocking, so the documented kill switch (block_dangerous_git_enabled / block_no_verify_enabled = false) still bypasses the guard even when jq is absent — confirmed by reading hook::check_enabled's implementation, not just the comment claiming it.
• hook_id/option arguments to hook::require_jq_blocking are call-site literals, not attacker-influenced data, so the echo ... >&2 messages built from them carry no injection surface.
• The 16-plugin hook-utils.sh sync is byte-identical everywhere (all copies share blob SHA 28fc4373591188d3cf553af020728cf668b46f28 per the changed-files list), and no call site outside guardrails invokes the new blocking gate — matching the PR's claim of "no behavior change" for the other 15 plugins. No divergent/backdoored copy.
• No permissions:, manifest-trust, or config-secret surface touched — the non-guardrails diffs are version bump + CHANGELOG + the identical lib sync only.
• New fail-closed test fixture (require-jq-posture.test.sh) hides jq via a BASH_ENV command/jq shell-function override, PATH left intact — test-only, runs against test-controlled fixtures/payloads (not attacker input), and its own JSON-payload builder correctly backslash/quote-escapes the embedded command string.
• This PR itself closes a genuine fail-open gap (fix(guardrails): every hook calling hook::require_jq fails OPEN when jq is absent, while the same guard fails closed on length #2146) rather than introducing one; the accepted cost — denying all Bash/PowerShell tool calls on a jq-less machine until jq is installed or the kill switch is set — is disclosed in code comments, the CHANGELOG, and the PR body, and is a deliberate availability/safety tradeoff rather than a vulnerability.

Per instructions, deferred to zizmor's advisory lane for supply-chain/unpinned-action, dangerous-trigger, permission, and template-injection concerns — none of the changed files here are GitHub Actions workflows.

[claude]

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

---

Code review complete

• Gather context (PR body, diff, CLAUDE.md)
• Review lib/hook-utils.sh changes (fail-closed helper, posture block)
• Review block-dangerous-git.sh and block-no-verify.sh changes
• Review new require-jq-posture.test.sh and updated require-jq-notice-isolation.test.sh
• Review 16-plugin CHANGELOG/version bump mechanics
• Post inline findings and summary

Findings (2 inline)

1. CI-failing — missing executable bit on plugins/guardrails/hooks/require-jq-posture.test.sh. I checked the ci workflow run for this PR directly (run 31435494302) — the hygiene job fails on exec-bit=failure, and every other hygiene sub-check (shellcheck, gitleaks, markdown, etc.) passed. The new file carries a #!/usr/bin/env bash shebang but was committed with mode 100644. One-line fix: git update-index --chmod=+x -- plugins/guardrails/hooks/require-jq-posture.test.sh.
2. Test-coverage gap on lib/hook-utils.sh#L233: the new hook::require_jq_blocking has no direct unit test in lib/hook-utils.test.sh (unlike its sibling hook::require_jq, covered at that file's "Test 17"). It's exercised only indirectly through the two guardrails hooks in require-jq-posture.test.sh. The 14 other plugins that now carry this function via the synced lib have no coverage of it at all, and the no-option-name message branch is never exercised by any current caller.

What held up well

• The core design is sound and internally consistent: hook::require_jq_blocking is correctly placed after hook::check_enabled (kill switch still wins) and after hook::buffer_stdin (jq-free) in both block-dangerous-git.sh and block-no-verify.sh — verified by reading the current files, not just the diff.
• I independently re-derived the "membership is mechanical" claim: grep -l MAX_COMMAND_LEN plugins/guardrails/hooks/*.sh returns exactly block-dangerous-git.sh and block-no-verify.sh (plus hook-utils.sh/the test file, which the test's own exclusion list already accounts for), and no other plugin's require_jq-calling hook (context-guard/zone-gate.sh, autonomy, source-control's pr-*-gate scripts) defines a length ceiling — matching the PR's exclusion table.
• The anchored regex fix in require-jq-notice-isolation.test.sh (hook::require_jq[[:space:]]) does correctly distinguish hook::require_jq "..." from hook::require_jq_blocking "..." — confirmed by running the pattern against both call forms.
• Spot-checked guardrails (0.24.1→0.25.0, minor bump for the new denial behavior) and source-control (0.51.5→0.51.7, consistent with the PR's stated merge-order note re: docs(source-control): scope exec-bit-check's content-determinism claim to the A/C arms #2167) version/CHANGELOG bumps — both consistent with the PR's own claims.
• I did not independently re-run the jq-hidden four-cell measurement or the full test suites described in the PR's "Verification run" table — those are author-claimed and unverified by me beyond what CI shows; the CI failure above is the one exception I confirmed directly via the Actions API.

[github-actions]

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

[github-actions]

Warning

Automated security review did not complete — this is an infrastructure failure, not a review verdict.

Treat any Claude comment on this PR (including a placeholder like "I'll analyze this and get back to you") as incomplete, not "no findings."

• Run: https://github.com/melodic-software/claude-code-plugins/actions/runs/31552365310
• Failure class: other
• Last SDK result: (no execution file was produced)

The check is green on purpose, and it is not evidence. It certifies that a security pass ran, and this one did not complete — but the cause is outside this PR's control, so merging is deliberately left unblocked rather than locking every merge for the length of the outage. Nothing was reviewed at this head. Where this check is required, it is satisfied without that evidence; a human should review security-sensitive changes here before merging.

Re-run the job to retry the review; a new push also retries it only if the caller's pull_request triggers include synchronize (the canonical security caller keeps it). An automatic retry may already have run — it is skipped when a partial review could duplicate comments, or when the failure class needs an operator.

Re-running does NOT help for every class:

• rate-limit that persists across re-runs, or auth — the credential or usage budget needs an operator; retrying will not clear it.
• a run that exhausted its turn budget ("subtype":"error_max_turns" above) will exhaust it again. As the PR author, split the change into smaller PRs; raising --max-turns is a change to the caller workflow, not something you can set on this PR.

[github-actions]

Warning

Automated review did not complete — this is an infrastructure failure, not a review verdict.

Treat any Claude comment on this PR (including a placeholder like "I'll analyze this and get back to you") as incomplete, not "no findings."

• Run: https://github.com/melodic-software/claude-code-plugins/actions/runs/31552365330
• Failure class: other
• Last SDK result: (no execution file was produced)

Re-run the job to retry the review. A new push re-triggers this lane only if the caller's pull_request triggers include synchronize (the canonical caller omits it).
An automatic retry may already have run — it is skipped when a partial review could duplicate comments, or when the failure class needs an operator (auth).