mirror of
https://github.com/openai/codex.git
synced 2026-11-09 08:40:33 +00:00
Respect config layers when removing plugin marketplaces (#40683)
## Why Removing a base-user marketplace must not delete a snapshot that is still referenced by another enabled configuration layer. ## What changed - Load the current configuration stack for marketplace commands in the CLI and app server. - Reject removal when a matching marketplace is defined by another enabled layer, and identify the configuration source that must be updated instead. - Allow cleanup of an installed marketplace snapshot even when no base-user entry exists. ## Testing Cover project, enterprise-managed, system, and session-flag configurations, as well as snapshot-only removal through the CLI and app server. GitOrigin-RevId: 4f0853a40c2b5fae0410696b79eee4284239c7a9
This commit is contained in:
1 parent
f1a6f7c9b3
commit
a6e63f9f32
7 files changed
+247
-41
No files matched your search
@@ -295,6 +295,12 @@ Example with notification opt-out:
|
|||||||
- `config/batchWrite` — apply multiple config edits atomically to the user's config.toml on disk, with optional `reloadUserConfig: true` to hot-reload loaded threads, including multiple `desktop.*` edits. Session-static model, reasoning-effort, Plan-mode reasoning-effort, service-tier, and personality defaults do not reload existing threads.
|
- `config/batchWrite` — apply multiple config edits atomically to the user's config.toml on disk, with optional `reloadUserConfig: true` to hot-reload loaded threads, including multiple `desktop.*` edits. Session-static model, reasoning-effort, Plan-mode reasoning-effort, service-tier, and personality defaults do not reload existing threads.
|
||||||
- `configRequirements/read` — fetch loaded requirements constraints from `requirements.toml` and/or MDM (or `null` if none are configured), including exact managed values (`cliAuthCredentialsStore`, `chatgptBaseUrl`, `sqliteHome`, `logDir`, `modelCatalogJson`, `checkForUpdateOnStartup`, `allowLoginShell`, `feedback.enabled`, and `windowsSandboxPrivateDesktop`), requirements-only developer instructions (`additionalDeveloperInstructions`, supplied independently of ordinary developer instructions), allow-lists (`allowedApprovalPolicies`, `allowedSandboxModes`, `allowedWebSearchModes`), the layered permission-profile allow map (`allowedPermissionProfiles`), the managed permission-profile default (`defaultPermissions`), lifecycle hook lockdown (`allowManagedHooksOnly`), remote-control policy (`allowRemoteControl`; `false` force-disables remote control while `true` or `null` preserves existing behavior), the Browser/Computer Use umbrella policy (`allowBrowserAndComputerUse`), computer use policy (`computerUse`, including persistent approval, default application access, and per-platform application rules), Browser Use policy (`browserUse`, including history access, origin rules, auto-review, and approval controls), interactive browser import policy (`inAppBrowser.allowExternalBrowserSettingsImport`), pinned feature values (`featureRequirements`, including the default-allowed `in_app_updates` policy that administrators can set to `false`), managed lifecycle hooks (`hooks`, including command handlers with optional `additionalContextLimit` and `mcp_tool` handlers with `server`, `tool`, `input`, `timeoutSec`, and `statusMessage`), `enforceResidency`, managed automatic review (`autoReview.requiredOnModels` and `autoReview.ignoreRules`), model defaults (`models.newThread.model`, `models.newThread.modelReasoningEffort`, and `models.newThread.serviceTier`), and `network` constraints such as canonical domain/socket permissions plus `managedAllowedDomainsOnly` and `dangerFullAccessDenylistOnly`.
|
- `configRequirements/read` — fetch loaded requirements constraints from `requirements.toml` and/or MDM (or `null` if none are configured), including exact managed values (`cliAuthCredentialsStore`, `chatgptBaseUrl`, `sqliteHome`, `logDir`, `modelCatalogJson`, `checkForUpdateOnStartup`, `allowLoginShell`, `feedback.enabled`, and `windowsSandboxPrivateDesktop`), requirements-only developer instructions (`additionalDeveloperInstructions`, supplied independently of ordinary developer instructions), allow-lists (`allowedApprovalPolicies`, `allowedSandboxModes`, `allowedWebSearchModes`), the layered permission-profile allow map (`allowedPermissionProfiles`), the managed permission-profile default (`defaultPermissions`), lifecycle hook lockdown (`allowManagedHooksOnly`), remote-control policy (`allowRemoteControl`; `false` force-disables remote control while `true` or `null` preserves existing behavior), the Browser/Computer Use umbrella policy (`allowBrowserAndComputerUse`), computer use policy (`computerUse`, including persistent approval, default application access, and per-platform application rules), Browser Use policy (`browserUse`, including history access, origin rules, auto-review, and approval controls), interactive browser import policy (`inAppBrowser.allowExternalBrowserSettingsImport`), pinned feature values (`featureRequirements`, including the default-allowed `in_app_updates` policy that administrators can set to `false`), managed lifecycle hooks (`hooks`, including command handlers with optional `additionalContextLimit` and `mcp_tool` handlers with `server`, `tool`, `input`, `timeoutSec`, and `statusMessage`), `enforceResidency`, managed automatic review (`autoReview.requiredOnModels` and `autoReview.ignoreRules`), model defaults (`models.newThread.model`, `models.newThread.modelReasoningEffort`, and `models.newThread.serviceTier`), and `network` constraints such as canonical domain/socket permissions plus `managedAllowedDomainsOnly` and `dangerFullAccessDenylistOnly`.
|
||||||
|
|
||||||
|
### Plugin configuration scope
|
||||||
|
|
||||||
|
`marketplace/remove` rejects removal when the marketplace name is defined in another
|
||||||
|
enabled layer of the operation's loaded config stack. Otherwise it removes the
|
||||||
|
snapshot and any base-user entry; a base-user entry is not required for cleanup.
|
||||||
|
|
||||||
### Example: Start or resume a thread
|
### Example: Start or resume a thread
|
||||||
|
|
||||||
Start a fresh thread when you need a new Codex conversation.
|
Start a fresh thread when you need a new Codex conversation.
|
||||||
|
|||||||
@@ -51,8 +51,10 @@ impl MarketplaceRequestProcessor {
|
|||||||
&self,
|
&self,
|
||||||
params: MarketplaceRemoveParams,
|
params: MarketplaceRemoveParams,
|
||||||
) -> Result<MarketplaceRemoveResponse, JSONRPCErrorError> {
|
) -> Result<MarketplaceRemoveResponse, JSONRPCErrorError> {
|
||||||
|
let config = self.load_latest_config(/*fallback_cwd*/ None).await?;
|
||||||
remove_marketplace(
|
remove_marketplace(
|
||||||
self.config.codex_home.to_path_buf(),
|
config.codex_home.to_path_buf(),
|
||||||
|
config.config_layer_stack,
|
||||||
CoreMarketplaceRemoveRequest {
|
CoreMarketplaceRemoveRequest {
|
||||||
marketplace_name: params.marketplace_name,
|
marketplace_name: params.marketplace_name,
|
||||||
},
|
},
|
||||||
|
|||||||
@@ -9,9 +9,12 @@ use codex_app_server_protocol::MarketplaceRemoveResponse;
|
|||||||
use codex_app_server_protocol::RequestId;
|
use codex_app_server_protocol::RequestId;
|
||||||
use codex_config::MarketplaceConfigUpdate;
|
use codex_config::MarketplaceConfigUpdate;
|
||||||
use codex_config::record_user_marketplace;
|
use codex_config::record_user_marketplace;
|
||||||
|
use codex_core::config::set_project_trust_level;
|
||||||
use codex_core_plugins::installed_marketplaces::marketplace_install_root;
|
use codex_core_plugins::installed_marketplaces::marketplace_install_root;
|
||||||
|
use codex_protocol::config_types::TrustLevel;
|
||||||
use pretty_assertions::assert_eq;
|
use pretty_assertions::assert_eq;
|
||||||
use tempfile::TempDir;
|
use tempfile::TempDir;
|
||||||
|
use test_case::test_case;
|
||||||
use tokio::time::timeout;
|
use tokio::time::timeout;
|
||||||
|
|
||||||
const DEFAULT_TIMEOUT: Duration = Duration::from_secs(10);
|
const DEFAULT_TIMEOUT: Duration = Duration::from_secs(10);
|
||||||
@@ -43,16 +46,19 @@ fn canonicalize_path_with_existing_parent(path: &std::path::Path) -> Result<std:
|
|||||||
Ok(parent.canonicalize()?.join(file_name))
|
Ok(parent.canonicalize()?.join(file_name))
|
||||||
}
|
}
|
||||||
|
|
||||||
|
#[test_case(false; "snapshot only")]
|
||||||
|
#[test_case(true; "user entry and snapshot")]
|
||||||
#[tokio::test]
|
#[tokio::test]
|
||||||
async fn marketplace_remove_deletes_config_and_installed_root() -> Result<()> {
|
async fn marketplace_remove_deletes_config_and_installed_root(user_entry: bool) -> Result<()> {
|
||||||
let codex_home = TempDir::new()?;
|
let codex_home = TempDir::new()?;
|
||||||
|
if user_entry {
|
||||||
record_user_marketplace(codex_home.path(), "debug", &configured_marketplace_update())?;
|
record_user_marketplace(codex_home.path(), "debug", &configured_marketplace_update())?;
|
||||||
|
}
|
||||||
write_installed_marketplace(codex_home.path(), "debug")?;
|
write_installed_marketplace(codex_home.path(), "debug")?;
|
||||||
let installed_root = marketplace_install_root(codex_home.path()).join("debug");
|
let installed_root = marketplace_install_root(codex_home.path()).join("debug");
|
||||||
|
|
||||||
let mut mcp = TestAppServer::builder()
|
let mut mcp = TestAppServer::builder()
|
||||||
.with_codex_home(codex_home.path())
|
.with_codex_home(codex_home.path())
|
||||||
.without_auto_env()
|
|
||||||
.build_initialized()
|
.build_initialized()
|
||||||
.await?;
|
.await?;
|
||||||
let response: MarketplaceRemoveResponse = mcp
|
let response: MarketplaceRemoveResponse = mcp
|
||||||
@@ -72,8 +78,13 @@ async fn marketplace_remove_deletes_config_and_installed_root() -> Result<()> {
|
|||||||
canonicalize_path_with_existing_parent(&installed_root)?,
|
canonicalize_path_with_existing_parent(&installed_root)?,
|
||||||
);
|
);
|
||||||
|
|
||||||
let config = std::fs::read_to_string(codex_home.path().join("config.toml"))?;
|
let config_path = codex_home.path().join("config.toml");
|
||||||
|
if user_entry {
|
||||||
|
let config = std::fs::read_to_string(config_path)?;
|
||||||
assert!(!config.contains("[marketplaces.debug]"));
|
assert!(!config.contains("[marketplaces.debug]"));
|
||||||
|
} else {
|
||||||
|
assert!(!config_path.exists());
|
||||||
|
}
|
||||||
assert!(
|
assert!(
|
||||||
!marketplace_install_root(codex_home.path())
|
!marketplace_install_root(codex_home.path())
|
||||||
.join("debug")
|
.join("debug")
|
||||||
@@ -111,3 +122,54 @@ async fn marketplace_remove_rejects_unknown_marketplace() -> Result<()> {
|
|||||||
);
|
);
|
||||||
Ok(())
|
Ok(())
|
||||||
}
|
}
|
||||||
|
|
||||||
|
#[test_case(false; "project only")]
|
||||||
|
#[test_case(true; "project and user")]
|
||||||
|
#[tokio::test]
|
||||||
|
async fn marketplace_remove_preserves_project_marketplace(user_entry: bool) -> Result<()> {
|
||||||
|
let codex_home = TempDir::new()?;
|
||||||
|
if user_entry {
|
||||||
|
record_user_marketplace(codex_home.path(), "debug", &configured_marketplace_update())?;
|
||||||
|
}
|
||||||
|
// TestAppServer starts in CODEX_HOME, so make it a trusted project as well.
|
||||||
|
std::fs::create_dir_all(codex_home.path().join(".git"))?;
|
||||||
|
std::fs::create_dir_all(codex_home.path().join(".codex"))?;
|
||||||
|
let project_config_path = codex_home.path().join(".codex/config.toml");
|
||||||
|
let project_config = "[marketplaces.debug]\nsource_type = \"git\"\nsource = \"https://github.com/owner/repo.git\"\n";
|
||||||
|
std::fs::write(&project_config_path, project_config)?;
|
||||||
|
set_project_trust_level(codex_home.path(), codex_home.path(), TrustLevel::Trusted)?;
|
||||||
|
write_installed_marketplace(codex_home.path(), "debug")?;
|
||||||
|
let snapshot_path =
|
||||||
|
marketplace_install_root(codex_home.path()).join("debug/.agents/plugins/marketplace.json");
|
||||||
|
let user_config_path = codex_home.path().join("config.toml");
|
||||||
|
|
||||||
|
let mut mcp = TestAppServer::builder()
|
||||||
|
.with_codex_home(codex_home.path())
|
||||||
|
.build_initialized()
|
||||||
|
.await?;
|
||||||
|
let user_config = std::fs::read_to_string(&user_config_path)?;
|
||||||
|
let request_id = mcp
|
||||||
|
.send_marketplace_remove_request(MarketplaceRemoveParams {
|
||||||
|
marketplace_name: "debug".to_string(),
|
||||||
|
})
|
||||||
|
.await?;
|
||||||
|
let err = timeout(
|
||||||
|
DEFAULT_TIMEOUT,
|
||||||
|
mcp.read_stream_until_error_message(RequestId::Integer(request_id)),
|
||||||
|
)
|
||||||
|
.await??;
|
||||||
|
|
||||||
|
assert_eq!(err.error.code, -32600);
|
||||||
|
assert!(
|
||||||
|
err.error
|
||||||
|
.message
|
||||||
|
.starts_with("marketplace `debug` is configured in project (")
|
||||||
|
);
|
||||||
|
assert_eq!(std::fs::read_to_string(user_config_path)?, user_config);
|
||||||
|
assert_eq!(
|
||||||
|
std::fs::read_to_string(project_config_path)?,
|
||||||
|
project_config
|
||||||
|
);
|
||||||
|
assert_eq!(std::fs::read_to_string(snapshot_path)?, "{}");
|
||||||
|
Ok(())
|
||||||
|
}
|
||||||
@@ -3,7 +3,7 @@ use anyhow::Result;
|
|||||||
use anyhow::bail;
|
use anyhow::bail;
|
||||||
use clap::Parser;
|
use clap::Parser;
|
||||||
use codex_core::config::Config;
|
use codex_core::config::Config;
|
||||||
use codex_core::config::find_codex_home;
|
use codex_core::config::LoaderOverrides;
|
||||||
use codex_core::plugins_manager_for_config;
|
use codex_core::plugins_manager_for_config;
|
||||||
use codex_core_plugins::PluginMarketplaceUpgradeOutcome;
|
use codex_core_plugins::PluginMarketplaceUpgradeOutcome;
|
||||||
use codex_core_plugins::PluginsConfigInput;
|
use codex_core_plugins::PluginsConfigInput;
|
||||||
@@ -23,6 +23,7 @@ use std::collections::HashSet;
|
|||||||
use std::path::Path;
|
use std::path::Path;
|
||||||
use std::path::PathBuf;
|
use std::path::PathBuf;
|
||||||
|
|
||||||
|
use crate::cloud_config;
|
||||||
use crate::plugin_cmd::JsonMarketplaceSource;
|
use crate::plugin_cmd::JsonMarketplaceSource;
|
||||||
use crate::plugin_cmd::configured_marketplace_snapshot_issues;
|
use crate::plugin_cmd::configured_marketplace_snapshot_issues;
|
||||||
use crate::plugin_cmd::configured_marketplace_sources;
|
use crate::plugin_cmd::configured_marketplace_sources;
|
||||||
@@ -127,22 +128,21 @@ impl MarketplaceCli {
|
|||||||
subcommand,
|
subcommand,
|
||||||
} = self;
|
} = self;
|
||||||
|
|
||||||
let overrides = config_overrides
|
let config =
|
||||||
.parse_overrides()
|
cloud_config::load_config(&config_overrides, LoaderOverrides::default()).await?;
|
||||||
.map_err(anyhow::Error::msg)?;
|
|
||||||
|
|
||||||
match subcommand {
|
match subcommand {
|
||||||
MarketplaceSubcommand::Add(args) => run_add(overrides, args).await?,
|
MarketplaceSubcommand::Add(args) => run_add(config, args).await?,
|
||||||
MarketplaceSubcommand::List(args) => run_list(overrides, args).await?,
|
MarketplaceSubcommand::List(args) => run_list(config, args).await?,
|
||||||
MarketplaceSubcommand::Upgrade(args) => run_upgrade(overrides, args).await?,
|
MarketplaceSubcommand::Upgrade(args) => run_upgrade(config, args).await?,
|
||||||
MarketplaceSubcommand::Remove(args) => run_remove(args).await?,
|
MarketplaceSubcommand::Remove(args) => run_remove(config, args).await?,
|
||||||
}
|
}
|
||||||
|
|
||||||
Ok(())
|
Ok(())
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
async fn run_add(overrides: Vec<(String, toml::Value)>, args: AddMarketplaceArgs) -> Result<()> {
|
async fn run_add(config: Config, args: AddMarketplaceArgs) -> Result<()> {
|
||||||
let AddMarketplaceArgs {
|
let AddMarketplaceArgs {
|
||||||
source,
|
source,
|
||||||
ref_name,
|
ref_name,
|
||||||
@@ -150,9 +150,6 @@ async fn run_add(overrides: Vec<(String, toml::Value)>, args: AddMarketplaceArgs
|
|||||||
json,
|
json,
|
||||||
} = args;
|
} = args;
|
||||||
|
|
||||||
let config = Config::load_with_cli_overrides(overrides)
|
|
||||||
.await
|
|
||||||
.context("failed to load configuration")?;
|
|
||||||
let outcome = add_marketplace(
|
let outcome = add_marketplace(
|
||||||
config.codex_home.to_path_buf(),
|
config.codex_home.to_path_buf(),
|
||||||
config.config_layer_stack.requirements().clone(),
|
config.config_layer_stack.requirements().clone(),
|
||||||
@@ -207,10 +204,7 @@ impl JsonMarketplaceAddOutput {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
async fn run_list(overrides: Vec<(String, toml::Value)>, args: ListMarketplaceArgs) -> Result<()> {
|
async fn run_list(config: Config, args: ListMarketplaceArgs) -> Result<()> {
|
||||||
let config = Config::load_with_cli_overrides(overrides)
|
|
||||||
.await
|
|
||||||
.context("failed to load configuration")?;
|
|
||||||
let manager = plugins_manager_for_config(&config, load_cli_auth_manager(&config).await?);
|
let manager = plugins_manager_for_config(&config, load_cli_auth_manager(&config).await?);
|
||||||
let plugins_input = config.plugins_config_input();
|
let plugins_input = config.plugins_config_input();
|
||||||
let marketplace_listing = manager
|
let marketplace_listing = manager
|
||||||
@@ -366,17 +360,11 @@ fn configured_marketplace_sources_by_root(
|
|||||||
.collect()
|
.collect()
|
||||||
}
|
}
|
||||||
|
|
||||||
async fn run_upgrade(
|
async fn run_upgrade(config: Config, args: UpgradeMarketplaceArgs) -> Result<()> {
|
||||||
overrides: Vec<(String, toml::Value)>,
|
|
||||||
args: UpgradeMarketplaceArgs,
|
|
||||||
) -> Result<()> {
|
|
||||||
let UpgradeMarketplaceArgs {
|
let UpgradeMarketplaceArgs {
|
||||||
marketplace_name,
|
marketplace_name,
|
||||||
json,
|
json,
|
||||||
} = args;
|
} = args;
|
||||||
let config = Config::load_with_cli_overrides(overrides)
|
|
||||||
.await
|
|
||||||
.context("failed to load configuration")?;
|
|
||||||
let manager = plugins_manager_for_config(&config, load_cli_auth_manager(&config).await?);
|
let manager = plugins_manager_for_config(&config, load_cli_auth_manager(&config).await?);
|
||||||
let plugins_input = config.plugins_config_input();
|
let plugins_input = config.plugins_config_input();
|
||||||
let outcome = manager
|
let outcome = manager
|
||||||
@@ -389,14 +377,14 @@ async fn run_upgrade(
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
async fn run_remove(args: RemoveMarketplaceArgs) -> Result<()> {
|
async fn run_remove(config: Config, args: RemoveMarketplaceArgs) -> Result<()> {
|
||||||
let RemoveMarketplaceArgs {
|
let RemoveMarketplaceArgs {
|
||||||
marketplace_name,
|
marketplace_name,
|
||||||
json,
|
json,
|
||||||
} = args;
|
} = args;
|
||||||
let codex_home = find_codex_home().context("failed to resolve CODEX_HOME")?;
|
|
||||||
let outcome = remove_marketplace(
|
let outcome = remove_marketplace(
|
||||||
codex_home.to_path_buf(),
|
config.codex_home.to_path_buf(),
|
||||||
|
config.config_layer_stack,
|
||||||
MarketplaceRemoveRequest { marketplace_name },
|
MarketplaceRemoveRequest { marketplace_name },
|
||||||
)
|
)
|
||||||
.await?;
|
.await?;
|
||||||
|
|||||||
@@ -11,6 +11,7 @@ use codex_config::ConfigLoadOptions;
|
|||||||
use codex_config::types::AuthCredentialsStoreMode;
|
use codex_config::types::AuthCredentialsStoreMode;
|
||||||
use codex_core::config::load_config_toml_with_layer_stack;
|
use codex_core::config::load_config_toml_with_layer_stack;
|
||||||
use codex_core::config::load_global_mcp_servers;
|
use codex_core::config::load_global_mcp_servers;
|
||||||
|
use codex_core_plugins::installed_marketplaces::marketplace_install_root;
|
||||||
use codex_utils_absolute_path::AbsolutePathBuf;
|
use codex_utils_absolute_path::AbsolutePathBuf;
|
||||||
use pretty_assertions::assert_eq;
|
use pretty_assertions::assert_eq;
|
||||||
use serde_json::Value;
|
use serde_json::Value;
|
||||||
@@ -37,14 +38,14 @@ const MANAGED_SCOPE: &str = "managed.read";
|
|||||||
const MOCK_ACCESS_TOKEN: &str = "mock-managed-access-token";
|
const MOCK_ACCESS_TOKEN: &str = "mock-managed-access-token";
|
||||||
const MOCK_REFRESH_TOKEN: &str = "mock-managed-refresh-token";
|
const MOCK_REFRESH_TOKEN: &str = "mock-managed-refresh-token";
|
||||||
|
|
||||||
struct CloudManagedMcpFixture {
|
struct CloudManagedConfigFixture {
|
||||||
server: MockServer,
|
server: MockServer,
|
||||||
codex_home: TempDir,
|
codex_home: TempDir,
|
||||||
user_config: String,
|
user_config: String,
|
||||||
mcp_url: String,
|
mcp_url: String,
|
||||||
}
|
}
|
||||||
|
|
||||||
impl CloudManagedMcpFixture {
|
impl CloudManagedConfigFixture {
|
||||||
async fn new() -> Result<Option<Self>> {
|
async fn new() -> Result<Option<Self>> {
|
||||||
let server = MockServer::start().await;
|
let server = MockServer::start().await;
|
||||||
let chatgpt_base_url = format!("{}/backend-api", server.uri());
|
let chatgpt_base_url = format!("{}/backend-api", server.uri());
|
||||||
@@ -67,7 +68,7 @@ impl CloudManagedMcpFixture {
|
|||||||
!= Some(chatgpt_base_url.as_str())
|
!= Some(chatgpt_base_url.as_str())
|
||||||
{
|
{
|
||||||
eprintln!(
|
eprintln!(
|
||||||
"skipping cloud-managed MCP subprocess: host-managed authentication or backend routing prevents isolated mock credentials"
|
"skipping cloud-managed subprocess: host-managed authentication or backend routing prevents isolated mock credentials"
|
||||||
);
|
);
|
||||||
return Ok(None);
|
return Ok(None);
|
||||||
}
|
}
|
||||||
@@ -90,7 +91,10 @@ impl CloudManagedMcpFixture {
|
|||||||
auth = \"oauth\"\n\
|
auth = \"oauth\"\n\
|
||||||
scopes = [\"{MANAGED_SCOPE}\"]\n\n\
|
scopes = [\"{MANAGED_SCOPE}\"]\n\n\
|
||||||
[mcp_servers.{MANAGED_SERVER_NAME}.oauth]\n\
|
[mcp_servers.{MANAGED_SERVER_NAME}.oauth]\n\
|
||||||
client_id = \"{MANAGED_CLIENT_ID}\"\n"
|
client_id = \"{MANAGED_CLIENT_ID}\"\n\n\
|
||||||
|
[marketplaces.managed]\n\
|
||||||
|
source_type = \"git\"\n\
|
||||||
|
source = \"https://github.com/owner/repo.git\"\n"
|
||||||
);
|
);
|
||||||
Mock::given(method("GET"))
|
Mock::given(method("GET"))
|
||||||
.and(path("/backend-api/wham/config/bundle"))
|
.and(path("/backend-api/wham/config/bundle"))
|
||||||
@@ -99,8 +103,8 @@ impl CloudManagedMcpFixture {
|
|||||||
.respond_with(ResponseTemplate::new(200).set_body_json(json!({
|
.respond_with(ResponseTemplate::new(200).set_body_json(json!({
|
||||||
"config_toml": {
|
"config_toml": {
|
||||||
"enterprise_managed": [{
|
"enterprise_managed": [{
|
||||||
"id": "managed-mcp-config",
|
"id": "managed-config",
|
||||||
"name": "Managed MCP servers",
|
"name": "Managed resources",
|
||||||
"contents": managed_config,
|
"contents": managed_config,
|
||||||
}],
|
}],
|
||||||
},
|
},
|
||||||
@@ -156,7 +160,7 @@ impl CloudManagedMcpFixture {
|
|||||||
|
|
||||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||||
async fn list_and_get_resolve_cloud_managed_mcp_without_writing_user_config() -> Result<()> {
|
async fn list_and_get_resolve_cloud_managed_mcp_without_writing_user_config() -> Result<()> {
|
||||||
let Some(fixture) = CloudManagedMcpFixture::new().await? else {
|
let Some(fixture) = CloudManagedConfigFixture::new().await? else {
|
||||||
return Ok(());
|
return Ok(());
|
||||||
};
|
};
|
||||||
|
|
||||||
@@ -187,7 +191,7 @@ async fn list_and_get_resolve_cloud_managed_mcp_without_writing_user_config() ->
|
|||||||
|
|
||||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||||
async fn login_and_logout_persist_only_cloud_managed_mcp_oauth_credentials() -> Result<()> {
|
async fn login_and_logout_persist_only_cloud_managed_mcp_oauth_credentials() -> Result<()> {
|
||||||
let Some(fixture) = CloudManagedMcpFixture::new().await? else {
|
let Some(fixture) = CloudManagedConfigFixture::new().await? else {
|
||||||
return Ok(());
|
return Ok(());
|
||||||
};
|
};
|
||||||
|
|
||||||
@@ -382,11 +386,29 @@ async fn login_and_logout_persist_only_cloud_managed_mcp_oauth_credentials() ->
|
|||||||
}
|
}
|
||||||
|
|
||||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||||
async fn add_and_remove_do_not_copy_or_delete_cloud_managed_mcp_servers() -> Result<()> {
|
async fn add_and_remove_preserve_cloud_managed_resources() -> Result<()> {
|
||||||
let Some(fixture) = CloudManagedMcpFixture::new().await? else {
|
let Some(fixture) = CloudManagedConfigFixture::new().await? else {
|
||||||
return Ok(());
|
return Ok(());
|
||||||
};
|
};
|
||||||
|
|
||||||
|
let installed_root = marketplace_install_root(fixture.codex_home.path()).join("managed");
|
||||||
|
std::fs::create_dir_all(&installed_root)?;
|
||||||
|
let marker = installed_root.join("marker.txt");
|
||||||
|
std::fs::write(&marker, "installed")?;
|
||||||
|
// Exercise both the initial fetch and the cached bundle without a user entry.
|
||||||
|
for _ in 0..2 {
|
||||||
|
let output = fixture
|
||||||
|
.command(&["plugin", "marketplace", "remove", "managed"])?
|
||||||
|
.output()
|
||||||
|
.await?;
|
||||||
|
assert!(!output.status.success());
|
||||||
|
assert!(String::from_utf8(output.stderr)?.contains(
|
||||||
|
"marketplace `managed` is configured in enterprise-managed (Managed resources, managed-config); remove it from that configuration source instead"
|
||||||
|
));
|
||||||
|
fixture.assert_user_config_unchanged()?;
|
||||||
|
assert_eq!(std::fs::read_to_string(&marker)?, "installed");
|
||||||
|
}
|
||||||
|
|
||||||
fixture
|
fixture
|
||||||
.output(&["mcp", "get", MANAGED_SERVER_NAME, "--json"])
|
.output(&["mcp", "get", MANAGED_SERVER_NAME, "--json"])
|
||||||
.await?;
|
.await?;
|
||||||
@@ -95,3 +95,33 @@ async fn marketplace_remove_rejects_unknown_marketplace() -> Result<()> {
|
|||||||
|
|
||||||
Ok(())
|
Ok(())
|
||||||
}
|
}
|
||||||
|
|
||||||
|
#[tokio::test]
|
||||||
|
async fn marketplace_remove_preserves_marketplace_referenced_by_session_config() -> Result<()> {
|
||||||
|
let codex_home = TempDir::new()?;
|
||||||
|
record_user_marketplace(codex_home.path(), "debug", &configured_marketplace_update())?;
|
||||||
|
write_installed_marketplace(codex_home.path(), "debug")?;
|
||||||
|
let config_path = codex_home.path().join("config.toml");
|
||||||
|
let config = std::fs::read_to_string(&config_path)?;
|
||||||
|
|
||||||
|
codex_command(codex_home.path())?
|
||||||
|
.args([
|
||||||
|
"plugin", "marketplace",
|
||||||
|
"-c", "marketplaces.debug.source_type=\"git\"",
|
||||||
|
"remove", "debug",
|
||||||
|
])
|
||||||
|
.assert()
|
||||||
|
.failure()
|
||||||
|
.stderr(contains(
|
||||||
|
"marketplace `debug` is configured in session-flags; remove it from that configuration source instead",
|
||||||
|
));
|
||||||
|
|
||||||
|
assert_eq!(std::fs::read_to_string(config_path)?, config);
|
||||||
|
assert_eq!(
|
||||||
|
std::fs::read_to_string(
|
||||||
|
marketplace_install_root(codex_home.path()).join("debug/marker.txt")
|
||||||
|
)?,
|
||||||
|
"installed",
|
||||||
|
);
|
||||||
|
Ok(())
|
||||||
|
}
|
||||||
@@ -1,5 +1,9 @@
|
|||||||
use crate::installed_marketplaces::marketplace_install_root;
|
use crate::installed_marketplaces::marketplace_install_root;
|
||||||
|
use codex_config::CONFIG_TOML_FILE;
|
||||||
|
use codex_config::ConfigLayerSource;
|
||||||
|
use codex_config::ConfigLayerStack;
|
||||||
use codex_config::RemoveMarketplaceConfigOutcome;
|
use codex_config::RemoveMarketplaceConfigOutcome;
|
||||||
|
use codex_config::format_config_layer_source;
|
||||||
use codex_config::remove_user_marketplace_config;
|
use codex_config::remove_user_marketplace_config;
|
||||||
use codex_plugin::validate_plugin_segment;
|
use codex_plugin::validate_plugin_segment;
|
||||||
use codex_utils_absolute_path::AbsolutePathBuf;
|
use codex_utils_absolute_path::AbsolutePathBuf;
|
||||||
@@ -26,11 +30,16 @@ pub enum MarketplaceRemoveError {
|
|||||||
Internal(String),
|
Internal(String),
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// Removes a marketplace's snapshot and any base-user entry only when no other
|
||||||
|
/// enabled layer in the current operation's config stack references its name.
|
||||||
pub async fn remove_marketplace(
|
pub async fn remove_marketplace(
|
||||||
codex_home: PathBuf,
|
codex_home: PathBuf,
|
||||||
|
config_layer_stack: ConfigLayerStack,
|
||||||
request: MarketplaceRemoveRequest,
|
request: MarketplaceRemoveRequest,
|
||||||
) -> Result<MarketplaceRemoveOutcome, MarketplaceRemoveError> {
|
) -> Result<MarketplaceRemoveOutcome, MarketplaceRemoveError> {
|
||||||
tokio::task::spawn_blocking(move || remove_marketplace_sync(codex_home.as_path(), request))
|
tokio::task::spawn_blocking(move || {
|
||||||
|
remove_marketplace_sync(codex_home.as_path(), &config_layer_stack, request)
|
||||||
|
})
|
||||||
.await
|
.await
|
||||||
.map_err(|err| {
|
.map_err(|err| {
|
||||||
MarketplaceRemoveError::Internal(format!("failed to remove marketplace: {err}"))
|
MarketplaceRemoveError::Internal(format!("failed to remove marketplace: {err}"))
|
||||||
@@ -39,12 +48,36 @@ pub async fn remove_marketplace(
|
|||||||
|
|
||||||
fn remove_marketplace_sync(
|
fn remove_marketplace_sync(
|
||||||
codex_home: &Path,
|
codex_home: &Path,
|
||||||
|
config_layer_stack: &ConfigLayerStack,
|
||||||
request: MarketplaceRemoveRequest,
|
request: MarketplaceRemoveRequest,
|
||||||
) -> Result<MarketplaceRemoveOutcome, MarketplaceRemoveError> {
|
) -> Result<MarketplaceRemoveOutcome, MarketplaceRemoveError> {
|
||||||
let marketplace_name = request.marketplace_name;
|
let marketplace_name = request.marketplace_name;
|
||||||
validate_plugin_segment(&marketplace_name, "marketplace name")
|
validate_plugin_segment(&marketplace_name, "marketplace name")
|
||||||
.map_err(MarketplaceRemoveError::InvalidRequest)?;
|
.map_err(MarketplaceRemoveError::InvalidRequest)?;
|
||||||
|
|
||||||
|
// Removing a user override must not delete a snapshot still referenced by
|
||||||
|
// another enabled layer, including a lower-precedence layer it shadows.
|
||||||
|
let user_config_file = AbsolutePathBuf::resolve_path_against_base(CONFIG_TOML_FILE, codex_home);
|
||||||
|
if let Some(layer) = config_layer_stack.layers_high_to_low().find(|layer| {
|
||||||
|
!matches!(
|
||||||
|
&layer.name,
|
||||||
|
ConfigLayerSource::User { file, profile: None } if file == &user_config_file
|
||||||
|
) && layer
|
||||||
|
.config
|
||||||
|
.get("marketplaces")
|
||||||
|
.and_then(toml::Value::as_table)
|
||||||
|
.is_some_and(|marketplaces| {
|
||||||
|
marketplaces
|
||||||
|
.keys()
|
||||||
|
.any(|name| name.eq_ignore_ascii_case(&marketplace_name))
|
||||||
|
})
|
||||||
|
}) {
|
||||||
|
let source = format_config_layer_source(&layer.name, CONFIG_TOML_FILE);
|
||||||
|
return Err(MarketplaceRemoveError::InvalidRequest(format!(
|
||||||
|
"marketplace `{marketplace_name}` is configured in {source}; remove it from that configuration source instead"
|
||||||
|
)));
|
||||||
|
}
|
||||||
|
|
||||||
let destination = marketplace_install_root(codex_home).join(&marketplace_name);
|
let destination = marketplace_install_root(codex_home).join(&marketplace_name);
|
||||||
let config_outcome =
|
let config_outcome =
|
||||||
remove_user_marketplace_config(codex_home, &marketplace_name).map_err(|err| {
|
remove_user_marketplace_config(codex_home, &marketplace_name).map_err(|err| {
|
||||||
@@ -107,6 +140,7 @@ fn remove_marketplace_root(root: &Path) -> Result<Option<AbsolutePathBuf>, Marke
|
|||||||
#[cfg(test)]
|
#[cfg(test)]
|
||||||
mod tests {
|
mod tests {
|
||||||
use super::*;
|
use super::*;
|
||||||
|
use codex_config::ConfigLayerEntry;
|
||||||
use codex_config::MarketplaceConfigUpdate;
|
use codex_config::MarketplaceConfigUpdate;
|
||||||
use codex_config::record_user_marketplace;
|
use codex_config::record_user_marketplace;
|
||||||
use pretty_assertions::assert_eq;
|
use pretty_assertions::assert_eq;
|
||||||
@@ -136,6 +170,7 @@ mod tests {
|
|||||||
|
|
||||||
let outcome = remove_marketplace_sync(
|
let outcome = remove_marketplace_sync(
|
||||||
codex_home.path(),
|
codex_home.path(),
|
||||||
|
&ConfigLayerStack::default(),
|
||||||
MarketplaceRemoveRequest {
|
MarketplaceRemoveRequest {
|
||||||
marketplace_name: "debug".to_string(),
|
marketplace_name: "debug".to_string(),
|
||||||
},
|
},
|
||||||
@@ -159,6 +194,7 @@ mod tests {
|
|||||||
|
|
||||||
let err = remove_marketplace_sync(
|
let err = remove_marketplace_sync(
|
||||||
codex_home.path(),
|
codex_home.path(),
|
||||||
|
&ConfigLayerStack::default(),
|
||||||
MarketplaceRemoveRequest {
|
MarketplaceRemoveRequest {
|
||||||
marketplace_name: "debug".to_string(),
|
marketplace_name: "debug".to_string(),
|
||||||
},
|
},
|
||||||
@@ -171,6 +207,62 @@ mod tests {
|
|||||||
);
|
);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn remove_marketplace_sync_preserves_shadowed_system_marketplace() {
|
||||||
|
let codex_home = TempDir::new().unwrap();
|
||||||
|
let config = "[marketplaces.debug]\nsource_type = \"git\"\nsource = \"https://github.com/owner/repo.git\"\n";
|
||||||
|
let user_file =
|
||||||
|
AbsolutePathBuf::resolve_path_against_base(CONFIG_TOML_FILE, codex_home.path());
|
||||||
|
let system_file =
|
||||||
|
AbsolutePathBuf::resolve_path_against_base("system.toml", codex_home.path());
|
||||||
|
fs::write(&user_file, config).unwrap();
|
||||||
|
fs::write(&system_file, config).unwrap();
|
||||||
|
let installed_root = marketplace_install_root(codex_home.path()).join("debug");
|
||||||
|
fs::create_dir_all(&installed_root).unwrap();
|
||||||
|
let marker = installed_root.join("marker.txt");
|
||||||
|
fs::write(&marker, "installed").unwrap();
|
||||||
|
let stack = ConfigLayerStack::new(
|
||||||
|
vec![
|
||||||
|
ConfigLayerEntry::new(
|
||||||
|
ConfigLayerSource::System {
|
||||||
|
file: system_file.clone(),
|
||||||
|
},
|
||||||
|
toml::from_str(config).unwrap(),
|
||||||
|
),
|
||||||
|
ConfigLayerEntry::new(
|
||||||
|
ConfigLayerSource::User {
|
||||||
|
file: user_file.clone(),
|
||||||
|
profile: None,
|
||||||
|
},
|
||||||
|
toml::from_str(config).unwrap(),
|
||||||
|
),
|
||||||
|
],
|
||||||
|
Default::default(),
|
||||||
|
Default::default(),
|
||||||
|
)
|
||||||
|
.unwrap();
|
||||||
|
|
||||||
|
let err = remove_marketplace_sync(
|
||||||
|
codex_home.path(),
|
||||||
|
&stack,
|
||||||
|
MarketplaceRemoveRequest {
|
||||||
|
marketplace_name: "debug".to_string(),
|
||||||
|
},
|
||||||
|
)
|
||||||
|
.unwrap_err();
|
||||||
|
|
||||||
|
assert_eq!(
|
||||||
|
err.to_string(),
|
||||||
|
format!(
|
||||||
|
"marketplace `debug` is configured in system ({}); remove it from that configuration source instead",
|
||||||
|
system_file.display()
|
||||||
|
),
|
||||||
|
);
|
||||||
|
assert_eq!(fs::read_to_string(user_file).unwrap(), config);
|
||||||
|
assert_eq!(fs::read_to_string(system_file).unwrap(), config);
|
||||||
|
assert_eq!(fs::read_to_string(marker).unwrap(), "installed");
|
||||||
|
}
|
||||||
|
|
||||||
#[test]
|
#[test]
|
||||||
fn remove_marketplace_sync_rejects_case_mismatched_configured_name() {
|
fn remove_marketplace_sync_rejects_case_mismatched_configured_name() {
|
||||||
let codex_home = TempDir::new().unwrap();
|
let codex_home = TempDir::new().unwrap();
|
||||||
@@ -190,6 +282,7 @@ mod tests {
|
|||||||
|
|
||||||
let err = remove_marketplace_sync(
|
let err = remove_marketplace_sync(
|
||||||
codex_home.path(),
|
codex_home.path(),
|
||||||
|
&ConfigLayerStack::default(),
|
||||||
MarketplaceRemoveRequest {
|
MarketplaceRemoveRequest {
|
||||||
marketplace_name: "Debug".to_string(),
|
marketplace_name: "Debug".to_string(),
|
||||||
},
|
},
|
||||||
@@ -219,6 +312,7 @@ mod tests {
|
|||||||
|
|
||||||
let err = remove_marketplace_sync(
|
let err = remove_marketplace_sync(
|
||||||
codex_home.path(),
|
codex_home.path(),
|
||||||
|
&ConfigLayerStack::default(),
|
||||||
MarketplaceRemoveRequest {
|
MarketplaceRemoveRequest {
|
||||||
marketplace_name: "debug".to_string(),
|
marketplace_name: "debug".to_string(),
|
||||||
},
|
},
|
||||||
@@ -252,6 +346,7 @@ mod tests {
|
|||||||
|
|
||||||
let outcome = remove_marketplace_sync(
|
let outcome = remove_marketplace_sync(
|
||||||
codex_home.path(),
|
codex_home.path(),
|
||||||
|
&ConfigLayerStack::default(),
|
||||||
MarketplaceRemoveRequest {
|
MarketplaceRemoveRequest {
|
||||||
marketplace_name: "debug".to_string(),
|
marketplace_name: "debug".to_string(),
|
||||||
},
|
},
|
||||||
@@ -288,6 +383,7 @@ marketplaces = { debug = { source_type = "git", source = "https://github.com/own
|
|||||||
|
|
||||||
let outcome = remove_marketplace_sync(
|
let outcome = remove_marketplace_sync(
|
||||||
codex_home.path(),
|
codex_home.path(),
|
||||||
|
&ConfigLayerStack::default(),
|
||||||
MarketplaceRemoveRequest {
|
MarketplaceRemoveRequest {
|
||||||
marketplace_name: "debug".to_string(),
|
marketplace_name: "debug".to_string(),
|
||||||
},
|
},
|
||||||
|
|||||||
Reference in new issue
Block a user