diff --git a/lib/crates/fabro-sandbox/src/daytona/mod.rs b/lib/crates/fabro-sandbox/src/daytona/mod.rs index 168fe5ab7..a50a5e8b7 100644 --- a/lib/crates/fabro-sandbox/src/daytona/mod.rs +++ b/lib/crates/fabro-sandbox/src/daytona/mod.rs @@ -13,6 +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::sandbox::resolve_path; use crate::{ DirEntry, ExecResult, GrepOptions, Sandbox, SandboxEvent, SandboxEventCallback, @@ -54,6 +55,23 @@ fn elapsed_ms(start: &Instant) -> u64 { u64::try_from(start.elapsed().as_millis()).unwrap_or(u64::MAX) } +fn command_kind(command: &str) -> &'static str { + match command.split_whitespace().next().unwrap_or_default() { + "git" => "git", + "sh" | "/bin/sh" => "sh", + "bash" | "/bin/bash" => "bash", + "rg" => "rg", + "grep" => "grep", + "find" => "find", + "cat" => "cat", + "ls" => "ls", + "mkdir" => "mkdir", + "rm" => "rm", + "printf" => "printf", + _ => "other", + } +} + /// Sandbox that runs all operations inside a Daytona cloud sandbox. pub struct DaytonaSandbox { config: DaytonaConfig, @@ -639,22 +657,25 @@ impl Sandbox for DaytonaSandbox { let wrapped = wrap_bash_command(&cmd); match ps.execute_command(&wrapped, opts).await { Ok(r) if r.exit_code != 0 => { - let stderr = r.result.replace( - &auth_url.raw_string(), - &auth_url.redacted_string(), + let err = crate::Error::exec( + "git remote set-url origin (Daytona post-clone)", + r.exit_code, + false, + 0, + redact_auth_url(&r.result, Some(&auth_url)), + String::new(), ); tracing::warn!( - exit_code = r.exit_code, - output = %stderr.trim(), + error = %err, "Failed to set Daytona sandbox push credentials \ on origin — subsequent git push from this \ sandbox will fail" ); } Ok(_) => {} - Err(e) => { + Err(_) => { tracing::warn!( - error = %e, + error_class = "daytona_set_url_exec_failed", "Daytona exec failed while setting push credentials \ on origin — subsequent git push from this \ sandbox will fail" @@ -788,9 +809,9 @@ impl Sandbox for DaytonaSandbox { )] } - async fn git_push_ref(&self, refspec: &str) -> bool { + async fn git_push_ref(&self, refspec: &str) -> crate::Result<()> { if !self.repo_cloned() { - return false; + return Ok(()); } crate::git_push_via_exec(self, refspec).await } @@ -857,7 +878,10 @@ impl Sandbox for DaytonaSandbox { origin_url, ) .await - .map_err(|e| crate::Error::message(format!("Failed to refresh GitHub App token: {e}")))?; + .map_err(|e| { + let class = classify_credential_refresh_failure(&format!("token_mint_failed: {e}")); + crate::Error::message(format!("Failed to refresh push credentials: {class}")) + })?; let cmd = format!( "git -c maintenance.auto=0 remote set-url origin {}", @@ -866,15 +890,16 @@ impl Sandbox for DaytonaSandbox { let result = self .exec_command(&cmd, 10_000, None, None, None) .await - .map_err(|e| crate::Error::context("Failed to set refreshed push credentials", e))?; + .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}")) + })?; if result.exit_code != 0 { - let stderr = result - .stderr - .replace(&auth_url.raw_string(), &auth_url.redacted_string()); - return Err(crate::Error::message(format!( - "Failed to set refreshed push credentials (exit {}): {}", - result.exit_code, stderr - ))); + 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(()) @@ -1021,7 +1046,12 @@ impl Sandbox for DaytonaSandbox { env_vars: Option<&HashMap>, cancel_token: Option, ) -> crate::Result { - tracing::info!(command, timeout_ms, "exec_command: entered"); + tracing::info!( + timeout_ms, + command_kind = command_kind(command), + command_len = command.len(), + "exec_command: entered" + ); let sandbox = self.sandbox()?; let start = Instant::now(); @@ -1248,6 +1278,24 @@ mod tests { assert!(config.labels.is_none()); } + #[test] + fn command_kind_classifies_known_prefixes() { + assert_eq!(command_kind(" git status"), "git"); + assert_eq!(command_kind("bash -lc 'echo ok'"), "bash"); + assert_eq!(command_kind("/bin/sh -c 'echo ok'"), "sh"); + assert_eq!(command_kind("rg --version"), "rg"); + assert_eq!(command_kind("find . -maxdepth 1"), "find"); + assert_eq!(command_kind(""), "other"); + } + + #[test] + fn command_kind_does_not_echo_auth_url_commands() { + assert_eq!( + command_kind("https://x-access-token:ghs_FAKE@github.com/owner/repo.git"), + "other" + ); + } + #[test] fn wrap_bash_uses_base64_encoding() { let wrapped = wrap_bash_command("echo hello"); diff --git a/lib/crates/fabro-sandbox/src/docker.rs b/lib/crates/fabro-sandbox/src/docker.rs index 742854a50..d1c296442 100644 --- a/lib/crates/fabro-sandbox/src/docker.rs +++ b/lib/crates/fabro-sandbox/src/docker.rs @@ -22,6 +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::sandbox::resolve_path; use crate::{ DirEntry, ExecResult, GrepOptions, Sandbox, SandboxEvent, SandboxEventCallback, @@ -423,10 +424,12 @@ impl DockerSandbox { .docker_exec_shell(&command, 10_000, Some(WORKING_DIRECTORY), None, None) .await?; if result.exit_code != 0 { - let stderr = redact_auth_url(&result.stderr, Some(auth_url)); + let err = result + .into_exec_error_with_redactor("git remote set-url origin (post-clone)", |s| { + redact_auth_url(s, Some(auth_url)) + }); tracing::warn!( - exit_code = result.exit_code, - stderr = %stderr.trim(), + error = %err, "Failed to set Docker sandbox push credentials on origin — \ subsequent git push from this sandbox will fail" ); @@ -634,13 +637,6 @@ fn bash_remediation(error: &DockerError, image: &str) -> String { ) } -fn redact_auth_url(text: &str, auth_url: Option<&fabro_redact::DisplaySafeUrl>) -> String { - let Some(auth_url) = auth_url else { - return text.to_string(); - }; - text.replace(&auth_url.raw_string(), &auth_url.redacted_string()) -} - fn build_single_file_tar(file_name: &str, bytes: &[u8]) -> crate::Result> { let mut tar_builder = tar::Builder::new(Vec::new()); let mut header = tar::Header::new_gnu(); @@ -1208,9 +1204,9 @@ impl Sandbox for DockerSandbox { )] } - async fn git_push_ref(&self, refspec: &str) -> bool { + async fn git_push_ref(&self, refspec: &str) -> crate::Result<()> { if !self.repo_cloned() { - return false; + return Ok(()); } crate::git_push_via_exec(self, refspec).await } @@ -1254,7 +1250,10 @@ impl Sandbox for DockerSandbox { origin_url, ) .await - .map_err(|e| crate::Error::message(format!("Failed to refresh GitHub App token: {e}")))?; + .map_err(|e| { + let class = classify_credential_refresh_failure(&format!("token_mint_failed: {e}")); + crate::Error::message(format!("Failed to refresh push credentials: {class}")) + })?; let command = format!( "git -c maintenance.auto=0 remote set-url origin {}", @@ -1264,10 +1263,9 @@ impl Sandbox for DockerSandbox { .docker_exec_shell(&command, 10_000, Some(WORKING_DIRECTORY), None, None) .await?; if result.exit_code != 0 { - let stderr = redact_auth_url(&result.stderr, Some(&auth_url)); + let class = classify_credential_refresh_failure("set_url_nonzero"); return Err(crate::Error::message(format!( - "Failed to set refreshed push credentials (exit {}): {}", - result.exit_code, stderr + "Failed to refresh push credentials: {class}" ))); } diff --git a/lib/crates/fabro-sandbox/src/error.rs b/lib/crates/fabro-sandbox/src/error.rs index 030242f21..281e34270 100644 --- a/lib/crates/fabro-sandbox/src/error.rs +++ b/lib/crates/fabro-sandbox/src/error.rs @@ -36,6 +36,21 @@ pub enum Error { #[source] source: BollardError, }, + + #[error( + "{label} failed (exit {exit_code}, timed_out={timed_out}, duration_ms={duration_ms}) - hint: {hint}", + hint = classify_exec_failure(stderr) + .or_else(|| classify_exec_failure(stdout)) + .unwrap_or("unclassified") + )] + Exec { + label: String, + exit_code: i32, + timed_out: bool, + duration_ms: u64, + stderr: String, + stdout: String, + }, } impl Error { @@ -53,6 +68,66 @@ impl Error { } } + pub fn exec( + label: impl Into, + exit_code: i32, + timed_out: bool, + duration_ms: u64, + stderr: impl Into, + stdout: impl Into, + ) -> Self { + Self::Exec { + label: label.into(), + exit_code, + timed_out, + duration_ms, + stderr: stderr.into(), + stdout: stdout.into(), + } + } + + 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 } @@ -95,4 +170,145 @@ impl From<&str> for Error { } } +pub(crate) fn classify_exec_failure(stderr: &str) -> Option<&'static str> { + let lower = stderr.to_ascii_lowercase(); + if lower.contains("could not read username") || lower.contains("terminal prompts disabled") { + Some( + "no credentials in origin URL - check that the sandbox forwarded \ + GITHUB_APP_PRIVATE_KEY (or GITHUB_TOKEN) and that refresh_push_credentials succeeded", + ) + } else if lower.contains("permission to") && lower.contains("denied") { + Some( + "github denied the push - installation token lacks contents:write \ + on this repo, or a branch protection / push ruleset is rejecting the ref", + ) + } else if lower.contains("protected branch") + || lower.contains("ruleset") + || lower.contains("rejected") + { + Some("github rejected the ref - likely a branch protection rule or push ruleset") + } else if lower.contains("authentication failed") || lower.contains("invalid username") { + Some("github authentication failed - installation token may be expired or wrong scope") + } else if lower.contains("could not resolve host") || lower.contains("network is unreachable") { + Some("network failure inside sandbox - check DNS / egress from the run container") + } else if lower.contains("repository not found") { + Some("github 404 - the App installation may not include this repo") + } else if lower.contains("no such remote") && lower.contains("origin") { + Some("origin remote missing - push credentials could not be installed") + } else if lower.contains("not a git repository") + || lower.contains("does not appear to be a git repository") + { + Some("git repository unavailable in sandbox working directory") + } else { + None + } +} + pub type Result = std::result::Result; + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn exec_display_is_log_safe() { + let stderr = "fatal: unable to access \ + 'https://x-access-token:ghs_xK9mZ2vL8nQ5rT1wY4bC7dF0gH3jE6pA@github.com/owner/repo/':\n\ + remote: Permission to owner/repo.git denied\n\ + identity ~/.ssh/id_rsa_work"; + let error = Error::exec( + "git push origin refs/heads/run", + 128, + false, + 210, + stderr, + "", + ); + let rendered = error.to_string(); + + for forbidden in [ + "fatal:", + "remote:", + "x-access-token", + "ghs_xK9mZ2vL8nQ5rT1wY4bC7dF0gH3jE6pA", + "~/.ssh", + "id_rsa_work", + ] { + assert!( + !rendered.contains(forbidden), + "Display leaked {forbidden:?}: {rendered}" + ); + } + assert!(rendered.contains("git push origin refs/heads/run")); + assert!(rendered.contains("exit 128")); + assert!(rendered.contains("timed_out=false")); + assert!(rendered.contains("duration_ms=210")); + 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 = [ + ( + "fatal: could not read Username for 'https://github.com'", + "no credentials in origin URL", + ), + ( + "remote: Permission to owner/repo.git denied to fabro-app[bot].", + "github denied the push", + ), + ( + "remote: error: GH013: Repository rule violations found due to ruleset", + "github rejected the ref", + ), + ( + "fatal: Authentication failed for 'https://github.com/owner/repo'", + "github authentication failed", + ), + ( + "fatal: could not resolve host: github.com", + "network failure", + ), + ("remote: Repository not found.", "github 404"), + ("error: No such remote 'origin'", "origin remote missing"), + ("fatal: not a git repository", "git repository unavailable"), + ]; + + for (stderr, expected) in cases { + let hint = classify_exec_failure(stderr).expect(stderr); + assert!( + hint.contains(expected), + "expected {hint:?} to contain {expected:?}" + ); + } + assert_eq!(classify_exec_failure("weird new git error"), None); + } +} diff --git a/lib/crates/fabro-sandbox/src/lib.rs b/lib/crates/fabro-sandbox/src/lib.rs index a35b9d3ea..efd6e1038 100644 --- a/lib/crates/fabro-sandbox/src/lib.rs +++ b/lib/crates/fabro-sandbox/src/lib.rs @@ -8,6 +8,9 @@ mod clone_source; pub mod read_guard; +#[cfg(any(feature = "docker", feature = "daytona", test))] +pub(crate) mod redact; + pub mod reconnect; pub mod sandbox_provider; diff --git a/lib/crates/fabro-sandbox/src/local.rs b/lib/crates/fabro-sandbox/src/local.rs index ad97bb966..2889faa43 100644 --- a/lib/crates/fabro-sandbox/src/local.rs +++ b/lib/crates/fabro-sandbox/src/local.rs @@ -491,14 +491,17 @@ impl Sandbox for LocalSandbox { result } - async fn git_push_ref(&self, refspec: &str) -> bool { - let has_origin = matches!( - self.exec_command("git remote get-url origin", 10_000, None, None, None) - .await, - Ok(result) if result.exit_code == 0 - ); + async fn git_push_ref(&self, refspec: &str) -> crate::Result<()> { + let has_origin = match self + .exec_command("git remote get-url origin", 10_000, None, None, None) + .await + { + Ok(result) if result.exit_code == 0 => true, + Ok(_) => false, + Err(err) => return Err(crate::Error::context("git remote get-url origin", err)), + }; if !has_origin { - return true; + return Ok(()); } crate::git_push_via_exec(self, refspec).await diff --git a/lib/crates/fabro-sandbox/src/redact.rs b/lib/crates/fabro-sandbox/src/redact.rs new file mode 100644 index 000000000..4e9666641 --- /dev/null +++ b/lib/crates/fabro-sandbox/src/redact.rs @@ -0,0 +1,59 @@ +pub(crate) fn redact_auth_url( + text: &str, + auth_url: Option<&fabro_redact::DisplaySafeUrl>, +) -> String { + let Some(auth_url) = auth_url else { + return text.to_string(); + }; + 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 50c0c7f0b..8cb744b29 100644 --- a/lib/crates/fabro-sandbox/src/sandbox.rs +++ b/lib/crates/fabro-sandbox/src/sandbox.rs @@ -145,7 +145,7 @@ macro_rules! delegate_sandbox { self.$field.resume_setup_commands(run_branch) } - async fn git_push_ref(&self, refspec: &str) -> bool { + async fn git_push_ref(&self, refspec: &str) -> $crate::Result<()> { self.$field.git_push_ref(refspec).await } @@ -380,6 +380,48 @@ pub struct ExecResult { pub duration_ms: u64, } +impl ExecResult { + pub fn is_success(&self) -> bool { + self.exit_code == 0 && !self.timed_out + } + + pub fn into_exec_error(self, label: impl Into) -> crate::Error { + crate::Error::exec( + label, + self.exit_code, + self.timed_out, + self.duration_ms, + self.stderr, + self.stdout, + ) + } + + pub fn into_exec_error_with_redactor( + self, + label: impl Into, + redactor: impl Fn(&str) -> String, + ) -> crate::Error { + let stderr = redactor(&self.stderr); + let stdout = redactor(&self.stdout); + crate::Error::exec( + label, + self.exit_code, + self.timed_out, + self.duration_ms, + stderr, + stdout, + ) + } + + pub fn into_result(self, label: impl Into) -> crate::Result { + if self.is_success() { + Ok(self) + } else { + Err(self.into_exec_error(label)) + } + } +} + #[derive(Debug, Clone)] pub struct DirEntry { pub name: String, @@ -478,8 +520,10 @@ pub trait Sandbox: Send + Sync { } /// Push a full refspec to origin from inside the sandbox. - async fn git_push_ref(&self, _refspec: &str) -> bool { - false + async fn git_push_ref(&self, _refspec: &str) -> crate::Result<()> { + Err(crate::Error::message( + "git_push_ref not implemented for this sandbox", + )) } /// Compute the filesystem path for a parallel branch worktree. @@ -577,13 +621,8 @@ pub async fn setup_git_via_exec( let sha_result = sandbox .exec_command("git rev-parse HEAD", 10_000, None, None, None) .await - .map_err(|e| crate::Error::message(format!("git rev-parse HEAD failed: {e}")))?; - if sha_result.exit_code != 0 { - return Err(crate::Error::message(format!( - "git rev-parse HEAD failed (exit {}): {}", - sha_result.exit_code, sha_result.stderr - ))); - } + .map_err(|e| crate::Error::context("git rev-parse HEAD", e))? + .into_result("git rev-parse HEAD")?; ( sha_result.stdout.trim().to_string(), format!("fabro/run/{run_id}"), @@ -604,16 +643,11 @@ pub async fn setup_git_via_exec( shell_quote(&branch_name), shell_quote(&base_sha) ); - let checkout_result = sandbox + sandbox .exec_command(&checkout_cmd, 10_000, None, None, None) .await - .map_err(|e| crate::Error::message(format!("git checkout failed: {e}")))?; - if checkout_result.exit_code != 0 { - return Err(crate::Error::message(format!( - "git checkout -B failed (exit {}): {}", - checkout_result.exit_code, checkout_result.stderr - ))); - } + .map_err(|e| crate::Error::context("git checkout -B", e))? + .into_result("git checkout -B")?; Ok(GitRunInfo { base_sha, @@ -646,11 +680,9 @@ pub(crate) async fn fetch_source_run_ref( .exec_command(&fetch_cmd, 30_000, None, None, None) .await?; if fetch.exit_code != 0 { - last_error = format!( - "git fetch source run ref failed (exit {}): {}", - fetch.exit_code, - fetch.stderr.trim() - ); + last_error = fetch + .into_exec_error("git fetch source run ref") + .to_string(); } else { let check = sandbox .exec_command(&check_cmd, 10_000, None, None, None) @@ -658,11 +690,11 @@ pub(crate) async fn fetch_source_run_ref( if check.exit_code == 0 { return Ok(()); } - last_error = format!( - "checkpoint {checkpoint_sha} is not reachable from {remote_ref} (exit {}): {}", - check.exit_code, - check.stderr.trim() - ); + last_error = check + .into_exec_error(format!( + "checkpoint {checkpoint_sha} is not reachable from {remote_ref}" + )) + .to_string(); } time::sleep(Duration::from_millis(500)).await; } @@ -672,93 +704,23 @@ pub(crate) async fn fetch_source_run_ref( /// Helper for sandbox implementations that manage git internally. /// Pushes a refspec to origin via exec_command inside the sandbox. -pub async fn git_push_via_exec(sandbox: &dyn Sandbox, refspec: &str) -> bool { +pub async fn git_push_via_exec(sandbox: &dyn Sandbox, refspec: &str) -> crate::Result<()> { if let Err(e) = sandbox.refresh_push_credentials().await { tracing::warn!( - refspec, - error = %fabro_redact::redact_string(&e.to_string()), + refspec = %refspec, + error = %e, "Failed to refresh push credentials before git push" ); } let cmd = format!("{GIT} push origin {}", shell_quote(refspec)); - match sandbox.exec_command(&cmd, 60_000, None, None, None).await { - Ok(r) if r.exit_code == 0 => { - tracing::info!(refspec, "Pushed git ref to origin"); - true - } - Ok(r) => { - tracing::warn!( - refspec, - exit_code = r.exit_code, - timed_out = r.timed_out, - stderr = %trim_for_log(&r.stderr, GIT_LOG_TAIL_BYTES), - stdout = %trim_for_log(&r.stdout, GIT_LOG_TAIL_BYTES), - hint = classify_git_push_failure(&r.stderr).unwrap_or(""), - "Failed to push git ref" - ); - false - } - Err(e) => { - tracing::warn!( - refspec, - error = %fabro_redact::redact_string(&e.to_string()), - "Failed to invoke git push in sandbox" - ); - false - } - } -} - -/// Maximum bytes of git stdout/stderr to include in a single log line. -/// Long enough to capture the typical 1-3 line `fatal:` / `remote:` output -/// without flooding the log when git emits a large progress dump. -const GIT_LOG_TAIL_BYTES: usize = 2048; - -/// Redact secrets from `text`, then keep at most the trailing `limit` bytes. -/// Trailing because git's relevant `fatal:` / `remote: rejected` lines are -/// emitted at the end of the output. -fn trim_for_log(text: &str, limit: usize) -> String { - let redacted = fabro_redact::redact_string(text); - let trimmed = redacted.trim_end(); - if trimmed.len() <= limit { - return trimmed.to_string(); - } - let start = trimmed.len() - limit; - let safe_start = (start..=trimmed.len()) - .find(|i| trimmed.is_char_boundary(*i)) - .unwrap_or(trimmed.len()); - format!("…{}", &trimmed[safe_start..]) -} - -/// Map a git stderr to a short hint pointing at the likely cause. Returns -/// `None` when no known pattern matches; callers should still log the raw -/// (redacted) stderr so unknown failures stay debuggable. -fn classify_git_push_failure(stderr: &str) -> Option<&'static str> { - let lower = stderr.to_ascii_lowercase(); - if lower.contains("could not read username") || lower.contains("terminal prompts disabled") { - Some( - "no credentials in origin URL — check that the sandbox forwarded \ - GITHUB_APP_PRIVATE_KEY (or GITHUB_TOKEN) and that refresh_push_credentials succeeded", - ) - } else if lower.contains("permission to") && lower.contains("denied") { - Some( - "github denied the push — installation token lacks contents:write \ - on this repo, or a branch protection / push ruleset is rejecting the ref", - ) - } else if lower.contains("protected branch") - || lower.contains("ruleset") - || lower.contains("rejected") - { - Some("github rejected the ref — likely a branch protection rule or push ruleset") - } else if lower.contains("authentication failed") || lower.contains("invalid username") { - Some("github authentication failed — installation token may be expired or wrong scope") - } else if lower.contains("could not resolve host") || lower.contains("network is unreachable") { - Some("network failure inside sandbox — check DNS / egress from the run container") - } else if lower.contains("repository not found") { - Some("github 404 — the App installation may not include this repo") - } else { - None - } + let label = format!("git push origin {refspec}"); + sandbox + .exec_command(&cmd, 60_000, None, None, None) + .await + .map_err(|e| crate::Error::context(label.clone(), e))? + .into_result(&label)?; + tracing::info!(refspec = %refspec, "Pushed git ref to origin"); + Ok(()) } #[cfg(test)] @@ -780,57 +742,65 @@ mod tests { } #[test] - fn trim_for_log_keeps_short_output_intact() { - assert_eq!(trim_for_log("fatal: nope\n", 2048), "fatal: nope"); + fn exec_result_helpers_convert_failure_to_exec_error() { + let result = ExecResult { + stdout: "out".into(), + stderr: "fatal: could not read Username".into(), + exit_code: 128, + timed_out: false, + 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)); + assert!(error.to_string().contains("no credentials in origin URL")); } #[test] - fn trim_for_log_keeps_trailing_bytes_when_oversized() { - let prefix = "x".repeat(3000); - let suffix = "fatal: rejected"; - let trimmed = trim_for_log(&format!("{prefix}{suffix}"), 64); - assert!(trimmed.starts_with('…')); - assert!(trimmed.ends_with(suffix)); - assert!(trimmed.chars().count() <= 64 + 1); + fn exec_result_success_honors_timeouts() { + let success = ExecResult { + stdout: String::new(), + stderr: String::new(), + exit_code: 0, + timed_out: false, + duration_ms: 1, + }; + assert!(success.is_success()); + + let timeout = ExecResult { + timed_out: true, + ..success + }; + assert!(!timeout.is_success()); } #[test] - fn trim_for_log_redacts_high_entropy_secrets() { - let stderr = "fatal: unable to access \ - 'https://x-access-token:ghs_xK9mZ2vL8nQ5rT1wY4bC7dF0gH3jE6pA@github.com/owner/repo/'"; - let trimmed = trim_for_log(stderr, 2048); - assert!(!trimmed.contains("ghs_xK9mZ2vL8nQ5rT1wY4bC7dF0gH3jE6pA")); - assert!(trimmed.contains("REDACTED")); + fn exec_result_redactor_applies_to_stderr_and_stdout() { + let result = ExecResult { + stdout: "stdout https://token@example.com".into(), + stderr: "stderr https://token@example.com".into(), + exit_code: 1, + timed_out: false, + duration_ms: 1, + }; + let error = result.into_exec_error_with_redactor("git set-url", |s| { + 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")); } #[test] - fn classify_git_push_failure_recognises_missing_credentials() { - let hint = classify_git_push_failure( - "fatal: could not read Username for 'https://github.com': No such device or address", + fn sandbox_tracing_events_do_not_log_raw_command_fields() { + let root = std::path::Path::new(env!("CARGO_MANIFEST_DIR")).join("src"); + let mut failures = Vec::new(); + scan_for_command_tracing(&root, &mut failures); + assert!( + failures.is_empty(), + "raw command/cmd tracing fields found:\n{}", + failures.join("\n") ); - assert!(hint.unwrap().contains("no credentials in origin URL")); - } - - #[test] - fn classify_git_push_failure_recognises_permission_denied() { - let hint = classify_git_push_failure( - "remote: Permission to owner/repo.git denied to fabro-app[bot].", - ); - assert!(hint.unwrap().contains("github denied the push")); - } - - #[test] - fn classify_git_push_failure_recognises_branch_protection() { - let hint = classify_git_push_failure( - "remote: error: GH013: Repository rule violations found for refs/heads/main\n\ - remote: - Cannot create ref 'refs/heads/fabro/run/X' due to ruleset", - ); - assert!(hint.unwrap().contains("ruleset")); - } - - #[test] - fn classify_git_push_failure_returns_none_for_unknown() { - assert!(classify_git_push_failure("fatal: weird new git error message").is_none()); } #[test] @@ -960,4 +930,77 @@ mod tests { assert_eq!(shell_quote("hello"), "hello"); assert_eq!(shell_quote("hello world"), "'hello world'"); } + + #[expect( + clippy::disallowed_methods, + reason = "unit test performs a small synchronous source scan of local Rust files" + )] + fn scan_for_command_tracing(path: &std::path::Path, failures: &mut Vec) { + for entry in std::fs::read_dir(path).unwrap() { + let entry = entry.unwrap(); + let path = entry.path(); + if path.is_dir() { + scan_for_command_tracing(&path, failures); + continue; + } + if path.extension().and_then(|ext| ext.to_str()) != Some("rs") { + continue; + } + let source = std::fs::read_to_string(&path).unwrap(); + for macro_name in [ + "tracing::trace!", + "tracing::debug!", + "tracing::info!", + "tracing::warn!", + "tracing::error!", + "trace!", + "debug!", + "info!", + "warn!", + "error!", + ] { + let mut rest = source.as_str(); + while let Some(idx) = rest.find(macro_name) { + let start = source.len() - rest.len() + idx; + if start > 0 && source.as_bytes()[start - 1] == b'"' { + rest = &source[start + macro_name.len()..]; + continue; + } + let Some(call) = tracing_call(&source[start..]) else { + break; + }; + if call.contains("command,") + || call.contains("command =") + || call.contains("cmd,") + || call.contains("cmd =") + { + failures.push(format!( + "{}: {}", + path.display(), + call.lines().next().unwrap_or(call) + )); + } + rest = &source[start + call.len()..]; + } + } + } + } + + fn tracing_call(source: &str) -> Option<&str> { + let open = source.find('(')?; + let mut depth = 0usize; + for (idx, ch) in source.char_indices().skip(open) { + match ch { + '(' => depth += 1, + ')' => { + depth = depth.saturating_sub(1); + if depth == 0 { + return Some(&source[..=idx]); + } + } + _ => {} + } + } + None + } } diff --git a/lib/crates/fabro-sandbox/src/worktree.rs b/lib/crates/fabro-sandbox/src/worktree.rs index 36d350fe4..aef1b76ee 100644 --- a/lib/crates/fabro-sandbox/src/worktree.rs +++ b/lib/crates/fabro-sandbox/src/worktree.rs @@ -344,14 +344,17 @@ impl Sandbox for WorktreeSandbox { self.inner.resume_setup_commands(run_branch) } - async fn git_push_ref(&self, refspec: &str) -> bool { - let has_origin = matches!( - self.exec_command("git remote get-url origin", 10_000, None, None, None) - .await, - Ok(result) if result.exit_code == 0 - ); + async fn git_push_ref(&self, refspec: &str) -> crate::Result<()> { + let has_origin = match self + .exec_command("git remote get-url origin", 10_000, None, None, None) + .await + { + Ok(result) if result.exit_code == 0 => true, + Ok(_) => false, + Err(err) => return Err(crate::Error::context("git remote get-url origin", err)), + }; if !has_origin { - return true; + return Ok(()); } crate::git_push_via_exec(self, refspec).await diff --git a/lib/crates/fabro-workflow/src/lifecycle/git.rs b/lib/crates/fabro-workflow/src/lifecycle/git.rs index 2ad579d95..e9b229780 100644 --- a/lib/crates/fabro-workflow/src/lifecycle/git.rs +++ b/lib/crates/fabro-workflow/src/lifecycle/git.rs @@ -175,7 +175,17 @@ impl RunLifecycle for GitLifecycle { .and_then(|g| g.run_branch.as_ref()) { let refspec = format!("refs/heads/{branch}:refs/heads/{branch}"); - let push_ok = self.sandbox.git_push_ref(&refspec).await; + 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)); } } @@ -249,9 +259,10 @@ 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"); self.emit_metadata_warning( "checkpoint_metadata_push_failed", - format!("failed to push metadata ref refs/heads/{meta_branch}"), + format!("failed to push metadata ref refs/heads/{meta_branch}: {detail}"), ); } Some(snapshot.commit_sha) diff --git a/lib/crates/fabro-workflow/src/pipeline/finalize.rs b/lib/crates/fabro-workflow/src/pipeline/finalize.rs index 4c62301cd..9a9620e3d 100644 --- a/lib/crates/fabro-workflow/src/pipeline/finalize.rs +++ b/lib/crates/fabro-workflow/src/pipeline/finalize.rs @@ -179,10 +179,11 @@ 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"); emit_metadata_warning( services, "checkpoint_metadata_push_failed", - format!("failed to push metadata ref refs/heads/{meta_branch}"), + format!("failed to push metadata ref refs/heads/{meta_branch}: {detail}"), ); } } diff --git a/lib/crates/fabro-workflow/src/sandbox_git.rs b/lib/crates/fabro-workflow/src/sandbox_git.rs index 69306e584..4b3f1fd79 100644 --- a/lib/crates/fabro-workflow/src/sandbox_git.rs +++ b/lib/crates/fabro-workflow/src/sandbox_git.rs @@ -942,7 +942,7 @@ mod tests { self.inner.os_version() } - async fn git_push_ref(&self, refspec: &str) -> bool { + async fn git_push_ref(&self, refspec: &str) -> fabro_sandbox::Result<()> { self.pushes.lock().unwrap().push(refspec.to_string()); self.inner.git_push_ref(refspec).await } @@ -1351,6 +1351,8 @@ 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; let current = std::process::Command::new("git") @@ -1447,6 +1449,65 @@ mod tests { assert_eq!(sandbox.pushes(), vec![refspec.clone(), refspec]); } + #[tokio::test] + async fn sandbox_metadata_writer_records_log_safe_push_error() { + let repo_dir = tempfile::tempdir().unwrap(); + let repo = repo_dir.path(); + init_git_repo(repo); + std::fs::write(repo.join("tracked.txt"), "seed\n").unwrap(); + git_commit_all(repo, "initial"); + let missing_origin = repo_dir.path().join("missing-origin.git"); + let add_remote = std::process::Command::new("git") + .args(["remote", "add", "origin", missing_origin.to_str().unwrap()]) + .current_dir(repo) + .output() + .unwrap(); + assert!(add_remote.status.success()); + + let sandbox = fabro_agent::LocalSandbox::new(repo.to_path_buf()); + let run_id = fabro_types::fixtures::RUN_2.to_string(); + let branch = crate::sandbox_metadata::metadata_branch_name(&run_id); + let mut projection = fabro_store::RunProjection::default(); + projection.spec = Some(fabro_types::RunSpec { + run_id: fabro_types::fixtures::RUN_2, + settings: fabro_types::WorkflowSettings::default(), + graph: fabro_types::Graph::new("metadata"), + workflow_slug: Some("metadata".to_string()), + source_directory: Some("/Users/client/project".to_string()), + git: None, + labels: HashMap::new(), + provenance: None, + manifest_blob: None, + definition_blob: None, + fork_source_ref: None, + in_place: false, + }); + let dump = crate::run_dump::RunDump::from_projection(&projection); + let runtime = crate::sandbox_metadata::SandboxGitRuntime::new(); + let writer = crate::sandbox_metadata::SandboxMetadataWriter::new( + &sandbox, + &runtime, + &run_id, + &branch, + crate::git::GitAuthor::default(), + ); + + 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:")); + assert!( + !push_error.contains("fatal:"), + "push error should be log-safe: {push_error}" + ); + assert!( + !push_error.contains(missing_origin.to_str().unwrap()), + "push error should not include raw git stderr paths: {push_error}" + ); + } + #[tokio::test] async fn list_changed_files_raw_classifies_add_modify_delete() { let repo_dir = tempfile::tempdir().unwrap(); diff --git a/lib/crates/fabro-workflow/src/sandbox_metadata.rs b/lib/crates/fabro-workflow/src/sandbox_metadata.rs index db5433f51..cbf91b4b0 100644 --- a/lib/crates/fabro-workflow/src/sandbox_metadata.rs +++ b/lib/crates/fabro-workflow/src/sandbox_metadata.rs @@ -78,6 +78,7 @@ pub(crate) struct SandboxMetadataWriter<'a> { pub(crate) struct MetadataSnapshot { pub commit_sha: String, pub pushed: bool, + pub push_error: Option, } impl<'a> SandboxMetadataWriter<'a> { @@ -178,10 +179,13 @@ impl<'a> SandboxMetadataWriter<'a> { .await?; let commit = parse_fast_import_mark(&stdout)?; let refspec = format!("{full_ref}:{full_ref}"); - let pushed = self.sandbox.git_push_ref(&refspec).await; + 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()); Ok(MetadataSnapshot { commit_sha: commit, pushed, + push_error, }) } }