diff --git a/codex-rs/app-server/src/message_processor.rs b/codex-rs/app-server/src/message_processor.rs index d3fa01de87..171783335e 100644 --- a/codex-rs/app-server/src/message_processor.rs +++ b/codex-rs/app-server/src/message_processor.rs @@ -1,4 +1,3 @@ -use std::collections::BTreeMap; use std::collections::HashSet; use std::future::Future; use std::sync::Arc; @@ -69,6 +68,8 @@ use codex_chatgpt::connectors; use codex_config::CONFIG_TOML_FILE; use codex_core::ThreadManager; use codex_core::config::Config; +use codex_core_plugins::remote; +use codex_core_plugins::remote::RemotePluginEnableOrDisableBatchMessage; use codex_exec_server::EnvironmentManager; use codex_features::Feature; use codex_feedback::CodexFeedback; @@ -90,7 +91,6 @@ use codex_protocol::protocol::W3cTraceContext; use codex_state::log_db::LogDbLayer; use codex_utils_absolute_path::AbsolutePathBuf; use futures::FutureExt; -use serde_json::Value as JsonValue; use tokio::sync::broadcast; use tokio::sync::watch; use tokio::time::Duration; @@ -1000,7 +1000,7 @@ impl MessageProcessor { request_id: ConnectionRequestId, params: ConfigValueWriteParams, ) { - let result = if is_remote_plugin_enable_or_disable_value_message(¶ms) { + let result = if remote::is_remote_plugin_enable_or_disable_value_message(¶ms) { self.remote_plugin_enable_or_disable_value_message(params) .await } else { @@ -1015,7 +1015,7 @@ impl MessageProcessor { params: ConfigBatchWriteParams, ) { let result = async { - if is_remote_plugin_enable_or_disable_batch_message(¶ms)? { + if remote::is_remote_plugin_enable_or_disable_batch_message(¶ms) { self.remote_plugin_enable_or_disable_batch_message(params) .await } else { @@ -1030,7 +1030,7 @@ impl MessageProcessor { &self, params: ConfigValueWriteParams, ) -> Result { - let action = remote_plugin_enable_or_disable_edit(¶ms.key_path, ¶ms.value) + let action = remote::remote_plugin_enable_or_disable_value_message(¶ms) .ok_or_else(|| invalid_request("invalid remote plugin enablement message"))?; self.codex_message_processor .remote_plugin_enable_or_disable(action.plugin_id, action.enabled) @@ -1042,13 +1042,22 @@ impl MessageProcessor { &self, params: ConfigBatchWriteParams, ) -> Result { - let batch = remote_plugin_enable_or_disable_batch(¶ms)? - .ok_or_else(|| invalid_request("invalid remote plugin enablement message"))?; - - for (plugin_id, enabled) in batch.actions { - self.codex_message_processor - .remote_plugin_enable_or_disable(plugin_id, enabled) - .await?; + match remote::remote_plugin_enable_or_disable_batch_message(¶ms) { + RemotePluginEnableOrDisableBatchMessage::RemotePluginToggles(toggles) => { + for (plugin_id, enabled) in toggles { + self.codex_message_processor + .remote_plugin_enable_or_disable(plugin_id, enabled) + .await?; + } + } + RemotePluginEnableOrDisableBatchMessage::MixedWithLocalConfigEdits => { + return Err(invalid_request( + "remote plugin enablement edits cannot be batched with local config edits", + )); + } + RemotePluginEnableOrDisableBatchMessage::NotRemotePluginToggle => { + return Err(invalid_request("invalid remote plugin enablement message")); + } } self.remote_plugin_enable_or_disable_response().await @@ -1365,85 +1374,6 @@ fn migration_items_need_runtime_refresh(items: &[ExternalAgentConfigMigrationIte }) } -struct RemotePluginEnableOrDisable { - plugin_id: String, - enabled: bool, -} - -struct RemotePluginEnableOrDisableBatch { - actions: BTreeMap, -} - -fn is_remote_plugin_enable_or_disable_value_message(params: &ConfigValueWriteParams) -> bool { - remote_plugin_enable_or_disable_edit(¶ms.key_path, ¶ms.value).is_some() -} - -fn is_remote_plugin_enable_or_disable_batch_message( - params: &ConfigBatchWriteParams, -) -> Result { - Ok(remote_plugin_enable_or_disable_batch(params)?.is_some()) -} - -fn remote_plugin_enable_or_disable_batch( - params: &ConfigBatchWriteParams, -) -> Result, JSONRPCErrorError> { - let mut actions = BTreeMap::::new(); - let mut has_local_edits = false; - - for edit in ¶ms.edits { - if let Some(action) = remote_plugin_enable_or_disable_edit(&edit.key_path, &edit.value) { - actions.insert(action.plugin_id, action.enabled); - } else { - has_local_edits = true; - } - } - - if actions.is_empty() { - return Ok(None); - } - - if has_local_edits { - return Err(invalid_request( - "remote plugin enablement edits cannot be batched with local config edits", - )); - } - - Ok(Some(RemotePluginEnableOrDisableBatch { actions })) -} - -fn remote_plugin_enable_or_disable_edit( - key_path: &str, - value: &JsonValue, -) -> Option { - let enabled = value.as_bool()?; - let mut segments = key_path.split('.'); - let table = segments.next()?; - let plugin_id = segments.next()?; - let field = segments.next()?; - if table == "plugins" - && field == "enabled" - && segments.next().is_none() - && is_remote_plugin_config_id(plugin_id) - { - return Some(RemotePluginEnableOrDisable { - plugin_id: plugin_id.to_string(), - enabled, - }); - } - None -} - -fn is_remote_plugin_config_id(plugin_id: &str) -> bool { - !plugin_id.is_empty() - && plugin_id - .chars() - .all(|ch| ch.is_ascii_alphanumeric() || ch == '-' || ch == '_' || ch == '~') - && (plugin_id.starts_with("plugins~") - || plugin_id.starts_with("app_") - || plugin_id.starts_with("asdk_app_") - || plugin_id.starts_with("connector_")) -} - #[cfg(test)] mod tracing_tests; diff --git a/codex-rs/core-plugins/src/remote.rs b/codex-rs/core-plugins/src/remote.rs index 753bc86f52..e18f995480 100644 --- a/codex-rs/core-plugins/src/remote.rs +++ b/codex-rs/core-plugins/src/remote.rs @@ -1,5 +1,7 @@ use crate::store::PLUGINS_CACHE_DIR; use crate::store::PluginStore; +use codex_app_server_protocol::ConfigBatchWriteParams; +use codex_app_server_protocol::ConfigValueWriteParams; use codex_app_server_protocol::PluginAuthPolicy; use codex_app_server_protocol::PluginInstallPolicy; use codex_app_server_protocol::PluginInterface; @@ -9,6 +11,7 @@ use codex_login::default_client::build_reqwest_client; use codex_plugin::PluginId; use reqwest::RequestBuilder; use serde::Deserialize; +use serde_json::Value as JsonValue; use std::collections::BTreeMap; use std::collections::BTreeSet; use std::collections::HashSet; @@ -71,6 +74,19 @@ pub struct RemotePluginSkill { pub enabled: bool, } +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct RemotePluginEnableOrDisable { + pub plugin_id: String, + pub enabled: bool, +} + +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum RemotePluginEnableOrDisableBatchMessage { + NotRemotePluginToggle, + RemotePluginToggles(BTreeMap), + MixedWithLocalConfigEdits, +} + #[derive(Debug, thiserror::Error)] pub enum RemotePluginCatalogError { #[error("chatgpt authentication required for remote plugin catalog")] @@ -546,6 +562,81 @@ pub async fn set_remote_plugin_enabled( Ok(()) } +pub fn is_remote_plugin_enable_or_disable_value_message(params: &ConfigValueWriteParams) -> bool { + remote_plugin_enable_or_disable_value_message(params).is_some() +} + +pub fn remote_plugin_enable_or_disable_value_message( + params: &ConfigValueWriteParams, +) -> Option { + remote_plugin_enable_or_disable_edit(¶ms.key_path, ¶ms.value) +} + +pub fn is_remote_plugin_enable_or_disable_batch_message(params: &ConfigBatchWriteParams) -> bool { + !matches!( + remote_plugin_enable_or_disable_batch_message(params), + RemotePluginEnableOrDisableBatchMessage::NotRemotePluginToggle + ) +} + +pub fn remote_plugin_enable_or_disable_batch_message( + params: &ConfigBatchWriteParams, +) -> RemotePluginEnableOrDisableBatchMessage { + let mut toggles = BTreeMap::::new(); + let mut has_local_edits = false; + + for edit in ¶ms.edits { + if let Some(action) = remote_plugin_enable_or_disable_edit(&edit.key_path, &edit.value) { + toggles.insert(action.plugin_id, action.enabled); + } else { + has_local_edits = true; + } + } + + if toggles.is_empty() { + return RemotePluginEnableOrDisableBatchMessage::NotRemotePluginToggle; + } + + if has_local_edits { + return RemotePluginEnableOrDisableBatchMessage::MixedWithLocalConfigEdits; + } + + RemotePluginEnableOrDisableBatchMessage::RemotePluginToggles(toggles) +} + +fn remote_plugin_enable_or_disable_edit( + key_path: &str, + value: &JsonValue, +) -> Option { + let enabled = value.as_bool()?; + let mut segments = key_path.split('.'); + let table = segments.next()?; + let plugin_id = segments.next()?; + let field = segments.next()?; + if table == "plugins" + && field == "enabled" + && segments.next().is_none() + && is_remote_plugin_config_id(plugin_id) + { + return Some(RemotePluginEnableOrDisable { + plugin_id: plugin_id.to_string(), + enabled, + }); + } + None +} + +fn is_remote_plugin_config_id(plugin_id: &str) -> bool { + !plugin_id.is_empty() + && plugin_id + .chars() + .all(|ch| ch.is_ascii_alphanumeric() || ch == '-' || ch == '_' || ch == '~') + && (plugin_id.starts_with("plugins~") + || plugin_id.starts_with("app_") + || plugin_id.starts_with("asdk_app_") + || plugin_id.starts_with("connector_")) +} + pub async fn uninstall_remote_plugin( config: &RemotePluginServiceConfig, auth: Option<&CodexAuth>, @@ -907,3 +998,82 @@ async fn send_and_decode Deserialize<'de>>( source, }) } + +#[cfg(test)] +mod tests { + use super::*; + use codex_app_server_protocol::ConfigEdit; + use codex_app_server_protocol::MergeStrategy; + use pretty_assertions::assert_eq; + use serde_json::json; + + const REMOTE_PLUGIN_ID: &str = "plugins~Plugin_00000000000000000000000000000000"; + + #[test] + fn value_write_message_detects_remote_plugin_enable_or_disable() { + let params = ConfigValueWriteParams { + key_path: format!("plugins.{REMOTE_PLUGIN_ID}.enabled"), + value: json!(false), + merge_strategy: MergeStrategy::Upsert, + file_path: None, + expected_version: None, + }; + + assert!(is_remote_plugin_enable_or_disable_value_message(¶ms)); + assert_eq!( + remote_plugin_enable_or_disable_value_message(¶ms), + Some(RemotePluginEnableOrDisable { + plugin_id: REMOTE_PLUGIN_ID.to_string(), + enabled: false, + }) + ); + } + + #[test] + fn batch_write_message_rejects_mixed_remote_and_local_edits() { + let params = ConfigBatchWriteParams { + edits: vec![ + ConfigEdit { + key_path: format!("plugins.{REMOTE_PLUGIN_ID}.enabled"), + value: json!(true), + merge_strategy: MergeStrategy::Upsert, + }, + ConfigEdit { + key_path: "model".to_string(), + value: json!("gpt-5"), + merge_strategy: MergeStrategy::Replace, + }, + ], + file_path: None, + expected_version: None, + reload_user_config: true, + }; + + assert!(is_remote_plugin_enable_or_disable_batch_message(¶ms)); + assert_eq!( + remote_plugin_enable_or_disable_batch_message(¶ms), + RemotePluginEnableOrDisableBatchMessage::MixedWithLocalConfigEdits + ); + } + + #[test] + fn batch_write_message_collects_remote_plugin_toggles() { + let params = ConfigBatchWriteParams { + edits: vec![ConfigEdit { + key_path: format!("plugins.{REMOTE_PLUGIN_ID}.enabled"), + value: json!(true), + merge_strategy: MergeStrategy::Upsert, + }], + file_path: None, + expected_version: None, + reload_user_config: true, + }; + let mut expected = BTreeMap::new(); + expected.insert(REMOTE_PLUGIN_ID.to_string(), true); + + assert_eq!( + remote_plugin_enable_or_disable_batch_message(¶ms), + RemotePluginEnableOrDisableBatchMessage::RemotePluginToggles(expected) + ); + } +}