From 5883a907e502137be796fc0da35841b4e74e6e4c Mon Sep 17 00:00:00 2001 From: Sandipsinh Rathod <62684960+ssddOnTop@users.noreply.github.com> Date: Tue, 31 Mar 2026 16:33:28 -0400 Subject: [PATCH 01/23] feat(hooks): add user-configurable hook system with config, executor, and handler --- .../src/hooks/user_hook_config_loader.rs | 208 +++++++ .../forge_app/src/hooks/user_hook_executor.rs | 258 +++++++++ .../forge_app/src/hooks/user_hook_handler.rs | 531 ++++++++++++++++++ crates/forge_domain/src/user_hook_config.rs | 311 ++++++++++ crates/forge_domain/src/user_hook_io.rs | 313 +++++++++++ 5 files changed, 1621 insertions(+) create mode 100644 crates/forge_app/src/hooks/user_hook_config_loader.rs create mode 100644 crates/forge_app/src/hooks/user_hook_executor.rs create mode 100644 crates/forge_app/src/hooks/user_hook_handler.rs create mode 100644 crates/forge_domain/src/user_hook_config.rs create mode 100644 crates/forge_domain/src/user_hook_io.rs diff --git a/crates/forge_app/src/hooks/user_hook_config_loader.rs b/crates/forge_app/src/hooks/user_hook_config_loader.rs new file mode 100644 index 0000000000..7c24b70e05 --- /dev/null +++ b/crates/forge_app/src/hooks/user_hook_config_loader.rs @@ -0,0 +1,208 @@ +use std::path::Path; + +use forge_domain::{UserHookConfig, UserSettings}; +use tracing::{debug, warn}; + +/// Loads and merges user hook configurations from the three settings file +/// locations. +/// +/// Resolution order (all merged, not overridden): +/// 1. `~/.forge/settings.json` (user-level, applies to all projects) +/// 2. `.forge/settings.json` (project-level, committable) +/// 3. `.forge/settings.local.json` (project-level, gitignored) +pub struct UserHookConfigLoader; + +impl UserHookConfigLoader { + /// Loads and merges hook configurations from all settings files. + /// + /// # Arguments + /// * `home` - Home directory (e.g., `/Users/name`). If `None`, user-level + /// settings are skipped. + /// * `cwd` - Current working directory for project-level settings. + /// + /// # Errors + /// This function does not return errors. Invalid or missing files are + /// silently skipped with a debug log. + pub fn load(home: Option<&Path>, cwd: &Path) -> UserHookConfig { + let mut config = UserHookConfig::new(); + + // 1. User-level: ~/.forge/settings.json + if let Some(home) = home { + let user_settings_path = home.join("forge").join("settings.json"); + if let Some(user_config) = Self::load_file(&user_settings_path) { + debug!(path = %user_settings_path.display(), "Loaded user-level hook config"); + config.merge(user_config); + } + } + + // 2. Project-level: .forge/settings.json + let project_settings_path = cwd.join(".forge").join("settings.json"); + if let Some(project_config) = Self::load_file(&project_settings_path) { + debug!(path = %project_settings_path.display(), "Loaded project-level hook config"); + config.merge(project_config); + } + + // 3. Project-local: .forge/settings.local.json + let local_settings_path = cwd.join(".forge").join("settings.local.json"); + if let Some(local_config) = Self::load_file(&local_settings_path) { + debug!(path = %local_settings_path.display(), "Loaded project-local hook config"); + config.merge(local_config); + } + + if !config.is_empty() { + debug!( + event_count = config.events.len(), + "Merged user hook configuration" + ); + } + + config + } + + /// Loads a single settings file and extracts hook configuration. + /// + /// Returns `None` if the file doesn't exist or is invalid. + fn load_file(path: &Path) -> Option { + let contents = match std::fs::read_to_string(path) { + Ok(c) => c, + Err(_) => return None, + }; + + match serde_json::from_str::(&contents) { + Ok(settings) => { + if settings.hooks.is_empty() { + None + } else { + Some(settings.hooks) + } + } + Err(e) => { + warn!( + path = %path.display(), + error = %e, + "Failed to parse settings file for hooks" + ); + None + } + } + } +} + +#[cfg(test)] +mod tests { + use std::path::PathBuf; + + use pretty_assertions::assert_eq; + + use super::*; + + #[test] + fn test_load_nonexistent_paths() { + let home = PathBuf::from("/nonexistent/home"); + let cwd = PathBuf::from("/nonexistent/project"); + + let actual = UserHookConfigLoader::load(Some(&home), &cwd); + assert!(actual.is_empty()); + } + + #[test] + fn test_load_file_valid_settings() { + let dir = tempfile::tempdir().unwrap(); + let settings_path = dir.path().join("settings.json"); + std::fs::write( + &settings_path, + r#"{ + "hooks": { + "PreToolUse": [ + { "matcher": "Bash", "hooks": [{ "type": "command", "command": "check.sh" }] } + ] + } + }"#, + ) + .unwrap(); + + let actual = UserHookConfigLoader::load_file(&settings_path); + assert!(actual.is_some()); + let config = actual.unwrap(); + assert_eq!( + config + .get_groups(&forge_domain::UserHookEventName::PreToolUse) + .len(), + 1 + ); + } + + #[test] + fn test_load_file_settings_without_hooks() { + let dir = tempfile::tempdir().unwrap(); + let settings_path = dir.path().join("settings.json"); + std::fs::write(&settings_path, r#"{"other_key": "value"}"#).unwrap(); + + let actual = UserHookConfigLoader::load_file(&settings_path); + assert!(actual.is_none()); + } + + #[test] + fn test_load_merges_all_sources() { + // Set up a fake home directory + let home_dir = tempfile::tempdir().unwrap(); + let forge_dir = home_dir.path().join("forge"); + std::fs::create_dir_all(&forge_dir).unwrap(); + std::fs::write( + forge_dir.join("settings.json"), + r#"{ + "hooks": { + "PreToolUse": [ + { "matcher": "Bash", "hooks": [{ "type": "command", "command": "global.sh" }] } + ] + } + }"#, + ) + .unwrap(); + + // Set up a project directory + let project_dir = tempfile::tempdir().unwrap(); + let project_forge_dir = project_dir.path().join(".forge"); + std::fs::create_dir_all(&project_forge_dir).unwrap(); + std::fs::write( + project_forge_dir.join("settings.json"), + r#"{ + "hooks": { + "PreToolUse": [ + { "matcher": "Write", "hooks": [{ "type": "command", "command": "project.sh" }] } + ] + } + }"#, + ) + .unwrap(); + std::fs::write( + project_forge_dir.join("settings.local.json"), + r#"{ + "hooks": { + "Stop": [ + { "hooks": [{ "type": "command", "command": "local-stop.sh" }] } + ] + } + }"#, + ) + .unwrap(); + + let actual = + UserHookConfigLoader::load(Some(home_dir.path()), project_dir.path()); + + // PreToolUse should have 2 groups (global + project) + assert_eq!( + actual + .get_groups(&forge_domain::UserHookEventName::PreToolUse) + .len(), + 2 + ); + // Stop should have 1 group (local) + assert_eq!( + actual + .get_groups(&forge_domain::UserHookEventName::Stop) + .len(), + 1 + ); + } +} diff --git a/crates/forge_app/src/hooks/user_hook_executor.rs b/crates/forge_app/src/hooks/user_hook_executor.rs new file mode 100644 index 0000000000..2faf677d3c --- /dev/null +++ b/crates/forge_app/src/hooks/user_hook_executor.rs @@ -0,0 +1,258 @@ +use std::collections::HashMap; +use std::path::PathBuf; +use std::time::Duration; + +use forge_domain::HookExecutionResult; +use tokio::io::AsyncWriteExt; +use tracing::{debug, warn}; + +/// Default timeout for hook commands (10 minutes). +const DEFAULT_HOOK_TIMEOUT: Duration = Duration::from_secs(600); + +/// Executes user hook shell commands with stdin piping and timeout support. +/// +/// Uses `tokio::process::Command` directly (not `CommandInfra`) because we +/// need stdin piping which the existing infrastructure doesn't support. +pub struct UserHookExecutor; + +impl UserHookExecutor { + /// Executes a shell command, piping `input_json` to stdin and capturing + /// stdout/stderr. + /// + /// # Arguments + /// * `command` - The shell command string to execute. + /// * `input_json` - JSON string to pipe to the command's stdin. + /// * `timeout` - Optional timeout in milliseconds. Uses default (10 min) if + /// `None`. + /// * `cwd` - Working directory for the command. + /// * `env_vars` - Additional environment variables to set. + /// + /// # Errors + /// Returns an error if the process cannot be spawned. + pub async fn execute( + command: &str, + input_json: &str, + timeout: Option, + cwd: &PathBuf, + env_vars: &HashMap, + ) -> anyhow::Result { + let timeout_duration = timeout + .map(Duration::from_millis) + .unwrap_or(DEFAULT_HOOK_TIMEOUT); + + debug!( + command = command, + cwd = %cwd.display(), + timeout_ms = timeout_duration.as_millis() as u64, + "Executing user hook command" + ); + + let shell = if cfg!(target_os = "windows") { + "cmd" + } else { + "sh" + }; + let shell_arg = if cfg!(target_os = "windows") { + "/C" + } else { + "-c" + }; + + let mut child = tokio::process::Command::new(shell) + .arg(shell_arg) + .arg(command) + .current_dir(cwd) + .envs(env_vars) + .stdin(std::process::Stdio::piped()) + .stdout(std::process::Stdio::piped()) + .stderr(std::process::Stdio::piped()) + .spawn()?; + + // Pipe JSON input to stdin + if let Some(mut stdin) = child.stdin.take() { + let input = input_json.to_string(); + tokio::spawn(async move { + let _ = stdin.write_all(input.as_bytes()).await; + let _ = stdin.shutdown().await; + }); + } + + // Wait for the command with timeout. + // Note: `wait_with_output()` takes ownership of `child`. On timeout, + // the future is dropped, and tokio will clean up the child process. + let result = tokio::time::timeout(timeout_duration, child.wait_with_output()).await; + + match result { + Ok(Ok(output)) => { + let exit_code = output.status.code(); + let stdout = String::from_utf8_lossy(&output.stdout).to_string(); + let stderr = String::from_utf8_lossy(&output.stderr).to_string(); + + debug!( + command = command, + exit_code = ?exit_code, + stdout_len = stdout.len(), + stderr_len = stderr.len(), + "Hook command completed" + ); + + Ok(HookExecutionResult { exit_code, stdout, stderr }) + } + Ok(Err(e)) => { + warn!(command = command, error = %e, "Hook command failed to execute"); + Err(e.into()) + } + Err(_) => { + warn!( + command = command, + timeout_ms = timeout_duration.as_millis() as u64, + "Hook command timed out" + ); + // Process is already consumed by wait_with_output, tokio + // handles cleanup when the future is dropped. + Ok(HookExecutionResult { + exit_code: None, + stdout: String::new(), + stderr: format!( + "Hook command timed out after {}ms", + timeout_duration.as_millis() + ), + }) + } + } + } +} + +#[cfg(test)] +mod tests { + use std::collections::HashMap; + + use pretty_assertions::assert_eq; + + use super::*; + + #[tokio::test] + async fn test_execute_simple_command() { + let cwd = std::env::current_dir().unwrap(); + let actual = + UserHookExecutor::execute("echo hello", "{}", None, &cwd, &HashMap::new()) + .await + .unwrap(); + + assert_eq!(actual.exit_code, Some(0)); + assert_eq!(actual.stdout.trim(), "hello"); + assert!(actual.is_success()); + } + + #[tokio::test] + async fn test_execute_reads_stdin() { + let cwd = std::env::current_dir().unwrap(); + let actual = UserHookExecutor::execute( + "cat", + r#"{"hook_event_name": "PreToolUse"}"#, + None, + &cwd, + &HashMap::new(), + ) + .await + .unwrap(); + + assert_eq!(actual.exit_code, Some(0)); + assert!(actual.stdout.contains("PreToolUse")); + } + + #[tokio::test] + async fn test_execute_exit_code_2() { + let cwd = std::env::current_dir().unwrap(); + let actual = UserHookExecutor::execute( + "echo 'blocked' >&2; exit 2", + "{}", + None, + &cwd, + &HashMap::new(), + ) + .await + .unwrap(); + + assert_eq!(actual.exit_code, Some(2)); + assert!(actual.is_blocking_exit()); + assert!(actual.stderr.contains("blocked")); + } + + #[tokio::test] + async fn test_execute_non_blocking_error() { + let cwd = std::env::current_dir().unwrap(); + let actual = UserHookExecutor::execute( + "exit 1", + "{}", + None, + &cwd, + &HashMap::new(), + ) + .await + .unwrap(); + + assert_eq!(actual.exit_code, Some(1)); + assert!(actual.is_non_blocking_error()); + } + + #[tokio::test] + async fn test_execute_timeout() { + let cwd = std::env::current_dir().unwrap(); + let actual = UserHookExecutor::execute( + "sleep 10", + "{}", + Some(100), // 100ms timeout + &cwd, + &HashMap::new(), + ) + .await + .unwrap(); + + // Should have no exit code (killed by timeout) + assert!(actual.exit_code.is_none() || actual.is_non_blocking_error()); + assert!(actual.stderr.contains("timed out")); + } + + #[tokio::test] + async fn test_execute_with_env_vars() { + let cwd = std::env::current_dir().unwrap(); + let mut env_vars = HashMap::new(); + env_vars.insert( + "FORGE_TEST_VAR".to_string(), + "test_value".to_string(), + ); + + let actual = UserHookExecutor::execute( + "echo $FORGE_TEST_VAR", + "{}", + None, + &cwd, + &env_vars, + ) + .await + .unwrap(); + + assert_eq!(actual.exit_code, Some(0)); + assert_eq!(actual.stdout.trim(), "test_value"); + } + + #[tokio::test] + async fn test_execute_json_output() { + let cwd = std::env::current_dir().unwrap(); + let actual = UserHookExecutor::execute( + r#"echo '{"decision":"block","reason":"test"}'"#, + "{}", + None, + &cwd, + &HashMap::new(), + ) + .await + .unwrap(); + + assert!(actual.is_success()); + let output = actual.parse_output().unwrap(); + assert!(output.is_blocking()); + assert_eq!(output.reason, Some("test".to_string())); + } +} diff --git a/crates/forge_app/src/hooks/user_hook_handler.rs b/crates/forge_app/src/hooks/user_hook_handler.rs new file mode 100644 index 0000000000..3925b7b57f --- /dev/null +++ b/crates/forge_app/src/hooks/user_hook_handler.rs @@ -0,0 +1,531 @@ +use std::collections::HashMap; +use std::path::PathBuf; +use std::sync::atomic::{AtomicBool, Ordering}; + +use async_trait::async_trait; +use forge_domain::{ + Conversation, EndPayload, EventData, EventHandle, HookEventInput, HookExecutionResult, + HookInput, HookOutput, RequestPayload, ResponsePayload, StartPayload, ToolcallEndPayload, + ToolcallStartPayload, UserHookConfig, UserHookEntry, UserHookEventName, UserHookMatcherGroup, +}; +use regex::Regex; +use tracing::{debug, warn}; + +use super::user_hook_executor::UserHookExecutor; + +/// EventHandle implementation that bridges user-configured hooks with the +/// existing lifecycle event system. +/// +/// This handler is constructed from a `UserHookConfig` and executes matching +/// hook commands at each lifecycle event point. It wires into the existing +/// `Hook` system via `Hook::zip()`. +#[derive(Clone)] +pub struct UserHookHandler { + config: UserHookConfig, + cwd: PathBuf, + env_vars: HashMap, + /// Tracks whether a Stop hook has already fired to prevent infinite loops. + stop_hook_active: std::sync::Arc, +} + +impl UserHookHandler { + /// Creates a new user hook handler from configuration. + /// + /// # Arguments + /// * `config` - The merged user hook configuration. + /// * `cwd` - Current working directory for command execution. + /// * `project_dir` - Project root directory for `FORGE_PROJECT_DIR` env + /// var. + /// * `session_id` - Current session/conversation ID. + pub fn new( + config: UserHookConfig, + cwd: PathBuf, + project_dir: PathBuf, + session_id: String, + ) -> Self { + let mut env_vars = HashMap::new(); + env_vars.insert( + "FORGE_PROJECT_DIR".to_string(), + project_dir.to_string_lossy().to_string(), + ); + env_vars.insert("FORGE_SESSION_ID".to_string(), session_id); + env_vars.insert( + "FORGE_CWD".to_string(), + cwd.to_string_lossy().to_string(), + ); + + Self { + config, + cwd, + env_vars, + stop_hook_active: std::sync::Arc::new(AtomicBool::new(false)), + } + } + + /// Checks if the config has any hooks for the given event. + fn has_hooks(&self, event: &UserHookEventName) -> bool { + !self.config.get_groups(event).is_empty() + } + + /// Finds matching hook entries for an event, filtered by the optional + /// matcher regex against the given subject string. + fn find_matching_hooks<'a>( + groups: &'a [UserHookMatcherGroup], + subject: Option<&str>, + ) -> Vec<&'a UserHookEntry> { + let mut matching = Vec::new(); + + for group in groups { + let matches = match (&group.matcher, subject) { + (Some(pattern), Some(subj)) => { + match Regex::new(pattern) { + Ok(re) => re.is_match(subj), + Err(e) => { + warn!( + pattern = pattern, + error = %e, + "Invalid regex in hook matcher, skipping" + ); + false + } + } + } + (Some(_), None) => { + // Matcher specified but no subject to match against; skip + false + } + (None, _) => { + // No matcher means unconditional match + true + } + }; + + if matches { + matching.extend(group.hooks.iter()); + } + } + + matching + } + + /// Executes a list of hook entries and returns their results. + async fn execute_hooks( + &self, + hooks: &[&UserHookEntry], + input: &HookInput, + ) -> Vec { + let input_json = match serde_json::to_string(input) { + Ok(json) => json, + Err(e) => { + warn!(error = %e, "Failed to serialize hook input"); + return Vec::new(); + } + }; + + let mut results = Vec::new(); + for hook in hooks { + if let Some(command) = &hook.command { + match UserHookExecutor::execute( + command, + &input_json, + hook.timeout, + &self.cwd, + &self.env_vars, + ) + .await + { + Ok(result) => results.push(result), + Err(e) => { + warn!( + command = command, + error = %e, + "Hook command failed to execute" + ); + } + } + } + } + + results + } + + /// Processes hook results, returning a blocking reason if any hook blocked. + fn process_results(results: &[HookExecutionResult]) -> Option { + for result in results { + // Exit code 2 = blocking error + if result.is_blocking_exit() { + let message = result + .blocking_message() + .unwrap_or("Hook blocked execution") + .to_string(); + return Some(message); + } + + // Exit code 0 = check stdout for JSON decisions + if let Some(output) = result.parse_output() { + if output.is_blocking() { + let reason = output + .reason + .unwrap_or_else(|| "Hook blocked execution".to_string()); + return Some(reason); + } + } + + // Non-blocking errors (exit code 1, etc.) are logged but don't block + if result.is_non_blocking_error() { + warn!( + exit_code = ?result.exit_code, + stderr = result.stderr.as_str(), + "Hook command returned non-blocking error" + ); + } + } + + None + } + + /// Processes PreToolUse results, extracting updated input if present. + fn process_pre_tool_use_output(results: &[HookExecutionResult]) -> PreToolUseDecision { + for result in results { + // Exit code 2 = blocking error + if result.is_blocking_exit() { + let message = result + .blocking_message() + .unwrap_or("Hook blocked tool execution") + .to_string(); + return PreToolUseDecision::Block(message); + } + + // Exit code 0 = check stdout for JSON decisions + if let Some(output) = result.parse_output() { + // Check permission decision + if output.permission_decision.as_deref() == Some("deny") { + let reason = output + .reason + .unwrap_or_else(|| "Tool execution denied by hook".to_string()); + return PreToolUseDecision::Block(reason); + } + + // Check generic block decision + if output.is_blocking() { + let reason = output + .reason + .unwrap_or_else(|| "Hook blocked tool execution".to_string()); + return PreToolUseDecision::Block(reason); + } + + // Check for updated input + if output.updated_input.is_some() { + return PreToolUseDecision::AllowWithUpdate(output); + } + } + + // Non-blocking errors are logged but don't block + if result.is_non_blocking_error() { + warn!( + exit_code = ?result.exit_code, + stderr = result.stderr.as_str(), + "PreToolUse hook command returned non-blocking error" + ); + } + } + + PreToolUseDecision::Allow + } +} + +/// Decision result from PreToolUse hook processing. +enum PreToolUseDecision { + /// Allow the tool call to proceed. + Allow, + /// Allow but with updated input from the hook output. + AllowWithUpdate(HookOutput), + /// Block the tool call with the given reason. + Block(String), +} + +// --- EventHandle implementations --- + +#[async_trait] +impl EventHandle> for UserHookHandler { + async fn handle( + &self, + _event: &EventData, + _conversation: &mut Conversation, + ) -> anyhow::Result<()> { + if !self.has_hooks(&UserHookEventName::SessionStart) { + return Ok(()); + } + + let groups = self.config.get_groups(&UserHookEventName::SessionStart); + let hooks = Self::find_matching_hooks(groups, Some("startup")); + + if hooks.is_empty() { + return Ok(()); + } + + let input = HookInput { + hook_event_name: "SessionStart".to_string(), + cwd: self.cwd.to_string_lossy().to_string(), + session_id: self.env_vars.get("FORGE_SESSION_ID").cloned(), + event_data: HookEventInput::SessionStart { source: "startup".to_string() }, + }; + + let results = self.execute_hooks(&hooks, &input).await; + + // SessionStart hooks can provide additional context but not block + for result in &results { + if let Some(output) = result.parse_output() { + if let Some(context) = &output.additional_context { + debug!( + context_len = context.len(), + "SessionStart hook provided additional context" + ); + } + } + } + + Ok(()) + } +} + +#[async_trait] +impl EventHandle> for UserHookHandler { + async fn handle( + &self, + _event: &EventData, + _conversation: &mut Conversation, + ) -> anyhow::Result<()> { + // No user hook events map to Request currently + Ok(()) + } +} + +#[async_trait] +impl EventHandle> for UserHookHandler { + async fn handle( + &self, + _event: &EventData, + _conversation: &mut Conversation, + ) -> anyhow::Result<()> { + // No user hook events map to Response currently + Ok(()) + } +} + +#[async_trait] +impl EventHandle> for UserHookHandler { + async fn handle( + &self, + event: &EventData, + conversation: &mut Conversation, + ) -> anyhow::Result<()> { + if !self.has_hooks(&UserHookEventName::PreToolUse) { + return Ok(()); + } + + let tool_name = event.payload.tool_call.name.as_str(); + let groups = self.config.get_groups(&UserHookEventName::PreToolUse); + let hooks = Self::find_matching_hooks(groups, Some(tool_name)); + + if hooks.is_empty() { + return Ok(()); + } + + let tool_input = + serde_json::to_value(&event.payload.tool_call.arguments).unwrap_or_default(); + + let input = HookInput { + hook_event_name: "PreToolUse".to_string(), + cwd: self.cwd.to_string_lossy().to_string(), + session_id: self.env_vars.get("FORGE_SESSION_ID").cloned(), + event_data: HookEventInput::PreToolUse { + tool_name: tool_name.to_string(), + tool_input, + }, + }; + + let results = self.execute_hooks(&hooks, &input).await; + let decision = Self::process_pre_tool_use_output(&results); + + match decision { + PreToolUseDecision::Allow => Ok(()), + PreToolUseDecision::AllowWithUpdate(_output) => { + // Note: Updating tool call input would require modifying the tool call + // in-flight, which would need changes to the orchestrator. + // For now, we log and proceed. + debug!( + tool_name = tool_name, + "PreToolUse hook returned updatedInput (not yet supported for modification)" + ); + Ok(()) + } + PreToolUseDecision::Block(reason) => { + debug!( + tool_name = tool_name, + reason = reason.as_str(), + "PreToolUse hook blocked tool call" + ); + // Inject a user message with the block reason so the agent sees it + if let Some(context) = conversation.context.as_mut() { + let block_msg = format!( + "\nPreToolUse\n{}\nblocked\n{}\n", + tool_name, reason + ); + context.messages.push( + forge_domain::ContextMessage::user(block_msg, None).into(), + ); + } + // Return an error to signal the orchestrator to skip this tool call + Err(anyhow::anyhow!( + "Tool call '{}' blocked by PreToolUse hook: {}", + tool_name, + reason + )) + } + } + } +} + +#[async_trait] +impl EventHandle> for UserHookHandler { + async fn handle( + &self, + event: &EventData, + conversation: &mut Conversation, + ) -> anyhow::Result<()> { + let is_error = event.payload.result.is_error(); + let event_name = if is_error { + UserHookEventName::PostToolUseFailure + } else { + UserHookEventName::PostToolUse + }; + + if !self.has_hooks(&event_name) { + return Ok(()); + } + + let tool_name = event.payload.tool_call.name.as_str(); + let groups = self.config.get_groups(&event_name); + let hooks = Self::find_matching_hooks(groups, Some(tool_name)); + + if hooks.is_empty() { + return Ok(()); + } + + let tool_input = + serde_json::to_value(&event.payload.tool_call.arguments).unwrap_or_default(); + let tool_response = serde_json::to_value(&event.payload.result.output).unwrap_or_default(); + + let input = HookInput { + hook_event_name: event_name.to_string(), + cwd: self.cwd.to_string_lossy().to_string(), + session_id: self.env_vars.get("FORGE_SESSION_ID").cloned(), + event_data: HookEventInput::PostToolUse { + tool_name: tool_name.to_string(), + tool_input, + tool_response, + }, + }; + + let results = self.execute_hooks(&hooks, &input).await; + + // PostToolUse can provide feedback via blocking + if let Some(reason) = Self::process_results(&results) { + debug!( + tool_name = tool_name, + event = %event_name, + reason = reason.as_str(), + "PostToolUse hook blocked with feedback" + ); + // Inject feedback as a user message + if let Some(context) = conversation.context.as_mut() { + let feedback_msg = format!( + "\n{}\n{}\nblocked\n{}\n", + event_name, tool_name, reason + ); + context.messages.push( + forge_domain::ContextMessage::user(feedback_msg, None).into(), + ); + } + } + + Ok(()) + } +} + +#[async_trait] +impl EventHandle> for UserHookHandler { + async fn handle( + &self, + _event: &EventData, + conversation: &mut Conversation, + ) -> anyhow::Result<()> { + // Fire SessionEnd hooks + if self.has_hooks(&UserHookEventName::SessionEnd) { + let groups = self.config.get_groups(&UserHookEventName::SessionEnd); + let hooks = Self::find_matching_hooks(groups, None); + + if !hooks.is_empty() { + let input = HookInput { + hook_event_name: "SessionEnd".to_string(), + cwd: self.cwd.to_string_lossy().to_string(), + session_id: self.env_vars.get("FORGE_SESSION_ID").cloned(), + event_data: HookEventInput::Empty {}, + }; + self.execute_hooks(&hooks, &input).await; + } + } + + // Fire Stop hooks + if !self.has_hooks(&UserHookEventName::Stop) { + return Ok(()); + } + + // Prevent infinite loops + let was_active = self.stop_hook_active.swap(true, Ordering::SeqCst); + if was_active { + debug!("Stop hook already active, skipping to prevent infinite loop"); + return Ok(()); + } + + let groups = self.config.get_groups(&UserHookEventName::Stop); + let hooks = Self::find_matching_hooks(groups, None); + + if hooks.is_empty() { + self.stop_hook_active.store(false, Ordering::SeqCst); + return Ok(()); + } + + let input = HookInput { + hook_event_name: "Stop".to_string(), + cwd: self.cwd.to_string_lossy().to_string(), + session_id: self.env_vars.get("FORGE_SESSION_ID").cloned(), + event_data: HookEventInput::Stop { stop_hook_active: was_active }, + }; + + let results = self.execute_hooks(&hooks, &input).await; + + if let Some(reason) = Self::process_results(&results) { + debug!( + reason = reason.as_str(), + "Stop hook wants to continue conversation" + ); + // Inject a message to continue the conversation + if let Some(context) = conversation.context.as_mut() { + let continue_msg = format!( + "\nStop\ncontinue\n{}\n", + reason + ); + context + .messages + .push(forge_domain::ContextMessage::user(continue_msg, None).into()); + } + } + + // Reset the stop hook active flag + self.stop_hook_active.store(false, Ordering::SeqCst); + + Ok(()) + } +} diff --git a/crates/forge_domain/src/user_hook_config.rs b/crates/forge_domain/src/user_hook_config.rs new file mode 100644 index 0000000000..ae3dbb08b8 --- /dev/null +++ b/crates/forge_domain/src/user_hook_config.rs @@ -0,0 +1,311 @@ +use std::collections::HashMap; +use std::fmt; + +use serde::{Deserialize, Serialize}; + +/// Top-level user hook configuration. +/// +/// Maps hook event names to a list of matcher groups. This is deserialized +/// from the `"hooks"` key in `.forge/settings.json` or `~/.forge/settings.json`. +/// +/// Example JSON: +/// ```json +/// { +/// "PreToolUse": [ +/// { "matcher": "Bash", "hooks": [{ "type": "command", "command": "echo hi" }] } +/// ] +/// } +/// ``` +#[derive(Default, Debug, Clone, Serialize, Deserialize, PartialEq)] +pub struct UserHookConfig { + /// Map of event name -> list of matcher groups + #[serde(flatten)] + pub events: HashMap>, +} + +impl UserHookConfig { + /// Creates an empty user hook configuration. + pub fn new() -> Self { + Self { events: HashMap::new() } + } + + /// Returns the matcher groups for a given event name, or an empty slice if + /// none. + pub fn get_groups(&self, event: &UserHookEventName) -> &[UserHookMatcherGroup] { + self.events.get(event).map_or(&[], |v| v.as_slice()) + } + + /// Merges another config into this one, appending matcher groups for each + /// event. + pub fn merge(&mut self, other: UserHookConfig) { + for (event, groups) in other.events { + self.events.entry(event).or_default().extend(groups); + } + } + + /// Returns true if no hook events are configured. + pub fn is_empty(&self) -> bool { + self.events.is_empty() + } +} + +/// Supported hook event names that map to lifecycle points in the +/// orchestrator. +#[derive(Debug, Clone, PartialEq, Eq, Hash, Serialize, Deserialize)] +pub enum UserHookEventName { + /// Fired before a tool call executes. Can block execution. + PreToolUse, + /// Fired after a tool call succeeds. + PostToolUse, + /// Fired after a tool call fails. + PostToolUseFailure, + /// Fired when the agent finishes responding. Can block stop to continue. + Stop, + /// Fired when a notification is sent. + Notification, + /// Fired when a session starts or resumes. + SessionStart, + /// Fired when a session ends/terminates. + SessionEnd, + /// Fired when a user prompt is submitted. + UserPromptSubmit, + /// Fired before context compaction. + PreCompact, + /// Fired after context compaction. + PostCompact, +} + +impl fmt::Display for UserHookEventName { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + match self { + Self::PreToolUse => write!(f, "PreToolUse"), + Self::PostToolUse => write!(f, "PostToolUse"), + Self::PostToolUseFailure => write!(f, "PostToolUseFailure"), + Self::Stop => write!(f, "Stop"), + Self::Notification => write!(f, "Notification"), + Self::SessionStart => write!(f, "SessionStart"), + Self::SessionEnd => write!(f, "SessionEnd"), + Self::UserPromptSubmit => write!(f, "UserPromptSubmit"), + Self::PreCompact => write!(f, "PreCompact"), + Self::PostCompact => write!(f, "PostCompact"), + } + } +} + +/// A matcher group pairs an optional regex matcher with a list of hook +/// handlers. +/// +/// When a lifecycle event fires, only matcher groups whose `matcher` regex +/// matches the relevant event context (e.g., tool name) will have their hooks +/// executed. If `matcher` is `None`, all hooks in this group fire +/// unconditionally. +#[derive(Debug, Clone, Serialize, Deserialize, PartialEq)] +pub struct UserHookMatcherGroup { + /// Optional regex pattern to match against (e.g., tool name for + /// PreToolUse/PostToolUse). + #[serde(default, skip_serializing_if = "Option::is_none")] + pub matcher: Option, + + /// List of hook handlers to execute when this matcher matches. + #[serde(default)] + pub hooks: Vec, +} + +/// A single hook handler entry that defines what action to take. +#[derive(Debug, Clone, Serialize, Deserialize, PartialEq)] +pub struct UserHookEntry { + /// The type of hook handler. + #[serde(rename = "type")] + pub hook_type: UserHookType, + + /// The shell command to execute (for `Command` type hooks). + #[serde(default, skip_serializing_if = "Option::is_none")] + pub command: Option, + + /// Timeout in milliseconds for this hook. Defaults to 600000ms (10 + /// minutes). + #[serde(default, skip_serializing_if = "Option::is_none")] + pub timeout: Option, +} + +/// The type of hook handler to execute. +#[derive(Debug, Clone, Serialize, Deserialize, PartialEq)] +#[serde(rename_all = "lowercase")] +pub enum UserHookType { + /// Executes a shell command, piping JSON to stdin and reading JSON from + /// stdout. + Command, +} + +/// Wrapper for the top-level settings JSON that contains the hooks key. +/// +/// Used for deserializing the entire settings file and extracting just the +/// `"hooks"` section. +#[derive(Default, Debug, Clone, Serialize, Deserialize, PartialEq)] +pub struct UserSettings { + /// User hook configuration. + #[serde(default)] + pub hooks: UserHookConfig, +} + +#[cfg(test)] +mod tests { + use pretty_assertions::assert_eq; + + use super::*; + + #[test] + fn test_deserialize_empty_config() { + let json = r#"{}"#; + let actual: UserHookConfig = serde_json::from_str(json).unwrap(); + let expected = UserHookConfig::new(); + assert_eq!(actual, expected); + } + + #[test] + fn test_deserialize_pre_tool_use_hook() { + let json = r#"{ + "PreToolUse": [ + { + "matcher": "Bash", + "hooks": [ + { + "type": "command", + "command": "echo 'blocked'" + } + ] + } + ] + }"#; + + let actual: UserHookConfig = serde_json::from_str(json).unwrap(); + let groups = actual.get_groups(&UserHookEventName::PreToolUse); + + assert_eq!(groups.len(), 1); + assert_eq!(groups[0].matcher, Some("Bash".to_string())); + assert_eq!(groups[0].hooks.len(), 1); + assert_eq!(groups[0].hooks[0].hook_type, UserHookType::Command); + assert_eq!( + groups[0].hooks[0].command, + Some("echo 'blocked'".to_string()) + ); + } + + #[test] + fn test_deserialize_multiple_events() { + let json = r#"{ + "PreToolUse": [ + { "matcher": "Bash", "hooks": [{ "type": "command", "command": "pre.sh" }] } + ], + "PostToolUse": [ + { "hooks": [{ "type": "command", "command": "post.sh" }] } + ], + "Stop": [ + { "hooks": [{ "type": "command", "command": "stop.sh" }] } + ] + }"#; + + let actual: UserHookConfig = serde_json::from_str(json).unwrap(); + + assert_eq!( + actual.get_groups(&UserHookEventName::PreToolUse).len(), + 1 + ); + assert_eq!( + actual.get_groups(&UserHookEventName::PostToolUse).len(), + 1 + ); + assert_eq!(actual.get_groups(&UserHookEventName::Stop).len(), 1); + assert!(actual.get_groups(&UserHookEventName::SessionStart).is_empty()); + } + + #[test] + fn test_deserialize_hook_with_timeout() { + let json = r#"{ + "PreToolUse": [ + { + "hooks": [ + { "type": "command", "command": "slow.sh", "timeout": 30000 } + ] + } + ] + }"#; + + let actual: UserHookConfig = serde_json::from_str(json).unwrap(); + let groups = actual.get_groups(&UserHookEventName::PreToolUse); + + assert_eq!(groups[0].hooks[0].timeout, Some(30000)); + } + + #[test] + fn test_merge_configs() { + let json1 = r#"{ + "PreToolUse": [ + { "matcher": "Bash", "hooks": [{ "type": "command", "command": "hook1.sh" }] } + ] + }"#; + let json2 = r#"{ + "PreToolUse": [ + { "matcher": "Write", "hooks": [{ "type": "command", "command": "hook2.sh" }] } + ], + "Stop": [ + { "hooks": [{ "type": "command", "command": "stop.sh" }] } + ] + }"#; + + let mut actual: UserHookConfig = serde_json::from_str(json1).unwrap(); + let config2: UserHookConfig = serde_json::from_str(json2).unwrap(); + actual.merge(config2); + + assert_eq!( + actual.get_groups(&UserHookEventName::PreToolUse).len(), + 2 + ); + assert_eq!(actual.get_groups(&UserHookEventName::Stop).len(), 1); + } + + #[test] + fn test_deserialize_settings_with_hooks() { + let json = r#"{ + "hooks": { + "PreToolUse": [ + { "matcher": "Bash", "hooks": [{ "type": "command", "command": "check.sh" }] } + ] + } + }"#; + + let actual: UserSettings = serde_json::from_str(json).unwrap(); + + assert!(!actual.hooks.is_empty()); + assert_eq!( + actual + .hooks + .get_groups(&UserHookEventName::PreToolUse) + .len(), + 1 + ); + } + + #[test] + fn test_deserialize_settings_without_hooks() { + let json = r#"{}"#; + let actual: UserSettings = serde_json::from_str(json).unwrap(); + + assert!(actual.hooks.is_empty()); + } + + #[test] + fn test_no_matcher_group_fires_unconditionally() { + let json = r#"{ + "PostToolUse": [ + { "hooks": [{ "type": "command", "command": "always.sh" }] } + ] + }"#; + + let actual: UserHookConfig = serde_json::from_str(json).unwrap(); + let groups = actual.get_groups(&UserHookEventName::PostToolUse); + + assert_eq!(groups.len(), 1); + assert_eq!(groups[0].matcher, None); + } +} diff --git a/crates/forge_domain/src/user_hook_io.rs b/crates/forge_domain/src/user_hook_io.rs new file mode 100644 index 0000000000..38fe4c4df3 --- /dev/null +++ b/crates/forge_domain/src/user_hook_io.rs @@ -0,0 +1,313 @@ +use serde::{Deserialize, Serialize}; +use serde_json::Value; + +/// Exit code constants for hook script results. +pub mod exit_codes { + /// Hook executed successfully. stdout may contain JSON output. + pub const SUCCESS: i32 = 0; + /// Blocking error. stderr is used as feedback message. + pub const BLOCK: i32 = 2; +} + +/// JSON input sent to hook scripts via stdin. +/// +/// Contains common fields shared across all hook events plus event-specific +/// data in the `event_data` field. +#[derive(Debug, Clone, Serialize, Deserialize, PartialEq)] +pub struct HookInput { + /// The hook event name (e.g., "PreToolUse", "PostToolUse", "Stop"). + pub hook_event_name: String, + + /// Current working directory. + pub cwd: String, + + /// Session/conversation ID. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub session_id: Option, + + /// Event-specific payload data. + #[serde(flatten)] + pub event_data: HookEventInput, +} + +/// Event-specific input data variants. +#[derive(Debug, Clone, Serialize, Deserialize, PartialEq)] +#[serde(untagged)] +pub enum HookEventInput { + /// Input for PreToolUse events. + PreToolUse { + /// Name of the tool being called. + tool_name: String, + /// Tool call arguments as a JSON value. + tool_input: Value, + }, + /// Input for PostToolUse events. + PostToolUse { + /// Name of the tool that was called. + tool_name: String, + /// Tool call arguments as a JSON value. + tool_input: Value, + /// Tool output/response as a JSON value. + tool_response: Value, + }, + /// Input for Stop events. + Stop { + /// Whether a Stop hook has already fired (prevents infinite loops). + stop_hook_active: bool, + }, + /// Input for SessionStart events. + SessionStart { + /// Source of the session start (e.g., "startup", "resume"). + source: String, + }, + /// Empty input for events that don't need event-specific data. + Empty {}, +} + +/// JSON output parsed from hook script stdout. +/// +/// Fields are optional; scripts that don't need to control behavior can simply +/// exit 0 with empty stdout. +#[derive(Default, Debug, Clone, Serialize, Deserialize, PartialEq)] +pub struct HookOutput { + /// Whether execution should continue. `false` halts processing. + #[serde(default, rename = "continue", skip_serializing_if = "Option::is_none")] + pub continue_execution: Option, + + /// Decision for blocking events. `"block"` blocks the operation. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub decision: Option, + + /// Reason for blocking, used as feedback to the agent. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub reason: Option, + + /// For PreToolUse: permission decision ("allow", "deny", "ask"). + #[serde( + default, + rename = "permissionDecision", + skip_serializing_if = "Option::is_none" + )] + pub permission_decision: Option, + + /// For PreToolUse: modified tool input to replace the original. + #[serde( + default, + rename = "updatedInput", + skip_serializing_if = "Option::is_none" + )] + pub updated_input: Option, + + /// Additional context to inject into the conversation. + #[serde( + default, + rename = "additionalContext", + skip_serializing_if = "Option::is_none" + )] + pub additional_context: Option, + + /// Reason for stopping (for Stop hooks). + #[serde( + default, + rename = "stopReason", + skip_serializing_if = "Option::is_none" + )] + pub stop_reason: Option, +} + +impl HookOutput { + /// Attempts to parse stdout as JSON. Falls back to empty output on failure. + pub fn parse(stdout: &str) -> Self { + if stdout.trim().is_empty() { + return Self::default(); + } + serde_json::from_str(stdout).unwrap_or_default() + } + + /// Returns true if this output requests blocking. + pub fn is_blocking(&self) -> bool { + self.decision.as_deref() == Some("block") + || self.permission_decision.as_deref() == Some("deny") + } +} + +/// Result of executing a hook command. +#[derive(Debug, Clone)] +pub struct HookExecutionResult { + /// Process exit code (None if terminated by signal). + pub exit_code: Option, + /// Captured stdout. + pub stdout: String, + /// Captured stderr. + pub stderr: String, +} + +impl HookExecutionResult { + /// Returns true if the hook exited with the blocking exit code (2). + pub fn is_blocking_exit(&self) -> bool { + self.exit_code == Some(exit_codes::BLOCK) + } + + /// Returns true if the hook exited successfully (0). + pub fn is_success(&self) -> bool { + self.exit_code == Some(exit_codes::SUCCESS) + } + + /// Returns true if the hook exited with a non-blocking error (non-0, + /// non-2). + pub fn is_non_blocking_error(&self) -> bool { + match self.exit_code { + Some(code) => code != exit_codes::SUCCESS && code != exit_codes::BLOCK, + None => true, + } + } + + /// Parses the stdout as a HookOutput if the exit was successful. + pub fn parse_output(&self) -> Option { + if self.is_success() { + Some(HookOutput::parse(&self.stdout)) + } else { + None + } + } + + /// Returns the feedback message for blocking errors (stderr content). + pub fn blocking_message(&self) -> Option<&str> { + if self.is_blocking_exit() { + let msg = self.stderr.trim(); + if msg.is_empty() { None } else { Some(msg) } + } else { + None + } + } +} + +#[cfg(test)] +mod tests { + use pretty_assertions::assert_eq; + + use super::*; + + #[test] + fn test_hook_input_serialization_pre_tool_use() { + let fixture = HookInput { + hook_event_name: "PreToolUse".to_string(), + cwd: "/project".to_string(), + session_id: Some("sess-123".to_string()), + event_data: HookEventInput::PreToolUse { + tool_name: "Bash".to_string(), + tool_input: serde_json::json!({"command": "ls"}), + }, + }; + + let actual = serde_json::to_value(&fixture).unwrap(); + + assert_eq!(actual["hook_event_name"], "PreToolUse"); + assert_eq!(actual["cwd"], "/project"); + assert_eq!(actual["tool_name"], "Bash"); + assert_eq!(actual["tool_input"]["command"], "ls"); + } + + #[test] + fn test_hook_input_serialization_stop() { + let fixture = HookInput { + hook_event_name: "Stop".to_string(), + cwd: "/project".to_string(), + session_id: None, + event_data: HookEventInput::Stop { stop_hook_active: false }, + }; + + let actual = serde_json::to_value(&fixture).unwrap(); + + assert_eq!(actual["hook_event_name"], "Stop"); + assert_eq!(actual["stop_hook_active"], false); + } + + #[test] + fn test_hook_output_parse_valid_json() { + let stdout = r#"{"decision": "block", "reason": "unsafe command"}"#; + let actual = HookOutput::parse(stdout); + + assert_eq!(actual.decision, Some("block".to_string())); + assert_eq!(actual.reason, Some("unsafe command".to_string())); + } + + #[test] + fn test_hook_output_parse_empty_string() { + let actual = HookOutput::parse(""); + let expected = HookOutput::default(); + assert_eq!(actual, expected); + } + + #[test] + fn test_hook_output_parse_invalid_json_returns_default() { + let actual = HookOutput::parse("not json at all"); + let expected = HookOutput::default(); + assert_eq!(actual, expected); + } + + #[test] + fn test_hook_output_is_blocking() { + let fixture = HookOutput { + decision: Some("block".to_string()), + ..Default::default() + }; + assert!(fixture.is_blocking()); + + let fixture = HookOutput { + permission_decision: Some("deny".to_string()), + ..Default::default() + }; + assert!(fixture.is_blocking()); + + let fixture = HookOutput::default(); + assert!(!fixture.is_blocking()); + } + + #[test] + fn test_hook_execution_result_blocking() { + let fixture = HookExecutionResult { + exit_code: Some(2), + stdout: String::new(), + stderr: "Blocked: unsafe command".to_string(), + }; + + assert!(fixture.is_blocking_exit()); + assert!(!fixture.is_success()); + assert!(!fixture.is_non_blocking_error()); + assert_eq!( + fixture.blocking_message(), + Some("Blocked: unsafe command") + ); + assert!(fixture.parse_output().is_none()); + } + + #[test] + fn test_hook_execution_result_success() { + let fixture = HookExecutionResult { + exit_code: Some(0), + stdout: r#"{"decision": "block", "reason": "test"}"#.to_string(), + stderr: String::new(), + }; + + assert!(fixture.is_success()); + assert!(!fixture.is_blocking_exit()); + assert!(!fixture.is_non_blocking_error()); + let output = fixture.parse_output().unwrap(); + assert!(output.is_blocking()); + } + + #[test] + fn test_hook_execution_result_non_blocking_error() { + let fixture = HookExecutionResult { + exit_code: Some(1), + stdout: String::new(), + stderr: "some error".to_string(), + }; + + assert!(fixture.is_non_blocking_error()); + assert!(!fixture.is_success()); + assert!(!fixture.is_blocking_exit()); + assert!(fixture.blocking_message().is_none()); + } +} From cd1d97eb962d842172f431d8f862842f21b5126c Mon Sep 17 00:00:00 2001 From: Sandipsinh Rathod <62684960+ssddOnTop@users.noreply.github.com> Date: Tue, 31 Mar 2026 16:33:36 -0400 Subject: [PATCH 02/23] feat(hooks): integrate user-configurable hooks into app lifecycle and handle PreToolUse blocking in orchestrator --- .gitignore | 2 + crates/forge_app/src/app.rs | 30 +- crates/forge_app/src/hooks/mod.rs | 5 + .../forge_app/src/hooks/user_hook_handler.rs | 258 +++++++++++++++--- crates/forge_app/src/orch.rs | 24 +- crates/forge_domain/src/lib.rs | 4 + 6 files changed, 277 insertions(+), 46 deletions(-) diff --git a/.gitignore b/.gitignore index 077bfbece7..66351b131c 100644 --- a/.gitignore +++ b/.gitignore @@ -48,3 +48,5 @@ Cargo.lock **/.forge/request.body.json node_modules/ bench/__pycache__ +/hooksref* +#/cc diff --git a/crates/forge_app/src/app.rs b/crates/forge_app/src/app.rs index 69a83aa447..de56d9ef28 100644 --- a/crates/forge_app/src/app.rs +++ b/crates/forge_app/src/app.rs @@ -8,7 +8,10 @@ use forge_stream::MpscStream; use crate::apply_tunable_parameters::ApplyTunableParameters; use crate::changed_files::ChangedFiles; use crate::dto::ToolsOverview; -use crate::hooks::{CompactionHandler, DoomLoopDetector, TitleGenerationHandler, TracingHandler}; +use crate::hooks::{ + CompactionHandler, DoomLoopDetector, TitleGenerationHandler, TracingHandler, + UserHookConfigLoader, UserHookHandler, +}; use crate::init_conversation_metrics::InitConversationMetrics; use crate::orch::Orchestrator; use crate::services::{AgentRegistry, CustomInstructionsService, ProviderAuthService}; @@ -124,7 +127,7 @@ impl ForgeApp { // Create the orchestrator with all necessary dependencies let tracing_handler = TracingHandler::new(); let title_handler = TitleGenerationHandler::new(services.clone()); - let hook = Hook::default() + let internal_hook = Hook::default() .on_start(tracing_handler.clone().and(title_handler.clone())) .on_request(tracing_handler.clone().and(DoomLoopDetector::default())) .on_response( @@ -136,6 +139,29 @@ impl ForgeApp { .on_toolcall_end(tracing_handler.clone()) .on_end(tracing_handler.and(title_handler)); + // Load user-configurable hooks from settings files + let user_hook_config = + UserHookConfigLoader::load(environment.home.as_deref(), &environment.cwd); + + let hook = if !user_hook_config.is_empty() { + let user_handler = UserHookHandler::new( + user_hook_config, + environment.cwd.clone(), + environment.cwd.clone(), + conversation.id.to_string(), + ); + let user_hook = Hook::default() + .on_start(user_handler.clone()) + .on_request(user_handler.clone()) + .on_response(user_handler.clone()) + .on_toolcall_start(user_handler.clone()) + .on_toolcall_end(user_handler.clone()) + .on_end(user_handler); + internal_hook.zip(user_hook) + } else { + internal_hook + }; + let orch = Orchestrator::new(services.clone(), environment.clone(), conversation, agent) .error_tracker(ToolErrorTracker::new(max_tool_failure_per_turn)) .tool_definitions(tool_definitions) diff --git a/crates/forge_app/src/hooks/mod.rs b/crates/forge_app/src/hooks/mod.rs index fb5447a8e6..712fc5a2a4 100644 --- a/crates/forge_app/src/hooks/mod.rs +++ b/crates/forge_app/src/hooks/mod.rs @@ -2,8 +2,13 @@ mod compaction; mod doom_loop; mod title_generation; mod tracing; +mod user_hook_config_loader; +mod user_hook_executor; +mod user_hook_handler; pub use compaction::CompactionHandler; pub use doom_loop::DoomLoopDetector; pub use title_generation::TitleGenerationHandler; pub use tracing::TracingHandler; +pub use user_hook_config_loader::UserHookConfigLoader; +pub use user_hook_handler::UserHookHandler; diff --git a/crates/forge_app/src/hooks/user_hook_handler.rs b/crates/forge_app/src/hooks/user_hook_handler.rs index 3925b7b57f..c77e97fe8c 100644 --- a/crates/forge_app/src/hooks/user_hook_handler.rs +++ b/crates/forge_app/src/hooks/user_hook_handler.rs @@ -49,10 +49,7 @@ impl UserHookHandler { project_dir.to_string_lossy().to_string(), ); env_vars.insert("FORGE_SESSION_ID".to_string(), session_id); - env_vars.insert( - "FORGE_CWD".to_string(), - cwd.to_string_lossy().to_string(), - ); + env_vars.insert("FORGE_CWD".to_string(), cwd.to_string_lossy().to_string()); Self { config, @@ -77,19 +74,17 @@ impl UserHookHandler { for group in groups { let matches = match (&group.matcher, subject) { - (Some(pattern), Some(subj)) => { - match Regex::new(pattern) { - Ok(re) => re.is_match(subj), - Err(e) => { - warn!( - pattern = pattern, - error = %e, - "Invalid regex in hook matcher, skipping" - ); - false - } + (Some(pattern), Some(subj)) => match Regex::new(pattern) { + Ok(re) => re.is_match(subj), + Err(e) => { + warn!( + pattern = pattern, + error = %e, + "Invalid regex in hook matcher, skipping" + ); + false } - } + }, (Some(_), None) => { // Matcher specified but no subject to match against; skip false @@ -132,7 +127,7 @@ impl UserHookHandler { &self.cwd, &self.env_vars, ) - .await + .await { Ok(result) => results.push(result), Err(e) => { @@ -318,7 +313,7 @@ impl EventHandle> for UserHookHandler { async fn handle( &self, event: &EventData, - conversation: &mut Conversation, + _conversation: &mut Conversation, ) -> anyhow::Result<()> { if !self.has_hooks(&UserHookEventName::PreToolUse) { return Ok(()); @@ -339,10 +334,7 @@ impl EventHandle> for UserHookHandler { hook_event_name: "PreToolUse".to_string(), cwd: self.cwd.to_string_lossy().to_string(), session_id: self.env_vars.get("FORGE_SESSION_ID").cloned(), - event_data: HookEventInput::PreToolUse { - tool_name: tool_name.to_string(), - tool_input, - }, + event_data: HookEventInput::PreToolUse { tool_name: tool_name.to_string(), tool_input }, }; let results = self.execute_hooks(&hooks, &input).await; @@ -366,17 +358,9 @@ impl EventHandle> for UserHookHandler { reason = reason.as_str(), "PreToolUse hook blocked tool call" ); - // Inject a user message with the block reason so the agent sees it - if let Some(context) = conversation.context.as_mut() { - let block_msg = format!( - "\nPreToolUse\n{}\nblocked\n{}\n", - tool_name, reason - ); - context.messages.push( - forge_domain::ContextMessage::user(block_msg, None).into(), - ); - } - // Return an error to signal the orchestrator to skip this tool call + // Return an error to signal the orchestrator to skip this tool call. + // The orchestrator converts this into an error ToolResult visible to + // the model. Err(anyhow::anyhow!( "Tool call '{}' blocked by PreToolUse hook: {}", tool_name, @@ -444,9 +428,9 @@ impl EventHandle> for UserHookHandler { "\n{}\n{}\nblocked\n{}\n", event_name, tool_name, reason ); - context.messages.push( - forge_domain::ContextMessage::user(feedback_msg, None).into(), - ); + context + .messages + .push(forge_domain::ContextMessage::user(feedback_msg, None).into()); } } @@ -529,3 +513,205 @@ impl EventHandle> for UserHookHandler { Ok(()) } } + +#[cfg(test)] +mod tests { + use forge_domain::{ + HookExecutionResult, UserHookEntry, UserHookEventName, UserHookMatcherGroup, UserHookType, + }; + use pretty_assertions::assert_eq; + + use super::*; + + fn make_entry(command: &str) -> UserHookEntry { + UserHookEntry { + hook_type: UserHookType::Command, + command: Some(command.to_string()), + timeout: None, + } + } + + fn make_group(matcher: Option<&str>, commands: &[&str]) -> UserHookMatcherGroup { + UserHookMatcherGroup { + matcher: matcher.map(|s| s.to_string()), + hooks: commands.iter().map(|c| make_entry(c)).collect(), + } + } + + #[test] + fn test_find_matching_hooks_no_matcher_fires_unconditionally() { + let groups = vec![make_group(None, &["echo hi"])]; + let actual = UserHookHandler::find_matching_hooks(&groups, Some("Bash")); + assert_eq!(actual.len(), 1); + assert_eq!(actual[0].command, Some("echo hi".to_string())); + } + + #[test] + fn test_find_matching_hooks_no_matcher_fires_without_subject() { + let groups = vec![make_group(None, &["echo hi"])]; + let actual = UserHookHandler::find_matching_hooks(&groups, None); + assert_eq!(actual.len(), 1); + } + + #[test] + fn test_find_matching_hooks_regex_match() { + let groups = vec![make_group(Some("Bash"), &["block.sh"])]; + let actual = UserHookHandler::find_matching_hooks(&groups, Some("Bash")); + assert_eq!(actual.len(), 1); + } + + #[test] + fn test_find_matching_hooks_regex_no_match() { + let groups = vec![make_group(Some("Bash"), &["block.sh"])]; + let actual = UserHookHandler::find_matching_hooks(&groups, Some("Write")); + assert!(actual.is_empty()); + } + + #[test] + fn test_find_matching_hooks_regex_partial_match() { + let groups = vec![make_group(Some("Bash|Write"), &["check.sh"])]; + let actual = UserHookHandler::find_matching_hooks(&groups, Some("Bash")); + assert_eq!(actual.len(), 1); + } + + #[test] + fn test_find_matching_hooks_matcher_but_no_subject() { + let groups = vec![make_group(Some("Bash"), &["block.sh"])]; + let actual = UserHookHandler::find_matching_hooks(&groups, None); + assert!(actual.is_empty()); + } + + #[test] + fn test_find_matching_hooks_invalid_regex_skipped() { + let groups = vec![make_group(Some("[invalid"), &["block.sh"])]; + let actual = UserHookHandler::find_matching_hooks(&groups, Some("anything")); + assert!(actual.is_empty()); + } + + #[test] + fn test_find_matching_hooks_multiple_groups() { + let groups = vec![ + make_group(Some("Bash"), &["bash-hook.sh"]), + make_group(Some("Write"), &["write-hook.sh"]), + make_group(None, &["always.sh"]), + ]; + let actual = UserHookHandler::find_matching_hooks(&groups, Some("Bash")); + assert_eq!(actual.len(), 2); // Bash match + unconditional + } + + #[test] + fn test_process_pre_tool_use_output_allow_on_success() { + let results = vec![HookExecutionResult { + exit_code: Some(0), + stdout: String::new(), + stderr: String::new(), + }]; + let actual = UserHookHandler::process_pre_tool_use_output(&results); + assert!(matches!(actual, PreToolUseDecision::Allow)); + } + + #[test] + fn test_process_pre_tool_use_output_block_on_exit_2() { + let results = vec![HookExecutionResult { + exit_code: Some(2), + stdout: String::new(), + stderr: "Blocked: dangerous command".to_string(), + }]; + let actual = UserHookHandler::process_pre_tool_use_output(&results); + assert!( + matches!(actual, PreToolUseDecision::Block(msg) if msg.contains("dangerous command")) + ); + } + + #[test] + fn test_process_pre_tool_use_output_block_on_deny() { + let results = vec![HookExecutionResult { + exit_code: Some(0), + stdout: r#"{"permissionDecision": "deny", "reason": "Not allowed"}"#.to_string(), + stderr: String::new(), + }]; + let actual = UserHookHandler::process_pre_tool_use_output(&results); + assert!(matches!(actual, PreToolUseDecision::Block(msg) if msg == "Not allowed")); + } + + #[test] + fn test_process_pre_tool_use_output_block_on_decision() { + let results = vec![HookExecutionResult { + exit_code: Some(0), + stdout: r#"{"decision": "block", "reason": "Blocked by policy"}"#.to_string(), + stderr: String::new(), + }]; + let actual = UserHookHandler::process_pre_tool_use_output(&results); + assert!(matches!(actual, PreToolUseDecision::Block(msg) if msg == "Blocked by policy")); + } + + #[test] + fn test_process_pre_tool_use_output_non_blocking_error_allows() { + let results = vec![HookExecutionResult { + exit_code: Some(1), + stdout: String::new(), + stderr: "some error".to_string(), + }]; + let actual = UserHookHandler::process_pre_tool_use_output(&results); + assert!(matches!(actual, PreToolUseDecision::Allow)); + } + + #[test] + fn test_process_results_no_blocking() { + let results = vec![HookExecutionResult { + exit_code: Some(0), + stdout: String::new(), + stderr: String::new(), + }]; + let actual = UserHookHandler::process_results(&results); + assert!(actual.is_none()); + } + + #[test] + fn test_process_results_blocking_exit_code() { + let results = vec![HookExecutionResult { + exit_code: Some(2), + stdout: String::new(), + stderr: "stop reason".to_string(), + }]; + let actual = UserHookHandler::process_results(&results); + assert_eq!(actual, Some("stop reason".to_string())); + } + + #[test] + fn test_process_results_blocking_json_decision() { + let results = vec![HookExecutionResult { + exit_code: Some(0), + stdout: r#"{"decision": "block", "reason": "keep going"}"#.to_string(), + stderr: String::new(), + }]; + let actual = UserHookHandler::process_results(&results); + assert_eq!(actual, Some("keep going".to_string())); + } + + #[test] + fn test_has_hooks_returns_false_for_empty_config() { + let config = UserHookConfig::new(); + let handler = UserHookHandler::new( + config, + PathBuf::from("/tmp"), + PathBuf::from("/tmp"), + "sess-1".to_string(), + ); + assert!(!handler.has_hooks(&UserHookEventName::PreToolUse)); + } + + #[test] + fn test_has_hooks_returns_true_when_configured() { + let json = r#"{"PreToolUse": [{"hooks": [{"type": "command", "command": "echo hi"}]}]}"#; + let config: UserHookConfig = serde_json::from_str(json).unwrap(); + let handler = UserHookHandler::new( + config, + PathBuf::from("/tmp"), + PathBuf::from("/tmp"), + "sess-1".to_string(), + ); + assert!(handler.has_hooks(&UserHookEventName::PreToolUse)); + assert!(!handler.has_hooks(&UserHookEventName::Stop)); + } +} diff --git a/crates/forge_app/src/orch.rs b/crates/forge_app/src/orch.rs index 853118dd66..a3a16642f8 100644 --- a/crates/forge_app/src/orch.rs +++ b/crates/forge_app/src/orch.rs @@ -83,22 +83,30 @@ impl Orchestrator { notifier.notified().await; } - // Fire the ToolcallStart lifecycle event + // Fire the ToolcallStart lifecycle event. + // If a hook returns an error (e.g., PreToolUse hook blocked the + // call), skip execution and record an error result instead. let toolcall_start_event = LifecycleEvent::ToolcallStart(EventData::new( self.agent.clone(), self.agent.model.clone(), ToolcallStartPayload::new(tool_call.clone()), )); - self.hook + let hook_result = self + .hook .handle(&toolcall_start_event, &mut self.conversation) - .await?; - - // Execute the tool - let tool_result = self - .services - .call(&self.agent, tool_context, tool_call.clone()) .await; + let tool_result = if let Err(hook_err) = hook_result { + // Hook blocked this tool call — produce an error ToolResult + // so the model sees feedback without aborting the session. + ToolResult::from(tool_call.clone()).failure(hook_err) + } else { + // Execute the tool normally + self.services + .call(&self.agent, tool_context, tool_call.clone()) + .await + }; + // Fire the ToolcallEnd lifecycle event (fires on both success and failure) let toolcall_end_event = LifecycleEvent::ToolcallEnd(EventData::new( self.agent.clone(), diff --git a/crates/forge_domain/src/lib.rs b/crates/forge_domain/src/lib.rs index e8b4e74a99..2433fe9ba8 100644 --- a/crates/forge_domain/src/lib.rs +++ b/crates/forge_domain/src/lib.rs @@ -53,6 +53,8 @@ mod top_k; mod top_p; mod transformer; mod update; +mod user_hook_config; +mod user_hook_io; mod validation; mod workspace; mod xml; @@ -110,6 +112,8 @@ pub use top_k::*; pub use top_p::*; pub use transformer::*; pub use update::*; +pub use user_hook_config::*; +pub use user_hook_io::*; pub use validation::*; pub use workspace::*; pub use xml::*; From 8b8747c146b727c72eef88ce2cc6c4d413f93b0f Mon Sep 17 00:00:00 2001 From: "autofix-ci[bot]" <114827586+autofix-ci[bot]@users.noreply.github.com> Date: Tue, 31 Mar 2026 20:35:47 +0000 Subject: [PATCH 03/23] [autofix.ci] apply automated fixes --- .../src/hooks/user_hook_config_loader.rs | 3 +- .../forge_app/src/hooks/user_hook_executor.rs | 36 ++++++------------- .../forge_app/src/hooks/user_hook_handler.rs | 12 +++---- crates/forge_domain/src/user_hook_config.rs | 24 ++++++------- crates/forge_domain/src/user_hook_io.rs | 10 ++---- 5 files changed, 28 insertions(+), 57 deletions(-) diff --git a/crates/forge_app/src/hooks/user_hook_config_loader.rs b/crates/forge_app/src/hooks/user_hook_config_loader.rs index 7c24b70e05..519b8eb4ef 100644 --- a/crates/forge_app/src/hooks/user_hook_config_loader.rs +++ b/crates/forge_app/src/hooks/user_hook_config_loader.rs @@ -187,8 +187,7 @@ mod tests { ) .unwrap(); - let actual = - UserHookConfigLoader::load(Some(home_dir.path()), project_dir.path()); + let actual = UserHookConfigLoader::load(Some(home_dir.path()), project_dir.path()); // PreToolUse should have 2 groups (global + project) assert_eq!( diff --git a/crates/forge_app/src/hooks/user_hook_executor.rs b/crates/forge_app/src/hooks/user_hook_executor.rs index 2faf677d3c..4c453f4471 100644 --- a/crates/forge_app/src/hooks/user_hook_executor.rs +++ b/crates/forge_app/src/hooks/user_hook_executor.rs @@ -134,10 +134,9 @@ mod tests { #[tokio::test] async fn test_execute_simple_command() { let cwd = std::env::current_dir().unwrap(); - let actual = - UserHookExecutor::execute("echo hello", "{}", None, &cwd, &HashMap::new()) - .await - .unwrap(); + let actual = UserHookExecutor::execute("echo hello", "{}", None, &cwd, &HashMap::new()) + .await + .unwrap(); assert_eq!(actual.exit_code, Some(0)); assert_eq!(actual.stdout.trim(), "hello"); @@ -182,15 +181,9 @@ mod tests { #[tokio::test] async fn test_execute_non_blocking_error() { let cwd = std::env::current_dir().unwrap(); - let actual = UserHookExecutor::execute( - "exit 1", - "{}", - None, - &cwd, - &HashMap::new(), - ) - .await - .unwrap(); + let actual = UserHookExecutor::execute("exit 1", "{}", None, &cwd, &HashMap::new()) + .await + .unwrap(); assert_eq!(actual.exit_code, Some(1)); assert!(actual.is_non_blocking_error()); @@ -218,20 +211,11 @@ mod tests { async fn test_execute_with_env_vars() { let cwd = std::env::current_dir().unwrap(); let mut env_vars = HashMap::new(); - env_vars.insert( - "FORGE_TEST_VAR".to_string(), - "test_value".to_string(), - ); + env_vars.insert("FORGE_TEST_VAR".to_string(), "test_value".to_string()); - let actual = UserHookExecutor::execute( - "echo $FORGE_TEST_VAR", - "{}", - None, - &cwd, - &env_vars, - ) - .await - .unwrap(); + let actual = UserHookExecutor::execute("echo $FORGE_TEST_VAR", "{}", None, &cwd, &env_vars) + .await + .unwrap(); assert_eq!(actual.exit_code, Some(0)); assert_eq!(actual.stdout.trim(), "test_value"); diff --git a/crates/forge_app/src/hooks/user_hook_handler.rs b/crates/forge_app/src/hooks/user_hook_handler.rs index c77e97fe8c..58efa0c003 100644 --- a/crates/forge_app/src/hooks/user_hook_handler.rs +++ b/crates/forge_app/src/hooks/user_hook_handler.rs @@ -127,7 +127,7 @@ impl UserHookHandler { &self.cwd, &self.env_vars, ) - .await + .await { Ok(result) => results.push(result), Err(e) => { @@ -157,14 +157,13 @@ impl UserHookHandler { } // Exit code 0 = check stdout for JSON decisions - if let Some(output) = result.parse_output() { - if output.is_blocking() { + if let Some(output) = result.parse_output() + && output.is_blocking() { let reason = output .reason .unwrap_or_else(|| "Hook blocked execution".to_string()); return Some(reason); } - } // Non-blocking errors (exit code 1, etc.) are logged but don't block if result.is_non_blocking_error() { @@ -270,14 +269,13 @@ impl EventHandle> for UserHookHandler { // SessionStart hooks can provide additional context but not block for result in &results { - if let Some(output) = result.parse_output() { - if let Some(context) = &output.additional_context { + if let Some(output) = result.parse_output() + && let Some(context) = &output.additional_context { debug!( context_len = context.len(), "SessionStart hook provided additional context" ); } - } } Ok(()) diff --git a/crates/forge_domain/src/user_hook_config.rs b/crates/forge_domain/src/user_hook_config.rs index ae3dbb08b8..306e0bc088 100644 --- a/crates/forge_domain/src/user_hook_config.rs +++ b/crates/forge_domain/src/user_hook_config.rs @@ -6,7 +6,8 @@ use serde::{Deserialize, Serialize}; /// Top-level user hook configuration. /// /// Maps hook event names to a list of matcher groups. This is deserialized -/// from the `"hooks"` key in `.forge/settings.json` or `~/.forge/settings.json`. +/// from the `"hooks"` key in `.forge/settings.json` or +/// `~/.forge/settings.json`. /// /// Example JSON: /// ```json @@ -207,16 +208,14 @@ mod tests { let actual: UserHookConfig = serde_json::from_str(json).unwrap(); - assert_eq!( - actual.get_groups(&UserHookEventName::PreToolUse).len(), - 1 - ); - assert_eq!( - actual.get_groups(&UserHookEventName::PostToolUse).len(), - 1 - ); + assert_eq!(actual.get_groups(&UserHookEventName::PreToolUse).len(), 1); + assert_eq!(actual.get_groups(&UserHookEventName::PostToolUse).len(), 1); assert_eq!(actual.get_groups(&UserHookEventName::Stop).len(), 1); - assert!(actual.get_groups(&UserHookEventName::SessionStart).is_empty()); + assert!( + actual + .get_groups(&UserHookEventName::SessionStart) + .is_empty() + ); } #[test] @@ -257,10 +256,7 @@ mod tests { let config2: UserHookConfig = serde_json::from_str(json2).unwrap(); actual.merge(config2); - assert_eq!( - actual.get_groups(&UserHookEventName::PreToolUse).len(), - 2 - ); + assert_eq!(actual.get_groups(&UserHookEventName::PreToolUse).len(), 2); assert_eq!(actual.get_groups(&UserHookEventName::Stop).len(), 1); } diff --git a/crates/forge_domain/src/user_hook_io.rs b/crates/forge_domain/src/user_hook_io.rs index 38fe4c4df3..115f62a673 100644 --- a/crates/forge_domain/src/user_hook_io.rs +++ b/crates/forge_domain/src/user_hook_io.rs @@ -248,10 +248,7 @@ mod tests { #[test] fn test_hook_output_is_blocking() { - let fixture = HookOutput { - decision: Some("block".to_string()), - ..Default::default() - }; + let fixture = HookOutput { decision: Some("block".to_string()), ..Default::default() }; assert!(fixture.is_blocking()); let fixture = HookOutput { @@ -275,10 +272,7 @@ mod tests { assert!(fixture.is_blocking_exit()); assert!(!fixture.is_success()); assert!(!fixture.is_non_blocking_error()); - assert_eq!( - fixture.blocking_message(), - Some("Blocked: unsafe command") - ); + assert_eq!(fixture.blocking_message(), Some("Blocked: unsafe command")); assert!(fixture.parse_output().is_none()); } From e831ba4e23eb18bcfebb651a52ba975fdbd01306 Mon Sep 17 00:00:00 2001 From: "autofix-ci[bot]" <114827586+autofix-ci[bot]@users.noreply.github.com> Date: Tue, 31 Mar 2026 20:37:35 +0000 Subject: [PATCH 04/23] [autofix.ci] apply automated fixes (attempt 2/3) --- .../forge_app/src/hooks/user_hook_handler.rs | 26 ++++++++++--------- 1 file changed, 14 insertions(+), 12 deletions(-) diff --git a/crates/forge_app/src/hooks/user_hook_handler.rs b/crates/forge_app/src/hooks/user_hook_handler.rs index 58efa0c003..4f5dbd0023 100644 --- a/crates/forge_app/src/hooks/user_hook_handler.rs +++ b/crates/forge_app/src/hooks/user_hook_handler.rs @@ -158,12 +158,13 @@ impl UserHookHandler { // Exit code 0 = check stdout for JSON decisions if let Some(output) = result.parse_output() - && output.is_blocking() { - let reason = output - .reason - .unwrap_or_else(|| "Hook blocked execution".to_string()); - return Some(reason); - } + && output.is_blocking() + { + let reason = output + .reason + .unwrap_or_else(|| "Hook blocked execution".to_string()); + return Some(reason); + } // Non-blocking errors (exit code 1, etc.) are logged but don't block if result.is_non_blocking_error() { @@ -270,12 +271,13 @@ impl EventHandle> for UserHookHandler { // SessionStart hooks can provide additional context but not block for result in &results { if let Some(output) = result.parse_output() - && let Some(context) = &output.additional_context { - debug!( - context_len = context.len(), - "SessionStart hook provided additional context" - ); - } + && let Some(context) = &output.additional_context + { + debug!( + context_len = context.len(), + "SessionStart hook provided additional context" + ); + } } Ok(()) From 9d96e9b39c51b3a32f9717e0ef3f23e88a17bad3 Mon Sep 17 00:00:00 2001 From: Sandipsinh Dilipsinh Rathod <62684960+ssddOnTop@users.noreply.github.com> Date: Tue, 31 Mar 2026 23:51:03 -0400 Subject: [PATCH 05/23] feat(hooks): add user hook config service with multi-source merge logic --- crates/forge_services/src/user_hook_config.rs | 269 ++++++++++++++++++ 1 file changed, 269 insertions(+) create mode 100644 crates/forge_services/src/user_hook_config.rs diff --git a/crates/forge_services/src/user_hook_config.rs b/crates/forge_services/src/user_hook_config.rs new file mode 100644 index 0000000000..eba90d388d --- /dev/null +++ b/crates/forge_services/src/user_hook_config.rs @@ -0,0 +1,269 @@ +use std::path::Path; +use std::sync::Arc; + +use forge_app::{EnvironmentInfra, FileReaderInfra}; +use forge_domain::{UserHookConfig, UserSettings}; +use tracing::{debug, warn}; + +/// Loads and merges user hook configurations from the three settings file +/// locations using infrastructure abstractions. +/// +/// Resolution order (all merged, not overridden): +/// 1. `~/.forge/settings.json` (user-level, applies to all projects) +/// 2. `.forge/settings.json` (project-level, committable) +/// 3. `.forge/settings.local.json` (project-level, gitignored) +pub struct ForgeUserHookConfigService(Arc); + +impl ForgeUserHookConfigService { + /// Creates a new service with the given infrastructure dependency. + pub fn new(infra: Arc) -> Self { + Self(infra) + } +} + +impl ForgeUserHookConfigService { + /// Loads a single settings file and extracts hook configuration. + /// + /// Returns `None` if the file doesn't exist or is invalid. + async fn load_file(&self, path: &Path) -> Option { + let contents = match self.0.read_utf8(path).await { + Ok(c) => c, + Err(_) => return None, + }; + + match serde_json::from_str::(&contents) { + Ok(settings) => { + if settings.hooks.is_empty() { + None + } else { + Some(settings.hooks) + } + } + Err(e) => { + warn!( + path = %path.display(), + error = %e, + "Failed to parse settings file for hooks" + ); + None + } + } + } +} + +#[async_trait::async_trait] +impl forge_app::UserHookConfigService + for ForgeUserHookConfigService +{ + async fn get_user_hook_config(&self) -> anyhow::Result { + let env = self.0.get_environment(); + let mut config = UserHookConfig::new(); + + // 1. User-level: ~/.forge/settings.json + if let Some(home) = &env.home { + let user_settings_path = home.join("forge").join("settings.json"); + if let Some(user_config) = self.load_file(&user_settings_path).await { + debug!(path = %user_settings_path.display(), "Loaded user-level hook config"); + config.merge(user_config); + } + } + + // 2. Project-level: .forge/settings.json + let project_settings_path = env.cwd.join(".forge").join("settings.json"); + if let Some(project_config) = self.load_file(&project_settings_path).await { + debug!(path = %project_settings_path.display(), "Loaded project-level hook config"); + config.merge(project_config); + } + + // 3. Project-local: .forge/settings.local.json + let local_settings_path = env.cwd.join(".forge").join("settings.local.json"); + if let Some(local_config) = self.load_file(&local_settings_path).await { + debug!(path = %local_settings_path.display(), "Loaded project-local hook config"); + config.merge(local_config); + } + + if !config.is_empty() { + debug!( + event_count = config.events.len(), + "Merged user hook configuration" + ); + } + + Ok(config) + } +} + +#[cfg(test)] +mod tests { + use std::path::PathBuf; + + use pretty_assertions::assert_eq; + + use super::*; + + #[tokio::test] + async fn test_load_file_valid_settings() { + let dir = tempfile::tempdir().unwrap(); + let settings_path = dir.path().join("settings.json"); + std::fs::write( + &settings_path, + r#"{ + "hooks": { + "PreToolUse": [ + { "matcher": "Bash", "hooks": [{ "type": "command", "command": "check.sh" }] } + ] + } + }"#, + ) + .unwrap(); + + let infra = TestInfra { + home: None, + cwd: PathBuf::from("/nonexistent"), + }; + let service = ForgeUserHookConfigService::new(Arc::new(infra)); + + let actual = service.load_file(&settings_path).await; + assert!(actual.is_some()); + let config = actual.unwrap(); + assert_eq!( + config + .get_groups(&forge_domain::UserHookEventName::PreToolUse) + .len(), + 1 + ); + } + + #[tokio::test] + async fn test_load_file_settings_without_hooks() { + let dir = tempfile::tempdir().unwrap(); + let settings_path = dir.path().join("settings.json"); + std::fs::write(&settings_path, r#"{"other_key": "value"}"#).unwrap(); + + let infra = TestInfra { + home: None, + cwd: PathBuf::from("/nonexistent"), + }; + let service = ForgeUserHookConfigService::new(Arc::new(infra)); + + let actual = service.load_file(&settings_path).await; + assert!(actual.is_none()); + } + + #[tokio::test] + async fn test_get_user_hook_config_nonexistent_paths() { + let infra = TestInfra { + home: Some(PathBuf::from("/nonexistent/home")), + cwd: PathBuf::from("/nonexistent/project"), + }; + let service = ForgeUserHookConfigService::new(Arc::new(infra)); + + let actual = service.get_user_hook_config().await.unwrap(); + assert!(actual.is_empty()); + } + + #[tokio::test] + async fn test_get_user_hook_config_merges_all_sources() { + // Set up a fake home directory + let home_dir = tempfile::tempdir().unwrap(); + let forge_dir = home_dir.path().join("forge"); + std::fs::create_dir_all(&forge_dir).unwrap(); + std::fs::write( + forge_dir.join("settings.json"), + r#"{ + "hooks": { + "PreToolUse": [ + { "matcher": "Bash", "hooks": [{ "type": "command", "command": "global.sh" }] } + ] + } + }"#, + ) + .unwrap(); + + // Set up a project directory + let project_dir = tempfile::tempdir().unwrap(); + let project_forge_dir = project_dir.path().join(".forge"); + std::fs::create_dir_all(&project_forge_dir).unwrap(); + std::fs::write( + project_forge_dir.join("settings.json"), + r#"{ + "hooks": { + "PreToolUse": [ + { "matcher": "Write", "hooks": [{ "type": "command", "command": "project.sh" }] } + ] + } + }"#, + ) + .unwrap(); + std::fs::write( + project_forge_dir.join("settings.local.json"), + r#"{ + "hooks": { + "Stop": [ + { "hooks": [{ "type": "command", "command": "local-stop.sh" }] } + ] + } + }"#, + ) + .unwrap(); + + let infra = TestInfra { + home: Some(home_dir.path().to_path_buf()), + cwd: project_dir.path().to_path_buf(), + }; + let service = ForgeUserHookConfigService::new(Arc::new(infra)); + + let actual = service.get_user_hook_config().await.unwrap(); + + // PreToolUse should have 2 groups (global + project) + assert_eq!( + actual + .get_groups(&forge_domain::UserHookEventName::PreToolUse) + .len(), + 2 + ); + // Stop should have 1 group (local) + assert_eq!( + actual + .get_groups(&forge_domain::UserHookEventName::Stop) + .len(), + 1 + ); + } + + // --- Test infrastructure --- + + struct TestInfra { + home: Option, + cwd: PathBuf, + } + + #[async_trait::async_trait] + impl FileReaderInfra for TestInfra { + async fn read_utf8(&self, path: &Path) -> anyhow::Result { + Ok(tokio::fs::read_to_string(path).await?) + } + + async fn read_utf8_batch( + &self, + _paths: Vec, + _batch_size: usize, + ) -> forge_stream::MpscStream<(PathBuf, anyhow::Result)> { + unimplemented!("not needed for tests") + } + } + + impl EnvironmentInfra for TestInfra { + fn get_env_var(&self, _key: &str) -> Option { + None + } + fn get_env_vars(&self) -> std::collections::BTreeMap { + Default::default() + } + fn get_environment(&self) -> forge_domain::Environment { + forge_domain::Environment::default() + .home(self.home.clone()) + .cwd(self.cwd.clone()) + } + } +} From cb3e3e2eee6594a1c20d4ddd07e205e91fab6304 Mon Sep 17 00:00:00 2001 From: Sandipsinh Dilipsinh Rathod <62684960+ssddOnTop@users.noreply.github.com> Date: Tue, 31 Mar 2026 23:51:08 -0400 Subject: [PATCH 06/23] feat(hooks): add configurable hook timeout via hook_timeout_ms config --- crates/forge_app/src/app.rs | 11 +- crates/forge_app/src/hooks/mod.rs | 2 - .../src/hooks/user_hook_config_loader.rs | 207 ------------------ .../forge_app/src/hooks/user_hook_executor.rs | 36 ++- .../forge_app/src/hooks/user_hook_handler.rs | 9 + crates/forge_app/src/orch_spec/orch_setup.rs | 1 + crates/forge_app/src/services.rs | 23 ++ crates/forge_config/.forge.toml | 1 + crates/forge_config/src/config.rs | 4 + crates/forge_domain/src/env.rs | 4 + crates/forge_infra/src/env.rs | 2 + crates/forge_main/src/info.rs | 1 + crates/forge_services/src/forge_services.rs | 9 + crates/forge_services/src/lib.rs | 1 + crates/forge_services/src/user_hook_config.rs | 76 ++++--- forge.schema.json | 9 + 16 files changed, 145 insertions(+), 251 deletions(-) delete mode 100644 crates/forge_app/src/hooks/user_hook_config_loader.rs diff --git a/crates/forge_app/src/app.rs b/crates/forge_app/src/app.rs index de56d9ef28..a9434e2f79 100644 --- a/crates/forge_app/src/app.rs +++ b/crates/forge_app/src/app.rs @@ -9,12 +9,13 @@ use crate::apply_tunable_parameters::ApplyTunableParameters; use crate::changed_files::ChangedFiles; use crate::dto::ToolsOverview; use crate::hooks::{ - CompactionHandler, DoomLoopDetector, TitleGenerationHandler, TracingHandler, - UserHookConfigLoader, UserHookHandler, + CompactionHandler, DoomLoopDetector, TitleGenerationHandler, TracingHandler, UserHookHandler, }; use crate::init_conversation_metrics::InitConversationMetrics; use crate::orch::Orchestrator; -use crate::services::{AgentRegistry, CustomInstructionsService, ProviderAuthService}; +use crate::services::{ + AgentRegistry, CustomInstructionsService, ProviderAuthService, UserHookConfigService, +}; use crate::set_conversation_id::SetConversationId; use crate::system_prompt::SystemPrompt; use crate::tool_registry::ToolRegistry; @@ -140,8 +141,7 @@ impl ForgeApp { .on_end(tracing_handler.and(title_handler)); // Load user-configurable hooks from settings files - let user_hook_config = - UserHookConfigLoader::load(environment.home.as_deref(), &environment.cwd); + let user_hook_config = services.get_user_hook_config().await?; let hook = if !user_hook_config.is_empty() { let user_handler = UserHookHandler::new( @@ -149,6 +149,7 @@ impl ForgeApp { environment.cwd.clone(), environment.cwd.clone(), conversation.id.to_string(), + environment.hook_timeout, ); let user_hook = Hook::default() .on_start(user_handler.clone()) diff --git a/crates/forge_app/src/hooks/mod.rs b/crates/forge_app/src/hooks/mod.rs index 712fc5a2a4..cdc5c8f0af 100644 --- a/crates/forge_app/src/hooks/mod.rs +++ b/crates/forge_app/src/hooks/mod.rs @@ -2,7 +2,6 @@ mod compaction; mod doom_loop; mod title_generation; mod tracing; -mod user_hook_config_loader; mod user_hook_executor; mod user_hook_handler; @@ -10,5 +9,4 @@ pub use compaction::CompactionHandler; pub use doom_loop::DoomLoopDetector; pub use title_generation::TitleGenerationHandler; pub use tracing::TracingHandler; -pub use user_hook_config_loader::UserHookConfigLoader; pub use user_hook_handler::UserHookHandler; diff --git a/crates/forge_app/src/hooks/user_hook_config_loader.rs b/crates/forge_app/src/hooks/user_hook_config_loader.rs deleted file mode 100644 index 519b8eb4ef..0000000000 --- a/crates/forge_app/src/hooks/user_hook_config_loader.rs +++ /dev/null @@ -1,207 +0,0 @@ -use std::path::Path; - -use forge_domain::{UserHookConfig, UserSettings}; -use tracing::{debug, warn}; - -/// Loads and merges user hook configurations from the three settings file -/// locations. -/// -/// Resolution order (all merged, not overridden): -/// 1. `~/.forge/settings.json` (user-level, applies to all projects) -/// 2. `.forge/settings.json` (project-level, committable) -/// 3. `.forge/settings.local.json` (project-level, gitignored) -pub struct UserHookConfigLoader; - -impl UserHookConfigLoader { - /// Loads and merges hook configurations from all settings files. - /// - /// # Arguments - /// * `home` - Home directory (e.g., `/Users/name`). If `None`, user-level - /// settings are skipped. - /// * `cwd` - Current working directory for project-level settings. - /// - /// # Errors - /// This function does not return errors. Invalid or missing files are - /// silently skipped with a debug log. - pub fn load(home: Option<&Path>, cwd: &Path) -> UserHookConfig { - let mut config = UserHookConfig::new(); - - // 1. User-level: ~/.forge/settings.json - if let Some(home) = home { - let user_settings_path = home.join("forge").join("settings.json"); - if let Some(user_config) = Self::load_file(&user_settings_path) { - debug!(path = %user_settings_path.display(), "Loaded user-level hook config"); - config.merge(user_config); - } - } - - // 2. Project-level: .forge/settings.json - let project_settings_path = cwd.join(".forge").join("settings.json"); - if let Some(project_config) = Self::load_file(&project_settings_path) { - debug!(path = %project_settings_path.display(), "Loaded project-level hook config"); - config.merge(project_config); - } - - // 3. Project-local: .forge/settings.local.json - let local_settings_path = cwd.join(".forge").join("settings.local.json"); - if let Some(local_config) = Self::load_file(&local_settings_path) { - debug!(path = %local_settings_path.display(), "Loaded project-local hook config"); - config.merge(local_config); - } - - if !config.is_empty() { - debug!( - event_count = config.events.len(), - "Merged user hook configuration" - ); - } - - config - } - - /// Loads a single settings file and extracts hook configuration. - /// - /// Returns `None` if the file doesn't exist or is invalid. - fn load_file(path: &Path) -> Option { - let contents = match std::fs::read_to_string(path) { - Ok(c) => c, - Err(_) => return None, - }; - - match serde_json::from_str::(&contents) { - Ok(settings) => { - if settings.hooks.is_empty() { - None - } else { - Some(settings.hooks) - } - } - Err(e) => { - warn!( - path = %path.display(), - error = %e, - "Failed to parse settings file for hooks" - ); - None - } - } - } -} - -#[cfg(test)] -mod tests { - use std::path::PathBuf; - - use pretty_assertions::assert_eq; - - use super::*; - - #[test] - fn test_load_nonexistent_paths() { - let home = PathBuf::from("/nonexistent/home"); - let cwd = PathBuf::from("/nonexistent/project"); - - let actual = UserHookConfigLoader::load(Some(&home), &cwd); - assert!(actual.is_empty()); - } - - #[test] - fn test_load_file_valid_settings() { - let dir = tempfile::tempdir().unwrap(); - let settings_path = dir.path().join("settings.json"); - std::fs::write( - &settings_path, - r#"{ - "hooks": { - "PreToolUse": [ - { "matcher": "Bash", "hooks": [{ "type": "command", "command": "check.sh" }] } - ] - } - }"#, - ) - .unwrap(); - - let actual = UserHookConfigLoader::load_file(&settings_path); - assert!(actual.is_some()); - let config = actual.unwrap(); - assert_eq!( - config - .get_groups(&forge_domain::UserHookEventName::PreToolUse) - .len(), - 1 - ); - } - - #[test] - fn test_load_file_settings_without_hooks() { - let dir = tempfile::tempdir().unwrap(); - let settings_path = dir.path().join("settings.json"); - std::fs::write(&settings_path, r#"{"other_key": "value"}"#).unwrap(); - - let actual = UserHookConfigLoader::load_file(&settings_path); - assert!(actual.is_none()); - } - - #[test] - fn test_load_merges_all_sources() { - // Set up a fake home directory - let home_dir = tempfile::tempdir().unwrap(); - let forge_dir = home_dir.path().join("forge"); - std::fs::create_dir_all(&forge_dir).unwrap(); - std::fs::write( - forge_dir.join("settings.json"), - r#"{ - "hooks": { - "PreToolUse": [ - { "matcher": "Bash", "hooks": [{ "type": "command", "command": "global.sh" }] } - ] - } - }"#, - ) - .unwrap(); - - // Set up a project directory - let project_dir = tempfile::tempdir().unwrap(); - let project_forge_dir = project_dir.path().join(".forge"); - std::fs::create_dir_all(&project_forge_dir).unwrap(); - std::fs::write( - project_forge_dir.join("settings.json"), - r#"{ - "hooks": { - "PreToolUse": [ - { "matcher": "Write", "hooks": [{ "type": "command", "command": "project.sh" }] } - ] - } - }"#, - ) - .unwrap(); - std::fs::write( - project_forge_dir.join("settings.local.json"), - r#"{ - "hooks": { - "Stop": [ - { "hooks": [{ "type": "command", "command": "local-stop.sh" }] } - ] - } - }"#, - ) - .unwrap(); - - let actual = UserHookConfigLoader::load(Some(home_dir.path()), project_dir.path()); - - // PreToolUse should have 2 groups (global + project) - assert_eq!( - actual - .get_groups(&forge_domain::UserHookEventName::PreToolUse) - .len(), - 2 - ); - // Stop should have 1 group (local) - assert_eq!( - actual - .get_groups(&forge_domain::UserHookEventName::Stop) - .len(), - 1 - ); - } -} diff --git a/crates/forge_app/src/hooks/user_hook_executor.rs b/crates/forge_app/src/hooks/user_hook_executor.rs index 4c453f4471..e845c7dd40 100644 --- a/crates/forge_app/src/hooks/user_hook_executor.rs +++ b/crates/forge_app/src/hooks/user_hook_executor.rs @@ -22,8 +22,11 @@ impl UserHookExecutor { /// # Arguments /// * `command` - The shell command string to execute. /// * `input_json` - JSON string to pipe to the command's stdin. - /// * `timeout` - Optional timeout in milliseconds. Uses default (10 min) if - /// `None`. + /// * `timeout` - Optional per-hook timeout in milliseconds. Falls back to + /// `default_timeout_ms` when `None`. + /// * `default_timeout_ms` - Default timeout in milliseconds from the + /// environment configuration. Uses the built-in default (10 min) when + /// zero. /// * `cwd` - Working directory for the command. /// * `env_vars` - Additional environment variables to set. /// @@ -33,12 +36,19 @@ impl UserHookExecutor { command: &str, input_json: &str, timeout: Option, + default_timeout_ms: u64, cwd: &PathBuf, env_vars: &HashMap, ) -> anyhow::Result { let timeout_duration = timeout .map(Duration::from_millis) - .unwrap_or(DEFAULT_HOOK_TIMEOUT); + .unwrap_or_else(|| { + if default_timeout_ms > 0 { + Duration::from_millis(default_timeout_ms) + } else { + DEFAULT_HOOK_TIMEOUT + } + }); debug!( command = command, @@ -134,9 +144,10 @@ mod tests { #[tokio::test] async fn test_execute_simple_command() { let cwd = std::env::current_dir().unwrap(); - let actual = UserHookExecutor::execute("echo hello", "{}", None, &cwd, &HashMap::new()) - .await - .unwrap(); + let actual = + UserHookExecutor::execute("echo hello", "{}", None, 0, &cwd, &HashMap::new()) + .await + .unwrap(); assert_eq!(actual.exit_code, Some(0)); assert_eq!(actual.stdout.trim(), "hello"); @@ -150,6 +161,7 @@ mod tests { "cat", r#"{"hook_event_name": "PreToolUse"}"#, None, + 0, &cwd, &HashMap::new(), ) @@ -167,6 +179,7 @@ mod tests { "echo 'blocked' >&2; exit 2", "{}", None, + 0, &cwd, &HashMap::new(), ) @@ -181,7 +194,7 @@ mod tests { #[tokio::test] async fn test_execute_non_blocking_error() { let cwd = std::env::current_dir().unwrap(); - let actual = UserHookExecutor::execute("exit 1", "{}", None, &cwd, &HashMap::new()) + let actual = UserHookExecutor::execute("exit 1", "{}", None, 0, &cwd, &HashMap::new()) .await .unwrap(); @@ -196,6 +209,7 @@ mod tests { "sleep 10", "{}", Some(100), // 100ms timeout + 0, &cwd, &HashMap::new(), ) @@ -213,9 +227,10 @@ mod tests { let mut env_vars = HashMap::new(); env_vars.insert("FORGE_TEST_VAR".to_string(), "test_value".to_string()); - let actual = UserHookExecutor::execute("echo $FORGE_TEST_VAR", "{}", None, &cwd, &env_vars) - .await - .unwrap(); + let actual = + UserHookExecutor::execute("echo $FORGE_TEST_VAR", "{}", None, 0, &cwd, &env_vars) + .await + .unwrap(); assert_eq!(actual.exit_code, Some(0)); assert_eq!(actual.stdout.trim(), "test_value"); @@ -228,6 +243,7 @@ mod tests { r#"echo '{"decision":"block","reason":"test"}'"#, "{}", None, + 0, &cwd, &HashMap::new(), ) diff --git a/crates/forge_app/src/hooks/user_hook_handler.rs b/crates/forge_app/src/hooks/user_hook_handler.rs index 4f5dbd0023..59235ad14f 100644 --- a/crates/forge_app/src/hooks/user_hook_handler.rs +++ b/crates/forge_app/src/hooks/user_hook_handler.rs @@ -24,6 +24,8 @@ pub struct UserHookHandler { config: UserHookConfig, cwd: PathBuf, env_vars: HashMap, + /// Default timeout in milliseconds for hook commands from the environment. + default_hook_timeout: u64, /// Tracks whether a Stop hook has already fired to prevent infinite loops. stop_hook_active: std::sync::Arc, } @@ -37,11 +39,14 @@ impl UserHookHandler { /// * `project_dir` - Project root directory for `FORGE_PROJECT_DIR` env /// var. /// * `session_id` - Current session/conversation ID. + /// * `default_hook_timeout` - Default timeout in milliseconds for hook + /// commands. pub fn new( config: UserHookConfig, cwd: PathBuf, project_dir: PathBuf, session_id: String, + default_hook_timeout: u64, ) -> Self { let mut env_vars = HashMap::new(); env_vars.insert( @@ -55,6 +60,7 @@ impl UserHookHandler { config, cwd, env_vars, + default_hook_timeout, stop_hook_active: std::sync::Arc::new(AtomicBool::new(false)), } } @@ -124,6 +130,7 @@ impl UserHookHandler { command, &input_json, hook.timeout, + self.default_hook_timeout, &self.cwd, &self.env_vars, ) @@ -697,6 +704,7 @@ mod tests { PathBuf::from("/tmp"), PathBuf::from("/tmp"), "sess-1".to_string(), + 0, ); assert!(!handler.has_hooks(&UserHookEventName::PreToolUse)); } @@ -710,6 +718,7 @@ mod tests { PathBuf::from("/tmp"), PathBuf::from("/tmp"), "sess-1".to_string(), + 0, ); assert!(handler.has_hooks(&UserHookEventName::PreToolUse)); assert!(!handler.has_hooks(&UserHookEventName::Stop)); diff --git a/crates/forge_app/src/orch_spec/orch_setup.rs b/crates/forge_app/src/orch_spec/orch_setup.rs index f03ff78aca..3f4d3581bd 100644 --- a/crates/forge_app/src/orch_spec/orch_setup.rs +++ b/crates/forge_app/src/orch_spec/orch_setup.rs @@ -72,6 +72,7 @@ impl Default for TestContext { suppress_retry_errors: Default::default(), }, tool_timeout: 300, + hook_timeout: 600000, max_search_lines: 1000, fetch_truncation_limit: 1024, stdout_max_prefix_length: 256, diff --git a/crates/forge_app/src/services.rs b/crates/forge_app/src/services.rs index c11b4fafe4..023e766564 100644 --- a/crates/forge_app/src/services.rs +++ b/crates/forge_app/src/services.rs @@ -487,6 +487,18 @@ pub trait CommandLoaderService: Send + Sync { async fn get_commands(&self) -> anyhow::Result>; } +#[async_trait::async_trait] +pub trait UserHookConfigService: Send + Sync { + /// Loads and merges user hook configurations from all settings file + /// locations. + /// + /// Resolution order (all merged, not overridden): + /// 1. `~/.forge/settings.json` (user-level, applies to all projects) + /// 2. `.forge/settings.json` (project-level, committable) + /// 3. `.forge/settings.local.json` (project-level, gitignored) + async fn get_user_hook_config(&self) -> anyhow::Result; +} + #[async_trait::async_trait] pub trait PolicyService: Send + Sync { /// Check if an operation is allowed and handle user confirmation if needed @@ -566,6 +578,7 @@ pub trait Services: Send + Sync + 'static + Clone + EnvironmentInfra { type AuthService: AuthService; type AgentRegistry: AgentRegistry; type CommandLoaderService: CommandLoaderService; + type UserHookConfigService: UserHookConfigService; type PolicyService: PolicyService; type ProviderAuthService: ProviderAuthService; type WorkspaceService: WorkspaceService; @@ -594,6 +607,7 @@ pub trait Services: Send + Sync + 'static + Clone + EnvironmentInfra { fn auth_service(&self) -> &Self::AuthService; fn agent_registry(&self) -> &Self::AgentRegistry; fn command_loader_service(&self) -> &Self::CommandLoaderService; + fn user_hook_config_service(&self) -> &Self::UserHookConfigService; fn policy_service(&self) -> &Self::PolicyService; fn provider_auth_service(&self) -> &Self::ProviderAuthService; fn workspace_service(&self) -> &Self::WorkspaceService; @@ -931,6 +945,15 @@ impl CommandLoaderService for I { } } +#[async_trait::async_trait] +impl UserHookConfigService for I { + async fn get_user_hook_config(&self) -> anyhow::Result { + self.user_hook_config_service() + .get_user_hook_config() + .await + } +} + #[async_trait::async_trait] impl PolicyService for I { async fn check_operation_permission( diff --git a/crates/forge_config/.forge.toml b/crates/forge_config/.forge.toml index fa2331e690..be3fc7fe49 100644 --- a/crates/forge_config/.forge.toml +++ b/crates/forge_config/.forge.toml @@ -23,6 +23,7 @@ sem_search_top_k = 10 services_url = "https://api.forgecode.dev/" tool_supported = true tool_timeout_secs = 300 +hook_timeout_ms = 600000 top_k = 30 top_p = 0.8 diff --git a/crates/forge_config/src/config.rs b/crates/forge_config/src/config.rs index 366633054e..ae5f9500c8 100644 --- a/crates/forge_config/src/config.rs +++ b/crates/forge_config/src/config.rs @@ -60,6 +60,10 @@ pub struct ForgeConfig { /// cancelled. #[serde(default, skip_serializing_if = "Option::is_none")] pub tool_timeout_secs: Option, + /// Default timeout in milliseconds for user hook commands. + /// Individual hooks can override this via their own `timeout` field. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub hook_timeout_ms: Option, /// Whether to automatically open HTML dump files in the browser after /// creation. #[serde(default, skip_serializing_if = "Option::is_none")] diff --git a/crates/forge_domain/src/env.rs b/crates/forge_domain/src/env.rs index 1db2b2903c..8d8f9eb1f3 100644 --- a/crates/forge_domain/src/env.rs +++ b/crates/forge_domain/src/env.rs @@ -95,6 +95,10 @@ pub struct Environment { /// Maximum execution time in seconds for a single tool call. /// Controls how long a tool can run before being terminated. pub tool_timeout: u64, + /// Default timeout in milliseconds for user hook commands. + /// Individual hooks can override this via their own `timeout` field. + /// Controlled by FORGE_HOOK_TIMEOUT_MS environment variable. + pub hook_timeout: u64, /// Whether to automatically open HTML dump files in the browser. /// Controlled by FORGE_DUMP_AUTO_OPEN environment variable. pub auto_open_dump: bool, diff --git a/crates/forge_infra/src/env.rs b/crates/forge_infra/src/env.rs index aeb5882838..334f86cc1b 100644 --- a/crates/forge_infra/src/env.rs +++ b/crates/forge_infra/src/env.rs @@ -148,6 +148,7 @@ fn to_environment(fc: ForgeConfig, cwd: PathBuf) -> Environment { max_file_size: fc.max_file_size_bytes.unwrap_or_default(), max_image_size: fc.max_image_size_bytes.unwrap_or_default(), tool_timeout: fc.tool_timeout_secs.unwrap_or_default(), + hook_timeout: fc.hook_timeout_ms.unwrap_or_default(), auto_open_dump: fc.auto_open_dump.unwrap_or_default(), debug_requests: fc.debug_requests, custom_history_path: fc.custom_history_path, @@ -314,6 +315,7 @@ fn to_forge_config(env: &Environment) -> ForgeConfig { fc.max_file_size_bytes = Some(env.max_file_size); fc.max_image_size_bytes = Some(env.max_image_size); fc.tool_timeout_secs = Some(env.tool_timeout); + fc.hook_timeout_ms = Some(env.hook_timeout); fc.auto_open_dump = Some(env.auto_open_dump); fc.debug_requests = env.debug_requests.clone(); fc.custom_history_path = env.custom_history_path.clone(); diff --git a/crates/forge_main/src/info.rs b/crates/forge_main/src/info.rs index f732a2d6dc..914c404222 100644 --- a/crates/forge_main/src/info.rs +++ b/crates/forge_main/src/info.rs @@ -388,6 +388,7 @@ impl From<&Environment> for Info { .add_key_value("ForgeCode Service URL", env.service_url.to_string()) .add_title("TOOL CONFIGURATION") .add_key_value("Tool Timeout", format!("{}s", env.tool_timeout)) + .add_key_value("Hook Timeout", format!("{}ms", env.hook_timeout)) .add_key_value("Max Image Size", format!("{} bytes", env.max_image_size)) .add_key_value("Auto Open Dump", env.auto_open_dump.to_string()) .add_key_value( diff --git a/crates/forge_services/src/forge_services.rs b/crates/forge_services/src/forge_services.rs index 17e1a1f474..6accd181c5 100644 --- a/crates/forge_services/src/forge_services.rs +++ b/crates/forge_services/src/forge_services.rs @@ -24,6 +24,7 @@ use crate::mcp::{ForgeMcpManager, ForgeMcpService}; use crate::policy::ForgePolicyService; use crate::provider_service::ForgeProviderService; use crate::template::ForgeTemplateService; +use crate::user_hook_config::ForgeUserHookConfigService; use crate::tool_services::{ ForgeFetch, ForgeFollowup, ForgeFsPatch, ForgeFsRead, ForgeFsRemove, ForgeFsSearch, ForgeFsUndo, ForgeFsWrite, ForgeImageRead, ForgePlanCreate, ForgeShell, ForgeSkillFetch, @@ -79,6 +80,7 @@ pub struct ForgeServices< auth_service: Arc>, agent_registry_service: Arc>, command_loader_service: Arc>, + user_hook_config_service: Arc>, policy_service: ForgePolicyService, provider_auth_service: ForgeProviderAuthService, workspace_service: Arc>>, @@ -134,6 +136,7 @@ impl< Arc::new(ForgeCustomInstructionsService::new(infra.clone())); let agent_registry_service = Arc::new(ForgeAgentRegistryService::new(infra.clone())); let command_loader_service = Arc::new(ForgeCommandLoaderService::new(infra.clone())); + let user_hook_config_service = Arc::new(ForgeUserHookConfigService::new(infra.clone())); let policy_service = ForgePolicyService::new(infra.clone()); let provider_auth_service = ForgeProviderAuthService::new(infra.clone()); let discovery = Arc::new(FdDefault::new(infra.clone())); @@ -166,6 +169,7 @@ impl< config_service, agent_registry_service, command_loader_service, + user_hook_config_service, policy_service, provider_auth_service, workspace_service, @@ -233,6 +237,7 @@ impl< type AuthService = AuthService; type AgentRegistry = ForgeAgentRegistryService; type CommandLoaderService = ForgeCommandLoaderService; + type UserHookConfigService = ForgeUserHookConfigService; type PolicyService = ForgePolicyService; type ProviderService = ForgeProviderService; type WorkspaceService = crate::context_engine::ForgeWorkspaceService>; @@ -322,6 +327,10 @@ impl< &self.command_loader_service } + fn user_hook_config_service(&self) -> &Self::UserHookConfigService { + &self.user_hook_config_service + } + fn policy_service(&self) -> &Self::PolicyService { &self.policy_service } diff --git a/crates/forge_services/src/lib.rs b/crates/forge_services/src/lib.rs index e5ecc37452..58dce89cbe 100644 --- a/crates/forge_services/src/lib.rs +++ b/crates/forge_services/src/lib.rs @@ -21,6 +21,7 @@ mod provider_service; mod range; mod template; mod tool_services; +mod user_hook_config; mod utils; pub use app_config::*; diff --git a/crates/forge_services/src/user_hook_config.rs b/crates/forge_services/src/user_hook_config.rs index eba90d388d..95089704f3 100644 --- a/crates/forge_services/src/user_hook_config.rs +++ b/crates/forge_services/src/user_hook_config.rs @@ -97,6 +97,8 @@ impl forge_app::UserHookConfigService mod tests { use std::path::PathBuf; + use fake::Fake; + use forge_app::UserHookConfigService; use pretty_assertions::assert_eq; use super::*; @@ -117,11 +119,7 @@ mod tests { ) .unwrap(); - let infra = TestInfra { - home: None, - cwd: PathBuf::from("/nonexistent"), - }; - let service = ForgeUserHookConfigService::new(Arc::new(infra)); + let service = fixture(None, PathBuf::from("/nonexistent")); let actual = service.load_file(&settings_path).await; assert!(actual.is_some()); @@ -140,11 +138,7 @@ mod tests { let settings_path = dir.path().join("settings.json"); std::fs::write(&settings_path, r#"{"other_key": "value"}"#).unwrap(); - let infra = TestInfra { - home: None, - cwd: PathBuf::from("/nonexistent"), - }; - let service = ForgeUserHookConfigService::new(Arc::new(infra)); + let service = fixture(None, PathBuf::from("/nonexistent")); let actual = service.load_file(&settings_path).await; assert!(actual.is_none()); @@ -152,11 +146,10 @@ mod tests { #[tokio::test] async fn test_get_user_hook_config_nonexistent_paths() { - let infra = TestInfra { - home: Some(PathBuf::from("/nonexistent/home")), - cwd: PathBuf::from("/nonexistent/project"), - }; - let service = ForgeUserHookConfigService::new(Arc::new(infra)); + let service = fixture( + Some(PathBuf::from("/nonexistent/home")), + PathBuf::from("/nonexistent/project"), + ); let actual = service.get_user_hook_config().await.unwrap(); assert!(actual.is_empty()); @@ -207,11 +200,10 @@ mod tests { ) .unwrap(); - let infra = TestInfra { - home: Some(home_dir.path().to_path_buf()), - cwd: project_dir.path().to_path_buf(), - }; - let service = ForgeUserHookConfigService::new(Arc::new(infra)); + let service = fixture( + Some(home_dir.path().to_path_buf()), + project_dir.path().to_path_buf(), + ); let actual = service.get_user_hook_config().await.unwrap(); @@ -231,7 +223,14 @@ mod tests { ); } - // --- Test infrastructure --- + // --- Test helpers --- + + fn fixture( + home: Option, + cwd: PathBuf, + ) -> ForgeUserHookConfigService { + ForgeUserHookConfigService::new(Arc::new(TestInfra { home, cwd })) + } struct TestInfra { home: Option, @@ -244,11 +243,24 @@ mod tests { Ok(tokio::fs::read_to_string(path).await?) } - async fn read_utf8_batch( + fn read_batch_utf8( &self, - _paths: Vec, _batch_size: usize, - ) -> forge_stream::MpscStream<(PathBuf, anyhow::Result)> { + _paths: Vec, + ) -> impl futures::Stream)> + Send { + futures::stream::empty() + } + + async fn read(&self, path: &Path) -> anyhow::Result> { + Ok(tokio::fs::read(path).await?) + } + + async fn range_read_utf8( + &self, + _path: &Path, + _start_line: u64, + _end_line: u64, + ) -> anyhow::Result<(String, forge_domain::FileInfo)> { unimplemented!("not needed for tests") } } @@ -257,13 +269,23 @@ mod tests { fn get_env_var(&self, _key: &str) -> Option { None } + fn get_env_vars(&self) -> std::collections::BTreeMap { Default::default() } + fn get_environment(&self) -> forge_domain::Environment { - forge_domain::Environment::default() - .home(self.home.clone()) - .cwd(self.cwd.clone()) + let mut env: forge_domain::Environment = fake::Faker.fake(); + env.home = self.home.clone(); + env.cwd = self.cwd.clone(); + env + } + + async fn update_environment( + &self, + _ops: Vec, + ) -> anyhow::Result<()> { + unimplemented!("not needed for tests") } } } diff --git a/forge.schema.json b/forge.schema.json index b529d71e07..b75063a019 100644 --- a/forge.schema.json +++ b/forge.schema.json @@ -58,6 +58,15 @@ "null" ] }, + "hook_timeout_ms": { + "description": "Default timeout in milliseconds for user hook commands.\nIndividual hooks can override this via their own `timeout` field.", + "type": [ + "integer", + "null" + ], + "format": "uint64", + "minimum": 0 + }, "http": { "description": "HTTP client settings including proxy, TLS, and timeout configuration.", "anyOf": [ From 20cb9ad4c22e25c24fd671f38971391266f6ae8a Mon Sep 17 00:00:00 2001 From: "autofix-ci[bot]" <114827586+autofix-ci[bot]@users.noreply.github.com> Date: Wed, 1 Apr 2026 03:54:41 +0000 Subject: [PATCH 07/23] [autofix.ci] apply automated fixes --- .../forge_app/src/hooks/user_hook_executor.rs | 23 ++++++++----------- crates/forge_app/src/services.rs | 4 +--- crates/forge_services/src/forge_services.rs | 2 +- crates/forge_services/src/user_hook_config.rs | 5 +--- 4 files changed, 13 insertions(+), 21 deletions(-) diff --git a/crates/forge_app/src/hooks/user_hook_executor.rs b/crates/forge_app/src/hooks/user_hook_executor.rs index e845c7dd40..a33e7a63b8 100644 --- a/crates/forge_app/src/hooks/user_hook_executor.rs +++ b/crates/forge_app/src/hooks/user_hook_executor.rs @@ -40,15 +40,13 @@ impl UserHookExecutor { cwd: &PathBuf, env_vars: &HashMap, ) -> anyhow::Result { - let timeout_duration = timeout - .map(Duration::from_millis) - .unwrap_or_else(|| { - if default_timeout_ms > 0 { - Duration::from_millis(default_timeout_ms) - } else { - DEFAULT_HOOK_TIMEOUT - } - }); + let timeout_duration = timeout.map(Duration::from_millis).unwrap_or_else(|| { + if default_timeout_ms > 0 { + Duration::from_millis(default_timeout_ms) + } else { + DEFAULT_HOOK_TIMEOUT + } + }); debug!( command = command, @@ -144,10 +142,9 @@ mod tests { #[tokio::test] async fn test_execute_simple_command() { let cwd = std::env::current_dir().unwrap(); - let actual = - UserHookExecutor::execute("echo hello", "{}", None, 0, &cwd, &HashMap::new()) - .await - .unwrap(); + let actual = UserHookExecutor::execute("echo hello", "{}", None, 0, &cwd, &HashMap::new()) + .await + .unwrap(); assert_eq!(actual.exit_code, Some(0)); assert_eq!(actual.stdout.trim(), "hello"); diff --git a/crates/forge_app/src/services.rs b/crates/forge_app/src/services.rs index 023e766564..86b82433fa 100644 --- a/crates/forge_app/src/services.rs +++ b/crates/forge_app/src/services.rs @@ -948,9 +948,7 @@ impl CommandLoaderService for I { #[async_trait::async_trait] impl UserHookConfigService for I { async fn get_user_hook_config(&self) -> anyhow::Result { - self.user_hook_config_service() - .get_user_hook_config() - .await + self.user_hook_config_service().get_user_hook_config().await } } diff --git a/crates/forge_services/src/forge_services.rs b/crates/forge_services/src/forge_services.rs index 6accd181c5..34a0b26eed 100644 --- a/crates/forge_services/src/forge_services.rs +++ b/crates/forge_services/src/forge_services.rs @@ -24,11 +24,11 @@ use crate::mcp::{ForgeMcpManager, ForgeMcpService}; use crate::policy::ForgePolicyService; use crate::provider_service::ForgeProviderService; use crate::template::ForgeTemplateService; -use crate::user_hook_config::ForgeUserHookConfigService; use crate::tool_services::{ ForgeFetch, ForgeFollowup, ForgeFsPatch, ForgeFsRead, ForgeFsRemove, ForgeFsSearch, ForgeFsUndo, ForgeFsWrite, ForgeImageRead, ForgePlanCreate, ForgeShell, ForgeSkillFetch, }; +use crate::user_hook_config::ForgeUserHookConfigService; type McpService = ForgeMcpService, F, ::Client>; type AuthService = ForgeAuthService; diff --git a/crates/forge_services/src/user_hook_config.rs b/crates/forge_services/src/user_hook_config.rs index 95089704f3..3ae4c6d89c 100644 --- a/crates/forge_services/src/user_hook_config.rs +++ b/crates/forge_services/src/user_hook_config.rs @@ -225,10 +225,7 @@ mod tests { // --- Test helpers --- - fn fixture( - home: Option, - cwd: PathBuf, - ) -> ForgeUserHookConfigService { + fn fixture(home: Option, cwd: PathBuf) -> ForgeUserHookConfigService { ForgeUserHookConfigService::new(Arc::new(TestInfra { home, cwd })) } From 397bb4d0542d83a22727908e085524c70c114e6b Mon Sep 17 00:00:00 2001 From: Sandipsinh Dilipsinh Rathod <62684960+ssddOnTop@users.noreply.github.com> Date: Wed, 1 Apr 2026 01:22:52 -0400 Subject: [PATCH 08/23] feat(hooks): add HookError chat response and surface it in ui and orchestrator --- crates/forge_app/src/agent_executor.rs | 1 + crates/forge_app/src/hooks/user_hook_handler.rs | 2 ++ crates/forge_app/src/orch.rs | 9 +++++++-- crates/forge_domain/src/chat_response.rs | 7 +++++++ crates/forge_main/src/ui.rs | 8 ++++++++ 5 files changed, 25 insertions(+), 2 deletions(-) diff --git a/crates/forge_app/src/agent_executor.rs b/crates/forge_app/src/agent_executor.rs index 4c5ed94ff2..37e761742e 100644 --- a/crates/forge_app/src/agent_executor.rs +++ b/crates/forge_app/src/agent_executor.rs @@ -94,6 +94,7 @@ impl AgentExecutor { ChatResponse::ToolCallStart { .. } => ctx.send(message).await?, ChatResponse::ToolCallEnd(_) => ctx.send(message).await?, ChatResponse::RetryAttempt { .. } => ctx.send(message).await?, + ChatResponse::HookError { .. } => ctx.send(message).await?, ChatResponse::Interrupt { reason } => { return Err(Error::AgentToolInterrupted(reason)) .context(format!( diff --git a/crates/forge_app/src/hooks/user_hook_handler.rs b/crates/forge_app/src/hooks/user_hook_handler.rs index 59235ad14f..11d8e704e0 100644 --- a/crates/forge_app/src/hooks/user_hook_handler.rs +++ b/crates/forge_app/src/hooks/user_hook_handler.rs @@ -327,6 +327,8 @@ impl EventHandle> for UserHookHandler { } let tool_name = event.payload.tool_call.name.as_str(); + // TODO: Add a tool name transformer to map tool names to Forge + // equivalents (e.g. "Bash" → "shell") so that hook configs written let groups = self.config.get_groups(&UserHookEventName::PreToolUse); let hooks = Self::find_matching_hooks(groups, Some(tool_name)); diff --git a/crates/forge_app/src/orch.rs b/crates/forge_app/src/orch.rs index a3a16642f8..c611b12038 100644 --- a/crates/forge_app/src/orch.rs +++ b/crates/forge_app/src/orch.rs @@ -97,8 +97,13 @@ impl Orchestrator { .await; let tool_result = if let Err(hook_err) = hook_result { - // Hook blocked this tool call — produce an error ToolResult - // so the model sees feedback without aborting the session. + // Hook blocked this tool call — notify the UI and produce an + // error ToolResult so the model sees feedback without aborting. + self.send(ChatResponse::HookError { + tool_name: tool_call.name.clone(), + reason: hook_err.to_string(), + }) + .await?; ToolResult::from(tool_call.clone()).failure(hook_err) } else { // Execute the tool normally diff --git a/crates/forge_domain/src/chat_response.rs b/crates/forge_domain/src/chat_response.rs index e24cd9d731..8017caeab7 100644 --- a/crates/forge_domain/src/chat_response.rs +++ b/crates/forge_domain/src/chat_response.rs @@ -65,6 +65,13 @@ pub enum ChatResponse { notifier: Arc, }, ToolCallEnd(ToolResult), + /// A user-configured hook blocked execution of a tool call. + HookError { + /// Name of the tool that was blocked. + tool_name: ToolName, + /// Human-readable reason provided by the hook (from stderr or JSON output). + reason: String, + }, RetryAttempt { cause: Cause, duration: Duration, diff --git a/crates/forge_main/src/ui.rs b/crates/forge_main/src/ui.rs index ec0cd1a38a..d426fcfccc 100644 --- a/crates/forge_main/src/ui.rs +++ b/crates/forge_main/src/ui.rs @@ -3174,6 +3174,14 @@ impl A + Send + Sync> UI { self.writeln_title(TitleFormat::error(cause.as_str()))?; } } + ChatResponse::HookError { tool_name, reason } => { + writer.finish()?; + self.spinner.stop(None)?; + self.writeln_title(TitleFormat::error(format!( + "PreToolUse:{tool_name} hook error: {reason}" + )))?; + self.spinner.start(None)?; + } ChatResponse::Interrupt { reason } => { writer.finish()?; self.spinner.stop(None)?; From 8e060d6fef3a7d5dbeef38b87eb098022c3ea0b6 Mon Sep 17 00:00:00 2001 From: "autofix-ci[bot]" <114827586+autofix-ci[bot]@users.noreply.github.com> Date: Wed, 1 Apr 2026 05:25:06 +0000 Subject: [PATCH 09/23] [autofix.ci] apply automated fixes --- crates/forge_domain/src/chat_response.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/crates/forge_domain/src/chat_response.rs b/crates/forge_domain/src/chat_response.rs index 8017caeab7..3f5f5347ec 100644 --- a/crates/forge_domain/src/chat_response.rs +++ b/crates/forge_domain/src/chat_response.rs @@ -69,7 +69,8 @@ pub enum ChatResponse { HookError { /// Name of the tool that was blocked. tool_name: ToolName, - /// Human-readable reason provided by the hook (from stderr or JSON output). + /// Human-readable reason provided by the hook (from stderr or JSON + /// output). reason: String, }, RetryAttempt { From 6f50be05143316cc68d7536692d8b16f7beff559 Mon Sep 17 00:00:00 2001 From: Sandipsinh Dilipsinh Rathod <62684960+ssddOnTop@users.noreply.github.com> Date: Wed, 1 Apr 2026 01:42:45 -0400 Subject: [PATCH 10/23] refactor(hooks): replace manual Display impl with strum_macros::Display for UserHookEventName --- crates/forge_domain/src/user_hook_config.rs | 21 ++------------------- 1 file changed, 2 insertions(+), 19 deletions(-) diff --git a/crates/forge_domain/src/user_hook_config.rs b/crates/forge_domain/src/user_hook_config.rs index 306e0bc088..89dbfb6929 100644 --- a/crates/forge_domain/src/user_hook_config.rs +++ b/crates/forge_domain/src/user_hook_config.rs @@ -1,7 +1,7 @@ use std::collections::HashMap; -use std::fmt; use serde::{Deserialize, Serialize}; +use strum_macros::Display; /// Top-level user hook configuration. /// @@ -52,7 +52,7 @@ impl UserHookConfig { /// Supported hook event names that map to lifecycle points in the /// orchestrator. -#[derive(Debug, Clone, PartialEq, Eq, Hash, Serialize, Deserialize)] +#[derive(Debug, Clone, PartialEq, Eq, Hash, Serialize, Deserialize, Display)] pub enum UserHookEventName { /// Fired before a tool call executes. Can block execution. PreToolUse, @@ -76,23 +76,6 @@ pub enum UserHookEventName { PostCompact, } -impl fmt::Display for UserHookEventName { - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - match self { - Self::PreToolUse => write!(f, "PreToolUse"), - Self::PostToolUse => write!(f, "PostToolUse"), - Self::PostToolUseFailure => write!(f, "PostToolUseFailure"), - Self::Stop => write!(f, "Stop"), - Self::Notification => write!(f, "Notification"), - Self::SessionStart => write!(f, "SessionStart"), - Self::SessionEnd => write!(f, "SessionEnd"), - Self::UserPromptSubmit => write!(f, "UserPromptSubmit"), - Self::PreCompact => write!(f, "PreCompact"), - Self::PostCompact => write!(f, "PostCompact"), - } - } -} - /// A matcher group pairs an optional regex matcher with a list of hook /// handlers. /// From 6ae1fb0413cb1a6da64497eb3457a6fefc9606c7 Mon Sep 17 00:00:00 2001 From: Sandipsinh Dilipsinh Rathod <62684960+ssddOnTop@users.noreply.github.com> Date: Wed, 1 Apr 2026 03:19:42 -0400 Subject: [PATCH 11/23] feat(hooks): implement UserPromptSubmit hook event with blocking and feedback injection --- .../forge_app/src/hooks/user_hook_handler.rs | 69 +++++++++++++++++-- crates/forge_domain/src/user_hook_io.rs | 27 ++++++++ 2 files changed, 90 insertions(+), 6 deletions(-) diff --git a/crates/forge_app/src/hooks/user_hook_handler.rs b/crates/forge_app/src/hooks/user_hook_handler.rs index 11d8e704e0..96d40fde97 100644 --- a/crates/forge_app/src/hooks/user_hook_handler.rs +++ b/crates/forge_app/src/hooks/user_hook_handler.rs @@ -4,9 +4,10 @@ use std::sync::atomic::{AtomicBool, Ordering}; use async_trait::async_trait; use forge_domain::{ - Conversation, EndPayload, EventData, EventHandle, HookEventInput, HookExecutionResult, - HookInput, HookOutput, RequestPayload, ResponsePayload, StartPayload, ToolcallEndPayload, - ToolcallStartPayload, UserHookConfig, UserHookEntry, UserHookEventName, UserHookMatcherGroup, + ContextMessage, Conversation, EndPayload, EventData, EventHandle, HookEventInput, + HookExecutionResult, HookInput, HookOutput, RequestPayload, ResponsePayload, Role, StartPayload, + ToolcallEndPayload, ToolcallStartPayload, UserHookConfig, UserHookEntry, UserHookEventName, + UserHookMatcherGroup, }; use regex::Regex; use tracing::{debug, warn}; @@ -295,10 +296,66 @@ impl EventHandle> for UserHookHandler { impl EventHandle> for UserHookHandler { async fn handle( &self, - _event: &EventData, - _conversation: &mut Conversation, + event: &EventData, + conversation: &mut Conversation, ) -> anyhow::Result<()> { - // No user hook events map to Request currently + // Only fire on the first request of a turn (user-submitted prompt). + // Subsequent iterations are internal LLM retry/tool-call loops and + // should not re-trigger UserPromptSubmit. + if event.payload.request_count != 0 { + return Ok(()); + } + + if !self.has_hooks(&UserHookEventName::UserPromptSubmit) { + return Ok(()); + } + + let groups = self.config.get_groups(&UserHookEventName::UserPromptSubmit); + let hooks = Self::find_matching_hooks(groups, None); + + if hooks.is_empty() { + return Ok(()); + } + + // Extract the last user message text as the prompt sent to the hook. + let prompt = conversation + .context + .as_ref() + .and_then(|ctx| { + ctx.messages + .iter() + .rev() + .find(|m| m.has_role(Role::User)) + .and_then(|m| m.content()) + .map(|s| s.to_string()) + }) + .unwrap_or_default(); + + let input = HookInput { + hook_event_name: "UserPromptSubmit".to_string(), + cwd: self.cwd.to_string_lossy().to_string(), + session_id: self.env_vars.get("FORGE_SESSION_ID").cloned(), + event_data: HookEventInput::UserPromptSubmit { prompt }, + }; + + let results = self.execute_hooks(&hooks, &input).await; + + if let Some(reason) = Self::process_results(&results) { + debug!( + reason = reason.as_str(), + "UserPromptSubmit hook blocked with feedback" + ); + // Inject feedback so the model sees why the prompt was flagged. + if let Some(context) = conversation.context.as_mut() { + let feedback_msg = format!( + "\nUserPromptSubmit\nblocked\n{reason}\n" + ); + context + .messages + .push(ContextMessage::user(feedback_msg, None).into()); + } + } + Ok(()) } } diff --git a/crates/forge_domain/src/user_hook_io.rs b/crates/forge_domain/src/user_hook_io.rs index 115f62a673..a48145fbf2 100644 --- a/crates/forge_domain/src/user_hook_io.rs +++ b/crates/forge_domain/src/user_hook_io.rs @@ -60,6 +60,11 @@ pub enum HookEventInput { /// Source of the session start (e.g., "startup", "resume"). source: String, }, + /// Input for UserPromptSubmit events. + UserPromptSubmit { + /// The raw prompt text submitted by the user. + prompt: String, + }, /// Empty input for events that don't need event-specific data. Empty {}, } @@ -223,6 +228,28 @@ mod tests { assert_eq!(actual["stop_hook_active"], false); } + #[test] + fn test_hook_input_serialization_user_prompt_submit() { + let fixture = HookInput { + hook_event_name: "UserPromptSubmit".to_string(), + cwd: "/project".to_string(), + session_id: Some("sess-abc".to_string()), + event_data: HookEventInput::UserPromptSubmit { + prompt: "fix the bug".to_string(), + }, + }; + + let actual = serde_json::to_value(&fixture).unwrap(); + + assert_eq!(actual["hook_event_name"], "UserPromptSubmit"); + assert_eq!(actual["cwd"], "/project"); + assert_eq!(actual["session_id"], "sess-abc"); + assert_eq!(actual["prompt"], "fix the bug"); + // No tool_name, stop_hook_active, or other variant fields present + assert!(actual["tool_name"].is_null()); + assert!(actual["stop_hook_active"].is_null()); + } + #[test] fn test_hook_output_parse_valid_json() { let stdout = r#"{"decision": "block", "reason": "unsafe command"}"#; From 156cbc8ed9a2621c9129e6e5dcca9f3927268157 Mon Sep 17 00:00:00 2001 From: "autofix-ci[bot]" <114827586+autofix-ci[bot]@users.noreply.github.com> Date: Wed, 1 Apr 2026 07:21:28 +0000 Subject: [PATCH 12/23] [autofix.ci] apply automated fixes --- crates/forge_app/src/hooks/user_hook_handler.rs | 6 +++--- crates/forge_domain/src/user_hook_io.rs | 4 +--- 2 files changed, 4 insertions(+), 6 deletions(-) diff --git a/crates/forge_app/src/hooks/user_hook_handler.rs b/crates/forge_app/src/hooks/user_hook_handler.rs index 96d40fde97..df63959994 100644 --- a/crates/forge_app/src/hooks/user_hook_handler.rs +++ b/crates/forge_app/src/hooks/user_hook_handler.rs @@ -5,9 +5,9 @@ use std::sync::atomic::{AtomicBool, Ordering}; use async_trait::async_trait; use forge_domain::{ ContextMessage, Conversation, EndPayload, EventData, EventHandle, HookEventInput, - HookExecutionResult, HookInput, HookOutput, RequestPayload, ResponsePayload, Role, StartPayload, - ToolcallEndPayload, ToolcallStartPayload, UserHookConfig, UserHookEntry, UserHookEventName, - UserHookMatcherGroup, + HookExecutionResult, HookInput, HookOutput, RequestPayload, ResponsePayload, Role, + StartPayload, ToolcallEndPayload, ToolcallStartPayload, UserHookConfig, UserHookEntry, + UserHookEventName, UserHookMatcherGroup, }; use regex::Regex; use tracing::{debug, warn}; diff --git a/crates/forge_domain/src/user_hook_io.rs b/crates/forge_domain/src/user_hook_io.rs index a48145fbf2..3f260428c7 100644 --- a/crates/forge_domain/src/user_hook_io.rs +++ b/crates/forge_domain/src/user_hook_io.rs @@ -234,9 +234,7 @@ mod tests { hook_event_name: "UserPromptSubmit".to_string(), cwd: "/project".to_string(), session_id: Some("sess-abc".to_string()), - event_data: HookEventInput::UserPromptSubmit { - prompt: "fix the bug".to_string(), - }, + event_data: HookEventInput::UserPromptSubmit { prompt: "fix the bug".to_string() }, }; let actual = serde_json::to_value(&fixture).unwrap(); From cb100b4c059b68ffd7bb93473d870b819e6f6761 Mon Sep 17 00:00:00 2001 From: Sandipsinh Dilipsinh Rathod <62684960+ssddOnTop@users.noreply.github.com> Date: Wed, 1 Apr 2026 18:45:03 -0400 Subject: [PATCH 13/23] feat(hooks): add ForgeHookCommandService wrapping CommandInfra for hook execution --- .../src/tool_services/hook_command.rs | 39 +++++++++++++++++++ 1 file changed, 39 insertions(+) create mode 100644 crates/forge_services/src/tool_services/hook_command.rs diff --git a/crates/forge_services/src/tool_services/hook_command.rs b/crates/forge_services/src/tool_services/hook_command.rs new file mode 100644 index 0000000000..b4a2f84526 --- /dev/null +++ b/crates/forge_services/src/tool_services/hook_command.rs @@ -0,0 +1,39 @@ +use std::collections::HashMap; +use std::path::PathBuf; +use std::sync::Arc; +use std::time::Duration; + +use forge_app::{CommandInfra, HookCommandService}; +use forge_domain::CommandOutput; + +/// Thin wrapper around [`CommandInfra::execute_command_with_input`] that +/// satisfies the [`HookCommandService`] contract. +/// +/// By delegating to the underlying infra this service avoids duplicating +/// process-spawning, stdin-piping, and timeout logic; those concerns live +/// entirely inside the `CommandInfra` implementation. +#[derive(Clone)] +pub struct ForgeHookCommandService(Arc); + +impl ForgeHookCommandService { + /// Creates a new `ForgeHookCommandService` backed by the given infra. + pub fn new(infra: Arc) -> Self { + Self(infra) + } +} + +#[async_trait::async_trait] +impl HookCommandService for ForgeHookCommandService { + async fn execute_command_with_input( + &self, + command: String, + working_dir: PathBuf, + stdin_input: String, + timeout: Duration, + env_vars: HashMap, + ) -> anyhow::Result { + self.0 + .execute_command_with_input(command, working_dir, stdin_input, timeout, env_vars) + .await + } +} From ab08745b8fc748d4cbc9b2537beeb5824d8f666c Mon Sep 17 00:00:00 2001 From: Sandipsinh Dilipsinh Rathod <62684960+ssddOnTop@users.noreply.github.com> Date: Wed, 1 Apr 2026 18:45:07 -0400 Subject: [PATCH 14/23] refactor(hooks): inject HookCommandService into UserHookExecutor and UserHookHandler --- crates/forge_app/src/app.rs | 1 + .../forge_app/src/hooks/user_hook_executor.rs | 312 +++++++++--------- .../forge_app/src/hooks/user_hook_handler.rs | 153 ++++++--- crates/forge_app/src/infra.rs | 28 +- crates/forge_app/src/services.rs | 34 ++ crates/forge_infra/src/executor.rs | 60 +++- crates/forge_infra/src/forge_infra.rs | 16 +- crates/forge_repo/src/forge_repo.rs | 16 +- crates/forge_services/src/forge_services.rs | 11 +- .../forge_services/src/tool_services/mod.rs | 2 + .../forge_services/src/tool_services/shell.rs | 16 + 11 files changed, 431 insertions(+), 218 deletions(-) diff --git a/crates/forge_app/src/app.rs b/crates/forge_app/src/app.rs index a9434e2f79..27c2efe2e6 100644 --- a/crates/forge_app/src/app.rs +++ b/crates/forge_app/src/app.rs @@ -145,6 +145,7 @@ impl ForgeApp { let hook = if !user_hook_config.is_empty() { let user_handler = UserHookHandler::new( + services.hook_command_service().clone(), user_hook_config, environment.cwd.clone(), environment.cwd.clone(), diff --git a/crates/forge_app/src/hooks/user_hook_executor.rs b/crates/forge_app/src/hooks/user_hook_executor.rs index a33e7a63b8..5d691847b9 100644 --- a/crates/forge_app/src/hooks/user_hook_executor.rs +++ b/crates/forge_app/src/hooks/user_hook_executor.rs @@ -3,19 +3,29 @@ use std::path::PathBuf; use std::time::Duration; use forge_domain::HookExecutionResult; -use tokio::io::AsyncWriteExt; -use tracing::{debug, warn}; +use tracing::debug; + +use crate::services::HookCommandService; /// Default timeout for hook commands (10 minutes). const DEFAULT_HOOK_TIMEOUT: Duration = Duration::from_secs(600); -/// Executes user hook shell commands with stdin piping and timeout support. +/// Executes user hook commands by delegating to a [`HookCommandService`]. /// -/// Uses `tokio::process::Command` directly (not `CommandInfra`) because we -/// need stdin piping which the existing infrastructure doesn't support. -pub struct UserHookExecutor; +/// Holds the service by value; the service itself is responsible for any +/// internal reference counting (`Arc`). Keeps hook-specific timeout resolution +/// in one place. +#[derive(Clone)] +pub struct UserHookExecutor(S); + +impl UserHookExecutor { + /// Creates a new `UserHookExecutor` backed by the given service. + pub fn new(service: S) -> Self { + Self(service) + } +} -impl UserHookExecutor { +impl UserHookExecutor { /// Executes a shell command, piping `input_json` to stdin and capturing /// stdout/stderr. /// @@ -33,6 +43,7 @@ impl UserHookExecutor { /// # Errors /// Returns an error if the process cannot be spawned. pub async fn execute( + &self, command: &str, input_json: &str, timeout: Option, @@ -55,133 +66,135 @@ impl UserHookExecutor { "Executing user hook command" ); - let shell = if cfg!(target_os = "windows") { - "cmd" - } else { - "sh" - }; - let shell_arg = if cfg!(target_os = "windows") { - "/C" - } else { - "-c" - }; - - let mut child = tokio::process::Command::new(shell) - .arg(shell_arg) - .arg(command) - .current_dir(cwd) - .envs(env_vars) - .stdin(std::process::Stdio::piped()) - .stdout(std::process::Stdio::piped()) - .stderr(std::process::Stdio::piped()) - .spawn()?; - - // Pipe JSON input to stdin - if let Some(mut stdin) = child.stdin.take() { - let input = input_json.to_string(); - tokio::spawn(async move { - let _ = stdin.write_all(input.as_bytes()).await; - let _ = stdin.shutdown().await; - }); - } + let output = self + .0 + .execute_command_with_input( + command.to_string(), + cwd.clone(), + input_json.to_string(), + timeout_duration, + env_vars.clone(), + ) + .await?; - // Wait for the command with timeout. - // Note: `wait_with_output()` takes ownership of `child`. On timeout, - // the future is dropped, and tokio will clean up the child process. - let result = tokio::time::timeout(timeout_duration, child.wait_with_output()).await; - - match result { - Ok(Ok(output)) => { - let exit_code = output.status.code(); - let stdout = String::from_utf8_lossy(&output.stdout).to_string(); - let stderr = String::from_utf8_lossy(&output.stderr).to_string(); - - debug!( - command = command, - exit_code = ?exit_code, - stdout_len = stdout.len(), - stderr_len = stderr.len(), - "Hook command completed" - ); - - Ok(HookExecutionResult { exit_code, stdout, stderr }) - } - Ok(Err(e)) => { - warn!(command = command, error = %e, "Hook command failed to execute"); - Err(e.into()) - } - Err(_) => { - warn!( - command = command, - timeout_ms = timeout_duration.as_millis() as u64, - "Hook command timed out" - ); - // Process is already consumed by wait_with_output, tokio - // handles cleanup when the future is dropped. - Ok(HookExecutionResult { - exit_code: None, - stdout: String::new(), - stderr: format!( - "Hook command timed out after {}ms", - timeout_duration.as_millis() - ), - }) - } - } + debug!( + command = command, + exit_code = ?output.exit_code, + stdout_len = output.stdout.len(), + stderr_len = output.stderr.len(), + "Hook command completed" + ); + + Ok(HookExecutionResult { + exit_code: output.exit_code, + stdout: output.stdout, + stderr: output.stderr, + }) } } #[cfg(test)] mod tests { use std::collections::HashMap; + use std::path::PathBuf; + use std::time::Duration; + use forge_domain::CommandOutput; use pretty_assertions::assert_eq; use super::*; - #[tokio::test] - async fn test_execute_simple_command() { - let cwd = std::env::current_dir().unwrap(); - let actual = UserHookExecutor::execute("echo hello", "{}", None, 0, &cwd, &HashMap::new()) - .await - .unwrap(); + /// A minimal service stub that records calls and returns a fixed result. + #[derive(Clone)] + struct StubInfra { + result: CommandOutput, + } - assert_eq!(actual.exit_code, Some(0)); - assert_eq!(actual.stdout.trim(), "hello"); - assert!(actual.is_success()); + impl StubInfra { + fn success(stdout: &str) -> Self { + Self { + result: CommandOutput { + command: String::new(), + exit_code: Some(0), + stdout: stdout.to_string(), + stderr: String::new(), + }, + } + } + + fn exit(code: i32, stderr: &str) -> Self { + Self { + result: CommandOutput { + command: String::new(), + exit_code: Some(code), + stdout: String::new(), + stderr: stderr.to_string(), + }, + } + } + + fn timeout() -> Self { + Self { + result: CommandOutput { + command: String::new(), + exit_code: None, + stdout: String::new(), + stderr: "Hook command timed out after 100ms".to_string(), + }, + } + } + } + + #[async_trait::async_trait] + impl HookCommandService for StubInfra { + async fn execute_command_with_input( + &self, + command: String, + _working_dir: PathBuf, + _stdin_input: String, + _timeout: Duration, + _env_vars: HashMap, + ) -> anyhow::Result { + let mut out = self.result.clone(); + out.command = command; + Ok(out) + } } #[tokio::test] - async fn test_execute_reads_stdin() { - let cwd = std::env::current_dir().unwrap(); - let actual = UserHookExecutor::execute( - "cat", - r#"{"hook_event_name": "PreToolUse"}"#, - None, - 0, - &cwd, - &HashMap::new(), - ) - .await - .unwrap(); + async fn test_execute_success() { + let fixture = UserHookExecutor::new(StubInfra::success("hello")); + let actual = fixture + .execute( + "echo hello", + "{}", + None, + 0, + &std::env::current_dir().unwrap(), + &HashMap::new(), + ) + .await + .unwrap(); assert_eq!(actual.exit_code, Some(0)); - assert!(actual.stdout.contains("PreToolUse")); + assert_eq!(actual.stdout, "hello"); + assert!(actual.is_success()); } #[tokio::test] async fn test_execute_exit_code_2() { - let cwd = std::env::current_dir().unwrap(); - let actual = UserHookExecutor::execute( - "echo 'blocked' >&2; exit 2", - "{}", - None, - 0, - &cwd, - &HashMap::new(), - ) - .await - .unwrap(); + let fixture = UserHookExecutor::new(StubInfra::exit(2, "blocked")); + let actual = fixture + .execute( + "exit 2", + "{}", + None, + 0, + &std::env::current_dir().unwrap(), + &HashMap::new(), + ) + .await + .unwrap(); assert_eq!(actual.exit_code, Some(2)); assert!(actual.is_blocking_exit()); @@ -190,8 +203,16 @@ mod tests { #[tokio::test] async fn test_execute_non_blocking_error() { - let cwd = std::env::current_dir().unwrap(); - let actual = UserHookExecutor::execute("exit 1", "{}", None, 0, &cwd, &HashMap::new()) + let fixture = UserHookExecutor::new(StubInfra::exit(1, "")); + let actual = fixture + .execute( + "exit 1", + "{}", + None, + 0, + &std::env::current_dir().unwrap(), + &HashMap::new(), + ) .await .unwrap(); @@ -201,55 +222,20 @@ mod tests { #[tokio::test] async fn test_execute_timeout() { - let cwd = std::env::current_dir().unwrap(); - let actual = UserHookExecutor::execute( - "sleep 10", - "{}", - Some(100), // 100ms timeout - 0, - &cwd, - &HashMap::new(), - ) - .await - .unwrap(); - - // Should have no exit code (killed by timeout) - assert!(actual.exit_code.is_none() || actual.is_non_blocking_error()); - assert!(actual.stderr.contains("timed out")); - } - - #[tokio::test] - async fn test_execute_with_env_vars() { - let cwd = std::env::current_dir().unwrap(); - let mut env_vars = HashMap::new(); - env_vars.insert("FORGE_TEST_VAR".to_string(), "test_value".to_string()); - - let actual = - UserHookExecutor::execute("echo $FORGE_TEST_VAR", "{}", None, 0, &cwd, &env_vars) - .await - .unwrap(); - - assert_eq!(actual.exit_code, Some(0)); - assert_eq!(actual.stdout.trim(), "test_value"); - } - - #[tokio::test] - async fn test_execute_json_output() { - let cwd = std::env::current_dir().unwrap(); - let actual = UserHookExecutor::execute( - r#"echo '{"decision":"block","reason":"test"}'"#, - "{}", - None, - 0, - &cwd, - &HashMap::new(), - ) - .await - .unwrap(); + let fixture = UserHookExecutor::new(StubInfra::timeout()); + let actual = fixture + .execute( + "sleep 10", + "{}", + Some(100), + 0, + &std::env::current_dir().unwrap(), + &HashMap::new(), + ) + .await + .unwrap(); - assert!(actual.is_success()); - let output = actual.parse_output().unwrap(); - assert!(output.is_blocking()); - assert_eq!(output.reason, Some("test".to_string())); + assert!(actual.exit_code.is_none()); + assert!(actual.stderr.contains("timed out")); } } diff --git a/crates/forge_app/src/hooks/user_hook_handler.rs b/crates/forge_app/src/hooks/user_hook_handler.rs index df63959994..486f4d2295 100644 --- a/crates/forge_app/src/hooks/user_hook_handler.rs +++ b/crates/forge_app/src/hooks/user_hook_handler.rs @@ -13,6 +13,7 @@ use regex::Regex; use tracing::{debug, warn}; use super::user_hook_executor::UserHookExecutor; +use crate::services::HookCommandService; /// EventHandle implementation that bridges user-configured hooks with the /// existing lifecycle event system. @@ -20,8 +21,8 @@ use super::user_hook_executor::UserHookExecutor; /// This handler is constructed from a `UserHookConfig` and executes matching /// hook commands at each lifecycle event point. It wires into the existing /// `Hook` system via `Hook::zip()`. -#[derive(Clone)] -pub struct UserHookHandler { +pub struct UserHookHandler { + executor: UserHookExecutor, config: UserHookConfig, cwd: PathBuf, env_vars: HashMap, @@ -31,10 +32,24 @@ pub struct UserHookHandler { stop_hook_active: std::sync::Arc, } -impl UserHookHandler { +impl Clone for UserHookHandler { + fn clone(&self) -> Self { + Self { + executor: self.executor.clone(), + config: self.config.clone(), + cwd: self.cwd.clone(), + env_vars: self.env_vars.clone(), + default_hook_timeout: self.default_hook_timeout, + stop_hook_active: self.stop_hook_active.clone(), + } + } +} + +impl UserHookHandler { /// Creates a new user hook handler from configuration. /// /// # Arguments + /// * `service` - The hook command service used to execute hook commands. /// * `config` - The merged user hook configuration. /// * `cwd` - Current working directory for command execution. /// * `project_dir` - Project root directory for `FORGE_PROJECT_DIR` env @@ -43,6 +58,7 @@ impl UserHookHandler { /// * `default_hook_timeout` - Default timeout in milliseconds for hook /// commands. pub fn new( + service: I, config: UserHookConfig, cwd: PathBuf, project_dir: PathBuf, @@ -58,6 +74,7 @@ impl UserHookHandler { env_vars.insert("FORGE_CWD".to_string(), cwd.to_string_lossy().to_string()); Self { + executor: UserHookExecutor::new(service), config, cwd, env_vars, @@ -115,7 +132,10 @@ impl UserHookHandler { &self, hooks: &[&UserHookEntry], input: &HookInput, - ) -> Vec { + ) -> Vec + where + I: HookCommandService, + { let input_json = match serde_json::to_string(input) { Ok(json) => json, Err(e) => { @@ -127,15 +147,17 @@ impl UserHookHandler { let mut results = Vec::new(); for hook in hooks { if let Some(command) = &hook.command { - match UserHookExecutor::execute( - command, - &input_json, - hook.timeout, - self.default_hook_timeout, - &self.cwd, - &self.env_vars, - ) - .await + match self + .executor + .execute( + command, + &input_json, + hook.timeout, + self.default_hook_timeout, + &self.cwd, + &self.env_vars, + ) + .await { Ok(result) => results.push(result), Err(e) => { @@ -250,7 +272,7 @@ enum PreToolUseDecision { // --- EventHandle implementations --- #[async_trait] -impl EventHandle> for UserHookHandler { +impl EventHandle> for UserHookHandler { async fn handle( &self, _event: &EventData, @@ -293,7 +315,7 @@ impl EventHandle> for UserHookHandler { } #[async_trait] -impl EventHandle> for UserHookHandler { +impl EventHandle> for UserHookHandler { async fn handle( &self, event: &EventData, @@ -361,7 +383,7 @@ impl EventHandle> for UserHookHandler { } #[async_trait] -impl EventHandle> for UserHookHandler { +impl EventHandle> for UserHookHandler { async fn handle( &self, _event: &EventData, @@ -373,7 +395,9 @@ impl EventHandle> for UserHookHandler { } #[async_trait] -impl EventHandle> for UserHookHandler { +impl EventHandle> + for UserHookHandler +{ async fn handle( &self, event: &EventData, @@ -438,7 +462,9 @@ impl EventHandle> for UserHookHandler { } #[async_trait] -impl EventHandle> for UserHookHandler { +impl EventHandle> + for UserHookHandler +{ async fn handle( &self, event: &EventData, @@ -505,7 +531,7 @@ impl EventHandle> for UserHookHandler { } #[async_trait] -impl EventHandle> for UserHookHandler { +impl EventHandle> for UserHookHandler { async fn handle( &self, _event: &EventData, @@ -582,13 +608,52 @@ impl EventHandle> for UserHookHandler { #[cfg(test)] mod tests { + use std::collections::HashMap; + use std::path::PathBuf; + use std::time::Duration; + use forge_domain::{ - HookExecutionResult, UserHookEntry, UserHookEventName, UserHookMatcherGroup, UserHookType, + CommandOutput, HookExecutionResult, UserHookEntry, UserHookEventName, UserHookMatcherGroup, + UserHookType, }; use pretty_assertions::assert_eq; use super::*; + /// A no-op service stub for tests that only exercise config/matching logic. + #[derive(Clone)] + struct NullInfra; + + #[async_trait::async_trait] + impl HookCommandService for NullInfra { + async fn execute_command_with_input( + &self, + command: String, + _working_dir: PathBuf, + _stdin_input: String, + _timeout: Duration, + _env_vars: HashMap, + ) -> anyhow::Result { + Ok(CommandOutput { + command, + exit_code: Some(0), + stdout: String::new(), + stderr: String::new(), + }) + } + } + + fn null_handler(config: UserHookConfig) -> UserHookHandler { + UserHookHandler::new( + NullInfra, + config, + PathBuf::from("/tmp"), + PathBuf::from("/tmp"), + "sess-1".to_string(), + 0, + ) + } + fn make_entry(command: &str) -> UserHookEntry { UserHookEntry { hook_type: UserHookType::Command, @@ -607,7 +672,7 @@ mod tests { #[test] fn test_find_matching_hooks_no_matcher_fires_unconditionally() { let groups = vec![make_group(None, &["echo hi"])]; - let actual = UserHookHandler::find_matching_hooks(&groups, Some("Bash")); + let actual = UserHookHandler::::find_matching_hooks(&groups, Some("Bash")); assert_eq!(actual.len(), 1); assert_eq!(actual[0].command, Some("echo hi".to_string())); } @@ -615,42 +680,42 @@ mod tests { #[test] fn test_find_matching_hooks_no_matcher_fires_without_subject() { let groups = vec![make_group(None, &["echo hi"])]; - let actual = UserHookHandler::find_matching_hooks(&groups, None); + let actual = UserHookHandler::::find_matching_hooks(&groups, None); assert_eq!(actual.len(), 1); } #[test] fn test_find_matching_hooks_regex_match() { let groups = vec![make_group(Some("Bash"), &["block.sh"])]; - let actual = UserHookHandler::find_matching_hooks(&groups, Some("Bash")); + let actual = UserHookHandler::::find_matching_hooks(&groups, Some("Bash")); assert_eq!(actual.len(), 1); } #[test] fn test_find_matching_hooks_regex_no_match() { let groups = vec![make_group(Some("Bash"), &["block.sh"])]; - let actual = UserHookHandler::find_matching_hooks(&groups, Some("Write")); + let actual = UserHookHandler::::find_matching_hooks(&groups, Some("Write")); assert!(actual.is_empty()); } #[test] fn test_find_matching_hooks_regex_partial_match() { let groups = vec![make_group(Some("Bash|Write"), &["check.sh"])]; - let actual = UserHookHandler::find_matching_hooks(&groups, Some("Bash")); + let actual = UserHookHandler::::find_matching_hooks(&groups, Some("Bash")); assert_eq!(actual.len(), 1); } #[test] fn test_find_matching_hooks_matcher_but_no_subject() { let groups = vec![make_group(Some("Bash"), &["block.sh"])]; - let actual = UserHookHandler::find_matching_hooks(&groups, None); + let actual = UserHookHandler::::find_matching_hooks(&groups, None); assert!(actual.is_empty()); } #[test] fn test_find_matching_hooks_invalid_regex_skipped() { let groups = vec![make_group(Some("[invalid"), &["block.sh"])]; - let actual = UserHookHandler::find_matching_hooks(&groups, Some("anything")); + let actual = UserHookHandler::::find_matching_hooks(&groups, Some("anything")); assert!(actual.is_empty()); } @@ -661,7 +726,7 @@ mod tests { make_group(Some("Write"), &["write-hook.sh"]), make_group(None, &["always.sh"]), ]; - let actual = UserHookHandler::find_matching_hooks(&groups, Some("Bash")); + let actual = UserHookHandler::::find_matching_hooks(&groups, Some("Bash")); assert_eq!(actual.len(), 2); // Bash match + unconditional } @@ -672,7 +737,7 @@ mod tests { stdout: String::new(), stderr: String::new(), }]; - let actual = UserHookHandler::process_pre_tool_use_output(&results); + let actual = UserHookHandler::::process_pre_tool_use_output(&results); assert!(matches!(actual, PreToolUseDecision::Allow)); } @@ -683,7 +748,7 @@ mod tests { stdout: String::new(), stderr: "Blocked: dangerous command".to_string(), }]; - let actual = UserHookHandler::process_pre_tool_use_output(&results); + let actual = UserHookHandler::::process_pre_tool_use_output(&results); assert!( matches!(actual, PreToolUseDecision::Block(msg) if msg.contains("dangerous command")) ); @@ -696,7 +761,7 @@ mod tests { stdout: r#"{"permissionDecision": "deny", "reason": "Not allowed"}"#.to_string(), stderr: String::new(), }]; - let actual = UserHookHandler::process_pre_tool_use_output(&results); + let actual = UserHookHandler::::process_pre_tool_use_output(&results); assert!(matches!(actual, PreToolUseDecision::Block(msg) if msg == "Not allowed")); } @@ -707,7 +772,7 @@ mod tests { stdout: r#"{"decision": "block", "reason": "Blocked by policy"}"#.to_string(), stderr: String::new(), }]; - let actual = UserHookHandler::process_pre_tool_use_output(&results); + let actual = UserHookHandler::::process_pre_tool_use_output(&results); assert!(matches!(actual, PreToolUseDecision::Block(msg) if msg == "Blocked by policy")); } @@ -718,7 +783,7 @@ mod tests { stdout: String::new(), stderr: "some error".to_string(), }]; - let actual = UserHookHandler::process_pre_tool_use_output(&results); + let actual = UserHookHandler::::process_pre_tool_use_output(&results); assert!(matches!(actual, PreToolUseDecision::Allow)); } @@ -729,7 +794,7 @@ mod tests { stdout: String::new(), stderr: String::new(), }]; - let actual = UserHookHandler::process_results(&results); + let actual = UserHookHandler::::process_results(&results); assert!(actual.is_none()); } @@ -740,7 +805,7 @@ mod tests { stdout: String::new(), stderr: "stop reason".to_string(), }]; - let actual = UserHookHandler::process_results(&results); + let actual = UserHookHandler::::process_results(&results); assert_eq!(actual, Some("stop reason".to_string())); } @@ -751,20 +816,14 @@ mod tests { stdout: r#"{"decision": "block", "reason": "keep going"}"#.to_string(), stderr: String::new(), }]; - let actual = UserHookHandler::process_results(&results); + let actual = UserHookHandler::::process_results(&results); assert_eq!(actual, Some("keep going".to_string())); } #[test] fn test_has_hooks_returns_false_for_empty_config() { let config = UserHookConfig::new(); - let handler = UserHookHandler::new( - config, - PathBuf::from("/tmp"), - PathBuf::from("/tmp"), - "sess-1".to_string(), - 0, - ); + let handler = null_handler(config); assert!(!handler.has_hooks(&UserHookEventName::PreToolUse)); } @@ -772,13 +831,7 @@ mod tests { fn test_has_hooks_returns_true_when_configured() { let json = r#"{"PreToolUse": [{"hooks": [{"type": "command", "command": "echo hi"}]}]}"#; let config: UserHookConfig = serde_json::from_str(json).unwrap(); - let handler = UserHookHandler::new( - config, - PathBuf::from("/tmp"), - PathBuf::from("/tmp"), - "sess-1".to_string(), - 0, - ); + let handler = null_handler(config); assert!(handler.has_hooks(&UserHookEventName::PreToolUse)); assert!(!handler.has_hooks(&UserHookEventName::Stop)); } diff --git a/crates/forge_app/src/infra.rs b/crates/forge_app/src/infra.rs index aab687c1c8..7712c23e8c 100644 --- a/crates/forge_app/src/infra.rs +++ b/crates/forge_app/src/infra.rs @@ -1,6 +1,7 @@ -use std::collections::BTreeMap; +use std::collections::{BTreeMap, HashMap}; use std::hash::Hash; use std::path::{Path, PathBuf}; +use std::time::Duration; use anyhow::Result; use bytes::Bytes; @@ -148,6 +149,31 @@ pub trait CommandInfra: Send + Sync { working_dir: PathBuf, env_vars: Option>, ) -> anyhow::Result; + + /// Executes a shell command with stdin input and a timeout. + /// + /// Pipes `stdin_input` to the process stdin, captures stdout and stderr, + /// and waits up to `timeout` for the process to complete. On timeout, + /// returns `CommandOutput` with `exit_code: None` and a timeout message in + /// `stderr`. + /// + /// # Arguments + /// * `command` - Shell command string to execute. + /// * `working_dir` - Working directory for the command. + /// * `stdin_input` - Data to pipe to the process stdin. + /// * `timeout` - Maximum duration to wait for the command. + /// * `env_vars` - Additional environment variables as key-value pairs. + /// + /// # Errors + /// Returns an error if the process cannot be spawned. + async fn execute_command_with_input( + &self, + command: String, + working_dir: PathBuf, + stdin_input: String, + timeout: Duration, + env_vars: HashMap, + ) -> anyhow::Result; } #[async_trait::async_trait] diff --git a/crates/forge_app/src/services.rs b/crates/forge_app/src/services.rs index 86b82433fa..b90311269e 100644 --- a/crates/forge_app/src/services.rs +++ b/crates/forge_app/src/services.rs @@ -1,3 +1,4 @@ +use std::collections::HashMap; use std::path::{Path, PathBuf}; use std::time::Duration; @@ -554,6 +555,37 @@ pub trait ProviderAuthService: Send + Sync { ) -> anyhow::Result>; } +/// Service for executing hook commands with stdin input and timeout. +/// +/// Abstracts over the underlying process execution so that `UserHookExecutor` +/// depends on a service rather than infrastructure directly. +#[async_trait::async_trait] +pub trait HookCommandService: Send + Sync { + /// Executes a shell command with stdin input and a timeout. + /// + /// Pipes `stdin_input` to the process stdin and waits up to `timeout`. + /// Returns `CommandOutput` with `exit_code: None` and a timeout message in + /// `stderr` when the timeout expires. + /// + /// # Arguments + /// * `command` - Shell command string to execute. + /// * `working_dir` - Working directory for the command. + /// * `stdin_input` - Data to pipe to the process stdin. + /// * `timeout` - Maximum duration to wait for the command. + /// * `env_vars` - Additional environment variables as key-value pairs. + /// + /// # Errors + /// Returns an error if the process cannot be spawned. + async fn execute_command_with_input( + &self, + command: String, + working_dir: PathBuf, + stdin_input: String, + timeout: Duration, + env_vars: HashMap, + ) -> anyhow::Result; +} + pub trait Services: Send + Sync + 'static + Clone + EnvironmentInfra { type ProviderService: ProviderService; type AppConfigService: AppConfigService; @@ -583,6 +615,7 @@ pub trait Services: Send + Sync + 'static + Clone + EnvironmentInfra { type ProviderAuthService: ProviderAuthService; type WorkspaceService: WorkspaceService; type SkillFetchService: SkillFetchService; + type HookCommandService: HookCommandService + Clone; fn provider_service(&self) -> &Self::ProviderService; fn config_service(&self) -> &Self::AppConfigService; @@ -612,6 +645,7 @@ pub trait Services: Send + Sync + 'static + Clone + EnvironmentInfra { fn provider_auth_service(&self) -> &Self::ProviderAuthService; fn workspace_service(&self) -> &Self::WorkspaceService; fn skill_fetch_service(&self) -> &Self::SkillFetchService; + fn hook_command_service(&self) -> &Self::HookCommandService; } #[async_trait::async_trait] diff --git a/crates/forge_infra/src/executor.rs b/crates/forge_infra/src/executor.rs index f4952c9046..9a1abaa43c 100644 --- a/crates/forge_infra/src/executor.rs +++ b/crates/forge_infra/src/executor.rs @@ -1,10 +1,12 @@ +use std::collections::HashMap; use std::io::{self, Write}; use std::path::{Path, PathBuf}; use std::sync::Arc; +use std::time::Duration; use forge_app::CommandInfra; use forge_domain::{CommandOutput, ConsoleWriter as OutputPrinterTrait, Environment}; -use tokio::io::AsyncReadExt; +use tokio::io::{AsyncReadExt, AsyncWriteExt}; use tokio::process::Command; use tokio::sync::Mutex; @@ -224,6 +226,62 @@ impl CommandInfra for ForgeCommandExecutorService { Ok(prepared_command.spawn()?.wait().await?) } + + async fn execute_command_with_input( + &self, + command: String, + working_dir: PathBuf, + stdin_input: String, + timeout: Duration, + env_vars: HashMap, + ) -> anyhow::Result { + let mut prepared_command = self.prepare_command(&command, &working_dir, None); + + // Set directly-provided key-value env vars + for (key, value) in &env_vars { + prepared_command.env(key, value); + } + + // Override stdin to piped so we can write to it + prepared_command.stdin(std::process::Stdio::piped()); + + let mut child = prepared_command.spawn()?; + + // Pipe the JSON input to stdin + if let Some(mut stdin) = child.stdin.take() { + let input = stdin_input.clone(); + tokio::spawn(async move { + let _ = stdin.write_all(input.as_bytes()).await; + let _ = stdin.shutdown().await; + }); + } + + // Wait for the command with timeout + let result = tokio::time::timeout(timeout, child.wait_with_output()).await; + + match result { + Ok(Ok(output)) => Ok(CommandOutput { + command, + exit_code: output.status.code(), + stdout: String::from_utf8_lossy(&output.stdout).into_owned(), + stderr: String::from_utf8_lossy(&output.stderr).into_owned(), + }), + Ok(Err(e)) => Err(e.into()), + Err(_) => { + tracing::warn!( + command = command, + timeout_ms = timeout.as_millis() as u64, + "Hook command timed out" + ); + Ok(CommandOutput { + command, + exit_code: None, + stdout: String::new(), + stderr: format!("Hook command timed out after {}ms", timeout.as_millis()), + }) + } + } + } } #[cfg(test)] diff --git a/crates/forge_infra/src/forge_infra.rs b/crates/forge_infra/src/forge_infra.rs index b899eee341..9aa64e6500 100644 --- a/crates/forge_infra/src/forge_infra.rs +++ b/crates/forge_infra/src/forge_infra.rs @@ -1,7 +1,8 @@ -use std::collections::BTreeMap; +use std::collections::{BTreeMap, HashMap}; use std::path::{Path, PathBuf}; use std::process::ExitStatus; use std::sync::Arc; +use std::time::Duration; use bytes::Bytes; use forge_app::{ @@ -211,6 +212,19 @@ impl CommandInfra for ForgeInfra { .execute_command_raw(command, working_dir, env_vars) .await } + + async fn execute_command_with_input( + &self, + command: String, + working_dir: PathBuf, + stdin_input: String, + timeout: Duration, + env_vars: HashMap, + ) -> anyhow::Result { + self.command_executor_service + .execute_command_with_input(command, working_dir, stdin_input, timeout, env_vars) + .await + } } #[async_trait::async_trait] diff --git a/crates/forge_repo/src/forge_repo.rs b/crates/forge_repo/src/forge_repo.rs index 82df21eda0..a1db110867 100644 --- a/crates/forge_repo/src/forge_repo.rs +++ b/crates/forge_repo/src/forge_repo.rs @@ -1,6 +1,7 @@ -use std::collections::BTreeMap; +use std::collections::{BTreeMap, HashMap}; use std::path::{Path, PathBuf}; use std::sync::Arc; +use std::time::Duration; use bytes::Bytes; use forge_app::{ @@ -453,6 +454,19 @@ where .execute_command_raw(command, working_dir, env_vars) .await } + + async fn execute_command_with_input( + &self, + command: String, + working_dir: PathBuf, + stdin_input: String, + timeout: Duration, + env_vars: HashMap, + ) -> anyhow::Result { + self.infra + .execute_command_with_input(command, working_dir, stdin_input, timeout, env_vars) + .await + } } #[async_trait::async_trait] diff --git a/crates/forge_services/src/forge_services.rs b/crates/forge_services/src/forge_services.rs index 34a0b26eed..e75aeea1e2 100644 --- a/crates/forge_services/src/forge_services.rs +++ b/crates/forge_services/src/forge_services.rs @@ -26,7 +26,8 @@ use crate::provider_service::ForgeProviderService; use crate::template::ForgeTemplateService; use crate::tool_services::{ ForgeFetch, ForgeFollowup, ForgeFsPatch, ForgeFsRead, ForgeFsRemove, ForgeFsSearch, - ForgeFsUndo, ForgeFsWrite, ForgeImageRead, ForgePlanCreate, ForgeShell, ForgeSkillFetch, + ForgeFsUndo, ForgeFsWrite, ForgeHookCommandService, ForgeImageRead, ForgePlanCreate, + ForgeShell, ForgeSkillFetch, }; use crate::user_hook_config::ForgeUserHookConfigService; @@ -85,6 +86,7 @@ pub struct ForgeServices< provider_auth_service: ForgeProviderAuthService, workspace_service: Arc>>, skill_service: Arc>, + hook_command_service: Arc>, infra: Arc, } @@ -145,6 +147,7 @@ impl< discovery, )); let skill_service = Arc::new(ForgeSkillFetch::new(infra.clone())); + let hook_command_service = Arc::new(ForgeHookCommandService::new(infra.clone())); Self { conversation_service, @@ -174,6 +177,7 @@ impl< provider_auth_service, workspace_service, skill_service, + hook_command_service, chat_service, infra, } @@ -242,6 +246,7 @@ impl< type ProviderService = ForgeProviderService; type WorkspaceService = crate::context_engine::ForgeWorkspaceService>; type SkillFetchService = ForgeSkillFetch; + type HookCommandService = ForgeHookCommandService; fn config_service(&self) -> &Self::AppConfigService { &self.config_service @@ -346,6 +351,10 @@ impl< &self.skill_service } + fn hook_command_service(&self) -> &Self::HookCommandService { + &self.hook_command_service + } + fn provider_service(&self) -> &Self::ProviderService { &self.chat_service } diff --git a/crates/forge_services/src/tool_services/mod.rs b/crates/forge_services/src/tool_services/mod.rs index 64a5c6f3c0..75e78f3d7a 100644 --- a/crates/forge_services/src/tool_services/mod.rs +++ b/crates/forge_services/src/tool_services/mod.rs @@ -6,6 +6,7 @@ mod fs_remove; mod fs_search; mod fs_undo; mod fs_write; +mod hook_command; mod image_read; mod plan_create; mod shell; @@ -19,6 +20,7 @@ pub use fs_remove::*; pub use fs_search::*; pub use fs_undo::*; pub use fs_write::*; +pub use hook_command::*; pub use image_read::*; pub use plan_create::*; pub use shell::*; diff --git a/crates/forge_services/src/tool_services/shell.rs b/crates/forge_services/src/tool_services/shell.rs index cdfae686ab..ed117aef07 100644 --- a/crates/forge_services/src/tool_services/shell.rs +++ b/crates/forge_services/src/tool_services/shell.rs @@ -108,6 +108,22 @@ mod tests { ) -> anyhow::Result { unimplemented!() } + + async fn execute_command_with_input( + &self, + command: String, + _working_dir: PathBuf, + _stdin_input: String, + _timeout: std::time::Duration, + _env_vars: std::collections::HashMap, + ) -> anyhow::Result { + Ok(forge_domain::CommandOutput { + command, + exit_code: Some(0), + stdout: String::new(), + stderr: String::new(), + }) + } } impl EnvironmentInfra for MockCommandInfra { From 2ff8b98c4477ae200c899088e417691a0e3ffc31 Mon Sep 17 00:00:00 2001 From: Sandipsinh Dilipsinh Rathod <62684960+ssddOnTop@users.noreply.github.com> Date: Wed, 1 Apr 2026 19:10:27 -0400 Subject: [PATCH 15/23] fix(info): read hook timeout from config instead of env --- crates/forge_main/src/info.rs | 2 +- crates/forge_services/src/user_hook_config.rs | 6 ++++++ 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/crates/forge_main/src/info.rs b/crates/forge_main/src/info.rs index c9bc38af45..609c3daef5 100644 --- a/crates/forge_main/src/info.rs +++ b/crates/forge_main/src/info.rs @@ -387,7 +387,7 @@ impl From<&ForgeConfig> for Info { .add_key_value("ForgeCode Service URL", config.services_url.to_string()) .add_title("TOOL CONFIGURATION") .add_key_value("Tool Timeout", format!("{}s", config.tool_timeout_secs)) - .add_key_value("Hook Timeout", format!("{}ms", env.hook_timeout)) + .add_key_value("Hook Timeout", format!("{}ms", config.hook_timeout_ms.unwrap_or(0))) .add_key_value( "Max Image Size", format!("{} bytes", config.max_image_size_bytes), diff --git a/crates/forge_services/src/user_hook_config.rs b/crates/forge_services/src/user_hook_config.rs index 3ae4c6d89c..f85ec73781 100644 --- a/crates/forge_services/src/user_hook_config.rs +++ b/crates/forge_services/src/user_hook_config.rs @@ -263,6 +263,8 @@ mod tests { } impl EnvironmentInfra for TestInfra { + type Config = forge_config::ForgeConfig; + fn get_env_var(&self, _key: &str) -> Option { None } @@ -278,6 +280,10 @@ mod tests { env } + fn get_config(&self) -> forge_config::ForgeConfig { + Default::default() + } + async fn update_environment( &self, _ops: Vec, From 01a6bf648a6bf84e356cfb336ab62660898328c5 Mon Sep 17 00:00:00 2001 From: Sandipsinh Dilipsinh Rathod <62684960+ssddOnTop@users.noreply.github.com> Date: Wed, 1 Apr 2026 22:11:32 -0400 Subject: [PATCH 16/23] fix merge errors --- crates/forge_app/src/app.rs | 2 - .../forge_app/src/hooks/user_hook_executor.rs | 30 +- .../forge_app/src/hooks/user_hook_handler.rs | 47 +- crates/forge_domain/src/env.rs | 204 +------ crates/forge_infra/src/env.rs | 508 +++++------------- 5 files changed, 143 insertions(+), 648 deletions(-) diff --git a/crates/forge_app/src/app.rs b/crates/forge_app/src/app.rs index f5c9f7a36b..162729c2e7 100644 --- a/crates/forge_app/src/app.rs +++ b/crates/forge_app/src/app.rs @@ -167,9 +167,7 @@ impl ForgeApp { services.hook_command_service().clone(), user_hook_config, environment.cwd.clone(), - environment.cwd.clone(), conversation.id.to_string(), - environment.hook_timeout, ); let user_hook = Hook::default() .on_start(user_handler.clone()) diff --git a/crates/forge_app/src/hooks/user_hook_executor.rs b/crates/forge_app/src/hooks/user_hook_executor.rs index 5d691847b9..40047d90aa 100644 --- a/crates/forge_app/src/hooks/user_hook_executor.rs +++ b/crates/forge_app/src/hooks/user_hook_executor.rs @@ -7,9 +7,6 @@ use tracing::debug; use crate::services::HookCommandService; -/// Default timeout for hook commands (10 minutes). -const DEFAULT_HOOK_TIMEOUT: Duration = Duration::from_secs(600); - /// Executes user hook commands by delegating to a [`HookCommandService`]. /// /// Holds the service by value; the service itself is responsible for any @@ -32,9 +29,7 @@ impl UserHookExecutor { /// # Arguments /// * `command` - The shell command string to execute. /// * `input_json` - JSON string to pipe to the command's stdin. - /// * `timeout` - Optional per-hook timeout in milliseconds. Falls back to - /// `default_timeout_ms` when `None`. - /// * `default_timeout_ms` - Default timeout in milliseconds from the + /// * `timeout` - per-hook timeout in milliseconds. Falls back to /// environment configuration. Uses the built-in default (10 min) when /// zero. /// * `cwd` - Working directory for the command. @@ -46,19 +41,10 @@ impl UserHookExecutor { &self, command: &str, input_json: &str, - timeout: Option, - default_timeout_ms: u64, + timeout_duration: Duration, cwd: &PathBuf, env_vars: &HashMap, ) -> anyhow::Result { - let timeout_duration = timeout.map(Duration::from_millis).unwrap_or_else(|| { - if default_timeout_ms > 0 { - Duration::from_millis(default_timeout_ms) - } else { - DEFAULT_HOOK_TIMEOUT - } - }); - debug!( command = command, cwd = %cwd.display(), @@ -168,8 +154,7 @@ mod tests { .execute( "echo hello", "{}", - None, - 0, + Duration::from_secs(0), &std::env::current_dir().unwrap(), &HashMap::new(), ) @@ -188,8 +173,7 @@ mod tests { .execute( "exit 2", "{}", - None, - 0, + Duration::from_secs(0), &std::env::current_dir().unwrap(), &HashMap::new(), ) @@ -208,8 +192,7 @@ mod tests { .execute( "exit 1", "{}", - None, - 0, + Duration::from_secs(0), &std::env::current_dir().unwrap(), &HashMap::new(), ) @@ -227,8 +210,7 @@ mod tests { .execute( "sleep 10", "{}", - Some(100), - 0, + Duration::from_millis(100), &std::env::current_dir().unwrap(), &HashMap::new(), ) diff --git a/crates/forge_app/src/hooks/user_hook_handler.rs b/crates/forge_app/src/hooks/user_hook_handler.rs index 486f4d2295..92ff6a8c0c 100644 --- a/crates/forge_app/src/hooks/user_hook_handler.rs +++ b/crates/forge_app/src/hooks/user_hook_handler.rs @@ -1,7 +1,3 @@ -use std::collections::HashMap; -use std::path::PathBuf; -use std::sync::atomic::{AtomicBool, Ordering}; - use async_trait::async_trait; use forge_domain::{ ContextMessage, Conversation, EndPayload, EventData, EventHandle, HookEventInput, @@ -10,41 +6,34 @@ use forge_domain::{ UserHookEventName, UserHookMatcherGroup, }; use regex::Regex; +use std::collections::HashMap; +use std::path::PathBuf; +use std::sync::atomic::{AtomicBool, Ordering}; +use std::time::Duration; use tracing::{debug, warn}; use super::user_hook_executor::UserHookExecutor; use crate::services::HookCommandService; +/// Default timeout for hook commands (10 minutes). +const DEFAULT_HOOK_TIMEOUT: Duration = Duration::from_secs(600); + /// EventHandle implementation that bridges user-configured hooks with the /// existing lifecycle event system. /// /// This handler is constructed from a `UserHookConfig` and executes matching /// hook commands at each lifecycle event point. It wires into the existing /// `Hook` system via `Hook::zip()`. +#[derive(Clone)] pub struct UserHookHandler { executor: UserHookExecutor, config: UserHookConfig, cwd: PathBuf, env_vars: HashMap, - /// Default timeout in milliseconds for hook commands from the environment. - default_hook_timeout: u64, /// Tracks whether a Stop hook has already fired to prevent infinite loops. stop_hook_active: std::sync::Arc, } -impl Clone for UserHookHandler { - fn clone(&self) -> Self { - Self { - executor: self.executor.clone(), - config: self.config.clone(), - cwd: self.cwd.clone(), - env_vars: self.env_vars.clone(), - default_hook_timeout: self.default_hook_timeout, - stop_hook_active: self.stop_hook_active.clone(), - } - } -} - impl UserHookHandler { /// Creates a new user hook handler from configuration. /// @@ -61,14 +50,12 @@ impl UserHookHandler { service: I, config: UserHookConfig, cwd: PathBuf, - project_dir: PathBuf, session_id: String, - default_hook_timeout: u64, ) -> Self { let mut env_vars = HashMap::new(); env_vars.insert( "FORGE_PROJECT_DIR".to_string(), - project_dir.to_string_lossy().to_string(), + cwd.to_string_lossy().to_string(), ); env_vars.insert("FORGE_SESSION_ID".to_string(), session_id); env_vars.insert("FORGE_CWD".to_string(), cwd.to_string_lossy().to_string()); @@ -78,7 +65,6 @@ impl UserHookHandler { config, cwd, env_vars, - default_hook_timeout, stop_hook_active: std::sync::Arc::new(AtomicBool::new(false)), } } @@ -152,8 +138,9 @@ impl UserHookHandler { .execute( command, &input_json, - hook.timeout, - self.default_hook_timeout, + hook.timeout + .map(Duration::from_millis) + .unwrap_or(DEFAULT_HOOK_TIMEOUT), &self.cwd, &self.env_vars, ) @@ -395,9 +382,7 @@ impl EventHandle> for UserHook } #[async_trait] -impl EventHandle> - for UserHookHandler -{ +impl EventHandle> for UserHookHandler { async fn handle( &self, event: &EventData, @@ -462,9 +447,7 @@ impl EventHandle> } #[async_trait] -impl EventHandle> - for UserHookHandler -{ +impl EventHandle> for UserHookHandler { async fn handle( &self, event: &EventData, @@ -648,9 +631,7 @@ mod tests { NullInfra, config, PathBuf::from("/tmp"), - PathBuf::from("/tmp"), "sess-1".to_string(), - 0, ) } diff --git a/crates/forge_domain/src/env.rs b/crates/forge_domain/src/env.rs index 23e3f0b0b5..a614ce0e27 100644 --- a/crates/forge_domain/src/env.rs +++ b/crates/forge_domain/src/env.rs @@ -1,16 +1,11 @@ use std::hash::{DefaultHasher, Hash, Hasher}; use std::path::PathBuf; -use std::str::FromStr; use derive_more::Display; use derive_setters::Setters; use serde::{Deserialize, Serialize}; -use url::Url; -use crate::{ - Compact, HttpConfig, MaxTokens, ModelId, ProviderId, RetryConfig, - Temperature, TopK, TopP, Update, -}; +use crate::{ModelId, ProviderId}; /// Domain-level session configuration pairing a provider with a model. /// @@ -70,206 +65,9 @@ pub struct Environment { pub shell: String, /// The base path relative to which everything else is stored. pub base_path: PathBuf, - /// Configuration for the retry mechanism - pub retry_config: RetryConfig, - /// The maximum number of lines returned for FSSearch. - pub max_search_lines: usize, - /// Maximum bytes allowed for search results - pub max_search_result_bytes: usize, - /// Maximum characters for fetch content - pub fetch_truncation_limit: usize, - /// Maximum lines for shell output prefix - pub stdout_max_prefix_length: usize, - /// Maximum lines for shell output suffix - pub stdout_max_suffix_length: usize, - /// Maximum characters per line for shell output - pub stdout_max_line_length: usize, - /// Maximum characters per line for file read operations - /// Controlled by FORGE_MAX_LINE_LENGTH environment variable. - pub max_line_length: usize, - /// Maximum number of lines to read from a file - pub max_read_size: u64, - /// Maximum number of files that can be read in a single batch operation. - /// Controlled by FORGE_MAX_READ_BATCH_SIZE environment variable. - pub max_file_read_batch_size: usize, - /// Http configuration - pub http: HttpConfig, - /// Maximum file size in bytes for operations - pub max_file_size: u64, - /// Maximum image file size in bytes for binary read operations - pub max_image_size: u64, - /// Maximum execution time in seconds for a single tool call. - /// Controls how long a tool can run before being terminated. - pub tool_timeout: u64, - /// Default timeout in milliseconds for user hook commands. - /// Individual hooks can override this via their own `timeout` field. - /// Controlled by FORGE_HOOK_TIMEOUT_MS environment variable. - pub hook_timeout: u64, - /// Whether to automatically open HTML dump files in the browser. - /// Controlled by FORGE_DUMP_AUTO_OPEN environment variable. - pub auto_open_dump: bool, - /// Path where debug request files should be written. - /// Controlled by FORGE_DEBUG_REQUESTS environment variable. - pub debug_requests: Option, - /// Custom history file path from FORGE_HISTORY_FILE environment variable. - /// If None, uses the default history path. - pub custom_history_path: Option, - /// Maximum number of conversations to show in list. - /// Controlled by FORGE_MAX_CONVERSATIONS environment variable. - pub max_conversations: usize, - /// Maximum number of results to return from initial vector search. - /// Controlled by FORGE_SEM_SEARCH_LIMIT environment variable. - pub sem_search_limit: usize, - /// Top-k parameter for relevance filtering during semantic search. - /// Controls the number of nearest neighbors to consider. - /// Controlled by FORGE_SEM_SEARCH_TOP_K environment variable. - pub sem_search_top_k: usize, - /// URL for the indexing server. - /// Controlled by FORGE_WORKSPACE_SERVER_URL environment variable. - #[dummy(expr = "url::Url::parse(\"http://localhost:8080\").unwrap()")] - pub service_url: Url, - /// Maximum number of file extensions to include in the system prompt. - /// Controlled by FORGE_MAX_EXTENSIONS environment variable. - pub max_extensions: usize, - /// Format for automatically creating a dump when a task is completed. - /// Controlled by FORGE_AUTO_DUMP environment variable. - /// Set to "json" (or "true"/"1"/"yes") for JSON, "html" for HTML, or - /// unset/other to disable. - pub auto_dump: Option, - /// Maximum number of files read concurrently in parallel operations. - /// Controlled by FORGE_PARALLEL_FILE_READS environment variable. - /// Caps the `buffer_unordered` concurrency to avoid EMFILE errors. - pub parallel_file_reads: usize, - /// TTL in seconds for the model API list cache. - /// Controlled by FORGE_MODEL_CACHE_TTL environment variable. - pub model_cache_ttl: u64, - - // --- User configuration fields (from ForgeConfig) --- - /// The active session (provider + model). - #[dummy(default)] - pub session: Option, - /// Provider and model for commit message generation. - #[dummy(default)] - pub commit: Option, - /// Provider and model for shell command suggestion generation. - #[dummy(default)] - pub suggest: Option, - /// Whether the application is running in restricted mode. - /// When true, tool execution requires explicit permission grants. - pub is_restricted: bool, - - /// Whether tool use is supported in the current environment. - /// When false, tool calls are disabled regardless of agent configuration. - pub tool_supported: bool, - - // --- Workflow configuration fields --- - /// Output randomness for all agents; lower values are deterministic, higher - /// values are creative (0.0–2.0). - #[dummy(default)] - #[serde(default, skip_serializing_if = "Option::is_none")] - pub temperature: Option, - - /// Nucleus sampling threshold for all agents; limits token selection to the - /// top cumulative probability mass (0.0–1.0). - #[dummy(default)] - #[serde(default, skip_serializing_if = "Option::is_none")] - pub top_p: Option, - - /// Top-k vocabulary cutoff for all agents; restricts sampling to the k - /// highest-probability tokens (1–1000). - #[dummy(default)] - #[serde(default, skip_serializing_if = "Option::is_none")] - pub top_k: Option, - - /// Maximum tokens the model may generate per response for all agents - /// (1–100,000). - #[dummy(default)] - #[serde(default, skip_serializing_if = "Option::is_none")] - pub max_tokens: Option, - - /// Maximum tool failures per turn before the orchestrator forces - /// completion. - #[serde(default, skip_serializing_if = "Option::is_none")] - pub max_tool_failure_per_turn: Option, - - /// Maximum number of requests that can be made in a single turn. - #[serde(default, skip_serializing_if = "Option::is_none")] - pub max_requests_per_turn: Option, - - /// Context compaction settings applied to all agents. - #[dummy(default)] - #[serde(default, skip_serializing_if = "Option::is_none")] - pub compact: Option, - - /// Configuration for automatic forge updates. - #[dummy(default)] - #[serde(default, skip_serializing_if = "Option::is_none")] - pub updates: Option, -} - -/// The output format used when auto-dumping a conversation on task completion. -#[derive(Debug, Clone, PartialEq, Serialize, Deserialize, fake::Dummy)] -#[serde(rename_all = "camelCase")] -pub enum AutoDumpFormat { - /// Dump as a JSON file. - Json, - /// Dump as an HTML file. - Html, -} - -impl FromStr for AutoDumpFormat { - type Err = (); - - fn from_str(s: &str) -> Result { - match s.to_lowercase().as_str() { - "html" => Ok(AutoDumpFormat::Html), - "json" | "true" | "1" | "yes" => Ok(AutoDumpFormat::Json), - _ => Err(()), - } - } } impl Environment { - /// Applies a single [`ConfigOperation`] to this environment in-place. - pub fn apply_op(&mut self, op: ConfigOperation) { - match op { - ConfigOperation::SetProvider(provider_id) => { - let pid = provider_id.as_ref().to_string(); - self.session = Some(match self.session.take() { - Some(sc) => sc.provider_id(pid), - None => SessionConfig::default().provider_id(pid), - }); - } - ConfigOperation::SetModel(provider_id, model_id) => { - let pid = provider_id.as_ref().to_string(); - let mid = model_id.to_string(); - self.session = Some(match self.session.take() { - Some(sc) if sc.provider_id.as_deref() == Some(&pid) => sc.model_id(mid), - _ => SessionConfig::default().provider_id(pid).model_id(mid), - }); - } - ConfigOperation::SetCommitConfig(commit) => { - self.commit = - commit - .provider - .as_ref() - .zip(commit.model.as_ref()) - .map(|(pid, mid)| { - SessionConfig::default() - .provider_id(pid.as_ref().to_string()) - .model_id(mid.to_string()) - }); - } - ConfigOperation::SetSuggestConfig(suggest) => { - self.suggest = Some( - SessionConfig::default() - .provider_id(suggest.provider.as_ref().to_string()) - .model_id(suggest.model.to_string()), - ); - } - } - } - pub fn log_path(&self) -> PathBuf { self.base_path.join("logs") } diff --git a/crates/forge_infra/src/env.rs b/crates/forge_infra/src/env.rs index 10bce7b7b6..c2b2d91f15 100644 --- a/crates/forge_infra/src/env.rs +++ b/crates/forge_infra/src/env.rs @@ -4,122 +4,17 @@ use std::sync::Arc; use forge_app::EnvironmentInfra; use forge_config::{ConfigReader, ForgeConfig, ModelConfig}; -use forge_domain::{ - AutoDumpFormat, Compact, ConfigOperation, Environment, HttpConfig, MaxTokens, ModelId, - RetryConfig, SessionConfig, Temperature, TlsBackend, TlsVersion, TopK, TopP, Update, - UpdateFrequency, -}; -use reqwest::Url; +use forge_domain::{ConfigOperation, Environment}; use tracing::{debug, error}; -/// Converts a [`ModelConfig`] into a domain-level [`SessionConfig`]. -fn to_session_config(mc: &ModelConfig) -> SessionConfig { - SessionConfig { - provider_id: mc.provider_id.clone(), - model_id: mc.model_id.clone(), - } -} - -/// Converts a [`forge_config::TlsVersion`] into a [`forge_domain::TlsVersion`]. -fn to_tls_version(v: forge_config::TlsVersion) -> TlsVersion { - match v { - forge_config::TlsVersion::V1_0 => TlsVersion::V1_0, - forge_config::TlsVersion::V1_1 => TlsVersion::V1_1, - forge_config::TlsVersion::V1_2 => TlsVersion::V1_2, - forge_config::TlsVersion::V1_3 => TlsVersion::V1_3, - } -} - -/// Converts a [`forge_config::TlsBackend`] into a [`forge_domain::TlsBackend`]. -fn to_tls_backend(b: forge_config::TlsBackend) -> TlsBackend { - match b { - forge_config::TlsBackend::Default => TlsBackend::Default, - forge_config::TlsBackend::Rustls => TlsBackend::Rustls, - } -} - -/// Converts a [`forge_config::HttpConfig`] into a [`forge_domain::HttpConfig`]. -fn to_http_config(h: forge_config::HttpConfig) -> HttpConfig { - HttpConfig { - connect_timeout: h.connect_timeout_secs, - read_timeout: h.read_timeout_secs, - pool_idle_timeout: h.pool_idle_timeout_secs, - pool_max_idle_per_host: h.pool_max_idle_per_host, - max_redirects: h.max_redirects, - hickory: h.hickory, - tls_backend: to_tls_backend(h.tls_backend), - min_tls_version: h.min_tls_version.map(to_tls_version), - max_tls_version: h.max_tls_version.map(to_tls_version), - adaptive_window: h.adaptive_window, - keep_alive_interval: h.keep_alive_interval_secs, - keep_alive_timeout: h.keep_alive_timeout_secs, - keep_alive_while_idle: h.keep_alive_while_idle, - accept_invalid_certs: h.accept_invalid_certs, - root_cert_paths: h.root_cert_paths, - } -} - -/// Converts a [`forge_config::RetryConfig`] into a -/// [`forge_domain::RetryConfig`]. -fn to_retry_config(r: forge_config::RetryConfig) -> RetryConfig { - RetryConfig { - initial_backoff_ms: r.initial_backoff_ms, - min_delay_ms: r.min_delay_ms, - backoff_factor: r.backoff_factor, - max_retry_attempts: r.max_attempts, - retry_status_codes: r.status_codes, - max_delay: r.max_delay_secs, - suppress_retry_errors: r.suppress_errors, - } -} - -/// Converts a [`forge_config::AutoDumpFormat`] into a -/// [`forge_domain::AutoDumpFormat`]. -fn to_auto_dump_format(f: forge_config::AutoDumpFormat) -> AutoDumpFormat { - match f { - forge_config::AutoDumpFormat::Json => AutoDumpFormat::Json, - forge_config::AutoDumpFormat::Html => AutoDumpFormat::Html, - } -} - -/// Converts a [`forge_config::UpdateFrequency`] into a -/// [`forge_domain::UpdateFrequency`]. -fn to_update_frequency(f: forge_config::UpdateFrequency) -> UpdateFrequency { - match f { - forge_config::UpdateFrequency::Daily => UpdateFrequency::Daily, - forge_config::UpdateFrequency::Weekly => UpdateFrequency::Weekly, - forge_config::UpdateFrequency::Always => UpdateFrequency::Always, - } -} - -/// Converts a [`forge_config::Update`] into a [`forge_domain::Update`]. -fn to_update(u: forge_config::Update) -> Update { - Update { - frequency: u.frequency.map(to_update_frequency), - auto_update: u.auto_update, - } -} - -/// Converts a [`forge_config::Compact`] into a [`forge_domain::Compact`]. -fn to_compact(c: forge_config::Compact) -> Compact { - Compact { - retention_window: c.retention_window, - eviction_window: c.eviction_window.value(), - max_tokens: c.max_tokens, - token_threshold: c.token_threshold, - turn_threshold: c.turn_threshold, - message_threshold: c.message_threshold, - model: c.model.map(ModelId::new), - on_turn_end: c.on_turn_end, - } -} - -/// Builds a [`forge_domain::Environment`] entirely from a [`ForgeConfig`] and -/// runtime context (`cwd`), mapping every config field to its corresponding -/// environment field. -fn to_environment(fc: ForgeConfig, cwd: PathBuf) -> Environment { +/// Builds a [`forge_domain::Environment`] from runtime context only. +/// +/// Only the six fields that cannot be sourced from [`ForgeConfig`] are set +/// here: `os`, `pid`, `cwd`, `home`, `shell`, and `base_path`. All +/// configuration values are now accessed through +/// `EnvironmentInfra::get_config()`. +fn to_environment(cwd: PathBuf) -> Environment { Environment { - // --- Infrastructure-derived fields --- os: std::env::consts::OS.to_string(), pid: std::process::id(), cwd, @@ -132,225 +27,47 @@ fn to_environment(fc: ForgeConfig, cwd: PathBuf) -> Environment { base_path: dirs::home_dir() .map(|h| h.join("forge")) .unwrap_or_else(|| PathBuf::from(".").join("forge")), - - // --- ForgeConfig-mapped fields --- - retry_config: fc.retry.map(to_retry_config).unwrap_or_default(), - max_search_lines: fc.max_search_lines, - max_search_result_bytes: fc.max_search_result_bytes, - fetch_truncation_limit: fc.max_fetch_chars, - stdout_max_prefix_length: fc.max_stdout_prefix_lines, - stdout_max_suffix_length: fc.max_stdout_suffix_lines, - stdout_max_line_length: fc.max_stdout_line_chars, - max_line_length: fc.max_line_chars, - max_read_size: fc.max_read_lines, - max_file_read_batch_size: fc.max_file_read_batch_size, - http: fc.http.map(to_http_config).unwrap_or_default(), - max_file_size: fc.max_file_size_bytes, - max_image_size: fc.max_image_size_bytes, - tool_timeout: fc.tool_timeout_secs, - hook_timeout: fc.hook_timeout_ms.unwrap_or_default(), - auto_open_dump: fc.auto_open_dump, - debug_requests: fc.debug_requests, - custom_history_path: fc.custom_history_path, - max_conversations: fc.max_conversations, - sem_search_limit: fc.max_sem_search_results, - sem_search_top_k: fc.sem_search_top_k, - service_url: Url::parse(&fc.services_url) - .unwrap_or_else(|_| Url::parse("https://api.forgecode.dev").unwrap()), - max_extensions: fc.max_extensions, - auto_dump: fc.auto_dump.map(to_auto_dump_format), - parallel_file_reads: fc.max_parallel_file_reads, - model_cache_ttl: fc.model_cache_ttl_secs, - session: fc.session.as_ref().map(to_session_config), - commit: fc.commit.as_ref().map(to_session_config), - suggest: fc.suggest.as_ref().map(to_session_config), - is_restricted: fc.restricted, - tool_supported: fc.tool_supported, - temperature: fc - .temperature - .and_then(|v| Temperature::new(v.value() as f32).ok()), - top_p: fc.top_p.and_then(|v| TopP::new(v.value() as f32).ok()), - top_k: fc.top_k.and_then(|v| TopK::new(v).ok()), - max_tokens: fc.max_tokens.and_then(|v| MaxTokens::new(v).ok()), - max_tool_failure_per_turn: fc.max_tool_failure_per_turn, - max_requests_per_turn: fc.max_requests_per_turn, - compact: fc.compact.map(to_compact), - updates: fc.updates.map(to_update), - } -} - -/// Converts a [`forge_domain::RetryConfig`] back into a -/// [`forge_config::RetryConfig`]. -fn from_retry_config(r: &RetryConfig) -> forge_config::RetryConfig { - forge_config::RetryConfig { - initial_backoff_ms: r.initial_backoff_ms, - min_delay_ms: r.min_delay_ms, - backoff_factor: r.backoff_factor, - max_attempts: r.max_retry_attempts, - status_codes: r.retry_status_codes.clone(), - max_delay_secs: r.max_delay, - suppress_errors: r.suppress_retry_errors, - } -} - -/// Converts a [`forge_domain::HttpConfig`] back into a -/// [`forge_config::HttpConfig`]. -fn from_http_config(h: &HttpConfig) -> forge_config::HttpConfig { - forge_config::HttpConfig { - connect_timeout_secs: h.connect_timeout, - read_timeout_secs: h.read_timeout, - pool_idle_timeout_secs: h.pool_idle_timeout, - pool_max_idle_per_host: h.pool_max_idle_per_host, - max_redirects: h.max_redirects, - hickory: h.hickory, - tls_backend: from_tls_backend(h.tls_backend.clone()), - min_tls_version: h.min_tls_version.clone().map(from_tls_version), - max_tls_version: h.max_tls_version.clone().map(from_tls_version), - adaptive_window: h.adaptive_window, - keep_alive_interval_secs: h.keep_alive_interval, - keep_alive_timeout_secs: h.keep_alive_timeout, - keep_alive_while_idle: h.keep_alive_while_idle, - accept_invalid_certs: h.accept_invalid_certs, - root_cert_paths: h.root_cert_paths.clone(), } } -/// Converts a [`forge_domain::TlsVersion`] back into a -/// [`forge_config::TlsVersion`]. -fn from_tls_version(v: TlsVersion) -> forge_config::TlsVersion { - match v { - TlsVersion::V1_0 => forge_config::TlsVersion::V1_0, - TlsVersion::V1_1 => forge_config::TlsVersion::V1_1, - TlsVersion::V1_2 => forge_config::TlsVersion::V1_2, - TlsVersion::V1_3 => forge_config::TlsVersion::V1_3, - } -} - -/// Converts a [`forge_domain::TlsBackend`] back into a -/// [`forge_config::TlsBackend`]. -fn from_tls_backend(b: TlsBackend) -> forge_config::TlsBackend { - match b { - TlsBackend::Default => forge_config::TlsBackend::Default, - TlsBackend::Rustls => forge_config::TlsBackend::Rustls, - } -} - -/// Converts a [`forge_domain::AutoDumpFormat`] back into a -/// [`forge_config::AutoDumpFormat`]. -fn from_auto_dump_format(f: &AutoDumpFormat) -> forge_config::AutoDumpFormat { - match f { - AutoDumpFormat::Json => forge_config::AutoDumpFormat::Json, - AutoDumpFormat::Html => forge_config::AutoDumpFormat::Html, - } -} - -/// Converts a [`forge_domain::UpdateFrequency`] back into a -/// [`forge_config::UpdateFrequency`]. -fn from_update_frequency(f: UpdateFrequency) -> forge_config::UpdateFrequency { - match f { - UpdateFrequency::Daily => forge_config::UpdateFrequency::Daily, - UpdateFrequency::Weekly => forge_config::UpdateFrequency::Weekly, - UpdateFrequency::Always => forge_config::UpdateFrequency::Always, - } -} - -/// Converts a [`forge_domain::Update`] back into a [`forge_config::Update`]. -fn from_update(u: &Update) -> forge_config::Update { - forge_config::Update { - frequency: u.frequency.clone().map(from_update_frequency), - auto_update: u.auto_update, - } -} - -/// Converts a [`forge_domain::Compact`] back into a [`forge_config::Compact`]. -fn from_compact(c: &Compact) -> forge_config::Compact { - forge_config::Compact { - retention_window: c.retention_window, - eviction_window: forge_config::Percentage::from(c.eviction_window), - max_tokens: c.max_tokens, - token_threshold: c.token_threshold, - turn_threshold: c.turn_threshold, - message_threshold: c.message_threshold, - model: c.model.as_ref().map(|m| m.to_string()), - on_turn_end: c.on_turn_end, - } -} - -/// Converts an [`Environment`] back into a [`ForgeConfig`] suitable for -/// persisting. +/// Applies a single [`ConfigOperation`] directly to a [`ForgeConfig`]. /// -/// Builds a fresh [`ForgeConfig`] from [`ForgeConfig::default()`] and maps -/// every field that originated from [`ForgeConfig`] back from the -/// [`Environment`], preserving the round-trip identity. -fn to_forge_config(env: &Environment) -> ForgeConfig { - let mut fc = ForgeConfig::default(); - - // --- Fields mapped through Environment --- - let default_retry = RetryConfig::default(); - fc.retry = if env.retry_config == default_retry { - None - } else { - Some(from_retry_config(&env.retry_config)) - }; - fc.max_search_lines = env.max_search_lines; - fc.max_search_result_bytes = env.max_search_result_bytes; - fc.max_fetch_chars = env.fetch_truncation_limit; - fc.max_stdout_prefix_lines = env.stdout_max_prefix_length; - fc.max_stdout_suffix_lines = env.stdout_max_suffix_length; - fc.max_stdout_line_chars = env.stdout_max_line_length; - fc.max_line_chars = env.max_line_length; - fc.max_read_lines = env.max_read_size; - fc.max_file_read_batch_size = env.max_file_read_batch_size; - let default_http = HttpConfig::default(); - fc.http = if env.http == default_http { - None - } else { - Some(from_http_config(&env.http)) - }; - fc.max_file_size_bytes = env.max_file_size; - fc.max_image_size_bytes = env.max_image_size; - fc.tool_timeout_secs = env.tool_timeout; - fc.hook_timeout_ms = Some(env.hook_timeout); - fc.auto_open_dump = env.auto_open_dump; - fc.debug_requests = env.debug_requests.clone(); - fc.custom_history_path = env.custom_history_path.clone(); - fc.max_conversations = env.max_conversations; - fc.max_sem_search_results = env.sem_search_limit; - fc.sem_search_top_k = env.sem_search_top_k; - fc.services_url = env.service_url.to_string(); - fc.max_extensions = env.max_extensions; - fc.auto_dump = env.auto_dump.as_ref().map(from_auto_dump_format); - fc.max_parallel_file_reads = env.parallel_file_reads; - fc.model_cache_ttl_secs = env.model_cache_ttl; - fc.restricted = env.is_restricted; - fc.tool_supported = env.tool_supported; - - // --- Workflow fields --- - fc.temperature = env - .temperature - .map(|t| forge_config::Decimal(t.value() as f64)); - fc.top_p = env.top_p.map(|t| forge_config::Decimal(t.value() as f64)); - fc.top_k = env.top_k.map(|t| t.value()); - fc.max_tokens = env.max_tokens.map(|t| t.value()); - fc.max_tool_failure_per_turn = env.max_tool_failure_per_turn; - fc.max_requests_per_turn = env.max_requests_per_turn; - fc.compact = env.compact.as_ref().map(from_compact); - fc.updates = env.updates.as_ref().map(from_update); - - // --- Session configs --- - fc.session = env.session.as_ref().map(|sc| ModelConfig { - provider_id: sc.provider_id.clone(), - model_id: sc.model_id.clone(), - }); - fc.commit = env.commit.as_ref().map(|sc| ModelConfig { - provider_id: sc.provider_id.clone(), - model_id: sc.model_id.clone(), - }); - fc.suggest = env.suggest.as_ref().map(|sc| ModelConfig { - provider_id: sc.provider_id.clone(), - model_id: sc.model_id.clone(), - }); - fc +/// Used by [`ForgeEnvironmentInfra::update_environment`] to mutate the +/// persisted config without an intermediate `Environment` round-trip. +fn apply_config_op(fc: &mut ForgeConfig, op: ConfigOperation) { + match op { + ConfigOperation::SetProvider(pid) => { + let session = fc.session.get_or_insert_with(ModelConfig::default); + session.provider_id = Some(pid.as_ref().to_string()); + } + ConfigOperation::SetModel(pid, mid) => { + let pid_str = pid.as_ref().to_string(); + let mid_str = mid.to_string(); + let session = fc.session.get_or_insert_with(ModelConfig::default); + if session.provider_id.as_deref() == Some(&pid_str) { + session.model_id = Some(mid_str); + } else { + fc.session = + Some(ModelConfig { provider_id: Some(pid_str), model_id: Some(mid_str) }); + } + } + ConfigOperation::SetCommitConfig(commit) => { + fc.commit = commit + .provider + .as_ref() + .zip(commit.model.as_ref()) + .map(|(pid, mid)| ModelConfig { + provider_id: Some(pid.as_ref().to_string()), + model_id: Some(mid.to_string()), + }); + } + ConfigOperation::SetSuggestConfig(suggest) => { + fc.suggest = Some(ModelConfig { + provider_id: Some(suggest.provider.as_ref().to_string()), + model_id: Some(suggest.model.to_string()), + }); + } + } } /// Infrastructure implementation for managing application configuration with @@ -414,7 +131,7 @@ impl EnvironmentInfra for ForgeEnvironmentInfra { } fn get_environment(&self) -> Environment { - to_environment(self.cached_config(), self.cwd.clone()) + to_environment(self.cwd.clone()) } fn get_config(&self) -> ForgeConfig { @@ -422,23 +139,18 @@ impl EnvironmentInfra for ForgeEnvironmentInfra { } async fn update_environment(&self, ops: Vec) -> anyhow::Result<()> { - // Load the global config - let fc = ConfigReader::default() + // Load the global config (with defaults applied) for the update round-trip + let mut fc = ConfigReader::default() .read_defaults() .read_global() .build()?; - debug!(config = ?fc, "loaded config for update"); + debug!(config = ?fc, ?ops, "applying app config operations"); - // Convert to Environment and apply each operation - debug!(?ops, "applying app config operations"); - let mut env = to_environment(fc, self.cwd.clone()); for op in ops { - env.apply_op(op); + apply_config_op(&mut fc, op); } - // Convert Environment back to ForgeConfig and persist - let fc = to_forge_config(&env); fc.write()?; debug!(config = ?fc, "written .forge.toml"); @@ -459,60 +171,84 @@ mod tests { use super::*; #[test] - fn test_to_environment_default_config() { - let fixture = ForgeConfig::default(); - let actual = to_environment(fixture, PathBuf::from("/test/cwd")); - - // Config-derived fields should all be zero/default since ForgeConfig - // derives Default (all-zeros) without the defaults file. - assert_eq!(actual.cwd, PathBuf::from("/test/cwd")); - assert!(!actual.is_restricted); - assert_eq!(actual.retry_config, RetryConfig::default()); - assert_eq!(actual.http, HttpConfig::default()); - assert!(!actual.auto_open_dump); - assert_eq!(actual.auto_dump, None); - assert_eq!(actual.debug_requests, None); - assert_eq!(actual.custom_history_path, None); - assert_eq!(actual.session, None); - assert_eq!(actual.commit, None); - assert_eq!(actual.suggest, None); + fn test_to_environment_sets_cwd() { + let fixture_cwd = PathBuf::from("/test/cwd"); + let actual = to_environment(fixture_cwd.clone()); + assert_eq!(actual.cwd, fixture_cwd); } #[test] - fn test_to_environment_restricted_mode() { - let fixture = ForgeConfig::default().restricted(true); - let actual = to_environment(fixture, PathBuf::from("/tmp")); + fn test_apply_config_op_set_provider() { + use forge_domain::ProviderId; + + let mut fixture = ForgeConfig::default(); + apply_config_op( + &mut fixture, + ConfigOperation::SetProvider(ProviderId::ANTHROPIC), + ); + + let actual = fixture + .session + .as_ref() + .and_then(|s| s.provider_id.as_deref()); + let expected = Some("anthropic"); - assert!(actual.is_restricted); + assert_eq!(actual, expected); } #[test] - fn test_forge_config_environment_identity() { - // Property test: for ANY randomly generated ForgeConfig `fc`, the - // config-mapped fields of the Environment produced by - // `to_environment(fc)` must survive a full round-trip through - // `to_forge_config` and back unchanged. - // - // fc --> env --> fc' --> env' - // ^ ^ - // |--- config fields --| must be equal - use fake::{Fake, Faker}; - - let cwd = PathBuf::from("/identity/test"); - - for _ in 0..100 { - let fixture: ForgeConfig = Faker.fake(); - - // fc -> env -> fc' -> env' - let env = to_environment(fixture, cwd.clone()); - let fc_prime = to_forge_config(&env); - let env_prime = to_environment(fc_prime, cwd.clone()); - - // Infrastructure-derived fields (os, pid, home, shell, base_path) - // are re-derived from the runtime, so they are equal by - // construction. Config-mapped fields must satisfy the identity: - // env == env' - assert_eq!(env, env_prime); - } + fn test_apply_config_op_set_model_matching_provider() { + use forge_domain::{ModelId, ProviderId}; + + let mut fixture = ForgeConfig { + session: Some(ModelConfig { + provider_id: Some("anthropic".to_string()), + model_id: None, + }), + ..Default::default() + }; + + apply_config_op( + &mut fixture, + ConfigOperation::SetModel( + ProviderId::ANTHROPIC, + ModelId::new("claude-3-5-sonnet-20241022"), + ), + ); + + let actual = fixture.session.as_ref().and_then(|s| s.model_id.as_deref()); + let expected = Some("claude-3-5-sonnet-20241022"); + + assert_eq!(actual, expected); + } + + #[test] + fn test_apply_config_op_set_model_different_provider_replaces_session() { + use forge_domain::{ModelId, ProviderId}; + + let mut fixture = ForgeConfig { + session: Some(ModelConfig { + provider_id: Some("openai".to_string()), + model_id: Some("gpt-4".to_string()), + }), + ..Default::default() + }; + + apply_config_op( + &mut fixture, + ConfigOperation::SetModel( + ProviderId::ANTHROPIC, + ModelId::new("claude-3-5-sonnet-20241022"), + ), + ); + + let actual_provider = fixture + .session + .as_ref() + .and_then(|s| s.provider_id.as_deref()); + let actual_model = fixture.session.as_ref().and_then(|s| s.model_id.as_deref()); + + assert_eq!(actual_provider, Some("anthropic")); + assert_eq!(actual_model, Some("claude-3-5-sonnet-20241022")); } } From 8120fc0ca49668ee2e3b8a402220c18e44f446ea Mon Sep 17 00:00:00 2001 From: "autofix-ci[bot]" <114827586+autofix-ci[bot]@users.noreply.github.com> Date: Thu, 2 Apr 2026 02:14:56 +0000 Subject: [PATCH 17/23] [autofix.ci] apply automated fixes --- crates/forge_app/src/hooks/user_hook_handler.rs | 16 ++++++---------- crates/forge_main/src/info.rs | 5 ++++- 2 files changed, 10 insertions(+), 11 deletions(-) diff --git a/crates/forge_app/src/hooks/user_hook_handler.rs b/crates/forge_app/src/hooks/user_hook_handler.rs index 92ff6a8c0c..1b91462cd4 100644 --- a/crates/forge_app/src/hooks/user_hook_handler.rs +++ b/crates/forge_app/src/hooks/user_hook_handler.rs @@ -1,3 +1,8 @@ +use std::collections::HashMap; +use std::path::PathBuf; +use std::sync::atomic::{AtomicBool, Ordering}; +use std::time::Duration; + use async_trait::async_trait; use forge_domain::{ ContextMessage, Conversation, EndPayload, EventData, EventHandle, HookEventInput, @@ -6,10 +11,6 @@ use forge_domain::{ UserHookEventName, UserHookMatcherGroup, }; use regex::Regex; -use std::collections::HashMap; -use std::path::PathBuf; -use std::sync::atomic::{AtomicBool, Ordering}; -use std::time::Duration; use tracing::{debug, warn}; use super::user_hook_executor::UserHookExecutor; @@ -46,12 +47,7 @@ impl UserHookHandler { /// * `session_id` - Current session/conversation ID. /// * `default_hook_timeout` - Default timeout in milliseconds for hook /// commands. - pub fn new( - service: I, - config: UserHookConfig, - cwd: PathBuf, - session_id: String, - ) -> Self { + pub fn new(service: I, config: UserHookConfig, cwd: PathBuf, session_id: String) -> Self { let mut env_vars = HashMap::new(); env_vars.insert( "FORGE_PROJECT_DIR".to_string(), diff --git a/crates/forge_main/src/info.rs b/crates/forge_main/src/info.rs index 609c3daef5..45779fbced 100644 --- a/crates/forge_main/src/info.rs +++ b/crates/forge_main/src/info.rs @@ -387,7 +387,10 @@ impl From<&ForgeConfig> for Info { .add_key_value("ForgeCode Service URL", config.services_url.to_string()) .add_title("TOOL CONFIGURATION") .add_key_value("Tool Timeout", format!("{}s", config.tool_timeout_secs)) - .add_key_value("Hook Timeout", format!("{}ms", config.hook_timeout_ms.unwrap_or(0))) + .add_key_value( + "Hook Timeout", + format!("{}ms", config.hook_timeout_ms.unwrap_or(0)), + ) .add_key_value( "Max Image Size", format!("{} bytes", config.max_image_size_bytes), From 4b747f79d5fd42f2b2a705dd2e3b43f2e3c19b2c Mon Sep 17 00:00:00 2001 From: Sandipsinh Dilipsinh Rathod <62684960+ssddOnTop@users.noreply.github.com> Date: Wed, 1 Apr 2026 22:22:59 -0400 Subject: [PATCH 18/23] feat(hooks): pass env vars from services into UserHookHandler --- crates/forge_app/src/app.rs | 1 + .../forge_app/src/hooks/user_hook_handler.rs | 26 +++++++++++-------- 2 files changed, 16 insertions(+), 11 deletions(-) diff --git a/crates/forge_app/src/app.rs b/crates/forge_app/src/app.rs index 162729c2e7..1e1f3f7e36 100644 --- a/crates/forge_app/src/app.rs +++ b/crates/forge_app/src/app.rs @@ -165,6 +165,7 @@ impl ForgeApp { let hook = if !user_hook_config.is_empty() { let user_handler = UserHookHandler::new( services.hook_command_service().clone(), + services.get_env_vars(), user_hook_config, environment.cwd.clone(), conversation.id.to_string(), diff --git a/crates/forge_app/src/hooks/user_hook_handler.rs b/crates/forge_app/src/hooks/user_hook_handler.rs index 1b91462cd4..4968fa0457 100644 --- a/crates/forge_app/src/hooks/user_hook_handler.rs +++ b/crates/forge_app/src/hooks/user_hook_handler.rs @@ -1,8 +1,5 @@ -use std::collections::HashMap; -use std::path::PathBuf; -use std::sync::atomic::{AtomicBool, Ordering}; -use std::time::Duration; - +use super::user_hook_executor::UserHookExecutor; +use crate::services::HookCommandService; use async_trait::async_trait; use forge_domain::{ ContextMessage, Conversation, EndPayload, EventData, EventHandle, HookEventInput, @@ -11,11 +8,12 @@ use forge_domain::{ UserHookEventName, UserHookMatcherGroup, }; use regex::Regex; +use std::collections::{BTreeMap, HashMap}; +use std::path::PathBuf; +use std::sync::atomic::{AtomicBool, Ordering}; +use std::time::Duration; use tracing::{debug, warn}; -use super::user_hook_executor::UserHookExecutor; -use crate::services::HookCommandService; - /// Default timeout for hook commands (10 minutes). const DEFAULT_HOOK_TIMEOUT: Duration = Duration::from_secs(600); @@ -47,8 +45,13 @@ impl UserHookHandler { /// * `session_id` - Current session/conversation ID. /// * `default_hook_timeout` - Default timeout in milliseconds for hook /// commands. - pub fn new(service: I, config: UserHookConfig, cwd: PathBuf, session_id: String) -> Self { - let mut env_vars = HashMap::new(); + pub fn new( + service: I, + mut env_vars: BTreeMap, + config: UserHookConfig, + cwd: PathBuf, + session_id: String, + ) -> Self { env_vars.insert( "FORGE_PROJECT_DIR".to_string(), cwd.to_string_lossy().to_string(), @@ -60,7 +63,7 @@ impl UserHookHandler { executor: UserHookExecutor::new(service), config, cwd, - env_vars, + env_vars: env_vars.into_iter().collect(), stop_hook_active: std::sync::Arc::new(AtomicBool::new(false)), } } @@ -625,6 +628,7 @@ mod tests { fn null_handler(config: UserHookConfig) -> UserHookHandler { UserHookHandler::new( NullInfra, + BTreeMap::new(), config, PathBuf::from("/tmp"), "sess-1".to_string(), From 50ecb0afc3cfb8775600f7fb65b71ba10bf2c53a Mon Sep 17 00:00:00 2001 From: "autofix-ci[bot]" <114827586+autofix-ci[bot]@users.noreply.github.com> Date: Thu, 2 Apr 2026 02:26:37 +0000 Subject: [PATCH 19/23] [autofix.ci] apply automated fixes --- crates/forge_app/src/hooks/user_hook_handler.rs | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/crates/forge_app/src/hooks/user_hook_handler.rs b/crates/forge_app/src/hooks/user_hook_handler.rs index 4968fa0457..c247e3b6fa 100644 --- a/crates/forge_app/src/hooks/user_hook_handler.rs +++ b/crates/forge_app/src/hooks/user_hook_handler.rs @@ -1,5 +1,8 @@ -use super::user_hook_executor::UserHookExecutor; -use crate::services::HookCommandService; +use std::collections::{BTreeMap, HashMap}; +use std::path::PathBuf; +use std::sync::atomic::{AtomicBool, Ordering}; +use std::time::Duration; + use async_trait::async_trait; use forge_domain::{ ContextMessage, Conversation, EndPayload, EventData, EventHandle, HookEventInput, @@ -8,12 +11,11 @@ use forge_domain::{ UserHookEventName, UserHookMatcherGroup, }; use regex::Regex; -use std::collections::{BTreeMap, HashMap}; -use std::path::PathBuf; -use std::sync::atomic::{AtomicBool, Ordering}; -use std::time::Duration; use tracing::{debug, warn}; +use super::user_hook_executor::UserHookExecutor; +use crate::services::HookCommandService; + /// Default timeout for hook commands (10 minutes). const DEFAULT_HOOK_TIMEOUT: Duration = Duration::from_secs(600); From 0daca786c852eaac53a2809bc9fc52097cacc457 Mon Sep 17 00:00:00 2001 From: Sandipsinh Dilipsinh Rathod <62684960+ssddOnTop@users.noreply.github.com> Date: Wed, 1 Apr 2026 22:45:06 -0400 Subject: [PATCH 20/23] refactor(hooks): propagate load errors and use FileInfoInfra for existence checks --- crates/forge_services/src/user_hook_config.rs | 165 +++++++++++++----- 1 file changed, 117 insertions(+), 48 deletions(-) diff --git a/crates/forge_services/src/user_hook_config.rs b/crates/forge_services/src/user_hook_config.rs index f85ec73781..1f340561c1 100644 --- a/crates/forge_services/src/user_hook_config.rs +++ b/crates/forge_services/src/user_hook_config.rs @@ -1,9 +1,8 @@ use std::path::Path; use std::sync::Arc; -use forge_app::{EnvironmentInfra, FileReaderInfra}; +use forge_app::{EnvironmentInfra, FileInfoInfra, FileReaderInfra}; use forge_domain::{UserHookConfig, UserSettings}; -use tracing::{debug, warn}; /// Loads and merges user hook configurations from the three settings file /// locations using infrastructure abstractions. @@ -21,72 +20,74 @@ impl ForgeUserHookConfigService { } } -impl ForgeUserHookConfigService { +impl ForgeUserHookConfigService { /// Loads a single settings file and extracts hook configuration. /// - /// Returns `None` if the file doesn't exist or is invalid. - async fn load_file(&self, path: &Path) -> Option { - let contents = match self.0.read_utf8(path).await { - Ok(c) => c, - Err(_) => return None, - }; + /// Returns `Ok(None)` if the file does not exist or cannot be read. + /// Returns `Err` if the file exists but fails to deserialize, including the file path in the + /// error message. + async fn load_file(&self, path: &Path) -> anyhow::Result> { + if !self.0.exists(path).await? { + return Ok(None); + } + let contents = self + .0 + .read_utf8(path) + .await + .map_err(|e| anyhow::anyhow!("Failed to read '{}': {}", path.display(), e))?; match serde_json::from_str::(&contents) { Ok(settings) => { if settings.hooks.is_empty() { - None + Ok(None) } else { - Some(settings.hooks) + Ok(Some(settings.hooks)) } } - Err(e) => { - warn!( - path = %path.display(), - error = %e, - "Failed to parse settings file for hooks" - ); - None - } + Err(e) => Err(anyhow::anyhow!( + "Failed to deserialize '{}': {}", + path.display(), + e + )), } } } #[async_trait::async_trait] -impl forge_app::UserHookConfigService +impl forge_app::UserHookConfigService for ForgeUserHookConfigService { async fn get_user_hook_config(&self) -> anyhow::Result { let env = self.0.get_environment(); - let mut config = UserHookConfig::new(); - // 1. User-level: ~/.forge/settings.json + // Collect all candidate paths in resolution order + let mut paths: Vec = Vec::new(); if let Some(home) = &env.home { - let user_settings_path = home.join("forge").join("settings.json"); - if let Some(user_config) = self.load_file(&user_settings_path).await { - debug!(path = %user_settings_path.display(), "Loaded user-level hook config"); - config.merge(user_config); - } + paths.push(home.join("forge").join("settings.json")); } + paths.push(env.cwd.join(".forge").join("settings.json")); + paths.push(env.cwd.join(".forge").join("settings.local.json")); - // 2. Project-level: .forge/settings.json - let project_settings_path = env.cwd.join(".forge").join("settings.json"); - if let Some(project_config) = self.load_file(&project_settings_path).await { - debug!(path = %project_settings_path.display(), "Loaded project-level hook config"); - config.merge(project_config); - } + // Load every file, keeping the (path, result) pairs + let results = + futures::future::join_all(paths.iter().map(|path| self.load_file(path))).await; + + // Collect the error message from every file that failed + let errors: Vec = results + .iter() + .filter_map(|r| r.as_ref().err().map(|e| e.to_string())) + .collect(); - // 3. Project-local: .forge/settings.local.json - let local_settings_path = env.cwd.join(".forge").join("settings.local.json"); - if let Some(local_config) = self.load_file(&local_settings_path).await { - debug!(path = %local_settings_path.display(), "Loaded project-local hook config"); - config.merge(local_config); + if !errors.is_empty() { + return Err(anyhow::anyhow!("{}", errors.join("\n\n"))); } - if !config.is_empty() { - debug!( - event_count = config.events.len(), - "Merged user hook configuration" - ); + // Merge every successfully loaded config + let mut config = UserHookConfig::new(); + for result in results { + if let Ok(Some(file_config)) = result { + config.merge(file_config); + } } Ok(config) @@ -117,11 +118,11 @@ mod tests { } }"#, ) - .unwrap(); + .unwrap(); let service = fixture(None, PathBuf::from("/nonexistent")); - let actual = service.load_file(&settings_path).await; + let actual = service.load_file(&settings_path).await.unwrap(); assert!(actual.is_some()); let config = actual.unwrap(); assert_eq!( @@ -140,10 +141,56 @@ mod tests { let service = fixture(None, PathBuf::from("/nonexistent")); - let actual = service.load_file(&settings_path).await; + let actual = service.load_file(&settings_path).await.unwrap(); assert!(actual.is_none()); } + #[tokio::test] + async fn test_load_file_invalid_json_returns_error_with_path() { + let dir = tempfile::tempdir().unwrap(); + let settings_path = dir.path().join("settings.json"); + std::fs::write(&settings_path, r#"{ invalid json }"#).unwrap(); + + let service = fixture(None, PathBuf::from("/nonexistent")); + + let actual = service.load_file(&settings_path).await; + assert!(actual.is_err()); + let err = actual.unwrap_err().to_string(); + assert!( + err.contains(&settings_path.display().to_string()), + "Error message should contain the file path, got: {err}" + ); + } + + #[tokio::test] + async fn test_get_user_hook_config_reports_all_invalid_files() { + let project_dir = tempfile::tempdir().unwrap(); + let project_forge_dir = project_dir.path().join(".forge"); + std::fs::create_dir_all(&project_forge_dir).unwrap(); + + // Both project files have invalid JSON + std::fs::write(project_forge_dir.join("settings.json"), r#"{ bad }"#).unwrap(); + std::fs::write( + project_forge_dir.join("settings.local.json"), + r#"{ also bad }"#, + ) + .unwrap(); + + let service = fixture(None, project_dir.path().to_path_buf()); + + let actual = service.get_user_hook_config().await; + assert!(actual.is_err()); + let err = actual.unwrap_err().to_string(); + assert!( + err.contains("settings.json"), + "Error should mention settings.json, got: {err}" + ); + assert!( + err.contains("settings.local.json"), + "Error should mention settings.local.json, got: {err}" + ); + } + #[tokio::test] async fn test_get_user_hook_config_nonexistent_paths() { let service = fixture( @@ -171,7 +218,7 @@ mod tests { } }"#, ) - .unwrap(); + .unwrap(); // Set up a project directory let project_dir = tempfile::tempdir().unwrap(); @@ -187,7 +234,7 @@ mod tests { } }"#, ) - .unwrap(); + .unwrap(); std::fs::write( project_forge_dir.join("settings.local.json"), r#"{ @@ -234,6 +281,28 @@ mod tests { cwd: PathBuf, } + #[async_trait::async_trait] + impl FileInfoInfra for TestInfra { + async fn is_binary(&self, _path: &Path) -> anyhow::Result { + Ok(false) + } + + async fn is_file(&self, path: &Path) -> anyhow::Result { + Ok(tokio::fs::metadata(path) + .await + .map(|m| m.is_file()) + .unwrap_or(false)) + } + + async fn exists(&self, path: &Path) -> anyhow::Result { + Ok(tokio::fs::try_exists(path).await.unwrap_or(false)) + } + + async fn file_size(&self, path: &Path) -> anyhow::Result { + Ok(tokio::fs::metadata(path).await?.len()) + } + } + #[async_trait::async_trait] impl FileReaderInfra for TestInfra { async fn read_utf8(&self, path: &Path) -> anyhow::Result { From 9358557fedab0a9eec2a55c845d54d2ed364d4e9 Mon Sep 17 00:00:00 2001 From: "autofix-ci[bot]" <114827586+autofix-ci[bot]@users.noreply.github.com> Date: Thu, 2 Apr 2026 02:46:52 +0000 Subject: [PATCH 21/23] [autofix.ci] apply automated fixes --- crates/forge_services/src/user_hook_config.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/forge_services/src/user_hook_config.rs b/crates/forge_services/src/user_hook_config.rs index 1f340561c1..cee18c6439 100644 --- a/crates/forge_services/src/user_hook_config.rs +++ b/crates/forge_services/src/user_hook_config.rs @@ -24,8 +24,8 @@ impl ForgeUserHookConfigS /// Loads a single settings file and extracts hook configuration. /// /// Returns `Ok(None)` if the file does not exist or cannot be read. - /// Returns `Err` if the file exists but fails to deserialize, including the file path in the - /// error message. + /// Returns `Err` if the file exists but fails to deserialize, including the + /// file path in the error message. async fn load_file(&self, path: &Path) -> anyhow::Result> { if !self.0.exists(path).await? { return Ok(None); From bfc1bb7176206517be11ca1f76a5e3d99d849e77 Mon Sep 17 00:00:00 2001 From: Sandipsinh Dilipsinh Rathod <62684960+ssddOnTop@users.noreply.github.com> Date: Wed, 1 Apr 2026 22:56:35 -0400 Subject: [PATCH 22/23] refactor(hooks): move timeout enforcement from infra to executor layer --- .../forge_app/src/hooks/user_hook_executor.rs | 44 ++++++++++++++----- .../forge_app/src/hooks/user_hook_handler.rs | 2 - crates/forge_app/src/infra.rs | 11 ++--- crates/forge_app/src/services.rs | 9 ++-- crates/forge_infra/src/executor.rs | 34 +++----------- crates/forge_infra/src/forge_infra.rs | 4 +- crates/forge_repo/src/forge_repo.rs | 4 +- .../src/tool_services/hook_command.rs | 8 ++-- .../forge_services/src/tool_services/shell.rs | 1 - 9 files changed, 51 insertions(+), 66 deletions(-) diff --git a/crates/forge_app/src/hooks/user_hook_executor.rs b/crates/forge_app/src/hooks/user_hook_executor.rs index 40047d90aa..cc267e77c5 100644 --- a/crates/forge_app/src/hooks/user_hook_executor.rs +++ b/crates/forge_app/src/hooks/user_hook_executor.rs @@ -2,7 +2,7 @@ use std::collections::HashMap; use std::path::PathBuf; use std::time::Duration; -use forge_domain::HookExecutionResult; +use forge_domain::{CommandOutput, HookExecutionResult}; use tracing::debug; use crate::services::HookCommandService; @@ -26,12 +26,14 @@ impl UserHookExecutor { /// Executes a shell command, piping `input_json` to stdin and capturing /// stdout/stderr. /// + /// Applies `timeout_duration` by racing the service call against the + /// deadline. On timeout, returns a `HookExecutionResult` with + /// `exit_code: None` and a descriptive message in `stderr`. + /// /// # Arguments /// * `command` - The shell command string to execute. /// * `input_json` - JSON string to pipe to the command's stdin. - /// * `timeout` - per-hook timeout in milliseconds. Falls back to - /// environment configuration. Uses the built-in default (10 min) when - /// zero. + /// * `timeout_duration` - Maximum time to wait for the command. /// * `cwd` - Working directory for the command. /// * `env_vars` - Additional environment variables to set. /// @@ -52,16 +54,37 @@ impl UserHookExecutor { "Executing user hook command" ); - let output = self - .0 - .execute_command_with_input( + let result = tokio::time::timeout( + timeout_duration, + self.0.execute_command_with_input( command.to_string(), cwd.clone(), input_json.to_string(), - timeout_duration, env_vars.clone(), - ) - .await?; + ), + ) + .await; + + let output = match result { + Ok(Ok(output)) => output, + Ok(Err(e)) => return Err(e), + Err(_) => { + tracing::warn!( + command = command, + timeout_ms = timeout_duration.as_millis() as u64, + "Hook command timed out" + ); + CommandOutput { + command: command.to_string(), + exit_code: None, + stdout: String::new(), + stderr: format!( + "Hook command timed out after {}ms", + timeout_duration.as_millis() + ), + } + } + }; debug!( command = command, @@ -138,7 +161,6 @@ mod tests { command: String, _working_dir: PathBuf, _stdin_input: String, - _timeout: Duration, _env_vars: HashMap, ) -> anyhow::Result { let mut out = self.result.clone(); diff --git a/crates/forge_app/src/hooks/user_hook_handler.rs b/crates/forge_app/src/hooks/user_hook_handler.rs index c247e3b6fa..b537baf9f2 100644 --- a/crates/forge_app/src/hooks/user_hook_handler.rs +++ b/crates/forge_app/src/hooks/user_hook_handler.rs @@ -594,7 +594,6 @@ impl EventHandle> for UserHookHandl mod tests { use std::collections::HashMap; use std::path::PathBuf; - use std::time::Duration; use forge_domain::{ CommandOutput, HookExecutionResult, UserHookEntry, UserHookEventName, UserHookMatcherGroup, @@ -615,7 +614,6 @@ mod tests { command: String, _working_dir: PathBuf, _stdin_input: String, - _timeout: Duration, _env_vars: HashMap, ) -> anyhow::Result { Ok(CommandOutput { diff --git a/crates/forge_app/src/infra.rs b/crates/forge_app/src/infra.rs index d074788abc..93bf60090e 100644 --- a/crates/forge_app/src/infra.rs +++ b/crates/forge_app/src/infra.rs @@ -1,8 +1,6 @@ use std::collections::{BTreeMap, HashMap}; use std::hash::Hash; use std::path::{Path, PathBuf}; -use std::time::Duration; - use anyhow::Result; use bytes::Bytes; use forge_config::ForgeConfig; @@ -162,18 +160,16 @@ pub trait CommandInfra: Send + Sync { env_vars: Option>, ) -> anyhow::Result; - /// Executes a shell command with stdin input and a timeout. + /// Executes a shell command with stdin input. /// /// Pipes `stdin_input` to the process stdin, captures stdout and stderr, - /// and waits up to `timeout` for the process to complete. On timeout, - /// returns `CommandOutput` with `exit_code: None` and a timeout message in - /// `stderr`. + /// and waits for the process to complete. Timeout enforcement is handled + /// by the caller. /// /// # Arguments /// * `command` - Shell command string to execute. /// * `working_dir` - Working directory for the command. /// * `stdin_input` - Data to pipe to the process stdin. - /// * `timeout` - Maximum duration to wait for the command. /// * `env_vars` - Additional environment variables as key-value pairs. /// /// # Errors @@ -183,7 +179,6 @@ pub trait CommandInfra: Send + Sync { command: String, working_dir: PathBuf, stdin_input: String, - timeout: Duration, env_vars: HashMap, ) -> anyhow::Result; } diff --git a/crates/forge_app/src/services.rs b/crates/forge_app/src/services.rs index b90311269e..63eb1f690e 100644 --- a/crates/forge_app/src/services.rs +++ b/crates/forge_app/src/services.rs @@ -561,17 +561,15 @@ pub trait ProviderAuthService: Send + Sync { /// depends on a service rather than infrastructure directly. #[async_trait::async_trait] pub trait HookCommandService: Send + Sync { - /// Executes a shell command with stdin input and a timeout. + /// Executes a shell command with stdin input. /// - /// Pipes `stdin_input` to the process stdin and waits up to `timeout`. - /// Returns `CommandOutput` with `exit_code: None` and a timeout message in - /// `stderr` when the timeout expires. + /// Pipes `stdin_input` to the process stdin and captures stdout/stderr. + /// Timeout enforcement is handled by the caller. /// /// # Arguments /// * `command` - Shell command string to execute. /// * `working_dir` - Working directory for the command. /// * `stdin_input` - Data to pipe to the process stdin. - /// * `timeout` - Maximum duration to wait for the command. /// * `env_vars` - Additional environment variables as key-value pairs. /// /// # Errors @@ -581,7 +579,6 @@ pub trait HookCommandService: Send + Sync { command: String, working_dir: PathBuf, stdin_input: String, - timeout: Duration, env_vars: HashMap, ) -> anyhow::Result; } diff --git a/crates/forge_infra/src/executor.rs b/crates/forge_infra/src/executor.rs index bd18f276c0..a795556abc 100644 --- a/crates/forge_infra/src/executor.rs +++ b/crates/forge_infra/src/executor.rs @@ -2,7 +2,6 @@ use std::collections::HashMap; use std::io::{self, Write}; use std::path::{Path, PathBuf}; use std::sync::Arc; -use std::time::Duration; use forge_app::CommandInfra; use forge_domain::{CommandOutput, ConsoleWriter as OutputPrinterTrait, Environment}; @@ -232,7 +231,6 @@ impl CommandInfra for ForgeCommandExecutorService { command: String, working_dir: PathBuf, stdin_input: String, - timeout: Duration, env_vars: HashMap, ) -> anyhow::Result { let mut prepared_command = self.prepare_command(&command, &working_dir, None); @@ -256,31 +254,13 @@ impl CommandInfra for ForgeCommandExecutorService { }); } - // Wait for the command with timeout - let result = tokio::time::timeout(timeout, child.wait_with_output()).await; - - match result { - Ok(Ok(output)) => Ok(CommandOutput { - command, - exit_code: output.status.code(), - stdout: String::from_utf8_lossy(&output.stdout).into_owned(), - stderr: String::from_utf8_lossy(&output.stderr).into_owned(), - }), - Ok(Err(e)) => Err(e.into()), - Err(_) => { - tracing::warn!( - command = command, - timeout_ms = timeout.as_millis() as u64, - "Hook command timed out" - ); - Ok(CommandOutput { - command, - exit_code: None, - stdout: String::new(), - stderr: format!("Hook command timed out after {}ms", timeout.as_millis()), - }) - } - } + let output = child.wait_with_output().await?; + Ok(CommandOutput { + command, + exit_code: output.status.code(), + stdout: String::from_utf8_lossy(&output.stdout).into_owned(), + stderr: String::from_utf8_lossy(&output.stderr).into_owned(), + }) } } diff --git a/crates/forge_infra/src/forge_infra.rs b/crates/forge_infra/src/forge_infra.rs index dda28f8402..5b7857109b 100644 --- a/crates/forge_infra/src/forge_infra.rs +++ b/crates/forge_infra/src/forge_infra.rs @@ -2,7 +2,6 @@ use std::collections::{BTreeMap, HashMap}; use std::path::{Path, PathBuf}; use std::process::ExitStatus; use std::sync::Arc; -use std::time::Duration; use bytes::Bytes; use forge_app::{ @@ -234,11 +233,10 @@ impl CommandInfra for ForgeInfra { command: String, working_dir: PathBuf, stdin_input: String, - timeout: Duration, env_vars: HashMap, ) -> anyhow::Result { self.command_executor_service - .execute_command_with_input(command, working_dir, stdin_input, timeout, env_vars) + .execute_command_with_input(command, working_dir, stdin_input, env_vars) .await } } diff --git a/crates/forge_repo/src/forge_repo.rs b/crates/forge_repo/src/forge_repo.rs index c5a7c5c599..4289b74266 100644 --- a/crates/forge_repo/src/forge_repo.rs +++ b/crates/forge_repo/src/forge_repo.rs @@ -1,7 +1,6 @@ use std::collections::{BTreeMap, HashMap}; use std::path::{Path, PathBuf}; use std::sync::Arc; -use std::time::Duration; use bytes::Bytes; use forge_app::{ @@ -466,11 +465,10 @@ where command: String, working_dir: PathBuf, stdin_input: String, - timeout: Duration, env_vars: HashMap, ) -> anyhow::Result { self.infra - .execute_command_with_input(command, working_dir, stdin_input, timeout, env_vars) + .execute_command_with_input(command, working_dir, stdin_input, env_vars) .await } } diff --git a/crates/forge_services/src/tool_services/hook_command.rs b/crates/forge_services/src/tool_services/hook_command.rs index b4a2f84526..3588bc0fa3 100644 --- a/crates/forge_services/src/tool_services/hook_command.rs +++ b/crates/forge_services/src/tool_services/hook_command.rs @@ -1,7 +1,6 @@ use std::collections::HashMap; use std::path::PathBuf; use std::sync::Arc; -use std::time::Duration; use forge_app::{CommandInfra, HookCommandService}; use forge_domain::CommandOutput; @@ -10,8 +9,8 @@ use forge_domain::CommandOutput; /// satisfies the [`HookCommandService`] contract. /// /// By delegating to the underlying infra this service avoids duplicating -/// process-spawning, stdin-piping, and timeout logic; those concerns live -/// entirely inside the `CommandInfra` implementation. +/// process-spawning and stdin-piping logic; those concerns live entirely inside +/// the `CommandInfra` implementation. #[derive(Clone)] pub struct ForgeHookCommandService(Arc); @@ -29,11 +28,10 @@ impl HookCommandService for ForgeHookCommandService { command: String, working_dir: PathBuf, stdin_input: String, - timeout: Duration, env_vars: HashMap, ) -> anyhow::Result { self.0 - .execute_command_with_input(command, working_dir, stdin_input, timeout, env_vars) + .execute_command_with_input(command, working_dir, stdin_input, env_vars) .await } } diff --git a/crates/forge_services/src/tool_services/shell.rs b/crates/forge_services/src/tool_services/shell.rs index 242e555cb1..1988779f77 100644 --- a/crates/forge_services/src/tool_services/shell.rs +++ b/crates/forge_services/src/tool_services/shell.rs @@ -114,7 +114,6 @@ mod tests { command: String, _working_dir: PathBuf, _stdin_input: String, - _timeout: std::time::Duration, _env_vars: std::collections::HashMap, ) -> anyhow::Result { Ok(forge_domain::CommandOutput { From f9eb27840d130ee846d8b3dc9057e168a3f6b308 Mon Sep 17 00:00:00 2001 From: "autofix-ci[bot]" <114827586+autofix-ci[bot]@users.noreply.github.com> Date: Thu, 2 Apr 2026 02:59:14 +0000 Subject: [PATCH 23/23] [autofix.ci] apply automated fixes --- crates/forge_app/src/infra.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/crates/forge_app/src/infra.rs b/crates/forge_app/src/infra.rs index 93bf60090e..fa71f7b479 100644 --- a/crates/forge_app/src/infra.rs +++ b/crates/forge_app/src/infra.rs @@ -1,6 +1,7 @@ use std::collections::{BTreeMap, HashMap}; use std::hash::Hash; use std::path::{Path, PathBuf}; + use anyhow::Result; use bytes::Bytes; use forge_config::ForgeConfig;