diff --git a/CONTEXT.md b/CONTEXT.md index 81cba52a6..ad2b4b61a 100644 --- a/CONTEXT.md +++ b/CONTEXT.md @@ -86,7 +86,7 @@ A **Run** asked to end with its pull request merged rather than left for review, _Avoid_: auto-merge (GitHub's own feature, which thirdshift does not use) **Run notification**: -A message thirdshift sends when a **Run** or a **Spec run** ends, whatever its outcome (ready, merged, failed or interrupted), to the address given with the email flag or the default in the **User config**. A Run or Spec run sends one only when asked to, by the flag or by the User config; a Spec run's notification lists each **Ticket**'s outcome, and a Ticket's **Run** never sends one of its own. Failing to send one never changes the outcome. +A message thirdshift sends when a **Run**, a **Spec run** or an **Architect run** ends, whatever its outcome (ready, merged, failed or interrupted; for an Architect run that dispatched nothing, plan published, idea filed, idea already filed or review failed), to the address given with the email flag or the default in the **User config**. Each sends one only when asked to, by the flag or by the User config; a Spec run's notification lists each **Ticket**'s outcome, and a Ticket's **Run** never sends one of its own. An Architect run's notification tells how its **Architecture review** ended, naming the plan or idea issue, and how the Spec run or Run it dispatched ended, which sends none of its own. Failing to send one never changes the outcome. _Avoid_: completion email, alert **User config**: diff --git a/README.md b/README.md index ad57a57d2..5520b1f4c 100644 --- a/README.md +++ b/README.md @@ -242,7 +242,7 @@ no Resend API key. Either: (a crontab line, CI secret, or a shell profile the Run's shell reads) ``` -The Credentials are read only when `RESEND_API_KEY` is unset or empty, and only by `email-test`, `setup`, and a Run that asks for a notification. A missing file just means no key from it. One that isn't valid TOML, or holds anything but a quoted `resend.key`, such as a typo like `kye`, stops the command with exit `1`, naming the file and the offending key, and nothing is sent. One that others can read is still used, with a `warning:` line on stderr saying to `chmod 600` it. Nothing is sent to check the key itself. When Resend accepts the email, it prints `accepted by Resend; check your inbox` and exits `0`; that is all it can verify, so check that the email arrives. When Resend refuses it, for example for a bad key or a sender it won't send from, it prints Resend's error text word for word, with where the key came from, and exits `1`. It gives up after 30 seconds without an answer. +The Credentials are read only when `RESEND_API_KEY` is unset or empty, and only by `email-test`, `setup`, and a Run or an Architect run that asks for a notification. A missing file just means no key from it. One that isn't valid TOML, or holds anything but a quoted `resend.key`, such as a typo like `kye`, stops the command with exit `1`, naming the file and the offending key, and nothing is sent. One that others can read is still used, with a `warning:` line on stderr saying to `chmod 600` it. Nothing is sent to check the key itself. When Resend accepts the email, it prints `accepted by Resend; check your inbox` and exits `0`; that is all it can verify, so check that the email arrives. When Resend refuses it, for example for a bad key or a sender it won't send from, it prints Resend's error text word for word, with where the key came from, and exits `1`. It gives up after 30 seconds without an answer. #### Run notifications @@ -255,6 +255,8 @@ A Run that asks for a notification, by the flag or by `email.always`, makes the A [Spec run](#spec-runs) sends at most one notification for the whole Spec, under the same rules, with its checks made once before any Ticket starts. Its subject names the Spec, its outcome is the Spec run's, and its body, after what a Run's holds (the Spec PR, if any), lists each Ticket's outcome, one line per Ticket as in the summary on stderr, such as `#21 landed with https://github.com/acme/widgets/pull/1` or `#22 blocked by #21`. The Ticket Runs inside it never send a notification of their own, whatever the User config says. +An [Architect run](#architect-runs) takes the same flags and the same `email.always`, and sends [one notification](#one-run-notification) covering its review and the run it dispatched. + A notification that can't be sent is a `warning:` line on stderr with Resend's error. It never changes the Run's outcome, stdout or exit code. ### Logs @@ -355,13 +357,14 @@ thirdshift architect # review the whole codebase, t thirdshift architect "the Spec run" # point the review at an area thirdshift architect merge parallel 2 # merge the plan's pull request, two Tickets at once thirdshift architect "the Spec run" --plan-only # publish the plan, mark it ready and stop +thirdshift architect --email you@example.com # email how the Architect run ended ``` The focus is optional free text, given as one argument, anywhere among the flags; it goes into the Session prompt. `architect` is a command only as the first argument. -`merge`, `no-merge` and `parallel ` (or `--merge`, `--no-merge` and `--parallel `) are for the run the plan is dispatched as, and mean what they do for `thirdshift `, so none of them is ever read as the focus. `--plan-only` stops the Architect run once the plan is marked ready, and dispatches nothing. +`merge`, `no-merge` and `parallel ` (or `--merge`, `--no-merge` and `--parallel `) are for the run the plan is dispatched as, and mean what they do for `thirdshift `, so none of them is ever read as the focus. `--plan-only` stops the Architect run once the plan is marked ready, and dispatches nothing. `email`, optionally followed by an address, and `no-email` (or `--email` and `--no-email`) are for the Architect run's own [Run notification](#one-run-notification), with or without `--plan-only`. The word after `email` is the address only if it contains `@`, so a focus without one is never taken for it. -A second focus, a repeated flag, `merge` with `no-merge`, `parallel` without a whole number from 1 up, any other argument starting with a dash, or an empty focus is an argument error (exit `2`). So is `merge`, `no-merge` or `parallel` with `--plan-only`, since nothing is dispatched for them to apply to. +A second focus, a repeated flag, `merge` with `no-merge`, `email` with `no-email`, `parallel` without a whole number from 1 up, any other argument starting with a dash, or an empty focus is an argument error (exit `2`). So is `merge`, `no-merge` or `parallel` with `--plan-only`, since nothing is dispatched for them to apply to. An Architect run: @@ -371,7 +374,7 @@ An Architect run: 4. Checks the plan: the issue is in this repository, is open, was created after the Architect run started, and carries no other label that says it is not agent work (`ready-for-human`, `needs-info` or `wontfix`). 5. Marks the plan ready, in one request: `needs-triage` is swapped for `ready-for-agent`, and its other labels are kept. A Spec's Tickets are left as the review labelled them. 6. Removes the worktree and the temporary plugin directory, whatever the outcome. -7. Dispatches the plan, unless given `--plan-only`, exactly as `thirdshift ` would from the same clone: a [Spec run](#spec-runs) when the plan has sub-issues, a Run otherwise. `merge` and `no-merge` apply to the Spec PR or the Run's pull request, and `parallel ` to the Spec run, as if given to that command, and the [User config](#user-config) sets what they leave unsaid: `merge.always`, `spec.parallel`, `launch.pull` and `logs.dir`. The one difference is that no [Run notification](#run-notifications) is sent, whatever `email.always` says. +7. Dispatches the plan, unless given `--plan-only`, exactly as `thirdshift ` would from the same clone: a [Spec run](#spec-runs) when the plan has sub-issues, a Run otherwise. `merge` and `no-merge` apply to the Spec PR or the Run's pull request, and `parallel ` to the Spec run, as if given to that command, and the [User config](#user-config) sets what they leave unsaid: `merge.always`, `spec.parallel`, `launch.pull` and `logs.dir`. The one difference is that it sends no [Run notification](#run-notifications) of its own, whatever `email.always` says: the Architect run sends [the one](#one-run-notification). The dispatched run's ending is the Architect run's: its exit code, its pull request's URL alone on stdout, and its last line on stderr, `PR is ready for review` or `PR is merged`. If it fails, the Architect run fails as that Failed run or Failed spec run does, with the cause and the session log on stderr and the pull request's URL on stdout if it left one. The plan stays `ready-for-agent`, for `thirdshift ` to take up again. `parallel ` on a plan that is a single Ticket fails the same way as it does for `thirdshift ` on an issue that isn't a Spec: `parallel is only for a Spec, and # has no sub-issues`, exit `1`, before any implementing, with the plan left ready to run without it. @@ -381,6 +384,13 @@ Progress lines on stderr say when the review starts, which plan it reported, whe A review session that fails or is interrupted, a final message without one of the lines the prompt asks for, or a plan that fails a check ends the Architect run as a failure: exit `1`, nothing on stdout, the cause on stderr and then the path of the session log. Nothing is dispatched and no label is changed, so a plan the review did publish stays `needs-triage` for you to finish or close. +### One Run notification + +An Architect run asked for a [Run notification](#run-notifications), by `email` or by `email.always = true` in the [User config](#user-config) without `no-email`, sends exactly one, whatever its outcome and with or without `--plan-only`. It makes a Run's checks before any other work, an address and a Resend API key, and stops with exit `1` if either is missing. The email goes after the outcome is final and printed, and a failed send is only a `warning:` line on stderr: it changes neither the exit code nor stdout. + +- **Subject**: `[thirdshift] / Architect run: `. With a dispatched run, the outcome is that run's: `ready for review`, `merged`, `failed` or `interrupted`. Without one, it is the review's: `plan published` (with `--plan-only`), `idea filed`, `idea already filed`, `review failed` (also for a plan that fails a check) or `interrupted`. The repository is left out if `origin` doesn't name one on GitHub. +- **Body**, plain text: a `Review:` line saying how the Architecture review ended, with the URL of the plan or idea issue it named (`plan published: `, `idea filed: `, `idea already filed: `, `failed` or `interrupted`); when the plan was dispatched, a `Dispatched:` line with that run's outcome; then what a Run's notification holds, for the dispatched run or else the failed review: the pull request URL (if any), the failure cause (if failed), the session log path (if any), the hostname and how long the Architect run took. After a dispatched Spec run, it ends with a line per Ticket, as a Spec run's notification does. + ### No Strong candidate Only a Strong top recommendation becomes a plan. When the review's top recommendation is Worth exploring or Speculative, it publishes no plan, and the Architect run ends in one of two ways, both a success, with or without `--plan-only`: exit `0`, with one issue's URL alone on stdout. thirdshift changes no label on that issue and dispatches nothing: there is nothing to run. diff --git a/src/architect.rs b/src/architect.rs index 97518f821..3f943163c 100644 --- a/src/architect.rs +++ b/src/architect.rs @@ -49,6 +49,16 @@ pub enum Outcome { } impl Outcome { + /// How the Architecture review ended, as the Architect run's Run + /// notification says it. + pub fn review(&self) -> &'static str { + match self { + Self::PlanReady(_) => "plan published", + Self::IdeaFiled(_) => "idea filed", + Self::AlreadyFiled(_) => "idea already filed", + } + } + /// The URL of the issue the Architect run ended on. pub fn url(&self) -> &str { match self { @@ -87,11 +97,7 @@ impl fmt::Display for Outcome { pub fn run(focus: Option<&str>, logs_dir: &Path, launch_pull: bool) -> Result { let started = Utc::now(); let timestamp = started.format("%Y%m%dT%H%M%SZ").to_string(); - let launch = Git::new(std::env::current_dir().context("no current directory")?); - - let origin = launch.run(&["config", "remote.origin.url"])?; - let repo = Repo::of_origin(&origin) - .with_context(|| format!("origin {origin} is not a GitHub repository"))?; + let (launch, origin, repo) = launch()?; preflight::check_identity(&launch)?; let base = launch .run(&["symbolic-ref", "--quiet", "--short", "HEAD"]) @@ -116,6 +122,21 @@ pub fn run(focus: Option<&str>, logs_dir: &Path, launch_pull: bool) -> Result Result<(Git, String, Repo)> { + let launch = Git::new(std::env::current_dir().context("no current directory")?); + let origin = launch.run(&["config", "remote.origin.url"])?; + let repo = Repo::of_origin(&origin) + .with_context(|| format!("origin {origin} is not a GitHub repository"))?; + Ok((launch, origin, repo)) +} + +/// The repository an Architect run started from the Launch directory is on. +pub fn repo() -> Result { + launch().map(|(_, _, repo)| repo) +} + /// The Architecture review session in `worktree`, which is removed once the /// session ends. Returns the session's final message. `log` is left at the /// most recent session's log. diff --git a/src/args.rs b/src/args.rs index 857cbea6d..67ac77431 100644 --- a/src/args.rs +++ b/src/args.rs @@ -32,6 +32,9 @@ pub struct ArchitectArgs { /// The free text that points the Architecture review at an area, if /// given. pub focus: Option, + /// What `email` or `no-email` asked for, if either was given; without + /// one, the User config decides. + pub email: Option, /// The flags for the Spec run or Run the plan is dispatched as, or none /// with [`PLAN_ONLY`], which dispatches nothing. pub dispatch: Option, @@ -144,13 +147,16 @@ pub fn parse(args: &[String]) -> Result { /// Parse the arguments after `architect`: at most one focus, and its flags, /// each at most once, in any order. [`PLAN_ONLY`] dispatches nothing, so the /// flags for the dispatched run, `merge`, `no-merge` and `parallel`, which a -/// Run takes too, can't go with it. They are never the focus, and any other -/// argument that starts with a dash is unexpected rather than a focus. +/// Run takes too, can't go with it. `email` and `no-email` are for the +/// Architect run's own Run notification, so they can, and `email` takes an +/// address as it does for a Run. None of these is ever the focus, and any +/// other argument that starts with a dash is unexpected rather than a focus. fn parse_architect(args: &[String]) -> Result { let mut focus = None; let mut plan_only = false; + let mut email = None; let mut dispatch = DispatchArgs::default(); - let mut args = args.iter(); + let mut args = args.iter().peekable(); while let Some(arg) = args.next() { match arg.as_str() { PLAN_ONLY => { @@ -163,6 +169,13 @@ fn parse_architect(args: &[String]) -> Result { "no-merge" | "--no-merge" => { ask_once(&mut dispatch.goal, Goal::ReadyForReview, arg, MERGE_FLAGS)? } + "email" | "--email" => { + let to = args.next_if(|next| is_address(next)).cloned(); + ask_once(&mut email, NotificationAsk::Send(to), arg, EMAIL_FLAGS)? + } + "no-email" | "--no-email" => { + ask_once(&mut email, NotificationAsk::Skip, arg, EMAIL_FLAGS)? + } "parallel" | "--parallel" => ask_parallel(&mut dispatch.parallel, arg, args.next())?, _ if arg.starts_with('-') => bail!("unexpected argument after architect: {arg}"), _ if focus.is_some() => bail!("unexpected argument after the focus: {arg}"), @@ -173,6 +186,7 @@ fn parse_architect(args: &[String]) -> Result { if !plan_only { return Ok(ArchitectArgs { focus, + email, dispatch: Some(dispatch), }); } @@ -184,6 +198,7 @@ fn parse_architect(args: &[String]) -> Result { } Ok(ArchitectArgs { focus, + email, dispatch: None, }) } @@ -220,7 +235,8 @@ fn ask_parallel(parallel: &mut Option, arg: &str, n: Option<&Strin } /// Is `arg`, after `email`, the address to send to? Only if it looks like -/// one, so the Issue URL is never taken for it. +/// one, so the Issue URL is never taken for it, nor is an Architect run's +/// focus unless it has an `@`. fn is_address(arg: &str) -> bool { arg.contains('@') && !arg.starts_with("https://") } @@ -331,6 +347,60 @@ mod tests { assert_eq!(architect_args(&args).focus.as_deref(), Some("the Spec run")); } + #[test] + fn architect_takes_the_email_flags_with_or_without_plan_only_and_never_as_the_focus() { + let bare = || Some(NotificationAsk::Send(None)); + let to_me = || Some(NotificationAsk::Send(Some("me@example.com".to_string()))); + let focus = || Some("the Spec run".to_string()); + for (args, email, focus) in [ + (vec!["architect"], None, None), + (vec!["architect", "email"], bare(), None), + (vec!["architect", "--email", "--plan-only"], bare(), None), + (vec!["architect", "email", "the Spec run"], bare(), focus()), + (vec!["architect", "email", "merge"], bare(), None), + (vec!["architect", "email", "me@example.com"], to_me(), None), + ( + vec!["architect", "the Spec run", "--email", "me@example.com"], + to_me(), + focus(), + ), + ( + vec!["architect", "--plan-only", "no-email"], + Some(NotificationAsk::Skip), + None, + ), + ( + vec!["architect", "--no-email", "the Spec run"], + Some(NotificationAsk::Skip), + focus(), + ), + ] { + let architect_args = architect_args(&args); + assert_eq!(architect_args.email, email, "{args:?}"); + assert_eq!(architect_args.focus, focus, "{args:?}"); + } + } + + #[test] + fn architect_rejects_contradictory_and_repeated_email_flags() { + for (args, error) in [ + ( + vec!["architect", "email", "--no-email"], + "email and no-email can't be used together", + ), + ( + vec!["architect", "no-email", "--plan-only", "--email"], + "email and no-email can't be used together", + ), + ( + vec!["architect", "--email", "me@example.com", "email"], + "repeated argument: email", + ), + ] { + assert_eq!(rejection(&args), error, "{args:?}"); + } + } + #[test] fn architect_rejects_contradictory_repeated_and_malformed_dispatch_flags() { for (args, error) in [ @@ -391,8 +461,8 @@ mod tests { "repeated argument: --plan-only", ), ( - vec!["architect", "--email", "--plan-only"], - "unexpected argument after architect: --email", + vec!["architect", "--verbose", "--plan-only"], + "unexpected argument after architect: --verbose", ), (vec!["architect", " ", "--plan-only"], "the focus is empty"), ] { diff --git a/src/config.rs b/src/config.rs index dc8f1a400..262494995 100644 --- a/src/config.rs +++ b/src/config.rs @@ -42,8 +42,8 @@ pub struct UserConfig { /// file holds no secret. #[derive(Debug, Default, PartialEq, Eq)] pub struct EmailSettings { - /// `email.always`: every Run sends a Run notification unless told - /// `no-email`. + /// `email.always`: every Run, and every Architect run, sends a Run + /// notification unless told `no-email`. pub always: bool, /// `email.to`: the address email goes to when the command names none. pub to: Option, diff --git a/src/main.rs b/src/main.rs index 72571f3bb..9ce36e965 100644 --- a/src/main.rs +++ b/src/main.rs @@ -32,7 +32,7 @@ use std::process::ExitCode; use args::{ArchitectArgs, Command, RunArgs}; use config::UserConfig; use failed_run::FailedRun; -use notification::{NotificationAsk, RunNotification}; +use notification::{ArchitectNotification, NotificationAsk, RunNotification}; use run::{Goal, Reached}; use spec_run::Parallel; @@ -87,8 +87,7 @@ a single Ticket. thirdshift then checks that the plan is open, new and not label ready-for-human, needs-info or wontfix, swaps its needs-triage label for ready-for-agent, and dispatches it as thirdshift would: a Spec run on a Spec, a Run on a single Ticket. The Architect run ends as that run does, with its exit code and its PR's URL. -merge, --no-merge and parallel apply to that run, as do the User config's defaults, -except that it sends no Run notification, whatever email.always says; +merge, --no-merge and parallel apply to that run, as do the User config's defaults; parallel fails it if the plan is a single Ticket. With --plan-only, the Architect run prints the plan's URL and stops instead, for you to read, edit and run with thirdshift , and takes none of those flags. is free text, one argument, @@ -105,6 +104,12 @@ A review that fails, is interrupted, or ends without naming one of these issues Architect run and leaves any plan it published labelled needs-triage. Start one Architect run per repository at a time: two at once may publish the same plan. +--email, --email
and --no-email ask an Architect run for its Run notification as +they do a Run, with or without --plan-only, and email.always sets the default. It sends one +for the whole Architect run, however it ends: how the review ended, with the plan or idea +issue it named, and how the run the plan was dispatched as ended, with a line on each Ticket +of a Spec run. The run the plan is dispatched as sends none of its own. + The User config, ~/.thirdshift/config.toml, sets defaults for every Run on this machine; thirdshift setup asks for your defaults and writes one listing every setting, to edit. With merge.always set, every Run is a Merge run unless given --no-merge: @@ -163,7 +168,7 @@ fn main() -> ExitCode { UserConfig::load().and_then(|config| email::send_test(to, &config.email)), ); } - Ok(Command::Architect(architect_args)) => return architect(&architect_args), + Ok(Command::Architect(architect_args)) => return architect(architect_args), Ok(Command::Run(run_args)) => run_args, Err(error) => return argument_error(format_args!("{error:#}")), }; @@ -185,11 +190,7 @@ fn main() -> ExitCode { if let Err(error) = interrupt::install() { return failure(&error); } - let notification = match email { - NotificationAsk::Send(to) => RunNotification::new(to, &config.email, &issue).map(Some), - NotificationAsk::Skip => Ok(None), - }; - let notification = match notification { + let notification = match asked(email, |to| RunNotification::new(to, &config.email, &issue)) { Ok(notification) => notification, Err(error) => return failure(&error), }; @@ -211,43 +212,72 @@ fn main() -> ExitCode { /// An Architect run: the Architecture review and its plan marked ready, then, /// unless the command asked to stop at the plan, the plan dispatched as /// `thirdshift ` with the same flags would be, whose ending is the -/// Architect run's, though it sends no Run notification. One that stops at -/// the plan, or whose review found no Strong candidate and so published no -/// plan to dispatch, puts the URL of the issue it ended on on stdout: the -/// plan, the idea issue the review filed, or the issue that already covers -/// its top recommendation. One whose review or plan fails puts the cause and -/// the session log on stderr. -fn architect(args: &ArchitectArgs) -> ExitCode { +/// Architect run's. One that stops at the plan, or whose review found no +/// Strong candidate and so published no plan to dispatch, puts the URL of the +/// issue it ended on on stdout: the plan, the idea issue the review filed, or +/// the issue that already covers its top recommendation. One whose review or +/// plan fails puts the cause and the session log on stderr. If asked, by the +/// command or the User config, it sends one Run notification, however it +/// ended; the run it dispatched sends none of its own. +fn architect(args: ArchitectArgs) -> ExitCode { let config = match user_config() { Ok(config) => config, Err(failure) => return failure, }; + // First, so no interrupt can end the Architect run once its notification + // is checked. if let Err(error) = interrupt::install() { return failure(&error); } - let outcome = match architect::run(args.focus.as_deref(), &config.logs_dir, config.launch_pull) - { - Ok(outcome) => outcome, - Err(failed) => return report(&failed), + let email = args.email.unwrap_or(config.email.default_ask()); + let notification = match asked(email, |to| ArchitectNotification::new(to, &config.email)) { + Ok(notification) => notification, + Err(error) => return failure(&error), }; - let (architect::Outcome::PlanReady(plan), Some(dispatch)) = (&outcome, &args.dispatch) else { - // Also on stderr, so the outcome shows even when stdout is captured. - progress::step(format_args!("{outcome}")); - print_url(outcome.url()); - return ExitCode::SUCCESS; + let reviewed = architect::run(args.focus.as_deref(), &config.logs_dir, config.launch_pull); + let dispatched = match (&reviewed, &args.dispatch) { + (Ok(architect::Outcome::PlanReady(plan)), Some(dispatch)) => { + progress::step(format_args!( + "dispatching the plan {url}, as thirdshift {url} would", + url = plan.url + )); + Some(run::run( + plan, + dispatch.goal.unwrap_or(config.default_goal()), + &config.logs_dir, + config.launch_pull, + Parallel::new(dispatch.parallel, config.spec_parallel), + None, + )) + } + _ => None, }; - progress::step(format_args!( - "dispatching the plan {url}, as thirdshift {url} would", - url = plan.url - )); - run_outcome(&run::run( - plan, - dispatch.goal.unwrap_or(config.default_goal()), - &config.logs_dir, - config.launch_pull, - Parallel::new(dispatch.parallel, config.spec_parallel), - None, - )) + let code = match (&reviewed, &dispatched) { + (_, Some(ended)) => run_outcome(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), + }; + if let Some(notification) = notification { + notification.send(&reviewed, dispatched.as_ref()); + } + code +} + +/// The Run notification `email` asks for, if it asks for one: what `checked` +/// makes of the address it gave, or its error if a check fails. +fn asked( + email: NotificationAsk, + checked: impl FnOnce(Option) -> anyhow::Result, +) -> anyhow::Result> { + match email { + NotificationAsk::Send(to) => checked(to).map(Some), + NotificationAsk::Skip => Ok(None), + } } /// How a Run or a Spec run that `ended` shows: its pull request's URL on diff --git a/src/notification.rs b/src/notification.rs index 2da5b25b3..fd3201cb3 100644 --- a/src/notification.rs +++ b/src/notification.rs @@ -1,22 +1,24 @@ -//! The Run notification: one email when a Run or a Spec run ends, whatever -//! its outcome, through the same checks and the same send as `email-test`. +//! The Run notification: one email when a Run, a Spec run or an Architect run +//! ends, whatever its outcome, through the same checks and the same send as +//! `email-test`. use std::path::Path; use std::time::{Duration, Instant}; use anyhow::Result; +use crate::architect; use crate::config::EmailSettings; use crate::email::Resend; use crate::failed_run::FailedRun; use crate::github; use crate::host; -use crate::issue::IssueUrl; +use crate::issue::{IssueUrl, Repo}; use crate::progress; use crate::run::Reached; -/// What a Run asks about its Run notification, by its command or, without -/// `email` or `no-email`, by the User config. +/// What a Run or an Architect run asks about its Run notification, by its +/// command or, without `email` or `no-email`, by the User config. #[derive(Debug, PartialEq, Eq)] pub enum NotificationAsk { /// Send one, to this address, else to `email.to`. @@ -57,47 +59,145 @@ impl RunNotification { /// Send the notification for the Run that `ended`. A failed send is only /// a warning: it never changes the Run's outcome. pub fn send(self, ended: &Result) { - let (outcome, pr_url, cause, log, tickets) = match ended { - Ok(reached) => ( - reached.goal.outcome(), - Some(reached.pr_url.as_str()), - None, - reached.log.as_deref(), - &reached.ticket_lines[..], - ), - Err(failed) => { - let (outcome, cause) = if failed.interrupted { - ("interrupted", None) - } else { - ("failed", Some(format!("{:#}", failed.error))) - }; - ( - outcome, - failed.pr_url.as_deref(), - cause, - failed.log.as_deref(), - &failed.ticket_lines[..], - ) - } + let ending = Ending::of(ended); + let subject = subject(&self.issue, self.title.as_deref(), ending.outcome); + send(&self.resend, &subject, self.started, None, &ending); + } +} + +/// An Architect run's Run notification, checked and waiting for the Architect +/// run to end. +pub struct ArchitectNotification { + resend: Resend, + /// The repository the Launch directory's `origin` names, if it names one. + repo: Option, + started: Instant, +} + +impl ArchitectNotification { + /// The Run notification for an Architect run starting now from the Launch + /// directory, sent to `to`, else to `email.to`. Fails, before any work, + /// with the checks [`RunNotification::new`] makes. + pub fn new(to: Option, settings: &EmailSettings) -> Result { + let started = Instant::now(); + let resend = Resend::new(to, settings)?; + Ok(ArchitectNotification { + resend, + // Left out of the subject if origin names none; the Architect + // run's own preflight reports why. + repo: architect::repo().ok(), + started, + }) + } + + /// Send the one notification for the Architect run whose Architecture + /// review ended as `reviewed` and, if its plan was dispatched, whose Spec + /// run or Run `dispatched`. A failed send is only a warning: it never + /// changes the Architect run's outcome. + pub fn send( + self, + reviewed: &Result, + dispatched: Option<&Result>, + ) { + let review = match reviewed { + Ok(outcome) => format!("{}: {}", outcome.review(), outcome.url()), + Err(failed) => failure_outcome(failed, "failed").to_string(), }; - let host = host::name(); - let body = Body { - pr_url, - cause: cause.as_deref(), - log, - host: host.as_deref().unwrap_or("unknown host"), - took: self.started.elapsed(), - tickets, + let ending = match (reviewed, dispatched) { + (_, Some(ended)) => Ending::of(ended), + (Ok(outcome), None) => Ending { + outcome: outcome.review(), + pr_url: None, + cause: None, + log: None, + tickets: &[], + }, + (Err(failed), None) => Ending { + outcome: failure_outcome(failed, "review failed"), + ..Ending::of_failure(failed) + }, + }; + let lines = ArchitectLines { + review: &review, + dispatched: dispatched.map(|_| ending.outcome), }; - let subject = subject(&self.issue, self.title.as_deref(), outcome); - if let Err(error) = self.resend.send(&subject, &body.text()) { - progress::step(format_args!( - "warning: could not send the Run notification: {error:#}" - )); + let subject = architect_subject(self.repo.as_ref(), ending.outcome); + send(&self.resend, &subject, self.started, Some(lines), &ending); + } +} + +/// How a Run, a Spec run or an Architecture review ended, as a Run +/// notification tells it. +struct Ending<'a> { + outcome: &'static str, + pr_url: Option<&'a str>, + cause: Option, + log: Option<&'a Path>, + /// In a Spec run, a line on each Ticket, as in its summary on stderr. + tickets: &'a [String], +} + +impl<'a> Ending<'a> { + fn of(ended: &'a Result) -> Self { + match ended { + Ok(reached) => Ending { + outcome: reached.goal.outcome(), + pr_url: Some(&reached.pr_url), + cause: None, + log: reached.log.as_deref(), + tickets: &reached.ticket_lines, + }, + Err(failed) => Ending::of_failure(failed), + } + } + + fn of_failure(failed: &'a FailedRun) -> Self { + Ending { + outcome: failure_outcome(failed, "failed"), + pr_url: failed.pr_url.as_deref(), + cause: (!failed.interrupted).then(|| format!("{:#}", failed.error)), + log: failed.log.as_deref(), + tickets: &failed.ticket_lines, } } } +/// `interrupted`, or `failure` for a failure that was not an interrupt. +fn failure_outcome(failed: &FailedRun, failure: &'static str) -> &'static str { + if failed.interrupted { + "interrupted" + } else { + failure + } +} + +/// Send the notification with `subject` for what `started` then and ended as +/// `ending`, in an Architect run with its `architect` lines first. A failed +/// send is only a warning. +fn send( + resend: &Resend, + subject: &str, + started: Instant, + architect: Option, + ending: &Ending, +) { + let host = host::name(); + let body = Body { + architect, + pr_url: ending.pr_url, + cause: ending.cause.as_deref(), + log: ending.log, + host: host.as_deref().unwrap_or("unknown host"), + took: started.elapsed(), + tickets: ending.tickets, + }; + if let Err(error) = resend.send(subject, &body.text()) { + progress::step(format_args!( + "warning: could not send the Run notification: {error:#}" + )); + } +} + /// `[thirdshift] /# : <outcome>`, without the title /// if it isn't known. fn subject(issue: &IssueUrl, title: Option<&str>, outcome: &str) -> String { @@ -109,8 +209,26 @@ fn subject(issue: &IssueUrl, title: Option<&str>, outcome: &str) -> String { ) } +/// `[thirdshift] <owner>/<repo> Architect run: <outcome>`, without the +/// repository if it isn't known. +fn architect_subject(repo: Option<&Repo>, outcome: &str) -> String { + let repo = repo + .map(|repo| format!(" {}/{}", repo.owner, repo.name)) + .unwrap_or_default(); + format!("[thirdshift]{repo} Architect run: {outcome}") +} + +/// What an Architect run's notification says before what a Run's does. +struct ArchitectLines<'a> { + /// How the Architecture review ended, with the issue it ended on. + review: &'a str, + /// How the Spec run or Run the plan was dispatched as ended, if it was. + dispatched: Option<&'a str>, +} + /// What the notification's plain-text body says. struct Body<'a> { + architect: Option<ArchitectLines<'a>>, pr_url: Option<&'a str>, cause: Option<&'a str>, log: Option<&'a Path>, @@ -123,6 +241,12 @@ struct Body<'a> { impl Body<'_> { fn text(&self) -> String { let mut text = String::new(); + if let Some(architect) = &self.architect { + text += &format!("Review: {}\n", architect.review); + if let Some(dispatched) = architect.dispatched { + text += &format!("Dispatched: {dispatched}\n"); + } + } if let Some(pr_url) = self.pr_url { text += &format!("Pull request: {pr_url}\n"); } @@ -180,6 +304,7 @@ mod tests { #[test] fn the_body_leaves_out_what_the_run_does_not_have() { let body = Body { + architect: None, pr_url: None, cause: Some("origin mismatch"), log: None, @@ -215,6 +340,7 @@ mod tests { "#22 blocked by #21".to_string(), ]; let body = Body { + architect: None, pr_url: None, cause: Some("Tickets not done: #21, #22"), log: None, @@ -234,6 +360,57 @@ mod tests { ); } + #[test] + fn an_architect_runs_subject_names_the_repository_if_known_and_the_outcome() { + let repo = Repo::of_origin("https://github.com/acme/widgets.git").unwrap(); + assert_eq!( + architect_subject(Some(&repo), "plan published"), + "[thirdshift] acme/widgets Architect run: plan published" + ); + assert_eq!( + architect_subject(None, "review failed"), + "[thirdshift] Architect run: review failed" + ); + } + + #[test] + fn an_architect_runs_body_starts_with_the_review_and_the_dispatched_runs_outcome() { + let body = Body { + architect: Some(ArchitectLines { + review: "plan published: https://github.com/acme/widgets/issues/8", + dispatched: Some("merged"), + }), + pr_url: Some("https://github.com/acme/widgets/pull/1"), + cause: None, + log: None, + host: "droplet-1", + took: Duration::from_secs(4), + tickets: &[], + }; + assert_eq!( + body.text(), + "Review: plan published: https://github.com/acme/widgets/issues/8\n\ + Dispatched: merged\n\ + Pull request: https://github.com/acme/widgets/pull/1\n\ + Host: droplet-1\n\ + Took: 4s\n" + ); + let body = Body { + architect: Some(ArchitectLines { + review: "idea filed: https://github.com/acme/widgets/issues/8", + dispatched: None, + }), + pr_url: None, + ..body + }; + assert_eq!( + body.text(), + "Review: idea filed: https://github.com/acme/widgets/issues/8\n\ + Host: droplet-1\n\ + Took: 4s\n" + ); + } + #[test] fn took_is_to_the_second() { for (seconds, text) in [ diff --git a/tests/architect_run.rs b/tests/architect_run.rs index b2cd431ec..f196b266c 100644 --- a/tests/architect_run.rs +++ b/tests/architect_run.rs @@ -6,11 +6,15 @@ //! run or a Run, and ends as that does. A review with no Strong candidate //! has no plan: the Architect run prints the URL of the idea issue it filed, //! or of the open issue that already covers it, and dispatches nothing. +//! Asked to, by `email` or the User config, it sends one Run notification, +//! through Resend, here a local stand-in, however it ended, and the run it +//! dispatched sends none. mod support; use std::fs; +use support::resend::ResendStandIn; use support::{REPO, RunResult, Scenario}; /// The first issue the fake agent creates: the scenario starts with issue #7. @@ -794,3 +798,370 @@ fn progress_lines_show_the_dispatch_after_the_label_swap_and_before_the_run() { result.stderr ); } + +const KEY: &str = "re_test_123"; + +const ACCEPTED: &str = r#"{"id":"49a3999c-0ce1-4ea6-ab68-afcd6dc2e794"}"#; + +/// The environment for an Architect run against `resend`, with a Resend API +/// key. +fn resend_env(resend: &ResendStandIn) -> [(&str, &str); 2] { + [ + ("THIRDSHIFT_RESEND_URL", resend.url()), + ("RESEND_API_KEY", KEY), + ] +} + +/// Run thirdshift with `args` against `resend`, with a Resend API key in the +/// environment. +fn run_with_resend(scenario: &Scenario, resend: &ResendStandIn, args: &[&str]) -> RunResult { + scenario.run_with_env(args, &resend_env(resend)) +} + +/// The subject and the text of the one Run notification `resend` received. +fn the_one_notification(resend: &ResendStandIn) -> (String, String) { + let requests = resend.requests(); + assert_eq!(requests.len(), 1, "{requests:?}"); + let body = &requests[0].body; + let part = |name: &str| body[name].as_str().unwrap().to_string(); + (part("subject"), part("text")) +} + +#[test] +fn plan_only_sends_one_notification_naming_the_plan() { + let scenario = Scenario::new(); + scenario.agent_does(AGENT_PUBLISHES_A_TICKET); + let resend = ResendStandIn::replying(200, ACCEPTED); + + let result = run_with_resend( + &scenario, + &resend, + &["architect", "--plan-only", "--email", "me@example.com"], + ); + + assert_eq!(result.code, Some(0), "stderr: {}", result.stderr); + assert_eq!(result.stdout, format!("{PLAN_URL}\n")); + assert_eq!(resend.requests()[0].body["to"], "me@example.com"); + let (subject, text) = the_one_notification(&resend); + assert_eq!( + subject, + "[thirdshift] acme/widgets Architect run: plan published" + ); + assert!( + text.starts_with(&format!("Review: plan published: {PLAN_URL}\n")), + "{text}" + ); + assert!(!text.contains("Dispatched:"), "{text}"); + assert!(text.contains("Took:"), "{text}"); +} + +#[test] +fn a_review_that_files_an_idea_sends_one_notification_naming_the_idea() { + let scenario = Scenario::new(); + scenario.agent_does( + r#" +url=$(gh issue create --title "Deepen the session module" --body "The idea" --label needs-triage) +printf 'Architecture review idea: %s\n' "$url" > "$FAKE_CLAUDE_FINAL_MESSAGE" +"#, + ); + let resend = ResendStandIn::replying(200, ACCEPTED); + + let result = run_with_resend( + &scenario, + &resend, + &["architect", "email", "me@example.com"], + ); + + assert_eq!(result.code, Some(0), "stderr: {}", result.stderr); + let (subject, text) = the_one_notification(&resend); + assert_eq!( + subject, + "[thirdshift] acme/widgets Architect run: idea filed" + ); + assert!( + text.starts_with(&format!("Review: idea filed: {PLAN_URL}\n")), + "{text}" + ); + assert!(!text.contains("Dispatched:"), "{text}"); +} + +#[test] +fn a_review_whose_idea_is_already_filed_sends_one_notification_naming_that_issue() { + let scenario = Scenario::new(); + let url = scenario.issue_url(7); + scenario.agent_does(&ends_with(&format!( + "Architecture review already filed: {url}" + ))); + let resend = ResendStandIn::replying(200, ACCEPTED); + + let result = run_with_resend( + &scenario, + &resend, + &["architect", "--email", "me@example.com", "--plan-only"], + ); + + assert_eq!(result.code, Some(0), "stderr: {}", result.stderr); + let (subject, text) = the_one_notification(&resend); + assert_eq!( + subject, + "[thirdshift] acme/widgets Architect run: idea already filed" + ); + assert!( + text.starts_with(&format!("Review: idea already filed: {url}\n")), + "{text}" + ); +} + +#[test] +fn a_failed_review_sends_one_notification_with_the_cause_and_the_session_log() { + let scenario = Scenario::new(); + scenario.agent_does(&publishes_a_ticket_then("exit 3")); + let resend = ResendStandIn::replying(200, ACCEPTED); + + let result = run_with_resend( + &scenario, + &resend, + &["architect", "--email", "me@example.com"], + ); + + assert_failed(&scenario, &result, "claude exited 3"); + let (subject, text) = the_one_notification(&resend); + assert_eq!( + subject, + "[thirdshift] acme/widgets Architect run: review failed" + ); + let log = result.stderr.lines().last().unwrap(); + let log = log.strip_prefix("thirdshift: session log: ").unwrap(); + assert!( + text.starts_with(&format!( + "Review: failed\n\ + Cause: claude exited 3\n\ + Session log: {log}\n" + )), + "{text}" + ); +} + +/// The User config of a machine where every Run and every Architect run +/// sends a Run notification. +const EMAIL_ALWAYS: &str = "[email]\nalways = true\nto = \"config@example.com\"\n"; + +#[test] +fn email_always_sends_one_notification_for_the_review_and_the_run_it_dispatched() { + let scenario = single_ticket_plan(); + scenario.user_config_is(EMAIL_ALWAYS); + let resend = ResendStandIn::replying(200, ACCEPTED); + + let result = run_with_resend(&scenario, &resend, &["architect"]); + + assert_eq!(result.code, Some(0), "stderr: {}", result.stderr); + assert_eq!(result.stdout, format!("{PR_URL}\n")); + assert_eq!(resend.requests()[0].body["to"], "config@example.com"); + let (subject, text) = the_one_notification(&resend); + assert_eq!( + subject, + "[thirdshift] acme/widgets Architect run: ready for review" + ); + assert!( + text.starts_with(&format!( + "Review: plan published: {PLAN_URL}\n\ + Dispatched: ready for review\n\ + Pull request: {PR_URL}\n" + )), + "{text}" + ); +} + +#[test] +fn a_dispatched_spec_run_sends_no_notification_of_its_own_and_its_tickets_are_in_the_one_sent() { + let scenario = spec_plan(""); + scenario.user_config_is(EMAIL_ALWAYS); + let resend = ResendStandIn::replying(200, ACCEPTED); + + let result = run_with_resend(&scenario, &resend, &["architect", "merge"]); + + assert_eq!(result.code, Some(0), "stderr: {}", result.stderr); + let (subject, text) = the_one_notification(&resend); + assert_eq!(subject, "[thirdshift] acme/widgets Architect run: merged"); + let spec_pr = pr_from(&scenario, "issue-8"); + assert!( + text.starts_with(&format!( + "Review: plan published: {PLAN_URL}\n\ + Dispatched: merged\n\ + Pull request: {}\n", + spec_pr["url"].as_str().unwrap() + )), + "{text}" + ); + let (_, tickets) = text.split_once("\nTickets:\n").expect(&text); + for ticket in [9, 10] { + let pr = pr_from(&scenario, &format!("issue-{ticket}")); + let line = format!("#{ticket} landed with {}\n", pr["url"].as_str().unwrap()); + assert!(tickets.contains(&line), "expected {line:?} in: {text}"); + } +} + +#[test] +fn a_dispatched_run_that_fails_sends_one_notification_with_its_outcome_and_cause() { + let scenario = single_ticket_plan(); + scenario.agent_does_for(8, &format!("{}exit 3\n", agent_opens_pr(8, "main"))); + let resend = ResendStandIn::replying(200, ACCEPTED); + + let result = run_with_resend( + &scenario, + &resend, + &["architect", "--email", "me@example.com"], + ); + + assert_eq!(result.code, Some(1), "stderr: {}", result.stderr); + let (subject, text) = the_one_notification(&resend); + assert_eq!(subject, "[thirdshift] acme/widgets Architect run: failed"); + assert!( + text.starts_with(&format!( + "Review: plan published: {PLAN_URL}\n\ + Dispatched: failed\n\ + Pull request: {PR_URL}\n\ + Cause: claude exited 3\n" + )), + "{text}" + ); +} + +#[test] +fn no_email_skips_the_notification_email_always_asks_for() { + let scenario = single_ticket_plan(); + scenario.user_config_is(EMAIL_ALWAYS); + let resend = ResendStandIn::replying(200, ACCEPTED); + + let result = run_with_resend(&scenario, &resend, &["architect", "--no-email"]); + + assert_eq!(result.code, Some(0), "stderr: {}", result.stderr); + assert!(resend.requests().is_empty()); +} + +#[test] +fn an_architect_run_that_is_not_asked_for_a_notification_sends_none() { + let scenario = single_ticket_plan(); + scenario.user_config_is("[email]\nto = \"config@example.com\"\n"); + let resend = ResendStandIn::replying(200, ACCEPTED); + + let result = run_with_resend(&scenario, &resend, &["architect"]); + + assert_eq!(result.code, Some(0), "stderr: {}", result.stderr); + assert!(resend.requests().is_empty()); +} + +#[test] +fn with_no_address_known_the_architect_run_stops_before_any_work_and_sends_nothing() { + let scenario = Scenario::new(); + let resend = ResendStandIn::replying(200, ACCEPTED); + + let result = run_with_resend(&scenario, &resend, &["architect", "--email"]); + + scenario.assert_rejected_before_any_work(&result, "no email address"); + assert!(resend.requests().is_empty()); +} + +#[test] +fn a_failed_send_is_a_warning_that_changes_neither_the_exit_code_nor_stdout() { + let scenario = Scenario::new(); + scenario.agent_does(AGENT_PUBLISHES_A_TICKET); + let resend = ResendStandIn::replying(500, "upstream exploded"); + + let result = run_with_resend( + &scenario, + &resend, + &["architect", "--plan-only", "--email", "me@example.com"], + ); + + assert_eq!(result.code, Some(0), "stderr: {}", result.stderr); + assert_eq!(result.stdout, format!("{PLAN_URL}\n")); + assert_eq!(resend.requests().len(), 1); + let warning = result.stderr.lines().find(|line| line.contains("warning:")); + assert!( + warning.is_some_and(|warning| warning.contains("upstream exploded")), + "stderr: {}", + result.stderr + ); +} + +#[test] +fn an_interrupted_review_sends_one_notification_that_it_was_interrupted() { + let scenario = Scenario::new(); + scenario.agent_does( + r#"touch "$(dirname "$FAKE_CLAUDE_RECORD")/started" +sleep 60"#, + ); + let resend = ResendStandIn::replying(200, ACCEPTED); + + let result = scenario.run_and_signal_with_env( + &["architect", "--email", "me@example.com"], + &resend_env(&resend), + "started", + "TERM", + ); + + assert_failed(&scenario, &result, "interrupted"); + let (subject, text) = the_one_notification(&resend); + assert_eq!( + subject, + "[thirdshift] acme/widgets Architect run: interrupted" + ); + assert!(text.starts_with("Review: interrupted\n"), "{text}"); + assert!(!text.contains("Cause:"), "{text}"); +} + +#[test] +fn a_plan_that_fails_its_checks_sends_one_notification_naming_it_in_the_cause() { + let scenario = Scenario::new(); + scenario.agent_does(&publishes_a_ticket_then("gh fake issue 8 CLOSED")); + let resend = ResendStandIn::replying(200, ACCEPTED); + + let result = run_with_resend( + &scenario, + &resend, + &["architect", "--email", "me@example.com"], + ); + + assert_eq!(result.code, Some(1), "stderr: {}", result.stderr); + let (subject, text) = the_one_notification(&resend); + assert_eq!( + subject, + "[thirdshift] acme/widgets Architect run: review failed" + ); + assert!( + text.starts_with(&format!( + "Review: failed\n\ + Cause: the plan {PLAN_URL} is closed\n" + )), + "{text}" + ); +} + +#[test] +fn a_dispatched_spec_run_that_fails_sends_one_notification_with_each_tickets_outcome() { + let scenario = spec_plan("exit 3"); + let resend = ResendStandIn::replying(200, ACCEPTED); + + let result = run_with_resend( + &scenario, + &resend, + &["architect", "--email", "me@example.com"], + ); + + assert_eq!(result.code, Some(1), "stderr: {}", result.stderr); + let (subject, text) = the_one_notification(&resend); + assert_eq!(subject, "[thirdshift] acme/widgets Architect run: failed"); + assert!( + text.starts_with(&format!( + "Review: plan published: {PLAN_URL}\n\ + Dispatched: failed\n" + )), + "{text}" + ); + let (_, tickets) = text.split_once("\nTickets:\n").expect(&text); + let landed = pr_from(&scenario, "issue-9"); + let landed = format!("#9 landed with {}\n", landed["url"].as_str().unwrap()); + assert!(tickets.contains(&landed), "expected {landed:?} in: {text}"); + assert!(tickets.contains("#10 failed: "), "{text}"); +} diff --git a/tests/command_surface.rs b/tests/command_surface.rs index 293307cb0..20b929aba 100644 --- a/tests/command_surface.rs +++ b/tests/command_surface.rs @@ -85,6 +85,9 @@ fn help_documents_architect_its_focus_the_dispatch_plan_only_and_one_architect_r "A review that finds no Strong candidate publishes no plan", "thirdshift prints that issue's URL instead, changing no label", "Start one Architect run per repository at a time", + "--email, --email <address> and --no-email ask an Architect run for its Run notification", + "It sends one for the whole Architect run, however it ends", + "The run the plan is dispatched as sends none of its own", ] { assert!(help.contains(mention), "help lacks {mention:?}: {help}"); } @@ -103,8 +106,8 @@ fn architect_with_arguments_it_cant_use_prints_an_error_and_the_help_to_stderr() "unexpected argument after the focus: the Run", ), ( - vec!["architect", "--plan-only", "--email"], - "unexpected argument after architect: --email", + vec!["architect", "--plan-only", "--verbose"], + "unexpected argument after architect: --verbose", ), ( vec!["architect", "merge", "no-merge"],