From ad186968947e1d56dfdea71e1d2b69fe3b6fc8c2 Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Fri, 24 Apr 2026 13:22:17 -0400 Subject: [PATCH] fix(workflow): prevent signed sandbox checkpoint commits Disable git signing for sandbox bookkeeping commits and surface timeout or killed git operations with actionable error text. --- lib/crates/fabro-workflow/src/sandbox_git.rs | 266 +++++++++++++++++-- 1 file changed, 248 insertions(+), 18 deletions(-) diff --git a/lib/crates/fabro-workflow/src/sandbox_git.rs b/lib/crates/fabro-workflow/src/sandbox_git.rs index 3a952a7bb..55a03e9a9 100644 --- a/lib/crates/fabro-workflow/src/sandbox_git.rs +++ b/lib/crates/fabro-workflow/src/sandbox_git.rs @@ -20,7 +20,22 @@ pub struct GitState { pub git_author: GitAuthor, } -pub const GIT_REMOTE: &str = "git -c maintenance.auto=0 -c gc.auto=0"; +pub const GIT_REMOTE: &str = + "git -c maintenance.auto=0 -c gc.auto=0 -c commit.gpgsign=false -c tag.gpgsign=false"; + +fn exec_err(label: &str, r: &fabro_sandbox::ExecResult) -> String { + if r.timed_out { + return format!("{label} timed out after {}ms", r.duration_ms); + } + + let detail = format!("{}{}", r.stdout, r.stderr); + let detail = detail.trim(); + if detail.is_empty() { + format!("{label} killed (exit {}, no output)", r.exit_code) + } else { + format!("{label} failed (exit {}): {detail}", r.exit_code) + } +} /// Shell-escape a string using `shlex::try_quote` (POSIX-safe). fn shell_quote(s: &str) -> String { @@ -61,12 +76,7 @@ pub async fn git_checkpoint( .await; match &add_result { Ok(r) if r.exit_code == 0 => {} - Ok(r) => { - return Err(format!( - "git add failed (exit {}): {}{}", - r.exit_code, r.stdout, r.stderr - )); - } + Ok(r) => return Err(exec_err("git add", r)), Err(e) => return Err(format!("git add failed: {e}")), } @@ -107,12 +117,7 @@ pub async fn git_checkpoint( .await; match &commit_result { Ok(r) if r.exit_code == 0 => {} - Ok(r) => { - return Err(format!( - "git commit failed (exit {}): {}{}", - r.exit_code, r.stdout, r.stderr - )); - } + Ok(r) => return Err(exec_err("git commit", r)), Err(e) => return Err(format!("git commit failed: {e}")), } @@ -122,10 +127,7 @@ pub async fn git_checkpoint( .await; match sha_result { Ok(r) if r.exit_code == 0 => Ok(r.stdout.trim().to_string()), - Ok(r) => Err(format!( - "git rev-parse HEAD failed (exit {}): {}{}", - r.exit_code, r.stdout, r.stderr - )), + Ok(r) => Err(exec_err("git rev-parse HEAD", &r)), Err(e) => Err(format!("git rev-parse HEAD failed: {e}")), } } @@ -214,7 +216,7 @@ pub(crate) async fn git_diff_with_timeout( .await { Ok(r) if r.exit_code == 0 => Ok(r.stdout), - Ok(r) => Err(format!("exit {}: {}", r.exit_code, r.stderr.trim())), + Ok(r) => Err(exec_err("git diff", &r)), Err(e) => Err(e.clone()), } } @@ -789,8 +791,236 @@ mod tests { reason = "These unit tests use the real git CLI to construct sandbox-git fixture repositories and sync-write fixtures to disk." )] + use std::collections::VecDeque; + use std::sync::Mutex; + + use async_trait::async_trait; + use fabro_agent::{DirEntry, ExecResult, GrepOptions}; + use tokio_util::sync::CancellationToken; + use super::*; + struct ScriptedSandbox { + exec_results: Mutex>, + } + + impl ScriptedSandbox { + fn new(exec_results: Vec) -> Self { + Self { + exec_results: Mutex::new(exec_results.into()), + } + } + } + + #[async_trait] + impl Sandbox for ScriptedSandbox { + async fn read_file( + &self, + _path: &str, + _offset: Option, + _limit: Option, + ) -> Result { + Err("read_file not implemented for ScriptedSandbox".to_string()) + } + + async fn write_file(&self, _path: &str, _content: &str) -> Result<(), String> { + Ok(()) + } + + async fn delete_file(&self, _path: &str) -> Result<(), String> { + Ok(()) + } + + async fn file_exists(&self, _path: &str) -> Result { + Ok(false) + } + + async fn list_directory( + &self, + _path: &str, + _depth: Option, + ) -> Result, String> { + Ok(Vec::new()) + } + + async fn exec_command( + &self, + _command: &str, + _timeout_ms: u64, + _working_dir: Option<&str>, + _env_vars: Option<&std::collections::HashMap>, + _cancel_token: Option, + ) -> Result { + self.exec_results + .lock() + .expect("exec_results lock poisoned") + .pop_front() + .ok_or_else(|| "unexpected exec_command call".to_string()) + } + + async fn grep( + &self, + _pattern: &str, + _path: &str, + _options: &GrepOptions, + ) -> Result, String> { + Ok(Vec::new()) + } + + async fn glob(&self, _pattern: &str, _path: Option<&str>) -> Result, String> { + Ok(Vec::new()) + } + + async fn download_file_to_local( + &self, + _remote_path: &str, + _local_path: &std::path::Path, + ) -> Result<(), String> { + Ok(()) + } + + async fn upload_file_from_local( + &self, + _local_path: &std::path::Path, + _remote_path: &str, + ) -> Result<(), String> { + Ok(()) + } + + async fn initialize(&self) -> Result<(), String> { + Ok(()) + } + + async fn cleanup(&self) -> Result<(), String> { + Ok(()) + } + + fn working_directory(&self) -> &str { + "/work" + } + + fn platform(&self) -> &str { + "darwin" + } + + fn os_version(&self) -> String { + "Darwin".to_string() + } + } + + fn exec_ok() -> ExecResult { + ExecResult { + stdout: String::new(), + stderr: String::new(), + exit_code: 0, + timed_out: false, + duration_ms: 1, + } + } + + fn exec_timed_out(duration_ms: u64) -> ExecResult { + ExecResult { + stdout: String::new(), + stderr: String::new(), + exit_code: -1, + timed_out: true, + duration_ms, + } + } + + fn exec_failed(exit_code: i32, stdout: &str, stderr: &str) -> ExecResult { + ExecResult { + stdout: stdout.to_string(), + stderr: stderr.to_string(), + exit_code, + timed_out: false, + duration_ms: 1, + } + } + + #[test] + fn git_remote_disables_commit_and_tag_signing() { + assert!(GIT_REMOTE.contains("-c commit.gpgsign=false")); + assert!(GIT_REMOTE.contains("-c tag.gpgsign=false")); + } + + #[tokio::test] + async fn git_checkpoint_reports_add_timeout() { + let sandbox = ScriptedSandbox::new(vec![exec_timed_out(77)]); + let err = git_checkpoint( + &sandbox, + "run1", + "work", + "success", + 1, + None, + &[], + &crate::git::GitAuthor::default(), + ) + .await + .unwrap_err(); + + assert_eq!(err, "git add timed out after 77ms"); + } + + #[tokio::test] + async fn git_checkpoint_reports_commit_timeout() { + let sandbox = ScriptedSandbox::new(vec![exec_ok(), exec_timed_out(88)]); + let err = git_checkpoint( + &sandbox, + "run1", + "work", + "success", + 1, + None, + &[], + &crate::git::GitAuthor::default(), + ) + .await + .unwrap_err(); + + assert_eq!(err, "git commit timed out after 88ms"); + } + + #[tokio::test] + async fn git_checkpoint_reports_rev_parse_killed_without_output() { + let sandbox = ScriptedSandbox::new(vec![exec_ok(), exec_ok(), exec_failed(-1, "", "")]); + let err = git_checkpoint( + &sandbox, + "run1", + "work", + "success", + 1, + None, + &[], + &crate::git::GitAuthor::default(), + ) + .await + .unwrap_err(); + + assert_eq!(err, "git rev-parse HEAD killed (exit -1, no output)"); + } + + #[tokio::test] + async fn git_diff_reports_timeout() { + let sandbox = ScriptedSandbox::new(vec![exec_timed_out(99)]); + let err = git_diff_with_timeout(&sandbox, "HEAD~1", 99) + .await + .unwrap_err(); + + assert_eq!(err, "git diff timed out after 99ms"); + } + + #[tokio::test] + async fn git_diff_reports_failure_detail() { + let sandbox = ScriptedSandbox::new(vec![exec_failed(128, "", "fatal: bad revision\n")]); + let err = git_diff_with_timeout(&sandbox, "bad-base", 100) + .await + .unwrap_err(); + + assert_eq!(err, "git diff failed (exit 128): fatal: bad revision"); + } + #[tokio::test] async fn git_checkpoint_includes_builtin_excludes() { // Set up a real git repo