diff --git a/crates/jp_cli/src/cmd.rs b/crates/jp_cli/src/cmd.rs index 53704a31..17266e17 100644 --- a/crates/jp_cli/src/cmd.rs +++ b/crates/jp_cli/src/cmd.rs @@ -9,6 +9,7 @@ pub(crate) mod plugin; mod query; pub(crate) mod target; pub(crate) mod time; +pub(crate) mod turn_range; use std::{fmt, num::NonZeroU8}; @@ -22,7 +23,6 @@ use crate::{Ctx, ctx::IntoPartialAppConfig}; #[derive(Debug, clap::Subcommand)] #[command(disable_help_subcommand = true, allow_external_subcommands = true)] -#[expect(clippy::large_enum_variant)] pub(crate) enum Commands { /// Initialize a new workspace. Init(init::Init), diff --git a/crates/jp_cli/src/cmd/compact_flag.rs b/crates/jp_cli/src/cmd/compact_flag.rs index d2bdd789..56503eab 100644 --- a/crates/jp_cli/src/cmd/compact_flag.rs +++ b/crates/jp_cli/src/cmd/compact_flag.rs @@ -106,8 +106,9 @@ impl clap::Args for CompactFlag { `r` / `reasoning`: strip reasoning blocks\n- `s` / `summarize`: generate an \ LLM summary\n- `t` / `tools` (or `t=MODE`): strip tool calls; bare strips \ both, or MODE is one of `strip`/`s`, `strip-requests`/`sreq`, \ - `strip-responses`/`sres`, `omit`/`o`\n\nRange: FROM..TO, single number, or \ - .. for all\n\nExamples: s:..-3, r+t, t=sreq:5..-3, r:-20", + `strip-responses`/`sres`, `omit`/`o`\n\nRange: FROM..TO (1-based, inclusive \ + on both ends, so 1..5 is turns 1-5), single number, or .. for \ + all\n\nExamples: s:..-3, r+t, t=sreq:5..-3, r:-20", ) .action(ArgAction::Append) .num_args(0..=1) @@ -165,10 +166,10 @@ pub(crate) struct CompactSpec { pub range: Option, } -/// A parsed DSL range, Python-slice style. +/// A parsed DSL range (`FROM..TO`), inclusive on both ends. /// -/// Each bound is an absolute turn index (positive in the DSL) or a from-end -/// offset (negative). +/// Each bound is a 1-based absolute turn index (positive in the DSL) or a +/// from-end offset (negative). /// `None` means that end is open (the start or the end of the conversation). #[derive(Debug, Clone, PartialEq, Eq)] pub(crate) struct DslRange { @@ -193,9 +194,10 @@ impl CompactSpec { } if let Some(range) = &self.range { - // Open ends map to start / end: `Absolute(0)` is turn 0, `FromEnd(0)` + // Open ends map to the conversation start / end: `Turns(0)` keeps + // nothing at the front (compact from the first turn), `FromEnd(0)` // is the last turn. - rule.keep_first = Some(range.from.clone().unwrap_or(RuleBound::Absolute(0))); + rule.keep_first = Some(range.from.clone().unwrap_or(RuleBound::Turns(0))); rule.keep_last = Some(range.to.clone().unwrap_or(RuleBound::FromEnd(0))); } @@ -263,8 +265,8 @@ impl FromStr for CompactSpec { } } -/// Parse one DSL range bound: a positive integer is an absolute turn index, a -/// negative integer is an offset from the end. +/// Parse one DSL range bound: a positive integer is a 1-based absolute turn +/// index, a negative integer is an offset from the end. fn parse_dsl_bound(s: &str) -> Result { if let Some(rest) = s.strip_prefix('-') { let n = rest @@ -272,14 +274,18 @@ fn parse_dsl_bound(s: &str) -> Result { .map_err(|_| format!("invalid bound '-{rest}'"))?; Ok(RuleBound::FromEnd(n)) } else { - let n = s.parse().map_err(|_| format!("invalid bound '{s}'"))?; + let n: usize = s.parse().map_err(|_| format!("invalid bound '{s}'"))?; + if n == 0 { + return Err("turn indices are 1-based; `0` is not a valid turn".to_owned()); + } Ok(RuleBound::Absolute(n)) } } fn parse_dsl_range(s: &str) -> Result { // Explicit range: FROM..TO (either side may be empty). Both ends are - // Python-slice style: positive = absolute turn, negative = from the end. + // inclusive: a positive bound is a 1-based absolute turn, a negative bound + // counts from the end. if let Some((left, right)) = s.split_once("..") { let from = if left.is_empty() { None diff --git a/crates/jp_cli/src/cmd/compact_flag_tests.rs b/crates/jp_cli/src/cmd/compact_flag_tests.rs index da0038f9..e51bb322 100644 --- a/crates/jp_cli/src/cmd/compact_flag_tests.rs +++ b/crates/jp_cli/src/cmd/compact_flag_tests.rs @@ -156,6 +156,9 @@ fn parse_errors() { assert!("s:abc".parse::().is_err()); // Non-numeric bound assert!("s:5..x".parse::().is_err()); + // Absolute bounds are 1-based; `0` is invalid. + assert!("s:0..".parse::().is_err()); + assert!("s:0..5".parse::().is_err()); // Unknown tool mode assert!("t=nope".parse::().is_err()); // Boolean policies reject values @@ -170,8 +173,9 @@ fn to_partial_rule_with_range() { assert_eq!(rule.reasoning, Some(ReasoningMode::Strip)); assert_eq!(rule.tool_calls, Some(ToolCallsMode::Strip)); assert!(rule.summary.is_none()); - // Open start maps to absolute turn 0; `-3` keeps the last 3. - assert_eq!(rule.keep_first, Some(RuleBound::Absolute(0))); + // Open start maps to keep-first 0 (compact from the first turn); `-3` keeps + // the last 3. + assert_eq!(rule.keep_first, Some(RuleBound::Turns(0))); assert_eq!(rule.keep_last, Some(RuleBound::FromEnd(3))); } diff --git a/crates/jp_cli/src/cmd/conversation/compact.rs b/crates/jp_cli/src/cmd/conversation/compact.rs index f483864c..c0f9f6ef 100644 --- a/crates/jp_cli/src/cmd/conversation/compact.rs +++ b/crates/jp_cli/src/cmd/conversation/compact.rs @@ -1,6 +1,6 @@ -use std::{env, fs, path::PathBuf, str::FromStr as _, time::Duration}; +use std::{env, fs, path::PathBuf}; -use chrono::{DateTime, Utc}; +use chrono::Utc; use jp_config::conversation::compaction::{ CompactionConfig, CompactionRuleConfig, PartialCompactionRuleConfig, PartialSummaryConfig, ReasoningMode, RuleBound, ToolCallsMode, @@ -18,6 +18,7 @@ use crate::{ ConversationLoadRequest, Output, conversation_id::PositionalIds, lock::{LockOutcome, LockRequest, acquire_lock}, + turn_range::{Bound, TurnRange}, }, ctx::Ctx, }; @@ -32,7 +33,7 @@ pub(crate) struct Compact { /// Accepts a turn count (e.g. /// `2`) or a duration (e.g. /// `5h`). - #[arg(long)] + #[arg(long, conflicts_with_all = ["from", "first", "last", "turn"])] keep_first: Option, /// Preserve the last N turns (or turns within a duration). @@ -40,23 +41,18 @@ pub(crate) struct Compact { /// Accepts a turn count (e.g. /// `3`) or a duration (e.g. /// `2h`). - #[arg(long)] + #[arg(long, conflicts_with_all = ["to", "first", "last", "turn"])] keep_last: Option, - /// Start compacting from a specific turn or time. - /// - /// Accepts an absolute turn index, a duration (e.g. - /// `5h`), or `last` to start after the most recent compaction. - /// Overrides `--keep-first`. - #[arg(long, value_parser = parse_bound, conflicts_with = "keep_first")] - from: Option, - - /// Stop compacting at a specific turn or time. + /// Which turns to compact. /// - /// Accepts an absolute turn index or a duration. - /// Overrides `--keep-last`. - #[arg(long, value_parser = parse_bound, conflicts_with = "keep_last")] - to: Option, + /// `--from`/`--to` bound the compacted range directly (overriding + /// `--keep-first`/`--keep-last`); `--first N`/`--last N` compact the first + /// or last N turns; `--turn N` compacts a single turn, or `--turn A..B` an + /// inclusive range (e.g. + /// `1..5` is turns 1-5). + #[command(flatten)] + range: TurnRange, /// Strip reasoning (thinking) blocks from the compacted range. #[arg(short, long, conflicts_with = "compact")] @@ -103,8 +99,8 @@ pub(crate) struct Compact { #[arg( long, conflicts_with_all = [ - "keep_first", "keep_last", "from", "to", "reasoning", "tools", - "summarize", "compact", + "keep_first", "keep_last", "from", "to", "first", "last", "turn", + "reasoning", "tools", "summarize", "compact", ], )] reset: bool, @@ -179,38 +175,6 @@ impl Compact { } } -/// A CLI range bound before time-based resolution. -#[derive(Debug, Clone)] -enum CliRangeBound { - /// Already resolved to a `RangeBound`. - Resolved(RangeBound), - /// Duration ago — needs the stream to find the turn. - Duration(DateTime), -} - -fn parse_bound(s: &str) -> Result { - if s.eq_ignore_ascii_case("last") { - return Ok(CliRangeBound::Resolved(RangeBound::AfterLastCompaction)); - } - - // Negative integer → FromEnd. - if let Some(rest) = s.strip_prefix('-') - && let Ok(n) = rest.parse::() - { - return Ok(CliRangeBound::Resolved(RangeBound::FromEnd(n))); - } - - // Positive integer → Absolute. - if let Ok(n) = s.parse::() { - return Ok(CliRangeBound::Resolved(RangeBound::Absolute(n))); - } - - // Duration string → resolve to DateTime. - humantime::Duration::from_str(s) - .map(|d| CliRangeBound::Duration(Utc::now() - Duration::from(d))) - .map_err(|e| format!("invalid range bound `{s}`: {e}")) -} - fn parse_tool_calls_mode(s: &str) -> Result { s.parse().map_err(|_| { "expected one of: strip (s), strip-requests (sreq), strip-responses (sres), omit (o)" @@ -218,31 +182,13 @@ fn parse_tool_calls_mode(s: &str) -> Result { }) } -/// The resolution of one range bound (`from` or `to`) against a stream. -/// -/// Separates "no bound configured for this side" (use the side's default) from -/// "this bound selects no turns" (so the whole compaction is a no-op), which a -/// plain `Option` conflated — e.g. `keep_last = "30d"` covering -/// the entire conversation must preserve everything, not fall back to the -/// default `keep_last` and compact through the end. -#[derive(Debug, Clone)] -pub(crate) enum Bound { - /// No bound configured; the range defaults to the start (`from`) or end - /// (`to`) of the conversation. - Default, - /// The bound resolves to a concrete `RangeBound`. - At(RangeBound), - /// The bound falls outside the conversation such that nothing is compacted. - Empty, -} - /// Resolve the turn range a single rule would compact. /// /// `range_stream` is the baseline for resolving bounds, including -/// `AfterLastCompaction` (`--from last`): it must be the stream as it existed -/// at the start of the invocation, so every rule resolves "last" against the -/// same compactions and a rule generated earlier in the same invocation doesn't -/// shift the baseline for a later one. +/// `AfterLastCompaction` (`--from last-compaction`): it must be the stream as +/// it existed at the start of the invocation, so every rule resolves it against +/// the same compactions and a rule generated earlier in the same invocation +/// doesn't shift the baseline for a later one. /// /// `overlap_stream` is consulted only to extend summary ranges over partially /// overlapping summaries; it accumulates the compactions generated so far in @@ -341,7 +287,8 @@ pub(crate) async fn build_compaction_events( // Two distinct baselines: // // - Range resolution uses the original `events` for every rule, so - // `AfterLastCompaction` (`--from last` / `keep_first = "last"`) resolves + // `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. @@ -529,14 +476,17 @@ fn timeline_lines(segments: &[TimelineSegment], last_turn: usize, dry_run: bool) }; let count = segment.to - segment.from + 1; + // Stored indices are 0-based; turn numbers shown to the user are 1-based. lines.push(match &segment.label { Some(label) => format!( - "{compacted} turns {}..={} ({count} total, {label}).", - segment.from, segment.to, + "{compacted} turns {}..{} ({count} total, {label}).", + segment.from + 1, + segment.to + 1, ), None => format!( - "{compacted} turns {}..={} ({count} total).", - segment.from, segment.to, + "{compacted} turns {}..{} ({count} total).", + segment.from + 1, + segment.to + 1, ), }); @@ -553,10 +503,11 @@ fn timeline_lines(segments: &[TimelineSegment], last_turn: usize, dry_run: bool) /// Format a single kept line for the inclusive range `[from, to]`. fn kept_line(verb: &str, from: usize, to: usize) -> String { + // Stored indices are 0-based; turn numbers shown to the user are 1-based. if from == to { - format!("{verb} turn {from}.") + format!("{verb} turn {}.", from + 1) } else { - format!("{verb} turns {from}..={to}.") + format!("{verb} turns {}..{}.", from + 1, to + 1) } } @@ -564,7 +515,9 @@ fn kept_line(verb: &str, from: usize, to: usize) -> String { fn keep_first_to_bound(bound: &RuleBound, events: &ConversationStream) -> Bound { match bound { // "Keep first N" means compaction starts at turn N. - RuleBound::Turns(n) | RuleBound::Absolute(n) => Bound::At(RangeBound::Absolute(*n)), + RuleBound::Turns(n) => Bound::At(RangeBound::Absolute(*n)), + // `Absolute` is the 1-based user value; the stream is 0-based. + RuleBound::Absolute(n) => Bound::At(RangeBound::Absolute(n.saturating_sub(1))), RuleBound::FromEnd(n) => Bound::At(RangeBound::FromEnd(*n)), RuleBound::Duration(d) => { // Preserve the opening `d` window: start compacting at the first @@ -590,7 +543,8 @@ fn keep_last_to_bound(bound: &RuleBound, events: &ConversationStream) -> Bound { match bound { // "Keep last N" means compaction stops N turns before the end. RuleBound::Turns(n) | RuleBound::FromEnd(n) => Bound::At(RangeBound::FromEnd(*n)), - RuleBound::Absolute(n) => Bound::At(RangeBound::Absolute(*n)), + // `Absolute` is the 1-based user value; the stream is 0-based. + RuleBound::Absolute(n) => Bound::At(RangeBound::Absolute(n.saturating_sub(1))), RuleBound::Duration(d) => { // Compact turns older than the last `d` window. A window covering // the whole conversation preserves everything → nothing to compact. @@ -682,6 +636,19 @@ impl Compact { return Ok(()); } + // `--last 0` explicitly selects no turns. + if self.range.is_empty() { + ctx.printer.println("Nothing to compact."); + return Ok(()); + } + + // `--turn` names specific turns; an out-of-range endpoint is an error + // rather than an empty/clamped range (matching `print`). + let count = events_snapshot.turn_count(); + if let Some(n) = self.range.turn_out_of_range(count) { + return Err(format!("turn {n} out of range (conversation has {count} turns)").into()); + } + // The effective rules combine the configured rules with any policy // flags / inline DSL (replace, or append under bare `--compact`). let rules = self @@ -798,66 +765,37 @@ impl Compact { } } - /// Resolve the `from` range override, preferring `--from` over - /// `--keep-first`. - /// Returns [`Bound::Default`] when neither is set. + /// Resolve the `from` range override. + /// + /// The shared selector (`--from`/`--last`/`--turn`) takes precedence; when + /// none is set it falls back to `--keep-first`, and to [`Bound::Default`] + /// when that is also unset. fn resolve_from(&self, events: &ConversationStream) -> Bound { - if let Some(bound) = self.from.as_ref() { - return resolve_cli_from(bound, events); - } - match &self.keep_first { - Some(bound) => keep_first_to_bound(bound, events), - None => Bound::Default, + match self.range.resolve_from(events) { + Bound::Default => match &self.keep_first { + Some(bound) => keep_first_to_bound(bound, events), + None => Bound::Default, + }, + other => other, } } - /// Resolve the `to` range override, preferring `--to` over `--keep-last`. - /// Returns [`Bound::Default`] when neither is set. + /// Resolve the `to` range override. + /// + /// The shared selector (`--to`/`--turn`) takes precedence; when none is set + /// it falls back to `--keep-last`, and to [`Bound::Default`] when that is + /// also unset. fn resolve_to(&self, events: &ConversationStream) -> Bound { - if let Some(bound) = self.to.as_ref() { - return resolve_cli_to(bound, events); - } - match &self.keep_last { - Some(bound) => keep_last_to_bound(bound, events), - None => Bound::Default, + match self.range.resolve_to(events) { + Bound::Default => match &self.keep_last { + Some(bound) => keep_last_to_bound(bound, events), + None => Bound::Default, + }, + other => other, } } } -/// Resolve a `--from ` cutoff: compaction starts at the first turn -/// after the cutoff. -/// No turn after the cutoff (an old conversation) means nothing to compact; a -/// cutoff before the conversation starts compacts from the beginning. -fn resolve_cli_from(bound: &CliRangeBound, events: &ConversationStream) -> Bound { - match bound { - CliRangeBound::Resolved(b) => Bound::At(b.clone()), - CliRangeBound::Duration(dt) => match events.turn_at_time(*dt) { - Some(turn) => { - let from = turn.index() + 1; - if from >= events.turn_count() { - Bound::Empty - } else { - Bound::At(RangeBound::Absolute(from)) - } - } - None => Bound::At(RangeBound::Absolute(0)), - }, - } -} - -/// Resolve a `--to ` cutoff: compaction stops at (and includes) the -/// turn active at the cutoff. -/// A cutoff preceding the conversation means nothing to compact. -fn resolve_cli_to(bound: &CliRangeBound, events: &ConversationStream) -> Bound { - match bound { - CliRangeBound::Resolved(b) => Bound::At(b.clone()), - CliRangeBound::Duration(dt) => match events.turn_at_time(*dt) { - Some(turn) => Bound::At(RangeBound::Absolute(turn.index())), - None => Bound::Empty, - }, - } -} - #[cfg(test)] #[path = "compact_tests.rs"] mod tests; diff --git a/crates/jp_cli/src/cmd/conversation/compact_tests.rs b/crates/jp_cli/src/cmd/conversation/compact_tests.rs index d1541658..5d8e36f8 100644 --- a/crates/jp_cli/src/cmd/conversation/compact_tests.rs +++ b/crates/jp_cli/src/cmd/conversation/compact_tests.rs @@ -189,7 +189,7 @@ fn keep_last_duration_covering_whole_conversation_compacts_nothing() { #[test] fn from_last_resolves_against_original_stream_for_every_rule() { - // `--from last` (AfterLastCompaction) must resolve against the compactions + // `--from last-compaction` (AfterLastCompaction) must resolve against the compactions // present at invocation start for *every* rule, not against a compaction // generated by an earlier rule in the same invocation. With two mechanical // rules and no pre-existing compaction, both resolve from turn 0. @@ -333,6 +333,27 @@ fn summarize_flag_distinguishes_absent_bare_and_valued() { ); } +#[test] +fn turn_out_of_range_is_rejected() { + // `--turn` names specific turns, so an endpoint past the conversation is an + // error rather than an empty (`--turn 100`) or clamped (`--turn ..100`) + // range. With 5 turns, both forms flag turn 100; an in-range turn does not. + assert_eq!( + parse_compact(&["--turn", "100"]).range.turn_out_of_range(5), + Some(100) + ); + assert_eq!( + parse_compact(&["--turn", "..100"]) + .range + .turn_out_of_range(5), + Some(100) + ); + assert_eq!( + parse_compact(&["--turn", "3"]).range.turn_out_of_range(5), + None + ); +} + #[test] fn timeline_keeps_genesis_and_trailing_turns() { // The default `-s` case from a 9-turn conversation (indices 0..=8): @@ -345,9 +366,9 @@ fn timeline_keeps_genesis_and_trailing_turns() { }]; let lines = timeline_lines(&segments, 8, false); assert_eq!(lines, vec![ - "Kept turn 0.".to_owned(), - "Compacted turns 1..=7 (7 total).".to_owned(), - "Kept turn 8.".to_owned(), + "Kept turn 1.".to_owned(), + "Compacted turns 2..8 (7 total).".to_owned(), + "Kept turn 9.".to_owned(), ]); } @@ -370,11 +391,11 @@ fn timeline_interleaves_gaps_between_compactions() { ]; let lines = timeline_lines(&segments, 10, false); assert_eq!(lines, vec![ - "Kept turn 0.".to_owned(), - "Compacted turns 1..=3 (3 total).".to_owned(), - "Kept turns 4..=5.".to_owned(), - "Compacted turns 6..=8 (3 total).".to_owned(), - "Kept turns 9..=10.".to_owned(), + "Kept turn 1.".to_owned(), + "Compacted turns 2..4 (3 total).".to_owned(), + "Kept turns 5..6.".to_owned(), + "Compacted turns 7..9 (3 total).".to_owned(), + "Kept turns 10..11.".to_owned(), ]); } @@ -398,10 +419,10 @@ fn timeline_sorts_by_start_turn_regardless_of_generation_order() { ]; let lines = timeline_lines(&segments, 8, false); assert_eq!(lines, vec![ - "Kept turn 0.".to_owned(), - "Compacted turns 1..=3 (3 total).".to_owned(), - "Kept turns 4..=5.".to_owned(), - "Compacted turns 6..=8 (3 total).".to_owned(), + "Kept turn 1.".to_owned(), + "Compacted turns 2..4 (3 total).".to_owned(), + "Kept turns 5..6.".to_owned(), + "Compacted turns 7..9 (3 total).".to_owned(), ]); } @@ -425,10 +446,10 @@ fn timeline_collapses_overlapping_ranges() { ]; let lines = timeline_lines(&segments, 10, false); assert_eq!(lines, vec![ - "Kept turn 0.".to_owned(), - "Compacted turns 1..=5 (5 total).".to_owned(), - "Compacted turns 3..=8 (6 total).".to_owned(), - "Kept turns 9..=10.".to_owned(), + "Kept turn 1.".to_owned(), + "Compacted turns 2..6 (5 total).".to_owned(), + "Compacted turns 4..9 (6 total).".to_owned(), + "Kept turns 10..11.".to_owned(), ]); } @@ -442,9 +463,9 @@ fn timeline_labels_describe_compaction_type() { }]; let lines = timeline_lines(&segments, 4, false); assert_eq!(lines, vec![ - "Kept turn 0.".to_owned(), - "Compacted turns 1..=3 (3 total, reasoning + tools).".to_owned(), - "Kept turn 4.".to_owned(), + "Kept turn 1.".to_owned(), + "Compacted turns 2..4 (3 total, reasoning + tools).".to_owned(), + "Kept turn 5.".to_owned(), ]); } @@ -458,9 +479,9 @@ fn timeline_dry_run_uses_conditional_verbs() { }]; let lines = timeline_lines(&segments, 4, true); assert_eq!(lines, vec![ - "Would have kept turn 0.".to_owned(), - "Would have compacted turns 1..=3 (3 total).".to_owned(), - "Would have kept turn 4.".to_owned(), + "Would have kept turn 1.".to_owned(), + "Would have compacted turns 2..4 (3 total).".to_owned(), + "Would have kept turn 5.".to_owned(), ]); } @@ -481,8 +502,8 @@ fn segment_label_reflects_mechanical_policies() { #[test] fn timeline_reports_pre_existing_compactions_not_as_kept() { - // Regression: a prior run compacted turns 1..=5; a new run (e.g. `--from - // last`) compacts 6..=8. The already-compacted range must read as compacted, + // Regression: a prior run compacted turns 1..5; a new run (e.g. `--from + // last`) compacts 6..8. The already-compacted range must read as compacted, // not kept, since the projected conversation still compacts it. let mut snapshot = ConversationStream::new_test(); for i in 0..10 { @@ -500,10 +521,10 @@ fn timeline_reports_pre_existing_compactions_not_as_kept() { let lines = timeline_lines(&segments, 9, false); assert_eq!(lines, vec![ - "Kept turn 0.".to_owned(), - "Compacted turns 1..=5 (5 total, already compacted).".to_owned(), - "Compacted turns 6..=8 (3 total).".to_owned(), - "Kept turn 9.".to_owned(), + "Kept turn 1.".to_owned(), + "Compacted turns 2..6 (5 total, already compacted).".to_owned(), + "Compacted turns 7..9 (3 total).".to_owned(), + "Kept turn 10.".to_owned(), ]); } @@ -528,9 +549,9 @@ fn timeline_dry_run_keeps_pre_existing_compactions_factual() { let lines = timeline_lines(&segments, 9, true); assert_eq!(lines, vec![ - "Would have kept turn 0.".to_owned(), - "Compacted turns 1..=5 (5 total, already compacted).".to_owned(), - "Would have compacted turns 6..=8 (3 total).".to_owned(), - "Would have kept turn 9.".to_owned(), + "Would have kept turn 1.".to_owned(), + "Compacted turns 2..6 (5 total, already compacted).".to_owned(), + "Would have compacted turns 7..9 (3 total).".to_owned(), + "Would have kept turn 10.".to_owned(), ]); } diff --git a/crates/jp_cli/src/cmd/conversation/fork.rs b/crates/jp_cli/src/cmd/conversation/fork.rs index 08b26dde..e242009d 100644 --- a/crates/jp_cli/src/cmd/conversation/fork.rs +++ b/crates/jp_cli/src/cmd/conversation/fork.rs @@ -135,8 +135,8 @@ impl Fork { &events_snapshot, &cfg, &rules, - super::compact::Bound::Default, - super::compact::Bound::Default, + crate::cmd::turn_range::Bound::Default, + crate::cmd::turn_range::Bound::Default, // Compaction during a fork is an implicit adjunct; only an // explicit `jp c compact` reports compaction details. None, diff --git a/crates/jp_cli/src/cmd/conversation/print.rs b/crates/jp_cli/src/cmd/conversation/print.rs index a560cdbf..ea07d49a 100644 --- a/crates/jp_cli/src/cmd/conversation/print.rs +++ b/crates/jp_cli/src/cmd/conversation/print.rs @@ -4,11 +4,16 @@ use jp_config::{ }, style::{reasoning::ReasoningDisplayConfig, typewriter::DelayDuration}, }; +use jp_conversation::compaction::resolve_range; use jp_llm::tool::InvocationContext; use jp_workspace::ConversationHandle; use crate::{ - cmd::{ConversationLoadRequest, Output, conversation_id::PositionalIds}, + cmd::{ + ConversationLoadRequest, Output, + conversation_id::PositionalIds, + turn_range::{Bound, TurnRange}, + }, ctx::Ctx, render::{ConfigSource, TurnRenderer}, }; @@ -54,15 +59,11 @@ pub(crate) struct Print { #[command(flatten)] target: PositionalIds, - /// Print only the last N turns. - /// Without a value, prints the last turn. - #[arg(long, num_args = 0..=1, default_missing_value = "1", conflicts_with = "turn")] - last: Option, - - /// Print a specific turn by number (1-based). - /// Stable across new turns. - #[arg(long, conflicts_with = "last")] - turn: Option, + /// Which turns to print. + /// + /// Without any selector, prints the whole conversation. + #[command(flatten)] + range: TurnRange, /// Use the current workspace config instead of the per-turn config. /// @@ -114,19 +115,11 @@ impl Print { } pub(crate) fn run(self, ctx: &mut Ctx, handles: &[ConversationHandle]) -> Output { - let selection = match self.turn { - Some(n) => TurnSelection::Index(n), - None => match self.last { - Some(n) => TurnSelection::Last(n), - None => TurnSelection::All, - }, - }; - for handle in handles { Self::print_conversation( ctx, handle, - &selection, + &self.range, self.current_config, self.style, self.compacted, @@ -140,7 +133,7 @@ impl Print { fn print_conversation( ctx: &mut Ctx, handle: &ConversationHandle, - selection: &TurnSelection, + range: &TurnRange, current_config: bool, print_style: Option, compacted: bool, @@ -197,30 +190,45 @@ impl Print { ); renderer.set_user_only(user_only); - let mut turns = events.iter_turns(); - let count = turns.len(); + let count = events.turn_count(); + + // `--last 0` explicitly selects nothing. + if range.is_empty() { + renderer.flush(); + return Ok(()); + } - match selection { - TurnSelection::All => { - for turn in turns { - renderer.render_turn(&turn); - } + // `--turn` names specific turns; an out-of-range endpoint is an error. + if let Some(n) = range.turn_out_of_range(count) { + return Err(format!("turn {n} out of range (conversation has {count} turns)").into()); + } + + let from = match range.resolve_from(&events) { + Bound::Empty => { + renderer.flush(); + return Ok(()); } - TurnSelection::Last(n) => { - let skip = count.saturating_sub(*n); - for turn in turns.skip(skip) { - renderer.render_turn(&turn); - } + Bound::Default => None, + Bound::At(b) => Some(b), + }; + let to = match range.resolve_to(&events) { + Bound::Empty => { + renderer.flush(); + return Ok(()); } - TurnSelection::Index(n) => { - if *n == 0 || *n > count { - return Err( - format!("turn {n} out of range (conversation has {count} turns)").into(), - ); - } - if let Some(turn) = turns.nth(n - 1) { - renderer.render_turn(&turn); - } + Bound::Default => None, + Bound::At(b) => Some(b), + }; + + // A `from > to` or otherwise empty range selects nothing. + let Some(selected) = resolve_range(&events, from, to) else { + renderer.flush(); + return Ok(()); + }; + + for turn in events.iter_turns() { + if (selected.from_turn..=selected.to_turn).contains(&turn.index()) { + renderer.render_turn(&turn); } } @@ -248,16 +256,6 @@ fn apply_style_preset( } } -/// How to select which turns to print. -enum TurnSelection { - /// Print all turns. - All, - /// Print the last N turns. - Last(usize), - /// Print a specific turn by 1-based index. - Index(usize), -} - #[cfg(test)] #[path = "print_tests.rs"] mod tests; diff --git a/crates/jp_cli/src/cmd/conversation/print_tests.rs b/crates/jp_cli/src/cmd/conversation/print_tests.rs index c54c8a7d..55c069b4 100644 --- a/crates/jp_cli/src/cmd/conversation/print_tests.rs +++ b/crates/jp_cli/src/cmd/conversation/print_tests.rs @@ -84,8 +84,7 @@ fn prints_user_message() { let print = Print { target: PositionalIds::from_targets(vec![ConversationTarget::Id(id)]), - last: None, - turn: None, + range: TurnRange::from_last_turn(None, None), current_config: false, style: None, compacted: false, @@ -108,8 +107,7 @@ fn prints_assistant_message() { let print = Print { target: PositionalIds::from_targets(vec![ConversationTarget::Id(id)]), - last: None, - turn: None, + range: TurnRange::from_last_turn(None, None), current_config: false, style: None, compacted: false, @@ -138,8 +136,7 @@ fn prints_reasoning_full() { let print = Print { target: PositionalIds::from_targets(vec![ConversationTarget::Id(id)]), - last: None, - turn: None, + range: TurnRange::from_last_turn(None, None), current_config: false, style: None, compacted: false, @@ -169,8 +166,7 @@ fn hides_reasoning_when_hidden() { let print = Print { target: PositionalIds::from_targets(vec![ConversationTarget::Id(id)]), - last: None, - turn: None, + range: TurnRange::from_last_turn(None, None), current_config: false, style: None, compacted: false, @@ -202,8 +198,7 @@ fn truncates_reasoning() { let print = Print { target: PositionalIds::from_targets(vec![ConversationTarget::Id(id)]), - last: None, - turn: None, + range: TurnRange::from_last_turn(None, None), current_config: false, style: None, compacted: false, @@ -244,8 +239,7 @@ fn prints_tool_call_and_result() { let print = Print { target: PositionalIds::from_targets(vec![ConversationTarget::Id(id)]), - last: None, - turn: None, + range: TurnRange::from_last_turn(None, None), current_config: false, style: None, compacted: false, @@ -273,8 +267,7 @@ fn prints_structured_data() { let print = Print { target: PositionalIds::from_targets(vec![ConversationTarget::Id(id)]), - last: None, - turn: None, + range: TurnRange::from_last_turn(None, None), current_config: false, style: None, compacted: false, @@ -305,8 +298,7 @@ fn structured_fence_is_closed_at_end_of_replay() { let print = Print { target: PositionalIds::from_targets(vec![ConversationTarget::Id(id)]), - last: None, - turn: None, + range: TurnRange::from_last_turn(None, None), current_config: false, style: None, compacted: false, @@ -345,8 +337,7 @@ fn structured_response_followed_by_message_closes_fence_first() { let print = Print { target: PositionalIds::from_targets(vec![ConversationTarget::Id(id)]), - last: None, - turn: None, + range: TurnRange::from_last_turn(None, None), current_config: false, style: None, compacted: false, @@ -394,8 +385,7 @@ fn structured_to_message_in_same_turn_closes_fence_first() { let print = Print { target: PositionalIds::from_targets(vec![ConversationTarget::Id(id)]), - last: None, - turn: None, + range: TurnRange::from_last_turn(None, None), current_config: false, style: None, compacted: false, @@ -433,8 +423,7 @@ fn turn_separators_between_turns() { let print = Print { target: PositionalIds::from_targets(vec![ConversationTarget::Id(id)]), - last: None, - turn: None, + range: TurnRange::from_last_turn(None, None), current_config: false, style: None, compacted: false, @@ -462,8 +451,7 @@ fn turn_header_shows_turn_number_and_relative_time() { let print = Print { target: PositionalIds::from_targets(vec![ConversationTarget::Id(id)]), - last: None, - turn: None, + range: TurnRange::from_last_turn(None, None), current_config: false, style: None, compacted: false, @@ -518,8 +506,7 @@ fn turn_header_detail_on_assistant_first_turn() { let print = Print { target: PositionalIds::from_targets(vec![ConversationTarget::Id(id)]), - last: None, - turn: None, + range: TurnRange::from_last_turn(None, None), current_config: false, style: None, compacted: false, @@ -549,8 +536,7 @@ fn prints_conversation_by_id() { let print = Print { target: PositionalIds::from_targets(vec![ConversationTarget::Id(id)]), - last: None, - turn: None, + range: TurnRange::from_last_turn(None, None), current_config: false, style: None, compacted: false, @@ -573,8 +559,7 @@ fn empty_conversation_produces_no_content() { let print = Print { target: PositionalIds::from_targets(vec![ConversationTarget::Id(id)]), - last: None, - turn: None, + range: TurnRange::from_last_turn(None, None), current_config: false, style: None, compacted: false, @@ -626,8 +611,7 @@ fn full_conversation_round_trip() { let print = Print { target: PositionalIds::from_targets(vec![ConversationTarget::Id(id)]), - last: None, - turn: None, + range: TurnRange::from_last_turn(None, None), current_config: false, style: None, compacted: false, @@ -667,8 +651,7 @@ fn last_prints_only_last_turn() { let print = Print { target: PositionalIds::from_targets(vec![ConversationTarget::Id(id)]), - last: Some(1), - turn: None, + range: TurnRange::from_last_turn(Some(1), None), current_config: false, style: None, compacted: false, @@ -709,8 +692,7 @@ fn last_two_with_three_turns() { let print = Print { target: PositionalIds::from_targets(vec![ConversationTarget::Id(id)]), - last: Some(2), - turn: None, + range: TurnRange::from_last_turn(Some(2), None), current_config: false, style: None, compacted: false, @@ -739,8 +721,7 @@ fn last_exceeding_turn_count_prints_all() { let print = Print { target: PositionalIds::from_targets(vec![ConversationTarget::Id(id)]), - last: Some(5), - turn: None, + range: TurnRange::from_last_turn(Some(5), None), current_config: false, style: None, compacted: false, @@ -785,8 +766,7 @@ fn blank_line_between_tool_calls_and_message() { let print = Print { target: PositionalIds::from_targets(vec![ConversationTarget::Id(id)]), - last: None, - turn: None, + range: TurnRange::from_last_turn(None, None), current_config: false, style: None, compacted: false, @@ -839,8 +819,7 @@ fn blank_line_between_message_and_tool_calls() { let print = Print { target: PositionalIds::from_targets(vec![ConversationTarget::Id(id)]), - last: None, - turn: None, + range: TurnRange::from_last_turn(None, None), current_config: false, style: None, compacted: false, @@ -909,8 +888,7 @@ fn no_extra_blank_line_between_consecutive_tool_calls() { let print = Print { target: PositionalIds::from_targets(vec![ConversationTarget::Id(id)]), - last: None, - turn: None, + range: TurnRange::from_last_turn(None, None), current_config: false, style: None, compacted: false, @@ -947,8 +925,7 @@ fn last_zero_prints_nothing() { let print = Print { target: PositionalIds::from_targets(vec![ConversationTarget::Id(id)]), - last: Some(0), - turn: None, + range: TurnRange::from_last_turn(Some(0), None), current_config: false, style: None, compacted: false, @@ -983,8 +960,7 @@ fn turn_prints_specific_turn() { // Print only turn 2. let print = Print { target: PositionalIds::from_targets(vec![ConversationTarget::Id(id)]), - last: None, - turn: Some(2), + range: TurnRange::from_last_turn(None, Some(2)), current_config: false, style: None, compacted: false, @@ -1022,8 +998,7 @@ fn turn_out_of_range_errors() { let print = Print { target: PositionalIds::from_targets(vec![ConversationTarget::Id(id)]), - last: None, - turn: Some(5), + range: TurnRange::from_last_turn(None, Some(5)), current_config: false, style: None, compacted: false, @@ -1042,8 +1017,7 @@ fn turn_zero_errors() { let print = Print { target: PositionalIds::from_targets(vec![ConversationTarget::Id(id)]), - last: None, - turn: Some(0), + range: TurnRange::from_last_turn(None, Some(0)), current_config: false, style: None, compacted: false, @@ -1088,8 +1062,7 @@ fn style_brief_hides_reasoning_and_tool_details() { let print = Print { target: PositionalIds::from_targets(vec![ConversationTarget::Id(id)]), - last: None, - turn: None, + range: TurnRange::from_last_turn(None, None), current_config: false, style: Some(PrintStyle::Brief), compacted: false, @@ -1166,8 +1139,7 @@ fn style_chat_hides_reasoning_and_tool_calls() { let print = Print { target: PositionalIds::from_targets(vec![ConversationTarget::Id(id)]), - last: None, - turn: None, + range: TurnRange::from_last_turn(None, None), current_config: false, style: Some(PrintStyle::Chat), compacted: false, @@ -1246,8 +1218,7 @@ fn style_user_shows_only_user_messages() { let print = Print { target: PositionalIds::from_targets(vec![ConversationTarget::Id(id)]), - last: None, - turn: None, + range: TurnRange::from_last_turn(None, None), current_config: false, style: Some(PrintStyle::User), compacted: false, @@ -1292,8 +1263,7 @@ fn role_header_renders_user_label_from_author() { let print = Print { target: PositionalIds::from_targets(vec![ConversationTarget::Id(id)]), - last: None, - turn: None, + range: TurnRange::from_last_turn(None, None), current_config: false, style: None, compacted: false, @@ -1318,8 +1288,7 @@ fn role_header_falls_back_to_user_label_without_author() { let print = Print { target: PositionalIds::from_targets(vec![ConversationTarget::Id(id)]), - last: None, - turn: None, + range: TurnRange::from_last_turn(None, None), current_config: false, style: None, compacted: false, @@ -1344,8 +1313,7 @@ fn role_header_renders_assistant_label_with_model_suffix() { let print = Print { target: PositionalIds::from_targets(vec![ConversationTarget::Id(id)]), - last: None, - turn: None, + range: TurnRange::from_last_turn(None, None), current_config: false, style: None, compacted: false, @@ -1371,8 +1339,7 @@ fn role_header_assistant_appears_once_per_turn() { let print = Print { target: PositionalIds::from_targets(vec![ConversationTarget::Id(id)]), - last: None, - turn: None, + range: TurnRange::from_last_turn(None, None), current_config: false, style: None, compacted: false, @@ -1414,8 +1381,7 @@ fn role_header_assistant_emitted_before_first_tool_call() { let print = Print { target: PositionalIds::from_targets(vec![ConversationTarget::Id(id)]), - last: None, - turn: None, + range: TurnRange::from_last_turn(None, None), current_config: false, style: None, compacted: false, @@ -1446,8 +1412,7 @@ fn role_header_does_not_emit_plain_hr_separator() { let print = Print { target: PositionalIds::from_targets(vec![ConversationTarget::Id(id)]), - last: None, - turn: None, + range: TurnRange::from_last_turn(None, None), current_config: false, style: None, compacted: false, @@ -1497,8 +1462,7 @@ fn style_chat_separates_messages_across_hidden_reasoning() { let print = Print { target: PositionalIds::from_targets(vec![ConversationTarget::Id(id)]), - last: None, - turn: None, + range: TurnRange::from_last_turn(None, None), current_config: false, style: Some(PrintStyle::Chat), compacted: false, @@ -1549,8 +1513,7 @@ fn style_chat_separates_messages_across_hidden_tool_call() { let print = Print { target: PositionalIds::from_targets(vec![ConversationTarget::Id(id)]), - last: None, - turn: None, + range: TurnRange::from_last_turn(None, None), current_config: false, style: Some(PrintStyle::Chat), compacted: false, @@ -1611,8 +1574,7 @@ fn style_full_shows_reasoning_and_untruncated_results() { let print = Print { target: PositionalIds::from_targets(vec![ConversationTarget::Id(id)]), - last: None, - turn: None, + range: TurnRange::from_last_turn(None, None), current_config: false, style: Some(PrintStyle::Full), compacted: false, diff --git a/crates/jp_cli/src/cmd/query.rs b/crates/jp_cli/src/cmd/query.rs index 7adf80f4..a9eff8d6 100644 --- a/crates/jp_cli/src/cmd/query.rs +++ b/crates/jp_cli/src/cmd/query.rs @@ -937,8 +937,8 @@ impl Query { &events, cfg, &rules, - super::conversation::compact::Bound::Default, - super::conversation::compact::Bound::Default, + crate::cmd::turn_range::Bound::Default, + crate::cmd::turn_range::Bound::Default, // `--compact` on a query is a quick adjunct; apply it silently so // compaction details don't clutter the query output. None, diff --git a/crates/jp_cli/src/cmd/turn_range.rs b/crates/jp_cli/src/cmd/turn_range.rs new file mode 100644 index 00000000..2d192971 --- /dev/null +++ b/crates/jp_cli/src/cmd/turn_range.rs @@ -0,0 +1,318 @@ +//! Shared turn-range selector for `print` and `compact`. +//! +//! Both commands need to name a subset of turns. +//! This module owns that selector — the `--last`/`--turn`/`--from`/`--to` +//! flags — and the parsing and stream resolution behind them, so a range built +//! for one command means the same thing in the other. +//! +//! Turn positions are 1-based on the CLI and 0-based in the stream; the +//! translation happens here. +//! See `docs/architecture/indexing-conventions.md`. + +use std::{str::FromStr as _, time::Duration}; + +use chrono::{DateTime, Utc}; +use jp_conversation::{ConversationStream, RangeBound}; + +/// A `--from`/`--to` bound before time-based resolution. +#[derive(Debug, Clone)] +pub(crate) enum CliRangeBound { + /// Already resolved to a `RangeBound` (0-based for the core). + Resolved(RangeBound), + /// Duration ago — needs the stream to find the turn. + Duration(DateTime), +} + +/// Whether `s` is the most-recent-compaction marker. +/// +/// `last-compaction` is canonical; `last` is accepted as a deprecated alias. +fn is_last_compaction(s: &str) -> bool { + s.eq_ignore_ascii_case("last-compaction") || s.eq_ignore_ascii_case("last") +} + +/// Parse a `--from`/`--to` bound. +/// +/// Accepts a 1-based turn index, `-N` (offset from the end, `-1` is the last +/// turn), a duration (`5h`), or `last-compaction` (after the most recent +/// compaction; `last` is accepted as a deprecated alias). +pub(crate) fn parse_bound(s: &str) -> Result { + if is_last_compaction(s) { + return Ok(CliRangeBound::Resolved(RangeBound::AfterLastCompaction)); + } + + // From-end offset. `-1` is the last turn, so `-N` maps to `FromEnd(N - 1)`. + if let Some(rest) = s.strip_prefix('-') + && let Ok(n) = rest.parse::() + { + if n == 0 { + return Err("from-end offsets are 1-based; use `-1` for the last turn".to_owned()); + } + return Ok(CliRangeBound::Resolved(RangeBound::FromEnd(n - 1))); + } + + // 1-based user index → 0-based core index. + if let Ok(n) = s.parse::() { + if n == 0 { + return Err("turn numbers are 1-based; `0` is not a valid turn".to_owned()); + } + return Ok(CliRangeBound::Resolved(RangeBound::Absolute(n - 1))); + } + + humantime::Duration::from_str(s) + .map(|d| CliRangeBound::Duration(Utc::now() - Duration::from(d))) + .map_err(|e| format!("invalid range bound `{s}`: {e}")) +} + +/// Like [`parse_bound`], but rejects the `last-compaction` marker. +/// +/// `last-compaction` (the most recent compaction) is only meaningful as a start +/// bound (`--from last-compaction`), so it is not accepted for `--to`. +fn parse_to_bound(s: &str) -> Result { + if is_last_compaction(s) { + return Err( + "`last-compaction` is only valid for `--from` (it marks the most recent compaction)" + .to_owned(), + ); + } + parse_bound(s) +} + +/// Parse a 1-based turn number for `--turn`, rejecting `0`. +fn parse_one_based(s: &str) -> Result { + match s.parse::() { + Ok(0) => Err("turn numbers are 1-based; `0` is not a valid turn".to_owned()), + Ok(n) => Ok(n), + Err(_) => Err(format!("invalid turn number `{s}`")), + } +} + +/// A `--turn` value: a single 1-based turn, or an inclusive 1-based range. +/// +/// Either end of a range may be open: `10..` is turn 10 through the end, `..10` +/// is the first 10 turns, and `..` is the whole conversation. +#[derive(Debug, Clone)] +pub(crate) enum TurnSpec { + /// A single turn. + Single(usize), + /// An inclusive range `from..to`. + /// `None` on either side is open (the start or end of the conversation). + Range(Option, Option), +} + +/// Parse a `--turn` value: `N` (a single turn) or a range `A..B`. +/// +/// The separator is `..` and both ends are inclusive, matching the compaction +/// DSL (`1..5` is turns 1 through 5). +/// Either end may be omitted: `10..`, `..10`, or `..` (all turns). +fn parse_turn(s: &str) -> Result { + if let Some((a, b)) = s.split_once("..") { + let from = if a.is_empty() { + None + } else { + Some(parse_one_based(a)?) + }; + let to = if b.is_empty() { + None + } else { + Some(parse_one_based(b)?) + }; + return Ok(TurnSpec::Range(from, to)); + } + Ok(TurnSpec::Single(parse_one_based(s)?)) +} + +/// The resolution of one range bound (`from` or `to`) against a stream. +/// +/// Separates "no bound configured for this side" (use the side's default) from +/// "this bound selects no turns" (so the whole selection is empty), which a +/// plain `Option` conflates. +#[derive(Debug, Clone)] +pub(crate) enum Bound { + /// No bound configured; the range defaults to the start (`from`) or end + /// (`to`) of the conversation. + Default, + /// The bound resolves to a concrete `RangeBound`. + At(RangeBound), + /// The bound falls outside the conversation such that nothing is selected. + Empty, +} + +/// Resolve a `--from ` cutoff: the range starts at the first turn after +/// the cutoff. +/// +/// No turn after the cutoff (an old conversation) means an empty selection; a +/// cutoff before the conversation starts selects from the beginning. +pub(crate) fn resolve_cli_from(bound: &CliRangeBound, events: &ConversationStream) -> Bound { + match bound { + CliRangeBound::Resolved(b) => Bound::At(b.clone()), + CliRangeBound::Duration(dt) => match events.turn_at_time(*dt) { + Some(turn) => { + let from = turn.index() + 1; + if from >= events.turn_count() { + Bound::Empty + } else { + Bound::At(RangeBound::Absolute(from)) + } + } + None => Bound::At(RangeBound::Absolute(0)), + }, + } +} + +/// Resolve a `--to ` cutoff: the range stops at (and includes) the turn +/// active at the cutoff. +/// +/// A cutoff preceding the conversation means an empty selection. +pub(crate) fn resolve_cli_to(bound: &CliRangeBound, events: &ConversationStream) -> Bound { + match bound { + CliRangeBound::Resolved(b) => Bound::At(b.clone()), + CliRangeBound::Duration(dt) => match events.turn_at_time(*dt) { + Some(turn) => Bound::At(RangeBound::Absolute(turn.index())), + None => Bound::Empty, + }, + } +} + +/// A positive turn-range selector shared by `print` and `compact`. +/// +/// Names the turns a command acts on. +/// The selectors are mutually constrained so only one way of expressing the +/// range is given at a time. +#[derive(Debug, Clone, Default, clap::Args)] +pub(crate) struct TurnRange { + /// Select the first N turns. + /// Without a value, selects the first turn. + #[arg(long, num_args = 0..=1, default_missing_value = "1", conflicts_with_all = ["last", "turn", "from", "to"])] + first: Option, + + /// Select the last N turns. + /// Without a value, selects the last turn. + #[arg(long, num_args = 0..=1, default_missing_value = "1", conflicts_with_all = ["first", "turn", "from", "to"])] + last: Option, + + /// Select turns by number (1-based): a single turn (`3`), an inclusive + /// range (`1..5`), or an open range like `10..` (turn 10 onward), `..10` + /// (the first 10), or `..` (all). + /// Stable across new turns. + #[arg(long, value_parser = parse_turn, conflicts_with_all = ["first", "last", "from", "to"])] + turn: Option, + + /// Start of the range: a 1-based turn index, `-N` from the end, a duration + /// (e.g. + /// `5h`), or `last-compaction` (after the most recent compaction). + #[arg(long, value_parser = parse_bound)] + from: Option, + + /// End of the range: a 1-based turn index, `-N` from the end, or a duration + /// (e.g. + /// `2h`). + #[arg(long, value_parser = parse_to_bound)] + to: Option, +} + +impl TurnRange { + /// Build a selector from explicit `--last`/`--turn` values. + #[cfg(test)] + pub(crate) fn from_last_turn(last: Option, turn: Option) -> Self { + Self { + first: None, + last, + turn: turn.map(TurnSpec::Single), + from: None, + to: None, + } + } + + /// The first `--turn` endpoint outside `1..=count`, if any. + /// + /// `--turn` names specific turns, so an endpoint past the conversation is + /// an error rather than a clamped selection (unlike `--first`/`--last`). + pub(crate) fn turn_out_of_range(&self, count: usize) -> Option { + let oob = |n: usize| n == 0 || n > count; + let ends = match self.turn.as_ref()? { + TurnSpec::Single(n) => [Some(*n), None], + TurnSpec::Range(a, b) => [*a, *b], + }; + ends.into_iter().flatten().find(|&n| oob(n)) + } + + /// Whether the selector explicitly names an empty range (`--last 0` or + /// `--first 0`). + pub(crate) fn is_empty(&self) -> bool { + self.last == Some(0) || self.first == Some(0) + } + + /// The `from` bound as a CLI bound, folding the positive selectors. + /// + /// `--first`/`--last`/`--turn` are complete selectors that set both bounds; + /// `--from` is the explicit start of a range. + fn cli_from_bound(&self) -> Option { + if let Some(spec) = &self.turn { + let bound = match spec { + TurnSpec::Single(n) => RangeBound::Absolute(n.saturating_sub(1)), + // Open start (`--turn ..B`) begins at the first turn. + TurnSpec::Range(Some(a), _) => RangeBound::Absolute(a.saturating_sub(1)), + TurnSpec::Range(None, _) => RangeBound::Absolute(0), + }; + return Some(CliRangeBound::Resolved(bound)); + } + if let Some(n) = self.last { + return Some(CliRangeBound::Resolved(RangeBound::FromEnd( + n.saturating_sub(1), + ))); + } + if self.first.is_some() { + // `--first N` starts at the first turn. + return Some(CliRangeBound::Resolved(RangeBound::Absolute(0))); + } + self.from.clone() + } + + /// The `to` bound as a CLI bound, folding the positive selectors. + /// + /// `--first`/`--last`/`--turn` are complete selectors that set both bounds; + /// `--to` is the explicit end of a range. + fn cli_to_bound(&self) -> Option { + if let Some(spec) = &self.turn { + let bound = match spec { + TurnSpec::Single(n) => RangeBound::Absolute(n.saturating_sub(1)), + // Open end (`--turn A..`) runs through the last turn. + TurnSpec::Range(_, Some(b)) => RangeBound::Absolute(b.saturating_sub(1)), + TurnSpec::Range(_, None) => RangeBound::FromEnd(0), + }; + return Some(CliRangeBound::Resolved(bound)); + } + if self.last.is_some() { + // `--last N` ends at the last turn. + return Some(CliRangeBound::Resolved(RangeBound::FromEnd(0))); + } + if let Some(n) = self.first { + return Some(CliRangeBound::Resolved(RangeBound::Absolute( + n.saturating_sub(1), + ))); + } + self.to.clone() + } + + /// Resolve the `from` override against the stream. + /// Returns [`Bound::Default`] when no `from`-affecting flag is set. + pub(crate) fn resolve_from(&self, events: &ConversationStream) -> Bound { + match self.cli_from_bound() { + Some(bound) => resolve_cli_from(&bound, events), + None => Bound::Default, + } + } + + /// Resolve the `to` override against the stream. + /// Returns [`Bound::Default`] when no `to`-affecting flag is set. + pub(crate) fn resolve_to(&self, events: &ConversationStream) -> Bound { + match self.cli_to_bound() { + Some(bound) => resolve_cli_to(&bound, events), + None => Bound::Default, + } + } +} + +#[cfg(test)] +#[path = "turn_range_tests.rs"] +mod tests; diff --git a/crates/jp_cli/src/cmd/turn_range_tests.rs b/crates/jp_cli/src/cmd/turn_range_tests.rs new file mode 100644 index 00000000..dcdaab1e --- /dev/null +++ b/crates/jp_cli/src/cmd/turn_range_tests.rs @@ -0,0 +1,172 @@ +use jp_conversation::RangeBound; + +use super::*; + +#[test] +fn parse_bound_absolute_is_one_based() { + // `1` is the first turn (0-based `Absolute(0)` internally). + assert!(matches!( + parse_bound("1").unwrap(), + CliRangeBound::Resolved(RangeBound::Absolute(0)) + )); + assert!(matches!( + parse_bound("5").unwrap(), + CliRangeBound::Resolved(RangeBound::Absolute(4)) + )); +} + +#[test] +fn parse_bound_from_end_is_one_based() { + // `-1` is the last turn (0-based `FromEnd(0)` internally). + assert!(matches!( + parse_bound("-1").unwrap(), + CliRangeBound::Resolved(RangeBound::FromEnd(0)) + )); + assert!(matches!( + parse_bound("-3").unwrap(), + CliRangeBound::Resolved(RangeBound::FromEnd(2)) + )); +} + +#[test] +fn parse_bound_rejects_zero() { + // `0` is not a valid 1-based turn, on either end. + assert!(parse_bound("0").is_err()); + assert!(parse_bound("-0").is_err()); +} + +#[test] +fn parse_bound_accepts_last_compaction_and_alias() { + // `last-compaction` is canonical; `last` is a deprecated alias. + for s in ["last-compaction", "last"] { + assert!( + matches!( + parse_bound(s).unwrap(), + CliRangeBound::Resolved(RangeBound::AfterLastCompaction) + ), + "`{s}` should parse as the last-compaction marker" + ); + } +} + +#[test] +fn parse_to_bound_rejects_last_compaction_but_accepts_indices() { + // The most-recent-compaction marker is start-only, so `--to` rejects it + // (canonical name and alias). + assert!(parse_to_bound("last-compaction").is_err()); + assert!(parse_to_bound("last").is_err()); + assert!(parse_to_bound("3").is_ok()); + assert!(parse_to_bound("-1").is_ok()); +} + +#[test] +fn first_and_last_are_complete_selectors() { + // `--first N` is the first N turns: start of conversation through turn N. + let first = TurnRange { + first: Some(3), + ..Default::default() + }; + assert!(matches!( + first.cli_from_bound(), + Some(CliRangeBound::Resolved(RangeBound::Absolute(0))) + )); + assert!(matches!( + first.cli_to_bound(), + Some(CliRangeBound::Resolved(RangeBound::Absolute(2))) + )); + + // `--last N` is the last N turns: N-from-the-end through the last turn. + let last = TurnRange { + last: Some(3), + ..Default::default() + }; + assert!(matches!( + last.cli_from_bound(), + Some(CliRangeBound::Resolved(RangeBound::FromEnd(2))) + )); + assert!(matches!( + last.cli_to_bound(), + Some(CliRangeBound::Resolved(RangeBound::FromEnd(0))) + )); +} + +#[test] +fn first_zero_is_an_empty_selection() { + assert!( + TurnRange { + first: Some(0), + ..Default::default() + } + .is_empty() + ); +} + +#[test] +fn parse_turn_single_and_range() { + assert!(matches!(parse_turn("3").unwrap(), TurnSpec::Single(3))); + assert!(matches!( + parse_turn("1..5").unwrap(), + TurnSpec::Range(Some(1), Some(5)) + )); + + // Open-ended ranges. + assert!(matches!( + parse_turn("10..").unwrap(), + TurnSpec::Range(Some(10), None) + )); + assert!(matches!( + parse_turn("..10").unwrap(), + TurnSpec::Range(None, Some(10)) + )); + assert!(matches!( + parse_turn("..").unwrap(), + TurnSpec::Range(None, None) + )); + + // 1-based: `0` is rejected wherever a number appears. + assert!(parse_turn("0").is_err()); + assert!(parse_turn("0..5").is_err()); + assert!(parse_turn("1..0").is_err()); + + // The separator is `..`, not `..=`. + assert!(parse_turn("1..=5").is_err()); +} + +#[test] +fn turn_open_ended_ranges_set_both_bounds() { + // `--turn 10..` is turn 10 through the last turn. + let onward = TurnRange { + turn: Some(TurnSpec::Range(Some(10), None)), + ..Default::default() + }; + assert!(matches!( + onward.cli_from_bound(), + Some(CliRangeBound::Resolved(RangeBound::Absolute(9))) + )); + assert!(matches!( + onward.cli_to_bound(), + Some(CliRangeBound::Resolved(RangeBound::FromEnd(0))) + )); + + // `--turn ..` is the whole conversation. + let all = TurnRange { + turn: Some(TurnSpec::Range(None, None)), + ..Default::default() + }; + assert!(matches!( + all.cli_from_bound(), + Some(CliRangeBound::Resolved(RangeBound::Absolute(0))) + )); + assert!(matches!( + all.cli_to_bound(), + Some(CliRangeBound::Resolved(RangeBound::FromEnd(0))) + )); +} + +#[test] +fn parse_one_based_rejects_zero() { + assert_eq!(parse_one_based("1").unwrap(), 1); + assert_eq!(parse_one_based("7").unwrap(), 7); + assert!(parse_one_based("0").is_err()); + assert!(parse_one_based("x").is_err()); +} diff --git a/crates/jp_config/src/conversation/compaction.rs b/crates/jp_config/src/conversation/compaction.rs index 88ad9ed4..7635cb62 100644 --- a/crates/jp_config/src/conversation/compaction.rs +++ b/crates/jp_config/src/conversation/compaction.rs @@ -359,7 +359,7 @@ impl ToPartial for SummaryConfig { pub enum RuleBound { /// A number of turns to preserve (relative; the stable config form). Turns(usize), - /// An absolute, 0-based turn index. + /// An absolute, 1-based turn index (the first turn is `1`). /// Written `@N` as a string. Absolute(usize), /// An offset from the end (`FromEnd(3)` = three turns before the last). @@ -368,7 +368,7 @@ pub enum RuleBound { /// Preserve turns within this duration, e.g. `"5h"`, `"2days"`. Duration(std::time::Duration), /// Start after the most recent compaction's `to_turn`. - /// Only meaningful for `keep_first` (used via `from = "last"`). + /// Only meaningful for `keep_first` (used via `from = "last-compaction"`). AfterLastCompaction, } @@ -376,15 +376,19 @@ impl FromStr for RuleBound { type Err = BoxedError; fn from_str(s: &str) -> Result { - if s.eq_ignore_ascii_case("last") { + // `last-compaction` is canonical; `last` is a deprecated alias. + if s.eq_ignore_ascii_case("last-compaction") || s.eq_ignore_ascii_case("last") { return Ok(Self::AfterLastCompaction); } if let Some(rest) = s.strip_prefix('@') { - return rest + let n: usize = rest .parse() - .map(Self::Absolute) - .map_err(|_| format!("invalid absolute turn `{s}`").into()); + .map_err(|_| format!("invalid absolute turn `{s}`"))?; + if n == 0 { + return Err("absolute turns are 1-based; `@0` is not a valid turn".into()); + } + return Ok(Self::Absolute(n)); } if let Some(rest) = s.strip_prefix('-') { @@ -411,7 +415,7 @@ impl fmt::Display for RuleBound { Self::Absolute(n) => write!(f, "@{n}"), Self::FromEnd(n) => write!(f, "-{n}"), Self::Duration(d) => write!(f, "{}", humantime::format_duration(*d)), - Self::AfterLastCompaction => write!(f, "last"), + Self::AfterLastCompaction => write!(f, "last-compaction"), } } } @@ -430,7 +434,8 @@ impl<'de> Deserialize<'de> for RuleBound { type Value = RuleBound; fn expecting(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result { - formatter.write_str("a turn count, a duration string like `5h`, or `last`") + formatter + .write_str("a turn count, a duration string like `5h`, or `last-compaction`") } fn visit_u64(self, v: u64) -> Result { diff --git a/crates/jp_config/src/conversation/compaction_tests.rs b/crates/jp_config/src/conversation/compaction_tests.rs index b8b75d07..005e7ba1 100644 --- a/crates/jp_config/src/conversation/compaction_tests.rs +++ b/crates/jp_config/src/conversation/compaction_tests.rs @@ -78,6 +78,33 @@ fn rule_bound_deserializes_from_integer_and_string() { )); } +#[test] +fn rule_bound_after_last_compaction_is_canonical_with_alias() { + // `last-compaction` is canonical; `last` is a deprecated input alias. + // Serialization always emits the canonical form. + assert_eq!( + "last-compaction".parse::().unwrap(), + RuleBound::AfterLastCompaction + ); + assert_eq!( + "last".parse::().unwrap(), + RuleBound::AfterLastCompaction + ); + assert_eq!( + RuleBound::AfterLastCompaction.to_string(), + "last-compaction" + ); +} + +#[test] +fn rule_bound_absolute_is_one_based_and_rejects_zero() { + // `@N` is a 1-based absolute position; `@0` is invalid input, not an alias + // for the first turn. + assert_eq!("@1".parse::().unwrap(), RuleBound::Absolute(1)); + assert_eq!("@5".parse::().unwrap(), RuleBound::Absolute(5)); + assert!("@0".parse::().is_err()); +} + #[test] fn rule_config_deserializes_integer_bounds() { // Two distinct explicit bounds, exercising integer deserialization. diff --git a/docs/.vitepress/rfd-summaries.json b/docs/.vitepress/rfd-summaries.json index 4002f1b6..3f3bfb44 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": "3cd56aee44cb9becf11a1e62f95710266656a93fae1e13b803316887f0297b70", + "hash": "bfe9b3d6110c000021220b53073d836ed3c9e663fbb10b5e41137f56b7a41bd4", "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/index.md b/docs/architecture/index.md index 05d54007..311a60e5 100644 --- a/docs/architecture/index.md +++ b/docs/architecture/index.md @@ -32,6 +32,11 @@ This section describes the technical architecture of JP. Defines the core terms (Workspace, Conversation, Turn, Thread, Attachment, Inquiry, Provider, Backend, etc.) and the distinctions between them. +- [Indexing and Counting Conventions] - How turn positions are numbered: 1-based + on the user-facing side (CLI and config), 0-based for stored and internal + state, translated once at the boundary. + +[Indexing and Counting Conventions]: indexing-conventions.md [JP Query Architecture]: architecture.md [Knowledge Base]: knowledge-base.md [Query Stream Pipeline]: query-stream-pipeline.md diff --git a/docs/architecture/indexing-conventions.md b/docs/architecture/indexing-conventions.md new file mode 100644 index 00000000..855b6cf6 --- /dev/null +++ b/docs/architecture/indexing-conventions.md @@ -0,0 +1,91 @@ +# Indexing and Counting Conventions + +JP exposes turn positions and counts in two places: the CLI (flags, arguments) +and configuration (config files, `--cfg`, the inline compaction DSL). +Internally those same positions are stored as zero-based indices in the +conversation stream. +This document fixes the convention so the translation between the two sides is +consistent and happens in exactly one place per boundary. + +## The rule + +- **User-facing positions are 1-based.** The first turn is turn `1`. + This holds for every CLI flag and configuration value that names a turn + position. + +- **Stored and internal positions are 0-based.** `Compaction.from_turn`, + `Compaction.to_turn`, `Turn::index()`, and `RangeBound::Absolute` are all + 0-based and never change. + The conversation stream is the source of truth and it counts from zero. + +- **Translate at the boundary, once.** A 1-based user value becomes a 0-based + index at the point where user input is resolved against the stream, and a + 0-based index becomes a 1-based display value at the point where it is + rendered. + Nothing in between carries an ambiguous "is this 0- or 1-based?" value. + +The boundary is the `jp_cli` resolution layer. +`jp_config` carries user values (1-based), `jp_conversation` carries core values +(0-based), and `jp_cli` translates between them. + +## Positions vs. counts + +Only *positions* (indices into the conversation) are subject to the 1-based +rule. +A *count* — "how many turns" — is base-independent and is never shifted. + +| Kind | Examples | Translated? | +| ------------------- | ------------------------------------------------------------------------------------------------------- | ---------------------------------------------------- | +| Position (absolute) | `--turn N`, `--from N`, `--to N`, DSL `N..M`, config `keep_first = "@N"` | yes, `N` (1-based) → `N - 1` (0-based) | +| Position (from end) | `--from -N`, `--to -N` | yes, `-1` is the last turn (`-N` → `FromEnd(N - 1)`) | +| Count | `--first N`, `--last N`, `--keep-first N`, `--keep-last N`, config `keep_first = N`, DSL `-N` shorthand | no | +| Duration | `5h`, `2days` | n/a (resolved against timestamps) | + +Two consequences worth calling out: + +- `--from -1` and `--to -1` address the **last** turn, matching the 1-based + reading where `1` is the first turn and `-1` is the last. + As a result `--from -N` selects the same starting turn as `--last N`. + +- The compaction DSL's `-N` (e.g. + `..-3`, "keep the last 3 turns") is a *count*, not a position. + It is unaffected by the 1-based rule and keeps its Python-slice "keep last N" + meaning. + Only the DSL's absolute bounds (`5..`) are positions and are 1-based. + +- Ranges are written `A..B` and are **inclusive on both ends** — `1..5` is + turns 1 through 5 (five turns). + This one format is shared by `--turn A..B`, the compaction DSL, and the + timeline output. + Either end may be omitted to mean the conversation start or end (`10..`, + `..10`, `..`). + Note this diverges from Rust's `..` (which is exclusive); there is no `..=` + form. + +## Where the translation lives + +- **CLI `--from`/`--to`/`--first`/`--last`/`--turn`** + (`jp_cli::cmd::turn_range`): `parse_bound` maps a 1-based absolute `N` to + `RangeBound::Absolute(N - 1)` and a from-end `-N` to `RangeBound::FromEnd(N - + 1)`. + `--first`/`--last`/`--turn` are complete selectors that set both bounds: + `--turn N` is `Absolute(N - 1)` on both ends (`--turn A..B` spans \`Absolute(A + + - 1\)`through`Absolute(B - 1)` ), `--first + N`is`Absolute(0)`through`Absolute(N - 1)` , and `--last N`is`FromEnd(N - + 1)\` through the last turn. + +- **Config `keep_first`/`keep_last` and the inline DSL** + (`jp_cli::cmd::conversation::compact`): `keep_first_to_bound` / + `keep_last_to_bound` map `RuleBound::Absolute(N)` (the 1-based value parsed + from `@N`) to `RangeBound::Absolute(N - 1)`. + The `RuleBound::Turns(N)` (count) arm is untouched. + +- **Timeline output** (`jp_cli::cmd::conversation::compact::timeline_lines`): + the stored 0-based `from_turn`/`to_turn` are rendered as 1-based, e.g. + `Compacted turns 2..8`. + +When adding a new flag, config key, or output that names a turn, decide first +whether it is a position or a count. +If it is a position, it is 1-based on the user side and translated to 0-based +exactly where it meets the stream. diff --git a/docs/rfd/064-non-destructive-conversation-compaction.md b/docs/rfd/064-non-destructive-conversation-compaction.md index 8de5f93e..5ecc2943 100644 --- a/docs/rfd/064-non-destructive-conversation-compaction.md +++ b/docs/rfd/064-non-destructive-conversation-compaction.md @@ -135,10 +135,21 @@ Range bounds accept several formats: | `last` | `--from last` | Turn of the most recent compaction | | | | event, or start if none. | -`--from` requires a value (use `--from last` for the most recent compaction). +`--from` requires a value (use `--from last-compaction` for the most recent +compaction). All bounds are **resolved to absolute turn indices at creation time** and stored as integers. +> [!NOTE] +> Turn positions on the CLI are now **1-based**: `--from 1` is the first turn +> and `--to -1` is the last. +> The stored indices remain 0-based. +> The two `5`/`-3` examples above describe the original 0-based behavior; see +> [Indexing and Counting Conventions] for the current rule. +> +> The `last` keyword is now spelled `last-compaction` (with `last` kept as a +> deprecated alias) and is accepted only for `--from`. + `--reset` removes all `InternalEvent::Compaction` variants from the stream, restoring the raw event history. The projection layer then has nothing to apply, and the LLM sees the original diff --git a/justfile b/justfile index 4b0bd322..758aa3d0 100644 --- a/justfile +++ b/justfile @@ -64,7 +64,7 @@ run *ARGS: # Install the `jp` binary from your local checkout. [group('build')] [group('main')] -install: +install $JP_NO_INSTALL="": @just quiet_flag="" _install-jp [group('jp')]