From bc3c5be59e90152df125401e18282e61aec994f3 Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Sun, 15 Mar 2026 12:07:41 -0400 Subject: [PATCH] Fix --dry-run executing command/script nodes instead of simulating them Command nodes were running for real during dry-run mode because the dry_run flag only affected LLM-backed handlers. Propagate dry_run through EngineServices so CommandHandler can skip execution and return a simulated success. Co-Authored-By: Claude Opus 4.6 (1M context) --- lib/crates/fabro-workflows/src/cli/run.rs | 8 +++- lib/crates/fabro-workflows/src/engine.rs | 8 ++++ .../fabro-workflows/src/handler/agent.rs | 4 ++ .../fabro-workflows/src/handler/command.rs | 41 +++++++++++++++++++ .../src/handler/conditional.rs | 1 + .../fabro-workflows/src/handler/exit.rs | 1 + .../fabro-workflows/src/handler/fan_in.rs | 1 + .../fabro-workflows/src/handler/human.rs | 1 + .../src/handler/manager_loop.rs | 8 +++- lib/crates/fabro-workflows/src/handler/mod.rs | 2 + .../fabro-workflows/src/handler/parallel.rs | 3 ++ .../fabro-workflows/src/handler/prompt.rs | 1 + .../fabro-workflows/src/handler/start.rs | 1 + .../fabro-workflows/src/handler/wait.rs | 1 + 14 files changed, 79 insertions(+), 2 deletions(-) diff --git a/lib/crates/fabro-workflows/src/cli/run.rs b/lib/crates/fabro-workflows/src/cli/run.rs index 4a3b2b071..d084ee5ca 100644 --- a/lib/crates/fabro-workflows/src/cli/run.rs +++ b/lib/crates/fabro-workflows/src/cli/run.rs @@ -1133,6 +1133,9 @@ pub async fn run_command( if !sandbox_env.is_empty() { engine.set_env(sandbox_env); } + if dry_run_mode { + engine.set_dry_run(true); + } // Wire up hook runner from run config or run defaults { @@ -1769,12 +1772,15 @@ async fn run_from_branch( Some(Box::new(BackendRouter::new(Box::new(api), cli))) } }); - let engine = crate::engine::WorkflowRunEngine::with_interviewer( + let mut engine = crate::engine::WorkflowRunEngine::with_interviewer( registry, Arc::clone(&emitter), interviewer, Arc::clone(&sandbox), ); + if dry_run_mode { + engine.set_dry_run(true); + } let meta_branch = Some(crate::git::MetadataStore::branch_name(&run_id)); let config = RunConfig { diff --git a/lib/crates/fabro-workflows/src/engine.rs b/lib/crates/fabro-workflows/src/engine.rs index e758782d0..1b4e7e7b8 100644 --- a/lib/crates/fabro-workflows/src/engine.rs +++ b/lib/crates/fabro-workflows/src/engine.rs @@ -832,6 +832,7 @@ impl WorkflowRunEngine { git_state: std::sync::RwLock::new(None), hook_runner: None, env: HashMap::new(), + dry_run: false, }, interviewer: None, } @@ -848,6 +849,7 @@ impl WorkflowRunEngine { git_state: std::sync::RwLock::new(None), hook_runner: services.hook_runner.clone(), env: services.env.clone(), + dry_run: services.dry_run, }, interviewer: None, } @@ -869,6 +871,7 @@ impl WorkflowRunEngine { git_state: std::sync::RwLock::new(None), hook_runner: None, env: HashMap::new(), + dry_run: false, }, interviewer: Some(interviewer), } @@ -884,6 +887,11 @@ impl WorkflowRunEngine { self.services.env = env; } + /// Enable dry-run mode so handlers skip real execution. + pub fn set_dry_run(&mut self, dry_run: bool) { + self.services.dry_run = dry_run; + } + /// Run lifecycle hooks and return the merged decision. /// Returns `Proceed` if no hook runner is configured. async fn run_hooks(&self, hook_context: &HookContext, work_dir: Option<&Path>) -> HookDecision { diff --git a/lib/crates/fabro-workflows/src/handler/agent.rs b/lib/crates/fabro-workflows/src/handler/agent.rs index bc5bcbcee..e394fd195 100644 --- a/lib/crates/fabro-workflows/src/handler/agent.rs +++ b/lib/crates/fabro-workflows/src/handler/agent.rs @@ -360,6 +360,7 @@ mod tests { git_state: std::sync::RwLock::new(None), hook_runner: None, env: HashMap::new(), + dry_run: false, } } @@ -497,6 +498,7 @@ mod tests { git_state: std::sync::RwLock::new(None), hook_runner: None, env: HashMap::new(), + dry_run: false, }; let outcome = handler @@ -561,6 +563,7 @@ mod tests { git_state: std::sync::RwLock::new(None), hook_runner: None, env: HashMap::new(), + dry_run: false, }; let outcome = handler @@ -626,6 +629,7 @@ mod tests { git_state: std::sync::RwLock::new(None), hook_runner: None, env: HashMap::new(), + dry_run: false, }; let outcome = handler diff --git a/lib/crates/fabro-workflows/src/handler/command.rs b/lib/crates/fabro-workflows/src/handler/command.rs index 9b0b06887..43bf14410 100644 --- a/lib/crates/fabro-workflows/src/handler/command.rs +++ b/lib/crates/fabro-workflows/src/handler/command.rs @@ -46,6 +46,18 @@ impl Handler for CommandHandler { return Ok(Outcome::fail_classify("No script specified")); } + if services.dry_run { + let mut outcome = Outcome::success(); + outcome.notes = Some(format!("[Simulated] Command skipped: {script}")); + outcome + .context_updates + .insert(keys::COMMAND_OUTPUT.to_string(), serde_json::json!("")); + outcome + .context_updates + .insert(keys::COMMAND_STDERR.to_string(), serde_json::json!("")); + return Ok(outcome); + } + let language = node .attrs .get("language") @@ -168,6 +180,7 @@ mod tests { git_state: std::sync::RwLock::new(None), hook_runner: None, env: std::collections::HashMap::new(), + dry_run: false, } } @@ -187,6 +200,33 @@ mod tests { assert_eq!(outcome.failure_reason(), Some("No script specified")); } + #[tokio::test] + async fn dry_run_skips_execution() { + let handler = CommandHandler; + let mut node = Node::new("script_node"); + node.attrs.insert( + "script".to_string(), + AttrValue::String("echo hello".to_string()), + ); + let context = Context::new(); + let graph = Graph::new("test"); + let run_dir = tempfile::tempdir().unwrap(); + + let mut services = make_services(); + services.dry_run = true; + + let outcome = handler + .execute(&node, &context, &graph, run_dir.path(), &services) + .await + .unwrap(); + assert_eq!(outcome.status, StageStatus::Success); + assert!(outcome.notes.as_deref().unwrap().contains("[Simulated]")); + assert!(outcome.notes.as_deref().unwrap().contains("echo hello")); + // No stdout/stderr logs should be written + let stage_dir = run_dir.path().join("nodes").join("script_node"); + assert!(!stage_dir.join("stdout.log").exists()); + } + #[tokio::test] async fn script_handler_echo_command() { let handler = CommandHandler; @@ -678,6 +718,7 @@ mod tests { git_state: std::sync::RwLock::new(None), hook_runner: None, env: std::collections::HashMap::new(), + dry_run: false, } } diff --git a/lib/crates/fabro-workflows/src/handler/conditional.rs b/lib/crates/fabro-workflows/src/handler/conditional.rs index 1732b4ce2..beaa4c0a6 100644 --- a/lib/crates/fabro-workflows/src/handler/conditional.rs +++ b/lib/crates/fabro-workflows/src/handler/conditional.rs @@ -46,6 +46,7 @@ mod tests { git_state: std::sync::RwLock::new(None), hook_runner: None, env: std::collections::HashMap::new(), + dry_run: false, } } diff --git a/lib/crates/fabro-workflows/src/handler/exit.rs b/lib/crates/fabro-workflows/src/handler/exit.rs index e7f65e182..3911e3dbd 100644 --- a/lib/crates/fabro-workflows/src/handler/exit.rs +++ b/lib/crates/fabro-workflows/src/handler/exit.rs @@ -43,6 +43,7 @@ mod tests { git_state: std::sync::RwLock::new(None), hook_runner: None, env: std::collections::HashMap::new(), + dry_run: false, } } diff --git a/lib/crates/fabro-workflows/src/handler/fan_in.rs b/lib/crates/fabro-workflows/src/handler/fan_in.rs index 25b58ad51..07718376c 100644 --- a/lib/crates/fabro-workflows/src/handler/fan_in.rs +++ b/lib/crates/fabro-workflows/src/handler/fan_in.rs @@ -299,6 +299,7 @@ mod tests { git_state: std::sync::RwLock::new(None), hook_runner: None, env: std::collections::HashMap::new(), + dry_run: false, } } diff --git a/lib/crates/fabro-workflows/src/handler/human.rs b/lib/crates/fabro-workflows/src/handler/human.rs index a93e0951d..14ef1206d 100644 --- a/lib/crates/fabro-workflows/src/handler/human.rs +++ b/lib/crates/fabro-workflows/src/handler/human.rs @@ -301,6 +301,7 @@ mod tests { git_state: std::sync::RwLock::new(None), hook_runner: None, env: std::collections::HashMap::new(), + dry_run: false, } } diff --git a/lib/crates/fabro-workflows/src/handler/manager_loop.rs b/lib/crates/fabro-workflows/src/handler/manager_loop.rs index aef3542ee..b1dbc1d5d 100644 --- a/lib/crates/fabro-workflows/src/handler/manager_loop.rs +++ b/lib/crates/fabro-workflows/src/handler/manager_loop.rs @@ -141,7 +141,7 @@ impl Handler for SubWorkflowHandler { let child_config = RunConfig { run_dir: child_logs, cancel_token: Some(cancel_token), - dry_run: false, + dry_run: services.dry_run, run_id: format!("{parent_run_id}_child_{}", node.id), git_checkpoint_enabled: false, host_repo_path: None, @@ -274,6 +274,7 @@ mod tests { git_state: std::sync::RwLock::new(None), hook_runner: None, env: HashMap::new(), + dry_run: false, } } @@ -407,6 +408,7 @@ mod tests { git_state: std::sync::RwLock::new(None), hook_runner: None, env: HashMap::new(), + dry_run: false, }; let handler = SubWorkflowHandler; @@ -516,6 +518,7 @@ mod tests { git_state: std::sync::RwLock::new(None), hook_runner: None, env: HashMap::new(), + dry_run: false, }; let handler = SubWorkflowHandler; @@ -577,6 +580,7 @@ mod tests { git_state: std::sync::RwLock::new(None), hook_runner: None, env: HashMap::new(), + dry_run: false, }; let handler = SubWorkflowHandler; @@ -744,6 +748,7 @@ mod tests { git_state: std::sync::RwLock::new(None), hook_runner: None, env: HashMap::new(), + dry_run: false, }; let handler = SubWorkflowHandler; @@ -828,6 +833,7 @@ mod tests { git_state: std::sync::RwLock::new(None), hook_runner: None, env: HashMap::new(), + dry_run: false, }; let handler = SubWorkflowHandler; diff --git a/lib/crates/fabro-workflows/src/handler/mod.rs b/lib/crates/fabro-workflows/src/handler/mod.rs index c0d62c8fb..541de95ce 100644 --- a/lib/crates/fabro-workflows/src/handler/mod.rs +++ b/lib/crates/fabro-workflows/src/handler/mod.rs @@ -38,6 +38,8 @@ pub struct EngineServices { pub hook_runner: Option>, /// Environment variables from `[sandbox.env]` config, injected into command nodes. pub env: HashMap, + /// When true, handlers should skip real execution and return simulated results. + pub dry_run: bool, } impl EngineServices { diff --git a/lib/crates/fabro-workflows/src/handler/parallel.rs b/lib/crates/fabro-workflows/src/handler/parallel.rs index 8ecc3cc1e..7c36c1421 100644 --- a/lib/crates/fabro-workflows/src/handler/parallel.rs +++ b/lib/crates/fabro-workflows/src/handler/parallel.rs @@ -384,6 +384,7 @@ impl Handler for ParallelHandler { let emitter = Arc::clone(&services.emitter); let hook_runner = services.hook_runner.clone(); let env = services.env.clone(); + let dry_run = services.dry_run; let graph = graph.clone(); let run_dir = run_dir.to_path_buf(); let sem = Arc::clone(&semaphore); @@ -432,6 +433,7 @@ impl Handler for ParallelHandler { git_state: std::sync::RwLock::new(None), hook_runner: hook_runner.clone(), env: env.clone(), + dry_run, }; let handler = registry.resolve(target_node); let outcome = handler @@ -742,6 +744,7 @@ mod tests { git_state: std::sync::RwLock::new(None), hook_runner: None, env: std::collections::HashMap::new(), + dry_run: false, } } diff --git a/lib/crates/fabro-workflows/src/handler/prompt.rs b/lib/crates/fabro-workflows/src/handler/prompt.rs index fa8b721f1..978109151 100644 --- a/lib/crates/fabro-workflows/src/handler/prompt.rs +++ b/lib/crates/fabro-workflows/src/handler/prompt.rs @@ -164,6 +164,7 @@ mod tests { git_state: std::sync::RwLock::new(None), hook_runner: None, env: std::collections::HashMap::new(), + dry_run: false, } } diff --git a/lib/crates/fabro-workflows/src/handler/start.rs b/lib/crates/fabro-workflows/src/handler/start.rs index 690cfde6a..e722ba4d0 100644 --- a/lib/crates/fabro-workflows/src/handler/start.rs +++ b/lib/crates/fabro-workflows/src/handler/start.rs @@ -42,6 +42,7 @@ mod tests { git_state: std::sync::RwLock::new(None), hook_runner: None, env: std::collections::HashMap::new(), + dry_run: false, } } diff --git a/lib/crates/fabro-workflows/src/handler/wait.rs b/lib/crates/fabro-workflows/src/handler/wait.rs index 10b73ae05..41b984937 100644 --- a/lib/crates/fabro-workflows/src/handler/wait.rs +++ b/lib/crates/fabro-workflows/src/handler/wait.rs @@ -57,6 +57,7 @@ mod tests { git_state: std::sync::RwLock::new(None), hook_runner: None, env: std::collections::HashMap::new(), + dry_run: false, } }