From 50847ebc6f73710d38cfc30df1ef084b3b93ae6e Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Mon, 27 Apr 2026 10:41:39 -0700 Subject: [PATCH] fix(sandbox): preserve error chains Introduce typed sandbox errors and carry source causes through workflow events, persisted failure summaries, and API/CLI string boundaries so Docker client failures keep the actionable underlying cause. --- Cargo.lock | 3 + lib/crates/fabro-agent/src/session.rs | 15 +- lib/crates/fabro-agent/src/tools.rs | 54 ++- lib/crates/fabro-agent/src/v4a_patch.rs | 19 +- .../fabro-cli/src/commands/run/runner.rs | 2 + lib/crates/fabro-sandbox/Cargo.toml | 2 + lib/crates/fabro-sandbox/src/clone_source.rs | 6 +- lib/crates/fabro-sandbox/src/daytona/mod.rs | 330 ++++++++++-------- lib/crates/fabro-sandbox/src/docker.rs | 286 ++++++++------- lib/crates/fabro-sandbox/src/error.rs | 94 +++++ lib/crates/fabro-sandbox/src/lib.rs | 2 + lib/crates/fabro-sandbox/src/local.rs | 96 ++--- lib/crates/fabro-sandbox/src/read_guard.rs | 10 +- lib/crates/fabro-sandbox/src/sandbox.rs | 135 ++++--- lib/crates/fabro-sandbox/src/test_support.rs | 79 +++-- lib/crates/fabro-sandbox/src/worktree.rs | 42 +-- lib/crates/fabro-sandbox/tests/error.rs | 32 ++ lib/crates/fabro-server/src/run_files.rs | 2 +- lib/crates/fabro-server/src/server.rs | 27 +- lib/crates/fabro-store/Cargo.toml | 1 + lib/crates/fabro-store/src/run_state.rs | 35 +- lib/crates/fabro-types/src/run_event/infra.rs | 16 +- lib/crates/fabro-types/src/run_event/run.rs | 2 + lib/crates/fabro-util/src/error.rs | 28 ++ lib/crates/fabro-util/src/lib.rs | 1 + lib/crates/fabro-util/tests/error_chain.rs | 53 +++ lib/crates/fabro-workflow/src/artifact.rs | 40 +-- .../fabro-workflow/src/artifact_snapshot.rs | 41 +-- .../fabro-workflow/src/devcontainer_bridge.rs | 31 +- lib/crates/fabro-workflow/src/error.rs | 104 +++++- lib/crates/fabro-workflow/src/event.rs | 104 +++++- .../fabro-workflow/src/handler/command.rs | 32 +- .../fabro-workflow/src/handler/llm/cli.rs | 40 ++- .../fabro-workflow/src/pipeline/finalize.rs | 8 +- .../fabro-workflow/src/pipeline/initialize.rs | 2 +- lib/crates/fabro-workflow/src/sandbox_git.rs | 50 ++- .../tests/it/daytona_integration.rs | 2 +- .../fabro-workflow/tests/it/integration.rs | 116 +++--- 38 files changed, 1308 insertions(+), 634 deletions(-) create mode 100644 lib/crates/fabro-sandbox/src/error.rs create mode 100644 lib/crates/fabro-sandbox/tests/error.rs create mode 100644 lib/crates/fabro-util/src/error.rs create mode 100644 lib/crates/fabro-util/tests/error_chain.rs diff --git a/Cargo.lock b/Cargo.lock index d037d2e18..45c7f6a19 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2067,6 +2067,7 @@ dependencies = [ "fabro-redact", "fabro-static", "fabro-types", + "fabro-util", "futures", "git2", "glob", @@ -2077,6 +2078,7 @@ dependencies = [ "strum", "tar", "tempfile", + "thiserror 2.0.18", "tokio", "tokio-util", "toml 0.8.23", @@ -2204,6 +2206,7 @@ dependencies = [ "chrono", "dashmap", "fabro-types", + "fabro-util", "futures", "hex", "insta", diff --git a/lib/crates/fabro-agent/src/session.rs b/lib/crates/fabro-agent/src/session.rs index 0a814f6be..6273703eb 100644 --- a/lib/crates/fabro-agent/src/session.rs +++ b/lib/crates/fabro-agent/src/session.rs @@ -299,7 +299,7 @@ impl Session { let launch_result = sandbox .exec_command(&launch_script, 30_000, None, env_ref, None) .await - .map_err(|e| format!("Failed to launch MCP server: {e}"))?; + .map_err(|e| format!("Failed to launch MCP server: {}", e.display_with_causes()))?; let pid = launch_result.stdout.trim(); info!(pid, port, "MCP server process launched in sandbox"); @@ -311,7 +311,12 @@ impl Session { let poll_result = sandbox .exec_command(&poll_cmd, 60_000, None, None, None) .await - .map_err(|e| format!("Failed to poll MCP server readiness: {e}"))?; + .map_err(|e| { + format!( + "Failed to poll MCP server readiness: {}", + e.display_with_causes() + ) + })?; if poll_result.stdout.trim() != "ready" { // Grab stderr for debugging @@ -333,7 +338,11 @@ impl Session { // Get the preview URL for the port, or fall back to localhost for local // sandboxes - if let Some(url_and_headers) = sandbox.get_preview_url(port).await? { + if let Some(url_and_headers) = sandbox + .get_preview_url(port) + .await + .map_err(|e| e.display_with_causes())? + { Ok(url_and_headers) } else { info!(port, "No preview URL available, using localhost"); diff --git a/lib/crates/fabro-agent/src/tools.rs b/lib/crates/fabro-agent/src/tools.rs index 6394982d9..948d2fc80 100644 --- a/lib/crates/fabro-agent/src/tools.rs +++ b/lib/crates/fabro-agent/src/tools.rs @@ -13,6 +13,10 @@ use crate::tool_registry::{RegisteredTool, ToolRegistry}; const MAX_WEB_FETCH_BYTES: usize = 100 * 1024; +fn sandbox_error(error: fabro_sandbox::Error) -> String { + error.display_with_causes() +} + /// Configuration for the optional LLM-based summarizer used by `web_fetch`. #[derive(Clone)] pub struct WebFetchSummarizer { @@ -103,7 +107,8 @@ pub fn make_read_file_tool() -> RegisteredTool { let content = ctx .env .read_file(file_path, offset_usize, limit_usize) - .await?; + .await + .map_err(sandbox_error)?; ctx.env.mark_agent_read(file_path); Ok(content) }) @@ -131,7 +136,10 @@ pub fn make_write_file_tool() -> RegisteredTool { let file_path = required_str(&args, "file_path")?; let content = required_str(&args, "content")?; - ctx.env.write_file(file_path, content).await?; + ctx.env + .write_file(file_path, content) + .await + .map_err(sandbox_error)?; Ok(format!("Successfully wrote to {file_path}")) }) }), @@ -165,7 +173,11 @@ pub fn make_edit_file_tool() -> RegisteredTool { .and_then(serde_json::Value::as_bool) .unwrap_or(false); - let numbered_content = ctx.env.read_file(file_path, None, None).await?; + let numbered_content = ctx + .env + .read_file(file_path, None, None) + .await + .map_err(sandbox_error)?; // Strip line numbers: each line looks like " 1 | content" or " 10 | content" let raw_lines: Vec<&str> = numbered_content @@ -190,7 +202,10 @@ pub fn make_edit_file_tool() -> RegisteredTool { raw_content.replacen(old_string, new_string, 1) }; - ctx.env.write_file(file_path, &new_content).await?; + ctx.env + .write_file(file_path, &new_content) + .await + .map_err(sandbox_error)?; Ok(format!("Successfully edited {file_path}")) }) }), @@ -245,7 +260,8 @@ pub fn make_shell_tool_with_config(config: &SessionOptions) -> RegisteredTool { ctx.tool_env.as_ref(), Some(ctx.cancel), ) - .await?; + .await + .map_err(sandbox_error)?; let mut output = String::new(); if result.timed_out { @@ -307,7 +323,11 @@ pub fn make_grep_tool() -> RegisteredTool { max_results, }; - let results = ctx.env.grep(pattern, path, &options).await?; + let results = ctx + .env + .grep(pattern, path, &options) + .await + .map_err(sandbox_error)?; let mut seen_files = std::collections::HashSet::new(); for line in &results { if let Some(file_path) = line.split(':').next() { @@ -342,7 +362,7 @@ pub fn make_glob_tool() -> RegisteredTool { let pattern = required_str(&args, "pattern")?; let path = args.get("path").and_then(serde_json::Value::as_str); - let results = ctx.env.glob(pattern, path).await?; + let results = ctx.env.glob(pattern, path).await.map_err(sandbox_error)?; Ok(results.join("\n")) }) }), @@ -414,7 +434,11 @@ pub(crate) fn make_list_dir_tool() -> RegisteredTool { let path = required_str(&args, "path")?; let depth = optional_usize_arg(&args, "depth")?; - let entries = ctx.env.list_directory(path, depth).await?; + let entries = ctx + .env + .list_directory(path, depth) + .await + .map_err(sandbox_error)?; let lines: Vec = entries .iter() .map(|e| { @@ -579,9 +603,17 @@ pub(crate) fn make_web_fetch_tool(summarizer: Option) -> Reg "curl -sL --max-time {timeout_secs} -H 'User-Agent: fabro-agent/0.1' {escaped_url}" ); - let result = ctx.env - .exec_command(&command, timeout_ms, None, ctx.tool_env.as_ref(), Some(ctx.cancel)) - .await?; + let result = ctx + .env + .exec_command( + &command, + timeout_ms, + None, + ctx.tool_env.as_ref(), + Some(ctx.cancel), + ) + .await + .map_err(sandbox_error)?; if result.exit_code != 0 { return Err(format!( diff --git a/lib/crates/fabro-agent/src/v4a_patch.rs b/lib/crates/fabro-agent/src/v4a_patch.rs index eb45971b1..41875c789 100644 --- a/lib/crates/fabro-agent/src/v4a_patch.rs +++ b/lib/crates/fabro-agent/src/v4a_patch.rs @@ -6,6 +6,10 @@ use crate::sandbox::{Sandbox, format_lines_numbered}; use crate::tool_registry::RegisteredTool; use crate::truncation::{TruncationMode, truncate_output}; +fn sandbox_error(error: fabro_sandbox::Error) -> String { + error.display_with_causes() +} + #[derive(Debug, Clone, PartialEq, Eq)] pub enum Change { Remove(String), @@ -205,11 +209,11 @@ pub async fn apply_patch_operations( for op in ops { match op { PatchOperation::Add { path, content } => { - env.write_file(path, content).await?; + env.write_file(path, content).await.map_err(sandbox_error)?; results.push(format!("Added file: {path}")); } PatchOperation::Delete { path } => { - env.delete_file(path).await?; + env.delete_file(path).await.map_err(sandbox_error)?; results.push(format!("Deleted file: {path}")); } PatchOperation::Update { @@ -217,13 +221,18 @@ pub async fn apply_patch_operations( new_path, hunks, } => { - let original = env.read_file(path, None, None).await?; + let original = env + .read_file(path, None, None) + .await + .map_err(sandbox_error)?; let updated = apply_hunks(&original, hunks) .map_err(|err| format_patch_error(&err, path, &original))?; let dest = new_path.as_deref().unwrap_or(path); - env.write_file(dest, &updated).await?; + env.write_file(dest, &updated) + .await + .map_err(sandbox_error)?; if new_path.is_some() { - env.delete_file(path).await?; + env.delete_file(path).await.map_err(sandbox_error)?; results.push(format!("Moved file: {path} → {dest}")); } else { results.push(format!("Updated file: {path}")); diff --git a/lib/crates/fabro-cli/src/commands/run/runner.rs b/lib/crates/fabro-cli/src/commands/run/runner.rs index 27b4f753d..3596dfa24 100644 --- a/lib/crates/fabro-cli/src/commands/run/runner.rs +++ b/lib/crates/fabro-cli/src/commands/run/runner.rs @@ -690,6 +690,7 @@ mod tests { assert_eq!( worker_title_phase_for_event(&EventBody::RunFailed(RunFailedProps { error: "cancelled".to_string(), + causes: Vec::new(), duration_ms: 10, reason: FailureReason::Cancelled, git_commit_sha: None, @@ -700,6 +701,7 @@ mod tests { assert_eq!( worker_title_phase_for_event(&EventBody::RunFailed(RunFailedProps { error: "boom".to_string(), + causes: Vec::new(), duration_ms: 10, reason: FailureReason::Terminated, git_commit_sha: None, diff --git a/lib/crates/fabro-sandbox/Cargo.toml b/lib/crates/fabro-sandbox/Cargo.toml index 83ed3808b..8ad0add49 100644 --- a/lib/crates/fabro-sandbox/Cargo.toml +++ b/lib/crates/fabro-sandbox/Cargo.toml @@ -22,6 +22,7 @@ workspace = true [dependencies] anyhow.workspace = true async-trait.workspace = true +thiserror.workspace = true tokio.workspace = true tokio-util.workspace = true serde.workspace = true @@ -31,6 +32,7 @@ tracing.workspace = true base64.workspace = true fabro-proc = { path = "../fabro-proc" } fabro-static.workspace = true +fabro-util = { path = "../fabro-util" } fabro-redact.workspace = true shlex = "1" diff --git a/lib/crates/fabro-sandbox/src/clone_source.rs b/lib/crates/fabro-sandbox/src/clone_source.rs index 5e20da362..e36b0512b 100644 --- a/lib/crates/fabro-sandbox/src/clone_source.rs +++ b/lib/crates/fabro-sandbox/src/clone_source.rs @@ -30,7 +30,7 @@ pub(crate) fn decide_clone( skip_clone: bool, clone_origin_url: Option<&str>, clone_branch: Option<&str>, -) -> Result { +) -> crate::Result { if skip_clone { return Ok(CloneDecision::EmptyWorkspace { reason: EmptyWorkspaceReason::SkipClone, @@ -45,9 +45,9 @@ pub(crate) fn decide_clone( 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!( + return Err(crate::Error::message(format!( "Clone-based sandboxes currently support GitHub repository origins only: {err}" - )); + ))); } Ok(CloneDecision::GitHub { diff --git a/lib/crates/fabro-sandbox/src/daytona/mod.rs b/lib/crates/fabro-sandbox/src/daytona/mod.rs index 68db9f568..6fbef8471 100644 --- a/lib/crates/fabro-sandbox/src/daytona/mod.rs +++ b/lib/crates/fabro-sandbox/src/daytona/mod.rs @@ -75,10 +75,10 @@ impl DaytonaSandbox { clone_origin_url: Option, clone_branch: Option, api_key: Option, - ) -> Result { + ) -> crate::Result { let client = build_daytona_client(api_key) .await - .map_err(|e| format!("Failed to create Daytona client: {e}"))?; + .map_err(|e| crate::Error::context("Failed to create Daytona client", e))?; Ok(Self { config, client, @@ -104,14 +104,16 @@ impl DaytonaSandbox { repo_cloned: bool, clone_origin_url: Option, clone_branch: Option, - ) -> Result { + ) -> crate::Result { let client = build_daytona_client(api_key) .await - .map_err(|e| format!("Failed to create Daytona client: {e}"))?; - let sdk_sandbox = client - .get(sandbox_name) - .await - .map_err(|e| format!("Failed to reconnect to Daytona sandbox '{sandbox_name}': {e}"))?; + .map_err(|e| crate::Error::context("Failed to create Daytona client", e))?; + let sdk_sandbox = client.get(sandbox_name).await.map_err(|e| { + crate::Error::context( + format!("Failed to reconnect to Daytona sandbox '{sandbox_name}'"), + e, + ) + })?; let sandbox_cell = OnceCell::new(); let _ = sandbox_cell.set(sdk_sandbox); let origin_url = OnceCell::new(); @@ -144,31 +146,30 @@ impl DaytonaSandbox { /// Get the `ComputerUseService` for this sandbox. /// /// Requires the sandbox to be initialized first. - pub async fn computer_use(&self) -> Result { + pub async fn computer_use(&self) -> crate::Result { let sandbox = self.sandbox()?; sandbox .computer_use() .await - .map_err(|e| format!("Failed to get computer use service: {e}")) + .map_err(|e| crate::Error::context("Failed to get computer use service", e)) } /// Create SSH access and return the connection command string. - pub async fn create_ssh_access(&self, ttl_minutes: Option) -> Result { + pub async fn create_ssh_access(&self, ttl_minutes: Option) -> crate::Result { let sandbox = self.sandbox()?; let dto = sandbox .create_ssh_access(ttl_minutes) .await - .map_err(|e| format!("Failed to create SSH access: {e}"))?; + .map_err(|e| crate::Error::context("Failed to create SSH access", e))?; Ok(dto.ssh_command) } /// Get a preview link (URL + token) for a port on this sandbox. - pub async fn get_preview_link(&self, port: u16) -> Result { + pub async fn get_preview_link(&self, port: u16) -> crate::Result { let sandbox = self.sandbox()?; - sandbox - .get_preview_link(port) - .await - .map_err(|e| format!("Failed to get preview link for port {port}: {e}")) + sandbox.get_preview_link(port).await.map_err(|e| { + crate::Error::context(format!("Failed to get preview link for port {port}"), e) + }) } /// Get a signed preview URL for a port on this sandbox. @@ -176,12 +177,17 @@ impl DaytonaSandbox { &self, port: u16, expires_in_seconds: Option, - ) -> Result { + ) -> crate::Result { let sandbox = self.sandbox()?; sandbox .get_signed_preview_url(i32::from(port), expires_in_seconds) .await - .map_err(|e| format!("Failed to get signed preview URL for port {port}: {e}")) + .map_err(|e| { + crate::Error::context( + format!("Failed to get signed preview URL for port {port}"), + e, + ) + }) } fn emit(&self, event: SandboxEvent) { @@ -200,10 +206,10 @@ impl DaytonaSandbox { } /// Get the sandbox, returning an error if not yet initialized. - fn sandbox(&self) -> Result<&daytona_sdk::Sandbox, String> { - self.sandbox - .get() - .ok_or_else(|| "Daytona sandbox not initialized — call initialize() first".to_string()) + fn sandbox(&self) -> crate::Result<&daytona_sdk::Sandbox> { + self.sandbox.get().ok_or_else(|| { + crate::Error::message("Daytona sandbox not initialized — call initialize() first") + }) } fn repo_cloned(&self) -> bool { @@ -243,19 +249,19 @@ impl DaytonaSandbox { /// If the snapshot doesn't exist and a dockerfile is provided, creates it /// and polls until it reaches `Active` state. Returns an error if the /// snapshot is in a terminal failure state. - async fn ensure_snapshot(&self, snap_cfg: &DaytonaSnapshotConfig) -> Result<(), String> { + async fn ensure_snapshot(&self, snap_cfg: &DaytonaSnapshotConfig) -> crate::Result<()> { match self.client.snapshot.get(&snap_cfg.name).await { Ok(dto) => { use daytona_api_client::models::SnapshotState; match dto.state { SnapshotState::Active => return Ok(()), SnapshotState::Error | SnapshotState::BuildFailed => { - return Err(format!( + return Err(crate::Error::message(format!( "Snapshot '{}' is in state '{}': {}", snap_cfg.name, dto.state, dto.error_reason.unwrap_or_default() - )); + ))); } _ => { // Building/Pending/Pulling — fall through to poll @@ -266,16 +272,16 @@ impl DaytonaSandbox { let dockerfile = match &snap_cfg.dockerfile { Some(DockerfileSource::Inline(s)) => s.as_str(), Some(DockerfileSource::Path { .. }) => { - return Err(format!( + return Err(crate::Error::message(format!( "Snapshot '{}': dockerfile path should have been resolved to inline content before sandbox creation", snap_cfg.name - )); + ))); } None => { - return Err(format!( + return Err(crate::Error::message(format!( "Snapshot '{}' does not exist and no dockerfile provided to create it", snap_cfg.name - )); + ))); } }; @@ -292,14 +298,18 @@ impl DaytonaSandbox { }), entrypoint: None, }; - self.client - .snapshot - .create(¶ms) - .await - .map_err(|e| format!("Failed to create snapshot '{}': {e}", snap_cfg.name))?; + self.client.snapshot.create(¶ms).await.map_err(|e| { + crate::Error::context( + format!("Failed to create snapshot '{}'", snap_cfg.name), + e, + ) + })?; } Err(e) => { - return Err(format!("Failed to get snapshot '{}': {e}", snap_cfg.name)); + return Err(crate::Error::context( + format!("Failed to get snapshot '{}'", snap_cfg.name), + e, + )); } } @@ -309,7 +319,7 @@ impl DaytonaSandbox { /// Poll a snapshot until it reaches `Active` state, with exponential /// back-off. - async fn poll_snapshot_active(&self, name: &str) -> Result<(), String> { + async fn poll_snapshot_active(&self, name: &str) -> crate::Result<()> { use daytona_api_client::models::SnapshotState; let mut delay = std::time::Duration::from_secs(2); let max_delay = std::time::Duration::from_secs(30); @@ -317,21 +327,18 @@ impl DaytonaSandbox { while Instant::now() < deadline { time::sleep(delay).await; - let dto = self - .client - .snapshot - .get(name) - .await - .map_err(|e| format!("Failed to poll snapshot '{name}': {e}"))?; + let dto = self.client.snapshot.get(name).await.map_err(|e| { + crate::Error::context(format!("Failed to poll snapshot '{name}'"), e) + })?; match dto.state { SnapshotState::Active => return Ok(()), SnapshotState::Error | SnapshotState::BuildFailed => { - return Err(format!( + return Err(crate::Error::message(format!( "Snapshot '{name}' failed ({}): {}", dto.state, dto.error_reason.unwrap_or_default() - )); + ))); } _ => { delay = (delay * 2).min(max_delay); @@ -339,9 +346,9 @@ impl DaytonaSandbox { } } - Err(format!( + Err(crate::Error::message(format!( "Timed out waiting for snapshot '{name}' to become active" - )) + ))) } } @@ -349,15 +356,19 @@ impl DaytonaSandbox { /// /// Uses `git2` to discover the repo at `path`, reads the `origin` remote URL /// and the HEAD branch name. -pub fn detect_repo_info(path: &Path) -> Result<(String, Option), String> { - let repo = git2::Repository::discover(path) - .map_err(|e| format!("Failed to discover git repo at {}: {e}", path.display()))?; +pub fn detect_repo_info(path: &Path) -> crate::Result<(String, Option)> { + let repo = git2::Repository::discover(path).map_err(|e| { + crate::Error::context( + format!("Failed to discover git repo at {}", path.display()), + e, + ) + })?; let url = repo .find_remote("origin") - .map_err(|e| format!("Failed to find 'origin' remote: {e}"))? + .map_err(|e| crate::Error::context("Failed to find 'origin' remote", e))? .url() - .ok_or_else(|| "origin remote URL is not valid UTF-8".to_string())? + .ok_or_else(|| crate::Error::message("origin remote URL is not valid UTF-8"))? .to_string(); let branch = repo @@ -374,28 +385,28 @@ impl Sandbox for DaytonaSandbox { &self, remote_path: &str, local_path: &Path, - ) -> Result<(), String> { + ) -> crate::Result<()> { let sandbox = self.sandbox()?; let resolved = self.resolve_path(remote_path); let fs_svc = sandbox .fs() .await - .map_err(|e| format!("Failed to get fs service: {e}"))?; + .map_err(|e| crate::Error::context("Failed to get fs service", e))?; let bytes = fs_svc .download_file(&resolved) .await - .map_err(|e| format!("Failed to download file {resolved}: {e}"))?; + .map_err(|e| crate::Error::context(format!("Failed to download file {resolved}"), e))?; if let Some(parent) = local_path.parent() { fs::create_dir_all(parent) .await - .map_err(|e| format!("Failed to create parent dirs: {e}"))?; + .map_err(|e| crate::Error::context("Failed to create parent dirs", e))?; } - fs::write(local_path, &bytes) - .await - .map_err(|e| format!("Failed to write {}: {e}", local_path.display()))?; + fs::write(local_path, &bytes).await.map_err(|e| { + crate::Error::context(format!("Failed to write {}", local_path.display()), e) + })?; Ok(()) } @@ -404,7 +415,7 @@ impl Sandbox for DaytonaSandbox { &self, local_path: &Path, remote_path: &str, - ) -> Result<(), String> { + ) -> crate::Result<()> { let sandbox = self.sandbox()?; let resolved = self.resolve_path(remote_path); @@ -415,29 +426,29 @@ impl Sandbox for DaytonaSandbox { let fs_svc = sandbox .fs() .await - .map_err(|e| format!("Failed to get fs service: {e}"))?; + .map_err(|e| crate::Error::context("Failed to get fs service", e))?; let _ = fs_svc.create_folder(&parent_str, None).await; } } - let bytes = fs::read(local_path) - .await - .map_err(|e| format!("Failed to read {}: {e}", local_path.display()))?; + let bytes = fs::read(local_path).await.map_err(|e| { + crate::Error::context(format!("Failed to read {}", local_path.display()), e) + })?; let fs_svc = sandbox .fs() .await - .map_err(|e| format!("Failed to get fs service: {e}"))?; + .map_err(|e| crate::Error::context("Failed to get fs service", e))?; fs_svc .upload_file_bytes(&resolved, &bytes) .await - .map_err(|e| format!("Failed to upload file {resolved}: {e}"))?; + .map_err(|e| crate::Error::context(format!("Failed to upload file {resolved}"), e))?; Ok(()) } - async fn initialize(&self) -> Result<(), String> { + async fn initialize(&self) -> crate::Result<()> { self.emit(SandboxEvent::Initializing { provider: "daytona".into(), }); @@ -450,14 +461,16 @@ impl Sandbox for DaytonaSandbox { let snap_start = Instant::now(); if let Err(e) = self.ensure_snapshot(snap_cfg).await { self.emit(SandboxEvent::SnapshotFailed { - name: snap_cfg.name.clone(), - error: e.clone(), + name: snap_cfg.name.clone(), + error: e.to_string(), + causes: e.causes(), }); let duration_ms = u64::try_from(init_start.elapsed().as_millis()).unwrap_or(u64::MAX); self.emit(SandboxEvent::InitializeFailed { provider: "daytona".into(), - error: e.clone(), + error: e.to_string(), + causes: e.causes(), duration_ms, }); return Err(e); @@ -485,12 +498,13 @@ impl Sandbox for DaytonaSandbox { .create(params, daytona_sdk::CreateSandboxOptions::default()) .await .map_err(|e| { - let err = format!("Failed to create Daytona sandbox: {e}"); + let err = crate::Error::context("Failed to create Daytona sandbox", e); let duration_ms = u64::try_from(init_start.elapsed().as_millis()).unwrap_or(u64::MAX); self.emit(SandboxEvent::InitializeFailed { provider: "daytona".into(), - error: err.clone(), + error: err.to_string(), + causes: err.causes(), duration_ms, }); err @@ -507,7 +521,8 @@ impl Sandbox for DaytonaSandbox { u64::try_from(init_start.elapsed().as_millis()).unwrap_or(u64::MAX); self.emit(SandboxEvent::InitializeFailed { provider: "daytona".into(), - error: e.clone(), + error: e.to_string(), + causes: e.causes(), duration_ms, }); return Err(e); @@ -526,11 +541,11 @@ impl Sandbox for DaytonaSandbox { let fs_svc = sandbox .fs() .await - .map_err(|e| format!("Failed to get Daytona fs service: {e}"))?; + .map_err(|e| crate::Error::context("Failed to get Daytona fs service", e))?; fs_svc .create_folder(WORKING_DIRECTORY, None) .await - .map_err(|e| format!("Failed to create working directory: {e}"))?; + .map_err(|e| crate::Error::context("Failed to create working directory", e))?; let _ = self.repo_cloned.set(false); } CloneDecision::GitHub { origin_url, branch } => { @@ -544,10 +559,13 @@ impl Sandbox for DaytonaSandbox { Some(creds) => { let (owner, repo) = fabro_github::parse_github_owner_repo(&origin_url) .map_err(|e| { - let err = format!("Failed to parse GitHub URL for clone: {e}"); + let err = crate::Error::message(format!( + "Failed to parse GitHub URL for clone: {e}" + )); self.emit(SandboxEvent::GitCloneFailed { - url: origin_url.clone(), - error: err.clone(), + url: origin_url.clone(), + error: err.to_string(), + causes: err.causes(), }); err })?; @@ -561,17 +579,20 @@ impl Sandbox for DaytonaSandbox { ) .await .map_err(|e| { - let err = - format!("Failed to get GitHub App credentials for clone: {e}"); + let err = crate::Error::message(format!( + "Failed to get GitHub App credentials for clone: {e}" + )); self.emit(SandboxEvent::GitCloneFailed { - url: origin_url.clone(), - error: err.clone(), + url: origin_url.clone(), + error: err.to_string(), + causes: err.causes(), }); let duration_ms = u64::try_from(init_start.elapsed().as_millis()).unwrap_or(u64::MAX); self.emit(SandboxEvent::InitializeFailed { provider: "daytona".into(), - error: err.clone(), + error: err.to_string(), + causes: err.causes(), duration_ms, }); err @@ -583,19 +604,21 @@ impl Sandbox for DaytonaSandbox { let git_svc = sandbox .git() .await - .map_err(|e| format!("Failed to get Daytona git service: {e}")); + .map_err(|e| crate::Error::context("Failed to get Daytona git service", e)); let git_svc = match git_svc { Ok(g) => g, Err(e) => { self.emit(SandboxEvent::GitCloneFailed { - url: origin_url.clone(), - error: e.clone(), + url: origin_url.clone(), + error: e.to_string(), + causes: e.causes(), }); let duration_ms = u64::try_from(init_start.elapsed().as_millis()).unwrap_or(u64::MAX); self.emit(SandboxEvent::InitializeFailed { provider: "daytona".into(), - error: e.clone(), + error: e.to_string(), + causes: e.causes(), duration_ms, }); return Err(e); @@ -661,35 +684,41 @@ impl Sandbox for DaytonaSandbox { } } Err(e) if self.github_app.is_none() => { - let err = format!( - "Git clone failed: {e}. If this is a private repository, \ + let err = crate::Error::context( + "Git clone failed. If this is a private repository, \ configure a GitHub App with `fabro install` and install it \ - for your organization." + for your organization.", + e, ); self.emit(SandboxEvent::GitCloneFailed { - url: origin_url, - error: err.clone(), + url: origin_url, + error: err.to_string(), + causes: err.causes(), }); let duration_ms = u64::try_from(init_start.elapsed().as_millis()).unwrap_or(u64::MAX); self.emit(SandboxEvent::InitializeFailed { provider: "daytona".into(), - error: err.clone(), + error: err.to_string(), + causes: err.causes(), duration_ms, }); return Err(err); } Err(e) => { - let err = format!("Failed to clone repo into Daytona sandbox: {e}"); + let err = + crate::Error::context("Failed to clone repo into Daytona sandbox", e); self.emit(SandboxEvent::GitCloneFailed { - url: origin_url, - error: err.clone(), + url: origin_url, + error: err.to_string(), + causes: err.causes(), }); let duration_ms = u64::try_from(init_start.elapsed().as_millis()).unwrap_or(u64::MAX); self.emit(SandboxEvent::InitializeFailed { provider: "daytona".into(), - error: err.clone(), + error: err.to_string(), + causes: err.causes(), duration_ms, }); return Err(err); @@ -703,7 +732,7 @@ impl Sandbox for DaytonaSandbox { let sandbox_memory = sandbox.memory; self.sandbox .set(sandbox) - .map_err(|_| "Daytona sandbox already initialized".to_string())?; + .map_err(|_| crate::Error::message("Daytona sandbox already initialized"))?; tracing::info!("Daytona sandbox ready"); let init_duration = u64::try_from(init_start.elapsed().as_millis()).unwrap_or(u64::MAX); @@ -719,7 +748,7 @@ impl Sandbox for DaytonaSandbox { Ok(()) } - async fn cleanup(&self) -> Result<(), String> { + async fn cleanup(&self) -> crate::Result<()> { self.emit(SandboxEvent::CleanupStarted { provider: "daytona".into(), }); @@ -727,10 +756,11 @@ impl Sandbox for DaytonaSandbox { if let Some(sandbox) = self.sandbox.get() { tracing::info!("Destroying Daytona sandbox"); if let Err(e) = sandbox.delete().await { - let err = format!("Failed to delete Daytona sandbox: {e}"); + let err = crate::Error::context("Failed to delete Daytona sandbox", e); self.emit(SandboxEvent::CleanupFailed { provider: "daytona".into(), - error: err.clone(), + error: err.to_string(), + causes: err.causes(), }); return Err(err); } @@ -762,7 +792,7 @@ impl Sandbox for DaytonaSandbox { .unwrap_or_default() } - async fn setup_git_for_run(&self, run_id: &str) -> Result, String> { + async fn setup_git_for_run(&self, run_id: &str) -> crate::Result> { if !self.repo_cloned() { return Ok(None); } @@ -803,7 +833,7 @@ impl Sandbox for DaytonaSandbox { ) } - async fn ssh_access_command(&self) -> Result, String> { + async fn ssh_access_command(&self) -> crate::Result> { self.create_ssh_access(Some(60.0)).await.map(Some) } @@ -817,12 +847,11 @@ impl Sandbox for DaytonaSandbox { async fn get_preview_url( &self, port: u16, - ) -> Result)>, String> { + ) -> crate::Result)>> { let sandbox = self.sandbox()?; - let preview = sandbox - .get_preview_link(port) - .await - .map_err(|e| format!("Failed to get preview link for port {port}: {e}"))?; + let preview = sandbox.get_preview_link(port).await.map_err(|e| { + crate::Error::context(format!("Failed to get preview link for port {port}"), e) + })?; let mut headers = HashMap::new(); if !preview.token.is_empty() { headers.insert("x-daytona-preview-token".to_string(), preview.token); @@ -834,7 +863,7 @@ impl Sandbox for DaytonaSandbox { Ok(Some((preview.url, headers))) } - async fn refresh_push_credentials(&self) -> Result<(), String> { + async fn refresh_push_credentials(&self) -> crate::Result<()> { if !self.repo_cloned() { return Ok(()); } @@ -850,7 +879,7 @@ impl Sandbox for DaytonaSandbox { origin_url, ) .await - .map_err(|e| format!("Failed to refresh GitHub App token: {e}"))?; + .map_err(|e| crate::Error::message(format!("Failed to refresh GitHub App token: {e}")))?; let cmd = format!( "git -c maintenance.auto=0 remote set-url origin {}", @@ -859,31 +888,30 @@ impl Sandbox for DaytonaSandbox { let result = self .exec_command(&cmd, 10_000, None, None, None) .await - .map_err(|e| format!("Failed to set refreshed push credentials: {e}"))?; + .map_err(|e| crate::Error::context("Failed to set refreshed push credentials", e))?; if result.exit_code != 0 { let stderr = result .stderr .replace(&auth_url.raw_string(), &auth_url.redacted_string()); - return Err(format!( + return Err(crate::Error::message(format!( "Failed to set refreshed push credentials (exit {}): {}", result.exit_code, stderr - )); + ))); } Ok(()) } - async fn set_autostop_interval(&self, minutes: i32) -> Result<(), String> { + async fn set_autostop_interval(&self, minutes: i32) -> crate::Result<()> { let sandbox_id = self.sandbox()?.id.clone(); - let mut sandbox = self - .client - .get(&sandbox_id) - .await - .map_err(|e| format!("Failed to get sandbox for autostop update: {e}"))?; + let mut sandbox = + self.client.get(&sandbox_id).await.map_err(|e| { + crate::Error::context("Failed to get sandbox for autostop update", e) + })?; sandbox .set_autostop_interval(minutes) .await - .map_err(|e| format!("Failed to set autostop interval: {e}")) + .map_err(|e| crate::Error::context("Failed to set autostop interval", e)) } async fn read_file( @@ -891,27 +919,27 @@ impl Sandbox for DaytonaSandbox { path: &str, offset: Option, limit: Option, - ) -> Result { + ) -> crate::Result { let sandbox = self.sandbox()?; let resolved = self.resolve_path(path); let fs_svc = sandbox .fs() .await - .map_err(|e| format!("Failed to get fs service: {e}"))?; + .map_err(|e| crate::Error::context("Failed to get fs service", e))?; let bytes = fs_svc .download_file(&resolved) .await - .map_err(|e| format!("Failed to read file {resolved}: {e}"))?; + .map_err(|e| crate::Error::context(format!("Failed to read file {resolved}"), e))?; - let content = - String::from_utf8(bytes).map_err(|e| format!("File is not valid UTF-8: {e}"))?; + let content = String::from_utf8(bytes) + .map_err(|e| crate::Error::context("File is not valid UTF-8", e))?; Ok(format_lines_numbered(&content, offset, limit)) } - async fn write_file(&self, path: &str, content: &str) -> Result<(), String> { + async fn write_file(&self, path: &str, content: &str) -> crate::Result<()> { let sandbox = self.sandbox()?; let resolved = self.resolve_path(path); @@ -922,7 +950,7 @@ impl Sandbox for DaytonaSandbox { let fs_svc = sandbox .fs() .await - .map_err(|e| format!("Failed to get fs service: {e}"))?; + .map_err(|e| crate::Error::context("Failed to get fs service", e))?; let _ = fs_svc.create_folder(&parent_str, None).await; } } @@ -930,46 +958,49 @@ impl Sandbox for DaytonaSandbox { let fs_svc = sandbox .fs() .await - .map_err(|e| format!("Failed to get fs service: {e}"))?; + .map_err(|e| crate::Error::context("Failed to get fs service", e))?; fs_svc .upload_file_bytes(&resolved, content.as_bytes()) .await - .map_err(|e| format!("Failed to write file {resolved}: {e}"))?; + .map_err(|e| crate::Error::context(format!("Failed to write file {resolved}"), e))?; Ok(()) } - async fn delete_file(&self, path: &str) -> Result<(), String> { + async fn delete_file(&self, path: &str) -> crate::Result<()> { let sandbox = self.sandbox()?; let resolved = self.resolve_path(path); let fs_svc = sandbox .fs() .await - .map_err(|e| format!("Failed to get fs service: {e}"))?; + .map_err(|e| crate::Error::context("Failed to get fs service", e))?; fs_svc .delete_file(&resolved, false) .await - .map_err(|e| format!("Failed to delete file {resolved}: {e}"))?; + .map_err(|e| crate::Error::context(format!("Failed to delete file {resolved}"), e))?; Ok(()) } - async fn file_exists(&self, path: &str) -> Result { + async fn file_exists(&self, path: &str) -> crate::Result { let sandbox = self.sandbox()?; let resolved = self.resolve_path(path); let fs_svc = sandbox .fs() .await - .map_err(|e| format!("Failed to get fs service: {e}"))?; + .map_err(|e| crate::Error::context("Failed to get fs service", e))?; match fs_svc.get_file_info(&resolved).await { Ok(_) => Ok(true), Err(daytona_sdk::DaytonaError::NotFound { .. }) => Ok(false), - Err(e) => Err(format!("Failed to check file existence {resolved}: {e}")), + Err(e) => Err(crate::Error::context( + format!("Failed to check file existence {resolved}"), + e, + )), } } @@ -977,19 +1008,18 @@ impl Sandbox for DaytonaSandbox { &self, path: &str, _depth: Option, - ) -> Result, String> { + ) -> crate::Result> { let sandbox = self.sandbox()?; let resolved = self.resolve_path(path); let fs_svc = sandbox .fs() .await - .map_err(|e| format!("Failed to get fs service: {e}"))?; + .map_err(|e| crate::Error::context("Failed to get fs service", e))?; - let files = fs_svc - .list_files(&resolved) - .await - .map_err(|e| format!("Failed to list directory {resolved}: {e}"))?; + let files = fs_svc.list_files(&resolved).await.map_err(|e| { + crate::Error::context(format!("Failed to list directory {resolved}"), e) + })?; Ok(files .into_iter() @@ -1012,7 +1042,7 @@ impl Sandbox for DaytonaSandbox { working_dir: Option<&str>, env_vars: Option<&HashMap>, cancel_token: Option, - ) -> Result { + ) -> crate::Result { tracing::info!(command, timeout_ms, "exec_command: entered"); let sandbox = self.sandbox()?; @@ -1024,7 +1054,7 @@ impl Sandbox for DaytonaSandbox { let process_svc = sandbox .process() .await - .map_err(|e| format!("Failed to get process service: {e}"))?; + .map_err(|e| crate::Error::context("Failed to get process service", e))?; tracing::info!( elapsed_ms = elapsed_ms(&start), @@ -1070,7 +1100,7 @@ impl Sandbox for DaytonaSandbox { ok = res.is_ok(), "exec_command: HTTP response received" ); - res.map_err(|e| format!("Failed to execute command: {e}"))? + res.map_err(|e| crate::Error::context("Failed to execute command", e))? } () = time::sleep(timeout_duration) => { tracing::info!( @@ -1119,7 +1149,7 @@ impl Sandbox for DaytonaSandbox { pattern: &str, path: &str, options: &GrepOptions, - ) -> Result, String> { + ) -> crate::Result> { let resolved = self.resolve_path(path); // Detect ripgrep availability (cached) @@ -1178,16 +1208,16 @@ impl Sandbox for DaytonaSandbox { return Ok(Vec::new()); } if result.exit_code != 0 { - return Err(format!( + return Err(crate::Error::message(format!( "grep failed (exit {}): {}", result.exit_code, result.stderr - )); + ))); } Ok(result.stdout.lines().map(String::from).collect()) } - async fn glob(&self, pattern: &str, path: Option<&str>) -> Result, String> { + async fn glob(&self, pattern: &str, path: Option<&str>) -> crate::Result> { let base = path.map_or_else(|| WORKING_DIRECTORY.to_string(), |p| self.resolve_path(p)); let cmd = format!( @@ -1199,10 +1229,10 @@ impl Sandbox for DaytonaSandbox { let result = self.exec_command(&cmd, 30_000, None, None, None).await?; if result.exit_code != 0 { - return Err(format!( + return Err(crate::Error::message(format!( "glob failed (exit {}): {}", result.exit_code, result.stderr - )); + ))); } Ok(result diff --git a/lib/crates/fabro-sandbox/src/docker.rs b/lib/crates/fabro-sandbox/src/docker.rs index 200f16ae9..64258396a 100644 --- a/lib/crates/fabro-sandbox/src/docker.rs +++ b/lib/crates/fabro-sandbox/src/docker.rs @@ -88,9 +88,8 @@ impl DockerSandbox { run_id: Option, clone_origin_url: Option, clone_branch: Option, - ) -> Result { - let docker = Docker::connect_with_local_defaults() - .map_err(|e| format!("Failed to connect to Docker daemon: {e}"))?; + ) -> crate::Result { + let docker = Docker::connect_with_local_defaults().map_err(crate::Error::docker_connect)?; Ok(Self { docker, config, @@ -113,7 +112,7 @@ impl DockerSandbox { repo_cloned: bool, clone_origin_url: Option, clone_branch: Option, - ) -> Result { + ) -> crate::Result { let sandbox = Self::new( DockerSandboxOptions::default(), None, @@ -149,11 +148,10 @@ impl DockerSandbox { } } - fn container_id(&self) -> Result<&str, String> { - self.container_id - .get() - .map(String::as_str) - .ok_or_else(|| "Container not initialized — call initialize() first".to_string()) + fn container_id(&self) -> crate::Result<&str> { + self.container_id.get().map(String::as_str).ok_or_else(|| { + crate::Error::message("Container not initialized — call initialize() first") + }) } fn resolve_container_path(path: &str) -> String { @@ -169,7 +167,7 @@ impl DockerSandbox { cmd: Vec, working_dir: Option<&str>, env: Option>, - ) -> Result<(String, String, i32), String> { + ) -> crate::Result<(String, String, i32)> { let container_id = self.container_id()?; let exec_opts = CreateExecOptions { @@ -185,13 +183,13 @@ impl DockerSandbox { .docker .create_exec(container_id, exec_opts) .await - .map_err(|e| format!("Failed to create exec: {e}"))?; + .map_err(|e| crate::Error::message(format!("Failed to create exec: {e}")))?; let start_result = self .docker .start_exec(&exec_instance.id, None) .await - .map_err(|e| format!("Failed to start exec: {e}"))?; + .map_err(|e| crate::Error::message(format!("Failed to start exec: {e}")))?; let mut stdout = String::new(); let mut stderr = String::new(); @@ -206,7 +204,11 @@ impl DockerSandbox { stderr.push_str(&String::from_utf8_lossy(&message)); } Ok(_) => {} - Err(e) => return Err(format!("Error reading exec output: {e}")), + Err(e) => { + return Err(crate::Error::message(format!( + "Error reading exec output: {e}" + ))); + } } } } @@ -215,7 +217,7 @@ impl DockerSandbox { .docker .inspect_exec(&exec_instance.id) .await - .map_err(|e| format!("Failed to inspect exec: {e}"))?; + .map_err(|e| crate::Error::message(format!("Failed to inspect exec: {e}")))?; let exit_code = inspect .exit_code @@ -231,7 +233,7 @@ impl DockerSandbox { working_dir: Option<&str>, env_vars: Option<&HashMap>, cancel_token: Option, - ) -> Result { + ) -> crate::Result { let start = Instant::now(); let effective_dir = working_dir.unwrap_or(WORKING_DIRECTORY).to_string(); let env: Option> = @@ -280,13 +282,20 @@ impl DockerSandbox { } } - async fn ensure_image(&self) -> Result<(), String> { + async fn ensure_image(&self) -> crate::Result<()> { if !self.config.auto_pull { return Ok(()); } - if self.docker.inspect_image(&self.config.image).await.is_ok() { - return Ok(()); + match self.docker.inspect_image(&self.config.image).await { + Ok(_) => return Ok(()), + Err(e) if docker_not_found(&e) => {} + Err(e) => { + return Err(crate::Error::docker_image_inspect( + self.config.image.clone(), + e, + )); + } } let (repo, tag) = if let Some((r, t)) = self.config.image.rsplit_once(':') { @@ -303,13 +312,13 @@ impl DockerSandbox { let mut stream = self.docker.create_image(Some(opts), None, None); while let Some(result) = stream.next().await { - result.map_err(|e| format!("Failed to pull image {}: {e}", self.config.image))?; + result.map_err(|e| crate::Error::docker_image_pull(self.config.image.clone(), e))?; } Ok(()) } - async fn create_workspace(&self) -> Result<(), String> { + async fn create_workspace(&self) -> crate::Result<()> { let result = self .docker_exec_shell( &format!("mkdir -p {}", shell_quote(WORKING_DIRECTORY)), @@ -320,23 +329,23 @@ impl DockerSandbox { ) .await?; if result.exit_code != 0 { - return Err(format!( + return Err(crate::Error::message(format!( "Failed to create Docker workspace (exit {}): {}", result.exit_code, result.stderr - )); + ))); } Ok(()) } - async fn verify_git_available(&self) -> Result<(), String> { + async fn verify_git_available(&self) -> crate::Result<()> { let result = self .docker_exec_shell("git --version", 10_000, Some("/"), None, None) .await?; if result.exit_code != 0 { - return Err(format!( + return Err(crate::Error::message(format!( "Docker image '{}' must include git for repository clone and git lifecycle operations. Use an image with bash and git, such as buildpack-deps:noble.", self.config.image - )); + ))); } Ok(()) } @@ -345,7 +354,7 @@ impl DockerSandbox { &self, origin_url: String, branch: Option, - ) -> Result<(), String> { + ) -> crate::Result<()> { self.verify_git_available().await?; self.emit(SandboxEvent::GitCloneStarted { @@ -361,7 +370,11 @@ impl DockerSandbox { &origin_url, ) .await - .map_err(|e| format!("Failed to get GitHub App credentials for clone: {e}"))?, + .map_err(|e| { + crate::Error::message(format!( + "Failed to get GitHub App credentials for clone: {e}" + )) + })?, ), None => None, }; @@ -385,16 +398,17 @@ impl DockerSandbox { .await?; if result.exit_code != 0 { let stderr = redact_auth_url(&result.stderr, auth_url.as_ref()); - let err = if self.github_app.is_none() { + let err = crate::Error::message(if self.github_app.is_none() { format!( "Git clone failed: {stderr}. If this is a private repository, configure a GitHub App with `fabro install` and install it for your organization." ) } else { format!("Failed to clone repo into Docker sandbox: {stderr}") - }; + }); self.emit(SandboxEvent::GitCloneFailed { - url: origin_url, - error: err.clone(), + url: origin_url, + error: err.to_string(), + causes: err.causes(), }); return Err(err); } @@ -426,21 +440,23 @@ impl DockerSandbox { Ok(()) } - async fn validate_managed_container(&self, container_id: &str) -> Result<(), String> { + async fn validate_managed_container(&self, container_id: &str) -> crate::Result<()> { let labels = self.inspect_labels(container_id).await?; verify_managed_labels(container_id, &labels, self.run_id.as_ref()) } - async fn inspect_labels(&self, container_id: &str) -> Result, String> { + async fn inspect_labels(&self, container_id: &str) -> crate::Result> { let inspect = self .docker .inspect_container(container_id, None::) .await .map_err(|e| { if docker_not_found(&e) { - format!("Docker container '{container_id}' is gone") + crate::Error::message(format!("Docker container '{container_id}' is gone")) } else { - format!("Failed to inspect Docker container '{container_id}': {e}") + crate::Error::message(format!( + "Failed to inspect Docker container '{container_id}': {e}" + )) } })?; Ok(inspect @@ -449,7 +465,7 @@ impl DockerSandbox { .unwrap_or_default()) } - async fn ensure_name_available(&self) -> Result, String> { + async fn ensure_name_available(&self) -> crate::Result> { let Some(run_id) = self.run_id.as_ref() else { return Ok(None); }; @@ -459,17 +475,17 @@ impl DockerSandbox { .inspect_container(&name, None::) .await { - Ok(_) => Err(format!( + Ok(_) => Err(crate::Error::message(format!( "Docker container name '{name}' already exists for run {run_id}. Remove the stale container manually before retrying." - )), + ))), Err(e) if docker_not_found(&e) => Ok(Some(name)), - Err(e) => Err(format!( + Err(e) => Err(crate::Error::message(format!( "Failed to check Docker container name '{name}' before creation: {e}" - )), + ))), } } - async fn upload_bytes_to_container(&self, path: &str, bytes: &[u8]) -> Result<(), String> { + async fn upload_bytes_to_container(&self, path: &str, bytes: &[u8]) -> crate::Result<()> { let container_path = Self::resolve_container_path(path); let container_id = self.container_id()?; let parent_dir = std::path::Path::new(&container_path) @@ -477,7 +493,7 @@ impl DockerSandbox { .map_or_else(|| "/".to_string(), |p| p.to_string_lossy().to_string()); let file_name = std::path::Path::new(&container_path) .file_name() - .ok_or_else(|| format!("Invalid path: {container_path}"))? + .ok_or_else(|| crate::Error::message(format!("Invalid path: {container_path}")))? .to_string_lossy() .to_string(); @@ -491,10 +507,10 @@ impl DockerSandbox { ) .await?; if result.exit_code != 0 { - return Err(format!( + return Err(crate::Error::message(format!( "Failed to create parent dirs for {container_path}: {}", result.stderr - )); + ))); } let tar_bytes = build_single_file_tar(&file_name, bytes)?; @@ -506,13 +522,14 @@ impl DockerSandbox { self.docker .upload_to_container(container_id, Some(upload_opts), tar_bytes.into()) .await - .map_err(|e| format!("Failed to upload file to container: {e}")) + .map_err(|e| crate::Error::context("Failed to upload file to container", e)) } - fn cleanup_error(&self, error: String) -> Result<(), String> { + fn cleanup_error(&self, error: crate::Error) -> crate::Result<()> { self.emit(SandboxEvent::CleanupFailed { provider: "docker".into(), - error: error.clone(), + error: error.to_string(), + causes: error.causes(), }); Err(error) } @@ -567,19 +584,19 @@ fn verify_managed_labels( container_id: &str, labels: &HashMap, run_id: Option<&RunId>, -) -> Result<(), String> { +) -> crate::Result<()> { if labels.get(MANAGED_LABEL).map(String::as_str) != Some("true") { - return Err(format!( + return Err(crate::Error::message(format!( "Refusing to operate on Docker container '{container_id}' because it is missing label {MANAGED_LABEL}=true" - )); + ))); } if let Some(run_id) = run_id { let actual = labels.get(RUN_ID_LABEL).map(String::as_str); let expected = run_id.to_string(); if actual != Some(expected.as_str()) { - return Err(format!( + return Err(crate::Error::message(format!( "Refusing to operate on Docker container '{container_id}' because label {RUN_ID_LABEL}={actual:?} does not match run {run_id}" - )); + ))); } } Ok(()) @@ -612,23 +629,24 @@ fn redact_auth_url(text: &str, auth_url: Option<&fabro_redact::DisplaySafeUrl>) text.replace(&auth_url.raw_string(), &auth_url.redacted_string()) } -fn build_single_file_tar(file_name: &str, bytes: &[u8]) -> Result, String> { +fn build_single_file_tar(file_name: &str, bytes: &[u8]) -> crate::Result> { let mut tar_builder = tar::Builder::new(Vec::new()); let mut header = tar::Header::new_gnu(); header .set_path(file_name) - .map_err(|e| format!("Failed to set tar path: {e}"))?; + .map_err(|e| crate::Error::message(format!("Failed to set tar path: {e}")))?; header.set_size( - u64::try_from(bytes.len()).map_err(|_| "file is too large for tar header".to_string())?, + u64::try_from(bytes.len()) + .map_err(|_| crate::Error::message("file is too large for tar header"))?, ); header.set_mode(0o644); header.set_cksum(); tar_builder .append(&header, bytes) - .map_err(|e| format!("Failed to build tar archive: {e}"))?; + .map_err(|e| crate::Error::message(format!("Failed to build tar archive: {e}")))?; tar_builder .into_inner() - .map_err(|e| format!("Failed to finalize tar archive: {e}")) + .map_err(|e| crate::Error::message(format!("Failed to finalize tar archive: {e}"))) } #[async_trait] @@ -637,7 +655,7 @@ impl Sandbox for DockerSandbox { &self, remote_path: &str, local_path: &std::path::Path, - ) -> Result<(), String> { + ) -> crate::Result<()> { let container_id = self.container_id()?; let container_path = Self::resolve_container_path(remote_path); let opts = DownloadFromContainerOptions { @@ -648,8 +666,12 @@ impl Sandbox for DockerSandbox { .download_from_container(container_id, Some(opts)); let mut archive_bytes = Vec::new(); while let Some(chunk) = stream.next().await { - let chunk = chunk - .map_err(|e| format!("Failed to download {container_path} from container: {e}"))?; + let chunk = chunk.map_err(|e| { + crate::Error::context( + format!("Failed to download {container_path} from container"), + e, + ) + })?; archive_bytes.extend_from_slice(&chunk); } @@ -661,51 +683,62 @@ impl Sandbox for DockerSandbox { use std::io::Read as _; let mut archive = tar::Archive::new(Cursor::new(archive_bytes)); - let entries = archive - .entries() - .map_err(|e| format!("Failed to read Docker archive for {container_path}: {e}"))?; + let entries = archive.entries().map_err(|e| { + crate::Error::context( + format!("Failed to read Docker archive for {container_path}"), + e, + ) + })?; let mut file_bytes = None; for entry in entries { let mut entry = entry.map_err(|e| { - format!("Failed to read Docker archive entry for {container_path}: {e}") + crate::Error::context( + format!("Failed to read Docker archive entry for {container_path}"), + e, + ) })?; if !entry.header().entry_type().is_file() { continue; } let mut bytes = Vec::new(); entry.read_to_end(&mut bytes).map_err(|e| { - format!("Failed to read Docker archive file for {container_path}: {e}") + crate::Error::context( + format!("Failed to read Docker archive file for {container_path}"), + e, + ) })?; file_bytes = Some(bytes); break; } file_bytes.ok_or_else(|| { - format!("Docker archive for {container_path} did not contain a file") + crate::Error::message(format!( + "Docker archive for {container_path} did not contain a file" + )) })? }; if let Some(parent) = local_path.parent() { fs::create_dir_all(parent) .await - .map_err(|e| format!("Failed to create parent dirs: {e}"))?; + .map_err(|e| crate::Error::context("Failed to create parent dirs", e))?; } - fs::write(local_path, bytes) - .await - .map_err(|e| format!("Failed to write {}: {e}", local_path.display())) + fs::write(local_path, bytes).await.map_err(|e| { + crate::Error::context(format!("Failed to write {}", local_path.display()), e) + }) } async fn upload_file_from_local( &self, local_path: &std::path::Path, remote_path: &str, - ) -> Result<(), String> { - let bytes = fs::read(local_path) - .await - .map_err(|e| format!("Failed to read {}: {e}", local_path.display()))?; + ) -> crate::Result<()> { + let bytes = fs::read(local_path).await.map_err(|e| { + crate::Error::context(format!("Failed to read {}", local_path.display()), e) + })?; self.upload_bytes_to_container(remote_path, &bytes).await } - async fn initialize(&self) -> Result<(), String> { + async fn initialize(&self) -> crate::Result<()> { self.emit(SandboxEvent::Initializing { provider: "docker".into(), }); @@ -719,7 +752,8 @@ impl Sandbox for DockerSandbox { let duration_ms = u64::try_from(init_start.elapsed().as_millis()).unwrap_or(u64::MAX); self.emit(SandboxEvent::InitializeFailed { provider: "docker".into(), - error: e.clone(), + error: e.to_string(), + causes: e.causes(), duration_ms, }); return Err(e); @@ -737,7 +771,8 @@ impl Sandbox for DockerSandbox { u64::try_from(init_start.elapsed().as_millis()).unwrap_or(u64::MAX); self.emit(SandboxEvent::InitializeFailed { provider: "docker".into(), - error: e.clone(), + error: e.to_string(), + causes: e.causes(), duration_ms, }); return Err(e); @@ -752,24 +787,24 @@ impl Sandbox for DockerSandbox { .create_container(create_options, container_config(&self.config, self.run_id.as_ref())) .await .map_err(|e| { - let err = if matches!( + let message = if matches!( e, DockerError::DockerResponseServerError { status_code: 409, .. } ) { - format!( - "Docker container for run already exists. Remove the stale fabro-run container manually before retrying: {e}" - ) + "Docker container for run already exists. Remove the stale fabro-run container manually before retrying.".to_string() } else { - format!("Failed to create container: {e}") + "Failed to create Docker container".to_string() }; + let err = crate::Error::context(message, e); let duration_ms = u64::try_from(init_start.elapsed().as_millis()).unwrap_or(u64::MAX); self.emit(SandboxEvent::InitializeFailed { provider: "docker".into(), - error: err.clone(), + error: err.to_string(), + causes: err.causes(), duration_ms, }); err @@ -778,18 +813,19 @@ impl Sandbox for DockerSandbox { let id = container.id.clone(); self.container_id .set(id.clone()) - .map_err(|_| "Container already initialized".to_string())?; + .map_err(|_| crate::Error::message("Container already initialized"))?; self.docker .start_container(&id, None::>) .await .map_err(|e| { - let err = bash_remediation(&e, &self.config.image); + let err = crate::Error::context(bash_remediation(&e, &self.config.image), e); let duration_ms = u64::try_from(init_start.elapsed().as_millis()).unwrap_or(u64::MAX); self.emit(SandboxEvent::InitializeFailed { provider: "docker".into(), - error: err.clone(), + error: err.to_string(), + causes: err.causes(), duration_ms, }); err @@ -807,13 +843,14 @@ impl Sandbox for DockerSandbox { ) .await?; if exit_code != 0 || !stdout.contains("ready") { - let err = format!( + let err = crate::Error::message(format!( "Docker container health check failed. Docker sandboxes require /bin/bash; use an image with bash and git, such as buildpack-deps:noble. {stderr}" - ); + )); let duration_ms = u64::try_from(init_start.elapsed().as_millis()).unwrap_or(u64::MAX); self.emit(SandboxEvent::InitializeFailed { provider: "docker".into(), - error: err.clone(), + error: err.to_string(), + causes: err.causes(), duration_ms, }); return Err(err); @@ -838,7 +875,8 @@ impl Sandbox for DockerSandbox { u64::try_from(init_start.elapsed().as_millis()).unwrap_or(u64::MAX); self.emit(SandboxEvent::InitializeFailed { provider: "docker".into(), - error: e.clone(), + error: e.to_string(), + causes: e.causes(), duration_ms, }); return Err(e); @@ -859,7 +897,8 @@ impl Sandbox for DockerSandbox { u64::try_from(init_start.elapsed().as_millis()).unwrap_or(u64::MAX); self.emit(SandboxEvent::InitializeFailed { provider: "docker".into(), - error: e.clone(), + error: e.to_string(), + causes: e.causes(), duration_ms, }); return Err(e); @@ -872,7 +911,8 @@ impl Sandbox for DockerSandbox { u64::try_from(init_start.elapsed().as_millis()).unwrap_or(u64::MAX); self.emit(SandboxEvent::InitializeFailed { provider: "docker".into(), - error: e.clone(), + error: e.to_string(), + causes: e.causes(), duration_ms, }); return Err(e); @@ -893,7 +933,7 @@ impl Sandbox for DockerSandbox { Ok(()) } - async fn cleanup(&self) -> Result<(), String> { + async fn cleanup(&self) -> crate::Result<()> { self.emit(SandboxEvent::CleanupStarted { provider: "docker".into(), }); @@ -923,8 +963,11 @@ impl Sandbox for DockerSandbox { .await { if !docker_not_found(&e) && !docker_already_stopped(&e) { - return self.cleanup_error(format!( - "Failed to stop Docker container '{container_id}' with labels {labels:?}: {e}" + return self.cleanup_error(crate::Error::context( + format!( + "Failed to stop Docker container '{container_id}' with labels {labels:?}" + ), + e, )); } } @@ -939,8 +982,11 @@ impl Sandbox for DockerSandbox { .await { if !docker_not_found(&e) { - return self.cleanup_error(format!( - "Failed to remove Docker container '{container_id}' with labels {labels:?}: {e}" + return self.cleanup_error(crate::Error::context( + format!( + "Failed to remove Docker container '{container_id}' with labels {labels:?}" + ), + e, )); } } @@ -961,7 +1007,7 @@ impl Sandbox for DockerSandbox { working_dir: Option<&str>, env_vars: Option<&HashMap>, cancel_token: Option, - ) -> Result { + ) -> crate::Result { let dir = working_dir.map(Self::resolve_container_path); self.docker_exec_shell(command, timeout_ms, dir.as_deref(), env_vars, cancel_token) .await @@ -972,25 +1018,27 @@ impl Sandbox for DockerSandbox { path: &str, offset: Option, limit: Option, - ) -> Result { + ) -> crate::Result { let container_path = Self::resolve_container_path(path); let (stdout, stderr, exit_code) = self .docker_exec(vec!["cat".to_string(), container_path.clone()], None, None) .await?; if exit_code != 0 { - return Err(format!("Failed to read {container_path}: {stderr}")); + return Err(crate::Error::message(format!( + "Failed to read {container_path}: {stderr}" + ))); } Ok(format_lines_numbered(&stdout, offset, limit)) } - async fn write_file(&self, path: &str, content: &str) -> Result<(), String> { + async fn write_file(&self, path: &str, content: &str) -> crate::Result<()> { self.upload_bytes_to_container(path, content.as_bytes()) .await } - async fn delete_file(&self, path: &str) -> Result<(), String> { + async fn delete_file(&self, path: &str) -> crate::Result<()> { let container_path = Self::resolve_container_path(path); let (_, stderr, exit_code) = self .docker_exec( @@ -1001,12 +1049,14 @@ impl Sandbox for DockerSandbox { .await?; if exit_code != 0 { - return Err(format!("Failed to delete {container_path}: {stderr}")); + return Err(crate::Error::message(format!( + "Failed to delete {container_path}: {stderr}" + ))); } Ok(()) } - async fn file_exists(&self, path: &str) -> Result { + async fn file_exists(&self, path: &str) -> crate::Result { let container_path = Self::resolve_container_path(path); let (_, _, exit_code) = self .docker_exec( @@ -1023,7 +1073,7 @@ impl Sandbox for DockerSandbox { &self, path: &str, depth: Option, - ) -> Result, String> { + ) -> crate::Result> { let container_path = Self::resolve_container_path(path); let max_depth = depth.unwrap_or(1); let (stdout, stderr, exit_code) = self @@ -1044,9 +1094,9 @@ impl Sandbox for DockerSandbox { .await?; if exit_code != 0 { - return Err(format!( + return Err(crate::Error::message(format!( "Failed to list directory {container_path}: {stderr}" - )); + ))); } let mut entries: Vec = stdout @@ -1078,7 +1128,7 @@ impl Sandbox for DockerSandbox { pattern: &str, path: &str, options: &GrepOptions, - ) -> Result, String> { + ) -> crate::Result> { let container_path = Self::resolve_container_path(path); let use_rg = *self .rg_available @@ -1133,10 +1183,10 @@ impl Sandbox for DockerSandbox { return Ok(Vec::new()); } if result.exit_code != 0 { - return Err(format!( + return Err(crate::Error::message(format!( "grep failed (exit {}): {}", result.exit_code, result.stderr - )); + ))); } Ok(result @@ -1147,7 +1197,7 @@ impl Sandbox for DockerSandbox { .collect()) } - async fn glob(&self, pattern: &str, path: Option<&str>) -> Result, String> { + async fn glob(&self, pattern: &str, path: Option<&str>) -> crate::Result> { let base_dir = path.map_or_else( || WORKING_DIRECTORY.to_string(), Self::resolve_container_path, @@ -1161,10 +1211,10 @@ impl Sandbox for DockerSandbox { .docker_exec_shell(&command, 30_000, None, None, None) .await?; if result.exit_code != 0 { - return Err(format!( + return Err(crate::Error::message(format!( "glob failed (exit {}): {}", result.exit_code, result.stderr - )); + ))); } Ok(result @@ -1194,7 +1244,7 @@ impl Sandbox for DockerSandbox { self.container_id.get().cloned().unwrap_or_default() } - async fn setup_git_for_run(&self, run_id: &str) -> Result, String> { + async fn setup_git_for_run(&self, run_id: &str) -> crate::Result> { if !self.repo_cloned() { return Ok(None); } @@ -1242,7 +1292,7 @@ impl Sandbox for DockerSandbox { self.origin_url.get().map(String::as_str) } - async fn refresh_push_credentials(&self) -> Result<(), String> { + async fn refresh_push_credentials(&self) -> crate::Result<()> { if !self.repo_cloned() { return Ok(()); } @@ -1258,7 +1308,7 @@ impl Sandbox for DockerSandbox { origin_url, ) .await - .map_err(|e| format!("Failed to refresh GitHub App token: {e}"))?; + .map_err(|e| crate::Error::message(format!("Failed to refresh GitHub App token: {e}")))?; let command = format!( "git -c maintenance.auto=0 remote set-url origin {}", @@ -1269,10 +1319,10 @@ impl Sandbox for DockerSandbox { .await?; if result.exit_code != 0 { let stderr = redact_auth_url(&result.stderr, Some(&auth_url)); - return Err(format!( + return Err(crate::Error::message(format!( "Failed to set refreshed push credentials (exit {}): {}", result.exit_code, stderr - )); + ))); } Ok(()) diff --git a/lib/crates/fabro-sandbox/src/error.rs b/lib/crates/fabro-sandbox/src/error.rs new file mode 100644 index 000000000..afcae8ef4 --- /dev/null +++ b/lib/crates/fabro-sandbox/src/error.rs @@ -0,0 +1,94 @@ +#[derive(Debug, thiserror::Error)] +pub enum Error { + #[error("{0}")] + Message(String), + + #[error("{message}")] + Context { + message: String, + #[source] + source: Box, + }, + + #[cfg(feature = "docker")] + #[error("Failed to connect to Docker daemon")] + DockerConnect { + #[source] + source: bollard::errors::Error, + }, + + #[cfg(feature = "docker")] + #[error("Failed to inspect Docker image {image}")] + DockerImageInspect { + image: String, + #[source] + source: bollard::errors::Error, + }, + + #[cfg(feature = "docker")] + #[error("Failed to pull Docker image {image}")] + DockerImagePull { + image: String, + #[source] + source: bollard::errors::Error, + }, +} + +impl Error { + pub fn message(message: impl Into) -> Self { + Self::Message(message.into()) + } + + pub fn context( + message: impl Into, + source: impl std::error::Error + Send + Sync + 'static, + ) -> Self { + Self::Context { + message: message.into(), + source: Box::new(source), + } + } + + #[cfg(feature = "docker")] + pub fn docker_connect(source: bollard::errors::Error) -> Self { + Self::DockerConnect { source } + } + + #[cfg(feature = "docker")] + pub fn docker_image_inspect(image: impl Into, source: bollard::errors::Error) -> Self { + Self::DockerImageInspect { + image: image.into(), + source, + } + } + + #[cfg(feature = "docker")] + pub fn docker_image_pull(image: impl Into, source: bollard::errors::Error) -> Self { + Self::DockerImagePull { + image: image.into(), + source, + } + } + + pub fn causes(&self) -> Vec { + fabro_util::error::collect_causes(self) + } + + pub fn display_with_causes(&self) -> String { + fabro_util::error::render_with_causes(&self.to_string(), &self.causes()) + } +} + +impl From for Error { + fn from(value: String) -> Self { + Self::Message(value) + } +} + +impl From<&str> for Error { + fn from(value: &str) -> Self { + Self::Message(value.to_string()) + } +} + +pub type Result = std::result::Result; diff --git a/lib/crates/fabro-sandbox/src/lib.rs b/lib/crates/fabro-sandbox/src/lib.rs index d6b9528da..83945b081 100644 --- a/lib/crates/fabro-sandbox/src/lib.rs +++ b/lib/crates/fabro-sandbox/src/lib.rs @@ -1,4 +1,5 @@ pub mod config; +pub mod error; pub mod sandbox; pub mod sandbox_spec; @@ -28,6 +29,7 @@ pub mod test_support; #[cfg(feature = "docker")] pub use docker::{DockerSandbox, DockerSandboxOptions}; +pub use error::{Error, Result}; pub use local::LocalSandbox; pub use read_guard::ReadBeforeWriteSandbox; pub use sandbox::{ diff --git a/lib/crates/fabro-sandbox/src/local.rs b/lib/crates/fabro-sandbox/src/local.rs index c32162a48..2f3fd8719 100644 --- a/lib/crates/fabro-sandbox/src/local.rs +++ b/lib/crates/fabro-sandbox/src/local.rs @@ -132,35 +132,35 @@ impl Sandbox for LocalSandbox { path: &str, offset: Option, limit: Option, - ) -> Result { + ) -> crate::Result { let full_path = self.resolve_path(path); - let content = fs::read_to_string(&full_path) - .await - .map_err(|e| format!("Failed to read {}: {e}", full_path.display()))?; + let content = fs::read_to_string(&full_path).await.map_err(|e| { + crate::Error::context(format!("Failed to read {}", full_path.display()), e) + })?; Ok(format_lines_numbered(&content, offset, limit)) } - async fn write_file(&self, path: &str, content: &str) -> Result<(), String> { + async fn write_file(&self, path: &str, content: &str) -> crate::Result<()> { let full_path = self.resolve_path(path); if let Some(parent) = full_path.parent() { fs::create_dir_all(parent) .await - .map_err(|e| format!("Failed to create parent dirs: {e}"))?; + .map_err(|e| crate::Error::context("Failed to create parent dirs", e))?; } - fs::write(&full_path, content) - .await - .map_err(|e| format!("Failed to write {}: {e}", full_path.display())) + fs::write(&full_path, content).await.map_err(|e| { + crate::Error::context(format!("Failed to write {}", full_path.display()), e) + }) } - async fn delete_file(&self, path: &str) -> Result<(), String> { + async fn delete_file(&self, path: &str) -> crate::Result<()> { let full_path = self.resolve_path(path); - fs::remove_file(&full_path) - .await - .map_err(|e| format!("Failed to delete {}: {e}", full_path.display())) + fs::remove_file(&full_path).await.map_err(|e| { + crate::Error::context(format!("Failed to delete {}", full_path.display()), e) + }) } - async fn file_exists(&self, path: &str) -> Result { + async fn file_exists(&self, path: &str) -> crate::Result { let full_path = self.resolve_path(path); Ok(full_path.exists()) } @@ -169,7 +169,7 @@ impl Sandbox for LocalSandbox { &self, path: &str, depth: Option, - ) -> Result, String> { + ) -> crate::Result> { #[expect( clippy::disallowed_methods, reason = "sync recursive read_dir; caller wraps invocation in tokio::task::spawn_blocking" @@ -180,9 +180,11 @@ impl Sandbox for LocalSandbox { current_depth: usize, max_depth: usize, entries: &mut Vec, - ) -> Result<(), String> { + ) -> crate::Result<()> { let mut dir_entries: Vec = std::fs::read_dir(base) - .map_err(|e| format!("Failed to read directory {}: {e}", base.display()))? + .map_err(|e| { + crate::Error::context(format!("Failed to read directory {}", base.display()), e) + })? .filter_map(std::result::Result::ok) .collect(); dir_entries.sort_by_key(std::fs::DirEntry::file_name); @@ -190,7 +192,7 @@ impl Sandbox for LocalSandbox { for entry in dir_entries { let metadata = entry .metadata() - .map_err(|e| format!("Failed to read metadata: {e}"))?; + .map_err(|e| crate::Error::context("Failed to read metadata", e))?; let name = if prefix.is_empty() { entry.file_name().to_string_lossy().into_owned() } else { @@ -221,7 +223,7 @@ impl Sandbox for LocalSandbox { Ok(entries) }) .await - .map_err(|e| format!("list_directory task failed: {e}"))? + .map_err(|e| crate::Error::context("list_directory task failed", e))? } async fn exec_command( @@ -231,7 +233,7 @@ impl Sandbox for LocalSandbox { working_dir: Option<&str>, env_vars: Option<&std::collections::HashMap>, cancel_token: Option, - ) -> Result { + ) -> crate::Result { let start = Instant::now(); let mut filtered_env: Vec<(String, String)> = process_env_vars() @@ -264,7 +266,7 @@ impl Sandbox for LocalSandbox { let mut child = cmd .spawn() - .map_err(|e| format!("Failed to spawn command: {e}"))?; + .map_err(|e| crate::Error::context("Failed to spawn command", e))?; let timeout_duration = std::time::Duration::from_millis(timeout_ms); let token = cancel_token.unwrap_or_default(); @@ -293,7 +295,8 @@ impl Sandbox for LocalSandbox { let (timed_out, exit_code) = tokio::select! { status_result = child.wait() => { - let status = status_result.map_err(|e| format!("Failed to wait for process: {e}"))?; + let status = status_result + .map_err(|e| crate::Error::context("Failed to wait for process", e))?; (false, status.code().unwrap_or(-1)) } () = time::sleep(timeout_duration) => { @@ -325,7 +328,7 @@ impl Sandbox for LocalSandbox { pattern: &str, path: &str, options: &GrepOptions, - ) -> Result, String> { + ) -> crate::Result> { let full_path = self.resolve_path(path); // Try rg (ripgrep) first, fall back to grep @@ -351,7 +354,7 @@ impl Sandbox for LocalSandbox { .args(&args) .output() .await - .map_err(|e| format!("Failed to run rg: {e}"))? + .map_err(|e| crate::Error::context("Failed to run rg", e))? } else { let mut args = vec!["-rn".to_string()]; if options.case_insensitive { @@ -372,7 +375,7 @@ impl Sandbox for LocalSandbox { .args(&args) .output() .await - .map_err(|e| format!("Failed to run grep: {e}"))? + .map_err(|e| crate::Error::context("Failed to run grep", e))? }; let stdout = String::from_utf8_lossy(&output.stdout); @@ -384,7 +387,7 @@ impl Sandbox for LocalSandbox { Ok(results) } - async fn glob(&self, pattern: &str, path: Option<&str>) -> Result, String> { + async fn glob(&self, pattern: &str, path: Option<&str>) -> crate::Result> { let base_dir = path.map_or_else(|| self.working_directory.clone(), std::path::PathBuf::from); @@ -395,7 +398,7 @@ impl Sandbox for LocalSandbox { }; let mut results: Vec = glob::glob(&full_pattern) - .map_err(|e| format!("Invalid glob pattern: {e}"))? + .map_err(|e| crate::Error::context("Invalid glob pattern", e))? .filter_map(Result::ok) .map(|p| p.to_string_lossy().into_owned()) .collect(); @@ -416,18 +419,21 @@ impl Sandbox for LocalSandbox { &self, remote_path: &str, local_path: &Path, - ) -> Result<(), String> { + ) -> crate::Result<()> { let full_path = self.resolve_path(remote_path); if let Some(parent) = local_path.parent() { fs::create_dir_all(parent) .await - .map_err(|e| format!("Failed to create parent dirs: {e}"))?; + .map_err(|e| crate::Error::context("Failed to create parent dirs", e))?; } fs::copy(&full_path, local_path).await.map_err(|e| { - format!( - "Failed to copy {} to {}: {e}", - full_path.display(), - local_path.display() + crate::Error::context( + format!( + "Failed to copy {} to {}", + full_path.display(), + local_path.display() + ), + e, ) })?; Ok(()) @@ -437,31 +443,34 @@ impl Sandbox for LocalSandbox { &self, local_path: &Path, remote_path: &str, - ) -> Result<(), String> { + ) -> crate::Result<()> { let full_path = self.resolve_path(remote_path); if let Some(parent) = full_path.parent() { fs::create_dir_all(parent) .await - .map_err(|e| format!("Failed to create parent dirs: {e}"))?; + .map_err(|e| crate::Error::context("Failed to create parent dirs", e))?; } fs::copy(local_path, &full_path).await.map_err(|e| { - format!( - "Failed to copy {} to {}: {e}", - local_path.display(), - full_path.display() + crate::Error::context( + format!( + "Failed to copy {} to {}", + local_path.display(), + full_path.display() + ), + e, ) })?; Ok(()) } - async fn initialize(&self) -> Result<(), String> { + async fn initialize(&self) -> crate::Result<()> { self.emit(SandboxEvent::Initializing { provider: "local".into(), }); let start = Instant::now(); let result = fs::create_dir_all(&self.working_directory) .await - .map_err(|e| format!("Failed to create working directory: {e}")); + .map_err(|e| crate::Error::context("Failed to create working directory", e)); let duration_ms = u64::try_from(start.elapsed().as_millis()).unwrap_or(u64::MAX); match &result { Ok(()) => self.emit(SandboxEvent::Ready { @@ -474,14 +483,15 @@ impl Sandbox for LocalSandbox { }), Err(e) => self.emit(SandboxEvent::InitializeFailed { provider: "local".into(), - error: e.clone(), + error: e.to_string(), + causes: e.causes(), duration_ms, }), } result } - async fn cleanup(&self) -> Result<(), String> { + async fn cleanup(&self) -> crate::Result<()> { self.emit(SandboxEvent::CleanupStarted { provider: "local".into(), }); diff --git a/lib/crates/fabro-sandbox/src/read_guard.rs b/lib/crates/fabro-sandbox/src/read_guard.rs index 19b65404a..0ab902f46 100644 --- a/lib/crates/fabro-sandbox/src/read_guard.rs +++ b/lib/crates/fabro-sandbox/src/read_guard.rs @@ -62,7 +62,7 @@ impl ReadBeforeWriteSandbox { .contains(&normalized) } - async fn guard_write(&self, path: &str) -> Result<(), String> { + async fn guard_write(&self, path: &str) -> crate::Result<()> { let normalized = self.normalize_path(path); if normalized.starts_with("/tmp/") { return Ok(()); @@ -70,10 +70,10 @@ impl ReadBeforeWriteSandbox { let exists = self.inner.file_exists(path).await?; if exists && !self.has_read(path) { warn!(path = %path, "Write blocked: file not read by agent"); - Err(format!( + Err(crate::Error::message(format!( "Cannot write to '{path}': file exists but has not been read. \ Use read_file to read the file before writing to it." - )) + ))) } else { Ok(()) } @@ -82,12 +82,12 @@ impl ReadBeforeWriteSandbox { crate::delegate_sandbox! { ReadBeforeWriteSandbox => inner { - async fn write_file(&self, path: &str, content: &str) -> Result<(), String> { + async fn write_file(&self, path: &str, content: &str) -> crate::Result<()> { self.guard_write(path).await?; self.inner.write_file(path, content).await } - async fn delete_file(&self, path: &str) -> Result<(), String> { + async fn delete_file(&self, path: &str) -> crate::Result<()> { self.guard_write(path).await?; self.inner.delete_file(path).await } diff --git a/lib/crates/fabro-sandbox/src/sandbox.rs b/lib/crates/fabro-sandbox/src/sandbox.rs index 3631403c5..2c23a05b1 100644 --- a/lib/crates/fabro-sandbox/src/sandbox.rs +++ b/lib/crates/fabro-sandbox/src/sandbox.rs @@ -27,7 +27,7 @@ pub struct GitRunInfo { /// delegate_sandbox! { /// MyDecorator => inner { /// // Only provide methods with custom logic — the rest delegate automatically. -/// async fn read_file(&self, path: &str, offset: Option, limit: Option) -> Result { +/// async fn read_file(&self, path: &str, offset: Option, limit: Option) -> $crate::Result { /// // custom logic... /// } /// } @@ -44,7 +44,7 @@ macro_rules! delegate_sandbox { impl $crate::Sandbox for $type { $($custom)* - async fn file_exists(&self, path: &str) -> Result { + async fn file_exists(&self, path: &str) -> $crate::Result { self.$field.file_exists(path).await } @@ -52,7 +52,7 @@ macro_rules! delegate_sandbox { &self, path: &str, depth: Option, - ) -> Result, String> { + ) -> $crate::Result> { self.$field.list_directory(path, depth).await } @@ -63,13 +63,13 @@ macro_rules! delegate_sandbox { working_dir: Option<&str>, env_vars: Option<&std::collections::HashMap>, cancel_token: Option, - ) -> Result<$crate::ExecResult, String> { + ) -> $crate::Result<$crate::ExecResult> { self.$field .exec_command(command, timeout_ms, working_dir, env_vars, cancel_token) .await } - async fn glob(&self, pattern: &str, path: Option<&str>) -> Result, String> { + async fn glob(&self, pattern: &str, path: Option<&str>) -> $crate::Result> { self.$field.glob(pattern, path).await } @@ -77,7 +77,7 @@ macro_rules! delegate_sandbox { &self, remote_path: &str, local_path: &std::path::Path, - ) -> Result<(), String> { + ) -> $crate::Result<()> { self.$field.download_file_to_local(remote_path, local_path).await } @@ -85,15 +85,15 @@ macro_rules! delegate_sandbox { &self, local_path: &std::path::Path, remote_path: &str, - ) -> Result<(), String> { + ) -> $crate::Result<()> { self.$field.upload_file_from_local(local_path, remote_path).await } - async fn initialize(&self) -> Result<(), String> { + async fn initialize(&self) -> $crate::Result<()> { self.$field.initialize().await } - async fn cleanup(&self) -> Result<(), String> { + async fn cleanup(&self) -> $crate::Result<()> { self.$field.cleanup().await } @@ -113,15 +113,15 @@ macro_rules! delegate_sandbox { self.$field.sandbox_info() } - async fn refresh_push_credentials(&self) -> Result<(), String> { + async fn refresh_push_credentials(&self) -> $crate::Result<()> { self.$field.refresh_push_credentials().await } - async fn set_autostop_interval(&self, minutes: i32) -> Result<(), String> { + async fn set_autostop_interval(&self, minutes: i32) -> $crate::Result<()> { self.$field.set_autostop_interval(minutes).await } - async fn setup_git_for_run(&self, run_id: &str) -> Result, String> { + async fn setup_git_for_run(&self, run_id: &str) -> $crate::Result> { self.$field.setup_git_for_run(run_id).await } @@ -147,7 +147,7 @@ macro_rules! delegate_sandbox { self.$field.parallel_worktree_path(run_dir, run_id, node_id, key) } - async fn ssh_access_command(&self) -> Result, String> { + async fn ssh_access_command(&self) -> $crate::Result> { self.$field.ssh_access_command().await } @@ -155,7 +155,7 @@ macro_rules! delegate_sandbox { self.$field.origin_url() } - async fn get_preview_url(&self, port: u16) -> Result)>, String> { + async fn get_preview_url(&self, port: u16) -> $crate::Result)>> { self.$field.get_preview_url(port).await } @@ -164,7 +164,7 @@ macro_rules! delegate_sandbox { path: &str, offset: Option, limit: Option, - ) -> Result { + ) -> $crate::Result { self.$field.read_file(path, offset, limit).await } @@ -173,7 +173,7 @@ macro_rules! delegate_sandbox { pattern: &str, path: &str, options: &$crate::GrepOptions, - ) -> Result, String> { + ) -> $crate::Result> { self.$field.grep(pattern, path, options).await } } @@ -198,6 +198,8 @@ pub enum SandboxEvent { InitializeFailed { provider: String, error: String, + #[serde(default, skip_serializing_if = "Vec::is_empty")] + causes: Vec, duration_ms: u64, }, CleanupStarted { @@ -210,6 +212,8 @@ pub enum SandboxEvent { CleanupFailed { provider: String, error: String, + #[serde(default, skip_serializing_if = "Vec::is_empty")] + causes: Vec, }, // -- Docker -- @@ -233,8 +237,10 @@ pub enum SandboxEvent { duration_ms: u64, }, SnapshotFailed { - name: String, - error: String, + name: String, + error: String, + #[serde(default, skip_serializing_if = "Vec::is_empty")] + causes: Vec, }, // -- Daytona git -- @@ -247,8 +253,10 @@ pub enum SandboxEvent { duration_ms: u64, }, GitCloneFailed { - url: String, - error: String, + url: String, + error: String, + #[serde(default, skip_serializing_if = "Vec::is_empty")] + causes: Vec, }, } @@ -269,9 +277,10 @@ impl SandboxEvent { Self::InitializeFailed { provider, error, + causes, duration_ms, } => { - error!(provider, error, duration_ms, "Sandbox init failed"); + error!(provider, error, causes = ?causes, duration_ms, "Sandbox init failed"); } Self::CleanupStarted { provider } => { debug!(provider, "Sandbox cleanup started"); @@ -282,8 +291,12 @@ impl SandboxEvent { } => { debug!(provider, duration_ms, "Sandbox cleanup completed"); } - Self::CleanupFailed { provider, error } => { - warn!(provider, error, "Sandbox cleanup failed"); + Self::CleanupFailed { + provider, + error, + causes, + } => { + warn!(provider, error, causes = ?causes, "Sandbox cleanup failed"); } Self::SnapshotPulling { name } => { debug!(name, "Snapshot pulling"); @@ -300,8 +313,12 @@ impl SandboxEvent { Self::SnapshotReady { name, duration_ms } => { info!(name, duration_ms, "Snapshot ready"); } - Self::SnapshotFailed { name, error } => { - error!(name, error, "Snapshot failed"); + Self::SnapshotFailed { + name, + error, + causes, + } => { + error!(name, error, causes = ?causes, "Snapshot failed"); } Self::GitCloneStarted { url, branch } => { debug!( @@ -313,8 +330,8 @@ impl SandboxEvent { Self::GitCloneCompleted { url, duration_ms } => { debug!(url, duration_ms, "Git clone completed"); } - Self::GitCloneFailed { url, error } => { - error!(url, error, "Git clone failed"); + Self::GitCloneFailed { url, error, causes } => { + error!(url, error, causes = ?causes, "Git clone failed"); } } } @@ -372,15 +389,15 @@ pub trait Sandbox: Send + Sync { path: &str, offset: Option, limit: Option, - ) -> Result; - async fn write_file(&self, path: &str, content: &str) -> Result<(), String>; - async fn delete_file(&self, path: &str) -> Result<(), String>; - async fn file_exists(&self, path: &str) -> Result; + ) -> crate::Result; + async fn write_file(&self, path: &str, content: &str) -> crate::Result<()>; + async fn delete_file(&self, path: &str) -> crate::Result<()>; + async fn file_exists(&self, path: &str) -> crate::Result; async fn list_directory( &self, path: &str, depth: Option, - ) -> Result, String>; + ) -> crate::Result>; async fn exec_command( &self, command: &str, @@ -388,30 +405,30 @@ pub trait Sandbox: Send + Sync { working_dir: Option<&str>, env_vars: Option<&std::collections::HashMap>, cancel_token: Option, - ) -> Result; + ) -> crate::Result; async fn grep( &self, pattern: &str, path: &str, options: &GrepOptions, - ) -> Result, String>; - async fn glob(&self, pattern: &str, path: Option<&str>) -> Result, String>; + ) -> crate::Result>; + async fn glob(&self, pattern: &str, path: Option<&str>) -> crate::Result>; /// Copy a file from the sandbox to a local filesystem path. /// Handles binary files correctly across all sandbox types. async fn download_file_to_local( &self, remote_path: &str, local_path: &Path, - ) -> Result<(), String>; + ) -> crate::Result<()>; /// Copy a file from the local filesystem into the sandbox. /// Handles binary files correctly across all sandbox types. async fn upload_file_from_local( &self, local_path: &Path, remote_path: &str, - ) -> Result<(), String>; - async fn initialize(&self) -> Result<(), String>; - async fn cleanup(&self) -> Result<(), String>; + ) -> crate::Result<()>; + async fn initialize(&self) -> crate::Result<()>; + async fn cleanup(&self) -> crate::Result<()>; fn working_directory(&self) -> &str; fn platform(&self) -> &str; fn os_version(&self) -> String; @@ -425,13 +442,13 @@ pub trait Sandbox: Send + Sync { /// Refresh git push credentials (e.g. rotate an expiring GitHub App token). /// Default is a no-op; Daytona overrides to update the remote URL with a /// fresh token. - async fn refresh_push_credentials(&self) -> Result<(), String> { + async fn refresh_push_credentials(&self) -> crate::Result<()> { Ok(()) } /// Set the auto-stop interval in minutes (0 to disable). /// Default is a no-op; Daytona overrides to call the Daytona API. - async fn set_autostop_interval(&self, _minutes: i32) -> Result<(), String> { + async fn set_autostop_interval(&self, _minutes: i32) -> crate::Result<()> { Ok(()) } @@ -439,7 +456,7 @@ pub trait Sandbox: Send + Sync { /// Sandboxes that manage their own git clone (e.g., remote VMs) should /// create a run branch and return the git info. Local sandboxes return /// `None`. - async fn setup_git_for_run(&self, _run_id: &str) -> Result, String> { + async fn setup_git_for_run(&self, _run_id: &str) -> crate::Result> { Ok(None) } @@ -481,7 +498,7 @@ pub trait Sandbox: Send + Sync { /// Return an SSH command string for connecting to this sandbox, if /// supported. - async fn ssh_access_command(&self) -> Result, String> { + async fn ssh_access_command(&self) -> crate::Result> { Ok(None) } @@ -497,7 +514,7 @@ pub trait Sandbox: Send + Sync { async fn get_preview_url( &self, _port: u16, - ) -> Result)>, String> { + ) -> crate::Result)>> { Ok(None) } @@ -530,12 +547,14 @@ pub fn shell_quote(s: &str) -> String { /// Helper for sandbox implementations that manage git internally. /// Executes git commands inside the sandbox to create a run branch. -pub async fn setup_git_via_exec(sandbox: &dyn Sandbox, run_id: &str) -> Result { +pub async fn setup_git_via_exec(sandbox: &dyn Sandbox, run_id: &str) -> crate::Result { // Get current branch name let branch_result = sandbox .exec_command("git rev-parse --abbrev-ref HEAD", 10_000, None, None, None) .await - .map_err(|e| format!("git rev-parse --abbrev-ref HEAD failed: {e}"))?; + .map_err(|e| { + crate::Error::message(format!("git rev-parse --abbrev-ref HEAD failed: {e}")) + })?; let base_branch = if branch_result.exit_code == 0 { let name = branch_result.stdout.trim().to_string(); if name.is_empty() || name == "HEAD" { @@ -551,12 +570,12 @@ pub async fn setup_git_via_exec(sandbox: &dyn Sandbox, run_id: &str) -> Result Result, limit: Option, - ) -> Result { + ) -> crate::Result { let content = self .files .get(path) .cloned() - .ok_or_else(|| format!("File not found: {path}"))?; + .ok_or_else(|| crate::Error::message(format!("File not found: {path}")))?; if self.apply_read_offset_limit { let lines: Vec<&str> = content.lines().collect(); @@ -107,7 +107,7 @@ impl Sandbox for MockSandbox { } } - async fn write_file(&self, path: &str, content: &str) -> Result<(), String> { + async fn write_file(&self, path: &str, content: &str) -> crate::Result<()> { self.written_files .lock() .expect("written_files lock poisoned") @@ -115,11 +115,11 @@ impl Sandbox for MockSandbox { Ok(()) } - async fn delete_file(&self, _path: &str) -> Result<(), String> { + async fn delete_file(&self, _path: &str) -> crate::Result<()> { Ok(()) } - async fn file_exists(&self, path: &str) -> Result { + async fn file_exists(&self, path: &str) -> crate::Result { Ok(self.files.contains_key(path)) } @@ -127,7 +127,7 @@ impl Sandbox for MockSandbox { &self, _path: &str, _depth: Option, - ) -> Result, String> { + ) -> crate::Result> { Ok(vec![]) } @@ -138,7 +138,7 @@ impl Sandbox for MockSandbox { working_dir: Option<&str>, env_vars: Option<&std::collections::HashMap>, _cancel_token: Option, - ) -> Result { + ) -> crate::Result { *self .captured_timeout .lock() @@ -167,11 +167,11 @@ impl Sandbox for MockSandbox { _pattern: &str, _path: &str, _options: &GrepOptions, - ) -> Result, String> { + ) -> crate::Result> { Ok(self.grep_results.clone()) } - async fn glob(&self, _pattern: &str, _path: Option<&str>) -> Result, String> { + async fn glob(&self, _pattern: &str, _path: Option<&str>) -> crate::Result> { Ok(self.glob_results.clone()) } @@ -179,19 +179,21 @@ impl Sandbox for MockSandbox { &self, remote_path: &str, local_path: &std::path::Path, - ) -> Result<(), String> { + ) -> crate::Result<()> { let content = self .files .get(remote_path) - .ok_or_else(|| format!("File not found: {remote_path}"))?; + .ok_or_else(|| crate::Error::message(format!("File not found: {remote_path}")))?; if let Some(parent) = local_path.parent() { fs::create_dir_all(parent) .await - .map_err(|e| format!("Failed to create parent dirs: {e}"))?; + .map_err(|e| crate::Error::context("Failed to create parent dirs", e))?; } fs::write(local_path, content.as_bytes()) .await - .map_err(|e| format!("Failed to write {}: {e}", local_path.display()))?; + .map_err(|e| { + crate::Error::context(format!("Failed to write {}", local_path.display()), e) + })?; Ok(()) } @@ -199,14 +201,17 @@ impl Sandbox for MockSandbox { &self, local_path: &std::path::Path, _remote_path: &str, - ) -> Result<(), String> { + ) -> crate::Result<()> { if !local_path.exists() { - return Err(format!("File not found: {}", local_path.display())); + return Err(crate::Error::message(format!( + "File not found: {}", + local_path.display() + ))); } Ok(()) } - async fn initialize(&self) -> Result<(), String> { + async fn initialize(&self) -> crate::Result<()> { self.emit(SandboxEvent::Initializing { provider: "mock".into(), }); @@ -221,7 +226,7 @@ impl Sandbox for MockSandbox { Ok(()) } - async fn cleanup(&self) -> Result<(), String> { + async fn cleanup(&self) -> crate::Result<()> { self.emit(SandboxEvent::CleanupStarted { provider: "mock".into(), }); @@ -269,16 +274,16 @@ impl Sandbox for MutableMockSandbox { path: &str, _offset: Option, _limit: Option, - ) -> Result { + ) -> crate::Result { self.files .lock() .expect("files lock poisoned") .get(path) .cloned() - .ok_or_else(|| format!("File not found: {path}")) + .ok_or_else(|| crate::Error::message(format!("File not found: {path}"))) } - async fn write_file(&self, path: &str, content: &str) -> Result<(), String> { + async fn write_file(&self, path: &str, content: &str) -> crate::Result<()> { self.files .lock() .expect("files lock poisoned") @@ -286,12 +291,12 @@ impl Sandbox for MutableMockSandbox { Ok(()) } - async fn delete_file(&self, path: &str) -> Result<(), String> { + async fn delete_file(&self, path: &str) -> crate::Result<()> { self.files.lock().expect("files lock poisoned").remove(path); Ok(()) } - async fn file_exists(&self, path: &str) -> Result { + async fn file_exists(&self, path: &str) -> crate::Result { Ok(self .files .lock() @@ -303,7 +308,7 @@ impl Sandbox for MutableMockSandbox { &self, _path: &str, _depth: Option, - ) -> Result, String> { + ) -> crate::Result> { Ok(vec![]) } @@ -314,7 +319,7 @@ impl Sandbox for MutableMockSandbox { _working_dir: Option<&str>, _env_vars: Option<&std::collections::HashMap>, _cancel_token: Option, - ) -> Result { + ) -> crate::Result { Ok(ExecResult { stdout: String::new(), stderr: String::new(), @@ -329,7 +334,7 @@ impl Sandbox for MutableMockSandbox { pattern: &str, _path: &str, _options: &GrepOptions, - ) -> Result, String> { + ) -> crate::Result> { let files = self.files.lock().expect("files lock poisoned"); let mut results = Vec::new(); for (path, content) in files.iter() { @@ -342,7 +347,7 @@ impl Sandbox for MutableMockSandbox { Ok(results) } - async fn glob(&self, _pattern: &str, _path: Option<&str>) -> Result, String> { + async fn glob(&self, _pattern: &str, _path: Option<&str>) -> crate::Result> { Ok(vec![]) } @@ -350,22 +355,24 @@ impl Sandbox for MutableMockSandbox { &self, remote_path: &str, local_path: &std::path::Path, - ) -> Result<(), String> { + ) -> crate::Result<()> { let content = self .files .lock() .expect("files lock poisoned") .get(remote_path) .cloned() - .ok_or_else(|| format!("File not found: {remote_path}"))?; + .ok_or_else(|| crate::Error::message(format!("File not found: {remote_path}")))?; if let Some(parent) = local_path.parent() { fs::create_dir_all(parent) .await - .map_err(|e| format!("Failed to create parent dirs: {e}"))?; + .map_err(|e| crate::Error::context("Failed to create parent dirs", e))?; } fs::write(local_path, content.as_bytes()) .await - .map_err(|e| format!("Failed to write {}: {e}", local_path.display()))?; + .map_err(|e| { + crate::Error::context(format!("Failed to write {}", local_path.display()), e) + })?; Ok(()) } @@ -373,10 +380,10 @@ impl Sandbox for MutableMockSandbox { &self, local_path: &std::path::Path, remote_path: &str, - ) -> Result<(), String> { - let content = fs::read_to_string(local_path) - .await - .map_err(|e| format!("Failed to read {}: {e}", local_path.display()))?; + ) -> crate::Result<()> { + let content = fs::read_to_string(local_path).await.map_err(|e| { + crate::Error::context(format!("Failed to read {}", local_path.display()), e) + })?; self.files .lock() .expect("files lock poisoned") @@ -384,11 +391,11 @@ impl Sandbox for MutableMockSandbox { Ok(()) } - async fn initialize(&self) -> Result<(), String> { + async fn initialize(&self) -> crate::Result<()> { Ok(()) } - async fn cleanup(&self) -> Result<(), String> { + async fn cleanup(&self) -> crate::Result<()> { Ok(()) } diff --git a/lib/crates/fabro-sandbox/src/worktree.rs b/lib/crates/fabro-sandbox/src/worktree.rs index 1d4a82d2b..46081d076 100644 --- a/lib/crates/fabro-sandbox/src/worktree.rs +++ b/lib/crates/fabro-sandbox/src/worktree.rs @@ -110,7 +110,7 @@ impl Sandbox for WorktreeSandbox { /// 3. Add the worktree, emit `WorktreeAdded`. /// /// Does NOT call `inner.initialize()`. - async fn initialize(&self) -> Result<(), String> { + async fn initialize(&self) -> crate::Result<()> { if self .initialized .swap(true, std::sync::atomic::Ordering::Relaxed) @@ -145,11 +145,11 @@ impl Sandbox for WorktreeSandbox { .exec_command(&cmd, 30_000, None, None, None) .await?; if result.exit_code != 0 { - return Err(format!( + return Err(crate::Error::message(format!( "git branch --force failed (exit {}): {}", result.exit_code, result.stderr.trim() - )); + ))); } self.emit(WorktreeEvent::BranchCreated { branch: self.config.branch_name.clone(), @@ -171,11 +171,11 @@ impl Sandbox for WorktreeSandbox { .exec_command(&rollback_cmd, 30_000, None, None, None) .await; } - return Err(format!( + return Err(crate::Error::message(format!( "git worktree add failed (exit {}): {}", result.exit_code, result.stderr.trim() - )); + ))); } self.emit(WorktreeEvent::WorktreeAdded { path: self.config.worktree_path.clone(), @@ -187,7 +187,7 @@ impl Sandbox for WorktreeSandbox { /// No-op — the worktree must survive cleanup for `fabro cp` access. /// Worktrees are pruned separately by `system prune`. - async fn cleanup(&self) -> Result<(), String> { + async fn cleanup(&self) -> crate::Result<()> { Ok(()) } @@ -204,7 +204,7 @@ impl Sandbox for WorktreeSandbox { working_dir: Option<&str>, env_vars: Option<&HashMap>, cancel_token: Option, - ) -> Result { + ) -> crate::Result { let wd = working_dir.unwrap_or(&self.config.worktree_path); self.inner .exec_command(command, timeout_ms, Some(wd), env_vars, cancel_token) @@ -218,22 +218,22 @@ impl Sandbox for WorktreeSandbox { path: &str, offset: Option, limit: Option, - ) -> Result { + ) -> crate::Result { let resolved = self.resolve_path(path); self.inner.read_file(&resolved, offset, limit).await } - async fn write_file(&self, path: &str, content: &str) -> Result<(), String> { + async fn write_file(&self, path: &str, content: &str) -> crate::Result<()> { let resolved = self.resolve_path(path); self.inner.write_file(&resolved, content).await } - async fn delete_file(&self, path: &str) -> Result<(), String> { + async fn delete_file(&self, path: &str) -> crate::Result<()> { let resolved = self.resolve_path(path); self.inner.delete_file(&resolved).await } - async fn file_exists(&self, path: &str) -> Result { + async fn file_exists(&self, path: &str) -> crate::Result { let resolved = self.resolve_path(path); self.inner.file_exists(&resolved).await } @@ -242,7 +242,7 @@ impl Sandbox for WorktreeSandbox { &self, path: &str, depth: Option, - ) -> Result, String> { + ) -> crate::Result> { let resolved = self.resolve_path(path); self.inner.list_directory(&resolved, depth).await } @@ -252,12 +252,12 @@ impl Sandbox for WorktreeSandbox { pattern: &str, path: &str, options: &GrepOptions, - ) -> Result, String> { + ) -> crate::Result> { let resolved = self.resolve_path(path); self.inner.grep(pattern, &resolved, options).await } - async fn glob(&self, pattern: &str, path: Option<&str>) -> Result, String> { + async fn glob(&self, pattern: &str, path: Option<&str>) -> crate::Result> { let resolved = path.map(|p| self.resolve_path(p)); let glob_path = resolved.as_deref().unwrap_or(&self.config.worktree_path); self.inner.glob(pattern, Some(glob_path)).await @@ -267,7 +267,7 @@ impl Sandbox for WorktreeSandbox { &self, remote_path: &str, local_path: &Path, - ) -> Result<(), String> { + ) -> crate::Result<()> { let resolved = self.resolve_path(remote_path); self.inner .download_file_to_local(&resolved, local_path) @@ -278,7 +278,7 @@ impl Sandbox for WorktreeSandbox { &self, local_path: &Path, remote_path: &str, - ) -> Result<(), String> { + ) -> crate::Result<()> { let resolved = self.resolve_path(remote_path); self.inner .upload_file_from_local(local_path, &resolved) @@ -297,11 +297,11 @@ impl Sandbox for WorktreeSandbox { self.inner.sandbox_info() } - async fn refresh_push_credentials(&self) -> Result<(), String> { + async fn refresh_push_credentials(&self) -> crate::Result<()> { self.inner.refresh_push_credentials().await } - async fn set_autostop_interval(&self, minutes: i32) -> Result<(), String> { + async fn set_autostop_interval(&self, minutes: i32) -> crate::Result<()> { self.inner.set_autostop_interval(minutes).await } @@ -309,7 +309,7 @@ impl Sandbox for WorktreeSandbox { Some(&self.config.worktree_path) } - async fn setup_git_for_run(&self, run_id: &str) -> Result, String> { + async fn setup_git_for_run(&self, run_id: &str) -> crate::Result> { self.inner.setup_git_for_run(run_id).await } @@ -332,7 +332,7 @@ impl Sandbox for WorktreeSandbox { .parallel_worktree_path(run_dir, run_id, node_id, key) } - async fn ssh_access_command(&self) -> Result, String> { + async fn ssh_access_command(&self) -> crate::Result> { self.inner.ssh_access_command().await } @@ -343,7 +343,7 @@ impl Sandbox for WorktreeSandbox { async fn get_preview_url( &self, port: u16, - ) -> Result)>, String> { + ) -> crate::Result)>> { self.inner.get_preview_url(port).await } diff --git a/lib/crates/fabro-sandbox/tests/error.rs b/lib/crates/fabro-sandbox/tests/error.rs new file mode 100644 index 000000000..e4f104637 --- /dev/null +++ b/lib/crates/fabro-sandbox/tests/error.rs @@ -0,0 +1,32 @@ +#[test] +fn context_error_preserves_source_cause() { + let source = std::io::Error::new(std::io::ErrorKind::PermissionDenied, "permission denied"); + + let error = fabro_sandbox::Error::context("Failed to read file", source); + + assert_eq!(error.to_string(), "Failed to read file"); + assert_eq!(error.causes(), vec!["permission denied"]); + assert_eq!( + error.display_with_causes(), + "Failed to read file\n caused by: permission denied" + ); +} + +#[cfg(feature = "docker")] +#[test] +fn docker_image_inspect_error_preserves_source_cause() { + let source = bollard::errors::Error::DockerResponseServerError { + status_code: 500, + message: "daemon unavailable".to_string(), + }; + + let error = fabro_sandbox::Error::docker_image_inspect("buildpack-deps:noble", source); + + assert_eq!( + error.to_string(), + "Failed to inspect Docker image buildpack-deps:noble" + ); + assert_eq!(error.causes(), vec![ + "Docker responded with status code 500: daemon unavailable" + ]); +} diff --git a/lib/crates/fabro-server/src/run_files.rs b/lib/crates/fabro-server/src/run_files.rs index 28bd0d42b..f664a70bb 100644 --- a/lib/crates/fabro-server/src/run_files.rs +++ b/lib/crates/fabro-server/src/run_files.rs @@ -670,7 +670,7 @@ async fn resolve_head_sha_and_time( None, ) .await - .map_err(|err| ApiError::new(StatusCode::SERVICE_UNAVAILABLE, err))?; + .map_err(|err| ApiError::new(StatusCode::SERVICE_UNAVAILABLE, err.display_with_causes()))?; if res.exit_code != 0 { return Err(ApiError::new( StatusCode::SERVICE_UNAVAILABLE, diff --git a/lib/crates/fabro-server/src/server.rs b/lib/crates/fabro-server/src/server.rs index c13184c83..1f10f03fb 100644 --- a/lib/crates/fabro-server/src/server.rs +++ b/lib/crates/fabro-server/src/server.rs @@ -6483,7 +6483,8 @@ async fn generate_preview_url( url: preview.url, }, Err(err) => { - return ApiError::new(StatusCode::CONFLICT, err).into_response(); + return ApiError::new(StatusCode::CONFLICT, err.display_with_causes()) + .into_response(); } } } else { @@ -6493,7 +6494,8 @@ async fn generate_preview_url( url: preview.url, }, Err(err) => { - return ApiError::new(StatusCode::CONFLICT, err).into_response(); + return ApiError::new(StatusCode::CONFLICT, err.display_with_causes()) + .into_response(); } } }; @@ -6517,7 +6519,7 @@ async fn create_ssh_access( }; match sandbox.create_ssh_access(Some(request.ttl_minutes)).await { Ok(command) => (StatusCode::CREATED, Json(SshAccessResponse { command })).into_response(), - Err(err) => ApiError::new(StatusCode::CONFLICT, err).into_response(), + Err(err) => ApiError::new(StatusCode::CONFLICT, err.display_with_causes()).into_response(), } } @@ -6547,7 +6549,7 @@ async fn list_sandbox_files( .collect(), }) .into_response(), - Err(err) => ApiError::new(StatusCode::NOT_FOUND, err).into_response(), + Err(err) => ApiError::new(StatusCode::NOT_FOUND, err.display_with_causes()).into_response(), } } @@ -6576,7 +6578,7 @@ async fn get_sandbox_file( .download_file_to_local(¶ms.path, temp.path()) .await { - return ApiError::new(StatusCode::NOT_FOUND, err).into_response(); + return ApiError::new(StatusCode::NOT_FOUND, err.display_with_causes()).into_response(); } match fs::read(temp.path()).await { Ok(bytes) => octet_stream_response(bytes.into()), @@ -6619,7 +6621,8 @@ async fn put_sandbox_file( .await { Ok(()) => StatusCode::NO_CONTENT.into_response(), - Err(err) => ApiError::new(StatusCode::INTERNAL_SERVER_ERROR, err).into_response(), + Err(err) => ApiError::new(StatusCode::INTERNAL_SERVER_ERROR, err.display_with_causes()) + .into_response(), } } @@ -6629,9 +6632,13 @@ async fn reconnect_run_sandbox( ) -> Result, Response> { let record = load_run_sandbox_record(state, run_id).await?; let daytona_api_key = state.vault_or_env(EnvVars::DAYTONA_API_KEY); - reconnect(&record, daytona_api_key) - .await - .map_err(|err| ApiError::new(StatusCode::CONFLICT, format!("{err}")).into_response()) + reconnect(&record, daytona_api_key).await.map_err(|err| { + let detail = fabro_util::error::render_with_causes( + &err.to_string(), + &fabro_util::error::collect_causes(err.as_ref()), + ); + ApiError::new(StatusCode::CONFLICT, detail).into_response() + }) } async fn reconnect_daytona_sandbox( @@ -6669,7 +6676,7 @@ async fn reconnect_daytona_sandbox( record.clone_branch.clone(), ) .await - .map_err(|err| ApiError::new(StatusCode::CONFLICT, err.clone()).into_response()) + .map_err(|err| ApiError::new(StatusCode::CONFLICT, err.display_with_causes()).into_response()) } async fn load_run_sandbox_record( diff --git a/lib/crates/fabro-store/Cargo.toml b/lib/crates/fabro-store/Cargo.toml index 9145a7730..016b55d3d 100644 --- a/lib/crates/fabro-store/Cargo.toml +++ b/lib/crates/fabro-store/Cargo.toml @@ -13,6 +13,7 @@ workspace = true [dependencies] fabro-types = { path = "../fabro-types" } +fabro-util = { path = "../fabro-util" } hex.workspace = true slatedb.workspace = true object_store.workspace = true diff --git a/lib/crates/fabro-store/src/run_state.rs b/lib/crates/fabro-store/src/run_state.rs index e60cdb7a2..b0535f18d 100644 --- a/lib/crates/fabro-store/src/run_state.rs +++ b/lib/crates/fabro-store/src/run_state.rs @@ -461,7 +461,10 @@ fn conclusion_from_failed(props: &RunFailedProps, timestamp: DateTime) -> C timestamp, status: StageStatus::Fail, duration_ms: props.duration_ms, - failure_reason: Some(props.error.clone()), + failure_reason: Some(fabro_util::error::render_with_causes( + &props.error, + &props.causes, + )), final_git_commit_sha: props.git_commit_sha.clone(), stages: Vec::new(), billing: None, @@ -1066,6 +1069,7 @@ mod tests { 1, EventBody::RunFailed(RunFailedProps { error: "boom".to_string(), + causes: Vec::new(), duration_ms: 42, reason: FailureReason::WorkflowError, git_commit_sha: Some("abc123".to_string()), @@ -1078,6 +1082,35 @@ mod tests { assert_eq!(state.final_patch.as_deref(), Some(patch)); } + #[test] + fn run_failed_projection_renders_causes() { + let mut state = RunProjection::default(); + state + .apply_event(&test_event( + 1, + EventBody::RunFailed(RunFailedProps { + error: "Engine error: Failed to initialize sandbox".to_string(), + causes: vec![ + "Failed to pull Docker image buildpack-deps:noble".to_string(), + "connection refused".to_string(), + ], + duration_ms: 42, + reason: FailureReason::WorkflowError, + git_commit_sha: None, + final_patch: None, + }), + None, + )) + .unwrap(); + + assert_eq!( + state.conclusion.unwrap().failure_reason.as_deref(), + Some( + "Engine error: Failed to initialize sandbox\n caused by: Failed to pull Docker image buildpack-deps:noble\n caused by: connection refused" + ) + ); + } + #[test] fn run_archived_captures_prior_status_and_preserves_reason() { use fabro_types::run_event::{RunArchivedProps, RunCompletedProps}; diff --git a/lib/crates/fabro-types/src/run_event/infra.rs b/lib/crates/fabro-types/src/run_event/infra.rs index 55aecf344..d4c40ce13 100644 --- a/lib/crates/fabro-types/src/run_event/infra.rs +++ b/lib/crates/fabro-types/src/run_event/infra.rs @@ -23,6 +23,8 @@ pub struct SandboxReadyProps { pub struct SandboxFailedProps { pub provider: String, pub error: String, + #[serde(default, skip_serializing_if = "Vec::is_empty")] + pub causes: Vec, pub duration_ms: u64, } @@ -41,6 +43,8 @@ pub struct SandboxCleanupCompletedProps { pub struct SandboxCleanupFailedProps { pub provider: String, pub error: String, + #[serde(default, skip_serializing_if = "Vec::is_empty")] + pub causes: Vec, } #[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] @@ -56,8 +60,10 @@ pub struct SnapshotCompletedProps { #[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] pub struct SnapshotFailedProps { - pub name: String, - pub error: String, + pub name: String, + pub error: String, + #[serde(default, skip_serializing_if = "Vec::is_empty")] + pub causes: Vec, } #[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] @@ -75,8 +81,10 @@ pub struct GitCloneCompletedProps { #[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] pub struct GitCloneFailedProps { - pub url: String, - pub error: String, + pub url: String, + pub error: String, + #[serde(default, skip_serializing_if = "Vec::is_empty")] + pub causes: Vec, } #[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] diff --git a/lib/crates/fabro-types/src/run_event/run.rs b/lib/crates/fabro-types/src/run_event/run.rs index 0cf6317a3..d8bc96866 100644 --- a/lib/crates/fabro-types/src/run_event/run.rs +++ b/lib/crates/fabro-types/src/run_event/run.rs @@ -125,6 +125,8 @@ pub struct RunCompletedProps { #[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] pub struct RunFailedProps { pub error: String, + #[serde(default, skip_serializing_if = "Vec::is_empty")] + pub causes: Vec, pub duration_ms: u64, pub reason: FailureReason, #[serde(default, skip_serializing_if = "Option::is_none")] diff --git a/lib/crates/fabro-util/src/error.rs b/lib/crates/fabro-util/src/error.rs new file mode 100644 index 000000000..bf78a1073 --- /dev/null +++ b/lib/crates/fabro-util/src/error.rs @@ -0,0 +1,28 @@ +pub fn collect_causes(error: &(dyn std::error::Error + 'static)) -> Vec { + let mut causes = Vec::new(); + let mut source = error.source(); + while let Some(cause) = source { + causes.push(cause.to_string()); + source = cause.source(); + } + causes +} + +pub fn collect_chain(error: &(dyn std::error::Error + 'static)) -> Vec { + let mut chain = vec![error.to_string()]; + chain.extend(collect_causes(error)); + chain +} + +pub fn render_with_causes(message: &str, causes: &[String]) -> String { + if causes.is_empty() { + return message.to_string(); + } + + let mut rendered = String::from(message); + for cause in causes { + rendered.push_str("\n caused by: "); + rendered.push_str(cause); + } + rendered +} diff --git a/lib/crates/fabro-util/src/lib.rs b/lib/crates/fabro-util/src/lib.rs index 8edce1215..303cfa4a4 100644 --- a/lib/crates/fabro-util/src/lib.rs +++ b/lib/crates/fabro-util/src/lib.rs @@ -3,6 +3,7 @@ pub mod browser; pub mod check_report; pub mod dev_token; pub mod env; +pub mod error; pub mod exit; pub mod home; pub mod json; diff --git a/lib/crates/fabro-util/tests/error_chain.rs b/lib/crates/fabro-util/tests/error_chain.rs new file mode 100644 index 000000000..96854e7cc --- /dev/null +++ b/lib/crates/fabro-util/tests/error_chain.rs @@ -0,0 +1,53 @@ +#[derive(Debug)] +struct Cause(&'static str); + +impl std::fmt::Display for Cause { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.write_str(self.0) + } +} + +impl std::error::Error for Cause {} + +#[derive(Debug)] +struct Outer { + message: &'static str, + source: Cause, +} + +impl std::fmt::Display for Outer { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.write_str(self.message) + } +} + +impl std::error::Error for Outer { + fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { + Some(&self.source) + } +} + +#[test] +fn collect_causes_walks_error_source_chain() { + let error = Outer { + message: "outer failure", + source: Cause("inner failure"), + }; + + assert_eq!(fabro_util::error::collect_causes(&error), vec![ + "inner failure" + ]); +} + +#[test] +fn render_with_causes_adds_indented_caused_by_lines() { + let rendered = fabro_util::error::render_with_causes("operation failed", &[ + "first cause".to_string(), + "second cause".to_string(), + ]); + + assert_eq!( + rendered, + "operation failed\n caused by: first cause\n caused by: second cause" + ); +} diff --git a/lib/crates/fabro-workflow/src/artifact.rs b/lib/crates/fabro-workflow/src/artifact.rs index 089f8d1eb..42a27cbd3 100644 --- a/lib/crates/fabro-workflow/src/artifact.rs +++ b/lib/crates/fabro-workflow/src/artifact.rs @@ -566,11 +566,11 @@ mod tests { _path: &str, _offset: Option, _limit: Option, - ) -> std::result::Result { - Err("not implemented".to_string()) + ) -> fabro_sandbox::Result { + Err("not implemented".into()) } - async fn write_file(&self, path: &str, content: &str) -> std::result::Result<(), String> { + async fn write_file(&self, path: &str, content: &str) -> fabro_sandbox::Result<()> { self.written .lock() .unwrap() @@ -578,11 +578,11 @@ mod tests { Ok(()) } - async fn delete_file(&self, _path: &str) -> std::result::Result<(), String> { - Err("not implemented".to_string()) + async fn delete_file(&self, _path: &str) -> fabro_sandbox::Result<()> { + Err("not implemented".into()) } - async fn file_exists(&self, _path: &str) -> std::result::Result { + async fn file_exists(&self, _path: &str) -> fabro_sandbox::Result { Ok(self.accessible) } @@ -590,8 +590,8 @@ mod tests { &self, _path: &str, _depth: Option, - ) -> std::result::Result, String> { - Err("not implemented".to_string()) + ) -> fabro_sandbox::Result> { + Err("not implemented".into()) } async fn exec_command( @@ -601,8 +601,8 @@ mod tests { _working_dir: Option<&str>, _env_vars: Option<&std::collections::HashMap>, _cancel_token: Option, - ) -> std::result::Result { - Err("not implemented".to_string()) + ) -> fabro_sandbox::Result { + Err("not implemented".into()) } async fn grep( @@ -610,39 +610,39 @@ mod tests { _pattern: &str, _path: &str, _options: &fabro_agent::GrepOptions, - ) -> std::result::Result, String> { - Err("not implemented".to_string()) + ) -> fabro_sandbox::Result> { + Err("not implemented".into()) } async fn glob( &self, _pattern: &str, _path: Option<&str>, - ) -> std::result::Result, String> { - Err("not implemented".to_string()) + ) -> fabro_sandbox::Result> { + Err("not implemented".into()) } async fn download_file_to_local( &self, _remote_path: &str, _local_path: &std::path::Path, - ) -> std::result::Result<(), String> { - Err("not implemented".to_string()) + ) -> fabro_sandbox::Result<()> { + Err("not implemented".into()) } async fn upload_file_from_local( &self, _local_path: &std::path::Path, _remote_path: &str, - ) -> std::result::Result<(), String> { - Err("not implemented".to_string()) + ) -> fabro_sandbox::Result<()> { + Err("not implemented".into()) } - async fn initialize(&self) -> std::result::Result<(), String> { + async fn initialize(&self) -> fabro_sandbox::Result<()> { Ok(()) } - async fn cleanup(&self) -> std::result::Result<(), String> { + async fn cleanup(&self) -> fabro_sandbox::Result<()> { Ok(()) } diff --git a/lib/crates/fabro-workflow/src/artifact_snapshot.rs b/lib/crates/fabro-workflow/src/artifact_snapshot.rs index ce9abeb47..c10c07e1e 100644 --- a/lib/crates/fabro-workflow/src/artifact_snapshot.rs +++ b/lib/crates/fabro-workflow/src/artifact_snapshot.rs @@ -285,7 +285,8 @@ pub async fn collect_artifacts( debug!(cmd = cmd.as_str(), "Collecting artifacts"); let result = sandbox .exec_command(&cmd, FIND_TIMEOUT_MS, None, None, None) - .await?; + .await + .map_err(|e| e.display_with_causes())?; let discovered = parse_find_output(&result.stdout, platform); let discovered = normalize_paths(discovered, root); @@ -323,9 +324,10 @@ pub async fn collect_artifacts( } }, Err(e) => { + let error = e.display_with_causes(); warn!( path = file.relative_path.as_str(), - error = e.as_str(), + error = error.as_str(), "Asset download failed" ); download_errors += 1; @@ -384,23 +386,23 @@ mod tests { _: &str, _: Option, _: Option, - ) -> Result { + ) -> fabro_sandbox::Result { Err("not implemented".into()) } - async fn write_file(&self, _: &str, _: &str) -> Result<(), String> { + async fn write_file(&self, _: &str, _: &str) -> fabro_sandbox::Result<()> { Ok(()) } - async fn delete_file(&self, _: &str) -> Result<(), String> { + async fn delete_file(&self, _: &str) -> fabro_sandbox::Result<()> { Ok(()) } - async fn file_exists(&self, _: &str) -> Result { + async fn file_exists(&self, _: &str) -> fabro_sandbox::Result { Ok(false) } async fn list_directory( &self, _: &str, _: Option, - ) -> Result, String> { + ) -> fabro_sandbox::Result> { Ok(vec![]) } async fn exec_command( @@ -410,7 +412,7 @@ mod tests { _: Option<&str>, _: Option<&std::collections::HashMap>, _: Option, - ) -> Result { + ) -> fabro_sandbox::Result { Ok(self.exec_result.clone()) } async fn grep( @@ -418,42 +420,41 @@ mod tests { _: &str, _: &str, _: &fabro_agent::sandbox::GrepOptions, - ) -> Result, String> { + ) -> fabro_sandbox::Result> { Ok(vec![]) } - async fn glob(&self, _: &str, _: Option<&str>) -> Result, String> { + async fn glob(&self, _: &str, _: Option<&str>) -> fabro_sandbox::Result> { Ok(vec![]) } async fn download_file_to_local( &self, remote_path: &str, local_path: &std::path::Path, - ) -> Result<(), String> { - let content = self - .files - .get(remote_path) - .ok_or_else(|| format!("File not found: {remote_path}"))?; + ) -> fabro_sandbox::Result<()> { + let content = self.files.get(remote_path).ok_or_else(|| { + fabro_sandbox::Error::message(format!("File not found: {remote_path}")) + })?; if let Some(parent) = local_path.parent() { fs::create_dir_all(parent) .await - .map_err(|e| format!("Failed to create dirs: {e}"))?; + .map_err(|e| fabro_sandbox::Error::context("Failed to create dirs", e))?; } fs::write(local_path, content.as_bytes()) .await - .map_err(|e| format!("Failed to write: {e}"))?; + .map_err(|e| fabro_sandbox::Error::context("Failed to write", e))?; Ok(()) } async fn upload_file_from_local( &self, _local_path: &std::path::Path, _remote_path: &str, - ) -> Result<(), String> { + ) -> fabro_sandbox::Result<()> { Ok(()) } - async fn initialize(&self) -> Result<(), String> { + async fn initialize(&self) -> fabro_sandbox::Result<()> { Ok(()) } - async fn cleanup(&self) -> Result<(), String> { + async fn cleanup(&self) -> fabro_sandbox::Result<()> { Ok(()) } fn working_directory(&self) -> &str { diff --git a/lib/crates/fabro-workflow/src/devcontainer_bridge.rs b/lib/crates/fabro-workflow/src/devcontainer_bridge.rs index b206e06de..57592eb56 100644 --- a/lib/crates/fabro-workflow/src/devcontainer_bridge.rs +++ b/lib/crates/fabro-workflow/src/devcontainer_bridge.rs @@ -278,23 +278,23 @@ mod tests { _path: &str, _offset: Option, _limit: Option, - ) -> Result { + ) -> fabro_sandbox::Result { Ok(String::new()) } - async fn write_file(&self, _path: &str, _content: &str) -> Result<(), String> { + async fn write_file(&self, _path: &str, _content: &str) -> fabro_sandbox::Result<()> { Ok(()) } - async fn delete_file(&self, _path: &str) -> Result<(), String> { + async fn delete_file(&self, _path: &str) -> fabro_sandbox::Result<()> { Ok(()) } - async fn file_exists(&self, _path: &str) -> Result { + async fn file_exists(&self, _path: &str) -> fabro_sandbox::Result { Ok(false) } async fn list_directory( &self, _path: &str, _depth: Option, - ) -> Result, String> { + ) -> fabro_sandbox::Result> { Ok(vec![]) } async fn exec_command( @@ -304,14 +304,15 @@ mod tests { _working_dir: Option<&str>, _env_vars: Option<&std::collections::HashMap>, cancel_token: Option, - ) -> Result { + ) -> fabro_sandbox::Result { self.commands.lock().unwrap().push(command.to_string()); self.cancel_tokens .lock() .unwrap() .push(cancel_token.is_some()); if self.wait_for_cancel { - let token = cancel_token.ok_or_else(|| "missing cancel token".to_string())?; + let token = cancel_token + .ok_or_else(|| fabro_sandbox::Error::message("missing cancel token"))?; token.cancelled().await; return Ok(ExecResult { stdout: String::new(), @@ -338,30 +339,34 @@ mod tests { _pattern: &str, _path: &str, _options: &GrepOptions, - ) -> Result, String> { + ) -> fabro_sandbox::Result> { Ok(vec![]) } - async fn glob(&self, _pattern: &str, _path: Option<&str>) -> Result, String> { + async fn glob( + &self, + _pattern: &str, + _path: Option<&str>, + ) -> fabro_sandbox::Result> { Ok(vec![]) } async fn download_file_to_local( &self, _remote_path: &str, _local_path: &std::path::Path, - ) -> Result<(), String> { + ) -> fabro_sandbox::Result<()> { Ok(()) } async fn upload_file_from_local( &self, _local_path: &std::path::Path, _remote_path: &str, - ) -> Result<(), String> { + ) -> fabro_sandbox::Result<()> { Ok(()) } - async fn initialize(&self) -> Result<(), String> { + async fn initialize(&self) -> fabro_sandbox::Result<()> { Ok(()) } - async fn cleanup(&self) -> Result<(), String> { + async fn cleanup(&self) -> fabro_sandbox::Result<()> { Ok(()) } fn working_directory(&self) -> &str { diff --git a/lib/crates/fabro-workflow/src/error.rs b/lib/crates/fabro-workflow/src/error.rs index af833bc15..ec58270e5 100644 --- a/lib/crates/fabro-workflow/src/error.rs +++ b/lib/crates/fabro-workflow/src/error.rs @@ -209,12 +209,16 @@ pub enum Error { Engine { message: String, failure_class: FailureCategory, + #[serde(default, skip_serializing_if = "Vec::is_empty")] + causes: Vec, }, #[error("Handler error: {message}")] Handler { message: String, failure_class: FailureCategory, + #[serde(default, skip_serializing_if = "Vec::is_empty")] + causes: Vec, }, #[error("LLM error: {0}")] @@ -251,6 +255,22 @@ impl Error { Self::Handler { message, failure_class, + causes: Vec::new(), + } + } + + pub fn handler_with_source( + message: impl Into, + source: &(dyn std::error::Error + 'static), + ) -> Self { + let message = message.into(); + let causes = fabro_util::error::collect_chain(source); + let rendered = fabro_util::error::render_with_causes(&message, &causes); + let failure_class = classify_failure_reason(&rendered); + Self::Handler { + message, + failure_class, + causes, } } @@ -262,9 +282,39 @@ impl Error { Self::Engine { message, failure_class, + causes: Vec::new(), } } + pub fn engine_with_source( + message: impl Into, + source: &(dyn std::error::Error + 'static), + ) -> Self { + let message = message.into(); + let causes = fabro_util::error::collect_chain(source); + let rendered = fabro_util::error::render_with_causes(&message, &causes); + let failure_class = classify_failure_reason(&rendered); + Self::Engine { + message, + failure_class, + causes, + } + } + + #[must_use] + pub fn causes(&self) -> Vec { + match self { + Self::Engine { causes, .. } | Self::Handler { causes, .. } => causes.clone(), + Self::Llm(err) => fabro_util::error::collect_causes(err), + _ => Vec::new(), + } + } + + #[must_use] + pub fn display_with_causes(&self) -> String { + fabro_util::error::render_with_causes(&self.to_string(), &self.causes()) + } + /// Whether this error category is retryable (transient) or terminal. /// /// Retryable: Handler (transient handler failures), Engine (could be @@ -322,7 +372,7 @@ impl Error { /// Build a fail `Outcome` with structured `FailureDetail`. pub fn to_fail_outcome(&self) -> Outcome { let failure = FailureDetail { - message: self.to_string(), + message: self.display_with_causes(), category: self.failure_category(), signature: self.failure_signature_hint(), }; @@ -390,6 +440,35 @@ mod tests { use super::*; use crate::outcome::OutcomeExt; + #[derive(Debug)] + struct TestCause(&'static str); + + impl std::fmt::Display for TestCause { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.write_str(self.0) + } + } + + impl std::error::Error for TestCause {} + + #[derive(Debug)] + struct TestOuterError { + message: &'static str, + source: TestCause, + } + + impl std::fmt::Display for TestOuterError { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.write_str(self.message) + } + } + + impl std::error::Error for TestOuterError { + fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { + Some(&self.source) + } + } + #[test] fn parse_error_display() { let err = Error::Parse("unexpected token".to_string()); @@ -423,6 +502,29 @@ mod tests { assert_eq!(err.to_string(), "Engine error: no outgoing edge"); } + #[test] + fn engine_error_with_source_preserves_cause_chain() { + let source = TestOuterError { + message: "Failed to pull Docker image buildpack-deps:noble", + source: TestCause("connection refused"), + }; + let err = Error::engine_with_source("Failed to initialize sandbox", &source); + + assert_eq!( + err.to_string(), + "Engine error: Failed to initialize sandbox" + ); + assert_eq!(err.causes(), vec![ + "Failed to pull Docker image buildpack-deps:noble".to_string(), + "connection refused".to_string(), + ]); + assert_eq!( + err.display_with_causes(), + "Engine error: Failed to initialize sandbox\n caused by: Failed to pull Docker image buildpack-deps:noble\n caused by: connection refused" + ); + assert_eq!(err.failure_category(), FailureCategory::TransientInfra); + } + #[test] fn handler_error_display() { let err = Error::handler("LLM call failed"); diff --git a/lib/crates/fabro-workflow/src/event.rs b/lib/crates/fabro-workflow/src/event.rs index 250683c1b..bf7e8dafb 100644 --- a/lib/crates/fabro-workflow/src/event.rs +++ b/lib/crates/fabro-workflow/src/event.rs @@ -660,7 +660,12 @@ impl Event { Self::WorkflowRunFailed { error, duration_ms, .. } => { - error!(error = %error, duration_ms, "Workflow run failed"); + error!( + error = %error, + causes = ?error.causes(), + duration_ms, + "Workflow run failed" + ); } Self::RunNotice { level, @@ -1643,6 +1648,7 @@ fn event_body_from_event(event: &Event) -> EventBody { final_patch, } => EventBody::RunFailed(fabro_types::RunFailedProps { error: error.to_string(), + causes: error.causes(), duration_ms: *duration_ms, reason: *reason, git_commit_sha: git_commit_sha.clone(), @@ -2160,10 +2166,12 @@ fn event_body_from_event(event: &Event) -> EventBody { SandboxEvent::InitializeFailed { provider, error, + causes, duration_ms, } => EventBody::SandboxFailed(fabro_types::SandboxFailedProps { provider: provider.clone(), error: error.clone(), + causes: causes.clone(), duration_ms: *duration_ms, }), SandboxEvent::CleanupStarted { provider } => { @@ -2178,12 +2186,15 @@ fn event_body_from_event(event: &Event) -> EventBody { provider: provider.clone(), duration_ms: *duration_ms, }), - SandboxEvent::CleanupFailed { provider, error } => { - EventBody::SandboxCleanupFailed(fabro_types::SandboxCleanupFailedProps { - provider: provider.clone(), - error: error.clone(), - }) - } + SandboxEvent::CleanupFailed { + provider, + error, + causes, + } => EventBody::SandboxCleanupFailed(fabro_types::SandboxCleanupFailedProps { + provider: provider.clone(), + error: error.clone(), + causes: causes.clone(), + }), SandboxEvent::SnapshotPulling { name } => { EventBody::SnapshotPulling(fabro_types::SnapshotNameProps { name: name.clone() }) } @@ -2205,12 +2216,15 @@ fn event_body_from_event(event: &Event) -> EventBody { duration_ms: *duration_ms, }) } - SandboxEvent::SnapshotFailed { name, error } => { - EventBody::SnapshotFailed(fabro_types::SnapshotFailedProps { - name: name.clone(), - error: error.clone(), - }) - } + SandboxEvent::SnapshotFailed { + name, + error, + causes, + } => EventBody::SnapshotFailed(fabro_types::SnapshotFailedProps { + name: name.clone(), + error: error.clone(), + causes: causes.clone(), + }), SandboxEvent::GitCloneStarted { url, branch } => { EventBody::GitCloneStarted(fabro_types::GitCloneStartedProps { url: url.clone(), @@ -2223,10 +2237,11 @@ fn event_body_from_event(event: &Event) -> EventBody { duration_ms: *duration_ms, }) } - SandboxEvent::GitCloneFailed { url, error } => { + SandboxEvent::GitCloneFailed { url, error, causes } => { EventBody::GitCloneFailed(fabro_types::GitCloneFailedProps { - url: url.clone(), - error: error.clone(), + url: url.clone(), + error: error.clone(), + causes: causes.clone(), }) } }, @@ -3163,6 +3178,30 @@ mod tests { assert_eq!(properties["duration_ms"], 2500); } + #[test] + fn run_event_sandbox_failure_serializes_causes() { + let stored = to_run_event(&fixtures::RUN_5, &Event::Sandbox { + event: SandboxEvent::InitializeFailed { + provider: "docker".to_string(), + error: "Failed to pull Docker image buildpack-deps:noble".to_string(), + causes: vec!["connection refused".to_string()], + duration_ms: 42, + }, + }); + + assert_eq!(stored.event_name(), "sandbox.failed"); + let properties = stored.properties().unwrap(); + assert_eq!(properties["provider"], "docker"); + assert_eq!( + properties["error"], + "Failed to pull Docker image buildpack-deps:noble" + ); + assert_eq!( + properties["causes"], + serde_json::json!(["connection refused"]) + ); + } + #[test] fn run_event_workflow_failure_uses_display_error() { let stored = to_run_event(&fixtures::RUN_6, &Event::WorkflowRunFailed { @@ -3179,6 +3218,39 @@ mod tests { assert_eq!(properties["duration_ms"], 900); } + #[derive(Debug)] + struct EventTestCause; + + impl std::fmt::Display for EventTestCause { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.write_str("connection refused") + } + } + + impl std::error::Error for EventTestCause {} + + #[test] + fn run_event_workflow_failure_serializes_causes() { + let source = EventTestCause; + let stored = to_run_event(&fixtures::RUN_6, &Event::WorkflowRunFailed { + error: Error::engine_with_source("Failed to initialize sandbox", &source), + duration_ms: 900, + reason: FailureReason::WorkflowError, + git_commit_sha: None, + final_patch: None, + }); + + let properties = stored.properties().unwrap(); + assert_eq!( + properties["error"], + "Engine error: Failed to initialize sandbox" + ); + assert_eq!( + properties["causes"], + serde_json::json!(["connection refused"]) + ); + } + #[tokio::test] async fn append_event_writes_store_event_shape() { let store = fabro_store::Database::new( diff --git a/lib/crates/fabro-workflow/src/handler/command.rs b/lib/crates/fabro-workflow/src/handler/command.rs index 6dd7af4aa..cf83c79c2 100644 --- a/lib/crates/fabro-workflow/src/handler/command.rs +++ b/lib/crates/fabro-workflow/src/handler/command.rs @@ -739,23 +739,23 @@ mod tests { _: &str, _: Option, _: Option, - ) -> Result { + ) -> fabro_sandbox::Result { unimplemented!() } - async fn write_file(&self, _: &str, _: &str) -> Result<(), String> { + async fn write_file(&self, _: &str, _: &str) -> fabro_sandbox::Result<()> { unimplemented!() } - async fn delete_file(&self, _: &str) -> Result<(), String> { + async fn delete_file(&self, _: &str) -> fabro_sandbox::Result<()> { unimplemented!() } - async fn file_exists(&self, _: &str) -> Result { + async fn file_exists(&self, _: &str) -> fabro_sandbox::Result { unimplemented!() } async fn list_directory( &self, _: &str, _: Option, - ) -> Result, String> { + ) -> fabro_sandbox::Result> { unimplemented!() } async fn exec_command( @@ -765,7 +765,7 @@ mod tests { _working_dir: Option<&str>, env_vars: Option<&std::collections::HashMap>, cancel_token: Option, - ) -> Result { + ) -> fabro_sandbox::Result { *self.captured_command.lock().unwrap() = Some(command.to_string()); *self.captured_env_vars.lock().unwrap() = env_vars.cloned(); *self.captured_cancel_token.lock().unwrap() = Some(cancel_token.is_some()); @@ -776,22 +776,30 @@ mod tests { _: &str, _: &str, _: &fabro_agent::sandbox::GrepOptions, - ) -> Result, String> { + ) -> fabro_sandbox::Result> { unimplemented!() } - async fn glob(&self, _: &str, _: Option<&str>) -> Result, String> { + async fn glob(&self, _: &str, _: Option<&str>) -> fabro_sandbox::Result> { unimplemented!() } - async fn download_file_to_local(&self, _: &str, _: &std::path::Path) -> Result<(), String> { + async fn download_file_to_local( + &self, + _: &str, + _: &std::path::Path, + ) -> fabro_sandbox::Result<()> { unimplemented!() } - async fn upload_file_from_local(&self, _: &std::path::Path, _: &str) -> Result<(), String> { + async fn upload_file_from_local( + &self, + _: &std::path::Path, + _: &str, + ) -> fabro_sandbox::Result<()> { unimplemented!() } - async fn initialize(&self) -> Result<(), String> { + async fn initialize(&self) -> fabro_sandbox::Result<()> { Ok(()) } - async fn cleanup(&self) -> Result<(), String> { + async fn cleanup(&self) -> fabro_sandbox::Result<()> { Ok(()) } fn working_directory(&self) -> &str { diff --git a/lib/crates/fabro-workflow/src/handler/llm/cli.rs b/lib/crates/fabro-workflow/src/handler/llm/cli.rs index d837bfa55..f3989dce1 100644 --- a/lib/crates/fabro-workflow/src/handler/llm/cli.rs +++ b/lib/crates/fabro-workflow/src/handler/llm/cli.rs @@ -890,23 +890,23 @@ mod tests { _path: &str, _offset: Option, _limit: Option, - ) -> Result { + ) -> fabro_sandbox::Result { Ok(String::new()) } - async fn write_file(&self, _path: &str, _content: &str) -> Result<(), String> { + async fn write_file(&self, _path: &str, _content: &str) -> fabro_sandbox::Result<()> { Ok(()) } - async fn delete_file(&self, _path: &str) -> Result<(), String> { + async fn delete_file(&self, _path: &str) -> fabro_sandbox::Result<()> { Ok(()) } - async fn file_exists(&self, _path: &str) -> Result { + async fn file_exists(&self, _path: &str) -> fabro_sandbox::Result { Ok(false) } async fn list_directory( &self, _path: &str, _depth: Option, - ) -> Result, String> { + ) -> fabro_sandbox::Result> { Ok(vec![]) } async fn exec_command( @@ -916,35 +916,47 @@ mod tests { _working_dir: Option<&str>, _env_vars: Option<&std::collections::HashMap>, _cancel_token: Option, - ) -> Result { + ) -> fabro_sandbox::Result { self.commands.lock().unwrap().push(command.to_string()); self.results .lock() .unwrap() .pop_front() - .ok_or_else(|| "no more mock results".to_string()) + .ok_or_else(|| fabro_sandbox::Error::message("no more mock results")) } async fn grep( &self, _pattern: &str, _path: &str, _options: &GrepOptions, - ) -> Result, String> { + ) -> fabro_sandbox::Result> { Ok(vec![]) } - async fn glob(&self, _pattern: &str, _path: Option<&str>) -> Result, String> { + async fn glob( + &self, + _pattern: &str, + _path: Option<&str>, + ) -> fabro_sandbox::Result> { Ok(vec![]) } - async fn download_file_to_local(&self, _remote: &str, _local: &Path) -> Result<(), String> { + async fn download_file_to_local( + &self, + _remote: &str, + _local: &Path, + ) -> fabro_sandbox::Result<()> { Ok(()) } - async fn upload_file_from_local(&self, _local: &Path, _remote: &str) -> Result<(), String> { + async fn upload_file_from_local( + &self, + _local: &Path, + _remote: &str, + ) -> fabro_sandbox::Result<()> { Ok(()) } - async fn initialize(&self) -> Result<(), String> { + async fn initialize(&self) -> fabro_sandbox::Result<()> { Ok(()) } - async fn cleanup(&self) -> Result<(), String> { + async fn cleanup(&self) -> fabro_sandbox::Result<()> { Ok(()) } fn working_directory(&self) -> &str { @@ -956,7 +968,7 @@ mod tests { fn os_version(&self) -> String { "Ubuntu 22.04".to_string() } - async fn set_autostop_interval(&self, _minutes: i32) -> Result<(), String> { + async fn set_autostop_interval(&self, _minutes: i32) -> fabro_sandbox::Result<()> { Ok(()) } } diff --git a/lib/crates/fabro-workflow/src/pipeline/finalize.rs b/lib/crates/fabro-workflow/src/pipeline/finalize.rs index 470e53381..cee2fb1c8 100644 --- a/lib/crates/fabro-workflow/src/pipeline/finalize.rs +++ b/lib/crates/fabro-workflow/src/pipeline/finalize.rs @@ -43,7 +43,7 @@ pub fn classify_engine_result( ), Err(err) => ( StageStatus::Fail, - Some(err.to_string()), + Some(err.display_with_causes()), RunStatus::Failed { reason: FailureReason::WorkflowError, }, @@ -284,7 +284,11 @@ async fn cleanup_sandbox( ); let _ = services.run_hooks(&hook_ctx).await; if !preserve { - services.sandbox.cleanup().await?; + services + .sandbox + .cleanup() + .await + .map_err(|e| e.display_with_causes())?; } Ok(()) } diff --git a/lib/crates/fabro-workflow/src/pipeline/initialize.rs b/lib/crates/fabro-workflow/src/pipeline/initialize.rs index efc677aee..c8b10b384 100644 --- a/lib/crates/fabro-workflow/src/pipeline/initialize.rs +++ b/lib/crates/fabro-workflow/src/pipeline/initialize.rs @@ -536,7 +536,7 @@ pub async fn initialize( sandbox .initialize() .await - .map_err(|e| Error::engine(format!("Failed to initialize sandbox: {e}")))?; + .map_err(|e| Error::engine_with_source("Failed to initialize sandbox", &e))?; let hook_ctx = HookContext::new( HookEvent::SandboxReady, diff --git a/lib/crates/fabro-workflow/src/sandbox_git.rs b/lib/crates/fabro-workflow/src/sandbox_git.rs index 67d18f29a..93ad1c9e2 100644 --- a/lib/crates/fabro-workflow/src/sandbox_git.rs +++ b/lib/crates/fabro-workflow/src/sandbox_git.rs @@ -218,7 +218,7 @@ pub(crate) async fn git_diff_with_timeout( { Ok(r) if r.exit_code == 0 => Ok(r.stdout), Ok(r) => Err(exec_err("git diff", &r)), - Err(e) => Err(e.clone()), + Err(e) => Err(e.display_with_causes()), } } @@ -401,7 +401,9 @@ pub async fn list_changed_files_raw( let res = sandbox .exec_command(&cmd, 10_000, None, Some(&env), None) .await - .map_err(|e| DiffError::Transient { message: e })?; + .map_err(|e| DiffError::Transient { + message: e.display_with_causes(), + })?; if res.timed_out { return Err(DiffError::Transient { @@ -585,7 +587,9 @@ pub async fn list_diff_numstat( let res = sandbox .exec_command(&cmd, 10_000, None, Some(&env), None) .await - .map_err(|e| DiffError::Transient { message: e })?; + .map_err(|e| DiffError::Transient { + message: e.display_with_causes(), + })?; if res.timed_out { return Err(DiffError::Transient { @@ -674,7 +678,9 @@ pub async fn stream_blob_metadata( let res = sandbox .exec_command(&cmd, 10_000, None, Some(&env), None) .await - .map_err(|e| DiffError::Transient { message: e })?; + .map_err(|e| DiffError::Transient { + message: e.display_with_causes(), + })?; if res.timed_out { return Err(DiffError::Transient { @@ -739,7 +745,9 @@ pub async fn stream_blobs( let res = sandbox .exec_command(&cmd, 10_000, None, Some(&env), None) .await - .map_err(|e| DiffError::Transient { message: e })?; + .map_err(|e| DiffError::Transient { + message: e.display_with_causes(), + })?; if res.timed_out { return Err(DiffError::Transient { @@ -853,19 +861,19 @@ mod tests { _path: &str, _offset: Option, _limit: Option, - ) -> Result { - Err("read_file not implemented for ScriptedSandbox".to_string()) + ) -> fabro_sandbox::Result { + Err("read_file not implemented for ScriptedSandbox".into()) } - async fn write_file(&self, _path: &str, _content: &str) -> Result<(), String> { + async fn write_file(&self, _path: &str, _content: &str) -> fabro_sandbox::Result<()> { Ok(()) } - async fn delete_file(&self, _path: &str) -> Result<(), String> { + async fn delete_file(&self, _path: &str) -> fabro_sandbox::Result<()> { Ok(()) } - async fn file_exists(&self, _path: &str) -> Result { + async fn file_exists(&self, _path: &str) -> fabro_sandbox::Result { Ok(false) } @@ -873,7 +881,7 @@ mod tests { &self, _path: &str, _depth: Option, - ) -> Result, String> { + ) -> fabro_sandbox::Result> { Ok(Vec::new()) } @@ -884,12 +892,12 @@ mod tests { _working_dir: Option<&str>, _env_vars: Option<&std::collections::HashMap>, _cancel_token: Option, - ) -> Result { + ) -> fabro_sandbox::Result { self.exec_results .lock() .expect("exec_results lock poisoned") .pop_front() - .ok_or_else(|| "unexpected exec_command call".to_string()) + .ok_or_else(|| fabro_sandbox::Error::message("unexpected exec_command call")) } async fn grep( @@ -897,11 +905,15 @@ mod tests { _pattern: &str, _path: &str, _options: &GrepOptions, - ) -> Result, String> { + ) -> fabro_sandbox::Result> { Ok(Vec::new()) } - async fn glob(&self, _pattern: &str, _path: Option<&str>) -> Result, String> { + async fn glob( + &self, + _pattern: &str, + _path: Option<&str>, + ) -> fabro_sandbox::Result> { Ok(Vec::new()) } @@ -909,7 +921,7 @@ mod tests { &self, _remote_path: &str, _local_path: &std::path::Path, - ) -> Result<(), String> { + ) -> fabro_sandbox::Result<()> { Ok(()) } @@ -917,15 +929,15 @@ mod tests { &self, _local_path: &std::path::Path, _remote_path: &str, - ) -> Result<(), String> { + ) -> fabro_sandbox::Result<()> { Ok(()) } - async fn initialize(&self) -> Result<(), String> { + async fn initialize(&self) -> fabro_sandbox::Result<()> { Ok(()) } - async fn cleanup(&self) -> Result<(), String> { + async fn cleanup(&self) -> fabro_sandbox::Result<()> { Ok(()) } diff --git a/lib/crates/fabro-workflow/tests/it/daytona_integration.rs b/lib/crates/fabro-workflow/tests/it/daytona_integration.rs index 04da9678e..c7f05506f 100644 --- a/lib/crates/fabro-workflow/tests/it/daytona_integration.rs +++ b/lib/crates/fabro-workflow/tests/it/daytona_integration.rs @@ -1419,7 +1419,7 @@ async fn daytona_ssh_access_before_init_fails() { let result = env.create_ssh_access(Some(60.0)).await; assert!(result.is_err(), "should fail before initialize()"); assert!( - result.unwrap_err().contains("not initialized"), + result.unwrap_err().to_string().contains("not initialized"), "error should mention not initialized" ); } diff --git a/lib/crates/fabro-workflow/tests/it/integration.rs b/lib/crates/fabro-workflow/tests/it/integration.rs index 069835e0a..35ccbf424 100644 --- a/lib/crates/fabro-workflow/tests/it/integration.rs +++ b/lib/crates/fabro-workflow/tests/it/integration.rs @@ -8823,11 +8823,11 @@ impl fabro_agent::Sandbox for RemoteMockEnv { _path: &str, _offset: Option, _limit: Option, - ) -> std::result::Result { - Err("not implemented".to_string()) + ) -> fabro_sandbox::Result { + Err("not implemented".into()) } - async fn write_file(&self, path: &str, content: &str) -> std::result::Result<(), String> { + async fn write_file(&self, path: &str, content: &str) -> fabro_sandbox::Result<()> { self.written .lock() .unwrap() @@ -8836,11 +8836,11 @@ impl fabro_agent::Sandbox for RemoteMockEnv { Ok(()) } - async fn delete_file(&self, _path: &str) -> std::result::Result<(), String> { - Err("not implemented".to_string()) + async fn delete_file(&self, _path: &str) -> fabro_sandbox::Result<()> { + Err("not implemented".into()) } - async fn file_exists(&self, path: &str) -> std::result::Result { + async fn file_exists(&self, path: &str) -> fabro_sandbox::Result { Ok(self.existing_paths.lock().unwrap().contains(path)) } @@ -8848,8 +8848,8 @@ impl fabro_agent::Sandbox for RemoteMockEnv { &self, _path: &str, _depth: Option, - ) -> std::result::Result, String> { - Err("not implemented".to_string()) + ) -> fabro_sandbox::Result> { + Err("not implemented".into()) } async fn exec_command( @@ -8859,8 +8859,8 @@ impl fabro_agent::Sandbox for RemoteMockEnv { _working_dir: Option<&str>, _env_vars: Option<&std::collections::HashMap>, _cancel_token: Option, - ) -> std::result::Result { - Err("not implemented".to_string()) + ) -> fabro_sandbox::Result { + Err("not implemented".into()) } async fn grep( @@ -8868,23 +8868,23 @@ impl fabro_agent::Sandbox for RemoteMockEnv { _pattern: &str, _path: &str, _options: &fabro_agent::GrepOptions, - ) -> std::result::Result, String> { - Err("not implemented".to_string()) + ) -> fabro_sandbox::Result> { + Err("not implemented".into()) } async fn glob( &self, _pattern: &str, _path: Option<&str>, - ) -> std::result::Result, String> { - Err("not implemented".to_string()) + ) -> fabro_sandbox::Result> { + Err("not implemented".into()) } - async fn initialize(&self) -> std::result::Result<(), String> { + async fn initialize(&self) -> fabro_sandbox::Result<()> { Ok(()) } - async fn cleanup(&self) -> std::result::Result<(), String> { + async fn cleanup(&self) -> fabro_sandbox::Result<()> { Ok(()) } @@ -8892,16 +8892,16 @@ impl fabro_agent::Sandbox for RemoteMockEnv { &self, _: &str, _: &std::path::Path, - ) -> std::result::Result<(), String> { - Err("not implemented".to_string()) + ) -> fabro_sandbox::Result<()> { + Err("not implemented".into()) } async fn upload_file_from_local( &self, _: &std::path::Path, _: &str, - ) -> std::result::Result<(), String> { - Err("not implemented".to_string()) + ) -> fabro_sandbox::Result<()> { + Err("not implemented".into()) } fn working_directory(&self) -> &str { @@ -9338,11 +9338,11 @@ impl fabro_agent::Sandbox for CliTestEnv { _path: &str, _offset: Option, _limit: Option, - ) -> Result { + ) -> fabro_sandbox::Result { Ok(String::new()) } - async fn write_file(&self, path: &str, content: &str) -> Result<(), String> { + async fn write_file(&self, path: &str, content: &str) -> fabro_sandbox::Result<()> { self.written_files .lock() .unwrap() @@ -9350,11 +9350,11 @@ impl fabro_agent::Sandbox for CliTestEnv { Ok(()) } - async fn delete_file(&self, _path: &str) -> Result<(), String> { + async fn delete_file(&self, _path: &str) -> fabro_sandbox::Result<()> { Ok(()) } - async fn file_exists(&self, _path: &str) -> Result { + async fn file_exists(&self, _path: &str) -> fabro_sandbox::Result { Ok(false) } @@ -9362,7 +9362,7 @@ impl fabro_agent::Sandbox for CliTestEnv { &self, _path: &str, _depth: Option, - ) -> Result, String> { + ) -> fabro_sandbox::Result> { Ok(vec![]) } @@ -9373,7 +9373,7 @@ impl fabro_agent::Sandbox for CliTestEnv { _working_dir: Option<&str>, _env_vars: Option<&std::collections::HashMap>, _cancel_token: Option, - ) -> Result { + ) -> fabro_sandbox::Result { self.commands.lock().unwrap().push(command.to_string()); // git diff calls: first pair returns empty (before), second pair returns @@ -9467,28 +9467,40 @@ impl fabro_agent::Sandbox for CliTestEnv { _pattern: &str, _path: &str, _options: &fabro_agent::GrepOptions, - ) -> Result, String> { + ) -> fabro_sandbox::Result> { Ok(vec![]) } - async fn glob(&self, _pattern: &str, _path: Option<&str>) -> Result, String> { + async fn glob( + &self, + _pattern: &str, + _path: Option<&str>, + ) -> fabro_sandbox::Result> { Ok(vec![]) } - async fn initialize(&self) -> Result<(), String> { + async fn initialize(&self) -> fabro_sandbox::Result<()> { Ok(()) } - async fn cleanup(&self) -> Result<(), String> { + async fn cleanup(&self) -> fabro_sandbox::Result<()> { Ok(()) } - async fn download_file_to_local(&self, _: &str, _: &std::path::Path) -> Result<(), String> { - Err("not implemented".to_string()) + async fn download_file_to_local( + &self, + _: &str, + _: &std::path::Path, + ) -> fabro_sandbox::Result<()> { + Err("not implemented".into()) } - async fn upload_file_from_local(&self, _: &std::path::Path, _: &str) -> Result<(), String> { - Err("not implemented".to_string()) + async fn upload_file_from_local( + &self, + _: &std::path::Path, + _: &str, + ) -> fabro_sandbox::Result<()> { + Err("not implemented".into()) } fn working_directory(&self) -> &str { @@ -9664,23 +9676,23 @@ async fn cli_backend_run_fails_on_nonzero_exit() { _: &str, _: Option, _: Option, - ) -> Result { + ) -> fabro_sandbox::Result { Ok(String::new()) } - async fn write_file(&self, _: &str, _: &str) -> Result<(), String> { + async fn write_file(&self, _: &str, _: &str) -> fabro_sandbox::Result<()> { Ok(()) } - async fn delete_file(&self, _: &str) -> Result<(), String> { + async fn delete_file(&self, _: &str) -> fabro_sandbox::Result<()> { Ok(()) } - async fn file_exists(&self, _: &str) -> Result { + async fn file_exists(&self, _: &str) -> fabro_sandbox::Result { Ok(false) } async fn list_directory( &self, _: &str, _: Option, - ) -> Result, String> { + ) -> fabro_sandbox::Result> { Ok(vec![]) } async fn exec_command( @@ -9690,7 +9702,7 @@ async fn cli_backend_run_fails_on_nonzero_exit() { _: Option<&str>, _: Option<&std::collections::HashMap>, _: Option, - ) -> Result { + ) -> fabro_sandbox::Result { if command.starts_with("git") { return Ok(fabro_agent::ExecResult { stdout: String::new(), @@ -9743,23 +9755,31 @@ async fn cli_backend_run_fails_on_nonzero_exit() { _: &str, _: &str, _: &fabro_agent::GrepOptions, - ) -> Result, String> { + ) -> fabro_sandbox::Result> { Ok(vec![]) } - async fn glob(&self, _: &str, _: Option<&str>) -> Result, String> { + async fn glob(&self, _: &str, _: Option<&str>) -> fabro_sandbox::Result> { Ok(vec![]) } - async fn initialize(&self) -> Result<(), String> { + async fn initialize(&self) -> fabro_sandbox::Result<()> { Ok(()) } - async fn cleanup(&self) -> Result<(), String> { + async fn cleanup(&self) -> fabro_sandbox::Result<()> { Ok(()) } - async fn download_file_to_local(&self, _: &str, _: &std::path::Path) -> Result<(), String> { - Err("not implemented".to_string()) + async fn download_file_to_local( + &self, + _: &str, + _: &std::path::Path, + ) -> fabro_sandbox::Result<()> { + Err("not implemented".into()) } - async fn upload_file_from_local(&self, _: &std::path::Path, _: &str) -> Result<(), String> { - Err("not implemented".to_string()) + async fn upload_file_from_local( + &self, + _: &std::path::Path, + _: &str, + ) -> fabro_sandbox::Result<()> { + Err("not implemented".into()) } fn working_directory(&self) -> &str { "/tmp"