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_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.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/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..a41646ca1 100644 --- a/crates/jp_cli/src/cmd/query/turn_loop_tests.rs +++ b/crates/jp_cli/src/cmd/query/turn_loop_tests.rs @@ -1390,11 +1390,11 @@ 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(), - options: IndexMap::default(), + questions: IndexMap::new().into(), + options: IndexMap::default().into(), access: None, cancellation_response: None, }); @@ -1539,11 +1539,11 @@ 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(), - options: IndexMap::default(), + questions: IndexMap::new().into(), + options: IndexMap::default().into(), access: None, cancellation_response: Some(CUSTOM_CANCELLATION_RESPONSE.to_string()), }); @@ -1679,14 +1679,15 @@ 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, - })]), - options: IndexMap::default(), + })]) + .into(), + options: IndexMap::default().into(), access: None, cancellation_response: None, }); @@ -2021,11 +2022,11 @@ 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(), - options: IndexMap::default(), + questions: IndexMap::new().into(), + options: IndexMap::default().into(), access: None, cancellation_response: None, }); @@ -2177,11 +2178,11 @@ 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(), - options: IndexMap::default(), + questions: IndexMap::new().into(), + options: IndexMap::default().into(), access: None, cancellation_response: None, }); @@ -2290,11 +2291,11 @@ 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(), - options: IndexMap::default(), + questions: IndexMap::new().into(), + options: IndexMap::default().into(), access: None, cancellation_response: None, }); @@ -2434,11 +2435,11 @@ 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(), - options: IndexMap::default(), + questions: IndexMap::new().into(), + options: IndexMap::default().into(), access: None, cancellation_response: None, }); @@ -2589,11 +2590,11 @@ 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(), - options: IndexMap::default(), + questions: IndexMap::new().into(), + options: IndexMap::default().into(), access: None, cancellation_response: None, }); @@ -2716,11 +2717,11 @@ 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(), - options: IndexMap::default(), + questions: IndexMap::new().into(), + options: IndexMap::default().into(), access: None, cancellation_response: None, }); @@ -2855,11 +2856,11 @@ 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(), - options: IndexMap::default(), + questions: IndexMap::new().into(), + options: IndexMap::default().into(), access: None, cancellation_response: None, }); @@ -3010,11 +3011,11 @@ 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(), - options: IndexMap::default(), + questions: IndexMap::new().into(), + options: IndexMap::default().into(), access: None, cancellation_response: None, }); @@ -3031,11 +3032,11 @@ 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(), - options: IndexMap::default(), + questions: IndexMap::new().into(), + options: IndexMap::default().into(), access: None, cancellation_response: None, }); @@ -3219,11 +3220,11 @@ 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(), - options: IndexMap::default(), + questions: IndexMap::new().into(), + options: IndexMap::default().into(), access: None, cancellation_response: None, }); @@ -4526,11 +4527,11 @@ 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(), - options: IndexMap::default(), + questions: IndexMap::new().into(), + options: IndexMap::default().into(), access: None, cancellation_response: None, }); @@ -4546,11 +4547,11 @@ 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(), - options: IndexMap::default(), + questions: IndexMap::new().into(), + options: IndexMap::default().into(), access: None, cancellation_response: None, }); @@ -4719,11 +4720,11 @@ 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(), - options: IndexMap::default(), + questions: IndexMap::new().into(), + options: IndexMap::default().into(), access: None, cancellation_response: None, }); @@ -4938,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, }); @@ -5434,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, @@ -5448,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, }); @@ -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,8 +6002,9 @@ fn inquiry_tool_config(questions: &[&str]) -> ToolConfig { answer: None, }) }) - .collect(), - options: IndexMap::default(), + .collect::>() + .into(), + options: IndexMap::default().into(), access: None, cancellation_response: None, } @@ -7108,11 +7110,11 @@ 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(), - options: IndexMap::default(), + questions: IndexMap::new().into(), + options: IndexMap::default().into(), access: None, cancellation_response: None, }); @@ -7543,11 +7545,11 @@ 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(), - options: IndexMap::default(), + questions: IndexMap::new().into(), + options: IndexMap::default().into(), access: None, cancellation_response: None, }); @@ -7975,11 +7977,11 @@ 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(), - options: IndexMap::default(), + questions: IndexMap::new().into(), + options: IndexMap::default().into(), access: None, cancellation_response: None, }); @@ -8101,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/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_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_cli/src/ctx.rs b/crates/jp_cli/src/ctx.rs index 7ecbf6ab9..ddd57ca44 100644 --- a/crates/jp_cli/src/ctx.rs +++ b/crates/jp_cli/src/ctx.rs @@ -130,7 +130,7 @@ impl Ctx { 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()) + 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(); 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 ebda01226..3cb18520c 100644 --- a/crates/jp_config/src/conversation.rs +++ b/crates/jp_config/src/conversation.rs @@ -14,16 +14,19 @@ 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_map_at, 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, @@ -121,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()?, @@ -132,41 +135,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 +145,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), } } @@ -186,7 +154,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 @@ -199,7 +169,12 @@ impl PartialConfigDelta for PartialConversationConfig { next.default_id, unsets, ), - labels: self.labels_delta(next.labels), + labels: delta_mergeable_map_at( + &path(prefix, "labels"), + &self.labels, + next.labels, + unsets, + ), } } } 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/conversation/tool.rs b/crates/jp_config/src/conversation/tool.rs index d09eeb8c0..94d25a5ff 100644 --- a/crates/jp_config/src/conversation/tool.rs +++ b/crates/jp_config/src/conversation/tool.rs @@ -17,11 +17,18 @@ use crate::{ access::{AccessConfig, PartialAccessConfig}, style::{DisplayStyleConfig, PartialDisplayStyleConfig}, }, - delta::{PartialConfigDelta, delta_map, delta_opt, delta_opt_partial, delta_vec}, + delta::{ + 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, partial::{ToPartial, partial_opt, partial_opt_config, partial_opts}, - types::json_value::JsonValue, - util::merge_nested_indexmap, + types::{ + json_value::JsonValue, + map::{MergeableMap, map_to_partial_per_key}, + }, validate::Validator, }; @@ -43,8 +50,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 { @@ -52,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(()) @@ -63,7 +70,16 @@ 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), + } + } + + 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_mergeable_map_at(prefix, &self.tools, next.tools, unsets), } } } @@ -82,21 +98,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(); + 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, } } } @@ -127,7 +163,8 @@ impl ToPartial for ToolsConfig { (name.clone(), tool) }) - .collect(); + .collect::>() + .into(); Self::Partial { defaults, tools } } @@ -219,7 +256,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; } @@ -363,6 +400,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 { @@ -470,8 +534,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. /// @@ -517,16 +586,26 @@ 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. /// /// 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. /// @@ -558,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), } @@ -576,7 +655,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), @@ -585,20 +664,69 @@ impl PartialConfigDelta for PartialToolConfig { next.cancellation_response, ), 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(), + questions: delta_mergeable_map(&self.questions, next.questions), + options: delta_mergeable_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_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), + 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_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(), + next.access, + unsets, + ), + } + } } impl ToPartial for ToolConfig { @@ -612,11 +740,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), @@ -625,16 +751,13 @@ 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(), - options: self - .options - .iter() - .map(|(k, v)| (k.clone(), v.clone())) - .collect(), + questions: map_to_partial_per_key(self.questions.iter()), + options: MergeableMap::Map( + self.options + .iter() + .map(|(k, v)| (k.clone(), v.clone())) + .collect(), + ), access: partial_opt_config(self.access.as_ref(), defaults.access), } } @@ -718,10 +841,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 { @@ -737,7 +865,66 @@ 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), + 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, + ), } } } @@ -755,11 +942,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()), } } } @@ -1133,7 +1316,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 } @@ -1216,13 +1399,13 @@ 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 } /// Return the per-tool options map. #[must_use] - pub const fn options(&self) -> &IndexMap { + pub fn options(&self) -> &IndexMap { &self.tool.options } @@ -1664,6 +1847,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/conversation/tool_tests.rs b/crates/jp_config/src/conversation/tool_tests.rs index dd4c2fd57..38dce8477 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,97 @@ 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")); +} + +/// 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() { + 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 a753246c6..d8d653a14 100644 --- a/crates/jp_config/src/delta.rs +++ b/crates/jp_config/src/delta.rs @@ -1,9 +1,11 @@ //! Configuration delta calculation. -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 +112,44 @@ 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. +/// +/// 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)) { + 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 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 @@ -152,19 +192,31 @@ pub fn delta_opt_partial_at( } } -/// Calculate the delta between two maps, reporting each entry's unsets. +/// Calculate the delta between two strategy-carrying maps, reporting what +/// merging cannot reach. /// -/// Mirrors [`delta_map`], descending into each entry with the entry's own -/// dotted path so a field inside it reports where it lives. -pub fn delta_map_with_unsets( +/// 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: &IndexMap, - next: IndexMap, + prev: &MergeableMap, + next: MergeableMap, unsets: &mut Vec, -) -> IndexMap +) -> MergeableMap where - V: PartialConfigDelta + PartialEq, + 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 { @@ -185,6 +237,75 @@ where .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 +/// 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)) { + 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 optional values, reporting a cleared field. /// /// A value that went away cannot be expressed by merging: schematic keeps the @@ -226,35 +347,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_law_tests.rs b/crates/jp_config/src/delta_law_tests.rs index 0d99ffe09..ddcd58622 100644 --- a/crates/jp_config/src/delta_law_tests.rs +++ b/crates/jp_config/src/delta_law_tests.rs @@ -81,35 +81,27 @@ 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 /// 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..9741b3045 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,10 +26,78 @@ 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 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] +fn map_delta_replaces_when_an_entry_is_removed() { + let prev = map(&["--a"]); + let next = MergeableMap::default(); + + let delta = delta_mergeable_map(&prev, next); + + 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 diffed, not replaced. +#[test] +fn map_delta_diffs_a_surviving_entry() { + let prev = map(&["--a"]); + let next = map(&["--a", "--b"]); + + let delta = delta_mergeable_map(&prev, next); + + assert!( + 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. @@ -254,10 +325,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] @@ -265,7 +336,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()])); @@ -279,7 +350,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(), @@ -288,13 +359,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/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..9e7038ee2 100644 --- a/crates/jp_config/src/lib_tests.rs +++ b/crates/jp_config/src/lib_tests.rs @@ -772,6 +772,119 @@ fn a_dropped_mcp_argument_is_recorded() { ); } +/// 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}; + + 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"], + "the removed key reports its path, so filling cannot restore it" + ); + assert!( + delta.providers.mcp.discard_when_merged() || !delta.providers.mcp.is_empty(), + "the delta carries the map the user is left with" + ); + + let mut folded = prev.to_partial(); + 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 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/plugins.rs b/crates/jp_config/src/plugins.rs index 801c433d2..09690363f 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::{PartialConfigDelta, delta_mergeable_map, delta_mergeable_map_at, delta_opt, path}, 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 { @@ -56,26 +61,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_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: 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_mergeable_map_at( + &path(prefix, "command"), + &self.command, + next.command, + unsets, + ), } } } @@ -87,7 +95,13 @@ 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(), + }, } } } @@ -102,11 +116,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.rs b/crates/jp_config/src/providers.rs index cbb23b130..3d7af4df8 100644 --- a/crates/jp_config/src/providers.rs +++ b/crates/jp_config/src/providers.rs @@ -3,19 +3,19 @@ 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, delta_mergeable_map_at, path}, fill::{FillDefaults, fill_map}, + internal::merge::map_with_strategy, partial::ToPartial, providers::{ llm::{LlmProviderConfig, PartialLlmProviderConfig}, mcp::McpProviderConfig, }, - util::merge_nested_indexmap, + types::map::{MergeableMap, map_to_partial_per_key}, validate::Validator, }; @@ -34,8 +34,20 @@ pub struct ProviderConfig { /// /// Configuration for Model Context Protocol (MCP) servers. /// The key is the server ID. - #[setting(nested, merge = merge_nested_indexmap)] - pub mcp: IndexMap, + /// + /// ```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. + /// Declare the map as `{ value = { … }, strategy = "replace" }` to drop + /// them instead. + #[setting(nested, merge = map_with_strategy)] + pub mcp: MergeableMap, } impl Validator for ProviderConfig { @@ -49,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), } @@ -65,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), } } @@ -74,7 +86,7 @@ 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), + mcp: delta_mergeable_map_at(&path(prefix, "mcp"), &self.mcp, next.mcp, unsets), } } } @@ -83,7 +95,14 @@ 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(), + }, } } } @@ -92,11 +111,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/providers/llm.rs b/crates/jp_config/src/providers/llm.rs index 6a834f760..bf8e1bf40 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, 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}, partial::ToPartial, providers::llm::{ @@ -28,7 +29,7 @@ use crate::{ openai::{OpenaiConfig, PartialOpenaiConfig}, openrouter::{OpenrouterConfig, PartialOpenrouterConfig}, }, - util::merge_nested_indexmap, + types::map::{MergeableMap, map_to_partial_per_key}, validate::Validator, }; @@ -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)] @@ -95,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)?, @@ -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,7 +137,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_mergeable_map_at( + &path(prefix, "aliases"), + &self.aliases, + next.aliases, + unsets, + ), anthropic: self.anthropic.delta_with_unsets( next.anthropic, &path(prefix, "anthropic"), @@ -155,7 +166,13 @@ 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), @@ -171,11 +188,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/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/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/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..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( @@ -211,11 +213,15 @@ PartialAppConfig { }, }, template: PartialTemplateConfig { - values: {}, + values: Map( + {}, + ), }, providers: PartialProviderConfig { llm: PartialLlmProviderConfig { - aliases: {}, + aliases: Map( + {}, + ), anthropic: PartialAnthropicConfig { auth: None, api_key_env: None, @@ -253,12 +259,16 @@ PartialAppConfig { base_url: None, }, }, - mcp: {}, + mcp: Map( + {}, + ), }, 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 014fb57ad..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( @@ -441,11 +443,15 @@ Ok( }, }, template: PartialTemplateConfig { - values: {}, + values: Map( + {}, + ), }, providers: PartialProviderConfig { llm: PartialLlmProviderConfig { - aliases: {}, + aliases: Map( + {}, + ), anthropic: PartialAnthropicConfig { auth: Some( [ @@ -525,7 +531,9 @@ Ok( ), }, }, - mcp: {}, + mcp: Map( + {}, + ), }, plugins: PartialPluginsConfig { auto_install: Some( @@ -534,7 +542,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 a9220c267..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( @@ -211,11 +213,15 @@ PartialAppConfig { }, }, template: PartialTemplateConfig { - values: {}, + values: Map( + {}, + ), }, providers: PartialProviderConfig { llm: PartialLlmProviderConfig { - aliases: {}, + aliases: Map( + {}, + ), anthropic: PartialAnthropicConfig { auth: None, api_key_env: None, @@ -253,12 +259,16 @@ PartialAppConfig { base_url: None, }, }, - mcp: {}, + mcp: Map( + {}, + ), }, plugins: PartialPluginsConfig { auto_install: None, shutdown_timeout_secs: None, - command: {}, + command: Map( + {}, + ), }, user: PartialUserConfig { name: None, diff --git a/crates/jp_config/src/template.rs b/crates/jp_config/src/template.rs index 8174269c5..206a4c00f 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::{PartialConfigDelta, delta_mergeable_value_map, delta_mergeable_value_map_at, path}, 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,15 +16,20 @@ 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 { 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), } @@ -36,34 +40,41 @@ 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_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 { - 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(), + ), } } } diff --git a/crates/jp_config/src/types/map.rs b/crates/jp_config/src/types/map.rs index 8553db23f..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) @@ -182,6 +185,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) 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 = diff --git a/crates/jp_config/src/util.rs b/crates/jp_config/src/util.rs index f4f5cef43..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::{ @@ -417,7 +416,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, @@ -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 { diff --git a/crates/jp_conversation/src/compat.rs b/crates/jp_conversation/src/compat.rs index d5e4f534c..c74a47cc4 100644 --- a/crates/jp_conversation/src/compat.rs +++ b/crates/jp_conversation/src/compat.rs @@ -325,9 +325,59 @@ 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. +/// +/// 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 variants = || { + union_type + .variants_types + .iter() + .map(Box::as_ref) + .filter(|variant| !variant.is_null()) + }; + + 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().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()) { @@ -392,7 +442,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 { @@ -403,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 @@ -426,7 +493,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() @@ -434,11 +501,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), } } 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; 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)?,