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) <noreply@anthropic.com>
This commit is contained in:
Bryan Helmkamp 2026-04-29 12:52:30 -04:00
parent 067fa3ee82
commit 32166e4cdf
No known key found for this signature in database
9 changed files with 33 additions and 151 deletions

View file

@ -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(

View file

@ -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(())

View file

@ -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<i32> {
match self {
Self::Exec { exit_code, .. } => Some(*exit_code),
_ => None,
}
}
pub fn exec_timed_out(&self) -> Option<bool> {
match self {
Self::Exec { timed_out, .. } => Some(*timed_out),
_ => None,
}
}
pub fn exec_duration_ms(&self) -> Option<u64> {
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 = [

View file

@ -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"
);
}
}

View file

@ -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]

View file

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

View file

@ -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",

View file

@ -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:"));

View file

@ -77,7 +77,6 @@ pub(crate) struct SandboxMetadataWriter<'a> {
pub(crate) struct MetadataSnapshot {
pub commit_sha: String,
pub pushed: bool,
pub push_error: Option<String>,
}
@ -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,
})
}