From 59e7b2040552b7f11892cbeed718328024bd6aa4 Mon Sep 17 00:00:00 2001 From: Layne Penney Date: Wed, 26 Aug 2026 11:43:29 -0500 Subject: [PATCH 1/3] fix(pr merge): report an unreadable base ref instead of silently clearing the merge verify_merge_commit_parents collapsed two different states onto None: an unreadable repository, which is deliberately silent so a missing local checkout cannot invent an outage, and a READABLE repository whose base ref cannot be read. The caller prints nothing for None, so the second state reported 'I could not look' in the exact shape of 'I looked and it was fine' -- clearance in the one direction the check cannot fail. Only the second state changes. The unreadable-repository case keeps its silence and its existing test still passes. Witness: an_absent_base_ref_in_a_readable_repository_is_reported, which carries a control asserting the same fixture DOES report a finding for a ref that exists -- without it, a guard broken into always returning None would pass by accident. --- src/cli/commands/pr/merge.rs | 63 +++++++++++++++++++++++++++++++++--- 1 file changed, 59 insertions(+), 4 deletions(-) diff --git a/src/cli/commands/pr/merge.rs b/src/cli/commands/pr/merge.rs index b6eac3c..ac9a9e4 100644 --- a/src/cli/commands/pr/merge.rs +++ b/src/cli/commands/pr/merge.rs @@ -73,10 +73,26 @@ fn verify_merge_commit_parents( // Fetch so the local ref reflects the merge that just happened remotely. let _ = crate::git::remote::fetch_remote(&repo, "origin"); - let reference = repo - .find_reference(&format!("refs/remotes/origin/{}", base)) - .ok()?; - let commit = reference.peel_to_commit().ok()?; + // A readable repository whose base ref we cannot read is NOT the same + // state as an unreadable checkout, and must not share its silence. The + // caller prints nothing for `None`, so returning `None` here would report + // "could not look" in the exact shape of "looked, and it was fine". + let ref_name = format!("refs/remotes/origin/{}", base); + let commit = match repo + .find_reference(&ref_name) + .and_then(|reference| reference.peel_to_commit()) + { + Ok(commit) => commit, + Err(e) => { + return Some(format!( + "could not check the merge result: {} is not readable ({}). \ + The merge was requested as {:?}, and a merge commit has two \ + parents, but this check did not run -- so nothing here says \ + the merge looks correct.", + ref_name, e, method + )); + } + }; let parents = commit.parent_count(); if parents >= 2 { @@ -1588,4 +1604,43 @@ mod parent_assertion_tests { let missing = dir.path().join("not-a-repo"); assert!(verify_merge_commit_parents(&missing, "main", MergeMethod::Merge).is_none()); } + + /// A READABLE repository whose named base ref does not exist. + /// + /// This is the state our own topology produces. `base` is the PR's base + /// branch, and a base branch can be deleted -- or, before the bind above + /// it was corrected, `base` could be a stale manifest target naming a + /// branch that no longer exists at all. `find_reference` then fails on a + /// repository that is perfectly readable, and a bare `?` collapsed that + /// onto the same `None` as an unreadable checkout. + /// + /// Those are two different states and only one of them was reasoned + /// about. "I looked and it was fine" and "I could not look" must not be + /// the same observation, because the caller prints nothing for `None` -- + /// so the silent branch reads as clearance in the one direction the + /// check cannot fail. + #[test] + fn an_absent_base_ref_in_a_readable_repository_is_reported() { + let dir = tempfile::tempdir().expect("tempdir"); + repo_with_head_parents(dir.path(), 1); + + // Control: the fixture must be able to produce a finding at all. + // Without this, a function that had been broken into always + // returning None would pass the assertion below by accident. + assert!( + verify_merge_commit_parents(dir.path(), "main", MergeMethod::Merge).is_some(), + "control: this fixture reports a single-parent head for a ref that EXISTS" + ); + + let problem = verify_merge_commit_parents(dir.path(), "sprint-39", MergeMethod::Merge); + let message = problem.expect("an absent base ref must be reported, not silently cleared"); + assert!( + message.contains("sprint-39"), + "the message must name the ref it could not read: {message}" + ); + assert!( + message.contains("could not"), + "and must say it could not check, never that the merge looked fine: {message}" + ); + } } From 537b894706d290880c92f7d95007e4c00fe5ef01 Mon Sep 17 00:00:00 2001 From: Layne Penney Date: Wed, 26 Aug 2026 11:46:10 -0500 Subject: [PATCH 2/3] fix(pr create): check the base the operator asked for, not the stored target run_pr_create resolves the base once at the top of the loop, honoring --base. The pre-flight existence check then re-derived it from the manifest's stored target, so the two disagreed on exactly the runs where --base was passed -- and --base is what we pass BECAUSE the stored target is stale. Measured symptom: with a retired sprint branch still stored, 'gr pr create --base dev' asked GitHub whether that retired branch existed, got 404 for a branch nobody named, and skipped the repo reporting a base the operator never asked for. The create call one block below was already passing the resolved base, so the only thing between the operator and a correct PR was a guard checking a different branch. Class swept, not just the cited instance: every remaining target_branch() in this file is either the resolution expression itself or sits inside the base_override.is_none() branch, where reading the stored target is correct. Witnesses (tests/test_pr_create_base.rs, run_pr_create end to end against wiremock, asserting the server's received-request log): - base_override_is_the_branch_that_gets_checked_and_the_pr_that_gets_opened fails on the old code with exactly the reported message, and carries a control requiring that SOME pre-flight check happened, so deleting the check could not pass it. - without_an_override_the_stored_target_is_still_what_gets_checked is the negative control; without it, 'prefer the override' and 'ignore the manifest' would be indistinguishable. run_pr_create had no test coverage before this. --- src/cli/commands/pr/create.rs | 14 ++-- tests/common/mock_platform.rs | 22 ++++++ tests/test_pr_create_base.rs | 144 ++++++++++++++++++++++++++++++++++ 3 files changed, 175 insertions(+), 5 deletions(-) create mode 100644 tests/test_pr_create_base.rs diff --git a/src/cli/commands/pr/create.rs b/src/cli/commands/pr/create.rs index 65e22bc..1a4c628 100644 --- a/src/cli/commands/pr/create.rs +++ b/src/cli/commands/pr/create.rs @@ -229,26 +229,30 @@ pub async fn run_pr_create( let spinner = Output::spinner(&format!("Creating PR for {}...", repo.name)); + // `target` above already resolved --base against the stored + // target. Re-deriving it here from the manifest asked about a + // branch the operator never named -- and the two only disagree + // when --base was passed, which is exactly when the stored + // target is stale. match platform - .check_branch_exists(&repo.owner, &repo.repo, repo.target_branch()) + .check_branch_exists(&repo.owner, &repo.repo, target) .await { Ok(false) => { spinner.finish_with_message(format!( "{}: skipped — base branch '{}' does not exist on remote", - repo.name, - repo.target_branch() + repo.name, target )); all_failed_repos.push(( repo.name.clone(), - format!("base branch '{}' not found on remote", repo.target_branch()), + format!("base branch '{}' not found on remote", target), )); continue; } Err(e) => { debug!( repo = repo.name.as_str(), - base = repo.target_branch(), + base = target, error = %e, "Could not verify base branch; proceeding to API call" ); diff --git a/tests/common/mock_platform.rs b/tests/common/mock_platform.rs index 5ab8ae0..50c4535 100644 --- a/tests/common/mock_platform.rs +++ b/tests/common/mock_platform.rs @@ -907,3 +907,25 @@ pub async fn mock_bb_reviewers(server: &MockServer, id: u64, reviewers: Vec` on a feature branch, so +/// the command's own selection step groups it and execution reaches the check. +fn repo_ahead_of(ws: &common::fixtures::WorkspaceFixture, repo: &str, base: &str, feature: &str) { + let path = ws.repo_path(repo); + git_helpers::create_branch(&path, base); + git_helpers::commit_file(&path, "base.txt", "base", "Add base"); + git_helpers::push_branch(&path, "origin", base); + git_helpers::create_branch(&path, feature); + git_helpers::commit_file(&path, "feature.txt", "feature", "Add feature"); +} + +/// The stale stored target must not be consulted when `--base` is explicit. +/// +/// Before the fix the command resolved `dev`, then asked the platform whether +/// `sprint-39` existed, got 404 for a branch nobody named, and skipped the +/// repo reporting a base the operator never asked for. The PR that GitHub +/// would have accepted was never attempted. +#[tokio::test] +async fn base_override_is_the_branch_that_gets_checked_and_the_pr_that_gets_opened() { + let (server, _adapter) = setup_github_mock().await; + + let ws = WorkspaceBuilder::new().add_repo("frontend").build(); + let mut manifest = ws.load_manifest(); + + // A retired sprint branch: still stored, gone from the remote. + manifest.settings.target = Some("sprint-39".to_string()); + + repo_ahead_of(&ws, "frontend", "dev", "feat/thing"); + point_repo_at_mock(&mut manifest, "frontend", &server); + + mock_branch_exists(&server, "owner", "repo", "dev").await; + mock_not_found(&server, "/repos/owner/repo/branches/sprint-39").await; + mock_create_pr(&server, 7, "https://github.com/owner/repo/pull/7").await; + + let filter = vec!["frontend".to_string()]; + let result = gitgrip::cli::commands::pr::run_pr_create( + &ws.workspace_root, + &manifest, + Some("Add feature"), + None, + false, + false, + false, + Some(&filter), + Some("dev"), + false, + ) + .await; + assert!(result.is_ok(), "command errored: {result:?}"); + + let requests = server.received_requests().await.unwrap(); + let branch_checks: Vec = requests + .iter() + .filter(|r| r.method == Method::GET && r.url.path().contains("/branches/")) + .map(|r| r.url.path().to_string()) + .collect(); + + // Control: the command must have performed a pre-flight check at all. + // Without it, deleting the check entirely would pass the assertion below. + assert!( + !branch_checks.is_empty(), + "control: a pre-flight base check must happen; saw none" + ); + assert!( + branch_checks.iter().any(|p| p.ends_with("/branches/dev")), + "the base the operator asked for must be the one checked; got {branch_checks:?}" + ); + assert!( + !branch_checks + .iter() + .any(|p| p.ends_with("/branches/sprint-39")), + "the stale stored target must not be consulted when --base is explicit; got {branch_checks:?}" + ); + + let posts: Vec<_> = requests + .iter() + .filter(|r| r.method == Method::POST && r.url.path().ends_with("/pulls")) + .collect(); + assert_eq!( + posts.len(), + 1, + "the PR must actually be created against the requested base" + ); +} + +/// The negative control for the test above: with NO `--base`, the stored +/// target is still the right thing to check. Without this case, "always use +/// the override" and "ignore the manifest entirely" are indistinguishable. +#[tokio::test] +async fn without_an_override_the_stored_target_is_still_what_gets_checked() { + let (server, _adapter) = setup_github_mock().await; + + let ws = WorkspaceBuilder::new().add_repo("frontend").build(); + let mut manifest = ws.load_manifest(); + manifest.settings.target = Some("release".to_string()); + + repo_ahead_of(&ws, "frontend", "release", "feat/thing"); + point_repo_at_mock(&mut manifest, "frontend", &server); + + mock_branch_exists(&server, "owner", "repo", "release").await; + mock_create_pr(&server, 8, "https://github.com/owner/repo/pull/8").await; + + let filter = vec!["frontend".to_string()]; + let result = gitgrip::cli::commands::pr::run_pr_create( + &ws.workspace_root, + &manifest, + Some("Add feature"), + None, + false, + false, + false, + Some(&filter), + None, + false, + ) + .await; + assert!(result.is_ok(), "command errored: {result:?}"); + + let requests = server.received_requests().await.unwrap(); + assert!( + requests + .iter() + .any(|r| r.method == Method::GET && r.url.path().ends_with("/branches/release")), + "with no override the stored target is the base and must be the one checked" + ); +} From b0c329ed62f28332093bf18f471e2a024c8fb3cb Mon Sep 17 00:00:00 2001 From: Layne Penney Date: Wed, 26 Aug 2026 11:49:53 -0500 Subject: [PATCH 3/3] fix(pr merge): bind the merge's base to the PR's actual base, not the stored target PRToMerge.base was set from repo.target_branch(), the workspace's stored target. The platform's answer was already in hand and discarded: the get_pull_request call two blocks above returns a PullRequest whose .base carries the branch the PR is genuinely open against, and only .mergeable was read off it. All four adapters populate base. The two values differ exactly when the stored target is stale, which is the ordinary state after a branch is retired. That wrong base then flowed into verify_merge_commit_parents -- the post-merge assertion added for the squash incident -- so it read a ref unrelated to the merge that just happened, and read nothing at all once the stored target named a deleted branch. Paired with the previous commit, which makes an unreadable base ref say so, the assertion now both looks at the right ref and reports when it cannot. The stored target stays as the fallback for a failed API call, and an empty platform answer falls back too rather than building refs/remotes/origin/. WITNESS SCOPE, stated rather than implied: base_binding_tests pins the resolution helper, NOT the call site. Measured, not assumed -- reverting only the call-site bind while leaving the helper intact fails no test in this suite. The call site's protection is structural, that repo.target_branch() no longer appears there as a standalone expression. An end-to-end witness would need the parent-assertion warning to be observable to a test; today it only reaches stdout. --- src/cli/commands/pr/merge.rs | 65 +++++++++++++++++++++++++++++++++--- 1 file changed, 61 insertions(+), 4 deletions(-) diff --git a/src/cli/commands/pr/merge.rs b/src/cli/commands/pr/merge.rs index ac9a9e4..064116e 100644 --- a/src/cli/commands/pr/merge.rs +++ b/src/cli/commands/pr/merge.rs @@ -61,6 +61,26 @@ fn resolve_check_status(status: &StatusCheckResult) -> CheckStatus { /// already happened. The point is that the operator finds out now, from the /// tool, rather than days later from a broken ancestry -- which is how the /// original incident was discovered. +/// The base a PR is actually open against. +/// +/// This is the hosting platform's answer, not the workspace's stored target. +/// The two differ whenever the stored target is stale, and a stale stored +/// target is the ordinary state after a branch is retired -- so binding the +/// merge's notion of "base" to the manifest meant the post-merge parent +/// assertion below read a ref that had nothing to do with the merge that just +/// happened, and read nothing at all once that ref was deleted. +/// +/// The platform value is already fetched for the mergeable flag; it was being +/// discarded one line later. The stored target remains the fallback for the +/// case where that call failed, because a plausible base still lets the +/// assertion run -- and the assertion now says so when it cannot read it. +fn pr_base_or_stored_target(platform_base: Option<&str>, stored_target: &str) -> String { + match platform_base { + Some(base) if !base.trim().is_empty() => base.to_string(), + _ => stored_target.to_string(), + } +} + fn verify_merge_commit_parents( local_path: &std::path::Path, base: &str, @@ -383,7 +403,7 @@ pub async fn run_pr_merge( { Ok(Some(pr)) => { // Get PR details - let (approved, mergeable) = match platform + let (approved, mergeable, platform_base) = match platform .get_pull_request(&repo.owner, &repo.repo, pr.number) .await { @@ -392,9 +412,13 @@ pub async fn run_pr_merge( .is_pull_request_approved(&repo.owner, &repo.repo, pr.number) .await .unwrap_or(false); - (is_approved, full_pr.mergeable.unwrap_or(false)) + ( + is_approved, + full_pr.mergeable.unwrap_or(false), + Some(full_pr.base.ref_name.clone()), + ) } - Err(_) => (false, false), + Err(_) => (false, false, None), }; // Get status checks @@ -428,7 +452,7 @@ pub async fn run_pr_merge( owner: repo.owner.clone(), repo: repo.repo.clone(), branch: branch.clone(), - base: repo.target_branch().to_string(), + base: pr_base_or_stored_target(platform_base.as_deref(), repo.target_branch()), local_path: repo.absolute_path.clone(), pr_number: pr.number, platform, @@ -1499,6 +1523,39 @@ settings: } } +#[cfg(test)] +mod base_binding_tests { + use super::pr_base_or_stored_target; + + /// The defect, as a test. A retired sprint branch stays in the manifest + /// long after it stops existing; the PR is open against something else. + #[test] + fn the_platform_base_wins_over_a_stale_stored_target() { + assert_eq!( + pr_base_or_stored_target(Some("dev"), "sprint-39"), + "dev", + "the PR is open against what the platform says, not what the workspace stored" + ); + } + + /// The fallback. Without this case, "prefer the platform" and "ignore the + /// manifest entirely" are indistinguishable, and a failed API call would + /// leave the parent assertion with no ref name at all. + #[test] + fn an_absent_platform_answer_falls_back_to_the_stored_target() { + assert_eq!(pr_base_or_stored_target(None, "main"), "main"); + } + + /// An adapter answering with an empty string must not produce the ref + /// `refs/remotes/origin/`, which would fail to resolve -- and before the + /// parent assertion learned to report that, would have gone silent. + #[test] + fn an_empty_platform_answer_falls_back_rather_than_building_a_bare_ref() { + assert_eq!(pr_base_or_stored_target(Some(""), "main"), "main"); + assert_eq!(pr_base_or_stored_target(Some(" "), "main"), "main"); + } +} + #[cfg(test)] mod parent_assertion_tests { use super::verify_merge_commit_parents;