Development record
The hook plane's fail-open fix covers sub-verbs only, not flags or leaf positionals. iss-267 routes the guard and hook PARENTS through failOpenNoArgs so an unknown sub-verb refuses at exit 1 rather than the host's blocking status 2. Three neighbouring paths still exit 2: an unknown TOP-LEVEL token hits the root's Args validator (root's exit-2 contract is pinned by 04-surfaces/08-abcd.md, lint_surface_test.go and root_dispatch_test.go, so inverting it is a larger change), a stray positional on a leaf such as 'guard hook' exits 2, and ANY unknown flag exits 2 via FlagErrorFunc which failOpenNoArgs does not touch. None is reachable from today's hooks.json, but each becomes reachable the moment the manifest changes shape: if a future hooks.json adds a flag (e.g. 'abcd guard hook --strict') and skews against an older binary, iss-267 returns verbatim -- same brick, same wrapper blind spot -- and TestHooksManifestNamesLiveSubverbs would not catch it, because its regex stops at the first non-subverb token and never inspects flags. Also worth deciding: whether a top-level rename of 'guard' itself (this release already renames audit to lint) should be covered, since a skewed pair would then block every Bash call through the root validator. Options: extend failOpenNoArgs to the leaf and to FlagErrorFunc on the hook plane; extend the manifest test to pin flags too; or rely solely on the manifest-freeze doctrine and document it. Found by the security and ruthless reviews of the iss-267 fix.
The hook plane's fail-open fix covers sub-verbs only, not flags or leaf positionals. iss-267 routes the guard and hook PARENTS through failOpenNoArgs so an unknown sub-verb refuses at exit 1 rather than the host's blocking status 2. Three neighbouring paths still exit 2: an unknown TOP-LEVEL token hits the root's Args validator (root's exit-2 contract is pinned by 04-surfaces/08-abcd.md, lint_surface_test.go and root_dispatch_test.go, so inverting it is a larger change), a stray positional on a leaf such as 'guard hook' exits 2, and ANY unknown flag exits 2 via FlagErrorFunc which failOpenNoArgs does not touch. None is reachable from today's hooks.json, but each becomes reachable the moment the manifest changes shape: if a future hooks.json adds a flag (e.g. 'abcd guard hook --strict') and skews against an older binary, iss-267 returns verbatim -- same brick, same wrapper blind spot -- and TestHooksManifestNamesLiveSubverbs would not catch it, because its regex stops at the first non-subverb token and never inspects flags. Also worth deciding: whether a top-level rename of 'guard' itself (this release already renames audit to lint) should be covered, since a skewed pair would then block every Bash call through the root validator. Options: extend failOpenNoArgs to the leaf and to FlagErrorFunc on the hook plane; extend the manifest test to pin flags too; or rely solely on the manifest-freeze doctrine and document it. Found by the security and ruthless reviews of the iss-267 fix.
Frontmatter
| id | iss-269 |
|---|---|
| type | issue |
| lifecycle | resolved |
| severity | minor |
.abcd/work/issues/resolved/iss-269-the-hook-plane-s-fail-open-fix-covers.md
open on GitHub ↗ · commit history ↗
mentions3
- iss-267Hook sub-verb names are now a compatibility contract, not a cushion. iss-266 makes every parent runnable, so an unknown sub-verb exits 2 instead of printing help at exit 0. On the Claude Code hook plane exit 2 is the BLOCKING status: exit 2 on UserPromptSubmit blocks the prompt, and exit 2 from 'guard hook' on PreToolUse blocks every Bash tool call -- and the PreToolUse wrapper in hooks/hooks.json treats 2 as a recognised code, so its 'FAILED TO RUN ... UNGUARDED' safety net does not fire. hooks.json ships with the plugin git clone while the binary is fetched from the latest release by hooks/bootstrap.sh, so the two can skew. No live instance today (every sub-verb hooks.json names exists at v0.5.1), and the fail-closed direction is intended. But a future rename of 'guard hook' or any 'hook <sub-verb>' must now carry an alias rather than rely on the old silent exit-0 cushion: against a skewed hooks.json/binary pair it would brick a session where it previously degraded silently. Found by the security review of the iss-266 fix.
- iss-2609012040003576GHSA-5wx3-2c86-fjpx (draft advisory, severity medium; CWE-636, Not Failing Securely): the wired PreToolUse guard hook fails open on two tokenizer error states that every shell an agent runs under executes. `tokenize` (internal/core/guard/tokenize.go) returns ErrUnparsableCommand for a backslash as the last byte of the line and for a here-document body that never reaches its delimiter line, and `newGuardHookCommand` (internal/surface/cli/guard.go) maps every Check error to exit 1 'NOT CHECKED … runs UNGUARDED', so `git push --force origin main \` and `git push --force origin main <<EOF` followed by a newline run without a verdict — bash 3.2 and 5.3, zsh and dash all execute both (the trailing backslash is dropped by bash 3.2/zsh, kept as a literal word by bash 5.3/dash; the empty heredoc body is recovered silently). A third shape shares the root cause: spaced arithmetic `echo $(( x << y ))` on one line and a hazard on the next is misread as a heredoc, because the `<<` branch tests only the byte immediately after the delimiter word for a paren and `followedByParen` sees the space, so the hazard line is swallowed as body and the same error returns. The sibling `shellInspect` (internal/core/guard/payload.go) maps the inner tokenizer's error to a warn, so `sh -c 'git push --force origin main \'` is a loud-warn allow through the same states. The fix must give the two bash-executable states a VERDICT in core rather than an error: arm 1 parses a trailing backslash as bash 3.2 does (dropped — the reading under which the hazard runs, so the fail-safe one) and yields a precise Tier-1 verdict; arm 2 turns an unterminated heredoc body into a segment flag that Check folds into a synthetic fail-closed block (the 2dbb0ca2 brace-group precedent: an error would route to the hook's fail-open), never a remap of the hook's error path, because the unterminated-quote class no shell runs must stay the loud fail-open of spc-16 / iss-269; prerequisite in the same change, the `<<` branch skips blanks before the paren test so `$(( x << y ))` classifies as arithmetic. The hook then blocks both spellings with the entry that names them, `echo hello \` allows, `rm -rf "unterminated` still fails open loud, and the repo corpus holds.
- iss-2609251755278758applyHookPlaneFailOpen resets FlagErrorFunc and Args on the hook plane but not the PreRunE that markUsageErrorsExitTwo now installs on every command, so a flag group declared on guard hook or hook * in future would refuse at exit 2, the host's BLOCK, bypassing iss-269's fail-open (internal/surface/cli/cli.go:138; hypothetical today: no hook command declares a group; review2-consolidate nit).