docs(hooks): document fail-closed PreToolUse exit contract (#7744)

[CrysisDeu]

Summary

PR #7422 changed the PreToolUse script-hook exit-code contract: any exit that is neither 0 nor 2 now BLOCKS the tool (fail closed), where it previously only warned. The user-facing hooks reference and the ScriptHook docstring still described the old warn-only rule — the opposite of shipped behaviour. This PR is documentation + test hardening only; no runtime behaviour changes.

• docs/reference/kiro-cli/hooks.md — the Hook output bullets and the PreToolUse section now state the shipped contract: exit 0 = allow (stdout injected), exit 2 = deny (stderr returned to the LLM), and any other exit (timeout, crash, unexecutable command) blocks the tool, with the block detail preferring result.error, then stderr, then exited with code N. Non-gating events (postToolUse, userPromptSubmit, stop) keep warn-only semantics. Added a plain note that there is no per-hook advisory/fail-open opt-out yet (tracked in PreToolUse hooks cannot deny on the subagent/task-runner paths (fire_tool_hooks is informational) #7547), and a clause that on non-gating events an exit 2 is not a gate (its marker is surfaced as injected text — matches the exit-2 branch in chat_runner.py, which has no event check).
• src/kiro_crew/hooks.py — the ScriptHook dataclass docstring now states the same contract instead of the pre-fix(hooks): fail closed when a PreToolUse hook delivers no verdict #7422 "Other: warning" rule.
• test/test_dashboard_approval.py — the two tests added by fix(hooks): fail closed when a PreToolUse hook delivers no verdict #7422 built their hook results with bare MagicMock, so a renamed production field would silently keep passing. They now construct real ScriptHookResult instances, so a field rename fails loudly as a TypeError. The _blocking_hook_store helper (same bare-mock pattern, same file) was hardened the same way. Note: MagicMock(spec=...) / spec_set=... was evaluated first and rejected — dataclass fields without defaults (hook_id, hook_name, event) are not class attributes, so mock's spec machinery rejects setting them; real dataclass instances are strictly stronger.

Every doc claim was verified against the runtime implementation (_fire() in src/kiro_crew/dashboard/chat_runner.py, the elif r.exit_code not in (0, 2) fail-closed branch).

Testing

• isort --check-only src/kiro_crew test — pass
• flake8 src/kiro_crew test — pass
• mypy src/kiro_crew/ — pass (no issues in 1279 files)
• scripts/check_black_formatting.py — pass (post-commit, diff-scoped)
• scripts/check_brand_name.py — pass (exit 0, no added-line hits)
• All four ScriptHookResult constructions used by the hardened tests verified by direct instantiation (timeout/crash/exit-127/exit-0 kwarg sets), plus a negative check that an unknown field raises TypeError
• pytest runs in CI (tests are not run locally on this host by policy)

Pre-push review

Two model-pinned read-only review lanes on the staged diff: GPT lane PASS (no findings); Opus lane PASS (0 blocking, 2 Low advisories — both applied in this commit).

Closes #7744

[github-actions]

Design Review (Fable 5) — ✅ PASS

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

Docs match the runtime (chat_runner.py:5311 fail-closed branch and the error → stderr → exit code detail preference are exactly as documented), the description accounts for every hunk bidirectionally, and no runtime behavior changes. This is a sound docs-truth + test-hardening PR fixing documentation that stated the opposite of shipped, security-relevant behavior.

Design-Verdict: PASS

Docs and docstring now match the shipped fail-closed gate they had contradicted; test hardening is strictly stronger than the bare mocks it replaces.

[DESIGN-REVIEWED] 5304ad1

[github-actions]

Opus 4.8 Review — ✅ no blocking findings

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

Review details

No findings.

[OPUS-REVIEWED] 5304ad1

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

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

[github-actions]

First Principles Review (Fable 5) — ✅ PASS

Premise-level review of 5304ad1828d1f66ad5b7e542daecb42756e0d31a — 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 verified against the runtime. Every documented behavior matches the code at chat_runner.py:5283-5349, no stale sibling docs remain, and no other bare-mock ScriptHookResult sites exist. Final review:

First-Principles-Verdict: PASS

Docs said the opposite of shipped fail-closed behavior; every hunk here either corrects that or pins the contract the docs now describe.

What this change ships

Intent: make the hooks reference and docstring stop describing the pre-#7422 warn-only rule that shipped behavior contradicts — a FIX.

1. Hooks reference now says a failed/timed-out/crashed PreToolUse hook blocks the tool — justified (verified against chat_runner.py:5311-5335, including the detail preference error → stderr → exited with code N)
2. Reference clause: exit 2 on non-gating events injects a marker, doesn't gate — justified (the exit-2 branch at chat_runner.py:5294 has no event check)
3. Reference note: no per-hook fail-open opt-out yet (PreToolUse hooks cannot deny on the subagent/task-runner paths (fire_tool_hooks is informational) #7547) — justified, states current behavior
4. ScriptHook docstring states the same contract — justified
5. Three test sites build real ScriptHookResults instead of bare MagicMocks — declared rider; hardens the tests of exactly this contract

Watch

Item 5 rides along in a docs fix, but it is declared, targets the same contract the docs now describe, and leaves no siblings: grepped MagicMock\(.*exit_code and MagicMock\(.*hook_name under test/ — 0 remaining hook-result mocks (the returncode= hits in test_cli.py mock subprocess.run, a different type). The internal spec (docs/system-specs/modules/learn-cron-dashboard.md:1958) already carries the shipped contract, so the two edited files were the last stale copies — nothing left unfixed.

[FIRST-PRINCIPLES-REVIEWED] 5304ad1

[github-actions]

GPT 5.6 Review — ✅ no blocking findings

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

This comment is updated in place on each push.

Review details

FINDING -- src/kiro_crew/hooks.py:3471 -- "PreToolUse BLOCKS the tool" contradicts notification-only callers that fire after an auto-approved tool starts and discard the result -> Fix: qualify this as the dashboard permission-gating contract. (origin: validation)
[GPT-REVIEWED] 5304ad1

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

[bolichen97]

Docs now match shipped behavior, and I verified each claim against the runtime rather than the PR text: chat_runner.py's elif r.exit_code not in (0, 2) branch gates its block on event == HOOK_EVENT_PRE_TOOL_USE, and its detail is exactly r.error or r.stderr or f"exited with code {r.exit_code}" — the precedence the reference now documents. The subtler claim also holds: the exit_code == 2 branch above it has no event check, so stating that an exit 2 on a non-gating event surfaces as injected text rather than denying anything is accurate, and that was the easiest thing to get wrong here. The ScriptHook docstring correcting the stale pre-#7422 "Other: warning" rule closes the last place the old contract was still asserted. The test change is a genuine hardening rather than churn: building real ScriptHookResult instances makes a production field rename fail loudly as a TypeError, where the bare MagicMocks would have kept passing while configuring a stale attribute — and the note explaining why spec=/spec_set= cannot substitute (no-default dataclass fields are not class attributes) is correct.