perf(hooks): fast-path skip when no hooks configured (#455 follow-up)
Now that `tool_call_before` / `tool_call_after` fire on every tool dispatch, the cost of constructing a `HookContext` (which allocates for `workspace`, `model`, `session_id`, …) shows up on the hot path even when the user has zero hooks configured — the common case. Adds `HookExecutor::has_hooks_for_event(event)` as a cheap boolean gate that callers consult before building the context. The pre-check returns false when: * `config.enabled == false` (globally disabled). * No hook in the config has the given `event`. Wired through every fire site: * `tool_routing.rs::handle_tool_call_started` — `ToolCallBefore`. * `tool_routing.rs::handle_tool_call_complete` — `ToolCallAfter`. Also skips the `result.content.clone()` that the `with_tool_result` builder demands. * `ui.rs::dispatch_user_message` — `MessageSubmit`. * `ui.rs::apply_engine_error_to_app` — `OnError`. Inside `HookExecutor::execute` itself, also short-circuit before calling `context.to_env_vars()` when no hooks match the event — defends against a caller that builds the context but forgets to gate. Tests: 3 new tests cover empty-config / globally-disabled / per-event filtering. The existing 18 hook tests pass unchanged. No behavioral change for users with hooks configured; pure allocation-free fast path otherwise.
This commit is contained in:
+5
-1
@@ -266,7 +266,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
|
||||
because it needs a synchronous-gate contract that doesn't
|
||||
exist today. Combined with the existing `session_start` /
|
||||
`session_end` / `mode_change` events, every variant in the
|
||||
`HookEvent` enum now has a live producer.
|
||||
`HookEvent` enum now has a live producer. Each fire is
|
||||
fast-path-gated by
|
||||
`HookExecutor::has_hooks_for_event(event)` so per-tool
|
||||
dispatch never pays for `HookContext` allocation when the
|
||||
user has no hooks configured (the common case).
|
||||
- **RLM tool family** (#512) — `rlm` tool cards map to
|
||||
`ToolFamily::Rlm` and render `rlm`, not `swarm`. Stale "swarm"
|
||||
wording cleaned out of docs / comments / tests.
|
||||
|
||||
@@ -481,6 +481,17 @@ impl HookExecutor {
|
||||
&self.session_id
|
||||
}
|
||||
|
||||
/// Cheap pre-check: are there any enabled hooks for this event?
|
||||
/// Lets call sites avoid building a [`HookContext`] (which allocates
|
||||
/// for `workspace`, `model`, `session_id`, …) on every tool call
|
||||
/// when the user hasn't configured any hooks. The cost matters
|
||||
/// because `ToolCallBefore` / `ToolCallAfter` fire from
|
||||
/// `tool_routing.rs` on every tool dispatch (#455).
|
||||
#[must_use]
|
||||
pub fn has_hooks_for_event(&self, event: HookEvent) -> bool {
|
||||
self.config.enabled && self.config.hooks.iter().any(|h| h.event == event)
|
||||
}
|
||||
|
||||
/// Execute all hooks for an event
|
||||
pub fn execute(&self, event: HookEvent, context: &HookContext) -> Vec<HookResult> {
|
||||
if !self.config.enabled {
|
||||
@@ -488,6 +499,14 @@ impl HookExecutor {
|
||||
}
|
||||
|
||||
let hooks = self.config.hooks_for_event(event);
|
||||
if hooks.is_empty() {
|
||||
// Fast path: no hooks for this event → skip the
|
||||
// `context.to_env_vars()` HashMap allocation. With
|
||||
// `tool_call_before` / `tool_call_after` firing per-tool
|
||||
// (#455) this allocation would otherwise happen on every
|
||||
// tool dispatch even for users with zero hooks configured.
|
||||
return Vec::new();
|
||||
}
|
||||
let env_vars = context.to_env_vars();
|
||||
let mut results = Vec::new();
|
||||
|
||||
@@ -820,4 +839,58 @@ mod tests {
|
||||
assert!(executor.session_id().starts_with("sess_"));
|
||||
assert_eq!(executor.session_id().len(), 13); // "sess_" + 8 chars
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn has_hooks_for_event_fast_path_returns_false_for_empty_config() {
|
||||
let executor = HookExecutor::disabled();
|
||||
// No hooks configured AT ALL — every event is a fast skip.
|
||||
for event in [
|
||||
HookEvent::SessionStart,
|
||||
HookEvent::SessionEnd,
|
||||
HookEvent::MessageSubmit,
|
||||
HookEvent::ToolCallBefore,
|
||||
HookEvent::ToolCallAfter,
|
||||
HookEvent::ModeChange,
|
||||
HookEvent::OnError,
|
||||
] {
|
||||
assert!(
|
||||
!executor.has_hooks_for_event(event),
|
||||
"empty config must short-circuit for {event:?}"
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn has_hooks_for_event_returns_false_when_globally_disabled() {
|
||||
let config = HooksConfig {
|
||||
enabled: false,
|
||||
hooks: vec![Hook::new(HookEvent::ToolCallBefore, "echo blocked")],
|
||||
..HooksConfig::default()
|
||||
};
|
||||
let executor = HookExecutor::new(config, PathBuf::from("."));
|
||||
assert!(
|
||||
!executor.has_hooks_for_event(HookEvent::ToolCallBefore),
|
||||
"globally-disabled hooks must report no fires even when one is configured"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn has_hooks_for_event_distinguishes_event_types() {
|
||||
let config = HooksConfig {
|
||||
enabled: true,
|
||||
hooks: vec![
|
||||
Hook::new(HookEvent::SessionStart, "echo start"),
|
||||
Hook::new(HookEvent::ToolCallBefore, "echo before"),
|
||||
],
|
||||
..HooksConfig::default()
|
||||
};
|
||||
let executor = HookExecutor::new(config, PathBuf::from("."));
|
||||
// Configured events return true.
|
||||
assert!(executor.has_hooks_for_event(HookEvent::SessionStart));
|
||||
assert!(executor.has_hooks_for_event(HookEvent::ToolCallBefore));
|
||||
// Unconfigured events return false even when other events are present.
|
||||
assert!(!executor.has_hooks_for_event(HookEvent::ToolCallAfter));
|
||||
assert!(!executor.has_hooks_for_event(HookEvent::OnError));
|
||||
assert!(!executor.has_hooks_for_event(HookEvent::ModeChange));
|
||||
}
|
||||
}
|
||||
|
||||
@@ -24,10 +24,10 @@ pub(super) fn handle_tool_call_started(
|
||||
// #455 (observer-only): fire `tool_call_before` hooks here, before
|
||||
// any UI bookkeeping. Hooks are read-only observers in this slice
|
||||
// — they can log, notify, or audit, but cannot mutate the args.
|
||||
// Mutation support is tracked separately and stays a v0.8.9
|
||||
// follow-up because it requires a synchronous gate contract that
|
||||
// doesn't exist today.
|
||||
{
|
||||
// Fast-path skip when no hooks are configured so per-tool
|
||||
// dispatch doesn't pay for context construction in the common
|
||||
// case (most users have no hooks).
|
||||
if app.hooks.has_hooks_for_event(HookEvent::ToolCallBefore) {
|
||||
let context = app
|
||||
.base_hook_context()
|
||||
.with_tool_name(name)
|
||||
@@ -518,16 +518,20 @@ pub(super) fn handle_tool_call_complete(
|
||||
// result has settled. Hooks see tool_name + the result content
|
||||
// (or error message) + success flag. Read-only — they cannot
|
||||
// mutate the result that goes back to the model. Mutation
|
||||
// remains a v0.8.9 follow-up.
|
||||
let (result_text, success): (String, bool) = match result.as_ref() {
|
||||
Ok(tool_result) => (tool_result.content.clone(), tool_result.success),
|
||||
Err(err) => (err.to_string(), false),
|
||||
};
|
||||
let context = app
|
||||
.base_hook_context()
|
||||
.with_tool_name(name)
|
||||
.with_tool_result(&result_text, success, None);
|
||||
let _ = app.execute_hooks(HookEvent::ToolCallAfter, &context);
|
||||
// remains a v0.8.9 follow-up. Fast-path skip avoids the
|
||||
// result.content.clone() and HookContext allocation when no
|
||||
// hooks are configured.
|
||||
if app.hooks.has_hooks_for_event(HookEvent::ToolCallAfter) {
|
||||
let (result_text, success): (String, bool) = match result.as_ref() {
|
||||
Ok(tool_result) => (tool_result.content.clone(), tool_result.success),
|
||||
Err(err) => (err.to_string(), false),
|
||||
};
|
||||
let context = app
|
||||
.base_hook_context()
|
||||
.with_tool_name(name)
|
||||
.with_tool_result(&result_text, success, None);
|
||||
let _ = app.execute_hooks(HookEvent::ToolCallAfter, &context);
|
||||
}
|
||||
}
|
||||
|
||||
/// Build a finalized standalone history cell for a tool completion whose
|
||||
|
||||
@@ -2578,7 +2578,11 @@ pub(crate) fn apply_engine_error_to_app(
|
||||
// #455 (observer-only): fire `on_error` hooks so operators can
|
||||
// page on auth / billing / invalid-request failures without
|
||||
// tailing the audit log. Read-only — the hook can react but not
|
||||
// suppress the error from reaching the transcript.
|
||||
// suppress the error from reaching the transcript. Fast-path
|
||||
// skip when no hooks configured.
|
||||
if app
|
||||
.hooks
|
||||
.has_hooks_for_event(crate::hooks::HookEvent::OnError)
|
||||
{
|
||||
let context = app.base_hook_context().with_error(&message);
|
||||
let _ = app.execute_hooks(crate::hooks::HookEvent::OnError, &context);
|
||||
@@ -2894,6 +2898,10 @@ async fn dispatch_user_message(
|
||||
// dispatch. Hooks see the user's display text via the
|
||||
// `with_message` builder. Read-only — they can log, audit, or
|
||||
// notify but cannot mutate the message that goes to the engine.
|
||||
// Fast-path skip when no hooks configured.
|
||||
if app
|
||||
.hooks
|
||||
.has_hooks_for_event(crate::hooks::HookEvent::MessageSubmit)
|
||||
{
|
||||
let context = app.base_hook_context().with_message(&message.display);
|
||||
let _ = app.execute_hooks(crate::hooks::HookEvent::MessageSubmit, &context);
|
||||
|
||||
Reference in New Issue
Block a user