mirror of
https://github.com/fabro-sh/fabro.git
synced 2026-09-05 08:10:39 +00:00
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) <noreply@anthropic.com>
This commit is contained in:
parent
7b7a7d829b
commit
cc7ab268aa
3 changed files with 30 additions and 35 deletions
|
|
@ -648,8 +648,20 @@ async fn execute_run(state: Arc<AppState>, 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,
|
||||
|
|
|
|||
|
|
@ -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<String> = None;
|
||||
let mut pr_url: Option<String> = 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");
|
||||
|
|
|
|||
|
|
@ -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.
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue