Leave a ready-for-review PR on green CI when the moved Base branch merges cleanly

GLOSSARY.md

@@ -218,7 +218,7 @@ What a **Run**, or a **Spec run** for its **Spec PR**, does from its worktree on
 If a Delivery gate observes that its pull request has already merged into the expected Base branch, and its merged head includes all local work (the exact local head, with no uncommitted changes), the Run ends as merged, even when it originally asked only for ready for review. Delivery stops further publication, PR edits and Self-merge work. A merged PR that leaves local work out still fails and retains that work; a merged state alone never establishes completion.
 
 **Repair loop**:
-The rounds a **Delivery** goes through until the pull request's head is mergeable with the **Base branch** and its CI is green or absent. Each round merges the Base branch in (and, in a **Merge run**, any **Foreign commits**), pushes, watches CI, and starts a **Repair** for a conflict or for the branch's own red checks. Repairs and upstream moves have fixed budgets.
+The rounds a **Delivery** goes through until the pull request's head is mergeable with the **Base branch** and its CI is green or absent. Each round merges the Base branch in (and, in a **Merge run**, any **Foreign commits**), pushes, watches CI, and starts a **Repair** for a conflict or for the branch's own red checks. When the Base branch moved while green or absent CI ran, a Merge run goes round again to merge it in. A Delivery that leaves the pull request ready for review stops on that head if GitHub reports the pull request mergeable, and the move is not counted; otherwise it goes round again. Repairs and upstream moves have fixed budgets.
 
 **Foreign commit**:
 A commit that appears on the **Issue branch** during a **Run** and was made neither by one of that Run's sessions nor by thirdshift itself. Commits already on the branch when a **Continuation** starts are not Foreign commits. A **Merge run** merges one only after a **Repair** has reviewed its resulting tree, unless that tree is identical to the Run's own head before reconciliation and is already covered by its existing review. A tree-identical batch still counts as an upstream move, but uses no Review Repair.

site/how-it-works/index.html

@@ -126,7 +126,7 @@ <h1>How thirdshift works</h1>
         </ol>
         <div class="guide-pair guide-loop-branches">
           <div class="guide-node"><strong>Conflict or the branch's own CI failure?</strong><span>Start a Repair to resolve or fix it.</span><a href="#delivery-sync">Then return to merging the Base branch ↑</a></div>
-          <div class="guide-node"><strong>Base branch moved during CI?</strong><span>Reconcile the new upstream state.</span><a href="#delivery-sync">Return to merging the Base branch ↑</a></div>
+          <div class="guide-node"><strong>Base branch moved during CI?</strong><span>A Self-merge reconciles the new upstream state. A pull request left for review stops on green CI if it still merges cleanly.</span><a href="#delivery-sync">Otherwise return to merging the Base branch ↑</a></div>
         </div>
       </figure>
       <p>Repair and upstream-move budgets keep the loop bounded. If a CI-fix Repair leaves the same failing head, that head gets one <strong>Check re-run</strong>; if its own checks stay red, the Run fails rather than spending more Repairs on unchanged code.</p>

src/delivery.rs

@@ -743,6 +743,10 @@ impl repair_loop::Outside for RunOutside<'_> {
             .fix(outside, self.pr_url, self.base, base_commit, failed)
     }
 
+    fn pr_mergeable(&mut self) -> Result<bool> {
+        self.pull_request.is_mergeable()
+    }
+
     fn ensure_pr_ready_and_mergeable(&mut self) -> Result<()> {
         self.pull_request.ensure_ready_and_mergeable()
     }

src/delivery/repair_loop.rs

@@ -92,6 +92,9 @@ pub(super) trait Outside {
     /// go round again.
     fn base_fix(&mut self, base_commit: &str, failed: &FailedChecks) -> Result<()>;
 
+    /// Whether GitHub reports the open, ready pull request mergeable with the
+    /// Base branch. Mergeability it has not worked out in time is not.
+    fn pr_mergeable(&mut self) -> Result<bool>;
     /// Fail unless the pull request is open, ready for review and mergeable.
     fn ensure_pr_ready_and_mergeable(&mut self) -> Result<()>;
     /// Merge the pull request at `head`.
@@ -162,7 +165,9 @@ const MAX_REPAIRS: usize = 5;
 /// CI ran or since a merge was tried, or, in a Merge run, because Foreign
 /// commits arrived, whether or not the merge that follows needs a Repair. A
 /// clean merge of the Base branch uses no Repair, so without this a busy Base
-/// branch could keep a Delivery going forever.
+/// branch could keep a Delivery going forever. A Base branch that moved while
+/// green or absent CI ran is not counted for a Delivery left ready for review
+/// whose pull request still merges cleanly, since that Delivery stops there.
 const MAX_UPSTREAM_MOVES: usize = 5;
 
 /// What a Delivery has spent of its Repair and upstream-move budgets,
@@ -288,6 +293,13 @@ impl<O: Outside> RepairLoop<'_, O> {
         self.outside.repair(kind, repair)
     }
 
+    /// Whether GitHub reports the pull request mergeable, read in the
+    /// readiness stage.
+    fn pr_merges_cleanly(&mut self) -> Result<bool> {
+        self.outside.stage(Stage::PullRequestReadiness);
+        self.outside.pr_mergeable()
+    }
+
     /// Count a round taken because the Base branch moved `when`.
     fn count_base_move(&mut self, when: &str) -> Result<()> {
         let line = self.budgets.count_base_move(self.base, when)?;
@@ -299,8 +311,11 @@ impl<O: Outside> RepairLoop<'_, O> {
     /// Base branch (never rebase), push, and watch CI on the head commit,
     /// starting a Repair session for a conflict or red
     /// CI and then going round again, since the Base branch may have moved
-    /// meanwhile. Green or absent CI also goes round again if the Base branch
-    /// moved while CI ran, or, in a Merge run, if Foreign commits arrived.
+    /// meanwhile. Green or absent CI also goes round again, in a Merge run, if
+    /// the Base branch moved while CI ran or Foreign commits arrived. Left
+    /// ready for review, it returns that head without counting the move if
+    /// GitHub reports the pull request mergeable, and otherwise goes round
+    /// again.
     /// A red check that also fails on the Base branch commit the head last
     /// merged in is an Inherited failure, which no Repair is started for: the
     /// CI-fix Repair is given the branch's own failures, and when there are
@@ -348,6 +363,20 @@ impl<O: Outside> RepairLoop<'_, O> {
             match ci {
                 Ci::Absent | Ci::Passed => {
                     if self.outside.base_branch_moved()? {
+                        // Green CI on a head that merges cleanly is enough
+                        // for review; only a Self-merge needs the moved Base
+                        // branch merged in and watched.
+                        if self.goal == Goal::ReadyForReview {
+                            if self.pr_merges_cleanly()? {
+                                self.outside.progress(format!(
+                                    "origin/{base} moved while CI ran; leaving the PR for \
+                                     review on {}, which merges cleanly",
+                                    ci::short(&head)
+                                ));
+                                return Ok(head);
+                            }
+                            self.outside.stage(Stage::RepairLoopRound);
+                        }
                         self.count_base_move("while CI ran")?;
                     } else if self.goal == Goal::ReadyForReview
                         || self.outside.new_commits_on_origin()?.commits().is_empty()
@@ -463,6 +492,8 @@ mod tests {
         /// Took the Base fix for these Inherited failures, at this Base
         /// branch commit.
         BaseFix(String, Vec<String>),
+        /// Read whether GitHub reports the PR mergeable.
+        ReadMergeable,
         /// Checked the PR is open, ready and mergeable.
         CheckPr,
         /// Merged the PR at this head.
@@ -519,8 +550,8 @@ mod tests {
 
     /// What the outside world answers, call by call. A head or Base branch
     /// commit stays the last one given; a merge is clean, the Base branch
-    /// unmoved, origin without new commits and a Base fix returns, unless
-    /// scripted otherwise.
+    /// unmoved, the PR mergeable, origin without new commits and a Base fix
+    /// returns, unless scripted otherwise.
     #[derive(Default)]
     struct Script {
         heads: VecDeque<&'static str>,
@@ -532,6 +563,9 @@ mod tests {
         tree_matches: VecDeque<Result<bool, &'static str>>,
         merge_completion_error: Option<&'static str>,
         base_moved: VecDeque<bool>,
+        /// For each read of the PR's mergeability, whether GitHub reports it
+        /// mergeable.
+        mergeable: VecDeque<bool>,
         new_commits: VecDeque<ForeignCommits>,
         watches: VecDeque<Is>,
         reruns: VecDeque<Option<Is>>,
@@ -709,6 +743,11 @@ mod tests {
             }
         }
 
+        fn pr_mergeable(&mut self) -> Result<bool> {
+            self.did.push(Did::ReadMergeable);
+            Ok(self.script.mergeable.pop_front().unwrap_or(true))
+        }
+
         fn ensure_pr_ready_and_mergeable(&mut self) -> Result<()> {
             self.did.push(Did::CheckPr);
             Ok(())
@@ -1045,6 +1084,8 @@ mod tests {
                 heads: script(["h1"]),
                 base_commits: script(["b1", "b2", "b3"]),
                 base_moved: script([true]),
+                // The PR conflicts with the moved Base branch.
+                mergeable: script([false]),
                 watches: script([Is::Failed(&["test"], &[]), Is::Failed(&["test"], &[])]),
                 reruns: script([Some(Is::Passed)]),
                 ..Script::default()
@@ -1075,6 +1116,8 @@ mod tests {
                 heads: script(["h1", "h1", "h2", "h1"]),
                 base_commits: script(["b1", "b2", "b3", "b4"]),
                 base_moved: script([true, false]),
+                // The PR conflicts with the moved Base branch.
+                mergeable: script([false]),
                 watches: script([
                     Is::Failed(&["test"], &[]),
                     Is::Failed(&[], &["lint"]),
@@ -1112,6 +1155,8 @@ mod tests {
                 heads: script([first, first, second, second]),
                 base_commits: script(["b1", "b2", "b3", "b4"]),
                 base_moved: script([true, false]),
+                // The PR conflicts with the moved Base branch.
+                mergeable: script([false]),
                 watches: script([Is::Failed(&["test"], &[]), Is::Failed(&["lint"], &[])]),
                 reruns: script([Some(Is::Passed), Some(Is::Passed)]),
                 ..Script::default()
@@ -1172,6 +1217,8 @@ mod tests {
                 heads: script(["h1"]),
                 base_commits: script(["b1", "b2", "rewound-base", "b3"]),
                 base_moved: script([true, false]),
+                // The PR conflicts with the moved Base branch.
+                mergeable: script([false]),
                 watches: script([
                     Is::Failed(&["test"], &[]),
                     Is::Failed(&[], &["lint"]),
@@ -1411,13 +1458,14 @@ mod tests {
     }
 
     #[test]
-    fn the_base_branch_moving_while_ci_ran_goes_round_again_as_a_base_move() {
+    fn in_a_merge_run_the_base_branch_moving_while_ci_ran_goes_round_again_as_a_base_move() {
         let (delivered, did) = deliver(
-            Goal::ReadyForReview,
+            Goal::Merged,
             Script {
                 heads: script(["h1", "h2"]),
                 base_moved: script([true]),
                 watches: script([Is::Passed, Is::Passed]),
+                merges: script([None]),
                 ..Script::default()
             },
         );
@@ -1429,6 +1477,72 @@ mod tests {
                 &round("h1")[..],
                 &[line("origin/main moved while CI ran; merging it again")],
                 &round("h2"),
+                &merge("h2"),
+            ]
+            .concat()
+        );
+    }
+
+    #[test]
+    fn a_run_left_ready_for_review_ends_on_green_ci_when_the_moved_base_branch_merges_cleanly() {
+        for is in [Is::Passed, Is::Absent] {
+            let (delivered, did) = deliver(
+                Goal::ReadyForReview,
+                Script {
+                    heads: script(["h1234567890", "h2"]),
+                    base_moved: script([true]),
+                    watches: script([is]),
+                    ..Script::default()
+                },
+            );
+
+            delivered.unwrap();
+            assert_eq!(
+                did,
+                [
+                    &[Did::MergeBase, Did::Push, watch("h1234567890")][..],
+                    &[
+                        Did::ReadMergeable,
+                        line(
+                            "origin/main moved while CI ran; leaving the PR for review on \
+                             h123456, which merges cleanly"
+                        ),
+                        Did::CheckPr,
+                    ],
+                ]
+                .concat()
+            );
+        }
+    }
+
+    #[test]
+    fn a_run_left_ready_for_review_goes_round_again_as_a_base_move_if_the_pr_does_not_merge() {
+        let (delivered, did) = deliver(
+            Goal::ReadyForReview,
+            Script {
+                heads: script(["h1", "h2"]),
+                base_moved: script([true]),
+                base_conflicts: script([false, true]),
+                mergeable: script([false]),
+                watches: script([Is::Passed, Is::Passed]),
+                ..Script::default()
+            },
+        );
+
+        delivered.unwrap();
+        assert_eq!(
+            did,
+            [
+                &round("h1")[..],
+                &[
+                    Did::ReadMergeable,
+                    line("origin/main moved while CI ran; merging it again"),
+                    Did::MergeBase,
+                    line("conflict; starting Repair 1 of 5"),
+                    repair(1, Started::Conflict(Upstream::BaseBranch)),
+                    Did::CheckMerged("origin/main"),
+                ],
+                &round("h2"),
                 &[Did::CheckPr],
             ]
             .concat()
@@ -2121,6 +2235,34 @@ mod tests {
         );
     }
 
+    #[test]
+    fn a_run_left_ready_for_review_reads_mergeability_in_readiness_and_counts_an_unmergeable_pr_in_a_round()
+     {
+        let (delivered, _, stages) = deliver_with_stages(
+            Goal::ReadyForReview,
+            Script {
+                heads: script(["h1"]),
+                base_moved: script([true; MAX_UPSTREAM_MOVES + 1]),
+                mergeable: script([false; MAX_UPSTREAM_MOVES + 1]),
+                watches: script((0..=MAX_UPSTREAM_MOVES).map(|_| Is::Passed)),
+                ..Script::default()
+            },
+        );
+
+        assert_eq!(
+            cause(delivered),
+            "origin/main kept moving: merged it again 5 times"
+        );
+        assert_eq!(
+            stages[stages.len() - 3..],
+            [
+                Stage::CiWatch,
+                Stage::PullRequestReadiness,
+                Stage::RepairLoopRound
+            ]
+        );
+    }
+
     #[test]
     fn a_failed_ci_watch_stops_in_the_ci_watch() {
         assert_eq!(

src/pull_request.rs

@@ -283,21 +283,34 @@ impl<A: Adapter> PullRequest<A> {
     /// coherent numbered observation. Waiting remains interruptible.
     pub fn ensure_ready_and_mergeable(&mut self) -> Result<()> {
         progress::step("checking the PR is open, ready and mergeable");
+        let snapshot = self.settled_ready_snapshot()?;
+        let url = &snapshot.pr.url;
+        match snapshot.mergeable {
+            Mergeable::Yes => Ok(()),
+            Mergeable::No => bail!("PR {url} is not mergeable"),
+            Mergeable::Unknown => {
+                bail!("GitHub has not worked out whether PR {url} is mergeable")
+            }
+        }
+    }
+
+    /// Whether GitHub reports this open, ready PR mergeable, as
+    /// [`Self::ensure_ready_and_mergeable`] polls it. Mergeability GitHub has
+    /// not worked out within the grace period is not mergeable.
+    pub fn is_mergeable(&mut self) -> Result<bool> {
+        Ok(self.settled_ready_snapshot()?.mergeable == Mergeable::Yes)
+    }
+
+    /// An observation of this open, ready PR, polled while its mergeability
+    /// is unknown until the grace period ends.
+    fn settled_ready_snapshot(&mut self) -> Result<PrSnapshot> {
         let deadline = std::time::Instant::now() + self.adapter.grace_period();
         let mut waiting_line = poll::WaitingLine::new();
         loop {
             let snapshot = self.observe(false)?.context("no PR found")?;
             self.validate(&snapshot, true)?;
             match snapshot.mergeable {
-                Mergeable::Yes => return Ok(()),
-                Mergeable::No => bail!("PR {} is not mergeable", snapshot.pr.url),
-                Mergeable::Unknown if std::time::Instant::now() >= deadline => {
-                    bail!(
-                        "GitHub has not worked out whether PR {} is mergeable",
-                        snapshot.pr.url
-                    );
-                }
-                Mergeable::Unknown => {
+                Mergeable::Unknown if std::time::Instant::now() < deadline => {
                     if waiting_line.should_report(()) {
                         progress::step(format!(
                             "waiting for GitHub to calculate mergeability of PR {}",
@@ -306,6 +319,7 @@ impl<A: Adapter> PullRequest<A> {
                     }
                     self.adapter.pause()?;
                 }
+                Mergeable::Yes | Mergeable::No | Mergeable::Unknown => return Ok(snapshot),
             }
         }
     }
@@ -1433,6 +1447,33 @@ mod tests {
         }
     }
 
+    #[test]
+    fn only_a_pr_github_reports_mergeable_is_mergeable_and_only_an_unready_one_fails() {
+        let mut conflicting = snapshot();
+        conflicting.mergeable = Mergeable::No;
+        let mut unknown = snapshot();
+        unknown.mergeable = Mergeable::Unknown;
+        let mut draft = snapshot();
+        draft.pr.is_draft = true;
+        for (answer, mergeable) in [
+            (snapshot(), Some(true)),
+            (conflicting, Some(false)),
+            (unknown, Some(false)),
+            (draft, None),
+        ] {
+            let (mut pr, _) = owner(World {
+                observations: VecDeque::from([
+                    Ok(Some(snapshot())),
+                    Ok(Some(snapshot())),
+                    Ok(Some(answer)),
+                ]),
+                ..World::default()
+            });
+            pr.mark_ready(None).unwrap();
+            assert_eq!(pr.is_mergeable().ok(), mergeable);
+        }
+    }
+
     #[test]
     fn disappearance_or_transport_failure_after_capture_never_rediscovers_by_branch() {
         for missing in [Ok(None), Err("gone")] {