From 7263cfbde38fbd106740ecacc6ac6cd2a59bbfe3 Mon Sep 17 00:00:00 2001 From: Jean Mertz Date: Mon, 7 Sep 2026 13:47:43 +0200 Subject: [PATCH 01/16] refactor(config): Extract the strategy-carrying map delta `conversation.labels` computed its own delta inline. The rule it encoded is not specific to labels: an entry both states share carries its own delta, an entry only the next state has is carried whole, and a key the previous state had and the next one does not was dropped, which entries cannot spell -- so the whole map carries `replace` rather than letting a deep merge resurrect the key. `delta_mergeable_map` states that once, alongside `delta_mergeable_vec`. It also drops an entry whose own delta comes out empty, which the inline version did not: a missing entry already means "unchanged", so an empty one reads as a change that is not there and makes the enclosing partial look non-empty. That is the same noise the map delta for `providers.mcp` was fixed for. Behaviour is otherwise unchanged; the helper is what the remaining `IndexMap` fields need as they gain a strategy of their own. Signed-off-by: Jean Mertz --- crates/jp_config/src/conversation.rs | 47 +++++----------------------- crates/jp_config/src/delta.rs | 46 ++++++++++++++++++++++++++- 2 files changed, 52 insertions(+), 41 deletions(-) diff --git a/crates/jp_config/src/conversation.rs b/crates/jp_config/src/conversation.rs index ebda01226..3efd5792e 100644 --- a/crates/jp_config/src/conversation.rs +++ b/crates/jp_config/src/conversation.rs @@ -14,16 +14,18 @@ use crate::{ conversation::{ attachment::{AttachmentConfig, PartialAttachmentConfig}, compaction::{CompactionConfig, PartialCompactionConfig}, - label::{LabelConfig, PartialLabelConfig}, + label::LabelConfig, title::{PartialTitleConfig, TitleConfig}, tool::{PartialToolsConfig, ToolsConfig}, }, - delta::{PartialConfigDelta, delta_mergeable_vec, delta_opt, delta_opt_at, path}, + delta::{ + PartialConfigDelta, delta_mergeable_map, delta_mergeable_vec, delta_opt, delta_opt_at, path, + }, fill::FillDefaults, internal::merge::{map_with_strategy, vec_with_strategy}, partial::{ToPartial, partial_opt}, types::{ - map::{MergeableMap, MergedMap, MergedMapStrategy, map_to_mergeable_partial}, + map::{MergeableMap, map_to_mergeable_partial}, vec::{MergeableVec, MergedVec, vec_to_mergeable_partial}, }, validate::Validator, @@ -132,41 +134,6 @@ impl AssignKeyValue for PartialConversationConfig { } } -impl PartialConversationConfig { - /// The label rules `next` changes. - fn labels_delta( - &self, - next: MergeableMap, - ) -> MergeableMap { - // A key in the previous state that is absent from the next one - // can only have been dropped by a replacing layer, and a - // minimal delta has no way to spell "removed": it carries - // entries, and a missing entry means "unchanged". Emit the - // whole wrapper in that case so the fold replaces the map - // instead of deep-merging the dropped rule back in. - let dropped = self.labels.keys().any(|key| !next.contains_key(key)); - - if dropped { - // Force replace semantics rather than trusting the shape - // `next` arrived in: a plain `Map` deep-merges on the fold - // and resurrects the dropped rule. - MergeableMap::Merged(MergedMap { - value: next.into_map(), - strategy: Some(MergedMapStrategy::Replace), - discard_when_merged: false, - }) - } else { - next.into_iter() - .filter_map(|(key, next)| match self.labels.get(&key) { - Some(prev) if prev == &next => None, - Some(prev) => Some((key, prev.delta(next))), - None => Some((key, next)), - }) - .collect() - } - } -} - impl PartialConfigDelta for PartialConversationConfig { fn delta(&self, next: Self) -> Self { Self { @@ -177,7 +144,7 @@ impl PartialConfigDelta for PartialConversationConfig { inquiry: self.inquiry.delta(next.inquiry), start_local: delta_opt(self.start_local.as_ref(), next.start_local), default_id: delta_opt(self.default_id.as_ref(), next.default_id), - labels: self.labels_delta(next.labels), + labels: delta_mergeable_map(&self.labels, next.labels), } } @@ -199,7 +166,7 @@ impl PartialConfigDelta for PartialConversationConfig { next.default_id, unsets, ), - labels: self.labels_delta(next.labels), + labels: delta_mergeable_map(&self.labels, next.labels), } } } diff --git a/crates/jp_config/src/delta.rs b/crates/jp_config/src/delta.rs index a753246c6..12361a6dd 100644 --- a/crates/jp_config/src/delta.rs +++ b/crates/jp_config/src/delta.rs @@ -3,7 +3,10 @@ use indexmap::IndexMap; use schematic::PartialConfig; -use crate::types::vec::{MergeableVec, MergedVec, MergedVecStrategy}; +use crate::types::{ + map::{MergeableMap, MergedMap, MergedMapStrategy}, + vec::{MergeableVec, MergedVec, MergedVecStrategy}, +}; /// Calculate the delta between two partial configurations. /// @@ -110,6 +113,47 @@ fn repeats_an_element(items: &[T]) -> bool { .any(|(index, item)| items[..index].contains(item)) } +/// Calculate the delta between two strategy-carrying maps. +/// +/// An entry only `next` has is carried whole, an entry both maps have carries +/// its own delta, and an entry whose delta is empty is left out: a missing +/// entry already means "unchanged", so carrying an empty one reads as a change +/// that isn't there. +/// +/// A key `prev` has and `next` does not was dropped, which entries cannot +/// spell. +/// The whole map is then carried with `replace`, since a deep merge would +/// resurrect the dropped key. +pub fn delta_mergeable_map(prev: &MergeableMap, next: MergeableMap) -> MergeableMap +where + T: PartialConfigDelta + PartialEq, +{ + if prev.keys().any(|key| !next.contains_key(key)) { + // Stated rather than inherited from `next`'s shape: a plain map + // deep-merges on the fold and brings the dropped key back. + return MergeableMap::Merged(MergedMap { + value: next.into_map(), + strategy: Some(MergedMapStrategy::Replace), + discard_when_merged: false, + }); + } + + next.into_iter() + .filter_map(|(key, next)| { + let Some(prev) = prev.get(&key) else { + return Some((key, next)); + }; + + if prev == &next { + return None; + } + + let delta = prev.delta(next); + (!delta.is_empty()).then_some((key, delta)) + }) + .collect() +} + /// Calculate the delta between two optional strategy-carrying lists. /// /// Wraps [`delta_mergeable_vec`] for a field whose partial is From 6b5a170f54edb86761d486cba8f9045a407c6a4c Mon Sep 17 00:00:00 2001 From: Jean Mertz Date: Mon, 7 Sep 2026 15:06:20 +0200 Subject: [PATCH 02/16] feat(config): Record config map entries the user removed Removing an MCP server, a tool, a model alias, a plugin, a template value, a tool parameter, question or option from a config file now reaches the conversation. Until now the removal was computed, found to be unexpressible, and dropped: the conversation kept starting the server the user had deleted, and recomputed the same non-delta on every turn. Map entries merge by key, which is what lets a server added to the workspace config reach a conversation created before it existed. The same property means no value a delta carries can take an entry away: the key survives from the previous layer. The entry's own path is reported in the delta's `unsets` instead, and the fold removes it before merging, which `unset` already supported. Every map in the configuration is covered, including the ones nested inside a tool, and clearing a field of the `conversation.tools.'*'` defaults block is recorded too. That block resolves through its own type, which had no path-reporting delta, so a cleared `enable` or `style.error.inline_results` there went unrecorded. A map whose values are plain rather than nested partials gets the same treatment through `delta_value_map_with_unsets`, replacing three copies of the same inline entry-comparison loop. Signed-off-by: Jean Mertz --- crates/jp_config/src/conversation.rs | 4 +- crates/jp_config/src/conversation/tool.rs | 147 ++++++++++++++++-- .../jp_config/src/conversation/tool/style.rs | 63 +++++++- crates/jp_config/src/delta.rs | 52 ++++++- crates/jp_config/src/delta_law_tests.rs | 10 -- crates/jp_config/src/delta_tests.rs | 29 ++++ crates/jp_config/src/lib.rs | 12 +- crates/jp_config/src/lib_tests.rs | 51 ++++++ crates/jp_config/src/plugins.rs | 31 ++-- crates/jp_config/src/providers.rs | 10 ++ crates/jp_config/src/providers/llm.rs | 9 +- crates/jp_config/src/template.rs | 24 +-- 12 files changed, 387 insertions(+), 55 deletions(-) diff --git a/crates/jp_config/src/conversation.rs b/crates/jp_config/src/conversation.rs index 3efd5792e..786abcd7b 100644 --- a/crates/jp_config/src/conversation.rs +++ b/crates/jp_config/src/conversation.rs @@ -153,7 +153,9 @@ impl PartialConfigDelta for PartialConversationConfig { title: self .title .delta_with_unsets(next.title, &path(prefix, "title"), unsets), - tools: self.tools.delta(next.tools), + tools: self + .tools + .delta_with_unsets(next.tools, &path(prefix, "tools"), unsets), compaction: self.compaction.delta(next.compaction), attachments: delta_mergeable_vec(&self.attachments, next.attachments), inquiry: self diff --git a/crates/jp_config/src/conversation/tool.rs b/crates/jp_config/src/conversation/tool.rs index d09eeb8c0..1cc01da97 100644 --- a/crates/jp_config/src/conversation/tool.rs +++ b/crates/jp_config/src/conversation/tool.rs @@ -17,7 +17,11 @@ use crate::{ access::{AccessConfig, PartialAccessConfig}, style::{DisplayStyleConfig, PartialDisplayStyleConfig}, }, - delta::{PartialConfigDelta, delta_map, delta_opt, delta_opt_partial, delta_vec}, + delta::{ + PartialConfigDelta, delta_map, delta_map_with_unsets, delta_opt, delta_opt_at, + delta_opt_partial, delta_opt_partial_at, delta_value_map, delta_value_map_with_unsets, + delta_vec, path, + }, fill::{FillDefaults, fill_map}, partial::{ToPartial, partial_opt, partial_opt_config, partial_opts}, types::json_value::JsonValue, @@ -66,6 +70,15 @@ impl PartialConfigDelta for PartialToolsConfig { tools: delta_map(&self.tools, next.tools), } } + + fn delta_with_unsets(&self, next: Self, prefix: &str, unsets: &mut Vec) -> Self { + Self { + defaults: self + .defaults + .delta_with_unsets(next.defaults, &path(prefix, "*"), unsets), + tools: delta_map_with_unsets(prefix, &self.tools, next.tools, unsets), + } + } } impl FillDefaults for PartialToolsConfig { @@ -363,6 +376,33 @@ impl PartialConfigDelta for PartialToolsDefaultsConfig { access: delta_opt_partial(self.access.as_ref(), next.access), } } + + fn delta_with_unsets(&self, next: Self, prefix: &str, unsets: &mut Vec) -> Self { + Self { + enable: delta_opt_partial_at( + &path(prefix, "enable"), + self.enable.as_ref(), + next.enable, + unsets, + ), + run: delta_opt(self.run.as_ref(), next.run), + format: delta_opt(self.format.as_ref(), next.format), + result: delta_opt(self.result.as_ref(), next.result), + cancellation_response: delta_opt( + self.cancellation_response.as_ref(), + next.cancellation_response, + ), + style: self + .style + .delta_with_unsets(next.style, &path(prefix, "style"), unsets), + access: delta_opt_partial_at( + &path(prefix, "access"), + self.access.as_ref(), + next.access, + unsets, + ), + } + } } impl FillDefaults for PartialToolsDefaultsConfig { @@ -586,19 +626,68 @@ impl PartialConfigDelta for PartialToolConfig { ), style: delta_opt_partial(self.style.as_ref(), next.style), questions: delta_map(&self.questions, next.questions), - options: next - .options - .into_iter() - .filter_map(|(name, next)| { - if self.options.get(&name).is_some_and(|prev| prev == &next) { - return None; - } - Some((name, next)) - }) - .collect(), + options: delta_value_map(&self.options, next.options), access: delta_opt_partial(self.access.as_ref(), next.access), } } + + fn delta_with_unsets(&self, next: Self, prefix: &str, unsets: &mut Vec) -> Self { + Self { + source: delta_opt(self.source.as_ref(), next.source), + enable: delta_opt_partial_at( + &path(prefix, "enable"), + self.enable.as_ref(), + next.enable, + unsets, + ), + command: delta_opt_partial_at( + &path(prefix, "command"), + self.command.as_ref(), + next.command, + unsets, + ), + summary: delta_opt(self.summary.as_ref(), next.summary), + description: delta_opt(self.description.as_ref(), next.description), + examples: delta_opt(self.examples.as_ref(), next.examples), + parameters: delta_map_with_unsets( + &path(prefix, "parameters"), + &self.parameters, + next.parameters, + unsets, + ), + run: delta_opt(self.run.as_ref(), next.run), + format: delta_opt(self.format.as_ref(), next.format), + result: delta_opt(self.result.as_ref(), next.result), + cancellation_response: delta_opt( + self.cancellation_response.as_ref(), + next.cancellation_response, + ), + style: delta_opt_partial_at( + &path(prefix, "style"), + self.style.as_ref(), + next.style, + unsets, + ), + questions: delta_map_with_unsets( + &path(prefix, "questions"), + &self.questions, + next.questions, + unsets, + ), + options: delta_value_map_with_unsets( + &path(prefix, "options"), + &self.options, + next.options, + unsets, + ), + access: delta_opt_partial_at( + &path(prefix, "access"), + self.access.as_ref(), + next.access, + unsets, + ), + } + } } impl ToPartial for ToolConfig { @@ -740,6 +829,25 @@ impl PartialConfigDelta for PartialToolParameterConfig { properties: delta_map(&self.properties, next.properties), } } + + fn delta_with_unsets(&self, next: Self, prefix: &str, unsets: &mut Vec) -> Self { + Self { + kind: delta_opt_partial(self.kind.as_ref(), next.kind), + default: delta_opt(self.default.as_ref(), next.default), + required: delta_opt(self.required.as_ref(), next.required), + summary: delta_opt(self.summary.as_ref(), next.summary), + description: delta_opt(self.description.as_ref(), next.description), + examples: delta_opt(self.examples.as_ref(), next.examples), + enumeration: delta_opt(self.enumeration.as_ref(), next.enumeration), + items: delta_opt(self.items.as_ref(), next.items), + properties: delta_map_with_unsets( + &path(prefix, "properties"), + &self.properties, + next.properties, + unsets, + ), + } + } } impl ToPartial for ToolParameterConfig { @@ -1664,6 +1772,23 @@ impl PartialConfigDelta for PartialEnableConfig { allow_toggle: delta_opt(self.allow_toggle.as_ref(), next.allow_toggle), } } + + fn delta_with_unsets(&self, next: Self, prefix: &str, unsets: &mut Vec) -> Self { + Self { + state: delta_opt_at( + &path(prefix, "state"), + self.state.as_ref(), + next.state, + unsets, + ), + allow_toggle: delta_opt_at( + &path(prefix, "allow_toggle"), + self.allow_toggle.as_ref(), + next.allow_toggle, + unsets, + ), + } + } } impl ToPartial for EnableConfig { diff --git a/crates/jp_config/src/conversation/tool/style.rs b/crates/jp_config/src/conversation/tool/style.rs index 50182451b..1373da62a 100644 --- a/crates/jp_config/src/conversation/tool/style.rs +++ b/crates/jp_config/src/conversation/tool/style.rs @@ -30,7 +30,7 @@ use crate::{ BoxedError, assignment::{AssignKeyValue, AssignResult, KvAssignment, missing_key}, conversation::tool::CommandConfigOrString, - delta::{PartialConfigDelta, delta_opt}, + delta::{PartialConfigDelta, delta_opt, delta_opt_at, path}, fill::FillDefaults, partial::{ToPartial, partial_opt, partial_opts}, }; @@ -179,6 +179,50 @@ impl PartialConfigDelta for PartialDisplayStyleConfig { error: self.error.delta(next.error), } } + + fn delta_with_unsets(&self, next: Self, prefix: &str, unsets: &mut Vec) -> Self { + Self { + hidden: delta_opt_at( + &path(prefix, "hidden"), + self.hidden.as_ref(), + next.hidden, + unsets, + ), + joins_reasoning: delta_opt_at( + &path(prefix, "joins_reasoning"), + self.joins_reasoning.as_ref(), + next.joins_reasoning, + unsets, + ), + inline_results: delta_opt_at( + &path(prefix, "inline_results"), + self.inline_results.as_ref(), + next.inline_results, + unsets, + ), + results_file_link: delta_opt_at( + &path(prefix, "results_file_link"), + self.results_file_link.as_ref(), + next.results_file_link, + unsets, + ), + parameters: delta_opt_at( + &path(prefix, "parameters"), + self.parameters.as_ref(), + next.parameters, + unsets, + ), + print_stderr: delta_opt_at( + &path(prefix, "print_stderr"), + self.print_stderr.as_ref(), + next.print_stderr, + unsets, + ), + error: self + .error + .delta_with_unsets(next.error, &path(prefix, "error"), unsets), + } + } } impl FillDefaults for PartialDisplayStyleConfig { @@ -246,6 +290,23 @@ impl PartialConfigDelta for PartialErrorStyleConfig { results_file_link: delta_opt(self.results_file_link.as_ref(), next.results_file_link), } } + + fn delta_with_unsets(&self, next: Self, prefix: &str, unsets: &mut Vec) -> Self { + Self { + inline_results: delta_opt_at( + &path(prefix, "inline_results"), + self.inline_results.as_ref(), + next.inline_results, + unsets, + ), + results_file_link: delta_opt_at( + &path(prefix, "results_file_link"), + self.results_file_link.as_ref(), + next.results_file_link, + unsets, + ), + } + } } impl FillDefaults for PartialErrorStyleConfig { diff --git a/crates/jp_config/src/delta.rs b/crates/jp_config/src/delta.rs index 12361a6dd..ea03e00a2 100644 --- a/crates/jp_config/src/delta.rs +++ b/crates/jp_config/src/delta.rs @@ -196,10 +196,15 @@ pub fn delta_opt_partial_at( } } -/// Calculate the delta between two maps, reporting each entry's unsets. +/// Calculate the delta between two maps, reporting removed entries and each +/// entry's own unsets. /// -/// Mirrors [`delta_map`], descending into each entry with the entry's own -/// dotted path so a field inside it reports where it lives. +/// Entries merge by key, so an entry `next` no longer has cannot be expressed +/// by merging: the key would survive from the previous layer. +/// Its path joins `unsets` so the fold removes the entry before merging. +/// +/// Descends into an entry both maps have with that entry's own dotted path, so +/// a field inside it reports where it lives. pub fn delta_map_with_unsets( prefix: &str, prev: &IndexMap, @@ -209,6 +214,12 @@ pub fn delta_map_with_unsets( where V: PartialConfigDelta + PartialEq, { + for key in prev.keys() { + if !next.contains_key(key) { + unsets.push(path(prefix, key)); + } + } + next.into_iter() .filter_map(|(key, next)| { let Some(prev) = prev.get(&key) else { @@ -229,6 +240,41 @@ where .collect() } +/// Calculate the delta between two maps of plain values. +/// +/// An entry is kept when `next` holds a value for it that differs from +/// `prev`'s. +/// A map of nested partials wants [`delta_map`] instead, which records only the +/// changed fields of an entry both maps hold. +pub fn delta_value_map( + prev: &IndexMap, + next: IndexMap, +) -> IndexMap { + next.into_iter() + .filter(|(key, next)| !prev.get(key).is_some_and(|prev| prev == next)) + .collect() +} + +/// Calculate the delta between two maps of plain values, reporting removed +/// entries. +/// +/// Mirrors [`delta_map_with_unsets`] for a map whose values carry no partial of +/// their own. +pub fn delta_value_map_with_unsets( + prefix: &str, + prev: &IndexMap, + next: IndexMap, + unsets: &mut Vec, +) -> IndexMap { + for key in prev.keys() { + if !next.contains_key(key) { + unsets.push(path(prefix, key)); + } + } + + delta_value_map(prev, next) +} + /// Calculate the delta between two optional values, reporting a cleared field. /// /// A value that went away cannot be expressed by merging: schematic keeps the diff --git a/crates/jp_config/src/delta_law_tests.rs b/crates/jp_config/src/delta_law_tests.rs index 0d99ffe09..94e1f984e 100644 --- a/crates/jp_config/src/delta_law_tests.rs +++ b/crates/jp_config/src/delta_law_tests.rs @@ -94,22 +94,12 @@ fn assert_law(before: &[&str], after: &[&str]) { /// itself, and clearing removes an entry that was never there. /// Reaching the whole field needs a path vocabulary that can say "this map" /// where the map is also the fallback. -/// -/// `conversation.tools.*` addresses the tool defaults block, whose types have -/// no path-reporting delta yet. -/// Mechanical to add, and left for the pass that does the tool config as a -/// whole. const CLEAR_NOT_RECORDED: &[&str] = &[ "conversation.compaction.rules", "assistant.model.parameters.other", "style.reasoning.summary_model.parameters.other", "conversation.inquiry.assistant.model.parameters.other", "conversation.title.generate.model.parameters.other", - "conversation.tools.*.enable", - "conversation.tools.*.enable.state", - "conversation.tools.*.enable.allow_toggle", - "conversation.tools.*.style.error.inline_results", - "conversation.tools.*.style.error.results_file_link", ]; /// Set `path` to whichever of a few generic values it accepts. diff --git a/crates/jp_config/src/delta_tests.rs b/crates/jp_config/src/delta_tests.rs index 2a653477e..aaae4f532 100644 --- a/crates/jp_config/src/delta_tests.rs +++ b/crates/jp_config/src/delta_tests.rs @@ -29,6 +29,35 @@ fn map(arguments: &[&str]) -> IndexMap { map } +/// A removed map entry is reported, since merging cannot take a key away. +#[test] +fn map_delta_reports_a_removed_entry() { + let prev = map(&["--a"]); + let next = IndexMap::new(); + let mut unsets = Vec::new(); + + let delta = delta_map_with_unsets("providers.mcp", &prev, next, &mut unsets); + + assert!(delta.is_empty(), "nothing to merge for a removed entry"); + assert_eq!(unsets, ["providers.mcp.kagi"]); +} + +/// An entry both maps hold is not reported, only diffed. +#[test] +fn map_delta_does_not_report_a_surviving_entry() { + let prev = map(&["--a"]); + let next = map(&["--a", "--b"]); + let mut unsets = Vec::new(); + + let delta = delta_map_with_unsets("providers.mcp", &prev, next, &mut unsets); + + assert_eq!(delta.len(), 1); + assert!( + unsets.is_empty(), + "the entry survives, so nothing is cleared" + ); +} + /// The `arguments` of a server entry, for asserting on a computed delta. fn arguments(entry: &PartialMcpProviderConfig) -> Option<&Vec> { let PartialMcpProviderConfig::Stdio(config) = entry; diff --git a/crates/jp_config/src/lib.rs b/crates/jp_config/src/lib.rs index 1dcdb2453..2221eb4b1 100644 --- a/crates/jp_config/src/lib.rs +++ b/crates/jp_config/src/lib.rs @@ -316,13 +316,21 @@ impl PartialConfigDelta for PartialAppConfig { &delta_path(prefix, "editor"), unsets, ), - template: self.template.delta(next.template), + template: self.template.delta_with_unsets( + next.template, + &delta_path(prefix, "template"), + unsets, + ), providers: self.providers.delta_with_unsets( next.providers, &delta_path(prefix, "providers"), unsets, ), - plugins: self.plugins.delta(next.plugins), + plugins: self.plugins.delta_with_unsets( + next.plugins, + &delta_path(prefix, "plugins"), + unsets, + ), user: self .user .delta_with_unsets(next.user, &delta_path(prefix, "user"), unsets), diff --git a/crates/jp_config/src/lib_tests.rs b/crates/jp_config/src/lib_tests.rs index 376f40fa3..3be3413f4 100644 --- a/crates/jp_config/src/lib_tests.rs +++ b/crates/jp_config/src/lib_tests.rs @@ -772,6 +772,57 @@ fn a_dropped_mcp_argument_is_recorded() { ); } +/// A server the user removed is recorded, so the conversation stops starting +/// it. +/// +/// Entries merge by key, so no value a delta carries can take one away: the key +/// survives from the previous layer. +/// The entry's path is reported instead, and the fold removes it before +/// merging. +#[test] +fn a_removed_mcp_server_is_recorded() { + use crate::providers::mcp::{McpProviderConfig, StdioConfig}; + + let mut prev = AppConfig::new_test(); + prev.providers.mcp.insert( + "bookworm".to_owned(), + McpProviderConfig::Stdio(StdioConfig { + command: "just".into(), + arguments: vec!["serve".to_owned()], + variables: vec![], + checksum: None, + optional: false, + startup_timeout_secs: 60, + }), + ); + + let mut next = prev.clone(); + next.providers.mcp.shift_remove("bookworm"); + + let mut unsets = Vec::new(); + let delta = prev + .to_partial() + .delta_with_unsets(next.to_partial(), "", &mut unsets); + + assert_eq!(unsets, ["providers.mcp.bookworm"]); + + // Applying the report and then the delta reaches the config the user has. + let mut folded = prev.to_partial(); + folded + .unset("providers.mcp.bookworm") + .expect("a real field"); + folded.merge(&(), delta).expect("folding cannot fail"); + + assert!( + !crate::util::build(folded) + .expect("valid config") + .providers + .mcp + .contains_key("bookworm"), + "the server is gone after the fold" + ); +} + /// A union that names an expanded form contributes both the shorthand path and /// the expanded keys; a union of distinct values contributes only its path. /// diff --git a/crates/jp_config/src/plugins.rs b/crates/jp_config/src/plugins.rs index 801c433d2..ea3355de1 100644 --- a/crates/jp_config/src/plugins.rs +++ b/crates/jp_config/src/plugins.rs @@ -12,7 +12,7 @@ use schematic::Config; use crate::{ FillDefaults, assignment::{AssignKeyValue, AssignResult, KvAssignment, missing_key}, - delta::PartialConfigDelta, + delta::{PartialConfigDelta, delta_map, delta_map_with_unsets, delta_opt, path}, fill::fill_map, partial::ToPartial, plugins::command::CommandPluginConfig, @@ -56,26 +56,29 @@ impl AssignKeyValue for PartialPluginsConfig { impl PartialConfigDelta for PartialPluginsConfig { fn delta(&self, next: Self) -> Self { - use crate::delta::delta_opt; + Self { + auto_install: delta_opt(self.auto_install.as_ref(), next.auto_install), + shutdown_timeout_secs: delta_opt( + self.shutdown_timeout_secs.as_ref(), + next.shutdown_timeout_secs, + ), + command: delta_map(&self.command, next.command), + } + } + fn delta_with_unsets(&self, next: Self, prefix: &str, unsets: &mut Vec) -> Self { Self { auto_install: delta_opt(self.auto_install.as_ref(), next.auto_install), shutdown_timeout_secs: delta_opt( self.shutdown_timeout_secs.as_ref(), next.shutdown_timeout_secs, ), - command: next - .command - .into_iter() - .filter_map(|(name, next)| { - let next = match self.command.get(&name) { - Some(prev) if prev == &next => return None, - Some(prev) => prev.delta(next), - None => next, - }; - Some((name, next)) - }) - .collect(), + command: delta_map_with_unsets( + &path(prefix, "command"), + &self.command, + next.command, + unsets, + ), } } } diff --git a/crates/jp_config/src/providers.rs b/crates/jp_config/src/providers.rs index cbb23b130..41b9e7b2b 100644 --- a/crates/jp_config/src/providers.rs +++ b/crates/jp_config/src/providers.rs @@ -34,6 +34,16 @@ pub struct ProviderConfig { /// /// Configuration for Model Context Protocol (MCP) servers. /// The key is the server ID. + /// + /// ```toml + /// [providers.mcp.bookworm] + /// type = "stdio" + /// command = "just" + /// arguments = ["serve-bookworm"] + /// ``` + /// + /// Entries merge by key, so a server added to a later layer joins the ones + /// an earlier layer configured rather than replacing them. #[setting(nested, merge = merge_nested_indexmap)] pub mcp: IndexMap, } diff --git a/crates/jp_config/src/providers/llm.rs b/crates/jp_config/src/providers/llm.rs index 6a834f760..d81652bbd 100644 --- a/crates/jp_config/src/providers/llm.rs +++ b/crates/jp_config/src/providers/llm.rs @@ -14,7 +14,7 @@ use schematic::{Config, ConfigError}; use crate::{ assignment::{AssignKeyValue, AssignResult, KvAssignment, missing_key}, - delta::{PartialConfigDelta, delta_map, path}, + delta::{PartialConfigDelta, delta_map, delta_map_with_unsets, path}, fill::{FillDefaults, fill_map}, model::id::{ModelIdConfig, ModelIdConfigError, ModelIdOrAliasConfig, resolve_alias_chain}, partial::ToPartial, @@ -131,7 +131,12 @@ impl PartialConfigDelta for PartialLlmProviderConfig { fn delta_with_unsets(&self, next: Self, prefix: &str, unsets: &mut Vec) -> Self { Self { - aliases: delta_map(&self.aliases, next.aliases), + aliases: delta_map_with_unsets( + &path(prefix, "aliases"), + &self.aliases, + next.aliases, + unsets, + ), anthropic: self.anthropic.delta_with_unsets( next.anthropic, &path(prefix, "anthropic"), diff --git a/crates/jp_config/src/template.rs b/crates/jp_config/src/template.rs index 8174269c5..181a4c1ad 100644 --- a/crates/jp_config/src/template.rs +++ b/crates/jp_config/src/template.rs @@ -5,7 +5,7 @@ use schematic::Config; use crate::{ assignment::{AssignKeyValue, KvAssignment, missing_key}, - delta::PartialConfigDelta, + delta::{PartialConfigDelta, delta_value_map, delta_value_map_with_unsets, path}, fill::FillDefaults, partial::ToPartial, types::json_value::JsonValue, @@ -36,16 +36,18 @@ impl AssignKeyValue for PartialTemplateConfig { impl PartialConfigDelta for PartialTemplateConfig { fn delta(&self, next: Self) -> Self { Self { - values: next - .values - .into_iter() - .filter_map(|(name, next)| { - if self.values.get(&name).is_some_and(|prev| prev == &next) { - return None; - } - Some((name, next)) - }) - .collect(), + values: delta_value_map(&self.values, next.values), + } + } + + fn delta_with_unsets(&self, next: Self, prefix: &str, unsets: &mut Vec) -> Self { + Self { + values: delta_value_map_with_unsets( + &path(prefix, "values"), + &self.values, + next.values, + unsets, + ), } } } From 1697421d41c2c9394e155244fcb781bab9eb461e Mon Sep 17 00:00:00 2001 From: Jean Mertz Date: Mon, 7 Sep 2026 15:11:29 +0200 Subject: [PATCH 03/16] docs(config): Record why compaction rules cannot state a strategy `conversation.compaction.rules` is the one list field whose delta still compares elements rather than saying `replace`, and the reason recorded next to it was wrong. It named the built-in defaults' `discard_when_merged` marker, which is not what stops the field. What stops it is that the field's partial is a bare `MergeableVec`, so an empty one cannot say whether the user asked for no rules or said nothing about them. Every sparse partial that reaches the delta carries the empty one, and replacing with it would record zero rules the user never asked for, which makes a later `jp conversation compact` do nothing. Found by routing the field through the shared helper and reading what broke: `replace` with an empty list written into 37 conversation snapshots, and a config event appended where the suppression path should have left none. Reaching the field needs its partial to become an `Option>`, the shape every converted list field has, where `None` is absent and `Some([])` is a deliberate empty. Signed-off-by: Jean Mertz --- crates/jp_config/src/conversation/compaction.rs | 15 +++++++++++---- crates/jp_config/src/delta_law_tests.rs | 16 +++++++++------- 2 files changed, 20 insertions(+), 11 deletions(-) diff --git a/crates/jp_config/src/conversation/compaction.rs b/crates/jp_config/src/conversation/compaction.rs index 42f7dfd05..f6a3c0d40 100644 --- a/crates/jp_config/src/conversation/compaction.rs +++ b/crates/jp_config/src/conversation/compaction.rs @@ -87,10 +87,17 @@ impl AssignKeyValue for PartialCompactionConfig { impl PartialConfigDelta for PartialCompactionConfig { fn delta(&self, next: Self) -> Self { Self { - // Not `delta_mergeable_vec`: the built-in defaults carry - // `discard_when_merged`, so an empty resolved list and the defaults - // compare unequal while resolving alike, and a replace-with-empty - // delta would be written for no change at all. + // Not `delta_mergeable_vec`, which would say `replace` for any + // difference: this field's partial is a bare `MergeableVec`, so an + // empty one is both "the user said nothing about rules" and "the + // user asked for no rules". A sparse partial (a `--model` override, + // say) carries the empty one, and replacing with it would record + // zero rules the user never asked for, making a later + // `jp conversation compact` a no-op. + // + // Telling the two apart needs the field's partial to be an + // `Option>`, as every converted list field has, + // where `None` is absent and `Some([])` is a deliberate empty. rules: { next.rules .into_iter() diff --git a/crates/jp_config/src/delta_law_tests.rs b/crates/jp_config/src/delta_law_tests.rs index 94e1f984e..ddcd58622 100644 --- a/crates/jp_config/src/delta_law_tests.rs +++ b/crates/jp_config/src/delta_law_tests.rs @@ -81,13 +81,15 @@ fn assert_law(before: &[&str], after: &[&str]) { /// Fields whose clear is known not to survive a fold, and why. /// -/// `conversation.compaction.rules` has built-in defaults carrying -/// `discard_when_merged`, so a resolved empty list and the resolved defaults -/// compare unequal while resolving alike. -/// A delta helper that judged them by their elements would write a -/// replace-with-empty for no change at all, which is how it was found: routing -/// it through [`delta_mergeable_vec`] turned 39 tests red with exactly that -/// noise. +/// `conversation.compaction.rules` keeps its rules in a bare `MergeableVec`, so +/// an empty one cannot say whether the user asked for no rules or said nothing +/// about them. +/// A delta that replaced on any difference would record zero rules from any +/// sparse partial that reached it, which is how it was found: routing it +/// through [`delta_mergeable_vec`] wrote `replace` with an empty list into 37 +/// snapshots and appended an event that should not exist. +/// Reaching it needs the partial to be an `Option>`, as every +/// converted list field has. /// /// `model.parameters.other` is the catch-all arm of its own key-value dispatch, /// so `parameters.other` names a key *inside* the map rather than the map From df36ff576132dcce000c42bdd3721b4a80ec755a Mon Sep 17 00:00:00 2001 From: Jean Mertz Date: Mon, 7 Sep 2026 15:36:49 +0200 Subject: [PATCH 04/16] feat(config, mcp): Let the MCP server map declare its merge strategy `providers.mcp` accepts a strategy the way every other collection field does, so a workspace can drop the servers an outer layer configured rather than merging with them: ```toml [providers.mcp] value = { bookworm = { type = "stdio", command = "just" } } strategy = "replace" ``` The default is unchanged: entries merge by key, so a server added to a later layer joins the ones an earlier layer set rather than replacing them. That is what lets a server added to the workspace config reach a conversation created before it existed, and there is now a test holding that property. Recording a removed server no longer needs a reported path. The map states `replace` and carries the servers the user is left with, which the fold applies without help, so `unsets` is left to the fields that genuinely cannot speak for themselves. The conversation's snapshot of the map merges per key rather than stating `replace`, through the new `map_to_partial_per_key`. Stating `replace` there would drop every server the config files declare and the conversation does not, which is the behaviour the test above catches. Merging per key is safe because each server's own lists already state their strategies, so re-merging the snapshot over the layer it came from reproduces it rather than doubling its arguments. Signed-off-by: Jean Mertz --- crates/jp_cli/src/ctx.rs | 160 +++++++++--------- crates/jp_config/src/lib_tests.rs | 74 +++++++- crates/jp_config/src/providers.rs | 37 ++-- ...ig__tests__partial_app_config_default.snap | 4 +- ...ts__partial_app_config_default_values.snap | 4 +- ...s__partial_app_config_empty_serialize.snap | 4 +- crates/jp_config/src/types/map.rs | 17 ++ 7 files changed, 195 insertions(+), 105 deletions(-) diff --git a/crates/jp_cli/src/ctx.rs b/crates/jp_cli/src/ctx.rs index 7ecbf6ab9..6d1dfe17d 100644 --- a/crates/jp_cli/src/ctx.rs +++ b/crates/jp_cli/src/ctx.rs @@ -118,61 +118,61 @@ pub(crate) struct Term { impl Ctx { /// Create a new context with the given workspace pub(crate) fn new( - exec: ExecutionContext, - workspace: Workspace, - fs_backend: Option>, - runtime: Runtime, - args: Globals, - config: impl Into>, - session: Option, - printer: Printer, - ) -> Self { - let config = config.into(); - let escalation_cooldown = - Duration::from_secs(config.interrupt.escalation_cooldown_secs.into()); - let mcp_client = jp_mcp::Client::new(config.providers.mcp.clone()) - .with_child_cwd(exec.child_cwd().map(|cwd| cwd.as_std_path().to_path_buf())); - - let is_tty = io::stdout().is_terminal(); - let width = printer.output_width().columns(); - - let interactive = crate::interactive(args.no_interactive, is_tty); - - Self { - exec, - workspace, - fs_backend, - config, - term: Term { - args, - is_tty, - interactive, - width, - }, - session, - printer: Arc::new(printer), - mcp_client, - task_handler: TaskHandler::default(), - signals: SignalRouter::new(&runtime, escalation_cooldown), - config_reset: None, - runtime, - - #[cfg(test)] - stubbed_now: DateTime::::UNIX_EPOCH, - } + exec: ExecutionContext, + workspace: Workspace, + fs_backend: Option>, + runtime: Runtime, + args: Globals, + config: impl Into>, + session: Option, + printer: Printer, +) -> Self { + let config = config.into(); + let escalation_cooldown = + Duration::from_secs(config.interrupt.escalation_cooldown_secs.into()); + let mcp_client = jp_mcp::Client::new(config.providers.mcp.clone().into_map()) + .with_child_cwd(exec.child_cwd().map(|cwd| cwd.as_std_path().to_path_buf())); + + let is_tty = io::stdout().is_terminal(); + let width = printer.output_width().columns(); + + let interactive = crate::interactive(args.no_interactive, is_tty); + + Self { + exec, + workspace, + fs_backend, + config, + term: Term { + args, + is_tty, + interactive, + width, + }, + session, + printer: Arc::new(printer), + mcp_client, + task_handler: TaskHandler::default(), + signals: SignalRouter::new(&runtime, escalation_cooldown), + config_reset: None, + runtime, + + #[cfg(test)] + stubbed_now: DateTime::::UNIX_EPOCH, } +} #[cfg(not(test))] #[expect(clippy::unused_self)] pub(crate) fn now(&self) -> DateTime { Utc::now() } - - #[cfg(test)] pub(crate) fn now(&self) -> DateTime { self.stubbed_now } + #[cfg(test)] + #[cfg(test)] pub(crate) fn set_now(&mut self, now: DateTime) { self.stubbed_now = now; @@ -180,17 +180,17 @@ impl Ctx { /// Returns the storage path, if filesystem storage is configured. pub(crate) fn storage_path(&self) -> Option<&Utf8Path> { - self.fs_backend - .as_deref() - .map(FsStorageBackend::storage_path) - } + self.fs_backend + .as_deref() + .map(FsStorageBackend::storage_path) +} /// Returns the user storage path, if filesystem storage is configured. pub(crate) fn user_storage_path(&self) -> Option<&Utf8Path> { - self.fs_backend - .as_deref() - .and_then(FsStorageBackend::user_storage_path) - } + self.fs_backend + .as_deref() + .and_then(FsStorageBackend::user_storage_path) +} /// Get immutable access to the configuration. /// @@ -203,8 +203,8 @@ impl Ctx { /// configuration" API in [`jp_config`] *before* constructing the final /// [`AppConfig`] object. pub(crate) fn config(&self) -> Arc { - self.config.clone() - } + self.config.clone() +} /// Install a resolved config for one scoped run, returning the previous /// one. @@ -220,13 +220,13 @@ impl Ctx { /// rather than a mutation: assembling a config is still the partial API's /// job. pub(crate) fn swap_config(&mut self, config: Arc) -> Arc { - std::mem::replace(&mut self.config, config) - } + std::mem::replace(&mut self.config, config) +} /// Get a runtime handle. pub(crate) fn handle(&self) -> &Handle { - self.runtime.handle() - } + self.runtime.handle() +} /// Activate and deactivate MCP servers based on the active conversation /// context. @@ -237,31 +237,31 @@ impl Ctx { /// or `tool_definitions` drops the tool again as unreachable and the forced /// choice cannot be satisfied. pub(crate) async fn configure_active_mcp_servers( - &mut self, - forced_tool: Option<&str>, - scope: McpServerScope, - ) -> Result { - let mut server_ids = HashSet::new(); - - for (name, cfg) in self.config.conversation.tools.iter() { - if !cfg.is_enabled() && forced_tool != Some(name) { - continue; - } - - let ToolSource::Mcp { server, .. } = &cfg.source() else { - continue; - }; - - server_ids.insert(McpServerId::new(server)); + &mut self, + forced_tool: Option<&str>, + scope: McpServerScope, +) -> Result { + let mut server_ids = HashSet::new(); + + for (name, cfg) in self.config.conversation.tools.iter() { + if !cfg.is_enabled() && forced_tool != Some(name) { + continue; } - let handle = self.handle().clone(); - match scope { - McpServerScope::Exclusive => self.mcp_client.run_services(server_ids, handle).await, - McpServerScope::Shared => self.mcp_client.start_services(server_ids, handle).await, - } - .map_err(Into::into) + let ToolSource::Mcp { server, .. } = &cfg.source() else { + continue; + }; + + server_ids.insert(McpServerId::new(server)); } + + let handle = self.handle().clone(); + match scope { + McpServerScope::Exclusive => self.mcp_client.run_services(server_ids, handle).await, + McpServerScope::Shared => self.mcp_client.start_services(server_ids, handle).await, + } + .map_err(Into::into) +} } /// Whether a turn has the MCP client to itself. diff --git a/crates/jp_config/src/lib_tests.rs b/crates/jp_config/src/lib_tests.rs index 3be3413f4..f881ba760 100644 --- a/crates/jp_config/src/lib_tests.rs +++ b/crates/jp_config/src/lib_tests.rs @@ -775,10 +775,10 @@ fn a_dropped_mcp_argument_is_recorded() { /// A server the user removed is recorded, so the conversation stops starting /// it. /// -/// Entries merge by key, so no value a delta carries can take one away: the key -/// survives from the previous layer. -/// The entry's path is reported instead, and the fold removes it before -/// merging. +/// Entries merge by key, which is what lets a server the workspace config +/// gained reach a conversation created before it existed. +/// That same property means a deep merge would resurrect a removed one, so the +/// delta states `replace` and carries the map the user is left with. #[test] fn a_removed_mcp_server_is_recorded() { use crate::providers::mcp::{McpProviderConfig, StdioConfig}; @@ -804,13 +804,16 @@ fn a_removed_mcp_server_is_recorded() { .to_partial() .delta_with_unsets(next.to_partial(), "", &mut unsets); - assert_eq!(unsets, ["providers.mcp.bookworm"]); + assert!( + unsets.is_empty(), + "the map states its own strategy, so no path is reported: {unsets:?}" + ); + assert!( + delta.providers.mcp.discard_when_merged() || !delta.providers.mcp.is_empty(), + "the delta carries the map the user is left with" + ); - // Applying the report and then the delta reaches the config the user has. let mut folded = prev.to_partial(); - folded - .unset("providers.mcp.bookworm") - .expect("a real field"); folded.merge(&(), delta).expect("folding cannot fail"); assert!( @@ -823,6 +826,59 @@ fn a_removed_mcp_server_is_recorded() { ); } +/// A server only the workspace config declares reaches an existing +/// conversation. +/// +/// The conversation layer is a resolved snapshot merged over the layer built +/// from the config files. +/// Stating `replace` on that snapshot would drop every server the files declare +/// and the conversation does not, so it merges per key instead. +#[test] +fn a_server_added_to_the_workspace_reaches_an_existing_conversation() { + use schematic::PartialConfig as _; + + use crate::providers::mcp::{McpProviderConfig, StdioConfig}; + + let server = |command: &str| { + McpProviderConfig::Stdio(StdioConfig { + command: command.into(), + arguments: vec![], + variables: vec![], + checksum: None, + optional: false, + startup_timeout_secs: 60, + }) + }; + + // The conversation was created knowing only `bookworm`. + let mut conversation = AppConfig::new_test(); + conversation + .providers + .mcp + .insert("bookworm".to_owned(), server("just")); + + // The workspace config has since gained `kagi`. + let mut files = PartialAppConfig::new_test(); + files + .providers + .mcp + .insert("kagi".to_owned(), server("kagi").to_partial()); + + files + .merge(&(), conversation.to_partial()) + .expect("merging cannot fail"); + let resolved = crate::util::build(files).expect("valid config"); + + assert!( + resolved.providers.mcp.contains_key("kagi"), + "a server only the files declare survives the conversation layer" + ); + assert!( + resolved.providers.mcp.contains_key("bookworm"), + "the conversation's own server survives too" + ); +} + /// A union that names an expanded form contributes both the shorthand path and /// the expanded keys; a union of distinct values contributes only its path. /// diff --git a/crates/jp_config/src/providers.rs b/crates/jp_config/src/providers.rs index 41b9e7b2b..0fdc6b2a2 100644 --- a/crates/jp_config/src/providers.rs +++ b/crates/jp_config/src/providers.rs @@ -3,20 +3,20 @@ pub mod llm; pub mod mcp; -use indexmap::IndexMap; use schematic::{Config, ConfigError}; use crate::{ assignment::{AssignKeyValue, AssignResult, KvAssignment, missing_key}, - delta::{PartialConfigDelta, delta_map, delta_map_with_unsets, path}, + delta::{PartialConfigDelta, delta_mergeable_map, path}, fill::{FillDefaults, fill_map}, + internal::merge::map_with_strategy, partial::ToPartial, providers::{ llm::{LlmProviderConfig, PartialLlmProviderConfig}, mcp::McpProviderConfig, }, - util::merge_nested_indexmap, validate::Validator, + types::map::{MergeableMap, map_to_partial_per_key}, }; /// Provider configuration. @@ -44,8 +44,10 @@ pub struct ProviderConfig { /// /// Entries merge by key, so a server added to a later layer joins the ones /// an earlier layer configured rather than replacing them. - #[setting(nested, merge = merge_nested_indexmap)] - pub mcp: IndexMap, + /// Declare the map as `{ value = { … }, strategy = "replace" }` to drop + /// them instead. + #[setting(nested, merge = map_with_strategy)] + pub mcp: MergeableMap, } impl Validator for ProviderConfig { @@ -75,7 +77,7 @@ impl PartialConfigDelta for PartialProviderConfig { fn delta(&self, next: Self) -> Self { Self { llm: self.llm.delta(next.llm), - mcp: delta_map(&self.mcp, next.mcp), + mcp: delta_mergeable_map(&self.mcp, next.mcp), } } @@ -84,7 +86,9 @@ impl PartialConfigDelta for PartialProviderConfig { llm: self .llm .delta_with_unsets(next.llm, &path(prefix, "llm"), unsets), - mcp: delta_map_with_unsets(&path(prefix, "mcp"), &self.mcp, next.mcp, unsets), + // The map states its own strategy, so a removed server travels in + // the value as a `replace` and needs no path reported. + mcp: delta_mergeable_map(&self.mcp, next.mcp), } } } @@ -93,7 +97,16 @@ impl FillDefaults for PartialProviderConfig { fn fill_from(self, defaults: Self) -> Self { Self { llm: self.llm.fill_from(defaults.llm), - mcp: fill_map(self.mcp, defaults.mcp), + // Key by key, so a server only the defaults declare is added + // while one this layer already has keeps its own value. A map + // that states a strategy is left alone: its owner said how it + // combines, and filling gaps into it would answer differently. + mcp: match self.mcp { + merged @ MergeableMap::Merged(_) => merged, + MergeableMap::Map(entries) => { + fill_map(entries, defaults.mcp.into_map()).into() + } + }, } } } @@ -102,11 +115,9 @@ impl ToPartial for ProviderConfig { fn to_partial(&self) -> Self::Partial { Self::Partial { llm: self.llm.to_partial(), - mcp: self - .mcp - .iter() - .map(|(k, v)| (k.clone(), v.to_partial())) - .collect(), + // Per key rather than `replace`: a server the workspace config + // gained after this conversation was created still reaches it. + mcp: map_to_partial_per_key(self.mcp.iter()), } } } diff --git a/crates/jp_config/src/snapshots/jp_config__tests__partial_app_config_default.snap b/crates/jp_config/src/snapshots/jp_config__tests__partial_app_config_default.snap index 16521dbf1..2ef03dd8a 100644 --- a/crates/jp_config/src/snapshots/jp_config__tests__partial_app_config_default.snap +++ b/crates/jp_config/src/snapshots/jp_config__tests__partial_app_config_default.snap @@ -253,7 +253,9 @@ PartialAppConfig { base_url: None, }, }, - mcp: {}, + mcp: Map( + {}, + ), }, plugins: PartialPluginsConfig { auto_install: None, 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 014fb57ad..ed0bfb419 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 @@ -525,7 +525,9 @@ Ok( ), }, }, - mcp: {}, + mcp: Map( + {}, + ), }, plugins: PartialPluginsConfig { auto_install: Some( diff --git a/crates/jp_config/src/snapshots/jp_config__tests__partial_app_config_empty_serialize.snap b/crates/jp_config/src/snapshots/jp_config__tests__partial_app_config_empty_serialize.snap index a9220c267..7b89fcf17 100644 --- a/crates/jp_config/src/snapshots/jp_config__tests__partial_app_config_empty_serialize.snap +++ b/crates/jp_config/src/snapshots/jp_config__tests__partial_app_config_empty_serialize.snap @@ -253,7 +253,9 @@ PartialAppConfig { base_url: None, }, }, - mcp: {}, + mcp: Map( + {}, + ), }, plugins: PartialPluginsConfig { auto_install: None, diff --git a/crates/jp_config/src/types/map.rs b/crates/jp_config/src/types/map.rs index 8553db23f..d490bb6c8 100644 --- a/crates/jp_config/src/types/map.rs +++ b/crates/jp_config/src/types/map.rs @@ -182,6 +182,23 @@ pub fn map_to_mergeable_partial<'a, T: ToPartial + 'a>( }) } +/// Convert a resolved map to a `MergeableMap` that merges per key. +/// +/// Used by `ToPartial` impls for a map that should still take an entry a later +/// layer adds, which a `replace` strategy would drop. +/// Re-merging the result over the layer it came from reproduces it rather than +/// combining with it, because each entry's own fields state their strategies. +pub fn map_to_partial_per_key<'a, T: ToPartial + 'a>( + entries: impl IntoIterator, +) -> MergeableMap { + MergeableMap::Map( + entries + .into_iter() + .map(|(k, v)| (k.clone(), v.to_partial())) + .collect(), + ) +} + impl From> for MergeableMap { fn from(value: IndexMap) -> Self { Self::Map(value) From 4db59f436b419633ee046df7db7748f6a65c4d9d Mon Sep 17 00:00:00 2001 From: Jean Mertz Date: Mon, 7 Sep 2026 16:06:35 +0200 Subject: [PATCH 05/16] feat(config): Let plugin, alias and tool maps declare their strategy `plugins.command`, `providers.llm.aliases`, and a tool's `parameters`, `questions` and nested `properties` accept a strategy the way every other collection field does: ```toml [providers.llm.aliases] value = { opus = "anthropic/claude-opus-4" } strategy = "replace" ``` The default is unchanged: entries merge by key, so an alias, plugin or parameter configured in a later layer joins the ones an earlier layer set. That is what lets a workspace config gain an entry and have it reach a conversation created before it existed. Recording a removed entry no longer needs a reported path. Each map states `replace` and carries the entries the user is left with, which the fold applies on its own. `unsets` is left to the fields that genuinely cannot speak for themselves, and `PartialToolParameterConfig` no longer needs a path-reporting delta at all. Each conversation snapshot merges per key rather than stating `replace`, so an entry the files declare and the conversation does not survives the layering. Signed-off-by: Jean Mertz --- crates/jp_cli/src/cmd/query/tool/builtins.rs | 3 +- .../src/cmd/query/tool/coordinator_tests.rs | 6 +- .../jp_cli/src/cmd/query/turn_loop_tests.rs | 82 +++++++------- crates/jp_config/src/conversation/tool.rs | 106 ++++++++---------- crates/jp_config/src/plugins.rs | 51 ++++----- crates/jp_config/src/providers/llm.rs | 43 ++++--- ...ig__tests__partial_app_config_default.snap | 8 +- ...ts__partial_app_config_default_values.snap | 8 +- ...s__partial_app_config_empty_serialize.snap | 8 +- crates/jp_conversation/src/compat.rs | 47 ++++++++ crates/jp_llm/src/tool/json_schema.rs | 2 +- 11 files changed, 207 insertions(+), 157 deletions(-) diff --git a/crates/jp_cli/src/cmd/query/tool/builtins.rs b/crates/jp_cli/src/cmd/query/tool/builtins.rs index 111f64e9a..d30a4a246 100644 --- a/crates/jp_cli/src/cmd/query/tool/builtins.rs +++ b/crates/jp_cli/src/cmd/query/tool/builtins.rs @@ -30,7 +30,8 @@ pub fn describe_tools() -> PartialToolConfig { ..Default::default() })), ..Default::default() - })]), + })]) + .into(), run: Some(RunMode::Unattended), style: Some(PartialDisplayStyleConfig { hidden: Some(true), diff --git a/crates/jp_cli/src/cmd/query/tool/coordinator_tests.rs b/crates/jp_cli/src/cmd/query/tool/coordinator_tests.rs index 6791930a6..0cb1b65ce 100644 --- a/crates/jp_cli/src/cmd/query/tool/coordinator_tests.rs +++ b/crates/jp_cli/src/cmd/query/tool/coordinator_tests.rs @@ -190,7 +190,8 @@ fn test_question_target_with_configured_question() { target: Some(QuestionTarget::Assistant(Box::default())), answer: None, } - }, + } + .into(), ..Default::default() }, vec![], @@ -247,7 +248,8 @@ fn test_static_answer_with_configured_answer() { target: Some(QuestionTarget::User), answer: None, } - }, + } + .into(), ..Default::default() }, vec![], diff --git a/crates/jp_cli/src/cmd/query/turn_loop_tests.rs b/crates/jp_cli/src/cmd/query/turn_loop_tests.rs index 21ddaba91..57a47658c 100644 --- a/crates/jp_cli/src/cmd/query/turn_loop_tests.rs +++ b/crates/jp_cli/src/cmd/query/turn_loop_tests.rs @@ -1390,10 +1390,10 @@ async fn test_tool_interrupt_menu_cancel_escalates() { summary: None, description: None, examples: None, - parameters: IndexMap::new(), + parameters: IndexMap::new().into(), result: None, style: None, - questions: IndexMap::new(), + questions: IndexMap::new().into(), options: IndexMap::default(), access: None, cancellation_response: None, @@ -1539,10 +1539,10 @@ async fn test_tool_stop_on_interrupt_commits_responses_without_follow_up() { summary: None, description: None, examples: None, - parameters: IndexMap::new(), + parameters: IndexMap::new().into(), result: None, style: None, - questions: IndexMap::new(), + questions: IndexMap::new().into(), options: IndexMap::default(), access: None, cancellation_response: Some(CUSTOM_CANCELLATION_RESPONSE.to_string()), @@ -1679,13 +1679,14 @@ async fn test_interrupt_during_tool_prompt_completes_turn_early() { summary: None, description: None, examples: None, - parameters: IndexMap::new(), + parameters: IndexMap::new().into(), result: None, style: None, questions: IndexMap::from_iter([("confirm".to_string(), QuestionConfig { target: QuestionTarget::User, answer: None, - })]), + })]) + .into(), options: IndexMap::default(), access: None, cancellation_response: None, @@ -2021,10 +2022,10 @@ async fn test_tool_restart_on_interrupt() { summary: None, description: None, examples: None, - parameters: IndexMap::new(), + parameters: IndexMap::new().into(), result: None, style: None, - questions: IndexMap::new(), + questions: IndexMap::new().into(), options: IndexMap::default(), access: None, cancellation_response: None, @@ -2177,10 +2178,10 @@ async fn test_merged_stream_exits_after_tool_response() { summary: None, description: None, examples: None, - parameters: IndexMap::new(), + parameters: IndexMap::new().into(), result: None, style: None, - questions: IndexMap::new(), + questions: IndexMap::new().into(), options: IndexMap::default(), access: None, cancellation_response: None, @@ -2290,10 +2291,10 @@ async fn test_tool_call_with_run_mode_ask_approves() { summary: None, description: None, examples: None, - parameters: IndexMap::new(), + parameters: IndexMap::new().into(), result: None, style: None, - questions: IndexMap::new(), + questions: IndexMap::new().into(), options: IndexMap::default(), access: None, cancellation_response: None, @@ -2434,10 +2435,10 @@ async fn test_tool_call_with_run_mode_ask_skips() { summary: None, description: None, examples: None, - parameters: IndexMap::new(), + parameters: IndexMap::new().into(), result: None, style: None, - questions: IndexMap::new(), + questions: IndexMap::new().into(), options: IndexMap::default(), access: None, cancellation_response: None, @@ -2589,10 +2590,10 @@ async fn test_permission_prompt_follows_interactive_not_is_tty() { summary: None, description: None, examples: None, - parameters: IndexMap::new(), + parameters: IndexMap::new().into(), result: None, style: None, - questions: IndexMap::new(), + questions: IndexMap::new().into(), options: IndexMap::default(), access: None, cancellation_response: None, @@ -2716,10 +2717,10 @@ async fn test_tool_call_with_run_mode_unattended() { summary: None, description: None, examples: None, - parameters: IndexMap::new(), + parameters: IndexMap::new().into(), result: None, style: None, - questions: IndexMap::new(), + questions: IndexMap::new().into(), options: IndexMap::default(), access: None, cancellation_response: None, @@ -2855,10 +2856,10 @@ async fn test_tool_call_with_run_mode_skip() { summary: None, description: None, examples: None, - parameters: IndexMap::new(), + parameters: IndexMap::new().into(), result: None, style: None, - questions: IndexMap::new(), + questions: IndexMap::new().into(), options: IndexMap::default(), access: None, cancellation_response: None, @@ -3010,10 +3011,10 @@ async fn test_multiple_tools_with_different_run_modes() { summary: None, description: None, examples: None, - parameters: IndexMap::new(), + parameters: IndexMap::new().into(), result: None, style: None, - questions: IndexMap::new(), + questions: IndexMap::new().into(), options: IndexMap::default(), access: None, cancellation_response: None, @@ -3031,10 +3032,10 @@ async fn test_multiple_tools_with_different_run_modes() { summary: None, description: None, examples: None, - parameters: IndexMap::new(), + parameters: IndexMap::new().into(), result: None, style: None, - questions: IndexMap::new(), + questions: IndexMap::new().into(), options: IndexMap::default(), access: None, cancellation_response: None, @@ -3219,10 +3220,10 @@ async fn test_tool_call_returns_error() { summary: None, description: None, examples: None, - parameters: IndexMap::new(), + parameters: IndexMap::new().into(), result: None, style: None, - questions: IndexMap::new(), + questions: IndexMap::new().into(), options: IndexMap::default(), access: None, cancellation_response: None, @@ -4526,10 +4527,10 @@ async fn test_parallel_tool_calls_rendered_atomically() { summary: None, description: None, examples: None, - parameters: IndexMap::new(), + parameters: IndexMap::new().into(), result: None, style: fn_call_style.clone(), - questions: IndexMap::new(), + questions: IndexMap::new().into(), options: IndexMap::default(), access: None, cancellation_response: None, @@ -4546,10 +4547,10 @@ async fn test_parallel_tool_calls_rendered_atomically() { summary: None, description: None, examples: None, - parameters: IndexMap::new(), + parameters: IndexMap::new().into(), result: None, style: fn_call_style, - questions: IndexMap::new(), + questions: IndexMap::new().into(), options: IndexMap::default(), access: None, cancellation_response: None, @@ -4719,10 +4720,10 @@ async fn test_single_tool_call_rendered_with_args() { summary: None, description: None, examples: None, - parameters: IndexMap::new(), + parameters: IndexMap::new().into(), result: None, style: None, - questions: IndexMap::new(), + questions: IndexMap::new().into(), options: IndexMap::default(), access: None, cancellation_response: None, @@ -5990,7 +5991,7 @@ fn inquiry_tool_config(questions: &[&str]) -> ToolConfig { summary: None, description: None, examples: None, - parameters: IndexMap::new(), + parameters: IndexMap::new().into(), result: None, style: None, questions: questions @@ -6001,7 +6002,8 @@ fn inquiry_tool_config(questions: &[&str]) -> ToolConfig { answer: None, }) }) - .collect(), + .collect::>() + .into(), options: IndexMap::default(), access: None, cancellation_response: None, @@ -7108,10 +7110,10 @@ async fn test_parallel_tools_one_with_inquiry() { summary: None, description: None, examples: None, - parameters: IndexMap::new(), + parameters: IndexMap::new().into(), result: None, style: None, - questions: IndexMap::new(), + questions: IndexMap::new().into(), options: IndexMap::default(), access: None, cancellation_response: None, @@ -7543,10 +7545,10 @@ async fn test_unavailable_tool_before_approved_does_not_panic() { summary: None, description: None, examples: None, - parameters: IndexMap::new(), + parameters: IndexMap::new().into(), result: None, style: None, - questions: IndexMap::new(), + questions: IndexMap::new().into(), options: IndexMap::default(), access: None, cancellation_response: None, @@ -7975,10 +7977,10 @@ async fn reasoning_before_a_tool_call_shades_the_tool_chrome() { summary: None, description: None, examples: None, - parameters: IndexMap::new(), + parameters: IndexMap::new().into(), result: None, style: None, - questions: IndexMap::new(), + questions: IndexMap::new().into(), options: IndexMap::default(), access: None, cancellation_response: None, diff --git a/crates/jp_config/src/conversation/tool.rs b/crates/jp_config/src/conversation/tool.rs index 1cc01da97..0f36427ff 100644 --- a/crates/jp_config/src/conversation/tool.rs +++ b/crates/jp_config/src/conversation/tool.rs @@ -18,13 +18,17 @@ use crate::{ style::{DisplayStyleConfig, PartialDisplayStyleConfig}, }, delta::{ - PartialConfigDelta, delta_map, delta_map_with_unsets, delta_opt, delta_opt_at, - delta_opt_partial, delta_opt_partial_at, delta_value_map, delta_value_map_with_unsets, - delta_vec, path, + PartialConfigDelta, delta_map, delta_map_with_unsets, delta_mergeable_map, delta_opt, + delta_opt_at, delta_opt_partial, delta_opt_partial_at, delta_value_map, + delta_value_map_with_unsets, delta_vec, path, }, fill::{FillDefaults, fill_map}, + internal::merge::map_with_strategy, partial::{ToPartial, partial_opt, partial_opt_config, partial_opts}, - types::json_value::JsonValue, + types::{ + json_value::JsonValue, + map::{MergeableMap, map_to_partial_per_key}, + }, util::merge_nested_indexmap, validate::Validator, }; @@ -510,8 +514,13 @@ pub struct ToolConfig { /// values, or forcing a specific value by setting a single enum value. /// You CANNOT change the type of the argument, its name, or any other /// properties that would break the tool's original argument expectations. - #[setting(nested, merge = merge_nested_indexmap)] - pub parameters: IndexMap, + /// + /// Entries merge by key, so a parameter narrowed in a later layer joins the + /// ones an earlier layer set. + /// Declare the map as `{ value = { … }, strategy = "replace" }` to drop + /// them instead. + #[setting(nested, merge = map_with_strategy)] + pub parameters: MergeableMap, /// How to run the tool. /// @@ -557,8 +566,13 @@ pub struct ToolConfig { /// documented by the tool. /// For example, `fs_create_file` uses `overwrite_file` when a file already /// exists. - #[setting(nested, merge = merge_nested_indexmap)] - pub questions: IndexMap, + /// + /// Entries merge by key, so a question configured in a later layer joins + /// the ones an earlier layer set. + /// Declare the map as `{ value = { … }, strategy = "replace" }` to drop + /// them instead. + #[setting(nested, merge = map_with_strategy)] + pub questions: MergeableMap, /// Per-tool options passed to the tool at runtime. /// @@ -616,7 +630,7 @@ impl PartialConfigDelta for PartialToolConfig { summary: delta_opt(self.summary.as_ref(), next.summary), description: delta_opt(self.description.as_ref(), next.description), examples: delta_opt(self.examples.as_ref(), next.examples), - parameters: delta_map(&self.parameters, next.parameters), + parameters: delta_mergeable_map(&self.parameters, next.parameters), run: delta_opt(self.run.as_ref(), next.run), format: delta_opt(self.format.as_ref(), next.format), result: delta_opt(self.result.as_ref(), next.result), @@ -625,7 +639,7 @@ impl PartialConfigDelta for PartialToolConfig { next.cancellation_response, ), style: delta_opt_partial(self.style.as_ref(), next.style), - questions: delta_map(&self.questions, next.questions), + questions: delta_mergeable_map(&self.questions, next.questions), options: delta_value_map(&self.options, next.options), access: delta_opt_partial(self.access.as_ref(), next.access), } @@ -649,12 +663,9 @@ impl PartialConfigDelta for PartialToolConfig { summary: delta_opt(self.summary.as_ref(), next.summary), description: delta_opt(self.description.as_ref(), next.description), examples: delta_opt(self.examples.as_ref(), next.examples), - parameters: delta_map_with_unsets( - &path(prefix, "parameters"), - &self.parameters, - next.parameters, - unsets, - ), + // Each map states its own strategy, so a removed entry travels in + // the value as a `replace` and needs no path reported. + parameters: delta_mergeable_map(&self.parameters, next.parameters), run: delta_opt(self.run.as_ref(), next.run), format: delta_opt(self.format.as_ref(), next.format), result: delta_opt(self.result.as_ref(), next.result), @@ -668,12 +679,7 @@ impl PartialConfigDelta for PartialToolConfig { next.style, unsets, ), - questions: delta_map_with_unsets( - &path(prefix, "questions"), - &self.questions, - next.questions, - unsets, - ), + questions: delta_mergeable_map(&self.questions, next.questions), options: delta_value_map_with_unsets( &path(prefix, "options"), &self.options, @@ -701,11 +707,9 @@ impl ToPartial for ToolConfig { summary: partial_opts(self.summary.as_ref(), defaults.summary), description: partial_opts(self.description.as_ref(), defaults.description), examples: partial_opts(self.examples.as_ref(), defaults.examples), - parameters: self - .parameters - .iter() - .map(|(k, v)| (k.clone(), v.to_partial())) - .collect(), + // Per key rather than `replace`: an entry a later layer adds still + // reaches a conversation created before it existed. + parameters: map_to_partial_per_key(self.parameters.iter()), run: partial_opts(self.run.as_ref(), defaults.run), format: partial_opts(self.format.as_ref(), defaults.format), result: partial_opts(self.result.as_ref(), defaults.result), @@ -714,11 +718,7 @@ impl ToPartial for ToolConfig { defaults.cancellation_response, ), style: partial_opt_config(self.style.as_ref(), defaults.style), - questions: self - .questions - .iter() - .map(|(k, v)| (k.clone(), v.to_partial())) - .collect(), + questions: map_to_partial_per_key(self.questions.iter()), options: self .options .iter() @@ -807,10 +807,15 @@ pub struct ToolParameterConfig { /// MCP properties are merged by name. /// Entries here may narrow nested fields or add fields to local and /// built-in object parameters. - #[setting(nested, merge = merge_nested_indexmap)] - #[serde(default, skip_serializing_if = "IndexMap::is_empty")] + /// + /// Entries merge by key, so a property narrowed in a later layer joins the + /// ones an earlier layer set. + /// Declare the map as `{ value = { … }, strategy = "replace" }` to drop + /// them instead. + #[setting(nested, merge = map_with_strategy)] + #[serde(default, skip_serializing_if = "MergeableMap::is_empty")] #[expect(clippy::use_self, reason = "macro can't resolve `Self`")] - pub properties: IndexMap, + pub properties: MergeableMap, } impl PartialConfigDelta for PartialToolParameterConfig { @@ -826,26 +831,7 @@ impl PartialConfigDelta for PartialToolParameterConfig { // any element has to record the whole list. enumeration: delta_opt(self.enumeration.as_ref(), next.enumeration), items: delta_opt(self.items.as_ref(), next.items), - properties: delta_map(&self.properties, next.properties), - } - } - - fn delta_with_unsets(&self, next: Self, prefix: &str, unsets: &mut Vec) -> Self { - Self { - kind: delta_opt_partial(self.kind.as_ref(), next.kind), - default: delta_opt(self.default.as_ref(), next.default), - required: delta_opt(self.required.as_ref(), next.required), - summary: delta_opt(self.summary.as_ref(), next.summary), - description: delta_opt(self.description.as_ref(), next.description), - examples: delta_opt(self.examples.as_ref(), next.examples), - enumeration: delta_opt(self.enumeration.as_ref(), next.enumeration), - items: delta_opt(self.items.as_ref(), next.items), - properties: delta_map_with_unsets( - &path(prefix, "properties"), - &self.properties, - next.properties, - unsets, - ), + properties: delta_mergeable_map(&self.properties, next.properties), } } } @@ -863,11 +849,7 @@ impl ToPartial for ToolParameterConfig { examples: partial_opts(self.examples.as_ref(), defaults.examples), enumeration: self.enumeration.clone(), items: self.items.as_ref().map(|v| Box::new(v.to_partial())), - properties: self - .properties - .iter() - .map(|(k, v)| (k.clone(), v.to_partial())) - .collect(), + properties: map_to_partial_per_key(self.properties.iter()), } } } @@ -1241,7 +1223,7 @@ impl ToolConfigWithDefaults { /// Return the parameters of the tool. #[must_use] - pub const fn parameters(&self) -> &IndexMap { + pub fn parameters(&self) -> &IndexMap { &self.tool.parameters } @@ -1324,7 +1306,7 @@ impl ToolConfigWithDefaults { /// Return the questions configuration of the tool. #[must_use] - pub const fn questions(&self) -> &IndexMap { + pub fn questions(&self) -> &IndexMap { &self.tool.questions } diff --git a/crates/jp_config/src/plugins.rs b/crates/jp_config/src/plugins.rs index ea3355de1..27ec6c238 100644 --- a/crates/jp_config/src/plugins.rs +++ b/crates/jp_config/src/plugins.rs @@ -6,17 +6,17 @@ pub mod command; -use indexmap::IndexMap; use schematic::Config; use crate::{ FillDefaults, assignment::{AssignKeyValue, AssignResult, KvAssignment, missing_key}, - delta::{PartialConfigDelta, delta_map, delta_map_with_unsets, delta_opt, path}, + delta::{PartialConfigDelta, delta_mergeable_map, delta_opt}, fill::fill_map, + internal::merge::map_with_strategy, partial::ToPartial, plugins::command::CommandPluginConfig, - util::merge_nested_indexmap, + types::map::{MergeableMap, map_to_partial_per_key}, }; /// Plugin configuration. @@ -33,8 +33,13 @@ pub struct PluginsConfig { pub shutdown_timeout_secs: u16, /// Command plugin configurations, keyed by plugin name (e.g. `serve`). - #[setting(nested, merge = merge_nested_indexmap)] - pub command: IndexMap, + /// + /// Entries merge by key, so a plugin configured in a later layer joins the + /// ones an earlier layer set. + /// Declare the map as `{ value = { … }, strategy = "replace" }` to drop + /// them instead. + #[setting(nested, merge = map_with_strategy)] + pub command: MergeableMap, } impl AssignKeyValue for PartialPluginsConfig { @@ -62,23 +67,7 @@ impl PartialConfigDelta for PartialPluginsConfig { self.shutdown_timeout_secs.as_ref(), next.shutdown_timeout_secs, ), - command: delta_map(&self.command, next.command), - } - } - - fn delta_with_unsets(&self, next: Self, prefix: &str, unsets: &mut Vec) -> Self { - Self { - auto_install: delta_opt(self.auto_install.as_ref(), next.auto_install), - shutdown_timeout_secs: delta_opt( - self.shutdown_timeout_secs.as_ref(), - next.shutdown_timeout_secs, - ), - command: delta_map_with_unsets( - &path(prefix, "command"), - &self.command, - next.command, - unsets, - ), + command: delta_mergeable_map(&self.command, next.command), } } } @@ -90,7 +79,15 @@ impl FillDefaults for PartialPluginsConfig { shutdown_timeout_secs: self .shutdown_timeout_secs .or(defaults.shutdown_timeout_secs), - command: fill_map(self.command, defaults.command), + // Key by key, so a plugin only the defaults declare is added + // while one this layer already has keeps its own value. A map + // that states a strategy is left alone. + command: match self.command { + merged @ MergeableMap::Merged(_) => merged, + MergeableMap::Map(entries) => { + fill_map(entries, defaults.command.into_map()).into() + } + }, } } } @@ -105,11 +102,9 @@ impl ToPartial for PluginsConfig { &self.shutdown_timeout_secs, defaults.shutdown_timeout_secs, ), - command: self - .command - .iter() - .map(|(k, v)| (k.clone(), v.to_partial())) - .collect(), + // Per key rather than `replace`: a plugin the workspace config + // gained after this conversation was created still reaches it. + command: map_to_partial_per_key(self.command.iter()), } } } diff --git a/crates/jp_config/src/providers/llm.rs b/crates/jp_config/src/providers/llm.rs index d81652bbd..02b73d596 100644 --- a/crates/jp_config/src/providers/llm.rs +++ b/crates/jp_config/src/providers/llm.rs @@ -14,8 +14,9 @@ use schematic::{Config, ConfigError}; use crate::{ assignment::{AssignKeyValue, AssignResult, KvAssignment, missing_key}, - delta::{PartialConfigDelta, delta_map, delta_map_with_unsets, path}, + delta::{PartialConfigDelta, delta_mergeable_map, path}, fill::{FillDefaults, fill_map}, + internal::merge::map_with_strategy, model::id::{ModelIdConfig, ModelIdConfigError, ModelIdOrAliasConfig, resolve_alias_chain}, partial::ToPartial, providers::llm::{ @@ -28,8 +29,8 @@ use crate::{ openai::{OpenaiConfig, PartialOpenaiConfig}, openrouter::{OpenrouterConfig, PartialOpenrouterConfig}, }, - util::merge_nested_indexmap, validate::Validator, + types::map::{MergeableMap, map_to_partial_per_key}, }; /// Provider configuration. @@ -49,8 +50,13 @@ pub struct LlmProviderConfig { /// haiku = { provider = "anthropic", name = "claude-haiku-4-5" } /// coder = "opus" /// ``` - #[setting(nested, merge = merge_nested_indexmap)] - pub aliases: IndexMap, + /// + /// Entries merge by key, so an alias defined in a later layer joins the + /// ones an earlier layer set. + /// Declare the map as `{ value = { … }, strategy = "replace" }` to drop + /// them instead. + #[setting(nested, merge = map_with_strategy)] + pub aliases: MergeableMap, /// Anthropic API configuration. #[setting(nested)] @@ -117,7 +123,7 @@ impl PartialConfigDelta for PartialLlmProviderConfig { // that drops the paths would merge a list onto the one already there. fn delta(&self, next: Self) -> Self { Self { - aliases: delta_map(&self.aliases, next.aliases), + aliases: delta_mergeable_map(&self.aliases, next.aliases), anthropic: self.anthropic.delta(next.anthropic), cerebras: self.cerebras.delta(next.cerebras), deepseek: self.deepseek.delta(next.deepseek), @@ -131,12 +137,9 @@ impl PartialConfigDelta for PartialLlmProviderConfig { fn delta_with_unsets(&self, next: Self, prefix: &str, unsets: &mut Vec) -> Self { Self { - aliases: delta_map_with_unsets( - &path(prefix, "aliases"), - &self.aliases, - next.aliases, - unsets, - ), + // The map states its own strategy, so a removed alias travels in + // the value as a `replace` and needs no path reported. + aliases: delta_mergeable_map(&self.aliases, next.aliases), anthropic: self.anthropic.delta_with_unsets( next.anthropic, &path(prefix, "anthropic"), @@ -160,7 +163,15 @@ impl PartialConfigDelta for PartialLlmProviderConfig { impl FillDefaults for PartialLlmProviderConfig { fn fill_from(self, defaults: Self) -> Self { Self { - aliases: fill_map(self.aliases, defaults.aliases), + // Key by key, so an alias only the defaults declare is added + // while one this layer already has keeps its own value. A map + // that states a strategy is left alone. + aliases: match self.aliases { + merged @ MergeableMap::Merged(_) => merged, + MergeableMap::Map(entries) => { + fill_map(entries, defaults.aliases.into_map()).into() + } + }, anthropic: self.anthropic.fill_from(defaults.anthropic), cerebras: self.cerebras.fill_from(defaults.cerebras), deepseek: self.deepseek.fill_from(defaults.deepseek), @@ -176,11 +187,9 @@ impl FillDefaults for PartialLlmProviderConfig { impl ToPartial for LlmProviderConfig { fn to_partial(&self) -> Self::Partial { Self::Partial { - aliases: self - .aliases - .iter() - .map(|(k, v)| (k.clone(), v.to_partial())) - .collect(), + // Per key rather than `replace`: an alias the workspace config + // gained after this conversation was created still reaches it. + aliases: map_to_partial_per_key(self.aliases.iter()), anthropic: self.anthropic.to_partial(), cerebras: self.cerebras.to_partial(), deepseek: self.deepseek.to_partial(), diff --git a/crates/jp_config/src/snapshots/jp_config__tests__partial_app_config_default.snap b/crates/jp_config/src/snapshots/jp_config__tests__partial_app_config_default.snap index 2ef03dd8a..c53ccf312 100644 --- a/crates/jp_config/src/snapshots/jp_config__tests__partial_app_config_default.snap +++ b/crates/jp_config/src/snapshots/jp_config__tests__partial_app_config_default.snap @@ -215,7 +215,9 @@ PartialAppConfig { }, providers: PartialProviderConfig { llm: PartialLlmProviderConfig { - aliases: {}, + aliases: Map( + {}, + ), anthropic: PartialAnthropicConfig { auth: None, api_key_env: None, @@ -260,7 +262,9 @@ PartialAppConfig { plugins: PartialPluginsConfig { auto_install: None, shutdown_timeout_secs: None, - command: {}, + command: Map( + {}, + ), }, user: PartialUserConfig { name: None, 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 ed0bfb419..dcf960294 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 @@ -445,7 +445,9 @@ Ok( }, providers: PartialProviderConfig { llm: PartialLlmProviderConfig { - aliases: {}, + aliases: Map( + {}, + ), anthropic: PartialAnthropicConfig { auth: Some( [ @@ -536,7 +538,9 @@ Ok( shutdown_timeout_secs: Some( 5, ), - command: {}, + command: Map( + {}, + ), }, user: PartialUserConfig { name: None, diff --git a/crates/jp_config/src/snapshots/jp_config__tests__partial_app_config_empty_serialize.snap b/crates/jp_config/src/snapshots/jp_config__tests__partial_app_config_empty_serialize.snap index 7b89fcf17..3abe6f42f 100644 --- a/crates/jp_config/src/snapshots/jp_config__tests__partial_app_config_empty_serialize.snap +++ b/crates/jp_config/src/snapshots/jp_config__tests__partial_app_config_empty_serialize.snap @@ -215,7 +215,9 @@ PartialAppConfig { }, providers: PartialProviderConfig { llm: PartialLlmProviderConfig { - aliases: {}, + aliases: Map( + {}, + ), anthropic: PartialAnthropicConfig { auth: None, api_key_env: None, @@ -260,7 +262,9 @@ PartialAppConfig { plugins: PartialPluginsConfig { auto_install: None, shutdown_timeout_secs: None, - command: {}, + command: Map( + {}, + ), }, user: PartialUserConfig { name: None, diff --git a/crates/jp_conversation/src/compat.rs b/crates/jp_conversation/src/compat.rs index d5e4f534c..513dc775e 100644 --- a/crates/jp_conversation/src/compat.rs +++ b/crates/jp_conversation/src/compat.rs @@ -325,9 +325,56 @@ fn sole_matching_variant<'a>(union_type: &'a UnionType, value: &Value) -> Option let variants = || union_type.variants_types.iter().map(Box::as_ref); sole(variants().filter(|variant| !variant.is_null())) + .or_else(|| strategy_carrying_variant(union_type, value)) .or_else(|| sole(variants().filter(|variant| accepts(&variant.ty, value)))) } +/// The variant of a collection that can state its own merge strategy. +/// +/// A `MergeableMap` is the plain map beside a wrapper struct holding it under +/// `value` next to the strategy. +/// Both are objects on the wire, so shape alone leaves the union ambiguous, and +/// every key inside a tool, server, alias or plugin would go unwalked: a stale +/// one would then survive to fail typed deserialization, which discards the +/// whole stored config rather than the key. +/// +/// Told apart the way the wrapper's own deserializer does it: an object +/// carrying both `value` and `strategy` is the wrapper, anything else is the +/// map. +/// An entry named `value` needs the sibling `strategy` before it reads as the +/// wrapper, which is what keeps a tool called `value` addressable. +fn strategy_carrying_variant<'a>(union_type: &'a UnionType, value: &Value) -> Option<&'a Schema> { + let mut variants = union_type + .variants_types + .iter() + .map(Box::as_ref) + .filter(|variant| !variant.is_null()); + + let (first, second) = (variants.next()?, variants.next()?); + if variants.next().is_some() { + return None; + } + + let is_wrapper = |schema: &Schema| { + matches!(&schema.ty, SchemaType::Struct(wrapper) + if wrapper.fields.contains_key("value") && wrapper.fields.contains_key("strategy")) + }; + + let (wrapper, collection) = if is_wrapper(first) { + (first, second) + } else if is_wrapper(second) { + (second, first) + } else { + return None; + }; + + let stated = value + .as_object() + .is_some_and(|obj| obj.contains_key("value") && obj.contains_key("strategy")); + + Some(if stated { wrapper } else { collection }) +} + /// The only item an iterator yields, if it yields exactly one. fn sole<'a>(mut variants: impl Iterator) -> Option<&'a Schema> { match (variants.next(), variants.next()) { diff --git a/crates/jp_llm/src/tool/json_schema.rs b/crates/jp_llm/src/tool/json_schema.rs index fda533054..042210dff 100644 --- a/crates/jp_llm/src/tool/json_schema.rs +++ b/crates/jp_llm/src/tool/json_schema.rs @@ -745,7 +745,7 @@ fn apply_config_fields( .cloned() .unwrap_or_default(); - for (name, property) in &config.properties { + for (name, property) in config.properties.iter() { let path = format!("{path}.properties.{name}"); let merged = match properties.get(name) { Some(source) => node_with_override(&path, source, root, property)?, From ddf0a4494808c69992461ba0b0602c977ebc880b Mon Sep 17 00:00:00 2001 From: Jean Mertz Date: Mon, 7 Sep 2026 16:12:02 +0200 Subject: [PATCH 06/16] feat(config): Let template and tool option maps declare their strategy `template.values` and a tool's `options` accept a strategy the way every other collection field does: ```toml [template.values] value = { branch = "main" } strategy = "replace" ``` The default is unchanged: entries merge by key, so a value set in a later layer joins the ones an earlier layer set. Both maps hold free-form JSON rather than nested config, so their entries have no partial to diff and are compared and carried whole through `delta_mergeable_value_map`. A removed entry travels as a `replace` with the entries the user is left with, so neither map needs a reported path any more. `delta_value_map` and its unset-reporting sibling are gone with them. Every map in the configuration except `conversation.tools` itself now states its own strategy, and each conversation snapshot merges per key so an entry the files declare and the conversation does not survives the layering. Signed-off-by: Jean Mertz --- .../jp_cli/src/cmd/query/turn_loop_tests.rs | 40 ++++++++-------- crates/jp_config/src/conversation/tool.rs | 37 +++++++------- crates/jp_config/src/delta.rs | 46 +++++++----------- ...ig__tests__partial_app_config_default.snap | 4 +- ...ts__partial_app_config_default_values.snap | 4 +- ...s__partial_app_config_empty_serialize.snap | 4 +- crates/jp_config/src/template.rs | 48 +++++++++---------- 7 files changed, 88 insertions(+), 95 deletions(-) diff --git a/crates/jp_cli/src/cmd/query/turn_loop_tests.rs b/crates/jp_cli/src/cmd/query/turn_loop_tests.rs index 57a47658c..37274fd45 100644 --- a/crates/jp_cli/src/cmd/query/turn_loop_tests.rs +++ b/crates/jp_cli/src/cmd/query/turn_loop_tests.rs @@ -1394,7 +1394,7 @@ async fn test_tool_interrupt_menu_cancel_escalates() { result: None, style: None, questions: IndexMap::new().into(), - options: IndexMap::default(), + options: IndexMap::default().into(), access: None, cancellation_response: None, }); @@ -1543,7 +1543,7 @@ async fn test_tool_stop_on_interrupt_commits_responses_without_follow_up() { result: None, style: None, questions: IndexMap::new().into(), - options: IndexMap::default(), + options: IndexMap::default().into(), access: None, cancellation_response: Some(CUSTOM_CANCELLATION_RESPONSE.to_string()), }); @@ -1687,7 +1687,7 @@ async fn test_interrupt_during_tool_prompt_completes_turn_early() { answer: None, })]) .into(), - options: IndexMap::default(), + options: IndexMap::default().into(), access: None, cancellation_response: None, }); @@ -2026,7 +2026,7 @@ async fn test_tool_restart_on_interrupt() { result: None, style: None, questions: IndexMap::new().into(), - options: IndexMap::default(), + options: IndexMap::default().into(), access: None, cancellation_response: None, }); @@ -2182,7 +2182,7 @@ async fn test_merged_stream_exits_after_tool_response() { result: None, style: None, questions: IndexMap::new().into(), - options: IndexMap::default(), + options: IndexMap::default().into(), access: None, cancellation_response: None, }); @@ -2295,7 +2295,7 @@ async fn test_tool_call_with_run_mode_ask_approves() { result: None, style: None, questions: IndexMap::new().into(), - options: IndexMap::default(), + options: IndexMap::default().into(), access: None, cancellation_response: None, }); @@ -2439,7 +2439,7 @@ async fn test_tool_call_with_run_mode_ask_skips() { result: None, style: None, questions: IndexMap::new().into(), - options: IndexMap::default(), + options: IndexMap::default().into(), access: None, cancellation_response: None, }); @@ -2594,7 +2594,7 @@ async fn test_permission_prompt_follows_interactive_not_is_tty() { result: None, style: None, questions: IndexMap::new().into(), - options: IndexMap::default(), + options: IndexMap::default().into(), access: None, cancellation_response: None, }); @@ -2721,7 +2721,7 @@ async fn test_tool_call_with_run_mode_unattended() { result: None, style: None, questions: IndexMap::new().into(), - options: IndexMap::default(), + options: IndexMap::default().into(), access: None, cancellation_response: None, }); @@ -2860,7 +2860,7 @@ async fn test_tool_call_with_run_mode_skip() { result: None, style: None, questions: IndexMap::new().into(), - options: IndexMap::default(), + options: IndexMap::default().into(), access: None, cancellation_response: None, }); @@ -3015,7 +3015,7 @@ async fn test_multiple_tools_with_different_run_modes() { result: None, style: None, questions: IndexMap::new().into(), - options: IndexMap::default(), + options: IndexMap::default().into(), access: None, cancellation_response: None, }); @@ -3036,7 +3036,7 @@ async fn test_multiple_tools_with_different_run_modes() { result: None, style: None, questions: IndexMap::new().into(), - options: IndexMap::default(), + options: IndexMap::default().into(), access: None, cancellation_response: None, }); @@ -3224,7 +3224,7 @@ async fn test_tool_call_returns_error() { result: None, style: None, questions: IndexMap::new().into(), - options: IndexMap::default(), + options: IndexMap::default().into(), access: None, cancellation_response: None, }); @@ -4531,7 +4531,7 @@ async fn test_parallel_tool_calls_rendered_atomically() { result: None, style: fn_call_style.clone(), questions: IndexMap::new().into(), - options: IndexMap::default(), + options: IndexMap::default().into(), access: None, cancellation_response: None, }); @@ -4551,7 +4551,7 @@ async fn test_parallel_tool_calls_rendered_atomically() { result: None, style: fn_call_style, questions: IndexMap::new().into(), - options: IndexMap::default(), + options: IndexMap::default().into(), access: None, cancellation_response: None, }); @@ -4724,7 +4724,7 @@ async fn test_single_tool_call_rendered_with_args() { result: None, style: None, questions: IndexMap::new().into(), - options: IndexMap::default(), + options: IndexMap::default().into(), access: None, cancellation_response: None, }); @@ -6004,7 +6004,7 @@ fn inquiry_tool_config(questions: &[&str]) -> ToolConfig { }) .collect::>() .into(), - options: IndexMap::default(), + options: IndexMap::default().into(), access: None, cancellation_response: None, } @@ -7114,7 +7114,7 @@ async fn test_parallel_tools_one_with_inquiry() { result: None, style: None, questions: IndexMap::new().into(), - options: IndexMap::default(), + options: IndexMap::default().into(), access: None, cancellation_response: None, }); @@ -7549,7 +7549,7 @@ async fn test_unavailable_tool_before_approved_does_not_panic() { result: None, style: None, questions: IndexMap::new().into(), - options: IndexMap::default(), + options: IndexMap::default().into(), access: None, cancellation_response: None, }); @@ -7981,7 +7981,7 @@ async fn reasoning_before_a_tool_call_shades_the_tool_chrome() { result: None, style: None, questions: IndexMap::new().into(), - options: IndexMap::default(), + options: IndexMap::default().into(), access: None, cancellation_response: None, }); diff --git a/crates/jp_config/src/conversation/tool.rs b/crates/jp_config/src/conversation/tool.rs index 0f36427ff..ccf41da98 100644 --- a/crates/jp_config/src/conversation/tool.rs +++ b/crates/jp_config/src/conversation/tool.rs @@ -18,9 +18,9 @@ use crate::{ style::{DisplayStyleConfig, PartialDisplayStyleConfig}, }, delta::{ - PartialConfigDelta, delta_map, delta_map_with_unsets, delta_mergeable_map, delta_opt, - delta_opt_at, delta_opt_partial, delta_opt_partial_at, delta_value_map, - delta_value_map_with_unsets, delta_vec, path, + PartialConfigDelta, delta_map, delta_map_with_unsets, delta_mergeable_map, + delta_mergeable_value_map, delta_opt, delta_opt_at, delta_opt_partial, + delta_opt_partial_at, delta_vec, path, }, fill::{FillDefaults, fill_map}, internal::merge::map_with_strategy, @@ -579,8 +579,13 @@ pub struct ToolConfig { /// A free-form map of key-value pairs that configure tool behavior. /// Each tool defines its own supported options and defaults. /// Unknown options are silently forwarded. - #[setting(nested, merge = merge_nested_indexmap)] - pub options: IndexMap, + /// + /// Entries merge by key, so an option set in a later layer joins the ones + /// an earlier layer set. + /// Declare the map as `{ value = { … }, strategy = "replace" }` to drop + /// them instead. + #[setting(nested, merge = map_with_strategy)] + pub options: MergeableMap, /// Resource access grants for the tool. /// @@ -640,7 +645,7 @@ impl PartialConfigDelta for PartialToolConfig { ), style: delta_opt_partial(self.style.as_ref(), next.style), questions: delta_mergeable_map(&self.questions, next.questions), - options: delta_value_map(&self.options, next.options), + options: delta_mergeable_value_map(&self.options, next.options), access: delta_opt_partial(self.access.as_ref(), next.access), } } @@ -680,12 +685,7 @@ impl PartialConfigDelta for PartialToolConfig { unsets, ), questions: delta_mergeable_map(&self.questions, next.questions), - options: delta_value_map_with_unsets( - &path(prefix, "options"), - &self.options, - next.options, - unsets, - ), + options: delta_mergeable_value_map(&self.options, next.options), access: delta_opt_partial_at( &path(prefix, "access"), self.access.as_ref(), @@ -719,11 +719,12 @@ impl ToPartial for ToolConfig { ), style: partial_opt_config(self.style.as_ref(), defaults.style), questions: map_to_partial_per_key(self.questions.iter()), - options: self - .options - .iter() - .map(|(k, v)| (k.clone(), v.clone())) - .collect(), + options: MergeableMap::Map( + self.options + .iter() + .map(|(k, v)| (k.clone(), v.clone())) + .collect(), + ), access: partial_opt_config(self.access.as_ref(), defaults.access), } } @@ -1312,7 +1313,7 @@ impl ToolConfigWithDefaults { /// Return the per-tool options map. #[must_use] - pub const fn options(&self) -> &IndexMap { + pub fn options(&self) -> &IndexMap { &self.tool.options } diff --git a/crates/jp_config/src/delta.rs b/crates/jp_config/src/delta.rs index ea03e00a2..f263623cc 100644 --- a/crates/jp_config/src/delta.rs +++ b/crates/jp_config/src/delta.rs @@ -240,41 +240,29 @@ where .collect() } -/// Calculate the delta between two maps of plain values. +/// Calculate the delta between two strategy-carrying maps of plain values. /// -/// An entry is kept when `next` holds a value for it that differs from -/// `prev`'s. -/// A map of nested partials wants [`delta_map`] instead, which records only the -/// changed fields of an entry both maps hold. -pub fn delta_value_map( - prev: &IndexMap, - next: IndexMap, -) -> IndexMap { +/// Mirrors [`delta_mergeable_map`] for a map whose values carry no partial of +/// their own, so an entry is compared and carried whole rather than diffed. +pub fn delta_mergeable_value_map( + prev: &MergeableMap, + next: MergeableMap, +) -> MergeableMap { + if prev.keys().any(|key| !next.contains_key(key)) { + // Stated rather than inherited from `next`'s shape: a plain map + // deep-merges on the fold and brings the dropped key back. + return MergeableMap::Merged(MergedMap { + value: next.into_map(), + strategy: Some(MergedMapStrategy::Replace), + discard_when_merged: false, + }); + } + next.into_iter() .filter(|(key, next)| !prev.get(key).is_some_and(|prev| prev == next)) .collect() } -/// Calculate the delta between two maps of plain values, reporting removed -/// entries. -/// -/// Mirrors [`delta_map_with_unsets`] for a map whose values carry no partial of -/// their own. -pub fn delta_value_map_with_unsets( - prefix: &str, - prev: &IndexMap, - next: IndexMap, - unsets: &mut Vec, -) -> IndexMap { - for key in prev.keys() { - if !next.contains_key(key) { - unsets.push(path(prefix, key)); - } - } - - delta_value_map(prev, next) -} - /// Calculate the delta between two optional values, reporting a cleared field. /// /// A value that went away cannot be expressed by merging: schematic keeps the diff --git a/crates/jp_config/src/snapshots/jp_config__tests__partial_app_config_default.snap b/crates/jp_config/src/snapshots/jp_config__tests__partial_app_config_default.snap index c53ccf312..b84555e35 100644 --- a/crates/jp_config/src/snapshots/jp_config__tests__partial_app_config_default.snap +++ b/crates/jp_config/src/snapshots/jp_config__tests__partial_app_config_default.snap @@ -211,7 +211,9 @@ PartialAppConfig { }, }, template: PartialTemplateConfig { - values: {}, + values: Map( + {}, + ), }, providers: PartialProviderConfig { llm: PartialLlmProviderConfig { 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 dcf960294..1c77a3f04 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 @@ -441,7 +441,9 @@ Ok( }, }, template: PartialTemplateConfig { - values: {}, + values: Map( + {}, + ), }, providers: PartialProviderConfig { llm: PartialLlmProviderConfig { diff --git a/crates/jp_config/src/snapshots/jp_config__tests__partial_app_config_empty_serialize.snap b/crates/jp_config/src/snapshots/jp_config__tests__partial_app_config_empty_serialize.snap index 3abe6f42f..e7b22e986 100644 --- a/crates/jp_config/src/snapshots/jp_config__tests__partial_app_config_empty_serialize.snap +++ b/crates/jp_config/src/snapshots/jp_config__tests__partial_app_config_empty_serialize.snap @@ -211,7 +211,9 @@ PartialAppConfig { }, }, template: PartialTemplateConfig { - values: {}, + values: Map( + {}, + ), }, providers: PartialProviderConfig { llm: PartialLlmProviderConfig { diff --git a/crates/jp_config/src/template.rs b/crates/jp_config/src/template.rs index 181a4c1ad..60645c5de 100644 --- a/crates/jp_config/src/template.rs +++ b/crates/jp_config/src/template.rs @@ -1,15 +1,14 @@ //! Template configuration for Jean-Pierre. -use indexmap::IndexMap; use schematic::Config; use crate::{ assignment::{AssignKeyValue, KvAssignment, missing_key}, - delta::{PartialConfigDelta, delta_value_map, delta_value_map_with_unsets, path}, + delta::{PartialConfigDelta, delta_mergeable_value_map}, fill::FillDefaults, + internal::merge::map_with_strategy, partial::ToPartial, - types::json_value::JsonValue, - util::merge_nested_indexmap, + types::{json_value::JsonValue, map::MergeableMap}, }; /// Template configuration. @@ -17,8 +16,13 @@ use crate::{ #[config(rename_all = "snake_case")] pub struct TemplateConfig { /// Template variable values used to render query templates. - #[setting(nested, merge = merge_nested_indexmap)] - pub values: IndexMap, + /// + /// Entries merge by key, so a value set in a later layer joins the ones an + /// earlier layer set. + /// Declare the map as `{ value = { … }, strategy = "replace" }` to drop + /// them instead. + #[setting(nested, merge = map_with_strategy)] + pub values: MergeableMap, } impl AssignKeyValue for PartialTemplateConfig { @@ -36,36 +40,30 @@ impl AssignKeyValue for PartialTemplateConfig { impl PartialConfigDelta for PartialTemplateConfig { fn delta(&self, next: Self) -> Self { Self { - values: delta_value_map(&self.values, next.values), - } - } - - fn delta_with_unsets(&self, next: Self, prefix: &str, unsets: &mut Vec) -> Self { - Self { - values: delta_value_map_with_unsets( - &path(prefix, "values"), - &self.values, - next.values, - unsets, - ), + values: delta_mergeable_value_map(&self.values, next.values), } } } impl FillDefaults for PartialTemplateConfig { - fn fill_from(self, _defaults: Self) -> Self { - self + fn fill_from(self, defaults: Self) -> Self { + Self { + values: self.values.fill_from(defaults.values), + } } } impl ToPartial for TemplateConfig { fn to_partial(&self) -> Self::Partial { Self::Partial { - values: self - .values - .iter() - .map(|(k, v)| (k.clone(), v.clone())) - .collect(), + // Per key rather than `replace`: a value the workspace config + // gained after this conversation was created still reaches it. + values: MergeableMap::Map( + self.values + .iter() + .map(|(k, v)| (k.clone(), v.clone())) + .collect(), + ), } } } From 3f89b4d5146dbf47490bd1f9d26b73f790d9e877 Mon Sep 17 00:00:00 2001 From: Jean Mertz Date: Mon, 7 Sep 2026 16:23:07 +0200 Subject: [PATCH 07/16] feat(config): Let the tools map declare its merge strategy `conversation.tools` accepts a strategy, which completes the set: every collection field in the configuration now states how it merges. ```toml [conversation.tools] strategy = "replace" [conversation.tools.value.my_tool] source = "builtin" ``` The default is unchanged: tools merge by key, so a tool configured in a later layer joins the ones an earlier layer set. Tool entries are flattened to sit directly under `conversation.tools`, and the wrapper needs both `value` and `strategy` to be recognised, so a tool may still be named `value` or `strategy` on its own. Two tests hold both halves of that. `delta_map` and `delta_map_with_unsets` are gone. Every map states its own strategy, so a removed entry travels in the value as a `replace` and no map needs a reported path. What remains of `unsets` is what only it can express: a scalar that went away. Signed-off-by: Jean Mertz --- crates/jp_cli/src/cmd/query.rs | 4 +- crates/jp_cli/src/cmd/query_tests.rs | 4 +- crates/jp_config/src/conversation/tool.rs | 22 +++--- .../jp_config/src/conversation/tool_tests.rs | 62 ++++++++++++++-- crates/jp_config/src/delta.rs | 74 ------------------- crates/jp_config/src/delta_tests.rs | 53 +++++++------ ...ig__tests__partial_app_config_default.snap | 4 +- ...ts__partial_app_config_default_values.snap | 4 +- ...s__partial_app_config_empty_serialize.snap | 4 +- crates/jp_config/src/util.rs | 2 +- 10 files changed, 110 insertions(+), 123 deletions(-) diff --git a/crates/jp_cli/src/cmd/query.rs b/crates/jp_cli/src/cmd/query.rs index 1c59dca42..c48f13881 100644 --- a/crates/jp_cli/src/cmd/query.rs +++ b/crates/jp_cli/src/cmd/query.rs @@ -2558,12 +2558,12 @@ fn apply_enable_tools( for d in directives.iter() { match d { ToolDirective::EnableAll => { - for (name, tool) in &mut partial.conversation.tools.tools { + for (name, tool) in partial.conversation.tools.tools.iter_mut() { apply_directive_to_tool(name, tool, &defaults, ToggleScope::Bulk, true)?; } } ToolDirective::DisableAll => { - for (name, tool) in &mut partial.conversation.tools.tools { + for (name, tool) in partial.conversation.tools.tools.iter_mut() { apply_directive_to_tool(name, tool, &defaults, ToggleScope::Bulk, false)?; } } diff --git a/crates/jp_cli/src/cmd/query_tests.rs b/crates/jp_cli/src/cmd/query_tests.rs index 5b99e741f..da9d092c0 100644 --- a/crates/jp_cli/src/cmd/query_tests.rs +++ b/crates/jp_cli/src/cmd/query_tests.rs @@ -48,7 +48,7 @@ use crate::{ fn make_partial_with_tools() -> PartialAppConfig { let mut partial = PartialAppConfig::default(); - partial.conversation.tools.tools = IndexMap::from_iter([ + *partial.conversation.tools.tools = IndexMap::from_iter([ ("implicitly_enabled_tool".into(), PartialToolConfig { enable: None, ..Default::default() @@ -1204,7 +1204,7 @@ fn test_builtin_config_preserves_tool_order() { // Tool order is the order tools are presented to the provider, so merging // a builtin block must not move an existing entry to the end. let mut partial = PartialAppConfig::default(); - partial.conversation.tools.tools = IndexMap::from_iter([ + *partial.conversation.tools.tools = IndexMap::from_iter([ ("describe_tools".into(), PartialToolConfig { result: Some(ResultMode::Ask), ..Default::default() diff --git a/crates/jp_config/src/conversation/tool.rs b/crates/jp_config/src/conversation/tool.rs index ccf41da98..de4bb9c86 100644 --- a/crates/jp_config/src/conversation/tool.rs +++ b/crates/jp_config/src/conversation/tool.rs @@ -18,9 +18,8 @@ use crate::{ style::{DisplayStyleConfig, PartialDisplayStyleConfig}, }, delta::{ - PartialConfigDelta, delta_map, delta_map_with_unsets, delta_mergeable_map, - delta_mergeable_value_map, delta_opt, delta_opt_at, delta_opt_partial, - delta_opt_partial_at, delta_vec, path, + PartialConfigDelta, delta_mergeable_map, delta_mergeable_value_map, delta_opt, + delta_opt_at, delta_opt_partial, delta_opt_partial_at, delta_vec, path, }, fill::{FillDefaults, fill_map}, internal::merge::map_with_strategy, @@ -29,7 +28,6 @@ use crate::{ json_value::JsonValue, map::{MergeableMap, map_to_partial_per_key}, }, - util::merge_nested_indexmap, validate::Validator, }; @@ -51,8 +49,8 @@ pub struct ToolsConfig { /// This section configures individual tools. /// The key is the tool ID, and cannot contain a comma: a comma separates /// one tool ID from the next wherever several are named at once. - #[setting(nested, flatten, merge = merge_nested_indexmap)] - tools: IndexMap, + #[setting(nested, flatten, merge = map_with_strategy)] + tools: MergeableMap, } impl AssignKeyValue for PartialToolsConfig { @@ -71,7 +69,7 @@ impl PartialConfigDelta for PartialToolsConfig { fn delta(&self, next: Self) -> Self { Self { defaults: self.defaults.delta(next.defaults), - tools: delta_map(&self.tools, next.tools), + tools: delta_mergeable_map(&self.tools, next.tools), } } @@ -80,7 +78,9 @@ impl PartialConfigDelta for PartialToolsConfig { defaults: self .defaults .delta_with_unsets(next.defaults, &path(prefix, "*"), unsets), - tools: delta_map_with_unsets(prefix, &self.tools, next.tools, unsets), + // The map states its own strategy, so a removed tool travels in + // the value as a `replace` and needs no path reported. + tools: delta_mergeable_map(&self.tools, next.tools), } } } @@ -109,7 +109,8 @@ impl FillDefaults for PartialToolsConfig { (name, tool) }) - .collect(); + .collect::>() + .into(); Self { defaults: tool_defaults, @@ -144,7 +145,8 @@ impl ToPartial for ToolsConfig { (name.clone(), tool) }) - .collect(); + .collect::>() + .into(); Self::Partial { defaults, tools } } diff --git a/crates/jp_config/src/conversation/tool_tests.rs b/crates/jp_config/src/conversation/tool_tests.rs index dd4c2fd57..2795f421c 100644 --- a/crates/jp_config/src/conversation/tool_tests.rs +++ b/crates/jp_config/src/conversation/tool_tests.rs @@ -933,18 +933,21 @@ fn test_tools_config() { assert_eq!( p.tools, - IndexMap::<_, _>::from_iter(vec![("cargo_check".to_owned(), PartialToolConfig { - enable: Some(PartialEnableConfig::ON), - source: Some(ToolSource::Local { tool: None }), - ..Default::default() - })]) + MergeableMap::from(IndexMap::<_, _>::from_iter(vec![( + "cargo_check".to_owned(), + PartialToolConfig { + enable: Some(PartialEnableConfig::ON), + source: Some(ToolSource::Local { tool: None }), + ..Default::default() + } + )])) ); let kv = KvAssignment::try_from_cli("foo:", r#"{"source":"builtin"}"#).unwrap(); p.assign(kv).unwrap(); assert_eq!( p.tools, - IndexMap::<_, _>::from_iter(vec![ + MergeableMap::from(IndexMap::<_, _>::from_iter(vec![ ("cargo_check".to_owned(), PartialToolConfig { enable: Some(PartialEnableConfig::ON), source: Some(ToolSource::Local { tool: None }), @@ -954,7 +957,52 @@ fn test_tools_config() { source: Some(ToolSource::Builtin { tool: None }), ..Default::default() }) - ]) + ])) + ); +} + +/// The tools map takes a strategy, even though its entries are flattened to sit +/// directly under `conversation.tools`. +#[test] +fn tools_map_accepts_a_replace_strategy() { + let config: PartialToolsConfig = toml::from_str( + r#" + strategy = "replace" + + [value.my_tool] + source = "builtin" + "#, + ) + .expect("a strategy-carrying tools map parses"); + + assert!( + matches!(&config.tools, MergeableMap::Merged(merged) + if merged.strategy == Some(crate::types::map::MergedMapStrategy::Replace)), + "expected the declared strategy to survive the flatten: {:?}", + config.tools + ); + assert!(config.tools.contains_key("my_tool")); +} + +/// A plain tools map keeps merging per key, and a tool may be named `value`. +#[test] +fn tools_map_without_a_strategy_merges_per_key() { + let config: PartialToolsConfig = toml::from_str( + r#" + [value] + source = "builtin" + "#, + ) + .expect("a plain tools map parses"); + + assert!( + matches!(&config.tools, MergeableMap::Map(_)), + "expected a plain map: {:?}", + config.tools + ); + assert!( + config.tools.contains_key("value"), + "`value` alone names a tool, since a strategy needs both keys" ); } diff --git a/crates/jp_config/src/delta.rs b/crates/jp_config/src/delta.rs index f263623cc..36b8336fc 100644 --- a/crates/jp_config/src/delta.rs +++ b/crates/jp_config/src/delta.rs @@ -1,6 +1,5 @@ //! Configuration delta calculation. -use indexmap::IndexMap; use schematic::PartialConfig; use crate::types::{ @@ -196,50 +195,6 @@ pub fn delta_opt_partial_at( } } -/// Calculate the delta between two maps, reporting removed entries and each -/// entry's own unsets. -/// -/// Entries merge by key, so an entry `next` no longer has cannot be expressed -/// by merging: the key would survive from the previous layer. -/// Its path joins `unsets` so the fold removes the entry before merging. -/// -/// Descends into an entry both maps have with that entry's own dotted path, so -/// a field inside it reports where it lives. -pub fn delta_map_with_unsets( - prefix: &str, - prev: &IndexMap, - next: IndexMap, - unsets: &mut Vec, -) -> IndexMap -where - V: PartialConfigDelta + PartialEq, -{ - for key in prev.keys() { - if !next.contains_key(key) { - unsets.push(path(prefix, key)); - } - } - - next.into_iter() - .filter_map(|(key, next)| { - let Some(prev) = prev.get(&key) else { - return Some((key, next)); - }; - - if prev == &next { - return None; - } - - let mut entry = Vec::new(); - let delta = prev.delta_with_unsets(next, &path(prefix, &key), &mut entry); - let cleared = !entry.is_empty(); - unsets.append(&mut entry); - - (cleared || !delta.is_empty()).then_some((key, delta)) - }) - .collect() -} - /// Calculate the delta between two strategy-carrying maps of plain values. /// /// Mirrors [`delta_mergeable_map`] for a map whose values carry no partial of @@ -304,35 +259,6 @@ pub fn delta_opt_partial( } } -/// Calculate the delta between two maps of partial configurations. -/// -/// An entry only `next` has is kept whole. -/// An entry both maps have contributes its own delta, and is left out when that -/// delta is empty. -/// -/// Dropping the empty ones is what keeps [`PartialConfig::is_empty`] meaningful -/// for the enclosing config: a map counts as empty only when it has no entries -/// at all, so an entry that carries no values still reads as a change. -pub fn delta_map(prev: &IndexMap, next: IndexMap) -> IndexMap -where - V: PartialConfigDelta + PartialEq, -{ - next.into_iter() - .filter_map(|(key, next)| { - let Some(prev) = prev.get(&key) else { - return Some((key, next)); - }; - - if prev == &next { - return None; - } - - let delta = prev.delta(next); - (!delta.is_empty()).then_some((key, delta)) - }) - .collect() -} - /// Calculate the delta between two vectors that merge by appending. /// /// The delta holds the elements `next` adds to `prev`. diff --git a/crates/jp_config/src/delta_tests.rs b/crates/jp_config/src/delta_tests.rs index aaae4f532..a12f50080 100644 --- a/crates/jp_config/src/delta_tests.rs +++ b/crates/jp_config/src/delta_tests.rs @@ -4,7 +4,10 @@ use test_log::test; use super::*; use crate::{ providers::mcp::{PartialMcpProviderConfig, PartialStdioConfig}, - types::vec::{MergeableVec, MergedVec, MergedVecStrategy}, + types::{ + map::{MergeableMap, MergedMapStrategy}, + vec::{MergeableVec, MergedVec, MergedVecStrategy}, + }, }; /// A server entry with `arguments` set and every other field unset. @@ -23,39 +26,41 @@ fn server(arguments: &[&str]) -> PartialMcpProviderConfig { } /// A one-server map, keyed as `kagi`. -fn map(arguments: &[&str]) -> IndexMap { +fn map(arguments: &[&str]) -> MergeableMap { let mut map = IndexMap::new(); map.insert("kagi".to_owned(), server(arguments)); - map + map.into() } -/// A removed map entry is reported, since merging cannot take a key away. +/// A removed entry is carried as a `replace`, since a deep merge would bring +/// the key back. #[test] -fn map_delta_reports_a_removed_entry() { +fn map_delta_replaces_when_an_entry_is_removed() { let prev = map(&["--a"]); - let next = IndexMap::new(); - let mut unsets = Vec::new(); + let next = MergeableMap::default(); - let delta = delta_map_with_unsets("providers.mcp", &prev, next, &mut unsets); + let delta = delta_mergeable_map(&prev, next); - assert!(delta.is_empty(), "nothing to merge for a removed entry"); - assert_eq!(unsets, ["providers.mcp.kagi"]); + assert!( + matches!(&delta, MergeableMap::Merged(merged) + if merged.strategy == Some(MergedMapStrategy::Replace) && merged.value.is_empty()), + "expected an empty map stated as `replace`, got: {delta:?}" + ); } -/// An entry both maps hold is not reported, only diffed. +/// An entry both maps hold is diffed, not replaced. #[test] -fn map_delta_does_not_report_a_surviving_entry() { +fn map_delta_diffs_a_surviving_entry() { let prev = map(&["--a"]); let next = map(&["--a", "--b"]); - let mut unsets = Vec::new(); - let delta = delta_map_with_unsets("providers.mcp", &prev, next, &mut unsets); + let delta = delta_mergeable_map(&prev, next); - assert_eq!(delta.len(), 1); assert!( - unsets.is_empty(), - "the entry survives, so nothing is cleared" + matches!(&delta, MergeableMap::Map(_)), + "no key went away, so the map merges per key: {delta:?}" ); + assert_eq!(delta.len(), 1); } /// The `arguments` of a server entry, for asserting on a computed delta. @@ -283,10 +288,10 @@ fn a_dropped_stop_word_is_recorded_at_every_site() { #[test] fn map_delta_keeps_an_entry_only_next_has() { - let prev = IndexMap::new(); + let prev = MergeableMap::default(); let next = map(&["--a"]); - assert_eq!(delta_map(&prev, next.clone()), next); + assert_eq!(delta_mergeable_map(&prev, next.clone()), next); } #[test] @@ -294,7 +299,7 @@ fn map_delta_keeps_the_changed_fields_of_an_entry() { let prev = map(&["--a"]); let next = map(&["--a", "--b"]); - let delta = delta_map(&prev, next); + let delta = delta_mergeable_map(&prev, next); assert_eq!(delta.len(), 1); assert_eq!(arguments(&delta["kagi"]), Some(&vec!["--b".to_owned()])); @@ -308,7 +313,7 @@ fn map_delta_keeps_the_changed_fields_of_an_entry() { /// can now say `replace`, so the case is built directly. #[test] fn map_delta_drops_an_entry_whose_delta_is_empty() { - let entry = |command: &str| -> IndexMap { + let entry = |command: &str| -> MergeableMap { let mut map = IndexMap::new(); map.insert( "kagi".to_owned(), @@ -317,13 +322,13 @@ fn map_delta_drops_an_entry_whose_delta_is_empty() { ..PartialStdioConfig::default() }), ); - map + map.into() }; // Equal entries are dropped by the equality check ahead of the delta. - assert!(delta_map(&entry("serve"), entry("serve")).is_empty()); + assert!(delta_mergeable_map(&entry("serve"), entry("serve")).is_empty()); // A differing entry contributes only what changed. - let delta = delta_map(&entry("serve"), entry("other")); + let delta = delta_mergeable_map(&entry("serve"), entry("other")); assert_eq!(delta.len(), 1); } diff --git a/crates/jp_config/src/snapshots/jp_config__tests__partial_app_config_default.snap b/crates/jp_config/src/snapshots/jp_config__tests__partial_app_config_default.snap index b84555e35..19597f429 100644 --- a/crates/jp_config/src/snapshots/jp_config__tests__partial_app_config_default.snap +++ b/crates/jp_config/src/snapshots/jp_config__tests__partial_app_config_default.snap @@ -75,7 +75,9 @@ PartialAppConfig { }, access: None, }, - tools: {}, + tools: Map( + {}, + ), }, compaction: PartialCompactionConfig { rules: Vec( 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 1c77a3f04..cad679453 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 @@ -144,7 +144,9 @@ Ok( }, access: None, }, - tools: {}, + tools: Map( + {}, + ), }, compaction: PartialCompactionConfig { rules: Merged( diff --git a/crates/jp_config/src/snapshots/jp_config__tests__partial_app_config_empty_serialize.snap b/crates/jp_config/src/snapshots/jp_config__tests__partial_app_config_empty_serialize.snap index e7b22e986..0cb53742a 100644 --- a/crates/jp_config/src/snapshots/jp_config__tests__partial_app_config_empty_serialize.snap +++ b/crates/jp_config/src/snapshots/jp_config__tests__partial_app_config_empty_serialize.snap @@ -75,7 +75,9 @@ PartialAppConfig { }, access: None, }, - tools: {}, + tools: Map( + {}, + ), }, compaction: PartialCompactionConfig { rules: Vec( diff --git a/crates/jp_config/src/util.rs b/crates/jp_config/src/util.rs index f4f5cef43..223e036ec 100644 --- a/crates/jp_config/src/util.rs +++ b/crates/jp_config/src/util.rs @@ -417,7 +417,7 @@ pub fn log_load_diagnostics(partial: &PartialAppConfig) { "Configuration details." ); - for (name, tool) in &partial.conversation.tools.tools { + for (name, tool) in partial.conversation.tools.tools.iter() { if tool.source.is_none() { error!( tool = %name, From 2c8a9688536c01eb963893fc0682796125fbce2c Mon Sep 17 00:00:00 2001 From: Jean Mertz Date: Mon, 7 Sep 2026 16:27:11 +0200 Subject: [PATCH 08/16] refactor(config): Remove the strategy-less map merge `merge_nested_indexmap` merged two maps per key with no way for a config to ask for anything else. Every map field now carries a `MergeableMap`, whose `map_with_strategy` does the same per-key merge by default and honours a declared `deep_merge`, `merge`, `keep` or `replace`, so the older function has no callers left. Signed-off-by: Jean Mertz --- crates/jp_config/src/util.rs | 33 +-------------------------------- 1 file changed, 1 insertion(+), 32 deletions(-) diff --git a/crates/jp_config/src/util.rs b/crates/jp_config/src/util.rs index 223e036ec..4897d5373 100644 --- a/crates/jp_config/src/util.rs +++ b/crates/jp_config/src/util.rs @@ -9,8 +9,7 @@ use std::{ use camino::Utf8Path; use glob::glob; -use indexmap::IndexMap; -use schematic::{ConfigLoader, MergeError, MergeResult, PartialConfig}; +use schematic::{ConfigLoader, PartialConfig as _}; use tracing::{debug, error, info, trace, warn}; use crate::{ @@ -615,36 +614,6 @@ fn dedup_keep_last(entries: Vec) -> Vec { .collect() } -/// Merge [`IndexMap`]s of nested [`PartialConfig`]s. -/// -/// # Errors -/// -/// Returns an error if merging the partials fails, which returns a -/// [`schematic::MergeError`]. -pub fn merge_nested_indexmap( - prev: IndexMap, - mut next: IndexMap, - c: &C, -) -> MergeResult> -where - V: PartialConfig, - C: Default, -{ - let mut prev = prev - .into_iter() - .map(|(name, mut prev)| { - if let Some(next) = next.shift_remove(&name) { - prev.merge(c, next).map_err(MergeError::new)?; - } - - Ok((name, prev)) - }) - .collect::, _>>()?; - - prev.append(&mut next); - Ok(Some(prev)) -} - /// Define the name to serialize and deserialize for a unit variant. #[macro_export] macro_rules! named_unit_variant { From fc0f348ecfed49b3c9bc2ef8ebd04573c73b02da Mon Sep 17 00:00:00 2001 From: Jean Mertz Date: Tue, 8 Sep 2026 09:20:55 +0200 Subject: [PATCH 09/16] fix(config): Name a generic schema by its instantiation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A schema names each type it expands and refers back to that name wherever the type appears again, so a name has to identify one type. A generic named after its base alone does not: `MergeableMap` and `MergeableMap` both answered `MergeableMap`, and a consumer resolving a reference by name walked a value against whichever of them it met first. That reached users through stored conversations. Stripping a stored config walks it against the schema to drop keys a newer release wrote, and resolving a tool's `parameters` to the map of tools instead walks each parameter against `ToolConfig` — deleting valid keys, or leaving a stale one behind for typed deserialization to reject, which discards the whole stored config rather than the key. The arguments are appended, so the two are `MergeableMap_ToolConfig` and `MergeableMap_ToolParameterConfig`. An argument with no name of its own contributes nothing, which leaves the base name for a type generic only over primitives. Signed-off-by: Jean Mertz --- .../schematic_macros/src/config/mod.rs | 45 ++++++++++++++++++- crates/jp_config/src/types/map_tests.rs | 33 ++++++++++++++ 2 files changed, 76 insertions(+), 2 deletions(-) diff --git a/crates/contrib/schematic_macros/src/config/mod.rs b/crates/contrib/schematic_macros/src/config/mod.rs index 77c216633..bd3021e3a 100644 --- a/crates/contrib/schematic_macros/src/config/mod.rs +++ b/crates/contrib/schematic_macros/src/config/mod.rs @@ -135,6 +135,44 @@ struct SchematicImplArgs<'a> { instrument: &'a TokenStream, } +/// The body of a `schema_name` that says which instantiation it describes. +/// +/// A schema names each type it expands and refers back to that name wherever +/// the type appears again, so a name has to identify one type. +/// A generic named after its base alone does not: `MergeableMap` +/// and `MergeableMap` would both answer `MergeableMap`, +/// and a consumer resolving a reference by name would walk a value against +/// whichever of them it met first. +/// +/// The arguments are appended instead, so the two are `MergeableMap_ToolConfig` +/// and `MergeableMap_ToolParameterConfig`. +/// An argument with no name of its own (a primitive) contributes nothing, which +/// leaves the base name for a type generic only over those. +#[cfg(feature = "schema")] +fn generate_schema_name(base: &str, generics: &syn::Generics) -> TokenStream { + let type_params = generics.type_params().map(|param| ¶m.ident); + let mut appends = type_params.peekable(); + + if appends.peek().is_none() { + return quote! { Some(#base.into()) }; + } + + let appends = appends.map(|ident| { + quote! { + if let Some(argument) = <#ident as schematic::Schematic>::schema_name() { + name.push('_'); + name.push_str(&argument); + } + } + }); + + quote! { + let mut name = String::from(#base); + #(#appends)* + Some(name) + } +} + #[cfg(feature = "schema")] fn emit_schematic_impls(args: &SchematicImplArgs<'_>) -> TokenStream { let &SchematicImplArgs { @@ -153,6 +191,9 @@ fn emit_schematic_impls(args: &SchematicImplArgs<'_>) -> TokenStream { let partial_schema_name = partial_name.to_string(); let partial_schema_impl = crate::common::Container::generate_partial_schema(name, cfg.generics); + let schema_name_impl = generate_schema_name(&schema_name, cfg.generics); + let partial_schema_name_impl = generate_schema_name(&partial_schema_name, cfg.generics); + // `schema_union_with` unions the derived schema with caller-supplied // variants, for a type that deserializes from more shapes than its fields // describe. Using it asserts the extra shapes are shorthands for the fields, @@ -180,7 +221,7 @@ fn emit_schematic_impls(args: &SchematicImplArgs<'_>) -> TokenStream { #[automatically_derived] impl #impl_generics schematic::Schematic for #name #ty_generics #schematic_where { fn schema_name() -> Option { - Some(#schema_name.into()) + #schema_name_impl } #instrument @@ -194,7 +235,7 @@ fn emit_schematic_impls(args: &SchematicImplArgs<'_>) -> TokenStream { #[automatically_derived] impl #impl_generics schematic::Schematic for #partial_name #ty_generics #partial_schematic_where { fn schema_name() -> Option { - Some(#partial_schema_name.into()) + #partial_schema_name_impl } #instrument diff --git a/crates/jp_config/src/types/map_tests.rs b/crates/jp_config/src/types/map_tests.rs index d6999b382..b21ffee9a 100644 --- a/crates/jp_config/src/types/map_tests.rs +++ b/crates/jp_config/src/types/map_tests.rs @@ -2,6 +2,39 @@ use serde_json::json; use super::*; +/// A schema names each type it expands and refers back to that name below, so +/// two instantiations of one generic have to answer differently. +/// +/// Sharing a name makes a consumer that resolves a reference walk a value +/// against whichever instantiation it met first: a tool's `parameters` read as +/// the map of tools, whose entries are a different type entirely. +#[test] +fn a_generic_schema_is_named_by_its_instantiation() { + use schematic::Schematic as _; + + use crate::{conversation::label::LabelConfig, providers::mcp::McpProviderConfig}; + + assert_eq!( + MergeableMap::::schema_name().as_deref(), + Some("MergeableMap_LabelConfig") + ); + assert_eq!( + MergeableMap::::schema_name().as_deref(), + Some("MergeableMap_McpProviderConfig") + ); +} + +/// An argument with no name of its own leaves the base name alone. +#[test] +fn a_generic_over_a_primitive_keeps_its_base_name() { + use schematic::Schematic as _; + + assert_eq!( + MergeableMap::::schema_name().as_deref(), + Some("MergeableMap") + ); +} + #[test] fn deserialize_plain_map() { let v: MergeableMap = From 540f7be4ec1e26c25cf9ed862da4c4f609797f75 Mon Sep 17 00:00:00 2001 From: Jean Mertz Date: Tue, 8 Sep 2026 09:21:20 +0200 Subject: [PATCH 10/16] fix(conversation): Strip through a map that states its merge strategy Stripping a stored config walks it against the schema and removes the keys the schema has no field for, so a key an older release does not know is dropped rather than left to fail typed deserialization, which would discard the whole stored config. A map that can state its own merge strategy is described as the plain map beside the wrapper holding it under `value`. Both are objects on the wire, so shape alone left the union ambiguous and the walk stopped: every key inside a tool, server, alias or plugin went unvisited. The variants are now told apart the way the wrapper's own deserializer does it, by whether the value carries `value` and `strategy` together, so a tool called `value` is still a tool. A flattened map is resolved the same way, which is what lets the entries of `conversation.tools` be walked at all. Signed-off-by: Jean Mertz --- crates/jp_conversation/src/compat.rs | 69 +++++++++++++++++----------- 1 file changed, 43 insertions(+), 26 deletions(-) diff --git a/crates/jp_conversation/src/compat.rs b/crates/jp_conversation/src/compat.rs index 513dc775e..42dd82c07 100644 --- a/crates/jp_conversation/src/compat.rs +++ b/crates/jp_conversation/src/compat.rs @@ -343,30 +343,24 @@ fn sole_matching_variant<'a>(union_type: &'a UnionType, value: &Value) -> Option /// map. /// An entry named `value` needs the sibling `strategy` before it reads as the /// wrapper, which is what keeps a tool called `value` addressable. +/// +/// The pair is recognised by one side being a map rather than by the wrapper's +/// own fields: the wrapper type is described once and referred to by name +/// wherever it appears again, so most of its uses are a reference with no +/// fields to inspect. fn strategy_carrying_variant<'a>(union_type: &'a UnionType, value: &Value) -> Option<&'a Schema> { - let mut variants = union_type - .variants_types - .iter() - .map(Box::as_ref) - .filter(|variant| !variant.is_null()); - - let (first, second) = (variants.next()?, variants.next()?); - if variants.next().is_some() { - return None; - } - - let is_wrapper = |schema: &Schema| { - matches!(&schema.ty, SchemaType::Struct(wrapper) - if wrapper.fields.contains_key("value") && wrapper.fields.contains_key("strategy")) + let variants = || { + union_type + .variants_types + .iter() + .map(Box::as_ref) + .filter(|variant| !variant.is_null()) }; - let (wrapper, collection) = if is_wrapper(first) { - (first, second) - } else if is_wrapper(second) { - (second, first) - } else { - return None; - }; + let is_map = |schema: &Schema| matches!(schema.ty, SchemaType::Object(_)); + + let collection = sole(variants().filter(|variant| is_map(variant)))?; + let wrapper = sole(variants().filter(|variant| !is_map(variant)))?; let stated = value .as_object() @@ -439,7 +433,8 @@ fn strip_struct<'a>( return 0; }; - let entry_schema = flattened_entry_schema(struct_type); + let flattened = flattened_field_schema(struct_type); + let entry_schema = flattened.and_then(map_value_schema); let has_flatten = struct_type.fields.values().any(|f| f.flatten); let mut stripped = if has_flatten { @@ -473,7 +468,7 @@ fn strip_struct<'a>( /// flattens something other than a map — in each of those cases the shape of a /// leftover key is not knowable, and walking it against the wrong schema would /// delete valid data. -fn flattened_entry_schema(struct_type: &StructType) -> Option<&Schema> { +fn flattened_field_schema(struct_type: &StructType) -> Option<&Schema> { let mut flattened = struct_type .fields .values() @@ -481,11 +476,33 @@ fn flattened_entry_schema(struct_type: &StructType) -> Option<&Schema> { .map(Box::as_ref); match (flattened.next(), flattened.next()) { - (Some(SchemaField { schema, .. }), None) => match &schema.ty { + (Some(SchemaField { schema, .. }), None) => Some(schema), + _ => None, + } +} + +/// The schema of a map's values, for a map written either plainly or with a +/// stated merge strategy. +/// +/// A map that can state one is a union of the plain map and the wrapper holding +/// it under `value`. +/// Flattened, its entries are sibling keys of the struct around it, which is +/// the plain map's shape, so that is the variant their values are walked +/// against. +fn map_value_schema(schema: &Schema) -> Option<&Schema> { + fn value_type(ty: &SchemaType) -> Option<&Schema> { + match ty { SchemaType::Object(object_type) => Some(&object_type.value_type), _ => None, - }, - _ => None, + } + } + + match &schema.ty { + SchemaType::Union(union_type) => union_type + .variants_types + .iter() + .find_map(|variant| value_type(&variant.ty)), + ty => value_type(ty), } } From e835ab098c6fa944f921c83d449233804eceb227 Mon Sep 17 00:00:00 2001 From: Jean Mertz Date: Tue, 8 Sep 2026 09:22:15 +0200 Subject: [PATCH 11/16] fix(config): Fill the tools map key by key through its wrapper A tool the workspace config gained after a conversation was created still reaches that conversation, as it did before the map could state its own merge strategy. Filling took the conversation's map whole, so a tool added later was invisible to every conversation that predated it. A map that states a strategy is left alone instead: its owner said how it combines, and filling gaps into it would answer differently. Only the styles of the tools it holds are filled, which is what carries a single `[conversation.tools.'*'.style]` key to each of them. Signed-off-by: Jean Mertz --- crates/jp_config/src/conversation/tool.rs | 45 ++++++++++++++++------- crates/jp_config/src/plugins.rs | 4 +- crates/jp_config/src/providers.rs | 4 +- crates/jp_config/src/providers/llm.rs | 4 +- 4 files changed, 35 insertions(+), 22 deletions(-) diff --git a/crates/jp_config/src/conversation/tool.rs b/crates/jp_config/src/conversation/tool.rs index de4bb9c86..2ae880b31 100644 --- a/crates/jp_config/src/conversation/tool.rs +++ b/crates/jp_config/src/conversation/tool.rs @@ -99,22 +99,41 @@ impl FillDefaults for PartialToolsConfig { // tool's grants must be complete where they are written, so the `*` // block applies whole or not at all, `fs` and `env` together (resolved // in `ToolConfigWithDefaults::access`). - let tools = self - .tools - .into_iter() - .map(|(name, mut tool)| { - tool.style = tool - .style - .map(|style| style.fill_from(tool_defaults.style.clone())); + let fill_style = |mut tool: PartialToolConfig| { + tool.style = tool + .style + .map(|style| style.fill_from(tool_defaults.style.clone())); + tool + }; - (name, tool) - }) - .collect::>() - .into(); + let tools = match self.tools { + // A map that states a strategy said how it combines, so only its + // tools' styles are filled and no default tool joins them. + MergeableMap::Merged(mut merged) => { + merged.value = merged + .value + .into_iter() + .map(|(name, tool)| (name, fill_style(tool))) + .collect(); + + MergeableMap::Merged(merged) + } + + // Key by key, so a tool only the defaults declare is added while + // one this layer already has keeps its own value. + MergeableMap::Map(entries) => { + let entries = entries + .into_iter() + .map(|(name, tool)| (name, fill_style(tool))) + .collect(); + + fill_map(entries, defaults.tools.into_map()).into() + } + }; Self { defaults: tool_defaults, - tools: fill_map(tools, defaults.tools), + tools, } } } @@ -238,7 +257,7 @@ fn reject_comma_in_tool_names(tools: &ToolsConfig) -> Result<(), ConfigError> { /// reporting; the `'*'` defaults make no claim about any individual tool, so /// they pass over builtin and MCP tools instead of failing the whole config. fn reject_access_on_non_local_tools(tools: &ToolsConfig) -> Result<(), ConfigError> { - for (name, tool) in &tools.tools { + for (name, tool) in tools.tools.iter() { if tool.access.is_none() { continue; } diff --git a/crates/jp_config/src/plugins.rs b/crates/jp_config/src/plugins.rs index 27ec6c238..05b9d8766 100644 --- a/crates/jp_config/src/plugins.rs +++ b/crates/jp_config/src/plugins.rs @@ -84,9 +84,7 @@ impl FillDefaults for PartialPluginsConfig { // that states a strategy is left alone. command: match self.command { merged @ MergeableMap::Merged(_) => merged, - MergeableMap::Map(entries) => { - fill_map(entries, defaults.command.into_map()).into() - } + MergeableMap::Map(entries) => fill_map(entries, defaults.command.into_map()).into(), }, } } diff --git a/crates/jp_config/src/providers.rs b/crates/jp_config/src/providers.rs index 0fdc6b2a2..7a683ed7c 100644 --- a/crates/jp_config/src/providers.rs +++ b/crates/jp_config/src/providers.rs @@ -103,9 +103,7 @@ impl FillDefaults for PartialProviderConfig { // combines, and filling gaps into it would answer differently. mcp: match self.mcp { merged @ MergeableMap::Merged(_) => merged, - MergeableMap::Map(entries) => { - fill_map(entries, defaults.mcp.into_map()).into() - } + MergeableMap::Map(entries) => fill_map(entries, defaults.mcp.into_map()).into(), }, } } diff --git a/crates/jp_config/src/providers/llm.rs b/crates/jp_config/src/providers/llm.rs index 02b73d596..fc85f8f7f 100644 --- a/crates/jp_config/src/providers/llm.rs +++ b/crates/jp_config/src/providers/llm.rs @@ -168,9 +168,7 @@ impl FillDefaults for PartialLlmProviderConfig { // that states a strategy is left alone. aliases: match self.aliases { merged @ MergeableMap::Merged(_) => merged, - MergeableMap::Map(entries) => { - fill_map(entries, defaults.aliases.into_map()).into() - } + MergeableMap::Map(entries) => fill_map(entries, defaults.aliases.into_map()).into(), }, anthropic: self.anthropic.fill_from(defaults.anthropic), cerebras: self.cerebras.fill_from(defaults.cerebras), From 348aa95dae33ac0a0c81228f3d68e27f76bdb236 Mon Sep 17 00:00:00 2001 From: Jean Mertz Date: Fri, 18 Sep 2026 22:14:43 +0200 Subject: [PATCH 12/16] fix(config, cli): Reconcile the map conversion with main Restores a test-only `now` whose `cfg` attribute a structural merge detached, and carries the `MergeableMap` hand-off into the plugin dispatch path main added since. The schema probe stops inventing strings below a collection, so a tool's `source` is left out of a document probing something else, and `MergeableMap` commits to the wrapper once a table carries both `value` and `strategy`, so a misspelled strategy is an error rather than an entry by that name. Signed-off-by: Jean Mertz --- .../src/cmd/conversation/print_tests.rs | 6 +- crates/jp_cli/src/cmd/plugin/dispatch.rs | 2 +- .../jp_cli/src/cmd/query/turn_loop_tests.rs | 18 +- crates/jp_cli/src/ctx.rs | 160 +-- crates/jp_config/src/providers.rs | 2 +- crates/jp_config/src/providers/llm.rs | 2 +- crates/jp_config/src/schema_probe.rs | 11 +- ...onfig__tests__app_config_schema_shape.snap | 1100 +++++++++++++---- crates/jp_config/src/types/map.rs | 15 +- 9 files changed, 963 insertions(+), 353 deletions(-) diff --git a/crates/jp_cli/src/cmd/conversation/print_tests.rs b/crates/jp_cli/src/cmd/conversation/print_tests.rs index 9c91d8fe0..88a51f00a 100644 --- a/crates/jp_cli/src/cmd/conversation/print_tests.rs +++ b/crates/jp_cli/src/cmd/conversation/print_tests.rs @@ -70,14 +70,14 @@ fn tool_with_style(style: DisplayStyleConfig) -> ToolConfig { summary: None, description: None, examples: None, - parameters: IndexMap::new(), + parameters: IndexMap::new().into(), run: None, format: None, result: None, cancellation_response: None, style: Some(style), - questions: IndexMap::new(), - options: IndexMap::new(), + questions: IndexMap::new().into(), + options: IndexMap::new().into(), access: None, } } diff --git a/crates/jp_cli/src/cmd/plugin/dispatch.rs b/crates/jp_cli/src/cmd/plugin/dispatch.rs index 7ff21bb79..3066a7a93 100644 --- a/crates/jp_cli/src/cmd/plugin/dispatch.rs +++ b/crates/jp_cli/src/cmd/plugin/dispatch.rs @@ -937,7 +937,7 @@ async fn prepare_turn( // added to the workspace since then is otherwise unknown to it, and starting // it fails. ctx.mcp_client - .set_servers(config.providers.mcp.clone()) + .set_servers(config.providers.mcp.clone().into_map()) .await; // Shared, because a turn spawned earlier may still be using a server this diff --git a/crates/jp_cli/src/cmd/query/turn_loop_tests.rs b/crates/jp_cli/src/cmd/query/turn_loop_tests.rs index 37274fd45..a41646ca1 100644 --- a/crates/jp_cli/src/cmd/query/turn_loop_tests.rs +++ b/crates/jp_cli/src/cmd/query/turn_loop_tests.rs @@ -4939,11 +4939,11 @@ fn talking_tool_config(names: &[&str]) -> AppConfig { summary: None, description: None, examples: None, - parameters: IndexMap::new(), + parameters: IndexMap::new().into(), result: None, style: None, - questions: IndexMap::new(), - options: IndexMap::default(), + questions: IndexMap::new().into(), + options: IndexMap::default().into(), access: None, cancellation_response: None, }); @@ -5435,7 +5435,7 @@ async fn a_tool_can_opt_out_of_the_progress_window() { summary: None, description: None, examples: None, - parameters: IndexMap::new(), + parameters: IndexMap::new().into(), result: None, style: Some(DisplayStyleConfig { hidden: false, @@ -5449,8 +5449,8 @@ async fn a_tool_can_opt_out_of_the_progress_window() { results_file_link: None, }, }), - questions: IndexMap::new(), - options: IndexMap::default(), + questions: IndexMap::new().into(), + options: IndexMap::default().into(), access: None, cancellation_response: None, }); @@ -8103,11 +8103,11 @@ async fn a_tool_that_does_not_join_reasoning_renders_unshaded_live() { summary: None, description: None, examples: None, - parameters: IndexMap::new(), + parameters: IndexMap::new().into(), result: None, style: Some(non_joining_style()), - questions: IndexMap::new(), - options: IndexMap::default(), + questions: IndexMap::new().into(), + options: IndexMap::default().into(), access: None, cancellation_response: None, }); diff --git a/crates/jp_cli/src/ctx.rs b/crates/jp_cli/src/ctx.rs index 6d1dfe17d..ddd57ca44 100644 --- a/crates/jp_cli/src/ctx.rs +++ b/crates/jp_cli/src/ctx.rs @@ -118,61 +118,61 @@ pub(crate) struct Term { impl Ctx { /// Create a new context with the given workspace pub(crate) fn new( - exec: ExecutionContext, - workspace: Workspace, - fs_backend: Option>, - runtime: Runtime, - args: Globals, - config: impl Into>, - session: Option, - printer: Printer, -) -> Self { - let config = config.into(); - let escalation_cooldown = - Duration::from_secs(config.interrupt.escalation_cooldown_secs.into()); - let mcp_client = jp_mcp::Client::new(config.providers.mcp.clone().into_map()) - .with_child_cwd(exec.child_cwd().map(|cwd| cwd.as_std_path().to_path_buf())); - - let is_tty = io::stdout().is_terminal(); - let width = printer.output_width().columns(); - - let interactive = crate::interactive(args.no_interactive, is_tty); - - Self { - exec, - workspace, - fs_backend, - config, - term: Term { - args, - is_tty, - interactive, - width, - }, - session, - printer: Arc::new(printer), - mcp_client, - task_handler: TaskHandler::default(), - signals: SignalRouter::new(&runtime, escalation_cooldown), - config_reset: None, - runtime, - - #[cfg(test)] - stubbed_now: DateTime::::UNIX_EPOCH, + exec: ExecutionContext, + workspace: Workspace, + fs_backend: Option>, + runtime: Runtime, + args: Globals, + config: impl Into>, + session: Option, + printer: Printer, + ) -> Self { + let config = config.into(); + let escalation_cooldown = + Duration::from_secs(config.interrupt.escalation_cooldown_secs.into()); + let mcp_client = jp_mcp::Client::new(config.providers.mcp.clone().into_map()) + .with_child_cwd(exec.child_cwd().map(|cwd| cwd.as_std_path().to_path_buf())); + + let is_tty = io::stdout().is_terminal(); + let width = printer.output_width().columns(); + + let interactive = crate::interactive(args.no_interactive, is_tty); + + Self { + exec, + workspace, + fs_backend, + config, + term: Term { + args, + is_tty, + interactive, + width, + }, + session, + printer: Arc::new(printer), + mcp_client, + task_handler: TaskHandler::default(), + signals: SignalRouter::new(&runtime, escalation_cooldown), + config_reset: None, + runtime, + + #[cfg(test)] + stubbed_now: DateTime::::UNIX_EPOCH, + } } -} #[cfg(not(test))] #[expect(clippy::unused_self)] pub(crate) fn now(&self) -> DateTime { Utc::now() } + + #[cfg(test)] pub(crate) fn now(&self) -> DateTime { self.stubbed_now } - #[cfg(test)] - #[cfg(test)] pub(crate) fn set_now(&mut self, now: DateTime) { self.stubbed_now = now; @@ -180,17 +180,17 @@ impl Ctx { /// Returns the storage path, if filesystem storage is configured. pub(crate) fn storage_path(&self) -> Option<&Utf8Path> { - self.fs_backend - .as_deref() - .map(FsStorageBackend::storage_path) -} + self.fs_backend + .as_deref() + .map(FsStorageBackend::storage_path) + } /// Returns the user storage path, if filesystem storage is configured. pub(crate) fn user_storage_path(&self) -> Option<&Utf8Path> { - self.fs_backend - .as_deref() - .and_then(FsStorageBackend::user_storage_path) -} + self.fs_backend + .as_deref() + .and_then(FsStorageBackend::user_storage_path) + } /// Get immutable access to the configuration. /// @@ -203,8 +203,8 @@ impl Ctx { /// configuration" API in [`jp_config`] *before* constructing the final /// [`AppConfig`] object. pub(crate) fn config(&self) -> Arc { - self.config.clone() -} + self.config.clone() + } /// Install a resolved config for one scoped run, returning the previous /// one. @@ -220,13 +220,13 @@ impl Ctx { /// rather than a mutation: assembling a config is still the partial API's /// job. pub(crate) fn swap_config(&mut self, config: Arc) -> Arc { - std::mem::replace(&mut self.config, config) -} + std::mem::replace(&mut self.config, config) + } /// Get a runtime handle. pub(crate) fn handle(&self) -> &Handle { - self.runtime.handle() -} + self.runtime.handle() + } /// Activate and deactivate MCP servers based on the active conversation /// context. @@ -237,31 +237,31 @@ impl Ctx { /// or `tool_definitions` drops the tool again as unreachable and the forced /// choice cannot be satisfied. pub(crate) async fn configure_active_mcp_servers( - &mut self, - forced_tool: Option<&str>, - scope: McpServerScope, -) -> Result { - let mut server_ids = HashSet::new(); - - for (name, cfg) in self.config.conversation.tools.iter() { - if !cfg.is_enabled() && forced_tool != Some(name) { - continue; + &mut self, + forced_tool: Option<&str>, + scope: McpServerScope, + ) -> Result { + let mut server_ids = HashSet::new(); + + for (name, cfg) in self.config.conversation.tools.iter() { + if !cfg.is_enabled() && forced_tool != Some(name) { + continue; + } + + let ToolSource::Mcp { server, .. } = &cfg.source() else { + continue; + }; + + server_ids.insert(McpServerId::new(server)); } - let ToolSource::Mcp { server, .. } = &cfg.source() else { - continue; - }; - - server_ids.insert(McpServerId::new(server)); - } - - let handle = self.handle().clone(); - match scope { - McpServerScope::Exclusive => self.mcp_client.run_services(server_ids, handle).await, - McpServerScope::Shared => self.mcp_client.start_services(server_ids, handle).await, + let handle = self.handle().clone(); + match scope { + McpServerScope::Exclusive => self.mcp_client.run_services(server_ids, handle).await, + McpServerScope::Shared => self.mcp_client.start_services(server_ids, handle).await, + } + .map_err(Into::into) } - .map_err(Into::into) -} } /// Whether a turn has the MCP client to itself. diff --git a/crates/jp_config/src/providers.rs b/crates/jp_config/src/providers.rs index 7a683ed7c..416ca3563 100644 --- a/crates/jp_config/src/providers.rs +++ b/crates/jp_config/src/providers.rs @@ -15,8 +15,8 @@ use crate::{ llm::{LlmProviderConfig, PartialLlmProviderConfig}, mcp::McpProviderConfig, }, - validate::Validator, types::map::{MergeableMap, map_to_partial_per_key}, + validate::Validator, }; /// Provider configuration. diff --git a/crates/jp_config/src/providers/llm.rs b/crates/jp_config/src/providers/llm.rs index fc85f8f7f..68deb64ea 100644 --- a/crates/jp_config/src/providers/llm.rs +++ b/crates/jp_config/src/providers/llm.rs @@ -29,8 +29,8 @@ use crate::{ openai::{OpenaiConfig, PartialOpenaiConfig}, openrouter::{OpenrouterConfig, PartialOpenrouterConfig}, }, - validate::Validator, types::map::{MergeableMap, map_to_partial_per_key}, + validate::Validator, }; /// Provider configuration. diff --git a/crates/jp_config/src/schema_probe.rs b/crates/jp_config/src/schema_probe.rs index 9fc0b14ce..27363b562 100644 --- a/crates/jp_config/src/schema_probe.rs +++ b/crates/jp_config/src/schema_probe.rs @@ -209,14 +209,17 @@ fn collect_type( } } } + // A collection's contents sit below the keys that identify its variant, + // so the document is unambiguous there and the conservative fill + // applies, exactly as it does below a struct's own keys. SchemaType::Object(object) => { path.push(Step::MapEntry); - collect(&object.value_type, path, out, enclosing, strings); + collect(&object.value_type, path, out, enclosing, StringFill::Skip); path.pop(); } SchemaType::Array(array) => { path.push(Step::Item); - collect(&array.items_type, path, out, enclosing, strings); + collect(&array.items_type, path, out, enclosing, StringFill::Skip); path.pop(); } SchemaType::Union(union) => { @@ -419,12 +422,12 @@ fn collect_rejections( } SchemaType::Object(object) => { path.push(Step::MapEntry); - collect_rejections(&object.value_type, path, out, enclosing, strings); + collect_rejections(&object.value_type, path, out, enclosing, StringFill::Skip); path.pop(); } SchemaType::Array(array) => { path.push(Step::Item); - collect_rejections(&array.items_type, path, out, enclosing, strings); + collect_rejections(&array.items_type, path, out, enclosing, StringFill::Skip); path.pop(); } SchemaType::Union(union) => { 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 8bd969950..5a6bd931f 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 @@ -3,7 +3,7 @@ source: crates/jp_config/src/lib_tests.rs expression: "crate::schema_shape::render(&AppConfig::schema())" --- assistant: AssistantConfig - instructions?: MergeableVec + instructions?: MergeableVec_InstructionsConfig |: []: InstructionsConfig description: string | null @@ -17,7 +17,7 @@ assistant: AssistantConfig items: [string] position?: int title: string | null - |: MergedVec + |: MergedVec_InstructionsConfig dedup?: "inherit" | "true" | "false" | bool | null discard_when_merged?: bool strategy?: "append" | "prepend" | "replace" | null @@ -79,14 +79,14 @@ assistant: AssistantConfig separator?: "none" | "space" | "line" | "paragraph" strategy?: "append" | "prepend" | "replace" value?: string - system_prompt_sections?: MergeableVec + system_prompt_sections?: MergeableVec_SectionConfig |: []: SectionConfig content: string position?: int tag: string | null title: string | null - |: MergedVec + |: MergedVec_SectionConfig dedup?: "inherit" | "true" | "false" | bool | null discard_when_merged?: bool strategy?: "append" | "prepend" | "replace" | null @@ -105,7 +105,7 @@ config_load_paths: MergeableVec strategy?: "append" | "prepend" | "replace" | null value?: [string] conversation: ConversationConfig - attachments?: MergeableVec + attachments?: MergeableVec_AttachmentConfig |: []: AttachmentConfig |: string @@ -114,7 +114,7 @@ conversation: ConversationConfig *: unknown path: string type: string - |: MergedVec + |: MergedVec_AttachmentConfig dedup?: "inherit" | "true" | "false" | bool | null discard_when_merged?: bool strategy?: "append" | "prepend" | "replace" | null @@ -127,7 +127,7 @@ conversation: ConversationConfig path: string type: string compaction: CompactionConfig - rules?: MergeableVec + rules?: MergeableVec_CompactionRuleConfig |: []: CompactionRuleConfig keep_first?: int | string @@ -163,7 +163,13 @@ conversation: ConversationConfig exclude?: bool |: null service_tier: "off" | "flex" | "standard" | "priority" | null - stop_words?: @MergeableVec + 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 @@ -177,7 +183,7 @@ conversation: ConversationConfig over: int | string policy: "strip" (aka s) | "strip-responses" (aka strip_responses, sres) | "strip-requests" (aka strip_requests, sreq) | "omit" (aka o) |: null - |: MergedVec + |: MergedVec_CompactionRuleConfig dedup?: "inherit" | "true" | "false" | bool | null discard_when_merged?: bool strategy?: "append" | "prepend" | "replace" | null @@ -216,7 +222,13 @@ conversation: ConversationConfig exclude?: bool |: null service_tier: "off" | "flex" | "standard" | "priority" | null - stop_words?: @MergeableVec + 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 @@ -233,7 +245,7 @@ conversation: ConversationConfig default_id: "ask" | "last-activated" (aka last, last_activated) | "last-created" (aka last_created) | "previous" (aka prev) | string | null inquiry: InquiryConfig assistant: AssistantConfig - instructions?: MergeableVec + instructions?: MergeableVec_InstructionsConfig |: []: InstructionsConfig description: string | null @@ -247,7 +259,7 @@ conversation: ConversationConfig items: [string] position?: int title: string | null - |: MergedVec + |: MergedVec_InstructionsConfig dedup?: "inherit" | "true" | "false" | bool | null discard_when_merged?: bool strategy?: "append" | "prepend" | "replace" | null @@ -309,14 +321,14 @@ conversation: ConversationConfig separator?: "none" | "space" | "line" | "paragraph" strategy?: "append" | "prepend" | "replace" value?: string - system_prompt_sections?: MergeableVec + system_prompt_sections?: MergeableVec_SectionConfig |: []: SectionConfig content: string position?: int tag: string | null title: string | null - |: MergedVec + |: MergedVec_SectionConfig dedup?: "inherit" | "true" | "false" | bool | null discard_when_merged?: bool strategy?: "append" | "prepend" | "replace" | null @@ -327,7 +339,7 @@ conversation: ConversationConfig tag: string | null title: string | null tool_choice?: "auto" | "none" | "required" | string - labels: MergeableMap + labels: MergeableMap_LabelConfig |: *: LabelConfig |: string @@ -348,7 +360,7 @@ conversation: ConversationConfig args?: [string] program: string shell?: bool - |: MergedMap + |: MergedMap_LabelConfig discard_when_merged?: bool strategy?: "deep_merge" | "merge" | "keep" | "replace" | null value?: @@ -411,12 +423,12 @@ conversation: ConversationConfig *: ToolsDefaultsConfig access: |: AccessConfig - env: MergeableVec + env: MergeableVec_EnvRuleConfig |: []: EnvRuleConfig name: string read: bool | null - |: MergedVec + |: MergedVec_EnvRuleConfig dedup?: "inherit" | "true" | "false" | bool | null discard_when_merged?: bool strategy?: "append" | "prepend" | "replace" | null @@ -424,7 +436,7 @@ conversation: ConversationConfig []: EnvRuleConfig name: string read: bool | null - fs: MergeableVec + fs: MergeableVec_FsRuleConfig |: []: FsRuleConfig create: bool | null @@ -435,7 +447,7 @@ conversation: ConversationConfig read: bool | null update: bool | null write: bool | null - |: MergedVec + |: MergedVec_FsRuleConfig dedup?: "inherit" | "true" | "false" | bool | null discard_when_merged?: bool strategy?: "append" | "prepend" | "replace" | null @@ -481,39 +493,26 @@ conversation: ConversationConfig shell?: bool print_stderr?: bool results_file_link?: "full" | "osc8" | "off" - tools (flattened): - *: ToolConfig - access: - |: AccessConfig - env: MergeableVec - |: - []: EnvRuleConfig - name: string - read: bool | null - |: MergedVec - dedup?: "inherit" | "true" | "false" | bool | null - discard_when_merged?: bool - strategy?: "append" | "prepend" | "replace" | null - value?: + tools (flattened): MergeableMap_ToolConfig + |: + *: ToolConfig + access: + |: AccessConfig + env: MergeableVec_EnvRuleConfig + |: []: EnvRuleConfig name: string read: bool | null - fs: MergeableVec - |: - []: FsRuleConfig - create: bool | null - delete: bool | null - execute: bool | null - external: bool | null - path: string - read: bool | null - update: bool | null - write: bool | null - |: MergedVec - dedup?: "inherit" | "true" | "false" | bool | null - discard_when_merged?: bool - strategy?: "append" | "prepend" | "replace" | null - value?: + |: MergedVec_EnvRuleConfig + dedup?: "inherit" | "true" | "false" | bool | null + discard_when_merged?: bool + strategy?: "append" | "prepend" | "replace" | null + value?: + []: EnvRuleConfig + name: string + read: bool | null + fs: MergeableVec_FsRuleConfig + |: []: FsRuleConfig create: bool | null delete: bool | null @@ -523,69 +522,84 @@ conversation: ConversationConfig read: bool | null update: bool | null write: bool | null - |: null - cancellation_response: string | null - command: - |: CommandConfigOrString - |: string - | (expanded): CommandConfig - args?: [string] - program: string - shell?: bool - |: null - description: string | null - enable: - |: - |: bool - |: "on" | "off" | "always" | "explicit" - | (expanded): EnableConfig - allow_toggle: "any" | "never" | "if_named" | "if_named_or_group" | null - state: bool | null - |: null - examples: string | null - format: "ask" | "unattended" | null - options: - *: unknown - parameters: - *: ToolParameterConfig - default?: unknown | null - description?: string | null - enum?: [unknown] | null - examples?: string | null - items?: @ToolParameterConfig | null - properties?: - *: @ToolParameterConfig - required?: bool | null - summary?: string | null - type?: string | [string] | null - questions: - *: QuestionConfig - answer: unknown | null - target: QuestionTarget - |: "user" | "assistant" - |: PartialAssistantConfig - instructions?: - |: PartialMergeableVec - |: - []: PartialInstructionsConfig - description?: string | null - examples?: - |: - []: PartialExampleConfig - |: string - |: PartialContrastConfig - bad?: string | null - good?: string | null - reason?: string | null - |: null - items?: [string] | null - position?: int | null - title?: string | null - |: PartialMergedVec - dedup?: "inherit" | "true" | "false" | bool | null | null - discard_when_merged?: bool | null - strategy?: "append" | "prepend" | "replace" | null - value?: + |: MergedVec_FsRuleConfig + dedup?: "inherit" | "true" | "false" | bool | null + discard_when_merged?: bool + strategy?: "append" | "prepend" | "replace" | null + value?: + []: FsRuleConfig + create: bool | null + delete: bool | null + execute: bool | null + external: bool | null + path: string + read: bool | null + update: bool | null + write: bool | null + |: null + cancellation_response: string | null + command: + |: CommandConfigOrString + |: string + | (expanded): CommandConfig + args?: [string] + program: string + shell?: bool + |: null + description: string | null + enable: + |: + |: bool + |: "on" | "off" | "always" | "explicit" + | (expanded): EnableConfig + allow_toggle: "any" | "never" | "if_named" | "if_named_or_group" | null + state: bool | null + |: null + examples: string | null + format: "ask" | "unattended" | null + options: MergeableMap_JsonValue + |: + *: unknown + |: MergedMap_JsonValue + discard_when_merged?: bool + strategy?: "deep_merge" | "merge" | "keep" | "replace" | null + value?: + *: unknown + parameters: MergeableMap_ToolParameterConfig + |: + *: ToolParameterConfig + default?: unknown | null + description?: string | null + enum?: [unknown] | null + examples?: string | null + items?: @ToolParameterConfig | null + properties?: @MergeableMap_ToolParameterConfig + required?: bool | null + summary?: string | null + type?: string | [string] | null + |: MergedMap_ToolParameterConfig + discard_when_merged?: bool + strategy?: "deep_merge" | "merge" | "keep" | "replace" | null + value?: + *: ToolParameterConfig + default?: unknown | null + description?: string | null + enum?: [unknown] | null + examples?: string | null + items?: @ToolParameterConfig | null + properties?: @MergeableMap_ToolParameterConfig + required?: bool | null + summary?: string | null + type?: string | [string] | null + questions: MergeableMap_QuestionConfig + |: + *: QuestionConfig + answer: unknown | null + target: QuestionTarget + |: "user" | "assistant" + |: PartialAssistantConfig + instructions?: + |: PartialMergeableVec_InstructionsConfig |: []: PartialInstructionsConfig description?: string | null @@ -601,114 +615,644 @@ conversation: ConversationConfig items?: [string] | null position?: int | null title?: string | null - |: null - |: null - model?: - |: PartialModelConfig - id?: - |: PartialModelIdOrAliasConfig - |: PartialModelIdConfig - name?: string | null - provider?: "anthropic" | "cerebras" | "deepseek" | "google" | "llamacpp" | "ollama" | "openai" | "openrouter" | "xai" | null - |: string + |: PartialMergedVec_InstructionsConfig + dedup?: "inherit" | "true" | "false" | bool | null | null + discard_when_merged?: bool | null + strategy?: "append" | "prepend" | "replace" | null + value?: + |: + []: PartialInstructionsConfig + description?: string | null + examples?: + |: + []: PartialExampleConfig + |: string + |: PartialContrastConfig + bad?: string | null + good?: string | null + reason?: string | null + |: null + items?: [string] | null + position?: int | null + title?: string | null + |: null |: null - parameters?: - |: PartialParametersConfig - max_tokens?: int | null - other?: - |: - *: unknown + model?: + |: PartialModelConfig + id?: + |: PartialModelIdOrAliasConfig + |: PartialModelIdConfig + name?: string | null + provider?: "anthropic" | "cerebras" | "deepseek" | "google" | "llamacpp" | "ollama" | "openai" | "openrouter" | "xai" | null + |: string |: null - reasoning?: - |: PartialReasoningConfig - |: "off" - |: "auto" - |: PartialCustomReasoningConfig - effort?: "none" | "auto" | "max" | "xhigh" | "high" | "medium" | "low" | "xlow" | string | null - exclude?: bool | null - |: null - service_tier?: "off" | "flex" | "standard" | "priority" | 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 + parameters?: + |: PartialParametersConfig + max_tokens?: int | null + other?: + |: + *: unknown + |: null + reasoning?: + |: PartialReasoningConfig + |: "off" + |: "auto" + |: PartialCustomReasoningConfig + effort?: "none" | "auto" | "max" | "xhigh" | "high" | "medium" | "low" | "xlow" | string | null + exclude?: bool | null + |: null + service_tier?: "off" | "flex" | "standard" | "priority" | 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 |: null - temperature?: float | null - top_k?: int | null - top_p?: float | null |: null - |: null - name?: string | null - request?: - |: PartialRequestConfig - base_backoff_ms?: int | null - cache?: bool | "off" | "short" | "long" | string | null - max_backoff_secs?: int | null - max_response_bytes?: int | "disabled" | "off" | bool | null - max_retries?: int | null - stream_idle_timeout_secs?: int | null - |: null - system_prompt?: - |: PartialMergeableString - |: string - |: PartialMergedString - dedup?: bool | "inherit" | "off" | "exact" | "block" | "contains" | null | null - discard_when_merged?: bool | null - separator?: "none" | "space" | "line" | "paragraph" | null - strategy?: "append" | "prepend" | "replace" | null - value?: string | null - |: null - system_prompt_sections?: - |: PartialMergeableVec - |: - []: PartialSectionConfig - content?: string | null - position?: int | null - tag?: string | null - title?: string | null - |: PartialMergedVec - dedup?: "inherit" | "true" | "false" | bool | null | null - discard_when_merged?: bool | null - strategy?: "append" | "prepend" | "replace" | null - value?: + name?: string | null + request?: + |: PartialRequestConfig + base_backoff_ms?: int | null + cache?: bool | "off" | "short" | "long" | string | null + max_backoff_secs?: int | null + max_response_bytes?: int | "disabled" | "off" | bool | null + max_retries?: int | null + stream_idle_timeout_secs?: int | null + |: null + system_prompt?: + |: PartialMergeableString + |: string + |: PartialMergedString + dedup?: bool | "inherit" | "off" | "exact" | "block" | "contains" | null | null + discard_when_merged?: bool | null + separator?: "none" | "space" | "line" | "paragraph" | null + strategy?: "append" | "prepend" | "replace" | null + value?: string | null + |: null + system_prompt_sections?: + |: PartialMergeableVec_SectionConfig |: []: PartialSectionConfig content?: string | null position?: int | null tag?: string | null title?: string | null + |: PartialMergedVec_SectionConfig + dedup?: "inherit" | "true" | "false" | bool | null | null + discard_when_merged?: bool | null + strategy?: "append" | "prepend" | "replace" | null + value?: + |: + []: PartialSectionConfig + content?: string | null + position?: int | null + tag?: string | null + title?: string | null + |: null + |: null + tool_choice?: "auto" | "none" | "required" | string | null + |: MergedMap_QuestionConfig + discard_when_merged?: bool + strategy?: "deep_merge" | "merge" | "keep" | "replace" | null + value?: + *: QuestionConfig + answer: unknown | null + target: QuestionTarget + |: "user" | "assistant" + |: PartialAssistantConfig + instructions?: + |: PartialMergeableVec_InstructionsConfig + |: + []: PartialInstructionsConfig + description?: string | null + examples?: + |: + []: PartialExampleConfig + |: string + |: PartialContrastConfig + bad?: string | null + good?: string | null + reason?: string | null + |: null + items?: [string] | null + position?: int | null + title?: string | null + |: PartialMergedVec_InstructionsConfig + dedup?: "inherit" | "true" | "false" | bool | null | null + discard_when_merged?: bool | null + strategy?: "append" | "prepend" | "replace" | null + value?: + |: + []: PartialInstructionsConfig + description?: string | null + examples?: + |: + []: PartialExampleConfig + |: string + |: PartialContrastConfig + bad?: string | null + good?: string | null + reason?: string | null + |: null + items?: [string] | null + position?: int | null + title?: string | null + |: null |: null - |: null - tool_choice?: "auto" | "none" | "required" | string | null - result: "unattended" | "ask" | "edit" | "skip" | null - run: "ask" | "unattended" | "edit" | "skip" | null - source: string(/^(builtin|local)(\..+)?$|^mcp\.[^.]+(\..+)?$/) - style: - |: DisplayStyleConfig - error: ErrorStyleConfig - inline_results: "off" | "full" | string | null - results_file_link: "full" | "osc8" | "off" | null - hidden?: bool - inline_results?: "off" | "full" | string - joins_reasoning?: bool - parameters?: ParametersStyle - |: "json" - |: "function_call" - |: "off" + model?: + |: PartialModelConfig + id?: + |: PartialModelIdOrAliasConfig + |: PartialModelIdConfig + name?: string | null + provider?: "anthropic" | "cerebras" | "deepseek" | "google" | "llamacpp" | "ollama" | "openai" | "openrouter" | "xai" | null + |: string + |: null + parameters?: + |: PartialParametersConfig + max_tokens?: int | null + other?: + |: + *: unknown + |: null + reasoning?: + |: PartialReasoningConfig + |: "off" + |: "auto" + |: PartialCustomReasoningConfig + effort?: "none" | "auto" | "max" | "xhigh" | "high" | "medium" | "low" | "xlow" | string | null + exclude?: bool | null + |: null + service_tier?: "off" | "flex" | "standard" | "priority" | 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 + |: null + |: null + name?: string | null + request?: + |: PartialRequestConfig + base_backoff_ms?: int | null + cache?: bool | "off" | "short" | "long" | string | null + max_backoff_secs?: int | null + max_response_bytes?: int | "disabled" | "off" | bool | null + max_retries?: int | null + stream_idle_timeout_secs?: int | null + |: null + system_prompt?: + |: PartialMergeableString + |: string + |: PartialMergedString + dedup?: bool | "inherit" | "off" | "exact" | "block" | "contains" | null | null + discard_when_merged?: bool | null + separator?: "none" | "space" | "line" | "paragraph" | null + strategy?: "append" | "prepend" | "replace" | null + value?: string | null + |: null + system_prompt_sections?: + |: PartialMergeableVec_SectionConfig + |: + []: PartialSectionConfig + content?: string | null + position?: int | null + tag?: string | null + title?: string | null + |: PartialMergedVec_SectionConfig + dedup?: "inherit" | "true" | "false" | bool | null | null + discard_when_merged?: bool | null + strategy?: "append" | "prepend" | "replace" | null + value?: + |: + []: PartialSectionConfig + content?: string | null + position?: int | null + tag?: string | null + title?: string | null + |: null + |: null + tool_choice?: "auto" | "none" | "required" | string | null + result: "unattended" | "ask" | "edit" | "skip" | null + run: "ask" | "unattended" | "edit" | "skip" | null + source: string(/^(builtin|local)(\..+)?$|^mcp\.[^.]+(\..+)?$/) + style: + |: DisplayStyleConfig + error: ErrorStyleConfig + inline_results: "off" | "full" | string | null + results_file_link: "full" | "osc8" | "off" | null + hidden?: bool + inline_results?: "off" | "full" | string + joins_reasoning?: bool + parameters?: ParametersStyle + |: "json" + |: "function_call" + |: "off" + |: CommandConfigOrString + |: string + | (expanded): CommandConfig + args?: [string] + program: string + shell?: bool + print_stderr?: bool + results_file_link?: "full" | "osc8" | "off" + |: null + summary: string | null + |: MergedMap_ToolConfig + discard_when_merged?: bool + strategy?: "deep_merge" | "merge" | "keep" | "replace" | null + value?: + *: ToolConfig + access: + |: AccessConfig + env: MergeableVec_EnvRuleConfig + |: + []: EnvRuleConfig + name: string + read: bool | null + |: MergedVec_EnvRuleConfig + dedup?: "inherit" | "true" | "false" | bool | null + discard_when_merged?: bool + strategy?: "append" | "prepend" | "replace" | null + value?: + []: EnvRuleConfig + name: string + read: bool | null + fs: MergeableVec_FsRuleConfig + |: + []: FsRuleConfig + create: bool | null + delete: bool | null + execute: bool | null + external: bool | null + path: string + read: bool | null + update: bool | null + write: bool | null + |: MergedVec_FsRuleConfig + dedup?: "inherit" | "true" | "false" | bool | null + discard_when_merged?: bool + strategy?: "append" | "prepend" | "replace" | null + value?: + []: FsRuleConfig + create: bool | null + delete: bool | null + execute: bool | null + external: bool | null + path: string + read: bool | null + update: bool | null + write: bool | null + |: null + cancellation_response: string | null + command: |: CommandConfigOrString |: string | (expanded): CommandConfig args?: [string] program: string shell?: bool - print_stderr?: bool - results_file_link?: "full" | "osc8" | "off" - |: null - summary: string | null + |: null + description: string | null + enable: + |: + |: bool + |: "on" | "off" | "always" | "explicit" + | (expanded): EnableConfig + allow_toggle: "any" | "never" | "if_named" | "if_named_or_group" | null + state: bool | null + |: null + examples: string | null + format: "ask" | "unattended" | null + options: MergeableMap_JsonValue + |: + *: unknown + |: MergedMap_JsonValue + discard_when_merged?: bool + strategy?: "deep_merge" | "merge" | "keep" | "replace" | null + value?: + *: unknown + parameters: MergeableMap_ToolParameterConfig + |: + *: ToolParameterConfig + default?: unknown | null + description?: string | null + enum?: [unknown] | null + examples?: string | null + items?: @ToolParameterConfig | null + properties?: @MergeableMap_ToolParameterConfig + required?: bool | null + summary?: string | null + type?: string | [string] | null + |: MergedMap_ToolParameterConfig + discard_when_merged?: bool + strategy?: "deep_merge" | "merge" | "keep" | "replace" | null + value?: + *: ToolParameterConfig + default?: unknown | null + description?: string | null + enum?: [unknown] | null + examples?: string | null + items?: @ToolParameterConfig | null + properties?: @MergeableMap_ToolParameterConfig + required?: bool | null + summary?: string | null + type?: string | [string] | null + questions: MergeableMap_QuestionConfig + |: + *: QuestionConfig + answer: unknown | null + target: QuestionTarget + |: "user" | "assistant" + |: PartialAssistantConfig + instructions?: + |: PartialMergeableVec_InstructionsConfig + |: + []: PartialInstructionsConfig + description?: string | null + examples?: + |: + []: PartialExampleConfig + |: string + |: PartialContrastConfig + bad?: string | null + good?: string | null + reason?: string | null + |: null + items?: [string] | null + position?: int | null + title?: string | null + |: PartialMergedVec_InstructionsConfig + dedup?: "inherit" | "true" | "false" | bool | null | null + discard_when_merged?: bool | null + strategy?: "append" | "prepend" | "replace" | null + value?: + |: + []: PartialInstructionsConfig + description?: string | null + examples?: + |: + []: PartialExampleConfig + |: string + |: PartialContrastConfig + bad?: string | null + good?: string | null + reason?: string | null + |: null + items?: [string] | null + position?: int | null + title?: string | null + |: null + |: null + model?: + |: PartialModelConfig + id?: + |: PartialModelIdOrAliasConfig + |: PartialModelIdConfig + name?: string | null + provider?: "anthropic" | "cerebras" | "deepseek" | "google" | "llamacpp" | "ollama" | "openai" | "openrouter" | "xai" | null + |: string + |: null + parameters?: + |: PartialParametersConfig + max_tokens?: int | null + other?: + |: + *: unknown + |: null + reasoning?: + |: PartialReasoningConfig + |: "off" + |: "auto" + |: PartialCustomReasoningConfig + effort?: "none" | "auto" | "max" | "xhigh" | "high" | "medium" | "low" | "xlow" | string | null + exclude?: bool | null + |: null + service_tier?: "off" | "flex" | "standard" | "priority" | 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 + |: null + |: null + name?: string | null + request?: + |: PartialRequestConfig + base_backoff_ms?: int | null + cache?: bool | "off" | "short" | "long" | string | null + max_backoff_secs?: int | null + max_response_bytes?: int | "disabled" | "off" | bool | null + max_retries?: int | null + stream_idle_timeout_secs?: int | null + |: null + system_prompt?: + |: PartialMergeableString + |: string + |: PartialMergedString + dedup?: bool | "inherit" | "off" | "exact" | "block" | "contains" | null | null + discard_when_merged?: bool | null + separator?: "none" | "space" | "line" | "paragraph" | null + strategy?: "append" | "prepend" | "replace" | null + value?: string | null + |: null + system_prompt_sections?: + |: PartialMergeableVec_SectionConfig + |: + []: PartialSectionConfig + content?: string | null + position?: int | null + tag?: string | null + title?: string | null + |: PartialMergedVec_SectionConfig + dedup?: "inherit" | "true" | "false" | bool | null | null + discard_when_merged?: bool | null + strategy?: "append" | "prepend" | "replace" | null + value?: + |: + []: PartialSectionConfig + content?: string | null + position?: int | null + tag?: string | null + title?: string | null + |: null + |: null + tool_choice?: "auto" | "none" | "required" | string | null + |: MergedMap_QuestionConfig + discard_when_merged?: bool + strategy?: "deep_merge" | "merge" | "keep" | "replace" | null + value?: + *: QuestionConfig + answer: unknown | null + target: QuestionTarget + |: "user" | "assistant" + |: PartialAssistantConfig + instructions?: + |: PartialMergeableVec_InstructionsConfig + |: + []: PartialInstructionsConfig + description?: string | null + examples?: + |: + []: PartialExampleConfig + |: string + |: PartialContrastConfig + bad?: string | null + good?: string | null + reason?: string | null + |: null + items?: [string] | null + position?: int | null + title?: string | null + |: PartialMergedVec_InstructionsConfig + dedup?: "inherit" | "true" | "false" | bool | null | null + discard_when_merged?: bool | null + strategy?: "append" | "prepend" | "replace" | null + value?: + |: + []: PartialInstructionsConfig + description?: string | null + examples?: + |: + []: PartialExampleConfig + |: string + |: PartialContrastConfig + bad?: string | null + good?: string | null + reason?: string | null + |: null + items?: [string] | null + position?: int | null + title?: string | null + |: null + |: null + model?: + |: PartialModelConfig + id?: + |: PartialModelIdOrAliasConfig + |: PartialModelIdConfig + name?: string | null + provider?: "anthropic" | "cerebras" | "deepseek" | "google" | "llamacpp" | "ollama" | "openai" | "openrouter" | "xai" | null + |: string + |: null + parameters?: + |: PartialParametersConfig + max_tokens?: int | null + other?: + |: + *: unknown + |: null + reasoning?: + |: PartialReasoningConfig + |: "off" + |: "auto" + |: PartialCustomReasoningConfig + effort?: "none" | "auto" | "max" | "xhigh" | "high" | "medium" | "low" | "xlow" | string | null + exclude?: bool | null + |: null + service_tier?: "off" | "flex" | "standard" | "priority" | 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 + |: null + |: null + name?: string | null + request?: + |: PartialRequestConfig + base_backoff_ms?: int | null + cache?: bool | "off" | "short" | "long" | string | null + max_backoff_secs?: int | null + max_response_bytes?: int | "disabled" | "off" | bool | null + max_retries?: int | null + stream_idle_timeout_secs?: int | null + |: null + system_prompt?: + |: PartialMergeableString + |: string + |: PartialMergedString + dedup?: bool | "inherit" | "off" | "exact" | "block" | "contains" | null | null + discard_when_merged?: bool | null + separator?: "none" | "space" | "line" | "paragraph" | null + strategy?: "append" | "prepend" | "replace" | null + value?: string | null + |: null + system_prompt_sections?: + |: PartialMergeableVec_SectionConfig + |: + []: PartialSectionConfig + content?: string | null + position?: int | null + tag?: string | null + title?: string | null + |: PartialMergedVec_SectionConfig + dedup?: "inherit" | "true" | "false" | bool | null | null + discard_when_merged?: bool | null + strategy?: "append" | "prepend" | "replace" | null + value?: + |: + []: PartialSectionConfig + content?: string | null + position?: int | null + tag?: string | null + title?: string | null + |: null + |: null + tool_choice?: "auto" | "none" | "required" | string | null + result: "unattended" | "ask" | "edit" | "skip" | null + run: "ask" | "unattended" | "edit" | "skip" | null + source: string(/^(builtin|local)(\..+)?$|^mcp\.[^.]+(\..+)?$/) + style: + |: DisplayStyleConfig + error: ErrorStyleConfig + inline_results: "off" | "full" | string | null + results_file_link: "full" | "osc8" | "off" | null + hidden?: bool + inline_results?: "off" | "full" | string + joins_reasoning?: bool + parameters?: ParametersStyle + |: "json" + |: "function_call" + |: "off" + |: CommandConfigOrString + |: string + | (expanded): CommandConfig + args?: [string] + program: string + shell?: bool + print_stderr?: bool + results_file_link?: "full" | "osc8" | "off" + |: null + summary: string | null editor: EditorConfig cmd: |: CommandConfigOrString @@ -746,25 +1290,49 @@ loader: LoaderConfig reset: "none" | null plugins: PluginsConfig auto_install?: bool - command: - *: CommandPluginConfig - checksum: - |: ChecksumConfig - algorithm?: "sha256" | "sha1" - value: string - |: null - install: bool | null - options: unknown | null - run: "ask" | "unattended" | "deny" | null + command: MergeableMap_CommandPluginConfig + |: + *: CommandPluginConfig + checksum: + |: ChecksumConfig + algorithm?: "sha256" | "sha1" + value: string + |: null + install: bool | null + options: unknown | null + run: "ask" | "unattended" | "deny" | null + |: MergedMap_CommandPluginConfig + discard_when_merged?: bool + strategy?: "deep_merge" | "merge" | "keep" | "replace" | null + value?: + *: CommandPluginConfig + checksum: + |: ChecksumConfig + algorithm?: "sha256" | "sha1" + value: string + |: null + install: bool | null + options: unknown | null + run: "ask" | "unattended" | "deny" | null shutdown_timeout_secs?: int providers: ProviderConfig llm: LlmProviderConfig - aliases: - *: ModelIdOrAliasConfig - |: ModelIdConfig - name: string - provider: "anthropic" | "cerebras" | "deepseek" | "google" | "llamacpp" | "ollama" | "openai" | "openrouter" | "xai" - |: string + aliases: MergeableMap_ModelIdOrAliasConfig + |: + *: ModelIdOrAliasConfig + |: ModelIdConfig + name: string + provider: "anthropic" | "cerebras" | "deepseek" | "google" | "llamacpp" | "ollama" | "openai" | "openrouter" | "xai" + |: string + |: MergedMap_ModelIdOrAliasConfig + discard_when_merged?: bool + strategy?: "deep_merge" | "merge" | "keep" | "replace" | null + value?: + *: ModelIdOrAliasConfig + |: ModelIdConfig + name: string + provider: "anthropic" | "cerebras" | "deepseek" | "google" | "llamacpp" | "ollama" | "openai" | "openrouter" | "xai" + |: string anthropic: AnthropicConfig api_key_env?: string auth?: [string] @@ -799,32 +1367,62 @@ providers: ProviderConfig app_name?: string app_referrer: string | null base_url?: string - mcp: - *: McpProviderConfig - |: StdioConfig - 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" - value: string - |: null - command: string - optional?: bool - startup_timeout_secs?: int - type: "stdio" - variables?: MergeableVec - |: [string] - |: MergedVec - dedup?: "inherit" | "true" | "false" | bool | null - discard_when_merged?: bool - strategy?: "append" | "prepend" | "replace" | null - value?: [string] + mcp: MergeableMap_McpProviderConfig + |: + *: McpProviderConfig + |: StdioConfig + 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" + value: string + |: null + command: string + optional?: bool + startup_timeout_secs?: int + type: "stdio" + variables?: MergeableVec + |: [string] + |: MergedVec + dedup?: "inherit" | "true" | "false" | bool | null + discard_when_merged?: bool + strategy?: "append" | "prepend" | "replace" | null + value?: [string] + |: MergedMap_McpProviderConfig + discard_when_merged?: bool + strategy?: "deep_merge" | "merge" | "keep" | "replace" | null + value?: + *: McpProviderConfig + |: StdioConfig + 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" + value: string + |: null + command: string + optional?: bool + startup_timeout_secs?: int + type: "stdio" + 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 @@ -898,7 +1496,13 @@ style: StyleConfig max_latency?: string text_delay?: string template: TemplateConfig - values: - *: unknown + values: MergeableMap_JsonValue + |: + *: unknown + |: MergedMap_JsonValue + discard_when_merged?: bool + strategy?: "deep_merge" | "merge" | "keep" | "replace" | null + value?: + *: unknown user: UserConfig name: string | null diff --git a/crates/jp_config/src/types/map.rs b/crates/jp_config/src/types/map.rs index d490bb6c8..6d381eafa 100644 --- a/crates/jp_config/src/types/map.rs +++ b/crates/jp_config/src/types/map.rs @@ -64,22 +64,25 @@ where where D: Deserializer<'de>, { - // Try as `MergedMap` first (has `value` + `strategy` keys), then - // fall back to a plain map. + // Both variants are maps, so the keys decide which one this is: a table + // carrying `value` and `strategy` is the wrapper, anything else is a + // plain map. An entry named `value` alone is still an entry. UntaggedEnumVisitor::new() .map(|map| { let value: serde_json::Value = map.deserialize()?; - // Peek: does this look like a MergedMap? if let Some(obj) = value.as_object() && obj.contains_key("value") && obj.contains_key("strategy") - && let Ok(merged) = serde_json::from_value::>(value.clone()) { - return Ok(Self::Merged(merged)); + // Committed: a malformed wrapper is an error rather than a + // map that happens to use these two names, so a misspelled + // strategy is reported instead of becoming an entry. + return serde_json::from_value::>(value) + .map(Self::Merged) + .map_err(serde::de::Error::custom); } - // Plain map. serde_json::from_value(value) .map(Self::Map) .map_err(serde::de::Error::custom) From e1c06ef075d9df46e99d27369b411d2ab5527ed3 Mon Sep 17 00:00:00 2001 From: Jean Mertz Date: Fri, 18 Sep 2026 22:50:34 +0200 Subject: [PATCH 13/16] fix(conversation): Keep the tools a conversation still has on reload Removing one of a conversation's tools stored the map the user was left with, stamped `replace`. Reading that conversation back emptied the map, so the replacement took every tool rather than the one removed, and the conversation loaded with no tools at all. Schema-aware stripping walks a flattened map's leftover keys against the map's entry type, since those keys are entry names. A map stating a strategy puts `value` and `strategy` where entry names otherwise sit, so `value` was walked as a tool and every key inside it was removed as unknown. Told apart the way the map's own deserializer does it: a table carrying both keys is the wrapper, and its `value` holds the map whose entries are walked. The metadata beside it is left alone. Signed-off-by: Jean Mertz --- crates/jp_conversation/src/compat.rs | 31 +++++++++-- crates/jp_conversation/src/compat_tests.rs | 61 ++++++++++++++++++++++ 2 files changed, 89 insertions(+), 3 deletions(-) diff --git a/crates/jp_conversation/src/compat.rs b/crates/jp_conversation/src/compat.rs index 42dd82c07..c74a47cc4 100644 --- a/crates/jp_conversation/src/compat.rs +++ b/crates/jp_conversation/src/compat.rs @@ -362,13 +362,22 @@ fn strategy_carrying_variant<'a>(union_type: &'a UnionType, value: &Value) -> Op let collection = sole(variants().filter(|variant| is_map(variant)))?; let wrapper = sole(variants().filter(|variant| !is_map(variant)))?; - let stated = value - .as_object() - .is_some_and(|obj| obj.contains_key("value") && obj.contains_key("strategy")); + let stated = value.as_object().is_some_and(states_a_strategy); Some(if stated { wrapper } else { collection }) } +/// Whether an object is a collection that states its own merge strategy, rather +/// than the collection itself. +/// +/// Both reach disk as a table, so the keys decide: the wrapper carries `value` +/// beside `strategy`, which is how its own deserializer tells them apart. +/// An entry named `value` needs the sibling `strategy` before it reads as the +/// wrapper, so a tool called `value` stays addressable. +fn states_a_strategy(obj: &serde_json::Map) -> bool { + obj.contains_key("value") && obj.contains_key("strategy") +} + /// The only item an iterator yields, if it yields exactly one. fn sole<'a>(mut variants: impl Iterator) -> Option<&'a Schema> { match (variants.next(), variants.next()) { @@ -445,6 +454,22 @@ fn strip_struct<'a>( before - obj.len() }; + // A flattened map stating a strategy puts the wrapper's own keys where + // entries otherwise sit: `value` holds the map, and the metadata beside it + // says how it merges rather than naming an entry. Walking those keys as + // entries would take the map for a single one and delete every key in it. + if has_flatten && states_a_strategy(obj) { + if let Some(entry_schema) = entry_schema + && let Some(Value::Object(entries)) = obj.get_mut("value") + { + for entry in entries.values_mut() { + stripped += strip_schema(entry, entry_schema, enclosing); + } + } + + return stripped; + } + for (key, child) in obj.iter_mut() { // The flattened field's own name is not a key in the serialized form, // so a key matching it is an entry of the map it flattens, not that diff --git a/crates/jp_conversation/src/compat_tests.rs b/crates/jp_conversation/src/compat_tests.rs index a952fb559..0eb191614 100644 --- a/crates/jp_conversation/src/compat_tests.rs +++ b/crates/jp_conversation/src/compat_tests.rs @@ -244,6 +244,41 @@ fn strip_descends_into_a_tool_named_after_the_flattened_field() { ); } +#[test] +fn strip_descends_into_a_flattened_map_stating_a_strategy() { + // Removing one of a conversation's tools records the map the user is left + // with, stamped `replace`. Flattened, that wrapper's own keys sit where + // tool names otherwise do, so `value` holds the map rather than a tool. + let schema = AppConfig::schema(); + let mut value = json!({ + "conversation": { + "tools": { + "value": { + "bash": { "source": "local", "from_a_newer_jp": 1 } + }, + "strategy": "replace" + } + } + }); + + let stripped = strip_unknown_fields(&mut value, &schema); + assert_eq!(stripped, 1); + assert_eq!( + value, + json!({ + "conversation": { + "tools": { + "value": { + "bash": { "source": "local" } + }, + "strategy": "replace" + } + } + }), + "the retained tool survives and the strategy is left alone" + ); +} + #[test] fn strip_descends_into_array_items() { let schema = AppConfig::schema(); @@ -862,6 +897,32 @@ fn legacy_enable_strings_survive_compat_deserialization() { assert_eq!(tools["off_tool"].enable, Some(PartialEnableConfig::OFF)); } +#[test] +fn a_removed_tool_leaves_the_others_standing() { + // Removing one of two tools stores the map the user is left with, stamped + // `replace`. Reading that back has to keep the tool they kept: an empty + // replacement would take every tool the conversation had. + let value = json!({ + "conversation": { + "tools": { + "value": { + "bash": { "source": "local", "from_a_newer_jp": 1 } + }, + "strategy": "replace" + } + } + }); + + let config = deserialize_partial_config(value); + let tools = &config.conversation.tools.tools; + + assert!( + tools.contains_key("bash"), + "the retained tool survives the read: {tools:?}" + ); + assert_eq!(tools.len(), 1, "and nothing else joins it"); +} + #[test] fn legacy_rule_bounds_survive_compat_deserialization() { use jp_config::conversation::compaction::RuleBound; From 9f8af3e4d5a2612b22844705a122d851e50156b1 Mon Sep 17 00:00:00 2001 From: Jean Mertz Date: Fri, 18 Sep 2026 22:51:28 +0200 Subject: [PATCH 14/16] fix(config): Report the map entries and fields a delta cannot merge A server, tool, alias or plugin removed from a conversation came back on the next invocation. The conversation layer is filled from the config files, and filling reads a key the conversation does not hold as one it never mentioned rather than one it removed, so the workspace put it back. A dropped key now reports its dotted path alongside the `replace` the delta already carried. Both readers need it: the value is what the conversation's own fold applies, and the path is what stops the layer above from restoring the key after filling. An entry both maps hold is diffed with its own path too, so a field cleared inside a surviving entry says where it lives. Clearing an MCP server's `checksum`, or a parameter's `enum`, was computed and dropped; the tool config's path-reporting deltas were unreachable through the map entirely. Signed-off-by: Jean Mertz --- crates/jp_cli/src/config_pipeline_tests.rs | 49 +++++++++ crates/jp_config/src/conversation.rs | 10 +- crates/jp_config/src/conversation/tool.rs | 91 ++++++++++++++-- crates/jp_config/src/delta.rs | 116 ++++++++++++++++++--- crates/jp_config/src/delta_tests.rs | 37 +++++++ crates/jp_config/src/lib_tests.rs | 16 ++- crates/jp_config/src/providers.rs | 6 +- crates/jp_config/src/providers/llm.rs | 11 +- crates/jp_config/src/providers/mcp.rs | 29 +++++- crates/jp_config/src/template.rs | 13 ++- 10 files changed, 335 insertions(+), 43 deletions(-) diff --git a/crates/jp_cli/src/config_pipeline_tests.rs b/crates/jp_cli/src/config_pipeline_tests.rs index 2adf590e0..81351202e 100644 --- a/crates/jp_cli/src/config_pipeline_tests.rs +++ b/crates/jp_cli/src/config_pipeline_tests.rs @@ -264,6 +264,55 @@ fn conversation_clears_prevent_base_values_from_returning() { assert!(!partial.providers.mcp.contains_key("kagi")); } +/// A server the conversation removed stays removed on the next invocation, with +/// the paths taken from the delta rather than named by hand. +/// +/// The two halves have to meet: the delta reports the key it dropped, and the +/// pipeline clears it after filling. +/// Either alone puts the server back, since filling reads a key the +/// conversation does not hold as one it never mentioned. +#[test] +fn a_removed_server_survives_the_next_invocation() { + use jp_config::PartialConfigDelta as _; + + let mut pipeline = empty_pipeline(); + pipeline + .base + .providers + .mcp + .insert("bookworm".to_owned(), mcp_server("serve")); + pipeline + .base + .providers + .mcp + .insert("kagi".to_owned(), mcp_server("search")); + + // The invocation that removed `kagi`, as the producer sees it: the + // conversation's state before, against the config the turn ran with. + let before = pipeline.base.clone(); + let mut after = pipeline.base.clone(); + after.providers.mcp.shift_remove("kagi"); + + let mut unsets = Vec::new(); + let delta = before.delta_with_unsets(after.clone(), "", &mut unsets); + + assert!( + !delta.providers.mcp.contains_key("kagi"), + "the delta carries the map the user is left with" + ); + + let partial = pipeline.partial_with_conversation(after, &unsets).unwrap(); + + assert!( + !partial.providers.mcp.contains_key("kagi"), + "the workspace must not restore a server the conversation removed" + ); + assert!( + partial.providers.mcp.contains_key("bookworm"), + "and the servers it kept are still there" + ); +} + #[test] fn conversation_clears_allow_explicit_cfg_values() { let mut pipeline = empty_pipeline(); diff --git a/crates/jp_config/src/conversation.rs b/crates/jp_config/src/conversation.rs index 786abcd7b..23d825142 100644 --- a/crates/jp_config/src/conversation.rs +++ b/crates/jp_config/src/conversation.rs @@ -19,7 +19,8 @@ use crate::{ tool::{PartialToolsConfig, ToolsConfig}, }, delta::{ - PartialConfigDelta, delta_mergeable_map, delta_mergeable_vec, delta_opt, delta_opt_at, path, + PartialConfigDelta, delta_mergeable_map, delta_mergeable_map_at, delta_mergeable_vec, + delta_opt, delta_opt_at, path, }, fill::FillDefaults, internal::merge::{map_with_strategy, vec_with_strategy}, @@ -168,7 +169,12 @@ impl PartialConfigDelta for PartialConversationConfig { next.default_id, unsets, ), - labels: delta_mergeable_map(&self.labels, next.labels), + labels: delta_mergeable_map_at( + &path(prefix, "labels"), + &self.labels, + next.labels, + unsets, + ), } } } diff --git a/crates/jp_config/src/conversation/tool.rs b/crates/jp_config/src/conversation/tool.rs index 2ae880b31..c62edcb61 100644 --- a/crates/jp_config/src/conversation/tool.rs +++ b/crates/jp_config/src/conversation/tool.rs @@ -18,8 +18,9 @@ use crate::{ style::{DisplayStyleConfig, PartialDisplayStyleConfig}, }, delta::{ - PartialConfigDelta, delta_mergeable_map, delta_mergeable_value_map, delta_opt, - delta_opt_at, delta_opt_partial, delta_opt_partial_at, delta_vec, path, + PartialConfigDelta, delta_mergeable_map, delta_mergeable_map_at, delta_mergeable_value_map, + delta_mergeable_value_map_at, delta_opt, delta_opt_at, delta_opt_partial, + delta_opt_partial_at, delta_vec, path, }, fill::{FillDefaults, fill_map}, internal::merge::map_with_strategy, @@ -78,9 +79,7 @@ impl PartialConfigDelta for PartialToolsConfig { defaults: self .defaults .delta_with_unsets(next.defaults, &path(prefix, "*"), unsets), - // The map states its own strategy, so a removed tool travels in - // the value as a `replace` and needs no path reported. - tools: delta_mergeable_map(&self.tools, next.tools), + tools: delta_mergeable_map_at(prefix, &self.tools, next.tools, unsets), } } } @@ -689,9 +688,12 @@ impl PartialConfigDelta for PartialToolConfig { summary: delta_opt(self.summary.as_ref(), next.summary), description: delta_opt(self.description.as_ref(), next.description), examples: delta_opt(self.examples.as_ref(), next.examples), - // Each map states its own strategy, so a removed entry travels in - // the value as a `replace` and needs no path reported. - parameters: delta_mergeable_map(&self.parameters, next.parameters), + parameters: delta_mergeable_map_at( + &path(prefix, "parameters"), + &self.parameters, + next.parameters, + unsets, + ), run: delta_opt(self.run.as_ref(), next.run), format: delta_opt(self.format.as_ref(), next.format), result: delta_opt(self.result.as_ref(), next.result), @@ -705,8 +707,18 @@ impl PartialConfigDelta for PartialToolConfig { next.style, unsets, ), - questions: delta_mergeable_map(&self.questions, next.questions), - options: delta_mergeable_value_map(&self.options, next.options), + questions: delta_mergeable_map_at( + &path(prefix, "questions"), + &self.questions, + next.questions, + unsets, + ), + options: delta_mergeable_value_map_at( + &path(prefix, "options"), + &self.options, + next.options, + unsets, + ), access: delta_opt_partial_at( &path(prefix, "access"), self.access.as_ref(), @@ -856,6 +868,65 @@ impl PartialConfigDelta for PartialToolParameterConfig { properties: delta_mergeable_map(&self.properties, next.properties), } } + + fn delta_with_unsets(&self, next: Self, prefix: &str, unsets: &mut Vec) -> Self { + Self { + kind: delta_opt_partial_at( + &path(prefix, "type"), + self.kind.as_ref(), + next.kind, + unsets, + ), + default: delta_opt_at( + &path(prefix, "default"), + self.default.as_ref(), + next.default, + unsets, + ), + required: delta_opt_at( + &path(prefix, "required"), + self.required.as_ref(), + next.required, + unsets, + ), + summary: delta_opt_at( + &path(prefix, "summary"), + self.summary.as_ref(), + next.summary, + unsets, + ), + description: delta_opt_at( + &path(prefix, "description"), + self.description.as_ref(), + next.description, + unsets, + ), + examples: delta_opt_at( + &path(prefix, "examples"), + self.examples.as_ref(), + next.examples, + unsets, + ), + enumeration: delta_opt_at( + &path(prefix, "enum"), + self.enumeration.as_ref(), + next.enumeration, + unsets, + ), + items: delta_opt_at( + &path(prefix, "items"), + self.items.as_ref(), + next.items, + unsets, + ), + properties: delta_mergeable_map_at( + &path(prefix, "properties"), + &self.properties, + next.properties, + unsets, + ), + } + } } impl ToPartial for ToolParameterConfig { diff --git a/crates/jp_config/src/delta.rs b/crates/jp_config/src/delta.rs index 36b8336fc..d8d653a14 100644 --- a/crates/jp_config/src/delta.rs +++ b/crates/jp_config/src/delta.rs @@ -123,18 +123,15 @@ fn repeats_an_element(items: &[T]) -> bool { /// spell. /// The whole map is then carried with `replace`, since a deep merge would /// resurrect the dropped key. +/// +/// See [`delta_mergeable_map_at`] for the variant that also reports the paths +/// it drops and clears, which a caller filling one layer from another needs. pub fn delta_mergeable_map(prev: &MergeableMap, next: MergeableMap) -> MergeableMap where T: PartialConfigDelta + PartialEq, { if prev.keys().any(|key| !next.contains_key(key)) { - // Stated rather than inherited from `next`'s shape: a plain map - // deep-merges on the fold and brings the dropped key back. - return MergeableMap::Merged(MergedMap { - value: next.into_map(), - strategy: Some(MergedMapStrategy::Replace), - discard_when_merged: false, - }); + return replace_with(next); } next.into_iter() @@ -195,6 +192,103 @@ pub fn delta_opt_partial_at( } } +/// Calculate the delta between two strategy-carrying maps, reporting what +/// merging cannot reach. +/// +/// A dropped key is said twice, because two readers need it. +/// The value says it by carrying `replace`, which is what the conversation's +/// own fold applies. +/// The path says it to a caller that fills this layer from another, where a key +/// this layer does not hold is indistinguishable from one it never mentioned — +/// and filling would put the dropped key back. +/// +/// An entry both maps hold is diffed with its own dotted path, so a field +/// cleared inside a surviving entry reports where it lives. +pub fn delta_mergeable_map_at( + prefix: &str, + prev: &MergeableMap, + next: MergeableMap, + unsets: &mut Vec, +) -> MergeableMap +where + T: PartialConfigDelta + PartialEq, +{ + if report_dropped_keys(prefix, prev, &next, unsets) { + return replace_with(next); + } + + next.into_iter() + .filter_map(|(key, next)| { + let Some(prev) = prev.get(&key) else { + return Some((key, next)); + }; + + if prev == &next { + return None; + } + + let mut entry = Vec::new(); + let delta = prev.delta_with_unsets(next, &path(prefix, &key), &mut entry); + let cleared = !entry.is_empty(); + unsets.append(&mut entry); + + (cleared || !delta.is_empty()).then_some((key, delta)) + }) + .collect() +} + +/// Report every key `prev` holds and `next` does not, returning whether there +/// were any. +fn report_dropped_keys( + prefix: &str, + prev: &MergeableMap, + next: &MergeableMap, + unsets: &mut Vec, +) -> bool { + let dropped: Vec = prev + .keys() + .filter(|key| !next.contains_key(*key)) + .map(|key| path(prefix, key)) + .collect(); + + let any = !dropped.is_empty(); + unsets.extend(dropped); + any +} + +/// The whole map, stated as a replacement. +/// +/// Stated rather than inherited from the map's shape: a plain map deep-merges +/// on the fold and brings a dropped key back. +fn replace_with(next: MergeableMap) -> MergeableMap { + MergeableMap::Merged(MergedMap { + value: next.into_map(), + strategy: Some(MergedMapStrategy::Replace), + discard_when_merged: false, + }) +} + +/// Calculate the delta between two strategy-carrying maps of plain values, +/// reporting the keys it drops. +/// +/// Mirrors [`delta_mergeable_map_at`] for a map whose values carry no partial +/// of their own, so an entry is compared and carried whole rather than diffed, +/// and only a dropped key has a path to report. +pub fn delta_mergeable_value_map_at( + prefix: &str, + prev: &MergeableMap, + next: MergeableMap, + unsets: &mut Vec, +) -> MergeableMap { + if report_dropped_keys(prefix, prev, &next, unsets) { + return replace_with(next); + } + + next.into_iter() + .filter(|(key, next)| !prev.get(key).is_some_and(|prev| prev == next)) + .collect() +} + /// Calculate the delta between two strategy-carrying maps of plain values. /// /// Mirrors [`delta_mergeable_map`] for a map whose values carry no partial of @@ -204,13 +298,7 @@ pub fn delta_mergeable_value_map( next: MergeableMap, ) -> MergeableMap { if prev.keys().any(|key| !next.contains_key(key)) { - // Stated rather than inherited from `next`'s shape: a plain map - // deep-merges on the fold and brings the dropped key back. - return MergeableMap::Merged(MergedMap { - value: next.into_map(), - strategy: Some(MergedMapStrategy::Replace), - discard_when_merged: false, - }); + return replace_with(next); } next.into_iter() diff --git a/crates/jp_config/src/delta_tests.rs b/crates/jp_config/src/delta_tests.rs index a12f50080..9741b3045 100644 --- a/crates/jp_config/src/delta_tests.rs +++ b/crates/jp_config/src/delta_tests.rs @@ -32,6 +32,43 @@ fn map(arguments: &[&str]) -> MergeableMap { map.into() } +/// A field cleared inside an entry both maps hold reports where it lives. +/// +/// No key disappears, so the map merges per key and the entry carries its own +/// delta. +/// That delta cannot say a field went away, which is what the path is for. +#[test] +fn map_delta_reports_a_field_cleared_inside_a_surviving_entry() { + use crate::providers::mcp::{PartialChecksumConfig, PartialStdioConfig}; + + let with_checksum = |checksum: Option| { + let mut map = IndexMap::new(); + map.insert( + "kagi".to_owned(), + PartialMcpProviderConfig::Stdio(PartialStdioConfig { + command: Some("serve".into()), + checksum, + ..PartialStdioConfig::default() + }), + ); + MergeableMap::from(map) + }; + + let prev = with_checksum(Some(PartialChecksumConfig { + value: Some("abc".to_owned()), + ..PartialChecksumConfig::default() + })); + + let mut unsets = Vec::new(); + let delta = delta_mergeable_map_at("providers.mcp", &prev, with_checksum(None), &mut unsets); + + assert_eq!(unsets, ["providers.mcp.kagi.checksum"]); + assert!( + !delta.is_empty(), + "the entry is carried so the clear has somewhere to land: {delta:?}" + ); +} + /// A removed entry is carried as a `replace`, since a deep merge would bring /// the key back. #[test] diff --git a/crates/jp_config/src/lib_tests.rs b/crates/jp_config/src/lib_tests.rs index f881ba760..9e7038ee2 100644 --- a/crates/jp_config/src/lib_tests.rs +++ b/crates/jp_config/src/lib_tests.rs @@ -772,13 +772,18 @@ fn a_dropped_mcp_argument_is_recorded() { ); } -/// A server the user removed is recorded, so the conversation stops starting -/// it. +/// A server the user removed is recorded twice over, so the conversation stops +/// starting it. /// /// Entries merge by key, which is what lets a server the workspace config /// gained reach a conversation created before it existed. /// That same property means a deep merge would resurrect a removed one, so the /// delta states `replace` and carries the map the user is left with. +/// +/// The value settles the conversation's own fold. +/// The path settles the layer above it, where the conversation is filled from +/// the config files and a server it does not hold would otherwise read as one +/// it never mentioned. #[test] fn a_removed_mcp_server_is_recorded() { use crate::providers::mcp::{McpProviderConfig, StdioConfig}; @@ -804,9 +809,10 @@ fn a_removed_mcp_server_is_recorded() { .to_partial() .delta_with_unsets(next.to_partial(), "", &mut unsets); - assert!( - unsets.is_empty(), - "the map states its own strategy, so no path is reported: {unsets:?}" + assert_eq!( + unsets, + ["providers.mcp.bookworm"], + "the removed key reports its path, so filling cannot restore it" ); assert!( delta.providers.mcp.discard_when_merged() || !delta.providers.mcp.is_empty(), diff --git a/crates/jp_config/src/providers.rs b/crates/jp_config/src/providers.rs index 416ca3563..6b040f12f 100644 --- a/crates/jp_config/src/providers.rs +++ b/crates/jp_config/src/providers.rs @@ -7,7 +7,7 @@ use schematic::{Config, ConfigError}; use crate::{ assignment::{AssignKeyValue, AssignResult, KvAssignment, missing_key}, - delta::{PartialConfigDelta, delta_mergeable_map, path}, + delta::{PartialConfigDelta, delta_mergeable_map, delta_mergeable_map_at, path}, fill::{FillDefaults, fill_map}, internal::merge::map_with_strategy, partial::ToPartial, @@ -86,9 +86,7 @@ impl PartialConfigDelta for PartialProviderConfig { llm: self .llm .delta_with_unsets(next.llm, &path(prefix, "llm"), unsets), - // The map states its own strategy, so a removed server travels in - // the value as a `replace` and needs no path reported. - mcp: delta_mergeable_map(&self.mcp, next.mcp), + mcp: delta_mergeable_map_at(&path(prefix, "mcp"), &self.mcp, next.mcp, unsets), } } } diff --git a/crates/jp_config/src/providers/llm.rs b/crates/jp_config/src/providers/llm.rs index 68deb64ea..45e65d785 100644 --- a/crates/jp_config/src/providers/llm.rs +++ b/crates/jp_config/src/providers/llm.rs @@ -14,7 +14,7 @@ use schematic::{Config, ConfigError}; use crate::{ assignment::{AssignKeyValue, AssignResult, KvAssignment, missing_key}, - delta::{PartialConfigDelta, delta_mergeable_map, path}, + delta::{PartialConfigDelta, delta_mergeable_map, delta_mergeable_map_at, path}, fill::{FillDefaults, fill_map}, internal::merge::map_with_strategy, model::id::{ModelIdConfig, ModelIdConfigError, ModelIdOrAliasConfig, resolve_alias_chain}, @@ -137,9 +137,12 @@ impl PartialConfigDelta for PartialLlmProviderConfig { fn delta_with_unsets(&self, next: Self, prefix: &str, unsets: &mut Vec) -> Self { Self { - // The map states its own strategy, so a removed alias travels in - // the value as a `replace` and needs no path reported. - aliases: delta_mergeable_map(&self.aliases, next.aliases), + aliases: delta_mergeable_map_at( + &path(prefix, "aliases"), + &self.aliases, + next.aliases, + unsets, + ), anthropic: self.anthropic.delta_with_unsets( next.anthropic, &path(prefix, "anthropic"), diff --git a/crates/jp_config/src/providers/mcp.rs b/crates/jp_config/src/providers/mcp.rs index 2a3290e7b..bda87572e 100644 --- a/crates/jp_config/src/providers/mcp.rs +++ b/crates/jp_config/src/providers/mcp.rs @@ -7,7 +7,10 @@ use serde::{Deserialize, Serialize}; use crate::{ assignment::{AssignKeyValue, AssignResult, KvAssignment, missing_key}, - delta::{PartialConfigDelta, delta_opt, delta_opt_mergeable_vec, delta_opt_partial}, + delta::{ + PartialConfigDelta, delta_opt, delta_opt_mergeable_vec, delta_opt_partial, + delta_opt_partial_at, path, + }, internal::merge::ordered_vec_with_strategy, partial::{ToPartial, partial_opt, partial_opt_config}, types::vec::MergeableVec, @@ -47,8 +50,28 @@ impl PartialConfigDelta for PartialMcpProviderConfig { } } - // No `delta_with_unsets`: `arguments` and `variables` state `replace` - // themselves now, so no field here needs a path reported. + 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` and `variables` state `replace` themselves, so a + // dropped element travels in the value. + 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_at( + &path(prefix, "checksum"), + prev.checksum.as_ref(), + next.checksum, + unsets, + ), + optional: delta_opt(prev.optional.as_ref(), next.optional), + startup_timeout_secs: delta_opt( + prev.startup_timeout_secs.as_ref(), + next.startup_timeout_secs, + ), + }), + } + } } impl McpProviderConfig { diff --git a/crates/jp_config/src/template.rs b/crates/jp_config/src/template.rs index 60645c5de..c739fcb1c 100644 --- a/crates/jp_config/src/template.rs +++ b/crates/jp_config/src/template.rs @@ -4,7 +4,7 @@ use schematic::Config; use crate::{ assignment::{AssignKeyValue, KvAssignment, missing_key}, - delta::{PartialConfigDelta, delta_mergeable_value_map}, + delta::{PartialConfigDelta, delta_mergeable_value_map, delta_mergeable_value_map_at, path}, fill::FillDefaults, internal::merge::map_with_strategy, partial::ToPartial, @@ -43,6 +43,17 @@ impl PartialConfigDelta for PartialTemplateConfig { values: delta_mergeable_value_map(&self.values, next.values), } } + + fn delta_with_unsets(&self, next: Self, prefix: &str, unsets: &mut Vec) -> Self { + Self { + values: delta_mergeable_value_map_at( + &path(prefix, "values"), + &self.values, + next.values, + unsets, + ), + } + } } impl FillDefaults for PartialTemplateConfig { From a7f832fb938e2629cf8dfc2eb0a9585dbd088312 Mon Sep 17 00:00:00 2001 From: Jean Mertz Date: Fri, 18 Sep 2026 22:52:11 +0200 Subject: [PATCH 15/16] fixup! fix(config): Report the map entries and fields a delta cannot merge Signed-off-by: Jean Mertz --- crates/jp_config/src/plugins.rs | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/crates/jp_config/src/plugins.rs b/crates/jp_config/src/plugins.rs index 05b9d8766..09690363f 100644 --- a/crates/jp_config/src/plugins.rs +++ b/crates/jp_config/src/plugins.rs @@ -11,7 +11,7 @@ use schematic::Config; use crate::{ FillDefaults, assignment::{AssignKeyValue, AssignResult, KvAssignment, missing_key}, - delta::{PartialConfigDelta, delta_mergeable_map, delta_opt}, + delta::{PartialConfigDelta, delta_mergeable_map, delta_mergeable_map_at, delta_opt, path}, fill::fill_map, internal::merge::map_with_strategy, partial::ToPartial, @@ -70,6 +70,22 @@ impl PartialConfigDelta for PartialPluginsConfig { command: delta_mergeable_map(&self.command, next.command), } } + + fn delta_with_unsets(&self, next: Self, prefix: &str, unsets: &mut Vec) -> Self { + Self { + auto_install: delta_opt(self.auto_install.as_ref(), next.auto_install), + shutdown_timeout_secs: delta_opt( + self.shutdown_timeout_secs.as_ref(), + next.shutdown_timeout_secs, + ), + command: delta_mergeable_map_at( + &path(prefix, "command"), + &self.command, + next.command, + unsets, + ), + } + } } impl FillDefaults for PartialPluginsConfig { From c08e8dd204fa4065c8be1d1e127e8d4ddfd35966 Mon Sep 17 00:00:00 2001 From: Jean Mertz Date: Fri, 18 Sep 2026 22:52:45 +0200 Subject: [PATCH 16/16] fix(config): Let `--cfg` state a map's merge strategy MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Writing `--cfg 'conversation.tools.cargo_check.options:={"value":{…}, "strategy":"replace"}'` created two options literally named `value` and `strategy`, and the tool ran with neither the setting the user asked for nor a replaced map. The command reported success. A whole-map assignment distributes the object's keys across entries, which is right for a map of values and wrong for the wrapper that states how the map merges. The two are told apart the way the map's own deserializer does it, so the form the field documentation shows means the same thing on the command line as in a config file. An entry named `value` needs the sibling `strategy` before it reads as the wrapper, so a tool called `value` stays assignable, and naming a single entry still leaves whatever wrapper the map already carries alone. Signed-off-by: Jean Mertz --- crates/jp_config/src/assignment.rs | 38 +++++++++++++++- crates/jp_config/src/conversation.rs | 2 +- crates/jp_config/src/conversation/tool.rs | 4 +- .../jp_config/src/conversation/tool_tests.rs | 45 +++++++++++++++++++ crates/jp_config/src/providers.rs | 2 +- crates/jp_config/src/providers/llm.rs | 2 +- crates/jp_config/src/template.rs | 2 +- 7 files changed, 88 insertions(+), 7 deletions(-) diff --git a/crates/jp_config/src/assignment.rs b/crates/jp_config/src/assignment.rs index 632fc541b..8fe5c231f 100644 --- a/crates/jp_config/src/assignment.rs +++ b/crates/jp_config/src/assignment.rs @@ -12,7 +12,10 @@ use schematic::{MergeResult, PartialConfig}; use serde::{Serialize, de::DeserializeOwned}; use serde_json::{Value, from_str}; -use crate::{AppConfig, BoxedError, types::vec::MergeableVec}; +use crate::{ + AppConfig, BoxedError, + types::{map::MergeableMap, vec::MergeableVec}, +}; /// The result of assigning a key-value pair to a configuration. pub type AssignResult = Result<(), BoxedError>; @@ -352,6 +355,39 @@ impl KvAssignment { Ok(()) } + /// Assign a key-value pair to an entry of a map that carries its own merge + /// strategy. + /// + /// Mirrors [`Self::assign_to_entry`], with one more shape to tell apart: a + /// whole-map object carrying `value` beside `strategy` is the wrapper that + /// states how the map merges, not two entries named after those keys. + /// + /// Told apart by the test the map's own deserializer uses, so a strategy + /// written on the command line and one written in a config file mean the + /// same thing. + /// An entry named `value` needs the sibling `strategy` before it reads as + /// the wrapper, which keeps a tool called `value` assignable. + /// + /// Anything else is an entry, and lands inside whatever wrapper the map + /// already carries: naming one entry says nothing about how the map + /// combines. + pub(crate) fn assign_to_mergeable_entry(self, map: &mut MergeableMap) -> AssignResult + where + V: AssignKeyValue + Default + Clone + DeserializeOwned, + { + if self.key.is_empty() + && let KvValue::Json(Value::Object(object)) = &self.value + && object.contains_key("value") + && object.contains_key("strategy") + { + let value = self.value.clone().into_value(); + *map = serde_json::from_value(value).map_err(|error| kv_error(&self.key, error))?; + return Ok(()); + } + + self.assign_to_entry(map) + } + /// Parse an assignment from an environment variable. /// /// The environment variable is expected to be in the format diff --git a/crates/jp_config/src/conversation.rs b/crates/jp_config/src/conversation.rs index 23d825142..3cb18520c 100644 --- a/crates/jp_config/src/conversation.rs +++ b/crates/jp_config/src/conversation.rs @@ -124,7 +124,7 @@ impl AssignKeyValue for PartialConversationConfig { _ if kv.p("tools") => self.tools.assign(kv)?, _ if kv.p("compaction") => self.compaction.assign(kv)?, _ if kv.p("attachments") => kv.try_vec_of_nested(self.attachments.as_mut())?, - _ if kv.p("labels") => kv.assign_to_entry(&mut self.labels)?, + _ if kv.p("labels") => kv.assign_to_mergeable_entry(&mut self.labels)?, _ if kv.p("inquiry") => self.inquiry.assign(kv)?, _ if kv.p("start_local") => self.start_local = kv.try_some_bool()?, "default_id" => self.default_id = kv.try_some_from_str()?, diff --git a/crates/jp_config/src/conversation/tool.rs b/crates/jp_config/src/conversation/tool.rs index c62edcb61..94d25a5ff 100644 --- a/crates/jp_config/src/conversation/tool.rs +++ b/crates/jp_config/src/conversation/tool.rs @@ -59,7 +59,7 @@ impl AssignKeyValue for PartialToolsConfig { match kv.key_string().as_str() { "" => kv.try_merge_object(self)?, _ if kv.p("*") => self.defaults.assign(kv)?, - _ => kv.assign_to_entry(&mut self.tools)?, + _ => kv.assign_to_mergeable_entry(&mut self.tools)?, } Ok(()) @@ -637,7 +637,7 @@ impl AssignKeyValue for PartialToolConfig { "cancellation_response" => self.cancellation_response = kv.try_some_string()?, _ if kv.p("style") => self.style.assign(kv)?, "questions" => self.questions = kv.try_object()?, - _ if kv.p("options") => kv.assign_to_entry(&mut self.options)?, + _ if kv.p("options") => kv.assign_to_mergeable_entry(&mut self.options)?, _ if kv.p("access") => self.access.assign(kv)?, _ => return missing_key(&kv), } diff --git a/crates/jp_config/src/conversation/tool_tests.rs b/crates/jp_config/src/conversation/tool_tests.rs index 2795f421c..38dce8477 100644 --- a/crates/jp_config/src/conversation/tool_tests.rs +++ b/crates/jp_config/src/conversation/tool_tests.rs @@ -984,6 +984,51 @@ fn tools_map_accepts_a_replace_strategy() { assert!(config.tools.contains_key("my_tool")); } +/// The strategy a config file states is the strategy `--cfg` states. +/// +/// The two spellings reach different code — one the map's deserializer, the +/// other the key-value dispatch — and the doc comments advertise the wrapper +/// without saying which of them it is for. +#[test] +fn a_map_strategy_means_the_same_through_cfg_as_through_toml() { + use crate::types::map::MergedMapStrategy; + + let from_toml: PartialToolsConfig = toml::from_str( + r#" + [cargo_check.options] + value = { profile = "release" } + strategy = "replace" + "#, + ) + .expect("a strategy-carrying options map parses from TOML"); + + let mut from_cli = PartialToolsConfig::default(); + let kv = KvAssignment::try_from_cli( + "cargo_check.options:", + r#"{"value":{"profile":"release"},"strategy":"replace"}"#, + ) + .unwrap(); + from_cli.assign(kv).unwrap(); + + for (source, config) in [("toml", &from_toml), ("cfg", &from_cli)] { + let options = &config.tools["cargo_check"].options; + + assert!( + matches!(options, MergeableMap::Merged(merged) + if merged.strategy == Some(MergedMapStrategy::Replace)), + "{source}: the declared strategy is the map's, not an entry: {options:?}" + ); + assert!( + options.contains_key("profile"), + "{source}: the option the user set is the one the tool receives: {options:?}" + ); + assert!( + !options.contains_key("strategy"), + "{source}: the metadata is not an option: {options:?}" + ); + } +} + /// A plain tools map keeps merging per key, and a tool may be named `value`. #[test] fn tools_map_without_a_strategy_merges_per_key() { diff --git a/crates/jp_config/src/providers.rs b/crates/jp_config/src/providers.rs index 6b040f12f..3d7af4df8 100644 --- a/crates/jp_config/src/providers.rs +++ b/crates/jp_config/src/providers.rs @@ -61,7 +61,7 @@ impl AssignKeyValue for PartialProviderConfig { match kv.key_string().as_str() { "" => kv.try_merge_object(self)?, _ if kv.p("llm") => self.llm.assign(kv)?, - _ if kv.p("mcp") => kv.assign_to_entry(&mut self.mcp)?, + _ if kv.p("mcp") => kv.assign_to_mergeable_entry(&mut self.mcp)?, // _ if kv.p("tts") => self.tts.assign(kv)?, _ => return missing_key(&kv), } diff --git a/crates/jp_config/src/providers/llm.rs b/crates/jp_config/src/providers/llm.rs index 45e65d785..bf8e1bf40 100644 --- a/crates/jp_config/src/providers/llm.rs +++ b/crates/jp_config/src/providers/llm.rs @@ -101,7 +101,7 @@ impl AssignKeyValue for PartialLlmProviderConfig { fn assign(&mut self, mut kv: KvAssignment) -> AssignResult { match kv.key_string().as_str() { "" => kv.try_merge_object(self)?, - _ if kv.p("aliases") => kv.assign_to_entry(&mut self.aliases)?, + _ if kv.p("aliases") => kv.assign_to_mergeable_entry(&mut self.aliases)?, _ if kv.p("anthropic") => self.anthropic.assign(kv)?, _ if kv.p("cerebras") => self.cerebras.assign(kv)?, _ if kv.p("deepseek") => self.deepseek.assign(kv)?, diff --git a/crates/jp_config/src/template.rs b/crates/jp_config/src/template.rs index c739fcb1c..206a4c00f 100644 --- a/crates/jp_config/src/template.rs +++ b/crates/jp_config/src/template.rs @@ -29,7 +29,7 @@ impl AssignKeyValue for PartialTemplateConfig { fn assign(&mut self, mut kv: KvAssignment) -> Result<(), crate::BoxedError> { match kv.key_string().as_str() { "" => kv.try_merge_object(self)?, - _ if kv.p("values") => kv.assign_to_entry(&mut self.values)?, + _ if kv.p("values") => kv.assign_to_mergeable_entry(&mut self.values)?, _ => return missing_key(&kv), }