fix(hooks): fail closed when a PreToolUse hook delivers no verdict

[iamwhatever]

Problem / Motivation

A PreToolUse script hook is the policy gate on the dashboard auto-approve path, but every failure mode of the hook itself resolved to allow. A hook that exits 2 inside its window blocks correctly; the same hook made slow, killed, or removed stops blocking anything, with no refusal and nothing beyond a log warning.

run_script_hook's asyncio.TimeoutError and generic-exception branches return a ScriptHookResult with no exit_code, so it keeps the dataclass default -1; a hook binary the shell cannot exec comes back as 127. _fire converted only exit 0 (inject stdout) and exit 2 (emit a BLOCKED: marker) into results — every other code fell into a log-only branch and contributed nothing, so _pre_tool_hooks_should_block([]) hit the documented empty-list pass-through and the tool proceeded.

Why it matters

Slowing, breaking, or deleting the deny hook silently disabled the policy it enforces, at the moment enforcement mattered. The reporter hit this wiring an external approval tool behind a preToolUse hook: any decision slower than the hook window — a human reading the command before approving it — was converted into an approval.

Two neighbouring paths on the same branch already fail closed (an uninitialized hook store emits BLOCKED:system:hook store not initialized; an exception out of fire() rejects via _reject_hook_error), so the intent existed — the timeout and crash cases just never reached either one.

What changed (motivation → approach → change)

Symptom: a timed-out or crashed deny hook approves the tool. Root cause: _fire treats "no verdict" and "allow" as the same thing, because both produce an empty results list. Change: for HOOK_EVENT_PRE_TOOL_USE, any hook result whose exit code is neither 0 nor 2 now emits a BLOCKED:<hook>:<detail> marker, logs at error level, and broadcasts the block to the activity feed — the same shape as the two neighbouring fail-closed paths.

A policy gate has a two-valued contract: exit 0 is a delivered allow, exit 2 is a delivered deny. Any third value means the gate did not decide, and for a gate that must resolve to deny. Exit 0 still approves, so a healthy silent-allow hook is untouched.

The detail string prefers result.error (already redacted inside run_script_hook) over stderr, falling back to the exit code when both are empty — a missing binary produces neither. Non-gating events (postToolUse, userPromptSubmit, stop) keep the previous warn-only behaviour, so nothing outside the gate changes.

This deliberately covers more than the undelivered shapes: a hook that runs to completion and exits 1 now blocks where it previously warned. That breadth is stated in the comment, because it is a behaviour change for an existing warn-style preToolUse hook. It is the right direction — a hook's own uncaught error surfaces as exit 1 and is indistinguishable from a deliberate one, and the exit code a failed exec produces is shell- and platform-specific (cmd /c yields 9009 or 1 where /bin/sh yields 127), so an allowlist of "real" failure codes would fail open on Windows for exactly this class. The hook store already calls every nonzero non-2 exit an error (last_status = "error"); this branch gives the gate the matching direction.

The spec moved with the code: docs/system-specs/modules/learn-cron-dashboard.md's Tool-Refusal Recovery section described only exit 2 → BLOCKED:<hook>:<stderr> and now also names any other nonzero exit. No CHANGELOG.md entry — that file holds shipped releases only, so the behaviour change is one for the release editor to pick up rather than something this PR can add.

Scope held deliberately narrow: the subagent / task-runner path (fire_tool_hooks) is informational-only by design — the tool is already running when hooks fire and results are discarded — so a preToolUse hook cannot deny there at any exit code or speed. Closing that means moving the hook ahead of execution on the autonomous paths, which is a control-flow change to the autonomous runtime rather than a classification fix. It is tracked on its own in #7547, so merging this and closing #7339 does not lose the tracker for the remaining fail-open path. #7547 also carries the per-hook direction switch the issue names as the escape hatch, and the point at which the verdict predicate is worth hoisting out of _fire — with the second caller in hand, the shape of the shared predicate is finally known.

Tests

• test_auto_approve_blocked_when_pretooluse_hook_delivers_no_verdict — parametrized over the three failure shapes from the issue's repro (timeout exit_code=-1 + error, crash exit_code=-1 + error, missing binary exit_code=127). Drives the full _run_chat auto-approve path and locks in that reject_tool is called and the user-facing pill reports the hook blocked.
• test_auto_approve_allowed_when_pretooluse_hook_exits_zero — pins the new branch to non-0/2 exits, so a delivered allow is not caught by it.
• Revert-verified: with the chat_runner.py change stashed, all three failure-shape cases fail (reject_tool called 0 times) and the exit-0 case still passes.

Manual verification

N/A — unit coverage is sufficient: the tests drive the real _run_chat auto-approve path end to end rather than the helper in isolation, and the issue's repro is reproduced as the parametrized failure shapes.

Related Issues

Fixes #7339

Related: #7547 — the autonomous-path (fire_tool_hooks) half, deliberately not closed by this PR.

Pattern harvest

Rule candidate: review-prompt
Pattern: a security verdict derived from an equality test against a permissive-defaulting field, so an error path that never sets the field reads as "allow" (exit_code: int = -1 with blocked = exit_code == 2, and a [] results list meaning both "no hooks" and "the hook died").

Checklist

• At most two commits (one is the norm), with a Conventional Commits title (feat|fix|docs|refactor|perf|test|chore|ci|build|revert: ...)
• Existing tests pass and new tests added for new functionality
• Self-review completed; code follows project style guidelines
• Documentation updated (if applicable)
• No secrets, credentials, or internal references in the diff

[github-actions]

GPT 5.6 Review — ✅ no blocking findings

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

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] 5fbce9d

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

[github-actions]

Design Review (Fable 5) — 🟡 CONCERNS

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

Design-Verdict: CONCERNS

Fail-closed on the gate is right and deliberately scoped; the exit-1 breadth flip and the still-open autonomous-path hole are the risks to watch.

Watch

• The branch "deliberately covers more than the undelivered shapes": any existing warn-style preToolUse hook that exits 1 (a linter, a notifier) now hard-blocks every tool call after upgrade, and the per-hook direction switch the issue names as the escape hatch is deferred. That is a behaviour flip on configured installs with no shipped opt-out — worth landing the switch in the same release, or at minimum a release-notes flag for the editor.
• The PR says "Fixes PreToolUse script hooks fail open: a deny hook that times out, crashes, or is missing is silently approved #7339" while also saying the subagent/task-runner fail-open "stays open on PreToolUse script hooks fail open: a deny hook that times out, crashes, or is missing is silently approved #7339 as follow-up" — merging auto-closes the only tracker for the remaining path where a deny hook is purely advisory (fire_tool_hooks fires after the tool runs and discards results). File the follow-up before merge.

Suggestions

• The 0/2-verdict interpretation now lives in a closure inside _run_chat; hoisting it onto ScriptHookResult (e.g. a verdict property in hooks.py) would let the PreToolUse script hooks fail open: a deny hook that times out, crashes, or is missing is silently approved #7339 autonomous-path follow-up reuse it instead of duplicating the branch per surface.

[DESIGN-REVIEWED] 5fbce9d

[github-actions]

First Principles Review (Fable 5) — ✅ PASS

Premise-level review of 5fbce9db419de88fbaa420468f435cc1c6483c62 — 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 claims in the description verified against the repo: ScriptHookResult.exit_code defaults to -1 (hooks.py:3431), the old branch warned-only, _pre_tool_hooks_should_block([]) passes through (chat_runner.py:461-475), fire_tool_hooks discards results by documented design (5 call sites, deferred to the issue), and the hook store already labels nonzero non-2 exits as "error" (hooks.py:3649). The fix sits exactly at the decision that conflated "no verdict" with "allow", reuses the existing BLOCKED: marker mechanism, and moves the spec in the same commit.

First-Principles-Verdict: PASS

A dead, slow, or missing deny hook no longer approves the tool; every item is the fix, its declared breadth, or the mandated spec move.

What this change ships

Intent: stop a broken/timed-out PreToolUse deny hook from silently approving tools on the dashboard auto-approve path — a FIX (#7339).

1. A deny hook that times out, crashes, or is missing now blocks the tool — justified (the agent-governance boundary; zero option leaves the gate disabled exactly when attacked).
2. A hook finishing with exit 1 now blocks where it only warned — declared changed default, derived (cmd /c yields 9009 or 1 where /bin/sh yields 127, so a "real failure" allowlist fails open on Windows; verified last_status = "error" at hooks.py:3649 already calls it an error).
3. The block appears in the activity feed and error log — justified (an operator can't fix a gate they can't see failing; same surface the exit-2 block already has).
4. Block reason prefers the hook's redacted error over stderr — justified (timeout/crash produce no stderr; redaction happens upstream in run_script_hook).
5. Spec's Tool-Refusal Recovery section names the new exit behavior — mandated same-commit by AGENTS.md.
6. Non-gating events (postToolUse etc.) keep warn-only — unchanged, declared.

Counted: gating consumers of hook exit codes = 1 (_fire, chat_runner.py:4711; grepped exit_code == 2|exit_code not in — hooks.py:3642 is status labeling, .blocked property has 0 consumers). The one sibling path (fire_tool_hooks, 5 call sites) is informational-by-design with results discarded; making it gating requires reordering hooks ahead of execution — genuinely out of scope, declared, tracked on #7339.

[FIRST-PRINCIPLES-REVIEWED] 5fbce9d

[github-actions]

Opus 4.8 Review — ✅ no blocking findings

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

Review details

No findings.

[OPUS-REVIEWED] 5fbce9d

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

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

[iamwhatever]

• span=8023cfe9506b — rebutted (src/kiro_crew/dashboard/chat_runner.py — "r.exit_code not in (0, 2) blocks documented warning exits such as 1")

There is no documented allow-on-exit-1 contract for preToolUse to preserve. The only documented codes are exit 0 (stdout injected into context) and exit 2 (docs/reference/kiro-cli/hooks.md: "Can block (exit 2)"). Every other code is already classified as an error, not a warning, by the hook store itself — run_script_hook sets hook.last_status = "error" for any nonzero non-2 exit, and the dashboard's Hooks page renders that as an error badge. There is no "warning" status in the model. The removed logger.warning line was an implementation detail of _fire, and no test or doc pins exit 1 as a permitted allow on the gate path (test_result_classification asserts only that ScriptHookResult.blocked is False for exit 1 — a dataclass property this PR does not touch).

The suggested narrowing — block execution failures only — is not implementable without reopening the reported defect. The only signal that separates "the hook ran and chose this code" from "the hook never ran" is ScriptHookResult.error, which is set for the timeout and exception branches but empty for the missing-binary case: /bin/sh reports it as an ordinary exit 127. Narrowing to error therefore re-admits the third case the issue measures. Narrowing by an exit-code allowlist instead is platform-dependent — hooks spawn through cmd /c on Windows, where a missing command yields 9009 or 1 rather than 127 — so the allowlist would fail open on Windows for exactly this class, which is the failure mode being fixed.

Accepted consequence, stated plainly: a preToolUse hook that exits 1 now blocks where it previously warned. That is the intended direction. A preToolUse hook is indistinguishable from the outside from a policy gate, so an errored one has to be treated as one — assuming otherwise is what makes a deny hook unenforceable. The issue's own Expected section sanctions the escape hatch for anyone who wants the old direction ("make the direction configurable per hook, with fail closed as the default for preToolUse"); that is a config surface and is tracked as follow-up on #7339 rather than widened into this fix.

[iamwhatever]

• Watch — the fail-closed set is broader than "undelivered verdicts", and that breadth was undocumented — fixed in 5fbce9db419de88fbaa420468f435cc1c6483c62

Taken as stated, and resolved the way the review's first option names it: keep the breadth, document the choice. The gate keeps blocking every non-0/2 exit, and the comment in _fire now says so in those terms — a PreToolUse hook has a two-valued contract (0 is a delivered allow, 2 a delivered deny), everything else means the gate did not decide, and a delivered exit 1 therefore blocks where it previously only warned.

The comment also records why the narrower option (scope to -1/126/127) was not taken: a hook's own uncaught error surfaces as exit 1 and is indistinguishable from a deliberate one, and the code a failed exec produces is shell- and platform-specific — cmd /c yields 9009 or 1 where /bin/sh yields 127 — so an allowlist of "real" failure codes would fail open on Windows for exactly the class being fixed. The supporting fact that the review itself implies is now named too: the hook store already calls every nonzero non-2 exit an error (run_script_hook sets last_status = "error"), so this branch gives the gate the direction the store's own classification already had.

The spec was under-describing the same mechanism, so it moved with the code: docs/system-specs/modules/learn-cron-dashboard.md's Tool-Refusal Recovery section named only exit 2 → BLOCKED:<hook>:<stderr> and now also names any other nonzero exit → BLOCKED:<hook>:<error>.

On release notes: CHANGELOG.md holds shipped releases only — check_changelog_history.py rejects a section written for an unreleased version — so this is called out for the release editor here rather than added to the file in this PR.

[iamwhatever]

• Suggestion — put a delivered_verdict/no_verdict property on ScriptHookResult so both gates share one definition — rebutted (disproportional)

The predicate has exactly one caller today. The second consumer is the fire_tool_hooks follow-up, which does not exist yet and cannot be satisfied by a shared property alone: that path is informational by construction — the tool is already running when hooks fire and the results are discarded — so making a hook deny there means moving the hook ahead of execution, not reading the same boolean. Extracting the property now would fix the shape of a shared contract against a consumer whose control flow is still unbuilt, and the risk is that it ships as an accessor the follow-up then has to change anyway.

There is also a semantic cost to putting it on ScriptHookResult. .blocked and .succeeded are properties of the hook run, decided the same way for every event. "No verdict, therefore deny" is a property of the PreToolUse gate, not of the run — postToolUse and userPromptSubmit see the identical result object and correctly keep warning. Naming it no_verdict on the result would read as if the direction were universal, which is the confusion this PR exists to remove.

The definition is one branch condition in one closure, and the tests pin all three failure shapes plus the exit-0 case, so a future second gate has an unambiguous thing to copy or extract when its own shape is known. Extraction is worth doing in the PR that adds the second caller — tracked as follow-up on #7339.

[iamwhatever]

• Watch — Fixes #7339 would auto-close the only tracker for the still-fail-open autonomous path; file the follow-up before merge — fixed

Filed as #7547 and linked from the PR body as Related: (not a closing keyword), so merging this closes #7339 and leaves #7547 open. It carries the whole remaining half: fire_tool_hooks fires after the tool is already running and discards its results, so closing it is a control-flow change to the autonomous runtime — the hook has to move ahead of execution and a refusal has to thread back through a call site that has already emitted its auto_approved audit record — not another classification branch. #7547 also carries the per-hook direction switch and the verdict-predicate hoist, and states its own done-when conditions.

[iamwhatever]

• Watch — the exit-1 breadth flip ships with no opt-out; land the per-hook switch in the same release or flag it for the release editor — accepted-and-deferred

Taking the review's stated minimum: the flag for the release editor. It is recorded on this PR (the What changed section states the flip in terms of who it affects — an existing warn-style preToolUse hook that exits 1 now blocks) and again in #7547, which owns the switch itself with a Due: 2026-09-30, the follow-up label and an assignee. CHANGELOG.md cannot carry it from here: check_changelog_history.py rejects a section written for an unreleased version, so the file is written at release time and this is the form the flag can take before then.

The switch is not folded in because it is a config surface — a new per-hook field, its validation, its persistence in hooks.json, and its UI on the Hooks page — attached to a fix whose whole diff is one branch plus tests. Shipping the fail-closed default first is also the safe ordering: the default is the security-relevant half, and the opt-out only ever weakens it, so a release that has the default without the switch is strictly safer than one that has neither. Whether to hold the release for #7547 is a maintainer call, and the flag is here to let it be made.

[iamwhatever]

• Suggestion — hoist the 0/2-verdict interpretation onto ScriptHookResult (e.g. a verdict property in hooks.py) so the PreToolUse script hooks fail open: a deny hook that times out, crashes, or is missing is silently approved #7339 follow-up can reuse it — rebutted (disproportional), repeat of the adjudicated point on c13facc47401

Unchanged reasoning, and the round has not added a caller: the predicate still has exactly one, and the second one is in #7547, whose control flow is not built. The reason it cannot be satisfied by a shared accessor is the same reason that work is a separate issue — fire_tool_hooks runs after the tool has started, so the follow-up's problem is ordering, not reading the same boolean, and a property fixed now would be shaped against a consumer that does not exist.

The naming objection also stands, and this round's wording (verdict on the result) sharpens it: "no verdict, therefore deny" is a property of the PreToolUse gate, not of the hook run. postToolUse and userPromptSubmit receive the identical ScriptHookResult and correctly keep warning, so a verdict property that implies a universal direction would reintroduce exactly the conflation this PR removes. .blocked and .succeeded are safe on the result because they are per-run facts; the direction is not.

#7547 names the hoist explicitly as part of its done-when, so the extraction is tracked rather than dropped — it just belongs in the PR that brings the second gate.

[bolichen97]

Open PR relationship audit

This is a consolidated, point-in-time code-level audit note. It compares complete merge-base diffs and current/merged code; it does not treat a shared topic as duplication or partial coverage as completion.

Relationship findings

• PR #7843 is PARTIALLY_COVERED relative to this PR. Coverage is explicitly incomplete; this finding is not a completion or closure claim. Recommended action for PR #7843: CONTINUE_DEVELOPMENT. PR #7422 landed the fail-closed rule on only the dashboard call site. The autonomous subagent and task-runner gates that are this PR's stated purpose are still absent from main, so the PR is not superseded and must not be closed as done; it needs the CI failures, the missing tests for the new gates and fail_open field, and the EVENT_TOOL_CALL enforcement gap resolved. Files: src/kiro_crew/dashboard/chat_runner.py, src/kiro_crew/subagent_manager/run.py, src/kiro_crew/task_executor.py, src/kiro_crew/hooks.py.

No PR, Issue, label, branch, or review state was changed by the relationship-note portion of this audit.