From b5c014443e4f5865951e58173e3ac750c5284fff Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Mon, 23 Mar 2026 10:04:49 -0400 Subject: [PATCH] Preserve split run metadata across restarts --- lib/crates/fabro-cli/src/commands/resume.rs | 64 +++++++++++++++++++-- lib/crates/fabro-cli/src/commands/run.rs | 32 +++++++++-- lib/crates/fabro-cli/tests/cli.rs | 64 +++++++++++++++++++++ 3 files changed, 148 insertions(+), 12 deletions(-) diff --git a/lib/crates/fabro-cli/src/commands/resume.rs b/lib/crates/fabro-cli/src/commands/resume.rs index 68b61a869..75390de61 100644 --- a/lib/crates/fabro-cli/src/commands/resume.rs +++ b/lib/crates/fabro-cli/src/commands/resume.rs @@ -568,7 +568,7 @@ async fn prepare_from_branch( } else if args.dry_run { default_run_dir(&run_id, true) } else { - find_existing_run_dir(&run_id).unwrap_or_else(|| default_run_dir(&run_id, false)) + find_existing_run_dir(&run_id, false).unwrap_or_else(|| default_run_dir(&run_id, false)) }; tokio::fs::create_dir_all(&run_dir).await?; let run_dir = tokio::fs::canonicalize(&run_dir).await.unwrap_or(run_dir); @@ -1503,20 +1503,35 @@ async fn run_resumed( } } -/// Scan `~/.fabro/runs/` for an existing directory whose name ends with `-{run_id}`. -fn find_existing_run_dir(run_id: &str) -> Option { - let base = dirs::home_dir()?.join(".fabro").join("runs"); +/// Scan a runs directory for an existing directory matching the requested dry-run mode. +fn find_existing_run_dir_in( + base: &std::path::Path, + run_id: &str, + dry_run: bool, +) -> Option { let suffix = format!("-{run_id}"); - let entries = std::fs::read_dir(&base).ok()?; + let entries = std::fs::read_dir(base).ok()?; for entry in entries.flatten() { let name = entry.file_name(); - if name.to_string_lossy().ends_with(&suffix) && entry.path().is_dir() { + let name = name.to_string_lossy(); + if entry.path().is_dir() && run_dir_name_matches_mode(&name, &suffix, dry_run) { return Some(entry.path()); } } None } +/// Scan `~/.fabro/runs/` for an existing directory matching the requested dry-run mode. +fn find_existing_run_dir(run_id: &str, dry_run: bool) -> Option { + let base = dirs::home_dir()?.join(".fabro").join("runs"); + find_existing_run_dir_in(&base, run_id, dry_run) +} + +fn run_dir_name_matches_mode(name: &str, run_id_suffix: &str, dry_run: bool) -> bool { + name.strip_suffix(run_id_suffix) + .is_some_and(|prefix| prefix.ends_with("-dry-run") == dry_run) +} + #[cfg(test)] mod tests { use super::*; @@ -1561,6 +1576,43 @@ mod tests { assert_eq!(selected, cwd.path()); } + #[test] + fn find_existing_run_dir_in_respects_dry_run_mode() { + let runs = tempfile::tempdir().unwrap(); + let non_dry = runs.path().join("20260323-run-1"); + let dry = runs.path().join("20260323-dry-run-run-1"); + std::fs::create_dir_all(&non_dry).unwrap(); + std::fs::create_dir_all(&dry).unwrap(); + + assert_eq!( + find_existing_run_dir_in(runs.path(), "run-1", false), + Some(non_dry) + ); + assert_eq!( + find_existing_run_dir_in(runs.path(), "run-1", true), + Some(dry) + ); + } + + #[test] + fn find_existing_run_dir_in_does_not_misclassify_run_ids_containing_dry_run() { + let runs = tempfile::tempdir().unwrap(); + let run_id = "feature-dry-run-fix"; + let non_dry = runs.path().join(format!("20260323-{run_id}")); + let dry = runs.path().join(format!("20260323-dry-run-{run_id}")); + std::fs::create_dir_all(&non_dry).unwrap(); + std::fs::create_dir_all(&dry).unwrap(); + + assert_eq!( + find_existing_run_dir_in(runs.path(), run_id, false), + Some(non_dry) + ); + assert_eq!( + find_existing_run_dir_in(runs.path(), run_id, true), + Some(dry) + ); + } + #[test] fn resume_bootstrap_guard_marks_failed_on_drop() { let dir = tempfile::tempdir().unwrap(); diff --git a/lib/crates/fabro-cli/src/commands/run.rs b/lib/crates/fabro-cli/src/commands/run.rs index abd57be46..8fc7f8a5c 100644 --- a/lib/crates/fabro-cli/src/commands/run.rs +++ b/lib/crates/fabro-cli/src/commands/run.rs @@ -25,6 +25,7 @@ use fabro_workflows::engine::{RunConfig, WorkflowRunEngine}; use fabro_workflows::event::{EventEmitter, RunNoticeLevel, WorkflowRunEvent}; use fabro_workflows::git::GitSyncStatus; use fabro_workflows::handler::default_registry; +use fabro_workflows::manifest::Manifest; use fabro_workflows::outcome::{Outcome, StageStatus}; use fabro_workflows::run_status::{RunStatus, StatusReason}; use fabro_workflows::sandbox_provider::SandboxProvider; @@ -202,6 +203,13 @@ pub(crate) fn workflow_slug_from_path(workflow_path: &Path) -> Option { } } +fn is_cached_run_restart(workflow_path: &Path, run_dir: &Path) -> bool { + workflow_path.starts_with(run_dir) + && workflow_path.file_name().is_some_and(|f| { + f == std::ffi::OsStr::new(RUN_CONFIG_FILE) || f == std::ffi::OsStr::new(RUN_GRAPH_FILE) + }) +} + /// 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. @@ -700,11 +708,7 @@ pub async fn run_command( run_defaults, } = prepare_workflow(&args, run_defaults, styles, false)?; - // Extract workflow slug from the workflow path argument. - // If bare name (no extension, e.g. "smoke"), use it directly. - // Otherwise derive from the parent directory of the resolved .toml path. let workflow_path = args.workflow.as_ref().unwrap(); // safe: prepare_workflow validated - let workflow_slug = workflow_slug_from_path(workflow_path); // Collect setup commands — they'll be run inside the sandbox let setup_commands: Vec = run_cfg @@ -746,6 +750,16 @@ pub async fn run_command( .run_dir .unwrap_or_else(|| default_run_dir(&run_id, args.dry_run)); tokio::fs::create_dir_all(&run_dir).await?; + let cached_run_restart = is_cached_run_restart(workflow_path, &run_dir); + let existing_manifest = if cached_run_restart { + Manifest::load(&run_dir.join("manifest.json")).ok() + } else { + None + }; + let workflow_slug = existing_manifest + .as_ref() + .and_then(|manifest| manifest.workflow_slug.clone()) + .or_else(|| workflow_slug_from_path(workflow_path)); fabro_util::run_log::activate(&run_dir.join("cli.log")) .context("Failed to activate per-run log")?; tokio::fs::write(cached_graph_path(&run_dir), &source).await?; @@ -1467,7 +1481,10 @@ pub async fn run_command( dry_run: dry_run_mode, run_id: run_id.clone(), git_checkpoint_enabled: worktree_path.is_some(), - host_repo_path: Some(original_cwd.clone()), + host_repo_path: existing_manifest + .as_ref() + .and_then(|manifest| manifest.host_repo_path.as_deref().map(PathBuf::from)) + .or_else(|| Some(original_cwd.clone())), base_sha: worktree_base_sha, run_branch: worktree_branch, meta_branch, @@ -1480,7 +1497,10 @@ pub async fn run_command( checkpoint_exclude_globs, github_app: github_app.clone(), git_author, - base_branch: detected_base_branch, + base_branch: existing_manifest + .as_ref() + .and_then(|manifest| manifest.base_branch.clone()) + .or(detected_base_branch), pull_request: run_cfg .as_ref() .and_then(|c| c.pull_request.as_ref()) diff --git a/lib/crates/fabro-cli/tests/cli.rs b/lib/crates/fabro-cli/tests/cli.rs index bb73bea2b..fda7a2f64 100644 --- a/lib/crates/fabro-cli/tests/cli.rs +++ b/lib/crates/fabro-cli/tests/cli.rs @@ -623,6 +623,70 @@ fn find_run_dir(home: &std::path::Path, run_id: &str) -> std::path::PathBuf { }) } +#[test] +fn completed_run_preserves_workflow_slug_for_lookup() { + let home = tempfile::tempdir().unwrap(); + let project = tempfile::tempdir().unwrap(); + let workflow_dir = project.path().join("workflows").join("sluggy"); + std::fs::create_dir_all(&workflow_dir).unwrap(); + let workflow_path = workflow_dir.join("workflow.fabro"); + std::fs::write( + &workflow_path, + "\ +digraph BarBaz { + start [shape=Mdiamond, label=\"Start\"] + exit [shape=Msquare, label=\"Exit\"] + start -> exit +} +", + ) + .unwrap(); + + arc() + .env("HOME", home.path()) + .current_dir(project.path()) + .args([ + "create", + "--dry-run", + "--auto-approve", + "--run-id", + "opaque-run-999", + workflow_path.to_str().unwrap(), + ]) + .assert() + .success(); + + arc() + .env("HOME", home.path()) + .current_dir(project.path()) + .args(["start", "sluggy"]) + .assert() + .success(); + + arc() + .env("HOME", home.path()) + .current_dir(project.path()) + .args(["attach", "opaque-run-999"]) + .timeout(std::time::Duration::from_secs(10)) + .assert() + .success(); + + arc() + .env("HOME", home.path()) + .current_dir(project.path()) + .args(["attach", "sluggy"]) + .timeout(std::time::Duration::from_secs(10)) + .assert() + .success(); + + let run_dir = find_run_dir(home.path(), "opaque-run-999"); + let manifest: serde_json::Value = + serde_json::from_str(&std::fs::read_to_string(run_dir.join("manifest.json")).unwrap()) + .unwrap(); + assert_eq!(manifest["workflow_name"].as_str(), Some("BarBaz")); + assert_eq!(manifest["workflow_slug"].as_str(), Some("sluggy")); +} + #[test] fn dry_run_create_start_attach_works_with_default_run_lookup() { let home = tempfile::tempdir().unwrap();