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, )))) } }