diff --git a/crates/contrib/schematic_macros/src/common/container.rs b/crates/contrib/schematic_macros/src/common/container.rs index 1a6281342..7063f5859 100644 --- a/crates/contrib/schematic_macros/src/common/container.rs +++ b/crates/contrib/schematic_macros/src/common/container.rs @@ -183,6 +183,7 @@ fn generate_enum_schema( .iter() .all(|v| matches!(v.value.fields, Fields::Unit)); let mut default_index = None; + let mut expanded_index = None; let variants_types = variants .iter() @@ -192,6 +193,10 @@ fn generate_enum_schema( default_index = Some(i); } + if v.args.expanded { + expanded_index = Some(i); + } + if v.is_excluded() { None } else { @@ -200,6 +205,10 @@ fn generate_enum_schema( }) .collect::>(); + let expanded = expanded_index.map_or_else( + || quote! {}, + |index| quote! { union = union.with_expanded_index(#index); }, + ); let default_index = map_option_argument_quote(default_index); if is_all_unit_enum { @@ -217,12 +226,16 @@ fn generate_enum_schema( quote! { #deprecated #description - schema.union(UnionType::from_schemas( + + let mut union = UnionType::from_schemas( [ #(#variants_types),* ], #default_index, - )) + ); + #expanded + + schema.union(union) } } } diff --git a/crates/contrib/schematic_macros/src/common/macros.rs b/crates/contrib/schematic_macros/src/common/macros.rs index 171c3dc5e..79c53b0b6 100644 --- a/crates/contrib/schematic_macros/src/common/macros.rs +++ b/crates/contrib/schematic_macros/src/common/macros.rs @@ -51,6 +51,15 @@ pub struct MacroArgs { pub serde: SerdeMeta, pub no_deserialize_derive: bool, + // Declare input shapes the derive cannot infer, for a type whose + // `Deserialize` accepts more than its fields describe (a struct that also + // deserializes from a bool, say). + // + // Names a `fn(&mut SchemaBuilder) -> Vec`. The derived struct schema + // is unioned with the returned variants, so field names and doc comments + // are still described alongside the extra shapes. + pub schema_union_with: Option, + // Quick hack to avoid `is_untagged` to generate a custom Deserialize impl, // which ignores any custom serde tags on an enum. pub skip_custom_untagged_enum_deserialize_impl: bool, diff --git a/crates/contrib/schematic_macros/src/common/variant.rs b/crates/contrib/schematic_macros/src/common/variant.rs index 59b3c73ed..97c423e69 100644 --- a/crates/contrib/schematic_macros/src/common/variant.rs +++ b/crates/contrib/schematic_macros/src/common/variant.rs @@ -34,6 +34,13 @@ pub struct VariantArgs { pub nested: bool, pub required: bool, pub empty: bool, + + /// Mark this variant as the form the enum's other variants abbreviate. + /// + /// Populates `UnionType::expanded_index`, so a schema consumer resolving + /// nested keys knows to follow this variant's fields. + pub expanded: bool, + #[darling(with = preserve_str_literal, map = "Some")] pub is_empty: Option, diff --git a/crates/contrib/schematic_macros/src/config/mod.rs b/crates/contrib/schematic_macros/src/config/mod.rs index 6cfd6fc77..77c216633 100644 --- a/crates/contrib/schematic_macros/src/config/mod.rs +++ b/crates/contrib/schematic_macros/src/config/mod.rs @@ -153,6 +153,29 @@ 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); + // `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, + // which is what lets the union name an expanded form below. The partial's + // impl delegates here, so it sees the union too. + let build_body = match cfg.args.schema_union_with.as_ref() { + None => quote! { + #schema_impl + }, + Some(path) => quote! { + let described = { #schema_impl }; + let mut variants = #path(&mut schema); + + // The derived schema goes last and is marked as the expanded form: + // the caller-supplied variants are shorthand spellings of it, so a + // consumer resolving keys should follow the derived fields. + let expanded = variants.len(); + variants.push(described); + + schema.union(UnionType::new_any(variants).with_expanded_index(expanded)) + }, + }; + quote! { #[automatically_derived] impl #impl_generics schematic::Schematic for #name #ty_generics #schematic_where { @@ -164,7 +187,7 @@ fn emit_schematic_impls(args: &SchematicImplArgs<'_>) -> TokenStream { fn build_schema(mut schema: schematic::SchemaBuilder) -> schematic::Schema { use schematic::schema::*; - #schema_impl + #build_body } } diff --git a/crates/contrib/schematic_types/src/unions.rs b/crates/contrib/schematic_types/src/unions.rs index fcf37ec87..a48322a98 100644 --- a/crates/contrib/schematic_types/src/unions.rs +++ b/crates/contrib/schematic_types/src/unions.rs @@ -19,6 +19,23 @@ pub struct UnionType { )] pub default_index: Option, + /// Index of the variant the other variants are shorthand spellings of. + /// + /// Set when a union describes one value written several ways, so a consumer + /// can find the form that names the value's parts: a tool's `enable` + /// accepts `true` or `{ state, allow_toggle }`, and the table is the + /// expanded form of the bool. + /// + /// Left unset when the variants are genuinely different values rather than + /// spellings of one. + /// A model id is either an id or an alias resolved through a lookup, and + /// neither expands into the other. + #[cfg_attr( + feature = "serde", + serde(default, skip_serializing_if = "Option::is_none") + )] + pub expanded_index: Option, + pub partial: bool, pub operator: UnionOperator, @@ -58,6 +75,23 @@ impl UnionType { } } + /// Mark which variant the others are shorthand spellings of. + /// + /// See [`Self::expanded_index`]. + #[must_use] + pub fn with_expanded_index(mut self, index: usize) -> Self { + self.expanded_index = Some(index); + self + } + + /// The variant the others are shorthand spellings of, if any. + #[must_use] + pub fn expanded_variant(&self) -> Option<&Schema> { + self.expanded_index + .and_then(|index| self.variants_types.get(index)) + .map(AsRef::as_ref) + } + #[must_use] pub fn has_null(&self) -> bool { self.variants_types.iter().any(|schema| schema.ty.is_null()) diff --git a/crates/jp_cli/src/cmd/conversation/edit.rs b/crates/jp_cli/src/cmd/conversation/edit.rs index cfdff5aa2..126b94596 100644 --- a/crates/jp_cli/src/cmd/conversation/edit.rs +++ b/crates/jp_cli/src/cmd/conversation/edit.rs @@ -356,8 +356,8 @@ async fn generate_titles( tool_choice: jp_config::assistant::tool_choice::ToolChoice::default(), }; - let retry_config = - RetryConfig::default().with_max_response_bytes(config.assistant.request.max_response_bytes); + let retry_config = RetryConfig::default() + .with_max_response_bytes(config.assistant.request.max_response_bytes.bytes()); let llm_events = collect_with_retry(provider.as_ref(), &model_details, query, &retry_config).await?; diff --git a/crates/jp_cli/src/cmd/conversation/summarize.rs b/crates/jp_cli/src/cmd/conversation/summarize.rs index 4a519c7d5..f56246ade 100644 --- a/crates/jp_cli/src/cmd/conversation/summarize.rs +++ b/crates/jp_cli/src/cmd/conversation/summarize.rs @@ -96,7 +96,7 @@ pub async fn generate_summary( stream, instructions, &user_message, - app_cfg.assistant.request.max_response_bytes, + app_cfg.assistant.request.max_response_bytes.bytes(), ) .await } @@ -121,7 +121,7 @@ async fn summarize_stream( mut stream: ConversationStream, instructions: &str, user_message: &str, - max_response_bytes: u32, + max_response_bytes: Option, ) -> Result { let retry_config = RetryConfig::default().with_max_response_bytes(max_response_bytes); diff --git a/crates/jp_cli/src/cmd/conversation/summarize_tests.rs b/crates/jp_cli/src/cmd/conversation/summarize_tests.rs index 6a8271266..b8f0340bd 100644 --- a/crates/jp_cli/src/cmd/conversation/summarize_tests.rs +++ b/crates/jp_cli/src/cmd/conversation/summarize_tests.rs @@ -54,13 +54,13 @@ async fn summarize_with( batches: Vec>, stream: ConversationStream, ) -> super::Result { - summarize_with_ceiling(batches, stream, 1_048_576).await + summarize_with_ceiling(batches, stream, Some(1_048_576)).await } async fn summarize_with_ceiling( batches: Vec>, stream: ConversationStream, - max_response_bytes: u32, + max_response_bytes: Option, ) -> super::Result { let provider = MockProvider::with_batches(batches); let model_id = test_model_id(); @@ -90,7 +90,7 @@ async fn summarize_applies_the_configured_output_ceiling() { FinishReason::Completed, )]; - let error = summarize_with_ceiling(batches, range_stream(&["sig"]), 25) + let error = summarize_with_ceiling(batches, range_stream(&["sig"]), Some(25)) .await .expect_err("the summary must stop at the configured ceiling"); diff --git a/crates/jp_cli/src/cmd/query/interrupt/signals_tests.rs b/crates/jp_cli/src/cmd/query/interrupt/signals_tests.rs index 1a752c69c..ae661207c 100644 --- a/crates/jp_cli/src/cmd/query/interrupt/signals_tests.rs +++ b/crates/jp_cli/src/cmd/query/interrupt/signals_tests.rs @@ -3,7 +3,7 @@ use std::sync::Arc; use assert_matches::assert_matches; use jp_config::{ AppConfig, - assistant::request::{CachePolicy, RequestConfig}, + assistant::request::{CachePolicy, MaxResponseBytes, RequestConfig}, }; use jp_conversation::{ ConversationEvent, ConversationStream, @@ -46,7 +46,7 @@ fn make_retry_state(max_retries: u32) -> StreamRetryState { base_backoff_ms: 1, max_backoff_secs: 1, stream_idle_timeout_secs: 120, - max_response_bytes: 1_048_576, + max_response_bytes: MaxResponseBytes::default(), cache: CachePolicy::default(), }; StreamRetryState::new(config, false) diff --git a/crates/jp_cli/src/cmd/query/stream/retry_tests.rs b/crates/jp_cli/src/cmd/query/stream/retry_tests.rs index a0fbbfa96..ba7d18e05 100644 --- a/crates/jp_cli/src/cmd/query/stream/retry_tests.rs +++ b/crates/jp_cli/src/cmd/query/stream/retry_tests.rs @@ -2,7 +2,7 @@ use std::{sync::Arc, time::Duration}; use jp_config::{ AppConfig, - assistant::request::{CachePolicy, RequestConfig}, + assistant::request::{CachePolicy, MaxResponseBytes, RequestConfig}, }; use jp_conversation::{ Conversation, @@ -21,7 +21,7 @@ fn make_retry_state(max_retries: u32) -> StreamRetryState { base_backoff_ms: 1, // 1ms for fast tests max_backoff_secs: 1, stream_idle_timeout_secs: 120, - max_response_bytes: 1_048_576, + max_response_bytes: MaxResponseBytes::default(), cache: CachePolicy::default(), }; StreamRetryState::new(config, false) @@ -86,7 +86,7 @@ fn backoff_uses_retry_after_when_present() { base_backoff_ms: 1, max_backoff_secs: 120, stream_idle_timeout_secs: 120, - max_response_bytes: 1_048_576, + max_response_bytes: MaxResponseBytes::default(), cache: CachePolicy::default(), }; let state = StreamRetryState::new(config, false); @@ -343,7 +343,7 @@ async fn interrupt_during_backoff_cuts_wait_short() { base_backoff_ms: 1, max_backoff_secs: 120, stream_idle_timeout_secs: 120, - max_response_bytes: 1_048_576, + max_response_bytes: MaxResponseBytes::default(), cache: CachePolicy::default(), }; let mut retry_state = StreamRetryState::new(config, false); diff --git a/crates/jp_cli/src/cmd/query/tool/inquiry.rs b/crates/jp_cli/src/cmd/query/tool/inquiry.rs index 724813a1d..1961ec973 100644 --- a/crates/jp_cli/src/cmd/query/tool/inquiry.rs +++ b/crates/jp_cli/src/cmd/query/tool/inquiry.rs @@ -147,8 +147,9 @@ pub struct InquiryConfig { pub sections: Vec, /// Output ceiling for the inquiry request, from - /// `assistant.request.max_response_bytes`. - pub max_response_bytes: u32, + /// `conversation.inquiry.assistant.request.max_response_bytes`. + /// `None` leaves the response unbounded. + pub max_response_bytes: Option, } /// Resolves inquiries by making structured output calls to an LLM provider. diff --git a/crates/jp_cli/src/cmd/query/tool/inquiry_tests.rs b/crates/jp_cli/src/cmd/query/tool/inquiry_tests.rs index ff8c3533a..8b64128c4 100644 --- a/crates/jp_cli/src/cmd/query/tool/inquiry_tests.rs +++ b/crates/jp_cli/src/cmd/query/tool/inquiry_tests.rs @@ -39,7 +39,7 @@ fn test_inquiry_config(provider: MockProvider) -> InquiryConfig { model: test_model(), system_prompt: None, sections: vec![], - max_response_bytes: 1_048_576, + max_response_bytes: Some(1_048_576), } } @@ -324,7 +324,7 @@ async fn llm_backend_uses_per_question_override() { model: test_model(), system_prompt: Some("Override prompt.".into()), sections: vec![], - max_response_bytes: 1_048_576, + max_response_bytes: Some(1_048_576), }; let overrides = IndexMap::from([(("test_tool".into(), "confirm".into()), override_config)]); @@ -559,7 +559,7 @@ async fn dedicated_model_backend_returns_answer() { }), system_prompt: Some("Answer concisely.".to_string()), sections: vec![], - max_response_bytes: 1_048_576, + max_response_bytes: Some(1_048_576), }; let backend = LlmInquiryBackend::new(config, IndexMap::new(), vec![], vec![]); diff --git a/crates/jp_cli/src/cmd/query/turn_loop.rs b/crates/jp_cli/src/cmd/query/turn_loop.rs index a751b0307..fb63957b9 100644 --- a/crates/jp_cli/src/cmd/query/turn_loop.rs +++ b/crates/jp_cli/src/cmd/query/turn_loop.rs @@ -18,8 +18,11 @@ use futures::{ use indexmap::IndexMap; use jp_attachment::Attachment; use jp_config::{ - AppConfig, PartialConfig, assistant::tool_choice::ToolChoice, - conversation::tool::QuestionTarget, model::id::ProviderId, style::streaming::StreamingConfig, + AppConfig, PartialConfig, + assistant::{request::MaxResponseBytes, tool_choice::ToolChoice}, + conversation::tool::QuestionTarget, + model::id::ProviderId, + style::streaming::StreamingConfig, }; use jp_conversation::{ ConversationStream, @@ -31,7 +34,6 @@ use jp_llm::{ error::StreamError, event::{Event, EventPart, FinishReason, ToolCallPart}, model::ModelDetails, - output_limit_bytes, provider::get_provider, query::ChatQuery, tool::{InvocationContext, ToolDefinition, executor::Executor}, @@ -207,7 +209,7 @@ pub(super) async fn run_turn_loop( 0 => None, secs => Some(Duration::from_secs(u64::from(secs))), }; - let output_limit = output_limit_bytes(cfg.assistant.request.max_response_bytes); + let output_limit = cfg.assistant.request.max_response_bytes.bytes(); let mut turn_coordinator = TurnCoordinator::new( printer.clone(), cfg.style.clone(), @@ -873,43 +875,34 @@ async fn build_inquiry_backend( provider: Arc, attachments: Vec, ) -> Result, Error> { - let sections = build_sections(&cfg.assistant, !tools.is_empty()); - let inquiry_override = &cfg.conversation.inquiry.assistant; - - // Use the inquiry system prompt if configured, otherwise fall back to the - // parent assistant's system prompt. - let default_system_prompt = inquiry_override - .system_prompt - .clone() - .or_else(|| cfg.assistant.system_prompt.clone()); - - // Same fallback for the output ceiling: an inquiry can be held to a tighter - // (or looser) ceiling than the parent assistant. - // - // `AssistantOverrideConfig::request` is a resolved `RequestConfig`, so a - // block where the user set only a sibling field (say `cache`) arrives here - // with every other field at Rust's `Default` rather than its schematic - // default. A `0` therefore cannot be distinguished from "unset", and reading - // it as the ceiling's disable sentinel would silently drop the runaway guard - // for every inquiry. Treat it as "inherit" instead, matching the block's - // documented unset-means-inherit rule. A per-question override carries real - // `Option`s, so `0` still disables the ceiling there. - let default_max_response_bytes = inquiry_override - .request - .as_ref() - .map_or(0, |request| request.max_response_bytes); - let default_max_response_bytes = match default_max_response_bytes { - 0 => cfg.assistant.request.max_response_bytes, - bytes => bytes, - }; + // Every field here is already resolved against the top-level assistant by + // `AppConfig::from_partial_with_defaults`, so an unset inquiry key carries + // the assistant's value rather than a placeholder to fall back from. + let inquiry_cfg = &cfg.conversation.inquiry.assistant; + let sections = build_sections(inquiry_cfg, !tools.is_empty()); + let default_system_prompt = inquiry_cfg.system_prompt.clone(); + let default_max_response_bytes = inquiry_cfg.request.max_response_bytes.bytes(); // Track providers we've already constructed to avoid duplicates. let mut providers: IndexMap> = IndexMap::new(); - // Build the default InquiryConfig from the global inquiry override - // merged with the parent assistant config. - let default_config = if let Some(inquiry_model_cfg) = inquiry_override.model.as_ref() { - let inquiry_model_id = inquiry_model_cfg.id.resolved(); + // Inquiries reuse the caller's provider and model details unless the config + // points them at a different model than the assistant uses, in which case a + // second provider is constructed for it. Comparing against the assistant's + // configured id (rather than the passed-in details) keeps a CLI model + // override on the main request from being mistaken for an inquiry override. + let inquiry_model_id = inquiry_cfg.model.id.resolved(); + let default_config = if inquiry_model_id == cfg.assistant.model.id.resolved() { + providers.insert(model.id.provider, Arc::clone(&provider)); + + InquiryConfig { + provider: Arc::clone(&provider), + model: model.clone(), + system_prompt: default_system_prompt, + sections: sections.clone(), + max_response_bytes: default_max_response_bytes, + } + } else { // Attribute failures to the override: without this, e.g. a missing // API key environment variable renders identically to a main-model // failure and points the user at the wrong config. @@ -952,16 +945,6 @@ async fn build_inquiry_backend( sections: sections.clone(), max_response_bytes: default_max_response_bytes, } - } else { - providers.insert(model.id.provider, Arc::clone(&provider)); - - InquiryConfig { - provider: Arc::clone(&provider), - model: model.clone(), - system_prompt: default_system_prompt, - sections: sections.clone(), - max_response_bytes: default_max_response_bytes, - } }; let overrides = build_inquiry_overrides(cfg, &default_config, &mut providers).await?; @@ -1056,7 +1039,7 @@ async fn build_inquiry_overrides( let max_response_bytes = per_q .request .max_response_bytes - .unwrap_or(default_config.max_response_bytes); + .map_or(default_config.max_response_bytes, MaxResponseBytes::bytes); overrides.insert((tool_name.to_owned(), question_id.clone()), InquiryConfig { provider: inq_provider, 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 3fe664f9c..86580b051 100644 --- a/crates/jp_cli/src/cmd/query/turn_loop_tests.rs +++ b/crates/jp_cli/src/cmd/query/turn_loop_tests.rs @@ -19,7 +19,7 @@ use jp_config::{ AppConfig, PartialAppConfig, assistant::{ PartialAssistantConfig, - request::{CachePolicy, PartialRequestConfig, RequestConfig}, + request::{CachePolicy, MaxResponseBytes, PartialRequestConfig}, }, conversation::tool::{ CommandConfigOrString, QuestionConfig, QuestionTarget, RunMode, ToolConfig, ToolSource, @@ -749,7 +749,7 @@ async fn output_ceiling_ends_turn_without_re_requesting() { let storage = root.join(".jp"); let mut config = AppConfig::new_test(); - config.assistant.request.max_response_bytes = 64; + config.assistant.request.max_response_bytes = MaxResponseBytes::Bytes(64); // A retry budget is left in place so the call-count assertion below has // something to catch: were the ceiling classified as retryable, the loop // would re-request the response instead of ending the turn. @@ -4528,19 +4528,20 @@ fn inquiry_mock_model() -> ModelDetails { }) } -/// The global inquiry override for the output ceiling wins over the parent -/// assistant's value. +/// The global inquiry ceiling wins over the top-level assistant's value. /// /// `conversation.inquiry.assistant.request.max_response_bytes` is a public key, -/// so reading the parent value here would silently ignore it. +/// so reading the assistant value here would silently ignore it. #[tokio::test] async fn inquiry_ceiling_honors_the_global_inquiry_override() { let mut config = AppConfig::new_test(); - config.assistant.request.max_response_bytes = 999_999; - config.conversation.inquiry.assistant.request = Some(RequestConfig { - max_response_bytes: 4096, - ..config.assistant.request - }); + config.assistant.request.max_response_bytes = MaxResponseBytes::Bytes(999_999); + config + .conversation + .inquiry + .assistant + .request + .max_response_bytes = MaxResponseBytes::Bytes(4096); let provider: Arc = Arc::new(MockProvider::new(vec![])); let model = inquiry_mock_model(); @@ -4553,42 +4554,58 @@ async fn inquiry_ceiling_honors_the_global_inquiry_override() { backend .config_for("any_tool", "any_question") .max_response_bytes, - 4096, + Some(4096), "the global inquiry override must win over the parent assistant" ); } -/// A partially-set inquiry request block must not disable the ceiling. +/// Setting one field in the inquiry request block leaves the ceiling inheriting +/// from the assistant rather than resolving to the disable sentinel. /// -/// Built through the real loading path rather than by hand: because -/// `AssistantOverrideConfig::request` is a resolved struct, setting only a -/// sibling field leaves `max_response_bytes` at `0`, which is the ceiling's -/// disable sentinel. -/// Reading it verbatim would silently drop the runaway guard for every inquiry. +/// Built through the real loading path, since the failure this guards against +/// only appears in the partial-to-resolved conversion. #[tokio::test] async fn inquiry_ceiling_survives_a_sibling_only_request_override() { let mut partial = PartialAppConfig::new_test(); - partial.assistant.request.max_response_bytes = Some(500_000); + partial.assistant.request.max_response_bytes = Some(MaxResponseBytes::Bytes(500_000)); - partial.conversation.inquiry.assistant.request = Some(PartialRequestConfig { + partial.conversation.inquiry.assistant.request = PartialRequestConfig { cache: Some(CachePolicy::Off), ..PartialRequestConfig::default() - }); + }; let config = AppConfig::from_partial_with_defaults(partial).expect("valid config"); - // The resolution the guard has to cope with: the block is present, and its - // ceiling field is a zero the user never asked for. + let provider: Arc = Arc::new(MockProvider::new(vec![])); + let model = inquiry_mock_model(); + + let backend = build_inquiry_backend(&config, vec![], model, provider, vec![]) + .await + .expect("the inquiry backend builds"); + assert_eq!( - config - .conversation - .inquiry - .assistant - .request - .expect("the block is set") + backend + .config_for("any_tool", "any_question") .max_response_bytes, - 0 + Some(500_000), + "an unset inquiry ceiling must inherit the assistant, not disable the guard" ); +} + +/// An explicit `0` at the inquiry layer disables the ceiling for inquiries, +/// even when the assistant sets one. +#[tokio::test] +async fn inquiry_ceiling_can_be_disabled_independently() { + let mut partial = PartialAppConfig::new_test(); + partial.assistant.request.max_response_bytes = Some(MaxResponseBytes::Bytes(500_000)); + partial + .conversation + .inquiry + .assistant + .request + .max_response_bytes = Some(MaxResponseBytes::Disabled); + + let config = AppConfig::from_partial_with_defaults(partial).expect("valid config"); let provider: Arc = Arc::new(MockProvider::new(vec![])); let model = inquiry_mock_model(); @@ -4601,24 +4618,26 @@ async fn inquiry_ceiling_survives_a_sibling_only_request_override() { backend .config_for("any_tool", "any_question") .max_response_bytes, - 500_000, - "an unset inquiry ceiling must inherit the parent, not disable the guard" + None, + "an explicit disable removes the ceiling for inquiries" ); } -/// A per-question ceiling wins over the global inquiry override, which in turn -/// wins over the parent assistant (RFD 034's resolution order). +/// A per-question ceiling wins over the global inquiry value, which in turn +/// wins over the top-level assistant (RFD 034's resolution order). #[tokio::test] async fn inquiry_ceiling_honors_the_per_question_override() { let mut config = AppConfig::new_test(); - config.assistant.request.max_response_bytes = 999_999; - config.conversation.inquiry.assistant.request = Some(RequestConfig { - max_response_bytes: 4096, - ..config.assistant.request - }); + config.assistant.request.max_response_bytes = MaxResponseBytes::Bytes(999_999); + config + .conversation + .inquiry + .assistant + .request + .max_response_bytes = MaxResponseBytes::Bytes(4096); let mut per_question = PartialAssistantConfig::default(); - per_question.request.max_response_bytes = Some(512); + per_question.request.max_response_bytes = Some(MaxResponseBytes::Bytes(512)); let mut tool = inquiry_tool_config(&["confirm"]); tool.questions @@ -4642,7 +4661,7 @@ async fn inquiry_ceiling_honors_the_per_question_override() { backend .config_for("inquiry_tool", "confirm") .max_response_bytes, - 512, + Some(512), "the per-question override must win over the global inquiry value" ); @@ -4652,7 +4671,7 @@ async fn inquiry_ceiling_honors_the_per_question_override() { backend .config_for("inquiry_tool", "other") .max_response_bytes, - 4096, + Some(4096), "an unset per-question ceiling falls back to the global inquiry value" ); } diff --git a/crates/jp_config/src/assistant/request.rs b/crates/jp_config/src/assistant/request.rs index 86c998228..a5b610545 100644 --- a/crates/jp_config/src/assistant/request.rs +++ b/crates/jp_config/src/assistant/request.rs @@ -2,7 +2,7 @@ use std::{fmt, time::Duration}; -use schematic::{Config, ConfigError, HandlerError}; +use schematic::{Config, ConfigError, HandlerError, TransformResult}; use serde::{Deserialize, Serialize}; use crate::{ @@ -78,7 +78,10 @@ pub struct RequestConfig { /// Abort a response after it generates more than this many bytes. /// /// Defaults to `1048576` (1 MiB, roughly 260,000 tokens). - /// Set to `0` to disable the ceiling. + /// Accepted values: + /// + /// - a byte count such as `4096` + /// - `0`, `"disabled"`, `"off"`, or `false`: no ceiling /// /// This is a runaway guard rather than a length preference. /// It exists so a model that gets stuck generating without end cannot run @@ -95,16 +98,14 @@ pub struct RequestConfig { /// Inquiry requests can be held to their own ceiling via /// `conversation.inquiry.assistant.request.max_response_bytes`, or per /// question via a question's assistant target. - /// In the `conversation.inquiry` block, `0` means "inherit this ceiling" - /// rather than "disable it"; disable it per question instead. /// /// The ceiling counts bytes rather than tokens because tokens cannot be /// counted locally; four bytes per token is a rough guide. /// It counts the bytes JP receives, which can be fewer than the bytes /// billed: a provider that assembles a response from several continuation /// requests may discard some of what it generated before returning it. - #[setting(default = 1_048_576)] - pub max_response_bytes: u32, + #[setting(default = default_max_response_bytes)] + pub max_response_bytes: MaxResponseBytes, /// Prompt caching policy. /// @@ -153,7 +154,9 @@ impl AssignKeyValue for PartialRequestConfig { "stream_idle_timeout_secs" => { self.stream_idle_timeout_secs = kv.try_some_u32()?; } - "max_response_bytes" => self.max_response_bytes = kv.try_some_u32()?, + "max_response_bytes" => { + self.max_response_bytes = kv.try_some_number_or_from_str()?; + } "cache" => self.cache = kv.try_some_bool_or_from_str()?, _ => return missing_key(&kv), } @@ -214,6 +217,174 @@ impl ToPartial for RequestConfig { } } +/// A ceiling on the bytes of generated content a single response may produce. +/// +/// The ceiling is a runaway guard: it bounds what a model that never stops +/// generating can cost. +/// `Disabled` removes the bound entirely. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum MaxResponseBytes { + /// No ceiling. + /// A response may generate without bound. + Disabled, + + /// Abort the response once it generates more than this many bytes. + Bytes(u32), +} + +/// 1 MiB, roughly 260,000 tokens. +const DEFAULT_MAX_RESPONSE_BYTES: u32 = 1_048_576; + +/// The default output ceiling. +#[expect(clippy::trivially_copy_pass_by_ref, clippy::unnecessary_wraps)] +const fn default_max_response_bytes(_: &()) -> TransformResult> { + Ok(Some(MaxResponseBytes::Bytes(DEFAULT_MAX_RESPONSE_BYTES))) +} + +impl Default for MaxResponseBytes { + fn default() -> Self { + Self::Bytes(DEFAULT_MAX_RESPONSE_BYTES) + } +} + +impl MaxResponseBytes { + /// Build a ceiling from a byte count, where `0` disables it. + /// + /// A zero-byte ceiling would abort every response before its first event, + /// so `0` reads as [`Self::Disabled`] instead, matching the sibling + /// settings that spell "off" the same way. + #[must_use] + pub const fn from_bytes(bytes: u32) -> Self { + match bytes { + 0 => Self::Disabled, + bytes => Self::Bytes(bytes), + } + } + + /// The ceiling in bytes, or `None` when no ceiling applies. + #[must_use] + pub const fn bytes(self) -> Option { + match self { + Self::Disabled => None, + Self::Bytes(bytes) => Some(bytes as u64), + } + } + + /// Returns `true` if no ceiling applies. + #[must_use] + pub const fn is_disabled(self) -> bool { + matches!(self, Self::Disabled) + } +} + +impl From for MaxResponseBytes { + /// `false` disables the ceiling; `true` selects the default ceiling. + fn from(v: bool) -> Self { + if v { Self::default() } else { Self::Disabled } + } +} + +impl std::str::FromStr for MaxResponseBytes { + type Err = String; + + fn from_str(s: &str) -> Result { + match s { + "disabled" | "off" | "false" => Ok(Self::Disabled), + "true" => Ok(Self::default()), + _ => s.parse::().map(Self::from_bytes).map_err(|_| { + format!( + "invalid max response bytes: '{s}', expected a byte count or one of: \ + disabled, off, false" + ) + }), + } + } +} + +impl fmt::Display for MaxResponseBytes { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + match self { + Self::Disabled => write!(f, "disabled"), + Self::Bytes(bytes) => write!(f, "{bytes}"), + } + } +} + +impl Serialize for MaxResponseBytes { + fn serialize(&self, serializer: S) -> Result + where + S: serde::Serializer, + { + match self { + Self::Disabled => serializer.serialize_str("disabled"), + Self::Bytes(bytes) => serializer.serialize_u32(*bytes), + } + } +} + +impl<'de> Deserialize<'de> for MaxResponseBytes { + fn deserialize(deserializer: D) -> Result + where + D: serde::Deserializer<'de>, + { + struct MaxResponseBytesVisitor; + + impl serde::de::Visitor<'_> for MaxResponseBytesVisitor { + type Value = MaxResponseBytes; + + fn expecting(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result { + formatter.write_str("a byte count, \"disabled\", or a boolean") + } + + fn visit_bool(self, v: bool) -> Result { + Ok(MaxResponseBytes::from(v)) + } + + fn visit_u64(self, v: u64) -> Result { + let bytes = u32::try_from(v).map_err(|_| { + serde::de::Error::custom(format!("max response bytes '{v}' exceeds u32")) + })?; + + Ok(MaxResponseBytes::from_bytes(bytes)) + } + + fn visit_i64(self, v: i64) -> Result { + let unsigned = u64::try_from(v).map_err(|_| { + serde::de::Error::custom(format!("max response bytes '{v}' is negative")) + })?; + self.visit_u64(unsigned) + } + + fn visit_str(self, v: &str) -> Result { + v.parse().map_err(serde::de::Error::custom) + } + } + + deserializer.deserialize_any(MaxResponseBytesVisitor) + } +} + +impl schematic::Schematic for MaxResponseBytes { + fn schema_name() -> Option { + Some("MaxResponseBytes".to_owned()) + } + + fn build_schema(mut schema: schematic::SchemaBuilder) -> schematic::Schema { + use schematic::schema::{BooleanType, EnumType, IntegerType, LiteralValue, UnionType}; + + schema.union(UnionType::new_any([ + schema + .nest() + .integer(IntegerType::new_kind(schematic::schema::IntegerKind::U32)), + schema.nest().enumerable(EnumType::new([ + LiteralValue::String("disabled".into()), + LiteralValue::String("off".into()), + ])), + schema.nest().boolean(BooleanType::default()), + ])) + } +} + /// Controls whether the provider should apply prompt caching. /// /// Providers map these values to their native caching mechanisms: diff --git a/crates/jp_config/src/assistant/request_tests.rs b/crates/jp_config/src/assistant/request_tests.rs index ea3ae0228..38da93d4e 100644 --- a/crates/jp_config/src/assistant/request_tests.rs +++ b/crates/jp_config/src/assistant/request_tests.rs @@ -12,7 +12,7 @@ fn request_config(stream_idle_timeout_secs: u32) -> RequestConfig { base_backoff_ms: 1000, max_backoff_secs: 60, stream_idle_timeout_secs, - max_response_bytes: 1_048_576, + max_response_bytes: MaxResponseBytes::default(), cache: CachePolicy::default(), } } @@ -55,13 +55,65 @@ fn test_request_config_assign() { let kv = KvAssignment::try_from_cli("max_response_bytes", "4096").unwrap(); p.assign(kv).unwrap(); - assert_eq!(p.max_response_bytes, Some(4096)); + assert_eq!(p.max_response_bytes, Some(MaxResponseBytes::Bytes(4096))); - // `0` is the documented way to disable the ceiling, so it must survive - // assignment rather than being treated as "unset". + let kv = KvAssignment::try_from_cli("max_response_bytes", "disabled").unwrap(); + p.assign(kv).unwrap(); + assert_eq!(p.max_response_bytes, Some(MaxResponseBytes::Disabled)); + + // `0` disables the ceiling, matching the sibling settings that spell "off" + // that way. let kv = KvAssignment::try_from_cli("max_response_bytes", "0").unwrap(); p.assign(kv).unwrap(); - assert_eq!(p.max_response_bytes, Some(0)); + assert_eq!(p.max_response_bytes, Some(MaxResponseBytes::Disabled)); +} + +#[test] +fn max_response_bytes_round_trips_through_json() { + for (value, expected) in [ + ("4096", MaxResponseBytes::Bytes(4096)), + ("0", MaxResponseBytes::Disabled), + ("\"disabled\"", MaxResponseBytes::Disabled), + ("\"off\"", MaxResponseBytes::Disabled), + ("false", MaxResponseBytes::Disabled), + ] { + let parsed: MaxResponseBytes = serde_json::from_str(value).expect(value); + assert_eq!(parsed, expected, "parsing {value}"); + + // Re-serializing and re-parsing must land on the same value, so a + // stored conversation config keeps its meaning. + let encoded = serde_json::to_string(&parsed).expect("serializes"); + let reparsed: MaxResponseBytes = serde_json::from_str(&encoded).expect(&encoded); + assert_eq!(reparsed, expected, "round-tripping {value} via {encoded}"); + } +} + +#[test] +fn max_response_bytes_rejects_a_nonsense_value() { + let err = "banana".parse::().unwrap_err(); + assert!(err.contains("expected a byte count"), "got: {err}"); +} + +/// `0` disables the ceiling on every input path. +/// +/// A zero-byte ceiling would abort every response before its first event, so no +/// path may produce `Bytes(0)`. +#[test] +fn zero_disables_the_ceiling() { + assert_eq!(MaxResponseBytes::from_bytes(0), MaxResponseBytes::Disabled); + assert_eq!( + "0".parse::().unwrap(), + MaxResponseBytes::Disabled + ); + assert_eq!( + serde_json::from_str::("0").unwrap(), + MaxResponseBytes::Disabled + ); + assert_eq!( + MaxResponseBytes::from_bytes(0).bytes(), + None, + "a disabled ceiling never yields a zero byte limit" + ); } #[test] diff --git a/crates/jp_config/src/conversation.rs b/crates/jp_config/src/conversation.rs index 17b272a13..d028a17ac 100644 --- a/crates/jp_config/src/conversation.rs +++ b/crates/jp_config/src/conversation.rs @@ -13,22 +13,17 @@ use serde::{Deserialize, Serialize}; use crate::{ BoxedError, assignment::{AssignKeyValue, AssignResult, KvAssignment, missing_key}, - assistant::{ - request::{PartialRequestConfig, RequestConfig}, - sections::SectionConfig, - tool_choice::ToolChoice, - }, + assistant::{AssistantConfig, PartialAssistantConfig}, conversation::{ attachment::{AttachmentConfig, PartialAttachmentConfig}, compaction::{CompactionConfig, PartialCompactionConfig}, title::{PartialTitleConfig, TitleConfig}, tool::{PartialToolsConfig, ToolsConfig}, }, - delta::{PartialConfigDelta, delta_opt, delta_opt_partial, delta_vec}, - fill::{self, FillDefaults}, + delta::{PartialConfigDelta, delta_opt}, + fill::FillDefaults, internal::merge::vec_with_strategy, - model::{ModelConfig, PartialModelConfig}, - partial::{ToPartial, partial_opt, partial_opts}, + partial::{ToPartial, partial_opt}, types::vec::{MergeableVec, MergedVec, vec_to_mergeable_partial}, validate::Validator, }; @@ -177,11 +172,15 @@ impl ToPartial for ConversationConfig { #[derive(Debug, Clone, PartialEq, Config)] #[config(rename_all = "snake_case")] pub struct InquiryConfig { - /// Assistant overrides for inquiry requests. + /// Assistant settings for inquiry requests. /// - /// Unset fields fall back to the parent assistant config. + /// Accepts every key from the top-level `assistant` section. + /// Keys left unset here take the value from `assistant`, so + /// `conversation.inquiry.assistant.model.id` can point an inquiry at a + /// cheaper model while its system prompt and request settings stay whatever + /// the main assistant uses. #[setting(nested)] - pub assistant: AssistantOverrideConfig, + pub assistant: AssistantConfig, } impl AssignKeyValue for PartialInquiryConfig { @@ -220,95 +219,6 @@ impl ToPartial for InquiryConfig { } } -/// Assistant configuration overrides for inquiry requests. -/// -/// Mirrors [`AssistantConfig`] but with all fields optional and no defaults. -/// Unset fields are filled from the parent assistant config at runtime. -/// -/// [`AssistantConfig`]: crate::assistant::AssistantConfig -#[derive(Debug, Clone, PartialEq, Config)] -#[config(rename_all = "snake_case")] -pub struct AssistantOverrideConfig { - /// Override the system prompt for inquiry requests. - pub system_prompt: Option, - - /// Override the system prompt sections. - #[setting(nested, merge = schematic::merge::append_vec)] - pub system_prompt_sections: Vec, - - /// Override the tool choice. - pub tool_choice: Option, - - /// Override the model. - #[setting(nested)] - pub model: Option, - - /// Override request behavior (retries, caching). - #[setting(nested)] - pub request: Option, -} - -impl AssignKeyValue for PartialAssistantOverrideConfig { - fn assign(&mut self, mut kv: KvAssignment) -> AssignResult { - match kv.key_string().as_str() { - "" => kv.try_merge_object(self)?, - "system_prompt" => self.system_prompt = kv.try_some_string()?, - _ if kv.p("system_prompt_sections") => { - kv.try_vec_of_nested(&mut self.system_prompt_sections)?; - } - "tool_choice" => self.tool_choice = kv.try_some_from_str()?, - _ if kv.p("model") => self.model.assign(kv)?, - _ if kv.p("request") => self.request.assign(kv)?, - _ => return missing_key(&kv), - } - - Ok(()) - } -} - -impl PartialConfigDelta for PartialAssistantOverrideConfig { - fn delta(&self, next: Self) -> Self { - Self { - system_prompt: delta_opt(self.system_prompt.as_ref(), next.system_prompt), - system_prompt_sections: delta_vec( - &self.system_prompt_sections, - next.system_prompt_sections, - ), - tool_choice: delta_opt(self.tool_choice.as_ref(), next.tool_choice), - model: delta_opt_partial(self.model.as_ref(), next.model), - request: delta_opt_partial(self.request.as_ref(), next.request), - } - } -} - -impl FillDefaults for PartialAssistantOverrideConfig { - fn fill_from(self, defaults: Self) -> Self { - Self { - system_prompt: self.system_prompt.or(defaults.system_prompt), - system_prompt_sections: self.system_prompt_sections, - tool_choice: self.tool_choice.or(defaults.tool_choice), - model: fill::fill_opt(self.model, defaults.model), - request: fill::fill_opt(self.request, defaults.request), - } - } -} - -impl ToPartial for AssistantOverrideConfig { - fn to_partial(&self) -> Self::Partial { - Self::Partial { - system_prompt: partial_opts(self.system_prompt.as_ref(), None), - system_prompt_sections: self - .system_prompt_sections - .iter() - .map(ToPartial::to_partial) - .collect(), - tool_choice: partial_opts(self.tool_choice.as_ref(), None), - model: self.model.as_ref().map(ToPartial::to_partial), - request: self.request.as_ref().map(ToPartial::to_partial), - } - } -} - /// Which conversation to default to when no session mapping exists. /// /// This is read during conversation resolution, before the full config is diff --git a/crates/jp_config/src/conversation/tool.rs b/crates/jp_config/src/conversation/tool.rs index 4dba4caa8..4d8141bd2 100644 --- a/crates/jp_config/src/conversation/tool.rs +++ b/crates/jp_config/src/conversation/tool.rs @@ -1505,7 +1505,11 @@ impl Enable { /// The legacy strings `"on"`, `"off"`, `"always"` (= locked-on), and /// `"explicit"` (= off-unless-named) are still accepted on input. #[derive(Debug, Clone, PartialEq, Config)] -#[config(rename_all = "snake_case", no_deserialize_derive)] +#[config( + rename_all = "snake_case", + no_deserialize_derive, + schema_union_with = enable_input_shapes +)] pub struct EnableConfig { /// Whether the tool is enabled. /// @@ -1568,6 +1572,25 @@ impl PartialEnableConfig { } } +/// The non-table shapes `enable` accepts, for the schema. +/// +/// The table form is described by the derived struct schema; these are the +/// shorthands its hand-written `Deserialize` also accepts, which the derive +/// cannot see. +fn enable_input_shapes(schema: &schematic::SchemaBuilder) -> Vec { + use schematic::schema::{BooleanType, EnumType, LiteralValue}; + + vec![ + schema.nest().boolean(BooleanType::default()), + schema.nest().enumerable(EnumType::new([ + LiteralValue::String("on".into()), + LiteralValue::String("off".into()), + LiteralValue::String("always".into()), + LiteralValue::String("explicit".into()), + ])), + ] +} + impl From for PartialEnableConfig { fn from(state: bool) -> Self { Self { diff --git a/crates/jp_config/src/conversation/tool_tests.rs b/crates/jp_config/src/conversation/tool_tests.rs index 607e796c1..eb9fc7b06 100644 --- a/crates/jp_config/src/conversation/tool_tests.rs +++ b/crates/jp_config/src/conversation/tool_tests.rs @@ -1,5 +1,5 @@ use assert_matches::assert_matches; -use schematic::{SchemaBuilder, SchemaType}; +use schematic::{SchemaBuilder, SchemaType, schema::LiteralValue}; use serde_json::json; use super::*; @@ -395,22 +395,55 @@ fn test_tool_config_command() { }); } +/// `EnableConfig`'s schema describes every shape its `Deserialize` accepts: a +/// bool, one of the legacy strings, or the `{ state, allow_toggle }` table. +/// +/// The hand-written `Deserialize` accepts more than the derived struct schema +/// can infer, so the extra shapes are declared via `schema_union_with`. +/// Without them a schema consumer would reject `enable = true`, which is the +/// form most configs use. #[test] fn test_enable_schema() { - // `EnableConfig`'s root schema is a struct exposing `state` and - // `allow_toggle` as real fields (not a bool|string union like the old - // `Enable`). This is the type's own schema: in `AppConfig::fields()` the - // `enable` field stays a flat leaf (`conversation.tools.*.enable`) because - // it's a `no_deserialize_derive` config. let schema = SchemaBuilder::build_root::(); - let SchemaType::Struct(s) = schema.ty else { - panic!("expected struct, got {:?}", schema.ty) + let SchemaType::Union(union) = schema.ty else { + panic!("expected a union, got {:?}", schema.ty) }; - assert!(s.fields.contains_key("state"), "missing `state` field"); + + let mut has_bool = false; + let mut legacy_strings = None; + let mut table_fields = None; + + for variant in union.variants_types { + match variant.ty { + SchemaType::Boolean(_) => has_bool = true, + SchemaType::Enum(e) => legacy_strings = Some(e.values), + SchemaType::Struct(s) => table_fields = Some(s.fields), + ty => panic!("unexpected variant: {ty:?}"), + } + } + + assert!(has_bool, "`enable = true` must be described"); + + let legacy = legacy_strings.expect("the legacy string forms are described"); + assert_eq!(legacy, [ + LiteralValue::String("on".into()), + LiteralValue::String("off".into()), + LiteralValue::String("always".into()), + LiteralValue::String("explicit".into()), + ]); + + // The table form keeps the derived field schema, so field names and their + // doc comments still reach the schema. + let fields = table_fields.expect("the table form is described"); + assert!(fields.contains_key("state"), "missing `state` field"); assert!( - s.fields.contains_key("allow_toggle"), + fields.contains_key("allow_toggle"), "missing `allow_toggle` field" ); + assert!( + fields["state"].comment.is_some(), + "field doc comments survive the union" + ); } #[test] diff --git a/crates/jp_config/src/lib.rs b/crates/jp_config/src/lib.rs index 46c038d74..928fac081 100644 --- a/crates/jp_config/src/lib.rs +++ b/crates/jp_config/src/lib.rs @@ -92,6 +92,22 @@ pub const ENV_PREFIX: &str = "JP_CFG_"; /// Convenience type for boxed errors. type BoxedError = Box; +/// What a user can write at a config path. +/// +/// A path takes a value, names the keys of a nested block, or both when a block +/// also accepts a shorthand value. +enum Addressable { + /// A value at this path, with nothing nested under it. + Value, + + /// Keys under this path, with no value at the path itself. + Keys(Schema), + + /// Either a shorthand value at this path, or the keys of the form it + /// abbreviates. + ValueOrKeys(Schema), +} + /// The global configuration for Jean Pierre. #[derive(Debug, Clone, PartialEq, Config)] #[config(rename_all = "snake_case")] @@ -279,7 +295,7 @@ impl ToPartial for AppConfig { fn to_partial(&self) -> Self::Partial { let defaults = Self::Partial::default(); - Self::Partial { + 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), extends: partial_opt(&self.extends, defaults.extends), @@ -292,7 +308,19 @@ impl ToPartial for AppConfig { providers: self.providers.to_partial(), plugins: self.plugins.to_partial(), user: self.user.to_partial(), - } + }; + + // Inquiry settings are resolved by inheriting from the top-level + // assistant, so a field that merely equals the assistant's value holds + // no choice of the user's. Keeping only the differences is what lets a + // later layer that changes `assistant` still reach the inquiry: a + // recorded copy of the inherited value would pin it instead. + partial.conversation.inquiry.assistant = self + .assistant + .to_partial() + .delta(partial.conversation.inquiry.assistant); + + partial } } @@ -315,7 +343,17 @@ impl AppConfig { /// /// Returns an error if `default_values` fails, if the partial is missing /// required fields, or if the resolved configuration fails validation. - pub fn from_partial_with_defaults(partial: PartialAppConfig) -> Result { + pub fn from_partial_with_defaults(mut partial: PartialAppConfig) -> Result { + // Inquiry settings inherit from the top-level assistant. This runs + // before the `#[setting(default)]` layer below, so an unset inquiry + // field takes the user's configured assistant value rather than the + // type's default. + partial.conversation.inquiry.assistant = partial + .conversation + .inquiry + .assistant + .fill_from(partial.assistant.clone()); + let partial = match PartialAppConfig::default_values(&())? { Some(defaults) => partial.fill_from(defaults), None => partial, @@ -386,9 +424,13 @@ impl AppConfig { format!("{prefix}.{name}") }; - match field.schema.ty { - SchemaType::Struct(_) => stack.push((field.schema, path)), - _ => output.push(path), + match Self::addressable(field.schema) { + Addressable::Value => output.push(path), + Addressable::Keys(inner) => stack.push((inner, path)), + Addressable::ValueOrKeys(inner) => { + output.push(path.clone()); + stack.push((inner, path)); + } } } } @@ -396,6 +438,45 @@ impl AppConfig { output } + /// What a schema lets a user write at its own path. + /// + /// `Option` wraps a named union in a second union rather than merging + /// into it, so this recurses through both levels. + fn addressable(schema: Schema) -> Addressable { + let SchemaType::Union(union) = schema.ty else { + return match schema.ty { + SchemaType::Struct(_) => Addressable::Keys(schema), + _ => Addressable::Value, + }; + }; + + // A union naming an expanded form describes one value written several + // ways: the shorthand goes at this path, and the expanded form names the + // keys. Both are writable, so both are reported. + if let Some(expanded) = union.expanded_variant() { + return match Self::addressable(expanded.clone()) { + Addressable::Keys(inner) | Addressable::ValueOrKeys(inner) => { + Addressable::ValueOrKeys(inner) + } + Addressable::Value => Addressable::Value, + }; + } + + // Otherwise the variants are distinct values. Only `Option` resolves + // to something addressable, since dropping null leaves one shape; a union + // of several real shapes has no single set of keys, and which keys apply + // depends on what the user wrote (`editor.cmd = "code"` has no `args`). + let mut variants = union + .variants_types + .into_iter() + .filter(|variant| !matches!(variant.ty, SchemaType::Null)); + + match (variants.next(), variants.next()) { + (Some(only), None) => Self::addressable(*only), + _ => Addressable::Value, + } + } + /// Return a list of all environment variable names in the configuration. /// /// ```rust @@ -469,11 +550,15 @@ impl AppConfig { .resolve_in_place(aliases) .map_err(|e| Error::Custom(format!("assistant.model.id: {e}").into()))?; - if let Some(ref mut model) = self.conversation.inquiry.assistant.model { - model.id.resolve_in_place(aliases).map_err(|e| { + self.conversation + .inquiry + .assistant + .model + .id + .resolve_in_place(aliases) + .map_err(|e| { Error::Custom(format!("conversation.inquiry.assistant.model.id: {e}").into()) })?; - } if let Some(ref mut model) = self.conversation.title.generate.model { model.id.resolve_in_place(aliases).map_err(|e| { @@ -585,10 +670,12 @@ impl PartialAppConfig { aliases: &indexmap::IndexMap, ) { self.assistant.model.id.resolve_in_place(aliases); - - if let Some(ref mut model) = self.conversation.inquiry.assistant.model { - model.id.resolve_in_place(aliases); - } + self.conversation + .inquiry + .assistant + .model + .id + .resolve_in_place(aliases); if let Some(ref mut model) = self.conversation.title.generate.model { model.id.resolve_in_place(aliases); diff --git a/crates/jp_config/src/lib_tests.rs b/crates/jp_config/src/lib_tests.rs index 241fba266..ab9613e53 100644 --- a/crates/jp_config/src/lib_tests.rs +++ b/crates/jp_config/src/lib_tests.rs @@ -25,49 +25,261 @@ fn test_app_config_fields() { insta::assert_debug_snapshot!(AppConfig::fields()); } -/// Setting one field in the inquiry request block must not silently zero the -/// rest of it. -/// -/// `AssistantOverrideConfig::request` is a resolved `Option`, so -/// the conversion has no per-field presence to preserve: every field the user -/// did not set resolves to Rust's `Default` rather than the schematic default. -/// This test pins that behavior so a reader of the resolution code knows the -/// zeros are an artifact, not a user's choice. +/// Setting one field in the inquiry request block leaves its siblings +/// inheriting from the top-level assistant, rather than resolving to `0`. #[test] -fn inquiry_request_override_zeroes_unset_fields() { - use crate::assistant::request::CachePolicy; +fn inquiry_inherits_unset_request_fields_from_the_assistant() { + use crate::assistant::request::{CachePolicy, MaxResponseBytes, PartialRequestConfig}; let mut partial = PartialAppConfig::new_test(); - partial.assistant.request.max_response_bytes = Some(500_000); + partial.assistant.request.max_response_bytes = Some(MaxResponseBytes::Bytes(500_000)); + partial.assistant.request.max_retries = Some(7); // Only a sibling field is set in the inquiry block. - partial.conversation.inquiry.assistant.request = - Some(crate::assistant::request::PartialRequestConfig { - cache: Some(CachePolicy::Off), - ..Default::default() - }); + partial.conversation.inquiry.assistant.request = PartialRequestConfig { + cache: Some(CachePolicy::Off), + ..PartialRequestConfig::default() + }; let config = AppConfig::from_partial_with_defaults(partial).expect("valid config"); + let request = config.conversation.inquiry.assistant.request; + + assert_eq!(request.cache, CachePolicy::Off, "the set field survives"); + assert_eq!( + request.max_response_bytes, + MaxResponseBytes::Bytes(500_000), + "an unset sibling inherits the assistant value" + ); + assert_eq!( + request.max_retries, 7, + "inheritance covers every field in the block" + ); + + // The parent keeps its own value; the fill is one-directional. + assert_eq!( + config.assistant.request.max_response_bytes, + MaxResponseBytes::Bytes(500_000) + ); +} - let request = config +/// An explicit inquiry value wins over the inherited assistant value. +#[test] +fn inquiry_request_override_wins_over_the_assistant() { + use crate::assistant::request::MaxResponseBytes; + + let mut partial = PartialAppConfig::new_test(); + partial.assistant.request.max_response_bytes = Some(MaxResponseBytes::Bytes(500_000)); + partial .conversation .inquiry .assistant .request - .expect("the inquiry request block is set"); + .max_response_bytes = Some(MaxResponseBytes::Bytes(4096)); - assert_eq!(request.cache, CachePolicy::Off, "the set field survives"); + let config = AppConfig::from_partial_with_defaults(partial).expect("valid config"); + + assert_eq!( + config + .conversation + .inquiry + .assistant + .request + .max_response_bytes, + MaxResponseBytes::Bytes(4096) + ); + assert_eq!( + config.assistant.request.max_response_bytes, + MaxResponseBytes::Bytes(500_000) + ); +} + +/// Disabling the ceiling for inquiries alone survives inheritance. +#[test] +fn inquiry_can_disable_a_ceiling_the_assistant_sets() { + use crate::assistant::request::MaxResponseBytes; + + let mut partial = PartialAppConfig::new_test(); + partial.assistant.request.max_response_bytes = Some(MaxResponseBytes::Bytes(500_000)); + partial + .conversation + .inquiry + .assistant + .request + .max_response_bytes = Some(MaxResponseBytes::Disabled); + + let config = AppConfig::from_partial_with_defaults(partial).expect("valid config"); + + assert_eq!( + config + .conversation + .inquiry + .assistant + .request + .max_response_bytes, + MaxResponseBytes::Disabled, + "an explicit disable must not be overwritten by the inherited value" + ); +} + +/// A round-trip through `to_partial` must not freeze an inherited inquiry +/// value. +/// +/// `to_partial` is how a resolved config becomes a layer again (a stored +/// conversation config, a `--cfg` baseline). +/// If it recorded the inherited inquiry values verbatim, a later layer that +/// changes `assistant` would no longer reach the inquiry, which is the +/// inheritance silently stopping. +#[test] +fn inquiry_inheritance_survives_a_partial_round_trip() { + use crate::model::id::{ModelIdConfig, PartialModelIdOrAliasConfig, ProviderId}; + + let mut partial = PartialAppConfig::new_test(); + partial.assistant.model.id = ModelIdConfig { + provider: ProviderId::Anthropic, + name: "first-model".parse().unwrap(), + } + .to_partial() + .into(); + + let config = AppConfig::from_partial_with_defaults(partial).expect("valid config"); assert_eq!( - request.max_response_bytes, 0, - "an unset field resolves to Rust's Default, not the schematic default" + config + .conversation + .inquiry + .assistant + .model + .id + .resolved() + .name + .as_ref(), + "first-model", + "the inquiry inherits the assistant model" ); + + // Round-trip, then change only the assistant on a later layer. + let mut round_tripped = config.to_partial(); + round_tripped.assistant.model.id = PartialModelIdOrAliasConfig::Id( + ModelIdConfig { + provider: ProviderId::Anthropic, + name: "second-model".parse().unwrap(), + } + .to_partial(), + ); + + let config = AppConfig::from_partial_with_defaults(round_tripped).expect("valid config"); + assert_eq!( - request.max_retries, 0, - "the same applies to every other field in the block" + config + .conversation + .inquiry + .assistant + .model + .id + .resolved() + .name + .as_ref(), + "second-model", + "the inquiry must follow the new assistant model, not the round-tripped copy" ); +} + +/// An inquiry value the user genuinely set survives the same round-trip. +#[test] +fn an_explicit_inquiry_value_survives_a_partial_round_trip() { + use crate::assistant::request::MaxResponseBytes; - // The parent keeps its own value; nothing merged into it. - assert_eq!(config.assistant.request.max_response_bytes, 500_000); + let mut partial = PartialAppConfig::new_test(); + partial.assistant.request.max_response_bytes = Some(MaxResponseBytes::Bytes(500_000)); + partial + .conversation + .inquiry + .assistant + .request + .max_response_bytes = Some(MaxResponseBytes::Bytes(4096)); + + let config = AppConfig::from_partial_with_defaults(partial).expect("valid config"); + let config = AppConfig::from_partial_with_defaults(config.to_partial()).expect("valid config"); + + assert_eq!( + config + .conversation + .inquiry + .assistant + .request + .max_response_bytes, + MaxResponseBytes::Bytes(4096), + "an explicitly-set inquiry value is not mistaken for an inherited one" + ); +} + +/// A union that names an expanded form contributes both the shorthand path and +/// the expanded keys; a union of distinct values contributes only its path. +/// +/// `enable` accepts `true` or `{ state, allow_toggle }`, and `editor.cmd` +/// accepts `"code --wait"` or `{ program, args }`. +/// Both spell one value two ways, so every spelling is writable. +/// `assistant.model.id` is either an id or an alias resolved through a lookup, +/// which are different values rather than spellings of one, so it contributes +/// only its own path. +#[test] +fn fields_follows_a_unions_expanded_form() { + let fields = AppConfig::fields(); + let has = |key: &str| fields.contains(&key.to_owned()); + + assert!( + has("conversation.tools.*.enable"), + "the shorthand `enable = true` stays writable" + ); + assert!( + has("conversation.tools.*.enable.state") && has("conversation.tools.*.enable.allow_toggle"), + "the expanded form contributes its keys" + ); + + assert!( + has("editor.cmd"), + "the shorthand `cmd = \"code\"` stays writable" + ); + assert!( + has("editor.cmd.program") && has("editor.cmd.args") && has("editor.cmd.shell"), + "the table form contributes its keys" + ); + + assert!(has("assistant.model.id"), "an id-or-alias is a leaf"); + assert!( + !has("assistant.model.id.provider"), + "an alias is not a spelling of an id, so its keys are not reported" + ); +} + +/// Every assignable sub-field of an optional nested config appears in +/// `fields()`. +/// +/// `Option` renders as a nullable union rather than a struct, so +/// a walk that only descends into structs stops at the block and reports it as +/// a leaf. +/// `assign` routes into those sub-keys regardless, and `envs()` is derived from +/// `fields()`, so the omission silently costs the env-var form of every key +/// inside such a block. +#[test] +fn fields_descends_into_optional_nested_configs() { + let fields = AppConfig::fields(); + + for key in [ + "conversation.title.generate.model.id", + "style.reasoning.summary_model.id", + ] { + assert!( + fields.contains(&key.to_owned()), + "{key} is assignable but missing from fields()" + ); + } + + // The block itself is no longer reported as a leaf: it has no value of its + // own to set. + assert!( + !fields.contains(&"conversation.title.generate.model".to_owned()), + "the containing block must not also appear as a leaf" + ); } #[test] diff --git a/crates/jp_config/src/model/id.rs b/crates/jp_config/src/model/id.rs index 72c7520f6..9d4288296 100644 --- a/crates/jp_config/src/model/id.rs +++ b/crates/jp_config/src/model/id.rs @@ -69,6 +69,10 @@ impl FillDefaults for PartialModelIdOrAliasConfig { fn fill_from(self, defaults: Self) -> Self { match (self, defaults) { (Self::Id(s), Self::Id(d)) => Self::Id(s.fill_from(d)), + // A value carrying no information takes the default whole, even + // across variants: an unset id filling from a config that names its + // model by alias has to end up with that alias. + (s, d) if s.is_empty() => d, (s, _) => s, } } 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 a68bde87c..bbcd6e98e 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 @@ -24,7 +24,14 @@ expression: "AppConfig::fields()" "style.reasoning.background", "style.reasoning.display", "style.reasoning.extend_across_tool_calls", - "style.reasoning.summary_model", + "style.reasoning.summary_model.id", + "style.reasoning.summary_model.parameters.max_tokens", + "style.reasoning.summary_model.parameters.other", + "style.reasoning.summary_model.parameters.reasoning", + "style.reasoning.summary_model.parameters.stop_words", + "style.reasoning.summary_model.parameters.temperature", + "style.reasoning.summary_model.parameters.top_k", + "style.reasoning.summary_model.parameters.top_p", "style.mcp_startup.delay_secs", "style.mcp_startup.interval_ms", "style.mcp_startup.show", @@ -74,6 +81,9 @@ expression: "AppConfig::fields()" "editor.cmd", "editor.envs", "editor.inline.edit_mode", + "editor.cmd.args", + "editor.cmd.program", + "editor.cmd.shell", "conversation.attachments", "conversation.default_id", "conversation.start_local", @@ -89,14 +99,37 @@ expression: "AppConfig::fields()" "conversation.tools.*.style.results_file_link", "conversation.tools.*.style.error.inline_results", "conversation.tools.*.style.error.results_file_link", + "conversation.tools.*.enable.allow_toggle", + "conversation.tools.*.enable.state", "conversation.title.from_heading", "conversation.title.generate.auto", - "conversation.title.generate.model", - "conversation.inquiry.assistant.model", - "conversation.inquiry.assistant.request", + "conversation.title.generate.model.id", + "conversation.title.generate.model.parameters.max_tokens", + "conversation.title.generate.model.parameters.other", + "conversation.title.generate.model.parameters.reasoning", + "conversation.title.generate.model.parameters.stop_words", + "conversation.title.generate.model.parameters.temperature", + "conversation.title.generate.model.parameters.top_k", + "conversation.title.generate.model.parameters.top_p", + "conversation.inquiry.assistant.instructions", + "conversation.inquiry.assistant.name", "conversation.inquiry.assistant.system_prompt", "conversation.inquiry.assistant.system_prompt_sections", "conversation.inquiry.assistant.tool_choice", + "conversation.inquiry.assistant.request.base_backoff_ms", + "conversation.inquiry.assistant.request.cache", + "conversation.inquiry.assistant.request.max_backoff_secs", + "conversation.inquiry.assistant.request.max_response_bytes", + "conversation.inquiry.assistant.request.max_retries", + "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.reasoning", + "conversation.inquiry.assistant.model.parameters.stop_words", + "conversation.inquiry.assistant.model.parameters.temperature", + "conversation.inquiry.assistant.model.parameters.top_k", + "conversation.inquiry.assistant.model.parameters.top_p", "conversation.compaction.rules", "assistant.instructions", "assistant.name", 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 ada2eff92..3c56d17d3 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 @@ -79,12 +79,41 @@ PartialAppConfig { [], ), inquiry: PartialInquiryConfig { - assistant: PartialAssistantOverrideConfig { + assistant: PartialAssistantConfig { + name: None, system_prompt: None, - system_prompt_sections: [], + system_prompt_sections: Vec( + [], + ), + instructions: Vec( + [], + ), tool_choice: None, - model: None, - request: None, + model: PartialModelConfig { + id: Id( + PartialModelIdConfig { + provider: None, + name: None, + }, + ), + parameters: PartialParametersConfig { + max_tokens: None, + reasoning: None, + temperature: None, + top_p: None, + top_k: None, + stop_words: None, + other: None, + }, + }, + request: PartialRequestConfig { + max_retries: None, + base_backoff_ms: None, + max_backoff_secs: None, + stream_idle_timeout_secs: None, + max_response_bytes: None, + cache: None, + }, }, }, start_local: 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 5ca956219..8f36db725 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 @@ -91,7 +91,9 @@ Ok( 60, ), max_response_bytes: Some( - 1048576, + Bytes( + 1048576, + ), ), cache: None, }, @@ -165,12 +167,89 @@ Ok( }, ), inquiry: PartialInquiryConfig { - assistant: PartialAssistantOverrideConfig { - system_prompt: None, - system_prompt_sections: [], + assistant: PartialAssistantConfig { + name: None, + system_prompt: Some( + Merged( + PartialMergedString { + value: Some( + "You are a helpful assistant.", + ), + strategy: None, + separator: None, + discard_when_merged: Some( + true, + ), + }, + ), + ), + system_prompt_sections: Vec( + [], + ), + instructions: Merged( + MergedVec { + value: [ + PartialInstructionsConfig { + title: Some( + "How to respond to the user", + ), + description: None, + position: None, + items: Some( + [ + "Be concise", + "Use simple sentences. But feel free to use technical jargon.", + "Do NOT overexplain basic concepts. Assume the user is technically proficient.", + "AVOID flattering, corporate-ish or marketing language. Maintain a neutral viewpoint.", + "AVOID vague and / or generic claims which may seem correct but are not substantiated by the context.", + ], + ), + examples: [], + }, + ], + strategy: None, + dedup: None, + discard_when_merged: true, + }, + ), tool_choice: None, - model: None, - request: None, + model: PartialModelConfig { + id: Id( + PartialModelIdConfig { + provider: None, + name: None, + }, + ), + parameters: PartialParametersConfig { + max_tokens: None, + reasoning: None, + temperature: None, + top_p: None, + top_k: None, + stop_words: None, + other: None, + }, + }, + request: PartialRequestConfig { + max_retries: Some( + 5, + ), + base_backoff_ms: Some( + 1000, + ), + max_backoff_secs: Some( + 60, + ), + stream_idle_timeout_secs: Some( + 60, + ), + max_response_bytes: Some( + Bytes( + 1048576, + ), + ), + cache: None, + }, }, }, start_local: 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 d9cd8343a..d553ec68a 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 @@ -79,12 +79,41 @@ PartialAppConfig { [], ), inquiry: PartialInquiryConfig { - assistant: PartialAssistantOverrideConfig { + assistant: PartialAssistantConfig { + name: None, system_prompt: None, - system_prompt_sections: [], + system_prompt_sections: Vec( + [], + ), + instructions: Vec( + [], + ), tool_choice: None, - model: None, - request: None, + model: PartialModelConfig { + id: Id( + PartialModelIdConfig { + provider: None, + name: None, + }, + ), + parameters: PartialParametersConfig { + max_tokens: None, + reasoning: None, + temperature: None, + top_p: None, + top_k: None, + stop_words: None, + other: None, + }, + }, + request: PartialRequestConfig { + max_retries: None, + base_backoff_ms: None, + max_backoff_secs: None, + stream_idle_timeout_secs: None, + max_response_bytes: None, + cache: None, + }, }, }, start_local: None, diff --git a/crates/jp_config/src/types/command.rs b/crates/jp_config/src/types/command.rs index e8ba761c6..aad0b14b0 100644 --- a/crates/jp_config/src/types/command.rs +++ b/crates/jp_config/src/types/command.rs @@ -40,7 +40,11 @@ pub enum CommandConfigOrString { String(String), /// A complete command configuration. - #[setting(nested)] + /// + /// Marked as the expanded form: the string above is a shorthand for this + /// table, so `cmd.program` and `cmd.args` address a command however it was + /// written. + #[setting(nested, expanded)] Config(CommandConfig), } @@ -57,8 +61,16 @@ impl AssignKeyValue for PartialCommandConfigOrString { fn assign(&mut self, kv: KvAssignment) -> AssignResult { match kv.key_string().as_str() { "" => *self = kv.try_object_or_from_str()?, + + // A key addressing the table's fields expands the shorthand first, + // rather than discarding the program it named. `editor.cmd = "code + // --wait"` followed by `editor.cmd.args = ["--foo"]` keeps `code`. _ => match self { - Self::String(_) => return missing_key(&kv), + Self::String(shorthand) => { + let mut config = expand_shorthand(shorthand); + config.assign(kv)?; + *self = Self::Config(config); + } Self::Config(config) => config.assign(kv)?, }, } @@ -67,6 +79,24 @@ impl AssignKeyValue for PartialCommandConfigOrString { } } +/// Expand the string shorthand into the table form it abbreviates. +/// +/// Splits exactly as [`CommandConfigOrString::command`] does, so expanding +/// before assigning a field cannot change which command ends up running. +/// `shell` and any empty part are left unset so their defaults still apply, and +/// so a later layer can still fill them. +fn expand_shorthand(shorthand: &str) -> PartialCommandConfig { + let mut tokens = shlex::split(shorthand).unwrap_or_default().into_iter(); + let program = tokens.next(); + let args: Vec<_> = tokens.collect(); + + PartialCommandConfig { + program, + args: (!args.is_empty()).then_some(args), + shell: None, + } +} + impl PartialConfigDelta for PartialCommandConfigOrString { fn delta(&self, next: Self) -> Self { match (self, next) { diff --git a/crates/jp_config/src/types/command_tests.rs b/crates/jp_config/src/types/command_tests.rs index c38cc432c..faa66cab9 100644 --- a/crates/jp_config/src/types/command_tests.rs +++ b/crates/jp_config/src/types/command_tests.rs @@ -92,6 +92,110 @@ fn shell_command_line_keeps_program_raw() { assert_eq!(line, "a && b c"); } +/// Expanding a shorthand yields the same command it would have run. +/// +/// This is the invariant that makes expanding-on-sub-key-assignment safe: if +/// the two ever diverged, addressing a field would silently change the command. +#[test] +fn expanding_a_shorthand_matches_the_command_it_describes() { + for shorthand in [ + "cargo check", + "echo 'hello world'", + r#"sh -c "ls -la""#, + "code", + "", + ] { + let expanded = CommandConfigOrString::from_partial( + PartialCommandConfigOrString::Config(expand_shorthand(shorthand)), + vec![], + ) + .expect("the expansion is a valid config"); + + let direct = CommandConfigOrString::String(shorthand.to_owned()); + + assert_eq!( + expanded.command(), + direct.command(), + "expanding {shorthand:?} changed the command" + ); + } +} + +/// A field of the table form is addressable even when a shorthand was written, +/// and the program the shorthand named survives. +#[test] +fn assigning_a_field_expands_the_shorthand() { + let mut p = PartialCommandConfigOrString::String("code --wait".to_owned()); + + let kv = KvAssignment::try_from_cli("shell", "true").unwrap(); + p.assign(kv).unwrap(); + + let cfg = CommandConfigOrString::from_partial(p, vec![]).unwrap(); + assert_eq!(cfg.command(), CommandConfig { + program: "code".to_owned(), + args: vec!["--wait".to_owned()], + shell: true, + }); +} + +/// Assigning `args` replaces the shorthand's arguments while keeping its +/// program. +#[test] +fn assigning_args_keeps_the_shorthand_program() { + let mut p = PartialCommandConfigOrString::String("code --wait".to_owned()); + + let kv = KvAssignment::try_from_cli("args:", r#"["--foo"]"#).unwrap(); + p.assign(kv).unwrap(); + + let cfg = CommandConfigOrString::from_partial(p, vec![]).unwrap(); + assert_eq!(cfg.command(), CommandConfig { + program: "code".to_owned(), + args: vec!["--foo".to_owned()], + shell: false, + }); +} + +/// Assigning a field to a fresh partial works, which is the shape environment +/// variables arrive in: they are assigned onto an empty partial and merged. +#[test] +fn assigning_a_field_to_a_default_partial() { + let mut p = PartialCommandConfigOrString::default(); + + let kv = KvAssignment::try_from_cli("program", "code").unwrap(); + p.assign(kv).unwrap(); + + let cfg = CommandConfigOrString::from_partial(p, vec![]).unwrap(); + assert_eq!(cfg.command(), CommandConfig { + program: "code".to_owned(), + args: vec![], + shell: false, + }); +} + +/// Writing the whole value after a field replaces it, so the order of `--cfg` +/// arguments matters once fields are addressed. +#[test] +fn a_whole_value_assignment_replaces_earlier_fields() { + let mut p = PartialCommandConfigOrString::default(); + + let kv = KvAssignment::try_from_cli("args:", r#"["--wait"]"#).unwrap(); + p.assign(kv).unwrap(); + + let kv = KvAssignment::try_from_cli("", "code").unwrap(); + p.assign(kv).unwrap(); + + let cfg = CommandConfigOrString::from_partial(p, vec![]).unwrap(); + assert_eq!( + cfg.command(), + CommandConfig { + program: "code".to_owned(), + args: vec![], + shell: false, + }, + "the later whole-value write wins outright" + ); +} + #[test] fn test_command_config_structured_passthrough() { let mut p = PartialCommandConfigOrString::default(); diff --git a/crates/jp_llm/src/lib.rs b/crates/jp_llm/src/lib.rs index 7cf1a311f..3cd0ef0fa 100644 --- a/crates/jp_llm/src/lib.rs +++ b/crates/jp_llm/src/lib.rs @@ -16,7 +16,6 @@ pub use error::{Error, StreamError, StreamErrorKind, ToolError}; pub use provider::Provider; pub use retry::exponential_backoff; pub use stream::{ - EventStream, chain::EventChain, output_limit_bytes, with_idle_timeout, with_output_limit, - with_tool_call_keepalive, + EventStream, chain::EventChain, with_idle_timeout, with_output_limit, with_tool_call_keepalive, }; pub use tool::{CommandResult, ExecutionOutcome, ToolTrace, run_tool_command}; diff --git a/crates/jp_llm/src/provider/openai_tests.rs b/crates/jp_llm/src/provider/openai_tests.rs index 9358f9491..824873328 100644 --- a/crates/jp_llm/src/provider/openai_tests.rs +++ b/crates/jp_llm/src/provider/openai_tests.rs @@ -1586,7 +1586,7 @@ mod recorded { use chrono::{TimeZone as _, Utc}; use jp_attachment::Attachment; - use jp_config::assistant::request::CachePolicy; + use jp_config::{AppConfig, assistant::request::CachePolicy}; use jp_conversation::{ConversationStream, event::ChatResponse}; use jp_test::{Result, function_name}; use test_log::test; @@ -1626,12 +1626,18 @@ mod recorded { return request; }; - let mut base = (*thread.events.base_config()).clone(); - base.assistant + // Routed through the partial and re-resolved rather than mutated in + // place: settings that inherit from `assistant` are filled during + // resolution, so an in-place edit would leave them on the old value. + let mut partial = thread.events.base_config().to_partial(); + partial + .assistant .model .parameters .other + .get_or_insert_default() .insert(key.to_owned(), serde_json::Value::from(value).into()); + let base = AppConfig::from_partial_with_defaults(partial).expect("a valid test config"); let placeholder = ConversationStream::new(thread.events.base_config()); let stream = std::mem::replace(&mut thread.events, placeholder); @@ -1661,8 +1667,12 @@ mod recorded { return request; }; - let mut base = (*thread.events.base_config()).clone(); - base.assistant.request.cache = CachePolicy::Off; + // Routed through the partial and re-resolved rather than mutated in + // place: settings that inherit from `assistant` are filled during + // resolution, so an in-place edit would leave them on the old value. + let mut partial = thread.events.base_config().to_partial(); + partial.assistant.request.cache = Some(CachePolicy::Off); + let base = AppConfig::from_partial_with_defaults(partial).expect("a valid test config"); let placeholder = ConversationStream::new(thread.events.base_config()); let stream = std::mem::replace(&mut thread.events, placeholder); diff --git a/crates/jp_llm/src/retry.rs b/crates/jp_llm/src/retry.rs index 099ea78a4..a822805ec 100644 --- a/crates/jp_llm/src/retry.rs +++ b/crates/jp_llm/src/retry.rs @@ -6,12 +6,8 @@ use futures::TryStreamExt as _; use tracing::{debug, warn}; use crate::{ - Provider, StreamError, - error::Result, - event::Event, - model::ModelDetails, - query::ChatQuery, - stream::{output_limit_bytes, with_output_limit}, + Provider, StreamError, error::Result, event::Event, model::ModelDetails, query::ChatQuery, + stream::with_output_limit, }; /// Configuration for resilient stream retries. @@ -28,8 +24,8 @@ pub struct RetryConfig { /// Abort a response after it generates more than this many bytes. /// - /// `0` disables the ceiling. - pub max_response_bytes: u32, + /// `None` leaves the response unbounded. + pub max_response_bytes: Option, } impl Default for RetryConfig { @@ -38,7 +34,7 @@ impl Default for RetryConfig { max_retries: 3, base_backoff_ms: 1000, max_backoff_secs: 30, - max_response_bytes: 1_048_576, + max_response_bytes: Some(1_048_576), } } } @@ -52,7 +48,7 @@ impl RetryConfig { /// call that inherited a large `max_retries` would keep an unattended /// request alive far longer than the caller expects. #[must_use] - pub fn with_max_response_bytes(mut self, max_response_bytes: u32) -> Self { + pub fn with_max_response_bytes(mut self, max_response_bytes: Option) -> Self { self.max_response_bytes = max_response_bytes; self } @@ -82,7 +78,7 @@ pub async fn collect_with_retry( // Bound a runaway response. These collect-style requests run with no // terminal attached (title generation, summarization, tool inquiries), // so nobody is watching to interrupt one that never stops. - let stream = match output_limit_bytes(config.max_response_bytes) { + let stream = match config.max_response_bytes { Some(max) => with_output_limit(stream, max), None => stream, }; diff --git a/crates/jp_llm/src/retry_tests.rs b/crates/jp_llm/src/retry_tests.rs index a9c608ba1..178604534 100644 --- a/crates/jp_llm/src/retry_tests.rs +++ b/crates/jp_llm/src/retry_tests.rs @@ -97,7 +97,7 @@ async fn collect_with_retry_applies_the_output_ceiling_without_retrying() { max_retries: 5, base_backoff_ms: 1, max_backoff_secs: 1, - max_response_bytes: 25, + max_response_bytes: Some(25), }; let error = collect_with_retry(&provider, &model(), empty_query(), &config) diff --git a/crates/jp_llm/src/stream.rs b/crates/jp_llm/src/stream.rs index 92f71a52a..3fb887bf0 100644 --- a/crates/jp_llm/src/stream.rs +++ b/crates/jp_llm/src/stream.rs @@ -148,19 +148,6 @@ pub fn with_output_limit(stream: EventStream, max_bytes: u64) -> EventStream { .boxed() } -/// Translate a configured output ceiling into the argument for -/// [`with_output_limit`], where `0` means the ceiling is disabled. -/// -/// Returns `None` when no ceiling applies, in which case the caller leaves the -/// stream unwrapped. -#[must_use] -pub fn output_limit_bytes(configured: u32) -> Option { - match configured { - 0 => None, - bytes => Some(u64::from(bytes)), - } -} - /// Byte size of the generated content carried by a stream item. /// /// A tool call's `id` and `name` are generated response bytes too, so a stream diff --git a/crates/jp_llm/src/stream_tests.rs b/crates/jp_llm/src/stream_tests.rs index 2f4068523..821ca847e 100644 --- a/crates/jp_llm/src/stream_tests.rs +++ b/crates/jp_llm/src/stream_tests.rs @@ -9,10 +9,7 @@ use std::{ use futures::{StreamExt as _, future, stream}; use serde_json::Map; -use super::{ - output_limit_bytes, with_idle_timeout, with_idle_timeout_at, with_output_limit, - with_tool_call_keepalive, -}; +use super::{with_idle_timeout, with_idle_timeout_at, with_output_limit, with_tool_call_keepalive}; use crate::{ StreamError, StreamErrorKind, event::{Event, EventPart, FinishReason}, @@ -242,18 +239,6 @@ async fn repeated_tool_call_openings_reach_the_ceiling() { ); } -#[test] -fn output_ceiling_of_zero_is_disabled() { - // `0` means "no ceiling" at the config layer. Both call sites route through - // this translation so they cannot drift on what `0` means. - assert!(output_limit_bytes(0).is_none(), "zero disables the ceiling"); - assert_eq!( - output_limit_bytes(512), - Some(512), - "a non-zero ceiling is used as-is" - ); -} - #[tokio::test(start_paused = true)] async fn tool_call_keepalive_emitted_during_open_tool_call() { // A tool-call Start opens the call; the model then goes silent for longer diff --git a/crates/jp_llm/src/test.rs b/crates/jp_llm/src/test.rs index 8b5337bd6..983c39fb7 100644 --- a/crates/jp_llm/src/test.rs +++ b/crates/jp_llm/src/test.rs @@ -4,14 +4,12 @@ use chrono::{TimeZone as _, Utc}; use futures::TryStreamExt as _; use jp_attachment::Attachment; use jp_config::{ - AppConfig, Config as _, PartialAppConfig, ToPartial as _, + AppConfig, PartialAppConfig, ToPartial as _, assistant::tool_choice::ToolChoice, conversation::tool::ToolParameterConfig, model::{ - id::{ModelIdConfig, ModelIdOrAliasConfig, Name, PartialModelIdOrAliasConfig, ProviderId}, - parameters::{ - PartialCustomReasoningConfig, PartialReasoningConfig, ReasoningConfig, ReasoningEffort, - }, + id::{ModelIdConfig, Name, PartialModelIdConfig, PartialModelIdOrAliasConfig, ProviderId}, + parameters::{PartialCustomReasoningConfig, PartialReasoningConfig, ReasoningEffort}, }, providers::llm::LlmProviderConfig, }; @@ -96,12 +94,22 @@ impl TestRequest { query: ChatQuery { thread: ThreadBuilder::new() .with_events({ - let mut config = AppConfig::new_test(); - config.assistant.model.parameters.reasoning = Some(ReasoningConfig::Off); - config.assistant.model.id = ModelIdOrAliasConfig::Id(ModelIdConfig { - provider, - name: "test".parse().unwrap(), - }); + // Set on the partial and resolved once, rather than + // mutating a resolved config: settings that inherit + // from `assistant` (such as `conversation.inquiry`) + // are filled during resolution and would otherwise + // keep the pre-mutation model. + let mut partial = PartialAppConfig::new_test(); + partial.assistant.model.parameters.reasoning = + Some(PartialReasoningConfig::Off); + partial.assistant.model.id = + PartialModelIdOrAliasConfig::Id(PartialModelIdConfig { + provider: Some(provider), + name: Some("test".parse().unwrap()), + }); + let config = AppConfig::from_partial_with_defaults(partial) + .expect("a valid test config"); + ConversationStream::new(config.into()) .with_created_at(Utc.with_ymd_and_hms(2020, 1, 1, 0, 0, 0).unwrap()) }) @@ -178,11 +186,13 @@ impl TestRequest { // Set on the base config directly. The test infra rebuilds the stream // via clear/extend which drops trailing ConfigDeltas (those placed // after the last event in the stream). - let mut base = (*thread.events.base_config()).clone(); - base.assistant.model.parameters.reasoning = reasoning - .map(|r| ReasoningConfig::from_partial(r, vec![])) - .transpose() - .expect("valid reasoning config"); + // + // Routed through the partial and re-resolved rather than mutated in + // place: settings that inherit from `assistant` are filled during + // resolution, so an in-place edit would leave them on the old value. + let mut partial = thread.events.base_config().to_partial(); + partial.assistant.model.parameters.reasoning = reasoning; + let base = AppConfig::from_partial_with_defaults(partial).expect("a valid test config"); let placeholder = ConversationStream::new(thread.events.base_config()); let stream = std::mem::replace(&mut thread.events, placeholder); diff --git a/crates/jp_task/src/task/title_generator.rs b/crates/jp_task/src/task/title_generator.rs index f364f5a21..6d98eb884 100644 --- a/crates/jp_task/src/task/title_generator.rs +++ b/crates/jp_task/src/task/title_generator.rs @@ -37,7 +37,8 @@ pub struct TitleGeneratorTask { pub title: Option, /// Output ceiling for the title request, from /// `assistant.request.max_response_bytes`. - pub max_response_bytes: u32, + /// `None` leaves the response unbounded. + pub max_response_bytes: Option, /// Whether the invoking process is attached to a terminal. /// When `false`, the OSC-2 title-update side effect on task sync is /// suppressed — the bytes would otherwise leak into a captured pipe. @@ -90,7 +91,7 @@ impl TitleGeneratorTask { providers: config.providers.llm.clone(), events, title: None, - max_response_bytes: config.assistant.request.max_response_bytes, + max_response_bytes: config.assistant.request.max_response_bytes.bytes(), is_tty, }) }