From 0f7ad6ded5c4e8fedb6d5b61b9784ef752e64414 Mon Sep 17 00:00:00 2001 From: Jean Mertz Date: Tue, 18 Aug 2026 14:47:20 +0200 Subject: [PATCH 1/4] refactor(config): Add `ByteSize` and `PolicySpec` types Two building blocks for narrowing a compaction policy to the large items in its range, neither of which has a consumer yet. `ByteSize` is a size in bytes written as a human-readable string (`1MB`, `512 KB`, `4GiB`) or a bare byte count. Unit suffixes are binary, so `1KB` is 1024 bytes. Its `Display` picks the largest unit that divides the value evenly and falls back to the raw count otherwise, which makes the rendered form always parse back to the same value and therefore safe to use as the serialized form. A separate `human()` gives the lossy one-decimal rendering for terminal output. `PolicySpec

` pairs a policy with the options qualifying which items it reaches, today just an `over` size threshold. Declaring it once as a generic wrapper keeps the option off each individual policy enum, so a future policy cannot forget to honor it. The serialized shape is the part worth attention: a spec without options serializes as the bare policy, so adding this changes no existing config file or event stream on disk. Only a spec that actually carries a threshold takes the table form, where a map-shaped policy gains `over` as a sibling key and a string-shaped one is promoted to `{"policy": "...", "over": "..."}`. Deserialization tries the whole map against `P` before falling back to the promoted string. The order is load-bearing: a tagged unit variant such as `{"policy": "omit"}` is indistinguishable from a promotion by shape alone, and only the map reading is correct for it. `FromStr` accepts `POLICY,option=value` so a `--cfg` assignment and the inline compaction DSL can share one option syntax. Signed-off-by: Jean Mertz --- crates/jp_config/src/types.rs | 2 + crates/jp_config/src/types/byte_size.rs | 187 +++++++++++++++ crates/jp_config/src/types/byte_size_tests.rs | 132 +++++++++++ crates/jp_config/src/types/policy_spec.rs | 201 ++++++++++++++++ .../jp_config/src/types/policy_spec_tests.rs | 223 ++++++++++++++++++ 5 files changed, 745 insertions(+) create mode 100644 crates/jp_config/src/types/byte_size.rs create mode 100644 crates/jp_config/src/types/byte_size_tests.rs create mode 100644 crates/jp_config/src/types/policy_spec.rs create mode 100644 crates/jp_config/src/types/policy_spec_tests.rs 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..b17107fdf --- /dev/null +++ b/crates/jp_config/src/types/policy_spec.rs @@ -0,0 +1,201 @@ +//! 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, json_value::JsonValue}; +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 promoted = value + .as_object() + .and_then(|map| map.get("policy")) + .cloned() + .ok_or_else(|| D::Error::custom(&error))?; + + 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 plus options. The table + // shape depends on `P`, so it is described only as an object. + schema.union(schematic::schema::UnionType { + variants_types: vec![ + Box::new(schema.infer::

()), + Box::new(schema.infer::()), + ], + ..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)); +} From 22f01b82fd5cb44f33e2ef66201fabadef98bd5e Mon Sep 17 00:00:00 2001 From: Jean Mertz Date: Tue, 18 Aug 2026 16:07:52 +0200 Subject: [PATCH 2/4] feat(config, conversation, cli): Compact only oversized items A compaction rule reaches every item in its turn range. That is the right default for old turns, but it is the wrong tool for the case that actually breaks a conversation: one `fs_read_file` that returned a 10 MB log and now dominates the context window. Cutting it today means either compacting a whole range of turns you wanted to keep, or editing `events.json` by hand. The reasoning and tool-call policies now accept an `over` size threshold, so a rule reaches only the items large enough to matter: jp conversation compact --tools=sres --over 1mb jp query -k 'r,over=16kb+t=sres,over=1mb:..-3' [[conversation.compaction.rules]] tool_calls = { policy = "strip-responses", over = "1MB" } Both surfaces compose with the existing range flags, so a threshold narrows what a rule touches rather than replacing how it is scoped. This also enables a tiered rule set: drop anything genuinely huge wherever it sits, and strip everything older than the last few turns regardless of size. The threshold is stored in the compaction event and evaluated during projection rather than resolved into a fixed set of matched items at creation. Size comparison is cheap and deterministic, so it is a lazy policy in RFD 064's terms, and projection stays a pure function of the stored stream. Sizes are judged per half for `Strip`, so a call with a short request and a huge response loses only the response. `Omit` removes whole pairs, so it is judged on the two halves combined and can never leave an orphaned request behind. Measurement uses decoded content (tool arguments are base64 at rest) against the raw stream, so re-running a rule reaches the same items. `summary` takes no threshold: it replaces its whole range rather than selecting from it. Because a threshold reaches an unpredictable subset of its range, the timeline names what it caught instead of leaving the range line to imply everything was compacted: Would have compacted turns 2..14 (13 total, tool responses over 1MB). turn 4 fs_read_file (response) 10.3 MB turn 9 cargo_test (response) 2.1 MB A rule without a threshold does not itemize, and a threshold that matched nothing says so. Nothing changes for a rule that sets no threshold, on disk or in behavior: a policy without one serializes as the bare string it always was, and the built-in defaults carry no threshold. BREAKING CHANGE: `reasoning` in `jp conversation show -F json` The `reasoning` field of a compaction entry was a boolean; it is now the serialized policy (`null`, `"strip"`, or `{"policy": "strip", "over": "16KB"}`), matching how `tool_calls` already reported itself. Without this a consumer could read `tool_calls.over` but had no way to see a reasoning threshold. Scripts testing truthiness need updating: `.reasoning == true` becomes `.reasoning != null`. Signed-off-by: Jean Mertz --- crates/jp_cli/src/cmd/compact_flag.rs | 107 +++- crates/jp_cli/src/cmd/compact_flag_tests.rs | 154 +++++- crates/jp_cli/src/cmd/conversation/compact.rs | 148 ++++- .../src/cmd/conversation/compact_tests.rs | 213 ++++++- crates/jp_cli/src/cmd/query_tests.rs | 2 +- crates/jp_cli/src/format.rs | 38 +- crates/jp_cli/src/format_tests.rs | 47 +- .../jp_config/src/conversation/compaction.rs | 35 +- .../src/conversation/compaction_tests.rs | 99 +++- crates/jp_config/src/lib_tests.rs | 6 +- ...ts__partial_app_config_default_values.snap | 10 +- crates/jp_conversation/src/compaction.rs | 25 +- .../jp_conversation/src/compaction_tests.rs | 21 +- crates/jp_conversation/src/lib.rs | 4 +- crates/jp_conversation/src/stream.rs | 16 +- .../jp_conversation/src/stream/projection.rs | 312 +++++++++-- .../src/stream/projection_tests.rs | 518 ++++++++++++++++-- crates/jp_conversation/src/stream_tests.rs | 15 +- 18 files changed, 1527 insertions(+), 243 deletions(-) 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..6ac1e6595 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,34 @@ 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. +fn threshold_items( + events: &ConversationStream, + compaction: &Compaction, +) -> Option> { + 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 +544,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 +566,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 +654,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 +746,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 +859,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 +942,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..7ad81af81 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,105 @@ 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_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 +990,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 +1019,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..8d2b895bd 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() }] } @@ -191,10 +194,32 @@ pub struct CompactionRuleConfig { pub keep_last: RuleBound, /// Policy for reasoning (thinking) blocks. - pub reasoning: Option, + /// + /// Accepts `"strip"`, or a table adding a size threshold: + /// + /// ```toml + /// reasoning = { policy = "strip", over = "16KB" } + /// ``` + /// + /// With `over` set, only reasoning 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, + /// + /// Accepts a mode string, or a table adding 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..b52d92e03 100644 --- a/crates/jp_config/src/conversation/compaction_tests.rs +++ b/crates/jp_config/src/conversation/compaction_tests.rs @@ -1,4 +1,97 @@ 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 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 +212,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 +226,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_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..21974886a 100644 --- a/crates/jp_conversation/src/stream/projection.rs +++ b/crates/jp_conversation/src/stream/projection.rs @@ -8,11 +8,11 @@ use std::collections::{HashMap, HashSet}; 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); // 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) 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,158 @@ 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 +/// its whole range rather than picking from it, so it contributes nothing here. +pub(super) fn affected_items( + events: &[InternalEvent], + compaction: &Compaction, +) -> Vec { + let turn_indices = assign_turn_indices(events); + let tool_calls = build_tool_calls(events); + 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 sizes = tool_call_sizes(event, &tool_calls); + + match &spec.policy { + // Report the pair once, from its request, since both halves go + // together. + ToolCallPolicy::Omit => { + if let Some(request) = event.as_tool_call_request() + && spec.covers(sizes.pair) + { + items.push(AffectedItem { + turn, + name: tool_name(&tool_calls, &request.id).to_owned(), + size: ByteSize::from_bytes(sizes.pair), + }); + } + } + ToolCallPolicy::Strip { request, response } => { + if *request + && let Some(req) = event.as_tool_call_request() + && spec.covers(sizes.own) + { + items.push(AffectedItem { + turn, + name: format!("{} (request)", tool_name(&tool_calls, &req.id)), + size: ByteSize::from_bytes(sizes.own), + }); + } + if *response + && let Some(resp) = event.as_tool_call_response() + && spec.covers(sizes.own) + { + items.push(AffectedItem { + turn, + name: format!("{} (response)", tool_name(&tool_calls, &resp.id)), + size: ByteSize::from_bytes(sizes.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, + tool_calls: &HashMap, +) -> 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() { + // Sizes are read up front so the lookup's borrow is released before the + // strip helpers mutate the event. + let sizes = tool_call_sizes(&event, tool_calls); + + 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(sizes.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(sizes.own) { + strip_tool_request(&mut event); + } + if *response && event.is_tool_call_response() && spec.covers(sizes.own) { + strip_tool_response(&mut event, tool_calls); + } + } + } + } + + Some(event) +} + /// Group projected events into turns (matching [`IterTurns`]) and return each /// turn's [`TurnOrigin`], read from the event that opens the turn. /// @@ -408,10 +534,18 @@ fn strip_tool_request(event: &mut ConversationEvent) { } } +/// The tool name recorded for a call ID, or `unknown` when no request for it +/// survives in the stream. +fn tool_name<'a>(tool_calls: &'a HashMap, id: &str) -> &'a str { + tool_calls + .get(id) + .map_or("unknown", |call| call.name.as_str()) +} + /// Replace a tool call response's content with a compact status line. -fn strip_tool_response(event: &mut ConversationEvent, tool_names: &HashMap) { +fn strip_tool_response(event: &mut ConversationEvent, tool_calls: &HashMap) { if let Some(resp) = event.as_tool_call_response_mut() { - let name = tool_names.get(&resp.id).map_or("unknown", String::as_str); + let name = tool_name(tool_calls, &resp.id); let status = if resp.result.is_ok() { "success" } else { @@ -426,17 +560,105 @@ fn strip_tool_response(event: &mut ConversationEvent, tool_names: &HashMap HashMap { - let mut map = HashMap::new(); +/// What the tool call policies need to know about a single call. +/// +/// The name feeds a stripped response's status line; the two sizes feed a +/// spec's `over` threshold. +#[derive(Default)] +struct ToolCall { + /// Name of the tool, taken from the request. + name: String, + /// Byte size of the request arguments. + request_size: u64, + /// Byte size of the response content. + response_size: u64, +} + +impl ToolCall { + /// Combined size of both halves. + const fn pair_size(&self) -> u64 { + self.request_size.saturating_add(self.response_size) + } +} + +/// Size figures for whichever half of a tool call pair an event holds. +#[derive(Clone, Copy, Default)] +struct ToolSizes { + /// Size of this event's own half. + own: u64, + /// Combined size of both halves. + pair: u64, +} + +/// Build a map from tool call ID to the name and sizes of that call. +fn build_tool_calls(events: &[InternalEvent]) -> HashMap { + let mut calls: HashMap = 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()); + let Some(conv_event) = event.as_event() else { + continue; + }; + + if let Some(req) = conv_event.as_tool_call_request() { + let call = calls.entry(req.id.clone()).or_default(); + call.name.clone_from(&req.name); + call.request_size = arguments_size(&req.arguments); + } else if let Some(resp) = conv_event.as_tool_call_response() { + calls.entry(resp.id.clone()).or_default().response_size = + byte_count(resp.content().len()); } } - map + calls +} + +/// Look up the sizes for whichever half of a tool call pair `event` holds. +/// +/// Both halves report the same `pair` total, so a threshold on `Omit` either +/// removes a pair or leaves it whole. +/// A non-tool event reports zero, which no threshold covers. +fn tool_call_sizes(event: &ConversationEvent, calls: &HashMap) -> ToolSizes { + let (id, own_is_request) = if let Some(req) = event.as_tool_call_request() { + (&req.id, true) + } else if let Some(resp) = event.as_tool_call_response() { + (&resp.id, false) + } else { + return ToolSizes::default(); + }; + + let Some(call) = calls.get(id) else { + return ToolSizes::default(); + }; + + ToolSizes { + own: if own_is_request { + call.request_size + } else { + call.response_size + }, + pair: call.pair_size(), + } +} + +/// 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..685bda09d 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,383 @@ 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" + ); +} + +#[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 +825,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 +892,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 +1113,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 +1121,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 +1153,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 +1164,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 +1203,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 +1213,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 +1244,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 +1286,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 +1319,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 +1375,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 +1430,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 +1463,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 +1620,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 +1814,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 +1955,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] From d0deed5d6e9e89821f681f2d0872b84a153173a6 Mon Sep 17 00:00:00 2001 From: Jean Mertz Date: Tue, 18 Aug 2026 16:50:13 +0200 Subject: [PATCH 3/4] fix(conversation, config, cli): Size each tool call occurrence Review follow-ups on the compaction size threshold, the first two of which are real defects. A tool call ID is not unique. `TurnMut::build` documents that providers like Google reuse one synthetic ID across streaming cycles within a turn, and validates responses by count for exactly that reason. Keying the size map by ID gave every occurrence of an ID the last one's sizes, so `--tools=sres --over 1mb` on a turn holding a 4 MB `tc1` response followed by a 2-byte `tc1` response kept the 4 MB one and reported no match. The oversized payload the threshold exists to remove then reached every later query, silently. Sizes are now keyed by stream position, and a response pairs with the oldest request in its turn carrying the same ID that has no response yet, mirroring the rule `TurnMut::build` already applies. That also fixes the tool name on a stripped response for a reused ID, which had the same weakness. A misspelled option key was silently dropped. `tool_calls = { policy = "strip-responses", oer = "1MB" }` deserialized to an unthresholded rule and stripped every response in range, the opposite of what the line asks for. Schematic emits `deny_unknown_fields` for the surrounding config types, so `PolicySpec` was the one place in this tree accepting junk. The promoted-string form now rejects any leftover key, naming it. A policy that legitimately serializes as a map with fields of its own is untouched, since it consumes the map itself. A summary rule no longer itemizes. `-k 's+r,over=1kb'` parses, and the threshold rides along on the rule, but a summary replaces its whole range and projection never consults the threshold. The timeline listed items nothing had selected, under a label that read `summary`. The generated JSON Schema describes the table form field by field rather than as an unconstrained value, so a schema consumer still validates `policy` and still rejects an unknown key. The `tool_calls` and `reasoning` doc comments list the modes they accept and say what omitting them does, which is what a reader scanning a generated `config.toml` needs. A test pins the stacking interaction the threshold makes visible: a later thresholded overlay replaces an earlier policy for its whole turn, so an item below the threshold is left raw rather than falling back to the earlier, broader policy. That follows from latest-wins resolution and is unchanged by this feature, but it was untested at item granularity. Signed-off-by: Jean Mertz --- crates/jp_cli/src/cmd/conversation/compact.rs | 7 + .../src/cmd/conversation/compact_tests.rs | 29 +++ .../jp_config/src/conversation/compaction.rs | 22 +- .../src/conversation/compaction_tests.rs | 41 ++++ crates/jp_config/src/types/policy_spec.rs | 41 +++- .../jp_conversation/src/stream/projection.rs | 208 +++++++++--------- .../src/stream/projection_tests.rs | 165 ++++++++++++++ 7 files changed, 393 insertions(+), 120 deletions(-) diff --git a/crates/jp_cli/src/cmd/conversation/compact.rs b/crates/jp_cli/src/cmd/conversation/compact.rs index 6ac1e6595..a5ae43bc6 100644 --- a/crates/jp_cli/src/cmd/conversation/compact.rs +++ b/crates/jp_cli/src/cmd/conversation/compact.rs @@ -523,10 +523,17 @@ struct TimelineSegment { /// /// 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() diff --git a/crates/jp_cli/src/cmd/conversation/compact_tests.rs b/crates/jp_cli/src/cmd/conversation/compact_tests.rs index 7ad81af81..97d73e133 100644 --- a/crates/jp_cli/src/cmd/conversation/compact_tests.rs +++ b/crates/jp_cli/src/cmd/conversation/compact_tests.rs @@ -955,6 +955,35 @@ fn timeline_says_so_when_a_threshold_caught_nothing() { ]); } +#[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 diff --git a/crates/jp_config/src/conversation/compaction.rs b/crates/jp_config/src/conversation/compaction.rs index 8d2b895bd..04df6d65c 100644 --- a/crates/jp_config/src/conversation/compaction.rs +++ b/crates/jp_config/src/conversation/compaction.rs @@ -193,22 +193,34 @@ pub struct CompactionRuleConfig { #[setting(default = default_keep_last)] pub keep_last: RuleBound, - /// Policy for reasoning (thinking) blocks. + /// What to do with reasoning (thinking) blocks in the compacted range. /// - /// Accepts `"strip"`, or a table adding a size threshold: + /// 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 reasoning blocks larger than that are stripped. + /// 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. + /// 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. /// - /// Accepts a mode string, or a table adding a size threshold: + /// A table form adds a size threshold: /// /// ```toml /// tool_calls = { policy = "strip-responses", over = "1MB" } diff --git a/crates/jp_config/src/conversation/compaction_tests.rs b/crates/jp_config/src/conversation/compaction_tests.rs index b52d92e03..4d07c193a 100644 --- a/crates/jp_config/src/conversation/compaction_tests.rs +++ b/crates/jp_config/src/conversation/compaction_tests.rs @@ -67,6 +67,47 @@ fn a_policy_without_a_threshold_serializes_back_as_a_bare_string() { 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 diff --git a/crates/jp_config/src/types/policy_spec.rs b/crates/jp_config/src/types/policy_spec.rs index b17107fdf..088b94834 100644 --- a/crates/jp_config/src/types/policy_spec.rs +++ b/crates/jp_config/src/types/policy_spec.rs @@ -21,7 +21,7 @@ use schematic::{Schema, SchemaBuilder, Schematic}; use serde::{Deserialize, Serialize, de::DeserializeOwned}; use serde_json::{Map, Value}; -use super::{byte_size::ByteSize, json_value::JsonValue}; +use super::byte_size::ByteSize; use crate::BoxedError; /// A compaction policy plus the options qualifying which items it applies to. @@ -119,12 +119,26 @@ impl<'de, P: DeserializeOwned> Deserialize<'de> for PolicySpec

{ let policy = match serde_json::from_value::

(value.clone()) { Ok(policy) => policy, Err(error) => { - let promoted = value - .as_object() - .and_then(|map| map.get("policy")) + 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))? } }; @@ -184,13 +198,20 @@ impl fmt::Display for PolicySpec

{ impl Schematic for PolicySpec

{ fn build_schema(mut schema: SchemaBuilder) -> Schema { - // Either the bare policy or a table carrying it plus options. The table - // shape depends on `P`, so it is described only as an object. + // 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(schema.infer::()), - ], + variants_types: vec![Box::new(schema.infer::

()), Box::new(table)], ..Default::default() }) } diff --git a/crates/jp_conversation/src/stream/projection.rs b/crates/jp_conversation/src/stream/projection.rs index 21974886a..05cfa128e 100644 --- a/crates/jp_conversation/src/stream/projection.rs +++ b/crates/jp_conversation/src/stream/projection.rs @@ -5,7 +5,7 @@ //! //! 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, Value}; @@ -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_calls = build_tool_calls(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,7 +197,7 @@ pub(super) fn apply(events: &mut Vec) -> Vec { continue; } - let Some(event) = apply_mechanical(*conv_event, policy, &tool_calls) else { + let Some(event) = apply_mechanical(*conv_event, policy, tool_calls.get(&i)) else { continue; }; @@ -228,14 +228,20 @@ pub struct AffectedItem { /// List the items `compaction`'s mechanical policies reach, in stream order. /// -/// Only the reasoning and tool-call policies select items; a summary replaces -/// its whole range rather than picking from it, so it contributes nothing here. +/// 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); + let tool_calls = build_tool_calls(events, &turn_indices); let mut items = Vec::new(); for (index, entry) in events.iter().enumerate() { @@ -266,41 +272,35 @@ pub(super) fn affected_items( let Some(spec) = &compaction.tool_calls else { continue; }; - let sizes = tool_call_sizes(event, &tool_calls); + 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 let Some(request) = event.as_tool_call_request() - && spec.covers(sizes.pair) - { + if event.is_tool_call_request() && spec.covers(info.pair) { items.push(AffectedItem { turn, - name: tool_name(&tool_calls, &request.id).to_owned(), - size: ByteSize::from_bytes(sizes.pair), + name: info.name.clone(), + size: ByteSize::from_bytes(info.pair), }); } } ToolCallPolicy::Strip { request, response } => { - if *request - && let Some(req) = event.as_tool_call_request() - && spec.covers(sizes.own) - { + if *request && event.is_tool_call_request() && spec.covers(info.own) { items.push(AffectedItem { turn, - name: format!("{} (request)", tool_name(&tool_calls, &req.id)), - size: ByteSize::from_bytes(sizes.own), + name: format!("{} (request)", info.name), + size: ByteSize::from_bytes(info.own), }); } - if *response - && let Some(resp) = event.as_tool_call_response() - && spec.covers(sizes.own) - { + if *response && event.is_tool_call_response() && spec.covers(info.own) { items.push(AffectedItem { turn, - name: format!("{} (response)", tool_name(&tool_calls, &resp.id)), - size: ByteSize::from_bytes(sizes.own), + name: format!("{} (response)", info.name), + size: ByteSize::from_bytes(info.own), }); } } @@ -318,7 +318,7 @@ pub(super) fn affected_items( fn apply_mechanical( mut event: ConversationEvent, policy: &TurnPolicy, - tool_calls: &HashMap, + info: Option<&ToolCallInfo>, ) -> Option { if let Some(spec) = &policy.reasoning && matches!(spec.policy, ReasoningPolicy::Strip) @@ -332,9 +332,10 @@ fn apply_mechanical( // 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() { - // Sizes are read up front so the lookup's borrow is released before the - // strip helpers mutate the event. - let sizes = tool_call_sizes(&event, tool_calls); + // 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 => { @@ -342,7 +343,7 @@ fn apply_mechanical( // 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(sizes.pair) + && spec.covers(pair) { return None; } @@ -350,11 +351,11 @@ fn apply_mechanical( 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(sizes.own) { + if *request && event.is_tool_call_request() && spec.covers(own) { strip_tool_request(&mut event); } - if *response && event.is_tool_call_response() && spec.covers(sizes.own) { - strip_tool_response(&mut event, tool_calls); + if *response && event.is_tool_call_response() && spec.covers(own) { + strip_tool_response(&mut event, info.map_or("unknown", |i| i.name.as_str())); } } } @@ -534,18 +535,12 @@ fn strip_tool_request(event: &mut ConversationEvent) { } } -/// The tool name recorded for a call ID, or `unknown` when no request for it -/// survives in the stream. -fn tool_name<'a>(tool_calls: &'a HashMap, id: &str) -> &'a str { - tool_calls - .get(id) - .map_or("unknown", |call| call.name.as_str()) -} - /// Replace a tool call response's content with a compact status line. -fn strip_tool_response(event: &mut ConversationEvent, tool_calls: &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_name(tool_calls, &resp.id); let status = if resp.result.is_ok() { "success" } else { @@ -560,82 +555,85 @@ fn strip_tool_response(event: &mut ConversationEvent, tool_calls: &HashMap u64 { - self.request_size.saturating_add(self.response_size) - } -} - -/// Size figures for whichever half of a tool call pair an event holds. -#[derive(Clone, Copy, Default)] -struct ToolSizes { - /// Size of this event's own half. + /// Byte size of this event's own half. own: u64, - /// Combined size of both halves. + /// Combined byte size of both halves of the pair. + /// + /// Equal to `own` for a half whose partner is missing from the stream. pair: u64, } -/// Build a map from tool call ID to the name and sizes of that call. -fn build_tool_calls(events: &[InternalEvent]) -> HashMap { - let mut calls: HashMap = HashMap::new(); - for event in events { - let Some(conv_event) = event.as_event() else { +/// 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) = conv_event.as_tool_call_request() { - let call = calls.entry(req.id.clone()).or_default(); - call.name.clone_from(&req.name); - call.request_size = arguments_size(&req.arguments); - } else if let Some(resp) = conv_event.as_tool_call_response() { - calls.entry(resp.id.clone()).or_default().response_size = - byte_count(resp.content().len()); + 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 }); } } - calls -} -/// Look up the sizes for whichever half of a tool call pair `event` holds. -/// -/// Both halves report the same `pair` total, so a threshold on `Omit` either -/// removes a pair or leaves it whole. -/// A non-tool event reports zero, which no threshold covers. -fn tool_call_sizes(event: &ConversationEvent, calls: &HashMap) -> ToolSizes { - let (id, own_is_request) = if let Some(req) = event.as_tool_call_request() { - (&req.id, true) - } else if let Some(resp) = event.as_tool_call_response() { - (&resp.id, false) - } else { - return ToolSizes::default(); - }; - - let Some(call) = calls.get(id) else { - return ToolSizes::default(); - }; - - ToolSizes { - own: if own_is_request { - call.request_size - } else { - call.response_size - }, - pair: call.pair_size(), - } + calls } /// Byte size of a tool call request's arguments as the provider receives them. diff --git a/crates/jp_conversation/src/stream/projection_tests.rs b/crates/jp_conversation/src/stream/projection_tests.rs index 685bda09d..f2fb1fb16 100644 --- a/crates/jp_conversation/src/stream/projection_tests.rs +++ b/crates/jp_conversation/src/stream/projection_tests.rs @@ -727,6 +727,171 @@ fn threshold_is_measured_against_the_raw_stream() { ); } +/// 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 From f1789b9cbb86c22b5c3074cc4ee8c9dcf0cb6ead Mon Sep 17 00:00:00 2001 From: Jean Mertz Date: Tue, 18 Aug 2026 17:20:31 +0200 Subject: [PATCH 4/4] file ticket 04hn6c8 Signed-off-by: Jean Mertz --- ...lsmode-schema-omits-its-accepted-values.md | 94 +++++++++++++++++++ 1 file changed, 94 insertions(+) create mode 100644 docs/ticket/04hn6c8-toolcallsmode-schema-omits-its-accepted-values.md 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.