Skip to content

Move Failed run preservation and retention into Worktree #484

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
JacobStephens2 merged 2 commits into from
Oct 7, 2026
Merged

Move Failed run preservation and retention into Worktree #484

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
617a879
Deepen Worktree ownership of Failed run preservation (#482)
JacobStephens2 Oct 7, 2026
29fc556
Verify Failed run reporting without internal call ordering
JacobStephens2 Oct 7, 2026
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
147 changes: 48 additions & 99 deletions src/delivery.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@
use std::path::PathBuf;

use anyhow::{Context, Result, bail};
use chrono::{SecondsFormat, Utc};

mod repair_loop;

Expand All @@ -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;
Expand Down Expand Up @@ -304,9 +302,9 @@ fn open(pr: Option<PullRequest>) -> Result<PullRequest> {
/// 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<PathBuf>, error: anyhow::Error) -> FailedRun {
let interrupted = outside.interrupted();
let error = interrupted_or(error, interrupted);
Expand All @@ -315,12 +313,7 @@ fn fail(outside: &mut impl FailedOutside, log: Option<PathBuf>, 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::<PolicyRefusal>();
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:#}"
));
Expand All @@ -332,9 +325,6 @@ fn fail(outside: &mut impl FailedOutside, log: Option<PathBuf>, error: anyhow::E
None
}
};
if pushed.is_err() {
outside.keep();
}
FailedRun {
error,
pr_url,
Expand Down Expand Up @@ -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<Option<PullRequest>>;
/// 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`.
Expand Down Expand Up @@ -485,47 +473,17 @@ 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,
worktree: Worktree,
}

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<Option<PullRequest>> {
Expand All @@ -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()
}
Expand Down Expand Up @@ -722,7 +676,7 @@ mod scripted {
DeleteBranch,
IssueIsOpen,
CloseIssue,
CommitAndPush,
PreserveFailedRun,
ConvertToDraft,
}

Expand All @@ -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.
Expand Down Expand Up @@ -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<Option<PullRequest>> {
Expand All @@ -968,10 +920,6 @@ mod scripted {
Ok(())
}

fn keep(&mut self) {
self.calls.push(Call::Keep);
}

fn interrupted(&mut self) -> bool {
Outside::interrupted(self)
}
Expand Down Expand Up @@ -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
);
Expand All @@ -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,
]
Expand All @@ -1448,7 +1396,7 @@ mod tests {
assert!(
outside
.calls
.contains(&Call::CommitAndPush("interrupted".to_string()))
.contains(&Call::PreserveFailedRun("interrupted".to_string()))
);
}

Expand Down Expand Up @@ -1486,39 +1434,40 @@ 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() {
let original = "session failed\nmore context";
for interrupted in [false, true] {
let mut outside = Scripted {
interrupted,
repair_loop_error: original,
..Scripted::default()
}
.failing(Fails::TakeToGoal)
.failing(Fails::PreserveFailedRun);
let log = PathBuf::from("/logs/7-implement.jsonl");

failed(Goal::ReadyForReview, &mut outside);
let failed = failed(Goal::ReadyForReview, &mut outside);

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!(outside.calls.contains(&step(
"could not push the failed run's work, so it may exist only locally: \
PreserveFailedRun failed"
)));
}
}

#[test]
Expand Down
Loading
Loading