Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
120 changes: 97 additions & 23 deletions crates/jp_cli/src/cmd/conversation/compact.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<Option<usize>>,

/// Compact using an inline DSL rule.
///
Expand Down Expand Up @@ -266,6 +272,48 @@ fn parse_tool_calls_mode(s: &str) -> Result<ToolCallsMode, String> {
})
}

/// Parse the `--reset=INDEX` value: a 1-based position among the conversation's
/// compaction events.
fn parse_compaction_index(s: &str) -> Result<usize, String> {
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
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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<usize>,
) -> 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
Expand Down
51 changes: 50 additions & 1 deletion crates/jp_cli/src/cmd/conversation/compact_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 <args>` for flag tests.
fn parse_compact(args: &[&str]) -> Compact {
Expand Down Expand Up @@ -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()
}
Expand Down
20 changes: 20 additions & 0 deletions crates/jp_conversation/src/stream.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<Compaction> {
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<Item = &Compaction> {
self.events.iter().filter_map(|e| match e {
Expand Down
28 changes: 28 additions & 0 deletions crates/jp_conversation/src/stream_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
18 changes: 11 additions & 7 deletions crates/jp_term/src/table.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<DetailItem>),
}
Expand Down Expand Up @@ -224,19 +224,23 @@ pub fn details(title: Option<&str>, rows: Vec<DetailRow>) -> 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::<Vec<_>>()
.join("\n");
format!("\n{bullets}")
format!("\n{numbered}")
}
};

Expand Down
47 changes: 36 additions & 11 deletions crates/jp_term/src/table_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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"));
Expand Down Expand Up @@ -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]
Expand Down Expand Up @@ -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"
);

Expand Down
Loading
Loading