Deepen Recovery so each layer adds its facts and Recovery owns the rules for its lines

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-<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.

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` |

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<O: Outside>(
 /// 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<PathBuf>,
     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"),
             );

src/recovery.rs

@@ -63,24 +63,27 @@ 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<PathBuf>,
-    /// Closed/total Tickets from the last lifecycle graph, and whether the
-    /// Spec review completed, independently of any later delivery failure.
-    pub tickets: Option<TicketProgress>,
-    pub continuation: Option<Continuation>,
+    stage: Stage,
+    preservation: Preservation,
+    worktree: Option<PathBuf>,
+    /// Ticket progress, and whether the Spec review completed,
+    /// independently of any later delivery failure.
+    tickets: Option<(TicketProgress, bool)>,
+    continuation: Option<Continuation>,
     /// Factory-reported retention facts from child Runs, including their paths.
-    pub local_child_work: Vec<String>,
+    local_child_work: Vec<String>,
 }
 
 impl Recovery {
@@ -95,15 +98,44 @@ impl Recovery {
         }
     }
 
+    /// 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) {
+        let reviewed = !matches!(self.stage, Stage::Tickets | Stage::SpecReview) && all_accounted;
+        self.tickets = Some((progress, reviewed));
+    }
+
+    /// 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;
+        }
+        self.continuation = Some(context);
+    }
+
+    /// Add child Runs' retained `work`, after any already added.
+    pub fn add_child_work(&mut self, work: impl IntoIterator<Item = String>) {
+        self.local_child_work.extend(work);
+    }
+
     /// Literal lines shared by terminal, Activity and Run notification output.
     pub fn lines(&self) -> Vec<String> {
         let mut lines = vec![format!("{STOPPED}{}", self.stage.name())];
-        if let Some(TicketProgress {
-            landed,
-            total,
-            reviewed,
-        }) = self.tickets
-        {
+        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}"
@@ -172,3 +204,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_command_context(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_command_context(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_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();
+        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_command_context(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));
+    }
+}

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<O: Outside>(
     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_command_context(
+            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));

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_command_context(
+            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);

src/spec_run.rs

@@ -87,7 +87,7 @@ fn run_through(
 ) -> Result<Reached, FailedRun> {
     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)
         }

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,
         }
     }