From 0e320e56eaddcb3ec4ac185bc43190734bc81c70 Mon Sep 17 00:00:00 2001 From: Layne Penney Date: Fri, 21 Aug 2026 08:29:52 -0500 Subject: [PATCH] fix(pr): count failed merges and disjoin them into the exit code gr pr merge reported success when every merge in a run had failed. The per-repo diagnostics were printed and truthful; the exit code was not, so a script driving the command was told a batch had merged when none of it had. Three exits returned Ok while carrying failures: - an empty candidate list that was empty because the PR lookups FAILED. "We looked and found nothing" and "we could not look" are different answers, and only the first is an absence of PRs. They are now tracked separately rather than arriving at the summary as one state. - the --auto path, where every attempt to enable auto-merge could fail. - the final exit, after a mixed or wholly failed run. Each now fails with a count of what failed against what was attempted. The test fixtures needed correcting first. mock_get_pr mounts a single invariant response, so a read issued after a successful merge still reported merged: false, which the real API cannot produce. Three groups of tests were affected, by three different causes, and they should not be counted together. FIVE tests began failing for the fixture reason once the exit code became truthful. The new mock_pr_lifecycle couples the GET and the merge PUT through shared state so the sequence behaves as the live API does; all five then passed with NO ASSERTION CHANGED, which is what distinguishes a fixture defect from a contract change. A SIXTH test, repo_filter_excludes_non_target, carried the same fixture defect and never went red at all: its PR was mocked as state "open" and merged: true simultaneously, making the command's own post-merge verification vacuous, so it passed whether or not the merge did anything. It now starts unmerged and GAINS an assertion that the merge actually fired. ONE test, branch_behind_suggests_update, was red for an unrelated reason and is the only assertion this change edits. It required is_ok() while its own failure message said "handled without crashing". Those are different claims. The merge did not happen, so graceful handling means a useful error rather than a success. Its fixture is untouched. Every added guard is mutation-proved: neutering each of the three exits independently turns a named test red. The --auto path had no test of any kind before this change, so its guard arrived with the first one. Ref #884 -- closes at promotion Ref #886 -- closes at promotion --- src/cli/commands/pr/merge.rs | 56 ++++++++ tests/common/mock_platform.rs | 100 ++++++++++++- tests/test_pr_merge.rs | 259 +++++++++++++++++++++++++++++++--- 3 files changed, 398 insertions(+), 17 deletions(-) diff --git a/src/cli/commands/pr/merge.rs b/src/cli/commands/pr/merge.rs index 76f28c9..b6eac3c 100644 --- a/src/cli/commands/pr/merge.rs +++ b/src/cli/commands/pr/merge.rs @@ -333,6 +333,11 @@ pub async fn run_pr_merge( let mut prs_to_merge: Vec = Vec::new(); let mut json_skipped: Vec = Vec::new(); + // A repo whose PR lookup failed belongs in neither of the two collections + // above: it is not a merge candidate and it was not skipped. Without a + // third one, "we looked and found nothing" and "we could not look" arrive + // at the summary as the same state. + let mut lookup_failures: Vec = Vec::new(); for repo in &all_repos { if !path_exists(&repo.absolute_path) { @@ -426,11 +431,24 @@ pub async fn run_pr_merge( if !opts.json { Output::error(&format!("{}: {}", repo.name, e)); } + lookup_failures.push(repo.name.clone()); } } } if prs_to_merge.is_empty() { + // An empty candidate list has two causes that read identically here. + // Only one of them is an absence of PRs; the other is an absence of + // knowledge, and reporting it as the first is a false statement the + // exit code then endorses. + if !lookup_failures.is_empty() { + anyhow::bail!( + "could not determine PR state for {} of {} repositories: {}", + lookup_failures.len(), + all_repos.len(), + lookup_failures.join(", ") + ); + } println!("No open PRs found for any repository."); println!("Repositories checked: {}", all_repos.len()); return Ok(()); @@ -730,6 +748,18 @@ pub async fn run_pr_merge( )); } + // Case 3: any per-repo failure makes the run a failure, including a + // mixed run. The warning above is read by a human; the exit code is + // the only part a script sees, and it reported this as done. + if error_count > 0 || !lookup_failures.is_empty() { + anyhow::bail!( + "{} of {} auto-merge attempts failed{}", + error_count, + success_count + error_count, + describe_lookup_failures(&lookup_failures) + ); + } + return Ok(()); } @@ -1112,9 +1142,35 @@ pub async fn run_pr_merge( } } + // Case 3, on the path that matters most: a run that failed to merge some + // of the PRs it selected has already emitted its JSON document and its + // human summary by this point. Both are truthful. The exit code was not. + if error_count > 0 || !lookup_failures.is_empty() { + anyhow::bail!( + "{} of {} PR merges failed{}", + error_count, + success_count + error_count, + describe_lookup_failures(&lookup_failures) + ); + } + Ok(()) } +/// Render the lookup-failure tail of a summary, or nothing when every repo +/// was successfully inspected. Kept separate so the three case-3 exits phrase +/// the same fact identically. +fn describe_lookup_failures(failures: &[String]) -> String { + if failures.is_empty() { + String::new() + } else { + format!( + "; PR state could not be determined for {}", + failures.join(", ") + ) + } +} + /// Check if a repo has changes ahead of its default branch /// Returns Ok(true) if there are changes, Ok(false) if no changes or on default branch fn check_repo_for_changes(repo: &RepoInfo) -> anyhow::Result { diff --git a/tests/common/mock_platform.rs b/tests/common/mock_platform.rs index 9aa6b4f..5ab8ae0 100644 --- a/tests/common/mock_platform.rs +++ b/tests/common/mock_platform.rs @@ -4,8 +4,10 @@ //! testing of platform adapter methods. use serde_json::{json, Map, Value}; +use std::sync::atomic::{AtomicBool, Ordering}; +use std::sync::Arc; use wiremock::matchers::{header, method, path}; -use wiremock::{Mock, MockServer, ResponseTemplate}; +use wiremock::{Mock, MockServer, Request, Respond, ResponseTemplate}; /// Start a wiremock server and configure GITHUB_TOKEN env var. /// Returns the server and a GitHubAdapter pointed at it. @@ -376,6 +378,76 @@ pub async fn mock_merge_pr(server: &MockServer, number: u64, merged: bool) { .await; } +/// GitHub API: a PR whose GET response reflects whether its merge PUT has fired. +/// +/// `mock_get_pr` mounts one invariant response, so a GET issued *after* a +/// successful merge PUT still reports `merged: false`. That is a state the real +/// API cannot produce, and any command that verifies its own merge by reading +/// the PR back sees a contradiction that belongs to the fixture rather than to +/// the code under test. This helper couples the two endpoints through shared +/// state so the sequence GET -> PUT -> GET behaves as the live API does. +/// +/// Returns the merged flag so a test can assert the PUT actually fired. That +/// matters: without it, a command that never attempted the merge and a command +/// that merged successfully both leave the fixture reporting `merged: false` +/// for different reasons. +pub async fn mock_pr_lifecycle(server: &MockServer, number: u64) -> Arc { + let merged = Arc::new(AtomicBool::new(false)); + + struct ReadPr { + merged: Arc, + number: u64, + } + + impl Respond for ReadPr { + fn respond(&self, _request: &Request) -> ResponseTemplate { + let is_merged = self.merged.load(Ordering::SeqCst); + ResponseTemplate::new(200).set_body_json(github_pr_json( + self.number, + if is_merged { "closed" } else { "open" }, + "feat/test", + "main", + is_merged, + "PR description\n", + )) + } + } + + struct MergePr { + merged: Arc, + } + + impl Respond for MergePr { + fn respond(&self, _request: &Request) -> ResponseTemplate { + self.merged.store(true, Ordering::SeqCst); + ResponseTemplate::new(200).set_body_json(json!({ + "sha": "merge123", + "merged": true, + "message": "Pull Request successfully merged" + })) + } + } + + Mock::given(method("GET")) + .and(path(format!("/repos/owner/repo/pulls/{}", number))) + .respond_with(ReadPr { + merged: Arc::clone(&merged), + number, + }) + .mount(server) + .await; + + Mock::given(method("PUT")) + .and(path(format!("/repos/owner/repo/pulls/{}/merge", number))) + .respond_with(MergePr { + merged: Arc::clone(&merged), + }) + .mount(server) + .await; + + merged +} + /// GitHub API: merge PR returns 405 with "branch behind" message. pub async fn mock_merge_pr_behind(server: &MockServer, number: u64) { let body = json!({ @@ -666,6 +738,32 @@ pub fn point_repo_at_mock( } /// Mock a GitHub repo info response (GET /repos/:owner/:repo). +/// GitHub API: repo info with explicit merge-method permissions. +/// +/// `mock_repo_info` always reports every method as allowed, so no test could +/// reach the branch where a command refuses a method the repository forbids. +pub async fn mock_repo_info_with_methods( + server: &MockServer, + owner: &str, + repo: &str, + allow_squash: bool, + allow_merge_commit: bool, + allow_rebase: bool, +) { + let mut body = github_repo_json(owner, repo); + if let Value::Object(ref mut m) = body { + m.insert("allow_squash_merge".into(), json!(allow_squash)); + m.insert("allow_merge_commit".into(), json!(allow_merge_commit)); + m.insert("allow_rebase_merge".into(), json!(allow_rebase)); + } + + Mock::given(method("GET")) + .and(path(format!("/repos/{}/{}", owner, repo))) + .respond_with(ResponseTemplate::new(200).set_body_json(body)) + .mount(server) + .await; +} + pub async fn mock_repo_info(server: &MockServer, owner: &str, repo: &str) { let body = github_repo_json(owner, repo); diff --git a/tests/test_pr_merge.rs b/tests/test_pr_merge.rs index 3cf186e..62426a9 100644 --- a/tests/test_pr_merge.rs +++ b/tests/test_pr_merge.rs @@ -10,7 +10,8 @@ use common::fixtures::WorkspaceBuilder; use common::git_helpers; use common::mock_platform::{ mock_check_runs, mock_get_pr, mock_legacy_combined_status, mock_list_prs, mock_merge_pr, - mock_merge_pr_behind, mock_pr_reviews, point_repo_at_mock, setup_github_mock, + mock_merge_pr_behind, mock_pr_lifecycle, mock_pr_reviews, mock_repo_info_with_methods, + mock_server_error, mock_server_error_put, point_repo_at_mock, setup_github_mock, }; use gitgrip::core::manifest::{PlatformConfig, PlatformType}; use wiremock::http::Method; @@ -235,10 +236,9 @@ async fn test_pr_merge_force_bypasses_checks() { }); mock_list_prs(&server, vec![(42, "feat/test")]).await; - mock_get_pr(&server, 42, "open", false).await; + mock_pr_lifecycle(&server, 42).await; mock_pr_reviews(&server, 42, vec![("COMMENTED", "alice")]).await; mock_check_runs(&server, "feat/test", vec![("CI", "in_progress", None)]).await; - mock_merge_pr(&server, 42, true).await; let result = gitgrip::cli::commands::pr::run_pr_merge( &ws.workspace_root, @@ -331,10 +331,16 @@ async fn test_pr_merge_branch_behind_suggests_update() { ) .await; + // This assertion used to require `is_ok()` while its own message said + // "handled without crashing" -- two different claims. The merge genuinely + // did not happen, so graceful handling means a useful error, not a success. + // Reporting a batch as done when every merge in it failed is the exact + // false-success this change removes. + let error = result.expect_err("a branch-behind merge did not merge, so the run failed"); assert!( - result.is_ok(), - "branch-behind merge should be handled without crashing: {:?}", - result.err() + error.to_string().contains("1 of 1"), + "the error should name how many merges failed, got: {}", + error ); let requests = server.received_requests().await.unwrap(); @@ -380,7 +386,12 @@ async fn test_pr_merge_repo_filter_excludes_non_target() { // Only mock PR for frontend (PR #10). Backend should never be queried. mock_list_prs(&server, vec![(10, "feat/shared")]).await; - mock_get_pr(&server, 10, "open", true).await; + // The fixture here used to be `mock_get_pr(&server, 10, "open", true)` -- a PR + // reported as state "open" AND already merged, which the real API cannot + // produce. It made the command's own post-merge verification vacuous: the + // read-back said "merged" whether or not the merge had done anything, so + // this test was green for a reason unrelated to what it claims to check. + let merged_flag = mock_pr_lifecycle(&server, 10).await; mock_pr_reviews(&server, 10, vec![("APPROVED", "alice")]).await; mock_check_runs( &server, @@ -388,7 +399,6 @@ async fn test_pr_merge_repo_filter_excludes_non_target() { vec![("CI", "completed", Some("success"))], ) .await; - mock_merge_pr(&server, 10, true).await; // Filter to frontend only, force to bypass readiness checks let result = gitgrip::cli::commands::pr::run_pr_merge( @@ -428,6 +438,14 @@ async fn test_pr_merge_repo_filter_excludes_non_target() { 1, "exactly one merge request should be sent (frontend only, not backend)" ); + + // The request count proves the filter. This proves the merge actually + // happened -- the claim the old fixture asserted by construction rather + // than by observing anything. + assert!( + merged_flag.load(std::sync::atomic::Ordering::SeqCst), + "the filtered repo's PR should have been merged, not merely attempted" + ); } // ── Repo Filter: No Matching Repos ──────────────────────────── @@ -498,10 +516,9 @@ async fn test_pr_merge_force_yes_merges_without_prompt() { }); mock_list_prs(&server, vec![(42, "feat/test")]).await; - mock_get_pr(&server, 42, "open", false).await; + mock_pr_lifecycle(&server, 42).await; mock_pr_reviews(&server, 42, vec![]).await; mock_check_runs(&server, "feat/test", vec![("CI", "in_progress", None)]).await; - mock_merge_pr(&server, 42, true).await; // --force --yes should merge without stdin prompt let result = gitgrip::cli::commands::pr::run_pr_merge( @@ -632,7 +649,7 @@ async fn test_pr_merge_all_flag_proceeds_and_merges_every_match() { } mock_list_prs(&server, vec![(1, "feat/shared-name")]).await; - mock_get_pr(&server, 1, "open", false).await; + mock_pr_lifecycle(&server, 1).await; mock_pr_reviews(&server, 1, vec![("APPROVED", "alice")]).await; mock_check_runs( &server, @@ -640,7 +657,6 @@ async fn test_pr_merge_all_flag_proceeds_and_merges_every_match() { vec![("CI", "completed", Some("success"))], ) .await; - mock_merge_pr(&server, 1, true).await; let result = gitgrip::cli::commands::pr::run_pr_merge( &ws.workspace_root, @@ -712,13 +728,12 @@ async fn test_pr_merge_wait_does_not_block_when_no_checks_are_configured() { }); mock_list_prs(&server, vec![(42, "feat/no-ci")]).await; - mock_get_pr(&server, 42, "open", false).await; + mock_pr_lifecycle(&server, 42).await; mock_pr_reviews(&server, 42, vec![("APPROVED", "alice")]).await; // Exact GitHub shape for a ref with no CI configured: check-runs reports // zero runs, and the legacy fallback reports "pending" with zero statuses. mock_check_runs(&server, "feat/no-ci", vec![]).await; mock_legacy_combined_status(&server, "feat/no-ci", "pending", vec![]).await; - mock_merge_pr(&server, 42, true).await; let start = std::time::Instant::now(); let result = gitgrip::cli::commands::pr::run_pr_merge( @@ -942,7 +957,7 @@ async fn test_skip_gate_approval_allows_a_comment_ratified_merge() { // PR that is open and mergeable. Passing `true` here made it UNmergeable and // the merge was correctly blocked by the `mergeable` gate, which looked like // the waiver failing. The scenario was wrong, not the waiver. - mock_get_pr(&server, 42, "open", false).await; + mock_pr_lifecycle(&server, 42).await; mock_pr_reviews(&server, 42, vec![("COMMENTED", "alice")]).await; mock_check_runs( &server, @@ -950,7 +965,6 @@ async fn test_skip_gate_approval_allows_a_comment_ratified_merge() { vec![("CI", "completed", Some("success"))], ) .await; - mock_merge_pr(&server, 42, true).await; let result = gitgrip::cli::commands::pr::run_pr_merge( &ws.workspace_root, @@ -982,3 +996,216 @@ async fn test_skip_gate_approval_allows_a_comment_ratified_merge() { "approval was the only failing gate and it was waived by name — the merge must proceed" ); } + +#[tokio::test] +async fn test_pr_merge_all_lookups_failing_is_not_success() { + let (server, _adapter) = setup_github_mock().await; + mock_server_error(&server, "/repos/owner/repo/pulls").await; + + let ws = WorkspaceBuilder::new() + .add_repo("frontend") + .add_repo("backend") + .build(); + + let mut manifest = ws.load_manifest(); + point_repo_at_mock(&mut manifest, "frontend", &server); + point_repo_at_mock(&mut manifest, "backend", &server); + + // Both off the default branch, or they are skipped before any lookup runs + // and the witness would pass for the wrong reason. + for name in ["frontend", "backend"] { + git_helpers::create_branch(&ws.repo_path(name), "feat/witness"); + git_helpers::commit_file(&ws.repo_path(name), "w.txt", "w", "witness commit"); + } + + let result = gitgrip::cli::commands::pr::run_pr_merge( + &ws.workspace_root, + &manifest, + &gitgrip::cli::commands::pr::MergeOptions { + method: None, + force: false, + skip_gates: Vec::new(), + update: false, + auto: false, + json: false, + wait: false, + timeout: 600, + delete_branch: true, + repo_filter: None, + yes: true, + allow_all: false, + }, + ) + .await; + + assert!( + result.is_err(), + "every PR lookup failed; the command must not report success" + ); +} + +#[tokio::test] +async fn test_pr_merge_all_lookups_failing_exits_nonzero() { + let (server, _adapter) = setup_github_mock().await; + + let ws = WorkspaceBuilder::new().add_repo("app").build(); + let mut manifest = ws.load_manifest(); + + git_helpers::create_branch(&ws.repo_path("app"), "feat/test"); + git_helpers::commit_file( + &ws.repo_path("app"), + "feature.txt", + "feature", + "Add feature", + ); + + point_repo_at_mock(&mut manifest, "app", &server); + let manifest_yaml = serde_yaml::to_string(&manifest).unwrap(); + std::fs::write( + ws.workspace_root.join(".gitgrip/spaces/main/gripspace.yml"), + manifest_yaml, + ) + .unwrap(); + + // Every PR lookup fails, so the candidate list is empty for a reason that + // is not "there are no PRs". + mock_server_error(&server, "/repos/owner/repo/pulls").await; + + let output = tokio::process::Command::new(assert_cmd::cargo::cargo_bin!("gr")) + .current_dir(&ws.workspace_root) + .env("GITHUB_TOKEN", "test") + .args(["pr", "merge", "--method", "merge", "--yes"]) + .output() + .await + .unwrap(); + + assert_ne!( + output.status.code(), + Some(0), + "a run that could not determine PR state must not exit 0; stdout={} stderr={}", + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr) + ); +} + +#[tokio::test] +async fn test_pr_merge_failed_merge_exits_nonzero() { + let (server, _adapter) = setup_github_mock().await; + + let ws = WorkspaceBuilder::new().add_repo("app").build(); + let mut manifest = ws.load_manifest(); + + git_helpers::create_branch(&ws.repo_path("app"), "feat/test"); + git_helpers::commit_file( + &ws.repo_path("app"), + "feature.txt", + "feature", + "Add feature", + ); + + point_repo_at_mock(&mut manifest, "app", &server); + let manifest_yaml = serde_yaml::to_string(&manifest).unwrap(); + std::fs::write( + ws.workspace_root.join(".gitgrip/spaces/main/gripspace.yml"), + manifest_yaml, + ) + .unwrap(); + + mock_list_prs(&server, vec![(42, "feat/test")]).await; + mock_get_pr(&server, 42, "open", false).await; + mock_pr_reviews(&server, 42, vec![("APPROVED", "alice")]).await; + mock_check_runs( + &server, + "feat/test", + vec![("CI", "completed", Some("success"))], + ) + .await; + // The lookup succeeds and the merge itself fails: a genuine per-repo error + // on the path that matters most. + mock_server_error_put(&server, "/repos/owner/repo/pulls/42/merge").await; + + let output = tokio::process::Command::new(assert_cmd::cargo::cargo_bin!("gr")) + .current_dir(&ws.workspace_root) + .env("GITHUB_TOKEN", "test") + .args(["pr", "merge", "--force", "--method", "merge", "--yes"]) + .output() + .await + .unwrap(); + + assert_ne!( + output.status.code(), + Some(0), + "a failed merge must not exit 0; stdout={} stderr={}", + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr) + ); +} + +/// The `--auto` path had no test of any kind, so its exit code was unverified +/// in both directions. A PR whose requested merge method the repository forbids +/// is counted as an error inside the auto loop; before this change the run +/// still returned Ok, so a script enabling auto-merge across a workspace could +/// be told every PR was queued when none of them were. +#[tokio::test] +async fn test_pr_merge_auto_enable_failure_exits_nonzero() { + let (server, _adapter) = setup_github_mock().await; + + let ws = WorkspaceBuilder::new().add_repo("app").build(); + let mut manifest = ws.load_manifest(); + + git_helpers::create_branch(&ws.repo_path("app"), "feat/test"); + git_helpers::commit_file( + &ws.repo_path("app"), + "feature.txt", + "feature", + "Add feature", + ); + + let repo_config = manifest.repos.get_mut("app").unwrap(); + repo_config.url = Some("https://github.com/owner/repo.git".to_string()); + repo_config.platform = Some(PlatformConfig { + platform_type: PlatformType::GitHub, + base_url: Some(server.uri()), + }); + + mock_list_prs(&server, vec![(42, "feat/test")]).await; + mock_pr_lifecycle(&server, 42).await; + mock_pr_reviews(&server, 42, vec![("APPROVED", "alice")]).await; + mock_check_runs( + &server, + "feat/test", + vec![("CI", "completed", Some("success"))], + ) + .await; + // Readiness passes, so the run reaches the auto loop. The repository then + // forbids the requested method, which is the loop's own error branch. + mock_repo_info_with_methods(&server, "owner", "repo", true, false, true).await; + + let merge_method = gitgrip::platform::MergeMethod::Merge; + let result = gitgrip::cli::commands::pr::run_pr_merge( + &ws.workspace_root, + &manifest, + &gitgrip::cli::commands::pr::MergeOptions { + method: Some(&merge_method), + force: false, + skip_gates: Vec::new(), + update: false, + auto: true, + json: false, + wait: false, + timeout: 600, + delete_branch: true, + repo_filter: None, + yes: true, + allow_all: false, + }, + ) + .await; + + let error = result.expect_err("no auto-merge was enabled, so the run failed"); + assert!( + error.to_string().contains("auto-merge attempts failed"), + "the error should say the auto-merge attempts failed, got: {}", + error + ); +}