From 9ed603c8d3e7a4d4e1131c7aa12f6009bcd037e1 Mon Sep 17 00:00:00 2001 From: Jacob Stephens Date: Mon, 5 Oct 2026 05:07:22 -0400 Subject: [PATCH 1/2] An Architect run owns its lifecycle to its dispatch, tested in-process behind the Pass seam (#415) The Pass seam grows a pull of the Launch directory's Base branch and the Architecture review session, which runs its conclusion inside the session and returns its result with the Session log. Everything in an Architect run after its lock and preflight checks runs in run_through, generic over the seam: the gates, the Harness check, the start, the pull, the review and the plan's dispatch, unless plan-only. The entry point no longer builds Asks or calls run_to_end for one. Co-Authored-By: Claude Opus 5.5 --- src/architect.rs | 555 ++++++++++++++++++++++++++++++++--------- src/main.rs | 32 +-- src/pass.rs | 170 +++++++++++-- src/run_ending.rs | 5 +- tests/architect_run.rs | 58 ----- 5 files changed, 594 insertions(+), 226 deletions(-) diff --git a/src/architect.rs b/src/architect.rs index 3ca647572..9683b28a3 100644 --- a/src/architect.rs +++ b/src/architect.rs @@ -1,9 +1,9 @@ -//! 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 +//! 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 @@ -26,21 +26,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 +49,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 got past its gates: 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. + Reviewed { + review: Result, + dispatched: Option, + }, } /// Why an Architect run was skipped. Its `Display` is the reason, as the @@ -142,9 +139,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 +154,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 +163,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 +174,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 +188,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 +238,116 @@ 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> { + 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 = review(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::Reviewed { review, dispatched } +} + +/// The Architect run failed, as `failed` says, before any review got +/// anywhere: nothing to dispatch. +fn failed(failed: FailedRun) -> Outcome { + Outcome::Reviewed { + 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 review(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")) + }); + } + match focus { + Some(focus) => outside.step(format!( + "starting the Architecture review of {base}, focused on: {focus}" + )), + None => outside.step(format!("starting the Architecture review of {base}")), + } + let prompt = prompt::architecture_review(base, focus); + let (reviewed, log) = outside.review(base, &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 +384,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 +415,7 @@ 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 +426,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)?; @@ -711,7 +755,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 +1052,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 +1060,281 @@ 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(crate::run::Reached { + pr_url: "https://github.com/acme/widgets/pull/12".to_string(), + goal: crate::run::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::Reviewed { 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()) + } + + /// An Architect run's calls once past its gates, up to and including + /// the start of its review of `main`, with no focus. + fn up_to_the_review() -> Vec { + vec![ + PLANS, + IDEAS, + Call::ReadySearch, + Call::HarnessCheck, + Call::Started(None), + step("starting the Architecture review of main"), + Call::Review { + base: "main".to_string(), + prompt: prompt::architecture_review("main", None), + }, + ] + } + + /// 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 = up_to_the_review(); + if pull { + expected.insert(5, Call::Pull("main".to_string())); + } + 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::Reviewed { + 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"); + }); + + assert_eq!( + calls[5], + step("starting the Architecture review of develop, focused on: the Spec run") + ); + let Call::Review { base, prompt } = &calls[6] else { + panic!("no review after its progress line: {calls:?}"); + }; + 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::Reviewed { + 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(std::path::Path::new(SESSION_LOG)), + "{:#}", + failed.error + ); + assert!( + !calls + .iter() + .any(|call| matches!(call, Call::DispatchPlan { .. })), + "{:#}", + failed.error + ); + } + } } diff --git a/src/main.rs b/src/main.rs index 430991844..c8af69b53 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::Reviewed { 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..8208a169d 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,22 @@ 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, 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, + 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 +151,34 @@ 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, + 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), + }; + 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 +212,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 +252,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. + 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 +288,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 +386,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 +502,42 @@ 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, + 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(), + }); + 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..2512d6903 100644 --- a/tests/architect_run.rs +++ b/tests/architect_run.rs @@ -679,25 +679,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 +1092,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 +2400,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()); -} From c7412019c455a0babad668c9d6a64f629dd1a591 Mon Sep 17 00:00:00 2001 From: Jacob Stephens Date: Mon, 5 Oct 2026 05:16:53 -0400 Subject: [PATCH 2/2] Review clean-ups for #415: the review's starting line follows its worktree, and clearer names The Pass seam's review call hands on the review's starting line once its worktree is ready, as before, so it follows the worktree's lines and is not printed when the worktree can't be made. The Architect run's outcome past its skip is Ran, the step before the dispatch is up_to_the_dispatch, and the tests no longer reach calls by position. Co-Authored-By: Claude Opus 5.5 --- src/architect.rs | 89 ++++++++++++++++++++++++------------------ src/command.rs | 6 +-- src/main.rs | 2 +- src/pass.rs | 18 ++++++--- tests/architect_run.rs | 3 +- 5 files changed, 69 insertions(+), 49 deletions(-) diff --git a/src/architect.rs b/src/architect.rs index 9683b28a3..fade0d3b9 100644 --- a/src/architect.rs +++ b/src/architect.rs @@ -3,8 +3,9 @@ //! 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 +//! 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 @@ -53,10 +54,10 @@ const ARCHITECT_IDEA: Label = Label::new( pub enum Outcome { /// Skipped, before any review, having done nothing. Skipped(Skipped), - /// It got past its gates: how its Architecture review ended, failing if + /// 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. - Reviewed { + Ran { review: Result, dispatched: Option, }, @@ -253,6 +254,7 @@ pub fn 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, @@ -287,7 +289,7 @@ fn run_through(outside: &mut impl Outside, architect_run: &ArchitectRun) -> Outc } Err(error) => return failed(error.into()), } - let review = review(outside, architect_run); + 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!( @@ -301,13 +303,13 @@ fn run_through(outside: &mut impl Outside, architect_run: &ArchitectRun) -> Outc } _ => None, }; - Outcome::Reviewed { review, dispatched } + 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::Reviewed { + Outcome::Ran { review: Err(failed), dispatched: None, } @@ -316,7 +318,10 @@ fn failed(failed: FailedRun) -> Outcome { /// [`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 review(outside: &mut impl Outside, architect_run: &ArchitectRun) -> Result { +fn up_to_the_dispatch( + outside: &mut impl Outside, + architect_run: &ArchitectRun, +) -> Result { let ArchitectRun { repo, base, @@ -337,14 +342,12 @@ fn review(outside: &mut impl Outside, architect_run: &ArchitectRun) -> Result outside.step(format!( - "starting the Architecture review of {base}, focused on: {focus}" - )), - None => outside.step(format!("starting the Architecture review of {base}")), - } + 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, &prompt, |outside, final_message| { + let (reviewed, log) = outside.review(base, starting, &prompt, |outside, final_message| { conclude(outside, final_message, origin, started) }); reviewed.map_err(|error| FailedRun { @@ -415,7 +418,8 @@ impl Report { } } -/// End the Architecture review 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 @@ -536,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"; @@ -1096,9 +1103,9 @@ mod tests { /// A dispatched run that ended ready for review. fn ready_for_review() -> Ended { Ended { - outcome: Ok(crate::run::Reached { + outcome: Ok(Reached { pr_url: "https://github.com/acme/widgets/pull/12".to_string(), - goal: crate::run::Goal::ReadyForReview, + goal: Goal::ReadyForReview, log: None, ticket_lines: Vec::new(), }), @@ -1110,7 +1117,7 @@ mod tests { /// 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::Reviewed { review, dispatched } = outcome else { + let Outcome::Ran { review, dispatched } = outcome else { panic!("the Architect run was skipped"); }; let review = review @@ -1119,20 +1126,15 @@ mod tests { (review, dispatched.is_some()) } - /// An Architect run's calls once past its gates, up to and including - /// the start of its review of `main`, with no focus. - fn up_to_the_review() -> Vec { - vec![ - PLANS, - IDEAS, - Call::ReadySearch, - Call::HarnessCheck, - Call::Started(None), - step("starting the Architecture review of main"), + /// 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"), ] } @@ -1187,10 +1189,17 @@ mod tests { for pull in [false, true] { let (_, calls) = architect(publishing_a_plan(), |run| run.pull = pull); - let mut expected = up_to_the_review(); + let mut expected = vec![ + PLANS, + IDEAS, + Call::ReadySearch, + Call::HarnessCheck, + Call::Started(None), + ]; if pull { - expected.insert(5, Call::Pull("main".to_string())); + expected.push(Call::Pull("main".to_string())); } + expected.extend(reviewing_main()); assert_eq!(calls[..expected.len()], expected, "pull: {pull}"); } } @@ -1201,7 +1210,7 @@ mod tests { let (outcome, calls) = architect(repo, |run| run.pull = true); - let Outcome::Reviewed { + let Outcome::Ran { review: Err(failed), dispatched: None, } = outcome @@ -1231,13 +1240,17 @@ mod tests { 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[5], + calls[review + 1], step("starting the Architecture review of develop, focused on: the Spec run") ); - let Call::Review { base, prompt } = &calls[6] else { - panic!("no review after its progress line: {calls:?}"); - }; assert_eq!(base, "develop"); assert!( prompt.contains("the base branch develop:"), @@ -1315,7 +1328,7 @@ mod tests { ] { let (outcome, calls) = architect(repo.session_log(SESSION_LOG), |_| {}); - let Outcome::Reviewed { + let Outcome::Ran { review: Err(failed), dispatched: None, } = outcome @@ -1324,7 +1337,7 @@ mod tests { }; assert_eq!( failed.log.as_deref(), - Some(std::path::Path::new(SESSION_LOG)), + Some(Path::new(SESSION_LOG)), "{:#}", 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 c8af69b53..bc55f95bf 100644 --- a/src/main.rs +++ b/src/main.rs @@ -437,7 +437,7 @@ fn architect(args: ArchitectArgs) -> ExitCode { started.built_with(&harness); started.finish(match outcome { Outcome::Skipped(skipped) => Ending::Skipped(skipped.into()), - Outcome::Reviewed { review, dispatched } => Ending::Architect { review, dispatched }, + Outcome::Ran { review, dispatched } => Ending::Architect { review, dispatched }, }) } diff --git a/src/pass.rs b/src/pass.rs index 8208a169d..9dd7c4a73 100644 --- a/src/pass.rs +++ b/src/pass.rs @@ -82,14 +82,16 @@ pub trait Outside { /// 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, 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. + /// `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); @@ -160,6 +162,7 @@ impl Outside for LaunchAndGitHub<'_> { fn review( &mut self, base: &str, + starting: String, prompt: &str, conclude: impl FnOnce(&mut Self, Option<&str>) -> Result, ) -> (Result, Option) { @@ -167,6 +170,7 @@ impl Outside for LaunchAndGitHub<'_> { 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(); @@ -255,7 +259,7 @@ mod in_memory { /// 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. + /// 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 { @@ -509,6 +513,7 @@ mod in_memory { fn review( &mut self, base: &str, + starting: String, prompt: &str, conclude: impl FnOnce(&mut Self, Option<&str>) -> Result, ) -> (Result, Option) { @@ -516,6 +521,7 @@ mod in_memory { 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), diff --git a/tests/architect_run.rs b/tests/architect_run.rs index 2512d6903..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(),