diff --git a/README.md b/README.md index 265408279..3ae25415a 100644 --- a/README.md +++ b/README.md @@ -118,7 +118,7 @@ To be emailed when the Run ends, add `--email` (or `email`), optionally followed thirdshift --email you@example.com https://github.com/acme/widgets/issues/7 ``` -It sends one [Run notification](#run-notifications), whatever the outcome. +It sends one [Run notification](#run-notifications), whatever the outcome. To have every Run on a machine send one, set `email.always` in the [User config](#user-config); `no-email` (or `--no-email`) then skips it for one Run. - **stdout** carries only the pull request's URL: on success, and on a Failed run that leaves an open pull request, a draft or, after a policy refusal, one ready for review. The exit code tells the two apart, so script it as `url=$(thirdshift "$issue") && echo "ready: $url"`. - **stderr** carries everything else: errors, cleanup problems, and progress lines while sessions run. A successful Run's last line names the pull request too: `PR is ready for review`, or `PR is merged` after a Merge run, followed only by a `warning:` line if a [Run notification](#run-notifications) can't be sent. @@ -146,6 +146,7 @@ A **User config** at `~/.thirdshift/config.toml` sets this machine's defaults fo always = true # every Run is a Merge run, without the merge word [email] +always = true # every Run sends a Run notification, without the email word to = "you@example.com" # where email goes when the command names no address from = "thirdshift@your-verified-domain.com" # the sender; onboarding@resend.dev if unset @@ -155,6 +156,8 @@ dir = "~/elsewhere/logs" # where session logs go, instead of ~/.thirdshift/log With `merge.always = true`, `thirdshift ` is a Merge run, and `thirdshift --no-merge ` (or `no-merge`, before or after the URL) leaves that one Run's pull request ready for review. +With `email.always = true`, every Run sends a [Run notification](#run-notifications) to `email.to`, as if given `--email`, and `thirdshift --no-email ` (or `no-email`, before or after the URL) sends none for that one Run. + `logs.dir` sets the directory [session logs](#logs) are written to, created if missing. It must be an absolute path, `~` or a path starting with `~/`, where `~` stands for `$HOME`. A relative path stops the Run before any work, since the directory a Run is launched from is no base for a setting that holds for every Run. A Run reads the file before any work. One that isn't valid TOML, or that has a key or section thirdshift doesn't know, such as `alway` for `always`, stops the Run with an error naming the file and the offending key, so a typo can't silently leave a setting off. `email-test` reads it the same way. `update`, `version` and `help` never read it, so a broken User config can't block them. @@ -178,9 +181,9 @@ It sends one test email, whose subject marks it as a test and whose body names t #### Run notifications -`--email` (or `email`) asks a Run for a **Run notification**: one email, sent when the Run ends, whatever the outcome. The word after the flag is the address only if it contains `@` and doesn't start with `https://`, so the Issue URL is never taken for it; otherwise the email goes to `email.to`. Giving the flag twice is an argument error. +`--email` (or `email`) asks a Run for a **Run notification**: one email, sent when the Run ends, whatever the outcome. The word after the flag is the address only if it contains `@` and doesn't start with `https://`, so the Issue URL is never taken for it; otherwise the email goes to `email.to`. With `email.always = true` in the [User config](#user-config), a Run asks for one without the flag, and `--no-email` (or `no-email`) skips it for that Run; an address after `--email` still wins over `email.to`. Giving a flag twice, or `--email` together with `--no-email`, is an argument error. -The Run makes the same checks as `email-test` before any other work: an address is known, and `RESEND_API_KEY` is set and not empty. If either fails, the Run stops, exits `1` naming what's missing, and sends nothing. Once they pass, every way the Run ends sends exactly one notification, after its outcome is final and its cleanup done: ready for review, merged, a [Failed run](#failed-runs) (including a later preflight failure such as an origin mismatch), or interrupted by Ctrl-C, SIGTERM or a closed terminal. +A Run that asks for a notification, by the flag or by `email.always`, makes the same checks as `email-test` before any other work: an address is known, and `RESEND_API_KEY` is set and not empty. If either fails, the Run stops, exits `1` naming what's missing, and sends nothing. Once they pass, every way the Run ends sends exactly one notification, after its outcome is final and its cleanup done: ready for review, merged, a [Failed run](#failed-runs) (including a later preflight failure such as an origin mismatch), or interrupted by Ctrl-C, SIGTERM or a closed terminal. - **Subject**: `[thirdshift] /# : `, where the outcome is `ready for review`, `merged`, `failed` or `interrupted`. The title is left out if it can't be read from GitHub. - **Body**, plain text: the pull request URL (if any), the failure cause (if failed), the session log path (if any), the hostname and how long the Run took. diff --git a/src/args.rs b/src/args.rs index ea1a39b6d..6ecb7fd71 100644 --- a/src/args.rs +++ b/src/args.rs @@ -1,8 +1,11 @@ //! The command line: which command, and for a Run, its Issue URL and flags. +use std::mem::discriminant; + use anyhow::{Result, bail}; use crate::issue::IssueUrl; +use crate::notification::NotificationAsk; use crate::run::Goal; /// What thirdshift was asked to do. @@ -21,15 +24,16 @@ pub struct RunArgs { /// The goal `merge` or `no-merge` asked for, if either was given; without /// one, the User config decides. pub goal: Option, - /// `Some` if `email` was given, so the Run sends a Run notification, - /// holding the address after it, if any. - pub email: Option>, + /// What `email` or `no-email` asked for, if either was given; without + /// one, the User config decides. + pub email: Option, } /// Parse the arguments after the program name. `help`, `version`, `update` /// and `email-test` are commands only as the first argument. Otherwise it is /// a Run: one Issue URL, with each Run flag at most once, before or after it. /// `email` may be followed by the address to send the Run notification to. +/// `merge` and `no-merge` contradict each other, as do `email` and `no-email`. pub fn parse(args: &[String]) -> Result { match args.first().map(String::as_str) { Some("help" | "--help" | "-h") => return Ok(Command::Help), @@ -49,28 +53,24 @@ pub fn parse(args: &[String]) -> Result { let mut email = None; let mut args = args.iter().peekable(); while let Some(arg) = args.next() { - let asked = match arg.as_str() { - "merge" | "--merge" => Goal::Merged, - "no-merge" | "--no-merge" => Goal::ReadyForReview, + match arg.as_str() { + "merge" | "--merge" => ask_once(&mut goal, Goal::Merged, arg, MERGE_FLAGS)?, + "no-merge" | "--no-merge" => { + ask_once(&mut goal, Goal::ReadyForReview, arg, MERGE_FLAGS)? + } "email" | "--email" => { - if email.is_some() { - bail!("repeated argument: {arg}"); - } - email = Some(args.next_if(|next| is_address(next)).cloned()); - continue; + 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)? } _ => { if issue.is_some() { bail!("unexpected argument after the Issue URL: {arg}"); } issue = Some(IssueUrl::parse(arg)?); - continue; } - }; - match goal { - None => goal = Some(asked), - Some(given) if given == asked => bail!("repeated argument: {arg}"), - Some(_) => bail!("merge and no-merge can't be used together"), } } let Some(issue) = issue else { @@ -79,6 +79,23 @@ pub fn parse(args: &[String]) -> Result { Ok(Command::Run(RunArgs { issue, goal, email })) } +const MERGE_FLAGS: &str = "merge and no-merge"; +const EMAIL_FLAGS: &str = "email and no-email"; + +/// Record in `given` what the flag `arg` asked for: the same kind of ask +/// twice is a repeated argument, and a different one contradicts the first, +/// as one of the pair of `flags` can't go with the other. +fn ask_once(given: &mut Option, asked: T, arg: &str, flags: &str) -> Result<()> { + match given { + None => *given = Some(asked), + Some(given) if discriminant(given) == discriminant(&asked) => { + bail!("repeated argument: {arg}") + } + Some(_) => bail!("{flags} can't be used together"), + } + Ok(()) +} + /// Is `arg`, after `email`, the address to send to? Only if it looks like /// one, so the Issue URL is never taken for it. fn is_address(arg: &str) -> bool { diff --git a/src/config.rs b/src/config.rs index 09c8eb239..874dd3960 100644 --- a/src/config.rs +++ b/src/config.rs @@ -7,6 +7,7 @@ use std::path::{Path, PathBuf}; use anyhow::{Context, Result, anyhow, bail}; use toml::{Table, Value}; +use crate::notification::NotificationAsk; use crate::run::Goal; /// The settings a User config can hold. Each is what a Run does when its @@ -27,6 +28,9 @@ pub struct UserConfig { /// secret. #[derive(Debug, Default, PartialEq, Eq)] pub struct EmailSettings { + /// `email.always`: every 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, /// `email.from`: the sender, in place of Resend's shared test sender. @@ -88,6 +92,8 @@ impl UserConfig { ), }, ("logs", "dir", _) => bail!("logs.dir must be a string in {file}"), + ("email", "always", Value::Boolean(always)) => config.email.always = *always, + ("email", "always", _) => bail!("email.always must be true or false in {file}"), ("email", "to", Value::String(to)) => config.email.to = Some(to.clone()), ("email", "from", Value::String(from)) => { config.email.from = Some(from.clone()) @@ -112,6 +118,18 @@ impl UserConfig { } } +impl EmailSettings { + /// What a Run whose command gave no `email` or `no-email` asks about its + /// Run notification. + pub fn default_ask(&self) -> NotificationAsk { + if self.always { + NotificationAsk::Send(None) + } else { + NotificationAsk::Skip + } + } +} + /// `path` as an absolute path, with a leading `~` expanded to `home`, or /// `None` if it is relative: the directory a Run is launched from is no base /// for a setting that holds for every Run. @@ -149,6 +167,22 @@ mod tests { assert_eq!(config.email.to.as_deref(), Some("me@example.com")); assert_eq!(config.email.from.as_deref(), Some("ts@acme.dev")); assert_eq!(parse("[email]\n").unwrap().email, EmailSettings::default()); + assert!(parse("[email]\nalways = true\n").unwrap().email.always); + assert!(!parse("[email]\nalways = false\n").unwrap().email.always); + } + + #[test] + fn email_always_asks_for_a_notification_to_email_to() { + let always = parse("[email]\nalways = true\n").unwrap(); + assert_eq!(always.email.default_ask(), NotificationAsk::Send(None)); + for text in ["", "[email]\nalways = false\n"] { + let config = parse(text).unwrap(); + assert_eq!( + config.email.default_ask(), + NotificationAsk::Skip, + "{text:?}" + ); + } } #[test] @@ -203,6 +237,10 @@ mod tests { ), ("[email]\nadress = \"a@b.c\"\n", "unknown key email.adress"), ("email = \"a@b.c\"\n", "email must be the section [email]"), + ( + "[email]\nalways = \"yes\"\n", + "email.always must be true or false", + ), ( "[email]\nto = 1\n", "email.to must be a quoted email address", diff --git a/src/main.rs b/src/main.rs index acf95d2a2..5dcd46098 100644 --- a/src/main.rs +++ b/src/main.rs @@ -27,7 +27,7 @@ use std::process::ExitCode; use args::{Command, RunArgs}; use config::UserConfig; -use notification::RunNotification; +use notification::{NotificationAsk, RunNotification}; const HELP: &str = "\ thirdshift turns a GitHub issue into a ready-for-review pull request, or a merged one, unattended. @@ -41,7 +41,7 @@ usage: thirdshift Run the factory on the issue, from th thirdshift version Print thirdshift's version thirdshift help Print this help -merge, --no-merge and --email go before or after the Issue URL, in any order. +merge, --no-merge, --email and --no-email go before or after the Issue URL, in any order. --email sends one Run notification when the Run ends, whatever the outcome: ready for review, merged, failed or interrupted. --email
sends it to
; a word @@ -60,9 +60,11 @@ logs.dir sets where session logs go instead of ~/.thirdshift/logs: an absolute p --email and email-test send to their
, else to email.to, from email.from, else from onboarding@resend.dev, which only delivers to your own Resend account's address. +With email.always set, every Run sends a Run notification unless given --no-email. The Resend API key comes only from the RESEND_API_KEY environment variable: [email] + always = true to = \"you@example.com\" from = \"thirdshift@your-verified-domain.com\" "; @@ -100,10 +102,11 @@ fn main() -> ExitCode { progress::step(format_args!("{error:#}")); return ExitCode::FAILURE; } - let notification = match email - .map(|to| RunNotification::new(to, &config.email, &issue)) - .transpose() - { + let notification = match email.unwrap_or(config.email.default_ask()) { + NotificationAsk::Send(to) => RunNotification::new(to, &config.email, &issue).map(Some), + NotificationAsk::Skip => Ok(None), + }; + let notification = match notification { Ok(notification) => notification, Err(error) => { progress::step(format_args!("{error:#}")); diff --git a/src/notification.rs b/src/notification.rs index a0df9187e..d99f6671c 100644 --- a/src/notification.rs +++ b/src/notification.rs @@ -15,6 +15,16 @@ use crate::issue::IssueUrl; use crate::progress; use crate::run::{Goal, Reached}; +/// What a 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`. + Send(Option), + /// Send none. + Skip, +} + /// A Run notification, checked and waiting for the Run to end. pub struct RunNotification { resend: Resend, diff --git a/tests/command_surface.rs b/tests/command_surface.rs index 86928ad7c..1130a76cb 100644 --- a/tests/command_surface.rs +++ b/tests/command_surface.rs @@ -170,6 +170,7 @@ fn a_repeated_flag_prints_an_error_and_the_help_to_stderr() { (vec!["--merge", url.as_str(), "--merge"], "--merge"), (vec!["merge", "--merge", url.as_str()], "--merge"), (vec![url.as_str(), "no-merge", "--no-merge"], "--no-merge"), + (vec!["no-email", url.as_str(), "--no-email"], "--no-email"), (vec!["--email", url.as_str(), "--email"], "--email"), ( vec!["email", "a@example.com", url.as_str(), "email"], @@ -212,6 +213,27 @@ fn merge_with_no_merge_prints_an_error_and_the_help_to_stderr() { } } +#[test] +fn email_with_no_email_prints_an_error_and_the_help_to_stderr() { + let scenario = Scenario::new(); + let url = scenario.issue_url(7); + + for args in [ + vec!["email", url.as_str(), "no-email"], + vec!["--no-email", "--email", "a@example.com", url.as_str()], + vec![url.as_str(), "--email", "--no-email"], + ] { + let result = scenario.run(&args); + + assert_argument_error( + &scenario, + &result, + "email and no-email can't be used together", + ); + assert!(scenario.claude_calls().is_empty(), "{args:?} started a Run"); + } +} + #[test] fn no_merge_without_an_issue_url_prints_an_error_and_the_help_to_stderr() { let scenario = Scenario::new(); @@ -245,6 +267,8 @@ fn help_lists_the_email_settings() { "email.from", "Run notification", "--email
", + "email.always", + "--no-email", "onboarding@resend.dev", "RESEND_API_KEY", ] { diff --git a/tests/run_notification.rs b/tests/run_notification.rs index 79291667b..f35f10b1a 100644 --- a/tests/run_notification.rs +++ b/tests/run_notification.rs @@ -338,3 +338,95 @@ fn a_failed_send_is_a_warning_that_changes_neither_the_exit_code_nor_stdout() { } } } + +/// The User config of a machine where every Run sends a Run notification. +const EMAIL_ALWAYS: &str = "[email]\nalways = true\nto = \"config@example.com\"\n"; + +#[test] +fn email_always_makes_a_run_without_the_flag_send_one_notification_to_email_to() { + let scenario = Scenario::new(); + scenario.user_config_is(EMAIL_ALWAYS); + scenario.agent_does(AGENT_OPENS_PR); + let resend = ResendStandIn::replying(200, ACCEPTED); + + let result = run(&scenario, &resend, &[&scenario.issue_url(7)], Some(KEY)); + + assert_eq!(result.code, Some(0), "stderr: {}", result.stderr); + let request = the_one_request(&resend); + assert_eq!(request.body["to"], "config@example.com"); + assert!( + subject(&request).ends_with(": ready for review"), + "{}", + subject(&request) + ); +} + +#[test] +fn no_email_skips_the_notification_email_always_asks_for() { + for no_email in ["no-email", "--no-email"] { + let scenario = Scenario::new(); + scenario.user_config_is(EMAIL_ALWAYS); + scenario.agent_does(AGENT_OPENS_PR); + let resend = ResendStandIn::replying(200, ACCEPTED); + let url = scenario.issue_url(7); + + let result = run(&scenario, &resend, &[no_email, &url], Some(KEY)); + + assert_eq!(result.code, Some(0), "{no_email}: {}", result.stderr); + assert!(resend.requests().is_empty(), "{no_email}"); + } +} + +#[test] +fn with_email_always_an_address_after_the_flag_still_wins() { + let scenario = Scenario::new(); + scenario.user_config_is(EMAIL_ALWAYS); + scenario.agent_does(AGENT_OPENS_PR); + let resend = ResendStandIn::replying(200, ACCEPTED); + + let result = run( + &scenario, + &resend, + &[&scenario.issue_url(7), "--email", "flag@example.com"], + Some(KEY), + ); + + assert_eq!(result.code, Some(0), "stderr: {}", result.stderr); + assert_eq!(the_one_request(&resend).body["to"], "flag@example.com"); +} + +#[test] +fn email_always_without_an_address_or_a_key_stops_the_run_before_any_work() { + for (config, key, named) in [ + ("[email]\nalways = true\n", Some(KEY), "no email address"), + (EMAIL_ALWAYS, None, "RESEND_API_KEY"), + ] { + let scenario = Scenario::new(); + scenario.user_config_is(config); + let resend = ResendStandIn::replying(200, ACCEPTED); + + let result = run(&scenario, &resend, &[&scenario.issue_url(7)], key); + + scenario.assert_rejected_before_any_work(&result, named); + assert!(scenario.gh_calls().is_empty(), "{:?}", scenario.gh_calls()); + assert!(resend.requests().is_empty()); + } +} + +#[test] +fn without_email_always_a_run_without_the_flag_sends_nothing() { + for config in [ + "[email]\nalways = false\nto = \"me@example.com\"\n", + "[email]\nto = \"me@example.com\"\n", + ] { + let scenario = Scenario::new(); + scenario.user_config_is(config); + scenario.agent_does(AGENT_OPENS_PR); + let resend = ResendStandIn::replying(200, ACCEPTED); + + let result = run(&scenario, &resend, &[&scenario.issue_url(7)], Some(KEY)); + + assert_eq!(result.code, Some(0), "{config}: {}", result.stderr); + assert!(resend.requests().is_empty(), "{config}"); + } +}