From b3b7c81d21ecd4c9c0bc40191c2d720591c17a06 Mon Sep 17 00:00:00 2001 From: mikhail-oai Date: Thu, 19 Mar 2026 15:10:16 -0400 Subject: [PATCH] cleaning up unnecessary code and exposed API --- codex-rs/keyring-store/src/lib.rs | 4 - codex-rs/keyring-store/src/split_json.rs | 163 +++++++++++++++-------- codex-rs/login/src/auth/storage_tests.rs | 30 ++--- codex-rs/rmcp-client/src/oauth.rs | 40 +++--- 4 files changed, 135 insertions(+), 102 deletions(-) diff --git a/codex-rs/keyring-store/src/lib.rs b/codex-rs/keyring-store/src/lib.rs index a635ab230e..138925e81b 100644 --- a/codex-rs/keyring-store/src/lib.rs +++ b/codex-rs/keyring-store/src/lib.rs @@ -8,13 +8,9 @@ use tracing::trace; mod split_json; pub use split_json::JsonKeyringError; -pub use split_json::SplitJsonKeyringError; pub use split_json::delete_json_from_keyring; -pub use split_json::delete_split_json_from_keyring; pub use split_json::load_json_from_keyring; -pub use split_json::load_split_json_from_keyring; pub use split_json::save_json_to_keyring; -pub use split_json::save_split_json_to_keyring; #[derive(Debug)] pub enum CredentialStoreError { diff --git a/codex-rs/keyring-store/src/split_json.rs b/codex-rs/keyring-store/src/split_json.rs index ca34b2d97a..388bf26b5e 100644 --- a/codex-rs/keyring-store/src/split_json.rs +++ b/codex-rs/keyring-store/src/split_json.rs @@ -91,10 +91,7 @@ pub fn load_json_from_keyring( service: &str, base_key: &str, ) -> Result, JsonKeyringError> { - if let Some(value) = load_split_json_from_keyring(keyring_store, service, base_key)? { - return Ok(Some(value)); - } - load_full_json_from_keyring(keyring_store, service, base_key) + load_split_json_from_keyring(keyring_store, service, base_key) } #[cfg(not(windows))] @@ -106,7 +103,7 @@ pub fn load_json_from_keyring( if let Some(value) = load_full_json_from_keyring(keyring_store, service, base_key)? { return Ok(Some(value)); } - load_split_json_from_keyring(keyring_store, service, base_key) + Ok(None) } #[cfg(windows)] @@ -117,9 +114,7 @@ pub fn save_json_to_keyring( value: &Value, ) -> Result<(), JsonKeyringError> { save_split_json_to_keyring(keyring_store, service, base_key, value)?; - if let Err(err) = delete_full_json_from_keyring(keyring_store, service, base_key) { - warn!("failed to remove stale full JSON record from keyring: {err}"); - } + Ok(()) } @@ -130,11 +125,10 @@ pub fn save_json_to_keyring( base_key: &str, value: &Value, ) -> Result<(), JsonKeyringError> { - save_full_json_to_keyring(keyring_store, service, base_key, value)?; - if let Err(err) = delete_split_json_from_keyring(keyring_store, service, base_key) { - warn!("failed to remove stale split JSON record from keyring: {err}"); - } - Ok(()) + let bytes = serde_json::to_vec(value).map_err(|err| { + SplitJsonKeyringError::new(format!("failed to serialize JSON record: {err}")) + })?; + save_secret_to_keyring(keyring_store, service, base_key, &bytes, "JSON record") } #[cfg(windows)] @@ -144,8 +138,7 @@ pub fn delete_json_from_keyring( base_key: &str, ) -> Result { let split_removed = delete_split_json_from_keyring(keyring_store, service, base_key)?; - let full_removed = delete_full_json_from_keyring(keyring_store, service, base_key)?; - Ok(split_removed || full_removed) + Ok(split_removed) } #[cfg(not(windows))] @@ -155,11 +148,10 @@ pub fn delete_json_from_keyring( base_key: &str, ) -> Result { let full_removed = delete_full_json_from_keyring(keyring_store, service, base_key)?; - let split_removed = delete_split_json_from_keyring(keyring_store, service, base_key)?; - Ok(full_removed || split_removed) + Ok(full_removed) } -pub fn load_split_json_from_keyring( +fn load_split_json_from_keyring( keyring_store: &K, service: &str, base_key: &str, @@ -170,7 +162,7 @@ pub fn load_split_json_from_keyring( inflate_split_json(keyring_store, service, base_key, &manifest).map(Some) } -pub fn save_split_json_to_keyring( +fn save_split_json_to_keyring( keyring_store: &K, service: &str, base_key: &str, @@ -234,7 +226,7 @@ pub fn save_split_json_to_keyring( Ok(()) } -pub fn delete_split_json_from_keyring( +fn delete_split_json_from_keyring( keyring_store: &K, service: &str, base_key: &str, @@ -288,19 +280,6 @@ fn load_full_json_from_keyring( } } -#[cfg(not(windows))] -fn save_full_json_to_keyring( - keyring_store: &K, - service: &str, - base_key: &str, - value: &Value, -) -> Result<(), SplitJsonKeyringError> { - let bytes = serde_json::to_vec(value).map_err(|err| { - SplitJsonKeyringError::new(format!("failed to serialize JSON record: {err}")) - })?; - save_secret_to_keyring(keyring_store, service, base_key, &bytes, "JSON record") -} - fn delete_full_json_from_keyring( keyring_store: &K, service: &str, @@ -653,7 +632,6 @@ mod tests { use super::save_json_to_keyring; use super::save_split_json_to_keyring; use super::value_key; - use crate::KeyringStore; use crate::tests::MockKeyringStore; use pretty_assertions::assert_eq; use serde_json::json; @@ -701,40 +679,109 @@ mod tests { } } - #[cfg(not(windows))] #[test] - fn json_storage_loads_split_json_compatibility_on_non_windows() { + fn json_storage_does_not_load_compatibility_layout() { let store = MockKeyringStore::default(); let expected = json!({ "token": "secret", "nested": {"id": 9} }); - save_split_json_to_keyring(&store, SERVICE, BASE_KEY, &expected) - .expect("split JSON should save"); + #[cfg(windows)] + { + store + .save( + SERVICE, + BASE_KEY, + &serde_json::to_string(&expected).expect("JSON should serialize"), + ) + .expect("full JSON should save"); + let loaded = + load_json_from_keyring(&store, SERVICE, BASE_KEY).expect("JSON should load"); + assert_eq!(loaded, None); + } + + #[cfg(not(windows))] + { + save_split_json_to_keyring(&store, SERVICE, BASE_KEY, &expected) + .expect("split JSON should save"); + let loaded = + load_json_from_keyring(&store, SERVICE, BASE_KEY).expect("JSON should load"); + assert_eq!(loaded, None); + } + } + + #[test] + fn json_storage_save_preserves_compatibility_layout() { + let store = MockKeyringStore::default(); + let current = json!({"current": true}); + let compat = json!({"split": true}); + + #[cfg(windows)] + { + store + .save( + SERVICE, + BASE_KEY, + &serde_json::to_string(&compat).expect("JSON should serialize"), + ) + .expect("full JSON should save"); + } + + #[cfg(not(windows))] + { + save_split_json_to_keyring(&store, SERVICE, BASE_KEY, &compat) + .expect("split JSON should save"); + } + + save_json_to_keyring(&store, SERVICE, BASE_KEY, ¤t).expect("JSON should save"); let loaded = load_json_from_keyring(&store, SERVICE, BASE_KEY) .expect("JSON should load") .expect("JSON should exist"); - assert_eq!(loaded, expected); + assert_eq!(loaded, current); + + #[cfg(windows)] + { + assert_eq!( + store.saved_value(BASE_KEY), + Some(serde_json::to_string(&compat).expect("JSON should serialize")) + ); + } + + #[cfg(not(windows))] + { + let compat_loaded = load_split_json_from_keyring(&store, SERVICE, BASE_KEY) + .expect("split JSON should load") + .expect("split JSON should exist"); + assert_eq!(compat_loaded, compat); + } } #[test] - fn json_storage_delete_removes_platform_and_compat_entries() { + fn json_storage_delete_removes_only_platform_entries() { let store = MockKeyringStore::default(); let current = json!({"current": true}); - let split = json!({"split": true}); + let compat = json!({"split": true}); - save_json_to_keyring(&store, SERVICE, BASE_KEY, ¤t).expect("JSON should save"); - save_split_json_to_keyring(&store, SERVICE, BASE_KEY, &split) - .expect("split JSON should save"); - store - .save( - SERVICE, - BASE_KEY, - &serde_json::to_string(¤t).expect("JSON should serialize"), - ) - .expect("legacy JSON should save"); + #[cfg(windows)] + { + store + .save( + SERVICE, + BASE_KEY, + &serde_json::to_string(&compat).expect("JSON should serialize"), + ) + .expect("full JSON should save"); + save_json_to_keyring(&store, SERVICE, BASE_KEY, ¤t).expect("JSON should save"); + } + + #[cfg(not(windows))] + { + save_json_to_keyring(&store, SERVICE, BASE_KEY, ¤t).expect("JSON should save"); + save_split_json_to_keyring(&store, SERVICE, BASE_KEY, &compat) + .expect("split JSON should save"); + } let removed = delete_json_from_keyring(&store, SERVICE, BASE_KEY) .expect("JSON delete should succeed"); @@ -745,8 +792,18 @@ mod tests { .expect("JSON load should succeed") .is_none() ); - assert!(!store.contains(BASE_KEY)); - assert!(!store.contains(&layout_key(BASE_KEY, MANIFEST_ENTRY))); + + #[cfg(windows)] + { + assert!(store.contains(BASE_KEY)); + assert!(!store.contains(&layout_key(BASE_KEY, MANIFEST_ENTRY))); + } + + #[cfg(not(windows))] + { + assert!(!store.contains(BASE_KEY)); + assert!(store.contains(&layout_key(BASE_KEY, MANIFEST_ENTRY))); + } } #[test] diff --git a/codex-rs/login/src/auth/storage_tests.rs b/codex-rs/login/src/auth/storage_tests.rs index 01ff479413..5da88c9ecd 100644 --- a/codex-rs/login/src/auth/storage_tests.rs +++ b/codex-rs/login/src/auth/storage_tests.rs @@ -292,7 +292,16 @@ fn keyring_auth_storage_load_supports_legacy_single_entry() -> anyhow::Result<() )?; let loaded = storage.load()?; - assert_eq!(Some(expected), loaded); + + #[cfg(not(windows))] + { + assert_eq!(Some(expected), loaded); + } + + #[cfg(windows)] + { + assert_eq!(None, loaded); + } Ok(()) } @@ -310,25 +319,6 @@ fn keyring_auth_storage_load_returns_deserialized_keyring_auth() -> anyhow::Resu Ok(()) } -#[test] -fn keyring_auth_storage_load_supports_split_json_compatibility() -> anyhow::Result<()> { - let codex_home = tempdir()?; - let mock_keyring = MockKeyringStore::default(); - let storage = KeyringAuthStorage::new( - codex_home.path().to_path_buf(), - Arc::new(mock_keyring.clone()), - ); - let expected = auth_with_prefix("split-compat"); - let key = compute_store_key(codex_home.path())?; - let value = serde_json::to_value(&expected)?; - - codex_keyring_store::save_split_json_to_keyring(&mock_keyring, KEYRING_SERVICE, &key, &value)?; - - let loaded = storage.load()?; - assert_eq!(Some(expected), loaded); - Ok(()) -} - #[test] fn keyring_auth_storage_compute_store_key_for_home_directory() -> anyhow::Result<()> { let codex_home = PathBuf::from("~/.codex"); diff --git a/codex-rs/rmcp-client/src/oauth.rs b/codex-rs/rmcp-client/src/oauth.rs index 8787b7c05e..ea43b757bd 100644 --- a/codex-rs/rmcp-client/src/oauth.rs +++ b/codex-rs/rmcp-client/src/oauth.rs @@ -603,7 +603,6 @@ mod tests { use codex_keyring_store::CredentialStoreError; use codex_keyring_store::load_json_from_keyring; use codex_keyring_store::save_json_to_keyring; - use codex_keyring_store::save_split_json_to_keyring; use keyring::Error as KeyringError; use pretty_assertions::assert_eq; use std::sync::Mutex; @@ -757,22 +756,6 @@ mod tests { Ok(()) } - #[test] - fn load_oauth_tokens_supports_split_json_compatibility() -> Result<()> { - let _env = TempCodexHome::new(); - let store = MockKeyringStore::default(); - let tokens = sample_tokens(); - let key = super::compute_store_key(&tokens.server_name, &tokens.url)?; - let value = serde_json::to_value(&tokens)?; - save_split_json_to_keyring(&store, KEYRING_SERVICE, &key, &value)?; - - let loaded = - super::load_oauth_tokens_from_keyring(&store, &tokens.server_name, &tokens.url)? - .expect("tokens should load from split-json compatibility format"); - assert_tokens_match_without_expiry(&loaded, &tokens); - Ok(()) - } - #[test] fn load_oauth_tokens_supports_legacy_single_entry() -> Result<()> { let _env = TempCodexHome::new(); @@ -783,9 +766,18 @@ mod tests { store.save(KEYRING_SERVICE, &key, &serialized)?; let loaded = - super::load_oauth_tokens_from_keyring(&store, &tokens.server_name, &tokens.url)? - .expect("tokens should load from keyring"); - assert_tokens_match_without_expiry(&loaded, &tokens); + super::load_oauth_tokens_from_keyring(&store, &tokens.server_name, &tokens.url)?; + + #[cfg(not(windows))] + { + let loaded = loaded.expect("tokens should load from keyring"); + assert_tokens_match_without_expiry(&loaded, &tokens); + } + + #[cfg(windows)] + { + assert!(loaded.is_none()); + } Ok(()) } @@ -895,15 +887,13 @@ mod tests { } #[test] - fn delete_oauth_tokens_removes_all_storage() -> Result<()> { + fn delete_oauth_tokens_removes_active_storage() -> Result<()> { let _env = TempCodexHome::new(); let store = MockKeyringStore::default(); let tokens = sample_tokens(); let key = super::compute_store_key(&tokens.server_name, &tokens.url)?; let value = serde_json::to_value(&tokens)?; - save_split_json_to_keyring(&store, KEYRING_SERVICE, &key, &value)?; - let serialized = serde_json::to_string(&tokens)?; - store.save(KEYRING_SERVICE, &key, &serialized)?; + save_json_to_keyring(&store, KEYRING_SERVICE, &key, &value)?; super::save_oauth_tokens_to_file(&tokens)?; let removed = super::delete_oauth_tokens_from_keyring_and_file( @@ -928,7 +918,7 @@ mod tests { let tokens = sample_tokens(); let key = super::compute_store_key(&tokens.server_name, &tokens.url)?; let value = serde_json::to_value(&tokens)?; - save_split_json_to_keyring(&store, KEYRING_SERVICE, &key, &value)?; + save_json_to_keyring(&store, KEYRING_SERVICE, &key, &value)?; assert!( super::load_oauth_tokens_from_keyring(&store, &tokens.server_name, &tokens.url)? .is_some()