scriptoriumadmin/fabro
1
0
Fork 0
mirror of https://github.com/fabro-sh/fabro.git synced 2026-09-05 08:10:39 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions 10

Fix 6 bugs in create/start/attach lifecycle

Browse source 
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) <noreply@anthropic.com>
This commit is contained in:
Bryan Helmkamp 2026-03-20 12:35:59 -04:00
parent 1490024823
commit 9c22bb8834
5 changed files with 134 additions and 19 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

8
lib/crates/fabro-cli/src/commands/attach.rs
View file

@ -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::<fabro_interview::Question>(&request_data)
{

6
lib/crates/fabro-cli/src/commands/run.rs
View file

@ -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<dyn Interviewer> = 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),

113
lib/crates/fabro-cli/src/commands/run_progress.rs
View file

@ -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);
}

7
lib/crates/fabro-cli/src/commands/start.rs
View file

@ -23,6 +23,13 @@ pub fn start_run(run_dir: &Path) -> Result<u32> {
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()?;

19
lib/crates/fabro-cli/src/main.rs
View file

@ -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,