From 32166e4cdfcfc7a9989b0f8c94b0fa955c14d485 Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Wed, 29 Apr 2026 12:52:30 -0400 Subject: [PATCH] refactor(sandbox): simplify exec error helpers and metadata snapshot Inline static credential-refresh failure tags instead of round-tripping through a classifier whose substring matches always returned the sentinel its callers prepended. Drop the dead `Error::Exec` accessors in favor of pattern matching, and replace the redundant `MetadataSnapshot::pushed` field with `push_error.is_none()`. Also fix a regression in Docker `refresh_push_credentials` that was discarding stderr and exit code on `set_url_nonzero` failures. Co-Authored-By: Claude Opus 4.7 (1M context) --- lib/crates/fabro-sandbox/src/daytona/mod.rs | 13 ++-- lib/crates/fabro-sandbox/src/docker.rs | 15 ++-- lib/crates/fabro-sandbox/src/error.rs | 70 ------------------- lib/crates/fabro-sandbox/src/redact.rs | 50 ------------- lib/crates/fabro-sandbox/src/sandbox.rs | 17 +++-- .../fabro-workflow/src/lifecycle/git.rs | 3 +- .../fabro-workflow/src/pipeline/finalize.rs | 3 +- lib/crates/fabro-workflow/src/sandbox_git.rs | 2 - .../fabro-workflow/src/sandbox_metadata.rs | 11 +-- 9 files changed, 33 insertions(+), 151 deletions(-) diff --git a/lib/crates/fabro-sandbox/src/daytona/mod.rs b/lib/crates/fabro-sandbox/src/daytona/mod.rs index a50a5e8b7..3a51ac505 100644 --- a/lib/crates/fabro-sandbox/src/daytona/mod.rs +++ b/lib/crates/fabro-sandbox/src/daytona/mod.rs @@ -13,7 +13,7 @@ use tokio::{fs, time}; use tokio_util::sync::CancellationToken; use crate::clone_source::{self, CloneDecision, EmptyWorkspaceReason}; -use crate::redact::{classify_credential_refresh_failure, redact_auth_url}; +use crate::redact::redact_auth_url; use crate::sandbox::resolve_path; use crate::{ DirEntry, ExecResult, GrepOptions, Sandbox, SandboxEvent, SandboxEventCallback, @@ -878,9 +878,8 @@ impl Sandbox for DaytonaSandbox { origin_url, ) .await - .map_err(|e| { - let class = classify_credential_refresh_failure(&format!("token_mint_failed: {e}")); - crate::Error::message(format!("Failed to refresh push credentials: {class}")) + .map_err(|_| { + crate::Error::message("Failed to refresh push credentials: token_mint_failed") })?; let cmd = format!( @@ -890,10 +889,8 @@ impl Sandbox for DaytonaSandbox { let result = self .exec_command(&cmd, 10_000, None, None, None) .await - .map_err(|e| { - let class = - classify_credential_refresh_failure(&format!("set_url_exec_failed: {e}")); - crate::Error::message(format!("Failed to refresh push credentials: {class}")) + .map_err(|_| { + crate::Error::message("Failed to refresh push credentials: set_url_exec_failed") })?; if result.exit_code != 0 { return Err(result.into_exec_error_with_redactor( diff --git a/lib/crates/fabro-sandbox/src/docker.rs b/lib/crates/fabro-sandbox/src/docker.rs index d1c296442..47bd60acd 100644 --- a/lib/crates/fabro-sandbox/src/docker.rs +++ b/lib/crates/fabro-sandbox/src/docker.rs @@ -22,7 +22,7 @@ use tokio::{fs, time}; use tokio_util::sync::CancellationToken; use crate::clone_source::{self, CloneDecision, EmptyWorkspaceReason}; -use crate::redact::{classify_credential_refresh_failure, redact_auth_url}; +use crate::redact::redact_auth_url; use crate::sandbox::resolve_path; use crate::{ DirEntry, ExecResult, GrepOptions, Sandbox, SandboxEvent, SandboxEventCallback, @@ -1250,9 +1250,8 @@ impl Sandbox for DockerSandbox { origin_url, ) .await - .map_err(|e| { - let class = classify_credential_refresh_failure(&format!("token_mint_failed: {e}")); - crate::Error::message(format!("Failed to refresh push credentials: {class}")) + .map_err(|_| { + crate::Error::message("Failed to refresh push credentials: token_mint_failed") })?; let command = format!( @@ -1263,10 +1262,10 @@ impl Sandbox for DockerSandbox { .docker_exec_shell(&command, 10_000, Some(WORKING_DIRECTORY), None, None) .await?; if result.exit_code != 0 { - let class = classify_credential_refresh_failure("set_url_nonzero"); - return Err(crate::Error::message(format!( - "Failed to refresh push credentials: {class}" - ))); + return Err(result.into_exec_error_with_redactor( + "git remote set-url origin (refresh push credentials)", + |s| redact_auth_url(s, Some(&auth_url)), + )); } Ok(()) diff --git a/lib/crates/fabro-sandbox/src/error.rs b/lib/crates/fabro-sandbox/src/error.rs index 281e34270..be2592535 100644 --- a/lib/crates/fabro-sandbox/src/error.rs +++ b/lib/crates/fabro-sandbox/src/error.rs @@ -86,48 +86,6 @@ impl Error { } } - pub fn exec_stderr(&self) -> Option<&str> { - match self { - Self::Exec { stderr, .. } => Some(stderr), - _ => None, - } - } - - pub fn exec_stdout(&self) -> Option<&str> { - match self { - Self::Exec { stdout, .. } => Some(stdout), - _ => None, - } - } - - pub fn exec_label(&self) -> Option<&str> { - match self { - Self::Exec { label, .. } => Some(label), - _ => None, - } - } - - pub fn exec_exit_code(&self) -> Option { - match self { - Self::Exec { exit_code, .. } => Some(*exit_code), - _ => None, - } - } - - pub fn exec_timed_out(&self) -> Option { - match self { - Self::Exec { timed_out, .. } => Some(*timed_out), - _ => None, - } - } - - pub fn exec_duration_ms(&self) -> Option { - match self { - Self::Exec { duration_ms, .. } => Some(*duration_ms), - _ => None, - } - } - #[cfg(feature = "docker")] pub fn docker_connect(source: BollardError) -> Self { Self::DockerConnect { source } @@ -246,34 +204,6 @@ mod tests { assert!(rendered.contains("hint:")); } - #[test] - fn exec_accessors_return_stored_values() { - let error = Error::exec("git push", 1, true, 5000, "stored stderr", "stored stdout"); - - assert_eq!(error.exec_label(), Some("git push")); - assert_eq!(error.exec_exit_code(), Some(1)); - assert_eq!(error.exec_timed_out(), Some(true)); - assert_eq!(error.exec_duration_ms(), Some(5000)); - assert_eq!(error.exec_stderr(), Some("stored stderr")); - assert_eq!(error.exec_stdout(), Some("stored stdout")); - } - - #[test] - fn non_exec_accessors_return_none() { - let message = Error::message("plain"); - assert_eq!(message.exec_label(), None); - assert_eq!(message.exec_exit_code(), None); - assert_eq!(message.exec_timed_out(), None); - assert_eq!(message.exec_duration_ms(), None); - assert_eq!(message.exec_stderr(), None); - assert_eq!(message.exec_stdout(), None); - - let source = std::io::Error::other("source"); - let context = Error::context("context", source); - assert_eq!(context.exec_label(), None); - assert_eq!(context.exec_stderr(), None); - } - #[test] fn classify_exec_failure_documents_known_branches() { let cases = [ diff --git a/lib/crates/fabro-sandbox/src/redact.rs b/lib/crates/fabro-sandbox/src/redact.rs index 4e9666641..f74ac6194 100644 --- a/lib/crates/fabro-sandbox/src/redact.rs +++ b/lib/crates/fabro-sandbox/src/redact.rs @@ -7,53 +7,3 @@ pub(crate) fn redact_auth_url( }; text.replace(&auth_url.raw_string(), &auth_url.redacted_string()) } - -pub(crate) fn classify_credential_refresh_failure(inner: &str) -> &'static str { - let lower = inner.to_ascii_lowercase(); - if lower.contains("set_url_exec_failed") - || lower.contains("execute command") - || lower.contains("failed to execute") - { - "set_url_exec_failed" - } else if lower.contains("set_url_nonzero") - || lower.contains("remote set-url") - || lower.contains("set refreshed push credentials") - || lower.contains("exit ") - { - "set_url_nonzero" - } else if lower.contains("token_mint_failed") - || lower.contains("github app") - || lower.contains("installation") - || lower.contains("private key") - || lower.contains("token") - { - "token_mint_failed" - } else { - "unclassified" - } -} - -#[cfg(test)] -mod tests { - use super::*; - - #[test] - fn classify_credential_refresh_failure_documents_known_branches() { - assert_eq!( - classify_credential_refresh_failure("GitHub App installation token request failed"), - "token_mint_failed" - ); - assert_eq!( - classify_credential_refresh_failure("set_url_exec_failed: sdk echoed command argv"), - "set_url_exec_failed" - ); - assert_eq!( - classify_credential_refresh_failure("git remote set-url origin failed with exit 128"), - "set_url_nonzero" - ); - assert_eq!( - classify_credential_refresh_failure("new provider error"), - "unclassified" - ); - } -} diff --git a/lib/crates/fabro-sandbox/src/sandbox.rs b/lib/crates/fabro-sandbox/src/sandbox.rs index 8cb744b29..82bc8c6b7 100644 --- a/lib/crates/fabro-sandbox/src/sandbox.rs +++ b/lib/crates/fabro-sandbox/src/sandbox.rs @@ -751,8 +751,14 @@ mod tests { duration_ms: 42, }; let error = result.into_result("git push").unwrap_err(); - assert_eq!(error.exec_label(), Some("git push")); - assert_eq!(error.exec_exit_code(), Some(128)); + let crate::Error::Exec { + label, exit_code, .. + } = &error + else { + panic!("expected Error::Exec, got {error:?}"); + }; + assert_eq!(label, "git push"); + assert_eq!(*exit_code, 128); assert!(error.to_string().contains("no credentials in origin URL")); } @@ -787,8 +793,11 @@ mod tests { s.replace("https://token@example.com", "https://****@example.com") }); - assert_eq!(error.exec_stderr(), Some("stderr https://****@example.com")); - assert_eq!(error.exec_stdout(), Some("stdout https://****@example.com")); + let crate::Error::Exec { stderr, stdout, .. } = &error else { + panic!("expected Error::Exec, got {error:?}"); + }; + assert_eq!(stderr, "stderr https://****@example.com"); + assert_eq!(stdout, "stdout https://****@example.com"); } #[test] diff --git a/lib/crates/fabro-workflow/src/lifecycle/git.rs b/lib/crates/fabro-workflow/src/lifecycle/git.rs index e9b229780..3dc420676 100644 --- a/lib/crates/fabro-workflow/src/lifecycle/git.rs +++ b/lib/crates/fabro-workflow/src/lifecycle/git.rs @@ -258,8 +258,7 @@ impl GitLifecycle { ); match writer.write_snapshot(dump, message).await { Ok(snapshot) => { - if !snapshot.pushed { - let detail = snapshot.push_error.as_deref().unwrap_or("unknown error"); + if let Some(detail) = snapshot.push_error.as_deref() { self.emit_metadata_warning( "checkpoint_metadata_push_failed", format!("failed to push metadata ref refs/heads/{meta_branch}: {detail}"), diff --git a/lib/crates/fabro-workflow/src/pipeline/finalize.rs b/lib/crates/fabro-workflow/src/pipeline/finalize.rs index 9a9620e3d..aa8c0b489 100644 --- a/lib/crates/fabro-workflow/src/pipeline/finalize.rs +++ b/lib/crates/fabro-workflow/src/pipeline/finalize.rs @@ -178,8 +178,7 @@ pub async fn write_finalize_commit( ); match writer.write_snapshot(&dump, "finalize run").await { Ok(snapshot) => { - if !snapshot.pushed { - let detail = snapshot.push_error.as_deref().unwrap_or("unknown error"); + if let Some(detail) = snapshot.push_error.as_deref() { emit_metadata_warning( services, "checkpoint_metadata_push_failed", diff --git a/lib/crates/fabro-workflow/src/sandbox_git.rs b/lib/crates/fabro-workflow/src/sandbox_git.rs index 4b3f1fd79..2df37bedc 100644 --- a/lib/crates/fabro-workflow/src/sandbox_git.rs +++ b/lib/crates/fabro-workflow/src/sandbox_git.rs @@ -1351,7 +1351,6 @@ mod tests { ); let snapshot = writer.write_snapshot(&dump, "checkpoint").await.unwrap(); - assert!(snapshot.pushed); assert_eq!(snapshot.push_error, None); let commit_sha = snapshot.commit_sha; @@ -1494,7 +1493,6 @@ mod tests { let snapshot = writer.write_snapshot(&dump, "checkpoint").await.unwrap(); - assert!(!snapshot.pushed); let push_error = snapshot.push_error.unwrap(); assert!(push_error.contains("git push origin")); assert!(push_error.contains("hint:")); diff --git a/lib/crates/fabro-workflow/src/sandbox_metadata.rs b/lib/crates/fabro-workflow/src/sandbox_metadata.rs index cbf91b4b0..9b83a9de6 100644 --- a/lib/crates/fabro-workflow/src/sandbox_metadata.rs +++ b/lib/crates/fabro-workflow/src/sandbox_metadata.rs @@ -77,7 +77,6 @@ pub(crate) struct SandboxMetadataWriter<'a> { pub(crate) struct MetadataSnapshot { pub commit_sha: String, - pub pushed: bool, pub push_error: Option, } @@ -179,12 +178,14 @@ impl<'a> SandboxMetadataWriter<'a> { .await?; let commit = parse_fast_import_mark(&stdout)?; let refspec = format!("{full_ref}:{full_ref}"); - let push_result = self.sandbox.git_push_ref(&refspec).await; - let pushed = push_result.is_ok(); - let push_error = push_result.err().map(|err| err.to_string()); + let push_error = self + .sandbox + .git_push_ref(&refspec) + .await + .err() + .map(|err| err.to_string()); Ok(MetadataSnapshot { commit_sha: commit, - pushed, push_error, }) }