diff --git a/CHANGELOG.md b/CHANGELOG.md index 416fc547c..88de99bac 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,13 @@ Newest first. `Unreleased` is what is on `main` and not yet tagged. ## Unreleased +### The desktop app stops adding a banner to `.env` on every start + +`env::write` keeps the lines it did not write, and its own header comment is one of them, so each +start preserved the previous banner and appended another. A deployment started fifty times had fifty +copies of "Written by OpenBot Desktop" and fifty blank lines stacked above its settings. The banner +is now recognised and replaced rather than kept, and comments somebody else put in the file are left +alone exactly as before. ### Starting the desktop app again keeps the secrets the first start generated The shell generated a fresh set of secrets every time Start was pressed, including the diff --git a/desktop/src-tauri/src/env.rs b/desktop/src-tauri/src/env.rs index c3ad599e6..9032afcee 100644 --- a/desktop/src-tauri/src/env.rs +++ b/desktop/src-tauri/src/env.rs @@ -218,6 +218,12 @@ pub struct Model { pub openai_api_key: String, } +/// The line that separates what the shell owns from what it found. +/// +/// Named rather than written inline, because `write` has to recognise its own from a previous start +/// as well as put one down. +const BANNER: &str = "# Written by OpenBot Desktop. Anything else in this file is left alone."; + const MINTED: [&str; 6] = [ "AGENT_TOOL_TOKEN", "COMPUTER_TOKEN", @@ -273,6 +279,12 @@ pub fn write(path: &Path, owned: &BTreeMap) -> std::io::Result<( let mut out = String::new(); for line in existing.lines() { + // The shell's own banner is not one of the lines it did not write. Keeping it and then + // writing another one added a banner and a blank line to the file on every start, so a + // deployment restarted fifty times had fifty of them above its settings. + if line.trim() == BANNER { + continue; + } let key = line.split('=').next().unwrap_or("").trim(); if key.is_empty() || line.trim_start().starts_with('#') || !owned.contains_key(key) { out.push_str(line); @@ -280,10 +292,15 @@ pub fn write(path: &Path, owned: &BTreeMap) -> std::io::Result<( } } - if !out.is_empty() && !out.ends_with('\n') { - out.push('\n'); - } - out.push_str("\n# Written by OpenBot Desktop. Anything else in this file is left alone.\n"); + // The blank lines the removed banners left behind go with them, so the separator below is one + // blank line rather than one more on every start. + let kept = out.trim_end_matches('\n'); + let mut out = if kept.is_empty() { + String::new() + } else { + format!("{kept}\n") + }; + out.push_str(&format!("\n{BANNER}\n")); for (key, value) in owned { let value = carried.get(key).unwrap_or(value); out.push_str(&format!("{key}={value}\n")); @@ -327,6 +344,59 @@ mod tests { } } + #[test] + fn restarting_does_not_add_a_banner_to_the_file_every_time() { + // The banner is a comment, and the preserve pass keeps comments, so the file grew by one + // banner and one blank line on every start: fifty restarts, fifty banners. + let dir = std::env::temp_dir().join(format!("openbot-env-banner-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&dir); + std::fs::create_dir_all(&dir).unwrap(); + let path = dir.join(".env"); + + let mut owned = BTreeMap::new(); + owned.insert("SERVER_PORT".to_string(), "3000".to_string()); + owned.insert("KEY_ENCRYPTION_KEY".to_string(), "abc=".to_string()); + + for _ in 0..5 { + write(&path, &owned).unwrap(); + } + let text = std::fs::read_to_string(&path).unwrap(); + let _ = std::fs::remove_dir_all(&dir); + + assert_eq!(text.matches(BANNER).count(), 1); + // And the file is the same size on the fifth start as on the first. + assert_eq!(text.lines().count(), 4); + } + + #[test] + fn a_comment_somebody_else_wrote_is_still_kept() { + // Only the shell's own banner is dropped; the rule about leaving other lines alone stands. + let dir = std::env::temp_dir().join(format!("openbot-env-keep-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&dir); + std::fs::create_dir_all(&dir).unwrap(); + let path = dir.join(".env"); + std::fs::write( + &path, + "# our proxy needs this +HTTPS_PROXY=http://proxy:8080 +", + ) + .unwrap(); + + let mut owned = BTreeMap::new(); + owned.insert("SERVER_PORT".to_string(), "3000".to_string()); + write(&path, &owned).unwrap(); + write(&path, &owned).unwrap(); + + let text = std::fs::read_to_string(&path).unwrap(); + let _ = std::fs::remove_dir_all(&dir); + + assert!(text.contains("# our proxy needs this")); + assert!(text.contains("HTTPS_PROXY=http://proxy:8080")); + assert_eq!(text.matches("# our proxy needs this").count(), 1); + assert_eq!(text.matches(BANNER).count(), 1); + } + #[test] fn a_pasted_value_is_trimmed_the_way_the_model_key_beside_it_is() { // The setup screen enables its button on `value.trim() !== ""` and sends the untrimmed