From 6b1a39cbccb87697f11f093b41bff62eacd3bc98 Mon Sep 17 00:00:00 2001 From: Jacob Stephens Date: Wed, 30 Sep 2026 12:43:44 +0000 Subject: [PATCH 1/2] Take in Foreign commits that land during a Repair before pushing take_in_foreign_commits merged one batch of Foreign commits, then ran a conflict Repair and a review Repair. Commits pushed to the Issue branch during either Repair were never fetched, so thirdshift's push was rejected and the Run failed. It now goes round again until origin has nothing new, each round counted against the upstream-move and Repair budgets. Co-Authored-By: Claude Opus 5.5 --- src/run.rs | 52 +++++++++++++++++++++++-------------------- tests/merge_run.rs | 55 ++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 83 insertions(+), 24 deletions(-) diff --git a/src/run.rs b/src/run.rs index 702b540c5..06ba42a2c 100644 --- a/src/run.rs +++ b/src/run.rs @@ -438,7 +438,9 @@ impl RepairLoop<'_> { /// Fetch the Issue branch from origin and merge in any Foreign commits on /// it (never rebase), counting that as a round and handing a conflict to /// a conflict Repair. Then a review Repair reviews them from the head the - /// Run last knew as its own, the local head before the merge. + /// Run last knew as its own, the local head before the merge. Goes round + /// again until origin has nothing new, since more may land during either + /// Repair and would otherwise make the push fail. fn take_in_foreign_commits( &mut self, run_session: &mut impl FnMut(&str, &str) -> Result<()>, @@ -446,35 +448,37 @@ impl RepairLoop<'_> { let (issue, worktree, pr_url) = (self.issue, self.worktree, self.pr_url); let branch = worktree.branch(); let upstream = worktree.upstream(); - let foreign = worktree.new_commits_on_origin()?; - if foreign.is_empty() { - return Ok(()); - } - let own_head = worktree.head()?; - self.budgets.count_upstream_move( - &upstream, - format_args!("{upstream} has new commits; merging them in"), - )?; - for sha in &foreign { - progress::step(format_args!("merging new commit {sha} from {upstream}")); - } - if let Merge::Conflicted(pending) = worktree.merge_new_commits()? { + loop { + let foreign = worktree.new_commits_on_origin()?; + if foreign.is_empty() { + return Ok(()); + } + let own_head = worktree.head()?; + self.budgets.count_upstream_move( + &upstream, + format_args!("{upstream} has new commits; merging them in"), + )?; + for sha in &foreign { + progress::step(format_args!("merging new commit {sha} from {upstream}")); + } + if let Merge::Conflicted(pending) = worktree.merge_new_commits()? { + let kind = self + .budgets + .next_repair(&format!("conflict with new commits on {upstream}"))?; + run_session( + &kind, + &prompt::conflict_repair(issue, branch, branch, pr_url), + )?; + worktree.ensure_merged(&pending)?; + } let kind = self .budgets - .next_repair(&format!("conflict with new commits on {upstream}"))?; + .next_repair(&format!("new commits on {upstream} to review"))?; run_session( &kind, - &prompt::conflict_repair(issue, branch, branch, pr_url), + &prompt::review_repair(issue, branch, pr_url, &own_head), )?; - worktree.ensure_merged(&pending)?; } - let kind = self - .budgets - .next_repair(&format!("new commits on {upstream} to review"))?; - run_session( - &kind, - &prompt::review_repair(issue, branch, pr_url, &own_head), - ) } /// Go round again after a merge of `watched` failed, counting a Base diff --git a/tests/merge_run.rs b/tests/merge_run.rs index 6817f3da4..e2eda02ca 100644 --- a/tests/merge_run.rs +++ b/tests/merge_run.rs @@ -558,6 +558,61 @@ fn a_foreign_commit_that_conflicts_with_local_work_gets_a_conflict_repair_then_a ); } +#[test] +fn foreign_commits_pushed_during_the_conflict_repair_are_merged_in_and_reviewed_too() { + let scenario = Scenario::new(); + scenario.agent_does_in_session(1, &format!("{AGENT_OPENS_PR}{}", checks_on_head(RED))); + scenario.agent_does_in_session( + 2, + &format!( + "{}echo fixed > feature.txt\ngit commit -q -am \"Fix CI 1\"\n", + someone_pushes_to_issue_7("feature.txt", "theirs", "Their feature") + ), + ); + // While the conflict Repair works, someone pushes to issue-7 again. + scenario.agent_does_in_session( + 3, + &format!( + "echo both > feature.txt\ngit add feature.txt\ngit commit -q --no-edit\n{}", + someone_pushes_to_issue_7("late.txt", "late", "Late commit") + ), + ); + + let result = scenario.run(&["merge", &scenario.issue_url(7)]); + + assert_eq!(result.code, Some(0), "stderr: {}", result.stderr); + let late = sha_of(&scenario, "main", "Late commit"); + assert!( + result.stderr.contains(&format!( + "thirdshift: merging new commit {late} from origin/issue-7" + )), + "stderr: {}", + result.stderr + ); + let calls = scenario.claude_calls(); + assert_eq!( + calls.len(), + 5, + "implement, CI fix, conflict, and a review for each round of Foreign commits" + ); + let fix = sha_of(&scenario, "main", "Fix CI 1"); + assert_review_repair_from(&calls[3]["prompt"], &fix); + // The second round reviews from the conflict Repair's merge commit. + let parents = format!("{fix} {}", sha_of(&scenario, "main", "Their feature")); + let resolution = scenario + .origin_git(&["log", "--format=%H %P", "main"]) + .lines() + .find_map(|line| line.strip_suffix(&format!(" {parents}")).map(String::from)) + .expect("no merge of Their feature into Fix CI 1"); + assert_review_repair_from(&calls[4]["prompt"], &resolution); + assert_issue_7_merged_into(&scenario, "main"); + assert_eq!(scenario.origin_file("main", "late.txt").unwrap(), "late\n"); + assert_eq!( + scenario.origin_file("main", "feature.txt").unwrap(), + "both\n" + ); +} + #[test] fn a_review_repair_whose_background_work_was_killed_gets_a_resume() { let scenario = Scenario::new(); From 41469b245989cc9b2328b3dcc15df042999a5c79 Mon Sep 17 00:00:00 2001 From: Jacob Stephens Date: Wed, 30 Sep 2026 12:46:07 +0000 Subject: [PATCH 2/2] Say what the Foreign-commit loop does and does not guard against Co-Authored-By: Claude Opus 5.5 --- src/run.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/run.rs b/src/run.rs index 06ba42a2c..4fa79ef03 100644 --- a/src/run.rs +++ b/src/run.rs @@ -440,7 +440,7 @@ impl RepairLoop<'_> { /// a conflict Repair. Then a review Repair reviews them from the head the /// Run last knew as its own, the local head before the merge. Goes round /// again until origin has nothing new, since more may land during either - /// Repair and would otherwise make the push fail. + /// Repair, and the push after them would be rejected. fn take_in_foreign_commits( &mut self, run_session: &mut impl FnMut(&str, &str) -> Result<()>,