From cd934c8bcb1ba50791a980db1d0b38cef7cf84f9 Mon Sep 17 00:00:00 2001 From: canvrno-oai Date: Tue, 26 May 2026 12:09:07 -0700 Subject: [PATCH] tui: keep inaccessible apps out of mentions (#24625) ## Summary Fix the TUI `$` app mention paths so App Directory rows that are not accessible are not treated as usable apps. This includes the core preservation fix from #24104, but expands it to the other app mention paths: - preserve app-server `is_accessible` flags when partial `app/list/updated` snapshots reach the TUI - require apps to be both accessible and enabled when resolving exact `$slug` mentions - require restored/stale `app://...` bindings to point at accessible, enabled apps before emitting structured app mentions - remove the now-unused `codex-chatgpt` dependency from `codex-tui`, which addresses the `cargo shear` failure seen on #24104 ## Root Cause The app server already sends merged app snapshots with accessibility computed. The TUI handled app-server app list updates as partial app loads and re-ran the old accessible-app merge path. That path treated every notification row as accessible, so App Directory entries with `isAccessible=false` could appear in `$` suggestions. Regression source: #22914 routed app-list updates through the app server while reusing the old TUI partial-load handling. Related precursor: #14717 introduced the partial-load path, but #22914 made it user-visible for app-server updates. ## Issues Fixes #24145 Fixes #24205 Fixes #24319 ## Validation - `just fmt` - `git diff --check` - `just bazel-lock-update` - `just bazel-lock-check` - `just argument-comment-lint -p codex-tui` - `just test -p codex-tui chatwidget::tests::popups_and_settings::apps_notification_update_excludes_inaccessible_apps_from_mentions chatwidget::tests::composer_submission::submit_user_message_ignores_inaccessible_app_mentions_from_bindings chatwidget::skills::tests::find_app_mentions_requires_accessible_enabled_apps_for_bound_paths chatwidget::skills::tests::find_app_mentions_requires_accessible_enabled_apps_for_slugs` --- codex-rs/Cargo.lock | 1 - codex-rs/tui/Cargo.toml | 1 - codex-rs/tui/src/chatwidget.rs | 2 +- codex-rs/tui/src/chatwidget/connectors.rs | 7 -- .../tui/src/chatwidget/input_submission.rs | 8 +- codex-rs/tui/src/chatwidget/skills.rs | 81 ++++++++++++++++++- .../chatwidget/tests/composer_submission.rs | 56 +++++++++++++ .../chatwidget/tests/popups_and_settings.rs | 71 ++++++++++++++++ 8 files changed, 212 insertions(+), 15 deletions(-) diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index a1884ba9e2..044146d9de 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -3790,7 +3790,6 @@ dependencies = [ "codex-app-server-client", "codex-app-server-protocol", "codex-arg0", - "codex-chatgpt", "codex-cli", "codex-cloud-requirements", "codex-config", diff --git a/codex-rs/tui/Cargo.toml b/codex-rs/tui/Cargo.toml index c213e92bae..ffc5c5a942 100644 --- a/codex-rs/tui/Cargo.toml +++ b/codex-rs/tui/Cargo.toml @@ -31,7 +31,6 @@ codex-app-server-client = { workspace = true } codex-app-server-protocol = { workspace = true } codex-arg0 = { workspace = true } codex-install-context = { workspace = true } -codex-chatgpt = { workspace = true } codex-cloud-requirements = { workspace = true } codex-config = { workspace = true } codex-connectors = { workspace = true } diff --git a/codex-rs/tui/src/chatwidget.rs b/codex-rs/tui/src/chatwidget.rs index 2b86ab3d71..b9c5101187 100644 --- a/codex-rs/tui/src/chatwidget.rs +++ b/codex-rs/tui/src/chatwidget.rs @@ -124,7 +124,6 @@ use codex_app_server_protocol::TurnCompletedNotification; use codex_app_server_protocol::TurnPlanStepStatus; use codex_app_server_protocol::TurnStatus; use codex_app_server_protocol::UserInput; -use codex_chatgpt::connectors as chatgpt_connectors; use codex_config::ConfigLayerStackOrdering; use codex_config::types::ApprovalsReviewer; use codex_config::types::Notifications; @@ -362,6 +361,7 @@ mod slash_dispatch; use self::skills::collect_tool_mentions; use self::skills::find_app_mentions; use self::skills::find_skill_mentions_with_tool_mentions; +use self::skills::is_app_mentionable; mod plugins; use self::plugins::PluginInstallAuthFlowState; use self::plugins::PluginListFetchState; diff --git a/codex-rs/tui/src/chatwidget/connectors.rs b/codex-rs/tui/src/chatwidget/connectors.rs index 2df64df7d7..683bff135b 100644 --- a/codex-rs/tui/src/chatwidget/connectors.rs +++ b/codex-rs/tui/src/chatwidget/connectors.rs @@ -303,13 +303,6 @@ impl ChatWidget { match result { Ok(mut snapshot) => { - if !is_final { - snapshot.connectors = chatgpt_connectors::merge_connectors_with_accessible( - Vec::new(), - snapshot.connectors, - /*all_connectors_loaded*/ false, - ); - } if let ConnectorsCacheState::Ready(existing_snapshot) = &self.connectors.cache { let enabled_by_id: HashMap<&str, bool> = existing_snapshot .connectors diff --git a/codex-rs/tui/src/chatwidget/input_submission.rs b/codex-rs/tui/src/chatwidget/input_submission.rs index 2c960ee3d5..4700ffc0cc 100644 --- a/codex-rs/tui/src/chatwidget/input_submission.rs +++ b/codex-rs/tui/src/chatwidget/input_submission.rs @@ -263,10 +263,14 @@ impl ChatWidget { else { continue; }; - if !selected_app_ids.insert(app_id.to_string()) { + if selected_app_ids.contains(app_id) { continue; } - if let Some(app) = apps.iter().find(|app| app.id == app_id && app.is_enabled) { + if let Some(app) = apps + .iter() + .find(|app| app.id == app_id && is_app_mentionable(app)) + { + selected_app_ids.insert(app_id.to_string()); items.push(UserInput::Mention { name: app.name.clone(), path: binding.path.clone(), diff --git a/codex-rs/tui/src/chatwidget/skills.rs b/codex-rs/tui/src/chatwidget/skills.rs index 356e191b8e..687ce56416 100644 --- a/codex-rs/tui/src/chatwidget/skills.rs +++ b/codex-rs/tui/src/chatwidget/skills.rs @@ -315,12 +315,12 @@ pub(crate) fn find_app_mentions( } let mut slug_counts: HashMap = HashMap::new(); - for app in apps.iter().filter(|app| app.is_enabled) { + for app in apps.iter().filter(|app| is_app_mentionable(app)) { let slug = codex_connectors::metadata::connector_mention_slug(app); *slug_counts.entry(slug).or_insert(0) += 1; } - for app in apps.iter().filter(|app| app.is_enabled) { + for app in apps.iter().filter(|app| is_app_mentionable(app)) { let slug = codex_connectors::metadata::connector_mention_slug(app); let slug_count = slug_counts.get(&slug).copied().unwrap_or(0); if mentions.names.contains(&slug) @@ -333,11 +333,15 @@ pub(crate) fn find_app_mentions( } apps.iter() - .filter(|app| app.is_enabled && selected_ids.contains(&app.id)) + .filter(|app| is_app_mentionable(app) && selected_ids.contains(&app.id)) .cloned() .collect() } +pub(crate) fn is_app_mentionable(app: &AppInfo) -> bool { + app.is_accessible && app.is_enabled +} + pub(crate) struct ToolMentions { names: HashSet, linked_paths: HashMap, @@ -497,3 +501,74 @@ fn app_id_from_path(path: &str) -> Option<&str> { path.strip_prefix("app://") .filter(|value| !value.is_empty()) } + +#[cfg(test)] +mod tests { + use super::*; + use pretty_assertions::assert_eq; + + fn app(id: &str, name: &str) -> AppInfo { + AppInfo { + id: id.to_string(), + name: name.to_string(), + description: None, + logo_url: None, + logo_url_dark: None, + distribution_channel: None, + branding: None, + app_metadata: None, + labels: None, + install_url: None, + is_accessible: true, + is_enabled: true, + plugin_display_names: Vec::new(), + } + } + + #[test] + fn find_app_mentions_requires_accessible_enabled_apps_for_slugs() { + let apps = vec![ + app("google_drive", "Google Drive"), + AppInfo { + is_accessible: false, + ..app("arabica_uae", "% Arabica UAE") + }, + AppInfo { + is_enabled: false, + ..app("linear", "Linear") + }, + ]; + let mentions = collect_tool_mentions("$google-drive $arabica-uae $linear", &HashMap::new()); + + assert_eq!( + find_app_mentions(&mentions, &apps, &HashSet::new()), + vec![apps[0].clone()] + ); + } + + #[test] + fn find_app_mentions_requires_accessible_enabled_apps_for_bound_paths() { + let apps = vec![ + app("google_drive", "Google Drive"), + AppInfo { + is_accessible: false, + ..app("arabica_uae", "% Arabica UAE") + }, + AppInfo { + is_enabled: false, + ..app("linear", "Linear") + }, + ]; + let mention_paths = HashMap::from([ + ("google-drive".to_string(), "app://google_drive".to_string()), + ("arabica-uae".to_string(), "app://arabica_uae".to_string()), + ("linear".to_string(), "app://linear".to_string()), + ]); + let mentions = collect_tool_mentions("$google-drive $arabica-uae $linear", &mention_paths); + + assert_eq!( + find_app_mentions(&mentions, &apps, &HashSet::new()), + vec![apps[0].clone()] + ); + } +} diff --git a/codex-rs/tui/src/chatwidget/tests/composer_submission.rs b/codex-rs/tui/src/chatwidget/tests/composer_submission.rs index 70b5c49b0e..bfe563b94c 100644 --- a/codex-rs/tui/src/chatwidget/tests/composer_submission.rs +++ b/codex-rs/tui/src/chatwidget/tests/composer_submission.rs @@ -1,4 +1,5 @@ use super::*; +use crate::app_event::ConnectorsSnapshot; use codex_protocol::models::ManagedFileSystemPermissions; use codex_protocol::permissions::FileSystemAccessMode; use codex_protocol::permissions::FileSystemPath; @@ -1172,6 +1173,61 @@ async fn enqueueing_history_prompt_multiple_times_is_stable() { } } +#[tokio::test] +async fn submit_user_message_ignores_inaccessible_app_mentions_from_bindings() { + let (mut chat, _rx, mut op_rx) = make_chatwidget_manual(/*model_override*/ None).await; + chat.thread_id = Some(ThreadId::new()); + set_chatgpt_auth(&mut chat); + chat.config + .features + .enable(Feature::Apps) + .expect("test config should allow feature update"); + + chat.on_connectors_loaded( + Ok(ConnectorsSnapshot { + connectors: vec![AppInfo { + id: "arabica_uae".to_string(), + name: "% Arabica UAE".to_string(), + description: Some("Directory-only app".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/arabica".to_string()), + is_accessible: false, + is_enabled: true, + plugin_display_names: Vec::new(), + }], + }), + /*is_final*/ false, + ); + + chat.submit_user_message(UserMessage { + text: "$arabica-uae".to_string(), + local_images: Vec::new(), + remote_image_urls: Vec::new(), + text_elements: Vec::new(), + mention_bindings: vec![MentionBinding { + mention: "arabica-uae".to_string(), + path: "app://arabica_uae".to_string(), + }], + }); + + let items = match next_submit_op(&mut op_rx) { + Op::UserTurn { items, .. } => items, + other => panic!("expected Op::UserTurn, got {other:?}"), + }; + assert_eq!( + items, + vec![UserInput::Text { + text: "$arabica-uae".to_string(), + text_elements: Vec::new(), + }] + ); +} + #[test] fn user_message_display_from_inputs_matches_flattened_user_message_shape() { let local_image = PathBuf::from("/tmp/local.png"); diff --git a/codex-rs/tui/src/chatwidget/tests/popups_and_settings.rs b/codex-rs/tui/src/chatwidget/tests/popups_and_settings.rs index 09d911ad16..3a4bdd1329 100644 --- a/codex-rs/tui/src/chatwidget/tests/popups_and_settings.rs +++ b/codex-rs/tui/src/chatwidget/tests/popups_and_settings.rs @@ -1399,6 +1399,77 @@ async fn apps_popup_stays_loading_until_final_snapshot_updates() { ); } +#[tokio::test] +async fn apps_notification_update_excludes_inaccessible_apps_from_mentions() { + let (mut chat, _rx, _op_rx) = make_chatwidget_manual(/*model_override*/ 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(/*enabled*/ true); + chat.bottom_pane + .set_composer_text("$".to_string(), Vec::new(), Vec::new()); + + chat.on_connectors_loaded( + Ok(ConnectorsSnapshot { + connectors: vec![ + AppInfo { + id: "google_drive".to_string(), + name: "Google Drive".to_string(), + description: Some("Connected files".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/google-drive".to_string()), + is_accessible: true, + is_enabled: true, + plugin_display_names: Vec::new(), + }, + AppInfo { + id: "arabica_uae".to_string(), + name: "% Arabica UAE".to_string(), + description: Some("Directory-only app".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/arabica".to_string()), + is_accessible: false, + is_enabled: true, + plugin_display_names: Vec::new(), + }, + ], + }), + /*is_final*/ false, + ); + + assert_matches!( + &chat.connectors.partial_snapshot, + Some(snapshot) + if snapshot + .connectors + .iter() + .find(|connector| connector.id == "arabica_uae") + .is_some_and(|connector| !connector.is_accessible) + ); + + let popup = render_bottom_popup(&chat, /*width*/ 80); + assert!( + popup.contains("Google Drive"), + "expected accessible apps to appear in the mention popup, got:\n{popup}" + ); + assert!( + !popup.contains("% Arabica UAE"), + "did not expect an inaccessible directory app in the mention popup, got:\n{popup}" + ); +} + #[tokio::test] async fn apps_refresh_failure_keeps_existing_full_snapshot() { let (mut chat, _rx, _op_rx) = make_chatwidget_manual(/*model_override*/ None).await;