From c43b14cef0f2f84bf4ce079cca3dc4fd3b6d6216 Mon Sep 17 00:00:00 2001 From: Jacob Stephens Date: Fri, 9 Oct 2026 09:31:40 -0400 Subject: [PATCH 1/2] Keep Setup Security settings after Harness sections (#597) --- README.md | 10 +++---- src/config.rs | 18 ++++++------ tests/setup.rs | 76 ++++++++++++++++++++++++++++++++++++++++++++++++-- 3 files changed, 88 insertions(+), 16 deletions(-) diff --git a/README.md b/README.md index 815ffa72..85e6a1db 100644 --- a/README.md +++ b/README.md @@ -253,11 +253,6 @@ parallel = 3 # how many Tickets a Spec run runs at once; default 3 limit = 3 # how many open issues labelled in-progress stop a Pickup run taking another; default 3 wait_minutes = 30 # minutes since the latest shaping event before an issue is Ready; 0 disables waiting; default 30 -[security] -fix = false # Security runs may fix reproduced findings; default false -review = false # Runs review their change for Security findings; default false -harness = "" # the Harness a Security run uses unless its command names one; default blank, for harness.default or Claude Code - [harness] default = "claude" # the Harness every Run's sessions run on, claude, codex, agy, grok, muse or opencode; default claude @@ -284,6 +279,11 @@ effort = "" # how hard that Model reasons; default blank, for Muse Code's own [harness.opencode] model = "" # the Model OpenCode's sessions run on, provider/model; default blank, for OpenCode's own effort = "" # the Model's variant, passed as #effort; default blank, for OpenCode's own + +[security] +fix = false # Security runs may fix reproduced findings; default false +review = false # Runs review their change for Security findings; default false +harness = "" # the Harness a Security run uses unless its command names one; default blank, for harness.default or Claude Code ``` 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: diff --git a/src/config.rs b/src/config.rs index 9815acbf..32ce61d5 100644 --- a/src/config.rs +++ b/src/config.rs @@ -704,11 +704,6 @@ parallel = 3 # how many Tickets a Spec run runs at once; default 3 limit = 3 # how many open issues labelled in-progress stop a Pickup run taking another; default 3 wait_minutes = 30 # minutes since the latest shaping event before an issue is Ready; 0 disables waiting; default 30 -[security] -fix = false # Security runs may fix reproduced findings; default false -review = false # Runs review their change for Security findings; default false -harness = "" # the Harness a Security run uses unless its command names one; default blank, for harness.default or Claude Code - [harness] default = "claude" # the Harness every Run's sessions run on, claude, codex, agy, grok, muse or opencode; default claude @@ -735,6 +730,11 @@ effort = "" # how hard that Model reasons; default blank, for Muse Code's own [harness.opencode] model = "" # the Model OpenCode's sessions run on, provider/model; default blank, for OpenCode's own effort = "" # the Model's variant, passed as #effort; default blank, for OpenCode's own + +[security] +fix = false # Security runs may fix reproduced findings; default false +review = false # Runs review their change for Security findings; default false +harness = "" # the Harness a Security run uses unless its command names one; default blank, for harness.default or Claude Code "#; /// The line `DEFAULTS` holds for `email.to`, which has no default. @@ -865,9 +865,6 @@ mod tests { "spec.parallel", "pickup.limit", "pickup.wait_minutes", - "security.fix", - "security.review", - "security.harness", "harness.default", "harness.claude.model", "harness.claude.effort", @@ -880,7 +877,10 @@ mod tests { "harness.muse.model", "harness.muse.effort", "harness.opencode.model", - "harness.opencode.effort" + "harness.opencode.effort", + "security.fix", + "security.review", + "security.harness", ] ); let commented_out: Vec<&str> = DEFAULTS diff --git a/tests/setup.rs b/tests/setup.rs index d65a2988..06450e36 100644 --- a/tests/setup.rs +++ b/tests/setup.rs @@ -51,14 +51,14 @@ fn with_no_terminal_and_no_user_config_setup_writes_the_defaults() { "[activity]", "[spec]", "[pickup]", - "[security]", "[harness]", "[harness.claude]", "[harness.codex]", "[harness.agy]", "[harness.grok]", "[harness.muse]", - "[harness.opencode]" + "[harness.opencode]", + "[security]" ], "{text}" ); @@ -91,6 +91,78 @@ fn with_no_terminal_and_no_user_config_setup_writes_the_defaults() { ); } +#[test] +fn upgrading_setup_puts_security_after_all_harness_settings() { + let scenario = Scenario::new(); + let original = "\ +[merge] +always = false +[base] +fix = false +[launch] +pull = false +[email] +always = false +from = 'onboarding@resend.dev' +[logs] +dir = '~/.thirdshift/logs' +[activity] +quiet_skips = false +[spec] +parallel = 3 +[pickup] +limit = 3 +wait_minutes = 30 +[harness] +default = 'codex' +[harness.claude] +model = '' +effort = '' +[harness.codex] +model = 'gpt-6.1-sol' # my Model +effort = 'high' # my Effort +"; + scenario.user_config_is(original); + + let result = scenario.run(&["setup"]); + + assert_eq!(result.code, Some(0), "stderr: {}", result.stderr); + let text = user_config(&scenario).unwrap(); + let sections: Vec<&str> = text.lines().filter(|line| line.starts_with('[')).collect(); + let harness = sections + .iter() + .position(|section| *section == "[harness]") + .unwrap(); + assert_eq!( + §ions[harness..], + [ + "[harness]", + "[harness.claude]", + "[harness.codex]", + "[harness.agy]", + "[harness.grok]", + "[harness.muse]", + "[harness.opencode]", + "[security]", + ], + "{text}" + ); + assert!( + text.contains("model = 'gpt-6.1-sol' # my Model\n"), + "{text}" + ); + assert!(text.contains("effort = 'high' # my Effort\n"), "{text}"); + let config: toml::Table = text.parse().unwrap(); + assert_eq!(config["harness"]["default"].as_str(), Some("codex")); + assert_eq!(config["security"]["fix"].as_bool(), Some(false)); + assert_eq!(config["security"]["review"].as_bool(), Some(false)); + assert_eq!(config["security"]["harness"].as_str(), Some("")); + + let again = scenario.run(&["setup"]); + assert_eq!(again.code, Some(0), "stderr: {}", again.stderr); + assert_eq!(user_config(&scenario).unwrap(), text); +} + #[test] fn completing_an_existing_file_replaces_it_atomically_keeps_permissions_and_then_avoids_replacement() { From 3178b4cb03dcbd5009a0ba3770f6e7414b67e550 Mon Sep 17 00:00:00 2001 From: Jacob Stephens Date: Fri, 9 Oct 2026 09:36:57 -0400 Subject: [PATCH 2/2] Move existing Security sections out of the Harness area (#597) --- README.md | 2 +- src/config.rs | 39 ++++++++++++++++++++++++++++++++++++++- src/setup.rs | 13 ++++--------- tests/setup.rs | 46 ++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 89 insertions(+), 11 deletions(-) diff --git a/README.md b/README.md index 85e6a1db..59d02bca 100644 --- a/README.md +++ b/README.md @@ -297,7 +297,7 @@ Every key holds its real value, so a Run reading it does exactly what it does wi 7. Run notifications? (`email.always`). If yes, the address (`email.to`), asked again until it has an `@`, then the sender (`email.from`). 8. 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`, `pickup.limit` and `security.harness` are not asked about. Setup adds a missing `security.harness` at its default, blank, with a comment explaining it; a configured Harness is kept. The answers are written like everything else below: in place, keeping your comments. Ctrl-C, SIGTERM or SIGHUP during the questions or a Model/Effort check exits `1` with `interrupted` and writes nothing: neither the User config nor the Credentials. Ordinary and hidden questions stop without another line of input, and hidden input restores the saved terminal settings, including echo. 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 . 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, asks nothing about a key, writes no Credentials, 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, SIGTERM or SIGHUP during the offer, the questions or a Model/Effort check exits `1` with `interrupted`, 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. diff --git a/src/config.rs b/src/config.rs index 32ce61d5..3f2c565e 100644 --- a/src/config.rs +++ b/src/config.rs @@ -227,6 +227,7 @@ impl UserConfig { /// A validated User config with every setting present, ready for Setup. /// Preparation and edits preserve the user's TOML spelling and comments. +/// A Security section between Harness sections moves after them. pub struct UserConfigDocument { document: DocumentMut, settings: UserConfig, @@ -271,7 +272,8 @@ impl UserConfigDocument { let settings = UserConfig::parse(&completed, path, home)?; // Completion appends missing implicit sections after the original // text. Reparse that text so subsequent edits keep their placement. - let document = completed.parse().context("can't parse the User config")?; + let mut document = completed.parse().context("can't parse the User config")?; + separate_security_from_harness(&mut document); Ok(Self { document, settings }) } @@ -312,6 +314,41 @@ impl UserConfigDocument { } } +/// Keep Security in its own area when completion or an earlier Setup put +/// it between Harness sections. Moving only its table preserves its keys, +/// comments and values, and every other section's relative order. +fn separate_security_from_harness(document: &mut DocumentMut) { + let Some(security_position) = document + .get("security") + .and_then(Item::as_table) + .filter(|table| !table.is_implicit() && !table.is_dotted()) + .and_then(toml_edit::Table::position) + else { + return; + }; + let Some(harness) = document.get("harness").and_then(Item::as_table) else { + return; + }; + let positions: Vec = std::iter::once(harness) + .chain(harness.iter().filter_map(|(_, item)| item.as_table())) + .filter(|table| !table.is_implicit() && !table.is_dotted()) + .filter_map(toml_edit::Table::position) + .collect(); + let (Some(first), Some(last)) = (positions.iter().min(), positions.iter().max()) else { + return; + }; + if security_position > *first && security_position < *last { + let mut end = *last; + while let Some(next) = following_section(document.as_table(), end) { + end = next.position().expect("a written section has a position"); + } + document["security"] + .as_table_mut() + .expect("Security has a section") + .set_position(Some(end + 1)); + } +} + /// Replace the file at `path` with `text` all at once, keeping its /// permissions, so a failed write leaves it as it was. pub fn replace(path: &Path, text: &str) -> std::io::Result<()> { diff --git a/src/setup.rs b/src/setup.rs index 96d61d4c..2607b71d 100644 --- a/src/setup.rs +++ b/src/setup.rs @@ -38,7 +38,8 @@ use questions::Answers; /// An existing User config is edited in place, once it parses as a Run would /// parse it: its comments and key order stay, as do the values Setup didn't /// ask about, and each key it lacks is added at its default, so Setup with no -/// terminal never resets a configured machine. A Resend API key the user +/// terminal never resets a configured machine. A Security section between +/// Harness sections moves after them. A Resend API key the user /// gave is saved in the Credentials once the User config is written; with no /// terminal, the Credentials are never read or written. Nothing is written /// until the last answer is in. A test email, if the user asked for one, goes @@ -96,10 +97,7 @@ fn run_setup(outside: &mut impl Outside, home: &Path, path: &Path) -> Result = text.lines().filter(|line| line.starts_with('[')).collect(); + assert_eq!( + sections.last(), + Some(&"[security] # separate area"), + "{text}" + ); + assert!( + text.contains( + "# my Security settings\n[security] # separate area\n\ + fix = true # allow fixes\nreview = true # review changes\n\ + harness = 'codex' # Security Harness\n" + ), + "{text}" + ); + let config: toml::Table = text.parse().unwrap(); + assert_eq!(config["security"]["fix"].as_bool(), Some(true)); + assert_eq!(config["security"]["review"].as_bool(), Some(true)); + assert_eq!(config["security"]["harness"].as_str(), Some("codex")); + assert_eq!( + config["harness"]["codex"]["model"].as_str(), + Some("gpt-6.1-sol") + ); + assert_eq!(config["harness"]["codex"]["effort"].as_str(), Some("high")); + + let again = scenario.run(&["setup"]); + assert_eq!(again.code, Some(0), "stderr: {}", again.stderr); + assert_eq!(user_config(&scenario).unwrap(), text); + } +} + #[test] fn completing_an_existing_file_replaces_it_atomically_keeps_permissions_and_then_avoids_replacement() {