From 28ac5e5d5c8e67127aa4e9df6066352eec04aa14 Mon Sep 17 00:00:00 2001 From: Jacob Stephens Date: Tue, 20 Oct 2026 03:37:33 -0400 Subject: [PATCH 1/2] Deepen Recovery so each layer adds its facts and Recovery owns the rules for its lines Recovery's fields are private. The Delivery records its worktree as it builds the Recovery, the Spec run adds Ticket progress and child work, and the Run's start and end add the Continuation context and Base fix work; Recovery decides the preservation upgrade, the final-review flag and the order of retained work, and its own tests read them through lines(). Co-Authored-By: Claude Opus 5.5 --- GLOSSARY.md | 5 +- docs/code-navigation.md | 1 + src/delivery.rs | 24 +++-- src/recovery.rs | 174 +++++++++++++++++++++++++++++++---- src/run.rs | 24 ++--- src/run_ending.rs | 31 ++++--- src/spec_run.rs | 8 +- src/spec_run/ticket_board.rs | 1 - 8 files changed, 209 insertions(+), 59 deletions(-) diff --git a/GLOSSARY.md b/GLOSSARY.md index 5aa2ec71..93263b7c 100644 --- a/GLOSSARY.md +++ b/GLOSSARY.md @@ -195,7 +195,10 @@ 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, 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. +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. It ends with its **Recovery**. + +**Recovery**: +The lines a **Failed run** ends with, saying where it stopped, what work was preserved and where, and the command that continues or retries it; built only from factory-owned facts, never agent output. **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. diff --git a/docs/code-navigation.md b/docs/code-navigation.md index cde2d27e..8bd6ad04 100644 --- a/docs/code-navigation.md +++ b/docs/code-navigation.md @@ -12,6 +12,7 @@ Start with the issue's terms in `GLOSSARY.md`, then use this index to find their | Ticket board, Tickets checklist, Unready Ticket | `src/spec_run/ticket_board.rs` — `TicketBoard`, `next_to_start`; `src/labels.rs` | Inline tests; `tests/spec_run.rs` | | Delivery, Repair loop, Foreign commit | `src/delivery.rs` — `Delivery`; `src/delivery/repair_loop.rs`; Delivery stage: `src/delivery/stage.rs` — `Stage`, `Tracker`, its table to the `Stopped at:` and Command record stages | Inline tests; `tests/merge_run.rs` | | Failed run | `src/failed_run.rs` | `tests/failed_run.rs` | +| Recovery | `src/recovery.rs` — `Recovery`, its add-a-fact methods and `lines` | Inline tests in `src/recovery.rs`; `tests/failed_run.rs`, `tests/spec_run.rs` | | Inherited failure, check-state reading, Base fix | CI watch, `src/ci.rs` — `reading` of each `Conclusion` the GitHub adapter (`src/github.rs`, `checks_on`) reports; `src/base_fix.rs` | Inline tests; `src/github/execution_tests.rs`; `tests/ci.rs`, `tests/base_fix.rs` | | Run notification | `src/notification.rs` — `RunNotification`; `src/email.rs` | `tests/run_notification.rs`, `tests/email_test.rs` | | Harness, Model, Effort | `src/harness.rs` and `src/harness/` | `tests/harness_choice.rs`, `tests/harness.rs` | diff --git a/src/delivery.rs b/src/delivery.rs index 9230dabe..6a81ed39 100644 --- a/src/delivery.rs +++ b/src/delivery.rs @@ -186,11 +186,7 @@ impl Delivery<'_> { worktree, pull_request, }; - let mut failed = fail(&mut outside, stopped_at, log, error); - if let Some(recovery) = &mut failed.recovery { - recovery.worktree = Some(path); - } - Err(failed) + Err(fail(&mut outside, stopped_at, path, log, error)) } } } @@ -354,10 +350,11 @@ fn retry_if_interrupted( /// 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; `stopped_at`, the stage its -/// recovery lines report. +/// recovery lines report; `worktree`, where work preserved only locally is. fn fail( outside: &mut impl FailedOutside, stopped_at: recovery::Stage, + worktree: PathBuf, log: Option, error: anyhow::Error, ) -> FailedRun { @@ -389,8 +386,10 @@ fn fail( None } }; + let mut recovery = Recovery::new(stopped_at, preservation); + recovery.record_worktree(worktree); FailedRun { - recovery: Some(Box::new(Recovery::new(stopped_at, preservation))), + recovery: Some(Box::new(recovery)), error, pr_url, log, @@ -942,13 +941,19 @@ mod tests { tracker.enter(*stage); } let stopped_at = stopped_at(&tracker, &error); - fail(outside, stopped_at, outside.log.clone(), error) + fail( + outside, + stopped_at, + PathBuf::from("/worktree"), + outside.log.clone(), + error, + ) }) } /// The stage `failed` stopped at. fn stopped(failed: &FailedRun) -> recovery::Stage { - failed.recovery.as_ref().expect("no recovery").stage + failed.recovery.as_ref().expect("no recovery").stage() } #[test] @@ -1096,6 +1101,7 @@ mod tests { let failed = fail( &mut outside, recovery::Stage::Implementation, + PathBuf::from("/worktree"), Some(PathBuf::from("/logs/implement.jsonl")), anyhow!("original\nextra context"), ); diff --git a/src/recovery.rs b/src/recovery.rs index c2af1f04..5abaa0a9 100644 --- a/src/recovery.rs +++ b/src/recovery.rs @@ -63,24 +63,28 @@ pub struct Continuation { pub branch: String, } +/// Closed/total Tickets from the last lifecycle graph, as the Ticket board +/// reports them. #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub struct TicketProgress { pub landed: usize, pub total: usize, - pub reviewed: bool, } +/// The facts each layer of a Failed run adds, from the Delivery up to the +/// Run's end; the rules for how each fact changes the lines live here. #[derive(Debug, PartialEq, Eq)] pub struct Recovery { - pub stage: Stage, - pub preservation: Preservation, - pub worktree: Option, - /// Closed/total Tickets from the last lifecycle graph, and whether the - /// Spec review completed, independently of any later delivery failure. - pub tickets: Option, - pub continuation: Option, + stage: Stage, + preservation: Preservation, + worktree: Option, + tickets: Option, + /// Whether the Spec review completed, independently of any later + /// delivery failure. + reviewed: bool, + continuation: Option, /// Factory-reported retention facts from child Runs, including their paths. - pub local_child_work: Vec, + local_child_work: Vec, } impl Recovery { @@ -90,21 +94,55 @@ impl Recovery { preservation, worktree: None, tickets: None, + reviewed: false, continuation: None, local_child_work: Vec::new(), } } + /// Only tests read the stage: its rules live here. + #[cfg(test)] + pub fn stage(&self) -> Stage { + self.stage + } + + /// Record `path` as the worktree that holds work preserved only locally. + pub fn record_worktree(&mut self, path: PathBuf) { + self.worktree = Some(path); + } + + /// Add Ticket `progress`; the final review is complete only past the + /// Ticket and Spec review stages, and only if `all_accounted`: every + /// Ticket was accounted for. + pub fn add_tickets(&mut self, progress: TicketProgress, all_accounted: bool) { + self.tickets = Some(progress); + self.reviewed = !matches!(self.stage, Stage::Tickets | Stage::SpecReview) && all_accounted; + } + + /// Add the public command `context`. A Continuation whose Issue branch + /// was already on origin, `on_origin`, keeps that work there even when + /// nothing more was preserved. + pub fn add_continuation(&mut self, context: Continuation, on_origin: bool) { + if self.preservation == Preservation::None && on_origin { + self.preservation = Preservation::Remote; + } + self.continuation = Some(context); + } + + /// Add child Runs' retained `work`, after any already added. + pub fn add_child_work(&mut self, work: impl IntoIterator) { + self.local_child_work.extend(work); + } + /// Literal lines shared by terminal, Activity and Run notification output. pub fn lines(&self) -> Vec { let mut lines = vec![format!("{STOPPED}{}", self.stage.name())]; - if let Some(TicketProgress { - landed, - total, - reviewed, - }) = self.tickets - { - let review = if reviewed { "complete" } else { "incomplete" }; + if let Some(TicketProgress { landed, total }) = self.tickets { + let review = if self.reviewed { + "complete" + } else { + "incomplete" + }; lines.push(format!( "{PROGRESS}{landed}/{total} Tickets landed; final review {review}" )); @@ -172,3 +210,107 @@ impl Recovery { .any(|prefix| line.starts_with(prefix)) } } + +#[cfg(test)] +mod tests { + use super::*; + + fn context() -> Continuation { + Continuation { + command: "thirdshift https://github.com/acme/widgets/issues/21".into(), + launch: "/launch".into(), + base: "main".into(), + branch: "issue-21".into(), + } + } + + fn preserved(recovery: &Recovery) -> String { + recovery + .lines() + .into_iter() + .find(|line| line.starts_with(PRESERVED)) + .expect("no Preserved line") + } + + fn has(recovery: &Recovery, prefix: &str) -> bool { + recovery.lines().iter().any(|line| line.starts_with(prefix)) + } + + #[test] + fn a_continuation_with_nothing_pushed_reads_preserved_remotely() { + let mut recovery = Recovery::new(Stage::Implementation, Preservation::None); + recovery.add_continuation(context(), true); + assert_eq!(preserved(&recovery), "Preserved: remotely on issue-21"); + assert!(has(&recovery, CONTINUING)); + assert!(!has(&recovery, RETRY)); + } + + #[test] + fn a_fresh_run_with_nothing_pushed_reads_no_work_preserved() { + let mut recovery = Recovery::new(Stage::Implementation, Preservation::None); + recovery.add_continuation(context(), false); + assert_eq!(preserved(&recovery), "Preserved: no work preserved"); + assert!(has(&recovery, RETRY)); + assert!(!has(&recovery, CONTINUING)); + } + + #[test] + fn the_final_review_is_complete_only_past_the_spec_review_with_every_ticket_accounted() { + let progress = TicketProgress { + landed: 3, + total: 3, + }; + for (stage, all_accounted, review) in [ + (Stage::Tickets, true, "incomplete"), + (Stage::SpecReview, true, "incomplete"), + (Stage::Push, false, "incomplete"), + (Stage::Push, true, "complete"), + (Stage::Ci, true, "complete"), + ] { + let mut recovery = Recovery::new(stage, Preservation::Remote); + recovery.add_tickets(progress, all_accounted); + assert_eq!( + recovery.lines()[1], + format!("Progress: 3/3 Tickets landed; final review {review}"), + "{stage:?}, all accounted: {all_accounted}" + ); + } + } + + #[test] + fn retained_work_from_two_layers_is_kept_in_order() { + let mut recovery = Recovery::new(Stage::Ci, Preservation::Remote); + recovery.add_continuation(context(), false); + recovery.add_child_work(["Ticket #22 kept at /kept/22".to_string()]); + recovery.add_child_work(["Base fix #30 kept at /kept/30".to_string()]); + let lines = recovery.lines(); + let retained: Vec<_> = lines + .iter() + .filter(|line| line.starts_with(RETAINED_WORK)) + .collect(); + assert_eq!( + retained, + [ + "Retained work: Ticket #22 kept at /kept/22", + "Retained work: Base fix #30 kept at /kept/30" + ] + ); + assert!(has(&recovery, BEFORE_RETRYING)); + } + + #[test] + fn local_preservation_names_the_recorded_worktree_or_else_the_launch_directory() { + let mut recovery = Recovery::new(Stage::Push, Preservation::Local); + recovery.add_continuation(context(), false); + assert_eq!( + preserved(&recovery), + "Preserved: work may exist only locally on issue-21 at /launch; push was not confirmed" + ); + recovery.record_worktree("/worktrees/issue-21".into()); + assert_eq!( + preserved(&recovery), + "Preserved: work may exist only locally on issue-21 at /worktrees/issue-21; push was not confirmed" + ); + assert!(has(&recovery, BEFORE_RETRYING)); + } +} diff --git a/src/run.rs b/src/run.rs index 6b5bf733..81f1910a 100644 --- a/src/run.rs +++ b/src/run.rs @@ -131,9 +131,7 @@ pub fn run_to_end(issue: &IssueUrl, asks: &mut Asks, started_by: StartedBy) -> E if let Err(failed) = &mut outcome && let Some(recovery) = &mut failed.recovery { - recovery - .local_child_work - .extend_from_slice(base_fix.local_work()); + recovery.add_child_work(base_fix.local_work().iter().cloned()); } Ended { outcome, @@ -289,17 +287,15 @@ fn start( if let Err(failed) = &mut outcome && let Some(recovery) = &mut failed.recovery { - if recovery.preservation == crate::recovery::Preservation::None - && matches!(selection, Selection::Continuation { .. }) - { - recovery.preservation = crate::recovery::Preservation::Remote; - } - recovery.continuation = Some(crate::recovery::Continuation { - command: crate::args::continuation(issue, asks, is_spec), - launch: outside.launch_path().to_path_buf(), - base: base.clone(), - branch: selection.branch().to_string(), - }); + recovery.add_continuation( + crate::recovery::Continuation { + command: crate::args::continuation(issue, asks, is_spec), + launch: outside.launch_path().to_path_buf(), + base: base.clone(), + branch: selection.branch().to_string(), + }, + matches!(selection, Selection::Continuation { .. }), + ); } if claims { outside.end_claim(outcome.as_ref().ok().map(|reached| reached.goal)); diff --git a/src/run_ending.rs b/src/run_ending.rs index 147072a3..5aa2f8c2 100644 --- a/src/run_ending.rs +++ b/src/run_ending.rs @@ -983,19 +983,24 @@ mod tests { let mut ended = failed("claude exited 3", Some(LOG), Some(PR)); let failure = ended.outcome.as_mut().err().unwrap(); let mut recovery = Recovery::new(Stage::SpecReview, Preservation::Remote); - recovery.tickets = Some(TicketProgress { - landed: 17, - total: 17, - reviewed: false, - }); - recovery.continuation = Some(Continuation { - command: - "thirdshift https://github.com/acme/widgets/issues/21 --no-merge harness claude" - .into(), - launch: "/launch".into(), - branch: "issue-21".into(), - base: "main".into(), - }); + recovery.add_tickets( + TicketProgress { + landed: 17, + total: 17, + }, + true, + ); + recovery.add_continuation( + Continuation { + command: + "thirdshift https://github.com/acme/widgets/issues/21 --no-merge harness claude" + .into(), + launch: "/launch".into(), + branch: "issue-21".into(), + base: "main".into(), + }, + false, + ); failure.recovery = Some(Box::new(recovery)); let account = Account::of_run(&ended); let shown = Shown::of(&account); diff --git a/src/spec_run.rs b/src/spec_run.rs index 4999318d..a62f0198 100644 --- a/src/spec_run.rs +++ b/src/spec_run.rs @@ -87,7 +87,7 @@ fn run_through( ) -> Result { let Landing { lines: ticket_lines, - mut progress, + progress, pushed, landed, local_work, @@ -130,10 +130,8 @@ fn run_through( let recovery = failed.recovery.get_or_insert_with(|| { Box::new(Recovery::new(Stage::SpecReview, Preservation::Remote)) }); - progress.reviewed = !matches!(recovery.stage, Stage::Tickets | Stage::SpecReview) - && all_tickets_accounted; - recovery.tickets = Some(progress); - recovery.local_child_work = local_work; + recovery.add_tickets(progress, all_tickets_accounted); + recovery.add_child_work(local_work); failed.ticket_lines = ticket_lines; Err(failed) } diff --git a/src/spec_run/ticket_board.rs b/src/spec_run/ticket_board.rs index e66905c3..454da31c 100644 --- a/src/spec_run/ticket_board.rs +++ b/src/spec_run/ticket_board.rs @@ -101,7 +101,6 @@ impl TicketBoard { crate::recovery::TicketProgress { landed: self.tickets.iter().filter(|ticket| !ticket.is_open).count(), total: self.tickets.len(), - reviewed: false, } } From aa4aa9f995ed8c5851e80ba403f47329c9d38d17 Mon Sep 17 00:00:00 2001 From: Jacob Stephens Date: Tue, 20 Oct 2026 03:50:53 -0400 Subject: [PATCH 2/2] Name Recovery's command context for what every Failed run gets, and keep the review flag with its Ticket progress Co-Authored-By: Claude Opus 5.5 --- src/recovery.rs | 36 +++++++++++++++--------------------- src/run.rs | 2 +- src/run_ending.rs | 2 +- 3 files changed, 17 insertions(+), 23 deletions(-) diff --git a/src/recovery.rs b/src/recovery.rs index 5abaa0a9..59d3054e 100644 --- a/src/recovery.rs +++ b/src/recovery.rs @@ -78,10 +78,9 @@ pub struct Recovery { stage: Stage, preservation: Preservation, worktree: Option, - tickets: Option, - /// Whether the Spec review completed, independently of any later - /// delivery failure. - reviewed: bool, + /// Ticket progress, and whether the Spec review completed, + /// independently of any later delivery failure. + tickets: Option<(TicketProgress, bool)>, continuation: Option, /// Factory-reported retention facts from child Runs, including their paths. local_child_work: Vec, @@ -94,7 +93,6 @@ impl Recovery { preservation, worktree: None, tickets: None, - reviewed: false, continuation: None, local_child_work: Vec::new(), } @@ -115,14 +113,14 @@ impl Recovery { /// Ticket and Spec review stages, and only if `all_accounted`: every /// Ticket was accounted for. pub fn add_tickets(&mut self, progress: TicketProgress, all_accounted: bool) { - self.tickets = Some(progress); - self.reviewed = !matches!(self.stage, Stage::Tickets | Stage::SpecReview) && all_accounted; + let reviewed = !matches!(self.stage, Stage::Tickets | Stage::SpecReview) && all_accounted; + self.tickets = Some((progress, reviewed)); } - /// Add the public command `context`. A Continuation whose Issue branch - /// was already on origin, `on_origin`, keeps that work there even when - /// nothing more was preserved. - pub fn add_continuation(&mut self, context: Continuation, on_origin: bool) { + /// Add the public command `context` that continues or retries the Run. + /// A **Continuation**, whose Issue branch was already on origin, + /// `on_origin`, keeps that work there even when nothing more was preserved. + pub fn add_command_context(&mut self, context: Continuation, on_origin: bool) { if self.preservation == Preservation::None && on_origin { self.preservation = Preservation::Remote; } @@ -137,12 +135,8 @@ impl Recovery { /// Literal lines shared by terminal, Activity and Run notification output. pub fn lines(&self) -> Vec { let mut lines = vec![format!("{STOPPED}{}", self.stage.name())]; - if let Some(TicketProgress { landed, total }) = self.tickets { - let review = if self.reviewed { - "complete" - } else { - "incomplete" - }; + if let Some((TicketProgress { landed, total }, reviewed)) = self.tickets { + let review = if reviewed { "complete" } else { "incomplete" }; lines.push(format!( "{PROGRESS}{landed}/{total} Tickets landed; final review {review}" )); @@ -239,7 +233,7 @@ mod tests { #[test] fn a_continuation_with_nothing_pushed_reads_preserved_remotely() { let mut recovery = Recovery::new(Stage::Implementation, Preservation::None); - recovery.add_continuation(context(), true); + recovery.add_command_context(context(), true); assert_eq!(preserved(&recovery), "Preserved: remotely on issue-21"); assert!(has(&recovery, CONTINUING)); assert!(!has(&recovery, RETRY)); @@ -248,7 +242,7 @@ mod tests { #[test] fn a_fresh_run_with_nothing_pushed_reads_no_work_preserved() { let mut recovery = Recovery::new(Stage::Implementation, Preservation::None); - recovery.add_continuation(context(), false); + recovery.add_command_context(context(), false); assert_eq!(preserved(&recovery), "Preserved: no work preserved"); assert!(has(&recovery, RETRY)); assert!(!has(&recovery, CONTINUING)); @@ -280,7 +274,7 @@ mod tests { #[test] fn retained_work_from_two_layers_is_kept_in_order() { let mut recovery = Recovery::new(Stage::Ci, Preservation::Remote); - recovery.add_continuation(context(), false); + recovery.add_command_context(context(), false); recovery.add_child_work(["Ticket #22 kept at /kept/22".to_string()]); recovery.add_child_work(["Base fix #30 kept at /kept/30".to_string()]); let lines = recovery.lines(); @@ -301,7 +295,7 @@ mod tests { #[test] fn local_preservation_names_the_recorded_worktree_or_else_the_launch_directory() { let mut recovery = Recovery::new(Stage::Push, Preservation::Local); - recovery.add_continuation(context(), false); + recovery.add_command_context(context(), false); assert_eq!( preserved(&recovery), "Preserved: work may exist only locally on issue-21 at /launch; push was not confirmed" diff --git a/src/run.rs b/src/run.rs index 81f1910a..8b08d4f3 100644 --- a/src/run.rs +++ b/src/run.rs @@ -287,7 +287,7 @@ fn start( if let Err(failed) = &mut outcome && let Some(recovery) = &mut failed.recovery { - recovery.add_continuation( + recovery.add_command_context( crate::recovery::Continuation { command: crate::args::continuation(issue, asks, is_spec), launch: outside.launch_path().to_path_buf(), diff --git a/src/run_ending.rs b/src/run_ending.rs index 5aa2f8c2..6181604f 100644 --- a/src/run_ending.rs +++ b/src/run_ending.rs @@ -990,7 +990,7 @@ mod tests { }, true, ); - recovery.add_continuation( + recovery.add_command_context( Continuation { command: "thirdshift https://github.com/acme/widgets/issues/21 --no-merge harness claude"