From ae68067257f153c194e8291f483e56c81578aab5 Mon Sep 17 00:00:00 2001 From: Chris Busillo Date: Tue, 28 Jul 2026 20:05:04 -0400 Subject: [PATCH 1/3] test(app-server): restore hermetic integration keyring --- codex-rs/Cargo.lock | 3 + codex-rs/app-server/Cargo.toml | 2 + codex-rs/app-server/src/lib.rs | 21 ++ codex-rs/app-server/src/main.rs | 14 + codex-rs/app-server/tests/all.rs | 36 +++ codex-rs/app-server/tests/common/Cargo.toml | 1 + .../app-server/tests/common/json_logging.rs | 32 +- codex-rs/app-server/tests/common/lib.rs | 9 + .../tests/common/test_app_server.rs | 22 ++ .../app-server/tests/suite/keyring_store.rs | 205 +++++++++++++ codex-rs/app-server/tests/suite/logging.rs | 4 +- codex-rs/app-server/tests/suite/mod.rs | 2 + .../app-server/tests/suite/strict_config.rs | 19 +- .../suite/v2/connection_handling_websocket.rs | 10 +- codex-rs/cli/src/main.rs | 12 + codex-rs/cli/tests/app_server.rs | 4 +- codex-rs/keyring-store/src/lib.rs | 290 ++++++++++++++++++ 17 files changed, 670 insertions(+), 16 deletions(-) create mode 100644 codex-rs/app-server/tests/suite/keyring_store.rs diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index 968719bebc79..b1f59d4f24aa 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -404,6 +404,7 @@ dependencies = [ "codex-core", "codex-exec-server", "codex-features", + "codex-keyring-store", "codex-login", "codex-models-manager", "codex-protocol", @@ -2071,6 +2072,7 @@ dependencies = [ "codex-hooks", "codex-http-client", "codex-image-generation-extension", + "codex-keyring-store", "codex-login", "codex-mcp", "codex-mcp-extension", @@ -2101,6 +2103,7 @@ dependencies = [ "codex-web-search-extension", "codex-windows-sandbox", "core_test_support", + "ctor 0.6.3", "flate2", "futures", "hmac 0.12.1", diff --git a/codex-rs/app-server/Cargo.toml b/codex-rs/app-server/Cargo.toml index 687275dd14c3..8774b9a3762d 100644 --- a/codex-rs/app-server/Cargo.toml +++ b/codex-rs/app-server/Cargo.toml @@ -68,6 +68,7 @@ codex-utils-pty = { workspace = true } codex-backend-client = { workspace = true } codex-file-search = { workspace = true } codex-chatgpt = { workspace = true } +codex-keyring-store = { workspace = true } codex-login = { workspace = true } codex-image-generation-extension = { workspace = true } codex-memories-extension = { workspace = true } @@ -130,6 +131,7 @@ codex-code-bridge-service = { workspace = true } codex-model-provider-info = { workspace = true } codex-utils-cargo-bin = { workspace = true } core_test_support = { workspace = true } +ctor = { workspace = true } flate2 = { workspace = true } hmac = { workspace = true } opentelemetry = { workspace = true } diff --git a/codex-rs/app-server/src/lib.rs b/codex-rs/app-server/src/lib.rs index 0f7361421d0d..b98184ba50f0 100644 --- a/codex-rs/app-server/src/lib.rs +++ b/codex-rs/app-server/src/lib.rs @@ -80,6 +80,27 @@ use tracing_subscriber::layer::SubscriberExt; use tracing_subscriber::registry::Registry; use tracing_subscriber::util::SubscriberInitExt; +#[cfg(debug_assertions)] +use codex_keyring_store::TEST_KEYRING_DIR_ENV_VAR; + +#[cfg(debug_assertions)] +#[doc(hidden)] +pub fn install_test_keyring_store_from_env() -> anyhow::Result<()> { + let test_keyring_dir = std::env::var_os(TEST_KEYRING_DIR_ENV_VAR).ok_or_else(|| { + anyhow::anyhow!( + "{TEST_KEYRING_DIR_ENV_VAR} must be set when --use-test-keyring-store is used" + ) + })?; + let test_keyring_dir = std::path::PathBuf::from(test_keyring_dir); + anyhow::ensure!( + codex_keyring_store::tests::install_persisted_default_test_keyring_store( + &test_keyring_dir + )?, + "test keyring store was already configured" + ); + Ok(()) +} + const SQLITE_RECOVERY_CONFIG_WARNING_SUMMARY: &str = "Codex rebuilt its local database."; mod analytics_utils; diff --git a/codex-rs/app-server/src/main.rs b/codex-rs/app-server/src/main.rs index 4d5ab3f122bf..83ed5dc91bea 100644 --- a/codex-rs/app-server/src/main.rs +++ b/codex-rs/app-server/src/main.rs @@ -4,6 +4,8 @@ use codex_app_server::AppServerRuntimeOptions; use codex_app_server::AppServerTransport; use codex_app_server::AppServerWebsocketAuthArgs; use codex_app_server::PluginStartupTasks; +#[cfg(debug_assertions)] +use codex_app_server::install_test_keyring_store_from_env; use codex_app_server::run_main_with_transport_options; use codex_arg0::Arg0DispatchPaths; use codex_arg0::arg0_dispatch_or_else; @@ -57,6 +59,12 @@ struct AppServerArgs { #[arg(long = "disable-plugin-startup-tasks-for-tests", hide = true)] disable_plugin_startup_tasks_for_tests: bool, + /// Hidden debug-only test hook used to redirect credential storage away + /// from the host keyring. + #[cfg(debug_assertions)] + #[arg(long = "use-test-keyring-store", hide = true)] + use_test_keyring_store: bool, + /// Enable remote control for this app-server process without changing persistence. #[arg(long = "remote-control", hide = true)] remote_control: bool, @@ -74,6 +82,8 @@ fn main() -> anyhow::Result<()> { strict_config, #[cfg(debug_assertions)] disable_plugin_startup_tasks_for_tests, + #[cfg(debug_assertions)] + use_test_keyring_store, remote_control, } = AppServerArgs::parse(); let loader_overrides = if disable_managed_config_from_debug_env() { @@ -90,6 +100,10 @@ fn main() -> anyhow::Result<()> { ..Default::default() }; #[cfg(debug_assertions)] + if use_test_keyring_store { + install_test_keyring_store_from_env()?; + } + #[cfg(debug_assertions)] if disable_plugin_startup_tasks_for_tests { runtime_options.plugin_startup_tasks = PluginStartupTasks::Skip; } diff --git a/codex-rs/app-server/tests/all.rs b/codex-rs/app-server/tests/all.rs index fdf98aa9455b..f610218db184 100644 --- a/codex-rs/app-server/tests/all.rs +++ b/codex-rs/app-server/tests/all.rs @@ -2,4 +2,40 @@ // Single integration test binary that aggregates all test modules. // The submodules live in `tests/suite/`. +#[cfg(debug_assertions)] +use ctor::ctor; +#[cfg(debug_assertions)] +use ctor::dtor; +#[cfg(debug_assertions)] +use std::io::Write; + +#[cfg(debug_assertions)] +#[ctor] +fn install_test_keyring_store() { + let install_result = codex_keyring_store::tests::install_persisted_default_test_keyring_store( + codex_keyring_store::tests::shared_test_keyring_root(), + ); + if !matches!(install_result, Ok(true)) { + let mut stderr = std::io::stderr().lock(); + let _ = writeln!( + stderr, + "failed to install persisted app-server test keyring store: {install_result:?}" + ); + drop(stderr); + std::process::abort(); + } +} + +#[cfg(debug_assertions)] +#[dtor] +fn remove_test_keyring_store() { + if let Err(error) = codex_keyring_store::tests::remove_shared_test_keyring_root() { + let mut stderr = std::io::stderr().lock(); + let _ = writeln!( + stderr, + "failed to remove app-server test keyring directory: {error}" + ); + } +} + mod suite; diff --git a/codex-rs/app-server/tests/common/Cargo.toml b/codex-rs/app-server/tests/common/Cargo.toml index 8dd43051c7e4..03a6137f410f 100644 --- a/codex-rs/app-server/tests/common/Cargo.toml +++ b/codex-rs/app-server/tests/common/Cargo.toml @@ -20,6 +20,7 @@ codex-config = { workspace = true } codex-core = { workspace = true } codex-exec-server = { workspace = true } codex-features = { workspace = true } +codex-keyring-store = { workspace = true } codex-login = { workspace = true } codex-models-manager = { workspace = true } codex-protocol = { workspace = true } diff --git a/codex-rs/app-server/tests/common/json_logging.rs b/codex-rs/app-server/tests/common/json_logging.rs index f16deaecb8a8..936643819182 100644 --- a/codex-rs/app-server/tests/common/json_logging.rs +++ b/codex-rs/app-server/tests/common/json_logging.rs @@ -11,6 +11,9 @@ use serde_json::Value; use serde_json::json; use tokio::sync::Notify; +#[cfg(debug_assertions)] +use crate::configure_test_keyring_for_std_command; + #[derive(Clone, Default)] pub(crate) struct JsonLogCapture { lines: Arc>>, @@ -75,16 +78,26 @@ impl JsonLogCapture { } } +#[derive(Debug, Clone, Copy)] +pub enum AppServerJsonInvocation { + Standalone, + CodexCli, +} + pub fn app_server_json_shutdown_event( - binary: &str, - args: &[&str], + invocation: AppServerJsonInvocation, codex_home: &Path, ) -> Result { std::fs::write( codex_home.join("config.toml"), "[features]\nplugins = false\n", )?; - let output = Command::new(codex_utils_cargo_bin::cargo_bin(binary)?) + let binary = match invocation { + AppServerJsonInvocation::Standalone => "codex-app-server", + AppServerJsonInvocation::CodexCli => "codex", + }; + let mut command = Command::new(codex_utils_cargo_bin::cargo_bin(binary)?); + command .stdin(Stdio::null()) .env("CODEX_LAB_HOME", codex_home) .env( @@ -92,9 +105,16 @@ pub fn app_server_json_shutdown_event( codex_home.join("managed_config.toml"), ) .env("LOG_FORMAT", "json") - .env("RUST_LOG", "codex_app_server=info") - .args(args) - .output()?; + .env("RUST_LOG", "codex_app_server=info"); + if matches!(invocation, AppServerJsonInvocation::CodexCli) { + command.arg("app-server"); + } + #[cfg(debug_assertions)] + configure_test_keyring_for_std_command( + &mut command, + &codex_home.join("app-server-test-keyring"), + ); + let output = command.output()?; let stderr = String::from_utf8(output.stderr)?; anyhow::ensure!(output.status.success(), "app-server failed: {stderr}"); diff --git a/codex-rs/app-server/tests/common/lib.rs b/codex-rs/app-server/tests/common/lib.rs index 1cb946128cbc..150650ccff93 100644 --- a/codex-rs/app-server/tests/common/lib.rs +++ b/codex-rs/app-server/tests/common/lib.rs @@ -18,6 +18,8 @@ pub use auth_fixtures::ChatGptIdTokenClaims; pub use auth_fixtures::encode_id_token; pub use auth_fixtures::write_chatgpt_auth; use codex_app_server_protocol::JSONRPCResponse; +#[cfg(debug_assertions)] +pub use codex_keyring_store::TEST_KEYRING_DIR_ENV_VAR; pub use config::MockResponsesConfig; pub use config::write_mock_responses_config_toml; pub use config::write_mock_responses_config_toml_with_chatgpt_base_url; @@ -30,6 +32,7 @@ pub use core_test_support::test_absolute_path; pub use core_test_support::test_path_buf_with_windows; pub use core_test_support::test_tmp_path; pub use core_test_support::test_tmp_path_buf; +pub use json_logging::AppServerJsonInvocation; pub use json_logging::app_server_json_shutdown_event; pub use mock_model_server::create_mock_responses_server_repeating_assistant; pub use mock_model_server::create_mock_responses_server_sequence; @@ -54,6 +57,12 @@ pub use test_app_server::DEFAULT_CLIENT_NAME; pub use test_app_server::DISABLE_PLUGIN_STARTUP_TASKS_ARG; pub use test_app_server::TestAppServer; pub use test_app_server::TestAppServerBuilder; +#[cfg(debug_assertions)] +pub use test_app_server::USE_TEST_KEYRING_STORE_ARG; +#[cfg(debug_assertions)] +pub use test_app_server::configure_test_keyring_for_std_command; +#[cfg(debug_assertions)] +pub use test_app_server::configure_test_keyring_for_tokio_command; pub fn to_response(response: JSONRPCResponse) -> anyhow::Result { let value = serde_json::to_value(response.result)?; diff --git a/codex-rs/app-server/tests/common/test_app_server.rs b/codex-rs/app-server/tests/common/test_app_server.rs index 3eab47102ed8..90570fa5ed4d 100644 --- a/codex-rs/app-server/tests/common/test_app_server.rs +++ b/codex-rs/app-server/tests/common/test_app_server.rs @@ -125,6 +125,10 @@ use codex_exec_server::CODEX_EXEC_SERVER_NOISE_CHATGPT_ACCOUNT_ID_ENV_VAR; use codex_exec_server::CODEX_EXEC_SERVER_NOISE_ENVIRONMENT_ID_ENV_VAR; use codex_exec_server::CODEX_EXEC_SERVER_NOISE_REGISTRY_URL_ENV_VAR; use codex_exec_server::CODEX_EXEC_SERVER_URL_ENV_VAR; +#[cfg(debug_assertions)] +use codex_keyring_store::TEST_KEYRING_DIR_ENV_VAR; +#[cfg(debug_assertions)] +use codex_keyring_store::tests::shared_test_keyring_root; use codex_login::default_client::CODEX_INTERNAL_ORIGINATOR_OVERRIDE_ENV_VAR; use core_test_support::is_remote_test_environment; use core_test_support::test_codex::TestEnv; @@ -163,6 +167,8 @@ pub struct TestAppServer { pub const DEFAULT_CLIENT_NAME: &str = "codex-app-server-tests"; pub const DISABLE_PLUGIN_STARTUP_TASKS_ARG: &str = "--disable-plugin-startup-tasks-for-tests"; +#[cfg(debug_assertions)] +pub const USE_TEST_KEYRING_STORE_ARG: &str = "--use-test-keyring-store"; const DISABLE_MANAGED_CONFIG_ENV_VAR: &str = "CODEX_APP_SERVER_DISABLE_MANAGED_CONFIG"; const CODE_MODE_HOST_PATH_ENV_VAR: &str = "CODEX_CODE_MODE_HOST_PATH"; #[cfg(windows)] @@ -247,6 +253,8 @@ impl TestAppServer { ); cmd.env_remove(CODEX_INTERNAL_ORIGINATOR_OVERRIDE_ENV_VAR); cmd.args(args); + #[cfg(debug_assertions)] + configure_test_keyring_for_tokio_command(&mut cmd, shared_test_keyring_root()); for (k, v) in env_overrides { match v { @@ -1793,6 +1801,20 @@ impl TestAppServer { } } +#[cfg(debug_assertions)] +pub fn configure_test_keyring_for_std_command(command: &mut std::process::Command, root: &Path) { + command + .arg(USE_TEST_KEYRING_STORE_ARG) + .env(TEST_KEYRING_DIR_ENV_VAR, root); +} + +#[cfg(debug_assertions)] +pub fn configure_test_keyring_for_tokio_command(command: &mut Command, root: &Path) { + command + .arg(USE_TEST_KEYRING_STORE_ARG) + .env(TEST_KEYRING_DIR_ENV_VAR, root); +} + /// Builder for TestAppServer. pub struct TestAppServerBuilder { codex_home: Option, diff --git a/codex-rs/app-server/tests/suite/keyring_store.rs b/codex-rs/app-server/tests/suite/keyring_store.rs new file mode 100644 index 000000000000..cfbbb5a33aab --- /dev/null +++ b/codex-rs/app-server/tests/suite/keyring_store.rs @@ -0,0 +1,205 @@ +use anyhow::Context; +use anyhow::Result; +use app_test_support::DISABLE_PLUGIN_STARTUP_TASKS_ARG; +use app_test_support::TEST_KEYRING_DIR_ENV_VAR; +use app_test_support::USE_TEST_KEYRING_STORE_ARG; +use codex_keyring_store::DefaultKeyringStore; +use codex_keyring_store::KeyringStore; +use codex_keyring_store::tests::HermeticTestKeyringStore; +#[cfg(unix)] +use std::os::unix::fs::PermissionsExt; +use std::process::Command; +use std::process::Stdio; +use std::time::Duration; +use tempfile::TempDir; +use tokio::time::sleep; +use tokio::time::timeout; + +const CHILD_PROCESS_ENV_VAR: &str = "CODEX_APP_SERVER_TEST_KEYRING_CHILD"; +const CHILD_ACCOUNT_ENV_VAR: &str = "CODEX_APP_SERVER_TEST_KEYRING_CHILD_ACCOUNT"; +const CHILD_VALUE_ENV_VAR: &str = "CODEX_APP_SERVER_TEST_KEYRING_CHILD_VALUE"; +const CROSS_PROCESS_TEST_NAME: &str = + "suite::keyring_store::persisted_store_shares_values_across_processes"; + +#[test] +fn persisted_store_round_trips_overwrites_and_deletes() -> Result<()> { + let root = TempDir::new()?; + let store = HermeticTestKeyringStore::persisted(root.path().to_path_buf()); + #[cfg(unix)] + std::fs::set_permissions(root.path(), std::fs::Permissions::from_mode(0o755))?; + + assert_eq!(store.load("service", "account")?, None); + store.save("service", "account", "first")?; + assert_eq!(store.load("service", "account")?, Some("first".into())); + + #[cfg(unix)] + let (service_dir, credential_file) = { + let service_dir = std::fs::read_dir(root.path())? + .next() + .context("persisted test keyring should contain a service directory")?? + .path(); + let credential_file = std::fs::read_dir(&service_dir)? + .next() + .context("persisted test keyring should contain a credential file")?? + .path(); + std::fs::set_permissions(&service_dir, std::fs::Permissions::from_mode(0o755))?; + std::fs::set_permissions(&credential_file, std::fs::Permissions::from_mode(0o644))?; + (service_dir, credential_file) + }; + + store.save("service", "account", "second")?; + assert_eq!(store.load("service", "account")?, Some("second".into())); + assert_eq!(store.load("service", "other")?, None); + + #[cfg(unix)] + { + assert_eq!(root.path().metadata()?.permissions().mode() & 0o777, 0o700); + assert_eq!(service_dir.metadata()?.permissions().mode() & 0o777, 0o700); + assert_eq!( + credential_file.metadata()?.permissions().mode() & 0o777, + 0o600 + ); + } + + assert!(store.delete("service", "account")?); + assert!(!store.delete("service", "account")?); + assert_eq!(store.load("service", "account")?, None); + Ok(()) +} + +#[test] +fn persisted_store_shares_values_across_instances_and_isolates_roots() -> Result<()> { + let root = TempDir::new()?; + let store = HermeticTestKeyringStore::persisted(root.path().to_path_buf()); + + store.save("codex", "secrets|test-home", "test-passphrase")?; + let reopened_store = HermeticTestKeyringStore::persisted(root.path().to_path_buf()); + assert_eq!( + reopened_store.load("codex", "secrets|test-home")?, + Some("test-passphrase".to_string()) + ); + assert_eq!(store.load("other", "secrets|test-home")?, None); + + let other_root = TempDir::new()?; + let other_store = HermeticTestKeyringStore::persisted(other_root.path().to_path_buf()); + assert_eq!(other_store.load("codex", "secrets|test-home")?, None); + Ok(()) +} + +#[test] +fn persisted_store_shares_values_across_processes() -> Result<()> { + if std::env::var_os(CHILD_PROCESS_ENV_VAR).is_some() { + let account = std::env::var(CHILD_ACCOUNT_ENV_VAR)?; + let value = std::env::var(CHILD_VALUE_ENV_VAR)?; + let store = DefaultKeyringStore; + assert_eq!(store.load("cross-process", &account)?, Some(value.clone())); + store.save("cross-process", &format!("{account}-ack"), "child-read")?; + return Ok(()); + } + + let root = TempDir::new()?; + let store = HermeticTestKeyringStore::persisted(root.path().to_path_buf()); + let process_id = std::process::id(); + let timestamp = std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH)? + .as_nanos(); + let account = format!("account-{process_id}-{timestamp}"); + let value = format!("parent-value-{process_id}-{timestamp}"); + store.save("cross-process", &account, &value)?; + let output = Command::new(std::env::current_exe()?) + .env(CHILD_PROCESS_ENV_VAR, "1") + .env(CHILD_ACCOUNT_ENV_VAR, &account) + .env(CHILD_VALUE_ENV_VAR, &value) + .env(TEST_KEYRING_DIR_ENV_VAR, root.path()) + .args(["--exact", CROSS_PROCESS_TEST_NAME, "--nocapture"]) + .output()?; + anyhow::ensure!( + output.status.success(), + "child keyring test failed\nstdout:\n{}\nstderr:\n{}", + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr) + ); + assert_eq!( + store.load("cross-process", &format!("{account}-ack"))?, + Some("child-read".to_string()), + "child test did not acknowledge reading from the persisted store" + ); + Ok(()) +} + +#[tokio::test] +async fn app_server_flag_initializes_selected_test_store() -> Result<()> { + let codex_home = TempDir::new()?; + let keyring_parent = TempDir::new()?; + let keyring_root = keyring_parent.path().join("app-server-keyring"); + let mut command = + tokio::process::Command::new(codex_utils_cargo_bin::cargo_bin("codex-app-server")?); + command + .kill_on_drop(true) + .stdin(Stdio::piped()) + .stdout(Stdio::null()) + .stderr(Stdio::null()) + .env("CODEX_LAB_HOME", codex_home.path()) + .env(TEST_KEYRING_DIR_ENV_VAR, &keyring_root) + .args([ + USE_TEST_KEYRING_STORE_ARG, + DISABLE_PLUGIN_STARTUP_TASKS_ARG, + "--listen", + "stdio://", + ]); + let mut child = command.spawn()?; + let initialization_result = timeout(Duration::from_secs(30), async { + loop { + if keyring_root.is_dir() { + anyhow::ensure!( + child.try_wait()?.is_none(), + "app-server exited before the selected test keyring store was ready" + ); + return Ok::<_, anyhow::Error>(()); + } + if let Some(status) = child.try_wait()? { + anyhow::bail!( + "app-server exited with {status} before initializing the selected test keyring store" + ); + } + sleep(Duration::from_millis(20)).await; + } + }) + .await + .context("timed out waiting for app-server test keyring initialization") + .and_then(std::convert::identity); + let cleanup_result = async { + if child.try_wait()?.is_none() { + child.kill().await?; + child.wait().await?; + } + Ok::<_, anyhow::Error>(()) + } + .await; + initialization_result?; + cleanup_result?; + assert!(keyring_root.is_dir()); + #[cfg(unix)] + assert_eq!(keyring_root.metadata()?.permissions().mode() & 0o777, 0o700); + Ok(()) +} + +#[test] +fn test_keyring_flag_requires_directory() -> Result<()> { + let codex_home = TempDir::new()?; + let output = Command::new(codex_utils_cargo_bin::cargo_bin("codex-app-server")?) + .env("CODEX_LAB_HOME", codex_home.path()) + .env_remove(TEST_KEYRING_DIR_ENV_VAR) + .args([USE_TEST_KEYRING_STORE_ARG, "--listen", "off"]) + .output()?; + + assert!(!output.status.success()); + let stderr = String::from_utf8(output.stderr)?; + assert!( + stderr.contains( + "CODEX_APP_SERVER_TEST_KEYRING_DIR must be set when --use-test-keyring-store is used" + ), + "expected missing test keyring directory error in stderr, got: {stderr}" + ); + Ok(()) +} diff --git a/codex-rs/app-server/tests/suite/logging.rs b/codex-rs/app-server/tests/suite/logging.rs index 04486a70cef8..d228692e44c5 100644 --- a/codex-rs/app-server/tests/suite/logging.rs +++ b/codex-rs/app-server/tests/suite/logging.rs @@ -1,5 +1,6 @@ use anyhow::Context; use anyhow::Result; +use app_test_support::AppServerJsonInvocation; use app_test_support::MockResponsesConfig; use app_test_support::TestAppServer; use app_test_support::app_server_json_shutdown_event; @@ -25,7 +26,8 @@ const READ_TIMEOUT: Duration = Duration::from_secs(10); #[test] fn standalone_app_server_emits_json_info_events() -> Result<()> { let codex_home = TempDir::new()?; - let event = app_server_json_shutdown_event("codex-app-server", &[], codex_home.path())?; + let event = + app_server_json_shutdown_event(AppServerJsonInvocation::Standalone, codex_home.path())?; assert_eq!( event, diff --git a/codex-rs/app-server/tests/suite/mod.rs b/codex-rs/app-server/tests/suite/mod.rs index 09c40493f6c5..f274d0cd59df 100644 --- a/codex-rs/app-server/tests/suite/mod.rs +++ b/codex-rs/app-server/tests/suite/mod.rs @@ -1,6 +1,8 @@ mod auth; mod conversation_summary; mod fuzzy_file_search; +#[cfg(debug_assertions)] +mod keyring_store; mod logging; mod strict_config; mod v2; diff --git a/codex-rs/app-server/tests/suite/strict_config.rs b/codex-rs/app-server/tests/suite/strict_config.rs index d7c6a97b210d..428763526cac 100644 --- a/codex-rs/app-server/tests/suite/strict_config.rs +++ b/codex-rs/app-server/tests/suite/strict_config.rs @@ -1,6 +1,8 @@ use std::process::Command; use anyhow::Result; +#[cfg(debug_assertions)] +use app_test_support::configure_test_keyring_for_std_command; use tempfile::TempDir; #[test] @@ -13,12 +15,17 @@ foo = "bar" "#, )?; - let output = Command::new(codex_utils_cargo_bin::cargo_bin("codex-app-server")?) - .env("CODEX_LAB_HOME", codex_home.path()) - .env( - "CODEX_APP_SERVER_MANAGED_CONFIG_PATH", - codex_home.path().join("managed_config.toml"), - ) + let mut command = Command::new(codex_utils_cargo_bin::cargo_bin("codex-app-server")?); + command.env("CODEX_LAB_HOME", codex_home.path()).env( + "CODEX_APP_SERVER_MANAGED_CONFIG_PATH", + codex_home.path().join("managed_config.toml"), + ); + #[cfg(debug_assertions)] + configure_test_keyring_for_std_command( + &mut command, + &codex_home.path().join("app-server-test-keyring"), + ); + let output = command .args(["--strict-config", "--listen", "off"]) .output()?; diff --git a/codex-rs/app-server/tests/suite/v2/connection_handling_websocket.rs b/codex-rs/app-server/tests/suite/v2/connection_handling_websocket.rs index 20aaa98aca31..c4376d422208 100644 --- a/codex-rs/app-server/tests/suite/v2/connection_handling_websocket.rs +++ b/codex-rs/app-server/tests/suite/v2/connection_handling_websocket.rs @@ -3,6 +3,8 @@ use anyhow::Result; use anyhow::bail; use app_test_support::ChatGptAuthFixture; use app_test_support::DISABLE_PLUGIN_STARTUP_TASKS_ARG; +#[cfg(debug_assertions)] +use app_test_support::configure_test_keyring_for_tokio_command; use app_test_support::create_mock_responses_server_sequence_unchecked; use app_test_support::to_response; use app_test_support::write_chatgpt_auth; @@ -661,12 +663,14 @@ async fn spawn_websocket_server_with_args_and_logs( cmd.arg("--listen") .arg(listen_url) .arg(DISABLE_PLUGIN_STARTUP_TASKS_ARG) - .args(extra_args) .stdin(Stdio::null()) .stdout(Stdio::null()) .stderr(Stdio::piped()) .env("CODEX_LAB_HOME", codex_home) .env("RUST_LOG", rust_log); + #[cfg(debug_assertions)] + configure_test_keyring_for_tokio_command(&mut cmd, &codex_home.join("app-server-test-keyring")); + cmd.args(extra_args); let mut process = cmd .kill_on_drop(true) .spawn() @@ -817,12 +821,14 @@ async fn run_websocket_server_to_completion_with_args( cmd.arg("--listen") .arg(listen_url) .arg(DISABLE_PLUGIN_STARTUP_TASKS_ARG) - .args(extra_args) .stdin(Stdio::null()) .stdout(Stdio::null()) .stderr(Stdio::piped()) .env("CODEX_LAB_HOME", codex_home) .env("RUST_LOG", "warn"); + #[cfg(debug_assertions)] + configure_test_keyring_for_tokio_command(&mut cmd, &codex_home.join("app-server-test-keyring")); + cmd.args(extra_args); timeout(DEFAULT_READ_TIMEOUT, cmd.output()) .await .context("timed out waiting for websocket app-server to exit")? diff --git a/codex-rs/cli/src/main.rs b/codex-rs/cli/src/main.rs index 33efebc9ab71..5e62c877efcb 100644 --- a/codex-rs/cli/src/main.rs +++ b/codex-rs/cli/src/main.rs @@ -5,6 +5,8 @@ use clap::FromArgMatches; use clap::Parser; use clap_complete::Shell; use clap_complete::generate; +#[cfg(debug_assertions)] +use codex_app_server::install_test_keyring_store_from_env; use codex_app_server_daemon::BootstrapOptions as AppServerBootstrapOptions; use codex_app_server_daemon::LifecycleCommand as AppServerLifecycleCommand; use codex_app_server_daemon::RemoteControlMode as AppServerRemoteControlMode; @@ -544,6 +546,10 @@ struct AppServerCommand { #[arg(long = "strict-config", default_value_t = false)] strict_config: bool, + #[cfg(debug_assertions)] + #[arg(long = "use-test-keyring-store", hide = true)] + use_test_keyring_store: bool, + /// Transport endpoint URL. Supported values: `stdio://` (default), /// `unix://`, `unix://PATH`, `ws://IP:PORT`, `off`. #[arg( @@ -1127,6 +1133,8 @@ async fn cli_main( subcommand, code_mode_host, strict_config: app_server_strict_config, + #[cfg(debug_assertions)] + use_test_keyring_store, listen, stdio, remote_control, @@ -1140,6 +1148,10 @@ async fn cli_main( root_remote_auth_token_env.as_deref(), subcommand.as_ref(), )?; + #[cfg(debug_assertions)] + if use_test_keyring_store { + install_test_keyring_store_from_env()?; + } match subcommand { None => { let transport = if stdio { diff --git a/codex-rs/cli/tests/app_server.rs b/codex-rs/cli/tests/app_server.rs index e2634eed56a0..cf9b005f3fa0 100644 --- a/codex-rs/cli/tests/app_server.rs +++ b/codex-rs/cli/tests/app_server.rs @@ -1,6 +1,7 @@ use std::path::Path; use anyhow::Result; +use app_test_support::AppServerJsonInvocation; use app_test_support::app_server_json_shutdown_event; use predicates::str::contains; use pretty_assertions::assert_eq; @@ -35,7 +36,8 @@ foo = "bar" #[test] fn app_server_emits_json_info_events() -> Result<()> { let codex_home = TempDir::new()?; - let event = app_server_json_shutdown_event("codex", &["app-server"], codex_home.path())?; + let event = + app_server_json_shutdown_event(AppServerJsonInvocation::CodexCli, codex_home.path())?; assert_eq!( event, diff --git a/codex-rs/keyring-store/src/lib.rs b/codex-rs/keyring-store/src/lib.rs index ee91af114284..f1c629e5f2cb 100644 --- a/codex-rs/keyring-store/src/lib.rs +++ b/codex-rs/keyring-store/src/lib.rs @@ -3,8 +3,15 @@ use keyring::Error as KeyringError; use std::error::Error; use std::fmt; use std::fmt::Debug; +#[cfg(debug_assertions)] +use std::sync::Arc; +#[cfg(debug_assertions)] +use std::sync::OnceLock; use tracing::trace; +#[cfg(debug_assertions)] +pub const TEST_KEYRING_DIR_ENV_VAR: &str = "CODEX_APP_SERVER_TEST_KEYRING_DIR"; + #[derive(Debug)] pub enum CredentialStoreError { Other(KeyringError), @@ -45,11 +52,23 @@ pub trait KeyringStore: Debug + Send + Sync { fn delete(&self, service: &str, account: &str) -> Result; } +#[cfg(debug_assertions)] +static DEFAULT_KEYRING_STORE_OVERRIDE: OnceLock> = OnceLock::new(); + +#[cfg(debug_assertions)] +fn set_default_keyring_store_for_tests(keyring_store: Arc) -> bool { + DEFAULT_KEYRING_STORE_OVERRIDE.set(keyring_store).is_ok() +} + #[derive(Debug, Clone, Copy)] pub struct DefaultKeyringStore; impl KeyringStore for DefaultKeyringStore { fn load(&self, service: &str, account: &str) -> Result, CredentialStoreError> { + #[cfg(debug_assertions)] + if let Some(keyring_store) = DEFAULT_KEYRING_STORE_OVERRIDE.get() { + return keyring_store.load(service, account); + } trace!("keyring.load start, service={service}, account={account}"); let entry = Entry::new(service, account).map_err(CredentialStoreError::new)?; match entry.get_password() { @@ -69,6 +88,10 @@ impl KeyringStore for DefaultKeyringStore { } fn save(&self, service: &str, account: &str, value: &str) -> Result<(), CredentialStoreError> { + #[cfg(debug_assertions)] + if let Some(keyring_store) = DEFAULT_KEYRING_STORE_OVERRIDE.get() { + return keyring_store.save(service, account, value); + } trace!( "keyring.save start, service={service}, account={account}, value_len={}", value.len() @@ -87,6 +110,10 @@ impl KeyringStore for DefaultKeyringStore { } fn delete(&self, service: &str, account: &str) -> Result { + #[cfg(debug_assertions)] + if let Some(keyring_store) = DEFAULT_KEYRING_STORE_OVERRIDE.get() { + return keyring_store.delete(service, account); + } trace!("keyring.delete start, service={service}, account={account}"); let entry = Entry::new(service, account).map_err(CredentialStoreError::new)?; match entry.delete_credential() { @@ -113,9 +140,29 @@ pub mod tests { use keyring::credential::CredentialApi as _; use keyring::mock::MockCredential; use std::collections::HashMap; + #[cfg(debug_assertions)] + use std::fs::OpenOptions; + #[cfg(debug_assertions)] + use std::io::Write; + #[cfg(all(debug_assertions, unix))] + use std::os::unix::fs::DirBuilderExt; + #[cfg(all(debug_assertions, unix))] + use std::os::unix::fs::OpenOptionsExt; + #[cfg(all(debug_assertions, unix))] + use std::os::unix::fs::PermissionsExt; + #[cfg(debug_assertions)] + use std::path::Path; + #[cfg(debug_assertions)] + use std::path::PathBuf; use std::sync::Arc; use std::sync::Mutex; + #[cfg(debug_assertions)] + use std::sync::OnceLock; use std::sync::PoisonError; + #[cfg(debug_assertions)] + use std::sync::atomic::AtomicU64; + #[cfg(debug_assertions)] + use std::sync::atomic::Ordering; #[derive(Default, Clone, Debug)] pub struct MockKeyringStore { @@ -223,4 +270,247 @@ pub mod tests { Ok(removed) } } + + #[cfg(debug_assertions)] + static SHARED_TEST_KEYRING_ROOT: OnceLock = OnceLock::new(); + + #[cfg(debug_assertions)] + struct SharedTestKeyringRoot { + path: PathBuf, + owned: bool, + } + + #[cfg(debug_assertions)] + #[derive(Debug)] + pub struct HermeticTestKeyringStore { + root: PathBuf, + next_temp_file_id: AtomicU64, + } + + #[cfg(debug_assertions)] + impl HermeticTestKeyringStore { + pub fn persisted(root: PathBuf) -> Self { + Self { + root, + next_temp_file_id: AtomicU64::new(0), + } + } + + fn entry_path(&self, service: &str, account: &str) -> PathBuf { + self.root + .join(encoded_path_component(service)) + .join(encoded_path_component(account)) + } + } + + #[cfg(debug_assertions)] + impl KeyringStore for HermeticTestKeyringStore { + fn load( + &self, + service: &str, + account: &str, + ) -> Result, CredentialStoreError> { + match std::fs::read(self.entry_path(service, account)) { + Ok(bytes) => String::from_utf8(bytes).map(Some).map_err(|error| { + CredentialStoreError::new(KeyringError::BadEncoding(error.into_bytes())) + }), + Err(error) if error.kind() == std::io::ErrorKind::NotFound => Ok(None), + Err(error) => Err(file_store_error(error)), + } + } + + fn save( + &self, + service: &str, + account: &str, + value: &str, + ) -> Result<(), CredentialStoreError> { + secure_directory(&self.root)?; + let path = self.entry_path(service, account); + let parent = path.parent().ok_or_else(|| { + file_store_error(std::io::Error::other("test keyring path has no parent")) + })?; + secure_directory(parent)?; + let process_id = std::process::id(); + let encoded_account = encoded_path_component(account); + let (temp_path, mut temp_file) = loop { + let temp_file_id = self.next_temp_file_id.fetch_add(1, Ordering::Relaxed); + let temp_path = parent.join(format!( + ".{process_id}.{temp_file_id}.{encoded_account}.tmp" + )); + let mut open_options = OpenOptions::new(); + open_options.create_new(true).write(true); + #[cfg(unix)] + open_options.mode(0o600); + match open_options.open(&temp_path) { + Ok(temp_file) => break (temp_path, temp_file), + Err(error) if error.kind() == std::io::ErrorKind::AlreadyExists => continue, + Err(error) => return Err(file_store_error(error)), + } + }; + temp_file + .write_all(value.as_bytes()) + .map_err(file_store_error)?; + temp_file.sync_all().map_err(file_store_error)?; + drop(temp_file); + if let Err(error) = replace_file(&temp_path, &path) { + let _ = std::fs::remove_file(&temp_path); + return Err(file_store_error(error)); + } + Ok(()) + } + + fn delete(&self, service: &str, account: &str) -> Result { + match std::fs::remove_file(self.entry_path(service, account)) { + Ok(()) => Ok(true), + Err(error) if error.kind() == std::io::ErrorKind::NotFound => Ok(false), + Err(error) => Err(file_store_error(error)), + } + } + } + + #[cfg(debug_assertions)] + fn encoded_path_component(value: &str) -> String { + const HEX_DIGITS: &[u8; 16] = b"0123456789abcdef"; + let mut encoded = String::with_capacity(value.len() * 2); + for byte in value.as_bytes() { + encoded.push(char::from(HEX_DIGITS[usize::from(byte >> 4)])); + encoded.push(char::from(HEX_DIGITS[usize::from(byte & 0x0f)])); + } + encoded + } + + #[cfg(debug_assertions)] + fn file_store_error(error: std::io::Error) -> CredentialStoreError { + CredentialStoreError::new(KeyringError::PlatformFailure(Box::new(error))) + } + + #[cfg(debug_assertions)] + fn secure_directory(path: &Path) -> Result<(), CredentialStoreError> { + #[cfg(unix)] + { + let mut dir_builder = std::fs::DirBuilder::new(); + dir_builder.recursive(true).mode(0o700); + dir_builder.create(path).map_err(file_store_error)?; + std::fs::set_permissions(path, std::fs::Permissions::from_mode(0o700)) + .map_err(file_store_error)?; + } + #[cfg(not(unix))] + std::fs::create_dir_all(path).map_err(file_store_error)?; + Ok(()) + } + + #[cfg(debug_assertions)] + fn replace_file(source: &Path, destination: &Path) -> std::io::Result<()> { + #[cfg(not(windows))] + { + std::fs::rename(source, destination) + } + #[cfg(windows)] + { + match replace_file_windows(source, destination) { + Ok(()) => Ok(()), + Err(error) if error.kind() == std::io::ErrorKind::NotFound => { + std::fs::rename(source, destination) + } + Err(error) => Err(error), + } + } + } + + #[cfg(all(debug_assertions, windows))] + fn replace_file_windows(source: &Path, destination: &Path) -> std::io::Result<()> { + use std::ffi::c_void; + use std::os::windows::ffi::OsStrExt; + + const REPLACEFILE_IGNORE_MERGE_ERRORS: u32 = 0x0000_0002; + + unsafe extern "system" { + fn ReplaceFileW( + replaced_file_name: *const u16, + replacement_file_name: *const u16, + backup_file_name: *const u16, + replace_flags: u32, + exclude: *mut c_void, + reserved: *mut c_void, + ) -> i32; + } + + let destination_wide: Vec = destination + .as_os_str() + .encode_wide() + .chain(Some(0)) + .collect(); + let source_wide: Vec = source.as_os_str().encode_wide().chain(Some(0)).collect(); + let replaced = unsafe { + ReplaceFileW( + destination_wide.as_ptr(), + source_wide.as_ptr(), + std::ptr::null(), + REPLACEFILE_IGNORE_MERGE_ERRORS, + std::ptr::null_mut(), + std::ptr::null_mut(), + ) + }; + if replaced == 0 { + return Err(std::io::Error::last_os_error()); + } + Ok(()) + } + + #[cfg(debug_assertions)] + pub fn shared_test_keyring_root() -> &'static Path { + SHARED_TEST_KEYRING_ROOT + .get_or_init(|| { + if let Some(path) = std::env::var_os(super::TEST_KEYRING_DIR_ENV_VAR) { + let path = PathBuf::from(path); + secure_directory(&path).expect("open shared app-server test keyring root"); + return SharedTestKeyringRoot { path, owned: false }; + } + let process_id = std::process::id(); + let timestamp = std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .unwrap_or_default() + .as_nanos(); + let root = std::env::temp_dir().join(format!( + "codex-app-server-test-keyring-{process_id}-{timestamp}" + )); + #[cfg(unix)] + { + let mut dir_builder = std::fs::DirBuilder::new(); + dir_builder.mode(0o700); + dir_builder + .create(&root) + .expect("create shared app-server test keyring root"); + } + #[cfg(not(unix))] + std::fs::create_dir(&root).expect("create shared app-server test keyring root"); + SharedTestKeyringRoot { + path: root, + owned: true, + } + }) + .path + .as_path() + } + + #[cfg(debug_assertions)] + pub fn remove_shared_test_keyring_root() -> std::io::Result<()> { + if let Some(root) = SHARED_TEST_KEYRING_ROOT.get() + && root.owned + { + std::fs::remove_dir_all(&root.path)?; + } + Ok(()) + } + + #[cfg(debug_assertions)] + pub fn install_persisted_default_test_keyring_store( + root: &Path, + ) -> Result { + secure_directory(root)?; + Ok(super::set_default_keyring_store_for_tests(Arc::new( + HermeticTestKeyringStore::persisted(root.to_path_buf()), + ))) + } } From 7b966ecf6608a0870bdb0d07e14ed5f6651ce0b3 Mon Sep 17 00:00:00 2001 From: Chris Busillo Date: Tue, 28 Jul 2026 20:08:47 -0400 Subject: [PATCH 2/3] chore(convergence): remove restored keyring waiver --- upstream/convergence-waivers.json | 1 - 1 file changed, 1 deletion(-) diff --git a/upstream/convergence-waivers.json b/upstream/convergence-waivers.json index dc858db6f278..2f2c2ca81194 100644 --- a/upstream/convergence-waivers.json +++ b/upstream/convergence-waivers.json @@ -5,7 +5,6 @@ {"disposition": "pending_restore", "issue": 428, "path": "README.md", "reason": "IDENTITY-1 has no canonical identity decision yet, so the candidate carries the upstream README until #428 resolves product identity.", "violation": "reverted_to_upstream"}, {"disposition": "pending_restore", "issue": 428, "path": "codex-rs/app-server/src/request_processors/external_agent_config_processor.rs", "reason": "AGENT-1 external-agent configuration API lost at anchor 9d2eea2238; restoration is tracked #428 work, not an accepted upstream deletion.", "violation": "absent"}, {"disposition": "pending_restore", "issue": 428, "path": "codex-rs/app-server/src/request_processors/external_agent_config_processor_tests.rs", "reason": "AGENT-1 external-agent configuration API lost at anchor 9d2eea2238; restoration is tracked #428 work, not an accepted upstream deletion.", "violation": "absent"}, - {"disposition": "converged_with_upstream", "issue": 428, "path": "codex-rs/app-server/tests/suite/mod.rs", "reason": "Every Every Code-owned app-server proof is a v2 suite, so all of them -- Code Bridge, remote control, external-agent config, Background Review control, and Project Validation -- register in codex-rs/app-server/tests/suite/v2/mod.rs, which is guarded separately and does diverge from upstream. The crate-level registry has no owned entry left to carry, so matching upstream is the correct end state rather than pending restoration work.", "violation": "reverted_to_upstream"}, {"disposition": "pending_restore", "issue": 428, "path": "codex-rs/core/src/agent/control/restore.rs", "reason": "AGENT-1 and INTEGRATION-1 behavior lost at anchor 9d2eea2238; restoration is tracked #428 work, not an accepted upstream deletion.", "violation": "absent"}, {"disposition": "pending_restore", "issue": 428, "path": "codex-rs/core/src/agent/status.rs", "reason": "AGENT-1 and INTEGRATION-1 behavior lost at anchor 9d2eea2238; restoration is tracked #428 work, not an accepted upstream deletion.", "violation": "reverted_to_upstream"}, {"disposition": "pending_restore", "issue": 428, "path": "codex-rs/core/src/session/multi_agents.rs", "reason": "AGENT-1 and INTEGRATION-1 behavior lost at anchor 9d2eea2238; restoration is tracked #428 work, not an accepted upstream deletion.", "violation": "reverted_to_upstream"}, From 9cff6eeaf79bac1251ae4788b979f8a5891ff4c9 Mon Sep 17 00:00:00 2001 From: Chris Busillo Date: Tue, 28 Jul 2026 20:13:25 -0400 Subject: [PATCH 3/3] test(convergence): cover restored app-server registry --- .../test_upstream_convergence_guard.py | 24 +++++++------------ 1 file changed, 8 insertions(+), 16 deletions(-) diff --git a/.github/scripts/test_upstream_convergence_guard.py b/.github/scripts/test_upstream_convergence_guard.py index 7193bc239f19..950295b7a8d5 100644 --- a/.github/scripts/test_upstream_convergence_guard.py +++ b/.github/scripts/test_upstream_convergence_guard.py @@ -21,6 +21,7 @@ def manifest_entry(path: str, upstream_blob: str | None) -> dict[str, object]: } +APP_SERVER_REGISTRY = "codex-rs/app-server/tests/suite/mod.rs" V2_REGISTRY = "codex-rs/app-server/tests/suite/v2/mod.rs" # Owned implementations and proofs that landed after the last guard regeneration. @@ -421,26 +422,17 @@ def test_manifest_records_why_each_path_is_guarded(self) -> None: self.assertEqual({"ownership_baseline", "current_tree"}, sources) - def test_owned_app_server_proofs_register_in_the_guarded_v2_registry(self) -> None: - """The crate-level app-server registry carries no owned entry. + def test_owned_app_server_proofs_register_in_guarded_registries(self) -> None: + """Both app-server registries now carry owned integration proofs.""" - Every Every Code-owned app-server proof is a v2 suite, so reverting - `suite/v2/mod.rs` unregisters all of them while every proof file stays in - the tree. That is exactly the failure the crate-level registry cannot - catch, which is why its own `reverted_to_upstream` waiver is - `converged_with_upstream` rather than pending work. - """ - - entry = guarded_entry(self, V2_REGISTRY) + crate_entry = guarded_entry(self, APP_SERVER_REGISTRY) + v2_entry = guarded_entry(self, V2_REGISTRY) waivers = guard.load_waivers(guard.DEFAULT_WAIVERS) - self.assertEqual("intentionally_owned", entry["lane"]) + self.assertEqual("intentionally_owned", crate_entry["lane"]) + self.assertEqual("intentionally_owned", v2_entry["lane"]) + self.assertNotIn(guard.waiver_key(APP_SERVER_REGISTRY, guard.REVERTED), waivers) self.assertNotIn(guard.waiver_key(V2_REGISTRY, guard.REVERTED), waivers) - crate_registry = waivers[ - guard.waiver_key("codex-rs/app-server/tests/suite/mod.rs", guard.REVERTED) - ] - self.assertEqual("converged_with_upstream", crate_registry["disposition"]) - self.assertIn(V2_REGISTRY, crate_registry["reason"]) def test_reverting_the_v2_registry_to_upstream_is_detected(self) -> None: entry = guarded_entry(self, V2_REGISTRY)