feat: run ToolCallBefore hooks before tool execution

[aboimpinto]

Summary

Phase 2 of the custom slash command lifecycle / hooks architecture work, stacked on top of Phase 1 (PR #2326).

This PR adds the first hook boundary at tool execution time:

• the hook executor is passed into engine turns
• ToolCallBefore hooks receive the tool name, args, mode, workspace, model, and session id before the tool runs
• a hook exit code of 2 denies the tool call and returns the hook output as the rejection reason

This is the hook-gate slice. The Phase 1 frontmatter/allowed-tools layer (PR #2326) is already merged. Pause/resume/cancel lifecycle work remains separate for Phase 3 on feat/allowed-tools-enforcement.

Scope

• Pass hook executor into engine turns
• ToolCallBefore hook interface with tool metadata
• Hook exit code 2 denies tool calls
• Hook output returned as rejection reason

Not in this slice

• Generic PreToolUse/PostToolUse lifecycle hook model
• PostToolUse-style receipts
• Pause/resume/cancel lifecycle
• Durable continuity/orientation-cache storage
• User-configurable hooks

Builds on

PR #2326 (Phase 1 — feat: enforce allowed tools for custom commands) — merged

Issues

Refs #1917 (EPIC: universal PreToolUse/PostToolUse hook layer)
Refs #1891, #1900
Part of #1894, #1895

Validation

cargo check -p codewhale-tui --bin codewhale-tui
cargo test -p codewhale-tui --bin codewhale-tui hook_gate -- --nocapture
cargo test -p codewhale-tui --bin codewhale-tui allowed_tools -- --nocapture
cargo test -p codewhale-tui --bin codewhale-tui user_command -- --nocapture

Paulo Aboim Pinto

Greptile Summary

This PR introduces the ToolCallBefore hook gate at tool-execution time (Phase 2 of the custom slash command lifecycle architecture). Hooks configured for ToolCallBefore now receive tool metadata before dispatch and can deny the call by exiting with code 2; the hook executor is offloaded via spawn_blocking to avoid stalling the Tokio runtime.

• Adds hook_executor to EngineConfig and Op::SendMessage, seeds it for fresh sessions in run_tui, and plumbs it through dispatch_user_message and build_engine_config.
• Removes the observer-only ToolCallBefore firing from tool_routing.rs to eliminate double-execution; the gate in the turn-loop planning sequence now serves as the single hook execution site.
• Adds has_background_hooks_for_event() to HookExecutor and emits a tracing::warn! when background hooks (which can never produce an exit code) are configured for ToolCallBefore.

Confidence Score: 5/5

Safe to merge — the gate logic is well-ordered, the blocking syscall is correctly offloaded, and the fresh-session hook_executor initialization fix removes a silent no-op regression.

All three previously flagged defects (double-firing, blocking call in async context, hook_executor None for new sessions) are addressed in this revision. The remaining finding is a per-call log-noise nit that doesn't affect correctness or runtime behavior.

No files require special attention; turn_loop.rs has the most logic but it reads cleanly and tests cover the main exit-code paths.

Important Files Changed

Filename	Overview
crates/tui/src/core/engine/turn_loop.rs	Adds ToolCallBefore hook gate with spawn_blocking offload; ordering is correct (gate fires after all earlier block checks). Per-call background-hook warning can be noisy.
crates/tui/src/tui/ui.rs	Seeds hook_executor for fresh sessions before rebuilding RuntimeToolServices and plumbs it into both EngineConfig and Op::SendMessage; correctly handles new-session and resume paths.
crates/tui/src/tui/tool_routing.rs	Removes observer-only ToolCallBefore firing to eliminate double-hook-execution; comment explains the rationale clearly.
crates/tui/src/hooks.rs	Adds has_background_hooks_for_event() helper with correct enabled-guard and background flag check; well-documented.
crates/tui/src/core/engine.rs	Adds hook_executor field to EngineConfig and Default; threads it through handle_send_message and auto-continue correctly.
crates/tui/src/core/ops.rs	Adds hook_executor to Op::SendMessage variant; propagation is consistent with allowed_tools pattern.
crates/tui/src/main.rs	Adds hook_executor: None to exec-agent EngineConfig; intentional — exec agent is out of scope for this hook slice.
crates/tui/src/runtime_threads.rs	Sets hook_executor: None in two RuntimeThreadManager Op::SendMessage constructions; consistent with exec-agent exclusion.
crates/tui/src/utils.rs	Minor Clippy fix: removes unnecessary early return inside a cfg(windows) block.

Sequence Diagram

sequenceDiagram
    participant Model as Model (API stream)
    participant TurnLoop as Engine::handle_deepseek_turn
    participant Gate as ToolCallBefore Gate
    participant Hooks as HookExecutor (spawn_blocking)
    participant ToolExec as Tool Execution

    Model->>TurnLoop: ToolCallStarted(name, input)
    TurnLoop->>Gate: check plan-mode block
    TurnLoop->>Gate: check command_allows_tool (allowed_tools list)
    TurnLoop->>Gate: check caller_allowed_for_tool
    TurnLoop->>Gate: check tool_def exists
    TurnLoop->>Hooks: execute(ToolCallBefore, context)
    Hooks-->>TurnLoop: "Vec<HookResult>"
    alt "exit_code == Some(2)"
        TurnLoop-->>Model: "blocked_error = permission_denied(reason)"
    else "exit_code == Some(0) or None"
        TurnLoop->>ToolExec: dispatch tool call
        ToolExec-->>TurnLoop: ToolResult
    end

Loading

Comments Outside Diff (1)

1. crates/tui/src/core/engine/turn_loop.rs, line 1303-1308 (link)

   caller_allowed_for_tool unconditionally overwrites a hook-set denial reason

   This check does not guard against an already-set blocked_error. If a ToolCallBefore hook denies the tool (setting blocked_error) and then caller_allowed_for_tool also returns false, the hook's human-readable denial message is silently replaced by the generic "does not allow caller" message. The tool is still blocked, but the error shown to the user is misleading. Adding && blocked_error.is_none() to the condition (as the hook gate already does) preserves the hook's reason when it fires first.

   

_{Reviews (5): Last reviewed commit: "fix: clippy needless_return and fmt comp..." | Re-trigger Greptile}

[gemini-code-assist]

Code Review

This pull request introduces a control-plane hook mechanism ('ToolCallBefore') that allows external hooks to intercept and potentially deny tool calls (with exit code 2) before execution. This is integrated into the TUI engine configuration, turn loop, and message dispatching, and is accompanied by comprehensive unit tests. The feedback highlights two key improvement opportunities: first, optimizing the turn loop by validating tool existence and caller authorization before executing hooks to prevent unnecessary and potentially unsafe hook executions; second, storing the hook executor as an 'Arc' inside the 'App' struct to avoid redundant cloning and heap allocations.

[Hmbown]

Thanks @aboimpinto. I did a quick v0.8.50 triage read. This is useful Phase 2 hooks work, but I am not harvesting it into #2504: it is broader control-plane behavior and the current patch still has at least one ordering issue to resolve before it is release-safe.

The key blocker is hook execution order. ToolCallBefore currently runs before caller_allowed_for_tool(...) and before the unknown-tool / missing-tool checks finish. That means model-emitted invalid or unauthorized tool names can still trigger user hooks. Please move the hook gate after the normal tool existence, caller, mode, and allowed-tools validation has established that this is a real executable tool call. Hooks should refine/deny a valid call, not be the first consumer of untrusted tool names and args.

I would also keep the executor as shared app state instead of rebuilding Arc::new(app.hooks.clone()) on each engine config/send path, but the validation order is the important correctness issue. Once that is fixed and CI/Greptile settle, this can stay on the hooks track rather than the v0.8.50 release harvest.

[aboimpinto]

Thanks for the detailed review @Hmbown.

[aboimpinto]

@Hmbown the hook ordering concern is resolved in the current code (commit 5e520c6). Here's the current execution order in turn_loop.rs:

[aboimpinto]

@Hmbown all review comments are resolved and all 10 CI checks are green on the latest commit (54c8aa5). Could you please harvest that build instead of the earlier one? The greptile P1 (fresh-session hook_executor being None) and the clippy lint have both been addressed.

[Hmbown]

Thanks @aboimpinto. I re-reviewed the latest green #2511 head (54c8aa5) and harvested it into the v0.8.50 triage branch #2504.

Harvest commits on #2504:

• 242899d4b from upstream c3cca6bd3 - ToolCallBefore hook gate
• 796e95caa from upstream 98e175075 - validation-order review fixes
• 2622db493 from upstream 8616b3736 - fmt compliance
• cc923d634 from upstream 5e520c6b5 - remove double-fire and offload blocking hook execution
• 77b57bd90 from upstream 37c0f2b6c - initialize hook executor for fresh sessions
• 2ca292765 from upstream 54c8aa50b - clippy/fmt cleanup

Local verification on the updated #2504 branch:

• cargo test -p codewhale-tui --all-features --locked hook_gate
• cargo test -p codewhale-tui --all-features --locked allowed_tools
• cargo test -p codewhale-tui --all-features --locked user_command
• cargo clippy -p codewhale-tui --all-targets --all-features --locked -- -D warnings

The branch is pushed; #2504 CI is the release-branch proof before merge.