mirror of
https://github.com/fabro-sh/fabro.git
synced 2026-10-08 03:10:26 +00:00
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) <noreply@anthropic.com>
This commit is contained in:
parent
0c80d1f616
commit
bc3c5be59e
14 changed files with 79 additions and 2 deletions
|
|
@ -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 {
|
||||
|
|
|
|||
|
|
@ -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 {
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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,
|
||||
}
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -46,6 +46,7 @@ mod tests {
|
|||
git_state: std::sync::RwLock::new(None),
|
||||
hook_runner: None,
|
||||
env: std::collections::HashMap::new(),
|
||||
dry_run: false,
|
||||
}
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -43,6 +43,7 @@ mod tests {
|
|||
git_state: std::sync::RwLock::new(None),
|
||||
hook_runner: None,
|
||||
env: std::collections::HashMap::new(),
|
||||
dry_run: false,
|
||||
}
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -299,6 +299,7 @@ mod tests {
|
|||
git_state: std::sync::RwLock::new(None),
|
||||
hook_runner: None,
|
||||
env: std::collections::HashMap::new(),
|
||||
dry_run: false,
|
||||
}
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -301,6 +301,7 @@ mod tests {
|
|||
git_state: std::sync::RwLock::new(None),
|
||||
hook_runner: None,
|
||||
env: std::collections::HashMap::new(),
|
||||
dry_run: false,
|
||||
}
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -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;
|
||||
|
|
|
|||
|
|
@ -38,6 +38,8 @@ pub struct EngineServices {
|
|||
pub hook_runner: Option<Arc<HookRunner>>,
|
||||
/// Environment variables from `[sandbox.env]` config, injected into command nodes.
|
||||
pub env: HashMap<String, String>,
|
||||
/// When true, handlers should skip real execution and return simulated results.
|
||||
pub dry_run: bool,
|
||||
}
|
||||
|
||||
impl EngineServices {
|
||||
|
|
|
|||
|
|
@ -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,
|
||||
}
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -164,6 +164,7 @@ mod tests {
|
|||
git_state: std::sync::RwLock::new(None),
|
||||
hook_runner: None,
|
||||
env: std::collections::HashMap::new(),
|
||||
dry_run: false,
|
||||
}
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -42,6 +42,7 @@ mod tests {
|
|||
git_state: std::sync::RwLock::new(None),
|
||||
hook_runner: None,
|
||||
env: std::collections::HashMap::new(),
|
||||
dry_run: false,
|
||||
}
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -57,6 +57,7 @@ mod tests {
|
|||
git_state: std::sync::RwLock::new(None),
|
||||
hook_runner: None,
|
||||
env: std::collections::HashMap::new(),
|
||||
dry_run: false,
|
||||
}
|
||||
}
|
||||
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue