mirror of
https://github.com/openai/codex.git
synced 2026-06-01 19:02:59 +00:00
Sync local plugin imports, async remote imports, refresh caches after… (#18246)
… import ## Why `externalAgentConfig/import` used to spawn plugin imports in the background and return immediately. That meant local marketplace imports could still be in flight when the caller refreshed plugin state, so newly imported plugins would not show up right away. This change makes local marketplace imports complete before the RPC returns, while keeping remote marketplace imports asynchronous so we do not block on remote fetches. ## What changed - split plugin migration details into local and remote marketplace imports based on the external config source - import local marketplaces synchronously during `externalAgentConfig/import` - return pending remote plugin imports to the app-server so it can finish them in the background - clear the plugin and skills caches before responding to plugin imports, and again after background remote imports complete, so the next `plugin/list` reloads fresh state - keep marketplace source parsing encapsulated behind `is_local_marketplace_source(...)` instead of re-exporting the internal enum - add core and app-server coverage for the synchronous local import path and the pending remote import path ## Verification - `cargo test -p codex-app-server-protocol` - `cargo test -p codex-core` (currently fails an existing unrelated test: `config_loader::tests::cli_override_can_update_project_local_mcp_server_when_project_is_trusted`) - `cargo test` (currently fails existing `codex-app-server` integration tests in MCP/skills/thread-start areas, plus the unrelated `codex-core` failure above)
This commit is contained in:
@@ -7,6 +7,7 @@ use crate::plugins::PluginsManager;
|
||||
use crate::plugins::add_marketplace;
|
||||
use crate::plugins::configured_plugins_from_stack;
|
||||
use crate::plugins::find_marketplace_manifest_path;
|
||||
use crate::plugins::is_local_marketplace_source;
|
||||
use crate::plugins::parse_marketplace_source;
|
||||
use codex_core_plugins::marketplace::MarketplacePluginInstallPolicy;
|
||||
use codex_protocol::protocol::Product;
|
||||
@@ -51,12 +52,18 @@ pub struct MigrationDetails {
|
||||
pub plugins: Vec<PluginsMigration>,
|
||||
}
|
||||
|
||||
#[derive(Debug, Clone, PartialEq, Eq)]
|
||||
pub struct PendingPluginImport {
|
||||
pub cwd: Option<PathBuf>,
|
||||
pub details: MigrationDetails,
|
||||
}
|
||||
|
||||
#[derive(Debug, Clone, Default, PartialEq, Eq)]
|
||||
struct PluginImportOutcome {
|
||||
succeeded_marketplaces: Vec<String>,
|
||||
succeeded_plugin_ids: Vec<String>,
|
||||
failed_marketplaces: Vec<String>,
|
||||
failed_plugin_ids: Vec<String>,
|
||||
pub struct PluginImportOutcome {
|
||||
pub succeeded_marketplaces: Vec<String>,
|
||||
pub succeeded_plugin_ids: Vec<String>,
|
||||
pub failed_marketplaces: Vec<String>,
|
||||
pub failed_plugin_ids: Vec<String>,
|
||||
}
|
||||
|
||||
#[derive(Debug, Clone, PartialEq, Eq)]
|
||||
@@ -113,7 +120,8 @@ impl ExternalAgentConfigService {
|
||||
pub async fn import(
|
||||
&self,
|
||||
migration_items: Vec<ExternalAgentConfigMigrationItem>,
|
||||
) -> io::Result<()> {
|
||||
) -> io::Result<Vec<PendingPluginImport>> {
|
||||
let mut pending_plugin_imports = Vec::new();
|
||||
for migration_item in migration_items {
|
||||
match migration_item.item_type {
|
||||
ExternalAgentConfigMigrationItemType::Config => {
|
||||
@@ -141,17 +149,23 @@ impl ExternalAgentConfigService {
|
||||
);
|
||||
}
|
||||
ExternalAgentConfigMigrationItemType::Plugins => {
|
||||
let service = self.clone();
|
||||
let cwd = migration_item.cwd;
|
||||
let details = migration_item.details;
|
||||
tokio::spawn(async move {
|
||||
if let Err(err) = service.import_plugins(cwd.as_deref(), details).await {
|
||||
tracing::warn!(
|
||||
error = %err,
|
||||
"external agent config plugin import failed"
|
||||
);
|
||||
}
|
||||
});
|
||||
let details = migration_item.details.ok_or_else(|| {
|
||||
invalid_data_error("plugins migration item is missing details".to_string())
|
||||
})?;
|
||||
let (local_details, remote_details) =
|
||||
self.partition_plugin_migration_details(cwd.as_deref(), details)?;
|
||||
|
||||
if let Some(local_details) = local_details {
|
||||
self.import_plugins(cwd.as_deref(), Some(local_details))
|
||||
.await?;
|
||||
}
|
||||
if let Some(remote_details) = remote_details {
|
||||
pending_plugin_imports.push(PendingPluginImport {
|
||||
cwd,
|
||||
details: remote_details,
|
||||
});
|
||||
}
|
||||
emit_migration_metric(
|
||||
EXTERNAL_AGENT_CONFIG_IMPORT_METRIC,
|
||||
ExternalAgentConfigMigrationItemType::Plugins,
|
||||
@@ -162,7 +176,7 @@ impl ExternalAgentConfigService {
|
||||
}
|
||||
}
|
||||
|
||||
Ok(())
|
||||
Ok(pending_plugin_imports)
|
||||
}
|
||||
|
||||
async fn detect_migrations(
|
||||
@@ -349,7 +363,52 @@ impl ExternalAgentConfigService {
|
||||
})
|
||||
}
|
||||
|
||||
async fn import_plugins(
|
||||
fn partition_plugin_migration_details(
|
||||
&self,
|
||||
cwd: Option<&Path>,
|
||||
details: MigrationDetails,
|
||||
) -> io::Result<(Option<MigrationDetails>, Option<MigrationDetails>)> {
|
||||
let source_settings = cwd.map_or_else(
|
||||
|| self.external_agent_home.join("settings.json"),
|
||||
|cwd| cwd.join(EXTERNAL_AGENT_DIR).join("settings.json"),
|
||||
);
|
||||
let source_root = cwd.unwrap_or(self.external_agent_home.as_path());
|
||||
let import_sources = read_external_settings(&source_settings)?
|
||||
.map(|settings| collect_marketplace_import_sources(&settings, source_root))
|
||||
.unwrap_or_default();
|
||||
|
||||
let mut local_plugins = Vec::new();
|
||||
let mut remote_plugins = Vec::new();
|
||||
for plugin_group in details.plugins {
|
||||
let is_local = import_sources
|
||||
.get(&plugin_group.marketplace_name)
|
||||
.and_then(|import_source| {
|
||||
is_local_marketplace_source(
|
||||
&import_source.source,
|
||||
import_source.ref_name.clone(),
|
||||
)
|
||||
.ok()
|
||||
})
|
||||
.unwrap_or(false);
|
||||
|
||||
if is_local {
|
||||
local_plugins.push(plugin_group);
|
||||
} else {
|
||||
remote_plugins.push(plugin_group);
|
||||
}
|
||||
}
|
||||
|
||||
let local_details = (!local_plugins.is_empty()).then_some(MigrationDetails {
|
||||
plugins: local_plugins,
|
||||
});
|
||||
let remote_details = (!remote_plugins.is_empty()).then_some(MigrationDetails {
|
||||
plugins: remote_plugins,
|
||||
});
|
||||
|
||||
Ok((local_details, remote_details))
|
||||
}
|
||||
|
||||
pub async fn import_plugins(
|
||||
&self,
|
||||
cwd: Option<&Path>,
|
||||
details: Option<MigrationDetails>,
|
||||
|
||||
@@ -295,6 +295,123 @@ async fn import_home_skips_empty_config_migration() {
|
||||
assert!(!codex_home.join("config.toml").exists());
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn import_local_plugins_returns_completed_status() {
|
||||
let (_root, external_agent_home, codex_home) = fixture_paths();
|
||||
let marketplace_root = external_agent_home.join("my-marketplace");
|
||||
let plugin_root = marketplace_root.join("plugins").join("cloudflare");
|
||||
fs::create_dir_all(marketplace_root.join(".claude-plugin"))
|
||||
.expect("create marketplace manifest dir");
|
||||
fs::create_dir_all(plugin_root.join(".codex-plugin")).expect("create plugin manifest dir");
|
||||
fs::create_dir_all(&codex_home).expect("create codex home");
|
||||
|
||||
fs::write(
|
||||
external_agent_home.join("settings.json"),
|
||||
serde_json::to_string_pretty(&serde_json::json!({
|
||||
"enabledPlugins": {
|
||||
"cloudflare@my-plugins": true
|
||||
},
|
||||
"extraKnownMarketplaces": {
|
||||
"my-plugins": {
|
||||
"source": "local",
|
||||
"path": marketplace_root
|
||||
}
|
||||
}
|
||||
}))
|
||||
.expect("serialize settings"),
|
||||
)
|
||||
.expect("write settings");
|
||||
fs::write(
|
||||
marketplace_root
|
||||
.join(".claude-plugin")
|
||||
.join("marketplace.json"),
|
||||
r#"{
|
||||
"name": "my-plugins",
|
||||
"plugins": [
|
||||
{
|
||||
"name": "cloudflare",
|
||||
"source": "./plugins/cloudflare"
|
||||
}
|
||||
]
|
||||
}"#,
|
||||
)
|
||||
.expect("write marketplace manifest");
|
||||
fs::write(
|
||||
plugin_root.join(".codex-plugin").join("plugin.json"),
|
||||
r#"{"name":"cloudflare","version":"0.1.0"}"#,
|
||||
)
|
||||
.expect("write plugin manifest");
|
||||
|
||||
let outcome = service_for_paths(external_agent_home, codex_home.clone())
|
||||
.import(vec![ExternalAgentConfigMigrationItem {
|
||||
item_type: ExternalAgentConfigMigrationItemType::Plugins,
|
||||
description: String::new(),
|
||||
cwd: None,
|
||||
details: Some(MigrationDetails {
|
||||
plugins: vec![PluginsMigration {
|
||||
marketplace_name: "my-plugins".to_string(),
|
||||
plugin_names: vec!["cloudflare".to_string()],
|
||||
}],
|
||||
}),
|
||||
}])
|
||||
.await
|
||||
.expect("import");
|
||||
|
||||
assert_eq!(outcome, Vec::<PendingPluginImport>::new());
|
||||
let config = fs::read_to_string(codex_home.join("config.toml")).expect("read config");
|
||||
assert!(config.contains(r#"[plugins."cloudflare@my-plugins"]"#));
|
||||
assert!(config.contains("enabled = true"));
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn import_git_plugins_returns_pending_async_status() {
|
||||
let (_root, external_agent_home, codex_home) = fixture_paths();
|
||||
fs::create_dir_all(&external_agent_home).expect("create external agent home");
|
||||
fs::write(
|
||||
external_agent_home.join("settings.json"),
|
||||
r#"{
|
||||
"enabledPlugins": {
|
||||
"formatter@acme-tools": true
|
||||
},
|
||||
"extraKnownMarketplaces": {
|
||||
"acme-tools": {
|
||||
"source": "owner/debug-marketplace"
|
||||
}
|
||||
}
|
||||
}"#,
|
||||
)
|
||||
.expect("write settings");
|
||||
|
||||
let outcome = service_for_paths(external_agent_home, codex_home.clone())
|
||||
.import(vec![ExternalAgentConfigMigrationItem {
|
||||
item_type: ExternalAgentConfigMigrationItemType::Plugins,
|
||||
description: String::new(),
|
||||
cwd: None,
|
||||
details: Some(MigrationDetails {
|
||||
plugins: vec![PluginsMigration {
|
||||
marketplace_name: "acme-tools".to_string(),
|
||||
plugin_names: vec!["formatter".to_string()],
|
||||
}],
|
||||
}),
|
||||
}])
|
||||
.await
|
||||
.expect("import");
|
||||
|
||||
assert_eq!(
|
||||
outcome,
|
||||
vec![PendingPluginImport {
|
||||
cwd: None,
|
||||
details: MigrationDetails {
|
||||
plugins: vec![PluginsMigration {
|
||||
marketplace_name: "acme-tools".to_string(),
|
||||
plugin_names: vec!["formatter".to_string()],
|
||||
}],
|
||||
},
|
||||
}]
|
||||
);
|
||||
assert!(!codex_home.join("config.toml").exists());
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn detect_home_skips_config_when_target_already_has_supported_fields() {
|
||||
let (_root, external_agent_home, codex_home) = fixture_paths();
|
||||
|
||||
@@ -56,6 +56,16 @@ pub async fn add_marketplace(
|
||||
.map_err(|err| MarketplaceAddError::Internal(format!("failed to add marketplace: {err}")))?
|
||||
}
|
||||
|
||||
pub(crate) fn is_local_marketplace_source(
|
||||
source: &str,
|
||||
explicit_ref: Option<String>,
|
||||
) -> Result<bool, MarketplaceAddError> {
|
||||
Ok(matches!(
|
||||
parse_marketplace_source(source, explicit_ref)?,
|
||||
source::MarketplaceSource::Local { .. }
|
||||
))
|
||||
}
|
||||
|
||||
fn add_marketplace_sync(
|
||||
codex_home: &Path,
|
||||
request: MarketplaceAddRequest,
|
||||
|
||||
@@ -1,5 +1,5 @@
|
||||
use super::MarketplaceAddError;
|
||||
use super::MarketplaceSource;
|
||||
use super::source::MarketplaceSource;
|
||||
use crate::plugins::installed_marketplaces::resolve_configured_marketplace_root;
|
||||
use codex_config::CONFIG_TOML_FILE;
|
||||
use codex_config::MarketplaceConfigUpdate;
|
||||
|
||||
@@ -49,6 +49,7 @@ pub use marketplace_add::MarketplaceAddError;
|
||||
pub use marketplace_add::MarketplaceAddOutcome;
|
||||
pub use marketplace_add::MarketplaceAddRequest;
|
||||
pub use marketplace_add::add_marketplace;
|
||||
pub(crate) use marketplace_add::is_local_marketplace_source;
|
||||
pub(crate) use marketplace_add::parse_marketplace_source;
|
||||
pub(crate) use render::render_explicit_plugin_instructions;
|
||||
pub(crate) use render::render_plugins_section;
|
||||
|
||||
Reference in New Issue
Block a user