From be3fe155a5cf052f2915fdd3cdd9ecf0fc857672 Mon Sep 17 00:00:00 2001 From: Jacob Stephens Date: Tue, 29 Sep 2026 19:56:24 +0000 Subject: [PATCH 1/2] Send a Run notification when a Run given --email ends `email` / `--email`, optionally followed by an address, makes a Run send one email through Resend however it ends: ready for review, merged, failed or interrupted. The address and RESEND_API_KEY are checked before any work, with the same checks as email-test; a failed send is only a warning. Co-Authored-By: Claude Opus 5.5 --- README.md | 21 ++- src/args.rs | 23 ++- src/email.rs | 4 +- src/github.rs | 17 ++ src/main.rs | 49 ++++-- src/notification.rs | 202 +++++++++++++++++++++++ src/run.rs | 18 +- tests/command_surface.rs | 12 ++ tests/fakes/gh | 9 +- tests/run_notification.rs | 335 ++++++++++++++++++++++++++++++++++++++ tests/support/mod.rs | 19 +++ 11 files changed, 684 insertions(+), 25 deletions(-) create mode 100644 src/notification.rs create mode 100644 tests/run_notification.rs diff --git a/README.md b/README.md index d74142e94..265408279 100644 --- a/README.md +++ b/README.md @@ -112,8 +112,16 @@ A Merge run ends in one of three ways: - **Policy refusal**: GitHub refused the merge and a round of merging the Base branch and watching CI found nothing left to fix. Exit `1`, the pull request's URL on stdout, and GitHub's error on stderr. The pull request stays open and ready for review, not a draft, for you to merge by hand or to change the repository's settings. thirdshift never reads GitHub's error text to decide this. - **Any other failure**: a [Failed run](#failed-runs), as for a Run without `merge`. +To be emailed when the Run ends, add `--email` (or `email`), optionally followed by the address, before or after the URL and alongside `merge`: + +```sh +thirdshift --email you@example.com https://github.com/acme/widgets/issues/7 +``` + +It sends one [Run notification](#run-notifications), whatever the outcome. + - **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. +- **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. - **Exit code** `0` means the Run ended with a pull request the factory stands behind, merged in a Merge run. Once the Self-merge has merged, the Run succeeds even if deleting the Issue branch on `origin` or closing the issue then fails: the merge can't be undone, so each failed step is a `warning:` line on stderr naming the command to run by hand, and the Run still exits `0` with the URL on stdout. Ctrl-C likewise: before the merge it makes a Failed run, after it thirdshift finishes these steps and exits as merged. `2` means the Issue URL is missing or isn't a GitHub Issue URL, there is an argument other than the URL and the Run flags, or a Run flag is repeated or contradicts another; the error and the help text go to stderr, before any work. A [User config](#user-config) thirdshift can't use exits `1`, also before any work. Any other failure exits `1`. The other commands: @@ -168,6 +176,17 @@ thirdshift email-test you@example.com # or just `thirdshift email-test`, to se It sends one test email, whose subject marks it as a test and whose body names the host, the time and the sender. Before sending, it checks that it has an address (the argument, else `email.to`) and a non-empty `RESEND_API_KEY`; if either is missing, it exits `1` naming what's missing and sends nothing. 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 and exits `1`. It gives up after 30 seconds without an answer. +#### 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. + +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. + +- **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. + +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 Each session's full transcript, as Claude Code's `stream-json` output, is written to its own file under `~/.thirdshift/logs/`, or the `logs.dir` set in the [User config](#user-config) (created if missing): diff --git a/src/args.rs b/src/args.rs index de189af67..ea1a39b6d 100644 --- a/src/args.rs +++ b/src/args.rs @@ -21,11 +21,15 @@ 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>, } /// 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. pub fn parse(args: &[String]) -> Result { match args.first().map(String::as_str) { Some("help" | "--help" | "-h") => return Ok(Command::Help), @@ -42,10 +46,19 @@ pub fn parse(args: &[String]) -> Result { } let mut issue = None; let mut goal = None; - for arg in args { + 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, + "email" | "--email" => { + if email.is_some() { + bail!("repeated argument: {arg}"); + } + email = Some(args.next_if(|next| is_address(next)).cloned()); + continue; + } _ => { if issue.is_some() { bail!("unexpected argument after the Issue URL: {arg}"); @@ -63,5 +76,11 @@ pub fn parse(args: &[String]) -> Result { let Some(issue) = issue else { bail!("missing Issue URL"); }; - Ok(Command::Run(RunArgs { issue, goal })) + Ok(Command::Run(RunArgs { issue, goal, email })) +} + +/// 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 { + arg.contains('@') && !arg.starts_with("https://") } diff --git a/src/email.rs b/src/email.rs index f10e9ef4b..59d12c8ff 100644 --- a/src/email.rs +++ b/src/email.rs @@ -1,6 +1,6 @@ //! Email through Resend's HTTP API (ADR 0005): one HTTPS POST per email, with -//! the key from `RESEND_API_KEY`. `thirdshift email-test` sends through it, -//! and Run notifications are to use the same checks and the same send. +//! the key from `RESEND_API_KEY`. `thirdshift email-test` and Run +//! notifications both send through it, after the same checks. use std::time::Duration; diff --git a/src/github.rs b/src/github.rs index 14b866e78..fc8d0cd30 100644 --- a/src/github.rs +++ b/src/github.rs @@ -22,6 +22,23 @@ pub fn issue_is_open(issue: &IssueUrl) -> Result { Ok(json["state"].as_str().context("gh output has no state")? == "OPEN") } +/// The title of `issue`. +pub fn issue_title(issue: &IssueUrl) -> Result { + let json = gh_json(&[ + "issue", + "view", + &issue.number.to_string(), + "--repo", + &issue.repo_slug(), + "--json", + "title", + ])?; + Ok(json["title"] + .as_str() + .context("gh output has no title")? + .to_string()) +} + /// Close `issue` with `comment`. pub fn close_issue(issue: &IssueUrl, comment: &str) -> Result<()> { gh(&[ diff --git a/src/main.rs b/src/main.rs index 8c7d9af9e..b627dfa99 100644 --- a/src/main.rs +++ b/src/main.rs @@ -9,6 +9,7 @@ mod github; mod host; mod interrupt; mod issue; +mod notification; mod plugin; mod poll; mod preflight; @@ -25,6 +26,8 @@ use std::process::ExitCode; use args::{Command, RunArgs}; use config::UserConfig; +use failed_run::FailedRun; +use notification::RunNotification; const HELP: &str = "\ thirdshift turns a GitHub issue into a ready-for-review pull request, or a merged one, unattended. @@ -32,12 +35,17 @@ thirdshift turns a GitHub issue into a ready-for-review pull request, or a merge usage: thirdshift Run the factory on the issue, from the clone on the Base branch thirdshift merge Run the factory on the issue, then merge its pull request thirdshift --no-merge Run the factory on the issue and leave its pull request for review + thirdshift --email Run the factory on the issue, then email how the Run ended thirdshift email-test [
] Send a test email through Resend, to check the email setup thirdshift update Update thirdshift to the latest release thirdshift version Print thirdshift's version thirdshift help Print this help -merge and --no-merge go before or after the Issue URL. +merge, --no-merge and --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 +after --email is the address only if it has an @ and isn't a URL. The User config, ~/.thirdshift/config.toml, sets defaults for every Run on this machine. With merge.always set, every Run is a Merge run unless given --no-merge: @@ -50,8 +58,8 @@ logs.dir sets where session logs go instead of ~/.thirdshift/logs: an absolute p [logs] dir = \"~/elsewhere/logs\" -email-test sends to
, else to email.to, from email.from, else from -onboarding@resend.dev, which only delivers to your own Resend account's address. +--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. The Resend API key comes only from the RESEND_API_KEY environment variable: [email] @@ -61,7 +69,7 @@ The Resend API key comes only from the RESEND_API_KEY environment variable: fn main() -> ExitCode { let args: Vec = std::env::args().skip(1).collect(); - let RunArgs { issue, goal } = match args::parse(&args) { + let RunArgs { issue, goal, email } = match args::parse(&args) { Ok(Command::Help) => { print!("{HELP}"); return ExitCode::SUCCESS; @@ -87,28 +95,41 @@ fn main() -> ExitCode { } }; let goal = goal.unwrap_or(config.default_goal()); - if let Err(error) = interrupt::install() { - progress::step(format_args!("{error:#}")); - return ExitCode::FAILURE; - } - match run::run(&issue, goal, &config.logs_dir) { - Ok(pr_url) => { + let notification = match email + .map(|to| RunNotification::new(to, &config.email, &issue)) + .transpose() + { + Ok(notification) => notification, + Err(error) => { + progress::step(format_args!("{error:#}")); + return ExitCode::FAILURE; + } + }; + let ended = interrupt::install() + .map_err(FailedRun::from) + .and_then(|()| run::run(&issue, goal, &config.logs_dir)); + let code = match &ended { + Ok(reached) => { // Also on stderr, so the outcome shows even when stdout is captured. - progress::step(format_args!("PR {pr_url} {}", goal.outcome())); - println!("{pr_url}"); + progress::step(format_args!("PR {} {}", reached.pr_url, goal.outcome())); + println!("{}", reached.pr_url); ExitCode::SUCCESS } Err(failed) => { progress::step(format_args!("{:#}", failed.error)); - if let Some(log) = failed.log { + if let Some(log) = &failed.log { progress::step(format_args!("session log: {}", log.display())); } - if let Some(pr_url) = failed.pr_url { + if let Some(pr_url) = &failed.pr_url { println!("{pr_url}"); } ExitCode::FAILURE } + }; + if let Some(notification) = notification { + notification.send(&ended, goal); } + code } /// The end of a command other than a Run: the line that says how it went, diff --git a/src/notification.rs b/src/notification.rs new file mode 100644 index 000000000..c56f218d2 --- /dev/null +++ b/src/notification.rs @@ -0,0 +1,202 @@ +//! The Run notification: one email when a 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::config::EmailSettings; +use crate::email::Resend; +use crate::failed_run::FailedRun; +use crate::github; +use crate::host; +use crate::interrupt; +use crate::issue::IssueUrl; +use crate::progress; +use crate::run::{Goal, Reached}; + +/// A Run notification, checked and waiting for the Run to end. +pub struct RunNotification { + resend: Resend, + issue: IssueUrl, + started: Instant, +} + +impl RunNotification { + /// The Run notification for a Run on `issue` starting now, sent to `to`, + /// else to `email.to`. Fails, before any work, with the same checks as + /// `email-test`: an address is known and `RESEND_API_KEY` is set. + pub fn new(to: Option, settings: &EmailSettings, issue: &IssueUrl) -> Result { + Ok(RunNotification { + resend: Resend::new(to, settings)?, + issue: issue.clone(), + started: Instant::now(), + }) + } + + /// Send the notification for the Run that `ended`, whose goal was `goal`. + /// A failed send is only a warning: it never changes the Run's outcome. + pub fn send(self, ended: &Result, goal: Goal) { + let (outcome, pr_url, cause, log) = match ended { + Ok(reached) => ( + goal_outcome(goal), + Some(reached.pr_url.as_str()), + None, + Some(reached.log.as_path()), + ), + Err(failed) => { + let (outcome, cause) = if interrupt::requested() { + ("interrupted", None) + } else { + ("failed", Some(format!("{:#}", failed.error))) + }; + ( + outcome, + failed.pr_url.as_deref(), + cause, + failed.log.as_deref(), + ) + } + }; + // Unknown if the Run failed because GitHub couldn't be asked. + let title = github::issue_title(&self.issue).ok(); + 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(), + }; + let subject = subject(&self.issue, 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:#}" + )); + } + } +} + +/// The outcome of a Run that reached `goal`, as the subject names it. +fn goal_outcome(goal: Goal) -> &'static str { + match goal { + Goal::ReadyForReview => "ready for review", + Goal::Merged => "merged", + } +} + +/// `[thirdshift] /# : <outcome>`, without the title +/// if it isn't known. +fn subject(issue: &IssueUrl, title: Option<&str>, outcome: &str) -> String { + let title = title.map(|title| format!(" {title}")).unwrap_or_default(); + format!( + "[thirdshift] {}#{}{title}: {outcome}", + issue.repo_slug(), + issue.number + ) +} + +/// What the notification's plain-text body says. +struct Body<'a> { + pr_url: Option<&'a str>, + cause: Option<&'a str>, + log: Option<&'a Path>, + host: &'a str, + took: Duration, +} + +impl Body<'_> { + fn text(&self) -> String { + let mut text = String::new(); + if let Some(pr_url) = self.pr_url { + text += &format!("Pull request: {pr_url}\n"); + } + if let Some(cause) = self.cause { + text += &format!("Cause: {cause}\n"); + } + if let Some(log) = self.log { + text += &format!("Session log: {}\n", log.display()); + } + text += &format!("Host: {}\n", self.host); + text += &format!("Took: {}\n", took(self.took)); + text + } +} + +/// `duration` to the second, as in `1h 2m 3s`, `2m 3s` or `3s`. +fn took(duration: Duration) -> String { + let seconds = duration.as_secs(); + let (hours, minutes, seconds) = (seconds / 3600, seconds / 60 % 60, seconds % 60); + if hours > 0 { + format!("{hours}h {minutes}m {seconds}s") + } else if minutes > 0 { + format!("{minutes}m {seconds}s") + } else { + format!("{seconds}s") + } +} + +#[cfg(test)] +mod tests { + use super::*; + + fn issue() -> IssueUrl { + IssueUrl::parse("https://github.com/acme/widgets/issues/123").unwrap() + } + + #[test] + fn the_subject_names_the_issue_its_title_if_known_and_the_outcome() { + assert_eq!( + subject(&issue(), Some("Add export button"), "ready for review"), + "[thirdshift] acme/widgets#123 Add export button: ready for review" + ); + assert_eq!( + subject(&issue(), None, "failed"), + "[thirdshift] acme/widgets#123: failed" + ); + } + + #[test] + fn the_body_leaves_out_what_the_run_does_not_have() { + let body = Body { + pr_url: None, + cause: Some("origin mismatch"), + log: None, + host: "droplet-1", + took: Duration::from_secs(4), + }; + assert_eq!( + body.text(), + "Cause: origin mismatch\n\ + Host: droplet-1\n\ + Took: 4s\n" + ); + let body = Body { + pr_url: Some("https://github.com/acme/widgets/pull/1"), + cause: None, + log: Some(Path::new("/home/me/.thirdshift/logs/x.jsonl")), + ..body + }; + assert_eq!( + body.text(), + "Pull request: https://github.com/acme/widgets/pull/1\n\ + Session log: /home/me/.thirdshift/logs/x.jsonl\n\ + Host: droplet-1\n\ + Took: 4s\n" + ); + } + + #[test] + fn took_is_to_the_second() { + for (seconds, text) in [ + (0, "0s"), + (59, "59s"), + (60, "1m 0s"), + (3599, "59m 59s"), + (3723, "1h 2m 3s"), + ] { + assert_eq!(took(Duration::from_secs(seconds)), text); + } + } +} diff --git a/src/run.rs b/src/run.rs index 76cb86ac3..512d844aa 100644 --- a/src/run.rs +++ b/src/run.rs @@ -37,13 +37,19 @@ impl Goal { } } -/// Take `issue` to a ready PR, or in a Merge run a merged one, and return the -/// PR's URL. Any failure after the worktree exists, including a merge that +/// A Run that reached its goal. +pub struct Reached { + pub pr_url: String, + /// The most recent session's log. + pub log: PathBuf, +} + +/// Take `issue` to a ready PR, or in a Merge run a merged one. Any failure after the worktree exists, including a merge that /// fails, goes through the Failed run path. The worktree, the local /// Issue branch and the plugin directory are gone when this returns, except /// that a Failed run whose work did not reach origin keeps the worktree and /// branch. -pub fn run(issue: &IssueUrl, goal: Goal, logs_dir: &Path) -> Result<String, FailedRun> { +pub fn run(issue: &IssueUrl, goal: Goal, logs_dir: &Path) -> Result<Reached, FailedRun> { let timestamp = chrono::Utc::now().format("%Y%m%dT%H%M%SZ").to_string(); let launch = Git::new(std::env::current_dir().context("no current directory")?); @@ -75,8 +81,10 @@ pub fn run(issue: &IssueUrl, goal: Goal, logs_dir: &Path) -> Result<String, Fail timestamp: ×tamp, }; let mut log = logs.path("implement"); - implement(issue, &worktree, &base, &prompt, goal, &logs, &mut log) - .map_err(|error| failed_run::fail(issue, worktree, &base, &log, error)) + match implement(issue, &worktree, &base, &prompt, goal, &logs, &mut log) { + Ok(pr_url) => Ok(Reached { pr_url, log }), + Err(error) => Err(failed_run::fail(issue, worktree, &base, &log, error)), + } } /// The implement session given `prompt`, the checks on the PR it opened or diff --git a/tests/command_surface.rs b/tests/command_surface.rs index 5b5a39a31..86928ad7c 100644 --- a/tests/command_surface.rs +++ b/tests/command_surface.rs @@ -11,6 +11,7 @@ fn assert_help_text(text: &str) { for form in [ "thirdshift <Issue URL>", "thirdshift merge <Issue URL>", + "thirdshift --email <Issue URL>", "thirdshift email-test [<address>]", "thirdshift update", "thirdshift version", @@ -169,6 +170,15 @@ 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!["--email", url.as_str(), "--email"], "--email"), + ( + vec!["email", "a@example.com", url.as_str(), "email"], + "email", + ), + ( + vec![url.as_str(), "email", "--email", "b@example.com"], + "--email", + ), ] { let result = scenario.run(&args); @@ -233,6 +243,8 @@ fn help_lists_the_email_settings() { for mention in [ "email.to", "email.from", + "Run notification", + "--email <address>", "onboarding@resend.dev", "RESEND_API_KEY", ] { diff --git a/tests/fakes/gh b/tests/fakes/gh index 92d4b1ba5..ed85031ea 100644 --- a/tests/fakes/gh +++ b/tests/fakes/gh @@ -5,6 +5,7 @@ State lives in the JSON file named by $FAKE_GH_STATE: {"repo": "owner/repo", "issues": {"<number>": "OPEN" | "CLOSED"}, + "titles"?: {"<number>": "<title>"}, "comments"?: {"<number>": ["<body>", ...]}, "prs": [{"number", "url", "head", "base", "state", "isDraft", "title", "body", "mergeable"?, "unknown_polls"?}], @@ -25,6 +26,8 @@ A PR's `headRefOid` is its head branch's tip on origin, frozen when it is merged. Its `closingIssuesReferences` are the issues its body closes. `gh repo view` reports origin's HEAD as the default branch. +An issue's title is its entry in `titles`, else `Issue <number>`. + `gh issue close` closes the issue, recording its `--comment`; on an issue already closed it only warns, as gh does. @@ -273,7 +276,11 @@ def issue_view(state, positional, flags): number = positional[0] if number not in state["issues"]: die(f"GraphQL: Could not resolve to an issue or pull request with the number of {number}.") - fields = {"number": int(number), "state": state["issues"][number]} + fields = { + "number": int(number), + "state": state["issues"][number], + "title": state.get("titles", {}).get(number, f"Issue {number}"), + } wanted = flags["json"].split(",") print(json.dumps({key: fields[key] for key in wanted})) diff --git a/tests/run_notification.rs b/tests/run_notification.rs new file mode 100644 index 000000000..a88c88ccd --- /dev/null +++ b/tests/run_notification.rs @@ -0,0 +1,335 @@ +//! Run notifications: `email` / `--email`, optionally followed by an address, +//! makes a Run send exactly one email through Resend, here a local stand-in, +//! when it ends, whatever the outcome. + +mod support; + +use std::process::Command; + +use support::resend::{Request, ResendStandIn}; +use support::{RunResult, Scenario}; + +const KEY: &str = "re_test_123"; + +const ACCEPTED: &str = r#"{"id":"49a3999c-0ce1-4ea6-ab68-afcd6dc2e794"}"#; + +const PR_URL: &str = "https://github.com/acme/widgets/pull/1"; + +/// The agent commits its work and opens a PR that closes issue #7. +const AGENT_OPENS_PR: &str = r#" +echo "feature" > feature.txt +git add feature.txt +git commit -q -m "Add feature" +gh pr create --base main --head issue-7 --title "Add feature" --body "Closes #7" +"#; + +/// Run thirdshift with `args` against `resend`, with `RESEND_API_KEY` set to +/// `key`, or unset if `None`. +fn run(scenario: &Scenario, resend: &ResendStandIn, args: &[&str], key: Option<&str>) -> RunResult { + let mut env = vec![("THIRDSHIFT_RESEND_URL", resend.url())]; + if let Some(key) = key { + env.push(("RESEND_API_KEY", key)); + } + scenario.run_with_env(args, &env) +} + +/// The one request the stand-in received. +fn the_one_request(resend: &ResendStandIn) -> Request { + let requests = resend.requests(); + assert_eq!(requests.len(), 1, "{requests:?}"); + requests.into_iter().next().unwrap() +} + +fn subject(request: &Request) -> &str { + request.body["subject"].as_str().unwrap() +} + +fn text(request: &Request) -> &str { + request.body["text"].as_str().unwrap() +} + +/// The path of the one session log the Run wrote. +fn the_one_log(scenario: &Scenario) -> String { + let logs = scenario.entries("home/.thirdshift/logs"); + assert_eq!(logs.len(), 1, "logs: {logs:?}"); + let log = scenario.path("home/.thirdshift/logs").join(&logs[0]); + log.to_str().unwrap().to_string() +} + +fn hostname() -> String { + let output = Command::new("hostname").output().unwrap(); + String::from_utf8(output.stdout).unwrap().trim().to_string() +} + +fn assert_contains(text: &str, part: &str) { + assert!(text.contains(part), "expected {part:?} in: {text}"); +} + +#[test] +fn a_run_ready_for_review_sends_one_notification_to_the_address_given() { + let scenario = Scenario::new(); + scenario.issue_titled(7, "Add export button"); + scenario.agent_does(AGENT_OPENS_PR); + let resend = ResendStandIn::replying(200, ACCEPTED); + + let result = run( + &scenario, + &resend, + &[&scenario.issue_url(7), "--email", "me@example.com"], + Some(KEY), + ); + + assert_eq!(result.code, Some(0), "stderr: {}", result.stderr); + assert_eq!(result.stdout, format!("{PR_URL}\n")); + let request = the_one_request(&resend); + assert_eq!(request.method, "POST"); + assert_eq!(request.path, "/emails"); + assert_eq!(request.authorization.as_deref(), Some("Bearer re_test_123")); + assert_eq!(request.body["to"], "me@example.com"); + assert_eq!( + subject(&request), + "[thirdshift] acme/widgets#7 Add export button: ready for review" + ); + let text = text(&request); + assert_contains(text, PR_URL); + assert_contains(text, &the_one_log(&scenario)); + assert_contains(text, &hostname()); + assert_contains(text, "Took:"); +} + +#[test] +fn a_merge_run_sends_one_notification_that_it_merged() { + let scenario = Scenario::new(); + scenario.issue_titled(7, "Add export button"); + scenario.agent_does(AGENT_OPENS_PR); + let resend = ResendStandIn::replying(200, ACCEPTED); + + let result = run( + &scenario, + &resend, + &["email", "me@example.com", "merge", &scenario.issue_url(7)], + Some(KEY), + ); + + assert_eq!(result.code, Some(0), "stderr: {}", result.stderr); + let request = the_one_request(&resend); + assert_eq!( + subject(&request), + "[thirdshift] acme/widgets#7 Add export button: merged" + ); + assert_contains(text(&request), PR_URL); +} + +#[test] +fn a_failed_run_sends_one_notification_with_the_cause() { + let scenario = Scenario::new(); + scenario.issue_titled(7, "Add export button"); + scenario.agent_does("echo 'half done' > wip.txt\nexit 3\n"); + let resend = ResendStandIn::replying(200, ACCEPTED); + + let result = run( + &scenario, + &resend, + &["--email", "me@example.com", &scenario.issue_url(7)], + Some(KEY), + ); + + assert_eq!(result.code, Some(1), "stderr: {}", result.stderr); + assert_eq!(result.stdout, ""); + let request = the_one_request(&resend); + assert_eq!( + subject(&request), + "[thirdshift] acme/widgets#7 Add export button: failed" + ); + let text = text(&request); + assert_contains(text, "claude exited 3"); + assert_contains(text, &the_one_log(&scenario)); +} + +#[test] +fn an_interrupted_run_sends_one_notification_that_it_was_interrupted() { + let scenario = Scenario::new(); + let started = scenario.path("agent-started"); + scenario.agent_does(&format!("touch {}\nsleep 30\n", started.display())); + let resend = ResendStandIn::replying(200, ACCEPTED); + + let child = scenario.run_and_signal_with_env( + &[&scenario.issue_url(7), "--email", "me@example.com"], + &[ + ("THIRDSHIFT_RESEND_URL", resend.url()), + ("RESEND_API_KEY", KEY), + ], + "agent-started", + "TERM", + ); + + assert_eq!(child.code, Some(1), "stderr: {}", child.stderr); + let request = the_one_request(&resend); + assert!( + subject(&request).ends_with(": interrupted"), + "subject: {}", + subject(&request) + ); +} + +#[test] +fn a_failure_before_the_issue_is_read_sends_a_notification_without_a_title() { + let scenario = Scenario::new(); + let resend = ResendStandIn::replying(200, ACCEPTED); + let url = "https://github.com/other/widgets/issues/7"; + + let result = run( + &scenario, + &resend, + &["--email", "me@example.com", url], + Some(KEY), + ); + + assert_eq!(result.code, Some(1), "stderr: {}", result.stderr); + let request = the_one_request(&resend); + assert_eq!(subject(&request), "[thirdshift] other/widgets#7: failed"); + assert_contains(text(&request), "origin mismatch"); +} + +#[test] +fn a_bare_flag_sends_to_email_to_and_an_address_after_the_flag_wins() { + for (args, to) in [ + (vec!["--email"], "config@example.com"), + (vec!["email"], "config@example.com"), + (vec!["--email", "flag@example.com"], "flag@example.com"), + ] { + let scenario = Scenario::new(); + scenario.user_config_is("[email]\nto = \"config@example.com\"\n"); + scenario.agent_does(AGENT_OPENS_PR); + let resend = ResendStandIn::replying(200, ACCEPTED); + let url = scenario.issue_url(7); + let mut argv = vec![url.as_str()]; + argv.extend(args.iter().copied()); + + let result = run(&scenario, &resend, &argv, Some(KEY)); + + assert_eq!(result.code, Some(0), "{args:?}: {}", result.stderr); + assert_eq!(the_one_request(&resend).body["to"], to, "{args:?}"); + } +} + +#[test] +fn the_flag_works_in_any_position_and_never_takes_the_issue_url_as_its_address() { + let url = Scenario::new().issue_url(7); + for args in [ + vec!["--email", url.as_str()], + vec!["email", url.as_str(), "merge"], + vec!["merge", "--email", url.as_str()], + vec!["--email", "me@example.com", url.as_str(), "merge"], + vec![url.as_str(), "merge", "email"], + vec![url.as_str(), "--email", "merge"], + ] { + let scenario = Scenario::new(); + scenario.user_config_is("[email]\nto = \"me@example.com\"\n"); + scenario.agent_does(AGENT_OPENS_PR); + let resend = ResendStandIn::replying(200, ACCEPTED); + + let result = run(&scenario, &resend, &args, Some(KEY)); + + assert_eq!(result.code, Some(0), "{args:?}: {}", result.stderr); + let request = the_one_request(&resend); + assert_eq!(request.body["to"], "me@example.com", "{args:?}"); + let outcome = if args.contains(&"merge") { + "merged" + } else { + "ready for review" + }; + assert!( + subject(&request).ends_with(&format!(": {outcome}")), + "{args:?}: {}", + subject(&request) + ); + } +} + +#[test] +fn with_no_address_known_the_run_stops_before_any_work_and_sends_nothing() { + let scenario = Scenario::new(); + let resend = ResendStandIn::replying(200, ACCEPTED); + + let result = run( + &scenario, + &resend, + &["--email", &scenario.issue_url(7)], + Some(KEY), + ); + + scenario.assert_rejected_before_any_work(&result, "no email address"); + assert!(scenario.gh_calls().is_empty(), "{:?}", scenario.gh_calls()); + assert!(resend.requests().is_empty()); +} + +#[test] +fn without_resend_api_key_the_run_stops_before_any_work_and_sends_nothing() { + for key in [None, Some("")] { + let scenario = Scenario::new(); + let resend = ResendStandIn::replying(200, ACCEPTED); + + let result = run( + &scenario, + &resend, + &[&scenario.issue_url(7), "email", "me@example.com"], + key, + ); + + scenario.assert_rejected_before_any_work(&result, "RESEND_API_KEY"); + assert!(scenario.gh_calls().is_empty(), "{:?}", scenario.gh_calls()); + assert!(resend.requests().is_empty()); + } +} + +#[test] +fn a_run_without_the_flag_sends_nothing() { + let scenario = Scenario::new(); + scenario.user_config_is("[email]\nto = \"me@example.com\"\n"); + 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); + assert!(resend.requests().is_empty()); +} + +#[test] +fn a_failed_send_is_a_warning_that_changes_neither_the_exit_code_nor_stdout() { + for (status, reply, message) in [ + ( + 401, + r#"{"statusCode":401,"message":"API key is invalid","name":"validation_error"}"#, + "API key is invalid", + ), + (500, "upstream exploded", "upstream exploded"), + ] { + for (agent, code, stdout) in [ + (AGENT_OPENS_PR, 0, format!("{PR_URL}\n")), + ("exit 3", 1, String::new()), + ] { + let scenario = Scenario::new(); + scenario.agent_does(agent); + let resend = ResendStandIn::replying(status, reply); + + let result = run( + &scenario, + &resend, + &[&scenario.issue_url(7), "--email", "me@example.com"], + Some(KEY), + ); + + assert_eq!(result.code, Some(code), "{status}: {}", result.stderr); + assert_eq!(result.stdout, stdout, "{status}"); + assert_eq!(resend.requests().len(), 1, "{status}"); + let warning = result + .stderr + .lines() + .find(|line| line.contains("warning:")) + .unwrap_or_else(|| panic!("no warning in stderr: {}", result.stderr)); + assert_contains(warning, message); + } + } +} diff --git a/tests/support/mod.rs b/tests/support/mod.rs index 255ae7cea..5c852ba36 100644 --- a/tests/support/mod.rs +++ b/tests/support/mod.rs @@ -157,8 +157,20 @@ impl Scenario { /// Run thirdshift and send it `signal` (e.g. `"INT"`) once the fake agent /// has touched the file `started` in the scenario root. pub fn run_and_signal(&self, args: &[&str], started: &str, signal: &str) -> RunResult { + self.run_and_signal_with_env(args, &[], started, signal) + } + + /// Like [`Scenario::run_and_signal`], with extra environment variables. + pub fn run_and_signal_with_env( + &self, + args: &[&str], + env: &[(&str, &str)], + started: &str, + signal: &str, + ) -> RunResult { let child = self .command(args) + .envs(env.iter().copied()) .stdout(Stdio::piped()) .stderr(Stdio::piped()) .spawn() @@ -219,6 +231,13 @@ impl Scenario { self.write_gh_state(&gh); } + /// Give issue `number` the title `title` on the fake GitHub. + pub fn issue_titled(&self, number: u32, title: &str) { + let mut gh = self.gh_state(); + gh["titles"][number.to_string()] = json!(title); + self.write_gh_state(&gh); + } + pub fn gh_state(&self) -> Value { serde_json::from_str(&fs::read_to_string(self.path("gh-state.json")).unwrap()).unwrap() } From ce17a7f1b3a8f0f00a5c3785e3885f9dab71073e Mon Sep 17 00:00:00 2001 From: Jacob Stephens <jacob@stephens.page> Date: Tue, 29 Sep 2026 20:00:59 +0000 Subject: [PATCH 2/2] Decide the Run's outcome once, and let a closed terminal still get its notification The interrupt handler goes in before the notification is checked, the FailedRun records whether the Run was interrupted when it failed, and the issue title is read at the start of the Run. Writes to a closed terminal no longer panic before the notification is sent. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> --- src/failed_run.rs | 7 +++++- src/main.rs | 23 +++++++++++------ src/notification.rs | 33 ++++++++++++------------ src/progress.rs | 5 +++- src/run.rs | 11 ++++---- tests/run_notification.rs | 53 +++++++++++++++++++++------------------ 6 files changed, 77 insertions(+), 55 deletions(-) diff --git a/src/failed_run.rs b/src/failed_run.rs index b5ccaac52..407bac262 100644 --- a/src/failed_run.rs +++ b/src/failed_run.rs @@ -20,6 +20,8 @@ pub struct FailedRun { pub pr_url: Option<String>, /// The most recent session log, if a session was started. pub log: Option<PathBuf>, + /// Whether the Run was interrupted, as it was when the Run failed. + pub interrupted: bool, } /// A merge GitHub refused when a round of the Repair loop found nothing to @@ -43,6 +45,7 @@ impl From<anyhow::Error> for FailedRun { error, pr_url: None, log: None, + interrupted: interrupt::requested(), } } } @@ -60,7 +63,8 @@ pub fn fail( error: anyhow::Error, ) -> FailedRun { // An interrupt can surface as some other error, such as a killed git. - let error = if interrupt::requested() { + let interrupted = interrupt::requested(); + let error = if interrupted { anyhow!("interrupted") } else { error @@ -101,6 +105,7 @@ pub fn fail( error, pr_url, log: log.exists().then(|| log.to_path_buf()), + interrupted, } } diff --git a/src/main.rs b/src/main.rs index b627dfa99..acf95d2a2 100644 --- a/src/main.rs +++ b/src/main.rs @@ -22,11 +22,11 @@ mod session; mod update; mod worktree; +use std::io::Write; use std::process::ExitCode; use args::{Command, RunArgs}; use config::UserConfig; -use failed_run::FailedRun; use notification::RunNotification; const HELP: &str = "\ @@ -95,6 +95,11 @@ fn main() -> ExitCode { } }; let goal = goal.unwrap_or(config.default_goal()); + // First, so no interrupt can end the Run once its notification is checked. + if let Err(error) = interrupt::install() { + progress::step(format_args!("{error:#}")); + return ExitCode::FAILURE; + } let notification = match email .map(|to| RunNotification::new(to, &config.email, &issue)) .transpose() @@ -105,14 +110,12 @@ fn main() -> ExitCode { return ExitCode::FAILURE; } }; - let ended = interrupt::install() - .map_err(FailedRun::from) - .and_then(|()| run::run(&issue, goal, &config.logs_dir)); + let ended = run::run(&issue, goal, &config.logs_dir); let code = match &ended { Ok(reached) => { // Also on stderr, so the outcome shows even when stdout is captured. - progress::step(format_args!("PR {} {}", reached.pr_url, goal.outcome())); - println!("{}", reached.pr_url); + progress::step(format_args!("PR {} is {}", reached.pr_url, goal.outcome())); + print_pr_url(&reached.pr_url); ExitCode::SUCCESS } Err(failed) => { @@ -121,7 +124,7 @@ fn main() -> ExitCode { progress::step(format_args!("session log: {}", log.display())); } if let Some(pr_url) = &failed.pr_url { - println!("{pr_url}"); + print_pr_url(pr_url); } ExitCode::FAILURE } @@ -132,6 +135,12 @@ fn main() -> ExitCode { code } +/// The PR's URL on stdout. A failed write, as once the terminal has closed, +/// is ignored, so the Run notification still goes. +fn print_pr_url(pr_url: &str) { + let _ = writeln!(std::io::stdout(), "{pr_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<impl std::fmt::Display>) -> ExitCode { diff --git a/src/notification.rs b/src/notification.rs index c56f218d2..a0df9187e 100644 --- a/src/notification.rs +++ b/src/notification.rs @@ -11,7 +11,6 @@ use crate::email::Resend; use crate::failed_run::FailedRun; use crate::github; use crate::host; -use crate::interrupt; use crate::issue::IssueUrl; use crate::progress; use crate::run::{Goal, Reached}; @@ -20,18 +19,28 @@ use crate::run::{Goal, Reached}; pub struct RunNotification { resend: Resend, issue: IssueUrl, + /// The issue's title, if GitHub gave it when the Run started. + title: Option<String>, started: Instant, } impl RunNotification { /// The Run notification for a Run on `issue` starting now, sent to `to`, /// else to `email.to`. Fails, before any work, with the same checks as - /// `email-test`: an address is known and `RESEND_API_KEY` is set. + /// `email-test`: an address is known and `RESEND_API_KEY` is set. Once + /// they pass, it reads the issue's title now, while someone may be + /// watching, rather than after the Run, when a hung `gh` could keep the + /// notification from ever going. pub fn new(to: Option<String>, settings: &EmailSettings, issue: &IssueUrl) -> Result<Self> { + let started = Instant::now(); + let resend = Resend::new(to, settings)?; Ok(RunNotification { - resend: Resend::new(to, settings)?, + resend, issue: issue.clone(), - started: Instant::now(), + // Left out of the subject if GitHub can't be asked; the Run's own + // preflight reports why. + title: github::issue_title(issue).ok(), + started, }) } @@ -40,13 +49,13 @@ impl RunNotification { pub fn send(self, ended: &Result<Reached, FailedRun>, goal: Goal) { let (outcome, pr_url, cause, log) = match ended { Ok(reached) => ( - goal_outcome(goal), + goal.outcome(), Some(reached.pr_url.as_str()), None, Some(reached.log.as_path()), ), Err(failed) => { - let (outcome, cause) = if interrupt::requested() { + let (outcome, cause) = if failed.interrupted { ("interrupted", None) } else { ("failed", Some(format!("{:#}", failed.error))) @@ -59,8 +68,6 @@ impl RunNotification { ) } }; - // Unknown if the Run failed because GitHub couldn't be asked. - let title = github::issue_title(&self.issue).ok(); let host = host::name(); let body = Body { pr_url, @@ -69,7 +76,7 @@ impl RunNotification { host: host.as_deref().unwrap_or("unknown host"), took: self.started.elapsed(), }; - let subject = subject(&self.issue, title.as_deref(), 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:#}" @@ -78,14 +85,6 @@ impl RunNotification { } } -/// The outcome of a Run that reached `goal`, as the subject names it. -fn goal_outcome(goal: Goal) -> &'static str { - match goal { - Goal::ReadyForReview => "ready for review", - Goal::Merged => "merged", - } -} - /// `[thirdshift] <owner>/<repo>#<n> <title>: <outcome>`, without the title /// if it isn't known. fn subject(issue: &IssueUrl, title: Option<&str>, outcome: &str) -> String { diff --git a/src/progress.rs b/src/progress.rs index 3d87106fb..719fa08d3 100644 --- a/src/progress.rs +++ b/src/progress.rs @@ -3,6 +3,7 @@ use std::collections::HashMap; use std::fmt::Display; +use std::io::Write; use serde_json::Value; @@ -11,7 +12,9 @@ const MAX_DETAIL: usize = 100; /// Print one of thirdshift's own steps. pub fn step(message: impl Display) { - eprintln!("thirdshift: {message}"); + // 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(), "thirdshift: {message}"); } /// Condenses one session's stream, a line at a time. Unknown and malformed diff --git a/src/run.rs b/src/run.rs index 512d844aa..4587abee3 100644 --- a/src/run.rs +++ b/src/run.rs @@ -28,11 +28,11 @@ pub enum Goal { impl Goal { /// What became of the PR once the Run reached this goal, as in - /// "PR <url> is merged". + /// "PR <url> is merged" and a Run notification's subject. pub fn outcome(self) -> &'static str { match self { - Goal::ReadyForReview => "is ready for review", - Goal::Merged => "is merged", + Goal::ReadyForReview => "ready for review", + Goal::Merged => "merged", } } } @@ -44,8 +44,9 @@ pub struct Reached { pub log: PathBuf, } -/// Take `issue` to a ready PR, or in a Merge run a merged one. Any failure after the worktree exists, including a merge that -/// fails, goes through the Failed run path. The worktree, the local +/// Take `issue` to a ready PR, or in a Merge run a merged one. Any failure +/// after the worktree exists, including a merge that fails, goes through the +/// Failed run path. The worktree, the local /// Issue branch and the plugin directory are gone when this returns, except /// that a Failed run whose work did not reach origin keeps the worktree and /// branch. diff --git a/tests/run_notification.rs b/tests/run_notification.rs index a88c88ccd..79291667b 100644 --- a/tests/run_notification.rs +++ b/tests/run_notification.rs @@ -23,14 +23,20 @@ git commit -q -m "Add feature" gh pr create --base main --head issue-7 --title "Add feature" --body "Closes #7" "#; -/// Run thirdshift with `args` against `resend`, with `RESEND_API_KEY` set to +/// The environment for a Run against `resend`, with `RESEND_API_KEY` set to /// `key`, or unset if `None`. -fn run(scenario: &Scenario, resend: &ResendStandIn, args: &[&str], key: Option<&str>) -> RunResult { +fn env<'a>(resend: &'a ResendStandIn, key: Option<&'a str>) -> Vec<(&'a str, &'a str)> { let mut env = vec![("THIRDSHIFT_RESEND_URL", resend.url())]; if let Some(key) = key { env.push(("RESEND_API_KEY", key)); } - scenario.run_with_env(args, &env) + env +} + +/// Run thirdshift with `args` against `resend`, with `RESEND_API_KEY` set to +/// `key`, or unset if `None`. +fn run(scenario: &Scenario, resend: &ResendStandIn, args: &[&str], key: Option<&str>) -> RunResult { + scenario.run_with_env(args, &env(resend, key)) } /// The one request the stand-in received. @@ -148,32 +154,31 @@ fn a_failed_run_sends_one_notification_with_the_cause() { #[test] fn an_interrupted_run_sends_one_notification_that_it_was_interrupted() { - let scenario = Scenario::new(); - let started = scenario.path("agent-started"); - scenario.agent_does(&format!("touch {}\nsleep 30\n", started.display())); - let resend = ResendStandIn::replying(200, ACCEPTED); + for signal in ["INT", "TERM", "HUP"] { + let scenario = Scenario::new(); + let started = scenario.path("agent-started"); + scenario.agent_does(&format!("touch {}\nsleep 30\n", started.display())); + let resend = ResendStandIn::replying(200, ACCEPTED); - let child = scenario.run_and_signal_with_env( - &[&scenario.issue_url(7), "--email", "me@example.com"], - &[ - ("THIRDSHIFT_RESEND_URL", resend.url()), - ("RESEND_API_KEY", KEY), - ], - "agent-started", - "TERM", - ); + let result = scenario.run_and_signal_with_env( + &[&scenario.issue_url(7), "--email", "me@example.com"], + &env(&resend, Some(KEY)), + "agent-started", + signal, + ); - assert_eq!(child.code, Some(1), "stderr: {}", child.stderr); - let request = the_one_request(&resend); - assert!( - subject(&request).ends_with(": interrupted"), - "subject: {}", - subject(&request) - ); + assert_eq!(result.code, Some(1), "{signal}: {}", result.stderr); + let request = the_one_request(&resend); + assert!( + subject(&request).ends_with(": interrupted"), + "{signal}: {}", + subject(&request) + ); + } } #[test] -fn a_failure_before_the_issue_is_read_sends_a_notification_without_a_title() { +fn a_run_whose_issue_title_cant_be_read_sends_a_notification_without_it() { let scenario = Scenario::new(); let resend = ResendStandIn::replying(200, ACCEPTED); let url = "https://github.com/other/widgets/issues/7";