refactor: abstract keyring logic to better enable DI (#3262)

Cargo.lock

@@ -3629,6 +3629,7 @@ dependencies = [
  "google-docs1",
  "google-drive3",
  "google-sheets4",
+ "goose",
  "http-body-util",
  "hyper 1.6.0",
  "ignore",

Justfile

@@ -177,6 +177,33 @@ run-server:
     @echo "Running server..."
     cargo run -p goose-server
 
+# Run tests across all crates with keyring disabled
+test:
+    @echo "Running tests with keyring disabled..."
+    GOOSE_DISABLE_KEYRING=1 cargo test --workspace
+
+# Run linting checks across all crates
+lint:
+    @echo "Running linting checks..."
+    cargo clippy --workspace --all-features -- -D warnings
+
+# Run comprehensive linting checks (includes tests, examples, benchmarks)
+lint-all:
+    @echo "Running comprehensive linting checks..."
+    cargo clippy --workspace --all-targets --all-features -- -D warnings
+
+# Format code across all crates
+format:
+    @echo "Formatting code..."
+    cargo fmt --all
+
+# Quality assurance: format, lint, and test everything
+qa:
+    @echo "Running quality assurance: format, lint, and test..."
+    @just format
+    @just lint
+    @just test
+
 # make GUI with latest binary
 lint-ui:
     cd ui/desktop && npm run lint:check
@@ -355,16 +382,16 @@ set windows-shell := ["powershell.exe", "-NoLogo", "-Command"]
 ### Build the core code
 ### profile = --release or "" for debug
 ### allparam = OR/AND/ANY/NONE --workspace --all-features --all-targets
-win-bld profile allparam: 
+win-bld profile allparam:
   cargo run {{profile}} -p goose-server --bin  generate_schema
   cargo build {{profile}} {{allparam}}
 
 ### Build just debug
-win-bld-dbg: 
+win-bld-dbg:
   just win-bld " " " "
 
 ### Build debug and test, examples,...
-win-bld-dbg-all: 
+win-bld-dbg-all:
   just win-bld " " "--workspace --all-targets --all-features"
 
 ### Build just release
@@ -427,8 +454,8 @@ win-total-rls *allparam:
   just win-bld-rls{{allparam}}
   just win-run-rls
 
-### Build and run the Kotlin example with 
+### Build and run the Kotlin example with
-### auto-generated bindings for goose-llm 
+### auto-generated bindings for goose-llm
 kotlin-example:
     # Build Rust dylib and generate Kotlin bindings
     cargo build -p goose-llm

crates/goose-mcp/Cargo.toml

@@ -13,6 +13,7 @@ workspace = true
 [dependencies]
 mcp-core = { path = "../mcp-core" }
 mcp-server = { path = "../mcp-server" }
+goose = { path = "../goose" }
 anyhow = "1.0.94"
 tokio = { version = "1", features = ["full"] }
 tracing = "0.1"

crates/goose-mcp/src/google_drive/mod.rs

@@ -161,12 +161,16 @@ impl GoogleDriveRouter {
             Err(_) => false,
         };
 
+        // Use factory to create keyring backend consistently
+        let keyring = goose::keyring::create_default_keyring();
+
         // Create a credentials manager for storing tokens securely
         let credentials_manager = Arc::new(CredentialsManager::new(
             credentials_path.clone(),
             fallback_to_disk,
             KEYCHAIN_SERVICE.to_string(),
             KEYCHAIN_USERNAME.to_string(),
+            keyring,
         ));
 
         // Read the OAuth credentials from the keyfile

crates/goose-mcp/src/google_drive/oauth_pkce.rs

@@ -193,10 +193,10 @@ impl PkceOAuth2Client {
             };
 
             // Store updated token data
-            self.credentials_manager
+            match self.credentials_manager.write_credentials(&token_data) {
-                .write_credentials(&token_data)
+                Ok(_) => debug!("Successfully stored token data"),
-                .map(|_| debug!("Successfully stored token data"))
+                Err(e) => error!("Failed to store token data: {}", e),
-                .unwrap_or_else(|e| error!("Failed to store token data: {}", e));
+            }
         } else {
             debug!("No refresh token provided in OAuth flow response");
         }
@@ -248,10 +248,10 @@ impl PkceOAuth2Client {
         };
 
         // Store updated token data
-        self.credentials_manager
+        match self.credentials_manager.write_credentials(&token_data) {
-            .write_credentials(&token_data)
+            Ok(_) => debug!("Successfully stored token data"),
-            .map(|_| debug!("Successfully stored token data"))
+            Err(e) => error!("Failed to store token data: {}", e),
-            .unwrap_or_else(|e| error!("Failed to store token data: {}", e));
+        }
 
         Ok(access_token)
     }

crates/goose-mcp/src/google_drive/storage.rs

@@ -1,16 +1,17 @@
 use anyhow::Result;
-use keyring::Entry;
+use goose::keyring::{KeyringBackend, KeyringError};
 use serde::{de::DeserializeOwned, Serialize};
 use std::fs;
 use std::path::Path;
+use std::sync::Arc;
 use thiserror::Error;
 use tracing::{debug, error, warn};
 
 #[allow(dead_code)]
 #[derive(Error, Debug)]
 pub enum StorageError {
-    #[error("Failed to access keychain: {0}")]
+    #[error("Failed to access keyring: {0}")]
-    KeyringError(#[from] keyring::Error),
+    KeyringError(String),
     #[error("Failed to access file system: {0}")]
     FileSystemError(#[from] std::io::Error),
     #[error("No credentials found")]
@@ -21,6 +22,15 @@ pub enum StorageError {
     SerializationError(#[from] serde_json::Error),
 }
 
+impl From<KeyringError> for StorageError {
+    fn from(err: KeyringError) -> Self {
+        match err {
+            KeyringError::NotFound { .. } => StorageError::NotFound,
+            _ => StorageError::KeyringError(err.to_string()),
+        }
+    }
+}
+
 /// CredentialsManager handles secure storage of OAuth credentials.
 /// It attempts to store credentials in the system keychain first,
 /// with fallback to file system storage if keychain access fails and fallback is enabled.
@@ -29,6 +39,7 @@ pub struct CredentialsManager {
     fallback_to_disk: bool,
     keychain_service: String,
     keychain_username: String,
+    keyring: Arc<dyn KeyringBackend>,
 }
 
 impl CredentialsManager {
@@ -37,12 +48,14 @@ impl CredentialsManager {
         fallback_to_disk: bool,
         keychain_service: String,
         keychain_username: String,
+        keyring: Arc<dyn KeyringBackend>,
     ) -> Self {
         Self {
             credentials_path,
             fallback_to_disk,
             keychain_service,
             keychain_username,
+            keyring,
         }
     }
 
@@ -53,17 +66,19 @@ impl CredentialsManager {
     ///
     /// # Type Parameters
     ///
-    /// * `T` - The type to deserialize the credentials into. Must implement `serde::de::DeserializeOwned`.
+    /// * `T` - The type to deserialize to. Must implement `serde::DeserializeOwned`.
     ///
     /// # Returns
     ///
-    /// * `Ok(T)` - The deserialized credentials
+    /// * `Ok(T)` - The deserialized data
     /// * `Err(StorageError)` - If reading or deserialization fails
     ///
     /// # Examples
     ///
     /// ```no_run
     /// # use goose_mcp::google_drive::storage::CredentialsManager;
+    /// # use goose::keyring::SystemKeyringBackend;
+    /// # use std::sync::Arc;
     /// use serde::{Serialize, Deserialize};
     ///
     /// #[derive(Serialize, Deserialize)]
@@ -73,34 +88,45 @@ impl CredentialsManager {
     ///     expiry: u64,
     /// }
     ///
+    /// # fn example() -> Result<(), Box<dyn std::error::Error>> {
+    /// let keyring = Arc::new(SystemKeyringBackend);
     /// let manager = CredentialsManager::new(
     ///     String::from("/path/to/credentials.json"),
     ///     true,  // fallback to disk if keychain fails
     ///     String::from("test_service"),
-    ///     String::from("test_user")
+    ///     String::from("test_user"),
+    ///     keyring
     /// );
     /// match manager.read_credentials::<OAuthToken>() {
-    ///     Ok(token) => println!("Token expires at: {}", token.expiry),
+    ///     Ok(token) => println!("Access token: {}", token.access_token),
     ///     Err(e) => eprintln!("Failed to read token: {}", e),
     /// }
+    /// # Ok(())
+    /// # }
     /// ```
     pub fn read_credentials<T>(&self) -> Result<T, StorageError>
     where
         T: DeserializeOwned,
     {
-        let json_str = Entry::new(&self.keychain_service, &self.keychain_username)
+        let json_str = self
-            .and_then(|entry| entry.get_password())
+            .keyring
+            .get_password(&self.keychain_service, &self.keychain_username)
             .inspect(|_| {
                 debug!("Successfully read credentials from keychain");
             })
             .or_else(|e| {
                 if self.fallback_to_disk {
-                    debug!("Falling back to file system due to keyring error: {}", e);
+                    warn!("Falling back to file system due to keyring error: {}", e);
                     self.read_from_file()
                 } else {
-                    match e {
+                    // Convert anyhow::Error back to our error type
-                        keyring::Error::NoEntry => Err(StorageError::NotFound),
+                    if let Some(keyring_err) = e.downcast_ref::<KeyringError>() {
-                        _ => Err(StorageError::KeyringError(e)),
+                        match keyring_err {
+                            KeyringError::NotFound { .. } => Err(StorageError::NotFound),
+                            _ => Err(StorageError::KeyringError(e.to_string())),
+                        }
+                    } else {
+                        Err(StorageError::KeyringError(e.to_string()))
                     }
                 }
             })?;
@@ -149,6 +175,8 @@ impl CredentialsManager {
     ///
     /// ```no_run
     /// # use goose_mcp::google_drive::storage::CredentialsManager;
+    /// # use goose::keyring::SystemKeyringBackend;
+    /// # use std::sync::Arc;
     /// use serde::{Serialize, Deserialize};
     ///
     /// #[derive(Serialize, Deserialize)]
@@ -158,21 +186,26 @@ impl CredentialsManager {
     ///     expiry: u64,
     /// }
     ///
+    /// # fn example() -> Result<(), Box<dyn std::error::Error>> {
     /// let token = OAuthToken {
     ///     access_token: String::from("access_token_value"),
     ///     refresh_token: String::from("refresh_token_value"),
     ///     expiry: 1672531200, // Unix timestamp
     /// };
     ///
+    /// let keyring = Arc::new(SystemKeyringBackend);
     /// let manager = CredentialsManager::new(
     ///     String::from("/path/to/credentials.json"),
     ///     true,  // fallback to disk if keychain fails
     ///     String::from("test_service"),
-    ///     String::from("test_user")
+    ///     String::from("test_user"),
+    ///     keyring
     /// );
     /// if let Err(e) = manager.write_credentials(&token) {
     ///     eprintln!("Failed to write token: {}", e);
     /// }
+    /// # Ok(())
+    /// # }
     /// ```
     pub fn write_credentials<T>(&self, content: &T) -> Result<(), StorageError>
     where
@@ -180,8 +213,8 @@ impl CredentialsManager {
     {
         let json_str = serde_json::to_string(content).map_err(StorageError::SerializationError)?;
 
-        Entry::new(&self.keychain_service, &self.keychain_username)
+        self.keyring
-            .and_then(|entry| entry.set_password(&json_str))
+            .set_password(&self.keychain_service, &self.keychain_username, &json_str)
             .inspect(|_| {
                 debug!("Successfully wrote credentials to keychain");
             })
@@ -190,13 +223,14 @@ impl CredentialsManager {
                     warn!("Falling back to file system due to keyring error: {}", e);
                     self.write_to_file(&json_str)
                 } else {
-                    Err(StorageError::KeyringError(e))
+                    Err(StorageError::KeyringError(e.to_string()))
                 }
             })
     }
 
     fn write_to_file(&self, content: &str) -> Result<(), StorageError> {
         let path = Path::new(&self.credentials_path);
+
         if let Some(parent) = path.parent() {
             if !parent.exists() {
                 match fs::create_dir_all(parent) {
@@ -229,6 +263,7 @@ impl Clone for CredentialsManager {
             fallback_to_disk: self.fallback_to_disk,
             keychain_service: self.keychain_service.clone(),
             keychain_username: self.keychain_username.clone(),
+            keyring: self.keyring.clone(),
         }
     }
 }
@@ -236,6 +271,7 @@ impl Clone for CredentialsManager {
 #[cfg(test)]
 mod tests {
     use super::*;
+    use goose::keyring::MockKeyringBackend;
     use serde::{Deserialize, Serialize};
     use tempfile::tempdir;
 
@@ -263,25 +299,31 @@ mod tests {
         let cred_path = temp_dir.path().join("test_credentials.json");
         let cred_path_str = cred_path.to_str().unwrap().to_string();
 
+        // Create a mock keyring backend
+        let keyring = Arc::new(MockKeyringBackend::new());
+
         // Create a credentials manager with fallback enabled
-        // Using a unique service name to ensure keychain operation fails
         let manager = CredentialsManager::new(
             cred_path_str,
             true, // fallback to disk
             "test_service".to_string(),
             "test_user".to_string(),
+            keyring,
         );
 
         // Test credentials to store
         let creds = TestCredentials::new();
 
-        // Write should write to keychain
+        // Write should succeed with mock keyring
         let write_result = manager.write_credentials(&creds);
-        assert!(write_result.is_ok(), "Write should succeed with fallback");
+        assert!(
+            write_result.is_ok(),
+            "Write should succeed with mock keyring"
+        );
 
-        // Read should read from keychain
+        // Read should succeed with mock keyring
         let read_result = manager.read_credentials::<TestCredentials>();
-        assert!(read_result.is_ok(), "Read should succeed with fallback");
+        assert!(read_result.is_ok(), "Read should succeed with mock keyring");
 
         // Verify the read credentials match what we wrote
         assert_eq!(
@@ -298,21 +340,29 @@ mod tests {
         let cred_path = temp_dir.path().join("nonexistent_credentials.json");
         let cred_path_str = cred_path.to_str().unwrap().to_string();
 
+        // Create a mock keyring backend (empty by default)
+        let keyring = Arc::new(MockKeyringBackend::new());
+
         // Create a credentials manager with fallback disabled
         let manager = CredentialsManager::new(
             cred_path_str,
             false, // no fallback to disk
             "test_service_that_should_not_exist".to_string(),
             "test_user_no_fallback".to_string(),
+            keyring,
         );
 
-        // Read should fail with NotFound or KeyringError depending on the system
+        // Read should fail with NotFound since mock keyring is empty and no fallback
         let read_result = manager.read_credentials::<TestCredentials>();
         println!("{:?}", read_result);
         assert!(
             read_result.is_err(),
             "Read should fail when credentials don't exist"
         );
+        assert!(
+            matches!(read_result.unwrap_err(), StorageError::NotFound),
+            "Should return NotFound error"
+        );
     }
 
     #[test]
@@ -327,11 +377,13 @@ mod tests {
     fn test_file_system_error_handling() {
         // Test handling of file system errors by using an invalid path
         let invalid_path = String::from("/nonexistent_directory/credentials.json");
+        let keyring = Arc::new(MockKeyringBackend::new());
         let manager = CredentialsManager::new(
             invalid_path,
             true,
             "test_service".to_string(),
             "test_user".to_string(),
+            keyring,
         );
 
         // Create test credentials

crates/goose/src/config/base.rs

@@ -1,6 +1,8 @@
+use crate::keyring::{
+    create_default_keyring, create_keyring_with_file_path, FileKeyringBackend, KeyringBackend,
+};
 use etcetera::{choose_app_strategy, AppStrategy, AppStrategyArgs};
 use fs2::FileExt;
-use keyring::Entry;
 use once_cell::sync::{Lazy, OnceCell};
 use serde::Deserialize;
 use serde_json::Value;
@@ -9,6 +11,7 @@ use std::env;
 use std::fs::OpenOptions;
 use std::io::Write;
 use std::path::{Path, PathBuf};
+use std::sync::Arc;
 use thiserror::Error;
 
 pub static APP_STRATEGY: Lazy<AppStrategyArgs> = Lazy::new(|| AppStrategyArgs {
@@ -19,6 +22,7 @@ pub static APP_STRATEGY: Lazy<AppStrategyArgs> = Lazy::new(|| AppStrategyArgs {
 
 const KEYRING_SERVICE: &str = "goose";
 const KEYRING_USERNAME: &str = "secrets";
+const SECRETS_FILE_NAME: &str = "secrets.yaml";
 
 #[cfg(test)]
 const TEST_KEYRING_SERVICE: &str = "goose-test";
@@ -105,12 +109,8 @@ impl From<keyring::Error> for ConfigError {
 /// For Goose-specific configuration, consider prefixing with "goose_" to avoid conflicts.
 pub struct Config {
     config_path: PathBuf,
-    secrets: SecretStorage,
+    keyring: Arc<dyn KeyringBackend>,
-}
+    keyring_service: String,
-
-enum SecretStorage {
-    Keyring { service: String },
-    File { path: PathBuf },
 }
 
 // Global instance
@@ -118,6 +118,19 @@ static GLOBAL_CONFIG: OnceCell<Config> = OnceCell::new();
 
 impl Default for Config {
     fn default() -> Self {
+        let config_dir = choose_app_strategy(APP_STRATEGY.clone())
+            .expect("goose requires a home dir")
+            .config_dir();
+
+        // Use factory with custom file path to maintain same behavior
+        let keyring = create_keyring_with_file_path(config_dir.join(SECRETS_FILE_NAME));
+        Self::with_keyring(keyring)
+    }
+}
+
+impl Config {
+    /// Create a new configuration instance with a custom keyring backend
+    pub fn with_keyring(keyring: Arc<dyn KeyringBackend>) -> Self {
         // choose_app_strategy().config_dir()
         // - macOS/Linux: ~/.config/goose/
         // - Windows:     ~\AppData\Roaming\Block\goose\config\
@@ -129,22 +142,13 @@ impl Default for Config {
 
         let config_path = config_dir.join("config.yaml");
 
-        let secrets = match env::var("GOOSE_DISABLE_KEYRING") {
-            Ok(_) => SecretStorage::File {
-                path: config_dir.join("secrets.yaml"),
-            },
-            Err(_) => SecretStorage::Keyring {
-                service: KEYRING_SERVICE.to_string(),
-            },
-        };
         Config {
             config_path,
-            secrets,
+            keyring,
+            keyring_service: KEYRING_SERVICE.to_string(),
         }
     }
-}
 
-impl Config {
     /// Get the global configuration instance.
     ///
     /// This will initialize the configuration with the default path (~/.config/goose/config.yaml)
@@ -160,9 +164,8 @@ impl Config {
     pub fn new<P: AsRef<Path>>(config_path: P, service: &str) -> Result<Self, ConfigError> {
         Ok(Config {
             config_path: config_path.as_ref().to_path_buf(),
-            secrets: SecretStorage::Keyring {
+            keyring: create_default_keyring(),
-                service: service.to_string(),
+            keyring_service: service.to_string(),
-            },
         })
     }
 
@@ -176,9 +179,8 @@ impl Config {
     ) -> Result<Self, ConfigError> {
         Ok(Config {
             config_path: config_path.as_ref().to_path_buf(),
-            secrets: SecretStorage::File {
+            keyring: Arc::new(FileKeyringBackend::new(secrets_path.as_ref().to_path_buf())),
-                path: secrets_path.as_ref().to_path_buf(),
+            keyring_service: KEYRING_SERVICE.to_string(),
-            },
         })
     }
 
@@ -476,32 +478,23 @@ impl Config {
         Ok(())
     }
 
-    // Load current secrets from the keyring
     pub fn load_secrets(&self) -> Result<HashMap<String, Value>, ConfigError> {
-        match &self.secrets {
+        match self
-            SecretStorage::Keyring { service } => {
+            .keyring
-                let entry = Entry::new(service, KEYRING_USERNAME)?;
+            .get_password(&self.keyring_service, KEYRING_USERNAME)
-
+        {
-                match entry.get_password() {
+            Ok(content) => {
-                    Ok(content) => {
+                let values: HashMap<String, Value> = serde_json::from_str(&content)?;
-                        let values: HashMap<String, Value> = serde_json::from_str(&content)?;
+                Ok(values)
-                        Ok(values)
-                    }
-                    Err(keyring::Error::NoEntry) => Ok(HashMap::new()),
-                    Err(e) => Err(ConfigError::KeyringError(e.to_string())),
-                }
             }
-            SecretStorage::File { path } => {
+            Err(e) => {
-                if path.exists() {
+                if let Some(keyring_err) = e.downcast_ref::<crate::keyring::KeyringError>() {
-                    let file_content = std::fs::read_to_string(path)?;
+                    match keyring_err {
-                    let yaml_value: serde_yaml::Value = serde_yaml::from_str(&file_content)?;
+                        crate::keyring::KeyringError::NotFound { .. } => Ok(HashMap::new()),
-                    let json_value: Value = serde_json::to_value(yaml_value)?;
+                        _ => Err(ConfigError::KeyringError(e.to_string())),
-                    match json_value {
-                        Value::Object(map) => Ok(map.into_iter().collect()),
-                        _ => Ok(HashMap::new()),
                     }
                 } else {
-                    Ok(HashMap::new())
+                    Err(ConfigError::KeyringError(e.to_string()))
                 }
             }
         }
@@ -653,18 +646,10 @@ impl Config {
     pub fn set_secret(&self, key: &str, value: Value) -> Result<(), ConfigError> {
         let mut values = self.load_secrets()?;
         values.insert(key.to_string(), value);
-
+        let json_value = serde_json::to_string(&values)?;
-        match &self.secrets {
+        self.keyring
-            SecretStorage::Keyring { service } => {
+            .set_password(&self.keyring_service, KEYRING_USERNAME, &json_value)
-                let json_value = serde_json::to_string(&values)?;
+            .map_err(|e| ConfigError::KeyringError(e.to_string()))?;
-                let entry = Entry::new(service, KEYRING_USERNAME)?;
-                entry.set_password(&json_value)?;
-            }
-            SecretStorage::File { path } => {
-                let yaml_value = serde_yaml::to_string(&values)?;
-                std::fs::write(path, yaml_value)?;
-            }
-        };
         Ok(())
     }
 
@@ -681,18 +666,10 @@ impl Config {
     pub fn delete_secret(&self, key: &str) -> Result<(), ConfigError> {
         let mut values = self.load_secrets()?;
         values.remove(key);
-
+        let json_value = serde_json::to_string(&values)?;
-        match &self.secrets {
+        self.keyring
-            SecretStorage::Keyring { service } => {
+            .set_password(&self.keyring_service, KEYRING_USERNAME, &json_value)
-                let json_value = serde_json::to_string(&values)?;
+            .map_err(|e| ConfigError::KeyringError(e.to_string()))?;
-                let entry = Entry::new(service, KEYRING_USERNAME)?;
-                entry.set_password(&json_value)?;
-            }
-            SecretStorage::File { path } => {
-                let yaml_value = serde_yaml::to_string(&values)?;
-                std::fs::write(path, yaml_value)?;
-            }
-        };
         Ok(())
     }
 }
@@ -754,18 +731,8 @@ pub fn load_init_config_from_workspace() -> Result<HashMap<String, Value>, Confi
 #[cfg(test)]
 mod tests {
     use super::*;
-    use serial_test::serial;
     use tempfile::NamedTempFile;
 
-    fn cleanup_keyring() -> Result<(), ConfigError> {
-        let entry = Entry::new(TEST_KEYRING_SERVICE, KEYRING_USERNAME)?;
-        match entry.delete_credential() {
-            Ok(_) => Ok(()),
-            Err(keyring::Error::NoEntry) => Ok(()),
-            Err(e) => Err(ConfigError::KeyringError(e.to_string())),
-        }
-    }
-
     #[test]
     fn test_basic_config() -> Result<(), ConfigError> {
         let temp_file = NamedTempFile::new().unwrap();
@@ -876,11 +843,12 @@ mod tests {
     }
 
     #[test]
-    #[serial]
     fn test_secret_management() -> Result<(), ConfigError> {
-        cleanup_keyring()?;
+        use crate::keyring::MockKeyringBackend;
-        let temp_file = NamedTempFile::new().unwrap();
+
-        let config = Config::new(temp_file.path(), TEST_KEYRING_SERVICE)?;
+        let _temp_file = NamedTempFile::new().unwrap();
+        let mock_keyring = Arc::new(MockKeyringBackend::new());
+        let config = Config::with_keyring(mock_keyring);
 
         // Test setting and getting a simple secret
         config.set_secret("api_key", Value::String("secret123".to_string()))?;
@@ -898,16 +866,55 @@ mod tests {
         let result: Result<String, ConfigError> = config.get_secret("api_key");
         assert!(matches!(result, Err(ConfigError::NotFound(_))));
 
-        cleanup_keyring()?;
         Ok(())
     }
 
     #[test]
-    #[serial]
+    fn test_secret_management_with_mock() -> Result<(), ConfigError> {
+        use crate::keyring::MockKeyringBackend;
+
+        // Save and remove GOOSE_DISABLE_KEYRING to ensure we use the mock keyring
+        let saved_disable = env::var("GOOSE_DISABLE_KEYRING").ok();
+        env::remove_var("GOOSE_DISABLE_KEYRING");
+
+        let mock_keyring = Arc::new(MockKeyringBackend::new());
+        let config = Config::with_keyring(mock_keyring.clone());
+
+        // Test setting and getting a simple secret
+        config.set_secret("api_key", Value::String("secret123".to_string()))?;
+        let value: String = config.get_secret("api_key")?;
+        assert_eq!(value, "secret123");
+
+        // Verify it's in the mock keyring
+        assert!(mock_keyring.contains("goose", "secrets"));
+
+        // Test environment variable override
+        std::env::set_var("API_KEY", "env_secret");
+        let value: String = config.get_secret("api_key")?;
+        assert_eq!(value, "env_secret");
+        std::env::remove_var("API_KEY");
+
+        // Test deleting a secret
+        config.delete_secret("api_key")?;
+        let result: Result<String, ConfigError> = config.get_secret("api_key");
+        assert!(matches!(result, Err(ConfigError::NotFound(_))));
+
+        // Restore GOOSE_DISABLE_KEYRING
+        match saved_disable {
+            Some(val) => env::set_var("GOOSE_DISABLE_KEYRING", val),
+            None => env::remove_var("GOOSE_DISABLE_KEYRING"),
+        }
+
+        Ok(())
+    }
+
+    #[test]
     fn test_multiple_secrets() -> Result<(), ConfigError> {
-        cleanup_keyring()?;
+        use crate::keyring::MockKeyringBackend;
-        let temp_file = NamedTempFile::new().unwrap();
+
-        let config = Config::new(temp_file.path(), TEST_KEYRING_SERVICE)?;
+        let _temp_file = NamedTempFile::new().unwrap();
+        let mock_keyring = Arc::new(MockKeyringBackend::new());
+        let config = Config::with_keyring(mock_keyring);
 
         // Set multiple secrets
         config.set_secret("key1", Value::String("secret1".to_string()))?;
@@ -928,7 +935,131 @@ mod tests {
         assert!(matches!(result1, Err(ConfigError::NotFound(_))));
         assert_eq!(value2, "secret2");
 
-        cleanup_keyring()?;
+        Ok(())
+    }
+
+    #[test]
+    fn test_multiple_secrets_with_mock() -> Result<(), ConfigError> {
+        use crate::keyring::MockKeyringBackend;
+
+        // Save and remove GOOSE_DISABLE_KEYRING to ensure we use the mock keyring
+        let saved_disable = env::var("GOOSE_DISABLE_KEYRING").ok();
+        env::remove_var("GOOSE_DISABLE_KEYRING");
+
+        let mock_keyring = Arc::new(MockKeyringBackend::new());
+        let config = Config::with_keyring(mock_keyring.clone());
+
+        // Set multiple secrets
+        config.set_secret("key1", Value::String("secret1".to_string()))?;
+        config.set_secret("key2", Value::String("secret2".to_string()))?;
+
+        // Verify both exist
+        let value1: String = config.get_secret("key1")?;
+        let value2: String = config.get_secret("key2")?;
+        assert_eq!(value1, "secret1");
+        assert_eq!(value2, "secret2");
+
+        // Delete one secret
+        config.delete_secret("key1")?;
+
+        // Verify key1 is gone but key2 remains
+        let result1: Result<String, ConfigError> = config.get_secret("key1");
+        let value2: String = config.get_secret("key2")?;
+        assert!(matches!(result1, Err(ConfigError::NotFound(_))));
+        assert_eq!(value2, "secret2");
+
+        // Restore GOOSE_DISABLE_KEYRING
+        match saved_disable {
+            Some(val) => env::set_var("GOOSE_DISABLE_KEYRING", val),
+            None => env::remove_var("GOOSE_DISABLE_KEYRING"),
+        }
+
+        Ok(())
+    }
+
+    #[test]
+    fn test_keyring_error_propagation() -> Result<(), ConfigError> {
+        use crate::keyring::{KeyringBackend, KeyringError};
+
+        // Create a failing keyring that returns backend errors
+        struct FailingKeyring;
+
+        impl KeyringBackend for FailingKeyring {
+            fn get_password(&self, _: &str, _: &str) -> anyhow::Result<String> {
+                Err(KeyringError::Backend("Keyring service unavailable".to_string()).into())
+            }
+            fn set_password(&self, _: &str, _: &str, _: &str) -> anyhow::Result<()> {
+                Err(KeyringError::Backend("Keyring service unavailable".to_string()).into())
+            }
+            fn delete_password(&self, _: &str, _: &str) -> anyhow::Result<()> {
+                Err(KeyringError::Backend("Keyring service unavailable".to_string()).into())
+            }
+        }
+
+        // Save and remove GOOSE_DISABLE_KEYRING to ensure we use the failing keyring
+        let saved_disable = env::var("GOOSE_DISABLE_KEYRING").ok();
+        env::remove_var("GOOSE_DISABLE_KEYRING");
+
+        let failing_keyring = Arc::new(FailingKeyring);
+        let config = Config::with_keyring(failing_keyring);
+
+        // This should return an error, not an empty HashMap
+        let result = config.load_secrets();
+        assert!(result.is_err());
+        assert!(result
+            .unwrap_err()
+            .to_string()
+            .contains("Keyring service unavailable"));
+
+        // Restore GOOSE_DISABLE_KEYRING
+        match saved_disable {
+            Some(val) => env::set_var("GOOSE_DISABLE_KEYRING", val),
+            None => env::remove_var("GOOSE_DISABLE_KEYRING"),
+        }
+
+        Ok(())
+    }
+
+    #[test]
+    fn test_keyring_not_found_returns_empty() -> Result<(), ConfigError> {
+        use crate::keyring::{KeyringBackend, KeyringError};
+
+        // Create a keyring that always returns NotFound
+        struct NotFoundKeyring;
+
+        impl KeyringBackend for NotFoundKeyring {
+            fn get_password(&self, service: &str, username: &str) -> anyhow::Result<String> {
+                Err(KeyringError::NotFound {
+                    service: service.to_string(),
+                    username: username.to_string(),
+                }
+                .into())
+            }
+            fn set_password(&self, _: &str, _: &str, _: &str) -> anyhow::Result<()> {
+                Ok(())
+            }
+            fn delete_password(&self, _: &str, _: &str) -> anyhow::Result<()> {
+                Ok(())
+            }
+        }
+
+        // Save and remove GOOSE_DISABLE_KEYRING to ensure we use the not-found keyring
+        let saved_disable = env::var("GOOSE_DISABLE_KEYRING").ok();
+        env::remove_var("GOOSE_DISABLE_KEYRING");
+
+        let not_found_keyring = Arc::new(NotFoundKeyring);
+        let config = Config::with_keyring(not_found_keyring);
+
+        // This should return an empty HashMap, not an error
+        let result = config.load_secrets()?;
+        assert_eq!(result.len(), 0);
+
+        // Restore GOOSE_DISABLE_KEYRING
+        match saved_disable {
+            Some(val) => env::set_var("GOOSE_DISABLE_KEYRING", val),
+            None => env::remove_var("GOOSE_DISABLE_KEYRING"),
+        }
+
         Ok(())
     }
 

crates/goose/src/keyring/factory.rs

@@ -0,0 +1,345 @@
+#[cfg(test)]
+use super::KeyringError;
+use super::{FileKeyringBackend, KeyringBackend, MockKeyringBackend, SystemKeyringBackend};
+use etcetera::AppStrategy;
+use std::path::PathBuf;
+use std::sync::Arc;
+use std::{env, path::Path};
+
+/// Factory for creating keyring backends with environment-based defaults.
+///
+/// This factory provides a consistent way to create keyring backends across
+/// the entire codebase, handling environment variable detection and providing
+/// sensible defaults based on the execution context.
+///
+/// # Environment Variable Priority
+///
+/// The factory checks environment variables in this order:
+/// 1. `GOOSE_USE_MOCK_KEYRING` (highest priority) - Forces mock backend for testing
+/// 2. `GOOSE_DISABLE_KEYRING` - Forces file-based backend for production without OS keyring
+/// 3. Default behavior - Uses system keyring for production
+pub struct KeyringFactory;
+
+impl KeyringFactory {
+    /// Create a keyring backend using environment-based defaults.
+    ///
+    /// This is the most common use case - let the factory decide which backend
+    /// to use based on environment variables and execution context.
+    ///
+    /// # Examples
+    ///
+    /// ```rust,no_run
+    /// use goose::keyring::KeyringFactory;
+    ///
+    /// let keyring = KeyringFactory::create_default();
+    /// keyring.set_password("service", "user", "password").unwrap();
+    /// ```
+    pub fn create_default() -> Arc<dyn KeyringBackend> {
+        Self::create_with_config(DefaultKeyringConfig::new())
+    }
+
+    /// Create a keyring backend with custom configuration.
+    ///
+    /// This allows overriding default behavior such as specifying a custom
+    /// file path for the file-based backend.
+    ///
+    /// # Examples
+    ///
+    /// ```rust,no_run
+    /// use goose::keyring::{KeyringFactory, DefaultKeyringConfig};
+    /// use std::path::PathBuf;
+    ///
+    /// let config = DefaultKeyringConfig::new()
+    ///     .with_file_path(PathBuf::from("/custom/path/secrets.yaml"));
+    /// let keyring = KeyringFactory::create_with_config(config);
+    /// ```
+    pub fn create_with_config(config: DefaultKeyringConfig) -> Arc<dyn KeyringBackend> {
+        // Priority 1: Mock keyring for testing
+        if Self::is_env_var_truthy("GOOSE_USE_MOCK_KEYRING") {
+            return Arc::new(MockKeyringBackend::new());
+        }
+
+        // Priority 2: File-based keyring when system keyring disabled
+        if Self::is_env_var_truthy("GOOSE_DISABLE_KEYRING") {
+            let file_path = config
+                .file_path
+                .unwrap_or_else(|| Self::default_config_dir().join("secrets.yaml"));
+            return Arc::new(FileKeyringBackend::new(file_path));
+        }
+
+        // Priority 3: System keyring (default)
+        Arc::new(SystemKeyringBackend)
+    }
+
+    /// Check if an environment variable is set to a truthy value.
+    ///
+    /// This matches the same logic used throughout the goose codebase for
+    /// consistency in environment variable interpretation.
+    ///
+    /// Truthy values: "1", "true", "yes", "on" (case insensitive)
+    /// Falsy values: "0", "false", "no", "off", "" or unset
+    fn is_env_var_truthy(var_name: &str) -> bool {
+        match env::var(var_name) {
+            Ok(value) => {
+                let normalized = value.trim().to_lowercase();
+                matches!(normalized.as_str(), "1" | "true" | "yes" | "on")
+            }
+            Err(_) => false,
+        }
+    }
+
+    /// Get the default configuration directory using the same logic as Config.
+    ///
+    /// This ensures consistency with the rest of the goose configuration system.
+    fn default_config_dir() -> PathBuf {
+        use etcetera::{choose_app_strategy, AppStrategyArgs};
+
+        let strategy = AppStrategyArgs {
+            top_level_domain: "Block".to_string(),
+            author: "Block".to_string(),
+            app_name: "goose".to_string(),
+        };
+
+        choose_app_strategy(strategy)
+            .expect("goose requires a home dir")
+            .config_dir()
+    }
+}
+
+/// Configuration options for keyring factory creation.
+///
+/// This struct allows customizing the behavior of the keyring factory
+/// without breaking the simple default use case.
+#[derive(Default)]
+pub struct DefaultKeyringConfig {
+    /// Optional custom file path for FileKeyringBackend.
+    ///
+    /// If not provided, defaults to `{config_dir}/secrets.yaml`
+    pub file_path: Option<PathBuf>,
+}
+
+impl DefaultKeyringConfig {
+    /// Create a new configuration with default values.
+    pub fn new() -> Self {
+        Default::default()
+    }
+
+    /// Set a custom file path for the file-based keyring backend.
+    ///
+    /// This path will be used when `GOOSE_DISABLE_KEYRING` is set but
+    /// `GOOSE_USE_MOCK_KEYRING` is not set.
+    pub fn with_file_path<P: AsRef<Path>>(mut self, path: P) -> Self {
+        self.file_path = Some(path.as_ref().to_path_buf());
+        self
+    }
+}
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+    use serial_test::serial;
+    use std::env;
+    use tempfile::tempdir;
+
+    /// Test utility for managing environment variables in tests.
+    ///
+    /// This ensures that environment variables are properly restored
+    /// after each test, preventing test interference.
+    struct EnvGuard {
+        key: String,
+        original_value: Option<String>,
+    }
+
+    impl EnvGuard {
+        fn set(key: &str, value: &str) -> Self {
+            let original_value = env::var(key).ok();
+            env::set_var(key, value);
+            EnvGuard {
+                key: key.to_string(),
+                original_value,
+            }
+        }
+
+        fn remove(key: &str) -> Self {
+            let original_value = env::var(key).ok();
+            env::remove_var(key);
+            EnvGuard {
+                key: key.to_string(),
+                original_value,
+            }
+        }
+    }
+
+    impl Drop for EnvGuard {
+        fn drop(&mut self) {
+            match &self.original_value {
+                Some(value) => env::set_var(&self.key, value),
+                None => env::remove_var(&self.key),
+            }
+        }
+    }
+
+    #[test]
+    #[serial]
+    fn test_mock_keyring_priority() {
+        let _guard1 = EnvGuard::set("GOOSE_USE_MOCK_KEYRING", "true");
+        let _guard2 = EnvGuard::set("GOOSE_DISABLE_KEYRING", "true");
+
+        let keyring = KeyringFactory::create_default();
+
+        // Should use MockKeyringBackend despite GOOSE_DISABLE_KEYRING being set
+        // We can verify this by checking that a non-existent key returns NotFound
+        match keyring.get_password("test_service", "test_user") {
+            Err(e) => {
+                if let Some(keyring_err) = e.downcast_ref::<KeyringError>() {
+                    assert!(matches!(keyring_err, KeyringError::NotFound { .. }));
+                }
+            }
+            Ok(_) => panic!("Mock keyring should return NotFound for non-existent keys"),
+        }
+    }
+
+    #[test]
+    #[serial]
+    fn test_file_keyring_when_disabled() {
+        let _guard1 = EnvGuard::remove("GOOSE_USE_MOCK_KEYRING");
+        let _guard2 = EnvGuard::set("GOOSE_DISABLE_KEYRING", "true");
+
+        let temp_dir = tempdir().unwrap();
+        let keyring = KeyringFactory::create_with_config(
+            DefaultKeyringConfig::new().with_file_path(temp_dir.path().join("test_secrets.yaml")),
+        );
+
+        // Use unique service/user names to avoid conflicts
+        let service = format!("service_{}", std::process::id());
+        let user = format!("user_{}", std::process::id());
+
+        // Verify it creates a FileKeyringBackend by testing file operations
+        keyring.set_password(&service, &user, "password").unwrap();
+        assert_eq!(keyring.get_password(&service, &user).unwrap(), "password");
+
+        // Verify the file was actually created
+        assert!(temp_dir.path().join("test_secrets.yaml").exists());
+    }
+
+    #[test]
+    #[serial]
+    fn test_system_keyring_default() {
+        // For this test, we'll use mock keyring to avoid OS keyring popups
+        // but verify the logic by ensuring that when neither override is set,
+        // the factory doesn't choose the file backend
+        let _guard1 = EnvGuard::set("GOOSE_USE_MOCK_KEYRING", "true");
+        let _guard2 = EnvGuard::remove("GOOSE_DISABLE_KEYRING");
+
+        let keyring = KeyringFactory::create_default();
+
+        // Verify it creates a MockKeyringBackend (since we forced it for testing)
+        // The main thing we're testing is that the logic flow is correct
+        match keyring.get_password("non_existent_service", "non_existent_user") {
+            Err(e) => {
+                if let Some(keyring_err) = e.downcast_ref::<KeyringError>() {
+                    assert!(matches!(keyring_err, KeyringError::NotFound { .. }));
+                }
+            }
+            Ok(_) => panic!("Mock keyring should return NotFound for non-existent keys"),
+        }
+    }
+
+    #[test]
+    #[serial]
+    fn test_default_config_with_custom_path() {
+        let _guard1 = EnvGuard::remove("GOOSE_USE_MOCK_KEYRING");
+        let _guard2 = EnvGuard::set("GOOSE_DISABLE_KEYRING", "true");
+
+        let temp_dir = tempdir().unwrap();
+        let custom_path = temp_dir.path().join("custom_secrets.yaml");
+
+        let config = DefaultKeyringConfig::new().with_file_path(&custom_path);
+        let keyring = KeyringFactory::create_with_config(config);
+
+        // Use unique service/user names to avoid conflicts
+        let service = format!("test_service_{}", std::process::id());
+        let user = format!("test_user_{}", std::process::id());
+
+        // Test that the custom path is used
+        keyring
+            .set_password(&service, &user, "test_password")
+            .unwrap();
+
+        // Verify the custom file was created
+        assert!(custom_path.exists());
+
+        // Verify we can retrieve the password
+        assert_eq!(
+            keyring.get_password(&service, &user).unwrap(),
+            "test_password"
+        );
+    }
+
+    #[test]
+    #[serial]
+    fn test_env_var_truthy_values() {
+        // Test truthy values
+        for value in [
+            "1", "true", "TRUE", "yes", "YES", "on", "ON", " true ", "True",
+        ] {
+            let _guard = EnvGuard::set("TEST_TRUTHY_VAR", value);
+            assert!(
+                KeyringFactory::is_env_var_truthy("TEST_TRUTHY_VAR"),
+                "Value '{}' should be truthy",
+                value
+            );
+        }
+
+        // Test falsy values
+        for value in [
+            "0", "false", "FALSE", "no", "NO", "off", "OFF", "", " ", "random",
+        ] {
+            let _guard = EnvGuard::set("TEST_TRUTHY_VAR", value);
+            assert!(
+                !KeyringFactory::is_env_var_truthy("TEST_TRUTHY_VAR"),
+                "Value '{}' should be falsy",
+                value
+            );
+        }
+
+        // Test unset variable
+        let _guard = EnvGuard::remove("TEST_TRUTHY_VAR");
+        assert!(
+            !KeyringFactory::is_env_var_truthy("TEST_TRUTHY_VAR"),
+            "Unset variable should be falsy"
+        );
+    }
+
+    #[test]
+    #[serial]
+    fn test_factory_consistency_with_existing_is_env_var_truthy() {
+        // This test ensures our factory's is_env_var_truthy matches the existing implementation
+        // We'll test a few key values to ensure consistency
+
+        let test_cases = [
+            ("1", true),
+            ("true", true),
+            ("TRUE", true),
+            ("yes", true),
+            ("on", true),
+            ("0", false),
+            ("false", false),
+            ("no", false),
+            ("off", false),
+            ("", false),
+            ("random", false),
+        ];
+
+        for (value, expected) in test_cases {
+            let _guard = EnvGuard::set("TEST_CONSISTENCY_VAR", value);
+            assert_eq!(
+                KeyringFactory::is_env_var_truthy("TEST_CONSISTENCY_VAR"),
+                expected,
+                "Value '{}' should be {}",
+                value,
+                if expected { "truthy" } else { "falsy" }
+            );
+        }
+    }
+}

crates/goose/src/keyring/file.rs

@@ -0,0 +1,306 @@
+use super::{KeyringBackend, KeyringError};
+use anyhow::Result;
+use serde_json::Value;
+use std::collections::HashMap;
+use std::path::PathBuf;
+
+pub struct FileKeyringBackend {
+    secrets_path: PathBuf,
+}
+
+impl FileKeyringBackend {
+    pub fn new(secrets_path: PathBuf) -> Self {
+        Self { secrets_path }
+    }
+
+    fn load_all_secrets(&self) -> Result<HashMap<String, String>> {
+        if self.secrets_path.exists() {
+            let file_content = std::fs::read_to_string(&self.secrets_path)?;
+            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) => {
+                    let mut result = HashMap::new();
+
+                    // Check if this is the new format (has "goose:secrets" key)
+                    let service_key = Self::make_key("goose", "secrets");
+                    if let Some(service_value) = map.get(&service_key) {
+                        // New format: decode JSON from the service key
+                        if let Some(json_str) = service_value.as_str() {
+                            if let Ok(secrets_map) =
+                                serde_json::from_str::<HashMap<String, Value>>(json_str)
+                            {
+                                for (key, value) in secrets_map {
+                                    if let Some(string_value) = value.as_str() {
+                                        result.insert(key, string_value.to_string());
+                                    } else {
+                                        result.insert(key, serde_json::to_string(&value)?);
+                                    }
+                                }
+                                return Ok(result);
+                            }
+                        }
+                    }
+
+                    // Legacy format: direct key-value mapping (read-only)
+                    for (key, value) in &map {
+                        if let Some(string_value) = value.as_str() {
+                            result.insert(key.clone(), string_value.to_string());
+                        } else {
+                            result.insert(key.clone(), serde_json::to_string(&value)?);
+                        }
+                    }
+
+                    Ok(result)
+                }
+                _ => Ok(HashMap::new()),
+            }
+        } else {
+            Ok(HashMap::new())
+        }
+    }
+
+    fn save_all_secrets(&self, secrets: &HashMap<String, String>) -> Result<()> {
+        // Convert strings back to appropriate JSON values
+        let mut json_map = serde_json::Map::new();
+        for (key, value) in secrets {
+            // Try to parse as JSON first, fall back to string
+            let json_value =
+                serde_json::from_str(value).unwrap_or_else(|_| Value::String(value.clone()));
+            json_map.insert(key.clone(), json_value);
+        }
+
+        let yaml_value = serde_yaml::to_string(&json_map)?;
+
+        // Ensure parent directory exists
+        if let Some(parent) = self.secrets_path.parent() {
+            std::fs::create_dir_all(parent)?;
+        }
+
+        std::fs::write(&self.secrets_path, yaml_value)?;
+        Ok(())
+    }
+
+    fn make_key(service: &str, username: &str) -> String {
+        format!("{}:{}", service, username)
+    }
+}
+
+impl KeyringBackend for FileKeyringBackend {
+    fn get_password(&self, service: &str, username: &str) -> Result<String> {
+        let key = Self::make_key(service, username);
+        let secrets = self.load_all_secrets()?;
+
+        secrets.get(&key).cloned().ok_or_else(|| {
+            KeyringError::NotFound {
+                service: service.to_string(),
+                username: username.to_string(),
+            }
+            .into()
+        })
+    }
+
+    fn set_password(&self, service: &str, username: &str, password: &str) -> Result<()> {
+        let key = Self::make_key(service, username);
+        let mut secrets = self.load_all_secrets()?;
+        secrets.insert(key, password.to_string());
+        self.save_all_secrets(&secrets)?;
+        Ok(())
+    }
+
+    fn delete_password(&self, service: &str, username: &str) -> Result<()> {
+        let key = Self::make_key(service, username);
+        let mut secrets = self.load_all_secrets()?;
+        secrets.remove(&key);
+        self.save_all_secrets(&secrets)?;
+        Ok(())
+    }
+}
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+    use tempfile::NamedTempFile;
+
+    #[test]
+    fn test_basic_operations() -> Result<()> {
+        let temp_file = NamedTempFile::new().unwrap();
+        let backend = FileKeyringBackend::new(temp_file.path().to_path_buf());
+
+        // Test setting a password
+        backend.set_password("test_service", "test_user", "test_password")?;
+
+        // Test getting the password
+        let password = backend.get_password("test_service", "test_user")?;
+        assert_eq!(password, "test_password");
+
+        // Test deleting the password
+        backend.delete_password("test_service", "test_user")?;
+
+        // Test that getting deleted password returns NotFound error
+        let result = backend.get_password("test_service", "test_user");
+        assert!(result.is_err());
+        if let Err(e) = result {
+            if let Some(keyring_err) = e.downcast_ref::<KeyringError>() {
+                assert!(matches!(keyring_err, KeyringError::NotFound { .. }));
+            } else {
+                panic!("Expected KeyringError::NotFound, got: {:?}", e);
+            }
+        }
+
+        Ok(())
+    }
+
+    #[test]
+    fn test_multiple_services() -> Result<()> {
+        let temp_file = NamedTempFile::new().unwrap();
+        let backend = FileKeyringBackend::new(temp_file.path().to_path_buf());
+
+        // Set passwords for different services
+        backend.set_password("service1", "user1", "password1")?;
+        backend.set_password("service2", "user2", "password2")?;
+        backend.set_password("service1", "user2", "password3")?;
+
+        // Verify all passwords can be retrieved correctly
+        assert_eq!(backend.get_password("service1", "user1")?, "password1");
+        assert_eq!(backend.get_password("service2", "user2")?, "password2");
+        assert_eq!(backend.get_password("service1", "user2")?, "password3");
+
+        // Delete one password and verify others remain
+        backend.delete_password("service1", "user1")?;
+        assert!(backend.get_password("service1", "user1").is_err());
+        assert_eq!(backend.get_password("service2", "user2")?, "password2");
+        assert_eq!(backend.get_password("service1", "user2")?, "password3");
+
+        Ok(())
+    }
+
+    #[test]
+    fn test_password_update() -> Result<()> {
+        let temp_file = NamedTempFile::new().unwrap();
+        let backend = FileKeyringBackend::new(temp_file.path().to_path_buf());
+
+        // Set initial password
+        backend.set_password("service", "user", "old_password")?;
+        assert_eq!(backend.get_password("service", "user")?, "old_password");
+
+        // Update password
+        backend.set_password("service", "user", "new_password")?;
+        assert_eq!(backend.get_password("service", "user")?, "new_password");
+
+        Ok(())
+    }
+
+    #[test]
+    fn test_nonexistent_file() -> Result<()> {
+        let temp_file = NamedTempFile::new().unwrap();
+        let file_path = temp_file.path().to_path_buf();
+        drop(temp_file); // Delete the file
+
+        let backend = FileKeyringBackend::new(file_path);
+
+        // Getting from non-existent file should return NotFound
+        let result = backend.get_password("service", "user");
+        assert!(result.is_err());
+        if let Err(e) = result {
+            if let Some(keyring_err) = e.downcast_ref::<KeyringError>() {
+                assert!(matches!(keyring_err, KeyringError::NotFound { .. }));
+            }
+        }
+
+        // Setting should create the file
+        backend.set_password("service", "user", "password")?;
+        assert_eq!(backend.get_password("service", "user")?, "password");
+
+        Ok(())
+    }
+
+    #[test]
+    fn test_empty_password() -> Result<()> {
+        let temp_file = NamedTempFile::new().unwrap();
+        let backend = FileKeyringBackend::new(temp_file.path().to_path_buf());
+
+        // Test setting and getting empty password
+        backend.set_password("service", "user", "")?;
+        assert_eq!(backend.get_password("service", "user")?, "");
+
+        Ok(())
+    }
+
+    #[test]
+    fn test_special_characters_in_credentials() -> Result<()> {
+        let temp_file = NamedTempFile::new().unwrap();
+        let backend = FileKeyringBackend::new(temp_file.path().to_path_buf());
+
+        // Test with special characters in service, user, and password
+        let service = "service-with-dashes_and_underscores.and.dots";
+        let user = "user@domain.com";
+        let password = "password with spaces & special chars: !@#$%^&*()";
+
+        backend.set_password(service, user, password)?;
+        assert_eq!(backend.get_password(service, user)?, password);
+
+        Ok(())
+    }
+
+    #[test]
+    fn test_json_content_in_password() -> Result<()> {
+        let temp_file = NamedTempFile::new().unwrap();
+        let backend = FileKeyringBackend::new(temp_file.path().to_path_buf());
+
+        // Test storing JSON content as password
+        let json_password =
+            r#"{"access_token":"abc123","refresh_token":"def456","expires_in":3600}"#;
+        backend.set_password("oauth_service", "user", json_password)?;
+
+        let retrieved = backend.get_password("oauth_service", "user")?;
+
+        // Parse both as JSON to compare content regardless of key order
+        let original: serde_json::Value = serde_json::from_str(json_password).unwrap();
+        let retrieved_parsed: serde_json::Value = serde_json::from_str(&retrieved).unwrap();
+        assert_eq!(original, retrieved_parsed);
+
+        Ok(())
+    }
+
+    #[test]
+    fn test_delete_nonexistent_password() -> Result<()> {
+        let temp_file = NamedTempFile::new().unwrap();
+        let backend = FileKeyringBackend::new(temp_file.path().to_path_buf());
+
+        // Deleting non-existent password should not error (idempotent)
+        backend.delete_password("nonexistent_service", "nonexistent_user")?;
+
+        Ok(())
+    }
+
+    #[test]
+    fn test_legacy_format_compatibility() -> Result<()> {
+        let temp_file = NamedTempFile::new().unwrap();
+
+        // Write a legacy format secrets.yaml file
+        let legacy_content = r#"openai_api_key: sk-abc123
+anthropic_api_key: ant-def456
+complex_config:
+  nested: value
+  number: 42
+"#;
+        std::fs::write(temp_file.path(), legacy_content)?;
+
+        // Load with FileKeyringBackend - should read legacy format
+        let backend = FileKeyringBackend::new(temp_file.path().to_path_buf());
+
+        // Load secrets should work with legacy format (read-only)
+        let secrets = backend.load_all_secrets()?;
+        assert_eq!(secrets.get("openai_api_key").unwrap(), "sk-abc123");
+        assert_eq!(secrets.get("anthropic_api_key").unwrap(), "ant-def456");
+        assert!(secrets.get("complex_config").unwrap().contains("nested"));
+
+        // Verify the original file format is preserved (no auto-migration)
+        let file_content = std::fs::read_to_string(temp_file.path())?;
+        assert!(file_content.contains("openai_api_key: sk-abc123"));
+        assert!(!file_content.contains("goose:secrets"));
+
+        Ok(())
+    }
+}

crates/goose/src/keyring/mock.rs

@@ -0,0 +1,71 @@
+use super::{KeyringBackend, KeyringError};
+use anyhow::Result;
+use std::collections::HashMap;
+use std::sync::{Arc, RwLock};
+
+#[derive(Clone, Default)]
+pub struct MockKeyringBackend {
+    storage: Arc<RwLock<HashMap<String, String>>>,
+}
+
+impl MockKeyringBackend {
+    pub fn new() -> Self {
+        Self::default()
+    }
+
+    pub fn clear(&self) {
+        self.storage
+            .write()
+            .expect("Mock keyring lock poisoned")
+            .clear();
+    }
+
+    pub fn contains(&self, service: &str, username: &str) -> bool {
+        let key = format!("{}:{}", service, username);
+        self.storage
+            .read()
+            .expect("Mock keyring lock poisoned")
+            .contains_key(&key)
+    }
+
+    fn make_key(service: &str, username: &str) -> String {
+        format!("{}:{}", service, username)
+    }
+}
+
+impl KeyringBackend for MockKeyringBackend {
+    fn get_password(&self, service: &str, username: &str) -> Result<String> {
+        let key = Self::make_key(service, username);
+        let storage = self
+            .storage
+            .read()
+            .map_err(|e| KeyringError::Backend(format!("Mock keyring lock poisoned: {}", e)))?;
+
+        storage
+            .get(&key)
+            .cloned()
+            .ok_or_else(|| KeyringError::NotFound {
+                service: service.to_string(),
+                username: username.to_string(),
+            })
+            .map_err(anyhow::Error::from)
+    }
+
+    fn set_password(&self, service: &str, username: &str, password: &str) -> Result<()> {
+        let key = Self::make_key(service, username);
+        self.storage
+            .write()
+            .map_err(|e| KeyringError::Backend(format!("Mock keyring lock poisoned: {}", e)))?
+            .insert(key, password.to_string());
+        Ok(())
+    }
+
+    fn delete_password(&self, service: &str, username: &str) -> Result<()> {
+        let key = Self::make_key(service, username);
+        self.storage
+            .write()
+            .map_err(|e| KeyringError::Backend(format!("Mock keyring lock poisoned: {}", e)))?
+            .remove(&key);
+        Ok(())
+    }
+}

crates/goose/src/keyring/mod.rs

@@ -0,0 +1,66 @@
+use anyhow::Result;
+
+pub trait KeyringBackend: Send + Sync {
+    fn get_password(&self, service: &str, username: &str) -> Result<String>;
+    fn set_password(&self, service: &str, username: &str, password: &str) -> Result<()>;
+    fn delete_password(&self, service: &str, username: &str) -> Result<()>;
+}
+
+#[derive(Debug, thiserror::Error)]
+pub enum KeyringError {
+    #[error("No entry found for service '{service}' user '{username}'")]
+    NotFound { service: String, username: String },
+    #[error("Access denied to keyring")]
+    AccessDenied,
+    #[error("Keyring backend error: {0}")]
+    Backend(String),
+}
+
+pub mod factory;
+pub mod file;
+pub mod mock;
+pub mod system;
+
+pub use factory::{DefaultKeyringConfig, KeyringFactory};
+pub use file::FileKeyringBackend;
+pub use mock::MockKeyringBackend;
+pub use system::SystemKeyringBackend;
+
+/// Convenience function for creating a keyring backend with environment-based defaults.
+///
+/// This is the most common use case - it automatically selects the appropriate
+/// keyring backend based on environment variables:
+/// - `GOOSE_USE_MOCK_KEYRING=true` → MockKeyringBackend (for testing)
+/// - `GOOSE_DISABLE_KEYRING=true` → FileKeyringBackend (for systems without keyring)
+/// - Default → SystemKeyringBackend (for normal operation)
+///
+/// # Examples
+///
+/// ```rust,no_run
+/// use goose::keyring::create_default_keyring;
+///
+/// let keyring = create_default_keyring();
+/// keyring.set_password("service", "user", "password").unwrap();
+/// ```
+pub fn create_default_keyring() -> std::sync::Arc<dyn KeyringBackend> {
+    KeyringFactory::create_default()
+}
+
+/// Convenience function for creating a keyring backend with a custom file path.
+///
+/// This is useful when you need to store secrets in a specific location
+/// while still respecting the environment variable hierarchy.
+///
+/// # Examples
+///
+/// ```rust,no_run
+/// use goose::keyring::create_keyring_with_file_path;
+/// use std::path::PathBuf;
+///
+/// let keyring = create_keyring_with_file_path(PathBuf::from("/custom/secrets.yaml"));
+/// ```
+pub fn create_keyring_with_file_path(
+    file_path: std::path::PathBuf,
+) -> std::sync::Arc<dyn KeyringBackend> {
+    KeyringFactory::create_with_config(DefaultKeyringConfig::new().with_file_path(file_path))
+}

crates/goose/src/keyring/system.rs

@@ -0,0 +1,44 @@
+use super::{KeyringBackend, KeyringError};
+use anyhow::Result;
+use keyring::Entry;
+
+pub struct SystemKeyringBackend;
+
+impl KeyringBackend for SystemKeyringBackend {
+    fn get_password(&self, service: &str, username: &str) -> Result<String> {
+        let entry =
+            Entry::new(service, username).map_err(|e| KeyringError::Backend(e.to_string()))?;
+
+        entry
+            .get_password()
+            .map_err(|e| match e {
+                keyring::Error::NoEntry => KeyringError::NotFound {
+                    service: service.to_string(),
+                    username: username.to_string(),
+                },
+                _ => KeyringError::Backend(e.to_string()),
+            })
+            .map_err(anyhow::Error::from)
+    }
+
+    fn set_password(&self, service: &str, username: &str, password: &str) -> Result<()> {
+        let entry =
+            Entry::new(service, username).map_err(|e| KeyringError::Backend(e.to_string()))?;
+
+        entry
+            .set_password(password)
+            .map_err(|e| KeyringError::Backend(e.to_string()))
+            .map_err(anyhow::Error::from)
+    }
+
+    fn delete_password(&self, service: &str, username: &str) -> Result<()> {
+        let entry =
+            Entry::new(service, username).map_err(|e| KeyringError::Backend(e.to_string()))?;
+
+        match entry.delete_credential() {
+            Ok(()) => Ok(()),
+            Err(keyring::Error::NoEntry) => Ok(()), // Already deleted is fine
+            Err(e) => Err(anyhow::Error::from(KeyringError::Backend(e.to_string()))),
+        }
+    }
+}

crates/goose/src/lib.rs

@@ -1,6 +1,7 @@
 pub mod agents;
 pub mod config;
 pub mod context_mgmt;
+pub mod keyring;
 pub mod message;
 pub mod model;
 pub mod permission;

crates/goose/src/providers/factory.rs

@@ -180,6 +180,39 @@ mod tests {
     use mcp_core::{content::TextContent, Role};
     use std::env;
 
+    /// Helper to save and restore environment variables for testing
+    struct EnvVarGuard {
+        saved_vars: Vec<(&'static str, Option<String>)>,
+    }
+
+    impl EnvVarGuard {
+        fn new(var_names: &[&'static str]) -> Self {
+            let saved_vars = var_names
+                .iter()
+                .map(|&name| (name, env::var(name).ok()))
+                .collect();
+            Self { saved_vars }
+        }
+
+        fn clear_all(&self) {
+            for (key, _) in &self.saved_vars {
+                env::remove_var(key);
+            }
+        }
+    }
+
+    impl Drop for EnvVarGuard {
+        fn drop(&mut self) {
+            // Restore all env vars
+            for (key, value) in &self.saved_vars {
+                match value {
+                    Some(val) => env::set_var(key, val),
+                    None => env::remove_var(key),
+                }
+            }
+        }
+    }
+
     #[allow(dead_code)]
     #[derive(Clone)]
     struct MockTestProvider {
@@ -230,10 +263,17 @@ mod tests {
 
     #[test]
     fn test_create_lead_worker_provider() {
-        // Save current env vars
+        let _guard = EnvVarGuard::new(&[
-        let saved_lead = env::var("GOOSE_LEAD_MODEL").ok();
+            "GOOSE_LEAD_MODEL",
-        let saved_provider = env::var("GOOSE_LEAD_PROVIDER").ok();
+            "GOOSE_LEAD_PROVIDER",
-        let saved_turns = env::var("GOOSE_LEAD_TURNS").ok();
+            "GOOSE_LEAD_TURNS",
+            "OPENAI_API_KEY",
+            "ANTHROPIC_API_KEY",
+        ]);
+
+        // Set fake API keys to avoid keychain access
+        env::set_var("OPENAI_API_KEY", "fake-test-key");
+        env::set_var("ANTHROPIC_API_KEY", "fake-test-key");
 
         // Test with basic lead model configuration
         env::set_var("GOOSE_LEAD_MODEL", "gpt-4o");
@@ -241,16 +281,21 @@ mod tests {
         // This will try to create a lead/worker provider
         let result = create("openai", ModelConfig::new("gpt-4o-mini".to_string()));
 
-        // The creation might succeed or fail depending on API keys, but we can verify the logic path
+        // Since we provided fake API keys, the creation should proceed to the provider instantiation
+        // It may still fail at the provider level, but we should not get keychain-related errors
         match result {
             Ok(_) => {
-                // If it succeeds, it means we created a lead/worker provider successfully
+                // If it succeeds with fake keys, the logic worked
-                // This would happen if API keys are available in the test environment
             }
             Err(error) => {
-                // If it fails, it should be due to missing API keys, confirming we tried to create providers
+                // Should not fail due to missing secrets, but may fail due to invalid fake keys
-                let error_msg = error.to_string();
+                let error_msg = error.to_string().to_lowercase();
-                assert!(error_msg.contains("OPENAI_API_KEY") || error_msg.contains("secret"));
+                // Ensure it's not a keychain/secret-related error
+                assert!(
+                    !error_msg.contains("not found") || // Configuration not found
+                    error_msg.contains("invalid") ||    // Invalid API key format
+                    error_msg.contains("unauthorized") // API validation failed
+                );
             }
         }
 
@@ -259,44 +304,27 @@ mod tests {
         env::set_var("GOOSE_LEAD_TURNS", "5");
 
         let _result = create("openai", ModelConfig::new("gpt-4o-mini".to_string()));
-        // Similar validation as above - will fail due to missing API keys but confirms the logic
+        // Similar validation as above - confirms the logic path
 
-        // Restore env vars
+        // EnvVarGuard will automatically restore all env vars when dropped
-        match saved_lead {
-            Some(val) => env::set_var("GOOSE_LEAD_MODEL", val),
-            None => env::remove_var("GOOSE_LEAD_MODEL"),
-        }
-        match saved_provider {
-            Some(val) => env::set_var("GOOSE_LEAD_PROVIDER", val),
-            None => env::remove_var("GOOSE_LEAD_PROVIDER"),
-        }
-        match saved_turns {
-            Some(val) => env::set_var("GOOSE_LEAD_TURNS", val),
-            None => env::remove_var("GOOSE_LEAD_TURNS"),
-        }
     }
 
     #[test]
     fn test_lead_model_env_vars_with_defaults() {
-        // Save current env vars
+        let guard = EnvVarGuard::new(&[
-        let saved_vars = [
+            "GOOSE_LEAD_MODEL",
-            ("GOOSE_LEAD_MODEL", env::var("GOOSE_LEAD_MODEL").ok()),
+            "GOOSE_LEAD_PROVIDER",
-            ("GOOSE_LEAD_PROVIDER", env::var("GOOSE_LEAD_PROVIDER").ok()),
+            "GOOSE_LEAD_TURNS",
-            ("GOOSE_LEAD_TURNS", env::var("GOOSE_LEAD_TURNS").ok()),
+            "GOOSE_LEAD_FAILURE_THRESHOLD",
-            (
+            "GOOSE_LEAD_FALLBACK_TURNS",
-                "GOOSE_LEAD_FAILURE_THRESHOLD",
+            "OPENAI_API_KEY",
-                env::var("GOOSE_LEAD_FAILURE_THRESHOLD").ok(),
+        ]);
-            ),
-            (
-                "GOOSE_LEAD_FALLBACK_TURNS",
-                env::var("GOOSE_LEAD_FALLBACK_TURNS").ok(),
-            ),
-        ];
 
         // Clear all lead env vars
-        for (key, _) in &saved_vars {
+        guard.clear_all();
-            env::remove_var(key);
+
-        }
+        // Set fake API key to avoid keychain access
+        env::set_var("OPENAI_API_KEY", "fake-test-key");
 
         // Set only the required lead model
         env::set_var("GOOSE_LEAD_MODEL", "grok-3");
@@ -304,15 +332,21 @@ mod tests {
         // This should use defaults for all other values
         let result = create("openai", ModelConfig::new("gpt-4o-mini".to_string()));
 
-        // Should attempt to create lead/worker provider (will fail due to missing API keys but confirms logic)
+        // Should attempt to create lead/worker provider with fake keys
         match result {
             Ok(_) => {
-                // Success means we have API keys and created the provider
+                // Success means the logic path worked
             }
             Err(error) => {
-                // Should fail due to missing API keys, confirming we tried to create providers
+                // Should not fail due to missing secrets since we provided fake keys
-                let error_msg = error.to_string();
+                let error_msg = error.to_string().to_lowercase();
-                assert!(error_msg.contains("OPENAI_API_KEY") || error_msg.contains("secret"));
+                // Allow various provider-level failures but not keychain issues
+                assert!(
+                    error_msg.contains("invalid")
+                        || error_msg.contains("unauthorized")
+                        || error_msg.contains("model")
+                        || !error_msg.contains("not found")
+                );
             }
         }
 
@@ -324,63 +358,47 @@ mod tests {
         let _result = create("openai", ModelConfig::new("gpt-4o-mini".to_string()));
         // Should still attempt to create lead/worker provider with custom settings
 
-        // Restore all env vars
+        // EnvVarGuard will automatically restore all env vars when dropped
-        for (key, value) in saved_vars {
-            match value {
-                Some(val) => env::set_var(key, val),
-                None => env::remove_var(key),
-            }
-        }
     }
 
     #[test]
     fn test_create_regular_provider_without_lead_config() {
-        // Save current env vars
+        let guard = EnvVarGuard::new(&[
-        let saved_lead = env::var("GOOSE_LEAD_MODEL").ok();
+            "GOOSE_LEAD_MODEL",
-        let saved_provider = env::var("GOOSE_LEAD_PROVIDER").ok();
+            "GOOSE_LEAD_PROVIDER",
-        let saved_turns = env::var("GOOSE_LEAD_TURNS").ok();
+            "GOOSE_LEAD_TURNS",
-        let saved_threshold = env::var("GOOSE_LEAD_FAILURE_THRESHOLD").ok();
+            "GOOSE_LEAD_FAILURE_THRESHOLD",
-        let saved_fallback = env::var("GOOSE_LEAD_FALLBACK_TURNS").ok();
+            "GOOSE_LEAD_FALLBACK_TURNS",
+            "OPENAI_API_KEY",
+        ]);
 
         // Ensure all GOOSE_LEAD_* variables are not set
-        env::remove_var("GOOSE_LEAD_MODEL");
+        guard.clear_all();
-        env::remove_var("GOOSE_LEAD_PROVIDER");
+
-        env::remove_var("GOOSE_LEAD_TURNS");
+        // Set fake API key to avoid keychain access
-        env::remove_var("GOOSE_LEAD_FAILURE_THRESHOLD");
+        env::set_var("OPENAI_API_KEY", "fake-test-key");
-        env::remove_var("GOOSE_LEAD_FALLBACK_TURNS");
 
         // This should try to create a regular provider
         let result = create("openai", ModelConfig::new("gpt-4o-mini".to_string()));
 
-        // The creation might succeed or fail depending on API keys
+        // The creation should proceed with fake API key
         match result {
             Ok(_) => {
                 // If it succeeds, it means we created a regular provider successfully
-                // This would happen if API keys are available in the test environment
             }
             Err(error) => {
-                // If it fails, it should be due to missing API keys
+                // Should not fail due to missing secrets since we provided fake key
-                let error_msg = error.to_string();
+                let error_msg = error.to_string().to_lowercase();
-                assert!(error_msg.contains("OPENAI_API_KEY") || error_msg.contains("secret"));
+                // Allow provider-level failures but not keychain issues
+                assert!(
+                    error_msg.contains("invalid")
+                        || error_msg.contains("unauthorized")
+                        || !error_msg.contains("not found")
+                );
             }
         }
 
-        // Restore env vars
+        // EnvVarGuard will automatically restore all env vars when dropped
-        if let Some(val) = saved_lead {
-            env::set_var("GOOSE_LEAD_MODEL", val);
-        }
-        if let Some(val) = saved_provider {
-            env::set_var("GOOSE_LEAD_PROVIDER", val);
-        }
-        if let Some(val) = saved_turns {
-            env::set_var("GOOSE_LEAD_TURNS", val);
-        }
-        if let Some(val) = saved_threshold {
-            env::set_var("GOOSE_LEAD_FAILURE_THRESHOLD", val);
-        }
-        if let Some(val) = saved_fallback {
-            env::set_var("GOOSE_LEAD_FALLBACK_TURNS", val);
-        }
     }
 
     #[test]