From 498242f4f78e38dd7eb9b21e4583d7f62ea989a2 Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Sun, 26 Apr 2026 19:16:46 -0400 Subject: [PATCH] refactor(sandbox): drop dead helpers and tidy clone-based code - Delete unused `detect_clone_params` and `GitCloneParams` (the clone-based refactor sources clone params from the run spec, not the worker cwd). - Add `DaytonaSandbox::repo_cloned()` accessor mirroring Docker; replace five inline `OnceCell` reads. - Inline `sanitize_origin_url` one-liner wrapper in `manifest_builder`. - Drop unused `pub` on `docker::WORKING_DIRECTORY`. - Convert `cleanup` early-return to `let-else` and remove a `Some(...).expect(...)` round-trip in `decide_clone`. Co-Authored-By: Claude Opus 4.7 (1M context) --- lib/crates/fabro-cli/src/manifest_builder.rs | 6 +--- lib/crates/fabro-sandbox/src/clone_source.rs | 3 +- lib/crates/fabro-sandbox/src/daytona/mod.rs | 35 +++++--------------- lib/crates/fabro-sandbox/src/docker.rs | 6 ++-- lib/crates/fabro-sandbox/src/lib.rs | 2 -- 5 files changed, 13 insertions(+), 39 deletions(-) diff --git a/lib/crates/fabro-cli/src/manifest_builder.rs b/lib/crates/fabro-cli/src/manifest_builder.rs index 9d2aebc4e..f486c628c 100644 --- a/lib/crates/fabro-cli/src/manifest_builder.rs +++ b/lib/crates/fabro-cli/src/manifest_builder.rs @@ -500,15 +500,11 @@ fn build_manifest_git(repo_path: &Path) -> Option { Some(types::ManifestGit { branch, clean, - origin_url: sanitize_origin_url(&origin_url), + origin_url: fabro_github::normalize_repo_origin_url(&origin_url), sha, }) } -fn sanitize_origin_url(origin_url: &str) -> String { - fabro_github::normalize_repo_origin_url(origin_url) -} - fn normalize_absolute_path(base_dir: &Path, reference: &str) -> Option { let path = Path::new(reference); if path.is_absolute() || reference.starts_with('~') { diff --git a/lib/crates/fabro-sandbox/src/clone_source.rs b/lib/crates/fabro-sandbox/src/clone_source.rs index 7618d4605..5e20da362 100644 --- a/lib/crates/fabro-sandbox/src/clone_source.rs +++ b/lib/crates/fabro-sandbox/src/clone_source.rs @@ -43,8 +43,7 @@ pub(crate) fn decide_clone( }); }; - let origin_url = clean_clone_origin_for_record(Some(origin_url)) - .expect("origin URL is present after filter"); + let origin_url = fabro_github::normalize_repo_origin_url(origin_url); if let Err(err) = fabro_github::parse_github_owner_repo(&origin_url) { return Err(format!( "Clone-based sandboxes currently support GitHub repository origins only: {err}" diff --git a/lib/crates/fabro-sandbox/src/daytona/mod.rs b/lib/crates/fabro-sandbox/src/daytona/mod.rs index a212983ce..68db9f568 100644 --- a/lib/crates/fabro-sandbox/src/daytona/mod.rs +++ b/lib/crates/fabro-sandbox/src/daytona/mod.rs @@ -206,6 +206,10 @@ impl DaytonaSandbox { .ok_or_else(|| "Daytona sandbox not initialized — call initialize() first".to_string()) } + fn repo_cloned(&self) -> bool { + self.repo_cloned.get().copied().unwrap_or(false) + } + /// Build `SandboxBaseParams` from config, generating a unique sandbox name. fn base_params(&self) -> daytona_sdk::SandboxBaseParams { let name = if let Some(ref id) = self.run_id { @@ -341,27 +345,6 @@ impl DaytonaSandbox { } } -/// Parameters for cloning a git repo into the sandbox during initialization. -#[derive(Clone, Debug)] -pub struct GitCloneParams { - /// Clean HTTPS URL (no embedded credentials). - pub url: String, - /// Branch to clone. If None, uses the remote's default. - pub branch: Option, -} - -pub fn detect_clone_params(cwd: &Path) -> Option { - let (detected_url, branch) = match detect_repo_info(cwd) { - Ok(info) => info, - Err(err) => { - tracing::warn!("No git repo detected for sandbox clone: {err}"); - return None; - } - }; - let url = fabro_github::ssh_url_to_https(&detected_url); - Some(GitCloneParams { url, branch }) -} - /// Detect the git remote URL and current branch from a local repository. /// /// Uses `git2` to discover the repo at `path`, reads the `origin` remote URL @@ -780,14 +763,14 @@ impl Sandbox for DaytonaSandbox { } async fn setup_git_for_run(&self, run_id: &str) -> Result, String> { - if !self.repo_cloned.get().copied().unwrap_or(false) { + if !self.repo_cloned() { return Ok(None); } crate::setup_git_via_exec(self, run_id).await.map(Some) } fn resume_setup_commands(&self, run_branch: &str) -> Vec { - if !self.repo_cloned.get().copied().unwrap_or(false) { + if !self.repo_cloned() { return Vec::new(); } vec![format!( @@ -798,7 +781,7 @@ impl Sandbox for DaytonaSandbox { } async fn git_push_branch(&self, branch: &str) -> bool { - if !self.repo_cloned.get().copied().unwrap_or(false) { + if !self.repo_cloned() { return false; } crate::git_push_via_exec(self, branch).await @@ -825,7 +808,7 @@ impl Sandbox for DaytonaSandbox { } fn origin_url(&self) -> Option<&str> { - if !self.repo_cloned.get().copied().unwrap_or(false) { + if !self.repo_cloned() { return None; } self.origin_url.get().map(String::as_str) @@ -852,7 +835,7 @@ impl Sandbox for DaytonaSandbox { } async fn refresh_push_credentials(&self) -> Result<(), String> { - if !self.repo_cloned.get().copied().unwrap_or(false) { + if !self.repo_cloned() { return Ok(()); } let Some(origin_url) = self.origin_url.get() else { diff --git a/lib/crates/fabro-sandbox/src/docker.rs b/lib/crates/fabro-sandbox/src/docker.rs index 8881f4329..200f16ae9 100644 --- a/lib/crates/fabro-sandbox/src/docker.rs +++ b/lib/crates/fabro-sandbox/src/docker.rs @@ -28,7 +28,7 @@ use crate::{ format_lines_numbered, shell_quote, }; -pub const WORKING_DIRECTORY: &str = "/workspace"; +const WORKING_DIRECTORY: &str = "/workspace"; const MANAGED_LABEL: &str = "sh.fabro.managed"; const RUN_ID_LABEL: &str = "sh.fabro.run_id"; @@ -899,9 +899,7 @@ impl Sandbox for DockerSandbox { }); let start = Instant::now(); - let container_id = if let Some(id) = self.container_id.get() { - id.clone() - } else { + let Some(container_id) = self.container_id.get().cloned() else { let duration_ms = u64::try_from(start.elapsed().as_millis()).unwrap_or(u64::MAX); self.emit(SandboxEvent::CleanupCompleted { provider: "docker".into(), diff --git a/lib/crates/fabro-sandbox/src/lib.rs b/lib/crates/fabro-sandbox/src/lib.rs index 38c3379e5..d6b9528da 100644 --- a/lib/crates/fabro-sandbox/src/lib.rs +++ b/lib/crates/fabro-sandbox/src/lib.rs @@ -26,8 +26,6 @@ pub mod daytona; #[cfg(any(test, feature = "test-support"))] pub mod test_support; -#[cfg(feature = "daytona")] -pub use daytona::detect_clone_params; #[cfg(feature = "docker")] pub use docker::{DockerSandbox, DockerSandboxOptions}; pub use local::LocalSandbox;