diff --git a/README.md b/README.md index cdf7c7583..0c0079daf 100644 --- a/README.md +++ b/README.md @@ -242,15 +242,27 @@ parallel = 3 # how many Tickets a Spec run runs at once; default 3 [pickup] limit = 3 # how many open issues labelled in-progress stop a Pickup run taking another; default 3 + +[harness] +default = "claude" # the Harness every Run's sessions run on, claude or codex; default claude + +[harness.claude] +model = "" # the Model Claude Code's sessions run on; default blank, for Claude Code's own +effort = "" # how hard that Model reasons; default blank, for Claude Code's own + +[harness.codex] +model = "" # the Model Codex's sessions run on; default blank, for Codex's own +effort = "" # how hard that Model reasons; default blank, for Codex's own ``` Every key holds its real value, so a Run reading it does exactly what it does with no file. `email.to` has no default, so `setup` suggests one: the public email of your GitHub profile (from `gh api user`), else your global git `user.email`, unless that is a `@users.noreply.github.com` address, which can't receive mail. With neither, `email.to` is the only line written commented out, as above. `setup` never asks `gh` for more scopes, so a private GitHub email is not read, and a Run never looks the suggestion up: `--email` with no address and no `email.to` still stops the Run. From a terminal (stdin and stderr both terminals), `setup` first asks, on stderr: -1. Every Run a Merge run? (`merge.always`) -2. With that on, every Run may start a Base fix when the Base branch's CI is red? (`base.fix`). With it off, this is not asked, and `base.fix` is written at its default, `false`. -3. Every Run first fast-forwards your checkout of the Base branch? (`launch.pull`) -4. Run notifications? (`email.always`). If yes, the address (`email.to`), asked again until it has an `@`, then the sender (`email.from`). -5. With notifications on, the Resend API key, with input hidden. With none saved it asks `Resend API key (input hidden, Enter to skip):`; with one in the [Credentials](#email), `Resend API key (input hidden, Enter keeps the saved one):`, and a new one replaces it. Surrounding spaces are trimmed, and anything that doesn't start with `re_` is asked again. A key you give is saved in the Credentials, `~/.thirdshift/credentials.toml`, created with mode 600 (and `~/.thirdshift` with it) or edited in place, keeping its comments and anything else in it and changing only `resend.key`; `setup` then prints `wrote the Credentials `. The key is never printed, nor written to the User config. Skipping writes no Credentials and says how to add a key later: rerun `thirdshift setup`, or set `RESEND_API_KEY`. With `RESEND_API_KEY` set and not empty, which wins over the Credentials, nothing is asked, and it says the key comes from `RESEND_API_KEY`. With a key found or given, it offers to send a test email (default No), as `thirdshift email-test` does, once the files are written. +1. The Harness for every Run's sessions (`harness.default`), listing both, each marked where sessions can't run on it here: `claude (not installed)` when `claude` isn't on `PATH`, and `codex (not supported yet)` until Codex sessions are. A marked Harness, or a name that isn't one, is refused and asked again. The default is the one sessions can run on, `claude` when both can. Then the Model (`harness..model`) and the Effort (`harness..effort`) for the Harness chosen only, each defaulting to the User config's, where Enter on none keeps the Harness's own default and `-` clears one. A named Model gets the test call a Run makes (see [Harness, Model and Effort](#harness-model-and-effort)); if Claude refuses it, Setup shows what Claude said and asks the Model and Effort again. The other Harness's section is left as it is. With no Harness sessions can run on here, Setup says so, asks none of this, and leaves the harness settings as they are. +2. Every Run a Merge run? (`merge.always`) +3. With that on, every Run may start a Base fix when the Base branch's CI is red? (`base.fix`). With it off, this is not asked, and `base.fix` is written at its default, `false`. +4. Every Run first fast-forwards your checkout of the Base branch? (`launch.pull`) +5. Run notifications? (`email.always`). If yes, the address (`email.to`), asked again until it has an `@`, then the sender (`email.from`). +6. With notifications on, the Resend API key, with input hidden. With none saved it asks `Resend API key (input hidden, Enter to skip):`; with one in the [Credentials](#email), `Resend API key (input hidden, Enter keeps the saved one):`, and a new one replaces it. Surrounding spaces are trimmed, and anything that doesn't start with `re_` is asked again. A key you give is saved in the Credentials, `~/.thirdshift/credentials.toml`, created with mode 600 (and `~/.thirdshift` with it) or edited in place, keeping its comments and anything else in it and changing only `resend.key`; `setup` then prints `wrote the Credentials `. The key is never printed, nor written to the User config. Skipping writes no Credentials and says how to add a key later: rerun `thirdshift setup`, or set `RESEND_API_KEY`. With `RESEND_API_KEY` set and not empty, which wins over the Credentials, nothing is asked, and it says the key comes from `RESEND_API_KEY`. With a key found or given, it offers to send a test email (default No), as `thirdshift email-test` does, once the files are written. 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`, `activity.quiet_skips`, `spec.parallel` and `pickup.limit` are not asked about. The answers are written like everything else below: in place, keeping your comments. Ctrl-C during the questions, the key included, writes nothing: neither the User config nor the Credentials. With notifications off, nothing about a key is asked, and saved Credentials stay as they were, so `--email` on a single Run still works. Credentials a Run would refuse (see [Email](#email)) are refused before any question, exit `1`, and not touched. With no terminal, as from cron or `thirdshift setup Result { set(&mut document, "email", "from", notifications.from.as_str()); set_email_to(&mut document, ¬ifications.to); } + if let Some((harness, chosen)) = &answers.harness { + set(&mut document, "harness", "default", harness.name()); + let section = format!("harness.{}", harness.name()); + for (key, value) in [("model", &chosen.model), ("effort", &chosen.effort)] { + set(&mut document, §ion, key, value.as_deref().unwrap_or("")); + } + } Ok(document.to_string()) } /// Set `section.key`, which `document` holds, to `value`, keeping the /// spacing and comment around the old value, and the comment in the same /// column where the spaces before it allow. An equal value is left as it was -/// written. +/// written. `section` may be a subsection, as `harness.claude`. fn set(document: &mut DocumentMut, section: &str, key: &str, value: impl Into) { let value = value.into(); - let old = document[section] - .as_table_like_mut() + let old = section + .split('.') + .try_fold(document.as_item_mut(), |item, name| { + item.as_table_like_mut()?.get_mut(name) + }) + .and_then(Item::as_table_like_mut) .and_then(|settings| settings.get_mut(key)) .and_then(Item::as_value_mut) .unwrap_or_else(|| panic!("a completed User config has {section}.{key}")); @@ -475,46 +486,23 @@ pub fn replace(path: &Path, text: &str) -> std::io::Result<()> { /// `text`, a User config a Run accepts, with each key it lacks added at its /// default, as `DEFAULTS` writes it. Everything already in `text` stays as /// it was: a missing key goes at the end of its section, and a missing -/// section after the last line of `text`. A key added to an inline table +/// section, or a key of a section `text` names only in its subsections' +/// headers, after the last line of `text`. A key added to an inline table /// gets no comment, as TOML has no place for one there. fn complete(text: &str) -> Result { let mut document: DocumentMut = text.parse().context("can't parse the User config")?; let defaults: DocumentMut = DEFAULTS.parse().expect("DEFAULTS is valid TOML"); let mut missing = DocumentMut::new(); - for (section, default_settings) in defaults.iter() { - let default_settings = default_settings - .as_table() - .expect("every key in DEFAULTS is in a section"); - match document.get_mut(section) { - None => { - missing.insert(section, Item::Table(default_settings.clone())); - } - Some(Item::Table(settings)) => { - for (key, item) in default_settings.iter() { - if !settings.contains_key(key) { - let mut key = default_settings - .key(key) - .expect("the key is in DEFAULTS") - .clone(); - key.leaf_decor_mut().set_prefix(""); - settings.insert_formatted(&key, item.clone()); - } - } - if section == "email" && !settings.is_dotted() { - note_email_to(settings, default_settings, text); - } - } - Some(Item::Value(toml_edit::Value::InlineTable(settings))) => { - for (key, item) in default_settings.iter() { - if !settings.contains_key(key) { - let mut value = item.as_value().expect("DEFAULTS has only values").clone(); - value.decor_mut().clear(); - settings.insert(key, value); - } - } - } - Some(_) => unreachable!("a Run refuses {section} that isn't a section"), - } + complete_table( + document.as_table_mut(), + defaults.as_table(), + missing.as_table_mut(), + ); + if let Some(Item::Table(email)) = document.get_mut("email") + && !email.is_dotted() + { + let defaults = defaults["email"].as_table().expect("DEFAULTS has [email]"); + note_email_to(email, defaults, text); } let mut completed = document.to_string(); let missing = missing.to_string(); @@ -529,6 +517,78 @@ fn complete(text: &str) -> Result { Ok(completed) } +/// Add to `settings`, a table of a User config, each key of `defaults`, the +/// same table in `DEFAULTS`, that it lacks, and the same for each of its +/// subsections. A missing key goes at the end of `settings`, unless +/// `settings` has no header of its own, as `[harness]` with only +/// `[harness.claude]` written; then it goes, as does a missing subsection, in +/// `missing`, the same table of what is written after the file. +fn complete_table( + settings: &mut toml_edit::Table, + defaults: &toml_edit::Table, + missing: &mut toml_edit::Table, +) { + let implicit = settings.is_implicit() && !settings.is_dotted(); + for (key, item) in defaults.iter() { + let default_key = defaults.key(key).expect("the key is in DEFAULTS"); + match (item, settings.get_mut(key)) { + (Item::Table(default_settings), Some(Item::Table(settings))) => { + let missing = missing.entry(key).or_insert_with(|| { + let mut table = toml_edit::Table::new(); + table.set_implicit(true); + table.set_position(default_settings.position()); + Item::Table(table) + }); + let missing = missing + .as_table_mut() + .expect("a missing section is a table"); + complete_table(settings, default_settings, missing); + } + ( + Item::Table(default_settings), + Some(Item::Value(toml_edit::Value::InlineTable(settings))), + ) => complete_inline(settings, default_settings), + (Item::Table(_), Some(_)) => unreachable!("a Run refuses {key} that isn't a section"), + (Item::Table(_), None) => { + missing.insert_formatted(default_key, item.clone()); + } + (_, Some(_)) => {} + (_, None) if implicit => { + missing.set_implicit(false); + missing.insert_formatted(default_key, item.clone()); + } + (_, None) => { + let mut key = default_key.clone(); + key.leaf_decor_mut().set_prefix(""); + settings.insert_formatted(&key, item.clone()); + } + } + } +} + +/// Add to `settings`, an inline table of a User config, each key of +/// `defaults`, the same table in `DEFAULTS`, that it lacks, with no comment. +fn complete_inline(settings: &mut toml_edit::InlineTable, defaults: &toml_edit::Table) { + for (key, item) in defaults.iter() { + match (item, settings.get_mut(key)) { + (Item::Table(defaults), Some(toml_edit::Value::InlineTable(settings))) => { + complete_inline(settings, defaults) + } + (_, Some(_)) => {} + (Item::Table(defaults), None) => { + let mut table = toml_edit::InlineTable::new(); + complete_inline(&mut table, defaults); + settings.insert(key, toml_edit::Value::InlineTable(table)); + } + (_, None) => { + let mut value = item.as_value().expect("DEFAULTS has only values").clone(); + value.decor_mut().clear(); + settings.insert(key, value); + } + } + } +} + /// With no `email.to` in `email`, the `[email]` section of `text`, and no /// commented-out one either, add the commented-out example line from /// `defaults`, the `[email]` section of `DEFAULTS`, just before `email.from`: @@ -620,6 +680,17 @@ parallel = 3 # how many Tickets a Spec run runs at once; default 3 [pickup] limit = 3 # how many open issues labelled in-progress stop a Pickup run taking another; default 3 + +[harness] +default = "claude" # the Harness every Run's sessions run on, claude or codex; default claude + +[harness.claude] +model = "" # the Model Claude Code's sessions run on; default blank, for Claude Code's own +effort = "" # how hard that Model reasons; default blank, for Claude Code's own + +[harness.codex] +model = "" # the Model Codex's sessions run on; default blank, for Codex's own +effort = "" # how hard that Model reasons; default blank, for Codex's own "#; /// The line `DEFAULTS` holds for `email.to`, which has no default. @@ -730,6 +801,8 @@ mod tests { Some(crate::email::DEFAULT_FROM) ); config.email.from = None; + assert_eq!(config.harness.default, Some(Harness::Claude)); + config.harness.default = None; assert_eq!(config, UserConfig::defaults(Path::new("/home/me"))); } @@ -994,7 +1067,8 @@ mod tests { } /// `text` completed, after checking a Run reads it with the settings it - /// had, plus the defaults for the keys it lacked. + /// had, plus the defaults for the keys it lacked, which change nothing a + /// Run does. fn completed(text: &str) -> String { let completed = complete(text).unwrap(); let before = parse(text).unwrap(); @@ -1006,6 +1080,10 @@ mod tests { ); after.email.from = None; } + if before.harness.default.is_none() { + assert_eq!(after.harness.default, Some(Harness::Claude)); + after.harness.default = None; + } assert_eq!(after, before, "{completed}"); let again = complete(&completed).unwrap(); assert_eq!(again, completed, "completing twice changed it"); @@ -1044,6 +1122,38 @@ mod tests { } } + #[test] + fn completing_adds_each_harness_key_a_user_config_lacks() { + for text in [ + "[harness]\ndefault = \"claude\"\n", + "[harness.claude]\nmodel = \"opus\" # mine\n", + "[harness.codex]\neffort = \"max\"\n\n[merge]\nalways = true\n", + "[harness]\n\n[harness.claude]\neffort = \"high\"\n", + "harness.default = \"claude\"\nharness.claude.model = \"opus\"\n", + "harness = { claude = { model = \"opus\" } }\n", + "[harness]\nclaude = { effort = \"low\" }\n", + ] { + let completed = completed(text); + let config: toml::Table = completed.parse().unwrap(); + let harness = config["harness"].as_table().unwrap(); + assert!(harness.contains_key("default"), "{text:?}:\n{completed}"); + for name in ["claude", "codex"] { + let settings = harness[name].as_table().unwrap(); + for key in ["model", "effort"] { + assert!(settings.contains_key(key), "{text:?}:\n{completed}"); + } + } + // An inline table gains its missing keys on its own line. + let mut lines = completed.lines(); + for line in text.lines().filter(|line| !line.contains('{')) { + assert!( + lines.any(|kept| kept == line), + "{line:?} of {text:?} is lost or moved:\n{completed}" + ); + } + } + } + #[test] fn completing_adds_email_to_commented_out_once() { for text in [ @@ -1066,6 +1176,7 @@ mod tests { fn notifications_to(to: &str) -> Answers { Answers { + harness: None, merge_always: false, base_fix: false, launch_pull: false, diff --git a/src/harness.rs b/src/harness.rs index 616c8e760..cedd7c603 100644 --- a/src/harness.rs +++ b/src/harness.rs @@ -37,6 +37,40 @@ impl Harness { Harness::Codex => "codex", } } + + /// Every Harness, in the order Setup lists them. + pub const ALL: [Harness; 2] = [Harness::Claude, Harness::Codex]; + + /// Why sessions can't run on it on this machine, if they can't: Codex is + /// not supported yet, and otherwise its CLI must be on `PATH`. + pub fn unavailable(self) -> Option { + if self == Harness::Codex { + Some(Unavailable::NotSupportedYet) + } else if !on_path(self.name()) { + Some(Unavailable::NotInstalled) + } else { + None + } + } +} + +/// Why sessions can't run on a Harness. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum Unavailable { + /// thirdshift can't run its sessions yet. + NotSupportedYet, + /// Its CLI isn't on `PATH`. + NotInstalled, +} + +impl fmt::Display for Unavailable { + /// As Setup marks a Harness with it, as in `codex (not supported yet)`. + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + f.write_str(match self { + Unavailable::NotSupportedYet => "not supported yet", + Unavailable::NotInstalled => "not installed", + }) + } } /// The names a Harness is chosen by, for the messages that list them. @@ -63,7 +97,7 @@ pub struct Settings { impl Settings { /// The Model and Effort set for `harness`. - fn of(&self, harness: Harness) -> &ModelAndEffort { + pub fn of(&self, harness: Harness) -> &ModelAndEffort { match harness { Harness::Claude => &self.claude, Harness::Codex => &self.codex, @@ -170,20 +204,27 @@ impl Choice { /// Each failure says what to change. pub fn check(&self) -> Result<()> { let cli = self.harness.name(); - if self.harness == Harness::Codex { - bail!( - "harness codex, chosen by {}, is not supported yet: Codex sessions come in a \ + match self.harness.unavailable() { + Some(Unavailable::NotSupportedYet) => bail!( + "harness {cli}, chosen by {}, is not supported yet: Codex sessions come in a \ later thirdshift; choose harness claude", self.chosen_by - ); - } - if !on_path(cli) { - bail!( + ), + Some(Unavailable::NotInstalled) => bail!( "{cli} is not on PATH, and the Harness {cli} is chosen by {}: install it, \ or choose another Harness", self.chosen_by - ); + ), + None => {} } + self.test_call() + } + + /// Check that the Harness's CLI takes a minimal test call on the Model, + /// if one is named, with the Effort, if any. A refusal says what the CLI + /// said. Only Claude reaches it while Codex is not supported. + pub fn test_call(&self) -> Result<()> { + let cli = self.harness.name(); let Some(model) = &self.model else { return Ok(()); }; diff --git a/src/questions.rs b/src/questions.rs index baf80a117..b538111ed 100644 --- a/src/questions.rs +++ b/src/questions.rs @@ -13,10 +13,15 @@ use signal_hook::low_level; use crate::config::UserConfig; use crate::email::DEFAULT_FROM; +use crate::harness::{self, Choice, ChosenBy, Harness, ModelAndEffort, Unavailable}; use crate::resend_key::{self, Credentials, Source}; /// What the user chose. pub struct Answers { + /// `harness.default`, and the Model and Effort of the Harness it names, + /// each none for the Harness's own default; `None` when sessions can run + /// on no Harness here, and the harness settings stay as they were. + pub harness: Option<(Harness, ModelAndEffort)>, /// `merge.always`. pub merge_always: bool, /// `base.fix`: asked only with `merge.always` on, and otherwise its @@ -76,6 +81,7 @@ pub fn offer(path: &Path) -> Result { /// Ask the Setup questions, with the settings in `current` as the default /// answers, and `suggested_address` for `email.to` when `current` has none. +/// The Harness, Model and Effort come first, as [`ask_harness`] asks them. /// Base fixes are asked about only with every Run a Merge run; otherwise /// the answer is `base.fix`'s default, no. /// The address is re-asked until it has an `@`. With Run notifications on, @@ -87,6 +93,7 @@ pub fn ask( credentials: &Credentials, suggested_address: impl FnOnce() -> Option, ) -> Result { + let harness = ask_harness(¤t.harness)?; let merge_always = yes_or_no("Every Run a Merge run?", current.merge_always)?; let base_fix = merge_always && yes_or_no( @@ -102,6 +109,7 @@ pub fn ask( current.email.always, )? { return Ok(Answers { + harness, merge_always, base_fix, launch_pull, @@ -138,6 +146,7 @@ pub fn ask( false }; Ok(Answers { + harness, merge_always, base_fix, launch_pull, @@ -150,6 +159,87 @@ pub fn ask( }) } +/// Ask which Harness every Run's sessions run on, listing each with why +/// sessions can't run on it here, if they can't, and refusing that one; then +/// its Model and Effort, with those `current` sets for it as the defaults. +/// The Harness's default is the first one sessions can run on, so `claude` +/// when both can. A Model is checked with a test call, as a Run +/// checks it, and on a refusal the Model and Effort are asked again. With no +/// Harness to choose, nothing is asked, and the answer is `None`. +fn ask_harness(current: &harness::Settings) -> Result> { + let listed: Vec = Harness::ALL + .iter() + .map(|harness| match harness.unavailable() { + Some(why) => format!("{} ({why})", harness.name()), + None => harness.name().to_string(), + }) + .collect(); + let listed = listed.join(" or "); + let default = Harness::ALL + .into_iter() + .find(|harness| harness.unavailable().is_none()); + let Some(default) = default else { + say(&format!( + "Harness for every Run's sessions: {listed}. Sessions can run on neither here, so \ + the harness settings stay as they are; install claude, then rerun `thirdshift setup`." + )); + return Ok(None); + }; + let harness = loop { + let name = answer( + &format!("Harness for every Run's sessions, {listed}"), + Some(default.name()), + )?; + match Harness::named(&name) { + None => say(&format!("Choose {}.", harness::NAMES)), + Some(harness) => match harness.unavailable() { + None => break harness, + Some(Unavailable::NotSupportedYet) => say(&format!( + "{name} is not supported yet: Codex sessions come in a later thirdshift." + )), + Some(Unavailable::NotInstalled) => { + say(&format!("{name} is not installed: it isn't on PATH.")) + } + }, + } + }; + let current_settings = current.of(harness); + loop { + let model = ask_setting("Model", harness, current_settings.model.as_deref())?; + let effort = ask_setting("Effort", harness, current_settings.effort.as_deref())?; + let choice = Choice { + harness, + model, + effort, + chosen_by: ChosenBy::UserConfig, + }; + match choice.test_call() { + Ok(()) => { + return Ok(Some(( + harness, + ModelAndEffort { + model: choice.model, + effort: choice.effort, + }, + ))); + } + Err(error) => say(&format!("{error:#}")), + } + } +} + +/// Ask for the `setting`, the Model or the Effort, of `harness`, with +/// `current` as the default; `-` is none, the Harness's own default. +fn ask_setting(setting: &str, harness: Harness, current: Option<&str>) -> Result> { + let name = harness.name(); + let own = format!("{name}'s own default"); + let answer = match current { + Some(current) => answer(&format!("{setting} for {name}, - for {own}"), Some(current))?, + None => read(&format!("{setting} for {name} [{own}]: "))?, + }; + Ok(Some(answer).filter(|answer| !answer.is_empty() && answer != "-")) +} + /// Ask `question` until the answer is yes, no or nothing, which is `default`. fn yes_or_no(question: &str, default: bool) -> Result { let choices = if default { "[Y/n]" } else { "[y/N]" }; diff --git a/tests/setup.rs b/tests/setup.rs index d7d77d86c..c2c9e3106 100644 --- a/tests/setup.rs +++ b/tests/setup.rs @@ -4,7 +4,9 @@ //! existing one, it keeps its values and comments and adds the keys it lacks. //! From a terminal, it first asks the Setup questions on stderr, each with the //! current value as its default answer. Base fixes are asked about only -//! with every Run a Merge run. +//! with every Run a Merge run. The Harness question lists both Harnesses, +//! refusing one sessions can't run on here, and the Model and Effort are +//! asked for the Harness chosen, a Claude Model checked with a test call. mod support; @@ -73,7 +75,10 @@ fn with_no_terminal_and_no_user_config_setup_writes_the_defaults() { "[logs]", "[activity]", "[spec]", - "[pickup]" + "[pickup]", + "[harness]", + "[harness.claude]", + "[harness.codex]" ], "{text}" ); @@ -90,6 +95,12 @@ fn with_no_terminal_and_no_user_config_setup_writes_the_defaults() { assert_eq!(config["activity"]["quiet_skips"].as_bool(), Some(false)); assert_eq!(config["spec"]["parallel"].as_integer(), Some(3)); assert_eq!(config["pickup"]["limit"].as_integer(), Some(3)); + assert_eq!(config["harness"]["default"].as_str(), Some("claude")); + for harness in ["claude", "codex"] { + for key in ["model", "effort"] { + assert_eq!(config["harness"][harness][key].as_str(), Some(""), "{text}"); + } + } assert!( result.stderr.contains(".thirdshift/config.toml"), "stderr: {}", @@ -125,7 +136,12 @@ fn every_key_is_written_with_a_comment_giving_what_it_does_and_its_default() { "logs.dir", "activity.quiet_skips", "spec.parallel", - "pickup.limit" + "pickup.limit", + "harness.default", + "harness.claude.model", + "harness.claude.effort", + "harness.codex.model", + "harness.codex.effort" ], "{text}" ); @@ -224,12 +240,17 @@ fn setup_with_an_argument_is_an_argument_error_and_writes_nothing() { } /// Every key this version knows, as `section.key`, sorted. -const EVERY_KEY: [&str; 10] = [ +const EVERY_KEY: [&str; 15] = [ "activity.quiet_skips", "base.fix", "email.always", "email.from", "email.to", + "harness.claude.effort", + "harness.claude.model", + "harness.codex.effort", + "harness.codex.model", + "harness.default", "launch.pull", "logs.dir", "merge.always", @@ -340,7 +361,7 @@ fn setup_writes_pickup_limit_at_3_with_its_comment_and_keeps_one_already_there() let text = user_config(&scenario).unwrap(); let written = "\n[pickup]\nlimit = 3 # how many open issues labelled in-progress stop a \ Pickup run taking another; default 3\n"; - assert!(text.ends_with(written), "{text}"); + assert!(text.contains(&format!("{written}\n[harness]\n")), "{text}"); let mine = "[pickup]\nlimit = 7 # I review quickly\n"; scenario.user_config_is(mine); @@ -518,6 +539,9 @@ fn setup_never_replaces_an_existing_email_to() { // Setup from a terminal. +const HARNESS: &str = "Harness for every Run's sessions"; +const MODEL: &str = "Model for claude"; +const EFFORT: &str = "Effort for claude"; const MERGE: &str = "Merge run?"; const BASE_FIX: &str = "Every Run may start a Base fix when the Base branch's CI is red?"; const PULL: &str = "fast-forward"; @@ -551,6 +575,9 @@ fn setup_on_terminal( /// the default sender, then `rest`. fn notifications_on<'a>(rest: &[Keystrokes<'a>]) -> Vec> { let mut keystrokes = vec![ + (HARNESS, ""), + (MODEL, ""), + (EFFORT, ""), (MERGE, ""), (PULL, ""), (NOTIFY, "y"), @@ -574,7 +601,18 @@ fn on_a_terminal_pressing_enter_throughout_writes_what_setup_with_no_terminal_wr let scenario = Scenario::new(); scenario.git_email_is(None); - let result = setup_on_terminal(&scenario, &[], &[(MERGE, ""), (PULL, ""), (NOTIFY, "")]); + let result = setup_on_terminal( + &scenario, + &[], + &[ + (HARNESS, ""), + (MODEL, ""), + (EFFORT, ""), + (MERGE, ""), + (PULL, ""), + (NOTIFY, ""), + ], + ); assert_eq!(result.user_config, user_config(&unattended)); for prompt in [MERGE, PULL, NOTIFY] { @@ -595,6 +633,9 @@ fn on_a_terminal_the_answers_are_written_with_the_comments_on_each_key() { &scenario, &[("RESEND_API_KEY", KEY)], &[ + (HARNESS, ""), + (MODEL, ""), + (EFFORT, ""), (MERGE, "y"), (BASE_FIX, "y"), (PULL, "yes"), @@ -625,7 +666,12 @@ fn on_a_terminal_the_answers_are_written_with_the_comments_on_each_key() { "logs.dir", "activity.quiet_skips", "spec.parallel", - "pickup.limit" + "pickup.limit", + "harness.default", + "harness.claude.model", + "harness.claude.effort", + "harness.codex.model", + "harness.codex.effort" ], "{text}" ); @@ -656,7 +702,15 @@ fn on_a_terminal_with_merging_on_setup_asks_about_base_fixes_and_writes_the_answ let result = setup_on_terminal( &scenario, &[], - &[(MERGE, "y"), (BASE_FIX, answer), (PULL, ""), (NOTIFY, "")], + &[ + (HARNESS, ""), + (MODEL, ""), + (EFFORT, ""), + (MERGE, "y"), + (BASE_FIX, answer), + (PULL, ""), + (NOTIFY, ""), + ], ); let config = table(&result); @@ -676,8 +730,18 @@ fn on_a_terminal_with_merging_off_setup_asks_nothing_about_base_fixes_and_writes let scenario = Scenario::new(); scenario.git_email_is(None); - let result = - setup_on_terminal(&scenario, &[], &[(MERGE, answer), (PULL, ""), (NOTIFY, "")]); + let result = setup_on_terminal( + &scenario, + &[], + &[ + (HARNESS, ""), + (MODEL, ""), + (EFFORT, ""), + (MERGE, answer), + (PULL, ""), + (NOTIFY, ""), + ], + ); assert!( !result.stderr.contains(BASE_FIX), @@ -699,7 +763,15 @@ fn on_a_terminal_the_base_fix_question_defaults_to_the_user_configs_base_fix() { let result = setup_on_terminal( &scenario, &[], - &[(MERGE, ""), (BASE_FIX, ""), (PULL, ""), (NOTIFY, "")], + &[ + (HARNESS, ""), + (MODEL, ""), + (EFFORT, ""), + (MERGE, ""), + (BASE_FIX, ""), + (PULL, ""), + (NOTIFY, ""), + ], ); assert!( @@ -717,7 +789,18 @@ fn on_a_terminal_turning_merging_off_writes_base_fix_at_its_default() { scenario.git_email_is(None); scenario.user_config_is("[merge]\nalways = true\n\n[base]\nfix = true # mine\n"); - let result = setup_on_terminal(&scenario, &[], &[(MERGE, "n"), (PULL, ""), (NOTIFY, "")]); + let result = setup_on_terminal( + &scenario, + &[], + &[ + (HARNESS, ""), + (MODEL, ""), + (EFFORT, ""), + (MERGE, "n"), + (PULL, ""), + (NOTIFY, ""), + ], + ); assert!( !result.stderr.contains(BASE_FIX), @@ -740,6 +823,9 @@ fn on_a_terminal_the_address_defaults_to_the_suggested_github_email() { &scenario, &[], &[ + (HARNESS, ""), + (MODEL, ""), + (EFFORT, ""), (MERGE, ""), (PULL, ""), (NOTIFY, "y"), @@ -793,6 +879,17 @@ parallel = 5 [pickup] limit = 5 + +[harness] +default = \"claude\" + +[harness.claude] +model = \"opus\" +effort = \"high\" + +[harness.codex] +model = \"gpt-6.1-sol\" +effort = \"max\" "; scenario.user_config_is(mine); @@ -800,6 +897,9 @@ limit = 5 &scenario, &[("RESEND_API_KEY", KEY)], &[ + (HARNESS, ""), + (MODEL, ""), + (EFFORT, ""), (MERGE, ""), (BASE_FIX, ""), (PULL, ""), @@ -817,7 +917,13 @@ limit = 5 "terminal: {}", result.stderr ); - for current in ["[Y/n]", "mine@example.net", "ts@acme.dev"] { + for current in [ + "[Y/n]", + "mine@example.net", + "ts@acme.dev", + "[opus]", + "[high]", + ] { assert!( result.stderr.contains(current), "terminal: {}", @@ -846,6 +952,9 @@ always = false # quiet, please &scenario, &[], &[ + (HARNESS, ""), + (MODEL, ""), + (EFFORT, ""), (MERGE, "y"), (BASE_FIX, ""), (PULL, ""), @@ -881,6 +990,9 @@ fn on_a_terminal_an_address_without_an_at_is_asked_again() { &scenario, &[], &[ + (HARNESS, ""), + (MODEL, ""), + (EFFORT, ""), (MERGE, ""), (PULL, ""), (NOTIFY, "y"), @@ -911,7 +1023,14 @@ fn on_a_terminal_with_notifications_off_nothing_about_email_is_asked() { let result = setup_on_terminal( &scenario, &[("THIRDSHIFT_RESEND_URL", resend.url())], - &[(MERGE, ""), (PULL, ""), (NOTIFY, "n")], + &[ + (HARNESS, ""), + (MODEL, ""), + (EFFORT, ""), + (MERGE, ""), + (PULL, ""), + (NOTIFY, "n"), + ], ); assert_eq!(scenario.credentials().as_deref(), Some(saved)); @@ -1187,6 +1306,9 @@ fn on_a_terminal_accepting_the_test_email_sends_one_and_declining_sends_none() { ("THIRDSHIFT_RESEND_URL", resend.url()), ], &[ + (HARNESS, ""), + (MODEL, ""), + (EFFORT, ""), (MERGE, ""), (PULL, ""), (NOTIFY, "y"), @@ -1219,7 +1341,14 @@ fn ctrl_c_during_the_questions_writes_no_user_config() { let result = scenario.run_on_terminal( &["setup"], &[], - &[(MERGE, "y"), (BASE_FIX, ""), (PULL, CTRL_C)], + &[ + (HARNESS, ""), + (MODEL, ""), + (EFFORT, ""), + (MERGE, "y"), + (BASE_FIX, ""), + (PULL, CTRL_C), + ], ); assert_ne!(result.code, Some(0), "terminal: {}", result.stderr); @@ -1236,7 +1365,15 @@ fn ctrl_c_during_the_questions_leaves_an_existing_user_config_unchanged() { let result = scenario.run_on_terminal( &["setup"], &[], - &[(MERGE, "n"), (PULL, "y"), (NOTIFY, "y"), (TO, CTRL_C)], + &[ + (HARNESS, ""), + (MODEL, ""), + (EFFORT, ""), + (MERGE, "n"), + (PULL, "y"), + (NOTIFY, "y"), + (TO, CTRL_C), + ], ); assert_ne!(result.code, Some(0), "terminal: {}", result.stderr); @@ -1260,3 +1397,217 @@ fn on_a_terminal_setup_still_refuses_a_broken_user_config_before_asking() { ); assert_eq!(result.user_config.as_deref(), Some(broken)); } + +// The Harness, Model and Effort. + +/// What Claude says, and how it exits, when it refuses the Model it is +/// asked to run on. +const CLAUDE_REFUSES_THE_MODEL: &str = r#" +echo "There's an issue with the selected model (Opus 5.5). It may not exist or you may not have access to it." +exit 1 +"#; + +/// The answers that leave every setting but the Harness's as it is. +const NOTHING_ELSE: [Keystrokes; 3] = [(MERGE, ""), (PULL, ""), (NOTIFY, "")]; + +/// `PATH` with the scenario's fakes and the system's tools, and so no +/// `codex`, whatever is installed on the machine the tests run on. +fn fakes_only_path(scenario: &Scenario) -> String { + format!("{}:/usr/bin:/bin", scenario.path("bin").display()) +} + +/// Each call to `claude` that was the test call checking a Model, by its +/// arguments. +fn test_calls(scenario: &Scenario) -> Vec> { + scenario + .claude_calls() + .into_iter() + .map(|call| { + call["argv"] + .as_array() + .unwrap() + .iter() + .map(|arg| arg.as_str().unwrap().to_string()) + .collect() + }) + .collect() +} + +#[test] +fn on_a_terminal_the_harness_question_lists_both_harnesses_and_defaults_to_claude() { + let scenario = Scenario::new(); + scenario.git_email_is(None); + let path = fakes_only_path(&scenario); + let mut keystrokes = vec![(HARNESS, ""), (MODEL, ""), (EFFORT, "")]; + keystrokes.extend(NOTHING_ELSE); + + let result = setup_on_terminal(&scenario, &[("PATH", &path)], &keystrokes); + + assert!( + result.stderr.contains( + "Harness for every Run's sessions, claude or codex (not supported yet) [claude]: " + ), + "terminal: {}", + result.stderr + ); + assert!( + !result.stderr.contains("for codex"), + "terminal: {}", + result.stderr + ); + let config = table(&result); + assert_eq!(config["harness"]["default"].as_str(), Some("claude")); + for harness in ["claude", "codex"] { + for key in ["model", "effort"] { + assert_eq!(config["harness"][harness][key].as_str(), Some("")); + } + } + assert!(test_calls(&scenario).is_empty()); +} + +#[test] +fn on_a_terminal_a_harness_sessions_cant_run_on_is_refused_and_asked_again() { + let scenario = Scenario::new(); + scenario.git_email_is(None); + let path = fakes_only_path(&scenario); + let mut keystrokes = vec![ + (HARNESS, "codex"), + (HARNESS, "gemini"), + (HARNESS, "claude"), + (MODEL, ""), + (EFFORT, ""), + ]; + keystrokes.extend(NOTHING_ELSE); + + let result = setup_on_terminal(&scenario, &[("PATH", &path)], &keystrokes); + + for refusal in [ + "codex is not supported yet: Codex sessions come in a later thirdshift.", + "Choose claude or codex.", + ] { + assert!( + result.stderr.contains(refusal), + "terminal: {}", + result.stderr + ); + } + assert_eq!( + table(&result)["harness"]["default"].as_str(), + Some("claude") + ); +} + +#[test] +fn on_a_terminal_with_claude_not_installed_the_harness_is_left_as_it_was() { + let scenario = Scenario::new(); + scenario.git_email_is(None); + scenario.user_config_is("[harness.claude]\nmodel = \"opus\"\n"); + fs::remove_file(scenario.path("bin/claude")).unwrap(); + let path = fakes_only_path(&scenario); + + let result = setup_on_terminal(&scenario, &[("PATH", &path)], &NOTHING_ELSE); + + assert!( + result + .stderr + .contains("claude (not installed) or codex (not supported yet)"), + "terminal: {}", + result.stderr + ); + assert!( + !result.stderr.contains(MODEL), + "terminal: {}", + result.stderr + ); + let config = table(&result); + assert_eq!(config["harness"]["default"].as_str(), Some("claude")); + assert_eq!(config["harness"]["claude"]["model"].as_str(), Some("opus")); +} + +#[test] +fn on_a_terminal_the_model_and_effort_answers_are_written_after_a_test_call_with_them() { + let scenario = Scenario::new(); + scenario.git_email_is(None); + let mut keystrokes = vec![(HARNESS, ""), (MODEL, "claude-opus-5-5"), (EFFORT, "high")]; + keystrokes.extend(NOTHING_ELSE); + + let result = setup_on_terminal(&scenario, &[], &keystrokes); + + let config = table(&result); + assert_eq!( + config["harness"]["claude"]["model"].as_str(), + Some("claude-opus-5-5") + ); + assert_eq!(config["harness"]["claude"]["effort"].as_str(), Some("high")); + assert_eq!(config["harness"]["codex"]["model"].as_str(), Some("")); + let text = result.user_config.unwrap(); + assert!( + text.contains("model = \"claude-opus-5-5\" # the Model Claude Code's sessions run on"), + "{text}" + ); + assert_eq!( + test_calls(&scenario), + [["-p", "--model", "claude-opus-5-5", "--effort", "high"]] + ); +} + +#[test] +fn on_a_terminal_the_model_and_effort_default_to_the_current_ones_and_a_dash_clears_one() { + let scenario = Scenario::new(); + scenario.git_email_is(None); + scenario.user_config_is( + "[harness.claude]\nmodel = \"opus\" # mine\neffort = \"high\"\n\n\ + [harness.codex]\nmodel = \"gpt-6.1-sol\"\n", + ); + let mut keystrokes = vec![(HARNESS, ""), (MODEL, ""), (EFFORT, "-")]; + keystrokes.extend(NOTHING_ELSE); + + let result = setup_on_terminal(&scenario, &[], &keystrokes); + + for shown in [ + "Model for claude, - for claude's own default [opus]: ", + "Effort for claude, - for claude's own default [high]: ", + ] { + assert!(result.stderr.contains(shown), "terminal: {}", result.stderr); + } + let text = result.user_config.clone().unwrap(); + assert!(text.contains("model = \"opus\" # mine\n"), "{text}"); + let config = table(&result); + assert_eq!(config["harness"]["claude"]["effort"].as_str(), Some("")); + assert_eq!( + config["harness"]["codex"]["model"].as_str(), + Some("gpt-6.1-sol") + ); + assert_eq!(test_calls(&scenario), [["-p", "--model", "opus"]]); +} + +#[test] +fn on_a_terminal_a_model_claude_refuses_is_asked_again_with_claudes_error() { + let scenario = Scenario::new(); + scenario.git_email_is(None); + scenario.agent_does_in_session(1, CLAUDE_REFUSES_THE_MODEL); + let mut keystrokes = vec![ + (HARNESS, ""), + (MODEL, "Opus 5.5"), + (EFFORT, ""), + (MODEL, "opus"), + (EFFORT, ""), + ]; + keystrokes.extend(NOTHING_ELSE); + + let result = setup_on_terminal(&scenario, &[], &keystrokes); + + assert!( + result.stderr.contains( + "claude refused a test call on the Model Opus 5.5: There's an issue with the \ + selected model (Opus 5.5)." + ), + "terminal: {}", + result.stderr + ); + assert_eq!( + table(&result)["harness"]["claude"]["model"].as_str(), + Some("opus") + ); + assert_eq!(test_calls(&scenario).len(), 2); +} diff --git a/tests/setup_offer.rs b/tests/setup_offer.rs index 3b155178f..113054753 100644 --- a/tests/setup_offer.rs +++ b/tests/setup_offer.rs @@ -23,6 +23,9 @@ 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 HARNESS: &str = "Harness for every Run's sessions"; +const MODEL: &str = "Model for claude"; +const EFFORT: &str = "Effort for claude"; const MERGE: &str = "Merge run?"; const BASE_FIX: &str = "Every Run may start a Base fix when the Base branch's CI is red?"; const PULL: &str = "fast-forward"; @@ -79,6 +82,9 @@ fn accepting_and_choosing_merge_always_makes_that_run_a_merge_run() { &[], &[ (OFFER, ""), + (HARNESS, ""), + (MODEL, ""), + (EFFORT, ""), (MERGE, "y"), (BASE_FIX, ""), (PULL, ""), @@ -115,6 +121,9 @@ gh fake checks "$(git rev-parse HEAD)" '[{"name": "test", "conclusion": "success &[], &[ (OFFER, ""), + (HARNESS, ""), + (MODEL, ""), + (EFFORT, ""), (MERGE, "y"), (BASE_FIX, "y"), (PULL, ""), @@ -140,6 +149,9 @@ fn no_merge_in_the_command_wins_over_a_fresh_merge_always() { &[], &[ (OFFER, "y"), + (HARNESS, ""), + (MODEL, ""), + (EFFORT, ""), (MERGE, "y"), (BASE_FIX, ""), (PULL, ""), @@ -166,6 +178,9 @@ fn no_email_in_the_command_wins_over_fresh_run_notifications() { ], &[ (OFFER, "y"), + (HARNESS, ""), + (MODEL, ""), + (EFFORT, ""), (MERGE, ""), (PULL, ""), (NOTIFY, "y"), @@ -192,6 +207,9 @@ fn accepting_asks_for_the_key_saves_it_and_the_run_notification_goes_with_it() { &[("THIRDSHIFT_RESEND_URL", resend.url())], &[ (OFFER, "y"), + (HARNESS, ""), + (MODEL, ""), + (EFFORT, ""), (MERGE, ""), (PULL, ""), (NOTIFY, "y"), @@ -332,6 +350,9 @@ fn an_unwritable_user_config_is_a_warning_and_the_run_completes_on_the_defaults( &[], &[ (OFFER, "y"), + (HARNESS, ""), + (MODEL, ""), + (EFFORT, ""), (MERGE, "y"), (BASE_FIX, ""), (PULL, ""), @@ -385,7 +406,15 @@ fn ctrl_c_during_the_questions_writes_no_file_and_does_no_work() { let result = scenario.run_on_terminal( &[&scenario.issue_url(7)], &[], - &[(OFFER, "y"), (MERGE, "y"), (BASE_FIX, ""), (PULL, CTRL_C)], + &[ + (OFFER, "y"), + (HARNESS, ""), + (MODEL, ""), + (EFFORT, ""), + (MERGE, "y"), + (BASE_FIX, ""), + (PULL, CTRL_C), + ], ); assert_ne!(result.code, Some(0), "terminal: {}", result.stderr);