diff --git a/crates/contrib/schematic_macros/src/common/field.rs b/crates/contrib/schematic_macros/src/common/field.rs index e9ac6f60f..ea7751096 100644 --- a/crates/contrib/schematic_macros/src/common/field.rs +++ b/crates/contrib/schematic_macros/src/common/field.rs @@ -159,7 +159,21 @@ 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. + 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 53db0e33f..fca2bedc8 100644 --- a/crates/contrib/schematic_macros/src/config/field.rs +++ b/crates/contrib/schematic_macros/src/config/field.rs @@ -152,22 +152,51 @@ 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 = 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 { 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 { 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_cli/src/config_pipeline_tests.rs b/crates/jp_cli/src/config_pipeline_tests.rs index ed0bdbc58..2adf590e0 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()) @@ -296,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() }) } @@ -304,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/assignment.rs b/crates/jp_config/src/assignment.rs index 973609198..632fc541b 100644 --- a/crates/jp_config/src/assignment.rs +++ b/crates/jp_config/src/assignment.rs @@ -8,11 +8,11 @@ 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}; -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,80 @@ 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. + /// 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, + { + // 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 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(()); + } + + // 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()); + + 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>, + merge: impl Fn(MergeableVec, MergeableVec, &()) -> MergeResult>, + ) -> 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, merge) + } + /// 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..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 @@ -138,6 +110,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 @@ -236,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 3e916eac8..2a653477e 100644 --- a/crates/jp_config/src/delta_tests.rs +++ b/crates/jp_config/src/delta_tests.rs @@ -2,13 +2,22 @@ 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 { 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,47 +32,75 @@ 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() +} + +/// 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`. @@ -92,35 +129,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()]) @@ -129,7 +166,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,17 +182,30 @@ 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, + })) ); } -/// `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"]); @@ -174,14 +226,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" ); } @@ -204,14 +271,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 + }; + + // Equal entries are dropped by the equality check ahead of the delta. + assert!(delta_map(&entry("serve"), entry("serve")).is_empty()); - assert!(delta_map(&prev, next).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/editor.rs b/crates/jp_config/src/editor.rs index 5dffbdb05..04fe58e35 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,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_vec_of_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), } @@ -123,7 +134,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 +142,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 +164,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/internal/merge.rs b/crates/jp_config/src/internal/merge.rs index 1791664c6..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::vec_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/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.rs b/crates/jp_config/src/lib.rs index 0cc4681f3..1dcdb2453 100644 --- a/crates/jp_config/src/lib.rs +++ b/crates/jp_config/src/lib.rs @@ -77,8 +77,9 @@ 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}, + internal::merge::vec_with_strategy, interrupt::{InterruptConfig, PartialInterruptConfig}, loader::{LoaderConfig, PartialLoaderConfig}, partial::partial_opt, @@ -86,7 +87,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 +132,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 +242,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, vec_with_strategy)?; } _ if kv.p("assistant") => self.assistant.assign(kv)?, _ if kv.p("conversation") => self.conversation.assign(kv)?, @@ -264,7 +268,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 +292,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 +356,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(), @@ -845,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/lib_tests.rs b/crates/jp_config/src/lib_tests.rs index 473708add..376f40fa3 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 @@ -886,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(); @@ -919,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(); @@ -931,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/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 4716ea28c..614edb2b8 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,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_vec_of_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())?, } @@ -237,7 +244,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 +287,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 +322,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(); diff --git a/crates/jp_config/src/providers/llm/anthropic.rs b/crates/jp_config/src/providers/llm/anthropic.rs index 139f24fdd..90d73cc6c 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,9 @@ 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, vec_with_strategy)?; + } _ => return missing_key(&kv), } @@ -131,29 +138,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 +172,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/providers/mcp.rs b/crates/jp_config/src/providers/mcp.rs index 046660cf9..2a3290e7b 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,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_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, 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()?, @@ -169,8 +168,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/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..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 @@ -684,7 +716,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 +767,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 @@ -756,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" @@ -766,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 @@ -808,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 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..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 @@ -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, @@ -457,7 +459,9 @@ Ok( true, ), beta_headers: Some( - [], + Vec( + [], + ), ), }, cerebras: PartialCerebrasConfig { 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_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() } 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"]);