diff --git a/crates/jp_cli/src/cmd/compact_flag.rs b/crates/jp_cli/src/cmd/compact_flag.rs index 56503eab1..50de00f52 100644 --- a/crates/jp_cli/src/cmd/compact_flag.rs +++ b/crates/jp_cli/src/cmd/compact_flag.rs @@ -7,9 +7,12 @@ use std::str::FromStr; use clap::{Arg, ArgAction, ArgMatches, Command}; -use jp_config::conversation::compaction::{ - CompactionConfig, CompactionRuleConfig, PartialCompactionRuleConfig, PartialSummaryConfig, - ReasoningMode, RuleBound, ToolCallsMode, +use jp_config::{ + conversation::compaction::{ + CompactionConfig, CompactionRuleConfig, PartialCompactionRuleConfig, PartialSummaryConfig, + ReasoningMode, RuleBound, ToolCallsMode, + }, + types::{byte_size::ByteSize, policy_spec::PolicySpec}, }; /// Shared compaction flag that can be embedded in any command. @@ -106,9 +109,13 @@ impl clap::Args for CompactFlag { `r` / `reasoning`: strip reasoning blocks\n- `s` / `summarize`: generate an \ LLM summary\n- `t` / `tools` (or `t=MODE`): strip tool calls; bare strips \ both, or MODE is one of `strip`/`s`, `strip-requests`/`sreq`, \ - `strip-responses`/`sres`, `omit`/`o`\n\nRange: FROM..TO (1-based, inclusive \ - on both ends, so 1..5 is turns 1-5), single number, or .. for \ - all\n\nExamples: s:..-3, r+t, t=sreq:5..-3, r:-20", + `strip-responses`/`sres`, `omit`/`o`\n\nA policy can carry options after a \ + `,`:\n- `over=SIZE`: compact only items larger than SIZE (`512KB`, `1MB`, or \ + a byte count). Each half of a tool call is judged on its own size; `omit` is \ + judged on the pair combined. Not accepted on `summarize`.\n\nRange: FROM..TO \ + (1-based, inclusive on both ends, so 1..5 is turns 1-5), single number, or \ + .. for all\n\nExamples: s:..-3, r+t, t=sreq:5..-3, r:-20, t=sres,over=1mb, \ + r,over=16kb+t=sres,over=1mb:..-3", ) .action(ArgAction::Append) .num_args(0..=1) @@ -157,10 +164,13 @@ impl clap::FromArgMatches for CompactFlag { /// A parsed compaction DSL spec: `POLICIES[:RANGE]`. #[derive(Debug, Clone, PartialEq, Eq)] pub(crate) struct CompactSpec { - pub reasoning: bool, + /// `None` = no reasoning policy. + /// Carries the policy's own `over` threshold, if any. + pub reasoning: Option>, /// `None` = no tool-call policy. - /// The mode mirrors the `--tools` flag. - pub tools: Option, + /// The mode mirrors the `--tools` flag, plus the policy's own `over` + /// threshold. + pub tools: Option>, pub summarize: bool, /// `None` = use config defaults for range. pub range: Option, @@ -183,12 +193,12 @@ pub(crate) struct DslRange { impl CompactSpec { fn to_partial_rule(&self) -> PartialCompactionRuleConfig { - let mut rule = PartialCompactionRuleConfig::default(); + let mut rule = PartialCompactionRuleConfig { + reasoning: self.reasoning, + tool_calls: self.tools, + ..Default::default() + }; - if self.reasoning { - rule.reasoning = Some(ReasoningMode::Strip); - } - rule.tool_calls = self.tools; if self.summarize { rule.summary = Some(PartialSummaryConfig::default()); } @@ -214,12 +224,17 @@ impl FromStr for CompactSpec { None => (s, None), }; - let mut reasoning = false; - let mut tools: Option = None; + let mut reasoning: Option> = None; + let mut tools: Option> = None; let mut summarize = false; - for policy in policies_str.split('+') { - let policy = policy.trim(); + for term in policies_str.split('+') { + // Options bind to the policy they qualify, so `over` cannot be + // written without one: `r,over=16kb+t=sres,over=1mb`. + let mut parts = term.split(','); + let policy = parts.next().unwrap_or_default().trim(); + let over = parse_policy_options(parts)?; + let (key, value) = match policy.split_once('=') { Some((k, v)) => (k.trim(), Some(v.trim())), None => (policy, None), @@ -230,19 +245,31 @@ impl FromStr for CompactSpec { if value.is_some() { return Err("`reasoning` does not take a value".into()); } - reasoning = true; + reasoning = Some(PolicySpec { + policy: ReasoningMode::Strip, + over, + }); } "s" | "summarize" => { if value.is_some() { return Err("`summarize` does not take a value".into()); } + if over.is_some() { + // A summary replaces its whole range rather than acting + // per item, so there is nothing for a size threshold to + // select. + return Err("`summarize` does not take an `over` threshold".into()); + } summarize = true; } "t" | "tools" => { - tools = Some(match value { - Some(v) => v.parse().map_err(|e| format!("{e}"))?, - // Bare `t` mirrors `--tools` without a value. - None => ToolCallsMode::Strip, + tools = Some(PolicySpec { + policy: match value { + Some(v) => v.parse().map_err(|e| format!("{e}"))?, + // Bare `t` mirrors `--tools` without a value. + None => ToolCallsMode::Strip, + }, + over, }); } "" => return Err("empty policy".into()), @@ -250,7 +277,7 @@ impl FromStr for CompactSpec { } } - if !reasoning && tools.is_none() && !summarize { + if reasoning.is_none() && tools.is_none() && !summarize { return Err("at least one policy required (r, t=MODE, s)".into()); } @@ -265,6 +292,38 @@ impl FromStr for CompactSpec { } } +/// Parse the `key=value` options trailing a DSL policy term. +/// +/// `over` is the only option today, so the result is just its value. +fn parse_policy_options<'a>( + options: impl Iterator, +) -> Result, String> { + let mut over = None; + + for option in options { + let (key, value) = option.split_once('=').ok_or_else(|| { + format!( + "invalid policy option '{}': expected `key=value`", + option.trim() + ) + })?; + + match key.trim() { + "over" => { + over = Some( + value + .trim() + .parse::() + .map_err(|e| format!("{e}"))?, + ); + } + other => return Err(format!("unknown policy option '{other}'")), + } + } + + Ok(over) +} + /// Parse one DSL range bound: a positive integer is a 1-based absolute turn /// index, a negative integer is an offset from the end. fn parse_dsl_bound(s: &str) -> Result { diff --git a/crates/jp_cli/src/cmd/compact_flag_tests.rs b/crates/jp_cli/src/cmd/compact_flag_tests.rs index e51bb3222..92a2d25ce 100644 --- a/crates/jp_cli/src/cmd/compact_flag_tests.rs +++ b/crates/jp_cli/src/cmd/compact_flag_tests.rs @@ -3,14 +3,14 @@ use super::*; #[test] fn parse_policy_only() { assert_eq!("s".parse::().unwrap(), CompactSpec { - reasoning: false, + reasoning: None, tools: None, summarize: true, range: None, }); assert_eq!("r+t=strip".parse::().unwrap(), CompactSpec { - reasoning: true, - tools: Some(ToolCallsMode::Strip), + reasoning: Some(ReasoningMode::Strip.into()), + tools: Some(ToolCallsMode::Strip.into()), summarize: false, range: None, }); @@ -19,8 +19,8 @@ fn parse_policy_only() { .parse::() .unwrap(), CompactSpec { - reasoning: true, - tools: Some(ToolCallsMode::Strip), + reasoning: Some(ReasoningMode::Strip.into()), + tools: Some(ToolCallsMode::Strip.into()), summarize: true, range: None, } @@ -31,23 +31,23 @@ fn parse_policy_only() { fn parse_tool_modes() { let mode = |s: &str| s.parse::().unwrap().tools; // Bare `t` / `tools` defaults to stripping both. - assert_eq!(mode("t"), Some(ToolCallsMode::Strip)); - assert_eq!(mode("tools"), Some(ToolCallsMode::Strip)); - assert_eq!(mode("t=strip"), Some(ToolCallsMode::Strip)); - assert_eq!(mode("t=s"), Some(ToolCallsMode::Strip)); - assert_eq!(mode("t=sreq"), Some(ToolCallsMode::StripRequests)); + assert_eq!(mode("t"), Some(ToolCallsMode::Strip.into())); + assert_eq!(mode("tools"), Some(ToolCallsMode::Strip.into())); + assert_eq!(mode("t=strip"), Some(ToolCallsMode::Strip.into())); + assert_eq!(mode("t=s"), Some(ToolCallsMode::Strip.into())); + assert_eq!(mode("t=sreq"), Some(ToolCallsMode::StripRequests.into())); assert_eq!( mode("tools=strip-responses"), - Some(ToolCallsMode::StripResponses) + Some(ToolCallsMode::StripResponses.into()) ); - assert_eq!(mode("t=o"), Some(ToolCallsMode::Omit)); + assert_eq!(mode("t=o"), Some(ToolCallsMode::Omit.into())); } #[test] fn parse_tool_mode_with_range() { assert_eq!("t=sres:..-3".parse::().unwrap(), CompactSpec { - reasoning: false, - tools: Some(ToolCallsMode::StripResponses), + reasoning: None, + tools: Some(ToolCallsMode::StripResponses.into()), summarize: false, range: Some(DslRange { from: None, @@ -59,7 +59,7 @@ fn parse_tool_mode_with_range() { #[test] fn parse_with_range() { assert_eq!("s:..-3".parse::().unwrap(), CompactSpec { - reasoning: false, + reasoning: None, tools: None, summarize: true, range: Some(DslRange { @@ -70,8 +70,8 @@ fn parse_with_range() { assert_eq!( "r+t=strip:5..-3".parse::().unwrap(), CompactSpec { - reasoning: true, - tools: Some(ToolCallsMode::Strip), + reasoning: Some(ReasoningMode::Strip.into()), + tools: Some(ToolCallsMode::Strip.into()), summarize: false, range: Some(DslRange { from: Some(RuleBound::Absolute(5)), @@ -80,7 +80,7 @@ fn parse_with_range() { } ); assert_eq!("s:..".parse::().unwrap(), CompactSpec { - reasoning: false, + reasoning: None, tools: None, summarize: true, range: Some(DslRange { @@ -89,7 +89,7 @@ fn parse_with_range() { }), }); assert_eq!("r:5..".parse::().unwrap(), CompactSpec { - reasoning: true, + reasoning: Some(ReasoningMode::Strip.into()), tools: None, summarize: false, range: Some(DslRange { @@ -129,7 +129,7 @@ fn parse_absolute_range() { fn parse_single_number_shorthand() { // Negative shorthand `-3` = `..-3` (keep last 3). assert_eq!("s:-3".parse::().unwrap(), CompactSpec { - reasoning: false, + reasoning: None, tools: None, summarize: true, range: Some(DslRange { @@ -139,7 +139,7 @@ fn parse_single_number_shorthand() { }); // Positive shorthand `5` = `5..` (keep first 5). assert_eq!("r:5".parse::().unwrap(), CompactSpec { - reasoning: true, + reasoning: Some(ReasoningMode::Strip.into()), tools: None, summarize: false, range: Some(DslRange { @@ -166,12 +166,111 @@ fn parse_errors() { assert!("s=true".parse::().is_err()); } +// --------------------------------------------------------------------------- +// Policy options (`,over=SIZE`) +// --------------------------------------------------------------------------- + +#[test] +fn parse_over_option_binds_to_its_own_policy() { + // The whole point of binding options to a policy rather than joining them + // with `+`: two policies in one spec can carry different thresholds. + let spec = "r,over=16kb+t=sres,over=1mb" + .parse::() + .unwrap(); + + assert_eq!( + spec.reasoning, + Some(PolicySpec::over( + ReasoningMode::Strip, + ByteSize::from_bytes(16 * 1024) + )) + ); + assert_eq!( + spec.tools, + Some(PolicySpec::over( + ToolCallsMode::StripResponses, + ByteSize::from_bytes(1024 * 1024) + )) + ); +} + +#[test] +fn parse_over_option_composes_with_a_range() { + let spec = "t=sres,over=1mb:..-3".parse::().unwrap(); + + assert_eq!( + spec.tools, + Some(PolicySpec::over( + ToolCallsMode::StripResponses, + ByteSize::from_bytes(1024 * 1024) + )) + ); + assert_eq!( + spec.range, + Some(DslRange { + from: None, + to: Some(RuleBound::FromEnd(3)), + }) + ); +} + +#[test] +fn a_policy_without_an_option_carries_no_threshold() { + let spec = "t=sres".parse::().unwrap(); + + assert_eq!( + spec.tools, + Some(PolicySpec::new(ToolCallsMode::StripResponses)) + ); +} + +#[test] +fn parse_over_option_errors() { + // A summary replaces its whole range rather than acting per item, so a + // threshold on it would be a silent no-op. + assert_eq!( + "s,over=1mb".parse::().unwrap_err(), + "`summarize` does not take an `over` threshold" + ); + assert_eq!( + "t,under=1mb".parse::().unwrap_err(), + "unknown policy option 'under'" + ); + assert_eq!( + "t,over".parse::().unwrap_err(), + "invalid policy option 'over': expected `key=value`" + ); + // An option cannot stand on its own: with nothing before the comma it is + // read as a policy name, and there is no such policy. + assert_eq!( + "over=1mb".parse::().unwrap_err(), + "unknown policy 'over'" + ); + assert!("t,over=nonsense".parse::().is_err()); +} + +#[test] +fn over_option_reaches_the_partial_rule() { + let rule = "t=sres,over=1mb" + .parse::() + .unwrap() + .to_partial_rule(); + + assert_eq!( + rule.tool_calls, + Some(PolicySpec::over( + ToolCallsMode::StripResponses, + ByteSize::from_bytes(1024 * 1024) + )) + ); +} + #[test] fn to_partial_rule_with_range() { let spec = "r+t=strip:..-3".parse::().unwrap(); let rule = spec.to_partial_rule(); - assert_eq!(rule.reasoning, Some(ReasoningMode::Strip)); - assert_eq!(rule.tool_calls, Some(ToolCallsMode::Strip)); + assert_eq!(rule.reasoning, Some(ReasoningMode::Strip.into())); + assert_eq!(rule.tool_calls, Some(ToolCallsMode::Strip.into())); assert!(rule.summary.is_none()); // Open start maps to keep-first 0 (compact from the first turn); `-3` keeps // the last 3. @@ -210,7 +309,10 @@ fn specs_only_replace_config_rules() { let rules = flag.effective_rules(&config_rules()).unwrap(); assert_eq!(rules.len(), 1); - assert_eq!(rules[0].tool_calls, Some(ToolCallsMode::StripRequests)); + assert_eq!( + rules[0].tool_calls, + Some(ToolCallsMode::StripRequests.into()) + ); } #[test] @@ -225,8 +327,8 @@ fn bare_compact_plus_dsl_appends_to_config_rules() { assert_eq!(rules.len(), 2); // Config default first (strip reasoning + tools), then the summary spec. - assert_eq!(rules[0].reasoning, Some(ReasoningMode::Strip)); - assert_eq!(rules[0].tool_calls, Some(ToolCallsMode::Strip)); + assert_eq!(rules[0].reasoning, Some(ReasoningMode::Strip.into())); + assert_eq!(rules[0].tool_calls, Some(ToolCallsMode::Strip.into())); assert!(rules[1].summary.is_some()); } diff --git a/crates/jp_cli/src/cmd/conversation/compact.rs b/crates/jp_cli/src/cmd/conversation/compact.rs index d8793b9fc..a5ae43bc6 100644 --- a/crates/jp_cli/src/cmd/conversation/compact.rs +++ b/crates/jp_cli/src/cmd/conversation/compact.rs @@ -9,9 +9,10 @@ use jp_config::{ }, }; use jp_conversation::{ - Compaction, CompactionRange, ConversationStream, RangeBound, ReasoningPolicy, SummaryPolicy, - ToolCallPolicy, + ByteSize, Compaction, CompactionRange, ConversationStream, PolicySpec, RangeBound, + ReasoningPolicy, SummaryPolicy, ToolCallPolicy, compaction::{extend_summary_range, resolve_range}, + stream::AffectedItem, }; use jp_workspace::{ConversationHandle, ConversationMut, Workspace}; use tracing::warn; @@ -92,6 +93,26 @@ pub(crate) struct Compact { #[arg(short, long, conflicts_with = "compact")] summarize: Option>, + /// Only compact items larger than this. + /// + /// Accepts a human-readable size (`512KB`, `1MB`) or a bare byte count. + /// Applies to every mechanical policy this invocation sets; use the inline + /// DSL (`-k 'r,over=16kb+t=sres,over=1mb'`) to give each policy its own + /// threshold. + /// + /// Each half of a tool call is judged on its own size, so `--tools=strip` + /// on a call with a short request and a huge response drops only the + /// response. + /// `--tools=omit` removes whole pairs, so it is judged on the two halves + /// combined. + #[arg( + long, + value_name = "SIZE", + value_parser = parse_byte_size, + conflicts_with_all = ["summarize", "compact"], + )] + over: Option, + /// The model to summarize with. /// /// Accepts a model alias or a full `provider/name` ID, the same values as @@ -164,6 +185,13 @@ impl Compact { "--keep-last {keep} is greater than --last {last}: nothing would remain to compact" )); } + // A threshold narrows a policy, so it needs one to narrow. Without this + // the flag would be a silent no-op. + if self.over.is_some() && !self.reasoning && self.tools.is_none() { + return Err( + "--over needs a policy to narrow: pass --reasoning and/or --tools".to_owned(), + ); + } Ok(()) } @@ -204,9 +232,15 @@ impl Compact { if self.has_policy_overrides() { let mut rule = PartialCompactionRuleConfig::default(); if self.reasoning { - rule.reasoning = Some(ReasoningMode::Strip); + rule.reasoning = Some(PolicySpec { + policy: ReasoningMode::Strip, + over: self.over, + }); } - rule.tool_calls = self.tools; + rule.tool_calls = self.tools.map(|policy| PolicySpec { + policy, + over: self.over, + }); if let Some(context) = &self.summarize { rule.summary = Some(PartialSummaryConfig { context: context.clone(), @@ -265,6 +299,10 @@ impl IntoPartialAppConfig for Compact { } } +fn parse_byte_size(s: &str) -> Result { + s.parse::().map_err(|e| e.to_string()) +} + fn parse_tool_calls_mode(s: &str) -> Result { s.parse().map_err(|_| { "expected one of: strip (s), strip-requests (sreq), strip-responses (sres), omit (o)" @@ -471,6 +509,41 @@ struct TimelineSegment { /// Existing compactions are reported factually ("Compacted") even under /// `--dry-run`, since they pre-date the previewed run. existing: bool, + /// The individual items a size threshold selected. + /// + /// `None` when no policy carries a threshold, where the range and the label + /// already describe the effect exactly. + /// `Some(vec![])` means a threshold ran and matched nothing, which reads + /// very differently from the range line alone. + items: Option>, +} + +/// The items a compaction selects, when a size threshold makes the selection +/// worth spelling out. +/// +/// Returns `None` for a rule with no threshold: it reaches everything in its +/// range by definition, so listing each item would be noise. +/// Also `None` for a summary, which replaces its whole range regardless of any +/// mechanical policy it happens to carry, so naming individual items would +/// describe a selection that never happened. +fn threshold_items( + events: &ConversationStream, + compaction: &Compaction, +) -> Option> { + if compaction.summary.is_some() { + return None; + } + + let narrowed = compaction + .reasoning + .as_ref() + .is_some_and(|spec| spec.over.is_some()) + || compaction + .tool_calls + .as_ref() + .is_some_and(|spec| spec.over.is_some()); + + narrowed.then(|| events.affected_items(compaction)) } /// Build timeline segments for the compactions about to be applied, spilling @@ -478,7 +551,11 @@ struct TimelineSegment { /// /// `conv_id` prefixes the temp-file names so summaries from different /// conversations don't collide. -fn segments_for_compactions(compactions: &[Compaction], conv_id: &str) -> Vec { +fn segments_for_compactions( + compactions: &[Compaction], + events: &ConversationStream, + conv_id: &str, +) -> Vec { compactions .iter() .map(|c| { @@ -496,6 +573,7 @@ fn segments_for_compactions(compactions: &[Compaction], conv_id: &str) -> Vec Vec { to: c.to_turn, label: Some("already compacted".to_owned()), existing: true, + items: None, }) .collect() } @@ -582,6 +661,23 @@ fn timeline_lines(segments: &[TimelineSegment], last_turn: usize, dry_run: bool) ), }); + // A size threshold reaches an unpredictable subset of its range, so the + // range line alone doesn't say what happened. Name what it caught, and + // say so explicitly when it caught nothing. + if let Some(items) = &segment.items { + if items.is_empty() { + lines.push(" nothing over the threshold.".to_owned()); + } + for item in items { + lines.push(format!( + " turn {} {} {}", + item.turn + 1, + item.name, + item.size.human() + )); + } + } + covered = Some(covered.map_or(segment.to, |c| c.max(segment.to))); } @@ -657,25 +753,33 @@ fn build_mechanical_compaction( ) -> Compaction { let mut compaction = Compaction::new(from_turn, to_turn); - if rule.reasoning.is_some() { - compaction = compaction.with_reasoning(ReasoningPolicy::Strip); + // Each rule's size threshold carries across to the stored policy, so the + // projection applies the same narrowing the user configured. + if let Some(spec) = rule.reasoning { + compaction = compaction.with_reasoning(PolicySpec { + policy: ReasoningPolicy::Strip, + over: spec.over, + }); } - if let Some(mode) = rule.tool_calls { - compaction = compaction.with_tool_calls(match mode { - ToolCallsMode::Strip => ToolCallPolicy::Strip { - request: true, - response: true, + if let Some(spec) = rule.tool_calls { + compaction = compaction.with_tool_calls(PolicySpec { + policy: match spec.policy { + ToolCallsMode::Strip => ToolCallPolicy::Strip { + request: true, + response: true, + }, + ToolCallsMode::StripResponses => ToolCallPolicy::Strip { + request: false, + response: true, + }, + ToolCallsMode::StripRequests => ToolCallPolicy::Strip { + request: true, + response: false, + }, + ToolCallsMode::Omit => ToolCallPolicy::Omit, }, - ToolCallsMode::StripResponses => ToolCallPolicy::Strip { - request: false, - response: true, - }, - ToolCallsMode::StripRequests => ToolCallPolicy::Strip { - request: true, - response: false, - }, - ToolCallsMode::Omit => ToolCallPolicy::Omit, + over: spec.over, }); } @@ -762,6 +866,7 @@ impl Compact { let mut segments = existing_segments(&events_snapshot); segments.extend(segments_for_compactions( &compactions, + &events_snapshot, &conv.id().to_string(), )); apply_compactions(&conv, compactions); @@ -844,20 +949,18 @@ impl Compact { ) else { continue; }; + let preview = build_mechanical_compaction(range.from_turn, range.to_turn, rule); let label = if rule.summary.is_some() { Some("summary".to_owned()) } else { - compaction_policy_label(&build_mechanical_compaction( - range.from_turn, - range.to_turn, - rule, - )) + compaction_policy_label(&preview) }; new_segments.push(TimelineSegment { from: range.from_turn, to: range.to_turn, label, existing: false, + items: threshold_items(events_snapshot, &preview), }); if rule.summary.is_some() { overlap.add_compaction( diff --git a/crates/jp_cli/src/cmd/conversation/compact_tests.rs b/crates/jp_cli/src/cmd/conversation/compact_tests.rs index 89a25877a..97d73e133 100644 --- a/crates/jp_cli/src/cmd/conversation/compact_tests.rs +++ b/crates/jp_cli/src/cmd/conversation/compact_tests.rs @@ -10,7 +10,8 @@ use jp_config::{ model::{PartialModelConfig, id::PartialModelIdOrAliasConfig}, }; use jp_conversation::{ - Compaction, ConversationStream, RangeBound, ReasoningPolicy, ToolCallPolicy, + ByteSize, Compaction, ConversationStream, PolicySpec, RangeBound, ReasoningPolicy, + ToolCallPolicy, event::{ToolCallRequest, ToolCallResponse}, }; use jp_printer::Printer; @@ -43,6 +44,82 @@ fn bare_compact_flag_parses_without_a_value() { assert!(compact.compact_flag.specs.is_empty()); } +#[test] +fn over_flag_reaches_the_stored_policy() { + // The threshold has to survive the whole path (flag -> ad-hoc rule -> + // stored `Compaction`), because projection reads it from the event, not + // from the invocation. + let compact = parse_compact(&["--tools=sres", "--over", "1mb"]); + let rules = compact.effective_rules(&AppConfig::new_test()).unwrap(); + + assert_eq!(rules.len(), 1); + assert_eq!( + rules[0].tool_calls, + Some(PolicySpec::over( + ToolCallsMode::StripResponses, + ByteSize::from_bytes(1024 * 1024) + )) + ); + + let compaction = super::build_mechanical_compaction(0, 0, &rules[0]); + + assert_eq!( + compaction.tool_calls, + Some(PolicySpec::over( + ToolCallPolicy::Strip { + request: false, + response: true, + }, + ByteSize::from_bytes(1024 * 1024) + )) + ); +} + +#[test] +fn over_flag_applies_to_every_mechanical_policy_it_sets() { + let compact = parse_compact(&["--reasoning", "--tools=strip", "--over", "512kb"]); + let rules = compact.effective_rules(&AppConfig::new_test()).unwrap(); + + let over = Some(ByteSize::from_bytes(512 * 1024)); + assert_eq!(rules[0].reasoning.map(|spec| spec.over), Some(over)); + assert_eq!(rules[0].tool_calls.map(|spec| spec.over), Some(over)); +} + +#[test] +fn over_without_a_policy_is_rejected() { + // Without a policy to narrow, the flag would silently do nothing. + let compact = parse_compact(&["--over", "1mb"]); + + assert_eq!( + compact.validate().unwrap_err(), + "--over needs a policy to narrow: pass --reasoning and/or --tools" + ); +} + +#[test] +fn over_conflicts_with_summarize() { + // A summary replaces its whole range rather than acting per item, so it + // ignores the mechanical policies a threshold would narrow. + #[derive(clap::Parser)] + struct TestCli { + #[command(flatten)] + compact: Compact, + } + + assert!(TestCli::try_parse_from(["compact", "--summarize", "--over", "1mb"]).is_err()); +} + +#[test] +fn no_over_flag_leaves_the_policy_unnarrowed() { + let compact = parse_compact(&["--tools=sres"]); + let rules = compact.effective_rules(&AppConfig::new_test()).unwrap(); + + assert_eq!( + rules[0].tool_calls, + Some(PolicySpec::new(ToolCallsMode::StripResponses)) + ); +} + #[test] fn keep_last_only_does_not_inject_a_policyless_rule() { // Range-only flags carry no policy, so `effective_rules` must fall through @@ -320,7 +397,7 @@ fn tool_calls_mode_maps_to_policy() { keep_first: RuleBound::Turns(0), keep_last: RuleBound::Turns(0), reasoning: None, - tool_calls: Some(mode), + tool_calls: Some(mode.into()), summary: None, }; let compactions = rt @@ -334,7 +411,11 @@ fn tool_calls_mode_maps_to_policy() { )) .unwrap(); assert_eq!(compactions.len(), 1, "non-empty range, mode {mode:?}"); - assert_eq!(compactions[0].tool_calls, Some(expected), "mode {mode:?}"); + assert_eq!( + compactions[0].tool_calls, + Some(expected.into()), + "mode {mode:?}" + ); } } @@ -352,7 +433,7 @@ fn keep_last_duration_covering_whole_conversation_compacts_nothing() { keep_first: RuleBound::Turns(0), keep_last: RuleBound::Duration(Duration::from_hours(720)), reasoning: None, - tool_calls: Some(ToolCallsMode::Strip), + tool_calls: Some(ToolCallsMode::Strip.into()), summary: None, }; let compactions = runtime() @@ -386,7 +467,7 @@ fn from_last_resolves_against_original_stream_for_every_rule() { CompactionRuleConfig { keep_first: RuleBound::Turns(0), keep_last: RuleBound::Turns(3), - reasoning: Some(ReasoningMode::Strip), + reasoning: Some(ReasoningMode::Strip.into()), tool_calls: None, summary: None, }, @@ -394,7 +475,7 @@ fn from_last_resolves_against_original_stream_for_every_rule() { keep_first: RuleBound::Turns(0), keep_last: RuleBound::Turns(3), reasoning: None, - tool_calls: Some(ToolCallsMode::Strip), + tool_calls: Some(ToolCallsMode::Strip.into()), summary: None, }, ]; @@ -454,7 +535,7 @@ fn config_rule_strip_requests_blanks_args_through_projection() { keep_first: RuleBound::Turns(1), keep_last: RuleBound::Turns(1), reasoning: None, - tool_calls: Some(ToolCallsMode::StripRequests), + tool_calls: Some(ToolCallsMode::StripRequests.into()), summary: None, }]; @@ -475,10 +556,13 @@ fn config_rule_strip_requests_blanks_args_through_projection() { assert_eq!((compactions[0].from_turn, compactions[0].to_turn), (1, 4)); assert_eq!( compactions[0].tool_calls, - Some(ToolCallPolicy::Strip { - request: true, - response: false, - }) + Some( + ToolCallPolicy::Strip { + request: true, + response: false, + } + .into() + ) ); for compaction in compactions { @@ -649,6 +733,7 @@ fn timeline_keeps_genesis_and_trailing_turns() { to: 7, label: None, existing: false, + items: None, }]; let lines = timeline_lines(&segments, 8, false); assert_eq!(lines, vec![ @@ -667,12 +752,14 @@ fn timeline_interleaves_gaps_between_compactions() { to: 3, label: None, existing: false, + items: None, }, TimelineSegment { from: 6, to: 8, label: None, existing: false, + items: None, }, ]; let lines = timeline_lines(&segments, 10, false); @@ -695,12 +782,14 @@ fn timeline_sorts_by_start_turn_regardless_of_generation_order() { to: 8, label: None, existing: false, + items: None, }, TimelineSegment { from: 1, to: 3, label: None, existing: false, + items: None, }, ]; let lines = timeline_lines(&segments, 8, false); @@ -722,12 +811,14 @@ fn timeline_collapses_overlapping_ranges() { to: 5, label: None, existing: false, + items: None, }, TimelineSegment { from: 3, to: 8, label: None, existing: false, + items: None, }, ]; let lines = timeline_lines(&segments, 10, false); @@ -746,6 +837,7 @@ fn timeline_labels_describe_compaction_type() { to: 3, label: Some("reasoning + tools".to_owned()), existing: false, + items: None, }]; let lines = timeline_lines(&segments, 4, false); assert_eq!(lines, vec![ @@ -762,6 +854,7 @@ fn timeline_dry_run_uses_conditional_verbs() { to: 3, label: None, existing: false, + items: None, }]; let lines = timeline_lines(&segments, 4, true); assert_eq!(lines, vec![ @@ -781,11 +874,134 @@ fn segment_label_reflects_mechanical_policies() { request: true, response: true, }); - let segments = segments_for_compactions(std::slice::from_ref(&compaction), "test-conv"); + let segments = segments_for_compactions( + std::slice::from_ref(&compaction), + &ConversationStream::new_test(), + "test-conv", + ); assert_eq!(segments.len(), 1); assert_eq!(segments[0].label.as_deref(), Some("reasoning + tools")); } +/// A one-turn stream with two tool calls: a 4 KB response and a 2-byte one. +fn stream_with_a_large_and_a_small_call() -> ConversationStream { + let mut stream = ConversationStream::new_test(); + stream.start_turn("read them"); + stream + .current_turn_mut() + .add_tool_call_request(ToolCallRequest { + id: "big".into(), + name: "fs_read_file".into(), + arguments: Map::from_iter([("path".into(), Value::from("huge.log"))]), + }) + .add_tool_call_response(ToolCallResponse { + id: "big".into(), + result: Ok("x".repeat(4096)), + }) + .add_tool_call_request(ToolCallRequest { + id: "small".into(), + name: "fs_read_file".into(), + arguments: Map::from_iter([("path".into(), Value::from("tiny.log"))]), + }) + .add_tool_call_response(ToolCallResponse { + id: "small".into(), + result: Ok("ok".into()), + }) + .build() + .unwrap(); + stream +} + +#[test] +fn timeline_lists_what_a_threshold_caught() { + // A threshold reaches an unpredictable subset of its range, so the range + // line alone would leave the user guessing whether it hit 1 call or 12. + let stream = stream_with_a_large_and_a_small_call(); + let compaction = Compaction::new(0, 0).with_tool_calls(PolicySpec::over( + ToolCallPolicy::Strip { + request: false, + response: true, + }, + ByteSize::from_bytes(1024), + )); + + let segments = segments_for_compactions(std::slice::from_ref(&compaction), &stream, "conv"); + let lines = timeline_lines(&segments, 0, true); + + assert_eq!(lines, vec![ + "Would have compacted turns 1..1 (1 total, tool responses over 1KB).".to_owned(), + " turn 1 fs_read_file (response) 4.0 KB".to_owned(), + ]); +} + +#[test] +fn timeline_says_so_when_a_threshold_caught_nothing() { + // Silence here would read as "the whole range was compacted". + let stream = stream_with_a_large_and_a_small_call(); + let compaction = Compaction::new(0, 0).with_tool_calls(PolicySpec::over( + ToolCallPolicy::Strip { + request: false, + response: true, + }, + ByteSize::from_bytes(1024 * 1024), + )); + + let segments = segments_for_compactions(std::slice::from_ref(&compaction), &stream, "conv"); + let lines = timeline_lines(&segments, 0, true); + + assert_eq!(lines, vec![ + "Would have compacted turns 1..1 (1 total, tool responses over 1MB).".to_owned(), + " nothing over the threshold.".to_owned(), + ]); +} + +#[test] +fn timeline_does_not_itemize_a_summary_rule() { + // `-k 's+r,over=1kb'` parses: the threshold binds to `r`, and the summary + // rule carries it. A summary replaces its whole range, so projection never + // consults the threshold and itemizing it would name items nothing selected. + let stream = stream_with_a_large_and_a_small_call(); + let compaction = Compaction::new(0, 0) + .with_tool_calls(PolicySpec::over( + ToolCallPolicy::Strip { + request: false, + response: true, + }, + ByteSize::from_bytes(1024), + )) + .with_summary(jp_conversation::SummaryPolicy { + summary: "the gist".to_owned(), + }); + + let segments = segments_for_compactions(std::slice::from_ref(&compaction), &stream, "conv"); + let lines = timeline_lines(&segments, 0, true); + + assert_eq!(lines.len(), 1, "no item lines: {lines:?}"); + assert!( + lines[0].contains("summary"), + "summary wins the label: {}", + lines[0] + ); +} + +#[test] +fn timeline_does_not_itemize_a_rule_without_a_threshold() { + // An unnarrowed rule reaches everything in range by definition, so listing + // each call would be noise. + let stream = stream_with_a_large_and_a_small_call(); + let compaction = Compaction::new(0, 0).with_tool_calls(ToolCallPolicy::Strip { + request: false, + response: true, + }); + + let segments = segments_for_compactions(std::slice::from_ref(&compaction), &stream, "conv"); + let lines = timeline_lines(&segments, 0, true); + + assert_eq!(lines, vec![ + "Would have compacted turns 1..1 (1 total, tool responses).".to_owned(), + ]); +} + #[test] fn timeline_reports_pre_existing_compactions_not_as_kept() { // Regression: a prior run compacted turns 1..5; a new run (e.g. `--from @@ -803,6 +1019,7 @@ fn timeline_reports_pre_existing_compactions_not_as_kept() { to: 8, label: None, existing: false, + items: None, }); let lines = timeline_lines(&segments, 9, false); @@ -831,6 +1048,7 @@ fn timeline_dry_run_keeps_pre_existing_compactions_factual() { to: 8, label: None, existing: false, + items: None, }); let lines = timeline_lines(&segments, 9, true); diff --git a/crates/jp_cli/src/cmd/query_tests.rs b/crates/jp_cli/src/cmd/query_tests.rs index fee37da27..66a8fc533 100644 --- a/crates/jp_cli/src/cmd/query_tests.rs +++ b/crates/jp_cli/src/cmd/query_tests.rs @@ -755,7 +755,7 @@ fn query_cfg_sourced_compaction_persists_as_config_delta() { // reasoning + tools default). let mut partial = base_config.to_partial(); partial.conversation.compaction.rules = MergeableVec::Vec(vec![PartialCompactionRuleConfig { - reasoning: Some(ReasoningMode::Strip), + reasoning: Some(ReasoningMode::Strip.into()), ..Default::default() }]); let runtime_config = build(partial).unwrap(); diff --git a/crates/jp_cli/src/format.rs b/crates/jp_cli/src/format.rs index 4e3f1820b..d1cb3bffd 100644 --- a/crates/jp_cli/src/format.rs +++ b/crates/jp_cli/src/format.rs @@ -2,7 +2,7 @@ pub(crate) mod conversation; pub(crate) mod datetime; use jp_config::types::color::Color; -use jp_conversation::{Compaction, ToolCallPolicy}; +use jp_conversation::{ByteSize, Compaction, ToolCallPolicy}; use jp_term::table::DetailItem; use serde_json::json; use url::Url; @@ -68,10 +68,10 @@ pub(crate) fn label_detail_item(key: &str, value: &str) -> DetailItem { /// inclusive), `reasoning`, `tool_calls`, and `summary` (the full generated /// text, or `null`). /// -/// `tool_calls` mirrors [`ToolCallPolicy`]'s own serialized shape (e.g. +/// `reasoning` and `tool_calls` mirror their own serialized shapes (e.g. /// `{"policy": "strip", "request": true, "response": true}`) rather than the /// `--tools` flag vocabulary, since a policy can carry `request`/`response` -/// combinations the flag can't express. +/// combinations the flag can't express, plus an `over` size threshold. pub(crate) fn compaction_detail_item(compaction: &Compaction) -> DetailItem { let from = compaction.from_turn + 1; let to = compaction.to_turn + 1; @@ -93,7 +93,7 @@ pub(crate) fn compaction_detail_item(compaction: &Compaction) -> DetailItem { json!({ "from_turn": from, "to_turn": to, - "reasoning": compaction.reasoning.is_some(), + "reasoning": compaction.reasoning.as_ref(), "tool_calls": compaction.tool_calls.as_ref(), "summary": compaction.summary.as_ref().map(|s| &s.summary), }), @@ -103,33 +103,45 @@ pub(crate) fn compaction_detail_item(compaction: &Compaction) -> DetailItem { /// Describe a compaction's mechanical policies (reasoning / tool calls), e.g. /// `reasoning + tools`. /// +/// A policy narrowed by a size threshold reads as `tool responses over 1MB`, so +/// the label distinguishes a rule that compacted everything in its range from +/// one that only reached the large items. +/// /// Summaries take precedence over mechanical policies and are labeled /// separately by the caller. /// Returns `None` when the compaction carries no mechanical policy. pub(crate) fn compaction_policy_label(compaction: &Compaction) -> Option { + /// Append a policy's threshold, when it has one. + fn qualified(name: &str, over: Option) -> String { + over.map_or_else(|| name.to_owned(), |size| format!("{name} over {size}")) + } + let mut parts = Vec::new(); - if compaction.reasoning.is_some() { - parts.push("reasoning"); + if let Some(spec) = &compaction.reasoning { + parts.push(qualified("reasoning", spec.over)); } - if let Some(policy) = &compaction.tool_calls { - match policy { + if let Some(spec) = &compaction.tool_calls { + let name = match spec.policy { ToolCallPolicy::Strip { request: true, response: true, - } => parts.push("tools"), + } => Some("tools"), ToolCallPolicy::Strip { request: true, response: false, - } => parts.push("tool requests"), + } => Some("tool requests"), ToolCallPolicy::Strip { request: false, response: true, - } => parts.push("tool responses"), + } => Some("tool responses"), ToolCallPolicy::Strip { request: false, response: false, - } => {} - ToolCallPolicy::Omit => parts.push("tools omitted"), + } => None, + ToolCallPolicy::Omit => Some("tools omitted"), + }; + if let Some(name) = name { + parts.push(qualified(name, spec.over)); } } diff --git a/crates/jp_cli/src/format_tests.rs b/crates/jp_cli/src/format_tests.rs index b5a376440..f456c0033 100644 --- a/crates/jp_cli/src/format_tests.rs +++ b/crates/jp_cli/src/format_tests.rs @@ -1,4 +1,6 @@ -use jp_conversation::{Compaction, ReasoningPolicy, SummaryPolicy, ToolCallPolicy}; +use jp_conversation::{ + ByteSize, Compaction, PolicySpec, ReasoningPolicy, SummaryPolicy, ToolCallPolicy, +}; use super::*; @@ -53,7 +55,7 @@ fn compaction_detail_item_reports_reasoning_and_tools_policy() { // 0-based turn 2 is displayed as turn 3; a single-turn range still reads // as an inclusive range for consistency with multi-turn ranges. assert_eq!(item.text, "turns 3..3 (1 total, reasoning + tools)"); - assert!(item.json["reasoning"].as_bool().unwrap()); + assert_eq!(item.json["reasoning"], "strip"); assert_eq!(item.json["tool_calls"]["policy"], "strip"); assert!(item.json["summary"].is_null()); } @@ -99,3 +101,44 @@ fn compaction_policy_label_is_none_without_any_policy() { let compaction = Compaction::new(0, 0); assert_eq!(compaction_policy_label(&compaction), None); } + +#[test] +fn compaction_policy_label_names_a_size_threshold() { + // A rule that only reached the large items must not read the same as one + // that compacted everything in its range. + let compaction = Compaction::new(0, 0) + .with_reasoning(PolicySpec::over( + ReasoningPolicy::Strip, + ByteSize::from_bytes(16 * 1024), + )) + .with_tool_calls(PolicySpec::over( + ToolCallPolicy::Strip { + request: false, + response: true, + }, + ByteSize::from_bytes(1024 * 1024), + )); + + assert_eq!( + compaction_policy_label(&compaction), + Some("reasoning over 16KB + tool responses over 1MB".to_owned()) + ); +} + +#[test] +fn compaction_detail_item_reports_a_size_threshold() { + let compaction = Compaction::new(0, 0).with_tool_calls(PolicySpec::over( + ToolCallPolicy::Strip { + request: false, + response: true, + }, + ByteSize::from_bytes(1024 * 1024), + )); + + let item = compaction_detail_item(&compaction); + + assert_eq!(item.text, "turns 1..1 (1 total, tool responses over 1MB)"); + assert_eq!(item.json["tool_calls"]["policy"], "strip"); + assert_eq!(item.json["tool_calls"]["response"], true); + assert_eq!(item.json["tool_calls"]["over"], "1MB"); +} diff --git a/crates/jp_config/src/conversation/compaction.rs b/crates/jp_config/src/conversation/compaction.rs index 7635cb623..04df6d65c 100644 --- a/crates/jp_config/src/conversation/compaction.rs +++ b/crates/jp_config/src/conversation/compaction.rs @@ -13,7 +13,10 @@ use crate::{ internal::merge::vec_with_strategy, model::{ModelConfig, PartialModelConfig}, partial::{ToPartial, partial_opt_config, partial_opts}, - types::vec::{MergeableVec, MergedVec, vec_to_mergeable_partial}, + types::{ + policy_spec::PolicySpec, + vec::{MergeableVec, MergedVec, vec_to_mergeable_partial}, + }, }; /// Compaction configuration. @@ -62,8 +65,8 @@ impl PartialCompactionConfig { #[must_use] pub fn builtin_rules() -> Vec { vec![PartialCompactionRuleConfig { - reasoning: Some(ReasoningMode::Strip), - tool_calls: Some(ToolCallsMode::Strip), + reasoning: Some(ReasoningMode::Strip.into()), + tool_calls: Some(ToolCallsMode::Strip.into()), ..Default::default() }] } @@ -190,11 +193,45 @@ pub struct CompactionRuleConfig { #[setting(default = default_keep_last)] pub keep_last: RuleBound, - /// Policy for reasoning (thinking) blocks. - pub reasoning: Option, + /// What to do with reasoning (thinking) blocks in the compacted range. + /// + /// The only mode is `"strip"`, which drops the blocks from the view sent to + /// the model. + /// If unset, reasoning blocks in the range are left alone. + /// + /// A table form adds a size threshold: + /// + /// ```toml + /// reasoning = { policy = "strip", over = "16KB" } + /// ``` + /// + /// With `over` set, only blocks larger than that are stripped. + /// Sizes accept `"512KB"`, `"1MB"`, or a bare byte count, and the + /// comparison is strict: a block of exactly the threshold is left alone. + pub reasoning: Option>, - /// Policy for tool call arguments and responses. - pub tool_calls: Option, + /// What to do with tool call arguments and responses in the compacted + /// range. + /// + /// - `"strip"`: replace request arguments *and* response content. + /// - `"strip-requests"`: replace request arguments, keep responses. + /// - `"strip-responses"`: replace response content, keep arguments. + /// - `"omit"`: remove the request and response entirely. + /// + /// If unset, tool calls in the range are left alone. + /// + /// A table form adds a size threshold: + /// + /// ```toml + /// tool_calls = { policy = "strip-responses", over = "1MB" } + /// ``` + /// + /// With `over` set, only the large parts of a call are compacted. + /// Each half is judged on its own size, so `"strip"` on a call with a short + /// request and a huge response drops only the response. + /// `"omit"` removes whole pairs, so it is judged on the request and + /// response combined. + pub tool_calls: Option>, /// Summarization configuration. /// diff --git a/crates/jp_config/src/conversation/compaction_tests.rs b/crates/jp_config/src/conversation/compaction_tests.rs index 005e7ba15..4d07c193a 100644 --- a/crates/jp_config/src/conversation/compaction_tests.rs +++ b/crates/jp_config/src/conversation/compaction_tests.rs @@ -1,4 +1,138 @@ use super::*; +use crate::types::byte_size::ByteSize; + +// --------------------------------------------------------------------------- +// Rule policies in TOML +// --------------------------------------------------------------------------- + +/// Deserialize a single `[[conversation.compaction.rules]]` body from TOML. +fn rule_from_toml(body: &str) -> PartialCompactionRuleConfig { + toml::from_str(body).unwrap() +} + +#[test] +fn bare_string_policies_still_parse() { + let rule = rule_from_toml( + r#" + reasoning = "strip" + tool_calls = "strip-responses" + "#, + ); + + assert_eq!(rule.reasoning, Some(ReasoningMode::Strip.into())); + assert_eq!(rule.tool_calls, Some(ToolCallsMode::StripResponses.into())); +} + +#[test] +fn table_policies_carry_a_size_threshold() { + let rule = rule_from_toml( + r#" + reasoning = { policy = "strip", over = "16KB" } + tool_calls = { policy = "strip-responses", over = "1MB" } + "#, + ); + + assert_eq!( + rule.reasoning, + Some(PolicySpec::over( + ReasoningMode::Strip, + ByteSize::from_bytes(16 * 1024) + )) + ); + assert_eq!( + rule.tool_calls, + Some(PolicySpec::over( + ToolCallsMode::StripResponses, + ByteSize::from_bytes(1024 * 1024) + )) + ); +} + +#[test] +fn a_policy_table_without_over_matches_the_bare_string() { + let table = rule_from_toml(r#"tool_calls = { policy = "omit" }"#); + let bare = rule_from_toml(r#"tool_calls = "omit""#); + + assert_eq!(table.tool_calls, bare.tool_calls); +} + +#[test] +fn a_policy_without_a_threshold_serializes_back_as_a_bare_string() { + // Keeping the bare form when nothing narrows the policy means an existing + // config or event stream is not rewritten into the table shape. + let rule = rule_from_toml(r#"tool_calls = "strip""#); + + let json = serde_json::to_value(&rule).unwrap(); + + assert_eq!(json["tool_calls"], serde_json::json!("strip")); +} + +#[test] +fn a_misspelled_option_key_is_rejected() { + // Silently dropping the leftover key would apply the policy to every item + // in range instead of the large ones, which is the opposite of what the + // line asks for. Every other type in this config tree rejects unknown + // fields, so this does too. + let err = toml::from_str::( + r#"tool_calls = { policy = "strip-responses", oer = "1MB" }"#, + ) + .unwrap_err() + .to_string(); + + assert!(err.contains("unknown policy option `oer`"), "{err}"); +} + +#[test] +fn a_tagged_policy_keeps_its_own_sibling_fields() { + // The rejection above must not catch a policy that legitimately serializes + // as a map with fields of its own, which is how the stored `ToolCallPolicy` + // is shaped. + #[derive(Debug, PartialEq, serde::Serialize, serde::Deserialize)] + #[serde(tag = "policy", rename_all = "snake_case")] + enum Tagged { + Strip { request: bool, response: bool }, + } + + let spec: PolicySpec = serde_json::from_value(serde_json::json!({ + "policy": "strip", + "request": false, + "response": true, + "over": "1MB", + })) + .unwrap(); + + assert_eq!(spec.policy, Tagged::Strip { + request: false, + response: true, + }); + assert_eq!(spec.over, Some(ByteSize::from_bytes(1024 * 1024))); +} + +#[test] +fn assigning_a_policy_string_accepts_an_over_option() { + // `--cfg ...tool_calls=sres,over=1MB` goes through `FromStr`, which shares + // its option syntax with the inline DSL. + let spec: PolicySpec = "sres,over=1MB".parse().unwrap(); + + assert_eq!( + spec, + PolicySpec::over( + ToolCallsMode::StripResponses, + ByteSize::from_bytes(1024 * 1024) + ) + ); +} + +#[test] +fn builtin_rules_carry_no_threshold() { + // The out-of-the-box behavior must stay "compact everything in range"; + // adding a default threshold would silently change every workspace. + let rules = PartialCompactionConfig::builtin_rules(); + + assert_eq!(rules.len(), 1); + assert_eq!(rules[0].reasoning.and_then(|spec| spec.over), None); + assert_eq!(rules[0].tool_calls.and_then(|spec| spec.over), None); +} #[test] fn tool_calls_mode_parse() { @@ -119,8 +253,8 @@ fn rule_partial_roundtrip_json() { let rule = PartialCompactionRuleConfig { keep_first: None, keep_last: Some(RuleBound::Turns(3)), - reasoning: Some(ReasoningMode::Strip), - tool_calls: Some(ToolCallsMode::Strip), + reasoning: Some(ReasoningMode::Strip.into()), + tool_calls: Some(ToolCallsMode::Strip.into()), summary: None, }; let json = serde_json::to_value(&rule).unwrap(); @@ -133,7 +267,7 @@ fn rule_partial_none_fields_omitted() { let rule = PartialCompactionRuleConfig { keep_first: None, keep_last: None, - reasoning: Some(ReasoningMode::Strip), + reasoning: Some(ReasoningMode::Strip.into()), tool_calls: None, summary: None, }; diff --git a/crates/jp_config/src/lib_tests.rs b/crates/jp_config/src/lib_tests.rs index 241fba266..eb0706af6 100644 --- a/crates/jp_config/src/lib_tests.rs +++ b/crates/jp_config/src/lib_tests.rs @@ -278,7 +278,7 @@ fn compaction_rule_unset_bounds_resolve_to_field_defaults() { // A rule that sets only a tool-call policy, leaving keep_first/keep_last // unset — exactly what `jp c compact -t sreq` produces. partial.conversation.compaction.rules = MergeableVec::Vec(vec![PartialCompactionRuleConfig { - tool_calls: Some(ToolCallsMode::StripRequests), + tool_calls: Some(ToolCallsMode::StripRequests.into()), ..Default::default() }]); @@ -314,8 +314,8 @@ fn empty_config_preserves_default_compaction_rule() { let rules = &config.conversation.compaction.rules; assert_eq!(rules.len(), 1, "default rule must survive an empty config"); - assert_eq!(rules[0].reasoning, Some(ReasoningMode::Strip)); - assert_eq!(rules[0].tool_calls, Some(ToolCallsMode::Strip)); + assert_eq!(rules[0].reasoning, Some(ReasoningMode::Strip.into())); + assert_eq!(rules[0].tool_calls, Some(ToolCallsMode::Strip.into())); assert_eq!(rules[0].keep_first, RuleBound::Turns(1)); assert_eq!(rules[0].keep_last, RuleBound::Turns(1)); } 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 9a71fc347..7590d4d50 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 @@ -140,10 +140,16 @@ Ok( keep_first: None, keep_last: None, reasoning: Some( - Strip, + PolicySpec { + policy: Strip, + over: None, + }, ), tool_calls: Some( - Strip, + PolicySpec { + policy: Strip, + over: None, + }, ), summary: None, }, diff --git a/crates/jp_config/src/types.rs b/crates/jp_config/src/types.rs index a4c94f27c..96c6ab9b1 100644 --- a/crates/jp_config/src/types.rs +++ b/crates/jp_config/src/types.rs @@ -1,9 +1,11 @@ //! Extended configuration types. +pub mod byte_size; pub mod color; pub mod command; pub mod extending_path; pub mod json_value; pub mod map; +pub mod policy_spec; pub mod string; pub mod vec; diff --git a/crates/jp_config/src/types/byte_size.rs b/crates/jp_config/src/types/byte_size.rs new file mode 100644 index 000000000..f3cad2754 --- /dev/null +++ b/crates/jp_config/src/types/byte_size.rs @@ -0,0 +1,187 @@ +//! Human-readable byte sizes. +//! +//! [`ByteSize`] is the unit for size thresholds in configuration. +//! It accepts a human-readable string (`"1MB"`, `"512 KB"`) or a bare byte +//! count, and compares as a plain number of bytes. +//! +//! ```toml +//! [[conversation.compaction.rules]] +//! tool_calls = { policy = "strip-responses", over = "1MB" } +//! ``` + +use std::{fmt, str::FromStr}; + +use schematic::{Schema, SchemaBuilder, Schematic}; +use serde::{Deserialize, Serialize}; + +use crate::BoxedError; + +/// One kibibyte, in bytes. +const KB: u64 = 1024; +/// One mebibyte, in bytes. +const MB: u64 = KB * 1024; +/// One gibibyte, in bytes. +const GB: u64 = MB * 1024; + +/// Units recognized on input and used on output, largest first. +const UNITS: [(u64, &str); 3] = [(GB, "GB"), (MB, "MB"), (KB, "KB")]; + +/// A size in bytes. +/// +/// Written as a human-readable string (`"1MB"`, `"512 KB"`, `"4GiB"`) or a bare +/// byte count (`1048576`). +/// +/// Unit suffixes are binary: `1KB` is 1024 bytes, `1MB` is 1048576 bytes. +/// `KiB` / `MiB` / `GiB` are accepted as explicit spellings of the same values. +#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Default, Hash)] +pub struct ByteSize(u64); + +impl ByteSize { + /// A size of zero bytes. + pub const ZERO: Self = Self(0); + + /// Build a size from a raw byte count. + #[must_use] + pub const fn from_bytes(bytes: u64) -> Self { + Self(bytes) + } + + /// The size as a raw byte count. + #[must_use] + pub const fn as_bytes(self) -> u64 { + self.0 + } + + /// An approximate, one-decimal rendering for terminal output, e.g. `10.4 + /// MB`. + /// + /// Lossy by design. + /// [`Display`] is the exact, round-trippable form used for serialization. + /// + /// [`Display`]: fmt::Display + #[must_use] + pub fn human(self) -> String { + for (size, suffix) in UNITS { + if self.0 >= size { + let whole = self.0 / size; + let tenths = (self.0 % size) * 10 / size; + return format!("{whole}.{tenths} {suffix}"); + } + } + format!("{} B", self.0) + } +} + +impl FromStr for ByteSize { + type Err = BoxedError; + + fn from_str(s: &str) -> Result { + let trimmed = s.trim(); + let digit_count = trimmed + .find(|c: char| !c.is_ascii_digit()) + .unwrap_or(trimmed.len()); + + if digit_count == 0 { + return Err(format!("invalid size `{s}`: expected a leading byte count").into()); + } + + let value: u64 = trimmed[..digit_count] + .parse() + .map_err(|_| format!("invalid size `{s}`: byte count out of range"))?; + + let multiplier = match trimmed[digit_count..].trim().to_ascii_lowercase().as_str() { + "" | "b" => 1, + "k" | "kb" | "kib" => KB, + "m" | "mb" | "mib" => MB, + "g" | "gb" | "gib" => GB, + unit => { + return Err(format!( + "invalid size `{s}`: unknown unit `{unit}` (expected B, KB, MB, or GB)" + ) + .into()); + } + }; + + value.checked_mul(multiplier).map_or_else( + || Err(format!("invalid size `{s}`: value overflows").into()), + |bytes| Ok(Self(bytes)), + ) + } +} + +impl fmt::Display for ByteSize { + /// Render the exact size, using the largest unit that divides it evenly. + /// + /// The output always parses back to the same value, which is what makes it + /// safe to use as the serialized form. + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + for (size, suffix) in UNITS { + if self.0 >= size && self.0.is_multiple_of(size) { + return write!(f, "{}{suffix}", self.0 / size); + } + } + write!(f, "{}", self.0) + } +} + +impl From for ByteSize { + fn from(bytes: u64) -> Self { + Self(bytes) + } +} + +impl Serialize for ByteSize { + fn serialize(&self, serializer: S) -> Result { + serializer.serialize_str(&self.to_string()) + } +} + +impl<'de> Deserialize<'de> for ByteSize { + fn deserialize>(deserializer: D) -> Result { + struct ByteSizeVisitor; + + impl serde::de::Visitor<'_> for ByteSizeVisitor { + type Value = ByteSize; + + fn expecting(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result { + formatter.write_str("a byte count or a size string like `1MB`") + } + + fn visit_u64(self, v: u64) -> Result { + Ok(ByteSize(v)) + } + + fn visit_i64(self, v: i64) -> Result { + u64::try_from(v) + .map(ByteSize) + .map_err(|_| E::custom(format!("size must be non-negative, got `{v}`"))) + } + + fn visit_str(self, v: &str) -> Result { + v.parse().map_err(E::custom) + } + } + + // `deserialize_any` lets self-describing formats supply either an + // integer (`over = 1048576`) or a string (`over = "1MB"`). + deserializer.deserialize_any(ByteSizeVisitor) + } +} + +impl Schematic for ByteSize { + fn build_schema(mut schema: SchemaBuilder) -> Schema { + // Accepts either a bare integer (byte count) or a string (`"1MB"`), + // matching what the deserializer takes. + schema.union(schematic::schema::UnionType { + variants_types: vec![ + Box::new(schema.infer::()), + Box::new(schema.infer::()), + ], + ..Default::default() + }) + } +} + +#[cfg(test)] +#[path = "byte_size_tests.rs"] +mod tests; diff --git a/crates/jp_config/src/types/byte_size_tests.rs b/crates/jp_config/src/types/byte_size_tests.rs new file mode 100644 index 000000000..8639423e0 --- /dev/null +++ b/crates/jp_config/src/types/byte_size_tests.rs @@ -0,0 +1,132 @@ +use super::*; + +// --------------------------------------------------------------------------- +// Parsing +// --------------------------------------------------------------------------- + +#[test] +fn parses_bare_byte_count() { + assert_eq!("1024".parse::().unwrap().as_bytes(), 1024); + assert_eq!("0".parse::().unwrap().as_bytes(), 0); +} + +#[test] +fn parses_binary_units() { + // Unit suffixes are binary, so `1MB` is 1048576 rather than 1000000. + assert_eq!("1KB".parse::().unwrap().as_bytes(), 1024); + assert_eq!("1MB".parse::().unwrap().as_bytes(), 1_048_576); + assert_eq!("1GB".parse::().unwrap().as_bytes(), 1_073_741_824); + assert_eq!("256KB".parse::().unwrap().as_bytes(), 262_144); +} + +#[test] +fn parses_unit_spelling_variants() { + // `KiB` is an explicit spelling of the same binary value as `KB`, and + // casing and internal spacing are not significant. + for input in ["1KB", "1kb", "1KiB", "1kib", "1 KB", " 1MB ".trim(), "1k"] { + let parsed = input.parse::().unwrap(); + assert!( + parsed.as_bytes() == 1024 || parsed.as_bytes() == 1_048_576, + "`{input}` parsed to {} bytes", + parsed.as_bytes() + ); + } + + assert_eq!("1KiB".parse::().unwrap(), "1KB".parse().unwrap()); + assert_eq!("4MiB".parse::().unwrap().as_bytes(), 4_194_304); +} + +#[test] +fn parses_explicit_byte_suffix() { + assert_eq!("512B".parse::().unwrap().as_bytes(), 512); +} + +#[test] +fn rejects_unknown_unit() { + let err = "1TB".parse::().unwrap_err().to_string(); + assert_eq!( + err, + "invalid size `1TB`: unknown unit `tb` (expected B, KB, MB, or GB)" + ); +} + +#[test] +fn rejects_missing_byte_count() { + let err = "MB".parse::().unwrap_err().to_string(); + assert_eq!(err, "invalid size `MB`: expected a leading byte count"); +} + +#[test] +fn rejects_overflow() { + let err = "99999999999999999999GB" + .parse::() + .unwrap_err() + .to_string(); + assert_eq!( + err, + "invalid size `99999999999999999999GB`: byte count out of range" + ); + + let err = "17179869184GB".parse::().unwrap_err().to_string(); + assert_eq!(err, "invalid size `17179869184GB`: value overflows"); +} + +// --------------------------------------------------------------------------- +// Display +// --------------------------------------------------------------------------- + +#[test] +fn display_uses_largest_evenly_dividing_unit() { + assert_eq!(ByteSize::from_bytes(1_048_576).to_string(), "1MB"); + assert_eq!(ByteSize::from_bytes(262_144).to_string(), "256KB"); + assert_eq!(ByteSize::from_bytes(1_073_741_824).to_string(), "1GB"); + assert_eq!(ByteSize::from_bytes(512).to_string(), "512"); + assert_eq!(ByteSize::from_bytes(0).to_string(), "0"); +} + +#[test] +fn display_round_trips_exactly() { + // The serialized form is `Display`, so any value must parse back to itself + // even when no unit divides it evenly. + for bytes in [0, 1, 512, 1024, 1500, 1_048_576, 10_900_000, 1_073_741_825] { + let size = ByteSize::from_bytes(bytes); + let parsed: ByteSize = size.to_string().parse().unwrap(); + assert_eq!(parsed, size, "{bytes} did not round-trip"); + } +} + +#[test] +fn human_is_approximate_with_one_decimal() { + assert_eq!(ByteSize::from_bytes(10_900_000).human(), "10.3 MB"); + assert_eq!(ByteSize::from_bytes(1_048_576).human(), "1.0 MB"); + assert_eq!(ByteSize::from_bytes(1536).human(), "1.5 KB"); + assert_eq!(ByteSize::from_bytes(512).human(), "512 B"); +} + +// --------------------------------------------------------------------------- +// Serde +// --------------------------------------------------------------------------- + +#[test] +fn serializes_as_a_string() { + let json = serde_json::to_value(ByteSize::from_bytes(1_048_576)).unwrap(); + assert_eq!(json, serde_json::json!("1MB")); +} + +#[test] +fn deserializes_from_string_or_integer() { + let from_string: ByteSize = serde_json::from_value(serde_json::json!("1MB")).unwrap(); + let from_integer: ByteSize = serde_json::from_value(serde_json::json!(1_048_576)).unwrap(); + + assert_eq!(from_string, from_integer); + assert_eq!(from_string.as_bytes(), 1_048_576); +} + +#[test] +fn rejects_negative_integer() { + let err = serde_json::from_value::(serde_json::json!(-1)).unwrap_err(); + assert!( + err.to_string().contains("size must be non-negative"), + "unexpected error: {err}" + ); +} diff --git a/crates/jp_config/src/types/policy_spec.rs b/crates/jp_config/src/types/policy_spec.rs new file mode 100644 index 000000000..088b94834 --- /dev/null +++ b/crates/jp_config/src/types/policy_spec.rs @@ -0,0 +1,222 @@ +//! A compaction policy paired with the options that qualify when it applies. +//! +//! A policy says *what* to do to an item; the options attached here say *which +//! items* it reaches. +//! Today the only option is `over`, a size threshold: +//! +//! ```toml +//! [[conversation.compaction.rules]] +//! # Strip every reasoning block in range. +//! reasoning = "strip" +//! # Strip only the tool responses that are actually large. +//! tool_calls = { policy = "strip-responses", over = "1MB" } +//! ``` +//! +//! The bare form and the table form mean the same thing when no option is set, +//! and a spec without options serializes back out as the bare form. + +use std::{fmt, str::FromStr}; + +use schematic::{Schema, SchemaBuilder, Schematic}; +use serde::{Deserialize, Serialize, de::DeserializeOwned}; +use serde_json::{Map, Value}; + +use super::byte_size::ByteSize; +use crate::BoxedError; + +/// A compaction policy plus the options qualifying which items it applies to. +/// +/// `over` limits the policy to items whose decoded content exceeds the given +/// size. +/// Unset means the policy applies to every item in range. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub struct PolicySpec

{ + /// What to do to the items this spec covers. + pub policy: P, + + /// Apply the policy only to items larger than this. + /// + /// The comparison is strict: `over = "1MB"` leaves an item of exactly 1 MB + /// alone. + pub over: Option, +} + +impl

PolicySpec

{ + /// A spec that applies its policy to every item in range. + pub const fn new(policy: P) -> Self { + Self { policy, over: None } + } + + /// A spec that applies its policy only to items larger than `over`. + pub const fn over(policy: P, over: ByteSize) -> Self { + Self { + policy, + over: Some(over), + } + } + + /// Whether an item of `size` bytes is large enough for the policy to apply. + /// + /// Always true when no threshold is set. + #[must_use] + pub fn covers(&self, size: u64) -> bool { + self.over + .is_none_or(|threshold| size > threshold.as_bytes()) + } +} + +impl

From

for PolicySpec

{ + fn from(policy: P) -> Self { + Self::new(policy) + } +} + +impl Serialize for PolicySpec

{ + /// Serialize as the bare policy when no option is set, so a stream that + /// uses no thresholds keeps the shape older readers expect. + fn serialize(&self, serializer: S) -> Result { + use serde::ser::Error as _; + + let Some(over) = self.over else { + return self.policy.serialize(serializer); + }; + + // A policy that serializes to a map (one carrying its own `policy` tag, + // such as `ToolCallPolicy`) gains `over` alongside its own fields. One + // that serializes to a bare string is promoted to `{"policy": "..."}` + // so the option has somewhere to live. + let mut object = match serde_json::to_value(&self.policy).map_err(S::Error::custom)? { + Value::Object(map) => map, + bare => Map::from_iter([("policy".to_owned(), bare)]), + }; + + object.insert( + "over".to_owned(), + serde_json::to_value(over).map_err(S::Error::custom)?, + ); + + object.serialize(serializer) + } +} + +impl<'de, P: DeserializeOwned> Deserialize<'de> for PolicySpec

{ + fn deserialize>(deserializer: D) -> Result { + use serde::de::Error as _; + + let mut value = Value::deserialize(deserializer)?; + + let over = match value.as_object_mut().and_then(|map| map.remove("over")) { + Some(raw) => Some(serde_json::from_value(raw).map_err(D::Error::custom)?), + None => None, + }; + + // `P` may serialize either as a map carrying its own tag or as a bare + // string that `Serialize` promoted into `{"policy": "..."}`. Which one + // is not knowable from here, so try the whole value first and fall back + // to the promoted string. Trying the map first matters: a tagged unit + // variant (`{"policy": "omit"}`) is indistinguishable from a promotion + // by shape alone, and only the map reading is correct for it. + let policy = match serde_json::from_value::

(value.clone()) { + Ok(policy) => policy, + Err(error) => { + let Some(map) = value.as_object() else { + return Err(D::Error::custom(error)); + }; + let promoted = map + .get("policy") + .cloned() + .ok_or_else(|| D::Error::custom(&error))?; + + // `P` did not consume the map, so `policy` is the only key it + // can account for and anything left is a mistake. Reporting it + // matters because the leftover is typically a misspelled option + // (`oer = "1MB"`), and silently dropping it would apply the + // policy to every item instead of the large ones. The rest of + // this config tree rejects unknown fields, so this does too. + if let Some(unknown) = map.keys().find(|key| *key != "policy") { + return Err(D::Error::custom(format!( + "unknown policy option `{unknown}`" + ))); + } + + serde_json::from_value(promoted).map_err(|_| D::Error::custom(error))? + } + }; + + Ok(Self { policy, over }) + } +} + +impl FromStr for PolicySpec

+where + P::Err: Into, +{ + type Err = BoxedError; + + /// Parse `POLICY` or `POLICY,option=value`. + /// + /// The separator matches the inline compaction DSL, so `--cfg + /// ...tool_calls=strip-responses,over=1MB` and `-k 't=sres,over=1MB'` are + /// written the same way. + fn from_str(s: &str) -> Result { + let mut parts = s.split(','); + let policy = parts + .next() + .unwrap_or_default() + .trim() + .parse() + .map_err(Into::into)?; + + let mut spec = Self::new(policy); + for option in parts { + let (key, value) = option.split_once('=').ok_or_else(|| { + format!( + "invalid policy option `{}`: expected `key=value`", + option.trim() + ) + })?; + + match key.trim() { + "over" => spec.over = Some(value.trim().parse()?), + other => return Err(format!("unknown policy option `{other}`").into()), + } + } + + Ok(spec) + } +} + +impl fmt::Display for PolicySpec

{ + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + write!(f, "{}", self.policy)?; + if let Some(over) = self.over { + write!(f, ",over={over}")?; + } + Ok(()) + } +} + +impl Schematic for PolicySpec

{ + fn build_schema(mut schema: SchemaBuilder) -> Schema { + // Either the bare policy, or a table carrying it alongside the options + // that qualify it. The table is described field by field so a schema + // consumer still validates `policy` against `P` and still rejects an + // unknown key, rather than falling back to "any value here". + let table = schema.nest().structure(schematic::schema::StructType { + required: Some(vec!["policy".to_owned()]), + ..schematic::schema::StructType::new([ + ("policy".to_owned(), schema.infer::

()), + ("over".to_owned(), schema.infer::()), + ]) + }); + + schema.union(schematic::schema::UnionType { + variants_types: vec![Box::new(schema.infer::

()), Box::new(table)], + ..Default::default() + }) + } +} + +#[cfg(test)] +#[path = "policy_spec_tests.rs"] +mod tests; diff --git a/crates/jp_config/src/types/policy_spec_tests.rs b/crates/jp_config/src/types/policy_spec_tests.rs new file mode 100644 index 000000000..90780c814 --- /dev/null +++ b/crates/jp_config/src/types/policy_spec_tests.rs @@ -0,0 +1,223 @@ +use serde::{Deserialize, Serialize}; + +use super::*; +use crate::conversation::compaction::{ReasoningMode, ToolCallsMode}; + +/// A stand-in for a policy that carries its own tag and extra fields, matching +/// the shape of `jp_conversation`'s `ToolCallPolicy`. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)] +#[serde(tag = "policy", rename_all = "snake_case")] +enum TaggedPolicy { + Strip { request: bool, response: bool }, + Omit, +} + +// --------------------------------------------------------------------------- +// Serialization shape +// --------------------------------------------------------------------------- + +#[test] +fn bare_policy_serializes_without_a_wrapper() { + // A spec with no options must keep the shape a plain policy already had, + // so streams and configs that set no threshold are untouched. + let spec = PolicySpec::new(ToolCallsMode::StripResponses); + + let json = serde_json::to_value(spec).unwrap(); + + assert_eq!(json, serde_json::json!("strip-responses")); +} + +#[test] +fn string_policy_with_option_is_promoted_to_a_table() { + let spec = PolicySpec::over(ReasoningMode::Strip, ByteSize::from_bytes(16 * 1024)); + + let json = serde_json::to_value(spec).unwrap(); + + assert_eq!( + json, + serde_json::json!({ "policy": "strip", "over": "16KB" }) + ); +} + +#[test] +fn tagged_policy_with_option_gains_a_sibling_key() { + // A policy that already serializes as a map keeps its own fields at the top + // level rather than nesting under a wrapper. + let spec = PolicySpec::over( + TaggedPolicy::Strip { + request: false, + response: true, + }, + ByteSize::from_bytes(1024 * 1024), + ); + + let json = serde_json::to_value(spec).unwrap(); + + assert_eq!( + json, + serde_json::json!({ + "policy": "strip", + "request": false, + "response": true, + "over": "1MB", + }) + ); +} + +// --------------------------------------------------------------------------- +// Deserialization +// --------------------------------------------------------------------------- + +#[test] +fn reads_a_bare_string_policy() { + let spec: PolicySpec = + serde_json::from_value(serde_json::json!("omit")).unwrap(); + + assert_eq!(spec, PolicySpec::new(ToolCallsMode::Omit)); +} + +#[test] +fn reads_a_promoted_string_policy() { + let spec: PolicySpec = + serde_json::from_value(serde_json::json!({ "policy": "strip", "over": "16KB" })).unwrap(); + + assert_eq!( + spec, + PolicySpec::over(ReasoningMode::Strip, ByteSize::from_bytes(16 * 1024)) + ); +} + +#[test] +fn reads_a_tagged_unit_variant_as_the_policy_itself() { + // `{"policy": "omit"}` is both a valid tagged policy and shaped exactly + // like a promoted string. The tagged reading is the correct one, so it must + // win. + let spec: PolicySpec = + serde_json::from_value(serde_json::json!({ "policy": "omit" })).unwrap(); + + assert_eq!(spec, PolicySpec::new(TaggedPolicy::Omit)); +} + +#[test] +fn reads_a_tagged_unit_variant_carrying_an_option() { + let spec: PolicySpec = + serde_json::from_value(serde_json::json!({ "policy": "omit", "over": "2MB" })).unwrap(); + + assert_eq!( + spec, + PolicySpec::over(TaggedPolicy::Omit, ByteSize::from_bytes(2 * 1024 * 1024)) + ); +} + +#[test] +fn round_trips_with_and_without_an_option() { + let cases = [ + PolicySpec::new(TaggedPolicy::Strip { + request: true, + response: true, + }), + PolicySpec::over( + TaggedPolicy::Strip { + request: true, + response: false, + }, + ByteSize::from_bytes(4096), + ), + PolicySpec::new(TaggedPolicy::Omit), + PolicySpec::over(TaggedPolicy::Omit, ByteSize::from_bytes(512)), + ]; + + for spec in cases { + let json = serde_json::to_value(spec).unwrap(); + let parsed: PolicySpec = serde_json::from_value(json).unwrap(); + assert_eq!(parsed, spec); + } +} + +#[test] +fn reports_the_policy_error_for_an_unreadable_value() { + let err = serde_json::from_value::>(serde_json::json!("nonsense")) + .unwrap_err() + .to_string(); + + assert!( + err.contains("unknown tool_calls mode"), + "unexpected error: {err}" + ); +} + +// --------------------------------------------------------------------------- +// String form (`--cfg` assignment and the inline DSL) +// --------------------------------------------------------------------------- + +#[test] +fn parses_a_bare_policy_string() { + let spec: PolicySpec = "strip-responses".parse().unwrap(); + + assert_eq!(spec, PolicySpec::new(ToolCallsMode::StripResponses)); +} + +#[test] +fn parses_a_policy_string_with_an_over_option() { + let spec: PolicySpec = "sres,over=1MB".parse().unwrap(); + + assert_eq!( + spec, + PolicySpec::over( + ToolCallsMode::StripResponses, + ByteSize::from_bytes(1024 * 1024) + ) + ); +} + +#[test] +fn rejects_an_unknown_option() { + let err = "strip,under=1MB" + .parse::>() + .unwrap_err() + .to_string(); + + assert_eq!(err, "unknown policy option `under`"); +} + +#[test] +fn rejects_an_option_without_a_value() { + let err = "strip,over" + .parse::>() + .unwrap_err() + .to_string(); + + assert_eq!(err, "invalid policy option `over`: expected `key=value`"); +} + +#[test] +fn displays_the_form_it_parses() { + for input in ["strip", "strip-responses,over=1MB", "omit,over=512"] { + let spec: PolicySpec = input.parse().unwrap(); + assert_eq!(spec.to_string(), input); + } +} + +// --------------------------------------------------------------------------- +// Threshold predicate +// --------------------------------------------------------------------------- + +#[test] +fn covers_everything_without_a_threshold() { + let spec = PolicySpec::new(ToolCallsMode::Strip); + + assert!(spec.covers(0)); + assert!(spec.covers(u64::MAX)); +} + +#[test] +fn threshold_is_strictly_exclusive() { + let spec = PolicySpec::over(ToolCallsMode::Strip, ByteSize::from_bytes(1024)); + + assert!(!spec.covers(1023)); + assert!( + !spec.covers(1024), + "a payload of exactly the threshold is left alone" + ); + assert!(spec.covers(1025)); +} diff --git a/crates/jp_conversation/src/compaction.rs b/crates/jp_conversation/src/compaction.rs index 6d7b11c51..d36a3ba3c 100644 --- a/crates/jp_conversation/src/compaction.rs +++ b/crates/jp_conversation/src/compaction.rs @@ -11,6 +11,7 @@ //! [RFD 064]: https://github.com/dcdpr/jp/blob/main/docs/rfd/064-non-destructive-conversation-compaction.md use chrono::{DateTime, Utc}; +pub use jp_config::types::{byte_size::ByteSize, policy_spec::PolicySpec}; use serde::{Deserialize, Serialize}; /// A compaction overlay stored in the event stream. @@ -45,13 +46,19 @@ pub struct Compaction { /// Policy for `ChatResponse::Reasoning` events. /// Ignored when `summary` is set. + /// + /// An `over` threshold on the spec limits the policy to reasoning blocks + /// larger than that size. #[serde(default, skip_serializing_if = "Option::is_none")] - pub reasoning: Option, + pub reasoning: Option>, /// Policy for `ToolCallRequest` and `ToolCallResponse` pairs. /// Ignored when `summary` is set. + /// + /// An `over` threshold on the spec limits the policy to calls larger than + /// that size, judged per half for `Strip` and on the pair total for `Omit`. #[serde(default, skip_serializing_if = "Option::is_none")] - pub tool_calls: Option, + pub tool_calls: Option>, } impl Compaction { @@ -72,16 +79,22 @@ impl Compaction { } /// Set the reasoning policy. + /// + /// Accepts a bare [`ReasoningPolicy`] to apply it to every reasoning block + /// in range, or a [`PolicySpec`] carrying a size threshold. #[must_use] - pub const fn with_reasoning(mut self, policy: ReasoningPolicy) -> Self { - self.reasoning = Some(policy); + pub fn with_reasoning(mut self, policy: impl Into>) -> Self { + self.reasoning = Some(policy.into()); self } /// Set the tool call policy. + /// + /// Accepts a bare [`ToolCallPolicy`] to apply it to every tool call in + /// range, or a [`PolicySpec`] carrying a size threshold. #[must_use] - pub const fn with_tool_calls(mut self, policy: ToolCallPolicy) -> Self { - self.tool_calls = Some(policy); + pub fn with_tool_calls(mut self, policy: impl Into>) -> Self { + self.tool_calls = Some(policy.into()); self } diff --git a/crates/jp_conversation/src/compaction_tests.rs b/crates/jp_conversation/src/compaction_tests.rs index 4a3fa1047..dc5b1703b 100644 --- a/crates/jp_conversation/src/compaction_tests.rs +++ b/crates/jp_conversation/src/compaction_tests.rs @@ -10,7 +10,7 @@ use crate::ConversationStream; #[test] fn builder_with_reasoning() { let c = Compaction::new(0, 5).with_reasoning(ReasoningPolicy::Strip); - assert_eq!(c.reasoning, Some(ReasoningPolicy::Strip)); + assert_eq!(c.reasoning, Some(ReasoningPolicy::Strip.into())); assert!(c.tool_calls.is_none()); assert!(c.summary.is_none()); } @@ -18,7 +18,7 @@ fn builder_with_reasoning() { #[test] fn builder_with_tool_calls() { let c = Compaction::new(0, 5).with_tool_calls(ToolCallPolicy::Omit); - assert_eq!(c.tool_calls, Some(ToolCallPolicy::Omit)); + assert_eq!(c.tool_calls, Some(ToolCallPolicy::Omit.into())); } #[test] @@ -44,11 +44,14 @@ fn sample_compaction() -> Compaction { from_turn: 0, to_turn: 5, summary: None, - reasoning: Some(ReasoningPolicy::Strip), - tool_calls: Some(ToolCallPolicy::Strip { - request: true, - response: true, - }), + reasoning: Some(ReasoningPolicy::Strip.into()), + tool_calls: Some( + ToolCallPolicy::Strip { + request: true, + response: true, + } + .into(), + ), } } @@ -85,7 +88,7 @@ fn none_policies_omitted_from_json() { from_turn: 0, to_turn: 3, summary: None, - reasoning: Some(ReasoningPolicy::Strip), + reasoning: Some(ReasoningPolicy::Strip.into()), tool_calls: None, }; @@ -303,7 +306,7 @@ fn extend_ignores_mechanical_compactions() { from_turn: 0, to_turn: 9, summary: None, - reasoning: Some(ReasoningPolicy::Strip), + reasoning: Some(ReasoningPolicy::Strip.into()), tool_calls: None, }); diff --git a/crates/jp_conversation/src/lib.rs b/crates/jp_conversation/src/lib.rs index d5d82f296..77d35a45e 100644 --- a/crates/jp_conversation/src/lib.rs +++ b/crates/jp_conversation/src/lib.rs @@ -37,8 +37,8 @@ pub mod stream; pub mod thread; pub use compaction::{ - Compaction, CompactionRange, RangeBound, ReasoningPolicy, SummaryPolicy, ToolCallPolicy, - resolve_range, + ByteSize, Compaction, CompactionRange, PolicySpec, RangeBound, ReasoningPolicy, SummaryPolicy, + ToolCallPolicy, resolve_range, }; pub use conversation::{Conversation, ConversationId}; pub use error::Error; diff --git a/crates/jp_conversation/src/stream.rs b/crates/jp_conversation/src/stream.rs index beec86bc8..263f1384d 100644 --- a/crates/jp_conversation/src/stream.rs +++ b/crates/jp_conversation/src/stream.rs @@ -11,7 +11,7 @@ use tracing::{error, warn}; mod projection; pub mod turn_iter; pub mod turn_mut; -pub use projection::TurnOrigin; +pub use projection::{AffectedItem, TurnOrigin}; pub use turn_iter::{IterTurns, Turn}; pub use turn_mut::TurnMut; @@ -471,6 +471,20 @@ impl ConversationStream { projection::apply(&mut self.events) } + /// List the items `compaction`'s mechanical policies reach, in stream + /// order. + /// + /// A policy narrowed by a size threshold reaches an unpredictable subset of + /// its range, so this reports which items it actually selects. + /// A policy without a threshold reaches everything in range, and a summary + /// replaces its range rather than selecting from it. + /// + /// The stream is not modified. + #[must_use] + pub fn affected_items(&self, compaction: &Compaction) -> Vec { + projection::affected_items(&self.events, compaction) + } + /// Start a new turn with the given chat request. /// /// Atomically adds a [`TurnStart`] and the [`ChatRequest`] to the stream. diff --git a/crates/jp_conversation/src/stream/projection.rs b/crates/jp_conversation/src/stream/projection.rs index d3e2434ea..05cfa128e 100644 --- a/crates/jp_conversation/src/stream/projection.rs +++ b/crates/jp_conversation/src/stream/projection.rs @@ -5,14 +5,14 @@ //! //! See [`apply`] for the entry point. -use std::collections::{HashMap, HashSet}; +use std::collections::{HashMap, HashSet, VecDeque}; use chrono::{DateTime, Utc}; -use serde_json::Map; +use serde_json::{Map, Value}; use super::InternalEvent; use crate::{ - ReasoningPolicy, ToolCallPolicy, + ByteSize, Compaction, PolicySpec, ReasoningPolicy, ToolCallPolicy, event::{ChatRequest, ChatResponse, ConversationEvent, TurnStart}, }; @@ -61,12 +61,12 @@ struct TurnPolicy { /// Summary covering this turn. /// Takes precedence over per-type policies. summary: Option, - /// Reasoning policy. + /// Reasoning policy, with any size threshold that qualifies it. /// Ignored when `summary` is set. - reasoning: Option, - /// Tool call policy. + reasoning: Option>, + /// Tool call policy, with any size threshold that qualifies it. /// Ignored when `summary` is set. - tool_calls: Option, + tool_calls: Option>, } /// A summary that won the latest-timestamp contest for a set of turns. @@ -127,7 +127,7 @@ pub(super) fn apply(events: &mut Vec) -> Vec { let turn_indices = assign_turn_indices(events); let max_turn = turn_indices.iter().copied().max().unwrap_or(0); let policies = resolve_policies(max_turn, &compactions); - let tool_names = build_tool_name_map(events); + let tool_calls = build_tool_calls(events, &turn_indices); // Inject a summary once per contiguous run of turns that resolve to the // same winning summary. Injecting only at the originating `from_turn` drops @@ -197,35 +197,9 @@ pub(super) fn apply(events: &mut Vec) -> Vec { continue; } - let mut event = *conv_event; - - // Reasoning policy. - if matches!(policy.reasoning, Some(ReasoningPolicy::Strip)) - && event - .as_chat_response() - .is_some_and(ChatResponse::is_reasoning) - { + let Some(event) = apply_mechanical(*conv_event, policy, tool_calls.get(&i)) else { continue; - } - - // Tool call policy. - if let Some(tc_policy) = &policy.tool_calls { - match tc_policy { - ToolCallPolicy::Omit => { - if event.is_tool_call_request() || event.is_tool_call_response() { - continue; - } - } - ToolCallPolicy::Strip { request, response } => { - if *request { - strip_tool_request(&mut event); - } - if *response { - strip_tool_response(&mut event, &tool_names); - } - } - } - } + }; projected.push(InternalEvent::Event(Box::new(event))); event_origins.push(TurnOrigin::Kept(turn)); @@ -237,6 +211,159 @@ pub(super) fn apply(events: &mut Vec) -> Vec { collect_turn_origins(events, &event_origins) } +/// An item a compaction's mechanical policies reach. +/// +/// Reported so a preview can say what a size threshold actually selected. +/// A turn range predicts what it covers; a threshold does not. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct AffectedItem { + /// 0-based raw turn the item sits in. + pub turn: usize, + /// What the item is: a tool name qualified by the half being reached + /// (`fs_read_file (response)`), or `reasoning`. + pub name: String, + /// Byte size of the content the policy would remove. + pub size: ByteSize, +} + +/// List the items `compaction`'s mechanical policies reach, in stream order. +/// +/// Only the reasoning and tool-call policies select items. +/// A summary replaces every event in its range rather than picking from it, so +/// a compaction carrying one reports nothing even when its mechanical policies +/// are narrowed: projection ignores them. +pub(super) fn affected_items( + events: &[InternalEvent], + compaction: &Compaction, +) -> Vec { + if compaction.summary.is_some() { + return Vec::new(); + } + + let turn_indices = assign_turn_indices(events); + let tool_calls = build_tool_calls(events, &turn_indices); + let mut items = Vec::new(); + + for (index, entry) in events.iter().enumerate() { + let turn = turn_indices[index]; + if turn < compaction.from_turn || turn > compaction.to_turn { + continue; + } + + let Some(event) = entry.as_event() else { + continue; + }; + + if let Some(spec) = &compaction.reasoning + && matches!(spec.policy, ReasoningPolicy::Strip) + && let Some(response) = event.as_chat_response() + && response.is_reasoning() + { + let size = reasoning_size(response); + if spec.covers(size) { + items.push(AffectedItem { + turn, + name: "reasoning".to_owned(), + size: ByteSize::from_bytes(size), + }); + } + } + + let Some(spec) = &compaction.tool_calls else { + continue; + }; + let Some(info) = tool_calls.get(&index) else { + continue; + }; + + match &spec.policy { + // Report the pair once, from its request, since both halves go + // together. + ToolCallPolicy::Omit => { + if event.is_tool_call_request() && spec.covers(info.pair) { + items.push(AffectedItem { + turn, + name: info.name.clone(), + size: ByteSize::from_bytes(info.pair), + }); + } + } + ToolCallPolicy::Strip { request, response } => { + if *request && event.is_tool_call_request() && spec.covers(info.own) { + items.push(AffectedItem { + turn, + name: format!("{} (request)", info.name), + size: ByteSize::from_bytes(info.own), + }); + } + if *response && event.is_tool_call_response() && spec.covers(info.own) { + items.push(AffectedItem { + turn, + name: format!("{} (response)", info.name), + size: ByteSize::from_bytes(info.own), + }); + } + } + } + } + + items +} + +/// Apply a turn's mechanical policies (reasoning and tool calls) to one event. +/// +/// Returns `None` when the policies drop the event from the projected view. +/// A policy whose spec carries an `over` threshold reaches only the items +/// larger than it; without one, every item in range is reached. +fn apply_mechanical( + mut event: ConversationEvent, + policy: &TurnPolicy, + info: Option<&ToolCallInfo>, +) -> Option { + if let Some(spec) = &policy.reasoning + && matches!(spec.policy, ReasoningPolicy::Strip) + && let Some(response) = event.as_chat_response() + && response.is_reasoning() + && spec.covers(reasoning_size(response)) + { + return None; + } + + // A `None` tool-call policy means "no opinion", so the event passes through + // untouched rather than being dropped. + if let Some(spec) = policy.tool_calls.as_ref() { + // A non-tool event has no entry, and reports zero, which no threshold + // covers. + let own = info.map_or(0, |i| i.own); + let pair = info.map_or(0, |i| i.pair); + + match &spec.policy { + ToolCallPolicy::Omit => { + // Removing a pair is not a per-half choice, so the threshold is + // judged on the two halves combined. Both halves read the same + // total, so a pair is never half-removed. + if (event.is_tool_call_request() || event.is_tool_call_response()) + && spec.covers(pair) + { + return None; + } + } + ToolCallPolicy::Strip { request, response } => { + // Each half is judged on its own size, so a call with a short + // request and a huge response loses only the response. + if *request && event.is_tool_call_request() && spec.covers(own) { + strip_tool_request(&mut event); + } + if *response && event.is_tool_call_response() && spec.covers(own) { + strip_tool_response(&mut event, info.map_or("unknown", |i| i.name.as_str())); + } + } + } + } + + Some(event) +} + /// Group projected events into turns (matching [`IterTurns`]) and return each /// turn's [`TurnOrigin`], read from the event that opens the turn. /// @@ -409,9 +536,11 @@ fn strip_tool_request(event: &mut ConversationEvent) { } /// Replace a tool call response's content with a compact status line. -fn strip_tool_response(event: &mut ConversationEvent, tool_names: &HashMap) { +/// +/// `name` is the tool named by the paired request, or `unknown` when no request +/// for it survives in the stream. +fn strip_tool_response(event: &mut ConversationEvent, name: &str) { if let Some(resp) = event.as_tool_call_response_mut() { - let name = tool_names.get(&resp.id).map_or("unknown", String::as_str); let status = if resp.result.is_ok() { "success" } else { @@ -426,17 +555,108 @@ fn strip_tool_response(event: &mut ConversationEvent, tool_names: &HashMap HashMap { - let mut map = HashMap::new(); - for event in events { - if let InternalEvent::Event(ev) = event - && let Some(req) = ev.as_tool_call_request() - { - map.insert(req.id.clone(), req.name.clone()); +/// What the tool call policies need to know about the tool call event at one +/// stream position. +/// +/// The name feeds a stripped response's status line; the sizes feed a spec's +/// `over` threshold. +#[derive(Default)] +struct ToolCallInfo { + /// Name of the tool, taken from the request half of the pair. + name: String, + /// Byte size of this event's own half. + own: u64, + /// Combined byte size of both halves of the pair. + /// + /// Equal to `own` for a half whose partner is missing from the stream. + pair: u64, +} + +/// Map each tool call event's stream position to its name and sizes. +/// +/// Keyed by position rather than by call ID because a call ID is not unique: a +/// provider may reuse one synthetic ID across streaming cycles, which +/// [`TurnMut::build`] explicitly permits. +/// Keying by ID would give every occurrence the last one's sizes, so a +/// threshold would reach the wrong halves. +/// +/// Pairing mirrors `TurnMut::build`'s count-based rule: a response binds to the +/// oldest request in its turn that carries the same ID and has no response yet. +/// +/// [`TurnMut::build`]: super::TurnMut::build +fn build_tool_calls( + events: &[InternalEvent], + turn_indices: &[usize], +) -> HashMap { + let mut calls: HashMap = HashMap::new(); + // Positions of requests still awaiting a response, oldest first, per + // (turn, call ID). Scoped by turn because request-response pairing is + // turn-local, so an orphaned request cannot capture a later turn's response. + let mut pending: HashMap<(usize, &str), VecDeque> = HashMap::new(); + + for (index, entry) in events.iter().enumerate() { + let Some(event) = entry.as_event() else { + continue; + }; + let turn = turn_indices[index]; + + if let Some(req) = event.as_tool_call_request() { + let own = arguments_size(&req.arguments); + calls.insert(index, ToolCallInfo { + name: req.name.clone(), + own, + // Stands alone until its response is found. + pair: own, + }); + pending + .entry((turn, req.id.as_str())) + .or_default() + .push_back(index); + } else if let Some(resp) = event.as_tool_call_response() { + let own = byte_count(resp.content().len()); + let request = pending + .get_mut(&(turn, resp.id.as_str())) + .and_then(VecDeque::pop_front); + + // Both halves must read the same `pair` total, so a threshold on + // `Omit` either removes a pair or leaves it whole. + let (name, pair) = match request.and_then(|pos| calls.get_mut(&pos)) { + Some(request) => { + let pair = request.own.saturating_add(own); + request.pair = pair; + (request.name.clone(), pair) + } + None => ("unknown".to_owned(), own), + }; + + calls.insert(index, ToolCallInfo { name, own, pair }); } } - map + + calls +} + +/// Byte size of a tool call request's arguments as the provider receives them. +/// +/// Measured on the serialized JSON rather than the stored bytes: arguments are +/// base64-encoded at rest, so the on-disk size is not what reaches the model. +fn arguments_size(arguments: &Map) -> u64 { + serde_json::to_string(arguments).map_or(0, |json| byte_count(json.len())) +} + +/// Byte size of a chat response's reasoning content. +/// +/// Any other response kind reports zero. +fn reasoning_size(response: &ChatResponse) -> u64 { + match response { + ChatResponse::Reasoning { reasoning } => byte_count(reasoning.len()), + _ => 0, + } +} + +/// Narrow an in-memory length to the width the size thresholds compare against. +fn byte_count(len: usize) -> u64 { + u64::try_from(len).unwrap_or(u64::MAX) } #[cfg(test)] diff --git a/crates/jp_conversation/src/stream/projection_tests.rs b/crates/jp_conversation/src/stream/projection_tests.rs index 50ecc499a..f2fb1fb16 100644 --- a/crates/jp_conversation/src/stream/projection_tests.rs +++ b/crates/jp_conversation/src/stream/projection_tests.rs @@ -5,8 +5,8 @@ use proptest::prelude::*; use serde_json::Map; use crate::{ - Compaction, ConversationEvent, ConversationStream, EventKind, ReasoningPolicy, SummaryPolicy, - ToolCallPolicy, + ByteSize, Compaction, ConversationEvent, ConversationStream, EventKind, PolicySpec, + ReasoningPolicy, SummaryPolicy, ToolCallPolicy, event::{ChatRequest, ChatResponse, ToolCallRequest, ToolCallResponse, TurnStart}, stream::TurnOrigin, }; @@ -179,7 +179,7 @@ fn mechanical_compaction_keeps_one_to_one_origins() { from_turn: 0, to_turn: 1, summary: None, - reasoning: Some(ReasoningPolicy::Strip), + reasoning: Some(ReasoningPolicy::Strip.into()), tool_calls: None, }); @@ -236,7 +236,7 @@ fn strip_reasoning_removes_reasoning_events() { from_turn: 0, to_turn: 1, summary: None, - reasoning: Some(ReasoningPolicy::Strip), + reasoning: Some(ReasoningPolicy::Strip.into()), tool_calls: None, }); @@ -275,10 +275,13 @@ fn strip_request_only_blanks_args_and_keeps_response() { to_turn: 1, summary: None, reasoning: None, - tool_calls: Some(ToolCallPolicy::Strip { - request: true, - response: false, - }), + tool_calls: Some( + ToolCallPolicy::Strip { + request: true, + response: false, + } + .into(), + ), }); stream.apply_projection(); @@ -315,10 +318,13 @@ fn strip_tool_calls_replaces_content() { to_turn: 1, summary: None, reasoning: None, - tool_calls: Some(ToolCallPolicy::Strip { - request: true, - response: true, - }), + tool_calls: Some( + ToolCallPolicy::Strip { + request: true, + response: true, + } + .into(), + ), }); stream.apply_projection(); @@ -357,10 +363,13 @@ fn strip_tool_response_only() { to_turn: 1, summary: None, reasoning: None, - tool_calls: Some(ToolCallPolicy::Strip { - request: false, - response: true, - }), + tool_calls: Some( + ToolCallPolicy::Strip { + request: false, + response: true, + } + .into(), + ), }); stream.apply_projection(); @@ -410,10 +419,13 @@ fn strip_tool_response_preserves_error_status() { to_turn: 0, summary: None, reasoning: None, - tool_calls: Some(ToolCallPolicy::Strip { - request: false, - response: true, - }), + tool_calls: Some( + ToolCallPolicy::Strip { + request: false, + response: true, + } + .into(), + ), }); stream.apply_projection(); @@ -423,6 +435,548 @@ fn strip_tool_response_preserves_error_status() { assert_eq!(resp.content(), "[compacted] cargo_test: error"); } +// --------------------------------------------------------------------------- +// Size thresholds (`over`) +// --------------------------------------------------------------------------- + +/// One kibibyte, the threshold every test in this section uses. +const ONE_KB: u64 = 1024; + +/// A single turn holding two tool calls of very different sizes. +/// +/// `big` returns 4096 bytes and `small` returns 2, so a 1 KB threshold +/// separates the two responses. +/// Both requests carry ~16 bytes of arguments, so neither request crosses that +/// threshold. +fn mixed_size_tool_calls() -> ConversationStream { + let mut stream = ConversationStream::new_test(); + + stream.push(ConversationEvent::new(TurnStart, ts(0))); + stream.push(ConversationEvent::new( + ChatRequest::from("read them"), + ts(0), + )); + + stream.push(ConversationEvent::new( + ToolCallRequest { + id: "big".into(), + name: "fs_read_file".into(), + arguments: Map::from_iter([("path".into(), "huge.log".into())]), + }, + ts(0), + )); + stream.push(ConversationEvent::new( + ToolCallResponse { + id: "big".into(), + result: Ok("x".repeat(4096)), + }, + ts(0), + )); + + stream.push(ConversationEvent::new( + ToolCallRequest { + id: "small".into(), + name: "fs_read_file".into(), + arguments: Map::from_iter([("path".into(), "tiny.log".into())]), + }, + ts(0), + )); + stream.push(ConversationEvent::new( + ToolCallResponse { + id: "small".into(), + result: Ok("ok".into()), + }, + ts(0), + )); + + stream +} + +/// A compaction over turn 0 carrying `policy`. +fn tool_call_compaction(policy: PolicySpec) -> Compaction { + Compaction { + timestamp: ts(1), + from_turn: 0, + to_turn: 0, + summary: None, + reasoning: None, + tool_calls: Some(policy), + } +} + +#[test] +fn over_threshold_strips_only_the_large_response() { + let mut stream = mixed_size_tool_calls(); + stream.add_compaction(tool_call_compaction(PolicySpec::over( + ToolCallPolicy::Strip { + request: false, + response: true, + }, + ByteSize::from_bytes(ONE_KB), + ))); + + stream.apply_projection(); + + assert_eq!( + stream.find_tool_call_response("big").unwrap().content(), + "[compacted] fs_read_file: success" + ); + assert_eq!( + stream.find_tool_call_response("small").unwrap().content(), + "ok", + "a response under the threshold must survive verbatim" + ); +} + +#[test] +fn without_a_threshold_every_response_is_stripped() { + // The counterpart to the test above: the same stream and the same policy, + // minus the threshold, reaches the small response too. This is what pins + // the threshold as the cause of the difference rather than anything about + // the fixture. + let mut stream = mixed_size_tool_calls(); + stream.add_compaction(tool_call_compaction(PolicySpec::new( + ToolCallPolicy::Strip { + request: false, + response: true, + }, + ))); + + stream.apply_projection(); + + assert_eq!( + stream.find_tool_call_response("big").unwrap().content(), + "[compacted] fs_read_file: success" + ); + assert_eq!( + stream.find_tool_call_response("small").unwrap().content(), + "[compacted] fs_read_file: success" + ); +} + +#[test] +fn over_threshold_judges_each_half_on_its_own_size() { + // `big` has a ~16-byte request and a 4096-byte response. With both halves + // enabled and a 1 KB threshold, only the response is large enough to strip. + let mut stream = mixed_size_tool_calls(); + stream.add_compaction(tool_call_compaction(PolicySpec::over( + ToolCallPolicy::Strip { + request: true, + response: true, + }, + ByteSize::from_bytes(ONE_KB), + ))); + + stream.apply_projection(); + + let request = stream + .iter() + .filter_map(|e| e.event.as_tool_call_request().cloned()) + .find(|r| r.id == "big") + .expect("big request present"); + assert_eq!( + request.arguments.get("path").and_then(|v| v.as_str()), + Some("huge.log"), + "a request under the threshold keeps its arguments even when the response is stripped" + ); + assert_eq!( + stream.find_tool_call_response("big").unwrap().content(), + "[compacted] fs_read_file: success" + ); +} + +#[test] +fn over_threshold_on_omit_uses_the_pair_total() { + // Neither half of `split` crosses 1 KB on its own, but together they do, so + // the pair is removed. `small` stays well under the total and survives + // whole. This is what makes `Omit` a pair-level decision rather than a + // per-half one. + let mut stream = ConversationStream::new_test(); + stream.push(ConversationEvent::new(TurnStart, ts(0))); + stream.push(ConversationEvent::new(ChatRequest::from("go"), ts(0))); + + stream.push(ConversationEvent::new( + ToolCallRequest { + id: "split".into(), + name: "fs_modify_file".into(), + arguments: Map::from_iter([("data".into(), "x".repeat(700).into())]), + }, + ts(0), + )); + stream.push(ConversationEvent::new( + ToolCallResponse { + id: "split".into(), + result: Ok("y".repeat(700)), + }, + ts(0), + )); + stream.push(ConversationEvent::new( + ToolCallRequest { + id: "small".into(), + name: "fs_read_file".into(), + arguments: Map::from_iter([("path".into(), "tiny.log".into())]), + }, + ts(0), + )); + stream.push(ConversationEvent::new( + ToolCallResponse { + id: "small".into(), + result: Ok("ok".into()), + }, + ts(0), + )); + + stream.add_compaction(tool_call_compaction(PolicySpec::over( + ToolCallPolicy::Omit, + ByteSize::from_bytes(ONE_KB), + ))); + + stream.apply_projection(); + + let call_ids: Vec = stream + .iter() + .filter_map(|e| match &e.event.kind { + EventKind::ToolCallRequest(r) => Some(r.id.clone()), + EventKind::ToolCallResponse(r) => Some(r.id.clone()), + _ => None, + }) + .collect(); + + assert_eq!( + call_ids, + vec!["small".to_owned(), "small".to_owned()], + "both halves of `split` are removed together and `small` is untouched" + ); +} + +#[test] +fn over_threshold_strips_only_large_reasoning_blocks() { + let mut stream = ConversationStream::new_test(); + stream.push(ConversationEvent::new(TurnStart, ts(0))); + stream.push(ConversationEvent::new(ChatRequest::from("think"), ts(0))); + stream.push(ConversationEvent::new( + ChatResponse::reasoning("z".repeat(4096)), + ts(0), + )); + stream.push(ConversationEvent::new( + ChatResponse::reasoning("brief thought"), + ts(0), + )); + stream.push(ConversationEvent::new(ChatResponse::message("done"), ts(0))); + + stream.add_compaction(Compaction { + timestamp: ts(1), + from_turn: 0, + to_turn: 0, + summary: None, + reasoning: Some(PolicySpec::over( + ReasoningPolicy::Strip, + ByteSize::from_bytes(ONE_KB), + )), + tool_calls: None, + }); + + stream.apply_projection(); + + let reasoning: Vec<&str> = stream + .iter() + .filter_map(|e| match &e.event.kind { + EventKind::ChatResponse(ChatResponse::Reasoning { reasoning }) => { + Some(reasoning.as_str()) + } + _ => None, + }) + .collect(); + + assert_eq!( + reasoning, + vec!["brief thought"], + "only the block over the threshold is stripped" + ); +} + +#[test] +fn threshold_is_measured_against_the_raw_stream() { + // Projection reads raw events, so a second, identical compaction sees the + // same sizes as the first and reaches the same calls. Without this, a + // stripped response (now 33 bytes) would fall under the threshold and the + // second pass would disagree with the first. + let mut stream = mixed_size_tool_calls(); + let policy = PolicySpec::over( + ToolCallPolicy::Strip { + request: false, + response: true, + }, + ByteSize::from_bytes(ONE_KB), + ); + stream.add_compaction(tool_call_compaction(policy.clone())); + stream.add_compaction(Compaction { + timestamp: ts(2), + ..tool_call_compaction(policy) + }); + + stream.apply_projection(); + + assert_eq!( + stream.find_tool_call_response("big").unwrap().content(), + "[compacted] fs_read_file: success" + ); + assert_eq!( + stream.find_tool_call_response("small").unwrap().content(), + "ok" + ); +} + +/// One turn holding two tool calls that share the ID `tc1`, as a provider +/// reusing a synthetic ID across streaming cycles produces. +/// +/// The first response is 4096 bytes and the second is 2, so a 1 KB threshold +/// separates them. +/// Both requests carry the same short arguments. +fn reused_call_id_stream() -> ConversationStream { + let mut stream = ConversationStream::new_test(); + stream.push(ConversationEvent::new(TurnStart, ts(0))); + stream.push(ConversationEvent::new(ChatRequest::from("go"), ts(0))); + + for body in ["x".repeat(4096), "ok".to_owned()] { + stream.push(ConversationEvent::new( + ToolCallRequest { + id: "tc1".into(), + name: "fs_read_file".into(), + arguments: Map::from_iter([("path".into(), "a.log".into())]), + }, + ts(0), + )); + stream.push(ConversationEvent::new( + ToolCallResponse { + id: "tc1".into(), + result: Ok(body), + }, + ts(0), + )); + } + + stream +} + +/// Every tool call response in the stream, in stream order. +fn response_contents(stream: &ConversationStream) -> Vec { + stream + .iter() + .filter_map(|e| { + e.event + .as_tool_call_response() + .map(|r| r.content().to_owned()) + }) + .collect() +} + +#[test] +fn over_threshold_sizes_each_occurrence_of_a_reused_call_id() { + // A provider may reuse one synthetic call ID across cycles, so sizes cannot + // be keyed by ID: that gives every occurrence the last one's size, and the + // oversized response the threshold exists to catch survives untouched. + let mut stream = reused_call_id_stream(); + stream.add_compaction(tool_call_compaction(PolicySpec::over( + ToolCallPolicy::Strip { + request: false, + response: true, + }, + ByteSize::from_bytes(ONE_KB), + ))); + + stream.apply_projection(); + + assert_eq!(response_contents(&stream), vec![ + "[compacted] fs_read_file: success".to_owned(), + "ok".to_owned(), + ]); +} + +#[test] +fn over_threshold_on_omit_pairs_a_reused_call_id_by_occurrence() { + // Each pair is judged on its own combined size: the first totals ~4 KB and + // is removed, the second totals ~18 bytes and survives whole. + let mut stream = reused_call_id_stream(); + stream.add_compaction(tool_call_compaction(PolicySpec::over( + ToolCallPolicy::Omit, + ByteSize::from_bytes(ONE_KB), + ))); + + stream.apply_projection(); + + assert_eq!(response_contents(&stream), vec!["ok".to_owned()]); + assert_eq!( + stream + .iter() + .filter(|e| e.event.as_tool_call_request().is_some()) + .count(), + 1, + "only the small pair's request survives" + ); +} + +#[test] +fn affected_items_sizes_each_occurrence_of_a_reused_call_id() { + let stream = reused_call_id_stream(); + let compaction = tool_call_compaction(PolicySpec::over( + ToolCallPolicy::Strip { + request: false, + response: true, + }, + ByteSize::from_bytes(ONE_KB), + )); + + let items = stream.affected_items(&compaction); + + assert_eq!(items.len(), 1, "only the oversized occurrence"); + assert_eq!(items[0].name, "fs_read_file (response)"); + assert_eq!(items[0].size.as_bytes(), 4096); +} + +#[test] +fn affected_items_reports_nothing_for_a_summary_compaction() { + // A summary replaces every event in its range, so projection ignores the + // mechanical policies. Itemizing them would name items the summary made + // irrelevant. + let stream = mixed_size_tool_calls(); + let compaction = Compaction { + summary: Some(SummaryPolicy { + summary: "the gist".into(), + }), + ..tool_call_compaction(PolicySpec::over( + ToolCallPolicy::Strip { + request: false, + response: true, + }, + ByteSize::from_bytes(ONE_KB), + )) + }; + + assert!(stream.affected_items(&compaction).is_empty()); +} + +#[test] +fn a_thresholded_overlay_replaces_an_earlier_policy_for_its_whole_turn() { + // Stacking stays latest-wins per content type, so a later thresholded + // overlay takes over the turn entirely: an item below its threshold is left + // raw rather than falling back to the earlier, broader policy. Adding a + // targeted rule can therefore un-compact items an earlier rule had reached. + let mut stream = mixed_size_tool_calls(); + + stream.add_compaction(Compaction { + timestamp: ts(1), + ..tool_call_compaction(PolicySpec::new(ToolCallPolicy::Strip { + request: false, + response: true, + })) + }); + stream.add_compaction(Compaction { + timestamp: ts(2), + ..tool_call_compaction(PolicySpec::over( + ToolCallPolicy::Omit, + ByteSize::from_bytes(ONE_KB), + )) + }); + + stream.apply_projection(); + + assert_eq!( + stream.find_tool_call_response("small").unwrap().content(), + "ok", + "the earlier blanket strip does not apply below the later threshold" + ); + assert!( + stream.find_tool_call_response("big").is_none(), + "the oversized pair is omitted by the later overlay" + ); +} + +#[test] +fn affected_items_reports_an_omitted_pair_once() { + // `Omit` removes both halves together, so reporting each half would + // double-count a single decision. + let mut stream = mixed_size_tool_calls(); + let compaction = tool_call_compaction(PolicySpec::over( + ToolCallPolicy::Omit, + ByteSize::from_bytes(ONE_KB), + )); + + let items = stream.affected_items(&compaction); + + assert_eq!(items.len(), 1); + assert_eq!(items[0].turn, 0); + assert_eq!(items[0].name, "fs_read_file"); + // The pair total: the request's serialized arguments + // (`{"path":"huge.log"}`, 19 bytes) plus the 4096-byte response. + assert_eq!(items[0].size.as_bytes(), 4096 + 19); + + // Reporting must not disturb the stream it inspects. + stream.apply_projection(); +} + +#[test] +fn affected_items_reports_each_stripped_half_separately() { + let stream = mixed_size_tool_calls(); + let compaction = tool_call_compaction(PolicySpec::new(ToolCallPolicy::Strip { + request: true, + response: true, + })); + + let items = stream.affected_items(&compaction); + let names: Vec<&str> = items.iter().map(|i| i.name.as_str()).collect(); + + assert_eq!(names, vec![ + "fs_read_file (request)", + "fs_read_file (response)", + "fs_read_file (request)", + "fs_read_file (response)", + ]); +} + +#[test] +fn affected_items_reports_reasoning_blocks() { + let mut stream = ConversationStream::new_test(); + stream.push(ConversationEvent::new(TurnStart, ts(0))); + stream.push(ConversationEvent::new(ChatRequest::from("think"), ts(0))); + stream.push(ConversationEvent::new( + ChatResponse::reasoning("z".repeat(4096)), + ts(0), + )); + stream.push(ConversationEvent::new( + ChatResponse::reasoning("brief"), + ts(0), + )); + + let items = stream.affected_items(&Compaction { + timestamp: ts(1), + from_turn: 0, + to_turn: 0, + summary: None, + reasoning: Some(PolicySpec::over( + ReasoningPolicy::Strip, + ByteSize::from_bytes(ONE_KB), + )), + tool_calls: None, + }); + + assert_eq!(items.len(), 1, "only the block over the threshold"); + assert_eq!(items[0].name, "reasoning"); + assert_eq!(items[0].size.as_bytes(), 4096); +} + +#[test] +fn affected_items_ignores_turns_outside_the_range() { + let stream = mixed_size_tool_calls(); + let out_of_range = Compaction { + from_turn: 1, + to_turn: 5, + ..tool_call_compaction(PolicySpec::new(ToolCallPolicy::Omit)) + }; + + assert!(stream.affected_items(&out_of_range).is_empty()); +} + // --------------------------------------------------------------------------- // Tool call omit // --------------------------------------------------------------------------- @@ -436,7 +990,7 @@ fn omit_tool_calls_removes_them() { to_turn: 1, summary: None, reasoning: None, - tool_calls: Some(ToolCallPolicy::Omit), + tool_calls: Some(ToolCallPolicy::Omit.into()), }); stream.apply_projection(); @@ -503,11 +1057,14 @@ fn summary_ignores_per_type_policies() { summary: Some(SummaryPolicy { summary: "Everything summarized.".into(), }), - reasoning: Some(ReasoningPolicy::Strip), - tool_calls: Some(ToolCallPolicy::Strip { - request: true, - response: true, - }), + reasoning: Some(ReasoningPolicy::Strip.into()), + tool_calls: Some( + ToolCallPolicy::Strip { + request: true, + response: true, + } + .into(), + ), }); stream.apply_projection(); @@ -721,7 +1278,7 @@ fn timestamp_tie_breaks_by_stream_order() { to_turn: 1, summary: None, reasoning: None, - tool_calls: Some(ToolCallPolicy::Omit), + tool_calls: Some(ToolCallPolicy::Omit.into()), }); stream.add_compaction(Compaction { timestamp: ts(2), @@ -729,10 +1286,13 @@ fn timestamp_tie_breaks_by_stream_order() { to_turn: 1, summary: None, reasoning: None, - tool_calls: Some(ToolCallPolicy::Strip { - request: true, - response: true, - }), + tool_calls: Some( + ToolCallPolicy::Strip { + request: true, + response: true, + } + .into(), + ), }); stream.apply_projection(); @@ -758,7 +1318,7 @@ fn later_compaction_wins_for_same_turn() { from_turn: 0, to_turn: 1, summary: None, - reasoning: Some(ReasoningPolicy::Strip), + reasoning: Some(ReasoningPolicy::Strip.into()), tool_calls: None, }); @@ -769,10 +1329,13 @@ fn later_compaction_wins_for_same_turn() { to_turn: 1, summary: None, reasoning: None, - tool_calls: Some(ToolCallPolicy::Strip { - request: true, - response: true, - }), + tool_calls: Some( + ToolCallPolicy::Strip { + request: true, + response: true, + } + .into(), + ), }); stream.apply_projection(); @@ -805,7 +1368,7 @@ fn later_compaction_overrides_earlier_for_same_type() { to_turn: 1, summary: None, reasoning: None, - tool_calls: Some(ToolCallPolicy::Omit), + tool_calls: Some(ToolCallPolicy::Omit.into()), }); // Later: strip tool calls instead (less aggressive). @@ -815,10 +1378,13 @@ fn later_compaction_overrides_earlier_for_same_type() { to_turn: 1, summary: None, reasoning: None, - tool_calls: Some(ToolCallPolicy::Strip { - request: true, - response: true, - }), + tool_calls: Some( + ToolCallPolicy::Strip { + request: true, + response: true, + } + .into(), + ), }); stream.apply_projection(); @@ -843,11 +1409,14 @@ fn summary_wins_over_mechanical_for_same_turns() { from_turn: 0, to_turn: 1, summary: None, - reasoning: Some(ReasoningPolicy::Strip), - tool_calls: Some(ToolCallPolicy::Strip { - request: true, - response: true, - }), + reasoning: Some(ReasoningPolicy::Strip.into()), + tool_calls: Some( + ToolCallPolicy::Strip { + request: true, + response: true, + } + .into(), + ), }); // Later summary compaction for the same range. @@ -882,8 +1451,8 @@ fn compaction_applies_only_to_covered_turns() { from_turn: 0, to_turn: 0, summary: None, - reasoning: Some(ReasoningPolicy::Strip), - tool_calls: Some(ToolCallPolicy::Omit), + reasoning: Some(ReasoningPolicy::Strip.into()), + tool_calls: Some(ToolCallPolicy::Omit.into()), }); stream.apply_projection(); @@ -915,7 +1484,7 @@ fn compaction_beyond_max_turn_is_clamped() { from_turn: 0, to_turn: 99, summary: None, - reasoning: Some(ReasoningPolicy::Strip), + reasoning: Some(ReasoningPolicy::Strip.into()), tool_calls: None, }); @@ -971,7 +1540,7 @@ fn compaction_events_consumed_by_projection() { from_turn: 0, to_turn: 0, summary: None, - reasoning: Some(ReasoningPolicy::Strip), + reasoning: Some(ReasoningPolicy::Strip.into()), tool_calls: None, }); assert_eq!(stream.compactions().count(), 1); @@ -1026,7 +1595,7 @@ fn recompact_projected_stream_with_new_compaction() { from_turn: 0, to_turn: 1, summary: None, - reasoning: Some(ReasoningPolicy::Strip), + reasoning: Some(ReasoningPolicy::Strip.into()), tool_calls: None, }); @@ -1059,7 +1628,7 @@ fn recompact_projected_stream_with_new_compaction() { to_turn: 0, summary: None, reasoning: None, - tool_calls: Some(ToolCallPolicy::Omit), + tool_calls: Some(ToolCallPolicy::Omit.into()), }); stream.apply_projection(); @@ -1216,8 +1785,8 @@ fn spec_to_compaction(spec: &CompactionSpec) -> Compaction { from_turn: spec.from, to_turn: spec.to, summary, - reasoning, - tool_calls, + reasoning: reasoning.map(Into::into), + tool_calls: tool_calls.map(Into::into), } } @@ -1410,8 +1979,8 @@ proptest! { from_turn: from, to_turn: to, summary: None, - reasoning, - tool_calls, + reasoning: reasoning.map(Into::into), + tool_calls: tool_calls.map(Into::into), }); } @@ -1551,7 +2120,7 @@ fn compaction_targets_correct_turn_with_implicit_leading_turn() { from_turn: 1, to_turn: 1, summary: None, - reasoning: Some(ReasoningPolicy::Strip), + reasoning: Some(ReasoningPolicy::Strip.into()), tool_calls: None, }); diff --git a/crates/jp_conversation/src/stream_tests.rs b/crates/jp_conversation/src/stream_tests.rs index eb2699d6e..bee9b9c51 100644 --- a/crates/jp_conversation/src/stream_tests.rs +++ b/crates/jp_conversation/src/stream_tests.rs @@ -1008,11 +1008,14 @@ fn make_compaction(from: usize, to: usize) -> Compaction { from_turn: from, to_turn: to, summary: None, - reasoning: Some(ReasoningPolicy::Strip), - tool_calls: Some(ToolCallPolicy::Strip { - request: true, - response: true, - }), + reasoning: Some(ReasoningPolicy::Strip.into()), + tool_calls: Some( + ToolCallPolicy::Strip { + request: true, + response: true, + } + .into(), + ), } } @@ -1314,7 +1317,7 @@ fn test_compaction_roundtrip_via_to_parts_from_parts() { let c = restored.compactions().next().unwrap(); assert_eq!(c.from_turn, 0); assert_eq!(c.to_turn, 0); - assert_eq!(c.reasoning, Some(ReasoningPolicy::Strip)); + assert_eq!(c.reasoning, Some(ReasoningPolicy::Strip.into())); } #[test] diff --git a/docs/ticket/04hn6c8-toolcallsmode-schema-omits-its-accepted-values.md b/docs/ticket/04hn6c8-toolcallsmode-schema-omits-its-accepted-values.md new file mode 100644 index 000000000..69a806b29 --- /dev/null +++ b/docs/ticket/04hn6c8-toolcallsmode-schema-omits-its-accepted-values.md @@ -0,0 +1,94 @@ +# `ToolCallsMode` schema omits its accepted values + +- **Status**: Todo +- **Kind**: Chore +- **Authors**: jp +- **Date**: 2026-08-18 + +`ToolCallsMode` hand-writes its `Schematic` impl and returns a bare string +(`crates/jp_config/src/conversation/compaction.rs:590-593`): + +```rust +impl schematic::Schematic for ToolCallsMode { + fn build_schema(mut schema: schematic::SchemaBuilder) -> schematic::Schema { + schema.string_default() + } +} +``` + +So the four values it actually accepts never reach the schema. A consumer +validating a workspace config against it accepts `tool_calls = "nonsense"`, and +an editor driven by the schema offers no completion for a key whose whole +vocabulary is four fixed strings. + +It is the only `string_default()` in `jp_config`. Its sibling on the same rule, +`ReasoningMode`, derives `ConfigEnum`, which generates +`schema.enumerable(EnumType::from_fields(...))` +(`crates/contrib/schematic_macros/src/config_enum/mod.rs:270-280`) and does +carry its variants. The two keys sit next to each other in +`CompactionRuleConfig` and describe their values to a schema consumer +differently. + +## Why it is hand-written + +Not an oversight to delete — `FromStr` accepts two or three spellings per +variant: + +| Variant | Accepted | +| ---------------- | -------------------------------------------- | +| `Strip` | `strip`, `s` | +| `StripResponses` | `strip-responses`, `strip_responses`, `sres` | +| `StripRequests` | `strip-requests`, `strip_requests`, `sreq` | +| `Omit` | `omit`, `o` | + +`ConfigEnum` supports a single `alias` per variant +(`crates/contrib/schematic_macros/src/config_enum/variant.rs:109-112`), so +deriving it is not a drop-in for the two variants that need three spellings. +The hand-written impl exists to keep the aliases; only the *schema* half was +left as a placeholder. + +## The decision the fix forces + +Which spellings the schema lists is a real choice, not a detail: + +- **Canonical only** (`strip`, `strip-requests`, `strip-responses`, `omit`) + gives clean completion, but a schema-validating editor then flags + `tool_calls = "sres"` as invalid even though `jp` accepts it. The tool and its + schema would disagree about valid input. +- **Every accepted spelling** (ten values) keeps them in agreement but makes + completion noisy, and pushes the aliases into a published contract that is + currently only an input convenience. + +Worth settling before writing the `EnumType`, since it determines whether the +schema describes what `jp` accepts or what `jp` recommends. + +Two implementation routes: + +1. Hand-write an `EnumType` schema alongside the existing hand-written + `FromStr` / `Serialize` / `Display`. Smallest change, keeps the divergence + between how the two enums on this rule are described. +2. Extend `ConfigEnum` to take multiple aliases per variant and derive the whole + thing. Larger, touches the vendored macro crate, but removes the one-off and + would apply to any future enum with more than one alias. + +## Severity + +No internal consumer is affected. `AppConfig::schema()`'s only in-tree use is +`jp_conversation::compat::strip_unknown_fields`, which reads struct field +*names* to drop keys that no longer exist and returns early on any non-`Struct` +node (`crates/jp_conversation/src/compat.rs:64-67`), so a value's type is never +consulted. Config loading rejects a bad mode string on its own via `FromStr`, +independent of the schema. + +The gap is in what JP tells external tooling. There is no generated `AppConfig` +schema checked into the repository today, so nothing is currently shipping the +wrong thing — the cost is paid the first time something consumes it. + +## Why it is filed rather than fixed in place + +Raised in review of PR \#994, which added an `over` size threshold to the +compaction policies and wrapped both mode enums in `PolicySpec

`. That PR +fixed `PolicySpec`'s own schema, which had collapsed to "any JSON value" and +erased whatever `P` contributed. Fixing `P` itself is a separate change: it +touches a type \#994 otherwise leaves alone, and it needs the +canonical-versus-aliases decision above, which \#994 has no reason to make.