diff --git a/Cargo.lock b/Cargo.lock index dee7a68a..a9062819 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1052,6 +1052,12 @@ version = "1.6.2" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "abd57806937c9cc163efc8ea3910e00a62e2aeb0b8119f1793a978088f8f6b04" +[[package]] +name = "diff" +version = "0.1.13" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "56254986775e3233ffa9c4d7d3faaf6d36a2c09d30b20687e9f88bc8bafc16c8" + [[package]] name = "digest" version = "0.10.7" @@ -2159,11 +2165,13 @@ dependencies = [ "confique", "directories", "indexmap", + "indoc", "insta", "jp_mcp", "jp_model", "json5", "path-clean", + "pretty_assertions", "quick-xml", "serde", "serde_json", @@ -2173,7 +2181,6 @@ dependencies = [ "test-log", "thiserror 2.0.12", "toml 0.9.2", - "toml_edit 0.23.2", "tracing", "url", ] @@ -3150,6 +3157,16 @@ dependencies = [ "zerocopy", ] +[[package]] +name = "pretty_assertions" +version = "1.4.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "3ae130e2f271fbc2ac3a40fb1d07180839cdbbe443c7a27e1e3c13c5cac0116d" +dependencies = [ + "diff", + "yansi", +] + [[package]] name = "proc-macro2" version = "1.0.95" @@ -4461,7 +4478,7 @@ dependencies = [ "serde", "serde_spanned 0.6.9", "toml_datetime 0.6.11", - "toml_edit 0.22.27", + "toml_edit", ] [[package]] @@ -4470,6 +4487,7 @@ version = "0.9.2" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "ed0aee96c12fa71097902e0bb061a5e1ebd766a6636bb605ba401c45c1650eac" dependencies = [ + "indexmap", "serde", "serde_spanned 1.0.0", "toml_datetime 0.7.0", @@ -4510,21 +4528,6 @@ dependencies = [ "winnow", ] -[[package]] -name = "toml_edit" -version = "0.23.2" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "d1dee9dc43ac2aaf7d3b774e2fba5148212bf2bd9374f4e50152ebe9afd03d42" -dependencies = [ - "indexmap", - "serde", - "serde_spanned 1.0.0", - "toml_datetime 0.7.0", - "toml_parser", - "toml_writer", - "winnow", -] - [[package]] name = "toml_parser" version = "1.0.1" @@ -5431,6 +5434,12 @@ version = "1.0.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "4de5f056fb9dc8b7908754867544e26145767187aaac5a98495e88ad7cb8a80f" +[[package]] +name = "yansi" +version = "1.0.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "cfe53a6657fd280eaa890a3bc59152892ffa3e30101319d168b781ed6529b049" + [[package]] name = "yoke" version = "0.8.0" diff --git a/Cargo.toml b/Cargo.toml index 9c925a25..cc451410 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -63,6 +63,7 @@ openai = { git = "https://github.com/JeanMertz/openai", branch = "tmp", default- openai_responses = { git = "https://github.com/JeanMertz/openai-responses-rs", default-features = false } # path-clean = { version = "1", default-features = false } percent-encoding = { version = "2", default-features = false } +pretty_assertions = { version = "1", default-features = false } quick-xml = { version = "0.38", default-features = false } reqwest = { version = "0.12", default-features = false } reqwest-eventsource = { version = "0.6", default-features = false } @@ -87,7 +88,6 @@ timeago = { version = "0.5", default-features = false } tokio = { version = "1", default-features = false, features = ["full"] } tokio-util = { version = "0.7", default-features = false } toml = { version = "0.9", default-features = false } -toml_edit = { version = "0.23", default-features = false } tracing = { version = "0.1", default-features = false } tracing-subscriber = { version = "0.3", default-features = false } typetag = { version = "0.2", default-features = false } diff --git a/crates/jp_cli/Cargo.toml b/crates/jp_cli/Cargo.toml index 92819594..a757dce8 100644 --- a/crates/jp_cli/Cargo.toml +++ b/crates/jp_cli/Cargo.toml @@ -67,7 +67,7 @@ thiserror = { workspace = true } time = { workspace = true, features = ["local-offset"] } timeago = { workspace = true } tokio = { workspace = true } -toml = { workspace = true } +toml = { workspace = true, features = ["preserve_order"] } tracing = { workspace = true } tracing-subscriber = { workspace = true, features = [ "ansi", diff --git a/crates/jp_cli/src/cmd/query.rs b/crates/jp_cli/src/cmd/query.rs index 5f43ffb4..0634830a 100644 --- a/crates/jp_cli/src/cmd/query.rs +++ b/crates/jp_cli/src/cmd/query.rs @@ -16,7 +16,12 @@ use futures::StreamExt as _; use jp_config::{ assignment::{AssignKeyValue as _, KvAssignment}, assistant::Instructions, - expand_tilde, PartialConfig, + expand_tilde, + mcp::{ + server::{Server, ToolId}, + ServerId, + }, + PartialConfig, }; use jp_conversation::{ thread::{Thread, ThreadBuilder}, @@ -834,12 +839,24 @@ async fn list_enabled_tools(ctx: &Ctx) -> Result> { for tool in all_tools { let tool_id = McpToolId::new(&*tool.name); let server_id = ctx.mcp_client.get_tool_server_id(&tool_id).await?; - let server_cfg = ctx.config.mcp.get_server_with_defaults(server_id.as_str()); + let server_defaults = Server::default(); + let server_cfg = ctx + .config + .mcp + .servers + .get(&ServerId::new(server_id.as_str())) + .unwrap_or(&server_defaults); + if !server_cfg.enable { continue; } - let tool_cfg = server_cfg.get_tool_with_defaults(&tool.name); + let tool_defaults = jp_config::mcp::server::tool::Tool::default(); + let tool_cfg = server_cfg + .tools + .get(&ToolId::new(tool.name.as_ref())) + .unwrap_or(&tool_defaults); + if !tool_cfg.enable { continue; } diff --git a/crates/jp_cli/src/cmd/query/event.rs b/crates/jp_cli/src/cmd/query/event.rs index afb0930b..91dac97a 100644 --- a/crates/jp_cli/src/cmd/query/event.rs +++ b/crates/jp_cli/src/cmd/query/event.rs @@ -9,8 +9,12 @@ use hex::ToHex as _; use inquire::Confirm; use jp_config::{ mcp::{ - server::{checksum::Algorithm, tool}, - tool_call, + server::{ + checksum::Algorithm, + tool::{self, Tool}, + Server, ToolId, + }, + tool_call, ServerId, }, style::LinkStyle, }; @@ -110,8 +114,19 @@ async fn build_tool_call_result( ) -> Result, Error> { let tool_id = McpToolId::new(&call.name); let server_id = ctx.mcp_client.get_tool_server_id(&tool_id).await?; - let server_cfg = ctx.config.mcp.get_server_with_defaults(server_id.as_str()); - let tool_cfg = server_cfg.get_tool_with_defaults(&call.name); + let server_defaults = Server::default(); + let server_cfg = ctx + .config + .mcp + .servers + .get(&ServerId::new(server_id.as_str())) + .unwrap_or(&server_defaults); + + let tool_defaults = Tool::default(); + let tool_cfg = server_cfg + .tools + .get(&ToolId::new(&call.name)) + .unwrap_or(&tool_defaults); let mut lines = result.content.trim().lines().collect::>(); let ext = lines @@ -203,8 +218,19 @@ async fn call_tool(ctx: &Ctx, mut call: ToolCallRequest) -> Result toml_edit::ser::to_string_pretty(&value)?, + Format::Toml => toml::ser::to_string_pretty(&value)?, Format::Json => serde_json::to_string_pretty(&value)?, Format::Json5 => json5::to_string(&value)?, Format::Yaml => serde_yaml::to_string(&value)?, diff --git a/crates/jp_config/src/map.rs b/crates/jp_config/src/map.rs index 4181e1cf..96670d40 100644 --- a/crates/jp_config/src/map.rs +++ b/crates/jp_config/src/map.rs @@ -158,6 +158,34 @@ where } } +impl FromIterator<(K, V)> for ConfigMapPartial +where + K: Hash + Eq, + V: Partial, +{ + /// Create a `ConfigMapPartial` from the sequence of key-value pairs in the + /// iterable. + /// + /// `from_iter` uses the same logic as `extend`. See + /// [`extend`][IndexMap::extend] for more details. + fn from_iter>(iterable: I) -> Self { + let iter = iterable.into_iter(); + let (low, _) = iter.size_hint(); + let mut map = IndexMap::with_capacity_and_hasher(low, <_>::default()); + map.extend(iter); + Self(map) + } +} + +impl IntoIterator for ConfigMapPartial { + type Item = (K, V); + type IntoIter = indexmap::map::IntoIter; + + fn into_iter(self) -> Self::IntoIter { + self.0.into_iter() + } +} + // See: struct ConfigMapVisitor { marker: PhantomData ConfigMap>, diff --git a/crates/jp_config/src/mcp.rs b/crates/jp_config/src/mcp.rs index 4e6c628f..9031700f 100644 --- a/crates/jp_config/src/mcp.rs +++ b/crates/jp_config/src/mcp.rs @@ -14,6 +14,7 @@ use server::{Server, ServerPartial}; use crate::{ assignment::{set_error, AssignKeyValue, KvAssignment}, map::{ConfigKey, ConfigMap, ConfigMapPartial}, + mcp::server::ToolId, Error, }; @@ -55,6 +56,18 @@ impl AssignKeyValue for ::Partial { #[derive(Debug, Clone, PartialEq, Eq, Hash, Serialize, Deserialize)] pub struct ServerId(String); +impl ServerId { + #[must_use] + pub fn new(id: impl Into) -> Self { + Self(id.into()) + } + + #[must_use] + pub fn as_str(&self) -> &str { + &self.0 + } +} + impl Deref for ServerId { type Target = str; @@ -123,9 +136,22 @@ impl Partial for McpPartial { } fn with_fallback(self, fallback: Self) -> Self { - Self { - servers: self.servers.with_fallback(fallback.servers), - } + let servers = self + .merge_servers_with_inheritance(&fallback) + .into_iter() + .filter(|(k, _)| k != &ServerId::new("*")) + .map(|(k, mut v)| { + v.tools = v + .tools + .into_iter() + .filter(|(k, _)| k != &ToolId::new("*")) + .collect(); + + (k, v) + }) + .collect(); + + Self { servers } } fn is_empty(&self) -> bool { @@ -137,404 +163,529 @@ impl Partial for McpPartial { } } -impl Mcp { - /// Get a server by ID. - /// - /// This handles fetching defaults from any `*` server/tool as well. +impl McpPartial { + /// Get the server with the given ID, or an empty server if it does not + /// exist. #[must_use] - pub fn get_server_with_defaults(&self, id: &str) -> Server { - let global_id = ServerId(String::from("*")); - let id = ServerId(id.to_owned()); - - let defaults = self.servers.get(&global_id).cloned().unwrap_or_default(); - let server = self.servers.get(&id); - - Server { - enable: server - .filter(|s| s.enable != defaults.enable) - .unwrap_or(&defaults) - .enable, - - binary_checksum: server - .filter(|s| s.binary_checksum != defaults.binary_checksum) - .unwrap_or(&defaults) - .binary_checksum - .clone(), - - tools: server - .filter(|s| s.tools != defaults.tools) - .unwrap_or(&defaults) - .tools - .clone(), + fn get_server_or_empty(&self, server_id: &ServerId) -> ServerPartial { + self.servers + .get(server_id) + .cloned() + .unwrap_or(ServerPartial::empty()) + } + + /// Merge the servers of this configuration with the ones of the given + /// configuration, using nested inheritance. + /// + /// Servers inherit their configuration from the global (`*`) server, if the + /// server itself does not have a configuration for a given field. + /// + /// Tools have a more complex inheritance scheme: + /// + /// ```markdown,ignore + /// 1. server .servers.my_server.tools.my_tool + /// 2. fallback.servers.my_server.tools.my_tool + /// 3. server .servers.my_server.tools.* + /// 4. fallback.servers.my_server.tools.* + /// 5. server .servers.* .tools.my_tool + /// 6. fallback.servers.* .tools.my_tool + /// 7. server .servers.* .tools.* + /// 8. fallback.servers.* .tools.* + /// ``` + /// + /// Both `self` and `other` are merged together in the correct order. + fn merge_servers_with_inheritance( + &self, + other: &Self, + ) -> ConfigMapPartial { + // Iterate over all server IDs. + let ids = self.servers.keys().chain(other.servers.keys()); + + let mut result = ConfigMapPartial::default(); + for server_id in ids { + let server = self.get_server_or_empty(server_id); + let fallback = other.get_server_or_empty(server_id); + + let merged_server = if server_id.as_str() == "*" { + server.with_fallback(fallback) + } else { + let global_server = self.get_server_or_empty(&ServerId::new("*")); + let global_fallback = other.get_server_or_empty(&ServerId::new("*")); + + // Handle tools with mixed server/tool inheritance + let tools = server + .tools + .keys() + .chain(fallback.tools.keys()) + .chain(global_server.tools.keys()) + .chain(global_fallback.tools.keys()) + .map(|id| { + let server_tool = server.get_tool_or_empty(id); + let server_tool_global = server.get_tool_or_empty(&ToolId::new("*")); + let global_server_tool = global_server.get_tool_or_empty(id); + let global_server_tool_global = + global_server.get_tool_or_empty(&ToolId::new("*")); + + let fallback_tool = fallback.get_tool_or_empty(id); + let fallback_tool_global = fallback.get_tool_or_empty(&ToolId::new("*")); + let global_fallback_tool = global_fallback.get_tool_or_empty(id); + let global_fallback_tool_global = + global_fallback.get_tool_or_empty(&ToolId::new("*")); + + let tool = server_tool + .with_fallback(fallback_tool) + .with_fallback(server_tool_global) + .with_fallback(fallback_tool_global) + .with_fallback(global_server_tool) + .with_fallback(global_fallback_tool) + .with_fallback(global_server_tool_global) + .with_fallback(global_fallback_tool_global); + + (id.clone(), tool) + }) + .collect(); + + // Merge server-level fields (enable, binary_checksum) with simple inheritance + let mut server = server + .with_fallback(fallback) + .with_fallback(global_server) + .with_fallback(global_fallback); + + server.tools = tools; + server + }; + + result.insert(server_id.clone(), merged_server); } + + result } } #[cfg(test)] mod tests { + use indoc::indoc; + use pretty_assertions::assert_eq; + use super::*; - use crate::mcp::server::{ - checksum::{Algorithm, Checksum}, - tool::Tool, - ToolId, - }; #[test] - #[expect(clippy::too_many_lines)] - fn test_mcp_get_server_with_defaults() { + #[expect(clippy::too_many_lines, clippy::needless_raw_string_hashes)] + fn test_server_partial_with_fallback() { struct TestCase { - servers: Vec<(&'static str, Server)>, - input: &'static str, - expected: Server, + config: &'static str, + fallback: &'static str, + expected: &'static str, } let cases = vec![ - ("no servers", TestCase { - servers: vec![], - input: "foo", - expected: Server { - enable: true, - binary_checksum: None, - tools: ConfigMap::default(), - }, + ("direct", TestCase { + config: indoc! {r#" + [servers."*".tools.github_issues] + run = "always" + "#}, + fallback: indoc! {r#" + [servers.embedded.tools.github_issues] + result = "ask" + "#}, + expected: indoc! {r#" + [servers.embedded.tools.github_issues] + run = "always" + result = "ask" + "#}, }), - ("no match", TestCase { - servers: vec![("foo", Server { - enable: false, - binary_checksum: Some(Checksum { - algorithm: Algorithm::Sha256, - value: "foo".to_owned(), - }), - tools: ConfigMap::default(), - })], - input: "bar", - expected: Server { - enable: true, - binary_checksum: None, - tools: ConfigMap::default(), - }, + ("global_tool_inheritance", TestCase { + config: indoc! {r#" + [servers."*".tools."*"] + enable = false + run = "always" + result = "ask" + "#}, + fallback: indoc! {r#" + [servers.embedded.tools.github_issues] + enable = true + "#}, + expected: indoc! {r#" + [servers.embedded.tools.github_issues] + enable = true + run = "always" + result = "ask" + "#}, }), - ("single match", TestCase { - servers: vec![("foo", Server { - enable: false, - binary_checksum: Some(Checksum { - algorithm: Algorithm::Sha256, - value: "bar".to_owned(), - }), - tools: ConfigMap::default(), - })], - input: "foo", - expected: Server { - enable: false, - binary_checksum: Some(Checksum { - algorithm: Algorithm::Sha256, - value: "bar".to_owned(), - }), - tools: ConfigMap::default(), - }, + ("server_level_fallback", TestCase { + config: indoc! {r#" + [servers."*"] + enable = true + + [servers."*".tools."*"] + run = "edit" + "#}, + fallback: indoc! {r#" + [servers.embedded.binary_checksum] + value = "abc123" + + [servers.embedded.tools.github_issues] + result = "always" + "#}, + expected: indoc! {r#" + [servers.embedded] + enable = true + + [servers.embedded.tools.github_issues] + run = "edit" + result = "always" + + [servers.embedded.binary_checksum] + value = "abc123" + "#}, }), - ("global defaults only", TestCase { - servers: vec![("*", Server { - enable: false, - binary_checksum: Some(Checksum { - algorithm: Algorithm::Sha256, - value: "global".to_owned(), - }), - tools: ConfigMap::from_iter(vec![(ToolId::new("*"), Tool { - enable: !Tool::default().enable, - ..Tool::default() - })]), - })], - input: "nonexistent", - expected: Server { - enable: false, - binary_checksum: Some(Checksum { - algorithm: Algorithm::Sha256, - value: "global".to_owned(), - }), - tools: ConfigMap::from_iter(vec![(ToolId::new("*"), Tool { - enable: !Tool::default().enable, - ..Tool::default() - })]), - }, + ("complex_2d_inheritance", TestCase { + config: indoc! {r#" + [servers."*"] + enable = true + + [servers."*".tools."*"] + run = "always" + result = "always" + + [servers."*".tools.github_issues] + enable = false + + [servers.embedded.tools."*"] + run = "edit" + "#}, + fallback: indoc! {r#" + [servers.embedded.binary_checksum] + value = "fallback123" + + [servers.embedded.tools.github_issues] + result = "ask" + "#}, + expected: indoc! {r#" + [servers.embedded] + enable = true + + [servers.embedded.tools.github_issues] + enable = false + run = "edit" + result = "ask" + + [servers.embedded.binary_checksum] + value = "fallback123" + "#}, }), - ("merge with global defaults - full override", TestCase { - servers: vec![ - ("*", Server { - enable: false, - binary_checksum: Some(Checksum { - algorithm: Algorithm::Sha256, - value: "default".to_owned(), - }), - tools: ConfigMap::from_iter(vec![(ToolId::new("default_tool"), Tool { - enable: !Tool::default().enable, - ..Tool::default() - })]), - }), - ("specific", Server { - enable: true, - binary_checksum: Some(Checksum { - algorithm: Algorithm::Sha256, - value: "specific".to_owned(), - }), - tools: ConfigMap::from_iter(vec![( - ToolId::new("specific_tool"), - Tool::default(), - )]), - }), - ], - input: "specific", - expected: Server { - enable: true, - binary_checksum: Some(Checksum { - algorithm: Algorithm::Sha256, - value: "specific".to_owned(), - }), - tools: ConfigMap::from_iter(vec![( - ToolId::new("specific_tool"), - Tool::default(), - )]), - }, + ("tool_style_inheritance", TestCase { + config: indoc! {r#" + [servers."*".tools."*"] + enable = true + + [servers."*".tools."*".style] + inline_results = "off" + results_file_link = "off" + "#}, + fallback: indoc! {r#" + [servers.embedded.tools.github_issues.style] + inline_results = "off" + results_file_link = "full" + "#}, + expected: indoc! {r#" + [servers.embedded.tools.github_issues] + enable = true + + [servers.embedded.tools.github_issues.style] + inline_results = "off" + results_file_link = "full" + "#}, + }), + ("mixed_specific_and_global", TestCase { + config: indoc! {r#" + [servers."*".tools."*"] + enable = false + run = "always" + + [servers.embedded.tools."*"] + enable = true + + [servers.embedded.tools.github_issues] + run = "edit" + "#}, + fallback: indoc! {r#" + [servers."*"] + enable = true + + [servers."*".binary_checksum] + value = "global_fallback" + + [servers.another.tools.github_issues] + result = "ask" + "#}, + expected: indoc! {r#" + [servers.embedded] + enable = true + + [servers.embedded.tools.github_issues] + enable = true + run = "edit" + + [servers.embedded.binary_checksum] + value = "global_fallback" + + [servers.another] + enable = true + + [servers.another.tools.github_issues] + enable = false + run = "always" + result = "ask" + + [servers.another.binary_checksum] + value = "global_fallback" + "#}, + }), + ("deep_tool_config_override", TestCase { + config: indoc! {r#" + [servers."*".tools."*"] + run = "always" + + [servers."*".tools."*".style] + inline_results = "off" + + [servers.embedded.tools.github_issues.style] + inline_results = "50" + results_file_link = "osc8" + "#}, + fallback: indoc! {r#" + [servers.embedded.tools."*"] + result = "edit" + + [servers.embedded.tools."*".style] + results_file_link = "off" + "#}, + expected: indoc! {r#" + [servers.embedded.tools.github_issues] + run = "always" + result = "edit" + + [servers.embedded.tools.github_issues.style] + inline_results = "50" + results_file_link = "osc8" + "#}, + }), + ("empty_config_with_fallback", TestCase { + config: "", + fallback: indoc! {r#" + [servers."*"] + enable = true + + [servers."*".tools."*"] + run = "always" + result = "always" + + [servers.embedded.tools.github_issues] + enable = false + "#}, + expected: indoc! {r#" + [servers.embedded] + enable = true + + [servers.embedded.tools.github_issues] + enable = false + run = "always" + result = "always" + "#}, + }), + ("fallback_empty", TestCase { + config: indoc! {r#" + [servers.embedded] + enable = false + + [servers.embedded.tools.github_issues] + run = "edit" + result = "ask" + "#}, + fallback: "", + expected: indoc! {r#" + [servers.embedded] + enable = false + + [servers.embedded.tools.github_issues] + run = "edit" + result = "ask" + "#}, + }), + ("global server, global tool only", TestCase { + config: indoc! {r#" + [servers."*"] + enable = true + + [servers."*".tools."*"] + enable = false + run = "always" + result = "ask" + style.inline_results = "off" + style.results_file_link = "off" + + [servers.embedded.tools.github_issues] + "#}, + fallback: "", + expected: indoc! {r#" + [servers.embedded] + enable = true + + [servers.embedded.tools.github_issues] + enable = false + run = "always" + result = "ask" + + [servers.embedded.tools.github_issues.style] + inline_results = "off" + results_file_link = "off" + "#}, + }), + ("global server, specific tool overrides enable", TestCase { + config: indoc! {r#" + [servers."*"] + enable = true + + [servers."*".tools."*"] + enable = false + run = "always" + result = "ask" + style.inline_results = "off" + style.results_file_link = "off" + + [servers."*".tools.github_issues] + enable = true + style.inline_results = "full" + style.results_file_link = "osc8" + + [servers.embedded.tools.github_issues] + "#}, + fallback: "", + expected: indoc! {r#" + [servers.embedded] + enable = true + + [servers.embedded.tools.github_issues] + enable = true + run = "always" + result = "ask" + "#}, + }), + ("specific server, global tool overrides run", TestCase { + config: indoc! {r#" + [servers."*"] + enable = true + + [servers."*".tools."*"] + enable = false + run = "always" + result = "ask" + style.inline_results = "off" + style.results_file_link = "off" + + [servers.embedded.tools."*"] + run = "edit" + result = "edit" + + [servers.embedded.tools.github_issues] + "#}, + fallback: "", + expected: indoc! {r#" + [servers.embedded] + enable = true + + [servers.embedded.tools.github_issues] + enable = false + run = "edit" + result = "edit" + + [servers.embedded.tools.github_issues.style] + inline_results = "off" + results_file_link = "off" + "#}, + }), + ("complex inheritance chain", TestCase { + config: indoc! {r#" + [servers."*"] + enable = true + + [servers."*".tools."*"] + enable = false + run = "always" + result = "always" + style.inline_results = "full" + style.results_file_link = "off" + + [servers."*".tools.github_issues] + enable = true + + [servers.embedded.tools.github_issues] + result = "edit" + style.inline_results = "10" + + [servers.embedded.tools."*"] + run = "edit" + result = "ask" + "#}, + fallback: "", + expected: indoc! {r#" + [servers.embedded] + enable = true + + [servers.embedded.tools.github_issues] + enable = true + run = "edit" + result = "edit" + + [servers.embedded.tools.github_issues.style] + inline_results = "10" + results_file_link = "off" + "#}, }), ( - "merge with global defaults - partial override enable only", - TestCase { - servers: vec![ - ("*", Server { - enable: false, - binary_checksum: Some(Checksum { - algorithm: Algorithm::Sha256, - value: "default".to_owned(), - }), - tools: ConfigMap::from_iter(vec![( - ToolId::new("default_tool"), - Tool { - enable: !Tool::default().enable, - ..Tool::default() - }, - )]), - }), - ("specific", Server { - enable: true, - binary_checksum: Some(Checksum { - algorithm: Algorithm::Sha256, - value: "default".to_owned(), - }), - tools: ConfigMap::from_iter(vec![( - ToolId::new("default_tool"), - Tool { - enable: !Tool::default().enable, - ..Tool::default() - }, - )]), - }), - ], - input: "specific", - expected: Server { - enable: true, - binary_checksum: Some(Checksum { - algorithm: Algorithm::Sha256, - value: "default".to_owned(), - }), - tools: ConfigMap::from_iter(vec![(ToolId::new("default_tool"), Tool { - enable: !Tool::default().enable, - ..Tool::default() - })]), - }, - }, - ), - ( - "merge with global defaults - partial override checksum only", - TestCase { - servers: vec![ - ("*", Server { - enable: false, - binary_checksum: Some(Checksum { - algorithm: Algorithm::Sha256, - value: "default".to_owned(), - }), - tools: ConfigMap::from_iter(vec![( - ToolId::new("default_tool"), - Tool { - enable: !Tool::default().enable, - ..Tool::default() - }, - )]), - }), - ("specific", Server { - enable: false, - binary_checksum: Some(Checksum { - algorithm: Algorithm::Sha256, - value: "specific".to_owned(), - }), - tools: ConfigMap::from_iter(vec![( - ToolId::new("default_tool"), - Tool { - enable: !Tool::default().enable, - ..Tool::default() - }, - )]), - }), - ], - input: "specific", - expected: Server { - enable: false, - binary_checksum: Some(Checksum { - algorithm: Algorithm::Sha256, - value: "specific".to_owned(), - }), - tools: ConfigMap::from_iter(vec![(ToolId::new("default_tool"), Tool { - enable: !Tool::default().enable, - ..Tool::default() - })]), - }, - }, - ), - ( - "merge with global defaults - partial override tools only", + "complex inheritance with Some None distinctions", TestCase { - servers: vec![ - ("*", Server { - enable: false, - binary_checksum: Some(Checksum { - algorithm: Algorithm::Sha256, - value: "default".to_owned(), - }), - tools: ConfigMap::from_iter(vec![( - ToolId::new("default_tool"), - Tool { - enable: !Tool::default().enable, - ..Tool::default() - }, - )]), - }), - ("specific", Server { - enable: false, - binary_checksum: Some(Checksum { - algorithm: Algorithm::Sha256, - value: "default".to_owned(), - }), - tools: ConfigMap::from_iter(vec![( - ToolId::new("specific_tool"), - Tool::default(), - )]), - }), - ], - input: "specific", - expected: Server { - enable: false, - binary_checksum: Some(Checksum { - algorithm: Algorithm::Sha256, - value: "default".to_owned(), - }), - tools: ConfigMap::from_iter(vec![( - ToolId::new("specific_tool"), - Tool::default(), - )]), - }, + config: indoc! {r#" + [servers."*"] + enable = true + + [servers."*".tools."*"] + enable = false + run = "always" + result = "always" + style.inline_results = "off" + style.results_file_link = "off" + + [servers.embedded.tools.github_issues] + enable = true + "#}, + fallback: "", + expected: indoc! {r#" + [servers.embedded] + enable = true + + [servers.embedded.tools.github_issues] + enable = true + run = "always" + result = "always" + + [servers.embedded.tools.github_issues.style] + inline_results = "off" + results_file_link = "off" + "#}, }, ), - ("merge None checksum with Some default", TestCase { - servers: vec![ - ("*", Server { - enable: true, - binary_checksum: Some(Checksum { - algorithm: Algorithm::Sha256, - value: "default".to_owned(), - }), - tools: ConfigMap::default(), - }), - ("specific", Server { - enable: true, - binary_checksum: None, - tools: ConfigMap::default(), - }), - ], - input: "specific", - expected: Server { - enable: true, - binary_checksum: None, - tools: ConfigMap::default(), - }, - }), - ("merge Some checksum with None default", TestCase { - servers: vec![ - ("*", Server { - enable: true, - binary_checksum: None, - tools: ConfigMap::default(), - }), - ("specific", Server { - enable: true, - binary_checksum: Some(Checksum { - algorithm: Algorithm::Sha256, - value: "specific".to_owned(), - }), - tools: ConfigMap::default(), - }), - ], - input: "specific", - expected: Server { - enable: true, - binary_checksum: Some(Checksum { - algorithm: Algorithm::Sha256, - value: "specific".to_owned(), - }), - tools: ConfigMap::default(), - }, - }), - ("exact match with defaults should use defaults", TestCase { - servers: vec![ - ("*", Server { - enable: false, - binary_checksum: Some(Checksum { - algorithm: Algorithm::Sha256, - value: "same".to_owned(), - }), - tools: ConfigMap::from_iter(vec![(ToolId::new("tool"), Tool { - enable: !Tool::default().enable, - ..Tool::default() - })]), - }), - ("specific", Server { - enable: false, - binary_checksum: Some(Checksum { - algorithm: Algorithm::Sha256, - value: "same".to_owned(), - }), - tools: ConfigMap::from_iter(vec![(ToolId::new("tool"), Tool { - enable: !Tool::default().enable, - ..Tool::default() - })]), - }), - ], - input: "specific", - expected: Server { - enable: false, - binary_checksum: Some(Checksum { - algorithm: Algorithm::Sha256, - value: "same".to_owned(), - }), - tools: ConfigMap::from_iter(vec![(ToolId::new("tool"), Tool { - enable: !Tool::default().enable, - ..Tool::default() - })]), - }, - }), ]; for (name, test) in cases { - let mcp = Mcp { - servers: test - .servers - .into_iter() - .map(|(k, v)| (ServerId(k.to_string()), v)) - .collect(), - }; + let config = toml::from_str::(test.config).unwrap(); + let fallback = toml::from_str::(test.fallback).unwrap(); + + let merged = config.with_fallback(fallback); + let actual = toml::to_string_pretty(&merged).unwrap(); - let received = mcp.get_server_with_defaults(test.input); - dbg!(&received); - assert_eq!(received, test.expected, "test case: {name}"); + assert_eq!(test.expected.to_owned(), actual, "test case: {name}"); } } } diff --git a/crates/jp_config/src/mcp/server.rs b/crates/jp_config/src/mcp/server.rs index b506f136..fc4cbea0 100644 --- a/crates/jp_config/src/mcp/server.rs +++ b/crates/jp_config/src/mcp/server.rs @@ -172,6 +172,11 @@ impl ToolId { pub fn new(id: impl Into) -> Self { Self(id.into()) } + + #[must_use] + pub fn as_str(&self) -> &str { + &self.0 + } } impl Deref for ToolId { @@ -191,11 +196,22 @@ impl ConfigKey for ToolId { pub struct ServerPartial { #[serde(default, skip_serializing_if = "Option::is_none")] pub enable: Option, + #[serde(default, skip_serializing_if = "ConfigMapPartial::is_empty")] pub tools: ConfigMapPartial, #[serde(default, skip_serializing_if = "Option::is_none")] pub binary_checksum: Option, } +impl ServerPartial { + #[must_use] + pub(crate) fn get_tool_or_empty(&self, tool_id: &ToolId) -> ToolPartial { + self.tools + .get(tool_id) + .cloned() + .unwrap_or(ToolPartial::empty()) + } +} + impl Default for ServerPartial { fn default() -> Self { let mut tools = ConfigMapPartial::default(); @@ -229,8 +245,8 @@ impl Partial for ServerPartial { fn with_fallback(self, fallback: Self) -> Self { Self { enable: self.enable.or(fallback.enable), - tools: self.tools.with_fallback(fallback.tools), binary_checksum: self.binary_checksum.or(fallback.binary_checksum), + tools: self.tools.with_fallback(fallback.tools), } } diff --git a/crates/jp_config/src/mcp/server/tool.rs b/crates/jp_config/src/mcp/server/tool.rs index e100ef9c..0b4a29fd 100644 --- a/crates/jp_config/src/mcp/server/tool.rs +++ b/crates/jp_config/src/mcp/server/tool.rs @@ -104,18 +104,13 @@ pub struct ToolPartial { pub run: Option, #[serde(default, skip_serializing_if = "Option::is_none")] pub result: Option, - // TODO: fix this, have the partial impl `Default`? #[serde( - default = "default_style", + default = "confique::Partial::empty", skip_serializing_if = "is_nested_default_or_empty" )] pub style: ::Partial, } -fn default_style() -> ::Partial { - ::Partial::default_values() -} - impl AssignKeyValue for ToolPartial { fn assign(&mut self, mut kv: KvAssignment) -> Result<(), Error> { let k = kv.key().as_str().to_owned(); diff --git a/crates/jp_config/src/mcp/tool_call.rs b/crates/jp_config/src/mcp/tool_call.rs index f6e3d443..8da1c3b2 100644 --- a/crates/jp_config/src/mcp/tool_call.rs +++ b/crates/jp_config/src/mcp/tool_call.rs @@ -22,7 +22,11 @@ use crate::{ pub struct ToolCall { /// Whether to show the tool call results inline. Even if disabled, a link /// will be added to a file containing the tool call results. - #[config(default = "full", deserialize_with = de_from_str)] + #[config( + default = "full", + deserialize_with = de_from_str, + partial_attr(serde(skip_serializing_if = "Option::is_none")) + )] pub inline_results: InlineResults, /// Show a link to the file containing the source code in code blocks. @@ -30,7 +34,11 @@ pub struct ToolCall { /// Can be one of: `off`, `full`, `osc8`. /// /// See: - #[config(default = "osc8", deserialize_with = de_from_str)] + #[config( + default = "osc8", + deserialize_with = de_from_str, + partial_attr(serde(skip_serializing_if = "Option::is_none")) + )] pub results_file_link: LinkStyle, } @@ -64,9 +72,18 @@ pub enum InlineResults { Full, /// Show the first N lines of the tool call results inline. + #[serde(untagged, serialize_with = "serialize_truncate")] Truncate { lines: usize }, } +#[expect(clippy::trivially_copy_pass_by_ref)] +fn serialize_truncate(lines: &usize, serializer: S) -> std::result::Result +where + S: serde::Serializer, +{ + serializer.serialize_str(&lines.to_string()) +} + impl FromStr for InlineResults { type Err = Error; diff --git a/crates/jp_config/src/style.rs b/crates/jp_config/src/style.rs index ca0bb5b9..38257b1b 100644 --- a/crates/jp_config/src/style.rs +++ b/crates/jp_config/src/style.rs @@ -57,12 +57,13 @@ impl AssignKeyValue for