mirror of
https://github.com/openai/codex.git
synced 2026-05-28 06:55:01 +00:00
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`
This commit is contained in:
1
codex-rs/Cargo.lock
generated
1
codex-rs/Cargo.lock
generated
@@ -3790,7 +3790,6 @@ dependencies = [
|
||||
"codex-app-server-client",
|
||||
"codex-app-server-protocol",
|
||||
"codex-arg0",
|
||||
"codex-chatgpt",
|
||||
"codex-cli",
|
||||
"codex-cloud-requirements",
|
||||
"codex-config",
|
||||
|
||||
@@ -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 }
|
||||
|
||||
@@ -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;
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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(),
|
||||
|
||||
@@ -315,12 +315,12 @@ pub(crate) fn find_app_mentions(
|
||||
}
|
||||
|
||||
let mut slug_counts: HashMap<String, usize> = 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<String>,
|
||||
linked_paths: HashMap<String, String>,
|
||||
@@ -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()]
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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");
|
||||
|
||||
@@ -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;
|
||||
|
||||
Reference in New Issue
Block a user