fabro(01KM4DKAWADPG0HY3PZCGQJ7H2): simplify_opus (success)

Fabro-Run: 01KM4DKAWADPG0HY3PZCGQJ7H2
Fabro-Completed: 6
Fabro-Checkpoint: 140b789412

⚒️ Generated with [Fabro](https://fabro.sh)

lib/crates/fabro-cli/src/commands/attach.rs

@@ -2,7 +2,7 @@ use std::io::{BufRead, BufReader, IsTerminal};
 use std::path::Path;
 use std::process::ExitCode;
 use std::sync::atomic::{AtomicBool, Ordering};
-use std::sync::{Arc, Mutex};
+use std::sync::Arc;
 
 use anyhow::{bail, Result};
 
@@ -26,7 +26,7 @@ pub async fn attach_run(
     let pid_path = run_dir.join("run.pid");
 
     let is_tty = std::io::stderr().is_terminal();
-    let progress_ui = Arc::new(Mutex::new(run_progress::ProgressUI::new(is_tty, false)));
+    let mut progress_ui = run_progress::ProgressUI::new(is_tty, false);
 
     // Install Ctrl+C handler
     let cancelled = Arc::new(AtomicBool::new(false));
@@ -57,6 +57,7 @@ pub async fn attach_run(
     let file = std::fs::File::open(&progress_path)?;
     let mut reader = BufReader::new(file);
     let mut line = String::new();
+    let mut cached_pid: Option<u32> = None;
 
     loop {
         if cancelled.load(Ordering::Relaxed) {
@@ -85,10 +86,7 @@ pub async fn attach_run(
             }
             let trimmed = line.trim();
             if !trimmed.is_empty() {
-                progress_ui
-                    .lock()
-                    .expect("progress lock poisoned")
-                    .handle_json_line(trimmed);
+                progress_ui.handle_json_line(trimmed);
             }
         }
 
@@ -99,10 +97,7 @@ pub async fn attach_run(
                     serde_json::from_str::<fabro_interview::Question>(&request_data)
                 {
                     // Hide progress bars during interview
-                    progress_ui
-                        .lock()
-                        .expect("progress lock poisoned")
-                        .hide_bars();
+                    progress_ui.hide_bars();
 
                     // Prompt user via ConsoleInterviewer
                     let interviewer = ConsoleInterviewer::new(styles);
@@ -114,10 +109,7 @@ pub async fn attach_run(
                     }
 
                     // Show progress bars again
-                    progress_ui
-                        .lock()
-                        .expect("progress lock poisoned")
-                        .show_bars();
+                    progress_ui.show_bars();
                 }
             }
         }
@@ -125,28 +117,36 @@ pub async fn attach_run(
         // Check if run is complete
         if conclusion_path.exists() {
             // Drain any remaining lines
-            drain_remaining(&mut reader, &mut line, &progress_ui);
+            drain_remaining(&mut reader, &mut line, &mut progress_ui);
             break;
         }
 
-        // Check if engine process is still alive (if PID file exists)
-        if pid_path.exists() {
-            if let Ok(pid_str) = std::fs::read_to_string(&pid_path) {
-                if let Ok(pid) = pid_str.trim().parse::<u32>() {
-                    if !process_alive(pid) && !conclusion_path.exists() {
-                        // Engine died without writing conclusion — drain and exit
-                        drain_remaining(&mut reader, &mut line, &progress_ui);
-                        break;
+        // Check if engine process is still alive (cache PID after first read)
+        let engine_alive = match cached_pid {
+            Some(pid) => process_alive(pid),
+            None => {
+                if let Ok(pid_str) = std::fs::read_to_string(&pid_path) {
+                    if let Ok(pid) = pid_str.trim().parse::<u32>() {
+                        cached_pid = Some(pid);
+                        process_alive(pid)
+                    } else {
+                        true
                     }
+                } else {
+                    true // no PID file yet, assume alive
                 }
             }
+        };
+        if !engine_alive && !conclusion_path.exists() {
+            drain_remaining(&mut reader, &mut line, &mut progress_ui);
+            break;
         }
 
         tokio::time::sleep(std::time::Duration::from_millis(100)).await;
     }
 
     // Finish progress bars
-    progress_ui.lock().expect("progress lock poisoned").finish();
+    progress_ui.finish();
 
     // Determine exit code from conclusion
     if conclusion_path.exists() {
@@ -173,7 +173,7 @@ pub async fn attach_run(
 fn drain_remaining(
     reader: &mut BufReader<std::fs::File>,
     line: &mut String,
-    progress_ui: &Arc<Mutex<run_progress::ProgressUI>>,
+    progress_ui: &mut run_progress::ProgressUI,
 ) {
     loop {
         line.clear();
@@ -182,10 +182,7 @@ fn drain_remaining(
             Ok(_) => {
                 let trimmed = line.trim();
                 if !trimmed.is_empty() {
-                    progress_ui
-                        .lock()
-                        .expect("progress lock poisoned")
-                        .handle_json_line(trimmed);
+                    progress_ui.handle_json_line(trimmed);
                 }
             }
             Err(_) => break,

lib/crates/fabro-cli/src/commands/create.rs

@@ -1,118 +1,21 @@
 use std::path::PathBuf;
 
-use anyhow::{bail, Context};
+use anyhow::bail;
 use chrono::Local;
+use fabro_config::project as project_config;
 use fabro_config::run::RunDefaults;
-use fabro_config::{project as project_config, sandbox as sandbox_config};
 use fabro_validate::Severity;
 use fabro_workflows::run_spec::RunSpec;
 use fabro_workflows::sandbox_provider::SandboxProvider;
 use fabro_workflows::workflow::WorkflowBuilder;
 
-use super::run::RunArgs;
+use super::run::{
+    apply_goal_override, resolve_cli_goal, resolve_model_provider, resolve_sandbox_provider,
+    RunArgs,
+};
 use super::shared::{print_diagnostics, read_workflow_file, relative_path};
 use fabro_util::terminal::Styles;
 
-/// Resolve goal from `--goal` string or `--goal-file` path.
-fn resolve_cli_goal(
-    goal: &Option<String>,
-    goal_file: &Option<PathBuf>,
-) -> anyhow::Result<Option<String>> {
-    match (goal, goal_file) {
-        (Some(g), _) => Ok(Some(g.clone())),
-        (_, Some(path)) => {
-            let path = fabro_util::path::expand_tilde(path);
-            let content = std::fs::read_to_string(&path)
-                .with_context(|| format!("failed to read goal file: {}", path.display()))?;
-            tracing::debug!(path = %path.display(), "Goal loaded from file");
-            Ok(Some(content))
-        }
-        _ => Ok(None),
-    }
-}
-
-/// Apply goal to the graph from TOML config or CLI flag.
-fn apply_goal_override(
-    graph: &mut fabro_graphviz::graph::Graph,
-    cli_goal: Option<&str>,
-    toml_goal: Option<&str>,
-) {
-    let goal = cli_goal.or(toml_goal);
-    if let Some(goal) = goal {
-        graph.attrs.insert(
-            "goal".to_string(),
-            fabro_graphviz::graph::AttrValue::String(goal.to_string()),
-        );
-    }
-}
-
-/// Parse sandbox provider from an optional `SandboxConfig`.
-fn parse_sandbox_provider(
-    sandbox: Option<&sandbox_config::SandboxConfig>,
-) -> anyhow::Result<Option<SandboxProvider>> {
-    sandbox
-        .and_then(|s| s.provider.as_deref())
-        .map(|s| s.parse::<SandboxProvider>())
-        .transpose()
-        .map_err(|e| anyhow::anyhow!("Invalid sandbox provider: {e}"))
-}
-
-/// Resolve sandbox provider: CLI flag > TOML config > run defaults > default.
-fn resolve_sandbox_provider(
-    cli: Option<SandboxProvider>,
-    run_cfg: Option<&fabro_config::run::WorkflowRunConfig>,
-    run_defaults: &RunDefaults,
-) -> anyhow::Result<SandboxProvider> {
-    let toml = parse_sandbox_provider(run_cfg.and_then(|c| c.sandbox.as_ref()))?;
-    let defaults = parse_sandbox_provider(run_defaults.sandbox.as_ref())?;
-    Ok(cli.or(toml).or(defaults).unwrap_or_default())
-}
-
-/// Resolve model and provider through the full precedence chain.
-fn resolve_model_provider(
-    cli_model: Option<&str>,
-    cli_provider: Option<&str>,
-    run_cfg: Option<&fabro_config::run::WorkflowRunConfig>,
-    run_defaults: &RunDefaults,
-    graph: &fabro_graphviz::graph::Graph,
-) -> (String, Option<String>) {
-    let toml_model = run_cfg
-        .and_then(|c| c.llm.as_ref())
-        .and_then(|l| l.model.as_deref());
-    let toml_provider = run_cfg
-        .and_then(|c| c.llm.as_ref())
-        .and_then(|l| l.provider.as_deref());
-    let defaults_model = run_defaults.llm.as_ref().and_then(|l| l.model.as_deref());
-    let defaults_provider = run_defaults
-        .llm
-        .as_ref()
-        .and_then(|l| l.provider.as_deref());
-
-    let provider = cli_provider
-        .or(toml_provider)
-        .or(defaults_provider)
-        .or_else(|| graph.attrs.get("default_provider").and_then(|v| v.as_str()))
-        .map(String::from);
-
-    let model = cli_model
-        .or(toml_model)
-        .or(defaults_model)
-        .or_else(|| graph.attrs.get("default_model").and_then(|v| v.as_str()))
-        .map(String::from)
-        .unwrap_or_else(|| {
-            provider
-                .as_deref()
-                .and_then(fabro_llm::catalog::default_model_for_provider)
-                .unwrap_or_else(fabro_llm::catalog::default_model_from_env)
-                .id
-        });
-
-    match fabro_llm::catalog::get_model_info(&model) {
-        Some(info) => (info.id, provider.or(Some(info.provider))),
-        None => (model, provider),
-    }
-}
-
 /// Create a workflow run: allocate run directory, persist spec, return (run_id, run_dir).
 ///
 /// This does NOT execute the workflow — it only prepares the run directory.

lib/crates/fabro-cli/src/commands/run.rs

@@ -57,6 +57,19 @@ impl From<CliSandboxProvider> for SandboxProvider {
     }
 }
 
+impl From<SandboxProvider> for CliSandboxProvider {
+    fn from(value: SandboxProvider) -> Self {
+        match value {
+            SandboxProvider::Local => Self::Local,
+            SandboxProvider::Docker => Self::Docker,
+            SandboxProvider::Daytona => Self::Daytona,
+            #[cfg(feature = "exedev")]
+            SandboxProvider::Exe => Self::Exe,
+            SandboxProvider::Ssh => Self::Ssh,
+        }
+    }
+}
+
 #[derive(Args)]
 pub struct RunArgs {
     /// Path to a .fabro workflow file or .toml task config (not required with --run-branch)
@@ -137,7 +150,7 @@ pub struct RunArgs {
 }
 
 /// Resolve goal from `--goal` string or `--goal-file` path.
-fn resolve_cli_goal(
+pub(crate) fn resolve_cli_goal(
     goal: &Option<String>,
     goal_file: &Option<PathBuf>,
 ) -> anyhow::Result<Option<String>> {
@@ -156,7 +169,7 @@ fn resolve_cli_goal(
 
 /// Apply goal to the graph from TOML config or CLI flag.
 /// Precedence: CLI `--goal` / `--goal-file` > TOML `goal` > DOT `graph [goal="..."]`.
-fn apply_goal_override(
+pub(crate) fn apply_goal_override(
     graph: &mut fabro_graphviz::graph::Graph,
     cli_goal: Option<&str>,
     toml_goal: Option<&str>,
@@ -174,7 +187,7 @@ fn apply_goal_override(
 /// Resolve model and provider through the full precedence chain:
 /// CLI flag > TOML config > run defaults > DOT graph attrs > provider-specific defaults.
 /// Then resolve through the catalog for alias expansion.
-fn resolve_model_provider(
+pub(crate) fn resolve_model_provider(
     cli_model: Option<&str>,
     cli_provider: Option<&str>,
     run_cfg: Option<&WorkflowRunConfig>,
@@ -221,7 +234,7 @@ fn resolve_model_provider(
 }
 
 /// Parse sandbox provider from an optional `SandboxConfig`.
-fn parse_sandbox_provider(
+pub(crate) fn parse_sandbox_provider(
     sandbox: Option<&sandbox_config::SandboxConfig>,
 ) -> anyhow::Result<Option<SandboxProvider>> {
     sandbox
@@ -232,7 +245,7 @@ fn parse_sandbox_provider(
 }
 
 /// Resolve sandbox provider: CLI flag > TOML config > run defaults > default.
-fn resolve_sandbox_provider(
+pub(crate) fn resolve_sandbox_provider(
     cli: Option<SandboxProvider>,
     run_cfg: Option<&WorkflowRunConfig>,
     run_defaults: &RunDefaults,

lib/crates/fabro-cli/src/commands/run_progress.rs

@@ -776,15 +776,13 @@ impl ProgressUI {
                 let stage = str_field("stage").unwrap_or("?");
                 let tool_name = str_field("tool_name").unwrap_or("?");
                 let tool_call_id = str_field("tool_call_id").unwrap_or("?");
-                let arguments = envelope
-                    .get("arguments")
-                    .cloned()
-                    .unwrap_or(serde_json::Value::Object(serde_json::Map::new()));
+                let empty = serde_json::Value::Object(serde_json::Map::new());
+                let arguments = envelope.get("arguments").unwrap_or(&empty);
                 // Update tool_call count
                 if let Some(counts) = self.stage_counts.get_mut(stage) {
                     counts.1 += 1;
                 }
-                self.on_tool_call_started(stage, tool_name, tool_call_id, &arguments);
+                self.on_tool_call_started(stage, tool_name, tool_call_id, arguments);
             }
             "Agent.ToolCallCompleted" => {
                 let stage = str_field("stage").unwrap_or("?");
@@ -1560,43 +1558,33 @@ impl ProgressAwareInterviewer {
     pub fn new(inner: ConsoleInterviewer, progress: Arc<Mutex<ProgressUI>>) -> Self {
         Self { inner, progress }
     }
-
-    fn hide_bars(&self) {
-        let ui = self.progress.lock().expect("progress lock poisoned");
-        if let ProgressRenderer::Tty(tty) = &ui.renderer {
-            tty.multi.set_draw_target(ProgressDrawTarget::hidden());
-        }
-    }
-
-    fn show_bars(&self) {
-        let ui = self.progress.lock().expect("progress lock poisoned");
-        if let ProgressRenderer::Tty(tty) = &ui.renderer {
-            tty.multi.set_draw_target(ProgressDrawTarget::stderr());
-        }
-    }
 }
 
 #[async_trait]
 impl Interviewer for ProgressAwareInterviewer {
     async fn ask(&self, question: Question) -> Answer {
-        {
-            let ui = self.progress.lock().expect("progress lock poisoned");
-            if let ProgressRenderer::Tty(tty) = &ui.renderer {
-                let sep = tty.multi.add(ProgressBar::new_spinner());
-                sep.set_style(style_empty());
-                sep.finish();
-                tty.multi.set_draw_target(ProgressDrawTarget::hidden());
-            }
-        }
+        self.progress
+            .lock()
+            .expect("progress lock poisoned")
+            .hide_bars();
         let answer = self.inner.ask(question).await;
-        self.show_bars();
+        self.progress
+            .lock()
+            .expect("progress lock poisoned")
+            .show_bars();
         answer
     }
 
     async fn inform(&self, message: &str, stage: &str) {
-        self.hide_bars();
+        self.progress
+            .lock()
+            .expect("progress lock poisoned")
+            .hide_bars();
         self.inner.inform(message, stage).await;
-        self.show_bars();
+        self.progress
+            .lock()
+            .expect("progress lock poisoned")
+            .show_bars();
     }
 }
 

lib/crates/fabro-cli/src/commands/start.rs

@@ -9,25 +9,19 @@ use anyhow::{bail, Result};
 pub fn start_run(run_dir: &Path) -> Result<u32> {
     // Validate status is Submitted
     let status_path = run_dir.join("status.json");
-    if status_path.exists() {
-        let record = fabro_workflows::run_status::RunStatusRecord::load(&status_path)
-            .map_err(|e| anyhow::anyhow!("Failed to read status.json: {e}"))?;
-        if record.status != fabro_workflows::run_status::RunStatus::Submitted {
+    match fabro_workflows::run_status::RunStatusRecord::load(&status_path) {
+        Ok(record) if record.status != fabro_workflows::run_status::RunStatus::Submitted => {
             bail!(
                 "Cannot start run: status is {:?}, expected Submitted",
                 record.status
             );
         }
+        _ => {} // No status file or Submitted — proceed
     }
 
-    // Validate spec.json exists
-    let spec_path = run_dir.join("spec.json");
-    if !spec_path.exists() {
-        bail!(
-            "Cannot start run: spec.json not found in {}",
-            run_dir.display()
-        );
-    }
+    // Validate spec.json is loadable
+    fabro_workflows::run_spec::RunSpec::load(run_dir)
+        .map_err(|e| anyhow::anyhow!("Cannot start run: failed to load spec.json: {e}"))?;
 
     let log_file = std::fs::File::create(run_dir.join("detach.log"))?;
 

lib/crates/fabro-cli/src/main.rs

@@ -728,24 +728,7 @@ async fn main_inner() -> (String, Result<()>) {
                         .sandbox_provider
                         .parse::<fabro_workflows::sandbox_provider::SandboxProvider>()
                         .ok()
-                        .map(|sp| match sp {
-                            fabro_workflows::sandbox_provider::SandboxProvider::Local => {
-                                commands::run::CliSandboxProvider::Local
-                            }
-                            fabro_workflows::sandbox_provider::SandboxProvider::Docker => {
-                                commands::run::CliSandboxProvider::Docker
-                            }
-                            fabro_workflows::sandbox_provider::SandboxProvider::Daytona => {
-                                commands::run::CliSandboxProvider::Daytona
-                            }
-                            fabro_workflows::sandbox_provider::SandboxProvider::Ssh => {
-                                commands::run::CliSandboxProvider::Ssh
-                            }
-                            #[cfg(feature = "exedev")]
-                            fabro_workflows::sandbox_provider::SandboxProvider::Exe => {
-                                commands::run::CliSandboxProvider::Exe
-                            }
-                        }),
+                        .map(commands::run::CliSandboxProvider::from),
                     label: spec
                         .labels
                         .into_iter()

lib/crates/fabro-interview/Cargo.toml

@@ -19,4 +19,4 @@ fabro-util = { path = "../fabro-util" }
 
 [dev-dependencies]
 tokio = { workspace = true, features = ["test-util", "macros"] }
-tempfile = "3"
+tempfile = "3"

lib/crates/fabro-interview/src/file.rs

@@ -45,23 +45,24 @@ impl Interviewer for FileInterviewer {
         let poll = async {
             let response_path = self.response_path();
             loop {
-                if response_path.exists() {
-                    match tokio::fs::read_to_string(&response_path).await {
-                        Ok(data) => match serde_json::from_str::<Answer>(&data) {
-                            Ok(answer) => {
-                                // Clean up both files
-                                let _ = tokio::fs::remove_file(&request_path).await;
-                                let _ = tokio::fs::remove_file(&response_path).await;
-                                return answer;
-                            }
-                            Err(e) => {
-                                tracing::warn!(error = %e, "Failed to parse interview response, retrying");
-                                // File might be partially written, wait and retry
-                            }
-                        },
-                        Err(e) => {
-                            tracing::warn!(error = %e, "Failed to read interview response, retrying");
+                match tokio::fs::read_to_string(&response_path).await {
+                    Ok(data) => match serde_json::from_str::<Answer>(&data) {
+                        Ok(answer) => {
+                            // Clean up both files
+                            let _ = tokio::fs::remove_file(&request_path).await;
+                            let _ = tokio::fs::remove_file(&response_path).await;
+                            return answer;
                         }
+                        Err(e) => {
+                            tracing::warn!(error = %e, "Failed to parse interview response, retrying");
+                            // File might be partially written, wait and retry
+                        }
+                    },
+                    Err(e) if e.kind() == std::io::ErrorKind::NotFound => {
+                        // Not written yet, poll again
+                    }
+                    Err(e) => {
+                        tracing::warn!(error = %e, "Failed to read interview response, retrying");
                     }
                 }
                 tokio::time::sleep(std::time::Duration::from_millis(100)).await;

lib/crates/fabro-workflows/src/run_spec.rs

@@ -3,7 +3,7 @@ use std::path::{Path, PathBuf};
 
 use serde::{Deserialize, Serialize};
 
-#[derive(Debug, Clone, Serialize, Deserialize)]
+#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)]
 pub struct RunSpec {
     pub run_id: String,
     pub workflow_path: PathBuf,
@@ -78,23 +78,7 @@ mod tests {
         spec.save(dir.path()).unwrap();
         let loaded = RunSpec::load(dir.path()).unwrap();
 
-        assert_eq!(loaded.run_id, spec.run_id);
-        assert_eq!(loaded.workflow_path, spec.workflow_path);
-        assert_eq!(loaded.dot_source, spec.dot_source);
-        assert_eq!(loaded.working_directory, spec.working_directory);
-        assert_eq!(loaded.goal, spec.goal);
-        assert_eq!(loaded.model, spec.model);
-        assert_eq!(loaded.provider, spec.provider);
-        assert_eq!(loaded.sandbox_provider, spec.sandbox_provider);
-        assert_eq!(loaded.labels, spec.labels);
-        assert_eq!(loaded.verbose, spec.verbose);
-        assert_eq!(loaded.no_retro, spec.no_retro);
-        assert_eq!(loaded.ssh, spec.ssh);
-        assert_eq!(loaded.preserve_sandbox, spec.preserve_sandbox);
-        assert_eq!(loaded.dry_run, spec.dry_run);
-        assert_eq!(loaded.auto_approve, spec.auto_approve);
-        assert_eq!(loaded.resume, spec.resume);
-        assert_eq!(loaded.run_branch, spec.run_branch);
+        assert_eq!(loaded, spec);
     }
 
     #[test]