From 33a44e18bc561de18d61eefc985051c828c4ad79 Mon Sep 17 00:00:00 2001 From: Jacob Stephens Date: Wed, 30 Sep 2026 12:01:53 +0000 Subject: [PATCH 1/2] Check a conflict Repair against the commit it merged, not the moved ref After a conflict Repair, thirdshift checked that origin/ was an ancestor of HEAD. origin/ is shared by every worktree, so a concurrent Run's fetch (or the Repair's own) could move it during a long Repair, failing a Run whose merge was complete and correct. The check now uses the commit the merge began with (MERGE_HEAD), and the Repair loop's next round merges whatever the base has moved on to. Co-Authored-By: Claude Opus 5.5 --- src/run.rs | 8 ++++---- src/worktree.rs | 44 ++++++++++++++++++++++++-------------------- tests/mergeable.rs | 31 +++++++++++++++++++++++++++++++ 3 files changed, 59 insertions(+), 24 deletions(-) diff --git a/src/run.rs b/src/run.rs index 3e634a5a6..46aff7e1e 100644 --- a/src/run.rs +++ b/src/run.rs @@ -368,10 +368,10 @@ impl RepairLoop<'_> { if self.goal == Goal::Merged { self.take_in_foreign_commits(run_session)?; } - if worktree.merge_base_branch(base)? == Merge::Conflicted { + if let Merge::Conflicted(pending) = worktree.merge_base_branch(base)? { let kind = self.budgets.next_repair("conflict")?; run_session(&kind, &prompt::conflict_repair(issue, base, branch, pr_url))?; - worktree.ensure_base_branch_merged(base)?; + worktree.ensure_merged(&pending)?; continue; } worktree.push()?; @@ -427,7 +427,7 @@ impl RepairLoop<'_> { for sha in &foreign { progress::step(format_args!("merging new commit {sha} from {upstream}")); } - if worktree.merge_new_commits()? == Merge::Conflicted { + if let Merge::Conflicted(pending) = worktree.merge_new_commits()? { let kind = self .budgets .next_repair(&format!("conflict with new commits on {upstream}"))?; @@ -435,7 +435,7 @@ impl RepairLoop<'_> { &kind, &prompt::conflict_repair(issue, branch, branch, pr_url), )?; - worktree.ensure_new_commits_merged()?; + worktree.ensure_merged(&pending)?; } let kind = self .budgets diff --git a/src/worktree.rs b/src/worktree.rs index af1c9d2f2..cc3082fb4 100644 --- a/src/worktree.rs +++ b/src/worktree.rs @@ -16,7 +16,16 @@ pub enum Merge { /// Merged, or nothing to merge. Clean, /// Conflicted; the merge is left in progress for a conflict Repair. - Conflicted, + Conflicted(PendingMerge), +} + +/// A conflicted merge left for a conflict Repair: `upstream` as it was when +/// the merge began, pinned to `commit` because another Run's fetch may move +/// the shared remote-tracking ref before the Repair finishes. +#[derive(Debug, PartialEq, Eq)] +pub struct PendingMerge { + upstream: String, + commit: String, } pub struct Worktree { @@ -182,28 +191,24 @@ impl Worktree { fn merge(&self, upstream: &str) -> Result { match self.git.run(&["merge", "--no-edit", "--ff", upstream]) { Ok(_) => Ok(Merge::Clean), - Err(_) if self.merge_in_progress()? => Ok(Merge::Conflicted), + Err(_) if self.merge_in_progress()? => Ok(Merge::Conflicted(PendingMerge { + upstream: upstream.to_string(), + commit: self.git.run(&["rev-parse", "MERGE_HEAD"])?, + })), Err(error) => Err(error), } } - /// Fail unless `origin/` is fully merged into the Issue branch, e.g. - /// after a conflict Repair that left the merge unfinished or aborted it. - pub fn ensure_base_branch_merged(&self, base: &str) -> Result<()> { - self.ensure_merged(&format!("origin/{base}")) - } - - /// Like [`Worktree::ensure_base_branch_merged`], for the Issue branch as - /// last fetched from origin. - pub fn ensure_new_commits_merged(&self) -> Result<()> { - self.ensure_merged(&self.upstream()) - } - - fn ensure_merged(&self, upstream: &str) -> Result<()> { + /// Fail unless the commit of a conflicted merge is merged into the Issue + /// branch, e.g. after a conflict Repair that left the merge unfinished or + /// aborted it. The upstream may have moved on since; merging that is the + /// next round's work. + pub fn ensure_merged(&self, pending: &PendingMerge) -> Result<()> { + let upstream = &pending.upstream; if self.merge_in_progress()? { bail!("the merge of {upstream} is still in progress"); } - if !self.merged(upstream)? { + if !self.merged(&pending.commit)? { bail!("{upstream} is not merged into {}", self.branch); } Ok(()) @@ -216,11 +221,10 @@ impl Worktree { Ok(!self.merged(&format!("origin/{base}"))?) } - /// Whether `upstream`, as last fetched, is fully merged into the Issue - /// branch. - fn merged(&self, upstream: &str) -> Result { + /// Whether `commit` is fully merged into the Issue branch. + fn merged(&self, commit: &str) -> Result { self.git - .succeeds(&["merge-base", "--is-ancestor", upstream, "HEAD"]) + .succeeds(&["merge-base", "--is-ancestor", commit, "HEAD"]) } /// Whether a merge is in progress in the worktree. diff --git a/tests/mergeable.rs b/tests/mergeable.rs index 16600e077..767d064ea 100644 --- a/tests/mergeable.rs +++ b/tests/mergeable.rs @@ -208,6 +208,37 @@ fn fails_when_the_conflict_repair_aborts_the_merge() { ); } +#[test] +fn merges_a_base_branch_that_moved_again_during_the_conflict_repair() { + let scenario = Scenario::new(); + scenario.agent_does(&format!( + "{AGENT_OPENS_PR}{}", + base_moves_on("feature.txt", "base feature") + )); + // Another Run's fetch moves origin/main, which every worktree shares, + // after the Repair has resolved the merge it was given. + scenario.agent_does_in_session( + 2, + &format!( + "{REPAIR_RESOLVES_CONFLICT}{}git fetch -q origin main\n", + base_moves_on("other.txt", "other") + ), + ); + + let result = scenario.run(&[&scenario.issue_url(7)]); + + assert_eq!(result.code, Some(0), "stderr: {}", result.stderr); + assert_eq!(scenario.claude_calls().len(), 2); + assert_eq!( + scenario.origin_git(&["rev-parse", "issue-7^2"]), + scenario.origin_git(&["rev-parse", "main"]) + ); + assert_eq!( + scenario.origin_git(&["show", "issue-7:feature.txt"]), + "feature\nbase feature\n" + ); +} + /// While thirdshift waits for CI on each of the next `times` new head commits, /// someone else pushes `file` with `content` to main. The script goes inside /// single quotes, so it must not contain one. From 69b77323b69a90ec58388a8750e5b9cef1950fae Mon Sep 17 00:00:00 2001 From: Jacob Stephens Date: Wed, 30 Sep 2026 12:06:13 +0000 Subject: [PATCH 2/2] Name merged()'s argument a rev and drop unused derives on Merge Co-Authored-By: Claude Opus 5.5 --- src/worktree.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/worktree.rs b/src/worktree.rs index cc3082fb4..447258a27 100644 --- a/src/worktree.rs +++ b/src/worktree.rs @@ -11,7 +11,7 @@ use crate::progress; /// How merging the Base branch, or new commits on origin, into the Issue /// branch went. -#[derive(Debug, PartialEq, Eq)] +#[derive(Debug)] pub enum Merge { /// Merged, or nothing to merge. Clean, @@ -22,7 +22,7 @@ pub enum Merge { /// A conflicted merge left for a conflict Repair: `upstream` as it was when /// the merge began, pinned to `commit` because another Run's fetch may move /// the shared remote-tracking ref before the Repair finishes. -#[derive(Debug, PartialEq, Eq)] +#[derive(Debug)] pub struct PendingMerge { upstream: String, commit: String, @@ -221,10 +221,10 @@ impl Worktree { Ok(!self.merged(&format!("origin/{base}"))?) } - /// Whether `commit` is fully merged into the Issue branch. - fn merged(&self, commit: &str) -> Result { + /// Whether `rev` is fully merged into the Issue branch. + fn merged(&self, rev: &str) -> Result { self.git - .succeeds(&["merge-base", "--is-ancestor", commit, "HEAD"]) + .succeeds(&["merge-base", "--is-ancestor", rev, "HEAD"]) } /// Whether a merge is in progress in the worktree.