From c005d9477c13290b563962b86efef2a2b60224d4 Mon Sep 17 00:00:00 2001 From: Jacob Stephens Date: Thu, 1 Oct 2026 09:31:42 +0000 Subject: [PATCH 1/4] One module for how a Run's ending shows and how a child Run's is read back The Run ending module shows how a Run, a Spec run or a failed Architect run ended, and reads a child Run's ending back from the lines it relayed. The session-log and Base fix prefixes are private to it. A cause of several lines is read back as its first line, and only the child's own progress lines count toward its ending: the progress module now says which relayed lines those are. Co-Authored-By: Claude Opus 5.5 --- src/base_fix.rs | 4 - src/child_run.rs | 51 +++---- src/failed_run.rs | 4 - src/main.rs | 50 +----- src/progress.rs | 120 ++++++++++++--- src/run_ending.rs | 382 ++++++++++++++++++++++++++++++++++++++++++++++ 6 files changed, 508 insertions(+), 103 deletions(-) create mode 100644 src/run_ending.rs diff --git a/src/base_fix.rs b/src/base_fix.rs index 49d1223b5..cb85fdb4d 100644 --- a/src/base_fix.rs +++ b/src/base_fix.rs @@ -31,10 +31,6 @@ const LABELS: [(&str, &str); 2] = [ (READY_FOR_AGENT, "Ready for an agent to take on"), ]; -/// What starts the line on stderr saying what became of the Base fix a Run -/// took, which a Spec run reads back from a Ticket's Run. -pub const REPORT: &str = "Base fix: "; - /// What a Run asks about a Base fix, by its command or, without `base-fix` /// or `no-base-fix`, by the User config. #[derive(Debug, Clone, Copy, PartialEq, Eq)] diff --git a/src/child_run.rs b/src/child_run.rs index b0347292f..7f295e30c 100644 --- a/src/child_run.rs +++ b/src/child_run.rs @@ -10,11 +10,11 @@ use std::time::Duration; use anyhow::{Context, Result, anyhow}; use crate::args; -use crate::base_fix::{self, BaseFixAsk}; -use crate::failed_run; +use crate::base_fix::BaseFixAsk; use crate::interrupt; use crate::issue::IssueUrl; use crate::progress; +use crate::run_ending; /// How often to check whether a child Run has ended, or been interrupted. const POLL: Duration = Duration::from_millis(100); @@ -83,23 +83,17 @@ pub fn start(issue: &IssueUrl, kind: &Kind, base_fix: BaseFixAsk) -> Result Result { // Relay on its own thread, so this one can watch for an interrupt. let stderr = child.stderr.take().context("no stderr from the Run")?; - // A failed Run ends on its error, then its session log if it has one. let relay = thread::spawn(move || -> std::io::Result<_> { - let mut last_lines: [Option; 2] = [None, None]; - let mut base_fix = None; + let mut ending = run_ending::Reader::default(); for line in BufReader::new(stderr).lines() { - let line = line?; - let message = progress::relay(format_args!("#{number}"), &line).to_string(); - if let Some(report) = message.strip_prefix(base_fix::REPORT) { - base_fix = Some(report.to_string()); - } - last_lines = [last_lines[1].take(), Some(message)]; + ending.line(progress::relay(format_args!("#{number}"), &line?)); } - Ok((last_lines, base_fix)) + Ok(ending) }); let mut passed_on = false; let status = loop { @@ -120,7 +114,7 @@ pub fn wait(number: u64, mut child: Child) -> Result { } thread::sleep(POLL); }; - let (last_lines, base_fix) = relay + let ending = relay .join() .map_err(|_| anyhow!("the relay of the Run for #{number} panicked"))? .with_context(|| format!("could not read the Run for #{number}"))?; @@ -131,25 +125,16 @@ pub fn wait(number: u64, mut child: Child) -> Result { .context("no stdout from the Run")? .read_to_string(&mut stdout) .with_context(|| format!("could not read the Run for #{number}"))?; - if status.success() { - return Ok(Ended::Reached { - pr_url: stdout.lines().last().map(String::from), - base_fix, - }); - } - if interrupt::requested() { - return Ok(Ended::Interrupted); - } - let [before, last] = last_lines; - let (cause, log) = match last { - Some(last) => match last.strip_prefix(failed_run::SESSION_LOG) { - Some(log) => (before, Some(log.to_string())), - None => (Some(last), None), + let ending = ending.finish(&stdout, status.success()); + Ok(match ending.outcome { + Ok(pr_url) => Ended::Reached { + pr_url, + base_fix: ending.base_fix, + }, + Err(_) if interrupt::requested() => Ended::Interrupted, + Err(failure) => Ended::Failed { + cause: failure.cause.unwrap_or_else(|| format!("the Run {status}")), + log: failure.log, }, - None => (None, None), - }; - Ok(Ended::Failed { - cause: cause.unwrap_or_else(|| format!("the Run {status}")), - log, }) } diff --git a/src/failed_run.rs b/src/failed_run.rs index 901934a81..d051e105a 100644 --- a/src/failed_run.rs +++ b/src/failed_run.rs @@ -13,10 +13,6 @@ use crate::issue::IssueUrl; use crate::progress; use crate::worktree::Worktree; -/// What starts the line naming a Failed run's session log on stderr, which -/// a Spec run reads back from a Ticket's Run. -pub const SESSION_LOG: &str = "session log: "; - /// Why a Run did not end with a ready PR, and what the user should see. pub struct FailedRun { pub error: anyhow::Error, diff --git a/src/main.rs b/src/main.rs index a7566639e..24216f610 100644 --- a/src/main.rs +++ b/src/main.rs @@ -23,6 +23,7 @@ mod prompts_page; mod questions; mod resend_key; mod run; +mod run_ending; mod session; mod spec_run; mod update; @@ -34,9 +35,8 @@ use std::process::ExitCode; use args::{ArchitectArgs, Command, RunArgs}; use base_fix::BaseFixAsk; use config::UserConfig; -use failed_run::FailedRun; use notification::{ArchitectNotification, NotificationAsk, RunNotification}; -use run::{Ended, Goal}; +use run::Goal; use spec_run::Parallel; const HELP: &str = "\ @@ -233,7 +233,7 @@ fn main() -> ExitCode { child.as_ref(), base_fix, ); - let code = run_outcome(&ended); + let code = run_ending::show(&ended); if let Some(notification) = notification { notification.send(&ended); } @@ -286,14 +286,14 @@ fn architect(args: ArchitectArgs) -> ExitCode { _ => None, }; let code = match (&reviewed, &dispatched) { - (_, Some(ended)) => run_outcome(ended), + (_, Some(ended)) => run_ending::show(ended), (Ok(outcome), None) => { // Also on stderr, so the outcome shows even when stdout is captured. progress::step(format_args!("{outcome}")); print_url(outcome.url()); ExitCode::SUCCESS } - (Err(failed), None) => report(failed), + (Err(failed), None) => run_ending::show_failure(failed), }; if let Some(notification) = notification { notification.send(&reviewed, dispatched.as_ref()); @@ -313,29 +313,6 @@ fn asked( } } -/// How a Run or a Spec run that `ended` shows: what became of its Base fix, -/// if it took one, then its pull request's URL on stdout once it reached its -/// goal, or as a Failed run does. -fn run_outcome(ended: &Ended) -> ExitCode { - // Before the outcome, which a failed child Run's last lines are read as. - if let Some(report) = &ended.base_fix { - progress::step(format_args!("{}{report}", base_fix::REPORT)); - } - match &ended.outcome { - Ok(reached) => { - // Also on stderr, so the outcome shows even when stdout is captured. - progress::step(format_args!( - "PR {} is {}", - reached.pr_url, - reached.goal.outcome() - )); - print_url(&reached.pr_url); - ExitCode::SUCCESS - } - Err(failed) => report(failed), - } -} - /// The User config, after offering Setup where there is none, or the failure /// to exit with, its error reported. fn user_config() -> Result { @@ -350,22 +327,7 @@ fn failure(error: &anyhow::Error) -> ExitCode { ExitCode::FAILURE } -/// How a Failed run, or a failed Architect run, shows: its cause and its -/// session log on stderr, and its pull request's URL, if it left one, on -/// stdout. -fn report(failed: &FailedRun) -> ExitCode { - progress::step(format_args!("{:#}", failed.error)); - if let Some(log) = &failed.log { - progress::step(format_args!("{}{}", failed_run::SESSION_LOG, log.display())); - } - if let Some(pr_url) = &failed.pr_url { - print_url(pr_url); - } - ExitCode::FAILURE -} - -/// A pull request's URL, or the URL of the issue an Architect run ended on, -/// on stdout. A failed write, as once the terminal has closed, is ignored, so +/// The URL of the issue an Architect run ended on, on stdout. A failed write, as once the terminal has closed, is ignored, so /// the Run notification still goes. fn print_url(url: &str) { let _ = writeln!(std::io::stdout(), "{url}"); diff --git a/src/progress.rs b/src/progress.rs index 5be0dd9ba..f7f13bcdc 100644 --- a/src/progress.rs +++ b/src/progress.rs @@ -21,30 +21,65 @@ const STAMP: &str = "%H:%M:%S"; /// Print one of thirdshift's own steps, stamped with the time now. pub fn step(message: impl Display) { - write_line(Local::now().format(STAMP), message); + write_line(&stamped(message)); } -/// Print `line`, from the stderr of a child thirdshift, under `label`, -/// keeping the time the child stamped it with (or stamping it now, if it -/// has none), and return its message: the line without prefix or time. -pub fn relay(label: impl Display, line: &str) -> &str { - let line = line.strip_prefix(PREFIX).unwrap_or(line); - match split_stamp(line) { - Some((time, message)) => { - write_line(time, format_args!("{label}: {message}")); - message - } - None => { - step(format_args!("{label}: {line}")); - line - } - } +/// The progress line [`step`] prints for `message`. +pub fn stamped(message: impl Display) -> String { + progress_line(Local::now().format(STAMP), message) +} + +/// A line from the stderr of a child thirdshift, without prefix or time. +#[derive(Debug, PartialEq, Eq)] +pub enum Relayed<'a> { + /// The message of one of the child's own progress lines. + Step(&'a str), + /// A line that continues the one before it, as the later lines of an + /// error of several lines do. + Continuation(&'a str), +} + +/// Print `line`, from the stderr of a child thirdshift, under `label`, as +/// [`relayed`] gives it, and return what the line is. +pub fn relay(label: impl Display, line: &str) -> Relayed<'_> { + let (text, relayed) = relayed(label, line); + write_line(&text); + relayed } -fn write_line(time: impl Display, message: impl Display) { +/// The progress line that relays `line`, from the stderr of a child +/// thirdshift, under `label`, keeping the time the child stamped it with (or +/// stamped now, if it has none), and what the line is: one of the child's own +/// progress lines if it starts with their prefix, else a continuation. +pub fn relayed(label: impl Display, line: &str) -> (String, Relayed<'_>) { + let (rest, own) = match line.strip_prefix(PREFIX) { + Some(rest) => (rest, true), + None => (line, false), + }; + let (text, message) = match split_stamp(rest) { + Some((time, message)) => ( + progress_line(time, format_args!("{label}: {message}")), + message, + ), + None => (stamped(format_args!("{label}: {rest}")), rest), + }; + let relayed = if own { + Relayed::Step(message) + } else { + Relayed::Continuation(message) + }; + (text, relayed) +} + +/// The progress line for `message`, stamped with `time`. +fn progress_line(time: impl Display, message: impl Display) -> String { + format!("{PREFIX}{time} {message}") +} + +fn write_line(line: &str) { // Ignored if it fails, as it does once the terminal has closed: the Run // still has to clean up and send its Run notification. - let _ = writeln!(std::io::stderr(), "{PREFIX}{time} {message}"); + let _ = writeln!(std::io::stderr(), "{line}"); } /// A line without its prefix split into its `HH:MM:SS` stamp and message, @@ -569,6 +604,55 @@ mod tests { } } + #[test] + fn a_relayed_line_is_a_progress_line_or_continues_the_one_before() { + assert_eq!( + relayed("#21", "thirdshift: 12:14:49 could not push issue-21"), + ( + "thirdshift: 12:14:49 #21: could not push issue-21".to_string(), + Relayed::Step("could not push issue-21") + ) + ); + + // Stamped now, having no time of its own. + let (line, relayed) = relayed("#21", "hint: fetch first"); + let (_, message) = split_stamp(line.strip_prefix(PREFIX).unwrap()).unwrap(); + assert_eq!(message, "#21: hint: fetch first"); + assert_eq!(relayed, Relayed::Continuation("hint: fetch first")); + } + + #[test] + fn only_a_line_that_starts_with_the_prefix_is_a_progress_line() { + for (line, message) in [ + ("12:14:49 thirdshift: pushing", "thirdshift: pushing"), + ( + " thirdshift: 12:14:49 pushing", + " thirdshift: 12:14:49 pushing", + ), + ("", ""), + ] { + assert_eq!( + relayed("#21", line).1, + Relayed::Continuation(message), + "{line}" + ); + } + } + + #[test] + fn a_step_and_its_relay_are_one_progress_line_each() { + let line = stamped("pushing issue-7"); + let (time, message) = split_stamp(line.strip_prefix(PREFIX).unwrap()).unwrap(); + assert_eq!(message, "pushing issue-7"); + assert_eq!( + relayed("#7", &line), + ( + format!("thirdshift: {time} #7: pushing issue-7"), + Relayed::Step("pushing issue-7") + ) + ); + } + #[test] fn unknown_and_malformed_lines_are_skipped() { let mut progress = Progress::default(); diff --git a/src/run_ending.rs b/src/run_ending.rs new file mode 100644 index 000000000..d87994f0a --- /dev/null +++ b/src/run_ending.rs @@ -0,0 +1,382 @@ +//! A Run's ending, both ways: how a Run, a Spec run or a failed Architect +//! run shows how it ended, and how a Run reads back the ending of a child +//! Run it started. The lines a Run prints are all a child Run tells what +//! started it (ADR-0006), so which line carries what is known only here. + +use std::io::Write; +use std::process::ExitCode; + +use crate::failed_run::FailedRun; +use crate::progress::{self, Relayed}; +use crate::run::Ended; + +/// What starts the line naming a Failed run's session log. +const SESSION_LOG: &str = "session log: "; + +/// What starts the line saying what became of the Base fix a Run took. +const BASE_FIX: &str = "Base fix: "; + +/// Show how a Run or a Spec run `ended`: on stderr, what became of its Base +/// fix, if it took one, then its pull request's outcome once it reached its +/// goal, or as a Failed run does (see [`show_failure`]); its pull request's +/// URL on stdout. Returns its exit code. +pub fn show(ended: &Ended) -> ExitCode { + Shown::of(ended).print() +} + +/// Show a Failed run, or a failed Architect run: its cause and its session +/// log on stderr, and its pull request's URL, if it left one, on stdout. +/// Returns its exit code. +pub fn show_failure(failed: &FailedRun) -> ExitCode { + Shown::of_failure(failed).print() +} + +/// An ending as it shows. +struct Shown { + /// The messages of its progress lines on stderr, in order. + steps: Vec, + /// The pull request's URL on stdout. + url: Option, + /// Whether it exits 0. + reached: bool, +} + +impl Shown { + fn of(ended: &Ended) -> Self { + let mut shown = match &ended.outcome { + Ok(reached) => Shown { + // Also on stderr, so the outcome shows even when stdout is + // captured. + steps: vec![format!( + "PR {} is {}", + reached.pr_url, + reached.goal.outcome() + )], + url: Some(reached.pr_url.clone()), + reached: true, + }, + Err(failed) => Shown::of_failure(failed), + }; + if let Some(report) = &ended.base_fix { + shown.steps.insert(0, format!("{BASE_FIX}{report}")); + } + shown + } + + fn of_failure(failed: &FailedRun) -> Self { + let mut steps = vec![format!("{:#}", failed.error)]; + if let Some(log) = &failed.log { + steps.push(format!("{SESSION_LOG}{}", log.display())); + } + Shown { + steps, + url: failed.pr_url.clone(), + reached: false, + } + } + + fn print(self) -> ExitCode { + for step in self.steps { + progress::step(step); + } + if let Some(url) = self.url { + // A failed write, as once the terminal has closed, is ignored, + // so the Run notification still goes. + let _ = writeln!(std::io::stdout(), "{url}"); + } + if self.reached { + ExitCode::SUCCESS + } else { + ExitCode::FAILURE + } + } +} + +/// The ending of a child Run, as read back from what it showed. +#[derive(Debug, PartialEq, Eq)] +pub struct ChildEnding { + /// Its pull request's URL, if it showed one, once it reached its goal, + /// or how it failed. + pub outcome: Result, ChildFailure>, + /// What became of the Base fix it took, if any. + pub base_fix: Option, +} + +/// How a child Run failed, as read back from what it showed. +#[derive(Debug, PartialEq, Eq)] +pub struct ChildFailure { + /// The first line of its cause, or none if it showed nothing. + pub cause: Option, + pub log: Option, +} + +/// Reads a child Run's ending back from what it shows: each line of its +/// stderr, as it is relayed, then how it exited. Only the child's own +/// progress lines count: a line that continues one, as the later lines of a +/// cause do, tells nothing, and a line the child relayed from a Run it +/// started itself starts with that Run's number, so it is read as neither a +/// session log nor a Base fix. +#[derive(Default)] +pub struct Reader { + /// The last progress line that names neither a session log nor a Base + /// fix: once a failed child has shown its ending, its cause. + cause: Option, + /// The session log named since that line. + log: Option, + base_fix: Option, +} + +impl Reader { + /// Take in the next line of the child's stderr. + pub fn line(&mut self, relayed: Relayed<'_>) { + let Relayed::Step(message) = relayed else { + return; + }; + if let Some(report) = message.strip_prefix(BASE_FIX) { + self.base_fix = Some(report.to_string()); + } else if let Some(log) = message.strip_prefix(SESSION_LOG) { + self.log = Some(log.to_string()); + } else { + self.cause = Some(message.to_string()); + self.log = None; + } + } + + /// How the child ended, given its `stdout` and whether it `exited_zero`. + pub fn finish(self, stdout: &str, exited_zero: bool) -> ChildEnding { + let outcome = if exited_zero { + Ok(stdout.lines().last().map(String::from)) + } else { + Err(ChildFailure { + cause: self.cause, + log: self.log, + }) + }; + ChildEnding { + outcome, + base_fix: self.base_fix, + } + } +} + +#[cfg(test)] +mod tests { + use std::path::PathBuf; + + use anyhow::anyhow; + + use super::*; + use crate::run::{Goal, Reached}; + + const PR: &str = "https://github.com/acme/widgets/pull/31"; + const LOG: &str = "/home/me/.thirdshift/logs/widgets-21-implement.jsonl"; + const BASE_FIX_ISSUE: &str = "https://github.com/acme/widgets/issues/8"; + + fn reached(goal: Goal) -> Ended { + Ended { + outcome: Ok(Reached { + pr_url: PR.to_string(), + goal, + log: Some(PathBuf::from(LOG)), + ticket_lines: Vec::new(), + }), + base_fix: None, + } + } + + fn failed(cause: &str, log: Option<&str>, pr_url: Option<&str>) -> Ended { + Ended { + outcome: Err(FailedRun { + error: anyhow!("{cause}"), + pr_url: pr_url.map(String::from), + log: log.map(PathBuf::from), + interrupted: false, + ticket_lines: Vec::new(), + }), + base_fix: None, + } + } + + fn with_base_fix(ended: Ended, became: &str) -> Ended { + Ended { + base_fix: Some(format!("{BASE_FIX_ISSUE} {became}")), + ..ended + } + } + + /// The lines on the stderr of a Run that ends as `shown`. + fn stderr(shown: &Shown) -> Vec { + shown + .steps + .iter() + .flat_map(|step| { + progress::stamped(step) + .lines() + .map(String::from) + .collect::>() + }) + .collect() + } + + /// What a Run reads back from a child Run that `ended`, having first + /// put the lines `before` on its stderr. + fn read_back_after(before: &[String], ended: &Ended) -> ChildEnding { + let shown = Shown::of(ended); + let mut reader = Reader::default(); + for line in before.iter().chain(&stderr(&shown)) { + reader.line(progress::relayed("#21", line).1); + } + let stdout = shown + .url + .iter() + .map(|url| format!("{url}\n")) + .collect::(); + reader.finish(&stdout, shown.reached) + } + + fn read_back(ended: &Ended) -> ChildEnding { + read_back_after(&[], ended) + } + + fn failure(cause: &str, log: Option<&str>) -> Result, ChildFailure> { + Err(ChildFailure { + cause: Some(cause.to_string()), + log: log.map(String::from), + }) + } + + #[test] + fn a_run_that_reached_its_goal_reads_back_as_its_pull_request() { + for goal in [Goal::ReadyForReview, Goal::Merged] { + assert_eq!( + read_back(&reached(goal)), + ChildEnding { + outcome: Ok(Some(PR.to_string())), + base_fix: None, + } + ); + } + } + + #[test] + fn a_run_that_reached_its_goal_after_a_base_fix_reads_back_with_what_became_of_it() { + assert_eq!( + read_back(&with_base_fix(reached(Goal::Merged), "merged")), + ChildEnding { + outcome: Ok(Some(PR.to_string())), + base_fix: Some(format!("{BASE_FIX_ISSUE} merged")), + } + ); + } + + #[test] + fn a_failed_run_reads_back_as_its_cause_and_its_session_log() { + assert_eq!( + read_back(&failed("claude exited 1", Some(LOG), None)), + ChildEnding { + outcome: failure("claude exited 1", Some(LOG)), + base_fix: None, + } + ); + } + + #[test] + fn a_failed_run_with_no_session_log_reads_back_as_its_cause() { + assert_eq!( + read_back(&failed("origin mismatch", None, None)), + ChildEnding { + outcome: failure("origin mismatch", None), + base_fix: None, + } + ); + } + + #[test] + fn a_failed_run_that_left_a_pull_request_open_reads_back_as_a_failure() { + assert_eq!( + read_back(&failed("CI red on test", Some(LOG), Some(PR))), + ChildEnding { + outcome: failure("CI red on test", Some(LOG)), + base_fix: None, + } + ); + } + + #[test] + fn a_cause_of_several_lines_reads_back_as_its_first_line() { + let cause = "git push failed: exit status: 1\n\ + error: failed to push some refs\n\ + hint: fetch first"; + for log in [Some(LOG), None] { + assert_eq!( + read_back(&failed(cause, log, None)), + ChildEnding { + outcome: failure("git push failed: exit status: 1", log), + base_fix: None, + } + ); + } + } + + #[test] + fn a_run_that_failed_after_a_base_fix_reads_back_with_what_became_of_it() { + let cause = format!("Base fix {BASE_FIX_ISSUE} failed: claude exited 1"); + assert_eq!( + read_back(&with_base_fix( + failed(&cause, Some(LOG), Some(PR)), + "not merged" + )), + ChildEnding { + outcome: failure(&cause, Some(LOG)), + base_fix: Some(format!("{BASE_FIX_ISSUE} not merged")), + } + ); + } + + #[test] + fn lines_relayed_from_a_child_of_its_own_are_not_its_ending() { + // A Ticket's Run relays its Base fix, which took a Base fix of its + // own for the sake of the test, then ends with none of what it + // relayed. + let base_fix = with_base_fix( + failed( + "claude exited 1\nno such model", + Some("/logs/8.jsonl"), + None, + ), + "not merged", + ); + let relayed: Vec = stderr(&Shown::of(&base_fix)) + .iter() + .map(|line| progress::relayed("#8", line).0) + .collect(); + assert_eq!( + read_back_after(&relayed, &failed("interrupted", None, None)), + ChildEnding { + outcome: failure("interrupted", None), + base_fix: None, + } + ); + assert_eq!( + read_back_after(&relayed, &reached(Goal::Merged)), + ChildEnding { + outcome: Ok(Some(PR.to_string())), + base_fix: None, + } + ); + } + + #[test] + fn a_child_that_showed_nothing_reads_back_with_no_cause() { + assert_eq!( + Reader::default().finish("", false), + ChildEnding { + outcome: Err(ChildFailure { + cause: None, + log: None, + }), + base_fix: None, + } + ); + } +} From 76a8a158d7f01d8c9eae4ef6fa5899edd13e3a90 Mon Sep 17 00:00:00 2001 From: Jacob Stephens Date: Thu, 1 Oct 2026 09:38:10 +0000 Subject: [PATCH 2/4] Address review: relayed lines are never a child Run's cause, the Architect run's ending shows through the Run ending module, clearer names in the relay Co-Authored-By: Claude Opus 5.5 --- src/child_run.rs | 10 ++-- src/main.rs | 14 +---- src/progress.rs | 150 ++++++++++++++++++++++++++-------------------- src/run_ending.rs | 80 ++++++++++++++++--------- 4 files changed, 141 insertions(+), 113 deletions(-) diff --git a/src/child_run.rs b/src/child_run.rs index 7f295e30c..b79033a43 100644 --- a/src/child_run.rs +++ b/src/child_run.rs @@ -89,11 +89,11 @@ pub fn wait(number: u64, mut child: Child) -> Result { // Relay on its own thread, so this one can watch for an interrupt. let stderr = child.stderr.take().context("no stderr from the Run")?; let relay = thread::spawn(move || -> std::io::Result<_> { - let mut ending = run_ending::Reader::default(); + let mut reader = run_ending::Reader::default(); for line in BufReader::new(stderr).lines() { - ending.line(progress::relay(format_args!("#{number}"), &line?)); + reader.read(progress::relay(number, &line?)); } - Ok(ending) + Ok(reader) }); let mut passed_on = false; let status = loop { @@ -114,7 +114,7 @@ pub fn wait(number: u64, mut child: Child) -> Result { } thread::sleep(POLL); }; - let ending = relay + let reader = relay .join() .map_err(|_| anyhow!("the relay of the Run for #{number} panicked"))? .with_context(|| format!("could not read the Run for #{number}"))?; @@ -125,7 +125,7 @@ pub fn wait(number: u64, mut child: Child) -> Result { .context("no stdout from the Run")? .read_to_string(&mut stdout) .with_context(|| format!("could not read the Run for #{number}"))?; - let ending = ending.finish(&stdout, status.success()); + let ending = reader.finish(&stdout, status.success()); Ok(match ending.outcome { Ok(pr_url) => Ended::Reached { pr_url, diff --git a/src/main.rs b/src/main.rs index 24216f610..21f842ce4 100644 --- a/src/main.rs +++ b/src/main.rs @@ -29,7 +29,6 @@ mod spec_run; mod update; mod worktree; -use std::io::Write; use std::process::ExitCode; use args::{ArchitectArgs, Command, RunArgs}; @@ -287,12 +286,7 @@ fn architect(args: ArchitectArgs) -> ExitCode { }; let code = match (&reviewed, &dispatched) { (_, Some(ended)) => run_ending::show(ended), - (Ok(outcome), None) => { - // Also on stderr, so the outcome shows even when stdout is captured. - progress::step(format_args!("{outcome}")); - print_url(outcome.url()); - ExitCode::SUCCESS - } + (Ok(outcome), None) => run_ending::show_architect(outcome), (Err(failed), None) => run_ending::show_failure(failed), }; if let Some(notification) = notification { @@ -327,12 +321,6 @@ fn failure(error: &anyhow::Error) -> ExitCode { ExitCode::FAILURE } -/// The URL of the issue an Architect run ended on, on stdout. A failed write, as once the terminal has closed, is ignored, so -/// the Run notification still goes. -fn print_url(url: &str) { - let _ = writeln!(std::io::stdout(), "{url}"); -} - /// The end of a command other than a Run: the line that says how it went, /// or its error, on stderr. fn outcome(result: anyhow::Result) -> ExitCode { diff --git a/src/progress.rs b/src/progress.rs index f7f13bcdc..b212abdb7 100644 --- a/src/progress.rs +++ b/src/progress.rs @@ -29,46 +29,58 @@ pub fn stamped(message: impl Display) -> String { progress_line(Local::now().format(STAMP), message) } -/// A line from the stderr of a child thirdshift, without prefix or time. +/// What a line from the stderr of a child thirdshift is. #[derive(Debug, PartialEq, Eq)] -pub enum Relayed<'a> { - /// The message of one of the child's own progress lines. - Step(&'a str), +pub enum ChildLine<'a> { + /// One of the child's own progress lines, with its message: the line + /// without prefix or time. + Own(&'a str), + /// A progress line the child relayed from a child of its own. + Relayed, /// A line that continues the one before it, as the later lines of an /// error of several lines do. - Continuation(&'a str), + Continuation, } -/// Print `line`, from the stderr of a child thirdshift, under `label`, as -/// [`relayed`] gives it, and return what the line is. -pub fn relay(label: impl Display, line: &str) -> Relayed<'_> { - let (text, relayed) = relayed(label, line); - write_line(&text); - relayed +impl<'a> ChildLine<'a> { + /// What `line` is: a progress line if it starts with their prefix, + /// relayed if its message then starts as [`relayed`] starts one. + pub fn of(line: &'a str) -> Self { + let Some(rest) = line.strip_prefix(PREFIX) else { + return ChildLine::Continuation; + }; + let message = split_stamp(rest).map_or(rest, |(_, message)| message); + let relayed = message + .strip_prefix('#') + .and_then(|rest| rest.split_once(": ")) + .is_some_and(|(number, _)| { + !number.is_empty() && number.bytes().all(|byte| byte.is_ascii_digit()) + }); + if relayed { + ChildLine::Relayed + } else { + ChildLine::Own(message) + } + } } -/// The progress line that relays `line`, from the stderr of a child -/// thirdshift, under `label`, keeping the time the child stamped it with (or -/// stamped now, if it has none), and what the line is: one of the child's own -/// progress lines if it starts with their prefix, else a continuation. -pub fn relayed(label: impl Display, line: &str) -> (String, Relayed<'_>) { - let (rest, own) = match line.strip_prefix(PREFIX) { - Some(rest) => (rest, true), - None => (line, false), - }; - let (text, message) = match split_stamp(rest) { - Some((time, message)) => ( - progress_line(time, format_args!("{label}: {message}")), - message, - ), - None => (stamped(format_args!("{label}: {rest}")), rest), - }; - let relayed = if own { - Relayed::Step(message) - } else { - Relayed::Continuation(message) - }; - (text, relayed) +/// Print `line`, from the stderr of the child thirdshift running issue +/// `number`, as [`relayed`] gives it, and return what the line is. +pub fn relay(number: u64, line: &str) -> ChildLine<'_> { + write_line(&relayed(number, line)); + ChildLine::of(line) +} + +/// The progress line that relays `line`, from the stderr of the child +/// thirdshift running issue `number`: its message after `#: `, +/// keeping the time the child stamped it with (or stamped now, if it has +/// none). +pub fn relayed(number: u64, line: &str) -> String { + let rest = line.strip_prefix(PREFIX).unwrap_or(line); + match split_stamp(rest) { + Some((time, message)) => progress_line(time, format_args!("#{number}: {message}")), + None => stamped(format_args!("#{number}: {rest}")), + } } /// The progress line for `message`, stamped with `time`. @@ -605,52 +617,58 @@ mod tests { } #[test] - fn a_relayed_line_is_a_progress_line_or_continues_the_one_before() { + fn a_relayed_line_keeps_its_time_under_the_issues_number() { assert_eq!( - relayed("#21", "thirdshift: 12:14:49 could not push issue-21"), - ( - "thirdshift: 12:14:49 #21: could not push issue-21".to_string(), - Relayed::Step("could not push issue-21") - ) + relayed(21, "thirdshift: 12:14:49 could not push issue-21"), + "thirdshift: 12:14:49 #21: could not push issue-21" ); + } - // Stamped now, having no time of its own. - let (line, relayed) = relayed("#21", "hint: fetch first"); + #[test] + fn a_relayed_line_with_no_time_is_stamped_now() { + let line = relayed(21, "hint: fetch first"); let (_, message) = split_stamp(line.strip_prefix(PREFIX).unwrap()).unwrap(); assert_eq!(message, "#21: hint: fetch first"); - assert_eq!(relayed, Relayed::Continuation("hint: fetch first")); } #[test] - fn only_a_line_that_starts_with_the_prefix_is_a_progress_line() { - for (line, message) in [ - ("12:14:49 thirdshift: pushing", "thirdshift: pushing"), - ( - " thirdshift: 12:14:49 pushing", - " thirdshift: 12:14:49 pushing", - ), - ("", ""), + fn a_childs_line_that_starts_with_the_prefix_is_its_own_progress_line() { + assert_eq!( + ChildLine::of("thirdshift: 12:14:49 could not push issue-21"), + ChildLine::Own("could not push issue-21") + ); + assert_eq!( + ChildLine::of(&stamped("#21 failed: claude exited 1")), + ChildLine::Own("#21 failed: claude exited 1") + ); + } + + #[test] + fn a_childs_line_without_the_prefix_continues_the_one_before() { + for line in [ + "hint: fetch first", + "12:14:49 thirdshift: pushing", + " thirdshift: 12:14:49 pushing", + "#8: claude exited 1", + "", ] { - assert_eq!( - relayed("#21", line).1, - Relayed::Continuation(message), - "{line}" - ); + assert_eq!(ChildLine::of(line), ChildLine::Continuation, "{line}"); } } #[test] - fn a_step_and_its_relay_are_one_progress_line_each() { - let line = stamped("pushing issue-7"); - let (time, message) = split_stamp(line.strip_prefix(PREFIX).unwrap()).unwrap(); - assert_eq!(message, "pushing issue-7"); - assert_eq!( - relayed("#7", &line), - ( - format!("thirdshift: {time} #7: pushing issue-7"), - Relayed::Step("pushing issue-7") - ) - ); + fn a_line_a_child_relayed_is_not_its_own_progress_line() { + for line in [ + "thirdshift: 12:14:49 claude exited 1", + "hint: fetch first", + "thirdshift: 12:14:49 #9: relayed before", + ] { + assert_eq!( + ChildLine::of(&relayed(8, line)), + ChildLine::Relayed, + "{line}" + ); + } } #[test] diff --git a/src/run_ending.rs b/src/run_ending.rs index d87994f0a..65399bf3d 100644 --- a/src/run_ending.rs +++ b/src/run_ending.rs @@ -1,13 +1,14 @@ -//! A Run's ending, both ways: how a Run, a Spec run or a failed Architect -//! run shows how it ended, and how a Run reads back the ending of a child -//! Run it started. The lines a Run prints are all a child Run tells what -//! started it (ADR-0006), so which line carries what is known only here. +//! A Run's ending, both ways: how a Run, a Spec run or an Architect run +//! shows how it ended, and how a Run reads back the ending of a child Run it +//! started. The lines a Run prints are all a child Run tells what started +//! it (ADR-0006), so which line carries what is known only here. use std::io::Write; use std::process::ExitCode; +use crate::architect; use crate::failed_run::FailedRun; -use crate::progress::{self, Relayed}; +use crate::progress::{self, ChildLine}; use crate::run::Ended; /// What starts the line naming a Failed run's session log. @@ -31,14 +32,28 @@ pub fn show_failure(failed: &FailedRun) -> ExitCode { Shown::of_failure(failed).print() } +/// Show an Architect run that ended on an issue, having dispatched no run: +/// the line that says how it ended on stderr, and the issue's URL on stdout. +/// Returns its exit code. +pub fn show_architect(outcome: &architect::Outcome) -> ExitCode { + Shown { + // Also on stderr, so the outcome shows even when stdout is captured. + steps: vec![outcome.to_string()], + url: Some(outcome.url().to_string()), + success: true, + } + .print() +} + /// An ending as it shows. struct Shown { /// The messages of its progress lines on stderr, in order. steps: Vec, - /// The pull request's URL on stdout. + /// The URL on stdout: the pull request's, or that of the issue an + /// Architect run ended on. url: Option, /// Whether it exits 0. - reached: bool, + success: bool, } impl Shown { @@ -53,7 +68,7 @@ impl Shown { reached.goal.outcome() )], url: Some(reached.pr_url.clone()), - reached: true, + success: true, }, Err(failed) => Shown::of_failure(failed), }; @@ -71,7 +86,7 @@ impl Shown { Shown { steps, url: failed.pr_url.clone(), - reached: false, + success: false, } } @@ -84,7 +99,7 @@ impl Shown { // so the Run notification still goes. let _ = writeln!(std::io::stdout(), "{url}"); } - if self.reached { + if self.success { ExitCode::SUCCESS } else { ExitCode::FAILURE @@ -113,9 +128,8 @@ pub struct ChildFailure { /// Reads a child Run's ending back from what it shows: each line of its /// stderr, as it is relayed, then how it exited. Only the child's own /// progress lines count: a line that continues one, as the later lines of a -/// cause do, tells nothing, and a line the child relayed from a Run it -/// started itself starts with that Run's number, so it is read as neither a -/// session log nor a Base fix. +/// cause do, tells nothing, nor does a line the child relayed from a Run it +/// started itself. #[derive(Default)] pub struct Reader { /// The last progress line that names neither a session log nor a Base @@ -127,9 +141,9 @@ pub struct Reader { } impl Reader { - /// Take in the next line of the child's stderr. - pub fn line(&mut self, relayed: Relayed<'_>) { - let Relayed::Step(message) = relayed else { + /// Read the next line of the child's stderr. + pub fn read(&mut self, line: ChildLine<'_>) { + let ChildLine::Own(message) = line else { return; }; if let Some(report) = message.strip_prefix(BASE_FIX) { @@ -224,14 +238,14 @@ mod tests { let shown = Shown::of(ended); let mut reader = Reader::default(); for line in before.iter().chain(&stderr(&shown)) { - reader.line(progress::relayed("#21", line).1); + reader.read(ChildLine::of(line)); } let stdout = shown .url .iter() .map(|url| format!("{url}\n")) .collect::(); - reader.finish(&stdout, shown.reached) + reader.finish(&stdout, shown.success) } fn read_back(ended: &Ended) -> ChildEnding { @@ -348,7 +362,7 @@ mod tests { ); let relayed: Vec = stderr(&Shown::of(&base_fix)) .iter() - .map(|line| progress::relayed("#8", line).0) + .map(|line| progress::relayed(8, line)) .collect(); assert_eq!( read_back_after(&relayed, &failed("interrupted", None, None)), @@ -368,15 +382,23 @@ mod tests { #[test] fn a_child_that_showed_nothing_reads_back_with_no_cause() { - assert_eq!( - Reader::default().finish("", false), - ChildEnding { - outcome: Err(ChildFailure { - cause: None, - log: None, - }), - base_fix: None, - } - ); + let nothing = ChildEnding { + outcome: Err(ChildFailure { + cause: None, + log: None, + }), + base_fix: None, + }; + assert_eq!(Reader::default().finish("", false), nothing); + + // Nor is what it relayed from a child of its own what it showed. + let mut reader = Reader::default(); + for line in stderr(&Shown::of(&with_base_fix( + failed("claude exited 1", Some(LOG), None), + "not merged", + ))) { + reader.read(ChildLine::of(&progress::relayed(8, &line))); + } + assert_eq!(reader.finish("", false), nothing); } } From 07d073c41c3cd1136dc41129bb557be718293651 Mon Sep 17 00:00:00 2001 From: Jacob Stephens Date: Thu, 1 Oct 2026 09:45:33 +0000 Subject: [PATCH 3/4] thirdshift: failed run (CI red on 76a8a15 and the Repair found nothing to fix on the branch) 2026-10-01T09:45:33Z, host tracewake. Uncommitted work at the time of failure is included in this commit. From 3e233d3152c521217e6a7b3e2082b6d79d6ca1dc Mon Sep 17 00:00:00 2001 From: Jacob Stephens Date: Thu, 1 Oct 2026 17:49:09 +0000 Subject: [PATCH 4/4] Address review: the Run ending module says what it leaves to the Base fix and progress modules, a clearer name for a line from a child's own child Co-Authored-By: Claude Opus 5.5 --- src/base_fix.rs | 4 +--- src/progress.rs | 4 ++-- src/run_ending.rs | 4 +++- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/base_fix.rs b/src/base_fix.rs index f22b6c815..643652014 100644 --- a/src/base_fix.rs +++ b/src/base_fix.rs @@ -71,9 +71,7 @@ pub struct Advice { } impl Advice { - /// Whether `message`, a line a Run printed on stderr, is one of these: - /// a child Run's is relayed, but is neither its cause nor its session - /// log. + /// Whether `message`, a line a Run printed on stderr, is one of these. pub fn is_line(message: &str) -> bool { [BASE_CHECK, RETRY_WITH, OR_SET].iter().any(|label| { message diff --git a/src/progress.rs b/src/progress.rs index b212abdb7..c8f6dc443 100644 --- a/src/progress.rs +++ b/src/progress.rs @@ -50,13 +50,13 @@ impl<'a> ChildLine<'a> { return ChildLine::Continuation; }; let message = split_stamp(rest).map_or(rest, |(_, message)| message); - let relayed = message + let from_its_own_child = message .strip_prefix('#') .and_then(|rest| rest.split_once(": ")) .is_some_and(|(number, _)| { !number.is_empty() && number.bytes().all(|byte| byte.is_ascii_digit()) }); - if relayed { + if from_its_own_child { ChildLine::Relayed } else { ChildLine::Own(message) diff --git a/src/run_ending.rs b/src/run_ending.rs index b43dbce59..bb73832ba 100644 --- a/src/run_ending.rs +++ b/src/run_ending.rs @@ -1,7 +1,9 @@ //! A Run's ending, both ways: how a Run, a Spec run or an Architect run //! shows how it ended, and how a Run reads back the ending of a child Run it //! started. The lines a Run prints are all a child Run tells what started -//! it (ADR-0006), so which line carries what is known only here. +//! it (ADR-0006, ADR-0008), so which line carries what, and in what order, +//! is known only here. What a line of advice looks like is the Base fix +//! module's to say, and which lines are a child's own the progress module's. use std::io::Write; use std::process::ExitCode;