dgf1988/codewhale
1
0
Fork 0
Code Issues Pull Requests Actions 2 Packages Projects Releases Wiki Activity

fix: address PR #2511 review comments

Browse Source
This commit is contained in:
Paulo Aboim Pinto
2026-06-01 17:40:13 +02:00
committed by Hunter Bown
parent 242899d4b6
commit 796e95caa6
3 changed files with 55 additions and 27 deletions
Download Patch File Download Diff File Expand all files Collapse all files
crates/tui/src/core/engine/turn_loop.rs
+36 -22
View File
@@ -1255,16 +1255,50 @@ impl Engine {
)));
}
if !command_allows_tool(self.config.allowed_tools.as_deref(), &tool_name) {
if blocked_error.is_none()
&& !command_allows_tool(self.config.allowed_tools.as_deref(), &tool_name) {
blocked_error = Some(ToolError::permission_denied(format!(
"Tool '{tool_name}' is not in the allowed-tools list for the current command"
)));
}
if blocked_error.is_none()
&& !caller_allowed_for_tool(tool_caller.as_ref(), tool_def) {
blocked_error = Some(ToolError::permission_denied(format!(
"Tool '{tool_name}' does not allow caller '{}'",
caller_type_for_tool_use(tool_caller.as_ref())
)));
}
if blocked_error.is_none()
&& tool_def.is_none()
&& !McpPool::is_mcp_tool(&tool_name)
&& tool_name != CODE_EXECUTION_TOOL_NAME
&& tool_name != JS_EXECUTION_TOOL_NAME
&& !is_tool_search_tool(&tool_name)
{
blocked_error = Some(ToolError::not_available(missing_tool_error_message(
&tool_name,
&tool_catalog,
)));
}
if blocked_error.is_none()
&& let Some(hook_executor) = self.config.hook_executor.as_ref()
&& hook_executor.has_hooks_for_event(crate::hooks::HookEvent::ToolCallBefore)
{
// Warn if any ToolCallBefore hook is configured as background
// — background hooks return exit_code: None immediately, so
// the denial check (exit_code == Some(2)) can never match.
if hook_executor.has_background_hooks_for_event(crate::hooks::HookEvent::ToolCallBefore)
{
tracing::warn!(
"ToolCallBefore hook(s) configured with background=true — \
background hooks cannot deny tool calls because they exit \
immediately with no result"
);
}
let hook_context = crate::hooks::HookContext::new()
.with_tool_name(&tool_name)
.with_tool_args(&tool_input)
@@ -1300,26 +1334,6 @@ impl Engine {
}
}
if !caller_allowed_for_tool(tool_caller.as_ref(), tool_def) {
blocked_error = Some(ToolError::permission_denied(format!(
"Tool '{tool_name}' does not allow caller '{}'",
caller_type_for_tool_use(tool_caller.as_ref())
)));
}
if blocked_error.is_none()
&& tool_def.is_none()
&& !McpPool::is_mcp_tool(&tool_name)
&& tool_name != CODE_EXECUTION_TOOL_NAME
&& tool_name != JS_EXECUTION_TOOL_NAME
&& !is_tool_search_tool(&tool_name)
{
blocked_error = Some(ToolError::not_available(missing_tool_error_message(
&tool_name,
&tool_catalog,
)));
}
if McpPool::is_mcp_tool(&tool_name) {
read_only = mcp_tool_is_read_only(&tool_name);
supports_parallel = mcp_tool_is_parallel_safe(&tool_name);
@@ -2654,4 +2668,4 @@ mod tests {
assert_eq!(results[0].exit_code, Some(2));
assert!(results[0].stdout.contains("security"));
}
}
}
crates/tui/src/hooks.rs
+14
View File
@@ -551,6 +551,20 @@ impl HookExecutor {
self.config.enabled && self.config.hooks.iter().any(|h| h.event == event)
}
/// Check if there are any background hooks configured for a specific event.
///
/// Background hooks fire and forget — their `exit_code` is always `None`,
/// so they cannot deny tool calls. This is a known limitation; the check
/// is used to warn operators when a `ToolCallBefore` hook is configured
/// as background but expects to block a tool.
#[must_use]
pub fn has_background_hooks_for_event(&self, event: HookEvent) -> bool {
if !self.config.enabled {
return false;
}
self.config.hooks.iter().any(|h| h.event == event && h.background)
}
/// Run configured `message_submit` hooks as a mutable submit pipeline.
///
/// This is deliberately separate from [`Self::execute`]: most hook events
crates/tui/src/tui/ui.rs
+5 -5
View File
@@ -511,8 +511,8 @@ pub async fn run_tui(config: &Config, options: TuiOptions) -> Result<()> {
active_task_id: None,
active_thread_id: None,
// #456: plumb the App's HookExecutor so `exec_shell` can surface
// the configured `shell_env` hooks. Wrapped in Arc once and shared.
hook_executor: Some(std::sync::Arc::new(app.hooks.clone())),
// the configured `shell_env` hooks. Clone the shared Arc.
hook_executor: app.runtime_services.hook_executor.clone(),
handle_store: app.runtime_services.handle_store.clone(),
rlm_sessions: app.runtime_services.rlm_sessions.clone(),
};
@@ -754,7 +754,7 @@ fn build_engine_config(app: &App, config: &Config) -> EngineConfig {
),
max_spawn_depth: crate::tools::subagent::DEFAULT_MAX_SPAWN_DEPTH,
allowed_tools: app.active_allowed_tools.clone(),
hook_executor: Some(std::sync::Arc::new(app.hooks.clone())),
hook_executor: app.runtime_services.hook_executor.clone(),
network_policy: config.network.clone().map(|toml_cfg| {
crate::network_policy::NetworkPolicyDecider::with_default_audit(toml_cfg.into_runtime())
}),
@@ -4707,7 +4707,7 @@ async fn dispatch_user_message(
translation_enabled: app.translation_enabled,
show_thinking: app.show_thinking,
allowed_tools: app.active_allowed_tools.clone(),
hook_executor: Some(std::sync::Arc::new(app.hooks.clone())),
hook_executor: app.runtime_services.hook_executor.clone(),
})
.await
{
@@ -8875,4 +8875,4 @@ fn parse_semver(v: &str) -> Option<(u32, u32, u32)> {
}
#[cfg(test)]
mod tests;
mod tests;