Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion CONTEXT.md
Original file line number Diff line number Diff line change
Expand Up @@ -195,7 +195,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-<n>` (the first), then `issue-<n>-branch-<k>` for k ≥ 2. A number counts as used once its pull request is merged or closed, even if the branch itself was deleted.
Expand All @@ -209,6 +209,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.

Expand Down
17 changes: 15 additions & 2 deletions src/delivery.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -125,6 +127,17 @@ impl Delivery<'_> {
ticket_lines: Vec::new(),
}),
Err(error) => {
if let Some(merged) = error.downcast_ref::<AlreadyMerged>()
&& 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::<SecurityHold>() {
stage.set(Stage::SecurityReview);
}
Expand Down
27 changes: 27 additions & 0 deletions src/pull_request.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -410,6 +426,17 @@ impl<A: Adapter> PullRequest<A> {

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);
}
Expand Down
30 changes: 30 additions & 0 deletions src/worktree.rs
Original file line number Diff line number Diff line change
Expand Up @@ -235,6 +235,26 @@ 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<bool> {
self.checkout.ordinary(&self.launch, |operation| {
let entries = operation.run(&["-c", "core.fsmonitor=false", "ls-files", "-v", "-z"])?;
Ok(!index_hides_tracked_work(&entries)
&& 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<bool> {
Expand Down Expand Up @@ -351,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<Option<String>> {
let reference = format!("refs/heads/{branch}");
Expand Down
13 changes: 3 additions & 10 deletions src/worktree/acquisition.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down Expand Up @@ -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
Expand Down
131 changes: 131 additions & 0 deletions tests/merge_run.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,137 @@ 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);
}
}

#[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) {
Expand Down
10 changes: 5 additions & 5 deletions tests/pr_lifecycle.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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" {
Expand All @@ -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" {
Expand Down
8 changes: 2 additions & 6 deletions tests/spec_push_race.rs
Original file line number Diff line number Diff line change
Expand Up @@ -30,12 +30,8 @@ touch "$FAKE_CLAUDE_SCRIPT.armed"
"#,
);
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_eq!(result.code, Some(0), "{}", 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: {}",
Expand Down
Loading