From 9c22bb88349583fde3f4bf609dfd23c190951704 Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Fri, 20 Mar 2026 12:35:59 -0400 Subject: [PATCH] Fix 6 bugs in create/start/attach lifecycle Bug 1: handle_json_line now reads post-rename field names matching real JSONL output (node_label, node_id, error instead of name, stage, message). Existing tests updated to use post-rename names too. Bug 2: _run_engine restores spec.working_directory via set_current_dir and uses cached graph.fabro instead of spec.workflow_path. Bug 3: attach loop deletes interview_request.json immediately after reading to prevent re-prompting. run_command uses FileInterviewer when stdin is not a TTY (detached mode). Bug 4: attach_run loads spec.verbose and passes it to ProgressUI instead of hardcoding false. Bug 5: start_run writes Starting status before spawning the engine process to prevent duplicate engines from concurrent start calls. Bug 6: handle_json_line dispatches DevcontainerResolved, DevcontainerLifecycleStarted/Completed/Failed events. Co-Authored-By: Claude Opus 4.6 (1M context) --- lib/crates/fabro-cli/src/commands/attach.rs | 8 +- lib/crates/fabro-cli/src/commands/run.rs | 6 +- .../fabro-cli/src/commands/run_progress.rs | 113 +++++++++++++++--- lib/crates/fabro-cli/src/commands/start.rs | 7 ++ lib/crates/fabro-cli/src/main.rs | 19 ++- 5 files changed, 134 insertions(+), 19 deletions(-) diff --git a/lib/crates/fabro-cli/src/commands/attach.rs b/lib/crates/fabro-cli/src/commands/attach.rs index 03f6610c3..f500b513f 100644 --- a/lib/crates/fabro-cli/src/commands/attach.rs +++ b/lib/crates/fabro-cli/src/commands/attach.rs @@ -26,7 +26,10 @@ pub async fn attach_run( let pid_path = run_dir.join("run.pid"); let is_tty = std::io::stderr().is_terminal(); - let mut progress_ui = run_progress::ProgressUI::new(is_tty, false); + let verbose = fabro_workflows::run_spec::RunSpec::load(run_dir) + .map(|spec| spec.verbose) + .unwrap_or(false); + let mut progress_ui = run_progress::ProgressUI::new(is_tty, verbose); // Install Ctrl+C handler let cancelled = Arc::new(AtomicBool::new(false)); @@ -93,6 +96,9 @@ pub async fn attach_run( // Check for interview request if interview_request_path.exists() { if let Ok(request_data) = std::fs::read_to_string(&interview_request_path) { + // Delete the request file immediately to prevent re-prompting + let _ = std::fs::remove_file(&interview_request_path); + if let Ok(question) = serde_json::from_str::(&request_data) { diff --git a/lib/crates/fabro-cli/src/commands/run.rs b/lib/crates/fabro-cli/src/commands/run.rs index 2bca60f66..adf4260b0 100644 --- a/lib/crates/fabro-cli/src/commands/run.rs +++ b/lib/crates/fabro-cli/src/commands/run.rs @@ -10,7 +10,7 @@ use clap::{Args, ValueEnum}; use fabro_agent::{DockerSandbox, DockerSandboxConfig, LocalSandbox, Sandbox}; use fabro_config::run::{RunDefaults, WorkflowRunConfig}; use fabro_config::{project as project_config, run as run_config, sandbox as sandbox_config}; -use fabro_interview::{AutoApproveInterviewer, ConsoleInterviewer, Interviewer}; +use fabro_interview::{AutoApproveInterviewer, ConsoleInterviewer, FileInterviewer, Interviewer}; use fabro_llm::provider::Provider; use fabro_util::terminal::Styles; use fabro_validate::Severity; @@ -729,6 +729,10 @@ pub async fn run_command( // 4. Build interviewer let interviewer: Arc = if args.auto_approve { Arc::new(AutoApproveInterviewer) + } else if !std::io::stdin().is_terminal() { + // Detached mode (stdin is /dev/null): use file-based IPC so the + // attach process can prompt the user on our behalf. + Arc::new(FileInterviewer::new(run_dir.clone())) } else { Arc::new(run_progress::ProgressAwareInterviewer::new( ConsoleInterviewer::new(styles), diff --git a/lib/crates/fabro-cli/src/commands/run_progress.rs b/lib/crates/fabro-cli/src/commands/run_progress.rs index b78c44558..8e8214277 100644 --- a/lib/crates/fabro-cli/src/commands/run_progress.rs +++ b/lib/crates/fabro-cli/src/commands/run_progress.rs @@ -682,13 +682,13 @@ impl ProgressUI { } "StageStarted" => { let node_id = str_field("node_id").unwrap_or("?"); - let name = str_field("name").unwrap_or("?"); + let name = str_field("node_label").unwrap_or("?"); let script = str_field("script"); self.on_stage_started(node_id, name, script); } "StageCompleted" => { let node_id = str_field("node_id").unwrap_or("?"); - let name = str_field("name").unwrap_or("?"); + let name = str_field("node_label").unwrap_or("?"); let duration_ms = u64_field("duration_ms"); let status = str_field("status").unwrap_or("success"); let succeeded = matches!(status, "success" | "partial_success"); @@ -742,8 +742,10 @@ impl ProgressUI { } "StageFailed" => { let node_id = str_field("node_id").unwrap_or("?"); - let name = str_field("name").unwrap_or("?"); - let message = str_field("message").unwrap_or("unknown error"); + let name = str_field("node_label").unwrap_or("?"); + let message = str_field("error") + .or_else(|| str_field("failure_reason")) + .unwrap_or("unknown error"); self.finish_stage(node_id, name, red_cross(), ""); let red = Style::new().red(); let summary = last_line_truncated(message, 120); @@ -758,12 +760,12 @@ impl ProgressUI { .or_else(|| Some(String::new())); } "ParallelBranchStarted" => { - if let Some(branch) = str_field("branch") { + if let Some(branch) = str_field("node_id") { self.on_parallel_branch_started(branch); } } "ParallelBranchCompleted" => { - if let Some(branch) = str_field("branch") { + if let Some(branch) = str_field("node_id") { let duration_ms = u64_field("duration_ms"); let status = str_field("status").unwrap_or("success"); self.on_parallel_branch_completed(branch, duration_ms, status); @@ -773,7 +775,7 @@ impl ProgressUI { self.parallel_parent = None; } "Agent.ToolCallStarted" => { - let stage = str_field("stage").unwrap_or("?"); + let stage = str_field("node_id").unwrap_or("?"); let tool_name = str_field("tool_name").unwrap_or("?"); let tool_call_id = str_field("tool_call_id").unwrap_or("?"); let empty = serde_json::Value::Object(serde_json::Map::new()); @@ -785,7 +787,7 @@ impl ProgressUI { self.on_tool_call_started(stage, tool_name, tool_call_id, arguments); } "Agent.ToolCallCompleted" => { - let stage = str_field("stage").unwrap_or("?"); + let stage = str_field("node_id").unwrap_or("?"); let tool_call_id = str_field("tool_call_id").unwrap_or("?"); let is_error = envelope .get("is_error") @@ -794,7 +796,7 @@ impl ProgressUI { self.on_tool_call_completed(stage, tool_call_id, is_error); } "Agent.AssistantMessage" => { - let stage = str_field("stage").unwrap_or("?"); + let stage = str_field("node_id").unwrap_or("?"); let model = str_field("model").unwrap_or("?"); // Update turn count if let Some(counts) = self.stage_counts.get_mut(stage) { @@ -815,7 +817,7 @@ impl ProgressUI { } } "Agent.CompactionStarted" => { - let stage = str_field("stage").unwrap_or("?"); + let stage = str_field("node_id").unwrap_or("?"); if let ProgressRenderer::Tty(tty) = &self.renderer { if let Some(active_stage) = self.active_stages.get_mut(stage) { if let Some(old) = active_stage.compaction_bar.take() { @@ -832,7 +834,7 @@ impl ProgressUI { } } "Agent.CompactionCompleted" => { - let stage = str_field("stage").unwrap_or("?"); + let stage = str_field("node_id").unwrap_or("?"); let original = u64_field("original_turn_count"); let preserved = u64_field("preserved_turn_count"); let tracked = u64_field("tracked_file_count"); @@ -873,6 +875,85 @@ impl ProgressUI { let dur = format_duration_ms(u64_field("duration_ms")); self.finish_stage("retro", "Retro", red_cross(), &dur); } + "DevcontainerResolved" => { + let dockerfile_lines = u64_field("dockerfile_lines"); + let environment_count = u64_field("environment_count"); + let lifecycle_command_count = u64_field("lifecycle_command_count"); + let workspace_folder = str_field("workspace_folder").unwrap_or("?").to_string(); + let detail = format!( + "{dockerfile_lines} Dockerfile lines, {environment_count} env vars, \ + {lifecycle_command_count} lifecycle cmds, {workspace_folder}" + ); + match &self.renderer { + ProgressRenderer::Tty(tty) => { + let bar = tty.multi.add(ProgressBar::new_spinner()); + bar.set_style(style_header_done()); + bar.finish_with_message("Devcontainer: resolved".to_string()); + let detail_bar = tty.multi.insert_after(&bar, ProgressBar::new_spinner()); + detail_bar.set_style(style_sandbox_detail()); + detail_bar.finish_with_message(detail); + } + ProgressRenderer::Plain => { + eprintln!(" Devcontainer: resolved"); + eprintln!(" {detail}"); + } + } + } + "DevcontainerLifecycleStarted" => { + let phase = str_field("phase").unwrap_or("?"); + let command_count = u64_field("command_count") as usize; + self.devcontainer_command_count = command_count; + match &self.renderer { + ProgressRenderer::Tty(tty) => { + let bar = tty.multi.add(ProgressBar::new_spinner()); + bar.set_style(style_header_running()); + bar.set_message(format!( + "Running devcontainer {phase} ({command_count} commands)..." + )); + bar.enable_steady_tick(Duration::from_millis(100)); + self.devcontainer_bar = Some(bar); + } + ProgressRenderer::Plain => { + eprintln!(" Running devcontainer {phase} ({command_count} commands)..."); + } + } + } + "DevcontainerLifecycleCompleted" => { + let phase = str_field("phase").unwrap_or("?"); + let duration_ms = u64_field("duration_ms"); + let dur = format_duration_ms(duration_ms); + match &self.renderer { + ProgressRenderer::Tty(_) => { + if let Some(bar) = self.devcontainer_bar.take() { + bar.set_style(style_header_done()); + bar.set_prefix(dur); + bar.finish_with_message(format!("Devcontainer: {phase}")); + } + } + ProgressRenderer::Plain => { + eprintln!(" Devcontainer: {phase} ({dur})"); + } + } + } + "DevcontainerLifecycleFailed" => { + let phase = str_field("phase").unwrap_or("?"); + let command = str_field("command").unwrap_or("?"); + let exit_code = u64_field("exit_code"); + let stderr_text = str_field("stderr").unwrap_or(""); + if let Some(bar) = self.devcontainer_bar.take() { + bar.abandon(); + } + let red = Style::new().red(); + let summary = if stderr_text.len() > 120 { + &stderr_text[..120] + } else { + stderr_text + }; + self.insert_info_line(&format!( + "{} Devcontainer {phase} command failed (exit {exit_code}): {command}\n {summary}", + red.apply_to("Error:") + )); + } "CliEnsureStarted" => { if let Some(cli_name) = str_field("cli_name") { self.on_cli_ensure_started(cli_name); @@ -1812,11 +1893,11 @@ mod tests { fn handle_json_line_stage_started_and_completed() { let mut ui = ProgressUI::new(false, false); - let started = r#"{"ts":"2026-01-01T12:00:00Z","event":"StageStarted","node_id":"plan","name":"Plan","index":0,"script":null,"attempt":1,"max_attempts":1}"#; + let started = r#"{"ts":"2026-01-01T12:00:00Z","event":"StageStarted","node_id":"plan","node_label":"Plan","stage_index":0,"script":null,"attempt":1,"max_attempts":1}"#; ui.handle_json_line(started); assert!(ui.stage_counts.contains_key("plan")); - let completed = r#"{"ts":"2026-01-01T12:00:10Z","event":"StageCompleted","node_id":"plan","name":"Plan","index":0,"duration_ms":10000,"status":"success"}"#; + let completed = r#"{"ts":"2026-01-01T12:00:10Z","event":"StageCompleted","node_id":"plan","node_label":"Plan","stage_index":0,"duration_ms":10000,"status":"success"}"#; ui.handle_json_line(completed); // In Plain mode, finish_stage just prints, so verify no panic } @@ -1826,14 +1907,14 @@ mod tests { let mut ui = ProgressUI::new(false, true); // verbose // Start a stage first - let started = r#"{"ts":"2026-01-01T12:00:00Z","event":"StageStarted","node_id":"code","name":"Code","index":0,"attempt":1,"max_attempts":1}"#; + let started = r#"{"ts":"2026-01-01T12:00:00Z","event":"StageStarted","node_id":"code","node_label":"Code","stage_index":0,"attempt":1,"max_attempts":1}"#; ui.handle_json_line(started); - let tc_start = r#"{"ts":"2026-01-01T12:00:01Z","event":"Agent.ToolCallStarted","stage":"code","tool_name":"read_file","tool_call_id":"tc1","arguments":{"path":"src/main.rs"}}"#; + let tc_start = r#"{"ts":"2026-01-01T12:00:01Z","event":"Agent.ToolCallStarted","node_id":"code","node_label":"code","tool_name":"read_file","tool_call_id":"tc1","arguments":{"path":"src/main.rs"}}"#; ui.handle_json_line(tc_start); assert_eq!(ui.stage_counts.get("code").map(|c| c.1), Some(1)); - let tc_done = r#"{"ts":"2026-01-01T12:00:02Z","event":"Agent.ToolCallCompleted","stage":"code","tool_name":"read_file","tool_call_id":"tc1","is_error":false}"#; + let tc_done = r#"{"ts":"2026-01-01T12:00:02Z","event":"Agent.ToolCallCompleted","node_id":"code","node_label":"code","tool_name":"read_file","tool_call_id":"tc1","is_error":false}"#; ui.handle_json_line(tc_done); } diff --git a/lib/crates/fabro-cli/src/commands/start.rs b/lib/crates/fabro-cli/src/commands/start.rs index e409b0be9..b5791df2e 100644 --- a/lib/crates/fabro-cli/src/commands/start.rs +++ b/lib/crates/fabro-cli/src/commands/start.rs @@ -23,6 +23,13 @@ pub fn start_run(run_dir: &Path) -> Result { fabro_workflows::run_spec::RunSpec::load(run_dir) .map_err(|e| anyhow::anyhow!("Cannot start run: failed to load spec.json: {e}"))?; + // Write Starting status before spawning to prevent duplicate engines + fabro_workflows::run_status::write_run_status( + run_dir, + fabro_workflows::run_status::RunStatus::Starting, + None, + ); + let log_file = std::fs::File::create(run_dir.join("detach.log"))?; let exe = std::env::current_exe()?; diff --git a/lib/crates/fabro-cli/src/main.rs b/lib/crates/fabro-cli/src/main.rs index 28c73b232..e654d3054 100644 --- a/lib/crates/fabro-cli/src/main.rs +++ b/lib/crates/fabro-cli/src/main.rs @@ -711,8 +711,25 @@ async fn main_inner() -> (String, Result<()>) { // Load spec and reconstruct RunArgs let spec = fabro_workflows::run_spec::RunSpec::load(&run_dir)?; + + // Restore the working directory captured at create time + std::env::set_current_dir(&spec.working_directory).map_err(|e| { + anyhow::anyhow!( + "Failed to set working directory to {}: {e}", + spec.working_directory.display() + ) + })?; + + // Use the cached graph snapshot instead of the original file + let cached_graph = run_dir.join("graph.fabro"); + let workflow_path = if cached_graph.exists() { + cached_graph + } else { + spec.workflow_path + }; + let run_args = commands::run::RunArgs { - workflow: Some(spec.workflow_path), + workflow: Some(workflow_path), run_dir: Some(run_dir), dry_run: spec.dry_run, preflight: false,