diff --git a/crates/contrib/schematic_macros/src/common/field.rs b/crates/contrib/schematic_macros/src/common/field.rs index ebadf22a2..e878148d7 100644 --- a/crates/contrib/schematic_macros/src/common/field.rs +++ b/crates/contrib/schematic_macros/src/common/field.rs @@ -7,7 +7,7 @@ use syn::{Attribute, Expr, ExprPath, Field as NativeField, Type}; use crate::{ common::{FieldValue, PartialAttr, TypeInfo, extract_inner_type, macros::ContainerSerdeArgs}, - utils::{extract_common_attrs, format_case, preserve_str_literal}, + utils::{extract_common_attrs, format_case, parse_default}, }; // #[serde()] @@ -42,7 +42,7 @@ pub struct FieldArgs { pub exclude: bool, // config - #[darling(with = preserve_str_literal, map = "Some")] + #[darling(with = parse_default)] pub default: Option, #[cfg(feature = "env")] pub env: Option, @@ -58,6 +58,7 @@ pub struct FieldArgs { // serde pub alias: Option, + pub deserialize_with: Option, pub flatten: bool, pub rename: Option, pub skip: bool, @@ -82,7 +83,21 @@ pub struct Field<'l> { impl Field<'_> { pub fn from(field: &NativeField) -> Field<'_> { - let args = FieldArgs::from_attributes(&field.attrs).unwrap_or_default(); + // Errors are fatal rather than defaulted: a mistyped or unknown key in + // schematic's own `#[setting]` / `#[schema]` attribute would otherwise + // silently discard every other key on the same field. + // + // `#[serde]` stays lenient. It is serde's namespace, not schematic's, + // and carries keys (`with`, `bound`, `default = "path"`) that + // `FieldSerdeArgs` deliberately does not model. + let args = FieldArgs::from_attributes(&field.attrs).unwrap_or_else(|error| { + let name = field + .ident + .as_ref() + .map_or_else(|| "".to_owned(), ToString::to_string); + + panic!("Invalid `#[setting]` or `#[schema]` attribute on field `{name}`: {error}"); + }); let serde_args = FieldSerdeArgs::from_attributes(&field.attrs).unwrap_or_default(); let partial_via_ty = args.partial_via.as_ref().map(|ep| { @@ -305,6 +320,12 @@ impl Field<'_> { if self.args.skip_deserializing || self.serde_args.skip_deserializing { meta.push(quote! { skip_deserializing }); + } else if let Some(deserialize_with) = &self.args.deserialize_with { + // `deserialize_with` disables serde's implicit "missing + // `Option` field is `None`" handling, so the partial field + // needs an explicit default to stay optional. + meta.push(quote! { default }); + meta.push(quote! { deserialize_with = #deserialize_with }); } } diff --git a/crates/contrib/schematic_macros/src/utils.rs b/crates/contrib/schematic_macros/src/utils.rs index 25dfe74be..86adad8b0 100644 --- a/crates/contrib/schematic_macros/src/utils.rs +++ b/crates/contrib/schematic_macros/src/utils.rs @@ -39,6 +39,20 @@ pub fn preserve_str_literal(meta: &Meta) -> darling::Result { } } +/// Parse a `default` argument, which accepts both `default` and `default = +/// `. +/// +/// The bare form states that the field falls back to its type's `Default` impl, +/// which is what the generated code emits when no default expression is +/// present, so it parses to `None`. +pub fn parse_default(meta: &Meta) -> darling::Result> { + match meta { + Meta::Path(_) => Ok(None), + Meta::List(_) => Err(darling::Error::unsupported_format("list").with_span(meta)), + Meta::NameValue(nv) => Ok(Some(nv.value.clone())), + } +} + pub fn get_meta_path(meta: &Meta) -> &Path { match meta { Meta::Path(path) => path, diff --git a/crates/jp_config/src/assistant.rs b/crates/jp_config/src/assistant.rs index ab0949a82..46e00dc8c 100644 --- a/crates/jp_config/src/assistant.rs +++ b/crates/jp_config/src/assistant.rs @@ -21,7 +21,7 @@ use crate::{ tool_choice::ToolChoice, }, delta::{PartialConfigDelta, delta_opt, delta_opt_partial}, - fill::FillDefaults, + fill::{FillDefaults, fill_opt}, internal::merge::{string_with_strategy, vec_with_strategy}, model::{ModelConfig, PartialModelConfig}, partial::{ToPartial, partial_opt, partial_opts}, @@ -98,6 +98,16 @@ impl AssignKeyValue for PartialAssistantConfig { "" => kv.try_merge_object(self)?, "name" => self.name = kv.try_some_string()?, "system_prompt" => self.system_prompt = kv.try_some_object_or_from_str()?, + // Nested keys (`system_prompt.value`, `.strategy`, `.separator`, + // `.discard_when_merged`, `.dedup`) address the merge metadata, so + // an absent value starts as `Merged` rather than the plain-string + // default, which has nowhere to put them. + _ if kv.p("system_prompt") => self + .system_prompt + .get_or_insert_with(|| { + PartialMergeableString::Merged(PartialMergedString::default()) + }) + .assign(kv)?, _ if kv.p("instructions") => kv.try_vec_of_nested(self.instructions.as_mut())?, _ if kv.p("system_prompt_sections") => { kv.try_vec_of_nested(self.system_prompt_sections.as_mut())?; @@ -139,7 +149,10 @@ impl FillDefaults for PartialAssistantConfig { fn fill_from(self, defaults: Self) -> Self { Self { name: self.name.or(defaults.name), - system_prompt: self.system_prompt.or(defaults.system_prompt), + // `fill_opt` rather than `or`: a metadata-only prompt (from e.g. + // `--cfg assistant.system_prompt.dedup=false`) still needs the + // default's value filled in. + system_prompt: fill_opt(self.system_prompt, defaults.system_prompt), system_prompt_sections: self .system_prompt_sections .fill_from(defaults.system_prompt_sections), @@ -210,6 +223,7 @@ fn default_system_prompt(_: &()) -> TransformResult` 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 new file mode 100644 index 000000000..478ab2aac --- /dev/null +++ b/crates/jp_config/src/internal/merge/plain_vec_tests.rs @@ -0,0 +1,43 @@ +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/string.rs b/crates/jp_config/src/internal/merge/string.rs index 1619f6b3f..71c3383b1 100644 --- a/crates/jp_config/src/internal/merge/string.rs +++ b/crates/jp_config/src/internal/merge/string.rs @@ -12,9 +12,16 @@ pub fn string_with_strategy( next: PartialMergeableString, _context: &(), ) -> MergeResult { + // Resolve the explicit dedup opinion: next's choice wins, then inherit from + // prev. `None` means neither side expressed one. + // + // A discarded prev still contributes dedup when next has no opinion, but + // NOT when next explicitly sets it. + let dedup = dedup_flag(&next).or_else(|| dedup_flag(&prev)); + // If prev is default, replace regardless of strategy. if prev.discard_when_merged() { - return Ok(Some(next)); + return Ok(Some(with_dedup_flag(next, dedup))); } let prev_value = match prev { @@ -32,13 +39,25 @@ pub fn string_with_strategy( } }; + // Skip an append or prepend whose value is already present, unless a config + // explicitly opts out. Applying the same config source twice — through an + // `extends` diamond, or by re-supplying `--cfg` on a conversation that + // already merged it — must not duplicate its contribution. + let dedup_active = dedup.unwrap_or(true); + + // An unstated strategy means `append`, matching `MergedStringStrategy`'s + // default. Only the computation resolves it; the stored `strategy` keeps + // the unstated form so later merges can still express their own. + let resolved_strategy = strategy.unwrap_or_default(); + let sep = separator.as_ref().map_or("", |sep| sep.as_str()); let value = match (prev_value, next_value) { - (_, n) if strategy == Some(MergedStringStrategy::Replace) => n, - (Some(p), Some(n)) if strategy == Some(MergedStringStrategy::Append) => { + (_, n) if resolved_strategy == MergedStringStrategy::Replace => n, + (Some(p), Some(n)) if dedup_active && contains_block(&p, &n, sep) => Some(p), + (Some(p), Some(n)) if resolved_strategy == MergedStringStrategy::Append => { Some(format!("{p}{sep}{n}")) } - (Some(p), Some(n)) if strategy == Some(MergedStringStrategy::Prepend) => { + (Some(p), Some(n)) if resolved_strategy == MergedStringStrategy::Prepend => { Some(format!("{n}{sep}{p}")) } (Some(p), None) => Some(p), @@ -47,17 +66,83 @@ pub fn string_with_strategy( }; Ok(Some(if next_is_replace { - PartialMergeableString::String(value.unwrap_or_default()) + with_dedup_flag( + PartialMergeableString::String(value.unwrap_or_default()), + dedup, + ) } else { PartialMergeableString::Merged(PartialMergedString { value, strategy, separator, discard_when_merged, + dedup, }) })) } +/// The explicit `dedup` opinion carried by a value, if any. +const fn dedup_flag(v: &PartialMergeableString) -> Option { + match v { + PartialMergeableString::String(_) => None, + PartialMergeableString::Merged(m) => m.dedup, + } +} + +/// Attach an explicit `dedup` opinion, wrapping a plain string in `Merged` with +/// a `replace` strategy so the flag survives the next merge. +/// +/// A `None` opinion is left implicit and the value's shape is unchanged. +fn with_dedup_flag(v: PartialMergeableString, dedup: Option) -> PartialMergeableString { + if dedup.is_none() { + return v; + } + + match v { + PartialMergeableString::String(value) => { + PartialMergeableString::Merged(PartialMergedString { + value: Some(value), + strategy: Some(MergedStringStrategy::Replace), + separator: None, + discard_when_merged: None, + dedup, + }) + } + PartialMergeableString::Merged(mut m) => { + m.dedup = dedup; + PartialMergeableString::Merged(m) + } + } +} + +/// Whether `needle` already appears in `haystack` as a whole +/// `separator`-delimited block. +/// +/// A match must start at the beginning of `haystack` or right after a +/// separator, and end at the end of `haystack` or right before one. +/// Anchoring on both sides keeps a value that merely occurs inside a larger +/// block from counting as present. +/// +/// An empty separator leaves no boundaries to anchor on, so only an exact match +/// of the whole string counts. +fn contains_block(haystack: &str, needle: &str, separator: &str) -> bool { + if needle.is_empty() || haystack == needle { + return true; + } + + if separator.is_empty() { + return false; + } + + haystack.match_indices(needle).any(|(index, matched)| { + let end = index + matched.len(); + let starts_block = index == 0 || haystack[..index].ends_with(separator); + let ends_block = end == haystack.len() || haystack[end..].starts_with(separator); + + starts_block && ends_block + }) +} + #[cfg(test)] #[path = "string_tests.rs"] mod tests; diff --git a/crates/jp_config/src/internal/merge/string_tests.rs b/crates/jp_config/src/internal/merge/string_tests.rs index 94b9c64b4..2dfe5201f 100644 --- a/crates/jp_config/src/internal/merge/string_tests.rs +++ b/crates/jp_config/src/internal/merge/string_tests.rs @@ -24,12 +24,14 @@ fn test_string_with_append_strategy() { strategy: Some(MergedStringStrategy::Append), separator: Some(MergedStringSeparator::None), discard_when_merged: None, + dedup: None, }), expected: PartialMergeableString::Merged(PartialMergedString { value: Some("foobar".to_owned()), strategy: Some(MergedStringStrategy::Append), separator: Some(MergedStringSeparator::None), discard_when_merged: None, + dedup: None, }), }, TestCase { @@ -39,12 +41,14 @@ fn test_string_with_append_strategy() { strategy: Some(MergedStringStrategy::Replace), separator: Some(MergedStringSeparator::None), discard_when_merged: None, + dedup: None, }), expected: PartialMergeableString::Merged(PartialMergedString { value: Some("bar".to_owned()), strategy: Some(MergedStringStrategy::Replace), separator: Some(MergedStringSeparator::None), discard_when_merged: None, + dedup: None, }), }, TestCase { @@ -53,6 +57,7 @@ fn test_string_with_append_strategy() { strategy: Some(MergedStringStrategy::Append), separator: Some(MergedStringSeparator::None), discard_when_merged: None, + dedup: None, }), next: PartialMergeableString::String("bar".to_owned()), expected: PartialMergeableString::String("bar".to_owned()), @@ -63,18 +68,21 @@ fn test_string_with_append_strategy() { strategy: Some(MergedStringStrategy::Append), separator: Some(MergedStringSeparator::None), discard_when_merged: None, + dedup: None, }), next: PartialMergeableString::Merged(PartialMergedString { value: Some("bar".to_owned()), strategy: Some(MergedStringStrategy::Replace), separator: Some(MergedStringSeparator::None), discard_when_merged: None, + dedup: None, }), expected: PartialMergeableString::Merged(PartialMergedString { value: Some("bar".to_owned()), strategy: Some(MergedStringStrategy::Replace), separator: Some(MergedStringSeparator::None), discard_when_merged: None, + dedup: None, }), }, TestCase { @@ -83,18 +91,21 @@ fn test_string_with_append_strategy() { strategy: Some(MergedStringStrategy::Append), separator: Some(MergedStringSeparator::None), discard_when_merged: None, + dedup: None, }), next: PartialMergeableString::Merged(PartialMergedString { value: Some("bar".to_owned()), strategy: Some(MergedStringStrategy::Replace), separator: Some(MergedStringSeparator::None), discard_when_merged: None, + dedup: None, }), expected: PartialMergeableString::Merged(PartialMergedString { value: Some("bar".to_owned()), strategy: Some(MergedStringStrategy::Replace), separator: Some(MergedStringSeparator::None), discard_when_merged: None, + dedup: None, }), }, TestCase { @@ -103,18 +114,21 @@ fn test_string_with_append_strategy() { strategy: Some(MergedStringStrategy::Append), separator: Some(MergedStringSeparator::None), discard_when_merged: None, + dedup: None, }), next: PartialMergeableString::Merged(PartialMergedString { value: Some("bar".to_owned()), strategy: Some(MergedStringStrategy::Append), separator: Some(MergedStringSeparator::None), discard_when_merged: None, + dedup: None, }), expected: PartialMergeableString::Merged(PartialMergedString { value: Some("foobar".to_owned()), strategy: Some(MergedStringStrategy::Append), separator: Some(MergedStringSeparator::None), discard_when_merged: None, + dedup: None, }), }, TestCase { @@ -122,6 +136,7 @@ fn test_string_with_append_strategy() { value: Some("foo".to_owned()), strategy: Some(MergedStringStrategy::Append), discard_when_merged: None, + dedup: None, separator: Some(MergedStringSeparator::None), }), next: PartialMergeableString::Merged(PartialMergedString { @@ -129,12 +144,14 @@ fn test_string_with_append_strategy() { strategy: Some(MergedStringStrategy::Replace), separator: Some(MergedStringSeparator::None), discard_when_merged: None, + dedup: None, }), expected: PartialMergeableString::Merged(PartialMergedString { value: Some("bar".to_owned()), strategy: Some(MergedStringStrategy::Replace), separator: Some(MergedStringSeparator::None), discard_when_merged: None, + dedup: None, }), }, TestCase { @@ -144,12 +161,14 @@ fn test_string_with_append_strategy() { strategy: Some(MergedStringStrategy::Append), separator: Some(MergedStringSeparator::Space), discard_when_merged: None, + dedup: None, }), expected: PartialMergeableString::Merged(PartialMergedString { value: Some("foo bar".to_owned()), strategy: Some(MergedStringStrategy::Append), separator: Some(MergedStringSeparator::Space), discard_when_merged: None, + dedup: None, }), }, TestCase { @@ -159,12 +178,14 @@ fn test_string_with_append_strategy() { strategy: Some(MergedStringStrategy::Append), separator: Some(MergedStringSeparator::Line), discard_when_merged: None, + dedup: None, }), expected: PartialMergeableString::Merged(PartialMergedString { value: Some("foo\nbar".to_owned()), strategy: Some(MergedStringStrategy::Append), separator: Some(MergedStringSeparator::Line), discard_when_merged: None, + dedup: None, }), }, TestCase { @@ -174,12 +195,14 @@ fn test_string_with_append_strategy() { strategy: Some(MergedStringStrategy::Append), separator: Some(MergedStringSeparator::Paragraph), discard_when_merged: None, + dedup: None, }), expected: PartialMergeableString::Merged(PartialMergedString { value: Some("foo\n\nbar".to_owned()), strategy: Some(MergedStringStrategy::Append), separator: Some(MergedStringSeparator::Paragraph), discard_when_merged: None, + dedup: None, }), }, ]; @@ -216,12 +239,14 @@ fn test_string_with_prepend_strategy() { strategy: Some(MergedStringStrategy::Prepend), separator: Some(MergedStringSeparator::None), discard_when_merged: None, + dedup: None, }), expected: PartialMergeableString::Merged(PartialMergedString { value: Some("barfoo".to_owned()), strategy: Some(MergedStringStrategy::Prepend), separator: Some(MergedStringSeparator::None), discard_when_merged: None, + dedup: None, }), }, TestCase { @@ -231,12 +256,14 @@ fn test_string_with_prepend_strategy() { strategy: Some(MergedStringStrategy::Replace), separator: Some(MergedStringSeparator::None), discard_when_merged: None, + dedup: None, }), expected: PartialMergeableString::Merged(PartialMergedString { value: Some("bar".to_owned()), strategy: Some(MergedStringStrategy::Replace), separator: Some(MergedStringSeparator::None), discard_when_merged: None, + dedup: None, }), }, TestCase { @@ -245,6 +272,7 @@ fn test_string_with_prepend_strategy() { strategy: Some(MergedStringStrategy::Prepend), separator: Some(MergedStringSeparator::None), discard_when_merged: None, + dedup: None, }), next: PartialMergeableString::String("bar".to_owned()), expected: PartialMergeableString::String("bar".to_owned()), @@ -255,18 +283,21 @@ fn test_string_with_prepend_strategy() { strategy: Some(MergedStringStrategy::Prepend), separator: Some(MergedStringSeparator::None), discard_when_merged: None, + dedup: None, }), next: PartialMergeableString::Merged(PartialMergedString { value: Some("bar".to_owned()), strategy: Some(MergedStringStrategy::Replace), separator: Some(MergedStringSeparator::None), discard_when_merged: None, + dedup: None, }), expected: PartialMergeableString::Merged(PartialMergedString { value: Some("bar".to_owned()), strategy: Some(MergedStringStrategy::Replace), separator: Some(MergedStringSeparator::None), discard_when_merged: None, + dedup: None, }), }, TestCase { @@ -275,18 +306,21 @@ fn test_string_with_prepend_strategy() { strategy: Some(MergedStringStrategy::Prepend), separator: Some(MergedStringSeparator::None), discard_when_merged: None, + dedup: None, }), next: PartialMergeableString::Merged(PartialMergedString { value: Some("bar".to_owned()), strategy: Some(MergedStringStrategy::Replace), separator: Some(MergedStringSeparator::None), discard_when_merged: None, + dedup: None, }), expected: PartialMergeableString::Merged(PartialMergedString { value: Some("bar".to_owned()), strategy: Some(MergedStringStrategy::Replace), separator: Some(MergedStringSeparator::None), discard_when_merged: None, + dedup: None, }), }, TestCase { @@ -295,18 +329,21 @@ fn test_string_with_prepend_strategy() { strategy: Some(MergedStringStrategy::Prepend), separator: Some(MergedStringSeparator::None), discard_when_merged: None, + dedup: None, }), next: PartialMergeableString::Merged(PartialMergedString { value: Some("bar".to_owned()), strategy: Some(MergedStringStrategy::Prepend), separator: Some(MergedStringSeparator::None), discard_when_merged: None, + dedup: None, }), expected: PartialMergeableString::Merged(PartialMergedString { value: Some("barfoo".to_owned()), strategy: Some(MergedStringStrategy::Prepend), separator: Some(MergedStringSeparator::None), discard_when_merged: None, + dedup: None, }), }, TestCase { @@ -315,18 +352,21 @@ fn test_string_with_prepend_strategy() { strategy: Some(MergedStringStrategy::Prepend), separator: Some(MergedStringSeparator::None), discard_when_merged: None, + dedup: None, }), next: PartialMergeableString::Merged(PartialMergedString { value: Some("bar".to_owned()), strategy: Some(MergedStringStrategy::Replace), separator: Some(MergedStringSeparator::None), discard_when_merged: None, + dedup: None, }), expected: PartialMergeableString::Merged(PartialMergedString { value: Some("bar".to_owned()), strategy: Some(MergedStringStrategy::Replace), separator: Some(MergedStringSeparator::None), discard_when_merged: None, + dedup: None, }), }, TestCase { @@ -336,12 +376,14 @@ fn test_string_with_prepend_strategy() { strategy: Some(MergedStringStrategy::Prepend), separator: Some(MergedStringSeparator::Space), discard_when_merged: None, + dedup: None, }), expected: PartialMergeableString::Merged(PartialMergedString { value: Some("bar foo".to_owned()), strategy: Some(MergedStringStrategy::Prepend), separator: Some(MergedStringSeparator::Space), discard_when_merged: None, + dedup: None, }), }, TestCase { @@ -351,12 +393,14 @@ fn test_string_with_prepend_strategy() { strategy: Some(MergedStringStrategy::Prepend), separator: Some(MergedStringSeparator::Line), discard_when_merged: None, + dedup: None, }), expected: PartialMergeableString::Merged(PartialMergedString { value: Some("bar\nfoo".to_owned()), strategy: Some(MergedStringStrategy::Prepend), separator: Some(MergedStringSeparator::Line), discard_when_merged: None, + dedup: None, }), }, TestCase { @@ -366,12 +410,14 @@ fn test_string_with_prepend_strategy() { strategy: Some(MergedStringStrategy::Prepend), separator: Some(MergedStringSeparator::Paragraph), discard_when_merged: None, + dedup: None, }), expected: PartialMergeableString::Merged(PartialMergedString { value: Some("bar\n\nfoo".to_owned()), strategy: Some(MergedStringStrategy::Prepend), separator: Some(MergedStringSeparator::Paragraph), discard_when_merged: None, + dedup: None, }), }, ]; @@ -402,6 +448,7 @@ fn test_default_string() { strategy: None, separator: None, discard_when_merged: Some(true), + dedup: None, }), next: PartialMergeableString::String("bar".to_owned()), expected: PartialMergeableString::String("bar".to_owned()), @@ -412,18 +459,21 @@ fn test_default_string() { strategy: None, separator: None, discard_when_merged: Some(true), + dedup: None, }), next: PartialMergeableString::Merged(PartialMergedString { value: Some("bar".to_owned()), strategy: Some(MergedStringStrategy::Prepend), separator: Some(MergedStringSeparator::None), discard_when_merged: None, + dedup: None, }), expected: PartialMergeableString::Merged(PartialMergedString { value: Some("bar".to_owned()), strategy: Some(MergedStringStrategy::Prepend), separator: Some(MergedStringSeparator::None), discard_when_merged: None, + dedup: None, }), }), ("default stacking", TestCase { @@ -432,18 +482,21 @@ fn test_default_string() { strategy: None, separator: None, discard_when_merged: Some(true), + dedup: None, }), next: PartialMergeableString::Merged(PartialMergedString { value: Some("foo".to_owned()), strategy: Some(MergedStringStrategy::Prepend), separator: Some(MergedStringSeparator::None), discard_when_merged: Some(true), + dedup: None, }), expected: PartialMergeableString::Merged(PartialMergedString { value: Some("foo".to_owned()), strategy: Some(MergedStringStrategy::Prepend), separator: Some(MergedStringSeparator::None), discard_when_merged: Some(true), + dedup: None, }), }), ("next as default", TestCase { @@ -452,18 +505,21 @@ fn test_default_string() { strategy: None, separator: None, discard_when_merged: Some(false), + dedup: None, }), next: PartialMergeableString::Merged(PartialMergedString { value: Some("foo".to_owned()), strategy: Some(MergedStringStrategy::Prepend), separator: Some(MergedStringSeparator::None), discard_when_merged: Some(true), + dedup: None, }), expected: PartialMergeableString::Merged(PartialMergedString { value: Some("foobar".to_owned()), strategy: Some(MergedStringStrategy::Prepend), separator: Some(MergedStringSeparator::None), discard_when_merged: Some(true), + dedup: None, }), }), ]; @@ -505,6 +561,7 @@ fn test_finalized_round_trip_does_not_double_append() { strategy: Some(MergedStringStrategy::Append), separator: Some(MergedStringSeparator::Space), discard_when_merged: None, + dedup: None, }); // Step 2: Simulate a finalized config that was previously built from @@ -515,6 +572,7 @@ fn test_finalized_round_trip_does_not_double_append() { strategy: None, separator: None, discard_when_merged: Some(true), + dedup: None, }); let merged = string_with_strategy(default_partial, config_file_partial.clone(), &()) @@ -558,6 +616,7 @@ fn test_finalized_round_trip_does_not_double_prepend() { strategy: Some(MergedStringStrategy::Prepend), separator: Some(MergedStringSeparator::Space), discard_when_merged: None, + dedup: None, }); let default_partial = PartialMergeableString::Merged(PartialMergedString { @@ -565,6 +624,7 @@ fn test_finalized_round_trip_does_not_double_prepend() { strategy: None, separator: None, discard_when_merged: Some(true), + dedup: None, }); let merged = string_with_strategy(default_partial, config_file_partial.clone(), &()) @@ -586,3 +646,308 @@ fn test_finalized_round_trip_does_not_double_prepend() { "finalized config round-tripped via to_partial() should not re-apply the prepend strategy" ); } + +/// Build an appending partial with a paragraph separator. +fn appending(value: &str, dedup: Option) -> PartialMergeableString { + PartialMergeableString::Merged(PartialMergedString { + value: Some(value.to_owned()), + strategy: Some(MergedStringStrategy::Append), + separator: Some(MergedStringSeparator::Paragraph), + discard_when_merged: None, + dedup, + }) +} + +#[test] +fn test_append_skips_value_already_present_as_block() { + // A persona file appends a knowledge block and its own prompt. Supplying + // the same source again — a second `--cfg` on an existing conversation, or + // an `extends` diamond — must leave the accumulated value untouched. + let accumulated = PartialMergeableString::String("base\n\nknowledge\n\npersona".to_owned()); + + let result = string_with_strategy(accumulated, appending("knowledge", None), &()) + .unwrap() + .unwrap(); + assert_eq!(result.as_ref(), "base\n\nknowledge\n\npersona"); + + let result = string_with_strategy(result, appending("persona", None), &()) + .unwrap() + .unwrap(); + assert_eq!(result.as_ref(), "base\n\nknowledge\n\npersona"); +} + +#[test] +fn test_append_matches_first_and_last_block() { + let result = string_with_strategy( + PartialMergeableString::String("first\n\nlast".to_owned()), + appending("first", None), + &(), + ) + .unwrap() + .unwrap(); + assert_eq!(result.as_ref(), "first\n\nlast"); + + let result = string_with_strategy(result, appending("last", None), &()) + .unwrap() + .unwrap(); + assert_eq!(result.as_ref(), "first\n\nlast"); +} + +#[test] +fn test_append_ignores_match_inside_a_block() { + // "brief" occurs inside a block but is not a block of its own, so it is a + // genuinely new contribution and must be appended. + let result = string_with_strategy( + PartialMergeableString::String("Be brief and clear.".to_owned()), + appending("brief", None), + &(), + ) + .unwrap() + .unwrap(); + + assert_eq!(result.as_ref(), "Be brief and clear.\n\nbrief"); +} + +#[test] +fn test_append_without_separator_only_skips_exact_match() { + // With no separator there are no block boundaries to anchor a match on, so + // only a whole-string match counts as already present. + let no_separator = |value: &str| { + PartialMergeableString::Merged(PartialMergedString { + value: Some(value.to_owned()), + strategy: Some(MergedStringStrategy::Append), + separator: Some(MergedStringSeparator::None), + discard_when_merged: None, + dedup: None, + }) + }; + + let result = string_with_strategy( + PartialMergeableString::String("foobar".to_owned()), + no_separator("bar"), + &(), + ) + .unwrap() + .unwrap(); + assert_eq!(result.as_ref(), "foobarbar"); + + let result = string_with_strategy( + PartialMergeableString::String("foo".to_owned()), + no_separator("foo"), + &(), + ) + .unwrap() + .unwrap(); + assert_eq!(result.as_ref(), "foo"); +} + +#[test] +fn test_append_duplicates_when_dedup_disabled() { + let result = string_with_strategy( + PartialMergeableString::String("knowledge".to_owned()), + appending("knowledge", Some(false)), + &(), + ) + .unwrap() + .unwrap(); + assert_eq!(result.as_ref(), "knowledge\n\nknowledge"); + + // The opt-out is sticky: a later append with no opinion also duplicates. + let result = string_with_strategy(result, appending("knowledge", None), &()) + .unwrap() + .unwrap(); + assert_eq!(result.as_ref(), "knowledge\n\nknowledge\n\nknowledge"); +} + +#[test] +fn test_dedup_opt_out_survives_a_plain_string_replacement() { + // A plain string states a value, not an opinion on dedup. It must not reset + // an explicit opt-out set earlier in the chain, or the next append silently + // deduplicates under the default. + let opted_out = PartialMergeableString::Merged(PartialMergedString { + value: Some("old".to_owned()), + strategy: None, + separator: None, + discard_when_merged: None, + dedup: Some(false), + }); + + let replaced = string_with_strategy( + opted_out, + PartialMergeableString::String("new".to_owned()), + &(), + ) + .unwrap() + .unwrap(); + assert_eq!(replaced.as_ref(), "new"); + + let result = string_with_strategy(replaced, appending("new", None), &()) + .unwrap() + .unwrap(); + + assert_eq!(result.as_ref(), "new\n\nnew"); +} + +#[test] +fn test_dedup_opt_out_survives_a_discarded_default() { + // The built-in default is `discard_when_merged`, which returns `next` + // wholesale. An opt-out on either side has to come through that path. + let default = PartialMergeableString::Merged(PartialMergedString { + value: Some("You are a helpful assistant.".to_owned()), + strategy: None, + separator: None, + discard_when_merged: Some(true), + dedup: Some(false), + }); + + let replaced = string_with_strategy( + default, + PartialMergeableString::String("persona".to_owned()), + &(), + ) + .unwrap() + .unwrap(); + assert_eq!(replaced.as_ref(), "persona"); + + let result = string_with_strategy(replaced, appending("persona", None), &()) + .unwrap() + .unwrap(); + + assert_eq!(result.as_ref(), "persona\n\npersona"); +} + +#[test] +fn test_plain_replacement_without_an_opinion_stays_a_plain_string() { + // Carrying the flag needs the `Merged` wrapper, but only when there is an + // opinion to carry: an ordinary replacement keeps its shape. + let result = string_with_strategy( + PartialMergeableString::String("old".to_owned()), + PartialMergeableString::String("new".to_owned()), + &(), + ) + .unwrap() + .unwrap(); + + assert_eq!( + result, + PartialMergeableString::String("new".to_owned()), + "a replacement with no dedup opinion should not gain a Merged wrapper" + ); +} + +#[test] +fn test_unstated_strategy_appends() { + // `MergedStringStrategy` defaults to `append`, so a config that sets a + // value without naming a strategy appends it. + let no_strategy = PartialMergeableString::Merged(PartialMergedString { + value: Some("bar".to_owned()), + strategy: None, + separator: Some(MergedStringSeparator::Space), + discard_when_merged: None, + dedup: None, + }); + + let result = string_with_strategy( + PartialMergeableString::String("foo".to_owned()), + no_strategy, + &(), + ) + .unwrap() + .unwrap(); + + assert_eq!(result.as_ref(), "foo bar"); +} + +#[test] +fn test_dedup_accepts_inherit() { + let merged: PartialMergedString = + serde_json::from_str(r#"{"value":"foo","dedup":"inherit"}"#).unwrap(); + assert_eq!(merged.dedup, None); + + let merged: PartialMergedString = + serde_json::from_str(r#"{"value":"foo","dedup":false}"#).unwrap(); + assert_eq!(merged.dedup, Some(false)); + + let merged: PartialMergedString = serde_json::from_str(r#"{"value":"foo"}"#).unwrap(); + assert_eq!(merged.dedup, None); +} + +#[test] +fn test_dedup_assignment_accepts_every_documented_value() { + use crate::assignment::{AssignKeyValue as _, KvAssignment}; + + // The leaf parser accepts every value the field documents, matching what + // `deserialize_dedup` accepts from a config file. `true` and `false` start + // from the opposite opinion so the assertion cannot pass on a no-op; + // `inherit` states no opinion and must leave the seed standing. + for (input, seed, want) in [ + ("true", Some(false), Some(true)), + ("false", Some(true), Some(false)), + ("inherit", Some(false), Some(false)), + ("inherit", None, None), + ] { + let mut partial = PartialMergedString { + dedup: seed, + ..Default::default() + }; + let kv = KvAssignment::try_from_cli("dedup", input).unwrap(); + partial.assign(kv).unwrap(); + + assert_eq!(partial.dedup, want, "input: {input}"); + } + + let mut partial = PartialMergedString::default(); + let kv = KvAssignment::try_from_cli("dedup", "maybe").unwrap(); + assert!( + partial.assign(kv).is_err(), + "an undocumented value should be rejected" + ); +} + +#[test] +fn test_inherited_dedup_opt_out_still_duplicates_on_append() { + use crate::assignment::{AssignKeyValue as _, KvAssignment}; + + // `dedup=inherit` leaves a lower layer's opt-out in force, which is only + // observable through the merge it governs: the append below must duplicate. + let mut opted_out = PartialMergedString { + dedup: Some(false), + ..Default::default() + }; + let kv = KvAssignment::try_from_cli("dedup", "inherit").unwrap(); + opted_out.assign(kv).unwrap(); + + let result = string_with_strategy( + PartialMergeableString::Merged(PartialMergedString { + value: Some("persona".to_owned()), + ..opted_out + }), + appending("persona", None), + &(), + ) + .unwrap() + .unwrap(); + + assert_eq!(result.as_ref(), "persona\n\npersona"); +} + +#[test] +fn test_prepend_skips_value_already_present_as_block() { + let prepending = PartialMergeableString::Merged(PartialMergedString { + value: Some("persona".to_owned()), + strategy: Some(MergedStringStrategy::Prepend), + separator: Some(MergedStringSeparator::Paragraph), + discard_when_merged: None, + dedup: None, + }); + + let result = string_with_strategy( + PartialMergeableString::String("persona\n\nbase".to_owned()), + prepending, + &(), + ) + .unwrap() + .unwrap(); + + assert_eq!(result.as_ref(), "persona\n\nbase"); +} diff --git a/crates/jp_config/src/internal/merge/vec.rs b/crates/jp_config/src/internal/merge/vec.rs index 4452549e7..ddb7bc9ab 100644 --- a/crates/jp_config/src/internal/merge/vec.rs +++ b/crates/jp_config/src/internal/merge/vec.rs @@ -16,24 +16,23 @@ pub fn vec_with_strategy( where T: Clone + PartialEq + Serialize + DeserializeOwned + Schematic, { - let prev_dedup = dedup_flag(&prev); - let next_dedup = dedup_flag(&next); - - // Resolve dedup: next's explicit choice wins, then inherit from prev. + // Resolve the explicit dedup opinion: next's choice wins, then inherit from + // prev. `None` means neither side expressed one. // // A discarded prev still contributes dedup when next has no opinion (None / // "inherit"), but NOT when next explicitly sets it. - let dedup = next_dedup.or(prev_dedup).unwrap_or(false); + let dedup = dedup_flag(&next).or_else(|| dedup_flag(&prev)); - // If prev is default, replace regardless of strategy. + // If prev is default, replace regardless of strategy. Nothing is combined, + // so only an explicit opt-in deduplicates — see the note on `dedup_active` + // below. if prev.discard_when_merged() { - if dedup { - let mut next = ensure_dedup(next); + let mut next = next; + if dedup == Some(true) { dedup_in_place(&mut next); - return Ok(Some(next)); } - return Ok(Some(next)); + return Ok(Some(with_dedup_flag(next, dedup))); } let mut prev_value = match prev { @@ -47,6 +46,21 @@ where MergeableVec::Merged(v) => (v.strategy, v.value, v.discard_when_merged), }; + // Deduplicate unless a config explicitly opts out. Applying the same config + // source twice — through an `extends` diamond, or by re-supplying `--cfg` + // on a conversation that already merged it — must not duplicate entries. + // + // Only merges that actually combine two sources deduplicate by default. A + // `replace` contributes no second source, so duplicates in it are the + // author's own data: free-form `JsonValue` arrays (tool options, template + // values) are merged through here with an implicit `replace`, and their + // contents must survive untouched. An explicit `dedup = true` still applies. + let dedup_active = if matches!(strategy, Some(MergedVecStrategy::Replace)) { + dedup == Some(true) + } else { + dedup.unwrap_or(true) + }; + let mut value = match strategy { None | Some(MergedVecStrategy::Append) => { prev_value.append(&mut next_value); @@ -59,19 +73,17 @@ where Some(MergedVecStrategy::Replace) => next_value, }; - if dedup { + if dedup_active { dedup_in_place(&mut value); } - // Carry forward as Option: Some(true) when active, None otherwise. - let resolved_dedup = if dedup { Some(true) } else { None }; - - // When dedup is active, always use Merged to carry the flag forward. - Ok(Some(if next_is_merged || dedup { + // An explicit opinion needs the `Merged` wrapper to survive the next merge. + // Without one, the shape is left alone. + Ok(Some(if next_is_merged || dedup.is_some() { MergeableVec::Merged(MergedVec { value, strategy, - dedup: resolved_dedup, + dedup, discard_when_merged, }) } else { @@ -87,17 +99,24 @@ const fn dedup_flag(v: &MergeableVec) -> Option { } } -/// Ensure the dedup flag is set on a `MergeableVec`. -fn ensure_dedup(v: MergeableVec) -> MergeableVec { +/// Attach an explicit `dedup` opinion to a `MergeableVec`, wrapping a plain +/// `Vec` in `Merged` so the flag survives the next merge. +/// +/// A `None` opinion is left implicit and the value's shape is unchanged. +fn with_dedup_flag(v: MergeableVec, dedup: Option) -> MergeableVec { + if dedup.is_none() { + return v; + } + match v { MergeableVec::Vec(value) => MergeableVec::Merged(MergedVec { value, strategy: None, - dedup: Some(true), + dedup, discard_when_merged: false, }), MergeableVec::Merged(mut m) => { - m.dedup = Some(true); + m.dedup = dedup; MergeableVec::Merged(m) } } diff --git a/crates/jp_config/src/internal/merge/vec_tests.rs b/crates/jp_config/src/internal/merge/vec_tests.rs index 8995af14f..fad75e9a5 100644 --- a/crates/jp_config/src/internal/merge/vec_tests.rs +++ b/crates/jp_config/src/internal/merge/vec_tests.rs @@ -301,12 +301,100 @@ fn test_dedup_sticky_across_non_discarded_merges() { } #[test] -fn test_no_dedup_without_flag() { +fn test_dedup_without_flag() { let prev = MergeableVec::Vec(vec![1, 2]); let next = MergeableVec::Vec(vec![2, 3]); + let result = vec_with_strategy(prev, next, &()).unwrap().unwrap(); + assert_eq!(&*result, &[1, 2, 3]); + + // No explicit opinion was stated, so the shape stays a plain `Vec`. + assert!(matches!(result, MergeableVec::Vec(_))); +} + +#[test] +fn test_no_dedup_when_explicitly_disabled() { + let prev = MergeableVec::Merged(MergedVec { + value: vec![1, 2], + strategy: None, + dedup: Some(false), + discard_when_merged: false, + }); + let next = MergeableVec::Vec(vec![2, 3]); + let result = vec_with_strategy(prev, next, &()).unwrap().unwrap(); assert_eq!(&*result, &[1, 2, 2, 3]); + + // The opt-out is sticky: a third merge without an opinion keeps duplicates. + let more = MergeableVec::Vec(vec![3, 4]); + let result = vec_with_strategy(result, more, &()).unwrap().unwrap(); + assert_eq!(&*result, &[1, 2, 2, 3, 3, 4]); +} + +#[test] +fn test_replace_preserves_duplicates_in_the_replacement() { + // A replacement combines nothing, so duplicates in it are the author's own + // data. `JsonValue` merges every plain array through here with an implicit + // `replace`, so free-form tool options and template values must survive + // untouched. + let prev = MergeableVec::Vec(vec![0]); + let next = MergeableVec::Merged(MergedVec { + value: vec![1, 1], + strategy: Some(MergedVecStrategy::Replace), + dedup: None, + discard_when_merged: false, + }); + + let result = vec_with_strategy(prev, next, &()).unwrap().unwrap(); + assert_eq!(&*result, &[1, 1]); +} + +#[test] +fn test_replace_deduplicates_when_explicitly_enabled() { + let prev = MergeableVec::Vec(vec![0]); + let next = MergeableVec::Merged(MergedVec { + value: vec![1, 1], + strategy: Some(MergedVecStrategy::Replace), + dedup: Some(true), + discard_when_merged: false, + }); + + let result = vec_with_strategy(prev, next, &()).unwrap().unwrap(); + assert_eq!(&*result, &[1]); +} + +#[test] +fn test_discarded_default_preserves_duplicates_in_the_replacement() { + // Same reasoning as `replace`: the discarded default contributes nothing to + // combine with. + let default = MergeableVec::Merged(MergedVec { + value: vec![], + strategy: None, + dedup: None, + discard_when_merged: true, + }); + let config = MergeableVec::Vec(vec![1, 1, 2]); + + let result = vec_with_strategy(default, config, &()).unwrap().unwrap(); + assert_eq!(&*result, &[1, 1, 2]); +} + +#[test] +fn test_dedup_collapses_repeated_source() { + // The same source applied twice (an `extends` diamond, or `--cfg` supplied + // again on a conversation that already merged it) contributes its entries + // once. + let source = || MergeableVec::Vec(vec![10, 20]); + + let once = vec_with_strategy(MergeableVec::Vec(vec![1]), source(), &()) + .unwrap() + .unwrap(); + let twice = vec_with_strategy(once.clone(), source(), &()) + .unwrap() + .unwrap(); + + assert_eq!(&*once, &[1, 10, 20]); + assert_eq!(&*twice, &[1, 10, 20]); } #[test] diff --git a/crates/jp_config/src/lib.rs b/crates/jp_config/src/lib.rs index cbb489e11..5698c4fc4 100644 --- a/crates/jp_config/src/lib.rs +++ b/crates/jp_config/src/lib.rs @@ -97,7 +97,6 @@ type BoxedError = Box; #[config(rename_all = "snake_case")] pub struct AppConfig { /// Inherit from a local ancestor or global configuration file. - #[setting(optional)] pub inherit: bool, /// Directories to search for additional configuration files. @@ -109,7 +108,7 @@ pub struct AppConfig { /// /// For example, to load `.jp/agents/dev.toml`, add `.jp/agents` to this /// list and run `jp query --cfg dev`. - #[setting(optional, merge = schematic::merge::append_vec, transform = util::vec_dedup)] + #[setting(merge = internal::merge::append_vec_dedup)] pub config_load_paths: Vec, /// Extends the configuration from the given files. diff --git a/crates/jp_config/src/lib_tests.rs b/crates/jp_config/src/lib_tests.rs index 8a4abccbd..c5d331b11 100644 --- a/crates/jp_config/src/lib_tests.rs +++ b/crates/jp_config/src/lib_tests.rs @@ -96,6 +96,134 @@ fn test_partial_app_config_assign() { ); } +#[test] +fn config_load_paths_append_across_layers() { + // Each layer adds its search directories to the accumulated list instead of + // replacing it, and a directory named by two layers is kept once. Order + // 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()]); + + let mut overlay = PartialAppConfig::empty(); + overlay.config_load_paths = Some(vec![".jp/shared".into(), ".jp/workspace".into()]); + + base.merge(&(), overlay).unwrap(); + + let want: Vec = vec![ + ".jp/global".into(), + ".jp/shared".into(), + ".jp/workspace".into(), + ]; + assert_eq!(base.config_load_paths, Some(want)); +} + +#[test] +fn assign_routes_nested_system_prompt_keys() { + use crate::types::string::{MergedStringStrategy, PartialMergeableString, PartialMergedString}; + + // `--cfg assistant.system_prompt.dedup=false` addresses the merge metadata, + // so it has to reach `PartialMergedString` rather than stopping at + // `PartialAssistantConfig` with an unknown key. + let mut p = PartialAppConfig::default(); + + let kv = KvAssignment::try_from_cli("assistant.system_prompt.dedup", "false").unwrap(); + p.assign(kv).unwrap(); + + let kv = KvAssignment::try_from_cli("assistant.system_prompt.strategy", "prepend").unwrap(); + p.assign(kv).unwrap(); + + assert_eq!( + p.assistant.system_prompt, + Some(PartialMergeableString::Merged(PartialMergedString { + value: None, + strategy: Some(MergedStringStrategy::Prepend), + separator: None, + discard_when_merged: None, + dedup: Some(false), + })) + ); + + // `inherit` states no opinion, leaving the opt-out set above in force. + let kv = KvAssignment::try_from_cli("assistant.system_prompt.dedup", "inherit").unwrap(); + p.assign(kv).unwrap(); + + assert_eq!( + p.assistant.system_prompt, + Some(PartialMergeableString::Merged(PartialMergedString { + value: None, + strategy: Some(MergedStringStrategy::Prepend), + separator: None, + discard_when_merged: None, + dedup: Some(false), + })) + ); + + // `system_prompt_sections` shares the `system_prompt` prefix but is a + // different field, and must not be captured by the nested route. + let kv = KvAssignment::try_from_cli("assistant.system_prompt_sections:", r#"[{"tag":"foo"}]"#) + .unwrap(); + p.assign(kv).unwrap(); + + assert_eq!( + p.assistant.system_prompt_sections.first().unwrap().tag, + Some("foo".to_owned()) + ); +} + +#[test] +fn metadata_only_system_prompt_keeps_the_default_prompt() { + // A metadata-only override states no value, so the built-in default has to + // survive gap-filling. Without it, `--cfg assistant.system_prompt.dedup=false` + // resolves the prompt to an empty string. + let mut p = PartialAppConfig::new_test(); + + let kv = KvAssignment::try_from_cli("assistant.system_prompt.dedup", "false").unwrap(); + p.assign(kv).unwrap(); + + let config = AppConfig::from_partial_with_defaults(p).unwrap(); + + assert_eq!( + config.assistant.system_prompt.as_deref(), + Some("You are a helpful assistant.") + ); +} + +#[test] +fn scalar_system_prompt_accepts_nested_metadata() { + use crate::types::string::{MergedStringStrategy, PartialMergeableString, PartialMergedString}; + + // A lower layer supplying the common scalar form must not block a dotted + // override. The scalar is promoted to `Merged` with `replace` pinned, which + // is what the plain form means. + let mut p = PartialAppConfig::new_test(); + + let kv = KvAssignment::try_from_cli("assistant.system_prompt", "base").unwrap(); + p.assign(kv).unwrap(); + assert_eq!( + p.assistant.system_prompt, + Some(PartialMergeableString::String("base".to_owned())) + ); + + let kv = KvAssignment::try_from_cli("assistant.system_prompt.dedup", "false").unwrap(); + p.assign(kv).unwrap(); + + assert_eq!( + p.assistant.system_prompt, + Some(PartialMergeableString::Merged(PartialMergedString { + value: Some("base".to_owned()), + strategy: Some(MergedStringStrategy::Replace), + separator: None, + discard_when_merged: None, + dedup: Some(false), + })) + ); + + // The promotion preserves the scalar's meaning end to end. + let config = AppConfig::from_partial_with_defaults(p).unwrap(); + assert_eq!(config.assistant.system_prompt.as_deref(), Some("base")); +} + #[test] fn resolve_model_aliases_resolves_assistant_model() { use crate::model::id::{ diff --git a/crates/jp_config/src/model.rs b/crates/jp_config/src/model.rs index 164aab8d2..902b2d45d 100644 --- a/crates/jp_config/src/model.rs +++ b/crates/jp_config/src/model.rs @@ -11,7 +11,7 @@ use crate::{ fill::FillDefaults, model::{ id::{ModelIdOrAliasConfig, PartialModelIdOrAliasConfig}, - parameters::{ParametersConfig, PartialParametersConfig}, + parameters::{ParametersConfig, PartialParametersConfig, deserialize_collecting_other}, }, partial::ToPartial, }; @@ -31,7 +31,9 @@ pub struct ModelConfig { /// The model parameters. /// /// Configuration for model parameters such as temperature, max tokens, etc. - #[setting(nested)] + /// Parameters JP does not model are collected into `parameters.other` and + /// forwarded to the provider as written. + #[setting(nested, deserialize_with = "deserialize_collecting_other")] pub parameters: ParametersConfig, } diff --git a/crates/jp_config/src/model/parameters.rs b/crates/jp_config/src/model/parameters.rs index e18ffd908..6f8554872 100644 --- a/crates/jp_config/src/model/parameters.rs +++ b/crates/jp_config/src/model/parameters.rs @@ -4,7 +4,7 @@ use std::{fmt, str::FromStr}; use indexmap::IndexMap; use schematic::{Config, ConfigEnum}; -use serde::{Deserialize, Serialize}; +use serde::{Deserialize, Deserializer, Serialize, de::Error as DeError}; use crate::{ BoxedError, @@ -16,8 +16,11 @@ use crate::{ }; /// Assistant-specific configuration. +/// +/// Parameters JP does not model are collected into [`Self::other`], so a +/// provider-specific key can be written directly in the parameter block. #[derive(Debug, Clone, PartialEq, Config)] -#[config(default, rename_all = "snake_case", allow_unknown_fields)] +#[config(default, rename_all = "snake_case")] pub struct ParametersConfig { /// Maximum number of tokens to generate. /// @@ -77,14 +80,98 @@ 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, skip_serializing_if = "Vec::is_empty")] + #[setting(default, merge = schematic::merge::append_vec)] pub stop_words: Vec, /// Other non-typed parameters that some models might support. - #[setting(default, flatten, merge = schematic::merge::merge_iter, skip_serializing_if = "IndexMap::is_empty")] + /// + /// Any key in the parameter block that JP does not recognize lands here and + /// is forwarded to the provider as written: + /// + /// ```toml + /// [assistant.model.parameters] + /// presence_penalty = 0.5 + /// ``` + /// + /// The equivalent explicit form is also accepted: + /// + /// ```toml + /// [assistant.model.parameters.other] + /// presence_penalty = 0.5 + /// ``` + #[setting(default, merge = schematic::merge::merge_iter)] pub other: IndexMap, } +/// Every key [`ParametersConfig`] models. +/// Anything else is a provider parameter and is collected into `other`. +/// +/// Kept in sync with the struct by `known_keys_match_the_schema`. +pub(crate) const KNOWN_KEYS: &[&str] = &[ + "max_tokens", + "reasoning", + "temperature", + "top_p", + "top_k", + "stop_words", + "other", +]; + +/// Deserialize a parameter block, collecting unrecognized keys into `other`. +/// +/// Unrecognized keys are provider parameters JP does not model, so discarding +/// them (what serde does with an unknown field on a lenient container) silently +/// drops user intent. +/// An explicit `other` table is also accepted and merges with the collected +/// keys, the explicit entries winning. +/// +/// Applied through `#[setting(deserialize_with = ...)]` on the field holding +/// this config rather than as a `Deserialize` impl, so the generated +/// field-by-field deserializer still does the real work. +/// +/// # Errors +/// +/// Returns an error if the block is not a map, if `other` is present but is not +/// a map, or if any modelled field fails to deserialize. +pub(crate) fn deserialize_collecting_other<'de, D>( + deserializer: D, +) -> Result +where + D: Deserializer<'de>, +{ + let mut map = serde_json::Map::::deserialize(deserializer)?; + + let mut other = IndexMap::new(); + map.retain(|key, value| { + if KNOWN_KEYS.contains(&key.as_str()) { + return true; + } + + other.insert(key.clone(), JsonValue(value.clone())); + false + }); + + // Merged after the collected keys so an explicit entry wins a collision. + let explicit = map.remove("other"); + let has_explicit = explicit.is_some(); + if let Some(explicit) = explicit { + let explicit: IndexMap = + serde_json::from_value(explicit).map_err(DeError::custom)?; + other.extend(explicit); + } + + let mut partial: PartialParametersConfig = + serde_json::from_value(serde_json::Value::Object(map)).map_err(DeError::custom)?; + + // An explicit `other` is kept even when empty, so a serialize/deserialize + // round-trip of a config carrying `other = {}` is lossless. + if has_explicit || !other.is_empty() { + partial.other = Some(other); + } + + Ok(partial) +} + impl AssignKeyValue for PartialParametersConfig { fn assign(&mut self, mut kv: KvAssignment) -> AssignResult { match kv.key_string().as_str() { diff --git a/crates/jp_config/src/model/parameters_tests.rs b/crates/jp_config/src/model/parameters_tests.rs index 4b5e920a9..ea9e03a1e 100644 --- a/crates/jp_config/src/model/parameters_tests.rs +++ b/crates/jp_config/src/model/parameters_tests.rs @@ -23,6 +23,156 @@ fn assign_unknown_nested_key_delegates_to_other() { assert_eq!(other["custom"], JsonValue(json!({"depth": "3"}))); } +#[test] +fn known_keys_match_the_schema() { + use schematic::{SchemaBuilder, SchemaType, Schematic as _}; + + // `deserialize_collecting_other` splits the parameter block using this + // list. A field added to the struct but missed here would be rerouted into + // `other` and forwarded to the provider as a raw parameter instead. + let schema = ParametersConfig::build_schema(SchemaBuilder::default()); + let SchemaType::Struct(struct_type) = &schema.ty else { + panic!("expected a struct schema"); + }; + + let mut fields: Vec<&str> = struct_type.fields.keys().map(String::as_str).collect(); + let mut known = KNOWN_KEYS.to_vec(); + fields.sort_unstable(); + known.sort_unstable(); + + assert_eq!(fields, known); +} + +/// Deserialize a `[parameters]` block through the production path: the +/// collector is wired up on `ModelConfig::parameters`, not on the parameter +/// config itself. +fn parameters_from_toml(block: &str) -> PartialParametersConfig { + let toml = format!("[parameters]\n{block}"); + toml::from_str::(&toml) + .unwrap() + .parameters +} + +#[test] +fn deserialize_collects_unknown_keys_into_other() { + // A provider parameter JP doesn't model, written directly in the parameter + // block. Discarding it silently drops user intent. + let p = parameters_from_toml(indoc::indoc!( + r#" + max_tokens = 100 + presence_penalty = 0.5 + logit_bias = { "50256" = -100 } + "# + )); + + assert_eq!(p.max_tokens, Some(100)); + + let other = p.other.as_ref().unwrap(); + assert_eq!(other["presence_penalty"], JsonValue(json!(0.5))); + assert_eq!(other["logit_bias"], JsonValue(json!({"50256": -100}))); + assert_eq!(other.len(), 2, "known keys must not leak into `other`"); +} + +#[test] +fn deserialize_accepts_an_explicit_other_table() { + // The nested form is what every stored conversation config and existing + // user file writes, so it has to keep working. + let p = parameters_from_toml(indoc::indoc!( + r" + temperature = 0.7 + + [parameters.other] + presence_penalty = 0.5 + " + )); + + assert_eq!(p.temperature, Some(0.7)); + assert_eq!( + p.other.as_ref().unwrap()["presence_penalty"], + JsonValue(json!(0.5)) + ); +} + +#[test] +fn deserialize_prefers_the_explicit_other_entry_on_collision() { + let p = parameters_from_toml(indoc::indoc!( + r" + presence_penalty = 0.1 + + [parameters.other] + presence_penalty = 0.9 + " + )); + + assert_eq!( + p.other.as_ref().unwrap()["presence_penalty"], + JsonValue(json!(0.9)) + ); +} + +#[test] +fn deserialize_leaves_other_unset_when_every_key_is_known() { + let p = parameters_from_toml("top_k = 40"); + + assert_eq!(p.top_k, Some(40)); + assert_eq!(p.other, None); +} + +#[test] +fn deserialize_preserves_the_untagged_reasoning_field() { + // `reasoning` deserializes from a bare string or a table, and the collector + // routes every known field through a `serde_json::Value` intermediate, so + // the untagged forms have to survive that trip. + let p = parameters_from_toml(r#"reasoning = "off""#); + assert_eq!(p.reasoning, Some(PartialReasoningConfig::Off)); + + let p = parameters_from_toml(indoc::indoc!( + r#" + [parameters.reasoning] + effort = "low" + "# + )); + assert_eq!( + p.reasoning, + Some(PartialReasoningConfig::Custom( + PartialCustomReasoningConfig { + effort: Some(ReasoningEffort::Low), + exclude: None, + } + )) + ); +} + +#[test] +fn deserialize_keeps_an_explicit_empty_other() { + // Serialization emits `other = {}` for a present-but-empty map, so dropping + // it here would make a stored config lossy on round-trip. + let p = parameters_from_toml("other = {}"); + + assert_eq!(p.other, Some(IndexMap::new())); +} + +#[test] +fn stop_words_append_across_layers() { + use schematic::PartialConfig as _; + + let mut base = PartialParametersConfig { + stop_words: Some(vec!["STOP".to_owned()]), + ..Default::default() + }; + let overlay = PartialParametersConfig { + stop_words: Some(vec!["HALT".to_owned()]), + ..Default::default() + }; + + base.merge(&(), overlay).unwrap(); + + assert_eq!( + base.stop_words, + Some(vec!["STOP".to_owned(), "HALT".to_owned()]) + ); +} + #[test] fn assign_known_keys_not_routed_to_other() { let mut p = PartialParametersConfig::default(); diff --git a/crates/jp_config/src/providers/llm/anthropic.rs b/crates/jp_config/src/providers/llm/anthropic.rs index b6216aaaf..fd2f1d099 100644 --- a/crates/jp_config/src/providers/llm/anthropic.rs +++ b/crates/jp_config/src/providers/llm/anthropic.rs @@ -6,8 +6,8 @@ use crate::{ assignment::{AssignKeyValue, AssignResult, KvAssignment, missing_key}, delta::{PartialConfigDelta, delta_opt, delta_opt_vec}, fill::FillDefaults, + internal::merge::append_vec_dedup, partial::{ToPartial, partial_opt}, - util, }; /// Anthropic API configuration. @@ -38,7 +38,7 @@ pub struct AnthropicConfig { /// /// To find out which beta headers are available, see: /// - #[setting(default = vec![], merge = schematic::merge::append_vec, transform = util::vec_dedup)] + #[setting(default = vec![], merge = append_vec_dedup)] pub beta_headers: Vec, } diff --git a/crates/jp_config/src/providers/mcp_tests.rs b/crates/jp_config/src/providers/mcp_tests.rs index 2f5d517cd..bca21e13e 100644 --- a/crates/jp_config/src/providers/mcp_tests.rs +++ b/crates/jp_config/src/providers/mcp_tests.rs @@ -37,6 +37,35 @@ fn mcp_provider_optional_reports_stdio_flag() { assert!(optional.optional()); } +#[test] +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. + let mut base = PartialStdioConfig { + arguments: Some(vec!["serve".to_owned()]), + variables: Some(vec!["HOME".to_owned()]), + ..Default::default() + }; + let overlay = PartialStdioConfig { + arguments: Some(vec!["--verbose".to_owned()]), + variables: Some(vec!["PATH".to_owned()]), + ..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()]) + ); +} + #[test] fn assign_optional_flag_via_cli() { let mut p = PartialStdioConfig::default(); 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 d3b2b0082..60b4b07f9 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 @@ -27,6 +27,7 @@ Ok( discard_when_merged: Some( true, ), + dedup: None, }, ), ), diff --git a/crates/jp_config/src/types.rs b/crates/jp_config/src/types.rs index a4c94f27c..83f93939e 100644 --- a/crates/jp_config/src/types.rs +++ b/crates/jp_config/src/types.rs @@ -7,3 +7,99 @@ pub mod json_value; pub mod map; pub mod string; pub mod vec; + +use std::str::FromStr; + +use serde::de::{Deserializer, Error as DeError, Visitor}; + +use crate::BoxedError; + +/// The three states a `dedup` setting can be written in. +/// +/// `Inherit` carries no opinion, leaving the merge strategies to inherit one +/// from the previous layer. +/// Used to parse `dedup` from key-value assignments (`--cfg …dedup=inherit`), +/// mirroring the values [`deserialize_dedup`] accepts from config files. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub(crate) enum Dedup { + /// `true` + Enabled, + + /// `false` + Disabled, + + /// `"inherit"` + Inherit, +} + +impl Dedup { + /// The opinion this state carries, if any. + pub(crate) const fn opinion(self) -> Option { + match self { + Self::Enabled => Some(true), + Self::Disabled => Some(false), + Self::Inherit => None, + } + } +} + +impl From for Dedup { + fn from(v: bool) -> Self { + if v { Self::Enabled } else { Self::Disabled } + } +} + +impl FromStr for Dedup { + type Err = BoxedError; + + fn from_str(s: &str) -> Result { + match s { + "true" => Ok(Self::Enabled), + "false" => Ok(Self::Disabled), + "inherit" => Ok(Self::Inherit), + _ => Err(format!("expected `true`, `false` or `inherit`, got `{s}`").into()), + } + } +} + +/// Deserialize a `dedup` field from `true`, `false`, or `"inherit"`. +/// +/// `"inherit"` and an absent field both produce `None`, which the merge +/// strategies read as "no opinion" and inherit from the previous layer. +pub(crate) fn deserialize_dedup<'de, D>(deserializer: D) -> Result, D::Error> +where + D: Deserializer<'de>, +{ + struct DedupVisitor; + + impl Visitor<'_> for DedupVisitor { + type Value = Option; + + fn expecting(&self, formatter: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + formatter.write_str("a boolean or \"inherit\"") + } + + fn visit_bool(self, v: bool) -> Result { + Ok(Some(v)) + } + + fn visit_str(self, v: &str) -> Result { + match v { + "inherit" => Ok(None), + "true" => Ok(Some(true)), + "false" => Ok(Some(false)), + _ => Err(DeError::unknown_variant(v, &["true", "false", "inherit"])), + } + } + + fn visit_none(self) -> Result { + Ok(None) + } + + fn visit_unit(self) -> Result { + Ok(None) + } + } + + deserializer.deserialize_any(DedupVisitor) +} diff --git a/crates/jp_config/src/types/json_value_tests.rs b/crates/jp_config/src/types/json_value_tests.rs index 98d74128a..b71f1ae0b 100644 --- a/crates/jp_config/src/types/json_value_tests.rs +++ b/crates/jp_config/src/types/json_value_tests.rs @@ -72,6 +72,21 @@ fn merge_arrays_replace_by_default() { assert_eq!(base.0, json!([3, 4])); } +#[test] +fn merge_arrays_keep_duplicates_in_the_replacement() { + // Free-form values (tool options, template values) reach the vec merge with + // an implicit `replace`. Their contents are the author's own data, so a + // repeated element must not be collapsed. + let mut base = JsonValue(json!([0])); + base.merge(&(), JsonValue(json!([1, 1]))).unwrap(); + assert_eq!(base.0, json!([1, 1])); + + let mut base = JsonValue(json!({"retries": [500, 500, 500]})); + base.merge(&(), JsonValue(json!({"retries": [250, 250]}))) + .unwrap(); + assert_eq!(base.0, json!({"retries": [250, 250]})); +} + // --- Array strategies --- #[test] diff --git a/crates/jp_config/src/types/string.rs b/crates/jp_config/src/types/string.rs index 59125b176..405ab033c 100644 --- a/crates/jp_config/src/types/string.rs +++ b/crates/jp_config/src/types/string.rs @@ -9,7 +9,9 @@ use serde_untagged::UntaggedEnumVisitor; use crate::{ assignment::{AssignKeyValue, AssignResult, KvAssignment, missing_key}, delta::{PartialConfigDelta, delta_opt}, + fill::FillDefaults, partial::ToPartial, + types::{Dedup, deserialize_dedup}, }; /// String value, either defaulting to a merge strategy of `replace`, or @@ -75,15 +77,29 @@ impl Deref for PartialMergeableString { impl AssignKeyValue for PartialMergeableString { fn assign(&mut self, kv: KvAssignment) -> AssignResult { - match kv.key_string().as_str() { - "" => *self = kv.try_object_or_from_str()?, - _ => match self { - Self::String(_) => return missing_key(&kv), - Self::Merged(config) => config.assign(kv)?, - }, + if kv.key_string().is_empty() { + *self = kv.try_object_or_from_str()?; + return Ok(()); } - Ok(()) + // A nested key addresses the merge metadata, which the plain-string + // form has nowhere to put, so promote it. The strategy is pinned to + // `replace` because that is what a plain string means — leaving it + // unstated would resolve to `append`. + if let Self::String(value) = self { + let value = std::mem::take(value); + *self = Self::Merged(PartialMergedString { + value: Some(value), + strategy: Some(MergedStringStrategy::Replace), + ..PartialMergedString::default() + }); + } + + let Self::Merged(config) = self else { + return missing_key(&kv); + }; + + config.assign(kv) } } @@ -97,6 +113,25 @@ impl PartialConfigDelta for PartialMergeableString { } } +impl FillDefaults for PartialMergeableString { + /// Fill gaps from `defaults`, keeping every value this side states. + /// + /// A metadata-only `Merged` (`{ dedup = false }`, say) has no value of its + /// own, so it takes the default's — without this, stating metadata alone + /// would suppress the default value entirely. + /// A plain string states a complete value and has no gaps to fill. + fn fill_from(self, defaults: Self) -> Self { + match (self, defaults) { + (Self::Merged(v), Self::Merged(d)) => Self::Merged(v.fill_from(d)), + (Self::Merged(v), Self::String(d)) => Self::Merged(PartialMergedString { + value: v.value.or(Some(d)), + ..v + }), + (v, _) => v, + } + } +} + impl ToPartial for MergeableString { fn to_partial(&self) -> Self::Partial { // Always flatten to `String` variant. The finalized value already @@ -156,6 +191,30 @@ pub struct MergedString { /// other value is set. #[setting(default)] pub discard_when_merged: bool, + + /// Whether to skip an `append` or `prepend` whose value is already present. + /// + /// Defaults to `true`. + /// Set to `false` to append the value unconditionally. + /// Accepts `true`, `false`, or `"inherit"`. + /// + /// A value counts as present when it appears in the existing string as a + /// whole `separator`-delimited block. + /// Partial matches inside a block do not count. + /// With `separator = "none"` there are no block boundaries to match + /// against, so only an exact match of the whole string counts. + /// + /// This flag is "sticky": once a config in the merge chain sets it + /// explicitly, subsequent merges for this field use that value — unless a + /// later config states a different one. + /// + /// `"inherit"` (or omitting the field) means "no opinion" — inherit from + /// the previous merge, falling back to `true`. + #[setting( + skip_serializing_if = "Option::is_none", + deserialize_with = "deserialize_dedup" + )] + pub dedup: Option, } impl AssignKeyValue for PartialMergedString { @@ -166,6 +225,16 @@ impl AssignKeyValue for PartialMergedString { "strategy" => self.strategy = kv.try_some_from_str()?, "separator" => self.separator = kv.try_some_from_str()?, "discard_when_merged" => self.discard_when_merged = kv.try_some_bool()?, + // Tri-state. `inherit` states no opinion, which in a config file + // leaves an earlier layer's choice standing — so it is a no-op here + // too. Assignments mutate the accumulated partial in place, so + // writing `None` would instead erase what a lower layer set. + // Returning to the default takes an explicit `dedup=true`. + "dedup" => match kv.try_some_bool_or_from_str::()? { + Some(Dedup::Inherit) => {} + Some(opinion) => self.dedup = opinion.opinion(), + None => self.dedup = None, + }, _ => return missing_key(&kv), } @@ -180,6 +249,19 @@ impl ToPartial for MergedString { strategy: Some(self.strategy), separator: Some(self.separator), discard_when_merged: Some(self.discard_when_merged), + dedup: self.dedup, + } + } +} + +impl FillDefaults for PartialMergedString { + fn fill_from(self, defaults: Self) -> Self { + Self { + value: self.value.or(defaults.value), + strategy: self.strategy.or(defaults.strategy), + separator: self.separator.or(defaults.separator), + discard_when_merged: self.discard_when_merged.or(defaults.discard_when_merged), + dedup: self.dedup.or(defaults.dedup), } } } @@ -194,6 +276,7 @@ impl PartialConfigDelta for PartialMergedString { self.discard_when_merged.as_ref(), next.discard_when_merged, ), + dedup: delta_opt(self.dedup.as_ref(), next.dedup), } } } diff --git a/crates/jp_config/src/types/vec.rs b/crates/jp_config/src/types/vec.rs index 769dc7ed1..b30928c2b 100644 --- a/crates/jp_config/src/types/vec.rs +++ b/crates/jp_config/src/types/vec.rs @@ -6,7 +6,9 @@ use schematic::{Config, ConfigEnum, PartialConfig as _, Schematic}; use serde::{Deserialize, Deserializer, Serialize, de::DeserializeOwned}; use serde_untagged::UntaggedEnumVisitor; -use crate::{delta::PartialConfigDelta, fill::FillDefaults, partial::ToPartial}; +use crate::{ + delta::PartialConfigDelta, fill::FillDefaults, partial::ToPartial, types::deserialize_dedup, +}; /// Vec of `T`'s, either defaulting to a merge strategy of `replace`, or /// defining a specific merge strategy. @@ -300,18 +302,24 @@ pub struct MergedVec { /// Whether to remove duplicate items after merging. /// + /// Defaults to `true` for `append` and `prepend`, which combine two lists. + /// Set to `false` to keep duplicates. /// Accepts `true`, `false`, or `"inherit"`. /// - /// When `true`, items already present in the merged result are skipped. + /// `replace` keeps duplicates by default: it contributes a single list, so + /// repeated items in it are kept as written. + /// Set `dedup = true` alongside `strategy = "replace"` to collapse them. + /// + /// When enabled, items already present in the merged result are skipped. /// Comparison uses `PartialEq`. /// Order is preserved (first occurrence wins). /// /// This flag is "sticky": once a non-discarded config in the merge chain - /// sets it to `true`, all subsequent merges for this field will deduplicate - /// — unless a later config explicitly sets it to `false`. + /// sets it explicitly, all subsequent merges for this field use that value + /// — unless a later config states a different one. /// /// `"inherit"` (or omitting the field) means "no opinion" — inherit from - /// the previous merge. + /// the previous merge, falling back to the per-strategy default above. #[setting(default, skip_serializing_if = "Option::is_none")] #[serde( default, @@ -364,44 +372,3 @@ pub enum MergedVecStrategy { /// Replace the previous value with the new value. Replace, } - -/// Deserialize `dedup` from `true`, `false`, or `"inherit"` → `Option`. -fn deserialize_dedup<'de, D>(deserializer: D) -> Result, D::Error> -where - D: serde::Deserializer<'de>, -{ - struct DedupVisitor; - - impl serde::de::Visitor<'_> for DedupVisitor { - type Value = Option; - - fn expecting(&self, formatter: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - formatter.write_str("a boolean or \"inherit\"") - } - - fn visit_bool(self, v: bool) -> Result { - Ok(Some(v)) - } - - fn visit_str(self, v: &str) -> Result { - match v { - "inherit" => Ok(None), - "true" => Ok(Some(true)), - "false" => Ok(Some(false)), - _ => Err(serde::de::Error::unknown_variant(v, &[ - "true", "false", "inherit", - ])), - } - } - - fn visit_none(self) -> Result { - Ok(None) - } - - fn visit_unit(self) -> Result { - Ok(None) - } - } - - deserializer.deserialize_any(DedupVisitor) -} diff --git a/crates/jp_config/src/util.rs b/crates/jp_config/src/util.rs index 75593ca19..0a212d19d 100644 --- a/crates/jp_config/src/util.rs +++ b/crates/jp_config/src/util.rs @@ -1,6 +1,7 @@ //! Configuration utilities. use std::{ + collections::HashMap, ffi::OsStr, fs, path::{Path, PathBuf}, @@ -9,7 +10,7 @@ use std::{ use camino::Utf8Path; use glob::glob; use indexmap::IndexMap; -use schematic::{ConfigLoader, MergeError, MergeResult, PartialConfig, TransformResult}; +use schematic::{ConfigLoader, MergeError, MergeResult, PartialConfig}; use tracing::{debug, error, info, trace, warn}; use crate::{ @@ -163,21 +164,8 @@ pub fn find_file_in_load_path( /// /// # Errors /// -/// See `load_config_file_at_path`. +/// See [`resolve_extends_graph`]. pub fn load_partial_at_path>(path: P) -> Result, Error> { - // Cycle and depth guarding for `extends` chains is implemented lazily: we - // maintain a DFS ancestor stack (see [`ExtendsStack`]), pushing the - // canonicalized path of each file before recursing into its `extends` and - // popping on the way out. Re-entry into a file already on the stack is a - // cycle. - // - // Future direction: resolve the full `extends` DAG eagerly before any - // `ConfigLoader` work happens. Walk each file once to read its `extends`, - // expand globs, canonicalize, and build a graph of participating files. - // Cycles would then be caught statically (with clearer diagnostics), and - // the resolved graph would give `--explain` (RFD 060) a natural home for - // per-file provenance. Until then, the lazy approach below keeps the - // change surface small and relies on the depth cap as a backstop. load_partial_at_path_with_max_depth(path, MAX_EXTENDS_DEPTH) } @@ -191,14 +179,24 @@ fn load_partial_at_path_with_max_depth>( path: P, max_depth: u8, ) -> Result, Error> { - let mut loader = ConfigLoader::::new(); let mut stack = ExtendsStack::new(max_depth); - match load_config_file_at_path(path, &mut loader, false, &mut stack) { + let mut entries = Vec::new(); + + match resolve_extends_graph(path, false, &mut stack, &mut entries) { Ok(()) => {} Err(Error::Schematic(schematic::ConfigError::MissingFile(_))) => return Ok(None), Err(error) => return Err(error), } + let mut loader = ConfigLoader::::new(); + for entry in dedup_keep_last(entries) { + if entry.optional { + loader.file_optional(&entry.path)?; + } else { + loader.file(&entry.path)?; + } + } + loader.load_partial(&()).map(Some).map_err(Into::into) } @@ -307,19 +305,43 @@ pub fn build(partial: PartialAppConfig) -> Result { Ok(config) } -/// Open a configuration file at `path`, if it exists. +/// One config file in a resolved `extends` graph. +struct ExtendsEntry { + /// Path to the file, with a valid extension resolved. + path: PathBuf, + + /// Canonicalized path, used to recognize the same file reached through more + /// than one `extends` branch. + canonical: PathBuf, + + /// Whether the loader tolerates the file going missing. + /// + /// True for every file reached through `extends`, false for the file the + /// walk started from. + optional: bool, +} + +/// Walk the `extends` graph rooted at `path`, collecting every file to load in +/// merge order (least specific first). /// -/// If the file does not exist, the same file name is used but with one of the +/// A file reached through several branches appears once per branch; use +/// [`dedup_keep_last`] to reduce the result to one entry per file. +/// +/// If the file does not exist, the same file name is retried with each of the /// valid `VALID_CONFIG_FILE_EXTS` extensions. /// /// # Errors /// -/// Can error if file parsing fails, or if partial validation fails. -fn load_config_file_at_path>( +/// Returns [`Error::ExtendsCycle`] if a file extends itself through any chain, +/// [`Error::ExtendsDepthExceeded`] if nesting exceeds `stack`'s cap, and a +/// [`schematic::ConfigError::MissingFile`] if `path` does not resolve to a +/// file. +/// Parse failures in any visited file propagate. +fn resolve_extends_graph>( path: P, - loader: &mut ConfigLoader, optional: bool, stack: &mut ExtendsStack, + entries: &mut Vec, ) -> Result<(), Error> { let mut path: PathBuf = path.into(); @@ -340,20 +362,20 @@ fn load_config_file_at_path>( // canonicalization fails we fall back to the raw path; the depth cap in // `ExtendsStack` protects against cycles slipping through in that case. let canonical = fs::canonicalize(&path).unwrap_or_else(|_| path.clone()); - stack.try_push(canonical)?; - let result = load_config_file_with_extends(&path, loader, optional, stack); + stack.try_push(canonical.clone())?; + let result = resolve_extends_of(&path, canonical, optional, stack, entries); stack.pop(); result } -/// Load a configuration file at `path`, assuming it exists. -/// -/// If the file configures `extends`, those will be loaded as well. -fn load_config_file_with_extends( +/// Resolve the `extends` declarations of the file at `path`, then record the +/// file itself between its `before` and `after` extensions. +fn resolve_extends_of( path: &Path, - loader: &mut ConfigLoader, + canonical: PathBuf, optional: bool, stack: &mut ExtendsStack, + entries: &mut Vec, ) -> Result<(), Error> { let root = path.parent().map(Path::to_path_buf); @@ -365,25 +387,25 @@ fn load_config_file_with_extends( .flatten() .partition(ExtendingRelativePath::is_before); - load_optional_paths(before, root.as_deref(), loader, stack)?; + resolve_extended_paths(before, root.as_deref(), stack, entries)?; - if optional { - loader.file_optional(path)?; - } else { - loader.file(path)?; - } + entries.push(ExtendsEntry { + path: path.to_path_buf(), + canonical, + optional, + }); - load_optional_paths(after, root.as_deref(), loader, stack)?; + resolve_extended_paths(after, root.as_deref(), stack, entries)?; Ok(()) } -/// Load the optional paths. -fn load_optional_paths( +/// Resolve a list of `extends` declarations, expanding globs. +fn resolve_extended_paths( extends: impl IntoIterator, root: Option<&Path>, - loader: &mut ConfigLoader, stack: &mut ExtendsStack, + entries: &mut Vec, ) -> Result<(), Error> { for path in extends { let Some(root) = &root else { @@ -410,23 +432,44 @@ fn load_optional_paths( } }; - load_config_file_at_path(&path, loader, true, stack)?; + resolve_extends_graph(&path, true, stack, entries)?; } } Ok(()) } -/// Order-preserving dedup for use as `transform = vec_dedup`. -#[expect(clippy::trivially_copy_pass_by_ref, clippy::unnecessary_wraps)] -pub(crate) fn vec_dedup(v: Vec, _: &()) -> TransformResult> { - let mut seen = Vec::with_capacity(v.len()); - for item in v { - if !seen.contains(&item) { - seen.push(item); - } +/// Reduce a resolved `extends` graph to one entry per file, keeping each file's +/// last position. +/// +/// A file reached through two `extends` branches is loaded once. +/// Loading it twice re-applies `append` and `prepend` merge strategies, +/// duplicating prompt text, sections, instructions, and attachments. +/// +/// The last position is kept rather than the first so that an `extends` array +/// keeps its declared precedence: in `extends = ["a.toml", "b.toml"]` where +/// `a.toml` also extends `b.toml`, `b.toml`'s values still override `a.toml`'s. +fn dedup_keep_last(entries: Vec) -> Vec { + let mut last_index: HashMap = HashMap::with_capacity(entries.len()); + for (index, entry) in entries.iter().enumerate() { + last_index.insert(entry.canonical.clone(), index); } - Ok(seen) + + entries + .into_iter() + .enumerate() + .filter_map(|(index, entry)| { + if last_index.get(&entry.canonical) == Some(&index) { + return Some(entry); + } + + debug!( + path = %entry.path.display(), + "Skipping repeat visit to extended configuration file." + ); + None + }) + .collect() } /// Merge [`IndexMap`]s of nested [`PartialConfig`]s. diff --git a/crates/jp_config/src/util_tests.rs b/crates/jp_config/src/util_tests.rs index 195964ce6..077fbc8ad 100644 --- a/crates/jp_config/src/util_tests.rs +++ b/crates/jp_config/src/util_tests.rs @@ -732,18 +732,144 @@ fn test_load_partial_at_path_diamond_is_not_a_cycle() { ); write_config(&root.join("d.toml"), "assistant.system_prompt = \"d\""); - let partial = load_partial_at_path(root.join("a.toml")).unwrap(); - assert!(partial.is_some()); + let partial = load_partial_at_path(root.join("a.toml")).unwrap().unwrap(); + assert_eq!(partial.assistant.system_prompt.as_deref(), Some("d")); } #[test] -fn test_vec_dedup_preserves_order() { - let result = vec_dedup(vec![3, 1, 2, 1, 3, 4], &()).unwrap(); - assert_eq!(result, vec![3, 1, 2, 4]); +fn test_load_partial_at_path_diamond_applies_shared_file_once() { + // a -> b -> d + // a -> c -> d + // + // `d` appends to the system prompt. Reaching it through both branches must + // apply the append once: a shared dependency is a diamond, not two + // separate contributions. + let tmp = tempdir().unwrap(); + let root = tmp.path(); + write_config(&root.join("a.toml"), r#"extends = ["b.toml", "c.toml"]"#); + write_config(&root.join("b.toml"), r#"extends = ["d.toml"]"#); + write_config(&root.join("c.toml"), r#"extends = ["d.toml"]"#); + write_config( + &root.join("d.toml"), + indoc::indoc!( + r#" + [assistant.system_prompt] + strategy = "append" + separator = "space" + value = "d" + "# + ), + ); + + let partial = load_partial_at_path(root.join("a.toml")).unwrap().unwrap(); + assert_eq!(partial.assistant.system_prompt.as_deref(), Some("d")); +} + +#[test] +fn test_load_partial_at_path_diamond_keeps_shared_file_last() { + // a -> b -> d + // a -> c -> d + // + // `b` and `d` both set the same replace-merged field. Collapsing `d`'s two + // visits to the last one keeps `d` after `b`, so `d` wins — which is the + // same winner the uncollapsed graph produced (`[d, b, d, c, a]`), because + // the repeat visit already clobbered `b`. + let tmp = tempdir().unwrap(); + let root = tmp.path(); + write_config(&root.join("a.toml"), r#"extends = ["b.toml", "c.toml"]"#); + write_config( + &root.join("b.toml"), + indoc::indoc!( + r#" + extends = ["d.toml"] + assistant.name = "b" + "# + ), + ); + write_config(&root.join("c.toml"), r#"extends = ["d.toml"]"#); + write_config(&root.join("d.toml"), r#"assistant.name = "d""#); + + let partial = load_partial_at_path(root.join("a.toml")).unwrap().unwrap(); + assert_eq!(partial.assistant.name.as_deref(), Some("d")); +} + +#[test] +fn test_load_partial_at_path_repeat_visit_keeps_last_position() { + // a -> b -> d + // a -> d + // + // `a` declares `extends = ["b.toml", "d.toml"]`, so `d` overrides `b`. + // Collapsing the repeat visit must not move `d` ahead of `b` and flip that + // precedence. + let tmp = tempdir().unwrap(); + let root = tmp.path(); + write_config(&root.join("a.toml"), r#"extends = ["b.toml", "d.toml"]"#); + write_config( + &root.join("b.toml"), + indoc::indoc!( + r#" + extends = ["d.toml"] + assistant.name = "b" + "# + ), + ); + write_config(&root.join("d.toml"), r#"assistant.name = "d""#); + + let partial = load_partial_at_path(root.join("a.toml")).unwrap().unwrap(); + assert_eq!(partial.assistant.name.as_deref(), Some("d")); +} + +/// The `config_load_paths` entries of a loaded partial, as strings. +fn load_paths(partial: &PartialAppConfig) -> Vec<&str> { + partial + .config_load_paths + .as_deref() + .unwrap_or_default() + .iter() + .map(|p| p.as_str()) + .collect() +} + +#[test] +fn test_load_partial_at_path_dedups_load_paths_across_files() { + // Two files naming the same search directory contribute it once. This is + // the path the merge strategy actually runs on: `merge_setting` only + // invokes it when both layers supply a value. + let tmp = tempdir().unwrap(); + let root = tmp.path(); + write_config( + &root.join("a.toml"), + indoc::indoc!( + r#" + extends = ["b.toml"] + config_load_paths = ["shared", "a-only"] + "# + ), + ); + write_config( + &root.join("b.toml"), + r#"config_load_paths = ["shared", "b-only"]"#, + ); + + let partial = load_partial_at_path(root.join("a.toml")).unwrap().unwrap(); + + assert_eq!(load_paths(&partial), ["shared", "b-only", "a-only"]); } #[test] -fn test_vec_dedup_no_duplicates() { - let result = vec_dedup(vec![1, 2, 3], &()).unwrap(); - assert_eq!(result, vec![1, 2, 3]); +fn test_load_partial_at_path_keeps_repeats_from_a_single_file() { + // One file, so nothing is combined and the list is stored as written. + // Repeats inside a single source are the author's own data, the same rule + // `replace` follows on `MergeableVec`; the resolved list is searched in + // order and stops at the first match, so a repeat changes no outcome. + let tmp = tempdir().unwrap(); + let root = tmp.path(); + write_config( + &root.join("a.toml"), + r#"config_load_paths = ["dupe", "dupe"]"#, + ); + + let partial = load_partial_at_path(root.join("a.toml")).unwrap().unwrap(); + + assert_eq!(load_paths(&partial), ["dupe", "dupe"]); }