From 566e4cee4bd067aeae95591430cffb71e8ede228 Mon Sep 17 00:00:00 2001 From: Matthew Zeng Date: Mon, 9 Mar 2026 22:25:43 -0700 Subject: [PATCH] [apps] Fix apps enablement condition. (#14011) - [x] Fix apps enablement condition to check both the feature flag and that the user is not an API key user. --- .../app-server/src/codex_message_processor.rs | 4 +- .../app-server/tests/suite/v2/app_list.rs | 64 ++++++ codex-rs/chatgpt/src/connectors.rs | 17 +- codex-rs/core/src/auth.rs | 4 + codex-rs/core/src/codex.rs | 54 ++--- codex-rs/core/src/connectors.rs | 14 +- codex-rs/core/src/features.rs | 38 ++++ codex-rs/core/src/mcp/mod.rs | 2 +- codex-rs/core/src/tools/spec.rs | 82 ++++++- codex-rs/core/tests/suite/client.rs | 74 ++++++- codex-rs/core/tests/suite/plugins.rs | 2 +- codex-rs/core/tests/suite/search_tool.rs | 49 ++++- .../tui/src/bottom_pane/bottom_pane_view.rs | 6 + .../src/bottom_pane/list_selection_view.rs | 4 + codex-rs/tui/src/bottom_pane/mod.rs | 7 + codex-rs/tui/src/chatwidget.rs | 115 +++++++--- ...dget__tests__apps_popup_loading_state.snap | 8 + codex-rs/tui/src/chatwidget/tests.rs | 204 +++++++++++++++++- 18 files changed, 662 insertions(+), 86 deletions(-) create mode 100644 codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__apps_popup_loading_state.snap diff --git a/codex-rs/app-server/src/codex_message_processor.rs b/codex-rs/app-server/src/codex_message_processor.rs index fa9ec8577..01a377969 100644 --- a/codex-rs/app-server/src/codex_message_processor.rs +++ b/codex-rs/app-server/src/codex_message_processor.rs @@ -4855,7 +4855,7 @@ impl CodexMessageProcessor { .set_enabled(Feature::Apps, thread.enabled(Feature::Apps)); } - if !config.features.enabled(Feature::Apps) { + if !config.features.apps_enabled(Some(&self.auth_manager)).await { self.outgoing .send_response( request_id, @@ -5418,7 +5418,7 @@ impl CodexMessageProcessor { }; let plugin_apps = load_plugin_apps(result.installed_path.as_path()); let apps_needing_auth = if plugin_apps.is_empty() - || !config.features.enabled(Feature::Apps) + || !config.features.apps_enabled(Some(&self.auth_manager)).await { Vec::new() } else { diff --git a/codex-rs/app-server/tests/suite/v2/app_list.rs b/codex-rs/app-server/tests/suite/v2/app_list.rs index 9bd654349..a19cdad85 100644 --- a/codex-rs/app-server/tests/suite/v2/app_list.rs +++ b/codex-rs/app-server/tests/suite/v2/app_list.rs @@ -26,6 +26,7 @@ use codex_app_server_protocol::AppReview; use codex_app_server_protocol::AppScreenshot; use codex_app_server_protocol::AppsListParams; use codex_app_server_protocol::AppsListResponse; +use codex_app_server_protocol::AuthMode; use codex_app_server_protocol::JSONRPCError; use codex_app_server_protocol::JSONRPCResponse; use codex_app_server_protocol::RequestId; @@ -33,6 +34,8 @@ use codex_app_server_protocol::ServerNotification; use codex_app_server_protocol::ThreadStartParams; use codex_app_server_protocol::ThreadStartResponse; use codex_core::auth::AuthCredentialsStoreMode; +use codex_core::auth::AuthDotJson; +use codex_core::auth::save_auth; use pretty_assertions::assert_eq; use rmcp::handler::server::ServerHandler; use rmcp::model::JsonObject; @@ -82,6 +85,67 @@ async fn list_apps_returns_empty_when_connectors_disabled() -> Result<()> { Ok(()) } +#[tokio::test] +async fn list_apps_returns_empty_with_api_key_auth() -> Result<()> { + let connectors = vec![AppInfo { + id: "beta".to_string(), + name: "Beta".to_string(), + description: Some("Beta connector".to_string()), + logo_url: None, + logo_url_dark: None, + distribution_channel: None, + branding: None, + app_metadata: None, + labels: None, + install_url: None, + is_accessible: false, + is_enabled: true, + plugin_display_names: Vec::new(), + }]; + let tools = vec![connector_tool("beta", "Beta App")?]; + let (server_url, server_handle) = + start_apps_server_with_delays(connectors, tools, Duration::ZERO, Duration::ZERO).await?; + + let codex_home = TempDir::new()?; + write_connectors_config(codex_home.path(), &server_url)?; + save_auth( + codex_home.path(), + &AuthDotJson { + auth_mode: Some(AuthMode::ApiKey), + openai_api_key: Some("test-api-key".to_string()), + tokens: None, + last_refresh: None, + }, + AuthCredentialsStoreMode::File, + )?; + + let mut mcp = McpProcess::new(codex_home.path()).await?; + timeout(DEFAULT_TIMEOUT, mcp.initialize()).await??; + + let request_id = mcp + .send_apps_list_request(AppsListParams { + limit: Some(50), + cursor: None, + thread_id: None, + force_refetch: false, + }) + .await?; + + let response: JSONRPCResponse = timeout( + DEFAULT_TIMEOUT, + mcp.read_stream_until_response_message(RequestId::Integer(request_id)), + ) + .await??; + + let AppsListResponse { data, next_cursor } = to_response(response)?; + assert!(data.is_empty()); + assert!(next_cursor.is_none()); + + server_handle.abort(); + let _ = server_handle.await; + Ok(()) +} + #[tokio::test] async fn list_apps_uses_thread_feature_flag_when_thread_id_is_provided() -> Result<()> { let connectors = vec![AppInfo { diff --git a/codex-rs/chatgpt/src/connectors.rs b/codex-rs/chatgpt/src/connectors.rs index 2dfe6671a..dfc05fe31 100644 --- a/codex-rs/chatgpt/src/connectors.rs +++ b/codex-rs/chatgpt/src/connectors.rs @@ -3,8 +3,8 @@ use std::collections::HashSet; use std::sync::LazyLock; use std::sync::Mutex as StdMutex; +use codex_core::AuthManager; use codex_core::config::Config; -use codex_core::features::Feature; use codex_core::token_data::TokenData; use serde::Deserialize; use std::time::Duration; @@ -75,8 +75,17 @@ struct CachedAllConnectors { static ALL_CONNECTORS_CACHE: LazyLock>> = LazyLock::new(|| StdMutex::new(None)); +async fn apps_enabled(config: &Config) -> bool { + let auth_manager = AuthManager::shared( + config.codex_home.clone(), + false, + config.cli_auth_credentials_store_mode, + ); + config.features.apps_enabled(Some(&auth_manager)).await +} + pub async fn list_connectors(config: &Config) -> anyhow::Result> { - if !config.features.enabled(Feature::Apps) { + if !apps_enabled(config).await { return Ok(Vec::new()); } let (connectors_result, accessible_result) = tokio::join!( @@ -96,7 +105,7 @@ pub async fn list_all_connectors(config: &Config) -> anyhow::Result } pub async fn list_cached_all_connectors(config: &Config) -> Option> { - if !config.features.enabled(Feature::Apps) { + if !apps_enabled(config).await { return Some(Vec::new()); } @@ -118,7 +127,7 @@ pub async fn list_all_connectors_with_options( config: &Config, force_refetch: bool, ) -> anyhow::Result> { - if !config.features.enabled(Feature::Apps) { + if !apps_enabled(config).await { return Ok(Vec::new()); } init_chatgpt_token_from_auth(&config.codex_home, config.cli_auth_credentials_store_mode) diff --git a/codex-rs/core/src/auth.rs b/codex-rs/core/src/auth.rs index 546d53b79..ddce81b24 100644 --- a/codex-rs/core/src/auth.rs +++ b/codex-rs/core/src/auth.rs @@ -212,6 +212,10 @@ impl CodexAuth { } } + pub fn is_api_key_auth(&self) -> bool { + self.auth_mode() == AuthMode::ApiKey + } + pub fn is_chatgpt_auth(&self) -> bool { self.auth_mode() == AuthMode::Chatgpt } diff --git a/codex-rs/core/src/codex.rs b/codex-rs/core/src/codex.rs index f5132fabb..6d5a43d34 100644 --- a/codex-rs/core/src/codex.rs +++ b/codex-rs/core/src/codex.rs @@ -736,6 +736,11 @@ impl TurnContext { }) } + pub(crate) fn apps_enabled(&self) -> bool { + self.features + .apps_enabled_cached(self.auth_manager.as_deref()) + } + pub(crate) async fn with_model(&self, model: String, models_manager: &ModelsManager) -> Self { let mut config = (*self.config).clone(); config.model = Some(model.clone()); @@ -3407,7 +3412,7 @@ impl Session { ); } } - if turn_context.features.enabled(Feature::Apps) { + if turn_context.apps_enabled() { developer_sections.push(render_apps_section()); } if turn_context.features.enabled(Feature::CodexGitCommit) @@ -3894,7 +3899,7 @@ impl Session { .tool_plugin_provenance(config.as_ref()); let mcp_servers = with_codex_apps_mcp( mcp_servers, - self.features.enabled(Feature::Apps), + self.features.apps_enabled_for_auth(auth.as_ref()), auth.as_ref(), config.as_ref(), ); @@ -5357,28 +5362,27 @@ pub(crate) async fn run_turn( // enabled plugins, then converted into turn-scoped guidance below. let mentioned_plugins = collect_explicit_plugin_mentions(&input, loaded_plugins.capability_summaries()); - let mcp_tools = - if turn_context.config.features.enabled(Feature::Apps) || !mentioned_plugins.is_empty() { - // Plugin mentions need raw MCP/app inventory even when app tools - // are normally hidden so we can describe the plugin's currently - // usable capabilities for this turn. - match sess - .services - .mcp_connection_manager - .read() - .await - .list_all_tools() - .or_cancel(&cancellation_token) - .await - { - Ok(mcp_tools) => mcp_tools, - Err(_) if turn_context.config.features.enabled(Feature::Apps) => return None, - Err(_) => HashMap::new(), - } - } else { - HashMap::new() - }; - let available_connectors = if turn_context.config.features.enabled(Feature::Apps) { + let mcp_tools = if turn_context.apps_enabled() || !mentioned_plugins.is_empty() { + // Plugin mentions need raw MCP/app inventory even when app tools + // are normally hidden so we can describe the plugin's currently + // usable capabilities for this turn. + match sess + .services + .mcp_connection_manager + .read() + .await + .list_all_tools() + .or_cancel(&cancellation_token) + .await + { + Ok(mcp_tools) => mcp_tools, + Err(_) if turn_context.apps_enabled() => return None, + Err(_) => HashMap::new(), + } + } else { + HashMap::new() + }; + let available_connectors = if turn_context.apps_enabled() { let connectors = connectors::merge_plugin_apps_with_accessible( loaded_plugins.effective_apps(), connectors::accessible_connectors_from_mcp_tools(&mcp_tools), @@ -6234,7 +6238,7 @@ async fn built_tools( let mut effective_explicitly_enabled_connectors = explicitly_enabled_connectors.clone(); effective_explicitly_enabled_connectors.extend(sess.get_connector_selection().await); - let connectors = if turn_context.features.enabled(Feature::Apps) { + let connectors = if turn_context.apps_enabled() { let connectors = connectors::merge_plugin_apps_with_accessible( loaded_plugins.effective_apps(), connectors::accessible_connectors_from_mcp_tools(&mcp_tools), diff --git a/codex-rs/core/src/connectors.rs b/codex-rs/core/src/connectors.rs index d75b59aa1..b2b8ac109 100644 --- a/codex-rs/core/src/connectors.rs +++ b/codex-rs/core/src/connectors.rs @@ -93,12 +93,11 @@ pub async fn list_accessible_connectors_from_mcp_tools( pub async fn list_cached_accessible_connectors_from_mcp_tools( config: &Config, ) -> Option> { - if !config.features.enabled(Feature::Apps) { - return Some(Vec::new()); - } - let auth_manager = auth_manager_from_config(config); let auth = auth_manager.auth().await; + if !config.features.apps_enabled_for_auth(auth.as_ref()) { + return Some(Vec::new()); + } let cache_key = accessible_connectors_cache_key(config, auth.as_ref()); read_cached_accessible_connectors(&cache_key).map(filter_disallowed_connectors) } @@ -118,15 +117,14 @@ pub async fn list_accessible_connectors_from_mcp_tools_with_options_and_status( config: &Config, force_refetch: bool, ) -> anyhow::Result { - if !config.features.enabled(Feature::Apps) { + let auth_manager = auth_manager_from_config(config); + let auth = auth_manager.auth().await; + if !config.features.apps_enabled_for_auth(auth.as_ref()) { return Ok(AccessibleConnectorsStatus { connectors: Vec::new(), codex_apps_ready: true, }); } - - let auth_manager = auth_manager_from_config(config); - let auth = auth_manager.auth().await; let cache_key = accessible_connectors_cache_key(config, auth.as_ref()); let mcp_manager = McpManager::new(Arc::new(PluginsManager::new(config.codex_home.clone()))); let tool_plugin_provenance = mcp_manager.tool_plugin_provenance(config); diff --git a/codex-rs/core/src/features.rs b/codex-rs/core/src/features.rs index c21d7d963..107b99ad4 100644 --- a/codex-rs/core/src/features.rs +++ b/codex-rs/core/src/features.rs @@ -5,6 +5,8 @@ //! booleans through multiple types, call sites consult a single `Features` //! container attached to `Config`. +use crate::auth::AuthManager; +use crate::auth::CodexAuth; use crate::config::Config; use crate::config::ConfigToml; use crate::config::profile::ConfigProfile; @@ -257,6 +259,27 @@ impl Features { self.enabled.contains(&f) } + pub async fn apps_enabled(&self, auth_manager: Option<&AuthManager>) -> bool { + if !self.enabled(Feature::Apps) { + return false; + } + + let auth = match auth_manager { + Some(auth_manager) => auth_manager.auth().await, + None => None, + }; + self.apps_enabled_for_auth(auth.as_ref()) + } + + pub fn apps_enabled_cached(&self, auth_manager: Option<&AuthManager>) -> bool { + let auth = auth_manager.and_then(AuthManager::auth_cached); + self.apps_enabled_for_auth(auth.as_ref()) + } + + pub(crate) fn apps_enabled_for_auth(&self, auth: Option<&CodexAuth>) -> bool { + self.enabled(Feature::Apps) && auth.is_some_and(CodexAuth::is_chatgpt_auth) + } + pub fn enable(&mut self, f: Feature) -> &mut Self { self.enabled.insert(f); self @@ -973,4 +996,19 @@ mod tests { assert_eq!(feature_for_key("multi_agent"), Some(Feature::Collab)); assert_eq!(feature_for_key("collab"), Some(Feature::Collab)); } + + #[test] + fn apps_require_feature_flag_and_chatgpt_auth() { + let mut features = Features::with_defaults(); + assert!(!features.apps_enabled_for_auth(None)); + + features.enable(Feature::Apps); + assert!(!features.apps_enabled_for_auth(None)); + + let api_key_auth = CodexAuth::from_api_key("test-api-key"); + assert!(!features.apps_enabled_for_auth(Some(&api_key_auth))); + + let chatgpt_auth = CodexAuth::create_dummy_chatgpt_auth_for_testing(); + assert!(features.apps_enabled_for_auth(Some(&chatgpt_auth))); + } } diff --git a/codex-rs/core/src/mcp/mod.rs b/codex-rs/core/src/mcp/mod.rs index 8b8fdd394..39eca4b29 100644 --- a/codex-rs/core/src/mcp/mod.rs +++ b/codex-rs/core/src/mcp/mod.rs @@ -268,7 +268,7 @@ fn effective_mcp_servers( let servers = configured_mcp_servers(config, plugins_manager); with_codex_apps_mcp( servers, - config.features.enabled(Feature::Apps), + config.features.apps_enabled_for_auth(auth), auth, config, ) diff --git a/codex-rs/core/src/tools/spec.rs b/codex-rs/core/src/tools/spec.rs index 1553631d1..2b823e0a0 100644 --- a/codex-rs/core/src/tools/spec.rs +++ b/codex-rs/core/src/tools/spec.rs @@ -1294,8 +1294,13 @@ fn create_search_tool_bm25_tool(app_tools: &HashMap) -> ToolSp app_names.dedup(); let app_names = app_names.join(", "); - let description = - SEARCH_TOOL_BM25_DESCRIPTION_TEMPLATE.replace("{{app_names}}", app_names.as_str()); + let description = if app_names.is_empty() { + SEARCH_TOOL_BM25_DESCRIPTION_TEMPLATE + .replace("({{app_names}})", "(None currently enabled)") + .replace("{{app_names}}", "available apps") + } else { + SEARCH_TOOL_BM25_DESCRIPTION_TEMPLATE.replace("{{app_names}}", app_names.as_str()) + }; ToolSpec::Function(ResponsesApiTool { name: SEARCH_TOOL_BM25_TOOL_NAME.to_string(), @@ -1996,9 +2001,8 @@ pub(crate) fn build_specs( builder.register_handler("request_permissions", request_permissions_handler); } - if config.search_tool - && let Some(app_tools) = app_tools - { + if config.search_tool { + let app_tools = app_tools.unwrap_or_default(); builder.push_spec_with_parallel_support(create_search_tool_bm25_tool(&app_tools), true); builder.register_handler(SEARCH_TOOL_BM25_TOOL_NAME, search_tool_handler); } @@ -3393,6 +3397,74 @@ mod tests { assert!(!description.contains("mcp__rmcp__echo")); } + #[test] + fn search_tool_requires_apps_feature_flag_only() { + let config = test_config(); + let model_info = + ModelsManager::construct_model_info_offline_for_tests("gpt-5-codex", &config); + let app_tools = Some(HashMap::from([( + "mcp__codex_apps__calendar_create_event".to_string(), + ToolInfo { + server_name: crate::mcp::CODEX_APPS_MCP_SERVER_NAME.to_string(), + tool_name: "calendar_create_event".to_string(), + tool: mcp_tool( + "calendar_create_event", + "Create calendar event", + serde_json::json!({"type": "object"}), + ), + connector_id: Some("calendar".to_string()), + connector_name: Some("Calendar".to_string()), + plugin_display_names: Vec::new(), + }, + )])); + + let features = Features::with_defaults(); + let tools_config = ToolsConfig::new(&ToolsConfigParams { + model_info: &model_info, + features: &features, + web_search_mode: Some(WebSearchMode::Cached), + session_source: SessionSource::Cli, + }); + let (tools, _) = build_specs(&tools_config, None, app_tools.clone(), &[]).build(); + assert_lacks_tool_name(&tools, SEARCH_TOOL_BM25_TOOL_NAME); + + let mut features = Features::with_defaults(); + features.enable(Feature::Apps); + let tools_config = ToolsConfig::new(&ToolsConfigParams { + model_info: &model_info, + features: &features, + web_search_mode: Some(WebSearchMode::Cached), + session_source: SessionSource::Cli, + }); + let (tools, _) = build_specs(&tools_config, None, app_tools, &[]).build(); + assert_contains_tool_names(&tools, &[SEARCH_TOOL_BM25_TOOL_NAME]); + } + + #[test] + fn search_tool_description_handles_no_enabled_apps() { + let config = test_config(); + let model_info = + ModelsManager::construct_model_info_offline_for_tests("gpt-5-codex", &config); + let mut features = Features::with_defaults(); + features.enable(Feature::Apps); + let tools_config = ToolsConfig::new(&ToolsConfigParams { + model_info: &model_info, + features: &features, + web_search_mode: Some(WebSearchMode::Cached), + session_source: SessionSource::Cli, + }); + + let (tools, _) = build_specs(&tools_config, None, Some(HashMap::new()), &[]).build(); + let search_tool = find_tool(&tools, SEARCH_TOOL_BM25_TOOL_NAME); + let ToolSpec::Function(ResponsesApiTool { description, .. }) = &search_tool.spec else { + panic!("expected function tool"); + }; + + assert!(description.contains("(None currently enabled)")); + assert!(description.contains("available apps.")); + assert!(!description.contains("{{app_names}}")); + } + #[test] fn test_mcp_tool_property_missing_type_defaults_to_string() { let config = test_config(); diff --git a/codex-rs/core/tests/suite/client.rs b/codex-rs/core/tests/suite/client.rs index 43d0b143b..cfb84be83 100644 --- a/codex-rs/core/tests/suite/client.rs +++ b/codex-rs/core/tests/suite/client.rs @@ -921,7 +921,7 @@ async fn includes_user_instructions_message_in_request() { } #[tokio::test(flavor = "multi_thread", worker_threads = 2)] -async fn includes_apps_guidance_as_developer_message_when_enabled() { +async fn includes_apps_guidance_as_developer_message_for_chatgpt_auth() { skip_if_no_network!(); let server = MockServer::start().await; let apps_server = AppsTestServer::mount(&server) @@ -936,7 +936,7 @@ async fn includes_apps_guidance_as_developer_message_when_enabled() { .await; let mut builder = test_codex() - .with_auth(CodexAuth::from_api_key("Test API Key")) + .with_auth(create_dummy_codex_auth()) .with_config(move |config| { config .features @@ -1011,6 +1011,76 @@ async fn includes_apps_guidance_as_developer_message_when_enabled() { ); } +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn omits_apps_guidance_for_api_key_auth_even_when_feature_enabled() { + skip_if_no_network!(); + let server = MockServer::start().await; + let apps_server = AppsTestServer::mount(&server) + .await + .expect("mount apps MCP mock"); + let apps_base_url = apps_server.chatgpt_base_url.clone(); + + let resp_mock = mount_sse_once( + &server, + sse(vec![ev_response_created("resp1"), ev_completed("resp1")]), + ) + .await; + + let mut builder = test_codex() + .with_auth(CodexAuth::from_api_key("Test API Key")) + .with_config(move |config| { + config + .features + .enable(Feature::Apps) + .expect("test config should allow feature update"); + config + .features + .disable(Feature::AppsMcpGateway) + .expect("test config should allow feature update"); + config.chatgpt_base_url = apps_base_url; + }); + let codex = builder + .build(&server) + .await + .expect("create new conversation") + .codex; + + codex + .submit(Op::UserInput { + items: vec![UserInput::Text { + text: "hello".into(), + text_elements: Vec::new(), + }], + final_output_json_schema: None, + }) + .await + .unwrap(); + + wait_for_event(&codex, |ev| matches!(ev, EventMsg::TurnComplete(_))).await; + + let request = resp_mock.single_request(); + let request_body = request.body_json(); + let input = request_body["input"].as_array().expect("input array"); + let apps_snippet = "Apps are mentioned in the prompt in the format"; + + let has_apps_guidance = input.iter().any(|item| { + item.get("content") + .and_then(|value| value.as_array()) + .is_some_and(|content| { + content.iter().any(|entry| { + entry + .get("text") + .and_then(|value| value.as_str()) + .is_some_and(|text| text.contains(apps_snippet)) + }) + }) + }); + assert!( + !has_apps_guidance, + "did not expect apps guidance for API key auth, got {input:#?}" + ); +} + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn skills_append_to_instructions() { skip_if_no_network!(); diff --git a/codex-rs/core/tests/suite/plugins.rs b/codex-rs/core/tests/suite/plugins.rs index 441921cb2..b797ac3f2 100644 --- a/codex-rs/core/tests/suite/plugins.rs +++ b/codex-rs/core/tests/suite/plugins.rs @@ -114,7 +114,7 @@ async fn build_apps_enabled_plugin_test_codex( ) -> Result> { let mut builder = test_codex() .with_home(codex_home) - .with_auth(CodexAuth::from_api_key("Test API Key")) + .with_auth(CodexAuth::create_dummy_chatgpt_auth_for_testing()) .with_config(move |config| { config .features diff --git a/codex-rs/core/tests/suite/search_tool.rs b/codex-rs/core/tests/suite/search_tool.rs index 00315d25f..13fb666ee 100644 --- a/codex-rs/core/tests/suite/search_tool.rs +++ b/codex-rs/core/tests/suite/search_tool.rs @@ -5,6 +5,7 @@ use std::sync::Arc; use std::time::Duration; use anyhow::Result; +use codex_core::CodexAuth; use codex_core::CodexThread; use codex_core::NewThread; use codex_core::config::Config; @@ -163,9 +164,11 @@ fn configure_apps_with_optional_rmcp( } fn configured_builder(apps_base_url: String, rmcp_server_bin: Option) -> TestCodexBuilder { - test_codex().with_config(move |config| { - configure_apps_with_optional_rmcp(config, apps_base_url.as_str(), rmcp_server_bin); - }) + test_codex() + .with_auth(CodexAuth::create_dummy_chatgpt_auth_for_testing()) + .with_config(move |config| { + configure_apps_with_optional_rmcp(config, apps_base_url.as_str(), rmcp_server_bin); + }) } async fn submit_user_input(thread: &Arc, text: &str) -> Result<()> { @@ -218,6 +221,46 @@ async fn search_tool_flag_adds_tool() -> Result<()> { Ok(()) } +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn search_tool_flag_adds_tool_for_api_key_auth() -> Result<()> { + skip_if_no_network!(Ok(())); + + let server = start_mock_server().await; + let apps_server = AppsTestServer::mount(&server).await?; + let mock = mount_sse_once( + &server, + sse(vec![ + ev_response_created("resp-1"), + ev_assistant_message("msg-1", "done"), + ev_completed("resp-1"), + ]), + ) + .await; + + let mut builder = test_codex() + .with_auth(CodexAuth::from_api_key("Test API Key")) + .with_config(move |config| { + configure_apps_with_optional_rmcp(config, apps_server.chatgpt_base_url.as_str(), None); + }); + let test = builder.build(&server).await?; + + test.submit_turn_with_policies( + "list tools", + AskForApproval::Never, + SandboxPolicy::DangerFullAccess, + ) + .await?; + + let body = mock.single_request().body_json(); + let tools = tool_names(&body); + assert!( + tools.iter().any(|name| name == SEARCH_TOOL_BM25_TOOL_NAME), + "tools list should include {SEARCH_TOOL_BM25_TOOL_NAME} for API key auth when Apps is enabled: {tools:?}" + ); + + Ok(()) +} + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn search_tool_adds_discovery_instructions_to_tool_description() -> Result<()> { skip_if_no_network!(Ok(())); diff --git a/codex-rs/tui/src/bottom_pane/bottom_pane_view.rs b/codex-rs/tui/src/bottom_pane/bottom_pane_view.rs index a8561fe3d..35165db49 100644 --- a/codex-rs/tui/src/bottom_pane/bottom_pane_view.rs +++ b/codex-rs/tui/src/bottom_pane/bottom_pane_view.rs @@ -22,6 +22,12 @@ pub(crate) trait BottomPaneView: Renderable { None } + /// Actual item index for list-based views that want to preserve selection + /// across external refreshes. + fn selected_index(&self) -> Option { + None + } + /// Handle Ctrl-C while this view is active. fn on_ctrl_c(&mut self) -> CancellationEvent { CancellationEvent::NotHandled diff --git a/codex-rs/tui/src/bottom_pane/list_selection_view.rs b/codex-rs/tui/src/bottom_pane/list_selection_view.rs index 9a50a0783..e3b328713 100644 --- a/codex-rs/tui/src/bottom_pane/list_selection_view.rs +++ b/codex-rs/tui/src/bottom_pane/list_selection_view.rs @@ -677,6 +677,10 @@ impl BottomPaneView for ListSelectionView { self.view_id } + fn selected_index(&self) -> Option { + self.selected_actual_idx() + } + fn on_ctrl_c(&mut self) -> CancellationEvent { if let Some(cb) = &self.on_cancel { cb(&self.app_event_tx); diff --git a/codex-rs/tui/src/bottom_pane/mod.rs b/codex-rs/tui/src/bottom_pane/mod.rs index 5a37899b0..913b466bc 100644 --- a/codex-rs/tui/src/bottom_pane/mod.rs +++ b/codex-rs/tui/src/bottom_pane/mod.rs @@ -803,6 +803,13 @@ impl BottomPane { true } + pub(crate) fn selected_index_for_active_view(&self, view_id: &'static str) -> Option { + self.view_stack + .last() + .filter(|view| view.view_id() == Some(view_id)) + .and_then(|view| view.selected_index()) + } + /// Update the pending-input preview shown above the composer. pub(crate) fn set_pending_input_preview( &mut self, diff --git a/codex-rs/tui/src/chatwidget.rs b/codex-rs/tui/src/chatwidget.rs index b6c329788..676ce4a95 100644 --- a/codex-rs/tui/src/chatwidget.rs +++ b/codex-rs/tui/src/chatwidget.rs @@ -599,6 +599,7 @@ pub(crate) struct ChatWidget { /// currently executing. mcp_startup_status: Option>, connectors_cache: ConnectorsCacheState, + connectors_partial_snapshot: Option, connectors_prefetch_in_flight: bool, connectors_force_refetch_pending: bool, // Queue of interruptive UI events deferred during an active write cycle @@ -3237,6 +3238,7 @@ impl ChatWidget { agent_turn_running: false, mcp_startup_status: None, connectors_cache: ConnectorsCacheState::default(), + connectors_partial_snapshot: None, connectors_prefetch_in_flight: false, connectors_force_refetch_pending: false, interrupts: InterruptManager::new(), @@ -3316,7 +3318,7 @@ impl ChatWidget { widget .bottom_pane - .set_connectors_enabled(widget.config.features.enabled(Feature::Apps)); + .set_connectors_enabled(widget.connectors_enabled()); widget } @@ -3420,6 +3422,7 @@ impl ChatWidget { agent_turn_running: false, mcp_startup_status: None, connectors_cache: ConnectorsCacheState::default(), + connectors_partial_snapshot: None, connectors_prefetch_in_flight: false, connectors_force_refetch_pending: false, interrupts: InterruptManager::new(), @@ -3489,7 +3492,7 @@ impl ChatWidget { .set_queued_message_edit_binding(widget.queued_message_edit_binding); widget .bottom_pane - .set_connectors_enabled(widget.config.features.enabled(Feature::Apps)); + .set_connectors_enabled(widget.connectors_enabled()); widget } @@ -3595,6 +3598,7 @@ impl ChatWidget { agent_turn_running: false, mcp_startup_status: None, connectors_cache: ConnectorsCacheState::default(), + connectors_partial_snapshot: None, connectors_prefetch_in_flight: false, connectors_force_refetch_pending: false, interrupts: InterruptManager::new(), @@ -3673,7 +3677,7 @@ impl ChatWidget { widget.update_collaboration_mode_indicator(); widget .bottom_pane - .set_connectors_enabled(widget.config.features.enabled(Feature::Apps)); + .set_connectors_enabled(widget.connectors_enabled()); widget } @@ -7748,7 +7752,9 @@ impl ChatWidget { } fn connectors_enabled(&self) -> bool { - self.config.features.enabled(Feature::Apps) + self.config + .features + .apps_enabled_cached(Some(self.auth_manager.as_ref())) } fn connectors_for_mentions(&self) -> Option<&[connectors::AppInfo]> { @@ -7756,6 +7762,10 @@ impl ChatWidget { return None; } + if let Some(snapshot) = &self.connectors_partial_snapshot { + return Some(snapshot.connectors.as_slice()); + } + match &self.connectors_cache { ConnectorsCacheState::Ready(snapshot) => Some(snapshot.connectors.as_slice()), _ => None, @@ -7861,7 +7871,9 @@ impl ChatWidget { } let connectors_cache = self.connectors_cache.clone(); - self.prefetch_connectors_with_options(true); + let should_force_refetch = !self.connectors_prefetch_in_flight + || matches!(connectors_cache, ConnectorsCacheState::Ready(_)); + self.prefetch_connectors_with_options(should_force_refetch); match connectors_cache { ConnectorsCacheState::Ready(snapshot) => { @@ -7874,28 +7886,51 @@ impl ChatWidget { ConnectorsCacheState::Failed(err) => { self.add_to_history(history_cell::new_error_event(err)); } - ConnectorsCacheState::Loading => { - self.add_to_history(history_cell::new_info_event( - "Apps are still loading.".to_string(), - Some("Try again in a moment.".to_string()), - )); - } - ConnectorsCacheState::Uninitialized => { - self.add_to_history(history_cell::new_info_event( - "Apps are still loading.".to_string(), - Some("Try again in a moment.".to_string()), - )); + ConnectorsCacheState::Loading | ConnectorsCacheState::Uninitialized => { + self.open_connectors_loading_popup(); } } self.request_redraw(); } - fn open_connectors_popup(&mut self, connectors: &[connectors::AppInfo]) { - self.bottom_pane - .show_selection_view(self.connectors_popup_params(connectors)); + fn open_connectors_loading_popup(&mut self) { + if !self.bottom_pane.replace_selection_view_if_active( + CONNECTORS_SELECTION_VIEW_ID, + self.connectors_loading_popup_params(), + ) { + self.bottom_pane + .show_selection_view(self.connectors_loading_popup_params()); + } } - fn connectors_popup_params(&self, connectors: &[connectors::AppInfo]) -> SelectionViewParams { + fn open_connectors_popup(&mut self, connectors: &[connectors::AppInfo]) { + self.bottom_pane + .show_selection_view(self.connectors_popup_params(connectors, None)); + } + + fn connectors_loading_popup_params(&self) -> SelectionViewParams { + let mut header = ColumnRenderable::new(); + header.push(Line::from("Apps".bold())); + header.push(Line::from("Loading installed and available apps...".dim())); + + SelectionViewParams { + view_id: Some(CONNECTORS_SELECTION_VIEW_ID), + header: Box::new(header), + items: vec![SelectionItem { + name: "Loading apps...".to_string(), + description: Some("This updates when the full list is ready.".to_string()), + is_disabled: true, + ..Default::default() + }], + ..Default::default() + } + } + + fn connectors_popup_params( + &self, + connectors: &[connectors::AppInfo], + selected_connector_id: Option<&str>, + ) -> SelectionViewParams { let total = connectors.len(); let installed = connectors .iter() @@ -7909,6 +7944,11 @@ impl ChatWidget { header.push(Line::from( format!("Installed {installed} of {total} available apps.").dim(), )); + let initial_selected_idx = selected_connector_id.and_then(|selected_connector_id| { + connectors + .iter() + .position(|connector| connector.id == selected_connector_id) + }); let mut items: Vec = Vec::with_capacity(connectors.len()); for connector in connectors { let connector_label = connectors::connector_display_label(connector); @@ -7977,14 +8017,28 @@ impl ChatWidget { is_searchable: true, search_placeholder: Some("Type to search apps".to_string()), col_width_mode: ColumnWidthMode::AutoAllRows, + initial_selected_idx, ..Default::default() } } fn refresh_connectors_popup_if_open(&mut self, connectors: &[connectors::AppInfo]) { + let selected_connector_id = + if let (Some(selected_index), ConnectorsCacheState::Ready(snapshot)) = ( + self.bottom_pane + .selected_index_for_active_view(CONNECTORS_SELECTION_VIEW_ID), + &self.connectors_cache, + ) { + snapshot + .connectors + .get(selected_index) + .map(|connector| connector.id.as_str()) + } else { + None + }; let _ = self.bottom_pane.replace_selection_view_if_active( CONNECTORS_SELECTION_VIEW_ID, - self.connectors_popup_params(connectors), + self.connectors_popup_params(connectors, selected_connector_id), ); } @@ -8309,15 +8363,28 @@ impl ChatWidget { } } } - self.refresh_connectors_popup_if_open(&snapshot.connectors); - if is_final || !matches!(self.connectors_cache, ConnectorsCacheState::Ready(_)) { + if is_final { + self.connectors_partial_snapshot = None; + self.refresh_connectors_popup_if_open(&snapshot.connectors); self.connectors_cache = ConnectorsCacheState::Ready(snapshot.clone()); + } else { + self.connectors_partial_snapshot = Some(snapshot.clone()); } self.bottom_pane.set_connectors_snapshot(Some(snapshot)); } Err(err) => { - if matches!(self.connectors_cache, ConnectorsCacheState::Ready(_)) { + let partial_snapshot = self.connectors_partial_snapshot.take(); + if let ConnectorsCacheState::Ready(snapshot) = &self.connectors_cache { warn!("failed to refresh apps list; retaining current apps snapshot: {err}"); + self.bottom_pane + .set_connectors_snapshot(Some(snapshot.clone())); + } else if let Some(snapshot) = partial_snapshot { + warn!( + "failed to load full apps list; falling back to installed apps snapshot: {err}" + ); + self.refresh_connectors_popup_if_open(&snapshot.connectors); + self.connectors_cache = ConnectorsCacheState::Ready(snapshot.clone()); + self.bottom_pane.set_connectors_snapshot(Some(snapshot)); } else { self.connectors_cache = ConnectorsCacheState::Failed(err); self.bottom_pane.set_connectors_snapshot(None); diff --git a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__apps_popup_loading_state.snap b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__apps_popup_loading_state.snap new file mode 100644 index 000000000..e75302e5c --- /dev/null +++ b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__apps_popup_loading_state.snap @@ -0,0 +1,8 @@ +--- +source: tui/src/chatwidget/tests.rs +expression: before +--- + Apps + Loading installed and available apps... + +› 1. Loading apps... This updates when the full list is ready. diff --git a/codex-rs/tui/src/chatwidget/tests.rs b/codex-rs/tui/src/chatwidget/tests.rs index 85f4d051d..0896adc07 100644 --- a/codex-rs/tui/src/chatwidget/tests.rs +++ b/codex-rs/tui/src/chatwidget/tests.rs @@ -1849,6 +1849,7 @@ async fn make_chatwidget_manual( agent_turn_running: false, mcp_startup_status: None, connectors_cache: ConnectorsCacheState::default(), + connectors_partial_snapshot: None, connectors_prefetch_in_flight: false, connectors_force_refetch_pending: false, interrupts: InterruptManager::new(), @@ -6523,8 +6524,9 @@ fn render_bottom_popup(chat: &ChatWidget, width: u16) -> String { } #[tokio::test] -async fn apps_popup_refreshes_when_connectors_snapshot_updates() { +async fn apps_popup_stays_loading_until_final_snapshot_updates() { let (mut chat, _rx, _op_rx) = make_chatwidget_manual(None).await; + set_chatgpt_auth(&mut chat); chat.config .features .enable(Feature::Apps) @@ -6561,13 +6563,10 @@ async fn apps_popup_refreshes_when_connectors_snapshot_updates() { let before = render_bottom_popup(&chat, 80); assert!( - before.contains("Installed 1 of 1 available apps."), - "expected initial apps popup snapshot, got:\n{before}" - ); - assert!( - before.contains("Installed. Press Enter to open the app page"), - "expected selected app description to explain the app page action, got:\n{before}" + before.contains("Loading installed and available apps..."), + "expected /apps to stay in the loading state until the full list arrives, got:\n{before}" ); + assert_snapshot!("apps_popup_loading_state", before); chat.on_connectors_loaded( Ok(ConnectorsSnapshot { @@ -6621,6 +6620,7 @@ async fn apps_popup_refreshes_when_connectors_snapshot_updates() { #[tokio::test] async fn apps_refresh_failure_keeps_existing_full_snapshot() { let (mut chat, _rx, _op_rx) = make_chatwidget_manual(None).await; + set_chatgpt_auth(&mut chat); chat.config .features .enable(Feature::Apps) @@ -6703,9 +6703,130 @@ async fn apps_refresh_failure_keeps_existing_full_snapshot() { ); } +#[tokio::test] +async fn apps_popup_preserves_selected_app_across_refresh() { + let (mut chat, _rx, _op_rx) = make_chatwidget_manual(None).await; + set_chatgpt_auth(&mut chat); + chat.config + .features + .enable(Feature::Apps) + .expect("test config should allow feature update"); + chat.bottom_pane.set_connectors_enabled(true); + + chat.on_connectors_loaded( + Ok(ConnectorsSnapshot { + connectors: vec![ + codex_chatgpt::connectors::AppInfo { + id: "notion".to_string(), + name: "Notion".to_string(), + description: Some("Workspace docs".to_string()), + logo_url: None, + logo_url_dark: None, + distribution_channel: None, + branding: None, + app_metadata: None, + labels: None, + install_url: Some("https://example.test/notion".to_string()), + is_accessible: true, + is_enabled: true, + plugin_display_names: Vec::new(), + }, + codex_chatgpt::connectors::AppInfo { + id: "slack".to_string(), + name: "Slack".to_string(), + description: Some("Team chat".to_string()), + logo_url: None, + logo_url_dark: None, + distribution_channel: None, + branding: None, + app_metadata: None, + labels: None, + install_url: Some("https://example.test/slack".to_string()), + is_accessible: true, + is_enabled: true, + plugin_display_names: Vec::new(), + }, + ], + }), + true, + ); + chat.add_connectors_output(); + chat.handle_key_event(KeyEvent::from(KeyCode::Down)); + + let before = render_bottom_popup(&chat, 80); + assert!( + before.contains("› Slack"), + "expected Slack to be selected before refresh, got:\n{before}" + ); + + chat.on_connectors_loaded( + Ok(ConnectorsSnapshot { + connectors: vec![ + codex_chatgpt::connectors::AppInfo { + id: "airtable".to_string(), + name: "Airtable".to_string(), + description: Some("Spreadsheets".to_string()), + logo_url: None, + logo_url_dark: None, + distribution_channel: None, + branding: None, + app_metadata: None, + labels: None, + install_url: Some("https://example.test/airtable".to_string()), + is_accessible: true, + is_enabled: true, + plugin_display_names: Vec::new(), + }, + codex_chatgpt::connectors::AppInfo { + id: "notion".to_string(), + name: "Notion".to_string(), + description: Some("Workspace docs".to_string()), + logo_url: None, + logo_url_dark: None, + distribution_channel: None, + branding: None, + app_metadata: None, + labels: None, + install_url: Some("https://example.test/notion".to_string()), + is_accessible: true, + is_enabled: true, + plugin_display_names: Vec::new(), + }, + codex_chatgpt::connectors::AppInfo { + id: "slack".to_string(), + name: "Slack".to_string(), + description: Some("Team chat".to_string()), + logo_url: None, + logo_url_dark: None, + distribution_channel: None, + branding: None, + app_metadata: None, + labels: None, + install_url: Some("https://example.test/slack".to_string()), + is_accessible: true, + is_enabled: true, + plugin_display_names: Vec::new(), + }, + ], + }), + true, + ); + + let after = render_bottom_popup(&chat, 80); + assert!( + after.contains("› Slack"), + "expected Slack to stay selected after refresh, got:\n{after}" + ); + assert!( + !after.contains("› Notion"), + "did not expect selection to reset to Notion after refresh, got:\n{after}" + ); +} + #[tokio::test] async fn apps_refresh_failure_with_cached_snapshot_triggers_pending_force_refetch() { let (mut chat, _rx, _op_rx) = make_chatwidget_manual(None).await; + set_chatgpt_auth(&mut chat); chat.config .features .enable(Feature::Apps) @@ -6744,8 +6865,9 @@ async fn apps_refresh_failure_with_cached_snapshot_triggers_pending_force_refetc } #[tokio::test] -async fn apps_partial_refresh_uses_same_filtering_as_full_refresh() { +async fn apps_popup_keeps_existing_full_snapshot_while_partial_refresh_loads() { let (mut chat, _rx, _op_rx) = make_chatwidget_manual(None).await; + set_chatgpt_auth(&mut chat); chat.config .features .enable(Feature::Apps) @@ -6837,18 +6959,75 @@ async fn apps_partial_refresh_uses_same_filtering_as_full_refresh() { let popup = render_bottom_popup(&chat, 80); assert!( - popup.contains("Installed 1 of 1 available apps."), - "expected partial refresh popup to use filtered connectors, got:\n{popup}" + popup.contains("Installed 1 of 2 available apps."), + "expected popup to keep the last full snapshot while partial refresh loads, got:\n{popup}" ); assert!( !popup.contains("Hidden OpenAI"), - "expected disallowed connector to be filtered from partial refresh popup, got:\n{popup}" + "expected popup to ignore partial refresh rows until the full list arrives, got:\n{popup}" + ); +} + +#[tokio::test] +async fn apps_refresh_failure_without_full_snapshot_falls_back_to_installed_apps() { + let (mut chat, _rx, _op_rx) = make_chatwidget_manual(None).await; + set_chatgpt_auth(&mut chat); + chat.config + .features + .enable(Feature::Apps) + .expect("test config should allow feature update"); + chat.bottom_pane.set_connectors_enabled(true); + + chat.on_connectors_loaded( + Ok(ConnectorsSnapshot { + connectors: vec![codex_chatgpt::connectors::AppInfo { + id: "unit_test_apps_refresh_failure_fallback_connector".to_string(), + name: "Notion".to_string(), + description: Some("Workspace docs".to_string()), + logo_url: None, + logo_url_dark: None, + distribution_channel: None, + branding: None, + app_metadata: None, + labels: None, + install_url: Some("https://example.test/notion".to_string()), + is_accessible: true, + is_enabled: true, + plugin_display_names: Vec::new(), + }], + }), + false, + ); + + chat.add_connectors_output(); + let loading_popup = render_bottom_popup(&chat, 80); + assert!( + loading_popup.contains("Loading installed and available apps..."), + "expected /apps to keep showing loading before the final result, got:\n{loading_popup}" + ); + + chat.on_connectors_loaded(Err("failed to load apps".to_string()), true); + + assert_matches!( + &chat.connectors_cache, + ConnectorsCacheState::Ready(snapshot) if snapshot.connectors.len() == 1 + ); + + let popup = render_bottom_popup(&chat, 80); + assert!( + popup.contains("Installed 1 of 1 available apps."), + "expected /apps to fall back to the installed apps snapshot, got:\n{popup}" + ); + assert!( + popup.contains("Installed. Press Enter to open the app page"), + "expected the fallback popup to behave like the installed apps view, got:\n{popup}" ); } #[tokio::test] async fn apps_popup_shows_disabled_status_for_installed_but_disabled_apps() { let (mut chat, _rx, _op_rx) = make_chatwidget_manual(None).await; + set_chatgpt_auth(&mut chat); chat.config .features .enable(Feature::Apps) @@ -6891,6 +7070,7 @@ async fn apps_popup_shows_disabled_status_for_installed_but_disabled_apps() { #[tokio::test] async fn apps_initial_load_applies_enabled_state_from_config() { let (mut chat, _rx, _op_rx) = make_chatwidget_manual(None).await; + set_chatgpt_auth(&mut chat); chat.config .features .enable(Feature::Apps) @@ -6944,6 +7124,7 @@ async fn apps_initial_load_applies_enabled_state_from_config() { #[tokio::test] async fn apps_refresh_preserves_toggled_enabled_state() { let (mut chat, _rx, _op_rx) = make_chatwidget_manual(None).await; + set_chatgpt_auth(&mut chat); chat.config .features .enable(Feature::Apps) @@ -7014,6 +7195,7 @@ async fn apps_refresh_preserves_toggled_enabled_state() { #[tokio::test] async fn apps_popup_for_not_installed_app_uses_install_only_selected_description() { let (mut chat, _rx, _op_rx) = make_chatwidget_manual(None).await; + set_chatgpt_auth(&mut chat); chat.config .features .enable(Feature::Apps)