From 2683dd4515611b2edf464487dc83d982e646dccc Mon Sep 17 00:00:00 2001 From: Jean Mertz Date: Mon, 7 Sep 2026 12:58:01 +0200 Subject: [PATCH 1/7] feat(schematic): Honor `partial_via` on a plain field `partial_via` substituted the partial's field type only when the field was also `nested`, and `nested` requires the element type to be a `Config`. A `Vec` could therefore not carry a wrapper in its partial, which is what a list needs in order to declare its own merge strategy: five of JP's six plain-list config fields hold scalars. The substitution now applies to a plain field too, and `generate_from_partial_value` converts back to the field's own type on the way out. The conversion wraps the inner access rather than the container, since it is the value whose type differs and not its `Option` or its `Box`, and the nullable case maps over the option instead of converting it. A field opting in gains a partial of `Option>` while the resolved config keeps `Vec`, so callers see no change and the wrapper is confined to the merge path. Signed-off-by: Jean Mertz --- .../schematic_macros/src/common/field.rs | 6 +++- .../schematic_macros/src/config/field.rs | 31 ++++++++++++++++--- 2 files changed, 31 insertions(+), 6 deletions(-) diff --git a/crates/contrib/schematic_macros/src/common/field.rs b/crates/contrib/schematic_macros/src/common/field.rs index e9ac6f60f..ab4fc8d40 100644 --- a/crates/contrib/schematic_macros/src/common/field.rs +++ b/crates/contrib/schematic_macros/src/common/field.rs @@ -159,7 +159,11 @@ impl Field<'_> { } value_type } else { - FieldValue::value(result.value) + // `partial_via` applies to a plain field too, so a list of scalars + // can carry a wrapper that knows its own merge strategy. The + // partial holds the via type and `generate_from_partial_value` + // converts back to the field's own type. + FieldValue::value(result.partial_via_ty.as_ref().unwrap_or(result.value)) }; result diff --git a/crates/contrib/schematic_macros/src/config/field.rs b/crates/contrib/schematic_macros/src/config/field.rs index 53db0e33f..f0b270cdb 100644 --- a/crates/contrib/schematic_macros/src/config/field.rs +++ b/crates/contrib/schematic_macros/src/config/field.rs @@ -152,22 +152,43 @@ impl Field<'_> { #[allow(clippy::collapsible_else_if)] if matches!(self.value_type, FieldValue::Value { .. }) { + // A `partial_via` field stores the via type in the partial and its + // own type in the resolved config, so the value converts on the way + // out. The conversion wraps the *inner* access, before any boxing, + // since it is the value that changes type and not its container. + let via = self.args.partial_via.is_some(); + let convert = |value: TokenStream| { + if via { + quote! { Into::into(#value) } + } else { + value + } + }; + if self.value_type.is_outer_boxed() { if self.is_nullable() { - quote! { partial.#key.map(Box::new) } + let inner = convert(quote! { value }); + quote! { partial.#key.map(|value| Box::new(#inner)) } } else { - quote! { Box::new(partial.#key) } + let inner = convert(quote! { partial.#key }); + quote! { Box::new(#inner) } } } else { if self.is_nullable() { // Use optional values as-is as they're already wrapped in `Option` - quote! { partial.#key } + if via { + quote! { partial.#key.map(Into::into) } + } else { + quote! { partial.#key } + } } else if self.is_required() { // Trigger a validation error if the value is missing - quote! { partial.#key.ok_or(schematic::ConfigError::MissingRequired{ fields: { let mut fields = fields.clone(); fields.push(#key_quoted.to_owned()); fields } })? } + convert( + quote! { partial.#key.ok_or(schematic::ConfigError::MissingRequired{ fields: { let mut fields = fields.clone(); fields.push(#key_quoted.to_owned()); fields } })? }, + ) } else { // Otherwise unwrap the resolved value or use the type default - quote! { partial.#key.unwrap_or_default() } + convert(quote! { partial.#key.unwrap_or_default() }) } } } else { From 36f7d09077976bcdee234a1090e1f70948505217 Mon Sep 17 00:00:00 2001 From: Jean Mertz Date: Mon, 7 Sep 2026 12:58:26 +0200 Subject: [PATCH 2/7] feat(config): Let `editor.envs` declare its merge strategy `editor.envs` appended across layers with no way to say otherwise. It now carries a strategy of its own, so a user can replace the inherited list rather than extend it: ```toml [editor] envs = { value = ["MY_EDITOR"], strategy = "replace" } ``` The plain list still works and still appends, since that is the declared default. This is the first plain-list field to carry a wrapper in its partial, which the `partial_via` change makes possible. Two pieces make it work for a list of scalars rather than of configs: `try_some_mergeable_ strings` accepts either shape from `--cfg`, telling them apart the way the type's own deserializer does, and `delta_opt_mergeable_vec` diffs the field through the wrapper so a removal or a reorder is recorded as a replacement instead of being dropped. `editor.envs` is also the last list in the config whose removals were recorded through `unsets`; that mechanism is now only needed for scalars, and for the five remaining plain lists until they follow. Signed-off-by: Jean Mertz --- crates/jp_config/src/assignment.rs | 40 +++++++++++++++++- crates/jp_config/src/delta.rs | 18 ++++++++ crates/jp_config/src/editor.rs | 27 ++++++++---- crates/jp_config/src/editor_tests.rs | 42 +++++++++++++------ ...ts__partial_app_config_default_values.snap | 12 +++--- 5 files changed, 111 insertions(+), 28 deletions(-) diff --git a/crates/jp_config/src/assignment.rs b/crates/jp_config/src/assignment.rs index 973609198..851e7d1ef 100644 --- a/crates/jp_config/src/assignment.rs +++ b/crates/jp_config/src/assignment.rs @@ -12,7 +12,7 @@ use schematic::PartialConfig; use serde::{Serialize, de::DeserializeOwned}; use serde_json::{Value, from_str}; -use crate::{AppConfig, BoxedError}; +use crate::{AppConfig, BoxedError, types::vec::MergeableVec}; /// The result of assigning a key-value pair to a configuration. pub type AssignResult = Result<(), BoxedError>; @@ -1051,6 +1051,44 @@ impl KvAssignment { self.try_vec_of_strings(vec.get_or_insert_default()) } + /// Assign to a list that carries its own merge strategy. + /// + /// Accepts either the list itself or a `{ value, strategy }` object, which + /// is how the user declares a strategy for the field. + /// The two are told apart by shape, the same way [`MergeableVec`]'s own + /// deserializer does it: a sequence cannot be a table. + pub(crate) fn try_some_mergeable_strings( + self, + vec: &mut Option>, + ) -> Result<(), KvAssignmentError> + where + T: Clone + From + DeserializeOwned, + { + // An absent list and an empty one merge differently: `None` lets a + // later layer's value land verbatim, `Some([])` still runs the field's + // merge strategy against it. + if self.clears_collection() { + *vec = None; + return Ok(()); + } + + // An object declares a strategy alongside the value, so it is parsed as + // the wrapper rather than element by element. + if let KvValue::Json(value @ Value::Object(_)) = self.value.clone() { + let merged = + serde_json::from_value(value).map_err(|error| kv_error(&self.key, error))?; + + *vec = Some(merged); + return Ok(()); + } + + let mut elements = vec.take().map(MergeableVec::into_vec).unwrap_or_default(); + self.try_vec_of_strings(&mut elements)?; + *vec = Some(elements.into()); + + Ok(()) + } + /// Try to parse the value as a JSON array of partial configs, and set or /// merge the elements. pub(crate) fn try_vec_of_nested(mut self, vec: &mut Vec) -> Result<(), KvAssignmentError> diff --git a/crates/jp_config/src/delta.rs b/crates/jp_config/src/delta.rs index 1c46e8c52..4e3042e86 100644 --- a/crates/jp_config/src/delta.rs +++ b/crates/jp_config/src/delta.rs @@ -138,6 +138,24 @@ fn repeats_an_element(items: &[T]) -> bool { .any(|(index, item)| items[..index].contains(item)) } +/// Calculate the delta between two optional strategy-carrying lists. +/// +/// Wraps [`delta_mergeable_vec`] for a field whose partial is +/// `Option>`: an absent list on either side is no change, and +/// an empty delta is reported as absent so it does not read as one. +pub fn delta_opt_mergeable_vec( + prev: Option<&MergeableVec>, + next: Option>, +) -> Option> { + let next = next?; + let Some(prev) = prev else { + return Some(next); + }; + + let delta = delta_mergeable_vec(prev, next); + (!delta.is_empty()).then_some(delta) +} + /// Delta for an optional nested partial, reporting the fields it cannot reach. /// /// Mirrors [`delta_opt_partial`], descending with `path` as the nested value's diff --git a/crates/jp_config/src/editor.rs b/crates/jp_config/src/editor.rs index 5dffbdb05..e6c790450 100644 --- a/crates/jp_config/src/editor.rs +++ b/crates/jp_config/src/editor.rs @@ -11,12 +11,16 @@ use crate::types::command::shell_command_line; use crate::{ assignment::{AssignKeyValue, AssignResult, KvAssignment, missing_key}, delta::{ - PartialConfigDelta, delta_opt, delta_opt_partial, delta_opt_partial_at, delta_opt_vec, - delta_opt_vec_at, path, + PartialConfigDelta, delta_opt, delta_opt_mergeable_vec, delta_opt_partial, + delta_opt_partial_at, path, }, fill::FillDefaults, + internal::merge::vec_with_strategy, partial::{ToPartial, partial_opt, partial_opt_config}, - types::command::{CommandConfigOrString, PartialCommandConfigOrString}, + types::{ + command::{CommandConfigOrString, PartialCommandConfigOrString}, + vec::MergeableVec, + }, }; /// Editor configuration. @@ -62,8 +66,13 @@ pub struct EditorConfig { /// Values with unbalanced quoting are skipped (the next env var in the list /// is tried). #[setting( - default = vec!["JP_EDITOR".into(), "VISUAL".into(), "EDITOR".into()], - merge = schematic::merge::append_vec, + default = MergeableVec::from(vec![ + "JP_EDITOR".to_owned(), + "VISUAL".to_owned(), + "EDITOR".to_owned(), + ]), + partial_via = MergeableVec::, + merge = vec_with_strategy, )] pub envs: Vec, @@ -110,7 +119,7 @@ impl AssignKeyValue for PartialEditorConfig { match kv.key_string().as_str() { "" => kv.try_merge_object(self)?, _ if kv.p("cmd") => self.cmd.assign(kv)?, - _ if kv.p("envs") => kv.try_some_vec_of_strings(&mut self.envs)?, + _ if kv.p("envs") => kv.try_some_mergeable_strings(&mut self.envs)?, _ if kv.p("inline") => self.inline.assign(kv)?, _ => return missing_key(&kv), } @@ -123,7 +132,7 @@ impl PartialConfigDelta for PartialEditorConfig { fn delta(&self, next: Self) -> Self { Self { cmd: delta_opt_partial(self.cmd.as_ref(), next.cmd), - envs: delta_opt_vec(self.envs.as_ref(), next.envs), + envs: delta_opt_mergeable_vec(self.envs.as_ref(), next.envs), inline: self.inline.delta(next.inline), } } @@ -131,7 +140,7 @@ impl PartialConfigDelta for PartialEditorConfig { fn delta_with_unsets(&self, next: Self, prefix: &str, unsets: &mut Vec) -> Self { Self { cmd: delta_opt_partial_at(&path(prefix, "cmd"), self.cmd.as_ref(), next.cmd, unsets), - envs: delta_opt_vec_at(&path(prefix, "envs"), self.envs.as_ref(), next.envs, unsets), + envs: delta_opt_mergeable_vec(self.envs.as_ref(), next.envs), inline: self.inline.delta(next.inline), } } @@ -153,7 +162,7 @@ impl ToPartial for EditorConfig { Self::Partial { cmd: partial_opt_config(self.cmd.as_ref(), defaults.cmd), - envs: partial_opt(&self.envs, defaults.envs), + envs: partial_opt(&MergeableVec::from(self.envs.clone()), defaults.envs), inline: self.inline.to_partial(), } } diff --git a/crates/jp_config/src/editor_tests.rs b/crates/jp_config/src/editor_tests.rs index a5f397f4a..25f112f6f 100644 --- a/crates/jp_config/src/editor_tests.rs +++ b/crates/jp_config/src/editor_tests.rs @@ -45,37 +45,53 @@ fn test_editor_config_cmd() { #[test] fn test_editor_config_envs() { + let envs = |names: &[&str]| -> Option> { + Some(names.iter().map(|n| (*n).to_owned()).collect()) + }; + let mut p = PartialEditorConfig::default(); let kv = KvAssignment::try_from_cli("envs", "EDITOR,VISUAL").unwrap(); p.assign(kv).unwrap(); - assert_eq!(p.envs, Some(vec!["EDITOR".into(), "VISUAL".into()])); + assert_eq!(p.envs, envs(&["EDITOR", "VISUAL"])); let kv = KvAssignment::try_from_cli("envs:", r#"["EDITOR","VISUAL"]"#).unwrap(); p.assign(kv).unwrap(); - assert_eq!(p.envs, Some(vec!["EDITOR".into(), "VISUAL".into()])); + assert_eq!(p.envs, envs(&["EDITOR", "VISUAL"])); let kv = KvAssignment::try_from_cli("envs.0", "EDIT").unwrap(); p.assign(kv).unwrap(); - assert_eq!(p.envs, Some(vec!["EDIT".into(), "VISUAL".into()])); + assert_eq!(p.envs, envs(&["EDIT", "VISUAL"])); let kv = KvAssignment::try_from_cli("envs+:", r#"["OTHER"]"#).unwrap(); p.assign(kv).unwrap(); - assert_eq!( - p.envs, - Some(vec!["EDIT".into(), "VISUAL".into(), "OTHER".into()]) - ); + assert_eq!(p.envs, envs(&["EDIT", "VISUAL", "OTHER"])); let kv = KvAssignment::try_from_cli("envs+", "LAST").unwrap(); p.assign(kv).unwrap(); + assert_eq!(p.envs, envs(&["EDIT", "VISUAL", "OTHER", "LAST"])); +} + +/// The field accepts a strategy alongside its value, which is what carrying a +/// wrapper in the partial buys. +#[test] +fn envs_accepts_a_declared_strategy() { + use crate::types::vec::{MergedVec, MergedVecStrategy}; + + let mut p = PartialEditorConfig::default(); + + let kv = + KvAssignment::try_from_cli("envs:", r#"{"value":["ONLY"],"strategy":"replace"}"#).unwrap(); + p.assign(kv).unwrap(); + assert_eq!( p.envs, - Some(vec![ - "EDIT".into(), - "VISUAL".into(), - "OTHER".into(), - "LAST".into() - ]) + Some(MergeableVec::Merged(MergedVec { + value: vec!["ONLY".to_owned()], + strategy: Some(MergedVecStrategy::Replace), + dedup: None, + discard_when_merged: false, + })) ); } diff --git a/crates/jp_config/src/snapshots/jp_config__tests__partial_app_config_default_values.snap b/crates/jp_config/src/snapshots/jp_config__tests__partial_app_config_default_values.snap index b94f00f54..b31a760db 100644 --- a/crates/jp_config/src/snapshots/jp_config__tests__partial_app_config_default_values.snap +++ b/crates/jp_config/src/snapshots/jp_config__tests__partial_app_config_default_values.snap @@ -425,11 +425,13 @@ Ok( editor: PartialEditorConfig { cmd: None, envs: Some( - [ - "JP_EDITOR", - "VISUAL", - "EDITOR", - ], + Vec( + [ + "JP_EDITOR", + "VISUAL", + "EDITOR", + ], + ), ), inline: PartialInlineEditorConfig { edit_mode: None, From 481f14ecce3863e3e3989bc3764539050c74e787 Mon Sep 17 00:00:00 2001 From: Jean Mertz Date: Mon, 7 Sep 2026 13:04:31 +0200 Subject: [PATCH 3/7] feat(config, anthropic): Let `beta_headers` declare its merge strategy `providers.llm.anthropic.beta_headers` appended across layers with no way to say otherwise. It now carries a strategy of its own: ```toml [providers.llm.anthropic] beta_headers = { value = ["interleaved-thinking-2025-05-14"], strategy = "replace" } ``` Deduplication is unchanged. `vec_with_strategy` deduplicates any combining merge unless a config opts out, which is what `append_vec_dedup` did for this field, so a plain list behaves exactly as before. `PartialAnthropicConfig` loses its `delta_with_unsets` override entirely: with the last of its fields able to state `replace` on the wire, nothing here needs a path reported, and the trait's default diff is correct. That is the shape the rest of the conversion takes as it lands, one field at a time. Signed-off-by: Jean Mertz --- crates/jp_cli/src/config_pipeline_tests.rs | 6 ++- crates/jp_config/src/delta_tests.rs | 16 +++++-- .../jp_config/src/providers/llm/anthropic.rs | 43 ++++++++----------- .../src/providers/llm/anthropic_tests.rs | 2 +- ...onfig__tests__app_config_schema_shape.snap | 16 ++++++- ...ts__partial_app_config_default_values.snap | 4 +- 6 files changed, 54 insertions(+), 33 deletions(-) diff --git a/crates/jp_cli/src/config_pipeline_tests.rs b/crates/jp_cli/src/config_pipeline_tests.rs index ed0bdbc58..a87ea2959 100644 --- a/crates/jp_cli/src/config_pipeline_tests.rs +++ b/crates/jp_cli/src/config_pipeline_tests.rs @@ -114,6 +114,7 @@ fn an_override_restating_an_existing_rule_records_nothing() { /// A list that merges by appending, restated at a value it already holds. /// +/// MCP arguments preserve repetition. /// Appending without deduplicating is not idempotent: asking for `FOO` twice /// leaves the list holding it twice, which is a different config and so a real /// change to record. @@ -123,7 +124,10 @@ fn an_override_repeating_an_appending_list_is_recorded() { let current = base_partial(); let mut overrides = PartialAppConfig::empty(); - overrides.editor.envs = Some(vec!["FOO".to_owned()]); + overrides + .providers + .mcp + .insert("bookworm".to_owned(), mcp_server("FOO")); assert!( override_to_record(¤t, overrides.clone()) diff --git a/crates/jp_config/src/delta_tests.rs b/crates/jp_config/src/delta_tests.rs index 3e916eac8..0ad8c586e 100644 --- a/crates/jp_config/src/delta_tests.rs +++ b/crates/jp_config/src/delta_tests.rs @@ -129,7 +129,9 @@ fn a_reordered_argument_list_reports_its_path() { /// The report reaches a field nested several levels below the root. #[test] -fn a_dropped_beta_header_reports_its_full_path() { +fn a_dropped_beta_header_is_recorded_as_a_replacement() { + use crate::types::vec::{MergedVec, MergedVecStrategy}; + let headers = |values: &[&str]| { let mut partial = crate::PartialAppConfig::empty(); partial.providers.llm.anthropic.beta_headers = @@ -143,10 +145,18 @@ fn a_dropped_beta_header_reports_its_full_path() { let mut unsets = Vec::new(); let delta = prev.delta_with_unsets(next, "", &mut unsets); - assert_eq!(unsets, ["providers.llm.anthropic.beta_headers"]); + assert!( + unsets.is_empty(), + "the field says `replace` itself, so no path needs reporting: {unsets:?}" + ); assert_eq!( delta.providers.llm.anthropic.beta_headers, - Some(vec!["one".to_owned()]) + Some(MergeableVec::Merged(MergedVec { + value: vec!["one".to_owned()], + strategy: Some(MergedVecStrategy::Replace), + dedup: None, + discard_when_merged: false, + })) ); } diff --git a/crates/jp_config/src/providers/llm/anthropic.rs b/crates/jp_config/src/providers/llm/anthropic.rs index 139f24fdd..c084f3a58 100644 --- a/crates/jp_config/src/providers/llm/anthropic.rs +++ b/crates/jp_config/src/providers/llm/anthropic.rs @@ -7,10 +7,11 @@ use serde::{Deserialize, Serialize}; use crate::{ assignment::{AssignKeyValue, AssignResult, KvAssignment, missing_key}, - delta::{PartialConfigDelta, delta_opt, delta_opt_vec, delta_opt_vec_at, path}, + delta::{PartialConfigDelta, delta_opt, delta_opt_mergeable_vec}, fill::FillDefaults, - internal::merge::append_vec_dedup, + internal::merge::vec_with_strategy, partial::{ToPartial, partial_opt}, + types::vec::MergeableVec, validate::Validator, }; @@ -69,7 +70,11 @@ pub struct AnthropicConfig { /// /// To find out which beta headers are available, see: /// - #[setting(default = vec![], merge = append_vec_dedup)] + #[setting( + default = MergeableVec::default(), + partial_via = MergeableVec::, + merge = vec_with_strategy, + )] pub beta_headers: Vec, } @@ -113,7 +118,7 @@ impl AssignKeyValue for PartialAnthropicConfig { value => Err(format!("expected a string, got {value}").into()), })?; } - _ if kv.p("beta_headers") => kv.try_some_vec_of_strings(&mut self.beta_headers)?, + _ if kv.p("beta_headers") => kv.try_some_mergeable_strings(&mut self.beta_headers)?, _ => return missing_key(&kv), } @@ -131,29 +136,14 @@ impl PartialConfigDelta for PartialAnthropicConfig { self.chain_on_max_tokens.as_ref(), next.chain_on_max_tokens, ), - beta_headers: delta_opt_vec(self.beta_headers.as_ref(), next.beta_headers), + beta_headers: delta_opt_mergeable_vec(self.beta_headers.as_ref(), next.beta_headers), } } - fn delta_with_unsets(&self, next: Self, prefix: &str, unsets: &mut Vec) -> Self { - Self { - // `auth` merges by replacement, so merging reaches `next` - // without clearing the field first. - auth: delta_opt(self.auth.as_ref(), next.auth), - api_key_env: delta_opt(self.api_key_env.as_ref(), next.api_key_env), - base_url: delta_opt(self.base_url.as_ref(), next.base_url), - chain_on_max_tokens: delta_opt( - self.chain_on_max_tokens.as_ref(), - next.chain_on_max_tokens, - ), - beta_headers: delta_opt_vec_at( - &path(prefix, "beta_headers"), - self.beta_headers.as_ref(), - next.beta_headers, - unsets, - ), - } - } + // No `delta_with_unsets`: `auth` merges by replacement and `beta_headers` + // carries its own strategy, so every field here is reachable by merging and + // none needs a path reported. The default implementation, which is the plain + // diff, is correct. } impl FillDefaults for PartialAnthropicConfig { @@ -180,7 +170,10 @@ impl ToPartial for AnthropicConfig { &self.chain_on_max_tokens, defaults.chain_on_max_tokens, ), - beta_headers: partial_opt(&self.beta_headers, defaults.beta_headers), + beta_headers: partial_opt( + &MergeableVec::from(self.beta_headers.clone()), + defaults.beta_headers, + ), } } } diff --git a/crates/jp_config/src/providers/llm/anthropic_tests.rs b/crates/jp_config/src/providers/llm/anthropic_tests.rs index 8d2ec3cd3..e3445f782 100644 --- a/crates/jp_config/src/providers/llm/anthropic_tests.rs +++ b/crates/jp_config/src/providers/llm/anthropic_tests.rs @@ -222,6 +222,6 @@ fn test_assign_beta_headers() { assert_eq!( partial.beta_headers, - Some(vec!["context-editing-2025-06-27".to_owned()]) + Some(vec!["context-editing-2025-06-27".to_owned()].into()) ); } diff --git a/crates/jp_config/src/snapshots/jp_config__tests__app_config_schema_shape.snap b/crates/jp_config/src/snapshots/jp_config__tests__app_config_schema_shape.snap index 7462967b0..2b2aa03b0 100644 --- a/crates/jp_config/src/snapshots/jp_config__tests__app_config_schema_shape.snap +++ b/crates/jp_config/src/snapshots/jp_config__tests__app_config_schema_shape.snap @@ -684,7 +684,13 @@ editor: EditorConfig program: string shell?: bool |: null - envs?: [string] + envs?: MergeableVec + |: [string] + |: MergedVec + dedup?: "inherit" | "true" | "false" | bool | null + discard_when_merged?: bool + strategy?: "append" | "prepend" | "replace" | null + value?: [string] inline: InlineEditorConfig edit_mode?: "emacs" | "vi" extends?: @@ -729,7 +735,13 @@ providers: ProviderConfig api_key_env?: string auth?: [string] base_url?: string - beta_headers?: [string] + beta_headers?: MergeableVec + |: [string] + |: MergedVec + dedup?: "inherit" | "true" | "false" | bool | null + discard_when_merged?: bool + strategy?: "append" | "prepend" | "replace" | null + value?: [string] chain_on_max_tokens?: bool cerebras: CerebrasConfig api_key_env?: string diff --git a/crates/jp_config/src/snapshots/jp_config__tests__partial_app_config_default_values.snap b/crates/jp_config/src/snapshots/jp_config__tests__partial_app_config_default_values.snap index b31a760db..0c2431989 100644 --- a/crates/jp_config/src/snapshots/jp_config__tests__partial_app_config_default_values.snap +++ b/crates/jp_config/src/snapshots/jp_config__tests__partial_app_config_default_values.snap @@ -459,7 +459,9 @@ Ok( true, ), beta_headers: Some( - [], + Vec( + [], + ), ), }, cerebras: PartialCerebrasConfig { From 677b78845fe3dcde4d7be9095d2a7a7f730166a2 Mon Sep 17 00:00:00 2001 From: Jean Mertz Date: Mon, 7 Sep 2026 13:17:41 +0200 Subject: [PATCH 4/7] feat(config, mcp): Let MCP server lists declare their merge strategy `providers.mcp..arguments` and `.variables` appended across layers with no way to say otherwise. Each now carries a strategy of its own: ```toml [providers.mcp.bookworm] arguments = { value = ["serve"], strategy = "replace" } ``` That closes the last of the reported bug. Dropping an argument is now recorded: the delta says `replace` and carries the whole list, where before appending could not shorten one, the difference went unrecorded, and every turn recomputed the same non-delta. `PartialMcpProviderConfig` loses its `delta_with_unsets` override, since no field there needs a path reported. The new `ordered_vec_with_strategy` merge keeps duplicates unless a config asks for deduplication, the opposite of the default. An argument list is a command line: `["--flag", "x", "--flag", "y"]` means something else once the second `--flag` is dropped. The opinion has to live on the merge function rather than on the field's default, because config layering merges partials before any defaults are filled in -- carrying it as a value let a plain list silently deduplicate, which a test caught. Signed-off-by: Jean Mertz --- crates/jp_cli/src/config_pipeline_tests.rs | 4 +- crates/jp_config/src/delta_tests.rs | 50 ++++++++++---- crates/jp_config/src/internal/merge.rs | 2 +- crates/jp_config/src/internal/merge/vec.rs | 27 ++++++++ crates/jp_config/src/lib_tests.rs | 31 +++++---- crates/jp_config/src/providers/mcp.rs | 73 +++++++++++---------- crates/jp_config/src/providers/mcp_tests.rs | 36 +++++----- crates/jp_config/src/unset_tests.rs | 8 +-- crates/jp_conversation/src/stream_tests.rs | 57 ++++++++++++++-- 9 files changed, 196 insertions(+), 92 deletions(-) diff --git a/crates/jp_cli/src/config_pipeline_tests.rs b/crates/jp_cli/src/config_pipeline_tests.rs index a87ea2959..2adf590e0 100644 --- a/crates/jp_cli/src/config_pipeline_tests.rs +++ b/crates/jp_cli/src/config_pipeline_tests.rs @@ -300,7 +300,7 @@ fn conversation_clears_allow_cfg_resets() { fn mcp_server(argument: &str) -> PartialMcpProviderConfig { PartialMcpProviderConfig::Stdio(PartialStdioConfig { command: Some("just".into()), - arguments: Some(vec![argument.to_owned()]), + arguments: Some(vec![argument.to_owned()].into()), ..PartialStdioConfig::default() }) } @@ -308,7 +308,7 @@ fn mcp_server(argument: &str) -> PartialMcpProviderConfig { /// The `arguments` of a server in a resolved partial. fn mcp_arguments(partial: &PartialAppConfig, server: &str) -> Option> { let PartialMcpProviderConfig::Stdio(config) = partial.providers.mcp.get(server)?; - config.arguments.clone() + config.arguments.as_deref().cloned() } /// The per-conversation layer is a resolved snapshot, not a contribution. diff --git a/crates/jp_config/src/delta_tests.rs b/crates/jp_config/src/delta_tests.rs index 0ad8c586e..60e463834 100644 --- a/crates/jp_config/src/delta_tests.rs +++ b/crates/jp_config/src/delta_tests.rs @@ -8,7 +8,13 @@ use crate::providers::mcp::{PartialMcpProviderConfig, PartialStdioConfig}; fn server(arguments: &[&str]) -> PartialMcpProviderConfig { PartialMcpProviderConfig::Stdio(PartialStdioConfig { command: Some("serve".into()), - arguments: Some(arguments.iter().map(|a| (*a).to_owned()).collect()), + arguments: Some( + arguments + .iter() + .map(|a| (*a).to_owned()) + .collect::>() + .into(), + ), ..PartialStdioConfig::default() }) } @@ -23,7 +29,7 @@ fn map(arguments: &[&str]) -> IndexMap { /// The `arguments` of a server entry, for asserting on a computed delta. fn arguments(entry: &PartialMcpProviderConfig) -> Option<&Vec> { let PartialMcpProviderConfig::Stdio(config) = entry; - config.arguments.as_ref() + config.arguments.as_deref() } #[test] @@ -92,35 +98,35 @@ fn an_appended_argument_reports_no_path() { ); } -/// A change appending cannot reach reports its path and carries the whole list. +/// A change appending cannot reach carries the whole list with `replace`. /// -/// The path is what the fold clears, which is what lets the list that follows -/// land verbatim instead of being appended to the one already there. +/// No path is reported: the field states the strategy itself, so the fold has +/// nothing to clear first. #[test] -fn a_dropped_argument_reports_its_path_and_carries_the_whole_list() { +fn a_dropped_argument_is_recorded_as_a_replacement() { let prev = config_with_server(&["--a", "--b"]); let next = config_with_server(&["--a"]); let mut unsets = Vec::new(); let delta = prev.delta_with_unsets(next, "", &mut unsets); - assert_eq!(unsets, ["providers.mcp.kagi.arguments"]); + assert!(unsets.is_empty(), "nothing to clear: {unsets:?}"); assert_eq!( arguments(&delta.providers.mcp["kagi"]), Some(&vec!["--a".to_owned()]) ); } -/// Reordering is not an extension either, so it clears too. +/// Reordering is not an extension either, so it replaces too. #[test] -fn a_reordered_argument_list_reports_its_path() { +fn a_reordered_argument_list_is_recorded_as_a_replacement() { let prev = config_with_server(&["--a", "--b"]); let next = config_with_server(&["--b", "--a"]); let mut unsets = Vec::new(); let delta = prev.delta_with_unsets(next, "", &mut unsets); - assert_eq!(unsets, ["providers.mcp.kagi.arguments"]); + assert!(unsets.is_empty(), "nothing to clear: {unsets:?}"); assert_eq!( arguments(&delta.providers.mcp["kagi"]), Some(&vec!["--b".to_owned(), "--a".to_owned()]) @@ -214,14 +220,30 @@ fn map_delta_keeps_the_changed_fields_of_an_entry() { assert_eq!(arguments(&delta["kagi"]), Some(&vec!["--b".to_owned()])); } -/// An entry that differs but has no expressible delta is left out entirely. +/// An entry whose delta carries nothing is left out entirely. /// /// Keeping it would hand the caller a map with one entry holding nothing, which /// reads as a change to every emptiness check upstream. +/// A stdio entry no longer reaches that state through its `arguments`, which +/// can now say `replace`, so the case is built directly. #[test] fn map_delta_drops_an_entry_whose_delta_is_empty() { - let prev = map(&["--a", "--b"]); - let next = map(&["--a"]); + let entry = |command: &str| -> IndexMap { + let mut map = IndexMap::new(); + map.insert( + "kagi".to_owned(), + PartialMcpProviderConfig::Stdio(PartialStdioConfig { + command: Some(command.into()), + ..PartialStdioConfig::default() + }), + ); + map + }; - assert!(delta_map(&prev, next).is_empty()); + // Equal entries are dropped by the equality check ahead of the delta. + assert!(delta_map(&entry("serve"), entry("serve")).is_empty()); + + // A differing entry contributes only what changed. + let delta = delta_map(&entry("serve"), entry("other")); + assert_eq!(delta.len(), 1); } diff --git a/crates/jp_config/src/internal/merge.rs b/crates/jp_config/src/internal/merge.rs index 1791664c6..c6ce2735b 100644 --- a/crates/jp_config/src/internal/merge.rs +++ b/crates/jp_config/src/internal/merge.rs @@ -8,4 +8,4 @@ mod vec; pub use map::map_with_strategy; pub use plain_vec::append_vec_dedup; pub use string::string_with_strategy; -pub use vec::vec_with_strategy; +pub use vec::{ordered_vec_with_strategy, vec_with_strategy}; diff --git a/crates/jp_config/src/internal/merge/vec.rs b/crates/jp_config/src/internal/merge/vec.rs index ddb7bc9ab..2e1d8810c 100644 --- a/crates/jp_config/src/internal/merge/vec.rs +++ b/crates/jp_config/src/internal/merge/vec.rs @@ -91,6 +91,33 @@ where })) } +/// Merge two lists whose repetition is significant. +/// +/// Identical to [`vec_with_strategy`] except that duplicates survive unless a +/// config explicitly asks for deduplication, rather than the other way round. +/// +/// An argument list is a command line: `["--flag", "x", "--flag", "y"]` means +/// something different once the second `--flag` is dropped. +/// Stating the opinion here rather than on the field's default is what makes it +/// hold during config layering, which merges partials before any defaults are +/// filled in. +pub fn ordered_vec_with_strategy( + prev: MergeableVec, + next: MergeableVec, + context: &(), +) -> MergeResult> +where + T: Clone + PartialEq + Serialize + DeserializeOwned + Schematic, +{ + let next = if dedup_flag(&prev).is_none() && dedup_flag(&next).is_none() { + with_dedup_flag(next, Some(false)) + } else { + next + }; + + vec_with_strategy(prev, next, context) +} + /// Extract the explicit dedup flag from a `MergeableVec`. const fn dedup_flag(v: &MergeableVec) -> Option { match v { diff --git a/crates/jp_config/src/lib_tests.rs b/crates/jp_config/src/lib_tests.rs index 473708add..1ca143390 100644 --- a/crates/jp_config/src/lib_tests.rs +++ b/crates/jp_config/src/lib_tests.rs @@ -724,16 +724,15 @@ fn an_explicit_inquiry_value_survives_a_partial_round_trip() { ); } -/// An MCP server whose only difference cannot be expressed as a delta does not -/// produce one. +/// A dropped MCP argument is recorded, rather than producing an event holding +/// nothing but the server's transport tag on every turn. /// -/// `arguments` merges by appending, so a dropped argument has no delta to -/// record. -/// Keeping the server in the map anyway makes the whole partial look non-empty, -/// and every turn then writes a `config_delta` event holding nothing but the -/// server's transport tag. +/// `arguments` carries its own merge strategy, so the delta says `replace` and +/// the fold reaches the shorter list. +/// Before it could, appending was unable to express the removal, the difference +/// went unrecorded, and the next turn computed the same non-delta again. #[test] -fn an_mcp_server_with_no_expressible_change_yields_no_delta() { +fn a_dropped_mcp_argument_is_recorded() { use crate::providers::mcp::{McpProviderConfig, StdioConfig}; let server = |arguments: &[&str]| { @@ -759,12 +758,18 @@ fn an_mcp_server_with_no_expressible_change_yields_no_delta() { let delta = prev.to_partial().delta(next.to_partial()); - assert!( - delta.providers.mcp.is_empty(), - "expected no server entry, got: {:?}", - delta.providers.mcp + let entry = delta + .providers + .mcp + .get("bookworm") + .expect("the change is recorded"); + + let crate::providers::mcp::PartialMcpProviderConfig::Stdio(stdio) = entry; + assert_eq!( + stdio.arguments.as_deref(), + Some(&vec!["serve".to_owned()]), + "the delta carries the whole list, since appending cannot shorten one" ); - assert!(delta.is_empty(), "expected an empty delta, got: {delta:?}"); } /// A union that names an expanded form contributes both the shorthand path and diff --git a/crates/jp_config/src/providers/mcp.rs b/crates/jp_config/src/providers/mcp.rs index 046660cf9..a50af372e 100644 --- a/crates/jp_config/src/providers/mcp.rs +++ b/crates/jp_config/src/providers/mcp.rs @@ -7,10 +7,10 @@ use serde::{Deserialize, Serialize}; use crate::{ assignment::{AssignKeyValue, AssignResult, KvAssignment, missing_key}, - delta::{ - PartialConfigDelta, delta_opt, delta_opt_partial, delta_opt_vec, delta_opt_vec_at, path, - }, + delta::{PartialConfigDelta, delta_opt, delta_opt_mergeable_vec, delta_opt_partial}, + internal::merge::ordered_vec_with_strategy, partial::{ToPartial, partial_opt, partial_opt_config}, + types::vec::MergeableVec, }; /// MCP provider configuration. @@ -35,8 +35,8 @@ impl PartialConfigDelta for PartialMcpProviderConfig { match (self, next) { (Self::Stdio(prev), Self::Stdio(next)) => Self::Stdio(PartialStdioConfig { command: delta_opt(prev.command.as_ref(), next.command), - arguments: delta_opt_vec(prev.arguments.as_ref(), next.arguments), - variables: delta_opt_vec(prev.variables.as_ref(), next.variables), + arguments: delta_opt_mergeable_vec(prev.arguments.as_ref(), next.arguments), + variables: delta_opt_mergeable_vec(prev.variables.as_ref(), next.variables), checksum: delta_opt_partial(prev.checksum.as_ref(), next.checksum), optional: delta_opt(prev.optional.as_ref(), next.optional), startup_timeout_secs: delta_opt( @@ -47,31 +47,8 @@ impl PartialConfigDelta for PartialMcpProviderConfig { } } - fn delta_with_unsets(&self, next: Self, prefix: &str, unsets: &mut Vec) -> Self { - match (self, next) { - (Self::Stdio(prev), Self::Stdio(next)) => Self::Stdio(PartialStdioConfig { - command: delta_opt(prev.command.as_ref(), next.command), - arguments: delta_opt_vec_at( - &path(prefix, "arguments"), - prev.arguments.as_ref(), - next.arguments, - unsets, - ), - variables: delta_opt_vec_at( - &path(prefix, "variables"), - prev.variables.as_ref(), - next.variables, - unsets, - ), - checksum: delta_opt_partial(prev.checksum.as_ref(), next.checksum), - optional: delta_opt(prev.optional.as_ref(), next.optional), - startup_timeout_secs: delta_opt( - prev.startup_timeout_secs.as_ref(), - next.startup_timeout_secs, - ), - }), - } - } + // No `delta_with_unsets`: `arguments` and `variables` state `replace` + // themselves now, so no field here needs a path reported. } impl McpProviderConfig { @@ -105,7 +82,18 @@ pub struct StdioConfig { pub command: PathBuf, /// The arguments to pass to the command. - #[setting(default, merge = schematic::merge::append_vec)] + /// + /// Appends to the list from any earlier layer. + /// Set a strategy to override that: + /// + /// ```toml + /// arguments = { value = ["serve"], strategy = "replace" } + /// ``` + #[setting( + default, + partial_via = MergeableVec::, + merge = ordered_vec_with_strategy, + )] pub arguments: Vec, /// The environment variables to expose to the command. @@ -113,7 +101,14 @@ pub struct StdioConfig { /// By default, the command inherits the environment of the parent process. /// You can use this to add additional environment variables, or override /// existing ones. - #[setting(default, merge = schematic::merge::append_vec)] + /// + /// Appends to the list from any earlier layer, and accepts a `strategy` the + /// same way `arguments` does. + #[setting( + default, + partial_via = MergeableVec::, + merge = ordered_vec_with_strategy, + )] pub variables: Vec, /// The binary checksum for the binary. @@ -151,8 +146,8 @@ impl AssignKeyValue for PartialStdioConfig { match kv.key_string().as_str() { "" => kv.try_merge_object(self)?, "command" => self.command = kv.try_some_from_str()?, - _ if kv.p("arguments") => kv.try_some_vec_of_strings(&mut self.arguments)?, - _ if kv.p("variables") => kv.try_some_vec_of_strings(&mut self.variables)?, + _ if kv.p("arguments") => kv.try_some_mergeable_strings(&mut self.arguments)?, + _ if kv.p("variables") => kv.try_some_mergeable_strings(&mut self.variables)?, _ if kv.p("checksum") => self.checksum.assign(kv)?, "optional" => self.optional = kv.try_some_bool()?, "startup_timeout_secs" => self.startup_timeout_secs = kv.try_some_u32()?, @@ -169,8 +164,14 @@ impl ToPartial for StdioConfig { PartialStdioConfig { command: partial_opt(&self.command, defaults.command), - arguments: partial_opt(&self.arguments, defaults.arguments), - variables: partial_opt(&self.variables, defaults.variables), + arguments: partial_opt( + &MergeableVec::from(self.arguments.clone()), + defaults.arguments, + ), + variables: partial_opt( + &MergeableVec::from(self.variables.clone()), + defaults.variables, + ), checksum: partial_opt_config(self.checksum.as_ref(), defaults.checksum), optional: partial_opt(&self.optional, defaults.optional), startup_timeout_secs: partial_opt( diff --git a/crates/jp_config/src/providers/mcp_tests.rs b/crates/jp_config/src/providers/mcp_tests.rs index b82421ea7..8eb5cb1da 100644 --- a/crates/jp_config/src/providers/mcp_tests.rs +++ b/crates/jp_config/src/providers/mcp_tests.rs @@ -2,7 +2,10 @@ use schematic::PartialConfig as _; use test_log::test; use super::*; -use crate::assignment::KvAssignment; +use crate::{ + assignment::KvAssignment, + types::vec::{MergeableVec, MergedVec}, +}; #[test] fn stdio_optional_defaults_to_false() { @@ -61,29 +64,32 @@ fn assign_startup_timeout_via_cli() { fn arguments_and_variables_append_across_layers() { use schematic::PartialConfig as _; - // Both fields declare `merge = append_vec`, so a later layer adds to the - // earlier one rather than replacing it. + // Both fields append a later layer onto the earlier one, and keep + // duplicates while doing it, so the merged value carries `dedup = false`. let mut base = PartialStdioConfig { - arguments: Some(vec!["serve".to_owned()]), - variables: Some(vec!["HOME".to_owned()]), + arguments: Some(vec!["serve".to_owned()].into()), + variables: Some(vec!["HOME".to_owned()].into()), ..Default::default() }; let overlay = PartialStdioConfig { - arguments: Some(vec!["--verbose".to_owned()]), - variables: Some(vec!["PATH".to_owned()]), + arguments: Some(vec!["--verbose".to_owned()].into()), + variables: Some(vec!["PATH".to_owned()].into()), ..Default::default() }; base.merge(&(), overlay).unwrap(); - assert_eq!( - base.arguments, - Some(vec!["serve".to_owned(), "--verbose".to_owned()]) - ); - assert_eq!( - base.variables, - Some(vec!["HOME".to_owned(), "PATH".to_owned()]) - ); + let ordered = |values: &[&str]| { + Some(MergeableVec::Merged(MergedVec { + value: values.iter().map(|v| (*v).to_owned()).collect(), + strategy: None, + dedup: Some(false), + discard_when_merged: false, + })) + }; + + assert_eq!(base.arguments, ordered(&["serve", "--verbose"])); + assert_eq!(base.variables, ordered(&["HOME", "PATH"])); } #[test] diff --git a/crates/jp_config/src/unset_tests.rs b/crates/jp_config/src/unset_tests.rs index 2f800f6ec..230384f6d 100644 --- a/crates/jp_config/src/unset_tests.rs +++ b/crates/jp_config/src/unset_tests.rs @@ -13,7 +13,7 @@ fn partial_with_server() -> PartialAppConfig { "bookworm".to_owned(), PartialMcpProviderConfig::Stdio(PartialStdioConfig { command: Some("just".into()), - arguments: Some(vec!["serve".to_owned(), "--verbose".to_owned()]), + arguments: Some(vec!["serve".to_owned(), "--verbose".to_owned()].into()), ..PartialStdioConfig::default() }), ); @@ -23,7 +23,7 @@ fn partial_with_server() -> PartialAppConfig { /// The `arguments` of the `bookworm` server, if the entry is present. fn arguments(partial: &PartialAppConfig) -> Option<&Vec> { let PartialMcpProviderConfig::Stdio(config) = partial.providers.mcp.get("bookworm")?; - config.arguments.as_ref() + config.arguments.as_deref() } #[test] @@ -148,7 +148,7 @@ fn a_cleared_list_takes_the_next_layers_value_verbatim() { next.providers.mcp.insert( "bookworm".to_owned(), PartialMcpProviderConfig::Stdio(PartialStdioConfig { - arguments: Some(vec!["serve".to_owned()]), + arguments: Some(vec!["serve".to_owned()].into()), ..PartialStdioConfig::default() }), ); @@ -169,7 +169,7 @@ fn an_uncleared_list_appends_the_next_layers_value() { next.providers.mcp.insert( "bookworm".to_owned(), PartialMcpProviderConfig::Stdio(PartialStdioConfig { - arguments: Some(vec!["serve".to_owned()]), + arguments: Some(vec!["serve".to_owned()].into()), ..PartialStdioConfig::default() }), ); diff --git a/crates/jp_conversation/src/stream_tests.rs b/crates/jp_conversation/src/stream_tests.rs index 2730e802b..74e80dfdc 100644 --- a/crates/jp_conversation/src/stream_tests.rs +++ b/crates/jp_conversation/src/stream_tests.rs @@ -45,13 +45,36 @@ fn stream_with_server(arguments: &[&str]) -> ConversationStream { /// A partial setting the `bookworm` server's arguments and nothing else. fn server_arguments_partial(arguments: &[&str]) -> jp_config::PartialAppConfig { + arguments_partial( + arguments + .iter() + .map(|a| (*a).to_owned()) + .collect::>(), + ) +} + +/// The same, with the list asking to replace rather than extend. +fn replacing_server_arguments_partial(arguments: &[&str]) -> jp_config::PartialAppConfig { + use jp_config::types::vec::{MergeableVec, MergedVec, MergedVecStrategy}; + + arguments_partial(MergeableVec::Merged(MergedVec { + value: arguments.iter().map(|a| (*a).to_owned()).collect(), + strategy: Some(MergedVecStrategy::Replace), + dedup: None, + discard_when_merged: false, + })) +} + +fn arguments_partial( + arguments: impl Into>, +) -> jp_config::PartialAppConfig { use jp_config::providers::mcp::{PartialMcpProviderConfig, PartialStdioConfig}; let mut partial = jp_config::PartialAppConfig::empty(); partial.providers.mcp.insert( "bookworm".to_owned(), PartialMcpProviderConfig::Stdio(PartialStdioConfig { - arguments: Some(arguments.iter().map(|a| (*a).to_owned()).collect()), + arguments: Some(arguments.into()), ..PartialStdioConfig::default() }), ); @@ -84,13 +107,33 @@ fn an_unset_clears_a_field_before_the_delta_merges() { assert_eq!(resolved_arguments(&stream), ["serve"]); } -/// Without the clear, the same values append instead of removing anything. +/// A list that states `replace` needs no clear to reach the same result. +/// +/// `arguments` carries its own merge strategy, so a delta can shorten the list +/// on its own. +/// `unsets` remains for what cannot say it: a scalar going away, and a list +/// whose merge strategy is fixed by its field. +#[test] +fn a_replacing_list_needs_no_unset() { + let mut stream = stream_with_server(&["serve", "--verbose"]); + + stream.add_config_delta(ApplyDelta::new( + delta_timestamp(), + replacing_server_arguments_partial(&["serve"]), + )); + + assert_eq!(resolved_arguments(&stream), ["serve"]); +} + +/// Left to append, the same values grow the list rather than shortening it. /// -/// `arguments` merges by appending, so a delta naming the one argument to keep -/// lands next to the one it meant to drop — the same result the identical -/// value in a config file produces. -/// Removing an argument needs the field cleared first, which is what `unsets` -/// is for. +/// `arguments` merges through `ordered_vec_with_strategy`, which keeps +/// duplicates: repetition is meaningful on a command line. +/// A delta naming the one argument to keep therefore lands next to the one it +/// meant to drop — the same result the identical value in a config file +/// produces. +/// Shortening the list needs the delta to say `replace`, as +/// `a_replacing_list_needs_no_unset` covers. #[test] fn without_an_unset_a_dropped_argument_appends_instead() { let mut stream = stream_with_server(&["serve", "--verbose"]); From 1146d31974d68c516d55a874967a8a1f8825b8ac Mon Sep 17 00:00:00 2001 From: Jean Mertz Date: Mon, 7 Sep 2026 13:26:16 +0200 Subject: [PATCH 5/7] feat(config): Let `stop_words` declare its merge strategy `assistant.model.parameters.stop_words` appended across layers with no way to say otherwise. It now carries a strategy of its own, at every site the model parameters are reached from -- the assistant, the inquiry assistant, the title generator and the reasoning summary model: ```toml [assistant.model.parameters] stop_words = { value = ["\n\n"], strategy = "replace" } ``` The field also deduplicates now, matching every other converted list. Two identical stop words have the same effect as one, so nothing about generation changes; `providers.mcp.*.arguments` stays the sole exception, because a repeated flag in a command line is not a duplicate. `PartialParametersConfig` keeps its `delta_with_unsets` override for the scalars around it, but `stop_words` no longer contributes to it: a dropped word is recorded as a replacement at whichever site it was reached from, with no path to report. Signed-off-by: Jean Mertz --- crates/jp_config/src/delta_tests.rs | 43 ++++++++++++++----- crates/jp_config/src/model/parameters.rs | 26 +++++------ .../jp_config/src/model/parameters_tests.rs | 6 +-- crates/jp_config/src/model_tests.rs | 4 +- 4 files changed, 51 insertions(+), 28 deletions(-) diff --git a/crates/jp_config/src/delta_tests.rs b/crates/jp_config/src/delta_tests.rs index 60e463834..bf8a85843 100644 --- a/crates/jp_config/src/delta_tests.rs +++ b/crates/jp_config/src/delta_tests.rs @@ -2,7 +2,10 @@ use indexmap::IndexMap; use test_log::test; use super::*; -use crate::providers::mcp::{PartialMcpProviderConfig, PartialStdioConfig}; +use crate::{ + providers::mcp::{PartialMcpProviderConfig, PartialStdioConfig}, + types::vec::{MergeableVec, MergedVec, MergedVecStrategy}, +}; /// A server entry with `arguments` set and every other field unset. fn server(arguments: &[&str]) -> PartialMcpProviderConfig { @@ -166,10 +169,15 @@ fn a_dropped_beta_header_is_recorded_as_a_replacement() { ); } -/// `stop_words` is reached through four separate paths; each reports its own. +/// A dropped stop word is recorded wherever the parameters are reached from. +/// +/// The list carries its own strategy, so each site records a replacement and +/// none needs a path reported. #[test] -fn a_dropped_stop_word_reports_the_path_it_was_reached_by() { - let words = |values: &[&str]| Some(values.iter().map(|v| (*v).to_owned()).collect::>()); +fn a_dropped_stop_word_is_recorded_at_every_site() { + let words = |values: &[&str]| -> Option> { + Some(values.iter().map(|v| (*v).to_owned()).collect()) + }; let mut prev = crate::PartialAppConfig::empty(); prev.assistant.model.parameters.stop_words = words(&["halt", "stop"]); @@ -190,14 +198,29 @@ fn a_dropped_stop_word_reports_the_path_it_was_reached_by() { let mut unsets = Vec::new(); let delta = prev.delta_with_unsets(next, "", &mut unsets); - unsets.sort(); - assert_eq!(unsets, [ - "assistant.model.parameters.stop_words", - "style.reasoning.summary_model.parameters.stop_words", - ]); + let replaced_with = |values: &[&str]| { + Some(MergeableVec::Merged(MergedVec { + value: values.iter().map(|v| (*v).to_owned()).collect(), + strategy: Some(MergedVecStrategy::Replace), + dedup: None, + discard_when_merged: false, + })) + }; + + assert!(unsets.is_empty(), "nothing to clear: {unsets:?}"); assert_eq!( delta.assistant.model.parameters.stop_words, - Some(vec!["halt".to_owned()]) + replaced_with(&["halt"]) + ); + assert_eq!( + delta + .style + .reasoning + .summary_model + .as_ref() + .map(|model| model.parameters.stop_words.clone()), + Some(replaced_with(&["halt"])), + "the second site records its own replacement" ); } diff --git a/crates/jp_config/src/model/parameters.rs b/crates/jp_config/src/model/parameters.rs index 4716ea28c..197e7f0e6 100644 --- a/crates/jp_config/src/model/parameters.rs +++ b/crates/jp_config/src/model/parameters.rs @@ -10,12 +10,13 @@ use crate::{ BoxedError, assignment::{AssignKeyValue, AssignResult, KvAssignment, missing_key}, delta::{ - PartialConfigDelta, delta_opt, delta_opt_at, delta_opt_partial, delta_opt_partial_at, - delta_opt_vec, delta_opt_vec_at, path, + PartialConfigDelta, delta_opt, delta_opt_at, delta_opt_mergeable_vec, delta_opt_partial, + delta_opt_partial_at, path, }, fill::{FillDefaults, fill_opt}, + internal::merge::vec_with_strategy, partial::{ToPartial, partial_opt, partial_opt_config, partial_opts}, - types::json_value::JsonValue, + types::{json_value::JsonValue, vec::MergeableVec}, }; /// Assistant-specific configuration. @@ -117,7 +118,11 @@ pub struct ParametersConfig { /// The `stop_words` parameter can be set to specific sequences, such as a /// period or specific word, to stop the model from generating text when it /// encounters these sequences. - #[setting(default, merge = schematic::merge::append_vec)] + #[setting( + default, + partial_via = MergeableVec::, + merge = vec_with_strategy, + )] pub stop_words: Vec, /// Other non-typed parameters that some models might support. @@ -219,7 +224,7 @@ impl AssignKeyValue for PartialParametersConfig { "temperature" => self.temperature = kv.try_some_f32()?, "top_p" => self.top_p = kv.try_some_f32()?, "top_k" => self.top_k = kv.try_some_u32()?, - _ if kv.p("stop_words") => kv.try_some_vec_of_strings(&mut self.stop_words)?, + _ if kv.p("stop_words") => kv.try_some_mergeable_strings(&mut self.stop_words)?, _ if kv.p("reasoning") => self.reasoning.assign(kv)?, _ => kv.assign_to_entry(self.other.get_or_insert_default())?, } @@ -237,7 +242,7 @@ impl PartialConfigDelta for PartialParametersConfig { temperature: delta_opt(self.temperature.as_ref(), next.temperature), top_p: delta_opt(self.top_p.as_ref(), next.top_p), top_k: delta_opt(self.top_k.as_ref(), next.top_k), - stop_words: delta_opt_vec(self.stop_words.as_ref(), next.stop_words), + stop_words: delta_opt_mergeable_vec(self.stop_words.as_ref(), next.stop_words), other: delta_opt(self.other.as_ref(), next.other), } } @@ -280,12 +285,7 @@ impl PartialConfigDelta for PartialParametersConfig { next.top_k, unsets, ), - stop_words: delta_opt_vec_at( - &path(prefix, "stop_words"), - self.stop_words.as_ref(), - next.stop_words, - unsets, - ), + stop_words: delta_opt_mergeable_vec(self.stop_words.as_ref(), next.stop_words), other: delta_opt_at( &path(prefix, "other"), self.other.as_ref(), @@ -320,7 +320,7 @@ impl ToPartial for ParametersConfig { temperature: partial_opts(self.temperature.as_ref(), None), top_p: partial_opts(self.top_p.as_ref(), None), top_k: partial_opts(self.top_k.as_ref(), None), - stop_words: partial_opt(&self.stop_words, None), + stop_words: partial_opt(&MergeableVec::from(self.stop_words.clone()), None), other: partial_opt(&self.other, None), } } diff --git a/crates/jp_config/src/model/parameters_tests.rs b/crates/jp_config/src/model/parameters_tests.rs index 00c3e9b90..55e00b7f4 100644 --- a/crates/jp_config/src/model/parameters_tests.rs +++ b/crates/jp_config/src/model/parameters_tests.rs @@ -157,11 +157,11 @@ fn stop_words_append_across_layers() { use schematic::PartialConfig as _; let mut base = PartialParametersConfig { - stop_words: Some(vec!["STOP".to_owned()]), + stop_words: Some(vec!["STOP".to_owned()].into()), ..Default::default() }; let overlay = PartialParametersConfig { - stop_words: Some(vec!["HALT".to_owned()]), + stop_words: Some(vec!["HALT".to_owned()].into()), ..Default::default() }; @@ -169,7 +169,7 @@ fn stop_words_append_across_layers() { assert_eq!( base.stop_words, - Some(vec!["STOP".to_owned(), "HALT".to_owned()]) + Some(vec!["STOP".to_owned(), "HALT".to_owned()].into()) ); } diff --git a/crates/jp_config/src/model_tests.rs b/crates/jp_config/src/model_tests.rs index 1089e158d..163a31c10 100644 --- a/crates/jp_config/src/model_tests.rs +++ b/crates/jp_config/src/model_tests.rs @@ -118,7 +118,7 @@ fn test_model_config_parameters() { p.assign(kv).unwrap(); assert_eq!( p.parameters.stop_words, - Some(vec!["foo".into(), "bar".into()]) + Some(vec!["foo".to_owned(), "bar".to_owned()].into()) ); let kv = KvAssignment::try_from_cli("parameters:", r#"{"max_tokens":42,"reasoning":{"effort":"low"},"temperature":0.42,"top_p":0.42,"top_k":42,"stop_words":["foo","bar"]}"#).unwrap(); @@ -138,7 +138,7 @@ fn test_model_config_parameters() { assert_eq!(p.parameters.top_k, Some(42)); assert_eq!( p.parameters.stop_words, - Some(vec!["foo".into(), "bar".into()]) + Some(vec!["foo".to_owned(), "bar".to_owned()].into()) ); let kv = KvAssignment::try_from_cli("parameters:", r#"{"reasoning":"off"}"#).unwrap(); From b8249d60023ac096a3e592b7cfc73e9e13f12c69 Mon Sep 17 00:00:00 2001 From: Jean Mertz Date: Mon, 7 Sep 2026 13:36:48 +0200 Subject: [PATCH 6/7] feat(config): Let `config_load_paths` declare its merge strategy `config_load_paths` accumulated across layers with duplicates dropped and no way to ask for anything else. It now carries a strategy of its own, which matters because `--cfg ` walks the list in order and takes the first directory holding a match: ```toml config_load_paths = { value = [".jp/agents"], strategy = "prepend" } ``` A user-level config can put its own directory ahead of the workspace's rather than behind it, and a workspace can replace an inherited list instead of adding to it. The plain list still appends and deduplicates, so nothing changes for a config that does not ask. This is the last plain list in the configuration, so every list-valued field now states how it merges. Four helpers that existed only to guess at it are gone: `delta_opt_vec`, `delta_opt_vec_at`, `append_vec_dedup` and `try_some_vec`. A list no longer needs a path reported in `unsets` to record a removal -- it carries `replace` and the fold does the rest, leaving `unsets` to the scalars, which genuinely cannot say it. `try_some_mergeable_vec` takes an element parser, so a list of anything that is not a `String` -- here `RelativePathBuf` -- can accept both the plain and the `{ value, strategy }` shape from `--cfg`. Signed-off-by: Jean Mertz --- crates/jp_cli/src/cmd/query_tests.rs | 2 +- crates/jp_cli/src/config_pipeline.rs | 2 +- crates/jp_config/src/assignment.rs | 41 ++++++------ crates/jp_config/src/delta.rs | 50 -------------- crates/jp_config/src/delta_tests.rs | 66 +++++++++++++------ crates/jp_config/src/internal/merge.rs | 2 - .../jp_config/src/internal/merge/plain_vec.rs | 47 ------------- .../src/internal/merge/plain_vec_tests.rs | 43 ------------ crates/jp_config/src/lib.rs | 22 ++++--- crates/jp_config/src/lib_tests.rs | 13 ++-- crates/jp_config/src/util_tests.rs | 3 +- 11 files changed, 93 insertions(+), 198 deletions(-) delete mode 100644 crates/jp_config/src/internal/merge/plain_vec.rs delete mode 100644 crates/jp_config/src/internal/merge/plain_vec_tests.rs diff --git a/crates/jp_cli/src/cmd/query_tests.rs b/crates/jp_cli/src/cmd/query_tests.rs index 71cee3596..5b99e741f 100644 --- a/crates/jp_cli/src/cmd/query_tests.rs +++ b/crates/jp_cli/src/cmd/query_tests.rs @@ -1803,7 +1803,7 @@ async fn query_sequence_new_cfg_profile_then_model_override_persists_for_plain_q .unwrap(); let mut base = AppConfig::new_test().to_partial(); - base.config_load_paths = Some(vec![RelativePathBuf::from(".jp/config")]); + base.config_load_paths = Some(vec![RelativePathBuf::from(".jp/config")].into()); base.providers.llm.aliases.insert( "gpt".to_owned(), ModelIdConfig { diff --git a/crates/jp_cli/src/config_pipeline.rs b/crates/jp_cli/src/config_pipeline.rs index f7c3b0e84..642fee195 100644 --- a/crates/jp_cli/src/config_pipeline.rs +++ b/crates/jp_cli/src/config_pipeline.rs @@ -439,7 +439,7 @@ fn resolve_cfg_args( let load_paths: Vec = base .config_load_paths .iter() - .flatten() + .flat_map(|paths| paths.iter()) .filter_map(|p| { Utf8PathBuf::try_from(p.to_path(root)) .inspect_err(|e| { diff --git a/crates/jp_config/src/assignment.rs b/crates/jp_config/src/assignment.rs index 851e7d1ef..f60697603 100644 --- a/crates/jp_config/src/assignment.rs +++ b/crates/jp_config/src/assignment.rs @@ -1000,23 +1000,6 @@ impl KvAssignment { Ok(()) } - /// Convenience method for [`Self::try_vec`] that takes an optional target. - /// - /// A `null` value clears the field to `None` rather than to an empty list; - /// the two merge differently. - pub(crate) fn try_some_vec( - self, - vec: &mut Option>, - parser: impl Fn(Self) -> Result, - ) -> Result<(), KvAssignmentError> { - if self.clears_collection() { - *vec = None; - return Ok(()); - } - - self.try_vec(vec.get_or_insert_default(), parser) - } - /// Specialized version of [`Self::try_vec`] for parsing a JSON array of /// strings. pub(crate) fn try_vec_of_strings(self, vec: &mut Vec) -> Result<(), KvAssignmentError> @@ -1057,12 +1040,13 @@ impl KvAssignment { /// is how the user declares a strategy for the field. /// The two are told apart by shape, the same way [`MergeableVec`]'s own /// deserializer does it: a sequence cannot be a table. - pub(crate) fn try_some_mergeable_strings( + pub(crate) fn try_some_mergeable_vec( self, vec: &mut Option>, + parser: impl Fn(Self) -> Result, ) -> Result<(), KvAssignmentError> where - T: Clone + From + DeserializeOwned, + T: Clone + DeserializeOwned, { // An absent list and an empty one merge differently: `None` lets a // later layer's value land verbatim, `Some([])` still runs the field's @@ -1083,12 +1067,29 @@ impl KvAssignment { } let mut elements = vec.take().map(MergeableVec::into_vec).unwrap_or_default(); - self.try_vec_of_strings(&mut elements)?; + self.try_vec(&mut elements, parser)?; *vec = Some(elements.into()); Ok(()) } + /// Convenience method for [`Self::try_some_mergeable_vec`] whose elements + /// are built from strings. + pub(crate) fn try_some_mergeable_strings( + self, + vec: &mut Option>, + ) -> Result<(), KvAssignmentError> + where + T: Clone + From + DeserializeOwned, + { + let parser = |kv: Self| match kv.value.clone().into_value() { + Value::String(v) => Ok(v.into()), + _ => type_error(kv.key(), &kv.value, &["string"]).map_err(Into::into), + }; + + self.try_some_mergeable_vec(vec, parser) + } + /// Try to parse the value as a JSON array of partial configs, and set or /// merge the elements. pub(crate) fn try_vec_of_nested(mut self, vec: &mut Vec) -> Result<(), KvAssignmentError> diff --git a/crates/jp_config/src/delta.rs b/crates/jp_config/src/delta.rs index 4e3042e86..a753246c6 100644 --- a/crates/jp_config/src/delta.rs +++ b/crates/jp_config/src/delta.rs @@ -64,34 +64,6 @@ pub fn path(prefix: &str, name: &str) -> String { } } -/// Delta for an appending list, reporting when appending cannot reach `next`. -/// -/// Returns the elements `next` adds while appending suffices. -/// Otherwise pushes `path` to `unsets` and returns the whole of `next`, which -/// is what the caller merges after clearing the field. -pub fn delta_opt_vec_at( - path: &str, - prev: Option<&Vec>, - next: Option>, - unsets: &mut Vec, -) -> Option> { - let next = next?; - let Some(prev) = prev else { - return Some(next); - }; - - // Appending reaches `next` exactly when `next` starts with `prev`; the - // delta is then the tail. Anything else — a dropped element, a reorder, an - // insertion in the middle — needs the field cleared first. - if next.starts_with(prev) { - let added = next[prev.len()..].to_vec(); - return (!added.is_empty()).then_some(added); - } - - unsets.push(path.to_owned()); - Some(next) -} - /// Calculate the delta between two strategy-carrying lists. /// /// Appending reaches `next` exactly when `next` starts with `prev` and repeats @@ -254,28 +226,6 @@ pub fn delta_opt_partial( } } -/// Calculate the delta between two optional vectors that merge by appending. -/// -/// The delta holds the elements `next` adds to `prev`, since that is what an -/// appending merge needs to reach `next` from `prev`. -/// -/// Returns `None` when `next` adds nothing. -/// An element dropped from `prev` cannot be expressed by appending, so a -/// removal also yields `None` rather than a delta that fails to remove -/// anything. -/// -/// Use [`delta_opt`] instead for a vector field that merges by replacement: -/// there the whole of `next` is the delta. -pub fn delta_opt_vec(prev: Option<&Vec>, next: Option>) -> Option> { - let next = next?; - let Some(prev) = prev else { - return Some(next); - }; - - let added = delta_vec(prev, next); - (!added.is_empty()).then_some(added) -} - /// Calculate the delta between two maps of partial configurations. /// /// An entry only `next` has is kept whole. diff --git a/crates/jp_config/src/delta_tests.rs b/crates/jp_config/src/delta_tests.rs index bf8a85843..2a653477e 100644 --- a/crates/jp_config/src/delta_tests.rs +++ b/crates/jp_config/src/delta_tests.rs @@ -35,44 +35,72 @@ fn arguments(entry: &PartialMcpProviderConfig) -> Option<&Vec> { config.arguments.as_deref() } +/// A list the fold appends, holding `values`. +fn appended(values: &[&str]) -> MergeableVec { + values.iter().map(|v| (*v).to_owned()).collect() +} + +/// A list the fold replaces, holding `values`. +fn replaced(values: &[&str]) -> MergeableVec { + MergeableVec::Merged(MergedVec { + value: values.iter().map(|v| (*v).to_owned()).collect(), + strategy: Some(MergedVecStrategy::Replace), + dedup: None, + discard_when_merged: false, + }) +} + #[test] -fn vec_delta_holds_the_added_elements() { - let prev = vec!["--a".to_owned()]; - let next = vec!["--a".to_owned(), "--b".to_owned()]; +fn vec_delta_appends_the_added_elements() { + let prev = MergeableVec::from(vec!["--a".to_owned()]); assert_eq!( - delta_opt_vec(Some(&prev), Some(next)), - Some(vec!["--b".to_owned()]) + delta_opt_mergeable_vec(Some(&prev), Some(appended(&["--a", "--b"]))), + Some(appended(&["--b"])) ); } -/// The first element added to an empty vector is still an addition. +/// The first element added to an empty list is still an addition. #[test] -fn vec_delta_holds_the_first_added_element() { - let prev = vec![]; - let next = vec!["--a".to_owned()]; +fn vec_delta_appends_the_first_added_element() { + let prev = MergeableVec::from(Vec::::new()); assert_eq!( - delta_opt_vec(Some(&prev), Some(next)), - Some(vec!["--a".to_owned()]) + delta_opt_mergeable_vec(Some(&prev), Some(appended(&["--a"]))), + Some(appended(&["--a"])) ); } #[test] fn unchanged_vec_has_no_delta() { - let prev = vec!["--a".to_owned()]; - let next = vec!["--a".to_owned()]; + let prev = MergeableVec::from(vec!["--a".to_owned()]); - assert_eq!(delta_opt_vec(Some(&prev), Some(next)), None); + assert_eq!( + delta_opt_mergeable_vec(Some(&prev), Some(appended(&["--a"]))), + None + ); } -/// Appending cannot take an element away, so a removal has no delta to record. +/// Appending cannot take an element away, so a removal replaces the list. #[test] -fn removed_vec_element_has_no_delta() { - let prev = vec!["--a".to_owned(), "--b".to_owned()]; - let next = vec!["--a".to_owned()]; +fn removed_vec_element_replaces_the_list() { + let prev = MergeableVec::from(vec!["--a".to_owned(), "--b".to_owned()]); - assert_eq!(delta_opt_vec(Some(&prev), Some(next)), None); + assert_eq!( + delta_opt_mergeable_vec(Some(&prev), Some(appended(&["--a"]))), + Some(replaced(&["--a"])) + ); +} + +/// Order is part of the value, so a reorder replaces the list too. +#[test] +fn reordered_vec_replaces_the_list() { + let prev = MergeableVec::from(vec!["--a".to_owned(), "--b".to_owned()]); + + assert_eq!( + delta_opt_mergeable_vec(Some(&prev), Some(appended(&["--b", "--a"]))), + Some(replaced(&["--b", "--a"])) + ); } /// A one-server config, keyed as `kagi`. diff --git a/crates/jp_config/src/internal/merge.rs b/crates/jp_config/src/internal/merge.rs index c6ce2735b..90a604b68 100644 --- a/crates/jp_config/src/internal/merge.rs +++ b/crates/jp_config/src/internal/merge.rs @@ -1,11 +1,9 @@ //! Internal merge strategies. mod map; -mod plain_vec; mod string; mod vec; pub use map::map_with_strategy; -pub use plain_vec::append_vec_dedup; pub use string::string_with_strategy; pub use vec::{ordered_vec_with_strategy, vec_with_strategy}; diff --git a/crates/jp_config/src/internal/merge/plain_vec.rs b/crates/jp_config/src/internal/merge/plain_vec.rs deleted file mode 100644 index c62441246..000000000 --- a/crates/jp_config/src/internal/merge/plain_vec.rs +++ /dev/null @@ -1,47 +0,0 @@ -//! Merge strategies for plain `Vec` fields. -//! -//! These operate on `Vec` directly, unlike [`vec_with_strategy`], which -//! reads its strategy from a [`MergeableVec`] wrapper. -//! -//! [`MergeableVec`]: crate::types::vec::MergeableVec -//! [`vec_with_strategy`]: super::vec_with_strategy - -use schematic::MergeResult; - -/// Append `next` to `prev`, dropping items already present. -/// -/// Comparison uses `PartialEq` and the first occurrence wins, so the result -/// keeps `prev`'s order with `next`'s new items appended. -/// -/// Only combining merges reach this function: schematic's `merge_setting` -/// invokes a merge strategy only when both layers supply a value, so a list -/// supplied by a single layer is stored as written, duplicates included. -/// That is the same rule `replace` follows on [`MergeableVec`] — repeated -/// items within one source are the author's own data, not something a merge of -/// two sources should rewrite. -/// -/// Deduplicating here rather than through a `transform` is deliberate: -/// transforms run in [`PartialConfig::finalize`], which JP's config pipeline -/// never calls — it merges layers with `load_partial` and resolves them with -/// `AppConfig::from_partial_with_defaults`. -/// -/// [`MergeableVec`]: crate::types::vec::MergeableVec -/// [`PartialConfig::finalize`]: schematic::PartialConfig::finalize -#[expect(clippy::unnecessary_wraps)] -pub fn append_vec_dedup( - mut prev: Vec, - next: Vec, - _: &C, -) -> MergeResult> { - for item in next { - if !prev.contains(&item) { - prev.push(item); - } - } - - Ok(Some(prev)) -} - -#[cfg(test)] -#[path = "plain_vec_tests.rs"] -mod tests; diff --git a/crates/jp_config/src/internal/merge/plain_vec_tests.rs b/crates/jp_config/src/internal/merge/plain_vec_tests.rs deleted file mode 100644 index 478ab2aac..000000000 --- a/crates/jp_config/src/internal/merge/plain_vec_tests.rs +++ /dev/null @@ -1,43 +0,0 @@ -use test_log::test; - -use super::*; - -#[test] -fn appends_new_items() { - let result = append_vec_dedup(vec![1, 2], vec![3, 4], &()) - .unwrap() - .unwrap(); - - assert_eq!(result, vec![1, 2, 3, 4]); -} - -#[test] -fn drops_items_already_present() { - // Two config layers naming the same directory contribute it once, which is - // what `config_load_paths` and `beta_headers` need: the resolved list is - // searched (respectively sent) in order, and a repeat is pure noise. - let result = append_vec_dedup(vec!["a", "b"], vec!["b", "c"], &()) - .unwrap() - .unwrap(); - - assert_eq!(result, vec!["a", "b", "c"]); -} - -#[test] -fn keeps_first_occurrence_order() { - let result = append_vec_dedup(vec![3, 1], vec![2, 1, 3], &()) - .unwrap() - .unwrap(); - - assert_eq!(result, vec![3, 1, 2]); -} - -#[test] -fn collapses_repeats_inside_the_incoming_layer() { - // Only reachable when two layers combine — a list supplied by a single - // layer never reaches this function, so its own repeats are kept. See - // `test_load_partial_at_path_keeps_repeats_from_a_single_file`. - let result = append_vec_dedup(vec![1], vec![2, 2], &()).unwrap().unwrap(); - - assert_eq!(result, vec![1, 2]); -} diff --git a/crates/jp_config/src/lib.rs b/crates/jp_config/src/lib.rs index 0cc4681f3..441a24dc1 100644 --- a/crates/jp_config/src/lib.rs +++ b/crates/jp_config/src/lib.rs @@ -77,7 +77,7 @@ use crate::{ assignment::{AssignKeyValue, AssignResult, KvAssignment, missing_key, type_error}, assistant::{AssistantConfig, PartialAssistantConfig}, conversation::{ConversationConfig, PartialConversationConfig}, - delta::{delta_opt_vec, delta_opt_vec_at, path as delta_path}, + delta::{delta_opt_mergeable_vec, path as delta_path}, editor::{EditorConfig, PartialEditorConfig}, interrupt::{InterruptConfig, PartialInterruptConfig}, loader::{LoaderConfig, PartialLoaderConfig}, @@ -86,7 +86,7 @@ use crate::{ providers::{PartialProviderConfig, ProviderConfig}, style::{PartialStyleConfig, StyleConfig}, template::{PartialTemplateConfig, TemplateConfig}, - types::extending_path::ExtendingRelativePath, + types::{extending_path::ExtendingRelativePath, vec::MergeableVec}, user::{PartialUserConfig, UserConfig}, }; @@ -131,7 +131,10 @@ pub struct AppConfig { /// /// For example, to load `.jp/agents/dev.toml`, add `.jp/agents` to this /// list and run `jp query --cfg dev`. - #[setting(merge = internal::merge::append_vec_dedup)] + #[setting( + partial_via = MergeableVec::, + merge = internal::merge::vec_with_strategy, + )] pub config_load_paths: Vec, /// Extends the configuration from the given files. @@ -238,7 +241,7 @@ impl AssignKeyValue for PartialAppConfig { _ => type_error(kv.key(), &kv.value, &["string"]).map_err(Into::into), }; - kv.try_some_vec(&mut self.config_load_paths, parser)?; + kv.try_some_mergeable_vec(&mut self.config_load_paths, parser)?; } _ if kv.p("assistant") => self.assistant.assign(kv)?, _ if kv.p("conversation") => self.conversation.assign(kv)?, @@ -264,7 +267,7 @@ impl PartialConfigDelta for PartialAppConfig { inherit: None, loader: PartialLoaderConfig::default(), - config_load_paths: delta_opt_vec( + config_load_paths: delta_opt_mergeable_vec( self.config_load_paths.as_ref(), next.config_load_paths, ), @@ -288,11 +291,9 @@ impl PartialConfigDelta for PartialAppConfig { inherit: None, loader: PartialLoaderConfig::default(), - config_load_paths: delta_opt_vec_at( - &delta_path(prefix, "config_load_paths"), + config_load_paths: delta_opt_mergeable_vec( self.config_load_paths.as_ref(), next.config_load_paths, - unsets, ), assistant: self.assistant.delta_with_unsets( @@ -354,7 +355,10 @@ impl ToPartial for AppConfig { let mut partial = Self::Partial { inherit: partial_opt(&self.inherit, defaults.inherit), - config_load_paths: partial_opt(&self.config_load_paths, defaults.config_load_paths), + config_load_paths: partial_opt( + &MergeableVec::from(self.config_load_paths.clone()), + defaults.config_load_paths, + ), extends: partial_opt(&self.extends, defaults.extends), loader: self.loader.to_partial(), assistant: self.assistant.to_partial(), diff --git a/crates/jp_config/src/lib_tests.rs b/crates/jp_config/src/lib_tests.rs index 1ca143390..376f40fa3 100644 --- a/crates/jp_config/src/lib_tests.rs +++ b/crates/jp_config/src/lib_tests.rs @@ -891,7 +891,10 @@ fn test_partial_app_config_assign() { let kv = KvAssignment::try_from_cli("config_load_paths", "foo,bar").unwrap(); p.assign(kv).unwrap(); - assert_eq!(p.config_load_paths, Some(vec!["foo".into(), "bar".into()])); + assert_eq!( + p.config_load_paths, + Some(vec![RelativePathBuf::from("foo"), "bar".into()].into()) + ); let kv = KvAssignment::try_from_cli("assistant.name", "foo").unwrap(); p.assign(kv).unwrap(); @@ -924,10 +927,12 @@ fn config_load_paths_append_across_layers() { // matters downstream: `--cfg ` resolution walks the list and takes // the first directory that holds a matching file. let mut base = PartialAppConfig::empty(); - base.config_load_paths = Some(vec![".jp/global".into(), ".jp/shared".into()]); + base.config_load_paths = + Some(vec![RelativePathBuf::from(".jp/global"), ".jp/shared".into()].into()); let mut overlay = PartialAppConfig::empty(); - overlay.config_load_paths = Some(vec![".jp/shared".into(), ".jp/workspace".into()]); + overlay.config_load_paths = + Some(vec![RelativePathBuf::from(".jp/shared"), ".jp/workspace".into()].into()); base.merge(&(), overlay).unwrap(); @@ -936,7 +941,7 @@ fn config_load_paths_append_across_layers() { ".jp/shared".into(), ".jp/workspace".into(), ]; - assert_eq!(base.config_load_paths, Some(want)); + assert_eq!(base.config_load_paths, Some(want.into())); } #[test] diff --git a/crates/jp_config/src/util_tests.rs b/crates/jp_config/src/util_tests.rs index ccbb3d786..9862313ff 100644 --- a/crates/jp_config/src/util_tests.rs +++ b/crates/jp_config/src/util_tests.rs @@ -972,9 +972,8 @@ fn test_load_partial_at_path_repeat_visit_keeps_last_position() { fn load_paths(partial: &PartialAppConfig) -> Vec<&str> { partial .config_load_paths - .as_deref() - .unwrap_or_default() .iter() + .flat_map(|paths| paths.iter()) .map(|p| p.as_str()) .collect() } From 556df00ebc09341838d0cdf39347f8e6aa580238 Mon Sep 17 00:00:00 2001 From: Jean Mertz Date: Thu, 10 Sep 2026 22:51:15 +0200 Subject: [PATCH 7/7] review feedback Signed-off-by: Jean Mertz --- .../schematic_macros/src/common/field.rs | 12 +- .../schematic_macros/src/config/field.rs | 10 +- crates/jp_config/src/assignment.rs | 51 +++- crates/jp_config/src/editor.rs | 4 +- crates/jp_config/src/lib.rs | 7 +- crates/jp_config/src/list_strategy_tests.rs | 243 ++++++++++++++++++ crates/jp_config/src/model/parameters.rs | 4 +- .../jp_config/src/providers/llm/anthropic.rs | 4 +- crates/jp_config/src/providers/mcp.rs | 8 +- ...onfig__tests__app_config_schema_shape.snap | 70 ++++- 10 files changed, 387 insertions(+), 26 deletions(-) create mode 100644 crates/jp_config/src/list_strategy_tests.rs diff --git a/crates/contrib/schematic_macros/src/common/field.rs b/crates/contrib/schematic_macros/src/common/field.rs index ab4fc8d40..ea7751096 100644 --- a/crates/contrib/schematic_macros/src/common/field.rs +++ b/crates/contrib/schematic_macros/src/common/field.rs @@ -163,7 +163,17 @@ impl Field<'_> { // can carry a wrapper that knows its own merge strategy. The // partial holds the via type and `generate_from_partial_value` // converts back to the field's own type. - FieldValue::value(result.partial_via_ty.as_ref().unwrap_or(result.value)) + let mut value_type = + FieldValue::value(result.partial_via_ty.as_ref().unwrap_or(result.value)); + if result.partial_via_ty.is_some() + && let FieldValue::Value { info, .. } = &mut value_type + { + let mut field_info = TypeInfo::default(); + extract_inner_type(result.value, &mut field_info); + info.optional = field_info.optional; + info.boxed = field_info.boxed; + } + value_type }; result diff --git a/crates/contrib/schematic_macros/src/config/field.rs b/crates/contrib/schematic_macros/src/config/field.rs index f0b270cdb..fca2bedc8 100644 --- a/crates/contrib/schematic_macros/src/config/field.rs +++ b/crates/contrib/schematic_macros/src/config/field.rs @@ -170,7 +170,15 @@ impl Field<'_> { let inner = convert(quote! { value }); quote! { partial.#key.map(|value| Box::new(#inner)) } } else { - let inner = convert(quote! { partial.#key }); + let inner = if !via { + quote! { partial.#key } + } else if self.is_required() { + convert( + quote! { partial.#key.ok_or(schematic::ConfigError::MissingRequired{ fields: { let mut fields = fields.clone(); fields.push(#key_quoted.to_owned()); fields } })? }, + ) + } else { + convert(quote! { partial.#key.unwrap_or_default() }) + }; quote! { Box::new(#inner) } } } else { diff --git a/crates/jp_config/src/assignment.rs b/crates/jp_config/src/assignment.rs index f60697603..632fc541b 100644 --- a/crates/jp_config/src/assignment.rs +++ b/crates/jp_config/src/assignment.rs @@ -8,7 +8,7 @@ use std::{fmt, str::FromStr}; use indexmap::IndexMap; -use schematic::PartialConfig; +use schematic::{MergeResult, PartialConfig}; use serde::{Serialize, de::DeserializeOwned}; use serde_json::{Value, from_str}; @@ -1000,6 +1000,23 @@ impl KvAssignment { Ok(()) } + /// Convenience method for [`Self::try_vec`] that takes an optional target. + /// + /// A `null` value clears the field to `None` rather than to an empty list; + /// the two merge differently. + pub(crate) fn try_some_vec( + self, + vec: &mut Option>, + parser: impl Fn(Self) -> Result, + ) -> Result<(), KvAssignmentError> { + if self.clears_collection() { + *vec = None; + return Ok(()); + } + + self.try_vec(vec.get_or_insert_default(), parser) + } + /// Specialized version of [`Self::try_vec`] for parsing a JSON array of /// strings. pub(crate) fn try_vec_of_strings(self, vec: &mut Vec) -> Result<(), KvAssignmentError> @@ -1040,10 +1057,13 @@ impl KvAssignment { /// is how the user declares a strategy for the field. /// The two are told apart by shape, the same way [`MergeableVec`]'s own /// deserializer does it: a sequence cannot be a table. + /// Merge-assigned objects use the field's `merge` function; appends and + /// indexed edits preserve the existing wrapper metadata. pub(crate) fn try_some_mergeable_vec( self, vec: &mut Option>, parser: impl Fn(Self) -> Result, + merge: impl Fn(MergeableVec, MergeableVec, &()) -> MergeResult>, ) -> Result<(), KvAssignmentError> where T: Clone + DeserializeOwned, @@ -1058,15 +1078,29 @@ impl KvAssignment { // An object declares a strategy alongside the value, so it is parsed as // the wrapper rather than element by element. - if let KvValue::Json(value @ Value::Object(_)) = self.value.clone() { - let merged = - serde_json::from_value(value).map_err(|error| kv_error(&self.key, error))?; - - *vec = Some(merged); + if self.key.is_empty() + && let KvValue::Json(value @ Value::Object(_)) = self.value.clone() + { + let next = serde_json::from_value(value).map_err(|error| kv_error(&self.key, error))?; + + *vec = if self.is_merge() + && let Some(prev) = vec.as_ref() + { + merge(prev.clone(), next, &()).or_else(|error| { + assignment_error(&self.key, self.value.clone().into_value(), error.into()) + })? + } else { + Some(next) + }; return Ok(()); } - let mut elements = vec.take().map(MergeableVec::into_vec).unwrap_or_default(); + // Appends and indexed edits change the value, not its layering policy. + if self.is_merge() || !self.key.is_empty() { + return self.try_vec(vec.get_or_insert_default(), parser); + } + + let mut elements = Vec::new(); self.try_vec(&mut elements, parser)?; *vec = Some(elements.into()); @@ -1078,6 +1112,7 @@ impl KvAssignment { pub(crate) fn try_some_mergeable_strings( self, vec: &mut Option>, + merge: impl Fn(MergeableVec, MergeableVec, &()) -> MergeResult>, ) -> Result<(), KvAssignmentError> where T: Clone + From + DeserializeOwned, @@ -1087,7 +1122,7 @@ impl KvAssignment { _ => type_error(kv.key(), &kv.value, &["string"]).map_err(Into::into), }; - self.try_some_mergeable_vec(vec, parser) + self.try_some_mergeable_vec(vec, parser, merge) } /// Try to parse the value as a JSON array of partial configs, and set or diff --git a/crates/jp_config/src/editor.rs b/crates/jp_config/src/editor.rs index e6c790450..04fe58e35 100644 --- a/crates/jp_config/src/editor.rs +++ b/crates/jp_config/src/editor.rs @@ -119,7 +119,9 @@ impl AssignKeyValue for PartialEditorConfig { match kv.key_string().as_str() { "" => kv.try_merge_object(self)?, _ if kv.p("cmd") => self.cmd.assign(kv)?, - _ if kv.p("envs") => kv.try_some_mergeable_strings(&mut self.envs)?, + _ if kv.p("envs") => { + kv.try_some_mergeable_strings(&mut self.envs, vec_with_strategy)?; + } _ if kv.p("inline") => self.inline.assign(kv)?, _ => return missing_key(&kv), } diff --git a/crates/jp_config/src/lib.rs b/crates/jp_config/src/lib.rs index 441a24dc1..1dcdb2453 100644 --- a/crates/jp_config/src/lib.rs +++ b/crates/jp_config/src/lib.rs @@ -79,6 +79,7 @@ use crate::{ conversation::{ConversationConfig, PartialConversationConfig}, delta::{delta_opt_mergeable_vec, path as delta_path}, editor::{EditorConfig, PartialEditorConfig}, + internal::merge::vec_with_strategy, interrupt::{InterruptConfig, PartialInterruptConfig}, loader::{LoaderConfig, PartialLoaderConfig}, partial::partial_opt, @@ -241,7 +242,7 @@ impl AssignKeyValue for PartialAppConfig { _ => type_error(kv.key(), &kv.value, &["string"]).map_err(Into::into), }; - kv.try_some_mergeable_vec(&mut self.config_load_paths, parser)?; + kv.try_some_mergeable_vec(&mut self.config_load_paths, parser, vec_with_strategy)?; } _ if kv.p("assistant") => self.assistant.assign(kv)?, _ if kv.p("conversation") => self.conversation.assign(kv)?, @@ -849,3 +850,7 @@ mod tests; #[cfg(test)] #[path = "unset_tests.rs"] mod unset_tests; + +#[cfg(test)] +#[path = "list_strategy_tests.rs"] +mod list_strategy_tests; diff --git a/crates/jp_config/src/list_strategy_tests.rs b/crates/jp_config/src/list_strategy_tests.rs new file mode 100644 index 000000000..a4b418ff4 --- /dev/null +++ b/crates/jp_config/src/list_strategy_tests.rs @@ -0,0 +1,243 @@ +use schematic::{ + Config, ConfigError, PartialConfig as _, SchemaBuilder, SchemaType, Schematic as _, +}; +use serde_json::{from_str as from_json, to_string as to_json}; +use toml::{from_str as from_toml, to_string as to_toml}; + +use crate::{ + PartialAppConfig, assignment::AssignKeyValue as _, editor::EditorConfig, + providers::mcp::PartialStdioConfig, types::vec::MergeableVec, +}; + +/// Scalar-list wrappers exercise the declared field's outer containers. +#[derive(Debug, Clone, PartialEq, Config)] +#[config(rename_all = "snake_case")] +#[expect( + clippy::box_collection, + reason = "the fixture tests boxed macro fields" +)] +struct WrappedLists { + #[setting(partial_via = MergeableVec::)] + optional: Option>, + #[setting(partial_via = MergeableVec::)] + boxed: Box>, + #[setting(partial_via = MergeableVec::)] + optional_boxed: Option>>, + #[setting(required, partial_via = MergeableVec::)] + required_boxed: Box>, +} + +#[test] +fn scalar_list_via_preserves_optional_and_boxed_fields() { + let partial = from_json( + r#"{ + "optional": {"value":["optional"],"strategy":"replace"}, + "boxed": ["boxed"], + "optional_boxed": ["optional_boxed"], + "required_boxed": ["required_boxed"] + }"#, + ) + .unwrap(); + let config = WrappedLists::from_partial(partial, vec![]).unwrap(); + assert_eq!(config.optional, Some(vec!["optional".to_owned()])); + assert_eq!(*config.boxed, vec!["boxed".to_owned()]); + assert_eq!( + config.optional_boxed.as_deref(), + Some(&vec!["optional_boxed".to_owned()]) + ); + assert_eq!(*config.required_boxed, vec!["required_boxed".to_owned()]); +} + +#[test] +fn scalar_list_via_preserves_absence_defaults_and_required_validation() { + let partial = PartialWrappedLists { + required_boxed: Some(vec!["required".to_owned()].into()), + ..Default::default() + }; + let config = WrappedLists::from_partial(partial, vec![]).unwrap(); + assert_eq!(config.optional, None); + assert_eq!(*config.boxed, Vec::::new()); + assert_eq!(config.optional_boxed, None); + + let error = WrappedLists::from_partial(PartialWrappedLists::empty(), vec![]).unwrap_err(); + let ConfigError::MissingRequired { fields } = error else { + panic!("expected a missing required field, got {error}"); + }; + assert_eq!(fields, ["required_boxed"]); +} + +#[test] +fn object_merge_assignment_appends_and_preserves_repeated_flags() { + let mut partial = PartialStdioConfig { + arguments: Some(vec!["serve".to_owned(), "--flag".to_owned(), "x".to_owned()].into()), + ..Default::default() + }; + partial + .assign( + r#"arguments:+={"value":["--flag","y"],"strategy":"append"}"# + .parse() + .unwrap(), + ) + .unwrap(); + + assert_eq!( + partial.arguments.as_deref(), + Some(&vec![ + "serve".to_owned(), + "--flag".to_owned(), + "x".to_owned(), + "--flag".to_owned(), + "y".to_owned(), + ]) + ); +} + +#[test] +fn object_merge_assignment_prepends() { + let mut partial = PartialStdioConfig { + arguments: Some(vec!["serve".to_owned()].into()), + ..Default::default() + }; + partial + .assign( + r#"arguments:+={"value":["--verbose"],"strategy":"prepend"}"# + .parse() + .unwrap(), + ) + .unwrap(); + + assert_eq!( + partial.arguments.as_deref(), + Some(&vec!["--verbose".to_owned(), "serve".to_owned()]) + ); +} + +#[test] +fn object_assignment_and_replace_strategy_replace() { + let mut partial = PartialStdioConfig { + arguments: Some(vec!["serve".to_owned()].into()), + ..Default::default() + }; + partial + .assign( + r#"arguments:={"value":["first"],"strategy":"append"}"# + .parse() + .unwrap(), + ) + .unwrap(); + assert_eq!( + partial.arguments.as_deref(), + Some(&vec!["first".to_owned()]) + ); + + partial + .assign( + r#"arguments:+={"value":["second"],"strategy":"replace"}"# + .parse() + .unwrap(), + ) + .unwrap(); + assert_eq!( + partial.arguments.as_deref(), + Some(&vec!["second".to_owned()]) + ); +} + +#[test] +fn object_merge_assignment_uses_the_fields_dedup_policy() { + let mut partial = PartialAppConfig::empty(); + partial + .assign("editor.envs=EDITOR,VISUAL".parse().unwrap()) + .unwrap(); + partial + .assign( + r#"editor.envs:+={"value":["VISUAL","MY_EDITOR"],"strategy":"append"}"# + .parse() + .unwrap(), + ) + .unwrap(); + + assert_eq!( + partial.editor.envs.as_deref(), + Some(&vec![ + "EDITOR".to_owned(), + "VISUAL".to_owned(), + "MY_EDITOR".to_owned(), + ]) + ); +} + +#[test] +fn appending_to_a_replacement_survives_serialization_and_layering() { + let mut overlay = PartialAppConfig::empty(); + overlay + .assign( + r#"editor.envs:={"value":["VISUAL"],"strategy":"replace"}"# + .parse() + .unwrap(), + ) + .unwrap(); + overlay + .assign("editor.envs+=MY_EDITOR".parse().unwrap()) + .unwrap(); + + let serialized = to_json(&overlay.editor.envs).unwrap(); + // MergedVec serializes its discard flag even when it is false. + assert_eq!( + serialized, + r#"{"value":["VISUAL","MY_EDITOR"],"strategy":"replace","discard_when_merged":false}"# + ); + let overlay = from_toml(&to_toml(&overlay).unwrap()).unwrap(); + let mut base = PartialAppConfig::empty(); + base.assign("editor.envs=EDITOR".parse().unwrap()).unwrap(); + base.merge(&(), overlay).unwrap(); + + assert_eq!( + base.editor.envs.as_deref(), + Some(&vec!["VISUAL".to_owned(), "MY_EDITOR".to_owned()]) + ); +} + +#[test] +fn indexed_assignment_preserves_metadata_but_whole_list_assignment_replaces_it() { + let mut partial = PartialAppConfig::empty(); + partial.assign(r#"editor.envs:={"value":["VISUAL"],"strategy":"replace","dedup":false,"discard_when_merged":true}"#.parse().unwrap()).unwrap(); + partial + .assign("editor.envs.0=MY_EDITOR".parse().unwrap()) + .unwrap(); + assert_eq!( + to_json(&partial.editor.envs).unwrap(), + r#"{"value":["MY_EDITOR"],"strategy":"replace","dedup":false,"discard_when_merged":true}"# + ); + + partial + .assign("editor.envs=EDITOR".parse().unwrap()) + .unwrap(); + assert_eq!(to_json(&partial.editor.envs).unwrap(), r#"["EDITOR"]"#); + partial + .assign("editor.envs:=null".parse().unwrap()) + .unwrap(); + assert_eq!(partial.editor.envs, None); +} + +#[test] +fn scalar_list_schema_describes_array_and_strategy_object() { + let SchemaType::Struct(editor) = EditorConfig::build_schema(SchemaBuilder::default()).ty else { + panic!("editor schema must be a struct"); + }; + let SchemaType::Union(envs) = &editor.fields["envs"].schema.ty else { + panic!("envs schema must accept both a list and a strategy object"); + }; + assert_eq!(envs.variants_types.len(), 2); + assert!(matches!(envs.variants_types[0].ty, SchemaType::Array(_))); + let SchemaType::Struct(object) = &envs.variants_types[1].ty else { + panic!("the expanded list schema must be an object"); + }; + assert!(matches!( + object.fields["value"].schema.ty, + SchemaType::Array(_) + )); + assert!(object.fields.contains_key("strategy")); + assert!(object.fields.contains_key("dedup")); + assert!(object.fields.contains_key("discard_when_merged")); +} diff --git a/crates/jp_config/src/model/parameters.rs b/crates/jp_config/src/model/parameters.rs index 197e7f0e6..614edb2b8 100644 --- a/crates/jp_config/src/model/parameters.rs +++ b/crates/jp_config/src/model/parameters.rs @@ -224,7 +224,9 @@ impl AssignKeyValue for PartialParametersConfig { "temperature" => self.temperature = kv.try_some_f32()?, "top_p" => self.top_p = kv.try_some_f32()?, "top_k" => self.top_k = kv.try_some_u32()?, - _ if kv.p("stop_words") => kv.try_some_mergeable_strings(&mut self.stop_words)?, + _ if kv.p("stop_words") => { + kv.try_some_mergeable_strings(&mut self.stop_words, vec_with_strategy)?; + } _ if kv.p("reasoning") => self.reasoning.assign(kv)?, _ => kv.assign_to_entry(self.other.get_or_insert_default())?, } diff --git a/crates/jp_config/src/providers/llm/anthropic.rs b/crates/jp_config/src/providers/llm/anthropic.rs index c084f3a58..90d73cc6c 100644 --- a/crates/jp_config/src/providers/llm/anthropic.rs +++ b/crates/jp_config/src/providers/llm/anthropic.rs @@ -118,7 +118,9 @@ impl AssignKeyValue for PartialAnthropicConfig { value => Err(format!("expected a string, got {value}").into()), })?; } - _ if kv.p("beta_headers") => kv.try_some_mergeable_strings(&mut self.beta_headers)?, + _ if kv.p("beta_headers") => { + kv.try_some_mergeable_strings(&mut self.beta_headers, vec_with_strategy)?; + } _ => return missing_key(&kv), } diff --git a/crates/jp_config/src/providers/mcp.rs b/crates/jp_config/src/providers/mcp.rs index a50af372e..2a3290e7b 100644 --- a/crates/jp_config/src/providers/mcp.rs +++ b/crates/jp_config/src/providers/mcp.rs @@ -146,8 +146,12 @@ impl AssignKeyValue for PartialStdioConfig { match kv.key_string().as_str() { "" => kv.try_merge_object(self)?, "command" => self.command = kv.try_some_from_str()?, - _ if kv.p("arguments") => kv.try_some_mergeable_strings(&mut self.arguments)?, - _ if kv.p("variables") => kv.try_some_mergeable_strings(&mut self.variables)?, + _ if kv.p("arguments") => { + kv.try_some_mergeable_strings(&mut self.arguments, ordered_vec_with_strategy)?; + } + _ if kv.p("variables") => { + kv.try_some_mergeable_strings(&mut self.variables, ordered_vec_with_strategy)?; + } _ if kv.p("checksum") => self.checksum.assign(kv)?, "optional" => self.optional = kv.try_some_bool()?, "startup_timeout_secs" => self.startup_timeout_secs = kv.try_some_u32()?, diff --git a/crates/jp_config/src/snapshots/jp_config__tests__app_config_schema_shape.snap b/crates/jp_config/src/snapshots/jp_config__tests__app_config_schema_shape.snap index 2b2aa03b0..eb97f8a06 100644 --- a/crates/jp_config/src/snapshots/jp_config__tests__app_config_schema_shape.snap +++ b/crates/jp_config/src/snapshots/jp_config__tests__app_config_schema_shape.snap @@ -53,7 +53,13 @@ assistant: AssistantConfig exclude?: bool |: null service_tier: "off" | "flex" | "standard" | "priority" | null - stop_words?: [string] + stop_words?: MergeableVec + |: [string] + |: MergedVec + dedup?: "inherit" | "true" | "false" | bool | null + discard_when_merged?: bool + strategy?: "append" | "prepend" | "replace" | null + value?: [string] temperature: float | null top_k: int | null top_p: float | null @@ -91,7 +97,13 @@ assistant: AssistantConfig tag: string | null title: string | null tool_choice?: "auto" | "none" | "required" | string -config_load_paths: [string] +config_load_paths: MergeableVec + |: [string] + |: MergedVec + dedup?: "inherit" | "true" | "false" | bool | null + discard_when_merged?: bool + strategy?: "append" | "prepend" | "replace" | null + value?: [string] conversation: ConversationConfig attachments?: MergeableVec |: @@ -151,7 +163,7 @@ conversation: ConversationConfig exclude?: bool |: null service_tier: "off" | "flex" | "standard" | "priority" | null - stop_words?: [string] + stop_words?: @MergeableVec temperature: float | null top_k: int | null top_p: float | null @@ -204,7 +216,7 @@ conversation: ConversationConfig exclude?: bool |: null service_tier: "off" | "flex" | "standard" | "priority" | null - stop_words?: [string] + stop_words?: @MergeableVec temperature: float | null top_k: int | null top_p: float | null @@ -271,7 +283,13 @@ conversation: ConversationConfig exclude?: bool |: null service_tier: "off" | "flex" | "standard" | "priority" | null - stop_words?: [string] + stop_words?: MergeableVec + |: [string] + |: MergedVec + dedup?: "inherit" | "true" | "false" | bool | null + discard_when_merged?: bool + strategy?: "append" | "prepend" | "replace" | null + value?: [string] temperature: float | null top_k: int | null top_p: float | null @@ -378,7 +396,13 @@ conversation: ConversationConfig exclude?: bool |: null service_tier: "off" | "flex" | "standard" | "priority" | null - stop_words?: [string] + stop_words?: MergeableVec + |: [string] + |: MergedVec + dedup?: "inherit" | "true" | "false" | bool | null + discard_when_merged?: bool + strategy?: "append" | "prepend" | "replace" | null + value?: [string] temperature: float | null top_k: int | null top_p: float | null @@ -603,7 +627,15 @@ conversation: ConversationConfig exclude?: bool | null |: null service_tier?: "off" | "flex" | "standard" | "priority" | null - stop_words?: [string] | null + stop_words?: + |: MergeableVec + |: [string] + |: PartialMergedVec + dedup?: "inherit" | "true" | "false" | bool | null | null + discard_when_merged?: bool | null + strategy?: "append" | "prepend" | "replace" | null + value?: [string] | null + |: null temperature?: float | null top_k?: int | null top_p?: float | null @@ -768,7 +800,13 @@ providers: ProviderConfig mcp: *: McpProviderConfig |: StdioConfig - arguments?: [string] + arguments?: MergeableVec + |: [string] + |: MergedVec + dedup?: "inherit" | "true" | "false" | bool | null + discard_when_merged?: bool + strategy?: "append" | "prepend" | "replace" | null + value?: [string] checksum: |: ChecksumConfig algorithm?: "sha256" | "sha1" @@ -778,7 +816,13 @@ providers: ProviderConfig optional?: bool startup_timeout_secs?: int type: "stdio" - variables?: [string] + variables?: MergeableVec + |: [string] + |: MergedVec + dedup?: "inherit" | "true" | "false" | bool | null + discard_when_merged?: bool + strategy?: "append" | "prepend" | "replace" | null + value?: [string] style: StyleConfig code: CodeConfig color?: bool @@ -820,7 +864,13 @@ style: StyleConfig *: unknown reasoning: @ReasoningConfig | null service_tier: "off" | "flex" | "standard" | "priority" | null - stop_words?: [string] + stop_words?: MergeableVec + |: [string] + |: MergedVec + dedup?: "inherit" | "true" | "false" | bool | null + discard_when_merged?: bool + strategy?: "append" | "prepend" | "replace" | null + value?: [string] temperature: float | null top_k: int | null top_p: float | null