From efa97f9bc6814d6549fca8997678a10ef0111a04 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Fri, 14 Aug 2026 20:08:49 +0000 Subject: [PATCH] Show onboarding when Codex home lacks authentication state (#38644) ## Why Routine state such as history, logs, sessions, or temporary files does not mean that the default account can authenticate. Treating any non-pristine Codex home as configured can show the composer before onboarding is complete. ## What changed - Base the startup decision on authentication-relevant state instead of requiring an empty Codex home. Existing credentials, configuration, workload identity, managed configuration, or a running local daemon continue to keep the composer visible. - Treat an unreadable or ambiguous home conservatively and keep the composer visible. - Allow onboarding when the legacy `--search` flag is the only configuration override. ## Testing Expanded startup preflight and draft tests to cover existing home state, credential sources, workload identity markers, daemon state, ambiguous paths, and search-only overrides. GitOrigin-RevId: 76837e3fd6aa714f2a7f72d191088a3faae23515 --- codex-rs/tui/src/startup_draft_tests.rs | 22 ++- codex-rs/tui/src/startup_orchestration.rs | 10 +- codex-rs/tui/src/startup_preflight.rs | 73 ++++----- codex-rs/tui/src/startup_preflight_tests.rs | 160 +++++++++++++++++--- 4 files changed, 207 insertions(+), 58 deletions(-) diff --git a/codex-rs/tui/src/startup_draft_tests.rs b/codex-rs/tui/src/startup_draft_tests.rs index 9350db9de9..859daac12a 100644 --- a/codex-rs/tui/src/startup_draft_tests.rs +++ b/codex-rs/tui/src/startup_draft_tests.rs @@ -518,7 +518,27 @@ async fn startup_draft_waits_for_onboarding_before_accepting_input() { ] .into_iter(), ); - pump.initial_screen = StartupDraftInitialScreen::Onboarding; + let codex_home = tempfile::tempdir().expect("create an existing custom Codex home"); + std::fs::write(codex_home.path().join("history.jsonl"), "") + .expect("create existing startup history"); + let system_config_path = codex_utils_absolute_path::AbsolutePathBuf::from_absolute_path( + codex_home.path().join("system.toml"), + ) + .expect("resolve missing system configuration"); + let search_override = [("web_search".to_string(), toml::Value::String("live".into()))]; + pump.initial_screen = + if crate::startup_preflight::has_only_search_config_override(&search_override) + && crate::startup_preflight::should_delay_startup_composer_for_first_login( + codex_home.path(), + Ok(system_config_path), + || Ok(false), + |name| (name == "CODEX_HOME").then(|| codex_home.path().as_os_str().to_os_string()), + ) + { + StartupDraftInitialScreen::Onboarding + } else { + StartupDraftInitialScreen::Composer + }; let mut tui = crate::tui::test_support::make_test_tui().expect("create test terminal"); pump.show_initial_screen(&mut tui) .expect("keep the composer hidden until onboarding finishes"); diff --git a/codex-rs/tui/src/startup_orchestration.rs b/codex-rs/tui/src/startup_orchestration.rs index 800d9da8f6..f0d9181188 100644 --- a/codex-rs/tui/src/startup_orchestration.rs +++ b/codex-rs/tui/src/startup_orchestration.rs @@ -142,17 +142,23 @@ pub(super) async fn run_main_inner( strict_config, cli.bypass_hook_trust, ); + let search_only_config_override = !workload_identity_selected + && cli.web_search + && startup_preflight::has_only_search_config_override(&cli_kv_overrides) + && loader_overrides_are_default(&launch_loader_overrides) + && !strict_config + && !cli.bypass_hook_trust; let initial_screen = if cli.resume_picker || cli.fork_picker { startup_draft::StartupDraftInitialScreen::SessionPicker } else if !cli.oss && explicit_remote_endpoint.is_none() - && reuse_implicit_local_daemon + && (reuse_implicit_local_daemon || search_only_config_override) && launch_loader_overrides.packaged_defaults_path.is_none() && startup_preflight::should_delay_startup_composer_for_first_login( &codex_home, codex_config::loader::system_config_toml_file(), || codex_config::loader::has_local_managed_configuration(&codex_home), - |name| std::env::var(name).ok(), + |name| std::env::var_os(name), ) { startup_draft::StartupDraftInitialScreen::Onboarding diff --git a/codex-rs/tui/src/startup_preflight.rs b/codex-rs/tui/src/startup_preflight.rs index be6f7508b4..7741319b76 100644 --- a/codex-rs/tui/src/startup_preflight.rs +++ b/codex-rs/tui/src/startup_preflight.rs @@ -1,22 +1,36 @@ -//! Conservative first-install checks that run before the provisional composer appears. +//! Conservative authentication checks that run before the provisional composer appears. //! -//! Any existing user, system, daemon, or authentication state keeps the composer visible. +//! Existing configuration, daemon, or authentication state keeps the composer visible. +use std::ffi::OsString; use std::io; use std::path::Path; +use codex_protocol::shell_environment::OPENAI_FEDERATION_RULE_ID_ENV_VAR; +use codex_protocol::shell_environment::OPENAI_IDENTITY_TOKEN_FILE_ENV_VAR; use codex_utils_absolute_path::AbsolutePathBuf; -/// Hide the composer only on a first installation without user or machine-wide configuration. +/// Recognize the single configuration override synthesized by the legacy `--search` flag. +pub(super) fn has_only_search_config_override(cli_kv_overrides: &[(String, toml::Value)]) -> bool { + matches!( + cli_kv_overrides, + [(key, toml::Value::String(value))] if key == "web_search" && value == "live" + ) +} + +/// Hide the composer when the default file-backed account cannot already be authenticated. pub(super) fn should_delay_startup_composer_for_first_login( codex_home: &Path, system_config_path: io::Result, managed_configuration: impl FnOnce() -> io::Result, - environment_variable: impl Fn(&str) -> Option, + environment_variable: impl Fn(&str) -> Option, ) -> bool { - if environment_variable("CODEX_HOME").is_some_and(|value| !value.is_empty()) - || environment_variable(codex_login::CODEX_ACCESS_TOKEN_ENV_VAR) - .is_some_and(|credential| !credential.trim().is_empty()) + if environment_variable(codex_login::CODEX_ACCESS_TOKEN_ENV_VAR).is_some_and(|credential| { + credential + .to_str() + .is_some_and(|value| !value.trim().is_empty()) + }) || environment_variable(OPENAI_FEDERATION_RULE_ID_ENV_VAR).is_some() + || environment_variable(OPENAI_IDENTITY_TOKEN_FILE_ENV_VAR).is_some() { return false; } @@ -28,40 +42,31 @@ pub(super) fn should_delay_startup_composer_for_first_login( return false; } - let pristine_home = match codex_home.try_exists() { - Ok(false) => true, - Err(_) => false, - Ok(true) => { - let Ok(mut entries) = std::fs::read_dir(codex_home) else { - return false; - }; - let Some(Ok(temporary_root)) = entries.next() else { - return false; - }; - if temporary_root.file_name() != "tmp" - || !temporary_root - .file_type() - .is_ok_and(|file_type| file_type.is_dir()) - || entries.next().is_some() - { - return false; + match std::fs::metadata(codex_home) { + Ok(metadata) if !metadata.is_dir() => return false, + Ok(_) => { + for state_file in ["auth.json", "config.toml", "environments.toml"] { + if !matches!(codex_home.join(state_file).try_exists(), Ok(false)) { + return false; + } } - let Ok(mut entries) = std::fs::read_dir(temporary_root.path()) else { + let Ok(daemon_socket) = + codex_app_server_client::app_server_control_socket_path(codex_home) + else { return false; }; - let Some(Ok(arg0_root)) = entries.next() else { + if !matches!(daemon_socket.as_path().try_exists(), Ok(false)) { return false; - }; - arg0_root.file_name() == "arg0" - && arg0_root - .file_type() - .is_ok_and(|file_type| file_type.is_dir()) - && entries.next().is_none() + } } - }; + Err(error) + if error.kind() == io::ErrorKind::NotFound + && matches!(codex_home.try_exists(), Ok(false)) => {} + Err(_) => return false, + } - pristine_home && matches!(managed_configuration(), Ok(false)) + matches!(managed_configuration(), Ok(false)) } #[cfg(test)] diff --git a/codex-rs/tui/src/startup_preflight_tests.rs b/codex-rs/tui/src/startup_preflight_tests.rs index 100d10f67a..726277cd1f 100644 --- a/codex-rs/tui/src/startup_preflight_tests.rs +++ b/codex-rs/tui/src/startup_preflight_tests.rs @@ -1,10 +1,11 @@ use codex_utils_absolute_path::AbsolutePathBuf; use tempfile::TempDir; +use super::has_only_search_config_override; use super::should_delay_startup_composer_for_first_login; #[test] -fn startup_delays_composer_only_for_pristine_default_homes() -> std::io::Result<()> { +fn startup_delays_composer_for_homes_without_authentication_state() -> std::io::Result<()> { let temporary_directory = TempDir::new()?; let codex_home = temporary_directory.path().join("codex-home"); let system_config_path = @@ -20,7 +21,7 @@ fn startup_delays_composer_only_for_pristine_default_homes() -> std::io::Result< &codex_home, Ok(system_config_path.clone()), || Ok(false), - |name| (name == codex_login::CODEX_ACCESS_TOKEN_ENV_VAR).then(|| " ".to_string()), + |name| (name == codex_login::CODEX_ACCESS_TOKEN_ENV_VAR).then(|| " ".into()), )); std::fs::create_dir_all(codex_home.join("tmp").join("arg0"))?; @@ -43,19 +44,17 @@ fn startup_delays_composer_only_for_pristine_default_homes() -> std::io::Result< |_| None, )); - assert!(!should_delay_startup_composer_for_first_login( + assert!(should_delay_startup_composer_for_first_login( &codex_home, Ok(system_config_path.clone()), || Ok(false), - |name| (name == "CODEX_HOME").then(|| "/custom/home".to_string()), + |name| (name == "CODEX_HOME").then(|| "/custom/home".into()), )); assert!(!should_delay_startup_composer_for_first_login( &codex_home, Ok(system_config_path.clone()), || Ok(false), - |name| { - (name == codex_login::CODEX_ACCESS_TOKEN_ENV_VAR).then(|| "access-token".to_string()) - }, + |name| { (name == codex_login::CODEX_ACCESS_TOKEN_ENV_VAR).then(|| "access-token".into()) }, )); for disabled_credential in [ codex_login::OPENAI_API_KEY_ENV_VAR, @@ -65,17 +64,48 @@ fn startup_delays_composer_only_for_pristine_default_homes() -> std::io::Result< &codex_home, Ok(system_config_path.clone()), || Ok(false), - |name| (name == disabled_credential).then(|| "disabled-key".to_string()), + |name| (name == disabled_credential).then(|| "disabled-key".into()), )); } - for existing_state in ["auth.json", "config.toml", "history.jsonl", "sessions"] { + for existing_state in ["history.jsonl", "log", "sessions"] { + let state_path = codex_home.join(existing_state); + if existing_state == "history.jsonl" { + std::fs::write(&state_path, "")?; + } else { + std::fs::create_dir(&state_path)?; + } + assert!(should_delay_startup_composer_for_first_login( + &codex_home, + Ok(system_config_path.clone()), + || Ok(false), + |_| None, + )); + } + assert!(should_delay_startup_composer_for_first_login( + &codex_home, + Ok(system_config_path.clone()), + || Ok(false), + |name| (name == "CODEX_HOME").then(|| "/custom/home".into()), + )); + assert!(!should_delay_startup_composer_for_first_login( + &codex_home, + Ok(system_config_path.clone()), + || panic!("process credentials should not probe managed configuration"), + |name| match name { + "CODEX_HOME" => Some("/custom/home".into()), + codex_login::CODEX_ACCESS_TOKEN_ENV_VAR => Some("access-token".into()), + _ => None, + }, + )); + + for existing_state in ["auth.json", "config.toml", "environments.toml"] { let state_path = codex_home.join(existing_state); std::fs::write(&state_path, "")?; assert!(!should_delay_startup_composer_for_first_login( &codex_home, Ok(system_config_path.clone()), - || Ok(false), + || panic!("configured homes should not probe managed configuration"), |_| None, )); std::fs::remove_file(state_path)?; @@ -83,11 +113,17 @@ fn startup_delays_composer_only_for_pristine_default_homes() -> std::io::Result< let daemon_directory = codex_home.join("app-server-control"); std::fs::create_dir(&daemon_directory)?; + assert!(should_delay_startup_composer_for_first_login( + &codex_home, + Ok(system_config_path.clone()), + || Ok(false), + |_| None, + )); std::fs::write(daemon_directory.join("app-server-control.sock"), "")?; assert!(!should_delay_startup_composer_for_first_login( &codex_home, Ok(system_config_path.clone()), - || panic!("existing homes should not probe managed configuration"), + || panic!("daemon-owned homes should not probe managed configuration"), |_| None, )); std::fs::remove_file(daemon_directory.join("app-server-control.sock"))?; @@ -95,7 +131,7 @@ fn startup_delays_composer_only_for_pristine_default_homes() -> std::io::Result< let additional_temporary_state = codex_home.join("tmp").join("other"); std::fs::write(&additional_temporary_state, "")?; - assert!(!should_delay_startup_composer_for_first_login( + assert!(should_delay_startup_composer_for_first_login( &codex_home, Ok(system_config_path.clone()), || Ok(false), @@ -121,23 +157,105 @@ fn startup_keeps_composer_when_home_state_cannot_be_confirmed() -> std::io::Resu AbsolutePathBuf::from_absolute_path(temporary_directory.path().join("system.toml"))?; std::fs::create_dir(&codex_home)?; - assert!(!should_delay_startup_composer_for_first_login( - &codex_home, - Ok(system_config_path.clone()), - || Ok(false), - |_| None, - )); - - std::fs::write(codex_home.join("tmp"), "not a directory")?; - assert!(!should_delay_startup_composer_for_first_login( + assert!(should_delay_startup_composer_for_first_login( &codex_home, Ok(system_config_path), || Ok(false), |_| None, )); + + #[cfg(unix)] + { + let system_config_path = + AbsolutePathBuf::from_absolute_path(temporary_directory.path().join("system.toml"))?; + let daemon_directory = codex_home.join("app-server-control"); + std::fs::write(&daemon_directory, "not a directory")?; + assert!(!should_delay_startup_composer_for_first_login( + &codex_home, + Ok(system_config_path.clone()), + || panic!("ambiguous daemon state should not probe managed configuration"), + |_| None, + )); + std::fs::remove_file(&daemon_directory)?; + + let blocking_parent = temporary_directory.path().join("blocking-home-parent"); + std::fs::write(&blocking_parent, "not a directory")?; + assert!(!should_delay_startup_composer_for_first_login( + &blocking_parent.join("codex-home"), + Ok(system_config_path), + || panic!("ambiguous home state should not probe managed configuration"), + |_| None, + )); + } Ok(()) } +#[test] +fn startup_keeps_composer_for_workload_identity_markers() -> std::io::Result<()> { + let temporary_directory = TempDir::new()?; + let codex_home = temporary_directory.path().join("codex-home"); + let system_config_path = + AbsolutePathBuf::from_absolute_path(temporary_directory.path().join("system.toml"))?; + + for marker in [ + codex_protocol::shell_environment::OPENAI_FEDERATION_RULE_ID_ENV_VAR, + codex_protocol::shell_environment::OPENAI_IDENTITY_TOKEN_FILE_ENV_VAR, + ] { + for value in ["", "configured"] { + assert!(!should_delay_startup_composer_for_first_login( + &codex_home, + Ok(system_config_path.clone()), + || panic!("workload identity should not probe managed configuration"), + |name| (name == marker).then(|| value.into()), + )); + } + } + + #[cfg(unix)] + { + use std::os::unix::ffi::OsStringExt; + + assert!(!should_delay_startup_composer_for_first_login( + &codex_home, + Ok(system_config_path), + || panic!("workload identity should not probe managed configuration"), + |name| { + (name == codex_protocol::shell_environment::OPENAI_IDENTITY_TOKEN_FILE_ENV_VAR) + .then(|| std::ffi::OsString::from_vec(vec![b'/', 0xff])) + }, + )); + } + Ok(()) +} + +#[test] +fn startup_only_accepts_the_synthetic_search_override() { + let search = ( + "web_search".to_string(), + toml::Value::String("live".to_string()), + ); + + assert!(!has_only_search_config_override(&[])); + assert!(has_only_search_config_override(std::slice::from_ref( + &search + ))); + assert!(!has_only_search_config_override(&[( + "web_search".to_string(), + toml::Value::String("cached".to_string()), + )])); + assert!(!has_only_search_config_override(&[( + "model_provider".to_string(), + toml::Value::String("live".to_string()), + )])); + assert!(!has_only_search_config_override(&[ + search, + ( + "model".to_string(), + toml::Value::String("custom".to_string()) + ), + ])); +} + #[test] fn startup_keeps_composer_when_system_configuration_is_possible() -> std::io::Result<()> { let temporary_directory = TempDir::new()?;