diff --git a/crates/cli/tests/coverage/shared/config_tests.rs b/crates/cli/tests/coverage/shared/config_tests.rs index 2f4dc5114..cf74749b9 100644 --- a/crates/cli/tests/coverage/shared/config_tests.rs +++ b/crates/cli/tests/coverage/shared/config_tests.rs @@ -2048,8 +2048,16 @@ fn plugin_config_path_overrides_sibling_plugin_file() { let sibling_path = temp.path().join("plugins.toml"); let override_path = temp.path().join("override.toml"); std::fs::write(&config_path, "").unwrap(); - std::fs::write(&sibling_path, "version = 1\n").unwrap(); - std::fs::write(&override_path, "version = 2\n").unwrap(); + std::fs::write( + &sibling_path, + "version = 1\n[policy]\nunknown_field = \"warn\"\n", + ) + .unwrap(); + std::fs::write( + &override_path, + "version = 1\n[policy]\nunknown_field = \"error\"\n", + ) + .unwrap(); let command = RunOverrides { agent: Some(CodingAgent::Codex), config: Some(config_path), @@ -2066,7 +2074,10 @@ fn plugin_config_path_overrides_sibling_plugin_file() { assert_eq!( resolved.gateway.plugin_config, - Some(json!({ "version": 2 })) + Some(json!({ + "version": 1, + "policy": { "unknown_field": "error" } + })) ); } diff --git a/crates/core/src/plugin.rs b/crates/core/src/plugin.rs index b9c446764..97bab10bb 100644 --- a/crates/core/src/plugin.rs +++ b/crates/core/src/plugin.rs @@ -1597,9 +1597,12 @@ async fn initialize_plugin_components_catching_panics( /// Validates and activates `config` layered on top of the discovered /// `plugins.toml` configuration, so a direct integration sees the same file -/// layering as the gateway. `config` wins on conflicts; as a typed document its -/// default `version`/`policy`/`enabled` override the file, while `config` bodies -/// merge field-by-field. Delegates to [`initialize_plugins_exact`]. +/// layering as the gateway. Each file's schema version is validated before +/// layering. Non-default values in `config` win on conflicts, while default +/// `policy` and `enabled` values inherit from a matching file entry and +/// component `config` bodies merge field-by-field. Delegates to +/// [`initialize_plugins_exact`]. Call that function directly when `config` +/// is already fully resolved and every value must be applied exactly. pub async fn initialize_plugins(config: PluginConfig) -> Result { let config = resolve_plugin_config(config)?; initialize_plugins_exact(config).await @@ -1611,10 +1614,55 @@ pub async fn initialize_plugins(config: PluginConfig) -> Result { /// one-time configuration resolution as regular harness-native initialization. pub(crate) fn resolve_plugin_config(config: PluginConfig) -> Result { let mut base = resolve_default_file_plugin_config()?; - layer_config(&mut base, serde_json::to_value(config)?); + layer_config(&mut base, plugin_config_overlay_value(&config)?); Ok(serde_json::from_value(base)?) } +/// Serializes a typed configuration as a discovery overlay. +/// +/// A [`PluginConfig`] cannot record whether a default-valued field was supplied +/// explicitly or filled by serde. Treating those defaults as overlay values +/// would mask discovered file settings on every library initialization. Exact +/// callers bypass discovery through [`initialize_plugins_exact`]. +fn plugin_config_overlay_value(config: &PluginConfig) -> Result { + let mut overlay = serde_json::to_value(config)?; + let Json::Object(root) = &mut overlay else { + return Ok(overlay); + }; + + if config.version == default_plugin_config_version() { + root.remove("version"); + } + + if let Some(Json::Object(policy)) = root.get_mut("policy") { + let defaults = ConfigPolicy::default(); + if config.policy.unknown_component == defaults.unknown_component { + policy.remove("unknown_component"); + } + if config.policy.unknown_field == defaults.unknown_field { + policy.remove("unknown_field"); + } + if config.policy.unsupported_value == defaults.unsupported_value { + policy.remove("unsupported_value"); + } + if policy.is_empty() { + root.remove("policy"); + } + } + + if let Some(Json::Array(components)) = root.get_mut("components") { + for (component, typed) in components.iter_mut().zip(&config.components) { + if typed.enabled == default_enabled() + && let Json::Object(component) = component + { + component.remove("enabled"); + } + } + } + + Ok(overlay) +} + /// Resolves the default `plugins.toml` layering into one JSON document, or an /// empty object when no plugin file exists. fn resolve_default_file_plugin_config() -> Result { @@ -1682,6 +1730,7 @@ where let mut merged = Json::Object(Map::new()); let mut sources = Vec::new(); for (path, document) in documents { + validate_plugin_config_version(&path, &document)?; validate_unique_component_kinds(&path, &document)?; layer_config(&mut merged, document); sources.push(path); @@ -1689,6 +1738,28 @@ where Ok((!sources.is_empty()).then_some((merged, sources))) } +/// Rejects a file with an unsupported top-level plugin config version before layering can +/// overwrite it with a higher-precedence source or typed default. +fn validate_plugin_config_version(path: &Path, document: &Json) -> Result<()> { + let Some(raw_version) = document.get("version") else { + return Ok(()); + }; + let version = serde_json::from_value::(raw_version.clone()).map_err(|error| { + PluginError::InvalidConfig(format!( + "invalid plugin config version in {}: {error}", + path.display() + )) + })?; + if version == default_plugin_config_version() { + return Ok(()); + } + Err(PluginError::InvalidConfig(format!( + "plugin config version {version} in {} is unsupported; expected {}", + path.display(), + default_plugin_config_version() + ))) +} + /// Rejects a single file that declares the same component `kind` more than once. fn validate_unique_component_kinds(path: &Path, document: &Json) -> Result<()> { let Some(components) = document.get("components").and_then(Json::as_array) else { diff --git a/crates/core/tests/unit/plugin_tests.rs b/crates/core/tests/unit/plugin_tests.rs index 832c5b30d..8601e18da 100644 --- a/crates/core/tests/unit/plugin_tests.rs +++ b/crates/core/tests/unit/plugin_tests.rs @@ -2227,6 +2227,36 @@ fn test_load_plugin_config_files_merges_files_by_precedence() { ); } +#[test] +fn test_load_plugin_config_files_rejects_version_before_layering() { + let dir = tempfile::tempdir().unwrap(); + let invalid = dir.path().join("invalid.toml"); + let higher = dir.path().join("higher.toml"); + std::fs::write( + &invalid, + "version = 2\n\ + [[components]]\n\ + kind = \"observability\"\n", + ) + .unwrap(); + std::fs::write(&higher, "version = 1\n").unwrap(); + + let error = load_plugin_config_files([invalid.clone(), higher]) + .expect_err("a higher-precedence version must not mask an invalid source version"); + + match error { + PluginError::InvalidConfig(message) => { + assert!(message.contains("plugin config version 2"), "{message}"); + assert!( + message.contains(&invalid.display().to_string()), + "{message}" + ); + assert!(message.contains("expected 1"), "{message}"); + } + other => panic!("unexpected plugin config version error: {other}"), + } +} + #[test] fn test_default_plugin_config_paths_order_user_project_system() { let dir = tempfile::tempdir().unwrap(); @@ -2284,13 +2314,12 @@ fn test_load_plugin_config_files_deduplicates_aliases_at_highest_precedence() { } #[test] -fn test_layer_config_applies_typed_overlay_defaults_over_file_base() { - // The code-vs-file path `initialize_plugins` takes: a typed `PluginConfig` is layered - // over the discovered file base. Its serde defaults (`version`/`policy`/`enabled`) - // override the file, the free-form `config` body merges, and an undeclared component - // kind is inherited from the file. +fn test_plugin_config_overlay_inherits_file_values_for_typed_defaults() { + // After each file's schema version is validated, a typed `PluginConfig` is layered over + // the discovered file base. Default-valued `policy`/`enabled` fields inherit the file, + // the free-form `config` body merges, and an undeclared component kind is inherited. let file_base = json!({ - "version": 2, + "version": 1, "components": [ { "kind": "observability", @@ -2314,21 +2343,21 @@ fn test_layer_config_applies_typed_overlay_defaults_over_file_base() { }; let mut merged = file_base; - layer_config(&mut merged, serde_json::to_value(code).unwrap()); + layer_config(&mut merged, plugin_config_overlay_value(&code).unwrap()); let typed: PluginConfig = serde_json::from_value(merged).unwrap(); - // Typed defaults override the file base. - assert_eq!(typed.version, 1, "typed default version overrides the file"); + // Typed defaults do not mask the file base. + assert_eq!(typed.version, 1); assert_eq!( typed.policy.unknown_component, - UnsupportedBehavior::Warn, - "typed default policy overrides the file" + UnsupportedBehavior::Error, + "typed default policy inherits the file value" ); let observability = &typed.components[0]; assert_eq!(observability.kind, "observability"); assert!( - observability.enabled, - "typed default enabled=true overrides the file's false" + !observability.enabled, + "typed default enabled=true inherits the file's false" ); // The component config body merges: code's `mode` wins, the file's `output_directory` // is inherited. @@ -2337,3 +2366,42 @@ fn test_layer_config_applies_typed_overlay_defaults_over_file_base() { // A kind the code config does not declare is inherited from the file. assert_eq!(typed.components[1].kind, "adaptive"); } + +#[test] +fn test_plugin_config_overlay_applies_non_default_values() { + let mut file_base = json!({ + "version": 1, + "components": [{ "kind": "observability", "enabled": true }], + "policy": { + "unknown_component": "error", + "unknown_field": "warn", + "unsupported_value": "error" + } + }); + let code = PluginConfig { + components: vec![PluginComponentSpec { + enabled: false, + ..PluginComponentSpec::new("observability") + }], + policy: ConfigPolicy { + unknown_field: UnsupportedBehavior::Ignore, + ..ConfigPolicy::default() + }, + ..PluginConfig::default() + }; + + layer_config(&mut file_base, plugin_config_overlay_value(&code).unwrap()); + let typed: PluginConfig = serde_json::from_value(file_base).unwrap(); + + assert!(!typed.components[0].enabled); + assert_eq!( + typed.policy.unknown_component, + UnsupportedBehavior::Error, + "a default-valued field inherits the file" + ); + assert_eq!( + typed.policy.unknown_field, + UnsupportedBehavior::Ignore, + "a non-default field overrides the file" + ); +} diff --git a/crates/ffi/tests/integration/plugin_activation_tests.rs b/crates/ffi/tests/integration/plugin_activation_tests.rs index ab2be8e9f..bab91f2d9 100644 --- a/crates/ffi/tests/integration/plugin_activation_tests.rs +++ b/crates/ffi/tests/integration/plugin_activation_tests.rs @@ -122,7 +122,7 @@ fn run_discovered_config_activation_test() { std::fs::write( &plugins_toml, format!( - r#"version = 999 + r#"version = 1 [[components]] kind = {DISCOVERED_STATIC_PLUGIN_KIND:?} @@ -158,8 +158,7 @@ source = "project-file" "config": {} }])); - // The explicit version 1 must override the discovered version 999. The - // file-only component and its config must still survive the merge. + // The file-only component and its config must survive the merge. assert_eq!(report["diagnostics"], json!([])); assert_eq!(DISCOVERED_STATIC_REGISTRATIONS.load(Ordering::SeqCst), 1); assert_eq!( diff --git a/go/nemo_relay/plugin_activation_test.go b/go/nemo_relay/plugin_activation_test.go index 84b5ae77c..ba1cfd25b 100644 --- a/go/nemo_relay/plugin_activation_test.go +++ b/go/nemo_relay/plugin_activation_test.go @@ -623,7 +623,7 @@ func configureNativePluginProject(t *testing.T) string { } pluginsTOML := filepath.Join(projectConfigDir, "plugins.toml") const staticKind = "go.fixture.static_base" - fileConfig := fmt.Sprintf(`version = 999 + fileConfig := fmt.Sprintf(`version = 1 [[components]] kind = %q