From a19878e5906f6678e9de72ce7fa53d76e4ddfe29 Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Fri, 3 Apr 2026 14:58:18 -0700 Subject: [PATCH] Remove SandboxRecordExt trait (save/load were unused in production) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The trait just wrapped serde_json + std::fs. Tests using it were testing serialization, not sandbox behavior — deleted those and simplified the daytona cp test to pass the record directly to reconnect. Co-Authored-By: Claude Opus 4.6 (1M context) --- lib/crates/fabro-sandbox/src/lib.rs | 2 +- .../fabro-sandbox/src/sandbox_record.rs | 25 -------- .../fabro-workflow/tests/it/cp_integration.rs | 57 +------------------ .../tests/it/daytona_integration.rs | 22 +++---- 4 files changed, 9 insertions(+), 97 deletions(-) diff --git a/lib/crates/fabro-sandbox/src/lib.rs b/lib/crates/fabro-sandbox/src/lib.rs index e647ce2ed..fec1dd390 100644 --- a/lib/crates/fabro-sandbox/src/lib.rs +++ b/lib/crates/fabro-sandbox/src/lib.rs @@ -39,7 +39,7 @@ pub use local::LocalSandbox; #[cfg(feature = "docker")] pub use docker::{DockerSandbox, DockerSandboxOptions}; -pub use sandbox_record::{SandboxRecord, SandboxRecordExt}; +pub use sandbox_record::SandboxRecord; #[cfg(feature = "daytona")] pub use daytona::detect_clone_params; diff --git a/lib/crates/fabro-sandbox/src/sandbox_record.rs b/lib/crates/fabro-sandbox/src/sandbox_record.rs index d8076dbf2..aa7251712 100644 --- a/lib/crates/fabro-sandbox/src/sandbox_record.rs +++ b/lib/crates/fabro-sandbox/src/sandbox_record.rs @@ -1,26 +1 @@ -use std::path::Path; - pub use fabro_types::sandbox_record::SandboxRecord; - -pub trait SandboxRecordExt { - fn save(&self, path: &Path) -> anyhow::Result<()>; - fn load(path: &Path) -> anyhow::Result - where - Self: Sized; -} - -impl SandboxRecordExt for SandboxRecord { - fn save(&self, path: &Path) -> anyhow::Result<()> { - let json = serde_json::to_string_pretty(self) - .map_err(|e| anyhow::anyhow!("sandbox_record serialize failed: {e}"))?; - std::fs::write(path, json)?; - Ok(()) - } - - fn load(path: &Path) -> anyhow::Result { - let data = std::fs::read_to_string(path) - .map_err(|e| anyhow::anyhow!("failed to read {}: {e}", path.display()))?; - serde_json::from_str(&data) - .map_err(|e| anyhow::anyhow!("sandbox_record deserialize failed: {e}")) - } -} diff --git a/lib/crates/fabro-workflow/tests/it/cp_integration.rs b/lib/crates/fabro-workflow/tests/it/cp_integration.rs index 034e5df9b..2b13d2ba7 100644 --- a/lib/crates/fabro-workflow/tests/it/cp_integration.rs +++ b/lib/crates/fabro-workflow/tests/it/cp_integration.rs @@ -6,8 +6,8 @@ #![allow(clippy::ignore_without_reason)] +use fabro_sandbox::SandboxRecord; use fabro_sandbox::reconnect::reconnect; -use fabro_sandbox::{SandboxRecord, SandboxRecordExt}; // --------------------------------------------------------------------------- // Local sandbox @@ -111,25 +111,6 @@ async fn local_cp_creates_parent_dirs() { assert_eq!(std::fs::read(&local_dst).unwrap(), content); } -#[tokio::test] -async fn local_cp_record_save_load_round_trip() { - let sandbox_dir = tempfile::tempdir().unwrap(); - let scratch = tempfile::tempdir().unwrap(); - - let record = local_record(sandbox_dir.path()); - let path = scratch.path().join("sandbox.json"); - record.save(&path).unwrap(); - - let loaded = SandboxRecord::load(&path).unwrap(); - assert_eq!(loaded.provider, "local"); - assert_eq!(loaded.working_directory, record.working_directory); - assert!(loaded.identifier.is_none()); - - // Reconnect from the loaded record still works - let sandbox = reconnect(&loaded).await.expect("reconnect from loaded"); - assert_eq!(sandbox.working_directory(), record.working_directory); -} - // --------------------------------------------------------------------------- // Docker sandbox // --------------------------------------------------------------------------- @@ -265,39 +246,3 @@ async fn docker_cp_custom_mount_point() { assert_eq!(std::fs::read(&local_dst).unwrap(), content); } - -#[tokio::test] -#[ignore] // requires Docker daemon -async fn docker_cp_record_save_load_round_trip() { - let host_dir = tempfile::tempdir().unwrap(); - let scratch = tempfile::tempdir().unwrap(); - - let record = docker_record(host_dir.path(), "/workspace"); - let path = scratch.path().join("sandbox.json"); - record.save(&path).unwrap(); - - let loaded = SandboxRecord::load(&path).unwrap(); - assert_eq!(loaded.provider, "docker"); - assert_eq!( - loaded.host_working_directory.as_deref(), - Some(host_dir.path().to_str().unwrap()) - ); - assert_eq!(loaded.container_mount_point.as_deref(), Some("/workspace")); - - // Reconnect from the loaded record still works - let sandbox = reconnect(&loaded).await.expect("reconnect from loaded"); - - let content = b"from loaded record\n"; - let local_src = scratch.path().join("loaded.txt"); - std::fs::write(&local_src, content).unwrap(); - - sandbox - .upload_file_from_local(&local_src, "loaded.txt") - .await - .expect("upload via loaded record"); - - assert_eq!( - std::fs::read(host_dir.path().join("loaded.txt")).unwrap(), - content - ); -} diff --git a/lib/crates/fabro-workflow/tests/it/daytona_integration.rs b/lib/crates/fabro-workflow/tests/it/daytona_integration.rs index 4e0e19589..336bfb14b 100644 --- a/lib/crates/fabro-workflow/tests/it/daytona_integration.rs +++ b/lib/crates/fabro-workflow/tests/it/daytona_integration.rs @@ -20,7 +20,6 @@ use std::sync::Arc; use fabro_agent::Sandbox; use fabro_graphviz::graph::{AttrValue, Edge, Graph, Node}; use fabro_llm::provider::Provider; -use fabro_sandbox::SandboxRecordExt; use fabro_sandbox::daytona::{DaytonaConfig, DaytonaSandbox, DaytonaSnapshotConfig}; use fabro_store::RuntimeState; use fabro_types::{RunId, Settings}; @@ -1699,7 +1698,7 @@ async fn daytona_toolbox_idle_diagnostic() { /// E2E test for `fabro cp` against a live Daytona sandbox. /// -/// Creates a sandbox, saves a SandboxRecord, reconnects via `cp::reconnect`, +/// Creates a sandbox, reconnects via `cp::reconnect`, /// uploads a file, downloads it back, and verifies the round-trip. #[fabro_macros::e2e_test(live("DAYTONA_API_KEY"), live("GITHUB_APP_PRIVATE_KEY"))] async fn daytona_cp_upload_download_round_trip() { @@ -1725,18 +1724,11 @@ async fn daytona_cp_upload_download_round_trip() { container_mount_point: None, }; - // 3. Save to temp dir and reload (verify serialization round-trip) + // 3. Reconnect via the real cp::reconnect path let tmp = tempfile::tempdir().unwrap(); - let record_path = tmp.path().join("sandbox.json"); - record.save(&record_path).unwrap(); - let loaded = SandboxRecord::load(&record_path).unwrap(); - assert_eq!(loaded.provider, "daytona"); - assert_eq!(loaded.identifier.as_deref(), Some(sandbox_name.as_str())); + let reconnected = reconnect(&record).await.expect("reconnect should succeed"); - // 4. Reconnect via the real cp::reconnect path - let reconnected = reconnect(&loaded).await.expect("reconnect should succeed"); - - // 5. Upload: write a local file, then upload it to the sandbox + // 4. Upload: write a local file, then upload it to the sandbox let upload_content = b"hello from fabro cp e2e test\n"; let local_upload = tmp.path().join("upload.txt"); std::fs::write(&local_upload, upload_content).unwrap(); @@ -1746,7 +1738,7 @@ async fn daytona_cp_upload_download_round_trip() { .await .expect("upload_file_from_local should succeed"); - // 6. Verify the file exists in the sandbox via the original connection + // 5. Verify the file exists in the sandbox via the original connection assert!( env.file_exists("cp_test_upload.txt").await.unwrap(), "uploaded file should exist in the sandbox" @@ -1760,7 +1752,7 @@ async fn daytona_cp_upload_download_round_trip() { "expected uploaded content in sandbox, got: {remote_content}" ); - // 7. Download: retrieve the file back to local via the reconnected sandbox + // 6. Download: retrieve the file back to local via the reconnected sandbox let local_download = tmp.path().join("download.txt"); reconnected .download_file_to_local("cp_test_upload.txt", &local_download) @@ -1770,7 +1762,7 @@ async fn daytona_cp_upload_download_round_trip() { let downloaded = std::fs::read(&local_download).unwrap(); assert_eq!(downloaded, upload_content); - // 8. Upload a binary file to test non-UTF-8 content + // 7. Upload a binary file to test non-UTF-8 content let binary_content: Vec = (0..=255).collect(); let local_binary = tmp.path().join("binary.bin"); std::fs::write(&local_binary, &binary_content).unwrap();