From 6bf5f153542dfb881e928de068ac83e302a8caaa Mon Sep 17 00:00:00 2001 From: Jean Mertz Date: Mon, 3 Aug 2026 07:45:38 +0200 Subject: [PATCH 1/5] feat(cli, conversation): Support verbatim summaries in compaction `jp conversation compact --summary TEXT` now stores TEXT verbatim as the summary and calls no model, instead of passing it as extra context to the summarizer. Bare `--summary` still generates one. Guidance for the generator moves to a new `--summary-context TEXT` flag, which applies to whichever rules are active in the invocation (configured or ad-hoc) rather than defining a rule of its own, e.g.: jp conversation compact --summary-context "focus on the architecture" jp conversation compact --summary "we settled on the layered loader" The `--summarize` flag and DSL policy are renamed to `--summary` / `s`/`summary` for consistency with the new verbatim behavior; the `summarize` DSL spelling still parses so existing specs keep working. Every stored summary now records whether its text was generated by a model or authored by the user (`SummaryPolicy::source`). Generated summaries can be re-derived over a wider range by asking the model again; authored text cannot. When compaction would otherwise have to grow a summary's range across authored text, either widening it or overwriting it, JP now refuses and reports the exact range that would resolve the overlap, e.g.: Summary overlap A summary cannot be nested inside or split across another one, so your text for turns 1..4 would have to stand in for turns 1..6 as well. Re-run with `--from 1 --to 6` to cover the whole range, or `jp conversation compact --reset` to drop the existing compactions first. Compactions written before this change have no `source` field and load as `Generated`, so existing conversations are unaffected. The timeline preview (`--dry-run`) and the real run now share the same overlap check, so a preview never promises a compaction the run would refuse. Signed-off-by: Jean Mertz --- crates/jp_cli/src/cmd.rs | 34 +++ crates/jp_cli/src/cmd/compact_flag.rs | 20 +- crates/jp_cli/src/cmd/compact_flag_tests.rs | 37 ++- crates/jp_cli/src/cmd/conversation/compact.rs | 230 ++++++++++++----- .../src/cmd/conversation/compact_tests.rs | 244 +++++++++++++++++- crates/jp_cli/src/error.rs | 19 ++ crates/jp_cli/src/format_tests.rs | 4 +- .../jp_config/src/conversation/compaction.rs | 12 + crates/jp_conversation/src/compaction.rs | 146 +++++++++-- .../jp_conversation/src/compaction_tests.rs | 191 ++++++++++++-- crates/jp_conversation/src/lib.rs | 4 +- .../src/stream/projection_tests.rs | 66 ++--- .../src/provider/compaction_request_tests.rs | 21 +- docs/.vitepress/rfd-summaries.json | 2 +- docs/architecture/ubiquitous-language.md | 66 +++++ ...non-destructive-conversation-compaction.md | 11 +- 16 files changed, 907 insertions(+), 200 deletions(-) diff --git a/crates/jp_cli/src/cmd.rs b/crates/jp_cli/src/cmd.rs index d7473abdf..e7dcbeb4c 100644 --- a/crates/jp_cli/src/cmd.rs +++ b/crates/jp_cli/src/cmd.rs @@ -501,6 +501,40 @@ impl From for Error { )] .into(), Compaction(error) => [("message", "Compaction error".into()), ("error", error)].into(), + SummaryOverlap { + authored, + from, + to, + required_from, + required_to, + } => [ + ("message", "Summary overlap".to_owned()), + ( + "reason", + if authored { + format!( + "A summary cannot be nested inside or split across another one, so \ + your text for turns {from}..{to} would have to stand in for turns \ + {required_from}..{required_to} as well." + ) + } else { + format!( + "Summarizing turns {from}..{to} would have to grow to turns \ + {required_from}..{required_to}, replacing a summary you wrote by \ + hand with a generated one." + ) + }, + ), + ( + "suggestion", + format!( + "Re-run with `--from {required_from} --to {required_to}` to cover the \ + whole range, or `jp conversation compact --reset` to drop the existing \ + compactions first." + ), + ), + ] + .into(), Summarize { model, reason } => [ ("message", "Summarization failed".to_owned()), ("model", model), diff --git a/crates/jp_cli/src/cmd/compact_flag.rs b/crates/jp_cli/src/cmd/compact_flag.rs index 56503eab1..a0acdcd67 100644 --- a/crates/jp_cli/src/cmd/compact_flag.rs +++ b/crates/jp_cli/src/cmd/compact_flag.rs @@ -103,7 +103,7 @@ impl clap::Args for CompactFlag { `--compact=SPEC` flags add multiple rules.\n\nBoth forms compose: bare \ `--compact` includes config rules, each `--compact=SPEC` adds a DSL \ rule.\n\nDSL format: POLICIES[:RANGE]\n\nPolicies are joined with `+`:\n- \ - `r` / `reasoning`: strip reasoning blocks\n- `s` / `summarize`: generate an \ + `r` / `reasoning`: strip reasoning blocks\n- `s` / `summary`: 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 \ @@ -161,7 +161,7 @@ pub(crate) struct CompactSpec { /// `None` = no tool-call policy. /// The mode mirrors the `--tools` flag. pub tools: Option, - pub summarize: bool, + pub summary: bool, /// `None` = use config defaults for range. pub range: Option, } @@ -189,7 +189,7 @@ impl CompactSpec { rule.reasoning = Some(ReasoningMode::Strip); } rule.tool_calls = self.tools; - if self.summarize { + if self.summary { rule.summary = Some(PartialSummaryConfig::default()); } @@ -216,7 +216,7 @@ impl FromStr for CompactSpec { let mut reasoning = false; let mut tools: Option = None; - let mut summarize = false; + let mut summary = false; for policy in policies_str.split('+') { let policy = policy.trim(); @@ -232,11 +232,13 @@ impl FromStr for CompactSpec { } reasoning = true; } - "s" | "summarize" => { + // `summarize` predates the `summary` spelling and stays + // accepted so existing specs keep parsing. + "s" | "summary" | "summarize" => { if value.is_some() { - return Err("`summarize` does not take a value".into()); + return Err("`summary` does not take a value".into()); } - summarize = true; + summary = true; } "t" | "tools" => { tools = Some(match value { @@ -250,7 +252,7 @@ impl FromStr for CompactSpec { } } - if !reasoning && tools.is_none() && !summarize { + if !reasoning && tools.is_none() && !summary { return Err("at least one policy required (r, t=MODE, s)".into()); } @@ -259,7 +261,7 @@ impl FromStr for CompactSpec { Ok(CompactSpec { reasoning, tools, - summarize, + summary, range, }) } diff --git a/crates/jp_cli/src/cmd/compact_flag_tests.rs b/crates/jp_cli/src/cmd/compact_flag_tests.rs index e51bb3222..2bcf5f72b 100644 --- a/crates/jp_cli/src/cmd/compact_flag_tests.rs +++ b/crates/jp_cli/src/cmd/compact_flag_tests.rs @@ -5,28 +5,43 @@ fn parse_policy_only() { assert_eq!("s".parse::().unwrap(), CompactSpec { reasoning: false, tools: None, - summarize: true, + summary: true, range: None, }); assert_eq!("r+t=strip".parse::().unwrap(), CompactSpec { reasoning: true, tools: Some(ToolCallsMode::Strip), - summarize: false, + summary: false, range: None, }); assert_eq!( - "reasoning+tools=strip+summarize" + "reasoning+tools=strip+summary" .parse::() .unwrap(), CompactSpec { reasoning: true, tools: Some(ToolCallsMode::Strip), - summarize: true, + summary: true, range: None, } ); } +#[test] +fn summarize_is_accepted_as_an_alias_for_summary() { + // Specs written against the older spelling keep parsing. + assert_eq!( + "summarize:..-3".parse::().unwrap(), + "summary:..-3".parse::().unwrap() + ); + + // Either spelling still rejects a value, naming the canonical one. + assert_eq!( + "summarize=x".parse::().unwrap_err(), + "`summary` does not take a value" + ); +} + #[test] fn parse_tool_modes() { let mode = |s: &str| s.parse::().unwrap().tools; @@ -48,7 +63,7 @@ fn parse_tool_mode_with_range() { assert_eq!("t=sres:..-3".parse::().unwrap(), CompactSpec { reasoning: false, tools: Some(ToolCallsMode::StripResponses), - summarize: false, + summary: false, range: Some(DslRange { from: None, to: Some(RuleBound::FromEnd(3)), @@ -61,7 +76,7 @@ fn parse_with_range() { assert_eq!("s:..-3".parse::().unwrap(), CompactSpec { reasoning: false, tools: None, - summarize: true, + summary: true, range: Some(DslRange { from: None, to: Some(RuleBound::FromEnd(3)), @@ -72,7 +87,7 @@ fn parse_with_range() { CompactSpec { reasoning: true, tools: Some(ToolCallsMode::Strip), - summarize: false, + summary: false, range: Some(DslRange { from: Some(RuleBound::Absolute(5)), to: Some(RuleBound::FromEnd(3)), @@ -82,7 +97,7 @@ fn parse_with_range() { assert_eq!("s:..".parse::().unwrap(), CompactSpec { reasoning: false, tools: None, - summarize: true, + summary: true, range: Some(DslRange { from: None, to: None, @@ -91,7 +106,7 @@ fn parse_with_range() { assert_eq!("r:5..".parse::().unwrap(), CompactSpec { reasoning: true, tools: None, - summarize: false, + summary: false, range: Some(DslRange { from: Some(RuleBound::Absolute(5)), to: None, @@ -131,7 +146,7 @@ fn parse_single_number_shorthand() { assert_eq!("s:-3".parse::().unwrap(), CompactSpec { reasoning: false, tools: None, - summarize: true, + summary: true, range: Some(DslRange { from: None, to: Some(RuleBound::FromEnd(3)), @@ -141,7 +156,7 @@ fn parse_single_number_shorthand() { assert_eq!("r:5".parse::().unwrap(), CompactSpec { reasoning: true, tools: None, - summarize: false, + summary: false, range: Some(DslRange { from: Some(RuleBound::Absolute(5)), to: None, diff --git a/crates/jp_cli/src/cmd/conversation/compact.rs b/crates/jp_cli/src/cmd/conversation/compact.rs index d8793b9fc..5153cc532 100644 --- a/crates/jp_cli/src/cmd/conversation/compact.rs +++ b/crates/jp_cli/src/cmd/conversation/compact.rs @@ -9,8 +9,8 @@ use jp_config::{ }, }; use jp_conversation::{ - Compaction, CompactionRange, ConversationStream, RangeBound, ReasoningPolicy, SummaryPolicy, - ToolCallPolicy, + Compaction, CompactionRange, ConversationStream, RangeBound, ReasoningPolicy, SummaryOverlap, + SummaryPolicy, SummarySource, ToolCallPolicy, compaction::{extend_summary_range, resolve_range}, }; use jp_workspace::{ConversationHandle, ConversationMut, Workspace}; @@ -83,14 +83,24 @@ pub(crate) struct Compact { )] tools: Option, - /// Generate an LLM summary for the compacted range. + /// Replace the compacted turns with a summary. /// - /// When enabled, the compacted turns are replaced with a single - /// LLM-generated summary. - /// Optionally accepts text passed to the summarizer as additional context, - /// e.g. `--summarize "focus on the architectural design"`. + /// Without a value, the summary is generated by the assistant. + /// With a value, that text is used verbatim and no model is called, e.g. + /// `--summary "we settled on the layered loader"`. #[arg(short, long, conflicts_with = "compact")] - summarize: Option>, + summary: Option>, + + /// Extra guidance for the summarizer, e.g. "focus on the architecture". + /// + /// Rides on the summarization request as supplementary context; it adds to + /// the summarizer's instructions rather than replacing them. + /// Applies to every rule in this invocation that generates a summary, + /// overriding any `conversation.compaction.rules[].summary.context` they + /// set themselves. + /// Has no effect on a verbatim `--summary TEXT`, which calls no model. + #[arg(long)] + summary_context: Option, /// The model to summarize with. /// @@ -124,7 +134,7 @@ pub(crate) struct Compact { value_parser = parse_compaction_index, conflicts_with_all = [ "keep_first", "keep_last", "from", "to", "first", "last", "turn", - "reasoning", "tools", "summarize", "compact", "model", + "reasoning", "tools", "summary", "summary_context", "compact", "model", ], )] reset: Option>, @@ -132,7 +142,7 @@ pub(crate) struct Compact { /// Compact using an inline DSL rule. /// /// Mutually exclusive with the dedicated `--reasoning`/`--tools`/ - /// `--summarize` flags above: use either the flags or the DSL, not both. + /// `--summary` flags above: use either the flags or the DSL, not both. /// See `jp query --help` for DSL syntax. #[command(flatten)] compact_flag: crate::cmd::compact_flag::CompactFlag, @@ -169,20 +179,22 @@ impl Compact { /// Returns `true` if any dedicated policy flag is set. /// - /// Policy flags (`--reasoning`/`--tools`/`--summarize`) build a single - /// ad-hoc rule. + /// Policy flags (`--reasoning`/`--tools`/`--summary`) build a single ad-hoc + /// rule. /// Range flags (`--keep-first`/`--keep-last`/`--from`/`--to`) are /// deliberately excluded: they are applied at runtime as range overrides on /// the active rules, not as a rule of their own. + /// `--summary-context` and `--model` are excluded for the same reason: they + /// modify whichever rules end up active rather than defining one. fn has_policy_overrides(&self) -> bool { - self.reasoning || self.tools.is_some() || self.summarize.is_some() + self.reasoning || self.tools.is_some() || self.summary.is_some() } } impl Compact { /// Resolve the effective compaction rules for this invocation. /// - /// Dedicated policy flags (`--reasoning`/`--tools`/`--summarize`) build one + /// Dedicated policy flags (`--reasoning`/`--tools`/`--summary`) build one /// ad-hoc rule; inline DSL specs (`-k SPEC`) each build one. clap makes the /// two mutually exclusive (the policy flags `conflicts_with` the `compact` /// flag), so at most one side is ever populated. @@ -207,9 +219,9 @@ impl Compact { rule.reasoning = Some(ReasoningMode::Strip); } rule.tool_calls = self.tools; - if let Some(context) = &self.summarize { + if let Some(text) = &self.summary { rule.summary = Some(PartialSummaryConfig { - context: context.clone(), + text: text.clone(), ..PartialSummaryConfig::default() }); } @@ -229,6 +241,12 @@ impl Compact { redirect_summaries_to_assistant_model(&mut rules, cfg); } + if let Some(context) = &self.summary_context { + for summary in rules.iter_mut().filter_map(|rule| rule.summary.as_mut()) { + summary.context = Some(context.clone()); + } + } + Ok(rules) } } @@ -329,13 +347,20 @@ fn resolve_reset_index( /// For non-summary rules the two streams are interchangeable (only /// `extend_summary_range` reads `overlap_stream`). /// Shared by the dry-run preview and the real build so they always agree. +/// +/// `Ok(None)` means the rule selects no turns. +/// +/// # Errors +/// +/// Returns the [`SummaryOverlap`] when the range would have to grow over a +/// verbatim summary, or a verbatim summary's range would have to grow. fn resolve_rule_range( range_stream: &ConversationStream, overlap_stream: &ConversationStream, rule: &CompactionRuleConfig, from_override: Bound, to_override: Bound, -) -> Option { +) -> Result, SummaryOverlap> { // A CLI override (`--from`/`--to`/`--keep-first`/`--keep-last`) takes // precedence; otherwise fall back to the rule's own bound. Either side // resolving to `Empty` means nothing is compacted. @@ -349,28 +374,55 @@ fn resolve_rule_range( }; let from = match from { - Bound::Empty => return None, + Bound::Empty => return Ok(None), Bound::Default => None, Bound::At(b) => Some(b), }; let to = match to { - Bound::Empty => return None, + Bound::Empty => return Ok(None), Bound::Default => None, Bound::At(b) => Some(b), }; - let range = resolve_range(range_stream, from, to)?; - Some(if rule.summary.is_some() { - extend_summary_range(overlap_stream, range) - } else { - range + let Some(range) = resolve_range(range_stream, from, to) else { + return Ok(None); + }; + + match rule_summary_source(rule) { + Some(source) => extend_summary_range(overlap_stream, range, source).map(Some), + None => Ok(Some(range)), + } +} + +/// The provenance of the summary this rule would produce, or `None` when it +/// produces no summary. +fn rule_summary_source(rule: &CompactionRuleConfig) -> Option { + rule.summary.as_ref().map(|summary| { + if summary.text.is_some() { + SummarySource::Authored + } else { + SummarySource::Generated + } }) } -/// Generate the summary text (if any) and assemble a [`Compaction`] for an +/// Report an unresolvable summary overlap in the 1-based turn numbers the user +/// typed. +fn overlap_error(overlap: &SummaryOverlap) -> crate::error::Error { + crate::error::Error::SummaryOverlap { + authored: overlap.new_is_authored, + from: overlap.requested.from_turn + 1, + to: overlap.requested.to_turn + 1, + required_from: overlap.required.from_turn + 1, + required_to: overlap.required.to_turn + 1, + } +} + +/// Resolve the summary text (if any) and assemble a [`Compaction`] for an /// already-resolved range. /// -/// The summarizer reads the raw events in `events` for the range. +/// A rule carrying `summary.text` is satisfied without contacting a provider. +/// Otherwise the summarizer reads the raw events in `events` for the range. async fn build_compaction_for_range( events: &ConversationStream, cfg: &jp_config::AppConfig, @@ -378,30 +430,40 @@ async fn build_compaction_for_range( range: CompactionRange, printer: Option<&jp_printer::Printer>, ) -> crate::Result { - let summary_text = if rule.summary.is_some() { - if let Some(printer) = printer { - printer.println("Generating summary..."); - } - let text = super::summarize::generate_summary( - events, - range.from_turn, - range.to_turn, - rule.summary.as_ref(), - cfg, - ) - .await?; - Some(text) - } else { - None + let compaction = build_mechanical_compaction(range.from_turn, range.to_turn, rule); + + let Some(summary) = rule.summary.as_ref() else { + return Ok(compaction); }; - let mut compaction = build_mechanical_compaction(range.from_turn, range.to_turn, rule); + if let Some(text) = summary.text.as_deref() { + // The generated path rejects an empty response rather than let it + // replace the turns it stands for; verbatim text is held to the same + // bar, whether it came from `--summary ""` or a blank `summary.text`. + if text.trim().is_empty() { + return Err(crate::error::Error::Compaction( + "the summary text is empty; drop the value to generate a summary instead" + .to_owned(), + )); + } - if let Some(text) = summary_text { - compaction = compaction.with_summary(SummaryPolicy { summary: text }); + return Ok(compaction.with_summary(SummaryPolicy::authored(text))); } - Ok(compaction) + if let Some(printer) = printer { + printer.println("Generating summary..."); + } + + let text = super::summarize::generate_summary( + events, + range.from_turn, + range.to_turn, + Some(summary), + cfg, + ) + .await?; + + Ok(compaction.with_summary(SummaryPolicy::generated(text))) } /// Build compaction events from the given resolved rules. @@ -430,14 +492,16 @@ pub(crate) async fn build_compaction_events( let mut overlap = events.clone(); let mut compactions = Vec::new(); for rule in rules { - let Some(range) = resolve_rule_range( + let range = match resolve_rule_range( events, &overlap, rule, from_override.clone(), to_override.clone(), - ) else { - continue; + ) { + Ok(Some(range)) => range, + Ok(None) => continue, + Err(conflict) => return Err(overlap_error(&conflict)), }; let compaction = build_compaction_for_range(events, cfg, rule, range, printer).await?; overlap.add_compaction(compaction.clone()); @@ -483,12 +547,16 @@ fn segments_for_compactions(compactions: &[Compaction], conv_id: &str) -> Vec Some( - match write_summary_file(conv_id, c.from_turn, c.to_turn, &summary.summary) { - Some(path) => format!("summary: {}", path.display()), - None => "summary".to_owned(), - }, - ), + Some(summary) => { + let kind = summary_label(summary.source); + Some( + match write_summary_file(conv_id, c.from_turn, c.to_turn, &summary.summary) + { + Some(path) => format!("{kind}: {}", path.display()), + None => kind.to_owned(), + }, + ) + } None => compaction_policy_label(c), }; TimelineSegment { @@ -501,6 +569,17 @@ fn segments_for_compactions(compactions: &[Compaction], conv_id: &str) -> Vec &'static str { + match source { + SummarySource::Generated => "summary", + SummarySource::Authored => "verbatim summary", + } +} + /// Build timeline segments for compactions already present at invocation start. /// /// Without these, the turns they cover would be reported as kept even though @@ -737,8 +816,13 @@ impl Compact { let to_override = self.resolve_to(&events_snapshot); if self.dry_run { - Self::preview_compaction(ctx, &events_snapshot, &rules, &from_override, &to_override); - return Ok(()); + return Self::preview_compaction( + ctx, + &events_snapshot, + &rules, + &from_override, + &to_override, + ); } let compactions = build_compaction_events( @@ -820,38 +904,41 @@ impl Compact { /// /// Resolves the same per-rule ranges as the real run (minus the summarizer /// and the mutation), then prints the dry-run timeline. - /// Summary rules show a bare `summary` label since no text is generated in - /// a preview. + /// Summary rules show a bare label since no text is generated in a preview. + /// An overlap that the real run would refuse is refused here too, so a + /// preview never promises a compaction the run cannot perform. fn preview_compaction( ctx: &Ctx, events_snapshot: &ConversationStream, rules: &[CompactionRuleConfig], from_override: &Bound, to_override: &Bound, - ) { + ) -> Output { // Range resolution uses the original snapshot for every rule, while // `overlap` accumulates this run's summaries so later summary rules // preview the same (possibly extended) ranges as the real run. let mut overlap = events_snapshot.clone(); let mut new_segments = Vec::new(); for rule in rules { - let Some(range) = resolve_rule_range( + let range = match resolve_rule_range( events_snapshot, &overlap, rule, from_override.clone(), to_override.clone(), - ) else { - continue; + ) { + Ok(Some(range)) => range, + Ok(None) => continue, + Err(conflict) => return Err(overlap_error(&conflict).into()), }; - let label = if rule.summary.is_some() { - Some("summary".to_owned()) - } else { - compaction_policy_label(&build_mechanical_compaction( + let source = rule_summary_source(rule); + let label = match source { + Some(source) => Some(summary_label(source).to_owned()), + None => compaction_policy_label(&build_mechanical_compaction( range.from_turn, range.to_turn, rule, - )) + )), }; new_segments.push(TimelineSegment { from: range.from_turn, @@ -859,10 +946,13 @@ impl Compact { label, existing: false, }); - if rule.summary.is_some() { + // The placeholder carries the rule's provenance so a second summary + // rule previews the same refusal the real run would produce. + if let Some(source) = source { overlap.add_compaction( Compaction::new(range.from_turn, range.to_turn).with_summary(SummaryPolicy { summary: String::new(), + source, }), ); } @@ -870,7 +960,7 @@ impl Compact { if new_segments.is_empty() { ctx.printer.println("Nothing to compact."); - return; + return Ok(()); } // Prepend the pre-existing compactions so already-compacted turns aren't @@ -882,6 +972,8 @@ impl Compact { for line in timeline_lines(&segments, last_turn, true) { ctx.printer.println(line); } + + Ok(()) } /// Resolve the `from` range override. diff --git a/crates/jp_cli/src/cmd/conversation/compact_tests.rs b/crates/jp_cli/src/cmd/conversation/compact_tests.rs index 6611b64fe..1993f1ea4 100644 --- a/crates/jp_cli/src/cmd/conversation/compact_tests.rs +++ b/crates/jp_cli/src/cmd/conversation/compact_tests.rs @@ -5,12 +5,13 @@ use jp_config::{ AppConfig, PartialAppConfig, conversation::compaction::{ CompactionConfig, CompactionRuleConfig, PartialCompactionRuleConfig, PartialSummaryConfig, - ReasoningMode, RuleBound, ToolCallsMode, + ReasoningMode, RuleBound, SummaryConfig, ToolCallsMode, }, model::{PartialModelConfig, id::PartialModelIdOrAliasConfig}, }; use jp_conversation::{ - Compaction, ConversationStream, RangeBound, ReasoningPolicy, ToolCallPolicy, + Compaction, ConversationStream, RangeBound, ReasoningPolicy, SummaryPolicy, SummarySource, + ToolCallPolicy, event::{ToolCallRequest, ToolCallResponse}, }; use jp_printer::Printer; @@ -63,7 +64,7 @@ fn model_flag_targets_the_assistant_model() { // `--model` rides the same `assistant.model.id` path as `jp query --model`, // so the pipeline resolves the alias. The summarizer picks it up through its // fallback: an unset `summary.model` means "use the assistant model". - let compact = parse_compact(&["--summarize", "--model", "gpt"]); + let compact = parse_compact(&["--summary", "--model", "gpt"]); let mut partial = PartialAppConfig::new_test(); partial = compact.apply_cli_config(None, partial, None).unwrap(); @@ -97,7 +98,7 @@ fn model_alias_reaches_a_configured_summary_model_through_the_pipeline() { }] .into(); - // No `--summarize`: a policy flag would replace the configured rule with an + // No `--summary`: a policy flag would replace the configured rule with an // ad-hoc one, and the configured `summary.model` is what this exercises. let compact = parse_compact(&["--model", "gpt"]); let partial = compact.apply_cli_config(None, partial, None).unwrap(); @@ -285,6 +286,177 @@ fn runtime() -> tokio::runtime::Runtime { tokio::runtime::Runtime::new().unwrap() } +/// A rule that summarizes the whole conversation with `text`, if given. +fn summary_rule(text: Option<&str>) -> CompactionRuleConfig { + CompactionRuleConfig { + keep_first: RuleBound::Turns(0), + keep_last: RuleBound::Turns(0), + reasoning: None, + tool_calls: None, + summary: Some(SummaryConfig { + text: text.map(ToOwned::to_owned), + model: None, + instructions: None, + context: None, + }), + } +} + +fn stream_of(turns: usize) -> ConversationStream { + let mut stream = ConversationStream::new_test(); + for t in 0..turns { + stream.start_turn(format!("turn {t}")); + } + stream +} + +#[test] +fn verbatim_summary_is_stored_as_authored_text() { + let stream = stream_of(4); + let cfg = AppConfig::new_test(); + + let compactions = runtime() + .block_on(build_compaction_events( + &stream, + &cfg, + &[summary_rule(Some("we settled on the layered loader"))], + Bound::Default, + Bound::Default, + Some(&Printer::sink()), + )) + .unwrap(); + + // `Authored` is reachable only through the branch that skips the + // summarizer, so this pins the no-model path rather than just the text. + assert_eq!(compactions.len(), 1); + assert_eq!( + compactions[0].summary, + Some(SummaryPolicy::authored("we settled on the layered loader")) + ); + assert_eq!( + compactions[0].summary.as_ref().unwrap().source, + SummarySource::Authored + ); +} + +#[test] +fn blank_verbatim_summary_is_rejected() { + let stream = stream_of(4); + let cfg = AppConfig::new_test(); + + let error = runtime() + .block_on(build_compaction_events( + &stream, + &cfg, + &[summary_rule(Some(" "))], + Bound::Default, + Bound::Default, + Some(&Printer::sink()), + )) + .unwrap_err(); + + assert_eq!( + error.to_string(), + "Compaction error: the summary text is empty; drop the value to generate a summary instead" + ); +} + +#[test] +fn verbatim_summary_refuses_to_widen_over_an_existing_summary() { + let mut stream = stream_of(6); + // Raw turns 3..5 are already summarized, so a verbatim summary of 0..3 + // would have to grow to 0..5 and stand in for turns it never described. + stream.add_compaction(Compaction::new(3, 5).with_summary(SummaryPolicy::generated("earlier"))); + + let mut rule = summary_rule(Some("hand-written")); + rule.keep_last = RuleBound::FromEnd(2); + + let cfg = AppConfig::new_test(); + let error = runtime() + .block_on(build_compaction_events( + &stream, + &cfg, + &[rule], + Bound::Default, + Bound::Default, + Some(&Printer::sink()), + )) + .unwrap_err(); + + // Turn numbers are reported 1-based, matching `--from`/`--to`. + let crate::error::Error::SummaryOverlap { + authored, + from, + to, + required_from, + required_to, + } = error + else { + panic!("expected a summary overlap, got: {error}"); + }; + assert!(authored, "the new summary is the verbatim one"); + assert_eq!((from, to, required_from, required_to), (1, 4, 1, 6)); +} + +#[test] +fn generated_summary_refuses_to_widen_over_verbatim_text() { + let mut stream = stream_of(6); + stream.add_compaction(Compaction::new(3, 5).with_summary(SummaryPolicy::authored("mine"))); + + let mut rule = summary_rule(None); + rule.keep_last = RuleBound::FromEnd(2); + + let cfg = AppConfig::new_test(); + let error = runtime() + .block_on(build_compaction_events( + &stream, + &cfg, + &[rule], + Bound::Default, + Bound::Default, + Some(&Printer::sink()), + )) + .unwrap_err(); + + // The refusal happens during range resolution, before any provider lookup, + // so the hand-written text survives. + let crate::error::Error::SummaryOverlap { + authored, + from, + to, + required_from, + required_to, + } = error + else { + panic!("expected a summary overlap, got: {error}"); + }; + assert!(!authored, "the blocking text is the existing summary"); + assert_eq!((from, to, required_from, required_to), (1, 4, 1, 6)); +} + +#[test] +fn verbatim_summary_covering_an_existing_summary_is_accepted() { + let mut stream = stream_of(6); + stream.add_compaction(Compaction::new(3, 5).with_summary(SummaryPolicy::generated("earlier"))); + + // The rule covers every turn, so nothing has to grow. + let cfg = AppConfig::new_test(); + let compactions = runtime() + .block_on(build_compaction_events( + &stream, + &cfg, + &[summary_rule(Some("covers everything"))], + Bound::Default, + Bound::Default, + Some(&Printer::sink()), + )) + .unwrap(); + + assert_eq!(compactions.len(), 1); + assert_eq!(compactions[0].from_turn, 0); + assert_eq!(compactions[0].to_turn, 5); +} + /// Each `ToolCallsMode` from the config maps to the right `ToolCallPolicy` on /// the produced `Compaction` event (the `jp_config` -\> `jp_conversation` /// bridge that lives in `build_mechanical_compaction`). @@ -608,17 +780,67 @@ fn keep_last_greater_than_last_is_rejected() { } #[test] -fn summarize_flag_distinguishes_absent_bare_and_valued() { - // The three states the `Option>` encoding exists to separate. - assert_eq!(parse_compact(&[]).summarize, None); - assert_eq!(parse_compact(&["--summarize"]).summarize, Some(None)); - assert_eq!(parse_compact(&["-s"]).summarize, Some(None)); +fn summary_flag_distinguishes_absent_bare_and_valued() { + // The three states the `Option>` encoding exists to separate: + // no summary, generate one, and use this exact text. + assert_eq!(parse_compact(&[]).summary, None); + assert_eq!(parse_compact(&["--summary"]).summary, Some(None)); + assert_eq!(parse_compact(&["-s"]).summary, Some(None)); assert_eq!( - parse_compact(&["-s", "focus on the architectural design"]).summarize, - Some(Some("focus on the architectural design".to_owned())), + parse_compact(&["-s", "we settled on the layered loader"]).summary, + Some(Some("we settled on the layered loader".to_owned())), ); } +#[test] +fn valued_summary_flag_becomes_verbatim_text_not_summarizer_context() { + let compact = parse_compact(&["--summary", "the gist of it"]); + let cfg = AppConfig::new_test(); + + let rules = compact.effective_rules(&cfg).unwrap(); + let summary = rules[0].summary.as_ref().expect("summary rule"); + + assert_eq!(summary.text.as_deref(), Some("the gist of it")); + assert_eq!(summary.context, None); +} + +#[test] +fn summary_context_flag_applies_to_configured_rules() { + // `--summary-context` modifies whichever rules are active, the same way + // `--model` does, instead of replacing them with an ad-hoc rule. + let mut cfg = AppConfig::new_test(); + cfg.conversation.compaction.rules = + CompactionConfig::finalize_rules(vec![PartialCompactionRuleConfig { + summary: Some(PartialSummaryConfig { + context: Some("configured context".to_owned()), + ..PartialSummaryConfig::default() + }), + ..PartialCompactionRuleConfig::default() + }]) + .unwrap(); + + let compact = parse_compact(&["--summary-context", "focus on the architecture"]); + let rules = compact.effective_rules(&cfg).unwrap(); + + assert_eq!(rules.len(), 1, "the configured rule must survive"); + assert_eq!( + rules[0].summary.as_ref().unwrap().context.as_deref(), + Some("focus on the architecture") + ); +} + +#[test] +fn summary_context_does_not_add_a_rule_of_its_own() { + // Without a summary rule to modify there is nothing to summarize, so the + // flag must not synthesize one. + let compact = parse_compact(&["--summary-context", "focus on the architecture"]); + let cfg = AppConfig::new_test(); + + let rules = compact.effective_rules(&cfg).unwrap(); + + assert_eq!(rules, cfg.conversation.compaction.rules); +} + #[test] fn turn_out_of_range_is_rejected() { // `--turn` names specific turns, so an endpoint past the conversation is an diff --git a/crates/jp_cli/src/error.rs b/crates/jp_cli/src/error.rs index e2dfa1508..20c211923 100644 --- a/crates/jp_cli/src/error.rs +++ b/crates/jp_cli/src/error.rs @@ -171,6 +171,25 @@ pub(crate) enum Error { #[error("Compaction error: {0}")] Compaction(String), + /// A summary range would have to grow over text that cannot be re-derived. + /// + /// Turn numbers are 1-based, as displayed. + #[error( + "Summary for turns {from}..{to} overlaps an existing summary covering turns \ + {required_from}..{required_to}" + )] + SummaryOverlap { + /// Whether the summary being created is the verbatim one. + /// + /// `false` means a generated summary is blocked by verbatim text + /// already covering part of the range. + authored: bool, + from: usize, + to: usize, + required_from: usize, + required_to: usize, + }, + /// The summarizer produced no usable summary. /// /// A dedicated variant so the failure names the model that produced diff --git a/crates/jp_cli/src/format_tests.rs b/crates/jp_cli/src/format_tests.rs index a67e76663..94c5f8000 100644 --- a/crates/jp_cli/src/format_tests.rs +++ b/crates/jp_cli/src/format_tests.rs @@ -8,9 +8,7 @@ fn compaction_detail_item_summary_takes_precedence_over_mechanical_label() { // fields (e.g. from an older DSL rule); summary must still win the label. let compaction = Compaction::new(0, 4) .with_reasoning(ReasoningPolicy::Strip) - .with_summary(SummaryPolicy { - summary: "the gist of it".to_owned(), - }); + .with_summary(SummaryPolicy::generated("the gist of it")); let item = compaction_detail_item(&compaction); diff --git a/crates/jp_config/src/conversation/compaction.rs b/crates/jp_config/src/conversation/compaction.rs index 7635cb623..f9b6f5a07 100644 --- a/crates/jp_config/src/conversation/compaction.rs +++ b/crates/jp_config/src/conversation/compaction.rs @@ -281,6 +281,14 @@ impl ToPartial for CompactionRuleConfig { #[derive(Debug, Clone, PartialEq, Config)] #[config(rename_all = "snake_case")] pub struct SummaryConfig { + /// Use this exact text as the summary instead of generating one. + /// + /// No model is called for this rule: the text is stored as-is and replaces + /// the compacted turns. + /// `model`, `instructions`, and `context` have no effect alongside it. + /// If unset, the summary is generated. + pub text: Option, + /// Model to use for summarization. /// /// If not set, the main assistant model is used. @@ -307,6 +315,7 @@ impl AssignKeyValue for PartialSummaryConfig { fn assign(&mut self, mut kv: KvAssignment) -> AssignResult { match kv.key_string().as_str() { "" => kv.try_merge_object(self)?, + "text" => self.text = kv.try_some_string()?, _ if kv.p("model") => self.model.assign(kv)?, "instructions" => self.instructions = kv.try_some_string()?, "context" => self.context = kv.try_some_string()?, @@ -320,6 +329,7 @@ impl AssignKeyValue for PartialSummaryConfig { impl PartialConfigDelta for PartialSummaryConfig { fn delta(&self, next: Self) -> Self { Self { + text: delta_opt(self.text.as_ref(), next.text), model: delta_opt_partial(self.model.as_ref(), next.model), instructions: delta_opt(self.instructions.as_ref(), next.instructions), context: delta_opt(self.context.as_ref(), next.context), @@ -330,6 +340,7 @@ impl PartialConfigDelta for PartialSummaryConfig { impl FillDefaults for PartialSummaryConfig { fn fill_from(self, defaults: Self) -> Self { Self { + text: self.text.or(defaults.text), model: fill::fill_opt(self.model, defaults.model), instructions: self.instructions.or(defaults.instructions), context: self.context.or(defaults.context), @@ -340,6 +351,7 @@ impl FillDefaults for PartialSummaryConfig { impl ToPartial for SummaryConfig { fn to_partial(&self) -> Self::Partial { Self::Partial { + text: partial_opts(self.text.as_ref(), None), model: partial_opt_config(self.model.as_ref(), None), instructions: partial_opts(self.instructions.as_ref(), None), context: partial_opts(self.context.as_ref(), None), diff --git a/crates/jp_conversation/src/compaction.rs b/crates/jp_conversation/src/compaction.rs index 6d7b11c51..c48e87e4c 100644 --- a/crates/jp_conversation/src/compaction.rs +++ b/crates/jp_conversation/src/compaction.rs @@ -108,8 +108,63 @@ pub enum ReasoningPolicy { /// `ChatRequest`/`ChatResponse` pair containing the summary text. #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] pub struct SummaryPolicy { - /// The summary text, generated at compaction-creation time. + /// The summary text, fixed at compaction-creation time. pub summary: String, + + /// Where the text came from. + #[serde(default, skip_serializing_if = "SummarySource::is_generated")] + pub source: SummarySource, +} + +impl SummaryPolicy { + /// A summary produced by a model reading the raw events in the range. + #[must_use] + pub fn generated(summary: impl Into) -> Self { + Self { + summary: summary.into(), + source: SummarySource::Generated, + } + } + + /// A summary supplied verbatim by the user. + #[must_use] + pub fn authored(summary: impl Into) -> Self { + Self { + summary: summary.into(), + source: SummarySource::Authored, + } + } +} + +/// Where a summary's text came from. +/// +/// A generated summary can be re-derived: widen its range, ask the model again, +/// and the new text covers the new range. +/// Authored text cannot be re-derived, so anything that would grow its range +/// has to involve the user (see [`extend_summary_range`]). +#[derive(Debug, Clone, Copy, PartialEq, Eq, Default, Serialize, Deserialize)] +#[serde(rename_all = "snake_case")] +pub enum SummarySource { + /// Produced by a model reading the raw events in the range. + #[default] + Generated, + + /// Supplied verbatim by the user. + Authored, +} + +impl SummarySource { + /// Whether the text was produced by a model. + #[must_use] + pub const fn is_generated(&self) -> bool { + matches!(self, Self::Generated) + } + + /// Whether the text was supplied verbatim by the user. + #[must_use] + pub const fn is_authored(&self) -> bool { + matches!(self, Self::Authored) + } } /// Policy for handling tool call request/response pairs during compaction. @@ -158,6 +213,28 @@ pub struct CompactionRange { pub to_turn: usize, } +/// A summary range that would have to grow over text nobody can re-derive. +/// +/// Returned by [`extend_summary_range`]; see there for when the extension is +/// refused. +/// Turn indices are 0-based, matching [`CompactionRange`]. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct SummaryOverlap { + /// The range the caller asked to summarize. + pub requested: CompactionRange, + + /// The union of `requested` and every summary range it touches. + /// + /// Summarizing this range instead is what resolves the overlap. + pub required: CompactionRange, + + /// Whether the summary being created is the authored one. + /// + /// `false` means a generated summary is blocked by authored text already + /// covering part of `required`. + pub new_is_authored: bool, +} + /// Extend a summary compaction range to fully subsume any partially overlapping /// existing summary compactions in the stream. /// @@ -176,40 +253,65 @@ pub struct CompactionRange { /// The extension repeats until stable, handling transitive chains (A overlaps /// B, B overlaps C → extend to cover all three). /// -/// Only considers existing compactions that have `summary: Some(...)`. +/// Only considers existing compactions that have `summary: Some(...)`; a +/// mechanical compaction over the same turns is left alone, since a summary +/// supersedes it at projection time without discarding it. /// Returns the input range unchanged if no summary overlaps it. /// /// Call this before generating the summary text so the summarizer reads events /// for the full refreshed range. -#[must_use] +/// +/// # Errors +/// +/// Growing the range is only a safe automatic repair while every summary +/// involved is [`Generated`], because generating again re-reads the raw events +/// for whatever the range grew to. +/// [`Authored`] text has no such fallback: widening it would leave the user's +/// words standing in for turns they never described, and widening *over* it +/// would replace those words with generated text. +/// So when growth is required and either side is authored, this returns a +/// [`SummaryOverlap`] naming the range that resolves it, leaving the choice to +/// the caller. +/// +/// Growth that isn't required is never refused: a range that already covers +/// every summary it touches is exactly what the caller asked for. +/// +/// [`Authored`]: SummarySource::Authored +/// [`Generated`]: SummarySource::Generated pub fn extend_summary_range( stream: &crate::ConversationStream, range: CompactionRange, -) -> CompactionRange { + source: SummarySource, +) -> Result { let mut from = range.from_turn; let mut to = range.to_turn; + let mut touches_authored = false; // Repeat until stable — extension may expose new overlaps. loop { let mut changed = false; for c in stream.compactions() { - if c.summary.is_none() { + let Some(summary) = c.summary.as_ref() else { continue; - } + }; // Grow to the union of any summary we touch (partial overlap *or* // containment), so adding a contained summary refreshes the whole // enclosing range instead of nesting inside it. let intersects = from <= c.to_turn && to >= c.from_turn; - if intersects { - let new_from = from.min(c.from_turn); - let new_to = to.max(c.to_turn); - if new_from != from || new_to != to { - from = new_from; - to = new_to; - changed = true; - } + if !intersects { + continue; + } + + touches_authored |= summary.source.is_authored(); + + let new_from = from.min(c.from_turn); + let new_to = to.max(c.to_turn); + if new_from != from || new_to != to { + from = new_from; + to = new_to; + changed = true; } } @@ -218,10 +320,24 @@ pub fn extend_summary_range( } } - CompactionRange { + let required = CompactionRange { from_turn: from, to_turn: to, + }; + + if required == range { + return Ok(range); } + + if source.is_authored() || touches_authored { + return Err(SummaryOverlap { + requested: range, + required, + new_is_authored: source.is_authored(), + }); + } + + Ok(required) } /// Resolve user-specified range bounds against a conversation stream. diff --git a/crates/jp_conversation/src/compaction_tests.rs b/crates/jp_conversation/src/compaction_tests.rs index 4a3fa1047..2f673bd67 100644 --- a/crates/jp_conversation/src/compaction_tests.rs +++ b/crates/jp_conversation/src/compaction_tests.rs @@ -66,9 +66,9 @@ fn roundtrip_summary_compaction() { timestamp: Utc.with_ymd_and_hms(2025, 1, 15, 12, 0, 0).unwrap(), from_turn: 0, to_turn: 10, - summary: Some(SummaryPolicy { - summary: "Set up a Rust project with error handling.".into(), - }), + summary: Some(SummaryPolicy::generated( + "Set up a Rust project with error handling.", + )), reasoning: None, tool_calls: None, }; @@ -134,9 +134,7 @@ fn reasoning_policy_roundtrip() { #[test] fn summary_policy_roundtrip() { - let policy = SummaryPolicy { - summary: "This is a summary of the conversation.".into(), - }; + let policy = SummaryPolicy::generated("This is a summary of the conversation."); let json = serde_json::to_value(&policy).unwrap(); assert_eq!(json["summary"], "This is a summary of the conversation."); @@ -144,6 +142,35 @@ fn summary_policy_roundtrip() { assert_eq!(policy, deserialized); } +#[test] +fn generated_summary_omits_source_from_json() { + let json = serde_json::to_value(SummaryPolicy::generated("text")).unwrap(); + let obj = json.as_object().unwrap(); + + // Generated is the overwhelmingly common case and the pre-`source` shape, + // so it stays off the wire. + assert!(!obj.contains_key("source")); +} + +#[test] +fn authored_summary_roundtrip() { + let policy = SummaryPolicy::authored("I wrote this myself."); + let json = serde_json::to_value(&policy).unwrap(); + assert_eq!(json["source"], "authored"); + + let deserialized: SummaryPolicy = serde_json::from_value(json).unwrap(); + assert_eq!(policy, deserialized); +} + +#[test] +fn summary_stored_without_a_source_loads_as_generated() { + // Every compaction written before `source` existed was model-generated. + let json = serde_json::json!({ "summary": "stored by an older build" }); + let policy: SummaryPolicy = serde_json::from_value(json).unwrap(); + + assert_eq!(policy, SummaryPolicy::generated("stored by an older build")); +} + // --------------------------------------------------------------------------- // Summary range auto-extension // --------------------------------------------------------------------------- @@ -153,14 +180,28 @@ fn summary_compaction(from: usize, to: usize, hour: u32) -> Compaction { timestamp: Utc.with_ymd_and_hms(2025, 1, 1, hour, 0, 0).unwrap(), from_turn: from, to_turn: to, - summary: Some(SummaryPolicy { - summary: format!("summary {from}-{to}"), - }), + summary: Some(SummaryPolicy::generated(format!("summary {from}-{to}"))), + reasoning: None, + tool_calls: None, + } +} + +fn authored_compaction(from: usize, to: usize, hour: u32) -> Compaction { + Compaction { + timestamp: Utc.with_ymd_and_hms(2025, 1, 1, hour, 0, 0).unwrap(), + from_turn: from, + to_turn: to, + summary: Some(SummaryPolicy::authored(format!("authored {from}-{to}"))), reasoning: None, tool_calls: None, } } +/// Extend a generated summary over `stream`, expecting no refusal. +fn extend_generated(stream: &ConversationStream, range: CompactionRange) -> CompactionRange { + extend_summary_range(stream, range, SummarySource::Generated).unwrap() +} + /// Build a stream with `n` turns. #[expect(clippy::cast_possible_truncation)] fn stream_with_turns(n: usize) -> ConversationStream { @@ -187,7 +228,7 @@ fn extend_no_existing_summaries() { from_turn: 3, to_turn: 7, }; - let result = extend_summary_range(&stream, range); + let result = extend_generated(&stream, range); assert_eq!(result, range, "No existing summaries → unchanged"); } @@ -200,7 +241,7 @@ fn extend_no_overlap() { from_turn: 5, to_turn: 8, }; - let result = extend_summary_range(&stream, range); + let result = extend_generated(&stream, range); assert_eq!(result, range, "Disjoint ranges → unchanged"); } @@ -215,7 +256,7 @@ fn extend_partial_overlap_right() { from_turn: 3, to_turn: 7, }; - let result = extend_summary_range(&stream, range); + let result = extend_generated(&stream, range); assert_eq!(result, CompactionRange { from_turn: 3, to_turn: 9 @@ -233,7 +274,7 @@ fn extend_partial_overlap_left() { from_turn: 3, to_turn: 8, }; - let result = extend_summary_range(&stream, range); + let result = extend_generated(&stream, range); assert_eq!(result, CompactionRange { from_turn: 0, to_turn: 8 @@ -250,7 +291,7 @@ fn extend_new_fully_contains_old() { from_turn: 0, to_turn: 8, }; - let result = extend_summary_range(&stream, range); + let result = extend_generated(&stream, range); assert_eq!(result, range); } @@ -266,7 +307,7 @@ fn extend_old_fully_contains_new() { from_turn: 3, to_turn: 5, }; - let result = extend_summary_range(&stream, range); + let result = extend_generated(&stream, range); assert_eq!(result, CompactionRange { from_turn: 0, to_turn: 9 @@ -287,7 +328,7 @@ fn extend_transitive_chain() { from_turn: 3, to_turn: 7, }; - let result = extend_summary_range(&stream, range); + let result = extend_generated(&stream, range); assert_eq!(result, CompactionRange { from_turn: 0, to_turn: 15 @@ -311,6 +352,122 @@ fn extend_ignores_mechanical_compactions() { from_turn: 3, to_turn: 7, }; - let result = extend_summary_range(&stream, range); + let result = extend_generated(&stream, range); assert_eq!(result, range, "Mechanical compactions should be ignored"); } + +// --------------------------------------------------------------------------- +// Authored summaries block automatic extension +// --------------------------------------------------------------------------- + +#[test] +fn authored_summary_refuses_to_widen_over_an_existing_summary() { + let mut stream = stream_with_turns(10); + stream.add_compaction(summary_compaction(5, 9, 10)); + + // Widening 3..7 to 3..9 would leave the user's text standing in for turns + // 8 and 9, which they never wrote about. + let range = CompactionRange { + from_turn: 3, + to_turn: 7, + }; + let error = extend_summary_range(&stream, range, SummarySource::Authored).unwrap_err(); + + assert_eq!(error, SummaryOverlap { + requested: range, + required: CompactionRange { + from_turn: 3, + to_turn: 9 + }, + new_is_authored: true, + }); +} + +#[test] +fn authored_summary_covering_the_whole_overlap_is_accepted() { + let mut stream = stream_with_turns(10); + stream.add_compaction(summary_compaction(3, 5, 10)); + + // 0..8 already subsumes the existing summary, so nothing has to grow and + // the authored text covers exactly the turns the user asked for. + let range = CompactionRange { + from_turn: 0, + to_turn: 8, + }; + let result = extend_summary_range(&stream, range, SummarySource::Authored).unwrap(); + + assert_eq!(result, range); +} + +#[test] +fn authored_summary_with_no_overlap_at_all_is_accepted() { + let mut stream = stream_with_turns(10); + stream.add_compaction(summary_compaction(0, 2, 10)); + + let range = CompactionRange { + from_turn: 5, + to_turn: 8, + }; + let result = extend_summary_range(&stream, range, SummarySource::Authored).unwrap(); + + assert_eq!(result, range); +} + +#[test] +fn generated_summary_refuses_to_widen_over_authored_text() { + let mut stream = stream_with_turns(10); + stream.add_compaction(authored_compaction(0, 4, 10)); + + // Extending 3..8 to 0..8 would replace hand-written text with generated + // text, beyond the range the user named. + let range = CompactionRange { + from_turn: 3, + to_turn: 8, + }; + let error = extend_summary_range(&stream, range, SummarySource::Generated).unwrap_err(); + + assert_eq!(error, SummaryOverlap { + requested: range, + required: CompactionRange { + from_turn: 0, + to_turn: 8 + }, + new_is_authored: false, + }); +} + +#[test] +fn authored_text_anywhere_in_a_transitive_chain_blocks_extension() { + let mut stream = stream_with_turns(20); + // Only C is authored, and it is reached only after two rounds of growth. + stream.add_compaction(summary_compaction(0, 5, 10)); + stream.add_compaction(summary_compaction(4, 10, 11)); + stream.add_compaction(authored_compaction(9, 15, 12)); + + let range = CompactionRange { + from_turn: 3, + to_turn: 7, + }; + let error = extend_summary_range(&stream, range, SummarySource::Generated).unwrap_err(); + + assert_eq!(error.required, CompactionRange { + from_turn: 0, + to_turn: 15 + }); +} + +#[test] +fn generated_summary_covering_authored_text_exactly_is_accepted() { + let mut stream = stream_with_turns(10); + stream.add_compaction(authored_compaction(3, 5, 10)); + + // The user named a range that already subsumes their own summary, so + // replacing it is explicit rather than incidental. + let range = CompactionRange { + from_turn: 0, + to_turn: 9, + }; + let result = extend_summary_range(&stream, range, SummarySource::Generated).unwrap(); + + assert_eq!(result, range); +} diff --git a/crates/jp_conversation/src/lib.rs b/crates/jp_conversation/src/lib.rs index d5d82f296..3883d38ff 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, + Compaction, CompactionRange, RangeBound, ReasoningPolicy, SummaryOverlap, SummaryPolicy, + SummarySource, ToolCallPolicy, resolve_range, }; pub use conversation::{Conversation, ConversationId}; pub use error::Error; diff --git a/crates/jp_conversation/src/stream/projection_tests.rs b/crates/jp_conversation/src/stream/projection_tests.rs index 50ecc499a..09beaf53a 100644 --- a/crates/jp_conversation/src/stream/projection_tests.rs +++ b/crates/jp_conversation/src/stream/projection_tests.rs @@ -153,9 +153,7 @@ fn summary_origins_preserve_raw_turn_numbers() { timestamp: ts(2), from_turn: 1, to_turn: 4, - summary: Some(SummaryPolicy { - summary: "middle turns".into(), - }), + summary: Some(SummaryPolicy::generated("middle turns")), reasoning: None, tool_calls: None, }); @@ -198,9 +196,7 @@ fn contained_summary_origins_reflect_actual_runs() { timestamp: ts(1), from_turn: 0, to_turn: 3, - summary: Some(SummaryPolicy { - summary: "OUTER".into(), - }), + summary: Some(SummaryPolicy::generated("OUTER")), reasoning: None, tool_calls: None, }); @@ -208,9 +204,7 @@ fn contained_summary_origins_reflect_actual_runs() { timestamp: ts(2), from_turn: 1, to_turn: 2, - summary: Some(SummaryPolicy { - summary: "INNER".into(), - }), + summary: Some(SummaryPolicy::generated("INNER")), reasoning: None, tool_calls: None, }); @@ -468,9 +462,9 @@ fn summary_replaces_all_events_in_range() { timestamp: ts(2), from_turn: 0, to_turn: 1, - summary: Some(SummaryPolicy { - summary: "Set up a Rust project with error handling.".into(), - }), + summary: Some(SummaryPolicy::generated( + "Set up a Rust project with error handling.", + )), reasoning: None, tool_calls: None, }); @@ -500,9 +494,7 @@ fn summary_ignores_per_type_policies() { timestamp: ts(2), from_turn: 0, to_turn: 1, - summary: Some(SummaryPolicy { - summary: "Everything summarized.".into(), - }), + summary: Some(SummaryPolicy::generated("Everything summarized.")), reasoning: Some(ReasoningPolicy::Strip), tool_calls: Some(ToolCallPolicy::Strip { request: true, @@ -528,9 +520,7 @@ fn summary_partial_range() { timestamp: ts(2), from_turn: 0, to_turn: 0, - summary: Some(SummaryPolicy { - summary: "Project was set up.".into(), - }), + summary: Some(SummaryPolicy::generated("Project was set up.")), reasoning: None, tool_calls: None, }); @@ -564,9 +554,7 @@ fn summary_is_injected_as_its_own_turn() { timestamp: ts(2), from_turn: 1, to_turn: 1, - summary: Some(SummaryPolicy { - summary: "summary of turn 1".into(), - }), + summary: Some(SummaryPolicy::generated("summary of turn 1")), reasoning: None, tool_calls: None, }); @@ -612,9 +600,7 @@ fn distinct_adjacent_summaries_with_identical_text_stay_separate() { timestamp: ts(1), from_turn: 0, to_turn: 0, - summary: Some(SummaryPolicy { - summary: "SAME".into(), - }), + summary: Some(SummaryPolicy::generated("SAME")), reasoning: None, tool_calls: None, }); @@ -622,9 +608,7 @@ fn distinct_adjacent_summaries_with_identical_text_stay_separate() { timestamp: ts(2), from_turn: 1, to_turn: 1, - summary: Some(SummaryPolicy { - summary: "SAME".into(), - }), + summary: Some(SummaryPolicy::generated("SAME")), reasoning: None, tool_calls: None, }); @@ -667,9 +651,7 @@ fn contained_summary_reinjects_outer_summary_tail() { timestamp: ts(1), from_turn: 0, to_turn: 3, - summary: Some(SummaryPolicy { - summary: "OUTER".into(), - }), + summary: Some(SummaryPolicy::generated("OUTER")), reasoning: None, tool_calls: None, }); @@ -677,9 +659,7 @@ fn contained_summary_reinjects_outer_summary_tail() { timestamp: ts(2), from_turn: 1, to_turn: 2, - summary: Some(SummaryPolicy { - summary: "INNER".into(), - }), + summary: Some(SummaryPolicy::generated("INNER")), reasoning: None, tool_calls: None, }); @@ -855,9 +835,7 @@ fn summary_wins_over_mechanical_for_same_turns() { timestamp: ts(3), from_turn: 0, to_turn: 1, - summary: Some(SummaryPolicy { - summary: "All summarized.".into(), - }), + summary: Some(SummaryPolicy::generated("All summarized.")), reasoning: None, tool_calls: None, }); @@ -947,9 +925,7 @@ fn config_deltas_preserved_through_projection() { timestamp: ts(2), from_turn: 0, to_turn: 1, - summary: Some(SummaryPolicy { - summary: "all gone".into(), - }), + summary: Some(SummaryPolicy::generated("all gone")), reasoning: None, tool_calls: None, }); @@ -996,9 +972,7 @@ fn empty_stream_with_compaction() { timestamp: ts(0), from_turn: 0, to_turn: 0, - summary: Some(SummaryPolicy { - summary: "nothing here".into(), - }), + summary: Some(SummaryPolicy::generated("nothing here")), reasoning: None, tool_calls: None, }); @@ -1199,13 +1173,7 @@ fn spec_to_compaction(spec: &CompactionSpec) -> Compaction { }), ), 4 => (None, None, Some(ToolCallPolicy::Omit)), - _ => ( - Some(SummaryPolicy { - summary: "s".into(), - }), - None, - None, - ), + _ => (Some(SummaryPolicy::generated("s")), None, None), }; Compaction { diff --git a/crates/jp_llm/src/provider/compaction_request_tests.rs b/crates/jp_llm/src/provider/compaction_request_tests.rs index 2746db9c4..7a6cdbf3e 100644 --- a/crates/jp_llm/src/provider/compaction_request_tests.rs +++ b/crates/jp_llm/src/provider/compaction_request_tests.rs @@ -161,12 +161,9 @@ fn reasoning_strip(provider: ProviderId, name: &str) -> Result { /// carrying a pre-computed summary; the trailing turn survives. fn summary(provider: ProviderId, name: &str) -> Result { snapshot(provider, name, |stream| { - stream.add_compaction( - Compaction::new(0, 2).with_summary(SummaryPolicy { - summary: "Earlier: the user asked about France's capital and had their notes read." - .to_owned(), - }), - ); + stream.add_compaction(Compaction::new(0, 2).with_summary(SummaryPolicy::generated( + "Earlier: the user asked about France's capital and had their notes read.", + ))); }) } @@ -219,14 +216,14 @@ fn tool_omit(provider: ProviderId, name: &str) -> Result { /// shared turn, so turn 0 keeps summary A and turns 1-2 resolve to summary B. fn summary_overlap(provider: ProviderId, name: &str) -> Result { snapshot(provider, name, |stream| { - let mut a = Compaction::new(0, 1).with_summary(SummaryPolicy { - summary: "Summary A: France's capital and the start of the notes lookup.".to_owned(), - }); + let mut a = Compaction::new(0, 1).with_summary(SummaryPolicy::generated( + "Summary A: France's capital and the start of the notes lookup.", + )); a.timestamp = ts(); - let mut b = Compaction::new(1, 2).with_summary(SummaryPolicy { - summary: "Summary B: the notes lookup and Germany's capital.".to_owned(), - }); + let mut b = Compaction::new(1, 2).with_summary(SummaryPolicy::generated( + "Summary B: the notes lookup and Germany's capital.", + )); b.timestamp = ts() + Duration::seconds(1); stream.add_compaction(a); diff --git a/docs/.vitepress/rfd-summaries.json b/docs/.vitepress/rfd-summaries.json index 127c0fd40..1451601bd 100644 --- a/docs/.vitepress/rfd-summaries.json +++ b/docs/.vitepress/rfd-summaries.json @@ -248,7 +248,7 @@ "summary": "Extend config wizard with frecency-based field ordering using CLI usage tracking data." }, "064-non-destructive-conversation-compaction.md": { - "hash": "04546fb7f0e0b853c24867506721f7f9f6fa0a90903cde85729c80dc26d703bc", + "hash": "197c94f58cac2161fb16f20248f1b7e8847ad859974d2b1e0a26dcb97466a5a3", "summary": "Non-destructive conversation compaction through overlay events that project reduced views without mutating stored data." }, "065-typed-resource-model-for-attachments.md": { diff --git a/docs/architecture/ubiquitous-language.md b/docs/architecture/ubiquitous-language.md index 0767d46e9..ba26fc74d 100644 --- a/docs/architecture/ubiquitous-language.md +++ b/docs/architecture/ubiquitous-language.md @@ -22,6 +22,9 @@ In disagreements between code and docs, the code is authoritative. - [Attachment](#attachment) - [Background Task](#background-task) - [CommandConfig](#commandconfig) + - [Compacted View](#compacted-view) + - [Compaction](#compaction) + - [Compaction Rule](#compaction-rule) - [Conversation](#conversation) - [Conversation Event](#conversation-event) - [EditorBackend](#editorbackend) @@ -34,6 +37,7 @@ In disagreements between code and docs, the code is authoritative. - [RFD](#rfd) - [Search Hit](#search-hit) - [Signal Router](#signal-router) + - [Summary](#summary) - [Thread](#thread) - [Tool Call](#tool-call) - [Turn](#turn) @@ -81,6 +85,50 @@ The policy around *when* JP is allowed to run a `CommandConfig` (prompt or not, confirm `shell = true` invocations) lives on each consumer, not on the shape itself. +### Compacted View + +What the LLM actually receives for a conversation: the raw event stream with +every [Compaction](#compaction) overlay applied. +Produced by `ConversationStream::apply_projection` in `jp_conversation`, which +also returns a `TurnOrigin` per resulting turn mapping it back to the raw turn +number(s) it stands for. +`jp conversation print --compacted` renders it. + +Turn numbering differs between the two: a [Summary](#summary) collapses its +range into a single turn, so the compacted view can be shorter than the +conversation it came from. + +**Not the same as** a [Workspace Projection](#workspace-projection). +The word "projection" carries two unrelated meanings in the codebase: applying +compaction overlays (`jp_conversation::stream::projection`) and writing a +conversation into the workspace directory (`Projection` in `jp_storage`). + +### Compaction + +A non-destructive overlay that reduces what the provider sees for an inclusive +range of turns. +The original events are never removed: the overlay is appended to the +conversation stream and applied when the [Compacted View](#compacted-view) is +built. +Implemented as `Compaction` in `jp_conversation::compaction`, carrying up to +three independent policies over its range — a [Summary](#summary), a reasoning +policy, and a tool-call policy. +See [RFD-064]. + +A Summary supersedes the other two for the turns it covers. + +### Compaction Rule + +The configuration that produces a [Compaction](#compaction): how many turns to +preserve at each end, and which policies to apply to the rest. +Implemented as `CompactionRuleConfig` in `jp_config::conversation::compaction`; +each rule yields exactly one Compaction when applied. + +**Not the same as** a Compaction. +A rule is durable configuration in relative terms ("keep the last turn"); a +Compaction is the event it produced, pinned to absolute turn indices and stored +in the conversation. + ### Conversation A persistent sequence of events identified by a `ConversationId`, living within @@ -208,6 +256,23 @@ interrupt down the stack. The registered scopes are the streaming loop, the tool execution loop, and the turn-level handler covering gaps between turn phases. +### Summary + +Text that stands in for a range of turns in the [Compacted +View](#compacted-view): the turns it covers collapse into a single synthetic +request/response pair carrying the text. +Attached to a [Compaction](#compaction) as `SummaryPolicy` in +`jp_conversation::compaction`, whose `SummarySource` records whether the text +was *generated* (produced by a model reading the raw events in the range) or +*authored* (supplied verbatim by the user). + +The distinction is operational: a generated summary can be re-derived for a +wider range, an authored one cannot. + +**Not the same as** the mechanical compaction policies (reasoning stripping, +tool-call stripping), which filter events within their range rather than +replacing the range. + ### Thread The decomposed, provider-facing projection of a Conversation: a rendered system @@ -258,4 +323,5 @@ See [RFD-031]. [RFD-001]: ../rfd/001-jp-rfd-process.md [RFD-031]: ../rfd/031-durable-conversation-storage-with-workspace-projection.md +[RFD-064]: ../rfd/064-non-destructive-conversation-compaction.md [`shlex::split`]: https://docs.rs/shlex diff --git a/docs/rfd/064-non-destructive-conversation-compaction.md b/docs/rfd/064-non-destructive-conversation-compaction.md index 428666a5c..c11d44d30 100644 --- a/docs/rfd/064-non-destructive-conversation-compaction.md +++ b/docs/rfd/064-non-destructive-conversation-compaction.md @@ -125,6 +125,15 @@ jp conversation compact --reset | `--reset` | `false` | Remove all compaction events from the | | | | stream. | +> [!TIP] +> `--summarize` is now spelled `--summary`, and its value means something +> different: `--summary` on its own generates a summary, while `--summary TEXT` +> stores TEXT verbatim and calls no model. +> Guidance for the summarizer moved to its own flag, `--summary-context TEXT`, +> which modifies whichever rules are active rather than defining one. +> In the DSL the policy is spelled `s` / `summary`, with `summarize` kept as an +> alias. + Range bounds accept several formats: | Value | Example | Meaning | @@ -203,7 +212,7 @@ SPEC = POLICIES [":" RANGE] POLICIES = POLICY ["+" POLICY]* POLICY = "r" | "reasoning" | "t" | "tools" - | "s" | "summarize" + | "s" | "summary" # "summarize" is accepted as an alias RANGE = [BOUND] ".." [BOUND] # explicit range (at least "..") | BOUND # single-bound shorthand BOUND = INTEGER # >= 0: absolute turn index From 15df1dcc676748b2d69503bc95fe9975d0969080 Mon Sep 17 00:00:00 2001 From: Jean Mertz Date: Mon, 3 Aug 2026 09:04:38 +0200 Subject: [PATCH 2/5] review feedback Signed-off-by: Jean Mertz --- crates/jp_cli/src/cmd/conversation/compact.rs | 40 ++++++--- .../src/cmd/conversation/compact_tests.rs | 85 ++++++++++++++++++- .../jp_config/src/conversation/compaction.rs | 11 ++- ...non-destructive-conversation-compaction.md | 44 +++++----- 4 files changed, 144 insertions(+), 36 deletions(-) diff --git a/crates/jp_cli/src/cmd/conversation/compact.rs b/crates/jp_cli/src/cmd/conversation/compact.rs index 5153cc532..8a4f8a2ca 100644 --- a/crates/jp_cli/src/cmd/conversation/compact.rs +++ b/crates/jp_cli/src/cmd/conversation/compact.rs @@ -406,6 +406,28 @@ fn rule_summary_source(rule: &CompactionRuleConfig) -> Option { }) } +/// Reject a verbatim summary whose text is blank. +/// +/// The generated path refuses an empty model response rather than let it +/// replace the turns it stands for; verbatim text is held to the same bar, +/// whether it came from `--summary ""` or a blank `summary.text`. +/// +/// Shared by the dry-run preview and the real build so a preview never promises +/// a compaction the run would reject. +fn validate_summary_text(rule: &CompactionRuleConfig) -> crate::Result<()> { + let Some(text) = rule.summary.as_ref().and_then(|s| s.text.as_deref()) else { + return Ok(()); + }; + + if text.trim().is_empty() { + return Err(crate::error::Error::Compaction( + "the summary text is empty; drop the value to generate a summary instead".to_owned(), + )); + } + + Ok(()) +} + /// Report an unresolvable summary overlap in the 1-based turn numbers the user /// typed. fn overlap_error(overlap: &SummaryOverlap) -> crate::error::Error { @@ -430,6 +452,8 @@ async fn build_compaction_for_range( range: CompactionRange, printer: Option<&jp_printer::Printer>, ) -> crate::Result { + validate_summary_text(rule)?; + let compaction = build_mechanical_compaction(range.from_turn, range.to_turn, rule); let Some(summary) = rule.summary.as_ref() else { @@ -437,16 +461,6 @@ async fn build_compaction_for_range( }; if let Some(text) = summary.text.as_deref() { - // The generated path rejects an empty response rather than let it - // replace the turns it stands for; verbatim text is held to the same - // bar, whether it came from `--summary ""` or a blank `summary.text`. - if text.trim().is_empty() { - return Err(crate::error::Error::Compaction( - "the summary text is empty; drop the value to generate a summary instead" - .to_owned(), - )); - } - return Ok(compaction.with_summary(SummaryPolicy::authored(text))); } @@ -931,6 +945,12 @@ impl Compact { Ok(None) => continue, Err(conflict) => return Err(overlap_error(&conflict).into()), }; + + // Checked after the range resolves so a rule that selects no turns + // is skipped here exactly as it is in the real run, which never + // reaches `build_compaction_for_range` for it. + validate_summary_text(rule)?; + let source = rule_summary_source(rule); let label = match source { Some(source) => Some(summary_label(source).to_owned()), diff --git a/crates/jp_cli/src/cmd/conversation/compact_tests.rs b/crates/jp_cli/src/cmd/conversation/compact_tests.rs index 1993f1ea4..a769f707c 100644 --- a/crates/jp_cli/src/cmd/conversation/compact_tests.rs +++ b/crates/jp_cli/src/cmd/conversation/compact_tests.rs @@ -1,5 +1,6 @@ use std::time::Duration; +use camino_tempfile::{Utf8TempDir, tempdir}; use clap::Parser as _; use jp_config::{ AppConfig, PartialAppConfig, @@ -14,14 +15,17 @@ use jp_conversation::{ ToolCallPolicy, event::{ToolCallRequest, ToolCallResponse}, }; -use jp_printer::Printer; +use jp_printer::{OutputFormat, Printer, SharedBuffer}; +use jp_workspace::Workspace; use serde_json::{Map, Value}; +use tokio::runtime::Runtime; use super::{ Bound, Compact, IntoPartialAppConfig as _, TimelineSegment, build_compaction_events, existing_segments, resolve_reset_index, segments_for_compactions, timeline_lines, }; use crate::cmd::{conversation_id::ConversationIds as _, target::ConversationTarget}; +use crate::{Globals, ctx::Ctx}; /// Parse a `Compact` from `jp conversation compact ` for flag tests. fn parse_compact(args: &[&str]) -> Compact { @@ -361,6 +365,85 @@ fn blank_verbatim_summary_is_rejected() { ); } +/// A `Ctx` backed by an in-memory printer, for exercising the dry-run preview. +/// +/// The tempdir and runtime are returned so they outlive the ctx: `Ctx::drop` +/// persists, and needs both. +fn preview_ctx() -> (Ctx, SharedBuffer, Utf8TempDir, Runtime) { + let tmp = tempdir().unwrap(); + let (printer, out, _err) = Printer::memory(OutputFormat::TextPretty); + + let ctx = Ctx::new( + Workspace::new(tmp.path()), + None, + Runtime::new().unwrap(), + Globals::default(), + AppConfig::new_test(), + None, + printer, + ); + + (ctx, out, tmp, Runtime::new().unwrap()) +} + +#[test] +fn preview_rejects_a_blank_verbatim_summary() { + // Regression: `--summary '' --dry-run` used to print a successful preview + // while the real run rejected the same rule, so the preview promised a + // compaction that could not be performed. + let (ctx, _out, _tmp, _rt) = preview_ctx(); + let stream = stream_of(4); + + let error = Compact::preview_compaction( + &ctx, + &stream, + &[summary_rule(Some(" "))], + &Bound::Default, + &Bound::Default, + ) + .unwrap_err(); + + // `preview_compaction` yields the rendered command error, so this pins what + // the user actually reads. + assert_eq!( + error.to_string(), + "error 1: Compaction error (error:\"the summary text is empty; drop the value to generate \ + a summary instead\")" + ); +} + +#[test] +fn preview_refuses_an_overlap_the_real_run_would_refuse() { + // The preview shares `resolve_rule_range` with the real run, so the same + // widening over an existing summary is refused before anything is printed. + let (ctx, out, _tmp, _rt) = preview_ctx(); + let mut stream = stream_of(6); + stream.add_compaction(Compaction::new(3, 5).with_summary(SummaryPolicy::generated("earlier"))); + + let mut rule = summary_rule(Some("hand-written")); + rule.keep_last = RuleBound::FromEnd(2); + + let error = + Compact::preview_compaction(&ctx, &stream, &[rule], &Bound::Default, &Bound::Default) + .unwrap_err(); + + ctx.printer.flush(); + // The full refusal as the user reads it: what went wrong, and the exact + // range that resolves it. + assert_eq!( + error.to_string(), + "error 1: Summary overlap (reason:\"A summary cannot be nested inside or split across \ + another one, so your text for turns 1..4 would have to stand in for turns 1..6 as \ + well.\", suggestion:\"Re-run with `--from 1 --to 6` to cover the whole range, or `jp \ + conversation compact --reset` to drop the existing compactions first.\")" + ); + assert_eq!( + out.lock().clone(), + "", + "a refused preview must print no timeline" + ); +} + #[test] fn verbatim_summary_refuses_to_widen_over_an_existing_summary() { let mut stream = stream_of(6); diff --git a/crates/jp_config/src/conversation/compaction.rs b/crates/jp_config/src/conversation/compaction.rs index f9b6f5a07..65d744e9a 100644 --- a/crates/jp_config/src/conversation/compaction.rs +++ b/crates/jp_config/src/conversation/compaction.rs @@ -196,11 +196,14 @@ pub struct CompactionRuleConfig { /// Policy for tool call arguments and responses. pub tool_calls: Option, - /// Summarization configuration. + /// Replace the events in the compacted range with a single summary. /// - /// When set, all events in the compacted range are replaced by a single - /// LLM-generated summary. - /// This takes precedence over `reasoning` and `tool_calls`. + /// Set `summary.text` to supply the summary yourself; otherwise it is + /// generated by a model reading the events in the range. + /// Takes precedence over `reasoning` and `tool_calls`, which are ignored + /// for the turns it covers. + /// If unset, the range keeps its events and only the mechanical policies + /// apply. #[setting(nested)] pub summary: Option, } diff --git a/docs/rfd/064-non-destructive-conversation-compaction.md b/docs/rfd/064-non-destructive-conversation-compaction.md index c11d44d30..5bf99a6ab 100644 --- a/docs/rfd/064-non-destructive-conversation-compaction.md +++ b/docs/rfd/064-non-destructive-conversation-compaction.md @@ -110,29 +110,31 @@ jp conversation compact --reset **Flags:** -| Flag | Default | Description | -| ------------------ | --------------------- | --------------------------------------- | -| `--keep-first ` | from config | Preserve the first N turns. | -| `--keep-last ` | from config | Preserve the last N turns. | -| `--from ` | start of conversation | Start of the compacted range | -| | | (inclusive). Overrides `--keep-first`. | -| `--to ` | end of conversation | End of the compacted range (inclusive). | -| | | Overrides `--keep-last`. | -| `--reasoning` | from config | Strip reasoning (thinking) blocks. | -| `--tools` | from config | Strip tool call arguments/responses. | -| `--summarize` | from config | Generate an LLM summary for the range. | -| `--dry-run` | `false` | Preview effects without applying. | -| `--reset` | `false` | Remove all compaction events from the | -| | | stream. | +| Flag | Default | Description | +| ------------------- | --------------------- | --------------------------------------- | +| `--keep-first ` | from config | Preserve the first N turns. | +| `--keep-last ` | from config | Preserve the last N turns. | +| `--from ` | start of conversation | Start of the compacted range | +| | | (inclusive). Overrides `--keep-first`. | +| `--to ` | end of conversation | End of the compacted range (inclusive). | +| | | Overrides `--keep-last`. | +| `--reasoning` | from config | Strip reasoning (thinking) blocks. | +| `--tools` | from config | Strip tool call arguments/responses. | +| `--summary [TEXT]` | from config | Replace the range with a summary: | +| | | generated with no value, or TEXT | +| | | verbatim with one. | +| `--summary-context` | none | Extra guidance for the summarizer. | +| | | Only affects rules that generate one. | +| `--dry-run` | `false` | Preview effects without applying. | +| `--reset` | `false` | Remove all compaction events from the | +| | | stream. | > [!TIP] -> `--summarize` is now spelled `--summary`, and its value means something -> different: `--summary` on its own generates a summary, while `--summary TEXT` -> stores TEXT verbatim and calls no model. -> Guidance for the summarizer moved to its own flag, `--summary-context TEXT`, -> which modifies whichever rules are active rather than defining one. -> In the DSL the policy is spelled `s` / `summary`, with `summarize` kept as an -> alias. +> `--summary` was originally spelled `--summarize`, and its value meant +> something different: it was passed to the summarizer as extra guidance rather +> than used as the summary text. +> A script still passing `--summarize TEXT` fails rather than silently storing +> TEXT as the summary; that guidance now belongs to `--summary-context`. Range bounds accept several formats: From 0f0c6726cad67005e32e24aa2ea96e4a958e8e7e Mon Sep 17 00:00:00 2001 From: Jean Mertz Date: Mon, 3 Aug 2026 13:44:44 +0200 Subject: [PATCH 3/5] review feedback Signed-off-by: Jean Mertz --- crates/jp_cli/src/cmd/conversation/compact.rs | 74 ++++++++--- .../src/cmd/conversation/compact_tests.rs | 58 ++++++++- .../src/cmd/conversation/print_tests.rs | 116 +++++++++--------- .../jp_cli/src/cmd/conversation/summarize.rs | 4 +- .../src/cmd/conversation/summarize_tests.rs | 13 ++ 5 files changed, 184 insertions(+), 81 deletions(-) diff --git a/crates/jp_cli/src/cmd/conversation/compact.rs b/crates/jp_cli/src/cmd/conversation/compact.rs index 8a4f8a2ca..3e2b9c701 100644 --- a/crates/jp_cli/src/cmd/conversation/compact.rs +++ b/crates/jp_cli/src/cmd/conversation/compact.rs @@ -484,6 +484,10 @@ async fn build_compaction_for_range( /// /// Each rule produces one `Compaction` event. /// Runtime range overrides (`--from`/`--to`) apply to every rule. +/// +/// Every range is resolved and every deterministic check runs before the first +/// summarizer request, so a rule that turns out to be unsatisfiable cannot +/// strand a paid request made for an earlier one. pub(crate) async fn build_compaction_events( events: &ConversationStream, cfg: &jp_config::AppConfig, @@ -492,19 +496,45 @@ pub(crate) async fn build_compaction_events( to_override: Bound, printer: Option<&jp_printer::Printer>, ) -> crate::Result> { - // Two distinct baselines: - // - // - Range resolution uses the original `events` for every rule, so - // `AfterLastCompaction` (`--from last-compaction` / `keep_first = - // "last-compaction"`) resolves - // against the compactions present at invocation start and applies - // uniformly, rather than each rule starting after the previous rule's - // freshly generated compaction. - // - `overlap` accumulates the compactions generated so far, so a later - // summary rule's overlap extension sees earlier summaries in this same - // invocation and can't be appended unextended. + let plan = plan_compactions(events, rules, &from_override, &to_override)?; + + let mut compactions = Vec::with_capacity(plan.len()); + for (rule, range) in plan { + compactions.push(build_compaction_for_range(events, cfg, rule, range, printer).await?); + } + + Ok(compactions) +} + +/// Resolve the range each rule would compact, in order, rejecting anything the +/// build would reject. +/// +/// Rules that select no turns are dropped rather than reported. +/// +/// Two distinct baselines: +/// +/// - Range resolution uses the original `events` for every rule, so +/// `AfterLastCompaction` (`--from last-compaction` / `keep_first = +/// "last-compaction"`) resolves against the compactions present at invocation +/// start and applies uniformly, rather than each rule starting after the +/// previous rule's freshly generated compaction. +/// - `overlap` accumulates the compactions planned so far, so a later summary +/// rule's overlap extension sees earlier summaries in this same invocation +/// and can't be appended unextended. +/// +/// # Errors +/// +/// Returns the first unresolvable summary overlap, or the first blank verbatim +/// summary. +fn plan_compactions<'a>( + events: &ConversationStream, + rules: &'a [CompactionRuleConfig], + from_override: &Bound, + to_override: &Bound, +) -> crate::Result> { let mut overlap = events.clone(); - let mut compactions = Vec::new(); + let mut plan = Vec::new(); + for rule in rules { let range = match resolve_rule_range( events, @@ -517,12 +547,24 @@ pub(crate) async fn build_compaction_events( Ok(None) => continue, Err(conflict) => return Err(overlap_error(&conflict)), }; - let compaction = build_compaction_for_range(events, cfg, rule, range, printer).await?; - overlap.add_compaction(compaction.clone()); - compactions.push(compaction); + + validate_summary_text(rule)?; + + // Only the range and the provenance affect how a later rule resolves, so + // a placeholder stands in for the compaction this rule will produce. + if let Some(source) = rule_summary_source(rule) { + overlap.add_compaction( + Compaction::new(range.from_turn, range.to_turn).with_summary(SummaryPolicy { + summary: String::new(), + source, + }), + ); + } + + plan.push((rule, range)); } - Ok(compactions) + Ok(plan) } /// Apply compaction events to the conversation stream. diff --git a/crates/jp_cli/src/cmd/conversation/compact_tests.rs b/crates/jp_cli/src/cmd/conversation/compact_tests.rs index a769f707c..cdd10877b 100644 --- a/crates/jp_cli/src/cmd/conversation/compact_tests.rs +++ b/crates/jp_cli/src/cmd/conversation/compact_tests.rs @@ -367,9 +367,9 @@ fn blank_verbatim_summary_is_rejected() { /// A `Ctx` backed by an in-memory printer, for exercising the dry-run preview. /// -/// The tempdir and runtime are returned so they outlive the ctx: `Ctx::drop` -/// persists, and needs both. -fn preview_ctx() -> (Ctx, SharedBuffer, Utf8TempDir, Runtime) { +/// The tempdir is returned so it outlives the ctx, whose workspace points into +/// it. +fn preview_ctx() -> (Ctx, SharedBuffer, Utf8TempDir) { let tmp = tempdir().unwrap(); let (printer, out, _err) = Printer::memory(OutputFormat::TextPretty); @@ -383,7 +383,7 @@ fn preview_ctx() -> (Ctx, SharedBuffer, Utf8TempDir, Runtime) { printer, ); - (ctx, out, tmp, Runtime::new().unwrap()) + (ctx, out, tmp) } #[test] @@ -391,7 +391,7 @@ fn preview_rejects_a_blank_verbatim_summary() { // Regression: `--summary '' --dry-run` used to print a successful preview // while the real run rejected the same rule, so the preview promised a // compaction that could not be performed. - let (ctx, _out, _tmp, _rt) = preview_ctx(); + let (ctx, _out, _tmp) = preview_ctx(); let stream = stream_of(4); let error = Compact::preview_compaction( @@ -416,7 +416,7 @@ fn preview_rejects_a_blank_verbatim_summary() { fn preview_refuses_an_overlap_the_real_run_would_refuse() { // The preview shares `resolve_rule_range` with the real run, so the same // widening over an existing summary is refused before anything is printed. - let (ctx, out, _tmp, _rt) = preview_ctx(); + let (ctx, out, _tmp) = preview_ctx(); let mut stream = stream_of(6); stream.add_compaction(Compaction::new(3, 5).with_summary(SummaryPolicy::generated("earlier"))); @@ -517,6 +517,52 @@ fn generated_summary_refuses_to_widen_over_verbatim_text() { assert_eq!((from, to, required_from, required_to), (1, 4, 1, 6)); } +#[test] +fn a_later_overlap_is_refused_before_any_summarizer_request() { + // Regression: the first rule's summary used to be generated before the + // second rule's range was resolved, so an overlap in the second rule threw + // away a paid request nothing recorded. + // + // `AppConfig::new_test()` points the assistant at `anthropic/test`, so any + // summarizer call fails on provider lookup. Getting `SummaryOverlap` back is + // therefore proof that rule 1 never reached a provider. + let mut stream = stream_of(10); + stream.add_compaction(Compaction::new(6, 8).with_summary(SummaryPolicy::authored("mine"))); + + // Rule 1 generates a summary for turns 0..2, disjoint from the authored one. + let mut generated = summary_rule(None); + generated.keep_last = RuleBound::FromEnd(7); + + // Rule 2 covers 4..7, so it has to grow over the authored summary. + let mut conflicting = summary_rule(None); + conflicting.keep_first = RuleBound::Absolute(5); + conflicting.keep_last = RuleBound::FromEnd(2); + + let cfg = AppConfig::new_test(); + let error = runtime() + .block_on(build_compaction_events( + &stream, + &cfg, + &[generated, conflicting], + Bound::Default, + Bound::Default, + Some(&Printer::sink()), + )) + .unwrap_err(); + + let crate::error::Error::SummaryOverlap { + from, + to, + required_from, + required_to, + .. + } = error + else { + panic!("expected the overlap to be reported before summarizing, got: {error}"); + }; + assert_eq!((from, to, required_from, required_to), (5, 8, 5, 9)); +} + #[test] fn verbatim_summary_covering_an_existing_summary_is_accepted() { let mut stream = stream_of(6); diff --git a/crates/jp_cli/src/cmd/conversation/print_tests.rs b/crates/jp_cli/src/cmd/conversation/print_tests.rs index 0a449ff05..4e0299756 100644 --- a/crates/jp_cli/src/cmd/conversation/print_tests.rs +++ b/crates/jp_cli/src/cmd/conversation/print_tests.rs @@ -1,6 +1,6 @@ use std::time::Duration; -use camino_tempfile::tempdir; +use camino_tempfile::{Utf8TempDir, tempdir}; use chrono::{DateTime, TimeZone as _, Utc}; use jp_config::{ AppConfig, PartialAppConfig, @@ -39,16 +39,16 @@ fn ts(h: u32, m: u32, s: u32) -> DateTime { /// Create a `Ctx` backed by an in-memory printer. /// -/// Returns the ctx, conversation id, output buffer, and the runtime (kept alive -/// so `Ctx::drop` can persist without panicking). +/// Returns the ctx, conversation id, the stdout and stderr buffers, and the +/// tempdir, which is returned so it outlives the ctx whose workspace points +/// into it. fn setup_ctx_with_config( config: AppConfig, events: Vec, -) -> (Ctx, ConversationId, SharedBuffer, SharedBuffer, Runtime) { +) -> (Ctx, ConversationId, SharedBuffer, SharedBuffer, Utf8TempDir) { let tmp = tempdir().unwrap(); let (printer, out, err) = Printer::memory(OutputFormat::TextPretty); let workspace = Workspace::new(tmp.path()); - let runtime = Runtime::new().unwrap(); let mut ctx = Ctx::new( workspace, @@ -67,18 +67,18 @@ fn setup_ctx_with_config( let lock = ctx.workspace.test_lock(h); lock.as_mut().update_events(|e| e.extend(events)); - (ctx, id, out, err, runtime) + (ctx, id, out, err, tmp) } fn setup_ctx( events: Vec, -) -> (Ctx, ConversationId, SharedBuffer, SharedBuffer, Runtime) { +) -> (Ctx, ConversationId, SharedBuffer, SharedBuffer, Utf8TempDir) { setup_ctx_with_config(AppConfig::new_test(), events) } #[test] fn prints_user_message() { - let (mut ctx, id, out, _err, _rt) = setup_ctx(vec![ConversationEvent::new( + let (mut ctx, id, out, _err, _tmp) = setup_ctx(vec![ConversationEvent::new( ChatRequest::from("Hello world"), ts(0, 0, 0), )]); @@ -111,7 +111,7 @@ fn prints_reasoning_events_split_by_a_redacted_event_as_one_region() { config.style.reasoning.display = ReasoningDisplayConfig::Full; config.style.reasoning.background = None; - let (mut ctx, id, out, _err, _rt) = setup_ctx_with_config(config, vec![ + let (mut ctx, id, out, _err, _tmp) = setup_ctx_with_config(config, vec![ ConversationEvent::new( ChatResponse::reasoning("I can test this directly by ver"), ts(0, 0, 0), @@ -150,7 +150,7 @@ fn prints_reasoning_separator_as_a_block_break() { config.style.reasoning.display = ReasoningDisplayConfig::Full; config.style.reasoning.background = None; - let (mut ctx, id, out, _err, _rt) = setup_ctx_with_config(config, vec![ + let (mut ctx, id, out, _err, _tmp) = setup_ctx_with_config(config, vec![ ConversationEvent::new(ChatResponse::reasoning("First section."), ts(0, 0, 0)), ConversationEvent::new(ChatResponse::reasoning("\n\nSecond section."), ts(0, 0, 1)), ]); @@ -176,7 +176,7 @@ fn prints_reasoning_separator_as_a_block_break() { #[test] fn prints_assistant_message() { - let (mut ctx, id, out, _err, _rt) = setup_ctx(vec![ConversationEvent::new( + let (mut ctx, id, out, _err, _tmp) = setup_ctx(vec![ConversationEvent::new( ChatResponse::message("The answer is 42.\n\n"), ts(0, 0, 1), )]); @@ -202,7 +202,7 @@ fn prints_reasoning_full() { let mut config = AppConfig::new_test(); config.style.reasoning.display = ReasoningDisplayConfig::Full; - let (mut ctx, id, out, _err, _rt) = setup_ctx_with_config(config, vec![ + let (mut ctx, id, out, _err, _tmp) = setup_ctx_with_config(config, vec![ ConversationEvent::new( ChatResponse::reasoning("Let me think about this...\n\n"), ts(0, 0, 0), @@ -235,7 +235,7 @@ fn hides_reasoning_when_hidden() { let mut config = AppConfig::new_test(); config.style.reasoning.display = ReasoningDisplayConfig::Hidden; - let (mut ctx, id, out, _err, _rt) = setup_ctx_with_config(config, vec![ + let (mut ctx, id, out, _err, _tmp) = setup_ctx_with_config(config, vec![ ConversationEvent::new(ChatResponse::reasoning("Secret thoughts\n\n"), ts(0, 0, 0)), ConversationEvent::new(ChatResponse::message("Visible answer.\n\n"), ts(0, 0, 1)), ]); @@ -295,7 +295,7 @@ fn truncates_reasoning() { #[test] fn prints_tool_call_and_result() { - let (mut ctx, id, _out, err, _rt) = setup_ctx(vec![ + let (mut ctx, id, _out, err, _tmp) = setup_ctx(vec![ ConversationEvent::new( ToolCallRequest { id: "tc1".into(), @@ -336,7 +336,7 @@ fn prints_tool_call_and_result() { #[test] fn prints_structured_data() { let data = json!({"name": "Alice", "age": 30}); - let (mut ctx, id, out, _err, _rt) = setup_ctx(vec![ConversationEvent::new( + let (mut ctx, id, out, _err, _tmp) = setup_ctx(vec![ConversationEvent::new( ChatResponse::structured(data.clone()), ts(0, 0, 0), )]); @@ -367,7 +367,7 @@ fn prints_structured_data() { #[test] fn structured_fence_is_closed_at_end_of_replay() { let data = json!({"name": "Alice"}); - let (mut ctx, id, out, _err, _rt) = setup_ctx(vec![ConversationEvent::new( + let (mut ctx, id, out, _err, _tmp) = setup_ctx(vec![ConversationEvent::new( ChatResponse::structured(data), ts(0, 0, 0), )]); @@ -399,7 +399,7 @@ fn structured_fence_is_closed_at_end_of_replay() { /// the role/content boundary, not left open until end-of-stream. #[test] fn structured_response_followed_by_message_closes_fence_first() { - let (mut ctx, id, out, _err, _rt) = setup_ctx(vec![ + let (mut ctx, id, out, _err, _tmp) = setup_ctx(vec![ ConversationEvent::new(TurnStart, ts(0, 0, 0)), ConversationEvent::new(ChatRequest::from("Extract"), ts(0, 0, 1)), ConversationEvent::new( @@ -447,7 +447,7 @@ fn structured_response_followed_by_message_closes_fence_first() { /// values being appended inside the first one as `}{`. #[test] fn prints_consecutive_structured_events_as_separate_fences() { - let (mut ctx, id, out, _err, _rt) = setup_ctx(vec![ + let (mut ctx, id, out, _err, _tmp) = setup_ctx(vec![ ConversationEvent::new(TurnStart, ts(0, 0, 0)), ConversationEvent::new(ChatRequest::from("Extract"), ts(0, 0, 1)), ConversationEvent::new( @@ -490,7 +490,7 @@ fn prints_consecutive_structured_events_as_separate_fences() { /// the close has to come from that branch. #[test] fn structured_to_message_in_same_turn_closes_fence_first() { - let (mut ctx, id, out, _err, _rt) = setup_ctx(vec![ + let (mut ctx, id, out, _err, _tmp) = setup_ctx(vec![ ConversationEvent::new(TurnStart, ts(0, 0, 0)), ConversationEvent::new(ChatRequest::from("Extract"), ts(0, 0, 1)), ConversationEvent::new( @@ -529,7 +529,7 @@ fn structured_to_message_in_same_turn_closes_fence_first() { #[test] fn turn_separators_between_turns() { - let (mut ctx, id, out, _err, _rt) = setup_ctx(vec![ + let (mut ctx, id, out, _err, _tmp) = setup_ctx(vec![ ConversationEvent::new(TurnStart, ts(0, 0, 0)), ConversationEvent::new(ChatRequest::from("First question"), ts(0, 0, 1)), ConversationEvent::new(ChatResponse::message("First answer.\n\n"), ts(0, 0, 2)), @@ -557,7 +557,7 @@ fn turn_separators_between_turns() { #[test] fn turn_header_shows_turn_number_and_relative_time() { - let (mut ctx, id, out, _err, _rt) = setup_ctx(vec![ + let (mut ctx, id, out, _err, _tmp) = setup_ctx(vec![ ConversationEvent::new(TurnStart, ts(0, 0, 0)), ConversationEvent::new(ChatRequest::from("First question"), ts(0, 0, 1)), ConversationEvent::new(ChatResponse::message("First answer.\n\n"), ts(0, 0, 2)), @@ -616,7 +616,7 @@ fn turn_header_detail_on_assistant_first_turn() { // must still carry the detail on the assistant header. This pins the // `ensure_assistant_header` consumption path, which // `turn_header_shows_turn_number_and_relative_time` (user-first) does not. - let (mut ctx, id, out, _err, _rt) = setup_ctx(vec![ConversationEvent::new( + let (mut ctx, id, out, _err, _tmp) = setup_ctx(vec![ConversationEvent::new( ChatResponse::message("Answer only.\n\n"), ts(0, 0, 0), )]); @@ -646,7 +646,7 @@ fn turn_header_detail_on_assistant_first_turn() { #[test] fn prints_conversation_by_id() { - let (mut ctx, id, out, _err, _rt) = setup_ctx(vec![ConversationEvent::new( + let (mut ctx, id, out, _err, _tmp) = setup_ctx(vec![ConversationEvent::new( ChatRequest::from("active conversation content"), ts(0, 0, 0), )]); @@ -672,7 +672,7 @@ fn prints_conversation_by_id() { #[test] fn empty_conversation_produces_no_content() { - let (mut ctx, id, out, _err, _rt) = setup_ctx(vec![]); + let (mut ctx, id, out, _err, _tmp) = setup_ctx(vec![]); let print = Print { target: PositionalIds::from_targets(vec![ConversationTarget::Id(id)]), @@ -696,7 +696,7 @@ fn empty_conversation_produces_no_content() { #[test] fn full_conversation_round_trip() { - let (mut ctx, id, out, err, _rt) = setup_ctx(vec![ + let (mut ctx, id, out, err, _tmp) = setup_ctx(vec![ ConversationEvent::new(TurnStart, ts(0, 0, 0)), ConversationEvent::new(ChatRequest::from("What is Rust?"), ts(0, 0, 1)), ConversationEvent::new( @@ -757,7 +757,7 @@ fn full_conversation_round_trip() { #[test] fn last_prints_only_last_turn() { - let (mut ctx, id, out, _err, _rt) = setup_ctx(vec![ + let (mut ctx, id, out, _err, _tmp) = setup_ctx(vec![ ConversationEvent::new(TurnStart, ts(0, 0, 0)), ConversationEvent::new(ChatRequest::from("First question"), ts(0, 0, 1)), ConversationEvent::new(ChatResponse::message("First answer.\n\n"), ts(0, 0, 2)), @@ -795,7 +795,7 @@ fn last_prints_only_last_turn() { #[test] fn last_two_with_three_turns() { - let (mut ctx, id, out, _err, _rt) = setup_ctx(vec![ + let (mut ctx, id, out, _err, _tmp) = setup_ctx(vec![ ConversationEvent::new(TurnStart, ts(0, 0, 0)), ConversationEvent::new(ChatRequest::from("Turn one"), ts(0, 0, 1)), ConversationEvent::new(ChatResponse::message("Answer one.\n\n"), ts(0, 0, 2)), @@ -830,7 +830,7 @@ fn last_two_with_three_turns() { #[test] fn last_exceeding_turn_count_prints_all() { - let (mut ctx, id, out, _err, _rt) = setup_ctx(vec![ + let (mut ctx, id, out, _err, _tmp) = setup_ctx(vec![ ConversationEvent::new(TurnStart, ts(0, 0, 0)), ConversationEvent::new(ChatRequest::from("Only question"), ts(0, 0, 1)), ConversationEvent::new(ChatResponse::message("Only answer.\n\n"), ts(0, 0, 2)), @@ -857,7 +857,7 @@ fn last_exceeding_turn_count_prints_all() { #[test] fn blank_line_between_tool_calls_and_message() { - let (mut ctx, id, out, err, _rt) = setup_ctx(vec![ + let (mut ctx, id, out, err, _tmp) = setup_ctx(vec![ ConversationEvent::new(TurnStart, ts(0, 0, 0)), ConversationEvent::new(ChatRequest::from("Check this"), ts(0, 0, 1)), ConversationEvent::new( @@ -909,7 +909,7 @@ fn blank_line_between_tool_calls_and_message() { #[test] fn blank_line_between_message_and_tool_calls() { - let (mut ctx, id, out, err, _rt) = setup_ctx(vec![ + let (mut ctx, id, out, err, _tmp) = setup_ctx(vec![ ConversationEvent::new(TurnStart, ts(0, 0, 0)), ConversationEvent::new(ChatRequest::from("Help me"), ts(0, 0, 1)), ConversationEvent::new( @@ -968,7 +968,7 @@ fn blank_line_between_message_and_tool_calls() { #[test] fn no_extra_blank_line_between_consecutive_tool_calls() { - let (mut ctx, id, _out, err, _rt) = setup_ctx(vec![ + let (mut ctx, id, _out, err, _tmp) = setup_ctx(vec![ ConversationEvent::new(TurnStart, ts(0, 0, 0)), ConversationEvent::new(ChatRequest::from("Do two things"), ts(0, 0, 1)), ConversationEvent::new( @@ -1034,7 +1034,7 @@ fn no_extra_blank_line_between_consecutive_tool_calls() { #[test] fn last_zero_prints_nothing() { - let (mut ctx, id, out, _err, _rt) = setup_ctx(vec![ + let (mut ctx, id, out, _err, _tmp) = setup_ctx(vec![ ConversationEvent::new(TurnStart, ts(0, 0, 0)), ConversationEvent::new(ChatRequest::from("Hello"), ts(0, 0, 1)), ConversationEvent::new(ChatResponse::message("World.\n\n"), ts(0, 0, 2)), @@ -1062,7 +1062,7 @@ fn last_zero_prints_nothing() { #[test] fn turn_prints_specific_turn() { - let (mut ctx, id, out, _err, _rt) = setup_ctx(vec![ + let (mut ctx, id, out, _err, _tmp) = setup_ctx(vec![ ConversationEvent::new(TurnStart, ts(0, 0, 0)), ConversationEvent::new(ChatRequest::from("First question"), ts(0, 0, 1)), ConversationEvent::new(ChatResponse::message("First answer.\n\n"), ts(0, 0, 2)), @@ -1108,7 +1108,7 @@ fn turn_prints_specific_turn() { #[test] fn turn_out_of_range_errors() { - let (mut ctx, id, _out, _err, _rt) = setup_ctx(vec![ + let (mut ctx, id, _out, _err, _tmp) = setup_ctx(vec![ ConversationEvent::new(TurnStart, ts(0, 0, 0)), ConversationEvent::new(ChatRequest::from("Only turn"), ts(0, 0, 1)), ]); @@ -1127,7 +1127,7 @@ fn turn_out_of_range_errors() { #[test] fn turn_zero_errors() { - let (mut ctx, id, _out, _err, _rt) = setup_ctx(vec![ + let (mut ctx, id, _out, _err, _tmp) = setup_ctx(vec![ ConversationEvent::new(TurnStart, ts(0, 0, 0)), ConversationEvent::new(ChatRequest::from("Only turn"), ts(0, 0, 1)), ]); @@ -1149,7 +1149,7 @@ fn style_brief_hides_reasoning_and_tool_details() { let mut config = AppConfig::new_test(); config.style.reasoning.display = ReasoningDisplayConfig::Full; - let (mut ctx, id, out, err, _rt) = setup_ctx_with_config(config, vec![ + let (mut ctx, id, out, err, _tmp) = setup_ctx_with_config(config, vec![ ConversationEvent::new(TurnStart, ts(0, 0, 0)), ConversationEvent::new(ChatRequest::from("Explain Rust"), ts(0, 0, 1)), ConversationEvent::new( @@ -1226,7 +1226,7 @@ fn style_chat_hides_reasoning_and_tool_calls() { let mut config = AppConfig::new_test(); config.style.reasoning.display = ReasoningDisplayConfig::Full; - let (mut ctx, id, out, err, _rt) = setup_ctx_with_config(config, vec![ + let (mut ctx, id, out, err, _tmp) = setup_ctx_with_config(config, vec![ ConversationEvent::new(TurnStart, ts(0, 0, 0)), ConversationEvent::new(ChatRequest::from("Explain Rust"), ts(0, 0, 1)), ConversationEvent::new( @@ -1305,7 +1305,7 @@ fn style_user_shows_only_user_messages() { let mut config = AppConfig::new_test(); config.style.reasoning.display = ReasoningDisplayConfig::Full; - let (mut ctx, id, out, err, _rt) = setup_ctx_with_config(config, vec![ + let (mut ctx, id, out, err, _tmp) = setup_ctx_with_config(config, vec![ ConversationEvent::new(TurnStart, ts(0, 0, 0)), ConversationEvent::new(ChatRequest::from("Explain Rust"), ts(0, 0, 1)), ConversationEvent::new( @@ -1376,7 +1376,7 @@ fn role_header_renders_user_label_from_author() { let mut req = ChatRequest::from("hello"); req.author = Some("alice".into()); - let (mut ctx, id, out, _err, _rt) = setup_ctx(vec![ConversationEvent::new(req, ts(0, 0, 0))]); + let (mut ctx, id, out, _err, _tmp) = setup_ctx(vec![ConversationEvent::new(req, ts(0, 0, 0))]); let print = Print { target: PositionalIds::from_targets(vec![ConversationTarget::Id(id)]), @@ -1398,7 +1398,7 @@ fn role_header_renders_user_label_from_author() { #[test] fn role_header_falls_back_to_user_label_without_author() { - let (mut ctx, id, out, _err, _rt) = setup_ctx(vec![ConversationEvent::new( + let (mut ctx, id, out, _err, _tmp) = setup_ctx(vec![ConversationEvent::new( ChatRequest::from("hello"), ts(0, 0, 0), )]); @@ -1423,7 +1423,7 @@ fn role_header_falls_back_to_user_label_without_author() { #[test] fn role_header_renders_assistant_label_with_model_suffix() { - let (mut ctx, id, out, _err, _rt) = setup_ctx(vec![ + let (mut ctx, id, out, _err, _tmp) = setup_ctx(vec![ ConversationEvent::new(ChatRequest::from("hello"), ts(0, 0, 0)), ConversationEvent::new(ChatResponse::message("hi"), ts(0, 0, 1)), ]); @@ -1449,8 +1449,8 @@ fn role_header_renders_assistant_label_with_model_suffix() { /// A two-turn conversation whose model changed between the turns: turn 1 ran on /// `anthropic/test` (the conversation's base config), turn 2 on /// `openai/gpt-4o`. -fn two_turns_with_model_switch() -> (Ctx, ConversationId, SharedBuffer, SharedBuffer, Runtime) { - let (ctx, id, out, err, rt) = setup_ctx(vec![ +fn two_turns_with_model_switch() -> (Ctx, ConversationId, SharedBuffer, SharedBuffer, Utf8TempDir) { + let (ctx, id, out, err, tmp) = setup_ctx(vec![ ConversationEvent::new(TurnStart, ts(0, 0, 0)), ConversationEvent::new(ChatRequest::from("first"), ts(0, 0, 1)), ConversationEvent::new(ChatResponse::message("one"), ts(0, 0, 2)), @@ -1470,7 +1470,7 @@ fn two_turns_with_model_switch() -> (Ctx, ConversationId, SharedBuffer, SharedBu ]); }); - (ctx, id, out, err, rt) + (ctx, id, out, err, tmp) } fn print_with_style(style: Option, id: ConversationId) -> Print { @@ -1485,7 +1485,7 @@ fn print_with_style(style: Option, id: ConversationId) -> Print { #[test] fn assistant_header_names_the_model_each_turn_ran_on() { - let (mut ctx, id, out, _err, _rt) = two_turns_with_model_switch(); + let (mut ctx, id, out, _err, _tmp) = two_turns_with_model_switch(); let h = ctx.workspace.acquire_conversation(&id).unwrap(); print_with_style(None, id).run(&mut ctx, &[h]).unwrap(); @@ -1503,7 +1503,7 @@ fn assistant_header_names_the_model_each_turn_ran_on() { /// which would label every turn with the model configured right now. #[test] fn style_preset_keeps_the_per_turn_model_in_the_assistant_header() { - let (mut ctx, id, out, _err, _rt) = two_turns_with_model_switch(); + let (mut ctx, id, out, _err, _tmp) = two_turns_with_model_switch(); let h = ctx.workspace.acquire_conversation(&id).unwrap(); print_with_style(Some(PrintStyle::Full), id) @@ -1522,7 +1522,7 @@ fn style_preset_keeps_the_per_turn_model_in_the_assistant_header() { /// workspace config, so both turns carry that model. #[test] fn current_config_labels_every_turn_with_the_workspace_model() { - let (mut ctx, id, out, _err, _rt) = two_turns_with_model_switch(); + let (mut ctx, id, out, _err, _tmp) = two_turns_with_model_switch(); let mut print = print_with_style(None, id); print.current_config = true; @@ -1541,7 +1541,7 @@ fn current_config_labels_every_turn_with_the_workspace_model() { #[test] fn role_header_assistant_appears_once_per_turn() { - let (mut ctx, id, out, _err, _rt) = setup_ctx(vec![ + let (mut ctx, id, out, _err, _tmp) = setup_ctx(vec![ ConversationEvent::new(ChatRequest::from("hello"), ts(0, 0, 0)), ConversationEvent::new(ChatResponse::message("first chunk"), ts(0, 0, 1)), ConversationEvent::new(ChatResponse::message("second chunk"), ts(0, 0, 2)), @@ -1570,7 +1570,7 @@ fn role_header_assistant_appears_once_per_turn() { fn role_header_assistant_emitted_before_first_tool_call() { // The assistant's first event of the turn is a tool call (no message // first). The header should still appear before it. - let (mut ctx, id, out, _err, _rt) = setup_ctx(vec![ + let (mut ctx, id, out, _err, _tmp) = setup_ctx(vec![ ConversationEvent::new(ChatRequest::from("do it"), ts(0, 0, 0)), ConversationEvent::new( ToolCallRequest { @@ -1615,7 +1615,7 @@ fn role_header_does_not_emit_plain_hr_separator() { // Regression: the old renderer emitted a `---` HR after the user // message. The labeled-header design replaces that. Make sure no plain // `---` line shows up between user and assistant content. - let (mut ctx, id, out, _err, _rt) = setup_ctx(vec![ + let (mut ctx, id, out, _err, _tmp) = setup_ctx(vec![ ConversationEvent::new(ChatRequest::from("q"), ts(0, 0, 0)), ConversationEvent::new(ChatResponse::message("a"), ts(0, 0, 1)), ]); @@ -1653,7 +1653,7 @@ fn style_chat_separates_messages_across_hidden_reasoning() { // message chunks must not be glued together into the same markdown // paragraph — they should be separated by a blank line so the // transcript reads naturally. - let (mut ctx, id, out, _err, _rt) = setup_ctx(vec![ + let (mut ctx, id, out, _err, _tmp) = setup_ctx(vec![ ConversationEvent::new(TurnStart, ts(0, 0, 0)), ConversationEvent::new(ChatRequest::from("Explain"), ts(0, 0, 1)), ConversationEvent::new( @@ -1696,7 +1696,7 @@ fn style_chat_separates_messages_across_hidden_reasoning() { #[test] fn style_chat_separates_messages_across_hidden_tool_call() { // Same concern as above, but with a tool call as the hidden boundary. - let (mut ctx, id, out, _err, _rt) = setup_ctx(vec![ + let (mut ctx, id, out, _err, _tmp) = setup_ctx(vec![ ConversationEvent::new(TurnStart, ts(0, 0, 0)), ConversationEvent::new(ChatRequest::from("Check it"), ts(0, 0, 1)), ConversationEvent::new( @@ -1754,7 +1754,7 @@ fn style_full_shows_reasoning_and_untruncated_results() { // Start with reasoning hidden and results truncated to 1 line. config.style.reasoning.display = ReasoningDisplayConfig::Hidden; - let (mut ctx, id, out, err, _rt) = setup_ctx_with_config(config, vec![ + let (mut ctx, id, out, err, _tmp) = setup_ctx_with_config(config, vec![ ConversationEvent::new(TurnStart, ts(0, 0, 0)), ConversationEvent::new(ChatRequest::from("Check the file"), ts(0, 0, 1)), ConversationEvent::new( @@ -1828,7 +1828,7 @@ fn style_full_shows_reasoning_and_untruncated_results() { /// `display` to `full`. #[test] fn replay_shades_tool_chrome_after_reasoning() { - let (mut ctx, id, _out, err, _rt) = setup_ctx(vec![ + let (mut ctx, id, _out, err, _tmp) = setup_ctx(vec![ ConversationEvent::new(TurnStart, ts(0, 0, 0)), ConversationEvent::new(ChatRequest::from("read it"), ts(0, 0, 1)), ConversationEvent::new( @@ -1877,7 +1877,7 @@ fn replay_shades_tool_chrome_after_reasoning() { fn replay_does_not_shade_tool_chrome_when_extension_disabled() { let mut config = AppConfig::new_test(); config.style.reasoning.extend_across_tool_calls = false; - let (mut ctx, id, _out, err, _rt) = setup_ctx_with_config(config, vec![ + let (mut ctx, id, _out, err, _tmp) = setup_ctx_with_config(config, vec![ ConversationEvent::new(TurnStart, ts(0, 0, 0)), ConversationEvent::new(ChatRequest::from("read it"), ts(0, 0, 1)), ConversationEvent::new( @@ -1927,7 +1927,7 @@ fn replay_does_not_shade_tool_chrome_when_extension_disabled() { fn replay_suppresses_tool_chrome_when_show_disabled() { let mut config = AppConfig::new_test(); config.style.tool_call.show = false; - let (mut ctx, id, _out, err, _rt) = setup_ctx_with_config(config, vec![ + let (mut ctx, id, _out, err, _tmp) = setup_ctx_with_config(config, vec![ ConversationEvent::new(TurnStart, ts(0, 0, 0)), ConversationEvent::new(ChatRequest::from("read it"), ts(0, 0, 1)), ConversationEvent::new( @@ -1987,7 +1987,7 @@ fn replay_keeps_the_gap_after_a_result_when_reasoning_renders_nothing() { config.conversation.tools.defaults.style.inline_results = InlineResults::Full; config.conversation.tools.defaults.style.results_file_link = LinkStyle::Off; - let (mut ctx, id, _out, err, _rt) = setup_ctx_with_config(config, vec![ + let (mut ctx, id, _out, err, _tmp) = setup_ctx_with_config(config, vec![ ConversationEvent::new(TurnStart, ts(0, 0, 0)), ConversationEvent::new(ChatRequest::from("read it"), ts(0, 0, 1)), ConversationEvent::new( @@ -2046,7 +2046,7 @@ fn replay_keeps_the_gap_after_a_result_when_reasoning_renders_nothing() { /// render that tool's chrome unshaded. #[test] fn replay_does_not_leak_reasoning_region_across_turns() { - let (mut ctx, id, _out, err, _rt) = setup_ctx(vec![ + let (mut ctx, id, _out, err, _tmp) = setup_ctx(vec![ ConversationEvent::new(TurnStart, ts(0, 0, 0)), ConversationEvent::new(ChatRequest::from("think about it"), ts(0, 0, 1)), ConversationEvent::new(ChatResponse::reasoning("Deep thought.\n\n"), ts(0, 0, 2)), diff --git a/crates/jp_cli/src/cmd/conversation/summarize.rs b/crates/jp_cli/src/cmd/conversation/summarize.rs index 4a519c7d5..dd0ff8455 100644 --- a/crates/jp_cli/src/cmd/conversation/summarize.rs +++ b/crates/jp_cli/src/cmd/conversation/summarize.rs @@ -193,7 +193,7 @@ enum StreamOutcome { /// Reduce one summarizer stream to its outcome. /// /// Only a stream that both finishes with [`FinishReason::Completed`] and -/// carries message text yields a summary. +/// carries non-blank message text yields a summary. /// Every other terminal reason is unusable even when text was streamed first: a /// truncated or declined response would otherwise be stored as the summary and /// replace the turns it was meant to stand in for, silently dropping whatever @@ -240,6 +240,8 @@ fn summarize_events(events: Vec) -> StreamOutcome { }) .collect::(); + // Whitespace-only output is as unusable as no output: storing it would + // replace the turns it stands for with nothing. if matches!(finish, Some(FinishReason::Completed)) && !summary.is_empty() { return StreamOutcome::Summary(summary); } diff --git a/crates/jp_cli/src/cmd/conversation/summarize_tests.rs b/crates/jp_cli/src/cmd/conversation/summarize_tests.rs index 6a8271266..48596e147 100644 --- a/crates/jp_cli/src/cmd/conversation/summarize_tests.rs +++ b/crates/jp_cli/src/cmd/conversation/summarize_tests.rs @@ -216,6 +216,19 @@ fn completed_stream_without_text_is_unusable() { ); } +#[test] +fn completed_stream_with_only_whitespace_is_unusable() { + // A provider can complete with nothing but a newline. Storing that would + // replace the turns it stands for with a blank summary, in every future + // request, without telling anyone. + let events = stream_with_text(" \n", FinishReason::Completed); + + assert_eq!( + summarize_events(events), + StreamOutcome::Unusable("the model returned an empty response".to_owned()) + ); +} + #[test] fn truncated_stream_is_unusable_even_though_it_produced_text() { // A max-tokens stream normally carries partial text. Returning it would From 3de8d90e3b2e3ee23a289fc5bee12c6c38e6d1b9 Mon Sep 17 00:00:00 2001 From: Jean Mertz Date: Tue, 4 Aug 2026 20:20:45 +0200 Subject: [PATCH 4/5] fixup! feat(cli, conversation): Support verbatim summaries in compaction Signed-off-by: Jean Mertz --- crates/jp_cli/src/cmd/conversation/compact_tests.rs | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/crates/jp_cli/src/cmd/conversation/compact_tests.rs b/crates/jp_cli/src/cmd/conversation/compact_tests.rs index cdd10877b..c115a166a 100644 --- a/crates/jp_cli/src/cmd/conversation/compact_tests.rs +++ b/crates/jp_cli/src/cmd/conversation/compact_tests.rs @@ -24,8 +24,11 @@ use super::{ Bound, Compact, IntoPartialAppConfig as _, TimelineSegment, build_compaction_events, existing_segments, resolve_reset_index, segments_for_compactions, timeline_lines, }; -use crate::cmd::{conversation_id::ConversationIds as _, target::ConversationTarget}; -use crate::{Globals, ctx::Ctx}; +use crate::{ + Globals, + cmd::{conversation_id::ConversationIds as _, target::ConversationTarget}, + ctx::Ctx, +}; /// Parse a `Compact` from `jp conversation compact ` for flag tests. fn parse_compact(args: &[&str]) -> Compact { From 97f3202413d4ea2cf155ec6acea29375ba49df77 Mon Sep 17 00:00:00 2001 From: Jean Mertz Date: Tue, 4 Aug 2026 20:24:28 +0200 Subject: [PATCH 5/5] fixup! feat(cli, conversation): Support verbatim summaries in compaction Signed-off-by: Jean Mertz --- docs/.vitepress/rfd-summaries.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/.vitepress/rfd-summaries.json b/docs/.vitepress/rfd-summaries.json index 1451601bd..4e71ff3fa 100644 --- a/docs/.vitepress/rfd-summaries.json +++ b/docs/.vitepress/rfd-summaries.json @@ -248,7 +248,7 @@ "summary": "Extend config wizard with frecency-based field ordering using CLI usage tracking data." }, "064-non-destructive-conversation-compaction.md": { - "hash": "197c94f58cac2161fb16f20248f1b7e8847ad859974d2b1e0a26dcb97466a5a3", + "hash": "3bb648ee7a3b6d9319996ee1d3cb772a47305d6c2d0b1f82fb490229e8675455", "summary": "Non-destructive conversation compaction through overlay events that project reduced views without mutating stored data." }, "065-typed-resource-model-for-attachments.md": {