diff --git a/crates/contrib/schematic_macros/src/common/field.rs b/crates/contrib/schematic_macros/src/common/field.rs index e878148d7..a6ff88408 100644 --- a/crates/contrib/schematic_macros/src/common/field.rs +++ b/crates/contrib/schematic_macros/src/common/field.rs @@ -150,7 +150,11 @@ impl Field<'_> { } value_type } else { - FieldValue::value(result.value) + // `partial_via` applies to a plain field too, so a list of scalars + // can carry a wrapper that knows its own merge strategy. The + // partial holds the via type and `generate_from_partial_value` + // converts back to the field's own type. + FieldValue::value(result.partial_via_ty.as_ref().unwrap_or(result.value)) }; result diff --git a/crates/contrib/schematic_macros/src/config/field.rs b/crates/contrib/schematic_macros/src/config/field.rs index 53db0e33f..f0b270cdb 100644 --- a/crates/contrib/schematic_macros/src/config/field.rs +++ b/crates/contrib/schematic_macros/src/config/field.rs @@ -152,22 +152,43 @@ impl Field<'_> { #[allow(clippy::collapsible_else_if)] if matches!(self.value_type, FieldValue::Value { .. }) { + // A `partial_via` field stores the via type in the partial and its + // own type in the resolved config, so the value converts on the way + // out. The conversion wraps the *inner* access, before any boxing, + // since it is the value that changes type and not its container. + let via = self.args.partial_via.is_some(); + let convert = |value: TokenStream| { + if via { + quote! { Into::into(#value) } + } else { + value + } + }; + if self.value_type.is_outer_boxed() { if self.is_nullable() { - quote! { partial.#key.map(Box::new) } + let inner = convert(quote! { value }); + quote! { partial.#key.map(|value| Box::new(#inner)) } } else { - quote! { Box::new(partial.#key) } + let inner = convert(quote! { partial.#key }); + quote! { Box::new(#inner) } } } else { if self.is_nullable() { // Use optional values as-is as they're already wrapped in `Option` - quote! { partial.#key } + if via { + quote! { partial.#key.map(Into::into) } + } else { + quote! { partial.#key } + } } else if self.is_required() { // Trigger a validation error if the value is missing - quote! { partial.#key.ok_or(schematic::ConfigError::MissingRequired{ fields: { let mut fields = fields.clone(); fields.push(#key_quoted.to_owned()); fields } })? } + convert( + quote! { partial.#key.ok_or(schematic::ConfigError::MissingRequired{ fields: { let mut fields = fields.clone(); fields.push(#key_quoted.to_owned()); fields } })? }, + ) } else { // Otherwise unwrap the resolved value or use the type default - quote! { partial.#key.unwrap_or_default() } + convert(quote! { partial.#key.unwrap_or_default() }) } } } else { 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/query.rs b/crates/jp_cli/src/cmd/query.rs index 02773a54b..7bf12ea05 100644 --- a/crates/jp_cli/src/cmd/query.rs +++ b/crates/jp_cli/src/cmd/query.rs @@ -2068,12 +2068,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 413395e32..45283f1ee 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 091f0af8b..ef49e997a 100644 --- a/crates/jp_cli/src/cmd/query/tool/coordinator_tests.rs +++ b/crates/jp_cli/src/cmd/query/tool/coordinator_tests.rs @@ -174,7 +174,8 @@ fn test_question_target_with_configured_question() { target: Some(QuestionTarget::Assistant(Box::default())), answer: None, } - }, + } + .into(), ..Default::default() }, vec![], @@ -231,7 +232,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 f4ff4bf82..aa29a2921 100644 --- a/crates/jp_cli/src/cmd/query/turn_loop_tests.rs +++ b/crates/jp_cli/src/cmd/query/turn_loop_tests.rs @@ -1217,11 +1217,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, }); @@ -1366,11 +1366,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()), }); @@ -1506,14 +1506,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, }); @@ -1848,11 +1849,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, }); @@ -2004,11 +2005,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, }); @@ -2117,11 +2118,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, }); @@ -2261,11 +2262,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, }); @@ -2416,11 +2417,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, }); @@ -2543,11 +2544,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, }); @@ -2682,11 +2683,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, }); @@ -2837,11 +2838,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, }); @@ -2858,11 +2859,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, }); @@ -3046,11 +3047,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, }); @@ -4266,11 +4267,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, }); @@ -4286,11 +4287,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, }); @@ -4459,11 +4460,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, }); @@ -4697,7 +4698,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 @@ -4708,8 +4709,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, } @@ -5815,11 +5817,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, }); @@ -6250,11 +6252,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, }); @@ -6682,11 +6684,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, }); diff --git a/crates/jp_cli/src/cmd/query_tests.rs b/crates/jp_cli/src/cmd/query_tests.rs index cf2f20d91..1a0294e88 100644 --- a/crates/jp_cli/src/cmd/query_tests.rs +++ b/crates/jp_cli/src/cmd/query_tests.rs @@ -45,7 +45,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() @@ -1118,7 +1118,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() @@ -1481,7 +1481,7 @@ async fn query_sequence_new_cfg_profile_then_model_override_persists_for_plain_q .unwrap(); let mut base = AppConfig::new_test().to_partial(); - base.config_load_paths = Some(vec![RelativePathBuf::from(".jp/config")]); + base.config_load_paths = Some(vec![RelativePathBuf::from(".jp/config")].into()); base.providers.llm.aliases.insert( "gpt".to_owned(), ModelIdConfig { diff --git a/crates/jp_cli/src/config_pipeline.rs b/crates/jp_cli/src/config_pipeline.rs index f91e4621a..bf185dcf4 100644 --- a/crates/jp_cli/src/config_pipeline.rs +++ b/crates/jp_cli/src/config_pipeline.rs @@ -384,7 +384,7 @@ fn resolve_cfg_args( let load_paths: Vec = base .config_load_paths .iter() - .flatten() + .flat_map(|paths| paths.iter()) .filter_map(|p| { Utf8PathBuf::try_from(p.to_path(root)) .inspect_err(|e| { diff --git a/crates/jp_cli/src/config_pipeline_tests.rs b/crates/jp_cli/src/config_pipeline_tests.rs index f9b9da41b..7159499a9 100644 --- a/crates/jp_cli/src/config_pipeline_tests.rs +++ b/crates/jp_cli/src/config_pipeline_tests.rs @@ -66,7 +66,7 @@ fn conversation_layer_overrides_base() { fn mcp_server(argument: &str) -> PartialMcpProviderConfig { PartialMcpProviderConfig::Stdio(PartialStdioConfig { command: Some("just".into()), - arguments: Some(vec![argument.to_owned()]), + arguments: Some(vec![argument.to_owned()].into()), ..PartialStdioConfig::default() }) } @@ -74,7 +74,7 @@ fn mcp_server(argument: &str) -> PartialMcpProviderConfig { /// The `arguments` of a server in a resolved partial. fn mcp_arguments(partial: &PartialAppConfig, server: &str) -> Option> { let PartialMcpProviderConfig::Stdio(config) = partial.providers.mcp.get(server)?; - config.arguments.clone() + config.arguments.as_deref().cloned() } /// The per-conversation layer is a resolved snapshot, not a contribution. diff --git a/crates/jp_cli/src/ctx.rs b/crates/jp_cli/src/ctx.rs index 2e3a9940e..e10e3df08 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 973609198..f60697603 100644 --- a/crates/jp_config/src/assignment.rs +++ b/crates/jp_config/src/assignment.rs @@ -12,7 +12,7 @@ use schematic::PartialConfig; use serde::{Serialize, de::DeserializeOwned}; use serde_json::{Value, from_str}; -use crate::{AppConfig, BoxedError}; +use crate::{AppConfig, BoxedError, types::vec::MergeableVec}; /// The result of assigning a key-value pair to a configuration. pub type AssignResult = Result<(), BoxedError>; @@ -1000,23 +1000,6 @@ impl KvAssignment { Ok(()) } - /// Convenience method for [`Self::try_vec`] that takes an optional target. - /// - /// A `null` value clears the field to `None` rather than to an empty list; - /// the two merge differently. - pub(crate) fn try_some_vec( - self, - vec: &mut Option>, - parser: impl Fn(Self) -> Result, - ) -> Result<(), KvAssignmentError> { - if self.clears_collection() { - *vec = None; - return Ok(()); - } - - self.try_vec(vec.get_or_insert_default(), parser) - } - /// Specialized version of [`Self::try_vec`] for parsing a JSON array of /// strings. pub(crate) fn try_vec_of_strings(self, vec: &mut Vec) -> Result<(), KvAssignmentError> @@ -1051,6 +1034,62 @@ impl KvAssignment { self.try_vec_of_strings(vec.get_or_insert_default()) } + /// Assign to a list that carries its own merge strategy. + /// + /// Accepts either the list itself or a `{ value, strategy }` object, which + /// is how the user declares a strategy for the field. + /// The two are told apart by shape, the same way [`MergeableVec`]'s own + /// deserializer does it: a sequence cannot be a table. + pub(crate) fn try_some_mergeable_vec( + self, + vec: &mut Option>, + parser: impl Fn(Self) -> Result, + ) -> Result<(), KvAssignmentError> + where + T: Clone + DeserializeOwned, + { + // An absent list and an empty one merge differently: `None` lets a + // later layer's value land verbatim, `Some([])` still runs the field's + // merge strategy against it. + if self.clears_collection() { + *vec = None; + return Ok(()); + } + + // An object declares a strategy alongside the value, so it is parsed as + // the wrapper rather than element by element. + if let KvValue::Json(value @ Value::Object(_)) = self.value.clone() { + let merged = + serde_json::from_value(value).map_err(|error| kv_error(&self.key, error))?; + + *vec = Some(merged); + return Ok(()); + } + + let mut elements = vec.take().map(MergeableVec::into_vec).unwrap_or_default(); + self.try_vec(&mut elements, parser)?; + *vec = Some(elements.into()); + + Ok(()) + } + + /// Convenience method for [`Self::try_some_mergeable_vec`] whose elements + /// are built from strings. + pub(crate) fn try_some_mergeable_strings( + self, + vec: &mut Option>, + ) -> Result<(), KvAssignmentError> + where + T: Clone + From + DeserializeOwned, + { + let parser = |kv: Self| match kv.value.clone().into_value() { + Value::String(v) => Ok(v.into()), + _ => type_error(kv.key(), &kv.value, &["string"]).map_err(Into::into), + }; + + self.try_some_mergeable_vec(vec, parser) + } + /// Try to parse the value as a JSON array of partial configs, and set or /// merge the elements. pub(crate) fn try_vec_of_nested(mut self, vec: &mut Vec) -> Result<(), KvAssignmentError> diff --git a/crates/jp_config/src/assistant.rs b/crates/jp_config/src/assistant.rs index cff9b95db..2be552447 100644 --- a/crates/jp_config/src/assistant.rs +++ b/crates/jp_config/src/assistant.rs @@ -20,7 +20,9 @@ use crate::{ sections::{PartialSectionConfig, SectionConfig}, tool_choice::ToolChoice, }, - delta::{PartialConfigDelta, delta_opt, delta_opt_partial, path}, + delta::{ + PartialConfigDelta, delta_mergeable_vec, delta_opt, delta_opt_at, delta_opt_partial, path, + }, fill::{FillDefaults, fill_opt}, internal::merge::{string_with_strategy, vec_with_strategy}, model::{ModelConfig, PartialModelConfig}, @@ -127,17 +129,11 @@ impl PartialConfigDelta for PartialAssistantConfig { Self { name: delta_opt(self.name.as_ref(), next.name), system_prompt: delta_opt_partial(self.system_prompt.as_ref(), next.system_prompt), - instructions: next - .instructions - .into_iter() - .filter(|v| !self.instructions.contains(v)) - .collect::>() - .into(), - system_prompt_sections: next - .system_prompt_sections - .into_iter() - .filter(|v| !self.system_prompt_sections.contains(v)) - .collect(), + instructions: delta_mergeable_vec(&self.instructions, next.instructions), + system_prompt_sections: delta_mergeable_vec( + &self.system_prompt_sections, + next.system_prompt_sections, + ), tool_choice: delta_opt(self.tool_choice.as_ref(), next.tool_choice), model: self.model.delta(next.model), request: self.request.delta(next.request), @@ -146,24 +142,20 @@ impl PartialConfigDelta for PartialAssistantConfig { fn delta_with_unsets(&self, next: Self, prefix: &str, unsets: &mut Vec) -> Self { Self { - name: delta_opt(self.name.as_ref(), next.name), + name: delta_opt_at(&path(prefix, "name"), self.name.as_ref(), next.name, unsets), system_prompt: delta_opt_partial(self.system_prompt.as_ref(), next.system_prompt), - instructions: next - .instructions - .into_iter() - .filter(|v| !self.instructions.contains(v)) - .collect::>() - .into(), - system_prompt_sections: next - .system_prompt_sections - .into_iter() - .filter(|v| !self.system_prompt_sections.contains(v)) - .collect(), + instructions: delta_mergeable_vec(&self.instructions, next.instructions), + system_prompt_sections: delta_mergeable_vec( + &self.system_prompt_sections, + next.system_prompt_sections, + ), tool_choice: delta_opt(self.tool_choice.as_ref(), next.tool_choice), model: self .model .delta_with_unsets(next.model, &path(prefix, "model"), unsets), - request: self.request.delta(next.request), + request: self + .request + .delta_with_unsets(next.request, &path(prefix, "request"), unsets), } } } diff --git a/crates/jp_config/src/assistant/request.rs b/crates/jp_config/src/assistant/request.rs index f7a79e005..24bdd2ef2 100644 --- a/crates/jp_config/src/assistant/request.rs +++ b/crates/jp_config/src/assistant/request.rs @@ -7,7 +7,7 @@ use serde::{Deserialize, Serialize}; use crate::{ assignment::{AssignKeyValue, AssignResult, KvAssignment, missing_key}, - delta::{PartialConfigDelta, delta_opt}, + delta::{PartialConfigDelta, delta_opt, delta_opt_at, path}, fill::FillDefaults, partial::{ToPartial, partial_opt}, validate::Validator, @@ -191,6 +191,47 @@ impl PartialConfigDelta for PartialRequestConfig { cache: delta_opt(self.cache.as_ref(), next.cache), } } + + fn delta_with_unsets(&self, next: Self, prefix: &str, unsets: &mut Vec) -> Self { + Self { + max_retries: delta_opt_at( + &path(prefix, "max_retries"), + self.max_retries.as_ref(), + next.max_retries, + unsets, + ), + base_backoff_ms: delta_opt_at( + &path(prefix, "base_backoff_ms"), + self.base_backoff_ms.as_ref(), + next.base_backoff_ms, + unsets, + ), + max_backoff_secs: delta_opt_at( + &path(prefix, "max_backoff_secs"), + self.max_backoff_secs.as_ref(), + next.max_backoff_secs, + unsets, + ), + stream_idle_timeout_secs: delta_opt_at( + &path(prefix, "stream_idle_timeout_secs"), + self.stream_idle_timeout_secs.as_ref(), + next.stream_idle_timeout_secs, + unsets, + ), + max_response_bytes: delta_opt_at( + &path(prefix, "max_response_bytes"), + self.max_response_bytes.as_ref(), + next.max_response_bytes, + unsets, + ), + cache: delta_opt_at( + &path(prefix, "cache"), + self.cache.as_ref(), + next.cache, + unsets, + ), + } + } } impl FillDefaults for PartialRequestConfig { diff --git a/crates/jp_config/src/conversation.rs b/crates/jp_config/src/conversation.rs index 8cd5fbb49..1237ee853 100644 --- a/crates/jp_config/src/conversation.rs +++ b/crates/jp_config/src/conversation.rs @@ -18,16 +18,18 @@ use crate::{ conversation::{ attachment::{AttachmentConfig, PartialAttachmentConfig}, compaction::{CompactionConfig, PartialCompactionConfig}, - label::{LabelConfig, PartialLabelConfig}, + label::LabelConfig, title::{PartialTitleConfig, TitleConfig}, tool::{PartialToolsConfig, ToolsConfig}, }, - delta::{PartialConfigDelta, delta_opt, path}, + delta::{ + PartialConfigDelta, delta_mergeable_map, delta_mergeable_vec, delta_opt, delta_opt_at, path, + }, fill::FillDefaults, internal::merge::{map_with_strategy, vec_with_strategy}, partial::{ToPartial, partial_opt}, types::{ - map::{MergeableMap, MergedMap, MergedMapStrategy, map_to_mergeable_partial}, + map::{MergeableMap, map_to_mergeable_partial}, vec::{MergeableVec, MergedVec, vec_to_mergeable_partial}, }, validate::Validator, @@ -136,64 +138,17 @@ impl AssignKeyValue for PartialConversationConfig { } } -impl PartialConversationConfig { - /// The attachments `next` adds. - fn attachments_delta( - &self, - next: &MergeableVec, - ) -> MergeableVec { - next.iter() - .filter(|v| !self.attachments.contains(v)) - .cloned() - .collect::>() - .into() - } - - /// 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 { title: self.title.delta(next.title), tools: self.tools.delta(next.tools), compaction: self.compaction.delta(next.compaction), - attachments: self.attachments_delta(&next.attachments), + attachments: delta_mergeable_vec(&self.attachments, next.attachments), 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), } } @@ -202,15 +157,22 @@ 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: self.attachments_delta(&next.attachments), + attachments: delta_mergeable_vec(&self.attachments, next.attachments), inquiry: self .inquiry .delta_with_unsets(next.inquiry, &path(prefix, "inquiry"), unsets), 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), + default_id: delta_opt_at( + &path(prefix, "default_id"), + self.default_id.as_ref(), + next.default_id, + unsets, + ), + labels: delta_mergeable_map(&self.labels, next.labels), } } } diff --git a/crates/jp_config/src/conversation/compaction.rs b/crates/jp_config/src/conversation/compaction.rs index 8a8df004f..0940818ea 100644 --- a/crates/jp_config/src/conversation/compaction.rs +++ b/crates/jp_config/src/conversation/compaction.rs @@ -87,6 +87,17 @@ impl AssignKeyValue for PartialCompactionConfig { impl PartialConfigDelta for PartialCompactionConfig { fn delta(&self, next: Self) -> Self { Self { + // 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 802a34108..71e81f2e4 100644 --- a/crates/jp_config/src/conversation/tool.rs +++ b/crates/jp_config/src/conversation/tool.rs @@ -17,11 +17,17 @@ 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_value_map, delta_opt, + delta_opt_at, delta_opt_partial, delta_opt_partial_at, delta_vec, path, + }, fill::{FillDefaults, fill_map}, + internal::merge::map_with_strategy, 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 +49,8 @@ pub struct ToolsConfig { /// This section configures individual tools. /// The key is the tool ID, and cannot contain a comma: a comma separates /// one tool ID from the next wherever several are named at once. - #[setting(nested, flatten, merge = merge_nested_indexmap)] - tools: IndexMap, + #[setting(nested, flatten, merge = map_with_strategy)] + tools: MergeableMap, } impl AssignKeyValue for PartialToolsConfig { @@ -63,7 +69,18 @@ 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), + // The map states its own strategy, so a removed tool travels in + // the value as a `replace` and needs no path reported. + tools: delta_mergeable_map(&self.tools, next.tools), } } } @@ -82,21 +99,41 @@ impl FillDefaults for PartialToolsConfig { // tool's grants must be complete where they are written, so the `*` // block applies whole or not at all, `fs` and `env` together (resolved // in `ToolConfigWithDefaults::access`). - let tools = self - .tools - .into_iter() - .map(|(name, mut tool)| { - tool.style = tool - .style - .map(|style| style.fill_from(tool_defaults.style.clone())); + let fill_style = |mut tool: PartialToolConfig| { + tool.style = tool + .style + .map(|style| style.fill_from(tool_defaults.style.clone())); + tool + }; - (name, tool) - }) - .collect(); + 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 +164,8 @@ impl ToPartial for ToolsConfig { (name.clone(), tool) }) - .collect(); + .collect::>() + .into(); Self::Partial { defaults, tools } } @@ -219,7 +257,7 @@ fn reject_comma_in_tool_names(tools: &ToolsConfig) -> Result<(), ConfigError> { /// reporting; the `'*'` defaults make no claim about any individual tool, so /// they pass over builtin and MCP tools instead of failing the whole config. fn reject_access_on_non_local_tools(tools: &ToolsConfig) -> Result<(), ConfigError> { - for (name, tool) in &tools.tools { + for (name, tool) in tools.tools.iter() { if tool.access.is_none() { continue; } @@ -363,6 +401,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 +535,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 +587,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. /// @@ -576,7 +656,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 +665,56 @@ 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), + // Each map states its own strategy, so a removed entry travels in + // the value as a `replace` and needs no path reported. + parameters: delta_mergeable_map(&self.parameters, next.parameters), + run: delta_opt(self.run.as_ref(), next.run), + format: delta_opt(self.format.as_ref(), next.format), + result: delta_opt(self.result.as_ref(), next.result), + 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(&self.questions, next.questions), + options: delta_mergeable_value_map(&self.options, next.options), + access: delta_opt_partial_at( + &path(prefix, "access"), + self.access.as_ref(), + next.access, + unsets, + ), + } + } } impl ToPartial for ToolConfig { @@ -612,11 +728,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 +739,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 +829,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 +853,7 @@ impl PartialConfigDelta for PartialToolParameterConfig { // any element has to record the whole list. enumeration: delta_opt(self.enumeration.as_ref(), next.enumeration), items: delta_opt(self.items.as_ref(), next.items), - properties: delta_map(&self.properties, next.properties), + properties: delta_mergeable_map(&self.properties, next.properties), } } } @@ -755,11 +871,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()), } } } @@ -1122,7 +1234,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 } @@ -1205,13 +1317,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 } @@ -1653,6 +1765,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/access.rs b/crates/jp_config/src/conversation/tool/access.rs index 40db292df..94337aa9f 100644 --- a/crates/jp_config/src/conversation/tool/access.rs +++ b/crates/jp_config/src/conversation/tool/access.rs @@ -48,10 +48,10 @@ use serde::{Deserialize, Serialize}; use crate::{ BoxedError, assignment::{AssignKeyValue, AssignResult, KvAssignment, missing_key}, - delta::PartialConfigDelta, + delta::{PartialConfigDelta, delta_mergeable_vec}, internal::merge::vec_with_strategy, partial::{ToPartial, partial_opt, partial_opts}, - types::vec::{MergeableVec, MergedVec, MergedVecStrategy, vec_to_mergeable_partial}, + types::vec::{MergeableVec, vec_to_mergeable_partial}, }; /// Resource access grants for a tool. @@ -119,41 +119,12 @@ impl AssignKeyValue for PartialAccessConfig { impl PartialConfigDelta for PartialAccessConfig { fn delta(&self, next: Self) -> Self { Self { - fs: rule_delta(&self.fs, next.fs), - env: rule_delta(&self.env, next.env), + fs: delta_mergeable_vec(&self.fs, next.fs), + env: delta_mergeable_vec(&self.env, next.env), } } } -/// Diff two rule lists into a delta that replays to `next`. -/// -/// An append-shaped delta can only add, so a rule that disappeared between -/// `prev` and `next` would come back when the delta is folded over `prev` -/// again. -/// When anything is missing from `next`, the delta therefore carries the whole -/// list with `replace`; otherwise it carries just the new rules and appends. -/// -/// `next` comes from a fully resolved config, so it is the complete rule set -/// and replacing with it loses nothing. -fn rule_delta( - prev: &MergeableVec, - next: MergeableVec, -) -> MergeableVec { - if prev.iter().all(|rule| next.contains(rule)) { - return next - .into_iter() - .filter(|rule| !prev.contains(rule)) - .collect(); - } - - MergeableVec::Merged(MergedVec { - value: next.into_vec(), - strategy: Some(MergedVecStrategy::Replace), - dedup: None, - discard_when_merged: false, - }) -} - impl ToPartial for AccessConfig { fn to_partial(&self) -> Self::Partial { Self::Partial { diff --git a/crates/jp_config/src/conversation/tool/style.rs b/crates/jp_config/src/conversation/tool/style.rs index 78b1daaf3..a7a54dd10 100644 --- a/crates/jp_config/src/conversation/tool/style.rs +++ b/crates/jp_config/src/conversation/tool/style.rs @@ -28,7 +28,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}, }; @@ -144,6 +144,38 @@ 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, + ), + 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, + ), + error: self + .error + .delta_with_unsets(next.error, &path(prefix, "error"), unsets), + } + } } impl FillDefaults for PartialDisplayStyleConfig { @@ -207,6 +239,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 a07a21b68..69c635bfb 100644 --- a/crates/jp_config/src/conversation/tool_tests.rs +++ b/crates/jp_config/src/conversation/tool_tests.rs @@ -800,18 +800,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 }), @@ -821,7 +824,52 @@ fn test_tools_config() { source: Some(ToolSource::Builtin { tool: None }), ..Default::default() }) - ]) + ])) + ); +} + +/// The tools map takes a strategy, even though its entries are flattened to sit +/// directly under `conversation.tools`. +#[test] +fn tools_map_accepts_a_replace_strategy() { + let config: PartialToolsConfig = toml::from_str( + r#" + strategy = "replace" + + [value.my_tool] + source = "builtin" + "#, + ) + .expect("a strategy-carrying tools map parses"); + + assert!( + matches!(&config.tools, MergeableMap::Merged(merged) + if merged.strategy == Some(crate::types::map::MergedMapStrategy::Replace)), + "expected the declared strategy to survive the flatten: {:?}", + config.tools + ); + assert!(config.tools.contains_key("my_tool")); +} + +/// A plain tools map keeps merging per key, and a tool may be named `value`. +#[test] +fn tools_map_without_a_strategy_merges_per_key() { + let config: PartialToolsConfig = toml::from_str( + r#" + [value] + source = "builtin" + "#, + ) + .expect("a plain tools map parses"); + + assert!( + matches!(&config.tools, MergeableMap::Map(_)), + "expected a plain map: {:?}", + config.tools + ); + assert!( + config.tools.contains_key("value"), + "`value` alone names a tool, since a strategy needs both keys" ); } diff --git a/crates/jp_config/src/delta.rs b/crates/jp_config/src/delta.rs index 08373d541..fcb5c9931 100644 --- a/crates/jp_config/src/delta.rs +++ b/crates/jp_config/src/delta.rs @@ -1,8 +1,12 @@ //! Configuration delta calculation. -use indexmap::IndexMap; use schematic::PartialConfig; +use crate::types::{ + map::{MergeableMap, MergedMap, MergedMapStrategy}, + vec::{MergeableVec, MergedVec, MergedVecStrategy}, +}; + /// Calculate the delta between two partial configurations. /// /// It takes `self`, and should check for any value in `next` that differs from @@ -40,6 +44,18 @@ pub trait PartialConfigDelta: PartialConfig { } } +/// Fields a delta never stores. +/// +/// Each is read while the config file declaring it is loaded, and only its +/// effect outlives that: `extends` has already been merged in by the time a +/// partial exists, `inherit` has already stopped the merge chain, and `loader` +/// steered how its own entry was loaded ([RFD 038]). +/// Carrying any of them into a conversation would re-apply a decision that was +/// made once, so [`PartialConfigDelta::delta`] zeroes all three. +/// +/// [RFD 038]: https://jp.computer/rfd/038 +pub const LOAD_TIME_ONLY: &[&str] = &["extends", "inherit", "loader"]; + /// Join a field name onto its parent's dotted path. #[must_use] pub fn path(prefix: &str, name: &str) -> String { @@ -50,32 +66,94 @@ pub fn path(prefix: &str, name: &str) -> String { } } -/// Delta for an appending list, reporting when appending cannot reach `next`. +/// Calculate the delta between two strategy-carrying lists. /// -/// Returns the elements `next` adds while appending suffices. -/// Otherwise pushes `path` to `unsets` and returns the whole of `next`, which -/// is what the caller merges after clearing the field. -pub fn delta_opt_vec_at( - path: &str, - prev: Option<&Vec>, - next: Option>, - unsets: &mut Vec, -) -> Option> { +/// Appending reaches `next` exactly when `next` starts with `prev`, and the +/// delta is then the tail, carried as a plain list so the fold appends it. +/// Every other difference — an element removed, reordered, or inserted before +/// the last one — carries the whole of `next` with `replace`. +/// +/// Order is part of the answer, not a detail. +/// A delta that reproduced the set of elements while appending them in a +/// different order changes the meaning of any list whose order matters, and +/// says nothing at all about a list that only lost an element. +/// +/// A [`MergeableVec`] can express `replace` on the wire, which is why this +/// needs no separate path report. +/// A plain `Vec` cannot; see [`delta_opt_vec_at`]. +pub fn delta_mergeable_vec( + prev: &MergeableVec, + next: MergeableVec, +) -> MergeableVec { + if next.starts_with(prev) { + return next.iter().skip(prev.len()).cloned().collect(); + } + + MergeableVec::Merged(MergedVec { + value: next.into_vec(), + strategy: Some(MergedVecStrategy::Replace), + dedup: None, + discard_when_merged: false, + }) +} + +/// Calculate the delta between two strategy-carrying maps. +/// +/// An entry only `next` has is carried whole, an entry both maps have carries +/// its own delta, and an entry whose delta is empty is left out: a missing +/// entry already means "unchanged", so carrying an empty one reads as a change +/// that isn't there. +/// +/// A key `prev` has and `next` does not was dropped, which entries cannot +/// spell. +/// The whole map is then carried with `replace`, since a deep merge would +/// resurrect the dropped key. +pub fn delta_mergeable_map(prev: &MergeableMap, next: MergeableMap) -> MergeableMap +where + T: PartialConfigDelta + PartialEq, +{ + if prev.keys().any(|key| !next.contains_key(key)) { + // Stated rather than inherited from `next`'s shape: a plain map + // deep-merges on the fold and brings the dropped key back. + return MergeableMap::Merged(MergedMap { + value: next.into_map(), + strategy: Some(MergedMapStrategy::Replace), + discard_when_merged: false, + }); + } + + next.into_iter() + .filter_map(|(key, next)| { + let Some(prev) = prev.get(&key) else { + return Some((key, next)); + }; + + if prev == &next { + return None; + } + + let delta = prev.delta(next); + (!delta.is_empty()).then_some((key, delta)) + }) + .collect() +} + +/// Calculate the delta between two optional strategy-carrying lists. +/// +/// Wraps [`delta_mergeable_vec`] for a field whose partial is +/// `Option>`: an absent list on either side is no change, and +/// an empty delta is reported as absent so it does not read as one. +pub fn delta_opt_mergeable_vec( + prev: Option<&MergeableVec>, + next: Option>, +) -> Option> { let next = next?; let Some(prev) = prev else { return Some(next); }; - // Appending reaches `next` exactly when `next` starts with `prev`; the - // delta is then the tail. Anything else — a dropped element, a reorder, an - // insertion in the middle — needs the field cleared first. - if next.starts_with(prev) { - let added = next[prev.len()..].to_vec(); - return (!added.is_empty()).then_some(added); - } - - unsets.push(path.to_owned()); - Some(next) + let delta = delta_mergeable_vec(prev, next); + (!delta.is_empty()).then_some(delta) } /// Delta for an optional nested partial, reporting the fields it cannot reach. @@ -92,42 +170,57 @@ pub fn delta_opt_partial_at( (Some(prev), Some(next)) if prev != &next => { Some(prev.delta_with_unsets(next, path, unsets)) } + // The whole block went away, which merging cannot say. + (Some(_), None) => { + unsets.push(path.to_owned()); + None + } (None, next) => next, _ => None, } } -/// Calculate the delta between two maps, reporting each entry's unsets. +/// Calculate the delta between two strategy-carrying maps of plain values. /// -/// 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( - prefix: &str, - prev: &IndexMap, - next: IndexMap, - unsets: &mut Vec, -) -> IndexMap -where - V: PartialConfigDelta + PartialEq, -{ - next.into_iter() - .filter_map(|(key, next)| { - let Some(prev) = prev.get(&key) else { - return Some((key, next)); - }; +/// Mirrors [`delta_mergeable_map`] for a map whose values carry no partial of +/// their own, so an entry is compared and carried whole rather than diffed. +pub fn delta_mergeable_value_map( + prev: &MergeableMap, + next: MergeableMap, +) -> MergeableMap { + if prev.keys().any(|key| !next.contains_key(key)) { + // Stated rather than inherited from `next`'s shape: a plain map + // deep-merges on the fold and brings the dropped key back. + return MergeableMap::Merged(MergedMap { + value: next.into_map(), + strategy: Some(MergedMapStrategy::Replace), + discard_when_merged: false, + }); + } - if prev == &next { - return None; - } + next.into_iter() + .filter(|(key, next)| !prev.get(key).is_some_and(|prev| prev == next)) + .collect() +} - let mut entry = Vec::new(); - let delta = prev.delta_with_unsets(next, &path(prefix, &key), &mut entry); - let cleared = !entry.is_empty(); - unsets.append(&mut entry); +/// Calculate the delta between two optional values, reporting a cleared field. +/// +/// A value that went away cannot be expressed by merging: schematic keeps the +/// previous value when the next layer has none. +/// The path joins `unsets` so the fold clears the field before merging, and +/// resolution then supplies whatever the field's absence means. +pub fn delta_opt_at( + path: &str, + prev: Option<&T>, + next: Option, + unsets: &mut Vec, +) -> Option { + if prev.is_some() && next.is_none() { + unsets.push(path.to_owned()); + return None; + } - (cleared || !delta.is_empty()).then_some((key, delta)) - }) - .collect() + delta_opt(prev, next) } /// Calculate the delta between two optional values. @@ -151,57 +244,6 @@ pub fn delta_opt_partial( } } -/// Calculate the delta between two optional vectors that merge by appending. -/// -/// The delta holds the elements `next` adds to `prev`, since that is what an -/// appending merge needs to reach `next` from `prev`. -/// -/// Returns `None` when `next` adds nothing. -/// An element dropped from `prev` cannot be expressed by appending, so a -/// removal also yields `None` rather than a delta that fails to remove -/// anything. -/// -/// Use [`delta_opt`] instead for a vector field that merges by replacement: -/// there the whole of `next` is the delta. -pub fn delta_opt_vec(prev: Option<&Vec>, next: Option>) -> Option> { - let next = next?; - let Some(prev) = prev else { - return Some(next); - }; - - let added = delta_vec(prev, next); - (!added.is_empty()).then_some(added) -} - -/// Calculate the delta between two maps of partial configurations. -/// -/// An entry only `next` has is kept whole. -/// 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`. @@ -212,3 +254,7 @@ pub fn delta_vec(prev: &[T], next: Vec) -> Vec { #[cfg(test)] #[path = "delta_tests.rs"] mod tests; + +#[cfg(test)] +#[path = "delta_law_tests.rs"] +mod law_tests; diff --git a/crates/jp_config/src/delta_law_tests.rs b/crates/jp_config/src/delta_law_tests.rs new file mode 100644 index 000000000..24dcd5a67 --- /dev/null +++ b/crates/jp_config/src/delta_law_tests.rs @@ -0,0 +1,297 @@ +//! The delta law, checked per collection strategy. +//! +//! A delta earns its name by reproducing `next` when folded onto `prev`: +//! +//! ```text +//! fold(prev, delta(prev, next)) == next +//! ``` +//! +//! A collection carries its own merge strategy, so it can always satisfy the +//! law: where appending cannot reach `next`, the delta says `replace` and +//! carries the whole value. +//! These tests hold each collection to that, across every way one resolved +//! snapshot can differ from another. + +use schematic::PartialConfig as _; +use test_log::test; + +use crate::{ + PartialAppConfig, + assignment::{AssignKeyValue as _, KvAssignment}, + conversation::tool::access::{PartialAccessConfig, PartialEnvRuleConfig}, + delta::PartialConfigDelta as _, + types::vec::{MergeableVec, MergedVec, MergedVecStrategy}, +}; + +/// One environment-variable rule, named and granting read. +fn rule(name: &str) -> PartialEnvRuleConfig { + PartialEnvRuleConfig { + name: Some(name.to_owned()), + read: Some(true), + } +} + +/// An access block whose `env` rules are a resolved snapshot. +/// +/// `ToPartial` stamps `replace` onto a resolved list, so this is the shape both +/// sides of a delta actually arrive in. +fn snapshot(names: &[&str]) -> PartialAccessConfig { + PartialAccessConfig { + fs: MergeableVec::default(), + env: MergeableVec::Merged(MergedVec { + value: names.iter().map(|name| rule(name)).collect(), + strategy: Some(MergedVecStrategy::Replace), + dedup: None, + discard_when_merged: false, + }), + } +} + +/// The rule names of an access block, in order. +fn names(access: &PartialAccessConfig) -> Vec { + access + .env + .iter() + .filter_map(|rule| rule.name.clone()) + .collect() +} + +/// Assert that the delta between two snapshots folds back to `next`. +/// +/// Order is part of the assertion: rules of equal specificity break toward the +/// one declared last, so a delta that reproduces the set but not the sequence +/// silently inverts precedence. +fn assert_law(before: &[&str], after: &[&str]) { + let prev = snapshot(before); + let next = snapshot(after); + + let delta = prev.delta(next.clone()); + + let mut folded = prev; + folded + .merge(&(), delta) + .expect("folding a delta cannot fail"); + + assert_eq!( + names(&folded), + names(&next), + "{before:?} -> {after:?} did not fold back to the new value" + ); +} + +/// Fields whose clear is known not to survive a fold, and why. +/// +/// `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. +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", +]; + +/// Set `path` to whichever of a few generic values it accepts. +/// +/// A field has to hold something before clearing it proves anything, and there +/// is no generic way to ask a field for a value it would accept. +/// Trying a handful and keeping the first that parses reaches scalars and +/// collections alike; a path that accepts none of them stays as the fixture +/// left it. +fn populate(partial: &PartialAppConfig, path: &str) -> Option { + for value in ["1", "true", "x", "[]", "{}"] { + let Ok(kv) = KvAssignment::try_from_cli(path, value) else { + continue; + }; + + let mut candidate = partial.clone(); + if candidate.assign(kv).is_ok() { + return Some(candidate); + } + } + + None +} + +/// A config with as many fields set as the sweep can arrange. +/// +/// A population that leaves the config unresolvable is dropped rather than +/// carried, so the fixture the sweep starts from is always valid. +/// The result is round-tripped through a resolved config, since that is the +/// shape both sides of a real delta arrive in. +fn populated_fixture() -> PartialAppConfig { + let mut partial = crate::AppConfig::new_test().to_partial(); + + for path in crate::AppConfig::fields() { + let Some(candidate) = populate(&partial, &path) else { + continue; + }; + + if crate::util::build(candidate.clone()).is_ok() { + partial = candidate; + } + } + + crate::util::build(partial) + .expect("the populated fixture resolves") + .to_partial() +} + +/// A field that went away reports its path, since merging cannot say it. +#[test] +fn a_cleared_scalar_reports_its_path() { + let mut prev = PartialAppConfig::empty(); + prev.assistant.name = Some("Bot".to_owned()); + + let mut unsets = Vec::new(); + let delta = prev.delta_with_unsets(PartialAppConfig::empty(), "", &mut unsets); + + assert_eq!(unsets, ["assistant.name"]); + assert_eq!( + delta.assistant.name, None, + "the value is not carried; the clear is the whole change" + ); +} + +/// Every field, asked whether clearing it survives a fold. +/// +/// Shaped like the producer: a `--cfg foo=null` clears the field from the +/// partial, the invocation resolves it, and the delta is taken between two +/// resolved configs. +/// A field with a `#[setting(default)]` therefore comes back holding that +/// default rather than arriving cleared, and only a field whose resolved type +/// is `Option` reaches the delta as an absence. +/// +/// The law is checked on the resolved configs, since that is what a later turn +/// runs with. +/// +/// Paths the fixture leaves unset cannot change when cleared, so they prove +/// nothing; the count is reported so the test says how much it actually +/// covered. +#[test] +fn clearing_any_field_survives_a_fold() { + let prev = populated_fixture(); + + let mut vacuous = Vec::new(); + let mut lost = Vec::new(); + + for path in crate::AppConfig::fields() { + let mut next = prev.clone(); + if next.unset(&path).is_err() { + continue; + } + + // A load-time field is never carried by a delta at all, so clearing it + // has nothing to survive. + if crate::delta::LOAD_TIME_ONLY + .iter() + .any(|field| path == *field || path.starts_with(&format!("{field}."))) + { + continue; + } + + if next == prev { + vacuous.push(path); + continue; + } + + // A clear that leaves the config invalid is not a case the producer has + // to reproduce; the invocation that typed it fails instead. + let Ok(expected) = crate::util::build(next) else { + continue; + }; + + // The producer diffs two *resolved* configs, so `next` arrives through + // this round trip. A field with a default comes back holding it, which + // is why only a field whose resolved type is optional can arrive + // cleared. + let next = expected.to_partial(); + + let mut unsets = Vec::new(); + let delta = prev.delta_with_unsets(next, "", &mut unsets); + + let mut folded = prev.clone(); + for cleared in &unsets { + folded.unset(cleared).expect("a reported path is a field"); + } + folded.merge(&(), delta).expect("folding cannot fail"); + + if crate::util::build(folded).ok().as_ref() != Some(&expected) + && !CLEAR_NOT_RECORDED.contains(&path.as_str()) + { + lost.push(path); + } + } + + assert!( + lost.is_empty(), + "clearing these fields does not survive a fold: {lost:#?}" + ); + + // Reported rather than asserted on: the fixture is what it is, and a path it + // leaves unset cannot change when cleared. Shrinking this list is how the + // sweep's reach grows. + eprintln!( + "{} of {} paths were already unset in the fixture and proved nothing", + vacuous.len(), + crate::AppConfig::fields().len(), + ); +} + +#[test] +fn law_holds_for_an_unchanged_list() { + assert_law(&["A"], &["A"]); +} + +#[test] +fn law_holds_for_an_appended_rule() { + assert_law(&["A"], &["A", "B"]); +} + +#[test] +fn law_holds_for_a_prepended_rule() { + assert_law(&["A"], &["B", "A"]); +} + +#[test] +fn law_holds_for_a_rule_inserted_in_the_middle() { + assert_law(&["A", "C"], &["A", "B", "C"]); +} + +#[test] +fn law_holds_for_a_removed_rule() { + assert_law(&["A", "B"], &["A"]); +} + +#[test] +fn law_holds_for_a_reordered_list() { + assert_law(&["A", "B"], &["B", "A"]); +} + +#[test] +fn law_holds_for_a_wholly_replaced_list() { + assert_law(&["A"], &["B"]); +} + +#[test] +fn law_holds_for_a_cleared_list() { + assert_law(&["A"], &[]); +} + +#[test] +fn law_holds_for_a_first_rule() { + assert_law(&[], &["A"]); +} diff --git a/crates/jp_config/src/delta_tests.rs b/crates/jp_config/src/delta_tests.rs index 3e916eac8..a12f50080 100644 --- a/crates/jp_config/src/delta_tests.rs +++ b/crates/jp_config/src/delta_tests.rs @@ -2,68 +2,139 @@ use indexmap::IndexMap; use test_log::test; use super::*; -use crate::providers::mcp::{PartialMcpProviderConfig, PartialStdioConfig}; +use crate::{ + providers::mcp::{PartialMcpProviderConfig, PartialStdioConfig}, + types::{ + map::{MergeableMap, MergedMapStrategy}, + vec::{MergeableVec, MergedVec, MergedVecStrategy}, + }, +}; /// A server entry with `arguments` set and every other field unset. fn server(arguments: &[&str]) -> PartialMcpProviderConfig { PartialMcpProviderConfig::Stdio(PartialStdioConfig { command: Some("serve".into()), - arguments: Some(arguments.iter().map(|a| (*a).to_owned()).collect()), + arguments: Some( + arguments + .iter() + .map(|a| (*a).to_owned()) + .collect::>() + .into(), + ), ..PartialStdioConfig::default() }) } /// A one-server map, keyed as `kagi`. -fn map(arguments: &[&str]) -> IndexMap { +fn map(arguments: &[&str]) -> MergeableMap { let mut map = IndexMap::new(); map.insert("kagi".to_owned(), server(arguments)); - map + map.into() +} + +/// A removed 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. fn arguments(entry: &PartialMcpProviderConfig) -> Option<&Vec> { let PartialMcpProviderConfig::Stdio(config) = entry; - config.arguments.as_ref() + config.arguments.as_deref() +} + +/// A list the fold appends, holding `values`. +fn appended(values: &[&str]) -> MergeableVec { + values.iter().map(|v| (*v).to_owned()).collect() +} + +/// A list the fold replaces, holding `values`. +fn replaced(values: &[&str]) -> MergeableVec { + MergeableVec::Merged(MergedVec { + value: values.iter().map(|v| (*v).to_owned()).collect(), + strategy: Some(MergedVecStrategy::Replace), + dedup: None, + discard_when_merged: false, + }) } #[test] -fn vec_delta_holds_the_added_elements() { - let prev = vec!["--a".to_owned()]; - let next = vec!["--a".to_owned(), "--b".to_owned()]; +fn vec_delta_appends_the_added_elements() { + let prev = MergeableVec::from(vec!["--a".to_owned()]); assert_eq!( - delta_opt_vec(Some(&prev), Some(next)), - Some(vec!["--b".to_owned()]) + delta_opt_mergeable_vec(Some(&prev), Some(appended(&["--a", "--b"]))), + Some(appended(&["--b"])) ); } -/// The first element added to an empty vector is still an addition. +/// The first element added to an empty list is still an addition. #[test] -fn vec_delta_holds_the_first_added_element() { - let prev = vec![]; - let next = vec!["--a".to_owned()]; +fn vec_delta_appends_the_first_added_element() { + let prev = MergeableVec::from(Vec::::new()); assert_eq!( - delta_opt_vec(Some(&prev), Some(next)), - Some(vec!["--a".to_owned()]) + delta_opt_mergeable_vec(Some(&prev), Some(appended(&["--a"]))), + Some(appended(&["--a"])) ); } #[test] fn unchanged_vec_has_no_delta() { - let prev = vec!["--a".to_owned()]; - let next = vec!["--a".to_owned()]; + let prev = MergeableVec::from(vec!["--a".to_owned()]); - assert_eq!(delta_opt_vec(Some(&prev), Some(next)), None); + assert_eq!( + delta_opt_mergeable_vec(Some(&prev), Some(appended(&["--a"]))), + None + ); } -/// Appending cannot take an element away, so a removal has no delta to record. +/// Appending cannot take an element away, so a removal replaces the list. #[test] -fn removed_vec_element_has_no_delta() { - let prev = vec!["--a".to_owned(), "--b".to_owned()]; - let next = vec!["--a".to_owned()]; +fn removed_vec_element_replaces_the_list() { + let prev = MergeableVec::from(vec!["--a".to_owned(), "--b".to_owned()]); - assert_eq!(delta_opt_vec(Some(&prev), Some(next)), None); + assert_eq!( + delta_opt_mergeable_vec(Some(&prev), Some(appended(&["--a"]))), + Some(replaced(&["--a"])) + ); +} + +/// Order is part of the value, so a reorder replaces the list too. +#[test] +fn reordered_vec_replaces_the_list() { + let prev = MergeableVec::from(vec!["--a".to_owned(), "--b".to_owned()]); + + assert_eq!( + delta_opt_mergeable_vec(Some(&prev), Some(appended(&["--b", "--a"]))), + Some(replaced(&["--b", "--a"])) + ); } /// A one-server config, keyed as `kagi`. @@ -92,35 +163,35 @@ fn an_appended_argument_reports_no_path() { ); } -/// A change appending cannot reach reports its path and carries the whole list. +/// A change appending cannot reach carries the whole list with `replace`. /// -/// The path is what the fold clears, which is what lets the list that follows -/// land verbatim instead of being appended to the one already there. +/// No path is reported: the field states the strategy itself, so the fold has +/// nothing to clear first. #[test] -fn a_dropped_argument_reports_its_path_and_carries_the_whole_list() { +fn a_dropped_argument_is_recorded_as_a_replacement() { let prev = config_with_server(&["--a", "--b"]); let next = config_with_server(&["--a"]); let mut unsets = Vec::new(); let delta = prev.delta_with_unsets(next, "", &mut unsets); - assert_eq!(unsets, ["providers.mcp.kagi.arguments"]); + assert!(unsets.is_empty(), "nothing to clear: {unsets:?}"); assert_eq!( arguments(&delta.providers.mcp["kagi"]), Some(&vec!["--a".to_owned()]) ); } -/// Reordering is not an extension either, so it clears too. +/// Reordering is not an extension either, so it replaces too. #[test] -fn a_reordered_argument_list_reports_its_path() { +fn a_reordered_argument_list_is_recorded_as_a_replacement() { let prev = config_with_server(&["--a", "--b"]); let next = config_with_server(&["--b", "--a"]); let mut unsets = Vec::new(); let delta = prev.delta_with_unsets(next, "", &mut unsets); - assert_eq!(unsets, ["providers.mcp.kagi.arguments"]); + assert!(unsets.is_empty(), "nothing to clear: {unsets:?}"); assert_eq!( arguments(&delta.providers.mcp["kagi"]), Some(&vec!["--b".to_owned(), "--a".to_owned()]) @@ -129,7 +200,9 @@ fn a_reordered_argument_list_reports_its_path() { /// The report reaches a field nested several levels below the root. #[test] -fn a_dropped_beta_header_reports_its_full_path() { +fn a_dropped_beta_header_is_recorded_as_a_replacement() { + use crate::types::vec::{MergedVec, MergedVecStrategy}; + let headers = |values: &[&str]| { let mut partial = crate::PartialAppConfig::empty(); partial.providers.llm.anthropic.beta_headers = @@ -143,17 +216,30 @@ fn a_dropped_beta_header_reports_its_full_path() { let mut unsets = Vec::new(); let delta = prev.delta_with_unsets(next, "", &mut unsets); - assert_eq!(unsets, ["providers.llm.anthropic.beta_headers"]); + assert!( + unsets.is_empty(), + "the field says `replace` itself, so no path needs reporting: {unsets:?}" + ); assert_eq!( delta.providers.llm.anthropic.beta_headers, - Some(vec!["one".to_owned()]) + Some(MergeableVec::Merged(MergedVec { + value: vec!["one".to_owned()], + strategy: Some(MergedVecStrategy::Replace), + dedup: None, + discard_when_merged: false, + })) ); } -/// `stop_words` is reached through four separate paths; each reports its own. +/// A dropped stop word is recorded wherever the parameters are reached from. +/// +/// The list carries its own strategy, so each site records a replacement and +/// none needs a path reported. #[test] -fn a_dropped_stop_word_reports_the_path_it_was_reached_by() { - let words = |values: &[&str]| Some(values.iter().map(|v| (*v).to_owned()).collect::>()); +fn a_dropped_stop_word_is_recorded_at_every_site() { + let words = |values: &[&str]| -> Option> { + Some(values.iter().map(|v| (*v).to_owned()).collect()) + }; let mut prev = crate::PartialAppConfig::empty(); prev.assistant.model.parameters.stop_words = words(&["halt", "stop"]); @@ -174,23 +260,38 @@ fn a_dropped_stop_word_reports_the_path_it_was_reached_by() { let mut unsets = Vec::new(); let delta = prev.delta_with_unsets(next, "", &mut unsets); - unsets.sort(); - assert_eq!(unsets, [ - "assistant.model.parameters.stop_words", - "style.reasoning.summary_model.parameters.stop_words", - ]); + let replaced_with = |values: &[&str]| { + Some(MergeableVec::Merged(MergedVec { + value: values.iter().map(|v| (*v).to_owned()).collect(), + strategy: Some(MergedVecStrategy::Replace), + dedup: None, + discard_when_merged: false, + })) + }; + + assert!(unsets.is_empty(), "nothing to clear: {unsets:?}"); assert_eq!( delta.assistant.model.parameters.stop_words, - Some(vec!["halt".to_owned()]) + replaced_with(&["halt"]) + ); + assert_eq!( + delta + .style + .reasoning + .summary_model + .as_ref() + .map(|model| model.parameters.stop_words.clone()), + Some(replaced_with(&["halt"])), + "the second site records its own replacement" ); } #[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] @@ -198,20 +299,36 @@ 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()])); } -/// An entry that differs but has no expressible delta is left out entirely. +/// An entry whose delta carries nothing is left out entirely. /// /// Keeping it would hand the caller a map with one entry holding nothing, which /// reads as a change to every emptiness check upstream. +/// A stdio entry no longer reaches that state through its `arguments`, which +/// can now say `replace`, so the case is built directly. #[test] fn map_delta_drops_an_entry_whose_delta_is_empty() { - let prev = map(&["--a", "--b"]); - let next = map(&["--a"]); + let entry = |command: &str| -> MergeableMap { + let mut map = IndexMap::new(); + map.insert( + "kagi".to_owned(), + PartialMcpProviderConfig::Stdio(PartialStdioConfig { + command: Some(command.into()), + ..PartialStdioConfig::default() + }), + ); + map.into() + }; - assert!(delta_map(&prev, next).is_empty()); + // Equal entries are dropped by the equality check ahead of the delta. + assert!(delta_mergeable_map(&entry("serve"), entry("serve")).is_empty()); + + // A differing entry contributes only what changed. + let delta = delta_mergeable_map(&entry("serve"), entry("other")); + assert_eq!(delta.len(), 1); } diff --git a/crates/jp_config/src/editor.rs b/crates/jp_config/src/editor.rs index 23a41074d..e6c790450 100644 --- a/crates/jp_config/src/editor.rs +++ b/crates/jp_config/src/editor.rs @@ -11,11 +11,16 @@ use crate::types::command::shell_command_line; use crate::{ assignment::{AssignKeyValue, AssignResult, KvAssignment, missing_key}, delta::{ - PartialConfigDelta, delta_opt, delta_opt_partial, delta_opt_vec, delta_opt_vec_at, path, + PartialConfigDelta, delta_opt, delta_opt_mergeable_vec, delta_opt_partial, + delta_opt_partial_at, path, }, fill::FillDefaults, + internal::merge::vec_with_strategy, partial::{ToPartial, partial_opt, partial_opt_config}, - types::command::{CommandConfigOrString, PartialCommandConfigOrString}, + types::{ + command::{CommandConfigOrString, PartialCommandConfigOrString}, + vec::MergeableVec, + }, }; /// Editor configuration. @@ -61,8 +66,13 @@ pub struct EditorConfig { /// Values with unbalanced quoting are skipped (the next env var in the list /// is tried). #[setting( - default = vec!["JP_EDITOR".into(), "VISUAL".into(), "EDITOR".into()], - merge = schematic::merge::append_vec, + default = MergeableVec::from(vec![ + "JP_EDITOR".to_owned(), + "VISUAL".to_owned(), + "EDITOR".to_owned(), + ]), + partial_via = MergeableVec::, + merge = vec_with_strategy, )] pub envs: Vec, @@ -109,7 +119,7 @@ impl AssignKeyValue for PartialEditorConfig { match kv.key_string().as_str() { "" => kv.try_merge_object(self)?, _ if kv.p("cmd") => self.cmd.assign(kv)?, - _ if kv.p("envs") => kv.try_some_vec_of_strings(&mut self.envs)?, + _ if kv.p("envs") => kv.try_some_mergeable_strings(&mut self.envs)?, _ if kv.p("inline") => self.inline.assign(kv)?, _ => return missing_key(&kv), } @@ -122,15 +132,15 @@ impl PartialConfigDelta for PartialEditorConfig { fn delta(&self, next: Self) -> Self { Self { cmd: delta_opt_partial(self.cmd.as_ref(), next.cmd), - envs: delta_opt_vec(self.envs.as_ref(), next.envs), + envs: delta_opt_mergeable_vec(self.envs.as_ref(), next.envs), inline: self.inline.delta(next.inline), } } fn delta_with_unsets(&self, next: Self, prefix: &str, unsets: &mut Vec) -> Self { Self { - cmd: delta_opt_partial(self.cmd.as_ref(), next.cmd), - envs: delta_opt_vec_at(&path(prefix, "envs"), self.envs.as_ref(), next.envs, unsets), + cmd: delta_opt_partial_at(&path(prefix, "cmd"), self.cmd.as_ref(), next.cmd, unsets), + envs: delta_opt_mergeable_vec(self.envs.as_ref(), next.envs), inline: self.inline.delta(next.inline), } } @@ -152,7 +162,7 @@ impl ToPartial for EditorConfig { Self::Partial { cmd: partial_opt_config(self.cmd.as_ref(), defaults.cmd), - envs: partial_opt(&self.envs, defaults.envs), + envs: partial_opt(&MergeableVec::from(self.envs.clone()), defaults.envs), inline: self.inline.to_partial(), } } diff --git a/crates/jp_config/src/editor_tests.rs b/crates/jp_config/src/editor_tests.rs index a5f397f4a..25f112f6f 100644 --- a/crates/jp_config/src/editor_tests.rs +++ b/crates/jp_config/src/editor_tests.rs @@ -45,37 +45,53 @@ fn test_editor_config_cmd() { #[test] fn test_editor_config_envs() { + let envs = |names: &[&str]| -> Option> { + Some(names.iter().map(|n| (*n).to_owned()).collect()) + }; + let mut p = PartialEditorConfig::default(); let kv = KvAssignment::try_from_cli("envs", "EDITOR,VISUAL").unwrap(); p.assign(kv).unwrap(); - assert_eq!(p.envs, Some(vec!["EDITOR".into(), "VISUAL".into()])); + assert_eq!(p.envs, envs(&["EDITOR", "VISUAL"])); let kv = KvAssignment::try_from_cli("envs:", r#"["EDITOR","VISUAL"]"#).unwrap(); p.assign(kv).unwrap(); - assert_eq!(p.envs, Some(vec!["EDITOR".into(), "VISUAL".into()])); + assert_eq!(p.envs, envs(&["EDITOR", "VISUAL"])); let kv = KvAssignment::try_from_cli("envs.0", "EDIT").unwrap(); p.assign(kv).unwrap(); - assert_eq!(p.envs, Some(vec!["EDIT".into(), "VISUAL".into()])); + assert_eq!(p.envs, envs(&["EDIT", "VISUAL"])); let kv = KvAssignment::try_from_cli("envs+:", r#"["OTHER"]"#).unwrap(); p.assign(kv).unwrap(); - assert_eq!( - p.envs, - Some(vec!["EDIT".into(), "VISUAL".into(), "OTHER".into()]) - ); + assert_eq!(p.envs, envs(&["EDIT", "VISUAL", "OTHER"])); let kv = KvAssignment::try_from_cli("envs+", "LAST").unwrap(); p.assign(kv).unwrap(); + assert_eq!(p.envs, envs(&["EDIT", "VISUAL", "OTHER", "LAST"])); +} + +/// The field accepts a strategy alongside its value, which is what carrying a +/// wrapper in the partial buys. +#[test] +fn envs_accepts_a_declared_strategy() { + use crate::types::vec::{MergedVec, MergedVecStrategy}; + + let mut p = PartialEditorConfig::default(); + + let kv = + KvAssignment::try_from_cli("envs:", r#"{"value":["ONLY"],"strategy":"replace"}"#).unwrap(); + p.assign(kv).unwrap(); + assert_eq!( p.envs, - Some(vec![ - "EDIT".into(), - "VISUAL".into(), - "OTHER".into(), - "LAST".into() - ]) + Some(MergeableVec::Merged(MergedVec { + value: vec!["ONLY".to_owned()], + strategy: Some(MergedVecStrategy::Replace), + dedup: None, + discard_when_merged: false, + })) ); } diff --git a/crates/jp_config/src/internal/merge.rs b/crates/jp_config/src/internal/merge.rs index 1791664c6..90a604b68 100644 --- a/crates/jp_config/src/internal/merge.rs +++ b/crates/jp_config/src/internal/merge.rs @@ -1,11 +1,9 @@ //! Internal merge strategies. mod map; -mod plain_vec; mod string; mod vec; pub use map::map_with_strategy; -pub use plain_vec::append_vec_dedup; pub use string::string_with_strategy; -pub use vec::vec_with_strategy; +pub use vec::{ordered_vec_with_strategy, vec_with_strategy}; diff --git a/crates/jp_config/src/internal/merge/plain_vec.rs b/crates/jp_config/src/internal/merge/plain_vec.rs deleted file mode 100644 index c62441246..000000000 --- a/crates/jp_config/src/internal/merge/plain_vec.rs +++ /dev/null @@ -1,47 +0,0 @@ -//! Merge strategies for plain `Vec` fields. -//! -//! These operate on `Vec` directly, unlike [`vec_with_strategy`], which -//! reads its strategy from a [`MergeableVec`] wrapper. -//! -//! [`MergeableVec`]: crate::types::vec::MergeableVec -//! [`vec_with_strategy`]: super::vec_with_strategy - -use schematic::MergeResult; - -/// Append `next` to `prev`, dropping items already present. -/// -/// Comparison uses `PartialEq` and the first occurrence wins, so the result -/// keeps `prev`'s order with `next`'s new items appended. -/// -/// Only combining merges reach this function: schematic's `merge_setting` -/// invokes a merge strategy only when both layers supply a value, so a list -/// supplied by a single layer is stored as written, duplicates included. -/// That is the same rule `replace` follows on [`MergeableVec`] — repeated -/// items within one source are the author's own data, not something a merge of -/// two sources should rewrite. -/// -/// Deduplicating here rather than through a `transform` is deliberate: -/// transforms run in [`PartialConfig::finalize`], which JP's config pipeline -/// never calls — it merges layers with `load_partial` and resolves them with -/// `AppConfig::from_partial_with_defaults`. -/// -/// [`MergeableVec`]: crate::types::vec::MergeableVec -/// [`PartialConfig::finalize`]: schematic::PartialConfig::finalize -#[expect(clippy::unnecessary_wraps)] -pub fn append_vec_dedup( - mut prev: Vec, - next: Vec, - _: &C, -) -> MergeResult> { - for item in next { - if !prev.contains(&item) { - prev.push(item); - } - } - - Ok(Some(prev)) -} - -#[cfg(test)] -#[path = "plain_vec_tests.rs"] -mod tests; diff --git a/crates/jp_config/src/internal/merge/plain_vec_tests.rs b/crates/jp_config/src/internal/merge/plain_vec_tests.rs deleted file mode 100644 index 478ab2aac..000000000 --- a/crates/jp_config/src/internal/merge/plain_vec_tests.rs +++ /dev/null @@ -1,43 +0,0 @@ -use test_log::test; - -use super::*; - -#[test] -fn appends_new_items() { - let result = append_vec_dedup(vec![1, 2], vec![3, 4], &()) - .unwrap() - .unwrap(); - - assert_eq!(result, vec![1, 2, 3, 4]); -} - -#[test] -fn drops_items_already_present() { - // Two config layers naming the same directory contribute it once, which is - // what `config_load_paths` and `beta_headers` need: the resolved list is - // searched (respectively sent) in order, and a repeat is pure noise. - let result = append_vec_dedup(vec!["a", "b"], vec!["b", "c"], &()) - .unwrap() - .unwrap(); - - assert_eq!(result, vec!["a", "b", "c"]); -} - -#[test] -fn keeps_first_occurrence_order() { - let result = append_vec_dedup(vec![3, 1], vec![2, 1, 3], &()) - .unwrap() - .unwrap(); - - assert_eq!(result, vec![3, 1, 2]); -} - -#[test] -fn collapses_repeats_inside_the_incoming_layer() { - // Only reachable when two layers combine — a list supplied by a single - // layer never reaches this function, so its own repeats are kept. See - // `test_load_partial_at_path_keeps_repeats_from_a_single_file`. - let result = append_vec_dedup(vec![1], vec![2, 2], &()).unwrap().unwrap(); - - assert_eq!(result, vec![1, 2]); -} diff --git a/crates/jp_config/src/internal/merge/vec.rs b/crates/jp_config/src/internal/merge/vec.rs index ddb7bc9ab..2e1d8810c 100644 --- a/crates/jp_config/src/internal/merge/vec.rs +++ b/crates/jp_config/src/internal/merge/vec.rs @@ -91,6 +91,33 @@ where })) } +/// Merge two lists whose repetition is significant. +/// +/// Identical to [`vec_with_strategy`] except that duplicates survive unless a +/// config explicitly asks for deduplication, rather than the other way round. +/// +/// An argument list is a command line: `["--flag", "x", "--flag", "y"]` means +/// something different once the second `--flag` is dropped. +/// Stating the opinion here rather than on the field's default is what makes it +/// hold during config layering, which merges partials before any defaults are +/// filled in. +pub fn ordered_vec_with_strategy( + prev: MergeableVec, + next: MergeableVec, + context: &(), +) -> MergeResult> +where + T: Clone + PartialEq + Serialize + DeserializeOwned + Schematic, +{ + let next = if dedup_flag(&prev).is_none() && dedup_flag(&next).is_none() { + with_dedup_flag(next, Some(false)) + } else { + next + }; + + vec_with_strategy(prev, next, context) +} + /// Extract the explicit dedup flag from a `MergeableVec`. const fn dedup_flag(v: &MergeableVec) -> Option { match v { diff --git a/crates/jp_config/src/lib.rs b/crates/jp_config/src/lib.rs index f787a5ba8..747394571 100644 --- a/crates/jp_config/src/lib.rs +++ b/crates/jp_config/src/lib.rs @@ -56,7 +56,7 @@ pub(crate) mod validate; use std::sync::Arc; -pub use delta::PartialConfigDelta; +pub use delta::{LOAD_TIME_ONLY, PartialConfigDelta}; pub use error::Error; pub use fill::FillDefaults; use indexmap::IndexMap; @@ -73,7 +73,7 @@ use crate::{ assignment::{AssignKeyValue, AssignResult, KvAssignment, missing_key, type_error}, assistant::{AssistantConfig, PartialAssistantConfig}, conversation::{ConversationConfig, PartialConversationConfig}, - delta::{delta_opt_vec, delta_opt_vec_at, path as delta_path}, + delta::{delta_opt_mergeable_vec, path as delta_path}, editor::{EditorConfig, PartialEditorConfig}, interrupt::{InterruptConfig, PartialInterruptConfig}, loader::{LoaderConfig, PartialLoaderConfig}, @@ -82,7 +82,7 @@ use crate::{ providers::{PartialProviderConfig, ProviderConfig}, style::{PartialStyleConfig, StyleConfig}, template::{PartialTemplateConfig, TemplateConfig}, - types::extending_path::ExtendingRelativePath, + types::{extending_path::ExtendingRelativePath, vec::MergeableVec}, user::{PartialUserConfig, UserConfig}, }; @@ -127,7 +127,10 @@ pub struct AppConfig { /// /// For example, to load `.jp/agents/dev.toml`, add `.jp/agents` to this /// list and run `jp query --cfg dev`. - #[setting(merge = internal::merge::append_vec_dedup)] + #[setting( + partial_via = MergeableVec::, + merge = internal::merge::vec_with_strategy, + )] pub config_load_paths: Vec, /// Extends the configuration from the given files. @@ -234,7 +237,7 @@ impl AssignKeyValue for PartialAppConfig { _ => type_error(kv.key(), &kv.value, &["string"]).map_err(Into::into), }; - kv.try_some_vec(&mut self.config_load_paths, parser)?; + kv.try_some_mergeable_vec(&mut self.config_load_paths, parser)?; } _ if kv.p("assistant") => self.assistant.assign(kv)?, _ if kv.p("conversation") => self.conversation.assign(kv)?, @@ -255,23 +258,12 @@ impl AssignKeyValue for PartialAppConfig { impl PartialConfigDelta for PartialAppConfig { fn delta(&self, next: Self) -> Self { Self { - // Any `extends` paths are interpreted at runtime, so we don't need to - // store this information again, since the extended configuration is - // already merged into the current one. + // See `delta::LOAD_TIME_ONLY` for why these three are dropped. extends: None, - - // Any `inherit` value is interpreted at runtime, so we don't need to - // store this information again, since the config load logic will - // already have stopped the merge process when it encounters an - // `inherit` value of `true`. inherit: None, - - // Loader metadata is interpreted while the declaring file is - // loaded ([RFD 038]): only its *effect* outlives loading, never - // the field itself. loader: PartialLoaderConfig::default(), - config_load_paths: delta_opt_vec( + config_load_paths: delta_opt_mergeable_vec( self.config_load_paths.as_ref(), next.config_load_paths, ), @@ -290,15 +282,14 @@ impl PartialConfigDelta for PartialAppConfig { fn delta_with_unsets(&self, next: Self, prefix: &str, unsets: &mut Vec) -> Self { Self { + // See `delta::LOAD_TIME_ONLY`. extends: None, inherit: None, loader: PartialLoaderConfig::default(), - config_load_paths: delta_opt_vec_at( - &delta_path(prefix, "config_load_paths"), + config_load_paths: delta_opt_mergeable_vec( self.config_load_paths.as_ref(), next.config_load_paths, - unsets, ), assistant: self.assistant.delta_with_unsets( @@ -320,14 +311,24 @@ 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), - user: self.user.delta(next.user), + 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), } } } @@ -358,7 +359,10 @@ impl ToPartial for AppConfig { let mut partial = Self::Partial { inherit: partial_opt(&self.inherit, defaults.inherit), - config_load_paths: partial_opt(&self.config_load_paths, defaults.config_load_paths), + config_load_paths: partial_opt( + &MergeableVec::from(self.config_load_paths.clone()), + defaults.config_load_paths, + ), extends: partial_opt(&self.extends, defaults.extends), loader: self.loader.to_partial(), assistant: self.assistant.to_partial(), diff --git a/crates/jp_config/src/lib_tests.rs b/crates/jp_config/src/lib_tests.rs index 159927fd0..d09a01057 100644 --- a/crates/jp_config/src/lib_tests.rs +++ b/crates/jp_config/src/lib_tests.rs @@ -527,16 +527,15 @@ fn an_explicit_inquiry_value_survives_a_partial_round_trip() { ); } -/// An MCP server whose only difference cannot be expressed as a delta does not -/// produce one. +/// A dropped MCP argument is recorded, rather than producing an event holding +/// nothing but the server's transport tag on every turn. /// -/// `arguments` merges by appending, so a dropped argument has no delta to -/// record. -/// Keeping the server in the map anyway makes the whole partial look non-empty, -/// and every turn then writes a `config_delta` event holding nothing but the -/// server's transport tag. +/// `arguments` carries its own merge strategy, so the delta says `replace` and +/// the fold reaches the shorter list. +/// Before it could, appending was unable to express the removal, the difference +/// went unrecorded, and the next turn computed the same non-delta again. #[test] -fn an_mcp_server_with_no_expressible_change_yields_no_delta() { +fn a_dropped_mcp_argument_is_recorded() { use crate::providers::mcp::{McpProviderConfig, StdioConfig}; let server = |arguments: &[&str]| { @@ -562,12 +561,125 @@ fn an_mcp_server_with_no_expressible_change_yields_no_delta() { let delta = prev.to_partial().delta(next.to_partial()); + let entry = delta + .providers + .mcp + .get("bookworm") + .expect("the change is recorded"); + + let crate::providers::mcp::PartialMcpProviderConfig::Stdio(stdio) = entry; + assert_eq!( + stdio.arguments.as_deref(), + Some(&vec!["serve".to_owned()]), + "the delta carries the whole list, since appending cannot shorten one" + ); +} + +/// A server the user removed is recorded, 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. +#[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!( + unsets.is_empty(), + "the map states its own strategy, so no path is reported: {unsets:?}" + ); + assert!( + delta.providers.mcp.discard_when_merged() || !delta.providers.mcp.is_empty(), + "the delta carries the map the user is left with" + ); + + 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!( - delta.providers.mcp.is_empty(), - "expected no server entry, got: {:?}", - delta.providers.mcp + 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" ); - assert!(delta.is_empty(), "expected an empty delta, got: {delta:?}"); } /// A union that names an expanded form contributes both the shorthand path and @@ -689,7 +801,10 @@ fn test_partial_app_config_assign() { let kv = KvAssignment::try_from_cli("config_load_paths", "foo,bar").unwrap(); p.assign(kv).unwrap(); - assert_eq!(p.config_load_paths, Some(vec!["foo".into(), "bar".into()])); + assert_eq!( + p.config_load_paths, + Some(vec![RelativePathBuf::from("foo"), "bar".into()].into()) + ); let kv = KvAssignment::try_from_cli("assistant.name", "foo").unwrap(); p.assign(kv).unwrap(); @@ -722,10 +837,12 @@ fn config_load_paths_append_across_layers() { // matters downstream: `--cfg ` resolution walks the list and takes // the first directory that holds a matching file. let mut base = PartialAppConfig::empty(); - base.config_load_paths = Some(vec![".jp/global".into(), ".jp/shared".into()]); + base.config_load_paths = + Some(vec![RelativePathBuf::from(".jp/global"), ".jp/shared".into()].into()); let mut overlay = PartialAppConfig::empty(); - overlay.config_load_paths = Some(vec![".jp/shared".into(), ".jp/workspace".into()]); + overlay.config_load_paths = + Some(vec![RelativePathBuf::from(".jp/shared"), ".jp/workspace".into()].into()); base.merge(&(), overlay).unwrap(); @@ -734,7 +851,7 @@ fn config_load_paths_append_across_layers() { ".jp/shared".into(), ".jp/workspace".into(), ]; - assert_eq!(base.config_load_paths, Some(want)); + assert_eq!(base.config_load_paths, Some(want.into())); } #[test] diff --git a/crates/jp_config/src/model.rs b/crates/jp_config/src/model.rs index 28451a140..b3ae8b75a 100644 --- a/crates/jp_config/src/model.rs +++ b/crates/jp_config/src/model.rs @@ -11,7 +11,9 @@ use crate::{ fill::FillDefaults, model::{ id::{ModelIdOrAliasConfig, PartialModelIdOrAliasConfig}, - parameters::{ParametersConfig, PartialParametersConfig, deserialize_collecting_other}, + parameters::{ + ParametersConfig, PartialParametersConfig, deserialize_hoisting_legacy_other, + }, }, partial::ToPartial, }; @@ -31,9 +33,9 @@ pub struct ModelConfig { /// The model parameters. /// /// Configuration for model parameters such as temperature, max tokens, etc. - /// Parameters JP does not model are collected into `parameters.other` and - /// forwarded to the provider as written. - #[setting(nested, deserialize_with = "deserialize_collecting_other")] + /// Parameters JP does not model are written in the block itself and + /// forwarded to the provider as given. + #[setting(nested, deserialize_with = "deserialize_hoisting_legacy_other")] pub parameters: ParametersConfig, } diff --git a/crates/jp_config/src/model/parameters.rs b/crates/jp_config/src/model/parameters.rs index 97e3b4875..0d71c575a 100644 --- a/crates/jp_config/src/model/parameters.rs +++ b/crates/jp_config/src/model/parameters.rs @@ -10,11 +10,13 @@ use crate::{ BoxedError, assignment::{AssignKeyValue, AssignResult, KvAssignment, missing_key}, delta::{ - PartialConfigDelta, delta_opt, delta_opt_partial, delta_opt_vec, delta_opt_vec_at, path, + PartialConfigDelta, delta_opt, delta_opt_at, delta_opt_mergeable_vec, delta_opt_partial, + delta_opt_partial_at, path, }, fill::{FillDefaults, fill_opt}, + internal::merge::vec_with_strategy, partial::{ToPartial, partial_opt, partial_opt_config, partial_opts}, - types::json_value::JsonValue, + types::{json_value::JsonValue, vec::MergeableVec}, }; /// Assistant-specific configuration. @@ -22,7 +24,7 @@ use crate::{ /// Parameters JP does not model are collected into [`Self::other`], so a /// provider-specific key can be written directly in the parameter block. #[derive(Debug, Clone, PartialEq, Config)] -#[config(default, rename_all = "snake_case")] +#[config(default, rename_all = "snake_case", allow_unknown_fields)] pub struct ParametersConfig { /// Maximum number of tokens to generate. /// @@ -82,60 +84,50 @@ pub struct ParametersConfig { /// The `stop_words` parameter can be set to specific sequences, such as a /// period or specific word, to stop the model from generating text when it /// encounters these sequences. - #[setting(default, merge = schematic::merge::append_vec)] + #[setting( + default, + partial_via = MergeableVec::, + merge = vec_with_strategy, + )] pub stop_words: Vec, - /// Other non-typed parameters that some models might support. + /// Where the parameters JP does not model are collected. /// - /// Any key in the parameter block that JP does not recognize lands here and - /// is forwarded to the provider as written: + /// Not a key to write. + /// A parameter JP does not recognize is written in the block itself and + /// forwarded to the provider as given: /// /// ```toml /// [assistant.model.parameters] /// presence_penalty = 0.5 /// ``` /// - /// The equivalent explicit form is also accepted: - /// - /// ```toml - /// [assistant.model.parameters.other] - /// presence_penalty = 0.5 - /// ``` - #[setting(default, merge = schematic::merge::merge_iter)] + /// Flattened, so the parameters reach the wire under their own names and + /// this field is never a key anyone writes. + /// That is also what keeps them: the compat layer strips whatever the + /// schema does not name, and skips a struct holding a flattened field for + /// exactly this reason. + #[setting(flatten, default, merge = schematic::merge::merge_iter)] pub other: IndexMap, } -/// Every key [`ParametersConfig`] models. -/// Anything else is a provider parameter and is collected into `other`. +/// Deserialize a parameter block, hoisting a legacy `other` table into it. /// -/// Kept in sync with the struct by `known_keys_match_the_schema`. -pub(crate) const KNOWN_KEYS: &[&str] = &[ - "max_tokens", - "reasoning", - "temperature", - "top_p", - "top_k", - "stop_words", - "other", -]; - -/// Deserialize a parameter block, collecting unrecognized keys into `other`. -/// -/// Unrecognized keys are provider parameters JP does not model, so discarding -/// them (what serde does with an unknown field on a lenient container) silently -/// drops user intent. -/// An explicit `other` table is also accepted and merges with the collected -/// keys, the explicit entries winning. +/// A provider parameter is written in the block itself and collected by the +/// flattened [`ParametersConfig::other`]. +/// Config files and stored conversation configs written before that nested them +/// under an explicit `other` table, and left alone those entries would land in +/// a parameter *named* `other` and reach the provider as one. /// /// Applied through `#[setting(deserialize_with = ...)]` on the field holding -/// this config rather than as a `Deserialize` impl, so the generated -/// field-by-field deserializer still does the real work. +/// this config, so the generated field-by-field deserializer still does the +/// collecting. /// /// # Errors /// -/// Returns an error if the block is not a map, if `other` is present but is not -/// a map, or if any modelled field fails to deserialize. -pub(crate) fn deserialize_collecting_other<'de, D>( +/// Returns an error if the block is not a map, or if any field in it fails to +/// deserialize. +pub(crate) fn deserialize_hoisting_legacy_other<'de, D>( deserializer: D, ) -> Result where @@ -143,35 +135,14 @@ where { let mut map = serde_json::Map::::deserialize(deserializer)?; - let mut other = IndexMap::new(); - map.retain(|key, value| { - if KNOWN_KEYS.contains(&key.as_str()) { - return true; - } - - other.insert(key.clone(), JsonValue(value.clone())); - false - }); - - // Merged after the collected keys so an explicit entry wins a collision. - let explicit = map.remove("other"); - let has_explicit = explicit.is_some(); - if let Some(explicit) = explicit { - let explicit: IndexMap = - serde_json::from_value(explicit).map_err(DeError::custom)?; - other.extend(explicit); + // Hoisted after the siblings so a nested entry still wins a collision with + // one of the same name, which is what the nested form did when it was the + // documented spelling. + if let Some(serde_json::Value::Object(legacy)) = map.remove("other") { + map.extend(legacy); } - let mut partial: PartialParametersConfig = - serde_json::from_value(serde_json::Value::Object(map)).map_err(DeError::custom)?; - - // An explicit `other` is kept even when empty, so a serialize/deserialize - // round-trip of a config carrying `other = {}` is lossless. - if has_explicit || !other.is_empty() { - partial.other = Some(other); - } - - Ok(partial) + serde_json::from_value(serde_json::Value::Object(map)).map_err(DeError::custom) } impl AssignKeyValue for PartialParametersConfig { @@ -182,8 +153,13 @@ impl AssignKeyValue for PartialParametersConfig { "temperature" => self.temperature = kv.try_some_f32()?, "top_p" => self.top_p = kv.try_some_f32()?, "top_k" => self.top_k = kv.try_some_u32()?, - _ if kv.p("stop_words") => kv.try_some_vec_of_strings(&mut self.stop_words)?, + _ if kv.p("stop_words") => kv.try_some_mergeable_strings(&mut self.stop_words)?, _ if kv.p("reasoning") => self.reasoning.assign(kv)?, + + // Anything else is a provider parameter JP does not model, named + // as it reaches the provider. `other` holds them but is flattened, + // so it is not a name to trim here: a parameter called `other` + // is addressed like any other. _ => kv.assign_to_entry(self.other.get_or_insert_default())?, } @@ -199,25 +175,50 @@ impl PartialConfigDelta for PartialParametersConfig { temperature: delta_opt(self.temperature.as_ref(), next.temperature), top_p: delta_opt(self.top_p.as_ref(), next.top_p), top_k: delta_opt(self.top_k.as_ref(), next.top_k), - stop_words: delta_opt_vec(self.stop_words.as_ref(), next.stop_words), + stop_words: delta_opt_mergeable_vec(self.stop_words.as_ref(), next.stop_words), other: delta_opt(self.other.as_ref(), next.other), } } fn delta_with_unsets(&self, next: Self, prefix: &str, unsets: &mut Vec) -> Self { Self { - max_tokens: delta_opt(self.max_tokens.as_ref(), next.max_tokens), - reasoning: delta_opt_partial(self.reasoning.as_ref(), next.reasoning), - temperature: delta_opt(self.temperature.as_ref(), next.temperature), - top_p: delta_opt(self.top_p.as_ref(), next.top_p), - top_k: delta_opt(self.top_k.as_ref(), next.top_k), - stop_words: delta_opt_vec_at( - &path(prefix, "stop_words"), - self.stop_words.as_ref(), - next.stop_words, + max_tokens: delta_opt_at( + &path(prefix, "max_tokens"), + self.max_tokens.as_ref(), + next.max_tokens, + unsets, + ), + reasoning: delta_opt_partial_at( + &path(prefix, "reasoning"), + self.reasoning.as_ref(), + next.reasoning, + unsets, + ), + temperature: delta_opt_at( + &path(prefix, "temperature"), + self.temperature.as_ref(), + next.temperature, + unsets, + ), + top_p: delta_opt_at( + &path(prefix, "top_p"), + self.top_p.as_ref(), + next.top_p, + unsets, + ), + top_k: delta_opt_at( + &path(prefix, "top_k"), + self.top_k.as_ref(), + next.top_k, + unsets, + ), + stop_words: delta_opt_mergeable_vec(self.stop_words.as_ref(), next.stop_words), + other: delta_opt_at( + &path(prefix, "other"), + self.other.as_ref(), + next.other, unsets, ), - other: delta_opt(self.other.as_ref(), next.other), } } } @@ -244,7 +245,7 @@ impl ToPartial for ParametersConfig { temperature: partial_opts(self.temperature.as_ref(), None), top_p: partial_opts(self.top_p.as_ref(), None), top_k: partial_opts(self.top_k.as_ref(), None), - stop_words: partial_opt(&self.stop_words, None), + stop_words: partial_opt(&MergeableVec::from(self.stop_words.clone()), None), other: partial_opt(&self.other, None), } } diff --git a/crates/jp_config/src/model/parameters_tests.rs b/crates/jp_config/src/model/parameters_tests.rs index ea9e03a1e..faf8ab074 100644 --- a/crates/jp_config/src/model/parameters_tests.rs +++ b/crates/jp_config/src/model/parameters_tests.rs @@ -23,24 +23,43 @@ fn assign_unknown_nested_key_delegates_to_other() { assert_eq!(other["custom"], JsonValue(json!({"depth": "3"}))); } +/// A provider parameter is cleared by its own name, with no wrapper in the +/// path. #[test] -fn known_keys_match_the_schema() { - use schematic::{SchemaBuilder, SchemaType, Schematic as _}; - - // `deserialize_collecting_other` splits the parameter block using this - // list. A field added to the struct but missed here would be rerouted into - // `other` and forwarded to the provider as a raw parameter instead. - let schema = ParametersConfig::build_schema(SchemaBuilder::default()); - let SchemaType::Struct(struct_type) = &schema.ty else { - panic!("expected a struct schema"); - }; +fn assign_clears_a_collected_parameter() { + let mut p = PartialParametersConfig::default(); + p.assign(KvAssignment::try_from_cli("seed", "42").unwrap()) + .unwrap(); + p.assign(KvAssignment::unset("seed")).unwrap(); + + assert!( + p.other.as_ref().is_none_or(IndexMap::is_empty), + "expected the parameter gone, got: {:?}", + p.other + ); +} - let mut fields: Vec<&str> = struct_type.fields.keys().map(String::as_str).collect(); - let mut known = KNOWN_KEYS.to_vec(); - fields.sort_unstable(); - known.sort_unstable(); +/// The collector is flattened, so a provider parameter is written and read back +/// under its own name with no wrapper key in between. +#[test] +fn other_is_flattened_on_the_wire() { + let mut p = PartialParametersConfig::default(); + p.assign(KvAssignment::try_from_cli("seed", "42").unwrap()) + .unwrap(); - assert_eq!(fields, known); + let json = serde_json::to_value(&p).unwrap(); + assert_eq!( + json.get("seed"), + Some(&json!("42")), + "the parameter sits in the block: {json}" + ); + assert!( + json.get("other").is_none(), + "no wrapper key reaches the wire: {json}" + ); + + let back: PartialParametersConfig = serde_json::from_value(json).unwrap(); + assert_eq!(back.other.as_ref().map(IndexMap::len), Some(1)); } /// Deserialize a `[parameters]` block through the production path: the @@ -73,10 +92,10 @@ fn deserialize_collects_unknown_keys_into_other() { assert_eq!(other.len(), 2, "known keys must not leak into `other`"); } +/// A stored config or user file written before `other` was flattened nested its +/// parameters under it, and those still land as parameters. #[test] -fn deserialize_accepts_an_explicit_other_table() { - // The nested form is what every stored conversation config and existing - // user file writes, so it has to keep working. +fn deserialize_hoists_a_legacy_other_table() { let p = parameters_from_toml(indoc::indoc!( r" temperature = 0.7 @@ -111,11 +130,15 @@ fn deserialize_prefers_the_explicit_other_entry_on_collision() { } #[test] -fn deserialize_leaves_other_unset_when_every_key_is_known() { +fn deserialize_collects_nothing_when_every_key_is_known() { let p = parameters_from_toml("top_k = 40"); assert_eq!(p.top_k, Some(40)); - assert_eq!(p.other, None); + assert!( + p.other.as_ref().is_none_or(IndexMap::is_empty), + "expected no collected parameters, got: {:?}", + p.other + ); } #[test] @@ -144,12 +167,25 @@ fn deserialize_preserves_the_untagged_reasoning_field() { } #[test] -fn deserialize_keeps_an_explicit_empty_other() { - // Serialization emits `other = {}` for a present-but-empty map, so dropping - // it here would make a stored config lossy on round-trip. +fn deserialize_hoists_an_empty_legacy_other_table() { let p = parameters_from_toml("other = {}"); - assert_eq!(p.other, Some(IndexMap::new())); + assert!( + p.other.as_ref().is_none_or(IndexMap::is_empty), + "an empty legacy table leaves no parameter behind, got: {:?}", + p.other + ); +} + +/// A provider parameter that is itself called `other` is written like any +/// other, now that the name is not a wrapper. +#[test] +fn a_parameter_named_other_is_not_a_wrapper() { + let mut p = PartialParametersConfig::default(); + p.assign(KvAssignment::try_from_cli("other", "5").unwrap()) + .unwrap(); + + assert_eq!(p.other.as_ref().unwrap()["other"], JsonValue(json!("5"))); } #[test] @@ -157,11 +193,11 @@ fn stop_words_append_across_layers() { use schematic::PartialConfig as _; let mut base = PartialParametersConfig { - stop_words: Some(vec!["STOP".to_owned()]), + stop_words: Some(vec!["STOP".to_owned()].into()), ..Default::default() }; let overlay = PartialParametersConfig { - stop_words: Some(vec!["HALT".to_owned()]), + stop_words: Some(vec!["HALT".to_owned()].into()), ..Default::default() }; @@ -169,7 +205,7 @@ fn stop_words_append_across_layers() { assert_eq!( base.stop_words, - Some(vec!["STOP".to_owned(), "HALT".to_owned()]) + Some(vec!["STOP".to_owned(), "HALT".to_owned()].into()) ); } diff --git a/crates/jp_config/src/model_tests.rs b/crates/jp_config/src/model_tests.rs index 1089e158d..163a31c10 100644 --- a/crates/jp_config/src/model_tests.rs +++ b/crates/jp_config/src/model_tests.rs @@ -118,7 +118,7 @@ fn test_model_config_parameters() { p.assign(kv).unwrap(); assert_eq!( p.parameters.stop_words, - Some(vec!["foo".into(), "bar".into()]) + Some(vec!["foo".to_owned(), "bar".to_owned()].into()) ); let kv = KvAssignment::try_from_cli("parameters:", r#"{"max_tokens":42,"reasoning":{"effort":"low"},"temperature":0.42,"top_p":0.42,"top_k":42,"stop_words":["foo","bar"]}"#).unwrap(); @@ -138,7 +138,7 @@ fn test_model_config_parameters() { assert_eq!(p.parameters.top_k, Some(42)); assert_eq!( p.parameters.stop_words, - Some(vec!["foo".into(), "bar".into()]) + Some(vec!["foo".to_owned(), "bar".to_owned()].into()) ); let kv = KvAssignment::try_from_cli("parameters:", r#"{"reasoning":"off"}"#).unwrap(); diff --git a/crates/jp_config/src/plugins.rs b/crates/jp_config/src/plugins.rs index 801c433d2..05b9d8766 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_opt}, fill::fill_map, + internal::merge::map_with_strategy, partial::ToPartial, plugins::command::CommandPluginConfig, - util::merge_nested_indexmap, + types::map::{MergeableMap, map_to_partial_per_key}, }; /// Plugin configuration. @@ -33,8 +33,13 @@ pub struct PluginsConfig { pub shutdown_timeout_secs: u16, /// Command plugin configurations, keyed by plugin name (e.g. `serve`). - #[setting(nested, merge = merge_nested_indexmap)] - pub command: IndexMap, + /// + /// Entries merge by key, so a plugin configured in a later layer joins the + /// ones an earlier layer set. + /// Declare the map as `{ value = { … }, strategy = "replace" }` to drop + /// them instead. + #[setting(nested, merge = map_with_strategy)] + pub command: MergeableMap, } impl AssignKeyValue for PartialPluginsConfig { @@ -56,26 +61,13 @@ 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: 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(&self.command, next.command), } } } @@ -87,7 +79,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 +100,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 76b364e1d..d2d0150d1 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; use crate::{ assignment::{AssignKeyValue, AssignResult, KvAssignment, missing_key}, - delta::{PartialConfigDelta, delta_map, delta_map_with_unsets, path}, + delta::{PartialConfigDelta, delta_mergeable_map, path}, fill::{FillDefaults, fill_map}, + internal::merge::map_with_strategy, partial::ToPartial, providers::{ llm::{LlmProviderConfig, PartialLlmProviderConfig}, mcp::McpProviderConfig, }, - util::merge_nested_indexmap, + types::map::{MergeableMap, map_to_partial_per_key}, }; /// Provider configuration. @@ -33,8 +33,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 AssignKeyValue for PartialProviderConfig { @@ -58,7 +70,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), } } @@ -67,7 +79,9 @@ impl PartialConfigDelta for PartialProviderConfig { llm: self .llm .delta_with_unsets(next.llm, &path(prefix, "llm"), unsets), - mcp: delta_map_with_unsets(&path(prefix, "mcp"), &self.mcp, next.mcp, unsets), + // The map states its own strategy, so a removed server travels in + // the value as a `replace` and needs no path reported. + mcp: delta_mergeable_map(&self.mcp, next.mcp), } } } @@ -76,7 +90,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(), + }, } } } @@ -85,11 +106,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 ab2637560..0cef8b943 100644 --- a/crates/jp_config/src/providers/llm.rs +++ b/crates/jp_config/src/providers/llm.rs @@ -14,8 +14,9 @@ use schematic::Config; use crate::{ assignment::{AssignKeyValue, AssignResult, KvAssignment, missing_key}, - delta::{PartialConfigDelta, delta_map, path}, + delta::{PartialConfigDelta, delta_mergeable_map, path}, fill::{FillDefaults, fill_map}, + internal::merge::map_with_strategy, model::id::{ModelIdConfig, ModelIdConfigError, ModelIdOrAliasConfig, resolve_alias_chain}, partial::ToPartial, providers::llm::{ @@ -28,7 +29,7 @@ use crate::{ openai::{OpenaiConfig, PartialOpenaiConfig}, openrouter::{OpenrouterConfig, PartialOpenrouterConfig}, }, - util::merge_nested_indexmap, + types::map::{MergeableMap, map_to_partial_per_key}, }; /// Provider configuration. @@ -48,8 +49,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)] @@ -110,7 +116,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), @@ -124,7 +130,9 @@ 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), + // The map states its own strategy, so a removed alias travels in + // the value as a `replace` and needs no path reported. + aliases: delta_mergeable_map(&self.aliases, next.aliases), anthropic: self.anthropic.delta_with_unsets( next.anthropic, &path(prefix, "anthropic"), @@ -136,7 +144,11 @@ impl PartialConfigDelta for PartialLlmProviderConfig { llamacpp: self.llamacpp.delta(next.llamacpp), ollama: self.ollama.delta(next.ollama), openai: self.openai.delta(next.openai), - openrouter: self.openrouter.delta(next.openrouter), + openrouter: self.openrouter.delta_with_unsets( + next.openrouter, + &path(prefix, "openrouter"), + unsets, + ), } } } @@ -144,7 +156,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), @@ -160,11 +178,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/llm/anthropic.rs b/crates/jp_config/src/providers/llm/anthropic.rs index d5fe926b2..95cbeb9ad 100644 --- a/crates/jp_config/src/providers/llm/anthropic.rs +++ b/crates/jp_config/src/providers/llm/anthropic.rs @@ -4,10 +4,11 @@ use schematic::Config; use crate::{ assignment::{AssignKeyValue, AssignResult, KvAssignment, missing_key}, - delta::{PartialConfigDelta, delta_opt, delta_opt_vec, delta_opt_vec_at, path}, + delta::{PartialConfigDelta, delta_opt, delta_opt_mergeable_vec}, fill::FillDefaults, - internal::merge::append_vec_dedup, + internal::merge::vec_with_strategy, partial::{ToPartial, partial_opt}, + types::vec::MergeableVec, }; /// Anthropic API configuration. @@ -38,7 +39,11 @@ pub struct AnthropicConfig { /// /// To find out which beta headers are available, see: /// - #[setting(default = vec![], merge = append_vec_dedup)] + #[setting( + default = MergeableVec::default(), + partial_via = MergeableVec::, + merge = vec_with_strategy, + )] pub beta_headers: Vec, } @@ -49,7 +54,7 @@ impl AssignKeyValue for PartialAnthropicConfig { "api_key_env" => self.api_key_env = kv.try_some_string()?, "base_url" => self.base_url = kv.try_some_string()?, "chain_on_max_tokens" => self.chain_on_max_tokens = kv.try_some_bool()?, - "beta_headers" => kv.try_some_vec_of_strings(&mut self.beta_headers)?, + "beta_headers" => kv.try_some_mergeable_strings(&mut self.beta_headers)?, _ => return missing_key(&kv), } @@ -66,26 +71,13 @@ impl PartialConfigDelta for PartialAnthropicConfig { self.chain_on_max_tokens.as_ref(), next.chain_on_max_tokens, ), - beta_headers: delta_opt_vec(self.beta_headers.as_ref(), next.beta_headers), + beta_headers: delta_opt_mergeable_vec(self.beta_headers.as_ref(), next.beta_headers), } } - fn delta_with_unsets(&self, next: Self, prefix: &str, unsets: &mut Vec) -> Self { - Self { - api_key_env: delta_opt(self.api_key_env.as_ref(), next.api_key_env), - base_url: delta_opt(self.base_url.as_ref(), next.base_url), - chain_on_max_tokens: delta_opt( - self.chain_on_max_tokens.as_ref(), - next.chain_on_max_tokens, - ), - beta_headers: delta_opt_vec_at( - &path(prefix, "beta_headers"), - self.beta_headers.as_ref(), - next.beta_headers, - unsets, - ), - } - } + // No `delta_with_unsets`: every field here is reachable by merging, now that + // `beta_headers` carries its own strategy. The default implementation, which + // is the plain diff, is correct. } impl FillDefaults for PartialAnthropicConfig { @@ -110,7 +102,10 @@ impl ToPartial for AnthropicConfig { &self.chain_on_max_tokens, defaults.chain_on_max_tokens, ), - beta_headers: partial_opt(&self.beta_headers, defaults.beta_headers), + beta_headers: partial_opt( + &MergeableVec::from(self.beta_headers.clone()), + defaults.beta_headers, + ), } } } diff --git a/crates/jp_config/src/providers/llm/openrouter.rs b/crates/jp_config/src/providers/llm/openrouter.rs index faabadd33..a2bbdc1cf 100644 --- a/crates/jp_config/src/providers/llm/openrouter.rs +++ b/crates/jp_config/src/providers/llm/openrouter.rs @@ -4,7 +4,7 @@ use schematic::Config; use crate::{ assignment::{AssignKeyValue, AssignResult, KvAssignment, missing_key}, - delta::{PartialConfigDelta, delta_opt}, + delta::{PartialConfigDelta, delta_opt, delta_opt_at, path}, fill::FillDefaults, partial::{ToPartial, partial_opt, partial_opts}, }; @@ -55,6 +55,35 @@ impl PartialConfigDelta for PartialOpenrouterConfig { base_url: delta_opt(self.base_url.as_ref(), next.base_url), } } + + fn delta_with_unsets(&self, next: Self, prefix: &str, unsets: &mut Vec) -> Self { + Self { + api_key_env: delta_opt_at( + &path(prefix, "api_key_env"), + self.api_key_env.as_ref(), + next.api_key_env, + unsets, + ), + app_name: delta_opt_at( + &path(prefix, "app_name"), + self.app_name.as_ref(), + next.app_name, + unsets, + ), + app_referrer: delta_opt_at( + &path(prefix, "app_referrer"), + self.app_referrer.as_ref(), + next.app_referrer, + unsets, + ), + base_url: delta_opt_at( + &path(prefix, "base_url"), + self.base_url.as_ref(), + next.base_url, + unsets, + ), + } + } } impl FillDefaults for PartialOpenrouterConfig { diff --git a/crates/jp_config/src/providers/mcp.rs b/crates/jp_config/src/providers/mcp.rs index 046660cf9..a50af372e 100644 --- a/crates/jp_config/src/providers/mcp.rs +++ b/crates/jp_config/src/providers/mcp.rs @@ -7,10 +7,10 @@ use serde::{Deserialize, Serialize}; use crate::{ assignment::{AssignKeyValue, AssignResult, KvAssignment, missing_key}, - delta::{ - PartialConfigDelta, delta_opt, delta_opt_partial, delta_opt_vec, delta_opt_vec_at, path, - }, + delta::{PartialConfigDelta, delta_opt, delta_opt_mergeable_vec, delta_opt_partial}, + internal::merge::ordered_vec_with_strategy, partial::{ToPartial, partial_opt, partial_opt_config}, + types::vec::MergeableVec, }; /// MCP provider configuration. @@ -35,8 +35,8 @@ impl PartialConfigDelta for PartialMcpProviderConfig { match (self, next) { (Self::Stdio(prev), Self::Stdio(next)) => Self::Stdio(PartialStdioConfig { command: delta_opt(prev.command.as_ref(), next.command), - arguments: delta_opt_vec(prev.arguments.as_ref(), next.arguments), - variables: delta_opt_vec(prev.variables.as_ref(), next.variables), + arguments: delta_opt_mergeable_vec(prev.arguments.as_ref(), next.arguments), + variables: delta_opt_mergeable_vec(prev.variables.as_ref(), next.variables), checksum: delta_opt_partial(prev.checksum.as_ref(), next.checksum), optional: delta_opt(prev.optional.as_ref(), next.optional), startup_timeout_secs: delta_opt( @@ -47,31 +47,8 @@ impl PartialConfigDelta for PartialMcpProviderConfig { } } - fn delta_with_unsets(&self, next: Self, prefix: &str, unsets: &mut Vec) -> Self { - match (self, next) { - (Self::Stdio(prev), Self::Stdio(next)) => Self::Stdio(PartialStdioConfig { - command: delta_opt(prev.command.as_ref(), next.command), - arguments: delta_opt_vec_at( - &path(prefix, "arguments"), - prev.arguments.as_ref(), - next.arguments, - unsets, - ), - variables: delta_opt_vec_at( - &path(prefix, "variables"), - prev.variables.as_ref(), - next.variables, - unsets, - ), - checksum: delta_opt_partial(prev.checksum.as_ref(), next.checksum), - optional: delta_opt(prev.optional.as_ref(), next.optional), - startup_timeout_secs: delta_opt( - prev.startup_timeout_secs.as_ref(), - next.startup_timeout_secs, - ), - }), - } - } + // No `delta_with_unsets`: `arguments` and `variables` state `replace` + // themselves now, so no field here needs a path reported. } impl McpProviderConfig { @@ -105,7 +82,18 @@ pub struct StdioConfig { pub command: PathBuf, /// The arguments to pass to the command. - #[setting(default, merge = schematic::merge::append_vec)] + /// + /// Appends to the list from any earlier layer. + /// Set a strategy to override that: + /// + /// ```toml + /// arguments = { value = ["serve"], strategy = "replace" } + /// ``` + #[setting( + default, + partial_via = MergeableVec::, + merge = ordered_vec_with_strategy, + )] pub arguments: Vec, /// The environment variables to expose to the command. @@ -113,7 +101,14 @@ pub struct StdioConfig { /// By default, the command inherits the environment of the parent process. /// You can use this to add additional environment variables, or override /// existing ones. - #[setting(default, merge = schematic::merge::append_vec)] + /// + /// Appends to the list from any earlier layer, and accepts a `strategy` the + /// same way `arguments` does. + #[setting( + default, + partial_via = MergeableVec::, + merge = ordered_vec_with_strategy, + )] pub variables: Vec, /// The binary checksum for the binary. @@ -151,8 +146,8 @@ impl AssignKeyValue for PartialStdioConfig { match kv.key_string().as_str() { "" => kv.try_merge_object(self)?, "command" => self.command = kv.try_some_from_str()?, - _ if kv.p("arguments") => kv.try_some_vec_of_strings(&mut self.arguments)?, - _ if kv.p("variables") => kv.try_some_vec_of_strings(&mut self.variables)?, + _ if kv.p("arguments") => kv.try_some_mergeable_strings(&mut self.arguments)?, + _ if kv.p("variables") => kv.try_some_mergeable_strings(&mut self.variables)?, _ if kv.p("checksum") => self.checksum.assign(kv)?, "optional" => self.optional = kv.try_some_bool()?, "startup_timeout_secs" => self.startup_timeout_secs = kv.try_some_u32()?, @@ -169,8 +164,14 @@ impl ToPartial for StdioConfig { PartialStdioConfig { command: partial_opt(&self.command, defaults.command), - arguments: partial_opt(&self.arguments, defaults.arguments), - variables: partial_opt(&self.variables, defaults.variables), + arguments: partial_opt( + &MergeableVec::from(self.arguments.clone()), + defaults.arguments, + ), + variables: partial_opt( + &MergeableVec::from(self.variables.clone()), + defaults.variables, + ), checksum: partial_opt_config(self.checksum.as_ref(), defaults.checksum), optional: partial_opt(&self.optional, defaults.optional), startup_timeout_secs: partial_opt( diff --git a/crates/jp_config/src/providers/mcp_tests.rs b/crates/jp_config/src/providers/mcp_tests.rs index b82421ea7..8eb5cb1da 100644 --- a/crates/jp_config/src/providers/mcp_tests.rs +++ b/crates/jp_config/src/providers/mcp_tests.rs @@ -2,7 +2,10 @@ use schematic::PartialConfig as _; use test_log::test; use super::*; -use crate::assignment::KvAssignment; +use crate::{ + assignment::KvAssignment, + types::vec::{MergeableVec, MergedVec}, +}; #[test] fn stdio_optional_defaults_to_false() { @@ -61,29 +64,32 @@ fn assign_startup_timeout_via_cli() { fn arguments_and_variables_append_across_layers() { use schematic::PartialConfig as _; - // Both fields declare `merge = append_vec`, so a later layer adds to the - // earlier one rather than replacing it. + // Both fields append a later layer onto the earlier one, and keep + // duplicates while doing it, so the merged value carries `dedup = false`. let mut base = PartialStdioConfig { - arguments: Some(vec!["serve".to_owned()]), - variables: Some(vec!["HOME".to_owned()]), + arguments: Some(vec!["serve".to_owned()].into()), + variables: Some(vec!["HOME".to_owned()].into()), ..Default::default() }; let overlay = PartialStdioConfig { - arguments: Some(vec!["--verbose".to_owned()]), - variables: Some(vec!["PATH".to_owned()]), + arguments: Some(vec!["--verbose".to_owned()].into()), + variables: Some(vec!["PATH".to_owned()].into()), ..Default::default() }; base.merge(&(), overlay).unwrap(); - assert_eq!( - base.arguments, - Some(vec!["serve".to_owned(), "--verbose".to_owned()]) - ); - assert_eq!( - base.variables, - Some(vec!["HOME".to_owned(), "PATH".to_owned()]) - ); + let ordered = |values: &[&str]| { + Some(MergeableVec::Merged(MergedVec { + value: values.iter().map(|v| (*v).to_owned()).collect(), + strategy: None, + dedup: Some(false), + discard_when_merged: false, + })) + }; + + assert_eq!(base.arguments, ordered(&["serve", "--verbose"])); + assert_eq!(base.variables, ordered(&["HOME", "PATH"])); } #[test] diff --git a/crates/jp_config/src/snapshots/jp_config__tests__app_config_fields.snap b/crates/jp_config/src/snapshots/jp_config__tests__app_config_fields.snap index 736c176b0..e1e1d6cc1 100644 --- a/crates/jp_config/src/snapshots/jp_config__tests__app_config_fields.snap +++ b/crates/jp_config/src/snapshots/jp_config__tests__app_config_fields.snap @@ -26,7 +26,7 @@ expression: "AppConfig::fields()" "style.reasoning.extend_across_tool_calls", "style.reasoning.summary_model.id", "style.reasoning.summary_model.parameters.max_tokens", - "style.reasoning.summary_model.parameters.other", + "style.reasoning.summary_model.parameters", "style.reasoning.summary_model.parameters.reasoning", "style.reasoning.summary_model.parameters.stop_words", "style.reasoning.summary_model.parameters.temperature", @@ -110,7 +110,7 @@ expression: "AppConfig::fields()" "conversation.title.generate.auto", "conversation.title.generate.model.id", "conversation.title.generate.model.parameters.max_tokens", - "conversation.title.generate.model.parameters.other", + "conversation.title.generate.model.parameters", "conversation.title.generate.model.parameters.reasoning", "conversation.title.generate.model.parameters.stop_words", "conversation.title.generate.model.parameters.temperature", @@ -129,7 +129,7 @@ expression: "AppConfig::fields()" "conversation.inquiry.assistant.request.stream_idle_timeout_secs", "conversation.inquiry.assistant.model.id", "conversation.inquiry.assistant.model.parameters.max_tokens", - "conversation.inquiry.assistant.model.parameters.other", + "conversation.inquiry.assistant.model.parameters", "conversation.inquiry.assistant.model.parameters.reasoning", "conversation.inquiry.assistant.model.parameters.stop_words", "conversation.inquiry.assistant.model.parameters.temperature", @@ -149,7 +149,7 @@ expression: "AppConfig::fields()" "assistant.request.stream_idle_timeout_secs", "assistant.model.id", "assistant.model.parameters.max_tokens", - "assistant.model.parameters.other", + "assistant.model.parameters", "assistant.model.parameters.reasoning", "assistant.model.parameters.stop_words", "assistant.model.parameters.temperature", 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 5863e697b..f4f905dc0 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 @@ -72,7 +72,9 @@ PartialAppConfig { }, access: None, }, - tools: {}, + tools: Map( + {}, + ), }, compaction: PartialCompactionConfig { rules: Vec( @@ -205,11 +207,15 @@ PartialAppConfig { }, }, template: PartialTemplateConfig { - values: {}, + values: Map( + {}, + ), }, providers: PartialProviderConfig { llm: PartialLlmProviderConfig { - aliases: {}, + aliases: Map( + {}, + ), anthropic: PartialAnthropicConfig { api_key_env: None, base_url: None, @@ -247,12 +253,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 0e0b48fd2..3abe64882 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 @@ -137,7 +137,9 @@ Ok( }, access: None, }, - tools: {}, + tools: Map( + {}, + ), }, compaction: PartialCompactionConfig { rules: Merged( @@ -414,22 +416,28 @@ Ok( editor: PartialEditorConfig { cmd: None, envs: Some( - [ - "JP_EDITOR", - "VISUAL", - "EDITOR", - ], + Vec( + [ + "JP_EDITOR", + "VISUAL", + "EDITOR", + ], + ), ), inline: PartialInlineEditorConfig { edit_mode: None, }, }, template: PartialTemplateConfig { - values: {}, + values: Map( + {}, + ), }, providers: PartialProviderConfig { llm: PartialLlmProviderConfig { - aliases: {}, + aliases: Map( + {}, + ), anthropic: PartialAnthropicConfig { api_key_env: Some( "ANTHROPIC_API_KEY", @@ -441,7 +449,9 @@ Ok( true, ), beta_headers: Some( - [], + Vec( + [], + ), ), }, cerebras: PartialCerebrasConfig { @@ -503,7 +513,9 @@ Ok( ), }, }, - mcp: {}, + mcp: Map( + {}, + ), }, plugins: PartialPluginsConfig { auto_install: Some( @@ -512,7 +524,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 e17c8379f..bb906be6a 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 @@ -72,7 +72,9 @@ PartialAppConfig { }, access: None, }, - tools: {}, + tools: Map( + {}, + ), }, compaction: PartialCompactionConfig { rules: Vec( @@ -205,11 +207,15 @@ PartialAppConfig { }, }, template: PartialTemplateConfig { - values: {}, + values: Map( + {}, + ), }, providers: PartialProviderConfig { llm: PartialLlmProviderConfig { - aliases: {}, + aliases: Map( + {}, + ), anthropic: PartialAnthropicConfig { api_key_env: None, base_url: None, @@ -247,12 +253,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/style.rs b/crates/jp_config/src/style.rs index 1f6fe3244..4d3510606 100644 --- a/crates/jp_config/src/style.rs +++ b/crates/jp_config/src/style.rs @@ -133,7 +133,11 @@ impl PartialConfigDelta for PartialStyleConfig { fn delta_with_unsets(&self, next: Self, prefix: &str, unsets: &mut Vec) -> Self { Self { code: self.code.delta(next.code), - inline_code: self.inline_code.delta(next.inline_code), + inline_code: self.inline_code.delta_with_unsets( + next.inline_code, + &path(prefix, "inline_code"), + unsets, + ), markdown: self.markdown.delta(next.markdown), mcp_startup: self.mcp_startup.delta(next.mcp_startup), reasoning: self.reasoning.delta_with_unsets( diff --git a/crates/jp_config/src/style/inline_code.rs b/crates/jp_config/src/style/inline_code.rs index 1e6f3ed08..4ec56abac 100644 --- a/crates/jp_config/src/style/inline_code.rs +++ b/crates/jp_config/src/style/inline_code.rs @@ -4,7 +4,7 @@ use schematic::Config; use crate::{ assignment::{AssignKeyValue, AssignResult, KvAssignment, missing_key}, - delta::{PartialConfigDelta, delta_opt}, + delta::{PartialConfigDelta, delta_opt, delta_opt_at, path}, fill::FillDefaults, partial::ToPartial, types::color::Color, @@ -45,6 +45,17 @@ impl PartialConfigDelta for PartialInlineCodeConfig { background: delta_opt(self.background.as_ref(), next.background), } } + + fn delta_with_unsets(&self, next: Self, prefix: &str, unsets: &mut Vec) -> Self { + Self { + background: delta_opt_at( + &path(prefix, "background"), + self.background.as_ref(), + next.background, + unsets, + ), + } + } } impl FillDefaults for PartialInlineCodeConfig { diff --git a/crates/jp_config/src/template.rs b/crates/jp_config/src/template.rs index 8174269c5..60645c5de 100644 --- a/crates/jp_config/src/template.rs +++ b/crates/jp_config/src/template.rs @@ -1,15 +1,14 @@ //! Template configuration for Jean-Pierre. -use indexmap::IndexMap; use schematic::Config; use crate::{ assignment::{AssignKeyValue, KvAssignment, missing_key}, - delta::PartialConfigDelta, + delta::{PartialConfigDelta, delta_mergeable_value_map}, fill::FillDefaults, + internal::merge::map_with_strategy, partial::ToPartial, - types::json_value::JsonValue, - util::merge_nested_indexmap, + types::{json_value::JsonValue, map::MergeableMap}, }; /// Template configuration. @@ -17,8 +16,13 @@ use crate::{ #[config(rename_all = "snake_case")] pub struct TemplateConfig { /// Template variable values used to render query templates. - #[setting(nested, merge = merge_nested_indexmap)] - pub values: IndexMap, + /// + /// Entries merge by key, so a value set in a later layer joins the ones an + /// earlier layer set. + /// Declare the map as `{ value = { … }, strategy = "replace" }` to drop + /// them instead. + #[setting(nested, merge = map_with_strategy)] + pub values: MergeableMap, } impl AssignKeyValue for PartialTemplateConfig { @@ -36,34 +40,30 @@ 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), } } } 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..d490bb6c8 100644 --- a/crates/jp_config/src/types/map.rs +++ b/crates/jp_config/src/types/map.rs @@ -182,6 +182,23 @@ pub fn map_to_mergeable_partial<'a, T: ToPartial + 'a>( }) } +/// Convert a resolved map to a `MergeableMap` that merges per key. +/// +/// Used by `ToPartial` impls for a map that should still take an entry a later +/// layer adds, which a `replace` strategy would drop. +/// Re-merging the result over the layer it came from reproduces it rather than +/// combining with it, because each entry's own fields state their strategies. +pub fn map_to_partial_per_key<'a, T: ToPartial + 'a>( + entries: impl IntoIterator, +) -> MergeableMap { + MergeableMap::Map( + entries + .into_iter() + .map(|(k, v)| (k.clone(), v.to_partial())) + .collect(), + ) +} + impl From> for MergeableMap { fn from(value: IndexMap) -> Self { Self::Map(value) 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/unset_tests.rs b/crates/jp_config/src/unset_tests.rs index b1366a910..230384f6d 100644 --- a/crates/jp_config/src/unset_tests.rs +++ b/crates/jp_config/src/unset_tests.rs @@ -13,7 +13,7 @@ fn partial_with_server() -> PartialAppConfig { "bookworm".to_owned(), PartialMcpProviderConfig::Stdio(PartialStdioConfig { command: Some("just".into()), - arguments: Some(vec!["serve".to_owned(), "--verbose".to_owned()]), + arguments: Some(vec!["serve".to_owned(), "--verbose".to_owned()].into()), ..PartialStdioConfig::default() }), ); @@ -23,7 +23,7 @@ fn partial_with_server() -> PartialAppConfig { /// The `arguments` of the `bookworm` server, if the entry is present. fn arguments(partial: &PartialAppConfig) -> Option<&Vec> { let PartialMcpProviderConfig::Stdio(config) = partial.providers.mcp.get("bookworm")?; - config.arguments.as_ref() + config.arguments.as_deref() } #[test] @@ -78,11 +78,10 @@ fn unset_of_an_absent_map_entry_is_a_no_op() { /// Paths that clearing does not reach, and why. /// -/// Neither is settable by `--cfg` either: `extends` and `loader` are read while -/// the file declaring them is loaded, and only their effect outlives that ([RFD -/// 038]), so neither has a key-value arm to reach. -/// -/// [RFD 038]: https://jp.computer/rfd/038 +/// Both are load-time fields (see [`crate::delta::LOAD_TIME_ONLY`]) with no +/// key-value arm at all, so there is nothing to reach rather than something +/// that refuses. +/// `inherit` is the third of that set and *is* settable, so it is absent here. const UNREACHABLE: &[&str] = &["extends", "loader.reset"]; /// Every field the schema names is reachable by path, or listed as not. @@ -149,7 +148,7 @@ fn a_cleared_list_takes_the_next_layers_value_verbatim() { next.providers.mcp.insert( "bookworm".to_owned(), PartialMcpProviderConfig::Stdio(PartialStdioConfig { - arguments: Some(vec!["serve".to_owned()]), + arguments: Some(vec!["serve".to_owned()].into()), ..PartialStdioConfig::default() }), ); @@ -170,7 +169,7 @@ fn an_uncleared_list_appends_the_next_layers_value() { next.providers.mcp.insert( "bookworm".to_owned(), PartialMcpProviderConfig::Stdio(PartialStdioConfig { - arguments: Some(vec!["serve".to_owned()]), + arguments: Some(vec!["serve".to_owned()].into()), ..PartialStdioConfig::default() }), ); diff --git a/crates/jp_config/src/user.rs b/crates/jp_config/src/user.rs index d0e63954e..172258222 100644 --- a/crates/jp_config/src/user.rs +++ b/crates/jp_config/src/user.rs @@ -4,7 +4,7 @@ use schematic::Config; use crate::{ assignment::{AssignKeyValue, AssignResult, KvAssignment, missing_key}, - delta::{PartialConfigDelta, delta_opt}, + delta::{PartialConfigDelta, delta_opt, delta_opt_at, path}, fill::FillDefaults, partial::{ToPartial, partial_opts}, }; @@ -46,6 +46,12 @@ impl PartialConfigDelta for PartialUserConfig { name: delta_opt(self.name.as_ref(), next.name), } } + + fn delta_with_unsets(&self, next: Self, prefix: &str, unsets: &mut Vec) -> Self { + Self { + name: delta_opt_at(&path(prefix, "name"), self.name.as_ref(), next.name, unsets), + } + } } impl FillDefaults for PartialUserConfig { 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_config/src/util_tests.rs b/crates/jp_config/src/util_tests.rs index ccbb3d786..9862313ff 100644 --- a/crates/jp_config/src/util_tests.rs +++ b/crates/jp_config/src/util_tests.rs @@ -972,9 +972,8 @@ fn test_load_partial_at_path_repeat_visit_keeps_last_position() { fn load_paths(partial: &PartialAppConfig) -> Vec<&str> { partial .config_load_paths - .as_deref() - .unwrap_or_default() .iter() + .flat_map(|paths| paths.iter()) .map(|p| p.as_str()) .collect() } diff --git a/crates/jp_conversation/src/compat.rs b/crates/jp_conversation/src/compat.rs index f48d24ef0..3e7b1b173 100644 --- a/crates/jp_conversation/src/compat.rs +++ b/crates/jp_conversation/src/compat.rs @@ -325,9 +325,50 @@ 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(|obj| obj.contains_key("value") && obj.contains_key("strategy")); + + Some(if stated { wrapper } else { collection }) +} + /// The only item an iterator yields, if it yields exactly one. fn sole<'a>(mut variants: impl Iterator) -> Option<&'a Schema> { match (variants.next(), variants.next()) { @@ -392,7 +433,8 @@ fn strip_struct<'a>( return 0; }; - let entry_schema = flattened_entry_schema(struct_type); + let flattened = flattened_field_schema(struct_type); + let entry_schema = flattened.and_then(map_value_schema); let has_flatten = struct_type.fields.values().any(|f| f.flatten); let mut stripped = if has_flatten { @@ -426,7 +468,7 @@ fn strip_struct<'a>( /// flattens something other than a map — in each of those cases the shape of a /// leftover key is not knowable, and walking it against the wrong schema would /// delete valid data. -fn flattened_entry_schema(struct_type: &StructType) -> Option<&Schema> { +fn flattened_field_schema(struct_type: &StructType) -> Option<&Schema> { let mut flattened = struct_type .fields .values() @@ -434,11 +476,33 @@ fn flattened_entry_schema(struct_type: &StructType) -> Option<&Schema> { .map(Box::as_ref); match (flattened.next(), flattened.next()) { - (Some(SchemaField { schema, .. }), None) => match &schema.ty { + (Some(SchemaField { schema, .. }), None) => Some(schema), + _ => None, + } +} + +/// The schema of a map's values, for a map written either plainly or with a +/// stated merge strategy. +/// +/// A map that can state one is a union of the plain map and the wrapper holding +/// it under `value`. +/// Flattened, its entries are sibling keys of the struct around it, which is +/// the plain map's shape, so that is the variant their values are walked +/// against. +fn map_value_schema(schema: &Schema) -> Option<&Schema> { + fn value_type(ty: &SchemaType) -> Option<&Schema> { + match ty { SchemaType::Object(object_type) => Some(&object_type.value_type), _ => None, - }, - _ => None, + } + } + + match &schema.ty { + SchemaType::Union(union_type) => union_type + .variants_types + .iter() + .find_map(|variant| value_type(&variant.ty)), + ty => value_type(ty), } } diff --git a/crates/jp_conversation/src/compat_tests.rs b/crates/jp_conversation/src/compat_tests.rs index a952fb559..f1d2a2699 100644 --- a/crates/jp_conversation/src/compat_tests.rs +++ b/crates/jp_conversation/src/compat_tests.rs @@ -673,6 +673,72 @@ fn schema_style_code_is_struct_with_color() { ); } +/// Provider parameters survive a stored config, in either spelling. +/// +/// They are collected into a flattened field, so they arrive as keys the schema +/// does not name. +/// Stripping would forward the conversation's next request without them, +/// silently changing what the model is asked. +#[test] +fn partial_config_keeps_provider_parameters() { + let value = json!({ + "assistant": { + "model": { + "parameters": { + "temperature": 0.7, + "presence_penalty": 0.5, + }, + }, + }, + }); + + let config = deserialize_partial_config(value); + let parameters = &config.assistant.model.parameters; + + assert_eq!(parameters.temperature, Some(0.7)); + assert_eq!( + parameters + .other + .as_ref() + .and_then(|o| o.get("presence_penalty")), + Some(&jp_config::types::json_value::JsonValue(json!(0.5))), + "a parameter JP does not model is not a stray key to strip" + ); +} + +/// A config stored before the collector was flattened nested its parameters +/// under `other`, and they still arrive as parameters. +#[test] +fn partial_config_hoists_a_legacy_other_table() { + let value = json!({ + "assistant": { + "model": { + "parameters": { + "other": { "presence_penalty": 0.5 }, + }, + }, + }, + }); + + let config = deserialize_partial_config(value); + let other = config + .assistant + .model + .parameters + .other + .as_ref() + .expect("the legacy table is hoisted"); + + assert_eq!( + other.get("presence_penalty"), + Some(&jp_config::types::json_value::JsonValue(json!(0.5))) + ); + assert!( + !other.contains_key("other"), + "the wrapper is not itself a parameter: {other:?}" + ); +} + #[test] fn strip_directly_on_delta_subtree() { // Reproduce exactly what deserialize_config_delta does: strip the "delta" diff --git a/crates/jp_conversation/src/stream_tests.rs b/crates/jp_conversation/src/stream_tests.rs index 5f80ebe01..e6c65b6c6 100644 --- a/crates/jp_conversation/src/stream_tests.rs +++ b/crates/jp_conversation/src/stream_tests.rs @@ -45,13 +45,36 @@ fn stream_with_server(arguments: &[&str]) -> ConversationStream { /// A partial setting the `bookworm` server's arguments and nothing else. fn server_arguments_partial(arguments: &[&str]) -> jp_config::PartialAppConfig { + arguments_partial( + arguments + .iter() + .map(|a| (*a).to_owned()) + .collect::>(), + ) +} + +/// The same, with the list asking to replace rather than extend. +fn replacing_server_arguments_partial(arguments: &[&str]) -> jp_config::PartialAppConfig { + use jp_config::types::vec::{MergeableVec, MergedVec, MergedVecStrategy}; + + arguments_partial(MergeableVec::Merged(MergedVec { + value: arguments.iter().map(|a| (*a).to_owned()).collect(), + strategy: Some(MergedVecStrategy::Replace), + dedup: None, + discard_when_merged: false, + })) +} + +fn arguments_partial( + arguments: impl Into>, +) -> jp_config::PartialAppConfig { use jp_config::providers::mcp::{PartialMcpProviderConfig, PartialStdioConfig}; let mut partial = jp_config::PartialAppConfig::empty(); partial.providers.mcp.insert( "bookworm".to_owned(), PartialMcpProviderConfig::Stdio(PartialStdioConfig { - arguments: Some(arguments.iter().map(|a| (*a).to_owned()).collect()), + arguments: Some(arguments.into()), ..PartialStdioConfig::default() }), ); @@ -84,28 +107,22 @@ fn an_unset_clears_a_field_before_the_delta_merges() { assert_eq!(resolved_arguments(&stream), ["serve"]); } -/// Without the clear, the same change cannot be recorded at all. +/// A list that states `replace` needs no clear to reach the same result. /// -/// No list the delta could carry produces `["serve"]` by appending to -/// `["serve", "--verbose"]`, so the diff comes out empty and no event is -/// written — the conversation keeps the argument the user dropped. +/// `arguments` carries its own merge strategy, so a delta can shorten the list +/// on its own. +/// `unsets` remains for what cannot say it: a scalar going away, and a list +/// whose merge strategy is fixed by its field. #[test] -fn without_an_unset_a_dropped_argument_is_not_recorded() { +fn a_replacing_list_needs_no_unset() { let mut stream = stream_with_server(&["serve", "--verbose"]); stream.add_config_delta(ApplyDelta::new( delta_timestamp(), - server_arguments_partial(&["serve"]), + replacing_server_arguments_partial(&["serve"]), )); - assert_eq!(resolved_arguments(&stream), ["serve", "--verbose"]); - assert!( - !stream - .events - .iter() - .any(|event| matches!(event, InternalEvent::ConfigDelta(_))), - "an empty diff writes no event" - ); + assert_eq!(resolved_arguments(&stream), ["serve"]); } /// A delta that only clears carries no diff, and is still worth recording. 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)?, diff --git a/crates/jp_llm/tests/fixtures/anthropic/test_chat_completion_stream__conversation_stream.snap b/crates/jp_llm/tests/fixtures/anthropic/test_chat_completion_stream__conversation_stream.snap index 865a1c07b..8a1170af6 100644 --- a/crates/jp_llm/tests/fixtures/anthropic/test_chat_completion_stream__conversation_stream.snap +++ b/crates/jp_llm/tests/fixtures/anthropic/test_chat_completion_stream__conversation_stream.snap @@ -32,8 +32,7 @@ expression: v "effort": "low", "exclude": false }, - "stop_words": [], - "other": {} + "stop_words": [] } }, "request": { diff --git a/crates/jp_llm/tests/fixtures/anthropic/test_fable_5_forced_tool_soft_forces__conversation_stream.snap b/crates/jp_llm/tests/fixtures/anthropic/test_fable_5_forced_tool_soft_forces__conversation_stream.snap index e13c76aa8..0847dd23f 100644 --- a/crates/jp_llm/tests/fixtures/anthropic/test_fable_5_forced_tool_soft_forces__conversation_stream.snap +++ b/crates/jp_llm/tests/fixtures/anthropic/test_fable_5_forced_tool_soft_forces__conversation_stream.snap @@ -32,8 +32,7 @@ expression: v "effort": "low", "exclude": false }, - "stop_words": [], - "other": {} + "stop_words": [] } }, "request": { diff --git a/crates/jp_llm/tests/fixtures/anthropic/test_image_attachment__conversation_stream.snap b/crates/jp_llm/tests/fixtures/anthropic/test_image_attachment__conversation_stream.snap index ea9c013f7..a36aa7d0d 100644 --- a/crates/jp_llm/tests/fixtures/anthropic/test_image_attachment__conversation_stream.snap +++ b/crates/jp_llm/tests/fixtures/anthropic/test_image_attachment__conversation_stream.snap @@ -29,8 +29,7 @@ expression: v }, "parameters": { "reasoning": "off", - "stop_words": [], - "other": {} + "stop_words": [] } }, "request": { diff --git a/crates/jp_llm/tests/fixtures/anthropic/test_multi_turn_conversation__conversation_stream.snap b/crates/jp_llm/tests/fixtures/anthropic/test_multi_turn_conversation__conversation_stream.snap index 9fd79f0be..94f492ae3 100644 --- a/crates/jp_llm/tests/fixtures/anthropic/test_multi_turn_conversation__conversation_stream.snap +++ b/crates/jp_llm/tests/fixtures/anthropic/test_multi_turn_conversation__conversation_stream.snap @@ -32,8 +32,7 @@ expression: v "effort": "low", "exclude": false }, - "stop_words": [], - "other": {} + "stop_words": [] } }, "request": { diff --git a/crates/jp_llm/tests/fixtures/anthropic/test_opus_4_6_adaptive_thinking__conversation_stream.snap b/crates/jp_llm/tests/fixtures/anthropic/test_opus_4_6_adaptive_thinking__conversation_stream.snap index 88b3096f4..f9633e2d5 100644 --- a/crates/jp_llm/tests/fixtures/anthropic/test_opus_4_6_adaptive_thinking__conversation_stream.snap +++ b/crates/jp_llm/tests/fixtures/anthropic/test_opus_4_6_adaptive_thinking__conversation_stream.snap @@ -32,8 +32,7 @@ expression: v "effort": "high", "exclude": false }, - "stop_words": [], - "other": {} + "stop_words": [] } }, "request": { diff --git a/crates/jp_llm/tests/fixtures/anthropic/test_opus_4_6_max_effort__conversation_stream.snap b/crates/jp_llm/tests/fixtures/anthropic/test_opus_4_6_max_effort__conversation_stream.snap index d0f88c98f..71730ed84 100644 --- a/crates/jp_llm/tests/fixtures/anthropic/test_opus_4_6_max_effort__conversation_stream.snap +++ b/crates/jp_llm/tests/fixtures/anthropic/test_opus_4_6_max_effort__conversation_stream.snap @@ -32,8 +32,7 @@ expression: v "effort": "max", "exclude": false }, - "stop_words": [], - "other": {} + "stop_words": [] } }, "request": { diff --git a/crates/jp_llm/tests/fixtures/anthropic/test_redacted_thinking__conversation_stream.snap b/crates/jp_llm/tests/fixtures/anthropic/test_redacted_thinking__conversation_stream.snap index d3e787dcc..a731ab036 100644 --- a/crates/jp_llm/tests/fixtures/anthropic/test_redacted_thinking__conversation_stream.snap +++ b/crates/jp_llm/tests/fixtures/anthropic/test_redacted_thinking__conversation_stream.snap @@ -29,8 +29,7 @@ expression: v }, "parameters": { "reasoning": "off", - "stop_words": [], - "other": {} + "stop_words": [] } }, "request": { diff --git a/crates/jp_llm/tests/fixtures/anthropic/test_request_chaining__conversation_stream.snap b/crates/jp_llm/tests/fixtures/anthropic/test_request_chaining__conversation_stream.snap index ed26adec9..856232174 100644 --- a/crates/jp_llm/tests/fixtures/anthropic/test_request_chaining__conversation_stream.snap +++ b/crates/jp_llm/tests/fixtures/anthropic/test_request_chaining__conversation_stream.snap @@ -34,8 +34,7 @@ expression: v }, "exclude": false }, - "stop_words": [], - "other": {} + "stop_words": [] } }, "request": { diff --git a/crates/jp_llm/tests/fixtures/anthropic/test_structured_output__conversation_stream.snap b/crates/jp_llm/tests/fixtures/anthropic/test_structured_output__conversation_stream.snap index 211471c3a..c4eeed12c 100644 --- a/crates/jp_llm/tests/fixtures/anthropic/test_structured_output__conversation_stream.snap +++ b/crates/jp_llm/tests/fixtures/anthropic/test_structured_output__conversation_stream.snap @@ -29,8 +29,7 @@ expression: v }, "parameters": { "reasoning": "off", - "stop_words": [], - "other": {} + "stop_words": [] } }, "request": { diff --git a/crates/jp_llm/tests/fixtures/anthropic/test_tool_call_auto__conversation_stream.snap b/crates/jp_llm/tests/fixtures/anthropic/test_tool_call_auto__conversation_stream.snap index a919b8991..9eb1d2e60 100644 --- a/crates/jp_llm/tests/fixtures/anthropic/test_tool_call_auto__conversation_stream.snap +++ b/crates/jp_llm/tests/fixtures/anthropic/test_tool_call_auto__conversation_stream.snap @@ -29,8 +29,7 @@ expression: v }, "parameters": { "reasoning": "off", - "stop_words": [], - "other": {} + "stop_words": [] } }, "request": { diff --git a/crates/jp_llm/tests/fixtures/anthropic/test_tool_call_function__conversation_stream.snap b/crates/jp_llm/tests/fixtures/anthropic/test_tool_call_function__conversation_stream.snap index 72ff7212c..7f63994ea 100644 --- a/crates/jp_llm/tests/fixtures/anthropic/test_tool_call_function__conversation_stream.snap +++ b/crates/jp_llm/tests/fixtures/anthropic/test_tool_call_function__conversation_stream.snap @@ -29,8 +29,7 @@ expression: v }, "parameters": { "reasoning": "off", - "stop_words": [], - "other": {} + "stop_words": [] } }, "request": { diff --git a/crates/jp_llm/tests/fixtures/anthropic/test_tool_call_reasoning__conversation_stream.snap b/crates/jp_llm/tests/fixtures/anthropic/test_tool_call_reasoning__conversation_stream.snap index 32d357e05..df8edd445 100644 --- a/crates/jp_llm/tests/fixtures/anthropic/test_tool_call_reasoning__conversation_stream.snap +++ b/crates/jp_llm/tests/fixtures/anthropic/test_tool_call_reasoning__conversation_stream.snap @@ -29,8 +29,7 @@ expression: v }, "parameters": { "reasoning": "off", - "stop_words": [], - "other": {} + "stop_words": [] } }, "request": { diff --git a/crates/jp_llm/tests/fixtures/anthropic/test_tool_call_required_no_reasoning__conversation_stream.snap b/crates/jp_llm/tests/fixtures/anthropic/test_tool_call_required_no_reasoning__conversation_stream.snap index bd9f48c0c..384f1dc62 100644 --- a/crates/jp_llm/tests/fixtures/anthropic/test_tool_call_required_no_reasoning__conversation_stream.snap +++ b/crates/jp_llm/tests/fixtures/anthropic/test_tool_call_required_no_reasoning__conversation_stream.snap @@ -29,8 +29,7 @@ expression: v }, "parameters": { "reasoning": "off", - "stop_words": [], - "other": {} + "stop_words": [] } }, "request": { diff --git a/crates/jp_llm/tests/fixtures/anthropic/test_tool_call_required_reasoning__conversation_stream.snap b/crates/jp_llm/tests/fixtures/anthropic/test_tool_call_required_reasoning__conversation_stream.snap index 34e813dbb..712f6c39d 100644 --- a/crates/jp_llm/tests/fixtures/anthropic/test_tool_call_required_reasoning__conversation_stream.snap +++ b/crates/jp_llm/tests/fixtures/anthropic/test_tool_call_required_reasoning__conversation_stream.snap @@ -29,8 +29,7 @@ expression: v }, "parameters": { "reasoning": "off", - "stop_words": [], - "other": {} + "stop_words": [] } }, "request": { diff --git a/crates/jp_llm/tests/fixtures/anthropic/test_tool_call_stream__conversation_stream.snap b/crates/jp_llm/tests/fixtures/anthropic/test_tool_call_stream__conversation_stream.snap index 6171735c1..88d09e21d 100644 --- a/crates/jp_llm/tests/fixtures/anthropic/test_tool_call_stream__conversation_stream.snap +++ b/crates/jp_llm/tests/fixtures/anthropic/test_tool_call_stream__conversation_stream.snap @@ -29,8 +29,7 @@ expression: v }, "parameters": { "reasoning": "off", - "stop_words": [], - "other": {} + "stop_words": [] } }, "request": { diff --git a/crates/jp_llm/tests/fixtures/cerebras/test_chat_completion_stream__conversation_stream.snap b/crates/jp_llm/tests/fixtures/cerebras/test_chat_completion_stream__conversation_stream.snap index d9da145b1..a48d34c93 100644 --- a/crates/jp_llm/tests/fixtures/cerebras/test_chat_completion_stream__conversation_stream.snap +++ b/crates/jp_llm/tests/fixtures/cerebras/test_chat_completion_stream__conversation_stream.snap @@ -32,8 +32,7 @@ expression: v "effort": "low", "exclude": false }, - "stop_words": [], - "other": {} + "stop_words": [] } }, "request": { diff --git a/crates/jp_llm/tests/fixtures/cerebras/test_multi_turn_conversation__conversation_stream.snap b/crates/jp_llm/tests/fixtures/cerebras/test_multi_turn_conversation__conversation_stream.snap index 598974b27..d807034a3 100644 --- a/crates/jp_llm/tests/fixtures/cerebras/test_multi_turn_conversation__conversation_stream.snap +++ b/crates/jp_llm/tests/fixtures/cerebras/test_multi_turn_conversation__conversation_stream.snap @@ -32,8 +32,7 @@ expression: v "effort": "low", "exclude": false }, - "stop_words": [], - "other": {} + "stop_words": [] } }, "request": { diff --git a/crates/jp_llm/tests/fixtures/cerebras/test_structured_output__conversation_stream.snap b/crates/jp_llm/tests/fixtures/cerebras/test_structured_output__conversation_stream.snap index 334e25662..508a63831 100644 --- a/crates/jp_llm/tests/fixtures/cerebras/test_structured_output__conversation_stream.snap +++ b/crates/jp_llm/tests/fixtures/cerebras/test_structured_output__conversation_stream.snap @@ -29,8 +29,7 @@ expression: v }, "parameters": { "reasoning": "off", - "stop_words": [], - "other": {} + "stop_words": [] } }, "request": { diff --git a/crates/jp_llm/tests/fixtures/cerebras/test_tool_call_auto__conversation_stream.snap b/crates/jp_llm/tests/fixtures/cerebras/test_tool_call_auto__conversation_stream.snap index 3e3aa34fe..b891b269a 100644 --- a/crates/jp_llm/tests/fixtures/cerebras/test_tool_call_auto__conversation_stream.snap +++ b/crates/jp_llm/tests/fixtures/cerebras/test_tool_call_auto__conversation_stream.snap @@ -29,8 +29,7 @@ expression: v }, "parameters": { "reasoning": "off", - "stop_words": [], - "other": {} + "stop_words": [] } }, "request": { diff --git a/crates/jp_llm/tests/fixtures/cerebras/test_tool_call_function__conversation_stream.snap b/crates/jp_llm/tests/fixtures/cerebras/test_tool_call_function__conversation_stream.snap index 69c14e851..86b49c529 100644 --- a/crates/jp_llm/tests/fixtures/cerebras/test_tool_call_function__conversation_stream.snap +++ b/crates/jp_llm/tests/fixtures/cerebras/test_tool_call_function__conversation_stream.snap @@ -29,8 +29,7 @@ expression: v }, "parameters": { "reasoning": "off", - "stop_words": [], - "other": {} + "stop_words": [] } }, "request": { diff --git a/crates/jp_llm/tests/fixtures/cerebras/test_tool_call_reasoning__conversation_stream.snap b/crates/jp_llm/tests/fixtures/cerebras/test_tool_call_reasoning__conversation_stream.snap index 3ab34203f..f4502e382 100644 --- a/crates/jp_llm/tests/fixtures/cerebras/test_tool_call_reasoning__conversation_stream.snap +++ b/crates/jp_llm/tests/fixtures/cerebras/test_tool_call_reasoning__conversation_stream.snap @@ -29,8 +29,7 @@ expression: v }, "parameters": { "reasoning": "off", - "stop_words": [], - "other": {} + "stop_words": [] } }, "request": { diff --git a/crates/jp_llm/tests/fixtures/cerebras/test_tool_call_required_no_reasoning__conversation_stream.snap b/crates/jp_llm/tests/fixtures/cerebras/test_tool_call_required_no_reasoning__conversation_stream.snap index aa61172ac..5d7558f92 100644 --- a/crates/jp_llm/tests/fixtures/cerebras/test_tool_call_required_no_reasoning__conversation_stream.snap +++ b/crates/jp_llm/tests/fixtures/cerebras/test_tool_call_required_no_reasoning__conversation_stream.snap @@ -29,8 +29,7 @@ expression: v }, "parameters": { "reasoning": "off", - "stop_words": [], - "other": {} + "stop_words": [] } }, "request": { diff --git a/crates/jp_llm/tests/fixtures/cerebras/test_tool_call_required_reasoning__conversation_stream.snap b/crates/jp_llm/tests/fixtures/cerebras/test_tool_call_required_reasoning__conversation_stream.snap index 7cadca8f4..6111e7470 100644 --- a/crates/jp_llm/tests/fixtures/cerebras/test_tool_call_required_reasoning__conversation_stream.snap +++ b/crates/jp_llm/tests/fixtures/cerebras/test_tool_call_required_reasoning__conversation_stream.snap @@ -29,8 +29,7 @@ expression: v }, "parameters": { "reasoning": "off", - "stop_words": [], - "other": {} + "stop_words": [] } }, "request": { diff --git a/crates/jp_llm/tests/fixtures/cerebras/test_tool_call_stream__conversation_stream.snap b/crates/jp_llm/tests/fixtures/cerebras/test_tool_call_stream__conversation_stream.snap index 29007aa54..1658bc5ba 100644 --- a/crates/jp_llm/tests/fixtures/cerebras/test_tool_call_stream__conversation_stream.snap +++ b/crates/jp_llm/tests/fixtures/cerebras/test_tool_call_stream__conversation_stream.snap @@ -29,8 +29,7 @@ expression: v }, "parameters": { "reasoning": "off", - "stop_words": [], - "other": {} + "stop_words": [] } }, "request": { diff --git a/crates/jp_llm/tests/fixtures/cerebras/test_unknown_model_auto_omits_effort__conversation_stream.snap b/crates/jp_llm/tests/fixtures/cerebras/test_unknown_model_auto_omits_effort__conversation_stream.snap index cc9318ef9..11c4579ad 100644 --- a/crates/jp_llm/tests/fixtures/cerebras/test_unknown_model_auto_omits_effort__conversation_stream.snap +++ b/crates/jp_llm/tests/fixtures/cerebras/test_unknown_model_auto_omits_effort__conversation_stream.snap @@ -29,8 +29,7 @@ expression: v }, "parameters": { "reasoning": "auto", - "stop_words": [], - "other": {} + "stop_words": [] } }, "request": { diff --git a/crates/jp_llm/tests/fixtures/cerebras/test_unknown_model_off_sends_none__conversation_stream.snap b/crates/jp_llm/tests/fixtures/cerebras/test_unknown_model_off_sends_none__conversation_stream.snap index 80a4b7544..21729e1d0 100644 --- a/crates/jp_llm/tests/fixtures/cerebras/test_unknown_model_off_sends_none__conversation_stream.snap +++ b/crates/jp_llm/tests/fixtures/cerebras/test_unknown_model_off_sends_none__conversation_stream.snap @@ -29,8 +29,7 @@ expression: v }, "parameters": { "reasoning": "off", - "stop_words": [], - "other": {} + "stop_words": [] } }, "request": { diff --git a/crates/jp_llm/tests/fixtures/google/test_chat_completion_stream__conversation_stream.snap b/crates/jp_llm/tests/fixtures/google/test_chat_completion_stream__conversation_stream.snap index 19e1dbb7b..c68a45405 100644 --- a/crates/jp_llm/tests/fixtures/google/test_chat_completion_stream__conversation_stream.snap +++ b/crates/jp_llm/tests/fixtures/google/test_chat_completion_stream__conversation_stream.snap @@ -32,8 +32,7 @@ expression: v "effort": "low", "exclude": false }, - "stop_words": [], - "other": {} + "stop_words": [] } }, "request": { diff --git a/crates/jp_llm/tests/fixtures/google/test_gemini_3_reasoning__conversation_stream.snap b/crates/jp_llm/tests/fixtures/google/test_gemini_3_reasoning__conversation_stream.snap index 5a7a89a9b..736daac43 100644 --- a/crates/jp_llm/tests/fixtures/google/test_gemini_3_reasoning__conversation_stream.snap +++ b/crates/jp_llm/tests/fixtures/google/test_gemini_3_reasoning__conversation_stream.snap @@ -32,8 +32,7 @@ expression: v "effort": "low", "exclude": false }, - "stop_words": [], - "other": {} + "stop_words": [] } }, "request": { diff --git a/crates/jp_llm/tests/fixtures/google/test_image_attachment__conversation_stream.snap b/crates/jp_llm/tests/fixtures/google/test_image_attachment__conversation_stream.snap index df2ef4339..488e5b697 100644 --- a/crates/jp_llm/tests/fixtures/google/test_image_attachment__conversation_stream.snap +++ b/crates/jp_llm/tests/fixtures/google/test_image_attachment__conversation_stream.snap @@ -29,8 +29,7 @@ expression: v }, "parameters": { "reasoning": "off", - "stop_words": [], - "other": {} + "stop_words": [] } }, "request": { diff --git a/crates/jp_llm/tests/fixtures/google/test_multi_turn_conversation__conversation_stream.snap b/crates/jp_llm/tests/fixtures/google/test_multi_turn_conversation__conversation_stream.snap index 58db55e4d..deaca1736 100644 --- a/crates/jp_llm/tests/fixtures/google/test_multi_turn_conversation__conversation_stream.snap +++ b/crates/jp_llm/tests/fixtures/google/test_multi_turn_conversation__conversation_stream.snap @@ -32,8 +32,7 @@ expression: v "effort": "low", "exclude": false }, - "stop_words": [], - "other": {} + "stop_words": [] } }, "request": { diff --git a/crates/jp_llm/tests/fixtures/google/test_structured_output__conversation_stream.snap b/crates/jp_llm/tests/fixtures/google/test_structured_output__conversation_stream.snap index 74afcd92a..89d9952f7 100644 --- a/crates/jp_llm/tests/fixtures/google/test_structured_output__conversation_stream.snap +++ b/crates/jp_llm/tests/fixtures/google/test_structured_output__conversation_stream.snap @@ -29,8 +29,7 @@ expression: v }, "parameters": { "reasoning": "off", - "stop_words": [], - "other": {} + "stop_words": [] } }, "request": { diff --git a/crates/jp_llm/tests/fixtures/google/test_tool_call_auto__conversation_stream.snap b/crates/jp_llm/tests/fixtures/google/test_tool_call_auto__conversation_stream.snap index 874640906..2e1628860 100644 --- a/crates/jp_llm/tests/fixtures/google/test_tool_call_auto__conversation_stream.snap +++ b/crates/jp_llm/tests/fixtures/google/test_tool_call_auto__conversation_stream.snap @@ -29,8 +29,7 @@ expression: v }, "parameters": { "reasoning": "off", - "stop_words": [], - "other": {} + "stop_words": [] } }, "request": { diff --git a/crates/jp_llm/tests/fixtures/google/test_tool_call_function__conversation_stream.snap b/crates/jp_llm/tests/fixtures/google/test_tool_call_function__conversation_stream.snap index 52486b94b..a3e9ca598 100644 --- a/crates/jp_llm/tests/fixtures/google/test_tool_call_function__conversation_stream.snap +++ b/crates/jp_llm/tests/fixtures/google/test_tool_call_function__conversation_stream.snap @@ -29,8 +29,7 @@ expression: v }, "parameters": { "reasoning": "off", - "stop_words": [], - "other": {} + "stop_words": [] } }, "request": { diff --git a/crates/jp_llm/tests/fixtures/google/test_tool_call_reasoning__conversation_stream.snap b/crates/jp_llm/tests/fixtures/google/test_tool_call_reasoning__conversation_stream.snap index 74a25b42c..4c0339f8a 100644 --- a/crates/jp_llm/tests/fixtures/google/test_tool_call_reasoning__conversation_stream.snap +++ b/crates/jp_llm/tests/fixtures/google/test_tool_call_reasoning__conversation_stream.snap @@ -29,8 +29,7 @@ expression: v }, "parameters": { "reasoning": "off", - "stop_words": [], - "other": {} + "stop_words": [] } }, "request": { diff --git a/crates/jp_llm/tests/fixtures/google/test_tool_call_required_no_reasoning__conversation_stream.snap b/crates/jp_llm/tests/fixtures/google/test_tool_call_required_no_reasoning__conversation_stream.snap index 148919cc2..442495cfd 100644 --- a/crates/jp_llm/tests/fixtures/google/test_tool_call_required_no_reasoning__conversation_stream.snap +++ b/crates/jp_llm/tests/fixtures/google/test_tool_call_required_no_reasoning__conversation_stream.snap @@ -29,8 +29,7 @@ expression: v }, "parameters": { "reasoning": "off", - "stop_words": [], - "other": {} + "stop_words": [] } }, "request": { diff --git a/crates/jp_llm/tests/fixtures/google/test_tool_call_required_reasoning__conversation_stream.snap b/crates/jp_llm/tests/fixtures/google/test_tool_call_required_reasoning__conversation_stream.snap index 7cc252b35..44e5ba8d2 100644 --- a/crates/jp_llm/tests/fixtures/google/test_tool_call_required_reasoning__conversation_stream.snap +++ b/crates/jp_llm/tests/fixtures/google/test_tool_call_required_reasoning__conversation_stream.snap @@ -29,8 +29,7 @@ expression: v }, "parameters": { "reasoning": "off", - "stop_words": [], - "other": {} + "stop_words": [] } }, "request": { diff --git a/crates/jp_llm/tests/fixtures/google/test_tool_call_stream__conversation_stream.snap b/crates/jp_llm/tests/fixtures/google/test_tool_call_stream__conversation_stream.snap index 409698506..eb1f6b2f6 100644 --- a/crates/jp_llm/tests/fixtures/google/test_tool_call_stream__conversation_stream.snap +++ b/crates/jp_llm/tests/fixtures/google/test_tool_call_stream__conversation_stream.snap @@ -29,8 +29,7 @@ expression: v }, "parameters": { "reasoning": "off", - "stop_words": [], - "other": {} + "stop_words": [] } }, "request": { diff --git a/crates/jp_llm/tests/fixtures/google/test_unknown_model_inferred_thinking_level__conversation_stream.snap b/crates/jp_llm/tests/fixtures/google/test_unknown_model_inferred_thinking_level__conversation_stream.snap index 885b7d970..b63e2f173 100644 --- a/crates/jp_llm/tests/fixtures/google/test_unknown_model_inferred_thinking_level__conversation_stream.snap +++ b/crates/jp_llm/tests/fixtures/google/test_unknown_model_inferred_thinking_level__conversation_stream.snap @@ -32,8 +32,7 @@ expression: v "effort": "high", "exclude": false }, - "stop_words": [], - "other": {} + "stop_words": [] } }, "request": { diff --git a/crates/jp_llm/tests/fixtures/llamacpp/test_chat_completion_stream__conversation_stream.snap b/crates/jp_llm/tests/fixtures/llamacpp/test_chat_completion_stream__conversation_stream.snap index d9de3fc34..31f486d86 100644 --- a/crates/jp_llm/tests/fixtures/llamacpp/test_chat_completion_stream__conversation_stream.snap +++ b/crates/jp_llm/tests/fixtures/llamacpp/test_chat_completion_stream__conversation_stream.snap @@ -32,8 +32,7 @@ expression: v "effort": "low", "exclude": false }, - "stop_words": [], - "other": {} + "stop_words": [] } }, "request": { diff --git a/crates/jp_llm/tests/fixtures/llamacpp/test_image_attachment__conversation_stream.snap b/crates/jp_llm/tests/fixtures/llamacpp/test_image_attachment__conversation_stream.snap index ac8991f9d..f75e68d04 100644 --- a/crates/jp_llm/tests/fixtures/llamacpp/test_image_attachment__conversation_stream.snap +++ b/crates/jp_llm/tests/fixtures/llamacpp/test_image_attachment__conversation_stream.snap @@ -29,8 +29,7 @@ expression: v }, "parameters": { "reasoning": "off", - "stop_words": [], - "other": {} + "stop_words": [] } }, "request": { diff --git a/crates/jp_llm/tests/fixtures/llamacpp/test_multi_turn_conversation__conversation_stream.snap b/crates/jp_llm/tests/fixtures/llamacpp/test_multi_turn_conversation__conversation_stream.snap index cd8421f82..914f96b8f 100644 --- a/crates/jp_llm/tests/fixtures/llamacpp/test_multi_turn_conversation__conversation_stream.snap +++ b/crates/jp_llm/tests/fixtures/llamacpp/test_multi_turn_conversation__conversation_stream.snap @@ -32,8 +32,7 @@ expression: v "effort": "low", "exclude": false }, - "stop_words": [], - "other": {} + "stop_words": [] } }, "request": { diff --git a/crates/jp_llm/tests/fixtures/llamacpp/test_structured_output__conversation_stream.snap b/crates/jp_llm/tests/fixtures/llamacpp/test_structured_output__conversation_stream.snap index cf54aa66e..69b2cad7c 100644 --- a/crates/jp_llm/tests/fixtures/llamacpp/test_structured_output__conversation_stream.snap +++ b/crates/jp_llm/tests/fixtures/llamacpp/test_structured_output__conversation_stream.snap @@ -29,8 +29,7 @@ expression: v }, "parameters": { "reasoning": "off", - "stop_words": [], - "other": {} + "stop_words": [] } }, "request": { diff --git a/crates/jp_llm/tests/fixtures/llamacpp/test_tool_call_auto__conversation_stream.snap b/crates/jp_llm/tests/fixtures/llamacpp/test_tool_call_auto__conversation_stream.snap index ca3f6dc9a..14858b3e8 100644 --- a/crates/jp_llm/tests/fixtures/llamacpp/test_tool_call_auto__conversation_stream.snap +++ b/crates/jp_llm/tests/fixtures/llamacpp/test_tool_call_auto__conversation_stream.snap @@ -29,8 +29,7 @@ expression: v }, "parameters": { "reasoning": "off", - "stop_words": [], - "other": {} + "stop_words": [] } }, "request": { diff --git a/crates/jp_llm/tests/fixtures/llamacpp/test_tool_call_function__conversation_stream.snap b/crates/jp_llm/tests/fixtures/llamacpp/test_tool_call_function__conversation_stream.snap index ff9fd3a24..7c58c15db 100644 --- a/crates/jp_llm/tests/fixtures/llamacpp/test_tool_call_function__conversation_stream.snap +++ b/crates/jp_llm/tests/fixtures/llamacpp/test_tool_call_function__conversation_stream.snap @@ -29,8 +29,7 @@ expression: v }, "parameters": { "reasoning": "off", - "stop_words": [], - "other": {} + "stop_words": [] } }, "request": { diff --git a/crates/jp_llm/tests/fixtures/llamacpp/test_tool_call_reasoning__conversation_stream.snap b/crates/jp_llm/tests/fixtures/llamacpp/test_tool_call_reasoning__conversation_stream.snap index 04f462d22..1e71defd4 100644 --- a/crates/jp_llm/tests/fixtures/llamacpp/test_tool_call_reasoning__conversation_stream.snap +++ b/crates/jp_llm/tests/fixtures/llamacpp/test_tool_call_reasoning__conversation_stream.snap @@ -29,8 +29,7 @@ expression: v }, "parameters": { "reasoning": "off", - "stop_words": [], - "other": {} + "stop_words": [] } }, "request": { diff --git a/crates/jp_llm/tests/fixtures/llamacpp/test_tool_call_required_no_reasoning__conversation_stream.snap b/crates/jp_llm/tests/fixtures/llamacpp/test_tool_call_required_no_reasoning__conversation_stream.snap index 470b0026b..6d9d454f7 100644 --- a/crates/jp_llm/tests/fixtures/llamacpp/test_tool_call_required_no_reasoning__conversation_stream.snap +++ b/crates/jp_llm/tests/fixtures/llamacpp/test_tool_call_required_no_reasoning__conversation_stream.snap @@ -29,8 +29,7 @@ expression: v }, "parameters": { "reasoning": "off", - "stop_words": [], - "other": {} + "stop_words": [] } }, "request": { diff --git a/crates/jp_llm/tests/fixtures/llamacpp/test_tool_call_required_reasoning__conversation_stream.snap b/crates/jp_llm/tests/fixtures/llamacpp/test_tool_call_required_reasoning__conversation_stream.snap index 42969398d..bf36a9988 100644 --- a/crates/jp_llm/tests/fixtures/llamacpp/test_tool_call_required_reasoning__conversation_stream.snap +++ b/crates/jp_llm/tests/fixtures/llamacpp/test_tool_call_required_reasoning__conversation_stream.snap @@ -29,8 +29,7 @@ expression: v }, "parameters": { "reasoning": "off", - "stop_words": [], - "other": {} + "stop_words": [] } }, "request": { diff --git a/crates/jp_llm/tests/fixtures/llamacpp/test_tool_call_stream__conversation_stream.snap b/crates/jp_llm/tests/fixtures/llamacpp/test_tool_call_stream__conversation_stream.snap index 3488b7e1d..82517ff1d 100644 --- a/crates/jp_llm/tests/fixtures/llamacpp/test_tool_call_stream__conversation_stream.snap +++ b/crates/jp_llm/tests/fixtures/llamacpp/test_tool_call_stream__conversation_stream.snap @@ -29,8 +29,7 @@ expression: v }, "parameters": { "reasoning": "off", - "stop_words": [], - "other": {} + "stop_words": [] } }, "request": { diff --git a/crates/jp_llm/tests/fixtures/ollama/test_chat_completion_stream__conversation_stream.snap b/crates/jp_llm/tests/fixtures/ollama/test_chat_completion_stream__conversation_stream.snap index 487841e52..780be5070 100644 --- a/crates/jp_llm/tests/fixtures/ollama/test_chat_completion_stream__conversation_stream.snap +++ b/crates/jp_llm/tests/fixtures/ollama/test_chat_completion_stream__conversation_stream.snap @@ -32,8 +32,7 @@ expression: v "effort": "low", "exclude": false }, - "stop_words": [], - "other": {} + "stop_words": [] } }, "request": { diff --git a/crates/jp_llm/tests/fixtures/ollama/test_image_attachment__conversation_stream.snap b/crates/jp_llm/tests/fixtures/ollama/test_image_attachment__conversation_stream.snap index 9d8c5c51f..4adf26192 100644 --- a/crates/jp_llm/tests/fixtures/ollama/test_image_attachment__conversation_stream.snap +++ b/crates/jp_llm/tests/fixtures/ollama/test_image_attachment__conversation_stream.snap @@ -29,8 +29,7 @@ expression: v }, "parameters": { "reasoning": "off", - "stop_words": [], - "other": {} + "stop_words": [] } }, "request": { diff --git a/crates/jp_llm/tests/fixtures/ollama/test_multi_turn_conversation__conversation_stream.snap b/crates/jp_llm/tests/fixtures/ollama/test_multi_turn_conversation__conversation_stream.snap index e1e032647..6034df11e 100644 --- a/crates/jp_llm/tests/fixtures/ollama/test_multi_turn_conversation__conversation_stream.snap +++ b/crates/jp_llm/tests/fixtures/ollama/test_multi_turn_conversation__conversation_stream.snap @@ -32,8 +32,7 @@ expression: v "effort": "low", "exclude": false }, - "stop_words": [], - "other": {} + "stop_words": [] } }, "request": { diff --git a/crates/jp_llm/tests/fixtures/ollama/test_structured_output__conversation_stream.snap b/crates/jp_llm/tests/fixtures/ollama/test_structured_output__conversation_stream.snap index 0cc26e497..a26d90845 100644 --- a/crates/jp_llm/tests/fixtures/ollama/test_structured_output__conversation_stream.snap +++ b/crates/jp_llm/tests/fixtures/ollama/test_structured_output__conversation_stream.snap @@ -29,8 +29,7 @@ expression: v }, "parameters": { "reasoning": "off", - "stop_words": [], - "other": {} + "stop_words": [] } }, "request": { diff --git a/crates/jp_llm/tests/fixtures/ollama/test_tool_call_auto__conversation_stream.snap b/crates/jp_llm/tests/fixtures/ollama/test_tool_call_auto__conversation_stream.snap index 85f47ce8f..4867357e3 100644 --- a/crates/jp_llm/tests/fixtures/ollama/test_tool_call_auto__conversation_stream.snap +++ b/crates/jp_llm/tests/fixtures/ollama/test_tool_call_auto__conversation_stream.snap @@ -29,8 +29,7 @@ expression: v }, "parameters": { "reasoning": "off", - "stop_words": [], - "other": {} + "stop_words": [] } }, "request": { diff --git a/crates/jp_llm/tests/fixtures/ollama/test_tool_call_function__conversation_stream.snap b/crates/jp_llm/tests/fixtures/ollama/test_tool_call_function__conversation_stream.snap index 9848db462..42012d126 100644 --- a/crates/jp_llm/tests/fixtures/ollama/test_tool_call_function__conversation_stream.snap +++ b/crates/jp_llm/tests/fixtures/ollama/test_tool_call_function__conversation_stream.snap @@ -29,8 +29,7 @@ expression: v }, "parameters": { "reasoning": "off", - "stop_words": [], - "other": {} + "stop_words": [] } }, "request": { diff --git a/crates/jp_llm/tests/fixtures/ollama/test_tool_call_reasoning__conversation_stream.snap b/crates/jp_llm/tests/fixtures/ollama/test_tool_call_reasoning__conversation_stream.snap index 8541f9c9b..4ec7fac72 100644 --- a/crates/jp_llm/tests/fixtures/ollama/test_tool_call_reasoning__conversation_stream.snap +++ b/crates/jp_llm/tests/fixtures/ollama/test_tool_call_reasoning__conversation_stream.snap @@ -29,8 +29,7 @@ expression: v }, "parameters": { "reasoning": "off", - "stop_words": [], - "other": {} + "stop_words": [] } }, "request": { diff --git a/crates/jp_llm/tests/fixtures/ollama/test_tool_call_required_no_reasoning__conversation_stream.snap b/crates/jp_llm/tests/fixtures/ollama/test_tool_call_required_no_reasoning__conversation_stream.snap index 8cfa8d508..7d417e4b4 100644 --- a/crates/jp_llm/tests/fixtures/ollama/test_tool_call_required_no_reasoning__conversation_stream.snap +++ b/crates/jp_llm/tests/fixtures/ollama/test_tool_call_required_no_reasoning__conversation_stream.snap @@ -29,8 +29,7 @@ expression: v }, "parameters": { "reasoning": "off", - "stop_words": [], - "other": {} + "stop_words": [] } }, "request": { diff --git a/crates/jp_llm/tests/fixtures/ollama/test_tool_call_required_reasoning__conversation_stream.snap b/crates/jp_llm/tests/fixtures/ollama/test_tool_call_required_reasoning__conversation_stream.snap index 8f9e87349..11ef56200 100644 --- a/crates/jp_llm/tests/fixtures/ollama/test_tool_call_required_reasoning__conversation_stream.snap +++ b/crates/jp_llm/tests/fixtures/ollama/test_tool_call_required_reasoning__conversation_stream.snap @@ -29,8 +29,7 @@ expression: v }, "parameters": { "reasoning": "off", - "stop_words": [], - "other": {} + "stop_words": [] } }, "request": { diff --git a/crates/jp_llm/tests/fixtures/ollama/test_tool_call_stream__conversation_stream.snap b/crates/jp_llm/tests/fixtures/ollama/test_tool_call_stream__conversation_stream.snap index 8d4cf85e8..9aa59033c 100644 --- a/crates/jp_llm/tests/fixtures/ollama/test_tool_call_stream__conversation_stream.snap +++ b/crates/jp_llm/tests/fixtures/ollama/test_tool_call_stream__conversation_stream.snap @@ -29,8 +29,7 @@ expression: v }, "parameters": { "reasoning": "off", - "stop_words": [], - "other": {} + "stop_words": [] } }, "request": { diff --git a/crates/jp_llm/tests/fixtures/openai/test_chat_completion_stream__conversation_stream.snap b/crates/jp_llm/tests/fixtures/openai/test_chat_completion_stream__conversation_stream.snap index 4e64d467c..5b81f8bb9 100644 --- a/crates/jp_llm/tests/fixtures/openai/test_chat_completion_stream__conversation_stream.snap +++ b/crates/jp_llm/tests/fixtures/openai/test_chat_completion_stream__conversation_stream.snap @@ -32,8 +32,7 @@ expression: v "effort": "low", "exclude": false }, - "stop_words": [], - "other": {} + "stop_words": [] } }, "request": { diff --git a/crates/jp_llm/tests/fixtures/openai/test_gpt_5_6_cache_off_sends_explicit_optout__conversation_stream.snap b/crates/jp_llm/tests/fixtures/openai/test_gpt_5_6_cache_off_sends_explicit_optout__conversation_stream.snap index 36975472e..dd2581a7e 100644 --- a/crates/jp_llm/tests/fixtures/openai/test_gpt_5_6_cache_off_sends_explicit_optout__conversation_stream.snap +++ b/crates/jp_llm/tests/fixtures/openai/test_gpt_5_6_cache_off_sends_explicit_optout__conversation_stream.snap @@ -32,8 +32,7 @@ expression: v "effort": "low", "exclude": false }, - "stop_words": [], - "other": {} + "stop_words": [] } }, "request": { diff --git a/crates/jp_llm/tests/fixtures/openai/test_gpt_5_6_pro_reasoning_and_explicit_caching__conversation_stream.snap b/crates/jp_llm/tests/fixtures/openai/test_gpt_5_6_pro_reasoning_and_explicit_caching__conversation_stream.snap index 8d58ef14a..00c365278 100644 --- a/crates/jp_llm/tests/fixtures/openai/test_gpt_5_6_pro_reasoning_and_explicit_caching__conversation_stream.snap +++ b/crates/jp_llm/tests/fixtures/openai/test_gpt_5_6_pro_reasoning_and_explicit_caching__conversation_stream.snap @@ -33,9 +33,7 @@ expression: v "exclude": false }, "stop_words": [], - "other": { - "reasoning_mode": "pro" - } + "reasoning_mode": "pro" } }, "request": { diff --git a/crates/jp_llm/tests/fixtures/openai/test_gpt_5_6_prompt_cache_read_after_write__conversation_stream.snap b/crates/jp_llm/tests/fixtures/openai/test_gpt_5_6_prompt_cache_read_after_write__conversation_stream.snap index 389a47508..2fcabc42c 100644 --- a/crates/jp_llm/tests/fixtures/openai/test_gpt_5_6_prompt_cache_read_after_write__conversation_stream.snap +++ b/crates/jp_llm/tests/fixtures/openai/test_gpt_5_6_prompt_cache_read_after_write__conversation_stream.snap @@ -29,8 +29,7 @@ expression: v }, "parameters": { "reasoning": "off", - "stop_words": [], - "other": {} + "stop_words": [] } }, "request": { diff --git a/crates/jp_llm/tests/fixtures/openai/test_image_attachment__conversation_stream.snap b/crates/jp_llm/tests/fixtures/openai/test_image_attachment__conversation_stream.snap index 508e9baac..8c01c530f 100644 --- a/crates/jp_llm/tests/fixtures/openai/test_image_attachment__conversation_stream.snap +++ b/crates/jp_llm/tests/fixtures/openai/test_image_attachment__conversation_stream.snap @@ -29,8 +29,7 @@ expression: v }, "parameters": { "reasoning": "off", - "stop_words": [], - "other": {} + "stop_words": [] } }, "request": { diff --git a/crates/jp_llm/tests/fixtures/openai/test_multi_turn_conversation__conversation_stream.snap b/crates/jp_llm/tests/fixtures/openai/test_multi_turn_conversation__conversation_stream.snap index c993c40bc..8b3b7828e 100644 --- a/crates/jp_llm/tests/fixtures/openai/test_multi_turn_conversation__conversation_stream.snap +++ b/crates/jp_llm/tests/fixtures/openai/test_multi_turn_conversation__conversation_stream.snap @@ -32,8 +32,7 @@ expression: v "effort": "low", "exclude": false }, - "stop_words": [], - "other": {} + "stop_words": [] } }, "request": { diff --git a/crates/jp_llm/tests/fixtures/openai/test_reasoning_history_replayed_to_reasoning_unsupported_model__conversation_stream.snap b/crates/jp_llm/tests/fixtures/openai/test_reasoning_history_replayed_to_reasoning_unsupported_model__conversation_stream.snap index b51e696e5..1c778907c 100644 --- a/crates/jp_llm/tests/fixtures/openai/test_reasoning_history_replayed_to_reasoning_unsupported_model__conversation_stream.snap +++ b/crates/jp_llm/tests/fixtures/openai/test_reasoning_history_replayed_to_reasoning_unsupported_model__conversation_stream.snap @@ -29,8 +29,7 @@ expression: v }, "parameters": { "reasoning": "off", - "stop_words": [], - "other": {} + "stop_words": [] } }, "request": { diff --git a/crates/jp_llm/tests/fixtures/openai/test_structured_output__conversation_stream.snap b/crates/jp_llm/tests/fixtures/openai/test_structured_output__conversation_stream.snap index 8101c358d..ab93da729 100644 --- a/crates/jp_llm/tests/fixtures/openai/test_structured_output__conversation_stream.snap +++ b/crates/jp_llm/tests/fixtures/openai/test_structured_output__conversation_stream.snap @@ -29,8 +29,7 @@ expression: v }, "parameters": { "reasoning": "off", - "stop_words": [], - "other": {} + "stop_words": [] } }, "request": { diff --git a/crates/jp_llm/tests/fixtures/openai/test_tool_call_auto__conversation_stream.snap b/crates/jp_llm/tests/fixtures/openai/test_tool_call_auto__conversation_stream.snap index 166deed47..d6f9d86a6 100644 --- a/crates/jp_llm/tests/fixtures/openai/test_tool_call_auto__conversation_stream.snap +++ b/crates/jp_llm/tests/fixtures/openai/test_tool_call_auto__conversation_stream.snap @@ -29,8 +29,7 @@ expression: v }, "parameters": { "reasoning": "off", - "stop_words": [], - "other": {} + "stop_words": [] } }, "request": { diff --git a/crates/jp_llm/tests/fixtures/openai/test_tool_call_function__conversation_stream.snap b/crates/jp_llm/tests/fixtures/openai/test_tool_call_function__conversation_stream.snap index f2ece97d8..7974cfbe7 100644 --- a/crates/jp_llm/tests/fixtures/openai/test_tool_call_function__conversation_stream.snap +++ b/crates/jp_llm/tests/fixtures/openai/test_tool_call_function__conversation_stream.snap @@ -29,8 +29,7 @@ expression: v }, "parameters": { "reasoning": "off", - "stop_words": [], - "other": {} + "stop_words": [] } }, "request": { diff --git a/crates/jp_llm/tests/fixtures/openai/test_tool_call_reasoning__conversation_stream.snap b/crates/jp_llm/tests/fixtures/openai/test_tool_call_reasoning__conversation_stream.snap index 4ba7267d3..6345cf600 100644 --- a/crates/jp_llm/tests/fixtures/openai/test_tool_call_reasoning__conversation_stream.snap +++ b/crates/jp_llm/tests/fixtures/openai/test_tool_call_reasoning__conversation_stream.snap @@ -29,8 +29,7 @@ expression: v }, "parameters": { "reasoning": "off", - "stop_words": [], - "other": {} + "stop_words": [] } }, "request": { diff --git a/crates/jp_llm/tests/fixtures/openai/test_tool_call_required_no_reasoning__conversation_stream.snap b/crates/jp_llm/tests/fixtures/openai/test_tool_call_required_no_reasoning__conversation_stream.snap index 7252a1cec..851496e48 100644 --- a/crates/jp_llm/tests/fixtures/openai/test_tool_call_required_no_reasoning__conversation_stream.snap +++ b/crates/jp_llm/tests/fixtures/openai/test_tool_call_required_no_reasoning__conversation_stream.snap @@ -29,8 +29,7 @@ expression: v }, "parameters": { "reasoning": "off", - "stop_words": [], - "other": {} + "stop_words": [] } }, "request": { diff --git a/crates/jp_llm/tests/fixtures/openai/test_tool_call_required_reasoning__conversation_stream.snap b/crates/jp_llm/tests/fixtures/openai/test_tool_call_required_reasoning__conversation_stream.snap index 5937f1fce..a0bc24d49 100644 --- a/crates/jp_llm/tests/fixtures/openai/test_tool_call_required_reasoning__conversation_stream.snap +++ b/crates/jp_llm/tests/fixtures/openai/test_tool_call_required_reasoning__conversation_stream.snap @@ -29,8 +29,7 @@ expression: v }, "parameters": { "reasoning": "off", - "stop_words": [], - "other": {} + "stop_words": [] } }, "request": { diff --git a/crates/jp_llm/tests/fixtures/openai/test_tool_call_stream__conversation_stream.snap b/crates/jp_llm/tests/fixtures/openai/test_tool_call_stream__conversation_stream.snap index c0bd1a230..43f756e0c 100644 --- a/crates/jp_llm/tests/fixtures/openai/test_tool_call_stream__conversation_stream.snap +++ b/crates/jp_llm/tests/fixtures/openai/test_tool_call_stream__conversation_stream.snap @@ -29,8 +29,7 @@ expression: v }, "parameters": { "reasoning": "off", - "stop_words": [], - "other": {} + "stop_words": [] } }, "request": { diff --git a/crates/jp_llm/tests/fixtures/openrouter/anthropic_test_sub_provider_event_metadata__conversation_stream.snap b/crates/jp_llm/tests/fixtures/openrouter/anthropic_test_sub_provider_event_metadata__conversation_stream.snap index f659bf785..76133ae5a 100644 --- a/crates/jp_llm/tests/fixtures/openrouter/anthropic_test_sub_provider_event_metadata__conversation_stream.snap +++ b/crates/jp_llm/tests/fixtures/openrouter/anthropic_test_sub_provider_event_metadata__conversation_stream.snap @@ -32,8 +32,7 @@ expression: v "effort": "low", "exclude": false }, - "stop_words": [], - "other": {} + "stop_words": [] } }, "request": { diff --git a/crates/jp_llm/tests/fixtures/openrouter/google_test_sub_provider_event_metadata__conversation_stream.snap b/crates/jp_llm/tests/fixtures/openrouter/google_test_sub_provider_event_metadata__conversation_stream.snap index f6fdf7319..c426889ba 100644 --- a/crates/jp_llm/tests/fixtures/openrouter/google_test_sub_provider_event_metadata__conversation_stream.snap +++ b/crates/jp_llm/tests/fixtures/openrouter/google_test_sub_provider_event_metadata__conversation_stream.snap @@ -32,8 +32,7 @@ expression: v "effort": "low", "exclude": false }, - "stop_words": [], - "other": {} + "stop_words": [] } }, "request": { diff --git a/crates/jp_llm/tests/fixtures/openrouter/minimax_test_sub_provider_event_metadata__conversation_stream.snap b/crates/jp_llm/tests/fixtures/openrouter/minimax_test_sub_provider_event_metadata__conversation_stream.snap index 3c7223e5c..2b8c14b7e 100644 --- a/crates/jp_llm/tests/fixtures/openrouter/minimax_test_sub_provider_event_metadata__conversation_stream.snap +++ b/crates/jp_llm/tests/fixtures/openrouter/minimax_test_sub_provider_event_metadata__conversation_stream.snap @@ -32,8 +32,7 @@ expression: v "effort": "low", "exclude": false }, - "stop_words": [], - "other": {} + "stop_words": [] } }, "request": { diff --git a/crates/jp_llm/tests/fixtures/openrouter/test_anthropic_opus_5_parallel_tool_round_trip__conversation_stream.snap b/crates/jp_llm/tests/fixtures/openrouter/test_anthropic_opus_5_parallel_tool_round_trip__conversation_stream.snap index a5609ff09..3bb600f23 100644 --- a/crates/jp_llm/tests/fixtures/openrouter/test_anthropic_opus_5_parallel_tool_round_trip__conversation_stream.snap +++ b/crates/jp_llm/tests/fixtures/openrouter/test_anthropic_opus_5_parallel_tool_round_trip__conversation_stream.snap @@ -32,8 +32,7 @@ expression: v "effort": "low", "exclude": false }, - "stop_words": [], - "other": {} + "stop_words": [] } }, "request": { diff --git a/crates/jp_llm/tests/fixtures/openrouter/test_chat_completion_stream__conversation_stream.snap b/crates/jp_llm/tests/fixtures/openrouter/test_chat_completion_stream__conversation_stream.snap index d13b5abe0..766ab2b40 100644 --- a/crates/jp_llm/tests/fixtures/openrouter/test_chat_completion_stream__conversation_stream.snap +++ b/crates/jp_llm/tests/fixtures/openrouter/test_chat_completion_stream__conversation_stream.snap @@ -32,8 +32,7 @@ expression: v "effort": "low", "exclude": false }, - "stop_words": [], - "other": {} + "stop_words": [] } }, "request": { diff --git a/crates/jp_llm/tests/fixtures/openrouter/test_image_attachment__conversation_stream.snap b/crates/jp_llm/tests/fixtures/openrouter/test_image_attachment__conversation_stream.snap index ae32f7783..e428ef8e5 100644 --- a/crates/jp_llm/tests/fixtures/openrouter/test_image_attachment__conversation_stream.snap +++ b/crates/jp_llm/tests/fixtures/openrouter/test_image_attachment__conversation_stream.snap @@ -29,8 +29,7 @@ expression: v }, "parameters": { "reasoning": "off", - "stop_words": [], - "other": {} + "stop_words": [] } }, "request": { diff --git a/crates/jp_llm/tests/fixtures/openrouter/test_multi_turn_conversation__conversation_stream.snap b/crates/jp_llm/tests/fixtures/openrouter/test_multi_turn_conversation__conversation_stream.snap index 811fd33ea..961482276 100644 --- a/crates/jp_llm/tests/fixtures/openrouter/test_multi_turn_conversation__conversation_stream.snap +++ b/crates/jp_llm/tests/fixtures/openrouter/test_multi_turn_conversation__conversation_stream.snap @@ -32,8 +32,7 @@ expression: v "effort": "low", "exclude": false }, - "stop_words": [], - "other": {} + "stop_words": [] } }, "request": { diff --git a/crates/jp_llm/tests/fixtures/openrouter/test_structured_output__conversation_stream.snap b/crates/jp_llm/tests/fixtures/openrouter/test_structured_output__conversation_stream.snap index bd98fe204..934f56c16 100644 --- a/crates/jp_llm/tests/fixtures/openrouter/test_structured_output__conversation_stream.snap +++ b/crates/jp_llm/tests/fixtures/openrouter/test_structured_output__conversation_stream.snap @@ -29,8 +29,7 @@ expression: v }, "parameters": { "reasoning": "off", - "stop_words": [], - "other": {} + "stop_words": [] } }, "request": { diff --git a/crates/jp_llm/tests/fixtures/openrouter/test_tool_call_auto__conversation_stream.snap b/crates/jp_llm/tests/fixtures/openrouter/test_tool_call_auto__conversation_stream.snap index 42d458ce3..c16440589 100644 --- a/crates/jp_llm/tests/fixtures/openrouter/test_tool_call_auto__conversation_stream.snap +++ b/crates/jp_llm/tests/fixtures/openrouter/test_tool_call_auto__conversation_stream.snap @@ -29,8 +29,7 @@ expression: v }, "parameters": { "reasoning": "off", - "stop_words": [], - "other": {} + "stop_words": [] } }, "request": { diff --git a/crates/jp_llm/tests/fixtures/openrouter/test_tool_call_function__conversation_stream.snap b/crates/jp_llm/tests/fixtures/openrouter/test_tool_call_function__conversation_stream.snap index d333e7530..ff089c794 100644 --- a/crates/jp_llm/tests/fixtures/openrouter/test_tool_call_function__conversation_stream.snap +++ b/crates/jp_llm/tests/fixtures/openrouter/test_tool_call_function__conversation_stream.snap @@ -29,8 +29,7 @@ expression: v }, "parameters": { "reasoning": "off", - "stop_words": [], - "other": {} + "stop_words": [] } }, "request": { diff --git a/crates/jp_llm/tests/fixtures/openrouter/test_tool_call_reasoning__conversation_stream.snap b/crates/jp_llm/tests/fixtures/openrouter/test_tool_call_reasoning__conversation_stream.snap index cec9fcc45..c292d2e27 100644 --- a/crates/jp_llm/tests/fixtures/openrouter/test_tool_call_reasoning__conversation_stream.snap +++ b/crates/jp_llm/tests/fixtures/openrouter/test_tool_call_reasoning__conversation_stream.snap @@ -29,8 +29,7 @@ expression: v }, "parameters": { "reasoning": "off", - "stop_words": [], - "other": {} + "stop_words": [] } }, "request": { diff --git a/crates/jp_llm/tests/fixtures/openrouter/test_tool_call_required_no_reasoning__conversation_stream.snap b/crates/jp_llm/tests/fixtures/openrouter/test_tool_call_required_no_reasoning__conversation_stream.snap index e3d95e066..5bf81adb3 100644 --- a/crates/jp_llm/tests/fixtures/openrouter/test_tool_call_required_no_reasoning__conversation_stream.snap +++ b/crates/jp_llm/tests/fixtures/openrouter/test_tool_call_required_no_reasoning__conversation_stream.snap @@ -29,8 +29,7 @@ expression: v }, "parameters": { "reasoning": "off", - "stop_words": [], - "other": {} + "stop_words": [] } }, "request": { diff --git a/crates/jp_llm/tests/fixtures/openrouter/test_tool_call_required_reasoning__conversation_stream.snap b/crates/jp_llm/tests/fixtures/openrouter/test_tool_call_required_reasoning__conversation_stream.snap index c94234e7a..1a1472eb0 100644 --- a/crates/jp_llm/tests/fixtures/openrouter/test_tool_call_required_reasoning__conversation_stream.snap +++ b/crates/jp_llm/tests/fixtures/openrouter/test_tool_call_required_reasoning__conversation_stream.snap @@ -29,8 +29,7 @@ expression: v }, "parameters": { "reasoning": "off", - "stop_words": [], - "other": {} + "stop_words": [] } }, "request": { diff --git a/crates/jp_llm/tests/fixtures/openrouter/test_tool_call_stream__conversation_stream.snap b/crates/jp_llm/tests/fixtures/openrouter/test_tool_call_stream__conversation_stream.snap index ebec0ebbf..ddfc629a0 100644 --- a/crates/jp_llm/tests/fixtures/openrouter/test_tool_call_stream__conversation_stream.snap +++ b/crates/jp_llm/tests/fixtures/openrouter/test_tool_call_stream__conversation_stream.snap @@ -29,8 +29,7 @@ expression: v }, "parameters": { "reasoning": "off", - "stop_words": [], - "other": {} + "stop_words": [] } }, "request": { diff --git a/crates/jp_llm/tests/fixtures/openrouter/x-ai_test_sub_provider_event_metadata__conversation_stream.snap b/crates/jp_llm/tests/fixtures/openrouter/x-ai_test_sub_provider_event_metadata__conversation_stream.snap index 5f6e0864d..3451eed28 100644 --- a/crates/jp_llm/tests/fixtures/openrouter/x-ai_test_sub_provider_event_metadata__conversation_stream.snap +++ b/crates/jp_llm/tests/fixtures/openrouter/x-ai_test_sub_provider_event_metadata__conversation_stream.snap @@ -32,8 +32,7 @@ expression: v "effort": "low", "exclude": false }, - "stop_words": [], - "other": {} + "stop_words": [] } }, "request": {