Fix split run lifecycle lookup

This commit is contained in:
Bryan Helmkamp 2026-03-23 08:44:46 -04:00
parent 6fa94ee36c
commit 587e5488bd
3 changed files with 204 additions and 21 deletions

View file

@ -1,10 +1,13 @@
use std::path::PathBuf;
use chrono::Utc;
use fabro_config::run::RunDefaults;
use fabro_workflows::manifest::Manifest;
use fabro_workflows::run_spec::RunSpec;
use super::run::{
cached_graph_path, default_run_dir, prepare_workflow, write_run_config_snapshot, RunArgs,
cached_graph_path, default_run_dir, prepare_workflow, workflow_slug_from_path,
write_run_config_snapshot, RunArgs,
};
use fabro_util::terminal::Styles;
@ -52,11 +55,17 @@ pub async fn create_run(
// Build and save RunSpec
let working_directory = std::env::current_dir().unwrap_or_else(|_| PathBuf::from("."));
let labels: std::collections::HashMap<String, String> = args
.label
.iter()
.filter_map(|s| s.split_once('='))
.map(|(k, v)| (k.to_string(), v.to_string()))
.collect();
let spec = RunSpec {
run_id: run_id.clone(),
workflow_path: std::fs::canonicalize(workflow_path).unwrap_or(workflow_path.clone()),
dot_source: prep.source,
working_directory,
working_directory: working_directory.clone(),
goal: if goal.is_empty() {
None
} else {
@ -65,12 +74,7 @@ pub async fn create_run(
model: prep.model,
provider: prep.provider,
sandbox_provider: prep.sandbox_provider.to_string(),
labels: args
.label
.iter()
.filter_map(|s| s.split_once('='))
.map(|(k, v)| (k.to_string(), v.to_string()))
.collect(),
labels: labels.clone(),
verbose: args.verbose,
no_retro: args.no_retro,
preserve_sandbox: args.preserve_sandbox,
@ -79,5 +83,29 @@ pub async fn create_run(
};
spec.save(&run_dir)?;
let workflow_name = if prep.graph.name.is_empty() {
"unnamed".to_string()
} else {
prep.graph.name.clone()
};
let base_branch = fabro_sandbox::daytona::detect_repo_info(&working_directory)
.ok()
.and_then(|(_, branch)| branch);
let manifest = Manifest {
run_id: run_id.clone(),
workflow_name,
goal: goal.to_string(),
start_time: Utc::now(),
node_count: prep.graph.nodes.len(),
edge_count: prep.graph.edges.len(),
run_branch: None,
base_sha: None,
labels,
base_branch,
workflow_slug: workflow_slug_from_path(workflow_path),
host_repo_path: Some(working_directory.to_string_lossy().to_string()),
};
manifest.save(&run_dir.join("manifest.json"))?;
Ok((run_id, run_dir))
}

View file

@ -179,17 +179,29 @@ pub(crate) fn apply_goal_override(
/// Compute the default run directory when `--run-dir` is not provided.
pub(crate) fn default_run_dir(run_id: &str, dry_run: bool) -> PathBuf {
let base = fabro_workflows::run_lookup::default_runs_base();
if dry_run {
std::env::temp_dir().join("fabro-dry-run").join(run_id)
base.join(format!(
"{}-dry-run-{}",
Local::now().format("%Y%m%d"),
run_id
))
} else {
let base = dirs::home_dir()
.expect("could not determine home directory")
.join(".fabro")
.join("runs");
base.join(format!("{}-{}", Local::now().format("%Y%m%d"), run_id))
}
}
pub(crate) fn workflow_slug_from_path(workflow_path: &Path) -> Option<String> {
if workflow_path.extension().is_none() {
Some(workflow_path.to_string_lossy().into_owned())
} else {
workflow_path
.parent()
.and_then(|p| p.file_name())
.map(|n| n.to_string_lossy().into_owned())
}
}
/// 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.
@ -692,14 +704,7 @@ pub async fn run_command(
// 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: Option<String> = if workflow_path.extension().is_none() {
Some(workflow_path.to_string_lossy().into_owned())
} else {
workflow_path
.parent()
.and_then(|p| p.file_name())
.map(|n| n.to_string_lossy().into_owned())
};
let workflow_slug = workflow_slug_from_path(workflow_path);
// Collect setup commands — they'll be run inside the sandbox
let setup_commands: Vec<String> = run_cfg

View file

@ -603,6 +603,156 @@ fn setup_run_dir(
run_dir
}
fn find_run_dir(home: &std::path::Path, run_id: &str) -> std::path::PathBuf {
let runs_dir = home.join(".fabro").join("runs");
std::fs::read_dir(&runs_dir)
.unwrap()
.flatten()
.map(|entry| entry.path())
.find(|path| {
path.is_dir()
&& path
.file_name()
.is_some_and(|name| name.to_string_lossy().ends_with(run_id))
})
.unwrap_or_else(|| {
panic!(
"expected run directory for {run_id} under {}",
runs_dir.display()
)
})
}
#[test]
fn dry_run_create_start_attach_works_with_default_run_lookup() {
let home = tempfile::tempdir().unwrap();
let run_id = "drysplit-test-123";
arc()
.env("HOME", home.path())
.args([
"create",
"--dry-run",
"--auto-approve",
"--run-id",
run_id,
"../../../test/simple.fabro",
])
.assert()
.success()
.stdout(predicate::str::contains(run_id));
let run_dir = find_run_dir(home.path(), run_id);
assert!(
run_dir.join("manifest.json").exists(),
"create should persist manifest.json so the run is discoverable"
);
arc()
.env("HOME", home.path())
.args(["start", run_id])
.assert()
.success();
arc()
.env("HOME", home.path())
.args(["attach", run_id])
.timeout(std::time::Duration::from_secs(10))
.assert()
.success();
assert!(run_dir.join("conclusion.json").exists());
}
#[test]
fn dry_run_detach_attach_works_with_default_run_lookup() {
let home = tempfile::tempdir().unwrap();
let run_id = "drydetach-test-123";
arc()
.env("HOME", home.path())
.args([
"run",
"--detach",
"--dry-run",
"--auto-approve",
"--run-id",
run_id,
"../../../test/simple.fabro",
])
.assert()
.success()
.stdout(predicate::str::contains(run_id));
arc()
.env("HOME", home.path())
.args(["attach", run_id])
.timeout(std::time::Duration::from_secs(10))
.assert()
.success();
}
#[test]
fn start_by_workflow_name_prefers_newly_created_submitted_run() {
let home = tempfile::tempdir().unwrap();
let old_run_dir = home.path().join(".fabro").join("runs").join("old-smoke");
std::fs::create_dir_all(&old_run_dir).unwrap();
std::fs::write(
old_run_dir.join("manifest.json"),
serde_json::json!({
"run_id": "old-smoke",
"workflow_name": "Smoke",
"goal": "",
"start_time": "2026-01-01T00:00:00Z",
"node_count": 1,
"edge_count": 0
})
.to_string(),
)
.unwrap();
std::fs::write(
old_run_dir.join("status.json"),
serde_json::json!({"status": "succeeded", "updated_at": "2026-01-01T00:00:00Z"})
.to_string(),
)
.unwrap();
let run_id = "new-smoke-run-123";
arc()
.env("HOME", home.path())
.args([
"create",
"--dry-run",
"--auto-approve",
"--run-id",
run_id,
"smoke",
])
.assert()
.success()
.stdout(predicate::str::contains(run_id));
arc()
.env("HOME", home.path())
.args(["start", "smoke"])
.assert()
.success();
arc()
.env("HOME", home.path())
.args(["attach", run_id])
.timeout(std::time::Duration::from_secs(10))
.assert()
.success();
let new_run_dir = find_run_dir(home.path(), run_id);
let status = std::fs::read_to_string(new_run_dir.join("status.json")).unwrap();
assert!(
status.contains("\"status\": \"succeeded\""),
"expected the newly created Smoke run to be started and completed"
);
}
// Bug 2: _run_engine should use cached graph.fabro, not spec.workflow_path.
// When the original workflow file is deleted between create and start,
// the engine should read the snapshot saved at create time.