From b29a582d31eae8b16ea60e05e03866fa56ed0231 Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Mon, 6 Apr 2026 15:12:19 -0400 Subject: [PATCH] fix(storage): use Storage/RunScratch accessors instead of raw path joins MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four callsites were bypassing existing Storage/RunScratch methods: 1. install.rs — .join("secrets.json") → Storage::secrets_path() 2. initialize.rs — .join("worktree") → RunScratch::worktree_dir() 3. git.rs — .join("final.patch") → RunScratch::final_patch() 4. create.rs — duplicated date-format logic → RunScratch::for_run() Adds RunScratch::for_run(scratch_dir, run_id) to centralize the date-prefixed directory name, used by both Storage::run_scratch() and make_run_dir(). Co-Authored-By: Claude Opus 4.6 (1M context) --- lib/crates/fabro-cli/src/commands/install.rs | 5 +++-- lib/crates/fabro-config/src/storage.rs | 13 ++++++++----- lib/crates/fabro-workflow/src/lifecycle/git.rs | 5 ++++- lib/crates/fabro-workflow/src/operations/create.rs | 8 ++++---- .../fabro-workflow/src/pipeline/initialize.rs | 5 +++-- 5 files changed, 22 insertions(+), 14 deletions(-) diff --git a/lib/crates/fabro-cli/src/commands/install.rs b/lib/crates/fabro-cli/src/commands/install.rs index 2b6e12bc6..39f3d6a4b 100644 --- a/lib/crates/fabro-cli/src/commands/install.rs +++ b/lib/crates/fabro-cli/src/commands/install.rs @@ -13,6 +13,7 @@ use dialoguer::console::Term; use dialoguer::theme::ColorfulTheme; use dialoguer::{MultiSelect, Select}; use fabro_api::types::SetSecretRequest; +use fabro_config::Storage; use fabro_config::legacy_env; use fabro_config::user::SETTINGS_CONFIG_FILENAME; use fabro_model::Provider; @@ -556,7 +557,7 @@ async fn persist_install_secrets( return Ok(()); } - let mut store = SecretStore::load(storage_dir.join("secrets.json"))?; + let mut store = SecretStore::load(Storage::new(storage_dir).secrets_path())?; for (name, value) in secrets { store.set(name, value)?; } @@ -840,7 +841,7 @@ pub(crate) async fn run_install(args: &InstallArgs, globals: &GlobalArgs) -> Res " {} Saved {} secrets to {}", s.green.apply_to("✔"), secret_pairs.len(), - storage_dir.join("secrets.json").display() + Storage::new(&storage_dir).secrets_path().display() ); if server_was_running { eprintln!( diff --git a/lib/crates/fabro-config/src/storage.rs b/lib/crates/fabro-config/src/storage.rs index fe6067792..2838031cf 100644 --- a/lib/crates/fabro-config/src/storage.rs +++ b/lib/crates/fabro-config/src/storage.rs @@ -46,11 +46,7 @@ impl Storage { #[must_use] pub fn run_scratch(&self, run_id: &RunId) -> RunScratch { - let local_dt = run_id.created_at().with_timezone(&Local); - RunScratch::new( - self.scratch_dir() - .join(format!("{}-{run_id}", local_dt.format("%Y%m%d"))), - ) + RunScratch::for_run(&self.scratch_dir(), run_id) } #[must_use] @@ -92,6 +88,13 @@ impl RunScratch { Self { root: root.into() } } + /// Create a `RunScratch` for a given run under a scratch directory. + #[must_use] + pub fn for_run(scratch_dir: &Path, run_id: &RunId) -> Self { + let local_dt = run_id.created_at().with_timezone(&Local); + Self::new(scratch_dir.join(format!("{}-{run_id}", local_dt.format("%Y%m%d")))) + } + #[must_use] pub fn root(&self) -> &Path { &self.root diff --git a/lib/crates/fabro-workflow/src/lifecycle/git.rs b/lib/crates/fabro-workflow/src/lifecycle/git.rs index aecdbe28c..9ccaa4fc1 100644 --- a/lib/crates/fabro-workflow/src/lifecycle/git.rs +++ b/lib/crates/fabro-workflow/src/lifecycle/git.rs @@ -3,6 +3,7 @@ use std::path::PathBuf; use std::sync::{Arc, Mutex}; use async_trait::async_trait; +use fabro_config::RunScratch; use fabro_store::RunDatabase; use fabro_types::RunId; use tokio::fs; @@ -311,7 +312,9 @@ impl RunLifecycle for GitLifecycle { match git_diff(&*self.sandbox, &base_sha).await { Ok(patch) if !patch.is_empty() => { *self.final_patch.lock().unwrap() = Some(patch.clone()); - if let Err(err) = fs::write(self.run_dir.join("final.patch"), patch).await { + if let Err(err) = + fs::write(RunScratch::new(&self.run_dir).final_patch(), patch).await + { self.emitter.emit(&Event::RunNotice { level: RunNoticeLevel::Warn, code: "final_patch_write_failed".to_string(), diff --git a/lib/crates/fabro-workflow/src/operations/create.rs b/lib/crates/fabro-workflow/src/operations/create.rs index 6744a807d..f1d0fc955 100644 --- a/lib/crates/fabro-workflow/src/operations/create.rs +++ b/lib/crates/fabro-workflow/src/operations/create.rs @@ -1,4 +1,3 @@ -use chrono::Local; use fabro_config::Storage; use fabro_graphviz::graph::{AttrValue, Graph}; use fabro_model::{Catalog, Provider}; @@ -393,14 +392,15 @@ pub(crate) fn default_run_dir(run_id: &RunId) -> PathBuf { } pub fn make_run_dir(scratch_base: &Path, run_id: &RunId) -> PathBuf { - let local_dt = run_id.created_at().with_timezone(&Local); - scratch_base.join(format!("{}-{run_id}", local_dt.format("%Y%m%d"))) + fabro_config::RunScratch::for_run(scratch_base, run_id) + .root() + .to_path_buf() } #[cfg(test)] mod tests { use super::*; - use chrono::{TimeZone, Utc}; + use chrono::{Local, TimeZone, Utc}; use fabro_graphviz::graph::AttrValue; use fabro_store::Database; use fabro_types::fixtures; diff --git a/lib/crates/fabro-workflow/src/pipeline/initialize.rs b/lib/crates/fabro-workflow/src/pipeline/initialize.rs index 9ee43ffcf..2800960c6 100644 --- a/lib/crates/fabro-workflow/src/pipeline/initialize.rs +++ b/lib/crates/fabro-workflow/src/pipeline/initialize.rs @@ -4,6 +4,7 @@ use std::sync::Arc; use std::time::Instant; use fabro_agent::Sandbox; +use fabro_config::RunScratch; use fabro_graphviz::graph; use fabro_hooks::{HookContext, HookDecision, HookEvent, HookRunner}; use fabro_llm::client::Client; @@ -74,7 +75,7 @@ async fn resolve_worktree_plan( return Ok(Some(WorktreePlan { branch_name: run_branch.clone(), base_sha: base_sha.clone(), - worktree_path: options.run_options.run_dir.join("worktree"), + worktree_path: RunScratch::new(&options.run_options.run_dir).worktree_dir(), skip_branch_creation: true, })); } @@ -173,7 +174,7 @@ async fn resolve_worktree_plan( Ok(Some(WorktreePlan { branch_name: format!("{}{}", git::RUN_BRANCH_PREFIX, options.run_id), base_sha, - worktree_path: options.run_options.run_dir.join("worktree"), + worktree_path: RunScratch::new(&options.run_options.run_dir).worktree_dir(), skip_branch_creation: false, })) }