From 2aa6476437be8e66cce3ac87776508995aed2143 Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Tue, 24 Mar 2026 19:23:54 -0400 Subject: [PATCH] Remove RunSpec + Manifest types and all remaining references Delete run_spec.rs and manifest.rs modules. Remove write_manifest() from the engine, update DiskLifecycle and GitLifecycle to only write StartRecord. Remove read_manifest() from MetadataStore. Update run_fork to only handle run.json/start.json. Convert resume.rs to use RunRecord/StartRecord from the metadata branch. Update all tests and integration tests. Co-Authored-By: Claude Opus 4.6 (1M context) --- lib/crates/fabro-cli/src/commands/resume.rs | 74 ++++---- lib/crates/fabro-cli/src/commands/run.rs | 2 +- lib/crates/fabro-cli/src/main.rs | 2 +- lib/crates/fabro-cli/tests/cli.rs | 163 ++++++++---------- lib/crates/fabro-cli/tests/scenario.rs | 7 - .../src/core_adapter/lifecycle/disk.rs | 3 +- .../src/core_adapter/lifecycle/git.rs | 10 +- lib/crates/fabro-workflows/src/engine.rs | 134 ++++++-------- lib/crates/fabro-workflows/src/git.rs | 27 +-- lib/crates/fabro-workflows/src/lib.rs | 2 - lib/crates/fabro-workflows/src/manifest.rs | 129 -------------- lib/crates/fabro-workflows/src/run_fork.rs | 71 ++------ lib/crates/fabro-workflows/src/run_rewind.rs | 12 +- lib/crates/fabro-workflows/src/run_spec.rs | 84 --------- .../fabro-workflows/tests/integration.rs | 21 ++- 15 files changed, 205 insertions(+), 536 deletions(-) delete mode 100644 lib/crates/fabro-workflows/src/manifest.rs delete mode 100644 lib/crates/fabro-workflows/src/run_spec.rs diff --git a/lib/crates/fabro-cli/src/commands/resume.rs b/lib/crates/fabro-cli/src/commands/resume.rs index 5dfaf8fb7..1421c50ff 100644 --- a/lib/crates/fabro-cli/src/commands/resume.rs +++ b/lib/crates/fabro-cli/src/commands/resume.rs @@ -15,8 +15,8 @@ use fabro_workflows::backend::{AgentApiBackend, AgentCliBackend, BackendRouter}; use fabro_workflows::checkpoint::Checkpoint; use fabro_workflows::engine::RunConfig; use fabro_workflows::event::{EventEmitter, RunNoticeLevel}; -use fabro_workflows::manifest::Manifest; use fabro_workflows::outcome::StageStatus; +use fabro_workflows::run_record::RunRecord; use fabro_workflows::sandbox_provider::SandboxProvider; use indicatif::HumanDuration; @@ -141,10 +141,10 @@ fn resume_as_run_args(args: &ResumeArgs, workflow: PathBuf) -> RunArgs { fn preferred_resume_repo_path( original_cwd: &std::path::Path, - manifest: Option<&Manifest>, + record: Option<&RunRecord>, ) -> PathBuf { - manifest - .and_then(|m| m.host_repo_path.as_deref()) + record + .and_then(|r| r.host_repo_path.as_deref()) .map(PathBuf::from) .filter(|path| path.exists()) .unwrap_or_else(|| original_cwd.to_path_buf()) @@ -532,14 +532,22 @@ async fn prepare_from_branch( }; let original_cwd = std::env::current_dir()?; - let manifest_hint = fabro_workflows::git::MetadataStore::read_manifest(&original_cwd, &run_id)?; - let resume_repo_path = preferred_resume_repo_path(&original_cwd, manifest_hint.as_ref()); - let manifest = if resume_repo_path == original_cwd { - manifest_hint + let record_hint = fabro_workflows::git::MetadataStore::read_run_record(&original_cwd, &run_id) + .ok() + .flatten(); + let resume_repo_path = preferred_resume_repo_path(&original_cwd, record_hint.as_ref()); + let record = if resume_repo_path == original_cwd { + record_hint } else { - fabro_workflows::git::MetadataStore::read_manifest(&resume_repo_path, &run_id)? - .or(manifest_hint) + fabro_workflows::git::MetadataStore::read_run_record(&resume_repo_path, &run_id) + .ok() + .flatten() + .or(record_hint) }; + let start_record = + fabro_workflows::git::MetadataStore::read_start_record(&resume_repo_path, &run_id) + .ok() + .flatten(); let checkpoint = fabro_workflows::git::MetadataStore::read_checkpoint(&resume_repo_path, &run_id)? .ok_or_else(|| { @@ -552,11 +560,11 @@ async fn prepare_from_branch( let repo_info = fabro_sandbox::daytona::detect_repo_info(&resume_repo_path).ok(); let origin_url = repo_info.as_ref().map(|(url, _)| url.clone()); - let detected_base_branch = manifest + let detected_base_branch = record .as_ref() - .and_then(|m| m.base_branch.clone()) + .and_then(|r| r.base_branch.clone()) .or_else(|| repo_info.as_ref().and_then(|(_, branch)| branch.clone())); - let base_sha = manifest.as_ref().and_then(|m| m.base_sha.clone()); + let base_sha = start_record.as_ref().and_then(|s| s.base_sha.clone()); let (graph, graph_source, run_cfg, mut sandbox_provider, workflow_slug) = if let Some(ref workflow_path) = args.workflow { @@ -595,7 +603,7 @@ async fn prepare_from_branch( source.clone(), None, sandbox_provider, - manifest.as_ref().and_then(|m| m.workflow_slug.clone()), + record.as_ref().and_then(|r| r.workflow_slug.clone()), ) }; @@ -1624,41 +1632,41 @@ mod tests { use chrono::Utc; use fabro_workflows::run_status::{RunStatus, RunStatusRecord, StatusReason}; - fn sample_manifest() -> Manifest { - Manifest { + fn sample_run_record() -> RunRecord { + RunRecord { run_id: "run-1".to_string(), - workflow_name: "resume".to_string(), - goal: "fix bug".to_string(), - start_time: Utc::now(), - node_count: 1, - edge_count: 0, - run_branch: Some("fabro/run/run-1".to_string()), - base_sha: Some("abc123".to_string()), - labels: HashMap::new(), - base_branch: Some("main".to_string()), + created_at: Utc::now(), + config: fabro_config::config::FabroConfig::default(), + graph: fabro_graphviz::graph::Graph { + name: "resume".to_string(), + ..Default::default() + }, workflow_slug: None, + working_directory: std::path::PathBuf::from("/tmp"), host_repo_path: None, + base_branch: Some("main".to_string()), + labels: HashMap::new(), } } #[test] - fn preferred_resume_repo_path_uses_manifest_host_repo_path_when_present() { + fn preferred_resume_repo_path_uses_record_host_repo_path_when_present() { let cwd = tempfile::tempdir().unwrap(); let host_repo = tempfile::tempdir().unwrap(); - let mut manifest = sample_manifest(); - manifest.host_repo_path = Some(host_repo.path().to_string_lossy().to_string()); + let mut record = sample_run_record(); + record.host_repo_path = Some(host_repo.path().to_string_lossy().to_string()); - let selected = preferred_resume_repo_path(cwd.path(), Some(&manifest)); + let selected = preferred_resume_repo_path(cwd.path(), Some(&record)); assert_eq!(selected, host_repo.path()); } #[test] - fn preferred_resume_repo_path_falls_back_when_manifest_path_is_missing() { + fn preferred_resume_repo_path_falls_back_when_record_path_is_missing() { let cwd = tempfile::tempdir().unwrap(); - let mut manifest = sample_manifest(); - manifest.host_repo_path = Some(cwd.path().join("missing-repo").display().to_string()); + let mut record = sample_run_record(); + record.host_repo_path = Some(cwd.path().join("missing-repo").display().to_string()); - let selected = preferred_resume_repo_path(cwd.path(), Some(&manifest)); + let selected = preferred_resume_repo_path(cwd.path(), Some(&record)); assert_eq!(selected, cwd.path()); } diff --git a/lib/crates/fabro-cli/src/commands/run.rs b/lib/crates/fabro-cli/src/commands/run.rs index afd53fb15..aca671088 100644 --- a/lib/crates/fabro-cli/src/commands/run.rs +++ b/lib/crates/fabro-cli/src/commands/run.rs @@ -827,7 +827,7 @@ pub async fn run_command( write_run_config_snapshot(&run_dir, run_cfg.as_ref()).await?; } - // Write RunRecord (replaces spec.json + manifest.json) + // Write RunRecord if !cached_run_restart { let cli_flags = super::create::CliFlags { dry_run: dry_run_flag, diff --git a/lib/crates/fabro-cli/src/main.rs b/lib/crates/fabro-cli/src/main.rs index 03a68554f..e22861b35 100644 --- a/lib/crates/fabro-cli/src/main.rs +++ b/lib/crates/fabro-cli/src/main.rs @@ -84,7 +84,7 @@ enum Command { /// Run ID prefix or workflow name run: String, }, - /// Internal: run the engine process (reads spec.json from run dir) + /// Internal: run the engine process (reads run.json from run dir) #[command(name = "_run_engine", hide = true)] RunEngine { /// Path to the run directory diff --git a/lib/crates/fabro-cli/tests/cli.rs b/lib/crates/fabro-cli/tests/cli.rs index 562b9ab64..085f5cd3f 100644 --- a/lib/crates/fabro-cli/tests/cli.rs +++ b/lib/crates/fabro-cli/tests/cli.rs @@ -545,7 +545,7 @@ fn run_help_no_longer_shows_resume_or_run_branch() { // == Bug regression: create/start/attach lifecycle ============================ /// Helper: create a minimal run directory that `resolve_run` can find. -/// Sets up manifest.json, status.json, spec.json, and progress.jsonl. +/// Sets up run.json, status.json, and progress.jsonl. fn setup_run_dir( home: &std::path::Path, run_id: &str, @@ -555,68 +555,41 @@ fn setup_run_dir( let run_dir = home.join(".fabro").join("runs").join(run_id); std::fs::create_dir_all(&run_dir).unwrap(); - // manifest.json for resolve_run - let manifest = serde_json::json!({ - "run_id": run_id, - "workflow_name": "test", - "goal": "", - "start_time": "2026-01-01T00:00:00Z", - "node_count": 1, - "edge_count": 0 - }); - std::fs::write( - run_dir.join("manifest.json"), - serde_json::to_string(&manifest).unwrap(), - ) - .unwrap(); - - // Merge spec defaults with overrides - let mut spec = serde_json::json!({ - "run_id": run_id, - "workflow_path": "/tmp/test.fabro", - "dot_source": "digraph { start -> exit }", - "working_directory": "/tmp", - "goal": null, - "model": "test-model", - "provider": null, - "sandbox_provider": "local", - "labels": {}, - "verbose": false, - "no_retro": true, - - "preserve_sandbox": false, - "dry_run": true, - "auto_approve": true - }); - if let (Some(base), Some(overrides)) = (spec.as_object_mut(), spec_overrides.as_object()) { - for (k, v) in overrides { - base.insert(k.clone(), v.clone()); - } - } - std::fs::write( - run_dir.join("spec.json"), - serde_json::to_string(&spec).unwrap(), - ) - .unwrap(); + // Build defaults, then merge overrides + let overrides = spec_overrides; + let get_str = |key: &str, default: &str| -> serde_json::Value { + overrides + .get(key) + .and_then(|v| v.as_str()) + .map(|s| serde_json::json!(s)) + .unwrap_or_else(|| serde_json::json!(default)) + }; + let get_bool = |key: &str, default: bool| -> serde_json::Value { + overrides + .get(key) + .and_then(|v| v.as_bool()) + .map(|b| serde_json::json!(b)) + .unwrap_or_else(|| serde_json::json!(default)) + }; // run.json (RunRecord) for resolve_run and run_engine_entrypoint let run_record = serde_json::json!({ "run_id": run_id, "created_at": "2026-01-01T00:00:00Z", "config": { - "goal": spec.get("goal").and_then(|v| v.as_str()).map(String::from), + "goal": overrides.get("goal").and_then(|v| v.as_str()), "llm": { - "model": spec.get("model").and_then(|v| v.as_str()), - "provider": spec.get("provider").and_then(|v| v.as_str()) + "model": get_str("model", "test-model"), + "provider": overrides.get("provider").and_then(|v| v.as_str()) }, "sandbox": { - "provider": spec.get("sandbox_provider").and_then(|v| v.as_str()), - "preserve": spec.get("preserve_sandbox").and_then(|v| v.as_bool()) + "provider": get_str("sandbox_provider", "local"), + "preserve": get_bool("preserve_sandbox", false) }, - "verbose": spec.get("verbose").and_then(|v| v.as_bool()), - "dry_run": spec.get("dry_run").and_then(|v| v.as_bool()), - "auto_approve": spec.get("auto_approve").and_then(|v| v.as_bool()), - "no_retro": spec.get("no_retro").and_then(|v| v.as_bool()) + "verbose": get_bool("verbose", false), + "dry_run": get_bool("dry_run", true), + "auto_approve": get_bool("auto_approve", true), + "no_retro": get_bool("no_retro", true) }, "graph": { "name": "test", @@ -624,8 +597,8 @@ fn setup_run_dir( "edges": [], "attrs": {} }, - "working_directory": spec.get("working_directory").and_then(|v| v.as_str()).unwrap_or("/tmp"), - "labels": spec.get("labels").cloned().unwrap_or(serde_json::json!({})) + "working_directory": overrides.get("working_directory").and_then(|v| v.as_str()).unwrap_or("/tmp"), + "labels": overrides.get("labels").cloned().unwrap_or(serde_json::json!({})) }); std::fs::write( run_dir.join("run.json"), @@ -716,11 +689,10 @@ digraph BarBaz { .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")); + let run_record: serde_json::Value = + serde_json::from_str(&std::fs::read_to_string(run_dir.join("run.json")).unwrap()).unwrap(); + assert_eq!(run_record["graph"]["name"].as_str(), Some("BarBaz")); + assert_eq!(run_record["workflow_slug"].as_str(), Some("sluggy")); } #[test] @@ -767,11 +739,10 @@ digraph FooWorkflow { .success(); let run_dir = find_run_dir(home.path(), "opaque-run-alpha"); - 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("FooWorkflow")); - assert_eq!(manifest["workflow_slug"].as_str(), Some("alpha")); + let run_record: serde_json::Value = + serde_json::from_str(&std::fs::read_to_string(run_dir.join("run.json")).unwrap()).unwrap(); + assert_eq!(run_record["graph"]["name"].as_str(), Some("FooWorkflow")); + assert_eq!(run_record["workflow_slug"].as_str(), Some("alpha")); } #[test] @@ -927,14 +898,18 @@ fn start_by_workflow_name_prefers_newly_created_submitted_run() { 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"), + old_run_dir.join("run.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 + "created_at": "2026-01-01T00:00:00Z", + "config": {}, + "graph": { + "name": "Smoke", + "nodes": {}, + "edges": [], + "attrs": {} + }, + "working_directory": "/tmp" }) .to_string(), ) @@ -982,7 +957,7 @@ fn start_by_workflow_name_prefers_newly_created_submitted_run() { ); } -// Bug 2: _run_engine should use cached graph.fabro, not spec.workflow_path. +// Bug 2: _run_engine should use cached graph.fabro, not run.json working_directory. // When the original workflow file is deleted between create and start, // the engine should read the snapshot saved at create time. #[test] @@ -998,27 +973,32 @@ digraph G { start -> exit }"; - // spec.json: workflow_path points to a file that no longer exists - let spec = serde_json::json!({ + // run.json: working_directory is valid but original workflow path no longer exists + let run_record = serde_json::json!({ "run_id": "test-bug2", - "workflow_path": "/nonexistent/deleted-workflow.fabro", - "dot_source": dot, + "created_at": "2026-01-01T00:00:00Z", + "config": { + "dry_run": true, + "auto_approve": true, + "no_retro": true, + "llm": { + "model": "test-model" + }, + "sandbox": { + "provider": "local" + } + }, + "graph": { + "name": "G", + "nodes": {}, + "edges": [], + "attrs": {} + }, "working_directory": run_dir.to_str().unwrap(), - "goal": null, - "model": "test-model", - "provider": null, - "sandbox_provider": "local", - "labels": {}, - "verbose": false, - "no_retro": true, - - "preserve_sandbox": false, - "dry_run": true, - "auto_approve": true }); std::fs::write( - run_dir.join("spec.json"), - serde_json::to_string(&spec).unwrap(), + run_dir.join("run.json"), + serde_json::to_string(&run_record).unwrap(), ) .unwrap(); @@ -1026,7 +1006,6 @@ digraph G { std::fs::write(run_dir.join("graph.fabro"), dot).unwrap(); // _run_engine should use graph.fabro and never reference the deleted file. - // Bug: it reads spec.workflow_path → fails with file-not-found. let output = arc() .args(["_run_engine", "--run-dir", run_dir.to_str().unwrap()]) .env("NO_COLOR", "1") @@ -1178,8 +1157,8 @@ fn attach_closed_stdin_keeps_interview_pending() { ); } -// Bug 4: attach should respect the verbose flag from spec.json. -// Currently ProgressUI is created with verbose=false regardless of spec. +// Bug 4: attach should respect the verbose flag from run.json. +// Currently ProgressUI is created with verbose=false regardless of config. #[test] fn bug4_attach_respects_verbose_from_spec() { let home = tempfile::tempdir().unwrap(); diff --git a/lib/crates/fabro-cli/tests/scenario.rs b/lib/crates/fabro-cli/tests/scenario.rs index 842ea57f7..e0e7fded0 100644 --- a/lib/crates/fabro-cli/tests/scenario.rs +++ b/lib/crates/fabro-cli/tests/scenario.rs @@ -378,13 +378,6 @@ fn scenario_full_stack(sandbox: &str) { "run record should have graph.name" ); - // Manifest should still be written (legacy) - let manifest = read_json(&run_dir.join("manifest.json")); - assert!( - manifest["run_id"].as_str().is_some(), - "manifest should have run_id" - ); - // Progress events assert!( has_event(&run_dir, "WorkflowRunStarted"), diff --git a/lib/crates/fabro-workflows/src/core_adapter/lifecycle/disk.rs b/lib/crates/fabro-workflows/src/core_adapter/lifecycle/disk.rs index 20243b74a..013ea38b7 100644 --- a/lib/crates/fabro-workflows/src/core_adapter/lifecycle/disk.rs +++ b/lib/crates/fabro-workflows/src/core_adapter/lifecycle/disk.rs @@ -37,8 +37,7 @@ impl RunLifecycle for DiskLifecycle { _graph: &WorkflowGraph, _state: &WfRunState, ) -> fabro_core::error::Result<()> { - // Write manifest.json (legacy) and start.json - engine::write_manifest(&self.run_dir, &self.graph, &self.config); + // Write start.json engine::write_start_record(&self.run_dir, &self.config); // Write run status as Running crate::run_status::write_run_status( diff --git a/lib/crates/fabro-workflows/src/core_adapter/lifecycle/git.rs b/lib/crates/fabro-workflows/src/core_adapter/lifecycle/git.rs index 08f38c438..b1bcbf61c 100644 --- a/lib/crates/fabro-workflows/src/core_adapter/lifecycle/git.rs +++ b/lib/crates/fabro-workflows/src/core_adapter/lifecycle/git.rs @@ -56,13 +56,6 @@ impl RunLifecycle for GitLifecycle { (&self.config.meta_branch, &self.config.host_repo_path) { let store = crate::git::MetadataStore::new(repo_path, &self.config.git_author); - let manifest_bytes = { - let manifest_path = self.run_dir.join("manifest.json"); - std::fs::read(&manifest_path).unwrap_or_default() - }; - let dot_source = std::fs::read(self.run_dir.join("graph.fabro")) - .or_else(|_| std::fs::read(self.run_dir.join("graph.dot"))) - .unwrap_or_default(); let run_json = std::fs::read(self.run_dir.join("run.json")).ok(); let start_json = std::fs::read(self.run_dir.join("start.json")).ok(); let sandbox_json = std::fs::read(self.run_dir.join("sandbox.json")).ok(); @@ -76,8 +69,7 @@ impl RunLifecycle for GitLifecycle { if let Some(ref data) = sandbox_json { extra_files.push(("sandbox.json", data)); } - if let Err(e) = store.init_run(&self.run_id, &manifest_bytes, &dot_source, &extra_files) - { + if let Err(e) = store.init_run(&self.run_id, &[], &[], &extra_files) { tracing::warn!( run_id = %self.run_id, error = %e, diff --git a/lib/crates/fabro-workflows/src/engine.rs b/lib/crates/fabro-workflows/src/engine.rs index 76b2e3f3c..01c16f756 100644 --- a/lib/crates/fabro-workflows/src/engine.rs +++ b/lib/crates/fabro-workflows/src/engine.rs @@ -226,39 +226,6 @@ pub fn resolve_thread_id( // --- Run directory helpers (spec 5.6) --- -/// Write manifest.json at the start of a workflow run. Returns the manifest. -pub(crate) fn write_manifest( - run_dir: &Path, - graph: &Graph, - config: &RunConfig, -) -> crate::manifest::Manifest { - let workflow_name = if graph.name.is_empty() { - "unnamed".to_string() - } else { - graph.name.clone() - }; - let manifest = crate::manifest::Manifest { - run_id: config.run_id.clone(), - workflow_name, - goal: graph.goal().to_string(), - start_time: Utc::now(), - node_count: graph.nodes.len(), - edge_count: graph.edges.len(), - run_branch: config.run_branch.clone(), - base_sha: config.base_sha.clone(), - labels: config.labels.clone(), - base_branch: config.base_branch.clone(), - workflow_slug: config.workflow_slug.clone(), - host_repo_path: config - .host_repo_path - .as_ref() - .map(|p| p.to_string_lossy().to_string()), - }; - let _ = std::fs::create_dir_all(run_dir); - let _ = manifest.save(&run_dir.join("manifest.json")); - manifest -} - /// Write start.json at the start of a workflow run. Returns the StartRecord. pub(crate) fn write_start_record( run_dir: &Path, @@ -2483,10 +2450,10 @@ mod tests { ); } - // --- manifest.json and node status tests --- + // --- start.json and node status tests --- #[tokio::test] - async fn engine_writes_manifest_json() { + async fn engine_writes_start_json() { let dir = tempfile::tempdir().unwrap(); let g = simple_graph(); let engine = @@ -2499,6 +2466,38 @@ mod tests { git_checkpoint_enabled: false, host_repo_path: None, base_sha: None, + run_branch: Some("fabro/run/test-run".into()), + meta_branch: None, + labels: HashMap::new(), + checkpoint_exclude_globs: Vec::new(), + github_app: None, + git_author: crate::git::GitAuthor::default(), + base_branch: None, + pull_request: None, + asset_globs: Vec::new(), + workflow_slug: None, + }; + engine.run(&g, &config).await.unwrap(); + + let start = crate::start_record::StartRecord::load(dir.path()).unwrap(); + assert_eq!(start.run_id, "test-run"); + assert_eq!(start.run_branch.as_deref(), Some("fabro/run/test-run")); + } + + #[tokio::test] + async fn start_record_includes_base_sha() { + let dir = tempfile::tempdir().unwrap(); + let g = simple_graph(); + let engine = + WorkflowRunEngine::new(make_registry(), Arc::new(EventEmitter::new()), local_env()); + let config = RunConfig { + run_dir: dir.path().to_path_buf(), + cancel_token: None, + dry_run: false, + run_id: "sha-run".into(), + git_checkpoint_enabled: false, + host_repo_path: None, + base_sha: Some("abc123".into()), run_branch: None, meta_branch: None, labels: HashMap::new(), @@ -2512,17 +2511,12 @@ mod tests { }; engine.run(&g, &config).await.unwrap(); - let manifest_path = dir.path().join("manifest.json"); - assert!(manifest_path.exists()); - let manifest = crate::manifest::Manifest::load(&manifest_path).unwrap(); - assert_eq!(manifest.workflow_name, "test_pipeline"); - assert_eq!(manifest.goal, "Run tests"); - assert!(manifest.node_count > 0); - assert!(manifest.edge_count > 0); + let start = crate::start_record::StartRecord::load(dir.path()).unwrap(); + assert_eq!(start.base_sha.as_deref(), Some("abc123")); } #[tokio::test] - async fn manifest_includes_labels_when_present() { + async fn start_record_omits_optional_fields_when_empty() { let dir = tempfile::tempdir().unwrap(); let g = simple_graph(); let engine = @@ -2531,38 +2525,7 @@ mod tests { run_dir: dir.path().to_path_buf(), cancel_token: None, dry_run: false, - run_id: "labels-run".into(), - git_checkpoint_enabled: false, - host_repo_path: None, - base_sha: None, - run_branch: None, - meta_branch: None, - labels: HashMap::from([("env".into(), "test".into())]), - checkpoint_exclude_globs: Vec::new(), - github_app: None, - git_author: crate::git::GitAuthor::default(), - base_branch: None, - pull_request: None, - asset_globs: Vec::new(), - workflow_slug: None, - }; - engine.run(&g, &config).await.unwrap(); - - let manifest = crate::manifest::Manifest::load(&dir.path().join("manifest.json")).unwrap(); - assert_eq!(manifest.labels.get("env").map(String::as_str), Some("test")); - } - - #[tokio::test] - async fn manifest_omits_labels_when_empty() { - let dir = tempfile::tempdir().unwrap(); - let g = simple_graph(); - let engine = - WorkflowRunEngine::new(make_registry(), Arc::new(EventEmitter::new()), local_env()); - let config = RunConfig { - run_dir: dir.path().to_path_buf(), - cancel_token: None, - dry_run: false, - run_id: "no-labels-run".into(), + run_id: "no-optional-run".into(), git_checkpoint_enabled: false, host_repo_path: None, base_sha: None, @@ -2579,8 +2542,9 @@ mod tests { }; engine.run(&g, &config).await.unwrap(); - let manifest = crate::manifest::Manifest::load(&dir.path().join("manifest.json")).unwrap(); - assert!(manifest.labels.is_empty()); + let start = crate::start_record::StartRecord::load(dir.path()).unwrap(); + assert!(start.run_branch.is_none()); + assert!(start.base_sha.is_none()); } #[tokio::test] @@ -2798,10 +2762,10 @@ mod tests { assert_eq!(resolve_thread_id(None, &node, &graph, None), None); } - // --- Gap #15: Manifest goal field test --- + // --- Gap #15: StartRecord run_id field test --- #[tokio::test] - async fn engine_manifest_includes_goal() { + async fn engine_start_record_has_run_id() { let dir = tempfile::tempdir().unwrap(); let g = simple_graph(); let engine = @@ -2827,13 +2791,12 @@ mod tests { }; engine.run(&g, &config).await.unwrap(); - let manifest_path = dir.path().join("manifest.json"); - let manifest = crate::manifest::Manifest::load(&manifest_path).unwrap(); - assert_eq!(manifest.goal, "Run tests"); + let start = crate::start_record::StartRecord::load(dir.path()).unwrap(); + assert_eq!(start.run_id, "test-run"); } #[tokio::test] - async fn engine_manifest_goal_empty_when_unset() { + async fn engine_start_record_run_branch_none_when_unset() { let dir = tempfile::tempdir().unwrap(); let mut g = Graph::new("no_goal"); let mut start = Node::new("start"); @@ -2873,9 +2836,8 @@ mod tests { }; engine.run(&g, &config).await.unwrap(); - let manifest_path = dir.path().join("manifest.json"); - let manifest = crate::manifest::Manifest::load(&manifest_path).unwrap(); - assert_eq!(manifest.goal, ""); + let start = crate::start_record::StartRecord::load(dir.path()).unwrap(); + assert!(start.run_branch.is_none()); } // --- Gap #1: Auto status tests --- diff --git a/lib/crates/fabro-workflows/src/git.rs b/lib/crates/fabro-workflows/src/git.rs index 62aedb6d6..550a985f6 100644 --- a/lib/crates/fabro-workflows/src/git.rs +++ b/lib/crates/fabro-workflows/src/git.rs @@ -526,21 +526,6 @@ impl MetadataStore { } } - /// Read the manifest from the metadata branch. Returns `None` if not found. - pub fn read_manifest( - repo_path: &Path, - run_id: &str, - ) -> Result> { - match Self::read_file(repo_path, run_id, "manifest.json")? { - Some(bytes) => { - let manifest: crate::manifest::Manifest = serde_json::from_slice(&bytes) - .map_err(|e| git_error(format!("manifest deserialize failed: {e}")))?; - Ok(Some(manifest)) - } - None => Ok(None), - } - } - /// Read the run record from the metadata branch. Returns `None` if not found. pub fn read_run_record( repo_path: &Path, @@ -686,15 +671,17 @@ mod tests { init_repo(dir.path()); let store = MetadataStore::new(dir.path(), &GitAuthor::default()); - let manifest = br#"{"run_id":"RUN1","workflow_name":"test","goal":"g","start_time":"2025-01-01T00:00:00Z","node_count":2,"edge_count":1}"#; + let run_record = br#"{"run_id":"RUN1","created_at":"2025-01-01T00:00:00Z","config":{},"graph":{"name":"test","nodes":{},"edges":[],"attrs":{}},"working_directory":"/tmp"}"#; let dot = b"digraph { start -> end }"; - store.init_run("RUN1", manifest, dot, &[]).unwrap(); + store + .init_run("RUN1", &[], dot, &[("run.json", run_record)]) + .unwrap(); - let read_manifest = MetadataStore::read_manifest(dir.path(), "RUN1") + let read_record = MetadataStore::read_run_record(dir.path(), "RUN1") .unwrap() .unwrap(); - assert_eq!(read_manifest.run_id, "RUN1"); - assert_eq!(read_manifest.workflow_name, "test"); + assert_eq!(read_record.run_id, "RUN1"); + assert_eq!(read_record.workflow_name(), "test"); let read_dot = MetadataStore::read_graph_dot(dir.path(), "RUN1") .unwrap() diff --git a/lib/crates/fabro-workflows/src/lib.rs b/lib/crates/fabro-workflows/src/lib.rs index b19ecb97c..ee18d299f 100644 --- a/lib/crates/fabro-workflows/src/lib.rs +++ b/lib/crates/fabro-workflows/src/lib.rs @@ -105,7 +105,6 @@ pub mod event; pub mod git; pub mod graph_render; pub mod handler; -pub mod manifest; pub mod outcome; pub mod pipeline; pub mod preamble; @@ -114,7 +113,6 @@ pub mod run_fork; pub mod run_lookup; pub mod run_record; pub mod run_rewind; -pub mod run_spec; pub mod run_status; pub mod sandbox_provider; pub mod sandbox_reconnect; diff --git a/lib/crates/fabro-workflows/src/manifest.rs b/lib/crates/fabro-workflows/src/manifest.rs deleted file mode 100644 index 0f2035ef4..000000000 --- a/lib/crates/fabro-workflows/src/manifest.rs +++ /dev/null @@ -1,129 +0,0 @@ -use std::collections::HashMap; -use std::path::Path; - -use chrono::{DateTime, Utc}; -use serde::{Deserialize, Serialize}; - -use crate::error::Result; - -#[derive(Debug, Clone, Serialize, Deserialize)] -pub struct Manifest { - pub run_id: String, - pub workflow_name: String, - pub goal: String, - pub start_time: DateTime, - pub node_count: usize, - pub edge_count: usize, - #[serde(default, skip_serializing_if = "Option::is_none")] - pub run_branch: Option, - #[serde(default, skip_serializing_if = "Option::is_none")] - pub base_sha: Option, - #[serde(default, skip_serializing_if = "HashMap::is_empty")] - pub labels: HashMap, - #[serde(default, skip_serializing_if = "Option::is_none")] - pub base_branch: Option, - #[serde(default, skip_serializing_if = "Option::is_none")] - pub workflow_slug: Option, - #[serde(default, skip_serializing_if = "Option::is_none")] - pub host_repo_path: Option, -} - -impl Manifest { - pub fn save(&self, path: &Path) -> Result<()> { - crate::save_json(self, path, "manifest") - } - - pub fn load(path: &Path) -> Result { - crate::load_json(path, "manifest") - } -} - -#[cfg(test)] -mod tests { - use super::*; - - fn sample_manifest() -> Manifest { - Manifest { - run_id: "run-1".to_string(), - workflow_name: "test_pipeline".to_string(), - goal: "Fix the bug".to_string(), - start_time: Utc::now(), - node_count: 3, - edge_count: 2, - run_branch: Some("feature/test".to_string()), - base_sha: Some("abc123".to_string()), - labels: HashMap::from([("env".into(), "test".into())]), - base_branch: None, - workflow_slug: None, - host_repo_path: None, - } - } - - #[test] - fn save_and_load_roundtrip() { - let dir = tempfile::tempdir().unwrap(); - let path = dir.path().join("manifest.json"); - - let manifest = sample_manifest(); - manifest.save(&path).unwrap(); - let loaded = Manifest::load(&path).unwrap(); - - assert_eq!(loaded.run_id, "run-1"); - assert_eq!(loaded.workflow_name, "test_pipeline"); - assert_eq!(loaded.goal, "Fix the bug"); - assert_eq!(loaded.node_count, 3); - assert_eq!(loaded.edge_count, 2); - assert_eq!(loaded.run_branch.as_deref(), Some("feature/test")); - assert_eq!(loaded.base_sha.as_deref(), Some("abc123")); - assert_eq!(loaded.labels.get("env").map(String::as_str), Some("test")); - } - - #[test] - fn load_nonexistent_file() { - let result = Manifest::load(Path::new("/nonexistent/manifest.json")); - assert!(result.is_err()); - } - - #[test] - fn load_invalid_json() { - let dir = tempfile::tempdir().unwrap(); - let path = dir.path().join("bad.json"); - std::fs::write(&path, "not json").unwrap(); - - let result = Manifest::load(&path); - assert!(result.is_err()); - } - - #[test] - fn save_and_load_roundtrip_with_slug() { - let dir = tempfile::tempdir().unwrap(); - let path = dir.path().join("manifest.json"); - - let mut manifest = sample_manifest(); - manifest.workflow_slug = Some("smoke".to_string()); - manifest.save(&path).unwrap(); - let loaded = Manifest::load(&path).unwrap(); - - assert_eq!(loaded.workflow_slug.as_deref(), Some("smoke")); - } - - #[test] - fn labels_omitted_when_empty() { - let dir = tempfile::tempdir().unwrap(); - let path = dir.path().join("manifest.json"); - - let mut manifest = sample_manifest(); - manifest.labels = HashMap::new(); - manifest.run_branch = None; - manifest.base_sha = None; - manifest.save(&path).unwrap(); - - let raw: serde_json::Value = - serde_json::from_str(&std::fs::read_to_string(&path).unwrap()).unwrap(); - assert!(raw.get("labels").is_none()); - assert!(raw.get("run_branch").is_none()); - assert!(raw.get("base_sha").is_none()); - assert!(raw.get("workflow_slug").is_none()); - assert!(raw.get("host_repo_path").is_none()); - } -} diff --git a/lib/crates/fabro-workflows/src/run_fork.rs b/lib/crates/fabro-workflows/src/run_fork.rs index ebafac876..107202b36 100644 --- a/lib/crates/fabro-workflows/src/run_fork.rs +++ b/lib/crates/fabro-workflows/src/run_fork.rs @@ -4,7 +4,6 @@ use fabro_git_storage::gitobj::Store; use git2::{Oid, Signature}; use crate::git::MetadataStore; -use crate::manifest::Manifest; use crate::run_record::RunRecord; use crate::start_record::StartRecord; @@ -50,25 +49,17 @@ pub fn execute_fork( .ensure_branch() .map_err(|e| anyhow::anyhow!("failed to create metadata branch: {e}"))?; - // Read manifest, run record, start record, graph, and sandbox from source + // Read run record, start record, graph, and sandbox from source let source_entries = source_bs - .read_entries(&[ - "manifest.json", - "run.json", - "start.json", - "graph.fabro", - "sandbox.json", - ]) + .read_entries(&["run.json", "start.json", "graph.fabro", "sandbox.json"]) .map_err(|e| anyhow::anyhow!("failed to read source metadata: {e}"))?; - let mut manifest_bytes = None; let mut run_record_bytes = None; let mut start_record_bytes = None; let mut graph_bytes = None; let mut sandbox_bytes = None; for (path, data) in source_entries { match path { - "manifest.json" => manifest_bytes = Some(data), "run.json" => run_record_bytes = Some(data), "start.json" => start_record_bytes = Some(data), "graph.fabro" => graph_bytes = Some(data), @@ -76,32 +67,20 @@ pub fn execute_fork( _ => {} } } - let manifest_bytes = - manifest_bytes.ok_or_else(|| anyhow::anyhow!("source run has no manifest.json"))?; + let run_record_bytes = + run_record_bytes.ok_or_else(|| anyhow::anyhow!("source run has no run.json"))?; let graph_bytes = graph_bytes.ok_or_else(|| anyhow::anyhow!("source run has no graph.fabro"))?; let now = chrono::Utc::now(); - // Update legacy manifest - let mut manifest: Manifest = - serde_json::from_slice(&manifest_bytes).context("failed to parse source manifest.json")?; - manifest.run_id = new_run_id.clone(); - manifest.run_branch = Some(new_run_branch.clone()); - manifest.start_time = now; - let new_manifest_bytes = - serde_json::to_vec_pretty(&manifest).context("failed to serialize new manifest")?; - // Create new RunRecord for the forked run - let new_run_record_bytes = if let Some(ref rr_bytes) = run_record_bytes { - let mut run_record: RunRecord = - serde_json::from_slice(rr_bytes).context("failed to parse source run.json")?; - run_record.run_id = new_run_id.clone(); - run_record.created_at = now; - Some(serde_json::to_vec_pretty(&run_record).context("failed to serialize new run.json")?) - } else { - None - }; + let mut run_record: RunRecord = + serde_json::from_slice(&run_record_bytes).context("failed to parse source run.json")?; + run_record.run_id = new_run_id.clone(); + run_record.created_at = now; + let new_run_record_bytes = + serde_json::to_vec_pretty(&run_record).context("failed to serialize new run.json")?; // Create new StartRecord for the forked run let new_start_record_bytes = if start_record_bytes.is_some() { @@ -132,13 +111,10 @@ pub fn execute_fork( // Write all entries to the new metadata branch in a single commit let mut file_entries: Vec<(&str, &[u8])> = vec![ - ("manifest.json", &new_manifest_bytes), + ("run.json", &new_run_record_bytes), ("graph.fabro", &graph_bytes), ("checkpoint.json", &checkpoint_bytes), ]; - if let Some(ref run_record) = new_run_record_bytes { - file_entries.push(("run.json", run_record)); - } if let Some(ref start_record) = new_start_record_bytes { file_entries.push(("start.json", start_record)); } @@ -218,18 +194,6 @@ mod tests { serde_json::to_vec(&cp).unwrap() } - fn make_manifest_json(run_id: &str) -> Vec { - let manifest = serde_json::json!({ - "run_id": run_id, - "workflow_name": "test_workflow", - "goal": "Test goal", - "start_time": "2025-01-01T00:00:00Z", - "node_count": 3, - "edge_count": 2, - }); - serde_json::to_vec_pretty(&manifest).unwrap() - } - fn make_run_record_json(run_id: &str) -> Vec { let record = serde_json::json!({ "run_id": run_id, @@ -296,14 +260,12 @@ mod tests { let bs = BranchStore::new(store, &meta_branch, &sig); bs.ensure_branch().unwrap(); - // Write manifest, run record, start record, and graph - let manifest = make_manifest_json(run_id); + // Write run record, start record, and graph let run_record = make_run_record_json(run_id); let start_record = make_start_record_json(run_id); let graph = b"digraph { start -> build -> test }"; bs.write_entries( &[ - ("manifest.json", &manifest), ("run.json", &run_record), ("start.json", &start_record), ("graph.fabro", graph), @@ -354,15 +316,6 @@ mod tests { let sig = test_sig(); let bs = BranchStore::new(&store, &new_meta_branch, &sig); - // Check manifest has new run_id - let manifest_bytes = bs.read_entry("manifest.json").unwrap().unwrap(); - let manifest: Manifest = serde_json::from_slice(&manifest_bytes).unwrap(); - assert_eq!(manifest.run_id, new_run_id); - assert_eq!( - manifest.run_branch.as_deref(), - Some(format!("{}{new_run_id}", crate::git::RUN_BRANCH_PREFIX).as_str()) - ); - // Check RunRecord has new run_id and updated created_at let rr_bytes = bs.read_entry("run.json").unwrap().unwrap(); let run_record: RunRecord = serde_json::from_slice(&rr_bytes).unwrap(); diff --git a/lib/crates/fabro-workflows/src/run_rewind.rs b/lib/crates/fabro-workflows/src/run_rewind.rs index b215cca4a..9ab39eb54 100644 --- a/lib/crates/fabro-workflows/src/run_rewind.rs +++ b/lib/crates/fabro-workflows/src/run_rewind.rs @@ -468,7 +468,7 @@ mod tests { bs.ensure_branch().unwrap(); // init commit (should be skipped) - bs.write_entry("manifest.json", b"{}", "init run").unwrap(); + bs.write_entry("run.json", b"{}", "init run").unwrap(); // 3 checkpoint commits let cp1 = make_checkpoint_json("start", 1, Some("aaa")); @@ -502,7 +502,7 @@ mod tests { let bs = BranchStore::new(&store, &branch, &sig); bs.ensure_branch().unwrap(); - bs.write_entry("manifest.json", b"{}", "init run").unwrap(); + bs.write_entry("run.json", b"{}", "init run").unwrap(); let cp1 = make_checkpoint_json("start", 1, None); bs.write_entry("checkpoint.json", &cp1, "checkpoint") @@ -705,7 +705,7 @@ mod tests { let bs = BranchStore::new(&store, &branch, &sig); bs.ensure_branch().unwrap(); - bs.write_entry("manifest.json", b"{}", "init run").unwrap(); + bs.write_entry("run.json", b"{}", "init run").unwrap(); let cp1 = make_checkpoint_json("start", 1, None); let oid1 = bs @@ -751,9 +751,7 @@ mod tests { let meta_branch = MetadataStore::branch_name("run-2"); let meta_bs = BranchStore::new(&store, &meta_branch, &sig); meta_bs.ensure_branch().unwrap(); - meta_bs - .write_entry("manifest.json", b"{}", "init run") - .unwrap(); + meta_bs.write_entry("run.json", b"{}", "init run").unwrap(); let cp1 = make_checkpoint_json("start", 1, Some(&run_c1.to_string())); meta_bs @@ -783,7 +781,7 @@ mod tests { let bs = BranchStore::new(&store, &branch, &sig); bs.ensure_branch().unwrap(); - bs.write_entry("manifest.json", b"{}", "init run").unwrap(); + bs.write_entry("run.json", b"{}", "init run").unwrap(); let cp1 = make_checkpoint_json("start", 1, None); let oid1 = bs diff --git a/lib/crates/fabro-workflows/src/run_spec.rs b/lib/crates/fabro-workflows/src/run_spec.rs deleted file mode 100644 index a71041c11..000000000 --- a/lib/crates/fabro-workflows/src/run_spec.rs +++ /dev/null @@ -1,84 +0,0 @@ -use std::collections::HashMap; -use std::path::{Path, PathBuf}; - -use serde::{Deserialize, Serialize}; - -#[derive(Debug, Clone, Default, PartialEq, Serialize, Deserialize)] -#[serde(default)] -pub struct RunSpec { - pub run_id: String, - pub workflow_path: PathBuf, - pub dot_source: String, - pub working_directory: PathBuf, - pub goal: Option, - pub model: String, - pub provider: Option, - pub sandbox_provider: String, - pub labels: HashMap, - pub verbose: bool, - pub no_retro: bool, - pub preserve_sandbox: bool, - pub dry_run: bool, - pub auto_approve: bool, -} - -impl RunSpec { - pub fn save(&self, run_dir: &Path) -> anyhow::Result<()> { - let path = run_dir.join("spec.json"); - let json = serde_json::to_string_pretty(self)?; - std::fs::write(path, json)?; - Ok(()) - } - - pub fn load(run_dir: &Path) -> anyhow::Result { - let path = run_dir.join("spec.json"); - let json = std::fs::read_to_string(path)?; - let spec = serde_json::from_str(&json)?; - Ok(spec) - } -} - -#[cfg(test)] -mod tests { - use super::*; - - fn sample_spec() -> RunSpec { - let mut labels = HashMap::new(); - labels.insert("env".to_string(), "test".to_string()); - labels.insert("team".to_string(), "platform".to_string()); - - RunSpec { - run_id: "run-abc123".to_string(), - workflow_path: PathBuf::from("/home/user/workflows/deploy/workflow.toml"), - dot_source: "digraph { a -> b }".to_string(), - working_directory: PathBuf::from("/home/user/project"), - goal: Some("Deploy to staging".to_string()), - model: "claude-sonnet-4-20250514".to_string(), - provider: Some("anthropic".to_string()), - sandbox_provider: "local".to_string(), - labels, - verbose: true, - no_retro: false, - preserve_sandbox: false, - dry_run: false, - auto_approve: true, - } - } - - #[test] - fn save_load_roundtrip() { - let dir = tempfile::tempdir().unwrap(); - let spec = sample_spec(); - - spec.save(dir.path()).unwrap(); - let loaded = RunSpec::load(dir.path()).unwrap(); - - assert_eq!(loaded, spec); - } - - #[test] - fn load_nonexistent() { - let dir = PathBuf::from("/tmp/nonexistent-run-spec-dir-that-does-not-exist"); - assert!(RunSpec::load(&dir).is_err()); - } -} diff --git a/lib/crates/fabro-workflows/tests/integration.rs b/lib/crates/fabro-workflows/tests/integration.rs index b6b7ebfc2..b3fcc4198 100644 --- a/lib/crates/fabro-workflows/tests/integration.rs +++ b/lib/crates/fabro-workflows/tests/integration.rs @@ -11044,6 +11044,19 @@ async fn git_checkpoint_host_writes_shadow_branch() { let run_dir = tempfile::tempdir().unwrap(); // Write graph.fabro so init_run can read it std::fs::write(run_dir.path().join("graph.fabro"), "digraph {}").unwrap(); + // Write run.json so init_run stores it on the metadata branch + let run_record_json = serde_json::json!({ + "run_id": run_id, + "created_at": "2025-01-01T00:00:00Z", + "config": {}, + "graph": { "name": "ShadowBranchTest", "nodes": {}, "edges": [], "attrs": {} }, + "working_directory": worktree_path.to_str().unwrap(), + }); + std::fs::write( + run_dir.path().join("run.json"), + serde_json::to_string(&run_record_json).unwrap(), + ) + .unwrap(); let emitter = EventEmitter::new(); let env: Arc = @@ -11115,10 +11128,10 @@ async fn git_checkpoint_host_writes_shadow_branch() { ); // 8. Verify round-trip: shadow checkpoint's completed_nodes matches expected - let manifest = MetadataStore::read_manifest(repo.path(), run_id) - .expect("read_manifest should not error") - .expect("shadow branch should contain manifest"); - assert_eq!(manifest.run_id, run_id); + let run_record = MetadataStore::read_run_record(repo.path(), run_id) + .expect("read_run_record should not error") + .expect("shadow branch should contain run record"); + assert_eq!(run_record.run_id, run_id); // Cleanup worktree let _ = std::process::Command::new("git")