From cc7ab268aaa62c3381899a59a76dcbe7909baee1 Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Tue, 24 Mar 2026 21:15:11 -0400 Subject: [PATCH] Clean up RunSettings migration: fix naming, dedup, error handling - Rename `settings: mut config` binding to `mut settings` in resume.rs and update all 8 downstream references - Deduplicate normalize_config call in run.rs by reusing the result computed for RunRecord - Replace expect() with graceful error handling when loading RunRecord in the API server's execute_run Co-Authored-By: Claude Opus 4.6 (1M context) --- lib/crates/fabro-api/src/server.rs | 16 ++++++++++-- lib/crates/fabro-cli/src/commands/resume.rs | 22 ++++++++--------- lib/crates/fabro-cli/src/commands/run.rs | 27 ++++----------------- 3 files changed, 30 insertions(+), 35 deletions(-) diff --git a/lib/crates/fabro-api/src/server.rs b/lib/crates/fabro-api/src/server.rs index b75470c5e..26eb75cbf 100644 --- a/lib/crates/fabro-api/src/server.rs +++ b/lib/crates/fabro-api/src/server.rs @@ -648,8 +648,20 @@ async fn execute_run(state: Arc, run_id: String) { } } - let run_record = fabro_workflows::run_record::RunRecord::load(&run_dir) - .expect("RunRecord must exist — written by start_run"); + let run_record = match fabro_workflows::run_record::RunRecord::load(&run_dir) { + Ok(r) => r, + Err(e) => { + tracing::error!(run_id = %run_id, error = %e, "Failed to load RunRecord"); + let mut runs = state.runs.lock().expect("runs lock poisoned"); + if let Some(managed_run) = runs.get_mut(&run_id) { + managed_run.status = RunStatus::Failed; + managed_run.error = Some(format!("Failed to load run record: {e}")); + managed_run.event_tx = None; + } + state.scheduler_notify.notify_one(); + return; + } + }; let config = RunSettings { config: run_record.config, run_dir, diff --git a/lib/crates/fabro-cli/src/commands/resume.rs b/lib/crates/fabro-cli/src/commands/resume.rs index 2e5a247af..7026bcd06 100644 --- a/lib/crates/fabro-cli/src/commands/resume.rs +++ b/lib/crates/fabro-cli/src/commands/resume.rs @@ -937,7 +937,7 @@ async fn run_resumed( mut run_cfg, sandbox, emitter, - settings: mut config, + mut settings, setup_commands, devcontainer_phases, devcontainer_env, @@ -1137,7 +1137,7 @@ async fn run_resumed( } } }; - config.dry_run = dry_run_mode; + settings.dry_run = dry_run_mode; if let Some(ref mut cfg) = run_cfg { run_config::resolve_sandbox_env(cfg)?; @@ -1295,7 +1295,7 @@ async fn run_resumed( let run_start = Instant::now(); let engine_result = engine - .run_with_lifecycle(&graph, &mut config, lifecycle, Some(&checkpoint)) + .run_with_lifecycle(&graph, &mut settings, lifecycle, Some(&checkpoint)) .await; let run_duration_ms = run_start.elapsed().as_millis() as u64; let mut completion_guard = DetachedRunCompletionGuard::arm(&run_dir); @@ -1329,7 +1329,7 @@ async fn run_resumed( }; generate_retro( - &config.run_id, + &settings.run_id, &graph.name, graph.goal(), &run_dir, @@ -1350,13 +1350,13 @@ async fn run_resumed( progress_ui.lock().expect("progress lock poisoned").finish(); // Write finalize commit with retro.json + final node files (captures last diff.patch) - write_finalize_commit(&config, &run_dir).await; + write_finalize_commit(&settings, &run_dir).await; // Auto-create PR on successful completion (mirrors run_command) let mut pushed_branch: Option = None; let mut pr_url: Option = None; - if let Some(pr_cfg) = config.pull_request() { - if config.dry_run { + if let Some(pr_cfg) = settings.pull_request() { + if settings.dry_run { debug!("Skipping PR creation: dry-run mode"); } else if let Err(ref e) = engine_result { debug!(error = %e, "Skipping PR creation: engine returned an error"); @@ -1376,12 +1376,12 @@ async fn run_resumed( Some(ref creds), Some(ref origin), ) = ( - &config.base_branch, - config.git.as_ref().and_then(|g| g.run_branch.as_ref()), + &settings.base_branch, + settings.git.as_ref().and_then(|g| g.run_branch.as_ref()), &github_app, &origin_url, ) { - if config.git.is_some() { + if settings.git.is_some() { pushed_branch = Some(run_branch.to_string()); } @@ -1466,7 +1466,7 @@ async fn run_resumed( } } if let Err(e) = engine - .cleanup_sandbox(&config.run_id, &graph.name, preserve) + .cleanup_sandbox(&settings.run_id, &graph.name, preserve) .await { tracing::warn!(error = %e, "Sandbox cleanup failed"); diff --git a/lib/crates/fabro-cli/src/commands/run.rs b/lib/crates/fabro-cli/src/commands/run.rs index dea912c2d..e528b916d 100644 --- a/lib/crates/fabro-cli/src/commands/run.rs +++ b/lib/crates/fabro-cli/src/commands/run.rs @@ -982,8 +982,8 @@ async fn run_command_impl( write_run_config_snapshot(&run_dir, workflow_toml_path.as_deref()).await?; } - // Write RunRecord - if !cached_run_restart { + // Write RunRecord and compute normalized config for RunSettings + let settings_config = if !cached_run_restart { let cli_flags = super::create::CliFlags { dry_run: dry_run_flag, auto_approve: auto_approve_flag, @@ -1003,7 +1003,7 @@ async fn run_command_impl( let record = fabro_workflows::run_record::RunRecord { run_id: run_id.clone(), created_at: chrono::Utc::now(), - config: normalized_config, + config: normalized_config.clone(), graph: graph.clone(), workflow_slug: workflow_slug.clone(), working_directory: original_cwd.clone(), @@ -1016,29 +1016,12 @@ async fn run_command_impl( .collect(), }; record.save(&run_dir)?; - } - - let settings_config = if cached_run_restart { + normalized_config + } else { existing_record .as_ref() .map(|r| r.config.clone()) .unwrap_or_default() - } else { - super::create::normalize_config( - run_cfg.as_ref(), - &run_defaults, - &model, - provider.as_deref(), - sandbox_provider, - &graph, - super::create::CliFlags { - dry_run: dry_run_flag, - auto_approve: auto_approve_flag, - no_retro: no_retro_flag, - verbose: verbose_flag, - preserve_sandbox: preserve_sandbox_flag, - }, - ) }; // Now resolve ${env.VARNAME} references for runtime use.