From 79e48ddaea413980f1195474c2ccbb685b7ffcc7 Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Fri, 3 Apr 2026 15:52:49 -0700 Subject: [PATCH] Fix clippy warnings in fabro-workflow - run_dump: take &Path instead of PathBuf by value in path helpers - test_support: remove empty no-op persist_run_artifacts_for_tests - agent.rs: use u32::try_from instead of as u32 cast - retro.rs: remove unnecessary let binding - pull_request.rs: use NodeState::default() instead of Default::default() - execute/tests.rs: replace bool::then in filter_map with filter+map Co-Authored-By: Claude Opus 4.6 (1M context) --- .../fabro-workflow/src/handler/agent.rs | 3 +- .../src/pipeline/execute/tests.rs | 15 ++++------ .../src/pipeline/pull_request.rs | 7 +++-- .../fabro-workflow/src/pipeline/retro.rs | 5 ++-- lib/crates/fabro-workflow/src/run_dump.rs | 28 +++++++++---------- lib/crates/fabro-workflow/src/test_support.rs | 7 +---- 6 files changed, 29 insertions(+), 36 deletions(-) diff --git a/lib/crates/fabro-workflow/src/handler/agent.rs b/lib/crates/fabro-workflow/src/handler/agent.rs index 888d6de27..08a47b4d7 100644 --- a/lib/crates/fabro-workflow/src/handler/agent.rs +++ b/lib/crates/fabro-workflow/src/handler/agent.rs @@ -673,7 +673,8 @@ mod tests { ) -> Result { emitter.emit(&crate::event::WorkflowRunEvent::Agent { stage: node.id.clone(), - visit: crate::run_dir::visit_from_context(context) as u32, + visit: u32::try_from(crate::run_dir::visit_from_context(context)) + .unwrap_or(u32::MAX), event: fabro_agent::AgentEvent::SessionStarted { provider: Some("openai".to_string()), model: Some("gpt-5.4".to_string()), diff --git a/lib/crates/fabro-workflow/src/pipeline/execute/tests.rs b/lib/crates/fabro-workflow/src/pipeline/execute/tests.rs index 467662e67..b8e2350a6 100644 --- a/lib/crates/fabro-workflow/src/pipeline/execute/tests.rs +++ b/lib/crates/fabro-workflow/src/pipeline/execute/tests.rs @@ -936,10 +936,8 @@ async fn retry_emits_stage_started_per_attempt() { let collected = events.lock().unwrap(); let work_started: Vec<_> = collected .iter() - .filter_map(|event| { - (event.event == "stage.started" && event.node_id.as_deref() == Some("work")) - .then(|| event.properties["attempt"].as_u64().unwrap()) - }) + .filter(|event| event.event == "stage.started" && event.node_id.as_deref() == Some("work")) + .map(|event| event.properties["attempt"].as_u64().unwrap()) .collect(); assert_eq!(work_started, vec![1, 2]); } @@ -1060,16 +1058,15 @@ async fn git_checkpoint_skips_start_node() { let collected = events.lock().unwrap(); let checkpoint_node_ids: Vec<&str> = collected .iter() - .filter_map(|event| { - (event.event == "checkpoint.completed" + .filter(|event| { + event.event == "checkpoint.completed" && event .properties .get("git_commit_sha") .and_then(|value| value.as_str()) - .is_some()) - .then(|| event.node_id.as_deref()) - .flatten() + .is_some() }) + .filter_map(|event| event.node_id.as_deref()) .collect(); assert!(!checkpoint_node_ids.contains(&"start")); assert!(checkpoint_node_ids.contains(&"work")); diff --git a/lib/crates/fabro-workflow/src/pipeline/pull_request.rs b/lib/crates/fabro-workflow/src/pipeline/pull_request.rs index ad7477de7..9c2ad65d3 100644 --- a/lib/crates/fabro-workflow/src/pipeline/pull_request.rs +++ b/lib/crates/fabro-workflow/src/pipeline/pull_request.rs @@ -990,9 +990,10 @@ mod tests { #[test] fn read_plan_text_not_found() { let mut state = RunState::default(); - state - .nodes - .insert(("implement".to_string(), 1), Default::default()); + state.nodes.insert( + ("implement".to_string(), 1), + fabro_store::NodeState::default(), + ); let result = read_plan_text(&state); assert_eq!(result, None); diff --git a/lib/crates/fabro-workflow/src/pipeline/retro.rs b/lib/crates/fabro-workflow/src/pipeline/retro.rs index bbf3ff239..f0df0cb23 100644 --- a/lib/crates/fabro-workflow/src/pipeline/retro.rs +++ b/lib/crates/fabro-workflow/src/pipeline/retro.rs @@ -191,7 +191,7 @@ mod tests { context.set("response.work", serde_json::json!("done")); let mut outcomes = HashMap::new(); outcomes.insert("work".to_string(), crate::outcome::Outcome::success()); - let checkpoint = Checkpoint::from_context( + Checkpoint::from_context( &context, "work", vec!["work".to_string()], @@ -201,8 +201,7 @@ mod tests { HashMap::new(), HashMap::new(), HashMap::new(), - ); - checkpoint + ) } fn test_store() -> Arc { diff --git a/lib/crates/fabro-workflow/src/run_dump.rs b/lib/crates/fabro-workflow/src/run_dump.rs index d80452931..e730b64fd 100644 --- a/lib/crates/fabro-workflow/src/run_dump.rs +++ b/lib/crates/fabro-workflow/src/run_dump.rs @@ -66,7 +66,7 @@ impl RunDump { if let Some(status) = node.status.as_ref() { push_json_entry_path( &mut entries, - metadata_node_file_path(node_id, *visit, "status.json").into(), + &PathBuf::from(metadata_node_file_path(node_id, *visit, "status.json")), status, ); } @@ -153,28 +153,28 @@ impl RunDump { if let Some(prompt) = node.prompt.as_ref() { entries.push(RunDumpEntry::text_path( - base.join("prompt.md"), + &base.join("prompt.md"), prompt.clone(), )); } if let Some(response) = node.response.as_ref() { entries.push(RunDumpEntry::text_path( - base.join("response.md"), + &base.join("response.md"), response.clone(), )); } if let Some(status) = node.status.as_ref() { - push_json_entry_path(&mut entries, base.join("status.json"), status); + push_json_entry_path(&mut entries, &base.join("status.json"), status); } if let Some(stdout) = node.stdout.as_ref() { entries.push(RunDumpEntry::text_path( - base.join("stdout.log"), + &base.join("stdout.log"), stdout.clone(), )); } if let Some(stderr) = node.stderr.as_ref() { entries.push(RunDumpEntry::text_path( - base.join("stderr.log"), + &base.join("stderr.log"), stderr.clone(), )); } @@ -197,7 +197,7 @@ impl RunDump { for (seq, checkpoint) in &state.checkpoints { push_json_entry_path( &mut entries, - PathBuf::from("checkpoints").join(format!("{seq:04}.json")), + &PathBuf::from("checkpoints").join(format!("{seq:04}.json")), checkpoint, ); } @@ -211,7 +211,7 @@ impl RunDump { format!("artifact value {artifact_id:?} is missing from the store") })?; entries.push(RunDumpEntry::json_path( - PathBuf::from("artifacts") + &PathBuf::from("artifacts") .join("values") .join(format!("{}.json", artifact_id_segment.display())), value, @@ -234,7 +234,7 @@ impl RunDump { ) })?; entries.push(RunDumpEntry::bytes_path( - PathBuf::from("artifacts") + &PathBuf::from("artifacts") .join("nodes") .join(node_id_segment) .join(format!("visit-{visit}")) @@ -293,7 +293,7 @@ impl RunDumpEntry { } } - fn text_path(path: PathBuf, contents: String) -> Self { + fn text_path(path: &Path, contents: String) -> Self { Self { path: path_to_string(path), contents: RunDumpContents::Text(contents), @@ -307,7 +307,7 @@ impl RunDumpEntry { } } - fn json_path(path: PathBuf, contents: serde_json::Value) -> Self { + fn json_path(path: &Path, contents: serde_json::Value) -> Self { Self { path: path_to_string(path), contents: RunDumpContents::Json(contents), @@ -321,7 +321,7 @@ impl RunDumpEntry { } } - fn bytes_path(path: PathBuf, contents: Vec) -> Self { + fn bytes_path(path: &Path, contents: Vec) -> Self { Self { path: path_to_string(path), contents: RunDumpContents::Bytes(contents), @@ -357,7 +357,7 @@ where } } -fn push_json_entry_path(entries: &mut Vec, path: PathBuf, value: &T) +fn push_json_entry_path(entries: &mut Vec, path: &Path, value: &T) where T: serde::Serialize, { @@ -374,7 +374,7 @@ fn metadata_node_file_path(node_id: &str, visit: u32, filename: &str) -> String } } -fn path_to_string(path: PathBuf) -> String { +fn path_to_string(path: &Path) -> String { path.to_string_lossy().into_owned() } diff --git a/lib/crates/fabro-workflow/src/test_support.rs b/lib/crates/fabro-workflow/src/test_support.rs index 449120291..44386c9af 100644 --- a/lib/crates/fabro-workflow/src/test_support.rs +++ b/lib/crates/fabro-workflow/src/test_support.rs @@ -6,7 +6,7 @@ use std::time::Duration; use chrono::Utc; use fabro_agent::Sandbox; use fabro_graphviz::graph::Graph as GvGraph; -use fabro_store::{SlateRunStore, SlateStore}; +use fabro_store::SlateStore; use object_store::memory::InMemory; use crate::error::Result; @@ -127,7 +127,6 @@ pub async fn run_graph( ) .await; let executed = pipeline::execute(initialized).await; - persist_run_artifacts_for_tests(executed.run_store.as_ref(), &run_options.run_dir).await; executed.outcome } @@ -154,7 +153,6 @@ pub async fn run_graph_with_hooks( ) .await; let executed = pipeline::execute(initialized).await; - persist_run_artifacts_for_tests(executed.run_store.as_ref(), &run_options.run_dir).await; executed.outcome } @@ -180,12 +178,9 @@ pub async fn run_graph_from_checkpoint( ) .await; let executed = pipeline::execute(initialized).await; - persist_run_artifacts_for_tests(executed.run_store.as_ref(), &run_options.run_dir).await; executed.outcome } -async fn persist_run_artifacts_for_tests(_run_store: &SlateRunStore, _run_dir: &std::path::Path) {} - pub struct WorkflowRunner { registry: std::sync::Mutex>, emitter: Arc,