From d25fc56b10dca35920fab55adae843ec69c392ff Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Sun, 15 Mar 2026 12:28:54 -0400 Subject: [PATCH] Fix dry-run bug in API server and deduplicate test helpers The API server set RunConfig.dry_run but never called engine.set_dry_run(), so command/script nodes executed for real during API-served dry runs. Add the missing call. Also extract EngineServices::test_default() to replace 11 identical make_services() bodies and 8 inline struct constructions across handler test modules (-254/+57 lines). Co-Authored-By: Claude Opus 4.6 (1M context) --- lib/crates/fabro-api/src/server.rs | 3 + lib/crates/fabro-workflows/src/cli/run.rs | 1 - .../fabro-workflows/src/handler/agent.rs | 59 +++----------- .../fabro-workflows/src/handler/command.rs | 27 +------ .../src/handler/conditional.rs | 16 +--- .../fabro-workflows/src/handler/exit.rs | 16 +--- .../fabro-workflows/src/handler/fan_in.rs | 15 +--- .../fabro-workflows/src/handler/human.rs | 15 +--- .../src/handler/manager_loop.rs | 80 +++---------------- lib/crates/fabro-workflows/src/handler/mod.rs | 16 ++++ .../fabro-workflows/src/handler/parallel.rs | 16 +--- .../fabro-workflows/src/handler/prompt.rs | 15 +--- .../fabro-workflows/src/handler/start.rs | 15 +--- .../fabro-workflows/src/handler/wait.rs | 17 +--- 14 files changed, 57 insertions(+), 254 deletions(-) diff --git a/lib/crates/fabro-api/src/server.rs b/lib/crates/fabro-api/src/server.rs index dad9d2d6d..59debde38 100644 --- a/lib/crates/fabro-api/src/server.rs +++ b/lib/crates/fabro-api/src/server.rs @@ -581,6 +581,9 @@ async fn execute_run(state: Arc, run_id: String) { Arc::clone(&interviewer) as Arc, sandbox, ); + if state.dry_run { + engine.set_dry_run(true); + } // Wire up hook runner from server config if !state.hooks.is_empty() { diff --git a/lib/crates/fabro-workflows/src/cli/run.rs b/lib/crates/fabro-workflows/src/cli/run.rs index a0e0f305f..86ad2fabe 100644 --- a/lib/crates/fabro-workflows/src/cli/run.rs +++ b/lib/crates/fabro-workflows/src/cli/run.rs @@ -1136,7 +1136,6 @@ pub async fn run_command( if dry_run_mode { engine.set_dry_run(true); } - // Wire up hook runner from run config or run defaults { let hooks = run_cfg diff --git a/lib/crates/fabro-workflows/src/handler/agent.rs b/lib/crates/fabro-workflows/src/handler/agent.rs index e394fd195..cd49ced97 100644 --- a/lib/crates/fabro-workflows/src/handler/agent.rs +++ b/lib/crates/fabro-workflows/src/handler/agent.rs @@ -346,22 +346,10 @@ mod tests { use super::*; use crate::event::EventEmitter; use crate::graph::AttrValue; - use crate::handler::start::StartHandler; - use crate::handler::HandlerRegistry; use tempfile::TempDir; fn make_services() -> EngineServices { - EngineServices { - registry: std::sync::Arc::new(HandlerRegistry::new(Box::new(StartHandler))), - emitter: std::sync::Arc::new(EventEmitter::new()), - sandbox: std::sync::Arc::new(fabro_agent::LocalSandbox::new( - std::env::current_dir().unwrap_or_else(|_| std::path::PathBuf::from(".")), - )), - git_state: std::sync::RwLock::new(None), - hook_runner: None, - env: HashMap::new(), - dry_run: false, - } + EngineServices::test_default() } #[tokio::test] @@ -489,17 +477,10 @@ mod tests { let graph = Graph::new("test"); let tmp = TempDir::new().unwrap(); - let services = EngineServices { - registry: std::sync::Arc::new(HandlerRegistry::new(Box::new(StartHandler))), - emitter: std::sync::Arc::new(EventEmitter::new()), - sandbox: std::sync::Arc::new(fabro_agent::LocalSandbox::new( - sandbox_dir.path().to_path_buf(), - )), - git_state: std::sync::RwLock::new(None), - hook_runner: None, - env: HashMap::new(), - dry_run: false, - }; + let mut services = EngineServices::test_default(); + services.sandbox = std::sync::Arc::new(fabro_agent::LocalSandbox::new( + sandbox_dir.path().to_path_buf(), + )); let outcome = handler .execute(&node, &context, &graph, tmp.path(), &services) @@ -554,17 +535,10 @@ mod tests { let graph = Graph::new("test"); let tmp = TempDir::new().unwrap(); - let services = EngineServices { - registry: std::sync::Arc::new(HandlerRegistry::new(Box::new(StartHandler))), - emitter: std::sync::Arc::new(EventEmitter::new()), - sandbox: std::sync::Arc::new(fabro_agent::LocalSandbox::new( - sandbox_dir.path().to_path_buf(), - )), - git_state: std::sync::RwLock::new(None), - hook_runner: None, - env: HashMap::new(), - dry_run: false, - }; + let mut services = EngineServices::test_default(); + services.sandbox = std::sync::Arc::new(fabro_agent::LocalSandbox::new( + sandbox_dir.path().to_path_buf(), + )); let outcome = handler .execute(&node, &context, &graph, tmp.path(), &services) @@ -620,17 +594,10 @@ mod tests { let graph = Graph::new("test"); let tmp = TempDir::new().unwrap(); - let services = EngineServices { - registry: std::sync::Arc::new(HandlerRegistry::new(Box::new(StartHandler))), - emitter: std::sync::Arc::new(EventEmitter::new()), - sandbox: std::sync::Arc::new(fabro_agent::LocalSandbox::new( - sandbox_dir.path().to_path_buf(), - )), - git_state: std::sync::RwLock::new(None), - hook_runner: None, - env: HashMap::new(), - dry_run: false, - }; + let mut services = EngineServices::test_default(); + services.sandbox = std::sync::Arc::new(fabro_agent::LocalSandbox::new( + sandbox_dir.path().to_path_buf(), + )); let outcome = handler .execute(&node, &context, &graph, tmp.path(), &services) diff --git a/lib/crates/fabro-workflows/src/handler/command.rs b/lib/crates/fabro-workflows/src/handler/command.rs index 43bf14410..fc524ee3d 100644 --- a/lib/crates/fabro-workflows/src/handler/command.rs +++ b/lib/crates/fabro-workflows/src/handler/command.rs @@ -163,25 +163,12 @@ impl Handler for CommandHandler { #[cfg(test)] mod tests { use super::*; - use crate::event::EventEmitter; use crate::graph::AttrValue; - use crate::handler::start::StartHandler; - use crate::handler::HandlerRegistry; use crate::outcome::StageStatus; use std::time::Duration; fn make_services() -> EngineServices { - EngineServices { - registry: std::sync::Arc::new(HandlerRegistry::new(Box::new(StartHandler))), - emitter: std::sync::Arc::new(EventEmitter::new()), - sandbox: std::sync::Arc::new(fabro_agent::LocalSandbox::new( - std::env::current_dir().unwrap_or_else(|_| std::path::PathBuf::from(".")), - )), - git_state: std::sync::RwLock::new(None), - hook_runner: None, - env: std::collections::HashMap::new(), - dry_run: false, - } + EngineServices::test_default() } #[tokio::test] @@ -711,15 +698,9 @@ mod tests { } fn make_spy_services(sandbox: std::sync::Arc) -> EngineServices { - EngineServices { - registry: std::sync::Arc::new(HandlerRegistry::new(Box::new(StartHandler))), - emitter: std::sync::Arc::new(EventEmitter::new()), - sandbox, - git_state: std::sync::RwLock::new(None), - hook_runner: None, - env: std::collections::HashMap::new(), - dry_run: false, - } + let mut services = EngineServices::test_default(); + services.sandbox = sandbox; + services } #[tokio::test] diff --git a/lib/crates/fabro-workflows/src/handler/conditional.rs b/lib/crates/fabro-workflows/src/handler/conditional.rs index beaa4c0a6..90ecd9318 100644 --- a/lib/crates/fabro-workflows/src/handler/conditional.rs +++ b/lib/crates/fabro-workflows/src/handler/conditional.rs @@ -32,22 +32,8 @@ impl Handler for ConditionalHandler { #[cfg(test)] mod tests { use super::*; - use crate::event::EventEmitter; - use crate::handler::start::StartHandler; - use crate::handler::HandlerRegistry; - fn make_services() -> EngineServices { - EngineServices { - registry: std::sync::Arc::new(HandlerRegistry::new(Box::new(StartHandler))), - emitter: std::sync::Arc::new(EventEmitter::new()), - sandbox: std::sync::Arc::new(fabro_agent::LocalSandbox::new( - std::env::current_dir().unwrap_or_else(|_| std::path::PathBuf::from(".")), - )), - git_state: std::sync::RwLock::new(None), - hook_runner: None, - env: std::collections::HashMap::new(), - dry_run: false, - } + EngineServices::test_default() } #[tokio::test] diff --git a/lib/crates/fabro-workflows/src/handler/exit.rs b/lib/crates/fabro-workflows/src/handler/exit.rs index 3911e3dbd..39a3d35b5 100644 --- a/lib/crates/fabro-workflows/src/handler/exit.rs +++ b/lib/crates/fabro-workflows/src/handler/exit.rs @@ -29,22 +29,8 @@ impl Handler for ExitHandler { #[cfg(test)] mod tests { use super::*; - use crate::event::EventEmitter; - use crate::handler::start::StartHandler; - use crate::handler::HandlerRegistry; - fn make_services() -> EngineServices { - EngineServices { - registry: std::sync::Arc::new(HandlerRegistry::new(Box::new(StartHandler))), - emitter: std::sync::Arc::new(EventEmitter::new()), - sandbox: std::sync::Arc::new(fabro_agent::LocalSandbox::new( - std::env::current_dir().unwrap_or_else(|_| std::path::PathBuf::from(".")), - )), - git_state: std::sync::RwLock::new(None), - hook_runner: None, - env: std::collections::HashMap::new(), - dry_run: false, - } + EngineServices::test_default() } #[tokio::test] diff --git a/lib/crates/fabro-workflows/src/handler/fan_in.rs b/lib/crates/fabro-workflows/src/handler/fan_in.rs index 07718376c..6c8d96532 100644 --- a/lib/crates/fabro-workflows/src/handler/fan_in.rs +++ b/lib/crates/fabro-workflows/src/handler/fan_in.rs @@ -284,23 +284,10 @@ async fn llm_evaluate( #[cfg(test)] mod tests { use super::*; - use crate::event::EventEmitter; - use crate::handler::start::StartHandler; - use crate::handler::HandlerRegistry; use crate::outcome::StageStatus; fn make_services() -> EngineServices { - EngineServices { - registry: std::sync::Arc::new(HandlerRegistry::new(Box::new(StartHandler))), - emitter: std::sync::Arc::new(EventEmitter::new()), - sandbox: std::sync::Arc::new(fabro_agent::LocalSandbox::new( - std::env::current_dir().unwrap_or_else(|_| std::path::PathBuf::from(".")), - )), - git_state: std::sync::RwLock::new(None), - hook_runner: None, - env: std::collections::HashMap::new(), - dry_run: false, - } + EngineServices::test_default() } #[tokio::test] diff --git a/lib/crates/fabro-workflows/src/handler/human.rs b/lib/crates/fabro-workflows/src/handler/human.rs index 14ef1206d..5ed79b720 100644 --- a/lib/crates/fabro-workflows/src/handler/human.rs +++ b/lib/crates/fabro-workflows/src/handler/human.rs @@ -284,25 +284,12 @@ fn answer_text(answer: &Answer) -> String { #[cfg(test)] mod tests { use super::*; - use crate::event::EventEmitter; use crate::graph::{AttrValue, Edge}; - use crate::handler::start::StartHandler; - use crate::handler::HandlerRegistry; use crate::interviewer::auto_approve::AutoApproveInterviewer; use crate::interviewer::recording::RecordingInterviewer; fn make_services() -> EngineServices { - EngineServices { - registry: std::sync::Arc::new(HandlerRegistry::new(Box::new(StartHandler))), - emitter: std::sync::Arc::new(EventEmitter::new()), - sandbox: std::sync::Arc::new(fabro_agent::LocalSandbox::new( - std::env::current_dir().unwrap_or_else(|_| std::path::PathBuf::from(".")), - )), - git_state: std::sync::RwLock::new(None), - hook_runner: None, - env: std::collections::HashMap::new(), - dry_run: false, - } + EngineServices::test_default() } fn build_graph_with_human_gate() -> Graph { diff --git a/lib/crates/fabro-workflows/src/handler/manager_loop.rs b/lib/crates/fabro-workflows/src/handler/manager_loop.rs index b1dbc1d5d..a58106475 100644 --- a/lib/crates/fabro-workflows/src/handler/manager_loop.rs +++ b/lib/crates/fabro-workflows/src/handler/manager_loop.rs @@ -255,27 +255,18 @@ impl Handler for SubWorkflowHandler { #[cfg(test)] mod tests { use super::*; - use crate::event::EventEmitter; use crate::graph::AttrValue; use crate::handler::exit::ExitHandler; use crate::handler::start::StartHandler; use crate::handler::HandlerRegistry; fn make_services() -> EngineServices { + let mut services = EngineServices::test_default(); let mut registry = HandlerRegistry::new(Box::new(StartHandler)); registry.register("start", Box::new(StartHandler)); registry.register("exit", Box::new(ExitHandler)); - EngineServices { - registry: std::sync::Arc::new(registry), - emitter: std::sync::Arc::new(EventEmitter::new()), - sandbox: std::sync::Arc::new(fabro_agent::LocalSandbox::new( - std::env::current_dir().unwrap_or_else(|_| std::path::PathBuf::from(".")), - )), - git_state: std::sync::RwLock::new(None), - hook_runner: None, - env: HashMap::new(), - dry_run: false, - } + services.registry = std::sync::Arc::new(registry); + services } fn child_dot_succeeds() -> &'static str { @@ -399,17 +390,8 @@ mod tests { let mut registry = HandlerRegistry::new(Box::new(ContextEchoHandler)); registry.register("start", Box::new(StartHandler)); registry.register("exit", Box::new(ExitHandler)); - let services = EngineServices { - registry: std::sync::Arc::new(registry), - emitter: std::sync::Arc::new(EventEmitter::new()), - sandbox: std::sync::Arc::new(fabro_agent::LocalSandbox::new( - std::env::current_dir().unwrap_or_else(|_| std::path::PathBuf::from(".")), - )), - git_state: std::sync::RwLock::new(None), - hook_runner: None, - env: HashMap::new(), - dry_run: false, - }; + let mut services = EngineServices::test_default(); + services.registry = std::sync::Arc::new(registry); let handler = SubWorkflowHandler; let mut node = Node::new("manager"); @@ -509,17 +491,8 @@ mod tests { let mut registry = HandlerRegistry::new(Box::new(SlowHandler)); registry.register("start", Box::new(StartHandler)); registry.register("exit", Box::new(ExitHandler)); - let services = EngineServices { - registry: std::sync::Arc::new(registry), - emitter: std::sync::Arc::new(EventEmitter::new()), - sandbox: std::sync::Arc::new(fabro_agent::LocalSandbox::new( - std::env::current_dir().unwrap_or_else(|_| std::path::PathBuf::from(".")), - )), - git_state: std::sync::RwLock::new(None), - hook_runner: None, - env: HashMap::new(), - dry_run: false, - }; + let mut services = EngineServices::test_default(); + services.registry = std::sync::Arc::new(registry); let handler = SubWorkflowHandler; let mut node = Node::new("manager"); @@ -571,17 +544,8 @@ mod tests { let mut registry = HandlerRegistry::new(Box::new(SlowHandler)); registry.register("start", Box::new(StartHandler)); registry.register("exit", Box::new(ExitHandler)); - let services = EngineServices { - registry: std::sync::Arc::new(registry), - emitter: std::sync::Arc::new(EventEmitter::new()), - sandbox: std::sync::Arc::new(fabro_agent::LocalSandbox::new( - std::env::current_dir().unwrap_or_else(|_| std::path::PathBuf::from(".")), - )), - git_state: std::sync::RwLock::new(None), - hook_runner: None, - env: HashMap::new(), - dry_run: false, - }; + let mut services = EngineServices::test_default(); + services.registry = std::sync::Arc::new(registry); let handler = SubWorkflowHandler; let mut node = Node::new("manager"); @@ -739,17 +703,8 @@ mod tests { let mut registry = HandlerRegistry::new(Box::new(ContextEchoHandler)); registry.register("start", Box::new(StartHandler)); registry.register("exit", Box::new(ExitHandler)); - let services = EngineServices { - registry: std::sync::Arc::new(registry), - emitter: std::sync::Arc::new(EventEmitter::new()), - sandbox: std::sync::Arc::new(fabro_agent::LocalSandbox::new( - std::env::current_dir().unwrap_or_else(|_| std::path::PathBuf::from(".")), - )), - git_state: std::sync::RwLock::new(None), - hook_runner: None, - env: HashMap::new(), - dry_run: false, - }; + let mut services = EngineServices::test_default(); + services.registry = std::sync::Arc::new(registry); let handler = SubWorkflowHandler; let mut node = Node::new("manager"); @@ -824,17 +779,8 @@ mod tests { let mut registry = HandlerRegistry::new(Box::new(PreambleEchoHandler)); registry.register("start", Box::new(StartHandler)); registry.register("exit", Box::new(ExitHandler)); - let services = EngineServices { - registry: std::sync::Arc::new(registry), - emitter: std::sync::Arc::new(EventEmitter::new()), - sandbox: std::sync::Arc::new(fabro_agent::LocalSandbox::new( - std::env::current_dir().unwrap_or_else(|_| std::path::PathBuf::from(".")), - )), - git_state: std::sync::RwLock::new(None), - hook_runner: None, - env: HashMap::new(), - dry_run: false, - }; + let mut services = EngineServices::test_default(); + services.registry = std::sync::Arc::new(registry); let handler = SubWorkflowHandler; let mut node = Node::new("manager"); diff --git a/lib/crates/fabro-workflows/src/handler/mod.rs b/lib/crates/fabro-workflows/src/handler/mod.rs index 541de95ce..a7115066f 100644 --- a/lib/crates/fabro-workflows/src/handler/mod.rs +++ b/lib/crates/fabro-workflows/src/handler/mod.rs @@ -52,6 +52,22 @@ impl EngineServices { pub fn set_git_state(&self, state: Option>) { *self.git_state.write().unwrap() = state; } + + /// Test-only default: empty registry, no hooks, local sandbox at cwd. + #[cfg(test)] + pub fn test_default() -> Self { + Self { + registry: Arc::new(HandlerRegistry::new(Box::new(start::StartHandler))), + emitter: Arc::new(EventEmitter::new()), + sandbox: Arc::new(fabro_agent::LocalSandbox::new( + std::env::current_dir().unwrap_or_else(|_| std::path::PathBuf::from(".")), + )), + git_state: std::sync::RwLock::new(None), + hook_runner: None, + env: HashMap::new(), + dry_run: false, + } + } } /// The handler interface for node execution. diff --git a/lib/crates/fabro-workflows/src/handler/parallel.rs b/lib/crates/fabro-workflows/src/handler/parallel.rs index 7c36c1421..92eb08e2d 100644 --- a/lib/crates/fabro-workflows/src/handler/parallel.rs +++ b/lib/crates/fabro-workflows/src/handler/parallel.rs @@ -728,24 +728,10 @@ fn find_join_node(results: &[BranchResult], graph: &Graph) -> Option { #[cfg(test)] mod tests { use super::*; - use crate::event::EventEmitter; use crate::graph::{AttrValue, Edge}; - use crate::handler::start::StartHandler; - use crate::handler::HandlerRegistry; fn make_services() -> EngineServices { - let registry = HandlerRegistry::new(Box::new(StartHandler)); - EngineServices { - registry: Arc::new(registry), - emitter: Arc::new(EventEmitter::new()), - sandbox: Arc::new(fabro_agent::LocalSandbox::new( - std::env::current_dir().unwrap_or_else(|_| std::path::PathBuf::from(".")), - )), - git_state: std::sync::RwLock::new(None), - hook_runner: None, - env: std::collections::HashMap::new(), - dry_run: false, - } + EngineServices::test_default() } #[tokio::test] diff --git a/lib/crates/fabro-workflows/src/handler/prompt.rs b/lib/crates/fabro-workflows/src/handler/prompt.rs index 978109151..a0e5b7e95 100644 --- a/lib/crates/fabro-workflows/src/handler/prompt.rs +++ b/lib/crates/fabro-workflows/src/handler/prompt.rs @@ -147,25 +147,12 @@ impl Handler for PromptHandler { #[cfg(test)] mod tests { use super::*; - use crate::event::EventEmitter; use crate::graph::AttrValue; - use crate::handler::start::StartHandler; - use crate::handler::HandlerRegistry; use std::sync::Arc; use tempfile::TempDir; fn make_services() -> EngineServices { - EngineServices { - registry: Arc::new(HandlerRegistry::new(Box::new(StartHandler))), - emitter: Arc::new(EventEmitter::new()), - sandbox: Arc::new(fabro_agent::LocalSandbox::new( - std::env::current_dir().unwrap_or_else(|_| std::path::PathBuf::from(".")), - )), - git_state: std::sync::RwLock::new(None), - hook_runner: None, - env: std::collections::HashMap::new(), - dry_run: false, - } + EngineServices::test_default() } #[tokio::test] diff --git a/lib/crates/fabro-workflows/src/handler/start.rs b/lib/crates/fabro-workflows/src/handler/start.rs index e722ba4d0..2b7c44d78 100644 --- a/lib/crates/fabro-workflows/src/handler/start.rs +++ b/lib/crates/fabro-workflows/src/handler/start.rs @@ -29,21 +29,8 @@ impl Handler for StartHandler { #[cfg(test)] mod tests { use super::*; - use crate::event::EventEmitter; - use crate::handler::HandlerRegistry; - fn make_services() -> EngineServices { - EngineServices { - registry: std::sync::Arc::new(HandlerRegistry::new(Box::new(StartHandler))), - emitter: std::sync::Arc::new(EventEmitter::new()), - sandbox: std::sync::Arc::new(fabro_agent::LocalSandbox::new( - std::env::current_dir().unwrap_or_else(|_| std::path::PathBuf::from(".")), - )), - git_state: std::sync::RwLock::new(None), - hook_runner: None, - env: std::collections::HashMap::new(), - dry_run: false, - } + EngineServices::test_default() } #[tokio::test] diff --git a/lib/crates/fabro-workflows/src/handler/wait.rs b/lib/crates/fabro-workflows/src/handler/wait.rs index 41b984937..5a45dfafb 100644 --- a/lib/crates/fabro-workflows/src/handler/wait.rs +++ b/lib/crates/fabro-workflows/src/handler/wait.rs @@ -42,23 +42,8 @@ mod tests { use std::time::Duration; use super::*; - use crate::event::EventEmitter; - use crate::handler::HandlerRegistry; - fn make_services() -> EngineServices { - EngineServices { - registry: std::sync::Arc::new(HandlerRegistry::new(Box::new( - crate::handler::start::StartHandler, - ))), - emitter: std::sync::Arc::new(EventEmitter::new()), - sandbox: std::sync::Arc::new(fabro_agent::LocalSandbox::new( - std::env::current_dir().unwrap_or_else(|_| std::path::PathBuf::from(".")), - )), - git_state: std::sync::RwLock::new(None), - hook_runner: None, - env: std::collections::HashMap::new(), - dry_run: false, - } + EngineServices::test_default() } #[tokio::test]