feat(hooks): add per-hook fail_mode option for fail-closed gating#1906
Open
StressTestor wants to merge 1 commit into
Open
feat(hooks): add per-hook fail_mode option for fail-closed gating#1906StressTestor wants to merge 1 commit into
StressTestor wants to merge 1 commit into
Conversation
By default hooks fail open: spawn failure, crash, timeout, or an unexpected exit code all resolve to allow. That is the right default for notifications and lightweight checks, but it means a hook acting as a security gate silently stops gating when it breaks or times out. fail_mode = "closed" inverts the failure behavior per hook: any failure to deliver a verdict (spawn failure, runtime error, timeout, death by signal, exit code other than 0 or 2) blocks the operation with an explicit reason instead of allowing it. Exit 0/2 and structured stdout decisions behave exactly as before, and user interrupts still resolve to allow in both modes since the turn is being cancelled anyway. Implemented in both agent-core (session/hooks) and agent-core-v2 (agent/externalHooks). Note the config field is snake_case fail_mode in agent-core's schema (hook arrays bypass transformTomlData's key camelization) and camelCase failMode in agent-core-v2's section schema (hooksFromToml camelizes entries before validation); plugin manifests accept either spelling and normalize per core. Hook dedup now keeps the stricter mode when identical commands collide, and agent-core's close handler treats signal death (exit code null) as a failure rather than coercing it to a clean exit 0. Default remains "open"; existing configs are unaffected.
🦋 Changeset detectedLatest commit: 97f6b5e The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
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.
Related Issue
Resolve #1905
Problem
See linked issue. Hooks fail open in every failure mode, so a hook acting as a security gate silently stops gating when its script breaks, hangs, or fails to spawn. The docs currently warn that hooks "should not be used as the sole security barrier" for exactly this reason.
What changed
[[hooks]]entries accept an optionalfail_modefield:"open"(default, today's behavior, existing configs unaffected) or"closed", where any failure to deliver a verdict (spawn failure, runtime error, timeout, death by signal, exit code other than 0 or 2) blocks the operation with an explicit reason. Exit 0/2 and structured stdout decisions are unchanged, and a user interrupt still resolves to allow in both modes, since the turn is being cancelled anyway.Signal death needed its own handling in agent-core: the close handler coerced
code=nullto exit 0 viacode ?? 0, so a fail-closed hook killed by SIGKILL or crashing with SIGSEGV would have read as a clean allow. Underfail_mode = "closed"a null exit code now blocks; the default path keeps the existing coercion.Implemented in both
agent-core(session/hooks) andagent-core-v2(agent/externalHooks). The config field is snake_casefail_modein agent-core's schema because hook arrays reach it untransformed (transformTomlDatapasses arrays through without key camelization), and camelCasefailModein agent-core-v2's section schema becausehooksFromTomlcamelizes entries before validation; both are covered by tests. Since both cores read the samekimi.plugin.json, the plugin manifest readers accept either spelling and normalize to their core's canonical form, so a manifest written for one core is not silently dropped by the other.Also fixes hook dedup so identical
(cwd, command)entries with different fail modes collapse to the stricter one instead of whichever parses first, and adds test coverage for the previously untested spawn-failure branch in agent-core's runner.One pre-existing divergence is left untouched: in default fail-open mode, a hook that prints a structured deny and then dies by signal is blocked by agent-core (
code ?? 0coerces to exit 0, so stdout is parsed) but allowed by agent-core-v2 (code ?? -1skips structured parsing). Underfail_mode = "closed"both cores block, so the new mode is consistent; changing the fail-open behavior felt out of scope for this PR.Docs updated in en and zh (field table, fail-open section, warning box). Verified end to end against a built CLI: a crashing PreToolUse hook with
fail_mode = "closed"blocks the matched Bash call withHook failed (fail_mode=closed): exit code 1, and the same hook without the field preserves fail-open behavior.Checklist
gen-changesetsskill, or this PR needs no changeset.gen-docsskill, or this PR needs no doc update.