From e589fddaec4dc33681dd3e570e9a254abbc8d1b0 Mon Sep 17 00:00:00 2001 From: starr-openai Date: Fri, 17 Apr 2026 15:09:41 -0700 Subject: [PATCH] Make default environment lookup infallible Return concrete default and local environments from EnvironmentManager now that the manager always installs local and default entries. Keep arbitrary ID lookup optional. Co-authored-by: Codex --- codex-rs/app-server-client/src/lib.rs | 2 +- .../app-server/src/codex_message_processor.rs | 9 ++-- codex-rs/core/src/session/mod.rs | 6 +-- codex-rs/core/src/session/session.rs | 2 +- codex-rs/core/src/thread_manager.rs | 6 +-- codex-rs/exec-server/src/environment.rs | 52 ++++++++----------- codex-rs/tui/src/lib.rs | 4 +- 7 files changed, 34 insertions(+), 47 deletions(-) diff --git a/codex-rs/app-server-client/src/lib.rs b/codex-rs/app-server-client/src/lib.rs index 49ed5256f0..b767a3c50d 100644 --- a/codex-rs/app-server-client/src/lib.rs +++ b/codex-rs/app-server-client/src/lib.rs @@ -2006,7 +2006,7 @@ mod tests { runtime_args .environment_manager .default_environment() - .is_some_and(|environment| environment.is_remote()) + .is_remote() ); } diff --git a/codex-rs/app-server/src/codex_message_processor.rs b/codex-rs/app-server/src/codex_message_processor.rs index 08fb559d28..66547e1a9c 100644 --- a/codex-rs/app-server/src/codex_message_processor.rs +++ b/codex-rs/app-server/src/codex_message_processor.rs @@ -5698,8 +5698,7 @@ impl CodexMessageProcessor { let environment = self .thread_manager .environment_manager() - .default_environment() - .unwrap_or_else(|| Arc::new(codex_exec_server::Environment::default())); + .default_environment(); // Status listing has no turn cwd. This fallback is used only // by executor-backed stdio MCPs whose config omits `cwd`. McpRuntimeEnvironment::new(environment, config.cwd.to_path_buf()) @@ -5856,8 +5855,7 @@ impl CodexMessageProcessor { let environment = self .thread_manager .environment_manager() - .default_environment() - .unwrap_or_else(|| Arc::new(codex_exec_server::Environment::default())); + .default_environment(); // Resource reads without a thread have no turn cwd. This fallback // is used only by executor-backed stdio MCPs whose config omits `cwd`. McpRuntimeEnvironment::new(environment, config.cwd.to_path_buf()) @@ -6451,7 +6449,8 @@ impl CodexMessageProcessor { .thread_manager .environment_manager() .default_environment() - .map(|environment| environment.get_filesystem()); + .get_filesystem(); + let fs = Some(fs); let mut data = Vec::new(); for cwd in cwds { let extra_roots = extra_roots_by_cwd diff --git a/codex-rs/core/src/session/mod.rs b/codex-rs/core/src/session/mod.rs index ad392efda0..fcc66da7f4 100644 --- a/codex-rs/core/src/session/mod.rs +++ b/codex-rs/core/src/session/mod.rs @@ -461,9 +461,7 @@ impl Codex { let (tx_event, rx_event) = async_channel::unbounded(); let environment = environment_manager.default_environment(); - let fs = environment - .as_ref() - .map(|environment| environment.get_filesystem()); + let fs = Some(environment.get_filesystem()); let plugin_outcome = plugins_manager.plugins_for_config(&config).await; let effective_skill_roots = plugin_outcome.effective_skill_roots(); let skills_input = skills_load_input_from_config(&config, effective_skill_roots); @@ -513,7 +511,7 @@ impl Codex { } let user_instructions = AgentsMdManager::new(&config) - .user_instructions(environment.as_deref()) + .user_instructions(Some(environment.as_ref())) .await; let exec_policy = if crate::guardian::is_guardian_reviewer_source(&session_source) { diff --git a/codex-rs/core/src/session/session.rs b/codex-rs/core/src/session/session.rs index 94cf8ae3f8..1c5c1c9567 100644 --- a/codex-rs/core/src/session/session.rs +++ b/codex-rs/core/src/session/session.rs @@ -698,7 +698,7 @@ impl Session { config.js_repl_node_path.clone(), ), environment_manager, - environment, + environment: Some(environment), allows_agent_environment_access, }; services diff --git a/codex-rs/core/src/thread_manager.rs b/codex-rs/core/src/thread_manager.rs index c5da648bf5..da109bbcc7 100644 --- a/codex-rs/core/src/thread_manager.rs +++ b/codex-rs/core/src/thread_manager.rs @@ -923,8 +923,8 @@ impl ThreadManagerState { user_shell_override: Option, ) -> CodexResult { let environment = self.environment_manager.default_environment(); - let watch_registration = match environment.as_ref() { - Some(environment) if !environment.is_remote() => { + let watch_registration = match environment.is_remote() { + false => { self.skills_watcher .register_config( &config, @@ -934,7 +934,7 @@ impl ThreadManagerState { ) .await } - Some(_) | None => crate::file_watcher::WatchRegistration::default(), + true => crate::file_watcher::WatchRegistration::default(), }; let CodexSpawnOk { codex, thread_id, .. diff --git a/codex-rs/exec-server/src/environment.rs b/codex-rs/exec-server/src/environment.rs index 4cfcd6f9c0..634b2e7c79 100644 --- a/codex-rs/exec-server/src/environment.rs +++ b/codex-rs/exec-server/src/environment.rs @@ -23,7 +23,7 @@ pub const CODEX_EXEC_SERVER_URL_ENV_VAR: &str = "CODEX_EXEC_SERVER_URL"; /// separately tracking whether model-facing tools may access environments. #[derive(Debug)] pub struct EnvironmentManager { - default_environment: Option, + default_environment: String, environment_disabled_for_agent: bool, environments: HashMap>, } @@ -124,9 +124,9 @@ impl EnvironmentManager { .expect("valid remote environment"), ), ); - Some(REMOTE_ENVIRONMENT_ID.to_string()) + REMOTE_ENVIRONMENT_ID.to_string() } - None => Some(LOCAL_ENVIRONMENT_ID.to_string()), + None => LOCAL_ENVIRONMENT_ID.to_string(), }; Self { @@ -139,22 +139,19 @@ impl EnvironmentManager { /// Returns true when model-facing tools may access an environment. pub fn allows_agent_environment_access(&self) -> bool { !self.environment_disabled_for_agent - && self - .default_environment - .as_deref() - .is_some_and(|environment_id| self.environments.contains_key(environment_id)) + && self.environments.contains_key(&self.default_environment) } /// Returns the default environment instance. - pub fn default_environment(&self) -> Option> { - self.default_environment - .as_deref() - .and_then(|environment_id| self.get_environment(environment_id)) + pub fn default_environment(&self) -> Arc { + self.get_environment(&self.default_environment) + .expect("default environment exists") } /// Returns the local environment instance. - pub fn local_environment(&self) -> Option> { + pub fn local_environment(&self) -> Arc { self.get_environment(LOCAL_ENVIRONMENT_ID) + .expect("local environment exists") } /// Returns a named environment instance. @@ -297,10 +294,10 @@ mod tests { local_runtime_paths: None, }); - let environment = manager.default_environment().expect("local environment"); + let environment = manager.default_environment(); assert!(!environment.is_remote()); assert!(manager.allows_agent_environment_access()); - assert!(manager.local_environment().is_some()); + assert!(!manager.local_environment().is_remote()); assert!(manager.get_environment(REMOTE_ENVIRONMENT_ID).is_none()); } @@ -312,8 +309,8 @@ mod tests { }); assert!(!manager.allows_agent_environment_access()); - assert!(manager.default_environment().is_some()); - assert!(manager.local_environment().is_some()); + assert!(!manager.default_environment().is_remote()); + assert!(!manager.local_environment().is_remote()); assert!(manager.get_environment(REMOTE_ENVIRONMENT_ID).is_none()); } @@ -324,16 +321,11 @@ mod tests { local_runtime_paths: None, }); - let environment = manager.default_environment().expect("remote environment"); + let environment = manager.default_environment(); assert!(environment.is_remote()); assert!(manager.allows_agent_environment_access()); assert_eq!(environment.exec_server_url(), Some("ws://127.0.0.1:8765")); - assert!( - !manager - .local_environment() - .expect("local environment") - .is_remote() - ); + assert!(!manager.local_environment().is_remote()); assert_eq!( manager .get_environment(REMOTE_ENVIRONMENT_ID) @@ -347,8 +339,8 @@ mod tests { async fn environment_manager_default_environment_caches_environment() { let manager = EnvironmentManager::new(EnvironmentManagerArgs::default()); - let first = manager.default_environment().expect("local environment"); - let second = manager.default_environment().expect("local environment"); + let first = manager.default_environment(); + let second = manager.default_environment(); assert!(Arc::ptr_eq(&first, &second)); } @@ -365,14 +357,14 @@ mod tests { local_runtime_paths: Some(runtime_paths.clone()), }); - let environment = manager.default_environment().expect("local environment"); + let environment = manager.default_environment(); assert_eq!(environment.local_runtime_paths(), Some(&runtime_paths)); let manager = EnvironmentManager::new(EnvironmentManagerArgs { exec_server_url: environment.exec_server_url().map(str::to_owned), local_runtime_paths: environment.local_runtime_paths().cloned(), }); - let environment = manager.default_environment().expect("local environment"); + let environment = manager.default_environment(); assert_eq!(environment.local_runtime_paths(), Some(&runtime_paths)); } @@ -383,7 +375,7 @@ mod tests { local_runtime_paths: None, }); - assert!(manager.default_environment().is_some()); + assert!(!manager.default_environment().is_remote()); assert!(!manager.allows_agent_environment_access()); } @@ -394,9 +386,9 @@ mod tests { local_runtime_paths: None, }); - assert!(manager.default_environment().is_some()); + assert!(!manager.default_environment().is_remote()); assert!(!manager.allows_agent_environment_access()); - assert!(manager.local_environment().is_some()); + assert!(!manager.local_environment().is_remote()); assert!(manager.get_environment(REMOTE_ENVIRONMENT_ID).is_none()); } diff --git a/codex-rs/tui/src/lib.rs b/codex-rs/tui/src/lib.rs index bbc6df14bb..10ba55de80 100644 --- a/codex-rs/tui/src/lib.rs +++ b/codex-rs/tui/src/lib.rs @@ -625,9 +625,7 @@ fn config_cwd_for_app_server_target( app_server_target: &AppServerTarget, environment_manager: &EnvironmentManager, ) -> std::io::Result> { - if environment_manager - .default_environment() - .is_some_and(|environment| environment.is_remote()) + if environment_manager.default_environment().is_remote() || matches!(app_server_target, AppServerTarget::Remote { .. }) { return Ok(None);