fix(pr): resolve the PR base from the request and the platform, not the stored target

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"
                     );

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,
@@ -73,10 +93,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 {
@@ -367,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
                 {
@@ -376,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
@@ -412,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,
@@ -1483,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;
@@ -1588,4 +1661,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}"
+        );
+    }
 }

tests/common/mock_platform.rs

@@ -907,3 +907,25 @@ pub async fn mock_bb_reviewers(server: &MockServer, id: u64, reviewers: Vec<bool
         .mount(server)
         .await;
 }
+
+/// GitHub API response for a branch that EXISTS (GET /repos/:owner/:repo/branches/:branch).
+///
+/// `mock_not_found` already covers the absent case. Without this, no test
+/// could distinguish "the command asked about the right branch" from "the
+/// command asked about a branch that happened to be missing".
+pub async fn mock_branch_exists(server: &MockServer, owner: &str, repo: &str, branch: &str) {
+    let body = json!({
+        "name": branch,
+        "commit": { "sha": "0123456789abcdef0123456789abcdef01234567" },
+        "protected": false
+    });
+
+    Mock::given(method("GET"))
+        .and(path(format!(
+            "/repos/{}/{}/branches/{}",
+            owner, repo, branch
+        )))
+        .respond_with(ResponseTemplate::new(200).set_body_json(body))
+        .mount(server)
+        .await;
+}

tests/test_pr_create_base.rs

@@ -0,0 +1,144 @@
+//! `gr pr create --base` and the pre-flight base-existence check.
+//!
+//! `run_pr_create` resolves the base once, honoring `--base`, and then had a
+//! second expression that re-derived it from the manifest's stored target. The
+//! two disagreed whenever `--base` was passed, which is precisely when the
+//! stored target is stale -- passing `--base` is what we do BECAUSE it is
+//! stale. These tests pin the resolved base as the single source for the
+//! check, its messages, and the created PR.
+
+mod common;
+
+use common::fixtures::WorkspaceBuilder;
+use common::git_helpers;
+use common::mock_platform::{
+    mock_branch_exists, mock_create_pr, mock_not_found, point_repo_at_mock, setup_github_mock,
+};
+use wiremock::http::Method;
+
+/// Put the repo one commit ahead of `origin/<base>` 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<String> = 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"
+    );
+}