From 8722ab6641a12809bb34d29aba58e37f1d4d2b7a Mon Sep 17 00:00:00 2001 From: DanConwayDev Date: Sat, 1 Aug 2026 16:20:29 +0000 Subject: [PATCH] test: stop discarding load-bearing git exit statuses in fixtures Several fixture steps discarded git exit statuses even though later pushes and deterministic-hash assertions depend on them. The nip09 helpers were the worst case: `git branch main` + `git checkout main` with both results ignored only worked when the clone's initial branch was NOT already main, so the helpers passed under init.defaultBranch=main by accident (git exits 128 when the branch exists). Replace the dance with a checked `git checkout -B main` that panics with git's stderr when branch setup genuinely fails. Also checked now, failing loudly with context: - fixtures.rs: orphan-branch dances (checkout --orphan, rm --cached, branch -m/-M) behind a new run_fixture_git helper, the PurgatoryOwnerStateDataPushed checkout -B, and clone_repo's identity config, which the pinned hashes are derived from - push_authorization.rs: checkout -b develop1 exit code (previously only the spawn error was inspected) and ls-remote failures, which were indistinguishable from "ref absent" - purgatory_helpers.rs: remote set-url/add in push_ref_to_relay now go through run_git instead of `let _` - sync_helpers.rs: rev-parse HEAD is asserted before its output flows into a state event Deliberate best-effort calls (git clean -fd, deleting a possibly absent local main before renaming an orphan branch onto it) keep discarding their status, now with comments saying why. Validated: cargo clippy --workspace --all-targets -D warnings; full suite runs under both benign and hostile git config in the final validation pass. --- grasp-audit/src/fixtures.rs | 137 +++++++++++------- .../src/specs/grasp01/push_authorization.rs | 27 +++- tests/common/nip09_helpers.rs | 84 +++++++---- tests/common/purgatory_helpers.rs | 10 +- tests/common/sync_helpers.rs | 5 + 5 files changed, 168 insertions(+), 95 deletions(-) diff --git a/grasp-audit/src/fixtures.rs b/grasp-audit/src/fixtures.rs index 6847d56..b275788 100644 --- a/grasp-audit/src/fixtures.rs +++ b/grasp-audit/src/fixtures.rs @@ -1313,11 +1313,11 @@ impl<'a> TestContext<'a> { } // -B creates or resets main at HEAD, tolerating a pre-existing local - // main (e.g. when the host sets init.defaultBranch=main) - let _ = git_command() - .args(["checkout", "-B", "main"]) - .current_dir(&clone_path) - .output(); + // main (the clone's initial branch follows the server's advertised HEAD) + if let Err(e) = run_fixture_git(&clone_path, &["checkout", "-B", "main"]) { + cleanup(&clone_path); + return Err(anyhow::anyhow!("Failed to create/checkout main: {}", e)); + } let push_result = try_push(&clone_path); cleanup(&clone_path); @@ -1414,8 +1414,8 @@ impl<'a> TestContext<'a> { } // Create or reset main at our deterministic commit and check it out. - // -B tolerates a pre-existing local main (e.g. when the host sets - // init.defaultBranch=main) + // -B tolerates a pre-existing local main (the clone's initial branch + // follows the server's advertised HEAD) let checkout_output = git_command() .args(["checkout", "-B", "main"]) .current_dir(&clone_path) @@ -1553,16 +1553,16 @@ impl<'a> TestContext<'a> { // Reset to orphan state and create deterministic root commit // Step 1: Create orphan branch (removes all history) - let _ = git_command() - .args(["checkout", "--orphan", "main-new"]) - .current_dir(&clone_path) - .output(); + if let Err(e) = run_fixture_git(&clone_path, &["checkout", "--orphan", "main-new"]) { + cleanup(&clone_path); + return Err(anyhow::anyhow!("Failed to create orphan branch: {}", e)); + } // Step 2: Clear staged files (orphan keeps files staged from previous branch) - let _ = git_command() - .args(["rm", "-rf", "--cached", "."]) - .current_dir(&clone_path) - .output(); + if let Err(e) = run_fixture_git(&clone_path, &["rm", "-rf", "--cached", "."]) { + cleanup(&clone_path); + return Err(anyhow::anyhow!("Failed to clear staged files: {}", e)); + } // Step 3: Create deterministic commit using maintainer variant let commit_hash = match create_deterministic_commit_with_variant( @@ -1576,16 +1576,21 @@ impl<'a> TestContext<'a> { } }; - // Step 4: Replace main branch with our new orphan branch + // Step 4: Replace main branch with our new orphan branch. Deleting the + // old main is best-effort (the clone may not have had one); the rename + // must succeed for the push below to target refs/heads/main. let _ = git_command() .args(["branch", "-D", "main"]) .current_dir(&clone_path) .output(); - let _ = git_command() - .args(["branch", "-m", "main"]) - .current_dir(&clone_path) - .output(); + if let Err(e) = run_fixture_git(&clone_path, &["branch", "-m", "main"]) { + cleanup(&clone_path); + return Err(anyhow::anyhow!( + "Failed to rename orphan branch to main: {}", + e + )); + } // Verify commit hash matches expected if commit_hash != MAINTAINER_DETERMINISTIC_COMMIT_HASH { @@ -1730,16 +1735,16 @@ impl<'a> TestContext<'a> { // Reset to orphan state and create deterministic root commit // Step 1: Create orphan branch (removes all history) - let _ = git_command() - .args(["checkout", "--orphan", "main-new"]) - .current_dir(&clone_path) - .output(); + if let Err(e) = run_fixture_git(&clone_path, &["checkout", "--orphan", "main-new"]) { + cleanup(&clone_path); + return Err(anyhow::anyhow!("Failed to create orphan branch: {}", e)); + } // Step 2: Clear staged files (orphan keeps files staged from previous branch) - let _ = git_command() - .args(["rm", "-rf", "--cached", "."]) - .current_dir(&clone_path) - .output(); + if let Err(e) = run_fixture_git(&clone_path, &["rm", "-rf", "--cached", "."]) { + cleanup(&clone_path); + return Err(anyhow::anyhow!("Failed to clear staged files: {}", e)); + } // Step 3: Create deterministic commit using recursive maintainer variant let commit_hash = match create_deterministic_commit_with_variant( @@ -1756,16 +1761,21 @@ impl<'a> TestContext<'a> { } }; - // Step 4: Replace main branch with our new orphan branch + // Step 4: Replace main branch with our new orphan branch. Deleting the + // old main is best-effort (the clone may not have had one); the rename + // must succeed for the push below to target refs/heads/main. let _ = git_command() .args(["branch", "-D", "main"]) .current_dir(&clone_path) .output(); - let _ = git_command() - .args(["branch", "-m", "main"]) - .current_dir(&clone_path) - .output(); + if let Err(e) = run_fixture_git(&clone_path, &["branch", "-m", "main"]) { + cleanup(&clone_path); + return Err(anyhow::anyhow!( + "Failed to rename orphan branch to main: {}", + e + )); + } // Verify commit hash matches expected if commit_hash != RECURSIVE_MAINTAINER_DETERMINISTIC_COMMIT_HASH { @@ -1936,11 +1946,11 @@ impl<'a> TestContext<'a> { )); } - // Create master branch if needed and push to refs/nostr/ - let _ = git_command() - .args(["branch", "-M", "master"]) - .current_dir(&clone_path) - .output(); + // Force-rename the current branch to master; the push below targets it. + if let Err(e) = run_fixture_git(&clone_path, &["branch", "-M", "master"]) { + cleanup(&clone_path); + return Err(anyhow::anyhow!("Failed to create master branch: {}", e)); + } let push_output = git_command() .args([ @@ -2070,16 +2080,16 @@ impl<'a> TestContext<'a> { // Reset to orphan state and create deterministic root commit // Step 1: Create orphan branch (removes all history) - let _ = git_command() - .args(["checkout", "--orphan", "pr-branch"]) - .current_dir(&clone_path) - .output(); + if let Err(e) = run_fixture_git(&clone_path, &["checkout", "--orphan", "pr-branch"]) { + cleanup(&clone_path); + return Err(anyhow::anyhow!("Failed to create orphan branch: {}", e)); + } // Step 2: Clear staged files (orphan keeps files staged from previous branch) - let _ = git_command() - .args(["rm", "-rf", "--cached", "."]) - .current_dir(&clone_path) - .output(); + if let Err(e) = run_fixture_git(&clone_path, &["rm", "-rf", "--cached", "."]) { + cleanup(&clone_path); + return Err(anyhow::anyhow!("Failed to clear staged files: {}", e)); + } // Step 3: Remove all working directory files for clean state (except .git) for entry in fs::read_dir(&clone_path) @@ -2340,6 +2350,26 @@ use std::fs; use std::path::{Path, PathBuf}; use std::time::Duration; +/// Run a git command in `repo_path` and fail on a non-zero exit. +/// +/// Fixture steps whose success later pushes and hash assertions depend on +/// must not fail silently. +fn run_fixture_git(repo_path: &Path, args: &[&str]) -> Result<(), String> { + let output = git_command() + .args(args) + .current_dir(repo_path) + .output() + .map_err(|e| format!("git {} failed to spawn: {}", args.join(" "), e))?; + if !output.status.success() { + return Err(format!( + "git {} failed: {}", + args.join(" "), + String::from_utf8_lossy(&output.stderr) + )); + } + Ok(()) +} + /// Clone a repository from the relay and return the path /// /// # Arguments @@ -2378,15 +2408,12 @@ pub fn clone_repo(relay_domain: &str, npub: &str, repo_id: &str) -> Result Result { + let _ = fs::remove_dir_all(&clone_path); + return TestResult::new(test_name, SpecRef::GitSetHeadOnReceive, desc) + .fail(format!("Failed to create develop1 branch: {}", e)); + } + Ok(out) if !out.status.success() => { + let _ = fs::remove_dir_all(&clone_path); + return TestResult::new(test_name, SpecRef::GitSetHeadOnReceive, desc).fail( + format!( + "Failed to create develop1 branch: {}", + String::from_utf8_lossy(&out.stderr) + ), + ); + } + Ok(_) => {} } // Create a unique commit on develop1 diff --git a/tests/common/nip09_helpers.rs b/tests/common/nip09_helpers.rs index 84a5751..36d7e4b 100644 --- a/tests/common/nip09_helpers.rs +++ b/tests/common/nip09_helpers.rs @@ -106,14 +106,21 @@ pub async fn publish_served_repo(client: &AuditClient, test_name: &str) -> (Even "deterministic commit hash mismatch" ); - let _ = grasp_audit::git_command() - .args(["branch", "main"]) + // Create or reset `main` at the deterministic commit and check it out. + // `-B` tolerates a pre-existing local `main` (the clone's initial branch + // follows the branch the server advertises as HEAD). + let checkout = grasp_audit::git_command() + .args(["checkout", "-B", "main"]) .current_dir(&clone_path) - .output(); - let _ = grasp_audit::git_command() - .args(["checkout", "main"]) - .current_dir(&clone_path) - .output(); + .output() + .expect("spawn git checkout -B main"); + if !checkout.status.success() { + cleanup(&clone_path); + panic!( + "failed to create/checkout main before push: {}", + String::from_utf8_lossy(&checkout.stderr) + ); + } let pushed = try_push(&clone_path); cleanup(&clone_path); @@ -222,14 +229,21 @@ pub async fn publish_served_repo_with_maintainers( "deterministic commit hash mismatch" ); - let _ = grasp_audit::git_command() - .args(["branch", "main"]) + // Create or reset `main` at the deterministic commit and check it out. + // `-B` tolerates a pre-existing local `main` (the clone's initial branch + // follows the branch the server advertises as HEAD). + let checkout = grasp_audit::git_command() + .args(["checkout", "-B", "main"]) .current_dir(&clone_path) - .output(); - let _ = grasp_audit::git_command() - .args(["checkout", "main"]) - .current_dir(&clone_path) - .output(); + .output() + .expect("spawn git checkout -B main"); + if !checkout.status.success() { + cleanup(&clone_path); + panic!( + "failed to create/checkout main before push: {}", + String::from_utf8_lossy(&checkout.stderr) + ); + } let pushed = try_push(&clone_path); cleanup(&clone_path); @@ -498,14 +512,21 @@ pub async fn publish_served_announcement_for_identifier( "deterministic commit hash mismatch" ); - let _ = grasp_audit::git_command() - .args(["branch", "main"]) + // Create or reset `main` at the deterministic commit and check it out. + // `-B` tolerates a pre-existing local `main` (the clone's initial branch + // follows the branch the server advertises as HEAD). + let checkout = grasp_audit::git_command() + .args(["checkout", "-B", "main"]) .current_dir(&clone_path) - .output(); - let _ = grasp_audit::git_command() - .args(["checkout", "main"]) - .current_dir(&clone_path) - .output(); + .output() + .expect("spawn git checkout -B main"); + if !checkout.status.success() { + cleanup(&clone_path); + panic!( + "failed to create/checkout main before push: {}", + String::from_utf8_lossy(&checkout.stderr) + ); + } let pushed = try_push(&clone_path); cleanup(&clone_path); @@ -611,14 +632,21 @@ pub async fn publish_served_announcement_with_state_for_identifier( "deterministic commit hash mismatch" ); - let _ = grasp_audit::git_command() - .args(["branch", "main"]) + // Create or reset `main` at the deterministic commit and check it out. + // `-B` tolerates a pre-existing local `main` (the clone's initial branch + // follows the branch the server advertises as HEAD). + let checkout = grasp_audit::git_command() + .args(["checkout", "-B", "main"]) .current_dir(&clone_path) - .output(); - let _ = grasp_audit::git_command() - .args(["checkout", "main"]) - .current_dir(&clone_path) - .output(); + .output() + .expect("spawn git checkout -B main"); + if !checkout.status.success() { + cleanup(&clone_path); + panic!( + "failed to create/checkout main before push: {}", + String::from_utf8_lossy(&checkout.stderr) + ); + } let pushed = try_push(&clone_path); cleanup(&clone_path); diff --git a/tests/common/purgatory_helpers.rs b/tests/common/purgatory_helpers.rs index 66db09f..07fa844 100644 --- a/tests/common/purgatory_helpers.rs +++ b/tests/common/purgatory_helpers.rs @@ -658,16 +658,10 @@ pub fn push_ref_to_relay( if check_output.status.success() { // Remote exists, update it - let _ = git_command() - .args(["remote", "set-url", "origin", &remote_url]) - .current_dir(local_path) - .output(); + run_git(local_path, &["remote", "set-url", "origin", &remote_url])?; } else { // Add new remote - let _ = git_command() - .args(["remote", "add", "origin", &remote_url]) - .current_dir(local_path) - .output(); + run_git(local_path, &["remote", "add", "origin", &remote_url])?; } // Push specific commit to specific ref diff --git a/tests/common/sync_helpers.rs b/tests/common/sync_helpers.rs index cb26bb5..468e174 100644 --- a/tests/common/sync_helpers.rs +++ b/tests/common/sync_helpers.rs @@ -1274,6 +1274,11 @@ pub async fn push_unique_git_data_to_relay( .current_dir(path) .output() .expect("git rev-parse"); + assert!( + out.status.success(), + "git rev-parse HEAD failed: {}", + String::from_utf8_lossy(&out.stderr) + ); String::from_utf8_lossy(&out.stdout).trim().to_string() };