From 43f4207c28b319ca79a16dc96db14db50fd2e5dd Mon Sep 17 00:00:00 2001 From: Jacob Stephens Date: Tue, 20 Oct 2026 01:42:07 -0400 Subject: [PATCH 1/2] Give Delivery one stage module that records both Recovery and the Command record Delivery's steps and the Repair loop now name each stage they enter through their Outside seam, and one table in src/delivery/stage.rs maps it to the Stopped at: stage and, where there is one, the Command record stage. The adapters stop setting stages by hand. Co-Authored-By: Claude Opus 5.5 --- docs/code-navigation.md | 2 +- src/delivery.rs | 196 +++++++++++++++++++++++--------- src/delivery/repair_loop.rs | 186 ++++++++++++++++++++++++++++-- src/delivery/stage.rs | 221 ++++++++++++++++++++++++++++++++++++ src/run.rs | 3 +- src/spec_run.rs | 3 +- 6 files changed, 547 insertions(+), 64 deletions(-) create mode 100644 src/delivery/stage.rs diff --git a/docs/code-navigation.md b/docs/code-navigation.md index b7267fee..cde2d27e 100644 --- a/docs/code-navigation.md +++ b/docs/code-navigation.md @@ -10,7 +10,7 @@ Start with the issue's terms in `GLOSSARY.md`, then use this index to find their | Worktree | `src/worktree.rs` and `src/worktree/` | `src/worktree/acquisition_tests.rs`, `src/worktree/cleanup_tests.rs` | | Spec run, Spec branch, Spec PR | `src/spec_run.rs` | `tests/spec_run.rs` | | 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` | Inline tests; `tests/merge_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` | | 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` | diff --git a/src/delivery.rs b/src/delivery.rs index 68f24137..1a3779d4 100644 --- a/src/delivery.rs +++ b/src/delivery.rs @@ -11,7 +11,6 @@ //! [`FailedOutside`]: [`InWorktree`] and [`OnFailure`] do each for real; //! `Scripted`, in tests, from a script, recording each call. -use std::cell::Cell; use std::fs; use std::path::{Path, PathBuf}; @@ -19,6 +18,9 @@ use anyhow::{Context, Result}; use chrono::{DateTime, Utc}; mod repair_loop; +mod stage; + +pub use stage::Stage; use crate::pull_request::{AlreadyMerged, Identified, MergeAttempt, PullRequest}; @@ -30,10 +32,9 @@ use crate::github::GitHub; use crate::harness::Choice; use crate::interrupt; use crate::issue::IssueUrl; -use crate::logs::{self, Stage as CommandStage}; use crate::progress; use crate::prompt; -use crate::recovery::{Preservation, Recovery, Stage}; +use crate::recovery::{self, Preservation, Recovery}; use crate::run::{Goal, Reached}; use crate::run_ending::Cause; use crate::security::review::{Hold as SecurityHold, Outcome as SecurityOutcome, Review}; @@ -43,6 +44,7 @@ use crate::trust::{self, IssueText}; use crate::worktree::{ForeignCommits, Merge, PendingMerge, Preserved, Worktree}; use repair_loop::{Repair, Upstream}; +use stage::{CommandLog, CommandRecord, Tracker}; /// A Delivery of the pull request for `issue`, the Run's issue or the Spec, /// into the Base branch `base`, to `goal`. pub struct Delivery<'a> { @@ -70,6 +72,8 @@ pub struct Delivery<'a> { pub struct Opening<'a> { /// The session's kind, in its progress lines and log name. pub kind: &'a str, + /// Its stage: [`Stage::Implementation`] or [`Stage::SpecReview`]. + pub stage: Stage, pub prompt: String, /// Whether the branch is first fast-forwarded to its head on origin, as /// the Spec branch is to the Tickets landed there. @@ -100,11 +104,7 @@ impl Delivery<'_> { pull_request: &mut PullRequest, checklist: Option<&str>, ) -> Result { - let stage = Cell::new(if opening.catch_up_from_origin { - Stage::SpecReview - } else { - Stage::Implementation - }); + let tracker = Tracker::new(opening.stage, CommandLog); let route = Route { security_review: self.security_review, issue: self.issue, @@ -126,7 +126,7 @@ impl Delivery<'_> { &before_each, |sessions| { let mut outside = InWorktree { - stage: &stage, + tracker: &tracker, issue: self.issue, worktree: &worktree, base: self.base, @@ -156,18 +156,15 @@ impl Delivery<'_> { ticket_lines: Vec::new(), }); } - if error.is::() { - stage.set(Stage::SecurityReview); - } + let stopped_at = stopped_at(&tracker, &error); let path = worktree.path().to_path_buf(); let mut outside = OnFailure { base: self.base, worktree, pull_request, }; - let mut failed = fail(&mut outside, log, error); + let mut failed = fail(&mut outside, stopped_at, log, error); if let Some(recovery) = &mut failed.recovery { - recovery.stage = stage.get(); recovery.worktree = Some(path); } Err(failed) @@ -202,16 +199,20 @@ impl Route<'_> { if opening.catch_up_from_origin { outside.catch_up()?; } + outside.stage(opening.stage); outside.session(opening.kind, &opening.prompt)?; let review = match self.security_review.then(|| { + outside.stage(Stage::SecurityReview); outside.security_review(&prompt::security_review(self.issue, self.base, self.branch)) }) { // Not a Security review outcome: the Run itself must stop. Some(Err(error)) if error.is::() => return Err(error), review => review.map(SecurityOutcome::of), }; + outside.stage(Stage::Push); outside.push()?; self.write_built_with(outside); + outside.stage(Stage::PullRequestReadiness); let pr = outside.mark_pr_ready(checklist)?; let hold = review.as_ref().and_then(SecurityOutcome::hold); if let Some(review) = &review @@ -328,8 +329,14 @@ fn retry_if_interrupted( /// head whose CI was watched. Problems along the way are reported, not /// 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 { +/// Session log, if a session created one; `stopped_at`, the stage its +/// recovery lines report. +fn fail( + outside: &mut impl FailedOutside, + stopped_at: recovery::Stage, + log: Option, + error: anyhow::Error, +) -> FailedRun { let interrupted = outside.interrupted(); let error = interrupted_or(error, interrupted); // The cause as stderr gives it, down to what a session left running, cut @@ -359,7 +366,7 @@ fn fail(outside: &mut impl FailedOutside, log: Option, error: anyhow::E } }; FailedRun { - recovery: Some(Box::new(Recovery::new(Stage::Implementation, preservation))), + recovery: Some(Box::new(Recovery::new(stopped_at, preservation))), error, pr_url, log, @@ -368,9 +375,21 @@ fn fail(outside: &mut impl FailedOutside, log: Option, error: anyhow::E } } +/// The stage a Delivery that failed with `error` stopped at, from +/// `tracker`: its Security review if the error is a Security hold, found once +/// the pull request was ready. +fn stopped_at(tracker: &Tracker, error: &anyhow::Error) -> recovery::Stage { + if error.is::() { + tracker.security_hold(); + } + tracker.stopped_at() +} + /// What a Delivery's steps do or read outside the Delivery, within its /// sessions' scope. trait Outside { + /// Enter `stage`, before the step it names. + fn stage(&mut self, stage: Stage); /// Fast-forward the Issue branch to its head on origin. fn catch_up(&mut self) -> Result<()>; /// Run the session `kind` given `prompt`. @@ -464,7 +483,7 @@ impl std::error::Error for UntrustedEdit {} /// A Delivery's worktree, its sessions, its Base fix, and its issue and pull /// request on GitHub. struct InWorktree<'a> { - stage: &'a Cell, + tracker: &'a Tracker, issue: &'a IssueUrl, worktree: &'a Worktree, base: &'a str, @@ -474,23 +493,20 @@ struct InWorktree<'a> { } impl Outside for InWorktree<'_> { + fn stage(&mut self, stage: Stage) { + self.tracker.enter(stage); + } + fn catch_up(&mut self) -> Result<()> { self.worktree.fast_forward_to_origin() } fn session(&mut self, kind: &str, prompt: &str) -> Result<()> { - self.stage.set(if kind == "spec-review" { - Stage::SpecReview - } else { - Stage::Implementation - }); self.sessions.run(kind, prompt)?; self.pull_request.ensure_actionable(false) } fn security_review(&mut self, prompt: &str) -> Result { - self.stage.set(Stage::SecurityReview); - logs::stage(CommandStage::SecurityReview); self.pull_request.ensure_actionable(false)?; let review = crate::security::review::run( self.sessions, @@ -508,8 +524,6 @@ impl Outside for InWorktree<'_> { } fn push(&mut self) -> Result<()> { - self.stage.set(Stage::Push); - logs::stage(CommandStage::PullRequest); self.pull_request.ensure_actionable(false)?; self.worktree .push_if_actionable(&mut || self.pull_request.ensure_actionable(false)) @@ -520,15 +534,12 @@ impl Outside for InWorktree<'_> { } fn mark_pr_ready(&mut self, checklist: Option<&str>) -> Result { - self.stage.set(Stage::PullRequest); - logs::stage(CommandStage::PullRequest); self.pull_request.mark_ready(checklist) } fn take_to_goal(&mut self, pr_url: &str, goal: Goal) -> Result<()> { - self.stage.set(Stage::Repair); let mut outside = RunOutside { - stage: self.stage, + tracker: self.tracker, issue: self.issue, worktree: self.worktree, base: self.base, @@ -601,7 +612,7 @@ impl FailedOutside for OnFailure<'_> { /// What a Run's Repair loop reaches outside itself: the Run's worktree, its /// Repair sessions, its Base fix, its issue and its pull request. struct RunOutside<'a> { - stage: &'a Cell, + tracker: &'a Tracker, issue: &'a IssueUrl, worktree: &'a Worktree, base: &'a str, @@ -615,12 +626,7 @@ impl repair_loop::Outside for RunOutside<'_> { type Pending = PendingMerge; fn stage(&mut self, stage: Stage) { - self.stage.set(stage); - match stage { - Stage::SelfMerge => logs::stage(CommandStage::SelfMerge), - Stage::PullRequest => logs::stage(CommandStage::PullRequest), - _ => {} - } + self.tracker.enter(stage); } fn merge_base_branch(&mut self) -> Result { @@ -637,8 +643,6 @@ impl repair_loop::Outside for RunOutside<'_> { } fn push(&mut self) -> Result<()> { - self.stage.set(Stage::Push); - logs::stage(CommandStage::PullRequest); self.pull_request.ensure_actionable(true)?; self.worktree .push_if_actionable(&mut || self.pull_request.ensure_actionable(true)) @@ -662,8 +666,6 @@ impl repair_loop::Outside for RunOutside<'_> { } fn watch(&mut self, head: &str, base_commit: Option<&str>) -> Result { - self.stage.set(Stage::Ci); - logs::stage(CommandStage::CiWatch); ci::watch(self.issue, head, base_commit, &mut || { self.pull_request.ensure_actionable(true) }) @@ -675,16 +677,12 @@ impl repair_loop::Outside for RunOutside<'_> { base_commit: Option<&str>, failed: &FailedChecks, ) -> Result> { - self.stage.set(Stage::Ci); - logs::stage(CommandStage::CiWatch); ci::rerun(self.issue, head, base_commit, failed, &mut || { self.pull_request.ensure_actionable(true) }) } fn repair(&mut self, kind: &str, repair: Repair) -> Result<()> { - self.stage.set(Stage::Repair); - logs::stage(CommandStage::Repair); let (issue, base, pr_url) = (self.issue, self.base, self.pr_url); let branch = self.worktree.branch(); let prompt = match repair { @@ -707,7 +705,6 @@ impl repair_loop::Outside for RunOutside<'_> { } fn base_fix(&mut self, base_commit: &str, failed: &FailedChecks) -> Result<()> { - self.stage.set(Stage::BaseFix); self.base_fix.fix( self.worktree.launch(), self.issue, @@ -723,8 +720,6 @@ impl repair_loop::Outside for RunOutside<'_> { } fn merge(&mut self, head: &str) -> Result { - self.stage.set(Stage::SelfMerge); - logs::stage(CommandStage::SelfMerge); self.pull_request.merge(head) } @@ -741,6 +736,7 @@ impl repair_loop::Outside for RunOutside<'_> { mod tests { use super::*; use anyhow::{anyhow, bail}; + use stage::Recorded; const PR_URL: &str = "https://github.com/acme/widgets/pull/12"; @@ -781,6 +777,12 @@ mod tests { #[derive(Default)] struct Scripted { calls: Vec, + /// The stages entered, apart from the calls. + stages: Vec, + /// Whether the Delivery runs a Security review, and whether that + /// review ends incomplete, a Security hold. + reviews_security: bool, + security_held: bool, failing: Option, interrupted: bool, refused: bool, @@ -801,7 +803,13 @@ mod tests { fn record_security_review(&mut self, _: &SecurityOutcome) -> Result<()> { Ok(()) } + fn stage(&mut self, stage: Stage) { + self.stages.push(stage); + } fn security_review(&mut self, _: &str) -> Result { + if self.security_held { + return Review::from_final_message(Some("Security review: incomplete: timed out")); + } Review::from_final_message(Some( "Security review: {\"unaddressed_count\":0,\"findings\":[],\"pre_existing_count\":0}", )) @@ -886,7 +894,7 @@ mod tests { fn deliver(outside: &mut Scripted, goal: Goal, spec: bool) -> Result { let issue = IssueUrl::parse("https://github.com/acme/widgets/issues/7").unwrap(); let route = Route { - security_review: false, + security_review: outside.reviews_security, issue: &issue, base: "main", branch: "issue-7", @@ -895,12 +903,30 @@ mod tests { }; let opening = Opening { kind: if spec { "spec-review" } else { "implement" }, + stage: if spec { + Stage::SpecReview + } else { + Stage::Implementation + }, prompt: String::new(), catch_up_from_origin: spec, }; route .steps(outside, &opening, spec.then_some("Tickets")) - .map_err(|error| fail(outside, outside.log.clone(), error)) + .map_err(|error| { + // The real adapters enter each stage the steps name. + let tracker = Tracker::new(opening.stage, Recorded::default()); + for stage in &outside.stages { + tracker.enter(*stage); + } + let stopped_at = stopped_at(&tracker, &error); + fail(outside, stopped_at, outside.log.clone(), error) + }) + } + + /// The stage `failed` stopped at. + fn stopped(failed: &FailedRun) -> recovery::Stage { + failed.recovery.as_ref().expect("no recovery").stage } #[test] @@ -1047,6 +1073,7 @@ mod tests { }; let failed = fail( &mut outside, + recovery::Stage::Implementation, Some(PathBuf::from("/logs/implement.jsonl")), anyhow!("original\nextra context"), ); @@ -1089,4 +1116,71 @@ mod tests { .any(|call| matches!(call, Call::Close(_))) ); } + + #[test] + fn a_delivery_enters_each_stage_before_its_step() { + let mut outside = Scripted { + reviews_security: true, + ..Default::default() + }; + assert!(deliver(&mut outside, Goal::ReadyForReview, false).is_ok()); + assert_eq!( + outside.stages, + [ + Stage::Implementation, + Stage::SecurityReview, + Stage::Push, + Stage::PullRequestReadiness + ] + ); + } + + #[test] + fn a_spec_delivery_opens_in_its_spec_review() { + for failing in [Fails::CatchUp, Fails::Session] { + let mut outside = Scripted { + failing: Some(failing), + ..Default::default() + }; + let failed = deliver(&mut outside, Goal::ReadyForReview, true).unwrap_err(); + assert_eq!(stopped(&failed), recovery::Stage::SpecReview, "{failing:?}"); + } + } + + #[test] + fn each_failed_step_stops_at_its_stage() { + for (failing, stage) in [ + (Fails::Session, recovery::Stage::Implementation), + (Fails::Push, recovery::Stage::Push), + (Fails::Ready, recovery::Stage::PullRequest), + ] { + let mut outside = Scripted { + failing: Some(failing), + ..Default::default() + }; + let failed = deliver(&mut outside, Goal::ReadyForReview, false).unwrap_err(); + assert_eq!(stopped(&failed), stage, "{failing:?}"); + } + } + + #[test] + fn a_security_hold_stops_at_the_security_review_without_entering_it_again() { + let mut outside = Scripted { + reviews_security: true, + security_held: true, + ..Default::default() + }; + let failed = deliver(&mut outside, Goal::Merged, false).unwrap_err(); + assert!(failed.error.is::(), "{:#}", failed.error); + assert_eq!(stopped(&failed), recovery::Stage::SecurityReview); + assert_eq!( + outside.stages, + [ + Stage::Implementation, + Stage::SecurityReview, + Stage::Push, + Stage::PullRequestReadiness + ] + ); + } } diff --git a/src/delivery/repair_loop.rs b/src/delivery/repair_loop.rs index 2554feae..aa8320bd 100644 --- a/src/delivery/repair_loop.rs +++ b/src/delivery/repair_loop.rs @@ -13,10 +13,11 @@ use crate::pull_request::MergeAttempt; use crate::ci::{self, Ci, FailedChecks}; use crate::failed_run::PolicyRefusal; -use crate::recovery::Stage; use crate::run::Goal; use crate::worktree::{ForeignCommits, Merge}; +use super::Stage; + /// Which upstream a conflict Repair resolves a merge of. #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub(super) enum Upstream { @@ -42,7 +43,7 @@ pub(super) trait Outside { /// A merge left in progress for a conflict Repair. type Pending; - /// Record the lifecycle stage before entering an operation or its budget gate. + /// Enter `stage`, before the operation or budget gate it names. fn stage(&mut self, stage: Stage); /// Merge the Base branch, fetched from origin, into the Issue branch, @@ -123,7 +124,7 @@ pub(super) fn take_to_goal(outside: &mut impl Outside, base: &str, goal: Goal) - outside.stage(if goal == Goal::Merged { Stage::SelfMerge } else { - Stage::PullRequest + Stage::PullRequestReadiness }); outside.ensure_pr_ready_and_mergeable()?; if outside.interrupt_requested() { @@ -133,6 +134,7 @@ pub(super) fn take_to_goal(outside: &mut impl Outside, base: &str, goal: Goal) - return Ok(()); } outside.progress(format!("merging the PR into {base}")); + outside.stage(Stage::SelfMerge); let error = match outside.merge(&watched)? { MergeAttempt::Merged => return Ok(()), MergeAttempt::Refused(error) => error, @@ -246,6 +248,7 @@ impl CheckReruns { .handed_to_repair .take() .filter(|(handed, _)| handed == head); + outside.stage(Stage::CiWatch); let ci = match unchanged { Some((_, failed)) if self.consumed.insert(head.to_string()) => { outside.rerun(head, compared_with, &failed)? @@ -275,9 +278,11 @@ impl RepairLoop<'_, O> { /// Start the next Repair, `repair`, for `cause`, counting it against the /// Repair cap. fn repair(&mut self, cause: &str, repair: Repair) -> Result<()> { - self.outside.stage(Stage::Repair); + // Exhausted repairs start no session, so record none. + self.outside.stage(Stage::RepairLoopRound); let (kind, line) = self.budgets.next_repair(cause)?; self.outside.progress(line); + self.outside.stage(Stage::RepairSession); self.outside.repair(&kind, repair) } @@ -316,7 +321,7 @@ impl RepairLoop<'_, O> { fn rounds(&mut self) -> Result { let base = self.base; loop { - self.outside.stage(Stage::Repair); + self.outside.stage(Stage::RepairLoopRound); if self.goal == Goal::Merged { self.take_in_foreign_commits()?; } @@ -328,6 +333,7 @@ impl RepairLoop<'_, O> { continue; } }; + self.outside.stage(Stage::Push); self.outside.push()?; let head = self.outside.head()?; let compared_with = self @@ -361,6 +367,7 @@ impl RepairLoop<'_, O> { self.count_base_move("while CI ran")?; continue; } + self.outside.stage(Stage::BaseFix); self.outside.base_fix(&base_commit, &failed)?; continue; } @@ -412,7 +419,7 @@ impl RepairLoop<'_, O> { /// branch that moved since as an upstream move. GitHub's error text is never /// consulted. fn round_after_failed_merge(&mut self, watched: &str) -> Result { - self.outside.stage(Stage::Repair); + self.outside.stage(Stage::RepairLoopRound); let before = self.budgets; if self.outside.base_branch_moved()? { self.count_base_move("since the merge was tried")?; @@ -531,15 +538,22 @@ mod tests { /// For each merge, the error it fails with, if it does. merges: VecDeque>, merge_gate_error: Option<&'static str>, + /// The error a merge of the Base branch fails with, if it does. + base_merge_error: Option<&'static str>, + /// The error a watch of CI fails with, if it does. + watch_error: Option<&'static str>, + /// The error a Repair session fails with, if it does. + repair_error: Option<&'static str>, sees_no_inherited_failures: bool, interrupted: bool, } /// The outside world as `script` answers it, with what the loop did there - /// recorded in `did`. + /// recorded in `did`, and the stages it entered, apart, in `stages`. struct Scripted { script: Script, did: Vec, + stages: Vec, } /// The next of `answers`, keeping the last one for every call after it. @@ -563,10 +577,15 @@ mod tests { /// The upstream whose merge is pending. type Pending = &'static str; - fn stage(&mut self, _: Stage) {} + fn stage(&mut self, stage: Stage) { + self.stages.push(stage); + } fn merge_base_branch(&mut self) -> Result> { self.did.push(Did::MergeBase); + if let Some(error) = self.script.base_merge_error { + bail!(error); + } let conflicted = self.script.base_conflicts.pop_front().unwrap_or(false); if conflicted { return Ok(Merge::Conflicted("origin/main")); @@ -636,6 +655,9 @@ mod tests { fn watch(&mut self, head: &str, base_commit: Option<&str>) -> Result { self.did .push(Did::Watch(head.to_string(), base_commit.map(String::from))); + if let Some(error) = self.script.watch_error { + bail!(error); + } let is = self.script.watches.pop_front().expect("no watch scripted"); Ok(is.ci()) } @@ -664,7 +686,10 @@ mod tests { Repair::Review { from } => Started::Review(from.to_string()), }; self.did.push(Did::Repair(kind.to_string(), started)); - Ok(()) + match self.script.repair_error { + Some(error) => bail!(error), + None => Ok(()), + } } fn sees_inherited_failures(&mut self) -> bool { @@ -710,12 +735,27 @@ mod tests { /// Take a PR into `main` to `goal` against `script`: what it came to, /// and what the loop did. fn deliver(goal: Goal, script: Script) -> (Result<()>, Vec) { + let (delivered, did, _) = deliver_with_stages(goal, script); + (delivered, did) + } + + /// [`deliver`], with the stages the loop entered, in order. + fn deliver_with_stages(goal: Goal, script: Script) -> (Result<()>, Vec, Vec) { let mut outside = Scripted { script, did: Vec::new(), + stages: Vec::new(), }; let delivered = take_to_goal(&mut outside, "main", goal); - (delivered, outside.did) + (delivered, outside.did, outside.stages) + } + + /// The stage in force when a Delivery to `goal` against `script` failed, + /// and its cause. + fn failed_in(goal: Goal, script: Script) -> (Stage, String) { + let (delivered, _, stages) = deliver_with_stages(goal, script); + let stage = *stages.last().expect("no stage entered"); + (stage, cause(delivered)) } /// `answers` as a script's queue. @@ -2061,4 +2101,130 @@ mod tests { } ); } + + #[test] + fn a_failed_merge_of_the_base_branch_stops_in_a_repair_loop_round() { + assert_eq!( + failed_in( + Goal::ReadyForReview, + Script { + base_merge_error: Some("could not fetch origin/main"), + ..Script::default() + }, + ), + ( + Stage::RepairLoopRound, + "could not fetch origin/main".to_string() + ) + ); + } + + #[test] + fn a_failed_ci_watch_stops_in_the_ci_watch() { + assert_eq!( + failed_in( + Goal::ReadyForReview, + Script { + heads: script(["h1"]), + watch_error: Some("gh run list failed"), + ..Script::default() + }, + ), + (Stage::CiWatch, "gh run list failed".to_string()) + ); + } + + #[test] + fn a_failed_repair_session_stops_in_the_repair_session() { + assert_eq!( + failed_in( + Goal::ReadyForReview, + Script { + base_conflicts: script([true]), + repair_error: Some("the repair-1 session failed"), + ..Script::default() + }, + ), + ( + Stage::RepairSession, + "the repair-1 session failed".to_string() + ) + ); + } + + #[test] + fn exhausted_repairs_stop_in_a_repair_loop_round_without_a_session() { + let (delivered, _, stages) = deliver_with_stages( + Goal::ReadyForReview, + Script { + base_conflicts: script([true; MAX_REPAIRS + 1]), + ..Script::default() + }, + ); + assert!(cause(delivered).starts_with("repairs exhausted")); + assert_eq!(stages.last(), Some(&Stage::RepairLoopRound)); + assert_eq!( + stages + .iter() + .filter(|stage| **stage == Stage::RepairSession) + .count(), + MAX_REPAIRS + ); + } + + #[test] + fn a_refused_self_merge_stops_in_the_self_merge() { + let (delivered, _, stages) = deliver_with_stages( + Goal::Merged, + Script { + heads: script(["h1"]), + watches: script([Is::Passed, Is::Passed]), + merges: script([Some("refused by a ruleset")]), + ..Script::default() + }, + ); + assert!(delivered.unwrap_err().is::()); + assert_eq!( + stages, + [ + Stage::RepairLoopRound, + Stage::Push, + Stage::CiWatch, + Stage::SelfMerge, + Stage::SelfMerge, + Stage::RepairLoopRound, + Stage::RepairLoopRound, + Stage::Push, + Stage::CiWatch, + Stage::SelfMerge, + ] + ); + } + + #[test] + fn a_base_fix_and_a_ready_pull_request_enter_their_stages() { + let (delivered, _, stages) = deliver_with_stages( + Goal::ReadyForReview, + Script { + heads: script(["h1"]), + watches: script([Is::Failed(&[], &["lint"]), Is::Passed]), + base_fixes: script([None]), + ..Script::default() + }, + ); + assert!(delivered.is_ok()); + assert_eq!( + stages, + [ + Stage::RepairLoopRound, + Stage::Push, + Stage::CiWatch, + Stage::BaseFix, + Stage::RepairLoopRound, + Stage::Push, + Stage::CiWatch, + Stage::PullRequestReadiness, + ] + ); + } } diff --git a/src/delivery/stage.rs b/src/delivery/stage.rs new file mode 100644 index 00000000..a7dcde2e --- /dev/null +++ b/src/delivery/stage.rs @@ -0,0 +1,221 @@ +//! A Delivery's stage: the one table from each stage its steps and the +//! Repair loop enter to the stage a Failed run's recovery lines report as +//! `Stopped at:`, and to the Command record stage `thirdshift status` and the +//! Console show, and the [`Tracker`] that holds the stage in force. + +use std::cell::Cell; + +use crate::logs::{self, Stage as CommandStage}; +use crate::recovery; + +/// A stage of a Delivery, as its steps and the Repair loop enter it. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum Stage { + Implementation, + SpecReview, + SecurityReview, + Push, + PullRequestReadiness, + /// A round of the Repair loop: merging the Base branch or Foreign + /// commits in, before any Repair session. + RepairLoopRound, + RepairSession, + CiWatch, + BaseFix, + SelfMerge, +} + +impl Stage { + /// The stage a Failed run's recovery lines report, and the Command + /// record stage, if the Delivery records one. Implementation and Spec + /// review have none: their Session log names its stage. + fn records(self) -> (recovery::Stage, Option) { + match self { + Self::Implementation => (recovery::Stage::Implementation, None), + Self::SpecReview => (recovery::Stage::SpecReview, None), + Self::SecurityReview => ( + recovery::Stage::SecurityReview, + Some(CommandStage::SecurityReview), + ), + Self::Push => (recovery::Stage::Push, Some(CommandStage::PullRequest)), + Self::PullRequestReadiness => ( + recovery::Stage::PullRequest, + Some(CommandStage::PullRequest), + ), + Self::RepairLoopRound => (recovery::Stage::Repair, None), + Self::RepairSession => (recovery::Stage::Repair, Some(CommandStage::Repair)), + Self::CiWatch => (recovery::Stage::Ci, Some(CommandStage::CiWatch)), + Self::BaseFix => (recovery::Stage::BaseFix, None), + Self::SelfMerge => (recovery::Stage::SelfMerge, Some(CommandStage::SelfMerge)), + } + } +} + +/// Where a Delivery's Command record stages go. +pub(super) trait CommandRecord { + /// Record `stage`; the same stage recorded twice in a row is one. + fn stage(&self, stage: CommandStage); +} + +/// The command's own Command record. +pub(super) struct CommandLog; + +impl CommandRecord for CommandLog { + fn stage(&self, stage: CommandStage) { + logs::stage(stage); + } +} + +/// The stage a Delivery is in, which its Failed run path reports. +pub(super) struct Tracker { + stopped_at: Cell, + record: R, +} + +impl Tracker { + /// A Tracker that has entered `first`, recording to `record`. + pub(super) fn new(first: Stage, record: R) -> Self { + let tracker = Self { + stopped_at: Cell::new(first.records().0), + record, + }; + tracker.enter(first); + tracker + } + + /// Enter `stage`: it is what a failure from now on stopped at, and its + /// Command record stage, if it has one, is recorded. + pub(super) fn enter(&self, stage: Stage) { + let (stopped_at, recorded) = stage.records(); + self.stopped_at.set(stopped_at); + if let Some(recorded) = recorded { + self.record.stage(recorded); + } + } + + /// A Security hold, found after the pull request was made ready, stopped + /// the Delivery at its Security review. Nothing is recorded: the + /// Security review was over before the stages since. + pub(super) fn security_hold(&self) { + self.stopped_at.set(recovery::Stage::SecurityReview); + } + + /// The stage a failure now stopped at. + pub(super) fn stopped_at(&self) -> recovery::Stage { + self.stopped_at.get() + } +} + +/// An in-memory Command record, for tests: the names of the stages +/// recorded, in order, a repeated one collapsed as the Command record does. +#[cfg(test)] +#[derive(Default)] +pub(super) struct Recorded(std::cell::RefCell>); + +#[cfg(test)] +impl Recorded { + pub(super) fn names(&self) -> Vec<&'static str> { + self.0.borrow().clone() + } +} + +#[cfg(test)] +impl CommandRecord for Recorded { + fn stage(&self, stage: CommandStage) { + let mut names = self.0.borrow_mut(); + if names.last() != Some(&stage.name()) { + names.push(stage.name()); + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn entering_each_stage_sets_its_recovery_stage_and_records_its_command_record_stage() { + let table = [ + (Stage::Implementation, recovery::Stage::Implementation, None), + (Stage::SpecReview, recovery::Stage::SpecReview, None), + ( + Stage::SecurityReview, + recovery::Stage::SecurityReview, + Some("Security review"), + ), + (Stage::Push, recovery::Stage::Push, Some("pull request")), + ( + Stage::PullRequestReadiness, + recovery::Stage::PullRequest, + Some("pull request"), + ), + (Stage::RepairLoopRound, recovery::Stage::Repair, None), + ( + Stage::RepairSession, + recovery::Stage::Repair, + Some("Repair"), + ), + (Stage::CiWatch, recovery::Stage::Ci, Some("CI watch")), + (Stage::BaseFix, recovery::Stage::BaseFix, None), + ( + Stage::SelfMerge, + recovery::Stage::SelfMerge, + Some("Self-merge"), + ), + ]; + for (stage, stopped_at, recorded) in table { + let tracker = Tracker::new(Stage::Implementation, Recorded::default()); + tracker.enter(stage); + assert_eq!(tracker.stopped_at(), stopped_at, "{stage:?}"); + assert_eq!( + tracker.record.names(), + Vec::from_iter(recorded), + "{stage:?}" + ); + } + } + + #[test] + fn a_stage_entered_again_is_recorded_once() { + let tracker = Tracker::new(Stage::Implementation, Recorded::default()); + for stage in [ + Stage::Push, + Stage::PullRequestReadiness, + Stage::RepairLoopRound, + Stage::Push, + Stage::CiWatch, + Stage::CiWatch, + ] { + tracker.enter(stage); + } + assert_eq!(tracker.record.names(), ["pull request", "CI watch"]); + assert_eq!(tracker.stopped_at(), recovery::Stage::Ci); + } + + #[test] + fn a_security_hold_stops_at_the_security_review_without_recording_it_again() { + let tracker = Tracker::new(Stage::Implementation, Recorded::default()); + for stage in [ + Stage::SecurityReview, + Stage::Push, + Stage::PullRequestReadiness, + Stage::RepairLoopRound, + Stage::CiWatch, + ] { + tracker.enter(stage); + } + tracker.security_hold(); + assert_eq!(tracker.stopped_at(), recovery::Stage::SecurityReview); + assert_eq!( + tracker.record.names(), + ["Security review", "pull request", "CI watch"] + ); + } + + #[test] + fn a_new_tracker_has_entered_its_first_stage() { + let tracker = Tracker::new(Stage::SpecReview, Recorded::default()); + assert_eq!(tracker.stopped_at(), recovery::Stage::SpecReview); + assert!(tracker.record.names().is_empty()); + } +} diff --git a/src/run.rs b/src/run.rs index a3073095..795ec869 100644 --- a/src/run.rs +++ b/src/run.rs @@ -12,7 +12,7 @@ use crate::base_fix::{Advice, BaseFix}; use crate::branch::{self, Selection}; use crate::child_run::Kind; use crate::claim::{self, Claim}; -use crate::delivery::{Delivery, Opening}; +use crate::delivery::{Delivery, Opening, Stage}; use crate::failed_run::FailedRun; use crate::github::{GitHub, Ticket}; use crate::harness::Choice; @@ -335,6 +335,7 @@ fn start_in_worktree( } let opening = Opening { kind: IMPLEMENT, + stage: Stage::Implementation, prompt, catch_up_from_origin: false, }; diff --git a/src/spec_run.rs b/src/spec_run.rs index 5d7599f6..989db7c3 100644 --- a/src/spec_run.rs +++ b/src/spec_run.rs @@ -17,7 +17,7 @@ use anyhow::{Result, bail}; use crate::base_fix::BaseFixAsk; use crate::child_run::{Ended, Kind, Runs}; -use crate::delivery::{Delivery, Opening}; +use crate::delivery::{self, Delivery, Opening}; use crate::failed_run::{FailedRun, interrupted_or}; use crate::github::{GitHub, Ticket}; use crate::harness::Choice; @@ -163,6 +163,7 @@ fn review_and_deliver( } let opening = Opening { kind: SPEC_REVIEW, + stage: delivery::Stage::SpecReview, prompt: prompt::spec_review(spec, base, branch, spec_pr_url), catch_up_from_origin: true, }; From 820774ef2adabfe3b86d35307b633b859f266f8f Mon Sep 17 00:00:00 2001 From: Jacob Stephens Date: Tue, 20 Oct 2026 02:22:56 -0400 Subject: [PATCH 2/2] Open a Delivery from one fact, its OpeningSession Address the review: the opening's kind, its catch-up from origin and its stage all follow from OpeningSession, the Tracker reads its table once, and the Security hold's tracker method says what it does. Co-Authored-By: Claude Opus 5.5 --- src/delivery.rs | 68 ++++++++++++++++++++++++++++--------------- src/delivery/stage.rs | 9 +++--- src/run.rs | 12 ++------ src/spec_run.rs | 15 ++++------ 4 files changed, 58 insertions(+), 46 deletions(-) diff --git a/src/delivery.rs b/src/delivery.rs index 1a3779d4..9230dabe 100644 --- a/src/delivery.rs +++ b/src/delivery.rs @@ -20,8 +20,6 @@ use chrono::{DateTime, Utc}; mod repair_loop; mod stage; -pub use stage::Stage; - use crate::pull_request::{AlreadyMerged, Identified, MergeAttempt, PullRequest}; use crate::base_fix::BaseFix; @@ -44,7 +42,7 @@ use crate::trust::{self, IssueText}; use crate::worktree::{ForeignCommits, Merge, PendingMerge, Preserved, Worktree}; use repair_loop::{Repair, Upstream}; -use stage::{CommandLog, CommandRecord, Tracker}; +use stage::{CommandLog, CommandRecord, Stage, Tracker}; /// A Delivery of the pull request for `issue`, the Run's issue or the Spec, /// into the Base branch `base`, to `goal`. pub struct Delivery<'a> { @@ -67,22 +65,47 @@ pub struct Delivery<'a> { pub started: DateTime, } -/// The session a Delivery opens with: the implement session, or the Spec -/// review. -pub struct Opening<'a> { - /// The session's kind, in its progress lines and log name. - pub kind: &'a str, - /// Its stage: [`Stage::Implementation`] or [`Stage::SpecReview`]. - pub stage: Stage, +/// The session a Delivery opens with, given its prompt. +pub struct Opening { + pub session: OpeningSession, pub prompt: String, +} + +/// Which session a Delivery opens with. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum OpeningSession { + /// A Run's implement session. + Implementation, + /// A Spec run's Spec review, once its Tickets have landed. + SpecReview, +} + +impl OpeningSession { + /// The session's kind, in its progress lines and log name. + pub fn kind(self) -> &'static str { + match self { + Self::Implementation => "implement", + Self::SpecReview => "spec-review", + } + } + /// Whether the branch is first fast-forwarded to its head on origin, as /// the Spec branch is to the Tickets landed there. - pub catch_up_from_origin: bool, + pub fn catches_up_from_origin(self) -> bool { + self == Self::SpecReview + } + + fn stage(self) -> Stage { + match self { + Self::Implementation => Stage::Implementation, + Self::SpecReview => Stage::SpecReview, + } + } } impl Delivery<'_> { /// Take the pull request from the branch checked out in `worktree` to the - /// goal. With `opening.catch_up_from_origin`, first fast-forward the + /// goal. For a Spec review opening, first fast-forward the /// branch to origin. Then run the opening session and optional Security /// review, push the branch, for /// any commit the session left unpushed, write the line that says what it @@ -104,7 +127,7 @@ impl Delivery<'_> { pull_request: &mut PullRequest, checklist: Option<&str>, ) -> Result { - let tracker = Tracker::new(opening.stage, CommandLog); + let tracker = Tracker::new(opening.session.stage(), CommandLog); let route = Route { security_review: self.security_review, issue: self.issue, @@ -196,11 +219,12 @@ impl Route<'_> { opening: &Opening, checklist: Option<&str>, ) -> Result { - if opening.catch_up_from_origin { + let session = opening.session; + if session.catches_up_from_origin() { outside.catch_up()?; } - outside.stage(opening.stage); - outside.session(opening.kind, &opening.prompt)?; + outside.stage(session.stage()); + outside.session(session.kind(), &opening.prompt)?; let review = match self.security_review.then(|| { outside.stage(Stage::SecurityReview); outside.security_review(&prompt::security_review(self.issue, self.base, self.branch)) @@ -380,7 +404,7 @@ fn fail( /// the pull request was ready. fn stopped_at(tracker: &Tracker, error: &anyhow::Error) -> recovery::Stage { if error.is::() { - tracker.security_hold(); + tracker.stop_at_security_review(); } tracker.stopped_at() } @@ -902,20 +926,18 @@ mod tests { harness: &Choice::default(), }; let opening = Opening { - kind: if spec { "spec-review" } else { "implement" }, - stage: if spec { - Stage::SpecReview + session: if spec { + OpeningSession::SpecReview } else { - Stage::Implementation + OpeningSession::Implementation }, prompt: String::new(), - catch_up_from_origin: spec, }; route .steps(outside, &opening, spec.then_some("Tickets")) .map_err(|error| { // The real adapters enter each stage the steps name. - let tracker = Tracker::new(opening.stage, Recorded::default()); + let tracker = Tracker::new(opening.session.stage(), Recorded::default()); for stage in &outside.stages { tracker.enter(*stage); } diff --git a/src/delivery/stage.rs b/src/delivery/stage.rs index a7dcde2e..2e2d70ad 100644 --- a/src/delivery/stage.rs +++ b/src/delivery/stage.rs @@ -10,7 +10,7 @@ use crate::recovery; /// A stage of a Delivery, as its steps and the Repair loop enter it. #[derive(Debug, Clone, Copy, PartialEq, Eq)] -pub enum Stage { +pub(super) enum Stage { Implementation, SpecReview, SecurityReview, @@ -75,8 +75,9 @@ pub(super) struct Tracker { impl Tracker { /// A Tracker that has entered `first`, recording to `record`. pub(super) fn new(first: Stage, record: R) -> Self { + // Entering `first` replaces this placeholder at once. let tracker = Self { - stopped_at: Cell::new(first.records().0), + stopped_at: Cell::new(recovery::Stage::Implementation), record, }; tracker.enter(first); @@ -96,7 +97,7 @@ impl Tracker { /// A Security hold, found after the pull request was made ready, stopped /// the Delivery at its Security review. Nothing is recorded: the /// Security review was over before the stages since. - pub(super) fn security_hold(&self) { + pub(super) fn stop_at_security_review(&self) { self.stopped_at.set(recovery::Stage::SecurityReview); } @@ -204,7 +205,7 @@ mod tests { ] { tracker.enter(stage); } - tracker.security_hold(); + tracker.stop_at_security_review(); assert_eq!(tracker.stopped_at(), recovery::Stage::SecurityReview); assert_eq!( tracker.record.names(), diff --git a/src/run.rs b/src/run.rs index 795ec869..6b5bf733 100644 --- a/src/run.rs +++ b/src/run.rs @@ -12,7 +12,7 @@ use crate::base_fix::{Advice, BaseFix}; use crate::branch::{self, Selection}; use crate::child_run::Kind; use crate::claim::{self, Claim}; -use crate::delivery::{Delivery, Opening, Stage}; +use crate::delivery::{Delivery, Opening, OpeningSession}; use crate::failed_run::FailedRun; use crate::github::{GitHub, Ticket}; use crate::harness::Choice; @@ -27,9 +27,6 @@ use crate::spec_run; use crate::trust; use crate::worktree::Worktree; -/// The implement session's kind, in its progress lines and log name. -const IMPLEMENT: &str = "implement"; - /// Where a Run takes its PR: ready for review, or, in a Merge run, merged. #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub enum Goal { @@ -334,10 +331,8 @@ fn start_in_worktree( return outside.spec_run(tickets, worktree, base, parallel); } let opening = Opening { - kind: IMPLEMENT, - stage: Stage::Implementation, + session: OpeningSession::Implementation, prompt, - catch_up_from_origin: false, }; outside.deliver(worktree, base, opening) } @@ -751,8 +746,7 @@ mod tests { base: &str, opening: Opening, ) -> Result { - assert_eq!(opening.kind, IMPLEMENT); - assert!(!opening.catch_up_from_origin); + assert_eq!(opening.session, OpeningSession::Implementation); self.calls .push(Call::Deliver(worktree, base.to_string(), opening.prompt)); self.work_ends() diff --git a/src/spec_run.rs b/src/spec_run.rs index 989db7c3..4999318d 100644 --- a/src/spec_run.rs +++ b/src/spec_run.rs @@ -17,7 +17,7 @@ use anyhow::{Result, bail}; use crate::base_fix::BaseFixAsk; use crate::child_run::{Ended, Kind, Runs}; -use crate::delivery::{self, Delivery, Opening}; +use crate::delivery::{Delivery, Opening, OpeningSession}; use crate::failed_run::{FailedRun, interrupted_or}; use crate::github::{GitHub, Ticket}; use crate::harness::Choice; @@ -33,9 +33,6 @@ use crate::worktree::Worktree; use ticket_board::TicketBoard; -/// The Spec review session's kind, in its progress lines and log name. -const SPEC_REVIEW: &str = "spec-review"; - /// Whether an issue has Tickets, as a Spec has, and every one is closed: /// `open` says of each of its sub-issues whether it is open. pub fn all_closed(open: impl IntoIterator) -> bool { @@ -162,10 +159,8 @@ fn review_and_deliver( }); } let opening = Opening { - kind: SPEC_REVIEW, - stage: delivery::Stage::SpecReview, + session: OpeningSession::SpecReview, prompt: prompt::spec_review(spec, base, branch, spec_pr_url), - catch_up_from_origin: true, }; // The Spec review may have rewritten the body without the checklist. let delivered = outside.deliver(opening, checklist); @@ -708,9 +703,9 @@ mod tests { fn deliver(&mut self, opening: Opening, checklist: &str) -> Result { self.did.push(Did::Deliver { - kind: opening.kind.to_string(), + kind: opening.session.kind().to_string(), + catch_up: opening.session.catches_up_from_origin(), prompt: opening.prompt, - catch_up: opening.catch_up_from_origin, }); let delivered = if self.script.review_fails { Err(anyhow!("the spec-review session failed")) @@ -851,7 +846,7 @@ mod tests { fn spec_review_opening() -> Did { let spec = IssueUrl::parse("https://github.com/acme/widgets/issues/20").unwrap(); Did::Deliver { - kind: SPEC_REVIEW.to_string(), + kind: "spec-review".to_string(), prompt: prompt::spec_review(&spec, "main", "issue-20", SPEC_PR), catch_up: true, }