From 350df9315bb9d5ea548b640ee6cb2e97594bad26 Mon Sep 17 00:00:00 2001 From: Fabro Date: Fri, 20 Mar 2026 01:56:43 +0000 Subject: [PATCH] fabro(01KM4DKAWADPG0HY3PZCGQJ7H2): simplify_opus (success) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fabro-Run: 01KM4DKAWADPG0HY3PZCGQJ7H2 Fabro-Completed: 6 Fabro-Checkpoint: 140b78941227703052ca50cbb8f7743a7b8254b3 ⚒️ Generated with [Fabro](https://fabro.sh) --- lib/crates/fabro-cli/src/commands/attach.rs | 55 +++++---- lib/crates/fabro-cli/src/commands/create.rs | 109 +----------------- lib/crates/fabro-cli/src/commands/run.rs | 23 +++- .../fabro-cli/src/commands/run_progress.rs | 50 +++----- lib/crates/fabro-cli/src/commands/start.rs | 18 +-- lib/crates/fabro-cli/src/main.rs | 19 +-- lib/crates/fabro-interview/Cargo.toml | 2 +- lib/crates/fabro-interview/src/file.rs | 33 +++--- lib/crates/fabro-workflows/src/run_spec.rs | 20 +--- 9 files changed, 96 insertions(+), 233 deletions(-) diff --git a/lib/crates/fabro-cli/src/commands/attach.rs b/lib/crates/fabro-cli/src/commands/attach.rs index 3d8f3931f..03f6610c3 100644 --- a/lib/crates/fabro-cli/src/commands/attach.rs +++ b/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 = 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::(&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::() { - 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::() { + 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, line: &mut String, - progress_ui: &Arc>, + 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, diff --git a/lib/crates/fabro-cli/src/commands/create.rs b/lib/crates/fabro-cli/src/commands/create.rs index f3782cec0..d5db83934 100644 --- a/lib/crates/fabro-cli/src/commands/create.rs +++ b/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, - goal_file: &Option, -) -> anyhow::Result> { - 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> { - sandbox - .and_then(|s| s.provider.as_deref()) - .map(|s| s.parse::()) - .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, - run_cfg: Option<&fabro_config::run::WorkflowRunConfig>, - run_defaults: &RunDefaults, -) -> anyhow::Result { - 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) { - 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. diff --git a/lib/crates/fabro-cli/src/commands/run.rs b/lib/crates/fabro-cli/src/commands/run.rs index da58742f5..2bca60f66 100644 --- a/lib/crates/fabro-cli/src/commands/run.rs +++ b/lib/crates/fabro-cli/src/commands/run.rs @@ -57,6 +57,19 @@ impl From for SandboxProvider { } } +impl From 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, goal_file: &Option, ) -> anyhow::Result> { @@ -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> { 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, run_cfg: Option<&WorkflowRunConfig>, run_defaults: &RunDefaults, diff --git a/lib/crates/fabro-cli/src/commands/run_progress.rs b/lib/crates/fabro-cli/src/commands/run_progress.rs index 2619cee04..dc0bebea8 100644 --- a/lib/crates/fabro-cli/src/commands/run_progress.rs +++ b/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>) -> 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(); } } diff --git a/lib/crates/fabro-cli/src/commands/start.rs b/lib/crates/fabro-cli/src/commands/start.rs index f4213f6aa..840d89198 100644 --- a/lib/crates/fabro-cli/src/commands/start.rs +++ b/lib/crates/fabro-cli/src/commands/start.rs @@ -9,25 +9,19 @@ use anyhow::{bail, Result}; pub fn start_run(run_dir: &Path) -> Result { // 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"))?; diff --git a/lib/crates/fabro-cli/src/main.rs b/lib/crates/fabro-cli/src/main.rs index 2c8f5c8a9..28c73b232 100644 --- a/lib/crates/fabro-cli/src/main.rs +++ b/lib/crates/fabro-cli/src/main.rs @@ -728,24 +728,7 @@ async fn main_inner() -> (String, Result<()>) { .sandbox_provider .parse::() .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() diff --git a/lib/crates/fabro-interview/Cargo.toml b/lib/crates/fabro-interview/Cargo.toml index d641da188..ef57afbe6 100644 --- a/lib/crates/fabro-interview/Cargo.toml +++ b/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" \ No newline at end of file +tempfile = "3" diff --git a/lib/crates/fabro-interview/src/file.rs b/lib/crates/fabro-interview/src/file.rs index 1f0def2a9..2a7e41dc0 100644 --- a/lib/crates/fabro-interview/src/file.rs +++ b/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::(&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::(&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; diff --git a/lib/crates/fabro-workflows/src/run_spec.rs b/lib/crates/fabro-workflows/src/run_spec.rs index da5d51f82..df5dbac52 100644 --- a/lib/crates/fabro-workflows/src/run_spec.rs +++ b/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]