From e8dad5a599b0b87704760e55f3e8b82dfb7cb958 Mon Sep 17 00:00:00 2001 From: Jacob Stephens Date: Sun, 27 Sep 2026 13:04:28 +0000 Subject: [PATCH 1/2] Send a failed Self-merge back round the Repair loop A Merge run whose merge fails now goes round again, merging the Base branch, repairing conflicts and red CI within the Run's budgets, and retries the merge on the new green head. A round that finds nothing to fix is a policy refusal: the Run fails but leaves the PR ready for review. The fake gh can refuse merges and run a script at merge time. Co-Authored-By: Claude Opus 5.5 --- README.md | 12 ++- src/failed_run.rs | 29 ++++-- src/run.rs | 195 ++++++++++++++++++++++++++-------------- tests/fakes/gh | 50 +++++++++-- tests/merge_run.rs | 218 +++++++++++++++++++++++++++++++++++++++++++++ tests/mergeable.rs | 2 +- 6 files changed, 420 insertions(+), 86 deletions(-) diff --git a/README.md b/README.md index 4f68b20c3..a2fd9162b 100644 --- a/README.md +++ b/README.md @@ -22,7 +22,7 @@ From a clone of the issue's repository, `thirdshift `: 4. Runs a headless Claude Code session in that worktree with the **Factory skills** loaded. The agent implements the issue, reviews its work against the Base branch, addresses the **Standards findings** and **Spec findings** it agrees with, and opens a ready-for-review pull request that lists every **Unaddressed finding** with a reason and closes the issue. 5. Takes over deterministically: pushes the Issue branch, skipping your repo's git hooks since the session runs the tests and CI gates the pull request, checks through `gh` that the pull request exists, is open and targets the Base branch, and marks it ready for review (`gh pr ready`) if the agent left it as a draft. 6. Keeps the pull request mergeable and green: merges the Base branch in and watches CI, starting a **Repair** session for a merge conflict or failing checks, at most 5 per Run. If the Base branch moves while CI runs, it merges it again and goes round, at most 5 times per Run. -7. In a **Merge run** (`thirdshift merge `), does the **Self-merge**: once the pull request is open, ready for review, mergeable and green, thirdshift merges it into the Base branch with a merge commit, on exactly the head commit whose CI it watched (`gh pr merge --merge --match-head-commit `). It never uses GitHub's auto-merge ([ADR-0004](docs/adr/0004-self-merge-by-thirdshift-not-github-auto-merge.md)). A merge that fails makes the Run a Failed run. +7. In a **Merge run** (`thirdshift merge `), does the **Self-merge**: once the pull request is open, ready for review, mergeable and green, thirdshift merges it into the Base branch with a merge commit, on exactly the head commit whose CI it watched (`gh pr merge --merge --match-head-commit `). It never uses GitHub's auto-merge ([ADR-0004](docs/adr/0004-self-merge-by-thirdshift-not-github-auto-merge.md)). A merge that fails goes back round step 6, within the same budgets, and is tried again on the new green head. If that round finds nothing to fix, the refusal is a **policy refusal**, such as merge commits being disallowed or a review being required. 8. Cleans up: removes the worktree, the local Issue branch and the temporary plugin directory, whether the Run succeeded or not. The one exception is a **Failed run** whose work could not be pushed: see below. ### Status @@ -106,7 +106,13 @@ thirdshift merge https://github.com/acme/widgets/issues/7 `thirdshift --merge ` does the same. The agent gets the same prompts either way; the only difference is the Self-merge at the end. The keyword goes before the URL: anything after the URL, `--merge` included, is an argument error. -- **stdout** carries only the pull request's URL: on success, and on a Failed run that leaves a draft pull request. The exit code tells the two apart, so script it as `url=$(thirdshift "$issue") && echo "ready: $url"`. +A Merge run ends in one of three ways: + +- **Merged**: exit `0`, the pull request's URL on stdout, and `PR is merged` as stderr's last line. +- **Policy refusal**: GitHub refused the merge and a round of merging the Base branch and watching CI found nothing left to fix. Exit `1`, the pull request's URL on stdout, and GitHub's error on stderr. The pull request stays open and ready for review, not a draft, for you to merge by hand or to change the repository's settings. thirdshift never reads GitHub's error text to decide this. +- **Any other failure**: a [Failed run](#failed-runs), as for a Run without `merge`. + +- **stdout** carries only the pull request's URL: on success, and on a Failed run that leaves an open pull request, a draft or, after a policy refusal, one ready for review. The exit code tells the two apart, so script it as `url=$(thirdshift "$issue") && echo "ready: $url"`. - **stderr** carries everything else: errors, cleanup problems, and progress lines while sessions run. A successful Run's last line names the pull request too: `PR is ready for review`, or `PR is merged` after a Merge run. - **Exit code** `0` means the Run ended with a pull request the factory stands behind, merged in a Merge run. `2` means the Issue URL is missing or isn't a GitHub Issue URL, or something follows it; the error and the help text go to stderr. Any other failure exits `1`. @@ -154,7 +160,7 @@ A **Failed run** is one that ends, including by Ctrl-C or a closed terminal, wit A Failed run: 1. Commits any uncommitted work as `thirdshift: failed run ()`, with a timestamp and the hostname, and pushes the Issue branch, so nothing is lost. If the branch has no changes against the Base branch, nothing is pushed. -2. Converts its open pull request, if any, back to a draft, so a pull request only claims to be ready when the factory stands behind it. The next successful Continuation marks it ready again. +2. Converts its open pull request, if any, back to a draft, so a pull request only claims to be ready when the factory stands behind it. The next successful Continuation marks it ready again. The exception is a Merge run's policy refusal: the pull request is ready, mergeable and green and only the Self-merge could not happen, so it stays ready for review. 3. Cleans up as usual, prints the reason to stderr and exits non-zero. If the push failed, the worktree and local Issue branch are kept instead, and stderr names the branch, its head commit and the worktree path, so you can recover the work or push it by hand. Merges, never rebases or force-pushes: a branch worked on from several servers never loses history. diff --git a/src/failed_run.rs b/src/failed_run.rs index 21119be2d..7a0c5c82c 100644 --- a/src/failed_run.rs +++ b/src/failed_run.rs @@ -1,5 +1,6 @@ //! The Failed run path: what happens when a Run can't end with a ready PR. +use std::fmt; use std::path::{Path, PathBuf}; use anyhow::{Result, anyhow}; @@ -20,6 +21,20 @@ pub struct FailedRun { pub log: Option, } +/// A merge GitHub refused when a round of the Repair loop found nothing to +/// fix. The Run fails, but, as it can do no more, leaves the PR ready for review +/// rather than a draft. It is the context of the merge error. +#[derive(Debug)] +pub struct PolicyRefusal; + +impl fmt::Display for PolicyRefusal { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + f.write_str( + "the merge was refused with nothing left to fix, so the PR stays ready for review", + ) + } +} + /// A failure before the worktree exists: nothing to push or clean up. impl From for FailedRun { fn from(error: anyhow::Error) -> Self { @@ -32,7 +47,8 @@ impl From for FailedRun { } /// Take the Run in `worktree` down the Failed run path: commit and push the -/// work, and send an open PR back to draft. Problems along the way are +/// work, and send an open PR back to draft unless `error` is a +/// `PolicyRefusal`. Problems along the way are /// reported, not raised, so `error` is what the Run fails with. The worktree /// is cleaned up, or kept if its work may not have reached origin. pub fn fail( @@ -61,7 +77,8 @@ pub fn fail( "could not push the failed run's work, so it may exist only locally: {problem:#}" )); } - let pr_url = match open_pr_as_draft(issue, worktree.branch()) { + let keep_ready = error.is::(); + let pr_url = match open_pr(issue, worktree.branch(), keep_ready) { Ok(pr_url) => pr_url, Err(problem) => { progress::step(format_args!( @@ -112,13 +129,13 @@ fn commit_and_push(worktree: &Worktree, base: &str, reason: &str) -> Result<()> worktree.push() } -/// Convert the open PR for `branch`, if there is one, to a draft, and return -/// its URL. -fn open_pr_as_draft(issue: &IssueUrl, branch: &str) -> Result> { +/// The URL of the open PR for `branch`, if there is one, converted to a draft +/// unless `keep_ready`. +fn open_pr(issue: &IssueUrl, branch: &str, keep_ready: bool) -> Result> { let Some(pr) = github::pull_request_for(issue, branch)?.filter(|pr| pr.is_open()) else { return Ok(None); }; - if !pr.is_draft { + if !pr.is_draft && !keep_ready { github::convert_to_draft(issue, branch)?; } Ok(Some(pr.url)) diff --git a/src/run.rs b/src/run.rs index 9578512a8..78f0d6bc6 100644 --- a/src/run.rs +++ b/src/run.rs @@ -6,7 +6,7 @@ use anyhow::{Context, Result, anyhow, bail}; use crate::branch::{self, Selection}; use crate::ci::{self, Ci}; -use crate::failed_run::{self, FailedRun}; +use crate::failed_run::{self, FailedRun, PolicyRefusal}; use crate::git::Git; use crate::github::{self, Mergeable, PullRequest}; use crate::interrupt; @@ -76,7 +76,9 @@ pub fn run(issue: &IssueUrl, goal: Goal) -> Result { /// The implement session given `prompt`, the checks on the PR it opened or /// updated, keeping that PR mergeable and, in a Merge run, the Self-merge. -/// `log` is left at the most recent session's log. +/// A merge that fails goes back round the Repair loop and is tried again on +/// the new head; if that round finds nothing to fix, the Run fails with a +/// `PolicyRefusal`. `log` is left at the most recent session's log. fn implement( issue: &IssueUrl, worktree: &Worktree, @@ -108,92 +110,149 @@ fn implement( github::mark_ready(issue, branch)?; } - let watched = repair_loop(issue, worktree, base, &pr.url, &mut run_session)?; - ensure_pr_ready_and_mergeable(issue, branch)?; - if interrupt::requested() { - bail!("interrupted"); - } - if goal == Goal::Merged { + let mut repair_loop = RepairLoop { + issue, + worktree, + base, + pr_url: &pr.url, + budgets: Budgets::default(), + }; + let mut watched = repair_loop.run(&mut run_session)?; + loop { + ensure_pr_ready_and_mergeable(issue, branch)?; + if interrupt::requested() { + bail!("interrupted"); + } + if goal == Goal::ReadyForReview { + return Ok(pr.url); + } progress::step(format_args!("merging the PR into {base}")); - github::merge(issue, branch, &watched)?; + let Err(error) = github::merge(issue, branch, &watched) else { + return Ok(pr.url); + }; + progress::step(format_args!("the merge failed: {error:#}")); + match repair_loop.round_after_failed_merge(&watched, &mut run_session)? { + Some(head) => watched = head, + None => { + // Only a PR still ready and mergeable is left ready. + ensure_pr_ready_and_mergeable(issue, branch)?; + return Err(error.context(PolicyRefusal)); + } + } } - Ok(pr.url) } /// The most Repair sessions a Run starts, conflict and CI-fix combined. const MAX_REPAIRS: usize = 5; /// The most times a Run goes round again because the Base branch moved while -/// CI ran, whether or not the merge that follows needs a Repair. A clean merge -/// uses no Repair, so without this a busy Base branch could keep a Run going -/// forever. +/// CI ran or since a merge was tried, whether or not the merge that follows +/// needs a Repair. A clean merge uses no Repair, so without this a busy Base +/// branch could keep a Run going forever. const MAX_BASE_MOVES: usize = 5; -/// Keep the PR mergeable and its CI green: merge the Base branch (never -/// rebase), push, and watch CI on the head commit, starting a Repair session -/// through `run_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. Returns the head -/// commit whose CI was last watched and found green or absent. Fails once a -/// Repair beyond `MAX_REPAIRS`, or a round beyond `MAX_BASE_MOVES`, would be -/// needed. -fn repair_loop( - issue: &IssueUrl, - worktree: &Worktree, - base: &str, - pr_url: &str, - run_session: &mut impl FnMut(&str, &str) -> Result<()>, -) -> Result { - let branch = worktree.branch(); - let mut repairs = 0; - let mut base_moves = 0; - // Counts the Repair about to start, as `repair-`, or fails if it would - // be one too many. - let mut next_repair = |cause: &str| -> Result { - if repairs == MAX_REPAIRS { +/// What a Run has spent of its Repair and base-move budgets, across every +/// round of the Repair loop, those after a failed merge included. +#[derive(Debug, Default, Clone, Copy, PartialEq, Eq)] +struct Budgets { + repairs: usize, + base_moves: usize, +} + +impl Budgets { + /// Counts the Repair about to start, as `repair-`, or fails if it would + /// be one too many. + fn next_repair(&mut self, cause: &str) -> Result { + if self.repairs == MAX_REPAIRS { bail!("repairs exhausted: {cause}"); } - repairs += 1; + self.repairs += 1; progress::step(format_args!( - "{cause}; starting Repair {repairs} of {MAX_REPAIRS}" + "{cause}; starting Repair {} of {MAX_REPAIRS}", + self.repairs )); - Ok(format!("repair-{repairs}")) - }; - loop { - if worktree.merge_base_branch(base)? == Merge::Conflicted { - let kind = next_repair("conflict")?; - run_session(&kind, &prompt::conflict_repair(issue, base, branch, pr_url))?; - worktree.ensure_base_branch_merged(base)?; - worktree.push()?; - continue; + Ok(format!("repair-{}", self.repairs)) + } + + /// Counts a round taken because `origin/` moved `when`, or fails if + /// it would be one too many. + fn count_base_move(&mut self, base: &str, when: &str) -> Result<()> { + if self.base_moves == MAX_BASE_MOVES { + bail!("origin/{base} kept moving: merged it again {MAX_BASE_MOVES} times"); } - worktree.push()?; - let head = worktree.head()?; - match ci::watch(issue, &head)? { - Ci::Absent | Ci::Passed => { - if !worktree.base_branch_moved(base)? { - return Ok(head); + self.base_moves += 1; + progress::step(format_args!("origin/{base} moved {when}; merging it again")); + Ok(()) + } +} + +/// Keeps the PR mergeable and its CI green, within the Run's budgets. +struct RepairLoop<'a> { + issue: &'a IssueUrl, + worktree: &'a Worktree, + base: &'a str, + pr_url: &'a str, + budgets: Budgets, +} + +impl RepairLoop<'_> { + /// Merge the Base branch (never rebase), push, and watch CI on the head + /// commit, starting a Repair session through `run_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. Returns the head commit whose CI was last + /// watched and found green or absent. Fails once a Repair beyond + /// `MAX_REPAIRS`, or a round beyond `MAX_BASE_MOVES`, would be needed. + fn run(&mut self, run_session: &mut impl FnMut(&str, &str) -> Result<()>) -> Result { + let (issue, worktree, base, pr_url) = (self.issue, self.worktree, self.base, self.pr_url); + let branch = worktree.branch(); + loop { + if worktree.merge_base_branch(base)? == Merge::Conflicted { + 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.push()?; + continue; + } + worktree.push()?; + let head = worktree.head()?; + match ci::watch(issue, &head)? { + Ci::Absent | Ci::Passed => { + if !worktree.base_branch_moved(base)? { + return Ok(head); + } + self.budgets.count_base_move(base, "while CI ran")?; } - if base_moves == MAX_BASE_MOVES { - bail!( - "origin/{base} kept moving while CI ran: merged it again {MAX_BASE_MOVES} times" - ); + Ci::Failed(failed) => { + let kind = self.budgets.next_repair("CI red")?; + run_session( + &kind, + &prompt::ci_fix_repair(issue, base, branch, pr_url, &failed), + )?; + worktree.push()?; } - base_moves += 1; - progress::step(format_args!( - "origin/{base} moved while CI ran; merging it again" - )); - } - Ci::Failed(failed) => { - let kind = next_repair("CI red")?; - run_session( - &kind, - &prompt::ci_fix_repair(issue, base, branch, pr_url, &failed), - )?; - worktree.push()?; } } } + + /// Go round again after a merge of `watched` failed, counting a Base + /// branch that moved since as a base move. Returns the new head whose CI + /// was found green or absent, or `None` if the round found nothing to fix: + /// the same head, no Repair and no base move. GitHub's error text is never + /// consulted. + fn round_after_failed_merge( + &mut self, + watched: &str, + run_session: &mut impl FnMut(&str, &str) -> Result<()>, + ) -> Result> { + let before = self.budgets; + if self.worktree.base_branch_moved(self.base)? { + self.budgets + .count_base_move(self.base, "since the merge was tried")?; + } + let head = self.run(run_session)?; + Ok((head != watched || self.budgets != before).then_some(head)) + } } /// The PR whose head is `branch`, failing unless it exists and is open. diff --git a/tests/fakes/gh b/tests/fakes/gh index 8d53cb1ec..acecd6a4e 100644 --- a/tests/fakes/gh +++ b/tests/fakes/gh @@ -9,7 +9,9 @@ State lives in the JSON file named by $FAKE_GH_STATE: "mergeable"?, "unknown_polls"?}], "checks": {"": [{"name", "conclusion", "url", "pending_polls"?}]}, "statuses": {"": [{"context", "state", "url"}]}, - "on_ci_read"?: {"times", "script", "seen"}} + "on_ci_read"?: {"times", "script", "seen"}, + "on_merge"?: {"times", "script"}, + "refuse_merges"?: {"times", "error"}} `gh pr merge` makes a real merge commit of the PR's head into its base in the bare origin repo beside $FAKE_GH_STATE (`origin.git`), whose HEAD names the @@ -28,7 +30,13 @@ command): gh fake pr '' set a field of 's newest PR gh fake on-ci-read '