diff --git a/lib/apps/fabro-server/src/run_files.rs b/lib/apps/fabro-server/src/run_files.rs index a1efb138f..4a9f645ca 100644 --- a/lib/apps/fabro-server/src/run_files.rs +++ b/lib/apps/fabro-server/src/run_files.rs @@ -36,7 +36,7 @@ use fabro_api::types::{ RunFilesMetaToSha, }; use fabro_sandbox::reconnect::reconnect_for_run; -use fabro_sandbox::shell_quote; +use fabro_sandbox::{Termination, shell_quote}; use fabro_types::RunId; use fabro_workflow::sandbox_git::{ DiffError, DiffNumstat, RawDiffEntry, SubmoduleChange, SymlinkChange, list_changed_files_raw, @@ -772,13 +772,13 @@ async fn sandbox_git_stdout( .exec_command(command, SANDBOX_GIT_TIMEOUT_MS, None, None, None) .await .map_err(|err| ApiError::new(StatusCode::SERVICE_UNAVAILABLE, err.display_with_causes()))?; - if res.is_timed_out() { + if res.termination == Termination::TimedOut { return Err(transient_503(op, "command timed out")); } - if !res.is_success() { - return Err(transient_503(op, res.stderr.trim())); + if !res.success() { + return Err(transient_503(op, res.stderr_lossy().trim())); } - Ok(res.stdout) + Ok(res.stdout_lossy()) } /// Build the degraded response from the stored terminal diff patch. @@ -1240,13 +1240,13 @@ async fn resolve_ref_sha_and_time( ) .await .map_err(|err| ApiError::new(StatusCode::SERVICE_UNAVAILABLE, err.display_with_causes()))?; - if !res.is_success() { + if !res.success() { return Err(ApiError::new( StatusCode::SERVICE_UNAVAILABLE, "Failed to resolve sandbox git ref.", )); } - parse_head_show_output(&res.stdout).ok_or_else(|| { + parse_head_show_output(&res.stdout_lossy()).ok_or_else(|| { ApiError::new( StatusCode::SERVICE_UNAVAILABLE, "Sandbox HEAD resolved to an empty value.", @@ -1704,7 +1704,9 @@ fn count_flags(data: &[FileDiff]) -> (u64, u64, u64, u64) { mod tests { use std::sync::atomic::{AtomicUsize, Ordering}; - use fabro_types::{CommandTermination, RunId, test_support}; + use fabro_sandbox::Termination; + use fabro_sandbox::test_support::exec_result; + use fabro_types::{RunId, test_support}; use tokio::time::{Duration, sleep}; use super::*; @@ -1763,13 +1765,7 @@ diff --git a/src/live.rs b/src/live.rs } else { return None; }; - Some(fabro_sandbox::ExecResult { - stdout, - stderr: String::new(), - exit_code: Some(0), - termination: CommandTermination::Exited, - duration_ms: 0, - }) + Some(exec_result(&stdout, "", Some(0), Termination::Exited, 0)) }); let body = materialize_working_tree_sandbox_path( @@ -2874,23 +2870,11 @@ rename to .env.production } fn ok_exec(stdout: &str) -> ExecResult { - ExecResult { - stdout: stdout.to_string(), - stderr: String::new(), - exit_code: Some(0), - termination: CommandTermination::Exited, - duration_ms: 0, - } + exec_result(stdout, "", Some(0), Termination::Exited, 0) } fn fail_exec(stderr: &str) -> ExecResult { - ExecResult { - stdout: String::new(), - stderr: stderr.to_string(), - exit_code: Some(1), - termination: CommandTermination::Exited, - duration_ms: 0, - } + exec_result("", stderr, Some(1), Termination::Exited, 0) } #[tokio::test] diff --git a/lib/apps/fabro-server/src/server/handler/sandbox.rs b/lib/apps/fabro-server/src/server/handler/sandbox.rs index 7cd27fa31..bedf79e1d 100644 --- a/lib/apps/fabro-server/src/server/handler/sandbox.rs +++ b/lib/apps/fabro-server/src/server/handler/sandbox.rs @@ -602,7 +602,7 @@ async fn list_sandbox_services( return ApiError::new(StatusCode::CONFLICT, err.display_with_causes()).into_response(); } }; - if !result.is_success() { + if !result.success() { return ApiError::new( StatusCode::CONFLICT, sandbox_service_command_failure_detail(&result), @@ -610,7 +610,7 @@ async fn list_sandbox_services( .into_response(); } - let discovery = parse_sandbox_services(&result.stdout, &provider); + let discovery = parse_sandbox_services(&result.stdout_lossy(), &provider); Json(SandboxServiceListResponse { data: discovery.services, meta: SandboxServiceListMeta { @@ -621,11 +621,13 @@ async fn list_sandbox_services( } fn sandbox_service_command_failure_detail(result: &fabro_sandbox::ExecResult) -> String { - let stderr = result.stderr.trim(); + let stderr = result.stderr_lossy(); + let stderr = stderr.trim(); if !stderr.is_empty() { return stderr.to_string(); } - let stdout = result.stdout.trim(); + let stdout = result.stdout_lossy(); + let stdout = stdout.trim(); if !stdout.is_empty() { return stdout.to_string(); } @@ -926,6 +928,8 @@ async fn load_run_sandbox_instance( #[cfg(test)] mod tests { use axum::http::{HeaderMap, HeaderValue}; + use fabro_sandbox::Termination; + use fabro_sandbox::test_support::exec_result; use futures_util::FutureExt; use super::*; @@ -1146,13 +1150,13 @@ FABRO_PROC_NET_TCP /proc/net/tcp6 #[test] fn sandbox_service_command_failure_prefers_stderr_then_stdout() { - let mut result = fabro_sandbox::ExecResult { - stdout: "stdout detail".to_string(), - stderr: "stderr detail".to_string(), - exit_code: Some(127), - termination: fabro_types::CommandTermination::Exited, - duration_ms: 10, - }; + let mut result = exec_result( + "stdout detail", + "stderr detail", + Some(127), + Termination::Exited, + 10, + ); assert_eq!( sandbox_service_command_failure_detail(&result), "stderr detail" diff --git a/lib/components/fabro-acp/src/transport.rs b/lib/components/fabro-acp/src/transport.rs index a479bacde..39636fb90 100644 --- a/lib/components/fabro-acp/src/transport.rs +++ b/lib/components/fabro-acp/src/transport.rs @@ -10,9 +10,9 @@ use agent_client_protocol::{ }; use fabro_sandbox::{ DEFAULT_EXEC_OUTPUT_TAIL_BYTES, Error as SandboxError, Result as SandboxResult, RunSandbox, - StderrCollector, StdioProcessHandle, StdioProcessTermination, + StderrTail, StdioProcessHandle, Termination, command_termination, program_exit_code, }; -use fabro_types::{CommandTermination, ExecOutputTail}; +use fabro_types::ExecOutputTail; use futures::io::BufReader; use futures::sink::unfold; use futures::{AsyncBufReadExt, AsyncWriteExt, Stream}; @@ -27,8 +27,8 @@ const CLEAN_EXIT_PROTOCOL_GRACE: Duration = Duration::from_millis(500); #[derive(Clone)] pub(crate) struct TransportState { - handle: Arc>>, - stderr: Arc>>, + handle: Arc>>>, + stderr: Arc>>, startup_error: Arc>>, process_exit: Arc>>, } @@ -43,7 +43,7 @@ impl TransportState { } } - async fn set_process(&self, handle: StdioProcessHandle, stderr: StderrCollector) { + async fn set_process(&self, handle: Arc, stderr: StderrTail) { *self.handle.lock().await = Some(handle); *self.stderr.lock().await = Some(stderr); } @@ -52,10 +52,15 @@ impl TransportState { *self.startup_error.lock().await = Some(error); } - async fn set_process_exit(&self, termination: StdioProcessTermination, stderr: &str) { + async fn set_process_exit( + &self, + termination: Termination, + exit_code: Option, + stderr: &str, + ) { *self.process_exit.lock().await = Some(AcpProcessExit { - termination: termination.termination, - exit_code: termination.exit_code, + termination: command_termination(termination), + exit_code: program_exit_code(termination, exit_code), exec_output_tail: redacted_stderr_tail(stderr), }); } @@ -70,14 +75,14 @@ impl TransportState { pub(crate) async fn terminate(&self) -> SandboxResult<()> { if let Some(handle) = self.handle.lock().await.as_ref().cloned() { - handle.terminate().await?; + handle.terminate().await; } Ok(()) } pub(crate) async fn stderr_tail(&self) -> String { if let Some(stderr) = self.stderr.lock().await.as_ref().cloned() { - return stderr.tail_string().await; + return stderr.to_string_lossy(); } String::new() } @@ -125,7 +130,6 @@ impl ConnectTo for SandboxAcpTransport { &self.command.to_shell_command(), Some(&self.cwd), Some(&env), - None, ) .await { @@ -136,9 +140,11 @@ impl ConnectTo for SandboxAcpTransport { } }; - let handle = process.handle.clone(); - let stderr = process.stderr.clone(); - self.state.set_process(handle.clone(), stderr.clone()).await; + let handle: Arc = Arc::from(process.handle); + let stderr = process.stderr_tail.clone(); + self.state + .set_process(Arc::clone(&handle), stderr.clone()) + .await; let incoming_lines = Box::pin(BufReader::new(process.stdout.compat()).lines()) as Pin> + Send>>; @@ -158,25 +164,20 @@ impl ConnectTo for SandboxAcpTransport { )); tokio::select! { result = &mut protocol => { - if let Err(err) = handle.terminate().await { - tracing::warn!(error = %err, "Failed to terminate ACP process after protocol completion"); - } + handle.terminate().await; let _ = timeout(Duration::from_millis(500), handle.wait()).await; result } - termination = handle.wait() => { - let termination = termination.map_err(ProtocolError::into_internal_error)?; - let stderr = stderr.tail_string().await; - if termination.termination == CommandTermination::Exited - && termination.exit_code == Some(0) - { + (termination, exit_code) = handle.wait() => { + let stderr = stderr.to_string_lossy(); + if termination == Termination::Exited && exit_code == Some(0) { // Stdio agents commonly exit immediately after writing their final response. // Process wait can observe that exit before the line reader drains stdout. if let Ok(result) = timeout(CLEAN_EXIT_PROTOCOL_GRACE, &mut protocol).await { return result; } } - self.state.set_process_exit(termination, &stderr).await; + self.state.set_process_exit(termination, exit_code, &stderr).await; Err(process_exited_before_protocol_completed()) } } diff --git a/lib/components/fabro-agent/src/event.rs b/lib/components/fabro-agent/src/event.rs index e16f71e50..422e53d4d 100644 --- a/lib/components/fabro-agent/src/event.rs +++ b/lib/components/fabro-agent/src/event.rs @@ -3,8 +3,8 @@ use std::time::SystemTime; use tokio::sync::broadcast; -use crate::sandbox::OutputCaptureStats; use crate::tool_registry::AgentEventEmitter; +use crate::truncation::OutputCaptureStats; use crate::types::{AgentEvent, SessionEvent}; #[derive(Clone)] diff --git a/lib/components/fabro-agent/src/lib.rs b/lib/components/fabro-agent/src/lib.rs index f0c775254..f3ea13a7b 100644 --- a/lib/components/fabro-agent/src/lib.rs +++ b/lib/components/fabro-agent/src/lib.rs @@ -55,10 +55,11 @@ pub use question_tools::{ OPENAI_REQUEST_USER_INPUT_TOOL, register_question_tools, }; pub use sandbox::{ - CommandOutputCallback, DirEntry, ExecResult, ExecStreamingRequest, ExecStreamingResult, - FileKind, GrepMatch, GrepOptions, OutputCaptureStats, RefreshOutcome, RemoteCredentialAction, - RunSandbox, SandboxFile, StderrCollector, StdioProcess, StdioProcessHandle, TokenProvenance, - TokenSnapshot, WalkOptions, format_lines_numbered, shell_quote, + CaptureStats, DirEntry, ExecControls, ExecResult, ExecResultExt, ExecSpec, ExecStreamingResult, + FileKind, GrepMatch, GrepOptions, OutputSink, OutputStream, RefreshOutcome, + RemoteCredentialAction, RunSandbox, SandboxFile, StderrTail, StdioProcess, StdioProcessHandle, + Termination, TokenProvenance, TokenSnapshot, WalkOptions, command_termination, + format_lines_numbered, program_exit_code, shell_quote, }; pub use session::{ CompletionCoordinator, Session, SessionControlHandle, SessionInputTiming, @@ -77,7 +78,9 @@ pub use tools::{ WebFetchSummarizer, make_edit_file_tool, make_glob_tool, make_grep_tool, make_read_file_tool, make_shell_tool, make_shell_tool_with_options, make_write_file_tool, register_core_tools, }; -pub use truncation::{TruncationMode, truncate_lines, truncate_output, truncate_tool_output}; +pub use truncation::{ + OutputCaptureStats, TruncationMode, truncate_lines, truncate_output, truncate_tool_output, +}; pub use types::{ AgentEvent, McpToolSummary, MemoryFileSummary, Message, SessionEvent, SessionState, SkillActivationSource, SkillSummary, diff --git a/lib/components/fabro-agent/src/profiles/kimi_tools.rs b/lib/components/fabro-agent/src/profiles/kimi_tools.rs index 937b3453e..119a92453 100644 --- a/lib/components/fabro-agent/src/profiles/kimi_tools.rs +++ b/lib/components/fabro-agent/src/profiles/kimi_tools.rs @@ -27,7 +27,7 @@ use serde_json::Value; use strum::EnumString; use crate::native_tool::NativeTool; -use crate::sandbox::{GrepOptions, format_lines_numbered}; +use crate::sandbox::{ExecResultExt, GrepOptions, Termination, format_lines_numbered}; use crate::tool_registry::{RegisteredTool, ToolSource}; use crate::tools::{ DEFAULT_READ_LINES, emit_shell_process_completed, execute_grep, execute_shell_command, @@ -120,25 +120,27 @@ explicitly asked. Never run commands requiring superuser privileges unless expli let result = &streaming.result; let mut out = String::new(); - if result.is_timed_out() { - out.push_str("Command timed out.\n"); - } else if result.is_cancelled() { - out.push_str("Command cancelled.\n"); + match result.termination { + Termination::TimedOut => out.push_str("Command timed out.\n"), + Termination::Cancelled | Termination::Killed => { + out.push_str("Command cancelled.\n"); + } + _ => {} } - out.push_str(&result.stdout); + out.push_str(&result.stdout_lossy()); if !result.stderr.is_empty() { if !out.is_empty() { out.push('\n'); } - out.push_str(&result.stderr); + out.push_str(&result.stderr_lossy()); } - if let Some(code) = result.exit_code.filter(|c| *c != 0) { + if let Some(code) = result.program_exit_code().filter(|c| *c != 0) { if !out.is_empty() { out.push('\n'); } let _ = write!(out, "Command failed with exit code: {code}"); } - let is_success = result.is_success(); + let is_success = result.success(); let out = retain_shell_output(&ctx, &streaming, out); emit_shell_process_completed(&ctx, streaming).await; if is_success { Ok(out) } else { Err(out) } @@ -367,11 +369,13 @@ pub fn make_kimi_edit_tool(description: &str) -> RegisteredTool { mod tests { use std::collections::HashMap; + use fabro_sandbox::Termination; + use fabro_sandbox::test_support::exec_result; use serde_json::json; use tokio_util::sync::CancellationToken; use super::*; - use crate::sandbox::{ExecResult, RunSandbox}; + use crate::sandbox::RunSandbox; use crate::test_support::MockSandbox; use crate::tool_registry::{ToolContext, ToolDefinitionExt}; @@ -684,17 +688,9 @@ mod tests { #[tokio::test] async fn bash_reuses_session_env_cwd_and_timeout_rendering() { - use fabro_types::CommandTermination; - let tool = make_kimi_bash_tool(60_000, 600_000); let env = MockSandbox { - exec_result: ExecResult { - stdout: String::new(), - stderr: String::new(), - exit_code: None, - termination: CommandTermination::TimedOut, - duration_ms: 7_000, - }, + exec_result: exec_result("", "", None, Termination::TimedOut, 7_000), ..MockSandbox::default() }; let mut tool_ctx = ctx(env.sandbox()); diff --git a/lib/components/fabro-agent/src/sandbox.rs b/lib/components/fabro-agent/src/sandbox.rs index 2f99a5e5f..9706536e1 100644 --- a/lib/components/fabro-agent/src/sandbox.rs +++ b/lib/components/fabro-agent/src/sandbox.rs @@ -1,8 +1,8 @@ // Re-export the sandbox types the agent works with from fabro-sandbox. pub use fabro_sandbox::{ - CommandOutputCallback, DirEntry, ExecResult, ExecStreamingRequest, ExecStreamingResult, - FileKind, GrepMatch, GrepOptions, OutputCaptureStats, RefreshOutcome, RemoteCredentialAction, - RunSandbox, SandboxFile, StderrCollector, StdioProcess, StdioProcessHandle, - StdioProcessTermination, TokenProvenance, TokenSnapshot, WalkOptions, format_lines_numbered, - shell_quote, + CaptureStats, DirEntry, ExecControls, ExecResult, ExecResultExt, ExecSpec, ExecStreamingResult, + FileKind, GrepMatch, GrepOptions, OutputSink, OutputStream, RefreshOutcome, + RemoteCredentialAction, RunSandbox, SandboxFile, StderrTail, StdioProcess, StdioProcessHandle, + Termination, TokenProvenance, TokenSnapshot, WalkOptions, command_termination, + format_lines_numbered, program_exit_code, shell_quote, }; diff --git a/lib/components/fabro-agent/src/session.rs b/lib/components/fabro-agent/src/session.rs index 3d42bd697..152de768a 100644 --- a/lib/components/fabro-agent/src/session.rs +++ b/lib/components/fabro-agent/src/session.rs @@ -897,7 +897,7 @@ impl Session { } }; - let pid = launch_result.stdout.trim().to_string(); + let pid = launch_result.stdout_lossy().trim().to_string(); info!(pid = %pid, port, "MCP server process launched in sandbox"); // Wait for the server to start listening on the port @@ -929,7 +929,7 @@ impl Session { } }; - if poll_result.stdout.trim() != "ready" { + if poll_result.stdout_lossy().trim() != "ready" { // Grab stderr for debugging let stderr = sandbox .exec_command( @@ -940,7 +940,7 @@ impl Session { Some(cancel_token.child_token()), ) .await - .map(|r| r.stdout) + .map(|r| r.stdout_lossy()) .unwrap_or_default(); return Ok(Err(format!( "MCP server did not start listening on port {port} within 30s. stderr:\n{stderr}" @@ -993,8 +993,8 @@ impl Session { ) .await .ok() - .filter(fabro_sandbox::ExecResult::is_success) - .map(|r| r.stdout.trim().to_string()); + .filter(fabro_sandbox::ExecResult::success) + .map(|r| r.stdout_lossy().trim().to_string()); if cancel_token.is_cancelled() { return Err(Error::Interrupted(InterruptReason::Cancelled)); @@ -1013,8 +1013,8 @@ impl Session { ) .await .ok() - .filter(fabro_sandbox::ExecResult::is_success) - .map(|r| r.stdout.trim().to_string()) + .filter(fabro_sandbox::ExecResult::success) + .map(|r| r.stdout_lossy().trim().to_string()) .filter(|s| !s.is_empty()) } else { None @@ -1035,8 +1035,8 @@ impl Session { ) .await .ok() - .filter(fabro_sandbox::ExecResult::is_success) - .map(|r| r.stdout.trim().to_string()) + .filter(fabro_sandbox::ExecResult::success) + .map(|r| r.stdout_lossy().trim().to_string()) .filter(|s| !s.is_empty()) } else { None diff --git a/lib/components/fabro-agent/src/tool_execution.rs b/lib/components/fabro-agent/src/tool_execution.rs index 80110986c..9ea5d5a98 100644 --- a/lib/components/fabro-agent/src/tool_execution.rs +++ b/lib/components/fabro-agent/src/tool_execution.rs @@ -10,11 +10,11 @@ use tracing::debug; use crate::config::{SessionOptions, ToolHookCallback, ToolHookDecision}; use crate::event::{Emitter, SessionBoundEmitter}; use crate::question_tools::{self, AgentToolRuntime, is_question_tool}; -use crate::sandbox::{OutputCaptureStats, RunSandbox}; +use crate::sandbox::RunSandbox; use crate::session::ToolEnvProvider; use crate::tool_registry::{AgentEventEmitter, RegisteredTool, ToolContext, ToolRegistry}; use crate::truncation::{ - MAX_RETAINED_TOOL_OUTPUT_BYTES, preview_tool_output, serialized_json_bytes, + MAX_RETAINED_TOOL_OUTPUT_BYTES, OutputCaptureStats, preview_tool_output, serialized_json_bytes, truncate_tool_output, }; use crate::types::AgentEvent; @@ -672,6 +672,8 @@ mod tests { use std::sync::{Arc, Mutex}; use async_trait::async_trait; + use fabro_sandbox::Termination; + use fabro_sandbox::test_support::exec_result; use fabro_types::run_event::{AgentToolCompletedProps, MAX_RUN_EVENT_BODY_BYTES}; use fabro_types::{AgentProfileKind, tool_result_to_json}; use lithos_llm::types::{ToolCall, ToolDefinition}; @@ -1373,23 +1375,11 @@ mod tests { } fn exited(exit_code: i32) -> fabro_sandbox::ExecResult { - fabro_sandbox::ExecResult { - stdout: "out".into(), - stderr: "err".into(), - exit_code: Some(exit_code), - termination: fabro_types::CommandTermination::Exited, - duration_ms: 12, - } + exec_result("out", "err", Some(exit_code), Termination::Exited, 12) } fn cancelled() -> fabro_sandbox::ExecResult { - fabro_sandbox::ExecResult { - stdout: "out".into(), - stderr: String::new(), - exit_code: None, - termination: fabro_types::CommandTermination::Cancelled, - duration_ms: 12, - } + exec_result("out", "", None, Termination::Cancelled, 12) } async fn run_shell_tool( @@ -1458,13 +1448,13 @@ mod tests { let emitter = Emitter::new(); let mut receiver = emitter.subscribe(); let result = run_shell_tool( - fabro_sandbox::ExecResult { - stdout: "x".repeat(output_len), - stderr: String::new(), - exit_code: Some(0), - termination: fabro_types::CommandTermination::Exited, - duration_ms: 12, - }, + exec_result( + &("x".repeat(output_len)), + "", + Some(0), + Termination::Exited, + 12, + ), None, &emitter, ) diff --git a/lib/components/fabro-agent/src/tool_registry.rs b/lib/components/fabro-agent/src/tool_registry.rs index f2c9e74d4..d88aba7bb 100644 --- a/lib/components/fabro-agent/src/tool_registry.rs +++ b/lib/components/fabro-agent/src/tool_registry.rs @@ -9,9 +9,10 @@ use tokio_util::sync::CancellationToken; use crate::config::{ToolAccessPolicy, ToolExposureMode}; use crate::native_tool::{NativeTool, ToolVocabulary}; -use crate::sandbox::{OutputCaptureStats, RunSandbox}; +use crate::sandbox::RunSandbox; use crate::session::ToolEnvProvider; use crate::tool_permissions; +use crate::truncation::OutputCaptureStats; use crate::types::AgentEvent; /// Narrow handle a tool uses to publish typed agent events (e.g. todo diff --git a/lib/components/fabro-agent/src/tools.rs b/lib/components/fabro-agent/src/tools.rs index 89162e660..d5dd47466 100644 --- a/lib/components/fabro-agent/src/tools.rs +++ b/lib/components/fabro-agent/src/tools.rs @@ -11,9 +11,12 @@ use lithos_llm::types::ToolDefinition; use tokio::task; use crate::config::NativeToolOptions; -use crate::sandbox::{ExecStreamingResult, FileKind, GrepOptions}; +use crate::sandbox::{ + ExecControls, ExecResultExt, ExecSpec, ExecStreamingResult, FileKind, GrepOptions, + command_termination, +}; use crate::tool_registry::{RegisteredTool, ToolContext, ToolRegistry, ToolSource}; -use crate::truncation::{MAX_RETAINED_TOOL_OUTPUT_BYTES, retain_tool_output}; +use crate::truncation::{MAX_RETAINED_TOOL_OUTPUT_BYTES, OutputCaptureStats, retain_tool_output}; use crate::types::AgentEvent; use crate::web_search::{SearchBackend, make_web_search_tool}; @@ -297,15 +300,20 @@ pub(crate) async fn execute_shell_command( env_var_count = tool_env.as_ref().map_or(0, std::collections::HashMap::len), "Injecting sandbox env vars into tool execution" ); + let mut spec = ExecSpec::bash(command).timeout(std::time::Duration::from_millis(timeout_ms)); + if let Some(cwd) = cwd { + spec = spec.working_dir(cwd); + } + for (key, value) in tool_env.iter().flatten() { + spec = spec.env_var(key, value); + } + let controls = ExecControls { + term: Some(ctx.cancel.clone()), + retained_output_limit: Some(MAX_RETAINED_TOOL_OUTPUT_BYTES), + ..ExecControls::default() + }; ctx.env - .exec_command_streaming(crate::ExecStreamingRequest { - timeout_ms: Some(timeout_ms), - working_dir: cwd, - env_vars: tool_env.as_ref(), - cancel_token: Some(ctx.cancel.clone()), - stream_output_bytes_cap: Some(MAX_RETAINED_TOOL_OUTPUT_BYTES), - ..crate::ExecStreamingRequest::new(command) - }) + .exec_command_streaming(spec, controls) .await .map_err(|e| format!("{SHELL_NO_PROCESS_RESULT}: {}", e.display_with_causes())) } @@ -320,7 +328,7 @@ pub(crate) async fn run_shell_command( ) -> Result { let streaming = execute_shell_command(ctx, command, timeout_ms, cwd).await?; let text = retain_shell_output(ctx, &streaming, render_shell_result(&streaming)); - let is_success = streaming.result.is_success(); + let is_success = streaming.result.success(); emit_shell_process_completed(ctx, streaming).await; if is_success { Ok(text) } else { Err(text) } @@ -336,7 +344,7 @@ pub(crate) fn retain_shell_output( let retained = retain_tool_output( output, MAX_RETAINED_TOOL_OUTPUT_BYTES, - streaming.output_capture().omitted_bytes, + OutputCaptureStats::from_streaming(streaming).omitted_bytes, ); ctx.record_tool_output_stats(retained.stats); retained.output @@ -353,11 +361,11 @@ pub(crate) async fn emit_shell_process_completed( return; } - let exit_code = streaming.result.exit_code; - let termination = streaming.result.termination; - let duration_ms = streaming.result.duration_ms; + let exit_code = streaming.result.program_exit_code(); + let termination = command_termination(streaming.result.termination); + let duration_ms = streaming.result.duration_ms(); let streams_separated = streaming.streams_separated; - let output_stats = streaming.output_capture(); + let output_stats = OutputCaptureStats::from_streaming(&streaming); let result = streaming.result; let exec_output_tail = match task::spawn_blocking(move || result.default_redacted_output_tail()).await { @@ -389,21 +397,23 @@ fn render_shell_result(streaming: &ExecStreamingResult) -> String { let result = &streaming.result; let mut output = format!( "Termination: {}\nExit code: {}\nDuration: {}ms\n", - result.termination.as_str(), + command_termination(result.termination).as_str(), result - .exit_code + .program_exit_code() .map_or_else(|| "none".to_string(), |code| code.to_string()), - result.duration_ms, + result.duration_ms(), ); + let stdout = result.stdout_lossy(); + let stderr = result.stderr_lossy(); if streaming.streams_separated { - if !result.stdout.is_empty() { - let _ = write!(output, "stdout:\n{}\n", result.stdout); + if !stdout.is_empty() { + let _ = write!(output, "stdout:\n{stdout}\n"); } - if !result.stderr.is_empty() { - let _ = write!(output, "stderr:\n{}\n", result.stderr); + if !stderr.is_empty() { + let _ = write!(output, "stderr:\n{stderr}\n"); } - } else if !result.stdout.is_empty() { - let _ = write!(output, "output (combined):\n{}\n", result.stdout); + } else if !stdout.is_empty() { + let _ = write!(output, "output (combined):\n{stdout}\n"); } output } @@ -686,15 +696,15 @@ pub(crate) fn make_web_fetch_tool(summarizer: Option) -> Reg .await .map_err(|e| e.display_with_causes())?; - if !result.is_success() { + if !result.success() { return Err(format!( "curl failed (exit code {}): {}", - result.display_exit_code(), - result.stderr.trim() + result.program_exit_code().unwrap_or(-1), + result.stderr_lossy().trim() )); } - let mut content = html_to_markdown(&result.stdout); + let mut content = html_to_markdown(&result.stdout_lossy()); if content.len() > MAX_WEB_FETCH_BYTES { content.truncate(MAX_WEB_FETCH_BYTES); content.push_str("\n\n[Output truncated at 100KB]"); @@ -737,6 +747,8 @@ mod tests { use std::collections::HashMap; use fabro_llm::adapter::ProviderAdapter; + use fabro_sandbox::Termination; + use fabro_sandbox::test_support::exec_result; use fabro_types::CommandTermination; use lithos_llm::catalog::{ModelId, builtin}; use tokio::sync::broadcast; @@ -1144,13 +1156,13 @@ mod tests { #[tokio::test] async fn shell_success_returns_ok_with_metadata_and_separate_streams() { let tool = make_shell_tool(); - let env = mock_sandbox_with(ExecResult { - stdout: "hello".into(), - stderr: "a warning".into(), - exit_code: Some(0), - termination: CommandTermination::Exited, - duration_ms: 10, - }) + let env = mock_sandbox_with(exec_result( + "hello", + "a warning", + Some(0), + Termination::Exited, + 10, + )) .sandbox(); let output = (tool.executor)( serde_json::json!({"command": "echo hello"}), @@ -1169,13 +1181,7 @@ mod tests { #[tokio::test] async fn shell_forwards_command_without_stream_redirection_wrapper() { let tool = make_shell_tool(); - let env = mock_sandbox_with(ExecResult { - stdout: String::new(), - stderr: String::new(), - exit_code: Some(0), - termination: CommandTermination::Exited, - duration_ms: 1, - }); + let env = mock_sandbox_with(exec_result("", "", Some(0), Termination::Exited, 1)); let _ = (tool.executor)( serde_json::json!({"command": "make test"}), shell_context(env.sandbox()), @@ -1211,13 +1217,7 @@ mod tests { async fn shell_nonzero_exit_code() { let tool = make_shell_tool(); let env = MockSandbox { - exec_result: ExecResult { - stdout: "error".into(), - stderr: String::new(), - exit_code: Some(1), - termination: CommandTermination::Exited, - duration_ms: 10, - }, + exec_result: exec_result("error", "", Some(1), Termination::Exited, 10), ..Default::default() } .sandbox(); @@ -1233,13 +1233,13 @@ mod tests { #[tokio::test] async fn shell_timeout_returns_error_with_partial_output() { let tool = make_shell_tool(); - let env = mock_sandbox_with(ExecResult { - stdout: "partial".into(), - stderr: String::new(), - exit_code: None, - termination: CommandTermination::TimedOut, - duration_ms: 10000, - }) + let env = mock_sandbox_with(exec_result( + "partial", + "", + None, + Termination::TimedOut, + 10000, + )) .sandbox(); let output = (tool.executor)( serde_json::json!({"command": "sleep 100"}), @@ -1256,14 +1256,8 @@ mod tests { #[tokio::test] async fn shell_cancellation_returns_error_with_partial_output() { let tool = make_shell_tool(); - let env = mock_sandbox_with(ExecResult { - stdout: "partial".into(), - stderr: String::new(), - exit_code: None, - termination: CommandTermination::Cancelled, - duration_ms: 42, - }) - .sandbox(); + let env = mock_sandbox_with(exec_result("partial", "", None, Termination::Cancelled, 42)) + .sandbox(); let output = (tool.executor)( serde_json::json!({"command": "sleep 100"}), shell_context(env), @@ -1312,13 +1306,13 @@ mod tests { #[tokio::test] async fn shell_emits_process_event_with_typed_outcome_and_redacted_tails() { let tool = make_shell_tool(); - let env = mock_sandbox_with(ExecResult { - stdout: "out".into(), - stderr: "boom key=AKIAYRWQG5EJLPZLBYNP".into(), - exit_code: Some(7), - termination: CommandTermination::Exited, - duration_ms: 12, - }) + let env = mock_sandbox_with(exec_result( + "out", + "boom key=AKIAYRWQG5EJLPZLBYNP", + Some(7), + Termination::Exited, + 12, + )) .sandbox(); let emitter = Emitter::new(); let mut receiver = emitter.subscribe(); @@ -1360,13 +1354,7 @@ mod tests { async fn shell_renders_combined_output_when_streams_are_not_separated() { let tool = make_shell_tool(); let env = MockSandbox { - exec_result: ExecResult { - stdout: "interleaved".into(), - stderr: String::new(), - exit_code: Some(0), - termination: CommandTermination::Exited, - duration_ms: 5, - }, + exec_result: exec_result("interleaved", "", Some(0), Termination::Exited, 5), streams_separated: false, ..Default::default() } @@ -1402,13 +1390,13 @@ mod tests { .collect::>() .join("\n"); assert!(stdout.len() > 30_000); - let env = mock_sandbox_with(ExecResult { - stdout, - stderr: "the build failed".into(), - exit_code: Some(2), - termination: CommandTermination::Exited, - duration_ms: 900, - }) + let env = mock_sandbox_with(exec_result( + &stdout, + "the build failed", + Some(2), + Termination::Exited, + 900, + )) .sandbox(); let output = (tool.executor)( @@ -1647,13 +1635,7 @@ mod tests { async fn web_fetch_passes_tool_env_to_exec_command() { let tool = make_web_fetch_tool(None); let env = MockSandbox { - exec_result: ExecResult { - stdout: "fetched content".into(), - stderr: String::new(), - exit_code: Some(0), - termination: CommandTermination::Exited, - duration_ms: 100, - }, + exec_result: exec_result("fetched content", "", Some(0), Termination::Exited, 100), ..Default::default() }; let env_clone = env.sandbox(); @@ -1797,13 +1779,13 @@ mod tests { async fn web_fetch_builds_curl_command() { let tool = make_web_fetch_tool(None); let env = MockSandbox { - exec_result: ExecResult { - stdout: "

hello

".into(), - stderr: String::new(), - exit_code: Some(0), - termination: CommandTermination::Exited, - duration_ms: 100, - }, + exec_result: exec_result( + "

hello

", + "", + Some(0), + Termination::Exited, + 100, + ), ..Default::default() }; let env_clone = env.sandbox(); @@ -1925,13 +1907,7 @@ mod tests { let large_content = "x".repeat(150 * 1024); let tool = make_web_fetch_tool(None); let env = MockSandbox { - exec_result: ExecResult { - stdout: large_content, - stderr: String::new(), - exit_code: Some(0), - termination: CommandTermination::Exited, - duration_ms: 100, - }, + exec_result: exec_result(&large_content, "", Some(0), Termination::Exited, 100), ..Default::default() } .sandbox(); @@ -1957,13 +1933,13 @@ mod tests { async fn web_fetch_returns_error_on_nonzero_exit() { let tool = make_web_fetch_tool(None); let env = MockSandbox { - exec_result: ExecResult { - stdout: String::new(), - stderr: "curl: (6) Could not resolve host".into(), - exit_code: Some(6), - termination: CommandTermination::Exited, - duration_ms: 100, - }, + exec_result: exec_result( + "", + "curl: (6) Could not resolve host", + Some(6), + Termination::Exited, + 100, + ), ..Default::default() } .sandbox(); @@ -2006,14 +1982,13 @@ mod tests { let tool = make_web_fetch_tool(Some(summarizer)); let env = MockSandbox { - exec_result: ExecResult { - stdout: "

Lots of content about Rust...

" - .into(), - stderr: String::new(), - exit_code: Some(0), - termination: CommandTermination::Exited, - duration_ms: 100, - }, + exec_result: exec_result( + "

Lots of content about Rust...

", + "", + Some(0), + Termination::Exited, + 100, + ), ..Default::default() } .sandbox(); @@ -2041,15 +2016,13 @@ mod tests { async fn web_fetch_prompt_without_summarizer_returns_content_with_note() { let tool = make_web_fetch_tool(None); let env = MockSandbox { - exec_result: ExecResult { - stdout: - "

Rust is a systems programming language.

" - .into(), - stderr: String::new(), - exit_code: Some(0), - termination: CommandTermination::Exited, - duration_ms: 100, - }, + exec_result: exec_result( + "

Rust is a systems programming language.

", + "", + Some(0), + Termination::Exited, + 100, + ), ..Default::default() } .sandbox(); @@ -2105,13 +2078,13 @@ mod tests { let tool = make_web_fetch_tool(Some(summarizer)); let env = MockSandbox { - exec_result: ExecResult { - stdout: "

Page content

".into(), - stderr: String::new(), - exit_code: Some(0), - termination: CommandTermination::Exited, - duration_ms: 100, - }, + exec_result: exec_result( + "

Page content

", + "", + Some(0), + Termination::Exited, + 100, + ), ..Default::default() } .sandbox(); diff --git a/lib/components/fabro-agent/src/truncation.rs b/lib/components/fabro-agent/src/truncation.rs index f3c0bfc4e..8792ad008 100644 --- a/lib/components/fabro-agent/src/truncation.rs +++ b/lib/components/fabro-agent/src/truncation.rs @@ -5,9 +5,56 @@ use fabro_types::run_event::MAX_RUN_EVENT_BODY_BYTES; use serde::Serialize; use crate::config::SessionOptions; -use crate::sandbox::OutputCaptureStats; +use crate::sandbox::{CaptureStats, ExecStreamingResult}; use crate::tool_permissions::canonical_tool_name; +/// Byte counts for a tool's output: what was observed, what the model sees, +/// and what was dropped in between. Covers every tool, not only commands; +/// a command's counts start from the driver's per-stream [`CaptureStats`]. +#[derive(Debug, Clone, Copy, Default, PartialEq, Eq)] +pub struct OutputCaptureStats { + pub observed_bytes: usize, + pub retained_bytes: usize, + pub omitted_bytes: usize, +} + +impl OutputCaptureStats { + /// Accounting for output that was kept whole. + #[must_use] + pub fn complete(byte_count: usize) -> Self { + Self { + observed_bytes: byte_count, + retained_bytes: byte_count, + omitted_bytes: 0, + } + } + + /// Both streams of a command run, added together. + #[must_use] + pub fn from_streaming(streaming: &ExecStreamingResult) -> Self { + Self::from(streaming.stdout_capture).combine(Self::from(streaming.stderr_capture)) + } + + #[must_use] + pub fn combine(self, other: Self) -> Self { + Self { + observed_bytes: self.observed_bytes.saturating_add(other.observed_bytes), + retained_bytes: self.retained_bytes.saturating_add(other.retained_bytes), + omitted_bytes: self.omitted_bytes.saturating_add(other.omitted_bytes), + } + } +} + +impl From for OutputCaptureStats { + fn from(stats: CaptureStats) -> Self { + Self { + observed_bytes: stats.observed_bytes, + retained_bytes: stats.retained_bytes, + omitted_bytes: stats.omitted_bytes, + } + } +} + pub(crate) const MAX_RETAINED_TOOL_OUTPUT_BYTES: usize = 1024 * 1024; /// Reserve half the run-event body limit for serialized tool output; the /// other half is headroom for the rest of the event envelope. diff --git a/lib/components/fabro-hooks/src/executor.rs b/lib/components/fabro-hooks/src/executor.rs index c0dae65e6..11b0ca60d 100644 --- a/lib/components/fabro-hooks/src/executor.rs +++ b/lib/components/fabro-hooks/src/executor.rs @@ -4,8 +4,8 @@ use std::sync::{Arc, LazyLock}; use std::time::Instant; use async_trait::async_trait; -use fabro_agent::RunSandbox; use fabro_agent::tool_registry::ToolContext; +use fabro_agent::{ExecResultExt, RunSandbox}; use fabro_llm::credentials::CredentialProvider; use fabro_llm::lithos_catalog::Catalog; use fabro_llm::{Client, ClientOptions, Request}; @@ -174,7 +174,10 @@ impl HookExecutorImpl { ) .await { - Ok(result) => Self::parse_decision(result.exit_code.unwrap_or(-1), &result.stdout), + Ok(result) => Self::parse_decision( + result.program_exit_code().unwrap_or(-1), + &result.stdout_lossy(), + ), Err(e) => HookDecision::Block { reason: Some(format!("sandbox exec failed: {e}")), }, diff --git a/lib/components/fabro-sandbox/src/clone.rs b/lib/components/fabro-sandbox/src/clone.rs index 2342361dd..4ec8d2e00 100644 --- a/lib/components/fabro-sandbox/src/clone.rs +++ b/lib/components/fabro-sandbox/src/clone.rs @@ -15,12 +15,13 @@ use std::time::Duration; use fabro_github::token_source::ResolvedToken; use fabro_redact::DisplaySafeUrl; use fabro_types::SandboxProviderKind; -use sandbox_driver::{Git as _, GitCloneOptions, GitCredentials, Sandbox as DriverHandle}; +use sandbox_driver::{ + ExecResult, Git as _, GitCloneOptions, GitCredentials, Sandbox as DriverHandle, +}; use tokio::time; -use crate::ExecResult; use crate::clone_source::{self, GitHubRepoLayout, PinnedRevision}; -use crate::exec::SandboxExec; +use crate::exec::{ExecResultExt, SandboxExec}; use crate::git_retry::{self, CredentialContext, GitRetryReason, RetryPlan}; use crate::push_credentials::PushCredentialState; use crate::redact::redact_auth_url; @@ -169,7 +170,7 @@ pub(crate) async fn clone_github_repo( has_app, ) .await?; - pin.verify_head(&head.stdout)?; + pin.verify_head(&head.stdout_lossy())?; } run_local_step( @@ -192,7 +193,7 @@ async fn verify_git_available(exec: &SandboxExec<'_>) -> crate::Result<()> { let result = exec .run("git --version", Some(STEP_TIMEOUT), Some("/"), None, None) .await?; - if !result.is_success() { + if !result.success() { return Err(crate::Error::message( "The sandbox image must include git for repository clone and git lifecycle \ operations. Use an image with bash and git, such as buildpack-deps:noble.", @@ -224,7 +225,7 @@ async fn run_local_step( .run(command, Some(remaining), Some("/"), None, None) .await .map_err(|error| crate::Error::context(format!("{label} transport failed"), error))?; - if result.is_success() { + if result.success() { return Ok(result); } Err(clone_failure_error( @@ -275,7 +276,7 @@ async fn embed_origin_credentials( ) .await { - Ok(result) if result.is_success() => {} + Ok(result) if result.success() => {} Ok(result) => { let err = result .into_exec_error_with_redactor("git remote set-url origin (post-clone)", |s| { diff --git a/lib/components/fabro-sandbox/src/daytona.rs b/lib/components/fabro-sandbox/src/daytona.rs index 99c6354e6..6076a5cd6 100644 --- a/lib/components/fabro-sandbox/src/daytona.rs +++ b/lib/components/fabro-sandbox/src/daytona.rs @@ -800,8 +800,8 @@ mod wire_gate { ) .await .expect("layout check"); - assert!(result.is_success(), "{result:?}"); - assert!(result.stdout.contains("true")); + assert!(result.success(), "{result:?}"); + assert!(result.stdout_lossy().contains("true")); let layout = sandbox.workspace_layout().expect("layout record"); assert_eq!( layout.primary_repo_path.as_deref(), diff --git a/lib/components/fabro-sandbox/src/driver_sandbox.rs b/lib/components/fabro-sandbox/src/driver_sandbox.rs index ef52b833b..acea8f57c 100644 --- a/lib/components/fabro-sandbox/src/driver_sandbox.rs +++ b/lib/components/fabro-sandbox/src/driver_sandbox.rs @@ -23,9 +23,10 @@ use fabro_github::token_source::InstallationTokenSource; use fabro_types::SandboxProviderKind; use fabro_util::workspace_glob::WorkspaceGlob; use sandbox_driver::{ - DirEntry, EventContext, FileKind, GrepMatch, GrepOptions, LifecycleTimers, PtyOptions, PtySize, - Sandbox as DriverHandle, SandboxProvider as DriverProvider, SandboxSource, - SandboxSpec as DriverSpec, SandboxState, Search as _, WaitOptions, WalkOptions, + DirEntry, EventContext, ExecControls, ExecResult, ExecSpec, ExecStreamingResult, FileKind, + GrepMatch, GrepOptions, LifecycleTimers, PtyOptions, PtySize, Sandbox as DriverHandle, + SandboxProvider as DriverProvider, SandboxSource, SandboxSpec as DriverSpec, SandboxState, + Search as _, StdioProcess, WaitOptions, WalkOptions, }; use sandbox_driver_host::HostProvider; use tokio::fs; @@ -71,10 +72,7 @@ pub async fn local_sandbox_with_events( Ok(sandbox) } use crate::exec::{ExplicitEnvPolicy, SandboxExec}; -use crate::sandbox::{ - self, ExecResult, ExecStreamingRequest, ExecStreamingResult, PushError, PushReport, - SandboxFile, SandboxWorkspaceLayout, StdioProcess, -}; +use crate::sandbox::{self, PushError, PushReport, SandboxFile, SandboxWorkspaceLayout}; /// Where a clone-based provider puts its files: the run works under /// `workspace_root`, and repositories check out under `repos_root`. @@ -799,22 +797,29 @@ impl RunSandbox { .await } + /// Runs `spec` under fabro's exec policy, delivering output through + /// `controls.sink` as it arrives. Build the spec with + /// [`ExecSpec::bash`]; the policy fills the stop grace, the run's + /// working directory, and the environment filter where the spec leaves + /// them open. pub async fn exec_command_streaming( &self, - request: ExecStreamingRequest<'_>, + spec: ExecSpec, + controls: ExecControls, ) -> crate::Result { - self.exec()?.run_streaming(request).await + self.exec()?.run_streaming(spec, controls).await } + /// Launches a long-lived process with bidirectional stdio. The returned + /// handle terminates the process; dropping it does not. pub async fn spawn_stdio_process( &self, command: &str, working_dir: Option<&str>, env_vars: Option<&HashMap>, - cancel_token: Option, ) -> crate::Result { self.exec()? - .spawn_stdio(command, working_dir, env_vars, cancel_token) + .spawn_stdio(command, working_dir, env_vars) .await } @@ -1077,7 +1082,7 @@ impl RunSandbox { .exec_command("git remote get-url origin", 10_000, None, None, None) .await { - Ok(result) if result.is_success() => true, + Ok(result) if result.success() => true, Ok(_) => false, Err(err) => { return Err(PushError { @@ -1192,12 +1197,12 @@ fn elapsed_ms(started: Instant) -> u64 { mod tests { use std::sync::Mutex; - use fabro_types::CommandTermination; - use sandbox_driver::{SandboxProvider as _, SandboxSource, SandboxSpec}; + use sandbox_driver::{SandboxProvider as _, SandboxSource, SandboxSpec, Termination}; use sandbox_driver_host::HostProvider; use tokio::fs; use super::*; + use crate::exec::ExecResultExt; struct Fixture { dir: tempfile::TempDir, @@ -1284,15 +1289,15 @@ mod tests { ) .await .unwrap(); - assert_eq!(ok.stdout, "hello\nbash\n"); - assert!(ok.is_success()); + assert_eq!(ok.stdout_lossy(), "hello\nbash\n"); + assert!(ok.success()); let timed_out = f .sandbox .exec_command("sleep 10", 200, None, None, None) .await .unwrap(); - assert_eq!(timed_out.termination, CommandTermination::TimedOut); - assert_eq!(timed_out.exit_code, None); + assert_eq!(timed_out.termination, Termination::TimedOut); + assert_eq!(timed_out.program_exit_code(), None); } #[tokio::test] diff --git a/lib/components/fabro-sandbox/src/error.rs b/lib/components/fabro-sandbox/src/error.rs index 57c132f23..45c6b4f01 100644 --- a/lib/components/fabro-sandbox/src/error.rs +++ b/lib/components/fabro-sandbox/src/error.rs @@ -2,7 +2,6 @@ use std::fmt::Write as _; use fabro_util::error::{collect_causes, render_with_causes}; -use crate::ExecResult; use crate::sandbox::{DEFAULT_EXEC_OUTPUT_TAIL_BYTES, redacted_output_tail}; #[derive(Debug, thiserror::Error)] @@ -30,17 +29,6 @@ pub enum Error { /// `Transport`, and `Incomplete` without string matching. #[error(transparent)] Driver(Box), - - #[error( - "{label} failed (exit {exit}, termination={termination}, duration_ms={duration_ms}) - hint: {hint}", - exit = format_exit_code(result.exit_code), - termination = result.termination, - duration_ms = result.duration_ms, - hint = classify_exec_failure(&result.stderr) - .or_else(|| classify_exec_failure(&result.stdout)) - .unwrap_or("unclassified") - )] - Exec { label: String, result: ExecResult }, } impl Error { @@ -65,13 +53,6 @@ impl Error { } } - pub fn exec(label: impl Into, result: ExecResult) -> Self { - Self::Exec { - label: label.into(), - result, - } - } - pub fn default_redacted_output_tail(&self) -> Option { default_redacted_output_tail(self) } @@ -100,6 +81,16 @@ impl Error { None } + /// The command that ran and failed, when this error reports one: a + /// non-zero exit fabro turned into an error, or a driver operation whose + /// command failed underneath it. + pub fn exec_failure(&self) -> Option<&sandbox_driver::ExecFailure> { + match self.driver()? { + sandbox_driver::Error::Exec(failure) => Some(failure), + _ => None, + } + } + /// The facts established when a driver operation ended without a /// complete outcome. A caller that sees `Some` must not replay the /// operation: its effects may already have happened. @@ -153,44 +144,6 @@ impl From<&str> for Error { } } -pub(crate) fn classify_exec_failure(stderr: &str) -> Option<&'static str> { - let lower = stderr.to_ascii_lowercase(); - if lower.contains("could not read username") || lower.contains("terminal prompts disabled") { - Some( - "no credentials in origin URL - check that the sandbox forwarded \ - GITHUB_APP_PRIVATE_KEY (or GITHUB_TOKEN) and that refresh_push_credentials succeeded", - ) - } else if lower.contains("permission to") && lower.contains("denied") { - Some( - "github denied the push - installation token lacks contents:write \ - on this repo, or a branch protection / push ruleset is rejecting the ref", - ) - } else if lower.contains("protected branch") - || lower.contains("ruleset") - || lower.contains("rejected") - { - Some("github rejected the ref - likely a branch protection rule or push ruleset") - } else if lower.contains("authentication failed") || lower.contains("invalid username") { - Some("github authentication failed - installation token may be expired or wrong scope") - } else if lower.contains("could not resolve host") || lower.contains("network is unreachable") { - Some("network failure inside sandbox - check DNS / egress from the run container") - } else if lower.contains("repository not found") { - Some("github 404 - repository is unavailable to the current credentials") - } else if lower.contains("no such remote") && lower.contains("origin") { - Some("origin remote missing - push credentials could not be installed") - } else if lower.contains("not a git repository") - || lower.contains("does not appear to be a git repository") - { - Some("git repository unavailable in sandbox working directory") - } else { - None - } -} - -fn format_exit_code(exit_code: Option) -> String { - exit_code.map_or_else(|| "none".to_string(), |code| code.to_string()) -} - pub type Result = std::result::Result; pub fn default_redacted_output_tail( @@ -198,14 +151,10 @@ pub fn default_redacted_output_tail( ) -> Option { let mut current = Some(err); while let Some(err) = current { - match err.downcast_ref::() { - Some(Error::Exec { result, .. }) => return result.default_redacted_output_tail(), - Some(Error::Driver(driver)) => { - if let Some(tail) = driver_output_tail(driver) { - return Some(tail); - } + if let Some(Error::Driver(driver)) = err.downcast_ref::() { + if let Some(tail) = driver_output_tail(driver) { + return Some(tail); } - _ => {} } if let Some(driver) = err.downcast_ref::() { if let Some(tail) = driver_output_tail(driver) { @@ -262,64 +211,67 @@ fn append_tail_for_log(rendered: &mut String, stream: &str, tail: Option<&str>, #[cfg(test)] mod tests { - use fabro_types::CommandTermination; + use std::time::Duration; + + use sandbox_driver::{ExecResult, Termination}; use super::*; + use crate::exec::ExecResultExt; + + const SECRET: &str = "ghs_xK9mZ2vL8nQ5rT1wY4bC7dF0gH3jE6pA"; + + fn failed_push(stdout: &str, stderr: &str) -> Error { + let mut result = + ExecResult::new(Termination::Exited, Some(128), Duration::from_millis(210)); + result.stdout = stdout.as_bytes().to_vec(); + result.stderr = stderr.as_bytes().to_vec(); + result.into_exec_error("git push origin refs/heads/run") + } + + fn leaky_stderr() -> String { + format!( + "fatal: unable to access 'https://x-access-token:{SECRET}@github.com/owner/repo/':\n\ + remote: Permission to owner/repo.git denied\n\ + identity ~/.ssh/id_rsa_work" + ) + } #[test] fn exec_display_is_log_safe() { - let stderr = "fatal: unable to access \ - 'https://x-access-token:ghs_xK9mZ2vL8nQ5rT1wY4bC7dF0gH3jE6pA@github.com/owner/repo/':\n\ - remote: Permission to owner/repo.git denied\n\ - identity ~/.ssh/id_rsa_work"; - let error = Error::exec("git push origin refs/heads/run", crate::ExecResult { - stdout: String::new(), - stderr: stderr.to_string(), - exit_code: Some(128), - termination: CommandTermination::Exited, - duration_ms: 210, - }); + let error = failed_push("", &leaky_stderr()); let rendered = error.to_string(); assert_exec_rendering_is_safe(&rendered); assert!(rendered.contains("git push origin refs/heads/run")); - assert!(rendered.contains("exit 128")); - assert!(rendered.contains("termination=exited")); - assert!(rendered.contains("duration_ms=210")); - assert!(rendered.contains("hint:")); + assert!(rendered.contains("128")); + assert!(rendered.contains("210 ms")); } #[test] fn display_with_causes_does_not_reintroduce_raw_exec_output() { - let stderr = "fatal: unable to access \ - 'https://x-access-token:ghs_xK9mZ2vL8nQ5rT1wY4bC7dF0gH3jE6pA@github.com/owner/repo/':\n\ - remote: Permission to owner/repo.git denied\n\ - identity ~/.ssh/id_rsa_work"; - let exec_error = Error::exec("git push origin refs/heads/run", crate::ExecResult { - stdout: "stdout secret ghs_xK9mZ2vL8nQ5rT1wY4bC7dF0gH3jE6pA".to_string(), - stderr: stderr.to_string(), - exit_code: Some(128), - termination: CommandTermination::Exited, - duration_ms: 210, - }); + let exec_error = failed_push(&format!("stdout secret {SECRET}"), &leaky_stderr()); let error = Error::context("metadata push failed", exec_error); let rendered = error.display_with_causes(); assert_exec_rendering_is_safe(&rendered); assert!(rendered.contains("metadata push failed")); assert!(rendered.contains("git push origin refs/heads/run")); - assert!(rendered.contains("hint:")); + } + + #[test] + fn exec_failure_is_reachable_through_the_context_chain() { + let error = Error::context("metadata push failed", failed_push("", "boom")); + + let failure = error.exec_failure().expect("exec failure"); + assert_eq!(failure.label(), "git push origin refs/heads/run"); + assert_eq!(failure.exit_code(), Some(128)); + assert_eq!(failure.termination(), Termination::Exited); + assert!(Error::message("plain").exec_failure().is_none()); } #[test] fn display_for_log_walks_context_chain_and_emits_tail() { - let exec_error = Error::exec("git push origin refs/heads/run", crate::ExecResult { - stdout: "last stdout line".to_string(), - stderr: "last stderr line".to_string(), - exit_code: Some(128), - termination: CommandTermination::Exited, - duration_ms: 210, - }); + let exec_error = failed_push("last stdout line", "last stderr line"); let error = Error::context("metadata push failed", exec_error); let rendered = display_for_log(&error); @@ -334,18 +286,15 @@ mod tests { #[test] fn display_for_log_redacts_secrets() { - let error = Error::exec("git push origin refs/heads/run", crate::ExecResult { - stdout: "stdout secret ghs_xK9mZ2vL8nQ5rT1wY4bC7dF0gH3jE6pA".to_string(), - stderr: "stderr secret ghs_xK9mZ2vL8nQ5rT1wY4bC7dF0gH3jE6pA".to_string(), - exit_code: Some(128), - termination: CommandTermination::Exited, - duration_ms: 210, - }); + let error = failed_push( + &format!("stdout secret {SECRET}"), + &format!("stderr secret {SECRET}"), + ); let rendered = display_for_log(&error); assert!( - !rendered.contains("ghs_xK9mZ2vL8nQ5rT1wY4bC7dF0gH3jE6pA"), + !rendered.contains(SECRET), "log rendering leaked raw secret: {rendered}" ); assert!(rendered.contains("REDACTED")); @@ -367,7 +316,7 @@ mod tests { "fatal:", "remote:", "x-access-token", - "ghs_xK9mZ2vL8nQ5rT1wY4bC7dF0gH3jE6pA", + SECRET, "~/.ssh", "id_rsa_work", ] { @@ -380,14 +329,7 @@ mod tests { #[test] fn exec_error_exposes_default_redacted_output_tail() { - let stderr = "stderr secret ghs_xK9mZ2vL8nQ5rT1wY4bC7dF0gH3jE6pA"; - let error = Error::exec("git push origin refs/heads/run", crate::ExecResult { - stdout: "last stdout line".to_string(), - stderr: stderr.to_string(), - exit_code: Some(128), - termination: CommandTermination::Exited, - duration_ms: 210, - }); + let error = failed_push("last stdout line", &format!("stderr secret {SECRET}")); let tail = error.default_redacted_output_tail().expect("tail present"); assert_eq!(tail.stdout.as_deref(), Some("last stdout line")); @@ -401,13 +343,7 @@ mod tests { #[test] fn free_tail_helper_walks_context_chain() { - let exec_error = Error::exec("git push origin refs/heads/run", crate::ExecResult { - stdout: "last stdout line".to_string(), - stderr: "last stderr line".to_string(), - exit_code: Some(128), - termination: CommandTermination::Exited, - duration_ms: 210, - }); + let exec_error = failed_push("last stdout line", "last stderr line"); let error = Error::context("metadata push failed", exec_error); let tail = default_redacted_output_tail(&error).expect("tail present"); @@ -415,42 +351,4 @@ mod tests { assert_eq!(tail.stdout.as_deref(), Some("last stdout line")); assert_eq!(tail.stderr.as_deref(), Some("last stderr line")); } - - #[test] - fn classify_exec_failure_documents_known_branches() { - let cases = [ - ( - "fatal: could not read Username for 'https://github.com'", - "no credentials in origin URL", - ), - ( - "remote: Permission to owner/repo.git denied to fabro-app[bot].", - "github denied the push", - ), - ( - "remote: error: GH013: Repository rule violations found due to ruleset", - "github rejected the ref", - ), - ( - "fatal: Authentication failed for 'https://github.com/owner/repo'", - "github authentication failed", - ), - ( - "fatal: could not resolve host: github.com", - "network failure", - ), - ("remote: Repository not found.", "github 404"), - ("error: No such remote 'origin'", "origin remote missing"), - ("fatal: not a git repository", "git repository unavailable"), - ]; - - for (stderr, expected) in cases { - let hint = classify_exec_failure(stderr).expect(stderr); - assert!( - hint.contains(expected), - "expected {hint:?} to contain {expected:?}" - ); - } - assert_eq!(classify_exec_failure("weird new git error"), None); - } } diff --git a/lib/components/fabro-sandbox/src/exec.rs b/lib/components/fabro-sandbox/src/exec.rs index a01eacb9c..463f9c7c0 100644 --- a/lib/components/fabro-sandbox/src/exec.rs +++ b/lib/components/fabro-sandbox/src/exec.rs @@ -1,41 +1,40 @@ //! Fabro's command execution policy over the sandbox-driver [`Exec`] facet. //! +//! The vocabulary is the driver's own: an [`ExecSpec`] and [`ExecControls`] +//! go in, an [`ExecResult`] or [`ExecStreamingResult`] comes out. This +//! module adds fabro's policy on the way in and fabro's reading of a result +//! on the way out. +//! //! A command runs as Bash source under `bash -c` with `BASH_ENV` blanked, //! and ends in one of three ways: //! //! - **timeout**: the spec's timeout fires and the provider runs the stop //! ladder fabro asks for — `TERM`, then `KILL` after -//! [`SandboxExec::stop_grace`]. The result reports -//! [`CommandTermination::TimedOut`]. +//! [`SandboxExec::stop_grace`]. The result reports [`Termination::TimedOut`]. //! - **cancellation**: the caller's [`CancellationToken`] is the `term` stop; //! the provider escalates to `KILL` after the same grace. The result reports -//! [`CommandTermination::Cancelled`]. +//! [`Termination::Cancelled`]. //! - **exit**: the process ended on its own. //! //! Output is drained regardless of the retention cap, redacted only when a -//! tail is rendered for events or logs, and delivered live through the -//! caller's callback. Explicit environment variables pass through a -//! fail-closed secret filter under [`ExplicitEnvPolicy::FilterSensitive`], -//! matching what the Host provider already does for inherited variables. +//! tail is rendered for events or logs ([`ExecResultExt`]), and delivered +//! live through the caller's [`sandbox_driver::OutputSink`]. Explicit +//! environment variables pass through a fail-closed secret filter under +//! [`ExplicitEnvPolicy::FilterSensitive`], matching what the Host provider +//! already does for inherited variables. -use std::collections::HashMap; -use std::sync::Arc; +use std::collections::{BTreeMap, HashMap}; use std::time::Duration; -use async_trait::async_trait; use fabro_static::EnvVars; -use fabro_types::{CommandOutputStream, CommandTermination}; +use fabro_types::{CommandTermination, ExecOutputTail}; use sandbox_driver::{ - BASH_ENV_VAR, CaptureStats, Exec, ExecControls, ExecSpec, OutputStream, SpawnSpec, - StdioProcessHandle as DriverStdioProcessHandle, Termination, TransportError, + BASH_ENV_VAR, Exec, ExecControls, ExecFailure, ExecResult, ExecSpec, ExecStreamingResult, + SpawnSpec, StdioProcess, Termination, }; use tokio_util::sync::CancellationToken; -use crate::sandbox::{ - CommandOutputCallback, ExecResult, ExecStreamingRequest, ExecStreamingResult, - OutputCaptureStats, StderrCollector, StdioProcess, StdioProcessControl, StdioProcessHandle, - StdioProcessTermination, -}; +use crate::sandbox::{DEFAULT_EXEC_OUTPUT_TAIL_BYTES, redacted_output_tail}; /// Time between `TERM` and `KILL` when fabro stops a command. pub const DEFAULT_STOP_GRACE: Duration = Duration::from_secs(2); @@ -131,7 +130,8 @@ impl<'a> SandboxExec<'a> { /// /// Equivalent to `bash -c ` with a clean, non-login shell: no /// `errexit`, no `pipefail`, `BASH_ENV` blanked. A caller that wants - /// different semantics writes them into the command. + /// different semantics writes them into the command. `None` for + /// `timeout` runs without a deadline. pub async fn run( &self, command: &str, @@ -140,216 +140,198 @@ impl<'a> SandboxExec<'a> { env_vars: Option<&HashMap>, cancel_token: Option, ) -> crate::Result { - let streaming = self - .run_streaming(ExecStreamingRequest { - timeout_ms: timeout - .map(|timeout| u64::try_from(timeout.as_millis()).unwrap_or(u64::MAX)), - working_dir, - env_vars, - cancel_token, - ..ExecStreamingRequest::new(command) - }) - .await?; - Ok(streaming.result) - } - - /// Runs Bash source, delivering output through `request.output_callback` - /// as it arrives. Same interpreter contract as [`Self::run`]. - pub async fn run_streaming( - &self, - request: ExecStreamingRequest<'_>, - ) -> crate::Result { - let ExecStreamingRequest { - command, - timeout_ms, - working_dir, - env_vars, - cancel_token, - stdin, - output_callback, - stream_output_bytes_cap, - } = request; - - let mut spec = ExecSpec::bash(command) - .no_timeout() - .stop_grace(self.stop_grace); - if let Some(timeout_ms) = timeout_ms { - spec = spec.timeout(Duration::from_millis(timeout_ms)); + let mut spec = ExecSpec::bash(command).no_timeout(); + if let Some(timeout) = timeout { + spec = spec.timeout(timeout); } - if let Some(dir) = working_dir.or(self.working_dir.as_deref()) { + if let Some(dir) = working_dir { spec = spec.working_dir(dir); } - for (key, value) in self.explicit_env(env_vars) { + for (key, value) in env_vars.into_iter().flatten() { spec = spec.env_var(key, value); } - if let Some(bytes) = stdin { - spec = spec.stdin(bytes); - } - - // The caller's cancellation is the `term` stop; the provider runs - // the grace and the `kill` itself. let controls = ExecControls { - term: cancel_token, - kill: None, - stdin: None, - sink: output_callback.map(adapt_output_callback), - retained_output_limit: Some( - stream_output_bytes_cap.unwrap_or(DEFAULT_RETAINED_OUTPUT_BYTES), - ), + term: cancel_token, + ..ExecControls::default() }; + Ok(self.run_streaming(spec, controls).await?.result) + } - let streaming = self.exec.run_streaming(&spec, controls).await?; - - let termination = map_termination(streaming.result.termination); - let duration_ms = duration_ms(streaming.result.duration); - Ok(ExecStreamingResult { - result: ExecResult { - stdout: String::from_utf8_lossy(&streaming.result.stdout).into_owned(), - stderr: String::from_utf8_lossy(&streaming.result.stderr).into_owned(), - exit_code: exit_code_for(termination, streaming.result.exit_code), - termination, - duration_ms, - }, - streams_separated: streaming.streams_separated, - live_streaming: streaming.live_streaming, - stdout_capture: capture_stats(streaming.stdout_capture), - stderr_capture: capture_stats(streaming.stderr_capture), - }) + /// Runs `spec` under fabro's policy, delivering output through + /// `controls.sink` as it arrives. + /// + /// The policy fills what the spec leaves open: the stop grace, the + /// working directory, and the explicit environment filter. The + /// caller's `controls.term` is the `term` stop; the provider runs the + /// grace and the `kill` itself. Output beyond + /// `controls.retained_output_limit` (fabro's default when unset) is + /// drained and counted, not kept. + pub async fn run_streaming( + &self, + spec: ExecSpec, + mut controls: ExecControls, + ) -> crate::Result { + let spec = self.apply_policy(spec); + if controls.retained_output_limit.is_none() { + controls.retained_output_limit = Some(DEFAULT_RETAINED_OUTPUT_BYTES); + } + Ok(self.exec.run_streaming(&spec, controls).await?) } /// Launches a long-lived process with bidirectional stdio. /// /// `command` is evaluated under the same non-login Bash contract before - /// the shell replaces itself with the requested process. Cancelling - /// `cancel_token` terminates the process. + /// the shell replaces itself with the requested process. The returned + /// handle terminates the process; dropping it does not. pub async fn spawn_stdio( &self, command: &str, working_dir: Option<&str>, env_vars: Option<&HashMap>, - cancel_token: Option, ) -> crate::Result { let mut spec = SpawnSpec::bash(format!("exec {command}")); if let Some(dir) = working_dir.or(self.working_dir.as_deref()) { spec = spec.working_dir(dir); } - for (key, value) in self.explicit_env(env_vars) { + for (key, value) in env_vars.into_iter().flatten() { spec = spec.env_var(key, value); } - let process = self.exec.spawn_stdio(&spec).await?; - let handle = StdioProcessHandle::new(DriverStdioControl { - handle: Arc::from(process.handle), - }); - if let Some(token) = cancel_token { - let handle = handle.clone(); - tokio::spawn(async move { - token.cancelled().await; - if let Err(error) = handle.terminate().await { - tracing::warn!(error = %error, "failed to terminate stdio process on cancel"); - } - }); - } - Ok(StdioProcess { - stdin: process.stdin, - stdout: process.stdout, - stderr: StderrCollector::from_driver_tail(process.stderr_tail), - handle, - }) + self.apply_env_policy(&mut spec.env); + Ok(self.exec.spawn_stdio(&spec).await?) } - /// The explicit environment after policy: `BASH_ENV` never passes, - /// because the Bash helper blanks it and a caller value would override - /// that; credential-shaped names pass only under `TrustCaller`. - fn explicit_env(&self, env_vars: Option<&HashMap>) -> Vec<(String, String)> { - let mut entries: Vec<(String, String)> = env_vars - .into_iter() - .flatten() - .filter(|(key, _)| key.as_str() != BASH_ENV_VAR) - .filter(|(key, _)| { - self.env_policy == ExplicitEnvPolicy::TrustCaller || !is_sensitive_env_var(key) - }) - .map(|(key, value)| (key.clone(), value.clone())) - .collect(); - entries.sort(); - entries + fn apply_policy(&self, mut spec: ExecSpec) -> ExecSpec { + if spec.stop_grace.is_none() { + spec.stop_grace = Some(self.stop_grace); + } + if spec.working_dir.is_none() { + spec.working_dir.clone_from(&self.working_dir); + } + self.apply_env_policy(&mut spec.env); + spec + } + + /// The explicit environment after policy: credential-shaped names pass + /// only under `TrustCaller`, and the Bash helper's `BASH_ENV` blank + /// wins over any caller value, so a worker's startup file never runs + /// inside a sandboxed `bash -c`. + fn apply_env_policy(&self, env: &mut BTreeMap) { + if self.env_policy == ExplicitEnvPolicy::FilterSensitive { + env.retain(|key, _| !is_sensitive_env_var(key)); + } + env.insert(BASH_ENV_VAR.to_string(), String::new()); } } -/// The driver says how the command ended; fabro's vocabulary has two stops. -/// A timeout is the provider's deadline (the ladder ran for it); a +/// The driver says how the command ended; fabro's event vocabulary has two +/// stops. A timeout is the provider's deadline (the ladder ran for it); a /// cancelled or killed command was stopped by the caller's token, by a /// foreign `kill`, or by a provider-side abort — it did not finish and no -/// deadline passed. -fn map_termination(termination: Termination) -> CommandTermination { +/// deadline passed. `Exited`, or a provider that could not tell, is a +/// completed process; nothing asserts success here. +#[must_use] +pub fn command_termination(termination: Termination) -> CommandTermination { match termination { Termination::TimedOut => CommandTermination::TimedOut, Termination::Cancelled | Termination::Killed => CommandTermination::Cancelled, - // `Exited`, or a provider that could not tell how the command ended. - // Nothing asserts success here: `exit_code` is whatever was observed - // and `is_success` still requires `Some(0)`. _ => CommandTermination::Exited, } } /// An exit code is only the command's own when it exited on its own. A /// stopped command may still report the shell's `128 + signal` (143 for a -/// trapped `TERM`), which callers must not mistake for a program result. -fn exit_code_for(termination: CommandTermination, exit_code: Option) -> Option { - match termination { +/// trapped `TERM`), which events must not present as a program result. +#[must_use] +pub fn program_exit_code(termination: Termination, exit_code: Option) -> Option { + match command_termination(termination) { CommandTermination::Exited => exit_code, CommandTermination::TimedOut | CommandTermination::Cancelled => None, } } -fn capture_stats(stats: CaptureStats) -> OutputCaptureStats { - OutputCaptureStats { - observed_bytes: stats.observed_bytes, - retained_bytes: stats.retained_bytes, - omitted_bytes: stats.omitted_bytes, - } +/// Fabro's reading of a driver [`ExecResult`]: the event-facing numbers, +/// the redacted output tail, and the failure a non-zero exit is. +pub trait ExecResultExt { + /// The provider's measured run time in whole milliseconds. + fn duration_ms(&self) -> u64; + + /// The exit code when the command ended on its own; see + /// [`program_exit_code`]. + fn program_exit_code(&self) -> Option; + + /// Redacted, sanitized tails of both streams, each bounded to + /// `max_bytes_per_stream`. `None` when both streams are empty. + fn redacted_output_tail(&self, max_bytes_per_stream: usize) -> Option; + + /// [`Self::redacted_output_tail`] at fabro's event budget. + fn default_redacted_output_tail(&self) -> Option; + + /// The failure this result is, reported under `label`. The raw output + /// stays behind the driver's [`ExecFailure`] accessors; `Display` + /// carries only the label and the classified metadata. + fn into_exec_error(self, label: impl Into) -> crate::Error; + + /// [`Self::into_exec_error`] with `redactor` applied to both streams + /// first, for output that can carry a credentialed URL. + fn into_exec_error_with_redactor( + self, + label: impl Into, + redactor: impl Fn(&str) -> String, + ) -> crate::Error; + + /// `Ok(self)` for a clean exit, the failure under `label` otherwise. + fn into_result(self, label: impl Into) -> crate::Result; } -fn adapt_output_callback(callback: CommandOutputCallback) -> sandbox_driver::OutputSink { - Arc::new(move |stream, chunk| { - let stream = match stream { - OutputStream::Stdout => CommandOutputStream::Stdout, - OutputStream::Stderr => CommandOutputStream::Stderr, - }; - let callback = Arc::clone(&callback); - Box::pin(async move { - callback(stream, chunk).await.map_err(|error| { - sandbox_driver::Error::Transport(TransportError::with_source( - "command output callback failed", - error, - )) - }) - }) - }) -} - -/// The provider's measured run time in whole milliseconds. -fn duration_ms(duration: Duration) -> u64 { - u64::try_from(duration.as_millis()).unwrap_or(u64::MAX) -} - -struct DriverStdioControl { - handle: Arc, -} - -#[async_trait] -impl StdioProcessControl for DriverStdioControl { - async fn terminate(&self) -> crate::Result<()> { - self.handle.terminate().await; - Ok(()) +impl ExecResultExt for ExecResult { + fn duration_ms(&self) -> u64 { + u64::try_from(self.duration.as_millis()).unwrap_or(u64::MAX) } - async fn wait(&self) -> crate::Result { - let (termination, exit_code) = self.handle.wait().await; - let termination = map_termination(termination); - Ok(StdioProcessTermination { - termination, - exit_code: exit_code_for(termination, exit_code), - }) + fn program_exit_code(&self) -> Option { + program_exit_code(self.termination, self.exit_code) + } + + fn redacted_output_tail(&self, max_bytes_per_stream: usize) -> Option { + redacted_output_tail( + &self.stdout_lossy(), + &self.stderr_lossy(), + max_bytes_per_stream, + ) + } + + fn default_redacted_output_tail(&self) -> Option { + self.redacted_output_tail(DEFAULT_EXEC_OUTPUT_TAIL_BYTES) + } + + fn into_exec_error(self, label: impl Into) -> crate::Error { + let failure = ExecFailure::new( + label, + self.termination, + self.exit_code, + self.stdout, + self.stderr, + ) + .with_duration(self.duration); + crate::Error::driver_error(failure.into()) + } + + fn into_exec_error_with_redactor( + mut self, + label: impl Into, + redactor: impl Fn(&str) -> String, + ) -> crate::Error { + self.stdout = redactor(&self.stdout_lossy()).into_bytes(); + self.stderr = redactor(&self.stderr_lossy()).into_bytes(); + self.into_exec_error(label) + } + + fn into_result(self, label: impl Into) -> crate::Result { + if self.success() { + Ok(self) + } else { + Err(self.into_exec_error(label)) + } } } @@ -358,7 +340,9 @@ mod tests { use std::sync::{Arc, Mutex}; use std::time::Instant; - use sandbox_driver::{SandboxProvider as _, SandboxSource, SandboxSpec}; + use sandbox_driver::{ + OutputSink, OutputStream, SandboxProvider as _, SandboxSource, SandboxSpec, TransportError, + }; use sandbox_driver_host::HostProvider; use tokio::io::{AsyncBufReadExt, AsyncWriteExt, BufReader}; use tokio::{fs, time}; @@ -404,16 +388,30 @@ mod tests { .unwrap() } + fn exec_result( + stdout: &str, + stderr: &str, + exit_code: Option, + termination: Termination, + duration_ms: u64, + ) -> ExecResult { + let mut result = + ExecResult::new(termination, exit_code, Duration::from_millis(duration_ms)); + result.stdout = stdout.as_bytes().to_vec(); + result.stderr = stderr.as_bytes().to_vec(); + result + } + #[tokio::test] async fn runs_bash_source_and_reports_exit_code_and_streams() { let fixture = HostFixture::new().await; let result = run(&fixture, "echo out; echo err >&2; exit 3").await; - assert_eq!(result.stdout, "out\n"); - assert_eq!(result.stderr, "err\n"); + assert_eq!(result.stdout_lossy(), "out\n"); + assert_eq!(result.stderr_lossy(), "err\n"); assert_eq!(result.exit_code, Some(3)); - assert_eq!(result.termination, CommandTermination::Exited); - assert!(!result.is_success()); - assert!(run(&fixture, "true").await.is_success()); + assert_eq!(result.termination, Termination::Exited); + assert!(!result.success()); + assert!(run(&fixture, "true").await.success()); } #[tokio::test] @@ -425,7 +423,7 @@ mod tests { set -o | grep -E '^(errexit|pipefail)' | awk '{print $2}' | sort -u", ) .await; - assert_eq!(result.stdout, "nonlogin\noff\n", "{result:?}"); + assert_eq!(result.stdout_lossy(), "nonlogin\noff\n", "{result:?}"); } #[tokio::test] @@ -447,7 +445,7 @@ mod tests { ) .await .unwrap(); - assert_eq!(result.stdout, "body\n"); + assert_eq!(result.stdout_lossy(), "body\n"); } #[tokio::test] @@ -461,16 +459,18 @@ mod tests { .exec(ExplicitEnvPolicy::FilterSensitive) .run("env", Some(Duration::from_secs(10)), None, Some(&env), None) .await - .unwrap(); - assert!(!filtered.stdout.contains("FABRO_WORKER_TOKEN=leaked")); - assert!(filtered.stdout.contains("MY_VAR=ok")); + .unwrap() + .stdout_lossy(); + assert!(!filtered.contains("FABRO_WORKER_TOKEN=leaked")); + assert!(filtered.contains("MY_VAR=ok")); let trusted = fixture .exec(ExplicitEnvPolicy::TrustCaller) .run("env", Some(Duration::from_secs(10)), None, Some(&env), None) .await - .unwrap(); - assert!(trusted.stdout.contains("FABRO_WORKER_TOKEN=leaked")); + .unwrap() + .stdout_lossy(); + assert!(trusted.contains("FABRO_WORKER_TOKEN=leaked")); } #[test] @@ -505,8 +505,8 @@ mod tests { ) .await .unwrap(); - assert_eq!(result.termination, CommandTermination::TimedOut); - assert_eq!(result.exit_code, None); + assert_eq!(result.termination, Termination::TimedOut); + assert_eq!(result.program_exit_code(), None); assert!( started.elapsed() < Duration::from_secs(5), "sleep honours TERM, so KILL should not have been needed" @@ -529,7 +529,7 @@ mod tests { ) .await .unwrap(); - assert_eq!(result.termination, CommandTermination::TimedOut); + assert_eq!(result.termination, Termination::TimedOut); let elapsed = started.elapsed(); assert!(elapsed >= Duration::from_millis(400), "{elapsed:?}"); assert!(elapsed < Duration::from_secs(5), "{elapsed:?}"); @@ -555,8 +555,8 @@ mod tests { ) .await .unwrap(); - assert_eq!(result.termination, CommandTermination::Cancelled); - assert_eq!(result.exit_code, None); + assert_eq!(result.termination, Termination::Cancelled); + assert_eq!(result.program_exit_code(), None); } #[tokio::test] @@ -564,33 +564,36 @@ mod tests { let fixture = HostFixture::new().await; let seen = Arc::new(Mutex::new(Vec::::new())); let sink_seen = Arc::clone(&seen); - let callback: CommandOutputCallback = Arc::new(move |stream, chunk| { + let sink: OutputSink = Arc::new(move |stream, chunk| { let seen = Arc::clone(&sink_seen); Box::pin(async move { - assert_eq!(stream, CommandOutputStream::Stdout); + assert_eq!(stream, OutputStream::Stdout); seen.lock().unwrap().extend_from_slice(&chunk); Ok(()) }) }); let streaming = fixture .exec(ExplicitEnvPolicy::FilterSensitive) - .run_streaming(ExecStreamingRequest { - timeout_ms: Some(10_000), - output_callback: Some(callback), - stream_output_bytes_cap: Some(64), - ..ExecStreamingRequest::new("for i in $(seq 1 200); do echo line-$i; done") - }) + .run_streaming( + ExecSpec::bash("for i in $(seq 1 200); do echo line-$i; done") + .timeout(Duration::from_secs(10)), + ExecControls { + sink: Some(sink), + retained_output_limit: Some(64), + ..ExecControls::default() + }, + ) .await .unwrap(); - assert!(streaming.result.is_success()); + assert!(streaming.result.success()); assert!(streaming.live_streaming); assert!(streaming.streams_separated); let delivered = seen.lock().unwrap().len(); assert_eq!(streaming.stdout_capture.observed_bytes, delivered); assert!(streaming.stdout_capture.omitted_bytes > 0); assert!(streaming.result.stdout.len() <= 64); - assert!(streaming.result.stdout.starts_with("line-1\n")); - assert!(streaming.result.stdout.ends_with("line-200\n")); + assert!(streaming.result.stdout.starts_with(b"line-1\n")); + assert!(streaming.result.stdout.ends_with(b"line-200\n")); } #[tokio::test] @@ -599,34 +602,42 @@ mod tests { let stdin = b"first line\n$(touch must-not-run)\nlast line".to_vec(); let streaming = fixture .exec(ExplicitEnvPolicy::FilterSensitive) - .run_streaming(ExecStreamingRequest { - timeout_ms: Some(10_000), - stdin: Some(stdin.clone()), - ..ExecStreamingRequest::new("cat; test -e must-not-run && echo RAN") - }) + .run_streaming( + ExecSpec::bash("cat; test -e must-not-run && echo RAN") + .timeout(Duration::from_secs(10)) + .stdin(stdin.clone()), + ExecControls::default(), + ) .await .unwrap(); - assert_eq!(streaming.result.stdout.as_bytes(), stdin.as_slice()); + assert_eq!(streaming.result.stdout, stdin); } #[tokio::test] - async fn a_failing_output_callback_stops_the_command_with_an_error() { + async fn a_failing_output_sink_stops_the_command_with_an_error() { let fixture = HostFixture::new().await; - let callback: CommandOutputCallback = - Arc::new(|_, _| Box::pin(async { Err(crate::Error::message("consumer gave up")) })); + let sink: OutputSink = Arc::new(|_, _| { + Box::pin(async { + Err(sandbox_driver::Error::Transport(TransportError::new( + "consumer gave up", + ))) + }) + }); let error = fixture .exec(ExplicitEnvPolicy::FilterSensitive) - .run_streaming(ExecStreamingRequest { - timeout_ms: Some(10_000), - output_callback: Some(callback), - ..ExecStreamingRequest::new("echo hello; sleep 5") - }) + .run_streaming( + ExecSpec::bash("echo hello; sleep 5").timeout(Duration::from_secs(10)), + ExecControls { + sink: Some(sink), + ..ExecControls::default() + }, + ) .await .map(|streaming| streaming.result.termination); // The driver either surfaces the sink failure or reports the command // cancelled by it; both keep the consumer's error visible. match error { - Ok(termination) => assert_eq!(termination, CommandTermination::Cancelled), + Ok(termination) => assert_eq!(termination, Termination::Cancelled), Err(error) => assert!(error.to_string().contains("consumer gave up"), "{error}"), } } @@ -636,7 +647,7 @@ mod tests { let fixture = HostFixture::new().await; let process = fixture .exec(ExplicitEnvPolicy::FilterSensitive) - .spawn_stdio("cat", None, None, None) + .spawn_stdio("cat", None, None) .await .unwrap(); let mut stdin = process.stdin; @@ -646,52 +657,160 @@ mod tests { stdout.read_line(&mut line).await.unwrap(); assert_eq!(line, "ping\n"); drop(stdin); - let termination = process.handle.wait().await.unwrap(); - assert_eq!(termination.termination, CommandTermination::Exited); - assert_eq!(termination.exit_code, Some(0)); + let (termination, exit_code) = process.handle.wait().await; + assert_eq!(termination, Termination::Exited); + assert_eq!(exit_code, Some(0)); } #[tokio::test] - async fn stdio_process_terminates_on_cancel_and_keeps_a_stderr_tail() { + async fn stdio_process_terminates_on_request_and_keeps_a_stderr_tail() { let fixture = HostFixture::new().await; - let token = CancellationToken::new(); let process = fixture .exec(ExplicitEnvPolicy::FilterSensitive) - .spawn_stdio( - "sh -c 'echo diag >&2; sleep 30'", - None, - None, - Some(token.clone()), - ) + .spawn_stdio("sh -c 'echo diag >&2; sleep 30'", None, None) .await .unwrap(); time::sleep(Duration::from_millis(200)).await; - token.cancel(); - let termination = time::timeout(Duration::from_secs(5), process.handle.wait()) + process.handle.terminate().await; + let (termination, _) = time::timeout(Duration::from_secs(5), process.handle.wait()) .await - .expect("cancel terminates the process") - .unwrap(); - assert_ne!(termination.termination, CommandTermination::Exited); - assert_eq!(process.stderr.tail_string().await, "diag\n"); + .expect("terminate ends the process"); + assert_ne!(termination, Termination::Exited); + assert_eq!(process.stderr_tail.to_string_lossy(), "diag\n"); } #[test] fn termination_mapping_reads_the_drivers_verdict() { assert_eq!( - map_termination(Termination::TimedOut), + command_termination(Termination::TimedOut), CommandTermination::TimedOut ); assert_eq!( - map_termination(Termination::Cancelled), + command_termination(Termination::Cancelled), CommandTermination::Cancelled ); assert_eq!( - map_termination(Termination::Killed), + command_termination(Termination::Killed), CommandTermination::Cancelled ); assert_eq!( - map_termination(Termination::Exited), + command_termination(Termination::Exited), CommandTermination::Exited ); } + + #[test] + fn program_exit_code_is_the_commands_own_only_when_it_exited() { + assert_eq!(program_exit_code(Termination::Exited, Some(3)), Some(3)); + assert_eq!(program_exit_code(Termination::TimedOut, Some(143)), None); + assert_eq!(program_exit_code(Termination::Cancelled, Some(143)), None); + assert_eq!(program_exit_code(Termination::Killed, Some(137)), None); + } + + #[test] + fn into_result_reports_a_failure_under_its_label() { + let result = exec_result( + "out", + "fatal: could not read Username", + Some(128), + Termination::Exited, + 42, + ); + let error = result.into_result("git push").unwrap_err(); + let failure = error.exec_failure().expect("exec failure"); + assert_eq!(failure.label(), "git push"); + assert_eq!(failure.exit_code(), Some(128)); + assert_eq!(failure.duration(), Some(Duration::from_millis(42))); + assert!( + !error.to_string().contains("could not read Username"), + "raw output leaked into Display: {error}" + ); + + let ok = exec_result("out", "", Some(0), Termination::Exited, 1); + assert!(ok.into_result("true").is_ok()); + } + + #[test] + fn redactor_applies_to_stderr_and_stdout() { + let result = exec_result( + "stdout https://token@example.com", + "stderr https://token@example.com", + Some(1), + Termination::Exited, + 1, + ); + let error = result.into_exec_error_with_redactor("git set-url", |s| { + s.replace("https://token@example.com", "https://****@example.com") + }); + let failure = error.exec_failure().expect("exec failure"); + assert_eq!(failure.stderr(), b"stderr https://****@example.com"); + assert_eq!(failure.stdout(), b"stdout https://****@example.com"); + } + + #[test] + fn output_tail_redacts_before_truncating() { + let secret = "sk-ant-api03-xK9mZ2vL8nQ5rT1wY4bC7dF0gH3jE6pA"; + let result = exec_result( + &format!("{} {secret} done", "context ".repeat(20)), + "", + Some(1), + Termination::Exited, + 1, + ); + + let tail = result + .redacted_output_tail(32) + .expect("redacted output tail"); + let stdout = tail.stdout.expect("stdout tail"); + assert!(stdout.contains("REDACTED"), "{stdout}"); + assert!(!stdout.contains("F0gH3jE6pA"), "{stdout}"); + assert!(tail.stdout_truncated); + } + + #[test] + fn output_tail_sanitizes_terminal_control_sequences() { + let result = exec_result( + "\u{1b}[31mred\u{1b}[0m \u{1b}]0;window-title\u{7}shown \ + \u{1b}(Bset \u{1b}Mtwo-byte \u{8}backspace", + "", + Some(1), + Termination::Exited, + 1, + ); + + let tail = result + .redacted_output_tail(1024) + .expect("redacted output tail"); + let stdout = tail.stdout.expect("stdout tail"); + assert_eq!(stdout, "red shown set two-byte backspace"); + } + + #[test] + fn default_output_tail_serialized_budget_stays_below_40_kib() { + let result = exec_result( + &"o".repeat(DEFAULT_EXEC_OUTPUT_TAIL_BYTES + 128), + &"e".repeat(DEFAULT_EXEC_OUTPUT_TAIL_BYTES + 128), + Some(1), + Termination::Exited, + 1, + ); + + let tail = result.default_redacted_output_tail().expect("tail present"); + assert_eq!( + tail.stdout.as_deref().map(str::len), + Some(DEFAULT_EXEC_OUTPUT_TAIL_BYTES) + ); + assert_eq!( + tail.stderr.as_deref().map(str::len), + Some(DEFAULT_EXEC_OUTPUT_TAIL_BYTES) + ); + assert!(tail.stdout_truncated); + assert!(tail.stderr_truncated); + let serialized = serde_json::to_vec(&tail).expect("serialize tail"); + assert!( + serialized.len() < 40 * 1024, + "tail JSON was {} bytes", + serialized.len() + ); + } } diff --git a/lib/components/fabro-sandbox/src/lib.rs b/lib/components/fabro-sandbox/src/lib.rs index 22bf75e2f..2587e8574 100644 --- a/lib/components/fabro-sandbox/src/lib.rs +++ b/lib/components/fabro-sandbox/src/lib.rs @@ -39,7 +39,10 @@ pub use docker::check_docker_daemon; pub use driver::{DaytonaCredentials, ProviderAccess}; pub use driver_sandbox::{RunSandbox, local_sandbox}; pub use error::{Error, Result, default_redacted_output_tail, display_for_log}; -pub use exec::{ExplicitEnvPolicy, SandboxExec, is_sensitive_env_var}; +pub use exec::{ + DEFAULT_RETAINED_OUTPUT_BYTES, DEFAULT_STOP_GRACE, ExecResultExt, ExplicitEnvPolicy, + SandboxExec, command_termination, is_sensitive_env_var, program_exit_code, +}; pub use fabro_github::token_source::{ InstallationTokenSource, ResolvedToken, TokenProvenance, TokenSnapshot, }; @@ -61,15 +64,18 @@ pub use reconnect::{ reconnect, reconnect_driver_for_run, reconnect_for_run, reconnect_for_run_with_events, }; pub use sandbox::{ - CommandOutputCallback, DEFAULT_EXEC_OUTPUT_TAIL_BYTES, ExecResult, ExecStreamingRequest, - ExecStreamingResult, GitRunInfo, GitSetupIntent, OutputCaptureStats, PushAttempt, PushError, - PushReport, RefreshOutcome, RemoteCredentialAction, SandboxFile, SandboxWorkspaceLayout, - StderrCollector, StdioProcess, StdioProcessHandle, StdioProcessTermination, + DEFAULT_EXEC_OUTPUT_TAIL_BYTES, GitRunInfo, GitSetupIntent, PushAttempt, PushError, PushReport, + RefreshOutcome, RemoteCredentialAction, SandboxFile, SandboxWorkspaceLayout, format_lines_numbered, redacted_output_tail, setup_git, shell_quote, }; -/// Driver types a run sandbox's file and search operations speak, and the -/// network policy a [`SandboxOptions`] asks for, re-exported so consumers -/// need no direct driver dependency. -pub use sandbox_driver::{DirEntry, FileKind, GrepMatch, GrepOptions, NetworkPolicy, WalkOptions}; +/// Driver types a run sandbox speaks: what a command is and how it ended, +/// what the file and search operations return, and the network policy a +/// [`SandboxOptions`] asks for. Re-exported so consumers need no direct +/// driver dependency. +pub use sandbox_driver::{ + CaptureStats, DirEntry, ExecControls, ExecFailure, ExecResult, ExecSpec, ExecStreamingResult, + FileKind, GrepMatch, GrepOptions, NetworkPolicy, OutputSink, OutputStream, StderrTail, + StdioProcess, StdioProcessHandle, Termination, TransportError, WalkOptions, +}; pub use sandbox_spec::{ProviderSandboxSpec, SandboxSpec}; pub use terminal::{DriverTerminalSession, TerminalSession, TerminalSize, open_terminal_for_run}; diff --git a/lib/components/fabro-sandbox/src/push_credentials.rs b/lib/components/fabro-sandbox/src/push_credentials.rs index 01ad446e6..1fb4834ce 100644 --- a/lib/components/fabro-sandbox/src/push_credentials.rs +++ b/lib/components/fabro-sandbox/src/push_credentials.rs @@ -14,8 +14,10 @@ use fabro_github::GitHubCredentials; use fabro_github::token_source::{InstallationTokenSource, ResolvedToken, TokenSnapshot}; use fabro_redact::DisplaySafeUrl; pub use fabro_types::run_event::GitCredentialRefreshError as RefreshErrorKind; +use sandbox_driver::Termination; use tokio::sync::{Mutex, MutexGuard}; +use crate::exec::ExecResultExt; use crate::redact; use crate::sandbox::{RefreshOutcome, RemoteCredentialAction}; @@ -321,11 +323,10 @@ impl CredentialLease<'_> { }) } Err(err) => { - if matches!( - &err, - crate::Error::Exec { result, .. } - if result.termination != fabro_types::CommandTermination::Exited - ) { + if err + .exec_failure() + .is_some_and(|failure| failure.termination() != Termination::Exited) + { return Err(err); } tracing::warn!( @@ -377,7 +378,7 @@ pub(crate) async fn set_auth_url_via_exec( RedactedSetUrlError(message), ) })?; - if !result.is_success() { + if !result.success() { return Err(result.into_exec_error_with_redactor( "git remote set-url origin (refresh push credentials)", |s| redact::redact_auth_url(s, Some(&auth_url)), diff --git a/lib/components/fabro-sandbox/src/sandbox.rs b/lib/components/fabro-sandbox/src/sandbox.rs index 7d0dbd010..07fdf9805 100644 --- a/lib/components/fabro-sandbox/src/sandbox.rs +++ b/lib/components/fabro-sandbox/src/sandbox.rs @@ -1,24 +1,15 @@ -use std::collections::HashMap; use std::fmt::Write; -use std::future::Future; -use std::pin::Pin; -use std::sync::Arc; use std::time::Duration; -use async_trait::async_trait; use fabro_github::token_source::TokenSnapshot; pub use fabro_types::run_event::GitCredentialAction as RemoteCredentialAction; -use fabro_types::{CommandOutputStream, CommandTermination}; use fabro_util::shell; use sandbox_driver::{Git as _, GitCheckoutOptions, GitFailureKind, GitPushOptions, Termination}; use serde::{Deserialize, Serialize}; -use tokio::io::{AsyncRead, AsyncReadExt, AsyncWrite}; -use tokio::sync::Mutex as TokioMutex; -use tokio::task::JoinHandle; use tokio::time; -use tokio_util::sync::CancellationToken; use crate::driver_sandbox::RunSandbox; +use crate::exec::ExecResultExt; use crate::git_retry::{self, CredentialContext, GitRetryReason, RetryPlan}; use crate::push_credentials::{CredentialLease, PushCredentialState, RefreshErrorKind}; @@ -78,89 +69,6 @@ pub fn format_lines_numbered(content: &str, offset: Option, limit: Option result } -#[derive(Debug, Clone)] -pub struct ExecResult { - pub stdout: String, - pub stderr: String, - pub exit_code: Option, - pub termination: CommandTermination, - pub duration_ms: u64, -} - -impl ExecResult { - pub fn is_success(&self) -> bool { - self.exit_code == Some(0) && self.termination == CommandTermination::Exited - } - - pub fn is_timed_out(&self) -> bool { - self.termination == CommandTermination::TimedOut - } - - pub fn is_cancelled(&self) -> bool { - self.termination == CommandTermination::Cancelled - } - - pub fn display_exit_code(&self) -> i32 { - self.exit_code.unwrap_or(-1) - } - - pub fn into_exec_error(self, label: impl Into) -> crate::Error { - crate::Error::exec(label, self) - } - - pub fn into_exec_error_with_redactor( - self, - label: impl Into, - redactor: impl Fn(&str) -> String, - ) -> crate::Error { - crate::Error::exec(label, Self { - stdout: redactor(&self.stdout), - stderr: redactor(&self.stderr), - ..self - }) - } - - pub fn into_result(self, label: impl Into) -> crate::Result { - if self.is_success() { - Ok(self) - } else { - Err(self.into_exec_error(label)) - } - } - - pub fn redacted_output_tail( - &self, - max_bytes_per_stream: usize, - ) -> Option { - redacted_output_tail(&self.stdout, &self.stderr, max_bytes_per_stream) - } - - pub fn default_redacted_output_tail(&self) -> Option { - self.redacted_output_tail(DEFAULT_EXEC_OUTPUT_TAIL_BYTES) - } - - /// Converts host process output into the canonical full exec result. - /// - /// This stores raw stdout/stderr. Callers must not log these fields - /// directly; use `default_redacted_output_tail()` for events and - /// `display_for_log()` for tracing. - #[cfg(test)] - pub fn from_process_output(output: std::process::Output, duration_ms: u64) -> Self { - let std::process::Output { - status, - stdout, - stderr, - } = output; - Self { - stdout: String::from_utf8_lossy(&stdout).into_owned(), - stderr: String::from_utf8_lossy(&stderr).into_owned(), - exit_code: Some(status.code().unwrap_or(-1)), - termination: CommandTermination::Exited, - duration_ms, - } - } -} - /// Build a redacted `ExecOutputTail` from raw stdout/stderr without /// fabricating a synthetic `ExecResult`. Pass `""` for either stream that /// isn't relevant. Returns `None` when both streams are empty. @@ -240,235 +148,6 @@ fn sanitize_exec_output(text: &str) -> String { sanitized } -#[derive(Debug, Clone)] -pub struct ExecStreamingResult { - pub result: ExecResult, - pub streams_separated: bool, - pub live_streaming: bool, - pub stdout_capture: OutputCaptureStats, - pub stderr_capture: OutputCaptureStats, -} - -impl ExecStreamingResult { - #[must_use] - pub fn output_capture(&self) -> OutputCaptureStats { - self.stdout_capture.combine(self.stderr_capture) - } -} - -/// Byte counts for output observed and retained while draining a process. -#[derive(Debug, Clone, Copy, Default, PartialEq, Eq)] -pub struct OutputCaptureStats { - pub observed_bytes: usize, - pub retained_bytes: usize, - pub omitted_bytes: usize, -} - -impl OutputCaptureStats { - #[must_use] - pub fn complete(byte_count: usize) -> Self { - Self { - observed_bytes: byte_count, - retained_bytes: byte_count, - omitted_bytes: 0, - } - } - - #[must_use] - pub fn combine(self, other: Self) -> Self { - Self { - observed_bytes: self.observed_bytes.saturating_add(other.observed_bytes), - retained_bytes: self.retained_bytes.saturating_add(other.retained_bytes), - omitted_bytes: self.omitted_bytes.saturating_add(other.omitted_bytes), - } - } -} - -pub type CommandOutputCallback = Arc< - dyn Fn(CommandOutputStream, Vec) -> Pin> + Send>> - + Send - + Sync, ->; - -/// Inputs for a streaming command execution. -/// -/// Construct with a struct literal over [`ExecStreamingRequest::new`]: -/// `ExecStreamingRequest { stdin, ..ExecStreamingRequest::new(command) }`. -/// Providers should destructure exhaustively so a new field is a compile -/// error rather than silently ignored input. -/// -/// Standard input is owned so providers can move it into a writer task. This -/// type does not implement `Debug` because standard input can contain -/// sensitive workflow data. -pub struct ExecStreamingRequest<'a> { - pub command: &'a str, - pub timeout_ms: Option, - pub working_dir: Option<&'a str>, - pub env_vars: Option<&'a HashMap>, - pub cancel_token: Option, - pub stdin: Option>, - pub output_callback: Option, - /// Maximum bytes retained from each stream. Providers continue draining - /// stdout and stderr after the cap is reached. - pub stream_output_bytes_cap: Option, -} - -impl<'a> ExecStreamingRequest<'a> { - #[must_use] - pub fn new(command: &'a str) -> Self { - Self { - command, - timeout_ms: None, - working_dir: None, - env_vars: None, - cancel_token: None, - stdin: None, - output_callback: None, - stream_output_bytes_cap: None, - } - } -} - -pub struct StdioProcess { - pub stdin: Pin>, - pub stdout: Pin>, - pub stderr: StderrCollector, - pub handle: StdioProcessHandle, -} - -#[derive(Debug, Clone)] -pub struct StderrCollector { - inner: StderrCollectorInner, -} - -#[derive(Debug, Clone)] -enum StderrCollectorInner { - Buffer { - bytes: Arc>>, - max_bytes: usize, - }, - /// A tail the sandbox driver already keeps for a spawned process. - Driver(sandbox_driver::StderrTail), -} - -impl StderrCollector { - #[must_use] - pub fn new(max_bytes: usize) -> Self { - Self { - inner: StderrCollectorInner::Buffer { - bytes: Arc::new(TokioMutex::new(Vec::new())), - max_bytes, - }, - } - } - - /// Wraps the rolling stderr tail of a driver-spawned process. - #[must_use] - pub fn from_driver_tail(tail: sandbox_driver::StderrTail) -> Self { - Self { - inner: StderrCollectorInner::Driver(tail), - } - } - - pub async fn push(&self, bytes: &[u8]) { - match &self.inner { - StderrCollectorInner::Buffer { - bytes: buffer, - max_bytes, - } => { - let mut tail = buffer.lock().await; - tail.extend_from_slice(bytes); - if tail.len() > *max_bytes { - let excess = tail.len() - max_bytes; - tail.drain(..excess); - } - } - StderrCollectorInner::Driver(tail) => tail.push(bytes), - } - } - - pub async fn tail_string(&self) -> String { - match &self.inner { - StderrCollectorInner::Buffer { bytes, .. } => { - let tail = bytes.lock().await; - String::from_utf8_lossy(&tail).into_owned() - } - StderrCollectorInner::Driver(tail) => tail.to_string_lossy(), - } - } - - pub fn spawn_reader(&self, mut reader: R) -> JoinHandle<()> - where - R: AsyncRead + Unpin + Send + 'static, - { - let collector = self.clone(); - tokio::spawn(async move { - let mut buf = [0_u8; 8192]; - loop { - match reader.read(&mut buf).await { - Ok(0) => return, - Ok(read) => collector.push(&buf[..read]).await, - Err(err) => { - tracing::warn!(error = %err, "Failed to read stdio process stderr"); - return; - } - } - } - }) - } -} - -#[derive(Clone)] -pub struct StdioProcessHandle { - control: Arc, -} - -impl StdioProcessHandle { - pub(crate) fn new(control: impl StdioProcessControl + 'static) -> Self { - Self { - control: Arc::new(control), - } - } - - pub async fn terminate(&self) -> crate::Result<()> { - self.control.terminate().await - } - - pub async fn wait(&self) -> crate::Result { - self.control.wait().await - } -} - -#[derive(Debug, Clone, Copy, PartialEq, Eq)] -pub struct StdioProcessTermination { - pub termination: CommandTermination, - pub exit_code: Option, -} - -impl StdioProcessTermination { - #[must_use] - pub fn exited(exit_code: Option) -> Self { - Self { - termination: CommandTermination::Exited, - exit_code, - } - } - - #[must_use] - pub fn cancelled() -> Self { - Self { - termination: CommandTermination::Cancelled, - exit_code: None, - } - } -} - -#[async_trait] -pub(crate) trait StdioProcessControl: Send + Sync { - async fn terminate(&self) -> crate::Result<()>; - async fn wait(&self) -> crate::Result; -} - /// A regular file discovered inside a sandbox. #[derive(Debug, Clone, PartialEq, Eq)] pub struct SandboxFile { @@ -629,11 +308,11 @@ pub(crate) async fn fetch_source_run_ref( let fetch = sandbox .exec_command(&fetch_cmd, 30_000, None, None, None) .await?; - if fetch.is_success() { + if fetch.success() { let check = sandbox .exec_command(&check_cmd, 10_000, None, None, None) .await?; - if check.is_success() { + if check.success() { return Ok(()); } last_error = check @@ -913,14 +592,16 @@ fn push_deadline_error(attempts: Vec, stage: &str) -> PushError { #[cfg(test)] mod push_tests { use std::collections::VecDeque; - use std::sync::Mutex; use std::sync::atomic::{AtomicUsize, Ordering}; + use std::sync::{Arc, Mutex}; + use async_trait::async_trait; use chrono::Utc; use fabro_github::InstallationToken; use fabro_github::test_support::{InstallationTokenMinter, installation_token_source}; use fabro_github::token_source::{InstallationTokenSource, REFRESH_MARGIN}; use fabro_types::SandboxProviderKind; + use sandbox_driver::ExecResult; use sandbox_driver_testing::ScriptedSandbox; use tokio::sync::Mutex as AsyncMutex; @@ -931,34 +612,20 @@ mod push_tests { const ORIGIN: &str = "https://github.com/fabro-testing/repo"; const REFSPEC: &str = "refs/heads/fabro/run/01M0DH033P2XSTHAGVBHG6922F"; - fn ok_fabro_exec() -> ExecResult { - ExecResult { - stdout: String::new(), - stderr: String::new(), - exit_code: Some(0), - termination: CommandTermination::Exited, - duration_ms: 5, - } + fn ok_exec() -> ExecResult { + ExecResult::new(Termination::Exited, Some(0), Duration::from_millis(5)) } fn failed_exec(stderr: &str) -> ExecResult { - ExecResult { - stdout: String::new(), - stderr: stderr.to_string(), - exit_code: Some(128), - termination: CommandTermination::Exited, - duration_ms: 5, - } + let mut result = ExecResult::new(Termination::Exited, Some(128), Duration::from_millis(5)); + result.stderr = stderr.as_bytes().to_vec(); + result } fn timed_out_exec() -> ExecResult { - ExecResult { - stdout: String::new(), - stderr: "Command timed out".to_string(), - exit_code: None, - termination: CommandTermination::TimedOut, - duration_ms: 60_000, - } + let mut result = ExecResult::new(Termination::TimedOut, None, Duration::from_mins(1)); + result.stderr = b"Command timed out".to_vec(); + result } /// A run sandbox over a scripted driver double: `git push` answers come @@ -987,25 +654,19 @@ mod push_tests { driver.scripted_exec().respond_with(move |spec| { let script = spec.args.last().map(String::as_str).unwrap_or_default(); if script.contains("remote set-url") { - return Some( - set_urls - .lock() - .unwrap() - .pop_front() - .map_or_else(ok_exec, driver_result), - ); + return Some(set_urls.lock().unwrap().pop_front().unwrap_or_else(ok_exec)); } assert!( script.contains("'push' 'origin'"), "unexpected exec: {script}" ); - Some(driver_result( + Some( pushes .lock() .unwrap() .pop_front() .expect("push script exhausted"), - )) + ) }); let run = RunSandbox::new(SandboxProviderKind::LOCAL, Arc::clone(&driver) as _); Self { run, driver } @@ -1030,28 +691,6 @@ mod push_tests { } } - /// The driver-level result fabro's exec policy reads back as the fabro - /// result the push tests script. - fn driver_result(result: ExecResult) -> sandbox_driver::ExecResult { - let termination = match result.termination { - CommandTermination::Exited => sandbox_driver::Termination::Exited, - CommandTermination::TimedOut => sandbox_driver::Termination::TimedOut, - CommandTermination::Cancelled => sandbox_driver::Termination::Cancelled, - }; - let mut driver = sandbox_driver::ExecResult::new( - termination, - result.exit_code, - Duration::from_millis(result.duration_ms), - ); - driver.stdout = result.stdout.into_bytes(); - driver.stderr = result.stderr.into_bytes(); - driver - } - - fn ok_exec() -> sandbox_driver::ExecResult { - driver_result(ok_fabro_exec()) - } - enum MintAction { Token(&'static str, chrono::Duration), Error(&'static str), @@ -1137,7 +776,7 @@ mod push_tests { let sandbox = ScriptedGitSandbox::new(vec![ failed_exec("remote: Repository not found."), failed_exec("remote: Repository not found."), - ok_fabro_exec(), + ok_exec(), ]); let report = git_push( @@ -1182,7 +821,7 @@ mod push_tests { failed_exec("remote: Repository not found."), failed_exec("remote: Repository not found."), failed_exec("remote: Repository not found."), - ok_fabro_exec(), + ok_exec(), ]); let report = git_push( @@ -1213,7 +852,7 @@ mod push_tests { let sandbox = ScriptedGitSandbox::new(vec![ failed_exec("remote: Repository not found."), failed_exec("remote: Repository not found."), - ok_fabro_exec(), + ok_exec(), ]); let report = git_push( @@ -1274,7 +913,7 @@ mod push_tests { MintAction::Error("mint failed"), ]); seed_clone_token(&state).await; - let sandbox = ScriptedGitSandbox::new(vec![ok_fabro_exec()]); + let sandbox = ScriptedGitSandbox::new(vec![ok_exec()]); let report = git_push( &sandbox.run, @@ -1332,7 +971,7 @@ mod push_tests { seed_clone_token(&state).await; let sandbox = ScriptedGitSandbox::new(vec![ failed_exec("fatal: Authentication failed for 'https://github.com'"), - ok_fabro_exec(), + ok_exec(), ]); let report = git_push( @@ -1372,7 +1011,7 @@ mod push_tests { let sandbox = ScriptedGitSandbox::with_set_url_results( vec![ failed_exec("error: RPC failed; connection reset by peer"), - ok_fabro_exec(), + ok_exec(), ], vec![failed_exec("error: could not lock config file")], ); @@ -1448,7 +1087,7 @@ mod push_tests { failed_exec( "fatal: could not read Username for 'https://github.com': No such device or address\nremote: Repository not found.", ), - ok_fabro_exec(), + ok_exec(), ]); let report = git_push( @@ -1478,7 +1117,7 @@ mod push_tests { #[tokio::test(start_paused = true)] async fn push_without_managed_credentials_reports_no_token() { - let sandbox = ScriptedGitSandbox::new(vec![ok_fabro_exec()]); + let sandbox = ScriptedGitSandbox::new(vec![ok_exec()]); let report = git_push(&sandbox.run, None, REFSPEC, &RetryPlan::checkpoint_push()) .await @@ -1552,189 +1191,6 @@ mod push_tests { mod tests { use super::*; - #[test] - fn exec_result_fields() { - let result = ExecResult { - stdout: "out".into(), - stderr: "err".into(), - exit_code: Some(1), - termination: CommandTermination::Exited, - duration_ms: 5000, - }; - assert_eq!(result.exit_code, Some(1)); - assert_eq!(result.termination, CommandTermination::Exited); - assert_eq!(result.duration_ms, 5000); - } - - #[test] - fn exec_result_helpers_convert_failure_to_exec_error() { - let result = ExecResult { - stdout: "out".into(), - stderr: "fatal: could not read Username".into(), - exit_code: Some(128), - termination: CommandTermination::Exited, - duration_ms: 42, - }; - let error = result.into_result("git push").unwrap_err(); - let crate::Error::Exec { label, result, .. } = &error else { - panic!("expected Error::Exec, got {error:?}"); - }; - assert_eq!(label, "git push"); - assert_eq!(result.exit_code, Some(128)); - assert!(error.to_string().contains("no credentials in origin URL")); - } - - #[test] - fn exec_result_success_honors_timeouts() { - let success = ExecResult { - stdout: String::new(), - stderr: String::new(), - exit_code: Some(0), - termination: CommandTermination::Exited, - duration_ms: 1, - }; - assert!(success.is_success()); - - let timeout = ExecResult { - exit_code: None, - termination: CommandTermination::TimedOut, - ..success - }; - assert!(!timeout.is_success()); - } - - #[test] - fn exec_result_redactor_applies_to_stderr_and_stdout() { - let result = ExecResult { - stdout: "stdout https://token@example.com".into(), - stderr: "stderr https://token@example.com".into(), - exit_code: Some(1), - termination: CommandTermination::Exited, - duration_ms: 1, - }; - let error = result.into_exec_error_with_redactor("git set-url", |s| { - s.replace("https://token@example.com", "https://****@example.com") - }); - - let crate::Error::Exec { result, .. } = &error else { - panic!("expected Error::Exec, got {error:?}"); - }; - assert_eq!(result.stderr, "stderr https://****@example.com"); - assert_eq!(result.stdout, "stdout https://****@example.com"); - } - - #[test] - fn exec_result_redacts_before_taking_tail() { - let secret = "sk-ant-api03-xK9mZ2vL8nQ5rT1wY4bC7dF0gH3jE6pA"; - let result = ExecResult { - stdout: format!("{} {secret} done", "context ".repeat(20)), - stderr: String::new(), - exit_code: Some(1), - termination: CommandTermination::Exited, - duration_ms: 1, - }; - - let tail = result - .redacted_output_tail(32) - .expect("redacted output tail"); - let stdout = tail.stdout.expect("stdout tail"); - assert!(stdout.contains("REDACTED"), "{stdout}"); - assert!(!stdout.contains("F0gH3jE6pA"), "{stdout}"); - assert!(tail.stdout_truncated); - } - - #[test] - fn exec_result_tail_sanitizes_terminal_control_sequences() { - let result = ExecResult { - stdout: "\u{1b}[31mred\u{1b}[0m \u{1b}]0;window-title\u{7}shown \ - \u{1b}(Bset \u{1b}Mtwo-byte \u{8}backspace" - .to_string(), - stderr: String::new(), - exit_code: Some(1), - termination: CommandTermination::Exited, - duration_ms: 1, - }; - - let tail = result - .redacted_output_tail(1024) - .expect("redacted output tail"); - let stdout = tail.stdout.expect("stdout tail"); - assert_eq!(stdout, "red shown set two-byte backspace"); - } - - #[cfg(unix)] - #[test] - #[expect( - clippy::disallowed_methods, - reason = "test intentionally creates host process output for conversion coverage" - )] - fn from_process_output_uses_minus_one_for_signal_exit_without_code() { - let output = std::process::Command::new("sh") - .arg("-c") - .arg("printf out; printf err >&2; kill -9 $$") - .output() - .expect("signal-killed process output"); - - let result = ExecResult::from_process_output(output, 12); - - assert_eq!(result.stdout, "out"); - assert_eq!(result.stderr, "err"); - assert_eq!(result.exit_code, Some(-1)); - assert_eq!(result.termination, CommandTermination::Exited); - assert_eq!(result.duration_ms, 12); - } - - #[cfg(unix)] - #[test] - #[expect( - clippy::disallowed_methods, - reason = "test intentionally creates host process output for conversion coverage" - )] - fn from_process_output_handles_lossy_non_utf8_output() { - let output = std::process::Command::new("sh") - .arg("-c") - .arg("printf '\\377'; printf '\\376' >&2") - .output() - .expect("non-utf8 process output"); - - let result = ExecResult::from_process_output(output, 3); - let tail = result - .redacted_output_tail(16) - .expect("redacted output tail"); - - assert!(tail.stdout.expect("stdout tail").len() <= 16); - assert!(tail.stderr.expect("stderr tail").len() <= 16); - } - - #[test] - fn default_exec_output_tail_serialized_budget_stays_below_40_kib() { - let result = ExecResult { - stdout: "o".repeat(DEFAULT_EXEC_OUTPUT_TAIL_BYTES + 128), - stderr: "e".repeat(DEFAULT_EXEC_OUTPUT_TAIL_BYTES + 128), - exit_code: Some(1), - termination: CommandTermination::Exited, - duration_ms: 1, - }; - - let tail = result.default_redacted_output_tail().expect("tail present"); - assert_eq!( - tail.stdout.as_deref().map(str::len), - Some(DEFAULT_EXEC_OUTPUT_TAIL_BYTES) - ); - assert_eq!( - tail.stderr.as_deref().map(str::len), - Some(DEFAULT_EXEC_OUTPUT_TAIL_BYTES) - ); - assert!(tail.stdout_truncated); - assert!(tail.stderr_truncated); - let serialized = serde_json::to_vec(&tail).expect("serialize tail"); - assert!( - serialized.len() < 40 * 1024, - "tail JSON was {} bytes", - serialized.len() - ); - } - #[test] fn sandbox_tracing_events_do_not_log_raw_command_or_stdin_fields() { let root = std::path::Path::new(env!("CARGO_MANIFEST_DIR")).join("src"); diff --git a/lib/components/fabro-sandbox/src/test_support.rs b/lib/components/fabro-sandbox/src/test_support.rs index 9e532d807..734aa9711 100644 --- a/lib/components/fabro-sandbox/src/test_support.rs +++ b/lib/components/fabro-sandbox/src/test_support.rs @@ -11,13 +11,31 @@ use std::collections::HashMap; use std::sync::{Arc, OnceLock}; use std::time::Duration; -use fabro_types::{CommandTermination, SandboxProviderKind}; -use sandbox_driver::{GrepMatch, PlatformInfo, SandboxState, Termination, WalkedFile}; +use fabro_types::SandboxProviderKind; +use sandbox_driver::{ + ExecResult, GrepMatch, PlatformInfo, SandboxState, StderrTail, Termination, WalkedFile, +}; pub use sandbox_driver_testing::{ScriptedExec, ScriptedSandbox, ScriptedStdioProcess}; use tokio::io::DuplexStream; use crate::driver_sandbox::RunSandbox; -use crate::sandbox::{ExecResult, SandboxFile, StderrCollector}; +use crate::sandbox::SandboxFile; + +/// A driver [`ExecResult`] with the given streams, for scripting a mock +/// sandbox's answers. +#[must_use] +pub fn exec_result( + stdout: &str, + stderr: &str, + exit_code: Option, + termination: Termination, + duration_ms: u64, +) -> ExecResult { + let mut result = ExecResult::new(termination, exit_code, Duration::from_millis(duration_ms)); + result.stdout = stdout.as_bytes().to_vec(); + result.stderr = stderr.as_bytes().to_vec(); + result +} // --- MockSandbox --- @@ -71,12 +89,11 @@ impl Default for MockSandbox { fn default() -> Self { Self { files: HashMap::new(), - exec_result: ExecResult { - stdout: "mock output".into(), - stderr: String::new(), - exit_code: Some(0), - termination: CommandTermination::Exited, - duration_ms: 10, + exec_result: { + let mut result = + ExecResult::new(Termination::Exited, Some(0), Duration::from_millis(10)); + result.stdout = b"mock output".to_vec(); + result }, exec_error: None, working_dir: "/work", @@ -145,17 +162,15 @@ impl MockSandbox { ) -> &Self { self.driver().scripted_exec().respond_with(move |spec| { let command = spec.args.last().map(String::as_str).unwrap_or_default(); - responder(command).map(|result| driver_result(&result)) + responder(command) }); self } /// Queues the result for the next command, ahead of `exec_result`. /// Results answer in the order they were pushed. - pub fn push_exec_result(&self, result: &ExecResult) -> &Self { - self.driver() - .scripted_exec() - .push_result(driver_result(result)); + pub fn push_exec_result(&self, result: ExecResult) -> &Self { + self.driver().scripted_exec().push_result(result); self } @@ -197,7 +212,7 @@ impl MockSandbox { let exec = driver.scripted_exec(); match &self.exec_error { Some(message) => exec.fail_by_default(message.clone()), - None => exec.set_default(driver_result(&self.exec_result)), + None => exec.set_default(self.exec_result.clone()), }; exec.set_streams_separated(self.streams_separated); if let Some(message) = &self.stdio_process_error { @@ -363,43 +378,22 @@ impl MockSandbox { } } -/// The driver result fabro's exec policy reads back as `result`. -fn driver_result(result: &ExecResult) -> sandbox_driver::ExecResult { - let termination = match result.termination { - CommandTermination::Exited => Termination::Exited, - CommandTermination::TimedOut => Termination::TimedOut, - CommandTermination::Cancelled => Termination::Cancelled, - }; - let mut driver = sandbox_driver::ExecResult::new( - termination, - result.exit_code, - Duration::from_millis(result.duration_ms), - ); - driver.stdout = result.stdout.clone().into_bytes(); - driver.stderr = result.stderr.clone().into_bytes(); - driver -} - // --- MockStdioProcess --- /// A stdio process a test drives, over the driver's scripted process. /// /// The driver closure receives the process's end of standard input, its -/// end of standard output, and fabro's stderr collector for the process. +/// end of standard output, and the rolling stderr tail the process reports. pub struct MockStdioProcess { inner: std::sync::Mutex>, } impl MockStdioProcess { pub fn new( - driver: impl FnOnce(DuplexStream, DuplexStream, StderrCollector) + Send + 'static, + driver: impl FnOnce(DuplexStream, DuplexStream, StderrTail) + Send + 'static, ) -> Self { Self { - inner: std::sync::Mutex::new(Some(ScriptedStdioProcess::new( - move |stdin, stdout, tail| { - driver(stdin, stdout, StderrCollector::from_driver_tail(tail)); - }, - ))), + inner: std::sync::Mutex::new(Some(ScriptedStdioProcess::new(driver))), } } diff --git a/lib/components/fabro-sandbox/tests/daytona_streaming_live.rs b/lib/components/fabro-sandbox/tests/daytona_streaming_live.rs index 4ac6193c2..d5dbfd1c5 100644 --- a/lib/components/fabro-sandbox/tests/daytona_streaming_live.rs +++ b/lib/components/fabro-sandbox/tests/daytona_streaming_live.rs @@ -4,18 +4,18 @@ mod daytona_streaming_live { use anyhow::{Context, Result, ensure}; use fabro_sandbox::{ - CommandOutputCallback, DaytonaCredentials, ExecStreamingResult, ProviderAccess, RunSandbox, - SandboxOptions, SandboxProviderKind, provider_sandbox, + DaytonaCredentials, ExecControls, ExecSpec, ExecStreamingResult, OutputSink, OutputStream, + ProviderAccess, RunSandbox, SandboxOptions, SandboxProviderKind, Termination, + provider_sandbox, }; use fabro_static::EnvVars; - use fabro_types::{CommandOutputStream, CommandTermination}; use tokio::sync::Mutex; use tokio::time::{Instant, sleep}; use tokio_util::sync::CancellationToken; #[derive(Debug, Clone)] struct CapturedChunk { - stream: CommandOutputStream, + stream: OutputStream, text: String, } @@ -100,7 +100,7 @@ mod daytona_streaming_live { &format!("exec_command should run Bash-only syntax: {non_streaming:?}"), )?; ensure_contains( - &non_streaming.stdout, + &non_streaming.stdout_lossy(), "two", "exec_command should report the Bash-only result", )?; @@ -112,7 +112,7 @@ mod daytona_streaming_live { &format!("exec_command_streaming should run Bash-only syntax: {streaming:?}"), )?; ensure_contains( - &streaming.result.stdout, + &streaming.result.stdout_lossy(), "two", "exec_command_streaming should report the Bash-only result", )?; @@ -133,7 +133,7 @@ mod daytona_streaming_live { )?; ensure_eq( &stdin_result.result.stdout, - &stdin.to_string(), + &stdin.as_bytes().to_vec(), "exec_command_streaming should preserve exact stdin bytes", )?; let stdin_cleanup = sandbox @@ -147,7 +147,7 @@ mod daytona_streaming_live { ) .await?; ensure!( - stdin_cleanup.is_success(), + stdin_cleanup.success(), "Daytona stdin data must stay inert and its temporary file must be deleted: {stdin_cleanup:?}" ); @@ -272,13 +272,13 @@ mod daytona_streaming_live { let cleanup_result = sandbox.cleanup().await.context("clean up Daytona sandbox"); ensure!( - result.is_success(), + result.success(), "layout verification failed: stdout={} stderr={}", - result.stdout, - result.stderr + result.stdout_lossy(), + result.stderr_lossy() ); ensure_contains( - &result.stdout, + &result.stdout_lossy(), "true", "default cwd should be inside the work tree", )?; @@ -343,10 +343,10 @@ mod daytona_streaming_live { ) .await?; ensure!( - seed.is_success(), + seed.success(), "seeding the skills tree failed: stdout={} stderr={}", - seed.stdout, - seed.stderr + seed.stdout_lossy(), + seed.stderr_lossy() ); // `*/SKILL.md` matches exactly one path segment: only the file one level @@ -383,21 +383,22 @@ mod daytona_streaming_live { let live_exec = tokio::spawn(async move { sandbox_for_exec - .exec_command_streaming(fabro_sandbox::ExecStreamingRequest { - timeout_ms: Some(60_000), - cancel_token: Some(cancel_for_exec), - output_callback: Some(callback), - ..fabro_sandbox::ExecStreamingRequest::new( - "printf 'live-out\\n'; printf 'live-err\\n' >&2; sleep 30", - ) - }) + .exec_command_streaming( + ExecSpec::bash("printf 'live-out\\n'; printf 'live-err\\n' >&2; sleep 30") + .timeout(Duration::from_mins(1)), + ExecControls { + term: Some(cancel_for_exec), + sink: Some(callback), + ..ExecControls::default() + }, + ) .await }); let saw_live_stdout_and_stderr = wait_for_chunks(&chunks, Duration::from_secs(20), |chunks| { - contains_chunk(chunks, CommandOutputStream::Stdout, "live-out") - && contains_chunk(chunks, CommandOutputStream::Stderr, "live-err") + contains_chunk(chunks, OutputStream::Stdout, "live-out") + && contains_chunk(chunks, OutputStream::Stderr, "live-err") }) .await; @@ -423,16 +424,16 @@ mod daytona_streaming_live { ); ensure_eq( &live_result.result.termination, - &CommandTermination::Cancelled, + &Termination::Cancelled, "cancelled command should preserve cancellation termination", )?; ensure_contains( - &live_result.result.stdout, + &live_result.result.stdout_lossy(), "live-out", "cancelled command stdout should preserve partial logs", )?; ensure_contains( - &live_result.result.stderr, + &live_result.result.stderr_lossy(), "live-err", "cancelled command stderr should preserve partial logs", )?; @@ -451,25 +452,25 @@ mod daytona_streaming_live { )?; ensure_eq( &nonzero.result.termination, - &CommandTermination::Exited, + &Termination::Exited, "nonzero command should be represented as a completed process", )?; ensure_contains( - &nonzero.result.stdout, + &nonzero.result.stdout_lossy(), "exit-out", "nonzero command stdout should be captured", )?; ensure_contains( - &nonzero.result.stderr, + &nonzero.result.stderr_lossy(), "exit-err", "nonzero command stderr should be captured", )?; ensure!( - contains_chunk(&nonzero_chunks, CommandOutputStream::Stdout, "exit-out"), + contains_chunk(&nonzero_chunks, OutputStream::Stdout, "exit-out"), "nonzero command should stream stdout chunks" ); ensure!( - contains_chunk(&nonzero_chunks, CommandOutputStream::Stderr, "exit-err"), + contains_chunk(&nonzero_chunks, OutputStream::Stderr, "exit-err"), "nonzero command should stream stderr chunks" ); @@ -482,16 +483,16 @@ mod daytona_streaming_live { .await?; ensure_eq( &timed_out.result.termination, - &CommandTermination::TimedOut, + &Termination::TimedOut, "timed-out command should preserve timeout termination", )?; ensure_contains( - &timed_out.result.stdout, + &timed_out.result.stdout_lossy(), "timeout-out", "timed-out command stdout should preserve partial logs", )?; ensure_contains( - &timed_out.result.stderr, + &timed_out.result.stderr_lossy(), "timeout-err", "timed-out command stderr should preserve partial logs", )?; @@ -517,13 +518,15 @@ mod daytona_streaming_live { ) -> Result<(ExecStreamingResult, Vec)> { let chunks = Arc::new(Mutex::new(Vec::new())); let callback = capture_callback(Arc::clone(&chunks)); + let mut spec = ExecSpec::bash(command).timeout(Duration::from_millis(timeout_ms)); + if let Some(stdin) = stdin { + spec = spec.stdin(stdin); + } let result = sandbox - .exec_command_streaming(fabro_sandbox::ExecStreamingRequest { - timeout_ms: Some(timeout_ms), - cancel_token, - stdin, - output_callback: Some(callback), - ..fabro_sandbox::ExecStreamingRequest::new(command) + .exec_command_streaming(spec, ExecControls { + term: cancel_token, + sink: Some(callback), + ..ExecControls::default() }) .await?; let chunks = chunks.lock().await.clone(); @@ -531,7 +534,7 @@ mod daytona_streaming_live { Ok((result, chunks)) } - fn capture_callback(chunks: Arc>>) -> CommandOutputCallback { + fn capture_callback(chunks: Arc>>) -> OutputSink { Arc::new(move |stream, bytes| { let chunks = Arc::clone(&chunks); Box::pin(async move { @@ -597,7 +600,7 @@ mod daytona_streaming_live { } } - fn contains_chunk(chunks: &[CapturedChunk], stream: CommandOutputStream, text: &str) -> bool { + fn contains_chunk(chunks: &[CapturedChunk], stream: OutputStream, text: &str) -> bool { chunks .iter() .any(|chunk| chunk.stream == stream && chunk.text.contains(text)) diff --git a/lib/components/fabro-sandbox/tests/docker_streaming.rs b/lib/components/fabro-sandbox/tests/docker_streaming.rs index 07b0e082a..b99597c7e 100644 --- a/lib/components/fabro-sandbox/tests/docker_streaming.rs +++ b/lib/components/fabro-sandbox/tests/docker_streaming.rs @@ -2,10 +2,11 @@ use std::collections::BTreeMap; use std::sync::Arc; +use std::time::Duration; use fabro_sandbox::{ - CommandOutputCallback, ExecStreamingRequest, ProviderAccess, SandboxOptions, - SandboxProviderKind, provider_sandbox, + ExecControls, ExecSpec, OutputSink, ProviderAccess, SandboxOptions, SandboxProviderKind, + Termination, provider_sandbox, }; use tokio::process::Command; use tokio::sync::Mutex; @@ -22,7 +23,7 @@ async fn docker_image_available(image: &str) -> bool { .is_ok_and(|status| status.success()) } -fn capture_bytes(chunks: Arc>>) -> CommandOutputCallback { +fn capture_bytes(chunks: Arc>>) -> OutputSink { Arc::new(move |_stream, bytes| { let chunks = Arc::clone(&chunks); Box::pin(async move { @@ -67,15 +68,17 @@ async fn streaming_timeout_terminates_docker_exec_before_returning() { let marker = "fabro_streaming_timeout_sentinel"; let command = format!("trap '' HUP TERM; echo start; sleep 5 # {marker}"); let result = sandbox - .exec_command_streaming(ExecStreamingRequest { - timeout_ms: Some(200), - output_callback: Some(capture_bytes(Arc::clone(&chunks))), - ..ExecStreamingRequest::new(&command) - }) + .exec_command_streaming( + ExecSpec::bash(&command).timeout(Duration::from_millis(200)), + ExecControls { + sink: Some(capture_bytes(Arc::clone(&chunks))), + ..ExecControls::default() + }, + ) .await .expect("streaming command should return a timeout result"); - assert!(result.result.is_timed_out()); + assert_eq!(result.result.termination, Termination::TimedOut); assert!( String::from_utf8_lossy(&chunks.lock().await).contains("start"), "stream should include output emitted before timeout" @@ -98,10 +101,10 @@ async fn streaming_timeout_terminates_docker_exec_before_returning() { .await .expect("docker cleanup should succeed"); + let probe = probe.stdout_lossy(); assert!( - !probe.stdout.contains(marker), - "timed-out docker exec should be terminated before returning, found: {}", - probe.stdout + !probe.contains(marker), + "timed-out docker exec should be terminated before returning, found: {probe}" ); } @@ -137,11 +140,12 @@ async fn streaming_command_receives_exact_stdin_and_eof() { let stdin = b"first line\n$(touch /tmp/must-not-run)\nlast line".to_vec(); let result = sandbox - .exec_command_streaming(ExecStreamingRequest { - timeout_ms: Some(10_000), - stdin: Some(stdin.clone()), - ..ExecStreamingRequest::new("cat") - }) + .exec_command_streaming( + ExecSpec::bash("cat") + .timeout(Duration::from_secs(10)) + .stdin(stdin.clone()), + ExecControls::default(), + ) .await .expect("streaming command should read stdin and finish at EOF"); let injection_probe = sandbox @@ -155,14 +159,14 @@ async fn streaming_command_receives_exact_stdin_and_eof() { .expect("docker cleanup should succeed"); assert!( - result.result.is_success(), + result.result.success(), "stdin command failed: stdout={} stderr={}", - result.result.stdout, - result.result.stderr + result.result.stdout_lossy(), + result.result.stderr_lossy() ); - assert_eq!(result.result.stdout.as_bytes(), stdin); + assert_eq!(result.result.stdout, stdin); assert!( - injection_probe.is_success(), + injection_probe.success(), "stdin bytes must not be evaluated as shell source" ); } @@ -220,12 +224,12 @@ async fn cloned_docker_sandbox_uses_repos_checkout_and_workspace_symlink() { .expect("docker cleanup should succeed"); assert!( - result.is_success(), + result.success(), "layout verification failed: stdout={} stderr={}", - result.stdout, - result.stderr + result.stdout_lossy(), + result.stderr_lossy() ); - assert!(result.stdout.contains("true")); + assert!(result.stdout_lossy().contains("true")); } // Both command paths must evaluate the same interpreter, so Bash-only syntax @@ -276,7 +280,7 @@ async fn docker_runs_clean_bash_through_both_command_paths() { ) .await .expect("startup-file fixture should be created"); - assert!(setup.is_success()); + assert!(setup.success()); // Arrays, `[[ ]]`, and `${arr[@]}` are Bash-only; `shopt -q login_shell` // proves the command did not run under a login shell. Exact output also @@ -291,11 +295,13 @@ async fn docker_runs_clean_bash_through_both_command_paths() { let chunks = Arc::new(Mutex::new(Vec::new())); let streaming = sandbox - .exec_command_streaming(ExecStreamingRequest { - timeout_ms: Some(10_000), - output_callback: Some(capture_bytes(Arc::clone(&chunks))), - ..ExecStreamingRequest::new(command) - }) + .exec_command_streaming( + ExecSpec::bash(command).timeout(Duration::from_secs(10)), + ExecControls { + sink: Some(capture_bytes(Arc::clone(&chunks))), + ..ExecControls::default() + }, + ) .await .expect("streaming command should run"); @@ -305,19 +311,19 @@ async fn docker_runs_clean_bash_through_both_command_paths() { .expect("docker cleanup should succeed"); assert!( - non_streaming.is_success(), + non_streaming.success(), "non-streaming Bash-only command failed: stdout={} stderr={}", - non_streaming.stdout, - non_streaming.stderr + non_streaming.stdout_lossy(), + non_streaming.stderr_lossy() ); - assert_eq!(non_streaming.stdout.trim(), "two"); + assert_eq!(non_streaming.stdout_lossy().trim(), "two"); assert!( - streaming.result.is_success(), + streaming.result.success(), "streaming Bash-only command failed: stdout={} stderr={}", - streaming.result.stdout, - streaming.result.stderr + streaming.result.stdout_lossy(), + streaming.result.stderr_lossy() ); - assert_eq!(streaming.result.stdout.trim(), "two"); + assert_eq!(streaming.result.stdout_lossy().trim(), "two"); assert_eq!(String::from_utf8_lossy(&chunks.lock().await).trim(), "two"); } @@ -384,10 +390,10 @@ async fn docker_glob_matches_patterns_containing_a_path_separator() { .expect("docker cleanup should succeed"); assert!( - seed.is_success(), + seed.success(), "seeding the skills tree failed: stdout={} stderr={}", - seed.stdout, - seed.stderr + seed.stdout_lossy(), + seed.stderr_lossy() ); let one_level = one_level.expect("glob should run"); @@ -475,8 +481,9 @@ async fn docker_runtime_directory_is_private_and_outside_workspace() { .await .expect("docker cleanup should succeed"); - assert!(modes.is_success(), "stat failed: {}", modes.stderr); - let modes: Vec<&str> = modes.stdout.split_whitespace().collect(); + assert!(modes.success(), "stat failed: {}", modes.stderr_lossy()); + let modes = modes.stdout_lossy(); + let modes: Vec<&str> = modes.split_whitespace().collect(); assert_eq!( modes, ["700", "600"], diff --git a/lib/components/fabro-sandbox/tests/driver_bench.rs b/lib/components/fabro-sandbox/tests/driver_bench.rs index 5f9646319..961db5b7d 100644 --- a/lib/components/fabro-sandbox/tests/driver_bench.rs +++ b/lib/components/fabro-sandbox/tests/driver_bench.rs @@ -279,7 +279,7 @@ async fn unpack_fabro(sandbox: &RunSandbox, repo: &Repository) { ) .await .expect("unpack exec"); - assert!(result.is_success(), "unpack failed: {}", result.stderr); + assert!(result.success(), "unpack failed: {}", result.stderr_lossy()); } async fn unpack_driver(sandbox: &dyn DriverHandle, repo: &Repository) { diff --git a/lib/components/fabro-workflow/src/handler/command.rs b/lib/components/fabro-workflow/src/handler/command.rs index 8d024ece9..59fa28f9a 100644 --- a/lib/components/fabro-workflow/src/handler/command.rs +++ b/lib/components/fabro-workflow/src/handler/command.rs @@ -1,9 +1,12 @@ use std::path::Path; use async_trait::async_trait; -use fabro_agent::{CommandOutputCallback, ExecStreamingRequest}; use fabro_graphviz::graph::{ContextKeyAttr, Graph, Node}; -use fabro_types::{CommandTermination, StageTiming}; +use fabro_sandbox::{ + ExecControls, ExecResultExt, ExecSpec, OutputSink, Termination, TransportError, + command_termination, +}; +use fabro_types::StageTiming; use fabro_util::shell::shell_quote; use super::structured_output::{self, StructuredOutputError}; @@ -108,7 +111,7 @@ impl Handler for CommandHandler { let cancel_token = services.run.cancel_token().child_token(); let stage_id = stage_scope.stage_id(); let recorder = CommandLogRecorder::create(run_dir, &stage_id).await?; - let output_callback: CommandOutputCallback = { + let sink: OutputSink = { let recorder = recorder.clone(); std::sync::Arc::new(move |_stream, bytes| { let recorder = recorder.clone(); @@ -116,21 +119,26 @@ impl Handler for CommandHandler { recorder .append(&bytes) .await - .map_err(|err| fabro_sandbox::Error::message(err.to_string())) + .map_err(|err| TransportError::new(err.to_string()).into()) }) }) }; + let mut spec = + ExecSpec::bash(&command).timeout(std::time::Duration::from_millis(timeout_ms)); + for (key, value) in env_vars.into_iter().flatten() { + spec = spec.env_var(key, value); + } + if let Some(stdin) = stdin { + spec = spec.stdin(stdin); + } let result = services .run .sandbox - .exec_command_streaming(ExecStreamingRequest { - timeout_ms: Some(timeout_ms), - env_vars, - cancel_token: Some(cancel_token.clone()), - stdin, - output_callback: Some(output_callback), - ..ExecStreamingRequest::new(&command) + .exec_command_streaming(spec, ExecControls { + term: Some(cancel_token.clone()), + sink: Some(sink), + ..ExecControls::default() }) .await; cancel_token.cancel(); @@ -148,28 +156,31 @@ impl Handler for CommandHandler { &Event::CommandCompleted { node_id: node.id.clone(), output: finalized.output_ref.clone(), - exit_code: result.exit_code, - duration_ms: result.duration_ms, - termination: result.termination, + exit_code: result.program_exit_code(), + duration_ms: result.duration_ms(), + termination: command_termination(result.termination), output_bytes: finalized.output_bytes, live_streaming: streaming.live_streaming, }, &stage_scope, ); - if result.termination == CommandTermination::TimedOut { + if result.termination == Termination::TimedOut { let mut reason = format!("Script timed out after {timeout_ms}ms: {script}"); append_output_tail(&mut reason, &finalized.output_text); return Err(Error::handler(reason)); } - if result.termination == CommandTermination::Cancelled { + if matches!( + result.termination, + Termination::Cancelled | Termination::Killed + ) { let mut reason = format!("Script cancelled: {script}"); append_output_tail(&mut reason, &finalized.output_text); return Err(Error::handler(reason)); } - if result.exit_code == Some(0) { + if result.success() { let validation = output_schema.as_ref().map(|schema| { ( schema, @@ -191,7 +202,7 @@ impl Handler for CommandHandler { keys::COMMAND_OUTPUT.to_string(), serde_json::json!(finalized.output_ref), ); - outcome.timing = Some(StageTiming::active_only(0, result.duration_ms)); + outcome.timing = Some(StageTiming::active_only(0, result.duration_ms())); if let Some((schema, Ok(validated))) = validation { structured_output::apply_validated_output(node, schema, &validated, &mut outcome); } @@ -199,7 +210,7 @@ impl Handler for CommandHandler { } else { let mut reason = format!( "Script failed with exit code: {}", - result.exit_code.unwrap_or(-1) + result.program_exit_code().unwrap_or(-1) ); append_output_tail(&mut reason, &finalized.output_text); let mut outcome = Outcome::fail_classify(reason); @@ -207,7 +218,7 @@ impl Handler for CommandHandler { keys::COMMAND_OUTPUT.to_string(), serde_json::json!(finalized.output_ref), ); - outcome.timing = Some(StageTiming::active_only(0, result.duration_ms)); + outcome.timing = Some(StageTiming::active_only(0, result.duration_ms())); Ok(outcome) } } @@ -321,7 +332,8 @@ mod tests { use bytes::Bytes; use fabro_graphviz::graph::AttrValue; - use fabro_sandbox::test_support::MockSandbox; + use fabro_sandbox::Termination; + use fabro_sandbox::test_support::{MockSandbox, exec_result}; use fabro_store::{Database, RunDatabase, StageId}; use fabro_types::{Graph, RunProjection, RunSpec, WorkflowSettings, fixtures, test_support}; use object_store::memory::InMemory; @@ -842,13 +854,7 @@ mod tests { #[tokio::test] async fn command_invalid_output_schema_fails_before_execution() { let spy = MockSandbox { - exec_result: fabro_agent::sandbox::ExecResult { - stdout: String::new(), - stderr: String::new(), - exit_code: Some(0), - termination: CommandTermination::Exited, - duration_ms: 1, - }, + exec_result: exec_result("", "", Some(0), Termination::Exited, 1), ..Default::default() }; let handler = CommandHandler; @@ -1590,13 +1596,7 @@ mod tests { #[tokio::test] async fn executes_script_via_sandbox() { let spy = MockSandbox { - exec_result: fabro_agent::sandbox::ExecResult { - stdout: "SANDBOX_MARKER\n".into(), - stderr: String::new(), - exit_code: Some(0), - termination: CommandTermination::Exited, - duration_ms: 5, - }, + exec_result: exec_result("SANDBOX_MARKER\n", "", Some(0), Termination::Exited, 5), ..Default::default() }; @@ -1633,13 +1633,7 @@ mod tests { #[tokio::test] async fn executes_python_script_via_sandbox() { let spy = MockSandbox { - exec_result: fabro_agent::sandbox::ExecResult { - stdout: "PYTHON_SANDBOX\n".into(), - stderr: String::new(), - exit_code: Some(0), - termination: CommandTermination::Exited, - duration_ms: 5, - }, + exec_result: exec_result("PYTHON_SANDBOX\n", "", Some(0), Termination::Exited, 5), ..Default::default() }; @@ -1679,13 +1673,7 @@ mod tests { #[tokio::test] async fn passes_env_vars_to_sandbox() { let spy = MockSandbox { - exec_result: fabro_agent::sandbox::ExecResult { - stdout: String::new(), - stderr: String::new(), - exit_code: Some(0), - termination: CommandTermination::Exited, - duration_ms: 5, - }, + exec_result: exec_result("", "", Some(0), Termination::Exited, 5), ..Default::default() }; @@ -1717,13 +1705,7 @@ mod tests { #[tokio::test] async fn refreshes_github_token_for_each_command_stage_when_near_expiry() { let spy = MockSandbox { - exec_result: fabro_agent::sandbox::ExecResult { - stdout: String::new(), - stderr: String::new(), - exit_code: Some(0), - termination: CommandTermination::Exited, - duration_ms: 5, - }, + exec_result: exec_result("", "", Some(0), Termination::Exited, 5), ..Default::default() }; let minter = std::sync::Arc::new(RefreshingMinter { @@ -1772,13 +1754,7 @@ mod tests { #[tokio::test] async fn passes_run_cancellation_to_sandbox() { let spy = MockSandbox { - exec_result: fabro_agent::sandbox::ExecResult { - stdout: String::new(), - stderr: String::new(), - exit_code: Some(0), - termination: CommandTermination::Exited, - duration_ms: 5, - }, + exec_result: exec_result("", "", Some(0), Termination::Exited, 5), ..Default::default() }; @@ -1806,13 +1782,13 @@ mod tests { #[tokio::test] async fn script_handler_timeout_error_includes_output_tails() { let spy = MockSandbox { - exec_result: fabro_agent::sandbox::ExecResult { - stdout: "partial stdout\n".into(), - stderr: "partial stderr\n".into(), - exit_code: None, - termination: CommandTermination::TimedOut, - duration_ms: 50, - }, + exec_result: exec_result( + "partial stdout\n", + "partial stderr\n", + None, + Termination::TimedOut, + 50, + ), ..Default::default() }; diff --git a/lib/components/fabro-workflow/src/handler/llm/changed_files.rs b/lib/components/fabro-workflow/src/handler/llm/changed_files.rs index b7d93c7c7..e0526648e 100644 --- a/lib/components/fabro-workflow/src/handler/llm/changed_files.rs +++ b/lib/components/fabro-workflow/src/handler/llm/changed_files.rs @@ -18,8 +18,8 @@ pub async fn detect_changed_files(sandbox: &Arc) -> Vec { .exec_command(&command, 30_000, None, None, None) .await { - if result.is_success() { - files.extend(parse_changed_files(&result.stdout)); + if result.success() { + files.extend(parse_changed_files(&result.stdout_lossy())); } } @@ -50,8 +50,8 @@ pub async fn files_touched_since( .await .ok() .and_then(|result| { - let trimmed = result.stdout.trim().to_string(); - (result.is_success() && !trimmed.is_empty()).then_some(trimmed) + let trimmed = result.stdout_lossy().trim().to_string(); + (result.success() && !trimmed.is_empty()).then_some(trimmed) }) }; diff --git a/lib/components/fabro-workflow/src/pipeline/initialize.rs b/lib/components/fabro-workflow/src/pipeline/initialize.rs index 616337be9..b5c2b8fb9 100644 --- a/lib/components/fabro-workflow/src/pipeline/initialize.rs +++ b/lib/components/fabro-workflow/src/pipeline/initialize.rs @@ -11,8 +11,8 @@ use fabro_hooks::{HookContext, HookDecision, HookEvent, HookExecutionContext, Ho use fabro_llm::credentials::{CredentialProvider, readiness}; use fabro_llm::lithos_catalog::Catalog; use fabro_sandbox::{ - DaytonaCredentials, GitSetupIntent, ProviderAccess, SandboxSpec, reconnect_for_run_with_events, - shell_quote, + DaytonaCredentials, ExecResultExt, GitSetupIntent, ProviderAccess, SandboxSpec, + reconnect_for_run_with_events, shell_quote, }; use fabro_static::EnvVars; use fabro_types::RunSandboxKind; @@ -672,19 +672,19 @@ pub async fn initialize( } cancel_token.cancel(); let duration_ms = crate::millis_u64(cmd_start.elapsed()); - if !result.is_success() { - let exit_code = result.display_exit_code(); + if !result.success() { + let exit_code = result.program_exit_code().unwrap_or(-1); let exec_output_tail = result.default_redacted_output_tail(); + let stderr = result.stderr_lossy(); options.emitter.emit(&Event::SetupFailed { command: command.clone(), index, exit_code, - stderr: result.stderr.clone(), + stderr: stderr.clone(), exec_output_tail, }); return Err(Error::engine(format!( - "Setup command failed (exit code {}): {command}\n{}", - exit_code, result.stderr, + "Setup command failed (exit code {exit_code}): {command}\n{stderr}", ))); } let exit_code = result.exit_code.unwrap_or(0); diff --git a/lib/components/fabro-workflow/src/sandbox_git.rs b/lib/components/fabro-workflow/src/sandbox_git.rs index 400a5772d..6889d6c13 100644 --- a/lib/components/fabro-workflow/src/sandbox_git.rs +++ b/lib/components/fabro-workflow/src/sandbox_git.rs @@ -3,7 +3,7 @@ use std::collections::{HashMap, HashSet}; use fabro_agent::RunSandbox; use fabro_checkpoint::trailer as trailerlink; use fabro_checkpoint::trailer::Trailer; -use fabro_sandbox::shell_quote; +use fabro_sandbox::{ExecResult, ExecResultExt, Termination, shell_quote}; use fabro_types::settings::run::RunCheckpointSettings; use fabro_util::error::SharedError; @@ -22,24 +22,21 @@ pub struct GitCommandError { pub const GIT_REMOTE: &str = "git -c maintenance.auto=0 -c gc.auto=0 -c commit.gpgsign=false -c tag.gpgsign=false"; -pub(crate) fn exec_err(label: &str, r: fabro_sandbox::ExecResult) -> GitCommandError { - if r.is_timed_out() { - return GitCommandError { - message: format!("{label} timed out after {}ms", r.duration_ms), - source: fabro_sandbox::Error::exec(label, r), - }; - } - if r.is_cancelled() { - return GitCommandError { - message: format!("{label} cancelled after {}ms", r.duration_ms), - source: fabro_sandbox::Error::exec(label, r), - }; - } - - let exit = r.display_exit_code(); +pub(crate) fn exec_err(label: &str, r: ExecResult) -> GitCommandError { + let duration_ms = r.duration_ms(); + let message = match r.termination { + Termination::TimedOut => format!("{label} timed out after {duration_ms}ms"), + Termination::Cancelled | Termination::Killed => { + format!("{label} cancelled after {duration_ms}ms") + } + _ => format!( + "{label} failed (exit {})", + r.program_exit_code().unwrap_or(-1) + ), + }; GitCommandError { - message: format!("{label} failed (exit {exit})"), - source: fabro_sandbox::Error::exec(label, r), + message, + source: r.into_exec_error(label), } } @@ -73,7 +70,7 @@ pub async fn git_checkpoint( .exec_command(&add_cmd, checkpoint.commit_timeout_ms, None, None, None) .await; match add_result { - Ok(r) if r.is_success() => {} + Ok(r) if r.success() => {} Ok(r) => return Err(exec_err("git add", r)), Err(e) => { return Err(GitCommandError { @@ -129,7 +126,7 @@ pub async fn git_checkpoint( .await; let _ = sandbox.delete_file(&msg_path).await; match commit_result { - Ok(r) if r.is_success() => {} + Ok(r) if r.success() => {} Ok(r) => return Err(exec_err("git commit", r)), Err(e) => { return Err(GitCommandError { @@ -144,7 +141,7 @@ pub async fn git_checkpoint( .exec_command(&sha_cmd, 10_000, None, None, None) .await; match sha_result { - Ok(r) if r.is_success() => Ok(r.stdout.trim().to_string()), + Ok(r) if r.success() => Ok(r.stdout_lossy().trim().to_string()), Ok(r) => Err(exec_err("git rev-parse HEAD", r)), Err(e) => Err(GitCommandError { message: "git rev-parse HEAD failed".to_string(), @@ -217,7 +214,7 @@ pub(crate) async fn git_diff_with_timeout( .exec_command(&cmd, timeout_ms, None, None, None) .await { - Ok(r) if r.is_success() => Ok(r.stdout), + Ok(r) if r.success() => Ok(r.stdout_lossy()), Ok(r) => Err(exec_err("git diff", r)), Err(e) => Err(GitCommandError { message: "git diff failed".to_string(), @@ -367,22 +364,22 @@ pub async fn list_changed_files_raw( message: e.display_with_causes(), })?; - if res.is_timed_out() { + if res.termination == Termination::TimedOut { return Err(DiffError::Transient { message: "git diff --raw timed out".to_string(), }); } - if !res.is_success() { + if !res.success() { // An unknown-object / bad-revision error is permanent; everything // else we treat as transient so the server can retry safely. - let stderr = res.stderr.trim().to_string(); + let stderr = res.stderr_lossy().trim().to_string(); if is_permanent_git_error(&stderr) { return Err(DiffError::Permanent { message: stderr }); } return Err(DiffError::Transient { message: stderr }); } - parse_raw_z(&res.stdout).map_err(|message| DiffError::Permanent { message }) + parse_raw_z(&res.stdout_lossy()).map_err(|message| DiffError::Permanent { message }) } fn is_permanent_git_error(stderr: &str) -> bool { @@ -570,13 +567,13 @@ pub async fn list_diff_numstat( message: e.display_with_causes(), })?; - if res.is_timed_out() { + if res.termination == Termination::TimedOut { return Err(DiffError::Transient { message: "git diff --numstat timed out".to_string(), }); } - if !res.is_success() { - let stderr = res.stderr.trim().to_string(); + if !res.success() { + let stderr = res.stderr_lossy().trim().to_string(); if is_permanent_git_error(&stderr) { return Err(DiffError::Permanent { message: stderr }); } @@ -584,7 +581,7 @@ pub async fn list_diff_numstat( } let mut out = DiffNumstat::default(); - for line in res.stdout.lines() { + for line in res.stdout_lossy().lines() { // `-\t-\t` marks binary. Rename lines read `<+>\t<->\t => // ` or `<+>\t<->\t{ => }`. if let Some(rest) = line.strip_prefix("-\t-\t") { @@ -661,19 +658,22 @@ pub async fn stream_blob_metadata( message: e.display_with_causes(), })?; - if res.is_timed_out() { + if res.termination == Termination::TimedOut { return Err(DiffError::Transient { message: "git cat-file --batch-check timed out".to_string(), }); } - if !res.is_success() { + if !res.success() { return Err(DiffError::Transient { - message: format!("git cat-file --batch-check failed: {}", res.stderr.trim()), + message: format!( + "git cat-file --batch-check failed: {}", + res.stderr_lossy().trim() + ), }); } let mut metas = Vec::with_capacity(shas.len()); - for line in res.stdout.lines() { + for line in res.stdout_lossy().lines() { // Lines: " " OR " missing" let mut parts = line.split(' '); let sha = parts @@ -728,18 +728,18 @@ pub async fn stream_blobs( message: e.display_with_causes(), })?; - if res.is_timed_out() { + if res.termination == Termination::TimedOut { return Err(DiffError::Transient { message: "git cat-file --batch timed out".to_string(), }); } - if !res.is_success() { + if !res.success() { return Err(DiffError::Transient { - message: format!("git cat-file --batch failed: {}", res.stderr.trim()), + message: format!("git cat-file --batch failed: {}", res.stderr_lossy().trim()), }); } - parse_batch_output(&res.stdout, shas, size_cap_bytes) + parse_batch_output(&res.stdout_lossy(), shas, size_cap_bytes) .map_err(|message| DiffError::Permanent { message }) } @@ -812,9 +812,7 @@ mod tests { reason = "These unit tests use the real git CLI to construct sandbox-git fixture repositories and sync-write fixtures to disk." )] - use fabro_agent::ExecResult; - use fabro_sandbox::test_support::MockSandbox; - use fabro_types::CommandTermination; + use fabro_sandbox::test_support::{MockSandbox, exec_result}; use super::*; @@ -822,39 +820,21 @@ mod tests { fn scripted(exec_results: &[ExecResult]) -> MockSandbox { let sandbox = MockSandbox::default(); for result in exec_results { - sandbox.push_exec_result(result); + sandbox.push_exec_result(result.clone()); } sandbox } fn exec_ok() -> ExecResult { - ExecResult { - stdout: String::new(), - stderr: String::new(), - exit_code: Some(0), - termination: CommandTermination::Exited, - duration_ms: 1, - } + exec_result("", "", Some(0), Termination::Exited, 1) } fn exec_timed_out(duration_ms: u64) -> ExecResult { - ExecResult { - stdout: String::new(), - stderr: String::new(), - exit_code: None, - termination: CommandTermination::TimedOut, - duration_ms, - } + exec_result("", "", None, Termination::TimedOut, duration_ms) } fn exec_failed(exit_code: i32, stdout: &str, stderr: &str) -> ExecResult { - ExecResult { - stdout: stdout.to_string(), - stderr: stderr.to_string(), - exit_code: Some(exit_code), - termination: CommandTermination::Exited, - duration_ms: 1, - } + exec_result(stdout, stderr, Some(exit_code), Termination::Exited, 1) } #[test] @@ -1199,7 +1179,8 @@ mod tests { ) .await .unwrap(); - let staged_files: Vec<&str> = status.stdout.lines().collect(); + let status_stdout = status.stdout_lossy(); + let staged_files: Vec<&str> = status_stdout.lines().collect(); assert!( staged_files.contains(&"hello.txt"), "expected hello.txt to be staged, got: {staged_files:?}" diff --git a/lib/components/fabro-workflow/src/sandbox_git_runtime.rs b/lib/components/fabro-workflow/src/sandbox_git_runtime.rs index 21d404ba3..053bab66c 100644 --- a/lib/components/fabro-workflow/src/sandbox_git_runtime.rs +++ b/lib/components/fabro-workflow/src/sandbox_git_runtime.rs @@ -87,7 +87,7 @@ async fn exec_ok(sandbox: &RunSandbox, command: &str) -> Result<(), SharedError> .map_err(|err| { SharedError::new(anyhow::Error::new(err).context("sandbox git probe command failed")) })?; - if result.is_success() { + if result.success() { Ok(()) } else { Err(SharedError::new(anyhow::Error::new(exec_err( diff --git a/lib/components/fabro-workflow/tests/it/daytona_integration.rs b/lib/components/fabro-workflow/tests/it/daytona_integration.rs index 16b20d610..b559e5b15 100644 --- a/lib/components/fabro-workflow/tests/it/daytona_integration.rs +++ b/lib/components/fabro-workflow/tests/it/daytona_integration.rs @@ -286,7 +286,7 @@ async fn daytona_exec_command() { .await .unwrap(); assert_eq!(result.exit_code, Some(0)); - assert!(result.stdout.contains("hello")); + assert!(result.stdout_lossy().contains("hello")); env.cleanup().await.unwrap(); } @@ -302,7 +302,7 @@ async fn daytona_exec_command_with_pipe() { .await .unwrap(); assert_eq!(result.exit_code, Some(0)); - assert!(result.stdout.trim().contains('2')); + assert!(result.stdout_lossy().trim().contains('2')); env.cleanup().await.unwrap(); } @@ -329,8 +329,11 @@ async fn daytona_exec_command_cancelled() { .unwrap(); assert_eq!(result.exit_code, None); - assert!(result.is_cancelled()); - assert_eq!(result.stderr, "Command cancelled"); + assert!(matches!( + result.termination, + fabro_sandbox::Termination::Cancelled | fabro_sandbox::Termination::Killed + )); + assert_eq!(result.stderr_lossy(), "Command cancelled"); env.cleanup().await.unwrap(); } @@ -362,8 +365,8 @@ async fn daytona_exec_command_local_timeout() { "Command stalled for longer than the local timeout mechanism" ); assert_eq!(result.exit_code, None); - assert!(result.is_timed_out()); - assert_eq!(result.stderr, "Command timed out locally"); + assert_eq!(result.termination, fabro_sandbox::Termination::TimedOut); + assert_eq!(result.stderr_lossy(), "Command timed out locally"); env.cleanup().await.unwrap(); } @@ -453,7 +456,7 @@ async fn daytona_snapshot_sandbox() { .await .unwrap(); assert_eq!(result.exit_code, Some(0)); - assert!(result.stdout.contains("ripgrep")); + assert!(result.stdout_lossy().contains("ripgrep")); env.cleanup().await.unwrap(); } @@ -668,9 +671,9 @@ async fn setup_daytona_git(sandbox: &RunSandbox) -> (RunId, String, String) { sha_result.exit_code, Some(0), "git rev-parse HEAD failed: {}", - sha_result.stderr + sha_result.stderr_lossy() ); - let base_sha = sha_result.stdout.trim().to_string(); + let base_sha = sha_result.stdout_lossy().trim().to_string(); let run_id = RunId::from(Ulid::new()); let branch_name = format!("fabro/run/{run_id}"); @@ -685,8 +688,8 @@ async fn setup_daytona_git(sandbox: &RunSandbox) -> (RunId, String, String) { Some(0), "git checkout -b failed (exit {:?}): stdout={} stderr={}", checkout_result.exit_code, - checkout_result.stdout, - checkout_result.stderr + checkout_result.stdout_lossy(), + checkout_result.stderr_lossy() ); (run_id, base_sha, branch_name) @@ -702,7 +705,7 @@ async fn daytona_git_checkpoint_remote_emits_events() { let git_check = env .exec_command("git --version", 10_000, None, None, None) .await; - if git_check.as_ref().map_or(true, |r| !r.is_success()) { + if git_check.as_ref().map_or(true, |r| !r.success()) { let install = env .exec_command( "apt-get update -qq && apt-get install -y -qq git >/dev/null 2>&1", @@ -717,7 +720,7 @@ async fn daytona_git_checkpoint_remote_emits_events() { install.exit_code, Some(0), "git install failed: {}", - install.stderr + install.stderr_lossy() ); } @@ -852,7 +855,7 @@ async fn daytona_git_checkpoint_with_shadow_branch() { let git_check = env .exec_command("git --version", 10_000, None, None, None) .await; - if git_check.as_ref().map_or(true, |r| !r.is_success()) { + if git_check.as_ref().map_or(true, |r| !r.success()) { let install = env .exec_command( "apt-get update -qq && apt-get install -y -qq git >/dev/null 2>&1", @@ -867,7 +870,7 @@ async fn daytona_git_checkpoint_with_shadow_branch() { install.exit_code, Some(0), "git install failed: {}", - install.stderr + install.stderr_lossy() ); } @@ -948,9 +951,9 @@ async fn daytona_git_checkpoint_with_shadow_branch() { ) .await .expect("git show should succeed"); - assert_eq!(run_json.exit_code, Some(0), "{}", run_json.stderr); + assert_eq!(run_json.exit_code, Some(0), "{}", run_json.stderr_lossy()); let projection: fabro_store::RunProjection = - serde_json::from_slice(run_json.stdout.as_bytes()).expect("run.json should parse"); + serde_json::from_slice(run_json.stdout_lossy().as_bytes()).expect("run.json should parse"); let checkpoint = projection .current_checkpoint() .cloned() @@ -970,7 +973,7 @@ async fn daytona_git_checkpoint_with_shadow_branch() { .await .expect("git log should succeed"); assert_eq!(log_result.exit_code, Some(0)); - let commit_msg = log_result.stdout.trim().to_string(); + let commit_msg = log_result.stdout_lossy().trim().to_string(); assert!( commit_msg.contains("Fabro-Checkpoint:"), "sandbox commit should have Fabro-Checkpoint trailer, got:\n{commit_msg}" @@ -1174,7 +1177,7 @@ async fn daytona_clone_private_repo_with_github_app_iat() { "CLAUDE.md should exist after clone" ); assert!( - result.stdout.contains("EXISTS"), + result.stdout_lossy().contains("EXISTS"), "clone should have populated the workspace" ); @@ -1182,7 +1185,7 @@ async fn daytona_clone_private_repo_with_github_app_iat() { let git_check = env .exec_command("git --version", 10_000, None, None, None) .await; - if git_check.as_ref().map_or(true, |r| !r.is_success()) { + if git_check.as_ref().map_or(true, |r| !r.success()) { let install = env .exec_command( "apt-get update -qq && apt-get install -y -qq git >/dev/null 2>&1", @@ -1197,7 +1200,7 @@ async fn daytona_clone_private_repo_with_github_app_iat() { install.exit_code, Some(0), "git install failed: {}", - install.stderr + install.stderr_lossy() ); } @@ -1208,9 +1211,9 @@ async fn daytona_clone_private_repo_with_github_app_iat() { .unwrap(); assert_eq!(result.exit_code, Some(0)); assert!( - result.stdout.contains("fabro-sh/fabro"), + result.stdout_lossy().contains("fabro-sh/fabro"), "origin should point to fabro-sh/fabro, got: {}", - result.stdout.trim() + result.stdout_lossy().trim() ); env.cleanup().await.unwrap(); @@ -1285,7 +1288,7 @@ async fn daytona_git_push_run_branch_to_origin() { let git_check = env .exec_command("git --version", 10_000, None, None, None) .await; - if git_check.as_ref().map_or(true, |r| !r.is_success()) { + if git_check.as_ref().map_or(true, |r| !r.success()) { let install = env .exec_command( "apt-get update -qq && apt-get install -y -qq git >/dev/null 2>&1", @@ -1300,7 +1303,7 @@ async fn daytona_git_push_run_branch_to_origin() { install.exit_code, Some(0), "git install failed: {}", - install.stderr + install.stderr_lossy() ); } @@ -1377,12 +1380,12 @@ async fn daytona_git_push_run_branch_to_origin() { ls_result.exit_code, Some(0), "git ls-remote failed: {}", - ls_result.stdout + ls_result.stdout_lossy() ); assert!( - ls_result.stdout.contains(&branch_name), + ls_result.stdout_lossy().contains(&branch_name), "run branch should exist on origin after push, got: {}", - ls_result.stdout.trim() + ls_result.stdout_lossy().trim() ); // Clean up the remote branch @@ -1391,10 +1394,10 @@ async fn daytona_git_push_run_branch_to_origin() { .exec_command(&delete_cmd, 30_000, None, None, None) .await; if let Ok(r) = &delete_result { - if !r.is_success() { + if !r.success() { eprintln!( "Warning: failed to delete remote branch {branch_name}: {}", - r.stdout + r.stdout_lossy() ); } } @@ -1446,7 +1449,7 @@ async fn daytona_toolbox_idle_diagnostic() { eprintln!( "[t=+{sleep_secs}s] OK exit_code={:?} stdout={}", r.exit_code, - r.stdout.trim() + r.stdout_lossy().trim() ); } Err(e) => { @@ -1690,9 +1693,9 @@ async fn daytona_computer_use_browser_screenshot() { ) .await .unwrap(); - eprintln!("Browser check: {}", check.stdout.trim()); + eprintln!("Browser check: {}", check.stdout_lossy().trim()); - if check.stdout.trim() == "NONE" { + if check.stdout_lossy().trim() == "NONE" { let install_result = env .exec_command( "apt-get update -qq && DEBIAN_FRONTEND=noninteractive apt-get install -y -qq chromium 2>&1", @@ -1703,7 +1706,7 @@ async fn daytona_computer_use_browser_screenshot() { eprintln!( "Browser install exit_code={:?}, last_line={}", install_result.exit_code, - install_result.stdout.lines().last().unwrap_or("") + install_result.stdout_lossy().lines().last().unwrap_or("") ); assert_eq!(install_result.exit_code, Some(0), "Chromium install failed"); } @@ -1718,7 +1721,7 @@ async fn daytona_computer_use_browser_screenshot() { ) .await .unwrap(); - let browser = browser_bin.stdout.trim().to_string(); + let browser = browser_bin.stdout_lossy().trim().to_string(); eprintln!("Using browser: {browser}"); // 3. Detect the DISPLAY that computer use started @@ -1732,7 +1735,7 @@ async fn daytona_computer_use_browser_screenshot() { ) .await .unwrap(); - eprintln!("Xvfb process: {}", display_check.stdout.trim()); + eprintln!("Xvfb process: {}", display_check.stdout_lossy().trim()); // 4. Launch browser with setsid to fully detach, and log stderr let launch_cmd = format!( @@ -1760,7 +1763,7 @@ async fn daytona_computer_use_browser_screenshot() { ) .await .unwrap(); - eprintln!("Chrome processes:\n{}", ps_check.stdout); + eprintln!("Chrome processes:\n{}", ps_check.stdout_lossy()); let stderr_check = env .exec_command( @@ -1772,7 +1775,7 @@ async fn daytona_computer_use_browser_screenshot() { ) .await .unwrap(); - eprintln!("Chrome stderr:\n{}", stderr_check.stdout); + eprintln!("Chrome stderr:\n{}", stderr_check.stdout_lossy()); // 5. The desktop is serving: noVNC listens on its port. let listening = env @@ -1786,7 +1789,7 @@ async fn daytona_computer_use_browser_screenshot() { .await .unwrap(); assert!( - listening.is_success(), + listening.success(), "noVNC should be reachable inside the sandbox" ); @@ -1832,7 +1835,7 @@ async fn daytona_playwright_mcp_sandbox_transport() { "Install exit_code={:?}, last_lines:\n{}", install.exit_code, install - .stdout + .stdout_lossy() .lines() .rev() .take(5) @@ -1889,7 +1892,7 @@ async fn daytona_playwright_mcp_sandbox_transport() { .exec_command(&launch_script, 30_000, None, None, None) .await .unwrap(); - eprintln!("MCP server PID: {}", launch_result.stdout.trim()); + eprintln!("MCP server PID: {}", launch_result.stdout_lossy().trim()); // Wait for server to listen let poll_cmd = format!( @@ -1899,9 +1902,9 @@ async fn daytona_playwright_mcp_sandbox_transport() { .exec_command(&poll_cmd, 60_000, None, None, None) .await .unwrap(); - eprintln!("Server readiness: {}", poll_result.stdout.trim()); + eprintln!("Server readiness: {}", poll_result.stdout_lossy().trim()); - if poll_result.stdout.trim() != "ready" { + if poll_result.stdout_lossy().trim() != "ready" { let stderr = sandbox .exec_command( "cat /tmp/mcp_server_stderr.log 2>/dev/null | tail -20", @@ -1911,7 +1914,7 @@ async fn daytona_playwright_mcp_sandbox_transport() { None, ) .await - .map(|r| r.stdout) + .map(|r| r.stdout_lossy()) .unwrap_or_default(); panic!("MCP server did not start on port {port}. stderr:\n{stderr}"); }