fix: address greptile review comments — remove double-firing, wrap blocking execute in spawn_blocking
- Remove ToolCallBefore observer firing from tool_routing.rs (the turn-loop gate now handles it) to prevent double-firing hooks for each tool call (greptile P1). - Wrap hook_executor.execute() call in tokio::task::spawn_blocking so the Tokio worker thread is not blocked by child.wait_timeout() during hook execution (greptile P1).
This commit is contained in:
committed by
Hunter Bown
parent
2622db4935
commit
cc923d634c
@@ -1309,8 +1309,18 @@ impl Engine {
|
||||
.with_workspace(self.session.workspace.clone())
|
||||
.with_model(&self.config.model)
|
||||
.with_session_id(&self.session.id);
|
||||
let hook_results = hook_executor
|
||||
.execute(crate::hooks::HookEvent::ToolCallBefore, &hook_context);
|
||||
// Run hooks off the Tokio worker thread: `execute()` calls
|
||||
// `child.wait_timeout()` which is a blocking syscall that
|
||||
// would stall all other async tasks on this thread.
|
||||
let executor = hook_executor.clone();
|
||||
let hook_results = tokio::task::spawn_blocking(move || {
|
||||
executor.execute(crate::hooks::HookEvent::ToolCallBefore, &hook_context)
|
||||
})
|
||||
.await
|
||||
.unwrap_or_else(|join_err| {
|
||||
tracing::error!("Hook executor task panicked: {join_err}");
|
||||
Vec::new()
|
||||
});
|
||||
if let Some(denial) = hook_results
|
||||
.iter()
|
||||
.find(|result| result.exit_code == Some(2))
|
||||
|
||||
@@ -22,19 +22,11 @@ pub(super) fn handle_tool_call_started(
|
||||
name: &str,
|
||||
input: &serde_json::Value,
|
||||
) {
|
||||
// #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.
|
||||
// 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)
|
||||
.with_tool_args(input);
|
||||
let _ = app.execute_hooks(HookEvent::ToolCallBefore, &context);
|
||||
}
|
||||
// #2511: ToolCallBefore gate moved to turn-loop planning loop
|
||||
// (Engine::handle_deepseek_turn). Removing observer-only firing
|
||||
// here to avoid double-firing hooks for each tool call.
|
||||
// Hooks that need observation can configure ToolCallBefore on
|
||||
// the turn-loop gate — it processes the denial (exit code 2).
|
||||
|
||||
let id = id.to_string();
|
||||
|
||||
|
||||
Reference in New Issue
Block a user