codex: address PR review feedback (#16274)

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<Mutex<HashMap<String, Arc<AtomicBool>>>>,
     fuzzy_search_sessions: Arc<Mutex<HashMap<String, FuzzyFileSearchSession>>>,
+    personality_catalogs:
+        Arc<Mutex<HashMap<PathBuf, codex_core::personalities::PersonalityCatalog>>>,
     background_tasks: TaskTracker,
     feedback: CodexFeedback,
     log_db: Option<LogDbLayer>,
@@ -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<codex_core::personalities::PersonalityCatalog, JSONRPCErrorError> {
+        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()),

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<PersonalitiesListResponse> {
+    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(())
+}