diff --git a/crates/jp_cli/src/cmd/conversation/compact.rs b/crates/jp_cli/src/cmd/conversation/compact.rs index a6c43b5d..d8793b9f 100644 --- a/crates/jp_cli/src/cmd/conversation/compact.rs +++ b/crates/jp_cli/src/cmd/conversation/compact.rs @@ -108,20 +108,26 @@ pub(crate) struct Compact { #[arg(long)] dry_run: bool, - /// Remove all compaction events from the stream. + /// Remove compaction events from the stream. /// - /// Restores the raw event history so the LLM sees all original events. + /// Without a value, removes every compaction event, restoring the raw event + /// history so the LLM sees all original events. + /// With a value (`--reset=2`), removes only that compaction event, numbered + /// as `jp conversation show` lists them. /// Mutually exclusive with the policy, range, and DSL flags: `--reset` /// undoes compaction, it does not re-compact in the same invocation. /// Composes with `--dry-run` to preview the removal. #[arg( long, + value_name = "INDEX", + require_equals = true, + value_parser = parse_compaction_index, conflicts_with_all = [ "keep_first", "keep_last", "from", "to", "first", "last", "turn", "reasoning", "tools", "summarize", "compact", "model", ], )] - reset: bool, + reset: Option>, /// Compact using an inline DSL rule. /// @@ -266,6 +272,48 @@ fn parse_tool_calls_mode(s: &str) -> Result { }) } +/// Parse the `--reset=INDEX` value: a 1-based position among the conversation's +/// compaction events. +fn parse_compaction_index(s: &str) -> Result { + match s.parse() { + Ok(0) => Err("compaction indices are 1-based; `0` is not a valid index".to_owned()), + Ok(n) => Ok(n), + Err(_) => Err(format!("invalid compaction index '{s}'")), + } +} + +/// Look up the compaction event a 1-based `--reset=INDEX` addresses. +/// +/// Returns its 0-based position among the stream's compaction events, plus a +/// `turns X..Y` label for the removal message (turn numbers 1-based, as the +/// user sees them elsewhere). +/// An index naming no event is an error rather than a silent no-op, matching +/// how `--turn` treats an out-of-range turn. +fn resolve_reset_index( + events: &ConversationStream, + index: usize, +) -> Result<(usize, String), String> { + let Some(position) = index.checked_sub(1) else { + return Err("compaction indices are 1-based; `0` is not a valid index".to_owned()); + }; + + let Some(compaction) = events.compactions().nth(position) else { + let count = events.compactions().count(); + return Err(format!( + "compaction {index} out of range (conversation has {count} compaction event(s))" + )); + }; + + Ok(( + position, + format!( + "turns {}..{}", + compaction.from_turn + 1, + compaction.to_turn + 1 + ), + )) +} + /// Resolve the turn range a single rule would compact. /// /// `range_stream` is the baseline for resolving bounds, including @@ -659,26 +707,8 @@ impl Compact { let conv = lock.into_mut(); let events_snapshot = conv.events().clone(); - if self.reset { - if self.dry_run { - // Preview only — `--dry-run` must not mutate the conversation. - let count = events_snapshot.compactions().count(); - if count > 0 { - ctx.printer - .println(format!("Would remove {count} compaction event(s).")); - } else { - ctx.printer.println("No compaction events to remove."); - } - } else { - let removed = conv.update_events(ConversationStream::remove_compactions); - if removed > 0 { - ctx.printer - .println(format!("Removed {removed} compaction event(s).")); - } else { - ctx.printer.println("No compaction events to remove."); - } - } - return Ok(()); + if let Some(index) = self.reset { + return self.run_reset(ctx, &conv, &events_snapshot, index); } // `--last 0` explicitly selects no turns. @@ -742,6 +772,50 @@ impl Compact { Ok(()) } + /// Handle `--reset[=INDEX]`: remove every compaction event, or just the one + /// at `index` (1-based, in stream order). + /// + /// Under `--dry-run` nothing is mutated and the message describes what + /// would be removed. + /// An `index` past the last compaction event is an error rather than a + /// silent no-op (matching `--turn`). + fn run_reset( + &self, + ctx: &Ctx, + conv: &ConversationMut, + events: &ConversationStream, + index: Option, + ) -> Output { + let Some(index) = index else { + let count = if self.dry_run { + events.compactions().count() + } else { + conv.update_events(ConversationStream::remove_compactions) + }; + + ctx.printer.println(match (count, self.dry_run) { + (0, _) => "No compaction events to remove.".to_owned(), + (count, true) => format!("Would remove {count} compaction event(s)."), + (count, false) => format!("Removed {count} compaction event(s)."), + }); + return Ok(()); + }; + + let (position, range) = resolve_reset_index(events, index)?; + + if self.dry_run { + ctx.printer + .println(format!("Would remove compaction {index} ({range}).")); + return Ok(()); + } + + conv.update_events(|stream| stream.remove_compaction(position)); + ctx.printer + .println(format!("Removed compaction {index} ({range}).")); + + Ok(()) + } + /// Preview the compaction timeline without mutating the conversation. /// /// Resolves the same per-rule ranges as the real run (minus the summarizer diff --git a/crates/jp_cli/src/cmd/conversation/compact_tests.rs b/crates/jp_cli/src/cmd/conversation/compact_tests.rs index 0602f7ec..6611b64f 100644 --- a/crates/jp_cli/src/cmd/conversation/compact_tests.rs +++ b/crates/jp_cli/src/cmd/conversation/compact_tests.rs @@ -18,8 +18,9 @@ use serde_json::{Map, Value}; use super::{ Bound, Compact, IntoPartialAppConfig as _, TimelineSegment, build_compaction_events, - existing_segments, segments_for_compactions, timeline_lines, + existing_segments, resolve_reset_index, segments_for_compactions, timeline_lines, }; +use crate::cmd::{conversation_id::ConversationIds as _, target::ConversationTarget}; /// Parse a `Compact` from `jp conversation compact ` for flag tests. fn parse_compact(args: &[&str]) -> Compact { @@ -232,6 +233,54 @@ fn reset_conflicts_with_selection_flags() { assert!(TestCli::try_parse_from(["compact", "--reset", "--dry-run"]).is_ok()); } +#[test] +fn reset_takes_an_optional_compaction_index() { + #[derive(clap::Parser)] + struct TestCli { + #[command(flatten)] + compact: Compact, + } + + assert_eq!(parse_compact(&["--reset"]).reset, Some(None)); + assert_eq!(parse_compact(&["--reset=2"]).reset, Some(Some(2))); + assert_eq!(parse_compact(&[]).reset, None); + + // The index requires `=`, so a bare `--reset` followed by a conversation + // target still targets the conversation instead of swallowing it as the + // index. + let compact = parse_compact(&["--reset", "latest"]); + assert_eq!(compact.reset, Some(None)); + assert_eq!(compact.target.ids(), [ConversationTarget::Latest]); + + // Indices are 1-based, so `0` names nothing. + assert!(TestCli::try_parse_from(["compact", "--reset=0"]).is_err()); + assert!(TestCli::try_parse_from(["compact", "--reset=x"]).is_err()); +} + +#[test] +fn reset_index_addresses_compactions_in_stream_order() { + let mut stream = ConversationStream::new_test(); + for t in 0..6 { + stream.start_turn(format!("turn {t}")); + } + stream.add_compaction(Compaction::new(0, 1)); + stream.add_compaction(Compaction::new(2, 4)); + + // The label carries 1-based turn numbers, matching `jp conversation show`. + assert_eq!( + resolve_reset_index(&stream, 1), + Ok((0, "turns 1..2".to_owned())) + ); + assert_eq!( + resolve_reset_index(&stream, 2), + Ok((1, "turns 3..5".to_owned())) + ); + assert_eq!( + resolve_reset_index(&stream, 3), + Err("compaction 3 out of range (conversation has 2 compaction event(s))".to_owned()) + ); +} + fn runtime() -> tokio::runtime::Runtime { tokio::runtime::Runtime::new().unwrap() } diff --git a/crates/jp_conversation/src/stream.rs b/crates/jp_conversation/src/stream.rs index 5c2794b3..beec86bc 100644 --- a/crates/jp_conversation/src/stream.rs +++ b/crates/jp_conversation/src/stream.rs @@ -423,6 +423,26 @@ impl ConversationStream { before - self.events.len() } + /// Remove a single compaction event, addressed by its 0-based position + /// among the compaction events in the stream. + /// + /// Returns the removed event, or `None` when the stream holds fewer + /// compaction events than that (in which case the stream is unchanged). + pub fn remove_compaction(&mut self, index: usize) -> Option { + let position = self + .events + .iter() + .enumerate() + .filter(|(_, event)| matches!(event, InternalEvent::Compaction(_))) + .map(|(position, _)| position) + .nth(index)?; + + match self.events.remove(position) { + InternalEvent::Compaction(compaction) => Some(compaction), + _ => unreachable!("position points at a compaction event"), + } + } + /// Returns an iterator over the [`Compaction`] events in the stream. pub fn compactions(&self) -> impl Iterator { self.events.iter().filter_map(|e| match e { diff --git a/crates/jp_conversation/src/stream_tests.rs b/crates/jp_conversation/src/stream_tests.rs index 42b39a44..eb2699d6 100644 --- a/crates/jp_conversation/src/stream_tests.rs +++ b/crates/jp_conversation/src/stream_tests.rs @@ -1040,6 +1040,34 @@ fn test_compaction_not_counted_by_is_empty() { ); } +#[test] +fn test_remove_compaction_by_index() { + let mut stream = ConversationStream::new_test(); + stream.start_turn(ChatRequest::from("hello")); + stream.add_compaction(make_compaction(0, 1)); + stream.add_compaction(make_compaction(2, 3)); + stream.add_compaction(make_compaction(4, 5)); + + let removed = stream.remove_compaction(1).expect("second compaction"); + + assert_eq!((removed.from_turn, removed.to_turn), (2, 3)); + let remaining: Vec<_> = stream + .compactions() + .map(|c| (c.from_turn, c.to_turn)) + .collect(); + assert_eq!(remaining, vec![(0, 1), (4, 5)]); +} + +#[test] +fn test_remove_compaction_out_of_range_is_a_no_op() { + let mut stream = ConversationStream::new_test(); + stream.start_turn(ChatRequest::from("hello")); + stream.add_compaction(make_compaction(0, 1)); + + assert!(stream.remove_compaction(1).is_none()); + assert_eq!(stream.compactions().count(), 1); +} + #[test] fn test_retain_removing_events_drops_compactions() { let mut stream = ConversationStream::new_test(); diff --git a/crates/jp_term/src/table.rs b/crates/jp_term/src/table.rs index 3f0e70b9..58b0fa9b 100644 --- a/crates/jp_term/src/table.rs +++ b/crates/jp_term/src/table.rs @@ -9,7 +9,7 @@ pub enum DetailValue { /// A single value. Scalar(String), - /// A list of items: a bulleted multi-line cell in the pretty view, one row + /// A list of items: a numbered multi-line cell in the pretty view, one row /// per item in markdown, and a JSON array in the JSON views. List(Vec), } @@ -224,19 +224,23 @@ pub fn details(title: Option<&str>, rows: Vec) -> String { /// Build a pretty (borderless table) row from a detail row. /// -/// A list value renders with the label on its own line and the items bulleted -/// beneath it (the leading newline pushes the items below the label, indented -/// into the value column). +/// A list value renders with the label on its own line and the items numbered +/// from 1 beneath it (the leading newline pushes the items below the label, +/// indented into the value column). +/// The numbers are right-aligned so the item text stays in one column past the +/// tenth item. fn detail_pretty_row(row: DetailRow) -> Row { let value = match row.value { DetailValue::Scalar(s) => s, DetailValue::List(items) => { - let bullets = items + let width = items.len().to_string().len(); + let numbered = items .into_iter() - .map(|item| format!("- {}", item.text)) + .enumerate() + .map(|(index, item)| format!("{:>width$}. {}", index + 1, item.text)) .collect::>() .join("\n"); - format!("\n{bullets}") + format!("\n{numbered}") } }; diff --git a/crates/jp_term/src/table_tests.rs b/crates/jp_term/src/table_tests.rs index 5979a175..1b2e5565 100644 --- a/crates/jp_term/src/table_tests.rs +++ b/crates/jp_term/src/table_tests.rs @@ -2,6 +2,27 @@ use comfy_table::Cell; use super::*; +/// The pretty details view of a two-item list, exactly as it reaches the +/// terminal: the label on the first line, items numbered and indented into the +/// value column. +const PRETTY_LIST_TWO_ITEMS: &str = " Attachments + 1. a://x + 2. b://y"; + +/// The same view for ten items, where the single-digit numbers are padded to +/// line up with `10.`. +const PRETTY_LIST_TEN_ITEMS: &str = " Items + 1. item-1 + 2. item-2 + 3. item-3 + 4. item-4 + 5. item-5 + 6. item-6 + 7. item-7 + 8. item-8 + 9. item-9 + 10. item-10"; + fn header() -> Row { let mut row = Row::new(); row.add_cell(Cell::new("Name")); @@ -66,21 +87,25 @@ fn markdown_details_no_title() { } #[test] -fn pretty_details_list_puts_label_above_bulleted_items() { +fn pretty_details_list_puts_label_above_numbered_items() { let output = details(None, vec![DetailRow::list("Attachments", vec![ DetailItem::plain("a://x"), DetailItem::plain("b://y"), ])]); - let lines: Vec<&str> = output.lines().collect(); - // Label sits on its own line; items are bulleted beneath it. - assert!( - lines[0].trim_end().ends_with("Attachments"), - "got: {output}" - ); - assert!(!lines[0].contains("a://x"), "got: {output}"); - assert!(output.contains("- a://x"), "got: {output}"); - assert!(output.contains("- b://y"), "got: {output}"); + assert_eq!(output, PRETTY_LIST_TWO_ITEMS); +} + +#[test] +fn pretty_details_list_right_aligns_numbers_past_the_tenth_item() { + // Single-digit numbers are padded so every item's text starts in the same + // column. + let items = (1..=10) + .map(|n| DetailItem::plain(format!("item-{n}"))) + .collect(); + let output = details(None, vec![DetailRow::list("Items", items)]); + + assert_eq!(output, PRETTY_LIST_TEN_ITEMS); } #[test] @@ -122,7 +147,7 @@ fn list_item_text_and_json_forms_can_differ() { // Pretty uses the text form. assert!( - details(None, rows.clone()).contains("- cmd (Desc): cmd://x"), + details(None, rows.clone()).contains("1. cmd (Desc): cmd://x"), "text form should drive the pretty view" ); diff --git a/docs/.vitepress/rfd-summaries.json b/docs/.vitepress/rfd-summaries.json index de5b7904..127c0fd4 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": "e1407202e2c53a7168b2b92ca4f0528644848cb8ca40062fe9883557be713b9a", + "hash": "04546fb7f0e0b853c24867506721f7f9f6fa0a90903cde85729c80dc26d703bc", "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/rfd/064-non-destructive-conversation-compaction.md b/docs/rfd/064-non-destructive-conversation-compaction.md index 4b077dec..428666a5 100644 --- a/docs/rfd/064-non-destructive-conversation-compaction.md +++ b/docs/rfd/064-non-destructive-conversation-compaction.md @@ -156,6 +156,12 @@ The projection layer then has nothing to apply, and the LLM sees the original uncompacted events. This is useful for undoing compaction when the result is unsatisfactory. +> [!NOTE] +> `--reset` also takes an optional index: `--reset=2` removes only the second +> compaction event, numbered as `jp conversation show` lists them. +> The value requires `=`, so a bare `--reset` followed by a conversation ID +> still targets that conversation. + #### The `--compact` Flag (DSL) The `--compact` flag is available on `query`, `fork`, and `compact` itself.