From 5c70e25b03ad1efd36f29e1d646424f69ee0dd44 Mon Sep 17 00:00:00 2001 From: Eric Traut Date: Mon, 30 Mar 2026 16:39:54 -0600 Subject: [PATCH] codex: address PR review feedback (#16274) --- .../app-server/src/codex_message_processor.rs | 58 +++++++++--- .../tests/suite/v2/personalities_list.rs | 88 ++++++++++++++++--- 2 files changed, 121 insertions(+), 25 deletions(-) diff --git a/codex-rs/app-server/src/codex_message_processor.rs b/codex-rs/app-server/src/codex_message_processor.rs index 1bc9436360..b24c4c5a14 100644 --- a/codex-rs/app-server/src/codex_message_processor.rs +++ b/codex-rs/app-server/src/codex_message_processor.rs @@ -420,6 +420,8 @@ pub(crate) struct CodexMessageProcessor { command_exec_manager: CommandExecManager, pending_fuzzy_searches: Arc>>>, fuzzy_search_sessions: Arc>>, + personality_catalogs: + Arc>>, background_tasks: TaskTracker, feedback: CodexFeedback, log_db: Option, @@ -536,6 +538,10 @@ impl CodexMessageProcessor { feedback, log_db, } = args; + let personality_catalogs = HashMap::from([( + config.cwd.to_path_buf(), + codex_core::personalities::catalog_for_config(config.as_ref()), + )]); Self { auth_manager, thread_manager, @@ -553,6 +559,7 @@ impl CodexMessageProcessor { command_exec_manager: CommandExecManager::default(), pending_fuzzy_searches: Arc::new(Mutex::new(HashMap::new())), fuzzy_search_sessions: Arc::new(Mutex::new(HashMap::new())), + personality_catalogs: Arc::new(Mutex::new(personality_catalogs)), background_tasks: TaskTracker::new(), feedback, log_db, @@ -603,6 +610,39 @@ impl CodexMessageProcessor { .unwrap_or_default() } + async fn personality_catalog_for_cwd( + &self, + cwd: &AbsolutePathBuf, + cli_overrides: &[(String, TomlValue)], + ) -> Result { + if let Some(catalog) = self + .personality_catalogs + .lock() + .await + .get(cwd.as_path()) + .cloned() + { + return Ok(catalog); + } + + let config_layer_stack = load_config_layers_state( + &self.config.codex_home, + Some(cwd.clone()), + cli_overrides, + LoaderOverrides::default(), + CloudRequirementsLoader::default(), + ) + .await + .map_err(|err| config_load_error(&err))?; + let catalog = codex_core::personalities::catalog_from_layer_stack(&config_layer_stack); + + let mut personality_catalogs = self.personality_catalogs.lock().await; + Ok(personality_catalogs + .entry(cwd.to_path_buf()) + .or_insert_with(|| catalog.clone()) + .clone()) + } + /// If a client sends `developer_instructions: null` during a mode switch, /// use the built-in instructions for that mode. fn normalize_turn_start_collaboration_mode( @@ -5819,24 +5859,16 @@ impl CodexMessageProcessor { return; } }; - let config_layer_stack = match load_config_layers_state( - &self.config.codex_home, - Some(cwd_abs), - &cli_overrides, - LoaderOverrides::default(), - CloudRequirementsLoader::default(), - ) - .await + let catalog = match self + .personality_catalog_for_cwd(&cwd_abs, &cli_overrides) + .await { - Ok(config_layer_stack) => config_layer_stack, + Ok(catalog) => catalog, Err(err) => { - self.outgoing - .send_error(request_id, config_load_error(&err)) - .await; + self.outgoing.send_error(request_id, err).await; return; } }; - let catalog = codex_core::personalities::catalog_from_layer_stack(&config_layer_stack); data.push(codex_app_server_protocol::PersonalitiesListEntry { cwd, personalities: personalities_to_info(catalog.personalities()), diff --git a/codex-rs/app-server/tests/suite/v2/personalities_list.rs b/codex-rs/app-server/tests/suite/v2/personalities_list.rs index cf0d475de4..4ad9a41715 100644 --- a/codex-rs/app-server/tests/suite/v2/personalities_list.rs +++ b/codex-rs/app-server/tests/suite/v2/personalities_list.rs @@ -29,6 +29,24 @@ fn write_personality( Ok(()) } +async fn request_personalities_list( + mcp: &mut McpProcess, + cwd: &std::path::Path, +) -> Result { + let request_id = mcp + .send_personalities_list_request(PersonalitiesListParams { + cwds: Some(vec![cwd.to_path_buf().try_into()?]), + }) + .await?; + + let response: JSONRPCResponse = timeout( + DEFAULT_TIMEOUT, + mcp.read_stream_until_response_message(RequestId::Integer(request_id)), + ) + .await??; + to_response(response) +} + #[tokio::test] async fn personalities_list_returns_builtin_and_file_backed_personalities() -> Result<()> { let codex_home = TempDir::new()?; @@ -62,18 +80,8 @@ async fn personalities_list_returns_builtin_and_file_backed_personalities() -> R let mut mcp = McpProcess::new(codex_home.path()).await?; timeout(DEFAULT_TIMEOUT, mcp.initialize()).await??; - let request_id = mcp - .send_personalities_list_request(PersonalitiesListParams { - cwds: Some(vec![repo.path().to_path_buf().try_into()?]), - }) - .await?; - - let response: JSONRPCResponse = timeout( - DEFAULT_TIMEOUT, - mcp.read_stream_until_response_message(RequestId::Integer(request_id)), - ) - .await??; - let PersonalitiesListResponse { data } = to_response(response)?; + let PersonalitiesListResponse { data } = + request_personalities_list(&mut mcp, repo.path()).await?; assert_eq!(data.len(), 1); assert_eq!(data[0].cwd, repo.path().to_path_buf()); @@ -133,3 +141,59 @@ async fn personalities_list_returns_builtin_and_file_backed_personalities() -> R ); Ok(()) } + +#[tokio::test] +async fn personalities_list_uses_cached_catalog_for_repeated_requests() -> Result<()> { + let codex_home = TempDir::new()?; + let repo = TempDir::new()?; + let repo_personalities = repo.path().join(".codex/personalities"); + + write_personality( + &repo_personalities, + "night-owl", + "Original personality", + "Original instructions", + )?; + + let mut mcp = McpProcess::new(codex_home.path()).await?; + timeout(DEFAULT_TIMEOUT, mcp.initialize()).await??; + + let first = request_personalities_list(&mut mcp, repo.path()).await?; + let first_entry = &first.data[0]; + let first_night_owl = first_entry + .personalities + .iter() + .find(|personality| personality.name == "night-owl") + .ok_or_else(|| anyhow!("night-owl personality missing on first request"))?; + assert_eq!(first_night_owl.description, "Original personality"); + + write_personality( + &repo_personalities, + "night-owl", + "Updated personality", + "Updated instructions", + )?; + write_personality( + &repo_personalities, + "fresh-file", + "New personality", + "New instructions", + )?; + + let second = request_personalities_list(&mut mcp, repo.path()).await?; + let second_entry = &second.data[0]; + let second_night_owl = second_entry + .personalities + .iter() + .find(|personality| personality.name == "night-owl") + .ok_or_else(|| anyhow!("night-owl personality missing on second request"))?; + assert_eq!(second_night_owl.description, "Original personality"); + assert!( + second_entry + .personalities + .iter() + .all(|personality| personality.name != "fresh-file") + ); + + Ok(()) +}