From 0577de38f914ce9f49cc807a34ded463e664bbac Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Fri, 3 Apr 2026 16:16:53 -0700 Subject: [PATCH] refactor(run): remove vestigial run dir params --- lib/crates/fabro-store/src/run_state.rs | 17 +--- lib/crates/fabro-store/src/slate/run_store.rs | 18 ---- lib/crates/fabro-store/src/types.rs | 2 +- .../src/pipeline/execute/tests.rs | 8 +- .../fabro-workflow/src/pipeline/finalize.rs | 20 +--- lib/crates/fabro-workflow/src/pipeline/mod.rs | 4 +- .../src/pipeline/pull_request.rs | 97 +++---------------- 7 files changed, 27 insertions(+), 139 deletions(-) diff --git a/lib/crates/fabro-store/src/run_state.rs b/lib/crates/fabro-store/src/run_state.rs index fa601cf41..02d50c039 100644 --- a/lib/crates/fabro-store/src/run_state.rs +++ b/lib/crates/fabro-store/src/run_state.rs @@ -56,7 +56,7 @@ pub(crate) struct EventProjectionCache { } impl RunState { - pub fn apply_events(events: &[EventEnvelope]) -> Result { + pub(crate) fn apply_events(events: &[EventEnvelope]) -> Result { let mut state = Self::default(); for event in events { state.apply_event(event)?; @@ -64,7 +64,7 @@ impl RunState { Ok(state) } - pub fn apply_event(&mut self, event: &EventEnvelope) -> Result<()> { + pub(crate) fn apply_event(&mut self, event: &EventEnvelope) -> Result<()> { let value = event.payload.as_value(); let ts = parse_ts(value)?; let event_name = value @@ -284,17 +284,6 @@ impl RunState { self.nodes.get(&(node.node_id.to_string(), node.visit)) } - pub fn list_node_ids(&self) -> Vec { - let mut ids = self - .nodes - .keys() - .map(|(node_id, _)| node_id.clone()) - .collect::>(); - ids.sort(); - ids.dedup(); - ids - } - pub fn list_node_visits(&self, node_id: &str) -> Vec { let mut visits = self .nodes @@ -307,7 +296,7 @@ impl RunState { visits } - pub fn build_summary(&self, catalog: &CatalogRecord) -> RunSummary { + pub(crate) fn build_summary(&self, catalog: &CatalogRecord) -> RunSummary { let workflow_name = self.run.as_ref().map(|run| { if run.graph.name.is_empty() { "unnamed".to_string() diff --git a/lib/crates/fabro-store/src/slate/run_store.rs b/lib/crates/fabro-store/src/slate/run_store.rs index bc9df6b3c..73ddd1160 100644 --- a/lib/crates/fabro-store/src/slate/run_store.rs +++ b/lib/crates/fabro-store/src/slate/run_store.rs @@ -184,10 +184,6 @@ impl SlateRunStore { self.inner.db.list_events_from(1).await } - pub async fn list_events_from(&self, seq: u32) -> Result> { - self.inner.db.list_events_from(seq).await - } - pub fn watch_events_from( &self, seq: u32, @@ -271,20 +267,6 @@ impl SlateRunStore { .await } - pub async fn list_assets(&self, node: &NodeVisitRef<'_>) -> Result> { - let prefix = format!("{}/", keys::node_asset_prefix(node)); - let mut iter = self.inner.db.scan_prefix(prefix.as_bytes()).await?; - let mut assets = Vec::new(); - while let Some(entry) = iter.next().await? { - let key = key_to_string(&entry.key)?; - if let Some(asset) = key.strip_prefix(&prefix) { - assets.push(asset.to_string()); - } - } - assets.sort(); - Ok(assets) - } - pub async fn list_all_assets(&self) -> Result> { self.inner.db.list_all_assets().await } diff --git a/lib/crates/fabro-store/src/types.rs b/lib/crates/fabro-store/src/types.rs index 840b15661..ed06f1fa3 100644 --- a/lib/crates/fabro-store/src/types.rs +++ b/lib/crates/fabro-store/src/types.rs @@ -49,7 +49,7 @@ impl EventPayload { Ok(payload) } - pub fn validate(&self, expected_run_id: &RunId) -> Result<()> { + pub(crate) fn validate(&self, expected_run_id: &RunId) -> Result<()> { let obj = self.0.as_object().ok_or_else(|| { StoreError::InvalidEvent("event payload must be a JSON object".into()) })?; diff --git a/lib/crates/fabro-workflow/src/pipeline/execute/tests.rs b/lib/crates/fabro-workflow/src/pipeline/execute/tests.rs index b8e2350a6..2e01e37f2 100644 --- a/lib/crates/fabro-workflow/src/pipeline/execute/tests.rs +++ b/lib/crates/fabro-workflow/src/pipeline/execute/tests.rs @@ -157,7 +157,7 @@ fn test_lifecycle(setup_commands: Vec) -> LifecycleOptions { } } -async fn test_run_store(_run_dir: &Path, run_id: &RunId) -> fabro_store::RunStoreHandle { +async fn test_run_store(run_id: &RunId) -> fabro_store::SlateRunStore { let store: StoreHandle = Arc::new(SlateStore::new( Arc::new(InMemory::new()), "", @@ -180,7 +180,7 @@ async fn execute_test_run_with_options( ) -> Executed { let run_id_value = run_options.run_id; let git_options = run_options.git.clone(); - let run_store = test_run_store(&run_options.run_dir, &run_id_value).await; + let run_store = test_run_store(&run_id_value).await; let emitter = test_emitter_arc("test-run"); let store_logger = StoreProgressLogger::new(run_store.clone()); store_logger.register(&emitter); @@ -241,7 +241,7 @@ async fn execute_runs_start_to_exit_and_returns_final_context() { persisted_workflow(graph, source, &run_dir, test_run_id("run-test")), InitOptions { run_id: test_run_id("run-test"), - run_store: test_run_store(&run_dir, &test_run_id("run-test")).await, + run_store: test_run_store(&test_run_id("run-test")).await, dry_run: false, emitter: test_emitter_arc("run-test"), sandbox: SandboxSpec::Local { @@ -308,7 +308,7 @@ async fn run_with_lifecycle( persisted_workflow(graph.clone(), String::new(), &run_dir, run_id), InitOptions { run_id, - run_store: test_run_store(&run_dir, &run_id).await, + run_store: test_run_store(&run_id).await, dry_run: false, emitter, sandbox: SandboxSpec::Local { diff --git a/lib/crates/fabro-workflow/src/pipeline/finalize.rs b/lib/crates/fabro-workflow/src/pipeline/finalize.rs index e1477c833..0d97664c8 100644 --- a/lib/crates/fabro-workflow/src/pipeline/finalize.rs +++ b/lib/crates/fabro-workflow/src/pipeline/finalize.rs @@ -1,4 +1,3 @@ -use std::path::Path; use std::sync::Arc; use crate::error::FabroError; @@ -166,24 +165,11 @@ fn build_conclusion_from_parts( } } -pub fn persist_terminal_outcome( - _run_dir: &Path, - conclusion: &Conclusion, - run_status: RunStatus, - status_reason: Option, -) { - let _ = (conclusion, run_status, status_reason); -} - /// Write a finalize commit to the shadow branch with retro.json and final node files. /// /// This captures the last diff.patch (written after the final checkpoint) and retro.json. /// Best-effort: errors are logged as warnings. -pub async fn write_finalize_commit( - run_options: &RunOptions, - _run_dir: &Path, - run_store: &SlateRunStore, -) { +pub async fn write_finalize_commit(run_options: &RunOptions, run_store: &SlateRunStore) { let (Some(meta_branch), Some(repo_path)) = ( run_options .git @@ -271,7 +257,7 @@ pub async fn finalize( let (final_status, failure_reason, _run_status, _status_reason) = classify_engine_result(&outcome); let conclusion = build_conclusion_from_store( - options.run_store.as_ref(), + &options.run_store, final_status, failure_reason, duration_ms, @@ -279,7 +265,7 @@ pub async fn finalize( ) .await; - write_finalize_commit(&run_options, &options.run_dir, options.run_store.as_ref()).await; + write_finalize_commit(&run_options, &options.run_store).await; if options.preserve_sandbox { let info = sandbox.sandbox_info(); diff --git a/lib/crates/fabro-workflow/src/pipeline/mod.rs b/lib/crates/fabro-workflow/src/pipeline/mod.rs index 302a2b546..4adcf20f8 100644 --- a/lib/crates/fabro-workflow/src/pipeline/mod.rs +++ b/lib/crates/fabro-workflow/src/pipeline/mod.rs @@ -12,9 +12,7 @@ mod validate; pub use execute::execute; pub use fabro_types::PullRequestRecord; pub(crate) use finalize::build_conclusion_from_store; -pub use finalize::{ - classify_engine_result, finalize, persist_terminal_outcome, write_finalize_commit, -}; +pub use finalize::{classify_engine_result, finalize, write_finalize_commit}; pub use initialize::initialize; pub use parse::parse; pub(crate) use persist::persist; diff --git a/lib/crates/fabro-workflow/src/pipeline/pull_request.rs b/lib/crates/fabro-workflow/src/pipeline/pull_request.rs index 9c2ad65d3..54c5ee42d 100644 --- a/lib/crates/fabro-workflow/src/pipeline/pull_request.rs +++ b/lib/crates/fabro-workflow/src/pipeline/pull_request.rs @@ -1,5 +1,3 @@ -use std::path::Path; - use fabro_config::run::MergeStrategy; use fabro_store::{RunState, SlateRunStore}; use fabro_types::PullRequestRecord; @@ -201,29 +199,6 @@ fn parse_dot_summary(dot: &str) -> (String, usize, usize) { } } -#[cfg(test)] -fn read_dot_source(run_dir: &Path) -> Option { - let workflow_fabro_path = run_dir.join("workflow.fabro"); - if let Ok(content) = std::fs::read_to_string(&workflow_fabro_path) { - debug!(path = %workflow_fabro_path.display(), "Read workflow graph for PR body"); - return Some(content); - } - - let legacy_fabro_path = run_dir.join("graph.fabro"); - if let Ok(content) = std::fs::read_to_string(&legacy_fabro_path) { - debug!(path = %legacy_fabro_path.display(), "Read workflow graph for PR body (legacy)"); - return Some(content); - } - let dot_path = run_dir.join("graph.dot"); - match std::fs::read_to_string(&dot_path) { - Ok(content) => { - debug!(path = %dot_path.display(), "Read workflow graph for PR body (dot fallback)"); - Some(content) - } - Err(_) => None, - } -} - /// Read plan text from the first `plan*` node response in run state. /// /// Nodes are sorted alphabetically so `plan` is preferred over `planning`. @@ -306,8 +281,7 @@ fn emit_run_notice( }); } -async fn load_pull_request_diff(run_store: &SlateRunStore, run_dir: &Path) -> String { - let _ = run_dir; +async fn load_pull_request_diff(run_store: &SlateRunStore) -> String { run_store .state() .await @@ -326,7 +300,6 @@ pub async fn build_pr_body( goal: &str, model: &str, run_store: &SlateRunStore, - _run_dir: &Path, conclusion: Option<&Conclusion>, ) -> Result { debug!("Building PR body"); @@ -435,7 +408,6 @@ pub async fn maybe_open_pull_request( draft: bool, auto_merge: Option, run_store: &SlateRunStore, - run_dir: &Path, conclusion: Option<&Conclusion>, ) -> Result, String> { if diff.is_empty() { @@ -446,7 +418,7 @@ pub async fn maybe_open_pull_request( let https_url = ssh_url_to_https(origin_url); let (owner, repo) = github_app::parse_github_owner_repo(&https_url)?; - let body = build_pr_body(diff, goal, model, run_store, run_dir, conclusion).await?; + let body = build_pr_body(diff, goal, model, run_store, conclusion).await?; let body = truncate_pr_body(&body); let title = pr_title_from_goal(goal); @@ -533,8 +505,7 @@ pub async fn pull_request(concluded: Concluded, options: &PullRequestOptions) -> result.status, StageStatus::Success | StageStatus::PartialSuccess ) { - let diff = - load_pull_request_diff(options.run_store.as_ref(), &options.run_dir).await; + let diff = load_pull_request_diff(&options.run_store).await; if let (Some(base_branch), Some(run_branch), Some(creds), Some(origin)) = ( &run_options.base_branch, pushed_branch.as_deref(), @@ -559,8 +530,7 @@ pub async fn pull_request(concluded: Concluded, options: &PullRequestOptions) -> &options.model, pr_cfg.draft, auto_merge, - options.run_store.as_ref(), - &options.run_dir, + &options.run_store, Some(&conclusion), ) .await @@ -1103,8 +1073,7 @@ mod tests { "diff --git a/src/lib.rs b/src/lib.rs\n+fn new_feature() {}\n", "Implement feature", "mock-model", - run_store.as_ref(), - tmp.path(), + &run_store, Some(&conclusion), ) .await @@ -1144,7 +1113,7 @@ mod tests { labels: HashMap::new(), }; append_workflow_event( - run_store.as_ref(), + &run_store, &fixtures::RUN_1, &WorkflowRunEvent::RunCreated { run_id: fixtures::RUN_1, @@ -1164,7 +1133,7 @@ mod tests { .await .unwrap(); append_workflow_event( - run_store.as_ref(), + &run_store, &fixtures::RUN_1, &WorkflowRunEvent::RetroCompleted { duration_ms: 1, @@ -1180,8 +1149,7 @@ mod tests { "diff --git a/src/lib.rs b/src/lib.rs\n+fn new_feature() {}\n", "Implement feature", "mock-model", - run_store.as_ref(), - tmp.path(), + &run_store, Some(&conclusion), ) .await @@ -1221,7 +1189,7 @@ mod tests { labels: HashMap::new(), }; append_workflow_event( - run_store.as_ref(), + &run_store, &fixtures::RUN_1, &WorkflowRunEvent::RunCreated { run_id: fixtures::RUN_1, @@ -1241,7 +1209,7 @@ mod tests { .await .unwrap(); append_workflow_event( - run_store.as_ref(), + &run_store, &fixtures::RUN_1, &WorkflowRunEvent::StageCompleted { node_id: "plan".to_string(), @@ -1273,8 +1241,7 @@ mod tests { "diff --git a/src/lib.rs b/src/lib.rs\n+fn new_feature() {}\n", "Implement feature", "mock-model", - run_store.as_ref(), - tmp.path(), + &run_store, Some(&make_test_conclusion()), ) .await @@ -1324,39 +1291,6 @@ mod tests { assert_eq!(format_duration_ms(0), "0s"); } - // ── read_dot_source tests ─────────────────────────────────────────── - - #[test] - fn read_dot_source_found() { - let tmp = tempfile::tempdir().unwrap(); - std::fs::write(tmp.path().join("workflow.fabro"), "digraph test {}").unwrap(); - let result = read_dot_source(tmp.path()); - assert_eq!(result, Some("digraph test {}".to_string())); - } - - #[test] - fn read_dot_source_legacy_fabro_fallback() { - let tmp = tempfile::tempdir().unwrap(); - std::fs::write(tmp.path().join("graph.fabro"), "digraph legacy {}").unwrap(); - let result = read_dot_source(tmp.path()); - assert_eq!(result, Some("digraph legacy {}".to_string())); - } - - #[test] - fn read_dot_source_dot_fallback() { - let tmp = tempfile::tempdir().unwrap(); - std::fs::write(tmp.path().join("graph.dot"), "digraph old {}").unwrap(); - let result = read_dot_source(tmp.path()); - assert_eq!(result, Some("digraph old {}".to_string())); - } - - #[test] - fn read_dot_source_not_found() { - let tmp = tempfile::tempdir().unwrap(); - let result = read_dot_source(tmp.path()); - assert_eq!(result, None); - } - // ── Existing tests ───────────────────────────────────────────────── #[test] @@ -1458,8 +1392,7 @@ mod tests { "claude-sonnet-4-20250514", false, None, - run_store.as_ref(), - tmp.path(), + &run_store, None, ) .await; @@ -1492,7 +1425,7 @@ mod tests { labels: std::collections::HashMap::new(), }; append_workflow_event( - run_store.as_ref(), + &run_store, &fixtures::RUN_1, &WorkflowRunEvent::RunCreated { run_id: fixtures::RUN_1, @@ -1512,7 +1445,7 @@ mod tests { .await .unwrap(); append_workflow_event( - run_store.as_ref(), + &run_store, &fixtures::RUN_1, &WorkflowRunEvent::WorkflowRunCompleted { duration_ms: 1, @@ -1530,7 +1463,7 @@ mod tests { .await .unwrap(); - let diff = load_pull_request_diff(run_store.as_ref(), tmp.path()).await; + let diff = load_pull_request_diff(&run_store).await; assert!(diff.contains("from_store")); }