From 3064725004c1016b5931d1892921f2e412527fcd Mon Sep 17 00:00:00 2001 From: Jacob Stephens Date: Mon, 19 Oct 2026 13:32:21 +0000 Subject: [PATCH 1/5] Report externally completed Delivery merges accurately (#806) --- CONTEXT.md | 4 +- src/delivery.rs | 17 ++++++- src/pull_request.rs | 27 ++++++++++++ src/worktree.rs | 18 ++++++++ tests/merge_run.rs | 105 ++++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 168 insertions(+), 3 deletions(-) diff --git a/CONTEXT.md b/CONTEXT.md index 7f9dc9ea6..079c0bb8a 100644 --- a/CONTEXT.md +++ b/CONTEXT.md @@ -192,7 +192,7 @@ A **Merge run** into a **Run**'s **Base branch** that thirdshift starts when tha _Avoid_: hotfix, base repair (a **Repair** works on the Issue branch) **Failed run**: -A **Run** that ends, including by interruption, without an open pull request from its **Issue branch** that targets the **Base branch**, is mergeable, and has passing CI. For a **Merge run**, it is also a Run that ends without its pull request merged. Even after Command interruption, any unfinished merge is aborted and its work is still pushed so nothing is lost (or, if the push fails, its worktree and local **Issue branch** are kept), and its open pull request, if any, is converted back to a draft through the same finishing path, unless the pull request is ready, mergeable and green and only the **Self-merge** could not happen. +A **Run** that ends, including by interruption, without an open pull request from its **Issue branch** that targets the **Base branch**, is mergeable, and has passing CI, or a confirmed completed merge as described under **Delivery**. For a **Merge run**, it is also a Run that ends without its pull request merged. Even after Command interruption, any unfinished merge is aborted and its work is still pushed so nothing is lost (or, if the push fails, its worktree and local **Issue branch** are kept), and its open pull request, if any, is converted back to a draft through the same finishing path, unless the pull request is ready, mergeable and green and only the **Self-merge** could not happen. **Issue branch**: A branch a **Run** works on for one issue: `issue-` (the first), then `issue--branch-` for k ≥ 2. A number counts as used once its pull request is merged or closed, even if the branch itself was deleted. @@ -206,6 +206,8 @@ A follow-up agent session a **Run** starts after the pull request exists, or a * **Delivery**: What a **Run**, or a **Spec run** for its **Spec PR**, does from its worktree once its work begins. It runs the opening session (the implement session, or the **Spec review**), pushes, marks the pull request ready, keeps it mergeable and green through the **Repair loop**, and, in a **Merge run**, does the **Self-merge**. For a Spec PR, Delivery ends Ticket accounting's capture before the Spec review, allowing that session to replace the PR. The first successful observation after the opening session, or during Failed run completion after an opening failure, captures one pull request number and URL in the issue's repository; all later observations and transitions use that identity. Readiness and every mergeability poll validate its expected **Issue branch** and **Base branch**, open state and readiness where required. A missing, closed or retargeted captured PR fails Delivery; a replacement from the same branch is never substituted. A Delivery that fails takes the **Failed run** path, drafting that same captured open PR after preserving work. Spec PR checklist restoration uses that Delivery capture before readiness and for final accounting after failure. +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. diff --git a/src/delivery.rs b/src/delivery.rs index ebf179986..f7d005b98 100644 --- a/src/delivery.rs +++ b/src/delivery.rs @@ -18,7 +18,7 @@ use anyhow::Result; mod repair_loop; -use crate::pull_request::{Identified, MergeAttempt, PullRequest}; +use crate::pull_request::{AlreadyMerged, Identified, MergeAttempt, PullRequest}; use crate::base_fix::BaseFix; use crate::ci::{self, Ci, FailedChecks}; @@ -82,7 +82,9 @@ impl Delivery<'_> { /// is tried again on the new head; if that round finds nothing to fix, /// this fails with a `PolicyRefusal`. Any failure goes through the Failed /// run path, which keeps the worktree if its work did not reach origin; - /// otherwise it is cleaned up when this returns. + /// otherwise it is cleaned up when this returns. A gate observing an + /// already merged PR completes as merged only if that head includes all + /// local work; no further publication or PR mutations are performed. pub fn deliver( self, worktree: Worktree, @@ -125,6 +127,17 @@ impl Delivery<'_> { ticket_lines: Vec::new(), }), Err(error) => { + if let Some(merged) = error.downcast_ref::() + && worktree.fully_landed_at(&merged.head).unwrap_or(false) + { + progress::step("the pull request was merged while Delivery was running"); + return Ok(Reached { + pr_url: merged.pr.url.clone(), + goal: Goal::Merged, + log, + ticket_lines: Vec::new(), + }); + } if error.is::() { stage.set(Stage::SecurityReview); } diff --git a/src/pull_request.rs b/src/pull_request.rs index 890ead8dd..6136cff28 100644 --- a/src/pull_request.rs +++ b/src/pull_request.rs @@ -17,6 +17,22 @@ pub(crate) struct Identified { pub url: String, } +/// A terminal observation stops all Delivery mutations. Delivery may report +/// this as completion only after checking that no local work is left out. +#[derive(Debug)] +pub(crate) struct AlreadyMerged { + pub pr: Identified, + pub head: String, +} + +impl std::fmt::Display for AlreadyMerged { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!(f, "PR {} is merged, not open", self.pr.url) + } +} + +impl std::error::Error for AlreadyMerged {} + /// A gate error is terminal (`Err`); only a refused request can return to /// the Repair loop. A successful guarded command needs no additional read. pub(crate) enum MergeAttempt { @@ -410,6 +426,17 @@ impl PullRequest { fn validate(&self, snapshot: &PrSnapshot, ready: bool) -> Result<()> { let pr = &snapshot.pr; + if pr.state == PrState::Merged && pr.base == self.base { + return Err(AlreadyMerged { + pr: self + .identified + .as_ref() + .expect("observation captured identity") + .clone(), + head: snapshot.head_commit.clone(), + } + .into()); + } if !pr.is_open() { bail!("PR {} is {}, not open", pr.url, pr.state); } diff --git a/src/worktree.rs b/src/worktree.rs index 219aa959a..c596b6cd0 100644 --- a/src/worktree.rs +++ b/src/worktree.rs @@ -235,6 +235,24 @@ impl Worktree { .ordinary(&self.launch, |operation| operation.head()) } + /// Whether a merged PR includes all local work. Observe the acquired + /// checkout under the synchronization lock, without staging or committing. + pub fn fully_landed_at(&self, head: &str) -> Result { + self.checkout.ordinary(&self.launch, |operation| { + Ok(operation.head()? == head + && operation + .run(&[ + "-c", + "core.fsmonitor=false", + "status", + "--porcelain", + "--untracked-files=all", + "--ignore-submodules=none", + ])? + .is_empty()) + }) + } + /// Whether the committed tree at the Issue branch's head equals `commit`'s. /// Comparison errors are propagated, never treated as identical trees. pub fn same_tree_as(&self, commit: &str) -> Result { diff --git a/tests/merge_run.rs b/tests/merge_run.rs index 374bb27b1..780ff9a00 100644 --- a/tests/merge_run.rs +++ b/tests/merge_run.rs @@ -19,6 +19,111 @@ gh pr create --base main --head issue-7 --title "Add feature" --body "Closes #7" const PR_URL: &str = "https://github.com/acme/widgets/pull/1"; +#[test] +fn a_pr_merged_while_the_opening_session_runs_is_reported_as_merged() { + for merge in [false, true] { + let scenario = Scenario::new(); + // Simulate a human merging the published work while the session is + // still running, then GitHub deleting the Issue branch. + scenario.agent_does(&format!( + "{AGENT_OPENS_PR}\ + git push -q origin HEAD\n\ + gh pr merge 1 --merge --match-head-commit \"$(git rev-parse HEAD)\"\n\ + git push -q origin --delete issue-7\n" + )); + let issue = scenario.issue_url(7); + let args = if merge { + vec!["merge", &issue] + } else { + vec![&*issue] + }; + + let result = scenario.run(&args); + + assert_eq!(result.code, Some(0), "{}", result.stderr); + assert_eq!(result.stdout, format!("{PR_URL}\n")); + assert_eq!( + result.stderr.lines().last(), + Some(format!("thirdshift: PR {PR_URL} is merged").as_str()) + ); + assert_eq!(scenario.gh_calls_of("pr", "merge").len(), 1); + assert!(scenario.gh_calls_of("pr", "edit").is_empty()); + assert!(scenario.gh_calls_of("pr", "ready").is_empty()); + assert_eq!(scenario.origin_log("issue-7"), None); + scenario.assert_cleaned_up("issue-7"); + } +} + +#[test] +fn a_pr_merged_during_ci_is_reported_as_merged_without_recreating_its_branch() { + let scenario = Scenario::new(); + scenario.agent_does(&format!( + "{AGENT_OPENS_PR}{}", + on_ci_read( + 1, + r#"gh pr merge 1 --merge --match-head-commit "$(git ls-remote origin refs/heads/issue-7 | cut -f1)" +git push -q origin --delete issue-7"# + ) + )); + + let result = scenario.run(&["merge", &scenario.issue_url(7)]); + + assert_eq!(result.code, Some(0), "{}", result.stderr); + assert_eq!(result.stdout, format!("{PR_URL}\n")); + assert_eq!( + result.stderr.lines().last(), + Some(format!("thirdshift: PR {PR_URL} is merged").as_str()) + ); + assert_eq!(scenario.gh_calls_of("pr", "merge").len(), 1); + assert_eq!(scenario.origin_log("issue-7"), None); + scenario.assert_cleaned_up("issue-7"); +} + +#[test] +fn a_merged_pr_with_local_work_left_out_still_fails_and_retains_that_work() { + for leftover in [ + "echo leftover >> feature.txt", + "echo leftover > untracked.txt", + "echo leftover >> feature.txt; git add feature.txt", + "echo leftover >> feature.txt; git add feature.txt; git commit -qm Leftover", + ] { + let scenario = Scenario::new(); + scenario.agent_does(&format!( + "{AGENT_OPENS_PR}\ + git push -q origin HEAD\n\ + gh pr merge 1 --merge --match-head-commit \"$(git rev-parse HEAD)\"\n\ + git push -q origin --delete issue-7\n\ + {leftover}\n" + )); + + let result = scenario.run(&["merge", &scenario.issue_url(7)]); + + assert_eq!(result.code, Some(1), "{}", result.stderr); + assert!( + result.stderr.contains("is merged, not open"), + "{}", + result.stderr + ); + assert!( + result.stderr.contains("keeping the worktree"), + "{}", + result.stderr + ); + let worktree = scenario.launch_dir().with_file_name("widgets-issue-7"); + let file = if leftover.contains("untracked.txt") { + "untracked.txt" + } else { + "feature.txt" + }; + assert!( + std::fs::read_to_string(worktree.join(file)) + .unwrap() + .contains("leftover") + ); + assert_eq!(scenario.origin_log("issue-7"), None); + } +} + /// Launch from `develop`, a branch other than the default, and have the agent /// open its PR into it, then run `then`. fn merge_into_develop(scenario: &Scenario, then: &str) { From 1c651106847d8fae55dbcfbeb82423339b1f218a Mon Sep 17 00:00:00 2001 From: Jacob Stephens Date: Mon, 19 Oct 2026 13:35:25 +0000 Subject: [PATCH 2/5] Retain hidden tracked work when reconciling a merged PR --- src/worktree.rs | 14 +++++++++++++- src/worktree/acquisition.rs | 13 +++---------- tests/merge_run.rs | 26 ++++++++++++++++++++++++++ 3 files changed, 42 insertions(+), 11 deletions(-) diff --git a/src/worktree.rs b/src/worktree.rs index c596b6cd0..0aa85a1b8 100644 --- a/src/worktree.rs +++ b/src/worktree.rs @@ -239,7 +239,9 @@ impl Worktree { /// checkout under the synchronization lock, without staging or committing. pub fn fully_landed_at(&self, head: &str) -> Result { self.checkout.ordinary(&self.launch, |operation| { - Ok(operation.head()? == head + let entries = operation.run(&["-c", "core.fsmonitor=false", "ls-files", "-v", "-z"])?; + Ok(!index_hides_tracked_work(&entries) + && operation.head()? == head && operation .run(&[ "-c", @@ -369,6 +371,16 @@ impl Worktree { } } +/// Lowercase tags mark assume-unchanged entries; S marks skip-worktree. +/// Neither permits a clean status to establish absence of tracked work. +fn index_hides_tracked_work(entries: &str) -> bool { + entries.split('\0').any(|file| { + file.as_bytes() + .first() + .is_some_and(|flag| flag.is_ascii_lowercase() || *flag == b'S') + }) +} + /// The local Issue branch head sampled for acquisition or advisory preflight. pub(crate) fn local_head(launch: &Git, branch: &str) -> Result> { let reference = format!("refs/heads/{branch}"); diff --git a/src/worktree/acquisition.rs b/src/worktree/acquisition.rs index dcc31b9d3..8e084ff2f 100644 --- a/src/worktree/acquisition.rs +++ b/src/worktree/acquisition.rs @@ -5,8 +5,8 @@ use std::path::{Path, PathBuf}; use anyhow::{Context, Result, bail}; -use super::local_head; use super::ownership::{Checkout, Directory, remove_local_ref}; +use super::{index_hides_tracked_work, local_head}; use crate::git::Git; pub(super) fn add( @@ -297,15 +297,8 @@ impl Recovery<'_, '_> { } let git = Git::new(owner.path).completion(); // Hidden index entries cannot establish absence of tracked work. - if git - .run(&["-c", "core.fsmonitor=false", "ls-files", "-v", "-z"])? - .split('\0') - .any(|file| { - file.as_bytes() - .first() - .is_some_and(|flag| flag.is_ascii_lowercase() || *flag == b'S') - }) - { + let entries = git.run(&["-c", "core.fsmonitor=false", "ls-files", "-v", "-z"])?; + if index_hides_tracked_work(&entries) { bail!("checkout has uncertain index flags that can hide tracked work"); } if !git diff --git a/tests/merge_run.rs b/tests/merge_run.rs index 780ff9a00..c1e1e57e0 100644 --- a/tests/merge_run.rs +++ b/tests/merge_run.rs @@ -124,6 +124,32 @@ fn a_merged_pr_with_local_work_left_out_still_fails_and_retains_that_work() { } } +#[test] +fn externally_merged_pr_retains_tracked_changes_hidden_by_index_flags() { + for flag in ["assume-unchanged", "skip-worktree"] { + let scenario = Scenario::new(); + scenario.agent_does(&format!( + "{AGENT_OPENS_PR}\ + git push -q origin HEAD\n\ + gh pr merge 1 --merge --match-head-commit \"$(git rev-parse HEAD)\"\n\ + git push -q origin --delete issue-7\n\ + git update-index --{flag} feature.txt\n\ + echo leftover >> feature.txt\n" + )); + + let result = scenario.run(&["merge", &scenario.issue_url(7)]); + + assert_eq!(result.code, Some(1), "{flag}: {}", result.stderr); + let worktree = scenario.launch_dir().with_file_name("widgets-issue-7"); + assert!( + std::fs::read_to_string(worktree.join("feature.txt")) + .unwrap() + .contains("leftover") + ); + assert_eq!(scenario.origin_log("issue-7"), None); + } +} + /// Launch from `develop`, a branch other than the default, and have the agent /// open its PR into it, then run `then`. fn merge_into_develop(scenario: &Scenario, then: &str) { From 2481bc6ae4c15d2a1374806ff6214442aa092eaa Mon Sep 17 00:00:00 2001 From: Jacob Stephens Date: Mon, 19 Oct 2026 13:48:27 +0000 Subject: [PATCH 3/5] Update lifecycle regression for completed external merges --- tests/pr_lifecycle.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/tests/pr_lifecycle.rs b/tests/pr_lifecycle.rs index 14201380e..1af3eb608 100644 --- a/tests/pr_lifecycle.rs +++ b/tests/pr_lifecycle.rs @@ -12,10 +12,10 @@ gh pr create --base main --head issue-7 --title Feature --body 'Closes #7' #[test] fn lifecycle_changes_stop_pending_ci_without_recreating_a_deleted_branch() { - for (field, value, reason) in [ - ("state", "\"MERGED\"", "is merged, not open"), - ("state", "\"CLOSED\"", "is closed, not open"), - ("isDraft", "true", "is a draft"), + for (field, value, code, reason) in [ + ("state", "\"MERGED\"", 0, "is merged"), + ("state", "\"CLOSED\"", 1, "is closed, not open"), + ("isDraft", "true", 1, "is a draft"), ] { let scenario = Scenario::new(); let delete = if field == "state" { @@ -27,7 +27,7 @@ fn lifecycle_changes_stop_pending_ci_without_recreating_a_deleted_branch() { "{OPEN}\ngh fake checks \"$(git rev-parse HEAD)\" '[{{\"name\":\"test\",\"conclusion\":\"success\",\"pending_polls\":100}}]'\ngh fake on-ci-read 1 'gh fake pr issue-7 {field} '\"'\"'{value}'\"'\"'{delete}'\n" )); let result = scenario.run(&["merge", &scenario.issue_url(7)]); - assert_eq!(result.code, Some(1), "{}", result.stderr); + assert_eq!(result.code, Some(code), "{}", result.stderr); assert!(result.stderr.contains(reason), "{}", result.stderr); assert!(!result.stderr.contains("CI passed"), "{}", result.stderr); if field == "state" { From a2c930c017814be8d98130afcf1cc5a9a7a7df35 Mon Sep 17 00:00:00 2001 From: Jacob Stephens Date: Mon, 19 Oct 2026 13:55:19 +0000 Subject: [PATCH 4/5] Expect completed merges in the push comparison regression --- tests/spec_push_race.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/spec_push_race.rs b/tests/spec_push_race.rs index 1f2bab521..6241c4ae7 100644 --- a/tests/spec_push_race.rs +++ b/tests/spec_push_race.rs @@ -30,9 +30,9 @@ touch "$FAKE_CLAUDE_SCRIPT.armed" "#, ); let result = scenario.run(&["merge", &scenario.issue_url(7)]); - assert_eq!(result.code, Some(1), "{}", result.stderr); + assert_eq!(result.code, Some(0), "{}", result.stderr); assert!( - result.stderr.contains("is merged, not open"), + result.stderr.contains("is merged\n"), "{}", result.stderr ); From 40ca33efaca00f27380e4280f06240d8ba3f59ae Mon Sep 17 00:00:00 2001 From: Jacob Stephens Date: Mon, 19 Oct 2026 13:56:29 +0000 Subject: [PATCH 5/5] Format the push-comparison regression assertion --- tests/spec_push_race.rs | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/tests/spec_push_race.rs b/tests/spec_push_race.rs index 6241c4ae7..1946d2dd5 100644 --- a/tests/spec_push_race.rs +++ b/tests/spec_push_race.rs @@ -31,11 +31,7 @@ touch "$FAKE_CLAUDE_SCRIPT.armed" ); let result = scenario.run(&["merge", &scenario.issue_url(7)]); assert_eq!(result.code, Some(0), "{}", result.stderr); - assert!( - result.stderr.contains("is merged\n"), - "{}", - result.stderr - ); + assert!(result.stderr.contains("is merged\n"), "{}", result.stderr); assert!( scenario.origin_log("issue-7").is_none(), "obsolete push recreated the merged PR's deleted branch: {}",