From 610473f96e11883b4f77b952b98f28ac0635b736 Mon Sep 17 00:00:00 2001 From: Jean Mertz Date: Mon, 17 Aug 2026 19:28:31 +0200 Subject: [PATCH] fix(config): Inherit tool style from `'*'` field by field `[conversation.tools.'*'.style]` was silently ignored by any tool that set a style field of its own. `ToolConfig::style` is an `Option`, so the moment a tool declares one key the option is `Some` and every other field reads as deliberately set. Every neighbouring accessor fills from the defaults per field; only `style` fell back as a unit. The practical effect was that a one-line default did nothing: [conversation.tools.'*'.style] hidden = true Every tool that already set `style.inline_results` took its own style wholesale and dropped the default, with no error and no warning. Hiding read-only tool calls across a persona meant naming each tool by hand. `PartialToolsConfig::fill_from` now fills each tool's style from the resolved `*` block. Doing that at partial-merge time rather than in `ToolConfigWithDefaults::style` is what makes it possible: the partial still records which keys the tool asked for, so the gaps are identifiable. Once the config is resolved that information is gone. Signed-off-by: Jean Mertz --- crates/jp_config/src/conversation/tool.rs | 29 ++++++++- .../jp_config/src/conversation/tool_tests.rs | 61 +++++++++++++++++++ ...nherit-field-by-field-from-the-defaults.md | 57 +++++++++++++++++ 3 files changed, 144 insertions(+), 3 deletions(-) create mode 100644 docs/ticket/0011-tool-style-does-not-inherit-field-by-field-from-the-defaults.md diff --git a/crates/jp_config/src/conversation/tool.rs b/crates/jp_config/src/conversation/tool.rs index 4dba4caa8..600981c78 100644 --- a/crates/jp_config/src/conversation/tool.rs +++ b/crates/jp_config/src/conversation/tool.rs @@ -82,9 +82,28 @@ impl PartialConfigDelta for PartialToolsConfig { impl FillDefaults for PartialToolsConfig { fn fill_from(self, defaults: Self) -> Self { + let tool_defaults = self.defaults.fill_from(defaults.defaults); + + // A tool declaring any `style` field of its own resolves to a whole + // `DisplayStyleConfig`, at which point the fields it didn't set are + // indistinguishable from the ones it did. Fill the gaps from the `*` + // block here, while the partial still records what the tool asked for, + // so a single `[conversation.tools.'*'.style]` key reaches every tool. + let tools = self + .tools + .into_iter() + .map(|(name, mut tool)| { + tool.style = tool + .style + .map(|style| style.fill_from(tool_defaults.style.clone())); + + (name, tool) + }) + .collect(); + Self { - defaults: self.defaults.fill_from(defaults.defaults), - tools: self.tools, + defaults: tool_defaults, + tools, } } } @@ -430,7 +449,8 @@ pub struct ToolConfig { /// How to display the results of the tool in the terminal. /// - /// Overrides the global default. + /// Overrides the global default field by field: keys set here win, keys + /// left out take their value from `conversation.tools.'*'.style`. /// The error overlay lives at `style.error.*` (see /// [`DisplayStyleConfig::error`]). #[setting(nested)] @@ -1210,6 +1230,9 @@ impl ToolConfigWithDefaults { } /// Return the display style of the tool. + /// + /// Fields the tool does not set carry the value from the global `*` + /// defaults, so the returned style is already fully resolved. #[must_use] pub fn style(&self) -> &DisplayStyleConfig { self.tool.style.as_ref().unwrap_or(&self.defaults.style) diff --git a/crates/jp_config/src/conversation/tool_tests.rs b/crates/jp_config/src/conversation/tool_tests.rs index 607e796c1..1eca98cb8 100644 --- a/crates/jp_config/src/conversation/tool_tests.rs +++ b/crates/jp_config/src/conversation/tool_tests.rs @@ -70,6 +70,67 @@ fn access_on_local_tool_is_accepted_by_validation() { assert!(build(partial).is_ok()); } +#[test] +fn tool_style_fills_unset_fields_from_the_global_defaults() { + use crate::{ + PartialAppConfig, + conversation::tool::style::{InlineResults, PartialDisplayStyleConfig}, + util::build, + }; + + let mut partial = PartialAppConfig::new_test(); + partial.conversation.tools.defaults.style.hidden = Some(true); + partial + .conversation + .tools + .tools + .insert("my_tool".to_owned(), PartialToolConfig { + source: Some(ToolSource::Local { tool: None }), + style: Some(PartialDisplayStyleConfig { + inline_results: Some(InlineResults::Full), + ..Default::default() + }), + ..Default::default() + }); + + let config = build(partial).unwrap(); + let tool = config.conversation.tools.get("my_tool").unwrap(); + + assert!(tool.style().hidden); + assert_eq!(tool.style().inline_results, InlineResults::Full); +} + +#[test] +fn tool_style_overrides_the_global_default_per_field() { + use crate::{ + PartialAppConfig, + conversation::tool::style::{InlineResults, PartialDisplayStyleConfig}, + util::build, + }; + + let mut partial = PartialAppConfig::new_test(); + partial.conversation.tools.defaults.style.hidden = Some(true); + partial.conversation.tools.defaults.style.inline_results = Some(InlineResults::Off); + partial + .conversation + .tools + .tools + .insert("my_tool".to_owned(), PartialToolConfig { + source: Some(ToolSource::Local { tool: None }), + style: Some(PartialDisplayStyleConfig { + hidden: Some(false), + ..Default::default() + }), + ..Default::default() + }); + + let config = build(partial).unwrap(); + let tool = config.conversation.tools.get("my_tool").unwrap(); + + assert!(!tool.style().hidden); + assert_eq!(tool.style().inline_results, InlineResults::Off); +} + #[test] fn test_enable_config_from_bool() { assert_eq!(PartialEnableConfig::from(true), PartialEnableConfig::ON); diff --git a/docs/ticket/0011-tool-style-does-not-inherit-field-by-field-from-the-defaults.md b/docs/ticket/0011-tool-style-does-not-inherit-field-by-field-from-the-defaults.md new file mode 100644 index 000000000..c91209d14 --- /dev/null +++ b/docs/ticket/0011-tool-style-does-not-inherit-field-by-field-from-the-defaults.md @@ -0,0 +1,57 @@ +# T0011: Tool style does not inherit field-by-field from the '\*' defaults + +- **Status**: Done +- **Kind**: Bug +- **Authors**: jp +- **Date**: 2026-08-11 + +`[conversation.tools.'*'.style]` is silently ignored for any tool that sets a +single style field of its own. + +`ToolConfigWithDefaults::style` resolves the whole struct at once +(`crates/jp_config/src/conversation/tool.rs:1215`): + +```rust +pub fn style(&self) -> &DisplayStyleConfig { + self.tool.style.as_ref().unwrap_or(&self.defaults.style) +} +``` + +Every neighbouring accessor fills field-by-field from the defaults. +`enable` goes through `Enable::effective`, and `run`, `result`, `format`, and +`cancellation_response` each `unwrap_or` a scalar. +Only `style` falls back as a unit. + +## Why it bites + +Setting a single style default across every tool looks like this and does +nothing: + +```toml +[conversation.tools.'*'.style] +hidden = true +``` + +Every tool the skills enable already sets `style.inline_results`, so every one +of them takes its own style wholesale and drops the default. +The failure is silent: no error, no warning, the setting is simply absent from +the resolved config. + +Found while hiding read-only tool calls for the RFD pipeline personas, which +ended up enumerating twenty-two tools by name because the one-line version does +not work. +That list will drift as tools are added. + +## Fix + +`DisplayStyleConfig` already implements `FillDefaults`, and +`PartialToolConfig::fill_from` already calls `style.fill_from(defaults.style)` +at partial-merge time. +The resolved-read path wants the same treatment: resolve the per-tool style +against the defaults per field rather than choosing one struct. + +## Watch for + +Whether anything currently relies on the whole-struct behaviour, for instance a +tool that sets one style field expecting the others to fall back to the +hardcoded defaults rather than to a `'*'` block.