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..9230dabe 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,7 @@ use anyhow::{Context, Result}; use chrono::{DateTime, Utc}; mod repair_loop; +mod stage; use crate::pull_request::{AlreadyMerged, Identified, MergeAttempt, PullRequest}; @@ -30,10 +30,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 +42,7 @@ use crate::trust::{self, IssueText}; use crate::worktree::{ForeignCommits, Merge, PendingMerge, Preserved, Worktree}; use repair_loop::{Repair, Upstream}; +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> { @@ -65,20 +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, +/// 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 @@ -100,11 +127,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.session.stage(), CommandLog); let route = Route { security_review: self.security_review, issue: self.issue, @@ -126,7 +149,7 @@ impl Delivery<'_> { &before_each, |sessions| { let mut outside = InWorktree { - stage: &stage, + tracker: &tracker, issue: self.issue, worktree: &worktree, base: self.base, @@ -156,18 +179,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) @@ -199,19 +219,24 @@ 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.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)) }) { // 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 +353,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 +390,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 +399,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.stop_at_security_review(); + } + 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 +507,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 +517,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 +548,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 +558,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 +636,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 +650,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 +667,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 +690,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 +701,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 +729,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 +744,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 +760,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 +801,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 +827,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 +918,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", @@ -894,13 +926,29 @@ mod tests { harness: &Choice::default(), }; let opening = Opening { - kind: if spec { "spec-review" } else { "implement" }, + session: if spec { + OpeningSession::SpecReview + } else { + OpeningSession::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.session.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 +1095,7 @@ mod tests { }; let failed = fail( &mut outside, + recovery::Stage::Implementation, Some(PathBuf::from("/logs/implement.jsonl")), anyhow!("original\nextra context"), ); @@ -1089,4 +1138,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..2e2d70ad --- /dev/null +++ b/src/delivery/stage.rs @@ -0,0 +1,222 @@ +//! 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(super) 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 { + // Entering `first` replaces this placeholder at once. + let tracker = Self { + stopped_at: Cell::new(recovery::Stage::Implementation), + 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 stop_at_security_review(&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.stop_at_security_review(); + 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..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}; +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,9 +331,8 @@ fn start_in_worktree( return outside.spec_run(tickets, worktree, base, parallel); } let opening = Opening { - kind: IMPLEMENT, + session: OpeningSession::Implementation, prompt, - catch_up_from_origin: false, }; outside.deliver(worktree, base, opening) } @@ -750,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 5d7599f6..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::{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,9 +159,8 @@ fn review_and_deliver( }); } let opening = Opening { - kind: SPEC_REVIEW, + 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); @@ -707,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")) @@ -850,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, }