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 + ); +}