diff --git a/codex-rs/tui/src/agent_name.rs b/codex-rs/tui/src/agent_name.rs new file mode 100644 index 0000000000..2092301720 --- /dev/null +++ b/codex-rs/tui/src/agent_name.rs @@ -0,0 +1,46 @@ +use std::process::Command; + +const AGENT_PERSONA_ENV: &str = "AGENT_PERSONA"; + +/// Resolve the pane-local agent name used by avatar and voice integrations. +/// +/// The process-scoped environment identity takes precedence so avatar and voice +/// integrations observe the same agent. Director remains the standalone +/// fallback when that environment identity is unavailable or invalid. +pub(crate) fn resolve() -> Option { + let environment_name = std::env::var(AGENT_PERSONA_ENV).ok(); + resolve_from_sources(environment_name.as_deref(), resolve_director_agent_name) +} + +fn resolve_director_agent_name() -> Option { + let output = Command::new("director") + .args(["whoami", "--name"]) + .output() + .ok()?; + if !output.status.success() { + return None; + } + String::from_utf8(output.stdout).ok() +} + +fn resolve_from_sources(environment_name: Option<&str>, director_name: F) -> Option +where + F: FnOnce() -> Option, +{ + environment_name + .and_then(normalize) + .or_else(|| director_name().as_deref().and_then(normalize)) +} + +fn normalize(name: &str) -> Option { + let name = name.trim(); + (!name.is_empty() + && name + .bytes() + .all(|byte| byte.is_ascii_alphanumeric() || matches!(byte, b'.' | b'_' | b'-'))) + .then(|| name.to_string()) +} + +#[cfg(test)] +#[path = "agent_name_tests.rs"] +mod tests; diff --git a/codex-rs/tui/src/agent_name_tests.rs b/codex-rs/tui/src/agent_name_tests.rs new file mode 100644 index 0000000000..0c773d6e35 --- /dev/null +++ b/codex-rs/tui/src/agent_name_tests.rs @@ -0,0 +1,40 @@ +use std::cell::Cell; + +use pretty_assertions::assert_eq; + +use super::*; + +#[test] +fn environment_identity_wins_without_querying_director() { + let director_called = Cell::new(false); + + let resolved = resolve_from_sources(Some(" hai-os\n"), || { + director_called.set(true); + Some("chloe".to_string()) + }); + + assert_eq!( + (resolved, director_called.get()), + (Some("hai-os".to_string()), false) + ); +} + +#[test] +fn director_identity_is_used_when_environment_identity_is_unavailable() { + assert_eq!( + resolve_from_sources(None, || Some(" centurion\n".to_string())), + Some("centurion".to_string()) + ); + assert_eq!( + resolve_from_sources(Some("not a name"), || Some("chloe".to_string())), + Some("chloe".to_string()) + ); +} + +#[test] +fn invalid_names_are_ignored() { + assert_eq!( + resolve_from_sources(Some("not a name"), || Some("also/bad".to_string())), + None + ); +} diff --git a/codex-rs/tui/src/avatars/binding.rs b/codex-rs/tui/src/avatars/binding.rs index cef2e0fcc6..441c9f32b8 100644 --- a/codex-rs/tui/src/avatars/binding.rs +++ b/codex-rs/tui/src/avatars/binding.rs @@ -30,6 +30,43 @@ pub(crate) fn resolve_named_avatar_binding( binding_from_resolved_character(&resolved) } +pub(crate) fn resolve_startup_avatar_binding( + codex_home: &Path, + explicit_name: Option<&str>, + detected_name: Option<&str>, +) -> Result> { + if let Some(name) = explicit_name { + return resolve_named_avatar_binding(codex_home, name).map(Some); + } + let Some(name) = detected_name else { + return Ok(None); + }; + match resolve_named_avatar_binding(codex_home, name) { + Ok(binding) => Ok(Some(binding)), + Err(_) if !character_exists(codex_home, name) => Ok(None), + Err(err) => Err(err), + } +} + +fn character_exists(codex_home: &Path, name: &str) -> bool { + CharacterCatalog::load(codex_home) + .entries() + .iter() + .any(|report| { + report + .storage_id + .as_deref() + .is_some_and(|id| id.eq_ignore_ascii_case(name)) + || report.id().is_some_and(|id| id.eq_ignore_ascii_case(name)) + || report.manifest.as_ref().is_some_and(|manifest| { + manifest + .aliases + .iter() + .any(|alias| alias.eq_ignore_ascii_case(name)) + }) + }) +} + pub(crate) fn binding_from_resolved_character( resolved: &ResolvedCharacter, ) -> Result { @@ -130,6 +167,33 @@ mod tests { ); } + #[test] + fn unknown_detected_identity_ignores_unrelated_catalog_collisions() { + let home = tempfile::tempdir().unwrap(); + for id in ["one", "two"] { + let package = home.path().join("characters").join(id); + std::fs::create_dir_all(&package).unwrap(); + std::fs::write( + package.join("character.json"), + format!( + r#"{{ + "schemaVersion":1, + "id":"{id}", + "displayName":"{id}", + "aliases":["shared"], + "avatar":"avatar/default/avatar.json" + }}"# + ), + ) + .unwrap(); + } + + let binding = + resolve_startup_avatar_binding(home.path(), None, Some("unknown-agent")).unwrap(); + + assert!(binding.is_none()); + } + #[test] fn every_bundled_character_name_resolves_to_its_own_avatar_binding() { let home = tempfile::tempdir().unwrap(); @@ -197,7 +261,7 @@ mod tests { } #[test] - fn selected_partial_bundled_character_fails_during_named_resolution() { + fn selected_partial_bundled_character_fails_during_detected_resolution() { let home = tempfile::tempdir().unwrap(); super::super::assets::ensure_bundled_avatars(home.path()).unwrap(); std::fs::remove_file( @@ -206,7 +270,7 @@ mod tests { ) .unwrap(); - let error = resolve_named_avatar_binding(home.path(), "chloe").unwrap_err(); + let error = resolve_startup_avatar_binding(home.path(), None, Some("chloe")).unwrap_err(); let detail = format!("{error:#}"); assert!(detail.contains("failed to resolve character \"chloe\"")); diff --git a/codex-rs/tui/src/avatars/mod.rs b/codex-rs/tui/src/avatars/mod.rs index 21c8b0ecb5..4d04354a9f 100644 --- a/codex-rs/tui/src/avatars/mod.rs +++ b/codex-rs/tui/src/avatars/mod.rs @@ -12,7 +12,9 @@ mod runtime; #[allow(unused_imports)] pub(crate) use assets::ensure_bundled_avatars; pub use assets::ensure_bundled_character_for_name; +#[cfg(test)] pub(crate) use binding::resolve_named_avatar_binding; +pub(crate) use binding::resolve_startup_avatar_binding; pub(crate) use runtime::AvatarBinding; pub(crate) use runtime::AvatarPlacement; pub(crate) use runtime::AvatarRuntime; diff --git a/codex-rs/tui/src/lib.rs b/codex-rs/tui/src/lib.rs index 5952a13f6c..3163de0bf3 100644 --- a/codex-rs/tui/src/lib.rs +++ b/codex-rs/tui/src/lib.rs @@ -84,6 +84,7 @@ use uuid::Uuid; pub(crate) use codex_app_server_client::legacy_core; mod additional_dirs; +mod agent_name; mod app; mod app_backtrack; mod app_command; @@ -168,6 +169,7 @@ mod session_state; mod shimmer; mod skills_helpers; mod slash_command; +mod startup_avatar; mod startup_error; mod startup_hooks_review; mod status; @@ -898,12 +900,8 @@ pub async fn run_main( std::process::exit(1); } }; - let avatar_binding = cli - .name - .as_deref() - .map(|name| avatars::resolve_named_avatar_binding(&codex_home, name)) - .transpose() - .map_err(std::io::Error::other)?; + let avatar_binding = + startup_avatar::resolve(&codex_home, cli.name.as_deref()).map_err(std::io::Error::other)?; let mut launch_loader_overrides = loader_overrides.clone(); if let Some(profile_v2) = cli.config_profile_v2.as_ref() { diff --git a/codex-rs/tui/src/pets/talking_signal.rs b/codex-rs/tui/src/pets/talking_signal.rs index 2179c855af..5096413a18 100644 --- a/codex-rs/tui/src/pets/talking_signal.rs +++ b/codex-rs/tui/src/pets/talking_signal.rs @@ -1,8 +1,6 @@ //! Polls the cross-process `say` playback signal for the current agent. use std::path::PathBuf; -#[cfg(not(test))] -use std::process::Command; use std::time::Duration; use std::time::Instant; use std::time::SystemTime; @@ -69,36 +67,7 @@ impl TalkingSignal { } fn resolve_agent_name() -> Option { - std::env::var("AGENT_PERSONA") - .ok() - .filter(|name| valid_agent_name(name)) - .or_else(resolve_director_agent_name) -} - -#[cfg(not(test))] -fn resolve_director_agent_name() -> Option { - let output = Command::new("director") - .args(["whoami", "--name"]) - .output() - .ok()?; - if !output.status.success() { - return None; - } - let name = String::from_utf8(output.stdout).ok()?; - let name = name.trim(); - valid_agent_name(name).then(|| name.to_string()) -} - -#[cfg(test)] -fn resolve_director_agent_name() -> Option { - None -} - -fn valid_agent_name(name: &str) -> bool { - !name.is_empty() - && name - .bytes() - .all(|byte| byte.is_ascii_alphanumeric() || matches!(byte, b'.' | b'_' | b'-')) + crate::agent_name::resolve() } fn flag_is_active(path: &std::path::Path) -> bool { diff --git a/codex-rs/tui/src/pets/talking_signal_tests.rs b/codex-rs/tui/src/pets/talking_signal_tests.rs index 149118ee62..d6a41f97dd 100644 --- a/codex-rs/tui/src/pets/talking_signal_tests.rs +++ b/codex-rs/tui/src/pets/talking_signal_tests.rs @@ -6,14 +6,6 @@ use pretty_assertions::assert_eq; use super::TalkingSignal; use super::flag_is_active; -use super::valid_agent_name; - -#[test] -fn agent_names_are_safe_single_path_components() { - assert_eq!(valid_agent_name("clanker-coder_1.0"), true); - assert_eq!(valid_agent_name("../clanker"), false); - assert_eq!(valid_agent_name("clanker coder"), false); -} #[test] fn fresh_flag_for_live_process_is_active_and_stale_flag_is_not() { diff --git a/codex-rs/tui/src/startup_avatar.rs b/codex-rs/tui/src/startup_avatar.rs new file mode 100644 index 0000000000..4ff6b80af5 --- /dev/null +++ b/codex-rs/tui/src/startup_avatar.rs @@ -0,0 +1,32 @@ +use std::path::Path; + +use anyhow::Result; + +use crate::avatars::AvatarBinding; + +pub(crate) fn resolve( + codex_home: &Path, + explicit_name: Option<&str>, +) -> Result> { + resolve_with(codex_home, explicit_name, crate::agent_name::resolve) +} + +fn resolve_with( + codex_home: &Path, + explicit_name: Option<&str>, + detect_name: F, +) -> Result> +where + F: FnOnce() -> Option, +{ + let detected_name = explicit_name.is_none().then(detect_name).flatten(); + crate::avatars::resolve_startup_avatar_binding( + codex_home, + explicit_name, + detected_name.as_deref(), + ) +} + +#[cfg(test)] +#[path = "startup_avatar_tests.rs"] +mod tests; diff --git a/codex-rs/tui/src/startup_avatar_tests.rs b/codex-rs/tui/src/startup_avatar_tests.rs new file mode 100644 index 0000000000..50eeeb95a2 --- /dev/null +++ b/codex-rs/tui/src/startup_avatar_tests.rs @@ -0,0 +1,52 @@ +use std::cell::Cell; + +use pretty_assertions::assert_eq; + +use super::*; + +#[test] +fn detected_identity_selects_its_bundled_avatar() { + let home = tempfile::tempdir().unwrap(); + + let binding = resolve_with( + home.path(), + /*explicit_name*/ None, + || Some("chloe".to_string()), + ) + .unwrap() + .unwrap(); + + assert_eq!(binding.character_id(), "chloe"); +} + +#[test] +fn explicit_name_wins_without_running_identity_detection() { + let home = tempfile::tempdir().unwrap(); + let detector_called = Cell::new(false); + + let binding = resolve_with(home.path(), Some("centurion"), || { + detector_called.set(true); + Some("chloe".to_string()) + }) + .unwrap() + .unwrap(); + + assert_eq!( + (binding.character_id(), detector_called.get()), + ("centurion", false) + ); +} + +#[test] +fn unknown_detected_identity_does_not_block_startup() { + let home = tempfile::tempdir().unwrap(); + + let binding = resolve_with( + home.path(), + /*explicit_name*/ None, + || Some("unknown-agent".to_string()), + ) + .unwrap(); + + assert!(binding.is_none()); +}