From 09f5bb0f84f11a3174edd7ba8eca436d676e3e5a Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Fri, 21 Aug 2026 18:37:39 -0400 Subject: [PATCH] Simplify clone depth plumbing Make RunCloneSettings::DEFAULT_DEPTH the single owner of the default depth, and interpret the "0 = full history" sentinel in one place via RunCloneSettings::depth_limit(). Docker's clone_depth becomes Option to match Daytona's encoding, with a shared depth_argument() helper for both git command builders. Drop the unreachable Option on the resolved depth field, the hand-written DaytonaSettings::Default, and the pure-forwarding daytona_git_clone_options helper. The blob-import test helper reuses the pool's own connect options instead of rebuilding a partial copy. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_019bgXj5J218RXfiT72qhbLV --- .../fabro-sandbox/src/clone_source.rs | 20 +++---- lib/components/fabro-sandbox/src/config.rs | 17 ++---- .../fabro-sandbox/src/daytona/mod.rs | 54 ++++--------------- lib/components/fabro-sandbox/src/docker.rs | 24 ++++----- .../fabro-sandbox/src/from_environment.rs | 7 ++- .../fabro-store/src/legacy_blob_import.rs | 9 ++-- .../fabro-workflow/src/operations/start.rs | 6 +-- .../fabro-config/src/resolve/run.rs | 26 ++++----- .../fabro-config/src/tests/resolve_run.rs | 6 +-- .../fabro-types/src/settings/run.rs | 22 ++++---- 10 files changed, 67 insertions(+), 124 deletions(-) diff --git a/lib/components/fabro-sandbox/src/clone_source.rs b/lib/components/fabro-sandbox/src/clone_source.rs index 964084eb2..9f03a93e3 100644 --- a/lib/components/fabro-sandbox/src/clone_source.rs +++ b/lib/components/fabro-sandbox/src/clone_source.rs @@ -91,13 +91,9 @@ pub(crate) fn exact_fetch_command( checkout_path: &str, fetch_source: &str, commit_sha: &str, - depth: usize, + depth: Option, ) -> String { - let depth_arg = if depth == 0 { - String::new() - } else { - format!(" --depth {depth}") - }; + let depth_arg = depth_argument(depth); format!( "{git} -C {} fetch{depth_arg} --no-tags {} -- {}", sandbox::shell_quote(checkout_path), @@ -107,6 +103,12 @@ pub(crate) fn exact_fetch_command( ) } +/// Leading-space ` --depth N` fragment for a Git command, or empty when +/// `depth` is `None` to fetch full history. +pub(crate) fn depth_argument(depth: Option) -> String { + depth.map_or_else(String::new, |depth| format!(" --depth {depth}")) +} + /// Point the admitted branch at `revision` and attach HEAD to it. /// /// The checkout attaches to a real branch instead of detaching so callers that @@ -479,7 +481,7 @@ mod tests { "/repos/acme's widgets", "https://token@example.com/acme/widgets.git?x=a b", sha, - 10, + Some(10), ); let checkout = exact_checkout_verify_command("/repos/acme's widgets", "feature/a b", "FETCH_HEAD"); @@ -505,7 +507,7 @@ mod tests { "/repos/acme/widgets", "origin", "0123456789abcdef0123456789abcdef01234567", - 0, + None, ), "git -c maintenance.auto=0 -c gc.auto=0 -C /repos/acme/widgets fetch --no-tags origin -- 0123456789abcdef0123456789abcdef01234567" ); @@ -572,7 +574,7 @@ mod tests { ); run_shell( temp.path(), - &exact_fetch_command(checkout_path, remote_path, &admitted_sha, 10), + &exact_fetch_command(checkout_path, remote_path, &admitted_sha, Some(10)), ); let checked_out_sha = run_shell( temp.path(), diff --git a/lib/components/fabro-sandbox/src/config.rs b/lib/components/fabro-sandbox/src/config.rs index bddc7c369..208278d18 100644 --- a/lib/components/fabro-sandbox/src/config.rs +++ b/lib/components/fabro-sandbox/src/config.rs @@ -12,30 +12,19 @@ use std::collections::HashMap; use serde::de::{self, MapAccess, Visitor}; use serde::{Deserialize, Serialize}; -#[derive(Clone, Debug, Deserialize, PartialEq, Serialize)] +#[derive(Clone, Debug, Default, Deserialize, PartialEq, Serialize)] pub struct DaytonaSettings { pub auto_stop_interval: Option, pub labels: Option>, pub snapshot: Option, pub network: Option, + /// Git history depth for the repository clone; `None` clones full + /// history. pub clone_depth: Option, #[serde(default)] pub skip_clone: bool, } -impl Default for DaytonaSettings { - fn default() -> Self { - Self { - auto_stop_interval: None, - labels: None, - snapshot: None, - network: None, - clone_depth: Some(100), - skip_clone: false, - } - } -} - #[derive(Clone, Debug, PartialEq)] pub enum DaytonaNetwork { Block, diff --git a/lib/components/fabro-sandbox/src/daytona/mod.rs b/lib/components/fabro-sandbox/src/daytona/mod.rs index 1d1d8da23..0081a05ce 100644 --- a/lib/components/fabro-sandbox/src/daytona/mod.rs +++ b/lib/components/fabro-sandbox/src/daytona/mod.rs @@ -101,23 +101,6 @@ const DAYTONA_STATE_CHANGE_POLL_INTERVAL: Duration = Duration::from_secs(1); /// leaked by a dead worker. An explicit `0` disables auto-stop entirely. const DEFAULT_AUTO_STOP_INTERVAL_MINUTES: i32 = 120; -fn daytona_git_clone_options( - branch: Option, - commit_id: Option, - username: Option, - password: Option, - depth: Option, -) -> GitCloneOptions { - GitCloneOptions { - branch, - commit_id, - username, - password, - depth, - ..GitCloneOptions::default() - } -} - pub(crate) fn daytona_not_found(err: &DaytonaError) -> bool { matches!(err, DaytonaError::NotFound { .. }) || err.status_code() == Some(404) } @@ -1623,13 +1606,14 @@ impl Sandbox for DaytonaSandbox { let git_svc = &git_svc; let origin = origin_url.as_str(); let target = layout.primary_repo_path.as_str(); - let options = daytona_git_clone_options( - branch.clone(), - commit_sha.clone(), - username.clone(), - password.clone(), - self.config.clone_depth, - ); + let options = GitCloneOptions { + branch: branch.clone(), + commit_id: commit_sha.clone(), + username: username.clone(), + password: password.clone(), + depth: self.config.clone_depth, + ..GitCloneOptions::default() + }; async move { git_svc.clone(origin, target, options).await } }, |err: &DaytonaError| classify_clone_failure(err, clone_credential_context), @@ -3142,26 +3126,6 @@ mod tests { assert!(!error.to_string().contains("Daytona client")); } - #[test] - fn exact_checkout_uses_daytona_branch_and_commit_options() { - let options = daytona_git_clone_options( - Some("feature/work".to_string()), - Some("0123456789abcdef0123456789abcdef01234567".to_string()), - Some("x-access-token".to_string()), - Some("secret".to_string()), - Some(1), - ); - - assert_eq!(options.branch.as_deref(), Some("feature/work")); - assert_eq!( - options.commit_id.as_deref(), - Some("0123456789abcdef0123456789abcdef01234567") - ); - assert_eq!(options.username.as_deref(), Some("x-access-token")); - assert_eq!(options.password.as_deref(), Some("secret")); - assert_eq!(options.depth, Some(1)); - } - fn mock_sandbox_body(sandbox_id: &str) -> serde_json::Value { serde_json::json!({ "id": sandbox_id, @@ -3528,7 +3492,7 @@ mod tests { assert!(config.snapshot.is_none()); assert!(config.auto_stop_interval.is_none()); assert!(config.labels.is_none()); - assert_eq!(config.clone_depth, Some(100)); + assert!(config.clone_depth.is_none()); } #[test] diff --git a/lib/components/fabro-sandbox/src/docker.rs b/lib/components/fabro-sandbox/src/docker.rs index 97e965ed8..0585ecbdf 100644 --- a/lib/components/fabro-sandbox/src/docker.rs +++ b/lib/components/fabro-sandbox/src/docker.rs @@ -18,6 +18,7 @@ use bollard::image::CreateImageOptions; use bollard::models::{ContainerInspectResponse, HostConfig}; use fabro_github::GitHubCredentials; use fabro_github::token_source::InstallationTokenSource; +use fabro_types::settings::run::RunCloneSettings; use fabro_types::{CommandOutputStream, CommandTermination, RunId, SandboxProviderKind}; use fabro_util::time::elapsed_ms; use futures::StreamExt; @@ -48,7 +49,7 @@ const DOCKER_BASH_REQUIREMENT: &str = "Docker sandboxes require /bin/bash for ev pub(crate) const WORKING_DIRECTORY: &str = "/workspace"; pub(crate) const REPOS_ROOT: &str = "/repos"; -const DEFAULT_GIT_CLONE_DEPTH: usize = 100; +const DEFAULT_GIT_CLONE_DEPTH: usize = RunCloneSettings::DEFAULT_DEPTH.unsigned_abs() as usize; const GIT_CLONE_TIMEOUT: Duration = Duration::from_mins(5); #[cfg(test)] const EXEC_STOP_POLL_SLEEP_SECONDS: &str = "0.005"; @@ -121,9 +122,9 @@ pub struct DockerSandboxOptions { pub auto_pull: bool, /// Additional `KEY=VALUE` environment variables for the container. pub env_vars: Vec, - /// Maximum Git history depth fetched during clone. Zero fetches full + /// Maximum Git history depth fetched during clone; `None` fetches full /// history. - pub clone_depth: usize, + pub clone_depth: Option, /// Create an empty workspace instead of cloning even when an origin exists. pub skip_clone: bool, } @@ -137,7 +138,7 @@ impl Default for DockerSandboxOptions { cpu_quota: None, auto_pull: true, env_vars: Vec::new(), - clone_depth: DEFAULT_GIT_CLONE_DEPTH, + clone_depth: Some(DEFAULT_GIT_CLONE_DEPTH), skip_clone: false, } } @@ -1541,7 +1542,7 @@ fn git_clone_command( clone_url: &str, branch: Option<&str>, checkout_path: &str, - depth: usize, + depth: Option, ) -> String { let mut command = format!("{} clone", sandbox::GIT); if let Some(branch) = branch { @@ -1549,10 +1550,7 @@ fn git_clone_command( command.push_str(&shell_quote(branch)); command.push_str(" --single-branch"); } - if depth > 0 { - command.push_str(" --depth "); - command.push_str(&depth.to_string()); - } + command.push_str(&clone_source::depth_argument(depth)); command.push_str(" --no-tags"); command.push_str(" -- "); command.push_str(&shell_quote(clone_url)); @@ -2571,7 +2569,7 @@ mod tests { let options = DockerSandboxOptions::default(); assert_eq!(options.image, "buildpack-deps:noble"); assert_eq!(options.network_mode.as_deref(), Some("bridge")); - assert_eq!(options.clone_depth, DEFAULT_GIT_CLONE_DEPTH); + assert_eq!(options.clone_depth, Some(DEFAULT_GIT_CLONE_DEPTH)); assert!(!options.skip_clone); } @@ -2581,7 +2579,7 @@ mod tests { "https://github.com/fabro-sh/fabro", Some("main"), "/repos/fabro-sh/fabro", - 1, + Some(1), ); assert_eq!( command, @@ -2595,7 +2593,7 @@ mod tests { "https://github.com/fabro-sh/fabro", None, "/repos/fabro-sh/fabro", - DEFAULT_GIT_CLONE_DEPTH, + Some(DEFAULT_GIT_CLONE_DEPTH), ); assert_eq!( command, @@ -2609,7 +2607,7 @@ mod tests { "https://github.com/fabro-sh/fabro", Some("main"), "/repos/fabro-sh/fabro", - 0, + None, ); assert_eq!( command, diff --git a/lib/components/fabro-sandbox/src/from_environment.rs b/lib/components/fabro-sandbox/src/from_environment.rs index 4bf76696e..657b4c179 100644 --- a/lib/components/fabro-sandbox/src/from_environment.rs +++ b/lib/components/fabro-sandbox/src/from_environment.rs @@ -61,7 +61,7 @@ pub fn daytona_config_from_environment( DaytonaNetwork::AllowList(settings.network.allow.clone()) } }), - clone_depth: clone.depth.filter(|depth| *depth > 0), + clone_depth: clone.depth_limit(), skip_clone: !clone.enabled, } } @@ -133,9 +133,8 @@ fn docker_config_from_environment_env( .map(|cpu| i64::from(cpu).saturating_mul(100_000)), env_vars, clone_depth: clone - .depth - .and_then(|depth| usize::try_from(depth).ok()) - .unwrap_or(default_options.clone_depth), + .depth_limit() + .and_then(|depth| usize::try_from(depth).ok()), skip_clone: !clone.enabled, ..DockerSandboxOptions::default() } diff --git a/lib/components/fabro-store/src/legacy_blob_import.rs b/lib/components/fabro-store/src/legacy_blob_import.rs index 06199edc2..d9ea4ba93 100644 --- a/lib/components/fabro-store/src/legacy_blob_import.rs +++ b/lib/components/fabro-store/src/legacy_blob_import.rs @@ -790,7 +790,7 @@ mod tests { type TestResult = std::result::Result>; struct TestContext { - dir: tempfile::TempDir, + _dir: tempfile::TempDir, source: Database, source_db: slatedb::Db, sqlite: sqlx::SqlitePool, @@ -812,7 +812,7 @@ mod tests { let sqlite = sqlite.clone_pool(); let target = BlobStore::new(sqlite.clone()); Ok(Self { - dir, + _dir: dir, source, source_db, sqlite, @@ -822,11 +822,8 @@ mod tests { async fn new_with_single_sqlite_connection() -> TestResult { let mut context = Self::new().await?; + let options = context.sqlite.connect_options().as_ref().clone(); context.sqlite.close().await; - let options = SqliteConnectOptions::new() - .filename(context.dir.path().join("fabro.sqlite3")) - .foreign_keys(true) - .journal_mode(SqliteJournalMode::Wal); let sqlite = SqlitePoolOptions::new() .max_connections(1) .connect_with(options) diff --git a/lib/components/fabro-workflow/src/operations/start.rs b/lib/components/fabro-workflow/src/operations/start.rs index 9955f1a8d..053548123 100644 --- a/lib/components/fabro-workflow/src/operations/start.rs +++ b/lib/components/fabro-workflow/src/operations/start.rs @@ -1301,7 +1301,7 @@ reasoning = false resolve_docker_config(&settings.run, |_| None) .unwrap() .clone_depth, - 1 + Some(1) ); } @@ -1320,7 +1320,7 @@ reasoning = false resolve_docker_config(&settings.run, |_| None) .unwrap() .clone_depth, - 0 + None ); } @@ -1333,7 +1333,7 @@ reasoning = false resolve_docker_config(&settings.run, |_| None) .unwrap() .clone_depth, - 100 + Some(100) ); } diff --git a/lib/foundation/fabro-config/src/resolve/run.rs b/lib/foundation/fabro-config/src/resolve/run.rs index ba64fa67d..a60bb9b9f 100644 --- a/lib/foundation/fabro-config/src/resolve/run.rs +++ b/lib/foundation/fabro-config/src/resolve/run.rs @@ -248,24 +248,20 @@ fn resolve_clone( clone: Option<&RunCloneLayer>, errors: &mut Vec, ) -> RunCloneSettings { - let depth = - clone - .and_then(|clone| clone.depth) - .map_or(RunCloneSettings::DEFAULT_DEPTH, |depth| { - if depth < 0 { - errors.push(ResolveError::Invalid { - path: "run.clone.depth".to_string(), - reason: "depth must be at least 0".to_string(), - }); - RunCloneSettings::DEFAULT_DEPTH - } else { - depth - } - }); + let mut depth = clone + .and_then(|clone| clone.depth) + .unwrap_or(RunCloneSettings::DEFAULT_DEPTH); + if depth < 0 { + errors.push(ResolveError::Invalid { + path: "run.clone.depth".to_string(), + reason: "depth must be at least 0".to_string(), + }); + depth = RunCloneSettings::DEFAULT_DEPTH; + } RunCloneSettings { enabled: clone.and_then(|clone| clone.enabled).unwrap_or(true), - depth: Some(depth), + depth, } } diff --git a/lib/foundation/fabro-config/src/tests/resolve_run.rs b/lib/foundation/fabro-config/src/tests/resolve_run.rs index d66b075ea..c738869e9 100644 --- a/lib/foundation/fabro-config/src/tests/resolve_run.rs +++ b/lib/foundation/fabro-config/src/tests/resolve_run.rs @@ -126,7 +126,7 @@ fn resolves_run_defaults_from_empty_settings() { assert!(!settings.environment.lifecycle.preserve); assert!(settings.environment.lifecycle.stop_on_terminal); assert!(settings.clone.enabled); - assert_eq!(settings.clone.depth, Some(100)); + assert_eq!(settings.clone.depth, 100); assert!(settings.run_branch.enabled); assert!(settings.run_branch.push); assert!(settings.meta_branch.enabled); @@ -312,7 +312,7 @@ push = false .run; assert!(!settings.clone.enabled); - assert_eq!(settings.clone.depth, Some(1)); + assert_eq!(settings.clone.depth, 1); assert!(settings.run_branch.enabled); assert!(!settings.run_branch.push); assert!(settings.meta_branch.enabled); @@ -332,7 +332,7 @@ depth = 0 .expect("zero clone depth should resolve") .run; - assert_eq!(settings.clone.depth, Some(0)); + assert_eq!(settings.clone.depth, 0); } #[test] diff --git a/lib/foundation/fabro-types/src/settings/run.rs b/lib/foundation/fabro-types/src/settings/run.rs index a47cb48fb..7854ee6d3 100644 --- a/lib/foundation/fabro-types/src/settings/run.rs +++ b/lib/foundation/fabro-types/src/settings/run.rs @@ -961,32 +961,30 @@ impl Default for RunCheckpointSettings { #[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] pub struct RunCloneSettings { pub enabled: bool, - #[serde( - default = "default_clone_depth", - skip_serializing_if = "Option::is_none" - )] - pub depth: Option, + #[serde(default = "default_clone_depth")] + pub depth: i32, } impl RunCloneSettings { pub const DEFAULT_DEPTH: i32 = 100; + + /// Git history depth to fetch, or `None` to fetch full history. + pub fn depth_limit(&self) -> Option { + (self.depth > 0).then_some(self.depth) + } } impl Default for RunCloneSettings { fn default() -> Self { Self { enabled: true, - depth: Some(Self::DEFAULT_DEPTH), + depth: Self::DEFAULT_DEPTH, } } } -#[expect( - clippy::unnecessary_wraps, - reason = "serde default provider must return the field's Option type" -)] -fn default_clone_depth() -> Option { - Some(RunCloneSettings::DEFAULT_DEPTH) +fn default_clone_depth() -> i32 { + RunCloneSettings::DEFAULT_DEPTH } #[derive(Debug, Clone, PartialEq, Serialize, Deserialize)]