From 411cfc6b6e00271bda6f520ce44c78359ef8114c Mon Sep 17 00:00:00 2001 From: Jacob Stephens Date: Thu, 8 Oct 2026 09:16:18 -0400 Subject: [PATCH] Show Run results in notification email bodies (#572) --- README.md | 4 ++-- src/notification.rs | 43 ++++++++++++++++++++++++--------------- tests/architect_run.rs | 41 ++++++++++++++++++++++++++----------- tests/pickup_run.rs | 18 ++++++++++++---- tests/run_notification.rs | 22 ++++++++++++++++++-- tests/spec_run.rs | 6 ++++++ 6 files changed, 98 insertions(+), 36 deletions(-) diff --git a/README.md b/README.md index 06322edc..22c88778 100644 --- a/README.md +++ b/README.md @@ -390,7 +390,7 @@ The Credentials are read only when `RESEND_API_KEY` is unset or empty, and only 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 a key is found, in `RESEND_API_KEY` or else the Credentials. If either fails, or the Credentials are broken, the Run stops, exits `1` naming what's wrong (with no key, the message above listing every way to give one), and sends nothing. A Run that asks for no notification never reads the Credentials, so a broken file can't stop it, and `update`, `version` and `help` never read it either. 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), after an Inherited failure with no Base fix taken its [`Base check:`, `Retry with:` and `Or set:` lines](#an-inherited-failure-links-the-base-branchs-checks), the session log path (if any), the [Command log](#logs) path as a `Command log:` line, the hostname and how long the Run took. +- **Body**, plain text: a `Result:` line with the same outcome as the subject, so it is readable even when a long issue title hides the end of the subject; then the pull request URL (if any), the failure cause (if failed), after an Inherited failure with no Base fix taken its [`Base check:`, `Retry with:` and `Or set:` lines](#an-inherited-failure-links-the-base-branchs-checks), the session log path (if any), the [Command log](#logs) path as a `Command log:` line, the hostname and how long the Run took. 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. @@ -617,7 +617,7 @@ A review session that fails or is interrupted, a final message without one of th 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, with or without `--plan-only`, unless it is skipped. A skipped Architect run, [however](#one-at-a-time) [it](#one-architect-plan-at-a-time) [is](#an-architect-idea-waits-for-triage) [skipped](#a-ready-issue-goes-first), sends none, even when asked, so a schedule can start one every few minutes without flooding your inbox: its line is in the log its scheduler keeps. It makes a Run's checks before any other work all the same, an address and a Resend API key, and stops with exit `1` if either is missing, so a run that would have been skipped fails on them too. 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), with the [lines after it](#an-inherited-failure-links-the-base-branchs-checks) of a dispatched run that failed on an Inherited failure, a `Base fix:` line for a dispatched run that started or waited on a [Base fix](#base-fix), the session log path (if any), the `Command 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. +- **Body**, plain text: a `Result:` line with the same outcome as the subject; 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), with the [lines after it](#an-inherited-failure-links-the-base-branchs-checks) of a dispatched run that failed on an Inherited failure, a `Base fix:` line for a dispatched run that started or waited on a [Base fix](#base-fix), the session log path (if any), the `Command 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 diff --git a/src/notification.rs b/src/notification.rs index 2971e459..8f2e4838 100644 --- a/src/notification.rs +++ b/src/notification.rs @@ -186,10 +186,10 @@ fn architect_subject(repo: Option<&Repo>, outcome: &str) -> String { /// The plain-text body of the notification for a command that ended as /// `account` tells, with the line that says what its sessions were /// `built_with`, if known, naming `command_log`, if the command keeps one, -/// and `host`, for a command that `took` so long. An Architect run's starts -/// with how its review ended, and the outcome of the run it dispatched, if -/// any. The cause is left out of an interrupted command's, whose outcome says -/// so. +/// and `host`, for a command that `took` so long. It starts with the command's +/// outcome, as the subject gives it. An Architect run's then tells how its +/// review ended, and the outcome of the run it dispatched, if any. The cause +/// is left out of an interrupted command's, whose outcome says so. fn body( account: &Account, built_with: Option<&str>, @@ -197,7 +197,7 @@ fn body( host: &str, took: Duration, ) -> String { - let mut text = String::new(); + let mut text = format!("Result: {}\n", account.outcome); if let Some(review) = &account.review { text += &format!("Review: {}\n", review.line); if review.dispatched.is_some() { @@ -330,7 +330,8 @@ mod tests { Some(( "[thirdshift] acme/widgets#123 Add export button: merged".to_string(), format!( - "Pull request: {PR}\n\ + "Result: merged\n\ + Pull request: {PR}\n\ Session log: {LOG}\n\ Host: droplet-1\n\ Took: 4s\n" @@ -348,7 +349,8 @@ mod tests { Some(( "[thirdshift] acme/widgets#123 Add export button: failed".to_string(), format!( - "Cause: claude exited 1\n\ + "Result: failed\n\ + Cause: claude exited 1\n\ Base fix: {PLAN} not merged\n\ Session log: {LOG}\n\ Host: droplet-1\n\ @@ -369,7 +371,8 @@ mod tests { told(&about_issue(), &account), Some(( "[thirdshift] acme/widgets#123 Add export button: interrupted".to_string(), - "Host: droplet-1\n\ + "Result: interrupted\n\ + Host: droplet-1\n\ Took: 4s\n" .to_string() )) @@ -390,7 +393,8 @@ mod tests { Some(( "[thirdshift] acme/widgets Architect run: merged".to_string(), format!( - "Review: plan published: {PLAN}\n\ + "Result: merged\n\ + Review: plan published: {PLAN}\n\ Dispatched: merged\n\ Pull request: {PR}\n\ Session log: {LOG}\n\ @@ -422,7 +426,8 @@ mod tests { Some(( "[thirdshift] acme/widgets Architect run: idea filed".to_string(), format!( - "Review: idea filed: {PLAN}\n\ + "Result: idea filed\n\ + Review: idea filed: {PLAN}\n\ Host: droplet-1\n\ Took: 4s\n" ) @@ -446,7 +451,8 @@ mod tests { Some(( "[thirdshift] Architect run: review failed".to_string(), format!( - "Review: failed\n\ + "Result: review failed\n\ + Review: failed\n\ Cause: claude exited 1\n\ Session log: {LOG}\n\ Host: droplet-1\n\ @@ -477,7 +483,8 @@ mod tests { fn the_body_leaves_out_what_the_run_does_not_have() { assert_eq!( body_of(&failed("origin mismatch")), - "Cause: origin mismatch\n\ + "Result: failed\n\ + Cause: origin mismatch\n\ Host: droplet-1\n\ Took: 4s\n" ); @@ -493,7 +500,8 @@ mod tests { "droplet-1", Duration::from_secs(4) ), - "Pull request: https://github.com/acme/widgets/pull/1\n\ + "Result: merged\n\ + Pull request: https://github.com/acme/widgets/pull/1\n\ Session log: /home/me/.thirdshift/logs/sessions/x.jsonl\n\ Command log: /home/me/.thirdshift/logs/commands/issue/x.log\n\ Built with claude · opus · high\n\ @@ -511,7 +519,8 @@ mod tests { }; assert_eq!( body_of(&account), - "Pull request: https://github.com/acme/widgets/pull/1\n\ + "Result: failed\n\ + Pull request: https://github.com/acme/widgets/pull/1\n\ Cause: claude exited 1\n\ Base fix: https://github.com/acme/widgets/issues/8 merged\n\ Host: droplet-1\n\ @@ -539,7 +548,8 @@ mod tests { }; assert_eq!( body_of(&account), - "Pull request: https://github.com/acme/widgets/pull/1\n\ + "Result: failed\n\ + Pull request: https://github.com/acme/widgets/pull/1\n\ Cause: CI red on test, which also fails on main at 362b9ca; fix main first\n\ Base check: test: https://ci.example/main/test\n\ Or set: base.fix = true\n\ @@ -561,7 +571,8 @@ mod tests { }; assert_eq!( body_of(&account), - "Cause: Tickets not done: #21, #22\n\ + "Result: failed\n\ + Cause: Tickets not done: #21, #22\n\ Host: droplet-1\n\ Took: 4s\n\ \n\ diff --git a/tests/architect_run.rs b/tests/architect_run.rs index 3089e6b4..46b0e186 100644 --- a/tests/architect_run.rs +++ b/tests/architect_run.rs @@ -1373,7 +1373,9 @@ fn plan_only_sends_one_notification_naming_the_plan() { "[thirdshift] acme/widgets Architect run: plan published" ); assert!( - text.starts_with(&format!("Review: plan published: {PLAN_URL}\n")), + text.starts_with(&format!( + "Result: plan published\nReview: plan published: {PLAN_URL}\n" + )), "{text}" ); assert!(!text.contains("Dispatched:"), "{text}"); @@ -1404,7 +1406,9 @@ printf 'Architecture review idea: %s\n' "$url" > "$FAKE_CLAUDE_FINAL_MESSAGE" "[thirdshift] acme/widgets Architect run: idea filed" ); assert!( - text.starts_with(&format!("Review: idea filed: {PLAN_URL}\n")), + text.starts_with(&format!( + "Result: idea filed\nReview: idea filed: {PLAN_URL}\n" + )), "{text}" ); assert!(!text.contains("Dispatched:"), "{text}"); @@ -1432,7 +1436,9 @@ fn a_review_whose_idea_is_already_filed_sends_one_notification_naming_that_issue "[thirdshift] acme/widgets Architect run: idea already filed" ); assert!( - text.starts_with(&format!("Review: idea already filed: {url}\n")), + text.starts_with(&format!( + "Result: idea already filed\nReview: idea already filed: {url}\n" + )), "{text}" ); } @@ -1460,7 +1466,8 @@ fn a_failed_review_sends_one_notification_with_the_cause_and_the_session_log() { let log = ending[1].strip_prefix("thirdshift: session log: ").unwrap(); assert!( text.starts_with(&format!( - "Review: failed\n\ + "Result: review failed\n\ + Review: failed\n\ Cause: claude exited 3\n\ Session log: {log}\n\ Command log: {command_log}\n" @@ -1491,7 +1498,8 @@ fn email_always_sends_one_notification_for_the_review_and_the_run_it_dispatched( ); assert!( text.starts_with(&format!( - "Review: plan published: {PLAN_URL}\n\ + "Result: ready for review\n\ + Review: plan published: {PLAN_URL}\n\ Dispatched: ready for review\n\ Pull request: {PR_URL}\n" )), @@ -1515,7 +1523,8 @@ fn the_one_notification_says_what_became_of_the_dispatched_runs_base_fix() { ); assert!( text.starts_with(&format!( - "Review: plan published: {PLAN_URL}\n\ + "Result: ready for review\n\ + Review: plan published: {PLAN_URL}\n\ Dispatched: ready for review\n\ Pull request: {PR_URL}\n\ Base fix: https://github.com/acme/widgets/issues/9 merged\n" @@ -1538,7 +1547,8 @@ fn a_dispatched_spec_run_sends_no_notification_of_its_own_and_its_tickets_are_in let spec_pr = pr_from(&scenario, "issue-8"); assert!( text.starts_with(&format!( - "Review: plan published: {PLAN_URL}\n\ + "Result: merged\n\ + Review: plan published: {PLAN_URL}\n\ Dispatched: merged\n\ Pull request: {}\n", spec_pr["url"].as_str().unwrap() @@ -1570,7 +1580,8 @@ fn a_dispatched_run_that_fails_sends_one_notification_with_its_outcome_and_cause assert_eq!(subject, "[thirdshift] acme/widgets Architect run: failed"); assert!( text.starts_with(&format!( - "Review: plan published: {PLAN_URL}\n\ + "Result: failed\n\ + Review: plan published: {PLAN_URL}\n\ Dispatched: failed\n\ Pull request: {PR_URL}\n\ Cause: claude exited 3\n" @@ -1635,7 +1646,10 @@ sleep 60"#, subject, "[thirdshift] acme/widgets Architect run: interrupted" ); - assert!(text.starts_with("Review: interrupted\n"), "{text}"); + assert!( + text.starts_with("Result: interrupted\nReview: interrupted\n"), + "{text}" + ); assert!(!text.contains("Cause:"), "{text}"); } @@ -1659,7 +1673,8 @@ fn a_plan_that_fails_its_checks_sends_one_notification_naming_it_in_the_cause() ); assert!( text.starts_with(&format!( - "Review: failed\n\ + "Result: review failed\n\ + Review: failed\n\ Cause: the plan {PLAN_URL} is closed\n" )), "{text}" @@ -1682,7 +1697,8 @@ fn a_dispatched_spec_run_that_fails_sends_one_notification_with_each_tickets_out assert_eq!(subject, "[thirdshift] acme/widgets Architect run: failed"); assert!( text.starts_with(&format!( - "Review: plan published: {PLAN_URL}\n\ + "Result: failed\n\ + Review: plan published: {PLAN_URL}\n\ Dispatched: failed\n" )), "{text}" @@ -2228,7 +2244,8 @@ fn an_idea_that_cannot_be_labelled_sends_one_notification_with_the_cause() { ); assert!( text.starts_with( - "Review: failed\n\ + "Result: review failed\n\ + Review: failed\n\ Cause: could not label the Architect idea #7: gh api --method PUT" ), "{text}" diff --git a/tests/pickup_run.rs b/tests/pickup_run.rs index c27c1727..2cda8722 100644 --- a/tests/pickup_run.rs +++ b/tests/pickup_run.rs @@ -901,7 +901,8 @@ fn the_notification_of_a_run_left_ready_for_review_is_that_runs() { .join(&commands[0]); assert!( text.starts_with(&format!( - "Pull request: {}\n\ + "Result: ready for review\n\ + Pull request: {}\n\ Session log: {}\n\ Command log: {}\n\ Built with claude · default model · default effort\n\ @@ -930,7 +931,10 @@ fn the_notification_of_a_merge_run_says_it_merged() { subject, "[thirdshift] acme/widgets#7 Sharpen the widgets: merged" ); - let pull_request = format!("Pull request: {}\n", pr["url"].as_str().unwrap()); + let pull_request = format!( + "Result: merged\nPull request: {}\n", + pr["url"].as_str().unwrap() + ); assert!(text.starts_with(&pull_request), "{text}"); } @@ -950,7 +954,8 @@ fn the_notification_of_a_failed_run_has_its_pull_request_and_its_cause() { ); assert!( text.starts_with(&format!( - "Pull request: {}\n\ + "Result: failed\n\ + Pull request: {}\n\ Cause: claude exited 3\n\ Session log: ", pr_from(&scenario, "issue-7")["url"].as_str().unwrap() @@ -982,6 +987,7 @@ sleep 60"#, subject, "[thirdshift] acme/widgets#7 Sharpen the widgets: interrupted" ); + assert!(text.starts_with("Result: interrupted\n"), "{text}"); assert!(!text.contains("Cause:"), "{text}"); } @@ -1002,7 +1008,10 @@ fn the_notification_of_a_spec_run_is_that_spec_runs_with_a_line_per_ticket() { subject, "[thirdshift] acme/widgets#7 Sharpen every widget: ready for review" ); - let pull_request = format!("Pull request: {}\n", spec_pr["url"].as_str().unwrap()); + let pull_request = format!( + "Result: ready for review\nPull request: {}\n", + spec_pr["url"].as_str().unwrap() + ); assert!(text.starts_with(&pull_request), "{text}"); let (_, tickets) = text.split_once("\nTickets:\n").expect(&text); for ticket in [8, 9] { @@ -1022,6 +1031,7 @@ fn the_notification_of_a_failed_spec_run_has_each_tickets_outcome() { assert_eq!(result.code, Some(1), "stderr: {}", result.stderr); let (subject, text) = the_one_notification(&resend); assert!(subject.ends_with(": failed"), "{subject}"); + assert!(text.starts_with("Result: failed\n"), "{text}"); let (_, tickets) = text.split_once("\nTickets:\n").expect(&text); let landed = pr_from(&scenario, "issue-8"); let landed = format!("#8 landed with {}\n", landed["url"].as_str().unwrap()); diff --git a/tests/run_notification.rs b/tests/run_notification.rs index 57b8894c..87421ffc 100644 --- a/tests/run_notification.rs +++ b/tests/run_notification.rs @@ -100,6 +100,10 @@ fn a_run_ready_for_review_sends_one_notification_to_the_address_given() { "[thirdshift] acme/widgets#7 Add export button: ready for review" ); let text = text(&request); + assert!( + text.starts_with("Result: ready for review\n"), + "{text}" + ); assert_contains(text, PR_URL); assert_contains(text, &the_one_log(&scenario)); assert_contains(text, &hostname()); @@ -109,7 +113,10 @@ fn a_run_ready_for_review_sends_one_notification_to_the_address_given() { #[test] fn a_merge_run_sends_one_notification_that_it_merged() { let scenario = Scenario::new(); - scenario.issue_titled(7, "Add export button"); + scenario.issue_titled( + 7, + "I cancelled my subscription but the account still shows as part of the monthly revenue total on the site", + ); scenario.agent_does(AGENT_OPENS_PR); let resend = ResendStandIn::replying(200, ACCEPTED); @@ -124,7 +131,12 @@ fn a_merge_run_sends_one_notification_that_it_merged() { let request = the_one_request(&resend); assert_eq!( subject(&request), - "[thirdshift] acme/widgets#7 Add export button: merged" + "[thirdshift] acme/widgets#7 I cancelled my subscription but the account still shows as part of the monthly revenue total on the site: merged" + ); + assert!( + text(&request).starts_with("Result: merged\n"), + "{}", + text(&request) ); assert_contains(text(&request), PR_URL); } @@ -151,6 +163,7 @@ fn a_failed_run_sends_one_notification_with_the_cause() { "[thirdshift] acme/widgets#7 Add export button: failed" ); let text = text(&request); + assert!(text.starts_with("Result: failed\n"), "{text}"); assert_contains(text, "claude exited 3"); assert_contains(text, &the_one_log(&scenario)); } @@ -201,6 +214,11 @@ fn an_interrupted_run_sends_one_notification_that_it_was_interrupted() { "{}", subject(&request) ); + assert!( + text(&request).starts_with("Result: interrupted\n"), + "{}", + text(&request) + ); } #[test] diff --git a/tests/spec_run.rs b/tests/spec_run.rs index 4775deeb..f99770bf 100644 --- a/tests/spec_run.rs +++ b/tests/spec_run.rs @@ -1980,6 +1980,10 @@ fn a_ready_spec_run_sends_one_notification_with_a_line_per_ticket() { subject, format!("[thirdshift] acme/widgets#20 {SPEC_TITLE}: ready for review") ); + assert!( + text.starts_with("Result: ready for review\n"), + "{text}" + ); for part in [ "Pull request: https://github.com/acme/widgets/pull/2\n", "Took:", @@ -2015,6 +2019,7 @@ fn a_failed_spec_run_sends_one_notification_with_each_tickets_outcome() { subject, format!("[thirdshift] acme/widgets#20 {SPEC_TITLE}: failed") ); + assert!(text.starts_with("Result: failed\n"), "{text}"); for part in [ "Cause: Tickets not done: #21, #22\n", "#21 failed: claude exited 1 (session log: ", @@ -2053,6 +2058,7 @@ fn an_interrupted_spec_run_sends_one_notification_with_each_tickets_outcome() { subject, format!("[thirdshift] acme/widgets#20 {SPEC_TITLE}: interrupted") ); + assert!(text.starts_with("Result: interrupted\n"), "{text}"); assert_contains(&text, "#21 interrupted\n"); assert_contains(&text, "#22 blocked by #21\n"); }