diff --git a/src/architect.rs b/src/architect.rs index 3ca647572..fade0d3b9 100644 --- a/src/architect.rs +++ b/src/architect.rs @@ -1,10 +1,11 @@ -//! An Architect run up to its plan: an Architecture review of the Base -//! branch, the one its command named or else the one checked out in the -//! Launch directory, from the Launch directory with no Issue URL, then the -//! checks on the Architect plan it published and the label change that marks -//! it ready and labels it `architect-plan`, for the command to stop at or to -//! dispatch. A review that found no Strong candidate published no plan, and -//! the Architect run ends on the issue it named instead, labelled an +//! An Architect run: an Architecture review of the Base branch, the one its +//! command named or else the one checked out in the Launch directory, from +//! the Launch directory with no Issue URL, then the checks on the Architect +//! plan it published and the label change that marks it ready and labels it +//! `architect-plan`, then, unless the command asked to stop at the plan, the +//! plan dispatched on the Base branch. A review that found no Strong +//! candidate published no plan, and the Architect run ends on the issue it +//! named instead, labelled an //! Architect idea: the idea issue it filed for its top recommendation, or the //! open issue that already covers it. Only one Architect run or Pickup run //! per repository runs at a time on a machine: an Architect run started while @@ -26,21 +27,14 @@ use crate::config::UserConfig; use crate::failed_run::FailedRun; use crate::github::ListedIssue; use crate::harness::Choice; -use crate::interrupt; use crate::issue::{IssueUrl, Repo}; use crate::labels::{Edit, Label, Labels, NEEDS_TRIAGE, READY_FOR_AGENT}; use crate::launch::{self, AlreadyRunning, Launch, Start}; use crate::logs::{self, Pass, Work}; -use crate::pass::{LaunchAndGitHub, Outside}; -use crate::progress; +use crate::pass::{Dispatch, LaunchAndGitHub, Outside}; use crate::prompt; use crate::ready::ReadyIssue; -use crate::session::{Logs, Sessions}; -use crate::worktree::ReviewWorktree; - -/// The Architecture review session's kind, in its progress lines and log -/// name. -const REVIEW: &str = "architecture-review"; +use crate::run::Ended; /// The label thirdshift marks an Architect plan with, which a later Architect /// run finds an open one by. @@ -56,13 +50,17 @@ const ARCHITECT_IDEA: Label = Label::new( "An Architect idea: the issue an Architecture review with no Strong candidate ended on", ); -/// How an Architect run ended, short of a failure and before any dispatch. -#[derive(Debug)] +/// How an Architect run ended. pub enum Outcome { /// Skipped, before any review, having done nothing. Skipped(Skipped), - /// Its Architecture review ended on an issue. - Reviewed(Reviewed), + /// It was not skipped: how its Architecture review ended, failing if + /// anything before it did, and how the run it dispatched its plan as + /// ended, if it dispatched one. + Ran { + review: Result, + dispatched: Option, + }, } /// Why an Architect run was skipped. Its `Display` is the reason, as the @@ -142,9 +140,8 @@ fn write_joined( /// issue. #[derive(Debug)] pub enum Reviewed { - /// The plan the review published is marked ready, with the Architect - /// run's Base branch, which the run the plan is dispatched as takes. - PlanReady { plan: IssueUrl, base: String }, + /// The plan the review published is marked ready. + PlanReady(IssueUrl), /// The review found no Strong candidate, and filed its top /// recommendation as this issue. IdeaFiled(IssueUrl), @@ -158,7 +155,7 @@ impl Reviewed { /// notification says it. pub fn review(&self) -> &'static str { match self { - Self::PlanReady { .. } => "plan published", + Self::PlanReady(_) => "plan published", Self::IdeaFiled(_) => "idea filed", Self::AlreadyFiled(_) => "idea already filed", } @@ -167,9 +164,9 @@ impl Reviewed { /// The URL of the issue the Architecture review ended on. pub fn url(&self) -> &str { match self { - Self::PlanReady { plan: issue, .. } - | Self::IdeaFiled(issue) - | Self::AlreadyFiled(issue) => &issue.url, + Self::PlanReady(issue) | Self::IdeaFiled(issue) | Self::AlreadyFiled(issue) => { + &issue.url + } } } } @@ -178,7 +175,7 @@ impl fmt::Display for Reviewed { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { let url = self.url(); match self { - Self::PlanReady { .. } => write!(f, "plan {url} is ready for an agent"), + Self::PlanReady(_) => write!(f, "plan {url} is ready for an agent"), Self::IdeaFiled(_) => write!( f, "no Strong candidate: the Architecture review filed the idea {url}" @@ -192,50 +189,48 @@ impl fmt::Display for Reviewed { } /// Run an Architecture review of the Base branch, pointed at `focus` if -/// given, and mark the plan it publishes ready. The Base branch is `base`, -/// the branch the command named, whatever the Launch directory has checked -/// out, or without one the branch checked out there. A review that reports -/// an idea issue it filed, or the open issue that already covers its top -/// recommendation, instead of a plan, has that issue labelled an Architect -/// idea. With `config`'s `launch.pull`, the Launch directory's checkout of -/// the Base branch, if that is the branch checked out, is first brought up -/// to date with origin. The review's -/// worktree is gone when this returns. A failure -/// after the plan is published leaves its labels as the review left them. +/// given, and mark the plan it publishes ready, then, unless `plan_only`, +/// dispatch it as `thirdshift ` with `flags` would run it, but on +/// the Base branch. The Base branch is `base`, the branch the command named, +/// whatever the Launch directory has checked out, or without one the branch +/// checked out there. A review that reports an idea issue it filed, or the +/// open issue that already covers its top recommendation, instead of a plan, +/// has that issue labelled an Architect idea, and dispatches nothing. With +/// `config`'s `launch.pull`, the Launch directory's checkout of the Base +/// branch, if that is the branch checked out, is first brought up to date +/// with origin. The review's worktree is gone when the review is concluded. +/// A failure after the plan is published leaves its labels as the review +/// left them. A failure before the review, the preflight checks included, is +/// a failed review too. /// /// Once the preflight checks pass, and before anything else, the Architect /// run is skipped, with nothing done, if another on the same repository, or a -/// Pickup run, is still running on this machine: see [`launch::start`]. It is -/// then skipped, likewise, if the repository has an open Architect plan: only -/// then, so that the plan of an Architect run still running is never taken -/// for an unfinished one. It is then skipped if the repository has an open -/// Architect idea still labelled `needs-triage`. It is then skipped if the -/// repository has a Ready issue, by the very search a Pickup run makes, with -/// its lines on the issues passed over, but with no Sweep and no Claim limit, -/// which are the Pickup run's. A skip is recorded in the repository's -/// Activity log. Past all four, it checks `harness`, the Harness, Model and -/// Effort its review, and the run it dispatches, run their sessions on, -/// failing before any work if they can't run. Then it records that it -/// started work, which keeps its Command log with its repository's logs, -/// where its Session logs go too. +/// Pickup run, is still running on this machine: see [`launch::start`]. A +/// skip is recorded in the repository's Activity log. The rest, from the +/// gates to the dispatch, is [`run_through`], through the Launch directory, +/// GitHub, the logs and `harness`, the Harness, Model and Effort its review, +/// and the run it dispatches, run their sessions on, and `flags` and +/// `config`, which ask that run. pub fn run( focus: Option<&str>, base: Option<&str>, + plan_only: bool, flags: &Flags, config: &UserConfig, harness: &mut Choice, -) -> Result { +) -> Outcome { let started = Utc::now(); let Launch { directory, repo, base, - } = match launch::start(base)? { - Start::Clear(launch) => launch, - Start::AlreadyRunning(running) => { - let repo = running.0.clone(); - return Ok(skip(&repo, Skipped::AlreadyRunning(running))); + } = match launch::start(base) { + Ok(Start::Clear(launch)) => launch, + Ok(Start::AlreadyRunning(running)) => { + logs::skipped(Pass::ArchitectRun, &running.0, &running); + return Outcome::Skipped(Skipped::AlreadyRunning(running)); } + Err(error) => return failed(error.into()), }; let mut outside = LaunchAndGitHub { launch: &directory, @@ -244,35 +239,118 @@ pub fn run( flags, config, }; - if let Decision::Skip(skipped) = gates(&mut outside)? { - return Ok(skip(&repo, skipped)); + let architect_run = ArchitectRun { + repo: &repo, + base: &base, + origin: directory.origin(), + focus, + started, + pull: config.launch_pull, + plan_only, + }; + run_through(&mut outside, &architect_run) +} + +/// An Architect run past its lock and preflight checks: what it runs on, +/// and what it was asked for. +struct ArchitectRun<'a> { + /// The repository it runs on. + repo: &'a Repo, + /// The Base branch. + base: &'a str, + /// The URL of the Launch directory's `origin`, which its plan must be in. + origin: &'a str, + /// What the command pointed the review at, if anything. + focus: Option<&'a str>, + /// When the Architect run started, before its plan can have been + /// created. + started: DateTime, + /// Whether to pull the Launch directory's checkout of the Base branch + /// first. + pull: bool, + /// Whether to stop at the plan, dispatching nothing. + plan_only: bool, +} + +/// [`run`], once it holds its repository's lock, through `outside`: its +/// [`gates`], a skip recorded with its reason if one skips; then the check of +/// its Harness, failing before any work if it can't run; then the record +/// that it started work, which keeps its Command log with its repository's +/// logs, where its Session logs go too; then the Launch directory's pull, if +/// asked for. Then, unless it was interrupted, the Architecture review +/// session and its [`conclude`]; then, unless `plan_only`, the plan it +/// marked ready dispatched on its Base branch. +fn run_through(outside: &mut impl Outside, architect_run: &ArchitectRun) -> Outcome { + match gates(outside) { + Ok(Decision::GoAhead) => {} + Ok(Decision::Skip(skipped)) => { + outside.skipped(Pass::ArchitectRun, &skipped); + return Outcome::Skipped(skipped); + } + Err(error) => return failed(error.into()), + } + let review = up_to_the_dispatch(outside, architect_run); + let dispatched = match &review { + Ok(Reviewed::PlanReady(plan)) if !architect_run.plan_only => { + outside.step(format!( + "dispatching the plan {url}, as thirdshift {url} would", + url = plan.url + )); + Some(outside.dispatch(Dispatch::ArchitectPlan { + plan, + base: architect_run.base, + })) + } + _ => None, + }; + Outcome::Ran { review, dispatched } +} + +/// The Architect run failed, as `failed` says, before any review got +/// anywhere: nothing to dispatch. +fn failed(failed: FailedRun) -> Outcome { + Outcome::Ran { + review: Err(failed), + dispatched: None, } +} + +/// [`run_through`] past its gates, up to its dispatch: the Harness check, +/// the start, the pull, and the Architecture review, with its conclusion, +/// failing with the review's Session log if it gets that far. +fn up_to_the_dispatch( + outside: &mut impl Outside, + architect_run: &ArchitectRun, +) -> Result { + let ArchitectRun { + repo, + base, + origin, + focus, + started, + pull, + .. + } = *architect_run; outside.check_harness()?; - outside.started(Work::ArchitectRun(&repo)); - // Cloned, so the review's conclusion can take `outside` itself. - let harness = outside.harness.clone(); - if config.launch_pull { - directory.pull(&base); - } - - if interrupt::requested() { - return Err(anyhow!("interrupted").into()); - } - let worktree = ReviewWorktree::create(directory.git(), &repo.name, &base)?; - let logs = Logs::of_architect_run(&repo); - let (reviewed, log) = Sessions::within(&logs, worktree.path(), &harness, |sessions| { - review( - &mut outside, - sessions, - &base, - focus, - directory.origin(), - started, - ) + outside.started(Work::ArchitectRun(repo)); + if pull { + outside.pull(base); + } + if outside.interrupted() { + return Err(FailedRun { + interrupted: true, + ..FailedRun::from(anyhow!("interrupted")) + }); + } + let starting = match focus { + Some(focus) => format!("starting the Architecture review of {base}, focused on: {focus}"), + None => format!("starting the Architecture review of {base}"), + }; + let prompt = prompt::architecture_review(base, focus); + let (reviewed, log) = outside.review(base, starting, &prompt, |outside, final_message| { + conclude(outside, final_message, origin, started) }); - // Removed once the review is concluded. - drop(worktree); - reviewed.map(Outcome::Reviewed).map_err(|error| FailedRun { + reviewed.map_err(|error| FailedRun { log, ..FailedRun::from(error) }) @@ -309,34 +387,6 @@ fn gates(outside: &mut impl Outside) -> Result { }) } -/// The Architect run skipped as `skipped` says, recorded in the Activity log -/// of `repo`. -fn skip(repo: &Repo, skipped: Skipped) -> Outcome { - logs::skipped(Pass::ArchitectRun, repo, &skipped); - Outcome::Skipped(skipped) -} - -/// The Architecture review session, of the Base branch `base`, run through -/// `sessions`, then [`conclude`] on its final message, through `outside`. -fn review( - outside: &mut impl Outside, - sessions: &Sessions, - base: &str, - focus: Option<&str>, - origin: &str, - started: DateTime, -) -> Result { - match focus { - Some(focus) => progress::step(format_args!( - "starting the Architecture review of {base}, focused on: {focus}" - )), - None => progress::step(format_args!("starting the Architecture review of {base}")), - } - let prompt = prompt::architecture_review(base, focus); - let final_message = sessions.run_to_final_message(REVIEW, &prompt)?; - conclude(outside, final_message.as_deref(), origin, started, base) -} - /// What an Architecture review reported in the last line of its final /// message. #[derive(Debug, PartialEq, Eq)] @@ -368,8 +418,8 @@ impl Report { } } -/// End the Architecture review, of the Base branch `base`, on the issue the -/// last line of its `final_message` names, reaching it through `outside`: a +/// End the Architecture review on the issue the last line of its +/// `final_message` names, reaching it through `outside`: a /// plan is marked ready, and an idea issue, or the issue that already covers /// the top recommendation, is labelled an Architect idea. Fails, with no /// call made, if the session had no final message or its last line is not @@ -380,13 +430,11 @@ fn conclude( final_message: Option<&str>, origin: &str, started: DateTime, - base: &str, ) -> Result { match final_message.and_then(Report::read) { Some(Report::Plan(plan)) => { mark_plan_ready(outside, &plan, origin, started)?; - let base = base.to_string(); - Ok(Reviewed::PlanReady { plan, base }) + Ok(Reviewed::PlanReady(plan)) } Some(Report::Idea(idea)) => { label_idea(outside, &idea)?; @@ -492,9 +540,12 @@ fn check_plan( #[cfg(test)] mod tests { + use std::path::Path; + use super::*; use crate::github; use crate::pass::{Call, InMemory}; + use crate::run::{Goal, Reached}; const URL: &str = "https://github.com/acme/widgets/issues/8"; @@ -711,7 +762,7 @@ mod tests { /// The Architect run's conclusion on `final_message`, through `repo`: /// how the review ended, or the cause it failed with, and what it did. fn concluded(mut repo: InMemory, final_message: &str) -> (Result, Vec) { - let ended = conclude(&mut repo, Some(final_message), ORIGIN, started(), "main") + let ended = conclude(&mut repo, Some(final_message), ORIGIN, started()) .map(|reviewed| reviewed.to_string()) .map_err(|error| format!("{error:#}")); (ended, repo.calls) @@ -1008,7 +1059,7 @@ mod tests { fn no_final_message_fails_with_no_call_made() { let mut repo = InMemory::default(); - let ended = conclude(&mut repo, None, ORIGIN, started(), "main"); + let ended = conclude(&mut repo, None, ORIGIN, started()); assert_eq!( ended.unwrap_err().to_string(), @@ -1016,4 +1067,287 @@ mod tests { ); assert_eq!(repo.calls, []); } + + /// The repository every Architect run is on. + fn widgets() -> Repo { + Repo { + owner: "acme".to_string(), + name: "widgets".to_string(), + } + } + + /// An Architect run on `repo`, past its lock, with no focus, no pull, + /// and no plan-only, on the Base branch `main`, except as `asked` + /// changes them, its dispatched run ending ready for review: how it + /// ended, and what it did outside itself. + fn architect( + repo: InMemory, + asked: impl FnOnce(&mut ArchitectRun<'_>), + ) -> (Outcome, Vec) { + let mut repo = repo.dispatched_ending(ready_for_review()); + let widgets = widgets(); + let mut architect_run = ArchitectRun { + repo: &widgets, + base: "main", + origin: ORIGIN, + focus: None, + started: started(), + pull: false, + plan_only: false, + }; + asked(&mut architect_run); + let outcome = run_through(&mut repo, &architect_run); + (outcome, repo.calls) + } + + /// A dispatched run that ended ready for review. + fn ready_for_review() -> Ended { + Ended { + outcome: Ok(Reached { + pr_url: "https://github.com/acme/widgets/pull/12".to_string(), + goal: Goal::ReadyForReview, + log: None, + ticket_lines: Vec::new(), + }), + base_fix: None, + advice: Vec::new(), + } + } + + /// What an Architect run that got past its gates came to: how its review + /// ended, as its line or its cause, and whether it dispatched a run. + fn reviewed(outcome: Outcome) -> (Result, bool) { + let Outcome::Ran { review, dispatched } = outcome else { + panic!("the Architect run was skipped"); + }; + let review = review + .map(|reviewed| reviewed.to_string()) + .map_err(|failed| format!("{:#}", failed.error)); + (review, dispatched.is_some()) + } + + /// The calls that start the Architecture review of `main`, with no + /// focus. + fn reviewing_main() -> [Call; 2] { + [ + Call::Review { + base: "main".to_string(), + prompt: prompt::architecture_review("main", None), + }, + step("starting the Architecture review of main"), + ] + } + + /// A repository whose review publishes plan #8, which passes its checks. + fn publishing_a_plan() -> InMemory { + InMemory::default() + .reviewed(&message(prompt::PLAN_LINE)) + .viewable(8, viewed(true, 0, &["needs-triage"])) + } + + #[test] + fn a_skipped_architect_run_records_its_reason_with_no_harness_check_start_pull_or_review() { + for (repo, gates_read) in [ + (InMemory::default().issue(5, true, &["architect-plan"]), 1), + ( + InMemory::default().issue(5, true, &["architect-idea", "needs-triage"]), + 2, + ), + (InMemory::default().ready(5, false), 3), + ] { + let repo = repo.reviewed(&message(prompt::PLAN_LINE)); + + let (outcome, calls) = architect(repo, |run| run.pull = true); + + let Outcome::Skipped(skipped) = outcome else { + panic!("the Architect run was not skipped"); + }; + let mut expected: Vec = [PLANS, IDEAS, Call::ReadySearch] + .into_iter() + .take(gates_read) + .collect(); + expected.push(Call::Skipped(skipped.to_string())); + assert_eq!(calls, expected, "{skipped}"); + } + } + + #[test] + fn a_harness_that_fails_its_check_fails_the_architect_run_before_its_start() { + let repo = publishing_a_plan().harness_failing(); + + let (outcome, calls) = architect(repo, |run| run.pull = true); + + assert_eq!( + reviewed(outcome), + (Err("claude is not on PATH".to_string()), false) + ); + assert_eq!(calls, [PLANS, IDEAS, Call::ReadySearch, Call::HarnessCheck]); + } + + #[test] + fn the_pull_is_made_only_when_asked_after_the_start_is_recorded() { + for pull in [false, true] { + let (_, calls) = architect(publishing_a_plan(), |run| run.pull = pull); + + let mut expected = vec![ + PLANS, + IDEAS, + Call::ReadySearch, + Call::HarnessCheck, + Call::Started(None), + ]; + if pull { + expected.push(Call::Pull("main".to_string())); + } + expected.extend(reviewing_main()); + assert_eq!(calls[..expected.len()], expected, "pull: {pull}"); + } + } + + #[test] + fn an_interrupt_after_the_gates_fails_the_architect_run_as_interrupted_with_no_review() { + let repo = publishing_a_plan().interrupted(); + + let (outcome, calls) = architect(repo, |run| run.pull = true); + + let Outcome::Ran { + review: Err(failed), + dispatched: None, + } = outcome + else { + panic!("the Architect run did not fail, or dispatched a run"); + }; + assert_eq!(failed.error.to_string(), "interrupted"); + assert!(failed.interrupted); + assert_eq!(failed.log, None); + assert_eq!( + calls, + [ + PLANS, + IDEAS, + Call::ReadySearch, + Call::HarnessCheck, + Call::Started(None), + Call::Pull("main".to_string()), + ] + ); + } + + #[test] + fn the_reviews_prompt_carries_the_base_branch_and_the_focus() { + let (_, calls) = architect(publishing_a_plan(), |run| { + run.base = "develop"; + run.focus = Some("the Spec run"); + }); + + let review = calls + .iter() + .position(|call| matches!(call, Call::Review { .. })) + .expect("no review session"); + let Call::Review { base, prompt } = &calls[review] else { + unreachable!(); + }; + assert_eq!( + calls[review + 1], + step("starting the Architecture review of develop, focused on: the Spec run") + ); + assert_eq!(base, "develop"); + assert!( + prompt.contains("the base branch develop:"), + "prompt: {prompt}" + ); + assert!( + prompt.contains("Focus the review on: the Spec run\n"), + "prompt: {prompt}" + ); + } + + #[test] + fn a_plan_is_dispatched_on_the_base_branch_after_the_label_swap_unless_plan_only() { + for plan_only in [false, true] { + let (outcome, calls) = architect(publishing_a_plan(), |run| { + run.base = "develop"; + run.plan_only = plan_only; + }); + + assert_eq!( + reviewed(outcome), + ( + Ok(format!("plan {} is ready for an agent", url(8))), + !plan_only + ) + ); + let swap = calls + .iter() + .position(|call| matches!(call, Call::Edit { .. })) + .expect("the plan was not marked ready"); + let dispatch = [ + step(&format!( + "dispatching the plan {url}, as thirdshift {url} would", + url = url(8) + )), + Call::DispatchPlan { + plan: 8, + base: "develop".to_string(), + }, + ]; + let after_swap: &[Call] = if plan_only { &[] } else { &dispatch }; + assert_eq!(calls[swap + 1..], *after_swap, "plan-only: {plan_only}"); + } + } + + #[test] + fn an_idea_filed_or_already_filed_dispatches_nothing() { + for line in [prompt::IDEA_LINE, prompt::ALREADY_FILED_LINE] { + let repo = InMemory::default() + .reviewed(&message(line)) + .viewable(8, viewed(true, -3600, &[])); + + let (outcome, calls) = architect(repo, |_| {}); + + let (review, dispatched) = reviewed(outcome); + assert!(review.is_ok(), "{line}: {review:?}"); + assert!(!dispatched, "{line}"); + assert!( + matches!(calls.last(), Some(Call::Edit { .. })), + "{line}: {calls:?}" + ); + } + } + + const SESSION_LOG: &str = "/logs/acme/widgets/sessions/architect-1-architecture-review.jsonl"; + + #[test] + fn a_failed_review_dispatches_nothing_and_carries_the_reviews_session_log() { + for repo in [ + InMemory::default().review_failing("claude exited with status 1"), + InMemory::default().reviewed("Published the plan."), + InMemory::default() + .reviewed(&message(prompt::PLAN_LINE)) + .viewable(8, viewed(false, 0, &["needs-triage"])), + ] { + let (outcome, calls) = architect(repo.session_log(SESSION_LOG), |_| {}); + + let Outcome::Ran { + review: Err(failed), + dispatched: None, + } = outcome + else { + panic!("the review did not fail, or a run was dispatched"); + }; + assert_eq!( + failed.log.as_deref(), + Some(Path::new(SESSION_LOG)), + "{:#}", + failed.error + ); + assert!( + !calls + .iter() + .any(|call| matches!(call, Call::DispatchPlan { .. })), + "{:#}", + failed.error + ); + } + } } diff --git a/src/command.rs b/src/command.rs index 98d269aa8..2492c2265 100644 --- a/src/command.rs +++ b/src/command.rs @@ -33,9 +33,9 @@ pub enum Ending { /// A Run or a Spec run ended: started by hand, or dispatched by a Pickup /// run. Run(Ended), - /// An Architect run got past its skip checks: how its Architecture - /// review ended, and how the run it dispatched its plan as ended, if it - /// dispatched one. + /// An Architect run was not skipped: how its Architecture review ended, + /// failing if anything before it did, and how the run it dispatched its + /// plan as ended, if it dispatched one. Architect { review: Result, dispatched: Option, diff --git a/src/main.rs b/src/main.rs index 430991844..bc55f95bf 100644 --- a/src/main.rs +++ b/src/main.rs @@ -41,7 +41,7 @@ mod worktree; use std::process::ExitCode; -use architect::{Outcome, Reviewed}; +use architect::Outcome; use args::{ArchitectArgs, Command, PickupArgs, RunArgs}; use asks::Asks; use command::{Ending, failure}; @@ -426,37 +426,19 @@ fn architect(args: ArchitectArgs) -> ExitCode { Err(failure) => return failure, }; let mut harness = args.flags.harness(&config); - let review = architect::run( + let outcome = architect::run( args.focus.as_deref(), args.base.as_deref(), + args.plan_only, &args.flags, &config, &mut harness, ); started.built_with(&harness); - let review = match review { - Ok(Outcome::Skipped(skipped)) => { - return started.finish(Ending::Skipped(skipped.into())); - } - Ok(Outcome::Reviewed(reviewed)) => Ok(reviewed), - Err(failed) => Err(failed), - }; - let dispatched = match &review { - Ok(Reviewed::PlanReady { plan, base }) if !args.plan_only => { - progress::step(format_args!( - "dispatching the plan {url}, as thirdshift {url} would", - url = plan.url - )); - let mut asks = Asks { - harness: harness.clone(), - ..Asks::of_architect_plan(plan, &args.flags, &config) - }; - let started_by = StartedBy::Dispatch { base }; - Some(run::run_to_end(plan, &mut asks, started_by)) - } - _ => None, - }; - started.finish(Ending::Architect { review, dispatched }) + started.finish(match outcome { + Outcome::Skipped(skipped) => Ending::Skipped(skipped.into()), + Outcome::Ran { review, dispatched } => Ending::Architect { review, dispatched }, + }) } /// A Pickup run: the search for the lowest-numbered Ready issue in the Launch diff --git a/src/pass.rs b/src/pass.rs index bf3120f07..9dd7c4a73 100644 --- a/src/pass.rs +++ b/src/pass.rs @@ -3,13 +3,16 @@ //! what an Architect run's conclusion after its Architecture review does, //! viewing and labelling the issue the review ended on, and what a pass //! does outside itself around them: recording its skip or its start, -//! checking its Harness, and dispatching the issue it ends on. +//! checking its Harness, pulling the Launch directory, running the +//! Architecture review session, and dispatching the issue it ends on. //! [`LaunchAndGitHub`] does each through `gh`, the Ready issue search, the -//! interrupt flag, the progress lines, the logs, the Harness check and +//! interrupt flag, the progress lines, the logs, the Harness check, the +//! Launch directory, the review's worktree and its [`Sessions`], and //! [`run::run_to_end`]; [`InMemory`], in tests, from memory, recording each //! call. use std::fmt::Display; +use std::path::PathBuf; use anyhow::Result; @@ -25,16 +28,18 @@ use crate::logs::{self, Pass, Work}; use crate::progress; use crate::ready::{self, ReadyIssue}; use crate::run::{self, Ended, StartedBy}; +use crate::session::{Logs, Sessions}; +use crate::worktree::ReviewWorktree; + +/// The Architecture review session's kind, in its progress lines and log +/// name. +const REVIEW: &str = "architecture-review"; /// What a pass dispatches, as `thirdshift ` would run it, but on /// the pass's Base branch `base`, whatever the Launch directory has checked /// out. pub enum Dispatch<'a> { /// An Architect run's Architect plan. - #[expect( - dead_code, - reason = "the Architect run dispatches its plan through the Pass seam from #415" - )] ArchitectPlan { plan: &'a IssueUrl, base: &'a str }, /// The Ready issue a Pickup run took, a Spec or not, as `is_spec` says. ReadyIssue { @@ -47,7 +52,8 @@ pub enum Dispatch<'a> { /// What a pass's gates, and an Architect run's conclusion, read and change of /// its repository on GitHub, whether the run is interrupted, and where their /// progress lines go; and what the pass records in the logs, the check of -/// its Harness, and the run it dispatches. +/// its Harness, the Launch directory's pull, the Architecture review +/// session, and the run it dispatches. pub trait Outside { /// Every open issue in the repository labelled `label`. fn open_issues(&mut self, label: Label) -> Result>; @@ -71,6 +77,24 @@ pub trait Outside { fn check_harness(&mut self) -> Result<()>; /// Record that the pass started `work`, on the Harness it checked. fn started(&mut self, work: Work); + /// Bring the Launch directory's checkout of the Base branch `base` up to + /// date with origin, if it is the branch checked out there: only a + /// warning if it can't. + fn pull(&mut self, base: &str); + /// Run the Architecture review session of the Base branch `base` on + /// `prompt`, on the Harness the pass checked, to its final message, + /// handing on the progress line `starting` once it is ready to start it, + /// then `conclude` on this seam and that message, while the session is + /// still open: a failure of `conclude` after a session left background + /// work to be killed names that work too. Returns what `conclude` came + /// to, with the session's Session log, if it has one. + fn review( + &mut self, + base: &str, + starting: String, + prompt: &str, + conclude: impl FnOnce(&mut Self, Option<&str>) -> Result, + ) -> (Result, Option); /// Run `dispatch` to its end, on the Harness the pass checked. fn dispatch(&mut self, dispatch: Dispatch) -> Ended; } @@ -129,6 +153,36 @@ impl Outside for LaunchAndGitHub<'_> { logs::started(work, self.harness); } + fn pull(&mut self, base: &str) { + self.launch.pull(base); + } + + /// The session runs in its own worktree, detached at `base`'s head on + /// origin, which is removed once `conclude` is done. + fn review( + &mut self, + base: &str, + starting: String, + prompt: &str, + conclude: impl FnOnce(&mut Self, Option<&str>) -> Result, + ) -> (Result, Option) { + let worktree = match ReviewWorktree::create(self.launch.git(), &self.repo.name, base) { + Ok(worktree) => worktree, + Err(error) => return (Err(error), None), + }; + progress::step(starting); + let logs = Logs::of_architect_run(self.repo); + // Cloned, so the conclusion can take the seam itself. + let harness = self.harness.clone(); + let concluded = Sessions::within(&logs, worktree.path(), &harness, |sessions| { + let final_message = sessions.run_to_final_message(REVIEW, prompt)?; + conclude(self, final_message.as_deref()) + }); + // Removed once the review is concluded. + drop(worktree); + concluded + } + /// The run's asks are the Architect plan's or the Ready issue's, from /// the command's flags and the User config, on the checked Harness. fn dispatch(&mut self, dispatch: Dispatch) -> Ended { @@ -162,8 +216,9 @@ pub use in_memory::{Call, InMemory}; #[cfg(test)] mod in_memory { use std::fmt::Display; + use std::path::PathBuf; - use anyhow::{Result, bail}; + use anyhow::{Result, anyhow, bail}; use super::{Dispatch, Outside}; use crate::github::{Issue, ListedIssue}; @@ -201,19 +256,26 @@ mod in_memory { /// It recorded that it started work: on this issue, for a Pickup /// run, or on its repository, for an Architect run. Started(Option), + /// It pulled the Launch directory's checkout of this Base branch. + Pull(String), + /// It ran the Architecture review session of this Base branch on + /// this prompt, handing on its starting line next. + Review { base: String, prompt: String }, /// It dispatched this issue, a Spec or not, on this Base branch. Dispatch { issue: u64, is_spec: bool, base: String, }, + /// It dispatched this Architect plan on this Base branch. + DispatchPlan { plan: u64, base: String }, } /// A repository in memory: its issues, filed by label, open or /// closed, the issues it can view, the Ready issue search's answer, the /// issues whose Edits fail, and whether the run was interrupted; with - /// whether the Harness check fails and how a dispatched run ends. - #[derive(Default)] + /// whether the Harness check fails, how the Architecture review session + /// ends, and how a dispatched run ends. pub struct InMemory { /// Each issue, filed under the label a listing finds it by. filed: Vec, @@ -230,12 +292,38 @@ mod in_memory { ready: Option<(ListedIssue, bool)>, /// Whether the Harness check fails. harness_failing: bool, + /// The Architecture review session's final message, if it has one, + /// or the cause it fails with. + review: Result, String>, + /// The Architecture review session's Session log. + session_log: Option, /// How the run dispatched ends. ending: Option, /// Every call made, in order. pub calls: Vec, } + /// A repository with no issue, whose Harness check passes and whose + /// Architecture review session ends with no final message and no + /// Session log. + impl Default for InMemory { + fn default() -> Self { + InMemory { + filed: Vec::new(), + viewable: Vec::new(), + interrupted: false, + closed_listing_fails: false, + failing_edits: Vec::new(), + ready: None, + harness_failing: false, + review: Ok(None), + session_log: None, + ending: None, + calls: Vec::new(), + } + } + } + /// An issue filed under a label. struct Filed { /// The label, as spelled. @@ -302,6 +390,25 @@ mod in_memory { self } + /// Have the Architecture review session end with `final_message`. + pub fn reviewed(mut self, final_message: &str) -> Self { + self.review = Ok(Some(final_message.to_string())); + self + } + + /// Make the Architecture review session fail with `cause`, before + /// any conclusion. + pub fn review_failing(mut self, cause: &str) -> Self { + self.review = Err(cause.to_string()); + self + } + + /// Have the Architecture review session logged at `log`. + pub fn session_log(mut self, log: &str) -> Self { + self.session_log = Some(PathBuf::from(log)); + self + } + /// Have the run dispatched end as `ending`. pub fn dispatched_ending(mut self, ending: Ended) -> Self { self.ending = Some(ending); @@ -399,19 +506,44 @@ mod in_memory { self.calls.push(Call::Started(issue)); } - fn dispatch(&mut self, dispatch: Dispatch) -> Ended { - let Dispatch::ReadyIssue { - issue, - is_spec, - base, - } = dispatch - else { - unimplemented!("an Architect plan's dispatch, recorded from #415") - }; - self.calls.push(Call::Dispatch { - issue: issue.number, - is_spec, + fn pull(&mut self, base: &str) { + self.calls.push(Call::Pull(base.to_string())); + } + + fn review( + &mut self, + base: &str, + starting: String, + prompt: &str, + conclude: impl FnOnce(&mut Self, Option<&str>) -> Result, + ) -> (Result, Option) { + self.calls.push(Call::Review { base: base.to_string(), + prompt: prompt.to_string(), + }); + self.calls.push(Call::Step(starting)); + let log = self.session_log.clone(); + match self.review.clone() { + Ok(final_message) => (conclude(self, final_message.as_deref()), log), + Err(cause) => (Err(anyhow!(cause)), log), + } + } + + fn dispatch(&mut self, dispatch: Dispatch) -> Ended { + self.calls.push(match dispatch { + Dispatch::ArchitectPlan { plan, base } => Call::DispatchPlan { + plan: plan.number, + base: base.to_string(), + }, + Dispatch::ReadyIssue { + issue, + is_spec, + base, + } => Call::Dispatch { + issue: issue.number, + is_spec, + base: base.to_string(), + }, }); self.ending .take() diff --git a/src/run_ending.rs b/src/run_ending.rs index bd788a4ed..efc5974f0 100644 --- a/src/run_ending.rs +++ b/src/run_ending.rs @@ -378,10 +378,7 @@ mod tests { const PLAN: &str = "https://github.com/acme/widgets/issues/40"; fn plan() -> Reviewed { - Reviewed::PlanReady { - plan: IssueUrl::parse(PLAN).unwrap(), - base: "main".to_string(), - } + Reviewed::PlanReady(IssueUrl::parse(PLAN).unwrap()) } fn failed_run(cause: &str) -> FailedRun { diff --git a/tests/architect_run.rs b/tests/architect_run.rs index 5b0e1f2d0..17b85b714 100644 --- a/tests/architect_run.rs +++ b/tests/architect_run.rs @@ -419,7 +419,7 @@ echo "widgets, by the review" > README.md } #[test] -fn progress_lines_cover_the_review_starting_the_plan_it_reported_and_the_label_swap() { +fn progress_lines_cover_the_worktree_the_review_starting_the_plan_it_reported_and_the_label_swap() { let scenario = scenario(); scenario.agent_does(AGENT_PUBLISHES_A_TICKET); @@ -428,6 +428,7 @@ fn progress_lines_cover_the_review_starting_the_plan_it_reported_and_the_label_s assert_eq!(result.code, Some(0), "stderr: {}", result.stderr); let mut rest = result.stderr.as_str(); for line in [ + " detached at origin/main\n".to_string(), "thirdshift: starting the Architecture review of main, focused on: the Spec run\n" .to_string(), "thirdshift: architecture-review: session started\n".to_string(), @@ -679,25 +680,6 @@ fn an_idea_that_cannot_be_labelled_fails_the_architect_run_naming_the_cause() { } } -#[test] -fn a_review_with_no_strong_candidate_dispatches_nothing_without_plan_only() { - let scenario = scenario(); - scenario.issue_labelled(7, &["needs-triage"]); - let url = scenario.issue_url(7); - scenario.agent_does(&ends_with(&format!("Architecture review idea: {url}"))); - - let result = scenario.run(&["architect", "merge"]); - - assert_ended_without_a_plan( - &scenario, - &result, - &url, - &format!("no Strong candidate: the Architecture review filed the idea {url}"), - ); - assert_eq!(scenario.issue_labels(7), ["needs-triage", ARCHITECT_IDEA]); - assert!(scenario.gh_calls_of("pr", "create").is_empty()); -} - #[test] fn a_plan_older_than_the_architect_run_is_refused() { let scenario = scenario(); @@ -1111,32 +1093,6 @@ fn a_dispatched_run_that_fails_fails_the_architect_run_as_a_failed_run_does() { assert_eq!(scenario.issue_labels(8), [ARCHITECT_PLAN, IN_PROGRESS]); } -#[test] -fn progress_lines_show_the_dispatch_after_the_label_swap_and_before_the_run() { - let scenario = single_ticket_plan(); - - let result = scenario.run(&["architect"]); - - assert_eq!(result.code, Some(0), "stderr: {}", result.stderr); - let mut rest = result.stderr.as_str(); - for line in [ - "thirdshift: marking the plan ready: swapping needs-triage for ready-for-agent and adding architect-plan on #8\n" - .to_string(), - format!("thirdshift: dispatching the plan {PLAN_URL}, as thirdshift {PLAN_URL} would\n"), - "thirdshift: implement: session started\n".to_string(), - ] { - let Some(at) = rest.find(&line) else { - panic!("expected {line:?}, in order, in stderr: {}", result.stderr); - }; - rest = &rest[at + line.len()..]; - } - assert!( - !result.stderr.contains("is ready for an agent"), - "stderr: {}", - result.stderr - ); -} - /// A script that records the commit the session's worktree is at as /// `review-head` in the scenario root. const RECORDS_THE_REVIEW_HEAD: &str = @@ -2445,16 +2401,3 @@ fn the_review_and_the_run_it_dispatches_run_on_the_model_checked_once() { ); } } - -#[test] -fn a_skipped_architect_run_makes_no_check_of_its_harness_or_model() { - let scenario = scenario(); - scenario.issue_is(5, "OPEN"); - scenario.issue_labelled(5, &[ARCHITECT_PLAN]); - scenario.agent_does("echo 'no such model'\nexit 1"); - - let result = scenario.run(&["architect", "model", "Opus 5.5", "harness", "codex"]); - - assert_eq!(result.code, Some(0), "stderr: {}", result.stderr); - assert!(scenario.claude_calls().is_empty()); -}