diff --git a/lib/apps/fabro-cli/Cargo.toml b/lib/apps/fabro-cli/Cargo.toml index 3b997722f..4a97c0f07 100644 --- a/lib/apps/fabro-cli/Cargo.toml +++ b/lib/apps/fabro-cli/Cargo.toml @@ -118,6 +118,7 @@ chrono = { workspace = true } assert_cmd = "2" fabro-acp = { path = "../../components/fabro-acp", features = ["test-support"] } fabro-build-support = { path = "../../foundation/build-support" } +fabro-sandbox = { path = "../../components/fabro-sandbox", features = ["test-support"] } fabro-server = { path = "../fabro-server", features = ["test-support"] } fabro-workflow = { path = "../../components/fabro-workflow", features = ["test-support"] } fabro-types = { path = "../../foundation/fabro-types", features = ["clap", "test-support"] } diff --git a/lib/apps/fabro-cli/tests/it/cmd/attach.rs b/lib/apps/fabro-cli/tests/it/cmd/attach.rs index 585a29fe0..7f75eee71 100644 --- a/lib/apps/fabro-cli/tests/it/cmd/attach.rs +++ b/lib/apps/fabro-cli/tests/it/cmd/attach.rs @@ -1084,6 +1084,19 @@ fn attach_json_errors_without_prompting_for_human_input() { "run_id": "[ULID]", "ts": "[TIMESTAMP]" }, + { + "actor": { + "kind": "worker", + "run_id": "[ULID]" + }, + "event": "sandbox.initializing", + "id": "[EVENT_ID]", + "properties": { + "provider": "local" + }, + "run_id": "[ULID]", + "ts": "[TIMESTAMP]" + }, { "actor": { "kind": "worker", @@ -1169,19 +1182,6 @@ fn attach_json_errors_without_prompting_for_human_input() { "run_id": "[ULID]", "ts": "[TIMESTAMP]" }, - { - "actor": { - "kind": "worker", - "run_id": "[ULID]" - }, - "event": "sandbox.initializing", - "id": "[EVENT_ID]", - "properties": { - "provider": "local" - }, - "run_id": "[ULID]", - "ts": "[TIMESTAMP]" - }, { "actor": { "kind": "worker", @@ -1207,7 +1207,9 @@ fn attach_json_errors_without_prompting_for_human_input() { "id": "host-dir-[HEX]", "provider": "local", "repo_cloned": false, - "working_directory": "[TEMP_DIR]" + "repos_root": "[TEMP_DIR]/.repos", + "working_directory": "[TEMP_DIR]", + "workspace_root": "[TEMP_DIR]" }, "run_id": "[ULID]", "ts": "[TIMESTAMP]" diff --git a/lib/apps/fabro-cli/tests/it/cmd/support.rs b/lib/apps/fabro-cli/tests/it/cmd/support.rs index 51e179298..3d52f599b 100644 --- a/lib/apps/fabro-cli/tests/it/cmd/support.rs +++ b/lib/apps/fabro-cli/tests/it/cmd/support.rs @@ -1107,7 +1107,7 @@ async fn append_seeded_simple_completion_events( serde_json::json!({ "working_directory": context.temp_dir.display().to_string(), "provider": "local", - "id": format!("local:{}", run.run_id), + "id": fabro_sandbox::test_support::local_sandbox_id(&context.temp_dir).await, "repo_cloned": false, "clone_origin_url": null, "clone_branch": null, @@ -1276,7 +1276,7 @@ async fn append_seeded_git_completion_events( serde_json::json!({ "working_directory": context.temp_dir.display().to_string(), "provider": "local", - "id": format!("local:{}", run.run_id), + "id": fabro_sandbox::test_support::local_sandbox_id(&context.temp_dir).await, "repo_cloned": false, "clone_origin_url": null, "clone_branch": null, diff --git a/lib/apps/fabro-server/src/run_manifest.rs b/lib/apps/fabro-server/src/run_manifest.rs index 2fb76d177..dbad49938 100644 --- a/lib/apps/fabro-server/src/run_manifest.rs +++ b/lib/apps/fabro-server/src/run_manifest.rs @@ -18,8 +18,7 @@ use fabro_llm::FabroClient; use fabro_llm::lithos_catalog::Catalog; use fabro_llm::probe::{self, ModelTestStatus}; use fabro_sandbox::{ - CloneRequest, ProviderAccess, ProviderSandboxSpec, RunSandbox, SandboxSpec, - sandbox_spec_for_environment, + CloneRequest, ProviderAccess, RunSandbox, SandboxSpec, sandbox_spec_for_environment, }; use fabro_static::EnvVars; use fabro_types::settings::ModelRef; @@ -929,7 +928,7 @@ fn preflight_sandbox_spec( err, ) })?; - return Ok(SandboxSpec::Local { working_directory }); + return Ok(SandboxSpec::local(working_directory, access.clone())); } // No vault is available on this path, so a `{{ secrets.* }}` value keeps // its source form. Preflight never clones. @@ -942,14 +941,14 @@ fn preflight_sandbox_spec( branch: clone_branch, ..CloneRequest::none() }; - Ok(SandboxSpec::Provider(Box::new(ProviderSandboxSpec { + Ok(SandboxSpec { kind: sandbox_provider.clone(), access: access.clone(), spec, clone, github_app, run_id: None, - }))) + }) } async fn run_sandbox_check( @@ -2220,18 +2219,14 @@ provider = "local" &ProviderAccess::default(), ); - match spec { - Ok(SandboxSpec::Provider(spec)) => { - assert_eq!(spec.kind, SandboxProviderKind::DOCKER); - assert!(spec.clone.skip); - assert_eq!( - spec.clone.origin_url.as_deref(), - Some("https://github.com/acme/widgets") - ); - assert_eq!(spec.clone.branch.as_deref(), Some("main")); - } - _ => panic!("expected Docker preflight sandbox spec"), - } + let spec = spec.expect("Docker preflight sandbox spec"); + assert_eq!(spec.kind, SandboxProviderKind::DOCKER); + assert!(spec.clone.skip); + assert_eq!( + spec.clone.origin_url.as_deref(), + Some("https://github.com/acme/widgets") + ); + assert_eq!(spec.clone.branch.as_deref(), Some("main")); } #[test] diff --git a/lib/apps/fabro-server/src/server/handler/sandbox.rs b/lib/apps/fabro-server/src/server/handler/sandbox.rs index c01b9ae61..c76f8f155 100644 --- a/lib/apps/fabro-server/src/server/handler/sandbox.rs +++ b/lib/apps/fabro-server/src/server/handler/sandbox.rs @@ -1011,6 +1011,7 @@ mod tests { mod retrieve_sandbox_tests { use axum::body::{Body, to_bytes}; use axum::http::{Request, StatusCode}; + use fabro_sandbox::test_support::local_sandbox_id; use fabro_types::{Graph, RunId, WorkflowSettings, test_support}; use serde_json::{Value, json}; use tower::ServiceExt; @@ -1064,15 +1065,24 @@ mod retrieve_sandbox_tests { run_id: &RunId, provider: &str, ) { - append_sandbox_initialized_in(run_store, run_id, provider, "/workspace").await; + append_sandbox_initialized_in( + run_store, + run_id, + provider, + &format!("{provider}:sandbox-id"), + "/workspace", + ) + .await; } - /// A local sandbox reconnects by attaching to its working directory, so - /// a test that reaches one records a directory that exists. + /// A local sandbox reconnects by the id the Host provider derives from + /// its working directory, so a test that reaches one records an + /// existing directory under the id fabro would have written for it. async fn append_sandbox_initialized_in( run_store: &fabro_store::RunDatabase, run_id: &RunId, provider: &str, + id: &str, working_directory: &str, ) { let payload = fabro_store::EventPayload::new( @@ -1083,7 +1093,7 @@ mod retrieve_sandbox_tests { "event": "sandbox.initialized", "properties": { "provider": provider, - "id": format!("{provider}:sandbox-id"), + "id": id, "working_directory": working_directory, }, }), @@ -1218,11 +1228,10 @@ mod retrieve_sandbox_tests { .await .expect("test run should be creatable"); append_run_created(&run_store, &run_id).await; - // A record written before local sandboxes had directory-derived - // ids: the id is recomputed from the directory on reconnect. let workspace = tempfile::tempdir().expect("scratch directory"); let working_directory = workspace.path().to_str().expect("utf-8").to_owned(); - append_sandbox_initialized_in(&run_store, &run_id, "local", &working_directory).await; + let id = local_sandbox_id(workspace.path()).await; + append_sandbox_initialized_in(&run_store, &run_id, "local", &id, &working_directory).await; let response = app .oneshot(req_get(&format!("/api/v1/runs/{run_id}/sandbox"))) @@ -1231,7 +1240,7 @@ mod retrieve_sandbox_tests { assert_eq!(response.status(), StatusCode::OK); let body = body_json(response).await; assert_eq!(body["sandbox"]["provider"], "local"); - assert_eq!(body["sandbox"]["runtime"]["id"], "local:sandbox-id"); + assert_eq!(body["sandbox"]["runtime"]["id"], id); assert_eq!( body["sandbox"]["runtime"]["working_directory"], working_directory @@ -1265,6 +1274,7 @@ mod retrieve_sandbox_tests { &run_store, &run_id, "local", + &local_sandbox_id(workspace.path()).await, workspace.path().to_str().expect("utf-8"), ) .await; diff --git a/lib/components/fabro-sandbox/src/driver_sandbox.rs b/lib/components/fabro-sandbox/src/driver_sandbox.rs index 699826e5c..7313392f5 100644 --- a/lib/components/fabro-sandbox/src/driver_sandbox.rs +++ b/lib/components/fabro-sandbox/src/driver_sandbox.rs @@ -12,7 +12,7 @@ //! through the [`EventContext`] a sandbox is created or attached with. use std::collections::HashMap; -use std::path::{Path, PathBuf}; +use std::path::Path; use std::sync::{Arc, OnceLock}; use std::time::{Duration, Instant}; @@ -23,11 +23,9 @@ use fabro_util::workspace_glob::WorkspaceGlob; use sandbox_driver::{ DirEntry, EventContext, ExecControls, ExecResult, ExecSpec, ExecStreamingResult, FileKind, GitRetryPolicy, GrepMatch, GrepOptions, PtyOptions, PtySession, PtySize, - Sandbox as DriverHandle, SandboxProvider as DriverProvider, SandboxSource, - SandboxSpec as DriverSpec, SandboxState, Search as _, StdioProcess, WaitOptions, WalkOptions, + Sandbox as DriverHandle, SandboxProvider as DriverProvider, SandboxSpec as DriverSpec, + SandboxState, Search as _, StdioProcess, WaitOptions, WalkOptions, }; -use sandbox_driver_host::HostProvider; -use tokio::fs; use tokio::sync::OnceCell; use tokio_util::sync::CancellationToken; @@ -35,42 +33,9 @@ use crate::clone::{self, GitHubClone}; use crate::clone_source::{self, CloneDecision, EmptyWorkspaceReason}; use crate::credentials::{self, RepoCredentials}; use crate::environment::CloneRequest; -use crate::{GitRunInfo, GitSetupIntent}; - -/// A sandbox on the worker host at `working_directory`, the fabro `local` -/// kind, served by the driver's in-process Host provider. -/// -/// The directory is designated: the sandbox uses it in place and never -/// removes it. It is created when missing so a run can point at a fresh -/// scratch path. The registry lives in a per-process temporary root, so a -/// later process rebuilds the handle by calling this again with the -/// persisted working directory rather than by id. -pub async fn local_sandbox(working_directory: impl Into) -> crate::Result { - local_sandbox_with_events(working_directory, None).await -} - -/// [`local_sandbox`] whose driver lifecycle events reach `events`. -pub async fn local_sandbox_with_events( - working_directory: impl Into, - events: Option, -) -> crate::Result { - let working_directory: PathBuf = working_directory.into(); - fs::create_dir_all(&working_directory) - .await - .map_err(|error| crate::Error::context("Failed to create working directory", error))?; - let provider = HostProvider::new(); - let spec = DriverSpec::new(SandboxSource::HostDirectory) - .working_directory(working_directory.display().to_string()); - let handle = provider - .create(&spec, events) - .await - .map_err(|error| crate::Error::context("Failed to create local sandbox", error))?; - let sandbox = RunSandbox::new(SandboxProviderKind::LOCAL, handle); - sandbox.learn_platform().await?; - Ok(sandbox) -} use crate::exec::SandboxExec; use crate::sandbox::{self, PushError, PushReport, SandboxFile, SandboxWorkspaceLayout}; +use crate::{GitRunInfo, GitSetupIntent}; /// Where a clone-based provider puts its files: the run works under /// `workspace_root`, and repositories check out under `repos_root`. @@ -111,8 +76,10 @@ enum WorkspacePlan { Attached, } -/// Fabro's clone-based workspace on an isolated sandbox: the layout, the -/// clone it performs, and the GitHub credentials its checkout carries. +/// The run's workspace on a sandbox: the layout, the clone fabro performs +/// into it (if any), and the GitHub credentials its checkout carries. A +/// workspace fabro did not clone into is still a checkout the run may push +/// from, with whatever credentials the checkout carries itself. pub(crate) struct RepoWorkspace { layout: OnceLock, plan: WorkspacePlan, @@ -202,6 +169,21 @@ impl RepoWorkspace { workspace } + /// The workspace an existing handle already works in, whatever it + /// holds: nothing fabro cloned, laid out from the handle's own working + /// directory. + pub(crate) fn existing() -> Self { + Self { + layout: LayoutSource::ProviderWorkingDirectory.into_cell(), + plan: WorkspacePlan::Attached, + credentials: RepoCredentials::none(), + repo_cloned: OnceLock::new(), + origin_url: OnceLock::new(), + execution_directory: OnceLock::new(), + checkout_path: OnceLock::new(), + } + } + /// Settle a provider-dependent layout from the sandbox's working /// directory. A fixed layout is left alone. fn resolve_layout(&self, provider_working_directory: &str) -> &WorkspaceLayout { @@ -285,7 +267,7 @@ pub struct RunSandbox { /// pending one. handle: OnceCell>, pending: Option, - workspace: Option, + workspace: RepoWorkspace, /// Where the driver reports the lifecycle of a sandbox this creates. /// Set before `initialize` on a pending sandbox; an existing handle /// already carries the context it was created or attached with. @@ -298,12 +280,11 @@ pub struct RunSandbox { } impl RunSandbox { - /// Wraps an existing driver handle as a sandbox of `kind`. + /// Wraps an existing driver handle as a sandbox of `kind`, working in + /// whatever the handle's working directory holds. #[must_use] pub fn new(kind: SandboxProviderKind, handle: Arc) -> Self { - let sandbox = Self::empty(kind); - let _ = sandbox.handle.set(handle); - sandbox + Self::attached(kind, handle, RepoWorkspace::existing()) } /// A sandbox over an existing handle whose platform is already known, @@ -328,9 +309,8 @@ impl RunSandbox { spec: DriverSpec, workspace: RepoWorkspace, ) -> Self { - let mut sandbox = Self::empty(kind); + let mut sandbox = Self::empty(kind, workspace); sandbox.pending = Some(PendingCreate { provider, spec }); - sandbox.workspace = Some(workspace); sandbox } @@ -347,17 +327,17 @@ impl RunSandbox { workspace: RepoWorkspace, ) -> Self { workspace.resolve_layout(handle.working_directory()); - let mut sandbox = Self::new(kind, handle); - sandbox.workspace = Some(workspace); + let sandbox = Self::empty(kind, workspace); + let _ = sandbox.handle.set(handle); sandbox } - fn empty(kind: SandboxProviderKind) -> Self { + fn empty(kind: SandboxProviderKind, workspace: RepoWorkspace) -> Self { Self { kind, handle: OnceCell::new(), pending: None, - workspace: None, + workspace, events: None, platform: OnceLock::new(), snapshot: OnceLock::new(), @@ -393,10 +373,8 @@ impl RunSandbox { /// run's directory. Absent until a pending sandbox is initialized. pub fn exec(&self) -> crate::Result> { let mut exec = SandboxExec::new(self.handle()?.exec()); - if let Some(workspace) = &self.workspace { - if let Some(dir) = workspace.execution_directory.get() { - exec = exec.with_working_dir(dir.clone()); - } + if let Some(dir) = self.workspace.execution_directory.get() { + exec = exec.with_working_dir(dir.clone()); } Ok(exec) } @@ -405,11 +383,7 @@ impl RunSandbox { /// resolves relative paths against the sandbox's own working directory, /// which sits above a cloned repository's link. fn resolve(&self, path: &str) -> String { - match self - .workspace - .as_ref() - .and_then(|workspace| workspace.execution_directory.get()) - { + match self.workspace.execution_directory.get() { Some(working_directory) => sandbox::resolve_path(path, working_directory), None => path.to_string(), } @@ -488,9 +462,7 @@ impl RunSandbox { /// Prepare the workspace after the sandbox runs for the first time: /// an empty root, or fabro's clone. async fn prepare_workspace(&self) -> crate::Result<()> { - let Some(workspace) = &self.workspace else { - return Ok(()); - }; + let workspace = &self.workspace; let layout = workspace .resolve_layout(self.handle()?.working_directory()) .clone(); @@ -615,11 +587,7 @@ impl RunSandbox { // A cloned repository is reached through a workspace link. The // driver refuses a symlinked traversal root, so walk the real // checkout; results are reported under the link. - if let Some(checkout) = self - .workspace - .as_ref() - .and_then(|workspace| workspace.checkout_path.get()) - { + if let Some(checkout) = self.workspace.checkout_path.get() { return sandbox::join_sandbox_path(checkout, relative_start); } if relative_start.is_empty() { @@ -909,16 +877,10 @@ impl RunSandbox { /// The directory the run works in: the cloned repository's link for a /// clone-based workspace, the provider's working directory otherwise. pub fn working_directory(&self) -> &str { - if let Some(directory) = self - .workspace - .as_ref() - .and_then(RepoWorkspace::working_directory) - { - return directory; - } - self.handle - .get() - .map_or("", |handle| handle.working_directory()) + self.workspace + .working_directory() + .or_else(|| self.handle.get().map(|handle| handle.working_directory())) + .unwrap_or("") } pub fn runtime_directory(&self) -> Option<&str> { @@ -955,7 +917,7 @@ impl RunSandbox { } pub fn workspace_layout(&self) -> Option { - self.workspace.as_ref().and_then(RepoWorkspace::record) + self.workspace.record() } pub async fn setup_git(&self, intent: &GitSetupIntent) -> crate::Result> { @@ -965,44 +927,42 @@ impl RunSandbox { sandbox::setup_git(self, intent).await.map(Some) } + /// Push `refspec` from the run's checkout. A checkout fabro cloned + /// pushes with the credentials it was cloned with. Any other checkout + /// pushes only when it has an origin, with whatever credentials it + /// carries itself; a workspace without one has nothing to push. pub async fn git_push_ref( &self, refspec: &str, policy: &GitRetryPolicy, ) -> Result { - let Some(workspace) = &self.workspace else { - // A designated directory: push only when the checkout has an - // origin, with whatever credentials its URL already carries. - let has_origin = match self - .exec_command("git remote get-url origin", 10_000, None, None, None) - .await - { - Ok(result) if result.success() => true, - Ok(_) => false, - Err(err) => { - return Err(PushError { - report: PushReport::default(), - error: crate::Error::context("git remote get-url origin", err), - }); - } - }; - if !has_origin { - return Ok(PushReport::default()); + let workspace = &self.workspace; + if workspace.repo_cloned() { + return sandbox::git_push(self, Some(&workspace.credentials), refspec, policy).await; + } + let has_origin = match self + .exec_command("git remote get-url origin", 10_000, None, None, None) + .await + { + Ok(result) => result.success(), + Err(err) => { + return Err(PushError { + report: PushReport::default(), + error: crate::Error::context("git remote get-url origin", err), + }); } - return sandbox::git_push(self, None, refspec, policy).await; }; - if !workspace.repo_cloned() { + if !has_origin { return Ok(PushReport::default()); } - sandbox::git_push(self, Some(&workspace.credentials), refspec, policy).await + sandbox::git_push(self, None, refspec, policy).await } pub fn origin_url(&self) -> Option<&str> { - let workspace = self.workspace.as_ref()?; - if !workspace.repo_cloned() { + if !self.workspace.repo_cloned() { return None; } - workspace.origin_url.get().map(String::as_str) + self.workspace.origin_url.get().map(String::as_str) } /// Renew the credentials the agent's own git commands read for the @@ -1012,9 +972,7 @@ impl RunSandbox { /// checkout to install them in. #[tracing::instrument(name = "git_op", skip_all, fields(op = "refresh-credentials"))] pub async fn refresh_ambient_credentials(&self) -> crate::Result> { - let Some(workspace) = &self.workspace else { - return Ok(None); - }; + let workspace = &self.workspace; let Some(checkout) = workspace.checkout_path.get() else { return Ok(None); }; @@ -1026,9 +984,7 @@ impl RunSandbox { } pub fn push_token_source(&self) -> Option> { - self.workspace - .as_ref() - .and_then(|workspace| workspace.credentials.source().cloned()) + self.workspace.credentials.source().cloned() } /// The local command that opens a shell in the sandbox, from the @@ -1065,9 +1021,7 @@ impl RunSandbox { impl RunSandbox { fn repo_cloned(&self) -> bool { - self.workspace - .as_ref() - .is_some_and(RepoWorkspace::repo_cloned) + self.workspace.repo_cloned() } /// Delete the sandbox on the provider. A pending sandbox that was never @@ -1095,7 +1049,10 @@ mod tests { use tokio::fs; use super::*; + use crate::driver::ProviderAccess; use crate::exec::ExecResultExt; + use crate::provider_sandbox::local_sandbox; + use crate::sandbox_spec::SandboxSpec as RunSandboxSpec; struct Fixture { dir: tempfile::TempDir, @@ -1327,14 +1284,13 @@ mod tests { async fn lifecycle_reaches_the_driver_events_and_learns_the_platform() { let dir = tempfile::tempdir().unwrap(); let recorded = Arc::new(Recorded(Mutex::new(Vec::new()))); - let sandbox = local_sandbox_with_events( - dir.path(), - Some(EventContext::new( + let sandbox = RunSandboxSpec::local(dir.path(), ProviderAccess::default()) + .build(Some(EventContext::new( Arc::clone(&recorded) as Arc - )), - ) - .await - .unwrap(); + ))) + .await + .unwrap(); + sandbox.initialize().await.unwrap(); let expected = if cfg!(target_os = "macos") { "darwin" } else { diff --git a/lib/components/fabro-sandbox/src/lib.rs b/lib/components/fabro-sandbox/src/lib.rs index c935c02fe..31b9b1d7f 100644 --- a/lib/components/fabro-sandbox/src/lib.rs +++ b/lib/components/fabro-sandbox/src/lib.rs @@ -33,7 +33,7 @@ pub mod test_support; pub use details::sandbox_details; pub use docker::check_docker_daemon; pub use driver::{DaytonaCredentials, ProviderAccess}; -pub use driver_sandbox::{RunSandbox, local_sandbox}; +pub use driver_sandbox::RunSandbox; pub use environment::{CloneRequest, sandbox_spec_for_environment}; pub use error::{Error, Result, default_redacted_output_tail, display_for_log}; pub use exec::{ @@ -49,7 +49,7 @@ pub use git_policy::{ retry_git_messages, transient_git_failure, }; pub use provider::{SandboxInventory, SandboxLookupError}; -pub use provider_sandbox::{attach_provider_sandbox, provider_sandbox}; +pub use provider_sandbox::{attach_provider_sandbox, local_sandbox, provider_sandbox}; pub use reconnect::{open_terminal_for_run, reconnect_for_run}; pub use sandbox::{ DEFAULT_EXEC_OUTPUT_TAIL_BYTES, GitRunInfo, GitSetupIntent, PushAttempt, PushError, PushReport, @@ -65,4 +65,4 @@ pub use sandbox_driver::{ OutputStream, PtySession, PtySize, Resources, SandboxSource, SandboxSpec as DriverSpec, StderrTail, StdioProcess, StdioProcessHandle, Termination, TransportError, WalkOptions, }; -pub use sandbox_spec::{ProviderSandboxSpec, SandboxSpec}; +pub use sandbox_spec::SandboxSpec; diff --git a/lib/components/fabro-sandbox/src/provider_sandbox.rs b/lib/components/fabro-sandbox/src/provider_sandbox.rs index 93a5e627a..a5011a1ad 100644 --- a/lib/components/fabro-sandbox/src/provider_sandbox.rs +++ b/lib/components/fabro-sandbox/src/provider_sandbox.rs @@ -6,9 +6,11 @@ //! construction function, and a bundled provider adds only what its //! backend needs on top: Docker its fixed working directory and default //! image, Daytona its fixed working directory, default snapshot, and -//! lifecycle timers. A plugin gets the spec as is, trimmed to what it can -//! honor, laid out inside the working directory the provider chooses. +//! lifecycle timers, the Host the designated directory it works in, +//! created when missing. A plugin gets the spec as is, trimmed to what it +//! can honor, laid out inside the working directory the provider chooses. +use std::path::PathBuf; use std::sync::Arc; use fabro_github::GitHubCredentials; @@ -17,10 +19,12 @@ use sandbox_driver::{ EventContext, OwnedProvider, SandboxId, SandboxProvider, SandboxSource, SandboxSpec as DriverSpec, }; +use tokio::fs; use crate::driver::{ProviderAccess, connect_provider}; use crate::driver_sandbox::{LayoutSource, RepoWorkspace, RunSandbox}; use crate::environment::{self, CloneRequest}; +use crate::sandbox_spec::SandboxSpec; use crate::{daytona, docker, managed_labels}; /// A sandbox for a run on `kind`. The sandbox is created by `initialize`; @@ -51,12 +55,10 @@ pub async fn provider_sandbox( daytona::overlay(spec, run_id.as_ref()), workspace, ), - Some(BundledProvider::Local) => { - return Err(crate::Error::message( - "local sandboxes are built from a working directory, not a provider spec", - )); - } - None => { + Some(BundledProvider::Local) | None => { + if kind.is_local() { + designate_directory(&spec).await?; + } let capabilities = provider.capabilities(); spec.network = environment::supported_network(spec.network, capabilities); spec.timers = environment::supported_timers(spec.timers, capabilities); @@ -65,6 +67,33 @@ pub async fn provider_sandbox( }) } +/// The Host provider works in a designated directory in place and needs it +/// to exist. A run may point at a fresh scratch path, so the directory is +/// created before the provider sees the spec. +async fn designate_directory(spec: &DriverSpec) -> crate::Result<()> { + let Some(directory) = &spec.working_directory else { + return Ok(()); + }; + fs::create_dir_all(directory).await.map_err(|error| { + crate::Error::context( + format!("Failed to create working directory {directory}"), + error, + ) + }) +} + +/// A sandbox on this host at `working_directory`, ready to use: the `local` +/// kind, built through the provider path with default settings and +/// initialized. For the agent CLI and tests; a run builds its sandbox from +/// its [`SandboxSpec`] and initializes it itself. +pub async fn local_sandbox(working_directory: impl Into) -> crate::Result { + let spec = SandboxSpec::local(working_directory, ProviderAccess::default()); + let sandbox = + provider_sandbox(spec.kind, &spec.access, spec.spec, &spec.clone, None, None).await?; + sandbox.initialize().await?; + Ok(sandbox) +} + /// Reattach to a run's sandbox on `kind` by its persisted id. The driver /// reports the sandbox's lifecycle from here on through `events`. /// diff --git a/lib/components/fabro-sandbox/src/reconnect.rs b/lib/components/fabro-sandbox/src/reconnect.rs index cd5ef9f56..fa07fa345 100644 --- a/lib/components/fabro-sandbox/src/reconnect.rs +++ b/lib/components/fabro-sandbox/src/reconnect.rs @@ -1,9 +1,6 @@ -use std::path::Path; - use anyhow::{Context, Result}; -use fabro_types::{BundledProvider, RunId, RunSandboxInstance}; +use fabro_types::{RunId, RunSandboxInstance}; use sandbox_driver::{EventContext, PtySession, PtySize}; -use sandbox_driver_host::HostProvider; use crate::driver::ProviderAccess; use crate::driver_sandbox::RunSandbox; @@ -22,11 +19,10 @@ pub async fn reconnect_for_run( events: Option, ) -> Result { let runtime = &record.runtime; - let sandbox_id = sandbox_id(record).await; provider_sandbox::attach_provider_sandbox( record.provider.clone(), access, - &sandbox_id, + &runtime.id, // A record without the flag was written for a sandbox fabro never // cloned into. runtime.repo_cloned.unwrap_or(false), @@ -39,20 +35,6 @@ pub async fn reconnect_for_run( .with_context(|| format!("Failed to reconnect {} sandbox", record.provider)) } -/// The id the record's sandbox attaches by. A local sandbox is its working -/// directory, and the Host provider derives the directory's id from its -/// path, so the record's id is recomputed from the directory: a record -/// written before directories had ids attaches the same way. -async fn sandbox_id(record: &RunSandboxInstance) -> String { - let runtime = &record.runtime; - if record.provider.bundled() == Some(BundledProvider::Local) { - if let Some(id) = HostProvider::directory_id(Path::new(&runtime.working_directory)).await { - return id.to_string(); - } - } - runtime.id.clone() -} - /// Opens an interactive shell in a run's sandbox over the driver's Pty /// facet, reconnecting from the run record first. The session is the /// driver's own; it is closed by the caller. diff --git a/lib/components/fabro-sandbox/src/sandbox.rs b/lib/components/fabro-sandbox/src/sandbox.rs index 7f37c2d52..101a9092e 100644 --- a/lib/components/fabro-sandbox/src/sandbox.rs +++ b/lib/components/fabro-sandbox/src/sandbox.rs @@ -242,8 +242,8 @@ pub struct PushError { /// Pushes a refspec to origin through the driver's git facet, retried by /// the driver under `policy` with one token for the whole operation. /// `credentials` is the checkout's managed credentials; `None` pushes with -/// whatever the checkout already has (the local sandbox, or a workspace -/// without a GitHub App). +/// whatever the checkout already has (a checkout fabro did not clone, or a +/// clone made without a GitHub App). #[tracing::instrument(name = "git_op", skip_all, fields(op = "push"))] pub(crate) async fn git_push( sandbox: &RunSandbox, diff --git a/lib/components/fabro-sandbox/src/sandbox_spec.rs b/lib/components/fabro-sandbox/src/sandbox_spec.rs index 8198afdba..3250163c8 100644 --- a/lib/components/fabro-sandbox/src/sandbox_spec.rs +++ b/lib/components/fabro-sandbox/src/sandbox_spec.rs @@ -4,28 +4,18 @@ use std::sync::Arc; use anyhow::Context as _; use fabro_github::GitHubCredentials; use fabro_types::{RunId, RunSandboxInstance, RunSandboxRuntime, SandboxProviderKind}; -use sandbox_driver::{EventContext, SandboxSpec as DriverSpec}; +use sandbox_driver::{EventContext, SandboxSource, SandboxSpec as DriverSpec}; use crate::driver::ProviderAccess; -use crate::driver_sandbox::{LayoutSource, RunSandbox, local_sandbox_with_events}; +use crate::driver_sandbox::{LayoutSource, RunSandbox}; use crate::environment::CloneRequest; use crate::{clone_source, provider_sandbox}; -/// Options for sandbox initialization and construction. +/// A run's sandbox on any provider fabro can name: a bundled kind in +/// process or a sandbox-driver plugin. What the environment asked for, and +/// how the repository is cloned into it. #[derive(Clone, Debug)] -pub enum SandboxSpec { - Local { - working_directory: PathBuf, - }, - /// A sandbox on any provider fabro can name: a bundled kind in process - /// or a sandbox-driver plugin. - Provider(Box), -} - -/// A run's sandbox on a provider: what the environment asked for and how -/// the repository is cloned into it. -#[derive(Clone, Debug)] -pub struct ProviderSandboxSpec { +pub struct SandboxSpec { pub kind: SandboxProviderKind, /// The provider settings and vault credentials the kind needs. pub access: ProviderAccess, @@ -38,144 +28,114 @@ pub struct ProviderSandboxSpec { } impl SandboxSpec { - pub fn provider(&self) -> SandboxProviderKind { - match self { - Self::Local { .. } => SandboxProviderKind::LOCAL, - Self::Provider(spec) => spec.kind.clone(), + /// A sandbox on this host at `working_directory`, the fabro `local` + /// kind. The directory is designated: the sandbox uses it in place, + /// never removes it, and clones nothing into it. The Host provider has + /// no image, labels, or lifecycle timers, so the spec names only the + /// directory. + #[must_use] + pub fn local(working_directory: impl Into, access: ProviderAccess) -> Self { + Self { + kind: SandboxProviderKind::LOCAL, + access, + spec: DriverSpec::new(SandboxSource::HostDirectory) + .working_directory(working_directory.into().display().to_string()), + clone: CloneRequest::none(), + github_app: None, + run_id: None, } } + pub fn provider(&self) -> SandboxProviderKind { + self.kind.clone() + } + pub fn provider_name(&self) -> String { - self.provider().to_string() + self.kind.to_string() + } + + /// The directory the spec designates on the provider, when it names one. + #[must_use] + pub fn working_directory(&self) -> Option<&str> { + self.spec.working_directory.as_deref() } /// The image the run record names for this sandbox: the environment's, - /// or the provider's default when the environment names none. A local - /// sandbox has no image. + /// or the provider's default when the environment names none. pub fn image(&self) -> Option { - match self { - Self::Local { .. } => None, - Self::Provider(spec) => provider_sandbox::recorded_image(&spec.kind, &spec.spec), - } + provider_sandbox::recorded_image(&self.kind, &self.spec) } /// Build initialized sandbox metadata for persistence. pub fn to_run_sandbox_instance(&self, sandbox: &RunSandbox) -> RunSandboxInstance { let working_directory = sandbox.working_directory().to_string(); let id = sandbox.sandbox_info(); - - match self { - Self::Provider(spec) => { - let ProviderSandboxSpec { - kind, spec, clone, .. - } = spec.as_ref(); - let clone_origin_url = &clone.origin_url; - let repo_cloned = - clone_source::repo_cloned_for_record(clone.skip, clone_origin_url.as_deref()); - // A fixed layout is known before the sandbox exists; a - // provider-chosen one only from the sandbox. - let layout = match provider_sandbox::layout_source(kind) { - LayoutSource::Fixed(fixed) => { - let repo = runtime_layout_metadata( - repo_cloned, - clone_origin_url.as_deref(), - &fixed.workspace_root, - &fixed.repos_root, - ); - Some(crate::SandboxWorkspaceLayout { - workspace_root: fixed.workspace_root, - repos_root: fixed.repos_root, - primary_repo_path: repo - .as_ref() - .map(|layout| layout.primary_repo_path.clone()), - primary_repo_link: repo - .as_ref() - .map(|layout| layout.primary_repo_link.clone()), - }) - } - LayoutSource::ProviderWorkingDirectory => sandbox.workspace_layout(), - }; - RunSandboxInstance { - provider: kind.clone(), - image: provider_sandbox::recorded_image(kind, spec), - snapshot: sandbox.snapshot_info(), - runtime: RunSandboxRuntime { - id, - working_directory, - repo_cloned, - clone_origin_url: clone_source::clean_clone_origin_for_record( - clone_origin_url.as_deref(), - ), - clone_branch: clone.branch.clone(), - workspace_root: layout.as_ref().map(|layout| layout.workspace_root.clone()), - repos_root: layout.as_ref().map(|layout| layout.repos_root.clone()), - primary_repo_path: layout - .as_ref() - .and_then(|layout| layout.primary_repo_path.clone()), - primary_repo_link: layout - .as_ref() - .and_then(|layout| layout.primary_repo_link.clone()), - }, - } + let clone_origin_url = &self.clone.origin_url; + let repo_cloned = + clone_source::repo_cloned_for_record(self.clone.skip, clone_origin_url.as_deref()); + // A fixed layout is known before the sandbox exists; a + // provider-chosen one only from the sandbox. + let layout = match provider_sandbox::layout_source(&self.kind) { + LayoutSource::Fixed(fixed) => { + let repo = runtime_layout_metadata( + repo_cloned, + clone_origin_url.as_deref(), + &fixed.workspace_root, + &fixed.repos_root, + ); + Some(crate::SandboxWorkspaceLayout { + workspace_root: fixed.workspace_root, + repos_root: fixed.repos_root, + primary_repo_path: repo.as_ref().map(|layout| layout.primary_repo_path.clone()), + primary_repo_link: repo.as_ref().map(|layout| layout.primary_repo_link.clone()), + }) } - Self::Local { .. } => RunSandboxInstance { - provider: self.provider(), - image: None, - snapshot: None, - runtime: RunSandboxRuntime { - id, - working_directory, - repo_cloned: Some(false), - clone_origin_url: None, - clone_branch: None, - workspace_root: None, - repos_root: None, - primary_repo_path: None, - primary_repo_link: None, - }, + LayoutSource::ProviderWorkingDirectory => sandbox.workspace_layout(), + }; + RunSandboxInstance { + provider: self.kind.clone(), + image: self.image(), + snapshot: sandbox.snapshot_info(), + runtime: RunSandboxRuntime { + id, + working_directory, + repo_cloned, + clone_origin_url: clone_source::clean_clone_origin_for_record( + clone_origin_url.as_deref(), + ), + clone_branch: self.clone.branch.clone(), + workspace_root: layout.as_ref().map(|layout| layout.workspace_root.clone()), + repos_root: layout.as_ref().map(|layout| layout.repos_root.clone()), + primary_repo_path: layout + .as_ref() + .and_then(|layout| layout.primary_repo_path.clone()), + primary_repo_link: layout + .as_ref() + .and_then(|layout| layout.primary_repo_link.clone()), }, } } - /// Builds the sandbox. The driver reports its lifecycle through - /// `events`: the local sandbox's from creation here, a provider - /// sandbox's from `initialize` on. + /// Builds the sandbox; `initialize` creates it on the provider. The + /// driver reports its lifecycle through `events` from then on. pub async fn build( &self, events: Option, ) -> Result, anyhow::Error> { - match self { - Self::Local { working_directory } => { - let sandbox = local_sandbox_with_events(working_directory.clone(), events) - .await - .context("Failed to create local sandbox")?; - Ok(Arc::new(sandbox)) - } - Self::Provider(spec) => { - let ProviderSandboxSpec { - kind, - access, - spec, - clone, - github_app, - run_id, - } = spec.as_ref(); - let mut sandbox = provider_sandbox::provider_sandbox( - kind.clone(), - access, - spec.clone(), - clone, - github_app.as_ref(), - *run_id, - ) - .await - .with_context(|| format!("Failed to create {kind} sandbox"))?; - if let Some(events) = events { - sandbox.set_events(events); - } - Ok(Arc::new(sandbox)) - } + let mut sandbox = provider_sandbox::provider_sandbox( + self.kind.clone(), + &self.access, + self.spec.clone(), + &self.clone, + self.github_app.as_ref(), + self.run_id, + ) + .await + .with_context(|| format!("Failed to create {} sandbox", self.kind))?; + if let Some(events) = events { + sandbox.set_events(events); } + Ok(Arc::new(sandbox)) } } @@ -193,13 +153,12 @@ fn runtime_layout_metadata( #[cfg(test)] mod tests { - use sandbox_driver::SandboxSource; use sandbox_driver_testing::ScriptedSandbox; use super::*; - fn provider_spec(clone: CloneRequest) -> ProviderSandboxSpec { - ProviderSandboxSpec { + fn docker_spec(clone: CloneRequest) -> SandboxSpec { + SandboxSpec { kind: SandboxProviderKind::DOCKER, access: ProviderAccess::default(), spec: DriverSpec::new(SandboxSource::HostDirectory), @@ -209,9 +168,9 @@ mod tests { } } - fn sandbox_at(working_dir: &str) -> RunSandbox { + fn sandbox_at(kind: SandboxProviderKind, working_dir: &str) -> RunSandbox { RunSandbox::new( - SandboxProviderKind::DOCKER, + kind, Arc::new(ScriptedSandbox::with_id_and_working_dir( "scripted-1", working_dir, @@ -221,12 +180,12 @@ mod tests { #[test] fn docker_run_sandbox_persists_layout_metadata_for_cloned_repo() { - let spec = SandboxSpec::Provider(Box::new(provider_spec(CloneRequest { + let spec = docker_spec(CloneRequest { origin_url: Some("git@github.com:brynary/rack-test.git".to_string()), branch: Some("main".to_string()), ..CloneRequest::default() - }))); - let sandbox = sandbox_at("/workspace/rack-test"); + }); + let sandbox = sandbox_at(SandboxProviderKind::DOCKER, "/workspace/rack-test"); let record = spec.to_run_sandbox_instance(&sandbox); let runtime = record.runtime; @@ -253,12 +212,12 @@ mod tests { #[tokio::test] async fn invalid_exact_checkout_spec_fails_before_provider_connection() { - let spec = SandboxSpec::Provider(Box::new(provider_spec(CloneRequest { + let spec = docker_spec(CloneRequest { origin_url: Some("https://github.com/acme/widgets".to_string()), branch: Some("main".to_string()), commit_sha: Some("not-a-sha".to_string()), ..CloneRequest::default() - }))); + }); let error = spec .build(None) @@ -276,11 +235,11 @@ mod tests { #[test] fn docker_run_sandbox_omits_primary_repo_metadata_for_empty_workspace() { - let spec = SandboxSpec::Provider(Box::new(provider_spec(CloneRequest { + let spec = docker_spec(CloneRequest { origin_url: Some("https://gitlab.com/acme/widgets".to_string()), ..CloneRequest::none() - }))); - let sandbox = sandbox_at("/workspace"); + }); + let sandbox = sandbox_at(SandboxProviderKind::DOCKER, "/workspace"); let record = spec.to_run_sandbox_instance(&sandbox); let runtime = record.runtime; @@ -292,4 +251,34 @@ mod tests { assert!(runtime.primary_repo_path.is_none()); assert!(runtime.primary_repo_link.is_none()); } + + #[test] + fn local_spec_designates_the_directory_and_clones_nothing() { + let spec = SandboxSpec::local("/home/dev/project", ProviderAccess::default()); + + assert_eq!(spec.kind, SandboxProviderKind::LOCAL); + assert_eq!(spec.working_directory(), Some("/home/dev/project")); + assert!(spec.clone.skip); + assert_eq!(spec.clone.origin_url, None); + assert_eq!(spec.image(), None); + assert!(matches!(spec.spec.source, SandboxSource::HostDirectory)); + + let sandbox = sandbox_at(SandboxProviderKind::LOCAL, "/home/dev/project"); + let record = spec.to_run_sandbox_instance(&sandbox); + + assert_eq!(record.provider, SandboxProviderKind::LOCAL); + assert_eq!(record.image, None); + assert_eq!(record.snapshot, None); + assert_eq!(record.runtime.id, "scripted-1"); + assert_eq!(record.runtime.working_directory, "/home/dev/project"); + assert_eq!(record.runtime.repo_cloned, Some(false)); + assert_eq!(record.runtime.clone_origin_url, None); + assert_eq!(record.runtime.clone_branch, None); + assert_eq!( + record.runtime.workspace_root.as_deref(), + Some("/home/dev/project") + ); + assert!(record.runtime.primary_repo_path.is_none()); + assert!(record.runtime.primary_repo_link.is_none()); + } } diff --git a/lib/components/fabro-sandbox/src/test_support.rs b/lib/components/fabro-sandbox/src/test_support.rs index aa93ce22e..05c84b16e 100644 --- a/lib/components/fabro-sandbox/src/test_support.rs +++ b/lib/components/fabro-sandbox/src/test_support.rs @@ -10,6 +10,7 @@ //! fabro's exec policy, down to the scripted driver. use std::collections::HashMap; +use std::path::Path; use std::sync::{Arc, OnceLock}; use std::time::Duration; @@ -17,6 +18,7 @@ use fabro_types::SandboxProviderKind; use sandbox_driver::{ ExecResult, GrepMatch, PlatformInfo, SandboxState, StderrTail, Termination, WalkedFile, }; +use sandbox_driver_host::HostProvider; pub use sandbox_driver_testing::{ ScriptedExec, ScriptedProvider, ScriptedSandbox, ScriptedStdioProcess, }; @@ -27,6 +29,22 @@ use crate::driver_sandbox::RunSandbox; use crate::managed_labels::{MANAGED_LABEL, MANAGED_LABEL_VALUE}; use crate::sandbox::SandboxFile; +/// The id a run record carries for a local sandbox at `working_directory`, +/// as the Host provider derives it from the canonical path. A record a test +/// writes by hand reconnects the way one fabro wrote would. The directory +/// must exist. +pub async fn local_sandbox_id(working_directory: &Path) -> String { + HostProvider::directory_id(working_directory) + .await + .unwrap_or_else(|| { + panic!( + "no local sandbox id for {}: the directory must exist", + working_directory.display() + ) + }) + .to_string() +} + /// A driver [`ExecResult`] with the given streams, for scripting a mock /// sandbox's answers. #[must_use] diff --git a/lib/components/fabro-workflow/src/operations/start.rs b/lib/components/fabro-workflow/src/operations/start.rs index 406cb5023..8395d765f 100644 --- a/lib/components/fabro-workflow/src/operations/start.rs +++ b/lib/components/fabro-workflow/src/operations/start.rs @@ -10,8 +10,7 @@ use fabro_llm::credentials::readiness; use fabro_llm::lithos_catalog::Catalog; use fabro_mcp::config::McpServerSettings; use fabro_sandbox::{ - CloneRequest, DaytonaCredentials, ProviderAccess, ProviderSandboxSpec, SandboxSpec, - sandbox_spec_for_environment, + CloneRequest, DaytonaCredentials, ProviderAccess, SandboxSpec, sandbox_spec_for_environment, }; use fabro_static::EnvVars; #[cfg(test)] @@ -506,10 +505,17 @@ impl RunSession { ))); } } + let daytona = vault_guard + .get(EnvVars::DAYTONA_API_KEY) + .map(|api_key| DaytonaCredentials::from_api_key(api_key.to_string(), process_env_var)); + let access = ProviderAccess { + providers: services.sandbox_providers.clone(), + daytona, + }; let sandbox = match sandbox_provider.bundled() { - Some(BundledProvider::Local) if dry_run_clone_target => SandboxSpec::Local { - working_directory: dry_run_workspace_for_target(persisted).await?, - }, + Some(BundledProvider::Local) if dry_run_clone_target => { + SandboxSpec::local(dry_run_workspace_for_target(persisted).await?, access) + } Some(BundledProvider::Local) => match record.target.as_ref() { Some(target @ (RunTarget::Git(_) | RunTarget::None {})) => { return Err(Error::engine(format!( @@ -517,9 +523,10 @@ impl RunSession { target.kind_name() ))); } - Some(RunTarget::Folder { path }) => SandboxSpec::Local { - working_directory: folder_working_directory_from_record(record, path).await?, - }, + Some(RunTarget::Folder { path }) => SandboxSpec::local( + folder_working_directory_from_record(record, path).await?, + access, + ), None => { let working_directory = resolved .environment @@ -530,17 +537,10 @@ impl RunSession { err, ) })?; - SandboxSpec::Local { working_directory } + SandboxSpec::local(working_directory, access) } }, _ => { - let daytona = vault_guard.get(EnvVars::DAYTONA_API_KEY).map(|api_key| { - DaytonaCredentials::from_api_key(api_key.to_string(), process_env_var) - }); - let access = ProviderAccess { - providers: services.sandbox_providers.clone(), - daytona, - }; let spec = resolve_sandbox_spec(resolved, secret_lookup)?; let mut clone = CloneRequest::from_settings(&resolved.clone); clone.skip |= clone_source.skip_clone; @@ -548,14 +548,14 @@ impl RunSession { clone.branch = clone_source.branch; clone.tag = clone_source.tag; clone.commit_sha = clone_source.commit_sha; - SandboxSpec::Provider(Box::new(ProviderSandboxSpec { + SandboxSpec { kind: sandbox_provider.clone(), access, spec, clone, github_app: services.github_app.clone(), run_id: Some(record.run_id), - })) + } } }; @@ -1875,10 +1875,7 @@ mod tests { assert_eq!(runtime.clone_branch, None); assert_eq!(runtime.primary_repo_path, None); assert_eq!(runtime.primary_repo_link, None); - let SandboxSpec::Provider(spec) = sandbox else { - panic!("none target should retain the selected Docker provider"); - }; - let ProviderSandboxSpec { kind, clone, .. } = *spec; + let SandboxSpec { kind, clone, .. } = sandbox; assert_eq!(kind, SandboxProviderKind::DOCKER); assert!(clone.skip); assert_eq!(clone.origin_url, None); @@ -1937,15 +1934,12 @@ mod tests { assert_eq!(runtime.clone_branch, None); assert_eq!(runtime.primary_repo_path, None); assert_eq!(runtime.primary_repo_link, None); - let SandboxSpec::Provider(spec) = sandbox else { - panic!("none target should retain the selected Daytona provider"); - }; - let ProviderSandboxSpec { + let SandboxSpec { kind, access, clone, .. - } = *spec; + } = sandbox; assert_eq!(kind, SandboxProviderKind::DAYTONA); assert!(access.daytona.is_some(), "the vault key reaches the spec"); assert!(clone.skip); @@ -2024,12 +2018,20 @@ mod tests { .await .unwrap(); - let SandboxSpec::Local { working_directory } = session.sandbox else { - panic!("clone target dry-run should execute in a Local scratch sandbox"); - }; assert_eq!( - working_directory, - run_dir.join("dry-run-workspace").canonicalize().unwrap() + session.sandbox.kind, + SandboxProviderKind::LOCAL, + "clone target dry-run should execute in a Local scratch sandbox" + ); + assert_eq!( + session.sandbox.working_directory().map(Path::new), + Some( + run_dir + .join("dry-run-workspace") + .canonicalize() + .unwrap() + .as_path() + ) ); assert_eq!(session.sandbox_env.origin_url, None); assert_eq!(session.pr_origin_url, None); @@ -2138,11 +2140,14 @@ mod tests { .await .unwrap(); - let SandboxSpec::Local { working_directory } = session.sandbox else { - panic!("folder target should retain the selected Local provider"); - }; - assert_eq!(working_directory, canonical_folder); - assert_ne!(working_directory, environment_cwd); + assert_eq!( + session.sandbox.kind, + SandboxProviderKind::LOCAL, + "folder target should retain the selected Local provider" + ); + let working_directory = session.sandbox.working_directory().map(Path::new); + assert_eq!(working_directory, Some(canonical_folder.as_path())); + assert_ne!(working_directory, Some(environment_cwd.as_path())); assert_eq!(session.sandbox_env.origin_url.as_deref(), Some(origin_url)); assert_eq!(session.pr_origin_url.as_deref(), Some(origin_url)); } @@ -2207,10 +2212,15 @@ mod tests { .await .unwrap(); - let SandboxSpec::Local { working_directory } = session.sandbox else { - panic!("legacy Local run should retain the selected Local provider"); - }; - assert_eq!(working_directory, environment_cwd); + assert_eq!( + session.sandbox.kind, + SandboxProviderKind::LOCAL, + "legacy Local run should retain the selected Local provider" + ); + assert_eq!( + session.sandbox.working_directory().map(Path::new), + Some(environment_cwd.as_path()) + ); } #[tokio::test] diff --git a/lib/components/fabro-workflow/src/pipeline/execute/tests.rs b/lib/components/fabro-workflow/src/pipeline/execute/tests.rs index a8d49ed46..6291c9e2f 100644 --- a/lib/components/fabro-workflow/src/pipeline/execute/tests.rs +++ b/lib/components/fabro-workflow/src/pipeline/execute/tests.rs @@ -16,8 +16,8 @@ use fabro_auth::test_support as auth_test_support; use fabro_graphviz::graph::{AttrValue, Edge, Graph, Node}; use fabro_hooks::HookSettings; use fabro_interview::AutoApproveInterviewer; -use fabro_sandbox::SandboxSpec; -use fabro_sandbox::test_support::MockSandbox; +use fabro_sandbox::test_support::{MockSandbox, local_sandbox_id}; +use fabro_sandbox::{ProviderAccess, SandboxSpec}; use fabro_store::Database; use fabro_types::settings::run::RunModelControls; use fabro_types::{ @@ -262,9 +262,10 @@ async fn execute_test_run_with_options( run_store: run_store.into(), dry_run: false, emitter: emitter.clone(), - sandbox: SandboxSpec::Local { - working_directory: std::env::current_dir().unwrap(), - }, + sandbox: SandboxSpec::local( + std::env::current_dir().unwrap(), + ProviderAccess::default(), + ), llm: LlmSpec { model: "test-model".to_string(), provider_id: lithos_llm::catalog::builtin::anthropic(), @@ -324,9 +325,10 @@ async fn execute_runs_start_to_exit_and_returns_final_context() { run_store: run_store.into(), dry_run: false, emitter: test_emitter_arc("run-test"), - sandbox: SandboxSpec::Local { - working_directory: std::env::current_dir().unwrap(), - }, + sandbox: SandboxSpec::local( + std::env::current_dir().unwrap(), + ProviderAccess::default(), + ), llm: LlmSpec { model: "test-model".to_string(), provider_id: lithos_llm::catalog::builtin::anthropic(), @@ -411,10 +413,11 @@ async fn resumed_in_flight_node_starts_a_new_stage_execution() { let run_store = test_run_store(&run_id).await; seed_created_and_starting(&run_store, &run_options, &graph).await; // Resume reconnects to the previously recorded sandbox. + let working_directory = std::env::current_dir().unwrap(); append_event(&run_store, &run_id, &Event::SandboxInitialized { - working_directory: std::env::current_dir().unwrap().display().to_string(), + working_directory: working_directory.display().to_string(), provider: fabro_types::SandboxProviderKind::LOCAL, - id: "local".to_string(), + id: local_sandbox_id(&working_directory).await, image: None, snapshot: None, repo_cloned: None, @@ -467,9 +470,10 @@ async fn resumed_in_flight_node_starts_a_new_stage_execution() { run_store: run_store.into(), dry_run: false, emitter: emitter.clone(), - sandbox: SandboxSpec::Local { - working_directory: std::env::current_dir().unwrap(), - }, + sandbox: SandboxSpec::local( + std::env::current_dir().unwrap(), + ProviderAccess::default(), + ), llm: LlmSpec { model: "test-model".to_string(), provider_id: lithos_llm::catalog::builtin::anthropic(), @@ -583,9 +587,7 @@ async fn run_with_lifecycle( run_store: run_store.into(), dry_run: false, emitter: emitter.clone(), - sandbox: SandboxSpec::Local { - working_directory: PathBuf::from(sandbox.working_directory()), - }, + sandbox: SandboxSpec::local(sandbox.working_directory(), ProviderAccess::default()), llm: LlmSpec { model: "test-model".to_string(), provider_id: lithos_llm::catalog::builtin::anthropic(), diff --git a/lib/components/fabro-workflow/src/pipeline/initialize.rs b/lib/components/fabro-workflow/src/pipeline/initialize.rs index b38940756..d345397d4 100644 --- a/lib/components/fabro-workflow/src/pipeline/initialize.rs +++ b/lib/components/fabro-workflow/src/pipeline/initialize.rs @@ -11,8 +11,7 @@ use fabro_hooks::{HookContext, HookDecision, HookEvent, HookExecutionContext, Ho use fabro_llm::credentials::{CredentialProvider, readiness}; use fabro_llm::lithos_catalog::Catalog; use fabro_sandbox::{ - DaytonaCredentials, ExecResultExt, GitSetupIntent, ProviderAccess, SandboxSpec, - reconnect_for_run, + DaytonaCredentials, ExecResultExt, GitSetupIntent, ProviderAccess, reconnect_for_run, }; use fabro_static::EnvVars; use fabro_types::RunSandboxKind; @@ -368,7 +367,7 @@ pub async fn initialize( .as_ref() .and_then(|git| git.sha.clone()); if !is_resume - && !matches!(options.sandbox, SandboxSpec::Local { .. }) + && !options.sandbox.kind.is_local() && matches!( options .run_options @@ -936,7 +935,7 @@ mod tests { run_store, dry_run: false, emitter: Arc::clone(&emitter), - sandbox: SandboxSpec::Local { working_directory }, + sandbox: SandboxSpec::local(working_directory, ProviderAccess::default()), llm: LlmSpec { model: "test-model".to_string(), provider_id: lithos_llm::catalog::builtin::anthropic(), @@ -1387,9 +1386,7 @@ mod tests { run_store: run_store.into(), dry_run: false, emitter: emitter.clone(), - sandbox: SandboxSpec::Local { - working_directory: temp.path().to_path_buf(), - }, + sandbox: SandboxSpec::local(temp.path(), ProviderAccess::default()), llm: LlmSpec { model: "fake-acp".to_string(), provider_id: lithos_llm::catalog::builtin::openai(), @@ -1492,9 +1489,10 @@ mod tests { run_store: run_store.into(), dry_run: false, emitter: emitter.clone(), - sandbox: SandboxSpec::Local { - working_directory: std::env::current_dir().unwrap(), - }, + sandbox: SandboxSpec::local( + std::env::current_dir().unwrap(), + ProviderAccess::default(), + ), llm: LlmSpec { model: "test-model".to_string(), provider_id: lithos_llm::catalog::builtin::anthropic(), @@ -1636,9 +1634,10 @@ mod tests { }, dry_run: false, emitter: emitter.clone(), - sandbox: SandboxSpec::Local { - working_directory: std::env::current_dir().unwrap(), - }, + sandbox: SandboxSpec::local( + std::env::current_dir().unwrap(), + ProviderAccess::default(), + ), llm: LlmSpec { model: "test-model".to_string(), provider_id: lithos_llm::catalog::builtin::anthropic(), diff --git a/lib/components/fabro-workflow/tests/it/cp_integration.rs b/lib/components/fabro-workflow/tests/it/cp_integration.rs index df21ae5a3..6cdb19b43 100644 --- a/lib/components/fabro-workflow/tests/it/cp_integration.rs +++ b/lib/components/fabro-workflow/tests/it/cp_integration.rs @@ -15,6 +15,7 @@ )] use fabro_sandbox::reconnect::reconnect_for_run; +use fabro_sandbox::test_support::local_sandbox_id; use fabro_sandbox::{CloneRequest, ProviderAccess, provider_sandbox}; use fabro_types::{RunSandboxInstance, RunSandboxRuntime, SandboxProviderKind}; use sandbox_driver::{SandboxSource, SandboxSpec}; @@ -25,13 +26,13 @@ const DOCKER_CP_IMAGE: &str = "buildpack-deps:noble"; // Local sandbox // --------------------------------------------------------------------------- -fn local_record(working_directory: &std::path::Path) -> RunSandboxInstance { +async fn local_record(working_directory: &std::path::Path) -> RunSandboxInstance { RunSandboxInstance { provider: SandboxProviderKind::LOCAL, image: None, snapshot: None, runtime: RunSandboxRuntime { - id: "local:test".to_string(), + id: local_sandbox_id(working_directory).await, working_directory: working_directory.to_string_lossy().to_string(), repo_cloned: None, clone_origin_url: None, @@ -49,7 +50,7 @@ async fn local_cp_upload_download_round_trip() { let sandbox_dir = tempfile::tempdir().unwrap(); let scratch = tempfile::tempdir().unwrap(); - let record = local_record(sandbox_dir.path()); + let record = local_record(sandbox_dir.path()).await; let sandbox = reconnect_for_run(&record, &ProviderAccess::default(), None, None) .await .expect("reconnect local"); @@ -82,7 +83,7 @@ async fn local_cp_binary_round_trip() { let sandbox_dir = tempfile::tempdir().unwrap(); let scratch = tempfile::tempdir().unwrap(); - let record = local_record(sandbox_dir.path()); + let record = local_record(sandbox_dir.path()).await; let sandbox = reconnect_for_run(&record, &ProviderAccess::default(), None, None) .await .expect("reconnect local"); @@ -111,7 +112,7 @@ async fn local_cp_creates_parent_dirs() { let sandbox_dir = tempfile::tempdir().unwrap(); let scratch = tempfile::tempdir().unwrap(); - let record = local_record(sandbox_dir.path()); + let record = local_record(sandbox_dir.path()).await; let sandbox = reconnect_for_run(&record, &ProviderAccess::default(), None, None) .await .expect("reconnect local");