diff --git a/GLOSSARY.md b/GLOSSARY.md index 37a1c3c1..96dc7a3e 100644 --- a/GLOSSARY.md +++ b/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. diff --git a/site/how-it-works/index.html b/site/how-it-works/index.html index a384f2d5..2a199856 100644 --- a/site/how-it-works/index.html +++ b/site/how-it-works/index.html @@ -126,7 +126,7 @@

How thirdshift works

Conflict or the branch's own CI failure?Start a Repair to resolve or fix it.Then return to merging the Base branch ↑
-
Base branch moved during CI?Reconcile the new upstream state.Return to merging the Base branch ↑
+
Base branch moved during CI?A Self-merge reconciles the new upstream state. A pull request left for review stops on green CI if it still merges cleanly.Otherwise return to merging the Base branch ↑

Repair and upstream-move budgets keep the loop bounded. If a CI-fix Repair leaves the same failing head, that head gets one Check re-run; if its own checks stay red, the Run fails rather than spending more Repairs on unchanged code.

diff --git a/src/delivery.rs b/src/delivery.rs index 2cbbe4f2..b4dda75b 100644 --- a/src/delivery.rs +++ b/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 { + self.pull_request.is_mergeable() + } + fn ensure_pr_ready_and_mergeable(&mut self) -> Result<()> { self.pull_request.ensure_ready_and_mergeable() } diff --git a/src/delivery/repair_loop.rs b/src/delivery/repair_loop.rs index dc4e4cf4..11adc0f3 100644 --- a/src/delivery/repair_loop.rs +++ b/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; /// 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 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 { + 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 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 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), + /// 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>, merge_completion_error: Option<&'static str>, base_moved: VecDeque, + /// For each read of the PR's mergeability, whether GitHub reports it + /// mergeable. + mergeable: VecDeque, new_commits: VecDeque, watches: VecDeque, reruns: VecDeque>, @@ -709,6 +743,11 @@ mod tests { } } + fn pr_mergeable(&mut self) -> Result { + 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!( diff --git a/src/pull_request.rs b/src/pull_request.rs index e860b39b..752282da 100644 --- a/src/pull_request.rs +++ b/src/pull_request.rs @@ -283,21 +283,34 @@ impl PullRequest { /// 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 { + 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 { 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 PullRequest { } 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")] { diff --git a/tests/mergeable.rs b/tests/mergeable.rs index f47ddaf3..24d6812b 100644 --- a/tests/mergeable.rs +++ b/tests/mergeable.rs @@ -1,7 +1,8 @@ //! Keeping the PR mergeable: once the PR is confirmed, thirdshift merges the //! Base branch into the Issue branch and pushes, handing a conflicting merge to -//! a conflict Repair session first. If the Base branch moves while CI runs, it -//! merges it again, at most five times. +//! a conflict Repair session first. If the Base branch moves while CI runs, a +//! Merge run merges it again, at most five times; a PR left ready for review +//! that still merges cleanly is left on the head whose CI passed. mod support; @@ -253,25 +254,64 @@ fn base_moves_during_ci(times: usize, file: &str, content: &str) -> String { ) } -#[test] -fn merges_a_base_branch_that_moved_while_ci_passed() { - let scenario = Scenario::new(); - // Green CI on the agent's commit; the merge commit after it has none. - scenario.agent_does(&format!( +/// The agent opens the PR with green CI on its commit, and main moves while +/// that CI is read. +fn green_ci_while_the_base_branch_moves() -> String { + format!( "{AGENT_OPENS_PR}\ gh fake checks \"$(git rev-parse HEAD)\" '[{{\"name\": \"test\", \"conclusion\": \"success\"}}]'\n\ {}", base_moves_during_ci(1, "other.txt", "other") - )); + ) +} + +#[test] +fn a_merge_run_merges_a_base_branch_that_moved_while_ci_passed() { + let scenario = Scenario::new(); + // Green CI on the agent's commit; the merge commit after it has none. + scenario.agent_does(&green_ci_while_the_base_branch_moves()); + + let result = scenario.run(&[&scenario.issue_url(7), "merge"]); + + assert_eq!(result.code, Some(0), "stderr: {}", result.stderr); + assert!( + result.stderr.contains("CI passed on") + && result + .stderr + .contains("origin/main moved while CI ran; merging it again"), + "stderr: {}", + result.stderr + ); + assert_eq!(scenario.claude_calls().len(), 1); + // The Issue branch is gone once merged; main has the merge of main in. + assert!( + scenario + .origin_git(&["log", "--format=%s", "main"]) + .contains("Merge remote-tracking branch 'origin/main' into issue-7\n") + ); +} + +#[test] +fn a_pr_left_for_review_stays_on_green_ci_when_the_moved_base_branch_merges_cleanly() { + let scenario = Scenario::new(); + scenario.agent_does(&green_ci_while_the_base_branch_moves()); let result = scenario.run(&[&scenario.issue_url(7)]); assert_eq!(result.code, Some(0), "stderr: {}", result.stderr); + let head = scenario.origin_git(&["rev-parse", "--short=7", "issue-7"]); assert!( - result.stderr.contains("CI passed on"), + result.stderr.contains(&format!( + "origin/main moved while CI ran; leaving the PR for review on {}, which merges cleanly", + head.trim() + )), "stderr: {}", result.stderr ); + assert!(!result.stderr.contains("merging it again")); assert_eq!(scenario.claude_calls().len(), 1); - assert_merged_main_into_issue_7(&scenario); + assert_eq!( + scenario.origin_git(&["log", "--first-parent", "--format=%s", "issue-7"]), + "Add feature\nInitial commit\n" + ); }