From de4cb9c8af5450b40adb93360d166da56a0b1b6c Mon Sep 17 00:00:00 2001 From: Jacob Stephens Date: Wed, 30 Sep 2026 00:12:23 +0000 Subject: [PATCH 1/2] Offer Setup on the first Run from a terminal with no User config Closes #131 Co-Authored-By: Claude Opus 5.5 --- README.md | 4 +- src/config.rs | 76 ++++++++++++-- src/main.rs | 4 + src/questions.rs | 13 +++ tests/setup_offer.rs | 236 +++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 324 insertions(+), 9 deletions(-) create mode 100644 tests/setup_offer.rs diff --git a/README.md b/README.md index 5297d530d..c03e29888 100644 --- a/README.md +++ b/README.md @@ -187,6 +187,8 @@ Every key holds its real value, so a Run reading it does exactly what it does wi Pressing Enter takes the default shown, which is the file's current value, or else the setting's default, and for the address the suggested email above. `logs.dir` is not asked about. The answers are written like everything else below: in place, keeping your comments. Ctrl-C during the questions writes nothing. With no terminal, as from cron or `thirdshift setup . Set your defaults now? [Y/n]`. Yes (or Enter) asks the questions above, writes the file, and the Run carries on using your answers; a flag in the command, such as `--no-merge`, `--email` or `--no-email`, still wins over them. No writes every setting at its default, as `setup` with no terminal does, says that `thirdshift setup` changes it, and the Run carries on; later Runs find the file and don't offer again. If the file can't be written, stderr gets a `warning:` line and the Run carries on with the defaults. Ctrl-C during the offer or the questions writes nothing and ends the command before any work, with no Run notification. A command thirdshift can't parse exits `2` before any offer. A Run with no terminal, as from cron, CI, `nohup` or an agent's shell, offers nothing, writes nothing, and runs on the defaults, so a later Run from a terminal still gets the offer. + 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 `launch.pull = true`, every Run brings the Base branch checked out in the directory you start it from (the **Launch directory**) up to date with `origin`, so you no longer `git pull` by hand before each Run. It happens after the pre-flight checks pass and before the worktree is created, as `git merge --ff-only origin/`: fast-forward only, never a merge commit or a rebase, always from `origin`, whatever the branch's upstream or your `pull.*` settings. A progress line on stderr says when it updates the branch; an already up-to-date branch is left quietly as it is. It is skipped when the checked-out branch isn't the Base branch, as in a Continuation whose open pull request targets another base, or on a detached HEAD. If the update can't happen, for example because uncommitted changes are in the way, stderr gets a `warning:` line with git's error and the command to run by hand, your changes are left as they were, and the Run carries on with the same outcome and exit code. The setting only affects your checkout: the Run's worktree starts from `origin/` either way. @@ -195,7 +197,7 @@ With `email.always = true`, every Run sends a [Run notification](#run-notificati `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`, or a value of the wrong type, such as anything but `true` or `false` 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` and `setup` read it the same way. `update`, `version` and `help` never read it, so a broken User config can't block them. +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`, or a value of the wrong type, such as anything but `true` or `false` 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` and `setup` read it the same way. `update`, `version` and `help` never read it, so a broken User config can't block them, and they never offer Setup; nor does `email-test`. ### Email diff --git a/src/config.rs b/src/config.rs index f6c62beed..54201e9c1 100644 --- a/src/config.rs +++ b/src/config.rs @@ -1,6 +1,7 @@ //! The User config: `~/.thirdshift/config.toml`, this machine's defaults for //! every Run. Only a Run, `email-test` and `setup` read it, so a broken one -//! can't block `update`, `version` or `help`. +//! can't block `update`, `version` or `help`, and only a Run offers Setup +//! when there is none. use std::io::Write; use std::path::{Path, PathBuf}; @@ -166,16 +167,11 @@ pub fn setup() -> Result { let asked = questions::has_terminal(); let mut send_test = false; if asked { - let current = UserConfig::parse(&text, &path, &home)?; - let answers = questions::ask(¤t, || suggested_address(&home))?; - send_test = answers.send_test(); - text = with_answers(&text, &answers)?; + (text, send_test) = answered(&text, &path, &home)?; } let written = match &existing { None => { - std::fs::create_dir_all(home.join(".thirdshift")) - .and_then(|()| std::fs::write(&path, &text)) - .with_context(|| format!("can't write {}", path.display()))?; + write_new(&home, &path, &text)?; format!("wrote the User config {}", path.display()) } Some(existing) if *existing == text && asked => { @@ -208,6 +204,70 @@ pub fn setup() -> Result { Ok(email::send_test(None, &config.email)?.to_string()) } +/// The first Run's offer of Setup, made only from a terminal and only when +/// there is no User config. Yes asks the Setup questions and writes the +/// answers; no writes every setting at its default, as `setup` with no +/// terminal does. Either way the Run then loads what was written. A User +/// config that can't be written, or a test email that can't go, is a +/// warning, so the Run carries on with the defaults; stdin closing before +/// the last answer ends the command before any work, with nothing written. +pub fn offer_setup() -> Result<()> { + if !questions::has_terminal() { + return Ok(()); + } + let (home, path) = home_and_path()?; + match std::fs::symlink_metadata(&path) { + Err(error) if error.kind() == std::io::ErrorKind::NotFound => {} + _ => return Ok(()), + } + let accepted = questions::offer(&path)?; + let mut text = with_email_to(suggested_address(&home)); + let mut send_test = false; + if accepted { + (text, send_test) = answered(&text, &path, &home)?; + } + if let Err(error) = write_new(&home, &path, &text) { + progress::step(format_args!( + "warning: {error:#}; carrying on with the defaults" + )); + return Ok(()); + } + if !accepted { + progress::step(format_args!( + "wrote the User config {} with every setting at its default; \ + thirdshift setup changes it", + path.display() + )); + return Ok(()); + } + progress::step(format_args!("wrote the User config {}", path.display())); + if send_test { + let sent = UserConfig::load().and_then(|config| email::send_test(None, &config.email)); + match sent { + Ok(sent) => progress::step(sent), + Err(error) => progress::step(format_args!("warning: {error:#}")), + } + } + Ok(()) +} + +/// Ask the Setup questions, with the settings in `text`, the User config at +/// `path`, as the default answers. Returns `text` with the answers, and +/// whether the user asked for a test email. +fn answered(text: &str, path: &Path, home: &Path) -> Result<(String, bool)> { + let current = UserConfig::parse(text, path, home)?; + let answers = questions::ask(¤t, || suggested_address(home))?; + Ok((with_answers(text, &answers)?, answers.send_test())) +} + +/// Write `text` as the User config at `path`, where there is none yet, +/// creating `~/.thirdshift` under `home` if it is missing. +fn write_new(home: &Path, path: &Path, text: &str) -> Result<()> { + std::fs::create_dir_all(home.join(".thirdshift")) + .and_then(|()| std::fs::write(path, text)) + .with_context(|| format!("can't write {}", path.display())) +} + /// `text`, a User config with every key, or a commented-out `email.to`, with /// the values `answers` gives. Only those values change: the spacing and /// comments around each stay, as does everything else in `text`. diff --git a/src/main.rs b/src/main.rs index db81d40d8..238c3a2a9 100644 --- a/src/main.rs +++ b/src/main.rs @@ -105,6 +105,10 @@ fn main() -> ExitCode { Ok(Command::Run(run_args)) => run_args, Err(error) => return argument_error(format_args!("{error:#}")), }; + if let Err(error) = config::offer_setup() { + progress::step(format_args!("{error:#}")); + return ExitCode::FAILURE; + } let config = match UserConfig::load() { Ok(config) => config, Err(error) => { diff --git a/src/questions.rs b/src/questions.rs index e840bee2b..0c663b49d 100644 --- a/src/questions.rs +++ b/src/questions.rs @@ -3,6 +3,7 @@ //! by pressing Enter, is the current value. use std::io::{BufRead, IsTerminal, Write}; +use std::path::Path; use anyhow::{Result, bail}; @@ -44,6 +45,18 @@ pub fn has_terminal() -> bool { std::io::stdin().is_terminal() && std::io::stderr().is_terminal() } +/// The first Run's offer of Setup, with no User config at `path`: whether +/// the user wants to set their defaults now, yes unless they say no. +pub fn offer(path: &Path) -> Result { + yes_or_no( + &format!( + "No User config at {}. Set your defaults now?", + path.display() + ), + true, + ) +} + /// Ask the Setup questions, with the settings in `current` as the default /// answers, and `suggested_address` for `email.to` when `current` has none. /// The address is re-asked until it has an `@`. Ends in an error if stdin diff --git a/tests/setup_offer.rs b/tests/setup_offer.rs new file mode 100644 index 000000000..636f033c3 --- /dev/null +++ b/tests/setup_offer.rs @@ -0,0 +1,236 @@ +//! The first Run's offer of Setup: a Run from a terminal with no User config +//! asks, on stderr, whether to set the defaults now, before any work. Yes runs +//! the Setup questions and the Run carries on with the answers; no writes the +//! all-defaults User config. With no terminal, a Run offers nothing and writes +//! nothing. + +mod support; + +use std::fs; + +use support::{CTRL_C, Scenario, TerminalResult}; + +/// The agent commits its work and opens a PR that closes issue #7, into +/// `main`. +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" +"#; + +const PR_URL: &str = "https://github.com/acme/widgets/pull/1"; + +const OFFER: &str = "Set your defaults now? [Y/n]"; +const MERGE: &str = "Merge run?"; +const PULL: &str = "fast-forward"; +const NOTIFY: &str = "Run notifications, an email"; + +/// The first line a Run prints once it starts its work. +const FIRST_STEP: &str = "thirdshift: "; + +/// Assert the Run ended ready for review or merged, as `outcome` says, with +/// only the PR's URL on stdout. +fn assert_ended(scenario: &Scenario, result: &TerminalResult, outcome: &str, state: &str) { + assert_eq!(result.code, Some(0), "terminal: {}", result.stderr); + assert_eq!(result.stdout, format!("{PR_URL}\n")); + assert!( + result + .stderr + .trim_end() + .ends_with(&format!("thirdshift: PR {PR_URL} is {outcome}")), + "terminal: {}", + result.stderr + ); + assert_eq!(scenario.gh_state()["prs"][0]["state"], state); +} + +#[test] +fn a_first_run_from_a_terminal_offers_setup_before_any_work() { + let scenario = Scenario::new(); + scenario.agent_does(AGENT_OPENS_PR); + + let result = scenario.run_on_terminal(&[&scenario.issue_url(7)], &[], &[(OFFER, "n")]); + + assert_ended(&scenario, &result, "ready for review", "OPEN"); + let path = scenario.path("home/.thirdshift/config.toml"); + let offer = format!("No User config at {}. {OFFER}", path.display()); + let offered = result.stderr.find(&offer).expect(&result.stderr); + let first_step = result.stderr.find(FIRST_STEP).unwrap(); + assert!(offered < first_step, "terminal: {}", result.stderr); +} + +#[test] +fn accepting_and_choosing_merge_always_makes_that_run_a_merge_run() { + let scenario = Scenario::new(); + scenario.agent_does(AGENT_OPENS_PR); + + let result = scenario.run_on_terminal( + &[&scenario.issue_url(7)], + &[], + &[(OFFER, ""), (MERGE, "y"), (PULL, ""), (NOTIFY, "")], + ); + + assert_ended(&scenario, &result, "merged", "MERGED"); + let config: toml::Table = result.user_config.unwrap().parse().unwrap(); + assert_eq!(config["merge"]["always"].as_bool(), Some(true)); +} + +#[test] +fn no_merge_in_the_command_wins_over_a_fresh_merge_always() { + let scenario = Scenario::new(); + scenario.agent_does(AGENT_OPENS_PR); + + let result = scenario.run_on_terminal( + &["--no-merge", &scenario.issue_url(7)], + &[], + &[(OFFER, "y"), (MERGE, "y"), (PULL, ""), (NOTIFY, "")], + ); + + assert_ended(&scenario, &result, "ready for review", "OPEN"); + let config: toml::Table = result.user_config.unwrap().parse().unwrap(); + assert_eq!(config["merge"]["always"].as_bool(), Some(true)); +} + +#[test] +fn declining_writes_the_defaults_and_a_second_run_does_not_offer() { + let unattended = Scenario::new(); + assert_eq!(unattended.run(&["setup"]).code, Some(0)); + let defaults = fs::read_to_string(unattended.path("home/.thirdshift/config.toml")).unwrap(); + let scenario = Scenario::new(); + scenario.agent_does(AGENT_OPENS_PR); + + let first = scenario.run_on_terminal(&[&scenario.issue_url(7)], &[], &[(OFFER, "n")]); + + assert_ended(&scenario, &first, "ready for review", "OPEN"); + assert_eq!(first.user_config.as_deref(), Some(defaults.as_str())); + assert!( + first.stderr.contains("thirdshift setup"), + "terminal: {}", + first.stderr + ); + assert!(!first.stderr.contains(MERGE), "terminal: {}", first.stderr); + + scenario.issue_is(8, "OPEN"); + let second = scenario.run_on_terminal(&[&scenario.issue_url(8)], &[], &[]); + + assert!( + !second.stderr.contains(OFFER), + "terminal: {}", + second.stderr + ); + assert_eq!(second.user_config.as_deref(), Some(defaults.as_str())); +} + +#[test] +fn a_bad_command_exits_2_with_no_offer_and_no_file() { + let scenario = Scenario::new(); + + for args in [vec!["--merge"], vec!["not-a-url"], vec!["merge", "merge"]] { + let result = scenario.run_on_terminal(&args, &[], &[]); + + assert_eq!(result.code, Some(2), "{args:?}: {}", result.stderr); + assert!( + !result.stderr.contains(OFFER), + "{args:?}: {}", + result.stderr + ); + assert_eq!(result.user_config, None, "{args:?}"); + } +} + +#[test] +fn with_no_terminal_a_run_offers_nothing_and_writes_nothing() { + let scenario = Scenario::new(); + scenario.agent_does(AGENT_OPENS_PR); + + let result = scenario.run(&[&scenario.issue_url(7)]); + + assert_eq!(result.code, Some(0), "stderr: {}", result.stderr); + assert_eq!(result.stdout, format!("{PR_URL}\n")); + assert!(!result.stderr.contains(OFFER), "stderr: {}", result.stderr); + assert!(!scenario.path("home/.thirdshift/config.toml").exists()); + assert_eq!(scenario.gh_state()["prs"][0]["state"], "OPEN"); +} + +#[test] +fn an_unwritable_user_config_is_a_warning_and_the_run_completes_on_the_defaults() { + use std::os::unix::fs::PermissionsExt; + + let scenario = Scenario::new(); + scenario.agent_does(AGENT_OPENS_PR); + let dir = scenario.path("home/.thirdshift"); + fs::create_dir_all(dir.join("logs")).unwrap(); + fs::set_permissions(&dir, fs::Permissions::from_mode(0o555)).unwrap(); + + let result = scenario.run_on_terminal( + &[&scenario.issue_url(7)], + &[], + &[(OFFER, "y"), (MERGE, "y"), (PULL, ""), (NOTIFY, "")], + ); + + fs::set_permissions(&dir, fs::Permissions::from_mode(0o755)).unwrap(); + assert_ended(&scenario, &result, "ready for review", "OPEN"); + assert!( + result.stderr.contains("thirdshift: warning: "), + "terminal: {}", + result.stderr + ); + assert_eq!(result.user_config, None); +} + +#[test] +fn ctrl_c_at_the_offer_writes_no_file_and_does_no_work() { + let scenario = Scenario::new(); + scenario.agent_does(AGENT_OPENS_PR); + + let result = scenario.run_on_terminal(&[&scenario.issue_url(7)], &[], &[(OFFER, CTRL_C)]); + + assert_ne!(result.code, Some(0), "terminal: {}", result.stderr); + assert_eq!(result.stdout, ""); + assert_eq!(result.user_config, None); + assert!(scenario.claude_calls().is_empty(), "the Run started"); + assert!( + scenario.gh_calls().is_empty(), + "thirdshift asked GitHub: {:?}", + scenario.gh_calls() + ); +} + +#[test] +fn ctrl_c_during_the_questions_writes_no_file_and_does_no_work() { + let scenario = Scenario::new(); + scenario.agent_does(AGENT_OPENS_PR); + + let result = scenario.run_on_terminal( + &[&scenario.issue_url(7)], + &[], + &[(OFFER, "y"), (MERGE, "y"), (PULL, CTRL_C)], + ); + + assert_ne!(result.code, Some(0), "terminal: {}", result.stderr); + assert_eq!(result.stdout, ""); + assert_eq!(result.user_config, None); + assert!(scenario.claude_calls().is_empty(), "the Run started"); +} + +#[test] +fn help_version_update_and_email_test_never_offer_setup() { + let scenario = Scenario::new(); + + for args in [ + vec!["help"], + vec!["version"], + vec!["update"], + vec!["email-test", "me@example.com"], + ] { + let result = scenario.run_on_terminal(&args, &[], &[]); + + assert!( + !result.stderr.contains(OFFER), + "{args:?}: {}", + result.stderr + ); + assert_eq!(result.user_config, None, "{args:?}"); + } +} From 6fa468337a81617335ce624c7400f35bfd7ac6de Mon Sep 17 00:00:00 2001 From: Jacob Stephens Date: Wed, 30 Sep 2026 00:27:44 +0000 Subject: [PATCH 2/2] Address review: one error path for the offer and load, test flags and Ctrl-C with --email Co-Authored-By: Claude Opus 5.5 --- src/config.rs | 16 +++++++++------ src/main.rs | 6 +----- tests/setup_offer.rs | 46 +++++++++++++++++++++++++++++++++++++++++++- 3 files changed, 56 insertions(+), 12 deletions(-) diff --git a/src/config.rs b/src/config.rs index 54201e9c1..1ed6cfc35 100644 --- a/src/config.rs +++ b/src/config.rs @@ -167,7 +167,9 @@ pub fn setup() -> Result { let asked = questions::has_terminal(); let mut send_test = false; if asked { - (text, send_test) = answered(&text, &path, &home)?; + let answers; + (text, answers) = answered(&text, &path, &home)?; + send_test = answers.send_test(); } let written = match &existing { None => { @@ -224,7 +226,9 @@ pub fn offer_setup() -> Result<()> { let mut text = with_email_to(suggested_address(&home)); let mut send_test = false; if accepted { - (text, send_test) = answered(&text, &path, &home)?; + let answers; + (text, answers) = answered(&text, &path, &home)?; + send_test = answers.send_test(); } if let Err(error) = write_new(&home, &path, &text) { progress::step(format_args!( @@ -252,12 +256,12 @@ pub fn offer_setup() -> Result<()> { } /// Ask the Setup questions, with the settings in `text`, the User config at -/// `path`, as the default answers. Returns `text` with the answers, and -/// whether the user asked for a test email. -fn answered(text: &str, path: &Path, home: &Path) -> Result<(String, bool)> { +/// `path`, as the default answers. Returns `text` with the answers, and the +/// answers. +fn answered(text: &str, path: &Path, home: &Path) -> Result<(String, Answers)> { let current = UserConfig::parse(text, path, home)?; let answers = questions::ask(¤t, || suggested_address(home))?; - Ok((with_answers(text, &answers)?, answers.send_test())) + Ok((with_answers(text, &answers)?, answers)) } /// Write `text` as the User config at `path`, where there is none yet, diff --git a/src/main.rs b/src/main.rs index 238c3a2a9..fb9c81dd6 100644 --- a/src/main.rs +++ b/src/main.rs @@ -105,11 +105,7 @@ fn main() -> ExitCode { Ok(Command::Run(run_args)) => run_args, Err(error) => return argument_error(format_args!("{error:#}")), }; - if let Err(error) = config::offer_setup() { - progress::step(format_args!("{error:#}")); - return ExitCode::FAILURE; - } - let config = match UserConfig::load() { + let config = match config::offer_setup().and_then(|()| UserConfig::load()) { Ok(config) => config, Err(error) => { progress::step(format_args!("{error:#}")); diff --git a/tests/setup_offer.rs b/tests/setup_offer.rs index 636f033c3..6eea5a336 100644 --- a/tests/setup_offer.rs +++ b/tests/setup_offer.rs @@ -8,6 +8,7 @@ mod support; use std::fs; +use support::resend::ResendStandIn; use support::{CTRL_C, Scenario, TerminalResult}; /// The agent commits its work and opens a PR that closes issue #7, into @@ -25,6 +26,10 @@ const OFFER: &str = "Set your defaults now? [Y/n]"; const MERGE: &str = "Merge run?"; const PULL: &str = "fast-forward"; const NOTIFY: &str = "Run notifications, an email"; +const TO: &str = "Send Run notifications to"; +const FROM: &str = "Send them from"; +const TEST_EMAIL: &str = "test email now?"; +const KEY: &str = "re_secret_123"; /// The first line a Run prints once it starts its work. const FIRST_STEP: &str = "thirdshift: "; @@ -92,6 +97,35 @@ fn no_merge_in_the_command_wins_over_a_fresh_merge_always() { assert_eq!(config["merge"]["always"].as_bool(), Some(true)); } +#[test] +fn no_email_in_the_command_wins_over_fresh_run_notifications() { + let scenario = Scenario::new(); + let resend = ResendStandIn::replying(200, r#"{"id":"1"}"#); + scenario.agent_does(AGENT_OPENS_PR); + + let result = scenario.run_on_terminal( + &["--no-email", &scenario.issue_url(7)], + &[ + ("RESEND_API_KEY", KEY), + ("THIRDSHIFT_RESEND_URL", resend.url()), + ], + &[ + (OFFER, "y"), + (MERGE, ""), + (PULL, ""), + (NOTIFY, "y"), + (TO, "me@example.com"), + (FROM, ""), + (TEST_EMAIL, "n"), + ], + ); + + assert_ended(&scenario, &result, "ready for review", "OPEN"); + let config: toml::Table = result.user_config.unwrap().parse().unwrap(); + assert_eq!(config["email"]["always"].as_bool(), Some(true)); + assert!(resend.requests().is_empty(), "{:?}", resend.requests()); +} + #[test] fn declining_writes_the_defaults_and_a_second_run_does_not_offer() { let unattended = Scenario::new(); @@ -184,7 +218,16 @@ fn ctrl_c_at_the_offer_writes_no_file_and_does_no_work() { let scenario = Scenario::new(); scenario.agent_does(AGENT_OPENS_PR); - let result = scenario.run_on_terminal(&[&scenario.issue_url(7)], &[], &[(OFFER, CTRL_C)]); + let resend = ResendStandIn::replying(200, r#"{"id":"1"}"#); + + let result = scenario.run_on_terminal( + &["--email", "me@example.com", &scenario.issue_url(7)], + &[ + ("RESEND_API_KEY", KEY), + ("THIRDSHIFT_RESEND_URL", resend.url()), + ], + &[(OFFER, CTRL_C)], + ); assert_ne!(result.code, Some(0), "terminal: {}", result.stderr); assert_eq!(result.stdout, ""); @@ -195,6 +238,7 @@ fn ctrl_c_at_the_offer_writes_no_file_and_does_no_work() { "thirdshift asked GitHub: {:?}", scenario.gh_calls() ); + assert!(resend.requests().is_empty(), "a Run notification went"); } #[test]