From 6780dff63f5ab1fa30cb809e9d47e7a372888f2d Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Sun, 3 May 2026 21:30:34 -0400 Subject: [PATCH] fix(workflow): retain exec output tails on failures Keep sandbox exec failures structured until event emission so git push, checkpoint, notice, and retro failures can expose redacted output tails without expanding their terse error strings. Also add log rendering that appends sanitized tail content for exec-backed errors while preserving the existing safe Display behavior. --- docs/internal/logging-strategy.md | 8 +- .../src/commands/run/run_progress/event.rs | 7 +- .../src/commands/run/run_progress/mod.rs | 21 +-- lib/crates/fabro-sandbox/src/daytona/mod.rs | 2 +- lib/crates/fabro-sandbox/src/error.rs | 125 ++++++++++++++-- lib/crates/fabro-sandbox/src/lib.rs | 2 +- lib/crates/fabro-sandbox/src/sandbox.rs | 4 +- lib/crates/fabro-types/src/run_event/misc.rs | 13 +- lib/crates/fabro-types/src/run_event/mod.rs | 78 ++++++++++ lib/crates/fabro-types/src/run_event/run.rs | 10 +- lib/crates/fabro-types/src/run_event/stage.rs | 5 +- .../fabro-workflow/src/event/convert.rs | 133 +++++++++++++++--- .../fabro-workflow/src/event/emitter.rs | 18 ++- lib/crates/fabro-workflow/src/event/events.rs | 120 +++++++++++++--- lib/crates/fabro-workflow/src/event/sink.rs | 7 +- .../fabro-workflow/src/handler/parallel.rs | 5 +- .../fabro-workflow/src/lifecycle/event.rs | 7 +- .../fabro-workflow/src/lifecycle/git.rs | 63 ++++++--- .../fabro-workflow/src/operations/start.rs | 14 +- .../fabro-workflow/src/pipeline/finalize.rs | 16 +-- .../fabro-workflow/src/pipeline/retro.rs | 21 ++- lib/crates/fabro-workflow/src/sandbox_git.rs | 99 +++++++++---- .../fabro-workflow/src/sandbox_git_runtime.rs | 4 +- 23 files changed, 626 insertions(+), 156 deletions(-) diff --git a/docs/internal/logging-strategy.md b/docs/internal/logging-strategy.md index b720d491f..35ac6f84c 100644 --- a/docs/internal/logging-strategy.md +++ b/docs/internal/logging-strategy.md @@ -205,12 +205,12 @@ Some field values carry real or latent sensitivity and must not appear in `traci | `diff_contents` | File contents from a user workspace may include secrets, PII, or copyrighted code. | `bytes_total`, `file_count`, `truncated` counters | | `file_path` (for changed-file paths in the Run Files endpoint specifically) | Leaks workspace structure; combined with public run IDs can expose layout of private repos. | `file_count`, aggregate counts bucketed by `binary`, `sensitive`, `symlink`, `submodule` | | `git_stderr` | Raw git output for untrusted workspaces may include path-shaped secrets (e.g. `~/.ssh/id_rsa_work`) and terminal control sequences. | A short categorized reason (`"timeout"`, `"bad_revision"`, `"unknown_object"`) derived from stderr, never the stderr itself | -| Raw command stdout/stderr, including raw `git_stderr` | Process output for untrusted workspaces may include secrets, PII, paths, or terminal control sequences. Durable run events may include `ExecOutputTail`, which is bounded and redacted before serialization. | In tracing, emit only tail metadata such as presence, byte count, and truncation booleans. | +| Raw command stdout/stderr, including raw `git_stderr` | Process output for untrusted workspaces may include secrets, PII, paths, or terminal control sequences. | Use `ExecOutputTail`, which is sanitized, redacted, and tail-truncated before serialization or tracing. | | Credential-ish strings (`api_key`, `bearer_token`, `cookie`, `session_id`, …) | Exfiltration risk. | Emit `has_credentials: true` or a fingerprint (`token_last4`) only when debugging is the only option | -These prohibitions apply to every level (ERROR through TRACE). If an error path genuinely needs raw output for triage, route it through an authenticated support channel — not the default tracing subscriber. +These prohibitions apply to every level (ERROR through TRACE). If an error path genuinely needs raw output for triage, route it through an authenticated support channel — not the default tracing subscriber. The redacted, sanitized, tail-truncated `ExecOutputTail` form is permitted in tracing because that redaction layer is the safety boundary; never log the raw `ExecResult` streams directly. -Safe tracing for process failures looks like: +Safe event tracing for process failures should include bounded metadata when the structured tail is already present on the event: ```rust error!( @@ -225,4 +225,6 @@ error!( ); ``` +When logging a caught error that may wrap a sandbox `exec_command` failure, render it with `fabro_sandbox::display_for_log(&err)`. That preserves the normal cause chain and appends the redacted `ExecOutputTail` when one is present. + For URLs that may carry credentials, log `fabro_redact::DisplaySafeUrl` or a string produced by `DisplaySafeUrl::redacted_string()`. Its `Display` and `Debug` forms redact userinfo plus these query keys case-insensitively: `token`, `install_token`, `access_token`, `refresh_token`, `api_key`, `apikey`, `code`, `state`, `password`, `secret`, and `key`. Raw URL strings stay reserved for wire transit, subprocess arguments, redirects, and persistence. diff --git a/lib/crates/fabro-cli/src/commands/run/run_progress/event.rs b/lib/crates/fabro-cli/src/commands/run/run_progress/event.rs index f1a172220..9e40f0d8b 100644 --- a/lib/crates/fabro-cli/src/commands/run/run_progress/event.rs +++ b/lib/crates/fabro-cli/src/commands/run/run_progress/event.rs @@ -803,9 +803,10 @@ mod tests { #[test] fn round_trip_run_notice() { let event = Event::RunNotice { - level: RunNoticeLevel::Warn, - code: "sandbox_cleanup_failed".into(), - message: "sandbox cleanup failed".into(), + level: RunNoticeLevel::Warn, + code: "sandbox_cleanup_failed".into(), + message: "sandbox cleanup failed".into(), + exec_output_tail: None, }; let stored = to_run_event(&fixtures::RUN_1, &event); diff --git a/lib/crates/fabro-cli/src/commands/run/run_progress/mod.rs b/lib/crates/fabro-cli/src/commands/run/run_progress/mod.rs index 428e96315..2004ac0ba 100644 --- a/lib/crates/fabro-cli/src/commands/run/run_progress/mod.rs +++ b/lib/crates/fabro-cli/src/commands/run/run_progress/mod.rs @@ -1207,9 +1207,10 @@ mod tests { let (mut ui, buffer) = capture_ui(false); emit(&mut ui, Event::RunNotice { - level: RunNoticeLevel::Warn, - code: "sandbox_cleanup_failed".into(), - message: "sandbox cleanup failed".into(), + level: RunNoticeLevel::Warn, + code: "sandbox_cleanup_failed".into(), + message: "sandbox cleanup failed".into(), + exec_output_tail: None, }); emit(&mut ui, Event::PullRequestCreated { pr_url: "https://github.com/fabro-sh/fabro/pull/42".into(), @@ -1277,14 +1278,16 @@ mod tests { exec_output_tail: None, }); emit(&mut ui, Event::RunNotice { - level: RunNoticeLevel::Warn, - code: "checkpoint_metadata_write_failed".into(), - message: "legacy metadata warning".into(), + level: RunNoticeLevel::Warn, + code: "checkpoint_metadata_write_failed".into(), + message: "legacy metadata warning".into(), + exec_output_tail: None, }); emit(&mut ui, Event::RunNotice { - level: RunNoticeLevel::Warn, - code: "checkpoint_metadata_degraded".into(), - message: "metadata snapshots are disabled for this run".into(), + level: RunNoticeLevel::Warn, + code: "checkpoint_metadata_degraded".into(), + message: "metadata snapshots are disabled for this run".into(), + exec_output_tail: None, }); insta::assert_snapshot!(rendered(&buffer), @r" diff --git a/lib/crates/fabro-sandbox/src/daytona/mod.rs b/lib/crates/fabro-sandbox/src/daytona/mod.rs index 5d5570e3f..d4e1f4675 100644 --- a/lib/crates/fabro-sandbox/src/daytona/mod.rs +++ b/lib/crates/fabro-sandbox/src/daytona/mod.rs @@ -808,7 +808,7 @@ impl Sandbox for DaytonaSandbox { }, ); tracing::warn!( - error = %err, + error = %crate::display_for_log(&err), "Failed to set Daytona sandbox push credentials \ on origin — subsequent git push from this \ sandbox will fail" diff --git a/lib/crates/fabro-sandbox/src/error.rs b/lib/crates/fabro-sandbox/src/error.rs index da6043e5a..7b2377660 100644 --- a/lib/crates/fabro-sandbox/src/error.rs +++ b/lib/crates/fabro-sandbox/src/error.rs @@ -1,7 +1,11 @@ +use std::fmt::Write as _; + #[cfg(feature = "docker")] use bollard::errors::Error as BollardError; use fabro_util::error::{collect_causes, render_with_causes}; +use crate::ExecResult; + #[derive(Debug, thiserror::Error)] pub enum Error { #[error("{0}")] @@ -46,10 +50,7 @@ pub enum Error { .or_else(|| classify_exec_failure(&result.stdout)) .unwrap_or("unclassified") )] - Exec { - label: String, - result: crate::ExecResult, - }, + Exec { label: String, result: ExecResult }, } impl Error { @@ -67,7 +68,7 @@ impl Error { } } - pub fn exec(label: impl Into, result: crate::ExecResult) -> Self { + pub fn exec(label: impl Into, result: ExecResult) -> Self { Self::Exec { label: label.into(), result, @@ -75,10 +76,7 @@ impl Error { } pub fn default_redacted_output_tail(&self) -> Option { - match self { - Self::Exec { result, .. } => result.default_redacted_output_tail(), - _ => None, - } + default_redacted_output_tail(self) } #[cfg(feature = "docker")] @@ -163,6 +161,47 @@ fn format_exit_code(exit_code: Option) -> String { pub type Result = std::result::Result; +pub fn default_redacted_output_tail( + err: &(dyn std::error::Error + 'static), +) -> Option { + let mut current = Some(err); + while let Some(err) = current { + if let Some(Error::Exec { result, .. }) = err.downcast_ref::() { + return result.default_redacted_output_tail(); + } + current = err.source(); + } + None +} + +pub fn display_for_log(err: &(dyn std::error::Error + 'static)) -> String { + let mut rendered = render_with_causes(&err.to_string(), &collect_causes(err)); + if let Some(tail) = default_redacted_output_tail(err) { + append_tail_for_log( + &mut rendered, + "stderr", + tail.stderr.as_deref(), + tail.stderr_truncated, + ); + append_tail_for_log( + &mut rendered, + "stdout", + tail.stdout.as_deref(), + tail.stdout_truncated, + ); + } + rendered +} + +fn append_tail_for_log(rendered: &mut String, stream: &str, tail: Option<&str>, truncated: bool) { + let tail = tail.unwrap_or(""); + let _ = write!( + rendered, + "\n--- {stream} (truncated={truncated}, bytes={}) ---\n{tail}", + tail.len() + ); +} + #[cfg(test)] mod tests { use fabro_types::CommandTermination; @@ -214,6 +253,57 @@ mod tests { assert!(rendered.contains("hint:")); } + #[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 error = Error::context("metadata push failed", exec_error); + + let rendered = display_for_log(&error); + + assert!(rendered.contains("metadata push failed")); + assert!(rendered.contains("git push origin refs/heads/run")); + assert!(rendered.contains("--- stderr (truncated=false, bytes=16) ---")); + assert!(rendered.contains("last stderr line")); + assert!(rendered.contains("--- stdout (truncated=false, bytes=16) ---")); + assert!(rendered.contains("last stdout line")); + } + + #[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 rendered = display_for_log(&error); + + assert!( + !rendered.contains("ghs_xK9mZ2vL8nQ5rT1wY4bC7dF0gH3jE6pA"), + "log rendering leaked raw secret: {rendered}" + ); + assert!(rendered.contains("REDACTED")); + } + + #[test] + fn display_for_log_for_non_exec_error_returns_chain_only() { + let error = Error::context("outer failure", std::io::Error::other("leaf failure")); + + let rendered = display_for_log(&error); + + assert_eq!(rendered, "outer failure\n caused by: leaf failure"); + assert!(!rendered.contains("--- stderr")); + assert!(!rendered.contains("--- stdout")); + } + fn assert_exec_rendering_is_safe(rendered: &str) { for forbidden in [ "fatal:", @@ -251,6 +341,23 @@ 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 error = Error::context("metadata push failed", exec_error); + + let tail = default_redacted_output_tail(&error).expect("tail present"); + + 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 = [ diff --git a/lib/crates/fabro-sandbox/src/lib.rs b/lib/crates/fabro-sandbox/src/lib.rs index adad75bf2..44b44c038 100644 --- a/lib/crates/fabro-sandbox/src/lib.rs +++ b/lib/crates/fabro-sandbox/src/lib.rs @@ -32,7 +32,7 @@ pub mod test_support; #[cfg(feature = "docker")] pub use docker::{DockerSandbox, DockerSandboxOptions}; -pub use error::{Error, Result}; +pub use error::{Error, Result, default_redacted_output_tail, display_for_log}; pub use local::LocalSandbox; pub use read_guard::ReadBeforeWriteSandbox; pub use sandbox::{ diff --git a/lib/crates/fabro-sandbox/src/sandbox.rs b/lib/crates/fabro-sandbox/src/sandbox.rs index 1176c6dce..e30012969 100644 --- a/lib/crates/fabro-sandbox/src/sandbox.rs +++ b/lib/crates/fabro-sandbox/src/sandbox.rs @@ -447,7 +447,7 @@ impl ExecResult { /// /// This stores raw stdout/stderr. Callers must not log these fields /// directly; use `default_redacted_output_tail()` for events and - /// tracing metadata. + /// `display_for_log()` for tracing. #[cfg(test)] pub fn from_process_output(output: std::process::Output, duration_ms: u64) -> Self { let std::process::Output { @@ -884,7 +884,7 @@ pub async fn git_push_via_exec(sandbox: &dyn Sandbox, refspec: &str) -> crate::R if let Err(e) = sandbox.refresh_push_credentials().await { tracing::warn!( refspec = %refspec, - error = %e, + error = %crate::display_for_log(&e), "Failed to refresh push credentials before git push" ); } diff --git a/lib/crates/fabro-types/src/run_event/misc.rs b/lib/crates/fabro-types/src/run_event/misc.rs index e10203dbb..1570050a4 100644 --- a/lib/crates/fabro-types/src/run_event/misc.rs +++ b/lib/crates/fabro-types/src/run_event/misc.rs @@ -1,6 +1,7 @@ use serde::{Deserialize, Serialize}; use serde_json::Value; +use super::ExecOutputTail; use crate::CommandTermination; #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] @@ -95,8 +96,10 @@ pub struct GitCommitProps { #[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] pub struct GitPushProps { - pub branch: String, - pub success: bool, + pub branch: String, + pub success: bool, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub exec_output_tail: Option, } #[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] @@ -274,6 +277,8 @@ pub struct RetroCompletedProps { #[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] pub struct RetroFailedProps { - pub error: String, - pub duration_ms: u64, + pub error: String, + pub duration_ms: u64, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub exec_output_tail: Option, } diff --git a/lib/crates/fabro-types/src/run_event/mod.rs b/lib/crates/fabro-types/src/run_event/mod.rs index 8276534a3..9d5fbf11d 100644 --- a/lib/crates/fabro-types/src/run_event/mod.rs +++ b/lib/crates/fabro-types/src/run_event/mod.rs @@ -1353,4 +1353,82 @@ mod tests { .is_none() ); } + + #[test] + fn exec_output_tail_fields_are_additive_on_failure_props() { + let tail = ExecOutputTail { + stdout: Some("last stdout line".to_string()), + stderr: Some("last stderr line".to_string()), + stdout_truncated: false, + stderr_truncated: true, + }; + + for body in [ + EventBody::RunNotice(RunNoticeProps { + level: RunNoticeLevel::Warn, + code: "git_diff_failed".to_string(), + message: "git diff failed".to_string(), + exec_output_tail: Some(tail.clone()), + }), + EventBody::CheckpointFailed(CheckpointFailedProps { + error: "git commit failed".to_string(), + exec_output_tail: Some(tail.clone()), + }), + EventBody::GitPush(GitPushProps { + branch: "refs/heads/run:refs/heads/run".to_string(), + success: false, + exec_output_tail: Some(tail.clone()), + }), + EventBody::RetroFailed(RetroFailedProps { + error: "state unavailable".to_string(), + duration_ms: 10, + exec_output_tail: Some(tail.clone()), + }), + ] { + let value = serde_json::to_value(&body).unwrap(); + assert_eq!( + value["properties"]["exec_output_tail"]["stderr"], + "last stderr line" + ); + assert_eq!( + value["properties"]["exec_output_tail"]["stderr_truncated"], + true + ); + } + } + + #[test] + fn absent_exec_output_tail_is_omitted_from_new_failure_props() { + for body in [ + EventBody::RunNotice(RunNoticeProps { + level: RunNoticeLevel::Warn, + code: "git_diff_failed".to_string(), + message: "git diff failed".to_string(), + exec_output_tail: None, + }), + EventBody::CheckpointFailed(CheckpointFailedProps { + error: "git commit failed".to_string(), + exec_output_tail: None, + }), + EventBody::GitPush(GitPushProps { + branch: "refs/heads/run:refs/heads/run".to_string(), + success: false, + exec_output_tail: None, + }), + EventBody::RetroFailed(RetroFailedProps { + error: "state unavailable".to_string(), + duration_ms: 10, + exec_output_tail: None, + }), + ] { + let value = serde_json::to_value(&body).unwrap(); + assert!( + value["properties"] + .as_object() + .expect("properties object") + .get("exec_output_tail") + .is_none() + ); + } + } } diff --git a/lib/crates/fabro-types/src/run_event/run.rs b/lib/crates/fabro-types/src/run_event/run.rs index 4490a0240..6c8e1ddd1 100644 --- a/lib/crates/fabro-types/src/run_event/run.rs +++ b/lib/crates/fabro-types/src/run_event/run.rs @@ -2,7 +2,7 @@ use std::collections::BTreeMap; use serde::{Deserialize, Serialize}; -use super::{BilledTokenCounts, RunNoticeLevel}; +use super::{BilledTokenCounts, ExecOutputTail, RunNoticeLevel}; use crate::status::{BlockedReason, FailureReason, SuccessReason}; use crate::{ ForkSourceRef, GitContext, Graph, RunBlobId, RunControlAction, RunId, RunProvenance, @@ -147,7 +147,9 @@ pub struct RunFailedProps { #[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] pub struct RunNoticeProps { - pub level: RunNoticeLevel, - pub code: String, - pub message: String, + pub level: RunNoticeLevel, + pub code: String, + pub message: String, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub exec_output_tail: Option, } diff --git a/lib/crates/fabro-types/src/run_event/stage.rs b/lib/crates/fabro-types/src/run_event/stage.rs index 6f1a63b7a..9f1781d82 100644 --- a/lib/crates/fabro-types/src/run_event/stage.rs +++ b/lib/crates/fabro-types/src/run_event/stage.rs @@ -3,6 +3,7 @@ use std::collections::BTreeMap; use serde::{Deserialize, Serialize}; use serde_json::Value; +use super::ExecOutputTail; use crate::{BilledModelUsage, FailureDetail, Outcome, StageOutcome}; #[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] @@ -115,5 +116,7 @@ pub struct CheckpointCompletedProps { #[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] pub struct CheckpointFailedProps { - pub error: String, + pub error: String, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub exec_output_tail: Option, } diff --git a/lib/crates/fabro-workflow/src/event/convert.rs b/lib/crates/fabro-workflow/src/event/convert.rs index a66a43bdc..815aaec11 100644 --- a/lib/crates/fabro-workflow/src/event/convert.rs +++ b/lib/crates/fabro-workflow/src/event/convert.rs @@ -182,10 +182,12 @@ fn event_body_from_event(event: &Event) -> EventBody { level, code, message, + exec_output_tail, } => EventBody::RunNotice(fabro_types::RunNoticeProps { - level: *level, - code: code.clone(), - message: message.clone(), + level: *level, + code: code.clone(), + message: message.clone(), + exec_output_tail: exec_output_tail.clone(), }), Event::MetadataSnapshotStarted { phase, branch } => { EventBody::MetadataSnapshotStarted(fabro_types::MetadataSnapshotStartedProps { @@ -433,17 +435,25 @@ fn event_body_from_event(event: &Event) -> EventBody { node_visits: node_visits.clone(), diff: diff.clone(), }), - Event::CheckpointFailed { error, .. } => { - EventBody::CheckpointFailed(fabro_types::CheckpointFailedProps { - error: error.clone(), - }) - } + Event::CheckpointFailed { + error, + exec_output_tail, + .. + } => EventBody::CheckpointFailed(fabro_types::CheckpointFailedProps { + error: error.clone(), + exec_output_tail: exec_output_tail.clone(), + }), Event::GitCommit { sha, .. } => { EventBody::GitCommit(fabro_types::GitCommitProps { sha: sha.clone() }) } - Event::GitPush { branch, success } => EventBody::GitPush(fabro_types::GitPushProps { - branch: branch.clone(), - success: *success, + Event::GitPush { + branch, + success, + exec_output_tail, + } => EventBody::GitPush(fabro_types::GitPushProps { + branch: branch.clone(), + success: *success, + exec_output_tail: exec_output_tail.clone(), }), Event::GitBranch { branch, sha } => EventBody::GitBranch(fabro_types::GitBranchProps { branch: branch.clone(), @@ -1109,12 +1119,15 @@ fn event_body_from_event(event: &Event) -> EventBody { response: response.clone(), retro: retro.clone(), }), - Event::RetroFailed { error, duration_ms } => { - EventBody::RetroFailed(fabro_types::RetroFailedProps { - error: error.clone(), - duration_ms: *duration_ms, - }) - } + Event::RetroFailed { + error, + duration_ms, + exec_output_tail, + } => EventBody::RetroFailed(fabro_types::RetroFailedProps { + error: error.clone(), + duration_ms: *duration_ms, + exec_output_tail: exec_output_tail.clone(), + }), } } @@ -1154,8 +1167,8 @@ mod tests { use std::collections::BTreeMap; use ::fabro_types::{ - EventBody, FailureReason, ParallelBranchId, Principal, RunProvenance, StageId, - SystemActorKind, fixtures, run_event as fabro_types, + EventBody, FailureReason, ParallelBranchId, Principal, RunNoticeLevel, RunProvenance, + StageId, SystemActorKind, fixtures, run_event as fabro_types, }; use chrono::Utc; use fabro_agent::{AgentEvent, SandboxEvent}; @@ -1178,6 +1191,15 @@ mod tests { impl std::error::Error for EventTestCause {} + fn exec_tail() -> fabro_types::ExecOutputTail { + fabro_types::ExecOutputTail { + stdout: Some("last stdout line".to_string()), + stderr: Some("last stderr line".to_string()), + stdout_truncated: false, + stderr_truncated: true, + } + } + #[test] fn run_event_stage_completed_places_node_fields_in_header() { let stored = to_run_event_at( @@ -1590,6 +1612,79 @@ mod tests { } } + #[test] + fn run_notice_maps_exec_output_tail_to_props() { + let stored = to_run_event(&fixtures::RUN_1, &Event::RunNotice { + level: RunNoticeLevel::Warn, + code: "git_diff_failed".to_string(), + message: "git diff failed".to_string(), + exec_output_tail: Some(exec_tail()), + }); + + match stored.body { + EventBody::RunNotice(props) => { + let tail = props.exec_output_tail.expect("exec output tail"); + assert_eq!(tail.stderr.as_deref(), Some("last stderr line")); + assert!(tail.stderr_truncated); + } + other => panic!("expected RunNotice body, got {other:?}"), + } + } + + #[test] + fn checkpoint_failed_maps_exec_output_tail_to_props() { + let stored = to_run_event(&fixtures::RUN_1, &Event::CheckpointFailed { + node_id: "build".to_string(), + error: "git commit failed".to_string(), + exec_output_tail: Some(exec_tail()), + }); + + match stored.body { + EventBody::CheckpointFailed(props) => { + let tail = props.exec_output_tail.expect("exec output tail"); + assert_eq!(tail.stdout.as_deref(), Some("last stdout line")); + assert!(!tail.stdout_truncated); + } + other => panic!("expected CheckpointFailed body, got {other:?}"), + } + } + + #[test] + fn git_push_maps_exec_output_tail_to_props() { + let stored = to_run_event(&fixtures::RUN_1, &Event::GitPush { + branch: "refs/heads/run:refs/heads/run".to_string(), + success: false, + exec_output_tail: Some(exec_tail()), + }); + + match stored.body { + EventBody::GitPush(props) => { + assert!(!props.success); + let tail = props.exec_output_tail.expect("exec output tail"); + assert_eq!(tail.stderr.as_deref(), Some("last stderr line")); + } + other => panic!("expected GitPush body, got {other:?}"), + } + } + + #[test] + fn retro_failed_maps_exec_output_tail_to_props() { + let stored = to_run_event(&fixtures::RUN_1, &Event::RetroFailed { + error: "state load failed".to_string(), + duration_ms: 12, + exec_output_tail: Some(exec_tail()), + }); + + match stored.body { + EventBody::RetroFailed(props) => { + assert_eq!(props.duration_ms, 12); + let tail = props.exec_output_tail.expect("exec output tail"); + assert_eq!(tail.stdout.as_deref(), Some("last stdout line")); + } + other => panic!("expected RetroFailed body, got {other:?}"), + } + } + #[test] fn metadata_snapshot_events_map_to_typed_bodies() { let started = to_run_event(&fixtures::RUN_1, &Event::MetadataSnapshotStarted { diff --git a/lib/crates/fabro-workflow/src/event/emitter.rs b/lib/crates/fabro-workflow/src/event/emitter.rs index 4d54a0b40..a1bba727a 100644 --- a/lib/crates/fabro-workflow/src/event/emitter.rs +++ b/lib/crates/fabro-workflow/src/event/emitter.rs @@ -1,7 +1,7 @@ use std::sync::Arc; use std::sync::atomic::{AtomicI64, Ordering}; -use ::fabro_types::{RunEvent, RunId, RunNoticeLevel}; +use ::fabro_types::{ExecOutputTail, RunEvent, RunId, RunNoticeLevel}; use chrono::Utc; use fabro_agent::{WorktreeEvent, WorktreeEventCallback}; @@ -86,6 +86,22 @@ impl Emitter { level, code: code.into(), message: message.into(), + exec_output_tail: None, + }); + } + + pub fn notice_with_tail( + &self, + level: RunNoticeLevel, + code: impl Into, + message: impl Into, + exec_output_tail: Option, + ) { + self.emit(&Event::RunNotice { + level, + code: code.into(), + message: message.into(), + exec_output_tail, }); } diff --git a/lib/crates/fabro-workflow/src/event/events.rs b/lib/crates/fabro-workflow/src/event/events.rs index d54e95944..a3b2e2b5b 100644 --- a/lib/crates/fabro-workflow/src/event/events.rs +++ b/lib/crates/fabro-workflow/src/event/events.rs @@ -126,9 +126,11 @@ pub enum Event { final_patch: Option, }, RunNotice { - level: RunNoticeLevel, - code: String, - message: String, + level: RunNoticeLevel, + code: String, + message: String, + #[serde(default, skip_serializing_if = "Option::is_none")] + exec_output_tail: Option, }, MetadataSnapshotStarted { phase: fabro_types::MetadataSnapshotPhase, @@ -311,8 +313,10 @@ pub enum Event { diff: Option, }, CheckpointFailed { - node_id: String, - error: String, + node_id: String, + error: String, + #[serde(default, skip_serializing_if = "Option::is_none")] + exec_output_tail: Option, }, GitCommit { #[serde(default, skip_serializing_if = "Option::is_none")] @@ -320,8 +324,10 @@ pub enum Event { sha: String, }, GitPush { - branch: String, - success: bool, + branch: String, + success: bool, + #[serde(default, skip_serializing_if = "Option::is_none")] + exec_output_tail: Option, }, GitBranch { branch: String, @@ -588,8 +594,10 @@ pub enum Event { retro: Option, }, RetroFailed { - error: String, - duration_ms: u64, + error: String, + duration_ms: u64, + #[serde(default, skip_serializing_if = "Option::is_none")] + exec_output_tail: Option, }, } @@ -699,15 +707,49 @@ impl Event { level, code, message, + exec_output_tail, } => match level { RunNoticeLevel::Info => { - info!(code, message, "Run notice"); + let tail = + fabro_types::ExecOutputTail::trace_summary(exec_output_tail.as_ref()); + info!( + code, + message, + exec_output_tail_present = tail.present, + exec_stdout_tail_bytes = tail.stdout_bytes, + exec_stderr_tail_bytes = tail.stderr_bytes, + exec_stdout_truncated = tail.stdout_truncated, + exec_stderr_truncated = tail.stderr_truncated, + "Run notice" + ); } RunNoticeLevel::Warn => { - warn!(code, message, "Run notice"); + let tail = + fabro_types::ExecOutputTail::trace_summary(exec_output_tail.as_ref()); + warn!( + code, + message, + exec_output_tail_present = tail.present, + exec_stdout_tail_bytes = tail.stdout_bytes, + exec_stderr_tail_bytes = tail.stderr_bytes, + exec_stdout_truncated = tail.stdout_truncated, + exec_stderr_truncated = tail.stderr_truncated, + "Run notice" + ); } RunNoticeLevel::Error => { - error!(code, message, "Run notice"); + let tail = + fabro_types::ExecOutputTail::trace_summary(exec_output_tail.as_ref()); + error!( + code, + message, + exec_output_tail_present = tail.present, + exec_stdout_tail_bytes = tail.stdout_bytes, + exec_stderr_tail_bytes = tail.stderr_bytes, + exec_stdout_truncated = tail.stdout_truncated, + exec_stderr_truncated = tail.stderr_truncated, + "Run notice" + ); } }, Self::MetadataSnapshotStarted { phase, branch } => { @@ -906,8 +948,22 @@ impl Event { "Checkpoint completed" ); } - Self::CheckpointFailed { node_id, error } => { - error!(node_id, error, "Checkpoint failed"); + Self::CheckpointFailed { + node_id, + error, + exec_output_tail, + } => { + let tail = fabro_types::ExecOutputTail::trace_summary(exec_output_tail.as_ref()); + error!( + node_id, + error, + exec_output_tail_present = tail.present, + exec_stdout_tail_bytes = tail.stdout_bytes, + exec_stderr_tail_bytes = tail.stderr_bytes, + exec_stdout_truncated = tail.stdout_truncated, + exec_stderr_truncated = tail.stderr_truncated, + "Checkpoint failed" + ); } Self::GitCommit { node_id, sha } => { debug!( @@ -915,11 +971,25 @@ impl Event { sha, "Git commit" ); } - Self::GitPush { branch, success } => { + Self::GitPush { + branch, + success, + exec_output_tail, + } => { if *success { debug!(branch, "Git push succeeded"); } else { - warn!(branch, "Git push failed"); + let tail = + fabro_types::ExecOutputTail::trace_summary(exec_output_tail.as_ref()); + warn!( + branch, + exec_output_tail_present = tail.present, + exec_stdout_tail_bytes = tail.stdout_bytes, + exec_stderr_tail_bytes = tail.stderr_bytes, + exec_stdout_truncated = tail.stdout_truncated, + exec_stderr_truncated = tail.stderr_truncated, + "Git push failed" + ); } } Self::GitBranch { branch, sha } => { @@ -1275,8 +1345,22 @@ impl Event { Self::RetroCompleted { duration_ms, .. } => { info!(duration_ms, "Retro completed"); } - Self::RetroFailed { error, duration_ms } => { - error!(error = %error, duration_ms, "Retro failed"); + Self::RetroFailed { + error, + duration_ms, + exec_output_tail, + } => { + let tail = fabro_types::ExecOutputTail::trace_summary(exec_output_tail.as_ref()); + error!( + error = %error, + duration_ms, + exec_output_tail_present = tail.present, + exec_stdout_tail_bytes = tail.stdout_bytes, + exec_stderr_tail_bytes = tail.stderr_bytes, + exec_stdout_truncated = tail.stdout_truncated, + exec_stderr_truncated = tail.stderr_truncated, + "Retro failed" + ); } } } diff --git a/lib/crates/fabro-workflow/src/event/sink.rs b/lib/crates/fabro-workflow/src/event/sink.rs index f1d98e695..25bf392f6 100644 --- a/lib/crates/fabro-workflow/src/event/sink.rs +++ b/lib/crates/fabro-workflow/src/event/sink.rs @@ -232,9 +232,10 @@ mod tests { ); let run_store = store.create_run(&fixtures::RUN_7).await.unwrap(); let stored = to_run_event(&fixtures::RUN_7, &Event::RunNotice { - level: RunNoticeLevel::Warn, - code: "example".to_string(), - message: "notice".to_string(), + level: RunNoticeLevel::Warn, + code: "example".to_string(), + message: "notice".to_string(), + exec_output_tail: None, }); let payload = build_redacted_event_payload(&stored, &fixtures::RUN_7).unwrap(); run_store.append_event(&payload).await.unwrap(); diff --git a/lib/crates/fabro-workflow/src/handler/parallel.rs b/lib/crates/fabro-workflow/src/handler/parallel.rs index 5dd55c1d4..39862586c 100644 --- a/lib/crates/fabro-workflow/src/handler/parallel.rs +++ b/lib/crates/fabro-workflow/src/handler/parallel.rs @@ -203,7 +203,10 @@ impl Handler for ParallelHandler { return Err(Error::handler_with_source("sandbox git unavailable", &e)); } Err(e) => { - tracing::warn!(error = %e, "parallel base checkpoint failed"); + tracing::warn!( + error = %fabro_sandbox::display_for_log(&e), + "parallel base checkpoint failed" + ); None } } diff --git a/lib/crates/fabro-workflow/src/lifecycle/event.rs b/lib/crates/fabro-workflow/src/lifecycle/event.rs index 7f66fcf6d..482f0311a 100644 --- a/lib/crates/fabro-workflow/src/lifecycle/event.rs +++ b/lib/crates/fabro-workflow/src/lifecycle/event.rs @@ -413,10 +413,11 @@ impl RunLifecycle for EventLifecycle { &scope, ); } - for (branch, success) in &result.push_results { + for push in &result.push_results { self.emitter.emit(&Event::GitPush { - branch: branch.clone(), - success: *success, + branch: push.refspec.clone(), + success: push.success, + exec_output_tail: push.exec_output_tail.clone(), }); } } diff --git a/lib/crates/fabro-workflow/src/lifecycle/git.rs b/lib/crates/fabro-workflow/src/lifecycle/git.rs index 86cf5d7fd..0e0e9f195 100644 --- a/lib/crates/fabro-workflow/src/lifecycle/git.rs +++ b/lib/crates/fabro-workflow/src/lifecycle/git.rs @@ -10,7 +10,7 @@ use fabro_core::state::ExecutionState; use fabro_dump::RunDump; use fabro_types::RunId; use fabro_types::run_event::{MetadataSnapshotFailureKind, MetadataSnapshotPhase}; -use fabro_util::error::{collect_causes, render_with_causes}; +use fabro_util::error::collect_causes; use fabro_util::time::elapsed_ms; use crate::artifact; @@ -59,10 +59,17 @@ fn build_checkpoint( #[derive(Debug, Clone)] pub(crate) struct GitCheckpointResult { pub commit_sha: Option, - pub push_results: Vec<(String, bool)>, + pub push_results: Vec, pub diff: Option, } +#[derive(Debug, Clone)] +pub(crate) struct PushResult { + pub refspec: String, + pub success: bool, + pub exec_output_tail: Option, +} + /// Sub-lifecycle responsible for git operations (checkpoint commits, pushes, /// diffs). pub(crate) struct GitLifecycle { @@ -274,18 +281,25 @@ impl RunLifecycle for GitLifecycle { .and_then(|g| g.run_branch.as_ref()) { let refspec = format!("refs/heads/{branch}:refs/heads/{branch}"); - let push_ok = match self.sandbox.git_push_ref(&refspec).await { - Ok(()) => true, - Err(err) => { - tracing::warn!( - refspec = %refspec, - error = %err, - "git push from run lifecycle failed" - ); - false - } - }; - git_result.push_results.push((refspec, push_ok)); + let (push_ok, exec_output_tail) = + match self.sandbox.git_push_ref(&refspec).await { + Ok(()) => (true, None), + Err(err) => { + let exec_output_tail = + fabro_sandbox::default_redacted_output_tail(&err); + tracing::warn!( + refspec = %refspec, + error = %fabro_sandbox::display_for_log(&err), + "git push from run lifecycle failed" + ); + (false, exec_output_tail) + } + }; + git_result.push_results.push(PushResult { + refspec, + success: push_ok, + exec_output_tail, + }); } } @@ -303,11 +317,14 @@ impl RunLifecycle for GitLifecycle { } Ok(_) => {} Err(err) => { - self.emitter.emit(&Event::RunNotice { - level: RunNoticeLevel::Warn, - code: "git_diff_failed".to_string(), - message: format!("[node: {node_id}] git diff failed: {err}"), - }); + let exec_output_tail = + fabro_sandbox::default_redacted_output_tail(&err); + self.emitter.notice_with_tail( + RunNoticeLevel::Warn, + "git_diff_failed", + format!("[node: {node_id}] git diff failed: {err}"), + exec_output_tail, + ); } } } @@ -317,13 +334,15 @@ impl RunLifecycle for GitLifecycle { *self.checkpoint_git_result.lock().unwrap() = Some(git_result); } Err(e) => { - let error = render_with_causes(&e.to_string(), &collect_causes(&e)); + let exec_output_tail = fabro_sandbox::default_redacted_output_tail(&e); + let error = e.to_string(); // Emit CheckpointFailed and return error let scope = stage_scope_for(state, node_id); self.emitter.emit_scoped( &Event::CheckpointFailed { node_id: node_id.to_string(), - error: error.clone(), + error: error.clone(), + exec_output_tail, }, &scope, ); @@ -472,6 +491,7 @@ impl GitLifecycle { commit_sha, entry_count, bytes, + // TODO: thread exec_output_tail when an exec-backed metadata path lands. exec_output_tail: None, }, scope, @@ -492,6 +512,7 @@ impl GitLifecycle { level: RunNoticeLevel::Warn, code: code.to_string(), message, + exec_output_tail: None, }); } } diff --git a/lib/crates/fabro-workflow/src/operations/start.rs b/lib/crates/fabro-workflow/src/operations/start.rs index a67f72eba..4e623bcec 100644 --- a/lib/crates/fabro-workflow/src/operations/start.rs +++ b/lib/crates/fabro-workflow/src/operations/start.rs @@ -948,9 +948,10 @@ impl Drop for DetachedRunCompletionGuard { }) .await; let _ = append_event_to_sink(&event_sink, &run_id, &Event::RunNotice { - level: RunNoticeLevel::Error, - code: code.to_string(), - message: message.to_string(), + level: RunNoticeLevel::Error, + code: code.to_string(), + message: message.to_string(), + exec_output_tail: None, }) .await; }); @@ -981,9 +982,10 @@ async fn persist_detached_failure( } let event = Event::RunNotice { - level: RunNoticeLevel::Error, - code: format!("{phase}_failed"), - message: message.clone(), + level: RunNoticeLevel::Error, + code: format!("{phase}_failed"), + message: message.clone(), + exec_output_tail: None, }; if let Err(err) = append_event_to_sink(event_sink, &run_id, &event).await { tracing::warn!(error = %err, "Failed to append detached failure notice"); diff --git a/lib/crates/fabro-workflow/src/pipeline/finalize.rs b/lib/crates/fabro-workflow/src/pipeline/finalize.rs index 4f60ba2ca..921697369 100644 --- a/lib/crates/fabro-workflow/src/pipeline/finalize.rs +++ b/lib/crates/fabro-workflow/src/pipeline/finalize.rs @@ -468,7 +468,7 @@ async fn cleanup_sandbox( run_id: &fabro_types::RunId, workflow_name: &str, preserve: bool, -) -> std::result::Result<(), String> { +) -> fabro_sandbox::Result<()> { let hook_ctx = HookContext::new( HookEvent::SandboxCleanup, *run_id, @@ -476,11 +476,7 @@ async fn cleanup_sandbox( ); let _ = services.run_hooks(&hook_ctx).await; if !preserve { - services - .sandbox - .cleanup() - .await - .map_err(|e| e.display_with_causes())?; + services.sandbox.cleanup().await?; } Ok(()) } @@ -572,11 +568,13 @@ pub async fn finalize(retroed: Retroed, options: &FinalizeOptions) -> Result Option { + err.chain() + .find_map(fabro_sandbox::default_redacted_output_tail) +} + pub async fn run_retro(options: &RetroOptions, dry_run: bool) -> Option { let services = &options.services; let state = match services.run_store.state().await { @@ -18,8 +23,9 @@ pub async fn run_retro(options: &RetroOptions, dry_run: bool) -> Option { Err(e) => { tracing::warn!(error = %e, "Could not load run state, skipping retro"); services.emitter.emit(&Event::RetroFailed { - error: e.to_string(), - duration_ms: 0, + error: e.to_string(), + duration_ms: 0, + exec_output_tail: exec_output_tail_from_anyhow(&e), }); return None; } @@ -27,8 +33,9 @@ pub async fn run_retro(options: &RetroOptions, dry_run: bool) -> Option { let Some(ref cp) = state.checkpoint else { tracing::warn!("Could not load checkpoint, skipping retro"); services.emitter.emit(&Event::RetroFailed { - error: "checkpoint not found".to_string(), - duration_ms: 0, + error: "checkpoint not found".to_string(), + duration_ms: 0, + exec_output_tail: None, }); return None; }; @@ -39,8 +46,9 @@ pub async fn run_retro(options: &RetroOptions, dry_run: bool) -> Option { Err(err) => { tracing::warn!(error = %err, "Could not load events from store, skipping retro"); services.emitter.emit(&Event::RetroFailed { - error: err.to_string(), - duration_ms: 0, + error: err.to_string(), + duration_ms: 0, + exec_output_tail: exec_output_tail_from_anyhow(&err), }); return None; } @@ -129,6 +137,7 @@ pub async fn run_retro(options: &RetroOptions, dry_run: bool) -> Option { services.emitter.emit(&Event::RetroFailed { error: e.to_string(), duration_ms, + exec_output_tail: exec_output_tail_from_anyhow(&e), }); tracing::debug!(error = %e, "Retro agent skipped"); } diff --git a/lib/crates/fabro-workflow/src/sandbox_git.rs b/lib/crates/fabro-workflow/src/sandbox_git.rs index 38580296c..6d10e2b07 100644 --- a/lib/crates/fabro-workflow/src/sandbox_git.rs +++ b/lib/crates/fabro-workflow/src/sandbox_git.rs @@ -11,6 +11,14 @@ use crate::artifact_snapshot; use crate::git::GitAuthor; use crate::sandbox_git_runtime::SandboxGitRuntime; +#[derive(Debug, thiserror::Error)] +#[error("{message}")] +pub struct GitCommandError { + pub message: String, + #[source] + pub source: fabro_sandbox::Error, +} + /// Captured git state for a workflow run, shared with handlers. #[derive(Debug, Clone)] pub struct GitState { @@ -25,20 +33,24 @@ pub struct GitState { 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) -> String { +pub(crate) fn exec_err(label: &str, r: fabro_sandbox::ExecResult) -> GitCommandError { if r.is_timed_out() { - return format!("{label} timed out after {}ms", r.duration_ms); + return GitCommandError { + message: format!("{label} timed out after {}ms", r.duration_ms), + source: fabro_sandbox::Error::exec(label, r), + }; } if r.is_cancelled() { - return format!("{label} cancelled after {}ms", r.duration_ms); + return GitCommandError { + message: format!("{label} cancelled after {}ms", r.duration_ms), + source: fabro_sandbox::Error::exec(label, r), + }; } - let detail = format!("{}{}", r.stdout, r.stderr); - let detail = detail.trim(); - if detail.is_empty() { - format!("{label} killed (exit {}, no output)", r.display_exit_code()) - } else { - format!("{label} failed (exit {}): {detail}", r.display_exit_code()) + let exit = r.display_exit_code(); + GitCommandError { + message: format!("{label} failed (exit {exit})"), + source: fabro_sandbox::Error::exec(label, r), } } @@ -56,7 +68,7 @@ pub async fn git_checkpoint( shadow_sha: Option, exclude_globs: &[String], author: &GitAuthor, -) -> std::result::Result { +) -> std::result::Result { let mut all_excludes: Vec = artifact_snapshot::EXCLUDE_DIRS .iter() .map(|d| format!("**/{d}/**")) @@ -71,10 +83,15 @@ pub async fn git_checkpoint( let add_result = sandbox .exec_command(&add_cmd, 30_000, None, None, None) .await; - match &add_result { + match add_result { Ok(r) if r.is_success() => {} Ok(r) => return Err(exec_err("git add", r)), - Err(e) => return Err(format!("git add failed: {e}")), + Err(e) => { + return Err(GitCommandError { + message: "git add failed".to_string(), + source: e, + }); + } } let subject = format!("fabro({run_id}): {node_id} ({status})"); @@ -101,7 +118,10 @@ pub async fn git_checkpoint( let msg_path = format!("/tmp/fabro-commit-msg-{run_id}-{node_id}"); if let Err(e) = sandbox.write_file(&msg_path, &message).await { - return Err(format!("failed to write commit message file: {e}")); + return Err(GitCommandError { + message: "failed to write commit message file".to_string(), + source: e, + }); } let commit_cmd = format!( @@ -112,10 +132,15 @@ pub async fn git_checkpoint( let commit_result = sandbox .exec_command(&commit_cmd, 30_000, None, None, None) .await; - match &commit_result { + match commit_result { Ok(r) if r.is_success() => {} Ok(r) => return Err(exec_err("git commit", r)), - Err(e) => return Err(format!("git commit failed: {e}")), + Err(e) => { + return Err(GitCommandError { + message: "git commit failed".to_string(), + source: e, + }); + } } let sha_cmd = format!("{GIT_REMOTE} rev-parse HEAD"); @@ -124,8 +149,11 @@ pub async fn git_checkpoint( .await; match sha_result { Ok(r) if r.is_success() => Ok(r.stdout.trim().to_string()), - Ok(r) => Err(exec_err("git rev-parse HEAD", &r)), - Err(e) => Err(format!("git rev-parse HEAD failed: {e}")), + Ok(r) => Err(exec_err("git rev-parse HEAD", r)), + Err(e) => Err(GitCommandError { + message: "git rev-parse HEAD failed".to_string(), + source: e, + }), } } @@ -159,14 +187,14 @@ pub(crate) async fn checked_git_checkpoint( author, ) .await - .map_err(|err| SharedError::new(anyhow::anyhow!(err))) + .map_err(|err| SharedError::new(anyhow::Error::new(err))) } /// Run a git diff via the sandbox (30 s default timeout). pub(crate) async fn git_diff( sandbox: &dyn Sandbox, base: &str, -) -> std::result::Result { +) -> std::result::Result { git_diff_with_timeout(sandbox, base, 30_000).await } @@ -180,7 +208,7 @@ pub(crate) async fn git_diff_with_timeout( sandbox: &dyn Sandbox, base: &str, timeout_ms: u64, -) -> std::result::Result { +) -> std::result::Result { // `-c core.quotePath=false` forces paths with non-ASCII, tabs, quotes, // or backslashes to emit unquoted. The Run Files Changed endpoint's // `strip_denylisted_sections` parser only recognizes unquoted @@ -193,8 +221,11 @@ pub(crate) async fn git_diff_with_timeout( .await { Ok(r) if r.is_success() => Ok(r.stdout), - Ok(r) => Err(exec_err("git diff", &r)), - Err(e) => Err(e.display_with_causes()), + Ok(r) => Err(exec_err("git diff", r)), + Err(e) => Err(GitCommandError { + message: "git diff failed".to_string(), + source: e, + }), } } @@ -979,7 +1010,11 @@ mod tests { .await .unwrap_err(); - assert_eq!(err, "git add timed out after 77ms"); + assert_eq!(err.to_string(), "git add timed out after 77ms"); + assert!( + fabro_sandbox::default_redacted_output_tail(&err).is_none(), + "empty exec streams should not produce a tail" + ); } #[tokio::test] @@ -1001,7 +1036,7 @@ mod tests { .await .unwrap_err(); - let chain = anyhow::Error::new(err) + let chain = anyhow::Error::new(err.clone()) .chain() .map(ToString::to_string) .collect::>(); @@ -1010,8 +1045,8 @@ mod tests { "expected sandbox git context, got {chain:#?}" ); assert!( - chain.iter().any(|cause| cause.contains("git missing")), - "expected probe stderr in chain, got {chain:#?}" + fabro_sandbox::default_redacted_output_tail(&err).is_some(), + "expected probe exec output tail to survive SharedError wrapping" ); } @@ -1031,7 +1066,7 @@ mod tests { .await .unwrap_err(); - assert_eq!(err, "git commit timed out after 88ms"); + assert_eq!(err.to_string(), "git commit timed out after 88ms"); } #[tokio::test] @@ -1050,7 +1085,7 @@ mod tests { .await .unwrap_err(); - assert_eq!(err, "git rev-parse HEAD killed (exit -1, no output)"); + assert_eq!(err.to_string(), "git rev-parse HEAD failed (exit -1)"); } #[tokio::test] @@ -1060,7 +1095,7 @@ mod tests { .await .unwrap_err(); - assert_eq!(err, "git diff timed out after 99ms"); + assert_eq!(err.to_string(), "git diff timed out after 99ms"); } #[tokio::test] @@ -1070,7 +1105,11 @@ mod tests { .await .unwrap_err(); - assert_eq!(err, "git diff failed (exit 128): fatal: bad revision"); + assert_eq!(err.to_string(), "git diff failed (exit 128)"); + assert!(!err.to_string().contains("fatal: bad revision")); + + let tail = fabro_sandbox::default_redacted_output_tail(&err).expect("tail present"); + assert_eq!(tail.stderr.as_deref(), Some("fatal: bad revision\n")); } #[tokio::test] diff --git a/lib/crates/fabro-workflow/src/sandbox_git_runtime.rs b/lib/crates/fabro-workflow/src/sandbox_git_runtime.rs index 29fcfda47..2bd246e13 100644 --- a/lib/crates/fabro-workflow/src/sandbox_git_runtime.rs +++ b/lib/crates/fabro-workflow/src/sandbox_git_runtime.rs @@ -71,8 +71,8 @@ async fn exec_ok(sandbox: &dyn Sandbox, command: &str) -> Result<(), SharedError if result.is_success() { Ok(()) } else { - Err(SharedError::new(anyhow::anyhow!(exec_err( - command, &result + Err(SharedError::new(anyhow::Error::new(exec_err( + command, result, )))) } }