fix(hooks): fail closed on PreToolUse hooks that time out, crash, or are missing (#7339)

[bolichen97]

Fixes #7339.

Problem

PreToolUse script hooks are the policy gate on the auto-approve path, but every failure mode of the hook itself resolved to allow. A hook that exits 2 within its window blocks correctly; the same hook made slow, killed, or removed silently stopped blocking anything.

Root cause (as diagnosed in the issue):

• run_script_hook (src/kiro_crew/hooks.py) maps a timed-out hook to exit_code=-1 and a missing binary to 127; blocked is defined as exit_code == 2, so both resolve to "no opinion."
• _fire (src/kiro_crew/dashboard/chat_runner.py) drops any non-0/non-2 result, and _pre_tool_hooks_should_block treats the empty list as pass-through, so the tool is approved.

Design decision

The issue's maintainer-bot comment flagged that the remedy carries a design decision (default direction, per-hook override, autonomous-path contract) needing an owner. The decision made here:

• Per-hook on_error field on ScriptHook with values fail_closed / fail_open and a "" sentinel that resolves at read time: fail-closed by default for PreToolUse, fail-open for every other (non-gating) event. This preserves historic behavior for non-gating hooks and for anyone who explicitly opts into fail_open.

Changes

• hooks.py: added on_error field, ScriptHookResult.failed_to_run, and should_block_pre_tool_use(). Config parsed fail-soft in from_dict (junk resolves to sentinel, never raises).
• chat_runner.py (the gating surface): _fire now emits a BLOCKED: marker for a fail-closed failed-to-run PreToolUse hook, so _pre_tool_hooks_should_block blocks it. Explicit fail_open hooks keep the historic pass-through.
• subagent.py autonomous/task-runner path: fire_tool_hooks receives the tool-call event after kiro-cli has already auto-approved and started the tool, so it architecturally cannot block. Rather than a false guarantee, it now surfaces the fail-open gap loudly (non-fatal WARNING naming the hook, tool, and PreToolUse script hooks fail open: a deny hook that times out, crashes, or is missing is silently approved #7339) instead of silently discarding results. Its docstring was rewritten to state this contract precisely and point to chat_runner as the gating surface.
• validation.py + dashboard API schemas: on_error validated at the write boundary (create + update), reachable via HOOK_CREATE_SCHEMA / HOOK_UPDATE_SCHEMA; update() normalizes identically to create().

Tests

New test/test_fire_tool_hooks.py covers, with real subprocesses: timeout / missing-binary / crash cases now block under the fail-closed default (assertions that fail against pre-fix code); per-hook fail_open override still allows; non-gating events never block; config plumbing and API-schema flow-to-store; create/update normalization parity; and the autonomous-path WARNING + non-fatal contract.

Testing notes

This branch was developed under a network-restricted sandbox (repository access only), where PyPI is unreachable and the full pytest suite could not run (aiohttp/yarl/hypothesis/pytest-asyncio unavailable). Verification performed in-sandbox:

• py_compile passes on all changed files; import kiro_crew.hooks succeeds.
• Two standalone behavioral harnesses: 27/27 checks on the fail-closed logic and 8/8 on API-schema reachability + create/update normalization parity.
• Semantic review reached APPROVED.

Please run python -m pytest test/test_fire_tool_hooks.py in CI to confirm the new tests pass with real dependencies.

Follow-up (non-blocking)

The dashboard UI control for on_error in website/src/pages/HooksPage.tsx was intentionally deferred (the override is reachable via the API and the fail-closed default needs no UI; the TS/i18n build could not be verified under the restricted network mode). Worth tracking as a follow-up.

Related

• ACP provider doesn't honor an agent's declared hooks, and the agent catalog doesn't auto-register project-local agents #2598 (ACP provider never fires declared hooks; different mechanism, same outcome class)
• apps: bound lifecycle startup hooks with a per-hook timeout #5443 (timeout bounds for lifecycle startup hooks)

Pattern harvest

Rule candidate: A test that asserts a spawned child's own exit code must first neutralize the spawn chokepoint, or it is asserting about the host. In this repo run_script_hook, cron scripts, app backends and MCP spawns all route argv through sandbox.sandboxed_spawn_argv -> wrap_argv + cgroup_scope_argv, so the process that runs is the sandbox launcher and the reported returncode is the launcher's whenever it refuses — 1 with sandbox: BLOCKED where a hiding/sealing mount fails (dev container), -1 where unshare(CLONE_NEWNS) is EPERM (GitHub runners) or where there is no backend at all (Windows). If the test is about the child's semantics rather than about confinement, patch kiro_crew.sandbox.wrap_argv and kiro_crew.sandbox.cgroup_scope_argv to identity at the sandbox module (they are imported lazily at call time) and say in a docstring why. Two corollaries, both of which bit here: (a) prefer the passthrough patch over skipif(not _sandbox_backend_available()) — that guard trusts detect_backend(), which answers "available" on a host whose launcher then fails at the mount step, which is exactly why test_stop_hook_continuation.py::TestRealHookProcess is red on main in this environment; (b) a fail-direction test must assert WHICH failure branch ran ("Timed out" in error, the injected exception's text, a specific exit code) and not only the umbrella predicate — failed_to_run is satisfied by any spawn failure, so three tests here were green on all three hosts while the timeout branch they claimed to cover never executed. The same reasoning kills non-portable hook commands: sleep 8; exit 2 and true cannot run under cmd /c, so use the quoted-interpreter + quoted-script shape (test_script_hooks._script_command) and assert shell-specific exit codes like 127 under if not _IS_WINDOWS.

[github-actions]

Design Review (Fable 5) — ✅ PASS

Design-level review of e0282484cc3638ca9961ca767c8b92226f859c2c — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

Design-Verdict: PASS

Right default in the right layer: fail-closed only where the gate actually exists, per-hook escape hatch, fail-soft on load, strict at the write boundary.

Suggestions

• The autonomous-path "would have BLOCKED / failed to deliver a verdict" event is a security-relevant policy breach surfaced only as a logger.warning; also record it in the SEL audit log (and/or the activity feed) so it survives log rotation and reaches an operator — small follow-up serving this PR's own observability goal.
• When the changelog for this release is written, the default flip belongs under "Before you upgrade": a previously-ignored flaky/slow/unconfinable PreToolUse hook now hard-denies matching tools on the chat path (the docs' Windows-no-sandbox case especially).

[DESIGN-REVIEWED] e028248

[github-actions]

First Principles Review (Fable 5) — 🟡 CONCERNS

Premise-level review of e0282484cc3638ca9961ca767c8b92226f859c2c — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

All evidence gathered. Producing the review.

First-Principles-Verdict: CONCERNS

The fix is real and cause-level, but should_block_pre_tool_use's on_error parameter ships with zero production callers — a second, unused way to pass what the result already carries.

What this change ships

Intent: make a PreToolUse policy hook that times out, crashes, or is missing deny the tool instead of silently approving it (#7339) — a FIX.

1. Broken PreToolUse hook now blocks the tool on the dashboard chat path — justified, cause-level (fixed at _fire, the single chokepoint all four gate sites consume)
2. failed_to_run / should_block_pre_tool_use on ScriptHookResult — justified (the fix's mechanism)
3. New per-hook config key on_error (fail_closed/fail_open/sentinel) — declared; derived escape hatch for the changed default (doc names the unconfined-Windows-host consequence)
4. on_error accepted via dashboard create/update API schemas — justified given item 3
5. update() now normalizes on_error like create() — justified given item 3
6. Autonomous-path fire_tool_hooks logs a WARNING for a fail-closed block/failure it cannot prevent — declared, rides along; observability only, zero option leaves the gap silent
7. on_error= parameter on should_block_pre_tool_use — zero consumers
8. fail_open/non-gating hook failures that set only error (e.g. timeouts) now log a warning where they were dropped — undeclared
9. Spec section in memory-skills-hooks.md, same commit — justified (AGENTS.md mandate)
10. fire_tool_hooks docstring rewritten as an explicit cannot-gate contract — declared

Watch

• Item 8 is only described in a code comment, not the PR description — declared nowhere a reviewer of the description would see; grepped should_block_pre_tool_use across src/: 1 call site (chat_runner.py:4714), which passes no argument, so item 7's parameter is exercised by tests alone.
• The related fail-open sibling the author names (ACP provider doesn't honor an agent's declared hooks, and the agent catalog doesn't auto-register project-local agents #2598, ACP provider hooks) stays unfixed — declared and deferred, acceptable.

Subtractions

• Drop the on_error: str = "" parameter from ScriptHookResult.should_block_pre_tool_use (hooks.py:3577) — 0 production consumers (grep: only chat_runner.py:4714, argument-less); the result already threads self.on_error, so read that alone.

[FIRST-PRINCIPLES-REVIEWED] e028248

[github-actions]

Opus 4.8 Review — ✅ no blocking findings

Reviewed e0282484cc3638ca9961ca767c8b92226f859c2c — this comment is updated in place on each push.

Review details

No findings.

[OPUS-REVIEWED] e028248

Verdict parsed from the review's SHA-scoped output markers for commit e0282484cc3638ca9961ca767c8b92226f859c2c.

False positive or not applicable? A repository writer can comment:
/ai-review override fable e0282484cc3638ca9961ca767c8b92226f859c2c: <one-sentence reason>

[github-actions]

GPT 5.6 Review — ✅ no blocking findings

GPT 5.6 completed its review of e0282484cc3638ca9961ca767c8b92226f859c2c and found no blocking issues.

This comment is updated in place on each push.

Review details

FINDING -- src/kiro_crew/hooks.py:3127 -- "rejects a bad EXPLICIT value at the create/update boundary" contradicts both store paths normalizing junk before validation -> Fix: state that API schemas reject junk while store paths normalize it.
[GPT-REVIEWED] e028248

False positive or not applicable? A repository writer can comment:
/ai-review override gpt e0282484cc3638ca9961ca767c8b92226f859c2c: <one-sentence reason>

[bolichen97]

Superseded: issue #7339 is already fixed on main by merged PR #7422 (fix(hooks): fail closed when a PreToolUse hook delivers no verdict), which added the fail-closed handling for timeout/crash (-1) and unexecutable (126/127) exit codes now live in src/kiro_crew/dashboard/chat_runner.py. This PR's core fix is fully covered. Its only unique piece -- a per-hook on_error: fail_closed|fail_open opt-out knob -- is a security-gate opt-out that needs a maintainer decision, not a mechanical rebase, so closing rather than driving to green. Happy to re-open scoped to just that knob if it's wanted.