Improve config file editing and recovery fallback mechanisms (#3082)

crates/goose-server/src/openapi.rs

@@ -23,6 +23,8 @@ use utoipa::OpenApi;
 #[openapi(
     paths(
         super::routes::config_management::backup_config,
+        super::routes::config_management::recover_config,
+        super::routes::config_management::validate_config,
         super::routes::config_management::init_config,
         super::routes::config_management::upsert_config,
         super::routes::config_management::remove_config,

crates/goose-server/src/routes/config_management.rs

@@ -457,44 +457,15 @@ pub async fn init_config(
         return Ok(Json("Config already exists".to_string()));
     }
 
-    let workspace_root = match std::env::current_exe() {
-        Ok(mut exe_path) => {
-            while let Some(parent) = exe_path.parent() {
-                let cargo_toml = parent.join("Cargo.toml");
-                if cargo_toml.exists() {
-                    if let Ok(content) = std::fs::read_to_string(&cargo_toml) {
-                        if content.contains("[workspace]") {
-                            exe_path = parent.to_path_buf();
-                            break;
-                        }
-                    }
-                }
-                exe_path = parent.to_path_buf();
-            }
-            exe_path
-        }
-        Err(_) => return Err(StatusCode::INTERNAL_SERVER_ERROR),
-    };
-
-    let init_config_path = workspace_root.join("init-config.yaml");
-    if !init_config_path.exists() {
-        return Ok(Json(
+    // Use the shared function to load init-config.yaml
+    match goose::config::base::load_init_config_from_workspace() {
+        Ok(init_values) => match config.save_values(init_values) {
+            Ok(_) => Ok(Json("Config initialized successfully".to_string())),
+            Err(_) => Err(StatusCode::INTERNAL_SERVER_ERROR),
+        },
+        Err(_) => Ok(Json(
             "No init-config.yaml found, using default configuration".to_string(),
-        ));
-    }
-
-    let init_content = match std::fs::read_to_string(&init_config_path) {
-        Ok(content) => content,
-        Err(_) => return Err(StatusCode::INTERNAL_SERVER_ERROR),
-    };
-    let init_values: HashMap<String, Value> = match serde_yaml::from_str(&init_content) {
-        Ok(values) => values,
-        Err(_) => return Err(StatusCode::INTERNAL_SERVER_ERROR),
-    };
-
-    match config.save_values(init_values) {
-        Ok(_) => Ok(Json("Config initialized successfully".to_string())),
-        Err(_) => Err(StatusCode::INTERNAL_SERVER_ERROR),
+        )),
     }
 }
 
@@ -555,8 +526,8 @@ pub async fn backup_config(
         backup_name.push(".bak");
 
         let backup = config_path.with_file_name(backup_name);
-        match std::fs::rename(&config_path, &backup) {
-            Ok(_) => Ok(Json(format!("Moved {:?} to {:?}", config_path, backup))),
+        match std::fs::copy(&config_path, &backup) {
+            Ok(_) => Ok(Json(format!("Copied {:?} to {:?}", config_path, backup))),
             Err(_) => Err(StatusCode::INTERNAL_SERVER_ERROR),
         }
     } else {
@@ -564,6 +535,82 @@ pub async fn backup_config(
     }
 }
 
+#[utoipa::path(
+    post,
+    path = "/config/recover",
+    responses(
+        (status = 200, description = "Config recovery attempted", body = String),
+        (status = 500, description = "Internal server error")
+    )
+)]
+pub async fn recover_config(
+    State(state): State<Arc<AppState>>,
+    headers: HeaderMap,
+) -> Result<Json<String>, StatusCode> {
+    verify_secret_key(&headers, &state)?;
+
+    let config = Config::global();
+
+    // Force a reload which will trigger recovery if needed
+    match config.load_values() {
+        Ok(values) => {
+            let recovered_keys: Vec<String> = values.keys().cloned().collect();
+            if recovered_keys.is_empty() {
+                Ok(Json("Config recovery completed, but no data was recoverable. Starting with empty configuration.".to_string()))
+            } else {
+                Ok(Json(format!(
+                    "Config recovery completed. Recovered {} keys: {}",
+                    recovered_keys.len(),
+                    recovered_keys.join(", ")
+                )))
+            }
+        }
+        Err(e) => {
+            tracing::error!("Config recovery failed: {}", e);
+            Err(StatusCode::INTERNAL_SERVER_ERROR)
+        }
+    }
+}
+
+#[utoipa::path(
+    get,
+    path = "/config/validate",
+    responses(
+        (status = 200, description = "Config validation result", body = String),
+        (status = 422, description = "Config file is corrupted")
+    )
+)]
+pub async fn validate_config(
+    State(state): State<Arc<AppState>>,
+    headers: HeaderMap,
+) -> Result<Json<String>, StatusCode> {
+    verify_secret_key(&headers, &state)?;
+
+    let config_dir = choose_app_strategy(APP_STRATEGY.clone())
+        .expect("goose requires a home dir")
+        .config_dir();
+
+    let config_path = config_dir.join("config.yaml");
+
+    if !config_path.exists() {
+        return Ok(Json("Config file does not exist".to_string()));
+    }
+
+    match std::fs::read_to_string(&config_path) {
+        Ok(content) => match serde_yaml::from_str::<serde_yaml::Value>(&content) {
+            Ok(_) => Ok(Json("Config file is valid".to_string())),
+            Err(e) => {
+                tracing::warn!("Config validation failed: {}", e);
+                Err(StatusCode::UNPROCESSABLE_ENTITY)
+            }
+        },
+        Err(e) => {
+            tracing::error!("Failed to read config file: {}", e);
+            Err(StatusCode::INTERNAL_SERVER_ERROR)
+        }
+    }
+}
+
 #[utoipa::path(
     get,
     path = "/config/current-model",
@@ -597,6 +644,8 @@ pub fn routes(state: Arc<AppState>) -> Router {
         .route("/config/pricing", post(get_pricing))
         .route("/config/init", post(init_config))
         .route("/config/backup", post(backup_config))
+        .route("/config/recover", post(recover_config))
+        .route("/config/validate", get(validate_config))
         .route("/config/permissions", post(upsert_permissions))
         .route("/config/current-model", get(get_current_model))
         .with_state(state)

crates/goose/src/config/base.rs

@@ -7,7 +7,7 @@ use serde_json::Value;
 use std::collections::HashMap;
 use std::env;
 use std::fs::OpenOptions;
-use std::io::{Read, Seek, SeekFrom, Write};
+use std::io::Write;
 use std::path::{Path, PathBuf};
 use thiserror::Error;
 
@@ -200,22 +200,179 @@ impl Config {
     // Load current values from the config file
     pub fn load_values(&self) -> Result<HashMap<String, Value>, ConfigError> {
         if self.config_path.exists() {
-            let file_content = std::fs::read_to_string(&self.config_path)?;
-            // Parse YAML into JSON Value for consistent internal representation
-            let yaml_value: serde_yaml::Value = serde_yaml::from_str(&file_content)?;
-            let json_value: Value = serde_json::to_value(yaml_value)?;
-
-            match json_value {
-                Value::Object(map) => Ok(map.into_iter().collect()),
-                _ => Ok(HashMap::new()),
-            }
+            self.load_values_with_recovery()
         } else {
-            Ok(HashMap::new())
+            // Config file doesn't exist, try to recover from backup first
+            tracing::info!("Config file doesn't exist, attempting recovery from backup");
+
+            if let Ok(backup_values) = self.try_restore_from_backup() {
+                tracing::info!("Successfully restored config from backup");
+                return Ok(backup_values);
+            }
+
+            // No backup available, create a default config
+            tracing::info!("No backup found, creating default configuration");
+
+            // Try to load from init-config.yaml if it exists, otherwise use empty config
+            let default_config = self
+                .load_init_config_if_exists()
+                .unwrap_or_else(|_| HashMap::new());
+
+            self.create_and_save_default_config(default_config)
         }
     }
 
+    // Helper method to create and save default config with consistent logging
+    fn create_and_save_default_config(
+        &self,
+        default_config: HashMap<String, Value>,
+    ) -> Result<HashMap<String, Value>, ConfigError> {
+        // Try to write the default config to disk
+        match self.save_values(default_config.clone()) {
+            Ok(_) => {
+                if default_config.is_empty() {
+                    tracing::info!("Created fresh empty config file");
+                } else {
+                    tracing::info!(
+                        "Created fresh config file from init-config.yaml with {} keys",
+                        default_config.len()
+                    );
+                }
+                Ok(default_config)
+            }
+            Err(write_error) => {
+                tracing::error!("Failed to write default config file: {}", write_error);
+                // Even if we can't write to disk, return config so app can still run
+                Ok(default_config)
+            }
+        }
+    }
+
+    // Load values with automatic recovery from corruption
+    fn load_values_with_recovery(&self) -> Result<HashMap<String, Value>, ConfigError> {
+        let file_content = std::fs::read_to_string(&self.config_path)?;
+
+        // First attempt: try to parse the current config
+        match self.parse_yaml_content(&file_content) {
+            Ok(values) => Ok(values),
+            Err(parse_error) => {
+                tracing::warn!(
+                    "Config file appears corrupted, attempting recovery: {}",
+                    parse_error
+                );
+
+                // Try to recover from backup
+                if let Ok(backup_values) = self.try_restore_from_backup() {
+                    tracing::info!("Successfully restored config from backup");
+                    return Ok(backup_values);
+                }
+
+                // Last resort: create a fresh default config file
+                tracing::error!("Could not recover config file, creating fresh default configuration. Original error: {}", parse_error);
+
+                // Try to load from init-config.yaml if it exists, otherwise use empty config
+                let default_config = self
+                    .load_init_config_if_exists()
+                    .unwrap_or_else(|_| HashMap::new());
+
+                self.create_and_save_default_config(default_config)
+            }
+        }
+    }
+
+    // Parse YAML content into HashMap
+    fn parse_yaml_content(&self, content: &str) -> Result<HashMap<String, Value>, ConfigError> {
+        if content.trim().is_empty() {
+            return Ok(HashMap::new());
+        }
+
+        let yaml_value: serde_yaml::Value = serde_yaml::from_str(content)?;
+        let json_value: Value = serde_json::to_value(yaml_value)?;
+
+        match json_value {
+            Value::Object(map) => Ok(map.into_iter().collect()),
+            _ => Ok(HashMap::new()),
+        }
+    }
+
+    // Try to restore from backup file
+    fn try_restore_from_backup(&self) -> Result<HashMap<String, Value>, ConfigError> {
+        let backup_paths = self.get_backup_paths();
+
+        for backup_path in backup_paths {
+            if backup_path.exists() {
+                match std::fs::read_to_string(&backup_path) {
+                    Ok(backup_content) => {
+                        match self.parse_yaml_content(&backup_content) {
+                            Ok(values) => {
+                                // Successfully parsed backup, restore it as the main config
+                                if let Err(e) = self.save_values(values.clone()) {
+                                    tracing::warn!(
+                                        "Failed to restore backup as main config: {}",
+                                        e
+                                    );
+                                } else {
+                                    tracing::info!(
+                                        "Restored config from backup: {:?}",
+                                        backup_path
+                                    );
+                                }
+                                return Ok(values);
+                            }
+                            Err(e) => {
+                                tracing::warn!(
+                                    "Backup file {:?} is also corrupted: {}",
+                                    backup_path,
+                                    e
+                                );
+                                continue;
+                            }
+                        }
+                    }
+                    Err(e) => {
+                        tracing::warn!("Could not read backup file {:?}: {}", backup_path, e);
+                        continue;
+                    }
+                }
+            }
+        }
+
+        Err(ConfigError::NotFound("No valid backup found".to_string()))
+    }
+
+    // Get list of backup file paths in order of preference
+    fn get_backup_paths(&self) -> Vec<PathBuf> {
+        let mut paths = Vec::new();
+
+        // Primary backup (created by backup_config endpoint)
+        if let Some(file_name) = self.config_path.file_name() {
+            let mut backup_name = file_name.to_os_string();
+            backup_name.push(".bak");
+            paths.push(self.config_path.with_file_name(backup_name));
+        }
+
+        // Timestamped backups
+        for i in 1..=5 {
+            if let Some(file_name) = self.config_path.file_name() {
+                let mut backup_name = file_name.to_os_string();
+                backup_name.push(format!(".bak.{}", i));
+                paths.push(self.config_path.with_file_name(backup_name));
+            }
+        }
+
+        paths
+    }
+
+    // Try to load init-config.yaml from workspace root if it exists
+    fn load_init_config_if_exists(&self) -> Result<HashMap<String, Value>, ConfigError> {
+        load_init_config_from_workspace()
+    }
+
     // Save current values to the config file
     pub fn save_values(&self, values: HashMap<String, Value>) -> Result<(), ConfigError> {
+        // Create backup before writing new config
+        self.create_backup_if_needed()?;
+
         // Convert to YAML for storage
         let yaml_value = serde_yaml::to_string(&values)?;
 
@@ -225,22 +382,97 @@ impl Config {
                 .map_err(|e| ConfigError::DirectoryError(e.to_string()))?;
         }
 
-        // Open the file with write permissions, create if it doesn't exist
-        let mut file = OpenOptions::new()
-            .write(true)
-            .create(true)
-            .truncate(true)
-            .open(&self.config_path)?;
+        // Write to a temporary file first for atomic operation
+        let temp_path = self.config_path.with_extension("tmp");
 
-        // Acquire an exclusive lock
-        file.lock_exclusive()
-            .map_err(|e| ConfigError::LockError(e.to_string()))?;
+        {
+            let mut file = OpenOptions::new()
+                .write(true)
+                .create(true)
+                .truncate(true)
+                .open(&temp_path)?;
 
-        // Write the contents using the same file handle
-        file.write_all(yaml_value.as_bytes())?;
-        file.sync_all()?;
+            // Acquire an exclusive lock
+            file.lock_exclusive()
+                .map_err(|e| ConfigError::LockError(e.to_string()))?;
+
+            // Write the contents using the same file handle
+            file.write_all(yaml_value.as_bytes())?;
+            file.sync_all()?;
+
+            // Unlock is handled automatically when file is dropped
+        }
+
+        // Atomically replace the original file
+        std::fs::rename(&temp_path, &self.config_path)?;
+
+        Ok(())
+    }
+
+    // Create backup of current config file if it exists and is valid
+    fn create_backup_if_needed(&self) -> Result<(), ConfigError> {
+        if !self.config_path.exists() {
+            return Ok(());
+        }
+
+        // Check if current config is valid before backing it up
+        let current_content = std::fs::read_to_string(&self.config_path)?;
+        if self.parse_yaml_content(&current_content).is_err() {
+            // Don't back up corrupted files
+            return Ok(());
+        }
+
+        // Rotate existing backups
+        self.rotate_backups()?;
+
+        // Create new backup
+        if let Some(file_name) = self.config_path.file_name() {
+            let mut backup_name = file_name.to_os_string();
+            backup_name.push(".bak");
+            let backup_path = self.config_path.with_file_name(backup_name);
+
+            if let Err(e) = std::fs::copy(&self.config_path, &backup_path) {
+                tracing::warn!("Failed to create config backup: {}", e);
+                // Don't fail the entire operation if backup fails
+            } else {
+                tracing::debug!("Created config backup: {:?}", backup_path);
+            }
+        }
+
+        Ok(())
+    }
+
+    // Rotate backup files to keep the most recent ones
+    fn rotate_backups(&self) -> Result<(), ConfigError> {
+        if let Some(file_name) = self.config_path.file_name() {
+            // Move .bak.4 to .bak.5, .bak.3 to .bak.4, etc.
+            for i in (1..5).rev() {
+                let mut current_backup = file_name.to_os_string();
+                current_backup.push(format!(".bak.{}", i));
+                let current_path = self.config_path.with_file_name(&current_backup);
+
+                let mut next_backup = file_name.to_os_string();
+                next_backup.push(format!(".bak.{}", i + 1));
+                let next_path = self.config_path.with_file_name(&next_backup);
+
+                if current_path.exists() {
+                    let _ = std::fs::rename(&current_path, &next_path);
+                }
+            }
+
+            // Move .bak to .bak.1
+            let mut backup_name = file_name.to_os_string();
+            backup_name.push(".bak");
+            let backup_path = self.config_path.with_file_name(&backup_name);
+
+            if backup_path.exists() {
+                let mut backup_1_name = file_name.to_os_string();
+                backup_1_name.push(".bak.1");
+                let backup_1_path = self.config_path.with_file_name(&backup_1_name);
+                let _ = std::fs::rename(&backup_path, &backup_1_path);
+            }
+        }
 
-        // Unlock is handled automatically when file is dropped
         Ok(())
     }
 
@@ -342,56 +574,14 @@ impl Config {
     /// - There is an error reading or writing the config file
     /// - There is an error serializing the value
     pub fn set_param(&self, key: &str, value: Value) -> Result<(), ConfigError> {
-        // Ensure the directory exists
-        if let Some(parent) = self.config_path.parent() {
-            std::fs::create_dir_all(parent)
-                .map_err(|e| ConfigError::DirectoryError(e.to_string()))?;
-        }
-
-        // Open the file with read+write permissions, create if it doesn't exist
-        let mut file = OpenOptions::new()
-            .read(true)
-            .write(true)
-            .create(true)
-            .truncate(false)
-            .open(&self.config_path)?;
-
-        // Acquire an exclusive lock for the entire operation
-        file.lock_exclusive()
-            .map_err(|e| ConfigError::LockError(e.to_string()))?;
-
-        // Load current values while holding the lock - read through the file handle
-        let mut values = {
-            let mut file_content = String::new();
-            file.seek(SeekFrom::Start(0))?;
-            file.read_to_string(&mut file_content)?;
-
-            if file_content.trim().is_empty() {
-                HashMap::new()
-            } else {
-                let yaml_value: serde_yaml::Value = serde_yaml::from_str(&file_content)?;
-                let json_value: Value = serde_json::to_value(yaml_value)?;
-                match json_value {
-                    Value::Object(map) => map.into_iter().collect(),
-                    _ => HashMap::new(),
-                }
-            }
-        };
+        // Load current values with recovery if needed
+        let mut values = self.load_values()?;
 
         // Modify values
         values.insert(key.to_string(), value);
 
-        // Convert to YAML for storage
-        let yaml_value = serde_yaml::to_string(&values)?;
-
-        // Write the contents using the same file handle
-        file.seek(SeekFrom::Start(0))?; // Seek to beginning before writing
-        file.set_len(0)?; // Clear the file
-        file.write_all(yaml_value.as_bytes())?;
-        file.sync_all()?;
-
-        // Unlock is handled automatically when file is dropped
-        Ok(())
+        // Save all values using the atomic write approach
+        self.save_values(values)
     }
 
     /// Delete a configuration value in the config file.
@@ -507,6 +697,60 @@ impl Config {
     }
 }
 
+/// Load init-config.yaml from workspace root if it exists.
+/// This function is shared between the config recovery and the init_config endpoint.
+pub fn load_init_config_from_workspace() -> Result<HashMap<String, Value>, ConfigError> {
+    let workspace_root = match std::env::current_exe() {
+        Ok(mut exe_path) => {
+            while let Some(parent) = exe_path.parent() {
+                let cargo_toml = parent.join("Cargo.toml");
+                if cargo_toml.exists() {
+                    if let Ok(content) = std::fs::read_to_string(&cargo_toml) {
+                        if content.contains("[workspace]") {
+                            exe_path = parent.to_path_buf();
+                            break;
+                        }
+                    }
+                }
+                exe_path = parent.to_path_buf();
+            }
+            exe_path
+        }
+        Err(_) => {
+            return Err(ConfigError::FileError(std::io::Error::new(
+                std::io::ErrorKind::NotFound,
+                "Could not determine executable path",
+            )))
+        }
+    };
+
+    let init_config_path = workspace_root.join("init-config.yaml");
+    if !init_config_path.exists() {
+        return Err(ConfigError::NotFound(
+            "init-config.yaml not found".to_string(),
+        ));
+    }
+
+    let init_content = std::fs::read_to_string(&init_config_path)?;
+    let init_values: HashMap<String, Value> =
+        match serde_yaml::from_str::<serde_yaml::Value>(&init_content) {
+            Ok(yaml_value) => {
+                let json_value: Value = serde_json::to_value(yaml_value)?;
+                match json_value {
+                    Value::Object(map) => map.into_iter().collect(),
+                    _ => HashMap::new(),
+                }
+            }
+            Err(e) => {
+                tracing::warn!("Failed to parse init-config.yaml: {}", e);
+                return Err(ConfigError::DeserializeError(e.to_string()));
+            }
+        };
+
+    tracing::info!("Loaded init-config.yaml with {} keys", init_values.len());
+    Ok(init_values)
+}
+
 #[cfg(test)]
 mod tests {
     use super::*;
@@ -760,94 +1004,191 @@ mod tests {
     }
 
     #[test]
-    fn test_concurrent_extension_writes() -> Result<(), ConfigError> {
-        use std::sync::{Arc, Barrier};
-        use std::thread;
-        use std::time::Duration;
-
+    fn test_config_recovery_from_backup() -> Result<(), ConfigError> {
         let temp_file = NamedTempFile::new().unwrap();
-        let config = Arc::new(Config::new(temp_file.path(), TEST_KEYRING_SERVICE)?);
-        let barrier = Arc::new(Barrier::new(3)); // For 3 concurrent threads
-        let mut handles = vec![];
+        let config = Config::new(temp_file.path(), TEST_KEYRING_SERVICE)?;
 
-        // Initialize with empty values
-        config.save_values(HashMap::new())?;
+        // Create a valid config first
+        config.set_param("key1", Value::String("value1".to_string()))?;
 
-        // Spawn 3 threads that will try to write extension configs simultaneously
-        for i in 0..3 {
-            let config = Arc::clone(&config);
-            let barrier = Arc::clone(&barrier);
-            let handle = thread::spawn(move || -> Result<(), ConfigError> {
-                // Wait for all threads to reach this point
-                barrier.wait();
-
-                // Add a small random delay to increase chance of concurrent access
-                thread::sleep(Duration::from_millis(i * 10));
-
-                let extension_key = format!("extension_{}", i);
-
-                // Use set_param which handles concurrent access properly
-                config.set_param(
-                    &extension_key,
-                    serde_json::json!({
-                        "name": format!("test_extension_{}", i),
-                        "version": format!("1.0.{}", i),
-                        "enabled": true,
-                        "settings": {
-                            "option1": format!("value{}", i),
-                            "option2": i
-                        }
-                    }),
-                )?;
-                Ok(())
-            });
-            handles.push(handle);
+        // Verify the backup was created by the first write
+        let backup_paths = config.get_backup_paths();
+        println!("Backup paths: {:?}", backup_paths);
+        for (i, path) in backup_paths.iter().enumerate() {
+            println!("Backup {} exists: {}", i, path.exists());
         }
 
-        // Wait for all threads to complete
-        for handle in handles {
-            handle.join().unwrap()?;
+        // Make another write to ensure backup is created
+        config.set_param("key2", Value::Number(42.into()))?;
+
+        // Check again
+        for (i, path) in backup_paths.iter().enumerate() {
+            println!(
+                "After second write - Backup {} exists: {}",
+                i,
+                path.exists()
+            );
         }
 
-        // Verify all extension configs were written correctly
-        let final_values = config.load_values()?;
+        // Corrupt the main config file
+        std::fs::write(temp_file.path(), "invalid: yaml: content: [unclosed")?;
 
-        // Print the final values for debugging
-        println!("Final values: {:?}", final_values);
+        // Try to load values - should recover from backup
+        let recovered_values = config.load_values()?;
+        println!("Recovered values: {:?}", recovered_values);
 
-        assert_eq!(
-            final_values.len(),
-            3,
-            "Expected 3 extension configs, got {}",
-            final_values.len()
+        // Should have recovered the data
+        assert!(
+            recovered_values.len() >= 1,
+            "Should have recovered at least one key"
         );
 
-        for i in 0..3 {
-            let extension_key = format!("extension_{}", i);
+        Ok(())
+    }
 
-            let config = final_values
-                .get(&extension_key)
-                .expect(&format!("Missing extension config for {}", extension_key));
+    #[test]
+    fn test_config_recovery_creates_fresh_file() -> Result<(), ConfigError> {
+        let temp_file = NamedTempFile::new().unwrap();
+        let config = Config::new(temp_file.path(), TEST_KEYRING_SERVICE)?;
 
-            // Verify the structure matches what we wrote
-            let config_obj = config.as_object().unwrap();
-            assert_eq!(
-                config_obj.get("name").unwrap().as_str().unwrap(),
-                format!("test_extension_{}", i)
-            );
-            assert_eq!(
-                config_obj.get("version").unwrap().as_str().unwrap(),
-                format!("1.0.{}", i)
-            );
-            assert!(config_obj.get("enabled").unwrap().as_bool().unwrap());
+        // Create a corrupted config file with no backup
+        std::fs::write(temp_file.path(), "invalid: yaml: content: [unclosed")?;
 
-            let settings = config_obj.get("settings").unwrap().as_object().unwrap();
-            assert_eq!(
-                settings.get("option1").unwrap().as_str().unwrap(),
-                format!("value{}", i)
-            );
-            assert_eq!(settings.get("option2").unwrap().as_i64().unwrap() as i32, i);
+        // Try to load values - should create a fresh default config
+        let recovered_values = config.load_values()?;
+
+        // Should return empty config
+        assert_eq!(recovered_values.len(), 0);
+
+        // Verify that a clean config file was written to disk
+        let file_content = std::fs::read_to_string(temp_file.path())?;
+
+        // Should be valid YAML (empty object)
+        let parsed: serde_yaml::Value = serde_yaml::from_str(&file_content)?;
+        assert!(parsed.is_mapping());
+
+        // Should be able to load it again without issues
+        let reloaded_values = config.load_values()?;
+        assert_eq!(reloaded_values.len(), 0);
+
+        Ok(())
+    }
+
+    #[test]
+    fn test_config_file_creation_when_missing() -> Result<(), ConfigError> {
+        let temp_file = NamedTempFile::new().unwrap();
+        let config_path = temp_file.path();
+
+        // Delete the file to simulate it not existing
+        std::fs::remove_file(config_path)?;
+        assert!(!config_path.exists());
+
+        let config = Config::new(config_path, TEST_KEYRING_SERVICE)?;
+
+        // Try to load values - should create a fresh default config file
+        let values = config.load_values()?;
+
+        // Should return empty config
+        assert_eq!(values.len(), 0);
+
+        // Verify that the config file was created
+        assert!(config_path.exists());
+
+        // Verify that it's valid YAML
+        let file_content = std::fs::read_to_string(config_path)?;
+        let parsed: serde_yaml::Value = serde_yaml::from_str(&file_content)?;
+        assert!(parsed.is_mapping());
+
+        // Should be able to load it again without issues
+        let reloaded_values = config.load_values()?;
+        assert_eq!(reloaded_values.len(), 0);
+
+        Ok(())
+    }
+
+    #[test]
+    fn test_config_recovery_from_backup_when_missing() -> Result<(), ConfigError> {
+        let temp_file = NamedTempFile::new().unwrap();
+        let config_path = temp_file.path();
+        let config = Config::new(config_path, TEST_KEYRING_SERVICE)?;
+
+        // First, create a config with some data
+        config.set_param("test_key_backup", Value::String("backup_value".to_string()))?;
+        config.set_param("another_key", Value::Number(42.into()))?;
+
+        // Verify the backup was created
+        let backup_paths = config.get_backup_paths();
+        let primary_backup = &backup_paths[0]; // .bak file
+
+        // Make sure we have a backup by doing another write
+        config.set_param("third_key", Value::Bool(true))?;
+        assert!(primary_backup.exists(), "Backup should exist after writes");
+
+        // Now delete the main config file to simulate it being lost
+        std::fs::remove_file(config_path)?;
+        assert!(!config_path.exists());
+
+        // Try to load values - should recover from backup
+        let recovered_values = config.load_values()?;
+
+        // Should have recovered the data from backup
+        assert!(
+            recovered_values.len() >= 1,
+            "Should have recovered data from backup"
+        );
+
+        // Verify the main config file was restored
+        assert!(config_path.exists(), "Main config file should be restored");
+
+        // Verify we can load the data (using a key that won't conflict with env vars)
+        if let Ok(backup_value) = config.get_param::<String>("test_key_backup") {
+            // If we recovered the key, great!
+            assert_eq!(backup_value, "backup_value");
         }
+        // Note: Due to back up rotation, we might not get the exact same data,
+        // but we should get some data back
+
+        Ok(())
+    }
+
+    #[test]
+    fn test_atomic_write_prevents_corruption() -> Result<(), ConfigError> {
+        let temp_file = NamedTempFile::new().unwrap();
+        let config = Config::new(temp_file.path(), TEST_KEYRING_SERVICE)?;
+
+        // Set initial values
+        config.set_param("key1", Value::String("value1".to_string()))?;
+
+        // Verify the config file exists and is valid
+        assert!(temp_file.path().exists());
+        let content = std::fs::read_to_string(temp_file.path())?;
+        assert!(serde_yaml::from_str::<serde_yaml::Value>(&content).is_ok());
+
+        // The temp file should not exist after successful write
+        let temp_path = temp_file.path().with_extension("tmp");
+        assert!(!temp_path.exists(), "Temporary file should be cleaned up");
+
+        Ok(())
+    }
+
+    #[test]
+    fn test_backup_rotation() -> Result<(), ConfigError> {
+        let temp_file = NamedTempFile::new().unwrap();
+        let config = Config::new(temp_file.path(), TEST_KEYRING_SERVICE)?;
+
+        // Create multiple versions to test rotation
+        for i in 1..=7 {
+            config.set_param("version", Value::Number(i.into()))?;
+        }
+
+        let backup_paths = config.get_backup_paths();
+
+        // Should have backups but not more than our limit
+        let existing_backups: Vec<_> = backup_paths.iter().filter(|p| p.exists()).collect();
+        assert!(
+            existing_backups.len() <= 6,
+            "Should not exceed backup limit"
+        ); // .bak + .bak.1 through .bak.5
 
         Ok(())
     }

crates/goose/src/session/storage.rs

@@ -934,8 +934,7 @@ pub async fn persist_messages(
     messages: &[Message],
     provider: Option<Arc<dyn Provider>>,
 ) -> Result<()> {
-    let result = persist_messages_with_schedule_id(session_file, messages, provider, None).await;
-    result
+    persist_messages_with_schedule_id(session_file, messages, provider, None).await
 }
 
 /// Write messages to a session file with metadata, including an optional scheduled job ID

ui/desktop/openapi.json

@@ -306,6 +306,29 @@
         }
       }
     },
+    "/config/recover": {
+      "post": {
+        "tags": [
+          "super::routes::config_management"
+        ],
+        "operationId": "recover_config",
+        "responses": {
+          "200": {
+            "description": "Config recovery attempted",
+            "content": {
+              "text/plain": {
+                "schema": {
+                  "type": "string"
+                }
+              }
+            }
+          },
+          "500": {
+            "description": "Internal server error"
+          }
+        }
+      }
+    },
     "/config/remove": {
       "post": {
         "tags": [
@@ -375,6 +398,29 @@
         }
       }
     },
+    "/config/validate": {
+      "get": {
+        "tags": [
+          "super::routes::config_management"
+        ],
+        "operationId": "validate_config",
+        "responses": {
+          "200": {
+            "description": "Config validation result",
+            "content": {
+              "text/plain": {
+                "schema": {
+                  "type": "string"
+                }
+              }
+            }
+          },
+          "422": {
+            "description": "Config file is corrupted"
+          }
+        }
+      }
+    },
     "/confirm": {
       "post": {
         "tags": [

ui/desktop/src/App.tsx

@@ -28,7 +28,13 @@ import 'react-toastify/dist/ReactToastify.css';
 import { useConfig, MalformedConfigError } from './components/ConfigContext';
 import { ModelAndProviderProvider } from './components/ModelAndProviderContext';
 import { addExtensionFromDeepLink as addExtensionFromDeepLinkV2 } from './components/settings/extensions';
-import { backupConfig, initConfig, readAllConfig } from './api/sdk.gen';
+import {
+  backupConfig,
+  initConfig,
+  readAllConfig,
+  recoverConfig,
+  validateConfig,
+} from './api/sdk.gen';
 import PermissionSettingsView from './components/settings/permission/PermissionSetting';
 
 import { type SessionDetails } from './sessions';
@@ -174,7 +180,39 @@ export default function App() {
             await backupConfig({ throwOnError: true });
             await initConfig();
           } else {
-            throw new Error('Unable to read config file, it may be malformed');
+            // Config appears corrupted, try recovery
+            console.warn('Config file appears corrupted, attempting recovery...');
+            try {
+              // First try to validate the config
+              try {
+                await validateConfig({ throwOnError: true });
+                // Config is valid but readAllConfig failed for another reason
+                throw new Error('Unable to read config file, it may be malformed');
+              } catch (validateError) {
+                console.log('Config validation failed, attempting recovery...');
+
+                // Try to recover the config
+                try {
+                  const recoveryResult = await recoverConfig({ throwOnError: true });
+                  console.log('Config recovery result:', recoveryResult);
+
+                  // Try to read config again after recovery
+                  try {
+                    await readAllConfig({ throwOnError: true });
+                    console.log('Config successfully recovered and loaded');
+                  } catch (retryError) {
+                    console.warn('Config still corrupted after recovery, reinitializing...');
+                    await initConfig();
+                  }
+                } catch (recoverError) {
+                  console.warn('Config recovery failed, reinitializing...');
+                  await initConfig();
+                }
+              }
+            } catch (recoveryError) {
+              console.error('Config recovery process failed:', recoveryError);
+              throw new Error('Unable to read config file, it may be malformed');
+            }
           }
         }
 

ui/desktop/src/api/sdk.gen.ts

@@ -1,7 +1,7 @@
 // This file is auto-generated by @hey-api/openapi-ts
 
 import type { Options as ClientOptions, TDataShape, Client } from '@hey-api/client-fetch';
-import type { GetToolsData, GetToolsResponse, ReadAllConfigData, ReadAllConfigResponse, BackupConfigData, BackupConfigResponse, GetExtensionsData, GetExtensionsResponse, AddExtensionData, AddExtensionResponse, RemoveExtensionData, RemoveExtensionResponse, InitConfigData, InitConfigResponse, UpsertPermissionsData, UpsertPermissionsResponse, ProvidersData, ProvidersResponse2, ReadConfigData, RemoveConfigData, RemoveConfigResponse, UpsertConfigData, UpsertConfigResponse, ConfirmPermissionData, ManageContextData, ManageContextResponse, CreateScheduleData, CreateScheduleResponse, DeleteScheduleData, DeleteScheduleResponse, ListSchedulesData, ListSchedulesResponse2, UpdateScheduleData, UpdateScheduleResponse, InspectRunningJobData, InspectRunningJobResponse, KillRunningJobData, PauseScheduleData, PauseScheduleResponse, RunNowHandlerData, RunNowHandlerResponse, SessionsHandlerData, SessionsHandlerResponse, UnpauseScheduleData, UnpauseScheduleResponse, ListSessionsData, ListSessionsResponse, GetSessionHistoryData, GetSessionHistoryResponse } from './types.gen';
+import type { GetToolsData, GetToolsResponse, ReadAllConfigData, ReadAllConfigResponse, BackupConfigData, BackupConfigResponse, GetExtensionsData, GetExtensionsResponse, AddExtensionData, AddExtensionResponse, RemoveExtensionData, RemoveExtensionResponse, InitConfigData, InitConfigResponse, UpsertPermissionsData, UpsertPermissionsResponse, ProvidersData, ProvidersResponse2, ReadConfigData, RecoverConfigData, RecoverConfigResponse, RemoveConfigData, RemoveConfigResponse, UpsertConfigData, UpsertConfigResponse, ValidateConfigData, ValidateConfigResponse, ConfirmPermissionData, ManageContextData, ManageContextResponse, CreateScheduleData, CreateScheduleResponse, DeleteScheduleData, DeleteScheduleResponse, ListSchedulesData, ListSchedulesResponse2, UpdateScheduleData, UpdateScheduleResponse, InspectRunningJobData, InspectRunningJobResponse, KillRunningJobData, PauseScheduleData, PauseScheduleResponse, RunNowHandlerData, RunNowHandlerResponse, SessionsHandlerData, SessionsHandlerResponse, UnpauseScheduleData, UnpauseScheduleResponse, ListSessionsData, ListSessionsResponse, GetSessionHistoryData, GetSessionHistoryResponse } from './types.gen';
 import { client as _heyApiClient } from './client.gen';
 
 export type Options<TData extends TDataShape = TDataShape, ThrowOnError extends boolean = boolean> = ClientOptions<TData, ThrowOnError> & {
@@ -100,6 +100,13 @@ export const readConfig = <ThrowOnError extends boolean = false>(options: Option
     });
 };
 
+export const recoverConfig = <ThrowOnError extends boolean = false>(options?: Options<RecoverConfigData, ThrowOnError>) => {
+    return (options?.client ?? _heyApiClient).post<RecoverConfigResponse, unknown, ThrowOnError>({
+        url: '/config/recover',
+        ...options
+    });
+};
+
 export const removeConfig = <ThrowOnError extends boolean = false>(options: Options<RemoveConfigData, ThrowOnError>) => {
     return (options.client ?? _heyApiClient).post<RemoveConfigResponse, unknown, ThrowOnError>({
         url: '/config/remove',
@@ -122,6 +129,13 @@ export const upsertConfig = <ThrowOnError extends boolean = false>(options: Opti
     });
 };
 
+export const validateConfig = <ThrowOnError extends boolean = false>(options?: Options<ValidateConfigData, ThrowOnError>) => {
+    return (options?.client ?? _heyApiClient).get<ValidateConfigResponse, unknown, ThrowOnError>({
+        url: '/config/validate',
+        ...options
+    });
+};
+
 export const confirmPermission = <ThrowOnError extends boolean = false>(options: Options<ConfirmPermissionData, ThrowOnError>) => {
     return (options.client ?? _heyApiClient).post<unknown, unknown, ThrowOnError>({
         url: '/confirm',

ui/desktop/src/api/types.gen.ts

@@ -810,6 +810,29 @@ export type ReadConfigResponses = {
     200: unknown;
 };
 
+export type RecoverConfigData = {
+    body?: never;
+    path?: never;
+    query?: never;
+    url: '/config/recover';
+};
+
+export type RecoverConfigErrors = {
+    /**
+     * Internal server error
+     */
+    500: unknown;
+};
+
+export type RecoverConfigResponses = {
+    /**
+     * Config recovery attempted
+     */
+    200: string;
+};
+
+export type RecoverConfigResponse = RecoverConfigResponses[keyof RecoverConfigResponses];
+
 export type RemoveConfigData = {
     body: ConfigKeyQuery;
     path?: never;
@@ -860,6 +883,29 @@ export type UpsertConfigResponses = {
 
 export type UpsertConfigResponse = UpsertConfigResponses[keyof UpsertConfigResponses];
 
+export type ValidateConfigData = {
+    body?: never;
+    path?: never;
+    query?: never;
+    url: '/config/validate';
+};
+
+export type ValidateConfigErrors = {
+    /**
+     * Config file is corrupted
+     */
+    422: unknown;
+};
+
+export type ValidateConfigResponses = {
+    /**
+     * Config validation result
+     */
+    200: string;
+};
+
+export type ValidateConfigResponse = ValidateConfigResponses[keyof ValidateConfigResponses];
+
 export type ConfirmPermissionData = {
     body: PermissionConfirmationRequest;
     path?: never;