mirror of
https://github.com/openai/codex.git
synced 2026-11-05 16:29:31 +00:00
[2 of 4] tui: route app and skill enablement through app server (#22914)
## Why App and skill toggles are user config mutations too. When the TUI is attached to a remote app server, writing those toggles into the local `config.toml` makes the UI report success without updating the server that actually owns the session. This is **[2 of 4]** in a stacked series that moves TUI-owned config mutations onto app-server APIs. ## What changed - Routed app enable/disable persistence through app-server config batch writes. - Routed skill enable/disable persistence through `skills/config/write`. - Avoided refreshing local config from disk after these writes when the TUI is connected to a remote app server. ## Config keys affected - `apps.<app_id>.enabled` - `apps.<app_id>.disabled_reason` - `[[skills.config]]` entries keyed by `path`, with `enabled = false` used for persisted disables ## Suggested manual validation - Connect the TUI to a remote app server, disable an app, reconnect, and confirm the app remains disabled from remote config rather than local disk state. - Re-enable the same app and confirm both `apps.<app_id>.enabled` and `apps.<app_id>.disabled_reason` are cleared remotely. - Disable a skill in the manage-skills UI and confirm a remote `[[skills.config]]` disable entry appears. - Re-enable that skill and confirm the disable entry is removed and the effective enabled state updates without relying on local config reloads. ## Stack 1. [#22913](https://github.com/openai/codex/pull/22913) `[1 of 4]` primary settings writes 2. [#22914](https://github.com/openai/codex/pull/22914) `[2 of 4]` app and skill enablement 3. [#22915](https://github.com/openai/codex/pull/22915) `[3 of 4]` feature and memory toggles 4. [#22916](https://github.com/openai/codex/pull/22916) `[4 of 4]` startup and onboarding bookkeeping
This commit is contained in:
1 parent
f0663fd4fd
commit
ae10708ae0
18 files changed
+189
-326
No files matched your search
@@ -22,86 +22,36 @@ pub(super) struct ConnectorsState {
|
||||
|
||||
impl ChatWidget {
|
||||
pub(crate) fn refresh_connectors(&mut self, force_refetch: bool) {
|
||||
self.prefetch_connectors_with_options(force_refetch);
|
||||
self.queue_connectors_refresh(force_refetch);
|
||||
}
|
||||
|
||||
pub(super) fn prefetch_connectors(&mut self) {
|
||||
self.prefetch_connectors_with_options(/*force_refetch*/ false);
|
||||
self.queue_connectors_refresh(/*force_refetch*/ false);
|
||||
}
|
||||
|
||||
fn prefetch_connectors_with_options(&mut self, force_refetch: bool) {
|
||||
fn queue_connectors_refresh(&mut self, force_refetch: bool) {
|
||||
if self.begin_connectors_refresh(force_refetch) {
|
||||
self.app_event_tx
|
||||
.send(AppEvent::FetchConnectorsList { force_refetch });
|
||||
}
|
||||
}
|
||||
|
||||
fn begin_connectors_refresh(&mut self, force_refetch: bool) -> bool {
|
||||
if !self.connectors_enabled() {
|
||||
return;
|
||||
return false;
|
||||
}
|
||||
if self.connectors.prefetch_in_flight {
|
||||
if force_refetch {
|
||||
self.connectors.force_refetch_pending = true;
|
||||
}
|
||||
return;
|
||||
return false;
|
||||
}
|
||||
|
||||
self.connectors.prefetch_in_flight = true;
|
||||
if !matches!(self.connectors.cache, ConnectorsCacheState::Ready(_)) {
|
||||
self.connectors.cache = ConnectorsCacheState::Loading;
|
||||
}
|
||||
|
||||
let config = self.config.clone();
|
||||
let environment_manager = Arc::clone(&self.environment_manager);
|
||||
let app_event_tx = self.app_event_tx.clone();
|
||||
tokio::spawn(async move {
|
||||
let accessible_result =
|
||||
match chatgpt_connectors::list_accessible_connectors_from_mcp_tools_with_environment_manager(
|
||||
&config,
|
||||
force_refetch,
|
||||
&environment_manager,
|
||||
)
|
||||
.await
|
||||
{
|
||||
Ok(connectors) => connectors,
|
||||
Err(err) => {
|
||||
app_event_tx.send(AppEvent::ConnectorsLoaded {
|
||||
result: Err(format!("Failed to load apps: {err}")),
|
||||
is_final: true,
|
||||
});
|
||||
return;
|
||||
}
|
||||
};
|
||||
let should_schedule_force_refetch =
|
||||
!force_refetch && !accessible_result.codex_apps_ready;
|
||||
let accessible_connectors = accessible_result.connectors;
|
||||
|
||||
app_event_tx.send(AppEvent::ConnectorsLoaded {
|
||||
result: Ok(ConnectorsSnapshot {
|
||||
connectors: accessible_connectors.clone(),
|
||||
}),
|
||||
is_final: false,
|
||||
});
|
||||
|
||||
let result: Result<ConnectorsSnapshot, String> = async {
|
||||
let all_connectors =
|
||||
chatgpt_connectors::list_all_connectors_with_options(&config, force_refetch)
|
||||
.await?;
|
||||
let connectors = chatgpt_connectors::merge_connectors_with_accessible(
|
||||
all_connectors,
|
||||
accessible_connectors,
|
||||
/*all_connectors_loaded*/ true,
|
||||
);
|
||||
Ok(ConnectorsSnapshot { connectors })
|
||||
}
|
||||
.await
|
||||
.map_err(|err: anyhow::Error| format!("Failed to load apps: {err}"));
|
||||
|
||||
app_event_tx.send(AppEvent::ConnectorsLoaded {
|
||||
result,
|
||||
is_final: true,
|
||||
});
|
||||
|
||||
if should_schedule_force_refetch {
|
||||
app_event_tx.send(AppEvent::RefreshConnectors {
|
||||
force_refetch: true,
|
||||
});
|
||||
}
|
||||
});
|
||||
true
|
||||
}
|
||||
|
||||
pub(super) fn connectors_enabled(&self) -> bool {
|
||||
@@ -135,7 +85,7 @@ impl ChatWidget {
|
||||
let connectors_cache = self.connectors.cache.clone();
|
||||
let should_force_refetch = !self.connectors.prefetch_in_flight
|
||||
|| matches!(connectors_cache, ConnectorsCacheState::Ready(_));
|
||||
self.prefetch_connectors_with_options(should_force_refetch);
|
||||
self.queue_connectors_refresh(should_force_refetch);
|
||||
|
||||
match connectors_cache {
|
||||
ConnectorsCacheState::Ready(snapshot) => {
|
||||
@@ -360,8 +310,6 @@ impl ChatWidget {
|
||||
/*all_connectors_loaded*/ false,
|
||||
);
|
||||
}
|
||||
snapshot.connectors =
|
||||
chatgpt_connectors::with_app_enabled_state(snapshot.connectors, &self.config);
|
||||
if let ConnectorsCacheState::Ready(existing_snapshot) = &self.connectors.cache {
|
||||
let enabled_by_id: HashMap<&str, bool> = existing_snapshot
|
||||
.connectors
|
||||
@@ -404,7 +352,7 @@ impl ChatWidget {
|
||||
}
|
||||
|
||||
if trigger_pending_force_refetch {
|
||||
self.prefetch_connectors_with_options(/*force_refetch*/ true);
|
||||
self.queue_connectors_refresh(/*force_refetch*/ true);
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -13,7 +13,6 @@ impl ChatWidget {
|
||||
) -> Self {
|
||||
let ChatWidgetInit {
|
||||
config,
|
||||
environment_manager,
|
||||
frame_requester,
|
||||
app_event_tx,
|
||||
workspace_command_runner,
|
||||
@@ -104,7 +103,6 @@ impl ChatWidget {
|
||||
transcript: TranscriptState::new(active_cell),
|
||||
raw_output_mode: config.tui_raw_output_mode,
|
||||
config,
|
||||
environment_manager,
|
||||
effective_service_tier,
|
||||
skills_all: Vec::new(),
|
||||
skills_initial_state: None,
|
||||
|
||||
@@ -119,11 +119,7 @@ pub(super) use codex_app_server_protocol::TurnStatus as AppServerTurnStatus;
|
||||
pub(super) use codex_app_server_protocol::UserInput;
|
||||
pub(super) use codex_app_server_protocol::UserInput as AppServerUserInput;
|
||||
pub(super) use codex_app_server_protocol::WarningNotification;
|
||||
pub(super) use codex_config::AppRequirementToml;
|
||||
pub(super) use codex_config::AppsRequirementsToml;
|
||||
pub(super) use codex_config::ConfigLayerStack;
|
||||
pub(super) use codex_config::ConfigRequirements;
|
||||
pub(super) use codex_config::ConfigRequirementsToml;
|
||||
pub(super) use codex_config::RequirementSource;
|
||||
pub(super) use codex_config::types::ApprovalsReviewer;
|
||||
pub(super) use codex_config::types::Notifications;
|
||||
@@ -178,7 +174,6 @@ pub(super) use insta::assert_snapshot;
|
||||
pub(super) use serde_json::json;
|
||||
#[cfg(target_os = "windows")]
|
||||
pub(super) use serial_test::serial;
|
||||
pub(super) use std::collections::BTreeMap;
|
||||
pub(super) use std::collections::HashMap;
|
||||
pub(super) use std::path::PathBuf;
|
||||
pub(super) use tempfile::NamedTempFile;
|
||||
|
||||
@@ -161,7 +161,6 @@ pub(super) async fn make_chatwidget_manual(
|
||||
let model_catalog = test_model_catalog(&cfg);
|
||||
let common = ChatWidgetInit {
|
||||
config: cfg,
|
||||
environment_manager: Arc::new(codex_exec_server::EnvironmentManager::default_for_tests()),
|
||||
frame_requester: FrameRequester::test_dummy(),
|
||||
app_event_tx,
|
||||
workspace_command_runner: None,
|
||||
|
||||
@@ -1481,7 +1481,6 @@ async fn make_startup_chat_with_cli_overrides(
|
||||
let session_telemetry = test_session_telemetry(&cfg, resolved_model.as_str());
|
||||
let init = ChatWidgetInit {
|
||||
config: cfg.clone(),
|
||||
environment_manager: Arc::new(codex_exec_server::EnvironmentManager::default_for_tests()),
|
||||
frame_requester: FrameRequester::test_dummy(),
|
||||
app_event_tx: AppEventSender::new(unbounded_channel::<AppEvent>().0),
|
||||
workspace_command_runner: None,
|
||||
|
||||
@@ -72,7 +72,6 @@ async fn experimental_mode_plan_is_ignored_on_startup() {
|
||||
let session_telemetry = test_session_telemetry(&cfg, resolved_model.as_str());
|
||||
let init = ChatWidgetInit {
|
||||
config: cfg.clone(),
|
||||
environment_manager: Arc::new(codex_exec_server::EnvironmentManager::default_for_tests()),
|
||||
frame_requester: FrameRequester::test_dummy(),
|
||||
app_event_tx: AppEventSender::new(unbounded_channel::<AppEvent>().0),
|
||||
workspace_command_runner: None,
|
||||
@@ -1859,198 +1858,6 @@ 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(/*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);
|
||||
|
||||
let temp = tempdir().expect("tempdir");
|
||||
let config_toml_path = temp.path().join("config.toml").abs();
|
||||
let user_config = toml::from_str::<TomlValue>(
|
||||
"[apps.connector_1]\nenabled = false\ndisabled_reason = \"user\"\n",
|
||||
)
|
||||
.expect("apps config");
|
||||
chat.config.config_layer_stack = chat
|
||||
.config
|
||||
.config_layer_stack
|
||||
.with_user_config(&config_toml_path, user_config);
|
||||
|
||||
chat.on_connectors_loaded(
|
||||
Ok(ConnectorsSnapshot {
|
||||
connectors: vec![AppInfo {
|
||||
id: "connector_1".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(),
|
||||
}],
|
||||
}),
|
||||
/*is_final*/ true,
|
||||
);
|
||||
|
||||
assert_matches!(
|
||||
&chat.connectors.cache,
|
||||
ConnectorsCacheState::Ready(snapshot)
|
||||
if snapshot
|
||||
.connectors
|
||||
.iter()
|
||||
.find(|connector| connector.id == "connector_1")
|
||||
.is_some_and(|connector| !connector.is_enabled)
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn apps_initial_load_applies_enabled_state_from_requirements_with_user_override() {
|
||||
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);
|
||||
|
||||
let requirements = ConfigRequirementsToml {
|
||||
apps: Some(AppsRequirementsToml {
|
||||
apps: BTreeMap::from([(
|
||||
"connector_1".to_string(),
|
||||
AppRequirementToml {
|
||||
enabled: Some(false),
|
||||
tools: None,
|
||||
},
|
||||
)]),
|
||||
}),
|
||||
..Default::default()
|
||||
};
|
||||
let temp = tempdir().expect("tempdir");
|
||||
let config_toml_path = temp.path().join("config.toml").abs();
|
||||
chat.config.config_layer_stack =
|
||||
ConfigLayerStack::new(Vec::new(), ConfigRequirements::default(), requirements)
|
||||
.expect("requirements stack")
|
||||
.with_user_config(
|
||||
&config_toml_path,
|
||||
toml::from_str::<TomlValue>(
|
||||
"[apps.connector_1]\nenabled = true\ndisabled_reason = \"user\"\n",
|
||||
)
|
||||
.expect("apps config"),
|
||||
);
|
||||
|
||||
chat.on_connectors_loaded(
|
||||
Ok(ConnectorsSnapshot {
|
||||
connectors: vec![AppInfo {
|
||||
id: "connector_1".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(),
|
||||
}],
|
||||
}),
|
||||
/*is_final*/ true,
|
||||
);
|
||||
|
||||
assert_matches!(
|
||||
&chat.connectors.cache,
|
||||
ConnectorsCacheState::Ready(snapshot)
|
||||
if snapshot
|
||||
.connectors
|
||||
.iter()
|
||||
.find(|connector| connector.id == "connector_1")
|
||||
.is_some_and(|connector| !connector.is_enabled)
|
||||
);
|
||||
|
||||
chat.add_connectors_output();
|
||||
let popup = render_bottom_popup(&chat, /*width*/ 80);
|
||||
assert!(
|
||||
popup.contains("Installed · Disabled. Press Enter to open the app page"),
|
||||
"expected requirements-disabled connector to render as disabled, got:\n{popup}"
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn apps_initial_load_applies_enabled_state_from_requirements_without_user_entry() {
|
||||
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);
|
||||
|
||||
let requirements = ConfigRequirementsToml {
|
||||
apps: Some(AppsRequirementsToml {
|
||||
apps: BTreeMap::from([(
|
||||
"connector_1".to_string(),
|
||||
AppRequirementToml {
|
||||
enabled: Some(false),
|
||||
tools: None,
|
||||
},
|
||||
)]),
|
||||
}),
|
||||
..Default::default()
|
||||
};
|
||||
chat.config.config_layer_stack =
|
||||
ConfigLayerStack::new(Vec::new(), ConfigRequirements::default(), requirements)
|
||||
.expect("requirements stack");
|
||||
|
||||
chat.on_connectors_loaded(
|
||||
Ok(ConnectorsSnapshot {
|
||||
connectors: vec![AppInfo {
|
||||
id: "connector_1".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(),
|
||||
}],
|
||||
}),
|
||||
/*is_final*/ true,
|
||||
);
|
||||
|
||||
assert_matches!(
|
||||
&chat.connectors.cache,
|
||||
ConnectorsCacheState::Ready(snapshot)
|
||||
if snapshot
|
||||
.connectors
|
||||
.iter()
|
||||
.find(|connector| connector.id == "connector_1")
|
||||
.is_some_and(|connector| !connector.is_enabled)
|
||||
);
|
||||
|
||||
chat.add_connectors_output();
|
||||
let popup = render_bottom_popup(&chat, /*width*/ 80);
|
||||
assert!(
|
||||
popup.contains("Installed · Disabled. Press Enter to open the app page"),
|
||||
"expected requirements-disabled connector to render as disabled, got:\n{popup}"
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn apps_refresh_preserves_toggled_enabled_state() {
|
||||
let (mut chat, _rx, _op_rx) = make_chatwidget_manual(/*model_override*/ None).await;
|
||||
|
||||
@@ -413,7 +413,6 @@ async fn configured_pet_load_is_deferred_until_after_construction() {
|
||||
let session_telemetry = test_session_telemetry(&cfg, resolved_model.as_str());
|
||||
let init = ChatWidgetInit {
|
||||
config: cfg.clone(),
|
||||
environment_manager: Arc::new(codex_exec_server::EnvironmentManager::default_for_tests()),
|
||||
frame_requester: FrameRequester::test_dummy(),
|
||||
app_event_tx: tx,
|
||||
workspace_command_runner: None,
|
||||
|
||||
Reference in new issue
Block a user