mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
[plugins] Add marketplace source requirements (#29690)
## Why Managed deployments need a mergeable way to declare which marketplace sources Codex may use. An enterprise-keyed TOML table avoids array merge ambiguity and lets every requirements layer use the existing config precedence rules without a marketplace-specific merger. ## Requirements shape ```toml [marketplaces] restrict_to_allowed_sources = true [marketplaces.allowed_sources.company_plugins] source = "git" url = "https://github.com/example/company-plugins.git" ref = "main" [marketplaces.allowed_sources.internal_git] source = "host_pattern" host_pattern = "^git\\.example\\.com$" [marketplaces.allowed_sources.local_plugins] source = "local" path = "/opt/company/codex-plugins" ``` `restrict_to_allowed_sources` follows normal scalar precedence. `allowed_sources` follows normal recursive TOML table merge behavior: distinct keys accumulate and fields under the same key use normal layer precedence. The final `source` value later selects which fields the marketplace admission policy interprets. The raw rule fields remain optional while requirements layers are composed, so a higher-priority layer can override only `ref`, `url`, or another individual field. Source-specific validation and normalization intentionally belong to the marketplace admission layer, not requirements merging. This initial shape includes `git`, `host_pattern`, and `local` sources. It does not add npm or path-pattern rules. ## What changed - Add the marketplace requirements TOML shape to `ConfigRequirementsToml`, `ConfigRequirementsWithSources`, and `ConfigRequirements`. - Carry marketplace requirements through the existing regular requirements merge path. - Keep allowed-source entries as raw partial tables for downstream policy interpretation. - Cover partial same-key overlays, source changes, unknown fields, and unmodified local paths. This PR defines and composes the requirements only. Source admission is implemented by the next PR in the stack. ## Stack This is PR 1 of 3. #29753 adds source admission on top of this PR; draft #29691 will add runtime enforcement after it is rebased later. ## Test plan - `just test -p codex-config marketplace_`
This commit is contained in:
committed by
GitHub
Unverified
parent
6f65b9a98c
commit
2696e7199b
@@ -11,6 +11,7 @@ use serde::de::value::Error as ValueDeserializerError;
|
||||
use serde::de::value::StrDeserializer;
|
||||
use std::collections::BTreeMap;
|
||||
use std::fmt;
|
||||
use std::path::PathBuf;
|
||||
use wildmatch::WildMatchPattern;
|
||||
|
||||
use super::requirements_exec_policy::RequirementsExecPolicy;
|
||||
@@ -155,6 +156,7 @@ pub struct ConfigRequirements {
|
||||
pub managed_hooks: Option<ConstrainedWithSource<ManagedHooksRequirementsToml>>,
|
||||
pub mcp_servers: Option<Sourced<BTreeMap<String, McpServerRequirement>>>,
|
||||
pub plugins: Option<Sourced<BTreeMap<String, PluginRequirementsToml>>>,
|
||||
pub marketplaces: Option<Sourced<MarketplaceRequirementsToml>>,
|
||||
pub exec_policy: Option<Sourced<RequirementsExecPolicy>>,
|
||||
pub enforce_residency: ConstrainedWithSource<Option<ResidencyRequirement>>,
|
||||
/// Managed network constraints derived from requirements.
|
||||
@@ -196,6 +198,7 @@ impl Default for ConfigRequirements {
|
||||
managed_hooks: None,
|
||||
mcp_servers: None,
|
||||
plugins: None,
|
||||
marketplaces: None,
|
||||
exec_policy: None,
|
||||
enforce_residency: ConstrainedWithSource::new(
|
||||
Constrained::allow_any(/*initial_value*/ None),
|
||||
@@ -231,6 +234,41 @@ pub struct PluginRequirementsToml {
|
||||
pub mcp_servers: Option<BTreeMap<String, McpServerRequirement>>,
|
||||
}
|
||||
|
||||
#[derive(Deserialize, Debug, Clone, Default, PartialEq, Eq)]
|
||||
#[serde(deny_unknown_fields)]
|
||||
pub struct MarketplaceRequirementsToml {
|
||||
pub restrict_to_allowed_sources: Option<bool>,
|
||||
#[serde(default)]
|
||||
pub allowed_sources: BTreeMap<String, MarketplaceAllowedSourceToml>,
|
||||
}
|
||||
|
||||
impl MarketplaceRequirementsToml {
|
||||
pub fn is_empty(&self) -> bool {
|
||||
self.restrict_to_allowed_sources.is_none() && self.allowed_sources.is_empty()
|
||||
}
|
||||
}
|
||||
|
||||
/// Raw marketplace source rule whose active fields are interpreted after
|
||||
/// requirements composition.
|
||||
#[derive(Deserialize, Debug, Clone, PartialEq, Eq)]
|
||||
#[serde(deny_unknown_fields)]
|
||||
pub struct MarketplaceAllowedSourceToml {
|
||||
pub source: Option<MarketplaceAllowedSourceKind>,
|
||||
pub url: Option<String>,
|
||||
#[serde(rename = "ref")]
|
||||
pub ref_name: Option<String>,
|
||||
pub host_pattern: Option<String>,
|
||||
pub path: Option<PathBuf>,
|
||||
}
|
||||
|
||||
#[derive(Deserialize, Debug, Clone, Copy, PartialEq, Eq)]
|
||||
#[serde(rename_all = "snake_case")]
|
||||
pub enum MarketplaceAllowedSourceKind {
|
||||
Git,
|
||||
HostPattern,
|
||||
Local,
|
||||
}
|
||||
|
||||
impl PluginRequirementsToml {
|
||||
pub fn is_empty(&self) -> bool {
|
||||
self.mcp_servers.as_ref().is_none_or(BTreeMap::is_empty)
|
||||
@@ -842,6 +880,7 @@ pub struct ConfigRequirementsToml {
|
||||
pub hooks: Option<ManagedHooksRequirementsToml>,
|
||||
pub mcp_servers: Option<BTreeMap<String, McpServerRequirement>>,
|
||||
pub plugins: Option<BTreeMap<String, PluginRequirementsToml>>,
|
||||
pub marketplaces: Option<MarketplaceRequirementsToml>,
|
||||
pub apps: Option<AppsRequirementsToml>,
|
||||
pub rules: Option<RequirementsExecPolicyToml>,
|
||||
pub enforce_residency: Option<ResidencyRequirement>,
|
||||
@@ -896,6 +935,7 @@ pub struct ConfigRequirementsWithSources {
|
||||
pub hooks: Option<Sourced<ManagedHooksRequirementsToml>>,
|
||||
pub mcp_servers: Option<Sourced<BTreeMap<String, McpServerRequirement>>>,
|
||||
pub plugins: Option<Sourced<BTreeMap<String, PluginRequirementsToml>>>,
|
||||
pub marketplaces: Option<Sourced<MarketplaceRequirementsToml>>,
|
||||
pub apps: Option<Sourced<AppsRequirementsToml>>,
|
||||
pub rules: Option<Sourced<RequirementsExecPolicyToml>>,
|
||||
pub enforce_residency: Option<Sourced<ResidencyRequirement>>,
|
||||
@@ -939,6 +979,7 @@ impl ConfigRequirementsWithSources {
|
||||
hooks: _,
|
||||
mcp_servers: _,
|
||||
plugins: _,
|
||||
marketplaces: _,
|
||||
apps: _,
|
||||
rules: _,
|
||||
enforce_residency: _,
|
||||
@@ -975,6 +1016,7 @@ impl ConfigRequirementsWithSources {
|
||||
hooks,
|
||||
mcp_servers,
|
||||
plugins,
|
||||
marketplaces,
|
||||
rules,
|
||||
enforce_residency,
|
||||
network,
|
||||
@@ -1009,6 +1051,7 @@ impl ConfigRequirementsWithSources {
|
||||
hooks,
|
||||
mcp_servers,
|
||||
plugins,
|
||||
marketplaces,
|
||||
apps,
|
||||
rules,
|
||||
enforce_residency,
|
||||
@@ -1033,6 +1076,7 @@ impl ConfigRequirementsWithSources {
|
||||
hooks: hooks.map(|sourced| sourced.value),
|
||||
mcp_servers: mcp_servers.map(|sourced| sourced.value),
|
||||
plugins: plugins.map(|sourced| sourced.value),
|
||||
marketplaces: marketplaces.map(|sourced| sourced.value),
|
||||
apps: apps.map(|sourced| sourced.value),
|
||||
rules: rules.map(|sourced| sourced.value),
|
||||
enforce_residency: enforce_residency.map(|sourced| sourced.value),
|
||||
@@ -1138,6 +1182,10 @@ impl ConfigRequirementsToml {
|
||||
.plugins
|
||||
.as_ref()
|
||||
.is_none_or(|plugins| plugins.values().all(PluginRequirementsToml::is_empty))
|
||||
&& self
|
||||
.marketplaces
|
||||
.as_ref()
|
||||
.is_none_or(MarketplaceRequirementsToml::is_empty)
|
||||
&& self
|
||||
.apps
|
||||
.as_ref()
|
||||
@@ -1176,6 +1224,7 @@ impl TryFrom<ConfigRequirementsWithSources> for ConfigRequirements {
|
||||
hooks,
|
||||
mcp_servers,
|
||||
plugins,
|
||||
marketplaces,
|
||||
apps: _apps,
|
||||
rules,
|
||||
enforce_residency,
|
||||
@@ -1456,6 +1505,7 @@ impl TryFrom<ConfigRequirementsWithSources> for ConfigRequirements {
|
||||
managed_hooks,
|
||||
mcp_servers,
|
||||
plugins,
|
||||
marketplaces,
|
||||
exec_policy,
|
||||
enforce_residency,
|
||||
network,
|
||||
@@ -1549,6 +1599,7 @@ mod tests {
|
||||
hooks,
|
||||
mcp_servers,
|
||||
plugins,
|
||||
marketplaces,
|
||||
apps,
|
||||
rules,
|
||||
enforce_residency,
|
||||
@@ -1582,6 +1633,7 @@ mod tests {
|
||||
hooks: hooks.map(|value| Sourced::new(value, RequirementSource::Unknown)),
|
||||
mcp_servers: mcp_servers.map(|value| Sourced::new(value, RequirementSource::Unknown)),
|
||||
plugins: plugins.map(|value| Sourced::new(value, RequirementSource::Unknown)),
|
||||
marketplaces: marketplaces.map(|value| Sourced::new(value, RequirementSource::Unknown)),
|
||||
apps: apps.map(|value| Sourced::new(value, RequirementSource::Unknown)),
|
||||
rules: rules.map(|value| Sourced::new(value, RequirementSource::Unknown)),
|
||||
enforce_residency: enforce_residency
|
||||
@@ -1785,6 +1837,7 @@ mod tests {
|
||||
hooks: None,
|
||||
mcp_servers: None,
|
||||
plugins: None,
|
||||
marketplaces: None,
|
||||
apps: None,
|
||||
rules: None,
|
||||
enforce_residency: Some(enforce_residency),
|
||||
@@ -1834,6 +1887,7 @@ mod tests {
|
||||
hooks: None,
|
||||
mcp_servers: None,
|
||||
plugins: None,
|
||||
marketplaces: None,
|
||||
apps: None,
|
||||
rules: None,
|
||||
enforce_residency: Some(Sourced::new(enforce_residency, enforce_source)),
|
||||
@@ -1880,6 +1934,7 @@ mod tests {
|
||||
hooks: None,
|
||||
mcp_servers: None,
|
||||
plugins: None,
|
||||
marketplaces: None,
|
||||
apps: None,
|
||||
rules: None,
|
||||
enforce_residency: None,
|
||||
@@ -1934,6 +1989,7 @@ mod tests {
|
||||
hooks: None,
|
||||
mcp_servers: None,
|
||||
plugins: None,
|
||||
marketplaces: None,
|
||||
apps: None,
|
||||
rules: None,
|
||||
enforce_residency: None,
|
||||
|
||||
@@ -61,6 +61,9 @@ pub use config_requirements::ConstrainedWithSource;
|
||||
pub use config_requirements::FeatureRequirementsToml;
|
||||
pub use config_requirements::FilesystemConstraints;
|
||||
pub use config_requirements::FilesystemDenyReadPattern;
|
||||
pub use config_requirements::MarketplaceAllowedSourceKind;
|
||||
pub use config_requirements::MarketplaceAllowedSourceToml;
|
||||
pub use config_requirements::MarketplaceRequirementsToml;
|
||||
pub use config_requirements::McpServerIdentity;
|
||||
pub use config_requirements::McpServerRequirement;
|
||||
pub use config_requirements::NetworkConstraints;
|
||||
|
||||
@@ -222,6 +222,7 @@ fn populate_merged_regular_fields_with_sources(
|
||||
hooks: _,
|
||||
mcp_servers,
|
||||
plugins,
|
||||
marketplaces,
|
||||
apps,
|
||||
rules: _,
|
||||
enforce_residency,
|
||||
@@ -250,6 +251,7 @@ fn populate_merged_regular_fields_with_sources(
|
||||
set_sourced!(feature_requirements, &["features", "feature_requirements"]);
|
||||
set_sourced!(mcp_servers, &["mcp_servers"]);
|
||||
set_sourced!(plugins, &["plugins"]);
|
||||
set_sourced!(marketplaces, &["marketplaces"]);
|
||||
set_sourced!(apps, &["apps"]);
|
||||
set_sourced!(enforce_residency, &["enforce_residency"]);
|
||||
set_sourced!(network, &["experimental_network"]);
|
||||
|
||||
@@ -13,6 +13,7 @@ use codex_utils_absolute_path::AbsolutePathBuf;
|
||||
use pretty_assertions::assert_eq;
|
||||
use std::cell::Cell;
|
||||
use std::collections::BTreeMap;
|
||||
use tempfile::TempDir;
|
||||
|
||||
fn layer(id: &str, name: &str, contents: &str) -> RequirementsLayerEntry {
|
||||
RequirementsLayerEntry::from_toml(
|
||||
@@ -1026,3 +1027,154 @@ fn parse_error_names_layer() {
|
||||
assert!(err.to_string().contains("Bad layer (req_bad)"));
|
||||
assert!(err.to_string().contains("allowed_approval_policies"));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn marketplace_allowed_sources_use_default_toml_merge() {
|
||||
let composed = compose(vec![
|
||||
layer(
|
||||
"req_low",
|
||||
"Low",
|
||||
r#"
|
||||
[marketplaces]
|
||||
restrict_to_allowed_sources = true
|
||||
|
||||
[marketplaces.allowed_sources.shared]
|
||||
source = "git"
|
||||
url = "https://github.com/example/old.git"
|
||||
ref = "main"
|
||||
|
||||
[marketplaces.allowed_sources.other]
|
||||
source = "git"
|
||||
url = "https://github.com/example/other.git"
|
||||
"#,
|
||||
),
|
||||
layer(
|
||||
"req_high",
|
||||
"High",
|
||||
r#"
|
||||
[marketplaces.allowed_sources.shared]
|
||||
ref = "release"
|
||||
"#,
|
||||
),
|
||||
])
|
||||
.expect("compose requirements")
|
||||
.expect("requirements present");
|
||||
|
||||
assert_eq!(
|
||||
composed,
|
||||
expected_requirements(
|
||||
r#"
|
||||
[marketplaces]
|
||||
restrict_to_allowed_sources = true
|
||||
|
||||
[marketplaces.allowed_sources.shared]
|
||||
source = "git"
|
||||
url = "https://github.com/example/old.git"
|
||||
ref = "release"
|
||||
|
||||
[marketplaces.allowed_sources.other]
|
||||
source = "git"
|
||||
url = "https://github.com/example/other.git"
|
||||
"#,
|
||||
)
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn marketplace_source_switch_uses_default_toml_merge() {
|
||||
let composed = compose(vec![
|
||||
layer(
|
||||
"req_low",
|
||||
"Low",
|
||||
r#"
|
||||
[marketplaces.allowed_sources.company]
|
||||
source = "git"
|
||||
url = "https://github.com/example/plugins.git"
|
||||
ref = "main"
|
||||
"#,
|
||||
),
|
||||
layer(
|
||||
"req_high",
|
||||
"High",
|
||||
r#"
|
||||
[marketplaces.allowed_sources.company]
|
||||
source = "host_pattern"
|
||||
host_pattern = '^github\.example\.com$'
|
||||
"#,
|
||||
),
|
||||
])
|
||||
.expect("compose requirements")
|
||||
.expect("requirements present");
|
||||
|
||||
assert_eq!(
|
||||
composed,
|
||||
expected_requirements(
|
||||
r#"
|
||||
[marketplaces.allowed_sources.company]
|
||||
source = "host_pattern"
|
||||
url = "https://github.com/example/plugins.git"
|
||||
ref = "main"
|
||||
host_pattern = '^github\.example\.com$'
|
||||
"#,
|
||||
)
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn marketplace_allowed_source_rejects_unknown_fields() {
|
||||
let err = compose(vec![layer(
|
||||
"req_bad",
|
||||
"Bad marketplace layer",
|
||||
r#"
|
||||
[marketplaces]
|
||||
restrict_to_allowed_sources = true
|
||||
|
||||
[marketplaces.allowed_sources.invalid]
|
||||
source = "git"
|
||||
url = "https://github.com/example/plugins.git"
|
||||
reff = "main"
|
||||
"#,
|
||||
)])
|
||||
.expect_err("invalid marketplace rule should fail");
|
||||
|
||||
assert!(err.to_string().contains("Bad marketplace layer (req_bad)"));
|
||||
assert!(err.to_string().contains("unknown field `reff`"));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn local_marketplace_path_is_not_resolved_during_requirements_merge() {
|
||||
let base_dir = TempDir::new().expect("create requirements base directory");
|
||||
let base_dir = AbsolutePathBuf::try_from(base_dir.path().to_path_buf())
|
||||
.expect("absolute requirements base directory");
|
||||
let composed = compose(vec![
|
||||
layer(
|
||||
"req_local",
|
||||
"Local marketplace path",
|
||||
r#"
|
||||
[marketplaces]
|
||||
restrict_to_allowed_sources = true
|
||||
|
||||
[marketplaces.allowed_sources.local]
|
||||
source = "local"
|
||||
path = "../plugins"
|
||||
"#,
|
||||
)
|
||||
.with_base_dir(base_dir),
|
||||
])
|
||||
.expect("compose requirements")
|
||||
.expect("requirements present");
|
||||
|
||||
assert_eq!(
|
||||
composed,
|
||||
expected_requirements(
|
||||
r#"
|
||||
[marketplaces]
|
||||
restrict_to_allowed_sources = true
|
||||
|
||||
[marketplaces.allowed_sources.local]
|
||||
source = "local"
|
||||
path = "../plugins"
|
||||
"#,
|
||||
)
|
||||
);
|
||||
}
|
||||
|
||||
@@ -8679,6 +8679,7 @@ async fn test_requirements_web_search_mode_allowlist_does_not_warn_when_unset()
|
||||
hooks: None,
|
||||
mcp_servers: None,
|
||||
plugins: None,
|
||||
marketplaces: None,
|
||||
apps: None,
|
||||
rules: None,
|
||||
enforce_residency: None,
|
||||
|
||||
@@ -2940,6 +2940,7 @@ impl Config {
|
||||
managed_hooks: _,
|
||||
mcp_servers,
|
||||
plugins: _,
|
||||
marketplaces: _,
|
||||
exec_policy: _,
|
||||
enforce_residency,
|
||||
network: network_requirements,
|
||||
|
||||
@@ -785,6 +785,7 @@ mod tests {
|
||||
},
|
||||
)])),
|
||||
plugins: None,
|
||||
marketplaces: None,
|
||||
apps: None,
|
||||
rules: None,
|
||||
enforce_residency: Some(ResidencyRequirement::Us),
|
||||
@@ -1142,6 +1143,7 @@ approval_policy = "never"
|
||||
hooks: None,
|
||||
mcp_servers: None,
|
||||
plugins: None,
|
||||
marketplaces: None,
|
||||
apps: None,
|
||||
rules: None,
|
||||
enforce_residency: None,
|
||||
|
||||
Reference in New Issue
Block a user