fix(hooks): fail closed when a PRE_TOOL_USE enforcement hook crashes#16
Merged
Conversation
A PRE_TOOL_USE hook that raised an exception previously returned allow=True, so a crashing enforcement gate (e.g. an approval hook) silently let the tool call through — a broken security boundary became a bypass. PRE hooks now deny by default when their handler raises. Non-enforcement hooks (rate limiter, loggers) opt into the prior fail-open behavior via a new `fail_open` flag. POST/SESSION hooks are unaffected (they cannot block execution). Updates the two hook-error tests that previously asserted the fail-open behavior and adds coverage for the fail-closed / fail-open split.
tao-hpu
added a commit
that referenced
this pull request
Jun 13, 2026
…ity fixes - README all-contributors: add wcboy (code), Joshua-Medvinsky (security) - Security Hall of Fame: first entries (FailSafe SSRF findings) - changelog: move hook fail-closed + add SSRF fixes under [Unreleased]
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
2 participants
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.
What
PRE_TOOL_USEhooks now fail closed: if a hook's handler raises, the tool call is denied by default instead of allowed. A newfail_openflag lets non-enforcement hooks (rate limiter, loggers) keep the previous fail-open behavior. POST/SESSION hooks are unchanged.Why
Hook.executecaught any handler exception and returnedHookResult(allow=True)unconditionally — so a crashing enforcement hook (e.g.FeishuGateHook, which holds a tool call behind a human approval card) would silently let the tool through. The module docstring calls hooks a "deterministic enforcement layer ... [that] cannot be bypassed", which a stray exception turning a gate into a no-op contradicts.This is a deliberate design-posture change, so flagging it explicitly: the old comment ("Hooks must NEVER crash the agent ... a permissive HookResult (allow=True) is returned") was an availability-first choice. This PR preserves that property for hooks that aren't security boundaries (
fail_open=True, set on the built-inrate_limiter) while defaulting enforcement gates to deny-on-error. Happy to narrow the split (e.g. opt-in fail-closed only on gate subclasses) if you'd prefer.Type
Checklist
fail_openflag, PRE fail-closed default, POST non-blocking, chain behavioruv run pytest -m "not docker"— 3780 passedmypy --strictclean on changed source filesruff check— this change introduces no new lint; two pre-existing issues (UP035inhooks.py, import-sort intest_agent_hooks.py) are left untouched to keep the diff surgical