Add hook system for tool execution lifecycle

[iamnbutler]

Summary

• Adds a ToolHook trait and HookRegistry to crates/agent/ for customizing tool execution at three lifecycle points: pre-execution, post-execution, and post-failure
• Adds execute_tool_calls_with_hooks() that wraps the existing execute_tool_calls() with hook support, falling back to the fast path when no hooks are registered
• Includes HookScope for filtering hooks to specific tools (All, Tools, Except)
• 20 new tests covering all hook types, scope filtering, chaining, and accumulation

Hook capabilities

Lifecycle point	Can do
PreToolUse	Block execution, modify input
PostToolUse	Modify output, add context for model, stop query loop
PostToolUseFailure	Replace error message, request single retry with modified input

Use cases

1. Validation — Block dangerous operations (e.g., rm -rf /)
2. Logging — Log all tool calls for debugging
3. Transformation — Normalize file paths, sanitize outputs
4. Attribution — Track file edits for commit messages
5. Rate limiting — Throttle expensive operations

Test plan

• All 71 agent crate tests pass (20 new hook tests)
• Full workspace compiles cleanly (cargo check --workspace)
• Verify hook integration works when wired into orchestrator (future PR)

Closes #669

🤖 Generated with Claude Code

[github-actions]

This PR adds a clean, well-tested hook system for the tool execution lifecycle in crates/agent. The design — ToolHook trait, HookRegistry, HookScope, and the three lifecycle points (pre/post/failure) — is solid and the test coverage is thorough. All 71 tests pass. The clippy error in chain.rs is pre-existing and not introduced by this PR.

Two suggestions worth addressing before the hooks are wired into the actual session/chain execution path:

1. run_post_hooks doesn't short-circuit on StopLoop (inline comment on hooks.rs:254)

Pre-hooks stop on Block; post-hooks don't stop on StopLoop. This asymmetry means hooks registered after a StopLoop hook still run, and any accumulated_context gathered before the stop is silently dropped. The right fix depends on intended semantics — either short-circuit like Block does, or document the behavior explicitly.

2. Retry path skips post-hooks (inline comment on tool_exec.rs:163)

On FailureHookAction::Retry, the retry result is pushed without running post-hooks. If the retry succeeds, post-hooks never process the output. Worth documenting if intentional.

---

References:

• §23809754774

Reviewed by PR / Review

[github-actions]

[SUGGESTION]

Priority: Code Quality / Correctness

run_post_hooks doesn't short-circuit after a StopLoop action — the loop continues and subsequent hooks keep processing current_output. This is asymmetric with run_pre_hooks, which returns early on Block.

The consequence is two-fold:

1. A hook registered after the StopLoop hook can still modify the output (undocumented, surprising to callers).
2. Any accumulated_context collected before the StopLoop is silently dropped (line 253–254 discards it). If hook A adds context and hook B later emits StopLoop, the context disappears with no indication.

If the intended semantics are "first hook to request a stop wins and owns the output", the fix is a break (or return early) when StopLoop is encountered:

PostHookAction::StopLoop(output) => {
    return PostHookAction::StopLoop(output); // short-circuit like Block does
}

If hooks after StopLoop should still run (observable side-effects), that should be documented and the silent context-drop should be preserved deliberately with a comment explaining why.

[github-actions]

[SUGGESTION]

Priority: Correctness

The retry path executes the retry call and pushes the result directly, without running post-hooks (success or failure) on it:

FailureHookAction::Retry(retry_input) => {
    let retry_call = ToolCall { ... };
    let retry_result = executor(retry_call).await;
    results.push(retry_result);  // no post-hooks applied
}

If the retry succeeds, post-hooks never see the output. If it fails again, failure hooks don't get a second chance (one retry, silently drops). Whether this is intentional should be documented. If it is intentional (keep it simple, single retry, raw result), a comment explaining the decision would prevent future confusion.

[iamnbutler]

Orchestrator Evaluation: Rejected

The PR has merge conflicts (mergeable: No) and CI is failing, which are immediate blockers. Beyond that, the implementation is solid in terms of what it delivers — a well-designed hook trait, registry, scope filtering, and integration with the existing tool execution pipeline — but there are notable gaps:

1. Not integrated into Session: The issue proposal shows hooks being integrated into Session (via add_hook and execute_tool_with_hooks), but the actual Session type is not modified in this PR. The execute_tool_calls_with_hooks function exists but nothing in the codebase calls it yet. The hooks module is purely additive infrastructure with no actual integration point.

2. Synchronous trait methods: The issue proposal shows async hook methods, and the ToolHook trait methods are synchronous. While this is a reasonable simplification for v1, it limits use cases like rate limiting (mentioned in the issue) which would naturally be async.

3. No tests for execute_tool_calls_with_hooks: The new function in tool_exec.rs has no tests. All tests are for the hooks module itself (HookScope, HookRegistry), but the integration function that actually wires hooks into tool execution is untested.

4. tools parameter unused in execute_tool_calls_with_hooks: When hooks are present, the function serializes execution (no batching), and the tools parameter is only used in the hooks.is_empty() fast path via execute_tool_calls. This means concurrency-safe batching is completely lost when any hooks are registered, even if hooks only apply to specific tools.

5. Merge conflicts and CI failure are hard blockers regardless of code quality.

---

Feedback for agent:
This PR cannot be merged due to:

1. Merge conflicts — the PR is not mergeable in its current state.
2. CI is failing — tests/checks are not passing.

Additional code-level feedback:

3. No Session integration: The hook system is defined but never wired into the Session's tool execution flow. The issue specifically calls for Session::add_hook and integration with the session's execution loop. Consider adding a HookRegistry field to Session and having the session's tool execution path use execute_tool_calls_with_hooks.

4. No tests for execute_tool_calls_with_hooks: This is the key integration function and has zero test coverage. Add tests covering: pre-hook blocking, input modification flowing to executor, post-hook output modification, failure hook retry, the should_stop flag, and the fast path when hooks are empty.

5. Lost concurrency when hooks are registered: When hooks exist, all tools execute serially even if only some tools have matching hooks. Consider preserving batch execution for tools that no hooks match, or at minimum document this trade-off.

6. tools parameter is unused when hooks are active: In execute_tool_calls_with_hooks, the tools parameter is only used in the empty-hooks fast path. This is misleading — either use it for batching decisions or remove it from the hooked path's signature.