hooks/tests: a crashed hook is scored as "allow", so a dead safety guard reads as a passing test #366

New issue

Closed

opened 2026-07-28 04:18:34 +00:00 by hexajon · 1 comment

hexajon commented

2026-07-28 04:18:34 +00:00

(Migrated from codeberg.org)

Found this while diagnosing a red main. The CI gate job failed on hook-tests with one case out of 72:

FAIL [dec=allow missing dir= for /tmp/tmp.rS1pvqUG8q] cd prefix -> dir= in list deny
      cmd=cd\ /tmp/tmp.rS1pvqUG8q\ \&\&\ git\ worktree\ list
Results: 71 passed, 1 failed

The same commit passed the same step in the pull-request run, and the suite passes locally on repeat runs, so the decision logic is not wrong. What the failure exposed is a harness problem that matters more than the flake.

The harness cannot distinguish "allowed" from "crashed"

hooks/tests/check-git-worktree.test.sh scores every case like this:

out=$( cd "$REPO" && printf '%s' "$cmd" | jq -Rs '{tool_input:{command:.}}' | bash "$HOOK" 2>/dev/null )
got_block="allow"; echo "$out" | grep -qE '"(decision|permissionDecision)"\s*:\s*"(block|deny)"' && got_block="block"

Three things go wrong together:

stderr is discarded (2>/dev/null), so a crash leaves no trace.
the exit code is never captured, and there is no pipefail, so a failure in either jq or the hook is invisible.
empty output defaults to allow, because got_block is initialised to allow and only ever upgraded to block.

A hook that dies before printing anything therefore scores exactly the same as a hook that deliberately permitted the command. That is the wrong default for a suite whose entire job is guarding safety rails: the failure mode it is least able to detect is the guard being dead.

In this instance it produced a confusing red. The worse case is the silent one - a hook that crashes on some input would show up as dec=allow on a test that expects allow, and pass.

What I want

Capture the pipeline's exit status (set -o pipefail around the invocation, or run the hook separately from the input-building jq) and report a non-zero exit as its own distinct result, never as a decision.
Keep stderr instead of discarding it, and print it on failure - a crashed hook should say why.
Make "no output" a third state rather than a synonym for allow. An expect-allow case should assert the hook exited 0 AND produced no deny, not merely that no deny appeared.
Apply the same treatment to the sibling helpers in the file (run, failopen, dir_remedy_check) and check the other hooks/tests/*.test.sh files for the same pattern.

failopen is the one case that legitimately expects a clean exit with no deny, and it already checks rc -eq 0 - that is the shape the others should follow.

Acceptance criteria

A hook that exits non-zero, or whose input-building jq fails, is reported as an error naming that, and never as allow or block.
Hook stderr is surfaced on a failing case.
An expect-allow assertion requires exit 0, not just the absence of a deny.
The suite still passes.

Found this while diagnosing a red main. The CI `gate` job failed on `hook-tests` with one case out of 72: ``` FAIL [dec=allow missing dir= for /tmp/tmp.rS1pvqUG8q] cd prefix -> dir= in list deny cmd=cd\ /tmp/tmp.rS1pvqUG8q\ \&\&\ git\ worktree\ list Results: 71 passed, 1 failed ``` The same commit passed the same step in the pull-request run, and the suite passes locally on repeat runs, so the decision logic is not wrong. What the failure exposed is a harness problem that matters more than the flake. ## The harness cannot distinguish "allowed" from "crashed" `hooks/tests/check-git-worktree.test.sh` scores every case like this: ```bash out=$( cd "$REPO" && printf '%s' "$cmd" | jq -Rs '{tool_input:{command:.}}' | bash "$HOOK" 2>/dev/null ) got_block="allow"; echo "$out" | grep -qE '"(decision|permissionDecision)"\s*:\s*"(block|deny)"' && got_block="block" ``` Three things go wrong together: - **stderr is discarded** (`2>/dev/null`), so a crash leaves no trace. - **the exit code is never captured**, and there is no `pipefail`, so a failure in either `jq` or the hook is invisible. - **empty output defaults to `allow`**, because `got_block` is initialised to `allow` and only ever upgraded to `block`. A hook that dies before printing anything therefore scores exactly the same as a hook that deliberately permitted the command. That is the wrong default for a suite whose entire job is guarding safety rails: the failure mode it is least able to detect is the guard being dead. In this instance it produced a confusing red. The worse case is the silent one - a hook that crashes on some input would show up as `dec=allow` on a test that *expects* allow, and pass. ## What I want - Capture the pipeline's exit status (`set -o pipefail` around the invocation, or run the hook separately from the input-building `jq`) and report a non-zero exit as its own distinct result, never as a decision. - Keep stderr instead of discarding it, and print it on failure - a crashed hook should say why. - Make "no output" a third state rather than a synonym for `allow`. An expect-allow case should assert the hook exited 0 AND produced no deny, not merely that no deny appeared. - Apply the same treatment to the sibling helpers in the file (`run`, `failopen`, `dir_remedy_check`) and check the other `hooks/tests/*.test.sh` files for the same pattern. `failopen` is the one case that legitimately expects a clean exit with no deny, and it already checks `rc -eq 0` - that is the shape the others should follow. ## Acceptance criteria - A hook that exits non-zero, or whose input-building `jq` fails, is reported as an error naming that, and never as `allow` or `block`. - Hook stderr is surfaced on a failing case. - An expect-allow assertion requires exit 0, not just the absence of a deny. - The suite still passes.

hexajon commented

2026-07-28 17:42:25 +00:00

(Migrated from codeberg.org)

Two behaviours in the merged harness were judgement calls rather than things the issue pinned. Recording them here so they are findable if either turns out wrong - both are reversible.

1. A suite whose guard never emits anything fails, by default.

Liveness is judged per suite, not per case, because within a single case an inert guard and a genuine allow are indistinguishable - both are "exit 0, no output". So the harness ledgers every invocation and, at finish, fails a suite where the guard stayed silent across all of them. All eight current suites have deny cases, so none is affected, verified over repeated runs. HOOK_HARNESS_ALLOW_SILENT_SUITE=1 opts out.

The cost: a future suite that legitimately expects total silence from its guard goes red until someone sets that flag. The alternative shape is opt-in liveness (an explicit hook_harness_expect_output call per suite), which can never false-red but lets a new suite default to having no liveness evidence at all - which is the gap this issue was about. Fail-loud-by-default seemed the right way round given that, but it is a preference, not a fact.

2. A payload builder that writes to stderr while still producing valid JSON is a note, not a failure.

The structural check means a builder that actually corrupts the payload is a hard failure - the guard is not consulted and the suite reddens naming the cause. This softer path only covers the case where the built JSON is provably well-formed and the builder merely warned. Reddening on environment noise would trade this issue's flake for a new one, and it matches how the library already treats stderr from the guard itself.

A stricter reading of the defect would hard-fail on any builder stderr at all. Worth revisiting if a real corruption ever slips through as a note.

Neither is load-bearing for the fix itself; both are about where the harness sits on the noisy-vs-silent axis.

Two behaviours in the merged harness were judgement calls rather than things the issue pinned. Recording them here so they are findable if either turns out wrong - both are reversible. **1. A suite whose guard never emits anything fails, by default.** Liveness is judged per suite, not per case, because within a single case an inert guard and a genuine allow are indistinguishable - both are "exit 0, no output". So the harness ledgers every invocation and, at finish, fails a suite where the guard stayed silent across all of them. All eight current suites have deny cases, so none is affected, verified over repeated runs. `HOOK_HARNESS_ALLOW_SILENT_SUITE=1` opts out. The cost: a future suite that legitimately expects total silence from its guard goes red until someone sets that flag. The alternative shape is opt-in liveness (an explicit `hook_harness_expect_output` call per suite), which can never false-red but lets a new suite default to having no liveness evidence at all - which is the gap this issue was about. Fail-loud-by-default seemed the right way round given that, but it is a preference, not a fact. **2. A payload builder that writes to stderr while still producing valid JSON is a note, not a failure.** The structural check means a builder that actually corrupts the payload is a hard failure - the guard is not consulted and the suite reddens naming the cause. This softer path only covers the case where the built JSON is provably well-formed and the builder merely warned. Reddening on environment noise would trade this issue's flake for a new one, and it matches how the library already treats stderr from the guard itself. A stricter reading of the defect would hard-fail on any builder stderr at all. Worth revisiting if a real corruption ever slips through as a note. Neither is load-bearing for the fix itself; both are about where the harness sits on the noisy-vs-silent axis.