From 617a879f5440d13051e5c7fa22c5caa59cf1519b Mon Sep 17 00:00:00 2001 From: Jacob Stephens Date: Tue, 6 Oct 2026 23:56:39 -0400 Subject: [PATCH 1/2] Deepen Worktree ownership of Failed run preservation (#482) --- src/delivery.rs | 159 ++++------ src/worktree.rs | 76 ++++- src/worktree/preservation_tests.rs | 474 +++++++++++++++++++++++++++++ 3 files changed, 596 insertions(+), 113 deletions(-) create mode 100644 src/worktree/preservation_tests.rs diff --git a/src/delivery.rs b/src/delivery.rs index dea30caec..28619adde 100644 --- a/src/delivery.rs +++ b/src/delivery.rs @@ -13,7 +13,6 @@ use std::path::PathBuf; use anyhow::{Context, Result, bail}; -use chrono::{SecondsFormat, Utc}; mod repair_loop; @@ -22,7 +21,6 @@ use crate::ci::{self, Ci, FailedChecks}; use crate::failed_run::{FailedRun, PolicyRefusal, interrupted_or}; use crate::github::{GitHub, Mergeable, PullRequest}; use crate::harness::Choice; -use crate::host; use crate::interrupt; use crate::issue::IssueUrl; use crate::poll; @@ -304,9 +302,9 @@ fn open(pr: Option) -> Result { /// instead: it can surface as some other error, such as a killed git. A /// `PolicyRefusal` neither pushes nor converts, so the PR stays ready on the /// head whose CI was watched. Problems along the way are reported, not -/// raised, so the error is what the Run fails with. The worktree is kept if -/// its work may not have reached origin. `log` is the most recent Session -/// log, if a session created one. +/// raised, so the error is what the Run fails with. Worktree owns preservation +/// and retains work that may not have reached origin. `log` is the most recent +/// Session log, if a session created one. fn fail(outside: &mut impl FailedOutside, log: Option, error: anyhow::Error) -> FailedRun { let interrupted = outside.interrupted(); let error = interrupted_or(error, interrupted); @@ -315,12 +313,7 @@ fn fail(outside: &mut impl FailedOutside, log: Option, error: anyhow::E let reason = Cause::of(&error).first_line().to_string(); // Everything is already on origin: the Repair loop pushed the head. let keep_ready = error.is::(); - let pushed = if keep_ready { - Ok(()) - } else { - outside.commit_and_push(&reason) - }; - if let Err(problem) = &pushed { + if !keep_ready && let Err(problem) = outside.preserve_failed_run(&reason) { outside.step(format!( "could not push the failed run's work, so it may exist only locally: {problem:#}" )); @@ -332,9 +325,6 @@ fn fail(outside: &mut impl FailedOutside, log: Option, error: anyhow::E None } }; - if pushed.is_err() { - outside.keep(); - } FailedRun { error, pr_url, @@ -394,15 +384,13 @@ trait Outside { /// What the Failed run path does or reads outside the Delivery, once its /// sessions have ended. trait FailedOutside { - /// Commit the work as the failure commit for `reason`, and push it. - fn commit_and_push(&mut self, reason: &str) -> Result<()>; + /// Preserve the Failed run's work for `reason`, retaining it locally if + /// preservation fails. + fn preserve_failed_run(&mut self, reason: &str) -> Result<()>; /// The pull request for the Issue branch, if it has one. fn pull_request(&mut self) -> Result>; /// Convert the pull request back to a draft. fn convert_to_draft(&mut self) -> Result<()>; - /// Keep the worktree and the local Issue branch, for work that may exist - /// nowhere else. - fn keep(&mut self); /// Whether an interrupt was requested. fn interrupted(&mut self) -> bool; /// Hand on the progress line `line`. @@ -485,9 +473,8 @@ impl Outside for InWorktree<'_> { } } -/// A failed Delivery's worktree, which it owns, so it can keep it, and its -/// pull request on GitHub. The worktree is cleaned up, unless kept, when -/// this is dropped. +/// A failed Delivery's worktree and its pull request on GitHub. Worktree +/// owns preservation and the cleanup decision when this is dropped. struct OnFailure<'a> { issue: &'a IssueUrl, base: &'a str, @@ -495,37 +482,8 @@ struct OnFailure<'a> { } impl FailedOutside for OnFailure<'_> { - /// Commit everything in the worktree, uncommitted work included, as the - /// failure commit for `reason`, and push the Issue branch. An unfinished - /// merge is aborted first. Does neither if the branch has no changes - /// against the Base branch, so no empty Issue branch appears on origin. - fn commit_and_push(&mut self, reason: &str) -> Result<()> { - let worktree = &self.worktree; - let git = worktree.git().completion(); - if git.merge_in_progress()? { - git.run(&["merge", "--abort"])?; - } - git.run(&["add", "-A"])?; - let base = format!("origin/{}", self.base); - if git.succeeds(&["diff", "--cached", "--quiet", &base])? { - return Ok(()); - } - let message = format!( - "thirdshift: failed run ({reason})\n\n\ - {timestamp}, host {host}. Uncommitted work at the time of failure is included in this commit.", - timestamp = Utc::now().to_rfc3339_opts(SecondsFormat::Secs, true), - host = host::name().as_deref().unwrap_or("unknown"), - ); - // No hooks: a hook that rejects the commit would strand the work. - git.run(&[ - "commit", - "-q", - "--allow-empty", - "--no-verify", - "-m", - &message, - ])?; - git.push(worktree.branch()) + fn preserve_failed_run(&mut self, reason: &str) -> Result<()> { + self.worktree.preserve_failed_run(self.base, reason) } fn pull_request(&mut self) -> Result> { @@ -540,10 +498,6 @@ impl FailedOutside for OnFailure<'_> { .convert_to_draft(self.issue, self.worktree.branch()) } - fn keep(&mut self) { - self.worktree.keep(); - } - fn interrupted(&mut self) -> bool { interrupt::requested() } @@ -722,7 +676,7 @@ mod scripted { DeleteBranch, IssueIsOpen, CloseIssue, - CommitAndPush, + PreserveFailedRun, ConvertToDraft, } @@ -749,11 +703,9 @@ mod scripted { IssueIsOpen, /// It closed the issue with this comment. CloseIssue(String), - /// It committed the work and pushed it, for this reason. - CommitAndPush(String), + /// It preserved the Failed run's work, for this reason. + PreserveFailedRun(String), ConvertToDraft, - /// It kept the worktree. - Keep, /// It asked whether an interrupt was requested. Interrupted, /// It handed on this progress line. @@ -952,9 +904,9 @@ mod scripted { } impl FailedOutside for Scripted { - fn commit_and_push(&mut self, reason: &str) -> Result<()> { - self.calls.push(Call::CommitAndPush(reason.to_string())); - self.fail_if(Fails::CommitAndPush) + fn preserve_failed_run(&mut self, reason: &str) -> Result<()> { + self.calls.push(Call::PreserveFailedRun(reason.to_string())); + self.fail_if(Fails::PreserveFailedRun) } fn pull_request(&mut self) -> Result> { @@ -968,10 +920,6 @@ mod scripted { Ok(()) } - fn keep(&mut self) { - self.calls.push(Call::Keep); - } - fn interrupted(&mut self) -> bool { Outside::interrupted(self) } @@ -1404,7 +1352,7 @@ mod tests { assert_eq!(failed.log, log, "{failing:?}"); assert!(!failed.interrupted, "{failing:?}"); assert!( - outside.calls.contains(&Call::CommitAndPush(error)), + outside.calls.contains(&Call::PreserveFailedRun(error)), "{failing:?}: {:?}", outside.calls ); @@ -1426,7 +1374,7 @@ mod tests { after(&outside, &Call::TakeToGoal(Goal::Merged)), [ Call::Interrupted, - Call::CommitAndPush("CI failed".to_string()), + Call::PreserveFailedRun("CI failed".to_string()), Call::PullRequest, Call::ConvertToDraft, ] @@ -1448,7 +1396,7 @@ mod tests { assert!( outside .calls - .contains(&Call::CommitAndPush("interrupted".to_string())) + .contains(&Call::PreserveFailedRun("interrupted".to_string())) ); } @@ -1486,39 +1434,52 @@ mod tests { assert!( outside .calls - .contains(&Call::CommitAndPush("test failed on main".to_string())) + .contains(&Call::PreserveFailedRun("test failed on main".to_string())) ); } #[test] - fn a_failed_push_keeps_the_worktree_and_says_so() { - let mut outside = Scripted::default() - .failing(Fails::Session) - .failing(Fails::CommitAndPush); - - let failed = failed(Goal::ReadyForReview, &mut outside); - - assert_eq!(cause(&failed), "Session failed"); - assert_eq!( - after(&outside, &Call::CommitAndPush("Session failed".to_string())), - [ - step( - "could not push the failed run's work, so it may exist only locally: \ - CommitAndPush failed" - ), - Call::PullRequest, - Call::Keep, - ] - ); - } - - #[test] - fn a_pushed_failure_does_not_keep_the_worktree() { - let mut outside = Scripted::default().failing(Fails::Session); + fn a_preservation_error_is_secondary_and_pr_accounting_keeps_the_original_failure() { + for interrupted in [false, true] { + let mut outside = Scripted { + interrupted, + pr: Some(Pr { + state: PrState::Open, + base: "main", + draft: false, + }), + ..Scripted::default() + } + .failing(Fails::PreserveFailedRun); + let log = PathBuf::from("/logs/7-implement.jsonl"); + let original = "session failed\nmore context"; - failed(Goal::ReadyForReview, &mut outside); + let failed = fail(&mut outside, Some(log.clone()), anyhow::anyhow!(original)); - assert!(!outside.calls.contains(&Call::Keep)); + let reason = if interrupted { + "interrupted" + } else { + "session failed" + }; + assert_eq!(cause(&failed), if interrupted { reason } else { original }); + assert_eq!(failed.interrupted, interrupted); + assert_eq!(failed.log, Some(log)); + assert_eq!(failed.pr_url.as_deref(), Some(PR_URL)); + assert!(outside.pr.unwrap().draft); + assert_eq!( + outside.calls, + [ + Call::Interrupted, + Call::PreserveFailedRun(reason.to_string()), + step( + "could not push the failed run's work, so it may exist only locally: \ + PreserveFailedRun failed" + ), + Call::PullRequest, + Call::ConvertToDraft, + ] + ); + } } #[test] diff --git a/src/worktree.rs b/src/worktree.rs index 586cf6291..abb678ec9 100644 --- a/src/worktree.rs +++ b/src/worktree.rs @@ -1,16 +1,21 @@ //! The Run's worktree: a sibling of the launch repository, on the Issue //! branch, removed together with the local Issue branch when dropped unless -//! it is kept. And the Architecture review's: a sibling too, on no branch, -//! always removed when dropped. +//! Failed run preservation fails. And the Architecture review's: a sibling +//! too, on no branch, always removed when dropped. use std::fs::File; use std::path::{Path, PathBuf}; use anyhow::{Context, Result, bail}; +use chrono::{SecondsFormat, Utc}; use crate::git::Git; +use crate::host; use crate::progress; +#[cfg(test)] +mod preservation_tests; + /// How merging the Base branch, or new commits on origin, into the Issue /// branch went. #[derive(Debug)] @@ -98,10 +103,6 @@ impl Worktree { &self.branch } - pub fn git(&self) -> &Git { - &self.git - } - /// The Launch directory the worktree was added from. pub fn launch(&self) -> &Git { &self.launch @@ -115,6 +116,42 @@ impl Worktree { self.git.push(&self.branch) } + /// Preserve all work from a Failed run against the last-fetched Base + /// branch, even after Command interruption. An unfinished merge is + /// aborted first; changes are committed as the failure marker and pushed + /// without local hooks. With no changes, nothing is committed or pushed. + /// Retention is armed before salvage: any error or unwinding leaves the + /// worktree and local Issue branch in place. Success allows normal cleanup. + pub fn preserve_failed_run(&mut self, base: &str, reason: &str) -> Result<()> { + self.kept = true; + let git = self.git.completion(); + if git.merge_in_progress()? { + git.run(&["merge", "--abort"])?; + } + git.run(&["add", "-A"])?; + let base = format!("origin/{base}"); + if !git.succeeds(&["diff", "--cached", "--quiet", &base])? { + let message = format!( + "thirdshift: failed run ({reason})\n\n\ + {timestamp}, host {host}. Uncommitted work at the time of failure is included in this commit.", + timestamp = Utc::now().to_rfc3339_opts(SecondsFormat::Secs, true), + host = host::name().as_deref().unwrap_or("unknown"), + ); + // No hooks: a hook that rejects the commit would strand the work. + git.run(&[ + "commit", + "-q", + "--allow-empty", + "--no-verify", + "-m", + &message, + ])?; + git.push(&self.branch)?; + } + self.kept = false; + Ok(()) + } + /// Delete the Issue branch on origin, or do nothing if it is already gone /// there, e.g. deleted by GitHub after a merge. Hooks are skipped, as for /// [`Worktree::push`]. This finishes a confirmed Self-merge, including @@ -132,13 +169,6 @@ impl Worktree { } } - /// Keep the worktree and the local Issue branch once it is dropped, for - /// work that may exist nowhere else. Dropping it then says where they - /// are and the branch's head commit, rather than removing them. - pub fn keep(&mut self) { - self.kept = true; - } - /// The Issue branch's head commit. pub fn head(&self) -> Result { self.git.run(&["rev-parse", "HEAD"]) @@ -365,7 +395,7 @@ mod tests { /// A clone `work` of a bare `origin.git` with one commit on `main`, both /// in a temp directory. No global or system config is read. - fn launch_directory() -> (tempfile::TempDir, Git) { + pub(super) fn launch_directory() -> (tempfile::TempDir, Git) { let temp = tempfile::TempDir::new().unwrap(); let git = |dir: &Path, args: &[&str]| { let status = Command::new("git") @@ -383,6 +413,24 @@ mod tests { ); git(temp.path(), &["clone", "-q", "origin.git", "work"]); let work = temp.path().join("work"); + let hooks = temp.path().join("hooks"); + std::fs::create_dir(&hooks).unwrap(); + git( + &temp.path().join("origin.git"), + &[ + "config", + "core.hooksPath", + temp.path().join("origin.git/hooks").to_str().unwrap(), + ], + ); + for (key, value) in [ + ("user.name", "Test Runner"), + ("user.email", "runner@example.com"), + ("commit.gpgSign", "false"), + ("core.hooksPath", hooks.to_str().unwrap()), + ] { + git(&work, &["config", key, value]); + } git( &work, &[ diff --git a/src/worktree/preservation_tests.rs b/src/worktree/preservation_tests.rs new file mode 100644 index 000000000..156782d9a --- /dev/null +++ b/src/worktree/preservation_tests.rs @@ -0,0 +1,474 @@ +//! Failed run preservation through Worktree, with real local Git and origin. + +use std::fs; +use std::os::unix::fs::PermissionsExt; +use std::path::Path; + +use super::*; +use crate::interrupt; +use crate::test_support::with_recorded_signal; + +const BRANCH: &str = "issue-7"; + +fn fixture() -> (tempfile::TempDir, Git, Worktree) { + let (temp, launch) = super::tests::launch_directory(); + let worktree = Worktree::create_fresh(&launch, "work", BRANCH, "main").unwrap(); + (temp, launch, worktree) +} + +fn rejecting_hook(path: &Path) { + fs::write(path, "#!/bin/sh\necho 'hook says no' >&2\nexit 1\n").unwrap(); + fs::set_permissions(path, fs::Permissions::from_mode(0o755)).unwrap(); +} + +fn unfinished_merge(git: &Git) { + fs::write(git.dir().join("conflict.txt"), "base\n").unwrap(); + fs::write(git.dir().join("merged.txt"), "base\n").unwrap(); + git.run(&["add", "-A"]).unwrap(); + git.run(&["commit", "-q", "-m", "Shared files"]).unwrap(); + git.run(&["checkout", "-q", "-b", "other"]).unwrap(); + fs::write(git.dir().join("conflict.txt"), "theirs\n").unwrap(); + fs::write(git.dir().join("merged.txt"), "theirs\n").unwrap(); + git.run(&["commit", "-q", "-am", "Theirs"]).unwrap(); + git.run(&["checkout", "-q", BRANCH]).unwrap(); + fs::write(git.dir().join("conflict.txt"), "ours\n").unwrap(); + git.run(&["commit", "-q", "-am", "Ours"]).unwrap(); + assert!(git.run(&["merge", "--no-edit", "other"]).is_err()); + assert!(git.merge_in_progress().unwrap()); + assert_eq!( + fs::read_to_string(git.dir().join("merged.txt")).unwrap(), + "theirs\n" + ); +} + +fn assert_cleaned_up(launch: &Git, path: &Path) { + assert!(!path.exists(), "worktree was retained: {}", path.display()); + assert!( + !launch + .succeeds(&["show-ref", "--verify", "--quiet", "refs/heads/issue-7"]) + .unwrap(), + "local Issue branch was retained" + ); +} + +fn assert_retained(launch: &Git, path: &Path) { + assert!(path.exists(), "worktree was removed: {}", path.display()); + assert!( + launch + .succeeds(&["show-ref", "--verify", "--quiet", "refs/heads/issue-7"]) + .unwrap(), + "local Issue branch was removed" + ); +} + +#[test] +fn preservation_pushes_committed_and_uncommitted_work_with_failure_metadata_then_cleans_up() { + let (temp, launch, mut worktree) = fixture(); + let path = worktree.path().to_path_buf(); + let git = Git::new(&path); + fs::write(path.join("feature.txt"), "feature\n").unwrap(); + git.run(&["add", "feature.txt"]).unwrap(); + git.run(&["commit", "-q", "-m", "Add feature"]).unwrap(); + fs::write(path.join("wip.txt"), "half done\n").unwrap(); + + worktree + .preserve_failed_run("main", "session failed") + .unwrap(); + drop(worktree); + + let origin = Git::new(temp.path().join("origin.git")); + assert_eq!( + origin.run(&["show", "issue-7:feature.txt"]).unwrap(), + "feature" + ); + assert_eq!( + origin.run(&["show", "issue-7:wip.txt"]).unwrap(), + "half done" + ); + assert_eq!( + origin.run(&["log", "-2", "--format=%s", BRANCH]).unwrap(), + "thirdshift: failed run (session failed)\nAdd feature" + ); + let body = origin.run(&["log", "-1", "--format=%b", BRANCH]).unwrap(); + let (timestamp, rest) = body.split_once(", host ").expect(&body); + chrono::DateTime::parse_from_rfc3339(timestamp).unwrap(); + assert!(timestamp.ends_with('Z'), "{body}"); + let host = rest + .strip_suffix(". Uncommitted work at the time of failure is included in this commit.") + .expect(&body); + assert!(!host.is_empty(), "{body}"); + assert_cleaned_up(&launch, &path); +} + +#[test] +fn a_staging_error_retains_uncommitted_work_and_the_local_branch_after_drop() { + let (_temp, launch, mut worktree) = fixture(); + let path = worktree.path().to_path_buf(); + let git = Git::new(&path); + let index = path.join(git.run(&["rev-parse", "--git-path", "index"]).unwrap()); + fs::remove_file(&index).unwrap(); + fs::create_dir(&index).unwrap(); + fs::write(path.join("wip.txt"), "unstaged work\n").unwrap(); + + let error = worktree + .preserve_failed_run("main", "session failed") + .unwrap_err(); + assert!( + error.to_string().starts_with("git add -A failed"), + "{error:#}" + ); + drop(worktree); + + assert_retained(&launch, &path); + assert_eq!( + fs::read_to_string(path.join("wip.txt")).unwrap(), + "unstaged work\n" + ); + assert_eq!( + launch.run(&["log", "-1", "--format=%s", BRANCH]).unwrap(), + "Initial" + ); +} + +#[test] +fn unchanged_work_makes_no_failure_commit_or_remote_branch_and_allows_cleanup() { + let (_temp, launch, mut worktree) = fixture(); + let path = worktree.path().to_path_buf(); + let head = worktree.head().unwrap(); + + worktree + .preserve_failed_run("main", "session failed") + .unwrap(); + + assert_eq!(worktree.head().unwrap(), head); + assert!(!launch.on_origin(BRANCH).unwrap()); + drop(worktree); + assert_cleaned_up(&launch, &path); +} + +#[test] +fn committed_work_gets_an_empty_failure_marker_despite_rejecting_local_hooks() { + let (temp, launch, mut worktree) = fixture(); + let path = worktree.path().to_path_buf(); + let git = Git::new(&path); + fs::write(path.join("feature.txt"), "feature\n").unwrap(); + git.run(&["add", "feature.txt"]).unwrap(); + git.run(&["commit", "-q", "-m", "Add feature"]).unwrap(); + let feature = worktree.head().unwrap(); + for hook in ["pre-commit", "commit-msg", "pre-push"] { + rejecting_hook(&temp.path().join("hooks").join(hook)); + } + + worktree + .preserve_failed_run("main", "session failed") + .unwrap(); + drop(worktree); + + let origin = Git::new(temp.path().join("origin.git")); + assert_eq!( + origin.run(&["log", "-1", "--format=%s", BRANCH]).unwrap(), + "thirdshift: failed run (session failed)" + ); + assert_eq!(origin.run(&["rev-parse", "issue-7^"]).unwrap(), feature); + assert!( + origin + .succeeds(&["diff", "--quiet", "issue-7^", BRANCH]) + .unwrap() + ); + assert_eq!( + origin.run(&["show", "issue-7:feature.txt"]).unwrap(), + "feature" + ); + assert_cleaned_up(&launch, &path); +} + +#[test] +fn an_unfinished_merge_is_aborted_before_preserving_the_issue_branch() { + let (temp, launch, mut worktree) = fixture(); + let path = worktree.path().to_path_buf(); + let git = Git::new(&path); + unfinished_merge(&git); + fs::write(path.join("wip.txt"), "half done\n").unwrap(); + + worktree + .preserve_failed_run("main", "session failed") + .unwrap(); + + assert!(!git.merge_in_progress().unwrap()); + drop(worktree); + let origin = Git::new(temp.path().join("origin.git")); + assert_eq!( + origin.run(&["show", "issue-7:conflict.txt"]).unwrap(), + "ours" + ); + assert_eq!(origin.run(&["show", "issue-7:merged.txt"]).unwrap(), "base"); + assert_eq!( + origin.run(&["show", "issue-7:wip.txt"]).unwrap(), + "half done" + ); + assert_eq!( + origin.run(&["log", "-3", "--format=%s", BRANCH]).unwrap(), + "thirdshift: failed run (session failed)\nOurs\nShared files" + ); + assert_cleaned_up(&launch, &path); +} + +#[test] +fn a_merge_abort_error_retains_the_conflict_and_edits_after_drop() { + let (_temp, launch, mut worktree) = fixture(); + let path = worktree.path().to_path_buf(); + let git = Git::new(&path); + unfinished_merge(&git); + let conflict = fs::read_to_string(path.join("conflict.txt")).unwrap(); + fs::write(path.join("merged.txt"), "edited after conflict\n").unwrap(); + + let error = worktree + .preserve_failed_run("main", "session failed") + .unwrap_err(); + assert!( + error.to_string().starts_with("git merge --abort failed"), + "{error:#}" + ); + drop(worktree); + + assert_retained(&launch, &path); + assert!(git.merge_in_progress().unwrap()); + assert_eq!( + fs::read_to_string(path.join("conflict.txt")).unwrap(), + conflict + ); + assert_eq!( + fs::read_to_string(path.join("merged.txt")).unwrap(), + "edited after conflict\n" + ); + assert_eq!( + launch.run(&["log", "-1", "--format=%s", BRANCH]).unwrap(), + "Ours" + ); +} + +#[test] +fn a_commit_error_retains_staged_work_and_the_local_branch_after_drop() { + let (_temp, launch, mut worktree) = fixture(); + let path = worktree.path().to_path_buf(); + let git = Git::new(&path); + git.run(&["config", "user.name", ""]).unwrap(); + fs::write(path.join("wip.txt"), "half done\n").unwrap(); + + let error = worktree + .preserve_failed_run("main", "session failed") + .unwrap_err(); + assert!(error.to_string().starts_with("git commit"), "{error:#}"); + drop(worktree); + + assert_retained(&launch, &path); + assert_eq!(git.run(&["show", ":wip.txt"]).unwrap(), "half done"); + assert_eq!( + fs::read_to_string(path.join("wip.txt")).unwrap(), + "half done\n" + ); + assert_eq!( + launch.run(&["log", "-1", "--format=%s", BRANCH]).unwrap(), + "Initial" + ); +} + +#[test] +fn rejected_or_unreachable_origin_retains_the_failure_commit_and_work_after_drop() { + for rejecting_origin in [true, false] { + let (temp, launch, mut worktree) = fixture(); + let path = worktree.path().to_path_buf(); + fs::write(path.join("wip.txt"), "half done\n").unwrap(); + if rejecting_origin { + rejecting_hook(&temp.path().join("origin.git/hooks/pre-receive")); + } else { + launch + .run(&[ + "remote", + "set-url", + "origin", + temp.path().join("unreachable.git").to_str().unwrap(), + ]) + .unwrap(); + } + + let error = worktree + .preserve_failed_run("main", "session failed") + .unwrap_err(); + assert!(error.to_string().starts_with("git push"), "{error:#}"); + drop(worktree); + + assert_retained(&launch, &path); + let git = Git::new(&path); + assert_eq!(git.run(&["show", "HEAD:wip.txt"]).unwrap(), "half done"); + assert_eq!( + fs::read_to_string(path.join("wip.txt")).unwrap(), + "half done\n" + ); + assert_eq!( + launch.run(&["log", "-1", "--format=%s", BRANCH]).unwrap(), + "thirdshift: failed run (session failed)" + ); + let origin = Git::new(temp.path().join("origin.git")); + assert!( + !origin + .succeeds(&["show-ref", "--verify", "--quiet", "refs/heads/issue-7"]) + .unwrap() + ); + } +} + +#[test] +fn recorded_interruption_allows_preservation_and_cleanup_but_still_blocks_ordinary_git() { + with_recorded_signal( + "worktree::preservation_tests::recorded_interruption_allows_preservation_and_cleanup_but_still_blocks_ordinary_git", + |signal| { + let (temp, launch, mut worktree) = fixture(); + let path = worktree.path().to_path_buf(); + fs::write(path.join("wip.txt"), "half done\n").unwrap(); + signal_hook::low_level::raise(signal).unwrap(); + + worktree.preserve_failed_run("main", "interrupted").unwrap(); + + assert!(interrupt::requested()); + assert_eq!(worktree.head().unwrap_err().to_string(), "interrupted"); + assert_eq!(worktree.push().unwrap_err().to_string(), "interrupted"); + drop(worktree); + let origin = Git::new(temp.path().join("origin.git")).completion(); + assert_eq!( + origin.run(&["show", "issue-7:wip.txt"]).unwrap(), + "half done" + ); + assert_eq!( + origin.run(&["log", "-1", "--format=%s", BRANCH]).unwrap(), + "thirdshift: failed run (interrupted)" + ); + assert_cleaned_up(&launch.completion(), &path); + assert!(interrupt::requested()); + assert_eq!( + launch.run(&["rev-parse", "HEAD"]).unwrap_err().to_string(), + "interrupted" + ); + assert_eq!( + Git::new(launch.dir()) + .run(&["rev-parse", "HEAD"]) + .unwrap_err() + .to_string(), + "interrupted" + ); + }, + ); +} + +#[test] +fn successful_preservation_after_an_error_restores_cleanup_even_with_unchanged_work() { + let (_temp, launch, mut worktree) = fixture(); + let path = worktree.path().to_path_buf(); + let git = Git::new(&path); + let index = path.join(git.run(&["rev-parse", "--git-path", "index"]).unwrap()); + let original_index = fs::read(&index).unwrap(); + fs::remove_file(&index).unwrap(); + fs::create_dir(&index).unwrap(); + assert!( + worktree + .preserve_failed_run("main", "session failed") + .is_err() + ); + fs::remove_dir(&index).unwrap(); + fs::write(index, original_index).unwrap(); + + worktree + .preserve_failed_run("main", "session failed") + .unwrap(); + drop(worktree); + + assert!(!launch.on_origin(BRANCH).unwrap()); + assert_cleaned_up(&launch, &path); +} + +#[test] +fn preservation_compares_with_the_last_fetched_settled_base_without_fetching_again() { + let (temp, launch) = super::tests::launch_directory(); + launch.run(&["checkout", "-q", "-b", "develop"]).unwrap(); + fs::write(launch.dir().join("develop.txt"), "settled base\n").unwrap(); + launch.run(&["add", "-A"]).unwrap(); + launch.run(&["commit", "-q", "-m", "Develop base"]).unwrap(); + launch.run(&["push", "-q", "origin", "develop"]).unwrap(); + let mut worktree = Worktree::create_fresh(&launch, "work", BRANCH, "develop").unwrap(); + let path = worktree.path().to_path_buf(); + let head = worktree.head().unwrap(); + fs::write(launch.dir().join("later.txt"), "new base work\n").unwrap(); + launch.run(&["add", "-A"]).unwrap(); + launch + .run(&["commit", "-q", "-m", "Later base work"]) + .unwrap(); + let origin = Git::new(temp.path().join("origin.git")); + origin + .run(&["fetch", launch.dir().to_str().unwrap(), "develop:develop"]) + .unwrap(); + + worktree + .preserve_failed_run("develop", "session failed") + .unwrap(); + + assert_eq!(worktree.head().unwrap(), head); + assert_eq!( + origin.run(&["show", "develop:later.txt"]).unwrap(), + "new base work" + ); + assert!(!launch.on_origin(BRANCH).unwrap()); + drop(worktree); + assert_cleaned_up(&launch, &path); +} + +#[test] +fn a_completed_nonzero_cached_diff_probe_still_makes_and_pushes_the_failure_marker() { + let (temp, launch, mut worktree) = fixture(); + let path = worktree.path().to_path_buf(); + // A missing comparison ref makes Git's probe exit nonzero even though + // the index is unchanged. Preservation keeps the existing boolean semantics. + launch + .run(&["update-ref", "-d", "refs/remotes/origin/main"]) + .unwrap(); + + worktree + .preserve_failed_run("main", "session failed") + .unwrap(); + drop(worktree); + + let origin = Git::new(temp.path().join("origin.git")); + assert_eq!( + origin.run(&["log", "-2", "--format=%s", BRANCH]).unwrap(), + "thirdshift: failed run (session failed)\nInitial" + ); + assert!( + origin + .succeeds(&["diff", "--quiet", "main", BRANCH]) + .unwrap() + ); + assert_cleaned_up(&launch, &path); +} + +#[test] +fn unwinding_while_handling_a_preservation_error_keeps_the_local_work() { + let (_temp, launch, worktree) = fixture(); + let path = worktree.path().to_path_buf(); + let git = Git::new(&path); + git.run(&["config", "user.name", ""]).unwrap(); + fs::write(path.join("wip.txt"), "half done\n").unwrap(); + + let unwound = std::panic::catch_unwind(std::panic::AssertUnwindSafe(move || { + let mut worktree = worktree; + worktree + .preserve_failed_run("main", "session failed") + .unwrap(); + })); + + assert!(unwound.is_err()); + assert_retained(&launch, &path); + assert_eq!(git.run(&["show", ":wip.txt"]).unwrap(), "half done"); + assert_eq!( + fs::read_to_string(path.join("wip.txt")).unwrap(), + "half done\n" + ); +} From 29fc5563d74fe45688815b31b948c22170c23590 Mon Sep 17 00:00:00 2001 From: Jacob Stephens Date: Tue, 6 Oct 2026 23:58:45 -0400 Subject: [PATCH 2/2] Verify Failed run reporting without internal call ordering --- src/delivery.rs | 28 ++++++++-------------------- 1 file changed, 8 insertions(+), 20 deletions(-) diff --git a/src/delivery.rs b/src/delivery.rs index 28619adde..76ec7f23c 100644 --- a/src/delivery.rs +++ b/src/delivery.rs @@ -1440,21 +1440,18 @@ mod tests { #[test] fn a_preservation_error_is_secondary_and_pr_accounting_keeps_the_original_failure() { + let original = "session failed\nmore context"; for interrupted in [false, true] { let mut outside = Scripted { interrupted, - pr: Some(Pr { - state: PrState::Open, - base: "main", - draft: false, - }), + repair_loop_error: original, ..Scripted::default() } + .failing(Fails::TakeToGoal) .failing(Fails::PreserveFailedRun); let log = PathBuf::from("/logs/7-implement.jsonl"); - let original = "session failed\nmore context"; - let failed = fail(&mut outside, Some(log.clone()), anyhow::anyhow!(original)); + let failed = failed(Goal::ReadyForReview, &mut outside); let reason = if interrupted { "interrupted" @@ -1466,19 +1463,10 @@ mod tests { assert_eq!(failed.log, Some(log)); assert_eq!(failed.pr_url.as_deref(), Some(PR_URL)); assert!(outside.pr.unwrap().draft); - assert_eq!( - outside.calls, - [ - Call::Interrupted, - Call::PreserveFailedRun(reason.to_string()), - step( - "could not push the failed run's work, so it may exist only locally: \ - PreserveFailedRun failed" - ), - Call::PullRequest, - Call::ConvertToDraft, - ] - ); + assert!(outside.calls.contains(&step( + "could not push the failed run's work, so it may exist only locally: \ + PreserveFailedRun failed" + ))); } }