From 070d9ae20c40834f288e076a21fa42196841757f Mon Sep 17 00:00:00 2001 From: Jacob Stephens Date: Tue, 29 Sep 2026 18:52:19 +0000 Subject: [PATCH 1/2] Read the User config with merge.always, and accept Run flags before or after the Issue URL thirdshift reads ~/.thirdshift/config.toml at the start of a Run. merge.always = true makes every Run a Merge run; no-merge/--no-merge overrides it for one Run. An unparsable config or an unknown key or section stops the Run before any work, naming the file and the key. update, version and help never read it. Co-Authored-By: Claude Opus 5.5 --- Cargo.lock | 77 +++++++++++++++ Cargo.toml | 1 + README.md | 17 +++- src/args.rs | 58 ++++++++++++ src/config.rs | 108 +++++++++++++++++++++ src/main.rs | 54 ++++++----- tests/command_surface.rs | 78 +++++++++++++-- tests/support/mod.rs | 11 +++ tests/update.rs | 16 ++++ tests/user_config.rs | 198 +++++++++++++++++++++++++++++++++++++++ 10 files changed, 585 insertions(+), 33 deletions(-) create mode 100644 src/args.rs create mode 100644 src/config.rs create mode 100644 tests/user_config.rs diff --git a/Cargo.lock b/Cargo.lock index e7bab24fd..8155aaca5 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -265,6 +265,12 @@ version = "1.0.5" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "92773504d58c093f6de2459af4af33faa518c13451eb8f2b5698ed3d36e7c813" +[[package]] +name = "equivalent" +version = "1.0.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "877a4ace8713b0bcf2a4e7eec82529c029f1d0619886d18145fea96c3ffe5c0f" + [[package]] name = "errno" version = "0.3.14" @@ -362,6 +368,12 @@ dependencies = [ "wasm-bindgen", ] +[[package]] +name = "hashbrown" +version = "0.17.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "ed5909b6e89a2db4456e54cd5f673791d7eca6732202bbf2a9cc504fe2f9b84a" + [[package]] name = "homedir" version = "0.3.6" @@ -631,6 +643,16 @@ dependencies = [ "quote", ] +[[package]] +name = "indexmap" +version = "2.14.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "cc4e190f5d26ca7051642629da2c52fc03bde85a03197c99408dcd291734c855" +dependencies = [ + "equivalent", + "hashbrown", +] + [[package]] name = "ipnet" version = "2.12.2" @@ -1285,6 +1307,15 @@ dependencies = [ "zmij", ] +[[package]] +name = "serde_spanned" +version = "1.1.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "6662b5879511e06e8999a8a235d848113e942c9124f211511b16466ee2995f26" +dependencies = [ + "serde_core", +] + [[package]] name = "shlex" version = "2.0.1" @@ -1428,6 +1459,7 @@ dependencies = [ "serde_json", "signal-hook", "tempfile", + "toml", ] [[package]] @@ -1504,6 +1536,45 @@ dependencies = [ "tokio", ] +[[package]] +name = "toml" +version = "1.1.6+spec-1.1.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "920602543f0911ab71da12c50d59701da54c196d1a2bf5cb4b75667f137a406a" +dependencies = [ + "indexmap", + "serde_core", + "serde_spanned", + "toml_datetime", + "toml_parser", + "toml_writer", + "winnow", +] + +[[package]] +name = "toml_datetime" +version = "1.1.1+spec-1.1.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "3165f65f62e28e0115a00b2ebdd37eb6f3b641855f9d636d3cd4103767159ad7" +dependencies = [ + "serde_core", +] + +[[package]] +name = "toml_parser" +version = "1.1.3+spec-1.1.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "1d38ac1cf9b95face32296c0a3ede1fdc270627c9d9c02a7274dd6d960dc4d56" +dependencies = [ + "winnow", +] + +[[package]] +name = "toml_writer" +version = "1.1.2+spec-1.1.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "7d56353a2a665ad0f41a421187180aab746c8c325620617ad883a99a1cbe66d2" + [[package]] name = "tower" version = "0.5.3" @@ -1976,6 +2047,12 @@ version = "0.52.6" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "589f6da84c646204747d1270a2a5661ea66ed1cced2631d546fdfb155959f9ec" +[[package]] +name = "winnow" +version = "1.0.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "23b97319f7b8343df12cc98938e5c3eb436064524c8d2b4e30a1d3a36eecdf81" + [[package]] name = "writeable" version = "0.6.4" diff --git a/Cargo.toml b/Cargo.toml index 87395271f..9322b4cd5 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -19,6 +19,7 @@ libc = "0.2.189" serde_json = "1.0.151" signal-hook = "0.4.4" tempfile = "3.27.0" +toml = "1.1.6" # The profile that 'dist' will build with [profile.dist] diff --git a/README.md b/README.md index b30c41fbf..9e9c8ad6b 100644 --- a/README.md +++ b/README.md @@ -104,7 +104,7 @@ To have the Run merge its pull request instead of leaving it for your review, st thirdshift merge https://github.com/acme/widgets/issues/7 ``` -`thirdshift --merge ` does the same. The agent gets the same implement prompt either way; the differences are the Self-merge at the end and the review of [Foreign commits](#foreign-commits-in-a-merge-run). The keyword goes before the URL: anything after the URL, `--merge` included, is an argument error. +`thirdshift --merge ` does the same, and either goes before or after the URL. The agent gets the same implement prompt either way; the differences are the Self-merge at the end and the review of [Foreign commits](#foreign-commits-in-a-merge-run). To make every Run on a machine a Merge run, set `merge.always` in the [User config](#user-config); `no-merge` (or `--no-merge`), before or after the URL, then makes one Run end ready for review instead. Giving a flag twice, or `merge` together with `no-merge`, is an argument error. A Merge run ends in one of three ways: @@ -114,7 +114,7 @@ A Merge run ends in one of three ways: - **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. -- **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, or something follows it; the error and the help text go to stderr. Any other failure exits `1`. +- **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: @@ -128,6 +128,19 @@ thirdshift help # print every form of the command, each with a one-line des Uncommitted changes in your clone are fine: the Run works in its own worktree from `origin`, so they are simply left out. Unpushed commits on the Base branch are not: push them first, or the Run stops. +### User config + +A **User config** at `~/.thirdshift/config.toml` sets this machine's defaults for every Run. It is optional: with no file, or one that says nothing about a setting, a Run does only what its command asks for. + +```toml +[merge] +always = true # every Run is a Merge run, without the merge word +``` + +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. + +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. `update`, `version` and `help` never read it, so a broken User config can't block them. + ### Logs Each session's full transcript, as Claude Code's `stream-json` output, is written to its own file under `~/.thirdshift/logs/` (created if missing): diff --git a/src/args.rs b/src/args.rs new file mode 100644 index 000000000..6ac1617c4 --- /dev/null +++ b/src/args.rs @@ -0,0 +1,58 @@ +//! The command line: which command, and for a Run, its Issue URL and flags. + +use anyhow::{Result, bail}; + +use crate::issue::IssueUrl; +use crate::run::Goal; + +/// What thirdshift was asked to do. +pub enum Command { + Help, + Version, + Update, + Run(RunArgs), +} + +/// A Run's arguments. +pub struct RunArgs { + pub issue: IssueUrl, + /// The goal `merge` or `no-merge` asked for, if either was given; without + /// one, the User config decides. + pub goal: Option, +} + +/// Parse the arguments after the program name. `help`, `version` and +/// `update` 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. +pub fn parse(args: &[String]) -> Result { + match args.first().map(String::as_str) { + Some("help" | "--help" | "-h") => return Ok(Command::Help), + Some("version" | "--version" | "-V") => return Ok(Command::Version), + Some("update") => return Ok(Command::Update), + _ => {} + } + let mut url = None; + let mut goal = None; + for arg in args { + let flag = match arg.as_str() { + "merge" | "--merge" => Goal::Merged, + "no-merge" | "--no-merge" => Goal::ReadyForReview, + _ => { + if url.is_some() { + bail!("unexpected argument after the Issue URL: {arg}"); + } + url = Some(IssueUrl::parse(arg)?); + continue; + } + }; + match goal { + None => goal = Some(flag), + Some(given) if given == flag => bail!("repeated argument: {arg}"), + Some(_) => bail!("merge and no-merge can't be used together"), + } + } + let Some(issue) = url else { + bail!("missing Issue URL"); + }; + Ok(Command::Run(RunArgs { issue, goal })) +} diff --git a/src/config.rs b/src/config.rs new file mode 100644 index 000000000..a97ea00ec --- /dev/null +++ b/src/config.rs @@ -0,0 +1,108 @@ +//! The User config: `~/.thirdshift/config.toml`, this machine's defaults for +//! every Run. Only a Run reads it, so a broken one can't block `update`, +//! `version` or `help`. + +use std::path::{Path, PathBuf}; + +use anyhow::{Context, Result, anyhow, bail}; +use toml::{Table, Value}; + +use crate::run::Goal; + +/// The settings a User config can hold. Each is what a Run does when its +/// command says nothing about it. +#[derive(Debug, Default, PartialEq, Eq)] +pub struct UserConfig { + /// `merge.always`: every Run is a Merge run unless told `no-merge`. + pub merge_always: bool, +} + +impl UserConfig { + /// The User config under `$HOME`, or the defaults if there is none. + pub fn load() -> Result { + let Some(home) = std::env::var_os("HOME").filter(|home| !home.is_empty()) else { + return Ok(UserConfig::default()); + }; + let path = PathBuf::from(home).join(".thirdshift/config.toml"); + match std::fs::read_to_string(&path) { + Ok(text) => UserConfig::parse(&text, &path), + Err(error) if error.kind() == std::io::ErrorKind::NotFound => Ok(UserConfig::default()), + Err(error) => Err(error).with_context(|| format!("can't read {}", path.display())), + } + } + + /// Parse `text`, the contents of the User config at `path`. Any key or + /// section thirdshift doesn't know is an error, so a typo can't silently + /// leave a setting unset. + fn parse(text: &str, path: &Path) -> Result { + let in_file = || format!("in the User config {}", path.display()); + let table: Table = text + .parse() + .map_err(|error| anyhow!("{error}")) + .with_context(|| format!("can't parse the User config {}", path.display()))?; + let mut config = UserConfig::default(); + for (section, value) in &table { + let settings = match (section.as_str(), value) { + ("merge", Value::Table(settings)) => settings, + ("merge", _) => bail!("merge must be the section [merge] {}", in_file()), + (_, Value::Table(_)) => bail!("unknown section [{section}] {}", in_file()), + _ => bail!("unknown key {section} {}", in_file()), + }; + for (key, value) in settings { + match (key.as_str(), value) { + ("always", Value::Boolean(always)) => config.merge_always = *always, + ("always", _) => bail!("merge.always must be true or false {}", in_file()), + _ => bail!("unknown key merge.{key} {}", in_file()), + } + } + } + Ok(config) + } + + /// The goal of a Run whose command gave no `merge` or `no-merge`. + pub fn default_goal(&self) -> Goal { + if self.merge_always { + Goal::Merged + } else { + Goal::ReadyForReview + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + + fn parse(text: &str) -> Result { + UserConfig::parse(text, Path::new("/home/me/.thirdshift/config.toml")) + } + + #[test] + fn merge_always_is_read() { + assert!(parse("[merge]\nalways = true\n").unwrap().merge_always); + assert!(!parse("[merge]\nalways = false\n").unwrap().merge_always); + assert!(!parse("").unwrap().merge_always); + } + + #[test] + fn errors_name_the_file_and_the_offending_key() { + for (text, named) in [ + ("[merge\n", "can't parse"), + ("[merge]\nalway = true\n", "unknown key merge.alway"), + ("[logs]\n", "unknown section [logs]"), + ("merge = true\n", "merge must be the section [merge]"), + ("colour = true\n", "unknown key colour "), + ( + "[merge]\nalways = 1\n", + "merge.always must be true or false", + ), + ] { + let error = format!("{:#}", parse(text).unwrap_err()); + assert!(error.contains(named), "{text:?}: {error}"); + assert!( + error.contains("/home/me/.thirdshift/config.toml"), + "{text:?}: {error}" + ); + } + } +} diff --git a/src/main.rs b/src/main.rs index 24ebf459a..dbad53239 100644 --- a/src/main.rs +++ b/src/main.rs @@ -1,5 +1,7 @@ +mod args; mod branch; mod ci; +mod config; mod failed_run; mod git; mod github; @@ -19,31 +21,40 @@ mod worktree; use std::process::ExitCode; -use issue::IssueUrl; -use run::Goal; +use args::{Command, RunArgs}; +use config::UserConfig; const HELP: &str = "\ thirdshift turns a GitHub issue into a ready-for-review pull request, or a merged one, unattended. -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 update Update thirdshift to the latest release - thirdshift version Print thirdshift's version - thirdshift help Print this help +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 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. + +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: + + [merge] + always = true "; fn main() -> ExitCode { let args: Vec = std::env::args().skip(1).collect(); - let (goal, rest) = match args.first().map(String::as_str) { - Some("help" | "--help" | "-h") => { + let RunArgs { issue, goal } = match args::parse(&args) { + Ok(Command::Help) => { print!("{HELP}"); return ExitCode::SUCCESS; } - Some("version" | "--version" | "-V") => { + Ok(Command::Version) => { println!("thirdshift {}", env!("CARGO_PKG_VERSION")); return ExitCode::SUCCESS; } - Some("update") => { + Ok(Command::Update) => { return match update::update() { Ok(outcome) => { progress::step(outcome); @@ -55,21 +66,16 @@ fn main() -> ExitCode { } }; } - Some("merge" | "--merge") => (Goal::Merged, &args[1..]), - _ => (Goal::ReadyForReview, &args[..]), - }; - let Some((url, after)) = rest.split_first() else { - return argument_error(format_args!("missing Issue URL")); - }; - let issue = match IssueUrl::parse(url) { - Ok(issue) => issue, + Ok(Command::Run(run_args)) => run_args, Err(error) => return argument_error(format_args!("{error:#}")), }; - if let Some(extra) = after.first() { - return argument_error(format_args!( - "unexpected argument after the Issue URL: {extra}" - )); - } + let goal = match UserConfig::load() { + Ok(config) => goal.unwrap_or(config.default_goal()), + Err(error) => { + progress::step(format_args!("{error:#}")); + return ExitCode::FAILURE; + } + }; if let Err(error) = interrupt::install() { progress::step(format_args!("{error:#}")); return ExitCode::FAILURE; diff --git a/tests/command_surface.rs b/tests/command_surface.rs index 1e6e5934d..806b6b4f2 100644 --- a/tests/command_surface.rs +++ b/tests/command_surface.rs @@ -137,17 +137,16 @@ fn merge_without_an_issue_url_prints_an_error_and_the_help_to_stderr() { } #[test] -fn anything_after_the_issue_url_prints_an_error_and_the_help_to_stderr() { +fn an_extra_argument_prints_an_error_and_the_help_to_stderr() { let scenario = Scenario::new(); let url = scenario.issue_url(7); - for args in [ - vec![url.as_str(), "--merge"], - vec![url.as_str(), "merge"], - vec!["merge", url.as_str(), "extra"], - vec!["--merge", url.as_str(), "--merge"], + for (args, extra) in [ + (vec![url.as_str(), "extra"], "extra"), + (vec!["merge", url.as_str(), "extra"], "extra"), + (vec![url.as_str(), url.as_str()], url.as_str()), + (vec![url.as_str(), "--no-merge", "frobnicate"], "frobnicate"), ] { - let extra = args.last().unwrap(); let result = scenario.run(&args); assert_argument_error( @@ -158,3 +157,68 @@ fn anything_after_the_issue_url_prints_an_error_and_the_help_to_stderr() { assert!(scenario.claude_calls().is_empty(), "{args:?} started a Run"); } } + +#[test] +fn a_repeated_flag_prints_an_error_and_the_help_to_stderr() { + let scenario = Scenario::new(); + let url = scenario.issue_url(7); + + for (args, repeated) in [ + (vec!["merge", url.as_str(), "merge"], "merge"), + (vec!["--merge", url.as_str(), "--merge"], "--merge"), + (vec!["merge", "--merge", url.as_str()], "--merge"), + (vec![url.as_str(), "no-merge", "--no-merge"], "--no-merge"), + ] { + let result = scenario.run(&args); + + assert_argument_error( + &scenario, + &result, + &format!("repeated argument: {repeated}"), + ); + assert!(scenario.claude_calls().is_empty(), "{args:?} started a Run"); + } +} + +#[test] +fn merge_with_no_merge_prints_an_error_and_the_help_to_stderr() { + let scenario = Scenario::new(); + let url = scenario.issue_url(7); + + for args in [ + vec!["merge", url.as_str(), "no-merge"], + vec!["--no-merge", "--merge", url.as_str()], + vec![url.as_str(), "--merge", "no-merge"], + ] { + let result = scenario.run(&args); + + assert_argument_error( + &scenario, + &result, + "merge and no-merge 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(); + + for no_merge in ["no-merge", "--no-merge"] { + let result = scenario.run(&[no_merge]); + + assert_argument_error(&scenario, &result, "missing Issue URL"); + } +} + +#[test] +fn help_lists_no_merge_and_the_user_config() { + let scenario = Scenario::new(); + + let help = scenario.run(&["help"]).stdout; + + for mention in ["--no-merge", "~/.thirdshift/config.toml", "merge.always"] { + assert!(help.contains(mention), "help lacks {mention:?}: {help}"); + } +} diff --git a/tests/support/mod.rs b/tests/support/mod.rs index 62fcb6265..a41ecf21c 100644 --- a/tests/support/mod.rs +++ b/tests/support/mod.rs @@ -8,6 +8,7 @@ //! it rejects non-fast-forward pushes, so no rebase or //! force-push can reach it //! home/ $HOME: .gitconfig with identity and the insteadOf rule +//! home/.thirdshift/ the User config, config.toml, if the test writes one //! home/.config/ $XDG_CONFIG_HOME, where an install receipt would be //! bin/ fake gh and claude, first on PATH //! tmp/ $TMPDIR, so leftover temp directories are visible @@ -198,6 +199,16 @@ impl Scenario { command } + /// Write `toml` as the User config, `~/.thirdshift/config.toml` under the + /// scenario's `$HOME`, and return its path. + pub fn user_config_is(&self, toml: &str) -> PathBuf { + let dir = self.path("home/.thirdshift"); + fs::create_dir_all(&dir).unwrap(); + let path = dir.join("config.toml"); + fs::write(&path, toml).unwrap(); + path + } + /// Set issue `number`'s state on the fake GitHub: `"OPEN"` or `"CLOSED"`. pub fn issue_is(&self, number: u32, state: &str) { let mut gh = self.gh_state(); diff --git a/tests/update.rs b/tests/update.rs index b7e0a3e81..74c0651c4 100644 --- a/tests/update.rs +++ b/tests/update.rs @@ -286,3 +286,19 @@ fn when_github_is_unreachable_update_fails_with_an_explanation() { result.stderr ); } + +#[test] +fn update_never_reads_the_user_config_so_a_broken_one_cannot_block_it() { + let scenario = installed_scenario(); + scenario.user_config_is("[merge\nalway = true\n"); + let github = FakeReleases::start(); + github.publish(VERSION, "true"); + + let result = github.update(&scenario); + + assert_eq!(result.code, Some(0), "stderr: {}", result.stderr); + assert_eq!( + result.stderr, + format!("thirdshift: thirdshift {VERSION} is the latest release\n") + ); +} diff --git a/tests/user_config.rs b/tests/user_config.rs new file mode 100644 index 000000000..5463d5e19 --- /dev/null +++ b/tests/user_config.rs @@ -0,0 +1,198 @@ +//! The User config, `~/.thirdshift/config.toml`: `merge.always` makes every +//! Run a Merge run unless the command says `no-merge`, and a config thirdshift +//! can't use stops the Run before any work. + +mod support; + +use support::{RunResult, Scenario}; + +/// 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"; + +/// Assert the Run ended with PR #1 merged, as a Merge run does. +fn assert_merged(scenario: &Scenario, result: &RunResult) { + assert_eq!(result.code, Some(0), "stderr: {}", result.stderr); + assert_eq!(result.stdout, format!("{PR_URL}\n")); + assert_eq!( + result.stderr.lines().last(), + Some(format!("thirdshift: PR {PR_URL} is merged").as_str()), + "stderr: {}", + result.stderr + ); + assert_eq!(scenario.gh_state()["prs"][0]["state"], "MERGED"); +} + +/// Assert the Run ended with PR #1 open and ready for review, never merged. +fn assert_ready_for_review(scenario: &Scenario, result: &RunResult) { + assert_eq!(result.code, Some(0), "stderr: {}", result.stderr); + assert_eq!(result.stdout, format!("{PR_URL}\n")); + assert_eq!( + result.stderr.lines().last(), + Some(format!("thirdshift: PR {PR_URL} is ready for review").as_str()), + "stderr: {}", + result.stderr + ); + assert_eq!(scenario.gh_state()["prs"][0]["state"], "OPEN"); + assert!( + !scenario + .gh_calls() + .iter() + .any(|call| call.starts_with(&["pr".to_string(), "merge".to_string()])), + "thirdshift ran gh pr merge" + ); +} + +#[test] +fn merge_always_makes_a_run_without_the_merge_word_a_merge_run() { + let scenario = Scenario::new(); + scenario.user_config_is("[merge]\nalways = true\n"); + scenario.agent_does(AGENT_OPENS_PR); + + let result = scenario.run(&[&scenario.issue_url(7)]); + + assert_merged(&scenario, &result); +} + +#[test] +fn no_merge_overrides_merge_always_for_one_run() { + for flag in ["no-merge", "--no-merge"] { + for flag_first in [true, false] { + let scenario = Scenario::new(); + scenario.user_config_is("[merge]\nalways = true\n"); + scenario.agent_does(AGENT_OPENS_PR); + let url = scenario.issue_url(7); + let args = if flag_first { + [flag, url.as_str()] + } else { + [url.as_str(), flag] + }; + + let result = scenario.run(&args); + + assert_ready_for_review(&scenario, &result); + } + } +} + +#[test] +fn without_a_config_file_a_run_is_not_a_merge_run() { + let scenario = Scenario::new(); + scenario.agent_does(AGENT_OPENS_PR); + + let result = scenario.run(&[&scenario.issue_url(7)]); + + assert_ready_for_review(&scenario, &result); +} + +#[test] +fn a_config_that_says_nothing_about_merging_leaves_a_run_not_a_merge_run() { + for config in [ + "", + "# nothing set yet\n", + "[merge]\n", + "[merge]\nalways = false\n", + ] { + let scenario = Scenario::new(); + scenario.user_config_is(config); + scenario.agent_does(AGENT_OPENS_PR); + + let result = scenario.run(&[&scenario.issue_url(7)]); + + assert_ready_for_review(&scenario, &result); + } +} + +#[test] +fn the_merge_word_goes_before_or_after_the_issue_url() { + for merge in ["merge", "--merge"] { + let scenario = Scenario::new(); + scenario.agent_does(AGENT_OPENS_PR); + + let result = scenario.run(&[&scenario.issue_url(7), merge]); + + assert_merged(&scenario, &result); + } +} + +#[test] +fn an_unparsable_config_stops_the_run_naming_the_file() { + let scenario = Scenario::new(); + let path = scenario.user_config_is("[merge\nalways = true\n"); + scenario.agent_does(AGENT_OPENS_PR); + + let result = scenario.run(&[&scenario.issue_url(7)]); + + assert_eq!(result.code, Some(1), "stderr: {}", result.stderr); + scenario.assert_rejected_before_any_work(&result, &path.display().to_string()); + assert!(scenario.gh_calls().is_empty(), "thirdshift called gh"); +} + +#[test] +fn an_unknown_key_or_section_stops_the_run_naming_it_and_the_file() { + for (config, named) in [ + ("[merge]\nalway = true\n", "merge.alway"), + ("[merges]\nalways = true\n", "[merges]"), + ("colour = true\n", "colour"), + ] { + let scenario = Scenario::new(); + let path = scenario.user_config_is(config); + scenario.agent_does(AGENT_OPENS_PR); + + let result = scenario.run(&[&scenario.issue_url(7)]); + + assert_eq!(result.code, Some(1), "{config}: {}", result.stderr); + scenario.assert_rejected_before_any_work(&result, &path.display().to_string()); + assert!( + result.stderr.contains(named), + "expected {named:?} in stderr for {config:?}: {}", + result.stderr + ); + } +} + +#[test] +fn a_value_of_the_wrong_type_stops_the_run_naming_the_key() { + for config in ["merge = true\n", "[merge]\nalways = \"yes\"\n"] { + let scenario = Scenario::new(); + let path = scenario.user_config_is(config); + + let result = scenario.run(&[&scenario.issue_url(7)]); + + assert_eq!(result.code, Some(1), "{config}: {}", result.stderr); + scenario.assert_rejected_before_any_work(&result, &path.display().to_string()); + assert!(result.stderr.contains("merge"), "stderr: {}", result.stderr); + } +} + +#[test] +fn argument_errors_come_before_the_config_is_read() { + let scenario = Scenario::new(); + scenario.user_config_is("[merge\n"); + + let result = scenario.run(&["merge", "no-merge", &scenario.issue_url(7)]); + + assert_eq!(result.code, Some(2), "stderr: {}", result.stderr); +} + +#[test] +fn help_and_version_succeed_with_a_broken_config() { + for config in ["[merge\n", "[merge]\nalway = true\n"] { + let scenario = Scenario::new(); + scenario.user_config_is(config); + + for command in ["help", "version"] { + let result = scenario.run(&[command]); + + assert_eq!(result.code, Some(0), "{command}: {}", result.stderr); + assert_eq!(result.stderr, "", "{command}"); + } + } +} From cbd7bba17ff870d734e85cefc26742650306918f Mon Sep 17 00:00:00 2001 From: Jacob Stephens Date: Tue, 29 Sep 2026 18:55:16 +0000 Subject: [PATCH 2/2] Name what the Run argument parser holds, and phrase config errors once Co-Authored-By: Claude Opus 5.5 --- src/args.rs | 14 +++++++------- src/config.rs | 16 ++++++++-------- 2 files changed, 15 insertions(+), 15 deletions(-) diff --git a/src/args.rs b/src/args.rs index 6ac1617c4..1e9473ef3 100644 --- a/src/args.rs +++ b/src/args.rs @@ -31,27 +31,27 @@ pub fn parse(args: &[String]) -> Result { Some("update") => return Ok(Command::Update), _ => {} } - let mut url = None; + let mut issue = None; let mut goal = None; for arg in args { - let flag = match arg.as_str() { + let asked = match arg.as_str() { "merge" | "--merge" => Goal::Merged, "no-merge" | "--no-merge" => Goal::ReadyForReview, _ => { - if url.is_some() { + if issue.is_some() { bail!("unexpected argument after the Issue URL: {arg}"); } - url = Some(IssueUrl::parse(arg)?); + issue = Some(IssueUrl::parse(arg)?); continue; } }; match goal { - None => goal = Some(flag), - Some(given) if given == flag => bail!("repeated argument: {arg}"), + 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) = url else { + let Some(issue) = issue else { bail!("missing Issue URL"); }; Ok(Command::Run(RunArgs { issue, goal })) diff --git a/src/config.rs b/src/config.rs index a97ea00ec..92c915a45 100644 --- a/src/config.rs +++ b/src/config.rs @@ -35,24 +35,24 @@ impl UserConfig { /// section thirdshift doesn't know is an error, so a typo can't silently /// leave a setting unset. fn parse(text: &str, path: &Path) -> Result { - let in_file = || format!("in the User config {}", path.display()); + let file = format!("the User config {}", path.display()); let table: Table = text .parse() .map_err(|error| anyhow!("{error}")) - .with_context(|| format!("can't parse the User config {}", path.display()))?; + .with_context(|| format!("can't parse {file}"))?; let mut config = UserConfig::default(); for (section, value) in &table { let settings = match (section.as_str(), value) { ("merge", Value::Table(settings)) => settings, - ("merge", _) => bail!("merge must be the section [merge] {}", in_file()), - (_, Value::Table(_)) => bail!("unknown section [{section}] {}", in_file()), - _ => bail!("unknown key {section} {}", in_file()), + ("merge", _) => bail!("merge must be the section [merge] in {file}"), + (_, Value::Table(_)) => bail!("unknown section [{section}] in {file}"), + _ => bail!("unknown key {section} in {file}"), }; for (key, value) in settings { match (key.as_str(), value) { ("always", Value::Boolean(always)) => config.merge_always = *always, - ("always", _) => bail!("merge.always must be true or false {}", in_file()), - _ => bail!("unknown key merge.{key} {}", in_file()), + ("always", _) => bail!("merge.always must be true or false in {file}"), + _ => bail!("unknown key merge.{key} in {file}"), } } } @@ -87,7 +87,7 @@ mod tests { #[test] fn errors_name_the_file_and_the_offending_key() { for (text, named) in [ - ("[merge\n", "can't parse"), + ("[merge\n", "can't parse the User config"), ("[merge]\nalway = true\n", "unknown key merge.alway"), ("[logs]\n", "unknown section [logs]"), ("merge = true\n", "merge must be the section [merge]"),