From 66b2eb74303d1ea5484f63a50a80933f6cc0fb48 Mon Sep 17 00:00:00 2001 From: DanConwayDev Date: Sat, 15 Aug 2026 07:11:52 +0000 Subject: [PATCH] test: survive hostile git config and non-reaping PID 1 in CI CI runs the suite under a deliberately hostile global git configuration and inside an act container whose PID 1 does not reap orphans. Seven lib tests relied on a friendly environment and failed there: Five tests built git fixtures with bare Command::new("git"), so the hostile failing pre-commit hook (core.hooksPath) broke fixture commits; most fixture steps also ignored exit status, so the tests failed later at an unrelated assertion. Fixtures now go through grasp_audit::git_command(), which masks user and system git config, and every step asserts success with stderr in the failure message. The now-redundant local gpgsign toggles are dropped: hermetic fixtures see no signing config at all. Two process-group tests probed descendant death with kill(pid, 0), which still succeeds for an unreaped zombie. The group kill orphans the descendant, and without a reaping ancestor it stays a zombie forever, so the probes timed out. The probe now reads /proc//stat and also treats state Z as terminated; the group-kill behaviour under test is unchanged. Both failure modes were reproduced locally with the workflow's hostile git config (via GIT_CONFIG_GLOBAL) and a non-reaping PR_SET_CHILD_SUBREAPER wrapper, matching the CI panics exactly; with these changes the seven tests pass under both conditions, and cargo fmt, clippy -D warnings, the full workspace suite, and the grasp-audit suite pass. Four tests/sync.rs timing flakes seen under full parallel local load pass in isolation and predate this change; they are out of scope. --- src/git/mod.rs | 90 +++++++++++++------------------- src/purgatory/helpers.rs | 96 +++++++++++------------------------ src/purgatory/sync/context.rs | 51 +++++++++++++------ 3 files changed, 99 insertions(+), 138 deletions(-) diff --git a/src/git/mod.rs b/src/git/mod.rs index f912865..d738e45 100644 --- a/src/git/mod.rs +++ b/src/git/mod.rs @@ -573,16 +573,30 @@ mod tests { use std::fs; use tempfile::TempDir; + /// Run one fixture git step hermetically (immune to ambient git + /// configuration such as hooks, signing, and templates) and fail loudly + /// on error instead of letting a later assertion fail obscurely. + fn run_git(dir: Option<&Path>, args: &[&str]) -> String { + let mut command = grasp_audit::git_command(); + if let Some(dir) = dir { + command.current_dir(dir); + } + let output = command.args(args).output().unwrap(); + assert!( + output.status.success(), + "git {args:?} failed: {}", + String::from_utf8_lossy(&output.stderr) + ); + String::from_utf8_lossy(&output.stdout).trim().to_string() + } + /// Create a test bare repository with optional commits fn create_test_repo() -> (TempDir, PathBuf) { let temp_dir = TempDir::new().unwrap(); let repo_path = temp_dir.path().join("test.git"); // Initialize bare repository - Command::new("git") - .args(["init", "--bare", repo_path.to_str().unwrap()]) - .output() - .unwrap(); + run_git(None, &["init", "--bare", repo_path.to_str().unwrap()]); (temp_dir, repo_path) } @@ -594,76 +608,40 @@ mod tests { let bare_repo = temp_dir.path().join("test.git"); // Initialize bare repository - Command::new("git") - .args([ + run_git( + None, + &[ "init", "--bare", "--initial-branch=main", bare_repo.to_str().unwrap(), - ]) - .output() - .unwrap(); + ], + ); // Clone to working directory - Command::new("git") - .args([ + run_git( + None, + &[ "clone", bare_repo.to_str().unwrap(), work_dir.to_str().unwrap(), - ]) - .output() - .unwrap(); + ], + ); // Configure git for commits - Command::new("git") - .args(["config", "user.email", "test@test.com"]) - .current_dir(&work_dir) - .output() - .unwrap(); - Command::new("git") - .args(["config", "user.name", "Test"]) - .current_dir(&work_dir) - .output() - .unwrap(); - // Disable GPG signing for tests (prevents yubikey prompts) - Command::new("git") - .args(["config", "commit.gpgsign", "false"]) - .current_dir(&work_dir) - .output() - .unwrap(); - Command::new("git") - .args(["config", "tag.gpgsign", "false"]) - .current_dir(&work_dir) - .output() - .unwrap(); + run_git(Some(&work_dir), &["config", "user.email", "test@test.com"]); + run_git(Some(&work_dir), &["config", "user.name", "Test"]); // Create a file and commit fs::write(work_dir.join("README.md"), "# Test").unwrap(); - Command::new("git") - .args(["add", "README.md"]) - .current_dir(&work_dir) - .output() - .unwrap(); - Command::new("git") - .args(["commit", "-m", "Initial commit"]) - .current_dir(&work_dir) - .output() - .unwrap(); + run_git(Some(&work_dir), &["add", "README.md"]); + run_git(Some(&work_dir), &["commit", "-m", "Initial commit"]); // Get commit hash - let output = Command::new("git") - .args(["rev-parse", "HEAD"]) - .current_dir(&work_dir) - .output() - .unwrap(); - let commit_hash = String::from_utf8_lossy(&output.stdout).trim().to_string(); + let commit_hash = run_git(Some(&work_dir), &["rev-parse", "HEAD"]); // Push to bare repo - Command::new("git") - .args(["push", "origin", "main"]) - .current_dir(&work_dir) - .output() - .unwrap(); + run_git(Some(&work_dir), &["push", "origin", "main"]); (temp_dir, bare_repo, commit_hash) } diff --git a/src/purgatory/helpers.rs b/src/purgatory/helpers.rs index 1bf0d80..7d91628 100644 --- a/src/purgatory/helpers.rs +++ b/src/purgatory/helpers.rs @@ -611,99 +611,61 @@ mod tests { // can_apply_state tests // ========================================================================= + /// Run one fixture git step hermetically (immune to ambient git + /// configuration such as hooks, signing, and templates) and fail loudly + /// on error instead of letting a later assertion fail obscurely. + fn run_git(dir: &std::path::Path, args: &[&str]) -> String { + let output = grasp_audit::git_command() + .current_dir(dir) + .args(args) + .output() + .unwrap(); + assert!( + output.status.success(), + "git {args:?} failed: {}", + String::from_utf8_lossy(&output.stderr) + ); + String::from_utf8_lossy(&output.stdout).trim().to_string() + } + /// Helper to create a temporary bare git repository with a commit. /// Returns (temp_dir, commit_hash) where commit_hash is Some if a commit was created. fn create_test_repo_with_commit() -> (tempfile::TempDir, Option) { - use std::process::Command; - let temp_dir = tempfile::tempdir().unwrap(); let bare_path = temp_dir.path(); // Initialize bare repo - Command::new("git") - .args(["init", "--bare"]) - .current_dir(bare_path) - .output() - .expect("Failed to init bare git repo"); + run_git(bare_path, &["init", "--bare"]); // Create a working repo to generate a commit let work_dir = tempfile::tempdir().unwrap(); - Command::new("git") - .args(["init"]) - .current_dir(work_dir.path()) - .output() - .expect("Failed to init work repo"); - - Command::new("git") - .args(["config", "user.email", "test@test.com"]) - .current_dir(work_dir.path()) - .output() - .expect("Failed to set email"); - - Command::new("git") - .args(["config", "user.name", "Test"]) - .current_dir(work_dir.path()) - .output() - .expect("Failed to set name"); - - // Disable GPG signing for tests (prevents yubikey prompts) - Command::new("git") - .args(["config", "commit.gpgsign", "false"]) - .current_dir(work_dir.path()) - .output() - .expect("Failed to disable commit.gpgsign"); - - Command::new("git") - .args(["config", "tag.gpgsign", "false"]) - .current_dir(work_dir.path()) - .output() - .expect("Failed to disable tag.gpgsign"); + run_git(work_dir.path(), &["init"]); + run_git(work_dir.path(), &["config", "user.email", "test@test.com"]); + run_git(work_dir.path(), &["config", "user.name", "Test"]); // Create a commit std::fs::write(work_dir.path().join("file.txt"), "content").unwrap(); - Command::new("git") - .args(["add", "."]) - .current_dir(work_dir.path()) - .output() - .expect("Failed to add"); - - Command::new("git") - .args(["commit", "-m", "test"]) - .current_dir(work_dir.path()) - .output() - .expect("Failed to commit"); + run_git(work_dir.path(), &["add", "."]); + run_git(work_dir.path(), &["commit", "-m", "test"]); // Get the commit hash from the working repo - let output = Command::new("git") - .args(["rev-parse", "HEAD"]) - .current_dir(work_dir.path()) - .output() - .expect("Failed to get commit hash"); - - let commit_hash = String::from_utf8_lossy(&output.stdout).trim().to_string(); + let commit_hash = run_git(work_dir.path(), &["rev-parse", "HEAD"]); // Push to bare repo - Command::new("git") - .args(["push", bare_path.to_str().unwrap(), "HEAD:refs/heads/main"]) - .current_dir(work_dir.path()) - .output() - .expect("Failed to push"); + run_git( + work_dir.path(), + &["push", bare_path.to_str().unwrap(), "HEAD:refs/heads/main"], + ); (temp_dir, Some(commit_hash)) } /// Helper to create an empty bare git repository (no commits). fn create_empty_test_repo() -> tempfile::TempDir { - use std::process::Command; - let temp_dir = tempfile::tempdir().unwrap(); - Command::new("git") - .args(["init", "--bare"]) - .current_dir(temp_dir.path()) - .output() - .expect("Failed to init bare git repo"); + run_git(temp_dir.path(), &["init", "--bare"]); temp_dir } diff --git a/src/purgatory/sync/context.rs b/src/purgatory/sync/context.rs index 065e4bb..f83e423 100644 --- a/src/purgatory/sync/context.rs +++ b/src/purgatory/sync/context.rs @@ -1232,39 +1232,62 @@ impl SyncContext for RealSyncContext { mod fetch_helper_tests { use super::*; + /// Hermetic git for these tests, immune to ambient git configuration + /// such as hooks, signing, and templates. + fn fixture_git() -> std::process::Command { + grasp_audit::git_command() + } + + /// True once the descendant with this pid no longer runs. A process-group + /// kill leaves the orphaned descendant as an unreaped zombie whenever no + /// reaping ancestor is present (CI containers lack a reaping PID 1), and + /// `kill(pid, 0)` still succeeds for zombies, so an unreaped zombie must + /// also count as terminated. + fn descendant_terminated(pid: i32) -> bool { + match std::fs::read_to_string(format!("/proc/{pid}/stat")) { + Err(_) => true, + // The state field follows the parenthesised, possibly + // space-containing command name. + Ok(stat) => match stat.rsplit_once(") ") { + Some((_, rest)) => rest.starts_with('Z'), + None => false, + }, + } + } + fn create_source_repo(root: &Path, name: &str, contents: &str) -> (PathBuf, String) { let path = root.join(name); - assert!(std::process::Command::new("git") + assert!(fixture_git() .args(["init", "--quiet", path.to_str().unwrap()]) .status() .unwrap() .success()); - assert!(std::process::Command::new("git") + assert!(fixture_git() .current_dir(&path) .args(["config", "user.name", "Test"]) .status() .unwrap() .success()); - assert!(std::process::Command::new("git") + assert!(fixture_git() .current_dir(&path) .args(["config", "user.email", "test@example.com"]) .status() .unwrap() .success()); std::fs::write(path.join("payload"), contents).unwrap(); - assert!(std::process::Command::new("git") + assert!(fixture_git() .current_dir(&path) .args(["add", "payload"]) .status() .unwrap() .success()); - assert!(std::process::Command::new("git") + assert!(fixture_git() .current_dir(&path) .args(["commit", "--quiet", "-m", "fixture"]) .status() .unwrap() .success()); - let oid = std::process::Command::new("git") + let oid = fixture_git() .current_dir(&path) .args(["rev-parse", "HEAD"]) .output() @@ -1389,9 +1412,7 @@ mod fetch_helper_tests { let pid: i32 = pid.parse().unwrap(); let deadline = tokio::time::Instant::now() + Duration::from_secs(1); loop { - // Signal zero observes existence without changing process state. - let alive = unsafe { libc::kill(pid, 0) } == 0; - if !alive { + if descendant_terminated(pid) { break; } assert!( @@ -1444,7 +1465,7 @@ mod fetch_helper_tests { tokio::time::timeout(Duration::from_secs(1), async { loop { - if unsafe { libc::kill(pid, 0) } != 0 { + if descendant_terminated(pid) { break; } tokio::task::yield_now().await; @@ -1477,14 +1498,14 @@ mod fetch_helper_tests { let (first_source, first_oid) = create_source_repo(temp.path(), "first", "first"); let (second_source, second_oid) = create_source_repo(temp.path(), "second", "second"); let target = temp.path().join("target.git"); - assert!(std::process::Command::new("git") + assert!(fixture_git() .args(["init", "--bare", "--quiet", target.to_str().unwrap()]) .status() .unwrap() .success()); let fetch = |source: PathBuf, oid: String, role| { - let mut command = Command::new("git"); + let mut command = Command::from(fixture_git()); command .current_dir(&target) .args(["fetch", "--no-write-fetch-head", "--no-auto-maintenance"]) @@ -1502,7 +1523,7 @@ mod fetch_helper_tests { assert!(first.unwrap().status.success()); assert!(second.unwrap().status.success()); assert!(!target.join("FETCH_HEAD").exists()); - let refs = std::process::Command::new("git") + let refs = fixture_git() .current_dir(&target) .args(["for-each-ref", "--format=%(refname)"]) .output() @@ -1511,14 +1532,14 @@ mod fetch_helper_tests { assert!(refs.stdout.is_empty(), "object fetch must not update refs"); for oid in [first_oid, second_oid] { - assert!(std::process::Command::new("git") + assert!(fixture_git() .current_dir(&target) .args(["cat-file", "-e", &format!("{oid}^{{commit}}")]) .status() .unwrap() .success()); } - let fsck = std::process::Command::new("git") + let fsck = fixture_git() .current_dir(&target) .args(["fsck", "--no-dangling"]) .output()